Conversation
…e listings Every test in bun-prune.test.ts installed its starting tree from the registry into a cache of its own. beforeAll now installs every registry package the file uses once into one shared cache, and the installs the tests run are served from it without registry traffic. Identical starting trees are installed once and copied per test. The projects whose installs write to the cache (git and tarball dependencies, the global store) keep a cache of their own, and a final test checks that nothing else wrote to the shared cache. Each prune run now asserts stderr and the exit code, and each step asserts the whole listing of every node_modules folder, the isolated store and its hidden hoist folder, instead of the presence or absence of a few paths.
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes. 💤 Files selected but had no reviewable changes (1)
⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
|
Status: closed, not merged. The speed-up this PR set out to make does not show in CI (comparison in the comment below: five baselines at 14.8s to 16.2s on x64-asan, this PR at 20.4s to 20.7s in the modified-first slot, flat locally), and the file is not on the lane's critical path. The assertion half ( |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Updated 6:23 AM PT - Aug 21st, 2026
✅ @robobun, your commit 6f87762b848e2438c10da2572e3edec9915f43a6 passed in 🧪 To try this PR locally: bunx bun-pr 39954That installs a local version of the PR into your bun-39954 --bun |
There was a problem hiding this comment.
Thanks for addressing the Windows .bin shim-pair precision — shimPairName now only collapses to the base name when both .exe and .bunx are present, so a lone shim surfaces as an unexpected entry. LGTM.
Extended reasoning...
Overview
Test-only refactor of test/cli/install/bun-prune.test.ts (~1,800 lines changed, single file). Three pieces: (1) a shared warm BUN_INSTALL_CACHE_DIR populated in beforeAll so concurrent installs don't hit verdaccio, with per-project caches for the handful that write to the cache and a final non-concurrent test asserting the shared cache is untouched; (2) installed() template-and-copy so identical starting trees are installed once and copied per test, with copyTree re-pointing Windows junctions through the resolved path; (3) every prune run now asserts stderr + exit code via expectOk, and every step asserts a full tree(dir) listing (folders, store, hidden hoist, links marked, dangling links marked) instead of spot-checking a few paths.
Prior feedback addressed
My previous review flagged that the .bin branch of listFolder() collapsed a lone .exe or lone .bunx to the same entry a full pair would produce, weakening the kept-bin assertion on Windows relative to the removed expectBinInstalled. Commit 6f87762 adds shimPairName(files, file) which returns the base name only when files contains both ${name}.exe and ${name}.bunx, and the raw filename otherwise — a half-pair now shows up as .bin/foo.exe and fails equality. This restores (and slightly exceeds) the original precision.
Security risks
None. Test-only; no production code, no new dependencies, no network beyond the local verdaccio harness.
Level of scrutiny
Medium — the diff is large and introduces new test infrastructure, but it's confined to one test file with no runtime impact. Failure modes are CI-visible (either the file fails or flakes). The author verified 111 pass on the debug build across 3 runs and 110 pass / 2 skip on Windows canary, plus a manual check of the junction branch of copyTree.
Other factors
- Shared-cache concurrency: reads of a warm cache are safe; the design routes every cache-writing project (git deps, tarballs, global store, the mid-test tarball override via
ownCache.add(dir)) to its own cache, and the trailing non-concurrent test catches any missed case. copyTree: relative symlinks copied verbatim (stay valid inside the copy); absolute-target junctions re-pointed viarealpathSync.native+relative(template, ...)so the copy's junction resolves inside the copy — verified on Windows per the PR notes.- The two
hiddenHoistTargetbranches accommodate a documented install-order nondeterminism rather than masking a prune bug; both arms assert the resulting link is never dangling. - Net assertion strength is up: 276 whole-tree listings replace ~285 single-path checks, and
toEndWith/toContainon stdout/stderr became exact matches.
|
@robobun compare CI timings |
|
CI timings for debian 13 x64-asan, the lane the sweep flagged
Main builds have no asan lane, so the baselines are other PR builds. The runner runs modified files first, so in this PR's builds the file ran third in its shard, while squid, minio, redis and mysql were still starting. That inflates both PR numbers by an unknown amount (#39431 is about the same artifact). Even so, nothing here shows a win: the best case is "hidden by the noise", and the serial warm-up install in All lanes, one sample each (same caveat: this PR's runs were in the modified-first slot)
Local numbers from the PR body, for the record: flat with a release verdaccio (17.8s before, 17.2s to 18.2s after), 2.11s to 2.31s on Windows, and faster only when verdaccio itself runs on the debug build. Conclusion. The speed-up does not show in CI, and the file is 16s out of about 4950s of test time on that lane (build 102501, 20 shards). Its shard runs 253s of tests while the slowest shard runs 415s, so the lane's duration does not depend on this file at all. The per-process ASAN cost of the 201 prune runs and the verdaccio start dominate this file, and this PR cannot move either. The review I ran on the diff came to the same verdict and added that five open PRs (#38952, #39216, #38856, #38797, #39232) edit this file, so the rewrite would also cost each of them a rebase. I am closing this PR. Two things from the work that may still be useful:
|
2 participants
Problem
test/cli/install/bun-prune.test.tstook 16s on the debian x64-asan lane (build 102501). Each of its 169bun installruns fetched from verdaccio into its own cache. In CI verdaccio runs on the build under test, so on the asan lanes each fetch is slow.existsSyncon a few paths, so a prune that touched some other entry passed.Fix
beforeAllinstalls the 19 registry packages the file uses into one shared cache. From a warm cachebun installmakes no registry request. The 5 projects whose installs write to the cache (git, tarball, global store) get their own. A last, non-concurrent test asserts that the shared cache is unchanged.installed()installs each distinct starting tree once and copies it per test. Relative links are copied as they are, junctions are re-pointed into the copy. 350 processes instead of 381.tree(dir), the sorted listing of each node_modules folder, the store and its hidden hoist folder, with links and dangling links marked. 276 listings replace about 285 single-path checks.bun bd test test/cli/install/bun-prune.test.ts, 111 pass in 3 runs. Windows x64 on the canary of the base commit: 110 pass, 2 skipped. Timings are in the notes.Background
VerdaccioRegistryintest/harness.ts). In CI it runs onbunExe().BUN_INSTALL_CACHE_DIRholds tarballs and manifests and overridesbunfig.toml. A manifest stays fresh for 300s (src/install/npm.rs:578), so a warm cache serves a fresh install with no request.node_modules/.bun/node_modules, holds one link per package name of an isolated install. prune unlinks the ones it leaves dangling (housekeepinginsrc/install/prune.rs).Notes
Timings. Under ASAN
bun testruns at most 5 tests at a time (src/options_types/context.rs:506), so the per-test sum matters there. Local debug build, 16 shared cores, load average about 50, so wall times move by about 1s between runs.Bun.spawnplus reading both pipes costs it about 18ms per child in the debug build, and the 276 listings add fs calls.CI=1selects and what the asan lanes run (runs interleaved): before 62.5s and 59.5s wall, per-test sums 127.5s and 120.8s. After 53.0s and 50.8s wall, sums 63.0s and 58.4s. Starting verdaccio alone takes about 34s in this mode, in both versions. That start is the largest fixed cost of every install test file on the asan lanes. It is harness-level and not touched here.--production.Process counts. 381 before: 201 prune, 169 install, 11 other. 350 after: 201 prune, 138 install (67 template installs, 36 installs that are part of a scenario, 26
bun install --productioncross-checks, 8 projects built directly, 1 warm-up), 11 other. The 102 setups through the helpers build 71 distinct trees.Assertion changes.
expectOk(stderr is"", exit code 0) on every successful prune run, 121 places. Before, most of the 142 exit code checks came without a stderr check.not.toContain("warn:")andtoContain(PRUNED_NOTE)became exact stderr checks. The linker refusals, the missing workspace error, the--globalrejection (all four runs),bun run pruneand the global store prune output are exact now.toEndWith(NOTHING(...))became a check of the whole output.tree(dir)before and after each step. The state is asserted again after eachbun install --productioncross-check and after each--frozen-lockfileinstall. Before, nothing was asserted after them.--helpwithout a lockfile asserts the usage header and an empty stderr. The long flag dry run in the--helptest checks its exit code. The global store test asserts the listing of the cache'slinksfolder.nobodyand passes..bin/<name>only when both of its shims are present, so a listing that expects a kept bin still requires the complete pair, asexpectBinInstalleddid. A lone shim lists under its file name.What the listings pinned down. prune leaves an emptied
.binfolder and emptied nested or workspace folders in place, and removes emptied scope dirs.bun install --productionprints "no changes" but recreates the hidden hoist links. Afterbun installreplaces a root copy, the nested copy it made redundant stays until prune removes it. Afile:folder dependency gets no hidden hoist link.Observation, not changed here. When two versions of one package are both direct dependencies (an
npm:alias plus the real name), or when a full install follows a production install, the version that gets the hidden hoist link differs between installs of the same package.json (12 fresh installs of the alias case gave both). Two tests read the link before they prune and branch on it. With one direct and one transitive version the direct one gets the link every time (12 of 12).Own caches. Two git tests create repos with the same content in the same second, so they share one cache key, and two tests install the same left-pad tarball. With the shared cache both pairs would write the same entry at the same time. The final test catches any new case of this.
#38952 and #39216 also edit this file. Whichever lands second needs a rebase.