fix(acp): serialize wakeup prompts to prevent turn misalignment#1202
Conversation
ScheduleWakeup synthetic prompts could race user prompts, causing the bridge to return stop_reason against the wrong turn and shift chat messages one prompt behind. Add a context-aware prompt gate and pin wakeups to their scheduled session after re-validation. Co-authored-by: Cursor <cursoragent@cursor.com>
📝 WalkthroughWalkthroughThe PR adds a ChangesPrompt gate serialization for wakeup/user-prompt race prevention
Sequence DiagramsequenceDiagram
participant Adapter
participant promptGate
participant Agent
Adapter->>promptGate: request gate (sendPrompt)
promptGate->>Adapter: grant gate
Adapter->>Agent: Prompt(message) -- pinned to expectSession if provided
Agent-->>Adapter: Prompt returns
Adapter->>promptGate: release gate
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Claude finished @carlosflorencio's task in 7m 37s —— View job Code Review
FindingsSuggestion (recommended, doesn't block)
What's correct ✓
Summary
Verdict: Ready to merge. The gate logic is correct, the tests cover both key races, and the |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
No issues found across 3 files
You’re at about 92% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
|
| Filename | Overview |
|---|---|
| apps/backend/internal/agentctl/server/adapter/transport/acp/adapter.go | Adds promptGate (1-slot buffered channel) to the Adapter struct as a semaphore; initialized in NewAdapter. No functional change beyond introducing the field. |
| apps/backend/internal/agentctl/server/adapter/transport/acp/adapter_prompt.go | Refactors Prompt into a thin wrapper over sendPrompt, which acquires promptGate before proceeding. Wakeup-pinned calls pass expectSession and are dropped if the session has changed or the adapter is closed. pendingContext is intentionally not consumed for pinned wakeup calls. |
| apps/backend/internal/agentctl/server/adapter/transport/acp/wakeup_concurrency_test.go | Three regression tests using a blocking fake agent: serialization prevents concurrent prompts, dropped wakeup when session changes while queued, and wakeup doesn't consume pendingContext. Gate release and event ordering are handled correctly. |
Sequence Diagram
sequenceDiagram
participant U as User Prompt
participant W as Wakeup Goroutine
participant G as promptGate (1-slot)
participant B as ACP Bridge
U->>G: "send struct{} (acquire)"
activate G
U->>B: conn.Prompt(sessionID, ...)
Note over W: ScheduleWakeup timer fires
W->>G: "try send struct{} (blocks — gate full)"
Note over W: waits on gate or ctx.Done()
B-->>U: PromptResponse + stop_reason
U->>G: receive (release)
deactivate G
W->>G: "send struct{} (acquire)"
activate G
Note over W: re-validate: sessionID == expectSession?
alt session unchanged
W->>B: conn.Prompt(sessionID, ...)
B-->>W: PromptResponse + stop_reason
else session changed / adapter closed
W-->>W: drop silently (return nil)
end
W->>G: receive (release)
deactivate G
Reviews (3): Last reviewed commit: "test(acp): register cleanup early and jo..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Skip pendingContext consumption for wakeup-pinned prompts, tighten concurrency test synchronization, shorten negative-assertion timeouts, and add a regression test for pendingContext preservation. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Claude finished @carlosflorencio's task in 4m 30s —— View job Code Review
SummaryAll findings from the prior review round have been correctly addressed in commit What's correct ✓
Two minor observations (neither blocks the merge)
Verdict: Ready to merge. The race fix is correct, both key concurrency scenarios are tested, and all prior feedback is resolved. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/backend/internal/agentctl/server/adapter/transport/acp/wakeup_concurrency_test.go (1)
113-127: ⚡ Quick winReplace these real-clock timeout assertions with
synctest.The
100ms/5stime.Afterwindows make these concurrency checks scheduler-dependent and slower than they need to be. Wrapping the tests insynctest.Testwould keep the ordering assertions deterministic without relying on wall-clock delays. As per coding guidelines, "Prefertesting/synctest(Go 1.24+) overtime.Sleepfor time-dependent tests; usesynctest.Testto wrap tests with tickers or timeouts to advance fake time instantly when all goroutines are idle."Also applies to: 184-206, 248-252
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/backend/internal/agentctl/server/adapter/transport/acp/wakeup_concurrency_test.go` around lines 113 - 127, Wrap the test body with testing/synctest by replacing real-clock waits with synctest.Test so assertions are deterministic: change the outer test to synctest.Test(t, func(st *synctest.T) { ... }), replace time.After usages (the selects waiting on <-time.After(5*time.Second) and <-time.After(100*time.Millisecond) and the other occurrences at the noted ranges) with st.After(5*time.Second) and st.After(100*time.Millisecond) or st.Tick equivalents, and keep the same channel waits on fa.entered and the fireWakeup call (a.fireWakeup(sid, "synthetic wakeup prompt")) unchanged; do this for the blocks referencing fa.entered so the wakeup vs user-prompt ordering is asserted using the synctest fake clock instead of real time.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@apps/backend/internal/agentctl/server/adapter/transport/acp/wakeup_concurrency_test.go`:
- Around line 143-147: Move the t.Cleanup registration immediately after
creating the pipes and adapter to ensure resources are always released even if
early t.Fatal occurs; specifically, after you construct a, c2aW, and a2cW (the
adapter and the two pipe writers) call t.Cleanup to close them and return/stop
background goroutines (use _ = a.Close(); _ = c2aW.Close(); _ = a2cW.Close()).
Repeat the same change in the other test setup blocks that create those same
symbols (the other places where a, c2aW, a2cW are allocated) so cleanup is
registered before any potential failure paths.
---
Nitpick comments:
In
`@apps/backend/internal/agentctl/server/adapter/transport/acp/wakeup_concurrency_test.go`:
- Around line 113-127: Wrap the test body with testing/synctest by replacing
real-clock waits with synctest.Test so assertions are deterministic: change the
outer test to synctest.Test(t, func(st *synctest.T) { ... }), replace time.After
usages (the selects waiting on <-time.After(5*time.Second) and
<-time.After(100*time.Millisecond) and the other occurrences at the noted
ranges) with st.After(5*time.Second) and st.After(100*time.Millisecond) or
st.Tick equivalents, and keep the same channel waits on fa.entered and the
fireWakeup call (a.fireWakeup(sid, "synthetic wakeup prompt")) unchanged; do
this for the blocks referencing fa.entered so the wakeup vs user-prompt ordering
is asserted using the synctest fake clock instead of real time.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d92f3207-0ca5-422b-a9de-755a169b33df
📒 Files selected for processing (2)
apps/backend/internal/agentctl/server/adapter/transport/acp/adapter_prompt.goapps/backend/internal/agentctl/server/adapter/transport/acp/wakeup_concurrency_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/backend/internal/agentctl/server/adapter/transport/acp/adapter_prompt.go
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
You’re at about 93% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Extract setupConcurrencyFakeAgent so t.Cleanup runs before failure paths, and waitForPromptComplete so fireWakeup goroutines drain before assertions. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Claude finished @carlosflorencio's task in 4m 31s —— View job Code Review
FindingsAll feedback from the two prior review rounds has been correctly addressed. The latest commit (
What's correct ✓
Suggestion (doesn't block)One minor observation on Summary
Verdict: Ready to merge. The gate logic is correct, all three concurrency scenarios are tested, cleanup is robust, and all prior feedback from three review rounds is resolved. |
1 participant
When a ScheduleWakeup synthetic prompt fired while a user prompt was still in flight, both calls reached the ACP bridge concurrently and stop_reason was paired with the wrong turn — shifting chat messages one prompt behind. This serializes prompts through a context-aware gate and drops stale wakeups if the session changed while queued.
Important Changes
promptGatechannel (not a mutex) so queued wakeups honour context cancellationPromptintosendPromptwith optional session pinning; wakeups re-validate the session after acquiring the gateValidation
make -C apps/backend fmtmake -C apps/backend test(all packages pass)make -C apps/backend lintgo test -race ./internal/agentctl/server/adapter/transport/acp/... -run TestWakeupPossible Improvements
Low risk — wakeups may be dropped if a session switch happens while one is queued, which is the intended safety behaviour.
Checklist
apps/web/), I have added or updated Playwright e2e tests inapps/web/e2e/and verified them withmake test-e2e.Made with Cursor