Fix case where s3 uploads in wrong order by allada · Pull Request #12 · TraceMachina/nativelink

allada · 2021-11-05T07:49:28Z

This change is

allada

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

…raceMachina#12/TraceMachina#13/TraceMachina#14: minor cleanups + boundary/mode tests TraceMachina#8 (MINOR) — `fits()` defensive: added `(Some(_), Some(0)) => true` arm so a future code path that records a stale `(used, 0)` cannot lock the peer out of all picker rotations. Sits before the main `<=` arm. TraceMachina#9 (MINOR) — Reverted visibility widening on `BlobsAvailableState::fs_store` and `cas_server_fss` to private. Only consumer is in the same module (`local_worker.rs`). TraceMachina#11 (MINOR) — Replaced hand-rolled temp paths in `blobs_in_stable_storage_handler_test.rs` and `download_to_directory_mirror_test.rs` with `tempfile::Builder`. `.keep()` disarms auto-cleanup so the FilesystemStore (lives past the test body inside Arcs) doesn't see its content_path vanish mid-run. TraceMachina#12 (MINOR) — Inlined `pick_mirror_endpoint` wrapper and renamed `pick_mirror_endpoint_for_size` → `pick_mirror_endpoint`. The wrapper was only used by tests (not prod) and the duplication added cognitive load. All 5 test sites updated to pass `0` for size_bytes (preserves pre-fix behavior of disabling capacity filter). TraceMachina#13 (MINOR) — Added `test_fits_at_exact_boundary_returns_true`. Uses two peers — "a" exactly at boundary, "b" with room — and asserts the picker round-robins both. Mutate-verified: changing `<=` to `<` in `fits()` makes the test FAIL (only "b" gets picked). TraceMachina#14 (MINOR, Linux-only) — Added `mirror_materialize_sets_0o555_mode_on_disk`. Builds a FastSlowStore whose fast tier is a real FilesystemStore, runs the mirror-materialize path, then asserts the on-disk file's mode is exactly 0o555 via `digest_content_path`. Guards against a regression where the mirror-materialize path bypasses or post-overwrites the canonical CAS read-execute mode. cargo check --all-targets clean. mirror_durability_test: 15/15 pass (was 14). worker_proxy_store unit tests: 31/31 pass (was 30). Pre-existing unrelated failures in `bytestream_server_test` (locality_stale_evicts_and_ingests, locality_timeout_falls_through_without_eviction, write_too_many_bytes_fails) and `existence_store_test` (copes_with_dropped_items) are NOT caused by this commit — they fail identically on parent 70b2f59. Co-Authored-By: Claude Opus 4.7 (1M context) <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>

…ver-side registration ## Invariant walk (all 5 slots) Invariant being violated: A worker may publish an AC entry referencing output blobs before the server can route requests to the worker's CAS endpoint, creating a window where Bazel receives an ActionResult but the referenced blobs are invisible to the locality map. Mechanism that violates it: `upload_ac_results` emitted `UpdateActionResultRequest` with `cas_endpoint = String::new()` (running_actions_manager.rs:4546 pre-phase2). The server's `inner_update_action_result` never registered output digests in `pending_output_locality_registry` (None, inert from phase 1). Mechanism that re-establishes it after the fix: - Worker sets `cas_endpoint` from `local_worker.rs:cas_advertised_endpoint` → stored in `UploadActionResults::cas_endpoint` → cloned into `UpdateActionResultRequest.cas_endpoint` on the GrpcStore path (running_actions_manager.rs ~:4546). - Server's `inner_update_action_result` extracts `x-nativelink-worker` header (set via `IS_WORKER_REQUEST.scope(true, ...)`) → calls `register_output_locality` BEFORE AC store commit → inserts each output digest → endpoint into `pending_output_locality_registry` (ac_server.rs ~:256). - BIS drain in `blobs_in_stable_storage_loop` removes entries once the corresponding CAS digests reach stable storage (nativelink.rs ~:1239). Composite invariants this fix interacts with: 1. H4 ordering: locality-visible(outputs) happens-before AC-publish. Phase 2 establishes the server-side registration; phase 3 (CCS consult) will close the consumer side. 2. BIS-drain / ac_pin_registry: pending_output_locality_registry is a SECOND AcPinRegistry (distinct from ac_pin_registry). BIS drain now operates on both. Over-drain risk: CAS batch store_id="" is the same PENDING_STORE_ID used in ac_server — drain correctly scoped. 3. Worker liveness gate: dead/stale endpoints are silently ignored (liveness_checker from WorkerApiServer.endpoint_state). Oversized endpoints (>256 bytes) warn+ignore (Revision 5 item c). Test that proves the fix re-establishes the invariant: - `live_endpoint_registers_output_digests_and_increments_counter` (i): UAR with live cas_endpoint → registry contains output file digest + counter==1. Mutation: skip registration-skip → red. - `empty_cas_endpoint_no_registration_counter_unchanged` (ii): over-action. - `oversized_endpoint_ignored_ac_update_succeeds` (iii): guard-skip. - `dead_endpoint_not_registered` (iv): liveness check. - `no_worker_header_no_registration` (v): over-action, Bazel plane. - `bis_drain_removes_pending_entry` (vi): BIS drain API. - `t7_cas_endpoint_stored_in_upload_action_results` (vii): worker wiring. ## Mutation table (all 7 — all red-verified in this session) | # | Mutation | File | Guard test | Bespoke red message | |---|----------|------|-----------|---------------------| | 1 | registration-skip: comment out `registry.register_ac_pin(...)` | ac_server.rs:262 | (i) `live_endpoint_registers_output_digests_and_increments_counter` | "registration-skip: digest not in pending registry after worker UAR with live endpoint" | | 2 | counter-skip: comment out `fetch_add(1, AcqRel)` | ac_server.rs:265 | (i) same | "counter-skip: pending_output_registrations_total must be 1 after one live registration" | | 3 | length-guard-skip: remove `if len() > MAX_CAS_ENDPOINT_LEN { return; }` | ac_server.rs:236 | (iii) `oversized_endpoint_ignored_ac_update_succeeds` | "guard-skip: oversized endpoint MUST be ignored; found Some(1) entries" | | 4 | BIS-drain-skip: comment out `remove_digests_for_endpoint_batch(...)` | ac_server_h4_registration_test.rs:443 | (vi) `bis_drain_removes_pending_entry` | "pending entry survives drain — stale registry: remove_digests_for_endpoint_batch did not drain the entry; remaining: Some([…])" | | 5 | is_worker-gate-skip: make `register_output_locality` unconditional | ac_server.rs:407 | (v) `no_worker_header_no_registration` | "over-action: non-worker UAR must not register in pending registry; found Some(1) entries" | | 6a | empty-guard-skip: remove `if cas_endpoint.is_empty() { return; }` | ac_server.rs:231 | (ii) `empty_cas_endpoint_no_registration_counter_unchanged` | liveness guard catches ""; if liveness ALSO skipped: "over-action: registry must not contain entries when cas_endpoint is empty" | | 6b | liveness-guard-skip: remove `if !checker(...) { return; }` | ac_server.rs:248 | (iv) `dead_endpoint_not_registered` | "liveness check skipped: dead endpoint registered in pending registry; found Some(1) entries" | Mutation log tees: /tmp/h4p2-mut5-isworker-gate.log, /tmp/h4p2-mut6a-empty-guard.log, /tmp/h4p2-mut6b-liveness-guard.log (mutations 1-4 from czar's original session) ## Behavior changes (all activations enumerated) 1. `pending_output_locality_registry` (nativelink.rs:~289): now `Some` (was `None` / inert in phase 1). Registry is live. Every `UpdateActionResult` from a worker plane RPC with a live, validly-sized `cas_endpoint` now inserts its output digests into this registry. 2. AC updates from workers now register (ac_server.rs `inner_update_action_result`): when `is_worker=true` AND `cas_endpoint` non-empty AND live AND ≤256 bytes, `register_output_locality` fires before the AC store commit. No change to Bazel-plane (non-worker) updates. 3. BIS drain for pending registry (nativelink.rs ~:1239): `pending_registry_for_loop` companion drain added alongside existing `registry_for_loop` drain. Fires on every BIS-ack loop tick when CAS digests drain (`store_id == ""`). Keeps registry memory bounded at steady state. 4. Counter live (`pending_output_registrations_total`, ac_server.rs:99): increments once per successful registration call (1 per worker UAR, regardless of output count). Was always-zero in phase 1. 5. `WorkerApiServer::liveness_checker()` (worker_api_server.rs:~588): new `pub` method returning `SharedLivenessChecker` that captures `endpoint_state` Arc. Used by `AcServer` to validate `cas_endpoint` at UpdateActionResult time. 6. Worker sets `cas_endpoint` on `UpdateActionResultRequest` (running_actions_manager.rs:~4546): changed from `String::new()` to `self.cas_endpoint.clone()`. Populated from `RunningActionsManagerArgs:: cas_endpoint` (new field, sourced from `local_worker.rs::cas_advertised_endpoint`). IS_WORKER_REQUEST scope wrap added on same path. 7. `AcServer::new` unchanged; `new_with_pending_registry` added. Existing callers pass `None, None` — no behaviour change for Bazel-only deployments. 8. `PENDING_STORE_ID` visibility: `const` → `pub const` so integration tests in `nativelink-service/tests/` can import it directly. Structural coupling: a rename breaks the test at compile time. ## Seams named (wire-shape contract) Producer: `running_actions_manager.rs::upload_ac_results` GrpcStore arm. IS_WORKER_REQUEST task-local propagated through `GrpcStore::update_action_result` as `x-nativelink-worker` header (grpc_store.rs:~1911 existing, confirmed). Server extractor: `ac_server.rs::ActionCache::update_action_result` metadata check. Registration: `ac_server.rs::inner_update_action_result → register_output_locality`. BIS drain consumer: `nativelink.rs::blobs_in_stable_storage_loop` pending_registry drain. ## Design-drift notes Ordering (code vs design): `upload_ac_results` is awaited inline by `cache_action_result` via `futures::future::join(historical_fut, ac_fut).await` — it is NOT a detached spawn. `spawn_upload_to_remote` is the detached part and runs BEFORE `cache_action_result` (local_worker.rs:3111 vs 3190). The invariant direction is unaffected: at the point of AC write (inside the `ac_fut` join arm), the worker's local FilesystemStore already holds the output blobs (written during action execution). The pending registry hints CCS phase 3 to fetch FROM the worker's CAS endpoint — upload completion to the server's slow store is irrelevant to this fetch path. Full before-AC ordering confirmed. Bench scope: no data-plane path (ByteStream / FilesystemStore / CAS hot path) touched. Bench exempt per CLAUDE.md "Exempt" clause. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Closes the H4 false-dangling window: CompletenessCheckingStore now treats pending-registry-resident digests on LIVE worker endpoints as present, preventing spurious AC entry deletion and Bazel re-execute storms caused by large blobs published before they are server-visible. ## Invariant walk Invariant being violated: CCS marks an AR incomplete and deletes the AC entry for a CAS digest that is on a live worker but not yet in the server-side CAS. Mechanism that violates it: `get_and_verify_single` and `inner_has_with_results` call `cas_store.has_with_results`, get None, and fall through to the incomplete branch (`completeness_checking_store.rs:695` delete + warn). Mechanism that re-establishes it after the fix: `consult_pending_registry` is called immediately after `cas_store.has_with_results` reports MISSING. If the digest is registered in `pending_output_locality_registry` under an endpoint that passes `liveness_checker`, the digest is treated as PRESENT, the incomplete branch is skipped, and `ccs_pending_registry_rescues_total` is incremented. Composite invariants this fix interacts with: • gate(delete) ⇒ consult-first: the consult structurally precedes the delete-on-detection branch (missing_digests is built AFTER the consult in `get_and_verify_single`; results[index] is cleared only when NOT rescued in `check_existence_fut`). Enforced by construction. • SHORT-CIRCUIT BOUNDARY: consult fires ONLY in CCS completeness gate, NEVER in general `has_with_results` that bytestream/batch-update upload-dedup uses. CCS wraps AC store; the consult is reachable only after decoding an ActionResult proto — a path bytestream/batch never takes. • Liveness guard: registry consult re-checks endpoint liveness so a disconnected worker's registry entries do not rescue blobs that may already be gone. Test that proves the fix re-establishes the invariant: `composite_invariant_no_delete_on_registry_rescue` (nativelink-store/tests/ completeness_checking_ccs_consult_test.rs) — two sequential get_parts both succeed; no warn fires; mutation (skip consult) red-fails with bespoke "H4 rescue bypassed — pending blob reported missing instead of rescued; first get_part must return Ok". ## Deliverables (a) Seam-crossing guard: `registry_does_not_affect_cas_has` verifies the pending registry is INVISIBLE to the CAS-plane `has_with_results` used by upload-dedup. CCS-plane and CAS-plane are kept strictly separated. (b/c per Revision 5) Counter + 256-byte guard shipped in phase 2. (d) Real-tonic-boundary seam test `is_worker_request_scope_propagates_through_ real_tonic_boundary` (`nativelink-service/tests/ac_server_h4_seam_test.rs`) composes all 4 seams end-to-end over a real in-process tonic server: S1=IS_WORKER_REQUEST.scope, S2=GrpcStore x-nativelink-worker inject, S3=AcServer extract, S4=register_output_locality. ## Production wiring `src/bin/nativelink.rs`: after `WorkerApiServer` + `liveness_checker` are built, walks every AC store chain via `try_inject_into_chain` to find CCS instances and calls `inject_pending_registry`. Uses `OnceLock` in CCS for lock-free reads on the hot completeness-check path. Counts injections: warn! if count==0 with worker API active (silent-disabled detection); info!(injected_count, ...) otherwise. ## Tests (19 green, 6 mutations verified) nativelink-store tests: • completeness_checking_ccs_consult_test (8 new tests) • completeness_checking_store_test (4 existing, unchanged) nativelink-service tests: • ac_server_h4_seam_test (1 new, real tonic boundary) • ac_server_h4_registration_test (6 phase 1+2, unchanged) Mutations verified red with bespoke messages (czar verification 2026-06-10): 1. Skip consult in inner_has_with_results → "H4 rescue bypassed: registry-resident digest reported missing — consult skipped in inner_has_with_results" 2. Skip consult in get_and_verify_single → "H4 rescue bypassed: registry-resident digest reported missing — consult skipped in get_and_verify_single" 3. Skip liveness re-check (trust registry without live check) → "liveness-skip: dead-endpoint rescue must NOT fire — consult must re-check endpoint liveness" 4. Skip counter increment (counter-skip, both inc() + fetch_add) → "counter-skip: ccs_pending_registry_rescues_total must be 1 after rescue; got 0" (czar-performed; both rescue tests catch it) 5. Skip consult in get_and_verify_single only; run composite test → "H4 rescue bypassed — pending blob reported missing instead of rescued; first get_part must return Ok" (czar-performed; fires on FIRST assert — second "deleted as dangling" assert is unreachable post-mutation) 6. Rename x-nativelink-worker header at injection site → "seam-crossing: IS_WORKER_REQUEST scope did not reach AcServer registration — header name mismatch" (czar-performed; seam test catches it) Bench delta: exempt (observability-only code path on completeness miss; completeness_checking_store is not in the data_plane_bench hot path). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

…s_checker was always None for AC entries Invariant being violated: Every AC store containing a CompletenessCheckingStore must have the pending-output-locality registry + liveness_checker injected at startup so the H4 rescue path (consult-before-delete) can fire. Mechanism that violates it: src/bin/nativelink.rs per-entry loop (~old :1591) built WorkerApiServer and extracted liveness_checker INSIDE the services loop, scoped to the current entry. Production config has AC on :50051/:50071/:50072 and worker_api ONLY on :50061. Each AC entry saw liveness_checker = None (no worker_api on that entry) → `if let (Some(ac_cfgs), Some(checker))` silently skipped → CCS never received inject_pending_registry → H4 rescues INERT. The worker_api entry had Some(checker) but services.ac = None → also skipped. Startup log confirmed no "H4 phase 3" line at the 19:58 deploy. Mechanism that re-establishes it after the fix: A pre-loop pass over ALL server_cfgs finds the worker_api config (whichever entry hosts it), builds WorkerApiServer ONCE before the loop, extracts a global liveness_checker, then calls inject_h4_pending_registry_into_ac_chains(ac_store_names, ...) — using the cross-entry ac_store_names set — before any listener binds. The per-entry loop takes the pre-built WorkerApiServer via .take() for the entry that hosts worker_api; serving order is unchanged. Composite invariants this fix interacts with: - H4 rescue gate: CCS-inject ⇒ (registry consult fires for pending blobs). Triangle: gate = inject_pending_registry (this fix), eviction = BIS-drain (pending_registry loop), pin = WorkerProxyStore peer-fetch liveness check. - CCS OnceLock: inject_pending_registry is idempotent (second call silently ignored). Pre-loop single construction = at most one inject per CCS. - ac_store_names pre-scan (existing, unchanged): already collects AC store names from ALL server entries before the loop for the BIS drain loop. Test that proves the fix re-establishes the invariant: completeness_checking_ccs_consult_test::split_topology_ac_ccs_receives_registry — builds a StoreManager with a CCS in the AC chain, calls inject_h4_pending_registry_into_ac_chains with ac_store_names containing that store, and asserts (a) injected_count == 1 and (b) a rescue fires for a registry-resident digest. Mutation (comment out inject call) → red-fails with bespoke "H4 wiring inert under split ac/worker_api topology — CCS registry not injected; split-topology regression". Mutation confirmed red. Root cause section — why three cadres missed this: The per-entry pre-build introduced in phase 2 (TraceMachina#12 H4 phase 2) fixed the INTRA-entry ordering problem (build WorkerApiServer before AcServer so liveness_checker is available for the same entry's AcServer constructor). That was correct for a single-entry config. The phase 3 reviewers tested the injection logic in isolation (CCS + registry + live checker) and verified the consult fired, which passed. None of the test configurations exercised a config with AC and worker_api on SEPARATE server entries — the only topology where the bug manifests. The production config is ALWAYS split (worker API on a private port, AC/CAS on a public port), but tests used single-entry configs. The zero-injection warn at old :1693 could not fire because it was INSIDE the skipped `if let (Some(ac_cfgs), Some(checker))` block — the bug masked its own diagnostic. Files changed: nativelink-store/src/completeness_checking_store.rs — extract inject_h4_pending_registry_into_ac_chains as a testable pub fn nativelink-store/tests/completeness_checking_ccs_consult_test.rs — add split_topology_ac_ccs_receives_registry regression test (TDD, mutation-verified) src/bin/nativelink.rs — hoist WorkerApiServer construction + CCS injection to pre-loop block; per-entry loop takes from holder via .take() Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Invariant violated: inner_upload_results blocked the worker slot for p50≈152ms/p99≈720ms (n=140 actions) waiting for outputs to propagate to the remote CAS via FastSlowStore::update_with_whole_file's join!(). Mechanism that violates it: running_actions_manager.rs inner_upload_results (formerly line 3434) always used &*cas_store (full FSS), so every FastSlowStore::update_with_whole_file call executed join!(slow_fut, fast_fut) blocking the worker until the GrpcStore slow-tier ack. Mechanism that re-establishes it after the fix: when deferred_output_uploads_enabled is true, cas_store is set to cas_store.fast_store().clone() (FilesystemStore directly). inner_upload_results writes to local disk only; the remote slow-store upload is deferred to spawn_upload_to_remote after execution_complete frees the worker slot. The dda09ee has()-skip hole is closed by the existing BlobsAvailable locality map + spawn_upload_to_remote batch-has + UploadMissingBlobs backfill + pin_digest anti-eviction machinery (all verified live; see .claude/audits/f2-deferred-output-uploads-design-2026-06-12.md §2.3). Composite invariants this fix interacts with: (I1) ≥2-replica invariant: deferred path satisfies via spawn_upload_to_remote retries + UploadMissingBlobs drain fallback. (I2) BlobsAvailable ordering (TraceMachina#129): unchanged — BlobsAvailable sent after execution_complete, which follows upload_results. (I3) H4 pending-registry (TraceMachina#12): CCS pre-registration unchanged. (I4) Pin lifecycle: outputs pinned immediately in inner_upload_results; unpinned on BIS-ack. Unchanged. (I5) F3 dedup: spawn_upload_to_remote batch-has queries full FSS; under synchronous mode finds blobs in slow store and skips; under deferred mode finds them absent and uploads. No code change needed. (I6) No fsync: FilesystemStore writes are not fsync'd; durability from ZFS pool tank sync=disabled + ≥2-replica invariant. Tests that prove the fix: deferred_upload_enabled_completes_before_slow_store: deferred=true + blocking slow store → upload_results completes within FAST_DEADLINE=10s (slow store blocks forever). Output blob in fast store. Mutation (deferred→false): timeout fires with bespoke "deferred contract violated" message. deferred_upload_disabled_blocks_on_slow_store: deferred=false + blocking slow store → upload_results parks until release. Spawn task + await update_attempts_count > baseline + assert !is_finished() + release + assert completes. Mutation (deferred→true): timeout fires with bespoke "slow store was never entered" message. Behavior changes: nativelink-worker inner_upload_results: when deferred_output_uploads_enabled is false (DEFAULT), behavior is IDENTICAL to prior — full FastSlowStore write, synchronous. No runtime behavior change unless operator explicitly sets the field to true in LocalWorkerConfig. deferred_output_uploads_enabled: false (default-OFF kill-switch). Bench delta: not run (data_plane_bench has zero GrpcStore client cells; the output-upload path is bench-invisible per .claude/rules/data-plane-bench.md). Data from memory: production traces p50=152ms p99=720ms n=140 actions. Do NOT force-push. Do NOT flip default to true. Do NOT deploy. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

…ch flip Invariant violated: four MUST-FIX / SHOULD-FIX findings from the Tier-3 review cadre (distributed-systems-reviewer + pair-b) must be resolved before the deferred_output_uploads_enabled kill-switch can be safely flipped in production. Mechanism that violates it: (1) missing startup guard permits deferred_output_uploads_enabled=true + cas_server_port=None — a config that makes BlobsAvailable never fire, leaving outputs unroutable during the upload window (reopens dda09ee hole); (2) doc-comment understated the ≥2→≥1 replica regression and omitted hard prerequisites; (3) kill_switches_test.rs was missing the three schema tests promised by the design doc; (4) setup proto writes in the DISABLED test went through the full FSS, injecting background slow-write tasks that complicated the before_count baseline. Mechanism that re-establishes it after the fix: - local_worker.rs: new startup guard returns make_input_err! if deferred_output_uploads_enabled=true AND cas_server_port.is_none(), naming both fields and explaining the BlobsAvailable dependency (#F2). - cas_server.rs: doc-comment now explicitly states ≥2→≥1 replica regression, quantifies the single-copy window (p50≈152ms, p99≈720ms, source cited), names cas_server_port as a HARD PREREQUISITE, and names --remote_retries≥1 as a CLIENT REQUIREMENT for crash-window recovery. - kill_switches_test.rs: three new tests covering default-false / explicit-true / explicit-false for LocalWorkerConfig.deferred_output_uploads_enabled. - running_actions_manager_test.rs: DISABLED test now writes setup protos to fast_store.as_pin() directly, eliminating background FSS slow-write tasks from setup; import order fixed (config before metric per CLAUDE.md). - P4 code comment corrected: registry is populated asynchronously via the detached AC write task (after execution_complete), NOT before execution_response. - Design doc §3 P4, §4 Invariant 1, §2.3 Layer 4, §6, and §9 corrected to match code reality (distsys F2/F5, auditor 7b, latency provenance). Composite invariants this fix interacts with: - ≥2-replica durability: now explicitly documented as regressed to ≥1-replica during the deferred window (design §4 Invariant 1). - BlobsAvailable ordering (TraceMachina#129): startup guard enforces the cas_server_port prerequisite that makes BlobsAvailable fire. - H4 pending-registry (TraceMachina#12): P4 comment and design doc corrected to reflect asynchronous registration timing. - Pin-until-uploaded: design §2.3 Layer 4 now distinguishes eviction pin (held) from budget-guard (dropped immediately, observation-only). Test that proves the fix re-establishes the invariant: - deferred_uploads_without_cas_server_port_is_rejected: startup guard fires with message naming both fields; mutation (guard commented out) fails with bespoke message "guard missing or condition check is wrong (#F2)". - deferred_uploads_with_cas_server_port_passes_guard: guard does not over-reject valid config (enabled=true + port=Some(0)). - local_worker_deferred_uploads_default_false/explicit_true/explicit_false: schema tests guard against accidental field rename/skip. - deferred_upload_disabled_blocks_on_slow_store: now uses fast_store.as_pin() for setup protos; before_count baseline is clean. Behavior changes: - new_local_worker: REJECTS startup if deferred_output_uploads_enabled=true AND cas_server_port=None. All existing production configs have either (a) both OFF (default, no change) or (b) cas_server_port set (valid). No behavior change for any currently deployed worker. - All other changes: documentation and test only; zero runtime behavior change when deferred_output_uploads_enabled=false (the production default). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

…e (#[must_use] from v3 M1) The v3 M1 fix-up added #[must_use] to AcPinRegistry::remove_digests_for_endpoint_batch and gated the AC-pin stable-drain site (nativelink.rs:1287) on the return. The SECOND call site (:1330, TraceMachina#12 H4 phase-2 pending-output-locality drain) operates on a DISTINCT registry (pending_output_locality_registry, :319) that is server-internal — populated via inject_h4_pending_registry_into_ac_chains / AcServer, never worker-advertised — so a removal there cannot diverge any worker's last_sent_ac_pin_set and needs no AcPinResync push. Return intentionally ignored via `let _ =` with rationale. Surfaced by the consolidated verify-build; verified at the two registry construction sites before silencing. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Fix case where s3 uploads in wrong order

a9728c1

allada force-pushed the fix-s3-store branch from 4386f59 to a9728c1 Compare November 5, 2021 07:52

allada commented Nov 5, 2021

View reviewed changes

allada merged commit 4798fe9 into master Nov 5, 2021

allada deleted the fix-s3-store branch November 11, 2021 06:21

Navigation Menu

Repository navigation

Fix case where s3 uploads in wrong order - #12

Fix case where s3 uploads in wrong order#12
allada merged 1 commit into
masterfrom
fix-s3-store

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

edited

Loading