internal/jsonrpc2: plumb cancellation cause through request context#1103
Open
aha-jansen wants to merge 1 commit into
Open
internal/jsonrpc2: plumb cancellation cause through request context#1103aha-jansen wants to merge 1 commit into
aha-jansen wants to merge 1 commit into
Conversation
handleAsync reported why a request was canceled by inspecting the connection-level s.writeErr, guarded by a TODO awaiting golang/go#51365. That API shipped as context.WithCancelCause/context.Cause in Go 1.21, and this module targets Go 1.25. Carry the cancellation cause on each request's context instead of inferring it from global state. Both the write-failure and read-side teardowns cancel each in-flight request with a cause wrapping ErrServerClosing; the remaining cancel sites (peer cancel, normal completion) pass nil. handleAsync now reads context.Cause(req.ctx) instead of guessing from s.writeErr. This also fixes a latent mis-attribution: s.writeErr is connection-level, so a request canceled for an unrelated reason was reported as ErrServerClosing whenever s.writeErr happened to be non-nil. ctx.Err() still returns context.Canceled in all cases, so external behavior for callers inspecting ctx.Err() is unchanged.
aha-jansen
force-pushed
the
fix/cancel-cause-plumbing
branch
from
July 18, 2026 03:37
2e16971 to
04327e3
Compare
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.
Closes #1100.
What
handleAsyncininternal/jsonrpc2/conn.goreported why an in-flight request had been canceled by inspecting the connection-levels.writeErr, guarded by a TODO waiting on golang/go#51365:That API shipped as
context.WithCancelCause/context.Causein Go 1.21, and this module already targets Go 1.25, so the TODO is now actionable.This PR carries the cancellation cause on each request's context instead of inferring it from global state:
incomingRequest.cancelbecomes acontext.CancelCauseFunc(built viacontext.WithCancelCause).ErrServerClosing.Cancel, normal completion) passnil.handleAsyncreadscontext.Cause(req.ctx)instead of looking ups.writeErr.Why
Besides resolving the TODO, this fixes a latent mis-attribution.
s.writeErris connection-level state, not per-request: a request canceled for an unrelated reason (e.g. a peer cancel notification viaCancel) was reported asErrServerClosingwhenevers.writeErrhappened to be non-nil at that moment. Tying the cause to the specific request's context removes that coupling.ctx.Err()still returnscontext.Canceledin all cases, so external behavior is unchanged; only the reported cause becomes accurate and per-request.Test plan
go build ./...go test ./internal/... ./mcp/(including the longermcpintegration tests) — all pass.context.WithCancelCausesemantics: with a cause,Err()stayscontext.CanceledwhileCause()returns the wrappedErrServerClosing; withnil, both resolve tocontext.Canceled— matching prior behavior.Note
Per the discussion in #1100, the read-side teardown also carries a descriptive cause (wrapping
ErrServerClosing) rather than a barecontext.Canceled.