Repository navigation
Rename Turbo Cache to Native Link - #402
Merged
aaronmondal merged 2 commits intoNov 18, 2023
Merged
Conversation
MarcusSorealheis
force-pushed
the
rename-turbocache
branch
2 times, most recently
from
November 17, 2023 05:27
c6c5936 to
52e4d99
Compare
aaronmondal
suggested changes
Nov 17, 2023
aaronmondal
left a comment
aaronmondal
left a comment
Contributor
There was a problem hiding this comment.
Reviewed 6 of 237 files at r1.
Reviewable status: 6 of 238 files reviewed, 2 unresolved discussions (waiting on @allada and @MarcusSorealheis)
-- commits line 2 at r1:
Nit: Remove trailing period.
README.md line 3 at r1 (raw file):
# Native Link [](https://github.com/trace_machina/native-link/actions/workflows/main.yml)
This (and probably a bunch of other places) is an invalid link. trace_machina -> TraceMachina.
MarcusSorealheis
left a comment
MarcusSorealheis
left a comment
Member
Author
There was a problem hiding this comment.
Reviewable status: 6 of 238 files reviewed, 2 unresolved discussions (waiting on @aaronmondal and @allada)
README.md line 3 at r1 (raw file):
Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…
This (and probably a bunch of other places) is an invalid link.
trace_machina->TraceMachina.
I think this was resolved just before you reviewed the PR, see: 52e4d99
MarcusSorealheis
changed the title
MarcusSorealheis
force-pushed
the
rename-turbocache
branch
from
November 17, 2023 07:25
52e4d99 to
d9c5b8e
Compare
aaronmondal
approved these changes
Nov 18, 2023
aaronmondal
left a comment
aaronmondal
left a comment
Contributor
There was a problem hiding this comment.
Reviewed 231 of 237 files at r1, 16 of 18 files at r2, 101 of 101 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @allada and @MarcusSorealheis)
aaronmondal
approved these changes
Nov 18, 2023
aaronmondal
left a comment
aaronmondal
left a comment
Contributor
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @allada and @MarcusSorealheis)
MarcusSorealheis
added a commit
to MarcusSorealheis/nativelink
that referenced
this pull request
Nov 3, 2025
This project has expanded well beyond build caching.
rejuvenile
added a commit
to rejuvenile/nativelink
that referenced
this pull request
Aug 19, 2026
Sibling-of-TraceMachina#401 (same file region, same shape, different field). DSR review of TraceMachina#401 surfaced this: `bytestream_server.rs:337`'s `in_flight_writes: Arc<Mutex<HashMap<DigestInfo, watch::Receiver<Option<bool>>>>>` had the identical manual-insert / manual-remove pattern around `tokio::time::timeout(WRITE_TIMEOUT=300s, write_fut).await` (insert at :2395, remove at :2505). Cancellation of the wrapping gRPC future (client disconnect, RST_STREAM, server shutdown) drops `write_fut` mid-await; the manual remove never runs; HashMap entry + watch channel leak permanently per cancelled upload. Invariant being violated: every entry inserted into `in_flight_writes` MUST be removed on every exit path of the primary-writer code, including cancellation. Mechanism that violates it: `bytestream_server.rs:2393-2397` insert; `:2466-2496` `tokio::time::timeout(WRITE_TIMEOUT, write_fut).await` cancellation point; `:2505` `remove(&digest)` only on completion path. 300s cancel window per upload. Mechanism that re-establishes it after the fix: new `InFlightWritesGuard` (RAII). `from_inserted` constructs the guard from state already inserted under the dedup lock (preserves race-freedom versus a concurrent second arrival becoming a duplicate primary). `set_result(bool)` publishes the outcome to coalesced waiters via the watch channel before Drop. `Drop` removes the map entry + drops the `Sender`. On cancellation, `set_result` is never called; Drop's Sender drop signals failure to coalesced waiters via `rx.changed() = Err`, which the production loop at `bytestream_server.rs:2349-2358` already translates to false (failure). Composite invariants this fix interacts with: - watch-channel-disappearance contract for coalesced waiters: dropping the Sender MUST signal failure (preserved — `tx` is dropped in guard's Drop on every exit path). - dedup-lock atomicity: insert + check-then-insert MUST happen under one `in_flight_writes` lock to prevent two RPCs both becoming primary writers (preserved — `from_inserted` skips the insert because the caller already did it under the dedup lock at :2395). Test that proves the fix re-establishes the invariant: `nativelink-service/tests/bytestream_inflight_writes_cancel_safety_test.rs` - `guard_drop_removes_entry`: unit test of the RAII contract. - `guard_set_result_publishes_outcome`: success-path — set_result publishes the outcome to coalesced waiters BEFORE Drop removes. - `cancel_mid_await_removes_entry`: production-shape cancellation test via `select!`; asserts map empty post-cancellation with bespoke message naming bug TraceMachina#402. - `coalesced_waiter_observes_failure_on_writer_cancel`: cross-seam test composing primary writer + coalesced waiter + cancellation; asserts the waiter unblocks with `false` (rx.changed() = Err) within `tokio::time::timeout(2s)` — deadlock detector. Mutation step (per CLAUDE.md TDD TraceMachina#5): commenting out the body of `impl Drop for InFlightWritesGuard` red-fails all 4 tests, with `cancel_mid_await_removes_entry` reporting: "InFlightWritesGuard MUST remove entry on cancellation — leak detected (map still contains 1 entries after cancelled await; this is bug TraceMachina#402 — manual insert/remove around an await is not cancel-safe)". No behavior change on success/error paths: `set_result(result.is_ok())` publishes the same outcome the prior code's `tx.send(Some(result.is_ok()))` did; map removal still happens before function returns (just inside Drop, not as an explicit lock + remove). 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
…d corrupt StreamState (TraceMachina#418) Production observation 2026-05-12: post-TraceMachina#413-Option-A deploy 21:55:38 PDT, 22 follow-on `Code::Internal` failures concentrated on 2 large blobs (`38f6b374...-155605824` 155 MB, 9 retries; `bff125c2 ...-218429440` 218 MB, 11 retries — bff125c2 ABANDONED by client without successful commit). Two error variants observed depending on retry timing: `"Failed to write to data, receiver disconnected"` (buf_channel.rs:215-221, on first send-after-resume) and `"Tried to send while stream is closed"` (line 185, on subsequent sends). Both share root cause: `ActiveStreamGuard::Drop` recycles broken `StreamState` (dead `tx`, completed-with-Err `store_update_fut`) back into `IdleStream`, so Bazel client retries within the 60-sec sweeper TTL resume the same broken state. **Lifecycle invariant** (per CLAUDE.md "Admission/Eviction/Pin Composability" — `active_uploads` triangle: gate=IdleStream-resume, eviction=60s sweeper, pin=active_uploads-entry-while-in-use): a `StreamState` is RESUMABLE iff its future is paused mid-await AND its tx is connected to a live rx. Pre-fix: gate active + sweeper TTL too long for retry budget + no compensating cleanup → wedge. Post-fix: corrupt state arm closes the gate at Drop time. Discriminator is paused-vs-completed future state: - **Paused** (legitimate Bazel TCP-flap recovery): `tokio::try_join!` cancels the inner `store_update_fut` mid-await — but does NOT drop it; the future survives in `StreamState`, paused at `reader.recv().await` with `rx` still live. The wrapped body's `if result.is_err()` line never runs, so `store_errored` stays false. Recycle into `IdleStream` → next QueryWriteStatus + retry resumes cleanly. Existing tests `resume_write_success`, `restart_write_success`, `restart_mid_stream_write_success`, `resumable_write_reconnect_same_uuid`, `partial_write_bytes_counter_tracks_idle_and_resume`, `memory_pressure_evicts_oldest_idle_stream(s)` cover this path. - **Completed-with-Err** (the cascade-cancel-from-store case this fix addresses): `store.update()` returns Err, the wrapped future body completes, `store_errored` flips true, scope ends, `rx` drops. StreamState now unrecoverable; ANY retry hitting `tx.send` returns `Internal: receiver disconnected` (or, on subsequent send, `stream is closed`). Drop discards the entry. `tx.is_pipe_broken()` belt-and-suspenders is kept as future-regression defense (currently unreachable as an independent signal — every path that closes `tx` also sets `store_errored`). **Discriminator implementation:** - New `Arc<AtomicBool> store_errored` on `StreamState`. Set ONLY when the wrapped `store_update_fut` body returns Err (i.e. the future ran to completion). Cancellation does NOT set it (the future is dropped at its current suspension point and the assignment never runs). - Memory ordering: `Release` on store, `Acquire` on load (Relaxed would suffice since both happen in the same task context, but Acquire/Release is the safer choice and not measurably costly). - `ActiveStreamGuard::Drop` reads both `store_errored.load(Acquire)` and `tx.is_pipe_broken()`; if either, REMOVE the entry from `active_uploads` instead of recycling into `IdleStream`. New `warn!("TraceMachina#418: discarding corrupt StreamState ...")` for production observability. **Test:** `cancelled_chunked_write_replaced_not_recycled_on_retry` in `tests/bytestream_server_test.rs`. Production composition (real `ByteStreamServer::write` over real gRPC + real chunked store via `FailAfterNBytesStore` test fixture). `tokio::time::timeout(5s)` deadlock detector + bespoke `STREAM_CLOSED_BUG_MSG` panic message (NOT generic `is_err()` — guards against the `Elapsed` mask trap per CLAUDE.md). Test tolerates `Ok | Code::Unavailable | Code::InvalidArgument` on retry (any of these is a valid fix-path outcome); FORBIDS `Code::Internal: "Tried to send while stream is closed"` (the bug shape). Mutation: gate the corrupt-state arm to `false` → red-fails with the specific `STREAM_CLOSED_BUG_MSG`. Restored; test re-runs green. **Sibling-bug audit:** enumerated all `impl Drop for` in `bytestream_server.rs`. `InFlightWritesGuard::Drop` (TraceMachina#402) always REMOVES, no recycling — safe. `LoggingReadStream::Drop` logging only — safe. `ActiveStreamGuard::Drop` fixed. `ExpectedDropRx::Drop` is read-half mark — unrelated. `IdleStreamSweeper`: with corrupt entries removed at Drop, sweeper sees only healthy entries — unchanged behavior. Reviewer cadre (4): - distributed-systems-reviewer: APPROVE-WITH-FINDINGS — the `bad_client_input` framing concern from prior bundle reframed as "lifecycle invariant: paused vs completed" (this commit message reflects). Memory entry filed (`feedback_paused_vs_completed_future_state.md`). - code-reviewer: APPROVE-WITH-FINDINGS — doc reframe applied (this commit), `bytes_consumed` dead instrumentation removed (this commit), atomic ordering Acquire/Release confirmed correct. - testing-czar: APPROVE-WITH-FINDINGS — 152/152 crate sweep PASS, mutation independently re-verified RED-fail with bespoke message, asymmetric coverage (under-action by new test + over-action by 7 existing tests). - red-team: APPROVE-WITH-CONCERNS — flagged that the discriminator misses panic-in-`store.update()` (panic unwinds before the flag- set line). Filed as TraceMachina#422 (`TraceMachina#418 sibling: panic-safety`) for follow-up via `scopeguard::defer!` or PanicGuard RAII. Verdict: ship the named-Err-class fix; panic case is separate scope. Build: `cargo check -p nativelink-service` clean. Test: 34/34 bytestream_server_test pass; new test PASSES with mutation verified. Follow-ups: - TraceMachina#421 composability harness: assert_no_dead_state_after_drop<G> (testing-czar M3 — defer until 3rd RAII-guard member surfaces) - TraceMachina#422 panic-safety on the discriminator (red-team Q9) 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
The blocking-pool saturation investigation (.claude/audits/blocking-pool-saturation-investigation-20260512.md) parsed 50 representative back_edge_ms warns and found the dominant pattern is `mutex_acquire_ms=0, dispatch_ms=50-188, pwrite_ms=0` — i.e. the submit took 50-188 ms but the work itself was effectively free. The tokio blocking pool itself was at 1.5% utilization (15 of 1024 threads alive); the bottleneck was submission, not service. Hot path SHA-256 (admit-side per-chunk verify, commit-side end-to-end verify) is one of the major contributors fanning into the same submission pipeline. This commit moves SHA-256 off `tokio::task::spawn_blocking` onto a new dedicated rayon ThreadPool (`cpu_pool::cpu_pool()`). The rayon pool has its own work-stealing deque, so chunked-SHA submits no longer cross- traffic with the rest of the codebase's `spawn_blocking` calls. The pool is sized at `max(2, num_cpus / 2)` — half the CPU budget so tokio worker threads retain headroom; floor of 2 so single-CPU VMs don't serialize. The pool is SEPARATE from `nativelink_util::rayon_pool::init_rayon_pool` (which configures rayon's GLOBAL pool with a tokio-handle bridge for the blake3 mmap path). Sharing one pool would let bursty per-chunk SHA starve blake3's full-blob digest computation; the partition isolates the two workloads. Mechanism honesty (per assumption-auditor + user mechanism challenge): the doc-comment now describes the bottleneck as the SUBMISSION PIPELINE (mutex + parker futex wake + scheduler context-switch + cold-cache mutex re-acquire), not as a "single-mutex hot-spot". A parking_lot:: Mutex held for ~1 microsecond cannot bottleneck at 986 ops/sec — that's ~0.1% utilization. The 50-188 ms dispatch time is consistent with the combined wake/schedule pipeline; the audit explicitly marks the precise mechanism as "UNFALSIFIED until production deploy". This commit removes per-chunk SHA from that pipeline regardless of which submission step dominates. Production deploy IS the falsification step: if back_edge_ms p99 falls toward the ~50 ms threshold, SHA was the cross-traffic; if unchanged, the bottleneck is elsewhere (most likely worker-side starvation downstream of the closure entry). panic_handler installed on both the primary and the 2-thread fallback ThreadPoolBuilder. Without it, rayon's default policy is AbortIfPanic — a single SHA-256 panic in the hot path would take down the entire NativeLink server. Mirrors `rayon_pool::init_rayon_pool`'s handler at rayon_pool.rs:92-102 (canonical e406d00, 2026-04-22, after the worker-fleet abort cascade on 2026-04-19/20). The matching `tokio::sync::oneshot::Sender` drops on unwind, the awaiting task's Receiver sees `RecvError`, which the call site already maps to `Code::Internal`. Behavior changes: - nativelink-util/src/cpu_pool.rs (NEW): dedicated rayon ThreadPool; panic_handler logs and swallows panics so worker drops cleanly. - nativelink-util/tests/cpu_pool_test.rs (NEW): basic spawn + result + panic propagation coverage. - nativelink-store/src/chunked/chunked_driver.rs: SHA-256 of finalized blob moved from spawn_blocking to cpu_pool().spawn. - nativelink-service/src/chunked_write_handler.rs: per-chunk SHA-256 verify moved from spawn_blocking to cpu_pool().spawn. Composite invariant: chunked_in_flight_digests RAII guard lifecycle (TraceMachina#401, TraceMachina#402) UNTOUCHED. Pin/sidecar ordering UNTOUCHED. Mirror durability (≥2-replica + BIS ack) UNTOUCHED. SHA is verify-path, not write-path; the spawn vehicle changed, not the work. This is Option 1 of a 3-option investigation (Options 2 io_uring and 3 opportunistic batching DROPPED per audit gating discipline, which explicitly required a single-fix-then-observe rollout before adding parallel dispatch paths — Option 2 in particular structurally destroys the closure_to_resume_us discriminator probe the audit demanded). Reviewer attribution: - code-reviewer R1: APPROVE on commit, 8 tactical suggestions noted - distributed-systems R1+R2: BLOCK-1 (panic_handler) → CLOSED, READY-TO-MERGE - perf-optimizer R1+R2: M1 (panic_handler) → APPROVE - code-simplifier R1: MAJOR-1 (Chesterton's-fence on rayon_pool) → addressed - testing-czar R1: BLOCK on Option 2 → resolved by dropping Option 2 - red-team R1: drop Options 2+3 → acted upon - rust-crate-reviewer R1 NIT (core::sync::OnceLock) → REJECTED (verified via cargo check E0432: OnceLock is std-only in stable Rust) - assumption-auditor R1+R2: FIX-FIRST (fabricated dispatch_ms numbers in doc-comment) → struck; doc-comment now matches the audit's actual measurements ("50 representative warns parsed", "dispatch_ms=50-188", closure_to_resume_ms is a demanded-not-collected probe) 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
…eplacement) Per user direction: timeouts paper over problems. This commit removes two whole-RPC wall-clock kills on the bytestream Write RPC. No replacement timer is added — production-observed failure modes are covered by existing keepalive mechanisms. Removed: - WRITE_TIMEOUT 300s (writer-side wall-clock kill) - COALESCE_TIMEOUT 300s (coalesced-waiter wall-clock kill) No-progress detection (sufficient per production evidence + reconciles with fa6cdf43 2026-04-23 commit's premise that transport keepalive IS sufficient): - h2 keepalive 30s/20s (configured in prod-server.json5) - TCP keepalive (OS-level, enabled at src/bin/nativelink.rs) - QUIC keepalive 5s (configured) - Sender-drop on connection close → buf_channel observer - InFlightWritesGuard RAII (TraceMachina#402) cleans up on grpc-future-drop Why no per-recv-chunk timer: red-team R2 review noted that no production incident shows the "app-stuck-but-TCP-alive client" shape that such a timer would catch. Today's wedges (TSAN, build-failure-1438, fl_cli_dev-slow-link) all have shapes that h2/QUIC keepalive + sender-drop already handle. Per CLAUDE.md "Falsify before fixing": if the hypothetical failure surfaces, instrument-first then add a timer with real evidence. Evidence for removal: 2026-05-14 TSAN RCA documented 244 WRITE_TIMEOUT firings on 122 distinct digests (~15 GB) over 24 min; pipeline 1463 stall investigation observed 2 more firings on ci-mac-2 at 09:58:57 + 10:03:33 PDT. All on slow-but-progressing Tailscale CGNAT clients — kill was unjustified. Adds paused-time test asserting slow-but-progressing producer succeeds. Mutation-verified: restoring the 300s timeout makes the test red-fail with bespoke message ("slow-but-progressing producer must NOT be killed by wall-clock timeout"). Reconciles with fa6cdf43 (2026-04-23): that commit's premise (transport keepalive sufficient) holds; the subsequent re-add (dda09ee et al) was based on a misattribution of WRITE_TIMEOUT firings. This restores the fa6cdf43 design. UNBOUNDED-OK on `in_flight_writes`: production tonic 0.14 doesn't cap h2 concurrent streams (default usize::MAX); byte-budget makes this acceptable (~64 B per entry; 10K concurrent = ~640 KB). RAII guard cleans up on cancel. Pre-existing `out_of_order_data_fails` test amended: it relied on WRITE_TIMEOUT firing at exactly 300s wall-clock (test passed in 300.01s on origin/main). With WRITE_TIMEOUT removed it would hang forever. The test now declares one fewer byte than chunk1 + chunk2 contribute, sets finish_write=true on chunk2, and asserts the server's size-validation Err fires synchronously. This exercises the same code path the original test intended (size validation rejecting overlap-derived underrun) without relying on a wall-clock kill. Co-Authored-By: Claude Opus 4.7 (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
2 participants
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.
Description
This PR renames turbo-cache to be native link as this project has expanded well beyond build caching.
Fixes # (issue)
Type of change
rename
not work as expected)
How Has This Been Tested?
Please also list any relevant details for your test configuration
Checklist
bazel test //...passes locallygit amendsee some docsThis change is