fix: return 404 for stale cache entries with missing storage objects

When a cache entry exists in the database but the underlying storage
object has been lost (incomplete upload, manual deletion, lifecycle
policy), the download route previously threw and returned 500, causing
BuildKit clients to hang instead of falling back to the upstream
registry.

Storage.download() now pre-validates part presence via
countFilesInFolder before committing to an HTTP response, and adapters
throw a typed ObjectNotFoundError when the merged blob is missing.
Both are caught in the outer try/catch, which logs a warning and
returns undefined so the route returns 404. Stale rows are left for
the existing cleanup task to reap, avoiding mutating DB state on a
read path.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Author: mpecan

lib/storage.ts

@@ -30,6 +30,14 @@ import { match } from 'ts-pattern'
 import { getDatabase } from './db'
 import { env } from './env'
 import { generateNumberId } from './helpers'
+import { logger } from './logger'
+
+export class ObjectNotFoundError extends Error {
+  constructor(objectName: string) {
+    super(`Object not found in storage: ${objectName}`)
+    this.name = 'ObjectNotFoundError'
+  }
+}
 
 export class Storage {
   adapter
@@ -208,22 +216,30 @@ export class Storage {
       .where('id', '=', storageLocation.id)
       .execute()
 
-    if (storageLocation.mergedAt || storageLocation.mergeStartedAt)
-      return this.downloadFromCacheEntryLocation(storageLocation)
+    try {
+      if (storageLocation.mergedAt || storageLocation.mergeStartedAt)
+        return await this.downloadFromCacheEntryLocation(storageLocation)
 
-    await this.db
-      .updateTable('storage_locations')
-      .set({
-        mergeStartedAt: Date.now(),
-      })
-      .where('id', '=', storageLocation.id)
-      .execute()
+      // Verify all parts are still present before committing to the HTTP response.
+      // If the storage backend has lost the object (incomplete upload, manual
+      // deletion, lifecycle policy), failing here bubbles ObjectNotFoundError to
+      // the outer catch, which yields a 404. Detecting this mid-stream would
+      // truncate an already-started 200 response, leaving clients with
+      // unrecoverable partial data.
+      await this.ensurePartsExist(storageLocation)
 
-    const responseStream = new PassThrough()
-    const mergerStream = new PassThrough()
+      await this.db
+        .updateTable('storage_locations')
+        .set({
+          mergeStartedAt: Date.now(),
+        })
+        .where('id', '=', storageLocation.id)
+        .execute()
 
-    try {
-      const promise = this.adapter
+      const responseStream = new PassThrough()
+      const mergerStream = new PassThrough()
+
+      const mergePromise = this.adapter
         .uploadStream(`${storageLocation.folderName}/merged`, mergerStream)
         .then(async () => {
           await this.db
@@ -255,31 +271,38 @@ export class Storage {
             .execute()
           mergerStream.destroy()
         })
-      this.mergeStreamPromises.add(promise)
-      promise.finally(() => this.mergeStreamPromises.delete(promise))
+      this.mergeStreamPromises.add(mergePromise)
+      mergePromise.finally(() => this.mergeStreamPromises.delete(mergePromise))
+
+      this.pumpPartsToStreams(storageLocation, responseStream, mergerStream).catch((err) => {
+        responseStream.destroy(err)
+        mergerStream.destroy(err)
+        if (err instanceof ObjectNotFoundError)
+          logger.warn(`Stale cache entry ${cacheEntryId}: ${err.message}`)
+      })
+
+      return responseStream
     } catch (err) {
-      await this.db
-        .updateTable('storage_locations')
-        .set({
-          mergedAt: null,
-          mergeStartedAt: null,
-        })
-        .where('id', '=', storageLocation.id)
-        .execute()
+      if (err instanceof ObjectNotFoundError) {
+        logger.warn(`Stale cache entry ${cacheEntryId}: ${err.message}`)
+        return
+      }
       throw err
     }
+  }
 
-    this.pumpPartsToStreams(storageLocation, responseStream, mergerStream).catch((err) => {
-      responseStream.destroy(err)
-      mergerStream.destroy(err)
-    })
-
-    return responseStream
+  private async ensurePartsExist(location: StorageLocation) {
+    const partsFolder = `${location.folderName}/parts`
+    const actualPartCount = await this.adapter.countFilesInFolder(partsFolder)
+    if (actualPartCount < location.partCount) throw new ObjectNotFoundError(partsFolder)
   }
 
   private async downloadFromCacheEntryLocation(location: StorageLocation) {
     if (location.mergedAt) return this.adapter.createDownloadStream(`${location.folderName}/merged`)
 
+    // Parts-streaming path: the async generator is lazy, so a missing object
+    // would only surface mid-stream. Pre-validate before returning the reader.
+    await this.ensurePartsExist(location)
     return Readable.from(this.streamParts(location))
   }
 
@@ -522,15 +545,20 @@ class S3Adapter implements StorageAdapter {
   }
 
   async createDownloadStream(objectName: string) {
-    const response = await this.s3.send(
-      new GetObjectCommand({
-        Bucket: this.bucket,
-        Key: `${this.keyPrefix}/${objectName}`,
-      }),
-    )
-    if (!response.Body) throw new Error('No body in S3 get object response')
+    try {
+      const response = await this.s3.send(
+        new GetObjectCommand({
+          Bucket: this.bucket,
+          Key: `${this.keyPrefix}/${objectName}`,
+        }),
+      )
+      if (!response.Body) throw new Error('No body in S3 get object response')
 
-    return response.Body as Readable
+      return response.Body as Readable
+    } catch (err: any) {
+      if (err.name === 'NoSuchKey') throw new ObjectNotFoundError(objectName)
+      throw err
+    }
   }
 
   async deleteFolder(folderName: string) {
@@ -629,7 +657,13 @@ class FileSystemAdapter implements StorageAdapter {
   }
 
   async createDownloadStream(objectName: string) {
-    return createReadStream(path.join(this.rootFolder, objectName))
+    const filePath = path.join(this.rootFolder, objectName)
+    try {
+      await fs.access(filePath)
+    } catch {
+      throw new ObjectNotFoundError(objectName)
+    }
+    return createReadStream(filePath)
   }
 
   async deleteFolder(folderName: string) {
@@ -656,11 +690,16 @@ class FileSystemAdapter implements StorageAdapter {
   }
 
   async countFilesInFolder(folderName: string) {
-    const dir = await fs.readdir(path.join(this.rootFolder, folderName), {
-      withFileTypes: true,
-    })
-
-    return dir.filter((item) => item.isFile()).length
+    try {
+      const dir = await fs.readdir(path.join(this.rootFolder, folderName), {
+        withFileTypes: true,
+      })
+      return dir.filter((item) => item.isFile()).length
+    } catch (err: any) {
+      // Missing folder reports 0, matching S3/GCS list-by-prefix semantics.
+      if (err.code === 'ENOENT') return 0
+      throw err
+    }
   }
 }
 
@@ -690,7 +729,10 @@ class GcsAdapter implements StorageAdapter {
   }
 
   async createDownloadStream(objectName: string) {
-    return this.bucket.file(`${this.keyPrefix}/${objectName}`).createReadStream()
+    const file = this.bucket.file(`${this.keyPrefix}/${objectName}`)
+    const [exists] = await file.exists()
+    if (!exists) throw new ObjectNotFoundError(objectName)
+    return file.createReadStream()
   }
 
   async deleteFolder(folderName: string) {

tests/stale-cache.test.ts

@@ -0,0 +1,87 @@
+import crypto from 'node:crypto'
+import fs from 'node:fs/promises'
+import path from 'node:path'
+
+import { restoreCache, saveCache } from '@actions/cache'
+import { SignJWT } from 'jose'
+import { afterAll, beforeAll, describe, expect, test } from 'vitest'
+import { Storage } from '~/lib/storage'
+import { TEST_TEMP_DIR } from './setup'
+
+const testFilePath = path.join(TEST_TEMP_DIR, 'test-stale.bin')
+
+describe('stale cache entry handling (missing storage objects)', () => {
+  let adapter: Awaited<ReturnType<typeof Storage.getAdapterFromEnv>>
+
+  beforeAll(async () => {
+    process.env.ACTIONS_CACHE_SERVICE_V2 = 'true'
+    process.env.ACTIONS_RUNTIME_TOKEN = await new SignJWT({
+      ac: JSON.stringify([{ Scope: 'refs/heads/main', Permission: 3 }]),
+      repository_id: '123',
+    })
+      .setProtectedHeader({ alg: 'HS256' })
+      .sign(crypto.createSecretKey('mock-secret-key', 'ascii'))
+
+    adapter = await Storage.getAdapterFromEnv()
+  })
+  afterAll(() => {
+    delete process.env.ACTIONS_CACHE_SERVICE_V2
+    delete process.env.ACTIONS_RUNTIME_TOKEN
+  })
+
+  test(
+    'returns cache miss when parts are wiped before first download (unmerged entry)',
+    { timeout: 30_000 },
+    async () => {
+      const contents = crypto.randomBytes(1024)
+      await fs.writeFile(testFilePath, contents)
+      await saveCache([testFilePath], 'stale-fresh-key')
+      await fs.rm(testFilePath)
+
+      // Wipe backend storage before the first restore. The cache entry still
+      // exists in the DB with mergedAt/mergeStartedAt both null, so the next
+      // download hits the fresh-entry path that kicks off a background merge.
+      await adapter.clear()
+
+      const missKey = await restoreCache([testFilePath], 'stale-fresh-key')
+      expect(missKey).toBeUndefined()
+
+      // A subsequent restore must also return a miss (the stored entry is
+      // still unreachable — self-healing is intentionally left to the
+      // cleanup job, so the observable behavior is just a stable 404).
+      const missKey2 = await restoreCache([testFilePath], 'stale-fresh-key')
+      expect(missKey2).toBeUndefined()
+    },
+  )
+
+  test(
+    'returns cache miss when the merged blob is wiped after merge completes',
+    { timeout: 30_000 },
+    async () => {
+      const contents = crypto.randomBytes(1024)
+      await fs.writeFile(testFilePath, contents)
+      await saveCache([testFilePath], 'stale-merged-key')
+      await fs.rm(testFilePath)
+
+      // First restore triggers the background merge via the fresh-entry path.
+      const hitKey = await restoreCache([testFilePath], 'stale-merged-key')
+      expect(hitKey).toBe('stale-merged-key')
+      await fs.rm(testFilePath)
+
+      // Allow the server-side merge (uploadStream -> setMergedAt -> delete
+      // parts) to finish. restoreCache returning only guarantees the response
+      // was consumed, not that the merger upload has flushed.
+      await new Promise((resolve) => setTimeout(resolve, 2000))
+
+      // Wipe backend storage. The DB row now has mergedAt set, so the next
+      // download hits the merged-blob path.
+      await adapter.clear()
+
+      const missKey = await restoreCache([testFilePath], 'stale-merged-key')
+      expect(missKey).toBeUndefined()
+
+      const missKey2 = await restoreCache([testFilePath], 'stale-merged-key')
+      expect(missKey2).toBeUndefined()
+    },
+  )
+})