acp: keep prompt runs executing when the transport drops - #11668
acp: keep prompt runs executing when the transport drops#11668damienrj wants to merge 6 commits into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 36c63a5385
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 691369bd24
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7964ac39d9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if revocation.is_revoked() { | ||
| let cancelled = agent.revoke_and_cancel_own_runs().await; |
There was a problem hiding this comment.
Fresh evidence beyond the earlier live-connection revocation fix: when an authorized roaming peer disconnects before being revoked, serve returns, this one-time check sees false, and RoamingAcpHandler subsequently removes the connection from the node's live map. A later peers revoke or trust-file refresh therefore has neither a LiveConnection to signal nor the connection's GooseAcpAgent to cancel, so its detached prompt can continue provider calls and tool execution after the peer's authority is withdrawn; detached run ownership must remain associated with the peer until each run finishes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🤖 (sent by damienrj's AI agent) Addressed in 8347e58: detached-run ownership now outlives the connection, keyed by peer. goose-roaming gains a RevocableWork trait + a peer-keyed registry on RoamingNode; the connection's RevocationSignal doubles as the registrar (no further serve_stream signature change). The bridge registers a Weak<GooseAcpAgent> handle before serve(), and the detached prompt tasks hold the agent Arc for their full duration, so the handle stays reachable exactly while runs exist and prunes once they finish. enforce_trust (manual + trust-file watcher) drains and revokes parked work for keys leaving the allowlist — covering revoke-after-normal-disconnect — while re-accepted peers register fresh unfenced agents. Full-path test over real iroh transport: revoking_a_disconnected_peer_stops_its_detached_work, plus registry mechanics in revocation_registry_is_peer_keyed_and_prunes_dead_work.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8347e584b2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: df7d912dc5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e8098fbcd4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if !callback_pending_request.try_complete() { | ||
| return Ok(()); |
There was a problem hiding this comment.
When a client response and a transport I/O failure occur back-to-back, the SDK can drop this connection-owned callback while it is awaiting handle_confirmation; however, try_complete() has already cancelled the independent transport-termination fallback. Neither path then delivers a confirmation, leaving the detached run blocked indefinitely. Fresh evidence beyond the prior transport-termination comment is this new ordering: the request is marked complete before the awaited resolution finishes (the elicitation callback has the same ordering). Keep the fallback armed until resolution completes, or move the resolution to independently owned work before claiming completion.
Useful? React with 👍 / 👎.
1 participant
Problem
The ACP server runs each
session/promptinside a connection-scoped task (cx.spawnqueues it into the connection's event loop). When the client's transport closes — network drop, laptop sleep, client quit — the event loop ends, the task is dropped, andActiveRunDropGuard::drop()cancels the run'sCancellationToken. The in-flight turn dies with the connection.For remote clients (an ACP client over an SSH tunnel,
goose roampeers), this means any disconnect kills work in progress, even though the server, the session, and every subprocess it spawned are perfectly healthy.Change
Detach prompt execution from the transport:
server/dispatch.rs— thePromptRequestarm runson_promptontokio::spawn, with theRespondermoved into the task. The responder is the completion channel: it replies as before while the client is connected, and the reply send fails harmlessly (debug log) when it isn't. This also removes a latent hazard where a failed respond insidecx.spawn(whose contract shuts the server down on task error) could kill the whole connection loop.server.rs—ActiveRunDropGuardmoves with the detached task, so it fires only if the task itself dies (panic) before the explicitclear_active_run; its registry-leak protection is unchanged. Mid-turn client interactions become fail-soft: message-content, tool-notification, and usage-notification send failures warn and continue instead of aborting the turn. Provider-fatal errors (auth_required, credits exhausted) still abort.handle_tool_permission_requestresolves a send failure on a dead connection through the existing deny path (Permission::Cancel), so a detached turn never hangs waiting for an answer that cannot arrive. Elicitation already cancelled on send failure. If the connection dies while an answer is pending, the existingresend_pending_tool_permissionsonsession/loadre-asks when a client reconnects.Persistence needed no changes:
Agent::replywrites messages incrementally viasession_manager.add_message, and the detached task keeps driving the stream after disconnect, so a reconnecting client sees the completed turn viasession/load.Out of scope (follow-up): live re-attach — streaming a still-running turn's updates to a reconnecting client. The shared
ActiveRunRegistryis the natural anchor (updatable notification sink per run + an attach step onsession/load+ a bounded replay buffer).Tests
test_prompt_keeps_running_after_client_disconnect: raw JSON-RPC over the duplex fixture, drops the transport while the provider call is in flight, then polls session storage until the assistant reply persists. Non-vacuous: fails (timeout) without the dispatch change, passes in ~1.8s with it.cancel_from_another_connection_cancels_a_detached_run:session/cancelstill cancels a detached run via the shared registry.dropping_a_prompt_future_releases_the_shared_runrenamed toa_dying_prompt_task_releases_the_shared_run— same registry-leak protection, re-documented for task-death semantics.cargo test -p goose --lib 'acp::'(333) and all sevenacp_*integration binaries pass;cargo fmt --checkandcargo clippy -p goose --all-targets -- -D warningsclean.Also verified against a live deployment: a desktop ACP client connected over an SSH local port-forward sent a ~60s tool-using prompt, the tunnel was killed 20 seconds in, the remote turn ran to completion and persisted its final assistant reply, and the reconnected client loaded it via
session/load.Review follow-ups (second commit)
goose-roaminggains a per-connectionRevocationSignal;enforce_trust(and the trust-file watcher) set it before force-closing a revoked peer's connection, and it is passed toAcpStreamServer::serve_stream. After the stream ends, the full ACP bridge checks it and calls the newGooseAcpAgent::cancel_own_active_runs(), which cancels only runs this connection started (via the shared registry tokens), leaving entries for the detached tasks to clear — ordinary transport loss still detaches. Note: the extraserve_streamparameter is a breaking change for out-of-treeAcpStreamServerimplementors.send_active_run_update, local-inference progress, and cancelled-path clear sends now warn-and-continue, so a disconnect between dispatch andAgent::replyno longer silently discards the prompt. Pinned deterministically byprompt_on_dead_connection_still_runs_and_persists(runson_prompton a fully dead connection; fails on the old code), plus an earliest-possible transport-drop variant of the integration test.