fix: improve error handling when packfile cannot be read (fixes #1163) (#2295)

costajohnt · web-flow · commit 7be83098e2a1 · 2026-03-13T11:03:06.000+01:00
* fix: improve error handling when packfile cannot be read (fixes #1163) FileSystem.read() silently returns null on errors (including ERR_FS_FILE_TOO_LARGE). In GitPackIndex.readSlice(), the null check tested the Promise object (always truthy) instead of the resolved value, causing a confusing TypeError on .slice(null). - Await the promise before null-checking in readSlice() - Add null check in readObjectPacked after resolving pack data - Clear cached promise on failure to allow retry on transient errors - Add test verifying descriptive InternalError on missing packfile * fix(ci): correct test script name in CI workflow The CI workflow referenced `test.jest` which does not exist in package-scripts.cjs. The correct script name is `test.node`.
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
@@ -54,7 +54,7 @@ jobs:
           npx nps \
             test.typecheck \
             test.setup \
-            test.jest
+            test.node
 
       - name: Save test results
         if: ${{ !cancelled() }}
diff --git a/__tests__/test-packfileIntegrity.js b/__tests__/test-packfileIntegrity.js
@@ -140,4 +140,41 @@ describe('packfile integrity', () => {
     expect(obj2).not.toBeNull()
     expect(obj2.format).toBe(obj1.format)
   })
+
+  it('should throw descriptive error when packfile cannot be read', async () => {
+    // Setup - use a repo with packfile
+    const { fs, dir } = await makeFixture('test-readObject.git')
+    const cache = {}
+    const gitdir = dir
+
+    // Use a known OID from the packfile
+    const oid = '0001c3e2753b03648b6c43dd74ba7fe2f21123d6'
+
+    // Delete the .pack file but keep the .idx file
+    // This causes fs.read() to return null (file not found)
+    const packDir = gitdir + '/objects/pack'
+    const files = await fs.readdir(packDir)
+    const packFile = files.find(f => f.endsWith('.pack'))
+    if (!packFile) {
+      throw new Error('No packfile found in ' + packDir)
+    }
+    await fs.rm(packDir + '/' + packFile)
+
+    // Test - should throw InternalError with descriptive message
+    let error = null
+    try {
+      await readObjectPacked({
+        fs,
+        cache,
+        gitdir,
+        oid,
+      })
+    } catch (e) {
+      error = e
+    }
+
+    expect(error).not.toBeNull()
+    expect(error instanceof Errors.InternalError).toBe(true)
+    expect(error.data.message).toContain('Could not read packfile')
+  })
 })
diff --git a/src/models/GitPackIndex.js b/src/models/GitPackIndex.js
@@ -289,12 +289,13 @@ export class GitPackIndex {
       0b1100000: 'ofs_delta',
       0b1110000: 'ref_delta',
     }
-    if (!this.pack) {
+    const pack = await this.pack
+    if (!pack) {
       throw new InternalError(
-        'Tried to read from a GitPackIndex with no packfile loaded into memory'
+        'Could not read packfile data. The packfile may be missing, corrupted, or too large to read into memory.'
       )
     }
-    const raw = (await this.pack).slice(start)
+    const raw = pack.slice(start)
     const reader = new BufferCursor(raw)
     const byte = reader.readUInt8()
     // Object type is encoded in bits 654
diff --git a/src/storage/readObjectPacked.js b/src/storage/readObjectPacked.js
@@ -26,12 +26,18 @@ export async function readObjectPacked({
     if (p.error) throw new InternalError(p.error)
     // If the packfile DOES have the oid we're looking for...
     if (p.offsets.has(oid)) {
-      // Get the resolved git object from the packfile
+      // Derive the .pack path from the .idx path
+      const packFile = indexFile.replace(/idx$/, 'pack')
       if (!p.pack) {
-        const packFile = indexFile.replace(/idx$/, 'pack')
         p.pack = fs.read(packFile)
       }
       const pack = await p.pack
+      if (!pack) {
+        p.pack = null
+        throw new InternalError(
+          `Could not read packfile at ${packFile}. The file may be missing, corrupted, or too large to read into memory.`
+        )
+      }
 
       // === Packfile Integrity Verification ===
       // Performance optimization: use _checksumVerified flag to verify only once per packfile

Original file line number	Diff line number	Diff line change
`@@ -289,12 +289,13 @@ export class GitPackIndex {`
`289`	`289`	`0b1100000: 'ofs_delta',`
`290`	`290`	`0b1110000: 'ref_delta',`
`291`	`291`	`}`
`292`		`- if (!this.pack) {`
	`292`	`+ const pack = await this.pack`
	`293`	`+ if (!pack) {`
`293`	`294`	`throw new InternalError(`
`294`		`- 'Tried to read from a GitPackIndex with no packfile loaded into memory'`
	`295`	`+ 'Could not read packfile data. The packfile may be missing, corrupted, or too large to read into memory.'`
`295`	`296`	`)`
`296`	`297`	`}`
`297`		`- const raw = (await this.pack).slice(start)`
	`298`	`+ const raw = pack.slice(start)`
`298`	`299`	`const reader = new BufferCursor(raw)`
`299`	`300`	`const byte = reader.readUInt8()`
`300`	`301`	`// Object type is encoded in bits 654`