Skip to content

fix(coding-agent): remove blanket RPC timeouts, reject on real boundaries - #1840

Closed
snimu wants to merge 6 commits into
mainfrom
snimu/remove-rpc-blanket-timeouts
Closed

fix(coding-agent): remove blanket RPC timeouts, reject on real boundaries#1840
snimu wants to merge 6 commits into
mainfrom
snimu/remove-rpc-blanket-timeouts

Conversation

@snimu

@snimu snimu commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

What was wrong

Every RPC call had a hidden 30-second deadline (10 minutes for refine), and waiting for the agent to finish had a hidden 60-second one. Long-but-healthy commands — like a Bash call that legitimately takes minutes — got cancelled on the client while they kept running on the server. And because several timers overlapped, the same operation could be bounded three different ways.

The fix

  • Removed all the default deadlines. Callers can still pass a timeout explicitly if they want one.
  • Instead of timers guessing, failures now come from the places that actually know: the child process dying, its output stream closing, or an explicit stop(). Anything still waiting gets a clear error at that moment.
  • Replaced the test that asserted the old 10-minute refine deadline with three tests for what actually matters: a long command finishes, a long turn finishes, and everything pending fails cleanly when the process dies.

How it's verified

Type checks, lint, focused suites, and the repo-wide check pass. The process-death tests were initially flaky (two shutdown events race each other); the assertion was fixed and then run 11 times in a row without failure. Written and independently reviewed by two different models before opening this PR.

Note: intentionally no Linear ticket for this cleanup stack, so that check stays red.


Note

Medium Risk
Behavior change: callers that relied on implicit RPC/idle timeouts now wait indefinitely until transport failure unless they pass timeout; lifecycle fixes are complex but heavily tested.

Overview
Removes default client-side deadlines on RpcClient RPC calls and agent-turn waits so long-running bash, refine, and multi-minute turns are not cancelled while the server is still healthy. send() no longer applies a 30s (or 10-minute refine) timer; waitForIdle, collectEvents, and promptAndWait only time out when callers pass an explicit timeout.

Pending work now fails on real transport boundaries instead of overlapping timers. A latched transportError plus failPendingOperations() rejects in-flight commands and event waiters when the child errors, stdout closes, the process exits (with a short stdout drain so last responses can still resolve), stop(), or start() replaces a generation. Handlers are scoped per child so restarts and grandchild-held stdio pipes cannot let stale output resolve new-session waiters.

prompt() awaits the RPC response and surfaces command failures via getData. promptAndWait uses cancellable startEventCollection() so prompt rejection does not leave idle waiters hanging.

Tests replace the refine-timeout assertion with a hanging fixture and suites covering no default timeout, process death, restart isolation, and ghost events.

Reviewed by Cursor Bugbot for commit 6bf726d. Bugbot is set up for automated code reviews on this repo. Configure here.

Note

Remove blanket RPC timeouts in RpcClient, reject on transport failure boundaries

  • Removes the per-request 30s timeout and the 10-minute REFINE_REQUEST_TIMEOUT_MS constant; send() and refine() now wait indefinitely until a response arrives or the transport fails.
  • Adds failPendingOperations which latches the first transport error and rejects all pending requests and event waiters when the child process exits, stdout closes, or an error occurs.
  • prompt() now awaits the command response and throws on rejection instead of being fire-and-forget.
  • start() cancels prior pending operations before spawning a new child; stop() always resolves within ~1s via SIGKILL fallback even when a grandchild holds stdio pipes open.
  • Behavioral Change: waitForIdle(), collectEvents(), and promptAndWait() no longer time out by default and reject on transport failure; callers relying on the old 30s/10min timeouts must pass an explicit timeout.

Macroscope summarized 6bf726d.

Linear: ENG-5647

Comment thread packages/coding-agent/src/modes/rpc/rpc-client.ts
Comment thread packages/coding-agent/src/modes/rpc/rpc-client.ts
…es cannot hang shutdown

stop() waited for the child 'close' event with no fallback: a grandchild
inheriting the stdio pipes (or a child that never dies) kept 'close' from
firing, hanging stop() forever and leaving this.process set so a later
start() threw 'Client already started'. Wait on 'exit' instead, resolve
after the SIGKILL escalation as a fallback, and clear this.process
explicitly (guarded against a restarted child).
Comment thread packages/coding-agent/src/modes/rpc/rpc-client.ts Outdated
Comment thread packages/coding-agent/src/modes/rpc/rpc-client.ts Outdated
Comment thread packages/coding-agent/src/modes/rpc/rpc-client.ts Outdated
snimu added 2 commits August 27, 2026 15:41
…hild

Late error/stdout-close events from a replaced child could set
transportError on a freshly restarted client, rejecting its pending work.
Guard every child handler with this.process === child, fail pending
operations on 'exit' so a grandchild holding stdout cannot leave requests
and idle waiters hanging, and finalize on 'close' as well so a failed
spawn (which emits 'error'/'close' but never 'exit') clears this.process
and start() can be retried.
…n child exit

Node may emit 'exit' while a complete response is still buffered in the
stdout pipe (docs guarantee only that 'close' runs after the streams
drain), so the exit finalizer could reject a request whose answer was
already in flight. Defer failPendingOperations until stdout closes, with
a 1s unref'd fallback so pipes held open by a grandchild still fail
promptly, and skip the deferred failure if the client was restarted.
Comment thread packages/coding-agent/src/modes/rpc/rpc-client.ts
…start

If start() ran inside a dead child's stdout-drain window, the deferred
failure skipped (anti-poisoning guard) and the old generation's pending
requests and idle waiters hung forever; the old JSONL reader also stayed
attached, so late output from the dead child's pipe could resolve the new
session's waiters. start() now detaches the previous reader and fails any
leftover pending operations synchronously before spawning.

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit f1a560e. Configure here.

Comment thread packages/coding-agent/test/fixtures/rpc-client-hanging-fixture.mjs
The ghost grandchild now also writes a sentinel to the inherited stderr,
which still feeds the client's shared stderr accumulator after restart.
The test awaits the sentinel before asserting the new session's waiter is
untouched, so the still-pending assertion cannot pass vacuously if the
stdin-EOF trigger ever stops firing.
@snimu

snimu commented Aug 28, 2026

Copy link
Copy Markdown
Contributor Author

Folded into #1854: that branch already contains this PR's commits (the two changes share code and were sequenced as a merge-after chain). Merging #1854 lands both; per snimu's preference the chain is collapsed into one merge unit. The review history here remains valid for the contained commits.

@snimu snimu closed this Aug 28, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant