Skip to content

fix(v1.x): drop the request abort listener after the response arrives - #3091

Open
suryakant-source wants to merge 3 commits into
modelcontextprotocol:v1.xfrom
suryakant-source:fix-request-abort-listener-cleanup
Open

suryakant-source wants to merge 3 commits into
modelcontextprotocol:v1.xfrom
suryakant-source:fix-request-abort-listener-cleanup

Conversation

@suryakant-source

Copy link
Copy Markdown

Fixes #3090 (v1.x backport).

Protocol.request() on v1.x registers its abort listener on options.signal and never removes it. Once the request resolves, a caller that later aborts the controller still runs cancel(), which POSTs a notifications/cancelled for an already-answered request — and the listener itself leaks for the lifetime of the signal. The v2 line (packages/core-internal/src/shared/protocol.ts) already fixed this; this PR backports that pattern with no behavior change for in-flight requests:

  • cancel() returns early once the response has arrived (responseReceived guard), so no late notifications/cancelled goes on the wire.
  • The listener is registered as a named onAbort with { once: true }.
  • A .finally() removes the listener when the request settles, so a resolved request leaves nothing attached to the caller's AbortSignal.

Tests

Two new cases in test/shared/protocol.test.ts:

  • after the response arrives, the abort listener is removed and a late abort() sends no notifications/cancelled;
  • an abort before the response still sends exactly one notifications/cancelled (the existing behavior this must not break).

The first test fails against unfixed v1.x (verified by stashing the source change) and passes with it. Full non-e2e suite: 56 files, 1860 tests, all pass. tsc --noEmit, ESLint and Prettier are clean.

@suryakant-source
suryakant-source requested a review from a team as a code owner October 24, 2026 00:28
@changeset-bot

changeset-bot Bot commented Oct 24, 2026 •

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: f9a3f88

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@pkg-pr-new

pkg-pr-new Bot commented Oct 24, 2026 •

Copy link
Copy Markdown

Open in StackBlitz

npm i https://pkg.pr.new/@modelcontextprotocol/sdk@3091

commit: f9a3f88

@yaohuangguan yaohuangguan left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Independent review of f9a3f88 against v1.x.

I checked the responseReceived guard, { once: true } listener, and .finally() cleanup in Protocol.request(). These keep a completed response from emitting a late notifications/cancelled, while retaining cancellation before the response.

I ran the actual PR checkout locally on Node 24.21.0 / Vitest 4.0.8:

  • test/shared/protocol.test.ts: 136/136 passing.
  • Two additional local-only regression cases (not pushed) passed: a JSON-RPC error response followed by AbortController.abort() produces no late cancellation, and aborting after a timed-out request produces no second cancellation. The focused set passed 4/4 including the PR's two tests.

No blocking issue found in the touched paths. Non-blocking test suggestion: add one settled-error/timeout case to the PR suite to protect the .finally() cleanup guarantee beyond successful responses. The extra cases above passed without implementation changes.

The upstream CI is green; I did not independently rerun the complete repository suite.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

v1 Issues / PRs related to v1.x

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants