fix(npm): avoid spurious npm re-resolution that fails under --cached-only#35051
fix(npm): avoid spurious npm re-resolution that fails under --cached-only#35051bartlomieju wants to merge 2 commits into
Conversation
Older deno releases encoded npm peer-dependency resolution in package ids with a different cycle-unrolling depth. A complete deno.lock written by an older deno is therefore seen as not satisfying the requirements, so the whole npm graph is re-resolved against the registry. Under --cached-only (offline isolates booting from an immutable, pre-baked DENO_DIR) that re-resolution fails because the cache lacks the extra packuments the newer peer algorithm probes. The existing dedup_equivalent_peer_variants normalization already collapses ids that differ only in cycle-unrolling depth, but it was gated to install commands. Apply it when loading the snapshot from a lockfile on every path. It is an in-memory normalization and does not rewrite the user's lockfile unless a real resolution runs (which it is precisely what avoids for an already-satisfied lockfile).
bartlomieju
left a comment
There was a problem hiding this comment.
Review
Removing the install-only gating so the in-memory peer-variant normalization runs on every lockfile load is the right call, and the reasoning holds up against the code. Approve pending CI.
Correctness — the core safety claim checks out
The central assertion ("dedup is in-memory only and doesn't rewrite the lockfile unless a real resolution runs") is verifiable in the diff:
snapshot_from_lockfileonly readslockfile.content.packagesand mutates a localSerializedNpmResolutionSnapshot. It never callsinsert_npm_package/insert_package_specifier, sohas_content_changedstays false andis_pending(initializer.rs) stays false.dedup_equivalent_peer_variants()only collapses ids of the same name+version whose effective dep signature is identical, so merging can't change which reqs are satisfiable. The satisfied-lockfile path short-circuits with no re-resolution and no write — exactly the offline--cached-onlycase the PR targets.
The only new behavior on deno run: an old-encoding-but-complete lockfile is now satisfied in memory. If a genuinely stale lockfile does trigger a real resolution, the deduped form gets written, but that path already wrote before, so it's not a regression.
Tests
test_snapshot_from_lockfile_dedups_old_peer_encoding exercises both false (4 packages, over-unrolled root) and true (2 canonical packages) and would have caught the bug. Solid regression coverage.
Minor (non-blocking)
- LSP path left at
false(cli/lsp/resolver.rs). The title says "any lockfile" but the LSP resolver still passesdedup_equivalent_peer_variants: false, so an old-encoding lockfile still provokes a registry round-trip there. Probably intentional (LSP isn't--cached-only), but a one-line note would save the next reader from thinking it was missed. - Perf on the hot path.
dedup_equivalent_peer_variants()now runs on every npm-usingdeno run/deno teststartup. The inner partition loop recomputesdep_signature(part[0])per comparison inside an outerloop. Negligible for normal lockfiles; just flagging the spot in case a huge lockfile ever shows up in startup profiles.
LGTM, just needs CI green.
A lockfile can be flagged as changed for reasons unrelated to npm, most notably a `links` section that older deno versions did not write and that is re-derived from the workspace config on load. That flag forced a full npm re-resolution, which fetches registry metadata. Under `--cached-only` that metadata is absent from a precomputed cache, so the run failed even though the lockfile's npm section already resolved every requirement. Skip npm resolution when the lockfile snapshot satisfies all requirements and either the lockfile is unchanged or we are offline (`--cached-only`). Online resolution is unchanged, so the lockfile is still brought up to date when it needs to be. Adds spec tests covering a links-only lockfile change run offline.
Two unrelated triggers can make
denore-resolve the entire npm graphagainst the registry on startup. Under
--cached-only(offline isolatesbooting from an immutable, pre-baked
DENO_DIR) that re-resolution fails,because the offline cache lacks the registry metadata it would need, and
the run crashes with "npm package not found in cache" even though the
lockfile's npm section already resolved every requirement. This combines
two fixes for that class of problem.
First, older deno releases encoded npm peer-dependency resolution in
package ids with a different cycle-unrolling depth than current deno. A
complete
deno.lockwritten by an older deno is therefore treated as notsatisfying the requirements, so the whole npm graph is re-resolved. The
dedup_equivalent_peer_variantsnormalization already collapses ids thatdiffer only in unrolling depth, but it was gated to install commands. It
now runs whenever a snapshot is loaded from a lockfile, so an older
lockfile satisfies the requirements as-is and no registry round-trip is
attempted. The dedup is purely in-memory and does not rewrite the user's
lockfile unless a real resolution runs, which is exactly what it avoids
for an already-satisfied lockfile. For old-encoding lockfiles this is a
net win online too, since it replaces a startup re-resolution with a cheap
local pass.
Second, a lockfile can be flagged as changed for reasons unrelated to npm,
most notably a
linkssection that older deno versions did not write andthat is re-derived from the workspace config on load. That flag forced a
full npm re-resolution. The resolver and installer fast paths now skip npm
resolution when the lockfile snapshot satisfies all requirements and
either the lockfile is unchanged or we are offline (
--cached-only).Online resolution is unchanged, so the lockfile is still brought up to
date when it needs to be. Off the offline path the only added work is a
single cache-setting comparison.
Adds spec tests covering a links-only lockfile change run offline (both
the
deno.jsonimports and thepackage.jsondependency forms) and aunit test covering the old peer-dep encoding being deduped on load.