fix(uploads): treat a missing storage object as absent metadata, not a failure (#6378)

* fix(uploads): treat a missing storage object as absent metadata, not a failure

A workspace file is rewritten under a new key on every content update and the
superseded object is deleted, so any reader holding the previous key finds
nothing. getFileMetadata's provider lookups let that not-found propagate, so
authorization's catch-all logged it at ERROR and never reached the branch
already written for it. Return the function's established empty value instead,
and collapse the three divergent per-provider not-found predicates onto one.

* fix(uploads): read the not-found label from code as well as name

Azure raises a RestError whose name carries the class and whose code carries the
reason, so testing name first and falling back to code only when name was absent
missed BlobNotFound outright — narrower than the per-provider check it replaced.

* fix(uploads): keep a missing bucket or container out of the not-found path

NoSuchBucket and ContainerNotFound also answer 404, so the status-only match read
a total storage misconfiguration as an absent object — every file read would fail
closed with nothing left to alert on.

* fix(uploads): require an object-level label before treating a lookup as absent

GCS answers a missing object and a missing bucket identically, so a bare 404
cannot be attributed to the object by a dispatcher that does not know what was
requested. getFileMetadata now takes the labelled check and leaves an unlabelled
404 propagating as before; the provider clients keep the lenient form, which is
what each already used.

* refactor(uploads): let getFileMetadata delegate to the provider head helpers

getFileMetadata re-implemented the S3 and Blob HEAD calls inline, so it had to
inspect provider errors itself and needed a second, stricter predicate to do it
safely. headS3Object and headBlobObject already perform exactly those calls and
already report absence as null, so delegating removes the duplication, the error
inspection, and the extra predicate at once. GCS keeps raising, as before.

Covers the real provider path in the S3 client's own suite, where mocking the
seam had been hiding whether the two layers agree.

* fix(files): log a missing file at info rather than error when serving

Each serve handler rethrows into the outer one, so a superseded key produced two
ERROR lines for what is an ordinary 404 — two thirds of this module's error
volume. Route all five catch sites through one helper that reserves error for
failures that are actually the server's fault, matching how DocCompileUserError
is already handled a few lines above.

* test(uploads): cover the Blob not-found paths the shared predicate now governs

S3 and GCS already asserted absence and non-404 rethrow; Blob asserted neither,
so the container-level exclusion went unverified on the one provider whose error
puts the reason in code rather than name.

Author: waleedlatif1

apps/sim/app/api/files/serve/[...path]/route.test.ts

@@ -7,6 +7,13 @@ import { hybridAuthMockFns, storageServiceMock, storageServiceMockFns } from '@s
 import { NextRequest } from 'next/server'
 import { beforeEach, describe, expect, it, vi } from 'vitest'
 
+vi.mock('@sim/logger', () => ({
+  createLogger: vi.fn(() => serveLogger),
+  logger: serveLogger,
+  runWithRequestContext: vi.fn(<T>(_ctx: unknown, fn: () => T): T => fn()),
+  getRequestContext: vi.fn(() => undefined),
+}))
+
 const {
   mockVerifyFileAccess,
   mockReadFile,
@@ -18,6 +25,7 @@ const {
   mockCreateFileResponse,
   mockCreateErrorResponse,
   FileNotFoundError,
+  serveLogger,
 } = vi.hoisted(() => {
   class FileNotFoundErrorClass extends Error {
     constructor(message: string) {
@@ -26,6 +34,7 @@ const {
     }
   }
   return {
+    serveLogger: { info: vi.fn(), warn: vi.fn(), error: vi.fn(), debug: vi.fn() },
     mockVerifyFileAccess: vi.fn(),
     mockReadFile: vi.fn(),
     mockIsUsingCloudStorage: vi.fn(),
@@ -232,4 +241,32 @@ describe('File Serve API Route', () => {
       })
     }
   })
+
+  describe('failure log level', () => {
+    it('records a missing file at info, not error', async () => {
+      /** A superseded key is an ordinary 404, not a server fault. */
+      const req = new NextRequest('http://localhost:3000/api/files/serve/')
+      const response = await GET(req, { params: Promise.resolve({ path: [] }) })
+
+      expect(response.status).toBe(404)
+      expect(serveLogger.info).toHaveBeenCalledWith(
+        'Error serving file:',
+        expect.objectContaining({ reason: expect.any(String) })
+      )
+      expect(serveLogger.error).not.toHaveBeenCalled()
+    })
+
+    it('still records a genuine failure at error', async () => {
+      mockVerifyFileAccess.mockRejectedValueOnce(new Error('permission backend down'))
+
+      const req = new NextRequest(
+        'http://localhost:3000/api/files/serve/workspace/ws/test-file.txt'
+      )
+      await GET(req, {
+        params: Promise.resolve({ path: ['workspace', 'ws', 'test-file.txt'] }),
+      }).catch(() => undefined)
+
+      expect(serveLogger.error).toHaveBeenCalled()
+    })
+  })
 })

apps/sim/app/api/files/serve/[...path]/route.ts

@@ -26,6 +26,23 @@ import {
 
 const logger = createLogger('FilesServeAPI')
 
+/**
+ * Records a failed serve at a level that matches whose fault it is.
+ *
+ * A file that is not there is an ordinary answer rather than a server fault: a
+ * workspace file is rewritten under a new key on every content update, so a reader
+ * holding the previous key lands here routinely and correctly receives a 404. Each
+ * handler rethrows into the outer one, so logging those at `error` reports the same
+ * expected 404 twice and buries the failures that do warrant attention.
+ */
+function logServeFailure(message: string, error: unknown): void {
+  if (error instanceof FileNotFoundError) {
+    logger.info(message, { reason: error.message })
+    return
+  }
+  logger.error(message, error)
+}
+
 interface ServeOptions {
   /** `raw=1` — bypass all resolution and serve the stored source as-is. */
   raw: boolean
@@ -179,7 +196,7 @@ export const GET = withRouteHandler(
         return NextResponse.json({ error: 'Document is still being generated' }, { status: 409 })
       }
 
-      logger.error('Error serving file:', error)
+      logServeFailure('Error serving file:', error)
 
       if (error instanceof FileNotFoundError) {
         return createErrorResponse(error)
@@ -244,7 +261,7 @@ async function handleLocalFile(
       cacheControl: resolveServeCacheControl(options.versioned, contextParam),
     })
   } catch (error) {
-    logger.error('Error reading local file:', error)
+    logServeFailure('Error reading local file:', error)
     throw error
   }
 }
@@ -311,7 +328,7 @@ async function handleCloudProxy(
       cacheControl: resolveServeCacheControl(options.versioned, context),
     })
   } catch (error) {
-    logger.error('Error downloading from cloud storage:', error)
+    logServeFailure('Error downloading from cloud storage:', error)
     throw error
   }
 }
@@ -348,7 +365,7 @@ async function handleCloudProxyPublic(
       cacheControl: PUBLIC_ASSET_CACHE_CONTROL,
     })
   } catch (error) {
-    logger.error('Error serving public cloud file:', error)
+    logServeFailure('Error serving public cloud file:', error)
     throw error
   }
 }
@@ -373,7 +390,7 @@ async function handleLocalFilePublic(filename: string): Promise<NextResponse> {
       cacheControl: PUBLIC_ASSET_CACHE_CONTROL,
     })
   } catch (error) {
-    logger.error('Error reading public local file:', error)
+    logServeFailure('Error reading public local file:', error)
     throw error
   }
 }

apps/sim/lib/uploads/core/errors.test.ts

@@ -0,0 +1,69 @@
+/**
+ * @vitest-environment node
+ */
+import { describe, expect, it } from 'vitest'
+import { isObjectNotFoundError } from '@/lib/uploads/core/errors'
+
+describe('isObjectNotFoundError', () => {
+  it('matches the shapes each storage provider uses for a missing object', () => {
+    /** S3 HeadObject, exactly as the production payload arrived. */
+    expect(
+      isObjectNotFoundError({
+        name: 'NotFound',
+        $fault: 'client',
+        $metadata: { httpStatusCode: 404 },
+      })
+    ).toBe(true)
+    /** S3 GetObject. */
+    expect(isObjectNotFoundError({ name: 'NoSuchKey', $metadata: { httpStatusCode: 404 } })).toBe(
+      true
+    )
+    /** Azure Blob. */
+    expect(isObjectNotFoundError({ code: 'BlobNotFound', statusCode: 404 })).toBe(true)
+    /** GCS, which reports a numeric code. */
+    expect(isObjectNotFoundError({ code: 404 })).toBe(true)
+  })
+
+  it('reads the label from code when name carries the error class instead', () => {
+    /** Azure raises a `RestError`; the reason lives in `code`, not `name`. */
+    expect(isObjectNotFoundError({ name: 'RestError', code: 'BlobNotFound' })).toBe(true)
+    expect(isObjectNotFoundError({ name: 'Error', code: 'NoSuchKey' })).toBe(true)
+  })
+
+  it('does not read a missing bucket or container as an absent object', () => {
+    /**
+     * These answer 404 too. Reading them as absence would turn a total storage
+     * misconfiguration into silent fail-closed reads with nothing to alert on.
+     */
+    expect(
+      isObjectNotFoundError({ name: 'NoSuchBucket', $metadata: { httpStatusCode: 404 } })
+    ).toBe(false)
+    expect(
+      isObjectNotFoundError({ name: 'RestError', code: 'ContainerNotFound', statusCode: 404 })
+    ).toBe(false)
+  })
+
+  it('matches on status alone when the provider sends no label', () => {
+    expect(isObjectNotFoundError({ $metadata: { httpStatusCode: 404 } })).toBe(true)
+    expect(isObjectNotFoundError({ statusCode: 404 })).toBe(true)
+  })
+
+  it('does not swallow a genuine failure', () => {
+    expect(
+      isObjectNotFoundError({ name: 'AccessDenied', $metadata: { httpStatusCode: 403 } })
+    ).toBe(false)
+    expect(
+      isObjectNotFoundError({ name: 'InternalError', $metadata: { httpStatusCode: 500 } })
+    ).toBe(false)
+    expect(isObjectNotFoundError({ name: 'TimeoutError' })).toBe(false)
+    expect(isObjectNotFoundError({ code: 'ECONNRESET' })).toBe(false)
+    expect(isObjectNotFoundError({ code: 403 })).toBe(false)
+  })
+
+  it('tolerates values that are not error objects', () => {
+    expect(isObjectNotFoundError(null)).toBe(false)
+    expect(isObjectNotFoundError(undefined)).toBe(false)
+    expect(isObjectNotFoundError('NotFound')).toBe(false)
+    expect(isObjectNotFoundError(404)).toBe(false)
+  })
+})

apps/sim/lib/uploads/core/errors.ts

@@ -0,0 +1,47 @@
+const OBJECT_NOT_FOUND_LABELS = new Set(['NotFound', 'NoSuchKey', 'BlobNotFound'])
+
+/**
+ * A missing bucket or container is a misconfiguration, not an absent object, and
+ * it also answers 404. Without this it would read as "no metadata" and every file
+ * read would fail closed with no error to alert on.
+ */
+const CONTAINER_NOT_FOUND_LABELS = new Set(['NoSuchBucket', 'ContainerNotFound'])
+
+function readLabels(error: unknown): string[] | null {
+  if (!error || typeof error !== 'object') return null
+  const { name, code } = error as { name?: unknown; code?: unknown }
+  /**
+   * `name` and `code` are both consulted: Azure raises a `RestError` whose `name`
+   * carries the class and whose `code` carries the reason, while the AWS SDK puts
+   * the reason in `name`.
+   */
+  return [name, code].filter((value): value is string => typeof value === 'string')
+}
+
+/**
+ * True when a storage provider reports that an object does not exist.
+ *
+ * Call this only from code that has just performed an object-level operation, so a
+ * bare 404 can be attributed to that object. A bare 404 is otherwise ambiguous —
+ * GCS answers a missing object and a missing bucket identically (`code: 404`,
+ * `errors[].reason: 'notFound'`), separable only by a human-readable message — and
+ * every caller here is a provider client that knows exactly what it asked for.
+ *
+ * Absence is an expected outcome of a lookup, so callers turn it into an empty
+ * result rather than propagating it.
+ *
+ * A network failure, a permission denial, or a provider 5xx still propagates.
+ */
+export function isObjectNotFoundError(error: unknown): boolean {
+  const labels = readLabels(error)
+  if (!labels) return false
+  if (labels.some((label) => CONTAINER_NOT_FOUND_LABELS.has(label))) return false
+  if (labels.some((label) => OBJECT_NOT_FOUND_LABELS.has(label))) return true
+
+  const { code, statusCode, $metadata } = error as {
+    code?: unknown
+    statusCode?: unknown
+    $metadata?: { httpStatusCode?: unknown }
+  }
+  return code === 404 || statusCode === 404 || $metadata?.httpStatusCode === 404
+}

apps/sim/lib/uploads/core/storage-client.test.ts

@@ -0,0 +1,78 @@
+/**
+ * @vitest-environment node
+ */
+import { beforeEach, describe, expect, it, vi } from 'vitest'
+
+const { mockGetFileMetadataByKey, mockHeadS3Object } = vi.hoisted(() => ({
+  mockGetFileMetadataByKey: vi.fn(),
+  mockHeadS3Object: vi.fn(),
+}))
+
+vi.mock('@/lib/uploads/config', () => ({
+  USE_S3_STORAGE: true,
+  USE_BLOB_STORAGE: false,
+  USE_GCS_STORAGE: false,
+  S3_CONFIG: { bucket: 'bucket', region: 'region' },
+}))
+
+vi.mock('@/lib/uploads/providers/s3/client', () => ({
+  headS3Object: mockHeadS3Object,
+}))
+
+vi.mock('@/lib/uploads/server/metadata', () => ({
+  getFileMetadataByKey: mockGetFileMetadataByKey,
+}))
+
+import { getFileMetadata } from '@/lib/uploads/core/storage-client'
+
+describe('getFileMetadata', () => {
+  beforeEach(() => {
+    vi.clearAllMocks()
+    mockGetFileMetadataByKey.mockResolvedValue(null)
+  })
+
+  it('reports an absent object as no metadata rather than throwing', async () => {
+    /** The provider client owns not-found and reports absence as `null`. */
+    mockHeadS3Object.mockResolvedValue(null)
+
+    await expect(getFileMetadata('workspace/ws/superseded-key.md')).resolves.toEqual({})
+  })
+
+  it('still propagates a genuine storage failure', async () => {
+    mockHeadS3Object.mockRejectedValue(
+      Object.assign(new Error('AccessDenied'), {
+        name: 'AccessDenied',
+        $metadata: { httpStatusCode: 403 },
+      })
+    )
+
+    await expect(getFileMetadata('workspace/ws/key.md')).rejects.toThrow('AccessDenied')
+  })
+
+  it('returns provider metadata when the object exists', async () => {
+    mockHeadS3Object.mockResolvedValue({ size: 12, metadata: { workspaceid: 'ws-1' } })
+
+    await expect(getFileMetadata('workspace/ws/key.md')).resolves.toEqual({ workspaceid: 'ws-1' })
+  })
+
+  it('treats an object carrying no metadata as no metadata', async () => {
+    mockHeadS3Object.mockResolvedValue({ size: 12 })
+
+    await expect(getFileMetadata('workspace/ws/key.md')).resolves.toEqual({})
+  })
+
+  it('prefers the database record when one exists', async () => {
+    mockGetFileMetadataByKey.mockResolvedValue({
+      userId: 'user-1',
+      workspaceId: 'ws-1',
+      originalName: 'doc.md',
+      uploadedAt: new Date('2026-01-01T00:00:00Z'),
+      context: 'workspace',
+    })
+
+    const metadata = await getFileMetadata('workspace/ws/key.md')
+
+    expect(metadata.workspaceId).toBe('ws-1')
+    expect(mockHeadS3Object).not.toHaveBeenCalled()
+  })
+})