fix(desktop): stream safeFetch bodies with incremental maxSize enforcement (Closes #408)#550
Conversation
Read response bodies incrementally and abort once maxSize is exceeded instead of buffering the full payload with arrayBuffer() first. Closes clawwork-ai#408
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 optimizes the safeFetch utility to handle large response bodies more efficiently. By transitioning from a full-buffer approach to a streaming-based approach, the application can now enforce size constraints incrementally, preventing potential memory issues when processing large network responses. 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 refactors safeFetch to stream the response body instead of loading the entire array buffer at once, preventing excessively large responses from being loaded into memory. The review feedback correctly identifies a regression where responses without a body (such as a 204 No Content status) will now throw an error instead of returning an empty buffer, and suggests returning an empty buffer and adding a corresponding test case to verify this behavior.
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.
| const reader = res.body?.getReader(); | ||
| if (!reader) throw new Error('response has no body'); |
There was a problem hiding this comment.
Throwing an error when res.body is null/undefined introduces a regression. Previously, if the response had no body (e.g., a 204 No Content status or an empty response), res.arrayBuffer() would resolve to an empty buffer, and safeFetch would succeed by returning an empty Buffer.
To preserve this behavior and prevent unexpected failures on empty responses, we should return an empty Buffer when res.body is null or undefined.
| const reader = res.body?.getReader(); | |
| if (!reader) throw new Error('response has no body'); | |
| if (!res.body) { | |
| return Buffer.alloc(0); | |
| } | |
| const reader = res.body.getReader(); |
| it('rejects streaming bodies without content-length before exceeding maxSize', async () => { | ||
| assertNotPrivateHostMock.mockResolvedValue(null); | ||
| const big = new ArrayBuffer(4096); | ||
| netFetchMock.mockResolvedValue(mockResponse(big, 200, {})); | ||
|
|
||
| await expect(safeFetch('https://cdn.example.com/stream.bin', { maxSize: 1024 })).rejects.toThrow('too large'); | ||
| }); |
There was a problem hiding this comment.
Add a test case to verify that safeFetch successfully returns an empty buffer when the response has no body (e.g., a 204 No Content response), preventing future regressions.
it('rejects streaming bodies without content-length before exceeding maxSize', async () => {
assertNotPrivateHostMock.mockResolvedValue(null);
const big = new ArrayBuffer(4096);
netFetchMock.mockResolvedValue(mockResponse(big, 200, {}));
await expect(safeFetch('https://cdn.example.com/stream.bin', { maxSize: 1024 })).rejects.toThrow('too large');
});
it('returns empty buffer when response has no body (e.g. 204 No Content)', async () => {
assertNotPrivateHostMock.mockResolvedValue(null);
const response = mockResponse(new ArrayBuffer(0), 204);
Object.defineProperty(response, 'body', { value: null });
netFetchMock.mockResolvedValue(response);
const buf = await safeFetch('https://cdn.example.com/empty');
expect(buf.length).toBe(0);
});
|
PR is ready for merge! 🎉 All checks are green and there are no pending reviews. |
|
LGTM, ready for merge! 🚀 |
|
PR is ready for merge, CI is green and there are no pending reviews. |
|
This PR is ready for merge. CI is green and there are no review comments. |
1 participant
Closes #408
Summary
safeFetchnow reads response bodies withgetReader()and aborts oncemaxSizeis exceeded, instead of callingarrayBuffer()on the full payload first.Test plan
pnpm --filter @clawwork/desktop test -- safe-fetch.test.ts