install: add BUN_DISABLE_SLOW_FILESYSTEM_WARNING, set it in the test harness - #37000
Conversation
…harness
The "Slow filesystem detected" warning fires whenever creating and
renaming a temp file in the install cache takes over 100ms, which happens
regularly on loaded CI machines. Tests that assert on stderr then fail,
most recently bun-run-dir.test.ts on the alpine lanes: stderrForInstall
stripped the warning text but not its trailing newline, so
expect(err).toBe("") received "\n".
Instead of patching the filter into every test that spawns an install,
add a feature flag env var (same pattern as
BUN_DISABLE_SLOW_LIFECYCLE_SCRIPT_LOGGING) that suppresses the warning,
and set it in bunEnv and in the CI runner's child env so every
test-spawned bun is covered. Also make stderrForInstall consume the line
terminator, which keeps stderr assertions stable when tests run against
older binaries that do not know the flag, and filter the one unfiltered
stderr read in bun-run-dir.test.ts.
Verified by delaying renameat via an LD_PRELOAD shim to force the
warning: the old harness reproduces the exact CI failure, the new one
passes with both an old binary (regex path) and the new one (flag path).
WalkthroughBun adds ChangesSlow filesystem warning control
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — straightforward CI flake fix following the existing BUN_DISABLE_SLOW_LIFECYCLE_SCRIPT_LOGGING pattern.
What was reviewed:
- The new feature flag uses
new_feature_flag!with defaultfalse, so.get().unwrap_or(false)is safe and the warning still prints for real users. - The
if let Some(timer)refactor removes an.unwrap()and correctly unifies both gate conditions into the timer'sOptionstate. - The
stderrForInstallregex change (.*\r?\n?) correctly consumes the line terminator without over-matching, and handles a final line with no trailing newline.
Extended reasoning...
Overview
This PR fixes a CI flake in test/cli/install/bun-run-dir.test.ts where the timing-dependent "Slow filesystem detected" warning intermittently pollutes stderr on slow alpine CI runners. It touches five files: adds a BUN_DISABLE_SLOW_FILESYSTEM_WARNING feature flag in env_var.rs, gates the timer/warning on it in PackageManagerDirectories.rs, sets the flag in both test/harness.ts (bunEnv) and scripts/runner.node.mjs, tightens the stderrForInstall regex to consume the trailing newline, and applies stderrForInstall to the one unfiltered stderr read in the flaking test.
Security risks
None. The change only adds an opt-in env var that suppresses a diagnostic warn: line and skips starting a timer. No user input parsing, no filesystem/network behavior change, no auth or crypto paths.
Level of scrutiny
Low. This is test-infrastructure hardening plus a tiny, mechanical runtime change. The new flag exactly mirrors the adjacent BUN_DISABLE_SLOW_LIFECYCLE_SCRIPT_LOGGING (declared on the very next line). The Rust change replaces a duplicated log_level != Silent check + .unwrap() with if let Some(timer) = timer.as_mut(), which is strictly safer and keeps the two gate conditions in one place. Since new_feature_flag! with {} defaults to false, the warning remains on for end users; only the test harness and CI runner opt out.
Other factors
The PR description documents deterministic verification via an LD_PRELOAD shim that delays renameat, covering all three combinations (old harness + released bun reproduces; new harness + released bun passes via regex; new harness + new build passes via flag). The regex fix keeps stderrForInstall correct for older binaries that don't recognize the flag (belt-and-suspenders). The test-file change from if (err2) throw to expect(err2).toBe("") is a minor improvement in assertion quality. No outstanding reviewer comments and no prior reviews from me on this PR.
…ng knob With BUN_DISABLE_SLOW_FILESYSTEM_WARNING set by the harness and the CI runner, the slow filesystem warning can no longer reach test stderr, so the per-call-site filtering is dead weight. Remove the helper and unwrap its call sites. Add a test that forces the slow path deterministically (an LD_PRELOAD shim compiled at test time delays renameat past the 100ms threshold) and asserts the warning fires without the knob and stays silent with it. Linux glibc only, skipped when no C compiler is available. Also raise the file's default timeout: its auto-install tests download from the npm registry, which exceeds the 5s default under sanitizer builds on slow networks.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline nit: grepped the whole repo for remaining stderrForInstall callers — none, so removing it from harness.ts is safe. Also confirmed the if let Some(timer) refactor in PackageManagerDirectories.rs is load-bearing, not cosmetic — the old timer.as_mut().unwrap() under log_level != Silent would panic once the new flag can make timer None while log_level is non-silent.
Extended reasoning...
The reported finding is a test-quality nit (undrained pipes / exitCode-before-content in the new LD_PRELOAD test); it doesn't affect correctness. Separately I verified two things a reviewer would otherwise re-check: (1) stderrForInstall has zero remaining references after this PR, so deleting the export is safe; (2) the second hunk in PackageManagerDirectories.rs replacing if log_level != Silent { timer.as_mut().unwrap() } with if let Some(timer) is required by the first hunk — with the flag set, timer is None even when log_level != Silent, so the old unwrap would have panicked. The setDefaultTimeout(90_000) addition was also examined and ruled out as a violation given the file's pre-existing npm-registry auto-install tests.
The compiler spawn piped stdout/stderr without reading them, and the install helper asserted the exit code before callers saw stderr, so a failure reported only the code with the diagnostics swallowed.
![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)
test/harness.ts (1)
1501-1513: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDrain piped subprocess streams concurrently.
Use
Promise.allforstdout.text(),stderr.text(), andexitedintest/harness.ts#L1501-L1513andtest/cli/install/bun-publish.test.ts#L30-L32. Sequential reads can deadlock when the unread pipe buffer fills.🤖 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 `@test/harness.ts` around lines 1501 - 1513, Drain the piped subprocess streams concurrently by updating the subprocess result handling in test/harness.ts at lines 1501-1513 and test/cli/install/bun-publish.test.ts at lines 30-32 to await stdout.text(), stderr.text(), and exited together via Promise.all. Preserve the existing output assertions and exit-code validation after all three promises resolve.Source: Coding guidelines
🤖 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/cli/install/bun-run-dir.test.ts`:
- Around line 197-203: Update the compiler spawn configuration around the
`spawn` call to pass absolute paths for both `slow_rename.c` and
`slow_rename.so`, constructing each with `join(run_dir, ...)` while preserving
the existing `cwd` and compiler arguments.
In `@test/harness.ts`:
- Around line 77-79: Update runBunInstall so stdout and stderr are consumed
concurrently rather than reading stderr before stdout; start both stream reads
together with the process exit wait via Promise.all, preserving the existing
captured output and completion behavior.
---
Outside diff comments:
In `@test/harness.ts`:
- Around line 1501-1513: Drain the piped subprocess streams concurrently by
updating the subprocess result handling in test/harness.ts at lines 1501-1513
and test/cli/install/bun-publish.test.ts at lines 30-32 to await stdout.text(),
stderr.text(), and exited together via Promise.all. Preserve the existing output
assertions and exit-code validation after all three promises resolve.
🪄 Autofix
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: 151153d6-c267-4361-8033-1ac673cc1bb2
📒 Files selected for processing (13)
scripts/runner.node.mjssrc/bun_core/env_var.rssrc/install/PackageManager/PackageManagerDirectories.rstest/cli/install/bun-install-lifecycle-scripts.test.tstest/cli/install/bun-install-registry.test.tstest/cli/install/bun-link.test.tstest/cli/install/bun-lockb.test.tstest/cli/install/bun-publish.test.tstest/cli/install/bun-run-dir.test.tstest/cli/install/catalogs.test.tstest/cli/install/config-version.test.tstest/cli/install/npmrc.test.tstest/harness.ts
…ilter removal runBunInstall and the publish helper read one pipe to completion before the other; a filled pipe buffer could block the child. Read stdout, stderr, and the exit together. Also collapse the no-op aliases the stderrForInstall removal left behind.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/cli/install/bun-run-dir.test.ts (1)
32-40: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDrain both child pipes concurrently.
Each changed path starts a child with
stdout: "pipe"andstderr: "pipe", then waits for stderr before reading stdout. The child can block when either pipe buffer fills. The test can hang instead of reporting the process result.Start both stream reads and the matching exit promise in one
Promise.all. Assert the collected output and exit code before checking the cache contents.Proposed fix
- const err1 = await new Response(stderr1).text(); + const [err1, out1, exitCode1] = await Promise.all([ + new Response(stderr1).text(), + new Response(stdout1).text(), + exited1, + ]); expect(err1).toBe(""); + expect(out1.split(/\r?\n/)).toEqual(["print(42);", ""]); + expect(exitCode1).toBe(0); expect(await readdirSorted(run_dir)).toEqual([".cache", "test.js"]); ... - const out1 = await new Response(stdout1).text(); - expect(out1.split(/\r?\n/)).toEqual(["print(42);", ""]); - expect(await exited1).toBe(0);Apply the same pattern to the other three changed subprocess blocks.
As per coding guidelines, subprocess tests must drain stdout, stderr, and process exit concurrently.
Also applies to: 57-64, 97-113, 130-146
🤖 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 `@test/cli/install/bun-run-dir.test.ts` around lines 32 - 40, Update all four changed subprocess blocks to drain stdout, stderr, and the corresponding exit promise concurrently via one Promise.all, rather than awaiting stderr before reading stdout. In each block, assert the collected output and exit code before validating cache contents; preserve the existing assertions and apply the pattern consistently to the subprocess flows.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@test/cli/install/bun-run-dir.test.ts`:
- Around line 32-40: Update all four changed subprocess blocks to drain stdout,
stderr, and the corresponding exit promise concurrently via one Promise.all,
rather than awaiting stderr before reading stdout. In each block, assert the
collected output and exit code before validating cache contents; preserve the
existing assertions and apply the pattern consistently to the subprocess flows.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 82954a0a-bba7-4051-a89e-1e3b4f6eb399
📒 Files selected for processing (6)
test/cli/install/bun-lockb.test.tstest/cli/install/bun-publish.test.tstest/cli/install/bun-run-dir.test.tstest/cli/install/catalogs.test.tstest/cli/install/config-version.test.tstest/harness.ts
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
test/cli/install/bun-run-dir.test.ts:226— The new test overwritesLD_PRELOADoutright instead of chaining onto any inherited value. The sibling install-with-shim test in this same directory (bun-install-hardlink-fallback.test.ts:70) and 6 other LD_PRELOAD-shim tests across the repo use[shim, env.LD_PRELOAD].filter(Boolean).join(":")so an inherited preload (profiler, sanitizer runtime) isn't dropped — worth matching that here.Extended reasoning...
What the issue is
The new
BUN_DISABLE_SLOW_FILESYSTEM_WARNINGtest spawnsbun installwith:env: { ...env, LD_PRELOAD: join(run_dir, "slow_rename.so"), ... },
where
envisbunEnv, which spreadsprocess.env. This unconditionally replaces anyLD_PRELOADinherited from the parent environment with the shim path alone.The established convention
Seven of the nine existing LD_PRELOAD-shim tests in the repo chain the shim onto the inherited value rather than overwriting it. Most directly, the sibling test in the same directory with the same install-with-shim pattern —
test/cli/install/bun-install-hardlink-fallback.test.ts:70— does:LD_PRELOAD: [join(String(dir), "shim.so"), bunEnv.LD_PRELOAD].filter(Boolean).join(":"),
The other chaining sites are
bun-write.test.js:710,shell-pipe-read-fault.test.ts:281,shell-write-fault.test.ts:94,watch.test.ts:109,serve-epoll-add-fail.test.ts:170, andfs.test.ts:5383. Only two files (setTimeout.test.js,crypto-random.test.ts) overwrite directly, so the majority pattern — and, decisively, the same-directory sibling — establishes chaining as the convention.REVIEW.md's "Copy harness conventions exactly" and "Match the exact file's local conventions" both apply: the sibling install test in
test/cli/install/is the controlling precedent for this pattern.Step-by-step: how it could bite
- A developer or CI lane exports
LD_PRELOAD=/path/to/some_profiler.so(or an ASAN runtime shim on a build where it's preloaded rather than compiled in). bunEnvspreadsprocess.env, soenv.LD_PRELOADis"/path/to/some_profiler.so".- This test's spawn sets
LD_PRELOAD: join(run_dir, "slow_rename.so"), discarding the inherited value. - The child
bun installruns without the profiler/runtime preload. Depending on what was preloaded, the child may fail to start (missing interposed symbol) or behave differently — while every other LD_PRELOAD-shim test in the suite still works because they chained.
Why this is a nit, not a blocker
bunEnvdoes not itself setLD_PRELOAD; it only inherits one if the parent shell exported it.- Bun's ASAN builds compile the sanitizer in rather than preloading it, so the specific "drops the ASAN runtime" scenario doesn't apply to current CI.
- No current CI lane exports
LD_PRELOAD, so nothing concretely breaks today.
It's a one-line convention alignment, not a behavioral bug in the current setup.
How to fix
LD_PRELOAD: [join(run_dir, "slow_rename.so"), env.LD_PRELOAD].filter(Boolean).join(":"),
matching
bun-install-hardlink-fallback.test.ts:70. - A developer or CI lane exports
There was a problem hiding this comment.
LGTM — the earlier nits (undrained pipes, leftover rawErr aliases) are addressed in 1a77e07 and d3fc71c.
What was reviewed:
- The new flag mirrors
BUN_DISABLE_SLOW_LIFECYCLE_SCRIPT_LOGGINGexactly, and theif let Some(timer)refactor drops the prior.unwrap(). runBunInstall's return shape is unchanged ({ out, err, exited }—exitedstill the original promise), so the ~190 callers are unaffected.- Grepped for remaining
stderrForInstallreferences — none; every call site was unwrapped.
Extended reasoning...
Overview
Adds a BUN_DISABLE_SLOW_FILESYSTEM_WARNING env-var feature flag (2-line addition in env_var.rs alongside the identically-shaped BUN_DISABLE_SLOW_LIFECYCLE_SCRIPT_LOGGING), gates the install-cache slow-filesystem timer/warning on it in PackageManagerDirectories.rs, sets it in bunEnv and the CI runner, and removes the now-dead stderrForInstall harness helper along with its ~70 call sites across 9 install test files. Drive-by: runBunInstall and the publish helper now drain stdout/stderr/exit concurrently via Promise.all.
Security risks
None. The only runtime change is an opt-in env var that suppresses an advisory stderr warning; it is never set for real users. Everything else is test harness and mechanical test-file edits.
Level of scrutiny
Low-medium. The Rust change is 8 lines following an existing sibling pattern verbatim, and replaces a timer.as_mut().unwrap() with if let Some — strictly safer. The bulk of the diff is a mechanical find/replace (stderrForInstall(x) → x) that I spot-checked across the touched files. The one non-mechanical harness change is runBunInstall switching from sequential awaits to Promise.all; I verified the return object still exposes exited as the original spawn promise so none of the ~190 callers change behavior.
Other factors
Two prior review rounds (mine and CodeRabbit's) flagged undrained pipes in a since-removed LD_PRELOAD test, leftover const err = rawErr aliases from the mechanical unwrap, and sequential pipe reads in runBunInstall — all resolved in d3fc71c, f6b2d74, and 1a77e07. The comment-cop nag on the env_var.rs comment was trimmed in a437049. No remaining stderrForInstall references in the tree. The fix is placed at the right layer (suppress at source rather than filter at every call site), and the PR description names the deleted helper as required by REVIEW.md's dead-code rule.
…harness (oven-sh#37000) ### Problem `test/cli/install/bun-run-dir.test.ts` is flaky on the alpine CI lanes (5 of the last 60 main builds, e.g. builds 89312, 89106, 89071, 88806, 88526). The failing assertion is: ``` expect(err1).toBe(""); received "\n" ``` The root cause is the install warning ``` warn: Slow filesystem detected. If <cache dir> is a network drive, consider setting $BUN_INSTALL_CACHE_DIR to a local folder. ``` which fires whenever creating and renaming a temp file in the install cache takes more than 100ms. `stderrForInstall` in the harness stripped the warning text but left its trailing newline, so the assertion received `"\n"`. Any test that asserts on stderr without that filter had the same latent flake, and the filter had to be remembered at every call site. ### Fix - Add a `BUN_DISABLE_SLOW_FILESYSTEM_WARNING` feature flag env var (same pattern as `BUN_DISABLE_SLOW_LIFECYCLE_SCRIPT_LOGGING`) that suppresses the warning, and skip starting the timer entirely when it is set. - Set it in `bunEnv` (harness) and in the CI runner's child environment, so every test-spawned bun is covered instead of only the call sites that remembered to filter. - Remove `stderrForInstall` and unwrap its ~70 call sites across the install tests; the filtering is dead weight once the warning cannot fire under the harness. - While in those helpers, read stdout, stderr, and the exit concurrently (`runBunInstall`, the publish helper) so a filled pipe buffer cannot block the child. ### Verification - Forced the slow path deterministically with an `LD_PRELOAD` shim that delays `renameat` past the 100ms threshold: without the env var the warning prints, with it stderr stays silent, and the old harness reproduces the exact `""` vs `"\n"` assertion diff from the alpine lanes. - The warning itself is unchanged for real users; the env var is only set by the test harness. - Install suites pass locally with the helper removed: bun-run-dir, catalogs, config-version, npmrc, bun-lockb, bun-publish, bun-link, bun-install-lifecycle-scripts (the few local failures are pre-existing debug-build environment issues that reproduce with pristine test files). <!-- robobun:evidence:begin --> --- **no test proof** · iteration 2 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/cli/install/bun-install-lifecycle-scripts.test.ts test/cli/install/bun-install-registry.test.ts test/cli/install/bun-link.test.ts test/cli/install/bun-lockb.test.ts test/cli/install/bun-publish.test.ts <!-- robobun:evidence:end -->
2 participants
Problem
test/cli/install/bun-run-dir.test.tsis flaky on the alpine CI lanes (5 of the last 60 main builds, e.g. builds 89312, 89106, 89071, 88806, 88526). The failing assertion is:The root cause is the install warning
which fires whenever creating and renaming a temp file in the install cache takes more than 100ms.
stderrForInstallin the harness stripped the warning text but left its trailing newline, so the assertion received"\n". Any test that asserts on stderr without that filter had the same latent flake, and the filter had to be remembered at every call site.Fix
BUN_DISABLE_SLOW_FILESYSTEM_WARNINGfeature flag env var (same pattern asBUN_DISABLE_SLOW_LIFECYCLE_SCRIPT_LOGGING) that suppresses the warning, and skip starting the timer entirely when it is set.bunEnv(harness) and in the CI runner's child environment, so every test-spawned bun is covered instead of only the call sites that remembered to filter.stderrForInstalland unwrap its ~70 call sites across the install tests; the filtering is dead weight once the warning cannot fire under the harness.runBunInstall, the publish helper) so a filled pipe buffer cannot block the child.Verification
LD_PRELOADshim that delaysrenameatpast the 100ms threshold: without the env var the warning prints, with it stderr stays silent, and the old harness reproduces the exact""vs"\n"assertion diff from the alpine lanes.no test proof · iteration 2 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/cli/install/bun-install-lifecycle-scripts.test.ts test/cli/install/bun-install-registry.test.ts test/cli/install/bun-link.test.ts test/cli/install/bun-lockb.test.ts test/cli/install/bun-publish.test.ts