fix(files): archive rendered document bytes instead of generator source - #5986
fix(files): archive rendered document bytes instead of generator source#5986waleedlatif1 wants to merge 12 commits into
Conversation
Generated docs (docx/pptx/pdf/xlsx) store their generation source as the primary file; the rendered binary lives in a separate content-addressed artifact store. resolveServableDocBytes is the shared chokepoint that swaps one for the other, and the serve route, single-file download and ~50 tool routes all go through it. The bulk zip route and file_compress read raw bytes instead, so every generated document landed in the archive as source text under a .docx name and Word reported it as corrupt. Both now resolve through the servable helper, with bounded concurrency, a size cap re-checked against the rendered bytes, and a 409 naming any document whose artifact is still compiling.
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryMedium Risk Overview Bulk workspace zip ( GET Shared helpers: The files UI uses Adds extensive route tests for zip behavior (rendered bytes, 409, size limits, abort/cancellation edge cases). Reviewed by Cursor Bugbot for commit 95942d2. Configure here. |
Greptile SummaryThe PR corrects generated-document downloads across archive and public-file surfaces.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| apps/sim/app/api/workspaces/[id]/files/download/route.ts | Resolves servable bytes under bounded materialization, handles pending and size outcomes, and archives entries in selection order. |
| apps/sim/app/api/workspaces/[id]/files/download/route.test.ts | Covers rendered artifacts, size-policy branches, cancellation races, pending documents, and hard storage failures. |
| apps/sim/lib/uploads/contexts/workspace/workspace-file-manager.ts | Adds a shared workspace-file reader that resolves generated documents to their rendered artifacts. |
| apps/sim/app/api/tools/file/manage/route.ts | Updates file compression to archive servable bytes and return an explicit payload-too-large response. |
| apps/sim/app/api/v1/files/[fileId]/route.ts | Serves rendered document bytes with the resolved MIME type and a retryable response while compilation is pending. |
| apps/sim/lib/uploads/client/download.ts | Centralizes browser blob saving and exposes server-provided archive and file download errors. |
| apps/sim/app/workspace/[workspaceId]/files/files.tsx | Uses fetched archive downloads so failures appear as in-place error notifications. |
Sequence Diagram
sequenceDiagram
participant UI as Workspace UI
participant Route as Archive Download Route
participant Resolver as Servable File Resolver
participant Store as Artifact Storage
participant Zip as ZIP Builder
UI->>Route: Request selected files/folders
Route->>Route: Authorize and validate declared limits
loop Selected entries with bounded concurrency
Route->>Resolver: Resolve servable bytes with byte allowance
Resolver->>Store: Read rendered artifact or ordinary file
Store-->>Resolver: Binary bytes
Resolver-->>Route: Buffer and content type
end
alt Document still compiling
Route-->>UI: 409 with pending document names
else Size limit exceeded
Route-->>UI: 400 with actionable size message
else Storage failure
Route-->>UI: 500
else All entries resolved
Route->>Zip: Add resolved buffers
Zip-->>Route: Archive buffer
Route-->>UI: workspace-files.zip
end
Reviews (11): Last reviewed commit: "fix(files): read renderable documents on..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
…byte budget Adds fetchServableWorkspaceFileBuffer beside fetchWorkspaceFileBuffer so the record -> UserFile mapping (including storageContext, which the hand-rolled adapter dropped) lives in one place, and the raw reader's doc comment says it returns generation source. The v1 file download served source bytes for the same reason and now uses it too. Download route: the rendered-byte budget is tracked as files land and each read is capped at what is left, so the request is bounded by the limit rather than by 100 files each allowed the full limit; an oversized artifact returns the 400 size response instead of a 500; a hard failure outranks a pending artifact so clients are not told to retry something that cannot succeed; and queued reads stop once the request is doomed. docNotReadyResponse now takes the pending file names so the 409 copy and body shape stay in the shared helper. Compress drops its inner catch, which the handler's own catch already covered.
|
@cursor review |
…iling Concurrent reads all sample the same running total before any of them land, so capping each at the remaining budget still let every in-flight read be granted the whole 250 MiB. Ordinary uploads serve exactly their declared size, and the pre-download check already bounds their sum to the request budget, so only documents that render from a generation source can exceed what they declared. Those now carry a per-entry ceiling, which is what actually bounds peak memory. The renderable extension set moves into file-utils so the read path and this cap agree on which files can expand.
|
Addressed the remaining memory concern from the last round. You were right that capping each read at The fix separates the two classes of file:
Peak is now bounded by landed bytes (≤ 250 MiB, enforced by the abort) plus at most The renderable-extension set moved into |
|
@cursor review |
…ve surfaces Cancelling the shared controller made other in-flight reads reject, and those rejections were being recorded as hard failures — so an oversized selection surfaced as a generic 500 instead of the descriptive 400. A read that finds the controller already aborted now reports nothing, checked before the failing worker aborts so the real cause is still recorded, and the size check runs first. The compress handler can also hit the per-file cap now that rendered artifacts can exceed their stored source, so the handler maps PayloadSizeLimitError to 413 alongside the existing combined-size response. The v1 download resolved rendered bytes but still labelled them with the record's source MIME, handing clients .docx bytes typed as text/x-docxjs.
|
@cursor review |
The office-extension cap rejected ordinary uploads: the per-file limit is enforced on the stored object before the magic-byte passthrough decides whether it is a generated source, so a legitimate 80 MiB deck failed the zip at the 50 MiB render ceiling. Uploads and source-backed docs share those extensions and cannot be told apart before the bytes are read, so the allowance is now the larger of the declared size and the render headroom — real uploads download at their true size while a small generator source still cannot render unbounded. Zip materialization also drops to its own concurrency limit, since each in-flight entry is a whole file held in memory.
|
@cursor review |
The cancellation guard sat ahead of the size check, so a worker that hit its byte allowance after someone else's hard failure had already aborted the shared controller was treated as cancellation noise. overLimit never got set and an actionable 400 came back as an opaque 500. A size rejection describes the file that raised it regardless of who aborted, so it is recorded first; every other error from an already-aborted read stays suppressed as a consequence of the cancellation.
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit d9b9a30. Configure here.
|
@cursor review |
… docs Resolving rendered bytes there meant a still-compiling artifact threw into the generic catch, so a caller that would previously have received (corrupt) source now got a 500 with nothing to indicate the request is worth retrying. Also drops the fileNames parameter added to docNotReadyResponse last round: all 44 call sites pass a single argument, and the one route that names pending files builds its 409 from docNotReadyMessage because its error envelope differs.
|
Self-audit for regressions on the three changed download surfaces turned up two of my own, both fixed:
Behaviour changes worth a reviewer's attention, all intentional:
|
Every PayloadSizeLimitError was reported as the entry exceeding its per-document render allowance, quoting the 50 MiB headroom. That is wrong when the shared budget was the smaller cap, and wrong for extensions that carry no headroom at all — a video rejected because the budget ran out was told it exceeded a per-document render limit. The entry is now blamed only when its own allowance was the smaller of the two caps, and the message quotes the allowance that applied rather than the constant. The mapper also returns its outcome instead of mutating closure state, which is what the aggregate and per-entry branches now read.
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 339b648. Configure here.
Bulk and folder downloads navigated to the API route, so any rejection replaced the Files page with raw JSON — the user lost their view and got an unstyled error body. That was survivable when the only failures were "too many files" and "too large"; resolving rendered documents adds a 409 for a still-compiling artifact, which is reachable in normal use. Both now fetch the zip and save the blob, matching what single-file download already did, and show the server's message as a toast. The server writes that copy for the user — which document is compiling, which entry is too large — so it is worth showing rather than discarding. Single-file download surfaces its error too instead of only logging it.
|
Final audit pass turned up a UX gap that the server-side work made materially worse, now fixed. Bulk and folder downloads used That was survivable when the only failures were "too many files" and "too large". Resolving rendered documents adds a 409 for a still-compiling artifact, which is reachable in normal use, so the odds of a user landing on a JSON page went up because of this PR. Both archive paths now go through Full audit also confirmed: non-document extensions pass through byte-identically, office-extension uploads are capped at their declared size rather than the render headroom, and the v1 API returns a retryable 409 rather than a 500 for a compiling document. |
|
@cursor review |
A renderable document whose allowance exactly equalled the remaining budget was reported as the selection being too large. Its own ceiling was still what made it unshippable, and downloading it on its own is the way through, so the aggregate copy sent the user down a path that could not work. Attribution is now keyed off whether the entry has a cap of its own at all — only renderable documents do — and whether that cap was the binding one, ties included.
Two standing items for a human reviewerCorrection to an earlier version of this comment: I claimed this was "a strict improvement" on the memory axis. That was wrong, and I want it on the record rather than quietly edited away. The pre-change route did an unbounded Resolving rendered bytes breaks that invariant, because a rendered artifact can exceed what its source declared. Worst-case peak on this branch is roughly 250 MiB of landed bytes plus up to 1. Peak memory. It cannot be driven to a hard 250 MiB while reads run concurrently: a source-backed document's rendered size is unknowable until read, so a worker cannot reserve accurately up front. Reserving the headroom pessimistically would bound it, but would falsely reject a selection of many small generated documents. The honest options are (a) accept a bounded overshoot and tune the two constants, (b) serialize renderable documents so the overshoot is one entry rather than five, or (c) stream the archive to the response, which removes the ceiling entirely and is the real fix. 2. A generated document over 50 MiB is rejected even when the selection fits. Intentional, and the mechanism bounding item 1 — raising the ceiling relaxes the rejection and widens the memory overshoot. The rejection names the document, quotes the cap that applied, and points at single-file download, which has no such limit. Net: this PR fixes archive corruption, the v1 content type and retryable 409, the compress 413, both cancellation/size races, size attribution including the tie case, and the client replacing the Files page with raw JSON. It trades some worst-case memory headroom and some large-selection latency to do it. That trade is a human call, not one I should keep asserting away. |
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 003e75c. Configure here.
…ory regression Files that serve exactly their declared size cannot push residency past the pre-download check, however many run at once. Only renderable documents can exceed what they declared, so they were the sole source of overshoot — and with five in flight the worst case was five headrooms above the budget, which is worse than the hard cap the raw-byte reads used to give. Those documents now read one at a time while everything else stays parallel, so the overshoot is a single entry. The per-file read is a named function the two passes share; ordering is preserved by writing results back by index.
Memory regression removed; streaming written up as the follow-upFollowing the correction above, I went looking for precedent rather than inventing a design. It all exists:
What this commit does. The overshoot only ever came from renderable documents, because everything else serves exactly the size it declared and is therefore already bounded by the pre-download check no matter how many run at once. Those documents now read one at a time while the rest stay parallel, so the worst case is one headroom above the budget instead of five. That puts peak residency back at parity with the raw-byte behaviour it replaces, give or take a single entry. What it deliberately does not do. Two things surfaced while designing the streaming version that make it a poor fit for this PR:
Both are tractable and worth doing: streaming removes the 250 MiB ceiling entirely rather than managing it. But it is a rewrite of this route with its own error semantics, and this PR is a corruption fix that is currently clean. Better as its own change than bolted onto this one. |
|
@cursor review |
1 participant
Follow-up to #5985 (same two archive surfaces, different bug).
Summary
resolveServableDocBytesis the shared chokepoint that swaps one for the other — its own doc comment calls a raw read "the corruption every non-serve consumer used to ship"/api/files/export, and ~50 tool routes already go through it. The bulk zip route andfile_compressdid not, so every generated document landed in the archive as JavaScript source text named*.docx— Word reports it as corruptdownloadServableFileFromStoragemapWithConcurrency) instead of an unboundedPromise.allType of Change
Testing
New route tests cover four paths: rendered bytes land in the zip (not the stored source), 409 names only the still-compiling documents, the rendered-size cap rejects, and a storage failure surfaces as 500 rather than an archive with an empty entry. 358 tests pass across
lib/uploads,tools/file, andapp/api/tools. Typecheck, lint, andcheck:api-validationclean.Not covered here:
/api/v1/files/[fileId](public API) has the same raw-bytes bug. Left out to keep this focused — changing public API response bytes deserves its own call.Checklist