feat(debug-trace-server): age-based witness routing, configurable old-block budget, tip buffer - #158
Conversation
…-block budget, tip buffer Witness fetches route by block age: blocks at least --witness-local-window (default 4096, matching the witness generator's BACKUP retention) below the local DB tip skip the first witness endpoint — the internal generator, a guaranteed miss for anything it has pruned — via a skip parameter on the shared RpcClient witness path: one client, one witness semaphore, and retry-loop provider indices stay aligned with the configured endpoint list. Routing requires two or more witness endpoints and --data-dir (the local tip anchors block age); otherwise all fetches use the full chain and startup logs say so. The old-block witness budget (previously a hardcoded 3s fail-fast that was the dominant source of customer-visible -32001 timeouts) becomes --witness-old-block-timeout, defaulting to the full witness budget (8s). New --tip-buffer (default 2) keeps the chain-sync pipeline from racing the generator at the head, validated at startup to stay below --blocks-to-keep. Witness-fetch failure logs carry source/old_block/budget_ms/elapsed_ms for incident attribution, and the historical route gets its own witness_historical metrics label. The routing dispatch is tested end-to-end against two counting mock endpoints, plus route/label selection, env parsing of the new knobs, and a skip-all panic guard. README/AGENTS document the positional first-endpoint contract and the new knobs. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…iered-fetch # Conflicts: # crates/stateless-common/src/rpc_client.rs
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 569c41fcc8
ℹ️ 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".
…ite pressure The stall-WARN capture test raced two ways: a fixed 1s wall-clock window around the whole fetch could expire before a loaded runner scheduled the first 50ms attempt timeout, and concurrent tests driving the same tracing callsites with no subscriber installed could race set_default's interest-cache rebuild, re-caching a stale `never` for the instrument span so every WARN lost its block_number field. The fetch now runs as a background task while the test polls the captured logs (finishing on the first span-carrying WARN, 30s ceiling), periodically rebuilding the interest cache and respawning the fetch so a clobbered cache self-heals. Reproduced 6/15 before, 0/30 after under --test-threads=2. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… home tip-buffer invariant in core Apply the /simplify review of PR #158. WitnessFetchConfig loses route_historical: fetch_witness derives the skip from the client's own endpoint count (the data-dir half was already implied — an unknown tip counts as recent), so the cross-crate "must only be set when..." invariant becomes unrepresentable and the config turns Copy. round_robin_with_backoff drops provider_idx_base: endpoint labels bake in their global index, so the numeric provider_idx duplicated the adjacent label in every log line. PipelineConfig::validate() homes the tip_buffer < stale_reset_threshold invariant in core where both fields live and run_pipeline enforces it for every embedder; the CLI check stays for flag-named errors. Also: DEFAULT_OLD_BLOCK_WITNESS_TIMEOUT_SECS structurally equals DEFAULT_WITNESS_TIMEOUT_SECS, an is_old_block helper unifies the twice-written predicate, record_block_distance reuses db_tip_height, DataProvider::new takes a Duration like the config it sits beside, the deflake test becomes two bounded loops with a single abort path, and the retention rationale collapses onto the DEFAULT_WITNESS_LOCAL_WINDOW doc instead of eight restatements. 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: 08c87271bf
ℹ️ 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".
…; log stripped endpoint label at startup Address the remaining review findings on PR #158. Two coverage pins: a single-witness-endpoint e2e test proves a historical block fetches through the sole endpoint without tripping the skip assert (a >= 1 regression of the fallback derivation now fails the suite — mutation-verified), and a three-endpoint skip test asserts every recorded attempt keeps its original-index {idx}:{host} label (the parallel-slicing property the per-endpoint metrics rely on). The routing-active startup log now emits the credential-stripped label via the new RpcClient::witness_provider_label accessor instead of the raw configured URL, matching the crate's own label hygiene. Boundary prose unified onto fewer-than/at-least wording in the CLI help, module docs, and metric docs (the exactly-at-the-window block is historical, as the tests pin). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Codex review on PR #158 correctly noted the routing contract reads broader than the implementation: the background chain-sync prefetch still uses the full endpoint chain, so during deep catch-up with --blocks-to-keep beyond the generator's retention each pruned block burns one generator probe before failover. That trade-off is intentional — the sync frontier normally trails the remote head by only tip_buffer, and routing there would need a remote-head anchor instead of the local tip — so scope the claims instead of the code: README, AGENTS.md, the module doc, and the --witness-local-window help now say routing applies to request serving, and TraceFetcher documents the accepted catch-up cost. 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:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eb6d6af1b6
ℹ️ 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".
…-timeout Address the Codex P2 on PR #158: --witness-old-block-timeout becomes optional and, when unset, tracks the effective --witness-timeout instead of a fixed 8s constant — an operator who raises the witness budget no longer leaves old blocks silently capped at 8s. An explicit value still wins and is clamped to --witness-timeout (now also in the startup log, so the logged budget always equals the effective one). The DEFAULT_OLD_BLOCK_WITNESS_TIMEOUT_SECS constant is deleted; the test helper mirrors the unset-flag semantics, and CLI tests pin both raised-budget tracking and explicit-flag-wins. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
vincent-k2026
left a comment
vincent-k2026
left a comment
There was a problem hiding this comment.
Approve — age-based witness routing is a solid reliability fix. No correctness/security/data-loss/liveness blockers.
Verified locally on the PR branch (HEAD 1e2086a, nightly-2026-02-03):
cargo test -p stateless-common -p stateless-core -p debug-trace-server→ green, 0 failed. All new routing/skip/validation tests run and pass — incl. the single-endpoint>=2-fallback guard, the original-index label-alignment test under slicing, and the skip-allshould_panic.cargo fmt --check→ clean.
The crux is right: using the local DB tip as a proxy for the generator's retention is conservative. Because the local sync trails the head (tip_buffer), local_tip <= gen_tip, so any block judged historical (block + local_window <= local_tip) is genuinely below the generator's BACKUP floor — skipping it can never drop a servable endpoint, and the reverse mis-classification only costs one redundant generator probe that failover covers. --witness-local-window is safe to mis-set either direction. Invariant is structurally enforced (have_fallback = count>=2 derivation + assert!(skip < len) at the choke point, should_panic-tested) and double-validated for tip_buffer (CLI validate_args + core PipelineConfig::validate()). Metrics pre-registered, doc mirrors updated, db_tip read once (no torn read).
Non-blocking:
- (worth confirming) The old-block witness budget default moves 3s → full witness budget (8s). The routing is the real
-32001fix; the budget default is a coupled change. Net effect: a witness pruned everywhere now takes up to 8s to surface-32001(was 3s), and this applies to the within-window old class too (still generator-first). Intended? It's documented and mitigatable via--witness-old-block-timeout 3— just confirming the default is deliberate. - (minor) The success-path
witness_historicalmetric attribution isn't pinned by a test — the mocks only error, sofetch_witness's Ok branch never runs. Low risk (witness_route's label is unit-tested); a follow-up assertion would close it. - (minor) The deflaked
test_witness_failure_log_carries_block_numberhas a ~30s worst-case self-healing ceiling, so a real span-field regression takes that long to go red.#[serial]isolation might be simpler. Follow-up.
LGTM.
… generator convention Address the design review comment on PR #158: the generator is a different kind of endpoint (prunes beyond BACKUP, probed first only for recent blocks), so encode that in the interface instead of in ordering. New optional --witness-generator-endpoint declares it explicitly; --witness-endpoint now lists only durable fallbacks (optional when the generator flag is set — generator-only works like any single-endpoint config). The combined chain always puts the generator at index 0, so the fetch-side skip derivation (count >= 2) and everything in data_provider/rpc_client are unchanged. Misordering becomes unrepresentable, and validate_args rejects the likely migration mistake of leaving the generator duplicated in the fallback list. Backward compat for one release: without the flag, the first of two or more --witness-endpoint values still acts as the generator, with a startup deprecation warning when routing is active. The startup config log now shows the combined effective chain. README/AGENTS.md updated; tests pin all chain shapes, the deprecation flag, generator-only parsing, the neither-flag rejection, and the duplicate guard. 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: fd7cebcea6
ℹ️ 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".
… via --witness-generator-endpoint only Address the follow-up design review on PR #158: the positional convention never shipped, so there is no released behavior to stay compatible with, and keeping the inference would make the upgrade behavior-changing — a plain [durable-A, durable-B] failover pair on main would silently gain generator-skipping — while leaving "two durable endpoints, no generator" inexpressible. Now only a declared --witness-generator-endpoint is ever skipped; without it every witness endpoint is plain failover, exactly the pre-PR semantics, and routing is a deliberate opt-in. The generator-first knowledge cannot be derived from the client (all endpoints speak the same witness RPC), so WitnessFetchConfig carries generator_first — unlike the derivable routing bool removed earlier, this is irreducible CLI knowledge, and the count-based guard at the use site still makes an empty rotation unrepresentable. The deprecation warning, the chain bool, and the deprecated-form paragraphs in README/AGENTS.md and both flag docs all disappear. A new e2e test pins that a generator-less failover pair never skips; the single-endpoint test now pins the fallback guard with a declared generator. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
3 participants
Summary
Witness fetches in the debug-trace-server now route by block age: blocks at least
--witness-local-window(default 4096, matching the witness generator'sBACKUPretention) below the local DB tip skip the first witness endpoint — the internal generator, a guaranteed miss for anything it has pruned — and fetch straight from the fallback endpoints. Previously every historical request burned a doomed generator probe plus a failover round trip before reaching an endpoint that could actually serve it, and the hardcoded 3s old-block fail-fast was the dominant source of customer-visible-32001timeouts.Design
The historical route is a skip parameter on the shared witness path, not a second client:
RpcClient::get_witness_light_with_deadline_from(skip, ..)(crates/stateless-common/src/rpc_client.rs:649) sliceswitness_providers[skip..]and its parallel endpoint labels into the existing retry loop. Attribution rides on the{idx}:{host}labels (endpoint_label,rpc_client.rs:899), which bake in each endpoint's index in the full configured list at construction, so sliced routes stay globally correct in logs and per-endpoint metrics; the numericprovider_idxfield #156 added to the retry logs is dropped — under slicing the raw loop index would misattribute, and the label already carries the index. One client means one witness concurrency semaphore (the--witness-max-concurrent-requestscap stays global) and no duplicate connection pools.Routing is derived per fetch, not configured:
witness_route(bin/debug-trace-server/src/data_provider.rs:781) skips the generator when the block is historical and a fallback endpoint exists (witness_provider_count() >= 2,data_provider.rs:814), so the skip can never see an empty rotation. An unknown local tip counts as recent, so stateless mode (no--data-dir) never routes; the background chain-sync prefetch deliberately always uses the full chain (it fetches at the sync frontier, within the generator's retention except during deep catch-up with--blocks-to-keepbeyond it — documented onTraceFetcher). Startup logs state which mode is in effect, identifying the skipped endpoint by its credential-stripped label via the newRpcClient::witness_provider_label(rpc_client.rs:359). The first--witness-endpointis positionally the internal generator; README and AGENTS.md document the contract.New knobs:
--witness-old-block-timeout(main.rs:231) replaces the hardcoded 3s old-block fail-fast; when unset it tracks the effective--witness-timeoutbudget (raising the witness budget also raises the old-block cap), and explicit values are clamped to--witness-timeout.--witness-local-window(main.rs:225) sets the routing threshold;--tip-buffer(main.rs:238, default 2) keeps the chain-sync fetcher from racing the generator at the head. Thetip_buffer < stale-reset-thresholdinvariant is enforced twice:validate_args(main.rs:301) gives flag-named CLI errors, and the newPipelineConfig::validate()(crates/stateless-core/src/pipeline/config.rs:51) — enforced atrun_pipelineentry (pipeline/mod.rs:65) — protects every embedder that constructsPipelineConfigdirectly (the mega-reth FullNode setsstale_reset_threshold = Noneand is unaffected). Witness-fetch failure logs carrysource/old_block/budget_ms/elapsed_msfor incident attribution.Testing
The routing dispatch is pinned end-to-end in both directions: with two mock endpoints a historical block never touches the generator and a recent block probes it first; with a single endpoint a historical block is served by the sole endpoint without tripping the skip assert — a
>= 1regression of the fallback derivation now fails the suite (mutation-verified). A three-endpoint skip test asserts every recorded attempt keeps its original-index{idx}:{host}label, pinning the parallel-slicing property the per-endpoint metrics rely on. Unit tests cover theis_historicalboundary (including overflow and zero-window), the old-block budget clamp and its tracking default (unset follows a raised--witness-timeout; an explicit flag wins), route/label selection, CLI+env parsing of all three knobs, and the tip-buffer validation at both the CLI and core levels; a previously-flaky tracing log-capture test was made robust to runner load and callsite-interest races. Full workspacecargo test,cargo fmt --check,cargo sort --check, andcargo clippyare clean.Operational notes (dashboards & behavior changes)
source="witness_generator"no longer covers the whole RPC witness path: once routing is active, historical traffic records undersource="witness_historical"in bothDataSourceMetricsandWitnessSourceMetrics, and the RPC path is the sum of the two labels — rescope dashboards/alerts keyed onwitness_generator(same class of change as perf: zero-validation light witness decode; switch debug-trace-server onto it #154/feat(metrics): per-endpoint RPC attempt metrics + timeout reasons #156).provider_idxfield introduced by feat(metrics): per-endpoint RPC attempt metrics + timeout reasons #156;provider={idx}:{host}is the attribution mechanism — migrate any tooling that greps the numeric field.--witness-timeoutbudget (tracking it when unset); deployments that relied on the fail-fast should set--witness-old-block-timeout 3.--tip-buffer >= --blocks-to-keep(the built-in lag would otherwise trip a stale-anchor reset on every transient restart).--data-dir, routing is inert (fetch order unchanged from main); a startup warn calls out the multi-endpoint-without-data-dir case.Notes
origin/main(feat(metrics): per-endpoint RPC attempt metrics + timeout reasons #156, per-endpoint RPC attempt metrics) rather than rebasing; the retry-loop conflict was resolved by slicing providers and their labels in parallel onto feat(metrics): per-endpoint RPC attempt metrics + timeout reasons #156'sRpcAttemptOutcomeclassification, keeping the deadline-pressure timeout attribution intact.🤖 Generated with Claude Code