From 5843a78dbfdac45fe9bed24bd75311778036a06b Mon Sep 17 00:00:00 2001 From: roboomp Date: Sun, 31 May 2026 21:02:25 +0000 Subject: [PATCH 1/2] fix(tiny): isolated tiny model worker in subprocess to skip onnxruntime napi crash Moved the tiny title/memory worker from a Bun Worker thread into a child process spawned via Bun.spawn IPC. The agent CLI gains a hidden --tiny-worker dispatch the parent invokes through process.execPath; the parent SIGKILLs the child on dispose so onnxruntime-node's NAPI finalizer never runs in any address space the agent owns. On Windows that finalizer was segfaulting Bun at shutdown after the tiny title model loaded (issue #1606). Drops the now-dead 'close'/'closed' handshake and the unused parentPort bootstrap, and removes tiny/worker.ts from --compile worker entries in both build scripts plus the regression test that pinned them. Fixes #1606 --- packages/coding-agent/CHANGELOG.md | 4 + packages/coding-agent/scripts/build-binary.ts | 1 - packages/coding-agent/src/cli.ts | 48 ++++++ .../coding-agent/src/tiny/title-client.ts | 156 +++++++++++++----- .../coding-agent/src/tiny/title-protocol.ts | 15 +- packages/coding-agent/src/tiny/worker.ts | 45 +---- .../test/issue-1150-repro.test.ts | 2 - .../test/issue-1606-repro.test.ts | 41 +++++ scripts/ci-release-build-binaries.ts | 1 - 9 files changed, 217 insertions(+), 96 deletions(-) create mode 100644 packages/coding-agent/test/issue-1606-repro.test.ts diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 9e22dabb5..04649eb11 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -2,6 +2,10 @@ ## [Unreleased] +### Fixed + +- Fixed `omp` segfaulting on exit on Windows after the tiny title/memory model loaded `onnxruntime-node` (issue [#1606](https://github.com/can1357/oh-my-pi/issues/1606)). The tiny model now runs in a Bun subprocess instead of a Worker thread, so the NAPI finalizer that crashes during shutdown never executes in the agent's address space; the subprocess is `SIGKILL`'d on dispose to skip every native destructor on every platform. + ## [15.7.4] - 2026-05-31 ### Removed diff --git a/packages/coding-agent/scripts/build-binary.ts b/packages/coding-agent/scripts/build-binary.ts index 893fa5530..d0b663f07 100644 --- a/packages/coding-agent/scripts/build-binary.ts +++ b/packages/coding-agent/scripts/build-binary.ts @@ -56,7 +56,6 @@ async function main(): Promise { "../stats/src/sync-worker.ts", "./src/tools/browser/tab-worker-entry.ts", "./src/eval/js/worker-entry.ts", - "./src/tiny/worker.ts", // Legacy pi-* extension compat entrypoints served by // `legacy-pi-compat.ts`. These are reached via computed bunfs paths // (which `--compile`'s static analyzer cannot trace), so each must be diff --git a/packages/coding-agent/src/cli.ts b/packages/coding-agent/src/cli.ts index 8f6f99c1c..1d7f06218 100755 --- a/packages/coding-agent/src/cli.ts +++ b/packages/coding-agent/src/cli.ts @@ -54,12 +54,60 @@ async function runSmokeTest(): Promise { process.stdout.write("smoke-test: ok\n"); } +/** + * Hidden subcommand that boots the tiny-model worker inside this process + * over the parent's IPC channel. The agent's main process spawns the same + * binary with this flag so `onnxruntime-node` (loaded transitively by + * `@huggingface/transformers`) lives in a child address space. The parent + * `SIGKILL`s the child on shutdown so the NAPI finalizer never runs in + * either process — that finalizer segfaults Bun on Windows (issue #1606). + */ +async function runTinyWorker(): Promise { + const { startTinyTitleWorker } = await import("./tiny/worker"); + const { promise: shuttingDown, resolve: shutdown } = Promise.withResolvers(); + const send = (message: unknown): void => { + // `process.send` only exists when spawned with an IPC channel; the + // parent always spawns us that way. If it's missing, the parent + // vanished and there's no one to talk to. + const sender = (process as NodeJS.Process & { send?: (m: unknown) => boolean }).send; + if (!sender) { + shutdown(); + return; + } + try { + sender.call(process, message); + } catch { + shutdown(); + } + }; + startTinyTitleWorker({ + send, + onMessage(handler) { + const wrap = (data: unknown): void => handler(data as never); + process.on("message", wrap); + return () => { + process.off("message", wrap); + }; + }, + }); + // Parent went away (crashed, SIGKILL, etc.) — commit suicide so we don't + // linger as an orphan. SIGKILL via `process.kill` keeps us symmetrical + // with the parent's hard-kill on shutdown: skip every JS/native finalizer. + process.on("disconnect", () => shutdown()); + await shuttingDown; + process.kill(process.pid, "SIGKILL"); +} + /** Run the CLI with the given argv (no `process.argv` prefix). */ export async function runCli(argv: string[]): Promise { if (argv[0] === "--smoke-test") { await runSmokeTest(); return; } + if (argv[0] === "--tiny-worker") { + await runTinyWorker(); + return; + } // --help and --version are handled by run() directly, don't rewrite those. // Everything else that isn't a known subcommand routes to "launch". const first = argv[0]; diff --git a/packages/coding-agent/src/tiny/title-client.ts b/packages/coding-agent/src/tiny/title-client.ts index 1382f40bd..b3a15e995 100644 --- a/packages/coding-agent/src/tiny/title-client.ts +++ b/packages/coding-agent/src/tiny/title-client.ts @@ -1,4 +1,6 @@ +import * as path from "node:path"; import { $env, isCompiledBinary, logger } from "@oh-my-pi/pi-utils"; +import type { Subprocess } from "bun"; import { settings } from "../config/settings"; import { tinyModelDeviceSettingToEnv } from "./device"; import { tinyModelDtypeSettingToEnv } from "./dtype"; @@ -12,6 +14,14 @@ import { } from "./models"; import type { TinyTitleProgressEvent, TinyTitleWorkerInbound, TinyTitleWorkerOutbound } from "./title-protocol"; +/** + * Abstraction over the tiny-model subprocess. Modelled as a worker interface + * so existing callers (titles, memory completions, downloads) compose the + * same way; the runtime implementation is a Bun child process so + * `onnxruntime-node`'s NAPI finalizer never runs inside the main agent + * address space — that destructor segfaults Bun on Windows during shutdown + * (issue #1606). + */ interface WorkerHandle { send(message: TinyTitleWorkerInbound): void; onMessage(handler: (message: TinyTitleWorkerOutbound) => void): () => void; @@ -31,6 +41,12 @@ export interface TinyTitleDownloadOptions { const SMOKE_TEST_TIMEOUT_MS = 5_000; +/** + * Hidden subcommand on the main CLI that boots the tiny-model worker in the + * spawned subprocess. Kept in sync with the dispatch in `cli.ts`. + */ +export const TINY_WORKER_ARG = "--tiny-worker"; + function readTinyModelSetting(path: "providers.tinyModelDevice" | "providers.tinyModelDtype"): string | undefined { try { const value = settings.get(path); @@ -66,49 +82,108 @@ export function tinyWorkerEnvOverlay( } /** - * Env handed to the tiny-model worker. The `PI_TINY_DEVICE` / `PI_TINY_DTYPE` env - * vars win; otherwise the persisted `providers.tinyModelDevice` / - * `providers.tinyModelDtype` settings are mapped onto those vars so the worker's - * env-based resolution picks them up. Resolved once at spawn (pipelines are cached). + * Env handed to the tiny-model subprocess. The `PI_TINY_DEVICE` / `PI_TINY_DTYPE` + * env vars win; otherwise the persisted `providers.tinyModelDevice` / + * `providers.tinyModelDtype` settings are mapped onto those vars so the + * subprocess's env-based resolution picks them up. Resolved once at spawn + * (pipelines are cached for the lifetime of the subprocess). */ -function tinyWorkerEnv(): Record | undefined { +function tinyWorkerEnv(): Record { const overlay = tinyWorkerEnvOverlay( $env, readTinyModelSetting("providers.tinyModelDevice"), readTinyModelSetting("providers.tinyModelDtype"), ); - if (Object.keys(overlay).length === 0) return undefined; - return { ...($env as Record), ...overlay }; + const base = $env as Record; + const merged: Record = {}; + for (const key in base) { + const value = base[key]; + if (typeof value === "string") merged[key] = value; + } + for (const key in overlay) merged[key] = overlay[key]; + return merged; } -export function createTinyTitleWorker(): Worker { - const env = tinyWorkerEnv(); - const options: WorkerOptions = env ? { type: "module", env } : { type: "module" }; - return isCompiledBinary() - ? new Worker("./packages/coding-agent/src/tiny/worker.ts", options) - : new Worker(new URL("./worker.ts", import.meta.url).href, options); +/** + * Resolve the argv used to relaunch the agent CLI into tiny-worker mode. In a + * compiled binary the entry point is the binary itself; in dev/source the + * spawned `bun` needs the absolute path to `cli.ts` so it can resolve module + * imports against the on-disk source tree. + */ +function tinyWorkerSpawnCmd(): string[] { + if (isCompiledBinary()) return [process.execPath, TINY_WORKER_ARG]; + const cliPath = path.resolve(import.meta.dir, "..", "cli.ts"); + return [process.execPath, cliPath, TINY_WORKER_ARG]; } -function wrapBunWorker(worker: Worker): WorkerHandle { - (worker as Worker & { unref?: () => void }).unref?.(); +interface SpawnedSubprocess { + proc: Subprocess<"ignore", "inherit", "inherit">; + inbound: Set<(message: TinyTitleWorkerOutbound) => void>; + errors: Set<(error: Error) => void>; +} + +/** + * Spawn the tiny-model worker as a subprocess. Exported for tests and the + * smoke probe; production callers go through {@link spawnTinyTitleWorker} + * which wraps the result in a {@link WorkerHandle}. + */ +export function createTinyTitleSubprocess(): SpawnedSubprocess { + const inbound = new Set<(message: TinyTitleWorkerOutbound) => void>(); + const errors = new Set<(error: Error) => void>(); + const proc = Bun.spawn({ + cmd: tinyWorkerSpawnCmd(), + env: tinyWorkerEnv(), + stdin: "ignore", + stdout: "inherit", + stderr: "inherit", + serialization: "advanced", + windowsHide: true, + ipc(message) { + 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}`); + 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 }; +} + +function wrapSubprocess({ proc, inbound, errors }: SpawnedSubprocess): WorkerHandle { return { send(message) { - worker.postMessage(message); + try { + proc.send(message); + } catch (error) { + logger.debug("tiny-title: send to subprocess failed", { + error: error instanceof Error ? error.message : String(error), + }); + } }, onMessage(handler) { - const wrap = (event: MessageEvent): void => handler(event.data as TinyTitleWorkerOutbound); - worker.addEventListener("message", wrap); - return () => worker.removeEventListener("message", wrap); + inbound.add(handler); + return () => inbound.delete(handler); }, onError(handler) { - const wrap = (event: ErrorEvent): void => { - handler(event.error instanceof Error ? event.error : new Error(event.message || "tiny title worker error")); - }; - worker.addEventListener("error", wrap); - return () => worker.removeEventListener("error", wrap); + errors.add(handler); + return () => errors.delete(handler); }, async terminate() { - worker.terminate(); + // SIGKILL: the whole point of the subprocess isolation is that the + // parent never runs `onnxruntime-node`'s NAPI finalizer. A polite + // 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. + try { + proc.kill("SIGKILL"); + } catch { + // Already gone. + } }, }; } @@ -126,10 +201,6 @@ function spawnInlineUnavailableWorker(error: unknown): WorkerHandle { emit({ type: "pong", id: message.id }); return; } - if (message.type === "close") { - emit({ type: "closed" }); - return; - } emit({ type: "error", id: message.id, error: errorMessage }); }); }, @@ -148,9 +219,9 @@ function spawnInlineUnavailableWorker(error: unknown): WorkerHandle { function spawnTinyTitleWorker(): WorkerHandle { try { - return wrapBunWorker(createTinyTitleWorker()); + return wrapSubprocess(createTinyTitleSubprocess()); } catch (error) { - logger.warn("Tiny title Worker spawn failed; local titles disabled", { + logger.warn("Tiny title worker spawn failed; local titles disabled", { error: error instanceof Error ? error.message : String(error), }); return spawnInlineUnavailableWorker(error); @@ -293,9 +364,9 @@ export class TinyTitleClient { } this.#pending.clear(); try { - worker?.send({ type: "close" }); + await worker?.terminate(); } catch { - // Worker may already be gone. + // Already gone. } } @@ -317,7 +388,6 @@ export class TinyTitleClient { this.#emitProgress(message.event); return; } - if (message.type === "closed") return; if (message.type === "pong") return; const pending = this.#pending.get(message.id); @@ -371,25 +441,25 @@ export async function smokeTestTinyTitleWorker({ }: { timeoutMs?: number; } = {}): Promise { - const worker = createTinyTitleWorker(); + const handle = wrapSubprocess(createTinyTitleSubprocess()); const { promise, resolve, reject } = Promise.withResolvers(); const timer = setTimeout(() => reject(new Error(`tiny title worker did not pong within ${timeoutMs}ms`)), timeoutMs); - worker.onmessage = (event: MessageEvent) => { - const message = event.data; + const unsubscribeMessage = handle.onMessage(message => { if (message.type === "pong") { resolve(); return; } + if (message.type === "log") return; reject(new Error(`tiny title worker: expected pong, got ${JSON.stringify(message)}`)); - }; - worker.onerror = (event: ErrorEvent) => { - reject(event.error instanceof Error ? event.error : new Error(event.message || "tiny title worker error")); - }; + }); + const unsubscribeError = handle.onError(reject); try { - worker.postMessage({ type: "ping", id: "smoke" } satisfies TinyTitleWorkerInbound); + handle.send({ type: "ping", id: "smoke" } satisfies TinyTitleWorkerInbound); await promise; } finally { clearTimeout(timer); - worker.terminate(); + unsubscribeMessage(); + unsubscribeError(); + await handle.terminate(); } } diff --git a/packages/coding-agent/src/tiny/title-protocol.ts b/packages/coding-agent/src/tiny/title-protocol.ts index 9267a0b89..4f1bd67ba 100644 --- a/packages/coding-agent/src/tiny/title-protocol.ts +++ b/packages/coding-agent/src/tiny/title-protocol.ts @@ -31,8 +31,7 @@ export type TinyTitleWorkerInbound = | { type: "ping"; id: string } | { type: "generate"; id: string; modelKey: TinyTitleLocalModelKey; message: string } | { type: "complete"; id: string; modelKey: TinyLocalModelKey; prompt: string; maxTokens?: number } - | { type: "download"; id: string; modelKey: TinyLocalModelKey } - | { type: "close" }; + | { type: "download"; id: string; modelKey: TinyLocalModelKey }; export type TinyTitleWorkerOutbound = | { type: "pong"; id: string } @@ -41,11 +40,17 @@ export type TinyTitleWorkerOutbound = | { type: "downloaded"; id: string } | { type: "error"; id: string; error: string } | { type: "progress"; id: string; event: TinyTitleProgressEvent } - | { type: "log"; level: "debug" | "warn" | "error"; msg: string; meta?: Record } - | { type: "closed" }; + | { type: "log"; level: "debug" | "warn" | "error"; msg: string; meta?: Record }; +/** + * Wire transport between the parent (`TinyTitleClient`) and the tiny-model + * subprocess. The parent owns the subprocess lifecycle (graceful work, hard + * kill on shutdown); the protocol therefore carries no explicit close + * handshake — once the parent decides to terminate, it signals the OS to + * reap the child so `onnxruntime-node`'s NAPI finalizer never runs in any + * shared address space. See `title-client.ts` for the spawn/kill glue. + */ export interface TinyTitleTransport { send(message: TinyTitleWorkerOutbound): void; onMessage(handler: (message: TinyTitleWorkerInbound) => void): () => void; - close(): void; } diff --git a/packages/coding-agent/src/tiny/worker.ts b/packages/coding-agent/src/tiny/worker.ts index 2d7a1fa83..118838a67 100644 --- a/packages/coding-agent/src/tiny/worker.ts +++ b/packages/coding-agent/src/tiny/worker.ts @@ -1,7 +1,6 @@ import * as fs from "node:fs/promises"; import { createRequire } from "node:module"; import * as path from "node:path"; -import { parentPort } from "node:worker_threads"; import type { ProgressInfo, TextGenerationPipeline, @@ -20,12 +19,7 @@ import { type TinyTitleLocalModelSpec, } from "./models"; import { formatTitleUserMessage, normalizeGeneratedTitle } from "./text"; -import type { - TinyTitleProgressEvent, - TinyTitleTransport, - TinyTitleWorkerInbound, - TinyTitleWorkerOutbound, -} from "./title-protocol"; +import type { TinyTitleProgressEvent, TinyTitleTransport, TinyTitleWorkerInbound } from "./title-protocol"; const TITLE_PREFILL = ""; const TITLE_CLOSE = ""; @@ -497,16 +491,6 @@ async function generateCompletion( return generated === "" ? null : generated; } -function releasePipelines(): void { - // Intentionally NOT calling `pipeline.dispose()`. transformers.js disposes the - // underlying onnxruntime InferenceSession, freeing native memory that Bun's - // worker/NAPI teardown then frees a second time — a double-free that aborts the - // process on quit ("malloc: pointer being freed was not allocated" / - // "NAPI FATAL ERROR"). The worker is torn down immediately after `close`, so the - // OS reclaims the model memory regardless; skipping dispose avoids the crash. - pipelines.clear(); -} - function enqueueRequest( transport: TinyTitleTransport, request: Extract, @@ -555,33 +539,6 @@ export function startTinyTitleWorker(transport: TinyTitleTransport): void { transport.send({ type: "pong", id: message.id }); return; } - if (message.type === "close") { - releasePipelines(); - transport.send({ type: "closed" }); - transport.close(); - return; - } enqueueRequest(transport, message); }); } - -if (!parentPort) throw new Error("tiny-title-worker: missing parentPort"); - -const port = parentPort; -const transport: TinyTitleTransport = { - send: (message: TinyTitleWorkerOutbound) => port.postMessage(message), - onMessage: handler => { - const wrap = (data: unknown): void => handler(data as TinyTitleWorkerInbound); - port.on("message", wrap); - return () => port.off("message", wrap); - }, - close: () => { - try { - port.close(); - } catch { - // Already closed. - } - }, -}; - -startTinyTitleWorker(transport); diff --git a/packages/coding-agent/test/issue-1150-repro.test.ts b/packages/coding-agent/test/issue-1150-repro.test.ts index ab00ecd70..777c58869 100644 --- a/packages/coding-agent/test/issue-1150-repro.test.ts +++ b/packages/coding-agent/test/issue-1150-repro.test.ts @@ -35,7 +35,6 @@ describe("issue #1150 — release-build script must list all worker --compile en "./packages/stats/src/sync-worker.ts", "./packages/coding-agent/src/tools/browser/tab-worker-entry.ts", "./packages/coding-agent/src/eval/js/worker-entry.ts", - "./packages/coding-agent/src/tiny/worker.ts", ]; it("scripts/ci-release-build-binaries.ts lists every worker as an explicit --compile entrypoint", async () => { @@ -56,7 +55,6 @@ describe("issue #1150 — release-build script must list all worker --compile en "../stats/src/sync-worker.ts", "./src/tools/browser/tab-worker-entry.ts", "./src/eval/js/worker-entry.ts", - "./src/tiny/worker.ts", ]; const source = await Bun.file(devScriptPath).text(); for (const entry of devEntrypoints) { diff --git a/packages/coding-agent/test/issue-1606-repro.test.ts b/packages/coding-agent/test/issue-1606-repro.test.ts new file mode 100644 index 000000000..1c85c6804 --- /dev/null +++ b/packages/coding-agent/test/issue-1606-repro.test.ts @@ -0,0 +1,41 @@ +/** + * Regression for https://github.com/can1357/oh-my-pi/issues/1606 + * + * On Windows, `onnxruntime-node`'s NAPI finalizer segfaults Bun during + * shutdown after `@huggingface/transformers` has loaded a tiny model in a + * Worker thread. The agent used to host the tiny-model worker as a Worker + * inside its own process; tearing the worker down ran the native destructor + * in the parent's address space and crashed the CLI on exit. + * + * The fix relocates the worker to a child process: `title-client.ts` spawns + * `process.execPath … --tiny-worker`, `cli.ts` dispatches that flag into + * `runTinyWorker`, and the parent `SIGKILL`s the child on dispose so the + * native finalizer never runs in either address space. These tests pin the + * three pieces of that contract so a future refactor cannot quietly land + * the original crash again. + */ +import { describe, expect, it } from "bun:test"; +import { 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 () => { + // `smokeTestTinyTitleWorker` is the runtime probe wired into + // `omp --smoke-test`: it spawns the worker subprocess via + // `Bun.spawn`, sends a ping over the IPC channel, awaits the pong, + // then SIGKILLs the child. If anyone reverts the worker to an + // in-process `new Worker(...)` thread or drops the `--tiny-worker` + // CLI dispatch, the spawn either picks up the wrong entrypoint or + // the ping never round-trips, and this test fails. + await expect(smokeTestTinyTitleWorker({ timeoutMs: 15_000 })).resolves.toBeUndefined(); + }, 30_000); + + it("CLI dispatches the flag that `title-client.ts` passes to the spawned child", async () => { + // `tinyWorkerSpawnCmd()` and the cli switch must agree on the exact + // flag, character-for-character — the spawned `bun`/binary sees only + // `argv` and there is no fallback path that "re-routes" the worker + // on misnamed flags. Pin the spelling on both ends. + const cliSource = await Bun.file(new URL("../src/cli.ts", import.meta.url)).text(); + expect(cliSource).toContain(`argv[0] === "${TINY_WORKER_ARG}"`); + expect(cliSource).toContain("runTinyWorker"); + }); +}); diff --git a/scripts/ci-release-build-binaries.ts b/scripts/ci-release-build-binaries.ts index fad13b13a..ab72a8ec1 100644 --- a/scripts/ci-release-build-binaries.ts +++ b/scripts/ci-release-build-binaries.ts @@ -27,7 +27,6 @@ const workerEntrypoints = [ "./packages/stats/src/sync-worker.ts", "./packages/coding-agent/src/tools/browser/tab-worker-entry.ts", "./packages/coding-agent/src/eval/js/worker-entry.ts", - "./packages/coding-agent/src/tiny/worker.ts", ]; const isDryRun = process.argv.includes("--dry-run"); const targets: BinaryTarget[] = [ From e69e8b54f86abd6e2f595b409c556d4b288bbebb Mon Sep 17 00:00:00 2001 From: roboomp Date: Sun, 31 May 2026 21:09:07 +0000 Subject: [PATCH 2/2] fix(tiny): surfaced unexpected subprocess signal exits as worker errors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../coding-agent/src/tiny/title-client.ts | 30 +++++++++-- .../test/issue-1606-repro.test.ts | 50 ++++++++++++++++++- 2 files changed, 74 insertions(+), 6 deletions(-) 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); });