Fix s3_store by allada · Pull Request #6 · TraceMachina/nativelink

allada · 2021-11-03T07:56:25Z

This change is

allada

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

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>

… handler Extract the body of the `Update::BlobsInStableStorage` match arm into a free `handle_blobs_in_stable_storage(state, cas_store, proto_digests)` function in `local_worker`, and add a test-only `BlobsAvailableState::new_for_test(fs_store, cas_server_fss)` constructor. Both are gated on the new `test-utils` feature in nativelink-worker (which transitively enables nativelink-store/test-utils). Tests in `nativelink-worker/tests/blobs_in_stable_storage_handler_test.rs`: 1. `mirror_blob_removed_when_proto_received` — pin two mirror blobs, send a `BlobsInStableStorage` proto for one, assert the matching blob is gone and the other is untouched. Mutate-tested by commenting out `remove_mirror_blobs` inside the handler — test fails with `acked digest must be removed from mirror_blobs` on the count mismatch (left=2, right=1). 2. `invalid_proto_digest_does_not_crash_handler` — malformed hex digest in the proto must be skipped, but the rest of the batch must still be processed. 3. `handler_no_op_when_cas_server_fss_absent` — None for cas_server_fss must not panic; covers the workers that do not run a CAS server. Also adds a stub for the `download_to_directory_mirror_test` (review TraceMachina#6, filled in next commit) so cargo's `[[test]] required-features` block can reference it without a manifest error. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

…ob via download_to_directory End-to-end test that exercises the production materialization path with a mirror-only blob: * Build a worker `FastSlowStore` whose slow tier is an EMPTY MemoryStore (models server-down or server-lost-blob). * Insert a test blob into `mirror_blobs` ONLY (not into the fast tier or slow tier). * Construct a one-file Directory proto whose only file references that digest, hand it in via `pre_resolved_tree` to skip GetTree. * Call `download_to_directory(..)` and assert it succeeds and the materialized file on disk has the correct bytes. Mutate-tested by stubbing `materialize_mirror_to_fast` to always return `Ok(false)` in BOTH `populate_fast_store` and `populate_fast_store_unchecked`. The test fails with `NotFound` on the populate step (`Object … not found in either fast or slow store`). The first attempt of this test passed `fs_for_link` separately from `fs_for_cas` and hit a hardlink-after-populate failure unrelated to the mirror logic. Production wiring (in `nativelink::main`) shares the same `FilesystemStore` Arc as both `cas_store.fast_store()` and the `filesystem_store` parameter to `download_to_directory`; the test now matches that. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

… E2E reconciliation test, capacity plumbing test, RwLock short-circuit TraceMachina#6 (HIGH) — Added `end_to_end_mirror_reconciliation_test.rs` exercising the headline server-restart scenario at the worker FSS data-flow seams: mirror-only blob → pinned_mirror_digests advertise → UploadMissingBlobs read-through → slow store durability → BlobsInStableStorage → pin drop. Mutate-verified by commenting out `cas_fss.remove_mirror_blobs(&acked_digests)` in `handle_blobs_in_stable_storage` — test FAILS with "step 6: pin must be dropped after BlobsInStableStorage" (left=1, right=0). Full transport-layer E2E with WorkerApiServer + LocalWorkerImpl over an in-memory gRPC pair remains deferred (TODO in test body). The two endpoints are independently exercised in `worker_api_server_test.rs` and `blobs_in_stable_storage_handler_test.rs`; this test pins the data-flow contract between them. TraceMachina#7 (HIGH) — Added `mirror_capacity_report_plumbed_to_picker_test` and `zero_mirror_max_does_not_record_capacity_test` in `worker_api_server_test.rs`. Sends a real `BlobsAvailableNotification` through the dispatch arm and asserts the `(used, max)` reaches `WorkerProxyStore::mirror_capacity_for_test`. Pre-fix the existing tests just wrote `mirror_used_bytes: 0, mirror_max_bytes: 0` literals to compile — nothing asserted plumbing. Mutate-verified by commenting out the `proxy.record_mirror_capacity(...)` block in `worker_api_server.rs` — test FAILS within 5s with "plumbing is broken" panic. To support the test, added `mirror_capacity_for_test` accessor on `WorkerProxyStore` (gated on `cfg(any(test, feature = "test-utils"))`), and a `test-utils` feature on `nativelink-service` that propagates to the store. TraceMachina#10 (MINOR) — `record_mirror_capacity` now short-circuits when `(used, max)` is unchanged from the last report. Workers tick every ~100ms; without the short-circuit the RwLock::write was taken ~10/s per peer (and ~100/s per peer with high mirror activity) for no-op updates. The fast path takes only the read lock; the upgrade to write happens only on a real change. CI step extended to also build/run nativelink-service with test-utils so the new plumbing test runs. cargo check --all-targets clean. All new tests pass (mutate-verified for TraceMachina#6 and TraceMachina#7). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Per testing-czar MISSING-COVERAGE TraceMachina#6 from .claude/reviews/valkey-pool-521c13d1/testing-czar.md. `new_with_pool_size:261` uses `future::try_join_all`, which cancels all remaining dials on first failure. Behavior was unspecified by tests — nothing pinned the contract. Test `pool_partial_dial_failure_returns_err_fast` constructs a connect_func with a SeqCst attempt counter; the third attempt returns Err. With pool_size=3, `new_with_pool_size` must return Err containing the simulated failure tip. Wrapped under `tokio::time::timeout(5s)` per CLAUDE.md. Note: try_join_all spawns futures concurrently, so the order in which the counter sees attempts is racy. The contract is "ANY one failure fails the constructor"; this test exercises that, not "the third specific attempt fails". Mutation: change `try_join_all` to `join_all` (collects all results without short-circuiting). The .await? chain inside new_with_pool_size would then need a manual error-check; without it, behavior diverges (silent partial pool, or panic). Either failure mode → test fails on the .expect_err. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

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.

…pped); require server opt-in (TraceMachina#334 bundle fixup TraceMachina#6 — red-team TraceMachina#1) Red-team TraceMachina#1 pre-mortem: an 8 GiB server default `slow_writes_in_flight_max_bytes` would be inherited transparently by 16-32 GiB workers (25-50% of host RAM) without an explicit override — the exact pattern that cascades into the TraceMachina#203 OOM under slow-tier wedge. `local_worker.rs:2674,2778` already explicitly override to 0, but the DEFAULT in the spec is the load-bearing concern: future composition sites adding new FastSlowStores would inherit the dangerous default silently. Per CLAUDE.md R7 ("mechanism not operator"), defaults that wedge production on inheritance are design faults. Explicit opt-in with documented rationale is the safer baseline. Changes: - `nativelink-config/src/stores.rs`: `default_slow_writes_in_flight_max_bytes` returns 0 (was 8 GiB). Updated field doc-comment to call out the new default + recommended explicit values per host class. - Production config (`~/fl/bld/infra/nativelink/prod-server.json5`): `cas_FAST_SLOW_STORE` now sets `slow_writes_in_flight_max_bytes: 8589934592` explicitly (preserves the 8 GiB cap on the server). The other FastSlowStores (AC_BACKEND_CACHED, SMALL_CAS_CACHED) are intentionally left uncapped — slow tier is Redis (fast) so the 8 GiB cap target was always the FilesystemStore-backed CAS. - `local_worker.rs`: doc-comment update on the existing `: 0` overrides to note they are now redundant with the default but kept explicit so removing the default later wouldn't silently re-introduce a cap on workers. Note: `/srv/nativelink/buildcache-native.json5` (the canonical production config that the server reads) is permission-denied to this worktree; the user must sync the cap setting there before deploy. 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).

… dual-err + (Ok,Err) test) Addresses the two actionable MAJORs from the prior cadre red-team review (`.claude/reviews/agent-476-fixup/red-team.md`). The shutdown-path sibling (MAJOR-3 at `:4957-4958`) is left for branch `agent-512`. ## MAJOR-1: equality-guard the dual-err diagnostic append Pre-this-commit the `(Err, Err)` arm always appended: "consumer error preferred over producer 'receiver disconnected' symptom" After the TraceMachina#476 fixup Option A (`tx.send_error(err.clone())` in `forward_fut`), when the producer-side hits a real IO err, that err is ALSO routed to the consumer via terminal_error, so BOTH halves of the join return the SAME err. In that arm the append message was a lie: there was no "receiver disconnected" symptom; the consumer's err IS the routed producer err. Operators reading the log would have looked for a consumer root cause distinct from the producer — and there isn't one. Option chosen: 1 (equality-guard) per red-team finding TraceMachina#6 recommendation. - Option 2 (rewrite message) still appends in the same-err case, relabelling rather than acknowledging the routed-mirror semantics. - Option 3 (drop arm) loses information when errs ARE legitimately distinct (consumer h2 reset while producer was about to send EOF) — that case is still real and the append still helps observability. - Option 1 preserves the legitimate symptom-prefer append AND short-circuits cleanly to a single err when Option A routed both halves to the same payload. Applied symmetrically to both `stream_path_to_store` and `stream_file_to_store`. `Error` derives `Eq`+`PartialEq` (verified at `nativelink-error/src/lib.rs:54`). ## MAJOR-2: add (Ok producer, Err consumer) test Adds `surfaces_consumer_abort_when_producer_succeeded` to `nativelink-store/tests/stream_file_to_store_observability_test.rs`. This guards the `(Err(write_err), Ok(())) => Err(write_err)` arm — the most common production dual-err shape: VerifyStore post-EOF size mismatch, h2 RST_STREAM after the body completes, slow-store commit-time rejection. The two existing tests both hit the `(Err, Err)` dual-err arm; without this test, a refactor that flipped arm order or silently swallowed `(Err, Ok)` consumer-errs would ship undetected. Mechanism: - New `AcceptAllThenAbortOnEofStore` slow-tier fake drains every chunk + observes a clean EOF (`Ok(empty)` from `recv` per `buf_channel.rs:607,632`), THEN returns Err(POST_EOF_ABORT_TAG). - Small payload (4 chunks × 16 KiB = 64 KiB, well under buf_channel capacity 128 + bridge mpsc 4) — producer drains without blocking. - Production composition (real `FastSlowStore` + real `MemoryStore` fast tier reporting `FileUpdates` → takes the `update_with_whole_file` parallel `stream_file_to_store` path). - `tokio::time::timeout(5s)` deadlock detector with bespoke message. - Bespoke assertion message: "consumer-post-EOF-Err policy regressed — the `(Err, Ok)` arm dropped the consumer-side authoritative err". - Three guards confirm the right arm fires: 1. `saw_clean_eof == true` — producer fully drained, EOF observed. 2. Dual-err diagnostic append text MUST NOT appear (would mean the (Err, Err) arm fired instead of (Err, Ok)). 3. `update_invocations >= 1` — slow tier actually invoked. ## Verification `timeout 600 cargo check -p nativelink-store` → 47s, clean (existing warnings only). `timeout 300 cargo test -p nativelink-store --test stream_file_to_store_observability_test` → 3 tests pass (1.29s): - surfaces_consumer_error_over_symptom_dual_err - surfaces_producer_io_error_not_buf_channel_symptom - surfaces_consumer_abort_when_producer_succeeded ← NEW Mutation-verified: temporarily replaced `(Err(write_err), Ok(())) => Err(write_err)` → `(Err(_), Ok(())) => Ok(())` in `stream_file_to_store`. New test red-failed with the bespoke "update_with_whole_file must propagate the slow-tier post-EOF abort as Err; got Ok(...)" panic. Reverted; test re-green.

…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>

…erEventuallyTerminates from Bugged cfgs MAJOR-1 (red-team R2): the renamed BytesWrittenBoundedByChunkCount is also vacuous under the spec's single-atomic-action model — only WriterSendChunk modifies either side and it increments BOTH in lockstep, so 'bytesWritten <= chunkCount' reduces to 'chunkCount <= chunkCount'. Drop it from all 5 cfgs and the spec body; doc-comment names Phase 5 TraceMachina#6 (atomics-ordering hazard) as the proper home for the non-vacuous form once the writer action is split into two sub-actions. MAJOR-3 (red-team R5): WriterEventuallyTerminates was decorative on Bugged cfgs — TLC terminates on the safety violation at depth 4 BEFORE the liveness pass runs, so the PROPERTY was parsed but never checked. Drop from all 3 Bugged cfgs (Bugged, BothOffBugged, _BuggedSilentZero); the Fixed cfg already lists it and reaches the liveness pass. Also fix stale doc-comment in _BuggedSilentZero.cfg claiming it is gate-classified — the filename suffix BuggedSilentZero.cfg does NOT match the gate's *Bugged.cfg glob; it is informational (invariant-prover MINOR-1, surfaced as part of MAJOR cleanup). All 19 specs continue to PASS the Layer 1 gate; all 3 Bugged cfgs still violate NoSilentZeroToReader as their named invariant. Verified via direct TLC: Bugged.cfg=371 distinct states, BothOffBugged.cfg=258, _BuggedSilentZero.cfg=342.

- code-reviewer m2: drop the redundant `range.clone()` in make_chunk — `Bytes::slice` consumes the Range<usize> via impl RangeBounds and `range` is unused after the call. Reformat the signature one-per-line while there (was over the soft 100-char line limit). - testing-czar MINOR-2: strengthen build_chunks_is_zero_copy_over_payload: add chunk-tail pointer-bound check + byte-equality against the canonical payload slice. Head-only checks would pass under partial aliasing; byte-equality defends against the allocator-arena coincidence (fresh copy reusing the freed source's address). Remaining MINORs (code-reviewer m1/m3/m4/m6/m7, perf-optimizer m1-m5, testing-czar MINOR-1/MINOR-3/MINOR-4, red-team NIT TraceMachina#6 panic-arm shutdown) deferred as TraceMachina#533 follow-up trackers per dispatch instructions — low-risk polish, not load-bearing. Bench-only diff. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

…Machina#533 red-team NIT TraceMachina#6) Bench-only fix to `run_w3_iter`'s c>1 JoinSet drain loop. Prior shape called `res.expect(...)` which propagated the panic immediately but left the remaining N-1 spawned tasks running against the in-process server. Those stragglers continue mutating the per-cell `FilesystemStore` tempdir even after the panic unwinds the cell — when `_td: TempDir` drops at end of `run_w3_cell`, the tempdir is unlinked while straggler tasks still hold fds into it, leaking the tempdir contents on Linux until the straggler dies (red-team TraceMachina#533 NIT TraceMachina#6). Fix: on `Err(JoinError)` from `join_next`, call `set.shutdown().await` to abort + await every still-running task, then panic with a diagnostic message naming the cleanup. `shutdown` is idempotent so safe to call on an already-drained set; in the happy path (all tasks finish cleanly) we never reach the panic arm and the loop exits when `join_next` returns `None`. Falsification: a deliberate `panic!()` inside one of the spawned tasks would, under the prior shape, leave the surviving N-1 tasks running past tempdir drop; under the new shape, `set.shutdown()` aborts them before the panic propagates out of the cell. cargo check -p nativelink-benchmarks --features chunked_fast_slow --tests: clean. No production-code change. Co-Authored-By: Claude Opus 4.7 (1M context) <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>

… grace-after-boot distsys MAJOR-1 (re-opened in refix-distsys.md): the never-reconnect TTL sweep was SCHEDULED but its single one-shot fire ALWAYS swept 0. The sweep task slept `grace` (600s) from BOOT, but `reload_baseline` is stamped at the END of `reload_from_disk` (after the async read+decode = boot + reload_dur). At fire `baseline.elapsed() = grace - reload_dur < grace`, and the strict `<` time-gate in `sweep_unconfirmed` returned 0 every boot. The sentinel producer shipped; its sole consumer was a guaranteed no-op → reloaded never-reconnect locality entries leaked forever (degrades peer-fetch; bounded best-effort index per invariant TraceMachina#6, NOT durability loss). Fix: the sweep task now awaits the `bazel_ready` readiness gate (which the reload task flips `true` ONLY after `reload_from_disk` returns, i.e. after the baseline is stamped) BEFORE sleeping `grace`, so `baseline.elapsed() >= grace` holds at the single fire. The await->sleep->sweep sequence is extracted into `LocalityPersister::run_never_reconnect_sweep(reload_done, grace)` so the bin path and the production-timing test exercise the SAME scheduling logic. `reload_baseline` switched from `std::time::Instant` to `tokio::time::Instant` (3 sites, grace-gate only) so the sweep's `sleep(grace)` and the gate's `baseline.elapsed()` read ONE clock — required for the production-timing test to virtualize the 600s window under `tokio::time::pause()`. Production (never paused) is identical: `tokio::time::Instant` delegates to the real monotonic clock. Invariant being violated: a reloaded endpoint whose worker never reconnects within `grace` must have its sentinel locality entries swept (design §4.4 — no "hold-forever"). Mechanism that violates it: src/bin/nativelink.rs sweep task slept `grace` from boot while sweep_unconfirmed's gate (worker_api_server.rs:1806 region) measures `grace` from the strictly-later baseline -> elapsed < grace -> 0. Mechanism that re-establishes it: nativelink.rs sweep task awaits the bazel_ready gate (flips after reload_from_disk returns + stamps baseline) then run_never_reconnect_sweep sleeps grace -> baseline.elapsed() >= grace at fire. Composite invariants this fix interacts with: (a) item C or_insert guard (sweep must not drop a live reconnected endpoint — unchanged, still gated on the sentinel identity); (b) bazel_ready fail-open (gate flips even on reload error/timeout — sweep then no-ops on a None/late baseline, correct); (c) TraceMachina#141 boot-epoch wipe (sentinel identity, orthogonal to the timing gate). Test that proves the fix re-establishes the invariant: never_reconnect_sweep_fires_grace_after_baseline_not_boot. Behavior changes: - nativelink (bin) sweep task: prior -> the one-shot sweep slept grace from boot and was a guaranteed no-op (swept 0 every boot); new -> awaits the reload-done readiness gate, then sleeps grace, then sweeps once -> swept N (the actual never-reconnect endpoints). Verified safe in compositions: the production worker_api entry (locality_persister Some), where the reload task + Bazel-REAPI listeners + this sweep task share `bazel_ready_tx`; a no-worker_api entry yields None persister -> task returns early (no-op), unchanged. - LocalityPersister.reload_baseline / WorkerApiServer.locality_reload_baseline: std::time::Instant -> tokio::time::Instant. No runtime behavior change in production (un-paused tokio::time::Instant == real monotonic clock); enables virtual-time testing only. - No async<->sync flip, no buffer moved, no spawn removed, no new network buffer, no fsync/sync-write primitive. The slow-write decoupling and the shutdown sequence are untouched. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

…-band AC-pin registry removal Addresses the pair-a convergent BLOCK (distsys F1 + red-team RECONSIDER-PREMISE) on Stage A: removing the periodic AC-pin full-snapshot heartbeat (077f761) left NO bounded-time convergence path for OUT-OF-BAND server-side AcPinRegistry removals on a STABLE long-lived connection. Fix = an event-driven server→worker push (the user-authorized proper fix), replacing the timer the heartbeat was. Invariant being violated: the server's AcPinRegistry converges to the worker's local AC-pin set within a bounded number of ticks after an out-of-band server-side removal, on a stable connection (no reconnect). Mechanism that violates it: src/bin/nativelink.rs:1238 (BIS-ack sweep remove_digests_for_endpoint_batch, fires continuously) + ac_proxy_store.rs:393 (peer-NotFound remove_digests_for_endpoint) remove registry entries OUT-OF-BAND; the worker's own AC-pin set is unchanged so its next tick computes an empty delta and should_skip_blobs_available_tick (local_worker.rs:2253) SUPPRESSES it → divergence persists until reconnect (rare). The heartbeat was the only prior reconciler; Stage A removed it. Mechanism that re-establishes it after the fix: the server PUSHES Update::AcPinResync (proto tag 13) to the affected worker's UpdateForWorker stream; the worker's arm (local_worker.rs Update::AcPinResync) clears last_sent_ac_pin_set via force_ac_pin_resync (local_worker.rs:2226), so the next tick reports the full set as `added`, the skip-gate no longer suppresses, and field 17 (pinned_ac_mirror_entries, REPLACE-semantics) is re-sent → the server's replace_endpoint_ac_pins restores parity. Routing: ApiWorkerScheduler::notify_ac_pin_resync_for_endpoint resolves endpoint→worker_id via endpoint_to_worker and sends on worker.tx. Composite invariants this fix interacts with: - locality_map / AC-pin gossip eventual-consistency (TraceMachina#6): the AC-pin sub-protocol is now bounded-convergent on a stable connection (≤1 tick after the push), not only on reconnect. - The drain-on-ack replay-until-acked channel (Stage 2B) is ORTHOGONAL: it re-sends unacked buffered DELTA chunks; the resync is a separate force-clear of the memo so a NEW full re-advertisement is computed. No interaction. - No internal-RPC timeout added (operator directive): the resync rides the existing server→worker control stream; convergence is event-driven, the reconnect full snapshot remains the backstop. Test that proves the fix re-establishes the invariant: worker side — ac_pin_resync_convergence_test::resync_forces_readvertise_on_stable_connection_no_reconnect server side — ac_pin_resync_routing_test::resync_routes_to_owning_worker_only Proto: additive Update::AcPinResync(AcPinResyncRequest{}) at oneof tag 13 (tag 12 last used, 6 reserved). Empty payload = force-re-snapshot signal (the endpoint is implicit in routing; field 17 is replace-semantics so a removed-digest list would be dead precision — the worker can only re-send the whole set). genproto regenerated via the gen_protos_tool.rs prost-build 0.14.3 harness + diff-verify: the ONLY hunks are tag 13 + the AcPinResync arm + the empty AcPinResyncRequest message (zero unrelated drift; license header preserved). Emit sites (2 of 3 wired; site 3 verified non-convergent and OMITTED): - Site A — BIS-ack sweep (src/bin/nativelink.rs): one push per endpoint per sweep cycle after the registry removal. Coalesced/idempotent (payload-free). schedulers Vec already in scope; fanned to all, the owner routes. - Site B — AcProxy peer-NotFound (nativelink-store/src/ac_proxy_store.rs): fires a bin-injected callback (AcPinResyncNotify, mirroring the existing on_endpoint_wipe pattern) so nativelink-store gains NO nativelink-scheduler dependency. The callback fans to all worker schedulers via a background_spawn. Narrow backstop: a genuine worker eviction self-heals via the worker's own eviction delta; this push covers the transient-NotFound-while-worker-holds case. - Site C — cap-truncation (worker_api_server.rs field-17 handling): OMITTED. Verified non-convergent: replace_endpoint_ac_pins truncates deterministically (entries.iter().take(cap), ac_pin_registry.rs:341) so a forced re-advertisement re-sends the SAME over-cap set → SAME first-cap retained → SAME tail dropped. The removed heartbeat ALSO never converged this class (per its own doc-comment: "over-cap divergence persists until the worker's local AC pin set drops below the cap OR a separate over-cap design ships, tracked as TraceMachina#81"). So Site C is NOT a Stage-A regression and the push cannot fix it; it needs the TraceMachina#81 over-cap design. (confirm-necessity: an element the dispatch named but verification showed unnecessary; rationale recorded here per CLAUDE.md.) Behavior changes: - ApiWorkerScheduler (production WorkerScheduler): NEW — on an out-of-band AcPinRegistry removal for endpoint E, pushes one Update::AcPinResync to E's worker. Prior: no notification → divergence until reconnect. Verified safe: idempotent payload-free signal; unknown/dropped endpoint = silent no-op; routes only to the endpoint's worker (test: resync_routes_to_owning_worker_only). - LocalWorkerImpl (production worker run loop): NEW Update::AcPinResync arm — clears last_sent_ac_pin_set + wakes the periodic loop so the next tick re-advertises the full AC-pin set. Prior: unknown tag → ignored. Verified safe: one-shot force (clear → re-advertise → re-memo); a worker with no blobs_available_state (no fast tier) treats it as a trace-level no-op. - AcProxyStore (production AC peer-fetch wrapper): on peer-NotFound it now ALSO fires the optional resync-notify callback after the registry removal. Prior: registry removal only. Verified safe: callback is None in tests/standalone (removal unchanged); the only added effect is the resync push. - Other WorkerScheduler impls: default trait no-op (no behavior change). - No data-plane / store-chain / ack-semantics change; no internal-RPC timeout. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

…ult-no-op) Implements the settled §12/§13 design (scheduler-m1-v2-pload-refinement-design- 2026-07-01.md). Bounded p_load override for gate ELIGIBILITY (§2) + a fresh-count `p_headroom_pref` PRIMARY ranking key (§12) so the re-introduced stale p_load can never become a magnet. THREE surfaces change (Tier-1, Tier-1.5, fallback); Tier-2 is intentionally left load-blind (§13 Refinement 3, Chesterton c2afb93). Lands DEFAULT-NO-OP: p_idle_threshold_pct=0 → override never fires → exact v1. Invariant-walk (5 slots): - Invariant violated: M1 v1 over-excludes idle-P I/O-bound workers (92% of exclusions have p_load<50%), over-spreading cache-affine work during cold bursts. - Mechanism that violates it: worker_has_p_headroom uses fresh dispatch-count ONLY — a worker at running==p_core_count with idle P cores is excluded. - Mechanism that re-establishes it: §2 bounded p_load override (gate eligibility, worker_has_p_headroom 3-clause) + §12 p_headroom_pref ranking key applied as PRIMARY on Tier-1 / Tier-1.5 / fallback, so the override never becomes a stale-p_load magnet (a genuine free slot always out-ranks an override-admit). - Composite invariants: I1 (P-headroom-first, widened); I2 (no-wedge, soft fallback unchanged — still min_by_key, no filter); I5_Bounded (override admits ≤ p_count*factor, u64 overflow-safe ceiling); I6 (ranker-consistency: genuine-free-slot beats override under stale-low p_load); PrefMonotone (intra-override fresh-count decay via +(running-p_count)); THRESHOLD=0 parity (pref collapses to the v1 two-way order; gate-off → pref≡0). - Tests proving it: test_worker_has_p_headroom_v2_truth_table / _ceiling_bound, test_p_headroom_pref_monotone_intra_override / _threshold_zero_and_gate_off_parity (inline unit, private fns); t_i6_magnet_tier1/_tier15/_fallback_free_slot_beats_override, t_ceiling_bound_override_closes_e2e, t_threshold_zero_parity_no_override (inline production-composition); gate_on_with_threshold_config_plumbs_and_ dispatches + simple_spec_v2_defaults_are_override_off_factor_two (external config-plumbing seam). Config (nativelink-config): two SimpleSpec fields under p_headroom_gate_enabled — p_idle_threshold_pct (bare serde default 0 = override OFF, exact v1); p_headroom_override_factor (NAMED default fn = 2; a bare serde default 0 would make the ceiling p_count*0==0 → override silently never fires, the TraceMachina#6 fidelity trap). Both added to the manual impl Default (NOT derive). The 3rd hardcoded constructor copy (ApiWorkerScheduler::new) now sources default_load_byte_cost / default_assume_core_count / default_p_headroom_override_factor (made pub) — single source of truth, no drift. Code-reviewer tactical fixes applied: (1) worker_has_p_headroom stays a free fn(&Worker) — the two consts are threaded as params, both call sites updated (the :1397 fn-ptr alias is now a const-capturing closure; the fallback sort_key); (2) the ceiling term is overflow-safe u64::from(p_count)*u64::from(factor); (3) factor uses a named serde default fn. Behavior changes (enumerated against production composition): - With p_headroom_gate_enabled=false (DEFAULT, and prod today): NONE. p_gate_active is false → p_headroom_pref ≡ 0 for all workers → every tuple key collapses to the pre-existing key (Tier-1 min load_penalty; Tier-1.5 max blended_s; fallback (0,effective_load_score)); worker_has_p_headroom is unreachable (gate off). The fallback tuple widened bool→u64 but the order is identical. Byte-behavior parity. - With gate ON, p_idle_threshold_pct=0 (the default-no-op landing state): override clause `p_load < 0` never fires → worker_has_p_headroom == exact v1 dispatch-count predicate; p_headroom_pref is {0 free/A5, u64::MAX no-headroom} = the v1 two-way order. Selection identical to gate-on-v1. No behavior change vs the already-deployed v1 gate. - With gate ON, p_idle_threshold_pct>0 (operator opt-in, NOT the landing state): idle-P I/O-bound workers at running>=p_count become eligible up to p_count*factor (I5_Bounded) and are ranked BELOW genuine-free-slot workers (I6). This is the intended refinement; gated behind the soak (§11 necessity). - Tier-2 locality: UNCHANGED (inherits only gate eligibility via worker_is_viable_gated; ranking untouched — §13 Refinement 3). Design-drift flagged (see report): (a) §12.1 pref pseudocode omits A5 — I gave an A5/ungated worker pref 0 (top tier) as §5 I4/§12.3 require it to rank byte-identically to v1; (b) the dispatch named an external test file, but private fns + core-count/running control forced the fine-grained ranker tests inline (external file carries the config-plumbing seam the public API supports). Tests: full nativelink-scheduler (84 lib + integration) + nativelink-config suites green in isolated CARGO_TARGET_DIR=/tmp/cargo-target-m1v2. Every guarded contract mutation-verified (predicate override clause, ceiling factor, pref decay term, pref no-headroom arm, all three ranker surfaces) — each red-failed with its bespoke message, restored green. 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>

Fix s3_store

320f204

allada commented Nov 3, 2021

View reviewed changes

allada merged commit efcb653 into master Nov 3, 2021

allada deleted the s3-changes branch November 11, 2021 06:22

Navigation Menu

Repository navigation

Fix s3_store - #6

Fix s3_store#6
allada merged 1 commit into
masterfrom
s3-changes

allada commented Nov 3, 2021 •

edited

Loading

Uh oh!

allada left a comment

Uh oh!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

allada commented Nov 3, 2021 •

edited

Loading