DC-067: harden shutdown per Codex C-grade feedback

- Logger validation now requires info/warn/error (was info-only)
- Force-exit timer now survives manager stop drain so a hung manager
  cannot trap the process in half-shutdown
- installSignalHandlers is now actually idempotent — tracks installed
  signals on coordinator and skips duplicates
This commit is contained in:
hermes
2026-08-12 03:52:34 -07:00
parent bb01a77ae7
commit 5b74536472
2 changed files with 55 additions and 7 deletions
@@ -70,12 +70,23 @@ describe('ShutdownCoordinator (DC-067)', () => {
test('throws if log is missing or invalid', () => { test('throws if log is missing or invalid', () => {
expect(() => createShutdownCoordinator({ server: makeFakeServer(), managers: [] })) expect(() => createShutdownCoordinator({ server: makeFakeServer(), managers: [] }))
.toThrow('log with info/warn is required'); .toThrow('log must have info');
expect(() => createShutdownCoordinator({ expect(() => createShutdownCoordinator({
server: makeFakeServer(), server: makeFakeServer(),
log: { foo: 'bar' }, log: { foo: 'bar' },
managers: [], managers: [],
})).toThrow('log with info/warn is required'); })).toThrow('log must have info');
expect(() => createShutdownCoordinator({
server: makeFakeServer(),
log: { info: () => {}, warn: () => {} }, // missing error
managers: [],
})).toThrow('log must have info');
// A log with all three methods should NOT throw.
expect(() => createShutdownCoordinator({
server: makeFakeServer(),
log: { info: () => {}, warn: () => {}, error: () => {} },
managers: [],
})).not.toThrow();
}); });
test('uses DEFAULT_DRAIN_TIMEOUT_MS when drainTimeoutMs is not finite positive', () => { test('uses DEFAULT_DRAIN_TIMEOUT_MS when drainTimeoutMs is not finite positive', () => {
@@ -350,5 +361,24 @@ describe('ShutdownCoordinator (DC-067)', () => {
process.emit('SIGHUP'); process.emit('SIGHUP');
expect(shutdownSpy).toHaveBeenCalledWith('SIGHUP'); expect(shutdownSpy).toHaveBeenCalledWith('SIGHUP');
}); });
test('is idempotent — calling installSignalHandlers twice does not double-register', () => {
const c = createShutdownCoordinator({
server: makeFakeServer(),
log: makeFakeLog(),
managers: [],
});
const shutdownSpy = jest.spyOn(c, 'shutdown');
installSignalHandlers(c);
installSignalHandlers(c); // second call
installSignalHandlers(c); // third call
// The installedSignals tracker should have one entry per signal.
expect(c._installedSignals).toEqual(['SIGTERM', 'SIGINT']);
process.emit('SIGTERM');
expect(shutdownSpy).toHaveBeenCalledTimes(1);
});
}); });
}); });
+23 -5
View File
@@ -28,8 +28,9 @@ class ShutdownCoordinator extends EventEmitter {
constructor({ server, log, drainTimeoutMs, managers }) { constructor({ server, log, drainTimeoutMs, managers }) {
super(); super();
if (!server) throw new Error('createShutdownCoordinator: server is required'); if (!server) throw new Error('createShutdownCoordinator: server is required');
if (!log || typeof log.info !== 'function') { if (!log || typeof log.info !== 'function' || typeof log.warn !== 'function'
throw new Error('createShutdownCoordinator: log with info/warn is required'); || typeof log.error !== 'function') {
throw new Error('createShutdownCoordinator: log must have info(), warn(), and error() methods');
} }
this.server = server; this.server = server;
this.log = log; this.log = log;
@@ -92,12 +93,22 @@ class ShutdownCoordinator extends EventEmitter {
this.server.close(async () => { this.server.close(async () => {
closed = true; closed = true;
this.log.info('shutdown', 'HTTP server closed cleanly'); this.log.info('shutdown', 'HTTP server closed cleanly');
// Now that in-flight requests are done, stop managers in order.
// We do NOT clear the force-exit timer yet — if a manager's stop()
// hangs, the timer is the safety net that prevents the process
// from living forever in a half-shut-down state.
try {
await this._stopManagersInOrder();
} catch (err) {
// _stopManager already logs per-manager failures, but a top-level
// throw (e.g. from the for-loop itself) is still possible.
this.log.error('shutdown', 'manager shutdown loop threw', { error: err.message });
}
// Manager drain complete — NOW we can clear the safety timer.
if (this._forceTimer) { if (this._forceTimer) {
clearTimeout(this._forceTimer); clearTimeout(this._forceTimer);
this._forceTimer = null; this._forceTimer = null;
} }
// Now that in-flight requests are done, stop managers in order.
await this._stopManagersInOrder();
this.emit('closed', signal); this.emit('closed', signal);
process.exit(0); process.exit(0);
}); });
@@ -126,7 +137,9 @@ function createShutdownCoordinator(opts) {
} }
/** /**
* Install OS-level signal handlers. Idempotent: second call is a no-op. * Install OS-level signal handlers. Idempotent: second call for the same
* signal does NOT register a duplicate listener. Tracks registered signals
* on the coordinator itself so a future caller can introspect.
* *
* @param {ShutdownCoordinator} coordinator * @param {ShutdownCoordinator} coordinator
* @param {string[]} signals - signals to handle (default: SIGTERM, SIGINT) * @param {string[]} signals - signals to handle (default: SIGTERM, SIGINT)
@@ -135,11 +148,16 @@ function installSignalHandlers(coordinator, signals) {
if (!coordinator || typeof coordinator.shutdown !== 'function') { if (!coordinator || typeof coordinator.shutdown !== 'function') {
throw new Error('installSignalHandlers: coordinator required'); throw new Error('installSignalHandlers: coordinator required');
} }
if (!Array.isArray(coordinator._installedSignals)) {
coordinator._installedSignals = [];
}
const sigs = Array.isArray(signals) && signals.length > 0 const sigs = Array.isArray(signals) && signals.length > 0
? signals ? signals
: ['SIGTERM', 'SIGINT']; : ['SIGTERM', 'SIGINT'];
for (const sig of sigs) { for (const sig of sigs) {
if (coordinator._installedSignals.includes(sig)) continue;
process.on(sig, () => coordinator.shutdown(sig)); process.on(sig, () => coordinator.shutdown(sig));
coordinator._installedSignals.push(sig);
} }
} }