Conversation
process.stdout and process.stderr route writes through a FileSink fast path that bypasses writeAll/writevAll, so this.bytesWritten was never incremented and stayed at 0. Node (net.Socket / tty.WriteStream) reports the encoded byte count synchronously after write(). Count bytes at each fast-path fileSink.write() site. Strings are measured with Buffer.byteLength so multi-byte UTF-8 is counted correctly. Fixes #23061. Co-authored-by: Steven Zimmerman <15812269+EffortlessSteven@users.noreply.github.com>
|
Warning Review limit reached
Next review available in: 2 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 (2)
Comment |
|
No additional issues found beyond those already referenced in the PR. 🤖 Generated with Claude Code |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
FileSink.write(string) has no encoding parameter and always emits UTF-8, so measure strings as UTF-8 regardless of the caller's encoding argument. Also add the increment to the prototype _write fast-path branch so all four fileSink.write sites are consistent.
|
Updated 2:26 AM PT - Jul 31st, 2026
❌ @robobun, your commit 14a770c has 1 failures in 🧪 To try this PR locally: bunx bun-pr 36526That installs a local version of the PR into your bun-36526 --bun |
There was a problem hiding this comment.
LGTM — both earlier notes are addressed (UTF-8 counting, fourth fileSink.write site covered) and the trimmed comment satisfies comment-cop.
What was reviewed:
- All four
fileSink.writefast-path sites now incrementbytesWritten; increment sits after the write call so a synchronous throw doesn't over-count. bytesForWritemeasures UTF-8 for strings (matching whatFileSink.writeactually emits) andbyteLengthfor buffers/views; null-safe fallback to 0.- Test spawns over pipes, drains stdout/stderr/exited concurrently, covers multi-byte UTF-8 + Buffer, and side-channels the counters via a temp file so stderr stays assertable — fails on main with
{0,0}.
Extended reasoning...
Overview
Fixes #23061: process.stdout/stderr.bytesWritten stayed at 0 because the stdio FileSink fast path bypasses writeAll/writevAll. The change adds a small bytesForWrite helper and increments this.bytesWritten at each of the four fileSink.write() call sites (writeFast, underscoreWriteFast, prototype _write fast-path branch, prototype _writev fast-path branch). One new subprocess test in process-stdio.test.ts.
Prior feedback
My previous review raised two points, both now resolved: (1) bytesForWrite originally honored the caller's encoding while FileSink.write always emits UTF-8 — the encoding param was dropped in 467f785 so counting matches what's actually written; (2) the prototype _write fast-path branch was the one un-updated sibling — it now has the increment. The comment-cop flag on the helper's explanatory comment was addressed by trimming it to one line in f338d81.
Security risks
None. This only reads a length off the chunk and adds it to a plain number field on the stream instance. No new inputs are parsed, no allocation sizes derived from it, no cross-boundary data.
Level of scrutiny
Low-to-moderate. It's a Node-compat counter fix in a built-in JS module — hot path but purely additive (+= on an existing numeric field). The increment is placed after the fileSink.write() call, so a synchronous throw won't over-count; if the write returns a rejected promise the counter is still bumped, but that matches Node's net.Socket semantics (bytesWritten reflects bytes handed to the socket, reported synchronously) and is strictly better than the previous always-0.
Other factors
The test follows harness conventions closely: tempDir, bunEnv/bunExe, concurrent pipe draining, exact-value assertions on both the emitted bytes and the counters, exit code asserted last. It writes the counter report to a temp file rather than stderr so the stderr assertion stays exact. The before: {0,0} assertion confirms the fast path was exercised from a clean initial state, and the multi-byte string + Buffer inputs exercise both branches of bytesForWrite. The _writev site uses buffer.length on the concatenated Buffer, which equals its byte length. No outstanding reviewer threads; all inline comments are resolved.
A FileSink write that fails (EPIPE) returns a rejected promise; counting before the promise settles over-reports bytesWritten relative to Node, whose net.Socket only adds to _bytesDispatched after a successful low-level write. Move the increment into the synchronous-success branch and the promise fulfillment handler so a rejected write is not counted. Also drop the ?? data?.length fallback from bytesForWrite: every input without a numeric byteLength either already took the string branch or is rejected by FileSink.write before the increment runs.
There was a problem hiding this comment.
LGTM — the follow-up commits addressed both earlier notes (UTF-8 byte counting, and the fourth fileSink.write site in prototype _write).
What was reviewed:
- All four
fileSink.write()sites now incrementbytesWrittenonly in the success arms; the non-fast-pathwriteAll/writevAllbranches are mutually exclusive, so no double-counting. bytesForWritemeasures UTF-8 to match whatFileSink.write(string)actually emits;.byteLengthcovers Buffer/TypedArray.- New tests cover multi-byte UTF-8 + Buffer chunks and the EPIPE not-counted path; the EPIPE test awaits the drain and stdin signal rather than sleeping.
Extended reasoning...
Overview
Fixes #23061: process.stdout.bytesWritten / process.stderr.bytesWritten stayed at 0 because the stdio fast path (writeFast / underscoreWriteFast) hands chunks directly to a FileSink and never went through writeAll, the only place that incremented the counter. The fix adds a tiny bytesForWrite(data) helper and bumps this.bytesWritten at each of the four fileSink.write() call sites in src/js/internal/fs/streams.ts, only in the fulfillment / synchronous-success arms so a rejected write (EPIPE) is not counted. Two new spawned-child tests in test/js/node/process/process-stdio.test.ts cover the counter for UTF-8 strings + Buffers and the EPIPE case.
Prior review resolved
My earlier round flagged (1) bytesForWrite honoring the caller's encoding while the sink always emits UTF-8, and (2) the prototype _write fast-path branch being the one sibling not updated. Commits 467f785 / f338d81 / 14a770c dropped the encoding parameter (now Buffer.byteLength(data) → UTF-8), added the increment to prototype _write, and trimmed the explanatory comment to one line. All inline threads are resolved.
Security risks
None. This is a read-only counter on a Node-compat stream object; no input validation, auth, or protocol parsing is touched.
Level of scrutiny
Low-to-medium. The stdio write path is hot, but the change is purely additive bookkeeping — the actual write call and its error handling are unchanged. Buffer.byteLength on a string is a cheap native scan and matches what Node's net.Socket does for _bytesDispatched. I checked that the fast-path branch and the writeAll branch are mutually exclusive (if (fileSink && fileSink !== true) { ... } else { writeAll... }) so there is no double-increment when the fast path is not taken.
Other factors
The tests follow harness conventions (tempDir, bunEnv, concurrent pipe drain via Promise.all, exit-code asserted last) and assert exact byte counts against Node's behavior. The EPIPE test is POSIX-only via test.skipIf(isWindows) and awaits observable conditions (drain 3 bytes, then stdin signal) rather than sleeping. The PR description already flags the interaction with #33508 for whichever lands second.
|
CI on 14a770c: all failures are tagged pre-existing or flaky by the build annotation helper and none touch the stdio write path or Both new tests pass on every lane. Ready for review. |
|
Closing in favor of #31627, which was opened first for the same fix and still merges cleanly onto current main. The last revision here also moved the increment into the promise fulfillment handler, and that does not match Node on the common case: writing 1 MiB to a pipe that is not being drained, Node reports I left a note on #31627 suggesting it pick up the pipe-mode test from this PR, which passes unchanged against that branch. |
Fixes #23061. Adopts the approach from #31627 by @EffortlessSteven.
Reproduction
Cause
process.stdoutandprocess.stderrinstall an ownwrite(writeFast) and_write(underscoreWriteFast) that hand the chunk straight to aFileSinkwithout going throughwriteAll/writevAll, which are the only places that incrementthis.bytesWritten. So the counter stayed at its initial0no matter how much was written. Node's stdio streams (net.Socket/tty.WriteStream) report the encoded byte count synchronously afterwrite().Fix
Increment
this.bytesWrittenat eachfileSink.write()site (writeFast,underscoreWriteFast, the prototype_writefast-path branch, and the fast-path_writevbranch), but only in the success arms: when the sink returns synchronously, or inside the promise fulfillment handler. A write the sink rejects (EPIPE) is not counted, matching Node'snet.Socket, which only adds to_bytesDispatchedafter a successful low-level write.FileSink.write(string)has no encoding parameter and always emits UTF-8, so strings are measured withBuffer.byteLength(data)(UTF-8) to stay in lockstep with what actually reaches the fd; typed-array / Buffer chunks usebyteLength.Verification
Two new tests in
test/js/node/process/process-stdio.test.ts:{stdout: 10, stderr: 7}). Fails on main with{stdout: 0, stderr: 0}.bytesWrittenstays at the last successful write's total and the callback receives an error. Node printsbytesWritten: 3for both the first (ok) and second (EPIPE) write; so does the fixed build.The existing
fs.WriteStream,createWriteStream, child-process-stdio, and tty suites stay green.Related: #33508 reworks this write path to go through
Writable.prototype.writebut explicitly leavesbytesWrittenfor a separate change; whichever lands second needs a small rebase of the increment ontounderscoreWriteFast.no test proof · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/node/process/process-stdio.test.ts