Skip to content

refactor(coding-agent): await the spawn event instead of a startup sleep - #1744

Closed
snimu wants to merge 6 commits into
snimu/heartbeat-cache-presencefrom
snimu/rpc-start-readiness
Closed

refactor(coding-agent): await the spawn event instead of a startup sleep#1744
snimu wants to merge 6 commits into
snimu/heartbeat-cache-presencefrom
snimu/rpc-start-readiness

Conversation

@snimu

@snimu snimu commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

What was wrong

RpcClient.start() guessed process readiness by sleeping 100ms and then sampling the exit code — a timer standing in for a real signal. Children that died at 150ms were "ready"; healthy children paid 100ms for nothing (audit: timeouts as synchronization, timeouts.md finding 6).

The fix

The sleep and exit-code sampling are deleted. start() awaits the child's OS spawn event (node:events.once) — the readiness signal the child actually emits. Pre-spawn errors surface through the recorded transport error with the existing message format; fast post-spawn exits surface on first use via the stdout-close handler with captured stderr — strictly more honest than the old sample. +14/−6.

How it's verified

Reviewer traced both failure orderings (error-before-spawn, exit-after-spawn), confirmed events.once cleans its listeners on both paths with no missed-event race, and validated the new fake-timer test is meaningful (old code hangs under fake timers, new code resolves). Focused RPC suites green; full CI-style failures match the stack base exactly (dist-build extras pass after building). Two-model implement/review loop, approved first pass.

Stacked on #1743 (test the whole stack at the leaf; merge base-first).

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


Note

Low Risk
Local change to RPC client startup synchronization with clearer event-driven readiness; no auth or data-handling impact, with a small behavior change for very fast child exits after spawn.

Overview
RpcClient.start() no longer waits 100ms or throws when exitCode is already set right after that delay. Startup readiness now awaits the child process "spawn" event via once from node:events, so healthy clients avoid a fixed latency and failures before spawn still surface through the existing transport error path.

Behavior shift: immediate post-start exit is no longer detected inside start(); a child that dies quickly after spawn is treated as started until stdout closes or the first RPC use fails. A changelog note and a fake-timer test lock in that start() completes from spawn without relying on timers (the old sleep would hang under fake timers).

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

Linear ticket: ENG-5662
(ticket linked above)

Note

Await child process spawn event instead of fixed 100ms sleep in RpcClient.start

Removes the hard-coded 100ms startup delay in rpc-client.ts. RpcClient.start() now waits for the child's "spawn" event via once(child, "spawn") and rethrows a stored transportError if one occurs. Adds a test confirming start resolves from the spawn signal without timer reliance.

  • Behavioral Change: start() no longer throws immediately on a non-null exitCode right after startup; it rejects only if an "error" event fires before spawn.

Macroscope summarized fd62101.

@snimu

snimu commented Aug 27, 2026

Copy link
Copy Markdown
Contributor Author

Superseded by #1827: GitHub's stack lock prevents retargeting stacked PRs, so this PR was recreated as a plain PR against main (same branch, same content, all review threads here are resolved). Please review/merge #1827 instead.

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