Honor Blob slice offsets on non-seekable sources - #33510
Conversation
|
Warning Review limit reached
Next review available in: 22 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
Comment |
|
Updated 6:08 PM PT - Jul 6th, 2026
❌ @robobun, your commit 8380e2c has some failures in 🧪 To try this PR locally: bunx bun-pr 33510That installs a local version of the PR into your bun-33510 --bun |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Thanks — c2d053e addresses the readiness re-check I flagged in read_blocking_pipe, and the new trickle/close-at-offset tests cover it. I'm deferring to a human because this threads new state through four separate read loops (POSIX/Windows × buffered/one-shot) with event-loop-blocking implications, and the Windows paths are only type-checked.
Extended reasoning...
Overview
Fixes Bun.stdin.slice(start, end) ignoring start on non-seekable sources (pipes, sockets, ttys). Adds a skip_remaining counter to four read paths — PosixBufferedReader, WindowsBufferedReader, ReadFile (POSIX), ReadFileUV (Windows) — that drains and discards the leading bytes when lseek/pread cannot apply the offset. Adds a drain_skipped helper for the POSIX buffered reader with per-iteration readiness polling, and post-read head-dropping for the Windows paths. ~200 lines of native code changed plus 10 tests (8 new, 1 assertion corrected).
Since my last review, commit c2d053e added the is_readable re-check after drain_skipped returns true in read_blocking_pipe (exactly the fix suggested), plus two new tests exercising the offset-boundary and close-at-offset cases.
Security risks
None identified. No untrusted-input parsing, no auth/crypto/permissions surface. The change reads and discards bytes from an fd the caller already owns.
Level of scrutiny
High. PipeReader.rs is core I/O infrastructure shared by subprocess pipes, shell, stdin, and file streaming. The new drain_skipped path interacts with blocking-vs-nonblocking reads, poll re-arming, and event-loop liveness — the exact area where my previous review found a bug (now fixed). The Windows on_read change reslices the libuv buffer inside uncommitted spare capacity and early-returns before the debug-assertion pointer-range check and commit_spare; that looks correct but is subtle. Both Windows paths (WindowsBufferedReader::on_read, ReadFileUV::on_read) were verified only by rust:check-all, not by running tests.
Other factors
- Test coverage is thorough for POSIX: covers
.text(),.bytes(),.stream(),Response, unbounded slices, multi-read offsets, offset-past-EOF, trickle writer, close-at-offset, and a regular-file guard against double-consumption. - The
ReadFileUVchange toremaining_buffer()usessaturating_add(skip_remaining)on a value derived frommax_length - read_off; whenmax_length == MAX_SIZEandskip_remaining > 0this saturates rather than overflowing, which is the intent. - The corrected assertion in the existing overflow test (
"hello world"→"ello world") is a genuine behavior change — the old test encoded the bug. - No new bugs found by the current bug-hunting pass.
|
Thanks for the re-review. Two notes on the reservations, for whoever picks this up. The Windows paths are executed by this build, not only type-checked. The two pre-existing tests in One case no lane can turn red on. The readiness re-check only changes observable behavior where And yes, the |
Bun.stdin.slice(2, 7).text() returned bytes [0, 5) when stdin was a pipe: the blob's start offset was applied with lseek, which fails with ESPIPE on a pipe, socket or tty, and the error was swallowed. The end bound survived as a read length, so start was silently dropped. The readers now consume and discard the offset from the stream when the underlying fd cannot seek: - ReadFile (POSIX) skips into a scratch buffer when lseek fails. - ReadFileUV (Windows) drops the skipped head after each libuv read, since uv_fs_read only honors an offset on seekable handles. - BufferedReader (both platforms) does the same for the streaming path, where start_file_offset() previously only armed pread, which pipes ignore. Seekable sources keep using lseek/pread, so concurrent slices of the same file are unaffected.
read_blocking_pipe is only entered once its caller has confirmed the fd is readable, which is what makes its first read safe to block in. Draining the start offset spends that readiness, so the read that follows could block the event loop on an empty pipe: sys::read_nonblocking is a plain read(2) on macOS and on Linux once RWF_NOWAIT is disabled, and FileType::Pipe fds are blocking. A writer that emits exactly the offset and then pauses, without closing, was enough to trigger it.
c2d053e to
461e11e
Compare
CI statusThe diff is green on every lane that ran it. The remaining red on build #69270 is infrastructure, not this change. The one failure is an artifact-download timeout on a macOS shard — the A re-run of that single job should clear it. What did run, passed — including the paths that earlier review noted were only type-checked:
The two prior reds on this PR were also unrelated infra: a stale-base I've used my automated re-roll for this round, so I'm leaving the macOS job for a maintainer to re-run rather than pushing another no-op commit. |
What
Bun.stdin.slice(start, end)ignoresstartwhen stdin is a pipe.endsurvives, but as a length measured from 0:Every read method is affected:
.text(),.bytes(),.arrayBuffer(),.json(),.stream(), andnew Response(blob). Consumers silently get the wrong byte range with no error. Redirecting a regular file into stdin (bun child.js < file) is correct, so the bug only shows up for pipes, sockets and ttys.Why
A blob's start offset is applied by seeking.
lseekfails withESPIPEon anything that is not seekable, and the failure was discarded (src/runtime/webcore/blob/read_file.rs):So the read started at byte 0 while
max_length(end - start) still capped it, which is exactly the observed[0, end - start)window. The streaming path has the same hole one layer down:BufferedReader::start_file_offsetonly armspread, andpreadis never used for a pipe, socket or tty.How
When the source cannot seek, the offset is read out of the stream and discarded before any byte is kept. Seekable sources keep using
lseek/pread, so concurrent slices of the same file are untouched.ReadFile(POSIX): whenset_file_offsetfails,skip_remainingis armed and the read loop drains it into its stack buffer. The existing poll/EAGAINre-arm already covers an offset that straddles several reads.ReadFileUV(Windows):uv_fs_readonly honors an offset on seekable handles, so for a non-regular file the skipped head is dropped from the buffer after each read andread_offadvances only by the kept bytes.BufferedReader(both platforms) for.stream():start_file_offsetarms_skip_remainingwhen the source is not a file. POSIX drains it before the normal read loop runs; Windows shifts the kept tail down inside the uncommitted spare capacity, so the skipped bytes are never committed andFileReadernever sees them.Readiness is the subtle part of the POSIX drain.
sys::read_nonblockingis a plain blockingreadon macOS (and on Linux onceRWF_NOWAITis disabled), andFileType::Pipefds are blocking, so the drain polls before every read — the same hazardread_blocking_pipedocuments a few lines above. It then re-confirms readiness once more before handing control to the normal read loop, becauseread_blocking_pipeis only entered after its caller has proven the fd readable, and the drain has just spent that proof. Without it, a writer that emits exactly the offset and then pauses leaves the next read blocking on an empty pipe (thanks @claude for catching this one).One existing assertion in
test/js/bun/util/bun-stdin-slice.test.tsencoded the bug:Bun.stdin.slice(1)on"hello world"expected"hello world", and now expects"ello world".Verification
bun bd test test/js/bun/util/bun-stdin-slice.test.ts— 12 pass; 10 of the 12 fail onmain. They cover a piped stdin through.text(),.bytes(),.stream()andResponse, unboundedslice(start), an offset spanning many reads, an offset past EOF, a writer that trickles bytes so the offset straddles poll re-arms, and a writer that closes with the offset exactly consumed. The regular-file case guards against the offset being consumed twice.The two boundary tests cannot distinguish a blocking read on Linux, where
RWF_NOWAITturns it intoEAGAIN— they hang rather than fail on a macOS regression. Proving that one deterministically needs instrumentation insrc/.Also checked by hand
FileType::Pipe, the blocking-read path) for both.text()and.stream().setIntervalin the reader keeps ticking, i.e. the event loop is not blocked.spawn,streams,bun-write,blob,bun-serve-file,filesink: 459 pass, 0 new failures. (streams-leak.test.tsfails on an ASAN RSS ceiling both with and without this diff — 755 MB onmain, 700 MB here.)bun run rust:check-allclean across all 10 targets, which is what type-checks the Windows branches.Touches
src/io/PipeReader.rsalongside #31680, which fixes an unrelated hang in the same file; the two changes are in different functions.