Skip to content

fix(acp): queue busy follow-up prompts safely - #23772

Open
StiensWout wants to merge 7 commits into
pingdotgg:mainfrom
StiensWout:t3code/acp-stop-rpc-cleanup
Open

StiensWout wants to merge 7 commits into
pingdotgg:mainfrom
StiensWout:t3code/acp-stop-rpc-cleanup

Conversation

@StiensWout

Copy link
Copy Markdown
Contributor

ACP agents such as OMP can reject a follow-up while still processing, causing T3 to fail the turn. Retry only that exact busy rejection with exponential backoff capped at 30 seconds, until delivery or Stop.

Stop blocks later prompt admission and releases locally cancelled RPC requests. Unexpected agent exit during backoff remains a failure. Accepted prompts are never resent.

Closes #22660. Supersedes #22698 while preserving Julius Marminge's five commits and attribution. Wout requested this provider-parity work; maintainer review remains required.

Validation: check:changed passed all stages, 110 tests and 32 affected package typechecks. Two independent reviews passed, including ACP v1/v2 request cleanup. A real isolated server with a synthetic ACP subprocess passed recovery, queued/in-flight Stop, old-output and unrelated-error cases. No actual OMP or Windows run; the long-delay case was not repeated at the final revision.

ACP v1 has no acceptance event, so the queued notice can remain until the prompt finishes. Prior agent output still appears in the current turn. An agent that stays busy remains queued until Stop.

Prepared for Wout by gpt-6.1-sol in Codex through T3 Code.

@github-actions github-actions Bot added vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. size:XL 500-999 changed lines (additions + deletions). labels Oct 22, 2026
@macroscopeapp

macroscopeapp Bot commented Oct 22, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Needs Human Approval

Macroscope's review found this PR requires human approval — This PR changes production ACP prompt and cancellation lifecycles across the transport, session runtime, and adapter, including indefinite backoff retries and new Stop/termination coordination. It also adds a verifier-level directive disabling multiple static-analysis diagnostics, so the change warrants human review.

You can add or adjust custom eligibility rules. Learn more.

@coderabbitai

coderabbitai Bot commented Oct 22, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
📥 Commits

Reviewing files that changed from the base of the PR and between a2031ac and 0fd489c.

📒 Files selected for processing: 1
  • knip.jsonc
🚧 Files skipped from review as they are similar to previous changes: 1
  • knip.jsonc

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 8 remain after this review.

📝 Walkthrough

ACP prompt handling now retries exact busy-agent rejections and reports queued retry state. Cancellation is coordinated with prompt dispatch and can prevent queued requests from being sent. Tests and scripts cover retries, cancellation, agent termination, and related scenarios.

Changes: ACP busy prompt handling

Layer / File(s) Summary
Cancellation-aware request admission
packages/effect-acp/src/client.ts, packages/effect-acp/src/protocol.ts, packages/effect-acp/src/client.test.ts, packages/effect-acp/src/protocol.test.ts
The client passes an optional cancellation predicate to the protocol. The protocol checks it before queue admission and returns a correlated -32800 failure for cancelled requests. Tests cover cancelled v1 and v2 prompts and cancellation during outgoing logging.
Busy retry lifecycle and adapter state
packages/provider-acp/src/server/AcpSessionRuntime.ts, packages/provider-acp/src/server/adapter.ts, apps/server/src/orchestration-v2/Adapters/AcpRegistryAdapterV2.test.ts, apps/server/src/provider/acp/AcpJsonRpcConnection.test.ts
The runtime retries exact busy-agent rejections with increasing delays and coordinates queued or in-flight cancellation. The adapter reports retry metadata and keeps busy-queue retries active while prior output arrives. Tests cover backoff, cancellation, unrelated errors, and agent termination.
Live retry scenarios and evidence
apps/server/scripts/fixtures/acp-busy/agent.mjs, apps/server/scripts/verify-acp-busy-live.mjs, apps/server/scripts/acp-mock-agent.ts, knip.jsonc
The synthetic agent and verifier exercise busy retries, recovery, stop behavior, late output, and failure scenarios. The mock agent adds busy-response and malformed-output controls. Knip includes the verifier and fixture.

Priority: ➖ Normal

Estimated code review effort: 4 (Complex) | ~60 minutes

Change: Bug fix · Severity of issue fixed: Medium

Sequence Diagram(s)

sequenceDiagram
  participant ACPAgent
  participant AcpSessionRuntime
  participant Adapter
  ACPAgent-->>AcpSessionRuntime: Reject prompt as already processing
  AcpSessionRuntime->>Adapter: Report retry attempt and delay
  Adapter->>Adapter: Record queued retry state
  AcpSessionRuntime->>ACPAgent: Retry prompt after delay
Loading

Possibly related issues

Merge Risk: ⚪ Minimal · up to 0fd48

No actionable issue is established in the reviewed change; it is mergeable after normal checks.

Pre-merge checks: Passed 4
Check name Status Explanation
Title check Passed The title clearly and concisely describes the main change: safely queuing busy ACP follow-up prompts.
Description check Passed The description covers the problem, implementation, scope, verification results, limitations, linked issue, and required maintainer review. It provides specific test and environment details rather tha…
Linked Issues check Passed Issue #22660 requires a prompt sent while an ACP agent is busy to reach the agent or remain queued until it can run. AcpSessionRuntime.prompt retries only the exact already-processing -32603 rejec…
Out of Scope Changes check Passed The changes remain connected to issue #22660. Runtime changes implement busy-prompt retry, cancellation, queue admission, and termination handling. Protocol and client changes support cancellation of …
Full details: Description check

Explanation

The description covers the problem, implementation, scope, verification results, limitations, linked issue, and required maintainer review. It provides specific test and environment details rather than stating only that tests passed.

Full details: Linked Issues check

Explanation

Issue #22660 requires a prompt sent while an ACP agent is busy to reach the agent or remain queued until it can run. AcpSessionRuntime.prompt retries only the exact already-processing -32603 rejection with exponential delays capped at 30 seconds. The retry continues until delivery or Stop. Cancellation, serialized cancel, and isRequestCancelled handling prevent prompt admission after Stop and release cancelled requests. Tests cover recovery, Stop before and during retry, long busy periods, prior output, unrelated errors, agent exit, malformed output, and later prompts.

Full details: Out of Scope Changes check

Explanation

The changes remain connected to issue #22660. Runtime changes implement busy-prompt retry, cancellation, queue admission, and termination handling. Protocol and client changes support cancellation of queued or locally cancelled RPC requests. The adapter, ACP fixture, live verifier, and regression tests validate the required retry and Stop behavior. No unrelated product behavior is established by the reviewed changes.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

@StiensWout

StiensWout commented Oct 22, 2026 •

Copy link
Copy Markdown
Contributor Author

Update: main's #23798 pins patched package versions. This PR is now rebased to 0fd489c6 on 22840ab. Release Smoke passes locally, along with all six changed-file check stages, 110 tests and 32 affected package typechecks. The new head's CI is pending.


Release Smoke fails before exercising ACP behavior: lockfile regeneration reports ERR_PNPM_UNUSED_PATCH for @expo/fingerprint@0.21.5 and expo-network@58.0.3.

I reproduced the same failure using the exact 4eae8ae base commit in an isolated manifest fixture. This PR changes none of the release-smoke inputs, dependency manifests, lockfile or patch files. The base CI run 39827482146 passed earlier; regenerating the lockfile from those same inputs now fails before any ACP code is exercised. This is a shared dependency-resolution blocker, not an ACP test failure. I have not removed patches or weakened the smoke check.

All eight server shards, lint, build/typecheck, core tests and Rust passed on a2031ac. CodeRabbit reports no actionable comments, and Macroscope correctness passes; its human-approval requirement remains applicable.

gpt-6.1-sol via Codex in T3 Code, responding for Wout.

juliusmarminge and others added 7 commits October 22, 2026 21:53
Settle denied typed requests with correlated failures so RPC registrations are released. Preserve recorded runtime termination errors during busy backoff unless Stop was requested.
@StiensWout
StiensWout force-pushed the t3code/acp-stop-rpc-cleanup branch from a2031ac to 0fd489c Compare October 22, 2026 20:04
@StiensWout

Copy link
Copy Markdown
Contributor Author

@macroscope-app review

Please review the current head, 0fd489c6b463e6fa4a9e8f03b7a7c0331ea20165. It rebases onto the upstream patched-package version fix; Release Smoke and the required checks now pass. The ACP implementation is unchanged from the previously reviewed head.

gpt-6.1-sol via Codex in T3 Code, responding for Wout.

@macroscopeapp

macroscopeapp Bot commented Oct 22, 2026

Copy link
Copy Markdown
Contributor

Sorry, I'm unable to act on this request because you do not have permissions within this repository.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:XL 500-999 changed lines (additions + deletions). vouch:trusted PR author is trusted by repo permissions or the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: ACP provider: message sent while agent is still processing fails with "Agent is already processing. Use steer() or followUp()"

2 participants