Repository navigation
Various changes to improve debugging - #5
Merged
Merged
Conversation
Also fixes some spelling errors.
erneestoc
added a commit
to erneestoc/nativelink
that referenced
this pull request
May 22, 2026
Reverts the bounded-concurrency construct on this POC branch. After #1 makes construct metadata-only, intra-tree parallelism is bounded by APFS metadata serialization; on a busy worker (inter-action concurrency already saturates the box) 64-wide spawn_blocking fan-out risks oversubscription and stealing cycles from the compiles. Keeping #1/#2/TraceMachina#5. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
rejuvenile
added a commit
to rejuvenile/nativelink
that referenced
this pull request
Aug 19, 2026
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>
rejuvenile
added a commit
to rejuvenile/nativelink
that referenced
this pull request
Aug 19, 2026
… 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>
rejuvenile
added a commit
to rejuvenile/nativelink
that referenced
this pull request
Aug 19, 2026
… 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>
rejuvenile
added a commit
to rejuvenile/nativelink
that referenced
this pull request
Aug 19, 2026
…diagnosis Resolves the local_worker.rs conflict with the cleanup chain: - Cleanup chain extracted Update::BlobsInStableStorage handler into free function handle_blobs_in_stable_storage (item TraceMachina#5). - Instrumentation branch added info!/warn! inside the inline body that no longer exists. - Resolution: keep the call to the extracted handler; place the stable_storage_gate info! immediately before the call so it fires when the gate is taken. The arm-entry stable_storage_received info! and the else-branch stable_storage_gate warn! ("blobs_available_state is None, dropping unpin") auto-merged correctly outside the conflict region. The non-local_worker.rs portions (server stable_notify_fire, stable_storage_broadcast, scheduler stable_storage_dispatch_per_worker) auto-merged cleanly. Goal: distinguish whether the BlobsInStableStorage ack pipeline is broken at (a) server never broadcasts, (b) server broadcasts but message lost, or (c) worker arm never entered. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
rejuvenile
added a commit
to rejuvenile/nativelink
that referenced
this pull request
Aug 19, 2026
Per testing-czar MISSING-COVERAGE TraceMachina#5 from .claude/reviews/valkey-pool-521c13d1/testing-czar.md. `stores.rs:1571` documents "In Cluster mode the value is ignored" but no test pinned the contract. A future regression that tried to allocate 64-way pools per cluster node would not be caught. Test `cluster_mode_ignores_connection_pool_size` constructs a cluster- mode store via the existing MockRedisConnection scaffold and hammers `RedisManager::get_connection` 64 times (mirroring prod-server.json5's `connection_pool_size: 64`). Asserts every call returns the SAME uuid. Production `new_cluster` always wraps a single ClusterConnection in ClusterRedisManager (single uuid baked in at construction); a future pool-ization regression would surface as multiple distinct uuids and fail this test on the second iteration. Added `connection_manager()` accessor on `RedisStore<C, M>` (#[doc(hidden)]) so the test can issue trait calls directly without mediated Store operations. Production paths already route through the manager via the `M: RedisManager<C>` bound; this just exposes it for tests. Mutation evidence (verified locally): - ClusterRedisManager::get_connection returning Uuid::new_v4() each call (instead of self.uuid) → test fails on the second iteration with "cluster manager must yield ONE stable uuid across all get_connection calls (no per-call pool round-robin)". Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
rejuvenile
added a commit
to rejuvenile/nativelink
that referenced
this pull request
Aug 19, 2026
…eMachina#97 red-team TraceMachina#5) Per red-team finding TraceMachina#5 on the original TraceMachina#97 PR: `next_bis_broadcast_id` resets to 1 on every server start, so a worker holding a stale `BisAck { broadcast_id = 42 }` from a previous server-process can silently drop an unrelated chunk from the new server's resend buffer (the new server allocates broadcast_id=42 for an unrelated broadcast, sees the matching (broadcast_id, sequence) in the buffer, and removes it). Same end-result as the TraceMachina#89 unpin-leak — wrong chunk gets released, its pin state stays leaked. Wire-level fix: * proto: add `server_instance_token: uint64` (field 5) to `BlobsInStableStorageChunk` and `server_instance_token: uint64` (field 3) to `BisAck`. Both default-zero so unset values are detectable as legacy / pre-fixup; treated as always-mismatch today (per task spec: fleet rev's all-at-once). * generated .pb.rs hand-edited to match (consistent with prior BIS commits — Bazel proto-gen is the canonical regenerator). Server-side (`api_worker_scheduler.rs`): * `ApiWorkerScheduler` gains a `server_instance_token: u64` field, generated at `new_with_locality_map` via `rand::rng()`. Loop-until-non-zero so the all-zero token (proto default for legacy workers) cannot accidentally match this server's nonce. Emits an `info!` at startup so operators see the chosen value. * Inject token into every dispatched `BlobsInStableStorageChunk.server_instance_token`. * `bis_ack_received` now takes a `server_instance_token: u64` arg; mismatching acks emit a `debug!` and return early (silently dropped — the chunk waits for a same-token ack on the next reconnect). * `scheduler/Cargo.toml` gains the `rand` dependency (workspace-pinned 0.9.0; same default-features-off + thread_rng config as the existing nativelink-util usage). Worker-side (`local_worker.rs`): * `handle_bis_chunk` echoes `chunk.server_instance_token` into the `BisAck` it emits. Trait + dispatch wiring: * `WorkerScheduler::bis_ack_received` trait signature gains the `server_instance_token` arg (default trait impl is a no-op); `SimpleScheduler` + `ApiWorkerScheduler`'s trait impl forward. * `worker_api_server.rs` BisAck dispatch arm passes `ack.server_instance_token` through. Tests: * `bis_ack_with_stale_server_token_silently_dropped` (api_worker_scheduler.rs): pre-populates buffer, sends acks with `valid_token.wrapping_add(1)`, asserts buffer unchanged. Then sends matching-token acks, asserts buffer drains. Mutation step verified — comment out the `if server_instance_token !=` guard, test panics with the SPECIFIC "stale-token ack must NOT remove the chunk from the resend buffer" message. * `bis_concurrent_ack_and_replay_no_deadlock`: exercises the `bis_ack_received` × `replay_bis_chunks_to_worker` race on the same inner.write() lock. Wrapped in `tokio::time::timeout(5s)` deadlock detector. Asserts no chunk count above dispatched total. * Existing scheduler tests updated to thread `chunk.server_instance_token` through their `bis_ack_received` calls. * Existing worker tests updated to construct `BlobsInStableStorageChunk { ..., server_instance_token: ... }` AND assert `acks[0].server_instance_token` round-trips correctly. TLA+ spec (specs/BISChunkingAck.tla): * Added `StaleTokenAccepted` constant + `StaleAck(c)` action that models a stale-token ack attempting to drop a chunk from the buffer. When `StaleTokenAccepted = TRUE` the action removes `c` from `serverBuffer` (modeling pre-fixup behavior); when FALSE, the action is a no-op (modeling the token check). * `BISChunkingAckBugged.cfg`: `StaleTokenAccepted = TRUE` — `ServerBufferReleaseImpliesUnpin` now violates INDEPENDENTLY of `BugMode` (a stale ack releases a chunk before the worker has unpinned it). * `BISChunkingAckFixed.cfg`: `StaleTokenAccepted = FALSE` — no violations. * `bash scripts/verify_tla.sh`: 13/13 specs PASS, including BISChunkingAck. Verification: * `cargo check -p nativelink-scheduler -p nativelink-worker -p nativelink-proto -p nativelink-service --tests` clean. * `cargo test -p nativelink-scheduler --lib bis_` 6 passed. * `cargo test -p nativelink-worker --test bis_chunk_handler_test --features test-utils` 4 passed. * `cargo test -p nativelink-scheduler --test simple_scheduler_test` 38 passed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
rejuvenile
added a commit
to rejuvenile/nativelink
that referenced
this pull request
Aug 19, 2026
…mplemented) Add three end-to-end tests that codify the existing get_part_and_cache tee contract, exercised through the production CAS chain (ExistenceCacheStore -> VerifyStore -> FastSlowStore): * cdn_cache_populates_local_on_first_peer_fetch (UNDER-action guard): proves the tee populates inner CAS so a subsequent read hits local even when the peer is unreachable. This is the structural-amplification defense that motivates CDN-tee post-TraceMachina#171. * cdn_cache_does_not_populate_on_partial_range_read (OVER-action guard): uses a RecordingInnerStore to detect tee invocation directly, so the test fires on the over-action itself (caching prefix/middle bytes under the full-blob digest) rather than on downstream defenses. * cdn_cache_failure_does_not_fail_read: a FailingUpdateStore as inner proves tee best-effort semantics — peer-fetch must succeed even when inner.update returns Err. Each test is wrapped in tokio::time::timeout(5s) as a deadlock detector with a contract-specific .expect() message. Mutation steps verified for all three: * Test 1: gating cache_write_fut to drain-only -> assertion fails with "CDN tee MUST populate local CAS after peer-fetch". * Test 2: removing the (offset==0 && length.is_none()) guard -> update_calls=1, assertion fails with the over-action message. * Test 3: replacing the warn! log with `?` propagation -> read errors surface to the caller, assertion fails on the best-effort message. Sibling-bug audit (peer-fetch sites in the codebase): * worker_proxy_store::get_part parallel race path (line 2200) does NOT tee when peer wins. Owned by a parallel agent extending TraceMachina#171's INNER_MISS_NO_TERMINATE; not modified here per spec ownership rule. * worker_proxy_store::get_part_and_cache::!should_cache branch (line 907) intentionally bypasses the tee for partial-range and oversized blobs; matches design constraint TraceMachina#5. * bytestream_server::read delegates to store.get_part on the wrapped chain that includes WorkerProxyStore -- no separate peer-fetch path exists outside the proxy. No production code change in this commit; the existing tee at worker_proxy_store.rs:883-1056 (get_part_and_cache) is the implementation under test. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
rejuvenile
added a commit
to rejuvenile/nativelink
that referenced
this pull request
Aug 19, 2026
…aceMachina#334 bundle fixup TraceMachina#5) Code-reviewer MA-6: the Fix C eviction extension's success-path `info!` ("backpressure-driven eviction freed unpinned entries to admit incoming write") at `memory_store.rs:284-291` fires on every eviction- driven admission. In production at line rate that's hundreds of events per second per FastSlowStore — same shape as TraceMachina#262 burst-info demote class. Demoted to `debug!`. The distinct "eviction freed nothing → genuinely full" case is already signaled via the `Err(...)` arm below, where the debug-level message ("every byte is pinned") carries the operationally important info. No information is lost; only production log noise. Removed unused `use tracing::info;` import. All 3 `memory_store_backpressure_eviction_test` regression tests pass after the demote (the tests assert on behavior, not log content). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
rejuvenile
added a commit
to rejuvenile/nativelink
that referenced
this pull request
Aug 19, 2026
…ror} import (TraceMachina#334 bundle fixup CR-1) Fixup TraceMachina#5 (ad7656de "demote backpressure-driven eviction info! to debug!") deleted the `use tracing::info;` line but left the `#[cfg(feature = "chunked_fast_slow")]` attribute above it, which silently re-attached itself to the next `use` statement — `use tracing::{debug, error};`. Default-feature builds were broken (9 errors: `error` and `debug` macros undefined when chunked_fast_slow is disabled). Fix: drop the orphaned `#[cfg]` so `use tracing::{debug, error};` is unconditional. Both `debug!` and `error!` are used in non-chunked-gated paths. Verified: - `cargo build --all` (default features) succeeds. - `cargo build --all --features chunked_fast_slow` still works. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
rejuvenile
added a commit
to rejuvenile/nativelink
that referenced
this pull request
Aug 19, 2026
…BLOCKs closed) The convergent BLOCK from code-reviewer + testing-czar + red-team + dsr on TraceMachina#335 V3 (failed_slow_writes drainer self-retry) was: the V3 walker in `failed_writes_drain.rs:163-186` was a third copy that called `store.inner_store(None)`, which terminates at SizePartitioningStore (the partitioning trap). For the production CAS chain (WorkerProxyStore → ExistenceCacheStore → VerifyStore → SizePartitioningStore(16384) → FastSlowStore) the walker returned `None`, V3 was structurally inactive, and there was no test crossing the production composition. Invariant being violated: V3 self-retry must fire when the server's fast tier still holds the bytes; the walker must locate the FSS inside the production wrapper chain. Mechanism that violated it: `failed_writes_drain.rs:300`'s `store.inner_store(None)` + the local walker recursing via `None` both terminated at SizePartitioningStore (its `inner_store(None)` returns `self`, ptr-eq guard short-circuits). Mechanism that re-establishes it after the fix: canonical helper `wrapper_walker::find_fast_slow_via_chain` + `synthetic_large_key()` sentinel routes the walker into the upper arm. Both consumers (V3 drainer + chunked-dispatcher wiring) call the same helper. Composite invariants this fix interacts with: the walker contract plus the M1 stats invariant `drained == dispatched + no_worker + throttled + send_failed + self_retried + self_retry_failed`. The new test asserts both. Test that proves the fix re-establishes the invariant: `failed_slow_writes_v3_walker_descends_production_composition`. Mutation step physically executed: temporarily replaced all 3 `synthetic_large_key()` calls in `wrapper_walker.rs` with `None` → test red-failed with the bespoke message "V3 walker failed to descend production composition — synthetic_large_key pattern broken: walker returned None for the cas_INNER chain so self-retry was structurally inactive". The M1 observability `warn!` also fired with the named composition. Restored helper, all 8 tests green. Changes: * `nativelink-store/src/wrapper_walker.rs` (NEW): canonical walker + `synthetic_large_key()` sentinel. One source for the V3 drainer + chunked-dispatcher wiring. * `nativelink-service/src/failed_writes_drain.rs`: - Replace local 3rd-copy walker with `find_fast_slow_via_chain`. - dsr M1: emit `warn!` per-tick-per-store when walker returns None (V3-inactive observability — names the composition + the `walker_path = "no_fss_in_chain"` field). * `src/bin/nativelink.rs`: replace local `find_fast_slow_chunked` + `synthetic_large_key` with the shared helper. No behaviour change in production (chunked-dispatcher was already correct). * `nativelink-store/src/fast_slow_store.rs`: doc-comment block on `try_self_retry_slow_write` naming Fix C dependency (PIN_TIMEOUT_SECS=120 must outlive slow-tier outage; cap-rejected digests are intentionally excluded — they never had a fast-tier pin so V3 cannot recover them; admission-side fix is Fix A+B). Closes dsr MINOR-2 + red-team TraceMachina#2 + red-team TraceMachina#5. * `nativelink-service/tests/failed_writes_drain_test.rs`: - dsr M3: BIS push assertion in V3 headline test (`drain_stable_digests().contains(&digest)` after self-retry success — closes the FSS → stable_digests seam contract). - dsr M2 + code-reviewer + testing-czar: NEW test `failed_slow_writes_v3_walker_descends_production_composition` wraps the production composition (ExistenceCacheStore → VerifyStore → SizePartitioningStore(16384) → FastSlowStore + FastSlowStore lower arm) with a 32 KiB digest that routes upper. Asserts walker descent + self-retry + BIS push end-to- end. Mutation step physically run + green-confirm. Out of scope (explicitly deferred): * drain_tick FuturesUnordered+Semaphore parallelism (red-team TraceMachina#4 / dsr M4) — documentation-only until production data warrants; follow-up TraceMachina#338. * Cap-rejected digest recovery — V3's domain excludes them by construction (no pin → no fast-tier replica). Fix lives in Fix A+B (TraceMachina#334) admission gating. * `find_fast_slow_for_pin` (small-key walker) consolidation — TODO(TraceMachina#303) noted in `wrapper_walker.rs` module docs; different semantics (lower arm vs upper arm). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
rejuvenile
added a commit
to rejuvenile/nativelink
that referenced
this pull request
Aug 19, 2026
…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>
rejuvenile
added a commit
to rejuvenile/nativelink
that referenced
this pull request
Aug 19, 2026
…r-branch sibling guard Code-reviewer @ 2d46d33 found a NEW-BUG-INTRODUCED in the prior fixup: on token-mismatch rebuild, `inner.total_accumulated` is decremented at line 549-550 (wipe stale broadcast's entries), then unconditionally overwritten at line 596 by `prev_total.saturating_add(delta)` where `prev_total` was captured BEFORE the wipe. The wipe is undone, leaving stale entries double-counted in `total_accumulated` indefinitely. Invariant being violated: `inner.total_accumulated` MUST equal `sum(broadcasts.values().accumulated_entries)` at all times — this is the bookkeeping invariant the per-conn entries cap admission gate at lines 506-507 reads. Drift causes premature per-conn-entries-cap eviction (legitimate broadcasts dropped once enough token-mismatches accumulate). Mechanism that violates it: line 596 used the pre-wipe `prev_total` snapshot. Mechanism that re-establishes it: read CURRENT `inner.total_accumulated` after the wipe, so the increment is on top of the wiped value. Composite invariants this fix interacts with: per-conn entries cap admission gate (line 506-507) is correct iff total_accumulated matches sum-over-broadcasts. Eviction sites (548-550 token-mismatch, 590-592 validation-fail, 605-607 terminal-commit) all decrement based on snapshotted entries. Pre-merge cap projection (line 506-507) reads total_accumulated. NO pin corner. Tests: - `token_mismatch_drift_total_accumulated_consistency`: 3-step drift scenario; the BETWEEN-step-3-and-4 assertion red-fails pre-fix with the bespoke "expected total_accumulated == M after token-mismatch rebuild, got total_accumulated == N + M — bookkeeping drift not fixed" message (verified mutation-step output stored at /tmp/99-fixup-2-redfail-2018247.log: `left: 100, right: 20`). - `err_branch_does_not_double_subtract`: locks down the Err-branch subtraction safety claim with a multi-broadcast composition where broadcast 2's duplicate-sequence Err must NOT bleed into broadcast 1's contributions to `total_accumulated`. Sibling-bug audit (Err branch at lines 590-592): traced each of the three pre-merge_chunk cases (keep-existing, token-mismatch-rebuild, new-broadcast). `BroadcastAccumulator::merge` returns Err only at validation gates that ALL precede the `accumulated_entries` mutation at line 250. So: - keep-existing case: acc_accumulated_entries == prev_entries, already counted in inner.total_accumulated from prior successful merges. Subtracting it removes those prior contributions correctly. - token-mismatch-rebuild + new cases: acc was just constructed with accumulated_entries=0, never mutated by the failing merge; subtracting 0 is a safe no-op. The wipe at line 549-550 stands. SAFE-DOCUMENTED with inline comment + audit citation at the subtraction site. Verdict on Err-branch sibling concern: SAFE-DOCUMENTED. Mutation step (CLAUDE.md TDD TraceMachina#5): pre-fix red-fail verified at /tmp/99-fixup-2-redfail-2018247.log; post-fix all 21 accumulator tests pass (cargo test result: 21 passed; 0 failed).
rejuvenile
added a commit
to rejuvenile/nativelink
that referenced
this pull request
Aug 19, 2026
…test (testing-czar MAJOR-1) Per testing-czar MAJOR-1 from the TraceMachina#99 fix-up-2 cadre review: `BlobsAvailableAccumulator::new_with_drop_counts` had exactly one production caller (`worker_api_server.rs` near `Arc::clone(&metrics.chunked_blobs_available_drop_counts)`) and zero test callers, so a future refactor that accidentally swapped `Arc::clone` for `Arc::new(ChunkDropCounts::default())` would silently fork the per-connection counters and under-report server-wide drops on dashboards — without any existing test red-failing. The new `new_with_drop_counts_aggregates_across_accumulators` test constructs TWO accumulators sharing one `Arc<ChunkDropCounts>`, drives 3 token-mismatch increments through accumulator A and 5 through accumulator B, then asserts: (1) the shared Arc observes 8 increments, (2) both accumulators' `drop_counts` views observe the same 8 (Arc aliasing), (3) `Arc::ptr_eq` confirms all three handles point at one allocation. Bespoke `assert_eq!` message names the regression and the exact production callsite a refactor would touch. Mutation step (CLAUDE.md TDD TraceMachina#5): swapping `drop_counts` for `Arc::new(ChunkDropCounts::default())` inside `new_with_drop_counts` red-fails the test with `shared_view=0, expected 8` and the bespoke message — confirmed locally then reverted. Test count: 21 -> 22 passing in `nativelink-service::blobs_available_accumulator::tests`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
rejuvenile
added a commit
to rejuvenile/nativelink
that referenced
this pull request
Aug 19, 2026
`BazelChunkedDispatcherImpl::dispatch` used to manually
`set.lock().insert(digest)` then `dispatch_bazel_facing_internal_chunking
.await` then `set.lock().remove(&digest)` in match arms. The await is a
cancellation point: when an upstream `try_join!` (e.g.
`bytestream_server.rs:1761-1775`) short-circuits on a producer Err and
drops the dispatch future, the matched-arm removal NEVER runs. Result:
the digest leaks into `chunked_in_flight_digests` until process restart;
`FastSlowStore::has_with_results` reports `Some(size)` for leaked digests
forever; `ExistenceCacheStore` caches the `Some`; FMB returns "present"
forever; subsequent populates NotFound on the missing bytes. The
production smoking gun was the shutdown WARN at
`fast_slow_store.rs:2012-2017` ("unflushed chunked-path write at
shutdown") firing 3x for `4cb49612...` 10.5h after the cancel.
Fix: introduce `InFlightChunkedGuard`, an RAII guard whose `Drop`
removes the digest and fires the empty-notify. Drop runs on every exit
path including cancellation, so the leak is impossible by construction.
The success path explicitly `disarm`s the guard (turning Drop into a
no-op) and hands the (set, digest, notify) tuple to the post-dispatch
reaper task, which performs removal only after the chunked-driver's
in-flight tracker drains -- preserving the TraceMachina#210 graceful-drain
semantics. The Err path lets the guard's natural Drop perform removal,
matching the prior behavior.
Tests in `nativelink-service/tests/chunked_inflight_cancel_safety_test.rs`:
- `guard_drop_removes_digest`: RAII unit contract.
- `guard_drop_notifies_when_set_becomes_empty`: preserves the
`flush_slow_writes` lost-wakeup contract on the cancel path.
- `cancel_mid_await_removes_digest`: production-shape regression
test. Wraps the guard in a future awaiting a long sleep, races
against a short timer in `select!`, asserts the set is empty after
the dispatch future is cancelled. With the bug present
(manual insert/remove), this red-fails with the bespoke message
"InFlightChunkedGuard MUST remove digest on cancellation -- leak
detected". With the fix, this passes.
- `disarm_prevents_drop_removal`: success-path contract -- after
`disarm()`, Drop is a no-op so the spawned reaper owns removal.
Mutation step (CLAUDE.md TDD TraceMachina#5) verified: commenting out the body of
`impl Drop for InFlightChunkedGuard` red-fails
`cancel_mid_await_removes_digest` with the bespoke message above (and
also red-fails `guard_drop_removes_digest` and
`guard_drop_notifies_when_set_becomes_empty`). Drop body restored.
Invariant being violated: every digest inserted into
`chunked_in_flight_digests` MUST be removed before the dispatcher
returns control to its caller (or, in the success path, before the
chunked-driver's in-flight tracker drains).
Mechanism that violated it: `chunked_write_handler.rs:2462`
unconditional insert paired with match-arm removal at `:2492-2530`,
both bypassed when the inner `.await` is cancelled.
Mechanism that re-establishes it: `InFlightChunkedGuard::drop`
unconditionally removes (when armed), runs on every future exit path
including cancellation.
Composite invariants this fix interacts with:
- TraceMachina#210 graceful-drain (preserved via `disarm()` + spawned reaper)
- lost-wakeup on `in_flight_empty_notify` (preserved -- Drop fires
notify_waiters when the set transitions to empty)
- has_with_results chunked-path Some/None invariant (no longer
poisoned by leaked digests)
Test that proves the fix re-establishes the invariant:
`cancel_mid_await_removes_digest`.
Builds verified: `cargo check --tests -p nativelink-service --features
chunked_fast_slow` (lib + new test), `cargo check -p nativelink-service
-p nativelink-store` (no chunked_fast_slow). 4/4 tests pass.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
rejuvenile
added a commit
to rejuvenile/nativelink
that referenced
this pull request
Aug 19, 2026
Sibling-of-TraceMachina#401 (same file region, same shape, different field). DSR review of TraceMachina#401 surfaced this: `bytestream_server.rs:337`'s `in_flight_writes: Arc<Mutex<HashMap<DigestInfo, watch::Receiver<Option<bool>>>>>` had the identical manual-insert / manual-remove pattern around `tokio::time::timeout(WRITE_TIMEOUT=300s, write_fut).await` (insert at :2395, remove at :2505). Cancellation of the wrapping gRPC future (client disconnect, RST_STREAM, server shutdown) drops `write_fut` mid-await; the manual remove never runs; HashMap entry + watch channel leak permanently per cancelled upload. Invariant being violated: every entry inserted into `in_flight_writes` MUST be removed on every exit path of the primary-writer code, including cancellation. Mechanism that violates it: `bytestream_server.rs:2393-2397` insert; `:2466-2496` `tokio::time::timeout(WRITE_TIMEOUT, write_fut).await` cancellation point; `:2505` `remove(&digest)` only on completion path. 300s cancel window per upload. Mechanism that re-establishes it after the fix: new `InFlightWritesGuard` (RAII). `from_inserted` constructs the guard from state already inserted under the dedup lock (preserves race-freedom versus a concurrent second arrival becoming a duplicate primary). `set_result(bool)` publishes the outcome to coalesced waiters via the watch channel before Drop. `Drop` removes the map entry + drops the `Sender`. On cancellation, `set_result` is never called; Drop's Sender drop signals failure to coalesced waiters via `rx.changed() = Err`, which the production loop at `bytestream_server.rs:2349-2358` already translates to false (failure). Composite invariants this fix interacts with: - watch-channel-disappearance contract for coalesced waiters: dropping the Sender MUST signal failure (preserved — `tx` is dropped in guard's Drop on every exit path). - dedup-lock atomicity: insert + check-then-insert MUST happen under one `in_flight_writes` lock to prevent two RPCs both becoming primary writers (preserved — `from_inserted` skips the insert because the caller already did it under the dedup lock at :2395). Test that proves the fix re-establishes the invariant: `nativelink-service/tests/bytestream_inflight_writes_cancel_safety_test.rs` - `guard_drop_removes_entry`: unit test of the RAII contract. - `guard_set_result_publishes_outcome`: success-path — set_result publishes the outcome to coalesced waiters BEFORE Drop removes. - `cancel_mid_await_removes_entry`: production-shape cancellation test via `select!`; asserts map empty post-cancellation with bespoke message naming bug TraceMachina#402. - `coalesced_waiter_observes_failure_on_writer_cancel`: cross-seam test composing primary writer + coalesced waiter + cancellation; asserts the waiter unblocks with `false` (rx.changed() = Err) within `tokio::time::timeout(2s)` — deadlock detector. Mutation step (per CLAUDE.md TDD TraceMachina#5): commenting out the body of `impl Drop for InFlightWritesGuard` red-fails all 4 tests, with `cancel_mid_await_removes_entry` reporting: "InFlightWritesGuard MUST remove entry on cancellation — leak detected (map still contains 1 entries after cancelled await; this is bug TraceMachina#402 — manual insert/remove around an await is not cancel-safe)". No behavior change on success/error paths: `set_result(result.is_ok())` publishes the same outcome the prior code's `tx.send(Some(result.is_ok()))` did; map removal still happens before function returns (just inside Drop, not as an explicit lock + remove). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
rejuvenile
added a commit
to rejuvenile/nativelink
that referenced
this pull request
Aug 19, 2026
…impls Production wedge 2026-05-13 (PID 3818654, 13:19 - 17:21 PDT, 4h02m): every `/metrics` scrape that raced an `ApiWorkerSchedulerImpl` writer permanently parked one of ~48 tokio worker threads in the `event_listener` FIFO queue. The leak accumulated over ~3h until the runtime starved its accept loops: `/metrics` (port 50061) wedged ~14:01 PDT, then the Bazel port (50051) wedged at 16:36 PDT (`SslHandshakeTimeoutException 10000ms`). Restart required at 17:21:43. RCA: `.claude/audits/wedge-2026-05-13-postmortem.md`. Direct evidence in `/tmp/wedge-rca-stall-1778704391485.log` (TID 3818676 stack frames TraceMachina#5-TraceMachina#10: `event_listener::Blocking::poll_with_strategy` -> `RwLock<ApiWorkerScheduler Impl>::publish` -> `metrics_handler` on `tokio::runtime::scheduler::multi_th read::worker::run`). Root cause: blanket `impl MetricsComponent for async_lock::RwLock<T>` (`nativelink-metric/src/lib.rs:461`, introduced 3ddded5 on 2026-02-18) called `read_blocking()` from a sync trait method. `metrics_handler` (`nativelink-util/src/metrics_publisher.rs:499`) is `async fn` invoked on a tokio runtime worker thread; calling `read_blocking()` parks that worker on the event_listener FIFO queue indefinitely until the writer releases. The doc-comment "It is safe to block in the publishing thread" was load-bearing-false. CLAUDE.md "NEVER block a tokio worker thread" hard rule violated. Composite invariant being re-established: metrics-publisher-call-from-async-context => NEVER blocks the calling thread (skip-on-contention OR async-await OR spawn_blocking) Fix: skip-on-contention via `try_*()`. Stale metrics for one scrape are strictly preferable to a permanently-parked tokio worker thread. A scrape that races a writer publishes a default `Component` for the contended subtree on this tick; the next scrape that catches the lock idle publishes fresh values. Same fix applied to all four blanket impls in this file: - `async_lock::RwLock<T>`: read_blocking() -> try_read() - `async_lock::Mutex<T>`: lock_blocking() -> try_lock() - `parking_lot::RwLock<T>`: read() -> try_read() - `parking_lot::Mutex<T>`: lock() -> try_lock() The `parking_lot` variants are subject to the same wedge mechanism (writer- held lock parks the calling worker thread) even though they don't use the event_listener FIFO queue; same fix is justified by the same hard rule. `tokio::sync::watch::Sender::borrow` (`lib.rs:138`) left as-is: the metrics-side caller holding `&self` IS the writer end of the watch channel, so structural contention against it is not possible. Sibling-bug audit (workspace-wide via `grep -rn 'impl.*MetricsComponent for.*\(RwLock\|Mutex\)' --include='*.rs'`): | crate | site | type | status | |----------------------|-------------------------------|--------------------------|--------| | nativelink-metric | lib.rs:449 | async_lock::Mutex | FIXED | | nativelink-metric | lib.rs:461 | async_lock::RwLock | FIXED | | nativelink-metric | lib.rs:473 | parking_lot::Mutex | FIXED | | nativelink-metric | lib.rs:485 | parking_lot::RwLock | FIXED | | (any other crate) | none | - | n/a | `grep -rn 'read_blocking\|write_blocking\|lock_blocking' --include='*.rs' .` returns ZERO matches workspace-wide post-fix. Tests (`nativelink-metric/src/lib.rs` test module, all `#[tokio::test( flavor = "multi_thread", worker_threads = 2)]`): 1. `publish_does_not_block_on_contended_async_lock_rwlock` 2. `publish_does_not_block_on_contended_async_lock_mutex` 3. `publish_does_not_block_on_contended_parking_lot_rwlock` 4. `publish_does_not_block_on_contended_parking_lot_mutex` 5. `publish_returns_real_value_when_uncontended` (sanity check that the skip-on-contention default isn't masking real values) Each contention test holds a writer guard, then drives `publish` from a fresh `tokio::spawn` (NOT `spawn_blocking` - we want to test the same seam that production uses today, where `metrics_handler` runs on a runtime worker). A `tokio::time::timeout(Duration::from_secs(1))` deadline asserts the publish task completes; pre-fix it parks indefinitely. The bespoke assertion message is "publish() blocked tokio worker thread on contended <lock> (exceeded 1s deadline; pre-fix would park indefinitely)" so the failure mode is unambiguous; without it a generic timeout-elapsed would be misleading. Mutation verification (per CLAUDE.md TDD rule): each fix was reverted in turn and the corresponding test red-failed with the bespoke message. Verified 2026-05-13. Re-enabling `services.metrics: {}` in production after this commit ships will no longer wedge the server. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
rejuvenile
added a commit
to rejuvenile/nativelink
that referenced
this pull request
Aug 19, 2026
Closes the BLOCK + MAJOR set surfaced by the TraceMachina#494-v3 cadre audit at HEAD 5d7d08ac. The bundle ships with v2 default-on; this commit hardens the v2 path against the half-done H1 reader cascade, the missing QueryWriteStatus phantom-success guard, the v1 deferred-publish phantom-Ok on commit failure, and the chunked_in_flight set lacking refcount semantics for concurrent same-digest sessions. BLOCK-A (QueryWriteStatus phantom-success guard): - nativelink-service/src/bytestream_server.rs inner_query_write_status now mirrors the H2 pattern from bytestream_write: downcast to FastSlowStore, consult is_chunked_in_flight, return complete=false when the digest is in chunked_in_flight_digests. Pre-fix, the has()=Some cascade returned complete=true while the v2 commit could still fail, breaking Bazel's durability contract. - Test: bytestream_server_test.rs::block_a_query_write_status_does_ not_phantom_ack_for_chunked_in_flight. Mutation-verified red-fail with the bespoke "QueryWriteStatus phantom-acked in-flight chunked digest" message. BLOCK-B (H1 reader-cascade Option F1 — read-blocks-on-commit): - nativelink-store/src/fast_slow_store.rs get_part now waits (bounded 5s polling at 50ms) when the digest is in chunked_in_flight_digests AND the chunked_read_registry pin has fallen through (v2 case: no driver pin). Closes the gap where has_with_results said Some (via the in-flight set) but get_part returned NotFound (canonical not yet on disk + no pin to serve). - Test: chunked_write_handler_v2_test.rs::block_b_h1_reader_cascade_ blocks_on_v2_inflight_then_serves_from_slow. Test simulates v2 commit landing during the wait; reader observes correct bytes. Mutation-verified red-fail with bespoke "H1 reader-cascade returned NotFound during v2 in-flight window" message. BLOCK-D (H2 production-composition test): - New test bytestream_server_test.rs::block_d_bytestream_write_h2_ does_not_phantom_ack_when_chunked_in_flight. Wraps FSS + chunked_in_flight_digests + ByteStreamServer in production composition; asserts second concurrent ByteStream::write does NOT phantom-ack. Mutation-verified red-fail with bespoke "ByteStream:: write phantom-acked second concurrent writer while chunked commit was still in-flight" message. BLOCK-E (subscribe → enable → peek → notified): - nativelink-service/src/chunked_write_handler_v2.rs::v2_await_commit_ result and chunked_write_handler.rs v1 AwaitCommit branch now call tokio::pin!(notified) + notified.as_mut().enable() BEFORE peek. The dispatch-prompt's missed-wakeup hypothesis is mitigated in tokio 1.49 by notify.rs:572's counter-capture-at-creation, but enable() is defense-in-depth against future tokio API drift AND self-documenting (eager registration is the intent). - Test: chunked_write_handler_v2_test.rs::block_e_missed_wakeup_ publish_before_first_poll_is_observed. Verifies publish-between- subscribe-and-first-poll is observable through notified.await. - Doc-comment correction: source code now documents that the classical missed-wakeup is NOT active in tokio 1.49 (counter-capture handles it), and explains why enable() is still worth applying. MAJOR-F + MAJOR-H (deferred-publish reactive instead of busy-spin): - chunked_write_handler.rs::dispatch_chunks_to_driver gains an async_result_relay parameter; the AsyncCommit branch threads it to run_async_commit_reaper. dispatch_bazel_facing_internal_chunking also gains the parameter; the BazelChunkedDispatcherImpl::dispatch Owner branch creates a oneshot pair, passes the sender into dispatch, and the deferred-publish task AWAITS the real commit_result instead of polling in_flight.contains_digest. - Eliminates: busy-spin on yield_now (N×100us ticks during multi-second commits), synthetic-Cancelled trap on spawn drop, phantom-Ok on commit failure (the prime DS hazard). - Test: chunked_write_handler_v2_test.rs::major_f_deferred_publish_ propagates_commit_err_to_sibling_v2. Uses a hash-mismatch payload to force commit failure; sibling v2 in AwaitCommit MUST observe Err (not phantom-Ok). Mutation-verified red-fail with bespoke "deferred-publish published Ok despite v1 commit failure" message. MAJOR-G (chunked_in_flight_digests refcount): - Type change: Arc<Mutex<HashSet<DigestInfo>>> → Arc<Mutex<HashMap<DigestInfo, NonZeroU32>>> in fast_slow_store.rs + chunked_write_handler.rs. InFlightChunkedGuard::new bumps refcount; Drop decrements and removes only when refcount reaches zero. - Pre-fix: two concurrent v2 sessions for the same digest both "inserted" (idempotent on HashSet), but the first to drop removed the entry while the second was still in-flight. BLOCK-B's reader wait then misses the second session and gets NotFound from the not-yet-committed slow tier. - All call-sites updated: .contains(&d) → .contains_key(&d), iter() preserves (digest, refcount) tuple for shutdown-time warnings, test manipulations use lock().insert(digest, NonZeroU32::new(1).unwrap()). - Test: chunked_inflight_cancel_safety_test.rs::major_g_refcount_ keeps_digest_visible_until_both_guards_drop. Two guards on the same digest; assert refcount sequencing through 0→1→2→1→0 with entry visibility check at each step. Mutation-verified red-fail with bespoke "refcount removed digest while second guard still in-flight" message. MAJOR-I (H2 doc-comment honesty): - bytestream_server.rs:2742 doc-comment now reflects actual behavior: the H2 fall-through to in_flight_writes only dedups OTHER ByteStream::write callers (v2-chunked sessions don't populate in_flight_writes). For v2-in-flight cases, second writer becomes the primary and races; CAS convergence (digest = content) makes on-disk byte sequence well-defined. The H2 guard is "don't lie about durability" — NOT "serialize against the chunked writer." MAJOR-J (TLA+ test assertion message divergence): - chunked_write_handler_v2_test.rs::tla_all_attached_writers_terminate_ within_70s_v1plus2v2: the v1_size + v2 commit assertion messages now match what mutation of single_stream_owner actually catches ("failed to stat chunked partial ... NotFound" — i.e. v1+v2 racing on the .partial file, NOT a 70s timeout). MAJOR-K (chunked_v2_enabled=false rollback path): - New test chunked_write_handler_v2_test.rs::major_k_chunked_v2_ disabled_returns_unimplemented_for_v2_rpcs. Constructs adapter with v2_enabled=false; asserts v2 RPCs return Unimplemented with "chunked_v2_enabled" in the message; v1 RPC still works. Out-of-scope (file follow-up trackers): - DS MAJOR-1 (latent v1 AsyncCommit code path; gated by bazel_facing_internal_chunking=false default; eliminated by MAJOR-F refactor when the v1 path is enabled). - DS MINOR-1 (chunked_in_flight TTL/sweeper backstop — MAJOR-G's refcount + RAII covers the common case). - code MAJOR-1 (probabilistic metric), code MAJOR-3 (err_tip chaining), code MINOR-1/2/3, auditor MINOR (1-off test count, TLA+ line cite). - red-team TraceMachina#5 (pilot-vs-default-flip — user already decided). - red-team TraceMachina#8 (hand-edited pb.rs — pre-existing TraceMachina#87/TraceMachina#346). - Pre-existing test failure: bazel_facing_internal_chunking_test.rs:: fast_tier_ok_dispatch_err_records_failed_slow_write — the typed-BackpressureSignal early-return at fast_slow_store.rs:1654 (aeaf078) bypasses failed_slow_writes insertion. NOT a regression from this commit; flagged for follow-up tracker. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
rejuvenile
added a commit
to rejuvenile/nativelink
that referenced
this pull request
Aug 19, 2026
Adds 4 tests in `nativelink-service/tests/writer_site_logs_test.rs`: - `fss_registration_emits_info_with_writer_path_label` — production `InFlightChunkedGuard::new_with_caller` emits one info! line tagged `registry=fss_chunked_in_flight_digests` with the supplied caller label. - `fss_removal_emits_paired_info_with_outcome_drop` — Drop emits a paired removal line so a journal scan can compute in-flight duration per writer path. - `fss_disarm_emits_info_with_outcome_disarm` — the success-path hand-off emits one line with `outcome=disarm` so a journal scan distinguishes "reaper-owned removal" from "silent drop bug". - `v1_server_handler_local_insertion_label_contract` — pins the operator-facing label contract for the v1 server-side handler-local in_flight insertion. Adds 3 tests in `nativelink-store/tests/chunked_client_test.rs`: - `worker_write_chunked_stream_emits_entry_log` — worker-side write_chunked_stream emits `writer_path=worker_chunked_client` with the blob digest + expected_size. - `v1_worker_dispatcher_dispatch_emits_wire_shape_log` — `WorkerApiWriteChunkedDispatcher::dispatch` emits `writer_path=worker_dispatch_v1 + wire_shape=v1`. - `v2_worker_dispatcher_dispatch_emits_wire_shape_log` — same for v2 dispatcher with `wire_shape=v2`. Each test has a bespoke assertion message naming the source file:line of the deletable info! emission, so a mutation that removes the log red-fails with actionable attribution. Mutation verification (per CLAUDE.md TDD TraceMachina#5): manually commented out the v1 worker dispatcher's info! and re-ran the test — `v1_worker_dispatcher_dispatch_emits_wire_shape_log FAILED` with the expected bespoke message. Restored the info!, test passes again. Clippy fixups (no behavior change, addresses `clippy::map_unwrap_or` lint introduced by the new digest_str / peer_addr blocks): - chunked_client.rs: 2× sites use `map_or_else` instead of `.map().unwrap_or_else()`. - chunked_write_handler.rs / _v2.rs: peer_addr resolution uses `map_or_else`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
rejuvenile
added a commit
to rejuvenile/nativelink
that referenced
this pull request
Aug 19, 2026
…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>
rejuvenile
added a commit
to rejuvenile/nativelink
that referenced
this pull request
Aug 19, 2026
MAJOR-4 (red-team R8) — H3 Fixed cfg doc-comment reframed: explicitly names that the 5 structural invariants hold in BOTH the bugged and any hypothetical fixed module; none discriminate the H3 hazard. H3 fix proof remains in ChunkRaceReadersV2Fixed.cfg. The Fixed cfg is a gate- classifier-satisfying stopgap until the AllowEvictingMapInsert refactor lands (Phase 5 TraceMachina#5). MAJOR-5 (distributed-systems MAJOR-1) — spec TWO DISTINCT PRODUCTION SEAMS block now contains an ABSTRACTION CAVEAT: 'seamBPropagated' is a single global monotonic boolean; production 'ReaderState::maybe_get_part_result' is per-RPC (constructed fresh =None inside each inner_read unfold at bytestream_server.rs:1688). Spec is strictly weaker than production in admitting race-loser interleavings; under-models, never over-models. 'At most one in-flight RPC sees the race-loser' cannot be expressed in this spec. MAJOR-6 (distributed-systems MAJOR-2) — spec PRODUCTION SHAPE block now enumerates TWO READER PATHS: Path A is StreamingBlobReader::next_chunk (streaming_blob.rs:631) with NO tokio::select! race; Path B is the inner_read unfold w/ buf_channel (bytestream_server.rs:1712-1873) with the seam-B race. Spec collapses both into one FSM (UNION-models); race-loser DoneSilent* is unreachable in Path A in production. MAJOR-7 (distributed-systems MAJOR-3) — WriterTerminationDisjunct doc-comment now states the CFG CONTRACT: every cfg MUST satisfy 'at least one Allow*=TRUE' OR rely on WriterSendChunk reaching chunkCount=NumChunks; reviewers verify per-cfg.
rejuvenile
added a commit
to rejuvenile/nativelink
that referenced
this pull request
Aug 19, 2026
… AlreadyExists tonic_ok Two convergent reviewers (assumption-auditor N2 + perf-optimizer N4) flagged metric TraceMachina#5 (server_stable_digests_pusher_invoke_count) as incomplete: three direct-push sites bypass stable_digests_pusher: - fast_slow_store.rs:2616 try_self_retry_slow_write success arm - fast_slow_store.rs:5244 failed-write drain re-broadcast - fast_slow_store.rs:5498 failed-write drain re-broadcast These directly push to stable_digests.lock() + notify, so they DO get broadcast (queue_depth gauge reflects them) but they do NOT bump pusher_invoke_count and they do NOT populate pusher_timestamps. The pre-rename metric overstated its scope; per-digest bis_broadcast_queue_latency silently drops samples for those digests. Fix (option B from reviewer reports — smaller change, honest naming): - Rename metric `server_stable_digests_pusher_invoke_count` → `server_stable_digests_pusher_invoke_via_commit_path_count`. - Expand the field doc-comment to enumerate the three bypass sites and warn operators that this is NOT total BIS broadcast traffic (queue_depth gauge IS total). - Update last_at gauge help text to surface the NTP-step caveat (distributed-systems MINOR-1) and cross-reference _count for monotonic verification. Also addresses perf-optimizer N4 (AlreadyExists → tonic-Ok cache leak): drop the record_tonic_ok call from the Code::AlreadyExists arm at running_actions_manager.rs:5001-5010. When the slow tier short-circuits update for an already-present blob, stable_digests_pusher does NOT fire → no BIS chunk for this digest → the cache entry would sit for 10 min until TTL evicted it AND the pin-release histogram would be polluted by samples that never had a matching BIS arrival. Pure observability semantic: the metric reflects fresh-write semantics; AlreadyExists is correctly out of scope. Tests: updated publish_emits_metrics_via_render_prometheus to use the renamed metric path; all 13 phase0_metrics tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
rejuvenile
added a commit
to rejuvenile/nativelink
that referenced
this pull request
Aug 19, 2026
…na#502 (cross-crate miss) streaming_blob_reader_failpoint_returns_error wrote 512 of 1024 declared bytes then send_eof — encoding, incidentally, exactly the silent-short case that TraceMachina#502 (48be7c6, 2026-05-16) made a guarded error at the reader. TraceMachina#502 scoped its caller analysis to nativelink-util and updated that crate's tests; this nativelink-store integration test was outside that scope and has been red on main for 3+ weeks (invisible because the failpoints CI step only runs on the GitHub fork remote — gitforge origin main has no CI; follow-up tracked separately). Triage verdict (read-only agent + reviewer verification): STALE-TEST, not a production bug — the TraceMachina#502 defense is correct, production-active, and guarded by 8 dedicated tests in nativelink-util (incl. silent_short_eof_surfaces_as_internal + the production-shape populate_path_silent_short_propagates_to_reader_as_err). The 2026-05-15 download-truncation incident is unrelated (OOM cross-PID stitching). Fix: deliver the remaining 512 bytes (chunks 3+4, asserted) before send_eof so the test exercises its actual contract — failpoint RECOVERY → clean EOF — plus a 5s timeout deadlock detector and bespoke expect messages on the EOF read. Behavior changes: none (test-only). Reviewers: code-reviewer + testing-czar, both APPROVE; findings (comment wording, std::time::Duration consistency, inner unwrap → bespoke expect) addressed. testing-czar confirmed no coverage loss. Suite: 14/14 green (/tmp/fp5-final log). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
rejuvenile
added a commit
to rejuvenile/nativelink
that referenced
this pull request
Aug 19, 2026
…rmanent-anchor doc claims Addresses the bench-harness cadre findings on 7c5cdbde (red-team P1 + assumption-auditor TraceMachina#1-5 + review-pair-b nit). Bench-harness-only; no production code touched (only benchmarks/src/scenarios/worker_upload.rs). CONVERGENT FINDING (red-team P1 + assumption-auditor TraceMachina#5): inter-iteration async-spawn bleed in the *_rpc3ms cells. inner_upload_results uploads stdout/stderr + Tree/Directory protos via update_oneshot; FastSlowStore::update_oneshot spawns the slow write DETACHED (fast_slow_store.rs:5785), not awaited by upload_results(). The trait-default update_oneshot (store_trait.rs:1078) routes to LatencyInjectingStore::update → 3 ms sleep. measure() (mod.rs:200-202) starts the next sample's timer the instant body().await returns, so without a drain each non-F2 iteration left >=2 detached 3 ms-sleep tasks running into the NEXT sample's window — an uncontrolled, scheduler-/core-count-dependent bleed (the same async-spawn nondeterminism that disqualified N1, smaller amplitude). F2 + no_latency cells were already clean (no slow-tier spawn / 0 ms sleep). Invariant being violated: a permanent latency-anchor cell must measure one self-contained upload, reproducibly — not one upload plus the scheduler- dependent tail of the previous iteration's detached slow writes. Mechanism that violates it: fast_slow_store.rs:5785 detached tokio::spawn of the slow write, never awaited by upload_results(); the bench had no inter-iteration drain. Mechanism that re-establishes it: drain_slow_writes (worker_upload.rs) awaits the slow-write map to quiesce to 0 at the head of each timed body, using the production quiescence signals in_flight_empty_notify_handle() + in_flight_slow_write_count(), with the lost-wakeup-safe pin!+enable()-before- predicate pattern copied from FastSlowStore::flush_slow_writes (fast_slow_store.rs:2360-2362). It is NOT flush_slow_writes (that sets shutting_down=true permanently, fencing later iterations' spawns); it only waits. Composite invariants this fix interacts with: - Timed window: drain sits at the body head because measure() has no out-of- window teardown slot; in the common case the map is already empty (the 3 ms detached writes finish during the longer upload) so the settle is near-zero and bounded, not part of the measured upload cost. Bounded by SLOW_WRITE_DRAIN_DEADLINE = 5 s; a residual there panics rather than silently inflating a sample. - F2 / no_latency cells: drain returns immediately (no slow-tier spawn / 0 ms sleep), so the fix is inert for the already-clean cells. - Same cas_store Arc is shared by the manager and the drain (worker_upload.rs :828 vs :887), so the drained map is exactly the one the uploads populate. Test that proves the fix: u1_drain_slow_writes_quiesces_in_flight_map — drives one detached slow write through the bench FastSlowStore (3 ms latency, so the spawned task is provably mid-flight), asserts the in-flight count is non-zero, then asserts drain_slow_writes returns 0 and the map is empty, under a tokio::time::timeout deadlock detector. Mutation-verified: replacing the notified await with an immediate return red-fails with "drain_slow_writes MUST quiesce the in-flight map (return 0) — a non-zero residual means the detached slow-write tail would bleed into the next sample's timed window" (left: 1, right: 0). Drain-vs-document choice: chose the DRAIN (red-team option a), not the document-as-pipelined fallback (option b). A real quiescence signal exists (in_flight_empty_notify + in_flight_slow_write_count), the wait is cheap and deterministic (a definite signal, not sleep-as-sync), and a regression anchor is far better served by isolated single-upload samples than by documenting a scheduler-dependent pipeline tail. The one honest caveat — measure() has no out-of-window teardown slot, so the drain is at the body head rather than strictly between samples — is documented in both the helper and the cell doc; because the tail is near-zero by the time the next body begins, the body-head drain is a near-zero settle, not measured upload cost. Permanent-anchor doc claims corrected (assumption-auditor FIX-FIRST): 1. "valid + shippable" / baseline discipline: clarified "valid" = the timed- window STRUCTURE (code-proven), NOT the numbers; the cell's p50/p99 are NOT yet a collected baseline (no U1 run exists); 100-file p99 stability at iters=20 is asserted by reasoning, not a two-run variance check. 2. "Tier-3 cadre would block any N1 seam": corrected. The COARSE output-upload window is ALREADY persisted in ExecutionMetadata (output_upload_start_timestamp running_actions_manager.rs:3804 -> output_upload_completed_timestamp :4329 -> output_upload_ms :4436), free to read. Only the FINE intra-phase split (batch_start - upload_start) lacks a field, and even that needs only a #[cfg(test)] sink through the already- injected Callbacks, not a hot-path prod hook. Honest reason N1 is deferred: OUT OF SCOPE for a bench fix-up, not impossible. 3. 3 ms "observed on buildcache/banjo": dropped "observed". Reframed as an explicit order-of-magnitude MODELLING choice for the has() RTT; added the repo's real figures for calibration (wire ~0.2 ms; remote write-leg p50 ~152 ms / p99 ~720 ms, n=140) and noted 3 ms models the lightweight has() RTT, not the heavy update() write leg (so no conflict with 152 ms). 4. iters 5->20 "stable": softened to "more samples; first-sample outlier avoided; baseline not yet collected; stability unverified pending a two-run pass". 5. Differentiation story corrected: 3 ms differentiates no_latency<->rpc3ms reliably via the INLINE batch has() (+ foreground file update legs); stdout/proto update_oneshot latency is on the detached spawn and is now deliberately drained OUT of the sample (not racy-in-window); no_latency vs f2_mode are BOTH 0 ms — differentiated by the fast-store WRITE PATH, not by injected latency. review-pair-b nit (Issue 3): the register_item_callback comment cited fast_slow_store.rs:771 for the warn; the (false, Ok) arm's warn! macro is at :779. Fixed. Behavior changes: NONE in production. Bench-harness behavior change: each U1 rpc3ms sample now excludes the prior iteration's detached slow-write tail (drained to quiescence at the body head) — the cells now measure isolated single-upload latency instead of upload-plus-bleed. F2 + no_latency cells unchanged (drain inert). Tests: full benchmarks lib suite 56 passed (was 55, +1 drain test). The 15 clippy errors under --tests are all in nativelink-error/src/lib.rs (a dependency, untouched) — pre-existing toolchain drift, not this change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
rejuvenile
added a commit
to rejuvenile/nativelink
that referenced
this pull request
Aug 19, 2026
…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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Also fixes some spelling errors.
This change is