feat(sandbox): stream file downloads end to end - #409
Conversation
🦋 Changeset detectedLatest commit: 78d534b The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![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 default effort and found 5 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want higher recall? High effort reviews run extra passes and find more bugs. A team admin can switch effort levels in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 78d534b. Configure here.
| const w = wake; | ||
| wake = undefined; | ||
| w?.(); | ||
| }); |
There was a problem hiding this comment.
Stdout errors can crash process
High Severity
startSupervisorStdoutStream attaches a data listener to child.stdout but never an error listener, unlike runSupervisorSession which calls ignoreStreamError on both stdout and stderr. An stdout pipe error during abort, timeout, or kill can become an unhandled error event and take down the Node process.
Reviewed by Cursor Bugbot for commit 78d534b. Configure here.
| supervised.abort(); | ||
| }, | ||
| { once: true }, | ||
| ); |
There was a problem hiding this comment.
Pre-aborted signal ignored
Medium Severity
Local and Daytona only addEventListener('abort', …) after the transfer is already open. If signal is already aborted (client gone during auth, provider resolve, or stat), the listener never runs, so /bin/cat or the Daytona stream keeps reading until timeout or completion. TFY avoids this via AbortSignal.any.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 78d534b. Configure here.
| const w = wake; | ||
| wake = undefined; | ||
| w?.(); | ||
| }); |
There was a problem hiding this comment.
No stdout backpressure
Medium Severity
startSupervisorStdoutStream puts stdout in flowing mode and pushes every chunk into an unbounded pending array with no pause/resume. A slow or stalled HTTP consumer lets /bin/cat buffer the whole file in memory, so local downloads are not bounded by chunk size as the PR claims.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 78d534b. Configure here.
| // Early consumer return lands here too; a completed process is an ESRCH no-op. | ||
| killExecTree(child); | ||
| SandboxManager.cleanupAfterCommand(); | ||
| } |
There was a problem hiding this comment.
Cleanup skipped if unread
Medium Severity
SandboxManager.cleanupAfterCommand() runs only in the chunks() generator finally, not on the child close handler. If the returned stream is never pulled or cancelled, the process may be killed by the timeout while SRT wrap cleanup never runs, unlike runSupervisorSession which always cleans up on close.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 78d534b. Configure here.
| throw new SandboxFileNotFoundError(params.path); | ||
| } | ||
| throw error; | ||
| } |
There was a problem hiding this comment.
Stream can exceed Content-Length
Medium Severity
Local and Daytona stream whatever the backend returns afterstat. If the file grows but stays under fileMaxBytesForDownload, the body can exceed the declared size used as Content-Length. TFY avoids this by reading exact windows up to info.size; the route still advertises that size for all providers.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 78d534b. Configure here.
|
Heads up for reviewers: the CI / Generate SDK workflow runs are sitting in |
2 participants
Summary
Sandbox file downloads currently buffer the complete file inside every provider and then copy that buffer again into the HTTP response. This PR changes
SandboxProvider.downloadFileto return{ size, stream }and streams bytes end to end, so peak memory is bounded by chunk/backpressure rather than file size, the client starts receiving bytes before the whole file is read, and disconnects stop provider reads.Design notes for the questions raised in the issue:
{ size: number; stream: ReadableStream<Uint8Array> }. The size comes from the existing pre-download stat (which also keeps the is-dir and max-size checks ahead of any bytes), so the route still sends an exactContent-Length; errors raised before the first byte stay mappable to HTTP statuses, while mid-stream failures surface as an errored stream detectable against Content-Length.req.raw.signalinto the provider call, consumer cancellation propagates through the stream'scancel(), and both stop provider reads (process-group kill locally, stream destroy on Daytona, no further execs on TFY).tail -c +N | head -c M | base64 -w0). Memory stays bounded per 2 MiB chunk with no server-side changes; a temporary fully-buffered fallback was deliberately avoided.Closes #283
Changes
trueforge-core:SandboxProvider.downloadFilenow returnsSandboxFileDownload(size+ single-consumption web stream) and accepts an optional abortsignal.trueforge-core: new sharedboundedFileStreamhelper wraps incremental chunks in one canonical implementation that enforces the download cap while streaming (guards against a file growing after stat) and returns generator cleanup on cancel/error.startSupervisorStdoutStream, binary-safe — the old path utf8-decoded buffered output, which forced base64). Spawn/wrap failures still throw before any byte moves so the route can answer with a proper status;/bin/catruns under the same SRT wrap policy as exec/upload.sandbox.fs.downloadFileStream(...)(already exposed by@daytona/sdk) adapted into the contract stream, keeping stat-first checks and the 404 mapping.SandboxFileNotFoundError, and caller-abort support combined with the exec timeout viaAbortSignal.any.Content-Type, exactContent-Lengthfrom the stat size,Content-Disposition,Cache-Control) and unchanged error mapping; removed the now-dead buffering helper..changeset/stream-sandbox-file-downloads.md(published packages changed). No generated OpenAPI/SDK edits — wire shape is unchanged.How was this tested?
boundedFileStream.test.ts: ordered forwarding, cap enforcement (offending chunk withheld), source-error propagation, cleanup on consumer cancel.daytonaDownload.test.ts: streamed round-trip + stat size, dir/oversize rejected before opening the stream, missing-file mapping, streaming-cap re-enforcement when a file grows after stat.tfyDownload.test.ts: multi-window reassembly across the 2 MiB boundary (command shape asserted), empty file, pre-byte rejections, short-read failure, first-window failure mapping.sandboxFileDownload.test.ts): progressive delivery through the Hono response (first chunk observed while the producer is parked), exact headers,signalpassed down, and SandboxError → status mapping.pnpm --filter @truefoundry/trueforge-core typecheck/test,pnpm --filter @truefoundry/trueforge typecheck/test, targeted jest suites, eslint and prettier on all touched files pass. (Windows dev box: repo-widepnpm build/format:checkare blocked by Unix-only scripts/CRLF checkout and two local-sandbox suites fail on POSIX-mode assertions — all verified identical on a cleanmainstash, unrelated to this change.)Checklist
pnpm build,pnpm test,pnpm typecheck,pnpm lint:ci, andpnpm format:checkpass locallypackages/trueforge-sdk,.github/fern/openapi/openapi.json,docs/openapi.json) — fork PRs omit SDK regen; maintainers regenerate after merge.env.exampleupdated if configuration or behavior changedNote
Medium Risk
Changes the sandbox download contract and HTTP body path (streaming, abort, size-cap re-check) across all providers. Not auth, but a user-facing data transfer path where mid-stream failures become truncated bodies.
Overview
Sandbox file downloads no longer buffer the whole file.
SandboxProvider.downloadFilenow returns{ size, stream }plus an optional abortsignal, so peak memory tracks chunk size,Content-Lengthstill comes from the pre-stat, and client disconnects stop provider reads.Shared
boundedFileStreamwraps incremental chunks, re-enforces the download cap while streaming (withholds past-cap bytes), and runs generator cleanup on cancel. Daytona usesdownloadFileStream; TFY pulls 2 MiB windows via sequential execs instead of one base64 dump; local SRT cats raw stdout through a newstartSupervisorStdoutStream.The turns download route pipes the stream as the body (same headers) and maps pre-byte
SandboxErrors to JSON statuses as before. Mid-stream failures surface as a truncated body vsContent-Length.Reviewed by Cursor Bugbot for commit 78d534b. Bugbot is set up for automated code reviews on this repo. Configure here.