diff --git a/dashcaddy-api/__tests__/shutdown-coordinator.test.js b/dashcaddy-api/__tests__/shutdown-coordinator.test.js index 70f0c63..fe145f1 100644 --- a/dashcaddy-api/__tests__/shutdown-coordinator.test.js +++ b/dashcaddy-api/__tests__/shutdown-coordinator.test.js @@ -150,6 +150,45 @@ describe('ShutdownCoordinator (DC-067)', () => { expect(handler).toHaveBeenCalledWith('SIGTERM'); }); + test('swallows exceptions thrown by shutdown event listeners', () => { + const log = makeFakeLog(); + const server = makeFakeServer(); + const c = createShutdownCoordinator({ + server, + log, + managers: [], + }); + c.on('shutdown', () => { throw new Error('listener boom'); }); + + // shutdown() must NOT propagate the exception — that would abort + // the entire shutdown sequence before server.close is even called. + expect(() => c.shutdown('SIGTERM')).not.toThrow(); + expect(log.error).toHaveBeenCalledWith( + 'shutdown', + "event listener for 'shutdown' threw", + expect.objectContaining({ error: 'listener boom' }), + ); + // server.close should still have been called. + expect(server.close).toHaveBeenCalledTimes(1); + }); + + test('swallows exceptions thrown by closed event listeners', async () => { + const log = makeFakeLog(); + const server = makeFakeServer(); + const c = createShutdownCoordinator({ + server, + log, + managers: [], + }); + c.on('closed', () => { throw new Error('closed listener boom'); }); + + // process.exit is mocked; we just verify the throw doesn't bubble. + c.shutdown('SIGTERM'); + await new Promise((resolve) => setImmediate(resolve)); + // The closed listener threw but the exit still got recorded. + expect(exitCalls).toEqual([0]); + }); + test('calls server.close() once', () => { const server = makeFakeServer(); const c = createShutdownCoordinator({ @@ -289,36 +328,77 @@ describe('ShutdownCoordinator (DC-067)', () => { expect(exitCalls).toEqual([0]); expect(log.warn).toHaveBeenCalledWith( 'shutdown', - 'drain timeout (1000ms) reached, force-exiting', + expect.stringContaining('drain timeout (1000ms) reached before HTTP server closed'), ); } finally { jest.useRealTimers(); } }); - test('clears force-exit timer when server.close fires first', () => { + test('force-exits after drainTimeoutMs if manager.stop() hangs after HTTP close', () => { jest.useFakeTimers(); try { - const server = makeFakeServer(); // calls back on the same tick + const server = makeFakeServer(); // calls back immediately const log = makeFakeLog(); + // Manager that NEVER resolves — simulates a hung cleanup. + const hungManager = { + name: 'hung', + stop: jest.fn(() => new Promise(() => {})), // never resolves + }; const c = createShutdownCoordinator({ server, log, drainTimeoutMs: 1000, - managers: [], + managers: [hungManager], }); c.shutdown('SIGTERM'); - // 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. + // After the synchronous shutdown() call: server.close has fired + // (serverClosed=true), but hungManager.stop() has been called and + // its promise is pending. managersStopped is still false. + // process.exit should NOT have been called yet. + expect(exitCalls).toEqual([]); + + jest.advanceTimersByTime(1001); + // Now the safety-net timer fires — force-exit because manager hung. + expect(exitCalls).toEqual([0]); + expect(log.warn).toHaveBeenCalledWith( + 'shutdown', + expect.stringContaining('after HTTP close (manager hung)'), + ); + } finally { + jest.useRealTimers(); + } + }); + + test('clears force-exit timer when manager drain completes promptly', () => { + jest.useFakeTimers(); + try { + const server = makeFakeServer(); // calls back on the same tick + const log = makeFakeLog(); + // Quick-stopping manager. The close callback awaits stop(), + // which resolves immediately, so managersStopped flips true + // and the safety-net timer is cleared before it can fire. + const fastManager = { + name: 'fast', + stop: jest.fn(() => Promise.resolve()), + }; + const c = createShutdownCoordinator({ + server, + log, + drainTimeoutMs: 1000, + managers: [fastManager], + }); + + c.shutdown('SIGTERM'); + + // Flush microtasks so the close callback's await stop() resolves, + // managersStopped flips true, the timer is cleared, and + // process.exit(0) is recorded exactly once. return Promise.resolve().then(() => Promise.resolve()).then(() => { expect(exitCalls).toEqual([0]); - // Advance well past the drain timeout — no extra exit should fire - // because the close callback cleared the force-exit timer. + // Advance well past the drain timeout — no extra exit should fire. jest.advanceTimersByTime(5000); expect(exitCalls).toEqual([0]); }); diff --git a/dashcaddy-api/src/utilities/shutdown.js b/dashcaddy-api/src/utilities/shutdown.js index 155bb76..3a6127a 100644 --- a/dashcaddy-api/src/utilities/shutdown.js +++ b/dashcaddy-api/src/utilities/shutdown.js @@ -71,6 +71,24 @@ class ShutdownCoordinator extends EventEmitter { } } + /** + * Emit an event but swallow listener exceptions so one bad listener + * can't abort the shutdown sequence. Logs each failure with the + * listener's name (set via `listener.name`) if available. + */ + _safeEmit(event, ...args) { + const listeners = this.listeners(event); + for (const listener of listeners) { + try { + listener.apply(this, args); + } catch (err) { + const name = listener.name || ''; + this.log.error('shutdown', `event listener for '${event}' threw`, + { listener: name, error: err.message }); + } + } + } + shutdown(signal) { if (this._shuttingDown) { this.log.info('shutdown', `${signal || 'shutdown'} received — already shutting down, ignoring`); @@ -82,16 +100,19 @@ class ShutdownCoordinator extends EventEmitter { // 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); + // _safeEmit swallows listener exceptions so a buggy listener can't + // abort the entire shutdown sequence. + this._safeEmit('shutdown', signal); // 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; + let serverClosed = false; + let managersStopped = false; try { this.server.close(async () => { - closed = true; + serverClosed = 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() @@ -104,26 +125,31 @@ class ShutdownCoordinator extends EventEmitter { // throw (e.g. from the for-loop itself) is still possible. this.log.error('shutdown', 'manager shutdown loop threw', { error: err.message }); } + managersStopped = true; // Manager drain complete — NOW we can clear the safety timer. if (this._forceTimer) { clearTimeout(this._forceTimer); this._forceTimer = null; } - this.emit('closed', signal); + this._safeEmit('closed', signal); process.exit(0); }); } catch (err) { this.log.error('shutdown', 'server.close threw', { error: err.message }); } - // 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. + // Force-exit safety net. Fires when EITHER: + // (a) server.close never fires (HTTP server stuck draining), or + // (b) server.close fired but managers hung during stop() + // We only suppress when managersStopped === true (full drain complete). + // serverClosed alone is NOT enough — managers could still be running. 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. + if (managersStopped) return; // full shutdown complete + if (!serverClosed) { + this.log.warn('shutdown', `drain timeout (${this.drainTimeoutMs}ms) reached before HTTP server closed, force-exiting`); + } else { + this.log.warn('shutdown', `drain timeout (${this.drainTimeoutMs}ms) reached after HTTP close (manager hung), force-exiting`); + } process.exit(0); }, this.drainTimeoutMs); if (this._forceTimer && typeof this._forceTimer.unref === 'function') {