Skip to content

fix(agent_servers): serialize ACP session-creation per connection - #50

Merged
lukemarsden merged 2 commits into
mainfrom
fix/serialize-acp-session-creation
May 11, 2026
Merged

fix(agent_servers): serialize ACP session-creation per connection#50
lukemarsden merged 2 commits into
mainfrom
fix/serialize-acp-session-creation

Conversation

@lukemarsden

Copy link
Copy Markdown

Summary

Adds a per-AcpConnection chain so concurrent new_session / load_session / resume_session calls dispatch their ACP RPCs sequentially rather than in parallel.

This is the next layer of the recurring "chrome MCP not working in spec tasks" bug (see helixml/helix#2352 for the corresponding sandbox-versions bump).

Why

Two concurrent calls to connection.new_session() cause claude-agent-acp to spawn two child claude SDK processes back-to-back. Each child runs npx <pkg>@latest in parallel for stdio MCP servers (chrome-devtools-mcp, server-github, etc). npm tarball extraction isn't safe under concurrent invocations on the same _npx cache directory: the loser silently fails to register its MCP child, leaving the active claude process without (e.g.) chrome-devtools-mcp.

Reproduced in two distinct spec-task containers:

  • ubuntu-external-01kqvsbtsb4nc7pr14dy1zzfk9--resume <real> claude has no chrome-devtools-mcp, ghost --session-id <new> claude does
  • ubuntu-external-01kpz66jwq1vk8ctzrj054byqq — same pattern (different IDs)

PR #46 deduped at the wrapper level (one claude-agent-acp per (project, agent_id)). PR #47 bumped context_server timeout to 180s. Both still in place. Neither catches the in-wrapper race.

What

  • New session_creation_chain: Rc<RefCell<Option<Shared<Task<()>>>>> field on AcpConnection
  • New acquire_session_creation_slot() helper returning (prev_chain, SessionCreationGuard)
  • new_session and open_or_create_session (covers load_session and resume_session) take a slot at start, await prev, hold guard until response processed
  • Drop guard ensures cancellation/error releases the slot — chain can't wedge
  • Slot is acquired AFTER the pending_sessions dedup check, so same-id concurrent calls still share one task (existing behavior preserved)

Test

test_concurrent_session_creation_is_serialized:

  • Two distinct-session-id load_session calls dispatched together
  • First parks at fake-agent gate; second blocks on chain
  • Asserts only 1 RPC reached the wire while first is parked
  • After releasing gate, both complete; asserts 2 RPCs total

All 14 existing agent_servers tests still pass.

Caveat

This is a structural defence at the spawn point. The original trigger for two startup session/new calls in Zed (one for the saved thread, one for a "ghost" session whose cwd matches the workspace's first worktree path) is still unidentified — agent_panel.rs::load()'s draft branch is ruled out by saved KVP, and set_activeactivate_draft is ruled out by base_view being sync-set before set_active fires. Likely candidates: MultiWorkspace's ActiveWorkspaceChanged flow, or zed.rs::ensure_agent_panel_for_workspace. Tracking as follow-up; this PR prevents the symptom regardless of source.

🤖 Generated with Claude Code

lukemarsden and others added 2 commits May 6, 2026 23:40
Two concurrent calls to `connection.new_session()` /
`connection.load_session()` / `connection.resume_session()` on the same
`AcpConnection` cause `claude-agent-acp` (and any other compliant ACP
wrapper) to spawn two child claude SDK processes back-to-back. Each
child then runs `npx <pkg>@latest` in parallel for its stdio MCP
servers (chrome-devtools-mcp, server-github, etc). npm's tarball
extraction isn't safe under concurrent invocations on the same cache
directory: the loser silently fails to register its MCP child, leaving
the active claude process without (e.g.) `chrome-devtools-mcp` and the
agent visibly missing browser tools — the recurring "chrome MCP not
working in spec tasks" bug.

PR #46 deduped at the wrapper level so we get one `claude-agent-acp`
per `(project, agent_id)`, but a single wrapper can still receive two
distinct ACP `session/new` or `session/load` requests in flight (e.g.
panel restoration loading the saved thread + something else creating a
new session for a different worktree at startup), and each spawns its
own claude SDK child.

Fix: add `session_creation_chain: Rc<RefCell<Option<Shared<Task<()>>>>>`
on `AcpConnection`. Each `new_session` / `open_or_create_session` call
acquires the next slot in the chain — it awaits the previous request's
completion before dispatching its own ACP RPC, and signals completion
via a drop guard so cancellation/error releases the slot too.

Verified by `test_concurrent_session_creation_is_serialized`: two
distinct-session-id `load_session` calls dispatched together — only one
RPC reaches the wire until the first responds, then the second fires.

Note: this is a structural defence at the spawn point. The original
trigger that caused two startup `session/new` calls (one for the saved
thread, one for a "ghost" session with cwd matching the workspace's
first worktree path) is still unidentified — likely in
`agent_panel.rs::load()` or `MultiWorkspace`'s ActiveWorkspaceChanged
flow, but conditions in saved KVP rule out the obvious paths. Tracking
that as a follow-up; this fix prevents the symptom regardless of
source.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The doc comments said "#48" (a guess before the PR was
opened); the actual PR is #50. Per project rules, use full URLs.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@lukemarsden
lukemarsden merged commit 50c4308 into main May 11, 2026
29 checks passed
lukemarsden added a commit that referenced this pull request May 25, 2026
…, 2026-05-25)

Records the second upstream merge stacked onto the 002029 feature branch:
zero manual conflicts (ort strategy), one signature-drift repair for
ThreadView::new / ConversationView (code_span_resolver from cfd0461),
ancillary notes for the new ACP logout/additional-directories defaults
that compose cleanly with PR #50, and E2E validation results.

Spec-Ref: helix-specs@f5f507183:002029_merge-latest-zed
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