fix(debug-trace-server): R2-first witness fetches and a per-hop budget cap - #182
Conversation
Instrumentation only — no control flow changes.
Two client-visible `-32001`s on ore could not be attributed. After the
generator answered "not generated yet" the request path fell silent for
7.9 of its 8s budget and then returned via `record_deadline`, which logs
nothing; `rpc_client.rs` additionally suppresses the per-provider outcome
when the deadline blew alongside the attempt. Only two things can consume
that window — waiting for a concurrency permit, or an attempt that never
answers — and both were silent, so they produce identical observations
from outside while needing opposite fixes.
Three additions close that:
- `RpcAttemptOutcome::DeadlineClamped` records the abandoned attempt with
its provider and duration. Kept distinct from `Timeout` so the original
intent survives: the attempt window was clamped to whatever budget was
left, so blaming the endpoint for a stall would still be wrong.
- `on_rpc_permit_wait` (defaulted, so implementors are unaffected) times
the semaphore acquire; the trace server exports it as
`debug_trace_upstream_permit_wait_seconds{method}`.
- Every deadline give-up logs one WARN naming the phase it died in —
`before_attempt` / `attempt_clamped` / `before_backoff` — with provider,
round, permit_wait_ms and attempt_ms.
Volume is negligible: the WARN fires exactly where
`upstream_deadline_exceeded_total` already increments, which was 2 in 3h
on the affected host.
Worth recording alongside this: the premise of the reverted #179 does not
hold. The generator's RPC server answers from two bare `exists()` syscalls
with no cache (mega-reth `block_updates/file_ops.rs:466`), while the R2
uploader reads a different file type (`FileType::Upload`, `upload.rs:319`),
so for a frontier block R2 can hold a witness the RPC server's file does
not yet have. Rotating to the gateway is not useless there — it was the
earlier source 13 times in the same window.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…t cap
Two client-visible -32001s on ore (2026-08-09) came from one shape: a
frontier block's witness was "not found" at the generator, the chain
rotated to the public gateway, and that single hop stalled for 7.9 of the
8s witness budget — so the loop never reached round 1, where the
generator (whose file appeared 1.3s in) would have served. Two causes,
two fixes:
R2 first, for every fetch. The is_historical gate assumed the bucket
lags the generator at the frontier. It does not: the generator's RPC
server answers from bare exists() calls on FileType::Witness while the
R2 uploader publishes FileType::Upload — different files of the same
run — and in a 3h window the bucket held 13 frontier witnesses the
generator was still reporting missing (the stalled gateway hop was the
only route to them). Dropping the gate reaches the same store through a
client that is bounded where the gateway hop is not: connect timeout,
capped attempts, half-budget share. Frontier probes usually miss and
cost one fast 404; hits are labeled witness_r2_frontier so their rate
is separable, and a frontier "missing" is excluded from
r2_witness_errors_total{kind="missing"}, which stays the
bucket-integrity alarm for historical holes.
A per-hop cap that actually binds. attempt_timeout was
min(per_attempt = 20s, remaining), and every witness stage budget sits
below 20s — the guard was dead code exactly where it was needed, letting
the first stalled hop legally consume the entire stage.
RpcClientConfig::witness_per_attempt_timeout (trace server: half the
witness stage, derived from --witness-timeout) restores it, and permit
waits are deadline-bounded the same way, so no hop — generator, gateway,
or permit queue — can eat the stage and leave nothing for another
rotation. Applied only under a deadline: chain-sync's deadline-less
fetches must out-wait transfers slower than the cap, or an unbounded
retry loop would re-cut a deterministically slow transfer forever.
The rescue is pinned by a differential test replaying the incident
(generator miss -> gateway stall -> generator serves on round 1): it
passes in ~0.4s with the cap and rides the full deadline to failure
without it. Historical routing is byte-identical (the cap cannot bind
after R2's half-budget share; the generator skip is unchanged).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude review status
✅ Review clean Last reviewed: New this round: 0 finding(s), 0 question(s) · Resolved this round: 1 · Open questions: 0 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 160d31e90f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
… the configured stage Addresses all three Codex review findings on the hop cap, which share one defect: the cap was half the *configured* full stage, so it stopped binding exactly in the runs that had already lost budget. The effective cap is now the tightest of three bounds at chain entry: - the configured cap (half the full witness stage, unchanged); - the global per-attempt timeout — an operator who set --rpc-per-attempt-timeout-ms below the cap asked for stalled attempts to be cut sooner, and the witness path must not quietly loosen that; - half of what the stage actually has left — the old-block clamp and a slow R2 pre-try both shrink the real budget below the static cap, and min(cap, remaining) then resolves to remaining again, reintroducing the single-hop-eats-the-stage failure this PR exists to remove. Halving happens once at entry, not per attempt, so later rounds are not geometrically starved. Deadline-less fetches and configs that leave witness_per_attempt_timeout unset (the validator) are untouched. Both regressions are pinned by differential tests: a 1s stage under a 6s cap, and a 200ms explicit per-attempt under a 10s cap — each passes with the three-bound rule and fails against min(configured, remaining). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5a09203d7a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…eadline Fourth Codex finding, and the right completion of the hop-cap rule: the sleep was the one component still allowed to consume everything left. Clamped only to `remaining`, a backoff larger than the post-round remainder sleeps up to the deadline, wakes exactly on it, and fails at the next round's entry check — forfeiting the very retry it was sleeping for, and handing back the time the hop cap just saved. Under a deadline the sleep is now `min(backoff, max(1, remaining / 2))`: the loop always wakes with at least half the pre-sleep remainder for real attempts. When budgets are comfortable the clamp never binds (production's 12s stage leaves ~5.7s after round 0 against a 500-750ms backoff); it only shortens sleeps that were provably wasted. Applies to every deadline-carrying call — sleeping into a deadline is pointless on the data path too. Pinned by a differential test (800ms stage, 600ms backoff, stalled gateway, generator serving on round 1): rides the deadline to failure against `sleep.min(remaining)`, serves at ~0.6s with the clamp. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The Codex review on #180 pointed at the saturated-semaphore case: with a bare acquire().await, a deadline-bound call parks in the permit queue until some unrelated in-flight attempt frees a permit, sailing past its own deadline with neither the WARN nor the permit-wait sample fired. The stacked 160d31e already bounds the acquire with timeout_at and records phase="permit_wait" at the give-up — but nothing pinned it. Differential test: one permit, a hog stalled inside its attempt against a hung server, a victim with a 300ms deadline. With the bound the victim fails at ~0.3s; with a bare acquire it parks until the hog's per-attempt timeout at ~4s (verified both ways). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7c77d52e89
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…g site /simplify follow-ups on the instrumentation commit, four angles, deduped: - record_deadline's six flattened parameters encoded phase-dependent context as sentinels: two of three call sites passed None + two Duration::ZEROs, and the WARN printed fabricated permit_wait_ms=0 / attempt_ms=0 on phases where nothing was measured — indistinguishable from a genuine 0ms. The context is now a GiveUpPhase enum whose AttemptClamped variant alone carries measurements, the closure takes (phase, round) and derives elapsed itself, and the WARN emits measurement fields only for the phase that has them. - The DeadlineClamped block hand-rolled the on_rpc_attempt call that the generic recording site fifteen lines below already makes; the outcome is now reclassified before that single site instead. - attempt duration was collapsed to f64 and then reconstructed with Duration::from_secs_f64 to feed the closure; the Duration is now bound once and both uses derive from it. - debug_trace_upstream_permit_wait_seconds joins the method-keyed pre-registration loop (it was the one method-keyed upstream series missing from it, and the loop's comment claimed otherwise). - The permit-wait rationale existed in four wordings across three files; the trait hook keeps the authoritative copy, the other sites point at it. - Dropped a dated-incident sentence from the shared enum doc (provenance belongs in PR bodies), and extended the label-stability test with deadline_clamped. No behavior change: same metrics, same WARN sites, same control flow. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…witness-hop-cap-and-r2-first
This comment was marked as spam.
This comment was marked as spam.
Four review angles over the delta, deduped; production logic untouched.
Test scaffold (~63 duplicated lines): the four hop-cap tests each
hand-rolled the hanging-listener trio that test_support::hanging_url()
already provides, plus a verbatim client+config block now provided by
cap_fixture (routing_fixture delegates to it; each test keeps its
RpcClientConfig knobs, budget, and asserts visible at the call site).
The pre-existing hung-R2 test also inlined what the new fixture_wire()
helper encapsulates — converted.
Moved saturated_permit_queue_fails_at_the_deadline_not_after_it to
stateless-common's own test module: it exercises RpcClient alone, and a
shared-crate invariant guarded only by a downstream crate's tests would
not travel with the crate.
Six stale "historical" references orphaned by the gate deletion,
including a validate_args comment and bail text that had become false
("can never fire in stateless mode" — it now would fire; the honest
rationale is that without a DB tip a genuine bucket hole would be
demoted to an expected frontier miss and never reach the
kind="missing" alarm). The errors-counter doc now states the frontier
exemption where operators look, letting the call-site comment shrink.
The frontier missing-check now matches the typed variant
(R2WitnessError::is_missing) instead of string-comparing the metrics
label, so the alarm exemption survives a label rename.
Trimmed the three-bound cap rationale to one authoritative home (the
config field doc) with code-local pointers, dropped two dated-incident
provenance fragments from rustdoc, and de-witnessed the sleep-clamp
comment's phrasing (the clamp is generic).
Skipped deliberately: hoisting the four "half" constants into one
shared value (they bound different remainders and compose by min —
a shared constant would create false cross-crate coupling), and
threading `frontier` into witness_route (would trivialize its
selection test while moving nothing real).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
On a saturated semaphore a deadline-bound call could spend its whole budget inside acquire().await: the deadline was only checked before the await and the permit-wait sample only recorded after a permit arrived, so the saturated-queue case — the exact shape this instrumentation exists for — stayed unobservable until an unrelated permit freed. The acquire is now bounded by the deadline (tokio::time::timeout_at). A wait cut short at the budget records its permit-wait sample (dropping it would censor the histogram exactly when the queue is at its worst) and gives up through the single record_deadline site with a new phase, permit_wait_clamped, carrying permit_wait_ms. No provider is named: nothing was contacted, the loss is queue wait behind our own cap. Also reattaches the round_robin_with_backoff doc comment, which the GiveUpPhase enum had been inserted underneath. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…d tests /simplify pass over the PR diff, with an explicit ask to keep test code and comments concise. No behavior change: per-phase log output, metric values, and every test assertion's intent are preserved. - record_deadline emits one warn! with Option-valued fields instead of three near-identical arms; tracing skips None fields, so each phase logs exactly its documented field set. - Drop the derivable attempt_duration binding; reuse the loop-top clock read as the queue-entry timestamp. - One home per rationale: the clamped-attempt story lives on RpcAttemptOutcome::DeadlineClamped (provenance clause dropped), the permit-wait story on RpcMetrics::on_rpc_permit_wait; code comments and AGENTS.md point there instead of restating. - Tests: shared metered_client + expect_deadline_give_up helpers replace the copy-pasted metrics/config/client scaffolding in four tests; the permit test reuses LOCALHOST_A and a 20ms budget. - test_metrics_record_deadline_exceeded_once no longer insists every attempt is Error: near the deadline a laggy response is legitimately clamped to Timeout/DeadlineClamped (seen flaking once under full-suite load); exact attribution stays pinned by the per-provider test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…to liquan/fix/witness-hop-cap-and-r2-first Both sides had independently bounded the semaphore acquire by the caller deadline; the conflict is resolved by unifying on the base PR's published shape: GiveUpPhase::PermitWaitClamped (phase label "permit_wait_clamped", no provider field — nothing was contacted), the collapsed single-warn! record_deadline with Option-valued fields, and the shared permit-wait sample site. This branch's PermitWait variant and inline acquire implementation are dropped as superseded; its saturation test, the witness hop cap, and the backoff half-clamp are kept unchanged. AGENTS.md keeps this branch's R2-first/frontier/three-bound lines and takes the base's phase list and permit-wait sentences. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…scale The frontier classification — which picks the witness_r2_frontier metrics label and exempts a miss from the kind="missing" bucket-integrity alarm — reused the 4096-block witness routing window. Routing asks "may the generator have pruned this?"; the miss exemption asks "may the uploader not have reached it yet?". Conflating them demoted a genuine bucket hole anywhere in the routing window (e.g. a brief uploader outage now sitting at tip-1000) to an "expected frontier miss" that never fired the alarm. The frontier band is now its own constant, R2_FRONTIER_WINDOW = 32 blocks — generous for the uploader's PUT latency plus local tip sync lag (chain sync's GENERATOR_WITNESS_GRACE is the time-based analog), 128x narrower than the routing window. Label and miss exemption move together, so probes past the band count as witness_r2 (they should hit) and their misses reach the alarm. Unknown tip still classifies as frontier: that is the cold-start transient validate_args' --data-dir requirement bounds. Boundary test pins the band against the routing window; docs (metrics counter, AGENTS.md, README) updated to define the band. Addresses mega-maxwell review 4912944112 on #182. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 63c8facb49
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Tests (behavior-preserving, -140 lines net across the PR): - The saturated-permit test is rewritten as the witness-flavored twin of test_deadline_bounds_permit_wait: hold the single witness permit directly instead of a TCP listener + spawned hog + 150ms sleep + multi-thread runtime. Runs in 20ms instead of ~450ms, asserts more (give-up count, cut-short permit-wait sample, zero attempts, typed method) and cannot flake on the sleep. - The historical and frontier R2-hit tests were line-for-line identical except the block number; merged into one test iterating both. - cap_fixture's witness_timeout_secs parameter was dead — the hop-cap tests pass explicit deadlines and never call witness_budget — dropped. - r2_frontier_band test: drop the test_ prefix (module convention) and a redundant above-tip assertion. Docs — merge leftovers still describing the pre-R2-first shape fixed in main.rs module doc, DataProvider field/constructor docs, and a metrics section comment; the witness_per_attempt_timeout doc drops trace-server specifics from the shared crate and states when the ceiling actually binds (only for embedders whose deadline can exceed the stage — the --witness-timeout help no longer implies the flag value is the operative cap); one-home-per-rationale trims for the frontier band, the R2 miss arm, the backoff clamp, and validate_args; AGENTS.md drops a permit clause owned two lines below. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
0 blocking · 0 should-fix · 3 suggestion(s) · 0 open question(s)
Reviewed e12683a8..80920299.
Details are attached inline.
Findings without inline anchors:
bin/debug-trace-server/src/r2_witness.rs:142— [Minor] R2 witness decode runs unbounded after the R2 sub-deadline, can eat into the RPC fallback budget The design offetch_witnessrelies on the R2 pre-try being capped toR2_WITNESS_BUDGET_DIVISOR's share of the stage so the RPC fallback chain always keeps its own rotation budget. If decode of a large/slow-to-decompress object (or one queued behind otherspawn_blockingwork under load) runs past the R2 sub-deadline, the caller does not fall back to the RPC chain until decode finishes, silently eating into the RPC chain's budgeted rotation time. Suggested fix: Wrap thespawn_blockingdecode intokio::time::timeout_at(deadline, ...)(or check the deadline immediately after the GET returns and skip decode if it has already passed) so a slow decode falls back to the RPC chain instead of consuming its budget.
Codex round 2 (review 4912997741), three P2s, all confirmed: 1. Stale-tip frontier exemption. is_r2_frontier treated every block above the local DB tip as frontier, so during a catch-up (stale tip) the kind="missing" bucket-integrity alarm was silenced across the whole gap. The band is now two-sided: blocks more than R2_FRONTIER_WINDOW above the tip get no exemption either — with a healthy tip they cannot resolve at all, so reaching the probe there means the tip is stale, and a stale tip says nothing about uploader lag. Fail toward the alarm. 2. Unbounded R2 decode. The GET honored the R2 sub-deadline but the spawn_blocking light decode did not, so an oversized or pathological object could eat the RPC fallback's share of the stage. The decode now runs under the same deadline (already-elapsed deadlines skip the spawn entirely); on overrun the task is abandoned and a new decode_timeout error kind falls back to the RPC chain. 3. Entry-frozen hop cap defeated by permit queueing. The tightest-of- three witness cap froze remaining/2 at chain entry, before the concurrency permit; a call that queued past half its budget then handed the first stalled hop min(cap, remaining) == everything left. The cap policy moves into the retry loop as AttemptCap::ReserveHalf: the halving is recomputed as each attempt starts — after the permit wait — with a 25ms floor below which the final attempt takes the tail whole instead of micro-slicing it. Data methods keep AttemptCap::Fixed (unchanged behavior: a slow honest single-endpoint response may use the full remainder). Each fix carries a red/green test: the two-sided band boundary, the expired-deadline decode path (the mid-decode overrun cannot be scripted deterministically; same mechanism), and a permit-starved rotation that must still reach the second provider. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…witness-hop-cap-and-r2-first # Conflicts: # crates/stateless-common/src/rpc_client.rs
vincent-k2026
left a comment
There was a problem hiding this comment.
Theme: the strongest engineering of the stack, with four genuinely differential tests — but the per-hop halving turns a class of currently-succeeding slow witness fetches into deterministic client-visible timeouts. Reproduced below.
Verified against HEAD: cargo test -p debug-trace-server --bins → 142 passed / 0 failed.
Genuinely good
stalled_hop_is_capped_so_the_next_round_can_serve,hop_cap_tracks_the_shrunken_stage_budget,deadline_pressure_shortens_the_backoff_instead_of_sleeping_into_it, andexplicit_per_attempt_timeout_stays_the_tighter_boundare all real differential tests, each with a doc comment stating precisely how it goes red without its fix.- "An explicit
--rpc-per-attempt-timeout-msmay only tighten, never loosen" (rpc_client.rs:769,cap.min(self.config.per_attempt_timeout)) is the right direction. - The two-sided
is_r2_frontierband (data_provider.rs:1121), deliberately decoupled from the 4096-block routing window, is a better fix than the original suggestion — it stops the bucket-integrity alarm being silenced across 4096 blocks. I checked the sizing independently: mega-reth's own comment puts MegaETH at a ~1s EVM-block cadence, so 32 blocks is about 32s, a sane 5x margin overGENERATOR_WITNESS_GRACE = 6s. The number holds up. decode_light_with_deadlinebringing the blocking decode under the same deadline, and honestly documenting that the abandoned task cannot be cancelled and finishes in the background.
Blocking
B1. AttemptCap::ReserveHalf converts a slow-but-honest witness transfer from success into guaranteed failure.
rpc_client.rs:1181-1186 caps every deadline-bound attempt at min(ceiling, remaining / 2), independent of provider count. Any witness whose transfer takes more than half the stage is cut, and each retry gets strictly less (1/2 -> 1/4 -> ...), so it fails deterministically — where before this PR min(per_attempt = 20s, remaining) let it complete.
The single-provider case is sharpest (one --witness-endpoint, or a historical route that skips the generator and leaves one fallback): halving buys no rotation there at all. Codex raised exactly this as "Preserve full budget for single witness providers"; the thread is marked resolved with no reply, and there is no provider-count gate in the code.
Measured differential — one healthy witness endpoint that answers a fixture witness after 600ms, inside a 1s witness stage, driven through fetch_witness:
4642efb (this PR's base): ok
HEAD: FAILED - Timeout { stage: Witness, elapsed: 1.000997398s }
Same test, green on the base, red on the head.
Ask: before merge, post debug_trace_upstream_duration_seconds{method="mega_getWitness"} p99/p999 under the deployed config (12s stage -> 6s ceiling). If p999 sits well under half a stage the risk is acceptable — put the number in the PR body. Otherwise the smallest structural fix is to apply the halving only to hops that still have an untried provider left in the round, and let the round's last hop take the remainder whole (still bounded by ceiling) — the rescue property survives, while single-provider chains and last hops stop being structurally condemned.
Should address (non-blocking, but please respond)
S1. Attempt amplification under a stall — measured on both sides. Geometric halving plus the halved backoff turns one stalled hop into many rounds. Same scenario (2 providers, 12s stage, hanging gateway + always-missing generator, production-shaped backoff 500ms/30s):
base: 1 generator attempt (1 round)
HEAD: 6 generator attempts (6 rounds)
Against a hanging endpoint that is only connections; against a degraded-but-transferring endpoint it is 6 abandoned multi-MB GETs aimed at an already-degraded gateway. Consider a hard round cap (2-3) under a deadline rather than pure geometric slicing.
S2. The frontier probe gets the same half-stage share as the historical fetch. data_provider.rs:1234 uses one R2_WITNESS_BUDGET_DIVISOR = 2 for both, but the semantics differ: historical R2 is the primary source and deserves half the budget; the frontier probe is speculative and expected to miss. R2GetError::Connect / Throttled are retryable (crates/stateless-r2/src/fetch.rs:106) up to MAX_ATTEMPTS = 3 (bin/debug-trace-server/src/r2_witness.rs:31), so with degraded R2 credentials or egress every near-tip request now burns up to half the witness stage before the RPC chain is tried — where previously frontier requests never touched R2 and were immune to its health. That is the same "one hop eats the budget" shape this PR removes from the gateway, reintroduced (bounded to half) on the R2 side. --r2-max-concurrent-requests adds the same coupling: bulk backfill can now delay frontier request serving. Suggest a much tighter frontier slice (1/8 of the stage, or a fixed ~500ms) plus a short-circuit after consecutive failures.
S3. Chain-sync lag beyond 32 blocks turns routine uploader lag into bucket-integrity alarms. The band's upper bound is tip + 32 (about 32s). During restart or catch-up the local tip is routinely further behind, so requests for genuinely frontier blocks classify as non-frontier and their missing goes straight to kind="missing". The doc comment says "fail toward the alarm" — I agree with the direction, but the cost is a deterministic false-alarm burst on every catch-up. Consider gating on tip-observation freshness, or a separate kind so it does not pollute the bucket-integrity signal.
S4. PR body no longer matches the diff:
- The body describes the cap as "half the witness stage budget (6s at the deployed 12s)" and the incident replay says "gateway stall is cut at 6s ... success at ~7s". The final implementation is the three-way
ReserveHalfmin, which halves again —AGENTS.md/README.mdwere updated, the body was not. - The body says permit waits are reported as
phase="permit_wait"; the actual label ispermit_wait_clamped. - "Acceptance in production (needs #180's fields)" cites
rpc_errors_total{reason="deadline_witness"}, which is introduced by #178, not #180 — worth stating that dependency explicitly since #178 is on a separate stack.
S5. Merge note. This is stacked on #180, and both touch bin/debug-trace-server/src/metrics.rs alongside #178 (ErrorReason grid / permit-wait histogram / witness_r2_frontier source). pre_register_all_metrics will almost certainly conflict — whoever lands second should re-run --bins after the rebase.
…utcome Review 4914046458 (vincent-k2026), both actionable findings: - RpcAttemptOutcome gains #[non_exhaustive]: it is a pub enum in a library crate and this PR itself added a variant (DeadlineClamped) — the next one must not be a breaking change for downstream exhaustive matches. No in-workspace consumer matches exhaustively today. - The deadline give-up WARN no longer fires for best-effort internal probes. round_robin_with_backoff takes the caller's give-up disposition; best_effort demotes the give-up log to debug while the deadline-exceeded metric still fires. The trace server's throttled upstream tip seed — which deliberately degrades its own failure to a debug line and skips one memoization round — switches to the new get_latest_block_number_best_effort; every client-facing path keeps the WARN. Pinned by test_best_effort_give_up_logs_quietly: a positive control proves the WARN capture works (same log_at! expansion, so the same warn! callsite), then the best-effort give-up must leave the WARN buffer empty; interest-cache rebuild loop guards the known capture flake. The third note (the loosened all-Error assertion) is acknowledged as-is: attribution stays pinned by the per-provider test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…witness-hop-cap-and-r2-first # Conflicts: # AGENTS.md # crates/stateless-common/src/rpc_client.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 69e725e382
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
… split decode out of the attempt window Review 4914046550 (vincent-k2026) B1/S1/S2/S3 plus Codex P2 (4914986082): - B1: AttemptCap::ReserveHalf halves an attempt's window only while the round still has an untried provider to rotate to. The round's last hop and every hop of a single-provider chain take the remainder whole under the ceiling: there is no rotation left to reserve for, and condemning a slow-but-honest transfer bought nothing. This was the reviewer's measured regression — a 600ms serve inside a 1s stage went from success on base to deterministic timeout — now pinned green by slow_honest_single_provider_survives_the_reserve and slow_honest_last_hop_takes_the_remainder; the incident-shaped rescue (ceiling cut + retry round) keeps its own tests, re-pinned with the stall mid-round where the reserve actually applies. - S1 falls out of B1: the stall-amplification tail collapses with the last hop no longer halved (geometric slicing only runs while rotation targets remain). - Codex P2: the retry loop gains a finalize step — transport runs under the attempt window, the witness light decode runs after it, bounded by the caller's deadline alone. CPU-bound decode no longer burns the rotation reserve nor reads as a provider stall, while a corrupt payload still records as the provider's Error and rotates (the property the existing junk-payload tests keep pinned). A decode that outruns the deadline classifies as deadline_clamped, and the abandoned blocking task finishes in the background — the same accepted trade as the R2 decode. (No dedicated red/green test: the fixed fn-pointer decoders leave no seam to inject a deterministically slow decode; the property is pinned structurally and by the rotation tests.) - S2: the frontier probe runs on an eighth of the remaining stage (R2_FRONTIER_BUDGET_DIVISOR) instead of the historical half — the probe is speculative and expected to miss, so degraded R2 can no longer burn half of every near-tip request's budget in front of the RPC chain. Pinned by frontier_probe_gets_only_the_speculative_share. - S3: is_r2_frontier grows into a three-way R2Band. A missing above the band — only reachable behind a stale catching-up tip — records on its own kind="missing_above_tip" series instead of flooding the bucket-integrity alarm on every catch-up; below-band holes keep feeding kind="missing". Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
All of review 4914046550 addressed in 6085bf3, structural fix taken over the data ask. B1: implemented exactly the suggested rule — |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6085bf3e73
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…et share Codex P2 (review 4915452108, thread 3765546116): the above-band probe — reachable behind a deliberately buffered or catching-up local tip — was budgeted like a historical fetch (half the stage), but its object is not guaranteed to exist yet. Only the historical band keeps the half share; both near-tip bands are speculative and get the eighth, so degraded R2 cannot burn half of a near-head request's budget in either shape. The probe-share test now drives both speculative bands. The sibling P2 (shortened last hop, thread 3765546128) is answered without a code change: it asks for the exact opposite of review 4914046550's blocking B1, and the bounded-regret trade is documented on AttemptCap::ReserveHalf. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…p-cap-and-r2-first # Conflicts: # AGENTS.md # crates/stateless-common/src/rpc_client.rs
Summary
Stacked on #180 (instrumentation; merge that first). Fixes the failure shape behind the two client-visible
-32001s on ore (2026-08-09): a single stalled gateway hop consuming the entire witness budget while the witness was reachable elsewhere the whole time.Root cause (two, compounding)
The
is_historicalgate on R2 was built on a false assumption. It assumed the bucket lags the generator at the frontier. The generator's RPC server answers from bareexists()syscalls onFileType::Witness(mega-rethblock_updates/file_ops.rs:466) while the R2 uploader publishesFileType::Upload(upload.rs:319) — different files of the same generation run — so the bucket can lead. In one 3h window it held 13 frontier witnesses the generator was still reporting missing, and the only route to them was the gateway hop that also produced the two stalls.The per-attempt guard was dead code on the witness path.
attempt_timeout = min(per_attempt = 20s, remaining)with every witness stage budget below 20s meansmin()always resolves toremaining: the first stalled hop may legally consume the whole stage, and the retry loop — two providers, unbounded rounds, exponential backoff, all healthy — never gets a round 1. In the incident, round 1 would have succeeded: chain-sync got the witness from the generator 1.3s in.Fix
R2_FRONTIER_WINDOW= 32 blocks of uploader-lag grace on either side of the local tip; hits there are labeledwitness_r2_frontierso the probe's hit rate is separable. Amissingclassifies by band: in-band is the expected probe-ahead outcome (excluded from the alarm, logged atdebug), below-band feedsdebug_trace_r2_witness_errors_total{kind="missing"}— the bucket-integrity alarm — and above-band (only reachable behind a stale catching-up tip) lands on its ownkind="missing_above_tip"series so catch-up windows stay visible without flooding the alarm.RpcClientConfig::witness_per_attempt_timeout— the trace server sets half the witness stage budget (derived from--witness-timeout, no new flag; 6s at the deployed 12s), applied only under a deadline: chain-sync's deadline-less fetches keep the 20s cap, because an unbounded retry loop would re-cut a deterministically-slower-than-cap transfer on every round, forever. Each deadline-bound attempt's window is the tightest of that ceiling, the global--rpc-per-attempt-timeout-ms(an explicitly stricter operator setting is honored, never loosened), and — only while the round still has an untried provider to rotate to — half of what the call still has as the attempt starts, recomputed after any permit wait. The round's last hop, and every hop of a single-provider chain, takes the remainder whole under the ceiling: rotation stays protected without structurally condemning a slow-but-honest transfer (review-measured regression, now pinned green both ways).Errorand rotates.timeout_at, mirroringstateless-r2's own pattern), reported asphase="permit_wait_clamped"through feat(rpc-client): make a deadline give-up say where the budget went #180's give-up WARN — so queue saturation can neither silently eat the budget nor park past the deadline.Why not the alternatives
Incident replay under this change
The two failed blocks were in the bucket (the gateway served their immediate neighbours), so they resolve as R2 hits in ~350ms and never reach the gateway. Even with R2 missing too: gateway stall is cut at the 6s ceiling, backoff clamped, round 1 generator serve → success at ~7s of the 12s budget.
Testing
stalled_hop_is_capped_so_the_next_round_can_servereplaying the incident (generator miss → gateway TCP-accept-never-reply → generator serves round 1): passes with the cap, rides the full deadline to failure without it (verified both ways).hop_cap_tracks_the_shrunken_stage_budget,explicit_per_attempt_timeout_stays_the_tighter_bound, anddeadline_pressure_shortens_the_backoff_instead_of_sleeping_into_itpin the other two bounds and the backoff clamp the same way.slow_honest_single_provider_survives_the_reserveandslow_honest_last_hop_takes_the_remaindergo red under unconditional halving;frontier_probe_gets_only_the_speculative_budget_sharegoes red under a shared divisor;test_reserve_half_recomputes_after_permit_waitgoes red with the cap frozen at chain entry.fetch_witness_serves_from_r2_for_historical_and_frontier,fetch_witness_frontier_r2_miss_falls_to_the_generator,r2_frontier_band_is_narrower_than_routing,decode_past_the_deadline_surfaces_decode_timeout.cargo test --workspacegreen;fmt/clippy --all-targets --all-features/cargo sortclean.Acceptance in production
debug_trace_witness_requests_total{source="witness_r2_frontier"}appears; its error/request ratio is the real frontier hit rate (13/8,665 was only the lower bound observable via the gateway).phase=attempt_clampedwithattempt_ms≈6000followed by the same block succeeding onround=1(needs feat(rpc-client): make a deadline give-up say where the budget went #180's give-up fields).rpc_errors_total{reason="deadline_witness"}staying at 0 additionally needs feat(debug-trace-server): close the per-request accounting identity and label errors by reason #178 (separate stack) — until it lands, the client-error check falls back to the unlabeled error counter.upstream_requests_total{provider="1:mainnet...", outcome="success"}may drop toward zero on frontier traffic: expected, the bucket now serves those directly.🤖 Generated with Claude Code