Skip to content

Commit abe134a

Browse files
committed
fix(files): refuse unrendered bytes at the download boundary
Addresses the second review round. - Throw UnrenderableDocumentError from downloadServableFileFromStorage instead of returning bytes with an `unrendered` flag. Around 45 call sites (email attachments, cloud uploads, zip entries, provider attachments) receive only a Buffer and re-infer the type from the filename, so a flag they must remember to check is a flag they will not check. Those callers already handled the previous not-ready throw, so failing is the shape they expect. The file-serve route is unaffected — it resolves bytes directly and keeps the graceful passthrough, where a human downloading the file has a use for it. - Surface that failure through hydration: with throwOnDocNotReady set, the caller cannot use a file with no content, so an unrenderable document now reaches it verbatim instead of degrading to null and reporting a misleading "may exceed size limit or no longer accessible". - Stop a shared render inheriting one caller's cancellation. The coalesced run no longer carries any caller's signal; each caller races its own instead, so an aborting reader gives up promptly while the render finishes for the others and still lands in the cache.
1 parent 6a72bba commit abe134a

6 files changed

Lines changed: 98 additions & 20 deletions

File tree

apps/sim/lib/copilot/tools/server/files/doc-compile.ts

Lines changed: 22 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -512,6 +512,19 @@ const unrenderableSources = new Set<string>()
512512
*/
513513
const inFlightRenders = new Map<string, Promise<{ buffer: Buffer; contentType: string }>>()
514514

515+
/** Rejects when `signal` aborts, so a caller can abandon a shared render without cancelling it. */
516+
function rejectOnAbort(signal: AbortSignal): Promise<never> {
517+
return new Promise((_resolve, reject) => {
518+
if (signal.aborted) {
519+
reject(signal.reason ?? new Error('Aborted'))
520+
return
521+
}
522+
signal.addEventListener('abort', () => reject(signal.reason ?? new Error('Aborted')), {
523+
once: true,
524+
})
525+
})
526+
}
527+
515528
function coalesceRender(
516529
key: string,
517530
run: () => Promise<{ buffer: Buffer; contentType: string }>
@@ -667,15 +680,22 @@ export async function resolveServableDocBytes(args: {
667680
}
668681

669682
try {
670-
return await coalesceRender(renderKey, async () => {
683+
// The shared run deliberately carries no caller's signal: it is one piece of
684+
// work several readers are waiting on, so letting whoever happened to start it
685+
// cancel it would reject every other waiter with an AbortError they did not
686+
// ask for. Each caller instead races its own signal, so an aborting reader
687+
// gives up promptly while the render continues for the rest and still lands in
688+
// the cache.
689+
const shared = coalesceRender(renderKey, async () => {
671690
const compiled = await runSandboxTask(
672691
format.taskId,
673692
{ code: source, workspaceId: workspaceId || '' },
674-
{ ownerKey, signal }
693+
{ ownerKey }
675694
)
676695
compiledCacheSet(renderKey, compiled)
677696
return { buffer: compiled, contentType: format.contentType }
678697
})
698+
return await (signal ? Promise.race([shared, rejectOnAbort(signal)]) : shared)
679699
} catch (error) {
680700
// Unlike the E2B engine, the isolated-vm task does not distinguish a script
681701
// error from an infra one, so the only signal available here is cancellation —

apps/sim/lib/execution/payloads/materialization.server.ts

Lines changed: 0 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -315,19 +315,6 @@ export async function readUserFileContent(
315315
if (!servable) {
316316
throw new Error(`File content for ${file.name} is unavailable.`)
317317
}
318-
if (servable.unrendered) {
319-
// The resolver could not produce the document these bytes claim to be and is
320-
// serving them under a generic type. Every consumer here feeds the result to
321-
// something that expects the real document — a provider attachment, a document
322-
// parser, a function-block read — and this function returns only a string, so
323-
// the honest content type cannot travel with it. Relabelling the source as a
324-
// PDF downstream is the corruption this module exists to prevent, so refuse.
325-
// (The file-serve route keeps the graceful passthrough: a human downloading
326-
// the bytes and seeing what they actually are is useful.)
327-
throw new Error(
328-
`File ${file.name} could not be rendered; its stored bytes are not the format its name claims.`
329-
)
330-
}
331318
const { buffer } = servable
332319
if (buffer.length > maxSourceBytes) {
333320
throw new ExecutionResourceLimitError({

apps/sim/lib/uploads/utils/file-utils.server.test.ts

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -163,6 +163,30 @@ describe('downloadServableFileFromStorage generated-doc gating', () => {
163163
)
164164
})
165165

166+
it('refuses unrendered bytes rather than handing them to a caller that expects the document', async () => {
167+
// ~45 call sites (email attachments, cloud uploads, zip entries, provider
168+
// attachments) receive only a Buffer and re-infer the type from the filename, so
169+
// an opt-in flag would be silently ignored by all of them. Failing here is what
170+
// keeps generation source from going out under a .pdf.
171+
mockResolveServableDocBytes.mockResolvedValue({
172+
buffer: Buffer.from('<html>not a pdf</html>'),
173+
contentType: 'application/octet-stream',
174+
unrendered: true,
175+
})
176+
const userFile: UserFile = {
177+
id: 'f7',
178+
name: 'report.pdf',
179+
url: '',
180+
size: 5,
181+
type: 'text/x-python-pdf',
182+
key: 'workspace/ws-1/1700000000000-abc1234-report.pdf',
183+
}
184+
185+
await expect(
186+
downloadServableFileFromStorage(userFile, 'req-1', createLogger('test'))
187+
).rejects.toThrow(/could not be rendered/)
188+
})
189+
166190
it('does not mistake the generic octet-stream fallback for a real declared type', async () => {
167191
// convertToUserFile emits application/octet-stream for a type-less input, so this
168192
// value carries no information about whether the bytes are generation source.

apps/sim/lib/uploads/utils/file-utils.server.ts

Lines changed: 32 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -349,13 +349,38 @@ export interface ServableFile {
349349
contentType: string
350350
/**
351351
* Set when the bytes could not be rendered and are being served as-is under a
352-
* generic content type. The bytes are NOT the document the filename claims, so a
353-
* consumer that hands them to something expecting that format (a provider
354-
* attachment, a document parser) must refuse rather than relabel them.
352+
* generic content type. The bytes are NOT the document the filename claims.
353+
* {@link downloadServableFileFromStorage} refuses these rather than returning
354+
* them — see {@link UnrenderableDocumentError}.
355355
*/
356356
unrendered?: boolean
357357
}
358358

359+
/**
360+
* Thrown when a file's stored bytes are not the format its name claims and could
361+
* not be rendered into it.
362+
*
363+
* Every caller of {@link downloadServableFileFromStorage} hands the bytes to
364+
* something that expects the real document — an email attachment, a cloud upload,
365+
* a zip entry, a provider attachment — and none of them can tell a rendered
366+
* artifact from raw generation source once it is just a Buffer. Returning the
367+
* bytes with an honest content type would still be wrong, because the filename
368+
* travels separately and downstream re-infers the type from it. Failing here is
369+
* the only place that reliably prevents source text going out under a `.pdf`.
370+
*
371+
* The file-serve route deliberately does NOT go through this helper: it resolves
372+
* bytes directly and keeps the graceful passthrough, because a human downloading
373+
* the file and seeing what it actually is has a use for it.
374+
*/
375+
export class UnrenderableDocumentError extends Error {
376+
constructor(fileName: string) {
377+
super(
378+
`File ${fileName} could not be rendered; its stored bytes are not the format its name claims.`
379+
)
380+
this.name = 'UnrenderableDocumentError'
381+
}
382+
}
383+
359384
/**
360385
* Downloads a workspace file and resolves it to its SERVABLE bytes — the variant
361386
* every tool that hands a file to an external service (email attachments, chat
@@ -420,6 +445,10 @@ export async function downloadServableFileFromStorage(
420445
signal: options.signal,
421446
})
422447

448+
if (resolved.unrendered) {
449+
throw new UnrenderableDocumentError(userFile.name)
450+
}
451+
423452
// Re-check: the raw download enforced maxBytes on the source, but a generated doc
424453
// resolves to a larger artifact.
425454
if (options.maxBytes !== undefined && resolved.buffer.length > options.maxBytes) {

apps/sim/lib/uploads/utils/user-file-base64.server.test.ts

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,15 @@ const RENDERED_PDF_BUFFER = Buffer.from('%PDF-1.4 rendered', 'utf8')
4747

4848
vi.mock('@/lib/uploads/utils/file-utils.server', () => ({
4949
downloadFileFromStorage: mockDownloadFile,
50+
// Mirrors the real module: resolveBase64 narrows on this class to decide whether a
51+
// strict caller sees the failure, so omitting it from the mock would break that
52+
// check rather than exercise it.
53+
UnrenderableDocumentError: class UnrenderableDocumentError extends Error {
54+
constructor(fileName: string) {
55+
super(`File ${fileName} could not be rendered`)
56+
this.name = 'UnrenderableDocumentError'
57+
}
58+
},
5059
downloadServableFileFromStorage: async (
5160
file: UserFile,
5261
requestId: string,

apps/sim/lib/uploads/utils/user-file-base64.server.ts

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -443,12 +443,21 @@ async function resolveBase64(
443443
// already-finished result opt out, so a late compile cannot retroactively
444444
// fail completed work.
445445
if (options.throwOnDocNotReady) {
446+
// This caller cannot use a file without content, so any reason the bytes are
447+
// missing must reach it verbatim. Degrading to null here is what produced the
448+
// misleading "may exceed size limit or no longer accessible" message for a
449+
// document that was actually still compiling, or one whose stored bytes are
450+
// not the format its name claims.
451+
//
446452
// Imported lazily: `servable-file-response` pulls in the doc-compile module
447453
// graph (remote sandbox, sandbox task runner, execution limits), and a static
448454
// import here would load all of it for every hydration consumer — mirroring
449455
// the deliberate dynamic import in file-utils.server.ts.
450-
const { isDocNotReadyError } = await import('@/lib/uploads/utils/servable-file-response')
451-
if (isDocNotReadyError(error)) {
456+
const [{ isDocNotReadyError }, { UnrenderableDocumentError }] = await Promise.all([
457+
import('@/lib/uploads/utils/servable-file-response'),
458+
import('@/lib/uploads/utils/file-utils.server'),
459+
])
460+
if (isDocNotReadyError(error) || error instanceof UnrenderableDocumentError) {
452461
throw error
453462
}
454463
}

0 commit comments

Comments
 (0)