Skip to content

fix(remote-cache): stream artifact uploads to storage instead of buffering full body in memory - #797

Open
attehuhtakangas wants to merge 1 commit into
ducktors:mainfrom
attehuhtakangas:fix/stream-artifact-uploads
Open

fix(remote-cache): stream artifact uploads to storage instead of buffering full body in memory#797
attehuhtakangas wants to merge 1 commit into
ducktors:mainfrom
attehuhtakangas:fix/stream-artifact-uploads

Conversation

@attehuhtakangas

Copy link
Copy Markdown

In this PR:

  • Bug fix (non-breaking change which fixes an issue)

Uploads are currently buffered fully in memory before being written to storage, so handling large artifacts requires growing both BODY_LIMIT and 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 — createCachedArtifact does pipeline(artifact, location.createWriteStream(...)), and e.g. the S3 driver uses @aws-sdk/lib-storage's Upload over a PassThrough. The buffering happens earlier, at HTTP ingestion:

  1. src/plugins/remote-cache/index.ts registers the application/octet-stream parser with { parseAs: 'buffer', bodyLimit }, which materializes the entire body into a Buffer (req.body).
  2. src/plugins/remote-cache/routes/put-artifact.ts then does Readable.from(req.body) — wrapping the already-buffered Buffer back into a stream.

So each in-flight upload holds up to BODY_LIMIT bytes on the heap until GC.

Change

  • Replace the buffering parser with a raw-stream passthrough (done(null, payload)), so req.body is the raw request stream and is never materialized in memory.
  • Pipe the request stream directly into createCachedArtifact (drops Readable.from).
  • Keep enforcing BODY_LIMIT (Fastify only auto-enforces it for parseAs parsers):
    • reject up front with 413 when an advertised content-length exceeds the limit (the common case — the turbo client sends content-length), and
    • a byte-counting transform inside the storage pipeline is the backstop for chunked/unknown-length uploads, also returning 413.
  • The HTTP contract is unchanged: 200 on success, 413 when too large, 412 on storage errors.

Peak per-upload heap is now bounded by the storage driver's streaming chunk size (for S3, the lib-storage part 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 build all pass locally.
  • Existing per-provider PUT→GET round-trip suites (local, S3, MinIO, GCS, Azure) continue to pass, confirming streaming works across all drivers.
  • test/body-limit.ts: kept the content-length 413 case and added a real-socket test that a chunked upload (no content-length) exceeding BODY_LIMIT is rejected with 413 via the streaming guard.

Issues reference:

Checklist:

  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?
  • Have you linted your code locally with pnpm lint before submission?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your core changes, as applicable?
  • Have you successfully built locally with pnpm build?
  • Have you successfully tested locally with pnpm test?
  • Have you committed using Conventional Commits?

…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>

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ 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-stream content-type parser — switches from parseAs: 'buffer' to a raw-stream passthrough (done(null, payload)), so req.body is the request stream rather than a fully materialized Buffer.
  • Enforce BODY_LIMIT at the handler and in the storage pipeline — a content-length fast path rejects oversized uploads before streaming begins; createSizeLimitStream is the backstop for chunked/unknown-length requests, inserted into the storage pipeline.
  • Plumb maxBytes through createCachedArtifact — the function conditionally inserts the byte-counting Transform into pipeline(artifact, ..., writeStream) when a limit is provided.
  • Wire 413 rejection through the error handlerentityTooLarge boom errors from the streaming guard are re-thrown as-is (not wrapped in preconditionFailed), so the client gets a clean 413.
  • Add real-socket chunked upload test — exercises the streaming guard path for uploads without a content-length header, 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.

Pullfrog  | View workflow run | Using DeepSeek Pro𝕏

@matteovivona matteovivona left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines +187 to +191
const writeStream = location.createWriteStream(join(team, artifactId))
if (maxBytes && maxBytes > 0) {
return pipeline(artifact, createSizeLimitStream(maxBytes), writeStream)
}
return pipeline(artifact, writeStream)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants