perf(files): stream workspace archives instead of buffering them - #5993
perf(files): stream workspace archives instead of buffering them#5993waleedlatif1 wants to merge 1 commit into
Conversation
The zip 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 are needed to 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. Uses archiver, whose entry queue processes appends sequentially, with lazystream deferring each storage read; handing the archiver one open stream per entry would hold more connections than the storage client pools. The Node-to-web bridge that input-validation.server.ts already hardened moves to a shared util: Readable.toWeb throws ERR_INVALID_STATE when a consumer cancels mid-transfer, which is exactly what a cancelled download does. Trade: a storage read that fails mid-archive now truncates the response instead of returning 500, since the status is already sent. Matches how the table export route behaves.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
PR SummaryMedium Risk Overview Renderable documents (docx, etc.) are still resolved to buffers first, sequentially, so compile/pending/size errors can return 400/409/500 before any bytes are sent. Ordinary files go through
Trade-off: a storage failure after streaming starts can truncate the zip rather than return 500, since the status is already committed—documents are not affected because they are resolved before the archive starts. Reviewed by Cursor Bugbot for commit 80a4e0c. Configure here. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 80a4e0c. Configure here.
| ...skipped, | ||
| overLimit: true, | ||
| overLimitEntry: | ||
| renderable && allowance <= remaining ? { name: file.name, allowance } : null, |
There was a problem hiding this comment.
Byte budget ignores ordinary files
Medium Severity
renderedBytes starts at zero and only accumulates document buffers, so document render expansion is never charged against ordinary files that still stream into the same zip. A selection under the declared-size pre-check can still ship well past the MAX_ZIP_DOWNLOAD_BYTES download limit once sources render up to their headroom.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 80a4e0c. Configure here.
| }) | ||
| .catch((error) => relay.destroy(toError(error))) | ||
| return relay | ||
| }) |
There was a problem hiding this comment.
Storage streams leak on cancel
Medium Severity
lazyWorkspaceFileStream pipes the downloadFileStream result into a PassThrough without destroying that source when the relay or archive is cancelled. downloadFileStream requires callers to fully consume or destroy() the stream, so aborted downloads can leave storage connections open.
Reviewed by Cursor Bugbot for commit 80a4e0c. Configure here.
Greptile SummaryStreams workspace multi-file zip downloads with archiver/lazystream instead of buffering the full selection in JSZip.
Confidence Score: 3/5Not safe to merge until the zip byte budget still covers ordinary streamed files after documents expand past their declared sizes. Mixed selections can exceed the 250 MiB download limit because only document buffers count against remaining maxBytes while ordinary entries stream fully after a declared-size pre-check that undercounts expanded docs; pipe cleanup and dead concurrency constant are secondary. Files Needing Attention: apps/sim/app/api/workspaces/[id]/files/download/route.ts
|
| Filename | Overview |
|---|---|
| apps/sim/app/api/workspaces/[id]/files/download/route.ts | Switches zip assembly to streaming archiver; ordinary files lose remaining-budget maxBytes enforcement after document expansion. |
| apps/sim/lib/core/utils/node-stream.ts | Shared Node Readable → WHATWG stream bridge moved out of input-validation for reuse. |
| apps/sim/lib/core/security/input-validation.server.ts | Drops local nodeReadableToWebStream in favor of the shared util import. |
| apps/sim/app/api/workspaces/[id]/files/download/route.test.ts | Covers lazy storage open, nested paths, document budget attribution, and declared-size rejection; does not assert mixed doc+file overshoot. |
| apps/sim/package.json | Adds archiver 8, lazystream, and matching @types. |
Sequence Diagram
sequenceDiagram
participant Client
participant Route as files/download GET
participant Docs as fetchServableWorkspaceFileBuffer
participant Arch as ZipArchive
participant Stor as downloadFileStream
Client->>Route: "GET fileIds=..."
Route->>Route: auth + declaredBytes pre-check
loop each renderable document
Route->>Docs: "maxBytes = min(remaining, allowance)"
Docs-->>Route: buffer or error status
end
alt overLimit / pending / hard fail
Route-->>Client: 400 / 409 / 500
else ok
Route->>Arch: append buffers + lazy streams
Route->>Arch: finalize()
Route-->>Client: 200 application/zip (streaming)
loop each ordinary entry when pulled
Arch->>Stor: open read
Stor-->>Arch: bytes
Arch-->>Client: zip chunks
end
end
Comments Outside Diff (1)
-
apps/sim/app/api/workspaces/[id]/files/download/route.ts, line 41-46 (link)Unused materialize concurrency constant
ZIP_MATERIALIZE_CONCURRENCYis no longer referenced after ordinary files stopped materializing; leaving it implies a concurrency path that no longer exists and confuses later edits.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Reviews (1): Last reviewed commit: "perf(files): stream workspace archives i..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| function lazyWorkspaceFileStream(file: WorkspaceFileRecord): Readable { | ||
| return new lazystream.Readable(() => { | ||
| const relay = new PassThrough() | ||
| downloadFileStream({ key: file.key, context: file.storageContext ?? 'workspace' }) | ||
| .then((source) => { | ||
| source.on('error', (error) => relay.destroy(toError(error))) | ||
| source.pipe(relay) | ||
| }) | ||
| .catch((error) => relay.destroy(toError(error))) | ||
| return relay | ||
| }) | ||
| } |
There was a problem hiding this comment.
When a selection mixes renderable documents with ordinary files, the 250 MiB pre-check only sums declared file.size, documents can expand up to the full budget via fetchServableWorkspaceFileBuffer, and ordinary entries still stream their full declared sizes through lazyWorkspaceFileStream with no remaining-budget cap, so the client receives a zip well over MAX_ZIP_DOWNLOAD_BYTES.
| function lazyWorkspaceFileStream(file: WorkspaceFileRecord): Readable { | ||
| return new lazystream.Readable(() => { | ||
| const relay = new PassThrough() | ||
| downloadFileStream({ key: file.key, context: file.storageContext ?? 'workspace' }) | ||
| .then((source) => { | ||
| source.on('error', (error) => relay.destroy(toError(error))) | ||
| source.pipe(relay) | ||
| }) | ||
| .catch((error) => relay.destroy(toError(error))) | ||
| return relay | ||
| }) | ||
| } |
There was a problem hiding this comment.
lazyWorkspaceFileStream pipes downloadFileStream into a PassThrough without destroying the storage source when the relay is torn down, so a client abort mid-entry leaves the storage read open and holds provider sockets until timeout.
|
Superseded — folded into a single PR together with #5986, since the streaming rewrite is what makes the memory story coherent and splitting them left the first PR with a known regression. |
1 participant
Stacked on #5986 — review that first. Base retargets to
stagingonce it merges.Summary
Why these dependencies
archiver(35.6M weekly downloads, 8.0.0 released this May) — its entry queue processes appends sequentially. Rawzip-stream, which archiver wraps, has no queue managementlazystream(31.3M weekly) — defers each storage read until the archiver pulls it. Already a direct dependency of archiver, so declaring it just makes an import we use explicit. Handing the archiver one open stream per entry would hold more connections than the storage client pools, most sitting idle until their turnReuse
nodeReadableToWebStreammoves out ofinput-validation.server.tsintolib/core/utils/node-stream.tsand is shared. Its existing comment is the reason it exists:Readable.toWebthrows an unhandledERR_INVALID_STATEwhen a consumer cancels while the source is still flowing — exactly what a cancelled download does. It also copies chunks off pooled buffers, honours backpressure, and settles a destroy that never emitserror.Trade worth reviewing
A storage read that fails mid-archive now truncates the response instead of returning 500, because the status is already sent. Documents cannot hit this (they resolve up front); it applies to ordinary files.
app/api/table/[tableId]/exportalready behaves this way.Type of Change
Testing
Ten route tests, including one asserting that no storage read happens until the archive is pulled — the property the whole change rests on — plus nested paths across both entry kinds, the pending-artifact 409, per-entry vs shared-budget size attribution, and the declared-size pre-check rejecting before any read. 1112 tests pass across
lib/uploads,lib/core/security,lib/core/utils, and the touched routes. Lint,check:utils, typecheck andcheck:api-validationclean.Not live-tested in a browser yet.
Checklist