test(install): don't require the refetched manifest to be cached before install exits - #38580
test(install): don't require the refetched manifest to be cached before install exits#38580robobun wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThe manifest-cache test now accounts for asynchronous replacement of invalid cache entries. After reinstalling, it accepts an absent cache file or a refreshed ChangesManifest cache validation
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 — targeted test-only de-flake that stops depending on the fire-and-forget manifest cache write.
What was reviewed:
- Confirmed
existsis already imported fromfs/promises(line 5). - Checked the conditional assertion is not vacuous: without the #37669 name check the stale entry is never deleted, so the file exists and parses as
basic-1, and the earlier lockfile/node_modulesassertions fail first — the property is still enforced. - No TOCTOU between
exists()andparseManifest(): the subprocess has already exited by this point and the write is temp→rename, so the file state is stable.
Extended reasoning...
Overview
Single-file, test-only change to test/cli/install/bun-install-registry.test.ts. The final assertion in the "npm manifest cache entries are only reused for the package name they were saved for" test previously required the refetched no-deps manifest to be back on disk after the second install. That write is a fire-and-forget thread-pool task (Serializer::save_async) that bun install intentionally does not wait for (per #37203), and on Windows debug builds the install exits before it lands 5/5 times. The change wraps the parseManifest call in if (await exists(...)) and adds a comment explaining the race with an issue reference.
Security risks
None. Test-only change; no runtime code paths, no new inputs, no credential/network handling.
Level of scrutiny
Low. This is a 7-line test de-flake with a documented root cause (background write not awaited by design), a cited precedent (#37203 removed the same dependency elsewhere), and before/after verification on the failing platform. The main review concern for a change like this is whether it silently weakens the test — REVIEW.md's "when de-flaking, keep asserting the property the original assertion protected." I traced the regression case: without the #37669 name check, the corrupted entry is neither rejected nor deleted, so (a) the file still exists and the conditional branch is entered, failing on basic-1 ≠ no-deps, and (b) the earlier node_modules/no-deps/package.json and lockfile-resolution assertions would already have failed. The property is fully covered.
Other factors
- No TOCTOU risk between
exists()andparseManifest(): the subprocess has exited (awaited viaproc.exited), so the thread pool is dead and the file state is final; the write path is temp-then-rename so a present file is complete. - The added comment is longer than the repo's usual one-liner, but it documents a non-obvious timing behavior that would otherwise make the conditional look vacuous, and it links the governing issue. Acceptable.
- No prior reviews or comments on the PR; nothing outstanding to address.
|
Reproduced with a Windows x64 debug build ( The underlying timing (from a locally instrumented debug build, not part of this PR): the cache write task needs about 25 ms on that machine, while the second install, whose tarball is already cached, goes from receiving the manifest to exiting in about 12 ms. The first install's writes had finished about 45 ms before it was done resolving, which is the margin the rest of the suite relies on. Making the install wait for the write was tried and reverted in #37203, so the test is what changes here. Current head is rebased on main after #38333; the only change since the first push is the wording of the comment in the test. |
…re install exits The manifest name-check test asserted that the no-deps manifest fetched after the mismatched entry was rejected is back in the cache when the second install exits. That write is a thread pool task the install does not wait for, and with the tarball already cached the install exits before it lands on Windows debug builds, failing the test every time. Assert that the stale entry is gone and, when the write did land, that it holds the right manifest.
19ba9d0 to
6ca5eca
Compare
There was a problem hiding this comment.
LGTM — test-only de-flake that stops depending on the fire-and-forget manifest cache write having landed before install exits.
What was reviewed:
- Confirmed
existsis already imported fromfs/promisesin this file, so the new call resolves. - Verified the test still fails without the #37669 name check: the stale entry would not be deleted, so the file would still exist and parse as
basic-1; the lockfile andnode_modules/no-deps/package.jsonassertions above also independently catch that regression. - Checked for a TOCTOU between
exists()andparseManifest(): the second install has already exited so the delete-on-load is done, and the async write is temp-file-then-rename, so an existing file is always complete.
Extended reasoning...
Overview
This PR touches a single assertion at the end of the "npm manifest cache entries are only reused for the package name they were saved for" test in test/cli/install/bun-install-registry.test.ts. The final parseManifest(byName["no-deps"], ...) call is now guarded by if (await exists(...)), with a comment explaining that Serializer::save_async is a thread-pool task bun install intentionally does not wait for (per #37203), so on a Windows debug build the second install exits before the refetched manifest is written back to disk.
Security risks
None. Test-only change; no production code, no new inputs, no auth/crypto/permissions surface.
Level of scrutiny
Low. This is a targeted CI de-flake for a known 5/5 failure on Windows debug builds. The key REVIEW.md concern for de-flaking — "keep asserting the property the original assertion protected" — is satisfied: without the #37669 name-check fix, the basic-1 entry would neither be rejected nor deleted, so the file would still exist and the guarded expect(...).toBe("no-deps") would fire and fail. The earlier lockfile-resolution and node_modules/no-deps/package.json assertions also independently catch that regression, so the conditional does not make the test vacuous.
Other factors
existsis already imported at the top of the file (line 5, fromfs/promises), so no missing-import issue.- No TOCTOU: by the time we check,
proc.exitedhas resolved, so the synchronous delete-on-load inSerializer::load_by_file_idhas already happened; the only in-flight operation is the async write, which is temp-file + rename and therefore atomic. - The added comment is exactly the kind REVIEW.md asks for: it names why no observable signal exists (write is fire-and-forget by design) and links the PR that decided so.
- Same pattern was already accepted in #37203 for the security-scanner matrix.
- The bug hunting system found no issues.
|
Data point for this test from another branch: in https://buildkite.com/bun/bun/builds/97654 (alpine 3.23 x64, release build, passed on retry) the first half of "npm manifest cache entries are only reused for the package name they were saved for" failed as well: |
1 participant
Problem
test/cli/install/bun-install-registry.test.ts> "npm manifest cache entries are only reused for the package name they were saved for" (added in Robustness and input-handling pass across install, shell, TLS/QUIC, HTTP/3, SQL and crypto #37669) fails on every run with a debug build on Windows (Server 2019 x64, 5 of 5 runs; it passes on the release Windows lanes and on Linux):parseManifest(byName["no-deps"], ...)after the second install.Serializer::load_by_file_id, src/install/npm.rs), no-deps is fetched again,node_modules/no-depsis 1.0.0 and the lockfile points at the no-deps tarball. The assertion additionally requires the refetched manifest to be back on disk when the process exits.Serializer::save_async) thatbun installdoes not wait for. test(security-scanner-matrix): don't depend on the setup install's manifest cache writes having landed #37203 tried adding a wait and reverted it: the write is fire-and-forget by design, a missing entry only means a refetch. In this test the no-deps tarball is already in the cache from the first install, so nothing remains to do after the manifest arrives and the install exits before the task lands. On a Windows debug build that is not a narrow race: with timestamps added locally, the save task took about 25 ms from start to rename while the main thread went from receiving the manifest to "Saved lockfile" in about 12 ms, so the file was missing on every run.Fix
basic-1and fails this assertion, and no-deps would resolve through basic-1's manifest, failing the lockfile andnode_modulesassertions before it.bun bd test test/cli/install/bun-install-registry.test.ts -t "only reused for the package name": failed 5 of 5 runs before this change, passes 5 of 5 after; the expect count in those runs shows the file was absent every time, so the new branch is what runs there. Linux x64 debug+ASAN: passes before and after, as on main.Background
<cache>/<hash>.npmand reused by later installs until it expires. Since Robustness and input-handling pass across install, shell, TLS/QUIC, HTTP/3, SQL and crypto #37669, an entry whose stored package name does not match the requested one is deleted on load and the manifest is fetched again.Serializer::save_async(src/install/npm.rs): the parsed manifest is cloned into a task on the install thread pool, which writes a temporary file and renames it into place. The task is not counted as a pending task, so the command can finish, and the process exit, while it is still running. test(security-scanner-matrix): don't depend on the setup install's manifest cache writes having landed #37203 discusses this and keeps it that way.no test proof · iteration 0 · Platform-specific test-only change; deferring to CI.