fs: batch small WriteStream writes via _writev and fix retry position - #31764
fs: batch small WriteStream writes via _writev and fix retry position#31764robobun wants to merge 9 commits into
_writev and fix retry position#31764Conversation
|
Updated 5:10 PM PT - Jul 26th, 2026
❌ @robobun, your commit 38cf319 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 31764That installs a local version of the PR into your bun-31764 --bun |
|
Found 4 issues this PR may fix:
🤖 Generated with Claude Code |
WalkthroughJS WriteStream disables unsupported _writev and guards pos updates; Rust adds POSIX_IOV_MAX and single-call pwritev_one/writev_one helpers, batching vecs into POSIX_IOV_MAX-sized chunks and preserving short-write/error semantics; tests exercise streaming, partial-write retries, and >IOV_MAX cases. ChangesWriteStream and IOV_MAX Batching
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/sys/lib.rs`:
- Around line 4641-4667: The loop in pwritev batching sets position = offset and
always does position += bytes_written, which loses a negative-position sentinel
(offset == -1) used by sys_uv::pwritev to mean "use current file offset"; update
the loop so you only advance position when it is non-negative (e.g., check if
position >= 0 before doing position += bytes_written as i64), preserving the
negative sentinel across chunks and leaving other variables and the call to
pwritev_one unchanged.
In `@test/js/node/fs/fs.test.ts`:
- Around line 2662-2670: The async test using fs.writev may skip closeSync(fd)
if the write rejects; wrap the awaited Promise.withResolvers promise in a
try/finally and call closeSync(fd) in the finally block so the descriptor opened
by openSync(p, "w") is always closed; keep the existing fs.writev callback
resolution logic (Promise.withResolvers<number>), and ensure fd is declared
before the try so finally can always reference it.
🪄 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: dcc0724e-c855-45bd-8dc8-8a67e6d51688
📒 Files selected for processing (3)
src/js/internal/fs/streams.tssrc/sys/lib.rstest/js/node/fs/fs.test.ts
|
Thanks. I checked each:
So just |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thanks — the NaN-offset guard in 62d7811 addresses my earlier concern and the regression tests cover both retry paths. I'm not auto-approving because this re-enables a default fs.WriteStream code path and reworks the cross-platform writev/pwritev batching loop in src/sys/lib.rs, which is worth a human pass (and Build #60157 showed failures on 62d7811 before the retrigger).
Extended reasoning...
Overview
This PR re-enables _writev batching on the default fs.WriteStream path (it was explicitly nulled out in the constructor since #16422 / Bun 1.2.0), and adds an IOV_MAX batching loop to bun_sys::writev / bun_sys::pwritev so >1024-iovec arrays don't EINVAL. It also fixes writevAll to pass the local pos parameter instead of this.pos, and — in response to my earlier inline comment — guards pos += bytesWritten in both writeAll and writevAll so a partial-write retry with this.pos === undefined doesn't compute NaN (which Bun's binding coerces to offset 0). Three files: src/js/internal/fs/streams.ts, src/sys/lib.rs, test/js/node/fs/fs.test.ts.
Security risks
None identified. This is local file-I/O syscall plumbing and stream batching; no auth, crypto, network, or untrusted-input parsing is touched. The earlier silent-data-corruption risk (NaN→offset-0 retry) has been fixed and now has a regression test.
Level of scrutiny
Moderate-to-high. The Rust change refactors per-platform #[cfg] branches (macOS ``, Linux/Android linux_syscall, generic POSIX with EINTR retry) into `writev_one`/`pwritev_one` helpers and wraps them in a new accumulation loop that introduces a partial-result path (return bytes-so-far rather than the error if a later batch fails). That semantic is intentional and mirrors libuv / `sys_uv::pwritev`, but it's a behavioral change at the syscall wrapper layer that every `fs.writev`/`fs.WriteStream` call on POSIX now goes through. The JS side flips a default that was deliberately set in 1.2.0, so the original reason for disabling `_writev` (whatever motivated #16422) is worth a maintainer confirming as no longer applicable.
Other factors
- All three earlier review threads (coderabbit's negative-offset sentinel, fd cleanup in test, and my NaN-retry corruption) are resolved in the current diff; I verified the guards and the
position >= 0check are present. - Test coverage is good: a 5000-chunk
_writevend-to-end test that crosses IOV_MAX, parameterized partial-write retry tests for bothwriteandwritevusing a custom-fs shim, andwritevSync/pwritevSync/asyncfs.writevtests at 1023/1024/1025/2000/3000/5000 buffers. - The robobun build comment reports failures on Build #60157 for commit 62d7811; c88610b is a
ci: retriggerso the failure may be flaky, but a human should confirm CI is green before merge. - The Rust refactor is largely a code-motion of the existing per-OS bodies into helpers plus a straightforward chunking loop; the
<= POSIX_IOV_MAXfast path keeps the common case identical to before.
Given the cross-platform syscall surface, the re-enabling of a previously-disabled default path, and the pending CI status, this should get a human review rather than an auto-approval.
|
For a reviewer — on "why was It looks like an oversight in the original port rather than a deliberate correctness decision. In #16422 (the commit that created Two things corroborate that the default
So re-enabling it matches Node, and the latent bugs that would have bitten if it were ever live are fixed. Happy to adjust if there's history I'm missing. |
c88610b to
6860162
Compare
There was a problem hiding this comment.
No new issues found after the readv/preadv cap in 9c2e3fe, but this re-enables a previously-disabled _writev path on the default createWriteStream and rewrites the POSIX writev/pwritev/readv/preadv batching in src/sys/lib.rs, so it warrants a human look — particularly on the open question you raised about why #16422 disabled _writev in the first place.
Checked: the IOV_MAX batching loops preserve short-write/partial-error semantics and the negative-offset sentinel; writeAll/writevAll retry offsets are now guarded against undefined → NaN; readv/preadv cap (not loop) matching libuv. Also looked at the custom-fs-with-writev-but-no-write case — Node doesn't null _write there either, so leaving it as-is is correct.
Extended reasoning...
Overview
This PR touches three files: src/js/internal/fs/streams.ts (re-enables _writev on the default WriteStream path and guards partial-write retry offsets), src/sys/lib.rs (extracts writev_one/pwritev_one helpers and adds IOV_MAX batching loops for writes plus an IOV_MAX cap for reads), and test/js/node/fs/fs.test.ts (~220 lines of new coverage for batching, partial-write retries, and >1024-iovec writev/readv).
Security risks
None identified. No parsing of untrusted input, no auth/crypto/permissions surface. The syscall wrappers slice a caller-owned iovec array; slicing is bounds-checked Rust. The only user-influenced quantity is the iovec count, which is now capped rather than passed through unchecked — strictly a hardening.
Level of scrutiny
High. src/sys/lib.rs is the shared syscall layer for all POSIX file I/O in the runtime, and src/js/internal/fs/streams.ts is a hot-path built-in module. Re-enabling _writev activates a code path (writevAll) that had never run against Bun's native binding on the default createWriteStream(path) — two latent bugs in that path were surfaced and fixed during review (the this.pos vs pos parameter and the undefined + n → NaN → offset 0 corruption). The batching loop also introduces a new partial-result contract (error-after-progress returns Ok(total) rather than the error), which is the libuv behavior but changes what callers of bun_sys::writev/pwritev observe on mid-batch failure.
Other factors
- The author explicitly flagged an open question for a human reviewer: whether #16422 disabled
_writevdeliberately or as an oversight. That's a design/history judgment a bot shouldn't settle. - All prior bot feedback (CodeRabbit's negative-offset sentinel and fd-cleanup nits, my NaN-offset corruption finding, and the readv/preadv sibling gap) has been addressed with tests. No outstanding review comments.
- Cross-platform: the Rust changes are
#[cfg(unix)]-gated with per-OS branches (macOS nocancel, Linux syscall, generic libc EINTR loop); Windows already batches insys_uv. The extraction into*_onehelpers is a mechanical move of the pre-existing per-OS bodies.rust:check-allreportedly passes. - Test coverage looks solid (boundary counts 1023/1024/1025/2000/5000, sync + async, position + no-position, partial-write retry via a custom fs shim, readv short-read semantics). The readv tests are correctly
skipIf(isWindows). - Not a simple/mechanical change per the approval bar; deferring to human review.
|
CI status for build 82765 (latest push): seven lanes never ran because Buildkite agents failed to provision (infrastructure, not this change), and the only test failures are three known-flaky tests on darwin lanes (no-orphans, regression/20144, test-stream-pipeline), all unrelated to fs. Every lane that ran passes the fs tests from this PR. Ready for review. |
9c2e3fe to
36c7025
Compare
There was a problem hiding this comment.
All three of my earlier inline comments (NaN retry offset, readv/preadv IOV_MAX cap, comment length) have been addressed and no new issues surfaced this pass. I'm not auto-approving because this re-enables a WriteStream code path that #16422 deliberately shadowed and reworks the shared sys::writev/pwritev/readv/preadv wrappers — worth a maintainer confirming the #16422 history question the author raised.
What was reviewed:
writev/pwritevbatching loop: ≤1024-iovec fast path is byte-identical to the old body; short-write and mid-batch error both stop and report partial progress; negative-offset sentinel preserved across batches.- Other
sys::pwritevcallers (RuntimeTranspilerCache) use small fixed iovec counts, so they hit the unchanged fast path. writeAll/writevAllretry:posguarded againstundefined, andwritevAllnow forwards itsposparameter instead ofthis.pos.- Custom
options.fswithwritevbut nowrite:_writestays enabled (Node also keeps it) — ruled out as a compat gap.
Extended reasoning...
Overview
Three files: src/js/internal/fs/streams.ts stops nulling _writev on the default WriteStream path (only nulls it when a custom options.fs lacks writev, matching Node), fixes writevAll to pass its pos parameter instead of this.pos, and guards pos += bytesWritten in both writeAll/writevAll so an undefined position doesn't become NaN on partial-write retry. src/sys/lib.rs extracts the existing per-platform writev/pwritev bodies into *_one helpers unchanged, wraps them in an IOV_MAX batching loop for >1024 iovecs, and caps readv/preadv at IOV_MAX (short read, matching libuv). test/js/node/fs/fs.test.ts adds ~200 lines of coverage: _writev coalescing past 1024 iovecs, partial-write retry for both write and writev via a custom fs, writevSync/pwritevSync/async writev at 1023/1024/1025/2000/3000/5000 buffers, and readvSync/preadv/async readv at 1025 buffers.
Security risks
None identified. No auth, crypto, or untrusted-input parsing. The iovec count comes from a JS array length; the new code only reduces what's passed to the kernel (caps/batches at 1024), so it strictly narrows the syscall surface relative to before. The chunk_capacity sum is usize over caller-owned buffer lengths and only gates the short-write break — overflow would require >2^64 bytes of buffers.
Level of scrutiny
Medium-high. src/sys/lib.rs is a shared syscall layer, and fs.WriteStream is a heavily-used Node compat API. The refactor is defensive (≤1024 iovecs early-return to code identical to the old body), and I checked the other in-tree sys::pwritev caller (RuntimeTranspilerCache) — it uses a small fixed iovec array in its own retry loop, so it takes the unchanged fast path. Still, this changes runtime behavior for a widely-used stream and re-enables a path that was explicitly disabled in #16422; the author left an open note for a reviewer asking whether that disabling had history they're missing. That's a question for a maintainer, not a bot.
Other factors
All prior review feedback (mine and CodeRabbit's) is resolved: the negative-offset sentinel is preserved across pwritev batches, the async writev test closes its fd in finally, the NaN-retry-offset bug is fixed with regression tests for both write and writev, readv/preadv are capped, and the test header comments were trimmed. The bug-hunting pass this run found nothing new; the one candidate raised (custom fs with writev but no write should disable _write) was checked against Node's implementation and ruled out — Node keeps _write too. Test coverage is thorough across the boundary (1023/1024/1025) and both sync/async/positional variants. The remaining reason to defer is scope, not a specific concern.
## Problem
Every vectored-I/O entry point in `node:fs` (`writevSync` / `writev` /
`promises.writev` / `FileHandle.writev` / `FileHandle.readv` /
`readvSync`) throws `EINVAL` the moment the buffer array crosses
`IOV_MAX` (1024 on Linux and macOS). Node.js handles any count:
```js
import * as fsp from "node:fs/promises";
import * as fs from "node:fs";
for (const n of [1024, 1025, 2000]) {
const fh = await fsp.open(p, "w+");
await fh.writev(Array.from({ length: n }, (_, i) => Buffer.from([i & 255])));
// ...
}
```
```
node v26.3.0: n=1024 writev:1024 readv:1024 | n=1025 writev:1025 readv:1024 | n=2000 writev:2000 readv:1024
bun (before): n=1024 writev:1024 readv:1024 | n=1025 writev:EINVAL readv:EINVAL | n=2000 writev:EINVAL readv:EINVAL
bun (after): n=1024 writev:1024 readv:1024 | n=1025 writev:1025 readv:1024 | n=2000 writev:2000 readv:1024
```
## Cause
`NodeFS::{writev,pwritev,readv,preadv}_inner` pass the full iovec array
to a single `writev(2)` / `preadv(2)`. POSIX kernels reject `iovcnt >
IOV_MAX` with `EINVAL`.
Node's libuv handles this in `uv__fs_write_all` (loops `IOV_MAX`-sized
batches, accumulates bytes, returns the partial total on a mid-loop
error) and `uv__fs_read` (caps `nbufs` at `IOV_MAX`, single syscall).
## Fix
Mirror libuv in the `node:fs` layer (`src/runtime/node/node_fs.rs`):
- `writev_inner` / `pwritev_inner`: loop `IOV_MAX`-sized slices of the
iovec array, accumulating `bytes_written`. An error or short write after
the first batch returns the accumulated total; `pwritev` advances the
position by bytes written each batch.
- `readv_inner` / `preadv_inner`: slice the iovec array to at most
`IOV_MAX` entries and issue one syscall.
`IOV_MAX` is taken from `libc::UIO_MAXIOV` on Linux and `libc::IOV_MAX`
elsewhere (both 1024 on supported targets). Windows has no kernel iovec
limit; the constant is `c_uint::MAX` there so the loop degenerates to a
single call into `sys_uv`, which already batches.
A small `bun_sys::platform_iovec_len` helper is added so the
chunk-capacity sum compiles on both the `libc::iovec` (unix) and
`uv_buf_t` (windows) field layouts.
## Tests
`test/js/node/fs/fs.test.ts` gains a `writev/readv with more than
IOV_MAX buffers` block covering all entry points with 2000 one-byte
buffers: `writevSync`, `writevSync` with position, callback `fs.writev`,
`FileHandle.writev`, `readvSync`, `readvSync` with position, and
`FileHandle.readv`. Each writev case asserts the full byte count and
file contents; each readv case asserts `bytesRead == 1024` on POSIX (the
libuv cap). All seven fail with `EINVAL` on stock bun and pass with this
change.
`bun run rust:check-all` passes on every target.
## Related
#31764 overlaps on the `writev` side (it batches inside
`bun_sys::writev`/`pwritev` as part of a larger `fs.WriteStream` change)
but does not cover `readv`/`preadv`. This PR places the chunking in the
`node:fs` layer where the libuv semantics belong and leaves the raw
`bun_sys` wrappers as single-syscall primitives.
There was a problem hiding this comment.
I didn't find any bugs, but this enables the previously-disabled _writev path on default fs.WriteStream and reworks the POSIX writev/pwritev/readv/preadv syscall wrappers — worth a human look given how hot createWriteStream is.
Checked: the pwritev batching loop stops on short writes and preserves the negative-offset sentinel; writevAll's this.pos → pos fix is correct (the caller advances this.pos before the callback fires); the .len vs .iov_len field accesses match PlatformIoVecConst vs libc::iovec; the fast-path branch still nulls _writev so stdio streams are unaffected.
Extended reasoning...
Overview
This PR fixes GitHub issue #31763 across three layers:
src/sys/lib.rs: Extractspwritev_one/writev_onehelpers and wraps them in a batching loop capped atPOSIX_IOV_MAX(1024), so callers passing >1024 iovecs no longer getEINVALfrom the kernel.readv/preadvare capped to one batch and return a short read (matching libuv). Thepwritevloop advances an explicit offset per batch but preserves a negative sentinel, and reports partial progress rather than an error if a later batch fails.src/js/internal/fs/streams.ts: Stops unconditionally disabling_writevon default-fsWriteStream, so buffered writes now coalesce intofs.writev(Node-matching). Also fixes two latent bugs in the newly-livewritevAllpath: it was readingthis.pos(already advanced by_writev) instead of the capturedpos, and bothwriteAll/writevAllcomputedundefined + bytesWritten→NaNon retry when nostartwas set. For customfswithoutwritev,_writevis now correctly disabled (previously it stayed on the prototype and would have calledundefined(...)).test/js/node/fs/fs.test.ts: ~200 lines covering_writevbatching past IOV_MAX, the NaN retry regression for bothwriteandwritev, andwritevSync/readvSyncat the 1023/1024/1025/2000/5000 boundaries.
Security risks
None. This is I/O batching and offset arithmetic on file descriptors the caller already owns; no new user-controlled input reaches a trust boundary.
Level of scrutiny
Medium-high. fs.createWriteStream is one of the most heavily used Node APIs, and this PR flips its default write path from per-chunk _write to batched _writev — a code path that was previously dead for the default case (and had at least two latent bugs, both fixed here). The Rust changes also touch the shared syscall wrappers used by every vectored I/O caller in the runtime. Nothing looks wrong, but the blast radius is large enough that a maintainer should confirm the behavioral change is intended and that there are no other consumers of bun_sys::writev/pwritev that depend on single-syscall semantics.
Other factors
- Test coverage is thorough and hits the exact boundaries (1023/1024/1025) plus the retry/NaN case for both write variants.
- I verified the field names in the batching loops (
v.lenonPlatformIoVecConst,v.iov_lenonlibc::iovec) match the struct definitions. - The
$fastPathbranch still setsthis._writev = undefined, so stdio andBun.file().writer()paths are unchanged. - The commit history shows iterative fixes (negative-offset sentinel, NaN guard, readv cap) landed as separate commits, suggesting the author already caught and addressed the obvious edge cases.
fs.WriteStream unconditionally set `this._writev = undefined`, disabling Node's Writable writev batching. Many small writes (e.g. JSZip's generateNodeStream emitting tens of thousands of tiny chunks) were then drained one chunk per `_write()` -> `fs.write()` syscall instead of being coalesced, making it 20-50x slower than Node. Keep `_writev` enabled on the default path and disable it for a custom `fs` only when that fs has no `writev`, matching Node. This lets the existing `writeStreamPrototype._writev` coalesce buffered chunks into a single `fs.writev`. Coalescing exposed a second bug: a batch can exceed IOV_MAX (1024) iovecs, which writev(2)/pwritev(2) reject with EINVAL. Node's fs.writev handles >1024 buffers transparently. Batch oversized iovec arrays in bun_sys::writev/pwritev into groups of at most IOV_MAX, accumulating bytes and stopping on a short write — mirroring the existing Windows sys_uv path. This fixes raw fs.writev/fs.writevSync with many buffers too. Also fix writevAll passing `this.pos` instead of the `pos` parameter, which wrote at the wrong offset on partial-write retries.
… test Address review feedback: - In the batched pwritev path, only advance `position` when it is non-negative. A negative offset is the "use the current file offset" sentinel; incrementing it would switch later batches to explicit offsets on the oversized-iovec path. Matches sys_uv::pwritev. - Close the fd via try/finally in the async fs.writev test so it is not leaked if the write rejects.
On a partial write, writeAll/writevAll retried the unwritten tail with
`pos += bytesWritten`. For the default createWriteStream(path) (no
`start`), `this.pos` is undefined, so the retry computed
`undefined + bytesWritten === NaN`. Bun's fs binding coerces a NaN
position to offset 0, so the retry wrote the tail at the start of the
file, silently overwriting the head.
This path became reachable for plain createWriteStream once `_writev`
was re-enabled, and the new IOV_MAX batching loop adds a partial-result
path that can trigger it. Only advance an explicit position, leaving
undefined ("current file offset") intact on retry — matching the guards
already in `_write`/`_writev`. Add regression tests that simulate a
short write for both the write and writev paths.
b36e46c to
368a3e9
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Rebased onto main at 44f6469. The conflict was in the
Locally with this branch: All The previous build's red (74155) was all unrelated: |
robobun
changed the title
_writev and handle IOV_MAX_writev and fix retry position
There was a problem hiding this comment.
LGTM — the diff is now down to four production lines in streams.ts plus tests, and every prior review round's feedback has been addressed.
What was reviewed:
_writevre-enable: confirmed the fast-path branch still setsthis._writev = undefined, so only the defaultfs.writev-backed path is affected; matches Node's rule of disabling only when a custom fs lackswritev.writevAllthis.pos → posand the twoif (pos !== undefined)guards: traced the retry withstart: 0andstart: undefined— the new 4-case matrix makes each clause load-bearing.- Coalescing test: pump loop + 16KB HWM guarantees a >1024-chunk batch; #33695's node_fs IOV_MAX chunking handles it, so no EINVAL.
Extended reasoning...
Overview
After several rescoping rounds, this PR is now four production-line changes to src/js/internal/fs/streams.ts plus ~130 lines of tests in test/js/node/fs/fs.test.ts:
- Delete
this._writev = undefinedfrom the default-fselsebranch, soWriteStreaminherits the prototype_writevand Writable'sclearBufferbatches buffered chunks into onefs.writev(Node's behavior). writevAll: pass theposparameter instead ofthis.postothis[kFs].writev.writeAll/writevAll: guardpos += bytesWrittenwithif (pos !== undefined)so a retry with nostartdoesn't produce NaN → offset 0.
The src/sys/lib.rs IOV_MAX batching that was in earlier revisions has been dropped (superseded by #33695 in node_fs.rs), and the duplicate test blocks were folded into the existing writev/readv with more than IOV_MAX buffers describe. All comment-length nits from comment-cop and the earlier review rounds are resolved. My last round's feedback (the writev retry test being vacuous without start: 0) was addressed in 38cf319 by expanding the matrix to [write, writev] × [undefined, 0].
Security risks
None. This is a Node-compat performance/correctness fix in the fs WriteStream layer. No parsing of untrusted input, no auth/crypto/permissions surface. The re-enabled _writev path routes through the same fs.writev binding that has always been reachable via a custom options.fs that supplies writev.
Level of scrutiny
Medium. The line count is tiny, but the behavioral surface is real: every default createWriteStream() that buffers multiple chunks will now go through fs.writev instead of per-chunk fs.write. That warranted checking: (a) the fast-path ($fastPath) still nulls _writev explicitly, so stdio/FileSink streams are unaffected; (b) fileHandleStreamFs supplies writev (or undefined, which the customFs branch already handles via if (!writev) this._writev = null); (c) #33695 already chunks >IOV_MAX at the node_fs layer, so the 5000-chunk coalescing test won't EINVAL; (d) the writevAll retry logic is now correct for both positional and current-offset writes.
Other factors
- Test coverage is solid and each production clause is load-bearing in at least one test (verified in the previous review round; the author confirmed the
writev + start: 0variant fails on main'sstreams.ts). - Node parallel
test-fs-write-stream*.js/test-fs-writev*.jsreportedly pass. - All bot/inline threads are resolved; no outstanding human review comments.
- The bug-hunting system found nothing on this revision.
2 participants
Summary
fs.WriteStreamwas ~2x slower than Node for workloads that emit many small chunks (reported with JSZip'sgenerateNodeStream, #31763). The stream drained one chunk per_write()→fs.write()syscall instead of coalescing, and enabling the existing batched path surfaced anEINVALonce a batch crossedIOV_MAX(1024) iovecs.Closes #31763.
Fixes #21252 — same root cause: that issue bisected a 1.1.x→1.2.0 regression in line-by-line writes to
fs.createWriteStream(~1.34M small writes, ~7x slower over a network share). Thefs.WriteStreamrewrite that introducedthis._writev = undefined(#16422) first shipped in 1.2.0, which matches the bisect exactly; re-enabling_writevrestores the batching that made 1.1.x fast.Reproduction
JSZip repro from the issue (13442 files):
Cause
src/js/internal/fs/streams.tssetthis._writev = undefinedin theWriteStreamconstructor. Node'sWritableonly batches buffered chunks into a single_writev(chunks, cb)call whenstream._writevis truthy (clearBuffer:bufferedLength > 1 && stream._writev). With it nulled out, every buffered chunk went individually through_write→fs.write(). ForcreateWriteStream("out.zip")(the JSZip case) tens of thousands of chunks meant tens of thousands of trips through the per-chunk state machine and syscall.Node disables
_writevonly when the underlyingfshas nowritev; the defaultfshas it, so Node batches.Fix
Keep
_writevenabled on the default path, and for a customoptions.fsdisable it only when that fs has nowritev— matching Node. The already-presentwriteStreamPrototype._writevthen coalesces buffered chunks into onefs.writev.writevAlloffset fixes: it passedthis.posinstead of theposparameter, so partial-write retries wrote at the wrong offset; and with nostart, bothwriteAll/writevAllcomputedundefined + bytesWritten(NaN, coerced to offset 0) on retry, which would overwrite the file head.Note on scope: an earlier revision of this PR also batched
writev/pwritevand cappedreadv/preadvatIOV_MAXinbun_sys. Main has since landed the same handling one layer up (#33695,node:fschunks atIOV_MAXmatching libuv), so those changes and their duplicate tests were dropped after the rebase; this PR now only touches the WriteStream path and adds boundary coverage (1023/1024/1025/5000) to the existing IOV_MAX test block.Verification
New tests in
test/js/node/fs/fs.test.ts:createWriteStream > coalesces many small writes via _writev (issue #31763)— 5000 small writes produce byte-for-byte correct output,_writevis exercised, and at least one batch exceeds 1024 iovecs.createWriteStream > partial write/writev retry does not corrupt the file (issue #31763)— a custom fs forces a short write, over a matrix ofstart: undefinedandstart: 0; the retry must not pass a NaN position and must resume from the capturedpos, notthis.pos.writevSync writes all %d buffers at the IOV_MAX boundary(1023/1024/1025/5000), added to the existingwritev/readv with more than IOV_MAX buffersblock.The stream tests fail on stock bun (
_writevisundefined; NaN retry position) and pass with this change. Existingfs.WriteStream,createWriteStream, and writev/readv IOV_MAX coverage still passes, as do the Node paralleltest-fs-write-stream*.js/test-fs-writev*.jsscripts.no test proof · iteration 7 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/node/fs/fs.test.ts