Harden the compressed GrpcStore download path - #2640
Merged
palfrey merged 5 commits intoAug 5, 2026
Merged
Conversation
Follow-ups to the wire-compression work added in TraceMachina#2596. `get_part_compressed` classified pump-stage failures with `is_retryable_code`, but pump errors come from our own writer and `buf_channel` reports a dropped receiver as `Internal`, which counts as retryable. A consumer that hung up mid-download therefore returned `Ok(Some(forwarded))` and made `get_part` issue a second, full-size identity Read streaming the remainder into a channel nobody was reading. Pump failures are now always terminal: they mean either the consumer went away or the decoder aborted, and the decoder's verdict is already reported by the arm above it since `try_join!` polls decode first. The fallback also treated a late failure from another stage as a reason to resume even when the download had already completed and been verified. The pump now records that it forwarded the decoder's EOF -- which the decoder only sends after the blob passes its size and digest checks -- and a late error after that point resolves as success instead of resuming into an already-closed writer. On a genuine resume, the identity request now asks for exactly the undelivered tail. It previously kept the caller's original `length`, which the entry guard only permits when it covers the whole blob, so the resumed request ran past the end of the blob and relied on the server clamping it. The compressed upload path is now taken only for real digest keys: a string key's `into_digest()` hashes the key itself, which the remote's mandatory digest verification would reject. The background drain spawn is also skipped when the encoder finished cleanly and the reader is already at EOF. Tests: new cases in the service suite for the aborted-consumer and resume paths. The resume path had no coverage at all -- the fake ByteStream now honors `read_offset`/`read_limit` so the resumed range can be asserted byte-exactly, and grew a mid-frame abort mode plus a mixed-entropy payload helper, since `make_content` compresses far enough that multi-megabyte blobs fit in one wire chunk. Resume is asserted both with `length: None` and with an explicit `length` covering the blob; only the latter over-requested before this change, since `read_limit: 0` already means "to the end" on the wire. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
palfrey
approved these changes
Aug 3, 2026
Member
|
Seeing a failure in |
Contributor
Author
|
Looks like there was a nondeterministic test. Found & fixed, along with another unrelated bit of hardening |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Follow-ups to the opt-in zstd wire compression added to
GrpcStorein #2596.These are correctness fixes in the compressed download path, plus one guard
on the compressed upload path, found while reviewing that work.
1. Pump-stage failures were misclassified as retryable.
get_part_compressedclassified pump-stage failures withis_retryable_code,but pump errors come from our own writer, and
buf_channelreports a droppedreceiver as
Internal— which counts as retryable. A consumer that hung upmid-download therefore returned
Ok(Some(forwarded)), andget_partrespondedby issuing a second, full-size identity
Read, streaming the remainder of theblob into a channel nobody was reading. Pump failures are now always terminal:
they mean either the consumer went away or the decoder aborted, and the
decoder's own verdict is already surfaced by the arm above it, since
try_join!polls decode first.
2. Resume could fire after the download had already succeeded.
The fallback treated a late failure from another stage as a reason to resume
even when the blob had already been fully delivered and verified. The pump now
records that it forwarded the decoder's EOF — which the decoder only emits after
the blob passes its size and digest checks — and a late error after that point
resolves as success instead of resuming into an already-closed writer.
3. A genuine resume over-requested.
The identity retry now asks for exactly the undelivered tail. It previously
reused the caller's original
length, which the entry guard only permits whenit covers the whole blob, so the resumed request ran past the end of the blob
and relied on the server clamping it.
4. Compressed upload is now restricted to real digest keys.
A string key's
into_digest()hashes the key itself, which the remote'smandatory digest verification would reject. Also skips the background drain
spawn when the encoder finished cleanly and the reader is already at EOF.
Type of change
How Has This Been Tested?
New cases in
nativelink-service/tests/grpc_wire_compression_test.rscoveringthe aborted-consumer path and the resume path. The resume path had no coverage
at all before this change:
read_offset/read_limit, so the resumedrange can be asserted byte-exactly rather than just "it eventually finished".
make_contentcompresses far enough that multi-megabyte blobs otherwise fitin a single wire chunk and never exercise the partial paths.
length: Noneand with an explicitlengthcovering the blob. Only the latter over-requested before this change, because
read_limit: 0already means "to the end" on the wire.Verified locally on this branch:
Checklist
bazel test //...passes locally (ran the store + service wire-compressiontargets; the aspects cover clippy/rustfmt for the touched crates)
This change is