Skip to content

feat: stream PUT bodies to S3 in 8 MiB parts instead of buffering them - #35

Open
joh-klein wants to merge 1 commit into
nxcite:masterfrom
meddevo:feat/multipart-put
Open

joh-klein wants to merge 1 commit into
nxcite:masterfrom
meddevo:feat/multipart-put

Conversation

@joh-klein

Copy link
Copy Markdown
Contributor

Replaces #11, which streamed each upload as one sized PutObject. That design cost the SDK its retries, still buffered uploads without a Content-Length, and had to switch off request checksums on custom endpoints. This one keeps the StorageProvider::store signature and is what we have run in production since July.

Problem

store_artifact read the whole PUT body into memory, and S3Storage::store copied it into a second buffer before a single PutObject. Peak memory is about twice the artifact size per concurrent upload, so a few large task outputs in parallel can take the server down mid-run, and Nx then fails those tasks.

What changes

  • The body goes to S3 as it arrives. Up to 8 MiB it is one PutObject. Anything larger is a multipart upload of 8 MiB parts (S3's minimum is 5 MiB), so an upload holds at most one part in memory.
  • A failed multipart upload is aborted, so its parts don't stay billed in the bucket.
  • Parts are in-memory buffers, so the SDK can still retry them, unlike a streamed single PutObject.
  • If storage fails part-way, the rest of the body is drained before the answer, as fix: drain the remaining refusals, and bound the drain #22 does for the other refusals.
  • An upload cut short still gets 400. The stream error now surfaces inside store, so it gets its own StorageError::ClientAbort; without it e2e/aborted-upload.test.ts would get 500.
  • The README says s3:AbortMultipartUpload is needed next to s3:PutObject, and gives the lifecycle rule that removes uploads the server could not abort (it was killed mid-upload, or the abort failed).

What a user sees

Nx sees the same statuses as before: 200 stored, 409 present, 400 for an upload cut short, 500 for a storage failure. Operators see bounded memory, and need s3:AbortMultipartUpload for the cleanup.

Peak RSS of the server across one PUT, debug build, SeaweedFS:

upload master this PR
256 MiB 529 MiB 52 MiB
1 GiB not run (would need ~2 GiB) 52 MiB

Tests

Two cases in e2e/round-trip.test.ts: an artifact of exactly one part, and one spanning three parts, both returned byte for byte. Nothing in the suite sent more than one part to S3 before. Both also pass on master, because buffering works for any size: they guard the multipart path rather than show the fix. The memory bound has no test; the numbers above are measured by hand.

cargo fmt --check, cargo clippy --all-targets -- -D warnings, format, lint, typecheck, test:offline (24/24) and test:e2e (21/21) are green.


🤖 Generated with Claude Code

Nx uploads each task's output as one PUT. The server read the whole
body into memory, copied it into a second buffer and only then sent a
single PutObject, so memory grew with artifact size times concurrent
uploads, and a few large outputs in parallel could take the server
down mid-run.

The body now goes to S3 as it arrives. Up to 8 MiB it is one
PutObject; anything larger is a multipart upload of 8 MiB parts,
aborted if a part or the client fails. Parts are in-memory buffers, so
the SDK can still retry them. StorageProvider::store keeps its
signature. If storage fails part-way, the rest of the body is drained
before the answer, as for the other refusals (nxcite#22).

Nx sees the same statuses as before: 200 stored, 409 present, 400 for
an upload cut short, 500 for a storage failure. Operators see memory
bounded at about one part per upload, and need s3:AbortMultipartUpload
for the cleanup; the README recommends a lifecycle rule for uploads
the server could not abort.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

This branch has not been deployed

No deployments
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.

1 participant