test(install): isolate cache dir per concurrent case in 36577.test.ts - #36613
Conversation
The two test.concurrent cases install overlapping package names (f000-f023, lib, carrier, pdep, zz-late). The test tried to isolate each case's cache via bunfig [install].cache, but the CI runner sets BUN_INSTALL_CACHE_DIR on the test process, bunEnv inherits it, and fetch_cache_directory_path checks that env var before bunfig, so both concurrent installs raced on the same cache directory. On Windows that race hits the extract_tarball retry path, which moves an existing cache entry aside before retrying, so one install's verify step sees ENOENT on an entry the other install just moved (#28062). Override BUN_INSTALL_CACHE_DIR per test case so the isolation the test intended actually holds regardless of the ambient env.
|
Warning Review limit reached
Next review available in: 13 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 (1)
Comment |
|
Updated 2:43 AM PT - Aug 1st, 2026
✅ @robobun, your commit 6aa143765d66f63ab0843f3b760c63a6ba888acb passed in 🧪 To try this PR locally: bunx bun-pr 36613That installs a local version of the PR into your bun-36613 --bun |
There was a problem hiding this comment.
LGTM — test-only flake fix that overrides BUN_INSTALL_CACHE_DIR per concurrent case.
What was reviewed
- Verified
scripts/runner.node.mjssetsBUN_INSTALL_CACHE_DIRandbunEnvinherits it via...process.env. - Verified
fetch_cache_directory_pathreads the env var before bunfig[install].cache, so the old bunfig setting was dead under CI. - Confirmed the two concurrent cases share package names (
f000–f023,lib,carrier, …), so a shared cache dir is a real race; per-tempDircache path restores the intended isolation.
Extended reasoning...
Overview
This PR touches a single test file, test/regression/issue/36577.test.ts, to fix a Windows CI flake. The two test.concurrent cases previously relied on cache = "cache" in bunfig for cache isolation, but the CI runner exports BUN_INSTALL_CACHE_DIR, which fetch_cache_directory_path in src/install/PackageManager/PackageManagerDirectories.rs consults before bunfig. The fix spreads bunEnv and overrides BUN_INSTALL_CACHE_DIR to a path inside each case's own tempDir, then drops the now-dead bunfig cache key and the mkdirSync call (bun creates the cache dir on demand).
Security risks
None. Test-only change; no production code, no network, no auth/crypto surface.
Level of scrutiny
Low. This is a small, mechanical de-flake of a regression test. I verified the three claims the fix rests on against source: (1) runner.node.mjs:1789 sets BUN_INSTALL_CACHE_DIR on spawned tests, (2) bunEnv in test/harness.ts starts with ...process.env so inherits it, and (3) fetch_cache_directory_path returns early on the env var before ever looking at options. The overlapping package names between the 24- and 32-filler cases (f000–f023 plus the fixed graph packages) make a shared cache a genuine concurrent-writer race, matching the described Windows ENOENT symptom.
Other factors
The env override follows the repo convention for modifying bunEnv ({ ...bunEnv, KEY: value }). The added comment is a durable why-comment explaining the env-over-bunfig precedence, not narration. The PR description states the test still fails with the #36578 fix reverted, so regression coverage is preserved. No prior reviewer comments to address.
|
CI status for 64efd7c (build 86598):
Ready for merge. |
…imeout The frozen-lockfile comparison under test only needs a lockfile, so skip tarball download/extraction with --lockfile-only and drop the Bun.Archive.write tarball synthesis (integrity is never verified on this path). Debug+ASAN now completes in ~1.8s instead of timing out at 5s; fail-before with the src/install/lockfile.rs change reverted is preserved.
There was a problem hiding this comment.
LGTM — test-only de-flake and speedup for a Windows CI failure.
What was reviewed:
- Confirmed
bunEnvspreadsprocess.envandscripts/runner.node.mjssetsBUN_INSTALL_CACHE_DIR, so the env override is the right fix (bunfigcachewas indeed shadowed). - Confirmed the frozen-lockfile
Lockfile::eqlcheck runs before thelockfile_onlyearly return ininstall_with_manager.rs, so--lockfile-onlystill exercises the regression path. - Checked that
--lockfile-onlyskips tarball fetch (PackageManagerEnqueue.rs), so the fake integrity and removed tarball route/shasumare safe;await usingon the now-syncserveGraphreturn is fine sinceServerhas[Symbol.asyncDispose].
Extended reasoning...
Overview
This PR touches a single test file, test/regression/issue/36577.test.ts, to fix a Windows CI flake and bring the test under the default 5s timeout on debug+ASAN. Two changes: (1) override BUN_INSTALL_CACHE_DIR in the spawn env so the two test.concurrent cases don't share a cache dir when the CI runner sets that var (which shadows the bunfig [install].cache the test previously relied on), and (2) run both installs with --lockfile-only, which lets the fake registry drop tarball synthesis (Bun.Archive.write), the tarball route, and real integrity hashes.
Security risks
None. Test-only change; no production code, no auth/crypto/permissions surface. The fake sha512 integrity is derived from a constant string but is never verified because --lockfile-only skips tarball fetch entirely.
Level of scrutiny
Low. This is de-flaking test infrastructure with no runtime behavior change. The main risk with such changes is silently weakening the regression coverage — that's addressed: I verified in src/install/PackageManager/install_with_manager.rs that the frozen_lockfile / Lockfile::eql check (lines ~706-742) runs before the lockfile_only early return (line ~752), so the comparison the original regression test targets is still exercised. The PR description also confirms the test still fails when the #36578 fix is reverted.
Other factors
- Root cause is well-diagnosed:
bunEnvspreads...process.env(harness.ts:65) andscripts/runner.node.mjs:1789setsBUN_INSTALL_CACHE_DIR, whichfetch_cache_directory_pathprefers over bunfig — so the previous bunfig-based isolation was a no-op in CI. serveGraphbecame sync;await using server = serveGraph(pkgs)is correct sinceawait usingdoesn't await the RHS andBun.serve()'s return implements[Symbol.asyncDispose].- Dead code (
mkdirSync,rmSyncimports,tarballDir, tarball route,shasum) is removed in the same change, per repo convention. - The per-case
BUN_INSTALL_CACHE_DIRoverride remains useful even with--lockfile-onlysince the manifest cache also lives there. - CI (build 86598) passed the modified test on all 16 Windows shards; remaining reds are unrelated known flakes.
- No prior review comments on this PR; nothing outstanding to address.
Fixes the Windows CI flake in
test/regression/issue/36577.test.tsintroduced by #36578, and brings the test under the default timeout on debug builds.Repro
With the CI runner's env on Windows:
fails 12/15 runs with either
expect(r.code).toBe(0)receiving 1, or stderr containingCause
The two
test.concurrentcases install overlapping package names (f000-f023,lib,carrier,pdep,zz-late). The test setcache = "cache"in bunfig to give each case an isolated cache, butscripts/runner.node.mjssetsBUN_INSTALL_CACHE_DIRon the test process,bunEnvinherits it, andfetch_cache_directory_pathconsults that env var before the bunfig setting, so both concurrentbun installprocesses shared one cache directory.On Windows, the tarball-extraction retry path handles an occupied cache slot by renaming the existing entry into the temp dir and deleting it before retrying, so one process's post-rename verify can see
ENOENTon an entry the other process just moved aside. That underlying race is #28062 (fix in progress in #33884). The POSIX path usesrenameat_concurrently_a, which does not remove the existing entry, so the flake is Windows-only.Separately, under debug+ASAN each case took ~5s (two subprocess installs on 80-104 packages plus ~100
Bun.Archive.writecalls for tarball synthesis in the registry server), right at the 5s default timeout.Fix
BUN_INSTALL_CACHE_DIRper test case in the spawn env so the intended isolation holds regardless of the ambient environment; drop the now-dead bunfigcachekey andmkdirSync.--lockfile-only. TheLockfile::eqlcomparison under test only needs a lockfile, so tarball download, extraction and linking are irrelevant. With no tarball fetch the registry server no longer needs to synthesize real tarballs, so theBun.Archive.writepath and the tarball route are removed (integrity is never verified on this path).Verification
bun bd test: 5/5 pass, 0.8-1.8s per case (was timing out at 5s).src/install/lockfile.rschange from install: fix spurious --frozen-lockfile rejection when npm aliases duplicate a package name in one tree node #36578 reverted, both cases still fail withlockfile had changes, but lockfile is frozen, so the test continues to cover the original regression.[stamp-90s] gate passed · iteration 1 · 1 files touched
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 1 rejected · iteration 1
evidence per changed file
self-review · no surviving concerns
33 concerns were raised and did not survive verification.