diff --git a/.changeset/share-inflight-packuments.md b/.changeset/share-inflight-packuments.md new file mode 100644 index 0000000000..e7d5e8f799 --- /dev/null +++ b/.changeset/share-inflight-packuments.md @@ -0,0 +1,6 @@ +--- +"@pnpm/resolving.npm-resolver": patch +"pnpm": patch +--- + +Fixed an out-of-memory regression when workspace projects concurrently resolve a package with large registry metadata [pnpm/pnpm#13077](https://github.com/pnpm/pnpm/issues/13077). diff --git a/pnpm11/resolving/npm-resolver/src/fetch.ts b/pnpm11/resolving/npm-resolver/src/fetch.ts index 959c6aafda..bfcedce2a6 100644 --- a/pnpm11/resolving/npm-resolver/src/fetch.ts +++ b/pnpm11/resolving/npm-resolver/src/fetch.ts @@ -40,10 +40,10 @@ export interface FetchMetadataResult { meta: PackageMeta /** * The raw registry response body, used only to mirror the response to disk - * without re-serializing `meta`. A fresh fetch always sets it, but it - * reaches only the caller that initiated the request: the phase-long memo - * cache holds a body-less clone (see memoizeFetchMetadata.ts), so cache - * hits see `undefined` and the cache never pins the body. + * without re-serializing `meta`. A fresh fetch always sets it, and every + * caller sharing that in-flight request sees it. Once the request settles + * the phase-long memo cache drops the body (see memoizeFetchMetadata.ts), + * so later cache hits see `undefined` and the cache never pins the body. */ jsonText: string | undefined etag?: string diff --git a/pnpm11/resolving/npm-resolver/src/memoizeFetchMetadata.ts b/pnpm11/resolving/npm-resolver/src/memoizeFetchMetadata.ts index 51780bf48d..0abbaf380f 100644 --- a/pnpm11/resolving/npm-resolver/src/memoizeFetchMetadata.ts +++ b/pnpm11/resolving/npm-resolver/src/memoizeFetchMetadata.ts @@ -16,14 +16,20 @@ export interface MemoizedFetchMetadata { * `clear`, see `clearResolutionCache`), deduplicating concurrent and repeat * requests for the same package. * - * Unlike plain memoization, the cache holds a body-less clone of each result: - * `jsonText` — the raw registry response body, up to tens of MB for a popular - * package — reaches only the caller that initiated the fetch, which is the - * caller that writes the disk mirror. A phase-long cache that kept the bodies - * would pin hundreds of MB on large cold-cache graphs. A cache-hit caller - * that also writes the mirror falls back to `JSON.stringify(meta)` in - * `prepareJsonForDisk`, which is equivalent on read: `loadMeta` re-derives - * `etag` from the headers line. + * Unlike plain memoization, the entry is swapped for a body-less clone once + * the request settles. `jsonText` — the raw registry response body, up to tens + * of MB for a popular package — reaches every caller sharing the in-flight + * request, so a package resolved by many workspace projects at once mirrors + * that one body to disk instead of each project separately re-serializing + * `meta`. Retaining bodies past settlement would pin hundreds of MB on large + * cold-cache graphs, so a later cache-hit caller that writes the mirror falls + * back to `JSON.stringify(meta)` in `prepareJsonForDisk`, which is equivalent + * on read: `loadMeta` re-derives `etag` from the headers line. + * + * Because that swap lands a turn after the request settles, both settlement + * paths write back only while the entry is still their own promise — a `clear` + * (or a retry that already replaced the entry) must not be undone by a request + * that was in flight when it happened. * * A rejected fetch is evicted so a transient network failure is retried by * the next request instead of being cached for the rest of the phase. @@ -36,11 +42,21 @@ export function memoizeFetchMetadata (fetch: FetchMetadata): MemoizedFetchMetada const cached = cache.get(key) if (cached != null) return cached const pending = fetch(pkgName, opts) - const bodiless = pending.then((result) => - result.notModified ? result : { ...result, jsonText: undefined } + cache.set(key, pending) + void pending.then( + (result) => { + if (cache.get(key) !== pending) return + cache.set( + key, + Promise.resolve( + result.notModified ? result : { ...result, jsonText: undefined } + ) + ) + }, + () => { + if (cache.get(key) === pending) cache.delete(key) + } ) - bodiless.catch(() => cache.delete(key)) - cache.set(key, bodiless) return pending }, clear: () => { diff --git a/pnpm11/resolving/npm-resolver/test/memoizeFetchMetadata.test.ts b/pnpm11/resolving/npm-resolver/test/memoizeFetchMetadata.test.ts index a1083636b1..40bd6c6ace 100644 --- a/pnpm11/resolving/npm-resolver/test/memoizeFetchMetadata.test.ts +++ b/pnpm11/resolving/npm-resolver/test/memoizeFetchMetadata.test.ts @@ -37,7 +37,7 @@ test('the initiating caller receives the raw body; cache hits get a body-less cl expect(result.jsonText).toBe('{"name":"foo"}') }) -test('a caller arriving while the fetch is in flight gets the body-less clone', async () => { +test('callers sharing an in-flight fetch receive the same raw body', async () => { let release!: (result: FetchMetadataResult) => void const { fetch } = memoizeFetchMetadata(async () => new Promise((resolve) => { release = resolve @@ -50,7 +50,7 @@ test('a caller arriving while the fetch is in flight gets the body-less clone', const [first, second] = await Promise.all([firstPromise, secondPromise]) if (first.notModified || second.notModified) throw new Error('expected fresh fetch results') expect(first.jsonText).toBe('{"name":"foo"}') - expect(second.jsonText).toBeUndefined() + expect(second).toBe(first) }) test('requests with different options are cached separately', async () => { @@ -81,6 +81,52 @@ test('a rejected fetch is evicted so the next request retries', async () => { expect(calls).toBe(2) }) +test('clear does not let an in-flight fetch repopulate the cache', async () => { + let calls = 0 + let release!: (result: FetchMetadataResult) => void + const { fetch, clear } = memoizeFetchMetadata(async () => { + calls++ + return new Promise((resolve) => { + release = resolve + }) + }) + + const firstPromise = fetch('foo', { registry: REGISTRY }) + clear() + release(fooFetchResult()) + await firstPromise + + const secondPromise = fetch('foo', { registry: REGISTRY }) + expect(calls).toBe(2) + release(fooFetchResult()) + await secondPromise +}) + +test('a rejected fetch does not evict the request that replaced it', async () => { + let calls = 0 + let release!: (result: FetchMetadataResult) => void + const { fetch, clear } = memoizeFetchMetadata(async () => { + calls++ + if (calls === 1) throw new Error('network down') + return new Promise((resolve) => { + release = resolve + }) + }) + + const firstPromise = fetch('foo', { registry: REGISTRY }) + clear() + const secondPromise = fetch('foo', { registry: REGISTRY }) + await expect(firstPromise).rejects.toThrow('network down') + + // The eviction must leave the second request's entry in place, so a third + // caller joins it instead of opening a redundant request. + const thirdPromise = fetch('foo', { registry: REGISTRY }) + expect(calls).toBe(2) + release(fooFetchResult()) + const [second, third] = await Promise.all([secondPromise, thirdPromise]) + expect(third).toBe(second) +}) + test('clear() empties the cache', async () => { let calls = 0 const { fetch, clear } = memoizeFetchMetadata(async () => { diff --git a/pnpm11/resolving/npm-resolver/test/metaCache.test.ts b/pnpm11/resolving/npm-resolver/test/metaCache.test.ts index 66659bde0b..5dd3642550 100644 --- a/pnpm11/resolving/npm-resolver/test/metaCache.test.ts +++ b/pnpm11/resolving/npm-resolver/test/metaCache.test.ts @@ -1,11 +1,13 @@ import { rmSync } from 'node:fs' import { readFile } from 'node:fs/promises' -import { expect, test } from '@jest/globals' +import { expect, jest, test } from '@jest/globals' import { ABBREVIATED_META_DIR } from '@pnpm/constants' import type { PackageMeta } from '@pnpm/resolving.registry.types' import { temporaryDirectory } from 'tempy' +import type { FetchMetadataOptions } from '../src/fetch.js' +import { memoizeFetchMetadata } from '../src/memoizeFetchMetadata.js' import type { RegistryPackageSpec } from '../src/parseBareSpecifier.js' import { getPkgMetaCacheKey, @@ -191,6 +193,52 @@ test('the raw response body is written verbatim to the disk mirror', async () => expect(mirror?.slice(mirror.indexOf('\n') + 1)).toBe(rawBody) }) +test('projects sharing one in-flight fetch mirror the fetched body instead of re-serializing it', async () => { + const meta = fooMeta() + const rawBody = JSON.stringify(meta) + const cacheDir = temporaryDirectory() + const projects = 20 + + let fetches = 0 + let joined = 0 + let allJoined!: () => void + const inFlight = new Promise((resolve) => { + allJoined = resolve + }) + const memoized = memoizeFetchMetadata(async () => { + fetches++ + // Hold the request open until every project has joined it, so the fan-out + // this guards against is reproduced rather than raced for. + await inFlight + return { meta, jsonText: rawBody, etag: undefined } + }) + const ctx = { + fetch: async (pkgName: string, opts: FetchMetadataOptions) => { + if (++joined === projects) allJoined() + return memoized.fetch(pkgName, opts) + }, + metaCache: createMetaCache(), + cacheDir, + } + const spec: RegistryPackageSpec = { type: 'range', name: 'foo', fetchSpec: '^1.0.0' } + + const stringifySpy = jest.spyOn(JSON, 'stringify') + try { + const picks = await Promise.all(Array.from({ length: projects }, async () => + pickPackage(ctx, spec, { registry: REGISTRY, dryRun: false, preferredVersionSelectors: undefined }) + )) + expect(picks.every((pick) => pick.pickedPackage?.version === '1.0.0')).toBe(true) + expect(fetches).toBe(1) + // Re-serializing per project is what exhausted the heap: the body reaches + // tens of MB for a popular package, and every project holds its own copy + // until the mirror write limiter drains. + const serializations = stringifySpy.mock.calls.filter(([value]) => value === meta).length + expect(serializations).toBe(0) + } finally { + stringifySpy.mockRestore() + } +}) + test('a disk-promoted cache entry that cannot satisfy the spec falls back to the registry under prefer-offline', async () => { const staleMeta = fooMeta() const freshMeta = fooMeta()