Wgray/zstd store/conflict resolution - #2719
Open
MarcusSorealheis wants to merge 29 commits into
Open
Conversation
Adds a downcast helper that checks only the immediate inner StoreDriver, without following inner_store() like the existing recursive downcast_ref. Needed so callers (e.g. the service layer) can detect a directly-configured representation-changing store such as the upcoming ZstdStore at an instance boundary, rather than resolving through pass-through wrappers. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
nativelink-store failed to compile (E0004) because the StoreSpec::ZstdStore variant had no match arm in default_store_factory.rs. Add the arm, wiring ZstdStore::new to its backend store like the other pass-through stores, and add a factory test that builds a ZstdStore via store_factory and downcasts to confirm the concrete type.
…dation tests Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… concat-frame test Register staged/recompression temp file paths in TempFileGuard before create_empty_temp_file runs, so a set_permissions failure or cancellation during file creation can't leak an untracked file. Add a post_init write-probe so a read-only or mispermissioned temp_path fails at startup instead of on first upload. Document that the 0o600 permission is unix-only. Add a test proving get_for_batch's raw-decode path handles a concatenated multi-frame zstd physical stream.
…rite Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…sion The ZstdStore fast path in inner_batch_update_blobs took a client's zstd-compressed blob and stored it verbatim via update_zstd_oneshot whenever the instance's CAS store happened to be a ZstdStore, without checking remote_cache_compression_enabled. This let a ZstdStore-backed instance silently accept zstd uploads even when remote cache compression was disabled for it, unlike the non-ZstdStore path (which already rejects zstd via decompress_batch_update) and capabilities (which stops advertising zstd when disabled). Require remote_cache_compression_enabled alongside the ZstdStore/Zstd compressor check so a disabled instance falls through to decompress_batch_update and gets the same rejection as any other store. Also moved the size_bytes computation into the else branch since it is only used there.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…fast-path sites Add a non-recursive owned-Arc sibling to downcast_ref_immediate for callers that must move the concrete store into a spawned/'static future, and replace the four repeated `store.clone().into_inner().as_any_arc().downcast::<ZstdStore>()` idioms in the ByteStream and CAS servers with it. Behavior is unchanged: into_inner() returns the immediate inner Arc. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ZstdStore's identity encode/decode paths run inside `spawn_blocking!` and need `std::io::Read`/`Write` views over a buf_channel pair. Add `BufChannelReader`/`BufChannelWriter` to `nativelink_util::buf_channel`, where the underlying `DropCloserReadHalf`/`DropCloserWriteHalf` already live, and drop the private copies from zstd_store.rs. `BufChannelWriter` deliberately does not send EOF on drop: the caller sends it explicitly once an upload has been validated, so a failed write never commits downstream. The service wire codecs used to need the same adapter, but no longer do — they drive the zstd raw decoder from an async loop instead of a blocking one — so these adapters now exist solely for the store's blocking paths. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…tore Introduce a single private decode_all_zstd(data, size_hint, on_err) helper and use it at the three whole-buffer decode sites (two zstd::stream::decode_all and one zstd::bulk::decompress). The size_hint selects bulk vs streaming decode and the on_err closure preserves each site's exact error code and message (InvalidArgument for client input, DataLoss for stored data). The two streaming decoders are left untouched. Behavior is unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Under Bazel (unlike cargo, which inherits crate deps) the store integration test suite needs `@crates//:zstd` declared explicitly for the ZstdStore tests. Caught by the clippy/build aspect. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…pinned staging Harden the ZstdStore fast path against resource-exhaustion, starvation, and staging path-replacement races, and clean the patch-specific CI failures. Issue 1 — bounded decompression at the output sink: - DecodeSink enforces the decoded-output ceiling inside Write::write using checked_add; output past the digest's uncompressed size is rejected immediately (InvalidArgument) before hashing/collecting, stopping a zstd bomb at the first over-limit block. - Zero-digest validation streams through the same bounded decoder with an output cap of zero (no whole-buffer decode/allocation) and now acquires the staged-upload semaphore, so it participates in concurrency admission. Issue 2 — slow-client / stall starvation: - The compressed ByteStream client pump applies an idle timeout to waiting for the next WriteRequest (reusing persist_stream_on_disconnect_timeout_s); a client making progress is never timed out. On timeout the channel sender is dropped so the blocking validation unwinds through the existing join, and DeadlineExceeded is surfaced as the primary error. - Inner-store commit (and recompression) is bounded by a new commit_timeout_s (default 300s); on expiry the upload fails DeadlineExceeded and the staged file/permit are released, so a stalled backend cannot hold a slot forever. Issue 3 — descriptor-pinned staging: - Staging files are created exclusively (create_new/O_EXCL, 0o600 atomic on unix); the validated descriptor is retained, rewound, and handed to the inner store at commit (never reopened by pathname), defeating observe-and-replace races. Recompression candidate handled the same way. - post_init verifies temp_path is a directory and rejects a world-writable directory lacking the sticky bit. Adds fs::FileSlot::from_std. Issue 4/5 — CI + docs: - Regenerate stores-config.json5, add Vale vocabulary terms, fix MDX-unsafe comparison operators (windowLog <= 23 -> ≤), and reconcile store-overview docs with the new bounds/admission/deadline/temp_path/descriptor behavior. Adds focused tests: sink bound + zero-digest bomb + over-decode rejection, descriptor-pinned commit, commit-timeout slot release (store); ByteStream idle-timeout slot release + progress-not-timed-out (service); batch per-blob timeout sibling isolation (service). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Correctness and availability: - Qualify the descriptor-pinning claim. Only backends using the default `update_with_whole_file` read the validated descriptor; `filesystem` drops it and commits by `rename(2)` on the path, where the defence is `O_EXCL` creation of an unguessable name in an operator-private dir. Adds a filesystem-backed commit test, which the previous `MemoryStore`-only test could not cover. - Report the staging error, not the feeder's consequential channel error, when a large oneshot upload is rejected. - Route the negotiated compressor through a single `wire_compressor_capability` helper instead of hardcoding zstd in the ByteStream read/write and BatchUpdate fast paths. The helper lives in `nativelink_util::wire_compression` next to the codecs and `WireCompressor`, and is re-exported from the service module. - Add `stage_timeout_s`: a total validate-and-stage deadline, since a per-message idle timeout is reset by a client that trickles bytes to hold a staging slot open. - Make recompression best-effort (`try_acquire`). It previously queued on the recompression semaphore while holding a staging permit, letting a pool of 1 throttle the whole upload path. - Reject `max_recompression_size > 0` without `compression_level`, which silently disabled recompression. - Add `max_concurrent_identity_ops` to bound identity reads/writes, each of which holds a blocking thread for a whole transfer. - Add `max_inline_commit_size`: validate and commit small compressed uploads from memory, so BatchUpdateBlobs stops paying a per-blob fsync. - Give compressed uploads their own `compressed_upload_idle_timeout_s` instead of borrowing `persist_stream_on_disconnect_timeout_s`. - Publish metrics for fast-path hits, inline vs staged commits, in-flight gauges, recompression outcomes, and deadline expirations. Fix an unbalanced brace that made `deployment-examples/docker-compose/local-storage-cas-zstd.json5` unparseable, and extend the json5 test to parse every deployment example so nothing ships broken again. Drop stale references to the abandoned two-candidate staging design, consolidate the duplicated join/error/decode helpers, and cut the comment density from 18% to 15% while adding the above. Also adapts the branch to upstream API changes picked up by the rebase: the `RemoveCallback` alias, `FilesystemSpec::evict_page_cache`, and the newly denied `clippy::cast_possible_wrap` in tests. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three CI failures, all in files this branch touches: - rustfmt (the pinned nightly used by the Bazel aspect) reflows the `store_trait` import and the `register_remove_callback` signature in zstd_store.rs, both of which changed width when the rebase adopted the shorter `RemoveCallback` alias. It also wants the new `use crate::store_trait::WireCompressor` sorted after `digest_hasher` rather than after `buf_channel`. - `nativelink-config/examples/stores-config.json5` is generated from the ```json blocks in stores.rs doc comments by `generate-stores-config`; it is not hand-editable. Add `max_concurrent_identity_ops` and `max_inline_commit_size` to the doc comment, which is the source of truth, so the generator reproduces the committed file exactly. - Vale lints Rust doc comments as well as MDX. Rephrase to avoid the possessives `backend's`/`upload's` and the two words absent from its dictionary (`untrusted`, `expirations`) instead of widening the accepted vocabulary for ordinary prose. Verified with `bazel test //...` (103/103, so the `unit_test` targets that surfaced the rustfmt failures are covered this time) plus the cargo test, clippy, and nightly rustfmt runs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The rules_rust rustfmt aspect (nightly/2026-03-24) wanted three reformattings in zstd_store.rs that the local nightly/2026-04-09 rustfmt did not flag, breaking the Bazel Dev, asan, ubuntu-24.04, and Bazel 8.7.0 jobs. Apply them so both rustfmts agree. Vale flagged 'validator' as a spelling error in stores.rs and the store-overview.mdx; it is a legitimate word for the blocking decode task, so add it to the TraceMachina accept vocabulary.
The #[nativelink_macro::nativelink_test] attribute on the inline batch_identity_decode_waits_for_identity_admission test expands to tracing_test::logs_assert. The unit_test target's deps lacked @crates//:tracing-test, so the rules_rust clippy aspect (run by bazel test //...) failed with E0433/E0425. The integration test suite already had it; this mirrors nativelink-util's unit_test.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
This comment has been minimized.
This comment has been minimized.
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.
What and why
Resolved known merge conflicts. The project was ultimately verified by running
bazel test //...and observing all tests passing.Risk
If consumers enable zstd compression for
gRPCstore there could be high CPU costs for little benefit.This change is