test(install): give each long-label bun patch test its own install cache - #38255
test(install): give each long-label bun patch test its own install cache#38255robobun wants to merge 2 commits into
Conversation
|
Warning Review limit reached
Next review available in: 18 minutes Limit details: You’ve used all 5 included reviews currently available under your plan. 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 (2)
Comment |
|
Status: fixed, CI green on the rebased head (debec67), ready for a maintainer.
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — test-only de-flake that isolates concurrent installs into per-project caches.
What was reviewed:
- Confirmed
{ ...bunEnv, BUN_INSTALL_CACHE_DIR: join(packageDir, ".bun-cache") }matches the pattern inbun-install-lifecycle-scripts.test.ts,bun-install-native-binlink.test.ts,symlink-path-traversal.test.ts. - Confirmed
scripts/runner.node.mjs:1791exportsBUN_INSTALL_CACHE_DIRper test invocation, so the env override is required (bunfig alone is insufficient in CI). - Verified
expect(value, message)is an established pattern in the suite; assertions and command args are otherwise unchanged, so #37469's coverage is preserved.
Extended reasoning...
Overview
This PR touches a single test file, test/cli/install/bun-patch.test.ts, refactoring the three test.concurrent cases in the "packages whose label is longer than 1024 bytes" describe block. createProject now returns { packageDir, env } where env is { ...bunEnv, BUN_INSTALL_CACHE_DIR: join(packageDir, ".bun-cache") }, and runBun/install thread that env through to every spawned bun process. The install helper also attaches stderr as the assertion message on the exit-code check. No src/ changes.
Security risks
None. Test-only change; the only new value is a temp-dir path passed as an env var to a child process.
Level of scrutiny
Low. This is a test isolation/de-flake fix with a thoroughly documented root cause (CI runner exports a shared BUN_INSTALL_CACHE_DIR, two concurrent tests install bar from an identical spec, Windows move_to_cache_directory retry arm evicts the other process's entry). I verified the runner does export that var (scripts/runner.node.mjs:1791) and that the per-project-cache pattern is already the convention across test/cli/install/ (lifecycle-scripts, native-binlink, symlink-path-traversal, git-deps, etc.).
Other factors
- The refactor is mechanical: same
buncommands, same arguments, same assertions — only the env gains one key. ThepackageDiris still destructured where filesystem paths are needed, soBun.write/Bun.filecalls are unchanged. expect(exitCode, stderr).toBe(0)uses the two-arg custom-message form already used elsewhere in the suite (e.g.test/package-json-lint.test.ts,test/js/bun/jsonc/jsonc.test.ts).- The PR description explicitly notes the underlying Windows cache-eviction race is a real
bun installbug being tracked separately, and confirms the tests still fail on a pre-#37469 canary — so this is not weakening coverage to route around a runtime bug. - REVIEW.md's de-flake guidance ("keep asserting the property the original assertion protected") is satisfied: no assertions were removed or loosened.
|
Updated 12:59 PM PT - Aug 16th, 2026
✅ @robobun, your commit debec678c3437f92fa1de6f7a77dd9b1f8cb3659 passed in 🧪 To try this PR locally: bunx bun-pr 38255That installs a local version of the PR into your bun-38255 --bun |
There was a problem hiding this comment.
LGTM — test-only de-flake that gives each concurrent long-label test its own BUN_INSTALL_CACHE_DIR, matching the pattern used across the install test suite.
What was reviewed
- Confirmed the
{ ...bunEnv, BUN_INSTALL_CACHE_DIR: join(packageDir, ".bun-cache") }shape matches the existing pattern inbun-install-lifecycle-scripts.test.ts,bun-install-native-binlink.test.ts, and others. - Verified all three tests thread the per-project
envthrough every spawn (install,patch,patch --commit) — none still uses barebunEnv. - Checked no assertions were weakened or removed; the bun commands and their arguments are unchanged, so the coverage from #37469 is preserved.
Extended reasoning...
Overview
This PR touches a single test file, test/cli/install/bun-patch.test.ts, and only the packages whose label is longer than 1024 bytes describe block within it. It refactors the block's local createProject helper to return { packageDir, env } (where env spreads bunEnv and adds a per-project BUN_INSTALL_CACHE_DIR), refactors runBun to accept that object, and updates the three test.concurrent bodies to pass it. It also attaches stderr as the assertion message on the install exit-code check so future failures self-report. No src/ code is touched.
Security risks
None. This is test-only infrastructure that changes which temp directory child bun install processes use as their package cache. No auth, crypto, network, or user-facing code paths are involved.
Level of scrutiny
Low. This is a targeted de-flake of a known Windows CI failure with a well-understood root cause (two concurrent installs of the identical tarball spec racing on a shared BUN_INSTALL_CACHE_DIR inherited from the CI runner). The fix — pinning a per-project cache via the env var — is the established idiom in this test suite (grep shows the same join(packageDir, ".bun-cache") pattern in bun-install-lifecycle-scripts.test.ts, bun-install-native-binlink.test.ts, 36577.test.ts, and many others). The change is mechanical and self-contained within one describe block.
Other factors
Per REVIEW.md's de-flaking guidance ("keep asserting the property the original assertion protected"), I checked that no assertions were removed or loosened: every expect from the original tests is still present, and the bun invocations (install, patch <name>@<label>, patch <path>, patch --commit) run with identical arguments — only the cache directory differs, which is orthogonal to the long-label formatting these tests cover. The PR description confirms the tests still fail on a pre-#37469 canary, so coverage is intact. The added expect(exitCode, stderr).toBe(0) improves diagnosability without changing pass/fail semantics. The underlying Windows cache-eviction race is explicitly noted as tracked separately, which is the right layering — this PR fixes the test isolation, not the runtime bug.
There was a problem hiding this comment.
LGTM — test-only de-flake that gives each concurrent long-label bun patch test its own BUN_INSTALL_CACHE_DIR.
What was reviewed:
- The
createTestDirchange intest/harness.tsis additive (packageDir/packageJsonunchanged; newenvfield), so the ~200 other callers that destructure only those fields are unaffected. - The three tests still run the same
bun install/patch/patch --commitinvocations with the same arguments; onlycwd/envplumbing changed, so the #37469 coverage is preserved. - Checked that
BUN_INSTALL_CACHE_DIRpoints at the same.bun-cachedirwriteBunfigalready sets ininstall.cache, so this just makes the intended per-project isolation actually win over the CI-exported env var.
Extended reasoning...
Overview
This PR de-flakes the packages whose label is longer than 1024 bytes block in test/cli/install/bun-patch.test.ts, which fails ~50% of the time on Windows CI because two test.concurrent tests install the same tarball spec into a shared cache and race on the Windows rename-and-delete publish path. The fix threads a per-project env (with BUN_INSTALL_CACHE_DIR=<packageDir>/.bun-cache) from VerdaccioRegistry.createTestDir through the local runBun helper so each test uses its own cache. It also attaches stderr to the exit-code assertion for better diagnostics.
Security risks
None. Test-only change; no runtime code under src/ is touched. The new env var points into an ephemeral temp dir the test already creates.
Level of scrutiny
Low. This is a mechanical refactor of test plumbing: createProject now returns the whole createTestDir result instead of just packageDir, and runBun destructures {packageDir, env} from it. The harness change adds one field to a returned object; I grep-verified other createTestDir callers only destructure packageDir/packageJson, so they are unaffected. The cache dir chosen matches what writeBunfig already writes to bunfig.toml, so the only behavioral difference is that the env var now overrides the CI runner's export — exactly the intended isolation.
Other factors
- The PR description documents a clean before/after reproduction on Windows (5/10 → 0/20) and confirms the tests still fail on a pre-#37469 canary, so coverage is preserved rather than weakened.
- No CODEOWNERS entries match these paths, no outstanding human review comments, and no prior claude[bot] review on this PR.
- The underlying Windows cache-eviction bug is explicitly noted as tracked separately; this PR correctly does not paper over it in
src/.
|
Some extra data points for this flake, collected while triaging it independently (same diagnosis as the PR body, so nothing here changes the fix):
|
The "packages whose label is longer than 1024 bytes" tests run concurrently and two of them install `bar` from the identical spec. They relied on the cache dir createTestDir writes to bunfig.toml to keep the projects apart, but CI exports BUN_INSTALL_CACHE_DIR, which takes precedence, so both installs published the same cache entry into one shared cache at the same time. On Windows the install that publishes second evicts the first one's entry while it is still being read, and that install exits 1, which failed the test on both Windows lanes. Pin BUN_INSTALL_CACHE_DIR per project, as the other install tests do, and attach the install's stderr to the exit code assertion so a failing install says why.
The cache that createTestDir writes to bunfig.toml loses to the BUN_INSTALL_CACHE_DIR the CI runner exports, and several install test files re-pin the env var per project by hand to get the isolation back. Return that env from createTestDir instead and have the long-label bun patch tests use it, rather than adding another local copy.
7b31af1 to
debec67
Compare
There was a problem hiding this comment.
LGTM — test-only de-flake giving each concurrent long-label bun patch test its own BUN_INSTALL_CACHE_DIR.
What was reviewed:
createTestDir()return-shape change is additive; spot-checked callers (frozen-lockfile-pruned, nested-overrides, pnpm-migration, hoist) all destructurepackageDir/packageJsonand are unaffected..bun-cachepath in the newenvmatches whatwriteBunfigalready writes toinstall.cache, so bunfig and env agree.- The three tests' commands and assertions are unchanged;
packageDiris still available where needed for filesystem writes.
Extended reasoning...
Overview
This PR fixes a Windows-only flake in the packages whose label is longer than 1024 bytes block of test/cli/install/bun-patch.test.ts. Two of the three test.concurrent cases install bar from an identical tarball spec; in CI they inherit the file-wide BUN_INSTALL_CACHE_DIR the runner exports, which overrides the per-project install.cache in bunfig, so both installs race on the same cache entry and one hits the Windows-only eviction bug (#28062). The fix has VerdaccioRegistry.createTestDir() also return an env (bunEnv + BUN_INSTALL_CACHE_DIR pointing at the same <project>/.bun-cache it already writes to bunfig), and the three tests spawn every bun command with that env instead of plain bunEnv. The test file is refactored to pass the whole Project object through runBun/install instead of just packageDir, and install() now attaches stderr to the exit-code assertion.
Security risks
None. Test-only change; no src/ edits, no new inputs, no external network.
Level of scrutiny
Low. This is a self-contained test-harness/de-flake change. The harness edit is purely additive (a new property on the returned object); I grep-checked createTestDir callers across test/ and every one either destructures packageDir/packageJson or holds the whole object without enumerating its keys, so nothing can be broken by the extra property. The test-side refactor is mechanical — the bun invocations, arguments, and assertions are byte-identical to before, only the env passed to Bun.spawn changes.
Other factors
- The PR body documents a clear root-cause chain (env > bunfig precedence in
fetch_cache_directory_path, Windows non-atomic cache publish inmove_to_cache_directory), reproduces it standalone on Windows (5/10 fail → 0/20), and confirms the tests still fail on a pre-#37469 canary so their coverage is preserved. - Placing the pin in
createTestDirrather than copying the same wrapper a sixth time matches REVIEW.md's "one implementation, in the right place" guidance; existing per-file copies are intentionally left alone to keep the change scoped to fixing the red lane. - No prior claude reviews on this PR and no outstanding human reviewer comments.
…y.createTestDir createTestDir now returns the env that pins BUN_INSTALL_CACHE_DIR to the project's own .bun-cache (the hunk #38255 proposes), so the file-local wrapper goes. install() takes (env, cwd) like runBunInstall. The ranged peer case pins no-deps@1.1.0 through the nd11 alias instead of reaching it through two-range-deps' range, as #38858 does.
|
Heads-up on an overlap: #40087 rewrites |
1 participant
Problem
test/cli/install/bun-patch.test.tsfails on main on the Windows lanes (x64 and aarch64), in thepackages whose label is longer than 1024 bytesblock added by install: stop panicking on package labels longer than the stack buffers #37469: thebun installstep exits 1, asexpect(exitCode).toBe(0)atinstall()(builds 94812, 94844; about half of the runs, so it usually passes on a retry and fails outright on PRs whose retries all lose).error: failed to verify cache dir for "bar": ENOENT,ENOENT: failed opening cache/package/version dir for package barorENOENT: failed copying files from cache to destination for package bar.test.concurrent, and two of them installbarfrom the identical spec. They spawn with plainbunEnv, so in CI they inherit theBUN_INSTALL_CACHE_DIRthe test runner exports (scripts/runner.node.mjs), whichfetch_cache_directory_path(src/install/PackageManager/PackageManagerDirectories.rs:379) consults before theinstall.cachethatcreateTestDirwrites tobunfig.toml. So both installs extract and publish the same cache entry (@T@<hash of the spec>) into one shared cache at the same time.move_to_cache_directory(src/install/extract_tarball.rs:576) moves the first install's entry out of the cache and deletes it before publishing its own, while the first install is still opening or copying from it. The first install exits 1 with one of the messages above. This is Windows: parallel bun install --no-cache with shared BUN_INSTALL_CACHE_DIR can fail with ENOENT opening cache/package/version dir #28062 (fix in flight in install: never delete a concurrently published cache entry on Windows #33884); the POSIX arm swaps atomically, which is why only the Windows lanes see this.Fix
VerdaccioRegistry.createTestDir()now also returnsenv:bunEnvplusBUN_INSTALL_CACHE_DIRpointing at the same<project>/.bun-cacheit already writes tobunfig.toml. The long-label tests run every command (install,patch,patch --commit) with their project'senv, so the two installs no longer share a cache and have nothing to race on.bunfig.toml), and the same re-pin is currently copied by hand inbun-workspaces.test.ts,bun-install-lifecycle-scripts.test.ts,bun-install-registry.test.ts,bun-install-native-binlink.test.tsandisolated-install.test.ts; the othercreateTestDircallers are unaffected by the extra property (they destructure or readpackageDir/packageJson) and can switch over when they go concurrent. Those existing copies are left alone here to keep this a small fix for the red lane.expect(exitCode, stderr)), so a failing install reports why; in the CI failures above it did not, and the reason had to be dug out of the job log.src/; the Windows eviction itself is Windows: parallel bun install --no-cache with shared BUN_INSTALL_CACHE_DIR can fail with ENOENT opening cache/package/version dir #28062 / install: never delete a concurrently published cache entry on Windows #33884, after which the isolation here is still correct, just no longer load-bearing.BUN_INSTALL_CACHE_DIRper run: unfixed test 5/10 runs fail, fixed test 0/20; the full file passes there. On Linux the full file passes with the debug build (34/34),hoist.test.tsandcatalogs.test.ts(othercreateTestDircallers) pass, and the three tests still fail withpanic: unreachable: Erroron a canary from before install: stop panicking on package labels longer than the stack buffers #37469, so they still cover what they did. The first CI run of this PR had the file passing first try on both Windows lanes.Background
BUN_INSTALL_CACHE_DIRis the env override for bun's package cache. It wins overinstall.cacheinbunfig.toml, and the test runner exports one per test-file invocation, so a test that wants a cache of its own has to set the env var; setting it in bunfig is not enough in CI.bun installextracts a package into a temp dir and then renames that dir into the cache. On POSIX a collision with an entry another process published is resolved with an atomic swap, so the entry never disappears; the Windows arm instead renames the existing entry away and deletes it before retrying, which is the window the first process falls into (Windows: parallel bun install --no-cache with shared BUN_INSTALL_CACHE_DIR can fail with ENOENT opening cache/package/version dir #28062).Probe: concurrent installs sharing a cache on Windows (no test involved)
N project dirs with
{"dependencies":{"bar":"<long ./x/../... spec>/bar-0.0.2.tgz"}}and the tarball,bun installspawned in all of them at once, repeated. bun 1.4.0-canary.1+18059646e, Windows Server 2019 x64.The test itself under the same CI-like env (fresh shared
BUN_INSTALL_CACHE_DIRperbun testrun): unfixed 5/10 runs fail, always atinstall(); fixed 0/20.Earlier revision
The first revision pinned
BUN_INSTALL_CACHE_DIRinsidebun-patch.test.tsonly (the same local wrapper the files listed above have). Self-review pointed out that this would be the sixth copy of that wrapper and that the harness is where the intent already lives, so the pin moved intocreateTestDir; the test-side behavior is identical.no test proof · iteration 0 · Platform-specific test-only change; deferring to CI.