Repository navigation
fix: prevent session cancel and clear races - #523
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Team Run ID: ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (11)
💤 Files with no reviewable changes (1)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. WalkthroughThe ACP driver now separates active-prompt cancellation from session cancellation. Session clearing marks conversations as finalizing, avoids self-waiting, and coordinates database recovery and query-recorder access with finalization state. ChangesSession lifecycle coordination
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The PR narrows prompt cancellation to the active request and prevents conversation reads from entering the clear replacement window; focused race tests, package gates, build checks, and public-surface QA passed. No actionable merge-blocking risk remains beyond normal checks and review. Sequence Diagram(s)sequenceDiagram
participant Client
participant Driver
participant AgentProcess
participant ACPAgent
Client->>Driver: Cancel prompt
Driver->>AgentProcess: cancelCurrentPrompt()
AgentProcess->>ACPAgent: $/cancel_request for active prompt
Client->>Driver: Stop session
Driver->>ACPAgent: session/cancel
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Linked Issues checkExplanation The changes satisfy Full details: Out of Scope Changes checkExplanation All reviewed changes support the linked objectives. The implementation changes, synchronization logic, and regression tests are directly related to session cancellation and conversation-clear race prevention.
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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 |
|
| Filename | Overview |
|---|---|
| internal/acp/client_control.go | Restricts Driver.Cancel to the active prompt and sends the sole session-scoped cancellation directly from whole-session Stop. |
| internal/acp/client_prompt.go | Removes the redundant session-scoped cancellation notifier and relies on the request context for ACP request cancellation. |
| internal/session/manager_clear.go | Extends conversation finalization across the full serialized clear operation, including Stop and restart. |
| internal/session/manager_conversation_operation.go | Adds target-specific operation ownership to let a clear operation bypass its own finalization barrier. |
| internal/session/query_recorder.go | Waits for conversation finalization before recorder resolution and retries once after read-only pool quiescence. |
| internal/session/manager_session_db_family.go | Prevents query-driven clear recovery from running while conversation finalization is active. |
| internal/session/manager_clear_test.go | Adds deterministic coverage proving concurrent transcript reads wait during clear quiescence. |
Sequence Diagram
sequenceDiagram
participant Client
participant Manager
participant Session
participant Store
participant Reader
Client->>Manager: ClearConversation(sessionID)
Manager->>Manager: Lock conversation operation
Manager->>Manager: Publish finalization barrier
Manager->>Session: Stop using owner context
Session-->>Manager: Stopped
Manager->>Store: Quiesce, backup, and replace DB family
Reader->>Manager: TranscriptPage(sessionID)
Manager-->>Reader: Wait on finalization barrier
Manager->>Session: Restart durable session
Manager->>Manager: End finalization barrier
Manager-->>Reader: Resolve canonical recorder
Reader->>Store: Open read-only recorder
Store-->>Reader: Fresh empty transcript
Manager-->>Client: Cleared session
Reviews (1): Last reviewed commit: "fix: prevent session cancel and clear ra..." | Re-trigger Greptile
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This branch was successfully deployed
1 participant
What & why
Fixes #521.
Fixes #522.
This PR removes two timing-dependent session failures that meet in the same operator flow: cancel an active prompt, clear the conversation, and continue using the durable session.
#522 — a late prompt cancel could cancel the next turn
Driver.Cancelpreviously had two different cancellation behaviors:session/cancel.That fallback was unsafe because
session/cancelbelongs to the whole ACP session rather than one request. A delayed or repeated cancel could therefore arrive after turn A settled and cancel turn B. Prompt-context cancellation also started a second notifier that emitted the same session-scoped notification, even though the ACP SDK already maps context cancellation to request-scoped$/cancel_request.The fix makes prompt cancellation own only the active request:
Driver.Cancelcancels the active prompt context and becomes an idempotent no-op after settlement;session/canceldirectly fromcancelSessionForStop.The owning ACP lifecycle suite now proves all four issue invariants: active cancellation still works, no session-scoped notification is sent for active or late prompt cancellation, Stop sends exactly one session-scoped cancel, and the next turn completes after a settled late cancel.
#521 — transcript reads could enter the clear replacement window
Conversation clear already serialized its writer and quiesced the per-session SQLite family, but it published
conversationFinalizingtoo late: only after stopping the active runtime and inside the database replacement operation. A transcript reader could resolve the stored recorder immediately before quiescence, fail its read-only open withErrReadOnlyPoolQuiescing, normalize that temporary state tosession not found, and enter recovery while clear was replacing the database family.That is the same sequence recorded in recent
ci.ymlWeb E2E failures: backup completed, a concurrent transcript read reportedsession not found, then the clear endpoint returned HTTP 500.The fix makes the clear transition atomic to readers:
ErrReadOnlyPoolQuiescing, using the canonical resolved session ID.The existing clear suite owns the regression. Its deterministic quiescence hook now proves that a transcript read waits for clear instead of returning a transient missing-session error.
CI correlation
The #521 race is directly correlated with the repeated
E2E web (shard 3/4)failure atweb/e2e/__tests__/session-hardening.spec.ts:314in recent main runs:33538539190/ job99958871506;33535868025/ job99950062419; and33528977779/ job99927127061.#522 is adjacent to the same cancel → clear scenario and explains flaky prompt-cancel/next-turn behavior, but it is not the source of the clear endpoint's recorded HTTP 500. Other recent E2E failures involving the Loops builder, OS shell performance, terminal locators, and Desktop teardown have different failure signatures and are deliberately outside this PR.
How you verified it
Focused regressions
go test -race ./internal/acp -run 'TestDriverCancelPreservesTurnScope|TestStopCancelsANonInspectionSession' -count=1go test -race ./internal/session -run 'TestClearConversationRestartsSameSessionWithFreshContext' -count=1internal/acpandinternal/sessionpackage suites passed before the final scoped gate.GOOS=windows GOARCH=amd64 CGO_ENABLED=0 go build ./internal/acp ./internal/sessionRequired local gate
make gate./internal/acp/...and./internal/session/...;d992f081f6a9cdd0e1107f6f9070e02061112897.Per repository policy, I did not run
make gate-fullor the full Web E2E matrix locally. The exact-head PR CI run owns those heavy lanes.Public-surface QA
The targeted isolated lab used a real Codex provider and exercised CLI, HTTP, UDS, native-tool, runtime, and Web surfaces.
canceledfor turnturn-8a40d61cac8706d7.compozy__session_prompt_cancelcalls returnednothing-in-flight.NEXT_TURN_OKafter the canceled turn.CLEAR_NEXT_OKafter clear.RT-session-prompt-cancelandRT-017now recordqa_status: pass."clean": truewith no surviving processes.The adjacent stopped-session canary encountered the already-open
BUG-20260825-workspace-agent-unusable-for-sessions. That known workspace-profile agent resolution defect occurs after the in-scope clear and post-clear prompt have completed and is not changed here.Authorship: Codex implemented and co-wrote this change; I inspected the final diff and verified it through the commands and public-surface walks above.
Impact
compozy session prompt-cancelkeeps the existingcanceled/nothing-in-flightcontract and becomes reliably turn-scoped.session-hardening.spec.tsremains the full E2E backstop in PR CI.config.tomlkey, default, lifecycle, or Settings surface changes. Searches across runtime config and Settings found no cancellation or clear setting to update or remove.packages/siteproduct-doc change is needed because the existing lifecycle and orchestration docs already state the intended public behavior: prompt cancel preserves the session and repeat cancellation is safe. The living QA scenarios and run report are updated with fresh evidence.Compozy Impact Audit
compozy__session_prompt_cancelkeeps the same tool ID, toolset membership, descriptor, input/output schemas, digests, risk flags, availability gates, and CLI/API fallbacks. Its existing production path now inherits the corrected request-scoped runtime behavior. Verified through descriptor inspection and live invocation.skills/compozy/references/runtime-operations.md,skills/compozy/references/native-tools.md, and their bundled mirrors; they already document idempotent prompt cancellation, session preservation, and whole-session Stop separately.Risk and rollback
The main risk is deadlock or over-serialization around conversation replacement. The operation-owner marker limits barrier bypass to the context holding the exact session's conversation lock, while independent readers still obey caller cancellation. Race-enabled package tests, deterministic clear quiescence, real provider QA, and the Web walk cover those boundaries.
There is no migration or compatibility bridge to roll back. Reverting this single commit restores the previous behavior, though it would also restore both races.
make gatepasses locally; this PR is delivered only after its required CI checks are greenSummary by CodeRabbit