fix(remote-cache): stream artifact uploads to storage instead of buffering full body in memory - #797
Conversation
…ering full body in memory The `application/octet-stream` content-type parser used `parseAs: 'buffer'`, which loads the entire upload into the Node.js heap before `put-artifact` re-wraps it with `Readable.from(req.body)` and pipes it to storage. Each concurrent upload therefore holds up to `BODY_LIMIT` bytes in memory, so a burst of large uploads can OOM the server even though every storage driver already streams to the backend via `createWriteStream`. Replace the buffering parser with a raw-stream passthrough and pipe the request body straight into `createCachedArtifact`. `BODY_LIMIT` is still enforced: an advertised `content-length` over the limit is rejected up front, and a byte-counting transform inside the storage pipeline is the backstop for chunked/unknown-length uploads — both return 413. The HTTP contract is unchanged. Closes ducktors#679 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — streams artifact upload bodies directly to storage instead of buffering them fully in memory, eliminating OOM risk under concurrent large uploads (fixes #679).
- Replace the
application/octet-streamcontent-type parser — switches fromparseAs: 'buffer'to a raw-stream passthrough (done(null, payload)), soreq.bodyis the request stream rather than a fully materializedBuffer. - Enforce
BODY_LIMITat the handler and in the storage pipeline — acontent-lengthfast path rejects oversized uploads before streaming begins;createSizeLimitStreamis the backstop for chunked/unknown-length requests, inserted into the storage pipeline. - Plumb
maxBytesthroughcreateCachedArtifact— the function conditionally inserts the byte-countingTransformintopipeline(artifact, ..., writeStream)when a limit is provided. - Wire 413 rejection through the error handler —
entityTooLargeboom errors from the streaming guard are re-thrown as-is (not wrapped inpreconditionFailed), so the client gets a clean 413. - Add real-socket chunked upload test — exercises the streaming guard path for uploads without a
content-lengthheader, confirming 413 rejection via the in-pipeline byte counter.
ℹ️ Partial artifacts may remain in storage when the streaming guard triggers mid-upload
When createSizeLimitStream rejects mid-pipeline, the write stream has already received and may have committed some data to storage (local filesystem, S3 multipart parts, etc.). The pipeline destruction stops further writes but does not clean up what was already written. This is a behavioral change from the previous atomic write-after-buffer model.
Partial artifacts are harmless for the turbo use case — artifacts are content-addressed, and the client retries on errors — but worth being aware of since S3 multipart uploads that are abandoned mid-stream may leave incomplete parts accruing storage costs.
Technical details
# Partial artifacts from mid-stream rejection
## Affected sites
- `src/plugins/remote-cache/storage/index.ts:188-189` — `createSizeLimitStream` inserted between artifact and write stream; all chunks before the limit breach flow through to `writeStream` before the transform errors.
## Required outcome
- No action required for the turbo use case. If partial-artifact cleanup is ever needed, consider a try/catch in `createCachedArtifact` that removes the artifact on error — but doing so reliably across storage backends (canceling an in-progress S3 multipart upload) adds material complexity.
## Open questions for the human
- Should the `content-length` fast-path check also validate `content-length` is non-negative? Currently `Number('-1')` passes `Number.isFinite`; the streaming guard would catch the excess data, but accepting a negative `content-length` without comment is slightly surprising.DeepSeek Pro | 𝕏
matteovivona
left a comment
There was a problem hiding this comment.
when the size-limit transform errors, pipeline() destroys the destination writable. For the S3 provider, that writable wraps an internal PassThrough, but destruction is not propagated to that stream and Upload.abort() is not called because final() is skipped.
please update the S3 adapter as part of this PR so destroying its writable also destroys the internal stream and aborts the upload
| const writeStream = location.createWriteStream(join(team, artifactId)) | ||
| if (maxBytes && maxBytes > 0) { | ||
| return pipeline(artifact, createSizeLimitStream(maxBytes), writeStream) | ||
| } | ||
| return pipeline(artifact, writeStream) |
There was a problem hiding this comment.
createWriteStream() is opened directly on / before the complete request has been validated. With the local fs-blob-store provider, when createSizeLimitStream rejects or the client disconnect, the bytes already written remain on disk. subsequent HEAD requests then report a cache hit, and GET serves the truncated artifact.
Please make local writes atomic. write to a temporary file/key, promote it to the final key only after the pipeline succeeds, and remove the temporary file on failure. Avoid deleting the final key on failure because that could remove an existing valid artifact during a failed overwrite

In this PR:
Uploads are currently buffered fully in memory before being written to storage, so handling large artifacts requires growing both
BODY_LIMITand the server's memory. Under a burst of concurrent large uploads this leads to high heap usage and OOMs. This PR streams the upload straight to the storage backend instead.Root cause
The storage layer already streams —
createCachedArtifactdoespipeline(artifact, location.createWriteStream(...)), and e.g. the S3 driver uses@aws-sdk/lib-storage'sUploadover aPassThrough. The buffering happens earlier, at HTTP ingestion:src/plugins/remote-cache/index.tsregisters theapplication/octet-streamparser with{ parseAs: 'buffer', bodyLimit }, which materializes the entire body into aBuffer(req.body).src/plugins/remote-cache/routes/put-artifact.tsthen doesReadable.from(req.body)— wrapping the already-bufferedBufferback into a stream.So each in-flight upload holds up to
BODY_LIMITbytes on the heap until GC.Change
done(null, payload)), soreq.bodyis the raw request stream and is never materialized in memory.createCachedArtifact(dropsReadable.from).BODY_LIMIT(Fastify only auto-enforces it forparseAsparsers):content-lengthexceeds the limit (the common case — the turbo client sendscontent-length), and200on success,413when too large,412on storage errors.Peak per-upload heap is now bounded by the storage driver's streaming chunk size (for S3, the
lib-storagepart size × queue depth) rather than scaling with the full artifact size and the number of concurrent uploads.Testing
pnpm test(all 113 tests green),pnpm lint,pnpm buildall pass locally.test/body-limit.ts: kept thecontent-length413 case and added a real-socket test that a chunked upload (nocontent-length) exceedingBODY_LIMITis rejected with 413 via the streaming guard.Issues reference:
Checklist:
pnpm lintbefore submission?pnpm build?pnpm test?