diff --git a/dashcaddy-api/__tests__/shutdown-coordinator.test.js b/dashcaddy-api/__tests__/shutdown-coordinator.test.js index 456db5f..2314ace 100644 --- a/dashcaddy-api/__tests__/shutdown-coordinator.test.js +++ b/dashcaddy-api/__tests__/shutdown-coordinator.test.js @@ -45,10 +45,12 @@ describe('ShutdownCoordinator (DC-067)', () => { jest.clearAllTimers(); }); - function makeFakeServer() { - // Synchronous close callback. async (setImmediate) callbacks would fire - // AFTER the test that triggered shutdown has ended, hitting the real - // process.exit in the restored mock window. + function makeFakeServer({ closeBehavior = 'sync' } = {}) { + // 'sync' close calls back immediately. + // 'never' close never calls back (used to test force-exit). + if (closeBehavior === 'never') { + return { close: jest.fn() }; + } return { close: jest.fn((cb) => { cb(); }) }; } @@ -150,7 +152,7 @@ describe('ShutdownCoordinator (DC-067)', () => { expect(server.close).toHaveBeenCalledTimes(1); }); - test('stops each manager in declaration order', async () => { + test('stops each manager in declaration order AFTER server.close fires', async () => { const order = []; const managers = [ { name: 'first', stop: jest.fn(() => { order.push('first'); }) }, @@ -164,13 +166,44 @@ describe('ShutdownCoordinator (DC-067)', () => { }); c.shutdown('SIGTERM'); - - // Wait one microtask + macrotask for the async chain to settle. + // Wait for the async chain (server.close → _stopManagersInOrder → + // process.exit) to settle. The mock exit is synchronous so this + // resolves once all microtasks drain. await new Promise((resolve) => setImmediate(resolve)); expect(order).toEqual(['first', 'second', 'third']); }); + test('does NOT stop managers until server.close callback fires', () => { + const order = []; + // Use a server whose close callback fires only when we manually call it. + let deferredCloseCb; + const server = { + close: jest.fn((cb) => { deferredCloseCb = cb; }), + }; + const managers = [ + { name: 'first', stop: jest.fn(() => { order.push('first'); }) }, + ]; + const c = createShutdownCoordinator({ + server, + log: makeFakeLog(), + managers, + }); + + c.shutdown('SIGTERM'); + + // server.close has been called but its callback hasn't fired yet. + expect(server.close).toHaveBeenCalledTimes(1); + // Manager has NOT been stopped yet — server is still draining. + expect(order).toEqual([]); + + // Now fire the deferred callback to simulate drain completion. + deferredCloseCb(); + + // Manager stopped AFTER server.close fired. + expect(order).toEqual(['first']); + }); + test('continues stopping remaining managers if one throws', async () => { const order = []; const managers = [ @@ -226,7 +259,7 @@ describe('ShutdownCoordinator (DC-067)', () => { test('force-exits after drainTimeoutMs if server.close never fires', () => { jest.useFakeTimers(); try { - const server = { close: jest.fn(/* never calls back */) }; + const server = makeFakeServer({ closeBehavior: 'never' }); const log = makeFakeLog(); const c = createShutdownCoordinator({ server, @@ -255,7 +288,7 @@ describe('ShutdownCoordinator (DC-067)', () => { test('clears force-exit timer when server.close fires first', () => { jest.useFakeTimers(); try { - const server = makeFakeServer(); // calls back on setImmediate + const server = makeFakeServer(); // calls back on the same tick const log = makeFakeLog(); const c = createShutdownCoordinator({ server, @@ -265,16 +298,19 @@ describe('ShutdownCoordinator (DC-067)', () => { }); c.shutdown('SIGTERM'); - // Flush the setImmediate so server.close callback fires. - jest.runOnlyPendingTimers(); - jest.advanceTimersByTime(0); + // The close callback is async (it awaits _stopManagersInOrder), + // so the synchronous c.shutdown() returns BEFORE process.exit(0) + // is recorded. Flush pending timers + microtasks. + jest.runAllTimers(); + // Flush microtasks queued by the async close callback. + return Promise.resolve().then(() => Promise.resolve()).then(() => { + expect(exitCalls).toEqual([0]); - expect(exitCalls).toEqual([0]); - - // Even after the full drain timeout, no extra exit should fire - // (because we cleared the timer in the close callback). - jest.advanceTimersByTime(5000); - expect(exitCalls).toEqual([0]); + // Advance well past the drain timeout — no extra exit should fire + // because the close callback cleared the force-exit timer. + jest.advanceTimersByTime(5000); + expect(exitCalls).toEqual([0]); + }); } finally { jest.useRealTimers(); } diff --git a/dashcaddy-api/src/utilities/shutdown.js b/dashcaddy-api/src/utilities/shutdown.js index a7db89d..66fc45d 100644 --- a/dashcaddy-api/src/utilities/shutdown.js +++ b/dashcaddy-api/src/utilities/shutdown.js @@ -54,6 +54,22 @@ class ShutdownCoordinator extends EventEmitter { } } + /** + * Stop each manager sequentially in declaration order. Each manager's + * stop() is awaited so that a downstream manager is not stopped until + * its upstream dependency has finished draining. + * + * IMPORTANT: this runs AFTER server.close() returns (see shutdown()). + * We must wait for in-flight HTTP requests to complete before tearing + * down the services that serve them — otherwise those requests fail + * mid-drain with "service not found" / "monitor not running" errors. + */ + async _stopManagersInOrder() { + for (const m of this.managers) { + await this._stopManager(m); + } + } + shutdown(signal) { if (this._shuttingDown) { this.log.info('shutdown', `${signal || 'shutdown'} received — already shutting down, ignoring`); @@ -62,28 +78,26 @@ class ShutdownCoordinator extends EventEmitter { this._shuttingDown = true; this.log.info('shutdown', `${signal || 'shutdown'} received, draining (timeout=${this.drainTimeoutMs}ms)...`); - // Emit 'shutdown' event first so any listeners can flush their state - // before the managers start stopping. + // Emit 'shutdown' event first so any listeners can observe the signal + // before the drain begins. NOTE: listeners should NOT tear down their + // state here — that happens in the 'closed' event after server.close. this.emit('shutdown', signal); - // Stop each manager in order. Fire-and-forget — each manager's stop() - // is expected to be fast (cancel timers, flush buffers). If a manager - // has long async work, it should expose its own drain mechanism. - for (const m of this.managers) { - this._stopManager(m); - } - - // Close the HTTP server. Stops accepting new connections, waits for - // in-flight requests to complete naturally. + // Close the HTTP server FIRST. Stops accepting new connections, waits + // for in-flight requests to complete naturally. Only AFTER close fires + // do we tear down managers — otherwise in-flight requests could fail + // when the services they call have already been stopped. let closed = false; try { - this.server.close(() => { + this.server.close(async () => { closed = true; this.log.info('shutdown', 'HTTP server closed cleanly'); 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); }); @@ -91,10 +105,14 @@ class ShutdownCoordinator extends EventEmitter { this.log.error('shutdown', 'server.close threw', { error: err.message }); } - // Force-exit if drain doesn't complete in time. + // Force-exit if drain doesn't complete in time. This is the safety + // net for the case where a long-polling request or a stuck handler + // never returns and the drain never finishes. this._forceTimer = setTimeout(() => { if (closed) return; // race: server.close fired AND timer fired this.log.warn('shutdown', `drain timeout (${this.drainTimeoutMs}ms) reached, force-exiting`); + // On force-exit we don't run manager.stop() — the process is + // about to die anyway, and SIGKILL will be next if we don't exit. process.exit(0); }, this.drainTimeoutMs); if (this._forceTimer && typeof this._forceTimer.unref === 'function') {