fs: short write in createWriteStream overwrites head of file (NaN position coerced to 0) - #36135
Conversation
fs.write/writev/readv coerced a NaN or -Infinity position to 0 via i52 truncation, so the bytes landed at the head of the file instead of the current offset. Node's GetOffset (src/node_file.cc) returns -1 for any non-safe-integer, selecting write()/writev() at the current offset. The visible consequence: fs.createWriteStream's writeAll retry loop computes pos = undefined + n on a short kernel write (ENOSPC, RLIMIT_FSIZE, EDQUOT), passes NaN to fs.write, and the retried tail overwrites the start of the file. The stream then emits 'finish' with bytesWritten reporting the full size, so a disk-full write is reported successful with a scrambled file. Match Node: only a safe JS integer selects positional I/O. Also keep writeAll's pos undefined across retries so custom options.fs.write implementations see the same value Node passes.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 21 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)
WalkthroughChangesFilesystem position parsing now falls back to the current file offset for invalid inputs, supports BigInt positions for buffer writes, and preserves undefined positions during stream retries. Tests cover API variants, buffer pinning, short writes, and Filesystem position and stream behavior
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 5:53 PM PT - Jul 27th, 2026
❌ @autofix-ci[bot], your commit 87f6b59 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 36135That installs a local version of the PR into your bun-36135 --bun |
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
|
Neither suggestion applies:
|
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
#36136 opened three minutes after this one with the same root fix; this PR additionally covers the shared |
|
This landed a few minutes ahead of #36136 (same brief, raced); closing that one in favour of this. One gap here: function writevAll(chunks, size, pos, cb, retries = 0) {
- this[kFs].writev(this.fd, chunks, this.pos, (er, bytesWritten, buffers) => {
+ this[kFs].writev(this.fd, chunks, pos, (er, bytesWritten, buffers) => {
...
retries = bytesWritten ? 0 : retries + 1;
size -= bytesWritten;
- pos += bytesWritten;
+ if (pos !== undefined) pos += bytesWritten;The RLIMIT_FSIZE fixture and the |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
No bugs found. This is a well-researched fix for real data corruption on short writes, but it changes user-visible fs.write/fs.readv/fs.writev position-coercion semantics (including removing a throw path for non-number readv/writev positions), so it's worth a human look at the Node-compat surface.
What was reviewed:
i52::from_jshad no other callers;i52::MINis still referenced, so the struct stays live.- The removed
buffers.release()was on the now-deleted throw path — no remaining early return afterbuffersis built, so ownership always transfers to the returned struct. writevAllsibling: its retry readsthis.pos(not the localpos), so theundefined + n → NaNshape doesn't reach it; the native fix covers it regardless.- The updated "writev keeps buffers attached" test's final
DDDDDDDDassertion was checked for Windows pwritev file-pointer semantics and ruled out.
Extended reasoning...
Overview
Fixes a data-corruption bug where a short kernel write in createWriteStream (ENOSPC/EFBIG/EDQUOT) caused the retry loop to compute pos = undefined + bytesWritten → NaN, which the native fs.write argument parser coerced to offset 0 — overwriting the head of the file while reporting success. Touches:
src/js/internal/fs/streams.ts: guardpos += bytesWrittenbehindpos !== undefinedinwriteAll.src/runtime/node/node_fs.rs: replacei52::from_js(bit-shift truncation) withi52::offset_from_jsimplementing Node'sGetOffset— only safe JS integers select positional I/O; NaN/±Infinity/fractional/out-of-range/non-number all map to current-offset. Applied to thefs.writebuffer and string overloads and the sharedreadv/writevparser. Thereadv/writevnon-number-position throw path is removed (matching Node'sif (typeof position !== 'number') position = null).test/js/node/fs/fs.test.ts: ~110 lines of new coverage across the input-shape matrix, both overloads, sync/async, readv/writev, a positive guard case, and two end-to-endcreateWriteStreamshort-write tests (one via injectedoptions.fs.write, one Linux-onlyulimit -frepro). Also updates the existing "writev keeps buffers attached" test since non-number position no longer throws.
Security risks
None identified. The change loosens coercion (values that previously mis-coerced to a specific offset now select current-offset), which is strictly less surprising and matches Node. No untrusted-input parsing or resource-limit paths are weakened.
Level of scrutiny
Medium-high. This is Node-compat argument coercion for core fs primitives — exactly the surface REVIEW.md flags for the situational Node/Web-compat guidance. The fix removes a throw (readv/writev with non-number position) and changes how fractional/oversized numeric positions are interpreted; both are intentional and Node-matched, but they are observable behavior changes that a maintainer should sign off on. The data-corruption bug being fixed is serious enough that the change is clearly worth taking, but the coercion-semantics shift deserves human eyes.
Other factors
- Bug-hunting system found no issues; three candidate concerns (EFBIG block-size assumption,
writevAllsibling, Windows pwritev file-pointer in the updated pin test) were examined and refuted. - I verified
i52::from_jshad no other callers, the removedbuffers.release()was only reachable from the deleted error path, andget_number()returnsNonefor non-numbers so the string-overload path still consumes the position slot unconditionally as before. - Test coverage is thorough and the PR description documents 27/33 new cases failing on the unfixed build.
- CI is still building; the Linux-only EFBIG test and the Windows behavior of the modified pin test will get real coverage there.
|
On the Since Bun ports Node's stream code verbatim here, I've kept |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/runtime/node/node_fs.rs (1)
2817-2829: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winNegative
positionvalues should not collapse to the current offset.
FdVectorIo::from_jscurrently turns a safe negative integer like-5intoNone, sowritev/readvsilently use the current offset instead of surfacing an invalid offset error. Preserve the number long enough to reject negative positions explicitly, matching Node’s behavior for numeric offsets.🤖 Prompt for 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. In `@src/runtime/node/node_fs.rs` around lines 2817 - 2829, Update the position parsing in FdVectorIo::from_js to preserve numeric values before converting to u64, explicitly reject negative positions with the existing invalid-offset error path, and only use None when the argument is absent. Ensure writev/readv no longer treat a supplied negative position as the current offset.
🤖 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/runtime/node/node_fs.rs`:
- Around line 3844-3851: Update the BigInt branch in the position parsing logic
to validate its range before calling to_int64(), matching the existing read-side
bounds check. Reject BigInt values outside the supported non-negative position
range, and only assign args.position after validation and conversion; preserve
the current handling for i52 offsets.
In `@test/js/node/fs/fs.test.ts`:
- Around line 3752-3753: Remove the duplicate const writes declaration in the
createWriteStream test setup, keeping a single writes array in that scope and
preserving the existing stream behavior.
---
Outside diff comments:
In `@src/runtime/node/node_fs.rs`:
- Around line 2817-2829: Update the position parsing in FdVectorIo::from_js to
preserve numeric values before converting to u64, explicitly reject negative
positions with the existing invalid-offset error path, and only use None when
the argument is absent. Ensure writev/readv no longer treat a supplied negative
position as the current offset.
🪄 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: 20161735-24be-485a-ae22-973b1e314459
📒 Files selected for processing (3)
src/js/internal/fs/streams.tssrc/runtime/node/node_fs.rstest/js/node/fs/fs.test.ts
There was a problem hiding this comment.
I didn't find issues, and my earlier note on the ulimit -f block-size assumption was addressed in 992aa17. Deferring to a human because this changes fs.write/writev/readv position semantics (including relaxing the non-number writev position from a throw to current-offset) and leaves writevAll intentionally divergent-from-fix to match Node — both are well-argued in the thread but worth a maintainer's sign-off.
Checked: i52::from_js had no other callers before the rename; get_number() returns None for non-numbers so the string-overload/writev paths correctly fall through; the removed buffers.release() is dead now that the throw path is gone; SIGXFSZ is SIG_IGN'd at startup (src/bun_bin/lib.rs:170) so the RLIMIT_FSIZE test won't die on signal.
Extended reasoning...
Overview
Three files: a one-line guard in src/js/internal/fs/streams.ts (writeAll keeps pos undefined across retries), a rewrite of i52::from_js → i52::offset_from_js in src/runtime/node/node_fs.rs implementing Node's GetOffset (safe-integer-only positional I/O) applied to the fs.write buffer/string overloads and the shared readv/writev position parser, and ~200 lines of new tests in test/js/node/fs/fs.test.ts plus an update to the existing writev buffer-pinning test.
Security risks
None new. The bug being fixed is silent data corruption (short-write tail rewritten at offset 0). The fix is strictly more conservative — invalid positions now select the non-positional syscall instead of coercing to offset 0. No new user input reaches an allocation or syscall that didn't before.
Level of scrutiny
Medium-high. This is core node:fs write-path behavior with data-integrity implications, and it deliberately changes observable semantics for edge-case position values (NaN/±Infinity/fractional/>MAX_SAFE_INTEGER now mean "current offset" instead of coercing to some integer; non-number writev positions no longer throw). The change is aligning with Node's documented GetOffset, and the PR description shows empirical strace/Node comparison, but a maintainer should confirm the relaxed-validation direction and the decision to leave writevAll unchanged (matching Node's own bug per the author's verification against Node v26.3.0).
Other factors
- I confirmed
i52::from_jshad exactly the three call sites in this diff before the rename (no orphaned callers);i52::MINremains forFTruncate. - The BigInt path on the buffer overload is byte-identical to main (
(to_int64() << 12) >> 12) after a6ccb25, so no regression there. - The removed
buffers.release()in theReadv/Writevparser is correct: the only path that needed it was the removed throw;buffersnow always reaches theOk(Self { ... })return and is released via the normal Unprotect hook. - SIGXFSZ is set to
SIG_IGNat startup (src/bun_bin/lib.rs:170), and there's a pre-existingulimit -f 1test at line ~714 relying on the same, so the new Linux RLIMIT_FSIZE test won't be killed by signal before printing. - All prior review comments (comment-cop on comment length, coderabbit on BigInt truncation and a spurious duplicate-declaration report, my ulimit block-size note) are resolved with follow-up commits.
- Test coverage is thorough: a
describe.eachmatrix over 5 invalid position values × 5 API entry points, a non-numberwritevSyncblock, a positive-control "integer position is still positional" case, and twocreateWriteStreamshort-write tests (one hermetic via customoptions.fs.write, one Linux-only via RLIMIT_FSIZE). The PR description reports 27/33 new cases fail on the unfixed build. - There is cross-PR context (supersedes #32292, races #36136, relates to #31764) that a human should be aware of when merging.
|
CI on 87f6b59 (build 83679): the only hard failure is |
…andle Per the full positional-write sweep, the only remaining divergence from Node was fs.write(fd, buf, .., 5n) which did a positional write where Node appends. Drop the BigInt branch so the buffer overload follows the same GetOffset rule as every other entry point, and replace the old 'works with bigint' test with one that asserts Node's behavior. Also extend the describe.each position matrix with 5n and add FileHandle.write/writev rows.
There was a problem hiding this comment.
I didn't find any bugs. The GetOffset port and the writeAll guard look correct and are well-covered by the new matrix. Deferring for a human sign-off on the one deliberate behavioral change: dropping Bun's BigInt-position extension on the fs.write buffer overload (a6ccb25 preserved it, 6f0ad22 dropped it again) — the final direction is documented in the description but wasn't re-acknowledged in the thread.
What was reviewed:
i52::offset_from_jsagainst Node'sIsSafeJsInt— NaN/±Inf/fractional/>MAX_SAFE_INTEGER all fall through toNone;get_number()returnsNonefor BigInt so it correctly hits current-offset.FdVectorIo::from_js: removing the throw path also removes the only early-return that neededbuffers.release(); negative safe integers now map toNoneviau64::try_frominstead of wrapping to a huge offset.- No remaining callers of the old
i52::from_js;i52::MINis still used byFTruncate. - The relaxed EFBIG assertion still fails on the pre-fix build (
ev/headdiffer), so it hasn't gone vacuous.
Extended reasoning...
Overview
Three files: a one-line guard in src/js/internal/fs/streams.ts (writeAll keeps pos undefined across retries instead of computing undefined + n → NaN), ~30 net lines in src/runtime/node/node_fs.rs replacing i52::from_js (52-bit truncation of to_int64) with i52::offset_from_js (Node's GetOffset: only Number.isSafeInteger selects positional I/O), and ~230 lines of new tests in test/js/node/fs/fs.test.ts. The Rust change flows through three call sites: the shared readv/writev position parser, the fs.write buffer overload, and the fs.write string overload.
Security risks
None. This is argument-validation tightening on node:fs position handling; the change strictly narrows the set of values that select pwrite/pwritev/preadv. The removed buffers.release() in FdVectorIo::from_js is safe because the throw path it guarded was removed in the same hunk — the Unprotect hook now always receives ownership.
Level of scrutiny
Medium-high. fs.write position parsing is on every Node-compat file write path, and the fix addresses silent data corruption (short-write retry overwriting the file head). The mechanism is well-proven — the PR description straces the write → pwrite64(_, 0) sequence and the new tests exercise 7 entry points × 6 position shapes. What tips this to defer rather than approve is the intentional user-visible change: Bun previously honored BigInt as a positional offset on the fs.write buffer overload (a Bun-only extension), and this PR drops that to match Node (typeof position !== 'number' → null). That decision was reversed once mid-PR (a6ccb25 restored the truncation after CodeRabbit flagged it, then 6f0ad22 dropped it again) and the final drop wasn't re-discussed in the thread, so a maintainer should confirm it's the intended direction.
Other factors
All prior review feedback is addressed: the comment-cop paragraph-comment nits were tightened; my earlier note on ulimit -f block-size portability was fixed in 992aa17 (assertion now checks the invariant, not the exact clamp); CodeRabbit's duplicate-declaration report was a false positive. The updated fs.writev keeps buffers attached test correctly reflects that non-number positions no longer throw and still asserts the pin/unpin invariant. writevAll was intentionally left unchanged to match Node's identical upstream behavior (documented in-thread). CI on 992aa17 was green modulo an unrelated darwin-x64 flake; the two follow-up commits (6f0ad22 BigInt drop + FileHandle test extension, 87f6b59 autofix) were still building at the last timeline update.
|
On the BigInt reversal (a6ccb25 preserved it, 6f0ad22 dropped it): the drop was in response to the follow-up sweep which lists |
|
This also fixes the other half of what const fd = fs.openSync(p, "w+");
fs.writeSync(fd, "HEADER--"); // cursor at 8
fs.writeSync(fd, Buffer.from("X"), 0, 1, 2 ** 52); // bun 1.4.0: head becomes "XEADER--" node: EFBIG (ext4) or sparse file
fs.writeSync(fd, Buffer.from("X"), 0, 1, 2 ** 52 + 3); // bun 1.4.0: "HEAXER--"
fs.writeSync(fd, Buffer.from("X"), 0, 1, 2 ** 51); // bun 1.4.0: written at the cursor (size 9)
fs.writeSync(fd, Buffer.from("X"), 0, 1, 2 ** 53 - 1); // bun 1.4.0: written at the cursor (size 9)Same through Verification: applied this diff on current main (108e412, applies cleanly) and ran The matrix in this PR only covers positions that must fall back to the current offset; nothing asserts that a large safe integer reaches |
### Problem - GitHub closes only the first reference after a keyword, so "Fixes #1, #2" leaves #2 open. "Supersedes #3" links nothing, and no reference closes a pull request. - The last 1000 merged PRs name 274 such references. PR #32292 is open although merged #36135 says "Supersedes #32292". ### Fix - `.github/workflows/close-linked-issues.yml` runs on `pull_request_target` `closed` (a merge into the default branch of `oven-sh/bun`) and on `workflow_dispatch` with a PR number and `dry_run`. Everything is inline in one `actions/github-script` step, with no checkout. - Each open target is closed as `completed` with the comment "Closed as completed by #N." or "Superseded by #N.". Closed or missing targets, the PR itself and other repositories are skipped. - The parser has no regex. A closing keyword (close, fix, resolve, supersede, replace, any tense) must lead the reference, alone or in a list. A negated, hedged or noun keyword, or one whose subject is another reference, does not count ("may fix", "the rm fix #1", "#100 supersedes #1"). - Verified: `test/internal/close-linked-issues.test.ts` (333 cases) runs the YAML's script against fake `github`, `context` and `core`. Also the 1000-PR parse (Notes). ### Background - GitHub's own keywords are close, fix and resolve (-s, -ed). Each links one reference, and only a merge into the default branch closes it. - `pull_request_target` runs in the base repository with a write token, also for fork PRs. That is safe only when no PR-controlled code runs. Here the description is the only PR input, parsed as text. <details><summary>Notes</summary> A close through the API does not create the "closed this in #N" timeline link that GitHub makes for its own closes. The comment carries the PR number instead. How the parser was calibrated. I pulled the descriptions of the last 1000 merged PRs and listed every line with a keyword next to a reference. The keyword families, list shapes and reference forms in the script are the ones that appear there. A reference is `#1`, `owner/repo#1`, an issue or pull URL (bare or in `<>`), or a markdown link. Four lines would have been wrong with a plain keyword-then-reference rule, and each led to a rule: - "the open `rm` fix #37521" (#38379): "fix" as a noun. Base forms (fix, close, resolve, supersede, replace) count only at the start of a sentence or line, or after will, should, does, and, and a few similar words. "to" is not one of them ("unable to fix #1", "how to fix #1"). - "May also fix #12318 / #10046, untested" (#38242): hedged. may, might, could, would, partially and the negations disqualify the keyword, looking past adverbs such as "also". - "Supersedes the closed #26040" (#36289) and "a comment on closed #35351" (#35365): "closed" as an adjective. A determiner or preposition before the keyword disqualifies it. - "supersedes #33130's optimisation" (#35843): a number that continues into a word is not a reference. Review added: a reference before the keyword is the subject ("#100 supersedes #1"), also through "which" or "that" ("reverts #100, which fixed #1") and across a removed span ("#100 ~~also~~ fixes #1"). A hedge two words before the keyword disqualifies it ("hopefully this fixes #1", "could this fix #1?"). A clause that starts with if, when, once, until or unless is not a statement. The tokenizer keeps a line break as a token so that "Fixes #1" on one line and "Fixes #2" on the next stay two statements. Code spans, fences, indented code, blockquotes, HTML comments and strikethrough are skipped. The block stripping follows CommonMark for fences (also inside a blockquote), indented code, blockquotes with lazy continuation, setext underlines and HTML comments, and GFM for `~~` flanking. Result over the 1000 descriptions: 274 distinct references in 135 PRs. I checked the current state of all of them through GraphQL. All but one are closed (202 issues completed, 5 duplicates, 66 pull requests). The one open target is PR #32292, superseded by merged #36135. No open target is a false positive. Every review change kept this result. Patterns that are deliberately not handled: a bulleted list under "Closes:" on its own line (not seen in the sample), references separated by whitespace only ("#1 #2"), "fix for #1", and GH-1 style references. A `?` after the list is not treated as a question. The block parser tracks no list containers, so a second paragraph of a list item indented by four spaces is read as an indented code block and skipped. A removed span or inline comment reads as one word, so "Fixes <!-- n --> #1" finds nothing. The test suite covers: the phrases above, stopping at the right place in real sentences, CRLF descriptions, URLs with fragments or a `/files` suffix, case-insensitive `Owner/Repo#1`, the fake API where a lookup, an update or a comment fails, the `dry_run` input, an invalid `pr_number` input, an unmerged PR, a PR merged into a non-default branch, the merge event body against a later edit, and a description with no closing statement. The first revision of this PR checked out the repository and ran `scripts/close-linked-issues.ts`. Jarred asked for no checkout and no script file, so the script moved inline into the workflow and the test now reads it out of the YAML. </details> <!-- robobun:evidence:begin --> --- **[stamp-90s]** gate passed · iteration 9 · 2 files touched <details><summary>passes on PR (with fix)</summary> ```console Test-only change. Debug/ASAN (expected pass): $ bun bd test 'test/internal/close-linked-issues.test.ts' $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test test/internal/close-linked-issues.test.ts bun test v1.4.1 (4448a2e) test/internal/close-linked-issues.test.ts: (pass) finds "Fixes #39852" [176.21ms] (pass) finds "Closes #31772. Fixes #31771." [22.28ms] (pass) finds "- Fixes #39930" [12.28ms] (pass) finds "Fixes: #30429" [10.46ms] (pass) finds "FIXES #1" [7.86ms] (pass) finds "(Fixes #1)" [8.97ms] (pass) finds "**Fixes #1**" [10.20ms] (pass) finds "__Fixes #1__" [9.83ms] (pass) finds "_Fixes #1_" [11.25ms] (pass) finds "Fixes **#1**" [9.72ms] (pass) finds "**Fixes** #1" [7.13ms] (pass) finds "**Fixes:** #1" [8.11ms] (pass) finds "Fixes #1 and **#2**" [11.47ms] (pass) finds "Fixes **#1**, **#2**" [9.13ms] (pass) finds "## Why (fixes #13771, closes #30543)" [16.08ms] (pass) finds "Closes #11418" [19.46ms] (pass) finds "Resolves #1. Resolved #2. Resolve #3." [12.09ms] (pass) finds "Fixes #34055, #30327, #24394, #20816, #32403, #11898, #10056." [17.11ms] (pass) finds "Fixes #18192 and #31675 as a consequence" [10.45ms] (pass) finds "Fixes #1, #2, and #3" [10.96ms] (pass) finds "Fixes #1 & #2" [7.63ms] (pass) finds "Closes #33280, Closes #32864 and Closes #29696 (the timer in #32949 is orthogonal)" [20.29ms] (pass) finds "Closes #33182 and #32947 on top of current main (which already has #36304 for catalogs)." [16.12ms] (pass) finds "Fixes #1,\n#2" [7.76ms] (pass) finds "Fixes #1, #2,\nand #3" [9.27ms] (pass) finds "Fixes #1\nand #2" [8.57ms] (pass) finds "Fixes #1\n& #2" [6.80ms] (pass) finds "Fixes #1 and\n#2" [7.31ms] (pass) finds "Supersedes #39908 (same change, moved from a fork branch)" [13.21ms] (pass) finds "Supersedes #38778 and #38391. Carries the entry point arm of #35053." [14.43ms] (pass) finds "Supersedes #39193 and keeps its three tests." [11.48ms] (pass) finds "This supersedes #33306 and #32803. Their tests are kept here." [13.73ms] (pass) finds "- This replaces #33793. Its ... (truncated) Exit: 0 ``` </details> <details><summary>diff hotspot</summary> ``` .github/workflows/close-linked-issues.yml | 950 ++++++++++++++++++++++++++++++ test/internal/close-linked-issues.test.ts | 598 +++++++++++++++++++ 2 files changed, 1548 insertions(+) ``` </details> **gate history** · 29 passed · 0 rejected · iteration 9 <details><summary>evidence per changed file</summary> ``` file reads edits tests .github/workflows/close-linked-issues.yml 6 12 0 test/internal/close-linked-issues.test.ts 3 11 0 ``` </details> <!-- robobun:evidence:end -->
Stop shadowing the prototype _writev with an own-property undefined on the default fs path, so buffered chunks coalesce into one fs.writev per drain cycle like Node. Disable _writev only when a custom options.fs has no writev. writevAll passed this.pos instead of its pos parameter, so a partial write retry with `start` set resumed at the wrong offset. Guard `pos += bytesWritten` against an undefined pos (current file offset) so the retry never passes NaN. Rebased onto main: the writeAll NaN guard landed separately in #36135, and the IOV_MAX chunking in node:fs landed in #33695.
2 participants
Reproduction
Any short write from the kernel (disk full /
ENOSPC, quota /EDQUOT,RLIMIT_FSIZE) triggers this: the unwritten tail is re-written at offset 0, the head of the file is destroyed, and the stream emits'finish'withbytesWrittenreporting the full size.The primitive without
createWriteStream:Cause
writeAllinsrc/js/internal/fs/streams.tsretries a short write withpos += bytesWritten.posstarts asundefined(nostartoption) so the retry passesNaN. Node'swriteAlldoes the same; it works there because Node's nativeGetOffset(src/node_file.cc) returns-1(current file offset) for any value that isn't a safe JS integer.Bun's
fs.writeargument parser instead rani52::from_js(NaN), which is(to_int64(NaN) << 12) >> 12 = 0, and setargs.position = Some(0). strace confirms:write(4 MiB) = 1048576, thenpwrite64(3 MiB, 0),pwrite64(2 MiB, 0),pwrite64(1 MiB, 0); the remaining size reaches zero so the loop reports success.-Infinity, fractional values, and numbers pastMAX_SAFE_INTEGERmis-coerced the same way, and thefs.readv/fs.writevpositionparser had the same defect (plus it threw on non-number values Node accepts).Fix
i52::offset_from_jsnow implements Node'sGetOffset: apositionselectspwrite/pwritev/preadvonly whenNumber.isSafeInteger(position). Every other shape (NaN, ±Infinity, fractional, out-of-range, non-number) leavesposition = Noneso dispatch picks the non-positional syscall at the current offset. Thefs.writebuffer and string overloads and the sharedreadv/writevparser all go through it. This includes BigInt, which Bun previously honored as a positional write on the buffer overload; thewriteSync works with biginttest now asserts the Node behavior (current offset).writeAllalso now keepsposundefined across retries, so a customoptions.fs.writeobserves the same argument Node passes.Verification
New
describe.eachintest/js/node/fs/fs.test.tsrunswriteSync(buffer and string), asyncfs.write,writevSync,readvSync,FileHandle.write, andFileHandle.writevagainst each ofNaN,±Infinity,1.5,MAX_SAFE_INTEGER + 1, and5nand asserts the I/O happens at the current offset; a second block covers non-numberwritevSyncpositions. TwocreateWriteStreamtests cover the headline bug: one simulates a short write viaoptions.fs.writeand asserts the retry position staysundefinedand the file readsAAAABBBBCCCCDDDD; one (Linux) runs theulimit -frepro and asserts{ev: ["error:EFBIG"], bytesWritten: 1048576, head: "AAAAAAAA"}.40 of the 49 new cases fail on the unfixed build (the others are guard cases or +Infinity which already happened to truncate to
-1). All pass with this change, along with the existingwriteSync/writev/readvsuites and Node'stest-fs-writev*/test-fs-readv*parallel tests.Supersedes #32292 (the
readv/writevhalf of this same class).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/node/fs/fs.test.ts