Blob.writer(): force sync mode on stdout/stderr (POSIX) - #31180
Conversation
|
Updated 12:08 AM PT - Aug 16th, 2026
⏳ @robobun, your commit e3b2c7f is still building in
|
WalkthroughThis PR forces synchronous writes for non-Windows stdout/stderr FileSinks by detecting stdout/stderr via rare_data store pointer identity with a stdio_tag fallback, and adds an end-to-end subprocess test asserting write-order preservation and no cross-stream leakage. ChangesStdout/stderr synchronous writes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thanks for addressing the describe.concurrent nit. The POSIX detection logic and force_sync propagation look correct and mirror the Windows arm exactly, but since this changes stdout/stderr I/O semantics (buffered→sync) for every .writer() on POSIX, I'd like a maintainer to confirm before merge.
Extended reasoning...
Overview
This PR adds stdout/stderr detection to the POSIX arm of Blob::get_writer in src/runtime/webcore/Blob.rs (~32 lines) and sets force_sync on the resulting FileSink before setup(). It also adds 4 subprocess-based regression tests to test/js/bun/util/filesink.test.ts. The detection block (rare_data store-pointer comparison + fd.stdio_tag() fallback) is a near-verbatim copy of the existing Windows arm at Blob.rs:1878-1921, and the force_sync Cell is read by FileSink::setup at FileSink.rs:711 and propagated to writer.force_sync + O_NONBLOCK clearing via open_for_writing (FileSink.rs:726-729), so setting it pre-setup() is the right hook point.
Security risks
None identified. The change only toggles an internal buffering flag for stdout/stderr file descriptors; no new inputs are parsed, no auth/crypto/permissions surface is touched, and the unsafe { (*sink).force_sync.set(true) } deref follows the same sole-owner pattern as the surrounding code that already derefs the same freshly-allocated *mut FileSink.
Level of scrutiny
Moderate-to-high. The diff is small and mechanical (mirrors established code in the same function), and BunProcess.cpp:2632-2637 confirms forced-sync on stdout/stderr is the codebase-wide intent. However, this is a behavior change on a core runtime I/O path — every Bun.file(1/2).writer() and Bun.stdout/stderr.writer() on POSIX goes from buffered/deferred to synchronous writes. That's the correct fix for the reordering bug, but it has throughput implications for workloads that issue many small writes, and it's the kind of semantic shift a maintainer should sign off on rather than a bot.
Other factors
The bug-hunting system found no issues. My only prior comment (use describe.concurrent) was addressed in 7aaa95d and the thread is resolved. Test coverage is solid: 4 new tests exercise all four entry points (Bun.file(1), Bun.stdout, Bun.file(2), Bun.stderr) end-to-end via spawned subprocesses, and the PR description reports the broader stdio test suites pass unchanged. I'm confident the change is correct; I'm deferring solely because runtime I/O behavior changes warrant a human glance.
|
Status: draft, waiting on a direction decision from a maintainer.
|
There was a problem hiding this comment.
LGTM — brings the POSIX arm into parity with the Windows arm and process.stdout/stderr, with subprocess tests covering all four entry points.
Extended reasoning...
Overview
This PR adds a force_sync flag to the POSIX branch of Blob::get_writer in src/runtime/webcore/Blob.rs when the target is stdout/stderr (detected via rare_data store pointer identity or fd.stdio_tag()), and adds four subprocess regression tests in test/js/bun/util/filesink.test.ts. The detection block at lines 1943–1960 is a near-verbatim copy of the existing Windows arm at lines 1878–1895, and the effect (force_sync.set(true) before start()) matches what Bun__ForceFileSinkToBeSynchronousForProcessObjectStdio already does for process.stdout/process.stderr on POSIX. FileSink::setup (FileSink.rs:679–730) reads self.force_sync and propagates it to open_for_writing and writer.force_sync, so setting it before start() is the correct hook point.
Security risks
None. The change only toggles a boolean on a freshly-allocated FileSink to select the synchronous write path that already exists and is already used on Windows and for process.stdout/stderr. No new syscalls, no user-controlled input parsing, no auth/crypto/permissions surface. The unsafe { (*sink).force_sync.set(true) } deref targets a pointer returned two lines earlier by FileSink::init and follows the identical pattern used for (*sink).start() and (*sink).to_js() immediately below it.
Level of scrutiny
Low-to-medium. While Blob::get_writer is runtime-critical, this change is a parity fix: three of four code paths (Windows Blob.writer(), POSIX process.stdout, POSIX process.stderr) already force sync mode on stdout/stderr; only the POSIX Blob.writer() path was missing it. The diff introduces no new logic — it reuses the exact detection predicate from the Windows arm and sets a Cell<bool> that the existing setup() machinery already consumes. The behavioral change (sync instead of buffered writes to fd 1/2) matches Node.js semantics and what Bun already does everywhere else.
Other factors
The new tests spawn isolated subprocesses with piped stdout/stderr and assert byte-level ordering for each of Bun.file(1), Bun.stdout, Bun.file(2), Bun.stderr — directly exercising the bug from #11553. My earlier nit (use describe.concurrent) was applied in 7aaa95d and the inline comment is resolved. The bug-hunting system found no issues, CodeRabbit raised none, and the reported CI failures (HTTP3 LeakSanitizer, a flaky-tagged memory-threshold test) are unrelated to FileSink. No CODEOWNERS cover this path.
|
Re-checked against current main (f426a8e). The POSIX arm of #11553 was closed because its stdin half no longer reproduces; the write ordering half this PR addresses still does, so this stays open. The branch conflicts with main now and needs a rebase. |
On POSIX, Bun.file(1).writer() / Bun.stdout.writer() created a FileSink
whose PosixStreamingWriter buffered writes smaller than page_size until
a deferred auto-flush task ran. A large write through a second sink on
the same fd bypassed the buffer and went straight to the kernel, so
Bun.file(1).writer().write(Uint8Array.of(0,0,16,0)); // buffered
Bun.file(1).writer().write(oneMegabyte); // immediate
produced the 1 MiB body on stdout before the 4-byte header.
The Windows arm of get_writer already detects stdout/stderr (via the
rare_data singleton store or fd.stdio_tag()) and starts the sink in
sync mode; process.stdout does the same on POSIX via
Bun__ForceFileSinkToBeSynchronousForProcessObjectStdio. Apply the same
check to the POSIX arm of Blob.writer(): when the Blob is backed by fd
1/2 or the Bun.stdout/Bun.stderr store, set force_sync before start().
setup() then passes it to open_for_writing, which clears O_NONBLOCK on
the dup'd fd and propagates force_sync to the underlying writer so
every write goes straight to the fd.
Fixes the write-reordering half of #11553 (the stdin-read half no
longer reproduces on main).
d4da11b to
e3b2c7f
Compare
|
Rebased onto main (23d535a) and force-pushed; dropped the empty Rebasing surfaced a real problem, though, so I have converted this to a draft. Since this PR was opened, #38641 added tests that pin
Both pass on clean main; the 4 ordering tests from this PR still fail on clean main (the bug is still there), and still pass with the diff. So the fix works but is incompatible with the direction main has taken for stdio sinks (#33538, #35278, #35344, #35365, #36250, #38641 all build on the buffered path), and it would also make the correct single-writer pattern do a syscall per The PR description now lays out the three directions I can see (treat a never-flushed per-call |
1 participant
Problem
Two independent sinks on the same stdio fd write out of program order on POSIX:
The 64 KiB body reaches fd 1 first; the 4-byte header lands after it. Still reproduces on current main (verified at 23d535a).
Cause: each
.writer()call creates a freshFileSink.PosixStreamingWriter::writebuffers anything belowCHUNK_SIZEand leaves it for the deferred auto-flush task, while a write at or aboveCHUNK_SIZEgoes straight to the fd (src/io/PipeWriter.rs,should_buffer). Sink A's bytes sit in A's buffer while sink B's bytes hit the kernel.This is the write half of Bun.stdin.stream() and Bun.file(0).stream() consistently hang on 983036 bytes of 1048576 bytes expected #11553 (the Native Messaging host does exactly this, one
Bun.file(1).writer()per chunk). The stdin half of that issue no longer reproduces.Fix (as currently written)
Blob::get_writer(src/runtime/webcore/Blob.rs): when the Blob is fd 1/2 or theBun.stdout/Bun.stderrstore singleton, setforce_syncon the new sink beforestart(). This is the same predicate the Windows arm already uses to callstart_sync, and the same flagprocess.stdout/process.stderrset viaBun__ForceFileSinkToBeSynchronousForProcessObjectStdio.FileSink::setuppasses it toopen_for_writing, which clearsO_NONBLOCKon the dup'd fd and setswriter.force_sync, so everywrite()is an immediatewrite(2).test/js/bun/util/filesink.test.ts, "stdout/stderr sinks write synchronously" (4 cases:Bun.file(1),Bun.stdout,Bun.file(2),Bun.stderr). Fails on main, passes with the fix.Conflict with current main
#38641 added tests pinning
Bun.stdout.writer()on POSIX as buffered and non-blocking. With this diff applied on top of main, two of them break:force_syncclearsO_NONBLOCKon the shared open file description, sowrite()to a socket whose send buffer is full blocks the main thread, and the timer that drains it never runs. That is a real regression, not a test-shape issue.write()instead offlush().Both pass on clean main. So main has, since this PR was opened, committed to buffered/non-blocking semantics for stdio sinks on POSIX (#33538, #35278, #35344, #35365, #36250, #38641 all build on that path), and forcing sync mode cuts against it. It would also turn the correct usage (one cached writer, many small
write()calls) into one syscall per write.Options I see, none of which I think should be picked unilaterally:
.writer()that is never flushed as misuse.await Bun.write(Bun.stdout, x),process.stdout.write, and a single cached writer all produce correct output today. The remaining inconsistency would then be the Windows arm (sync) vs POSIX (buffered), plus docs (Document Bun.stdin.stream() and Bun.file("/dev/stdin").stream() are not the same, behave differently #11712 is the open docs issue from the same reporter).FileSinkper stdio fd across.writer()calls, so there is a single ordered buffer. Fixes ordering and keeps buffered/non-blocking semantics, but is a lifecycle/design change (end()/close()on one handle, per-callhighWaterMark, refcounting), not a bug fix.write(2)eagerly and only park on EAGAIN/partial. Fixes ordering and the backpressure test, but EPIPE would surface fromwrite()unless errors are deferred toflush(), and it adds a syscall perwrite()for the normal single-writer case.Background
FileSinkis the native object behindBun.file(...).writer(): a buffered writer over one fd.write()appends to an in-memory buffer; data reaches the fd onflush(),end(), when the buffer passes a size threshold, or from a deferred auto-flush task the event loop runs after the current macrotask.force_syncis aFileSink/PosixStreamingWriterflag that disables that buffering and makes the fd blocking. Bun already sets it forprocess.stdout/process.stderr(Node semantics: stdio writes are synchronous for files and, on Linux, pipes) and, on Windows, for any.writer()on fd 1/2.rare_data.stdout_store/stderr_storeare the singletonBlob.Stores behindBun.stdout/Bun.stderr; comparing store pointers is how both arms recognise those objects.fd.stdio_tag()coversBun.file(1)/Bun.file(2).Original PR description (before the rebase)
Repro
In the Native Messaging host from #11553 this surfaces as the 4-byte length prefix landing after the 1 MiB JSON payload, so the reader interprets the first 4 bytes of
[null,null,...as the message length (0x6c756e5b= 1819635291) and blows up onArrayBuffer.resize.Cause
Each
.writer()call allocates a freshFileSink. On POSIX itsPosixStreamingWriterbuffers writes belowpage_sizeand only flushes them from a deferred auto-flush task; writes ≥page_sizebypass the buffer and hit the fd immediately. So the 4-byte write sits in sink A's buffer while the 64 KiB write through sink B reaches the kernel first, then sink A's deferred flush appends the header at the end.The Windows arm of
Blob::get_writeralready detects stdout/stderr (via therare_datasingleton store orfd.stdio_tag()) and starts the sink withstart_sync.process.stdout/process.stderrget the same treatment on POSIX viaBun__ForceFileSinkToBeSynchronousForProcessObjectStdio. Only the POSIX arm ofBlob::get_writerwas missing it.Fix
Mirror the Windows check on POSIX: when the Blob is backed by fd 1/2 or by the
Bun.stdout/Bun.stderrstore singleton, setforce_syncon the newFileSinkbeforestart().FileSink::setuppropagates that toopen_for_writing, which clearsO_NONBLOCKon the dup'd fd and setswriter.force_sync, so every.write()goes straight to the fd in program order.The stdin-read side of #11553 (
Bun.stdin.stream()hanging at 983036 of 1048576 bytes) no longer reproduces on main —Bun.stdin.stream()delivers all bytes in order.Verification
New tests in
test/js/bun/util/filesink.test.tsspawn a subprocess that writes a 4-byte header and a 64 KiB body through two independent sinks on each ofBun.file(1),Bun.stdout,Bun.file(2),Bun.stderr, and assert the header arrives first.bun bdwithout fixbun bdwith fixBun.file(1).writer()Bun.stdout.writer()Bun.file(2).writer()Bun.stderr.writer()Full
filesink.test.ts(46 tests),process-stdio.test.ts,child-process-stdio.test.js, andconsole-log.test.tspass unchanged.Fixes #11553
no test proof · iteration 3 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/util/filesink.test.ts