Repository navigation
Fix minor issue in FastSlowStore - #21
Merged
Merged
Conversation
A case where the client can close the connection causing the future to be destoryed resulting in the EOF to fail to write to underlying store. We no longer care if the EOF suceeds.
rejuvenile
added a commit
to rejuvenile/nativelink
that referenced
this pull request
Aug 19, 2026
… memory-only CAS blobs Operator directive (2026-06-23): "remove the timeout from the shutdown path; give shutdown unlimited time to flush blobs to redis and to disk. we can't have this keep happening every time the server restarts." At the 2026-06-23 17:38 buildcache restart the Phase-2 shutdown flush hit its 30 s deadline and lost 827,204 memory-only SMALL_CAS_CACHED blobs (`flushed=0 skipped_already=591792 deadline_exceeded=827204`, `elapsed_ms=29997`), causing CI "lost input too many times (TraceMachina#21)" failures across >=3 branches for ~30 min post-restart. ROOT CAUSE of flushed=0 (verified, not a counter bug): the per-blob loop in `flush_fast_to_slow_at_shutdown` was STRICTLY SERIAL — one `slow.has` RTT then one `slow.update_oneshot` RTT per blob. With 1.4M keys the 30 s budget was spent on ~591k serial Redis `has` probes (all already-present -> skipped) and the deadline (`started.elapsed() >= deadline`) fired before the loop reached a single blob that NEEDED writing. `flushed` increments correctly on the success path; the loop simply never reached a writable blob in time. So removing the deadline alone was necessary-but-insufficient — the serial structure had to be parallelized too, or an unbounded drain of 1.4M serial RTTs would near-wedge. FIX (three parts): 1. Deadline `Duration` -> `Option<Duration>`; `None` = unbounded (production path), `Some(d)` = best-effort within `d` (kept for the deadline-bound test + any future bounded caller). `store_manager` Phase 2 and the bin now pass `None`; the bin drops its outer `tokio::time::timeout` around `flush_slow_writes` (it would have re-capped the unbounded drain). Phase 1 (in-flight slow-write drain) stays bounded by `timeout` — it was NOT the incident surface (`total_pending=0` in the incident). 2. Bounded-concurrency drain (`SHUTDOWN_FLUSH_CONCURRENCY = 64`, via `buffer_unordered`) so N serial RTTs become N/K; a large fast tier drains in minutes, not tens of minutes. Skip-existing `has` pre-check is preserved (Test 5 still passes) but now runs concurrently. 3. `info!` forward-progress line every 50,000 processed blobs (`flushed/skipped/remaining/elapsed`) so an operator watching journalctl through a long drain sees motion, not a wedge. Both arms are unbounded: SMALL_CAS_CACHED (Redis slow tier — the 827k victim) AND cas_FAST_SLOW_STORE (FilesystemStore /srv/bulk slow tier). The disk arm is a plain async `update_oneshot` (FilesystemStore dedups via its in-memory index; no sync-write primitive added anywhere). INVARIANT WALK - Invariant being violated: every memory-only CAS blob in a FastSlowStore fast tier must reach its slow tier before process exit, or AC entries dangle and Bazel reports "lost input". - Mechanism that violates it: `fast_slow_store.rs` serial per-blob flush loop + 30 s deadline — burned the whole budget on serial `has` RTTs, flushed 0. - Mechanism that re-establishes it: `flush_fast_to_slow_at_shutdown(None)` runs the per-blob has/get/update at concurrency 64 with NO deadline, draining to zero (`unflushed = errored + deadline_exceeded`, both 0 under `None`+no-error). - Composite invariants this fix interacts with: mirror_blobs >=2-replica (NOT touched — mirror blobs are deliberately not flushed here); ZFS sync=disabled durability (unchanged — async writes only); admission/eviction/pin (untouched — shutting_down already fences new writes via Phase 1). - Test that proves the fix: `shutdown_flush_unbounded_drains_all_blobs` (all N land, unflushed==0) + `shutdown_flush_unbounded_drains_despite_slow_tier` (slow tier + None still drains all N; exactly N writes issued). BEHAVIOR CHANGES (every runtime change vs production composition) - FastSlowStore::flush_fast_to_slow_at_shutdown: bounded(30s)+serial -> unbounded(None)+concurrent. In prod (StoreManager Phase 2 over SMALL_CAS_CACHED + cas_FAST_SLOW_STORE): shutdown now blocks until every memory-only blob is durable instead of dropping the remainder at 30 s. - StoreManager::flush_slow_writes Phase 2: removed per-store deadline math (PHASE_2_FLOOR) and the Phase-2 outer wall-clock timeout; Phase 2 awaits all per-store drains to completion. Phase 1 unchanged. - src/bin/nativelink.rs shutdown step 4: removed the `tokio::time::timeout(flush_budget+8s, ...)` wrapper around `flush_slow_writes`; calls it directly. Phase-1 budget still passed in. - ACCEPTED RISK (per directive): a genuinely wedged slow tier now blocks SIGTERM-to-exit indefinitely. No in-process safety valve added (TRUE unbounded per directive). REQUIRES systemd `TimeoutStopSec=infinity` (currently 90 in /etc/systemd/system/nativelink.service — parent applies at deploy) or systemd SIGKILLs the unit mid-flush and re-introduces the loss. TDD: 8/8 tests green. Mutations verified: - force `Some(Duration::ZERO)` on the None path -> unbounded test red-fails "TraceMachina#210 UNBOUNDED flush must drain ALL memory-only blobs — data-loss regression: 250 of 250 blobs left unflushed with no deadline". - skip the actual `update_oneshot` write -> content check red-fails "blob ... missing from slow tier post-unbounded-flush". No fsync/sync-write primitives; no tokio-worker blocking; mirror invariant untouched. 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
…rver durability bundle, directive-2) Adds graceful-shutdown Phase 3 (pull worker-only CAS blobs to the server slow tier while workers are still connected) plus a Phase-0 Bazel-REAPI quiesce, on top of directive-1's unbounded flush. New shutdown ordering: quiesce Bazel REAPI -> flush server fast tier -> pull worker-only blobs -> [directive-3 persist-locality slot] -> evict workers -> stop accepting -> drain -> shut down local workers -> exit. Eviction MOVED after the pull (you cannot pull from an evicted/disconnected worker) but stays BEFORE listener-drain (preserves d204e1b's drain-promptness rationale). Design: .claude/audits/shutdown-pull-worker-blobs-design-2026-06-23.md. == Invariant-walk == Invariant being violated: after a server restart, a CAS blob that exists ONLY on workers is not served by the server until the worker re-handshakes and re-backfills, causing a multi-minute NotFound window (Bazel "lost input TraceMachina#21"); and during shutdown the flush cannot converge because new Bazel writes keep arriving. Mechanism that violates it: src/bin/nativelink.rs evicted/disconnected all workers FIRST (old Step 1), wiping the locality map + per-worker tx before any localize attempt (worker_api_server.rs remove_endpoint); and no gate stopped new Bazel REAPI during shutdown so the flush chased fresh writes. Mechanism that re-establishes it: (Phase 0) BazelReapiQuiesce latch flipped at SIGTERM start makes new requests on the public Bazel listener return UNAVAILABLE (in-flight drain) so the in-flight write set converges; (Phase 3) ShutdownPuller::run (worker_api_server.rs) drives the existing has_with_results-gated request_missing_blob_uploads in a completion-poll loop over connected_workers_with_senders, BEFORE eviction, until every locality digest is durable in the server slow tier or has zero connected source (at-risk-skip; never wedges; no-progress watchdog bounds a stalled worker). Composite invariants this fix interacts with: (1) mirror_blobs >=2-replica + BIS-ack-as-durability-oath -- the pull ADDS the server as a replica and reuses the has_with_results-gated mark_stable (worker_api_server.rs ~2525), NOT weakened; (2) d204e1b evict-before-listener-drain -- preserved (eviction still precedes drain); (3) TraceMachina#210 unbounded memory->slow flush -- the pull runs AFTER it and depends on it for the existence check; (4) no per-RPC timeout on internal RPCs -- the pull is unbounded-time, bounded by skip-policy + no-progress watchdog, not RPC deadlines; (5) worker-facing CAS listeners (:50071/:50072) + worker_api (:50061) stay OPEN through shutdown -- the pull needs them. Test that proves the fix: shutdown_pull_lands_worker_only_blob_on_local_disk_before_future_resolves (pull future does not resolve until has_with_results == Some); shutdown_pull_skips_zero_source_blob_and_does_not_hang (zero-source -> resolves, at_risk_skipped==1, no wedge); closed_gate_rejects_new_request_with_unavailable (quiesce -> grpc 14). == What changes (architectural; .claude/rules/architectural-changes.md) == - X (today): SIGTERM evicts/disconnects workers FIRST, then drains, then flushes; worker-only blobs abandoned on disconnect; no Bazel-REAPI gate. - Y: SIGTERM quiesces Bazel REAPI, flushes, PULLS worker-only blobs (workers connected, unbounded), THEN evicts, drains, exits. - Shutdown-ordering change (worker disconnect moves after the new pull) + flow-control change (new unbounded-time, network-driven completion loop on the shutdown path). NOT an async<->sync flip: no .await-on-send added to a steady-state pump, no tokio::spawn removed from a producer/consumer; the loop is shutdown-only and uses the EXISTING fire-and-forget send + a new poll, so it adds no synchronous backpressure to any production write path. == Behavior changes (runtime, vs production composition) == - Steady state (no SIGTERM): NONE. The quiesce latch starts false (one Relaxed atomic load per request on the quiesce-marked listener only, never short-circuits). ShutdownPuller is only invoked from the SIGTERM handler. No production read/write path changes. - Config: new ServerConfig.quiesce_on_shutdown (default false). Absent => pre-change behavior. Set ONLY on the public Bazel listener (:50051). The worker-facing CAS listeners (:50071/:50072) and worker_api (:50061) do NOT set it and are unaffected. - Shutdown: ordering changes as above. The flush moves before eviction; the pull is new; eviction moves after the pull. Local-worker shutdown + exit unchanged. == Hard-constraint compliance (CLAUDE.md) == - NO fsync/sync-write: the pull writes nothing synchronously; blobs land via the worker's slow_store.update -> server gRPC CAS -> FilesystemStore/Redis async path; durability = mirror >=2-replica + ZFS sync=disabled. - Never block a tokio worker: pull is pure async (has_with_results, tokio::time::sleep poll, mpsc send); no std::thread::sleep / sync FS. - Unbounded-buffer rule: NO new blob buffer. Only a Vec<DigestInfo> + endpoints locality snapshot, annotated CAPPED AT digest_count() (~40 B/ entry, one-shot, dropped post-loop). Blob bytes stream worker->disk, never collected server-side. - BIS not weakened: the pull reuses the has_with_results-gated mark_stable; it acks-to-unpin only AFTER the server's existence check confirms durability. Tee logs: /tmp/dir2-*-{libbuild,testcompile,testrun,binbuild, mut-zerosource2,mut-skippull,mut-nocompletion,mut-quiesce}.log. Co-Authored-By: Claude Opus 4.8 <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.
A case where the client can close the connection causing the future
to be destoryed resulting in the EOF to fail to write to underlying
store. We no longer care if the EOF suceeds.
This change is