s3: emit HTTP suffix/open-ended Range for S3File.slice(-n) and slice(a) - #33687
s3: emit HTTP suffix/open-ended Range for S3File.slice(-n) and slice(a)#33687robobun wants to merge 3 commits into
Conversation
|
Updated 3:08 PM PT - Jul 9th, 2026
❌ @robobun, your commit 3c70520 has 1 failures in 🧪 To try this PR locally: bunx bun-pr 33687That installs a local version of the PR into your bun-33687 --bun |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughBlob slicing now uses absolute offset and size values, with special handling for S3 blobs of unknown size. S3 range header construction is centralized in a shared helper, and a new test suite exercises slice behavior against a mock S3 endpoint. ChangesBlob Slicing and S3 range handling
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@src/runtime/webcore/Blob.rs`:
- Around line 2112-2122: The S3 slice handling in Blob::slice is treating a
positive start at the exact MAX_SIZE boundary as the suffix sentinel, which can
cause an empty boundary slice to download the full object. Update the
unresolved-S3 branch to clamp via relative_start, and explicitly short-circuit
the case where relative_start equals this_size before assigning the sentinel
offset. Keep the offset/size sentinel logic in sync with the self.is_s3(),
start_raw, and end_raw path so exact-limit boundaries succeed and one-past-limit
still fails.
- Line 4959: The Blob cloning path is creating an empty slice because
get_slice_from now uses its size argument directly, so the source window length
is being lost. Update the get_slice_from call in the Blob source-window cloning
logic to pass the source blob’s current size instead of 0, preserving the
original slice length when constructing the new view.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8fb1b304-766a-4942-a27a-2a65a274be17
📒 Files selected for processing (3)
src/runtime/webcore/Blob.rssrc/runtime/webcore/s3/client.rstest/js/bun/s3/s3-slice-range.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@src/runtime/webcore/Blob.rs`:
- Around line 2107-2153: The wildcard branch in Blob::slice range handling can
collide with the MAX_SIZE suffix sentinel and return the wrong bytes for chained
slices like slice(F).slice(-F). Update the matching logic around the
default_offset/span fallback in Blob.rs to clamp or otherwise guard against
producing offset == MAX_SIZE for S3 unresolved-size blobs, similar to the
existing Some(s) if s >= 0 arm. Also add a regression test in the S3 slice range
tests for slice(700).slice(-700) that asserts InvalidRange rather than a
successful response.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5583135d-0790-4d22-9a26-9caca4fc6560
📒 Files selected for processing (3)
src/runtime/webcore/Blob.rssrc/runtime/webcore/s3/client.rstest/js/bun/s3/s3-slice-range.test.ts
|
CI status after rebase onto main (3bef55f + autofix b6fa2d4): the diff is green on every lane that ran
Neither touches Blob/S3. Locally: 46 tests pass with the fix, 20 fail without ( |
S3File.slice(-n) computed the negative offset against the unresolved MAX_SIZE sentinel (2^52-1) and sent Range: bytes=4503599627370490-..., which every real S3 endpoint rejects with 416 InvalidRange. slice(a) with no end sent bytes=a-4503599627370494, working only because servers clamp the last-byte-pos. When slicing an S3 blob whose size is still the lazy sentinel and no end is given, encode slice(-n) as an HTTP suffix range (bytes=-n) and slice(a) as an open-ended range (bytes=a-). The suffix form is stored as offset=MAX_SIZE, size=n and decoded by a shared range_header helper used by both download_slice and download_stream. The suffix encoding is restricted to a top-level file (self.offset == 0, n < MAX_SIZE) so chained slices and -Infinity fall through to the generic arithmetic, and default_offset is capped below MAX_SIZE so ordinary arithmetic can never land exactly on the sentinel. Re-slicing a suffix parent that still ends at the parent's end is encoded as a shorter suffix; one that stops short or is empty is pushed past the sentinel so the server 416s instead of returning wrong bytes. get_slice_from now takes the final (offset, size) pair directly instead of relative indices; its other caller in write_file_with_source_destination is updated to pass the same values the old arithmetic produced.
691a48a to
3bef55f
Compare
Repro
s3.file(k).slice(-n).bytes()is the Blob idiom for reading the tail of an object (footers, zip central directories). It always fails against AWS S3 / MinIO / R2 withInvalidRange.slice(a)with no end sendsbytes=a-4503599627370494, which only works because servers clamp the last-byte-pos.Cause
Blob::get_sliceapplies the W3C relative clamp againstself.size, which for a lazy S3 blob is theMAX_SIZEsentinel (2^52-1).slice(-5)becomesrelative_start = MAX_SIZE - 5, and that number is serialised straight into theRangeheader. The in-memoryBlobpath is correct because its size is always known.Fix
When slicing an S3 blob whose size is still the sentinel and no
endis given, encode the two forms HTTP already supports without knowing the size:slice(-n)storesoffset = MAX_SIZE, size = nand is serialised asbytes=-nslice(a)keepssize = MAX_SIZEand is serialised asbytes=a-The duplicated range-builder in
download_slice/download_streamis extracted into onerange_header()helper that handles all three forms.get_slice_fromnow takes the final(offset, size)pair so the caller owns the arithmetic.Bun.file()has the same sentinel leak (the sibling fix in #33601 resolves the size via stat); S3 cannot stat synchronously, so the encoding approach is used instead. Scoped tois_s3()so file-backed slice behaviour is unchanged here.Verification
The new test covers
.bytes()/.text()/.arrayBuffer()/.stream()against an RFC 7233 mock server and asserts both the exactRangeheader sent and the bytes returned.[review] gate passed · iteration 5 · 3 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 5
evidence per changed file