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
    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions