node:fs: chunk writev/readv at IOV_MAX to match libuv - #33695
Conversation
writevSync / writev / promises.writev / FileHandle.writev / FileHandle.readv all threw EINVAL when given more than IOV_MAX (1024) buffers because the whole iovec array was handed to a single writev(2)/readv(2). Node's libuv (uv__fs_write_all) writes in IOV_MAX-sized batches and loops until every buffer is written; uv__fs_read caps nbufs at IOV_MAX and issues one syscall. Mirror that in the node:fs *_inner helpers so readv/preadv are capped and writev/pwritev loop to completion.
|
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 (2)
WalkthroughThis PR adds an ChangesIOV_MAX-aware vectored I/O
Sequence Diagram(s)sequenceDiagram
participant NodeFS
participant Syscall
loop IOV_MAX-sized batches
NodeFS->>Syscall: pwritev/writev(batch)
Syscall-->>NodeFS: bytes written or error
NodeFS->>NodeFS: accumulate bytes_written and advance position
end
sequenceDiagram
participant NodeFS
participant Syscall
NodeFS->>NodeFS: cap iovec slice to IOV_MAX
NodeFS->>Syscall: preadv/readv(clamped iovecs)
Syscall-->>NodeFS: bytes read
Related issues: None specified 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 7:20 PM PT - Jul 7th, 2026
❌ @robobun, your commit a1c3a52 has some failures in 🧪 To try this PR locally: bunx bun-pr 33695That installs a local version of the PR into your bun-33695 --bun |
There was a problem hiding this comment.
No bugs found and the implementation looks correct, but this changes error-handling semantics on a runtime syscall path (mid-loop errors now return partial success) and overlaps with #31764's competing placement of the chunking — worth a maintainer sign-off on where this should live.
Extended reasoning...
Overview
This PR fixes EINVAL from fs.writev/readv (and their sync/promise/FileHandle variants) when passed more than IOV_MAX (1024) buffers, by mirroring libuv: writev/pwritev loop in IOV_MAX-sized batches accumulating bytes, and readv/preadv cap the iovec slice at IOV_MAX for a single syscall. It touches src/runtime/node/node_fs.rs (four *_inner functions plus a new IOV_MAX cfg-gated constant), adds a platform_iovec_len helper in src/sys/lib.rs, and adds seven tests in test/js/node/fs/fs.test.ts.
I verified the type asymmetry between the two write paths is intentional and correct: pwritev_inner iterates PlatformIoVecConst (which has a .len field on both unix and windows), while writev_inner iterates PlatformIoVec (libc::iovec on unix with iov_len, uv_buf_t on windows with len) — hence the new platform_iovec_len helper only on the latter.
Security risks
None. Input is a user-provided array of buffers already validated upstream; the change only slices/loops over an existing iovec array. No new untrusted-length arithmetic — chunk_len is bounded by remaining.len(), and chunk_capacity sums usize lengths that were already accepted before this change.
Level of scrutiny
Medium-high. This is production runtime code on a syscall path used by every node:fs vectored-I/O call, with per-platform #[cfg] gates (Linux UIO_MAXIOV, other unix IOV_MAX, Windows c_uint::MAX). The change is not mechanical: it introduces a loop with new error semantics — an error or Ok(0) after the first successful batch is now swallowed and the accumulated total returned, whereas previously the (single) syscall error propagated directly. That matches libuv's intent, but it's a behavioral change a maintainer should confirm.
Other factors
- The PR description explicitly flags overlap with #31764, which places the same chunking inside
bun_sys::writev/pwritevinstead. Which layer owns this is a design call for a maintainer. - The short-write handling here
breaks rather than advancing iovecs and retrying (which libuv'suv__fs_write_allactually does). That's a strict improvement over the prior single-syscall behavior and probably fine, but it's a deliberate simplification worth a human glance. - Test coverage is good: all seven entry points, sync/callback/promise, with-position and without, and a Windows-aware
readvCap. - No CODEOWNERS entries match the touched files.
|
On the two review points: Mid-loop error semantics. Returning the accumulated total when a batch after the first fails is libuv's Layer placement vs #31764. The chunking is placed in The short-write CI status (build 70065): 281 jobs passed, This is ready for a maintainer; the red is infrastructure, not the diff. The failed darwin shard can be retried individually from the Buildkite UI. |
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 `@test/js/node/fs/fs.test.ts`:
- Around line 1683-1792: The fs I/O cases in the writev/readv IOV_MAX suite are
independent and should be run concurrently. Update the enclosing describe block
around the writev/readv tests to use concurrent execution, since each test uses
its own tempDir, fd, or FileHandle with no shared state. Keep the existing test
bodies and locate the change in the writev/readv with more than IOV_MAX buffers
describe block.
🪄 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: 37884b86-156f-4ced-b112-b11b22df6234
📒 Files selected for processing (3)
src/runtime/node/node_fs.rssrc/sys/lib.rstest/js/node/fs/fs.test.ts
Problem
Every vectored-I/O entry point in
node:fs(writevSync/writev/promises.writev/FileHandle.writev/FileHandle.readv/readvSync) throwsEINVALthe moment the buffer array crossesIOV_MAX(1024 on Linux and macOS). Node.js handles any count:Cause
NodeFS::{writev,pwritev,readv,preadv}_innerpass the full iovec array to a singlewritev(2)/preadv(2). POSIX kernels rejectiovcnt > IOV_MAXwithEINVAL.Node's libuv handles this in
uv__fs_write_all(loopsIOV_MAX-sized batches, accumulates bytes, returns the partial total on a mid-loop error) anduv__fs_read(capsnbufsatIOV_MAX, single syscall).Fix
Mirror libuv in the
node:fslayer (src/runtime/node/node_fs.rs):writev_inner/pwritev_inner: loopIOV_MAX-sized slices of the iovec array, accumulatingbytes_written. An error or short write after the first batch returns the accumulated total;pwritevadvances the position by bytes written each batch.readv_inner/preadv_inner: slice the iovec array to at mostIOV_MAXentries and issue one syscall.IOV_MAXis taken fromlibc::UIO_MAXIOVon Linux andlibc::IOV_MAXelsewhere (both 1024 on supported targets). Windows has no kernel iovec limit; the constant isc_uint::MAXthere so the loop degenerates to a single call intosys_uv, which already batches.A small
bun_sys::platform_iovec_lenhelper is added so the chunk-capacity sum compiles on both thelibc::iovec(unix) anduv_buf_t(windows) field layouts.Tests
test/js/node/fs/fs.test.tsgains awritev/readv with more than IOV_MAX buffersblock covering all entry points with 2000 one-byte buffers:writevSync,writevSyncwith position, callbackfs.writev,FileHandle.writev,readvSync,readvSyncwith position, andFileHandle.readv. Each writev case asserts the full byte count and file contents; each readv case assertsbytesRead == 1024on POSIX (the libuv cap). All seven fail withEINVALon stock bun and pass with this change.bun run rust:check-allpasses on every target.Related
#31764 overlaps on the
writevside (it batches insidebun_sys::writev/pwritevas part of a largerfs.WriteStreamchange) but does not coverreadv/preadv. This PR places the chunking in thenode:fslayer where the libuv semantics belong and leaves the rawbun_syswrappers as single-syscall primitives.