Skip to content

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

Description

@aha-jansen

Background

handleAsync in internal/jsonrpc2/conn.go checks whether an incoming request was already canceled before dispatching it to the handler. When it was, it tries to report why. Today it can only recover the reason by inspecting the connection-level s.writeErr, guarding it behind a TODO:

// 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)
}
})
c.processResult("handleAsync", req, nil, err)
continue

The referenced Go issue (golang/go#51365) shipped as context.WithCancelCause / context.Cause in Go 1.21, and this module already targets Go 1.25, so the API the TODO was waiting for is now available.

Problem

Beyond being a stale TODO, the current approach has a latent mis-attribution: s.writeErr is connection-level state, not per-request. Any request canceled for an unrelated reason (e.g. a peer cancel notification via Cancel, or a read-side teardown) will be reported as ErrServerClosing as long as s.writeErr happens to be non-nil at that moment. The cancellation reason is not tied to the specific request that was canceled.

Proposal

Carry the cancellation cause on each request's context instead of inferring it from global state:

  • Change incomingRequest.cancel from context.CancelFunc to context.CancelCauseFunc (build it via context.WithCancelCause).
  • At the write-failure teardown, cancel each in-flight request with the cause: r.cancel(fmt.Errorf("%w: %v", ErrServerClosing, err)).
  • Other cancel sites (Cancel, read-side teardown, normal completion) pass nil, preserving the existing context.Canceled behavior.
  • In handleAsync, replace the s.writeErr lookup with context.Cause(req.ctx).

ctx.Err() still returns context.Canceled in all cases (unchanged), so external behavior is preserved; only the reported cause becomes accurate and per-request. Net effect on conn.go is a small diff across ~6 sites and removes the guessing/global-state read from the hot path.

Questions before I open a PR

  1. Would you welcome a PR for this, or is the current behavior intentional for now?
  2. Any preference on scope — cause-plumbing only for the write-failure path (the one the TODO calls out), or should read-side teardown also carry a descriptive cause rather than bare context.Canceled?

I have a working change (all internal/... and mcp tests pass) and am happy to send it once you confirm the direction.

Environment

  • go-sdk: current main
  • Go: 1.25

Activity

  1. guglielmo-san commented on Jul 17, 2026

    @guglielmo-san
    Contributor

    Hi @aha-jansen, thank you for your detailed breakdown. For me you can proceed with the proposed PR, including the cause at Read-side teardown

  2. added
    P2Moderate issues, valuable feature requests
    on Aug 6, 2026
  3. added a commit that references this issue on Aug 21, 2026
    fe27fa9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    P2Moderate issues, valuable feature requests

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions