Skip to content

Preserve headers in multipart stream uploads - #471

Open
mfroembgen wants to merge 2 commits into
durch:masterfrom
mfroembgen:codex/multipart-stream-headers
Open

Preserve headers in multipart stream uploads#471
mfroembgen wants to merge 2 commits into
durch:masterfrom
mfroembgen:codex/multipart-stream-headers

Conversation

@mfroembgen

@mfroembgen mfroembgen commented Aug 3, 2026

Copy link
Copy Markdown

Problem

put_object_stream_builder accepts per-upload metadata and headers, but drops them when the payload reaches CHUNK_SIZE and switches to multipart upload. S3 requires completed-object headers such as metadata, cache control, storage class, and SSE-S3/KMS configuration on CreateMultipartUpload.

Solution

  • add a backward-compatible initiate_multipart_upload_with_headers API while preserving the existing method signature
  • forward streaming-builder headers to multipart initiation through a request-scoped bucket clone
  • keep multipart parts, completion, bounded concurrency, and error handling unchanged
  • add a hermetic request/signing regression plus real MinIO coverage below, at, and above CHUNK_SIZE

The request-scoped clone preserves the existing credential provider and client configuration without mutating shared bucket headers.

Fixes #472.

This follows the maintainer direction in #446 and is related to #408. The motivating consumer bug is brunojppb/turbo-cache-server#620.

Verification

  • cargo fmt --all -- --check
  • cargo test -p rust-s3 multipart_initiation_with_headers_sends_and_signs_object_configuration
  • sync API compile with sync-native-tls
  • Tokio and async-std MinIO regression at 8,388,607, 8,388,608, and 8,715,039 bytes
  • MinIO HEAD metadata and downloaded object bytes verified at every boundary size

The request test also verifies that metadata, cache control, and SSE-S3 are included in SigV4 SignedHeaders on multipart initiation.

Summary by CodeRabbit

  • New Features

    • Added support for custom headers when initiating multipart uploads.
    • Streaming multipart uploads now preserve custom headers throughout the upload process.
    • Existing multipart upload methods remain available without requiring header configuration.
  • Bug Fixes

    • Improved header transmission and request signing across multipart upload size boundaries.
    • Preserved metadata consistently during multipart uploads.

This change is Reviewable

@coderabbitai

coderabbitai Bot commented Aug 3, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

Multipart initiation now supports optional custom headers in async and sync APIs. Existing methods delegate to header-aware methods. Streamed uploads forward headers during initiation. Tests cover signed headers, metadata, content integrity, cleanup, and multipart boundaries.

Changes

Multipart upload headers

Layer / File(s) Summary
Header-aware multipart initiation
s3/src/bucket.rs
Async and sync initiation methods accept optional custom headers and include them in signed initiation requests. Integration tests verify metadata, cache-control, and encryption headers.
Streamed upload forwarding and validation
s3/src/bucket.rs
Streamed uploads forward custom headers during initiation. Tokio and async-std tests cover sizes below, at, and above the multipart chunk boundary.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant Bucket
  participant S3
  Client->>Bucket: Start streamed multipart upload with custom headers
  Bucket->>Bucket: Merge headers into initiation request
  Bucket->>S3: Send signed multipart initiation request
  S3-->>Bucket: Return upload identifier
  Bucket-->>Client: Continue multipart upload
Loading
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The changes address issue #472 by forwarding and signing streaming-upload headers during multipart initiation while preserving metadata.
Out of Scope Changes check ✅ Passed The changes are limited to header-aware multipart initiation, streaming header propagation, and related tests.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly describes preserving custom headers during multipart stream uploads, which is the primary change.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@ghost

ghost commented Aug 3, 2026

Copy link
Copy Markdown

Brownian Motion (Brass)

Recommendation: Refactor

Summary: Fixes lost multipart headers; good direction, but watch API bloat and async test certainty
Risk: Medium · Confidence: 78%

Highlights

  • Targets a real S3 multipart semantics gap
  • Adds regression tests for request signing and MinIO boundaries
  • Preserves existing API via wrapper method

Unknowns

  • I can't see the implementation of RequestImpl::new / header merge order
  • It's unclear whether the new tests are stable across all enabled backends in CI
  • The linked issue is closed, but I can't confirm whether this PR matches the maintainer's preferred public API shape

Next actions

  • Keep: forwarding headers only on multipart initiation; that is the actual fix
  • Drop: any further API proliferation unless there is a concrete consumer need
  • Add: one focused compatibility test proving single-part uploads still behave exactly as before
  • Add: explicit docs or changelog note that these headers affect multipart initiation, not parts
  • Add: verify whether extra_headers cloning preserves/overrides header precedence exactly as intended

Reflection questions

  • Why is a new public method the right abstraction instead of extending the existing one with an optional parameter everywhere?
  • Do we actually need a request-scoped bucket clone, or can we pass initiation headers more directly without widening the surface area?
  • What breaks if a caller expects the bucket's shared extra_headers to remain the single source of truth?

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (1)
s3/src/bucket.rs (1)

1936-1948: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add a documentation example to the sync initiate_multipart_upload_with_headers.

The sync initiate_multipart_upload_with_headers at Line 1943 is a public API. Its doc comment (Lines 1936-1941) has prose only and no # Example code block, unlike the async twin at Lines 1879-1904. Add a short example, consistent with how other paired async/sync methods in this file present a single example that also demonstrates the #[cfg(feature = "sync")] variant (for example, see Lines 366-372).

As per coding guidelines, "All public APIs should have documentation examples" for **/*.rs.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@s3/src/bucket.rs` around lines 1936 - 1948, Add a concise Rustdoc # Example
block to the public sync method initiate_multipart_upload_with_headers,
following the async twin’s example and the paired async/sync documentation
pattern elsewhere in the file. Include the #[cfg(feature = "sync")] variant in
the same example so both API forms are demonstrated, without changing the method
implementation.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@s3/src/bucket.rs`:
- Around line 1912-1915: Update both async and sync
initiate_multipart_upload_with_headers flows to remove existing
request_bucket.extra_headers entries for every key in custom_headers before
extending them, ensuring request-level headers replace bucket-level values
rather than creating duplicates.

---

Nitpick comments:
In `@s3/src/bucket.rs`:
- Around line 1936-1948: Add a concise Rustdoc # Example block to the public
sync method initiate_multipart_upload_with_headers, following the async twin’s
example and the paired async/sync documentation pattern elsewhere in the file.
Include the #[cfg(feature = "sync")] variant in the same example so both API
forms are demonstrated, without changing the method implementation.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: c63caf42-340c-45c0-82ad-16993087ec2e

📥 Commits

Reviewing files that changed from the base of the PR and between b584ce7 and 5d3234d.

📒 Files selected for processing (1)
  • s3/src/bucket.rs

Comment thread s3/src/bucket.rs
@ghost

ghost commented Aug 3, 2026

Copy link
Copy Markdown

Brownian Motion (Brass)

Recommendation: Refactor

Summary: Fixes header loss at multipart initiation; approach is sound but needs tighter validation
Risk: Medium · Confidence: 78%

Highlights

  • Addresses a real S3 multipart semantics bug
  • Keeps existing API intact while adding a scoped opt-in path
  • Adds hermetic and MinIO regression coverage

Unknowns

  • Whether any other multipart initiation paths bypass this new helper
  • Whether all documented headers are supported consistently across AWS, MinIO, and other S3-compatible backends
  • Whether the added public API is worth long-term maintenance versus extending the existing builder

Next actions

  • Keep: the request-scoped bucket clone to avoid mutating shared headers
  • Keep: multipart initiation regression tests around CHUNK_SIZE
  • Drop: duplicated sync/async documentation and example churn unless strictly needed
  • Add: a focused test proving header precedence/merge rules for duplicate keys
  • Add: confirmation that all multipart-object headers are actually accepted by the supported backends, not just signed

Reflection questions

  • What core assumption underpins this PR's approach: that header preservation belongs at initiation, not later?
  • Why is a new public initiate_multipart_upload_with_headers API necessary instead of plumbing the existing builder state internally?
  • Could the same fix be achieved with less API surface and less duplicated doc/test code?

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.

Streaming multipart uploads drop custom headers at CHUNK_SIZE

1 participant