Repository navigation
Remove dependency on rust-nightly - #97
Merged
Merged
Conversation
Also upgrades cargo dependencies.
allada
force-pushed
the
remove-rust-nightly
branch
from
July 21, 2022 15:53
6fbe8db to
f35d676
Compare
allada
commented
Jul 21, 2022
allada
left a comment
Contributor
Author
There was a problem hiding this comment.
Reviewed 124 of 124 files at r1, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @allada)
allada
commented
Jul 21, 2022
allada
left a comment
Contributor
Author
There was a problem hiding this comment.
Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @allada)
rejuvenile
added a commit
to rejuvenile/nativelink
that referenced
this pull request
Aug 19, 2026
TraceMachina#98) Shared chunking iterator used by tasks TraceMachina#97 (BlobsInStableStorageChunk), TraceMachina#98 (PeerHintsChunk), and TraceMachina#99 (BlobsAvailableChunk). Each producer emits a sequence of chunks with monotonic `sequence` and a terminal `is_last` chunk; the empty-input case still emits one is_last chunk so receivers have a stable terminal marker. Six unit tests cover empty, partial, exact-boundary, multi-chunk, 1M-item stress, and the zero-max-per-chunk defensive case. Why a helper: three independent producers want the same envelope shape and the correctness-critical pieces (is_last only on the FINAL chunk; empty input still emits one) should live in one place rather than be re-implemented per call site.
rejuvenile
added a commit
to rejuvenile/nativelink
that referenced
this pull request
Aug 19, 2026
…ute.peer_hints (TraceMachina#98) Adds: - `PeerHintsChunk { repeated PeerHint peer_hints; string operation_id; uint32 sequence; bool is_last }` — one chunk of a streaming scheduler->worker peer-hints emission. - `ChunkedMessage { oneof payload { PeerHintsChunk peer_hints = 1 } }` — shared envelope for the cas->worker, scheduler->worker, and worker->scheduler streaming producers landing across TraceMachina#97/TraceMachina#98/TraceMachina#99. - `UpdateForWorker.chunked_message = 10` — the worker-side dispatch arm. Removes: - `StartExecute.peer_hints = 8` — replaced by streaming `Update::ChunkedMessage(PeerHintsChunk)` per the direct-merge design (no backwards compat per user directive 2026-04-24). Tag 8 is reserved. Direct-merge design: there is no per-action buffer, no chunk-count field on StartExecute, no race between chunk arrival and action dispatch. Chunks may arrive before, after, or interleaved with StartAction for any operation_id; each chunk's hints become available as soon as the worker processes it. Generated `.pb.rs` is hand-edited to mirror the proto change (matches recent practice in the codebase — see `754ccdf5` and `69b41ff2`).
rejuvenile
added a commit
to rejuvenile/nativelink
that referenced
this pull request
Aug 19, 2026
…S chunking + ack)
Adds:
- `BlobsInStableStorageChunk { repeated Digest digests; uint64
broadcast_id; uint32 sequence; bool is_last }` — one chunk of a
streaming server->worker BIS broadcast. The `(broadcast_id, sequence)`
pair lets the server track per-worker per-chunk acks.
- `ChunkedMessage::Payload::BlobsInStableStorage = 2` — second arm of
the shared envelope from TraceMachina#98 (peer hints chunking). Direct-merge
pattern: each chunk's digests unpin immediately, no per-broadcast
buffer.
- `UpdateForScheduler::BisAck { broadcast_id, sequence }` (tag 9) —
worker->scheduler ack channel. Independent from
`BlobsAvailableNotification` because BIS has per-chunk granularity
while BlobsAvailable has per-snapshot granularity; piggybacking would
either chatter the worker (one BlobsAvailable per chunk) or delay the
resend buffer release by up to one snapshot interval.
The legacy single-message `UpdateForWorker::BlobsInStableStorage`
(tag 8) stays in the proto for now; the broadcast loop will switch to
the chunked path in a follow-up commit, and the legacy arm is removed
once production confirms the chunked path holds.
Generated `.pb.rs` is hand-edited to mirror (matches recent practice
in TraceMachina#98 / TraceMachina#164 / TraceMachina#167).
rejuvenile
added a commit
to rejuvenile/nativelink
that referenced
this pull request
Aug 19, 2026
…1 worker arm)
Adds the worker-side handler for `Update::ChunkedMessage(
BlobsInStableStorageChunk)` plus the per-chunk ack channel
(`UpdateForScheduler::BisAck`) the server uses to release its
per-worker resend buffer.
Worker side (`nativelink-worker`):
- `local_worker::handle_bis_chunk(state, cas_store, chunk, ack_sink)` —
reuses the existing `handle_blobs_in_stable_storage` for the unpin /
mirror-cleanup work, then fires the ack via the caller-provided
closure. Ack is UNCONDITIONAL — even on the empty-terminal chunk
(chunk_iter contract) and on duplicate deliveries — so the server's
resend buffer always releases. Direct-merge: no per-broadcast buffer,
each chunk's digests unpin immediately.
- `Update::ChunkedMessage(BlobsInStableStorage)` arm in
`LocalWorkerImpl::run` calls `handle_bis_chunk` then spawns the
`bis_ack` send (so a slow ack doesn't block the dispatch loop).
- `WorkerApiClientTrait::bis_ack(BisAck)` + impl on
`WorkerApiClientWrapper`. MockWorkerApiClient gets a no-op impl
(the chunk-handler test injects its own ack sink directly).
Scheduler side (`nativelink-scheduler`):
- `WorkerScheduler::bis_ack_received` trait method (default no-op).
- `ApiWorkerSchedulerImpl::{bis_resend_buffers, next_bis_broadcast_id}`
state for per-cas-endpoint resend tracking. Endpoint is the stable
identity across reconnects (worker_id is regenerated each connect).
- `BisResendBuffer::{add, ack, drain}` primitives. Resend wiring +
broadcast loop switch land in the next commit.
Service side (`nativelink-service`):
- `worker_api_server.rs` routes `Update::BisAck` into the scheduler.
Tests (`nativelink-worker/tests/bis_chunk_handler_test.rs`):
- `bis_chunked_full_burst`: 32 digests, ack matches (broadcast_id,
sequence), mirror_blob_count drops to 0.
- `bis_chunked_empty_terminal_acks`: zero digests still emit one ack.
- `bis_chunked_idempotent_unpin`: duplicate delivery is safe and
emits an ack on every receipt.
TDD verification:
- Mutation 1 (suppress ack_sink call) — all 3 tests fail with the
specific "must emit ack" assertion. Reverted.
- Mutation 2 (suppress remove_mirror_blobs) — full_burst and
idempotent_unpin fail with specific "mirror blobs must be removed"
assertion (empty test correctly passes — no digests to remove).
Reverted.
Test harness uses real `FilesystemStore` + `FastSlowStore` with mirror
fixture (production-composition path; mirrors the existing
blobs_in_stable_storage_handler_test setup).
Coordination: builds on TraceMachina#98's `ChunkedMessage` envelope + `chunk_iter`
helper (cherry-picked into this branch). The next commit wires the
server's broadcast loop through `chunk_iter` + the per-worker resend
buffer.
rejuvenile
added a commit
to rejuvenile/nativelink
that referenced
this pull request
Aug 19, 2026
…hina#97 PR1 server arm) Implements the server-side state for TraceMachina#97 (BIS chunking + ack): - `broadcast_blobs_in_stable_storage_chunked(digests)`: splits the digest list into `BIS_DIGESTS_PER_CHUNK = 4096` chunks via the shared `chunk_iter` helper from TraceMachina#98, dispatches each chunk as `Update::ChunkedMessage(BlobsInStableStorageChunk)` with a unique per-broadcast `broadcast_id`, and adds the chunks to the per-worker resend buffer keyed by `cas_endpoint`. Direct-merge per TraceMachina#98: no per-broadcast worker-side buffer; chunks unpin immediately. - `replay_bis_chunks_to_worker(cas_endpoint, tx)`: called from `add_worker` whenever a worker (re)connects. Replays every chunk still in the per-endpoint resend buffer to the new tx. Same-epoch reconnects automatically pick up unacked chunks; epoch changes are handled below. - `clear_bis_resend_buffer_for_endpoint(cas_endpoint)`: drops every buffered chunk for an endpoint. Called by `worker_api_server` on `boot_epoch_id` change (a fresh worker process means the pin state died with the old process — replaying unpins for blobs that no longer exist would just waste memory). - `bis_ack_received(worker_id, broadcast_id, sequence)`: drops the matching chunk from the per-endpoint resend buffer. Idempotent. When the buffer hits zero, the per-endpoint entry is removed entirely (so a long-lived worker doesn't accumulate empty buffers). State lives on `ApiWorkerSchedulerImpl`: - `bis_resend_buffers: HashMap<String, BisResendBuffer>` — per-endpoint. - `next_bis_broadcast_id: u64` — monotonic allocator, reset at process start (workers tolerate the reset because broadcast_ids are not stable across server restarts; see proto doc-comment). Trait impl: - `WorkerScheduler::bis_ack_received` — both the trait method and the forwarding `ApiWorkerScheduler` / `SimpleScheduler` impls (already added in the worker-arm commit; this commit fills them in). Resend-buffer keying choice (cas_endpoint vs worker_id): the worker_id is regenerated on every connect (see `inner_connect_worker`), so it cannot serve as a stable identity across reconnects. `cas_endpoint` is the worker's own identity; pairing it with `boot_epoch_id` (per TraceMachina#141) distinguishes a fresh process restart from a transient reconnect, and the boot_epoch wipe path clears the buffer atomically with the locality_map wipe. Tests (`#[cfg(test)] mod tests` in the same file): - `bis_chunked_ack_acknowledged`: 100K digests → ~25 chunks; every chunk lands in the buffer; acking each drains the buffer to zero. - `bis_chunked_ack_lost_resend`: 10K digests → ~3 chunks; ack only half; reconnect with same endpoint replays exactly the unacked half (verified by sequence equality). - `bis_chunked_boot_epoch_change_clears_buffer`: after `clear_bis_resend_buffer_for_endpoint`, reconnect receives zero replayed chunks. - `bis_chunked_full_ack_drops_endpoint_entry`: full-ack removes the per-endpoint HashMap entry entirely. TDD verification: - Mutation 1 (suppress `buf.add(chunk.clone())`) — both the buffer count assertion and the lost_resend partial-ack assertion fail with specific messages. Reverted. - Mutation 2 (suppress `replay_bis_chunks_to_worker` call from `add_worker`) — `bis_chunked_ack_lost_resend` fails with the specific "reconnect must replay" assertion. Reverted. Production wiring NOT in this commit: - `worker_api_server` boot-epoch-change clear_buffer call. - `nativelink.rs` switch from `broadcast_blobs_in_stable_storage` → `_chunked` in the BIS broadcast loop. - TLA+ spec. Those land next.
rejuvenile
added a commit
to rejuvenile/nativelink
that referenced
this pull request
Aug 19, 2026
…raceMachina#97) Models the cross-component contract for TraceMachina#97 (BlobsInStableStorage chunking + per-chunk ack + reconnect-replay): * server splits BIS broadcast into N chunks, dispatches each, adds to per-cas-endpoint resend buffer * worker receives each chunk, unpins digests, emits per-chunk BisAck * server's ack handler drops the matching (broadcast_id, sequence) from the resend buffer * on connection drop + reconnect (same boot_epoch), server replays every still-buffered chunk Two configs: * `BISChunkingAckBugged.cfg` (BugMode = TRUE): NO resend on reconnect. Reproduces the pre-TraceMachina#97 production failure mode where a dropped h2/QUIC stream during a BIS broadcast leaks pin state forever: drop wipes inFlight, Reconnect doesn't replay, the chunk's digests never unpin, the server's resend buffer never drains. TLC produces a counter-example trace where AllChunksEventuallyUnpinned and ServerBufferEventuallyEmpty both fail. * `BISChunkingAckFixed.cfg` (BugMode = FALSE): the shipped implementation. Reconnect replays the buffer (modeling `replay_bis_chunks_to_worker` from `add_worker`), and TLC verifies every chunk eventually delivers + acks regardless of the drop/reconnect interleaving. Strong fairness on per-chunk DeliverChunk + AckChunk (not weak): the actions' enabledness flickers across DropConnection + Reconnect, so WF would allow infinite Drop/Reconnect cycles to starve a particular chunk forever. SF says: if the action is enabled INFINITELY OFTEN (true after every Reconnect under FixedMode), it must fire infinitely often. This is the right semantics for an action that's intermittently disabled by an asynchronous side action. Scope (documented in the ASSUMPTIONS block): - one cas_endpoint, one broadcast (broadcast_id collision impossible in this model — sufficient for the resend invariant); - boot_epoch change handled separately via `clear_bis_resend_buffer_for_endpoint`, NOT modeled here (this spec assumes same-epoch reconnects, the bug-prone case); - worker-side unpin idempotency is set-add semantics in the model (production code makes unpin a no-op on repeat). Also fixes a `verify_tla.sh` script bug surfaced by this spec: TLC reports "were violated" (plural) when multiple temporal properties fire in the same trace; the script only matched "is violated" / "was violated", misclassifying the Bugged config as "TLC errored" and falsely BLOCKing Layer 1. The fix adds the plural pattern.
rejuvenile
added a commit
to rejuvenile/nativelink
that referenced
this pull request
Aug 19, 2026
…eviewer) The doc claimed the buffer was bounded with oldest-broadcast eviction; implementation is unconditional HashMap::insert. Replace with the truthful statement: currently UNBOUNDED, eviction relies on per-chunk ack + endpoint disconnect + boot-epoch clear. File a TODO for a disconnect-timeout cap as TraceMachina#97-followup. 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
…hunks (TraceMachina#97 reviewer) Three orthogonal scheduler-side perf wins folded into one commit since they touch the same dispatch path: 1. **Write-lock hoist (perf-optimizer M1).** broadcast_blobs_in_stable_storage_chunked previously took the inner write lock ONCE PER WORKER to record the dispatched chunks into the resend buffer. With ~64 workers per broadcast, this was ~64 sequential write-lock round-trips contending against every other scheduler operation. Restructure: snapshot senders under one read lock, dispatch outside any lock, then take ONE write lock at the end to walk every per-endpoint buffer in a single pass. Matches what the comment at the top of the function already claimed ("one write lock per broadcast"). 2. **next_bis_broadcast_id → AtomicU64 (perf-optimizer M1).** Bumping a u64 under the inner RwLock was a write-lock taken purely to compute id + 1. Move to AtomicU64 on the outer ApiWorkerScheduler and use fetch_add — lock-free, ~1ns per call. 3. **Arc-wrap chunks (perf-optimizer M1).** Each chunk's Vec<Digest> was previously cloned per worker AND once more per resend-buffer entry: at 64 workers × 25 chunks per 100K broadcast = 1600 redundant Vec clones. Wrap in Arc<BlobsInStableStorageChunk> so dispatch + buffer share allocations. The terminal proto clone at the wire boundary (UpdateForWorker construction + replay) is unavoidable — Tonic owns the message — but every intermediate handle is now an 8-byte Arc clone. BisResendBuffer::drain was dead (no caller; per-feedback_public_api_needs_caller_in_diff.md) so it is removed in the same change since it depended on the old proto value type anyway. All four existing bis_chunked 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
…ewer red-team TraceMachina#3) Previously `handle_bis_chunk` ALWAYS fired the ack after the unpin pass, even when individual digests failed to decode (DigestInfo:: try_from Err). The server then dropped the chunk from its resend buffer → on next ConnectWorker the chunk was never replayed → the failed digests stayed pinned forever. Same outcome as the original TraceMachina#89 bug (unpin-leak), different mechanism, less detectable because the success path "looked" OK. The fix: * `handle_blobs_in_stable_storage` now returns `BisUnpinOutcome { unpinned, failed }`. Today the only failure mode is `DigestInfo::try_from` Err on a malformed proto digest; `unpin_digest` and mirror cleanup are infallible. Returning a struct (not a bare bool) keeps future failure-mode growth (e.g. disk-IO-backed unpin) non-breaking. * `handle_bis_chunk` only invokes `ack_sink` when `outcome.all_succeeded()`. On partial failure, `error!` logs `broadcast_id`, `sequence`, `unpinned`, `failed`, `digest_count` so operators see the unpin-failure rate, and the server's resend buffer keeps the chunk for next-reconnect replay. * Existing tests adapted to the new return type (now read `outcome.unpinned`/`outcome.failed` instead of bare usize). Asymmetric contract coverage (per CLAUDE.md): adds `bis_ack_not_sent_when_unpin_fails` — the over-action sibling of the existing `bis_chunked_full_burst` under-action test. The new test mixes one malformed digest with four well-formed ones in a chunk and asserts: (a) outcome.failed == 1, outcome.unpinned == 4, !all_succeeded(), (b) acks.is_empty() — the SPECIFIC "must NOT emit an ack" assertion message (not a generic `is_empty()`). Wrapped in `tokio::time::timeout(5s)` per the production-composition deadlock-detector convention. Mutation step verified: commenting out the `if outcome.all_succeeded()` gate causes `bis_ack_not_sent_when_unpin_fails` to panic with "must NOT emit an ack when ANY digest in the chunk fails to decode ... Acks emitted: 1" — confirming the test guards the behavior. 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
…r round-trip (TraceMachina#97 testing-czar a) Per testing-czar requirement (a) on the TraceMachina#97 fixup pass: the existing `bis_chunk_handler_test.rs` tests `handle_bis_chunk` with a hand-rolled `ack_sink` closure, bypassing two production layers: 1. The `Update::ChunkedMessage(BlobsInStableStorageChunk)` envelope-decode arm in `LocalWorkerImpl::run`. 2. The `worker_api_client_wrapper::bis_ack` send. A bug in either layer (wrong oneof tag, dropped ack on the `tokio::spawn` boundary, missing token echo in the wrapper) would not surface from the unit test. Adds `bis_chunked_dispatch_arm_round_trips_ack` to `local_worker_test.rs`: * Constructs a real `BlobsAvailableState` over a real `FilesystemStore`. * Sends a real `Update::ChunkedMessage(BlobsInStableStorage(chunk))` through the live tonic Streaming<UpdateForWorker>. * Wraps the ack-wait in `tokio::time::timeout(5s).expect(...)` with a SPECIFIC "must NOT time out" message — the deadlock detector pattern from CLAUDE.md §production-composition. * Asserts `(broadcast_id, sequence, server_instance_token)` round-trip in the captured BisAck. Test-utility scaffolding: * `MockWorkerApiClient::bis_ack` now records into the call queue instead of dropping (was previously `Ok(()) /* not exercised */`). * New `expect_bis_ack` and `expect_bis_ack_skipping_blobs_available` helpers. The latter auto-acks the periodic `BlobsAvailable` calls that fire when `BlobsAvailableState` is set, so the BIS-dispatch test is robust to the worker-loop's first-wakeup behavior. * New `setup_local_worker_with_blobs_state` helper that plumbs a constructed `BlobsAvailableState` through the worker's `new_with_connection_factory_and_actions_manager`. * Cargo.toml: add `required-features = ["test-utils"]` for `local_worker_test` because `BlobsAvailableState::new_for_test` is gated on `cfg(any(test, feature = "test-utils"))` and cfg-test does not propagate from the lib into integration tests. Mutation step verified: replacing the BIS dispatch arm with a no-op `if false && ... { } else { warn!() }` causes the test to panic at the 5s timeout with the SPECIFIC message "must NOT time out waiting for BisAck — the worker's Update::ChunkedMessage(BlobsInStableStorage) dispatch arm must wire through worker_api_client_wrapper::bis_ack ...". 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
…ent leak When a worker reconnects with a NEW boot_epoch BEFORE the OLD's disconnect-cleanup runs, the OLD epoch's `dispatcher.worker_txs[(endpoint, OLD_epoch)]` and per-(worker, OLD_epoch) queues leak forever — OLD's later cleanup hits the ownership-check guard (NEW owns the endpoint) and SKIPS its `dispatcher.unregister_worker` / `dispatcher.unpin_on_disconnect` calls. Symmetric to the locality_map wipe (TraceMachina#141) that the same boot-epoch wipe block already performs. Currently dormant in production behind `small_blob_mirror_enabled=false`; becomes an active leak the moment the flag flips. Fix: while `endpoint_state` lock is held in the wipe block, also call `dispatcher.unregister_worker(endpoint, prev_epoch)` and `dispatcher.unpin_on_disconnect(endpoint, prev_epoch)`. The lock guarantees the OLD disconnect task observes a fully wiped dispatcher state OR an unwiped one, never a partial state that could lose entries from EITHER epoch. Tests (production composition, both directions per asymmetric- contract-coverage rule): - `boot_epoch_wipe_clears_dispatcher_state` (under-action): seeds worker_tx + queue + pin entries for (E, OLD), reconnects with (E, NEW), asserts (E, OLD) state is cleared. Wrapped in 5 s `tokio::time::timeout` deadlock detector. - `boot_epoch_wipe_does_not_clear_new_epoch_state` (over-action): reconnects with (E, NEW), asserts (E, NEW) state survives. Guards against an over-broad regression that nukes both epochs. Adds two diagnostic accessors on SmallBlobDispatcher: `has_worker_tx(endpoint, boot_epoch_id)` and `queue_count_for_worker(endpoint, boot_epoch_id)` — used only by tests; production callers should rely on send-error results. Mutation step verified: commenting out the new `unregister_worker` + `unpin_on_disconnect` calls makes `boot_epoch_wipe_clears_dispatcher_state` panic with "TraceMachina#174 leak: boot-epoch wipe failed to call unregister_worker". Sibling-bug audit (read-only — out of scope for this PR): `api_worker_scheduler.rs` has the same race class on `endpoint_to_worker` and `remove_prefetch_for_endpoint`, both keyed by endpoint with no boot-epoch ownership check. Owned by TraceMachina#97 fixup worktree. 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
TraceMachina#97 follow-up) TraceMachina#97's fixup added `local_worker_test` to the `required-features = ["test-utils"]` list in nativelink-worker/Cargo.toml so that the new `bis_chunked_dispatch_arm_round_trips_ack` test can construct `BlobsAvailableState::new_for_test` (gated on `cfg(any(test, feature = "test-utils"))`). Cargo with `required-features` *silently skips* the entire test bin when invoked without that flag — no error, just zero tests run. `native-cargo.yaml` already had a follow-up step that runs `cargo test ... --features ...worker/test-utils --tests`, so the gated suites were covered there. `nix.yaml` did NOT — its `cargo test --all --profile=smol` step silently skipped all 5 gated worker tests (local_worker_test, bis_chunk_handler_test, blobs_in_stable_storage_handler_test, download_to_directory_mirror_test, end_to_end_mirror_reconciliation_test) plus mirror_durability_test in nativelink-store. With this step added, both CI workflows execute the full gated suite. Reproducer (without this fix): cargo test -p nativelink-worker --tests --no-run → 7 test bins cargo test -p nativelink-worker --tests --no-run --features test-utils → 12 test bins After: nix.yaml's new step builds + runs the same 12-bin set as native-cargo.yaml. Mirrors the existing native-cargo.yaml pattern verbatim (same `-p` / `--features` triple, plus `--profile=smol` to match the prior step's profile and reuse incremental artifacts). 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
…x.yaml to close silent-skip)
rejuvenile
added a commit
to rejuvenile/nativelink
that referenced
this pull request
Aug 19, 2026
…erver accumulator PR 3/3 of TraceMachina#80 (Generalized streaming design). Splits the unbounded fields of BlobsAvailableNotification into bounded BlobsAvailableChunk envelopes routed through the unified ChunkedMessage envelope (TraceMachina#97/TraceMachina#98 precedent). Path A semantics: server accumulator BUFFERS chunks per (broadcast_id, worker_instance_token); only when is_last=true arrives does it materialise the full BlobsAvailableNotification and hand it to the existing handle_blobs_available, which performs remove_endpoint + register_blobs_iter + AC pin replace + mirror pipeline ATOMICALLY in one block. This prevents the half-applied-snapshot bug class where chunks 0..N-1 land but the terminal is lost and the locality_map stays partial. Scope: all 5 unbounded fields (digests, cached_directory_digests, pinned_mirror_entries, pinned_ac_mirror_entries, evicted_digests) PLUS three secondary fields (added/removed_subtree_digests, pinned_mirror_digests). Bound: per-connection cap of 8 in-flight broadcasts × 200K total accumulated entries = ~20 MB worst-case heap per connection. drop_all_inflight on disconnect. Backward compat: legacy single-message Update::BlobsAvailable arm unchanged; worker uses chunked path only when notification estimated encoded size exceeds 32 KiB (should_chunk threshold). Files: proto: worker_api.proto + pb.rs - new BlobsAvailableChunk message - chunked_message.payload += BlobsAvailable arm (tag 3) - update_for_scheduler.update += chunked_message (tag 10) util: blobs_available_chunking.rs - chunk_blobs_available(notification, ids, max) -> Vec<chunk> - should_chunk(notification) -> bool - BLOBS_AVAILABLE_PER_CHUNK = 4096 - 9 unit tests (empty, partial, multi, headers, full_snapshot repeat, all-5-fields, 1M no truncation, should_chunk thresholds) worker: local_worker.rs - BlobsAvailableState += worker_instance_token + next_broadcast_id - send_periodic_blobs_available: chunked path when should_chunk - exhaustive match arm for Update::ChunkedMessage(BlobsAvailable) - new chunked_message trait method on WorkerApiClientTrait service: blobs_available_accumulator.rs - BroadcastAccumulator (per-broadcast state) - BlobsAvailableAccumulator (per-connection container) - merge_chunk validates token, store_id, is_full_snapshot, sequence; commits + drops on terminal - drop_all_inflight on disconnect - 11 unit tests covering happy path + token/store_id/seq drift + cap firing + 5-field reassembly service: worker_api_server.rs - WorkerConnection += blobs_available_accumulator field - Update::ChunkedMessage(BlobsAvailable) dispatch arm: accumulator.merge_chunk -> on terminal, handle_blobs_available(reassembled notification) - drop_all_inflight in disconnect cleanup test util: local_worker_test_utils.rs - MockWorkerApiClient::chunked_message + ChunkedMessage variants in WorkerClientApiCalls/Returns enums Composite invariant (admission/eviction/pin triangle, CLAUDE.md): per-conn cap (admission) ⇒ drop_all_inflight on disconnect (explicit eviction) AND drop on next is_last=true (TTL via terminal-chunk commit) Tests: per_conn_broadcast_cap_drops_overflow, drop_all_inflight_clears_partial_state, three_chunks_terminal_commits. Cap justification per CLAUDE.md "Unbounded in-process buffers": CAPPED AT 8 broadcasts × 200K entries: ~20 MB worst-case per connection. With ~10 worker connections, ~200 MB worst-case heap on the accumulator alone — tolerable inside the 80 GB MemoryMax. Falsification: synthetic 800-broadcast / 2M-entry test MUST trigger cap eviction rather than OOM (covered by the cap-drop 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
Models the deferred-wipe-until-terminal contract from the TraceMachina#99 server accumulator. Two cfg files cover both modes: * Fixed (BugMode=FALSE): Path A — wipe defers to is_last=true. Mid-broadcast drop discards partial accumulator; LocalityMapMatchesEndpointOrPriorFull holds. * Bugged (BugMode=TRUE): Path B — per-chunk wipe. Mid-broadcast drop leaves serverView as a strict-subset partial; LocalityMapMatchesEndpointOrPriorFull violated. Modeled after specs/BISChunkingAck.tla (TraceMachina#97 precedent). 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
…-ack worker buffer + server ack-send
Wires the additive `BlobsAvailableAck` (proto landed in bd474488) into a
worker-side bounded resend buffer + per-chunk drain-on-ack + an over-cap
full-snapshot reset valve, and makes the server emit a per-chunk ack after it
applies each `BlobsAvailableChunk`. This is the design's §3.8 mechanism (v2
`durability-ack-architecture-design-2026-06-24-v2.md` §3.8; v3 carries it
UNCHANGED in §KEEP), the reliable-delta prerequisite the reconcile / eviction
gate (§3.4 lost-reply self-heal) depends on.
WHAT (mechanism, file:line):
- Worker: `BlobsAvailableResendBuffer` (BTreeMap<(broadcast_id,sequence),
BlobsAvailableChunk>, `// CAPPED AT BLOBS_AVAILABLE_RESEND_MAX_CHUNKS = 256`)
on `BlobsAvailableState` (local_worker.rs). `buffer_delta_chunk` buffers each
SENT delta chunk; `handle_blobs_available_ack` drops the matching slot
(token-guard: echo == current worker_instance_token, reject 0; per-chunk drop;
unknown-ack no-op). A new `Update::BlobsAvailableAck` dispatch arm calls it.
- Worker send path (`send_periodic_blobs_available`): a full snapshot clears the
buffer (it supersedes every delta); a DELTA buffers each chunk after send. An
over-cap accumulation clears the buffer + sets
`blobs_available_force_full_snapshot`, which promotes the NEXT tick to a full
snapshot (the convergence the removed 60s heartbeat used to provide, now an
EVENT).
- Server (worker_api_server.rs `Update::ChunkedMessage(BlobsAvailable)`): uses
the new `BlobsAvailableAccumulator::merge_chunk_outcome` (returns
`MergeOutcome::{Accepted(Option<Notification>),Dropped}`) — on Accepted it
sends a per-chunk `BlobsAvailableAck` echoing (broadcast_id,sequence,
worker_instance_token) via `worker_tx`; on Dropped it sends NO ack (so the
worker keeps the chunk buffered, mirroring handle_bis_chunk's suppress-on-fail).
`merge_chunk` is preserved as a thin Option-returning wrapper (zero ripple to
its ~15 existing callers/tests).
CONFIRM-NECESSITY deviations from the design's literal text (CLAUDE.md §3):
- The design §3.8 lists FOUR drain-before-send sites to convert; this commit
buffers ONLY the two DELTA sites. A full snapshot is self-correcting (re-derived
from a whole-store scan on reconnect), so buffering it is unnecessary; only a
one-shot delta needs the drain-on-ack guarantee. The full-snapshot sites stay
drain-on-send and additionally CLEAR the buffer.
- The design's "replay every unacked chunk on reconnect" is SUBSUMED by the
existing full-snapshot-on-reconnect (which re-advertises the worker's whole
inventory). No separate replay path is added; the `unacked_chunks` accessor was
written then removed as dead.
- `BLOBS_AVAILABLE_RESEND_MAX_CHUNKS = 256` (declared local_worker.rs): sized to
one broadcast's MAX_SEQUENCES; worst-case ~63 MiB, near-empty in steady state.
ARCHITECTURAL SCOPE FLAG (Tier-3, .claude/rules/architectural-changes.md): this
commit does NOT add per-tick replay of unacked chunks. The design's full
"retransmit until acked" guarantee (§3.8 "leaves the deltas in the tracker … next
tick retransmits") is a flow-control change (drain-on-send -> drain-on-ack on the
LIVE delta path) that is DEFERRED to a later stage with explicit sign-off. As
shipped, a mid-connection lost ack is healed by the next reconnect's full snapshot
OR the over-cap valve — strictly NO WORSE than today's drain-on-send (a lost
delta is already permanent-until-reconnect today), plus the new over-cap
convergence. The buffer-on-send added here is ADDITIVE (a bounded copy of
already-sent chunks; the send timing is unchanged; no .await added on a send, no
tokio::spawn removed, no buffer moved).
--- INVARIANT-WALK (.claude/rules/reviewer-dispatch.md) ---
Invariant being violated: a worker->server `BlobsAvailable` DELTA send is
fire-and-forget with the change-tracker drained on send, so a lost delta leaves
the server's locality view permanently stale until the worker reconnects — the
orphaned-replica hole that defeats the re-pull (§3.2a) + reconcile (§3.4).
Mechanism that violates it: `send_periodic_blobs_available`
(local_worker.rs) drained the tracker destructively (`tracker.swap()`) and
sent each chunk with no ack/retain, so a dropped `chunked_message` send lost the
delta with no record.
Mechanism that re-establishes it after the fix: each sent DELTA chunk is retained
in the bounded `BlobsAvailableResendBuffer` and dropped only when the server's
per-chunk `BlobsAvailableAck` arrives (`handle_blobs_available_ack`); an over-cap
accumulation forces a full-snapshot re-converge; the server emits the ack only
for chunks it ACCEPTED (`merge_chunk_outcome` -> Accepted), suppressing it on
drop so the worker re-advertises on reconnect. (Stage 2 closes the
RECORD-and-converge half; the per-tick-retransmit half is the deferred flow-control
stage.)
Composite invariants this fix interacts with: the ≥2-replica + BIS-ack durability
oath (UNCHANGED — this is locality-delta reliability, not the ack-gate); the
chunked accumulator's Path-A atomic-commit + per-conn caps (UNCHANGED — the ack is
emitted around merge_chunk_outcome, which preserves every existing
acceptance/drop/cap path; 24 accumulator unit tests green); the
worker_instance_token nonce (TraceMachina#97/TraceMachina#99) reused as the ack echo guard; NO TTL / NO
timer added (the over-cap valve is event-triggered, R2/R3).
Test that proves the fix re-establishes the invariant:
blobs_available_ack_test::{ack_drops_matching_buffered_chunk,
stale_worker_token_ack_is_dropped, token_zero_ack_is_dropped,
unknown_ack_is_harmless_noop, over_cap_send_forces_full_snapshot_reset} (worker
drain-on-ack seam) + blobs_available_ack_server_test::{accepted_chunk_emits_ack_per_chunk,
token_zero_chunk_is_dropped_no_ack} (server per-chunk ack emission/suppression).
Behavior changes (.claude/rules/architectural-changes.md):
- Server now SENDS a `BlobsAvailableAck` on the `UpdateForWorker` stream for every
ACCEPTED worker `BlobsAvailableChunk` (was: silently consumed, no reply). Old
workers ignore the unknown tag-12 arm (catch-all warn+continue) — safe
degradation. Verified in production composition (server stream harness):
blobs_available_ack_server_test.
- Worker now BUFFERS each sent delta chunk (bounded, capped) and drains it on the
matching ack; over-cap forces a full snapshot next tick. The SEND timing and the
tracker drain are UNCHANGED; this is an additive bounded copy + an event-driven
convergence valve, not a flow-control flip (per the scope flag above). New
worker `Update::BlobsAvailableAck` handler is a no-op when no ack arrives.
- NO change to: the ack-gate / BIS durability oath, the chunked accumulator
accept/drop/commit/cap logic, pin/eviction paths, store chains, any timer/TTL.
No fsync/sync primitive. No async↔sync flip.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
rejuvenile
added a commit
to rejuvenile/nativelink
that referenced
this pull request
Aug 19, 2026
…the resend buffer + small/post-action delta coverage Finishes Stage 2: the resend buffer added in `af777280` was an honest no-op scaffold (3 reviewers convergent — it RECORDED + DROPPED-on-ack but had NO READER, and only the >32 KiB chunked delta was ever acked). (B) closes the orphaned-replica hole §3.4 depends on for ALL deltas. USER-AUTHORIZED architectural sign-off on the retransmit-at-the-worker->server-boundary (ack-semantic) change. FRAMING (corrected per Tier-3 pair-a P1 — this is NOT a TraceMachina#203-class buffer-source-drain flip): (B) does NOT move where the in-flight buffer lives and does NOT change the delta SOURCE — `state.tracker.swap()` still drains the change-tracker on send, UNTOUCHED by (B). What (B) adds is (a) a per-tick REPLAY READER over the already-existing, already-capped resend buffer and (b) routing for two more delta producers (small periodic delta; post-action output-publish delta) onto that same acked/chunked/buffered path. The DERIVED chunk already buffered by the scaffold (`buffer_delta_chunk`, send-then-buffer) now has a consumer that retransmits it until acked: a delta is RETAINED until the server acks it, and a lost send is RETRANSMITTED on the next tick, converging the server's locality view WITHOUT a reconnect. The architectural surface is the retransmit (a retry at the worker->server boundary) + the per-chunk-ack termination dependency — NOT a source-drain timing change, NOT a buffer relocation, NOT a spawn/await change. WHAT (mechanism, file:line): 1. Per-tick REPLAY READER — `LocalWorkerImpl::replay_unacked_chunks` (local_worker.rs), called at the HEAD of `send_periodic_blobs_available` (before the empty-tick skip-gate, skipped on `is_first`). Snapshots every still-unacked chunk via `BlobsAvailableResendBuffer::unacked_chunks` (CLONE, not drain — the chunk stays buffered until its ack) and retransmits each verbatim (same `(broadcast_id, sequence, worker_instance_token)`) so the server's accumulator is idempotent and the worker's drop-on-ack key matches. Rides the EXISTING event/backstop `select!` tick in `run` — NO new periodic timer. Placed before the skip-gate so an OTHERWISE-IDLE worker still retransmits its unacked backlog (the exact stale-forever scenario). 2. Small-delta coverage — the periodic send gate is now `!is_first || should_chunk(¬ification)`: every DELTA rides the chunked/ACKed/buffered/replayed path (the common case: most ticks carry < 100 entries via the legacy `blobs_available()`, which the server NEVER acked). A FULL SNAPSHOT keeps the size-gated decision (it is self-correcting, not buffered). 3. Post-action coverage — `send_post_action_blobs_available_delta` (local_worker.rs, called from the `make_publish_future` publish closure, was `grpc_client.blobs_available(...)` at ~:4378) routes the post-action output-digest delta through the same chunk+buffer-on-send path when the worker has `blobs_available_state`; falls back to the raw fire-and-forget send when `None` (workers with no fast-store — unchanged for them). CONFIRM-NECESSITY: the resend buffer's payloads (justified in `af777280`) are now load-bearing — they ARE what gets retransmitted. The `unacked_chunks` accessor (written-then-deleted-as-dead in `af777280`) is revived because it now HAS a reader. §3.4's "closes the orphaned-replica hole" becomes TRUE for all deltas (chunked AND small AND post-action). --- INVARIANT-WALK (.claude/rules/reviewer-dispatch.md) --- Invariant being violated: a worker->server BlobsAvailable DELTA send is fire-and-forget with the change-tracker drained on send, so a LOST delta on a still-up connection leaves the server's locality view permanently stale until the worker reconnects — the orphaned-replica hole that defeats the re-pull (§3.2a) + reconcile / eviction-gate self-heal (§3.4). Stage-2's buffer recorded the lost delta but never re-sent it (no reader), and small + post-action deltas were never acked at all (legacy single-message path). Mechanism that violates it: `send_periodic_blobs_available` drained the tracker (`tracker.swap()`) and sent each delta with no retransmit; small deltas took the unacked `grpc_client.blobs_available()` arm; the post-action publish used the same unacked call. Mechanism that re-establishes it after the fix: `replay_unacked_chunks` (head of `send_periodic_blobs_available`, before the skip-gate) retransmits every unacked chunk each tick until the server's per-chunk `BlobsAvailableAck` (`handle_blobs_available_ack`) drains the slot; small + post-action deltas now ride the chunked ACKed path so they too are acked + buffered + replayed. The tracker-drain-on-send is UNCHANGED — the re-establishment is the new reader over the buffer, not a change to where/when the source drains. Composite invariants this fix interacts with: the >=2-replica + BIS-ack durability oath (UNCHANGED — this is locality-delta reliability, not the ack-gate); slow-write-never-cancelled (UNTOUCHED — replay only re-sends BlobsAvailable chunks; no write is aborted); no-new-timer / event-driven (UPHELD — replay rides the existing `select!` tick); the over-cap full-snapshot reset valve (UNCHANGED — still fires on buffer overflow, and is load-bearing as one of the three replay stop conditions); the chunked accumulator's Path-A atomic-commit + per-conn caps (UNCHANGED — the replay is just another `chunked_message`, ACCEPTed idempotently); the server's ack-every-accepted-chunk behavior (incl. non-terminal) is load-bearing for replay termination — moving the ack inside the terminal-commit block would cause an unbounded resend storm; the worker_instance_token nonce (TraceMachina#97/TraceMachina#99) reused as the ack echo guard. Test that proves the fix re-establishes the invariant: blobs_available_replay_test::lost_delta_is_retransmitted_until_acked_and_locality_converges (drives the REAL worker send loop + real server BlobsAvailableAccumulator + real BlobLocalityMap: tick-1 delta is LOST (no ack), tick-2 — NO new changes, NO reconnect — replays it, the server locality view CONVERGES (`has_digest`), buffer drains on ack) + post_action_delta_routes_through_chunked_path_and_buffers_for_replay + post_action_delta_falls_back_to_raw_send_when_no_state. Behavior changes (.claude/rules/architectural-changes.md): - PER-TICK REPLAY READER (architectural, USER-AUTHORIZED): a reader is added over the existing capped resend buffer; the LIVE periodic delta path now RETRANSMITS every still-unacked chunk each tick until acked (was: buffered with no reader → a lost delta on a live connection stayed buffered forever). This is retransmit-at-the-worker->server-boundary, NOT a source-drain flip: `tracker.swap()` (the delta source) is UNCHANGED; the buffer was NOT moved; no `.await` was added on a channel send that lacked one (the send was already awaited); no `tokio::spawn` was removed. Verified in production composition (real send loop + real accumulator + real locality_map): blobs_available_replay_test. - Every periodic DELTA now rides the chunked path (`!is_first || should_chunk`); the legacy single-message `blobs_available()` is no longer used for the periodic delta (still used for full snapshots below the size threshold and for the post-action None-fallback). Wire-equivalent (server reassembles before handle_blobs_available). Verified: pending_bis_heartbeat_test (heartbeat delta now arrives as a ChunkedMessage) + blobs_available_replay_test. - The post-action output-digest publish (make_publish_future) routes through the chunked ACKed/buffered path when `blobs_available_state` is Some; unchanged raw fire-and-forget send when None. The existing post-action tests (`setup_local_worker` passes None) are unaffected — verified local_worker_test + kill_upload_tail_publish_seam_test + tree_proto_cache_test all green. - NO change to: the ack-gate / BIS durability oath, the slow-write watchdog (never aborts), the chunked accumulator accept/drop/commit/cap logic, pin/eviction paths, store chains, any timer/TTL. No fsync/sync primitive. No async<->sync flip. 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
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 upgrades cargo dependencies.
This change is