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