-
Notifications
You must be signed in to change notification settings - Fork 3.7k
fix(files): archive rendered document bytes instead of generator source #5986
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Closed
Closed
Changes from all commits
Commits
Show all changes
12 commits
Select commit
Hold shift + click to select a range
68ffafb
fix(files): archive rendered document bytes instead of generator source
waleedlatif1 9da5816
improvement(files): centralize servable-byte reads and bound the zip …
waleedlatif1 81a7e92
improvement(files): bound rendered documents with a per-entry byte ce…
waleedlatif1 84d174b
fix(files): keep size and cancellation outcomes distinct across archi…
waleedlatif1 257abfe
fix(files): size the per-entry allowance off the declared size
waleedlatif1 d9b9a30
fix(files): record a size rejection even when another read aborted first
waleedlatif1 396ef43
fix(files): name the entry that exceeds the per-document render cap
waleedlatif1 8a02ca6
fix(files): return a retryable 409 from the v1 download for compiling…
waleedlatif1 339b648
fix(files): attribute a size rejection to the cap that actually bound it
waleedlatif1 78f322f
improvement(files): surface archive download errors in place
waleedlatif1 003e75c
fix(files): blame the entry when its cap ties the remaining budget
waleedlatif1 95942d2
fix(files): read renderable documents one at a time to remove the mem…
waleedlatif1 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
282 changes: 282 additions & 0 deletions
282
apps/sim/app/api/workspaces/[id]/files/download/route.test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,282 @@ | ||
| /** | ||
| * @vitest-environment node | ||
| */ | ||
| import { createMockRequest } from '@sim/testing' | ||
| import { sleep } from '@sim/utils/helpers' | ||
| import JSZip from 'jszip' | ||
| import { beforeEach, describe, expect, it, vi } from 'vitest' | ||
|
|
||
| const { | ||
| mockGetSession, | ||
| mockVerifyWorkspaceMembership, | ||
| mockListWorkspaceFiles, | ||
| mockListWorkspaceFileFolders, | ||
| mockFetchServableWorkspaceFileBuffer, | ||
| } = vi.hoisted(() => ({ | ||
| mockGetSession: vi.fn(), | ||
| mockVerifyWorkspaceMembership: vi.fn(), | ||
| mockListWorkspaceFiles: vi.fn(), | ||
| mockListWorkspaceFileFolders: vi.fn(), | ||
| mockFetchServableWorkspaceFileBuffer: vi.fn(), | ||
| })) | ||
|
|
||
| vi.mock('@/lib/auth', () => ({ | ||
| auth: { api: { getSession: vi.fn() } }, | ||
| getSession: mockGetSession, | ||
| })) | ||
|
|
||
| vi.mock('@/app/api/workflows/utils', () => ({ | ||
| verifyWorkspaceMembership: mockVerifyWorkspaceMembership, | ||
| })) | ||
|
|
||
| vi.mock('@/lib/uploads/contexts/workspace', () => ({ | ||
| listWorkspaceFiles: mockListWorkspaceFiles, | ||
| listWorkspaceFileFolders: mockListWorkspaceFileFolders, | ||
| buildWorkspaceFileFolderPathMap: (folders: Array<{ id: string; name: string }>) => | ||
| new Map(folders.map((folder) => [folder.id, folder.name])), | ||
| fetchServableWorkspaceFileBuffer: mockFetchServableWorkspaceFileBuffer, | ||
| })) | ||
|
|
||
| vi.mock('@sim/audit', () => ({ | ||
| recordAudit: vi.fn(), | ||
| AuditAction: { FILE_DOWNLOADED: 'file.downloaded' }, | ||
| AuditResourceType: { FILE: 'file' }, | ||
| })) | ||
|
|
||
| vi.mock('@/lib/posthog/server', () => ({ captureServerEvent: vi.fn() })) | ||
|
|
||
| import { DocCompileUserError } from '@/lib/copilot/tools/server/files/doc-compile' | ||
| import { PayloadSizeLimitError } from '@/lib/core/utils/stream-limits' | ||
| import { GET } from '@/app/api/workspaces/[id]/files/download/route' | ||
|
|
||
| const WORKSPACE_ID = 'ws-1' | ||
| const context = { params: Promise.resolve({ id: WORKSPACE_ID }) } | ||
|
|
||
| function workspaceFile(id: string, name: string, folderId: string | null) { | ||
| return { | ||
| id, | ||
| name, | ||
| key: `workspace/${WORKSPACE_ID}/${id}`, | ||
| path: `/serve/${id}`, | ||
| size: 100, | ||
| type: 'application/octet-stream', | ||
| folderId, | ||
| } | ||
| } | ||
|
|
||
| function requestFor(query: string) { | ||
| return createMockRequest( | ||
| 'GET', | ||
| undefined, | ||
| {}, | ||
| `http://localhost:3000/api/workspaces/${WORKSPACE_ID}/files/download?${query}` | ||
| ) | ||
| } | ||
|
|
||
| describe('workspace files download route', () => { | ||
| beforeEach(() => { | ||
| vi.clearAllMocks() | ||
| mockGetSession.mockResolvedValue({ user: { id: 'user-1' } }) | ||
| mockVerifyWorkspaceMembership.mockResolvedValue({ role: 'member' }) | ||
| mockListWorkspaceFileFolders.mockResolvedValue([ | ||
| { id: 'folder-1', name: 'Reports', parentId: null }, | ||
| ]) | ||
| }) | ||
|
|
||
| it('zips the rendered bytes for a generated doc, not its stored source', async () => { | ||
| mockListWorkspaceFiles.mockResolvedValue([workspaceFile('f1', 'overview.docx', 'folder-1')]) | ||
| // A real .docx is a ZIP; the stored source would be plain JS text. | ||
| const rendered = Buffer.from('PKrendered-docx') | ||
| mockFetchServableWorkspaceFileBuffer.mockResolvedValue({ | ||
| buffer: rendered, | ||
| contentType: 'application/vnd.openxmlformats-officedocument.wordprocessingml.document', | ||
| }) | ||
|
|
||
| const response = await GET(requestFor('fileIds=f1'), context) | ||
|
|
||
| expect(response.status).toBe(200) | ||
| expect(mockFetchServableWorkspaceFileBuffer).toHaveBeenCalledTimes(1) | ||
|
|
||
| const zip = await JSZip.loadAsync(Buffer.from(await response.arrayBuffer())) | ||
| const entry = zip.file('Reports/overview.docx') | ||
| expect(entry).not.toBeNull() | ||
| expect(Buffer.from(await entry!.async('uint8array'))).toEqual(rendered) | ||
| }) | ||
|
|
||
| it('returns 409 naming the documents whose artifacts are still compiling', async () => { | ||
| mockListWorkspaceFiles.mockResolvedValue([ | ||
| workspaceFile('f1', 'ready.md', 'folder-1'), | ||
| workspaceFile('f2', 'pending.docx', 'folder-1'), | ||
| ]) | ||
| mockFetchServableWorkspaceFileBuffer.mockImplementation(async (file: { name: string }) => { | ||
| if (file.name === 'pending.docx') | ||
| throw new DocCompileUserError('Document is still being generated') | ||
| return { buffer: Buffer.from('ok'), contentType: 'text/markdown' } | ||
| }) | ||
|
|
||
| const response = await GET(requestFor('fileIds=f1&fileIds=f2'), context) | ||
|
|
||
| expect(response.status).toBe(409) | ||
| const body = await response.json() | ||
| expect(body.error).toContain('pending.docx') | ||
| expect(body.error).not.toContain('ready.md') | ||
| }) | ||
|
|
||
| it('rejects with 400, not 500, when a rendered document blows the byte budget', async () => { | ||
| mockListWorkspaceFiles.mockResolvedValue([workspaceFile('f1', 'huge.docx', 'folder-1')]) | ||
| mockFetchServableWorkspaceFileBuffer.mockRejectedValue( | ||
| new PayloadSizeLimitError('servable file download exceeds limit') | ||
| ) | ||
|
|
||
| const response = await GET(requestFor('fileIds=f1'), context) | ||
|
|
||
| expect(response.status).toBe(400) | ||
| // Names the offending entry rather than blaming the whole selection. | ||
| const body = await response.json() | ||
| expect(body.error).toContain('huge.docx') | ||
| expect(body.error).not.toContain('Selected files total') | ||
| }) | ||
|
|
||
| it('blames the entry when its render ceiling exactly equals the remaining budget', async () => { | ||
| // Declared at the full budget, so allowance === remaining and the caps tie. | ||
| const doc = { ...workspaceFile('f1', 'report.docx', 'folder-1'), size: 250 * 1024 * 1024 } | ||
| mockListWorkspaceFiles.mockResolvedValue([doc]) | ||
| mockFetchServableWorkspaceFileBuffer.mockRejectedValue( | ||
| new PayloadSizeLimitError({ label: 'servable file download', maxBytes: 1 }) | ||
| ) | ||
|
|
||
| const response = await GET(requestFor('fileIds=f1'), context) | ||
|
|
||
| expect(response.status).toBe(400) | ||
| // Downloading it on its own is still the way through, so name it. | ||
| expect((await response.json()).error).toContain('report.docx') | ||
| }) | ||
|
|
||
| it('blames the shared budget, not the entry, when the entry had no smaller cap', async () => { | ||
| // .mp4 has no render headroom, so its cap is whatever is left of the budget. | ||
| mockListWorkspaceFiles.mockResolvedValue([workspaceFile('f1', 'clip.mp4', 'folder-1')]) | ||
| mockFetchServableWorkspaceFileBuffer.mockRejectedValue( | ||
| new PayloadSizeLimitError({ label: 'servable file download', maxBytes: 1 }) | ||
| ) | ||
|
|
||
| const response = await GET(requestFor('fileIds=f1'), context) | ||
|
|
||
| expect(response.status).toBe(400) | ||
| const body = await response.json() | ||
| expect(body.error).toContain('Selected files total') | ||
| expect(body.error).not.toContain('clip.mp4') | ||
| }) | ||
|
|
||
| it('lets an uploaded office file larger than the render headroom through', async () => { | ||
| const big = { ...workspaceFile('f1', 'deck.pptx', 'folder-1'), size: 80 * 1024 * 1024 } | ||
| mockListWorkspaceFiles.mockResolvedValue([big]) | ||
| mockFetchServableWorkspaceFileBuffer.mockResolvedValue({ | ||
| buffer: Buffer.from('ok'), | ||
| contentType: 'application/vnd.openxmlformats-officedocument.presentationml.presentation', | ||
| }) | ||
|
|
||
| const response = await GET(requestFor('fileIds=f1'), context) | ||
|
|
||
| expect(response.status).toBe(200) | ||
| // Capped at the declared size, not the smaller render headroom. | ||
| expect(mockFetchServableWorkspaceFileBuffer.mock.calls[0][1].maxBytes).toBe(80 * 1024 * 1024) | ||
| }) | ||
|
|
||
| it('caps rendered documents per entry so concurrent reads cannot each claim the budget', async () => { | ||
| mockListWorkspaceFiles.mockResolvedValue([ | ||
| workspaceFile('f1', 'report.docx', 'folder-1'), | ||
| workspaceFile('f2', 'clip.mp4', 'folder-1'), | ||
| ]) | ||
| mockFetchServableWorkspaceFileBuffer.mockResolvedValue({ | ||
| buffer: Buffer.from('ok'), | ||
| contentType: 'application/octet-stream', | ||
| }) | ||
|
|
||
| await GET(requestFor('fileIds=f1&fileIds=f2'), context) | ||
|
|
||
| const maxBytesFor = (name: string) => | ||
| mockFetchServableWorkspaceFileBuffer.mock.calls.find( | ||
| (call: [{ name: string }, { maxBytes: number }]) => call[0].name === name | ||
| )?.[1].maxBytes | ||
|
|
||
| // Only the source-backed document can render larger than it declares. | ||
| expect(maxBytesFor('report.docx')).toBe(50 * 1024 * 1024) | ||
| expect(maxBytesFor('clip.mp4')).toBe(250 * 1024 * 1024) | ||
| }) | ||
|
|
||
| it('reports an oversized selection as 400 even when the abort cancels other reads', async () => { | ||
| const files = Array.from({ length: 40 }, (_, index) => | ||
| workspaceFile(`f${index}`, `doc${index}.docx`, 'folder-1') | ||
| ) | ||
| mockListWorkspaceFiles.mockResolvedValue(files) | ||
| mockFetchServableWorkspaceFileBuffer.mockImplementation(async (file: { name: string }) => { | ||
| if (file.name === 'doc0.docx') | ||
| throw new PayloadSizeLimitError({ label: 'servable file download', maxBytes: 1 }) | ||
| // Everything else fails the way a cancelled read would. | ||
| throw new DOMException('The operation was aborted', 'AbortError') | ||
| }) | ||
|
|
||
| const response = await GET( | ||
| requestFor(files.map((file) => `fileIds=${file.id}`).join('&')), | ||
| context | ||
| ) | ||
|
|
||
| // Cancellation noise must not turn the size rejection into a generic 500. | ||
| expect(response.status).toBe(400) | ||
| }) | ||
|
|
||
| it('keeps the size rejection when a hard failure aborted the read first', async () => { | ||
| mockListWorkspaceFiles.mockResolvedValue([ | ||
| workspaceFile('f1', 'broken.txt', 'folder-1'), | ||
| workspaceFile('f2', 'huge.docx', 'folder-1'), | ||
| ]) | ||
| mockFetchServableWorkspaceFileBuffer.mockImplementation(async (file: { name: string }) => { | ||
| if (file.name === 'broken.txt') throw new Error('storage down') | ||
| // Lands after the hard failure has already aborted the shared controller. | ||
| await sleep(1) | ||
| throw new PayloadSizeLimitError({ label: 'servable file download', maxBytes: 1 }) | ||
| }) | ||
|
|
||
| const response = await GET(requestFor('fileIds=f1&fileIds=f2'), context) | ||
|
|
||
| // "Select fewer files" is actionable; a generic 500 is not. | ||
| expect(response.status).toBe(400) | ||
| }) | ||
|
|
||
| it('stops issuing reads once one hard-fails instead of draining the selection', async () => { | ||
| const files = Array.from({ length: 60 }, (_, index) => | ||
| workspaceFile(`f${index}`, `doc${index}.txt`, 'folder-1') | ||
| ) | ||
| mockListWorkspaceFiles.mockResolvedValue(files) | ||
| mockFetchServableWorkspaceFileBuffer.mockImplementation(async (file: { name: string }) => { | ||
| if (file.name === 'doc0.txt') throw new Error('storage down') | ||
| return { buffer: Buffer.from('ok'), contentType: 'text/plain' } | ||
| }) | ||
|
|
||
| const response = await GET( | ||
| requestFor(files.map((file) => `fileIds=${file.id}`).join('&')), | ||
| context | ||
| ) | ||
|
|
||
| expect(response.status).toBe(500) | ||
| // Reads already in flight finish, but the queued remainder is never started. | ||
| expect(mockFetchServableWorkspaceFileBuffer.mock.calls.length).toBeLessThan(files.length) | ||
| }) | ||
|
|
||
| it('surfaces a storage failure as a 500 even when another document is pending', async () => { | ||
| mockListWorkspaceFiles.mockResolvedValue([ | ||
| workspaceFile('f1', 'pending.docx', 'folder-1'), | ||
| workspaceFile('f2', 'broken.txt', 'folder-1'), | ||
| ]) | ||
| mockFetchServableWorkspaceFileBuffer.mockImplementation(async (file: { name: string }) => { | ||
| if (file.name === 'pending.docx') | ||
| throw new DocCompileUserError('Document is still being generated') | ||
| throw new Error('storage down') | ||
| }) | ||
|
|
||
| const response = await GET(requestFor('fileIds=f1&fileIds=f2'), context) | ||
|
|
||
| // A 409 would tell the client to retry something that can never succeed. | ||
| expect(response.status).toBe(500) | ||
| }) | ||
| }) |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.