Skip to content

Stream PUT bodies directly to S3 instead of buffering in memory#11

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

Stream PUT bodies directly to S3 instead of buffering in memory#11
joh-klein wants to merge 1 commit into
nxcite:masterfrom
joh-klein:feat/streaming-put

Conversation

@joh-klein

Copy link
Copy Markdown

Problem

store_artifact currently collects the entire request body into memory (to_bytes(body, usize::MAX)), and S3Storage::store copies it into a second Vec before a single PutObject. Peak memory is roughly 2× artifact size per concurrent upload, while the README advertises direct streaming with <4MB RAM. GET already streams; this PR brings PUT in line.

Changes

  • StorageProvider::store now takes a byte stream (impl Stream<Item = Result<Bytes, io::Error>>) plus content_length — object stores need the size up front.
  • The handler pipes body.into_data_stream() straight through when the client sends Content-Length (the normal Nx client case). Requests without one (chunked transfer encoding) fall back to the previous buffering behavior, since the size must be known.
  • S3Storage wraps the stream in a small http_body::Body adapter reporting an exact size_hint. Without it the SDK rejects streaming uploads (UnsizedRequestBody from the checksum interceptor); with it the SDK sets Content-Length and streams the default integrity checksum itself.
  • Custom endpoints (S3_ENDPOINT_URL) set RequestChecksumCalculation::WhenRequired: the SDK streams its default CRC32 checksum as one aws-chunked chunk covering the whole body, and MinIO rejects chunks >16MiB (chunk too big: choose chunk size <= 16MiB). Plain AWS S3 accepts it, so the default checksum behavior is kept there.
  • New Cargo.toml entries (bytes, http-body, http-body-util, sync_wrapper, aws-smithy-types) were all already in Cargo.lock transitively — no new crates in the tree.

Trade-off to be aware of: a streamed body is non-replayable, so the SDK can't internally retry a failed PutObject; the Nx client's own retry covers that. The buffered fallback path retains SDK retries.

Testing

e2e against MinIO (minio/minio latest, bucket + token auth):

check result
PUT 512MB artifact (Content-Length set) 202, server RSS stayed ~20MB for the whole upload (buffering held ~1GB+ before)
GET round-trip sha256 identical
PUT existing hash 409
GET missing hash 404
PUT with Transfer-Encoding: chunked (no length) 202, round-trip sha256 identical
no/wrong token 401

cargo build and cargo fmt clean. I don't have a scratch AWS account wired up here, so the real-S3 path (default checksums) wasn't e2e-tested — it's the SDK's standard sized-streaming-body path (same as ByteStream::from_path), but happy to adjust if you'd rather keep it simpler.

🤖 Generated with Claude Code

Previously store_artifact collected the entire request body into memory
(and copied it a second time in S3Storage::store) before a single
PutObject, so peak memory was ~2x artifact size per concurrent upload.

Now the request body is piped straight through to S3:

- StorageProvider::store takes a byte stream plus the content length
  (object stores need the size up front) instead of a ReaderStream
- the handler passes the body's data stream through when the client
  sends Content-Length, and falls back to buffering only for chunked
  transfer encoding
- S3Storage wraps the stream in an http_body with an exact size hint,
  which the SDK needs to set Content-Length and stream the default
  integrity checksum (an unsized body fails with UnsizedRequestBody)
- custom endpoints skip request checksums: the SDK streams the checksum
  as a single aws-chunked chunk, which MinIO rejects for bodies >16MiB
  ("chunk too big"); plain AWS S3 keeps the default checksum behavior

All added dependencies were already in the tree transitively.

Verified against MinIO: a 512MB PUT holds server RSS at ~20MB (was
>1GB), round-trip sha256 matches, and 409/404/401 and the chunked
fallback behave as before.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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