Skip to content

Commit 2a9bd89

Browse files
zkochanzkochan
authored
perf: record locally-resolved lockfile in verification cache (#11714)
The lockfile verification cache currently only records the lockfile that exists at the **start** of an install. So a flow like: ``` pnpm install <pkg> rm -rf node_modules pnpm install ``` re-runs the per-package registry round-trip against the newly written lockfile, even though the local resolver already enforced the policy when picking those versions. The fresh lockfile is now recorded immediately after each install-time write, so the second install takes the cache fast path. ## Implementation ### Recording the post-resolution lockfile - New helper `recordLockfileVerified` (in `installing/deps-installer/src/install/`). Gated on `cacheDir` + non-empty `resolutionVerifiers` — same gate the pre-resolution verifier uses. - Two thin combiners over the lockfile writers: `writeWantedLockfileAndRecordVerified` and `writeLockfilesAndRecordVerified`. The install paths use these so the record always runs alongside the write. ### Hash stability: writer returns the canonical lockfile The cache stores `hashObject(LockfileObject)` and the next install computes the same hash off the file it loads from disk. For the hashes to match, both ends must compute over structurally identical objects. They don't, naïvely: the in-memory write object can carry `undefined` optional fields (e.g. `settings.dedupePeers = undefined` from `opts.dedupePeers || undefined` in install code) that YAML drops on serialize — `object-hash` treats undefined vs missing as distinct values. - `writeWantedLockfile` / `writeLockfiles` (in `@pnpm/lockfile.fs`) now return the canonical post-write `LockfileObject`: `convertToLockfileObject(stripUndefinedDeep(lockfileFile))`. The strip walks the existing object graph in memory rather than going through a `yaml.load` round-trip, so non-cache callers (deploy, deps-restorer, make-dedicated-lockfile, agent server) pay near-zero cost. - Install hooks hash the writer's returned value, not the raw in-memory input. Guaranteed by construction to match what the next reader produces. ### `useGitBranchLockfile` correctness The pre-resolution verification gate and the new post-write recorder were both keying cache records on a hard-coded `pnpm-lock.yaml`. Under `useGitBranchLockfile` the actual file is `pnpm-lock.<branch>.yaml`, so the stat shortcut hit `ENOENT` and the cache effectively never engaged for git-branch users. Both sites now resolve the real filename via `getWantedLockfileName`. The wrappers compute it once and pass it to the writer via a new optional `lockfileName` opt so `useGitBranchLockfile` installs don't fork `getCurrentBranch` twice per write. ### Bug fix unrelated to the cache, found during review `writeLockfiles`' differs branch was deciding whether to remove or keep `node_modules/.pnpm/lock.yaml` based on `isEmptyLockfile(wantedLockfile)`. Filtered-current callers (deps-restorer) pass an empty current against a non-empty wanted, so this could leave a stale current lockfile on disk. Fixed to key off the current. ### Comments policy `AGENTS.md` (and `pacquet/AGENTS.md`) now spell out the comment defaults: write self-documenting code, do not restate at call sites what the callee's JSDoc / doc comment already says, comments are reserved for the non-obvious *why*. The pruning pass in this PR brings the changed code in line. ## API surface - `@pnpm/lockfile.fs` (minor): - `writeWantedLockfile`: return widened from `Promise<void>` to `Promise<LockfileObject>`. New optional `lockfileName` opt. - `writeCurrentLockfile`: return widened to `Promise<LockfileObject | undefined>` (undefined when the empty-lockfile branch unlinks). - `writeLockfiles`: return widened from `Promise<void>` to `Promise<{ wantedLockfile, currentLockfile }>`. New optional `wantedLockfileName` opt. New exported `WriteLockfilesResult` type. - New export: `getWantedLockfileName`. - `@pnpm/installing.deps-installer` (patch): internal-only wrappers; no external API change.
1 parent e2a3c68 commit 2a9bd89

14 files changed

Lines changed: 588 additions & 19 deletions

File tree

‎.changeset/record-locally-resolved-lockfile-verified.md‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
---
2+
"@pnpm/installing.deps-installer": patch
3+
"@pnpm/lockfile.fs": minor
4+
"pnpm": patch
5+
---
6+
7+
Record the post-resolution lockfile in the verification cache. Previously the cache only captured the lockfile that was loaded at the start of an install, so a flow like `pnpm install <pkg>` followed by `rm -rf node_modules && pnpm install` re-ran the per-package registry round-trip against the newly written lockfile even though the local resolver had already enforced the policy when picking those versions. The fresh lockfile is now recorded immediately after each install-time write, so the second install takes the cache fast path.

‎AGENTS.md‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -186,6 +186,23 @@ To ensure your code adheres to the style guide, run:
186186
pnpm run lint
187187
```
188188

189+
### Comments
190+
191+
Write code that explains itself. A reader should understand what a function does from its name, parameters, and types — not from prose above the call site.
192+
193+
Defaults:
194+
195+
- **Do not write a comment** that restates what the code already says. If renaming a variable, splitting a helper, or moving a check to a more obvious place would carry the information, do that instead.
196+
- **Do not repeat documentation** at call sites that already lives on the callee. If the function has a JSDoc, the call site shouldn't re-explain what calling it does. Update the JSDoc once; let every call site benefit.
197+
- **JSDoc is for the function's contract** — preconditions, postconditions, edge cases, why the function exists. Not for re-narrating the body.
198+
199+
Write a comment only when:
200+
201+
- The reason for the code is non-obvious from reading it (a hidden invariant, a workaround for a known bug, a deliberate exception to the surrounding pattern).
202+
- The right name doesn't fit — e.g., a temporary technical constraint that's worth flagging but doesn't justify a new symbol.
203+
204+
Before adding a comment, ask: "Could I rename, restructure, or extract instead?" If yes, do that. The bar for prose-in-code is high; the bar for prose-that-restates-code is "don't."
205+
189206
## Common Gotchas
190207

191208
### Error Type Checking in Jest (TypeScript only)

‎installing/deps-installer/src/install/index.ts‎

Lines changed: 29 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,7 @@ import { writeModulesManifest } from '@pnpm/installing.modules-yaml'
4242
import {
4343
type CatalogSnapshots,
4444
cleanGitBranchLockfiles,
45+
getWantedLockfileName,
4546
type LockfileObject,
4647
type ProjectSnapshot,
4748
readWantedLockfile,
@@ -97,6 +98,8 @@ import { linkPackages } from './link.js'
9798
import { reportPeerDependencyIssues } from './reportPeerDependencyIssues.js'
9899
import { validateModules } from './validateModules.js'
99100
import { verifyLockfileResolutions } from './verifyLockfileResolutions.js'
101+
import { writeLockfilesAndRecordVerified } from './writeLockfilesAndRecordVerified.js'
102+
import { writeWantedLockfileAndRecordVerified } from './writeWantedLockfileAndRecordVerified.js'
100103

101104
class LockfileConfigMismatchError extends PnpmError {
102105
constructor (outdatedLockfileSettingName: string) {
@@ -358,10 +361,17 @@ export async function mutateModules (
358361
// resolver's own filters already cover fresh resolution. We run this
359362
// exactly once, right after the lockfile is loaded from disk, before any
360363
// path branches.
364+
const cacheActive = opts.cacheDir != null && opts.resolutionVerifiers.length > 0
365+
const wantedLockfilePath = cacheActive
366+
? path.resolve(ctx.lockfileDir, await getWantedLockfileName({
367+
useGitBranchLockfile: opts.useGitBranchLockfile,
368+
mergeGitBranchLockfiles: opts.mergeGitBranchLockfiles,
369+
}))
370+
: undefined
361371
try {
362372
await verifyLockfileResolutions(ctx.wantedLockfile, opts.resolutionVerifiers, {
363373
cacheDir: opts.cacheDir,
364-
lockfilePath: path.resolve(ctx.lockfileDir, WANTED_LOCKFILE),
374+
lockfilePath: wantedLockfilePath,
365375
})
366376
} catch (err) {
367377
// verifyLockfileResolutions is the one throw site in this function
@@ -1657,11 +1667,13 @@ const _installInContext: InstallFunction = async (projects, ctx, opts) => {
16571667
const currentLockfileDir = path.join(ctx.rootModulesDir, '.pnpm')
16581668
await Promise.all([
16591669
opts.useLockfile && opts.saveLockfile
1660-
? writeLockfiles({
1670+
? writeLockfilesAndRecordVerified({
16611671
currentLockfile: result.currentLockfile,
16621672
currentLockfileDir,
16631673
wantedLockfile: newLockfile,
16641674
wantedLockfileDir: ctx.lockfileDir,
1675+
cacheDir: opts.cacheDir,
1676+
resolutionVerifiers: opts.resolutionVerifiers,
16651677
...lockfileOpts,
16661678
})
16671679
: writeCurrentLockfile(ctx.virtualStoreDir, result.currentLockfile),
@@ -1716,7 +1728,13 @@ const _installInContext: InstallFunction = async (projects, ctx, opts) => {
17161728
}
17171729
} else {
17181730
if (opts.useLockfile && opts.saveLockfile && !isInstallationOnlyForLockfileCheck) {
1719-
await writeWantedLockfile(ctx.lockfileDir, newLockfile, lockfileOpts)
1731+
await writeWantedLockfileAndRecordVerified({
1732+
lockfileDir: ctx.lockfileDir,
1733+
lockfile: newLockfile,
1734+
cacheDir: opts.cacheDir,
1735+
resolutionVerifiers: opts.resolutionVerifiers,
1736+
...lockfileOpts,
1737+
})
17201738
}
17211739

17221740
if (opts.nodeLinker !== 'hoisted') {
@@ -2264,7 +2282,14 @@ async function installFromPnpmRegistry (
22642282
storeIndex.close()
22652283
}
22662284

2267-
await writeWantedLockfile(lockfileDir, lockfile)
2285+
await writeWantedLockfileAndRecordVerified({
2286+
lockfileDir,
2287+
lockfile,
2288+
cacheDir: opts.cacheDir,
2289+
resolutionVerifiers: opts.resolutionVerifiers,
2290+
useGitBranchLockfile: opts.useGitBranchLockfile,
2291+
mergeGitBranchLockfiles: opts.mergeGitBranchLockfiles,
2292+
})
22682293

22692294
logger.info({
22702295
message: `Resolved ${agentStats.totalPackages} packages: ${agentStats.alreadyInStore} cached, ${agentStats.filesToDownload} files to download`,

‎installing/deps-installer/src/install/recordLockfileVerified.ts‎

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
import { hashObject } from '@pnpm/crypto.object-hasher'
2+
import type { LockfileObject } from '@pnpm/lockfile.fs'
3+
import type { ResolutionVerifier } from '@pnpm/resolving.resolver-base'
4+
5+
import { recordVerification } from './verifyLockfileResolutionsCache.js'
6+
7+
export interface RecordLockfileVerifiedOptions {
8+
cacheDir?: string
9+
/** Absolute path of the lockfile the next install will read.
10+
* Under `useGitBranchLockfile` this is the branch-suffixed name. */
11+
lockfilePath: string
12+
/** The writer's canonical return value — see {@link writeWantedLockfile}.
13+
* Passing the raw in-memory write object would record a hash the
14+
* next install can't match (YAML drops undefined fields). */
15+
lockfile: LockfileObject
16+
resolutionVerifiers: readonly ResolutionVerifier[] | undefined
17+
}
18+
19+
/**
20+
* Records the post-resolution lockfile as verified so the next install
21+
* skips the registry round-trip. Skipping is safe: fresh local picks
22+
* are filtered by the resolver (see
23+
* `resolving/npm-resolver/src/pickPackage.ts`) and carried-over entries
24+
* already passed the gate at the top of `mutateModules`, so the
25+
* recorded lockfile is policy-clean by construction.
26+
*/
27+
export function recordLockfileVerified (opts: RecordLockfileVerifiedOptions): void {
28+
if (!opts.cacheDir) return
29+
if (!opts.resolutionVerifiers?.length) return
30+
if (!opts.lockfile.packages) return
31+
recordVerification(opts.cacheDir, {
32+
lockfilePath: opts.lockfilePath,
33+
verifiers: opts.resolutionVerifiers,
34+
hashLockfile: () => hashObject(opts.lockfile),
35+
})
36+
}

‎installing/deps-installer/src/install/verifyLockfileResolutions.ts‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -97,8 +97,7 @@ export async function verifyLockfileResolutions (
9797
// miss the precomputed stat+hash flow to recordVerification.
9898
type Precomputed = ReturnType<typeof tryLockfileVerificationCache>['precomputed']
9999
let cachePrecomputed: Precomputed | undefined
100-
// hashObject is streaming (no "Invalid string length" on huge lockfiles)
101-
// and key-order-stable, which JSON.stringify is not.
100+
// hashObject streams and is key-order-stable, unlike JSON.stringify.
102101
let cachedHash: string | undefined
103102
const hashLockfile = (): string => {
104103
if (cachedHash == null) cachedHash = hashObject(lockfile)

‎installing/deps-installer/src/install/writeLockfilesAndRecordVerified.ts‎

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,48 @@
1+
import path from 'node:path'
2+
3+
import { getWantedLockfileName, type LockfileObject, writeLockfiles, type WriteLockfilesResult } from '@pnpm/lockfile.fs'
4+
import type { ResolutionVerifier } from '@pnpm/resolving.resolver-base'
5+
6+
import { recordLockfileVerified } from './recordLockfileVerified.js'
7+
8+
export interface WriteLockfilesAndRecordVerifiedOptions {
9+
wantedLockfile: LockfileObject
10+
wantedLockfileDir: string
11+
currentLockfile: LockfileObject
12+
currentLockfileDir: string
13+
useGitBranchLockfile?: boolean
14+
mergeGitBranchLockfiles?: boolean
15+
cacheDir?: string
16+
resolutionVerifiers: readonly ResolutionVerifier[] | undefined
17+
}
18+
19+
/** Plural counterpart of {@link writeWantedLockfileAndRecordVerified}. */
20+
export async function writeLockfilesAndRecordVerified (
21+
opts: WriteLockfilesAndRecordVerifiedOptions
22+
): Promise<WriteLockfilesResult> {
23+
const cacheActive = opts.cacheDir != null && (opts.resolutionVerifiers?.length ?? 0) > 0
24+
const wantedLockfileName = cacheActive
25+
? await getWantedLockfileName({
26+
useGitBranchLockfile: opts.useGitBranchLockfile,
27+
mergeGitBranchLockfiles: opts.mergeGitBranchLockfiles,
28+
})
29+
: undefined
30+
const written = await writeLockfiles({
31+
wantedLockfile: opts.wantedLockfile,
32+
wantedLockfileDir: opts.wantedLockfileDir,
33+
currentLockfile: opts.currentLockfile,
34+
currentLockfileDir: opts.currentLockfileDir,
35+
useGitBranchLockfile: opts.useGitBranchLockfile,
36+
mergeGitBranchLockfiles: opts.mergeGitBranchLockfiles,
37+
wantedLockfileName,
38+
})
39+
if (cacheActive) {
40+
recordLockfileVerified({
41+
cacheDir: opts.cacheDir,
42+
lockfilePath: path.resolve(opts.wantedLockfileDir, wantedLockfileName!),
43+
lockfile: written.wantedLockfile,
44+
resolutionVerifiers: opts.resolutionVerifiers,
45+
})
46+
}
47+
return written
48+
}

‎installing/deps-installer/src/install/writeWantedLockfileAndRecordVerified.ts‎

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,42 @@
1+
import path from 'node:path'
2+
3+
import { getWantedLockfileName, type LockfileObject, writeWantedLockfile } from '@pnpm/lockfile.fs'
4+
import type { ResolutionVerifier } from '@pnpm/resolving.resolver-base'
5+
6+
import { recordLockfileVerified } from './recordLockfileVerified.js'
7+
8+
export interface WriteWantedLockfileAndRecordVerifiedOptions {
9+
lockfileDir: string
10+
lockfile: LockfileObject
11+
cacheDir?: string
12+
resolutionVerifiers: readonly ResolutionVerifier[] | undefined
13+
useGitBranchLockfile?: boolean
14+
mergeGitBranchLockfiles?: boolean
15+
}
16+
17+
/** Combines {@link writeWantedLockfile} and {@link recordLockfileVerified} — see each for semantics. */
18+
export async function writeWantedLockfileAndRecordVerified (
19+
opts: WriteWantedLockfileAndRecordVerifiedOptions
20+
): Promise<LockfileObject> {
21+
const cacheActive = opts.cacheDir != null && (opts.resolutionVerifiers?.length ?? 0) > 0
22+
const lockfileName = cacheActive
23+
? await getWantedLockfileName({
24+
useGitBranchLockfile: opts.useGitBranchLockfile,
25+
mergeGitBranchLockfiles: opts.mergeGitBranchLockfiles,
26+
})
27+
: undefined
28+
const written = await writeWantedLockfile(opts.lockfileDir, opts.lockfile, {
29+
useGitBranchLockfile: opts.useGitBranchLockfile,
30+
mergeGitBranchLockfiles: opts.mergeGitBranchLockfiles,
31+
lockfileName,
32+
})
33+
if (cacheActive) {
34+
recordLockfileVerified({
35+
cacheDir: opts.cacheDir,
36+
lockfilePath: path.resolve(opts.lockfileDir, lockfileName!),
37+
lockfile: written,
38+
resolutionVerifiers: opts.resolutionVerifiers,
39+
})
40+
}
41+
return written
42+
}

0 commit comments

Comments
 (0)