fix(tiny): surfaced unexpected subprocess signal exits as worker errors
The first cut at the subprocess isolation swallowed every signal exit (`exitCode === null`) on the assumption it was the intentional SIGKILL from `terminate()`. That misclassifies real worker deaths — SIGSEGV from a native crash, SIGKILL from the OOM killer, an operator `kill -9` — so any in-flight title/completion/download promise would await forever while `#worker` still pointed at a dead process. Added an `intentionalExit` flag flipped by `wrapSubprocess.terminate()` right before its SIGKILL. `onExit` swallows only the flagged exit; every other signal exit now fires the `errors` channel with a "signal SIGFOO" message so `TinyTitleClient.#handleWorkerError` clears `#pending` and dumps the dead worker handle. Added two regression tests pinning both branches. Reported by chatgpt-codex-connector on #1607.
This commit is contained in:
@@ -120,6 +120,13 @@ interface SpawnedSubprocess {
|
||||
proc: Subprocess<"ignore", "inherit", "inherit">;
|
||||
inbound: Set<(message: TinyTitleWorkerOutbound) => void>;
|
||||
errors: Set<(error: Error) => void>;
|
||||
/**
|
||||
* Flipped to `true` by {@link wrapSubprocess}'s `terminate()` right
|
||||
* before it SIGKILLs the child so `onExit` can distinguish the
|
||||
* expected hard-kill from a crash/OOM/external signal. Only the
|
||||
* latter is surfaced as a worker error.
|
||||
*/
|
||||
intentionalExit: { value: boolean };
|
||||
}
|
||||
|
||||
/**
|
||||
@@ -130,6 +137,7 @@ interface SpawnedSubprocess {
|
||||
export function createTinyTitleSubprocess(): SpawnedSubprocess {
|
||||
const inbound = new Set<(message: TinyTitleWorkerOutbound) => void>();
|
||||
const errors = new Set<(error: Error) => void>();
|
||||
const intentionalExit = { value: false };
|
||||
const proc = Bun.spawn({
|
||||
cmd: tinyWorkerSpawnCmd(),
|
||||
env: tinyWorkerEnv(),
|
||||
@@ -142,19 +150,28 @@ export function createTinyTitleSubprocess(): SpawnedSubprocess {
|
||||
for (const handler of inbound) handler(message as TinyTitleWorkerOutbound);
|
||||
},
|
||||
onExit(_proc, exitCode, signalCode) {
|
||||
if (exitCode === 0 || exitCode === null) return;
|
||||
const signalSuffix = signalCode ? ` (signal ${signalCode})` : "";
|
||||
const err = new Error(`tiny model subprocess exited with code ${exitCode}${signalSuffix}`);
|
||||
// Clean exit. The child only exits via SIGKILL in practice, but
|
||||
// treat code 0 as a no-op for symmetry.
|
||||
if (exitCode === 0) return;
|
||||
// `exitCode === null` + non-null `signalCode` covers both the
|
||||
// expected SIGKILL from `terminate()` AND external kills
|
||||
// (SIGSEGV from a native crash, SIGKILL from the OOM killer, an
|
||||
// operator `kill -9`, etc.). Swallow only the expected one;
|
||||
// every other signal exit is a real worker death that must
|
||||
// fault every in-flight request so callers don't await forever.
|
||||
if (exitCode === null && intentionalExit.value) return;
|
||||
const reason = exitCode !== null ? `code ${exitCode}` : `signal ${signalCode ?? "unknown"}`;
|
||||
const err = new Error(`tiny model subprocess exited with ${reason}`);
|
||||
for (const handler of errors) handler(err);
|
||||
},
|
||||
});
|
||||
// Don't keep the parent event loop alive on account of an idle worker; the
|
||||
// agent dispose path calls `terminate()` explicitly when shutting down.
|
||||
proc.unref();
|
||||
return { proc, inbound, errors };
|
||||
return { proc, inbound, errors, intentionalExit };
|
||||
}
|
||||
|
||||
function wrapSubprocess({ proc, inbound, errors }: SpawnedSubprocess): WorkerHandle {
|
||||
function wrapSubprocess({ proc, inbound, errors, intentionalExit }: SpawnedSubprocess): WorkerHandle {
|
||||
return {
|
||||
send(message) {
|
||||
try {
|
||||
@@ -179,6 +196,9 @@ function wrapSubprocess({ proc, inbound, errors }: SpawnedSubprocess): WorkerHan
|
||||
// SIGTERM lets the subprocess try to clean up, which is exactly the
|
||||
// codepath that crashes Bun on Windows. Hard-kill instead — the
|
||||
// model lives in process memory and the OS reclaims everything.
|
||||
// Flip the intentional-exit flag *before* killing so `onExit` can
|
||||
// tell this apart from a crash or external SIGKILL.
|
||||
intentionalExit.value = true;
|
||||
try {
|
||||
proc.kill("SIGKILL");
|
||||
} catch {
|
||||
|
||||
@@ -15,7 +15,7 @@
|
||||
* the original crash again.
|
||||
*/
|
||||
import { describe, expect, it } from "bun:test";
|
||||
import { smokeTestTinyTitleWorker, TINY_WORKER_ARG } from "../src/tiny/title-client";
|
||||
import { createTinyTitleSubprocess, smokeTestTinyTitleWorker, TINY_WORKER_ARG } from "../src/tiny/title-client";
|
||||
|
||||
describe("issue #1606 — tiny model lives in an isolated subprocess", () => {
|
||||
it("ping/pongs through the spawned worker subprocess and tears it down cleanly", async () => {
|
||||
@@ -38,4 +38,52 @@ describe("issue #1606 — tiny model lives in an isolated subprocess", () => {
|
||||
expect(cliSource).toContain(`argv[0] === "${TINY_WORKER_ARG}"`);
|
||||
expect(cliSource).toContain("runTinyWorker");
|
||||
});
|
||||
|
||||
it("surfaces unexpected signal exits so in-flight callers don't await forever", async () => {
|
||||
// If the child dies from a signal we did NOT request — SIGSEGV from a
|
||||
// native crash (the original Windows shutdown bug, now relocated to
|
||||
// the child), an OOM SIGKILL, or an operator `kill -9` — the
|
||||
// subprocess wrapper must fault every in-flight request via the
|
||||
// `errors` channel. The original fix swallowed any `exitCode === null`
|
||||
// exit unconditionally, which left `TinyTitleClient.#pending`
|
||||
// promises hanging forever. Pin the new contract: an external
|
||||
// SIGKILL (no `intentionalExit` flip) MUST surface a worker error.
|
||||
const sub = createTinyTitleSubprocess();
|
||||
try {
|
||||
const { promise, resolve } = Promise.withResolvers<Error>();
|
||||
sub.errors.add(resolve);
|
||||
sub.proc.kill("SIGKILL");
|
||||
const err = await promise;
|
||||
expect(err.message).toMatch(/signal/i);
|
||||
} finally {
|
||||
// Ensure the child is reaped even on assertion failure.
|
||||
try {
|
||||
sub.proc.kill("SIGKILL");
|
||||
} catch {}
|
||||
await sub.proc.exited;
|
||||
}
|
||||
}, 15_000);
|
||||
|
||||
it("does not surface intentional terminate() SIGKILLs as worker errors", async () => {
|
||||
// Inverse of the previous test: a SIGKILL issued by the wrapper's
|
||||
// own `terminate()` MUST NOT fault callers — terminate is the
|
||||
// shutdown path and the worker handle is already torn down by then.
|
||||
// Regression guard against an over-eager fix that surfaces every
|
||||
// signal exit indiscriminately.
|
||||
const sub = createTinyTitleSubprocess();
|
||||
let errored = false;
|
||||
sub.errors.add(() => {
|
||||
errored = true;
|
||||
});
|
||||
// Simulate what `wrapSubprocess.terminate()` does: flip the flag,
|
||||
// then SIGKILL. We test the primitive directly rather than going
|
||||
// through the wrapper to avoid coupling to `WorkerHandle` internals.
|
||||
sub.intentionalExit.value = true;
|
||||
sub.proc.kill("SIGKILL");
|
||||
await sub.proc.exited;
|
||||
// Give onExit a microtask to drain — Bun's exited promise resolves
|
||||
// after onExit fires, but be defensive.
|
||||
await Bun.sleep(20);
|
||||
expect(errored).toBe(false);
|
||||
}, 10_000);
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user