Skip to content

Commit 273ca06

Browse files
authored
fix(files): serve rendered documents instead of source code (#6139)
* fix(files): serve rendered documents to attachments and workflow reads Generated documents store their generation source under a .pdf/.docx name and keep the compiled binary in a separate content-addressed artifact store, so any consumer doing a raw read handed out source text under a document name. - Route attachments and readUserFileContent through the servable resolver so they get the compiled artifact, and stop the internal generation-source MIME marker reaching providers as a content type. - Render on read when the artifact is missing. The artifact key is (workspace, source hash), so forking a workspace, moving a file, or editing the source outside a recompiling writer orphaned it permanently and reported "still being generated" forever. Rendering self-heals those and stores the result. - Fall back to serving stored bytes as application/octet-stream when a render fails, instead of failing forever, and remember not to retry those bytes. - Only compile without a workspace context when the file's type positively says it is generation source, so unrelated stored bytes are never executed. - Make the doc-not-ready error opt-in per caller and give it a 409 via HttpError, so output decoration degrades instead of failing completed work. * fix(files): keep render failures retryable and never relabel unrendered bytes Addresses the first review round. - Only memoize a render failure when it is deterministic. A DocCompileUserError means the source will never render, so remembering it is safe; sandbox outages, timeouts, and cancellations are transient and were stranding valid documents for the life of the process. Infra failures now propagate, which also restores DocCompileUserError reaching callers that map it to 409. - Refuse to hand back bytes the resolver could not render. readUserFileContent returns a string, so the resolver's honest application/octet-stream could not travel with it and attachment builders re-inferred a document MIME from the filename — shipping generation source to a provider as a PDF. The file-serve route keeps the graceful passthrough, where a human downloading the bytes is useful. - Normalize the declared type once so a padded or upper-cased source marker cannot pass the resolver gate on one code path and fail it on the other. - Import the doc-not-ready guard lazily. The static import pulled the doc-compile module graph (remote sandbox, task runner, execution limits) into every hydration consumer and broke an unrelated test's module mock in CI. * perf(files): coalesce concurrent renders of the same missing artifact An artifact miss is identical for every concurrent reader — a freshly forked workspace whose document several viewers open at once, or one request whose blocks read the same file — and each was paying for its own compile of the same bytes. Share one in-flight render per (workspace, source, ext) key and drop the entry as soon as it settles, so a later read still re-renders normally. * 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. * fix(files): move the unrenderable error out of the 'use server' module file-utils.server.ts carries 'use server', whose exports must all be async functions, so exporting an error class from it failed the production build with 67 cascading errors. The class now lives in the plain file-utils.ts beside the other shared file helpers, which also lets the hydration path import it directly instead of through a dynamic import. Also bounds how long a failed render is remembered. The isolated-vm engine cannot tell a bad source from a sandbox outage, so a permanent entry let one transient failure block re-rendering that source for the life of the process. Entries now expire after five minutes: long enough to stop a read loop spending a sandbox run per read, short enough that an outage self-heals without a deploy. * fix(files): finish the render cancellation and failure-surfacing edges - Race the E2B render against the caller's signal too. Only the isolated-vm branch did, so an aborted request on the E2B path waited for the sandbox to finish and could return a success the caller no longer wanted. - Attach a terminal handler to the shared render. Every caller races it against its own signal, so all of them can walk away; a later rejection with no waiters left would otherwise surface as an unhandled rejection. - Stop narrowing what throwOnDocNotReady rethrows. readUserFileContent now runs document compiles and can fail in ways this module has no business enumerating; narrowing produced three consecutive review rounds of "this particular failure is still swallowed". The flag means "do not degrade". - Do not mark an unrendered response immutable. The serve route caches versioned responses for a year, which would pin a one-off render failure to that URL long after a later compile succeeds on the same version. * revert(files): drop the concurrent-render coalescing The coalescing was an optional efficiency win — rendering is content-addressed and idempotent, so duplicate concurrent renders produced the same artifact and cost only extra sandbox time on an artifact miss. It bought that at the price of the most intricate code in the change set, and produced three concurrency findings across two review rounds: a shared render inheriting one caller's cancellation, an E2B/isolated-vm asymmetry in how the signal was raced, and orphaned rejections once every caller could race away. Removing it also restores true cancellation on the isolated-vm path: the caller's signal now reaches runSandboxTask again, so an abort cancels the sandbox work rather than only abandoning the wait for it. * fix(review): simplify generated document attachments * fix(files): mock servable downloads in hydration tests * fix(files): preserve rendered attachment semantics * fix(files): preserve cached artifact size * fix(files): refuse unresolved xlsx source * fix(files): resolve execution artifact workspace
1 parent 06506bb commit 273ca06

10 files changed

Lines changed: 306 additions & 48 deletions

File tree

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

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -541,8 +541,9 @@ export async function resolveServableDocBytes(args: {
541541
}
542542
}
543543

544-
// Reaches here only for xlsx, which has no isolated-vm fallback.
545-
if (!format) return { buffer: rawBuffer, contentType: getContentType(fileName) }
544+
// Reaches here only for xlsx, which has no isolated-vm fallback. Returning these
545+
// bytes would expose generation source as a spreadsheet.
546+
if (!format) throw new DocCompileUserError('Document is still being generated')
546547

547548
const cacheKey = sha256Hex(`${ext}${source}${workspaceId ?? ''}`)
548549
const cached = compiledDocCache.get(cacheKey)

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

Lines changed: 23 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -158,14 +158,30 @@ describe('resolveServableDocBytes', () => {
158158
expect(mockRunSandboxTask).not.toHaveBeenCalled()
159159
})
160160

161-
it('returns raw XLSX source when there is no workspaceId (xlsx has no isolated-vm path)', async () => {
162-
const result = await resolveServableDocBytes({
163-
rawBuffer: XLSX_SOURCE,
164-
fileName: 'sheet.xlsx',
165-
workspaceId: undefined,
166-
})
161+
it('throws instead of returning XLSX source when E2B is disabled', async () => {
162+
mockLoadCompiledDoc.mockResolvedValue(null)
163+
setEnvFlags({ isDocSandboxEnabled: false })
164+
165+
await expect(
166+
resolveServableDocBytes({
167+
rawBuffer: XLSX_SOURCE,
168+
fileName: 'sheet.xlsx',
169+
workspaceId: WORKSPACE_ID,
170+
})
171+
).rejects.toBeInstanceOf(DocCompileUserError)
172+
173+
expect(mockRunSandboxTask).not.toHaveBeenCalled()
174+
})
175+
176+
it('throws instead of returning XLSX source when there is no workspaceId', async () => {
177+
await expect(
178+
resolveServableDocBytes({
179+
rawBuffer: XLSX_SOURCE,
180+
fileName: 'sheet.xlsx',
181+
workspaceId: undefined,
182+
})
183+
).rejects.toBeInstanceOf(DocCompileUserError)
167184

168-
expect(result.buffer).toBe(XLSX_SOURCE)
169185
expect(mockLoadCompiledDoc).not.toHaveBeenCalled()
170186
expect(mockRunSandboxTask).not.toHaveBeenCalled()
171187
})
Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,56 @@
1+
/**
2+
* @vitest-environment node
3+
*/
4+
import { beforeEach, describe, expect, it, vi } from 'vitest'
5+
6+
const { mockDownloadServableFileFromStorage, mockVerifyFileAccess } = vi.hoisted(() => ({
7+
mockDownloadServableFileFromStorage: vi.fn(),
8+
mockVerifyFileAccess: vi.fn(),
9+
}))
10+
11+
vi.mock('@/lib/uploads/utils/file-utils.server', () => ({
12+
downloadServableFileFromStorage: mockDownloadServableFileFromStorage,
13+
}))
14+
15+
vi.mock('@/app/api/files/authorization', () => ({
16+
verifyFileAccess: mockVerifyFileAccess,
17+
}))
18+
19+
import { readUserFileContent } from '@/lib/execution/payloads/materialization.server'
20+
import type { UserFile } from '@/executor/types'
21+
22+
const PDF_SOURCE = Buffer.from('from reportlab.pdfgen import canvas')
23+
const PDF_BYTES = Buffer.from('%PDF-1.4 rendered bytes')
24+
25+
const generatedPdf: UserFile = {
26+
id: 'file-1',
27+
name: 'report.pdf',
28+
url: '',
29+
size: PDF_SOURCE.length,
30+
type: 'text/x-python-pdf',
31+
key: 'workspace/2f1d8c3e-5b6a-4c7d-8e9f-0a1b2c3d4e5f/1700000000000-abc1234-report.pdf',
32+
}
33+
34+
describe('readUserFileContent', () => {
35+
beforeEach(() => {
36+
vi.clearAllMocks()
37+
generatedPdf.size = PDF_SOURCE.length
38+
mockVerifyFileAccess.mockResolvedValue(true)
39+
mockDownloadServableFileFromStorage.mockResolvedValue({
40+
buffer: PDF_BYTES,
41+
contentType: 'application/pdf',
42+
})
43+
})
44+
45+
it('returns the compiled artifact instead of the stored generation source', async () => {
46+
const content = await readUserFileContent(generatedPdf, {
47+
userId: 'user-1',
48+
encoding: 'base64',
49+
})
50+
51+
expect(mockDownloadServableFileFromStorage).toHaveBeenCalledOnce()
52+
expect(content).toBe(PDF_BYTES.toString('base64'))
53+
expect(content).not.toBe(PDF_SOURCE.toString('base64'))
54+
expect(generatedPdf.size).toBe(PDF_BYTES.length)
55+
})
56+
})

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

Lines changed: 20 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -10,8 +10,12 @@ import {
1010
} from '@/lib/execution/payloads/large-value-ref'
1111
import { ExecutionResourceLimitError } from '@/lib/execution/resource-errors'
1212
import type { StorageContext } from '@/lib/uploads'
13-
import { bufferToBase64, inferContextFromKey } from '@/lib/uploads/utils/file-utils'
14-
import { downloadFileFromStorage } from '@/lib/uploads/utils/file-utils.server'
13+
import {
14+
bufferToBase64,
15+
inferContextFromKey,
16+
isGeneratedDocumentSourceType,
17+
} from '@/lib/uploads/utils/file-utils'
18+
import { downloadServableFileFromStorage } from '@/lib/uploads/utils/file-utils.server'
1519
import type { UserFile } from '@/executor/types'
1620

1721
const logger = createLogger('ExecutionPayloadMaterialization')
@@ -267,6 +271,11 @@ export async function assertUserFileContentAccess(
267271
}
268272
}
269273

274+
/**
275+
* Reads the bytes a consumer should receive. For generated documents, updates the
276+
* file's size to the rendered artifact size so downstream attachment routing does
277+
* not make decisions from the smaller generation-source size.
278+
*/
270279
export async function readUserFileContent(
271280
file: unknown,
272281
options: ReadUserFileContentOptions
@@ -291,9 +300,14 @@ export async function readUserFileContent(
291300
const requestId = options.requestId ?? 'unknown'
292301

293302
try {
294-
buffer = await downloadFileFromStorage(file, requestId, log, { maxBytes: maxSourceBytes })
303+
buffer = (
304+
await downloadServableFileFromStorage(file, requestId, log, { maxBytes: maxSourceBytes })
305+
).buffer
295306
} catch (error) {
296307
if (isPayloadSizeLimitError(error)) {
308+
if (isGeneratedDocumentSourceType(file.type) && error.observedBytes !== undefined) {
309+
file.size = error.observedBytes
310+
}
297311
throw new ExecutionResourceLimitError({
298312
resource: 'execution_payload_bytes',
299313
attemptedBytes: error.observedBytes ?? maxSourceBytes + 1,
@@ -306,6 +320,9 @@ export async function readUserFileContent(
306320
if (!buffer) {
307321
throw new Error(`File content for ${file.name} is unavailable.`)
308322
}
323+
if (isGeneratedDocumentSourceType(file.type)) {
324+
file.size = buffer.length
325+
}
309326
if (buffer.length > maxSourceBytes) {
310327
throw new ExecutionResourceLimitError({
311328
resource: 'execution_payload_bytes',

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

Lines changed: 47 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3,27 +3,51 @@
33
*/
44
import { beforeEach, describe, expect, it, vi } from 'vitest'
55

6-
const { mockDownloadFile } = vi.hoisted(() => ({
7-
mockDownloadFile: vi.fn(),
8-
}))
6+
const { mockDownloadFile, mockParseWorkspaceFileKey, mockResolveServableDocBytes } = vi.hoisted(
7+
() => ({
8+
mockDownloadFile: vi.fn(),
9+
mockParseWorkspaceFileKey: vi.fn(),
10+
mockResolveServableDocBytes: vi.fn(),
11+
})
12+
)
913

1014
vi.mock('@/lib/uploads/core/storage-service', () => ({
1115
downloadFile: mockDownloadFile,
1216
hasCloudStorage: vi.fn(() => true),
1317
}))
1418

19+
vi.mock('@/lib/uploads/contexts/execution/execution-file-manager', () => ({
20+
downloadExecutionFile: mockDownloadFile,
21+
}))
22+
23+
vi.mock('@/lib/uploads/contexts/workspace/workspace-file-manager', () => ({
24+
parseWorkspaceFileKey: mockParseWorkspaceFileKey,
25+
}))
26+
27+
vi.mock('@/lib/copilot/tools/server/files/doc-compile', () => ({
28+
resolveServableDocBytes: mockResolveServableDocBytes,
29+
}))
30+
1531
vi.mock('@/app/api/files/authorization', () => ({
1632
verifyFileAccess: vi.fn(),
1733
}))
1834

1935
import { createLogger } from '@sim/logger'
20-
import { downloadFileFromStorage } from '@/lib/uploads/utils/file-utils.server'
36+
import {
37+
downloadFileFromStorage,
38+
downloadServableFileFromStorage,
39+
} from '@/lib/uploads/utils/file-utils.server'
2140
import type { UserFile } from '@/executor/types'
2241

2342
describe('downloadFileFromStorage context derivation', () => {
2443
beforeEach(() => {
2544
vi.clearAllMocks()
2645
mockDownloadFile.mockResolvedValue(Buffer.from('bytes'))
46+
mockParseWorkspaceFileKey.mockReturnValue(null)
47+
mockResolveServableDocBytes.mockImplementation(async ({ rawBuffer }) => ({
48+
buffer: rawBuffer,
49+
contentType: 'application/pdf',
50+
}))
2751
})
2852

2953
it('downloads with the key-derived context, ignoring a caller-supplied public context', async () => {
@@ -44,4 +68,23 @@ describe('downloadFileFromStorage context derivation', () => {
4468
expect.objectContaining({ key: userFile.key, context: 'workspace' })
4569
)
4670
})
71+
72+
it('uses the workspace ID embedded in an execution key to resolve generated artifacts', async () => {
73+
const workspaceId = '2f1d8c3e-5b6a-4c7d-8e9f-0a1b2c3d4e5f'
74+
const userFile: UserFile = {
75+
id: 'f1',
76+
name: 'report.pdf',
77+
url: '',
78+
size: 5,
79+
type: 'text/x-python-pdf',
80+
key: `execution/${workspaceId}/3f2e9d4c-6a7b-4d8e-9f0a-1b2c3d4e5f6a/4a3b2c1d-7e8f-4a9b-8c0d-1e2f3a4b5c6d/report.pdf`,
81+
context: 'execution',
82+
}
83+
84+
await downloadServableFileFromStorage(userFile, 'req-1', createLogger('test'))
85+
86+
expect(mockResolveServableDocBytes).toHaveBeenCalledWith(
87+
expect.objectContaining({ workspaceId })
88+
)
89+
})
4790
})

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

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ import { StorageService } from '@/lib/uploads'
1616
import { isExecutionFile } from '@/lib/uploads/contexts/execution/utils'
1717
import {
1818
extractStorageKey,
19+
extractWorkspaceIdFromExecutionKey,
1920
getFileExtension,
2021
getMimeTypeFromExtension,
2122
inferContextFromKey,
@@ -384,7 +385,11 @@ export async function downloadServableFileFromStorage(
384385
const { parseWorkspaceFileKey } = await import(
385386
'@/lib/uploads/contexts/workspace/workspace-file-manager'
386387
)
387-
const workspaceId = userFile.key ? (parseWorkspaceFileKey(userFile.key) ?? undefined) : undefined
388+
const workspaceId = userFile.key
389+
? (parseWorkspaceFileKey(userFile.key) ??
390+
extractWorkspaceIdFromExecutionKey(userFile.key) ??
391+
undefined)
392+
: undefined
388393

389394
const { resolveServableDocBytes } = await import('@/lib/copilot/tools/server/files/doc-compile')
390395
const resolved = await resolveServableDocBytes({

0 commit comments

Comments
 (0)