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.

Author: waleedlatif1

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

@@ -2,7 +2,7 @@
  * @vitest-environment node
  */
 import { describe, expect, it } from 'vitest'
-import { hasObjectNotFoundLabel, isObjectNotFoundError } from '@/lib/uploads/core/errors'
+import { isObjectNotFoundError } from '@/lib/uploads/core/errors'
 
 describe('isObjectNotFoundError', () => {
   it('matches the shapes each storage provider uses for a missing object', () => {
@@ -67,23 +67,3 @@ describe('isObjectNotFoundError', () => {
     expect(isObjectNotFoundError(404)).toBe(false)
   })
 })
-
-describe('hasObjectNotFoundLabel', () => {
-  it('accepts only a provider that names the object as the missing resource', () => {
-    expect(hasObjectNotFoundLabel({ name: 'NotFound' })).toBe(true)
-    expect(hasObjectNotFoundLabel({ name: 'NoSuchKey' })).toBe(true)
-    expect(hasObjectNotFoundLabel({ name: 'RestError', code: 'BlobNotFound' })).toBe(true)
-  })
-
-  it('rejects an unlabelled 404, which cannot be attributed to the object', () => {
-    /** GCS answers a missing object and a missing bucket identically. */
-    expect(hasObjectNotFoundLabel({ code: 404 })).toBe(false)
-    expect(hasObjectNotFoundLabel({ statusCode: 404 })).toBe(false)
-    expect(hasObjectNotFoundLabel({ $metadata: { httpStatusCode: 404 } })).toBe(false)
-  })
-
-  it('rejects a missing bucket or container', () => {
-    expect(hasObjectNotFoundLabel({ name: 'NoSuchBucket' })).toBe(false)
-    expect(hasObjectNotFoundLabel({ name: 'RestError', code: 'ContainerNotFound' })).toBe(false)
-  })
-})

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

@@ -18,31 +18,17 @@ function readLabels(error: unknown): string[] | null {
   return [name, code].filter((value): value is string => typeof value === 'string')
 }
 
-/**
- * True when a provider names the missing resource as the object itself.
- *
- * The strict form. Use it wherever the caller cannot vouch for what was requested,
- * because an unlabelled 404 is ambiguous: GCS answers a missing object and a
- * missing bucket identically (`code: 404`, `errors[].reason: 'notFound'`), so only
- * the human-readable message separates them. Treating that as absence would let a
- * bucket misconfiguration read as "no metadata" and fail every read closed with
- * nothing left to alert on, so it stays an error here.
- */
-export function hasObjectNotFoundLabel(error: unknown): boolean {
-  const labels = readLabels(error)
-  if (!labels) return false
-  if (labels.some((label) => CONTAINER_NOT_FOUND_LABELS.has(label))) return false
-  return labels.some((label) => OBJECT_NOT_FOUND_LABELS.has(label))
-}
-
 /**
  * True when a storage provider reports that an object does not exist.
  *
- * The lenient form, for a caller that has just performed an object-level operation
- * and can therefore attribute a bare 404 to that object — which is what every
- * provider client here does, and how each spelled this check before it was shared.
- * It additionally accepts a bare 404 (`code`, `statusCode`, or
- * `$metadata.httpStatusCode`), which GCS relies on since it carries no label.
+ * 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.
  */

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

@@ -3,14 +3,9 @@
  */
 import { beforeEach, describe, expect, it, vi } from 'vitest'
 
-const { mockGetFileMetadataByKey, mockSend, mockHeadObjectCommand } = vi.hoisted(() => ({
+const { mockGetFileMetadataByKey, mockHeadS3Object } = vi.hoisted(() => ({
   mockGetFileMetadataByKey: vi.fn(),
-  mockSend: vi.fn(),
-  mockHeadObjectCommand: vi.fn().mockImplementation(class {}),
-}))
-
-vi.mock('@aws-sdk/client-s3', () => ({
-  HeadObjectCommand: mockHeadObjectCommand,
+  mockHeadS3Object: vi.fn(),
 }))
 
 vi.mock('@/lib/uploads/config', () => ({
@@ -21,7 +16,7 @@ vi.mock('@/lib/uploads/config', () => ({
 }))
 
 vi.mock('@/lib/uploads/providers/s3/client', () => ({
-  getS3Client: () => ({ send: mockSend }),
+  headS3Object: mockHeadS3Object,
 }))
 
 vi.mock('@/lib/uploads/server/metadata', () => ({
@@ -30,41 +25,42 @@ vi.mock('@/lib/uploads/server/metadata', () => ({
 
 import { getFileMetadata } from '@/lib/uploads/core/storage-client'
 
-/** The exact error the AWS SDK raises from HeadObject for an absent object. */
-const notFound = Object.assign(new Error('NotFound'), {
-  name: 'NotFound',
-  $fault: 'client',
-  $metadata: { httpStatusCode: 404 },
-})
-
 describe('getFileMetadata', () => {
   beforeEach(() => {
     vi.clearAllMocks()
     mockGetFileMetadataByKey.mockResolvedValue(null)
   })
 
   it('reports an absent object as no metadata rather than throwing', async () => {
-    mockSend.mockRejectedValue(notFound)
+    /** 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 () => {
-    const denied = Object.assign(new Error('AccessDenied'), {
-      name: 'AccessDenied',
-      $metadata: { httpStatusCode: 403 },
-    })
-    mockSend.mockRejectedValue(denied)
+    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 () => {
-    mockSend.mockResolvedValue({ Metadata: { workspaceid: 'ws-1' } })
+    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',
@@ -77,6 +73,6 @@ describe('getFileMetadata', () => {
     const metadata = await getFileMetadata('workspace/ws/key.md')
 
     expect(metadata.workspaceId).toBe('ws-1')
-    expect(mockSend).not.toHaveBeenCalled()
+    expect(mockHeadS3Object).not.toHaveBeenCalled()
   })
 })

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

@@ -1,5 +1,4 @@
 import { USE_BLOB_STORAGE, USE_GCS_STORAGE, USE_S3_STORAGE } from '@/lib/uploads/config'
-import { hasObjectNotFoundLabel } from '@/lib/uploads/core/errors'
 import type { StorageConfig } from '@/lib/uploads/shared/types'
 
 export type { StorageConfig } from '@/lib/uploads/shared/types'
@@ -30,29 +29,6 @@ export function getServePathPrefix(): string {
 export async function getFileMetadata(
   key: string,
   customConfig?: StorageConfig
-): Promise<Record<string, string>> {
-  try {
-    return await readProviderMetadata(key, customConfig)
-  } catch (error) {
-    /**
-     * A key that no longer resolves is an ordinary outcome — a workspace file is
-     * rewritten under a new key on every content update, so any reader holding the
-     * previous key finds nothing. Report it the way this function already reports
-     * "nothing known about this key" rather than as a failure, so callers fall
-     * through to their own not-found handling instead of an error path.
-     *
-     * Deliberately the labelled check: this dispatches across every provider and so
-     * cannot attribute a bare 404 to the object rather than its bucket. An
-     * unlabelled 404 keeps propagating, exactly as it did before.
-     */
-    if (hasObjectNotFoundLabel(error)) return {}
-    throw error
-  }
-}
-
-async function readProviderMetadata(
-  key: string,
-  customConfig?: StorageConfig
 ): Promise<Record<string, string>> {
   const { getFileMetadataByKey } = await import('../server/metadata')
   const metadataRecord = await getFileMetadataByKey(key)
@@ -68,57 +44,46 @@ async function readProviderMetadata(
   }
 
   if (USE_BLOB_STORAGE) {
-    const { getBlobServiceClient } = await import('@/lib/uploads/providers/blob/client')
+    const { headBlobObject } = await import('@/lib/uploads/providers/blob/client')
     const { BLOB_CONFIG } = await import('@/lib/uploads/config')
-
-    let blobServiceClient = await getBlobServiceClient()
-    let containerName = BLOB_CONFIG.containerName
-
-    if (customConfig) {
-      const { BlobServiceClient, StorageSharedKeyCredential } = await import('@azure/storage-blob')
-      if (customConfig.connectionString) {
-        blobServiceClient = BlobServiceClient.fromConnectionString(customConfig.connectionString)
-      } else if (customConfig.accountName && customConfig.accountKey) {
-        const credential = new StorageSharedKeyCredential(
-          customConfig.accountName,
-          customConfig.accountKey
-        )
-        blobServiceClient = new BlobServiceClient(
-          `https://${customConfig.accountName}.blob.core.windows.net`,
-          credential
-        )
-      }
-      containerName = customConfig.containerName || containerName
-    }
-
-    const containerClient = blobServiceClient.getContainerClient(containerName)
-    const blockBlobClient = containerClient.getBlockBlobClient(key)
-    const properties = await blockBlobClient.getProperties()
-    return properties.metadata || {}
+    /** `headBlobObject` rejects a config that names no credentials, so only pass one that does. */
+    const credentialed = Boolean(
+      customConfig?.connectionString || (customConfig?.accountName && customConfig?.accountKey)
+    )
+    const object = await headBlobObject(
+      key,
+      credentialed
+        ? {
+            ...customConfig,
+            containerName: customConfig?.containerName || BLOB_CONFIG.containerName,
+          }
+        : undefined
+    )
+    return object?.metadata || {}
   }
 
   if (USE_S3_STORAGE) {
-    const { getS3Client } = await import('@/lib/uploads/providers/s3/client')
-    const { HeadObjectCommand } = await import('@aws-sdk/client-s3')
+    const { headS3Object } = await import('@/lib/uploads/providers/s3/client')
     const { S3_CONFIG } = await import('@/lib/uploads/config')
-
-    const s3Client = getS3Client()
     const bucket = customConfig?.bucket || S3_CONFIG.bucket
 
     if (!bucket) {
       throw new Error('S3 bucket not configured')
     }
 
-    const command = new HeadObjectCommand({
-      Bucket: bucket,
-      Key: key,
+    const object = await headS3Object(key, {
+      bucket,
+      region: customConfig?.region || S3_CONFIG.region,
     })
-
-    const response = await s3Client.send(command)
-    return response.Metadata || {}
+    return object?.metadata || {}
   }
 
   if (USE_GCS_STORAGE) {
+    /**
+     * Unlike the other two, this raises on a missing object rather than reporting
+     * absence, because GCS answers a missing object and a missing bucket the same
+     * way and only the caller's own bucket configuration separates them.
+     */
     const { getGcsObjectMetadata } = await import('@/lib/uploads/providers/gcs/client')
     return getGcsObjectMetadata(
       key,

apps/sim/lib/uploads/providers/s3/client.test.ts

@@ -213,6 +213,45 @@ describe('S3 Client', () => {
         metadata: { simuploadid: 'receipt-1' },
       })
     })
+
+    it('reports an absent object as null rather than raising', async () => {
+      /**
+       * A workspace file is rewritten under a new key on every content update, so a
+       * reader holding the previous key lands here routinely. Absence is the answer,
+       * not a failure.
+       */
+      mockSend.mockRejectedValueOnce(
+        Object.assign(new Error('NotFound'), {
+          name: 'NotFound',
+          $metadata: { httpStatusCode: 404 },
+        })
+      )
+
+      await expect(headS3Object('workspace/superseded.md')).resolves.toBeNull()
+    })
+
+    it('raises when the bucket itself is missing', async () => {
+      /** Also a 404, but a misconfiguration — reporting absence would hide an outage. */
+      mockSend.mockRejectedValueOnce(
+        Object.assign(new Error('NoSuchBucket'), {
+          name: 'NoSuchBucket',
+          $metadata: { httpStatusCode: 404 },
+        })
+      )
+
+      await expect(headS3Object('workspace/file.txt')).rejects.toThrow('NoSuchBucket')
+    })
+
+    it('raises on a permission failure', async () => {
+      mockSend.mockRejectedValueOnce(
+        Object.assign(new Error('AccessDenied'), {
+          name: 'AccessDenied',
+          $metadata: { httpStatusCode: 403 },
+        })
+      )
+
+      await expect(headS3Object('workspace/file.txt')).rejects.toThrow('AccessDenied')
+    })
   })
 
   describe('getPresignedUrl', () => {