Skip to content

Commit 6e9406c

Browse files
committed
fix(uploads): bound Sim-page asset inlining before the bytes are resident
The ceiling on the rendered page checked the finished document, by which point renderSimPageDocumentWithAssets had already downloaded every referenced image concurrently with no per-download limit and base64-inlined them — so the allocation the check exists to prevent had already happened. Pick the inline set from recorded sizes before fetching anything, against a per-document budget as well as the existing per-image one, and give each download its own ceiling in case a row understates its object. An image that does not fit keeps its URL reference, exactly as an oversized one already did.
1 parent 4fdd3e9 commit 6e9406c

2 files changed

Lines changed: 151 additions & 7 deletions

File tree

Lines changed: 112 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,112 @@
1+
/**
2+
* @vitest-environment node
3+
*/
4+
import { beforeEach, describe, expect, it, vi } from 'vitest'
5+
6+
const { mockDownloadFile, mockGetFileMetadataById, mockRenderSimPageDocument } = vi.hoisted(() => ({
7+
mockDownloadFile: vi.fn(),
8+
mockGetFileMetadataById: vi.fn(),
9+
mockRenderSimPageDocument: vi.fn(),
10+
}))
11+
12+
vi.mock('@/lib/uploads/core/storage-service', () => ({
13+
downloadFile: mockDownloadFile,
14+
}))
15+
16+
vi.mock('@/lib/uploads/server/metadata', () => ({
17+
getFileMetadataById: mockGetFileMetadataById,
18+
}))
19+
20+
vi.mock('@/lib/workspace-files/page-document', () => ({
21+
renderSimPageDocument: mockRenderSimPageDocument,
22+
}))
23+
24+
import { renderSimPageDocumentWithAssets } from '@/lib/workspace-files/page-document.server'
25+
26+
const WORKSPACE_ID = 'ws-1'
27+
const MB = 1024 * 1024
28+
29+
function imageRecord(id: string, size: number) {
30+
return {
31+
id,
32+
key: `workspace/${WORKSPACE_ID}/${id}.png`,
33+
context: 'workspace',
34+
workspaceId: WORKSPACE_ID,
35+
contentType: 'image/png',
36+
size,
37+
sizeBytes: size,
38+
}
39+
}
40+
41+
function documentReferencing(ids: string[]) {
42+
return ids.map((id) => `<img src="/api/files/view/${id}">`).join('')
43+
}
44+
45+
describe('renderSimPageDocumentWithAssets memory bounds', () => {
46+
beforeEach(() => {
47+
vi.clearAllMocks()
48+
mockDownloadFile.mockImplementation(async ({ maxBytes }) => Buffer.alloc(maxBytes ?? 4 * MB))
49+
})
50+
51+
it('stops inlining once the per-document budget is spent, without fetching the rest', async () => {
52+
// Five 8MB images against a 32MB document budget: four fit, the fifth must not
53+
// even be downloaded — discovering its size after the fact is the bug.
54+
const ids = ['a', 'b', 'c', 'd', 'e']
55+
mockRenderSimPageDocument.mockReturnValue(documentReferencing(ids))
56+
mockGetFileMetadataById.mockImplementation(async (id: string) => imageRecord(id, 8 * MB))
57+
mockDownloadFile.mockImplementation(async () => Buffer.alloc(8 * MB))
58+
59+
const html = await renderSimPageDocumentWithAssets('source', { workspaceId: WORKSPACE_ID })
60+
61+
expect(mockDownloadFile).toHaveBeenCalledTimes(4)
62+
// The image that did not fit keeps its URL reference rather than failing the render.
63+
expect(html).toContain('src="/api/files/view/e"')
64+
})
65+
66+
it('never fetches an image whose recorded size already exceeds the per-image limit', async () => {
67+
mockRenderSimPageDocument.mockReturnValue(documentReferencing(['big']))
68+
mockGetFileMetadataById.mockResolvedValue(imageRecord('big', 9 * MB))
69+
70+
const html = await renderSimPageDocumentWithAssets('source', { workspaceId: WORKSPACE_ID })
71+
72+
expect(mockDownloadFile).not.toHaveBeenCalled()
73+
expect(html).toContain('src="/api/files/view/big"')
74+
})
75+
76+
it('caps each download so a row understating its object cannot be inlined', async () => {
77+
mockRenderSimPageDocument.mockReturnValue(documentReferencing(['liar']))
78+
mockGetFileMetadataById.mockResolvedValue(imageRecord('liar', 1024))
79+
80+
await renderSimPageDocumentWithAssets('source', { workspaceId: WORKSPACE_ID })
81+
82+
expect(mockDownloadFile).toHaveBeenCalledWith(
83+
expect.objectContaining({ maxBytes: 8 * MB, context: 'workspace' })
84+
)
85+
})
86+
87+
it('keeps the URL reference when a capped download rejects', async () => {
88+
mockRenderSimPageDocument.mockReturnValue(documentReferencing(['liar']))
89+
mockGetFileMetadataById.mockResolvedValue(imageRecord('liar', 1024))
90+
mockDownloadFile.mockRejectedValue(new Error('storage download exceeds maximum size'))
91+
92+
const html = await renderSimPageDocumentWithAssets('source', { workspaceId: WORKSPACE_ID })
93+
94+
expect(html).toContain('src="/api/files/view/liar"')
95+
})
96+
97+
it('inlines images that fit and leaves cross-workspace references alone', async () => {
98+
mockRenderSimPageDocument.mockReturnValue(documentReferencing(['mine', 'theirs']))
99+
mockGetFileMetadataById.mockImplementation(async (id: string) =>
100+
id === 'mine'
101+
? imageRecord('mine', 1024)
102+
: { ...imageRecord('theirs', 1024), workspaceId: 'ws-2' }
103+
)
104+
mockDownloadFile.mockResolvedValue(Buffer.from('png-bytes'))
105+
106+
const html = await renderSimPageDocumentWithAssets('source', { workspaceId: WORKSPACE_ID })
107+
108+
expect(mockDownloadFile).toHaveBeenCalledTimes(1)
109+
expect(html).toContain(`data:image/png;base64,${Buffer.from('png-bytes').toString('base64')}`)
110+
expect(html).toContain('src="/api/files/view/theirs"')
111+
})
112+
})

apps/sim/lib/workspace-files/page-document.server.ts

Lines changed: 39 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,14 @@ import { renderSimPageDocument } from '@/lib/workspace-files/page-document'
55
/** Images past this size stay as URL references rather than bloating the document. */
66
const MAX_INLINE_IMAGE_BYTES = 8 * 1024 * 1024
77

8+
/**
9+
* Ceiling on everything a single document inlines. A per-image limit does not bound
10+
* the page on its own: N images each just under it still cost N times it, and they
11+
* are fetched concurrently, so that product is also the peak. Images that do not fit
12+
* the remaining budget keep their URL reference, exactly like an oversized one.
13+
*/
14+
const MAX_INLINE_TOTAL_BYTES = 32 * 1024 * 1024
15+
816
const IMAGE_SRC = /src="[^"]*\/api\/files\/view\/([^"]+)"/g
917

1018
/**
@@ -24,21 +32,45 @@ export async function renderSimPageDocumentWithAssets(
2432
const ids = [...new Set([...documentHtml.matchAll(IMAGE_SRC)].map((match) => match[1]))]
2533
if (ids.length === 0 || !options.workspaceId) return documentHtml
2634

35+
const candidates = await Promise.all(
36+
ids.map(async (id) => {
37+
const record = await getFileMetadataById(id).catch(() => null)
38+
if (!record || record.context !== 'workspace' || record.workspaceId !== options.workspaceId)
39+
return null
40+
return { id, record }
41+
})
42+
)
43+
44+
// Pick the inline set from recorded sizes BEFORE fetching anything, so the concurrent
45+
// downloads below are bounded in count and in total bytes rather than discovering the
46+
// size of each image only once it is already resident. These sizes are written by the
47+
// upload pipeline, not supplied by the caller, so they are sound to plan against —
48+
// each download still carries its own ceiling in case a row understates its object.
49+
let remaining = MAX_INLINE_TOTAL_BYTES
50+
const eligible: NonNullable<(typeof candidates)[number]>[] = []
51+
for (const candidate of candidates) {
52+
if (!candidate) continue
53+
const size = candidate.record.sizeBytes ?? candidate.record.size
54+
if (size > MAX_INLINE_IMAGE_BYTES || size > remaining) continue
55+
remaining -= size
56+
eligible.push(candidate)
57+
}
58+
2759
const inlined = new Map<string, string>()
2860
await Promise.all(
29-
ids.map(async (id) => {
61+
eligible.map(async ({ id, record }) => {
3062
try {
31-
const record = await getFileMetadataById(id)
32-
if (!record || record.context !== 'workspace' || record.workspaceId !== options.workspaceId)
33-
return
34-
const bytes = await downloadFile({ key: record.key, context: 'workspace' })
35-
if (bytes.length > MAX_INLINE_IMAGE_BYTES) return
63+
const bytes = await downloadFile({
64+
key: record.key,
65+
context: 'workspace',
66+
maxBytes: MAX_INLINE_IMAGE_BYTES,
67+
})
3668
const mime = record.contentType?.startsWith('image/')
3769
? record.contentType
3870
: 'application/octet-stream'
3971
inlined.set(id, `data:${mime};base64,${bytes.toString('base64')}`)
4072
} catch {
41-
// A missing or unreadable image keeps its URL reference.
73+
// A missing, unreadable or oversized image keeps its URL reference.
4274
}
4375
})
4476
)

0 commit comments

Comments
 (0)