Skip to content

internal/jsonrpc2: plumb cancellation cause through request context#1103

Open
aha-jansen wants to merge 1 commit into
modelcontextprotocol:mainfrom
aha-jansen:fix/cancel-cause-plumbing
Open

internal/jsonrpc2: plumb cancellation cause through request context#1103
aha-jansen wants to merge 1 commit into
modelcontextprotocol:mainfrom
aha-jansen:fix/cancel-cause-plumbing

Conversation

@aha-jansen

Copy link
Copy Markdown

Closes #1100.

What

handleAsync in internal/jsonrpc2/conn.go reported why an in-flight request had been canceled by inspecting the connection-level s.writeErr, guarded by a TODO waiting on golang/go#51365:

// Only deliver to the Handler if not already canceled.
if err := req.ctx.Err(); err != nil {
    c.updateInFlight(func(s *inFlightState) {
        if s.writeErr != nil {
            // Assume that req.ctx was canceled due to s.writeErr.
            // TODO(#51365): use a Context API to plumb this through req.ctx.
            err = fmt.Errorf("%w: %v", ErrServerClosing, s.writeErr)
        }
    })
    ...
}

That API shipped as context.WithCancelCause / context.Cause in 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.cancel becomes a context.CancelCauseFunc (built via context.WithCancelCause).
  • The write-failure and read-side teardowns cancel each in-flight request with a cause wrapping ErrServerClosing.
  • The remaining cancel sites (peer cancel via Cancel, normal completion) pass nil.
  • handleAsync reads context.Cause(req.ctx) instead of looking up s.writeErr.

Why

Besides resolving the TODO, this fixes a latent mis-attribution. s.writeErr is connection-level state, not per-request: a request canceled for an unrelated reason (e.g. a peer cancel notification via Cancel) was reported as ErrServerClosing whenever s.writeErr happened to be non-nil at that moment. Tying the cause to the specific request's context removes that coupling.

ctx.Err() still returns context.Canceled in 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 longer mcp integration tests) — all pass.
  • Verified context.WithCancelCause semantics: with a cause, Err() stays context.Canceled while Cause() returns the wrapped ErrServerClosing; with nil, both resolve to context.Canceled — matching prior behavior.

Note

Per the discussion in #1100, the read-side teardown also carries a descriptive cause (wrapping ErrServerClosing) rather than a bare context.Canceled.

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
aha-jansen force-pushed the fix/cancel-cause-plumbing branch from 2e16971 to 04327e3 Compare July 18, 2026 03:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

internal/jsonrpc2: plumb cancellation cause through req.ctx (resolve TODO #51365)

1 participant