Fix buf in bytestream_server when NotFound was returned by allada · Pull Request #10 · TraceMachina/nativelink

allada · 2021-11-04T22:09:23Z

Fixes bug found where the backend store could return NotFound but
would deadlock the request because the spawn that received the
error was never awaited properly.

This change is

Fixes bug found where the backend store could return NotFound but would deadlock the request because the spawn that received the error was never awaited properly.

allada

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

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

TraceMachina#4 + TraceMachina#10) Addresses Fix TraceMachina#4 (BLOCK, 4-reviewer convergence: code-reviewer Critical TraceMachina#2, testing-czar CRIT-1, red-team identify-the-seam, dsr MAJOR-3) and Fix TraceMachina#10 (testing-czar CRIT-4 dead-code reference cleanup). NEW: nativelink-service/tests/blobs_available_chunked_e2e_test.rs Three production-composition tests crossing every named seam from worker chunker through the server's locality_map. Each test explicitly enumerates the seam contract in the file's doc-comment (per CLAUDE.md "identify-the-seam discipline"): 1. Producer: chunk_blobs_available() 2. Wire envelope: Update::ChunkedMessage(ChunkedMessage { payload: Some(chunked_message::Payload::BlobsAvailable(chunk)) }) 3. Server dispatch arm: worker_api_server.rs:876-928 4. Server accumulator: BlobsAvailableAccumulator::merge_chunk 5. Path A commit: handle_blobs_available 6. Locality-map write: register_blobs_iter Tests: * chunked_5k_entry_snapshot_populates_locality_map: sends 5K entries in 2 chunks, asserts locality_map.digest_count() == 5000 within 5s, spot-checks first/last digest reach the endpoint. * chunked_50k_entry_snapshot_populates_locality_map: sends 50K entries in ~13 chunks, asserts convergence within 10s. * chunked_oversized_snapshot_rejected_by_chunker: drives 1 + 256×4096 entries; chunker MUST return Err (Fix TraceMachina#2 producer-side cap); locality_map MUST remain empty. Each `tokio::time::timeout` deadline carries a SPECIFIC `.expect()` message naming the seam ("production composition: worker chunker → wire → server accumulator → locality_map.write — must converge for N-entry snapshot"), not a generic `is_ok()` (which would let a generic `Elapsed` mask the real bug per CLAUDE.md mandate). MUTATION STEP EXECUTED: Commented out the entire `Update::ChunkedMessage` arm body in worker_api_server.rs:876-928 (replaced with `let _ = envelope; Ok(())`). Re-ran chunked_5k_entry_snapshot_populates_locality_map. Test red-failed with the bespoke message: thread 'chunked_5k_entry_snapshot_populates_locality_map' panicked: assertion `left == right` failed: production composition: worker chunker → wire → server accumulator → locality_map.write — must converge for 5000-entry snapshot; saw 0 entries Restored the dispatch arm; reran; PASS. The seam test demonstrably guards the seam. UPDATED: nativelink-worker/tests/utils/local_worker_test_utils.rs * Added `expect_chunked_message` helper mirroring the `expect_blobs_available` / `expect_bis_ack` pattern. Pulls the next ChunkedMessage off the mock's call channel and lets the caller assert on its payload. * Removed the dead-code reference in the `chunked_message` impl that claimed tests `blobs_available_chunked_*` existed when they didn't (testing-czar CRIT-4). Replaced with a pointer to the nativelink-service e2e test that DOES exist. INVARIANT WALKED: * Composite invariant: chunked envelope hits dispatch arm → fires accumulator merge → on terminal+contiguous, fires handle_blobs_available → register_blobs_iter populates the locality_map. The e2e test composes every link. * Mutation step proves the test guards the LINK (the dispatch arm), not just the unit (merge_chunk). * Failure mode coverage: producer-side cap fires before any chunk crosses the wire (chunked_oversized_snapshot_rejected_by_chunker). NOTE on Fix TraceMachina#10 minimal scope: The user's prompt for Fix TraceMachina#10 also asks for worker-side tests of the chunked emit path. The chunker unit tests in nativelink-util/src/blobs_available_chunking.rs (11 tests) already prove `should_chunk` + `chunk_blobs_available` correctness. The e2e test in this commit composes the same chunker through the real server, indirectly exercising the worker-emit shape via the chunker's contract. Adding a third worker-side mock test would duplicate coverage; the `expect_chunked_message` helper is in place for any future test that drives the LocalWorker periodic emit loop specifically. Reviewers cited: code-reviewer Critical TraceMachina#2, testing-czar CRIT-1 + CRIT-3 + CRIT-4, red-team identify-the-seam, dsr MAJOR-3. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

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

…CacheStore construction Invariant: an ECS over a registered-but-ref-wrapped backend must register its invalidation callback (the ECS stale-positive triangle's eviction corner). Violated: ECS::new_with_time queried supports_removal_callbacks() on a freshly-created RefStore (cell=None) — false-for-unresolved (e8b1498/TraceMachina#367, deliberate, preserved) latched vulnerable_mode=true at EVERY boot of this binary+config; AC_INNER's eager-invalidation callback was never registered on REDIS_AC_STORE (1 of 2 callbacks). Plausibly present since the config landed ~May 11 (pre-deploy journal retention prevented earlier detection). Fix: explicit inner_store(None) resolution before the query — mirrors what FastSlowStore::new_validated does accidentally at :1445. distsys traced the post-fix composite end-to-end: callback registers on the memory tier immediately and deferred-replays onto REDIS_AC_STORE at first Redis op (notifications default-on). Boot-order safe (declaration order + graceful degradation on missing names — guarded by test). Tests: TDD red-verified (timestamp-proven chain) + mutation red with bespoke 'TraceMachina#9' message; missing-name negative-contract test; callback liveness via eviction→ECS-clear with timeout-pattern poll. [[test]] required-features stanza added (testing-czar BLOCK — plain cargo test refused clearly instead of E0599). Guard comment added on RefStore::supports_removal_callbacks against future auto-resolve cleanup. Reviewers 7/7 (records .claude/reviews/acbb767d-ecs-fix/): perf/code/ auditor/security/distsys/red-team landable; testing-czar BLOCK closed in review batch. Follow-ups: TraceMachina#10 (FastSlow sibling), TraceMachina#11 (callback-chain observability: vulnerable_mode=0 can mask a dead chain; FastSlow capability honesty; metric help-text; get_store-abort edge; production-shape propagation test). Behavior changes: AC_INNER ECS registers eager invalidation at boot; the startup vulnerable_mode ERROR no longer fires for ref-wrapped registered backends. Closes TraceMachina#9. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

…k-chain correctness (ECS/FSS/RefStore) ## Invariant Walk Invariant being violated: A store reporting supports_removal_callbacks()==true must actually fire callbacks on every eviction observable to the composite; any composition layer that returns true unconditionally without querying its inner stores leaves ECS in a false-safe state where vulnerable_mode=false but no callbacks ever fire. Mechanism that violates it: - fast_slow_store.rs: no supports_removal_callbacks override → trait default `true` inherited unconditionally; ECS(FastSlow{*, GrpcStore}) would see supports=true, register a dead callback, and never fire on GrpcStore-side evictions. - fast_slow_store.rs:756-area register_slow_eviction_stable_set_listener called supports_removal_callbacks() on a possibly-unresolved RefStore; an unresolved RefStore returns false by design (e8b1498/TraceMachina#367 contract), so the BIS listener would log a false warn and skip registration. - ref_store.rs:get_store loop: `store.register_item_callback(callback)?` returned Err WITHOUT writing the cell — permanently bricking the RefStore; every subsequent op re-entered the slow path and failed forever. Mechanism that re-establishes it after the fix: - fast_slow_store.rs: new supports_removal_callbacks override resolves both tier stores via inner_store(None) before querying, then returns fast.supports() && slow.supports(); ECS gets the correct composite answer. - fast_slow_store.rs:register_slow_eviction_stable_set_listener: calls slow_store.inner_store(None) before supports_removal_callbacks(), forcing RefStore cell population so the capability query sees the resolved store. - ref_store.rs:get_store: writes the cell BEFORE replaying callbacks; replay Err is logged + counted but resolution is considered succeeded (degraded, not bricked). Composite invariants this fix interacts with: - ECS vulnerable_mode triangle: gate (supports=false → vulnerable_mode=true) OR eager-invalidation-via-callback; fix ensures the gate correctly reflects whether callbacks can fire end-to-end. - BIS listener registration: BIS ack requires the stable-set callback path fires on every slow-tier eviction; TraceMachina#10 fix ensures direct FastSlowStore::new callers with ref-wrapped slow stores don't silently lose the listener. - RefStore resolution contract: after get_store() returns Ok, the cell is permanently written and all ops delegate to the resolved store even if callback replay partially failed. Test that proves the fix re-establishes the invariant: - capability_query_callback_chain_test.rs (all 9 tests, 0 failures): - fast_slow_supports_callbacks_false_when_slow_does_not (TraceMachina#11 Item 2) - fast_slow_supports_callbacks_true_when_both_tiers_support (TraceMachina#11) - fast_slow_supports_callbacks_true_when_slow_ref_resolves_to_memory (TraceMachina#11) - ref_store_get_store_not_bricked_by_callback_replay_failure (TraceMachina#11 Item 5) - ecs_over_ref_fast_slow_slow_tier_eviction_clears_cache (TraceMachina#11 Item 6) - ecs_invalidation_callbacks_fired_counter_increments (TraceMachina#11 Item 3) - fast_slow_new_ref_wrapped_slow_registers_bis_listener (TraceMachina#10) - ecs_over_fss_no_callback_slow_enters_vulnerable_mode (TraceMachina#11 Item 2 over-action) - fss_supports_callbacks_false_when_fast_does_not_and_rule (TraceMachina#11 AND-rule pin) ## Changes ### Item 1 (TraceMachina#10) — resolve RefStore before BIS listener capability query fast_slow_store.rs: `register_slow_eviction_stable_set_listener` calls `slow_store.inner_store(None::<StoreKey<'_>>)` before `supports_removal_callbacks()`. Forces RefStore cell population so the unresolved-cell false-return doesn't trigger a misleading warn. ### Item 2 (TraceMachina#11) — FastSlowStore supports_removal_callbacks honesty New `StoreDriver::supports_removal_callbacks` override on FastSlowStore resolves both tiers via `inner_store(None)` then returns `fast.supports() && slow.supports()`. Pre-fix: trait default `true` inherited unconditionally. AND rule is over-conservative for ECS stale- positive prevention (slow-only suffices) but kept as defense-in-depth. ### Item 3 (TraceMachina#11) — ECS invalidation_callbacks_fired counter New `CounterWithTime` field on ExistenceCacheStore, incremented in the ExistenceCacheCallback::callback path when a cache entry is actually removed. Metric counts evictions from ANY registered tier (fast-tier LRU churn AND slow-tier evictions); nonzero does NOT prove slow-tier chain is alive; zero over a churn window with active writes is the meaningful dead-chain signal. ### Item 4 (TraceMachina#11) — vulnerable_mode metric help-text correction Updated metric help to describe the actual condition (construction-time supports_removal_callbacks()==false) rather than the incorrect prior text ("register_item_callback failed at construction"). ### Item 5 (TraceMachina#11) — RefStore cell-write-before-replay no-brick fix ref_store.rs:get_store: writes the cell unconditionally before the callback replay loop. Replay errors are degradations (log + counter), not failures. New `callback_replay_failures: AtomicU64` metric field. ### Item 6 (TraceMachina#11) — Production-shape propagation test New test `ecs_over_ref_fast_slow_slow_tier_eviction_clears_cache` exercises the full AC-chain shape: ECS(RefStore→FastSlow{Memory, Memory}). Verifies slow-tier eviction propagates via FastSlow fanout to ECS cache removal within a 3-second deadline. ## AND Rule: Invariant + Correctness FSS.supports_removal_callbacks() returns fast.supports() && slow.supports(). Only slow-tier evictions create ECS stale positives (fast-tier eviction while slow still holds the blob = false negative, self-correcting). So slow.supports() alone is sufficient; AND is defense-in-depth. A future fast tier returning supports=false (e.g. WorkerProxyStore) would put ECS in vulnerable_mode=true unnecessarily; revisit the rule then. ## Behavior Changes FastSlowStore::supports_removal_callbacks: prior → new FastSlow{Memory, Memory}: true (default) → true (explicit; no change) FastSlow{Memory, GrpcStore}: true (default, wrong) → false (correct) GrpcStore.supports=false per ce4d2ec/TraceMachina#367 original commit (grpc_store.rs:3468) FastSlow{Memory, RefStore(Memory)}: true (default) → true (after resolve) FastSlow{Memory, NoCallbackStore}: true (default, wrong) → false (correct) Production compositions (verified against /srv/nativelink/buildcache-native.json5): cas_FAST_SLOW_STORE = FSS{MemoryStore, FilesystemStore} (line 111-140) fast = plain MemoryStore (not SizePartitioningStore; SPS lives at cas_INNER level as the ECS backend). fast.supports=true, slow.supports=true. Composite stays true. No behavior flip. No ECS wraps directly. AC_BACKEND_CACHED = FSS{MemoryStore, RefStore(RedisStore)} (line ~66-area) Redis slow (enable_keyspace_notifications default true): composite stays true. NO flip. vulnerable_mode stays false, matching TraceMachina#9 deploy verification (vulnerable_mode=0). SMALL_CAS_CACHED = FSS{MemoryStore, RefStore(REDIS_CAS_SMALL)} (line 206-220) keyspace default true → composite true. No flip. Mutations verified red: MUTATION-1 (TraceMachina#10 inner_store resolution): observability-only — no behavioral falsifier exists; the callback fires via RefStore queuing regardless; the fix corrects which warn arm fires. No mutation evidence claimed. MUTATION-2: remove supports_removal_callbacks from FastSlowStore → tests fast_slow_supports_callbacks_false_when_slow_does_not, ecs_over_fss_no_callback_slow_enters_vulnerable_mode, fss_supports_callbacks_false_when_fast_does_not_and_rule all fail with bespoke messages. MUTATION-3: remove invalidation_callbacks_fired.inc() → test ecs_invalidation_callbacks_fired_counter_increments fails with bespoke "TraceMachina#11: invalidation_callbacks_fired counter did not increment after eviction" MUTATION-5: re-add `return Err(err)` in replay loop → test ref_store_get_store_not_bricked_by_callback_replay_failure fails with bespoke "TraceMachina#11: ref bricked by callback replay failure" MUTATION (new test a): revert supports_removal_callbacks to true → ecs_over_fss_no_callback_slow_enters_vulnerable_mode red-fails with bespoke "TraceMachina#11 over-action: ECS(FSS{Mem, NoCallback}) must enter vulnerable_mode=true..." MUTATION (new test b): flip AND to || → fss_supports_callbacks_false_when_fast_does_not_and_rule red-fails with bespoke "TraceMachina#11 AND-rule: FSS{NoCallback-fast, Memory-slow}.supports() must be false..." Cargo churn: tokio-epoll-uring exclude (Cargo.toml) + assert-panic/os_pipe removals from Cargo.lock were NOT required for this branch to build (verified: cargo check -p nativelink-store clean without exclude); reverted. Bench scope: observability + callback-path (not data-plane hot path). Exempt from data_plane_bench per CLAUDE.md (no ByteStream/FastSlow/FilesystemStore hot-path changes). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…egistry + proto surface Post-review fix-up on 6471f6a0 per 5-reviewer cadre findings. ## Operator decision Proto field relocated from `cas_endpoint = 6` to `cas_endpoint = 50001` (vendor-extension range; upstream REAPI fields end at 5). Proprietary-extension comment block added to both .proto and generated .pb.rs (tag "50001" verified). The 4 struct-init sites are unchanged (field name unchanged). ## Convergent fixes (czar C1-BLOCK + code-rev C3) Test 6 (`blobs_available_does_not_insert_into_pending_registry`): replaced `timeout(200ms, yield_now())` sleep-as-sync with a positive tripwire. Setup now wires a real `ac_pin_registry` (was None). After sending `BlobsAvailableNotification`, poll (5s timeout, bespoke message) until `ac_pin_registry.snapshot_endpoint` is Some — proves the background task processed the tick. Only then assert pending registry is None. The `ac_pin_registry_for_testing()` accessor is added to `WorkerApiServer`. ## Convergent fixes (czar I3 + code-rev C2) Test 5 dead setup code removed: `inner_scheduler` / `schedulers2` construction (lines ~486-500) never passed to any server and leaked a background `remove_timedout_workers` task into subsequent tests. Removed. ## Remaining fixes - [code-rev S2] Counter help-text prefixed with "[Phase 1: always 0 — increment wired in phase 2]". - [code-rev S1] CAPPED cross-ref comment added at all 4 SharedAcPinRegistry field declaration sites (WorkerApiServer ~:92/:126 + WorkerConnection ~:1043/:1052): "CAPPED: AcPinRegistry enforces DEFAULT_MAX_AC_PINS_PER_ENDPOINT = 1_000_000 internally." - [code-rev S3] Test-3 doc-comment: validate_pending_output_endpoint → pending_output_endpoint_is_live. - [auditor TraceMachina#10] Chesterton dismissal added to pending_output_locality_registry field doc: "the deleted register_action_result_digests approach was removed for an mpsc::channel(1) eviction race; this registry is fed server-side at AC publish time (phase 2) and never shares that channel." - [red-team P2 + distsys NIT-2 — convergent] Design doc (.claude/audits/dangling-ac-design-2026-06-10.md §3) updated with explicit PHASE 2 DELIVERABLES: (a) wire pending_output_locality_registry into BIS-ack drain loop at nativelink.rs:~1109 — currently only ac_pin_registry cloned; without this, entries accumulate until disconnect/cap; (b) increment pending_output_registrations_total + two czar-I1 tests; (c) 256-byte length guard on cas_endpoint before insertion. PHASE 3 DELIVERABLE: seam- crossing guard test (registry + WorkerProxyStore + real has_with_results). Design doc also names exact proto file + field number (50001). ## Mutation evidence (run live) Mutation 1 (disconnect wipe commented out): → wipe_endpoint_clears_on_disconnect FAILS: "pending_output_locality_registry not wiped on disconnect — wipe_endpoint was not called from WorkerConnection disconnect-cleanup path: Elapsed(())" Log: /tmp/h4p1-mut1-1781138883-1602553.log Mutation 2 (boot-epoch wipe commented out): → wipe_endpoint_on_boot_epoch_change FAILS: "pending_output_locality_registry not wiped on boot-epoch change — wipe_endpoint not called from the boot-epoch-flip path in inner_connect_worker: Elapsed(())" Log: /tmp/h4p1-mut2-1781138936-1603960.log ## Gates - 6/6 tests green post-fix-up: /tmp/h4p1-final-1781138958-1604924.log - cargo check -p nativelink-service -p nativelink-proto clean - pb.rs diff: ONLY tag renumber (6→50001) + 5-line proprietary comment - base cbdf8e6 confirmed Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

… tree guards Addresses the 5cfd4fce review (pair-a MAJOR D1, pair-b S1/S2/C1/T1-T3). Invariant being violated: a stalled/slow main-CAS read during the out-of-band `-incr` seed materialize must degrade to a COLD build within a bounded time, never stall the pre-rustc execution path. Mechanism that violates it: incr_seed_fetch.rs — the old index-only `tokio::time::timeout` bounded ONLY the index GetActionResult; the Tree read (materialize_tree) and every file-blob read (fetch_and_write_file) hit `cas_store` UNBOUNDED, so a §6.5 server-CAS fall-through (GrpcStore timeout=0, internal-RPC policy invariant TraceMachina#10) hangs the whole materialize with no deadline. Mechanism that re-establishes it: fetch_and_materialize_seed now wraps the entire inner op (fetch_and_materialize_seed_inner: index + Tree + all blob reads + writes) in ONE overall `tokio::time::timeout(timeout, ...)`; on expiry it returns the cold SeedOutcome::TimedOut and best-effort wipes any temp dir recorded in `temp_dir_slot`, so the tmp-then-rename / no-partial-dir guarantee (`!dest.exists()`) holds on the timeout path too. No per-step deadlines are added (internal-RPC policy). Composite invariants this fix interacts with: tmp-then-rename / no-partial-dir (dest is only ever touched by the final atomic rename in swap_into_place; temp-dir cleanup added to the deadline path); cold-is-always-correct (TimedOut is a cold outcome, never Err/wrong); the Tree-structure guards (cycle / symlink / `..`-`/`-NUL component) which become a live execution path once the module is wired (chunk 2b). Test that proves the fix: stalled_cas_tree_read_times_out_bounded and stalled_cas_blob_read_times_out_and_wipes_temp (FIX 1, mutation-verified: disabling the deadline trips both 5s detectors; disabling the deadline-path cleanup leaks a temp dir); directory_cycle_is_rejected / diamond_shared_child_materialized_per_location / in_tree_symlink_is_rejected / validate_component_rejects_traversal_separator_nul (FIX 5, each mutation-verified against its guard). FIX 1 (D1): overall deadline bounds index+Tree+blob reads+writes → cold TimedOut, temp-dir wiped on expiry. build_materialize_plan split into a testable traverse_into_plan (enables the adversarial-cycle test; pure refactor). FIX 2 (S1): // UNBOUNDED-OK annotation on the get_part_unchunked blob read. FIX 3 (S2): // CAPPED AT ~10MB proto notes on MaterializePlan.{dirs,files} + by_digest. FIX 4 (C1): add incr_seed_collision to the module-doc marker list. FIX 5 (T1/T2/T3): cycle-reject + diamond-accept, symlink-reject, component-reject tests. Behavior changes (module is NOT yet wired into any production path — TODO chunk 2b — so all runtime changes below are latent until a caller exists): - fetch_and_materialize_seed: `timeout` now bounds the WHOLE op, not just the index fetch. Prior → new: index-only deadline → whole-op deadline. On expiry: cold SeedOutcome::TimedOut (unchanged variant). Verified safe: no production caller yet; the returned outcome and Err-reservation semantics are unchanged; TimedOut is cold. - incr_index_fetch_timeout counter now also fires for Tree/blob-read stalls (was index-only). Debug-marker only (not a registered metric yet); wiring chunk maps timeout→cold. Verified safe: no metrics consumer yet. - New: on deadline expiry after skeleton creation, the partial temp dir is wiped. Previously unreachable (index-only timeout could not have created a temp dir). Verified safe: remove_dir_all tolerates absence; dest never touched pre-rename. - traverse_into_plan extraction, FIX 2/3/4 annotations + doc: no runtime behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

…ted declared path (full-empty wipe) Two load-bearing pre-canary divergences from the Bazel/rules_rust chunk-4 exchange (both cold-not-wrong but would reuse 0% / DARK). Flag-gated INERT. §2 — targetkey exclusion. TargetKey::derive excluded no outputs, so it keyed on the lexicographically-smallest raw output_paths entry. A rustc pipelined action declares <label>-incr / -incr-metadata / -incr-unused-inputs.txt; since '-' (0x2D) < '.' (0x2E) the bytewise-min is <label>-incr — config-blind, diverging from the carrier + the KAT. derive now excludes any entry whose BASENAME contains "-incr" (catches all three; no legit .rlib/.rmeta/.d basename contains it) before sorting; all-excluded => None. verify_against_command_outputs re-derives via the same derive(), so the worker-side integrity check now agrees with the client on the .rlib key. §3 — nested seed path + full-empty wipe (option A). seed_dest_dir returned a TOP-LEVEL <stem>-incr, but rustc reads the NESTED declared output bazel-out/cfg/bin/<pkg>/<label>-incr => cold; and the chunk-2 wipe preserved only top-level *-incr names, so a nested seed lived under bazel-out and was deleted. Now seed_dest_dir joins the action's OWN declared nested -incr output_paths entry onto the execroot (option A: the index OutputDirectory.path stays the .rlib collision guard), the wipe is FULL-EMPTY (nothing preserved), and the out-of-band fetch is MOVED to run after [B2] input-materialize + [C] output-dir creation so the nested parent exists (and [B2] clonefiles into an empty execroot). Invariant being violated: a portable-incr action's fleet-shared targetkey AND its on-disk seed path must both agree with the Bazel client so rustc reuse fires. Mechanism that violates it: targetkey.rs::derive keyed on -incr; seed_dest_dir returned a top-level <stem>-incr; portable_incr wipe preserved only top-level *-incr (deleting the nested seed under bazel-out). Mechanism that re-establishes it: derive excludes -incr basenames -> key on the .rlib; seed_dest_dir joins the declared nested -incr output; full-empty wipe + fetch-after-[C] materializes the seed fresh at the nested path. Composite invariants this fix interacts with: §11 verify_against_command_outputs (re-derives via derive => same exclusion, key matches carrier); §6.2 plan_seed_publish / select_incr_seed_folder (now share the ends_with("-incr") predicate; behavior preserved); §9 delete containment (unchanged; assert_under_prefix still gates every delete, symlink-safe); the macOS clonefile fast path (an empty execroot restores it). Test that proves the fix: targetkey_kat_excludes_incr_and_keys_on_rlib (§2 KAT, both hashes b3sum-verified) + seed_dest_dir_uses_declared_nested_incr_output + wipe_full_empty_removes_all_including_incr + portable_action_with_seeded_index_fetches_and_materializes (production composition). Behavior changes (all INERT on the fleet: every changed path is gated on Some(PortableIncrContext)/portable_targetkey, which is None everywhere): - targetkey.rs::derive: when output_paths contains an -incr basename the derived key/primary_output moves from the -incr entry to the smallest non-incr entry; all-incr => None (was Some(-incr key)). Reached only via the portable-incr worker verify/stash path and any future targetkey caller — none on the live fleet. - portable_incr wipe: Owner execroot is now FULL-EMPTY wiped each build (was: preserve top-level *-incr). Reached only when portable_execroot is Some (off). - running_actions_manager inner_prepare_action: the seed fetch MOVED from before [B2] to after [C]; execroot wipe is full-empty. Reached only when portable_execroot is Some (off). The normal-mode create_dir path is byte-identical. - fs_util.rs try_clonefile: NO behavior change — comments/log strings only (dropped the now-moot FL-1383 seed-preservation reasoning + a removed test cite). - Non-portable actions (the entire fleet): byte-identical. No fsync/sync-write primitive added. No per-RPC timeout added (the seed fetch keeps its existing bounded overall deadline, invariant TraceMachina#10). No new unbounded buffer. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

allada force-pushed the fix-bytestream-error branch from 43b8536 to de031d6 Compare November 4, 2021 22:16

Fix buf in bytestream_server when NotFound was returned

0131a1c

Fixes bug found where the backend store could return NotFound but would deadlock the request because the spawn that received the error was never awaited properly.

allada force-pushed the fix-bytestream-error branch from de031d6 to 0131a1c Compare November 4, 2021 22:17

allada commented Nov 4, 2021

View reviewed changes

allada merged commit a4634eb into master Nov 4, 2021

allada deleted the fix-bytestream-error branch November 11, 2021 06:22

Navigation Menu

Repository navigation

Fix buf in bytestream_server when NotFound was returned - #10

Fix buf in bytestream_server when NotFound was returned#10
allada merged 1 commit into
masterfrom
fix-bytestream-error

allada commented Nov 4, 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 4, 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 4, 2021 •

edited

Loading