Synced from GitHub
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. To continue using code reviews, add credits to your account and enable them for code reviews in your settings.
fix(server): stopping a Claude turn no longer hangs without an acknowledgement#16419
Stopping a running Claude turn waits for the Claude Agent SDK to acknowledge the interrupt
(existing.query.interrupt in ClaudeAdapterV2.interruptTurn) with no time limit. If the CLI never
answers, the Stop request never finishes and the turn stays running.
The fix bounds that wait to 10 seconds with Effect.timeoutOption. After it, the existing path takes
over unchanged: close the query, wait up to 10 seconds for it to close, then settle the turn as
interrupted once. A normal acknowledgement still closes the query only after the SDK has answered.
This qualifies as a small, focused fix for an obvious bug: a one-line change that stops an unbounded wait. The neighbouring close step already uses the same bound.
How the problem was established and checked
Two focused tests in ClaudeAdapterV2.test.ts (background wake turns):
bounds an unacknowledged native Stop and settles its turn once: the fake SDK interrupt never completes. After advancingTestClockby 11 seconds, Stop must have finished, the turn must be settled as interrupted exactly once, and the query closed once.- Without the fix: fails, "Stop must finish despite a missing SDK acknowledgement".
- With the fix: passes.
lets the native SDK acknowledge Stop before closing its own query: guards the normal path. The SDK acknowledges with an aborted result, and the query is closed only after that. Passes with and without the fix.
The test helper makeResultFrame gained an optional userMessageUuid, so the aborted result can
answer the submitted prompt the way the CLI does.
Commands, run in the worktree:
vp test run apps/server/src/orchestration-v2/Adapters/ClaudeAdapterV2.test.ts: 145 passed.- Server typecheck: no errors in the changed files. Two pre-existing errors remain in
src/sourceControl/SourceControlRepositoryService.test.ts, a file this change doesn't touch. vp lintandvp fmt --checkon the adapters directory: no findings on the changed lines.
Not checked: a live Claude CLI that actually withholds the acknowledgement. The tests reproduce it with the SDK query fake.
Found and fixed in a downstream fork (Scient), then ported to current main. Ported and verified with Claude Opus 5.5 in Claude Code; the original fix was written with Codex (gpt-6.1-sol).
š¤ Generated with Claude Code
Checks
1 failing Ā· 23 successful Ā· 16 skipped
Failing1
- CodeRabbitGitHubCancelled
Added 1 commit
- fix(server): stopping a Claude turn no longer hangs without an acknowledgementefdd9cb
Synced from GitHub
Synced from GitHub
Approvability
Verdict: Approved at 333e370
Macroscope's review found this PR approvable ā Claude Stop requests now wait at most 10 seconds for SDK acknowledgement before continuing through existing shutdown handling. The change is limited to requested interruptions and preserves closure confirmation and Force stop handling.
You can add or adjust custom eligibility rules. Learn more.
Synced from GitHub
No actionable comments were generated in the recent review. š
ā¹ļø Recent review info
āļø Run configuration
- Configuration used: Path: .coderabbit.config.ts
- Review profile: CHILL
- Plan: Advanced
- Run ID:
4f3803ba-ac35-4eb7-a78d-dc6019c7903a
š„ Commits
Reviewing files that changed from the base of the PR and between 9bd1d8009a6b7c50f9dd9458e2bf27d481ff3b43 and efdd9cb4885e100774cca05d5d4833e966764387.
š Files selected for processing (2)
apps/server/src/orchestration-v2/Adapters/ClaudeAdapterV2.test.tsapps/server/src/orchestration-v2/Adapters/ClaudeAdapterV2.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.
š Walkthrough
Walkthrough
interruptTurn now waits at most 10 seconds for query.interrupt before continuing query closure and shutdown. New tests cover acknowledged and unacknowledged Stop operations.
Changes
Turn interruption
| Layer / File(s) | Summary |
|---|---|
Bound interruption and verify Stop behavior apps/server/src/orchestration-v2/Adapters/ClaudeAdapterV2.ts, apps/server/src/orchestration-v2/Adapters/ClaudeAdapterV2.test.ts | interruptTurn limits its wait for query.interrupt to 10 seconds. The tests check that an acknowledged abort precedes query closure and that an unacknowledged interruption completes after 11 seconds, with one query close and one interrupted terminal. |
Priority: ā¬ļø Low
Estimated code review effort: 2 (Simple) | ~10 minutes
Change: Bug fix
Suggested reviewers: juliusmarminge
Merge Risk: āŖ Minimal Ā· up to efdd9
Stops without an SDK acknowledgement can now proceed to closure after the timeout. No merge-blocking issue is established.
Security Architecture Review
Security architecture risk: šµ Low Ā· up to efdd9
This narrow change reduces an indefinite wait without adding access or permissions. No new security issue was established, but termination during partial failure and overlapping Stop requests remain incompletely verified.
Retained concerns No architecture-level concerns identified.
Security review details
Security Blast Radius
- inferred ā The demonstrated exposure is the existing live query and its active turn within a provider session. The change adds no caller, credential, permission or cross-service dependency. The full impact of native work surviving failed cleanup remains unverified.
Trust Boundaries and Controls
- observed ā Production closure shuts down the prompt queue before calling SDK close. Frame handling rejects messages whose query is no longer current, and stream-exit cleanup checks query ownership before finalizing. These are concrete containment controls, although they do not prove cancellation of already-running native work or already-entered handlers.
Resilience and Maintainability Implications
- inferred ā The timeout improves recovery from missing acknowledgements. However, ignored close errors and forced logical completion cannot independently establish native termination. The finalizer emits terminal events before clearing activeTurn and has no visible atomic once guard; overlapping recovery remains an uncertainty rather than an established PR-introduced defect.
Hardening Proposals
- proposed ā Establish an explicit shutdown guarantee for close failure and missing stream completion, and verify that concurrent Stop and in-flight results share an idempotent terminal transition. This would distinguish logical settlement from confirmed termination of native authority.
š„ Pre-merge checks | ā 4
ā Passed checks (4 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ā Passed | The title clearly describes the main change: preventing a Claude turn from hanging when the SDK does not acknowledge Stop. |
| Description check | ā Passed | The description explains the problem, the fix, why the focused change needs no prior approval, and the tests and checks performed, including what was not tested. |
| Linked Issues check | ā Passed | Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | ā Passed | Check skipped because no linked issues were found for this pull request. |
āØ Finishing Touches
š§Ŗ Generate unit tests (beta)
- Create a new PR
- Autopilot Ā· Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
Synced from GitHub
Added 3 commits
Note
Written by gpt-6.1-sol on behalf of Maria
Merged current main without rebasing and preserved its explicit force-stop behavior. Adapted the two Stop tests to the current event-driven harness and removed an obsolete result-helper argument. The runtime diff remains the 10-second SDK interrupt timeout; the PR still changes only the adapter and its existing test file.
Blacksmith vp run check:changed passed: format, lint, knip, GraphQL generation check, typecheck for three affected packages, and 283 tests. Two independent source reviews found no blockers at 333e370f6268791c9f50676963aa69c40e46b2c2. A real Claude client stop interaction remains unverified.
Synced from GitHub