From 14c864f5c6827a1d22b11d1a8956212d047f0653 Mon Sep 17 00:00:00 2001 From: agent_coder Date: Sat, 11 Jul 2026 02:29:41 +0300 Subject: [PATCH] =?UTF-8?q?fix(health):=20=D1=83=D1=81=D1=82=D1=80=D0=B0?= =?UTF-8?q?=D0=BD=D0=B8=D1=82=D1=8C=20=D1=83=D1=82=D0=B5=D1=87=D0=BA=D1=83?= =?UTF-8?q?=20ioredis-=D0=BA=D0=BB=D0=B8=D0=B5=D0=BD=D1=82=D0=B0=20=D0=B2?= =?UTF-8?q?=20/health-=D0=BF=D1=80=D0=BE=D0=B1=D0=B5=20(#486)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit pingCheck строил new Redis(...) на КАЖДЫЙ вызов и делал disconnect() только на success-пути. Пока Redis лежит, каждый тик health-пробы добавлял свежий, вечно реконнектящийся клиент — неограниченный рост хэндлов/клиентов на всё время недоступности Redis. Теперь один долгоживущий probe-клиент, переиспользуемый между тиками: lazyConnect (конструктор не бросает и не коннектится жадно), maxRetriesPerRequest: 1 и enableOfflineQueue: false (проба фейлится быстро, команды не буферизуются), плюс listener на 'error' (иначе unhandled error роняет процесс). onModuleDestroy закрывает клиент при shutdown. Тест: интеграционный — N проб при лежащем Redis (реальный refused-порт, не мок поведения) создают РОВНО ОДИН клиент (на баге было бы N); onModuleDestroy освобождает клиент, следующая проба лениво строит новый. --- .../integrations/health/redis.health.spec.ts | 95 +++++++++++++++++++ .../src/integrations/health/redis.health.ts | 59 ++++++++++-- 2 files changed, 147 insertions(+), 7 deletions(-) create mode 100644 apps/server/src/integrations/health/redis.health.spec.ts diff --git a/apps/server/src/integrations/health/redis.health.spec.ts b/apps/server/src/integrations/health/redis.health.spec.ts new file mode 100644 index 00000000..1fd59c26 --- /dev/null +++ b/apps/server/src/integrations/health/redis.health.spec.ts @@ -0,0 +1,95 @@ +import type { HealthIndicatorService } from '@nestjs/terminus'; +import type { EnvironmentService } from '../environment/environment.service'; + +/** + * Integration guard for the /health Redis-probe handle leak (#486, commit 2). + * + * The bug: `pingCheck` built `new Redis(...)` per call and only disconnected on + * the SUCCESS path, so when Redis is DOWN every probe tick added ANOTHER + * forever-reconnecting client — an unbounded handle/client leak for the duration + * of the outage. The fix reuses ONE long-lived probe client. + * + * This is an OBSERVABLE-property test, not an assertion on a mocked return value: + * we point the indicator at a REAL, refused TCP endpoint (a dead port) so ioredis + * genuinely fails to connect, run many probes, and assert the number of live + * Redis CLIENTS created stays at exactly ONE. `ioredis` is delegated to its real + * implementation (requireActual) — only the constructor is wrapped to COUNT the + * real clients it creates, which is precisely the leaking resource. + */ +const mockLiveClients: Array<{ status: string; disconnect: () => void }> = []; + +jest.mock('ioredis', () => { + const actual = jest.requireActual('ioredis'); + const RealRedis = actual.Redis ?? actual.default ?? actual; + class CountingRedis extends RealRedis { + constructor(...args: unknown[]) { + super(...(args as [])); + mockLiveClients.push(this as never); + } + } + return { ...actual, Redis: CountingRedis, default: CountingRedis }; +}); + +// Import AFTER the mock is registered so the class picks up the counting client. +import { RedisHealthIndicator } from './redis.health'; + +describe('RedisHealthIndicator handle leak (#486)', () => { + const indicatorService = { + check: (key: string) => ({ + up: () => ({ [key]: { status: 'up' } }), + down: (message: string) => ({ [key]: { status: 'down', message } }), + }), + } as unknown as HealthIndicatorService; + + // A port with (almost certainly) nothing listening -> connection refused fast. + const environmentService = { + getRedisUrl: () => 'redis://127.0.0.1:6399/0', + } as unknown as EnvironmentService; + + let indicator: RedisHealthIndicator; + + beforeEach(() => { + mockLiveClients.length = 0; + indicator = new RedisHealthIndicator(indicatorService, environmentService); + }); + + afterEach(() => { + indicator.onModuleDestroy(); + // Belt-and-braces: tear down anything the test created so ioredis reconnect + // timers do not keep the jest worker alive. + for (const c of mockLiveClients) { + try { + c.disconnect(); + } catch { + /* already gone */ + } + } + }); + + it('creates exactly ONE Redis client across many probes while Redis is DOWN', async () => { + const N = 8; + for (let i = 0; i < N; i++) { + const result = await indicator.pingCheck('redis'); + // Down endpoint -> every probe reports "down" (not an unhandled crash). + expect(result.redis.status).toBe('down'); + } + + // THE OBSERVABLE LEAK: on the buggy code this is N (a fresh, never-cleaned + // reconnecting client per probe). The fix reuses one shared client. + expect(mockLiveClients).toHaveLength(1); + }); + + it('onModuleDestroy releases the probe client (a later probe builds a fresh one)', async () => { + await indicator.pingCheck('redis'); + expect(mockLiveClients).toHaveLength(1); + + indicator.onModuleDestroy(); + // A second destroy is a safe no-op (probeClient was nulled). + indicator.onModuleDestroy(); + + // After shutdown the indicator lazily builds a NEW client on the next probe, + // proving the old one was truly released rather than reused. + await indicator.pingCheck('redis'); + expect(mockLiveClients).toHaveLength(2); + }); +}); diff --git a/apps/server/src/integrations/health/redis.health.ts b/apps/server/src/integrations/health/redis.health.ts index 96ed53b6..c9160125 100644 --- a/apps/server/src/integrations/health/redis.health.ts +++ b/apps/server/src/integrations/health/redis.health.ts @@ -2,33 +2,78 @@ import { HealthIndicatorResult, HealthIndicatorService, } from '@nestjs/terminus'; -import { Injectable, Logger } from '@nestjs/common'; +import { Injectable, Logger, OnModuleDestroy } from '@nestjs/common'; import { EnvironmentService } from '../environment/environment.service'; import { Redis } from 'ioredis'; @Injectable() -export class RedisHealthIndicator { +export class RedisHealthIndicator implements OnModuleDestroy { private readonly logger = new Logger(RedisHealthIndicator.name); + /** + * ONE long-lived probe connection, reused across every /health tick. The old + * code built `new Redis(...)` per call and only `disconnect()`d on the SUCCESS + * path, so while Redis was DOWN every probe added a fresh, forever-reconnecting + * client — a handle leak that grew without bound for as long as the outage (and + * the health checker keeps polling) lasted. A single shared client keeps at most + * ONE background reconnect loop regardless of how many probes run. + */ + private probeClient: Redis | null = null; + constructor( private readonly healthIndicatorService: HealthIndicatorService, private environmentService: EnvironmentService, ) {} + private getProbeClient(): Redis { + if (!this.probeClient) { + this.probeClient = new Redis(this.environmentService.getRedisUrl(), { + // Constructing must never throw or eagerly connect; the first ping opens + // the socket. This lets us build the client once and reuse it. + lazyConnect: true, + // A health probe must fail FAST, not queue behind a stuck reconnect: one + // retry per request, and no offline queue so a ping while disconnected + // rejects immediately instead of buffering commands that pile up in RAM. + maxRetriesPerRequest: 1, + enableOfflineQueue: false, + }); + // ioredis emits 'error' on every failed (re)connect; with no listener that + // surfaces as an unhandled 'error' event and can crash the process. Swallow + // it here — pingCheck already reports health — and log at debug so a Redis + // outage does not flood the logs. + this.probeClient.on('error', (err) => { + this.logger.debug( + `Redis probe connection error: ${ + err instanceof Error ? err.message : String(err) + }`, + ); + }); + } + return this.probeClient; + } + async pingCheck(key: string): Promise { const indicator = this.healthIndicatorService.check(key); try { - const redis = new Redis(this.environmentService.getRedisUrl(), { - maxRetriesPerRequest: 15, - }); - + const redis = this.getProbeClient(); await redis.ping(); - redis.disconnect(); return indicator.up(); } catch (e) { this.logger.error(e); return indicator.down(`${key} is not available`); } } + + onModuleDestroy(): void { + if (this.probeClient) { + // disconnect() (not quit()) tears the socket + reconnect loop down + // immediately without waiting on a round-trip to a possibly-down server. + // Do NOT removeAllListeners() with no event name — that would also strip + // ioredis' OWN internal listeners and break its teardown; our 'error' + // listener is harmless and dies with the dropped client reference. + this.probeClient.disconnect(); + this.probeClient = null; + } + } }