test(plugin-worker-manager): close a timing race in the stdin supervision test

The negative-control test had a 200ms margin against a real process exit,
which is too thin to rely on under a loaded CI runner.

stopInternal() races the shutdown RPC against waitForExit(), and the RPC
resolves as soon as the worker acks. It then allows only a further 500ms
before escalating to SIGTERM. The fixture exited 300ms after acking, so a
late timer or a slow exit propagation would have surfaced as SIGTERM and
failed the "signal must be null" assertion for a reason unrelated to the
guard under test.

Two deadlines were squeezing from opposite sides: the destroy had to land
before the fixture exited, and the fixture had to exit before the host
escalated. Widening one window narrows the other, so the fix is to stop
polling rather than to retune the constants.

The test now kills the command channel from a hook on the host's own
stdin.write, firing the moment the shutdown payload is flushed, instead of
waiting for status "stopping" via vi.waitFor. That removes both races: the
destroy lands immediately after the shutdown reaches the worker, and the
fixture's exit delay drops to 100ms, leaving 400ms of margin.

The hook is also a stronger precondition than the status poll it replaces.
sendMessage() is only reached for "shutdown" from inside stopInternal(),
which sets intentionalStop before it writes -- so firing there proves we are
inside the intentional-stop window rather than inferring it from an
observable status. The previous writableLength check is subsumed: the pipe
is destroyed from the write path itself, after the payload is handed off.

Added an explicit aliveAtDestroy assertion so the vacuous case is loud. If
the fixture ever exits before the pipe is killed, the guard is never
exercised and the old test would still have passed; now it fails with
"fixture exited before the command channel was killed".

Re-verified by mutation, not just by going green:

- manager reverted to the pre-fix parent aa6b6bcb (supervision block absent,
  grep -c to zero, not a partial mutation) fails case 1 only, with
  "worker was left alive and uncommandable after its command channel died";
- removing only the `intentionalStop ||` guard fails case 2 only, with
  "expected 'SIGKILL' to be null".

Each mutation still reds exactly one test and leaves the other green. Suite
run 5x consecutively, clean. Server typecheck clean. Test-only change: no
source file is touched.
This commit is contained in:
Tycho 2026-08-05 13:11:25 -06:00 committed by coal
parent 505da32def
commit 1efea22d2f
2 changed files with 67 additions and 12 deletions

View File

@ -12,8 +12,16 @@
const readline = require("node:readline");
/** How long the worker waits after acking `shutdown` before exiting. */
const SHUTDOWN_EXIT_DELAY_MS = 300;
// How long the worker waits after acking `shutdown` before exiting.
//
// This has to sit inside the host's post-ack grace period: stopInternal()
// races the shutdown RPC (which resolves as soon as this ack lands) and then
// waits only 500ms more before escalating to SIGTERM. A delay close to that
// ceiling makes the negative-control test a timing race against a real process
// exit on a loaded CI runner, so keep the margin wide. The test does not
// depend on this window being long — it kills the pipe from a write hook the
// moment the shutdown is flushed, not after a poll.
const SHUTDOWN_EXIT_DELAY_MS = 100;
/** Hard ceiling so a fixture never outlives the test run that spawned it. */
const MAX_LIFETIME_MS = 30_000;

View File

@ -86,6 +86,55 @@ function exitWithin(child: ChildProcess, timeoutMs: number): Promise<Exit | null
]);
}
/**
* Destroy the command channel at the instant the host's `shutdown` RPC has
* been flushed to the worker.
*
* This is hooked rather than polled on purpose. Waiting for status
* `"stopping"` via vi.waitFor puts the destroy an unbounded number of
* milliseconds after the ack, which then has to beat the fixture's own
* deferred exit and the fixture in turn has to beat the host's 500ms
* post-ack SIGTERM escalation. Those two deadlines squeeze from opposite
* sides, and on a loaded runner one of them eventually loses. Hooking the
* write removes both races: the destroy lands immediately after the shutdown
* reaches the worker, so the fixture still has its whole exit window left.
*
* It is also a stronger precondition than polling for status. `sendMessage`
* is only reached for `shutdown` from inside stopInternal(), which sets
* `intentionalStop` before it writes so firing here proves we are inside the
* intentional-stop window rather than inferring it from an observable status.
*
* Resolves with whether the child was still alive when the pipe was killed.
* If it had already exited, the test never exercised the guard at all, and the
* assertion on this value turns that vacuous pass into a failure.
*/
function destroyStdinOnShutdown(child: ChildProcess): Promise<{ aliveAtDestroy: boolean }> {
const stdin = child.stdin;
if (!stdin) throw new Error("expected the forked child to have a stdin pipe");
return new Promise((resolve) => {
const originalWrite = stdin.write.bind(stdin) as typeof stdin.write;
let fired = false;
stdin.write = ((chunk: unknown, ...rest: unknown[]) => {
const accepted = (originalWrite as (...a: unknown[]) => boolean)(chunk, ...rest);
if (!fired && typeof chunk === "string" && chunk.includes('"shutdown"')) {
fired = true;
// Let the manager's own write callback run first, so the shutdown is
// fully handed off before the pipe dies.
setImmediate(() => {
const aliveAtDestroy = child.exitCode === null && child.signalCode === null;
stdin.destroy(epipe());
resolve({ aliveAtDestroy });
});
}
return accepted;
}) as typeof stdin.write;
});
}
async function startPersistentWorker() {
const before = forkedChildren.length;
const handle = createPluginWorkerHandle("test.plugin", {
@ -182,20 +231,18 @@ describe("plugin worker stdin command-channel supervision", () => {
handle.on("crash", (payload) => crashes.push(payload));
const exited = nextExit(child);
const stopping = handle.stop();
// stopInternal() sets intentionalStop before it writes the shutdown RPC.
await vi.waitFor(() => {
expect(handle.status).toBe("stopping");
// ...and the shutdown must have drained out of the host before the pipe
// is killed, or this would be testing a dropped shutdown instead.
expect(child.stdin?.writableLength ?? 0).toBe(0);
});
// Kill the command channel mid-stop, while the fixture is still inside its
// deferred-exit window. Without the intentionalStop guard this SIGKILLs a
// worker that was already shutting down cleanly.
child.stdin?.destroy(epipe());
const destroyed = destroyStdinOnShutdown(child);
const stopping = handle.stop();
const { aliveAtDestroy } = await destroyed;
expect(
aliveAtDestroy,
"fixture exited before the command channel was killed — the guard was never exercised",
).toBe(true);
await stopping;
const { code, signal } = await exited;