Skip to content

refactor(streaming): drop buffered fallback in S3/ECR upload paths#785

Merged
vieiralucas merged 1 commit intomainfrom
worktree-batch11-streaming-cleanup
Apr 26, 2026
Merged

refactor(streaming): drop buffered fallback in S3/ECR upload paths#785
vieiralucas merged 1 commit intomainfrom
worktree-batch11-streaming-cleanup

Conversation

@vieiralucas
Copy link
Copy Markdown
Member

@vieiralucas vieiralucas commented Apr 26, 2026

Summary

Pre-1.0 cleanup. PR #782 left two ingestion modes side-by-side: streaming spool plus a buffered `BodySource::Bytes` branch from `req.body`. Now that the streaming dispatch + test helper always wire `body_stream`, the buffered branches are dead. Remove them.

  • S3 PutObject / UploadPart: stream is required. No more buffered fallback. SSE-KMS reads spool back once for AES-GCM (unavoidable); non-KMS hands `BodySource::File` straight to the store.
  • ECR OCI `PATCH` / `PUT`: stream-only via `append_stream`. `append_bytes_sync` still in use by JSON `UploadLayerPart` (different wire format with inline base64 chunks).
  • Tests: `make_request` wires `body_stream` from the same bytes it puts in `body`, so direct service calls hit the streaming path; buffered handlers ignore the stream.

Net: -144 / +93 lines.

Test plan

  • `cargo build --workspace`
  • `cargo clippy --workspace --all-targets -- -D warnings`
  • `cargo fmt`
  • `cargo test --workspace --lib` (all green)
  • `cargo test -p fakecloud-e2e --test s3` (64 passed)
  • `cargo test -p fakecloud-e2e --test s3_streaming_body --test s3_persistence --test s3_sse_kms` (green)
  • `cargo test -p fakecloud-e2e --test ecr --test ecr_oci --test ecr_images --test ecr_kms_encryption --test ecr_persistence` (green)

Summary by cubic

Require streaming request bodies for S3 and ECR uploads by removing the buffered fallback. This unifies ingestion to a single streaming path, lowers memory pressure, and simplifies the code.

  • Refactors

    • fakecloud-s3 PutObject: streaming-only. Spools to a tempfile; SSE-KMS reads the spool back once; non-KMS passes BodySource::File to the store.
    • fakecloud-s3 UploadPart: streaming-only. Spools and computes MD5/size in constant memory.
    • fakecloud-ecr OCI blob PATCH/PUT: streaming-only via append_stream. JSON UploadLayerPart stays buffered (base64 inline).
    • Tests: S3 helper now wires body_stream alongside body so PutObject/UploadPart use the streaming path. Buffered endpoints keep reading body.
  • Migration

    • Clients must provide a streaming body for S3 PutObject and UploadPart, and ECR blob PATCH/PUT.
    • Missing streams return errors: S3 → MalformedRequestBody; ECR → BLOB_UPLOAD_INVALID.
    • For direct service calls, set request.body_stream (can be created from the same bytes as request.body).

Written for commit 7116cad. Summary will update on new commits.

Pre-1.0, no need to keep two ingestion modes for the same op. The
streaming dispatch always wires body_stream for these routes; the
test helper does the same for direct service calls. Buffered
branches were dead in production.

S3
- put_object: stream is now required, errors with MalformedRequestBody
  when missing. Spool-to-tempfile path is the only path. SSE-KMS
  reads the spool back once, non-KMS hands BodySource::File straight
  to the store.
- upload_part: same. Single Bytes-allocation branch removed.
- compute_md5 import dropped from objects.rs (now unused — md5 is
  computed by spool_request_stream).

ECR OCI
- blob_upload_patch / blob_upload_finish: stream-only via
  append_stream. Buffered append_bytes_sync branch deleted from
  these paths. JSON UploadLayerPart still uses append_bytes_sync
  because that protocol carries a base64-decoded chunk inline.

Tests
- make_request now wires body_stream from the same bytes it puts in
  body, so direct unit-test calls to put_object / upload_part take
  the streaming path. Buffered handlers (put_object_tagging, …)
  ignore the stream and read req.body as before.

Net: 144 lines removed, 93 added.
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 26, 2026

Codecov Report

❌ Patch coverage is 56.09756% with 18 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
crates/fakecloud-ecr/src/oci.rs 0.00% 8 Missing ⚠️
crates/fakecloud-s3/src/service/objects.rs 57.89% 8 Missing ⚠️
crates/fakecloud-s3/src/service/multipart.rs 81.81% 2 Missing ⚠️

📢 Thoughts on this report? Let us know!

Copy link
Copy Markdown

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

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

No issues found across 4 files

@vieiralucas vieiralucas merged commit 67f7eb0 into main Apr 26, 2026
47 of 48 checks passed
@vieiralucas vieiralucas deleted the worktree-batch11-streaming-cleanup branch April 26, 2026 16:34
vieiralucas added a commit that referenced this pull request Apr 26, 2026
PR #785 (drop buffered fallback) made S3 PutObject require the
streaming dispatch path. The streaming_route guard rejected single-
segment paths to avoid mis-routing CreateBucket as PutObject, but
that also rejected virtual-hosted PutObject (`/key` with bucket in
Host header), causing a 400 MalformedRequestBody.

Now the guard also accepts single-segment PUT when the Host header
carries a virtual-hosted bucket. Restores
e2e::host_routing::s3_virtual_hosted_put_get_delete and the dotted
/aws-dash-separated variants alongside it.
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