Skip to content

Commit fb85639

Browse files
committed
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.
1 parent bcde675 commit fb85639

2 files changed

Lines changed: 59 additions & 5 deletions

File tree

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

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,13 @@ import { hybridAuthMockFns, storageServiceMock, storageServiceMockFns } from '@s
77
import { NextRequest } from 'next/server'
88
import { beforeEach, describe, expect, it, vi } from 'vitest'
99

10+
vi.mock('@sim/logger', () => ({
11+
createLogger: vi.fn(() => serveLogger),
12+
logger: serveLogger,
13+
runWithRequestContext: vi.fn(<T>(_ctx: unknown, fn: () => T): T => fn()),
14+
getRequestContext: vi.fn(() => undefined),
15+
}))
16+
1017
const {
1118
mockVerifyFileAccess,
1219
mockReadFile,
@@ -18,6 +25,7 @@ const {
1825
mockCreateFileResponse,
1926
mockCreateErrorResponse,
2027
FileNotFoundError,
28+
serveLogger,
2129
} = vi.hoisted(() => {
2230
class FileNotFoundErrorClass extends Error {
2331
constructor(message: string) {
@@ -26,6 +34,7 @@ const {
2634
}
2735
}
2836
return {
37+
serveLogger: { info: vi.fn(), warn: vi.fn(), error: vi.fn(), debug: vi.fn() },
2938
mockVerifyFileAccess: vi.fn(),
3039
mockReadFile: vi.fn(),
3140
mockIsUsingCloudStorage: vi.fn(),
@@ -232,4 +241,32 @@ describe('File Serve API Route', () => {
232241
})
233242
}
234243
})
244+
245+
describe('failure log level', () => {
246+
it('records a missing file at info, not error', async () => {
247+
/** A superseded key is an ordinary 404, not a server fault. */
248+
const req = new NextRequest('http://localhost:3000/api/files/serve/')
249+
const response = await GET(req, { params: Promise.resolve({ path: [] }) })
250+
251+
expect(response.status).toBe(404)
252+
expect(serveLogger.info).toHaveBeenCalledWith(
253+
'Error serving file:',
254+
expect.objectContaining({ reason: expect.any(String) })
255+
)
256+
expect(serveLogger.error).not.toHaveBeenCalled()
257+
})
258+
259+
it('still records a genuine failure at error', async () => {
260+
mockVerifyFileAccess.mockRejectedValueOnce(new Error('permission backend down'))
261+
262+
const req = new NextRequest(
263+
'http://localhost:3000/api/files/serve/workspace/ws/test-file.txt'
264+
)
265+
await GET(req, {
266+
params: Promise.resolve({ path: ['workspace', 'ws', 'test-file.txt'] }),
267+
}).catch(() => undefined)
268+
269+
expect(serveLogger.error).toHaveBeenCalled()
270+
})
271+
})
235272
})

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

Lines changed: 22 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,23 @@ import {
2626

2727
const logger = createLogger('FilesServeAPI')
2828

29+
/**
30+
* Records a failed serve at a level that matches whose fault it is.
31+
*
32+
* A file that is not there is an ordinary answer rather than a server fault: a
33+
* workspace file is rewritten under a new key on every content update, so a reader
34+
* holding the previous key lands here routinely and correctly receives a 404. Each
35+
* handler rethrows into the outer one, so logging those at `error` reports the same
36+
* expected 404 twice and buries the failures that do warrant attention.
37+
*/
38+
function logServeFailure(message: string, error: unknown): void {
39+
if (error instanceof FileNotFoundError) {
40+
logger.info(message, { reason: error.message })
41+
return
42+
}
43+
logger.error(message, error)
44+
}
45+
2946
interface ServeOptions {
3047
/** `raw=1` — bypass all resolution and serve the stored source as-is. */
3148
raw: boolean
@@ -179,7 +196,7 @@ export const GET = withRouteHandler(
179196
return NextResponse.json({ error: 'Document is still being generated' }, { status: 409 })
180197
}
181198

182-
logger.error('Error serving file:', error)
199+
logServeFailure('Error serving file:', error)
183200

184201
if (error instanceof FileNotFoundError) {
185202
return createErrorResponse(error)
@@ -244,7 +261,7 @@ async function handleLocalFile(
244261
cacheControl: resolveServeCacheControl(options.versioned, contextParam),
245262
})
246263
} catch (error) {
247-
logger.error('Error reading local file:', error)
264+
logServeFailure('Error reading local file:', error)
248265
throw error
249266
}
250267
}
@@ -311,7 +328,7 @@ async function handleCloudProxy(
311328
cacheControl: resolveServeCacheControl(options.versioned, context),
312329
})
313330
} catch (error) {
314-
logger.error('Error downloading from cloud storage:', error)
331+
logServeFailure('Error downloading from cloud storage:', error)
315332
throw error
316333
}
317334
}
@@ -348,7 +365,7 @@ async function handleCloudProxyPublic(
348365
cacheControl: PUBLIC_ASSET_CACHE_CONTROL,
349366
})
350367
} catch (error) {
351-
logger.error('Error serving public cloud file:', error)
368+
logServeFailure('Error serving public cloud file:', error)
352369
throw error
353370
}
354371
}
@@ -373,7 +390,7 @@ async function handleLocalFilePublic(filename: string): Promise<NextResponse> {
373390
cacheControl: PUBLIC_ASSET_CACHE_CONTROL,
374391
})
375392
} catch (error) {
376-
logger.error('Error reading public local file:', error)
393+
logServeFailure('Error reading public local file:', error)
377394
throw error
378395
}
379396
}

0 commit comments

Comments
 (0)