Repository navigation
Change the version of C++ used for protoc to the earliest supported. - #147
Merged
Merged
Conversation
This allows builds to continue to work on older platforms.
Contributor
|
I feel like this is a good change at the moment. We should strive to make this at least C++17 to be in line with what larger repos use, (e.g. Google and LLVM set it to 17), but since this is just used for an external dependency it hardly matters at the moment. |
aaronmondal
approved these changes
Jul 8, 2023
allada
approved these changes
Jul 8, 2023
allada
left a comment
allada
left a comment
Contributor
There was a problem hiding this comment.
Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @chrisstaite-menlo)
rejuvenile
added a commit
to rejuvenile/nativelink
that referenced
this pull request
Aug 19, 2026
…k GOAWAY-stuck pool
## The wedge
Production blobs reported "not found in any worker" because pooled h2
`Channel`s with stale post-`GOAWAY` state were being silently reused.
The originating worker emitted
`GOAWAY(b"too_many_internal_resets", ENHANCE_YOUR_CALM, Library)`
(fleet-wide pattern: artist, citizen, isotope, julius, garlic, domino,
forget). The h2 client `Channel` retained the dead connection in its
internal `Buffer`/`Reconnect` layers; a subsequent `bs_client(channel)
.read()` call failed synchronously on attempt 1 with
`Code::Internal "Tried to send while stream is closed"`, 0 bytes written.
`worker_proxy_store::try_read_from_worker` saw `Code::Internal` (not
`Code::NotFound`/`DataLoss`), did NOT evict the locality entry, fell
through to the next peer — but its pool was independently affected by
the same fleet-wide GOAWAY storm. After exhausting peers, the server
returned "not found in any worker" → Bazel got `FAILED_PRECONDITION` →
build failed.
The same pool wedges slow-store writes (worker→server): 8-min retry
delays observed for the same digest's mirror write.
## Why the existing recovery didn't fire
`connection_manager::ResponseFuture::poll` already evicts the channel
when the *request future itself* returns `Err` (non-streaming RPCs).
But for streaming RPCs (ByteStream `Read`), errors surface inside the
response stream items (`Some(Err(status))`), not in the request future.
The `Connection` is dropped at the end of `read_internal`'s acquisition
statement — long before the streaming body errors out — and `Drop`
silently re-pools the dead channel as healthy. The next caller pulls
the same dead clone and fails identically.
## The fix
Two complementary mechanisms:
1. **Drop-side eviction** — `Connection::notify_transport_error()` sets
a flag. `Connection::Drop` checks the flag: if set, sends `Error` +
`Dropped(None)` to the worker (evicts channel + queues reconnect +
releases concurrency slot) instead of `Dropped(Some(channel))`.
Use this when the call site holds the `Connection` past the failure
moment (currently unused — exists for future call sites that want
to keep the lease alive across the streaming body).
2. **Pool-side coarse eviction** —
`ConnectionManager::evict_idle_channel(reason)` removes any one
currently-idle channel from `available_channels` and queues a
reconnect for that endpoint slot. Called from
`GrpcStore::evict_pool_on_transport_err` whenever a transport-shaped
error (`Code::Internal | Unavailable | Unknown`, classified by
`looks_like_dead_channel`) bubbles up from a streaming retry path.
Repeated calls (one per failed retry) progressively replace every
dead clone in the pool without flushing healthy ones.
Wired into:
- `get_part_single_stream` retry loop: both `read_internal` setup
errors AND streaming-body `Some(Err(status))` errors.
- `get_part_parallel` per-chunk retry loop: same two sites.
- `write` retry loop: belt-and-suspenders alongside the existing
auto-eviction in `ResponseFuture::poll`.
## Dual-direction coverage
Confirmed: `GrpcStore` is the same code on both sides — workers use it
as the slow-store backend (worker→server) and the server uses it as
peer-fetch transport (server→worker, via `WorkerProxyStore`). Both
construct `Transport::Tcp(ConnectionManager::new(...))` in
`GrpcStore::new`, so the fix applies symmetrically without
direction-specific code.
## Tests
Two unit tests in `connection_manager` (real local TCP listener, count
distinct accepts before/after the operation):
* `evict_idle_channel_triggers_reconnect_against_real_endpoint` —
asserts that calling `evict_idle_channel` triggers exactly one
additional TCP `accept()` (the reconnect dial) within ~4s.
Mutation step: comment out the `EvictIdle` arm body — test fails
with the explicit "accept_count is still N (baseline N)" panic.
* `notify_transport_error_evicts_channel_on_drop` — acquires a
Connection, calls `notify_transport_error`, drops it, asserts that
a fresh TCP accept happens within ~4s. Mutation step: comment out
the `if self.transport_error` branch in `Drop` — test fails
identically.
Wrapped under explicit deadlines (3s for initial dial, 4s for
reconnect) so a regression manifests as a clear timeout, not a hang.
All 5 connection_manager tests pass; existing 4 grpc_store_test
integration tests still 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
Extends the base h2-stale-channel fix (255cfc09) with reviewer-driven hardening and the producer-side mitigation surfaced by the GOAWAY root-cause investigator. ## Connection-manager (consumer-side) hardening * **Removed dead `notify_transport_error` Drop-side machinery.** Zero callers existed; production wedge is unreachable from the leased- connection path because `read_internal` drops the Connection at the end of its acquisition statement (long before the streaming body errs). Pool-side eviction with per-endpoint targeting + dedup is the working path. * **Per-endpoint targeting.** `EvictIdle` now carries `endpoint_uri: Option<String>`. With multi-endpoint pools, an evict for endpoint A no longer pops an idle channel for endpoint B (`evict_idle_targets_specified_endpoint_only` test). * **Per-endpoint dedup gate.** Concurrent EvictIdle requests within `EVICT_DEDUP_WINDOW = 500ms` are absorbed; a 32-way GOAWAY storm produces 1 reconnect, not 32 (`evict_idle_storm_dedups_to_single_eviction` test, validates with mutation). * **Tightened `looks_like_dead_channel` predicate.** Added `Code::ResourceExhausted` (defensive cover for ENHANCE_YOUR_CALM if it ever surfaces directly). Gated `Code::Internal` on a message- content check (`"Tried to send while stream is closed"` plus a small set of h2-shaped variants) so server-app `make_err!(Internal,...)` no longer evicts healthy channels. Production message verified against buildcache journal 2026-04-25. ## Sibling-bug fix * `running_actions_manager.rs:319` (GetTree streaming consumption) now calls `grpc_store.evict_pool_on_transport_err` on streaming-body status errors. Same pattern as `get_part_single_stream`. Made `evict_pool_on_transport_err` `pub` so external streaming consumers can apply the recovery. ## Producer-side GOAWAY fix (worker_proxy_store + config) * **`parallel_chunk_count` default 64 → 16.** Each blob ≥ 8MiB opens N concurrent streams against the same peer; race-mode aborts emit RST_STREAM per stream. 64 × multiple workers → >1024 RST/sec trips hyper's `max_local_error_reset_streams = 1024` budget and produces `GOAWAY(too_many_internal_resets, ENHANCE_YOUR_CALM, Remote)`. 16 keeps useful parallelism (4× over single-stream) while shrinking the abort multiplier 4×. * **Cooperative loser cancellation.** New `WorkerProxyStore::cancel_loser_racer` drops the loser's `rx` first (causing the producer's next `tx.send` to fail cleanly), then `abort()`s only if the task doesn't exit within a 50ms grace window. Replaces all 4 `peer_handle.abort()` / `server_handle.abort()` call sites in the race-mode `tokio::select!`. Server-streaming Read RPCs still emit RST_STREAM on stream-drop (no END_STREAM send-path on one-way receive), but the cooperative path produces cleaner drops from the producer's natural exit instead of mid-poll cancellation, which tonic/hyper handles better. ## Comment trim Net ~84 fewer comment lines across `connection_manager.rs` and `grpc_store.rs`. Removed: 14-line `EvictIdle` essay, 17-line `evict_idle_channel` rationale, 27-line `looks_like_dead_channel` exclusion list, 6-line `connection_tx` field comment, 5 inline callsite comments collapsed to 1 line each. Doc comments retained for public APIs and non-obvious behavior (per CLAUDE.md). ## Tests * `looks_like_dead_channel_classifies_codes` — table-driven over all gRPC codes plus message variants. Mutation verified. * `evict_idle_storm_dedups_to_single_eviction` — 32 concurrent EvictIdle on a 4-channel pool. Mutation (remove dedup gate) produces 2+ extra accepts; with dedup, exactly 1. * `evict_idle_targets_specified_endpoint_only` — 2 endpoints, evict only B. Mutation (ignore endpoint_uri filter) fails the test (B doesn't reconnect because A was popped). * Existing 4 grpc_store integration tests + 6 connection_manager tests still pass. Existing 45 worker_proxy_store unit tests still 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
- stores.rs:1400 doc comment "Default: 64" → "Default: 16" with TraceMachina#147 rationale (h2 RST_STREAM budget under race-loser drops) - grpc_store.rs:150 message regex "connection error" → "h2 connection error" so MongoDB/peer-app Internal errors carrying "connection error: ..." don't false-positive evict the local h2 channel pool 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
… actual blob bytes (Bug B) Production wedge mechanism (audit .claude/reviews/wedge-mechanism-mirror-blobs/audit.md, 2026-04-25): server-side `GrpcStore::get_part` for digests f479989b...-183 and ddb73a...-183 was called with `length: Some(10_485_760)` (10 MiB — the bytestream `read_limit` forwarded from a Bazel client RPC). The pre-fix `effective_length = length.unwrap_or(...)` took that value at face value, exceeded the 8 MiB `parallel_chunk_read_threshold`, and routed into `get_part_parallel` with `chunk_count=8`. The math split the 183-byte blob into 8 ~1.25 MiB sub-ranges, the worker correctly served chunk 0's 183 bytes + clean Status::OK trailer and EOFed chunks 1-7 (offsets past blob size). The post-7dc4f0ec `CleanShort` classifier folds those EOF chunks back into Ok, so the data DOES arrive — but the splitter still wastes 7 RPCs per read of every tiny blob, and under concurrent contention the parallel path produces the "Tried to send while stream is closed" / "in GrpcStore::get_part_parallel write" wedge shape observed in production logs (TraceMachina#147 producer-side h2 RST_STREAM bursts amplified by race-loser JoinHandle::abort()). Two-layer fix: 1. Call site (`grpc_store.rs:get_part`): clamp `effective_length` to `digest.size_bytes() - offset`. `length` is documented as "at most N bytes from this offset" — clamping to the actual remaining bytes is semantically correct, mirrors the existing pattern in `get_action_result_as_part` (`length.unwrap_or(default_len).min( default_len)`), and naturally keeps tiny blobs on the single-stream path. 2. Defense in depth inside `get_part_parallel`: take a `blob_size` parameter and clamp `chunk_count` per the audit-fix formula — `chunk_count.min(blob_remaining.div_ceil(threshold).max(1))`. A blob smaller than the parallel threshold collapses to chunk_count=1; a blob between 8 MiB and 16 MiB gets 2 chunks regardless of `parallel_chunk_count=16`. This protects the parallel splitter against any future caller that reaches it directly (tests, or a hypothetical caller bypassing `get_part`) without re-introducing the wasted-RPC pattern. Test changes (same commit so the test exercises the production scenario): - `TinyBlobByteStream` now honors `read_limit` per REAPI semantics (was returning the whole tail for any offset). - Test config switched to production defaults (`parallel_chunk_read_threshold = 8 MiB`, `parallel_chunk_count = 8`) so the pre-fix code path enters parallel iff `length` exceeds 8 MiB — exactly the production wedge shape. Verification: - Pre-fix run: `chunk_count=8 requested_chunk_count=8`, test FAILS on `expected ≤2 ReadRequest RPCs ... got 8`. - Post-fix run: `parallel chunked` log absent, single-stream path used, test PASSES. - Mutation step (per CLAUDE.md): comment out either clamp individually, test goes red on the same assertion. Documented in the test docstring. Sibling-bug audit (per CLAUDE.md): - `grpc_store.rs:get_part_parallel` — only chunk-splitter in the codebase; fixed in this commit. - `batch_read_blobs` — forwards the whole request unsplit; not affected. - `memory_store.rs:get_part` — already clamps via `length.unwrap_or(default_len).min(default_len)`. - `filesystem_store.rs:read_file_entry_bytes` — naturally clamped by file size on read. - `compression_store.rs`, `dedup_store.rs`, `redis_store.rs` use `length.unwrap_or(MAX)` without an upstream clamp, but they iterate over actual stored bytes — not RPC splits — so wasted-RPC class doesn't apply. No sibling fix needed. 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
…ket B)
`looks_like_dead_channel` already covered the production wedge signature
("Tried to send while stream is closed") and "h2 connection error", but
hyper / tonic frequently surface the post-GOAWAY shape as the SHORT
form `connection error: the server sent GOAWAY` — without the "h2 "
prefix. The classifier therefore failed to mark the channel as dead
and the caller would not retry on a fresh connection (TraceMachina#147 false-
negative reproducer in the existing
`looks_like_dead_channel_classifies_codes` test).
Fix: add a `m.contains("connection error")` matcher inside the
`Code::Internal` arm. The existing app-level guard (`"verify size
mismatch in CAS upload"`) does not contain that substring and remains
correctly classified as NOT a dead channel. Sibling-bug audit: no other
sites in the workspace duplicate this classifier (`worker_proxy_store`
has its own `is_connection_error` for socket-level codes, scoped
differently); no other `make_err!(Code::Internal, "...connection
error...")` callsites inside our trust boundary that the broadened
match could mis-classify.
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
…ation for prod h2 channel-reuse trace
Adds info!/warn! logs to disambiguate the four hypotheses for the
production wedge (109k events of "Tried to send while stream is closed"
since restart with no eviction-side logs):
(a) evict_pool_on_transport_err is never called on the failure path
(b) evict IS called but the predicate doesn't match / channel pool
returns the same stale clone
(c) the err is wrapped between buf_channel send-error and the predicate
(d) retrier classification is independent of looks_like_dead_channel
New trace points:
- ConnectionManager EvictIdle: dedup-skip and no-victim paths promoted
from debug! to info! so we observe whether eviction requests are
swallowed.
- Connection::channel_id_for_log() public accessor exposes the
(endpoint_idx, connection_idx) pair.
- read_internal: log channel-id for every TCP/Dual-TCP acquire keyed
on resource_name (join key with downstream get_part logs).
- evict_pool_on_transport_err: log every entry including no-op path,
with predicate result and msg-head — directly tests hypothesis (c).
- get_part_single_stream writer.send failure: log code+msg-head+
looks_like_dead+attempt — disambiguates writer-side termination
(downstream reader dropped) from upstream channel staleness.
Volume estimate: ~10 errors/sec * (1 acquire + 1 fail = 2 lines), so
~20 lines/sec at peak — within "cheap enough to ship permanently"
budget. All info!/warn!, no error!, no allocation in tight loops
beyond what tracing fields already do. Existing unit tests all pass
(looks_like_dead_channel_classifies_codes + 6 connection_manager
tests).
rejuvenile
added a commit
to rejuvenile/nativelink
that referenced
this pull request
Aug 19, 2026
… constructor + helpers Adds the wire-stable discriminator detail message that distinguishes TraceMachina#212 Q8 backpressure from genuine dead-h2-channel ResourceExhausted. Without this, every per-blob mpsc-full event would evict the underlying h2 channel and reproduce the production TraceMachina#147 stale-channel-reuse trace (commit 7b157d6). - worker_api.proto: new `BackpressureSignal { Reason reason; uint64 retry_after_ms }` message in the existing com.github.trace_machina.nativelink.remote_execution package. Reasons: REASON_UNSPECIFIED, GLOBAL_CHUNK_BUDGET_EXHAUSTED, PER_BLOB_MPSC_FULL. Wire-stable per design §3. - genproto/com.github.....pb.rs: hand-written prost-equivalent Rust for the new message (codegen is Bazel-driven; this matches the existing convention of committing generated .pb.rs files). Centralizes the type_url as `BACKPRESSURE_SIGNAL_TYPE_URL` so producer + consumer can never drift. - nativelink-error: `Error::resource_exhausted_backpressure(msg, any)` mirroring the existing `not_found_with_detail` pattern. Same decoupling rationale (no nativelink-util dep cycle). - nativelink-store/chunked_signal.rs (NOT feature-gated; both peers must be able to recognise the discriminator regardless of whether THIS process emits it): * `encode_backpressure_signal_any(reason, retry_after_ms)` — Phase 2 producer helper. * `error_has_backpressure_signal(&err)` — receiver-side classifier used by the Phase 1 `looks_like_dead_channel` tightening (next commit). Tests cover encode-decode roundtrip + signal-present / -absent / other-detail-only scenarios. - nativelink-store: prost-types added as a direct dep so the helper compiles without leaning on a transitive re-export. 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
…ssureSignal The historic classifier (TraceMachina#147) treated any `Code::ResourceExhausted` as a dead h2 channel and triggered eviction. With TraceMachina#212 Q8 admission about to start emitting tagged `Code::ResourceExhausted` for honest backpressure (per-blob mpsc full or global byte budget exhausted), that classifier would tear down the underlying h2 channel at backpressure rate — replaying the production TraceMachina#147 stale-channel-reuse trace whenever the global budget saturates. Fix per design §13.1.1 point 2: the `Code::ResourceExhausted` arm becomes "dead channel iff the error does NOT carry a `BackpressureSignal` detail." Bare ResourceExhausted (the legacy h2 ENHANCE_YOUR_CALM shape) continues to evict; tagged ResourceExhausted does not. This is a Phase-1 prerequisite — the tightening MUST land before any Phase 2 admission code emits the tagged status, otherwise the first backpressure event takes down the connection. Test extends the existing `looks_like_dead_channel_classifies_codes` with a sibling `..._respects_backpressure_signal` covering both reasons (GlobalChunkBudgetExhausted, PerBlobMpscFull) plus the absence-of-discriminator preservation case. 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
…ort_err for chunked path Per perf-optimizer BLOCK B2 (TraceMachina#212 Phase 2.4 worker WriteChunked client): the legacy ByteStream write/get_part paths call evict_pool_on_transport_err on transport-shaped failures (the TraceMachina#147 fix that prevents reuse of GOAWAY'd channels). The chunked-write path skipped this entirely — a GOAWAY-shaped failure would leave the dead channel sitting in ConnectionManager's pool for the next request to pick up and immediately re-fail. Wire the call into update_via_chunked_inner. The looks_like_dead_channel classifier (used inside evict_pool_on_transport_err) is the SAME one the legacy paths use and explicitly excludes ResourceExhausted+BackpressureSignal — so an honest backpressure rejection from the chunked-write handler (Q8) does NOT trigger eviction. Adds chunked_grpc_eviction_test integration test: bind an in-process WorkerApi server that returns Code::Unavailable, build a GrpcStore against it with chunked-writes enabled, push a CHUNK_SIZE blob, assert the TraceMachina#147 trace fires with predicate_matched=true. Adds chunked_size_cap_test for the B1 part 1 cap.
rejuvenile
added a commit
to rejuvenile/nativelink
that referenced
this pull request
Aug 19, 2026
…acity from MemoryStore Phase 2.6 extends Phase 1's wire-stable BackpressureSignal contract to a second producer site (in addition to the chunked admission + WriteChunked handler emission landed in Phase 2.2/2.3). When a MemoryStore write would force eviction of a recently-inserted blob to make room, the operator can now opt the store into emitting Code::ResourceExhausted with a MemoryStoreAtCapacity discriminator detail INSTEAD of the historic silent-evict behavior. The existing looks_like_dead_channel classifier in grpc_store.rs:159 already respects the discriminator (Phase 1 wire-up), so no client-side change is required for this signal to be classified as transient backpressure rather than a dead h2 channel. Architectural-change discipline: changing MemoryStore semantics from silent-evict to backpressure-error IS architectural per CLAUDE.md. The change is gated behind BOTH the chunked_fast_slow feature flag AND a default-OFF runtime kill-switch (set_emit_backpressure_for_test). The historic behavior is preserved bit-identically until an operator explicitly toggles the gate, so this commit ships the mechanism without committing operators to the new semantics. A follow-up tracker will wire JSON-config plumbing once the user signs off on the production rollout shape. Implementation: - nativelink-util/src/moka_evicting_map.rs: new would_exceed_capacity(incoming_bytes) predicate. Best-effort snapshot of moka's eventually-consistent weighted_size; biases toward emitting backpressure earlier (never silently overruns). - nativelink-store/src/memory_store.rs: AtomicBool kill-switch + check_backpressure_gate() shared by update() and update_oneshot(). No-op compile-time stub when chunked_fast_slow is OFF. - nativelink-proto/...: new MEMORY_STORE_AT_CAPACITY = 3 enum value (additive — existing 1 and 2 unchanged) committed in the prior Phase 2.6 commit. Asymmetric-contract test coverage (CLAUDE.md mandatory practice for state-mutating side effects on borrowed values): - Under-action: emits ResourceExhausted+signal when kill-switch ON + at capacity. - Over-action: NEVER fires when kill-switch OFF (production default); silent-evict path preserved. - Production composition: VerifyStore(MemoryStore) propagates the detail through the wrapper without swallowing it. - Classifier round-trip: producer-emitted error is recognized by error_has_backpressure_signal so looks_like_dead_channel correctly treats it as backpressure (NOT a dead channel — replaying TraceMachina#147 would be catastrophic at backpressure rate). All four tests run under tokio::time::timeout(5s) deadlock detector with specific assertion messages naming the contract violation. Mutation step verified: forcing the gate condition to false makes 3 of 4 tests fail (the OFF/preserves-silent-evict test correctly remains green because its assertion IS the silent-evict path).
rejuvenile
added a commit
to rejuvenile/nativelink
that referenced
this pull request
Aug 19, 2026
…S4 partial-state + S5 digest-mismatch
Six new tests covering the contract surfaces the reviewer findings
called out:
**B1 cap (in `bazel_facing_internal_chunking_test.rs`):**
`pin_budget_cap_rejects_admission_with_pinned_bytes_exhausted_signal`
— a PinBudget with cap = 2*CHUNK rejects the 3rd chunk's admission
with `Code::ResourceExhausted` + `BackpressureSignal{ reason:
PinnedBytesExhausted }`. Mutation-tested: `if false` around the
pin-budget acquire → red-fail with the specific anti-TraceMachina#203 message.
**S3 classifier (in `chunked_signal.rs`):**
`new_fixup_variants_roundtrip_and_classify` — both new variants
(`MemoryStoreAtCapacity` from Phase 2.6 + `PinnedBytesExhausted` from
B1) round-trip through proto encode/decode AND are detected by
`error_has_backpressure_signal` so `looks_like_dead_channel` (in
grpc_store) classifies them correctly. Updated the existing
`looks_like_dead_channel_respects_backpressure_signal` test to also
exercise both new variants. Mutation-tested: type_url comparison set
to `false` → red-fail with the specific TraceMachina#147 regression-risk message.
**S4 partial-state (in `bazel_facing_internal_chunking_test.rs`):**
`fast_tier_ok_dispatch_err_records_failed_slow_write` — when chunked
dispatch admission fails (PinBudget=0) but fast-tier write succeeds:
the update returns Err, the fast-tier replica remains visible
(≥2-replica invariant during the recovery window), AND the digest is
in `failed_slow_writes` for retry-on-reconnect. Mutation-tested:
removing the `failed_slow_writes.insert` in
`update_via_chunked_dispatcher`'s err branch → red-fail with the
specific recovery-hook message.
**S5 digest-mismatch on async-commit (in `bazel_facing_internal_chunking_test.rs`):**
`async_commit_digest_mismatch_blocks_canonical_cas_landing` — when
the bytes streamed into the chunked driver hash to a DIFFERENT digest
than declared, the (β) async-commit returns Ok at admission, then the
driver's e2e SHA-256 verify rejects the commit. The canonical CAS
file MUST NOT appear on disk. Mutation-tested: bypassing the e2e
SHA-256 check → red-fail with the specific message about lying bytes
landing.
**Plus accessor additions:**
- `ChunkedReadRegistry::pin_hits_total/pin_misses_total/pin_partial_misses_total`
(test introspection of the metric counters).
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
…romote 1 to warn! `fast_slow_store` was the dominant log source in today's 10-min anomaly sweep (14,486 lines / 10 min, ~24/sec — single-handedly larger than every other crate combined). Each `populate run_producer` invocation emitted 12+ per-call breadcrumbs at info! level (head_result Ok/Err, data_stream entry/Ok/Err, slow_store.get entry/Ok/Err, fast_store.update entry/Ok/Err, join3 returned, send_eof/send_error, run_producer exit Ok/Err). With ~1,440 populate calls per 10 min in steady state, this alone accounted for ~17K lines of the volume — dwarfing the 6 sites TraceMachina#262 already demoted in the same file. Same shape as TraceMachina#262: per-call info! breadcrumbs from TraceMachina#147-trace work that are no longer load-bearing in steady-state and compound under burst. Same risk profile: log volume × mimalloc retention contributes to RSS growth (TraceMachina#255). Demote (a) — happy-path per-call breadcrumbs that fire once per populate: - line 1921 populate head_result Ok (info → debug) - line 1927 populate head_result Err (info → debug; NotFound is normal) - line 1980 populate data_stream branch entry (info → debug) - line 2039 populate data_stream branch Ok (info → debug) - line 2047 populate data_stream branch Err (info → debug) - line 2075 populate slow_store.get branch entry (info → debug) - line 2082 populate slow_store.get branch Ok (info → debug) - line 2087 populate slow_store.get branch Err (info → debug) - line 2101 populate fast_store.update branch entry (info → debug) - line 2111 populate fast_store.update branch Ok (info → debug) - line 2116 populate fast_store.update branch Err (info → debug) - line 2130 populate join3 returned (info → debug) - line 2175 populate calling streaming_writer.send_eof (info → debug) - line 2188 populate calling streaming_writer.send_error (info → debug) - line 2199 populate run_producer exit Ok (info → debug) - line 3508 update_oneshot: background slow write spawned (info → debug) - line 3590 update_oneshot: background slow write complete (info → debug) Promote (c) — populate failure summary, anomalous condition, replaces the 5 intermediate per-step Err lines so operators get exactly ONE warn! per failed populate (not 5): - line 2204 populate run_producer exit failed — slow-store populate did not deliver the blob to the fast tier (info → warn!) Keep (b) — state transitions and once-per-major-event lines stay info!: - line 834 chunked admission complete (transfer with throughput) - line 1187 shutdown flush enumerated - line 1313 shutdown flush completed (transfer with throughput) - lines 1488/1497 chunked-read cascade ENABLED/DISABLED (kill-switch state) Anomaly observability is preserved by: - the Err-summary warn! at line 2204 (covers ALL populate failures) - the existing PHANTOM BLOB warn! at line 2159 (has-then-NotFound race) - the existing background-slow-write watchdog warn! at line 3542 - the existing background-slow-write FAILED error! at line 3604 - all pre-existing warn!/error! sites untouched No tests assert on any of the touched log strings (verified with `grep -rn logs_contain nativelink-store/tests/`); no test breakage. The file_gets_cleans_up_on_cache_eviction test in filesystem_store_test.rs hangs on origin/main without these changes — pre-existing flakiness, not caused by this diff. 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
…in_flight (1/64) Invariant violated: two high-volume INFO log sites fire on every read and every chunked blob admission (~302/10m and ~708/10m respectively), increasing log overhead without providing signal beyond what the closed TraceMachina#147 investigation needed and without a volume bound on the durability path log. Mechanism that violates it: (a) grpc_store.rs:read_internal emitted "channel acquired (TraceMachina#147 trace)" at info! on every read — 4 arms (TCP / QUIC / Dual-TCP / Dual-QUIC); TraceMachina#147 was fixed in 5adf34a and f47a7bd, so this is leftover instrumentation with no open follow-up. (b) chunked_write_handler.rs:bazel_facing_v1_dispatcher reaper inline code emitted "chunked_in_flight removed" (outcome=reaper_decrement) at info! on every chunked blob admission — the other two paired sites (registered + Drop-removal) were already demoted to debug! by the dafcf8b sweep and compile out in prod (release_max_level_info); this site was added post-sweep and missed it. Mechanism that re-establishes it after the fix: (a) remove all 4 read_internal info! calls and the sole-purpose resource_for_log local binding; ep_idx/conn_idx/post_conn are retained (used in map_err and return tuples). (b) gate the reaper_decrement info! behind chunked_inflight_log_sampled(&dig): a deterministic pure function that folds the first 8 bytes of the digest packed hash into a u64 key and tests key % 64 == 0 — same digest always yields the same bool so the (already-debug) registered and removed lines remain paired if the other sites are ever restored; volume drops from ~708/10m to ~11/10m. Composite invariants this fix interacts with: - FL-688 durability path: the "chunked_in_flight removed" signal distinguishes writer paths (TraceMachina#247/TraceMachina#477 DS disambiguation). Preserved at 1/64 sampling — enough for journal diagnosis, not per-blob. - release_max_level_info compile-out: debug! sites compile out; the remaining info! at the reaper site now has bounded volume. - refcount bookkeeping logic: unchanged; only the info! emission is gated — the map lock, decrement, per-digest wakeup, and FSS-wide notify are unconditional. - No per-RPC timeouts on internal paths: unchanged. Test that proves the fix re-establishes the invariant: - sampling_tests::sample_period_constant_is_64: numeric-constant discipline — literal 64 at declaration site. - sampling_tests::sampling_is_deterministic_per_digest: same digest → same bool across 5 repeated calls (register/remove pair safety). - sampling_tests::sampling_rate_is_approximately_1_in_64: 6400 distinct synthetic digests → 50–150 true (expected 100); bespoke messages for mutation A (% 1 → 6400) and mutation B (false → 0). Behavior changes: - grpc_store.rs read_internal: 4 info! removed entirely (~302/10m → 0). No data-path change; ep_idx/conn_idx/post_conn retained for error-path tuple. Verified in: nativelink-store --lib 98/98 pass. - chunked_write_handler.rs reaper_decrement: info! sampled 1/64 (~708/10m → ~11/10m). Refcount bookkeeping, wakeup logic, and registry semantics: NO change. Verified in: nativelink-service --lib sampling_tests 3/3 pass + mutation A/B red-verified. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
rejuvenile
added a commit
to rejuvenile/nativelink
that referenced
this pull request
Aug 19, 2026
…onnect::Reconnect
Root cause (confirmed from prod logs + code): the worker slow-store GrpcStore's
QUIC leg (nativelink-util/src/tls_utils.rs::h3_channel) is a pool of 32 members,
each `tower::buffer::Buffer::new(h3_channel, 1024)`. `tonic_h3`'s inner
`RequestSender::poll_ready` (h3-util-0.0.5 client_conn.rs:159) re-dials on a dead
connection but returns `Err` when that reconnect's `conn.connect()` fails — e.g.
after a quinn idle/connection timeout while the server is momentarily
unreachable. A plain `tower::buffer::Buffer` PERMANENTLY poisons on the first
inner `poll_ready` `Err`: its worker stores a `ServiceError` and exits
(worker.rs:186-202), replaying "buffered service failed: {inner}" instantly to
all clones forever. There was NO rebuild path — `grpc_store.rs::
evict_pool_on_transport_err` is `Transport::Quic(_) => {}`. Prod:
instant_fail_total=35682 vs slow_fail_total=42 — ~42 genuine timeouts latched all
32 members → every single-stream QUIC read instant-failed for hours →
input-fetch fails → action fails → full re-execution.
Fix (verified against tower-0.5.3 source): wrap each pool member's H3 channel in
`tower::reconnect::Reconnect` INSIDE the `Buffer`:
`Buffer::new(Reconnect::with_connection(initial_channel, maker, ()), 1024)`.
`Reconnect::poll_ready` NEVER returns `Err` (reconnect/mod.rs:84-140): a
Connected inner `poll_ready` error loops → `State::Idle` → `make_service`
(transparent reconnect); a make error returns `Ready(Ok)` and stashes the error
for the next `call()` (per-request error, not poison). The `Buffer` poisons ONLY
on an inner `poll_ready` `Err`, so with `Reconnect` inside it the `Buffer` can
NEVER poison. A new `H3ChannelMaker: Service<()>` rebuilds one fresh H3 channel
synchronously (fresh UDP socket + quinn Endpoint) per make; `with_connection`
seeds the eagerly-built initial channel so the existing eager UDP-buffer tuning +
one-warn-per-pool diagnostic and the initial socket-bind timing are unchanged —
only RECONNECTS are lazy. The pool stays 32 independent self-healing members with
identical round-robin behavior. Transport-wide: covers BOTH the single-stream
read path and the write path (both traverse h3_channel).
The existing TraceMachina#147/#FU/TraceMachina#2-Fix-C latch DETECTION + abort machinery in grpc_store.rs
is retained (correct fast-fail); the fix just makes the latch not happen.
Invariant being violated: a transient slow-store (QUIC) readiness failure must
not permanently latch the transport — a subsequent read must succeed — so it
cannot cause unnecessary duplicate action execution or truncated streamed data.
Mechanism that violates it: tls_utils.rs h3_channel wrapping each member as a
bare `Buffer::new(h3_channel, 1024)`, which poisons permanently on the first
inner `RequestSender::poll_ready` `Err` (tower buffer/worker.rs:186-202).
Mechanism that re-establishes it after the fix: tls_utils.rs h3_channel now wraps
each member as `Buffer::new(Reconnect::with_connection(initial_channel, maker,
()), 1024)`; `Reconnect::poll_ready` (tower reconnect/mod.rs:84-140) never
surfaces an `Err` to the `Buffer`, transparently re-making a fresh H3 channel via
`H3ChannelMaker` on a Connected `poll_ready` error.
Composite invariants this fix interacts with: (1) the per-RPC-timeout policy —
unchanged (no timeouts added; liveness stays keepalive + bounded Buffer queue of
1024/member); (2) the round-robin pool selection in `QuicChannel`
(poll_ready/call pairing) — unchanged, still 32 members; (3) the
`looks_like_latched_pool` / abort-threshold fast-fail in grpc_store.rs — retained
and now rarely reached; (4) in-flight stream integrity — `RequestSender::call`
clones its `send_request`, so a reconnect in a later `poll_ready` cannot truncate
an in-flight read.
Test that proves the fix re-establishes the invariant:
tls_utils::reconnect_seam_tests::buffer_with_reconnect_self_heals_after_transient_error
(deterministic seam) and tests/quic_reconnect_selfheal_test.rs::
quic_read_self_heals_after_transient_transport_failure (real quinn/h3).
Behavior changes:
- QUIC READ path (single-stream, Transport::Quic and Transport::Dual QUIC leg): a
transient quinn timeout / failed inner reconnect → transparent Reconnect
rebuild instead of a permanent "buffered service failed" latch. Verified safe
in compositions: WorkerProxyStore → VerifyStore → ExistenceCacheStore →
SizePartitioning → cas_FAST_SLOW_STORE(MemoryStore→FilesystemStore) with the
worker slow-store GrpcStore over Dual(TCP+QUIC); read TraceMachina#1 over a live server
still succeeds identically, and post-failure reads now self-heal
(nativelink-store grpc tests green; real-transport test green).
- QUIC WRITE path (sibling poison — same h3_channel): identical transparent
reconnect instead of permanent latch. Verified safe: batch/small-RPC writes and
chunked write RPCs that route over the QUIC leg now recover from a transient
timeout rather than instant-failing until process restart. No write ordering,
ack, or durability semantics change (no fsync/sync primitives; mirror_blobs +
BIS ack unchanged; Buffer queue still capped at 1024/member).
- Pool construction: the initial 32 members are still built eagerly (same UDP
bind + tune_quic_udp_buffers + one-warn-per-pool). Only the RECONNECT path is
new/lazy. No change to non-QUIC (TCP) transport, and all new code is
`#[cfg(feature = "quic")]`-gated (non-quic build unchanged).
- New buffering fields: none on any network path (the Buffer cap stays 1024;
H3ChannelMaker holds only cloned config/uri/name + an AtomicU64 generation
counter — no owned-bytes buffers).
Reproduce-first TDD: Test A (seam) — plain Buffer permanently latches after a
transient poll_ready Err (documents the bug); Buffer+Reconnect self-heals;
mutation dropping Reconnect fails with the bespoke never-poison message. Test B
(real quinn/h3) — dead-port-first forces the exact `poll_ready` Err at the
transport seam; fixed build heals; mutation dropping Reconnect fails end-to-end
with the production signature "buffered service failed: timed out" in
GrpcStore::read (quic).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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.
This allows builds to continue to work on older platforms.
This closes #146.
This change is