fix(files): serve rendered documents and stream workspace archives - #5995
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryMedium Risk Overview Workspace bulk/folder download stops buffering the whole selection in memory: Markdown export gains the same class of bundle limits (asset count, declared total, per-asset Shared Reviewed by Cursor Bugbot for commit dcc19cc. Configure here. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Greptile SummaryServes rendered document artifacts (not generator source) on download/export paths and streams ordinary files into workspace zip archives.
Confidence Score: 4/5Safe to merge from a blocking-defect standpoint; the only remaining prior concern is the intentional full-archive blob buffering in the browser download helper. The split zip byte-budget defect is addressed by reserving streamed declared sizes before document allowances. The client still loads the entire zip into memory via blob() for in-page errors, which remains as an accepted trade-off rather than a regression of the server budget fix. Files Needing Attention: apps/sim/lib/uploads/client/download.ts
|
| Filename | Overview |
|---|---|
| apps/sim/app/api/workspaces/[id]/files/download/route.ts | Shared zip budget reserves streamed declared sizes before rendering docs; lazy streams + pre-commit status for 409/size errors. |
| apps/sim/lib/uploads/client/download.ts | Archive download via requestRaw + full blob materialization (known deliberate memory trade for toasts). |
| apps/sim/app/api/files/export/[id]/route.ts | Embed export now caps asset count and total/per-asset bytes before download. |
| apps/sim/app/api/v1/files/[fileId]/route.ts | Public download serves rendered content type/bytes and maps pending compile to 409. |
Sequence Diagram
sequenceDiagram
participant UI as Files UI
participant API as download route
participant Meta as workspace metadata
participant Render as fetchServableWorkspaceFileBuffer
participant Store as downloadFileStream
participant Zip as ZipArchive
UI->>API: GET fileIds/folderIds
API->>Meta: list files/folders
API->>API: declaredBytes + reservedForStreamed
loop generated docs
API->>Render: "maxBytes = min(remaining, 50MB)"
Render-->>API: rendered buffer or 409/400
end
API->>Zip: append buffers + lazy streams
loop each ordinary file
Zip->>Store: open on first pull
Store-->>Zip: chunks
end
Zip-->>UI: application/zip stream
UI->>UI: response.blob() then save
Reviews (8): Last reviewed commit: "fix(files): stop the export caps from re..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
@cursor review |
|
@cursor review |
|
@cursor review |
|
@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 18bf362. Configure here.
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 chokepoint that swaps one for the other, and the serve route, single-file download and ~50 tool routes all go through it. Archive compression and the public v1 download read raw bytes instead, so a generated document arrived as source text under a .docx name and Word reported it as corrupt. Both now resolve through the servable reader. Adds fetchServableWorkspaceFileBuffer beside the raw reader so the record to UserFile mapping lives in one place, preserving storageContext; the raw reader's doc comment now says it returns generation source, so the next call site has to choose deliberately. v1 also has to send the resolved content type rather than the record's source MIME, and returns a retryable 409 rather than a 500 when an artifact is still compiling. docNotReadyMessage centralizes the 409 copy, and isRenderableDocumentName is shared so the read path and its callers agree on which extensions can expand.
The bulk download route materialized every selected file before writing a byte, so peak memory tracked the size of the selection. Ordinary files are now appended as lazy streams: each opens its storage read only when the archiver reaches it, so one entry is resident at a time rather than the whole archive. Generated documents still resolve to buffers first. They are the only entries whose bytes decide anything, and every status this route returns comes from them — once the first byte is written the status code is committed, so those decisions have to happen before the archive starts. The per-entry allowance and byte budget therefore govern documents only. archiver processes appended entries through a sequential queue; lazystream defers each storage read until that entry's turn, since handing the archiver an open stream per entry would hold more connections than the storage client pools. nodeReadableToWebStream moves out of input-validation.server.ts into a shared util rather than being written twice: Readable.toWeb throws ERR_INVALID_STATE when a consumer cancels while the source is still flowing, which is exactly what a cancelled download does. Trade: a storage read failing mid-archive truncates the response rather than returning 500, since the status is already sent. Documents cannot hit this. The table export route already behaves this way.
Bulk and folder downloads navigated to the API route, so any rejection replaced the Files page with the raw JSON error body. Single-file download already fetched and saved the blob, so the two paths had diverged. 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 archive paths now fetch the zip and show the server's message as a toast — the route writes that copy for a person, so it is worth surfacing rather than discarding. Single-file download shows its error too instead of only logging it.
…l uploads Four review passes over the branch converged on the same points. Routing every office extension through the buffered path held an entire selection of genuinely uploaded documents in memory — for a check the resolver settles on the first few magic bytes. Only files stored under a generator-source content type need resolving; a real .docx serves what is stored and now streams like anything else, which is what the change was for. With resolution sequential, the AbortController cancelled nothing (its signal never reached the storage read), the success-path over-limit branch was unreachable, and the cancellation guard in the catch could not fire. A plain loop that returns at the point of failure replaces the outcome record, the sentinel, the fan-out helper at limit 1, and four post-hoc scans. ZIP_MATERIALIZE_CONCURRENCY was dead, and the aggregate over-limit message reported a byte count that was by construction under the limit. lazystream and its types are gone: Readable.from over an async generator defers the open the same way and propagates source errors natively, so the PassThrough relay went with them. The client uses requestRaw with the existing binary contract instead of a hand-built query string and a re-typed error extractor, and both archive call sites share one callback. The render headroom moves beside isRenderableDocumentName, the servable reader stops minting a request id per file, and the v1 download returns a view rather than a second full copy.
Readable.from defaults to object mode; these chunks are bytes headed for an archive, so the mode is now explicit. Also makes the fire-and-forget bulk download call consistent with its sibling.
A detached anchor's click() works in current browsers, but every other download helper in the app attaches first, and a silent no-op on this path would look exactly like a download that never started. Not worth the ambiguity for two DOM operations. archive.finalize() was fired with void, so a failure after the response had started became an unhandled rejection on top of the stream error event that already fails the response. It is caught and logged now.
Navigating to the route meant a second click just re-navigated. Fetching the archive instead means each click starts another download that holds the whole zip in tab memory, and the Download button gave no sign anything was happening. The button now reflects the in-flight download alongside the existing move and delete states. The ref guard is there as well as the state because two clicks in the same tick would both pass a state check.
… five The archive keyed off a locally enumerated set of source content types that omitted text/x-pdflibjs — the isolated-vm PDF generator. Those files failed the check and streamed their generator source under a .pdf name, which is the exact corruption this change exists to fix. A canonical five-member set already existed in the file viewer; enumerating a fourth copy was the mistake, not the missing string. The set moves to file-utils beside the other file-type predicates, the viewer imports it, and the route asks isGeneratedDocumentSourceType rather than carrying its own list. A parameterized test pins all five, so adding a generator without extending the set fails.
Every existing test mocks the storage stream and reads the result back with the same JS library that wrote it, which cannot catch the failure this route exists to fix: an archive a real zip reader will not open. This drives 5 MB of non-repeating bytes from an fs stream through archiver, the lazy entry generator and the Node-to-web bridge, then checks the output with the operating system's unzip and compares the extracted bytes. Skips rather than fails where unzip is unavailable.
The embed list is produced by scanning the document body, so its length and the bytes behind it are whatever the author put there. Every referenced asset was downloaded at once with no concurrency bound, no per-file cap and no total cap, so one request could materialize an unbounded number of unbounded files. Its sibling bulk-download route already had a count cap and a byte cap. Metadata is now resolved first, which bounds the download from declared sizes before a byte is read and moves the authorization check off the download path. Assets are then fetched with bounded concurrency and a per-file cap; a single unreadable or oversized asset drops out of the bundle rather than failing the export, which is what the previous allSettled did.
…source Mounting a generated document handed the sandbox its generator source under a .docx name, so a python-docx or openpyxl script failed on a file that looked fine and the agent debugged code that was correct. Both branches were wrong, not just the buffered one: with cloud storage the mount presigns record.key, which is the raw source object, so swapping the buffered read alone would have fixed only local storage. Source-backed documents now always take the servable path — they are bounded by the render ceiling, so routing them through the web process instead of presigning is affordable. The text/binary decision also keyed off record.type, which for these files is text/x-docxjs, so a resolved binary would have been decoded as UTF-8. It reads the resolved content type now.
Staging reworked the OTel split, dropped dead deps and declared emcn peers, so the lockfile is regenerated on top of that rather than carrying a stale merge. The archive dependency is the only addition; lazystream stays as archiver's transitive dep now that it is no longer declared directly. Regenerated incrementally rather than from scratch: a clean resolve fails the bunfig age gate because minimumReleaseAgeExcludes lists only the darwin and linux-gnu @next/swc binaries, not the musl and windows variants that a full re-resolve also pulls.
37607d1 to
2ec32cd
Compare
|
Rebased onto staging (picking up the OTel/dep rework in #5994, lockfile regenerated) and folded in three follow-ups that were listed at the bottom of this PR. Markdown export asset bundling was uncapped. Sandbox mounts handed user code generator source. Archive validation against a real zip reader. Every prior test mocked the storage stream and read the result back with the same library that wrote it, which cannot catch "the archive will not open" — the actual customer symptom. There is now a test driving 5 MB of non-repeating bytes from an fs stream through archiver, the lazy generator and the web bridge, checked with the operating system's Worth flagging separately: |
|
@cursor review |
…rce size A source-backed document declares the size of its generator, not of the document, so the per-file and aggregate mount pre-checks were reading a number unrelated to what was about to be mounted — tiny sources cleared the guard and only afterwards was the running total updated with the real length. Those pre-checks now apply only to files that serve what they declare. A document is capped by the read instead, at the smaller of the per-file limit and the budget left, and a size rejection is reported in the same terms as the other mount limits. Same invariant the archive route already had to learn: declared size is not a bound once bytes can be rendered.
|
@cursor review |
The markdown export route had no test file at all, and the sandbox mount's source-backed branch was exercised by nothing — both were changed in this PR and one of them shipped a defect a reviewer caught within minutes. Export: the embed-count rejection, the declared-bytes rejection landing before any asset leaves storage, the per-asset download cap, an unreadable asset dropping out rather than failing the export, and an unauthorized asset never being read. Mount: a generated document never presigning its raw key even on cloud storage, its rendered bytes mounting as base64 rather than being utf-8 decoded off the source MIME, and the budget rejecting on rendered length.
|
@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 9db0867. Configure here.
…o work The caps I picked would have failed exports that previously succeeded: a screenshot-heavy document can hold more than 100 embeds, and 100 embeds of unoptimised PNGs can exceed 100 MB. Adding a limit to bound memory is right; setting it below real usage turns a memory risk into a broken feature. The byte ceiling now matches the bulk-download route, so the two export surfaces reject at the same size rather than at two invented ones. The count is only a guard on the metadata lookups that run before the byte check, so it moves well above any hand-authored document instead of sitting where a legitimate one could reach it.
|
@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 dcc19cc. Configure here.
1 participant
Replaces #5986 and #5993, which are closed. Those went through eleven review rounds; this squashes the churn into reviewable commits and carries the streaming rewrite, which removes the memory objection at its root rather than tuning around it.
Summary
resolveServableDocBytesis the chokepoint that swaps one for the other, and its own doc comment calls a raw read "the corruption every non-serve consumer used to ship".docxname and Word reported it as corrupt.docxserves exactly what is stored and streams like anything else; routing every office extension through the buffered path would hold a whole selection of real documents in memory for a check the resolver settles on the first few magic bytesDesign notes for review
app/api/table/[tableId]/exportalready behaves this way.archiver(35.6M weekly downloads, actively maintained) processes appended entries through a sequential queue.Readable.fromover an async generator gives the deferred open natively, so no lazy-stream dependency was needed.nodeReadableToWebStreammoves out ofinput-validation.server.tsinto a shared util rather than being written twice. Its existing comment is the reason it exists:Readable.toWebthrowsERR_INVALID_STATEwhen a consumer cancels while the source is still flowing — exactly what a cancelled download does.Type of Change
Testing
Eleven route tests, including one asserting that no storage read happens until the archive is pulled — the property the streaming rests on — plus rendered-vs-source bytes in the zip, nested folder paths across both entry kinds, uploads streaming rather than resolving, the pending-artifact 409, per-entry vs shared-budget size attribution, and the declared-size pre-check rejecting before any read. 1335 tests pass across
lib/uploads,lib/core, and the touched routes. Lint,check:utils, typecheck andcheck:api-validationclean.Not live-tested in a browser — the client change (navigation → fetch + toast) is worth a manual pass on a folder download.
Follow-ups found while working, deliberately not included
app/api/files/export/[id]bundles every embedded image with no file-count cap, no per-file byte cap and no total cap, where the embed count is whatever the user typed into the document. Same domain, same shape, zero caps where this route has twolib/copilot/tools/handlers/function-execute.tsmounts generator source into the sandbox under a.docxname, so a user'spython-docxscript fails on a file that looks fine. The cloud-storage branch presigns the raw key, so it needs more than a one-line swaplistWorkspaceFilesreads every row in the workspace to keep the selected handful, and itshydrateFolderPathsflag is a no-opChecklist