feat(health-check): add health-check package#242
Conversation
Co-authored-by: Copilot <copilot@github.com>
ronenkapelian
left a comment
There was a problem hiding this comment.
### src/common/config.ts
const terminatePod = process.env.TERMINATE_POD !== 'false';
should it come from config?
| }, | ||
| }; | ||
|
|
||
| createTerminus(this.server, newTerminusOptions); |
There was a problem hiding this comment.
- Every new HealthCheckManager(...) in a process attaches new signal listeners with no way to remove them (no dispose()/close() method). Any test suite, or any code path that ends up constructing more than one manager (e.g. DI re-registration, hot-module-reload, or a bug that resolves the factory twice), leaks process listeners silently — Node will eventually warn MaxListenersExceededWarning.
- This is also why the package's own test suite has to vi.mock('@godaddy/terminus') wholesale rather than testing against the real thing — a sign the constructor is doing too much for a unit boundary.
split wiring out of the constructor, and guard against accidental double-construction (note: @godaddy/terminus itself doesn't expose a way to unregister its signal handlers, so a full close() isn't possible without forking it — the pragmatic fix is to make "one per process" an enforced invariant instead of an assumption):
export class HealthCheckManager {
private static instantiated = false;
private readonly terminusOptions: TerminusOptions;
public constructor(server: Server, logger: Logger, terminusOptions?: TerminusOptions) {
if (HealthCheckManager.instantiated) {
throw new Error('HealthCheckManager must only be constructed once per process');
}
HealthCheckManager.instantiated = true;
this.logger = logger;
this.server = server;
this.server.on('request', (req, res) => { /* ...unchanged... */ });
// build this.terminusOptions here, but do NOT call createTerminus yet
this.terminusOptions = this.buildTerminusOptions(terminusOptions);
}
/** Wires the manager into terminus. Call once, right before `server.listen()`. */
public attach(): void {
createTerminus(this.server, this.terminusOptions);
}
}
// caller:
const manager = new HealthCheckManager(server, logger, opts);
manager.registerReadinessCheck('db', dbCheck);
manager.attach(); // signal handlers registered here, at a well-defined point
server.listen(port);
nice to have also close()/teardown API.
| while (this.getActiveRequestsCount() > 0) { | ||
| await sleep(checkIntervalMs); |
There was a problem hiding this comment.
Potentially, it can loop forever if the pod is stuck (for example, jobnik)
Should we add some parameter of timeout for graceful closing?
| const healthChecks: Record<string, () => Promise<void>> = { | ||
| '/liveness': async (): Promise<void> => { | ||
| await Promise.all( | ||
| Object.entries(this.livenessChecks).map(async ([name, check]) => { | ||
| try { | ||
| await check(); | ||
| } catch (error) { | ||
| this.logger.error({ msg: `Liveness check failed: ${name}`, err: error }); | ||
| throw error; | ||
| } | ||
| }) | ||
| ); | ||
| }, | ||
| '/readiness': async (): Promise<void> => { | ||
| await Promise.all( | ||
| Object.entries(this.readinessChecks).map(async ([name, check]) => { | ||
| try { | ||
| await check(); | ||
| } catch (error) { | ||
| this.logger.error({ msg: `Readiness check failed: ${name}`, err: error }); | ||
| throw error; |
There was a problem hiding this comment.
DRY: /liveness and /readiness handlers are copy-pasted
No description provided.