diff --git a/packages/coding-agent/src/tiny/title-client.ts b/packages/coding-agent/src/tiny/title-client.ts index b3a15e995..f575ff342 100644 --- a/packages/coding-agent/src/tiny/title-client.ts +++ b/packages/coding-agent/src/tiny/title-client.ts @@ -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 { diff --git a/packages/coding-agent/test/issue-1606-repro.test.ts b/packages/coding-agent/test/issue-1606-repro.test.ts index 1c85c6804..9b771b336 100644 --- a/packages/coding-agent/test/issue-1606-repro.test.ts +++ b/packages/coding-agent/test/issue-1606-repro.test.ts @@ -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(); + 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); });