fix(ai): a broken RPC pipe must fail the provider, not kill the host (#1378) - #1379
Conversation
…1378) Opening a plan review from Pi on Windows could exit the entire Pi host with an uncaught `write EPIPE` raised inside `PiProcessNode.send()`. The provider checked `stdin.destroyed` and then wrote, which cannot close the race: the nested `pi --mode rpc` child can close the pipe between the check and the write. Node then reports EPIPE either as a synchronous throw or as an `error` event on the stream, and because no stream had an `error` listener that became an `uncaughtException` and terminated the host agent process. Add a shared guard (`packages/ai/providers/child-io.ts`) and apply it to both JSONL/JSON-RPC providers: - `guardChildStreams` attaches `error` listeners to the child and every pipe immediately after spawn, so a stream error can never escalate. The previous one-shot spawn listener was removed on success, leaving the child with no `error` listener for the rest of its life. - `writeChildLine` reports a synchronous failure through its return value and an asynchronous one through the write callback, so both paths converge. - A failure now resolves as a provider failure: in-flight requests reject, the process end is broadcast to listeners so a streaming query terminates, the child is reaped, and `alive` flips false so the next query re-spawns. Previously a failed write also left `sendAndWait` pending forever, because `send()` was fire-and-forget and the Pi provider has no RPC timeout. Also guards the Bun variant's FileSink write/flush symmetrically, and switches Pi's Node stderr from an un-drained "pipe" to "ignore", matching the deadlock reasoning already documented in codex-app-server.ts. Regression test runs the provider in a real `node` child against a fake Pi that closes its own stdin; the child installs no `uncaughtException` handler, so surviving to print its results is the proof. Against the unfixed provider that child dies with `Error: write EPIPE`, exit 1. Reported by @Kaelenx.
|
Thanks for the quick fix and for crediting the report. I’m available to validate it on Windows 11 with Pi 0.84.2 if useful. |
|
That would genuinely help, since the crash manifests most readily on Windows and our CI exercises the guard with simulated pipes. To test the branch build: clone the repo, checkout fix/1378-pi-provider-stdio-epipe (or fetch the PR head), run bash apps/pi-extension/vendor.sh, then point Pi at the local extension path instead of the npm package and repeat your repro from the issue. Expected: the review page failure surfaces as an Ask AI provider error inside the session, Pi stays alive, and the next query works. If you see any exit or stack trace at all, paste it here. It ships in v0.27.7 either way, but a confirmation from the exact machine that crashed is the best evidence we can get. |
|
Verified on the original Windows environment against commit Environment:
I built the local Pi extension and exercised the full plan-review startup path under a real Pi host. To make the timing deterministic, I put a native Windows fake Control result on base PR result with the same harness:
I also ran the provider child harness directly with a native Windows broken pipe. It reported the in-flight request rejected with Additional checks:
So this fixes the reported host-killing failure on the machine/runtime where it occurred. |
2 participants
TLDR
Opening a Plannotator plan review from Pi could kill the entire Pi host with an uncaught
write EPIPE. A broken pipe to the nestedpi --mode rpcchild is now handled as an ordinary provider failure: the in-flight query rejects with a clear message, the error surfaces in Ask AI, the provider is marked dead so the next query re-spawns it, and the host stays alive.Closes #1378. Reported by @Kaelenx, who also did the root-cause analysis in the issue and pointed at the exact line.
What was actually going wrong
The provider guarded its write like this:
Two separate problems compound into a host kill.
The check cannot close the race.
destroyeddescribes the stream a moment ago. The child can close its end of the pipe between the check and the write, and no amount of pre-checking fixes that; the write itself is the only place the truth is known.Nothing was listening for the failure. Node reports EPIPE from a pipe write either as a synchronous throw or as an
errorevent on the stream, and which one you get is a timing race. Anerrorevent on a Node stream with no listener is not "an error you can ignore"; Node re-throws it as anuncaughtException. Inside an embedded extension that does not fail the provider, it terminates the process that loaded it, which is the user's whole Pi session.No stream here had an
errorlistener. TheChildProcessdid not have one either: the spawn handshake registered a one-shoterrorlistener and then removed it on success, so from the moment a spawn succeeded the child was completely unguarded for the rest of its life.Windows only made it likelier to land on the async path during teardown, which is why the report came from there. The mechanism is platform-independent.
There was a second, quieter bug behind the same line.
send()was fire-and-forget, so a write that failed left the matchingsendAndWaitpromise pending forever rather than rejecting. The Pi provider has no RPC timeout, so nothing else would have released it.The guard pattern
The two JSONL/JSON-RPC providers (Pi Node, Codex app-server) had byte-for-byte the same unguarded
send(), so rather than patching the reported line twice, the pattern is fixed once in a shared module,packages/ai/providers/child-io.ts:guardChildStreams(proc, label, onFailure)attacheserrorlisteners to the child process and every piped stream, immediately after spawn and before the spawn handshake, so nothing can escalate touncaughtException.writeChildLine(proc, line, label, onAsyncFailure)returns the error for a synchronous failure and routes an asynchronous one through the write callback, so the sync-throw and error-event paths converge on the same handling.failProcess(error)in each provider resolves a broken pipe exactly like any other process end: reject everything in flight, broadcast the process end so a streaming query terminates instead of hanging, reap the child, and leavealivefalse so the existing lifecycle re-spawns on the next query.I looked for a stronger pattern to copy first.
claude-agent-sdkspawns nothing of its own,opencode-sdkdelegates to its SDK, andcodex-app-serverhad the identical defect, so there was no good pattern among the siblings to match; the best guards in the repo are elsewhere (vcs.ts,semantic-diff.ts,call-flow.ts) and are swallow-only, which is right for a fire-and-forget CLI write but wrong here, where the failure has to become a visible provider error.Scope
In this PR, all in
packages/ai/providers/:pi-sdk-node.ts(the reported crash),codex-app-server.ts(identical defect, and the mechanism previously reported in Plan-review crashes pi with unhandled EPIPE from Codex app-server stdin #1039 / fix(ai): swallow async EPIPE on codex app-server stdin (crashes host) #1040),pi-sdk.ts(the Bun variant'sFileSinkwrite/flush, guarded symmetrically, including a flush that returns a promise).stderrmoves from an un-drained"pipe"to"ignore". Nothing ever read it, and a full stderr pipe deadlocks the child. This is the reasoning already documented incodex-app-server.ts; the Pi variant had simply not picked it up.Deliberately not in this PR, found while sweeping for the same class and reported separately so the diff stays reviewable: unguarded
stdinwrites inapps/pi-extension/server/agent-jobs.tsandserver/pr.ts, two fire-and-forgetspawn("open", ...)calls inserver/integrations.tswith noerrorlistener at all (an ENOENT there is an immediate host kill on any machine withoutopen), refusal-branch socket writes inlive-proxy-node.tsthat return before the socket'serrorlistener is attached, and the missing RPC timeout that lets a silent-but-alive child hang a Pi query indefinitely.Test evidence
packages/ai/providers/pi-stdio-failure.test.ts, in two layers.The end-to-end block runs the provider in a real
nodechild against a fake Pi that closes its own stdin, following the harness already used bylive-proxy-node.test.ts. Node is the runtime under Pi, and Bun'snode:streamshims do not reproduce the unhandled-error-kills-the-process rule, so an in-process test could pass against code that still kills the host. The child installs nouncaughtExceptionhandler on purpose: surviving to print its result line is the assertion.It asserts the in-flight request rejects naming the pipe,
aliveis false, listeners saw the process end, a later fire-and-forget send does not throw, a fresh process talks normally afterwards, and end to end through a session the failure surfaces as an error message while the next query on the same session re-spawns and completes.Against the unfixed provider that child dies exactly as reported:
With the fix, exit 0 and empty stderr.
The unit block drives
writeChildLine/guardChildStreamswith stream doubles, which is the only way to pin both shapes deterministically, since which one a real pipe produces is a platform and timing race: a synchronous throw fromwrite(), and an asynchronous error delivered to the write callback.Checks run: 6/6 new tests pass;
bun test packages/ai/122/122; fullbun test3819 pass with 15 failures that reproduce identically on unmodifiedorigin/main(config-from-disk, improvement-hook and route-404 suites, environmental);bun run typecheckclean across all nine projects;vendor.shre-run withchild-ioadded to the provider list sogenerated/ai/providers/reflects the fix;npm pack --dry-runcomplete withgenerated/ai/providers/child-io.tsshipping.AI-assisted (Claude) under maintainer direction.