fs.readFile: honor AbortSignal between read chunks

[robobun]

Reproduction

import fsp from "node:fs/promises";
import fs from "node:fs";
import os from "node:os";
import path from "node:path";

const MB = 1048576, SZ = 384 * MB;
const big = path.join(fs.mkdtempSync(path.join(os.tmpdir(), "rf-")), "big.bin");
{ const fd = fs.openSync(big, "w"); const b = Buffer.alloc(8*MB, 1);
  for (let i = 0; i < SZ / b.length; i++) fs.writeSync(fd, b); fs.closeSync(fd); }
const rchar = () => Number(/rchar: (\d+)/.exec(fs.readFileSync("/proc/self/io","utf8"))[1]);
const c = new AbortController(), r0 = rchar();
const p = fsp.readFile(big, { signal: c.signal });
await new Promise(r => setTimeout(r, 15)); c.abort(new Error("stop"));
try { await p; } catch (e) { console.log("rejected", e.name, e.code); }
console.log("readMBafterAbort", Math.round((rchar() - r0) / MB), "of", SZ / MB);
// bun : readMBafterAbort 384 of 384
// node: readMBafterAbort 0-15 of 384

The promise rejects with AbortError/ABORT_ERR in both runtimes, but Bun has already read the entire file into the process: readFile(bigLog, { signal: AbortSignal.timeout(200) }) bounds neither latency nor IO. This is the read face of #35021.

Cause

read_file_with_options in src/runtime/node/node_fs.rs does check args.aborted() at the top of each loop iteration, but each iteration asks for the whole remaining file:

let upper = (buf.capacity() as u64).min(max_size) as usize;   // stat size
let amt = Syscall::read(fd, &mut buf[total..upper])?;         // one read() for ~all of it

On a regular file a single read() can return the full request, so the file-descriptor position jumps 256 KiB (pre-stat probe) straight to the stat size in one syscall, and the abort check is only consulted again after the whole file has been read. FIFOs abort promptly because each read() there returns a small burst and the loop iterates.

Fix

When a signal is supplied, cap each read() at a bound that starts at 512 KiB and doubles every iteration, so the existing args.aborted() check runs between reads. An unaborted read pays O(log n) read() calls (about 10 for a 384 MB file) instead of O(n) at Node's fixed kReadFileBufferLength, and the first few reads remain small so an early abort is observed promptly. Without a signal the fast path is unchanged: the whole remaining buffer is still handed to read() on each iteration.

The callback form (fs.readFile(path, { signal }, cb)) and FileHandle.readFile({ signal }) route through the same function and are fixed as well.

Verification

New test in test/js/node/fs/promises.test.js opens a sparse 512 MiB file, starts readFile(fd, { signal }), busy-polls /proc/self/fdinfo/<fd> from the JS thread until the worker has advanced past the 256 KiB pre-stat read (bounded by a 4 s deadline), then aborts and asserts the final file position is strictly below the file size. f_pos is updated when each read() returns, so a single whole-file read() jumps 256 KiB to 512 MiB (fails the assertion) while the bounded loop crosses the threshold at ~768 KiB and stops within a few chunks of the abort.

fail-before / pass-after

Without the src change:

error: expect(received).toBeLessThan(expected)
Expected: < 536870912
Received: 536870912
(fail) readFile aborted mid-read stops before the full file is read

With the src change (5/5):

(pass) readFile aborted mid-read stops before the full file is read

---

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/promises.test.js

[coderabbitai]

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 14 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: ef83afe0-f53f-4ab7-bf2b-a377001bdd43

📥 Commits

Reviewing files that changed from the base of the PR and between 2638450 and 1d78baa.

📒 Files selected for processing (2)

• src/runtime/node/node_fs.rs
• test/js/node/fs/promises.test.js

Walkthrough

Signal-aware fs.promises.readFile operations now read in 512 KiB chunks, allowing abort checks between iterations. A Linux-only test verifies mid-stream abortion, Node-compatible error fields, and a file position below the complete file size.

Changes

Signal-aware file reading

Layer / File(s)	Summary
Bound signal-aware read iterations src/runtime/node/node_fs.rs	Signal-aware reads use a 512 KiB per-iteration limit while preserving existing capacity and maximum-size bounds.
Validate mid-stream abortion test/js/node/fs/promises.test.js	A Linux-only test aborts an in-progress large read and verifies the AbortError contract and partial read position.

Suggested reviewers: jarred-sumner, dylan-conway

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title clearly summarizes the main change: fs.readFile now respects AbortSignal between chunks.
Description check	✅ Passed	It explains the change and includes verification details, though it uses custom section names instead of the template headings.

---

_{Comment @coderabbitai help to get the list of available commands.}

[robobun]

^{Updated 10:01 PM PT - Jul 28th, 2026}

✅ @robobun, your commit 1d78baa79e8825bd4ddc3b14bc64f898ef8af07b passed in Build #84525! 🎉

---

🧪   To try this PR locally:

bunx bun-pr 36259

That installs a local version of the PR into your bun-36259 executable, so you can run:

bun-36259 --bun

[coderabbitai]

Actionable comments posted: 1

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)

7274-7292: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Add an abort check to the 256 KiB pre-stat read loop (src/runtime/node/node_fs.rs:7140-7152).

That loop can iterate multiple times on short reads, but it never re-checks args.aborted() between Syscall::read calls. On pipes/FIFOs/devices — and occasional short reads from regular files — an aborted request can keep reading until the buffer is exhausted before the next check.

🤖 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 7274 - 7292, Add an args.aborted()
check inside the 256 KiB pre-stat read loop, immediately before each
Syscall::read call, returning abort_err() when cancellation is detected. Ensure
repeated short-read iterations re-check cancellation without changing the loop’s
existing read and buffer behavior.

🤖 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/promises.test.js`:
- Around line 472-518: Adjust the readFile aborted mid-read test timeout around
the test declaration and its 10-second polling deadline so the test’s configured
timeout exceeds the maximum fallback wait. Keep the existing diagnostic error
path and polling behavior unchanged, ensuring slow CI reaches the intended
failure instead of the runner’s generic timeout.

---

Outside diff comments:
In `@src/runtime/node/node_fs.rs`:
- Around line 7274-7292: Add an args.aborted() check inside the 256 KiB pre-stat
read loop, immediately before each Syscall::read call, returning abort_err()
when cancellation is detected. Ensure repeated short-read iterations re-check
cancellation without changing the loop’s existing read and buffer behavior.

🪄 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: 7951ed73-1cd8-4f60-bf8d-b627dfa77e44

📥 Commits

Reviewing files that changed from the base of the PR and between e532ad9 and 2638450.

📒 Files selected for processing (2)

• src/runtime/node/node_fs.rs
• test/js/node/fs/promises.test.js

[robobun]

On the outside-diff suggestion (abort check inside the 256 KiB pre-stat loop): leaving that out of this PR. That loop is hard-bounded at 256 KiB and is immediately followed by an args.aborted() check before the main loop starts; for regular files it is a single read(). The reported bug is the main loop issuing a stat-sized read() for the whole file, which is what this change addresses. The report also measured a slowly-fed FIFO aborting promptly (~120 ms) on both runtimes, so the pre-stat path is not where the latency lives.

[claude]

LGTM — the src change is minimal and correct, and all prior review feedback has been addressed.

What was reviewed:

• The chunk cap only activates when args.signal.is_some(); the no-signal fast path is byte-identical to before.
• total.saturating_add(READ_FILE_CHUNK_SIZE) is always ≥ total, so the new .min() cannot introduce an upper < total slice panic; the phase-1 growth arm's 8 KiB capacity bound is already tighter than 512 KiB, so that path is unaffected.
• 512 KiB matches Node's kReadFileBufferLength from lib/internal/fs/utils.js.
• Test: 4 s bounded poll (below the 5 s default), sparse 512 MiB file, fd closed in finally, deadline branch aborts+awaits before throwing.

Extended reasoning...

Overview

Two files changed. src/runtime/node/node_fs.rs adds a 512 KiB per-iteration cap on the read() slice inside read_file_with_options's main loop, gated on args.signal.is_some(), so the existing args.aborted() check at the top of each iteration actually runs between chunks on regular files (previously a single read() returned the whole stat-sized file). test/js/node/fs/promises.test.js adds a Linux-only test that starts a readFile of a sparse 512 MiB file, busy-polls /proc/self/fdinfo/<fd> until the worker advances past the 256 KiB pre-stat probe, aborts, and asserts the final file position is strictly below the file size.

Security risks

None. The change only narrows the slice handed to Syscall::read; no new user input is parsed, no bounds are widened, and the saturating add cannot underflow the slice range.

Level of scrutiny

Medium. readFile is a hot Node-compat path, but the change is 7 lines, activates only when a signal is supplied, and matches Node's kReadFileBufferLength exactly. The no-signal path is unchanged. I traced the slice arithmetic: upper = min(capacity, max_size, total + 512K) — the third term is always ≥ total, so buf[total..upper] cannot newly panic. In the phase-1 tail (stat size overshot), capacity grows in 8 KiB steps which is already tighter than 512 KiB, so the chunk cap is a no-op there.

Other factors

I reviewed this PR twice previously. My first review flagged the unbounded synchronous busy-poll and the race window with a page-cached 128 MiB file; the author addressed both in 2638450 (4 s performance.now() deadline, sparse 512 MiB file → 1024 chunks, re-verified fail-before 3/3 / pass-after 5/5). My second review and CodeRabbit's both flagged the 10 s deadline exceeding the 5 s default test timeout; that was already fixed in 6b988f6 (lowered to 4 s). All inline threads are resolved. The remaining timing sensitivity in the test is inherent to observing a threadpool worker from the JS thread; the author widened the margin as far as the should_throw_out_of_memory_early_for_javascript guard allows (512 MiB is the practical ceiling), and the deadline branch cleans up the fd before throwing. The test transiently allocates a 512 MiB buffer, which is within CI runner budgets and freed immediately on abort.

[Jarred-Sumner]

Do not use a fixed chunk size.

[robobun]

Switched to a growing bound in f8d7149: the cap starts at 512 KiB and doubles each iteration, so an unaborted read pays O(log n) read() calls instead of O(n) at Node's fixed kReadFileBufferLength, while an early abort is still observed within the first few small reads. The no-signal path is unchanged (upper is not touched when args.signal.is_none()).

For the 384 MB file in the repro that is ~10 read() calls instead of 768; for a file large enough to saturate usize the cap saturates and falls out of the .min(), which is equivalent to no cap.

Happy to tune the starting size or add an upper cap if you had a different shape in mind.

[claude]

No findings this pass; all three earlier inline concerns (unbounded busy-poll, 10 s vs 5 s local deadline, stale chunk-count comment) are addressed at 1d78baa. Deferring to a maintainer for the one thing the author explicitly left open — whether the doubling per-read cap (O(log n) syscalls, last chunk up to ~N/2) is preferred over Node's fixed kReadFileBufferLength, or whether it wants an upper bound.

What was reviewed:

• read_file_with_options loop: no-signal path is byte-identical (upper untouched); signal path uses saturating_add/saturating_mul so the cap saturates and falls out of the .min(), and signal_chunk >= 512K guarantees upper > total whenever the pre-existing termination condition doesn't already hold — no premature amt == 0 exit.
• Test: 4 s bounded sync poll with abort+await on the deadline path so the fd is released before throwing; wall-clock margin is governed by the 512 MiB total copy, not the ~11 abort windows, so the doubling change did not reduce it.
• Checked the total > size && !has_max_size growth branch — continue still passes through the top-of-loop args.aborted() check, and signal_chunk doubles once per iteration on that path too.

Extended reasoning...

Overview

The PR changes 8 lines in read_file_with_options (src/runtime/node/node_fs.rs): when args.signal.is_some(), cap each Syscall::read slice at total + signal_chunk where signal_chunk starts at 512 KiB and doubles per iteration, so the existing top-of-loop args.aborted() check is consulted between reads instead of once after a stat-sized whole-file read. The no-signal branch does not touch upper and is unchanged. A Linux-only test in test/js/node/fs/promises.test.js opens a sparse 512 MiB file, busy-polls /proc/self/fdinfo/<fd> from the JS thread until f_pos > 256 KiB (bounded at 4 s), aborts, and asserts f_pos < SIZE plus the Node AbortError/ABORT_ERR shape.

Security risks

None. No user-controlled input reaches new code paths; the only new state is a local usize counter guarded by saturating arithmetic. The test's /proc/self/fdinfo/${fd} read is a numeric fd from openSync in the same process — not attacker-controlled.

Level of scrutiny

Medium. readFile is a hot path, but the change is gated on has_signal and the fast path is provably unchanged. The saturating math means once signal_chunk overflows it becomes usize::MAX, total.saturating_add(usize::MAX) == usize::MAX >= (buf.capacity()).min(max_size), and the added .min() becomes a no-op — equivalent to no cap. Because signal_chunk >= 512*1024 > 0, total.saturating_add(signal_chunk) > total always holds, so the new cap can never produce an empty read slice that the pre-existing upper = min(cap, max_size) wouldn't already have produced; the loop's amt == 0 termination is unaffected.

Other factors

I have already left three inline reviews on this PR and each was addressed by a follow-up commit (2638450, 6b988f6, 1d78baa). The bug hunting system found nothing this run. Two things keep me from approving outright:

1. The author explicitly asked for maintainer input on the cap schedule ("Happy to tune the starting size or add an upper cap if you had a different shape in mind"). Doubling means the last read before abort can be up to half the file, whereas Node bounds it at 512 KiB — a deliberate perf/responsiveness tradeoff a human should ratify.
2. The test reserves a real ~512 MiB buffer (try_reserve_exact(initial_cap)) on the worker thread and relies on the JS thread not being descheduled for the full ~512 MiB kernel→user copy window. The margin analysis is sound (wall-clock, not syscall count, governs it), but it is unusual enough that a maintainer should be aware before it lands in the Linux CI lane.

[claude]

No further issues from my side — all three earlier inline threads are addressed and the bug-hunting pass found nothing on 1d78baa. Deferring for a human to sign off on the doubling-cap schedule (you left "happy to tune the starting size or add an upper cap" open, and it does trade differently from Node's fixed 512 KiB — a late abort can still read a ~256 MiB chunk) and on the Linux-only test's residual scheduling dependency, since readFile is a hot path.

What was reviewed

• upper cap arithmetic: total.saturating_add(signal_chunk) cannot go below total, so buf[total..upper] never inverts; saturated signal_chunk falls out of the .min() and degenerates to no cap.
• No-signal path unchanged; the empty-slice EOF exit (total == min(cap, max_size)) still holds under the new .min().
• Growth-tail (total > size) branch: signal_chunk doubles per iteration alongside RawVec doubling, abort check still runs each pass.
• Test: 4 s poll deadline < 5 s default; sparse 512 MiB via truncateSync; fd closed in finally; comment now schedule-agnostic.

Extended reasoning...

Overview

Two files: an 8-line change in src/runtime/node/node_fs.rs::read_file_with_options that, when an AbortSignal is attached, caps each read() at a bound starting at 512 KiB and doubling per iteration so the existing args.aborted() check at the top of the loop runs between chunks; and a ~45-line Linux-only test in test/js/node/fs/promises.test.js that uses /proc/self/fdinfo/<fd> to observe the worker's file position, aborts once it passes the 256 KiB pre-stat probe, and asserts the final position is strictly below the 512 MiB file size.

Security risks

None. No user-controlled input feeds the new arithmetic (signal_chunk is a local constant that only grows via saturating_mul); the change only shrinks per-call read spans and cannot enlarge the buffer or the read window beyond what the existing (buf.capacity()).min(max_size) bound already allowed.

Level of scrutiny

Medium-high. fs.readFile is one of the most-called Node APIs, so even a small change to its inner loop deserves a maintainer's eye. The no-signal path is byte-for-byte unchanged (the new code is inside if has_signal), which bounds the blast radius, but the signal path now diverges from Node's fixed kReadFileBufferLength chunking: Bun's doubling schedule pays O(log n) syscalls instead of O(n), at the cost that an abort arriving late (e.g. after ~256 MiB has been read) may still let one large chunk through. The author explicitly flagged this as open for maintainer input in the thread. That is a design tradeoff, not a correctness question, and belongs to a human.

Other factors

Three prior rounds of my inline feedback were all addressed (bounded poll deadline, sparse 512 MiB file for margin, deadline lowered to 4 s under the 5 s default, stale chunk-count comment dropped). The remaining concern I originally raised — the JS thread must observe an intermediate f_pos and call ac.abort() before the worker copies the full 512 MiB — was mitigated as far as practical (can't go larger without tripping should_throw_out_of_memory_early_for_javascript), verified 5/5 locally, but is inherently probabilistic on an oversubscribed runner. Since I've already raised it and the author responded, I'm not re-litigating it inline; a human can decide whether the residual risk is acceptable or whether the rchar-delta approach from the PR description would be sturdier. CI (#84525) was still building at last update.

[robobun]

Build 84525: test/js/node/fs/promises.test.js passed on every Linux lane. The 18 red tests are all retry-green flakes in unrelated areas (bake dev-server, cpu-prof, shell, http smuggling, tls timing, spawn, etc); none touch node_fs.rs or the new test.

Holding for direction on the cap schedule per the review above before pushing again.