Skip to content

fix(acp): abort orphaned agent loops and deliver /review output - #6684

Merged
bug-ops merged 1 commit into
mainfrom
fix/6674-acp-session-turn-race
Jul 28, 2026
Merged

fix(acp): abort orphaned agent loops and deliver /review output#6684
bug-ops merged 1 commit into
mainfrom
fix/6674-acp-session-turn-race

Conversation

@bug-ops

@bug-ops bug-ops commented Jul 28, 2026

Copy link
Copy Markdown
Owner

Summary

  • do_close_session/do_delete_session previously only fired cancel_signal.notify_one() without joining or aborting the session's agent-loop task, so a session/load/session/resume reusing the same SessionId could race a still-running old loop and corrupt the new turn's event stream. SessionEntry now tracks the loop's JoinHandle; close/delete abort and await it (5s bounded timeout) before the entry is dropped, and Drop for SessionEntry also aborts it unconditionally as a safety net covering the LRU-eviction/reaper removal paths.
  • /review previously dispatched fire-and-forget via input_tx.try_send and returned EndTurn immediately, bypassing acquire_prompt_channels. After PR fix(acp): close PromptChannelGuard reload race and stale-event leak #6672's inter-turn drain fix, /review's output was silently discarded instead of leaking into the next turn. /review is now intercepted in do_prompt and routed through the normal acquire_prompt_channels/drain turn path like any other prompt, so its output actually reaches the client. This is a client-visible behavior change: /review now participates in the same turn-contention check as any other prompt (can be rejected with "prompt already in progress") and its expanded prompt is persisted/replayed as part of session history.

Both issues were follow-up findings from PR #6672's review. Went through two rounds of adversarial critique: the first found a real gap where set_agent_loop_handle could silently drop a JoinHandle if the session entry was removed before the loop was fully wired up (fixed: aborts immediately instead), and a /review trailing-whitespace parsing regression (fixed: trim both ends to match handle_slash_command's existing parity). The second round re-verified both fixes and found no new gaps.

Closes #6674
Closes #6673

Test plan

  • cargo +nightly fmt --check
  • cargo clippy --profile ci --workspace --all-targets --features "desktop,ide,server,chat,pdf,scheduler,testing" -- -D warnings
  • cargo nextest run --config-file .github/nextest.toml --workspace --features "desktop,ide,server,chat,pdf,scheduler" (zeph-acp, zeph-common, zeph-core scopes) — 2736 passed
  • cargo nextest run full zeph-acp package scope (including tests/integration.rs, not just --lib --bins) — 235/235 passed, including all 5 new/changed regression tests:
    • close_session_aborts_agent_loop_task, delete_session_aborts_agent_loop_task (prove the old loop is actually aborted+joined, not just signaled)
    • review_command_output_is_delivered_to_client (positive round-trip proving /review's output reaches the client)
    • review_command_rejects_when_a_turn_is_already_in_progress (loops over ["/review", "/review\n", "/review\t"] to lock in the trim fix)
    • review_command_threads_connection_owner_key_into_channel_message (now also asserts the forwarded text matches the expanded review prompt)
  • RUSTFLAGS="-D warnings" RUSTDOCFLAGS="--deny rustdoc::broken_intra_doc_links" cargo doc --no-deps --workspace --features "desktop,ide,server,chat,pdf,scheduler"
  • gitleaks protect --staged
  • CHANGELOG.md updated under [Unreleased]/Fixed

@github-actions github-actions Bot added bug Something isn't working size/XL Extra large PR (500+ lines) documentation Improvements or additions to documentation rust Rust code changes labels Jul 28, 2026
@bug-ops
bug-ops enabled auto-merge (squash) July 28, 2026 11:33
@bug-ops
bug-ops force-pushed the fix/6674-acp-session-turn-race branch from 01133e7 to 1dd115c Compare July 28, 2026 11:33
do_close_session/do_delete_session previously only signaled
cancel_signal.notify_one() without joining or aborting the session's
agent-loop task, so a reload/resume of the same SessionId could race
against a still-running old loop and corrupt the new turn's event
stream. SessionEntry now tracks the loop's JoinHandle; close/delete
abort and await it (5s bounded timeout) before the entry is dropped,
and Drop for SessionEntry aborts it unconditionally as a safety net
for the LRU-eviction/reaper removal paths.

/review previously dispatched fire-and-forget via input_tx.try_send
and returned EndTurn immediately, bypassing acquire_prompt_channels.
After the prior turn-race fix started draining queued events at
acquire time, /review's output was silently discarded instead of
leaking into the next turn. /review is now intercepted in do_prompt
and routed through the normal acquire_prompt_channels/drain turn
path like any other prompt, so its output reaches the client.

Closes #6674
Closes #6673
@bug-ops
bug-ops force-pushed the fix/6674-acp-session-turn-race branch from 1dd115c to 6870cbf Compare July 28, 2026 11:45
@bug-ops
bug-ops merged commit 4498ed5 into main Jul 28, 2026
43 checks passed
@bug-ops
bug-ops deleted the fix/6674-acp-session-turn-race branch July 28, 2026 11:53
bug-ops added a commit that referenced this pull request Aug 16, 2026
* docs(readme): sync crate READMEs with commits since v0.22.3

Reconciles all 24 changed crate READMEs against the actual shipped
implementation for the v0.22.3..HEAD range: new subsystems (risk-chain
detection, capability scoping, plugin dependency graph, session spawn
cap), several pre-existing factual errors unrelated to this release
(inverted file-sandbox precedence, fabricated MCP config keys, wrong
anomaly-detector defaults, stale trust-level names), and terminology/
API renames that had drifted out of sync with the code.

* docs(specs): reconcile spec drift for commits since v0.22.3

Closes drift left after the skill-quarantine trust fixes (#6701,
#6702, #6706, #6707, #6713), the subagent session-wide spawn cap
(#6545), four post-ACP-2.0.0-migration bugfixes (#6660, #6665, #6672,
#6684), the mention-picker and TUI interrupt-hint updates, the
MAX_RETRY_SECS compile-time bound, the sanitizer secret-shape masking
extension, the tracing-guard-flush invariants, and the VigilGate
per-process pattern-compile fix. Updates specs/README.md's index to
match.

* docs(book): sync user docs with commits since v0.22.3

Updates the TUI keybindings and mention-picker pages for the new
Ctrl+C semantics, the inline @ mention picker, and the input
separator's busy indicator; documents the new
[tools.shell] risk_chain_window_turns config key; corrects the ACP
protocol version reference (was stale at 0.11.1); bumps the sub-agent
frontmatter breaking-change note to v0.22.4.

* fix(serve): give build_combined_deps_wires_policy_gate test a dedicated stack

cargo nextest run --features full could crash with a stack overflow
(SIGABRT) on
serve::agent_factory::tests::build_combined_deps_wires_policy_gate_through_to_session_agent.
Same defect class already fixed once in this file for issue #6699:
building a full Agent under --features full's unboxed AnyProvider
variants (Candle/Gonka/Cocoon) reaches the same VigilGate::try_new
stack depth that overflows the default 2 MiB test-thread stack in an
unoptimized build. The #6699 fix only wrapped the one test it was
filed against, leaving this one - added in PR #6007, unrelated to any
change in this release - unprotected. CI's test job never caught it
because it runs the curated feature set, not full, so the deeper
AnyProvider frames never materialize there.

Runs the test body on a dedicated 32 MiB-stack thread instead of
directly under #[tokio::test], reusing the existing
TEST_THREAD_STACK_SIZE constant.

* release: prepare v0.22.4

Bump version across the workspace, finalize the CHANGELOG.md
[0.22.4] section, refresh the README tests badge, and re-accept the
splash-screen snapshots (embed the version string).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working documentation Improvements or additions to documentation rust Rust code changes size/XL Extra large PR (500+ lines)

Projects

None yet

1 participant