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 } } 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..974099c9e68 --- /dev/null +++ b/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) + }) +}) diff --git a/apps/sim/lib/uploads/core/errors.ts b/apps/sim/lib/uploads/core/errors.ts new file mode 100644 index 00000000000..d3d8b5298fd --- /dev/null +++ b/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 +} 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..da6f8429d31 --- /dev/null +++ b/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() + }) +}) diff --git a/apps/sim/lib/uploads/core/storage-client.ts b/apps/sim/lib/uploads/core/storage-client.ts index cd5d7c9f8a1..e9112648a50 100644 --- a/apps/sim/lib/uploads/core/storage-client.ts +++ b/apps/sim/lib/uploads/core/storage-client.ts @@ -44,57 +44,46 @@ export async function getFileMetadata( } 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/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', () => { diff --git a/apps/sim/lib/uploads/providers/blob/client.ts b/apps/sim/lib/uploads/providers/blob/client.ts index 4c5c12c9e7e..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, @@ -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..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, @@ -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.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', () => { diff --git a/apps/sim/lib/uploads/providers/s3/client.ts b/apps/sim/lib/uploads/providers/s3/client.ts index 6f3543ba620..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, @@ -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