fix: don't hang while reading package.json from the store

close #7051

.changeset/lazy-avocados-agree.md

@@ -0,0 +1,6 @@
+---
+"@pnpm/store.cafs": patch
+"pnpm": patch
+---
+
+Fixes a regression published with pnpm v8.7.3. Don't hang while reading `package.json` from the content-addressable store [#7051](https://github.com/pnpm/pnpm/pull/7051).

pnpm-lock.yaml

@@ -6044,9 +6044,6 @@ importers:
       load-json-file:
         specifier: ^6.2.0
         version: 6.2.0
-      safe-promise-defer:
-        specifier: ^1.0.1
-        version: 1.0.1
     devDependencies:
       '@pnpm/types':
         specifier: workspace:*
@@ -8959,8 +8956,8 @@ packages:
       '@types/lodash': 4.14.197
       '@types/semver': 7.3.13
       '@types/treeify': 1.0.0
-      '@yarnpkg/fslib': 3.0.0-rc.45
-      '@yarnpkg/libzip': 3.0.0-rc.45(@yarnpkg/fslib@3.0.0-rc.45)
+      '@yarnpkg/fslib': 3.0.0-rc.25
+      '@yarnpkg/libzip': 3.0.0-rc.25(@yarnpkg/fslib@3.0.0-rc.25)
       '@yarnpkg/parsers': 3.0.0-rc.45
       '@yarnpkg/shell': 4.0.0-rc.45(typanion@3.14.0)
       camelcase: 5.3.1
@@ -8998,22 +8995,22 @@ packages:
     engines: {node: '>=14.15.0'}
     dependencies:
       tslib: 2.6.2
-    dev: false
 
   /@yarnpkg/fslib@3.0.0-rc.45:
     resolution: {integrity: sha512-XarEtHTbeO4+rgZQObn16+uFzb1dXiVHuoqDNGoMywwBm/FF7DeCqSANeKiNYbi79WMgoLk8l3lO4g0vRYkJBg==}
     engines: {node: '>=14.15.0'}
     dependencies:
       tslib: 2.6.2
+    dev: false
 
-  /@yarnpkg/libzip@3.0.0-rc.45(@yarnpkg/fslib@3.0.0-rc.45):
-    resolution: {integrity: sha512-ZsYi6Y01yMJOLnJ5ISZgOFvCEXzp4EScrM91D7bvCx0lIfH3DZ40H4M5nGNeVFk7jXUHOXuJkNYlNoXixSconA==}
+  /@yarnpkg/libzip@3.0.0-rc.25(@yarnpkg/fslib@3.0.0-rc.25):
+    resolution: {integrity: sha512-YmG+oTBCyrAoMIx5g2I9CfyurSpHyoan+9SCj7laaFKseOe3lFEyIVKvwRBQMmSt8uzh+eY5RWeQnoyyOs6AbA==}
     engines: {node: '>=14.15.0'}
     peerDependencies:
       '@yarnpkg/fslib': 3.0.0-rc.45
     dependencies:
       '@types/emscripten': 1.39.7
-      '@yarnpkg/fslib': 3.0.0-rc.45
+      '@yarnpkg/fslib': 3.0.0-rc.25
       tslib: 2.6.2
 
   /@yarnpkg/lockfile@1.1.0:
@@ -9086,7 +9083,7 @@ packages:
     engines: {node: '>=14.15.0'}
     hasBin: true
     dependencies:
-      '@yarnpkg/fslib': 3.0.0-rc.45
+      '@yarnpkg/fslib': 3.0.0-rc.25
       '@yarnpkg/parsers': 3.0.0-rc.45
       chalk: 3.0.0
       clipanion: 3.2.0-rc.6(typanion@3.14.0)
@@ -9391,6 +9388,7 @@ packages:
 
   /aproba@2.0.0:
     resolution: {integrity: sha512-lYe4Gx7QT+MKGbDsA+Z+he/Wtef0BiwDOlK/XkBrdfsh9J/jPPXbX0tE9x9cl27Tmu5gg3QUbUrQYa/y+KOHPQ==}
+    requiresBuild: true
     dev: false
 
   /archy@1.0.0:
@@ -10118,6 +10116,7 @@ packages:
   /color-support@1.1.3:
     resolution: {integrity: sha512-qiBjkpbMLO/HL68y+lh4q0/O1MZFj2RX6X/KmMa3+gJD3z+WwI1ZzDHysvqHGS3mP6mznPckpXmw1nI9cJjyRg==}
     hasBin: true
+    requiresBuild: true
     dev: false
 
   /colors@0.6.2:
@@ -10558,6 +10557,7 @@ packages:
 
   /delegates@1.0.0:
     resolution: {integrity: sha512-bd2L678uiWATM6m5Z1VzNCErI3jiGzt6HGY8OVICs40JQq/HALfbyNJmp0UDakEY4pMMaN0Ly5om/B1VI/+xfQ==}
+    requiresBuild: true
 
   /depd@1.1.2:
     resolution: {integrity: sha512-7emPTl6Dpo6JRXOXjLRxck+FlLRX5847cLKEn00PLAgc3g2hTZZgr+e4c2v6QpSmLeFP3n5yUo7ft6avBK/5jQ==}
@@ -10751,6 +10751,7 @@ packages:
 
   /err-code@2.0.3:
     resolution: {integrity: sha512-2bmlRpNKBxT/CRmPOlyISQpNj+qSeYvcym/uT0Jx2bMOlKLtSy1ZmLuVxSEKKyor/N5yhvp/ZiG1oE3DEYMSFA==}
+    requiresBuild: true
     dev: false
 
   /error-ex@1.3.2:
@@ -11984,6 +11985,7 @@ packages:
 
   /has-unicode@2.0.1:
     resolution: {integrity: sha512-8Rf9Y83NBReMnx0gFzA8JImQACstCYWUplepDa9xprwwtmgEZUF0h/i5xSA625zB/I37EtrswSST6OXxwaaIJQ==}
+    requiresBuild: true
 
   /has@1.0.3:
     resolution: {integrity: sha512-f2dvO0VU6Oej7RkWJGrehjbzMAjFp5/VKPp5tTpWIV4JHHZK1/BxbFRtf/siA2SWTe09caDmVtYYzWEIbBS4zw==}
@@ -15826,6 +15828,7 @@ packages:
   /string-width@1.0.2:
     resolution: {integrity: sha512-0XsVpQLnVCXHJfyEs8tC0zpTVIr5PKKsQtkT29IwupnPTjtPmQ3xT/4yCREF9hYkV/3M3kzcUTSAZT6a6h81tw==}
     engines: {node: '>=0.10.0'}
+    requiresBuild: true
     dependencies:
       code-point-at: 1.1.0
       is-fullwidth-code-point: 1.0.0
@@ -16942,6 +16945,7 @@ packages:
 
   /wide-align@1.1.5:
     resolution: {integrity: sha512-eDMORYaPNZ4sQIuuYPDHdQvf4gyCF9rEEV/yPxGfwPkRodwEgiMUUXTx/dex+Me0wxx53S+NgUHaP7y3MGlDmg==}
+    requiresBuild: true
     dependencies:
       string-width: 4.2.3
 

store/cafs-types/src/index.ts

@@ -1,11 +1,6 @@
 import type { IntegrityLike } from 'ssri'
 import type { DependencyManifest } from '@pnpm/types'
 
-export interface DeferredManifestPromise {
-  resolve: (manifest: DependencyManifest | undefined) => void
-  reject: (err: Error) => void
-}
-
 export interface PackageFileInfo {
   checkedAt?: number // Nullable for backward compatibility
   integrity: string

store/cafs/src/checkPkgFilesIntegrity.ts

@@ -1,10 +1,11 @@
 import fs from 'fs'
-import type { DeferredManifestPromise, PackageFileInfo } from '@pnpm/cafs-types'
+import type { PackageFileInfo } from '@pnpm/cafs-types'
 import gfs from '@pnpm/graceful-fs'
+import { type DependencyManifest } from '@pnpm/types'
 import rimraf from '@zkochan/rimraf'
 import ssri from 'ssri'
 import { getFilePathByModeInCafs } from './getFilePathInCafs'
-import { parseJsonBuffer } from './parseJson'
+import { parseJsonBufferSync } from './parseJson'
 
 // We track how many files were checked during installation.
 // It should be rare that a files content should be checked.
@@ -13,6 +14,11 @@ import { parseJsonBuffer } from './parseJson'
 // @ts-expect-error
 global['verifiedFileIntegrity'] = 0
 
+export interface VerifyResult {
+  passed: boolean
+  manifest?: DependencyManifest
+}
+
 export interface PackageFilesIndex {
   // name and version are nullable for backward compatibility
   // the initial specs of pnpm store v3 did not require these fields.
@@ -28,52 +34,58 @@ export interface PackageFilesIndex {
 export function checkPkgFilesIntegrity (
   cafsDir: string,
   pkgIndex: PackageFilesIndex,
-  manifest?: DeferredManifestPromise
-) {
+  readManifest?: boolean
+): VerifyResult {
   // It might make sense to use this cache for all files in the store
   // but there's a smaller chance that the same file will be checked twice
   // so it's probably not worth the memory (this assumption should be verified)
   const verifiedFilesCache = new Set<string>()
   const _checkFilesIntegrity = checkFilesIntegrity.bind(null, verifiedFilesCache, cafsDir)
-  const verified = _checkFilesIntegrity(pkgIndex.files, manifest)
-  if (!verified) return false
+  const verified = _checkFilesIntegrity(pkgIndex.files, readManifest)
+  if (!verified) return { passed: false }
   if (pkgIndex.sideEffects) {
     // We verify all side effects cache. We could optimize it to verify only the side effects cache
     // that satisfies the current os/arch/platform.
     // However, it likely won't make a big difference.
     for (const [sideEffectName, files] of Object.entries(pkgIndex.sideEffects)) {
-      if (!_checkFilesIntegrity(files)) {
+      const { passed } = _checkFilesIntegrity(files)
+      if (!passed) {
         delete pkgIndex.sideEffects![sideEffectName]
       }
     }
   }
-  return true
+  return verified
 }
 
 function checkFilesIntegrity (
   verifiedFilesCache: Set<string>,
   cafsDir: string,
   files: Record<string, PackageFileInfo>,
-  manifest?: DeferredManifestPromise
-): boolean {
+  readManifest?: boolean
+): VerifyResult {
   let allVerified = true
+  let manifest: DependencyManifest | undefined
   for (const [f, fstat] of Object.entries(files)) {
     if (!fstat.integrity) {
       throw new Error(`Integrity checksum is missing for ${f}`)
     }
     const filename = getFilePathByModeInCafs(cafsDir, fstat.integrity, fstat.mode)
-    const deferredManifest = manifest && f === 'package.json' ? manifest : undefined
-    if (!deferredManifest && verifiedFilesCache.has(filename)) continue
-    if (verifyFile(filename, fstat, deferredManifest)) {
+    const readFile = readManifest && f === 'package.json'
+    if (!readFile && verifiedFilesCache.has(filename)) continue
+    const verifyResult = verifyFile(filename, fstat, readFile)
+    if (readFile) {
+      manifest = verifyResult.manifest
+    }
+    if (verifyResult.passed) {
       verifiedFilesCache.add(filename)
     } else {
       allVerified = false
     }
   }
-  if (manifest && !files['package.json']) {
-    manifest.resolve(undefined)
+  return {
+    passed: allVerified,
+    manifest,
   }
-  return allVerified
 }
 
 type FileInfo = Pick<PackageFileInfo, 'size' | 'checkedAt'> & {
@@ -83,48 +95,55 @@ type FileInfo = Pick<PackageFileInfo, 'size' | 'checkedAt'> & {
 function verifyFile (
   filename: string,
   fstat: FileInfo,
-  deferredManifest?: DeferredManifestPromise
-): boolean {
+  readManifest?: boolean
+): VerifyResult {
   const currentFile = checkFile(filename, fstat.checkedAt)
-  if (currentFile == null) return false
+  if (currentFile == null) return { passed: false }
   if (currentFile.isModified) {
     if (currentFile.size !== fstat.size) {
       rimraf.sync(filename)
-      return false
+      return { passed: false }
     }
-    return verifyFileIntegrity(filename, fstat, deferredManifest)
+    return verifyFileIntegrity(filename, fstat, readManifest)
   }
-  if (deferredManifest != null) {
-    parseJsonBuffer(gfs.readFileSync(filename), deferredManifest)
+  if (readManifest) {
+    return {
+      passed: true,
+      manifest: parseJsonBufferSync(gfs.readFileSync(filename)),
+    }
   }
   // If a file was not edited, we are skipping integrity check.
   // We assume that nobody will manually remove a file in the store and create a new one.
-  return true
+  return { passed: true }
 }
 
 export function verifyFileIntegrity (
   filename: string,
   expectedFile: FileInfo,
-  deferredManifest?: DeferredManifestPromise
-): boolean {
+  readManifest?: boolean
+): VerifyResult {
   // @ts-expect-error
   global['verifiedFileIntegrity']++
   try {
     const data = gfs.readFileSync(filename)
-    const ok = Boolean(ssri.checkData(data, expectedFile.integrity))
-    if (!ok) {
+    const passed = Boolean(ssri.checkData(data, expectedFile.integrity))
+    if (!passed) {
       gfs.unlinkSync(filename)
-    } else if (deferredManifest != null) {
-      parseJsonBuffer(data, deferredManifest)
+      return { passed }
+    } else if (readManifest) {
+      return {
+        passed,
+        manifest: parseJsonBufferSync(data),
+      }
     }
-    return ok
+    return { passed }
   } catch (err: any) { // eslint-disable-line
     switch (err.code) {
-    case 'ENOENT': return false
+    case 'ENOENT': return { passed: false }
     case 'EINTEGRITY': {
       // Broken files are removed from the store
       gfs.unlinkSync(filename)
-      return false
+      return { passed: false }
     }
     }
     throw err

store/cafs/src/index.ts

@@ -5,6 +5,7 @@ import { addFilesFromTarball } from './addFilesFromTarball'
 import {
   checkPkgFilesIntegrity,
   type PackageFilesIndex,
+  type VerifyResult,
 } from './checkPkgFilesIntegrity'
 import { readManifestFromStore } from './readManifestFromStore'
 import {
@@ -28,6 +29,7 @@ export {
   type PackageFilesIndex,
   optimisticRenameOverwrite,
   type FilesIndex,
+  type VerifyResult,
 }
 
 export type CafsLocker = Map<string, number>

store/cafs/src/parseJson.ts

@@ -1,17 +1,5 @@
-import type { DeferredManifestPromise } from '@pnpm/cafs-types'
 import stripBom from 'strip-bom'
 
 export function parseJsonBufferSync (buffer: Buffer) {
   return JSON.parse(stripBom(buffer.toString()))
 }
-
-export function parseJsonBuffer (
-  buffer: Buffer,
-  deferred: DeferredManifestPromise
-) {
-  try {
-    deferred.resolve(JSON.parse(stripBom(buffer.toString())))
-  } catch (err: any) { // eslint-disable-line
-    deferred.reject(err)
-  }
-}

store/cafs/src/writeBufferToCafs.ts

@@ -99,5 +99,5 @@ function existsSame (filename: string, integrity: ssri.IntegrityLike) {
   return verifyFileIntegrity(filename, {
     size: existingFile.size,
     integrity,
-  })
+  }).passed
 }

store/cafs/test/index.ts

@@ -59,7 +59,7 @@ describe('checkPkgFilesIntegrity()', () => {
           size: 10,
         },
       },
-    })).toBeFalsy()
+    }).passed).toBeFalsy()
   })
 })
 

worker/package.json

@@ -38,8 +38,7 @@
     "@pnpm/graceful-fs": "workspace:*",
     "@pnpm/store.cafs": "workspace:*",
     "@rushstack/worker-pool": "0.3.34",
-    "load-json-file": "^6.2.0",
-    "safe-promise-defer": "^1.0.1"
+    "load-json-file": "^6.2.0"
   },
   "devDependencies": {
     "@pnpm/types": "workspace:*",

worker/src/worker.ts

@@ -11,9 +11,9 @@ import {
   type FilesIndex,
   optimisticRenameOverwrite,
   readManifestFromStore,
+  type VerifyResult,
 } from '@pnpm/store.cafs'
 import { sync as loadJsonFile } from 'load-json-file'
-import safePromiseDefer from 'safe-promise-defer'
 import { parentPort } from 'worker_threads'
 import {
   type AddDirToStoreMessage,
@@ -67,21 +67,20 @@ async function handleMessage (message: TarballExtractMessage | LinkPkgMessage |
         })
         return
       }
-      const manifestP = readManifest ? safePromiseDefer() : undefined
-      let verified: boolean
+      let verifyResult: VerifyResult | undefined
       if (verifyStoreIntegrity) {
-        verified = checkPkgFilesIntegrity(cafsDir, pkgFilesIndex, manifestP)
+        verifyResult = checkPkgFilesIntegrity(cafsDir, pkgFilesIndex, readManifest)
       } else {
-        verified = true
-        if (manifestP) {
-          manifestP.resolve(readManifestFromStore(cafsDir, pkgFilesIndex))
+        verifyResult = {
+          passed: true,
+          manifest: readManifest ? readManifestFromStore(cafsDir, pkgFilesIndex) : undefined,
         }
       }
       parentPort!.postMessage({
         status: 'success',
         value: {
-          verified,
-          manifest: manifestP ? await manifestP() : undefined,
+          verified: verifyResult.passed,
+          manifest: verifyResult.manifest,
           pkgFilesIndex,
         },
       })