Skip to content

Commit 6ff2969

Browse files
committed
improvement(files): answer a file revalidation with 304 instead of the whole body
A response the browser is told to revalidate carried no validator, so every check re-sent the entire file. That is the cost a document compiled against other files now pays on each window focus: it cannot be given a cache lifetime, because its bytes really may have changed, so the only way to make the check cheap is to let the client prove what it already holds. Authorized serves now carry an ETag — the digest of the bytes about to be sent, which is exact by construction however those bytes were produced — and answer 304 to a matching If-None-Match. Matching is weak, per RFC 9110, so a cache that stored a weak validator still revalidates. Kept out of createFileResponse deliberately: digesting costs a pass over the buffer, up to the 100MB transfer ceiling, and a response served as immutable is never revalidated, so it would pay that pass and never collect. Public assets and the assistant-image path are unchanged.
1 parent 53fad9d commit 6ff2969

4 files changed

Lines changed: 149 additions & 18 deletions

File tree

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

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,7 @@ const {
3636
mockFindLocalFile,
3737
mockReadLocalFileWithinLimit,
3838
mockCreateFileResponse,
39+
mockCreateConditionalFileResponse,
3940
mockCreateErrorResponse,
4041
FileNotFoundError,
4142
serveLogger,
@@ -64,6 +65,7 @@ const {
6465
mockFindLocalFile: vi.fn(),
6566
mockReadLocalFileWithinLimit: vi.fn(),
6667
mockCreateFileResponse: vi.fn(),
68+
mockCreateConditionalFileResponse: vi.fn(),
6769
mockCreateErrorResponse: vi.fn(),
6870
FileNotFoundError: FileNotFoundErrorClass,
6971
}
@@ -129,6 +131,7 @@ vi.mock('@/lib/copilot/tools/server/files/doc-compile', () => ({
129131
vi.mock('@/app/api/files/utils', () => ({
130132
FileNotFoundError,
131133
createFileResponse: mockCreateFileResponse,
134+
createConditionalFileResponse: mockCreateConditionalFileResponse,
132135
createErrorResponse: mockCreateErrorResponse,
133136
getContentType: mockGetContentType,
134137
extractStorageKey: vi.fn().mockImplementation((path: string) => path.split('/').pop()),
@@ -192,6 +195,11 @@ describe('File Serve API Route', () => {
192195
})
193196
}
194197
)
198+
// Delegates so the existing assertions on the response payload — including its
199+
// Cache-Control — read the same call list whichever helper the route reached for.
200+
mockCreateConditionalFileResponse.mockImplementation((file: unknown) =>
201+
mockCreateFileResponse(file)
202+
)
195203
mockCreateErrorResponse.mockImplementation((error: Error) => {
196204
return new Response(JSON.stringify({ error: error.name, message: error.message }), {
197205
status: error.name === 'FileNotFoundError' ? 404 : 500,

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

Lines changed: 31 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@ import { isSimPageSource, SIM_PAGE_CONTENT_TYPE } from '@/lib/workspace-files/pa
3030
import { renderSimPageDocumentWithAssets } from '@/lib/workspace-files/page-document.server'
3131
import { type KnowledgeFileAccess, verifyFileAccess } from '@/app/api/files/authorization'
3232
import {
33+
createConditionalFileResponse,
3334
createErrorResponse,
3435
createFileResponse,
3536
FileNotFoundError,
@@ -66,6 +67,8 @@ interface ServeOptions {
6667
preview: boolean
6768
/** `v=<updatedAt>` — the caller asserts the URL addresses one fixed content revision. */
6869
versioned: boolean
70+
/** The request's `If-None-Match`, so a revalidation can be answered 304 instead of re-sending. */
71+
ifNoneMatch: string | null
6972
}
7073

7174
/**
@@ -327,6 +330,7 @@ export const GET = withRouteHandler(
327330
raw: query.raw === '1',
328331
preview: query.preview === '1',
329332
versioned: query.v != null,
333+
ifNoneMatch: request.headers.get('if-none-match'),
330334
}
331335

332336
if (workspacePrincipal) {
@@ -427,12 +431,15 @@ async function handleWorkspaceFile(
427431
workspaceId,
428432
size: resolved.buffer.length,
429433
})
430-
return createFileResponse({
431-
buffer: resolved.buffer,
432-
contentType: resolved.contentType,
433-
filename: file.name,
434-
cacheControl: resolveServeCacheControl(options.versioned, 'workspace', resolved.cacheability),
435-
})
434+
return createConditionalFileResponse(
435+
{
436+
buffer: resolved.buffer,
437+
contentType: resolved.contentType,
438+
filename: file.name,
439+
cacheControl: resolveServeCacheControl(options.versioned, 'workspace', resolved.cacheability),
440+
},
441+
options.ifNoneMatch
442+
)
436443
}
437444

438445
async function handleLocalFile(
@@ -489,12 +496,15 @@ async function handleLocalFile(
489496

490497
logger.info('Local file served', { userId, filename, size: fileBuffer.length })
491498

492-
return createFileResponse({
493-
buffer: fileBuffer,
494-
contentType,
495-
filename: displayName,
496-
cacheControl: resolveServeCacheControl(options.versioned, context, cacheability),
497-
})
499+
return createConditionalFileResponse(
500+
{
501+
buffer: fileBuffer,
502+
contentType,
503+
filename: displayName,
504+
cacheControl: resolveServeCacheControl(options.versioned, context, cacheability),
505+
},
506+
options.ifNoneMatch
507+
)
498508
} catch (error) {
499509
logServeFailure('Error reading local file:', error)
500510
throw error
@@ -565,12 +575,15 @@ async function handleCloudProxy(
565575
context,
566576
})
567577

568-
return createFileResponse({
569-
buffer: fileBuffer,
570-
contentType,
571-
filename: displayName,
572-
cacheControl: resolveServeCacheControl(options.versioned, context, cacheability),
573-
})
578+
return createConditionalFileResponse(
579+
{
580+
buffer: fileBuffer,
581+
contentType,
582+
filename: displayName,
583+
cacheControl: resolveServeCacheControl(options.versioned, context, cacheability),
584+
},
585+
options.ifNoneMatch
586+
)
574587
} catch (error) {
575588
logServeFailure('Error downloading from cloud storage:', error)
576589
throw error

‎apps/sim/app/api/files/utils.test.ts‎

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
import { describe, expect, it } from 'vitest'
22
import {
3+
createConditionalFileResponse,
34
createFileResponse,
45
encodeFilenameForHeader,
56
extractFilename,
@@ -508,3 +509,61 @@ describe('findLocalFile - Path Traversal Security Tests', () => {
508509
)
509510
})
510511
})
512+
513+
describe('createConditionalFileResponse', () => {
514+
const file = {
515+
buffer: Buffer.from('compiled-document-bytes'),
516+
contentType: 'application/pdf',
517+
filename: 'report.pdf',
518+
cacheControl: 'private, no-cache, must-revalidate',
519+
}
520+
521+
function etagOf(ifNoneMatch: string | null = null): string {
522+
return createConditionalFileResponse(file, ifNoneMatch).headers.get('ETag') as string
523+
}
524+
525+
it('sends the body with a strong validator when the client holds nothing', () => {
526+
const response = createConditionalFileResponse(file, null)
527+
528+
expect(response.status).toBe(200)
529+
expect(response.headers.get('ETag')).toMatch(/^"[A-Za-z0-9_-]+"$/)
530+
expect(response.headers.get('Cache-Control')).toBe('private, no-cache, must-revalidate')
531+
})
532+
533+
it('answers 304 without a body when the client already holds these bytes', async () => {
534+
const response = createConditionalFileResponse(file, etagOf())
535+
536+
expect(response.status).toBe(304)
537+
expect(await response.text()).toBe('')
538+
// Repeated so the stored response is refreshed with this request's lifetime.
539+
expect(response.headers.get('Cache-Control')).toBe('private, no-cache, must-revalidate')
540+
})
541+
542+
it('sends the body when the client holds a validator for different bytes', () => {
543+
const stale = createConditionalFileResponse(
544+
{ ...file, buffer: Buffer.from('an-earlier-render') },
545+
null
546+
).headers.get('ETag') as string
547+
548+
expect(createConditionalFileResponse(file, stale).status).toBe(200)
549+
})
550+
551+
it('matches weakly, so a cache that stored a weak validator still revalidates', () => {
552+
expect(createConditionalFileResponse(file, `W/${etagOf()}`).status).toBe(304)
553+
})
554+
555+
it('matches one entry out of a list, and the wildcard', () => {
556+
expect(createConditionalFileResponse(file, `"other", ${etagOf()}`).status).toBe(304)
557+
expect(createConditionalFileResponse(file, '*').status).toBe(304)
558+
})
559+
560+
it('gives bytes that differ only in one byte different validators', () => {
561+
const a = etagOf()
562+
const b = createConditionalFileResponse(
563+
{ ...file, buffer: Buffer.from('compiled-document-byteS') },
564+
null
565+
).headers.get('ETag')
566+
567+
expect(a).not.toBe(b)
568+
})
569+
})

‎apps/sim/app/api/files/utils.ts‎

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
import { createHash } from 'node:crypto'
12
import { createLogger } from '@sim/logger'
23
import { NextResponse } from 'next/server'
34
import {
@@ -267,6 +268,56 @@ export function createFileResponse(file: FileResponse): NextResponse {
267268
return new NextResponse(file.buffer as BodyInit, { status: 200, headers })
268269
}
269270

271+
/**
272+
* Whether an `If-None-Match` header claims the client already holds `etag`.
273+
*
274+
* Compared weakly, per RFC 9110: a cache that stored the response under a weak validator sends
275+
* `W/"…"` back, and that still identifies the same bytes for a GET.
276+
*/
277+
function ifNoneMatchHolds(header: string | null, etag: string): boolean {
278+
if (!header) return false
279+
if (header.trim() === '*') return true
280+
return header.split(',').some((candidate) => candidate.trim().replace(/^W\//, '').trim() === etag)
281+
}
282+
283+
/**
284+
* A file response carrying a strong validator, answering 304 when the client already holds
285+
* exactly these bytes.
286+
*
287+
* For responses the browser is told to revalidate, the alternative is re-sending the whole body on
288+
* every check — and a document resolved against other files is re-resolved per request precisely
289+
* BECAUSE its bytes may have changed, so it cannot be given a cache lifetime instead. The
290+
* validator is the digest of the bytes about to be sent, which makes it exact by construction: it
291+
* cannot claim freshness for a body that differs, however the body was produced.
292+
*
293+
* This is deliberately NOT folded into {@link createFileResponse}. Digesting costs a pass over the
294+
* buffer — up to the full transfer ceiling — which is worth it only where a 304 can actually be
295+
* returned. A response already served as immutable is never revalidated, so it would pay the pass
296+
* and never collect.
297+
*/
298+
export function createConditionalFileResponse(
299+
file: FileResponse,
300+
ifNoneMatch: string | null
301+
): NextResponse {
302+
const etag = `"${createHash('sha256').update(file.buffer).digest('base64url')}"`
303+
304+
if (ifNoneMatchHolds(ifNoneMatch, etag)) {
305+
// A 304 repeats the headers that govern caching, so the stored response is refreshed with the
306+
// lifetime this request would have granted it rather than keeping the one it was stored with.
307+
return new NextResponse(null, {
308+
status: 304,
309+
headers: {
310+
ETag: etag,
311+
'Cache-Control': file.cacheControl || 'private, no-cache',
312+
},
313+
})
314+
}
315+
316+
const response = createFileResponse(file)
317+
response.headers.set('ETag', etag)
318+
return response
319+
}
320+
270321
export function createErrorResponse(error: Error, status = 500): NextResponse {
271322
const statusCode =
272323
error instanceof FileNotFoundError

0 commit comments

Comments
 (0)