Skip to content

fix(rmcp): route peer cancellation by lifecycle - #1262

Open
aurokin wants to merge 4 commits into
modelcontextprotocol:mainfrom
aurokin:aurokin-publish-cancellation-fix
Open

fix(rmcp): route peer cancellation by lifecycle#1262
aurokin wants to merge 4 commits into
modelcontextprotocol:mainfrom
aurokin:aurokin-publish-cancellation-fix

Conversation

@aurokin

@aurokin aurokin commented Sep 12, 2026

Copy link
Copy Markdown

Fixes #1251.

Implements the approach agreed to in #1251 (comment).

Problem

In legacy sessions, incoming server notifications/cancelled messages remove an outbound client waiter instead of cancelling the server-originated request's RequestContext.ct. This leaves the handler running and can affect an unrelated request with the same ID.

Removing outbound cleanup entirely would break modern subscription termination.

Fix

Route cancellation by the established lifecycle:

  • Cancel the exact inbound token for legacy requests.
  • Retire only an actual outbound subscriptions/listen waiter for modern server cancellation, independently of notification-channel registration.
  • Reuse existing startup metadata so discovery retains modern semantics even when selecting an older application version.
  • Keep string and numeric request IDs distinct.
  • Suppress cancelled handler responses and errors during EOF draining while preserving uncancelled responses during graceful shutdown.

No dependency, wire-schema, or macro changes. Outgoing server teardown and same-direction ID reuse are outside this fix.

Regression coverage

The tests cover string-ID reverse cancellation on a live connection, handler-token observation, response suppression, subsequent traffic, legacy initialization, direct peers, unknown IDs, and equal IDs in opposite directions.

The subscription regression inspects the actual waiter after an ordered ping. It covers modern discovery selecting either application version, ordinary outbound request immunity, and listen requests without a registered notification channel.

Drain regressions cover successful and failed cancelled handlers and graceful completion of uncancelled handlers. Existing RoleServer stdio coverage remains in place.

Validation

The clean candidate passed:

  • Targeted tests: 209 unit, 5 cancellation, 12 subscription, and 1 drain.
  • Nightly formatting and focused Clippy.
  • SemVer and public-API checks with default and non-local features.
  • Full-range commitlint.

Full all-feature and non-local suites passed on the same production source before the final test-only error-response variant. That variant passed targeted validation, including against the published release with its original lockfile.

The same cancellation files also passed targeted validation after merging upstream 3023198 in a separate development branch. That merge is not included in this single-commit PR head. The regressions fail against unpatched upstream; an independent reproduction on 46db531 is recorded in #1251.

Full Clippy previously failed at an unchanged clippy::question_mark warning in auth.rs. The unchanged base reproduced the same warning with the same compiler and lockfile. No unrelated lint fix or suppression is included.

Cancel inbound legacy requests by their exact request ID. Preserve modern
subscription waiter cleanup using the peer lifecycle and outbound request kind.

Suppress cancelled handler results and errors during EOF draining without
dropping uncancelled responses during graceful shutdown.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@aurokin
aurokin requested a review from a team as a code owner September 12, 2026 00:19
@github-actions github-actions Bot added T-test Testing related changes T-core Core library changes T-service Service layer changes labels Sep 12, 2026
Comment thread crates/rmcp/src/service.rs
Comment thread crates/rmcp/src/service.rs
Comment thread crates/rmcp/tests/test_cancelled_response.rs Outdated
Hunter Sadler and others added 3 commits September 12, 2026 16:13
Use rstest to report each startup and request ID combination independently.
Keep the existing per-case timeout and cancellation assertions.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Merge upstream main fbfc5cc without rewriting history.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Replace the stale ServerInfo reference after merging the upstream alias migration.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

T-core Core library changes T-service Service layer changes T-test Testing related changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Client ignores legacy reverse-request cancellation and may cancel an unrelated outbound request

2 participants