[Fix] Cancel safeFetch body reader when maxSize exceeded#523
Conversation
advancedresearcharray
requested review from
HiddenPuppy,
mvanhorn and
samzong
as code owners
|
Hi @advancedresearcharray, DetailsInstructions for interacting with me using comments are available here. |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request improves the robustness of the safeFetch utility by ensuring that the response body reader is explicitly cancelled when a stream exceeds the defined size limit. This prevents unnecessary buffering and ensures that network resources are released promptly upon hitting a memory limit, effectively mitigating potential memory exhaustion issues. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the safeFetch utility to cancel the stream reader when the response size exceeds maxSize, and adds comprehensive unit tests to verify this behavior. The reviewer suggests triggering the stream reader cancellation asynchronously without awaiting it, which prevents unnecessary delays in aborting the request and throwing the size-limit error.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if (done) break; | ||
| total += value.byteLength; | ||
| if (total > maxSize) { | ||
| await reader.cancel().catch(() => undefined); |
There was a problem hiding this comment.
Awaiting reader.cancel() can unnecessarily delay throwing the error and aborting the request if the cancellation promise takes time to resolve. Since we want to fail fast and reject the promise immediately when maxSize is exceeded, we should trigger the cancellation asynchronously without awaiting it.
| await reader.cancel().catch(() => undefined); | |
| reader.cancel().catch(() => undefined); |
Cancel the response stream and abort the fetch signal as soon as incremental reads exceed maxSize so Chromium stops buffering the body. Adds regression tests for reader cancel and single-chunk overflow. Closes clawwork-ai#408 Signed-off-by: root <root@array-fleet-ops.array.local>
advancedresearcharray
force-pushed
the
fix/safefetch-stream-cancel-408
branch
from
8fd77b5 to
437c599
Compare
|
Thanks @advancedresearcharray! /approve |
clawwork-ai#408) The streaming maxSize enforcement and reader cancellation were merged in clawwork-ai#487 and clawwork-ai#523, but issue clawwork-ai#408 remained open. Add a regression test that verifies safeFetch stops reading an oversized body before draining all chunks, preventing the memory DoS described in the issue. Closes clawwork-ai#408 Signed-off-by: Array Agent <advancedresearcharray@array.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Closes clawwork-ai#408 The streaming maxSize enforcement (clawwork-ai#487), reader cancellation (clawwork-ai#523), and regression tests (clawwork-ai#534) are already on main. This commit closes the issue that remained open after squash merges omitted the closing keyword. Signed-off-by: advancedresearcharray <advancedresearcharray@github.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Closes clawwork-ai#408 The streaming maxSize enforcement (clawwork-ai#487), reader cancellation (clawwork-ai#523), and regression tests (clawwork-ai#534) are already on main. This commit closes the issue that remained open after squash merges omitted the closing keyword. Signed-off-by: advancedresearcharray <advancedresearcharray@github.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Closes clawwork-ai#408 The streaming maxSize enforcement (clawwork-ai#487), reader cancellation (clawwork-ai#523), and regression tests (clawwork-ai#534) are already on main. This commit closes the issue that remained open after squash merges omitted the closing keyword. Signed-off-by: advancedresearcharray <advancedresearcharray@github.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Verified reader.cancel() + controller.abort() on streamed bytes exceeding maxSize, IPv6 pinned hostname bracketing, and 25 regression tests passing. Fix already landed on main via clawwork-ai#487, clawwork-ai#523, clawwork-ai#534; this commit closes clawwork-ai#408. Closes clawwork-ai#408 Co-authored-by: Cursor <cursoragent@cursor.com>
2 participants
Summary
safeFetchbuffered the entire response viares.arrayBuffer()before enforcingmaxSize, enabling a memory DoS. That was addressed in fix(net): stream safeFetch body with incremental maxSize enforcement #487 by streaming with incremental size checks.maxSize, so Chromium stops buffering the body instead of relying on abort alone.Closes #408
Test plan
pnpm checkpassesrelease-note
NONE