fs: keep WriteStream short-write retries at the current offset - #36136
fs: keep WriteStream short-write retries at the current offset#36136robobun wants to merge 1 commit into
Conversation
A WriteStream with no start option passes pos = undefined to writeAll/writevAll. On a partial write the retry computed undefined += bytesWritten, which is NaN, and the fs.write argument parser coerced NaN to position 0. The retry then issued a pwrite at offset 0, stamping the unwritten tail over the head of the file and reporting success. streams.ts: only advance pos when it is a number, and pass the captured pos (not this.pos, which _writev has already advanced past the unwritten tail) to fs.writev. node_fs.rs: parse the fs.write/fs.writeSync position the way Node's GetOffset does: a non-negative safe integer selects pwrite; NaN, Infinity, -Infinity, non-integers and negatives fall back to the current file offset instead of becoming offset 0.
|
Warning Review limit reached
Next review available in: 17 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 (4)
Comment |
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Duplicate of #36135 (opened a few minutes earlier). The writevAll piece this PR adds on top is noted there; branch |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| fn write_position_from_js(v: JSValue) -> Option<i64> { | ||
| if let Some(num) = v.get_number() { | ||
| if num.is_finite() | ||
| && num.trunc() == num | ||
| && num >= 0.0 | ||
| && num <= bun_jsc::MAX_SAFE_INTEGER as f64 | ||
| { | ||
| return Some(num as i64); | ||
| } | ||
| return None; | ||
| } | ||
| let position = i52::from_js(v); | ||
| if position >= 0 { Some(position) } else { None } | ||
| } |
There was a problem hiding this comment.
🟡 Same-class site not covered: FdVectorIo::from_js (node_fs.rs:2829-2830, backing fs.writev/writevSync/readv/readvSync) still does Some(pos_value.to_int64() as u64) when is_number(), so fs.writev(fd, bufs, NaN, cb) still becomes pwritev at offset 0 instead of the current file offset. Node's WriteBuffers/ReadBuffers route position through the same GetOffset helper this PR mirrors, so write_position_from_js (adapted for Option<u64>) should replace the to_int64() coercion there too. The WriteStream corruption path is closed by the streams.ts fix (writevAll no longer passes NaN), so this only affects direct user calls with non-safe-integer positions — mentioning per REVIEW.md "fix the whole class in the same PR".
Extended reasoning...
What
The PR introduces write_position_from_js() to mirror Node's GetOffset semantics (only a non-negative safe integer selects positional I/O; NaN/±Infinity/non-integers/negatives fall back to the current file offset) and applies it to the fs.write/fs.writeSync argument parser at both call sites. However, the sibling parser FdVectorIo::from_js at node_fs.rs:2827-2830 — aliased as both args::Writev and args::Readv (:2848-2849) and therefore backing fs.writev, fs.writevSync, fs.readv, and fs.readvSync — still does:
if pos_value.is_number() {
position = Some(pos_value.to_int64() as u64);
}NaN passes is_number(), and JSValue::to_int64(NaN) returns 0 (as the PR description itself confirms for the fs.write case). So fs.writev(fd, bufs, NaN, cb) still issues pwritev at offset 0 rather than writev at the current file offset. ±Infinity and non-integers like 1.5 similarly get coerced to a positional offset rather than falling through to -1.
Why the JS layer doesn't intercept it
The fs.writev wrapper in src/js/node/fs.ts passes position straight through to the native binding with no coercion, so a user-supplied NaN reaches FdVectorIo::from_js unmodified.
Node parity
Node's binding.writeBuffers and binding.readBuffers (src/node_file.cc) both route args[2] through GetOffset, which is exactly the helper write_position_from_js was written to emulate: IsSafeJsInt(v) ? v->IntegerValue() : -1. So Node's fs.writev(fd, bufs, NaN, cb) writes at the current file offset, while Bun after this PR still writes at offset 0.
Step-by-step
const fd = fs.openSync(p, 'r+')on a 10-byte file ofAAAAAAAAAA.fs.readSync(fd, Buffer.alloc(5), 0, 5, null)— advances the fd cursor to offset 5.fs.writevSync(fd, [Buffer.from('XX')], NaN).- JS wrapper passes
NaNthrough unchanged →FdVectorIo::from_js. NaN.is_number()is true →position = Some(to_int64(NaN) as u64) = Some(0).pwritev_innerseesSome(0)→ issuespwritev(fd, iov, 0).- File becomes
XXAAAAAAAA. Node writes at the cursor and producesAAAAAXXAAA.
The new writeSync > treats position %s as the current file offset test in this PR would fail if extended to cover fs.writevSync/fs.readvSync.
Impact / severity
This is nit severity. The primary bug this PR set out to fix — WriteStream retries stamping the tail over the file head — is fully closed: writevAll now passes the captured pos (which stays undefined on retry when no start was given), so NaN never reaches fs.writev from the WriteStream path anymore. The remaining divergence is only reachable via direct user calls to fs.writev/fs.readv with a pathological non-safe-integer position, which is pre-existing behavior and not a regression. Flagging it because REVIEW.md asks to "fix the whole class in the same PR — grep for every sibling site sharing the pattern", and this is the exact sibling of the parser the PR just fixed.
Fix
Replace the is_number() → to_int64() branch in FdVectorIo::from_js with the same safe-integer gate used in write_position_from_js (adapted for Option<u64>, since this parser stores u64 rather than i64), and extend the new position-coercion test to cover writevSync/readvSync.
2 participants
Summary
A
fs.WriteStreamthat takes a short write (EFBIG, ENOSPC, a partial writev) could silently overwrite the head of its output file with the unwritten tail and still emit"finish".Reproduction
Under a 1 MiB
RLIMIT_FSIZE, write a single 4 MiB chunk ofA|B|C|D1 MiB blocks:["finish"]DDDDDDDD["error:EFBIG"]AAAAAAAA["error:EFBIG"]AAAAAAAACause
Two bugs compound:
(a)
writeAll/writevAllretry arithmetic (src/js/internal/fs/streams.ts): with nostart,_write/_writevpassthis.pos(which isundefined) as the initialpos. After a partial write the retry computedpos = undefined + bytesWritten, i.e.NaN, and reissuedfs.write(fd, rest, 0, size, NaN, cb).writevAlladditionally readthis.pos(already advanced past the whole payload by_writev) instead of the capturedposparameter.(b)
fs.writeposition coercion (src/runtime/node/node_fs.rs): the argument parser rani52::from_js(position)which callsJSValue::to_int64, andto_int64(NaN)returns0. With0 >= 0the retry becamepwrite64(fd, rest, 0), stamping the tail over offset 0. Node'sGetOffsetinstead treats any non-safe-integer as-1(current offset), so its identicalwriteAllarithmetic is masked.Fix
streams.ts: advanceposonly when it is defined (matching Node'slib/internal/fs/streams.js), and pass the capturedposparameter, notthis.pos, tofs.writev.node_fs.rs: a newwrite_position_from_jsmirrors Node'sGetOffset(IsSafeJsInt(v) ? v : -1): a non-negative safe integer selectspwrite;NaN,±Infinity, non-integers and negatives fall back to the current file offset. BigInt keeps the existingi52path.Either half alone prevents the corruption; both are needed for full Node parity on
fs.write(fd, buf, off, len, NaN, cb).Verification
New tests in
test/js/node/fs/fs.test.ts:writeSync > treats position NaN/Infinity/-Infinity/-1/1.5 as the current file offset: writes land at the advanced cursor, not offset 0, acrosswriteSync(fd, buf, ...),fs.write(fd, buf, ...), andwriteSync(fd, string, pos).NaN,-Infinity, and1.5fail on main.createWriteStream > retries a partial write/writev at the correct offset: a customfsforces a short first write over{start: undefined, start: 0}; the retry must not passNaNand must land at the right offset.write/undefinedandwritev/0fail on main.createWriteStream > surfaces EFBIG from a short write instead of overwriting the file head(Linux): spawns the fixture underulimit -f 1024; assertserror:EFBIG, nofinish, and that the on-disk bytes are a byte-exact prefix of the source. Fails on main withhead: "DDDDDDDD", isPrefix: false.Existing
fs.write*/writeSync/createWriteStreamcoverage and the Nodetest-fs-write*/test-fs-writev*parallel scripts still pass.Overlaps #31764 on the
streams.tsretry-arithmetic piece (that PR is primarily about_writevbatching and does not touch the native position parser).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/fs/fs.test.ts