docs(download): document DownloadWriter and the writer-truncation guarantee - #471
Conversation
…rantee from whatsapp-rust#1197 download_to_writer, download_from_params_to_writer, and MediaDownloader::download_to_writer now require W: DownloadWriter (Write + Seek + truncate) instead of plain Write + Seek, so a failed host's plaintext can no longer leave a stale tail behind a shorter successful retry. Documents the new trait, its built-in impls, the updated writer contract (exactly the media on success, empty on failure), and updates the ProgressWriter example to implement it.
|
Warning Review limit reached
Next review available in: 46 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughDocuments the new ChangesDownload writer contract
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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. Comment |
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8f518528a9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| --- | ||
|
|
||
| ## DownloadWriter Trait |
There was a problem hiding this comment.
The repository requires sentence case for headings, but DownloadWriter Trait capitalizes the common noun Trait. Change it to DownloadWriter trait so the new section follows the site convention.
AGENTS.md reference: AGENTS.md:L26-L26
Useful? React with 👍 / 👎.
| } | ||
| ``` | ||
|
|
||
| Media is authenticated by a single MAC over the whole ciphertext, so decryption has necessarily streamed plaintext into the writer by the time a forged body is caught, and a retry against the next host may end up writing fewer bytes than the attempt it replaces. Rewinding with `seek` alone can't remove bytes that are already there — shortening a sink requires a concrete operation (`File::set_len`, `Vec::truncate`) that no `std` trait exposes. `DownloadWriter::truncate` names that operation, which is what lets `download_to_writer` guarantee: **exactly the media on success, empty on failure.** |
There was a problem hiding this comment.
When truncate(0) fails during final best-effort cleanup, a caller with a shared handle can still observe pre-existing or unverified bytes—the note above explicitly acknowledges that cleanup can fail. Calling empty on failure a guarantee therefore encourages consumers to trust state the API does not promise; qualify the failure case as best effort while retaining the exact-on-success guarantee.
Useful? React with 👍 / 👎.
| fn truncate(&mut self, len: u64) -> std::io::Result<()> { | ||
| self.inner.truncate(len) |
There was a problem hiding this comment.
When a host writes bytes and then fails validation, retry startup calls this truncate(0), but the implementation truncates only the inner writer and leaves total unchanged. The next attempt therefore reports failed-attempt bytes plus current bytes and can finish above the actual file size; synchronize the shared counter with len after successful truncation so the progress example remains correct under automatic failover.
Useful? React with 👍 / 👎.
|
|
||
| Media is authenticated by a single MAC over the whole ciphertext, so decryption has necessarily streamed plaintext into the writer by the time a forged body is caught, and a retry against the next host may end up writing fewer bytes than the attempt it replaces. Rewinding with `seek` alone can't remove bytes that are already there — shortening a sink requires a concrete operation (`File::set_len`, `Vec::truncate`) that no `std` trait exposes. `DownloadWriter::truncate` names that operation, which is what lets `download_to_writer` guarantee: **exactly the media on success, empty on failure.** | ||
|
|
||
| Every attempt begins by truncating the writer to 0 and rewinding it, so only that attempt's own bytes are ever present when it finishes — including on an append-mode `File`, where truncating (not just seeking) is what brings the write position back to the start, since files opened for appending ignore `seek` and always write at the end. |
There was a problem hiding this comment.
For an append-mode File, set_len(0) does not move the cursor, and append mode does not make seek a no-op: the explicit rewind changes the current position, while each write is separately forced to the current end of the file. This explanation can mislead custom implementers about which operation satisfies each part of the contract; describe truncation as clearing the file length and seeking as resetting its position.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@api/download.mdx`:
- Around line 140-142: The DownloadWriter trait documentation should explicitly
describe the separate final rewind performed after a successful download. Update
the trait section near the existing truncate-and-rewind-attempt behavior to
state that successful completion seeks the writer back to position 0, matching
the postcondition documented for the ResponseField “writer”.
In `@guides/media-handling.mdx`:
- Line 622: Rewrite the guidance around download_to_writer in active voice and
address the reader directly, stating that when the reader passes a writer, they
must implement DownloadWriter rather than only Write + Seek. Preserve the
existing reference and one-line delegation guidance.
- Around line 624-627: Add the missing std::fs::File import to the fenced Rust
snippet containing DownloadWriter and the File::create("video.mp4") usage, so
the example compiles without changing its existing behavior.
- Line 682: Update the streaming-download documentation around
“download_to_writer” to describe writer cleanup before each attempt, including
the first, rather than before each retry. Use active voice stating that the
client empties and rewinds the writer, and retain the existing final best-effort
cleanup behavior and DownloadWriter reference.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b381bf59-9dfa-4699-ad8d-cb2554d450f1
📒 Files selected for processing (2)
api/download.mdxguides/media-handling.mdx
- Sentence-case the DownloadWriter trait heading. - Separate truncate's role (clears length) from seek's (resets position) in the append-mode File explanation, and note the final seek(0) that produces the "seeked back to position 0" postcondition on success. - Qualify empty-on-failure as best-effort, matching the existing note. - Fix ProgressWriter's truncate to reset its own byte counter, and add the File import the example was missing. - Rewrite "before each retry — including the first attempt" (contradictory) as "before each attempt, including the first", in active voice.
2 participants
Follows up on oxidezap/whatsapp-rust#1197, which fixed a bug where a failed CDN host could leave stale, unverified plaintext past the end of a successful
download_to_writerretry.What changed upstream
download_to_writer,download_from_params_to_writer, andMediaDownloader::download_to_writernow takeW: DownloadWriter(a new trait:Write + Seekplustruncate) instead of plainW: Write + Seek. This is a breaking API change. Every attempt now starts by truncating the writer to empty rather than only rewinding it, so:std::fs::File,std::io::Cursor<Vec<u8>>,std::io::Cursor<&mut Vec<u8>>,std::io::BufWriter<W>, and&mut Wall implementDownloadWriteralready; a custom writer needs one additional method.Doc updates
api/download.mdxdownload_to_writer,download_from_params_to_writer, andMediaDownloader::download_to_writersignatures and param/response fields toDownloadWriter.## DownloadWriter Traitsection: the trait definition, whyWrite + Seekwasn't enough, the built-in implementations, and a short example implementing it for a custom wrapper type — with aBreaking change (as of PR #1197)callout in the same style as the existingMediaConnection → MediaRoutenote.guides/media-handling.mdxProgressWriterexample to implementDownloadWriter(it would no longer compile against the new bound otherwise) and added a lead-in sentence pointing to the new reference section.Per the usual policy, no changelog entry was added — that's left for a human to write.
Generated by Claude Code
Summary by cubic
Documented the new
DownloadWritertrait and the truncate-then-rewind behavior for streaming downloads. Clarifies that writers contain exactly the decrypted media on success and are emptied on failure on a best-effort basis; updates API signatures, refines the append-mode explanation (truncate vs seek + final seek(0)), and fixes theProgressWriterexample (counter reset, addsFileimport).W: DownloadWriter:download_to_writer,download_from_params_to_writer,MediaDownloader::download_to_writer.std::fs::File,Cursor<Vec<u8>>,Cursor<&mut Vec<u8>>,BufWriter<W>, or&mut W.DownloadWriter::truncate; the Progress writer guide shows a one-line delegate.Written for commit 5461a0d. Summary will update on new commits.
Summary by CodeRabbit
DownloadWriterrequirement and its truncation behavior.