Fix ResponseDataStream small-buffer reads

[durch]

Linked issue: Fixes #437

Problem

ResponseDataStream implemented Tokio AsyncRead and async-std Read by copying only the bytes that fit in the caller-provided buffer from each stream chunk. If a stream chunk was larger than the read buffer, the unread tail of that chunk was dropped, causing data loss for small-buffer consumers.

Solution

When a chunk is larger than the caller buffer, preserve the unread tail by re-inserting it at the front of the boxed byte stream before continuing with the original stream. This keeps the existing ResponseDataStream::bytes() API and public struct fields intact while fixing both Tokio and async-std read implementations.

Focused verification

• cargo test -p rust-s3 --no-default-features --features tokio-native-tls request::request_trait::tests::test_async_read -- --nocapture — passed: 4 passed, 0 failed.
• cargo test -p rust-s3 --no-default-features --features async-std-native-tls request::request_trait::async_std_tests::test_async_read -- --nocapture — passed: 4 passed, 0 failed.

Risk notes

Low-to-moderate risk area because this touches streaming read semantics. The change is intentionally scoped to ResponseDataStream read adapters and does not alter request construction, signing, credentials, provider selection, TLS, or ResponseDataStream::bytes().

Provider/runtime cordons

• Provider behavior: no provider-specific behavior changed.
• Runtime behavior: focused coverage was run for Tokio/native-tls and async-std/native-tls read implementations.

---

This change is 

Summary by CodeRabbit

• Bug Fixes

  • Fixed an issue where streamed response data could be partially discarded when reads used buffers smaller than incoming chunks; sequential reads now preserve and return the full payload.

• Tests

  • Updated tests to validate that reading in loops returns the complete response for different async runtimes.

[brownian-motion-v0]

Brownian Motion (Brass)

Recommendation: Refactor

Summary: PR addresses data loss in ResponseDataStream; approach is sound but complex.
Risk: Medium · Confidence: 80%

Highlights

• Good test coverage
• Addresses a clear bug

Unknowns

• Impact on performance with large streams

Next actions

• Keep: Test cases that validate the fix
• Drop: Unused imports and redundant comments
• Add: Documentation for the new behavior

Reflection questions

• What core assumption underpins this PR's approach?
• How does this change align with the project's longer-term goals?
• Could there be a simpler way to achieve the primary objective here?

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 23d675e7-192a-4083-8600-f6bd10080a57

📥 Commits

Reviewing files that changed from the base of the PR and between acf4aa1 and d106d33.

📒 Files selected for processing (1)

• s3/src/request/request_trait.rs

✅ Files skipped from review due to trivial changes (1)

• s3/src/request/request_trait.rs

---

📝 Walkthrough

Walkthrough

Fixes the ResponseDataStream AsyncRead implementations (Tokio and async-std) so that when a destination buffer is smaller than an incoming chunk, only the buffer-sized prefix is written and the remainder is reinjected into the stream for subsequent reads, preventing loss of bytes across reads.

Changes

AsyncRead Stream Preservation

Layer / File(s)	Summary
Import Updates s3/src/request/request_trait.rs (lines 24–27)	Replaced runtime-specific stream imports with futures_util::Stream and futures_util::stream::{self, StreamExt} under the with-tokio/with-async-std feature gates.
Tokio AsyncRead Implementation s3/src/request/request_trait.rs (lines 125–163)	ResponseDataStream AsyncRead::poll_read now repeatedly polls the stream, writes at most buf.len() bytes, and if a chunk contains extra bytes, re-inserts the remainder into self.bytes by chaining a one-shot stream rather than dropping it.
Async-std Read Implementation s3/src/request/request_trait.rs (lines 169–207)	Mirrors Tokio change: Read::poll_read copies only the buffer-sized prefix from chunks and reinjects any leftover bytes into self.bytes using a one-shot chain to preserve them for subsequent reads.
Tokio Tests s3/src/request/request_trait.rs (lines 899–923)	Replaced small-buffer test with one that reads repeatedly into a small buffer until EOF and asserts the full payload is returned, verifying remainder preservation.
Async-std Tests s3/src/request/request_trait.rs (lines 1015–1040)	Same test pattern as Tokio: loops small-buffer reads until EOF and compares accumulated bytes to the expected full payload.
Manifest Cargo.toml	No API/export changes; feature-gated imports align with existing feature flags.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 I munched through streams that split a chunk in two,
Rewove the leftover bytes so none bid adieu.
Now reads loop cleanly, small buffers need not fear—
Every byte returns home, each nibble held dear. 🥕✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and specifically describes the main change: fixing ResponseDataStream's handling of small-buffer reads, which is the core issue addressed in the PR.
Linked Issues check	✅ Passed	The PR fully addresses issue #437 by implementing logic to preserve unread chunk remainders when read buffers are smaller than stream chunks, ensuring AsyncRead adapters return all bytes.
Out of Scope Changes check	✅ Passed	All changes are scoped to fixing the ResponseDataStream AsyncRead implementations for Tokio and async-std; no unrelated modifications detected.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/issue-437-response-stream-read

---

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

• X
• Mastodon
• Reddit
• LinkedIn

_{Review rate limit: 4/8 reviews remaining, refill in 28 minutes and 57 seconds.}

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[brownian-motion-v0]

Brownian Motion (Brass)

Recommendation: Refactor

Summary: PR addresses data loss in small-buffer reads, but complexity may outweigh benefits.
Risk: Medium · Confidence: 70%

Highlights

• Good test coverage
• Clear commit history

Unknowns

• Impact on performance with larger streams
• Long-term maintainability of the new stream handling approach

Next actions

• Keep: tests that verify correct behavior with small buffers
• Drop: unnecessary complexity in handling stream re-insertion
• Add: documentation explaining the rationale behind changes

Reflection questions

• What core assumption underpins this PR's approach?
• How does this change align with the project's longer-term goals?
• Could there be a simpler way to achieve the primary objective here?

[durch]

Thanks for the data-integrity fix. The previous AsyncRead implementation could drop bytes when a caller-provided read buffer was smaller than the next stream chunk; this preserves chunk remainders and adds focused coverage for small-buffer reads.

Verification before merge:

• local make ci passed in the issue worktree
• refreshed GitHub checks are green after Fix presign tests: use fake credentials instead of minio env vars #453 restored the presign-test baseline
• reviewed under triage rules: PASS; high-impact streaming area, but the fix is localized and covered by targeted regression tests

Merging this as the fix for #437.