fix(npm-resolver): minimumReleaseAge handling for cached abbreviated metadata #11622
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR upgrades cached abbreviated npm metadata to full metadata when maturity checks (publishedBy/minimumReleaseAge) require per-version ChangesAbbreviated Metadata Upgrade for Maturity Checks
Sequence Diagram(s): sequenceDiagram
participant PickPackage
participant MetaCache
participant DiskCache
participant Registry
PickPackage->>MetaCache: read cached meta (abbreviated?)
alt abbreviated meta and publishedBy/minimumReleaseAge active
MetaCache->>PickPackage: abbreviated meta missing per-version time
PickPackage->>Registry: fetch full metadata
Registry->>PickPackage: return full metadata with time
PickPackage->>MetaCache: update in-memory cache with full meta
alt 304 path persistence
PickPackage->>DiskCache: persist upgraded full meta to disk
end
else use cached meta
MetaCache->>PickPackage: use cached meta
end
PickPackage->>PickPackage: pickMatchingVersionFinal using chosen meta
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
There was a problem hiding this comment.
Pull request overview
Fixes a bug in pickPackage where the spec.type === 'version' cache fast path rethrew ERR_PNPM_MISSING_TIME under strictPublishedByCheck instead of falling through to the registry fetch. The fix makes this catch block consistent with the adjacent mtime-gated cache block, which already filters out missing-time errors. This surfaced after #11436 made minimumReleaseAgeStrict default to true when users explicitly set minimumReleaseAge.
Changes:
- Added
isMissingTimeErrorfilter to the version-spec cachecatchblock inpickPackage.ts. - Added a regression test in
publishedBy.test.tsthat stripstimefrom cached abbreviated metadata and asserts an exact-version resolve understrictPublishedByCheck: truefalls through to the registry fetch. - Added a patch-level changeset describing the fix.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
resolving/npm-resolver/src/pickPackage.ts |
Filters ERR_PNPM_MISSING_TIME in the version-spec cache catch block so it falls through to the network fetch, matching the sibling mtime-gated block. |
resolving/npm-resolver/test/publishedBy.test.ts |
Adds a regression test exercising the version-spec cache path with stripped time and strictPublishedByCheck: true. |
.changeset/fix-missing-time-rethrown-from-version-spec-cache.md |
Patch changeset for @pnpm/resolving.npm-resolver and pnpm. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Heey I think I found it @zkochan! When pnpm gets a 304 Not Modified for a package whose disk cache holds abbreviated metadata, it reuses that cached form as-is and never asks the registry for the full document. The downstream |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@resolving/npm-resolver/src/pickPackage.ts`:
- Around line 183-221: The abbreviated-metadata upgrade currently only runs for
304 responses but misses non-304 cache hits and other early-return code paths
(ctx.metaCache, preferOffline / pickLowestVersion) which call
pickMatchingVersionFinal on abbreviated metadata and can trigger
minimumReleaseAge / ERR_PNPM_MISSING_TIME; update those paths to use
maybeUpgradeAbbreviatedMetaForReleaseAge(spec, opts, meta) or, if meta.time is
missing, synchronously fetch full metadata via ctx.fetch(spec.name, {
fullMetadata: true, authHeaderValue: opts.authHeaderValue, registry:
opts.registry }) before calling pickMatchingVersionFinal (and ensure the
upgraded FetchMetadataResult is passed through so upgradedFrom is preserved), so
any branch that would operate on abbreviated meta (including preferOffline,
pickLowestVersion and ctx.metaCache usage) retries with fullMetadata when time
is absent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 21a3565f-0fc1-4a39-83bd-4caef4e3a823
📒 Files selected for processing (3)
.changeset/fix-minimum-release-age-cached-abbreviated-metadata.mdresolving/npm-resolver/src/pickPackage.tsresolving/npm-resolver/test/publishedBy.test.ts
✅ Files skipped from review due to trivial changes (1)
- .changeset/fix-minimum-release-age-cached-abbreviated-metadata.md
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{js,ts,tsx,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,tsx,jsx}: Use trailing commas in code following Standard Style with modifications
Prefer functions over classes
Declare functions after they are used, relying on hoisting
Functions should have no more than two or three arguments; use a single options object for functions needing more parameters
Organize imports in order: standard libraries, external dependencies (sorted alphabetically), then relative imports
Files:
resolving/npm-resolver/test/publishedBy.test.tsresolving/npm-resolver/src/pickPackage.ts
**/*.test.{js,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Do not use
instanceof Errorfor error type checking in Jest tests; useutil.types.isNativeError()instead to ensure compatibility across realms
Files:
resolving/npm-resolver/test/publishedBy.test.ts
🔇 Additional comments (3)
resolving/npm-resolver/src/pickPackage.ts (1)
309-316: LGTM!Also applies to: 374-395
resolving/npm-resolver/test/publishedBy.test.ts (2)
323-378: LGTM!
437-439: ⚡ Quick winThe
retryLoadJsonFile()function is explicitly NDJSON-aware viaparseNdjsonMeta(), which correctly parses the second line (body) whileprepareJsonForDisk()writes the two-line format. The assertion properly validates the persisted metadata and will correctly read thetimefield from the body, not the headers.> Likely an incorrect or invalid review comment.
4d84184 to
dbdbc3f
Compare
Benchmark Results
Run 25849991479 · 10 runs per scenario · triggered by @zkochan |
- Extract `persistUpgradedMeta` helper and call it from all three sites (in-memory cache hit, preferOffline disk-cache hit, 304 path) so a fresh process doesn't repeat the upgrade fetch. - Forward `etag`/`modified` to the upgrade fetch in `maybeUpgradeAbbreviatedMetaForReleaseAge` so the registry can answer 304. - Extract `shouldRethrowFromFastPathCache` so the two fast-path catch sites can't drift on the MISSING_TIME-vs-strict invariant. - Document the deliberate choice to upgrade-fetch when `meta.modified` is absent or unparseable (correctness over saving a network call). - Add a companion test that exercises the catch fix with the default `ignoreMissingTimeField` so the invariant holds regardless of that flag. - Fix the existing `bareSpecifier: '3.1.0'` test setup: 3.1.0 was published 2016-01-11, after the test's `publishedBy` of 2015-08-17, so strict mode correctly rejected it. Switch to 3.0.0 (released 2015-07-10).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@resolving/npm-resolver/test/publishedBy.test.ts`:
- Around line 350-354: The multiline fs.writeFileSync calls (e.g., the call that
writes to cachePath using cachedMeta) are missing trailing commas on the final
argument; update those calls (including the similar call around lines 395-399)
to add a trailing comma after the 'utf8' argument so they conform to the
project's trailing-comma style rule (locate the fs.writeFileSync(...)
invocations that reference cachePath and cachedMeta and append the comma).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 91833fdf-da85-4bd4-857d-c4e02ea77daf
📒 Files selected for processing (2)
resolving/npm-resolver/src/pickPackage.tsresolving/npm-resolver/test/publishedBy.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- resolving/npm-resolver/src/pickPackage.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{js,ts,tsx,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,tsx,jsx}: Use trailing commas in code following Standard Style with modifications
Prefer functions over classes
Declare functions after they are used, relying on hoisting
Functions should have no more than two or three arguments; use a single options object for functions needing more parameters
Organize imports in order: standard libraries, external dependencies (sorted alphabetically), then relative imports
Files:
resolving/npm-resolver/test/publishedBy.test.ts
**/*.test.{js,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Do not use
instanceof Errorfor error type checking in Jest tests; useutil.types.isNativeError()instead to ensure compatibility across realms
Files:
resolving/npm-resolver/test/publishedBy.test.ts
🔇 Additional comments (1)
resolving/npm-resolver/test/publishedBy.test.ts (1)
13-13: LGTM!Also applies to: 323-349, 356-394, 401-480
|
@coderabbitai approve |
✅ Actions performedComments resolved and changes approved. |
|
Congrats on merging your first pull request! 🎉🎉🎉 |
|
Thank you @zkochan |
…metadata (#11630) Backport of #11622 to release/10. The npm registry returns abbreviated package metadata (without per-version `time`) by default, which made the maturity check throw ERR_PNPM_MISSING_TIME whenever cached abbreviated metadata was reused under `publishedBy`/`minimumReleaseAge`. pnpm now upgrades cached abbreviated metadata to the full document via a follow-up fetch, persists the upgrade to the on-disk cache so subsequent installs skip the extra fetch, and lets ERR_PNPM_MISSING_TIME from the cache fast path fall through to the network fetch even under strict mode. Adapted to release/10's simpler pickPackage shape (no 304/etag plumbing, no `pickMatchingVersionFast`/`pickMatchingVersionFinal` split, no `ignoreMissingTimeField`), so the unconditional helper triggers the upgrade whenever the cached meta lacks `time`.
Update pnpm to 11.1.2 so that it contains [the fix](pnpm/pnpm#11622) for the "missing time" bug.
Summary
Two related bugs in how
pickPackagehandlesminimumReleaseAgewhen local metadata is the abbreviated form (no per-versiontime). Both are contained toresolving/npm-resolver/src/pickPackage.ts.Bug 1 —
ERR_PNPM_MISSING_TIMErethrown from the version-spec cache pathThe version-spec cache fast path (
spec.type === 'version') wrapspickMatchingVersionFastin atry/catchthat rethrows understrictPublishedByCheck. The adjacent mtime-gated cache block filters outERR_PNPM_MISSING_TIMEbefore rethrowing; the version-spec block does not. Both blocks share the same intent ("try cached fast path, fall through to the registry fetch on failure"), so this is a missing filter, not a design choice.Surfaced as a hard error in workspaces with
minimumReleaseAgeset, because v11.0.4 (#11436 —42a8f29f41on the 11.0.x branch,e3ccf6b134on main) mademinimumReleaseAgeStrict: truethe default when the user explicitly setsminimumReleaseAge.Fix: add
!isMissingTimeError(err)to the catch filter, matching the adjacent block.Bug 2 — silent bypass via 304 Not Modified
After Bug 1, the version-spec catch swallows
MISSING_TIMEand falls through to the network fetch. If the registry returns 304 Not Modified (etag still valid), pnpm reuses the cached abbreviated metadata and never upgrades to full. The downstreampickMatchingVersionFinalthen takes its warn-and-skip path — emitting a misleading "metadata is missing the time field" warning and silently bypassing the maturity check.This is the same family as #11619: registry has
timeavailable in the full format, but pnpm never asks for it from cached paths.Fix: at the 304 path, if the cached meta is abbreviated and
minimumReleaseAgewould needtime, re-fetch withfullMetadata: trueand persist the upgraded meta to disk so subsequent installs don't repeat the fetch. Factored into a helper (maybeUpgradeAbbreviatedMetaForReleaseAge) that mirrors the upgrade condition already present in the post-fetch path.Reproduction
>= 11.0.4.pnpm-workspace.yaml:package.jsonpinning an exact version whose abbreviated metadata is cached locally (e.g."vitest": "4.1.4"after a prior install).pnpm install→[ERR_PNPM_MISSING_TIME] The metadata of vitest is missing the "time" field, even thoughnpm view vitest timeshows the registry has it.Workaround that confirms the strict path:
minimumReleaseAgeStrict: false.Tests
Two regression tests added to
resolving/npm-resolver/test/publishedBy.test.ts:strictPublishedByCheck=true does not rethrow ERR_PNPM_MISSING_TIME from the version-spec cache path— covers Bug 1.upgrades cached abbreviated metadata to full when 304 Not Modified and publishedBy is set— covers Bug 2, including the disk-persistence assertion.Both fail before their respective fix and pass after.
Related
lodashmissing-time false warning is the same family as Bug 2.timeentry causes incorrectminimumReleaseAgefailure when usingtime-basedresolution #11616 — sibling symptom in time-based resolution; overlapping family.publishedAtpropagation in theresolveNpmfast path; different code path, no overlap.Summary by CodeRabbit
Bug Fixes
Tests