Repository navigation
fix(client): do not wait for the end of an unread POST reply body - #3082
claude[bot] wants to merge 4 commits into
Conversation
**Before:** after a successful POST whose reply body the client does not read (a 202, a 2xx reply to a message with no requests, a 2xx reply with an unexpected content type, any OK reply on the SSE transport), `send()` awaited `response.text()`, which settles only when the body ends. A server or proxy that sends the headers and keeps the response open stalled `send()`, and with it `Client.connect()`. **After:** the body is cancelled without awaiting. A fully received reply keeps its connection for reuse; a partly received one has it closed. **History of these lines:** #1214 added `await response.body?.cancel()`. #1320 replaced it with `await response.text()`, because `cancel()` could hang. Both forms wait, and both waits can hang: `cancel()` on a response that a custom fetch has cloned does not settle until the other branch is consumed. Not awaiting avoids both; a test covers the cloned case. A custom fetch may return a body that is not a WHATWG stream (node-fetch returns a Node stream, which has no `cancel()`). Such a body is still read with `response.text()`, also without awaiting. The four sites share one internal helper, `releaseBody()`, which never throws or rejects. The non-2xx drains, the GET stream and the DELETE are left as they are.
🦋 Changeset detectedLatest commit: 4db666d The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 packages
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 |
@modelcontextprotocol/client
@modelcontextprotocol/codemod
@modelcontextprotocol/core
@modelcontextprotocol/server
@modelcontextprotocol/server-legacy
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| return `Redirect to ${target.href} not followed; use that URL as the endpoint if it is the intended server`; | ||
| } | ||
|
|
||
| /** Releases the unread body of `response` without waiting for it; reads it to its end when it has no `cancel()`. Never throws. */ |
There was a problem hiding this comment.
🟡 nit (optional): maintainers get a JSDoc block on a helper that is not public API, which CLAUDE.md principle 8 asks to avoid. releaseBody at packages/core-internal/src/shared/transport.ts:115 is exported only through the private core-internal barrel, not through exports/public/index.ts. Fix: make the comment at line 114 a one-line // comment, or drop it and keep the reasoning in the PR description. Neighbouring internal helpers in this file (lines 51, 70, 77, 101) already carry the same one-line /** */ form, so this follows local precedent and nothing breaks at runtime. [also at: packages/core-internal/src/shared/transport.ts:114 - nit: CLAUDE.md (Brief comments) keeps JSDoc to the public API: the new releaseBody helper carries a /** ... */ JSDoc block, but it is internal — it lives in the private @ modelcontextprotocol/core-internal package and is not re-exported from src/exports/public/index.ts.]
Why this was flagged
No runtime trigger; this is a convention check. packages/core-internal/src/shared/transport.ts:114 adds a /** ... */ JSDoc to releaseBody. The function is reachable only via export * from './shared/transport' in packages/core-internal/src/index.ts:21; packages/core-internal/src/exports/public/index.ts:66-67 re-exports only FetchLike, Transport, TransportSendOptions and createFetchWithInit from that file, and packages/core-internal/package.json marks the package "private": true. So the documented symbol is internal. CLAUDE.md principle 8 limits JSDoc to the public API and wants inline comments kept to one line. The base branch has no such comment because the helper is new. No safeguard applies: Prettier and ESLint do not enforce this. The comment text itself is accurate (cancel when body.cancel is a function, otherwise response.text?.(), rejection swallowed).
Verification: nit — triggering condition: none at runtime; this is a rule check on a line the diff adds. packages/core-internal/src/shared/transport.ts:114, added by this diff, is a /** ... */ JSDoc block on releaseBody (line 115). So JSDoc syntax is used on a non-public helper, which breaks the rule as written. Nothing observable changes for users.
Finding ID: f-1893d194
|
|
||
| /** Releases the unread body of `response` without waiting for it; reads it to its end when it has no `cancel()`. Never throws. */ | ||
| export function releaseBody(response: Response): void { | ||
| const release = async () => (typeof response.body?.cancel === 'function' ? response.body.cancel() : response.text?.()); |
There was a problem hiding this comment.
🟡 (optional) Clients whose fetch returns a body with no cancel() (node-fetch, React Native) now leave one socket and one pending read open per POST when a server holds replies open. releaseBody at transport.ts:116 falls back to an unawaited response.text?.(). Nothing ends that read until close() aborts the transport signal. The base stalled on the first such send(); now they pile up, once per message on the SSE transport. Fix: make the release end the reply on every fetch, for example by aborting a per-POST signal instead of reading. A cloned response has the same gap, because cancel() on one branch leaves the reply open.
Why this was flagged
Trigger: a custom fetch whose reply body has no cancel() (the PR's own case 'a fetch whose reply body is a Node stream, which has no cancel()', heldOpenPostReply.test.ts:84) and a server that sends headers and never ends the reply (holdOpen, heldOpenPostReply.test.ts:22). releaseBody (transport.ts:116) calls response.text?.() and drops the promise; that read settles only when the body ends, so the socket and the buffered read stay alive. The only line that ends them is this._abortController?.abort() in close() (streamableHttp.ts:973, sse.ts:426). On the base branch send() awaited the same read, so a sequential caller never had more than one such reply open; after the merge each send() returns and the next adds another, with no cap for the life of the transport. No safeguard applies: .catch(() => {}) at transport.ts:117 only swallows the rejection.
Verification: Triggering condition: a custom fetch whose reply body has no cancel(), talking to a server that never ends the POST reply. packages/core-internal/src/shared/transport.ts:116: with no cancel(), the fallback is an unawaited response.text(), which only settles when the body ends. Base await response.text?.().catch(() => {}) never returned, so the first such send() hung.
Finding ID: f-8d01272d
| // If the response is 202 Accepted, there's no body to process | ||
| if (response.status === 202) { | ||
| await response.text?.().catch(() => {}); | ||
| releaseBody(response); |
There was a problem hiding this comment.
🟣 pre-existing, not blocking: pre-existing: A server or proxy that holds a reply open still stalls terminateSession() and any send() answered with a 401, exactly as on the base branch. Only the four 2xx POST drains were converted. Other drains whose text is thrown away still run await response.text?.(). Fix: release every discarded reply body without awaiting, for example with releaseBody(response), which covers the 5 sites listed. Same pattern at 5 sites (streamableHttp.ts:1095, streamableHttp.ts:1099, streamableHttp.ts:1266, sse.ts:472, sse.ts:476). The description leaves these out on purpose, but the DELETE reply is as unread as a 202.
A small fix can ride a push you are already making; otherwise a short reply is enough.
Why this was flagged
Trigger (peer behaviour, not reproducible from this checkout): the peer the changeset names, one that sends headers and keeps the response open, answers a DELETE with 200 or a POST with 401 while an authProvider is set. terminateSession() at packages/client/src/client/streamableHttp.ts:1266 runs await response.text?.().catch(() => {}) before it reads the status, so it never settles. _send() awaits the same discarded drain at :1095 (after onUnauthorized, before the retry) and :1099 (before throwing UnauthorizedError), and packages/client/src/client/sse.ts:472 and :476 do the same, so send() and a Client.connect() whose first POST is challenged still hang. The base branch behaves identically; the diff changes only :1168, :1212, :1219 and sse.ts:495. No timeout guards these awaits; only transport.close() aborting _abortController ends them. The drains at :1111 and sse.ts:488 use the text and are not part of this.
Verification: pre-existing — triggers when a server or proxy sends headers and holds the response open on a DELETE reply, or on a 401 POST reply while an authProvider is set. packages/client/src/client/streamableHttp.ts:1265-1266 — terminateSession() runs await response.text?.().catch(() => {}); before the status check at :1270. None of the lines above appear in the diff, so the base fails the same way by the same route.
Finding ID: f-a05dae63
claude
Bot
added
the
v2
**Before:** `releaseBody()` cancelled the unread body of a 2xx POST reply at once. That ended the stall on a reply that is never ended, but it also cut replies of normal servers whose end had not arrived yet. Measured against `main`, with nothing held open: about one new connection per 202 through a TLS reverse proxy built on Go's `httputil.ReverseProxy` (85 to 107 connections against 3 to 8, for 100 notifications and 100 calls), from a workerd client to this SDK's servers (50 to 52 against 0 to 2, for 50 notifications) and on the SSE transport to a Python server over TLS (206 against 3); Chromium logged each 202 POST as `net::ERR_ABORTED`; and with a custom `fetch` that returns `response.clone()`, `close()` was followed by an unhandled `AbortError` on Node 20 and 22. **After:** the body is read to its end, still without `send()` waiting for it, and is cancelled only if it has not ended after 100 ms. The same runs now give the results of `main`. A reply that is never ended still does not stall `send()` or `Client.connect()`; its connection closes after 100 ms instead of at once. The timer is cleared when the body ends or fails. A body with no `getReader()` (node-fetch) is read with `response.text()`, also without waiting, and is never cancelled. **Still open:** only POST replies with a 2xx status are covered. Replies with another status, the redirect drain and `terminateSession()` wait as before. When the reply has not ended after 100 ms and the custom `fetch` returns `response.clone()`, a later `close()` is still followed by the unhandled `AbortError` on Node 20 and 22. Each of the four call sites now has tests that fail when its call is deleted.
There was a problem hiding this comment.
No new blocking issues. 1 optional suggestion (a nit or a note on pre-existing code) was found and not posted. Nothing in this review needs a push before merging.
Still open from earlier reviews (3):
- Unresolved: 3 minor or pre-existing.
This review covers commit c140325, which is no longer the latest commit on this pull request; later commits are not covered by it.
Requested by Felix Weinberger · Slack thread
Before: after a successful POST whose reply body the client does not read (a 202, a 2xx reply to a message with no requests, a 2xx reply with an unexpected content type, any OK reply on the SSE transport),
send()awaitedresponse.text(), which settles only when the body ends. A server or proxy that sends the headers and keeps the response open stalledsend(), and with itClient.connect().After: the body is still read to its end, but
send()does not wait for that. A body that has not ended after 100 ms is cancelled. A reply of a normal server ends by itself and keeps its connection, as onmain. A reply that is never ended no longer stallssend()orClient.connect(); its connection closes after 100 ms.Why not cancel at once: the first commit of this PR did. It ended the stall, but it also cut replies of normal servers whose end had not arrived yet. Measured against
main, with nothing held open: about one new connection per 202 through a TLS reverse proxy built on Go'shttputil.ReverseProxy(85 to 107 connections against 3 to 8, for 100 notifications and 100 calls), from a workerd client to this SDK's servers (50 to 52 against 0 to 2, for 50 notifications) and on the SSE transport to a Python server over TLS (206 against 3); Chromium logged each 202 POST asnet::ERR_ABORTED; and with a customfetchthat returnsresponse.clone(),close()was followed by an unhandledAbortErroron Node 20 and 22. With the second commit the same runs give the results ofmain.History of these lines: #1214 added
await response.body?.cancel(). #1320 replaced it withawait response.text(), becausecancel()could hang. Both forms wait, and both waits can hang:cancel()on a response that a custom fetch has cloned does not settle until the other branch is consumed. Not awaiting avoids both; a test covers the cloned case.A custom fetch may return a body that is not a WHATWG stream (node-fetch returns a Node stream, which has no
getReader()). Such a body is read withresponse.text(), also without awaiting, and is never cancelled. The four sites share one internal helper,releaseBody(), which never throws or rejects. Each site has tests that fail when its call is deleted.Still open: only POST replies with a 2xx status are covered. Replies with another status, the redirect drain, the GET stream and
terminateSession()are left as they are. When the reply has not ended after 100 ms and the customfetchreturnsresponse.clone(), a laterclose()is still followed by the unhandledAbortErroron Node 20 and 22. While a reply is held open it occupies its connection for up to 100 ms, so a client with a small connection pool sends more slowly to such a server. A server whose reply takes longer than 100 ms to end has its connection closed, and any work it ties to that connection is aborted, whilesend()has already resolved. A body with nogetReader(), or a reply a customfetchholds a clone of, stays open untilclose()if the server never ends it.Generated by Claude Code