Retry an agent CLI that dies before answering initialize - #630
Open
Yuandi (DDKinger) wants to merge 1 commit into
Open
Retry an agent CLI that dies before answering initialize#630Yuandi (DDKinger) wants to merge 1 commit into
Yuandi (DDKinger) wants to merge 1 commit into
Conversation
Switching to an agent whose ACP adapter is launched through npx can fail
the first time with:
agent CLI unavailable: ACP initialize failed for
'npx -y @agentclientprotocol/claude-agent-acp@0.65.0':
Internal error: "response to `initialize` never received: oneshot canceled"
npx downloads and unpacks the adapter into the npm cache on first use.
When node starts before every file has landed, npm reports the partial
extraction as TAR_ENTRY_ERROR *warnings* and still launches the adapter,
which then dies with ERR_MODULE_NOT_FOUND. Master only observes its ACP
transport closing before `initialize` returns, so it reports the agent as
unavailable and the user has to restart the pane by hand. The cache is
complete by that point, so the very next attempt succeeds in seconds --
in the reported incident a second helper happened to trigger one 17ms
later and connected in 3.5s.
Retry once, scoped to exactly that failure shape:
* Classify a failed spawn as ExitedBeforeInitialize vs Fatal, deciding on
whether the child process had already exited when `initialize` failed.
A CLI that is still running answered with a real protocol error, and a
timeout already cost the user a full wait, so neither is retried.
* Re-enter the pool lookup on the retry instead of reusing the first
attempt's cell, which its reaper may already have evicted.
* Make `reap_agent` instance-aware: it now keeps a pool entry whose cell
holds a *different* CLI instance than the one it is reaping. Without
this, a reaper racing a successful retry would strand a live agent
outside the pool and make the next helper spawn a duplicate. An empty
cell still belongs to the reaper, so ordinary eviction is unchanged.
Tests: two new cases cover the instance guard in both directions; the
existing reap_agent coverage is unchanged.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 3ffabfe2-7838-472f-a7d2-e7b122607da3
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves WTA master resiliency when an ACP agent CLI (notably npx-launched adapters) dies before replying to initialize, by retrying once for that specific failure shape and hardening pool reaping to avoid evicting a successfully republished agent instance.
Changes:
- Add a classified spawn failure (
SpawnFailure) and implement a single retry path when the agent process exits beforeinitializecompletes. - Refactor spawning into a helper (
spawn_into_pool_cell) and adjustspawn_one_agentto return retry classification. - Make
reap_agentinstance-aware and add unit tests covering “stale reaper vs retry republish” and “ordinary eviction still works”.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tools/wta/src/master/mod.rs | Adds cold-start retry logic + spawn failure classification; updates reaping to preserve republished instances. |
| tools/wta/src/master/tests.rs | Adds new coverage for the instance-aware reaper behavior in both directions. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+3301
to
+3304
| /// The retry re-enters [`spawn_into_pool_cell`] rather than reusing the first | ||
| /// attempt's cell: that attempt's reaper may have already evicted it, and | ||
| /// racing it would publish a live CLI into a cell no longer in the pool. | ||
| async fn spawn_agent_with_cold_start_retry( |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Switching the agent pane to an agent whose ACP adapter is launched through
npxcan fail on the first attempt:npxdownloads and unpacks the adapter into the npm cache on first use. When node starts before every file has landed, npm reports the partial extraction asTAR_ENTRY_ERRORwarnings and still launches the adapter, which then dies withERR_MODULE_NOT_FOUND. Master only observes the ACP transport closing beforeinitializereturns, so it reports the agent as unavailable and the user has to restart the pane by hand.From
wta-main_master.login the reported incident:The cache is complete by the time the first attempt dies, so the very next attempt succeeds. Here a second helper happened to trigger one 17 ms later and connected fine — with a single pane open, nothing does, and the user just sees a broken pane.
Fix
Retry once, scoped to exactly that failure shape.
spawn_one_agentnow returnsSpawnFailure::{ExitedBeforeInitialize, Fatal}, decided by whether the child process had already exited wheninitializefailed. A CLI that is still running answered with a real protocol error, and a timeout already cost the user a full 60 s wait — neither is retried, so a missing/denied/unresponsive CLI fails just as fast as before.spawn_into_pool_cell) instead of reusing the first attempt'sOnceCell, which its reaper may already have evicted.reap_agentinstance-aware. It now keeps a pool entry whose cell holds a different CLI instance than the one being reaped. Without this, a reaper racing a successful retry would strand a live agent outside the pool and make the next helper spawn a duplicate CLI. An empty cell still belongs to the reaper, so ordinary eviction is unchanged.Testing
Two new cases cover the instance guard in both directions:
reap_agent_preserves_cell_republished_by_cold_start_retry— a stale reaper must not evict the CLI a retry just published.reap_agent_evicts_cell_holding_its_own_instance— the guard does not weaken ordinary eviction.Existing
reap_agentcoverage is unchanged and still passes, including the "uninitialized cell is still evicted" path.Also verified against the live repro: after the npm cache self-healed,
npx -y @agentclientprotocol/claude-agent-acp@0.65.0answersinitializeimmediately — confirming the second attempt is the one that would have succeeded.Notes
The root cause is an npm cache extraction race, not an Intelligent Terminal bug. This change makes the product resilient to it rather than surfacing a one-shot cold-start failure to the user.