Close the connection when an oversized fetch is refused - #553
Merged
Conversation
Fixes #545 A response body refused for exceeding a byte cap was left unread, and node-fetch holds the connection until the body stream is either consumed or destroyed. The declared-content-length check refuses before anything subscribes to the stream, so every over-cap source URL stranded one socket for as long as the server ran. The streamed-total check already disposed of the body, because leaving the read loop destroys the stream on the way out; only the declared-length path leaked. Measured against a loopback server: the refused connection stays open indefinitely, and destroying the body closes it at once. `readCapped` now destroys the body on any refusal, so the invariant holds at both cap checks and for both callers: the `*-from-url` upload tools and the capped SPARQL read behind `wikibase-query`. Aborting the `AbortController` that `fetchFileBytes` owns would close its own connection too, but it is neither necessary nor available to `postForm`, whose signal belongs to the caller. The regression test drives the real client against a loopback server, since the rest of the suite mocks node-fetch and cannot see a socket, and asserts that the server's end of the connection goes away. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The test server answered any unrecognised path with the chunked over-cap response, so a mistyped route in the streamed test still got a refusal to assert on and passed while exercising nothing it named. Unknown paths now answer 404, which the error-type assertion rejects. The socket the assertion reads is whichever one the server last served. It is now cleared before each test, and an assertion with no socket to observe fails rather than reporting an earlier test's closed connection as its own. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Lead with the three tools a reader scans for, and drop the mechanism the entry opened with. The old wording also hung the wikibase-query case off MCP_UPLOAD_MAX_BYTES, which governs only the upload tools, and counted the leak per URL rather than per call. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
node-fetch declares the body as the wider NodeJS.ReadableStream, which has no destroy(). Asserting past that means a body that is not a Node stream throws a TypeError from inside the catch, replacing the size-refusal it was reporting — and a TypeError does not rescue to wiki-side copy-upload, so a routine refusal would surface as an error. Narrowing by instanceof leaves such a body alone instead. Also narrows the changelog entry to the two released tools and to the setting that governs them. The capped SPARQL read passes a timeout signal, so its connection was freed after a minute rather than held for the life of the process, and both it and the tool that makes it arrive unreleased in this same cycle. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #545
A response body refused for exceeding a byte cap was left unread, and node-fetch holds the connection until the body stream is either consumed or destroyed. The declared-content-length check refuses before anything subscribes to the stream, so an over-cap source URL stranded one socket for as long as the server ran. The streamed-total check already disposed of the body, because leaving the read loop destroys the stream on the way out; only the declared-length path leaked.
readCappednow destroys the body on any refusal, so the invariant holds at both cap checks and for both callers: the*-from-urlupload tools and the capped SPARQL read behindwikibase-query. Only the upload path held its socket for the life of the process —wikibase-querypasses a 60-second timeout signal, which eventually freed the connection on its own. Aborting theAbortControllerthatfetchFileBytesowns would close its own connection too, but it is neither necessary nor available topostForm, whose signal belongs to the caller.The body is narrowed with
instanceof Readablerather than a type assertion: node-fetch declares it as the widerNodeJS.ReadableStream, which has nodestroy(), and a body that turned out not to be a Node stream should be left alone rather than throwing aTypeErrorover the refusal being reported — which would also stopupload-file-from-urlfalling back to wiki-side copy-upload.The regression test drives the real client against a loopback server, since the rest of the suite mocks node-fetch and cannot see a socket, and asserts that the server's end of the connection goes away. The server stalls its over-cap responses on purpose: node-fetch pumps a body that arrives complete into a buffer before the cap check runs, which returns the socket to the pool and hides the difference. An unknown route answers
404, and an assertion with no connection to observe fails rather than reading one an earlier test left behind.To decide: whether
fetchCoreneeds the same treatment, which this change deliberately leaves alone. It never reads a redirect response's body, so each hop leaves one behind — on the success path as well as at its three early exits (the redirect cap, aLocationthe guard refuses, and a cancellation between hops). Measured the same way, a 302 whose body the client never reads holds its socket indefinitely, while the socket of the hop that was followed returns to the pool. A short 3xx body is buffered whole and costs nothing, so this bites only where a source stalls or sends a large redirect body — which a caller-supplied URL can arrange. Worth its own change rather than widening this one.Verified: the two declared-length tests fail on master (
expected false to be true, the connection never closing) and pass with the fix; the streamed-total test passes on both, confirming the issue's reading of thefor awaitpath.npm run lint,npm run typecheck,npm run fmt:checkandnpm test(2050 tests) pass locally.