Fix compressed-download failure classification in GrpcStore wire compression

[walter-zeromatter]

Review follow-ups on top of TraceMachina#2596 (erneestoc:ec/grpc-wire-compression, at ead7c03b). Base branch here is that PR's head, mirrored into this repo so the diff reads as review deltas only.

The feature itself checks out: REAPI conformance is right (first-request write_offset uncompressed / subsequent cumulative-compressed, read_limit: 0 mandatory on compressed reads, ranged reads forced to identity), the exact-multiple decoder fix is real, and the premise is accurate — before TraceMachina#2596 nothing in NativeLink ever requested compressed-blobs, only accepted and advertised it, so every worker↔CAS hop shipped raw bytes.

Fixes

Pump-stage failures were classified as retryable. get_part_compressed ran pump errors through is_retryable_code, but pump errors come from our own writer and buf_channel reports a dropped receiver as Internal, which that function calls retryable. A consumer that hung up mid-download therefore returned Ok(Some(forwarded)), and get_part opened 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, since try_join! polls decode first. The identity path has always treated writer errors as terminal (RetryResult::Err), so this also removes an inconsistency.

A late error could discard a completed, verified download. If the upstream delivered the whole frame and then failed on the response trailer, decode and pump could both finish — writer closed, digest verified — while feed reported a retryable error, and the result was still a resume into an already-closed writer. 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 buf_channel surfaces a sender dropped without EOF as an error, not as EOF), so that flag is a sound completion signal. A late error after it resolves as success.

A resume kept the caller's original length. The entry guard only admits a length that covers the whole blob, so a resumed request from offset = forwarded ran past the end of the blob and relied on the server clamping it. It now asks for exactly size - forwarded.

Two smaller ones. The compressed upload path is now gated on is_digest_key like the download path — for a string key into_digest() hashes the key itself, which the remote's mandatory digest verification would reject (unreachable today only because the 64KiB threshold outruns any plausible key length, but the asymmetry invites a future bug). And the background drain spawn is skipped when the encoder finished cleanly, where the reader is already at EOF and drain() returns immediately.

Tests

The resume path — the subtlest logic in the feature — had no coverage at all. The fake ByteStream now honors read_offset/read_limit so a resumed range can be asserted byte-exactly, and grew a mid-frame abort mode. It also grew a mixed-entropy payload helper: make_content compresses far enough that an 8MiB blob encodes into a single 64KiB wire chunk, so the original interruption test could never fire mid-frame.

Three new cases, all fails-without-fix:

• interrupted_compressed_download_resumes_via_identity / ..._with_explicit_length — assert the blob comes back byte-identical, exactly one fallback, a non-zero resume offset, and offset + limit == size. Pre-fix, the explicit-length form requests past the end.
• compressed_download_consumer_hangup_does_not_refetch — pre-fix this issues an identity re-read into a dead channel (identity_reads == 1); post-fix zero.

Verification:

• cargo test: service grpc_wire_compression_test 12/12, util wire_compression_test 2/2, store grpc_store_test 8/8 and grpc_read_batching_test 7/7.
• cargo clippy -p nativelink-store -p nativelink-service --tests clean; nightly cargo fmt --check clean.
• Not run: the Bazel suites and the wider workspace.

End-to-end run against a real Bazel client

Two NativeLink tiers in one process — Bazel → front tier :50051 (fast_slow of a local filesystem store over a grpc slow tier) → upstream tier :50061 (filesystem CAS/AC, remote_cache_compression: true). The fast_slow wrapper is load-bearing for the test: a bare grpc store is short-circuited by the ByteStream/CAS services, which downcast to GrpcStore and forward the client's resource name verbatim, bypassing update/get_part and therefore wire compression entirely.

Workload: bazel build //lib/zerotrace in an unrelated ~3.4k-action Rust/C++ repo, disk cache disabled, 1168 executed actions, 184MB of CAS content. Bazel↔front is identity in every arm (Bazel was not given --remote_cache_compression), so the arm-to-arm delta in loopback bytes is the front↔upstream leg — the code under test.

phase	flag	wall	loopback bytes	front↔upstream (derived)
populate (uploads)	off	126.1s	363.4MB	~179MB
populate (uploads)	on	129.8s	263.5MB	~79MB — 2.3:1
1168 cache hits (downloads)	off	22.4s	37.4MB	~19MB
1168 cache hits (downloads)	on	17.5s	30.1MB	~12MB — 1.6:1

Both arms stored byte-identical content (184MB each side, Bazel digest-verified every downloaded output), and the compressed arms logged zero errors or warnings beyond the startup connection race. Caveats worth stating plainly: single run per arm, loopback has no bandwidth limit, so the wall times are not a controlled latency measurement — the upload deltas are within noise (the local build dominates) and the download arm's 4.9s is one sample. The byte ratios are the trustworthy numbers. Downloads compress less than uploads because most of what Bazel actually fetched sits below the 64KiB threshold and stays identity.

Left alone deliberately

• Misconfiguration is still a hard failure in both directions. InvalidArgument/Unimplemented at compressed-stream start propagate, so one wrong flag upstream fails every blob ≥64KiB. The fallback machinery already exists (Ok(Some(0))), so a download could fall back once and warn loudly — but silent downgrade hides misconfiguration, and the current behavior is documented and tested. Reads as a call for the feature owner, not a review fix.
• Compressed uploads remain non-resumable. Inherent: the source reader is one-shot, so a fresh-uuid replay from offset 0 isn't available. Worth knowing that enabling the flag trades some upload resilience for wire savings, with no outer retry around StoreDriver::update.
• Batch paths remain identity. BatchReadBlobs still sends acceptable_compressors: vec![] and BatchUpdateBlobs is uncompressed — consistent with the feature's small-blob reasoning.
• zstd is now a dependency of nativelink-util, hence transitively of nearly every crate. Unavoidable given nativelink-store cannot depend on nativelink-service.

🤖 Generated with Claude Code