Repository navigation
fix(client): do not wait for the end of an unread POST reply body - #3082
claude[bot] wants to merge 2 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: d02a991 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}
|
|
||
| /** 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) SSE-transport users can get a fresh connection per POST against ordinary servers, where the base reused one keep-alive connection. releaseBody calls body.cancel() in the same tick the headers resolve (sse.ts:495), so a short body arriving in a later read is cut off and the socket closed. Servers that write headers and body separately (uvicorn answering 202 with Accepted) hit this. Fix: never block send(), yet let a reply that ends promptly finish, e.g. read un-awaited and cancel after a short deadline, covering all 4 call sites. The changeset names the close but frames it as the held-open case. [also at: packages/client/src/client/streamableHttp.ts:1168 - Servers that flush the reply headers and finish the work before ending the body now have the POST connection closed mid-work; on the base branch the client waited.]
Why this was flagged
Trigger: a successful POST reply whose body bytes (or chunked terminator) reach the client in a later socket read or TLS record than the headers, e.g. uvicorn/Starlette Response("Accepted", 202), which writes headers and body with two transport writes. Entry points: SSEClientTransport._send packages/client/src/client/sse.ts:451-495 (every client message) and StreamableHTTPClientTransport._send packages/client/src/client/streamableHttp.ts:1168, 1212, 1219. Nothing is awaited between the fetch resolving and releaseBody(response), so packages/core-internal/src/shared/transport.ts:116 calls response.body.cancel() before the next read is processed. The base awaited response.text?.(), which read the body to its end and left the connection reusable. Result: a TCP (and TLS) handshake per message and connection resets on the server. No safeguard applies: the changeset sentence 'when part of it has not arrived yet, the connection is closed instead of reused' describes the mechanism but nothing limits it to held-open replies.
Verification: packages/core-internal/src/shared/transport.ts:115-118: releaseBody calls response.body.cancel() immediately and un-awaited, with no grace period for a body that is about to end. Base behaviour: the removed line await response.text?.().catch(() => {}) read the body to its end, which let the keep-alive connection return to the pool.
Finding ID: f-8c58c99d
| 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
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 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 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
cancel()). Such a body is still read withresponse.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.
Generated by Claude Code