Skip to content

fix: wait for pending requests before aborting transports on close #1692

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
travisbreaks wants to merge 2 commits into from
+48 −1
Closed

fix: wait for pending requests before aborting transports on close #1692

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c9dbfab
fix(transport): wait for in-flight requests before aborting on close
travisbreaks Mar 17, 2026
ce43e03
style: apply prettier formatting
felixweinberger Mar 23, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 23 additions & 1 deletion packages/client/src/client/sse.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@
private _fetch?: FetchLike;
private _fetchWithInit: FetchLike;
private _protocolVersion?: string;
private _pendingRequests = new Set<Promise<void>>();

onclose?: () => void;
onerror?: (error: Error) => void;
Expand Down Expand Up @@ -238,6 +239,15 @@
}

async close(): Promise<void> {
// Wait for any in-flight requests to complete before aborting, so that
// successful responses are not marked as aborted by Undici/OpenTelemetry.
if (this._pendingRequests.size > 0) {
const GRACEFUL_CLOSE_TIMEOUT = 2000;
await Promise.race([
Promise.allSettled(this._pendingRequests),
new Promise<void>(resolve => setTimeout(resolve, GRACEFUL_CLOSE_TIMEOUT))
]);

Check warning on line 249 in packages/client/src/client/sse.ts

View check run for this annotation

Claude / Claude Code Review

Graceful close timeout timer is never cleared 

Nit: The `setTimeout` fallback in `Promise.race` within `close()` is never cleared when `Promise.allSettled` resolves first. In Node.js, an active timer keeps the event loop alive, so the process cannot exit for up to 2 seconds after `close()` completes — this will cause test runners (Jest/Vitest) to report open handles. Fix by storing the timer ID and calling `clearTimeout` after the race, or by using `setTimeout(...).unref()`. Same issue exists in `streamableHttp.ts` line 462.
Comment on lines +243 to +249
Copy link
Copy Markdown

@claude claude bot Mar 27, 2026

Choose a reason for hiding this comment

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

🟡 Nit: The setTimeout fallback in Promise.race within close() is never cleared when Promise.allSettled resolves first. In Node.js, an active timer keeps the event loop alive, so the process cannot exit for up to 2 seconds after close() completes — this will cause test runners (Jest/Vitest) to report open handles. Fix by storing the timer ID and calling clearTimeout after the race, or by using setTimeout(...).unref(). Same issue exists in streamableHttp.ts line 462.

Extended reasoning...

What the bug is

In both sse.ts (line 246) and streamableHttp.ts (line 462), the close() method uses Promise.race between Promise.allSettled(this._pendingRequests) and a setTimeout-based promise as a 2-second fallback. However, the timer ID from setTimeout is never stored, so when Promise.allSettled wins the race (the common case — pending requests complete quickly), the timer is never cleared.

Why this matters

In Node.js, an active timer handle keeps the event loop alive. This means that even after close() has resolved and all cleanup is done, the process cannot exit for up to 2 seconds while waiting for the orphaned timer to fire. This is particularly problematic for:

  • Test suites: Jest and Vitest will report "open handles" warnings and may hang for 2 seconds after each test that calls close() with pending requests.
  • CLI tools: Short-lived processes will appear to hang briefly before exiting.
  • Any scenario requiring prompt process exit after closing the transport.

Step-by-step proof

  1. A client sends a request via send(), which adds a tracked promise to _pendingRequests.
  2. The server responds quickly (say, within 50ms).
  3. The caller invokes close(). Since _pendingRequests.size > 0, the code enters the Promise.race block.
  4. Promise.allSettled resolves almost immediately (the request already completed).
  5. Promise.race resolves, and close() proceeds to abort and clean up.
  6. However, the setTimeout(resolve, 2000) timer is still active in the event loop — nobody stored its ID or called clearTimeout.
  7. The process cannot exit for ~2 seconds until the timer fires.

How to fix

The simplest fix is to use .unref() on the timer, which tells Node.js not to keep the event loop alive for it:

new Promise<void>(resolve => setTimeout(resolve, GRACEFUL_CLOSE_TIMEOUT).unref())

Alternatively, store the timer ID and clear it after the race:

let timerId: ReturnType<typeof setTimeout>;
const timeout = new Promise<void>(resolve => { timerId = setTimeout(resolve, GRACEFUL_CLOSE_TIMEOUT); });
await Promise.race([Promise.allSettled(this._pendingRequests), timeout]);
clearTimeout(timerId\!);

Both approaches are straightforward and prevent the delayed-exit behavior.

}
this._abortController?.abort();
this._eventSource?.close();
this.onclose?.();
Expand All @@ -248,6 +258,18 @@
throw new SdkError(SdkErrorCode.NotConnected, 'Not connected');
}

const requestPromise = this._doSend(message);
// Track the promise for graceful close, but suppress unhandled rejections
// on the tracked copy since the caller handles the actual rejection.
const tracked = requestPromise.catch(() => {});
this._pendingRequests.add(tracked);
tracked.finally(() => {
this._pendingRequests.delete(tracked);
});
return requestPromise;
}

private async _doSend(message: JSONRPCMessage): Promise<void> {
try {
const headers = await this._commonHeaders();
headers.set('content-type', 'application/json');
Expand All @@ -259,7 +281,7 @@
signal: this._abortController?.signal
};

const response = await (this._fetch ?? fetch)(this._endpoint, init);
const response = await (this._fetch ?? fetch)(this._endpoint!, init);
if (!response.ok) {
const text = await response.text?.().catch(() => null);

Expand Down
25 changes: 25 additions & 0 deletions packages/client/src/client/streamableHttp.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,7 @@ export class StreamableHTTPClientTransport implements Transport {
private _lastUpscopingHeader?: string; // Track last upscoping header to prevent infinite upscoping.
private _serverRetryMs?: number; // Server-provided retry delay from SSE retry field
private _reconnectionTimeout?: ReturnType<typeof setTimeout>;
private _pendingRequests = new Set<Promise<void>>();

onclose?: () => void;
onerror?: (error: Error) => void;
Expand Down Expand Up @@ -452,13 +453,37 @@ export class StreamableHTTPClientTransport implements Transport {
clearTimeout(this._reconnectionTimeout);
this._reconnectionTimeout = undefined;
}
// Wait for any in-flight requests to complete before aborting, so that
// successful responses are not marked as aborted by Undici/OpenTelemetry.
if (this._pendingRequests.size > 0) {
const GRACEFUL_CLOSE_TIMEOUT = 2000;
await Promise.race([
Promise.allSettled(this._pendingRequests),
new Promise<void>(resolve => setTimeout(resolve, GRACEFUL_CLOSE_TIMEOUT))
]);
}
this._abortController?.abort();
this.onclose?.();
}

async send(
message: JSONRPCMessage | JSONRPCMessage[],
options?: { resumptionToken?: string; onresumptiontoken?: (token: string) => void }
): Promise<void> {
const requestPromise = this._doSend(message, options);
// Track the promise for graceful close, but suppress unhandled rejections
// on the tracked copy since the caller handles the actual rejection.
const tracked = requestPromise.catch(() => {});
this._pendingRequests.add(tracked);
tracked.finally(() => {
this._pendingRequests.delete(tracked);
});
return requestPromise;
}

private async _doSend(
message: JSONRPCMessage | JSONRPCMessage[],
options?: { resumptionToken?: string; onresumptiontoken?: (token: string) => void }
): Promise<void> {
try {
const { resumptionToken, onresumptiontoken } = options || {};
Expand Down
Loading