Skip to content

relatedRequestId 0 is treated as absent by notification debounce guard #2117

@tylerklose

Description

@tylerklose

Problem

Protocol.notification() correctly avoids debouncing notifications when relatedRequestId is present, because debouncing can change delivery semantics for request-associated messages.

The guard currently uses a truthiness check:

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

That treats relatedRequestId: 0 as absent. 0 is a valid MCP/JSON-RPC request id, and it is also the first id a Protocol instance uses.

This looks like the same falsy-id pattern as #2115, but in a separate path. #2116 changes _oncancel; it does not appear to change this debounce guard.

Why it matters

For a simple notification whose method is configured in debouncedNotificationMethods:

  • relatedRequestId: 1 is not debounced
  • relatedRequestId: "0" is not debounced
  • relatedRequestId: 0 is debounced

That can produce observable differences:

  1. Multiple same-tick related notifications for request id 0 are coalesced even though related notifications are supposed to bypass debouncing.
  2. If transport.send() rejects, notification() resolves and routes the error to onerror, while non-zero related ids reject the returned promise.

Reproducer

This standalone probe drives Protocol.notification() directly:

import { Protocol } from "./packages/core/src/shared/protocol.js";
import type { Transport, TransportSendOptions } from "./packages/core/src/shared/transport.js";
import type { JSONRPCMessage, Notification, Request, RequestId, Result } from "./packages/core/src/types.js";

class ProbeTransport implements Transport {
  onclose?: () => void;
  onerror?: (error: Error) => void;
  onmessage?: (message: JSONRPCMessage) => void;
  sent: Array<{ message: JSONRPCMessage; options?: TransportSendOptions }> = [];
  rejectSend = false;

  async start(): Promise<void> {}
  async close(): Promise<void> { this.onclose?.(); }

  async send(message: JSONRPCMessage, options?: TransportSendOptions): Promise<void> {
    if (this.rejectSend) throw new Error("send failed");
    this.sent.push({ message, options });
  }
}

class ProbeProtocol extends Protocol<Request, Notification, Result> {
  protected assertCapabilityForMethod(): void {}
  protected assertNotificationCapability(): void {}
  protected assertRequestHandlerCapability(): void {}
  protected assertTaskCapability(): void {}
  protected assertTaskHandlerCapability(): void {}
}

async function run(relatedRequestId: RequestId, rejectSend = false) {
  const transport = new ProbeTransport();
  transport.rejectSend = rejectSend;
  const protocol = new ProbeProtocol({
    debouncedNotificationMethods: ["probe/debounced"],
  });

  let onerrorCalled = false;
  protocol.onerror = () => { onerrorCalled = true; };
  await protocol.connect(transport);

  let rejected = false;
  await Promise.all([
    protocol.notification({ method: "probe/debounced" }, { relatedRequestId }),
    protocol.notification({ method: "probe/debounced" }, { relatedRequestId }),
  ]).catch(() => { rejected = true; });

  await new Promise(resolve => setTimeout(resolve, 0));

  return {
    relatedRequestId,
    rejectSend,
    sentCount: transport.sent.length,
    rejected,
    onerrorCalled,
  };
}

console.log(await run(1));
console.log(await run("0"));
console.log(await run(0));
console.log(await run(1, true));
console.log(await run(0, true));

Observed on v1.x commit bf1e022bd219f678b3865093d58595c6c8a67f1a:

{ relatedRequestId: 1, sentCount: 2, rejected: false, onerrorCalled: false }
{ relatedRequestId: '0', sentCount: 2, rejected: false, onerrorCalled: false }
{ relatedRequestId: 0, sentCount: 1, rejected: false, onerrorCalled: false }
{ relatedRequestId: 1, rejectSend: true, sentCount: 0, rejected: true, onerrorCalled: false }
{ relatedRequestId: 0, rejectSend: true, sentCount: 0, rejected: false, onerrorCalled: true }

Expected behavior

relatedRequestId: 0 should behave like every other present related request id:

  • it should bypass notification debouncing
  • duplicate same-tick related notifications should not be coalesced by the global debounce path
  • notification() should reject if transport.send() rejects

Proposed fix

Use an explicit presence check:

const hasRelatedRequestId = options?.relatedRequestId !== undefined;

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

Notes

This is intentionally narrow. It does not change normal debouncing for global simple notifications without a relatedRequestId.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions