DC-067: fix shutdown sequencing — stop managers AFTER server.close drains
Codex grade D flagged two real defects: 1. Managers were stopped before HTTP server finished draining, so in-flight requests could fail when their backing services were already down. 2. _stopManager() promises weren't awaited, contradicting the documented 'declaration order' claim for async stop methods. Fix: server.close callback now awaits _stopManagersInOrder() before exiting. The 'shutdown' event fires first (so listeners can observe the signal); the 'closed' event fires after all managers are stopped.
This commit is contained in:
hermes
@@ -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();
|
||||
}
|
||||
|
||||
@@ -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') {
|
||||
|
||||
Reference in New Issue
Block a user