diff --git a/docs/specs/agent-environments/http-transport/verification.md b/docs/specs/agent-environments/http-transport/verification.md index 0afdefde2c..661760348a 100644 --- a/docs/specs/agent-environments/http-transport/verification.md +++ b/docs/specs/agent-environments/http-transport/verification.md @@ -243,3 +243,9 @@ lifecycle owner picks the recorded signal up as soon as it registers, and a second signal during startup exits at once (test: the latch records `143`, exits on the second signal, hands the status over on release and leaves no listeners behind). + +Both reviewers then noted that the latched signal's status was overwritten by +the entry's normal `0` return: the HTTP entry now owns its exit status through +`createExitStatus`, where a `0` after a recorded signal keeps the signal's +status and a real failure still wins (unit test), and a process test sends the +signal the moment the listening line appears and asserts the signal status. diff --git a/src/main.ts b/src/main.ts index ee767f0c90..e84119a052 100644 --- a/src/main.ts +++ b/src/main.ts @@ -22,9 +22,11 @@ import { import { createToolResultDelivery } from "./server/toolResult.js"; import { createShutdown } from "./main/shutdown.js"; import { + createExitStatus, latchSignalShutdown, registerProcessShutdown, registerSignalShutdown, + type ExitStatus, type SignalLatch, } from "./main/processShutdown.js"; import { @@ -46,10 +48,12 @@ const runtimeDependencies = (): RuntimeDependencies => ({ const httpRuntimeDependencies = ( endpoint: McpHttpEndpoint, latch: SignalLatch, + exit: ExitStatus, ): RuntimeDependencies => { const base = runtimeDependencies(); return { ...base, + setExitCode: exit.set, serve: async (factory, options) => { // Loaded here so the default stdio entry never loads the HTTP listener. const { serveStreamableHttp } = @@ -64,13 +68,14 @@ const httpRuntimeDependencies = ( }, registerShutdown: (request) => { const unregister = registerSignalShutdown((exitStatus) => { - base.setExitCode(exitStatus); + exit.signal(exitStatus); request(); }); - // A signal that arrived during startup is honoured now that the lifecycle owner exists. + // A signal that arrived during startup is honoured now that the lifecycle owner exists; + // its status outlives the entry's own `0`, which startup returns right after this. const pending = latch.release(); if (pending !== undefined) { - base.setExitCode(pending); + exit.signal(pending); request(); } return unregister; @@ -190,11 +195,12 @@ export const runMcpHttpEntrypoint = async ( } // Latched before any asynchronous startup so a signal cannot end the process without cleanup. const latch = latchSignalShutdown(); + const exit = createExitStatus(io.setExitCode); try { await runEntrypoint( - () => run(httpRuntimeDependencies(endpoint.value, latch)), + () => run(httpRuntimeDependencies(endpoint.value, latch, exit)), io.writeStderr, - io.setExitCode, + exit.set, ); } finally { latch.release(); diff --git a/src/main/processShutdown.ts b/src/main/processShutdown.ts index f60e87ea70..754b73065c 100644 --- a/src/main/processShutdown.ts +++ b/src/main/processShutdown.ts @@ -90,3 +90,28 @@ export const latchSignalShutdown = ( }, }; }; + +/** The exit status an entry reports: a signal's status survives the entry's normal `0` return. */ +export interface ExitStatus { + /** The entry's own result; a `0` after a recorded signal keeps the signal's status. */ + readonly set: (code: number) => void; + /** Record the exit status of a signal-requested shutdown. */ + readonly signal: (status: number) => void; +} + +export const createExitStatus = ( + setExitCode: (code: number) => void, +): ExitStatus => { + let signalStatus: number | undefined; + return { + set: (code) => { + setExitCode( + code === 0 && signalStatus !== undefined ? signalStatus : code, + ); + }, + signal: (status) => { + signalStatus = status; + setExitCode(status); + }, + }; +}; diff --git a/src/server/shutdownRegistration.test.ts b/src/server/shutdownRegistration.test.ts index f3dda49b80..f5a1a242f7 100644 --- a/src/server/shutdownRegistration.test.ts +++ b/src/server/shutdownRegistration.test.ts @@ -4,6 +4,7 @@ import pino from "pino"; import { createShutdown } from "../main/shutdown.js"; import { + createExitStatus, latchSignalShutdown, registerProcessShutdown, registerSignalShutdown, @@ -237,3 +238,17 @@ it("latches the first startup signal, exits at once on a second, and hands the s const quiet = latchSignalShutdown(signals, (status) => exits.push(status)); expect(quiet.release()).toBeUndefined(); }); + +it("keeps a signal's exit status through the entry's normal return and yields to a real failure", () => { + const codes: number[] = []; + const exit = createExitStatus((code) => codes.push(code)); + exit.set(0); + exit.signal(143); + exit.set(0); + expect(codes).toEqual([0, 143, 143]); + exit.set(1); + expect(codes.at(-1)).toBe(1); + const plain = createExitStatus((code) => codes.push(code)); + plain.set(0); + expect(codes.at(-1)).toBe(0); +}); diff --git a/tests/boundary/process/mcpHttpShutdown.test.ts b/tests/boundary/process/mcpHttpShutdown.test.ts index 6b1ffdbde3..d9f1b62fff 100644 --- a/tests/boundary/process/mcpHttpShutdown.test.ts +++ b/tests/boundary/process/mcpHttpShutdown.test.ts @@ -36,4 +36,19 @@ describe("rea mcp --http shutdown", () => { }, SHUTDOWN_TIMEOUT_MS, ); + + processTest( + "exits with the signal status when the signal lands as startup completes", + async ({ processes }) => { + const started = await startMcpHttpProcess(processes); + // No request first: the signal arrives at the edge between startup and the lifecycle owner. + const [signal, status] = shutdownSignals[0]!; + started.child.kill(signal); + const [code, signalCode] = await once(started.child, "close"); + expect(code).toBe(status); + expect(signalCode).toBeNull(); + expect(started.stderr()).not.toContain(MCP_SHUTDOWN_FAILED); + }, + SHUTDOWN_TIMEOUT_MS, + ); });