fix(health): устранить утечку ioredis-клиента в /health-пробе (#486)
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 освобождает клиент, следующая проба лениво строит новый.
This commit is contained in:
@@ -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);
|
||||
});
|
||||
});
|
||||
@@ -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<HealthIndicatorResult> {
|
||||
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;
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user