atlas-agent-manager: the six ATL-212 children — connect once, free the process, keep the turn id

[ahammadnafiz]

Fixes the six ATL-212 children filed against crates/atlas-agent-manager. Five are defects; the sixth is the test suite that could not have caught any of them.

The first four are one import mistake with four symptoms. Zed's AgentConnectionStore is Rc-based and confined to the GPUI main thread, where its check-then-insert is one uninterruptible borrow and its session state lives elsewhere entirely. The port kept the shape, swapped Rc for Arc and GPUI for multi-threaded tokio, and src/lib.rs recorded the result as "the mechanism is unchanged; only the runtime is". It was the other way round: the mechanism's correctness was the runtime. That sentence is now the crate doc's warning rather than its claim.

What changed

ATL-226 — two concurrent requests started two agent processes. request_connection took the entries lock to look, released it, started a connect, then took it again to insert. connect_to added a second check-then-act in front of that window and restart_connection a third. Measured at 11–30% of rounds under real concurrency.

Every entry is now created in one place, open_entry, which holds a single guard across the check and the insert. connect_to's pre-check is kept as a fast path and documented as one — two callers arriving together both fall through, and the locked path picks a winner. Calling AgentServer::connect under the guard is safe and says why: both implementations build a boxed future and perform no I/O synchronously, and neither can reach back into the manager.

ATL-227 — a version bump or uninstall left the old process running, and it outlived Atlas. Four sites evicted a connection; one cleared that agent's sessions. A session pins the connection Arc (SessionHandle → AcpThread → Arc<dyn AgentConnection>), so the other three left a child alive that nothing could reach — including AgentHost::shutdown, which iterates connections() and therefore only sees entries still in the map.

forget_sessions_for now runs at all four. Sessions are forgotten locally rather than closed on the agent first: a version bump and an uninstall both end with that process being dropped, and a session/close to a peer about to be killed buys nothing. AgentManager::shutdown() is new and sweeps both maps directly; AgentHost::shutdown calls it instead of driving eviction through connections().

ATL-228 — killing an agent mid-install still downloaded, spawned and handshaked it. drop_connection removed the entry and returned. Dropping the manager's own handle on the connect could never have stopped it, because a caller parked in connection() holds a clone of the same Shared and keeps polling it.

The connect now runs on a task of its own and the entry keeps its AbortHandle. Every eviction path aborts it, which drops the connect future and the child it had spawned. Waiters get LoadError::Other("the agent was stopped while it was connecting") rather than a connection to a process the user killed, so AgentHost::spawn no longer returns success for one.

ATL-229 — a superseded turn's late reply closed the turn that superseded it. begin_turn returns a turn id and RunningTurn carries one precisely for this; send dropped it, and end_turn/set_error cleared running_turn unconditionally. atlas-acp-thread gains end_turn_unless_superseded and set_error_unless_superseded.

The guard is "no other turn is running", not "this turn is running", and the difference is the whole of the cancel path. cancel() clears the running turn before the agent answers, so a stricter guard swallows the only Stopped a cancelled turn ever emits — and Stopped is what becomes TurnFinished, which is what flushes analytics, the transcript and the turn's token usage. The strict version was written first, caught in review, and both layers now have a test that fails under it. The unguarded end_turn is untouched: atlas-agent-delta and this crate use it as "announce Stopped" with no turn open, which ATL-218 finding 4 settled deliberately.

The same omission was one layer up: AgentHost::send announced a failed turn's error through the projector, which stamps whatever turn_seq is current — so the cancelled turn's error landed on the live one. It now checks the turn is still current first.

ATL-230, all three findings.

1. The sessions map was keyed by the agent-chosen session id alone, so two connected agents minting the same id meant the second registration silently evicted the first. Now keyed by (Agent, SessionId). The id-only lookups still work, and refuse to answer when two agents share an id rather than routing a user's message into another agent's conversation — wrong in a way the user can see beats wrong invisibly. The host's own map and the frontend's SessionKey are still id-keyed; that is a separate change and is not in here.
2. An agent uninstalled between server_for and start_connection fell through to the native delegate and reported no command resolver for agent \x`, a description of Atlas's plumbing. It now says `x` is not installed`.
3. ResumeMode::Replayed restates the advertised capability rather than observing the replay. Deriving it from the loaded thread's entries was tried and backed out: for every external agent the replay frames are still queued on the connection's dispatch task when session/load answers, so an empty thread means "not drained yet" as often as it means "replayed nothing" — the check would have told users their history was gone on conversations that had it. The finding stands, with the reasoning recorded at the call site and pinned by a test. The fix belongs in the connection, which is the only layer that knows a frame arrived: count the session/updates between the load request and its response, or flush the dispatch queue before open_or_create_session returns.

Separately, close_session on an id two agents share used to report success having closed nothing; it is an error now. An id that names nothing is still Ok, which is the idempotent case a tab closing twice needs.

ATL-231 — the suite. 17 tests to 45. The join test called request_connection twice sequentially and commented it as concurrency; that comment now says what it tests and points at the real one. New: tests/support with a connect the test parks mid-flight and connections that report their own destruction, tests/invariants.rs with 22 tests covering each finding above, and tests/handshake.rs, which drives a real python ACP agent as a child process through the manager end to end — the handshake path had no coverage anywhere in the repo, and a fake connection cannot fail to die.

Every fix was checked by reverting it and watching a test fail — including the retain in drop_connection that was already correct, and the three tests that review showed could not fail and were rewritten until they could.

start_paused was considered and not adopted: every test that races anything needs the multi-thread flavour, and start_paused is current-thread only.

Verification

• cargo test in crates/atlas-agent-manager: 45 passed, was 17
• cargo test in crates/atlas-acp-thread: 72 passed, was 68
• cargo clippy --all-targets -- -D warnings clean on both, which is the gate both crates opt into
• cargo build --workspace and the src-tauri suite green

Not in here

ATL-230 finding 1's other half (the host's own session map and the frontend's SessionKey, which are still id-keyed), ATL-230 finding 3's connection-level replay signal, and the atlas-agent-delta compounding noted in ATL-229 where a superseded turn's terminal is stamped with the current turn_seq.

src-tauri's commands::skills tests fail on a developer machine that has real skills installed — they read the live skills directory, so one of them counts 138 where it expects 1. Pre-existing, unrelated to this change, and green on a clean CI runner.