Update the Fixed Buffer Dependency by MarcusSorealheis · Pull Request #502 · TraceMachina/nativelink

MarcusSorealheis · 2023-12-18T21:27:24Z

Description

The version of Fixed-Buffer used by the project is a bit out of date. I'm updating the dependency to the most recent version, which introduced quite a few changes to the library that improve safety.

Fixes # (issue)

Type of change

Please delete options that are not relevant.

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to
not work as expected)
This change requires a documentation update

How Has This Been Tested?

Please also list any relevant details for your test configuration

Checklist

Updated documentation if needed
Tests added/amended
bazel test //... passes locally
PR is contained in a single commit, using git amend see some docs

This change is

aaronmondal

It's failing because the Cargo.Bazel.lock file needs to be update via CARGO_BAZEL_REPIN=1 bazel test ... (it's ok if the command itself fails locally, but it should output an updated lockfile).

Reviewable status: 0 of 3 files reviewed, all discussions resolved

allada

Reviewed 3 of 5 files at r6.
Reviewable status: 3 of 5 files reviewed, 2 unresolved discussions (waiting on @MarcusSorealheis)

a discussion (no related file):
It looks like async_fixed_buffer and the related tests can be outright deleted. It looks like we don't use it any more in the code and use buf_channel everywhere instead.

I think this file is residual from when we used the AsyncRead/AsyncWrite style of reading/writing instead of AsyncStream style writing.

-- commits line 3 at r3:
nit: Squash these please.

aaronmondal

Reviewed 5 of 5 files at r10, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @MarcusSorealheis)

nativelink-util/Cargo.toml line 20 at r10 (raw file):

lru = "0.12.1"
parking_lot = "0.12.1"
pin-project = "1.1.3"

Is this needed?

MarcusSorealheis · 2023-12-19T02:39:02Z

Is this needed?

Nor is pin project lite

allada

Reviewed 2 of 5 files at r10, 3 of 3 files at r12.
Reviewable status: all files reviewed (commit messages unreviewed), 3 unresolved discussions (waiting on @aaronmondal and @MarcusSorealheis)

nativelink-util/Cargo.toml line 20 at r10 (raw file):

Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…

Is this needed?

Not any more.

…nk API boundary Sibling of the TraceMachina#500 inner_read fix (fb070f2). Same Bazel-visible wire shape — stream ends with status=ok and bytes_sent < expected_size, client treats as canonical complete stream, OutputDigestMismatchException fires as build failure — but a different producer-side trigger. ## Invariant being violated `StreamingBlobReader::next_chunk` returns `Ok(Bytes::new())` (clean EOF) ONLY when the writer's `bytes_written` equals `digest.size_bytes()`. Any terminal=Ok with `bytes_written < digest.size_bytes()` is silent-short corruption and must surface to consumers as `Err(_)` — never silent EOF — so that downstream classifiers and Bazel's `RemoteRetrier` get a chance to react. ## Mechanism that violates it (pre-fix) `StreamingBlobWriter::send_eof` at `nativelink-util/src/streaming_blob.rs:389` does not validate `bytes_written` vs `digest.size_bytes()`. When fired with partial bytes, the reader's terminal-state branch at the prior `:675` returned `Ok(Bytes::new())` — silent-short EOF to every consumer (`bytestream_server::inner_read::streaming_read_while_write` unfold, `fast_slow_store::drain_streaming_buffer`, `fast_slow_store::get_part` streaming-populate reader). The only production caller of `send_eof()` on a `StreamingBlobWriter` is `fast_slow_store::spawn_populate_producer` at `:3735`. When `slow_store.get` silently truncates (returns Ok but emits fewer bytes than `head_result.size`), the populator's `streaming_terminal` resolves to Ok and `send_eof` fires with partial bytes. The future is shape- preserved at the API boundary: any producer that calls `send_eof` after sending fewer bytes than `digest.size_bytes()` produces silent-short corruption. ## Mechanism that re-establishes it (post-fix) `StreamingBlobReader::next_chunk` at `nativelink-util/src/streaming_blob.rs:691` loads `bytes_written` and `digest.size_bytes()` inside the terminal-Ok branch. When `bytes_written < expected_size`, the function returns `Err(Code::Internal)` carrying the stable `STREAMING_BLOB_SILENT_SHORT_MARKER` substring (the constant pattern established by `SLIDING_WINDOW_EVICTION_MARKER` for TraceMachina#325 D.1), instead of the historic `Ok(Bytes::new())`. Defense-in-depth at the API boundary — every current AND future `StreamingBlobReader` consumer inherits the contract. The `bytestream_server::streaming_read_while_write` unfold at `:1565` and `:1606` matches `Err(e) => Some((Err(e.into()), state))`, so Bazel sees `status=error` and retries (verified end-to-end against bazel 9.1.0 in `audits/504-bazel-mid-stream-retry-verification-2026-05-16.md`) instead of accepting a clean-short stream as canonical bytes. ## Why Code::Internal, not Code::DataLoss The original commit returned `Code::DataLoss` from the silent-short check. After-the-fact distribution analysis on the 48 h buildcache journal (2026-05-14 → 2026-05-16) at `.claude/audits/502-distribution-investigation-2026-05-16.md` shows the silent-short class is overwhelmingly TRANSIENT, not deterministic per-blob corruption: - 73.5 % of digests that silent-shorted at least once ALSO completed successfully in the same 48 h window (median per-digest success rate 25 %, range 0.6-80 %). For deterministic per-blob corruption you would expect 0 % co-survival; the data rules that mechanism out. - 99.2 % of events truncate at a clean 1/N fraction of expected_size with N in {2..16} — the chunk-boundary signature of a transport-layer chunker race, not byte-level corruption. - 38.3 % of events share their `bytes_sent` value with at least one other distinct digest; the truncation point is buffer-bounded, not blob-bounded. - Bursty load correlation (17 seconds with >100 events; 9 of 48 hours account for all events; busiest second 239 events) — load-triggered race, not constant-rate corruption. Bazel's `RemoteRetrier::isRetriable()` (verified against bazel 9.1.0 in `audits/504-bazel-mid-stream-retry-verification-2026-05-16.md`) maps `Code::Internal` to TRANSIENT_FAILURE → retried up to 10× with exponential backoff; with the observed 25 % per-attempt success rate that converges to ~94 % effective recovery (0.75^10 = 0.0563 ⇒ 94.4 %). `Code::DataLoss` maps to PERMANENT_FAILURE → no retry → action fails on the first hit. Given the data, `Internal` is the correct classification at this seam. The counter-argument that `worker_proxy_store.rs:1018` already uses `Code::DataLoss` for digest-mismatch does NOT apply here. That site has post-hash evidence that the delivered bytes are wrong; no amount of retrying will reconcile a SHA-256 mismatch, so `DataLoss` is correct there. `StreamingBlobReader::next_chunk` does NOT have that information — it knows only that `bytes_written < expected_size` at a terminal-Ok branch, which the distribution data shows is dominantly a transport race. Different seams, different correct classification. On the residual ~6 % of truly-stuck cases where retries can't help, Bazel still fails the action after exhausting its 10-retry budget — same end-state as `DataLoss`, just slower. There is no downside to `Internal` on the deterministic-corruption tail. ## Production observation that motivated the fix `LoggingReadStream::log_completion` on buildcache journal 2026-05-15 shows events at the `bytestream_server::zero_copy_read` label with `expected_size: 47291739, bytes_sent: 7881957, status: "ok"` — a 6 MiB- class silent-short surfacing through `inner_read`'s `streaming_read_while_write` unfold for in-flight blobs. The pre-fix unfold consumed the `Ok(Bytes::new())` from `next_chunk` and yielded `None` (clean EOF), so Bazel saw status=ok with bytes_sent < expected_size and reported digest mismatch as a build failure. `LoggingReadStream` catches `silent_zero` (bytes_sent == 0 + expected > 0) but did NOT catch silent-short (0 < bytes_sent < expected_size); the defense at the streaming_blob API boundary is the structural fix. ## Composite invariants this fix interacts with This is an asymmetric-contract change on `StreamingBlobReader`'s `next_chunk`. Per CLAUDE.md "Asymmetric contract coverage": - under-action (terminal=Ok with partial bytes): MUST surface as Err(Internal). Tested by `silent_short_eof_surfaces_as_internal` and `populate_path_silent_short_propagates_to_reader_as_err`. - over-action (terminal=Ok with full bytes — the happy path): MUST surface as clean EOF. Tested by `full_byte_eof_observes_clean_terminal` and (for the 0-byte digest edge case) `zero_byte_digest_eof_observes_clean_terminal`. Existing tests that previously called `send_eof` with bytes_written < 1024 (the default `test_digest` size) now use the new `test_digest_with_size` helper to match the digest size to the actual payload — they tested terminal-Ok semantics, not silent-short behavior, so the size mismatch was an accidental side-effect of an overgenerous default. ## Tests that prove the fix re-establishes the invariant `silent_short_eof_surfaces_as_internal` — minimal under-action: writer sends 4 of 1024 bytes + send_eof; reader MUST observe Err(Internal) with the stable marker substring, NOT silent EOF. Bespoke `.expect_err` names TraceMachina#502 and the 2026-05-16 audit. `populate_path_silent_short_propagates_to_reader_as_err` — production- composition shape: 7.88 MiB sent of 47.29 MiB expected (scaled from the buildcache observation). Reader MUST observe Err(Internal) so the bytestream_server `streaming_read_while_write` unfold yields `Some((Err, _))` and Bazel's RemoteRetrier retries (transient classification). `full_byte_eof_observes_clean_terminal` — over-action: writer sends exactly 8 of 8 bytes + send_eof; reader MUST observe clean EOF (Ok empty), NOT a spurious Internal-error. Guards against a too- aggressive check that would break every CAS read. `zero_byte_digest_eof_observes_clean_terminal` — edge case: 0-byte digest with no chunks + send_eof; clean EOF, NOT Internal. Guards against a future refactor that flips the `<` inequality to `<=` and breaks zero-byte CAS reads. All four tests wrap `next_chunk` in `tokio::time::timeout(1-2 s)` as deadlock detector per CLAUDE.md "Test in production composition". ## Mutation verification Mutation: reverted `Code::Internal` back to `Code::DataLoss` in `next_chunk`. - `silent_short_eof_surfaces_as_internal` red-fails with bespoke `assert_eq!` message naming Code::Internal and the audit. - `populate_path_silent_short_propagates_to_reader_as_err` red-fails with bespoke `assert_eq!` message naming the seam. - Over-action tests still pass (correct — they assert happy-path semantics that the mutation does not affect). Re-mutation (commenting out the `if bytes_written < expected_size` block entirely) red-fails the same two tests at the `.expect_err` boundary — both layers of the contract are enforced. ## Sibling-bug audit Workspace-wide grep for `Ok(_) if _.is_empty() => return None|break|=> None`: - `bytestream_server.rs:1505, 1592` (`streaming_read_while_write` unfold) — both protected by the API-boundary fix; consumes `Err(Internal)` and propagates via `Some((Err, _))`. - `fast_slow_store.rs:3809` (`drain_streaming_buffer`) — protected; Err arm propagates as `Err(err)`. - `fast_slow_store.rs:6304` (`get_part` streaming-populate reader) — protected; Err arm handles `Unavailable` (sliding-window) specially and otherwise propagates Internal correctly via `Err(guard.fail(err))`. - `chunked_write_handler.rs:4087` uses `buf_channel::recv()`, not `StreamingBlob::next_chunk`, and already validates byte count at `:4101-4103`. Different primitive, not in scope. Only the `StreamingBlob::next_chunk` API boundary needs the new check; no per-consumer modifications required. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

…runs Brings the specs/ directory up to current origin/main + adds the new StreamingBlobReadDuringWrite spec covering N concurrent slow readers x slow writers with read-during-write over the streaming_blob primitive. WHAT LANDED 1. ChunkRaceReadersV2.tla: appended "RECENT PROTOCOL EVOLUTION" block walking TraceMachina#447 / TraceMachina#508 / TraceMachina#502 / TraceMachina#500 / TraceMachina#487 / TraceMachina#503 / TraceMachina#504 and stating why each commit did or did not require a spec edit. No model changes. 2. Layer-1 verify_tla.sh gate fixes (specs-only, no tooling edits): - Added ChunkRaceReadersV2Fixed.cfg + ChunkRaceReadersV2Bugged.cfg so the gate's *Fixed.cfg / *Bugged.cfg classifier recognises a canonical pair for the ChunkRaceReadersV2 spec. - Removed legacy specs/ChunkRaceReadersV2.cfg (duplicate of Fixed; classifier read *V2.cfg as bugged and FAIL'd on the clean run). - Renamed ChunkRaceReadersV2_BuggedH3.tla -> ChunkRaceReadersBuggedH3.tla (and its three cfgs in tandem) to drop the shared `ChunkRaceReadersV2_` prefix; the gate's `${name}*.cfg` discovery glob no longer cross-runs H3-specific cfgs (referencing `H3HazardStateUnreachable`) against the canonical ChunkRaceReadersV2 spec that does not define that invariant. - Added ChunkRaceReadersBuggedH3Fixed.cfg + ChunkRaceReadersBuggedH3Bugged.cfg to satisfy the gate's per-spec Fixed+Bugged pair requirement on the renamed module. 3. NEW spec StreamingBlobReadDuringWrite.tla: models concurrent slow-writers x slow-readers x read-during-write over the streaming_blob primitive (nativelink-util/src/streaming_blob.rs). Captures the post-TraceMachina#500 + post-TraceMachina#502 wire-shape contract between StreamingBlobWriter::{send,send_eof,send_error,Drop} and StreamingBlobReader::next_chunk. CONSTANTS toggle the fix gates: ShortShieldOn (TraceMachina#502 boundary defense), SilentZeroPropOn (TraceMachina#500 producer-Err propagation), AllowShortEof/AllowErrEof/AllowDropWithoutEof for writer fault injection. Safety invariants added: NoSilentZeroToReader, NoSilentShortToReader, NoCorruptBytesToReader, PostFixReaderOutcomeIsTwoWay, ReaderOkImpliesWriterFullOk, PostFixErrPropagationReachesReader, PostFixFullOkReachesReaderAsOk, CursorAtMostChunkCountAtTerminal. Liveness: WriterEventuallyTerminates, ReadersEventuallyTerminate, EventualConsistencyOkFull, NoEternalParkAfterWriterDone. Configs: - StreamingBlobReadDuringWriteFixed.cfg (all gates ON, all writer fault modes ON) — clean, 795 states. - StreamingBlobReadDuringWriteBugged.cfg (ShortShieldOn=FALSE) — NoSilentShortToReader violated, 24 states. - StreamingBlobReadDuringWrite_BuggedSilentZero.cfg (SilentZeroPropOn=FALSE) — NoSilentZeroToReader violated, 32 states. - StreamingBlobReadDuringWrite_1w_3r_3c.cfg — RDW concurrency (17 740 states clean). - StreamingBlobReadDuringWrite_1w_4r_2c.cfg — more readers (70 743 states clean). TLC RESULTS verify_tla.sh full-run (Layer-1 gate): 19/19 specs PASS, 0 FAIL. All NEW invariants are falsified by their Bugged.cfg variants with SPECIFIC named-invariant violations (no generic is_err() catches). See .claude/audits/tla-slow-rw-rdw-2026-05-16.md for the falsification matrix and per-cfg trace shapes. NO production source touched — specs/ + audit doc only. VERIFIED AGAINST origin/main: 48be7c6 + TraceMachina#501 + TraceMachina#510 (post-rebase) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

…record expected_size_on_store ROOT CAUSE The TraceMachina#502 silent-short check at `StreamingBlobReader::next_chunk` (added 48be7c6 May 16) compares `bytes_written < digest.size_bytes()`. For AC reads through `FastSlowStore` populate, `digest.size_bytes()` is the *Action* proto's encoded size (the key), NOT the *ActionResult* bytes stored under that key. The AC integrity contract at `ac_server.rs:199-205` and `docs/ac-integrity-contract.md` explicitly says: `H(store_data) != digest in general; AC entries are NOT content-addressed by their bytes.` The check is structurally wrong for AC. EVIDENCE (production 2026-06-04 05:55:13-15 PDT) - 28 silent_short events, ALL from `nativelink_service::ac_server:: get_action_result` (`ac_server.rs:288`); 100% AC reads, 0% CAS. - 217-byte action_digests where bytes_written in {203, 204, 205, 207, 208, 209, 211, 213, 215} - distinct values across distinct digests sharing expected_size=217. - Bursty (260 at 21:09 PDT June 3 deploy-cold-start + 28 at 05:55 PDT June 4) because populate path only fires on fast-tier miss (`AC_BACKEND_CACHED.fast = MemoryStore(4GB/500K)`). - Chain: `AcServer::get_action_result` -> `CompletenessCheckingStore:: get_and_verify_single` -> `ac_store.get_part_unchunked(key, 0, Some(10MB))` -> `ExistenceCacheStore::get_part` -> `FastSlowStore` (AC_BACKEND_CACHED) populate -> `StreamingBlobInner::new(action_digest, ...)`. FIX (v2: OnceLock<u64>, replaces v1 AtomicU64 + sentinel-0) Add `expected_size_on_store: OnceLock<u64>` to `StreamingBlobInner`. `OnceLock` semantics: `Some(n)` records the authoritative bytes-on-store size from `slow_store.has().ExactSize(n)`; `None` means caller does not know the size (falls back to `digest.size_bytes()`). Crucially, `Some(0)` is a legitimate value (zero-byte AC entries exist) and does NOT collide with "unset" - that collision was the v1 AtomicU64-sentinel-0 bug (red-team P3, code-reviewer TraceMachina#1). `FastSlowStore::run_producer` calls `streaming_writer.set_expected_size_on_store(n)` after the `slow_store.has()` returns `ExactSize(n)` and BEFORE any chunk is sent. Readers observe terminal state through the existing `terminal` mutex, which establishes the happens-before edge back to `OnceLock::set`. - CAS reads: `slow_store.has()` returns the file size = `digest.size_bytes` (content-addressed invariant), so the override is a no-op there. TraceMachina#502 protection is preserved. - AC reads: `slow_store.has()` returns Redis STRLEN = ActionResult encoded_len, the actual bytes-on-store size. The check now compares the right two numbers. - `LazyExistenceOnSync` paths return `MaxSize(u64::MAX)`, leaving the `OnceLock` unset - falls back to digest-based check (no regression for paths that don't know the real size). Production AC slow tier is RedisStore, which does NOT report `LazyExistenceOnSync`, so cache-miss AC reads always reach the `ExactSize(n)` branch. CONSTRUCTION SITES AUDITED (workspace-wide via grep StreamingBlobInner::new) Production: - `fast_slow_store.rs:3383` (get_loader): producer later records size via `set_expected_size_on_store` from `run_producer`. - `fast_slow_store.rs:4138` (spawn_populate_producer_with_role): same. - `streaming_blob.rs:1158` (StreamingBlob::new): leaves OnceLock unset. Used by InFlightBlobMap-adjacent paths; CAS digest is content-addressed so digest.size_bytes IS the on-store size; no override needed. - `streaming_blob.rs:1226` (InFlightBlobMap::register): same as above. Test sites: all leave OnceLock unset, preserving prior behavior. OBSERVABILITY SCHEMA NOTE The silent_short ERROR log now reports `expected_size = on-store value` (e.g. Redis STRLEN for AC) instead of always `digest.size_bytes`. Any dashboard keyed on `expected_size=217` for TraceMachina#502 alerting will see new values on AC paths. TESTS (TDD) - `ac_read_with_size_override_does_not_trip_silent_short` - production shape with 217-byte action digest + 207-byte ActionResult. - `cas_silent_short_still_trips_with_matching_override` - CAS shape with matching override; truncated stream still surfaces as Code::Internal. - `zero_byte_ac_entry_does_not_false_fire_silent_short` - the v2 regression guard. Zero-byte AC entry under non-zero action_digest.size_bytes; `expected_size_on_store = Some(0)` must NOT be conflated with unset. - `cas_silent_short_trips_when_size_on_store_unset` - unset OnceLock falls back to `digest.size_bytes()`, preserving prior CAS behavior at non-FastSlowStore construction sites. MUTATIONS VERIFIED (2026-06-04, live runs) - M1 (set_expected_size_on_store no-op): AC test red-failed with bespoke "streaming_blob_silent_short: terminal=Ok but bytes_written=207 < expected_size=217 for digest 9797…-217". - M2 (v1 AtomicU64 sentinel-0: if n==0 return): zero-byte test red-failed with bespoke "streaming_blob_silent_short: terminal=Ok but bytes_written=0 < expected_size=217 for digest 0e0e…-217". - M3 (accessor returns 0 when unset): CAS unset-fallback test red-failed with bespoke "unset OnceLock did NOT fall back to digest.size_bytes". All inline stamps in test doc-comments per TraceMachina#47 b1 T6/T8/T9 format. INVARIANT The TraceMachina#502 silent-short check's upper bound MUST equal the bytes-on-store size (`slow_store.has()` STRLEN/file_size), not the digest's declared size_bytes. These are equal under the CAS content-addressing invariant but UNEQUAL for AC where the key digest names a different proto than the stored bytes. FOLLOW-UPS - TraceMachina#50 (LazyExistenceOnSync AC): if a future AC slow tier reports LazyExistenceOnSync, the OnceLock stays unset and the bug recurs. RedisStore does not, so production AC is unaffected today. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

…ission caps (overshoot defense) Symmetric defense to TraceMachina#502's silent-short. Closes the OVERSHOOT direction of the streaming-blob primitive's bytes-on-wire contract: streaming-blob-read-bytes <= expected_size_on_store_or_digest Layer A (PRIMARY): StreamingBlobWriter::send rejects any chunk that would push bytes_written past the expected upper bound. Over-bytes never enter the buffer; no atomic state advances. Surfaces STREAMING_BLOB_SILENT_OVERSHOOT_MARKER (sibling of STREAMING_BLOB_SILENT_SHORT_MARKER) for journal-grep stability. Layer B (defense-in-depth): StreamingBlobReader::next_chunk surfaces the same marker if it observes bytes_written > expected_size (Layer A bypass via direct atomic mutation, alternate producer, or future regression). The reader-side check uses the same TraceMachina#49 expected_size_on_store() accessor — fallback to digest.size_bytes() when unset, producer-supplied size when set (AC-correct). Three production seams that share the contract: - Producer: StreamingBlobWriter::send (Layer A) - Reader: StreamingBlobReader::next_chunk (Layer B) - Server unfold: bytestream_server.rs:inner_read (Layer C, next commit) Test-only helpers added to StreamingBlobInner and InFlightBlobMap so Layer B and Layer C tests can simulate a Layer-A bypass without patching production code: - StreamingBlobInner::bytes_written_atomic() — forge bytes_written - StreamingBlobInner::append_chunk_for_test() — seed buffer w/o admission - InFlightBlobMap::insert_for_test() — direct-inject pre-built inner T1 + T2 + T4 green: - writer_rejects_overshoot_past_expected_size: T1 Layer A, MARKER surfaces + bytes_written did not advance - reader_silent_overshoot_marker_fires_on_bypass: T2 Layer B, forged bytes_written + reader refuses to emit - cas_normal_write_still_completes_under_overshoot_defense: T4 steady-state CAS write completes cleanly Identify-the-seam discipline (Audit .claude/audits/streaming-blob-cross-talk-2026-06-03.md §6): - writer producer + reader consumer covered. - server unfold (Layer C) follows in next commit. - mirror channel handler does not consume streaming-blob bytes (chunked_write_handler.rs is a different InFlightBlobMap-less path per audit §2d). Convergence with TraceMachina#49 v2: shares expected_size_on_store_or_digest accessor — the unified bound TraceMachina#49's doc-comment named for the OVERSHOOT direction. Audit: .claude/audits/streaming-blob-cross-talk-2026-06-03.md

…na#502 (cross-crate miss) streaming_blob_reader_failpoint_returns_error wrote 512 of 1024 declared bytes then send_eof — encoding, incidentally, exactly the silent-short case that TraceMachina#502 (48be7c6, 2026-05-16) made a guarded error at the reader. TraceMachina#502 scoped its caller analysis to nativelink-util and updated that crate's tests; this nativelink-store integration test was outside that scope and has been red on main for 3+ weeks (invisible because the failpoints CI step only runs on the GitHub fork remote — gitforge origin main has no CI; follow-up tracked separately). Triage verdict (read-only agent + reviewer verification): STALE-TEST, not a production bug — the TraceMachina#502 defense is correct, production-active, and guarded by 8 dedicated tests in nativelink-util (incl. silent_short_eof_surfaces_as_internal + the production-shape populate_path_silent_short_propagates_to_reader_as_err). The 2026-05-15 download-truncation incident is unrelated (OOM cross-PID stitching). Fix: deliver the remaining 512 bytes (chunks 3+4, asserted) before send_eof so the test exercises its actual contract — failpoint RECOVERY → clean EOF — plus a 5s timeout deadlock detector and bespoke expect messages on the EOF read. Behavior changes: none (test-only). Reviewers: code-reviewer + testing-czar, both APPROVE; findings (comment wording, std::time::Duration consistency, inner unwrap → bespoke expect) addressed. testing-czar confirmed no coverage loss. Suite: 14/14 green (/tmp/fp5-final log). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

…amingBlobReader Invariant being violated: A StreamingBlobReader at cursor C must emit exactly the bytes of chunk C; under sliding-window eviction it must either emit chunk C or fail with the eviction marker — never emit a different chunk's bytes. Mechanism that violates it: streaming_blob.rs:~1057 (pre-fix): `deque_idx = cursor - earliest` where `earliest` is loaded BEFORE `chunks.read()`. The producer (`StreamingBlobWriter::send`) pops chunks AND increments `earliest_chunk_idx` inside the same `chunks.write()` lock scope (lines 670-680). When cursor == earliest at the top-of-loop guard (guard passes: cursor < earliest is false), and the producer evicts between the pre-lock `earliest` load and the `chunks.read()` acquisition, the deque has shifted by 1 but `deque_idx = cursor - stale_earliest` is 1 too large. `chunks.get(deque_idx)` returns a valid but WRONG chunk (cursor+1 instead of cursor). The None guard never fires because the index is in-range. Chunk cursor is skipped → hash mismatch at VerifyStore EOF → DataLoss. Production incident: pipeline 3266, digest ad96c51f…-109283467, 2026-06-12 16:15:51 PDT. Mechanism that re-establishes it: streaming_blob.rs:1111 (post-fix): `resolve_deque_idx(cursor, post_lock_earliest)` where `post_lock_earliest` is loaded AFTER acquiring `chunks.read()`. Producer-atomicity premise verified: `pop_front` and `earliest_chunk_idx.fetch_add` both execute under `chunks.write()` (streaming_blob.rs:670-680). Therefore, once the reader holds `chunks.read()`, `post_lock_earliest` and deque contents are mutually consistent. `deque_idx = cursor - post_lock_earliest` addresses exactly chunk `cursor`. If the window advanced past cursor under the lock (None arm), the reader drops the lock and retries; the `cursor < earliest` guard fires next iteration → SLIDING_WINDOW_EVICTION_MARKER → FSS splice fallback → correct bytes from slow store. Composite invariants: - VerifyStore-on-read: still backstops; this fix prevents the wrong bytes from entering the stream before VerifyStore sees the hash mismatch. - TraceMachina#44 Layer A/B emission cap: undisturbed; `deque_idx` fix is orthogonal. - TraceMachina#502 silent-short defense: undisturbed; operates at terminal-Ok, not at chunk-index time. - FSS splice fallback (TraceMachina#515): the retry path now cleanly routes to it when cursor < post_lock_earliest, same as the existing cursor < earliest path. - `expected_size_on_store()` accessor: untouched. Test that proves the fix: streaming_blob::tests::h_alt_i_reader_at_eviction_boundary_returns_correct_chunk streaming_blob::tests::resolve_deque_idx_pure_logic Phase 0.2 audit status: The `.claude/audits/515-phase02-empirical-refutation-2026-05-17.md` conclusion ("benign per Phase 0.2 verification") is SUPERSEDED. The 585 events in that soak were all slow-reader (cursor < earliest) events that hit the clean splice fallback; the fast-reader-at-eviction-boundary path (cursor == earliest at the guard, window advances under the lock) was never observed and did materialize in production on 2026-06-12. Dated addendum added at the top of the audit file. Sibling audit: No other index-into-deque or cursor-earliest computation exists in streaming_blob.rs or adjacent chunked read paths. The external-atomic + separate-deque-lock pattern is unique to StreamingBlobInner. Behavior changes: - StreamingBlobReader::next_chunk: deque_idx now derived from post_lock earliest (loaded under chunks.read()) instead of pre-lock stale earliest. For the common case (no race: post_lock == pre_lock), identical behavior. For the race case (post_lock > pre_lock by 1): previously served wrong chunk; now serves correct chunk (or retries to eviction marker). - H_alt_I log line: INFO → WARN, reworded from "benign" to "fixed-race diagnostic, post_lock_earliest now authoritative". Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

MarcusSorealheis marked this pull request as draft December 18, 2023 21:27

aaronmondal reviewed Dec 18, 2023

View reviewed changes

MarcusSorealheis force-pushed the update-fixed-buffer-2.3 branch 2 times, most recently from f5adc7c to 9a26936 Compare December 18, 2023 22:41

update the fixed buffer depdency.

7cc1a3e

MarcusSorealheis force-pushed the update-fixed-buffer-2.3 branch from 9a26936 to 7cc1a3e Compare December 18, 2023 22:50

MarcusSorealheis added 7 commits December 18, 2023 15:12

Merge branch 'TraceMachina:main' into update-fixed-buffer-2.3

3147f8b

update the fixed buffer depdency.

d46a35c

update the fixed buffer depdency.

9a777fd

update the fixed buffer depdency.

5eca3b8

fixes.

e604105

something weird with highlighting.

e0865d7

some filesystem issues.

7ffecdb

allada reviewed Dec 19, 2023

View reviewed changes

remove the files

44d36cf

aaronmondal reviewed Dec 19, 2023

View reviewed changes

removing more files.

123470c

remove dep

b4e3942

allada reviewed Dec 19, 2023

View reviewed changes

MarcusSorealheis closed this Dec 19, 2023

MarcusSorealheis deleted the update-fixed-buffer-2.3 branch December 19, 2023 03:34

Navigation Menu

Repository navigation

Update the Fixed Buffer Dependency - #502

Update the Fixed Buffer Dependency#502
MarcusSorealheis wants to merge 11 commits into
TraceMachina:mainfrom
MarcusSorealheis:update-fixed-buffer-2.3

MarcusSorealheis commented Dec 18, 2023 •

edited

Loading

Uh oh!

aaronmondal left a comment

Uh oh!

allada left a comment

Uh oh!

aaronmondal left a comment

Uh oh!

MarcusSorealheis commented Dec 19, 2023

Uh oh!

allada left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

MarcusSorealheis commented Dec 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Type of change

How Has This Been Tested?

Checklist

Uh oh!

aaronmondal left a comment

Choose a reason for hiding this comment

Uh oh!

allada left a comment

Choose a reason for hiding this comment

Uh oh!

aaronmondal left a comment

Choose a reason for hiding this comment

Uh oh!

MarcusSorealheis commented Dec 19, 2023

Uh oh!

allada left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

MarcusSorealheis commented Dec 18, 2023 •

edited

Loading