Skip to content

Fix falsy-zero requestId bugs in debounce guard and cancellation handling#2504

Open
dumbCodesOnly wants to merge 2 commits into
modelcontextprotocol:mainfrom
dumbCodesOnly:main
Open

Fix falsy-zero requestId bugs in debounce guard and cancellation handling#2504
dumbCodesOnly wants to merge 2 commits into
modelcontextprotocol:mainfrom
dumbCodesOnly:main

Conversation

@dumbCodesOnly

Copy link
Copy Markdown

Summary

Two separate call sites in packages/core-internal/src/shared/protocol.ts treat a valid requestId/relatedRequestId of 0 as absent, because JS treats 0 as falsy. Request IDs start at 0 (_requestMessageId = 0), so this is a reachable bug in normal operation, not a theoretical edge case.

Site 1 — #2117: debounce guard wrongly skips zero-id

In _notificationViaCodec:

const canDebounce = debouncedMethods.includes(notification.method)
    && !notification.params
    && !options?.relatedRequestId;

!options?.relatedRequestId is true both when relatedRequestId is undefined and when it's 0. A notification tied to request 0 could be incorrectly treated as debounce-eligible and silently coalesced with unrelated notifications of the same method — when it should never be debounced.

Fixed to options?.relatedRequestId === undefined.

Site 2 — #2283: cancellation notification dropped for request id 0

In _oncancel:

private async _oncancel(notification: CancelledNotification): Promise<void> {
    if (!notification.params.requestId) {
        return;   // drops the notification when requestId is 0
    }
    ...
}

A notifications/cancelled message for request id 0 was silently ignored, so the abort controller for that request never fired.

Fixed to explicitly check for undefined/null.

Testing

Added regression tests in packages/core-internal/test/shared/protocol.test.ts:

  • relatedRequestId: 0 must never be debounce-coalesced
  • notifications/cancelled with requestId: 0 must abort the correct request's controller

Both new tests fail against the pre-fix code and pass with the fix applied.

Fixes #2117
Fixes #2283

- _notificationViaCodec: canDebounce used `!options?.relatedRequestId`,
  which treats relatedRequestId === 0 the same as undefined. A
  notification tied to request 0 could be incorrectly coalesced with
  unrelated notifications of the same method.
- _oncancel: `if (!notification.params.requestId)` dropped
  notifications/cancelled for requestId 0, so the abort controller for
  request 0 never fired.

Request IDs start at 0 (_requestMessageId = 0), so both are reachable
in practice, not just theoretical edge cases.

Fixes modelcontextprotocol#2117, modelcontextprotocol#2283
- relatedRequestId: 0 must never be debounce-coalesced (issue modelcontextprotocol#2117)
- notifications/cancelled with requestId: 0 must abort the correct
  request's controller (issue modelcontextprotocol#2283)
@dumbCodesOnly
dumbCodesOnly requested a review from a team as a code owner July 15, 2026 18:06
@changeset-bot

changeset-bot Bot commented Jul 15, 2026

Copy link
Copy Markdown

⚠️ No Changeset found

Latest commit: bfa0fb2

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

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.

notifications/cancelled is ignored for request id 0 relatedRequestId 0 is treated as absent by notification debounce guard

1 participant