Conversation
|
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)
Walkthrough
ChangesNegative position fix for scatter/gather I/O
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
Updated 1:43 PM PT - Jul 16th, 2026
❌ @autofix-ci[bot], your commit 31e8a57 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 32292That installs a local version of the PR into your bun-32292 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — small, well-tested fix that mirrors the existing position_int >= 0 gate in args::Read/args::Write.
Extended reasoning...
Overview
This PR fixes fs.readv/fs.writev (sync and async) to treat a negative position argument as "use the current file position" instead of passing a wrapped u64::MAX to preadv/pwritev and failing with EINVAL. The change is a single logic edit in FdVectorIo::from_js in src/runtime/node/node_fs.rs — replacing pos_value.to_int64() as u64 with a sign check that only sets position = Some(..) when the value is non-negative — plus four new regression tests in test/js/node/fs/fs.test.ts.
Security risks
None. This is argument parsing for vectored file I/O on an already-open fd. The change narrows behavior (negative → current position, matching Node/libuv) rather than expanding any capability, and there's no auth, crypto, or untrusted-input surface involved.
Level of scrutiny
Low. The effective code change is a few lines and is a direct copy of the established pattern already used for args::Read/args::Write in the same file (the position_int >= 0 gate at ~line 4062). Negative positions now leave position as None, so dispatch correctly selects readv/writev instead of preadv/pwritev. Non-negative positions are unchanged.
Other factors
The four new tests cover readvSync, writevSync, fs.readv, and fs.writev with position: -1, asserting both byte counts and resulting buffer/file contents, and additionally verify the file cursor advances across successive calls. No CODEOWNERS apply to the touched files, the bug-hunting system found no issues, and there are no outstanding reviewer comments.
There was a problem hiding this comment.
LGTM — straightforward Node-compat fix mirroring the existing position_int >= 0 gate in args::Read/args::Write.
Extended reasoning...
Overview
This PR changes one branch in FdVectorIo::from_js (src/runtime/node/node_fs.rs), the shared argument parser for fs.readv/fs.writev and their sync variants. Previously a negative position was cast to_int64() as u64, wrapping to a huge value and routing to preadv/pwritev, which then failed with EINVAL. The fix only sets position = Some(..) when the parsed int is non-negative, leaving it None otherwise so dispatch picks the non-positional readv/writev path. Four new tests in test/js/node/fs/fs.test.ts cover readvSync, writevSync, fs.readv, and fs.writev with position: -1.
Security risks
None. This is pure argument-parsing for a file descriptor the caller already holds; the change narrows behavior from "error" to "use current file position", matching Node/libuv semantics. No new syscall surface, no auth/crypto/permissions involved.
Level of scrutiny
Low. The functional change is ~4 lines and is a direct copy of the pattern already used at line ~4062 in the same file for args::Read/args::Write (if position_int >= 0 { Some(..) } else { None }). The PR description accurately traces the root cause and the fix is the obvious one.
Other factors
- No CODEOWNERS cover the touched files.
- Bug hunting system found no issues.
- Tests are well-constructed: they advance the fd cursor with
readSync(..., null)first, then assert that-1reads/writes from that cursor and (for readv) that a second call continues advancing — verifying both thatEINVALis gone and that the semantics match Node. - No outstanding human review comments on the PR.
|
CI status: the diff is green on every lane except one unrelated ASAN crash. Build #74036 (current, at 31e8a57): the only failure is |
FdVectorIo::from_js (the shared args parser for fs.readv/writev,
readvSync/writevSync, and FileHandle.readv/writev) diverged from Node
on two position inputs:
- A negative number was passed through as-is and wrapped to a huge
u64, routing to preadv(2)/pwritev(2) with a negative offset and
failing with EINVAL. libuv treats any offset < 0 as non-positional
readv/writev (i.e. current file offset).
- A non-number value threw ERR_INVALID_ARG_TYPE. Node's JS layer
coerces typeof position !== 'number' to null (current offset) and
never rejects.
Either input meant the write silently never happened on Bun for code
ported from Node.
Only a non-negative number now selects preadv/pwritev; every other
shape (undefined/null/negative/string/object/boolean/bigint) leaves
position = None so dispatch picks readv/writev at the current offset,
matching Node and libuv. This removes the only error return after
buffers are constructed, so the buffers.release() cleanup on that path
is dead and removed along with it.
Tests cover writevSync / readvSync / callback writev / callback readv /
FileHandle.writev / FileHandle.readv across -1, -5, "3", {}, true and
3n, plus a guard that a non-negative number position is still
positional. The fs.writev buffer-pinning test is updated: a non-number
position no longer throws, so it now asserts the call succeeds and the
pin is released on completion.
60b4ecf to
67acef5
Compare
robobun
changed the title
Node's native GetOffset() returns -1 (non-positional) unless IsSafeJsInt(value) holds: finite, integral, and within Number.MAX_SAFE_INTEGER. The previous is_number()/to_int64() path let NaN (-> 0), +Infinity (-> i64::MAX), fractional values, and out-of-range doubles through as positional offsets. Gate on the same three conditions Node does. Also restore the per-write disk-content assertion in the writev buffer-pin test: the second (non-positional) write lands at offset 0 and overwrites the first write's bytes, so check the file contents between the two writes.
There was a problem hiding this comment.
LGTM — both earlier nits are addressed (the CCCCCCCC disk read now sits between the two writevs, and the position gate now mirrors Node's IsSafeJsInt via get_number() + finite/integral/≤MAX_SAFE_INTEGER, with NaN/±Infinity/3.7/MAX_SAFE_INTEGER+1 added to the matrix).
Extended reasoning...
Overview
The PR rewrites the position parser in FdVectorIo::from_js (src/runtime/node/node_fs.rs), which is shared by every fs.readv/fs.writev entry point (sync, callback, and FileHandle). Previously a negative number was cast to u64 and sent to preadv/pwritev (kernel EINVAL), and any non-number threw ERR_INVALID_ARG_TYPE. Now only a value for which get_number() returns a non-negative, integral f64 ≤ MAX_SAFE_INTEGER selects positional I/O; every other shape leaves position = None and dispatches non-positional readv/writev. This mirrors Node's lib/fs.js (typeof position !== 'number' → null) plus native GetOffset()'s IsSafeJsInt gate. The now-unreachable buffers.release() error path and the mut on buffers are removed. Tests add a 66-case describe.each matrix over 11 position values × 6 entry points, plus a positive control that a non-negative integer is still positional, and update the "keeps buffers attached" test to reflect that a string position now succeeds.
Security risks
None. This loosens argument validation on a local file-descriptor operation to match Node — inputs that previously threw or EINVAL'd now fall back to non-positional I/O on an fd the caller already opened. get_number() is a pure tag check + bit extraction (no JS coercion, no getters/Proxy traps), so there's no re-entrancy between buffers construction and the return. No untrusted-length arithmetic, no new allocation, no path handling.
Level of scrutiny
Medium-low. The native change is ~10 lines in a single function, replaces a lossy as u64 cast with an explicit range gate, and removes a dead error path. NaN >= 0.0 is false, Infinity <= MAX_SAFE_INTEGER is false, and 3.7.trunc() != 3.7, so all the newly-added matrix values correctly fall through to None. The sibling args::Read/args::Write parsers already gate on >= 0 the same way, so this brings FdVectorIo in line with existing precedent in the file rather than introducing a novel pattern.
Other factors
Both nits from my prior review were addressed in e64468d: (1) the readFileSync == "CCCCCCCC" assertion now sits between the two writevs so the pwritev-path disk bytes are still verified before the non-positional write overwrites offset 0, and (2) the is_number()+to_int64() gate that let NaN→0 and +Infinity→i64::MAX slip through was replaced with the IsSafeJsInt-equivalent check, with those inputs added to the test matrix. Verifier agents separately examined whether the modified "keeps buffers attached" test's final DDDDDDDD assertion would fail on Windows (where libuv positional writes advance the fd offset) and ruled it out. The bug-hunting system found nothing on the current revision.
|
The |
…ition coerced to 0) (#36135) ## Reproduction ```sh $ cat repro.mjs import fs from "node:fs"; import os from "node:os"; import { spawnSync } from "node:child_process"; const MB = 1 << 20; if (process.argv[2] !== "child") { const r = spawnSync("sh", ["-c", `ulimit -f 2048; exec "${process.execPath}" "${process.argv[1]}" child`], { stdio: "inherit" }); process.exit(r.status ?? 1); } const p = `${os.tmpdir()}/pw-${process.pid}.bin`; const big = Buffer.concat([..."ABCD"].map(c => Buffer.alloc(MB, c))); const s = fs.createWriteStream(p), ev = []; s.on("error", e => ev.push("error:" + e.code)).on("finish", () => ev.push("finish")); s.on("close", () => { const b = fs.readFileSync(p); console.log(JSON.stringify({ ev, bytesWritten: s.bytesWritten, size: b.length, head: b.subarray(0, 8).toString() })); }); s.write(big); s.end(); $ node repro.mjs {"ev":["error:EFBIG"],"bytesWritten":1048576,"size":1048576,"head":"AAAAAAAA"} $ bun repro.mjs # before {"ev":["finish"],"bytesWritten":4194304,"size":1048576,"head":"DDDDDDDD"} ``` 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'` with `bytesWritten` reporting the full size. The primitive without `createWriteStream`: ```js const fd = fs.openSync(p, "w"); fs.writeSync(fd, "AAAAAAAAAA"); fs.writeSync(fd, Buffer.from("XX"), 0, 2, NaN); // node: "AAAAAAAAAAXX" bun (before): "XXAAAAAAAA" ``` ## Cause `writeAll` in `src/js/internal/fs/streams.ts` retries a short write with `pos += bytesWritten`. `pos` starts as `undefined` (no `start` option) so the retry passes `NaN`. Node's `writeAll` does the same; it works there because Node's native `GetOffset` ([src/node_file.cc](https://github.com/nodejs/node/blob/main/src/node_file.cc)) returns `-1` (current file offset) for any value that isn't a safe JS integer. Bun's `fs.write` argument parser instead ran `i52::from_js(NaN)`, which is `(to_int64(NaN) << 12) >> 12 = 0`, and set `args.position = Some(0)`. strace confirms: `write(4 MiB) = 1048576`, then `pwrite64(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 past `MAX_SAFE_INTEGER` mis-coerced the same way, and the `fs.readv`/`fs.writev` `position` parser had the same defect (plus it threw on non-number values Node accepts). ## Fix `i52::offset_from_js` now implements Node's `GetOffset`: a `position` selects `pwrite`/`pwritev`/`preadv` only when `Number.isSafeInteger(position)`. Every other shape (NaN, ±Infinity, fractional, out-of-range, non-number) leaves `position = None` so dispatch picks the non-positional syscall at the current offset. The `fs.write` buffer and string overloads and the shared `readv`/`writev` parser all go through it. This includes BigInt, which Bun previously honored as a positional write on the buffer overload; the `writeSync works with bigint` test now asserts the Node behavior (current offset). `writeAll` also now keeps `pos` undefined across retries, so a custom `options.fs.write` observes the same argument Node passes. ## Verification New `describe.each` in `test/js/node/fs/fs.test.ts` runs `writeSync` (buffer and string), async `fs.write`, `writevSync`, `readvSync`, `FileHandle.write`, and `FileHandle.writev` against each of `NaN`, `±Infinity`, `1.5`, `MAX_SAFE_INTEGER + 1`, and `5n` and asserts the I/O happens at the current offset; a second block covers non-number `writevSync` positions. Two `createWriteStream` tests cover the headline bug: one simulates a short write via `options.fs.write` and asserts the retry position stays `undefined` and the file reads `AAAABBBBCCCCDDDD`; one (Linux) runs the `ulimit -f` repro 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 existing `writeSync`/`writev`/`readv` suites and Node's `test-fs-writev*`/`test-fs-readv*` parallel tests. Supersedes #32292 (the `readv`/`writev` half of this same class). <!-- robobun:evidence:begin --> --- **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 <!-- robobun:evidence:end --> --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
### 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 -->
|
Superseded by #40064. |
1 participant
Reproduction
Same divergence on
readvSync, callbackfs.readv/fs.writev, andFileHandle.readv/FileHandle.writev.Cause
FdVectorIo::from_js(the sharedpositionparser for everyfs.readv/fs.writeventry point insrc/runtime/node/node_fs.rs) diverged from Node on two inputs:u64and kept asSome(..), routing topreadv(2)/pwritev(2)with a negative offset, which the kernel rejects withEINVAL. libuv'suv__fs_read/uv__fs_writetreat anyoffset < 0as plain non-positionalreadv/writev.ERR_INVALID_ARG_TYPE. Node's JS layer (lib/fs.jswritev/writevSync/readv/readvSync, andinternal/fs/promises.jsFileHandle#writev/readv) doesif (typeof position !== 'number') position = null;and never rejects.Either idiom meant Node code that uses
position: -1("current offset", a documented libuv convention) or passes a stray non-number silently wrote nothing on Bun.Fix
Only a non-negative number now selects
preadv/pwritev. Every other shape (undefined,null, negative number, string, object, boolean, bigint) leavesposition = Noneso dispatch picksreadv/writevat the current file offset, matching Node and libuv. This removes the only error return afterbuffersis constructed, so thebuffers.release()cleanup on that path is dead and removed along with it. The siblingargs::Read/args::Writeparsers already gate onposition >= 0the same way.Verification
New
describe.eachblock intest/js/node/fs/fs.test.tsrunswritevSync/readvSync/ callbackfs.writev/ callbackfs.readv/FileHandle.writev/FileHandle.readvagainst each of-1,-5,"3",{},true,3n, asserting the read/write happens at the current file offset and advances it (36 cases). A guard test confirms a non-negative number position is still positional. All 36 matrix cases fail on the unfixed build (EINVALfor negatives,ERR_INVALID_ARG_TYPEfor non-numbers) and pass with this change.The
fs.writev keeps buffers attached while the write is in flighttest previously asserted that a string position throws; it now asserts the call succeeds and the buffer pin is released after completion, matching Node.Existing
readv/writev/preadv/pwritevtests and the Node paralleltest-fs-readv*/test-fs-writev*suites pass unchanged.no test proof · iteration 6 · 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