Skip to content

Commit 34f7aa0

Browse files
committed
fix(files): bound the artifact and local reads at the read, not at the response
Review round 2. The response-level ceiling rejected an oversized compiled artifact only after the whole thing was resident, which is the allocation it exists to prevent. The artifact read is bounded at its own funnel instead, and a size breach is rethrown rather than folded into the "not built yet" null — that answer tells callers to retry, which an oversized artifact would never stop doing. The cached image derivative is bounded the same way, but keeps swallowing the breach, because a miss there re-transcodes from an already-bounded source. The local storage branch enforced its ceiling with stat-then-read, so it described the file only as of the stat. It now reads through the same bounded-stream reader the cloud branches use — this is the anonymous share path on a self-hosted deployment, and the route helper was already fixed while the storage service it sits next to was not. The default js-yaml schema turns a timestamp into a Date, which serializes to a 26-character quoted string; charging it the 16-byte flat allowance let a document of them exceed the byte cap.
1 parent cbc76d7 commit 34f7aa0

7 files changed

Lines changed: 206 additions & 12 deletions

File tree

‎apps/sim/lib/copilot/tools/server/files/doc-compiled-store.test.ts‎

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,8 @@ import {
1919
loadPublishedCompiledDoc,
2020
storeCompiledDoc,
2121
} from '@/lib/copilot/tools/server/files/doc-compiled-store'
22+
import { PayloadSizeLimitError } from '@/lib/core/utils/stream-limits'
23+
import { MAX_RENDERED_DOCUMENT_BYTES } from '@/lib/uploads/utils/file-utils'
2224

2325
describe('compiled document publication', () => {
2426
beforeEach(() => {
@@ -67,6 +69,54 @@ describe('compiled document publication', () => {
6769
)
6870
})
6971

72+
it('bounds the artifact read so an oversized artifact is never materialized', async () => {
73+
mockHeadObject.mockResolvedValue({ size: 1 })
74+
mockDownloadFile.mockResolvedValueOnce(
75+
Buffer.from(JSON.stringify({ version: 1, referencedInputIdentity: 'dependency-identity' }))
76+
)
77+
mockDownloadFile.mockResolvedValueOnce(Buffer.from('%PDF-artifact'))
78+
79+
await loadPublishedCompiledDoc('workspace-1', 'source', 'pdf')
80+
81+
// The artifact is fetched separately from the source that names it, so a source
82+
// that cleared its own ceiling says nothing about how large this is.
83+
expect(mockDownloadFile.mock.calls[1]?.[0]).toEqual(
84+
expect.objectContaining({ maxBytes: MAX_RENDERED_DOCUMENT_BYTES })
85+
)
86+
})
87+
88+
it('surfaces an oversized artifact instead of reporting it as not yet built', async () => {
89+
// `null` means "still compiling", which callers answer with a retry — an artifact
90+
// that is too large would sit behind that answer forever.
91+
mockHeadObject.mockResolvedValue({ size: 1 })
92+
mockDownloadFile.mockResolvedValueOnce(
93+
Buffer.from(JSON.stringify({ version: 1, referencedInputIdentity: 'dependency-identity' }))
94+
)
95+
mockDownloadFile.mockRejectedValueOnce(
96+
new PayloadSizeLimitError({
97+
label: 'storage download',
98+
maxBytes: MAX_RENDERED_DOCUMENT_BYTES,
99+
observedBytes: MAX_RENDERED_DOCUMENT_BYTES + 1,
100+
})
101+
)
102+
103+
await expect(loadPublishedCompiledDoc('workspace-1', 'source', 'pdf')).rejects.toThrow(
104+
PayloadSizeLimitError
105+
)
106+
})
107+
108+
it('still reports a missing artifact as not yet built', async () => {
109+
mockHeadObject.mockResolvedValue({ size: 1 })
110+
mockDownloadFile.mockResolvedValueOnce(
111+
Buffer.from(JSON.stringify({ version: 1, referencedInputIdentity: 'dependency-identity' }))
112+
)
113+
mockDownloadFile.mockRejectedValueOnce(new Error('NoSuchKey'))
114+
115+
await expect(loadPublishedCompiledDoc('workspace-1', 'source', 'pdf')).rejects.toThrow(
116+
'Published compiled document artifact is missing'
117+
)
118+
})
119+
70120
it('fails fast on a malformed published pointer', async () => {
71121
mockHeadObject.mockResolvedValue({ size: 1 })
72122
mockDownloadFile.mockResolvedValueOnce(Buffer.from('{not-json'))

‎apps/sim/lib/copilot/tools/server/files/doc-compiled-store.ts‎

Lines changed: 17 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,9 @@
11
import { createHash } from 'node:crypto'
22
import { createLogger } from '@sim/logger'
33
import { getErrorMessage, toError } from '@sim/utils/errors'
4+
import { isPayloadSizeLimitError } from '@/lib/core/utils/stream-limits'
45
import { downloadFile, headObject, uploadFile } from '@/lib/uploads/core/storage-service'
6+
import { MAX_RENDERED_DOCUMENT_BYTES } from '@/lib/uploads/utils/file-utils'
57

68
const logger = createLogger('CopilotDocCompiledStore')
79

@@ -65,7 +67,18 @@ async function loadPublishedArtifactPointer(key: string): Promise<PublishedArtif
6567
return { version: 1, referencedInputIdentity: decoded.referencedInputIdentity }
6668
}
6769

68-
/** Loads the compiled binary for the current source, or null if not yet built. */
70+
/**
71+
* Loads the compiled binary for the current source, or null if not yet built.
72+
*
73+
* The artifact is what a download or a public share actually serves, and it is read
74+
* separately from the source — so a source that cleared its own ceiling says nothing
75+
* about the size of this. Bounding it here rather than on the finished response is
76+
* what keeps an oversized artifact from being materialized before it is refused.
77+
*
78+
* A size breach is rethrown rather than folded into `null`: null means "not built
79+
* yet", which callers answer with "still being prepared, try again", and an artifact
80+
* that is too large would retry forever behind that.
81+
*/
6982
export async function loadCompiledDoc(
7083
workspaceId: string,
7184
source: string,
@@ -74,8 +87,9 @@ export async function loadCompiledDoc(
7487
): Promise<Buffer | null> {
7588
const key = compiledArtifactKey(workspaceId, source, ext, referencedInputIdentity)
7689
try {
77-
return await downloadFile({ key, context: 'copilot' })
78-
} catch {
90+
return await downloadFile({ key, context: 'copilot', maxBytes: MAX_RENDERED_DOCUMENT_BYTES })
91+
} catch (error) {
92+
if (isPayloadSizeLimitError(error)) throw error
7993
return null
8094
}
8195
}

‎apps/sim/lib/file-parsers/yaml-limits.test.ts‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,19 @@ describe('measureYamlExpansion', () => {
8080
expect(measureYamlExpansion(longNumbers, byteCap).within).toBe(false)
8181
})
8282

83+
it('charges a Date its quoted ISO length, not the flat allowance', () => {
84+
// The default js-yaml schema turns `!!timestamp` into a Date, and JSON.stringify
85+
// emits it as a 26-character quoted string — an aliased list of them would
86+
// otherwise be charged 16 apiece and slip past the byte cap.
87+
const dates = Array.from({ length: 200 }, () => new Date('2026-08-31T00:00:00.000Z'))
88+
const booleans = Array.from({ length: 200 }, () => true)
89+
90+
const byteCap = limits({ maxSerializedBytes: 5000 })
91+
92+
expect(measureYamlExpansion(booleans, byteCap).within).toBe(true)
93+
expect(measureYamlExpansion(dates, byteCap).within).toBe(false)
94+
})
95+
8396
it('charges object keys, so an aliased object with long keys cannot bypass the cap', () => {
8497
const key = 'k'.repeat(500)
8598
const shared = { [key]: 1 }

‎apps/sim/lib/file-parsers/yaml-limits.ts‎

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -91,6 +91,9 @@ function serializedStringLength(value: string): number {
9191
*/
9292
const NON_STRING_NODE_BYTES = 16
9393

94+
/** `"2026-08-31T00:00:00.000Z"` — 24 characters of ISO 8601 plus its two quotes. */
95+
const SERIALIZED_DATE_BYTES = 26
96+
9497
/**
9598
* Estimate the pretty-printed (`JSON.stringify(value, null, 2)`) size a single
9699
* value node contributes, including the indentation/newline overhead that
@@ -99,13 +102,18 @@ const NON_STRING_NODE_BYTES = 16
99102
function estimateNodeBytes(value: unknown, depth: number): number {
100103
const indentOverhead = depth * 2 + 4
101104
if (typeof value === 'string') return indentOverhead + serializedStringLength(value)
102-
// A double can serialize to 24 characters (`-1.2345678901234567e-308`), so a
103-
// number is the one non-string value that outgrows the flat allowance — charging
104-
// it the allowance would let a document of them exceed the byte cap by half again.
105-
// Taking the larger of the two never charges less than before.
105+
// Two non-string values outgrow the flat allowance, and charging them the allowance
106+
// would let a document of them exceed the byte cap by half again: a double serializes
107+
// to as many as 24 characters (`-1.2345678901234567e-308`), and a `Date` — which the
108+
// default js-yaml schema produces for a `!!timestamp`, so the file parser sees them —
109+
// serializes to a 26-character quoted ISO string. Taking the larger of the two never
110+
// charges less than the flat allowance did.
106111
if (typeof value === 'number') {
107112
return indentOverhead + Math.max(NON_STRING_NODE_BYTES, String(value).length)
108113
}
114+
if (value instanceof Date) {
115+
return indentOverhead + Math.max(NON_STRING_NODE_BYTES, SERIALIZED_DATE_BYTES)
116+
}
109117
return indentOverhead + NON_STRING_NODE_BYTES
110118
}
111119

Lines changed: 85 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,85 @@
1+
/**
2+
* @vitest-environment node
3+
*/
4+
import { Readable } from 'node:stream'
5+
import { beforeEach, describe, expect, it, vi } from 'vitest'
6+
7+
const { mockCreateReadStream, mockReadFile, mockStat } = vi.hoisted(() => ({
8+
mockCreateReadStream: vi.fn(),
9+
mockReadFile: vi.fn(),
10+
mockStat: vi.fn(),
11+
}))
12+
13+
vi.mock('fs', () => ({ createReadStream: mockCreateReadStream }))
14+
vi.mock('fs/promises', () => ({ readFile: mockReadFile, stat: mockStat }))
15+
16+
vi.mock('@/lib/uploads/config', () => ({
17+
USE_S3_STORAGE: false,
18+
USE_BLOB_STORAGE: false,
19+
USE_GCS_STORAGE: false,
20+
getStorageConfig: () => ({ bucket: 'b', region: 'r' }),
21+
}))
22+
23+
vi.mock('@/lib/uploads/core/setup.server', () => ({ UPLOAD_DIR_SERVER: '/uploads' }))
24+
25+
vi.mock('@/lib/uploads/server/metadata', () => ({ insertFileMetadata: vi.fn() }))
26+
27+
import { isPayloadSizeLimitError } from '@/lib/core/utils/stream-limits'
28+
import { downloadFile } from '@/lib/uploads/core/storage-service'
29+
30+
/** A stream that delivers `bytes`, whatever a prior `stat` would have claimed. */
31+
function streamOf(bytes: number) {
32+
const stream = Readable.from([Buffer.alloc(bytes)]) as Readable & { destroy: () => void }
33+
vi.spyOn(stream, 'destroy')
34+
return stream
35+
}
36+
37+
describe('downloadFile on local storage', () => {
38+
beforeEach(() => {
39+
vi.clearAllMocks()
40+
mockReadFile.mockResolvedValue(Buffer.alloc(10))
41+
})
42+
43+
it('reads without a ceiling when the caller asks for none', async () => {
44+
const buffer = await downloadFile({ key: 'workspace/ws/file.bin', context: 'workspace' })
45+
46+
expect(buffer.length).toBe(10)
47+
expect(mockReadFile).toHaveBeenCalled()
48+
expect(mockCreateReadStream).not.toHaveBeenCalled()
49+
})
50+
51+
it('enforces the ceiling on the bytes as they arrive, not on a prior stat', async () => {
52+
// The file grew (or was replaced) after any size a caller could have measured:
53+
// the stream delivers more than the ceiling allows, and a stat-then-read
54+
// implementation would have admitted it.
55+
mockCreateReadStream.mockReturnValue(streamOf(500))
56+
57+
await expect(
58+
downloadFile({ key: 'workspace/ws/file.bin', context: 'workspace', maxBytes: 100 })
59+
).rejects.toSatisfy(isPayloadSizeLimitError)
60+
61+
expect(mockStat).not.toHaveBeenCalled()
62+
expect(mockReadFile).not.toHaveBeenCalled()
63+
})
64+
65+
it('returns the bytes when they fit the ceiling', async () => {
66+
mockCreateReadStream.mockReturnValue(streamOf(50))
67+
68+
const buffer = await downloadFile({
69+
key: 'workspace/ws/file.bin',
70+
context: 'workspace',
71+
maxBytes: 100,
72+
})
73+
74+
expect(buffer.length).toBe(50)
75+
})
76+
77+
it('destroys the stream once the read settles', async () => {
78+
const stream = streamOf(50)
79+
mockCreateReadStream.mockReturnValue(stream)
80+
81+
await downloadFile({ key: 'workspace/ws/file.bin', context: 'workspace', maxBytes: 100 })
82+
83+
expect(stream.destroy).toHaveBeenCalled()
84+
})
85+
})

‎apps/sim/lib/uploads/core/storage-service.ts‎

Lines changed: 18 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
import type { Readable } from 'node:stream'
22
import { createLogger } from '@sim/logger'
33
import { getErrorMessage } from '@sim/utils/errors'
4-
import { assertKnownSizeWithinLimit } from '@/lib/core/utils/stream-limits'
4+
import { readNodeStreamToBufferWithLimit } from '@/lib/core/utils/stream-limits'
55
import {
66
getStorageConfig,
77
USE_BLOB_STORAGE,
@@ -507,16 +507,30 @@ export async function downloadFile(options: DownloadFileOptions): Promise<Buffer
507507
}
508508
}
509509

510-
const { readFile, stat } = await import('fs/promises')
510+
const { readFile } = await import('fs/promises')
511511
const { join } = await import('path')
512512
const { UPLOAD_DIR_SERVER } = await import('./setup.server')
513513

514514
const safeKey = sanitizeFileKey(key)
515515
const filePath = join(UPLOAD_DIR_SERVER, safeKey)
516516

517517
if (maxBytes !== undefined) {
518-
const fileStats = await stat(filePath)
519-
assertKnownSizeWithinLimit(fileStats.size, maxBytes, 'storage download')
518+
// Bounded as the bytes arrive, the way every cloud provider branch above reads.
519+
// Checking `stat` and then calling `readFile` describes the file only as of the
520+
// stat, so a caller that asked for a ceiling would still get whatever the file
521+
// became in between — and on a self-hosted deployment this is the same anonymous
522+
// share path the cloud branches serve.
523+
const { createReadStream } = await import('fs')
524+
const stream = createReadStream(filePath, signal ? { signal } : undefined)
525+
try {
526+
return await readNodeStreamToBufferWithLimit(stream, {
527+
maxBytes,
528+
label: 'storage download',
529+
signal,
530+
})
531+
} finally {
532+
stream.destroy()
533+
}
520534
}
521535

522536
return readFile(filePath, signal ? { signal } : undefined)

‎apps/sim/lib/uploads/server/image-derivative.ts‎

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ import { createLogger } from '@sim/logger'
33
import { getErrorMessage } from '@sim/utils/errors'
44
import { downloadFile, uploadFile } from '@/lib/uploads/core/storage-service'
55
import { isHevcHeifContainer, transcodeHeicToJpeg } from '@/lib/uploads/server/heic'
6+
import { MAX_RENDERED_DOCUMENT_BYTES } from '@/lib/uploads/utils/file-utils'
67

78
const logger = createLogger('ImageDerivative')
89

@@ -16,9 +17,18 @@ function derivativeKey(storageKey: string): string {
1617
return `image-derivative/${hash}.jpg`
1718
}
1819

20+
/**
21+
* A cache miss here is recoverable, so unlike the compiled-doc store this swallows a
22+
* size breach too: falling through re-transcodes the original, and the transcode is
23+
* bounded by the source read that produced its input.
24+
*/
1925
async function loadDerivative(storageKey: string): Promise<Buffer | null> {
2026
try {
21-
return await downloadFile({ key: derivativeKey(storageKey), context: 'copilot' })
27+
return await downloadFile({
28+
key: derivativeKey(storageKey),
29+
context: 'copilot',
30+
maxBytes: MAX_RENDERED_DOCUMENT_BYTES,
31+
})
2232
} catch {
2333
return null
2434
}

0 commit comments

Comments
 (0)