Repository navigation
Conversation
b7r6
force-pushed
the
pr/completeness-error-propagation
branch
3 times, most recently
from
October 8, 2026 03:51
4b3c21c to
f00f9c0
Compare
b7r6
force-pushed
the
pr/completeness-error-propagation
branch
from
October 14, 2026 17:40
f00f9c0 to
11ba620
Compare
b7r6
force-pushed
the
pr/completeness-error-propagation
branch
from
October 15, 2026 11:19
11ba620 to
22d253a
Compare
…allowing them CompletenessCheckingStore collapsed every backend failure into a NotFound miss. A miss tells the client the result does not exist and to re-execute; a backend fault (unavailable Redis, undecodable record) is transient and retryable. Laundering the fault as a clean miss silently discards a valid cached result — and behind an AC-serving frontend it turns a 5xx-class condition into a 404. NotFound from the backend still becomes a miss (the genuine eviction path is untouched); any other code now propagates. CacheLookupScheduler is updated in the same change: a cache-lookup fault there must never fail the Execute request, so non-NotFound lookup errors now log a warning and fall through to execution — the same availability the system had when the store laundered them, but visible to operators and correct at the AC service surface. Previously the scheduler's propagate-the-error branch was unreachable for backend faults precisely because the store swallowed them. Tests: an injected always-failing AC backend must surface its real code through both get_part and has (fails against the old store code, which demoted the error to NotFound), and the same injected fault at the scheduler must delegate to execution rather than fail the client. Co-Authored-By: Claude Fable 5 <[email protected]>
b7r6
force-pushed
the
pr/completeness-error-propagation
branch
2 times, most recently
from
October 15, 2026 11:59
5ca95bb to
20c288c
Compare
…ried blob (TraceMachina#2840) `sizes_for_keys` snapshotted `lru.len()` once for the whole batch and reused that stale length in `should_evict` for every key in the loop. When the map sits one entry over `max_count` — e.g. an action-input lease (or a TTL-pending entry) pins it at `max_count + 1` — the first unleased queried key is popped, bringing the real length back to `max_count`. But iterations 2..N still saw the stale `lru_len == max_count + 1`, so `is_over_count` stayed true and every remaining queried, unleased entry was popped too: remove-callbacks queued, `FileEntryImpl` unref deleting the backing files. The batch then reports `None` for all N digests, the client treats them as absent and re-uploads, and a plain existence check (`has()` → FilesystemStore → `sizes_for_keys`) has destroyed every resident blob it was only asked about. Fix: keep a mutable working count and decrement it as entries are reaped, so the over-count predicate is re-evaluated against the current length each iteration. Eviction now stops the moment the map is back at/under the limit — a size/existence query evicts at most the genuine overage and never cascades. Adds a white-box regression test that pins the map one over `max_count` with a leased entry (bypassing the insert-time eviction pass the way a held lease does) and asserts a batch `sizes_for_keys` over the resident keys leaves all but the single-entry overage present. It fails on the stale-len code (all N reported missing) and passes with the fix. Provenance: adversarial review fleet, highest-severity finding.
* Update Rust crate rustls to v0.23.46 [SECURITY] * Fix MODULE.bazel.lock
…report QueryWriteStatus honestly (TraceMachina#2881) * fix(service): reject ByteStream finish_write before the declared blob size A ByteStream Write that sets finish_write=true before sending the full size declared in its resource name was accepted as a complete upload: the handler returned OK with committed_size equal to the full declared size, the blob then showed as present in FindMissingBlobs, and Read served the truncated bytes under a digest they do not hash to -- silent CAS content corruption. Both the streaming path (process_client_stream) and the oneshot path (inner_write_oneshot) only guarded the too-many-bytes case and committed on finish_write without checking that the received byte count equalled the declared size. A VerifyStore-fronted write path catches this on EOF, but an unverified store tier (filesystem/memory fast tier) commits the partial. Reject finish_write with InvalidArgument when bytes_received != expected_size in both paths. The compressed path already verifies decoded size and digest. Also fixes a latent same-class defect in max_decoding_message_size_test, whose success case declared a size that did not match the bytes it sent and only passed because of this bug. Co-Authored-By: Claude Opus 4.8 <[email protected]> * fix(cas): reject short/torn uploads and report honest write status Unifies the handler-level short finish_write rejection (N13, e5d1b0d) with two further CAS-integrity fixes for the same disclosure (TRA-1373): - filesystem_store: refuse to emplace a blob whose byte count disagrees with an ExactSize upload hint. A torn/aborted upload stream closes the writer half, which arrives at the store as a clean EOF; without this commit-point guard the partial bytes are stored under the full digest key regardless of any handler check (finish_write never arrives on the abort path). - bytestream_server: QueryWriteStatus clamps committed_size to the declared digest size (the filesystem store reports block-rounded on-disk footprint, e.g. 17 -> 4096) and only reports complete once the store holds at least that many bytes, instead of unconditional complete=true. Regression tests: - short_finish_write_is_rejected_and_not_committed (oneshot path, N13) - short_finish_write_is_rejected (streaming multi-message path) - aborted_mid_stream_upload_is_not_committed (EOF without finish_write; only the store-level guard can catch this) - max_decoding_message_size_test updated: it asserted the old short-upload-accepted behavior. Co-Authored-By: Claude Opus 4.8 <[email protected]> --------- Co-authored-by: b7r6 <[email protected]> Co-authored-by: Claude Opus 4.8 <[email protected]> Co-authored-by: Tom Parker-Shemilt <[email protected]>
…ceMachina#2816) tonic codes a transport failure it cannot classify further (a connection reset or broken pipe mid-stream) as Code::Unknown with message "transport error"; tonic's own Status::from_error maps the shapes it does recognize (e.g. a connect refusal) to Unavailable already. The gRPC status model reserves UNKNOWN for genuinely unmapped errors and codes transport failures UNAVAILABLE. Left as Unknown, such a blip is treated as permanent by REAPI clients — Bazel retries UNAVAILABLE but not UNKNOWN — and by any retry configuration keyed to Unavailable, so a retryable failure becomes terminal and cacheable uploads are silently dropped mid-write. Detection is downcast-first: walk the Status source chain for a concrete tonic::transport::Error. The fallback, for a Status reconstructed from the wire where only the message survives, is anchored to the two shapes that actually occur: tonic's exact "transport error" rendering, and "transport error : <context>" as produced by this crate's own From<Error> for Status when a peer re-encodes and forwards one. An app-level Unknown that merely mentions the phrase mid-message is not reclassified. Upstream's TraceMachina#2657 made the same argument for transient Redis failures; this applies it to the adjacent tonic transport surface.
…ents (TraceMachina#2839) Two confirmed bugs left the RedisStore keyspace/pattern subscriptions recorded as active while never actually established on the live connection. Both come with a deterministic repro test that fails on the old code and passes on the fix. 1. psubscribe lost across a concurrent reconnect (StandardRedisManager). `psubscribe` used to fetch a connection, then insert the pattern into the tracked `subscriptions` set. If a concurrent `reconnect` snapshotted+replayed that set on the new connection in between, the pattern was PSUBSCRIBEd only on the old, about-to-be-demoted connection and recorded as active — so every PMessage for it was dropped until an unrelated later reconnect replayed the now-complete set. Fix: order registration against the reconnect snapshot. `psubscribe` now inserts the pattern BEFORE fetching a connection, and `reconnect` recomputes the missing delta and subscribes it while holding the `connection_manager` write lock just before publishing the new handle. A racing `psubscribe` then either lands in the replayed snapshot or blocks on the write lock and subscribes on the already-published live connection — no window where a pattern is "active" but subscribed only on a stale connection. 2. transient init error permanently disabled remove callbacks. `register_remove_callback` returned Ok(()) and deferred the CONFIG GET + PSUBSCRIBE "__key*__:*" to a background one-shot `OnceCell::get_or_try_init`. A single transient Redis error (e.g. -LOADING during a restart/failover) was only logged; the OnceCell stayed empty and nothing retried, so eviction/expiry callbacks were silently dead forever (ExistenceCacheStore never invalidated → stale `exists` hits). Fix: retry the init with capped backoff until it succeeds, and demote the advisory notify-keyspace-events CONFIG GET check to non-fatal (log misconfig, subscribe anyway) so a transient read failure can no longer permanently disable the subscription. Both repro tests fail on the unfixed code and pass on the fix (verified both directions on the build fleet). Full `cargo test -p nativelink-store` green, clippy clean, nightly rustfmt applied. Provenance: adversarial review fleet
Owner
Author
|
Landed upstream as TraceMachina#2815. Closing. |
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
Lane A stability fix from the findings catalog (see
nativelink-findings-2026-10-05.csvon the
meta/findings-catalog-2026-10-05branch), rebased onto OSS main@bf4b3aa855. This fork PR exists to validate the change on CI that is
representative of upstream PR lanes (public read-only cache, same endpoints)
before any upstream submission.
How was this verified?
The branch carries its own regression tests (fails-without where the fix
class allows it — see the branch's test commits). This PR's purpose is the
full representative CI run; green here rehearses the exact checks an
upstream PR would face.
Risk
Fork-internal validation PR; nothing lands anywhere by merging this. The
underlying fix's risk assessment happens at upstream-submission time.
AI assistance
Agent-assisted pipeline (Claude) for rebase and PR mechanics under my
direction; the fix itself carries verification in its tests.