Fixed AsyncFixedBuffer by allada · Pull Request #3 · TraceMachina/nativelink

allada · 2021-11-03T06:55:07Z

Fixes various errors in AsyncFixedBuffer.

This change is

Fixes various errors in AsyncFixedBuffer.

allada

Reviewed 1 of 2 files at r1, 1 of 1 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @allada)

`RunningActionImpl::new()` now sources `work_directory` from the worker-configured `shared_tree_root` (resolved at startup from `LocalWorkerConfig.platform_properties["InputRootAbsolutePath"]`) instead of the action proto's per-action `InputRootAbsolutePath`. The action-side property is ignored entirely — siso/clang/Chromium no longer need to stamp it, and there is no need to patch `compiler.gni::clang_need_input_root_absolute_path` to coerce siso. Shared-tree execution is now a pure worker-deployment fact: - Worker refuses to start without a non-empty `InputRootAbsolutePath` in `platform_properties` (Values or QueryCmd resolved at startup). - Every accepted action runs with `cwd` aliased to that path. - No per-action sandbox path exists in the binary. Updated CLAUDE.md hard rule TraceMachina#3 to match the new contract. Rewrote the two pinning tests: - `work_directory_is_worker_configured_shared_tree_root` — proves the action's IRAP is ignored by stamping a deliberately-wrong path. - `new_action_without_input_root_property_is_accepted` — proves an action with no IRAP runs (vs. the previous Code::InvalidArgument). All 71 worker tests pass; workspace builds clean.

Six review nits surfaced on 7d37257 (spawn-detach populator). 1. Lock-hold-time micro-regression (nit TraceMachina#1, FIXED) `Drop::drop` and `get_loader` were calling `into_owned()` inside the `populating_digests` lock. Restored 006f897's pre-spawn-detach pattern: clone/owned-allocation hoisted outside the lock so the critical section is just the hashmap entry. 2. `Loader = Arc<OnceCell<()>>` is dead weight (nit TraceMachina#2, FIXED) The OnceCell was used solely for `Arc::ptr_eq`-based loader identity in `LoaderGuard::Drop`; the actual `OnceCell::get_or_try_init` call was removed by 7d37257's spawn-detach refactor. Replaced with `Arc<()>` and dropped the unused `tokio::sync::OnceCell` import. 3. Misleading send_error / waiter comment (nit TraceMachina#3, FIXED) The "best-effort send to the streaming buffer; ignore errors so a slow waiter cannot stall the producer" comment claimed `streaming_writer.send()` waited on waiters. It does not — `send()` only blocks for the brief write-lock on the chunk deque, and waiter backpressure is handled by sliding-window eviction inside the writer. Re-wrote to match actual semantics. 4. Stale `populate_and_maybe_stream` reference (nit TraceMachina#4, FIXED) `LoaderGuard`'s doc comment still pointed at `populate_and_maybe_stream` — renamed to `run_producer` by 7d37257. 5. `is_populator_caller` asymmetry rationale (nit TraceMachina#5, DOC) Added a TODO(fast_slow_asymmetry) note explaining why the populator-vs-waiter error split exists (failpoint suite contract), what would let it be unified (post-spawn-detach the waiter fallback yields a typed error too), and what blocks doing so today. 6. drain_streaming_buffer terminal-with-data race (nit TraceMachina#6, FIXED) The recovery path under `Err(Unavailable) + is_terminal()` used to create a fresh reader and treat any `Ok(_)` chunk as proof the producer succeeded. This silently swallowed producer errors when chunks remained behind in the sliding window. Reachable in production: any populate where (a) the drain reader fell behind and (b) the producer ended with `send_error` (slow-store mid- stream failure, fast-store update error) could return Ok(()) from `copy_slow_to_fast` despite the producer failing — leading to downstream callers (populate_fast_store{,_unchecked}) treating half-populated blobs as ready. Fix: refactored `drain_streaming_buffer` to consult `streaming_inner.terminal_result()` BEFORE creating a reader. If the producer is terminal, return its actual outcome regardless of buffered chunks. Eliminates the buggy "data means success" branch structurally — the recovery path is now a single-loop break-back to terminal check. Added `StreamingBlobInner::terminal_result()` accessor (returns `Option<Result<(), Error>>`) so the new code path can read the structured result without consuming a reader. Visibility: `drain_streaming_buffer` is now `#[doc(hidden)] pub` so the regression tests can call it with a hand-built `StreamingBlobInner`. Same pattern as the existing `populating_streaming_inner` test accessor. Regression tests: - `drain_streaming_buffer_propagates_terminal_error_over_buffered_data`: contract test — terminal=Err with chunks in the sliding window must surface the structured error. - `drain_streaming_buffer_eviction_race_propagates_error`: stress test (50 iterations) of the genuine race — producer hammers chunks into a 10-byte buffer then send_errors while drain runs concurrently. Verified to fail on iter 0 pre-fix (returned Ok despite producer error); passes deterministically post-fix. - `streaming_blob::tests::terminal_result_reflects_writer_outcome`: unit test for the new accessor (None / Some(Ok) / Some(Err)). Verification: cargo check --features quic,pprof --all-targets — clean cargo test --features quic,failpoints -p nativelink-store \ --test fast_slow_store_test — 29/29 pass (incl. 2 new) cargo test --features quic,failpoints -p nativelink-store \ --test failpoint_tests — 13/13 pass cargo test -p nativelink-util --lib streaming_blob — 16/16 pass (incl. 1 new for terminal_result) Pre-commit gate: code-reviewer / perf-optimizer agents are NOT available in this sub-agent's tool environment (no Task tool, no general-purpose subagent). Self-review pass completed against the CLAUDE.md style and performance criteria. Parent agent should dispatch retroactive code+perf review before pushing or deploying. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

…_blobs in populate_fast_store_* Review TraceMachina#3 framed the directory_cache fast-store-only check as the durability bug, with the proposed fix being to route through `Store::new(cas_store_arc.clone())` so mirror_blobs is consulted. On investigation the deeper root cause is in `populate_fast_store_*` — both the check-first (`populate_fast_store`) and unchecked variants called `copy_slow_to_fast` only, with no mirror_blobs path. Any caller that hit a mirror-only blob (running_actions_manager OR directory_cache) would re-fetch from the slow store and, on a server-down or server-lost-blob scenario, fail with NotFound — losing the only durable copy. Fix: add `materialize_mirror_to_fast(key)` helper that copies bytes from `mirror_blobs` into the fast store via `update_oneshot`. Call it FIRST in both `populate_fast_store` and `populate_fast_store_unchecked`, falling back to `copy_slow_to_fast` only if the mirror copy is absent. This makes the existing fast_store-only has-checks in directory_cache correct end-to-end: a mirror-only digest appears as "missing" → goes through `populate_fast_store_unchecked` → materializes from mirror → hardlinks succeed. No structural changes to the directory_cache control flow are needed — only updated comments. Sibling-bug audit (review TraceMachina#7) — `cas_store.fast_store()` / `fss.fast_store()` callsites in the worker crate: | callsite | action | | local_worker.rs:1800 (was 1791) | doc: store CONSTRUCTION, | | | new FSS = empty mirror | | local_worker.rs:1872 (was 1863) | doc: store CONSTRUCTION, | | | sibling FSS = empty mirror| | local_worker.rs:1922 (was 1911) | doc: pinned_mirror is | | | reported via SEPARATE | | | proto field, must not | | | merge with on-disk set | | directory_cache.rs:470 | doc: hardlink reference | | | needs FilesystemStore | | directory_cache.rs:2114 (was 2108) | doc: download_to_directory| | | uses wrapper internally | | directory_cache.rs:2403 | doc: mirror flows via | | | populate_fast_store_* | | directory_cache.rs:2899 (was 2870) | same | | directory_cache.rs:2518 / 2549 | hardlink site, reachable | | | only after populate; safe | Tests: `populate_fast_store_unchecked_materializes_mirror_only_blob` and `populate_fast_store_uses_mirror_when_disk_empty` write a mirror blob with NO slow-store backing, then assert populate succeeds and the bytes land on the fast store. Mutate-tested by stubbing `materialize_result = Ok(false)` in both functions — both tests fail with `NotFound … not found in either fast or slow store`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

… comments, lock-ordering stress, register_blobs alloc Address punch-list items TraceMachina#1-TraceMachina#5 from the 7-reviewer pass on commit 70b2f59: TraceMachina#1 (CRITICAL) — Tests gated on `test-utils` were silently skipped under `cargo test --all`. Added a dedicated CI step `cargo test -p nativelink-store -p nativelink-worker --features test-utils --tests` so the mirror_durability / blobs_in_stable_storage_handler / download_to_directory_mirror tests actually run. Added matching rust_test entries with crate_features=["test-utils"] in nativelink-store/ BUILD.bazel and nativelink-worker/BUILD.bazel for Bazel parity (cargo CI step is the authoritative coverage; comments document the limitation). TraceMachina#2 (MAJOR) — pb.rs hand-edits diverged from the .proto: pinned_mirror_digests (field 13) had abbreviated 6-line doc; mirror_used_bytes (14) had a truncated 4-line doc. Restored verbatim from worker_api.proto. TraceMachina#3 (MAJOR) — `register_blobs` collapse introduced a fresh Vec<DigestInfo> + 2 extend_from_slice copies. Added `register_blobs_iter` taking `impl IntoIterator<Item = DigestInfo>`; worker_api_server now chains the two slices with `.iter().copied().chain(...)` — zero allocation, single endpoint Arc<str> alloc preserved. Backward-compat: `register_blobs` is now a thin wrapper. TraceMachina#4/TraceMachina#5 (MAJOR) — Lock-ordering stress was 200 iters with `yield_now()` inside tokio tasks, which under inverted lock order would wedge the ENTIRE tokio worker pool (sync parking_lot waits, not awaits — the inner `tokio::time::timeout(15s)` could never fire). Rewrote to use `std::thread::spawn` + `std::sync::mpsc::recv_timeout(15s)` outer guard so a deadlock surfaces as a clean panic ("AB/BA deadlock detected") within 15s. Bumped to 10_000 iters, dropped `yield_now()`. Mutate-verified: inverting `mirror_blobs`/`mirror_changes` lock order in `snapshot_and_reset_mirror_changes` causes the test to FAIL (not wedge) in exactly 15s. cargo check --all-targets clean. cargo test --features test-utils -p nativelink-store -p nativelink-worker --tests: 14/14 mirror_durability, 3/3 blobs_in_stable_storage_handler, 1/1 download_to_directory_mirror — all green. Mutate-tests: lock-ordering inversion produces correct failure mode within timeout. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

The SUBSCRIBER_SLOT = 0 invariant was the highest-correctness-risk change in this branch (subscribing on N connections delivers each message N times) and had ZERO test coverage per testing-czar MISSING-COVERAGE TraceMachina#3. Refactored `psubscribe` and the subscriber-replay branch in `reconnect` into generic `psubscribe_with` and `reconnect_with` helpers that take an async closure performing the actual SUBSCRIBE-issuing call. The trait impl `RedisManager<ConnectionManager>` wraps these with closures invoking the inherent `ConnectionManager::psubscribe` method (no functional change for production); tests pass closures with structural assertions. This is the minimum-invasive way to test pinning without spawning a real Valkey instance — the inherent method on the concrete ConnectionManager type cannot be reached from MockRedisConnection. Test `psubscribe_pins_to_subscriber_slot_only` sweeps every slot 0..3: holds a `read()` on slot N, races psubscribe_with against a 750ms timer. If N == 0 (SUBSCRIBER_SLOT): must time out (write blocks on held read, proving psubscribe_with tries to acquire slot 0). If N != 0: must complete (slot 0 is independently writable). Closure additionally records the slot uuid passed to it and asserts it equals slot 0's uuid. Added `debug_read_slot()` helper on `StandardRedisManager<C>` (#[doc(hidden)]) so tests can hold per-slot read-locks for the structural deadlock check without exposing `connections` as a pub field. Mutation evidence (verified locally): - SUBSCRIBER_SLOT = 1 → N==0 case completes when it should block AND N==1 case blocks when it should complete; test panics on the FIRST failed iteration with "pinning regression — psubscribe is not actually pinned to slot 0. Got result: Ok(Ok(Ok(())))". - Removing the write-lock from psubscribe_with → all sweeps complete (subscriber-slot iteration was supposed to block); same panic. - Locking ALL slots in psubscribe_with → every iteration blocks; test panics on N==1 with "psubscribe_with must complete when a non-subscriber slot is read-locked". Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Per CLAUDE.md TDD rule and plan B7: tests written FIRST against the spec before any implementation. Currently FAIL with the right reason: error[E0432]: unresolved import `nativelink_store::small_blob_dispatcher` This is exactly what we expect — the module does not exist yet. 8 tests covering: 1. EphemeralServerSidePin insert/remove tracks total_bytes 2. EphemeralServerSidePin cap-exceeded returns ResourceExhausted (per C2) 3. observe_pinned_mirror_ack filters by store_id (per C8 mutation) 4. dispatcher disabled enqueue is no-op (feature-flag inert, plan TraceMachina#3) 5. dispatcher rejects oversize blob (precondition per C9) 6. dispatcher rejects empty store_id (per C11) 7. dispatcher rejects malformed store_id (per C11) 8. SMALL_BLOB_THRESHOLD doc-tied to 16 KiB Per plan B7 the test-author would normally be a separate agent from the implementer (CLAUDE.md TDD step 1). Sub-agents cannot dispatch sub-agents, so the same agent enforces TDD by: 1. Writing tests against THIS PLAN, not against any implementation. 2. Running cargo check; observing E0432 unresolved import. 3. Implementing in the next commit. 4. Running cargo test; verifying pass. 5. Mutation step. Each async test wrapped in `tokio::time::timeout(5s, ...)` per CLAUDE.md "deadlock detector" rule. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

…worker Per CLAUDE.md TDD step 1-2: derive new failing tests for the next implementation step (drainer wiring + worker registration) BEFORE writing the implementation. Three tests added — all RED: - dispatcher_enqueue_delivers_batch_to_registered_worker: asserts a successful enqueue against a registered worker delivers a BatchWriteSmallBlobsRequest over worker_tx AND populates the pin set. Fails: register_pin_set / register_worker don't exist. - dispatcher_zero_window_coalesces_pending_into_single_batch: asserts rapid-fire enqueues coalesce into fewer batches than enqueues per decision TraceMachina#3. Mutation guard for try_recv-drain. - dispatcher_enqueue_for_unregistered_worker_drops_silently: asserts enqueue for an unknown (endpoint, boot_epoch_id, store_id) tuple silently drops + does NOT grow the pin set. Per plan §"Concurrency design" + B4 (boot_epoch_id keying); the post-Bazel-ack fire-and-forget contract requires Ok return. These RED tests drive the next commit (drainer + register hooks). [no-tla-needed: tests-only commit; protocol spec lands with the implementation commit that introduces the drainer]

Implements the drainer half of the Bug A small-CAS peer-mirror dispatcher (#bug-a; task TraceMachina#153) per plan §"Order of operations" step 2. Closes the three RED tests added in commit 36eed40a: * dispatcher_enqueue_delivers_batch_to_registered_worker * dispatcher_zero_window_coalesces_pending_into_single_batch * dispatcher_enqueue_for_unregistered_worker_drops_silently Drainer: per-(endpoint, boot_epoch_id, store_id) bounded mpsc with a spawned drainer task that recv()s the first item then try_recv()-drains pending into the same batch (zero-window coalesce per decision TraceMachina#3), emits a single Update::BatchWriteSmallBlobs over the registered worker_tx UnboundedSender, then loops. Holds NO locks during send (S3 future-regression guard). On worker_tx closed: rolls back pin entries for the dropped batch and exits cleanly. Registration: register_pin_set(store_id, pin_set) called by each FastSlowStore at startup; register_worker(endpoint, boot_epoch_id, worker_tx) called by WorkerApiServer::inner_connect_worker on every connect (per B4: new boot_epoch_id replaces stale entry, dropping its sender, drainer exits via rx.recv() returning None). unregister_worker on disconnect drops pending queues. Mutation steps run for each new test: * try_recv-drain disabled -> coalesce test fails (5 batches not 1) * unregistered-worker early-return replaced with Err -> drop test fails (got Err instead of Ok) * worker_tx.send disabled -> delivery test fails (Elapsed timeout) All three confirmed restored to PASS. Pin set integration: enqueue inserts (digest, data) into the EphemeralServerSidePin BEFORE handing to the drainer queue (so a fast ack cannot race ahead of our pin record); on try_send Full or pin-set cap exceeded, the pin entry is rolled back. Per plan §"Failure modes (general)": post-Bazel-ack fire-and-forget — Err is never propagated back to the caller (always Ok). fast_slow_store: pub fn insert_dispatched_mirror_blob(store_id, digest, data) wrapper around the existing private insert_mirror_blob, so the worker's Update::BatchWriteSmallBlobs handler can call from outside the crate. Carries store_id for future (store_id, digest) keying (B5 BTreeMap refactor follow-up); currently logged-only. specs/SmallBlobDispatcher.tla: TLA+ model of the cross-store routing contract (Option F, decision TraceMachina#6). Two stores (cas + ac) holding the same digest D collide under the bugged regime: an ack carrying only (cas, D) wrongly clears D from BOTH cas + ac pin sets. Auxiliary ackedByServer history lets the safety invariant detect the lost-pin without a temporal property. * Bugged.cfg (FixOptionF=FALSE): NoCrossStoreLeak VIOLATED — TLC finds the 2-step trace within seconds. * Fixed.cfg (FixOptionF=TRUE): NoCrossStoreLeak holds. Per-store binary-search filter (mirrored in code at small_blob_dispatcher.rs EphemeralServerSidePin::observe_pinned_mirror_ack:257) preserves the invariant. State-space constraint (Len queues <= 2) keeps Fixed.cfg under the TLC_TIMEOUT=120s budget; the bug fires within 1 push-per-store cycle so a deeper bound is unnecessary. Per plan §"Order of operations": this commit covers steps 2 (drainer) and the protocol-spec gate. Step 3 (worker handler full impl), step 4 (broadcast wiring at handle_blobs_available), step 5 (FastSlowStore observe_pinned_mirror_ack registration) and step 6 (enqueue at upload completion) are follow-ups.

…e-positive log tests (TraceMachina#172 reviewer) Adds 4 new tests (3 sibling + 1 negative-case) to close the testing-czar HOLD-MAJOR finding that 3 of 4 `error!` sites in the stale-positive logging contract shipped UNPROTECTED: - update_logs_error_for_stale_positive - update_oneshot_logs_error_for_stale_positive - batch_get_part_logs_error_for_stale_positive - get_part_not_found_does_not_log_error_for_never_cached_digest Each test uses an `ErrCodeOnGetStore` fake with a new `set_has_returns_none()` toggle so the test can prime the cache via a real `update_oneshot` (cache + inner both have the digest) and then flip the inner's `has()` to None — recreating the exact stale-positive scenario each `error!` site is meant to surface. Mutation step (per CLAUDE.md TDD step 5): for each new test, the corresponding `error!` block was commented out in source and the test re-run; each FAILED with a specific assertion message naming the path (`update path` / `update_oneshot path` / `batch_get_part`) or the gate (`if removed` for the negative case). Source restored before commit. Step 2 (testing-czar TraceMachina#3): replaced the brittle `format!("{digest}")` log-grep in `get_part_not_found_logs_error_for_stale_positive` with the stable `VALID_HASH` hex constant. The DigestInfo Display impl is not part of the contract this test guards. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

…ewer red-team TraceMachina#3) Previously `handle_bis_chunk` ALWAYS fired the ack after the unpin pass, even when individual digests failed to decode (DigestInfo:: try_from Err). The server then dropped the chunk from its resend buffer → on next ConnectWorker the chunk was never replayed → the failed digests stayed pinned forever. Same outcome as the original TraceMachina#89 bug (unpin-leak), different mechanism, less detectable because the success path "looked" OK. The fix: * `handle_blobs_in_stable_storage` now returns `BisUnpinOutcome { unpinned, failed }`. Today the only failure mode is `DigestInfo::try_from` Err on a malformed proto digest; `unpin_digest` and mirror cleanup are infallible. Returning a struct (not a bare bool) keeps future failure-mode growth (e.g. disk-IO-backed unpin) non-breaking. * `handle_bis_chunk` only invokes `ack_sink` when `outcome.all_succeeded()`. On partial failure, `error!` logs `broadcast_id`, `sequence`, `unpinned`, `failed`, `digest_count` so operators see the unpin-failure rate, and the server's resend buffer keeps the chunk for next-reconnect replay. * Existing tests adapted to the new return type (now read `outcome.unpinned`/`outcome.failed` instead of bare usize). Asymmetric contract coverage (per CLAUDE.md): adds `bis_ack_not_sent_when_unpin_fails` — the over-action sibling of the existing `bis_chunked_full_burst` under-action test. The new test mixes one malformed digest with four well-formed ones in a chunk and asserts: (a) outcome.failed == 1, outcome.unpinned == 4, !all_succeeded(), (b) acks.is_empty() — the SPECIFIC "must NOT emit an ack" assertion message (not a generic `is_empty()`). Wrapped in `tokio::time::timeout(5s)` per the production-composition deadlock-detector convention. Mutation step verified: commenting out the `if outcome.all_succeeded()` gate causes `bis_ack_not_sent_when_unpin_fails` to panic with "must NOT emit an ack when ANY digest in the chunk fails to decode ... Acks emitted: 1" — confirming the test guards the behavior. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

…teHalfGuard + gate fast_slow Err OOM-pressure mitigation per `.claude/audits/186-verifystore-noise-investigation.md`. Server emergency-restarted at 09:05 PDT (RSS hit 75 GB / 80 GB cap); the audit identified these two log classes as the dominant noise feeding mimalloc allocator pressure during phantom-blob bursts. Production rate (2026-05-04 20:37 → 2026-05-05 05:59, ~9h21m): - streaming_blob warn: 7,365 events; ~13K events / 6-min burst at 22:37–22:42. - WriteHalfGuard Drop error: 6,227 events; same burst window, 1.18:1 ratio with the streaming_blob warn — both reports of the SAME upstream NotFound from different layers (populator-side `send_error` vs outer-guard Drop). Per-site reasoning: 1. `streaming_blob.rs::StreamingBlobWriter::send_error` — demote `warn!` → `debug!` when `err.code == Code::NotFound`. NotFound is ALWAYS upstream-caused (slow_store NotFound + EC stale-positive cleanup; TraceMachina#247-class FilesystemStore disk/index divergence). The streaming_blob buffer itself isn't the suspect; logging the same fact 7K times drowns the actual phantom-blob signal. Other codes (Internal, Aborted, Unavailable) still warn — those are genuine producer wedges where the streaming buffer IS the suspect. Folds into the existing benign-vs-suspect split in the Drop arm (full-byte cancellation → debug, partial → warn). 2. `buf_channel.rs::WriteHalfGuard::Drop` — demote `error!` → `debug!` unconditionally. The composability harness (`writer_termination_245_drop_fallback_test.rs`, `waiter_explicit_termination_test.rs`, `fast_slow_store_test.rs::write_half_guard_drop_fallback_*`) is now the load-bearing detector for genuine writer-termination contract violations — production logs are not. Wire-side `Error` message ("buf_channel: writer dropped without commit") is preserved unchanged so remote clients still see a unique, greppable identifier; only the operator-side log level changes. 3. `fast_slow_store.rs::get_part` outer-guard NotFound exit paths — already gated via `commit_with_inner_miss_gate` in committed code (commits `009feab6` TraceMachina#171 sibling-bug audit, `ce468b18` parallel-path coverage, `9c02ce41` TraceMachina#171 fix, `b19a2ccf` TraceMachina#245). Verified all 9 NotFound exit paths in `get_part` (mirror_blobs size mismatch, in_flight size mismatch, local_only_reads, slow-store fallback, populator terminal-state NotFound branch, populate-then-fast-evicted retry, sliding-window-evicted fallback, populator-caller streaming-reader Err NotFound, waiter streaming-reader Err) — each routes through `commit_with_inner_miss_gate` or honors the `INNER_MISS_NO_TERMINATE` task-local explicitly. No code changes needed for site TraceMachina#3 in this commit. NOT covered here (out of audit scope, follow-up needed): - The new `Code::Unavailable` early-return at `fast_slow_store.rs:4143` (commit `ace7aba6` TraceMachina#252 chunked-pin partial-miss) does NOT terminate the outer guard. Drop fires + synthesizes an Internal error to the paired buf_channel reader, masking the Unavailable signal at the channel level. The function caller still observes Unavailable; the channel reader observes Internal. Noise impact is small now that TraceMachina#2 is debug, but the code-mismatch is a TraceMachina#252 contract gap to follow up separately. Tests: no existing test asserts on the WARN/ERROR level of either log; `waiter_explicit_termination_test.rs:308` asserts on the message body (`!logs_contain("WriteHalfGuard fired Drop fallback")`) and continues to guard correctly because the gate suppresses the Drop entirely (regardless of log level). Build: `cargo build --release --features quic,pprof,chunked_fast_slow` — 3m23s cold, no errors. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

… nits Three small follow-ups from the second reviewer pass on the v2 redesign (`88393c2c`): Fix A (perf-optimizer NIT-3): drop `async fn` from `enqueue_with_sender`. The body had zero `.await` points so the async-state-machine overhead was paid at every fan-out per worker for no benefit. Two callers updated: the fan-out loop in `dispatch_to_all_workers_inner` and the public `enqueue` wrapper. Fix B (security-reviewer LOW): hoist the `worker_cas_endpoint` spoof-check audit log from inside the field-16 (`pinned_mirror_entries`) block to function entry. The notification's `worker_cas_endpoint` is consumed at THREE sinks below: pinned_mirror_entries (Fix-K block), `record_mirror_capacity`, and the consolidated `register_blobs_iter` / `evict_blobs` / `remove_endpoint` block. Sink TraceMachina#3's `is_full_snapshot` × `remove_endpoint` is the load-bearing concern: a malicious worker can spoof another peer's endpoint to WIPE that peer's locality_map entries. Function-entry placement covers all three sinks. Detective only — still processes the report (benign misconfig recoverable; rejecting would prevent legit endpoint roll). Fix C (red-team TraceMachina#1): update `schedule_dispatch_to_all_workers` doc-comment to reflect Fix 4's removal of the dispatcher-internal size gate. Doc had described "step 2: data.len() > SMALL_BLOB_THRESHOLD → return" but Fix 4 removed that gate (producer hooks own the size check; the public `enqueue` async API still gates as defense-in-depth). The stale doc would form the wrong mental model for the next reviewer. Tests: 13/13 small_blob_dispatch_integration_test pass; 5/5 small_blob_dispatcher unit tests pass. Release build green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

…raceMachina#2 ordering (TraceMachina#279 reviewer fixups) Address three reviewer findings on commit ee0ca1ea (the boot-epoch wipe regression suite): 1. **code-reviewer FIXES-REQUIRED — test 3 race** `boot_epoch_wipe_does_not_block_repopulation` dropped tx1/stream1 BEFORE the reconnect, allowing the disconnect-cleanup at `worker_api_server.rs:912` to race the registration-path wipe at `:531`. If `:912` ran first, it called `state.remove(&endpoint)` — the reconnect's `:531` then saw `prev = None`, `needs_wipe = false`, and the `:531` wipe DID NOT FIRE. Under that interleaving, commenting out `:531` would still leave the test green via the `:912`-already-cleared path. Fix: hold tx1/stream1 alive across the reconnect, mirroring tests TraceMachina#1 and TraceMachina#2's mechanic. The wipe MUST now come from `:531` exclusively. 2. **testing-czar Important TraceMachina#1 — test 2 assertion ordering** `boot_epoch_wipe_of_endpoint_a_does_not_clear_endpoint_b` asserted A drained BEFORE B survives. Under the "wipe missing" mutation the under-action half (A drained) timed out FIRST and the over-action half (B survives) was never reached — a future reader running only that mutation would conclude the over-action contract is redundant with `boot_epoch_different_wipes_ac_registry`. Fix: invert the order so B-survives is asserted first. Both halves now exercise their respective contracts under their own mutations: under-action mutation ("comment out wipe call") trips A's drain timeout; over-action mutation ("wipe ALL endpoints") trips B's survival assertion. 3. **testing-czar Important TraceMachina#2 — test 3 polling predicate** `send_ac_blobs_and_wait` polls for `len() == expected`. With the OLD stream alive (per fix TraceMachina#1) the registry holds d_old (len=1) until the wipe fires; under the wipe-missing mutation the registry would hold d_old=1 while the d_new tick is in flight, and a `len() == 1` predicate would silently exit with `snap = [d_old]` then catch the bug only via the `assert_eq!(snap[0].1, d_new)` (a generic diff, not a bespoke message). Fix: in test TraceMachina#3 send the post-reconnect `BlobsAvailable` directly (bypassing `send_ac_blobs_and_wait`'s ambiguous predicate) and poll for `d_new`'s PRESENCE specifically. The bespoke message is now reachable on every mutation interleaving. Mutation step verified (post-fixup): * `worker_api_server.rs:531` commented out → all 3 boot-epoch tests RED-fail with bespoke messages: - `boot_epoch_different_wipes_ac_registry`: "boot-epoch wipe MUST clear AC pin registry — sibling-of-TraceMachina#141 regression: Elapsed(())" - `boot_epoch_wipe_of_endpoint_a_does_not_clear_endpoint_b`: "boot-epoch wipe of A MUST clear A's AC pin entries — under-action half: Elapsed(())" - `boot_epoch_wipe_does_not_block_repopulation`: "post-wipe registry MUST hold ONLY d_new — stale (pre-wipe) digest leaked through wipe" * `:531` replaced with "wipe ALL endpoints" loop → only test 2's over-action half red-fails: "boot-epoch wipe of A MUST NOT clear B — over-action: cross-endpoint wipe leaked" * Restored: 8/8 green in 0.25s. Module-level `//!` doc-block updated to enumerate tests 5–7 with the asymmetric-coverage framing and an explicit "wipe ALL endpoints" mutation entry that surfaces the over-action contract. Reviewer reports addressed: - .claude/reviews/279-drain-hardening-ee0ca1ea/code-review.md (MAJOR) - .claude/reviews/279-drain-hardening-ee0ca1ea/testing.md (Important TraceMachina#1 + TraceMachina#2) - .claude/reviews/279-drain-hardening-ee0ca1ea/red-team.md (Q5/Q7) - .claude/reviews/279-drain-hardening-ee0ca1ea/distributed-systems.md (MINOR-1 mutation-step verification) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

…TraceMachina#334 fix-AB fixup) Reviewer findings: 1. **code-reviewer H1 + dsr M2 + testing-czar**: new variant `BackpressureSignal::SlowWritesAtCapacity` was not added to the existing classifier-iteration tests (`looks_like_dead_channel_respects_backpressure_signal`, `new_fixup_variants_roundtrip_and_classify`). A future change to either helper could silently regress for the new variant. Add to both iteration lists. 2. **red-team blind spot**: `SLOW_WRITES_AT_CAPACITY_RETRY_MS = 100` had no rationale comment for the 4× difference vs `MEMORY_STORE_BACKPRESSURE_RETRY_MS = 25`. MemoryStore eviction resolves on single-digit-ms timescale; slow-tier ack on hundreds-of-ms. Document. 3. **red-team TraceMachina#3 + dsr M4**: 8 GiB server cap default needed an explicit per-host rationale and worst-case overshoot bound. Document both. 4. **code-reviewer C1 + dsr M4**: worker `slow_writes_in_flight_max_bytes: 0` lacked any explanatory comment — looked like a copy-paste oversight. Per parent decision: keep `: 0` on worker (worker's slow tier is GrpcStore→server, bounded by upstream h2 windowing + server-side cap). The 2026-05-08 OOM was on buildcache, not on workers. Add explicit comment justifying the choice at both worker FSS construction sites.

…es (TraceMachina#334 bundle fixup TraceMachina#3) Code-reviewer MA-3: production deploys with `chunked_fast_slow` enabled, but `.github/workflows/native-cargo.yaml` never runs the chunked_fast_slow-gated test bins. Three tests in this bundle (and ten others pre-existing) silently skip in CI today. Adds a CI step `Test chunked_fast_slow feature` analogous to the existing failpoints carve-out. Combines `chunked_fast_slow` AND `test-utils` so tests requiring both (e.g. `fast_slow_str_key_skips_chunked_dispatch_test`, `writer_termination_245_drop_fallback_test`, `chunked_filesystem_serialization_test`) all run in one pass. Also adds `[[test]] required-features = ["chunked_fast_slow"]` for `memory_store_backpressure_eviction_test` so cargo silently skips it from default `cargo test --all` (matches the existing pattern for `memory_store_backpressure_emission_test`). The other two new test files (`memory_store_pin_durability_test`, `ac_pin_durability_test`) do not depend on chunked_fast_slow APIs and run fine without the feature, so no required-features declaration for them. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

…d for dispatch_res typed signal (TraceMachina#334 bundle fixup TraceMachina#7 — red-team TraceMachina#3) Red-team TraceMachina#3 worried that Fix A's chunked-path sibling at `fast_slow_store.rs:1166-1194` only checks `fast_res` for typed BackpressureSignal preservation, leaving `dispatch_res` exposed to masking by `data_res` Internal when the dispatcher rejects-at-admit and drops `chunk_rx`. Investigation result: the existing `match dispatch_res { Err(err) => return Err(err) }` arm at line ~1227-1246 ALREADY preserves the typed err to the caller. The bug-precondition race (data_res becomes Err from chunk_rx-dropped wrapper before dispatch_res is observed) does NOT fire in any reproducible test scenario — the data-stream future races to EOF before observing the dropped rx, so `data_res` is Ok and the existing dispatch arm catches the typed signal. Verified via attempted-mutation: even with a pre-data_res `dispatch_res_carries_typed_backpressure` guard ENABLED, the existing arm catches it first (because data_res is consistently Ok in the test scenarios). The proposed extra guard would be dead code. Captured the investigation in the doc-comment at the `fast_res_carries_typed_backpressure` site so future readers (and sibling-bug audits) understand why only `fast_res` is checked explicitly — and what a future regression that broke this assumption would look like. Added regression test `fast_slow_str_key_skips_chunked_dispatch_test:: fix_a_chunked_sibling_dispatch_typed_signal_preserved` that wires up a `RejectingDispatcher` (synthetic `Code::ResourceExhausted + BackpressureSignal::PerBlobMpscFull` rejection at admit-time, drops chunk_rx immediately) and asserts the typed signal reaches the caller via the existing dispatch arm. If a future change ever causes data_res to be reliably Err-before-dispatch-arm in this case, the test will red-fail and alert that the symmetric pre-data_res guard is now needed. Documented dispatcher emit sites in the doc-comment for traceability in future audits. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

…na#334 bundle fixup CR-2) Fixup TraceMachina#3 (37f30475 "test chunked_fast_slow feature in CI") added `--features nativelink-worker/chunked_fast_slow` to the chunked-test step. `nativelink-worker/Cargo.toml` does not define a `chunked_fast_slow` feature; only `nativelink-store` and `nativelink-service` do. Cargo rejects the flag with "package does not contain this feature." Fix: drop the offending flag. The remaining `nativelink-store/chunked_fast_slow` and `nativelink-service/chunked_fast_slow` flags correctly enable the feature in the two crates that define it; `nativelink-worker` inherits it transitively via its store/service deps. Verified: `cargo check -p nativelink-store -p nativelink-worker -p nativelink-service --features nativelink-store/chunked_fast_slow --features nativelink-store/test-utils --features nativelink-worker/test-utils --features nativelink-service/chunked_fast_slow --features nativelink-service/test-utils --tests` no longer reports the "does not contain this feature" error. (Remaining errors are MA-1: missing FastSlowSpec fields, fixed in next commit.) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

…Machina#2+TraceMachina#3+TraceMachina#5+TraceMachina#9+TraceMachina#11 (BLOCK-class) Addresses 6 of 11 reviewer findings on the BlobsAvailable chunking landing (PR3 of TraceMachina#80). Each fix is annotated with the originating reviewer + finding ID. ACCUMULATOR (nativelink-service/src/blobs_available_accumulator.rs): * Fix TraceMachina#1 (invariant-prover BLOCK + dsr + code-reviewer): Add sequence-completeness gate at terminal commit. Before this fix, a terminal chunk arriving with gaps in `seen_sequences` committed a strict-subset partial — the half-applied-snapshot bug class the Path A semantics are meant to prevent. The new `BroadcastAccumulator::sequences_contiguous()` checks `seen_sequences == (1 << (max_seen_sequence + 1)) - 1`. On rejection: drop the partial accumulator, increment `dropped_incomplete_sequence` counter, warn-log, return None. Worker re-broadcasts on next tick. * Fix TraceMachina#3 (red-team BLOCK + testing-czar IMP-4 + assumption-auditor REFUTED-1): Header scalars (worker_cas_endpoint, cpu_load_pct, mirror_used_bytes, etc.) now stored as `Option<HeaderScalars>`, populated only when `sequence == 0` arrives. If the terminal commits without seq=0 ever arriving, refuse (subsumed by Fix TraceMachina#1 in practice but documented separately). `BroadcastAccumulator::new()` no longer reads scalars from "first chunk to arrive" — that path silently lost headers under out-of-order delivery. * Fix TraceMachina#2 (dsr BLOCK-1 + red-team alt 1): Raise MAX_SEQUENCES from 64 → 256 (bitset switched from u64 to u128-pair) AND MAX_ACCUMULATED_ENTRIES_PER_CONN from 200_000 → 1_000_000 to match the AC-pin per-endpoint cap of 1M. Worker FSS sizing 50K- 200K + AC pins + cached_directory_digests easily reaches 1M total; the prior 200K cap silently dropped legitimate worker traffic. * Fix TraceMachina#11 (security M1 + assumption-auditor REFUTED-2): - Pre-merge per-conn entries-cap projection: cap fires BEFORE `acc.merge(chunk)` so peak accumulator memory is bounded by `MAX_ACCUMULATED_ENTRIES_PER_CONN + MAX_ENTRIES_PER_CHUNK`, not `+ (one hostile chunk's worth)`. - Per-chunk entries cap (`MAX_ENTRIES_PER_CHUNK = 8192`): rejects oversized chunks before any per-broadcast state is touched. - Falsification test `entries_cap_drops_partial_at_2x_threshold`: drives 2× the cap of synthetic data, asserts cap eviction fires AT threshold (not far before, not after OOM). * Fix TraceMachina#9 (dsr MAJOR-1): Per-reason `chunks_dropped_*` counters exposed via `pub drop_counts: ChunkDropCounts`. 9 reasons: token_zero, token_mismatch, per_conn_broadcasts_cap, per_conn_entries_cap, per_chunk_entries_cap, sequence_cap, validation_other, incomplete_sequence, missing_chunk_zero. Each warn-log carries the reason as a structured field. Test `drop_counters_cover_all_documented_reasons` exercises 7 of 9 paths (the two missing-chunk-zero paths are structurally unreachable when the completeness gate fires first; counter exists for defense-in-depth). * Fix TraceMachina#5 (invariant-prover + assumption-auditor): Replaced `out_of_order_sequence_within_window_tolerated` with `out_of_order_sequence_with_gap_rejected`. The old test ENSHRINED THE BUG by asserting `is_some()` on a commit that skipped sequence 1 — the new test asserts `is_none()` with the bespoke message "sequence-completeness gate must reject incomplete commit; saw is_some() instead — Fix TraceMachina#1 (invariant-prover BLOCK) regression". * Removed gratuitous `total_accumulated: AtomicUsize` (code-reviewer TraceMachina#4): always accessed under the inner mutex; reduced to a plain `usize` field on `AccumulatorInner`. Adds a `total_accumulated()` test helper. CHUNKER (nativelink-util/src/blobs_available_chunking.rs): * Fix TraceMachina#2 (dsr BLOCK-1): Added `BLOBS_AVAILABLE_MAX_CHUNKS_PER_BROADCAST = 256` MUST-equal-the- server-MAX_SEQUENCES constant. Changed `chunk_blobs_available()` signature from `Vec<...>` to `Result<Vec<...>, &'static str>`. Returns Err if a broadcast would emit > 256 chunks. New tests: `chunker_errors_when_exceeding_max_chunks_per_broadcast` and `chunker_allows_exact_max_chunks_per_broadcast` (boundary). WORKER (nativelink-worker/src/local_worker.rs): * `send_periodic_blobs_available` updated to handle the chunker's new Result return: on Err, warn-log and return Ok(()) (skip the tick; next tick retries). Better than emitting chunks the server silently discards. INVARIANTS WALKED: * terminal-chunk-commits ⇒ seen_sequences forms contiguous prefix [0, max_seen_sequence] (Fix TraceMachina#1 — re-established at blobs_available_accumulator.rs `sequences_contiguous()`). * per-conn memory bound: `total_accumulated <= MAX_ACCUMULATED_ENTRIES_PER_CONN + MAX_ENTRIES_PER_CHUNK` even with a hostile chunk (Fix TraceMachina#11 — re-established at pre-merge cap-check). * chunker output count <= BLOBS_AVAILABLE_MAX_CHUNKS_PER_BROADCAST = server's MAX_SEQUENCES (Fix TraceMachina#2 — enforced at chunker producer-side guard). TESTS: * nativelink-util chunker: 11/11 pass (added 2 tests). * nativelink-service accumulator: 19/19 pass (added 7 tests, rewrote 1, removed 0). Reviewers cited: invariant-prover BLOCK, distributed-systems-reviewer BLOCK-1 + MAJOR-1, red-team blind-spot-1 + alt-1 + alt-2, testing-czar IMP-2 + IMP-3 + IMP-4, code-reviewer Critical TraceMachina#1 + suggestion TraceMachina#4, assumption-auditor REFUTED-1 + REFUTED-2, security-reviewer M1 + M2. Composite invariants this fix interacts with: Path A (deferred-wipe) commit semantics, mirror_blobs ≥2-replica durability (untouched — BlobsAvailable is locality hint only), worker_instance_token contract from TraceMachina#97/TraceMachina#98. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

…ceMachina#100) Reframe of the keyspace-notification dispatcher from "closes the stale-positive class for ExistenceCacheStore wrapping RedisStore" to **defensive infrastructure for Redis (Valkey) keyspace eviction notifications. Currently dormant in production (Valkey on buildcache has had zero evictions in this server's lifetime; deployed for forward-compat against future workloads stressing the eviction path).** The original framing overclaimed; the design doc §0 already established the zero-eviction baseline. Addresses 6 BLOCKER findings from the 681ae250 reviewer pass (code-reviewer / perf-optimizer / security-reviewer / red-team / distributed-systems-reviewer): - BLOCKER 1 (silent init failure): convert `RedisStore::new_standard` to perform CONFIG GET / SET / 3× PSUBSCRIBE eagerly, before the Arc is handed back. Failure → `Err` from the constructor → server fails to start with a clear error. `register_item_callback` becomes a sync fast-path-only operation (no background spawn that swallows failures); ECS panics loudly via the `.expect()` if the dispatcher is not wired. - BLOCKER 2 (consume-before-validate `take()`): defer `subscriber_channel.lock().take()` to the LAST step of eager init, so any failure leaves the channel still in `Mutex<Option<...>>`. The misleading "already consumed by SchedulerSubscriptionManager" error on retries is now unreachable through legitimate paths. - BLOCKER 3 (`StoreKey::Str` over-action): `parse_keyspace_payload` drops non-digest payloads at the parser. Admin `DEL cas:debug-foo` and any non-digest scheduler/index keys never propagate to ECS or other ItemCallback consumers. The Str fallback was perf+log-leak waste, not a panic risk (`StoreKey::into_digest` blake3-hashes), but guaranteed-no-op cache misses + foreign-tenant info leaks are still worth filtering. - BLOCKER 4 (CONFIG SET non-persistent across Valkey restart): `StandardRedisManager::reconnect_with` re-issues `CONFIG SET notify-keyspace-events <flags>` on every subscriber-slot reconnect before replaying PSUBSCRIBE. Without this, a Valkey restart silently reverts the in-memory CONFIG to whatever `valkey.conf` says (typically empty), the dispatcher keeps running, and ECS retains stale positives indefinitely. Operator should ALSO persist `notify-keyspace-events Eegx` in `valkey.conf` for resiliency across NativeLink restarts. - BLOCKER 5 (shared subscriber_channel): `set_spec_defaults` rejects the `enable_keyspace_notifications=true` + `experimental_pub_sub_channel.is_some()` collision at construction time with `Code::FailedPrecondition`. Both consumers want the single Redis push-sender; the loser silently degrades to no-op invalidation today. Fan-out task (per design §1.1) remains future work. - BLOCKER 6 (CI gating): wire `--features redis-integration-tests` into `.github/workflows/native-cargo.yaml` with a Linux-only `valkey-server` install step. Default `cargo test` on dev boxes remains a no-op for these tests (the test bin is `#![cfg(...)]`-gated out, not silently skipped). Hardening (security-reviewer MEDIUMs): - `keyspace_notifications_db: i64` → `u8` (Redis db indices are non-negative single bytes in practice). - `set_spec_defaults` cross-checks `keyspace_notifications_db` against the URL-embedded db; mismatch is `Err` instead of silent listen-on-wrong-db. - Empty `key_prefix` with `enable_keyspace_notifications=true` warns loudly at construction (foreign-tenant DELs would otherwise dispatch into our ItemCallback consumers). Tests: - New regression `non_digest_key_under_prefix_does_not_fire_callback` (BLOCKER 3): asserts a digest-shaped positive control reaches the callback while a same-prefix non-digest DEL is dropped at the parser. - New regression `keyspace_and_scheduler_collision_refused_at_construction` (BLOCKER 5): `RedisStore::new_standard` returns `Err` when both flags are set; bespoke message verifies the rejection text. - Existing `foreign_prefix_does_not_fire_callback` updated to use a digest-shaped positive control (the previous `cas:positive-control` string-key would now be filtered by BLOCKER 3 — the test relied on that pre-fix Str fallback). - All 8 integration tests pass against real `valkey-server` v9.0.4 with the `redis-integration-tests` feature enabled. Mutation-step verification (CLAUDE.md TDD rule 5): - BLOCKER 1: comment out `store.init_keyspace_dispatcher_eager()` in `new_standard` → integration test TraceMachina#6 (`existence_cache_drops_positive_after_redis_eviction`) panics with `register_item_callback` returning Err and ECS `.expect()` firing. - BLOCKER 3: revert `parse_keyspace_payload` to fall through to `StoreKey::Str(...)` → BLOCKER 3 regression FAILED as expected (Str leaks into captured set). - BLOCKER 5: gate the collision check on `if false` → BLOCKER 5 regression FAILED as expected (no Err returned from new_standard). All three mutations reverted post-verification. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --- Squash-in (2026-05-10 — 9-reviewer cadre fix-up follow-up): The original 7830d435 WIP commit by `parent <parent@local>` flipped `default_enable_keyspace_notifications: true → false`. Two reviewers (code-reviewer + DSR) caught that the flip boot-panics the production CAS chain `cas_INNER (ExistenceCacheStore) → SizePartitioning → FastSlow → REDIS_CAS_SMALL_STORE`: ECS::new_with_time `.expect()`s `register_item_callback` to succeed, FastSlow/SizePartitioning propagate the Err, and `RedisStore::register_item_callback` returns `Code::FailedPrecondition` whenever `enable_keyspace_notifications=false` — putting nativelink.service into a systemd restart loop. Production Valkey actually grants `+@all` (verified via `valkey-cli ACL LIST`), so the original `true` default is correct. User authorized Option 1 (revert the default flip). The bool flip is reverted; the genuinely-useful redis_store.rs additions are folded in here (where the eager-init they support already lives): - `subscriber_channel` and `keyspace_dispatcher_tx` get explicit `UNBOUNDED-OK` justifications per the CLAUDE.md unbounded-buffer rule (DSR M2 + perf-optimizer M1 + code-reviewer S1). - New `callbacks_registered_total: AtomicU64` counter, bumped on successful `register_item_callback`, exported via metrics. - Inert-dispatcher 60s startup `warn!` (red-team TraceMachina#3 from the cadre): if `enable_keyspace_notifications=true` but no callback registers within 60s, log a single warn so the operator sees the dispatcher firing into a void (e.g. AC-only stores not yet wrapped in ECS). Single-shot; uses `Arc::downgrade` so it doesn't keep the store alive. Docstring rewritten to keep `Default: true` while tightening operator guidance: hardened-cluster operators (no `+config` ACL) MUST set `enable_keyspace_notifications=false` AND remove any `ExistenceCacheStore` wrapping the `RedisStore`, or `ECS::new_with_time` panics → restart loop. Hardened-cluster ACL operators (no `+config` grant) handled separately: tracked as TraceMachina#379-pending — a config-validator that fails-fast at startup when `enable_keyspace_notifications=true` is wrapped by ECS but the runtime CONFIG GET fails would let operators see the misconfiguration in their dev-loop instead of in the systemd restart loop. See TraceMachina#378 for the follow-up minor-cleanup bundle from the same cadre (security MINOR-1/2: debug-log info-leak + cfg-gating `set_spec_defaults_for_test`; code-simplifier MINOR-1/2/3: duplicated default-line / `.clone()` / `raw_conn` extraction; rust-crate NIT-1/2: import ordering + safe indexing).

…t honesty Addresses 8 MAJOR findings from the 3-reviewer cadre (distributed-systems, invariant-prover, red-team) on commit 7e27c61e. Specs-only diff — no production source touched. REVISIONS TO StreamingBlobReadDuringWrite.tla R1 (DS-MAJOR-1, red-team-MAJOR-2): two-seam split. Replaced single boolean `producerErrPropagated` with `writerTerminalErr` (Seam A, `StreamingBlobInner::terminal`) + `seamBPropagated` (Seam B, `state.maybe_get_part_result` consumed by `inner_read::consume_ok_eof`). New action `SeamBObservesProducerErr` fires non-deterministically under `WF_vars` fairness, modeling the per-RPC tokio::select! arm-ordering race at `bytestream_server.rs:1726-1867` that the TraceMachina#500 commit message acknowledged as "addresses one of multiple candidate mechanisms". Consequence: strong "no Silent* under post-fix" is no longer a state invariant — leads-to liveness `PostFixErrPropagationReachesReaderEventually` is the post-revision safety-equivalent claim. R2 (red-team-MAJOR-3): replaced tautological `BytesWrittenMatchesChunkCount` (true by construction of the spec's single atomic writer action) with `BytesWrittenBoundedByChunkCount` (structural bound). The production two-AtomicU64 race is documented as an outstanding gap (Phase 5 TraceMachina#6). R3 (red-team-MAJOR-5): rewrote the CASE→IF/ELSE comment to name the actual spec issue (the original CASE was not total across `terminal` domain because TLC's evaluator does not propagate caller preconditions). No longer frames a spec bug as a TLC quirk. R7 (invariant-prover-NIT-1): dropped unused `Sequences` from EXTENDS. REVISIONS TO CFG FILES R4 (invariant-prover-MAJOR-1): new file `StreamingBlobReadDuringWriteBothOffBugged.cfg` covers the `ShortShieldOn=FALSE × SilentZeroPropOn=FALSE` cross-product cell that was missing pre-revision. Gate-classified via the `*Bugged.cfg` suffix. TLC reaches violation at depth 4, 38 states. R5 (red-team-MAJOR-4): all three Bugged cfgs now list `WriterEventuallyTerminates` as a PROPERTY. Without this, a future regression that masks the safety bug while introducing a lost-wakeup would CI-pass. R6: Fixed.cfg and informational 3r3c/4r2c cfgs updated to new invariant list (the strong "no silent" forms are NOT listed; the conditional `*IfSeamBFired` forms are kept in the spec as documentation only). REVISIONS TO ChunkRaceReadersBuggedH3.tla / Fixed.cfg R8 (invariant-prover-MAJOR-2): added 5 structural invariants (`CommitDoneFlagSticksTrue`, `RunnerWriterAssignmentDiscipline`, `WriterDoneImpliesCommitDone`, `ChunksInFlightDisjointFromPresent`, `ReaderTerminalHasObservation`) to the H3 spec. Updated Fixed.cfg to exercise all 6 invariants (was TypeOK-only — degenerate per the reviewer's MAJOR-2). Documentation explicitly names the stopgap nature; the proper fix is the `AllowEvictingMapInsert` refactor (Phase 5 TraceMachina#5). R9 (DS-MINOR-1): corrected stale citation `chunked_write_handler.rs:3997+` → `:4299` in ChunkRaceReadersV2.tla and the audit doc. REVISIONS TO AUDIT DOC R10 (red-team audit-overclaim): rewrote the "19/19 PASS / Layer 1 OK" framing as self-consistency of an author-encoded spec, NOT third-party validation. Added "WHAT 'Layer 1 OK' DOES AND DOES NOT MEAN" paragraph enumerating out-of-scope hazards (per-RPC race, sliding-window eviction, multi-blob, gRPC cancellation, chunked-WC compose seam, atomics-ordering). R11 (invariant-prover-MINOR-2): corrected Phase 4 trace-shape for `_BuggedSilentZero.cfg` row. R12 (red-team-MAJOR-7): Phase 5 gaps strengthened — TraceMachina#3 notes the per-RPC race is now PARTIALLY modeled by SeamBObservesProducerErr; new TraceMachina#6 names the atomics-ordering hazard. R13: added Phase 6 — Revision section recording all revisions, what remains DEFERRED with rationale, and a per-revision falsifiability check. VERIFICATION | Spec.cfg | State count | Outcome | | StreamingBlobReadDuringWriteFixed.cfg | 1,317 | clean | | StreamingBlobReadDuringWriteBugged.cfg | 24 | violated | | StreamingBlobReadDuringWrite_BuggedSilentZero | 32 | violated | | StreamingBlobReadDuringWriteBothOffBugged (NEW) | 38 | violated | | StreamingBlobReadDuringWrite_1w_3r_3c.cfg | 29,836 | clean | | StreamingBlobReadDuringWrite_1w_4r_2c.cfg | 127,929 | clean | | ChunkRaceReadersBuggedH3Fixed.cfg (revised) | 199 | clean | | ChunkRaceReadersBuggedH3Bugged.cfg | 67 | violated | Each Bugged cfg violates `NoSilentZeroToReader` for the SPECIFIC named trace shape (per CLAUDE.md "Tests"). Each Fixed cfg runs clean against its (now reviewer-honest) invariant list. INVARIANT WALK (per CLAUDE.md "Invariant-walk in every dispatch prompt") - Invariant being violated: the original spec's strong "no Silent* under post-fix" safety claim was OPTIMISTIC vs production (the per-RPC race window the TraceMachina#500 commit explicitly acknowledged as not fully closed). - Mechanism that violates it: `bytestream_server.rs:1726-1867` — `consume_fut` returning `Ok(empty)` can win the select arm before `get_part_fut` resolves with Err, leading to the silent-zero shape for THIS poll. - Mechanism that re-establishes it after the revision: the spec now models the race explicitly via `SeamBObservesProducerErr` + WF fairness; the safety form is replaced by a liveness leads-to that IS provable under fairness. - Composite invariants this revision interacts with: the read-side observation contract (NoSilent*, NoCorrupt) × the writer's terminal- state classifier × the per-RPC seam-B propagation timing. - Test that proves the revision re-establishes the invariant: `StreamingBlobReadDuringWriteFixed.cfg` runs clean against its post-revision invariant list AND `PostFixErrPropagationReachesReaderEventually` leads-to holds; the new `BothOffBugged.cfg` red-fails on `NoSilentZeroToReader`, confirming the composite-failure cell is exercised. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

…at the CQE reap point Invariant being violated: a latency metric must attribute time to the layer that spent it. writev_ms (chunked_writer.rs warn) spanned SQE-submit → future-RESUME, silently charging tokio dispatch time (eventfd → epoll → io-driver → poller → waker → poll) to "disk" — the mechanism behind months of storage misdiagnosis (task TraceMachina#3 reframe, .claude/audits/lifetime-io-p99-reframe-2026-06-09.md). Mechanism that violates it: no timestamp existed at CQE consumption. Mechanism that re-establishes it: tokio-epoll-uring fork (branch cqe-reap-timestamp @ 0a507ec) records Instant::now() in process_completion before the slot write/waker fire and threads it through UseForOpOutput; chunked_writer computes cqe_kernel_ms (submit→reap) + cqe_dispatch_ms (reap→resume) on the existing warn. writev_ms itself unchanged (baseline continuity); decomposition is algebraically exact (writev_ms ≡ kernel + dispatch). Test that proves it: fork writev_cqe_reap_timestamp_dispatch_window_guard (manual-poll over a full pipe; mutation 'capture at poll-time' red-fails "dispatch component lost") + ordering/decomposition test + T10 b1_writev_warn_contains_cqe_split_fields (field-drop mutation red-fails). Composite invariants: none behavioral — observability-only + fork internal API shape (writev returns 3-tuple; both callers updated, batch/linked ops discard explicitly). Behavior changes: - LOG SHAPE: legacy warn key dispatch_ms renamed pre_sqe_ms (it measured pre-SQE enqueue+submit time; collided with cqe_dispatch_ms semantics). Grep/dashboards must migrate. All other fields unchanged. - fork API: SystemHandle::writev -> (F, Instant, Result); per-CQE Instant::now() in poller (perf-blessed: ~25ns/CQE = 0.00125% CPU ceiling; below W-cell bench noise floor). Verification note: the original implementer's dispatch test was proven NON-GUARDING by parent mutation testing (park fired after timestamp consumption) and was replaced; T10 had never been compiled (missing io-uring feature) — both now run + mutation-verified. Fork standalone test binary made buildable (rt-multi-thread + parking_lot dev features; workspace Cargo.lock unchanged). Reviewers (records .claude/reviews/cqe-split-921cc9ac/): code-reviewer APPROVE, perf-optimizer APPROVE (no bench required for this item), assumption-auditor SHIP (6/6 incl. independent confirmation of the non-guarding-test analysis), testing-czar no-BLOCK (name-swap value assertion → test-debt TraceMachina#4). All findings applied pre-merge; suites green post-findings (/tmp/post-findings2-1781054852.log). Bench gate: chunked writev IS W-cell-covered; per repo rule a data_plane_bench run accompanies the DEPLOY (quiet-window, bundled with 69cd058) rather than this local merge — production busy, and the only hot-path delta is the perf-blessed per-CQE clock read. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

…rite Make the worker mirror a HARD ack-gate at the SERVICE layer: no CAS write is acked with fewer than 2 replicas, and the 2nd-replica confirmation runs CONCURRENTLY with the local store write (target ack latency = max(local, confirm), not local + confirm). Invariant being violated: a CAS write was acked to Bazel after only the volatile MemoryStore fast-tier write, with the 2nd replica (worker mirror / slow tier) placed fire-and-forget — so a crash between ack and async-replica landed lost the only durable copy (sole-replica window). Mechanism that violated it: batch handler sequenced store-then-mirror and the mirror was a detached `background_spawn!` (cas_server.rs:156/513, ungated ack at :550); the bytestream chunked tee mirror handle was dropped fire-and-forget (`drop(mirror_handle)`), and the oneshot path mirrored fire-and-forget after the write. Mechanism that re-establishes it after the fix: - worker_proxy_store.rs `ack_gated_write` drives `local_write` ‖ the 2nd-replica confirm under one `tokio::join!` and returns Ok ONLY on local-Ok AND confirmed-2nd-replica (Confirmed / NoPeers→slow-Ok), else Err. - `mirror_and_confirm_data` backpressures on the per-worker permit (`acquire().await`, not the try_acquire-drop), retries across every healthy endpoint, returns Confirmed / NoPeers / Failed; no per-RPC timeout. - bytestream chunked `inner_write` now AWAITs the tee task's `MirrorConfirmOutcome` and gates via `confirm_chunked_replica` (re-drives from the pinned fast-tier copy on a tee miss; awaits the slow tier on NoPeers). - small blobs (≤ SMALL_BLOB_THRESHOLD) confirm via the SLOW TIER (Redis SMALL_CAS_CACHED) — `confirm_via_slow_store` routed to the correct SizePartitioning arm via the live `partition_size()` — decoupling small-blob acks from worker reachability (the SmallBlobDispatcher still fans out for read-locality, fire-and-forget). Composite invariants this fix interacts with: - admission×eviction×pin: the mirror sources from in-hand `Bytes` (batch / oneshot) or the tee reader (chunked); the chunked re-drive reads the just-written FAST-tier copy, which FastSlowStore pins until BIS ack (`fast_slow_store.rs:5270/5727` `pin_digests`) — gate ⇒ pin covers the mirror/re-drive window. No new admission gate added. - steady-state SLOW-tier write stays ASYNC: only the NO-PEER path awaits the slow store; FastSlowStore's normal spawn path is untouched (anti-TraceMachina#203). - in-flight memory bound: a NEW process-global `AckGateBudget` byte-budget Semaphore (`DEFAULT_ACK_GATE_BUDGET_BYTES = 2 GiB`, declared worker_proxy_store.rs:445) is acquired per ack-gated write before the blob is pinned for the join; over-cap = blocking acquire = h2 backpressure to the client, never silent buffering / drop (anti-2026-05-08 67 GB OOM; the 2026-05-08 defect was an UNBOUNDED map — a bounded blocking wait cannot grow without bound). Test that proves the fix re-establishes the invariant: nativelink-service/tests/fl688_two_replica_ack_gate_test.rs (11 tests, production-composition: real CasServer/ByteStreamServer over a WorkerProxyStore wrapper, injected fake workers + locality) + store-crate unit tests in worker_proxy_store.rs. Each contract mutation-verified with a bespoke red-fail message (see report). The two added service tests (`chunked_small_blob_bytestream_gates_on_worker_mirror`, `chunked_small_blob_bytestream_no_peer_lands_on_slow_tier`) cover the small-blob-via-ByteStream path the false oneshot premise hid (see fix-up TraceMachina#3 below); `default_ack_gate_budget_bytes_is_exactly_2_gib` literal-pins the 2 GiB constant (fix-up TraceMachina#6). Behavior changes (each enumerated against its production composition): - cas_server batch handler: LARGE blob (> SMALL_BLOB_THRESHOLD) ack now waits for local-Ok AND ≥1 worker-mirror-Confirmed (or, no peers, awaited slow write); SMALL blob ack waits for local-Ok AND awaited slow-tier (Redis) write. Prior: ack returned after local write, mirror detached. Production composition (verified against prod-server.json5:169-230): WorkerProxy → VerifyStore(cas_STORE) → ExistenceCacheStore(50M) → SizePartitioning(16384) → {lower SMALL_CAS_CACHED=FSS{Memory→Redis}; upper cas_FAST_SLOW_STORE= FSS{Memory→Filesystem}}. is_mirror/is_worker writes are NOT gated (they ARE the 2nd-replica push) — plain write path preserved. - bytestream `inner_write` (chunked, the PRODUCTION CAS path): tee mirror handle is now AWAITED + gated via `confirm_chunked_replica` (was fire-and-forget `drop(mirror_handle)`). Tee still runs CONCURRENTLY with the store write; only the RESULT is now joined. This is the signed-off fire-and-forget→ack-gating conversion (architectural, operator-approved). - bytestream `inner_write_oneshot` (only fires for stores reporting SubscribesToUpdateOneshot — NOT the prod CAS chain; e.g. a direct MemoryStore CAS): now ack-gated like batch. - the random single fire-and-forget mirror (`mirror_blob_to_worker[_with_data]`) is SUPPRESSED when a WorkerProxyStore is present (the ack-gate now places + confirms the single mirror replica); it survives ONLY for the no-proxy fallback composition. - new `wrapper_walker::find_partition_size` (reads live `partition_size()`) routes the no-peer/small-blob slow confirm to the correct SizePartitioning arm without hardcoding 16384. - no-FSS composition (flat single-tier store, e.g. a bare MemoryStore CAS): `confirm_via_slow_store` warns + acks on the single replica (no 2nd tier to await; failing would break valid single-tier wirings; the prod CAS chain always wraps a FastSlowStore so this degraded branch never fires there). - No fsync/sync primitives added. No per-RPC timeout on internal RPCs added. Review fix-ups (cadre at .claude/reviews/139a0653/; DOC + TEST + COMMENT only, NO production-behavior delta — verified: git diff of bytestream_server.rs and store_manager.rs is 100% comment-only; worker_proxy_store.rs is doc-comments + one new test; the test file is test-only): 1. PREMISE CORRECTION (distsys BLOCK-1, convergent with red-team — high confidence). The earlier claim "oneshot is dead in prod because the chain never reports SubscribesToUpdateOneshot" reached the right conclusion via the WRONG mechanism. The real reason: cas_STORE = VerifyStore is the OUTERMOST named store; WorkerProxyStore::optimized_for delegates to self.inner.inner_store(None).optimized_for(...), self.inner is VerifyStore, VerifyStore::inner_store(None) returns self (verify_store.rs:519) and VerifyStore has NO optimized_for override → trait default false (store_trait.rs:1051). Delegation STOPS at VerifyStore. ExistenceCacheStore DOES report true (existence_cache_store.rs:661) but is SHADOWED by VerifyStore. Corrected in the test doc-comment (make_manager_fast_slow) and the store_manager.rs flush comment (which also had Verify/ExistenceCache order swapped — fix-up TraceMachina#7). 2. BEHAVIOR CHANGE — small blob via the ByteStream Write RPC (BLOCK-1). Because oneshot is dead, EVERY ByteStream Write — small blobs included — takes the chunked inner_write path. confirm_chunked_replica has NO confirm_via_slow_tier small-blob branch: with peers present a ≤16 KiB ByteStream blob is gated on a WORKER-MIRROR confirm (peers-present), or the Redis lower SizePartitioning arm on NoPeers. The author's "oneshot→Redis-gated, decoupled from worker reachability" model never applied to any prod blob (oneshot was already dead, so the prior ACTUAL behavior on this path was fire-and-forget chunked mirror). No SmallBlobDispatcher read-locality fan-out on this path. Durability HOLDS (NoPeers→Redis lower arm). Lost on the peers-present path: the decoupled-from-worker-reachability property + read-locality fan-out. DECISION (operator framing "doc fix-ups"): ACCEPT this gap — it is a secondary ingress; BatchUpdateBlobs (the dominant small-blob path) is correctly Redis-gated at cas_server.rs:483-503. Tracked in #FL-691. 3. ADDED TEST for the scenario the false premise hid (red-team blind-spot TraceMachina#2): a ≤16 KiB blob driven through ByteStream::write (NOT batch) on the chunked path, asserting WHERE the 2nd replica lands — worker-mirror (peers present, latency-floor + on-peer) and Redis lower arm (NoPeers, latency-floor + on-slow). Both mutation-verified (fire-and-forget drop(handle) / NoPeers-Ok → ~2ms < 250ms floor → bespoke red-fail). 4. SIZING DOC corrected (MAJOR, convergent distsys + red-team + assumption- auditor — addressed first). The chunked inner_write path has NO 64 MiB ceiling (the 64 MiB gate at bytestream_server.rs:3370 only routes oneshot-vs-chunked; >64 MiB falls THROUGH to chunked). A single large chunked re-drive reads the whole blob under one acquire(expected_size) permit (clamped to capacity), so worst-case concurrency at the cap is 1 blob, NOT the "32" the prior doc claimed. Doc rewritten to the two-path model (oneshot/batch ≤64 MiB vs chunked re-drive uncapped), the additive two-pool note (ack-gate 2 GiB + mirror_blobs 2 GiB independent), and the live 16 GB/~11.5 GB fast-tier budget (the "8 GiB / 2026-03-25" figure was stale post the 2026-05-12 48→16 GB re-tune). 5. KEEPALIVE COUPLING note (MAJOR). The chunked ack tail (handle.await + confirm_chunked_replica) is now bounded by mirror-RPC transport keepalive (h2 30s / QUIC 5s), not by the local write. stall_detector coverage VERIFIED: the enclosing StallGuard stays alive across the await but its progress_handle is NOT bumped during the await window (only the per-chunk recv loop bumps it), so a >30s stall trips the stall_detector's DIAGNOSTIC-ONLY stack dump (stall_detector never aborts the write) — a doc note, NOT a behavior change, so no production fix added. The app-hung-worker wedge this couples to is the accepted FLP risk tracked as #FL-690. 6. VALUE-PIN test for DEFAULT_ACK_GATE_BUDGET_BYTES (numeric-constant rule). Declaration line authoritative: worker_proxy_store.rs:445 literal is 2 * 1024 * 1024 * 1024 = 2 GiB. Mutation (2* → 1*) red-fails the pin with a bespoke message; restored green. 7. store_manager.rs flush comment had the prod chain mis-ordered (Verify/ExistenceCache swapped); corrected to the verified order. 8. Follow-up #FL-691: chunked-path small-blob Redis-confirm + SmallBlobDispatcher fan-out parity (the fix-up TraceMachina#2 gap), and streaming the chunked re-drive instead of buffering the whole blob under one permit (fix-up TraceMachina#4 worst-case). red-team alt-framings (delete the dead oneshot ack-gate; single confirm_for_size seam) are noted there as out-of-scope considerations. Does NOT heal already-lost blobs; does NOT touch the shutdown flush (sibling #FL-688 fix TraceMachina#1 owns those). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

…rt race Adds `race_peer_win_total` / `race_server_win_total` counters to WorkerProxyStore so the worker-side server-vs-peer `get_part` race's effectiveness is readable in production. The win/loss outcome was previously ONLY at `debug!`, which `release_max_level_info` compiles out of the prod binary — the race ran dark. SCOPE NOTE / design-drift (reported to dispatcher): the brainstorm's TraceMachina#3 premise — "the LARGE branch ... does NOT race fastest-of {server, peer-A, peer-B}" — is largely FALSE against current code. The large input path (download_to_directory large branch -> populate_fast_store_unchecked -> copy_slow_to_fast -> run_producer -> slow_store.get -> WorkerProxyStore::get_part) ALREADY races the inner CAS against peers[0] with PROMPT loser cancellation (`cancel_loser_racer`: cooperative rx-drop + 50ms LOSER_GRACE + abort fallback) — landed in `7457436d`, enabled in prod at `local_worker.rs:4733` (`enable_race_peers`). The `TraceMachina#88` bandwidth-doubling/loser-abort concern the dispatch flagged is therefore ALREADY solved. The only genuine residual gaps are (a) it races a SINGLE peer (peers[0]), not multiple peers, and (b) no load-spreading across peers for the large class (unlike the small path's round-robin). Both are an ARCHITECTURAL extension to an already-tuned race (the RST-budget note at `cancel_loser_racer` documents why parallel_chunk_count was cut 64->16 to stay under hyper's reset budget) — re-implementing/multiplying the race needs sign-off, so this commit ships ONLY the missing observability the dispatch explicitly asked for and flags the rest. Invariant being violated (pre-fix): the win-rate of the production server-vs-peer race is not observable in the release binary (debug!-only), so an operator cannot tell whether peer-fetch is offloading the central CAS or whether the race is pure overhead (near-zero peer wins) — and cannot size the bandwidth-doubling cost. Mechanism that violated it: the 4 winner-naming branches in `WorkerProxyStore::get_part`'s `tokio::select!` and the two fallback helpers only emitted `debug!`, compiled out under release_max_level_info. Mechanism that re-establishes it: two `CounterWithTime` fields incremented at EVERY serving point — the 2 primary data branches + 2 zero-length branches in the select (`get_part`), and the success points of `await_peer_after_empty_server` / `await_server_after_empty_peer` (now `&self` methods). "Who ultimately served the bytes" => complete, path-independent win-rate. Published via the existing `publish!` block in `MetricsComponent::publish` (so these ARE on /metrics, unlike the scheduler-side prefetch counters). Composite invariants this fix interacts with: none structurally — counting is side-effect-only on existing branches. The two helpers changed from associated fns to `&self` methods (4 call sites in `get_part`, all updated); no signature or behavior change beyond the `inc()`. No change to racing, loser-cancellation, stale-positive eviction, writer-termination, ack semantics, or store-chain shape. Test that proves the fix: test_race_peer_win_increments_peer_counter (inner empty -> peer serves -> peer counter +1, server counter unchanged) and test_race_server_win_increments_server_counter (peer stale-positive empty -> server serves via fallback -> server counter +1, peer counter unchanged). Both in production composition (race_peers=ON, INITIATOR mode). TDD fail->pass->mutate verified: commenting all peer inc sites red-fails the peer test with "peer win must increment race_peer_win_total (was 0, now 0)"; same for server. Behavior changes: - WorkerProxyStore (worker-side, race_peers=ON; prod composition wrapping the central-CAS GrpcStore as FastSlowStore.slow): on every raced get_part, ALSO increments race_peer_win_total or race_server_win_total. Prior: debug!-only. New: two registered counters. No runtime path/timing/ack change. Server-side WorkerProxyStore (race_peers=OFF) never enters the race branches, so the counters stay zero there — unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

… at 120s and re-lost) Cadre verdict `.claude/reviews/f5-b0fdd341/` = APPROVE except one required fix (pair-a MAJOR, parent-verified). The F5 spill pinned via `fast_store.pin_digests` = a NON-indefinite pin. `expire_stale_pins` (`moka_evicting_map.rs:1798`) demotes non-indefinite pins to plain LRU at `PIN_TIMEOUT_SECS=120` (it EXEMPTS indefinite pins). The Phase-2 C1->server flush is UNBOUNDED and on the degraded server that creates C2* it can exceed 120s; with F3b's 10s forced-drain tick running during shutdown, the demoted over-cap C2* entry is then evicted AFTER "spill complete" printed -> the sole copy is RE-LOST. The original pin-survival test used the DESTRUCTIVE `test_force_pin_expired` as a read-only probe and never ran the sweep, so it stayed GREEN with the bug. Invariant being violated: a C2* sole-copy blob spilled to disk at shutdown must remain durable until the post-restart BIS-ack — i.e. un-evictable across an UNBOUNDED (possibly >120s) graceful shutdown. Mechanism that violates it: the spill's time-bounded `fast_store.pin_digests` (`fast_slow_store.rs`) -> demoted by `expire_stale_pins` at 120s -> evicted by the F3b forced-drain while Phase-2's unbounded C1->server flush is still running. Mechanism that re-establishes it: the spill now takes an INDEFINITE pin via the new `Store::pin_digests_indefinite_with_results` -> `StoreDriver` (default dispatches through `pin_delegation`) -> `FilesystemStore` override calls `pin_digest_indefinite_with_result` -> `MokaEvictingMap::pin_key_indefinite` (exempt from the sweep, released only by `unpin_digests`/BIS-ack). This is durability-identical to F2 pending-BIS blobs. Composite invariants this fix interacts with (admission/eviction/pin triangle, worker FilesystemStore fast tier): pin = now INDEFINITE (was time-bounded); ttl = 120s sweep EXEMPTS indefinite pins; eviction = F3b forced-drain skips pinned entries. A pin REFUSE (indefinite-pin cap exhausted) or eviction race returns false -> treated as a FAILED spill (counted, LEFT in failed_slow_writes for retry), since a non-indefinite pin would re-lose the sole copy. NOT the time-bounded fallback (`pin_digest_indefinite_or_time_bounded`): a 120s pin does not give until-restart durability. Test that proves it: `c2star_mirror_spilled_to_disk_and_pinned` now (1) asserts the entry is pinned, (2) runs the REAL `expire_stale_pins().await` sweep with the pin's timestamp forced stale, (3) asserts STILL pinned -> only an indefinite pin survives. Changes: 1. INDEFINITE spill pin: `fast_slow_store.rs` spill now calls `pin_digests_indefinite_with_results`; false (refuse/race) -> count FAILED + keep in failed_slow_writes (fix TraceMachina#2). 2. New `StoreDriver::pin_digests_indefinite_with_results` (default body mirrors `pin_digests_with_results`, dispatches via `pin_delegation`) + `Store` wrapper (`store_trait.rs`) + `FilesystemStore` override calling `pin_digest_indefinite_with_result` per digest (`filesystem_store.rs`). 3. Pin-survival test rewritten to run the sweep (fix TraceMachina#3) + NEW mutation: revert to non-indefinite `pin_digests` -> reds post-sweep with "C2* spill pin demoted by expire_stale_pins — would be re-lost on a >120s shutdown; must be INDEFINITE". 4. Test doc nit: mutation (a) reds at the `errored==0` guard first when the slow Err is unmasked (pair-b). 5. e2e StoreManager Phase-2 spill-ordering test: FILED in deferred_tasks (pair-b coverage gap) — a StoreManager harness is disproportionate to a straight-line ordering; the spill is unit-tested directly. Behavior changes (vs the b0fdd341 F5 commit): - WORKER cas_server FastSlowStore: the spill pin is now INDEFINITE (was time-bounded). Effect: a spilled C2* blob survives a >120s degraded shutdown instead of being demoted+evicted. New API surface (`pin_digests_indefinite_with_results`) is additive: Leaf non-pinning stores (Memory, Noop) return all-false; wrapper stores fan out via `pin_delegation`; only FilesystemStore takes a real indefinite pin. No other call site uses the new method, so no other store's runtime behavior changes. TDD: green confirmed; the new MAJOR mutation reds with its bespoke message; the no-pin mutation still reds at the first pin check; util+store build clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

…re-first (default-off) Re-derives the existing v2 slot-full Stage B code (e5cebc2) to v3 per design §2.3-v3 (RE-DERIVATION banner). The live fleet runs `max_inflight_tasks=0` (unlimited slots), so the v2 "busy-because-FULL" gate could NEVER fire; v3 makes "busy" = P-SATURATED (the pcore-first signal), gates the hold on `p_gate_active`, and computes T_wait_W as the k-th-soonest completion (time to regain ONE P-slot), not the sum. Invariant-walk (§2.3-v3): - Invariant violated: the matcher must NOT reconstruct an op's tree on a cold X (routed-away-from a pre-built P-saturated holder W by pcore-first) when W will regain a P-slot before the reconstruct completes. - Mechanism that violates it: pcore-first's `worker_is_viable_gated` (api_worker_scheduler.rs:2259) EXCLUDES a P-saturated holder W from the cache tiers → the op routes to a cold X that reconstructs, losing W's pre-built tree. - Mechanism that re-establishes it: the temporal hold gate (api_worker_scheduler.rs:2592) — when `enable_speculative_hold && p_gate_active && all three cache winners None`, and ∃ a P-saturated root-holder W (from the already-materialized `p_gated_excluded` set) with `t_wait_w_locked(W) < T_SETUP` ∧ not overdue → return `None` (re-queue the op for W). - Composite invariants: lock discipline (the k-th core `t_wait_w_locked` is lock-free — no `.read()/.write()/.await` — preserving the deadlock fix); pcore-first spread-under-saturation (gated on `p_gate_active`, so NO hold on a Phase-2-lifted fully-saturated fleet); C1 non-locality-X (all three winner guards); max-hold cap (wall-time OR cycle-count starvation bound); A5 legacy worker (p_core_count=0 → ungated → never a hold candidate). - Proving tests: hold_fires_for_p_saturated_holder (k=1, cold X, p_gate_active → hold); no_hold_when_p_gate_inactive (fully-P-saturated fleet → NO hold, the distsys BLOCK-2 guard); no_hold_when_gossip_subtree_holder_x / c1_locality_winner_blocks_hold (C1); deep_backlog_declines + k_th_completion_frees_one_p_slot + stage_c_worker_time_to_free_kth_soonest (k-th order statistic); hold_no_deadlock_under_write_lock (lock-free core). v3 adaptations (each cites the design fix): 1. Busy → P-saturation (v3.1/v3.7-#2/TraceMachina#3): `find_busy_full_holder` → `find_p_saturated_holder`, which FILTERS the already-materialized `p_gated_excluded` set (:2194 — the viable-but-no-P-headroom holders) for root-holders. No second `self.workers.iter()` scan. Capability filter (v3.7-TraceMachina#6) holds by construction (`p_gated_excluded` is built from `worker_is_viable` candidates = capability-index ∩ platform ∩ health). 2. t_wait_w_locked sum → k-th soonest (v3.3/v3.7-#4): `k = running − p_core_count + 1`, clamped to [1, timed_count]; k-th smallest remaining via `select_nth_unstable_by` (O(running), no full sort); overdue computed over the k-soonest set only. KEPT lock-free. `worker_time_to_free` wrapper delegates. 3. Gate condition (v3.7-TraceMachina#1/TraceMachina#5): `enable_speculative_hold && p_gate_active && dir_cache_winner.is_none() && subtree_coverage_winner.is_none() && locality_winner.is_none()`. 4. KEPT verbatim (v3.4): capped HoldRecord LruCache + 3 reaps, None-return, 4 counters, default-OFF flag, max-hold cap. Carried Tier-3 fixes (from e5cebc2 review): - DELETED the fabricated T_SETUP histogram citation (":3408 #p1p2 p50 ~1.4s" — the file does not exist); replaced with an honest coarse-guess/tune-via-soak comment. T_SETUP=3s value unchanged. - FIXED the deadlock-guard test: the reserve now runs on a SEPARATE spawned task whose JoinHandle is `timeout`ed (the v2 inline-await form WEDGED the current- thread runtime — empirically confirmed: a mutated inline reserve did not return in 2 min despite a 5 s internal timeout; the JoinHandle form fires the bespoke DEADLOCK message at 5.01 s). - COVERAGE gaps: HOLD_MAX_CYCLES arm (hold_max_cycles_arm), both defensive reaps (defensive_reap_on_reroute/_on_evict — coverage+no-panic, the reap is defensive), the do_try_match None re-queue seam (do_try_match_none_requeue_seam — production composition), overdue boundary (overdue_boundary_elapsed_equals_estimate), subtree-holder disjunct (subtree_holder_disjunct). - MIGRATED the Stage-C `worker_time_to_free` unit test from SUM to k-th semantics (stage_c_worker_time_to_free_kth_soonest) since the core changed sum→k-th. Behavior changes (enumerated against production composition): - enable_speculative_hold=false (DEFAULT, prod): NONE. The gate is gated on the flag; with it off the matcher assigns byte-identically to the pre-Stage-B path (flag_off_no_hold pins this against a pcore-first-ON fleet). - worker_time_to_free (async wrapper) semantics changed SUM → k-th-soonest. It has NO production consumer (only Stage-C's own unit tests + the lock-free core, which the gate calls directly); prod behavior unaffected. - enable_speculative_hold=true (soak-only, NOT prod; requires p_headroom_gate ON): on a P-headroom-available fleet, an op whose only holder W is P-saturated and about to regain a P-slot (t_wait_W < 3 s, not overdue) and whose alternatives are all cold (no dir/subtree/locality winner) is HELD (None, re-queued) instead of rebound to a cold worker. Bounded by the max-hold cap; NO hold on a fully- P-saturated Phase-2-lifted fleet (p_gate_active false). All other reserve outcomes unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

…d/cumulative fix) + TraceMachina#6 reconnect-recovery signal QUIC follow-ups TraceMachina#3 + TraceMachina#6, FL #6951 aftermath. Observability-only; no data-plane or control-flow behavior change. DEFECT (TraceMachina#3): the QUIC read-failure diagnostic split failures ONLY on a 50ms elapsed gate into quic_read_instant_fail / quic_read_slow_fail (cumulative AtomicU64). The `slow` bucket (elapsed>50ms) conflated EVERYTHING: in prod it was dominated by FailedPrecondition NL_REDIRECT signals (normal P2P handoff, bytestream_server.rs:2427), NOT genuine transport timeouts — so slow_fail_total was not a timeout count, and being cumulative-since-start "42" read like an incident but was ~1.9 days of accumulation. Net: the counter could not answer "how many genuine QUIC timeouts happened." FIX (TraceMachina#3): add a CONTENT classifier `quic_read_fail_class(err)` (pure, no elapsed input) with three classes and three cumulative counters, incremented in record_quic_read_fail ALONGSIDE the retained elapsed split: - quic_read_transport_timeout: Code::Unknown + "buffered service"/"timed out" (the tower-Buffer/quinn-idle-timeout signature looks_like_latched_pool matches, MINUS the 50ms gate — an idle-timeout is SLOW). The honest timeout count; alert on rate(quic_read_transport_timeout). - quic_read_redirect: Code::FailedPrecondition + REDIRECT_PREFIX ("NL_REDIRECT:"). Normal protocol, help-text says NOT a transport failure. - quic_read_other_fail: everything else. Counters stay cumulative Prometheus counters (correct — isolate an incident with rate()/increase(); a reset would break rate()). The diagnostic warn! gains a `fail_class` field + the three content totals so a single line is honest (it no longer lets a `slow` event read as a timeout). FIX (TraceMachina#6): the FL #6951 tls_utils.rs Reconnect fix changed the failure SHAPE from "instant 35k-replay poison" to "transient timeout -> reconnect -> heal". Add quic_read_timeout_recovered: single-stream read operations that recorded a genuine transport timeout on the QUIC leg then SUCCEEDED on a later attempt of the same retrier operation (the Reconnect layer healed the member). Regime-shift discriminator: pair with quic_read_transport_timeout — healed-reconnect = both climb in lockstep; PERMANENT-latch (fix regressed) = timeout climbs while recovered stays FLAT, because a poisoned member instant-fails every retry (consecutive_latched_fails>=THRESHOLD -> abort -> Err, never reaching the success arm that increments recovered). So "timeout climbing WITHOUT recovery = regression." Scoped to the single-stream path (prefer_tcp=false -> always QUIC under Dual/Quic, one operation = one LocalState, exact attribution). Parallel path deliberately excluded (Dual routes over TCP; Quic-only shares a cross-chunk atomic -> would over-count -> dishonest). CROSS-CRATE: TraceMachina#6's alternative source (the tls_utils.rs maker's "rebuilding QUIC pool member" log, nativelink-util/src/tls_utils.rs:547) was NOT used — it lives in a different crate (nativelink-util), owned by another agent, and is buried in a per-Reconnect tower::Service. Used the dispatch-sanctioned grpc_store-side recovery counter instead. NO tls_utils.rs edit. Invariant being violated: the QUIC diagnostic's counters must let a scrape answer "how many genuine QUIC transport timeouts happened" and "did the fix's transient-timeout failures recover" — the elapsed-only cumulative split could answer neither (it conflated redirects with timeouts). Mechanism that violates it: record_quic_read_fail (grpc_store.rs) split only on elapsed; every >50ms failure — redirect, backpressure, or genuine timeout — landed in one undifferentiated cumulative `slow` counter. Mechanism that re-establishes it after the fix: quic_read_fail_class splits by error CONTENT into three counters (timeout/redirect/other); the TraceMachina#6 recovery counter increments at the single-stream success arm iff a genuine timeout was seen earlier in the same operation, giving the healed-vs-latched discriminator. Composite invariants this fix interacts with: (a) the #FU latch-detection control flow (looks_like_latched_pool + LATCHED_POOL_ABORT_THRESHOLD) — UNTOUCHED; the abort-on-latch is precisely why a permanent latch never reaches the recovery increment. (b) the elapsed instant/slow split — RETAINED (orthogonal shape signal). (c) the rate-limit warn! gate — UNCHANGED; content counters increment before the gate so they are never suppressed. (d) the QUIC-leg gate (quic_read_leg) — UNCHANGED; content + recovery still count ONLY genuine QUIC-leg reads. Test that proves the fix re-establishes the invariant: quic_read_fail_class_separates_timeout_from_redirect (+ _timeout_matches_either_substring, _other_is_not_timeout_or_redirect). Behavior changes (against production composition — GrpcStore as the worker slow-store QUIC client, Transport::Quic / Dual): - GrpcStore: adds 4 cumulative metric counters (quic_read_transport_timeout, quic_read_redirect, quic_read_other_fail, quic_read_timeout_recovered) + 3 new fields on the diagnostic warn! line (fail_class, and the 3 totals). Observability ONLY. No change to reads/writes/eviction/retry/abort routing. record_quic_read_fail now returns Option<QuicReadFailClass> (was ()); the single-stream site consumes it to set a per-operation flag, the parallel site discards it. No control-flow effect at either site. - All other stores / compositions: NONE. The change is confined to GrpcStore's QUIC-feature-gated diagnostic; the no-quic build compiles the metric fields + recovery method out entirely (verified: no-quic lib builds clean, no dead-code warning on new symbols). TDD: quic_read_fail_class tests written first, RED (E0432 undefined symbol) -> implemented -> GREEN -> mutation-verified 3 contracts (swap timeout/redirect arms; ||->&& narrows either-substring match; Other-fallthrough->TransportTimeout), each red-fails with its bespoke message. tee: /tmp/qf-obs-*.log. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

…lass→counter wiring test (no logic change) Convergent fix-up (red-team + assumption-auditor; testing-czar proved the test gap cheap) on the TraceMachina#6 quic_read_timeout_recovered counter. COUNTER LOGIC UNCHANGED — this is doc/help honesty + a wiring test only. FINDING (why the doc was wrong): "quic_read_transport_timeout climbing WITHOUT quic_read_timeout_recovered = the FL#6951 fix REGRESSED" is NOT a sound standalone conclusion. The SAME shape (timeout↑, recovery flat) occurs while the fix is perfectly intact in ≥3 other regimes — verified against code: 1. Server outage: server unreachable → every read times out, nothing recovers in-transport → flat recovery. An oncall trusting the old doc could REVERT the correct Reconnect fix during an outage → re-introduce the permanent poison. This is the load-bearing foot-gun. 2. Fallback-layer heal: a timeout that heals ACROSS operation boundaries (FSS terminal → slow-store/peer-fetch, or a fresh higher-layer get_part) — real heal, but lands in a different op, so this per-op counter stays flat. 3. Round-robin dodge: QuicChannel::poll_ready is round-robin (tls_utils.rs:430) + each read uses a fresh quic.clone(), so attempt-2's Ok is often a DIFFERENT healthy member, not the timed-out member self-healing via Reconnect. The counter conflates member-self-heal with round-robin-dodge. Also corrected: (a) "lockstep" is false — quic_read_transport_timeout counts per-ATTEMPT, recovered per-OP (N:1 in a healthy pool, not 1:1); (b) "recovered" means the transport RECONNECTED (read_internal returned Ok at ~:2680), NOT that the read COMPLETED (the frame loop runs after and can still mid-fail). CODE-VERIFIED premises (re-derived myself, not relayed): * retry-exhaustion path: retry.rs:174-175 — a retryable Retry(err) after the backoff sequence exhausts (iter.next()==None) returns Err via ok_or_else(...)?. So a purely-SLOW timeout (>50ms, no latch, skips the abort) still never reaches the success arm. This is the SECOND unreachability path the old doc omitted (it named only the latch-abort). * round-robin: tls_utils.rs:422-438 (counter.fetch_add % channels.len(), reset per call) + grpc_store.rs read_internal QUIC arm uses quic.clone(). * recovery-arm position: grpc_store.rs success arm precedes the frame loop (Some(Err(status)) still reachable after). FIX-UP: 1. Rewrote quic_read_timeout_recovered's #[metric(help)] + field doc: honest IN-TRANSPORT reconnect-recovery signal (reconnected, not read-completed, possibly a round-robin dodge); a recovery-RATE signal, NOT a standalone regression detector; enumerates the outage / fallback-heal / round-robin confounders; directs oncall to correlate with server-reachability + fallback/peer-fetch counters BEFORE concluding a regression. 2. record_quic_read_timeout_recovery doc now names BOTH unreachability paths (latch-abort AND retry-exhaustion) and corrects "healed" → "reconnected". 3. Softened the saw_quic_transport_timeout field doc + success-arm inline comment (reconnected, member-heal-OR-round-robin-dodge). Field NOT renamed (avoid churn) — only the causal claim removed. 4. Added record_quic_read_fail_increments_matching_content_counter (#[cfg(quic)] #[nativelink_test]): builds a real Transport::Quic store with NO server (GrpcStore::new does no network I/O for a QUIC spec — h3_channel binds local UDP sockets, QUIC dials lazily; literal-IP 127.0.0.1 skips DNS) and directly asserts (a) Unknown+"buffered service" → quic_read_transport_timeout +1 AND the returned Option == TransportTimeout; (b) FailedPrecondition+ "NL_REDIRECT:" → quic_read_redirect +1; (c) Unknown+unrelated → quic_read_other_fail +1; (d) record_quic_read_timeout_recovery(true) → +1, (false) → no change. Closes the class→counter dark-counter gap the pure classifier tests could not (a swapped fetch_add arm compiles clean + passes them). A `matches!(store.transport, Transport::Quic(_))` guard prevents a vacuous pass via the leg gate. Invariant being violated: the TraceMachina#6 counter's documentation must not license a false causal inference (flat-recovery ⟹ fix-regressed) that would make an oncall revert the correct never-poison fix during an outage; and the class→counter increment wiring must be test-covered (not a compile-clean dark counter). Mechanism that violates it: the prior help/doc asserted "timeout climbing WITHOUT recovery = regression" as a biconditional; the class→counter arms had no test, so a swapped fetch_add was invisible. Mechanism that re-establishes it: the rewritten help/doc frames it as a recovery-RATE signal with the three named confounders + a correlate-first directive; the new wiring test pins each class→counter arm (+ the recovery condition) and red-fails on a swapped arm. Composite invariants this fix interacts with: (a) the counter LOGIC — UNCHANGED (doc/help + test only); (b) the TraceMachina#3 content classifier — its pure tests still pass; the wiring test is complementary (classifier=WHAT-class, wiring=which-counter). Test that proves the fix: record_quic_read_fail_increments_matching_content_counter (mutation: swap redirect↔other fetch_add arms → red-fails "a FailedPrecondition NL_REDIRECT must increment quic_read_redirect", left:0 right:1). Behavior changes (against production composition — GrpcStore as the worker slow-store QUIC client): NONE at runtime. All four counters increment exactly as before; record_quic_read_fail / record_quic_read_timeout_recovery signatures and call sites are byte-for-byte unchanged. Only #[metric(help)] STRINGS, doc-comments, and a new #[cfg(quic)] test were added. No-quic build unaffected (all additions quic-gated; verified clean). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

allada force-pushed the async_buffer_fix branch from 8ed274f to 88638af Compare November 3, 2021 06:56

Fixed AsyncFixedBuffer

6dd57de

Fixes various errors in AsyncFixedBuffer.

allada force-pushed the async_buffer_fix branch from 88638af to 6dd57de Compare November 3, 2021 06:59

allada commented Nov 3, 2021

View reviewed changes

allada merged this pull request into master Nov 3, 2021

allada deleted the async_buffer_fix branch November 11, 2021 06:22

MarcusSorealheis restored the async_buffer_fix branch July 17, 2026 11:11

vercel Bot had a problem deploying to Preview – nativelink-aidm July 17, 2026 11:11 Failure

vercel Bot had a problem deploying to Preview – nativelink July 17, 2026 11:11 Failure

MarcusSorealheis deleted the async_buffer_fix branch July 17, 2026 11:28

Navigation Menu

Repository navigation

Fixed AsyncFixedBuffer - #3

Fixed AsyncFixedBuffer#3
allada merged 1 commit into
masterfrom
async_buffer_fix

allada commented Nov 3, 2021 •

edited

Loading

Uh oh!

allada left a comment

Uh oh!

This branch had an error being deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

allada commented Nov 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

allada left a comment

Choose a reason for hiding this comment

Uh oh!

This branch had an error being deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

allada commented Nov 3, 2021 •

edited

Loading