fix(acp): wake quiet hosts for respawns and queued retries - #7459
Conversation
Signed-off-by: Logan Johnson <loganj@squareup.com>
🔐 Codex Security Review
Review SummaryOverall Risk: NONE
FindingsNo concrete security, correctness, or reliability findings were identified. Notes
Generated by Codex Security Review | |
|
@buzz-security-review e4b3cb9 |
Independent combined evidence (host + companion adapter) — exact heads, no source changesThis PR (host scheduling fix): independent host-only review of exact head Companion adapter PR (pic-suite): buzz://pr?id=7473c7f5bab6568886f33b56738613f3d4321ae22b727622f4859a4157ee9850&owner=efccd8ff4cab0cf2fc878d4ce288c336756c4fb2376ba98922002e6d5b7afd8a&d=pic-suite — repaired head Composed quiet-recovery results: each case (before retry eligibility, after eligibility, circuit reopen, normal typing) redelivered the original interrupted input exactly once with one durable worker run, one worker write, one terminal, one callback; startup children retire within bounded grace; the hard turn cap is preserved (the new progress path does not bypass it). Gate status at posting: exact-range security review SUCCESS, zero findings (run 34156878857); CI success (run 34156407833). This post is independent evidence metadata only — not a formal GitHub review approval and not merge/activation authorization. Activation additionally requires the effective max-turn/idle policy check for the two originally affected managed agents (an activation-only gate, not a source blocker). |
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent
Verdict: APPROVE
Reviewed: 3c7f288c60d67df78577b237e27c3dfc8831aaa1..e4b3cb9057b33554aae8a1769a17a34dd3eb96ce (exact head e4b3cb9057b33554aae8a1769a17a34dd3eb96ce)
Risk: high — this changes ACP host scheduling across respawn completion, retry eligibility, maintenance/refill, cancellation, and shutdown.
Behavior/contracts traced:
- The biased production
select!preserves shutdown priority while independently waking for respawn completion, retry eligibility, and maintenance (crates/buzz-acp/src/lib.rs:3106-3125). - Retry wakes arm only when the pool is ready and has idle capacity;
next_retry_deadlineexcludes empty, removed, and in-flight scopes (crates/buzz-acp/src/queue.rs:497-513). Existing requeue/mark_completeordering retains retry budget, backoff, and timestamps. - Consuming a respawn result clears the single-slot in-flight state, restores a successful replacement to its indexed slot, and immediately dispatches pending work (
crates/buzz-acp/src/lib.rs:3713-3758). Maintenance compacts state, refills circuit-eligible empty slots, and dispatches flushable work (crates/buzz-acp/src/lib.rs:3049-3093). - Closed respawn receivers disable only that wake arm without spinning or suppressing timers; cancellation does not consume a later result (
crates/buzz-acp/src/recovery_wake.rs:21-32,65-104). Shutdown remains first priority and aborts/drains respawns while explicitly shutting down returned children (crates/buzz-acp/src/lib.rs:3118-3125,4122-4135). - Adversarial review found no changed path that bypasses retry budgets or the hard turn cap, duplicates dispatch, strands an eligible quiet-host batch, or leaks a returned respawn child. Batching, dedup/delivery state, removed-scope handling, and circuit policy remain unchanged.
Findings: no blocking or non-blocking defect found in the reviewed range.
Author action: none.
Verification owner: integration/deployment owner for composed companion-adapter runtime replay and confirmation of effective max-turn/idle activation policy for affected managed agents.
Validation at matching exact head:
- Clean detached worktree and
git rev-parse HEADconfirmede4b3cb9057b33554aae8a1769a17a34dd3eb96ceimmediately before submission. git diff --check 3c7f288c60d67df78577b237e27c3dfc8831aaa1..HEAD— pass.- Independent exact-head
cargo test -p buzz-acp— 920 library tests + 9 integration tests, 0 failures. - Required exact-head CI results inspected: Rust lint/unit tests, Linux cross-compiles, Windows Rust, Desktop core/build/smoke/integration, relay/backend integration, PostgreSQL, security, Semgrep, zizmor, and DCO are successful. Path-inapplicable jobs are skipped.
Runtime evidence: the PR documents loopback stock-host/adaptor fixture results for fast and delayed replacement, with unmodified-base mutation evidence. A later exact-head composed report covers before/after retry eligibility, circuit reopen/refill, normal typing, bounded child retirement, exactly-once durable delivery, and preserved hard cap. These are supporting author-posted artifacts; I did not independently replay the original live incident or run the final composed process suite.
Residual risk: adapter/runtime configuration and deployment activation outside this three-file host patch. No conventional UI or native visual surface changed, so screenshot/native UI evidence is not applicable.
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent
Verdict: APPROVE
Reviewed: 3c7f288c60d67df78577b237e27c3dfc8831aaa1..e4b3cb9057b33554aae8a1769a17a34dd3eb96ce (exact head e4b3cb9057b33554aae8a1769a17a34dd3eb96ce)
Risk: high — host scheduler wakeups, retry timing, respawn state, shutdown ordering, and child cleanup.
Behavior/contracts traced: Both lanes traced the production biased select!, retry-deadline eligibility, respawn result consumption and slot replacement, maintenance/refill/dispatch, closed-channel behavior, cancellation, and shutdown cleanup (crates/buzz-acp/src/lib.rs:3049-3125,3713-3758,4122-4135; queue.rs:497-513; recovery_wake.rs:21-32,65-104). Quiet-host recovery no longer depends on incidental relay/presence/typing traffic. Shutdown remains first priority; retry wakes only arm with a ready pool and idle capacity; removed/empty/in-flight scopes do not generate retry deadlines; returned children are explicitly shut down during teardown.
Findings: no concrete author-actionable defect found. The lanes found no path in the searched three-file host patch that bypasses retry budgets, duplicates dispatch, strands eligible quiet-host work, suppresses timers after a closed respawn receiver, or leaks a returned respawn child.
Author action: none.
Verification owner: integration/deployment owner for composed companion-adapter replay and activation-policy confirmation.
Validation
At matching exact head:
git diff --check 3c7f288...HEAD— pass.cargo test -p buzz-acp— pass in shared exact-head evidence: 920 library + 9 integration tests, 0 failures.- Exact-head GitHub Rust lint/unit, Windows Rust, Linux cross-compile, security, relay/backend integration, DCO, and aggregate required gates — success.
Manual/native evidence: no live incident replay or composed companion-adapter process run.
Residual risk: activation/configuration outside this three-file patch—effective max-turn/idle policy and companion-adapter runtime behavior—was not independently exercised. This is an integration/deployment confidence gap, not author rework.
Problem
With
--no-presence --no-typing --heartbeat-interval 0, a crashed ACP transport can leave work queued indefinitely. Background respawn results were only drained beforeselect!; neither their arrival nor queue retry eligibility woke the host. A respawn-only wake is insufficient: replacement initialization normally precedes the independent queue backoff deadline.Change
mark_completepath.No reconnect/gateway changes or adapter retry engine. Based directly on main, independent of other ACP plumbing work. Closest related open PR found: #7317 (usage-limit retry policy); this is a scheduling fix, not that policy change.
Validation
cargo test -p buzz-acp: 920 library tests + 9 integration tests pass.cargo clippy -p buzz-acp --all-targets --all-features -- -D warnings,cargo fmt --all -- --check,just file-size-check, andgit diff --check: pass.801d23374a588fd1154bb3da8783f6dcc9084e15and upstream pi-acp 0.0.33. Loopback-only synthetic relay/provider; no live agent/config changes.3c7f288c60d67df78577b237e27c3dfc8831aaa1: expected failure at the 45-second callback bound.Full workspace/Desktop/mobile
just cinot run locally; no Tauri files touched. Relevant ACP package checks above are complete.Review / integration gate
Draft for independent review. The fixture uses the pinned pre-repair adapter to isolate this host defect, not concurrent adapter shutdown-fencing changes. Composed testing with the final adapter candidate and independent review remain required before claiming the shared lifecycle fix complete. No activation, merge, direct main push, or incident replay performed. Existing all-agents-dead exit and max-turn safety policies remain unchanged.