mirror of
https://github.com/morluto/rea.git
synced 2026-11-03 13:26:10 +00:00
fix(mcp): latch startup signals so the HTTP entry never ends without cleanup
Addresses the sixth automated review finding: SIGINT and SIGTERM are recorded from the moment the HTTP entry starts, the lifecycle owner acts on a recorded signal as soon as it registers, and a second signal during startup exits at once. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
This commit is contained in:
1 parent
174188279b
commit
354b4d8c08
6 files changed
+87
-10
No files matched your search
@@ -459,7 +459,10 @@ name them. Listener errors reach the runtime's connection-loss handler only
|
||||
after a successful bind; a bind failure prints the listen-failure message alone. Request bodies are bounded by the SDK's
|
||||
4 MiB limit, and the response budget keeps its stdio-derived floor. The
|
||||
listener ends on SIGINT or SIGTERM only, not on stdin EOF, and exits `130` or
|
||||
`143` after cleanup settles.
|
||||
`143` after cleanup settles. A signal that arrives while startup is still
|
||||
running is recorded and acted on as soon as the listener and its providers are
|
||||
owned, so startup never ends without cleanup; a second signal during startup
|
||||
exits immediately.
|
||||
|
||||
## Progress and cancellation
|
||||
|
||||
|
||||
@@ -42,7 +42,9 @@ client registration keep stdio.
|
||||
checks replace an open session; refuse requests once the slot is closed, join
|
||||
an arming session during shutdown, and keep close stages retryable.
|
||||
- End the HTTP listener on SIGINT/SIGTERM only, never on stdin EOF, and exit with
|
||||
the signal status (`130`/`143`) after cleanup settles.
|
||||
the signal status (`130`/`143`) after cleanup settles; latch a signal that
|
||||
arrives during startup so it is acted on as soon as the lifecycle owner exists,
|
||||
and exit at once on a second one.
|
||||
- Export `serveStreamableHttp({ host, port, createMcpServer, routes, logger })`
|
||||
returning `{ url, close }` so an agent environment server can add plain routes
|
||||
beside `/mcp` without a second bridge.
|
||||
|
||||
@@ -234,3 +234,12 @@ request to drain (test: a 4.5 MB sessionless initialize is refused promptly
|
||||
while the connected client keeps echoing); and the node:http bridge passes
|
||||
repeated `Set-Cookie` values as an array instead of collapsing them (test: two
|
||||
cookies from a composed route both arrive).
|
||||
|
||||
A sixth round (Codex, one P1) was addressed next: signal handlers were only
|
||||
installed after the asynchronous startup returned, so a SIGINT or SIGTERM
|
||||
during target opening, provider loading or the HTTP bind ended the process
|
||||
without cleanup. The HTTP entry now latches signals before startup, the
|
||||
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).
|
||||
+25
-8
@@ -22,8 +22,10 @@ import {
|
||||
import { createToolResultDelivery } from "./server/toolResult.js";
|
||||
import { createShutdown } from "./main/shutdown.js";
|
||||
import {
|
||||
latchSignalShutdown,
|
||||
registerProcessShutdown,
|
||||
registerSignalShutdown,
|
||||
type SignalLatch,
|
||||
} from "./main/processShutdown.js";
|
||||
import {
|
||||
parseMcpHttpEndpoint,
|
||||
@@ -43,6 +45,7 @@ const runtimeDependencies = (): RuntimeDependencies => ({
|
||||
/** Replace stdio with the streamable HTTP listener; signals alone end it, never stdin. */
|
||||
const httpRuntimeDependencies = (
|
||||
endpoint: McpHttpEndpoint,
|
||||
latch: SignalLatch,
|
||||
): RuntimeDependencies => {
|
||||
const base = runtimeDependencies();
|
||||
return {
|
||||
@@ -59,11 +62,19 @@ const httpRuntimeDependencies = (
|
||||
...options,
|
||||
});
|
||||
},
|
||||
registerShutdown: (request) =>
|
||||
registerSignalShutdown((exitStatus) => {
|
||||
registerShutdown: (request) => {
|
||||
const unregister = registerSignalShutdown((exitStatus) => {
|
||||
base.setExitCode(exitStatus);
|
||||
request();
|
||||
}),
|
||||
});
|
||||
// A signal that arrived during startup is honoured now that the lifecycle owner exists.
|
||||
const pending = latch.release();
|
||||
if (pending !== undefined) {
|
||||
base.setExitCode(pending);
|
||||
request();
|
||||
}
|
||||
return unregister;
|
||||
},
|
||||
};
|
||||
};
|
||||
|
||||
@@ -177,11 +188,17 @@ export const runMcpHttpEntrypoint = async (
|
||||
io.setExitCode(1);
|
||||
return;
|
||||
}
|
||||
await runEntrypoint(
|
||||
() => run(httpRuntimeDependencies(endpoint.value)),
|
||||
io.writeStderr,
|
||||
io.setExitCode,
|
||||
);
|
||||
// Latched before any asynchronous startup so a signal cannot end the process without cleanup.
|
||||
const latch = latchSignalShutdown();
|
||||
try {
|
||||
await runEntrypoint(
|
||||
() => run(httpRuntimeDependencies(endpoint.value, latch)),
|
||||
io.writeStderr,
|
||||
io.setExitCode,
|
||||
);
|
||||
} finally {
|
||||
latch.release();
|
||||
}
|
||||
};
|
||||
|
||||
/** Return false when the runtime's argv path is synthetic or no longer exists. */
|
||||
|
||||
@@ -61,3 +61,32 @@ export const registerSignalShutdown = (
|
||||
},
|
||||
{ signals },
|
||||
);
|
||||
|
||||
/** A signal recorded while startup is still running, before the lifecycle owner can act on it. */
|
||||
export interface SignalLatch {
|
||||
/** Stop recording; returns the exit status of the first signal seen, if any. */
|
||||
readonly release: () => number | undefined;
|
||||
}
|
||||
|
||||
/** Record the first SIGINT or SIGTERM during startup and exit at once on a second one, so startup never dies without its cleanup. */
|
||||
export const latchSignalShutdown = (
|
||||
signals: ShutdownSignalSource = process,
|
||||
exit: (status: number) => void = (status) => process.exit(status),
|
||||
): SignalLatch => {
|
||||
let pending: number | undefined;
|
||||
const record = (signal: ShutdownSignal) => (): void => {
|
||||
if (pending === undefined) pending = SIGNAL_EXIT_STATUS[signal];
|
||||
else exit(pending);
|
||||
};
|
||||
const onInterrupt = record("SIGINT");
|
||||
const onTerminate = record("SIGTERM");
|
||||
signals.on("SIGINT", onInterrupt);
|
||||
signals.on("SIGTERM", onTerminate);
|
||||
return {
|
||||
release: () => {
|
||||
signals.off("SIGINT", onInterrupt);
|
||||
signals.off("SIGTERM", onTerminate);
|
||||
return pending;
|
||||
},
|
||||
};
|
||||
};
|
||||
@@ -4,6 +4,7 @@ import pino from "pino";
|
||||
|
||||
import { createShutdown } from "../main/shutdown.js";
|
||||
import {
|
||||
latchSignalShutdown,
|
||||
registerProcessShutdown,
|
||||
registerSignalShutdown,
|
||||
} from "../main/processShutdown.js";
|
||||
@@ -220,3 +221,19 @@ it("joins concurrent shutdown events and retries only failed owners", async () =
|
||||
expect(input.listenerCount("end")).toBe(0);
|
||||
expect(input.listenerCount("close")).toBe(0);
|
||||
});
|
||||
|
||||
it("latches the first startup signal, exits at once on a second, and hands the status over on release", () => {
|
||||
const signals = new EventEmitter();
|
||||
const exits: number[] = [];
|
||||
const latch = latchSignalShutdown(signals, (status) => exits.push(status));
|
||||
expect(signals.listenerCount("SIGINT")).toBe(1);
|
||||
signals.emit("SIGTERM");
|
||||
expect(exits).toEqual([]);
|
||||
signals.emit("SIGINT");
|
||||
expect(exits).toEqual([143]);
|
||||
expect(latch.release()).toBe(143);
|
||||
expect(signals.listenerCount("SIGINT")).toBe(0);
|
||||
expect(signals.listenerCount("SIGTERM")).toBe(0);
|
||||
const quiet = latchSignalShutdown(signals, (status) => exits.push(status));
|
||||
expect(quiet.release()).toBeUndefined();
|
||||
});
|
||||
Reference in new issue
Block a user