From 5b7453647222e87ffbabbe3967ee0ccca44b55d2 Mon Sep 17 00:00:00 2001 From: hermes Date: Wed, 12 Aug 2026 03:52:34 -0700 Subject: [PATCH] DC-067: harden shutdown per Codex C-grade feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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 --- .../__tests__/shutdown-coordinator.test.js | 34 +++++++++++++++++-- dashcaddy-api/src/utilities/shutdown.js | 28 ++++++++++++--- 2 files changed, 55 insertions(+), 7 deletions(-) diff --git a/dashcaddy-api/__tests__/shutdown-coordinator.test.js b/dashcaddy-api/__tests__/shutdown-coordinator.test.js index 2314ace..70f0c63 100644 --- a/dashcaddy-api/__tests__/shutdown-coordinator.test.js +++ b/dashcaddy-api/__tests__/shutdown-coordinator.test.js @@ -70,12 +70,23 @@ describe('ShutdownCoordinator (DC-067)', () => { test('throws if log is missing or invalid', () => { expect(() => createShutdownCoordinator({ server: makeFakeServer(), managers: [] })) - .toThrow('log with info/warn is required'); + .toThrow('log must have info'); expect(() => createShutdownCoordinator({ server: makeFakeServer(), log: { foo: 'bar' }, 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', () => { @@ -350,5 +361,24 @@ describe('ShutdownCoordinator (DC-067)', () => { process.emit('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); + }); }); }); diff --git a/dashcaddy-api/src/utilities/shutdown.js b/dashcaddy-api/src/utilities/shutdown.js index 66fc45d..155bb76 100644 --- a/dashcaddy-api/src/utilities/shutdown.js +++ b/dashcaddy-api/src/utilities/shutdown.js @@ -28,8 +28,9 @@ class ShutdownCoordinator extends EventEmitter { constructor({ server, log, drainTimeoutMs, managers }) { super(); if (!server) throw new Error('createShutdownCoordinator: server is required'); - if (!log || typeof log.info !== 'function') { - throw new Error('createShutdownCoordinator: log with info/warn is required'); + if (!log || typeof log.info !== 'function' || typeof log.warn !== 'function' + || typeof log.error !== 'function') { + throw new Error('createShutdownCoordinator: log must have info(), warn(), and error() methods'); } this.server = server; this.log = log; @@ -92,12 +93,22 @@ class ShutdownCoordinator extends EventEmitter { this.server.close(async () => { closed = true; 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) { clearTimeout(this._forceTimer); this._forceTimer = null; } - // Now that in-flight requests are done, stop managers in order. - await this._stopManagersInOrder(); this.emit('closed', signal); 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 {string[]} signals - signals to handle (default: SIGTERM, SIGINT) @@ -135,11 +148,16 @@ function installSignalHandlers(coordinator, signals) { if (!coordinator || typeof coordinator.shutdown !== 'function') { throw new Error('installSignalHandlers: coordinator required'); } + if (!Array.isArray(coordinator._installedSignals)) { + coordinator._installedSignals = []; + } const sigs = Array.isArray(signals) && signals.length > 0 ? signals : ['SIGTERM', 'SIGINT']; for (const sig of sigs) { + if (coordinator._installedSignals.includes(sig)) continue; process.on(sig, () => coordinator.shutdown(sig)); + coordinator._installedSignals.push(sig); } }