fix(http): bound the size of buffered JSON responses - #281
Conversation
response.json() buffers the entire body before parsing with no upper bound, so a large test result --history page or a run with a long steps[] array grows the heap in proportion to the payload. Read the typed JSON response bounded by a configurable cap (HttpClientOptions.maxResponseBytes, default 64 MiB): reject an over-cap Content-Length up front, count bytes while streaming chunked bodies, and throw a typed PAYLOAD_TOO_LARGE (exit 5) with guidance to narrow --page-size / --since.
|
✅ This PR is linked to an issue assigned to @Yazan-O — thanks! The |
github-actions
Bot
added
the
needs-issue
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Walkthrough
ChangesBounded HTTP response handling
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant HttpClient
participant Response
participant readBoundedText
participant JSON.parse
HttpClient->>Response: receive successful response
HttpClient->>readBoundedText: read with maxResponseBytes
readBoundedText->>Response: stream and count body bytes
readBoundedText-->>HttpClient: bounded text or PAYLOAD_TOO_LARGE
HttpClient->>JSON.parse: parse bounded text
JSON.parse-->>HttpClient: parsed JSON or malformed response error
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
removed
the
needs-issue
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/lib/http.ts (2)
971-979: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winNull-body fallback fully buffers before enforcing the cap.
When
response.bodyisnull,readBoundedTextcallsresponse.text()(fully materializing the body) and only checks its encoded length afterward — this defeats the memory-bounding goal this PR sets out to achieve for exactly the pathological/hostile-payload case it targets. It's only reachable whenContent-Lengthis absent AND no stream is exposed, but real fetch implementations rarely hit this combination for non-empty bodies, so it's a residual gap rather than the common path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/http.ts` around lines 971 - 979, Update readBoundedText’s stream === null fallback to avoid calling response.text() before enforcing maxBytes; use an incremental or otherwise bounded read mechanism that rejects once the encoded payload exceeds maxBytes while preserving the existing responseTooLargeError(requestId, maxBytes) behavior and returning the text for bodies within the cap.
989-992: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueUnguarded
reader.cancel()can mask the PAYLOAD_TOO_LARGE error.If
reader.cancel()rejects, thethrow responseTooLargeError(...)on the next line never executes; the cancel rejection propagates instead, falls through theerr instanceof ApiErrorcheck inrequestWithMeta, and gets misclassified viamalformedResponseError— silently breaking the exit-code-5 contract for this (admittedly rare) case.🛡️ Defensive fix
if (total > maxBytes) { - await reader.cancel(); + await reader.cancel().catch(() => {}); throw responseTooLargeError(requestId, maxBytes, total); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/http.ts` around lines 989 - 992, Guard the await reader.cancel() call in the response-size handling branch so any cancellation rejection is contained, then always throw responseTooLargeError(requestId, maxBytes, total). Preserve the existing oversized-payload behavior and ensure cancellation failures cannot replace the intended ApiError.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/lib/http.ts`:
- Around line 971-979: Update readBoundedText’s stream === null fallback to
avoid calling response.text() before enforcing maxBytes; use an incremental or
otherwise bounded read mechanism that rejects once the encoded payload exceeds
maxBytes while preserving the existing responseTooLargeError(requestId,
maxBytes) behavior and returning the text for bodies within the cap.
- Around line 989-992: Guard the await reader.cancel() call in the response-size
handling branch so any cancellation rejection is contained, then always throw
responseTooLargeError(requestId, maxBytes, total). Preserve the existing
oversized-payload behavior and ensure cancellation failures cannot replace the
intended ApiError.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 1f68da18-a6af-471f-9751-9298a900ac54
📒 Files selected for processing (2)
src/lib/http.test.tssrc/lib/http.ts
Resolves the two conflicts in src/lib/http.ts: - Constants block: keep both MAX_RESPONSE_BYTES_DEFAULT (this branch) and MAX_SCHEMA_ISSUES_IN_DETAILS (upstream TestSprite#264/TestSprite#273 line). - Success-path body read: keep the bounded read, but assign the parsed value to `raw` instead of returning early, so upstream's valibot `v.safeParse(options.schema, raw)` still runs. Returning here would have made the new schema validation dead code on every request. Verified: tsc --noEmit clean, eslint clean, vitest 58 files / 2030 passed / 3 skipped.
There was a problem hiding this comment.
Pull request overview
This PR adds a bounded read path for successful JSON responses in HttpClient.requestWithMeta() to prevent unbounded heap growth when large responses (history/steps) are returned, addressing issue #83.
Changes:
- Introduces
HttpClientOptions.maxResponseBytes(default 64 MiB) and enforces it for successful JSON responses. - Implements a streaming, byte-counted read (
readBoundedText) that rejects oversize responses early (viaContent-Length) or mid-stream (chunked). - Adds/updates HTTP client tests to cover oversize responses and timeouts during body reads.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/lib/http.ts | Replaces response.json() on the success path with a bounded read + JSON.parse, adds new size-cap option and helpers. |
| src/lib/http.test.ts | Adds tests for response-size capping and adjusts timeout-during-body-read coverage for the new read path. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (total > maxBytes) { | ||
| await reader.cancel(); | ||
| throw responseTooLargeError(requestId, maxBytes, total); | ||
| } |
| const limitMiB = Math.round(maxBytes / (1024 * 1024)); | ||
| return new ApiError({ | ||
| code: 'PAYLOAD_TOO_LARGE', | ||
| message: | ||
| `The server response exceeded the client-side ${limitMiB} MiB limit and was not ` + | ||
| `buffered, to avoid unbounded memory use.`, |
Closes #83
What
HttpClient.request()reads every successful response withawait response.json(), which buffers the entire body into memory before parsing, with no upper bound. A largetest result --historypage, or a run with a longsteps[]array (getRunwithincludeSteps), grows the heap in proportion to the payload.This adds a bounded read at that single choke point:
Content-Lengthover the cap is rejected before the body is read.Content-Length) are bounded by counting bytes as they stream, stopping once the cap is crossed.PAYLOAD_TOO_LARGEerror (exit 5 — the same code the backend already returns for oversized request bodies), with anextActionpointing at--page-size/--since.The cap is a new
HttpClientOptions.maxResponseBytes(default 64 MiB — far above any legitimate metadata/history/steps response), injectable for tests and future env/flag wiring.Scope
Focused on the unbounded typed-JSON read, which is the root cause described in #83. Pagination page-count bounding is handled separately (#248), so this does not touch
pagination.ts. Error-response bodies (small) keep using the existingsafeReadJsonpath. Happy to widen the scope if you'd prefer.The default cap value and whether to make it env-configurable are easy to adjust to your preference.
Tests (
src/lib/http.test.ts)Content-Length.Content-Length.RequestTimeoutError.npm run typecheck,npm run lint,npm run format:check, andsrc/lib/http.test.tsall pass locally.Summary by CodeRabbit
New Features
maxResponseBytes).PAYLOAD_TOO_LARGE) error including request details.Bug Fixes