Repository navigation
feat: answer multiple ranges with a multipart/byteranges body - #2496
alexander-akait wants to merge 6 commits into
Conversation
A Range header with more than one range (after range-parser merges the ones that overlap or touch) now gets 206 Partial Content with a multipart/byteranges body (RFC 9110, section 14.6) instead of the whole file. Each range is streamed from the output file system in turn, or cut from the buffer a strong ETag already read. Closes #1229 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UjuMAuk9o6UazjHzcAQCTA
🦋 Changeset detectedLatest commit: 6152bd1 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the
Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughThe middleware now handles multiple byte ranges with a Priority: ➖ Normal Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Description Check |  |
Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | |
The title clearly and concisely describes the main change: support for multiple range requests with a multipart/byteranges response body. |
| Linked Issues check | |
Issue #1229 requests multipart range support. The reviewed-head evidence shows 206 Partial Content with a multipart/byteranges body for multiple ranges. The implementation merges overlapping or to… |
| Out of Scope Changes check | |
The changes remain connected to issue #1229. Middleware and utility changes implement multipart responses. Tests verify the feature and stream error behavior. README and type updates document the mult… |
| Docstring Coverage | |
Docstring coverage is 81.82% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 11 functions across 6 files. (1 skipped: 1 … |
Full details: Linked Issues check
-
Explanation
Issue
#1229requests multipart range support. The reviewed-head evidence shows206 Partial Contentwith amultipart/byterangesbody for multiple ranges. The implementation merges overlapping or touching ranges and preserves the first requested range order. It streams one range at a time when possible and uses buffered parts when required. It also handlesHEAD, exactContent-Length, staleIf-Range, strongETag, filesystems withoutcreateReadStream, and the 200-range fallback. Tests cover these behaviors across the supported frameworks.
Full details: Out of Scope Changes check
-
Explanation
The changes remain connected to issue
#1229. Middleware and utility changes implement multipart responses. Tests verify the feature and stream error behavior. README and type updates document the multipart stream contract. The changeset and spelling entry support the same feature. No unrelated change is demonstrated by the supplied evidence.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 81.82% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 11 functions across 6 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches
-
📝 Generate docstrings
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Warning
Some tools did not complete. Review the errors below.
🔧 ast-grep (0.45.3)
-
test/middleware.test.js
-
ast-grep timed out on this file
-
🔧 ESLint
-
If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.
test/middleware.test.js
-
Oops! Something went wrong! :(
ESLint: 9.39.5
Error: Error while loading rule 'jest/no-deprecated-functions': Unable to detect Jest version - please ensure jest package is installed, or otherwise set version explicitly
Occurred while linting /test/middleware.test.js
at detectJestVersion (/.eslint-tmp/node_modules/eslint-plugin-jest/lib/rules/utils/detectJestVersion.js:22:9)
at create (/.eslint-tmp/node_modules/eslint-plugin-jest/lib/rules/no-deprecated-functions.js:31:107)
at Object.create (/.eslint-tmp/node_modules/@typescript-eslint/utils/dist/eslint-utils/RuleCreator.js:39:20)
at createRuleListeners (/.eslint-tmp/node_modules/eslint/lib/linter/linter.js:1019:15)
at /.eslint-tmp/node_modules/eslint/lib/linter/linter.js:1151:7
at Array.forEach ()
at runRules (/.eslint-tmp/node_modules/eslint/lib/linter/linter.js:1085:31)
at#flatVerifyWithoutProcessors(/.eslint-tmp/node_modules/eslint/lib/linter/linter.js:2115:4)
at Linter._verifyWithFlatConfigArrayAndWithoutProcessors (/.eslint-tmp/node_modules/eslint/lib/linter/linter.js:2203:43)
at Linter._verifyWithFlatConfigArray (/.eslint-tmp/node_modules/eslint/lib/linter/linter.js:2306:15)
-
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 @coderabbitai help to get the list of available commands.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
- Source mode: Auto
- Repository: Unavailable
- Branch: Unavailable
- File: No configuration file from this repository applied
- Commit: Unavailable
- Layers: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
e1ce5dd9-2773-47d0-89c6-6bff9218b109
📒 Files selected for processing: 7
.changeset/multipart-range-requests.md.cspell.jsonREADME.mdsrc/middleware.jssrc/utils.jstest/middleware.test.jstypes/utils.d.ts
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 7 remain after this review.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2496 +/- ##
==========================================
+ Coverage 96.61% 96.89% +0.28%
==========================================
Files 25 25
Lines 2657 2704 +47
==========================================
+ Hits 2567 2620 +53
+ Misses 90 84 -6 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
A multipart/byteranges body sends its first part before a later range is read; when that read fails, there is no error page left to send. sendError now destroys a response whose headers are out, and the stream error handler destroys it when sending the page throws (as on fastify, which does not report the headers as sent) instead of leaving the rejection unhandled and the client waiting. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UjuMAuk9o6UazjHzcAQCTA
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
🟡 Minor · Cap excessive multipart ranges. · middleware.js:1072
src/middleware.js:1072
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winCap excessive multipart ranges.
createMultipartByterangesopens ranges sequentially, so the request does not create one active file stream per range. However,range-parserdoes not limit range count, and each accepted range still adds filesystem-open and multipart-header overhead. A bounded request header can contain many disjoint single-byte ranges.Add a range-count cap before creating the multipart body. For requests above the cap, use the existing regular-response path or reject the request. This is a minor availability risk for development middleware, not a major issue.
ℹ️ Review info
⚙️ Run configuration
- Source mode: Auto
- Repository: Unavailable
- Branch: Unavailable
- File: No configuration file from this repository applied
- Commit: Unavailable
- Layers: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
3fc5de2c-aa23-468d-8318-ddb5cf53a195
📒 Files selected for processing: 2
src/middleware.jstest/middleware.test.js
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 6 remain after this review.
Also drops the clean-up of a stream left by a strong ETag: it buffers what it reads, so there is none. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UjuMAuk9o6UazjHzcAQCTA
|
@coderabbitai On the outside-diff Minor (cap the number of ranges): no change for now. The overhead is bounded by the request header size. Node's default is 16 KB, which allows roughly 2,000 one-byte ranges, so about 200 KB of part headers in one response. Parts are read one at a time, so only one file stream is open at once. A cap such as Apache's Generated by Claude Code |
|
Tip For best results, initiate chat on the files or code changes. @alexander-akait Understood. No range-count cap is needed for this PR in response to my minor suggestion. It can remain an optional follow-up. Would you like me to open a follow-up issue to consider a cap and full You are interacting with an AI system. |
As Apache's MaxRanges default: each range costs a read and a part header, so a request for more is answered with the regular response instead of a multipart body. Also documents the test helpers' callbacks. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UjuMAuk9o6UazjHzcAQCTA
|
@coderabbitai The maintainer asked for the remaining items to be resolved, so 96ea65e adds:
No follow-up issue is needed. Generated by Claude Code |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
🟡 Minor · Make the multipart callback input match the declared ReadStream type. · middleware.js:1165
src/middleware.js:1165
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winMake the multipart callback input match the declared
ReadStreamtype.For a multipart response,
src/middleware.jspasses a plainReadabletomodifyResponseData. The cast at line 1165 presents that value asBuffer | ReadStream, which hides the mismatch from the type checker. A callback that readsReadStream-only fields, such as.path, can receiveundefinedon this path.Do not fix this with the cast. Removing the cast alone causes a type error at this call. Either widen the declared callback
datatype to includeReadable, or stop passing a plainReadableto this callback. Widening changes the public callback input type for typed consumers, so make that choice deliberately.
ℹ️ Review info
⚙️ Run configuration
- Source mode: Auto
- Repository: Unavailable
- Branch: Unavailable
- File: No configuration file from this repository applied
- Commit: Unavailable
- Layers: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
396124b8-28c6-43ae-bfd3-ea3d98b00603
📒 Files selected for processing: 3
.changeset/multipart-range-requests.mdsrc/middleware.jstest/middleware.test.js
🚧 Files skipped from review as they are similar to previous changes: 1
- .changeset/multipart-range-requests.md
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 7 remain after this review.
|
Tip For best results, initiate chat on the files or code changes. @alexander-akait I couldn't produce a complete response to this request. Please try again. You are interacting with an AI system. |
A multipart/byteranges body is a plain Readable, not a ReadStream, so the callback's data, and the data it returns, are now typed Buffer | Readable instead of being cast to Buffer | ReadStream. A ReadStream is still one. Also replaces two inline callbacks in the new tests with loops. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UjuMAuk9o6UazjHzcAQCTA
|
@coderabbitai Addressed the outside-diff Minor on A Also replaced the two remaining inline callbacks in the new tests with loops, for the docstring coverage warning. Generated by Claude Code |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
🟡 Minor · Count requested ranges before combining them. · middleware.js:1077
src/middleware.js:1077
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCount requested ranges before combining them.
parsedRanges.lengthcounts merged ranges. For example, 201 identical requested ranges become one parsed range. This branch then sends a single-range206response instead of the promised full-file200response. Check the range count before parsing withcombine: true, and retain the existing merge for accepted requests. (raw.githubusercontent.com)
🟡 Minor · Preserve request order when a range is contained. · middleware.js:1100
src/middleware.js:1100
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPreserve request order when a range is contained.
For
bytes=100-200,400-500,0-300,range-parserv1.3.0 merges the first and third ranges but assigns the merged range the third range’s index. The multipart response therefore puts400-500before0-300, although the merged range includes the first requested range. Order the merged ranges by their earliest matching requested range, and add this case to the multipart tests. (raw.githubusercontent.com)
ℹ️ Review info
⚙️ Run configuration
- Source mode: Auto
- Repository: Unavailable
- Branch: Unavailable
- File: No configuration file from this repository applied
- Commit: Unavailable
- Layers: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
93e09870-4feb-48fc-949a-b10875827d57
📒 Files selected for processing: 6
.changeset/multipart-range-requests.mdREADME.mdsrc/index.jssrc/middleware.jstest/middleware.test.jstypes/index.d.ts
🚧 Files skipped from review as they are similar to previous changes: 1
- README.md
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 6 remain after this review.
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
git show --stat afe44d1
ast-grep outline src/index.js --match 'ResponseData|ModifyResponseData|Readable'
rg -n -C 4 'Buffer \| Readable|multipart|ReadStream|ModifyResponseData' src/index.js types/index.d.ts README.md .changeset/multipart-range-requests.md
git show afe44d1 -- test/middleware.test.js test/validation-options.test.jsLength of output: 13573
You are interacting with an AI system. |
The 200-range cap now counts the ranges the header lists, not the ones left after merging, and is applied when the header is read, so a strong ETag never reads only a range that then goes out as the whole file. Merged ranges are ordered by the first requested range each one holds: range-parser gives a merged range the place of the last one merged in. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UjuMAuk9o6UazjHzcAQCTA
|
@coderabbitai Both outside-diff Minors are fixed in 6152bd1. I reproduced both with
All three new tests fail on the previous commit and pass now. Generated by Claude Code |
|
@coderabbitai review Generated by Claude Code |
✅ Action performedReview finished.
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
git show --format=fuller --stat 6152bd1
ast-grep outline src/middleware.js --match 'getRangeHeader|parseRange' --view expanded
ast-grep outline test/middleware.test.js --match 'range|Range'
ast-grep outline test/validation-options.test.js --type function
git show 6152bd1 -- src/middleware.js test/middleware.test.js test/validation-options.test.js
rg -n -C 8 'MAX_RANGES|toSorted|getRangeHeader|combine: false|201|100-200,400-500,0-300' src/middleware.js test/middleware.test.jsLength of output: 18554
You are interacting with an AI system. |
Closes #1229.
A
Rangeheader that asks for more than one range (bytes=0-99,5000-5099) used to get the whole file with200and an error in the log. It now gets206 Partial Contentwith amultipart/byterangesbody, as RFC 9110 §14.6 describes:Details:
range-parser(combine: true, as before), sobytes=0-499, 499-800is still a single-range206. Parts keep the order they were asked in.Readableover an async generator that opens one range's stream at a time. Asking for a few slices of a large asset (the use case in the issue) reads only those slices.ETagalready read the whole file, or when the file system has nocreateReadStream.Content-Lengthis exact.HEADgets the headers only. A staleIf-Rangestill gets the full200.Content-Typeis removed before it is set: on Hono, setting a header adds a value, which the tests caught.modifyResponseDatareceives the multipart body as a stream; the README note says so.Tests, in
test/middleware.test.jsand run on every framework:GETwith two ranges in reverse order: both parts, their headers and bodies, the exactContent-Length, no top-levelContent-Range.HEADwith two ranges.ETag: parts cut from the read file. A staleIf-Rangefalls back to200.createReadStream: parts cut fromreadFileSync.200fallback, now asserts the206.Results:
test/middleware.test.jspasses 6681/6681. The full unit suite has 7424 passed. The 2 failures are thewriteToDiskEACCES cases, which fail onmaintoo when run as root.npm run lintpasses, including types and spelling (byterangesadded to.cspell.json).A minor changeset is included. This isn't breaking: a client that sends several ranges has to accept a multipart answer under the spec, and the full
200was only ever a fallback.Generated by Claude Code
Summary by CodeRabbit
206 Partial Contentresponse with amultipart/byterangesbody. Overlapping or adjacent ranges are combined, and parts follow request order with content type and range details.If-Rangecondition continues to return the full response.