install: keep the packages bun.lock holds only through optional peers when an install re-resolves - #39002
install: keep the packages bun.lock holds only through optional peers when an install re-resolves#39002robobun wants to merge 1 commit into
Conversation
… when an install re-resolves clean_with_logger let optional peer slots keep their targets alive only on a frozen install or when package.json had no diff, so an install that did re-resolve (any package.json change, or a diff a workspace reports on every run) pruned the packages a 1.3.x lockfile holds that way and rewrote the file. Record at load which packages a non-peer dependency resolves to; an optional peer slot releases only those, and a package the loaded lockfile held through optional peers alone is cloned like a dependency, whatever the install does.
|
Warning Review limit reached
Next review available in: 23 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 (5)
Comment |
|
Updated 10:05 AM PT - Aug 15th, 2026
❌ @robobun, your commit a803736 has some failures in 🧪 To try this PR locally: bunx bun-pr 39002That installs a local version of the PR into your bun-39002 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes core lockfile-rebuild semantics (what clean_with_logger keeps vs. drops) and the description notes it conflicts with the direction in #37925, a human look at the approach would still be worthwhile.
What was reviewed:
held_at_loadbitset construction and its use inPackage::clone— the out-of-bounds default (true) correctly defers session-appended packages, and the>= max_package_idguard above already handlesinvalid_package_id.keep_optional_peer_targetsfully removed (no remaining references);Clonerhas a single construction site.mark_loaded_packages's new fallible signature — sole caller propagates with?; the fresh-lockfile / load-error paths reset the set viainit_empty()beforeclean_with_loggerreads it.- The new test triggers a re-resolve (root gains
a-dep) against a 1.3-shaped lockfile and asserts the peer-onlyno-depsentry survives, is installed, and round-trips--frozen-lockfile.
Extended reasoning...
Overview
Replaces the session-level keep_optional_peer_targets boolean (previously frozen_lockfile() || !changes_resolutions()) with a per-package held_at_load bitset recorded at mark_loaded_packages time. In Package::clone, an optional-peer slot now defers to Cloner::flush only when its target either (a) had a non-optional-peer holder in the loaded lockfile or (b) was appended this session (out-of-bounds → true); a target the loaded file held through optional peers alone is cloned like a regular dependency, so it survives a re-resolving install unchanged. Touches lockfile.rs (new field, mark_loaded_packages now builds the bitset and returns Result<(), AllocError>), lockfile/Package.rs (the clone-time gate), install_with_manager.rs (propagate the ?), a comment in dedupe.rs, and adds one test to bun-lock.test.ts.
Security risks
None identified. This is internal lockfile-rebuild bookkeeping over data bun itself wrote; no new parsing of untrusted input, no path/URL handling, no privilege boundaries.
Level of scrutiny
High. This is the code path that decides which packages a bun install writes back to bun.lock and installs into node_modules, and it interacts with several recent changes (#35681, #37426, #38853). The description also flags that #37925 takes a conflicting direction (unbinding optional peers at load), so which approach lands is a maintainer call. The logic itself checks out — the is_set_allow_out_of_bound(.., true) semantics correctly encode "session-appended reads as held" per the field comment, and the pre-existing *old_resolution >= max_package_id guard covers invalid_package_id before the bitset lookup.
Other factors
The three #35681 tests and the #38853 frozen test are unchanged in the file and the description states they still pass, so the "remove drops it / non-peer keeps it" invariants are preserved by the new gate (a removed direct dep was held-at-load, so its peer slot still defers). The author notes the full bun-lock/bun-dedupe suites hit unrelated ConnectionRefused failures on this box, so CI is the confirmation that no snapshot drifted. Given the semantic scope and the open design question against #37925, deferring rather than approving.
|
Status: ready for review.
|
2 participants
Problem
encodingand@vitest/coverage-v8trees, activepieces'dockerodetree, ...). Since install: drop packages held only by optional-peer resolution slots from bun.lock #35681clean_with_loggerdrops such packages; install: keep optional-peer-held packages when the lockfile is frozen #38853 keeps them on a frozen install, so--frozen-lockfilepasses, but every other install that re-resolves still prunes them (keep_optional_peer_targets = frozen_lockfile() || !summary.changes_resolutions(),src/install/lockfile.rs:973).Workspace package "..." has ... updated 1 dependencieson main today, as they do on 1.3.14). On main, a plainbun installon a fresh clone of opencode printsClean lockfile: 2711 packages -> 2697 packages, rewrites the committedbun.lock(-36 lines) and installs without those packages; activepieces goes4069 -> 4063(-22 lines, measured with install: bind an unsatisfiable peer to the copy bun.lock nests under its dependent #38992 applied, which fixes its other four lines). 1.3.14 re-saves both files byte-identically. So what gets installed from one lockfile depends on whether the install was frozen, and abun addafter upgrading carries an unrelated removal of every such package into the diff.Fix
Lockfile::mark_loaded_packages, already called once right after load and before the differ, dedupe or an update re-point anything, also recordsheld_at_load: the loaded packages some non-optional-peer dependency resolves to (one pass over the dependency rows into a bitset). InPackage::clonean optional peer slot defers toCloner::flush, and so can release its target, only for a target in that set or one appended during this resolve; a target the loaded file held through optional peers alone is cloned like a dependency.keep_optional_peer_targetsgoes away: frozen and plain installs apply the same rule.bun remove xor the same edit by hand,xwas held by a real dependency when the file was loaded, that dependency is gone after the diff, and the slot releases it. A fresh resolve never binds an optional peer to a package nothing else holds, so files written by this version are unaffected;bun dedupekeeps its model because a surviving version keeps every edge it had (comment indedupe.rsupdated). No snapshot changes (install: keep optional-peer-held packages when the lockfile is frozen #38853 already updated the arborist fixture this affects;edit-package-json--removed, where a real dependent leaves, still exits 1).test/cli/install/bun-lock.test.ts, "a re-resolving install keeps the packages an older lockfile holds through optional peers alone": a 1.3-shaped lockfile plus a package.json that gains a dependency the file already has. Rebuilt with main'ssrc/installthe plain install drops theno-depsentry; with this change it keeps it, installs it, and--frozen-lockfileaccepts the saved file. install: keep optional-peer-held packages when the lockfile is frozen #38853's frozen test and the three install: drop packages held only by optional-peer resolution slots from bun.lock #35681 tests pass unchanged.ConnectionRefusedfrom the test registry (load average above 200) on unrelated tests as well; the same logic on yesterday's base passed bun-lock, bun-dedupe, bun-remove, frozen-lockfile-pruned, bun-update, bun-update-transitive, bun-audit, bun-prune, catalogs, bun-workspaces, isolated-install, bun-install-registry and the migration suites in full.Background
peerDependenciesMeta) is never installed on its own; when a package of that name is in the tree for another reason the hoister binds the edge to it, and the binding is a resolution slot in the lockfile like any other edge. 1.3.x copied every slot when rebuilding the lockfile, so once bound, such a package stayed in the file after its real dependent left.clean_with_loggerrebuilds the lockfile after resolution by cloning what the edges reach from the root (Package::clone,Cloner::flush); packages nothing reaches are dropped, and since install: keep optional peer bindings when cleaning the lockfile #37426 optional peer slots are bound afterwards, only to targets something else cloned.DiffSummary::changes_resolutions()says whether the root or a workspace changed a dependency row against the lockfile; it is what decides whether an install re-resolves, and it is true on every run for some untouched projects (install: keep optional-peer-held packages when the lockfile is frozen #38853 lists the shapes, install: fail --frozen-lockfile on manifest drift and fix the spurious lockfile re-saves behind it #33632 more), which is why tying the pruning to it made the result depend on how the install was invoked.no test proof · iteration 1 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/cli/install/bun-lock.test.ts