Repository navigation
feat: make request cancellation spec-conformant and avoid leaving a turn hanging - #131
Merged
Merged
Conversation
EugeneTheDev
force-pushed
the
eugenethedev/cancellation-v2
branch
from
September 16, 2026 17:28
f0b1d1f to
5e322ab
Compare
EugeneTheDev
force-pushed
the
eugenethedev/cancellation-v2
branch
from
September 18, 2026 13:18
5e322ab to
7c38491
Compare
Ololoshechkin
approved these changes
Sep 22, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Audited cancellation against the spec (
$/cancel_request,session/cancel, prompt lifecycle) and fixed seven defects. Two themes:$/cancel_requestdid not follow the spec — a cancelled request was left unanswered, a cancel arriving after the reply could kill work the peer never asked to stop, and the notification carried a field the schema does not define.state_update, leaving a client waiting forever. This was reachable from four different directions; all four are now closed by a single reporting path.session/cancelwas already conformant and is unchanged.Problems found and fixes
-32800. Removed the now-dead "omitted reply" plumbing. Round trip measured at 3–5 ms.$/cancel_requestfor an already answered request still hard-cancelled its follow-up work — which, for a v2 prompt, is the whole turn. The turn died silently with no idle update.cancellableByCounterpartflag that is disarmed (atomically) when the reply is collected, so a peer's cancel of a settled id is inert. Local cancellation is unaffected and still reaches settled requests, whichclose()needs.-32800response from a peer was converted into aCancellationExceptionand thrown from an uncancelled coroutine, silently tearing down the caller's scope. An agent whose permission request was answered-32800lost the turn with no error surfaced.AcpRequestCancelledException(a plainException, not aCancellationException), used on both the single-request and batch paths. The v2 agent catches it and ends the turn withcancelled. Only public API addition in this PR.CancelRequestNotificationcarried amessagefield the v2 schema does not define, and the SDK always populated it from the localCancellationException— so it was usually on the wire.-32800response path still carries the cancelled side's message.Throwableand emits a terminal idle withstopReason = null(the honest answer — there is no error stop reason, andrefusal/noticewould misinform the client).CancellationExceptionis rethrown untouched so shutdown stays silent; the emit is wrapped so a failed send cannot mask the original error.cancelPendingIncomingRequest(s)are public and callable on a live protocol. Used there, they stranded a settled prompt: the client kept its successful response but never got the rest of the stream or the terminal update.CancellationExceptionbranch now reportsIdle(cancelled)before rethrowing. To avoid a second terminal update on thesession/cancelpath, all updates funnel through an idempotentreportIdle(). Nothing reaches the wire on shutdown, becausesendFramealready refuses to send on a dead protocol.NonCancellable, so follow-up work can never overtake the reply that announced it.Testing
Every fix ships with tests that were confirmed to fail with that fix removed, at both the protocol level (
ProtocolTest,BatchProtocolTest) and end-to-end on both drivers (V2ClientTest,AgentTest) — including the negative cases:close()must not synthesise an idle update, a cancel for an answered prompt must leave the turn running, and a turn cancelled before it started must not report an outcome it never reached. The A4 removal also updated the tests that expected the caller's cancellation reason to cross the wire; they now expect the fixed message. Full JVM suite green.Compatibility
Two API changes, both in the cancellation surface:
AcpRequestCancelledException(A3). Callers that relied on catching aCancellationExceptionfor a peer's-32800must switch to it — which is the point: that conversion was cancelling scopes silently.CancelRequestNotification.message(A4), along with its constructor parameter.apiDumprefreshed.Everything else is unchanged; the typed Agent, Client, and session APIs are untouched.