Skip to content

Remove dependency on rust-nightly - #97

Merged
allada merged 1 commit into
masterfrom
remove-rust-nightly
Jul 21, 2022
Merged

allada merged 1 commit into
masterfrom
remove-rust-nightly

Conversation

@allada

@allada allada commented Jul 21, 2022 •

Copy link
Copy Markdown
Contributor

Also upgrades cargo dependencies.


This change is Reviewable

Also upgrades cargo dependencies.
@allada
allada force-pushed the remove-rust-nightly branch from 6fbe8db to f35d676 Compare July 21, 2022 15:53

@allada allada left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@allada allada left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@allada
allada merged commit 41028a9 into master Jul 21, 2022
@allada
allada deleted the remove-rust-nightly branch July 21, 2022 16:34
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
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(&notification)`: 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant