From e93bf2e944945b8acfe5fa55881435f192cd66d1 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Fri, 7 Aug 2026 10:19:09 -0700 Subject: [PATCH 1/7] 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. --- apps/sim/lib/uploads/core/errors.test.ts | 50 +++++++++++ apps/sim/lib/uploads/core/errors.ts | 31 +++++++ .../lib/uploads/core/storage-client.test.ts | 82 +++++++++++++++++++ apps/sim/lib/uploads/core/storage-client.ts | 20 +++++ apps/sim/lib/uploads/providers/blob/client.ts | 9 +- apps/sim/lib/uploads/providers/gcs/client.ts | 3 +- apps/sim/lib/uploads/providers/s3/client.ts | 6 +- 7 files changed, 190 insertions(+), 11 deletions(-) create mode 100644 apps/sim/lib/uploads/core/errors.test.ts create mode 100644 apps/sim/lib/uploads/core/errors.ts create mode 100644 apps/sim/lib/uploads/core/storage-client.test.ts diff --git a/apps/sim/lib/uploads/core/errors.test.ts b/apps/sim/lib/uploads/core/errors.test.ts new file mode 100644 index 00000000000..49f44c0d603 --- /dev/null +++ b/apps/sim/lib/uploads/core/errors.test.ts @@ -0,0 +1,50 @@ +/** + * @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('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) + }) +}) diff --git a/apps/sim/lib/uploads/core/errors.ts b/apps/sim/lib/uploads/core/errors.ts new file mode 100644 index 00000000000..62eec6088b2 --- /dev/null +++ b/apps/sim/lib/uploads/core/errors.ts @@ -0,0 +1,31 @@ +/** + * True when a storage provider reports that an object simply does not exist. + * + * Absence is an expected outcome of a lookup, not a failure, so callers turn this + * into an empty result rather than propagating it. Every provider spells it + * differently — S3 throws `NotFound` (HeadObject) or `NoSuchKey` (GetObject), + * Azure Blob throws `BlobNotFound`, and GCS throws a numeric `code: 404` — and the + * status may arrive as `$metadata.httpStatusCode`, `statusCode`, or `code`. + * + * Only genuine absence matches. A network failure, a permission denial, or a + * provider 5xx still propagates so it surfaces as the error it is. + */ +export function isObjectNotFoundError(error: unknown): boolean { + if (!error || typeof error !== 'object') return false + + const candidate = error as { + name?: unknown + code?: unknown + statusCode?: unknown + $metadata?: { httpStatusCode?: unknown } + } + + const label = typeof candidate.name === 'string' ? candidate.name : candidate.code + if (label === 'NotFound' || label === 'NoSuchKey' || label === 'BlobNotFound') return true + + return ( + candidate.code === 404 || + candidate.statusCode === 404 || + candidate.$metadata?.httpStatusCode === 404 + ) +} diff --git a/apps/sim/lib/uploads/core/storage-client.test.ts b/apps/sim/lib/uploads/core/storage-client.test.ts new file mode 100644 index 00000000000..32ececfbff5 --- /dev/null +++ b/apps/sim/lib/uploads/core/storage-client.test.ts @@ -0,0 +1,82 @@ +/** + * @vitest-environment node + */ +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const { mockGetFileMetadataByKey, mockSend, mockHeadObjectCommand } = vi.hoisted(() => ({ + mockGetFileMetadataByKey: vi.fn(), + mockSend: vi.fn(), + mockHeadObjectCommand: vi.fn().mockImplementation(class {}), +})) + +vi.mock('@aws-sdk/client-s3', () => ({ + HeadObjectCommand: mockHeadObjectCommand, +})) + +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', () => ({ + getS3Client: () => ({ send: mockSend }), +})) + +vi.mock('@/lib/uploads/server/metadata', () => ({ + getFileMetadataByKey: mockGetFileMetadataByKey, +})) + +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) + + 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) + + 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' } }) + + await expect(getFileMetadata('workspace/ws/key.md')).resolves.toEqual({ workspaceid: 'ws-1' }) + }) + + 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(mockSend).not.toHaveBeenCalled() + }) +}) diff --git a/apps/sim/lib/uploads/core/storage-client.ts b/apps/sim/lib/uploads/core/storage-client.ts index cd5d7c9f8a1..bc3bdeea195 100644 --- a/apps/sim/lib/uploads/core/storage-client.ts +++ b/apps/sim/lib/uploads/core/storage-client.ts @@ -1,4 +1,5 @@ import { USE_BLOB_STORAGE, USE_GCS_STORAGE, USE_S3_STORAGE } from '@/lib/uploads/config' +import { isObjectNotFoundError } from '@/lib/uploads/core/errors' import type { StorageConfig } from '@/lib/uploads/shared/types' export type { StorageConfig } from '@/lib/uploads/shared/types' @@ -29,6 +30,25 @@ export function getServePathPrefix(): string { export async function getFileMetadata( key: string, customConfig?: StorageConfig +): Promise> { + 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. + */ + if (isObjectNotFoundError(error)) return {} + throw error + } +} + +async function readProviderMetadata( + key: string, + customConfig?: StorageConfig ): Promise> { const { getFileMetadataByKey } = await import('../server/metadata') const metadataRecord = await getFileMetadataByKey(key) diff --git a/apps/sim/lib/uploads/providers/blob/client.ts b/apps/sim/lib/uploads/providers/blob/client.ts index 4c5c12c9e7e..13c229cb5d4 100644 --- a/apps/sim/lib/uploads/providers/blob/client.ts +++ b/apps/sim/lib/uploads/providers/blob/client.ts @@ -20,6 +20,7 @@ import type { } from '@/lib/uploads/shared/types' import { sanitizeStorageMetadata } from '@/lib/uploads/utils/file-utils' import { sanitizeFileName } from '@/executor/constants' +import { isObjectNotFoundError } from '@/lib/uploads/core/errors' const logger = createLogger('BlobClient') const MULTIPART_UPLOAD_ID_METADATA_KEY = 'sim_upload_id' @@ -446,9 +447,7 @@ export async function headBlobObject( ...(properties.metadata ? { metadata: properties.metadata } : {}), } } catch (err) { - const status = (err as { statusCode?: number }).statusCode - const code = (err as { code?: string }).code - if (status === 404 || code === 'BlobNotFound') { + if (isObjectNotFoundError(err)) { return null } throw err @@ -833,9 +832,7 @@ export async function abortMultipartUpload( await blockBlobClient.deleteIfExists() } } catch (error) { - const status = (error as { statusCode?: number }).statusCode - const code = (error as { code?: string }).code - if (status !== 404 && code !== 'BlobNotFound') { + if (!isObjectNotFoundError(error)) { logger.warn('Error cleaning up multipart upload:', error) } } diff --git a/apps/sim/lib/uploads/providers/gcs/client.ts b/apps/sim/lib/uploads/providers/gcs/client.ts index 11544e86dcc..2006fa8ee6d 100644 --- a/apps/sim/lib/uploads/providers/gcs/client.ts +++ b/apps/sim/lib/uploads/providers/gcs/client.ts @@ -24,6 +24,7 @@ import { sanitizeStorageMetadata, } from '@/lib/uploads/utils/file-utils' import { sanitizeFileName } from '@/executor/constants' +import { isObjectNotFoundError } from '@/lib/uploads/core/errors' const logger = createLogger('GcsClient') @@ -404,7 +405,7 @@ async function getGcsMultipartCompletionId( const metadata = await getGcsObjectMetadata(key, customConfig) return metadata[GCS_MULTIPART_UPLOAD_ID_METADATA_KEY] ?? null } catch (error) { - if ((error as { code?: number } | null)?.code === 404) return null + if (isObjectNotFoundError(error)) return null throw error } } diff --git a/apps/sim/lib/uploads/providers/s3/client.ts b/apps/sim/lib/uploads/providers/s3/client.ts index 6f3543ba620..123ae454a1c 100644 --- a/apps/sim/lib/uploads/providers/s3/client.ts +++ b/apps/sim/lib/uploads/providers/s3/client.ts @@ -36,6 +36,7 @@ import { sanitizeStorageMetadata, } from '@/lib/uploads/utils/file-utils' import { sanitizeFileName } from '@/executor/constants' +import { isObjectNotFoundError } from '@/lib/uploads/core/errors' let _s3Client: S3Client | null = null @@ -260,10 +261,7 @@ export async function headS3Object( ...(response.Metadata ? { metadata: response.Metadata } : {}), } } catch (error) { - const code = (error as { name?: string; $metadata?: { httpStatusCode?: number } } | null)?.name - const status = (error as { $metadata?: { httpStatusCode?: number } } | null)?.$metadata - ?.httpStatusCode - if (code === 'NotFound' || code === 'NoSuchKey' || status === 404) { + if (isObjectNotFoundError(error)) { return null } throw error From f3f36073ca4e048533385714d99f8208970376bd Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Fri, 7 Aug 2026 10:22:21 -0700 Subject: [PATCH 2/7] fix(uploads): read the not-found label from code as well as name MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- apps/sim/lib/uploads/core/errors.test.ts | 6 ++++++ apps/sim/lib/uploads/core/errors.ts | 19 +++++++++++-------- apps/sim/lib/uploads/providers/blob/client.ts | 2 +- apps/sim/lib/uploads/providers/gcs/client.ts | 2 +- apps/sim/lib/uploads/providers/s3/client.ts | 2 +- 5 files changed, 20 insertions(+), 11 deletions(-) diff --git a/apps/sim/lib/uploads/core/errors.test.ts b/apps/sim/lib/uploads/core/errors.test.ts index 49f44c0d603..7cf941dcb96 100644 --- a/apps/sim/lib/uploads/core/errors.test.ts +++ b/apps/sim/lib/uploads/core/errors.test.ts @@ -24,6 +24,12 @@ describe('isObjectNotFoundError', () => { 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('matches on status alone when the provider sends no label', () => { expect(isObjectNotFoundError({ $metadata: { httpStatusCode: 404 } })).toBe(true) expect(isObjectNotFoundError({ statusCode: 404 })).toBe(true) diff --git a/apps/sim/lib/uploads/core/errors.ts b/apps/sim/lib/uploads/core/errors.ts index 62eec6088b2..314c25ff363 100644 --- a/apps/sim/lib/uploads/core/errors.ts +++ b/apps/sim/lib/uploads/core/errors.ts @@ -1,3 +1,5 @@ +const NOT_FOUND_LABELS = new Set(['NotFound', 'NoSuchKey', 'BlobNotFound']) + /** * True when a storage provider reports that an object simply does not exist. * @@ -13,19 +15,20 @@ export function isObjectNotFoundError(error: unknown): boolean { if (!error || typeof error !== 'object') return false - const candidate = error as { + const { name, code, statusCode, $metadata } = error as { name?: unknown code?: unknown statusCode?: unknown $metadata?: { httpStatusCode?: unknown } } - const label = typeof candidate.name === 'string' ? candidate.name : candidate.code - if (label === 'NotFound' || label === 'NoSuchKey' || label === 'BlobNotFound') return true + /** + * `name` and `code` are checked independently: Azure raises a `RestError` whose + * `name` says nothing useful and whose `code` carries the reason, while the AWS + * SDK puts the reason in `name`. + */ + if (typeof name === 'string' && NOT_FOUND_LABELS.has(name)) return true + if (typeof code === 'string' && NOT_FOUND_LABELS.has(code)) return true - return ( - candidate.code === 404 || - candidate.statusCode === 404 || - candidate.$metadata?.httpStatusCode === 404 - ) + return code === 404 || statusCode === 404 || $metadata?.httpStatusCode === 404 } diff --git a/apps/sim/lib/uploads/providers/blob/client.ts b/apps/sim/lib/uploads/providers/blob/client.ts index 13c229cb5d4..d762493c571 100644 --- a/apps/sim/lib/uploads/providers/blob/client.ts +++ b/apps/sim/lib/uploads/providers/blob/client.ts @@ -7,6 +7,7 @@ import { readNodeStreamToBufferWithLimit, } from '@/lib/core/utils/stream-limits' import { BLOB_CONFIG } from '@/lib/uploads/config' +import { isObjectNotFoundError } from '@/lib/uploads/core/errors' import type { AzureMultipartPart, AzureMultipartUploadInit, @@ -20,7 +21,6 @@ import type { } from '@/lib/uploads/shared/types' import { sanitizeStorageMetadata } from '@/lib/uploads/utils/file-utils' import { sanitizeFileName } from '@/executor/constants' -import { isObjectNotFoundError } from '@/lib/uploads/core/errors' const logger = createLogger('BlobClient') const MULTIPART_UPLOAD_ID_METADATA_KEY = 'sim_upload_id' diff --git a/apps/sim/lib/uploads/providers/gcs/client.ts b/apps/sim/lib/uploads/providers/gcs/client.ts index 2006fa8ee6d..76450f9d235 100644 --- a/apps/sim/lib/uploads/providers/gcs/client.ts +++ b/apps/sim/lib/uploads/providers/gcs/client.ts @@ -8,6 +8,7 @@ import { readNodeStreamToBufferWithLimit, } from '@/lib/core/utils/stream-limits' import { GCS_CONFIG } from '@/lib/uploads/config' +import { isObjectNotFoundError } from '@/lib/uploads/core/errors' import type { GcsConfig, GcsMultipartPart, @@ -24,7 +25,6 @@ import { sanitizeStorageMetadata, } from '@/lib/uploads/utils/file-utils' import { sanitizeFileName } from '@/executor/constants' -import { isObjectNotFoundError } from '@/lib/uploads/core/errors' const logger = createLogger('GcsClient') diff --git a/apps/sim/lib/uploads/providers/s3/client.ts b/apps/sim/lib/uploads/providers/s3/client.ts index 123ae454a1c..7e6d56a9a0f 100644 --- a/apps/sim/lib/uploads/providers/s3/client.ts +++ b/apps/sim/lib/uploads/providers/s3/client.ts @@ -20,6 +20,7 @@ import { readNodeStreamToBufferWithLimit, } from '@/lib/core/utils/stream-limits' import { S3_CONFIG, S3_KB_CONFIG } from '@/lib/uploads/config' +import { isObjectNotFoundError } from '@/lib/uploads/core/errors' import type { S3Config, S3MultipartPart, @@ -36,7 +37,6 @@ import { sanitizeStorageMetadata, } from '@/lib/uploads/utils/file-utils' import { sanitizeFileName } from '@/executor/constants' -import { isObjectNotFoundError } from '@/lib/uploads/core/errors' let _s3Client: S3Client | null = null From b9ef75e3a5e9ba0b6dedc0b987c70fc17589c870 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Fri, 7 Aug 2026 10:26:16 -0700 Subject: [PATCH 3/7] fix(uploads): keep a missing bucket or container out of the not-found path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- apps/sim/lib/uploads/core/errors.test.ts | 13 +++++++++++++ apps/sim/lib/uploads/core/errors.ts | 21 +++++++++++++++------ 2 files changed, 28 insertions(+), 6 deletions(-) diff --git a/apps/sim/lib/uploads/core/errors.test.ts b/apps/sim/lib/uploads/core/errors.test.ts index 7cf941dcb96..974099c9e68 100644 --- a/apps/sim/lib/uploads/core/errors.test.ts +++ b/apps/sim/lib/uploads/core/errors.test.ts @@ -30,6 +30,19 @@ describe('isObjectNotFoundError', () => { 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) diff --git a/apps/sim/lib/uploads/core/errors.ts b/apps/sim/lib/uploads/core/errors.ts index 314c25ff363..6ec4b29cbe0 100644 --- a/apps/sim/lib/uploads/core/errors.ts +++ b/apps/sim/lib/uploads/core/errors.ts @@ -1,4 +1,11 @@ -const NOT_FOUND_LABELS = new Set(['NotFound', 'NoSuchKey', 'BlobNotFound']) +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']) /** * True when a storage provider reports that an object simply does not exist. @@ -23,12 +30,14 @@ export function isObjectNotFoundError(error: unknown): boolean { } /** - * `name` and `code` are checked independently: Azure raises a `RestError` whose - * `name` says nothing useful and whose `code` carries the reason, while the AWS - * SDK puts the reason in `name`. + * `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`. */ - if (typeof name === 'string' && NOT_FOUND_LABELS.has(name)) return true - if (typeof code === 'string' && NOT_FOUND_LABELS.has(code)) return true + const labels = [name, code].filter((value): value is string => typeof value === 'string') + + if (labels.some((label) => CONTAINER_NOT_FOUND_LABELS.has(label))) return false + if (labels.some((label) => OBJECT_NOT_FOUND_LABELS.has(label))) return true return code === 404 || statusCode === 404 || $metadata?.httpStatusCode === 404 } From 48133e99c5e08e8a3cd53035e9e4facadf1e7b35 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Fri, 7 Aug 2026 10:33:12 -0700 Subject: [PATCH 4/7] 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. --- apps/sim/lib/uploads/core/errors.test.ts | 22 +++++++- apps/sim/lib/uploads/core/errors.ts | 62 +++++++++++++-------- apps/sim/lib/uploads/core/storage-client.ts | 8 ++- 3 files changed, 67 insertions(+), 25 deletions(-) diff --git a/apps/sim/lib/uploads/core/errors.test.ts b/apps/sim/lib/uploads/core/errors.test.ts index 974099c9e68..e3fbc4afe02 100644 --- a/apps/sim/lib/uploads/core/errors.test.ts +++ b/apps/sim/lib/uploads/core/errors.test.ts @@ -2,7 +2,7 @@ * @vitest-environment node */ import { describe, expect, it } from 'vitest' -import { isObjectNotFoundError } from '@/lib/uploads/core/errors' +import { hasObjectNotFoundLabel, isObjectNotFoundError } from '@/lib/uploads/core/errors' describe('isObjectNotFoundError', () => { it('matches the shapes each storage provider uses for a missing object', () => { @@ -67,3 +67,23 @@ 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) + }) +}) diff --git a/apps/sim/lib/uploads/core/errors.ts b/apps/sim/lib/uploads/core/errors.ts index 6ec4b29cbe0..283bcb230ca 100644 --- a/apps/sim/lib/uploads/core/errors.ts +++ b/apps/sim/lib/uploads/core/errors.ts @@ -7,37 +7,55 @@ const OBJECT_NOT_FOUND_LABELS = new Set(['NotFound', 'NoSuchKey', 'BlobNotFound' */ 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 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 simply does not exist. + * True when a storage provider reports that an object does not exist. * - * Absence is an expected outcome of a lookup, not a failure, so callers turn this - * into an empty result rather than propagating it. Every provider spells it - * differently — S3 throws `NotFound` (HeadObject) or `NoSuchKey` (GetObject), - * Azure Blob throws `BlobNotFound`, and GCS throws a numeric `code: 404` — and the - * status may arrive as `$metadata.httpStatusCode`, `statusCode`, or `code`. + * 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. * - * Only genuine absence matches. A network failure, a permission denial, or a - * provider 5xx still propagates so it surfaces as the error it is. + * A network failure, a permission denial, or a provider 5xx still propagates. */ export function isObjectNotFoundError(error: unknown): boolean { - if (!error || typeof error !== 'object') return false + 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 { name, code, statusCode, $metadata } = error as { - name?: unknown + const { code, statusCode, $metadata } = error as { code?: unknown statusCode?: unknown $metadata?: { httpStatusCode?: 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`. - */ - const labels = [name, code].filter((value): value is string => typeof value === 'string') - - if (labels.some((label) => CONTAINER_NOT_FOUND_LABELS.has(label))) return false - if (labels.some((label) => OBJECT_NOT_FOUND_LABELS.has(label))) return true - return code === 404 || statusCode === 404 || $metadata?.httpStatusCode === 404 } diff --git a/apps/sim/lib/uploads/core/storage-client.ts b/apps/sim/lib/uploads/core/storage-client.ts index bc3bdeea195..e524220e0c5 100644 --- a/apps/sim/lib/uploads/core/storage-client.ts +++ b/apps/sim/lib/uploads/core/storage-client.ts @@ -1,5 +1,5 @@ import { USE_BLOB_STORAGE, USE_GCS_STORAGE, USE_S3_STORAGE } from '@/lib/uploads/config' -import { isObjectNotFoundError } from '@/lib/uploads/core/errors' +import { hasObjectNotFoundLabel } from '@/lib/uploads/core/errors' import type { StorageConfig } from '@/lib/uploads/shared/types' export type { StorageConfig } from '@/lib/uploads/shared/types' @@ -40,8 +40,12 @@ export async function getFileMetadata( * 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 (isObjectNotFoundError(error)) return {} + if (hasObjectNotFoundLabel(error)) return {} throw error } } From bcde6758c27faae79dd4111602ed7db5f6e9f186 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Fri, 7 Aug 2026 10:42:21 -0700 Subject: [PATCH 5/7] 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. --- apps/sim/lib/uploads/core/errors.test.ts | 22 +---- apps/sim/lib/uploads/core/errors.ts | 30 ++----- .../lib/uploads/core/storage-client.test.ts | 42 +++++---- apps/sim/lib/uploads/core/storage-client.ts | 85 ++++++------------- .../lib/uploads/providers/s3/client.test.ts | 39 +++++++++ 5 files changed, 92 insertions(+), 126 deletions(-) diff --git a/apps/sim/lib/uploads/core/errors.test.ts b/apps/sim/lib/uploads/core/errors.test.ts index e3fbc4afe02..974099c9e68 100644 --- a/apps/sim/lib/uploads/core/errors.test.ts +++ b/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) - }) -}) diff --git a/apps/sim/lib/uploads/core/errors.ts b/apps/sim/lib/uploads/core/errors.ts index 283bcb230ca..d3d8b5298fd 100644 --- a/apps/sim/lib/uploads/core/errors.ts +++ b/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. */ diff --git a/apps/sim/lib/uploads/core/storage-client.test.ts b/apps/sim/lib/uploads/core/storage-client.test.ts index 32ececfbff5..da6f8429d31 100644 --- a/apps/sim/lib/uploads/core/storage-client.test.ts +++ b/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,13 +25,6 @@ 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() @@ -44,27 +32,35 @@ describe('getFileMetadata', () => { }) 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() }) }) diff --git a/apps/sim/lib/uploads/core/storage-client.ts b/apps/sim/lib/uploads/core/storage-client.ts index e524220e0c5..e9112648a50 100644 --- a/apps/sim/lib/uploads/core/storage-client.ts +++ b/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> { - 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> { 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, diff --git a/apps/sim/lib/uploads/providers/s3/client.test.ts b/apps/sim/lib/uploads/providers/s3/client.test.ts index 8728671a386..ee6c834eb25 100644 --- a/apps/sim/lib/uploads/providers/s3/client.test.ts +++ b/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', () => { From fb85639be76b05d7499fb347e4e5a0d9862dace8 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Fri, 7 Aug 2026 10:54:54 -0700 Subject: [PATCH 6/7] fix(files): log a missing file at info rather than error when serving MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../api/files/serve/[...path]/route.test.ts | 37 +++++++++++++++++++ .../app/api/files/serve/[...path]/route.ts | 27 +++++++++++--- 2 files changed, 59 insertions(+), 5 deletions(-) diff --git a/apps/sim/app/api/files/serve/[...path]/route.test.ts b/apps/sim/app/api/files/serve/[...path]/route.test.ts index f0e7738c8d0..495f4ad4913 100644 --- a/apps/sim/app/api/files/serve/[...path]/route.test.ts +++ b/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((_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() + }) + }) }) diff --git a/apps/sim/app/api/files/serve/[...path]/route.ts b/apps/sim/app/api/files/serve/[...path]/route.ts index b8ed3154eab..a94eee6b48d 100644 --- a/apps/sim/app/api/files/serve/[...path]/route.ts +++ b/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 { cacheControl: PUBLIC_ASSET_CACHE_CONTROL, }) } catch (error) { - logger.error('Error reading public local file:', error) + logServeFailure('Error reading public local file:', error) throw error } } From fac2ed91cdf49223ed7d56964d7b83fb2cb8ec4f Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Fri, 7 Aug 2026 10:59:04 -0700 Subject: [PATCH 7/7] 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. --- .../lib/uploads/providers/blob/client.test.ts | 38 +++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/apps/sim/lib/uploads/providers/blob/client.test.ts b/apps/sim/lib/uploads/providers/blob/client.test.ts index ee464f6ba1b..2fbd11eb777 100644 --- a/apps/sim/lib/uploads/providers/blob/client.test.ts +++ b/apps/sim/lib/uploads/providers/blob/client.test.ts @@ -209,6 +209,44 @@ describe('Azure Blob Storage Client', () => { metadata: { simuploadid: 'receipt-1' }, }) }) + + it('reports an absent blob as null rather than raising', async () => { + /** Azure names the class in `name` and the reason in `code`. */ + mockGetProperties.mockRejectedValueOnce( + Object.assign(new Error('BlobNotFound'), { + name: 'RestError', + code: 'BlobNotFound', + statusCode: 404, + }) + ) + + await expect(headBlobObject('workspace/superseded.md')).resolves.toBeNull() + }) + + it('raises when the container itself is missing', async () => { + /** Also a 404, but a misconfiguration — reporting absence would hide an outage. */ + mockGetProperties.mockRejectedValueOnce( + Object.assign(new Error('ContainerNotFound'), { + name: 'RestError', + code: 'ContainerNotFound', + statusCode: 404, + }) + ) + + await expect(headBlobObject('workspace/file.txt')).rejects.toThrow('ContainerNotFound') + }) + + it('raises on a permission failure', async () => { + mockGetProperties.mockRejectedValueOnce( + Object.assign(new Error('AuthorizationFailure'), { + name: 'RestError', + code: 'AuthorizationFailure', + statusCode: 403, + }) + ) + + await expect(headBlobObject('workspace/file.txt')).rejects.toThrow('AuthorizationFailure') + }) }) describe('deleteFromBlob', () => {