Skip to content

test: stop the Bun.write copy_file_range fallback test from starving its concurrent siblings - #37792

Open
robobun wants to merge 1 commit into
mainfrom
farm/a73c4569/bun-write-large-copy-fallback-test
Open

robobun wants to merge 1 commit into
mainfrom
farm/a73c4569/bun-write-large-copy-fallback-test

Conversation

@robobun

@robobun robobun commented Aug 12, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

  • On a debug or ASAN build, should work when copyFileRange is not available > on large files times out after 5000ms and 1 to 5 unrelated tests in the same file fail with it. Which siblings fail varies from run to run.
  • That test was fully synchronous: it built and hashed a 256 MB buffer, wrote it, copied it in a spawnSync child, then read and hashed the result, holding the event loop for 5 s or more in debug (3 s even in release).
  • The file is describe.concurrent, so a test that blocks the loop that long also burns the timeout of every sibling in flight.

Fix

  • The small and large cases become one it.each that spawns the child with Bun.spawn and awaits it, so nothing in the file blocks the loop for more than tens of milliseconds.
  • The large payload shrinks from 256 MB to 2 x 8 MiB + 100000 bytes. That still takes the heap-buffer path, loops more than once, ends on a partial pass, and preallocates the destination; 256 MB only repeated the same loop. The 4 KiB case still covers the stack-buffer path.
  • The check gets stricter: the child prints the byte count Bun.write resolved with, and the copy is compared byte for byte against random data. The old hash of a 1 KiB-periodic pattern could not detect a misplaced or repeated chunk.
  • Verification: test-only, nothing to prove against a source diff. Same debug build went from 1 to 5 failures per run to 50/50 in 3 runs; an LD_PRELOAD trace confirmed the fallback path is still the one exercised; a deliberately truncated copy fails the new assertion.

Background

  • Bun.write(dest, Bun.file(src)) on Linux normally uses copy_file_range; BUN_CONFIG_DISABLE_COPY_FILE_RANGE=1 forces the read/write loop fallback, which is what this test covers.
  • That fallback uses a 64 KiB stack buffer for sources up to 1 MiB, a heap buffer capped at 8 MiB above that, and preallocates the destination above 2 MiB. The two payload sizes are chosen around those thresholds.
  • describe.concurrent in bun:test runs the tests in a block at the same time, so their per-test timeouts overlap and one blocking test can fail all of them.

no test proof · iteration 0 · Platform-specific test-only change; deferring to CI.

Original description

What

test/js/bun/io/bun-write.test.js is describe.concurrent on POSIX. Inside it, should work when copyFileRange is not available > on large files was fully synchronous: it filled a 64M-element Int32Array (256 MB) in a JS loop, Bun.hashed it, writeFileSync'd it, Bun.spawnSync'd a child bun to copy it with BUN_CONFIG_DISABLE_COPY_FILE_RANGE=1, then readFileSync + Bun.hash again. On a debug/ASAN build that holds the event loop for 5 s or more, so the test times out and so does every concurrent sibling that happened to be in flight at the time. Which siblings fail varies from run to run.

Unmodified main (626034fda0), bun bd test test/js/bun/io/bun-write.test.js in a 12-core container:

(fail) Bun.write > should work when copyFileRange is not available > on large files [5094.54ms]
  ^ this test timed out after 5000ms.
(fail) Bun.write > Bun.write(Bun.file(fd), Bun.file(path)) does not truncate the fd > preserves bytes past the slice window in an r+ fd [6926.91ms]
(fail) Bun.write > Bun.write(Bun.file(fd), Bun.file(path)) does not truncate the fd > does not fallocate an O_APPEND fd for a source above the preallocate threshold [6633.41ms]
(fail) Bun.write > Bun.write(Bun.stdout, Bun.stdin) copies the whole pipe (> 4096 bytes) [6112.58ms]
(fail) Bun.write > Bun.write('output.html', HTMLRewriter.transform(Bun.file))) [5520.49ms]
 45 pass
 5 fail
Ran 50 tests across 1 file. [14.37s]

Even the release binary spends 3 s in this one test (USE_SYSTEM_BUN=1: on large files [3064.48ms], almost all of it sys time).

Fix

Both cases of that describe (on small files / on large files, names unchanged) are now one it.each that spawns the child with Bun.spawn and awaits it, so nothing in the file blocks the loop for more than a few tens of milliseconds.

The large payload shrinks from 256 MB to 2 * 8 MiB + 100000 bytes. With copy_file_range disabled, Bun.write(path, Bun.file(path)) goes through copy_file_using_read_write_loop (src/runtime/node/node_fs.rs), whose only size-dependent behaviour is: a 64 KiB stack buffer up to 1 MiB, above that a heap slab clamped to 8 MiB, plus the destination preallocate path above 2 MiB in blob/copy_file.rs. The new size still allocates the clamped slab, runs the loop more than once, ends on a partial pass and takes the preallocate path; 256 MB only added more iterations of the same loop. The 4 KiB case still covers the stack-buffer path.

The assertions got slightly stronger while here: the child prints the byte count Bun.write resolved with and the test checks it, and the copy is compared byte for byte (Buffer#equals) against a randomBytes payload instead of comparing hashes of a 1 KiB-periodic pattern, which could not tell a misplaced or repeated chunk from a correct copy. The bun-write-exdev-fixture.js fixture is still used by the POSIX_FADV_SEQUENTIAL test.

Verification

Test-only change, so there is no source diff to prove against. Before/after on the same debug build and container:

  • before: 3 runs, 1 to 5 failures each (the large-files test plus whichever siblings were in flight), file takes ~14 s
  • after: 3 runs, 50/50 pass each, file takes ~7.8 s; on large files is 340 to 530 ms in debug, 117 ms in release (was 3064 ms)
  • the fallback path is the one being exercised: running the child under an LD_PRELOAD shim shows fallocate(len=16877216), posix_fadvise(advice=SEQUENTIAL) (only issued by the read/write loop) and ftruncate(len=16877216) for the large case, and no fallocate for the small one
  • a deliberately corrupted copy (dest truncated by 100000 bytes after the write) fails the new assertion with size: 16777216 vs 16877216, identical: false

…ink the payload

bun-write.test.js is describe.concurrent on POSIX. The "on large files"
case filled a 256 MB Int32Array in a JS loop, hashed it twice and
spawnSync'd the copying child, all on the main thread, which under a
debug/ASAN build blocks the event loop for 5 s or more. The test timed
out itself and every concurrent sibling that happened to be in flight
timed out with it.

Both cases now spawn the child asynchronously and await it. The large
payload is 2 x 8 MiB + 100000 bytes, which still makes
copy_file_using_read_write_loop allocate its clamped 8 MiB slab, loop
more than once, end on a partial pass and go through the preallocate
path. The child now also reports the resolved byte count, and the copy
is compared byte for byte against a random payload instead of a 1 KiB
periodic pattern.
@coderabbitai

coderabbitai Bot commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

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

Next review available in: 23 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: c8495e93-67dd-4a34-a4ca-33805d0aa463

📥 Commits

Reviewing files that changed from the base of the PR and between 626034f and 05d0626.

📒 Files selected for processing (1)
  • test/js/bun/io/bun-write.test.js

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

@robobun

robobun commented Aug 12, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 6:41 AM PT - Aug 12th, 2026

❌ @robobun, your commit 05d0626 has 1 failures in Build #93004 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 37792

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

bun-37792 --bun

@robobun

robobun commented Aug 12, 2026 •

Copy link
Copy Markdown
Collaborator Author

Status: reproduced on unmodified main (626034fda0) with bun bd test test/js/bun/io/bun-write.test.js on a debug/ASAN build: the large-files fallback test times out at 5 s and takes 1 to 4 concurrent siblings with it (5 failures on the first run, details in the PR body). With this change the same build passes 50/50 on three consecutive runs and the file takes about half the time.

CI (build 93004): bun-write.test.js passes on every lane, including x64 ASAN. The only red lane is Windows 2019 x64 on test/js/node/test/parallel/test-cluster-shared-leak.js, which this Linux-only test change does not touch; reported separately. Test-only change, ready for review.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — test-only de-flake that converts a synchronous 256 MB copy test to an async it.each with a right-sized payload.

What was reviewed:

  • tempDir accepts a Buffer value (DirectoryTree type + isDirectoryTree byteLength check in harness.ts) so { "src.blob": randomBytes(size) } writes binary content correctly.
  • The removed mkdirSync named import has no remaining uses in the file; bun-write-exdev-fixture.js is still referenced by the fadvise test so it isn't orphaned.
  • Test names on small files / on large files are preserved via the it.each template; pipes drained concurrently, stdout/stderr asserted before exitCode, paths embedded via JSON.stringify.
Extended reasoning...

Overview

Single-file, test-only change to test/js/bun/io/bun-write.test.js. Two nearly identical Linux-only test cases (on small files / on large files under should work when copyFileRange is not available) are collapsed into one it.each, switched from Bun.spawnSync + writeFileSync/readFileSync on a 256 MB buffer to async Bun.spawn on a ~16 MB randomBytes payload, and the assertion moves from hash-equality on a periodic pattern to byte-for-byte Buffer#equals plus a check on the resolved byte count. The now-unused mkdirSync named import is dropped and randomBytes is added.

Security risks

None. Test code only; no network, no auth, no user input handling. The child is spawned with bunExe() and a -e script whose only interpolation is JSON.stringify'd temp-dir paths.

Level of scrutiny

Low. This is a targeted flake fix in a test file with no runtime code touched. The PR description traces the size choice against the actual thresholds in copy_file_using_read_write_loop (64 KiB stack buffer, 1 MiB heap-slab threshold, 8 MiB slab cap, 2 MiB preallocate threshold) and verifies via LD_PRELOAD that the fallback path is still exercised, so the payload shrink does not weaken coverage. The assertions are strictly stronger than before (exact bytes vs. hash of a repeating pattern; resolved byte count now checked).

Other factors

I confirmed in test/harness.ts that tempDir's DirectoryTree accepts Buffer leaves and that makeTreeSyncFromDirectoryTree distinguishes them via byteLength, so passing randomBytes(size) writes the raw bytes. Grep confirms mkdirSync is no longer referenced in the file. The bun-write-exdev-fixture.js fixture the old tests used is still consumed by the POSIX_FADV_SEQUENTIAL test in the same block, so nothing is orphaned. The new code follows the file's existing conventions (concurrent pipe drain via Promise.all, await using proc, using dir, stdout/stderr asserted before exit code) and the describe.concurrent context that motivated the change is visible at the top of the file. The remaining synchronous work (randomBytes, tempDir's writeFileSync, readFileSync of ~16 MB) is on the order of tens of ms, not seconds.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant