fix(checkpoint-sync): retry on timeout

[MegaRedHand]

Problem

In Hive interop runs, ethlambda intermittently fails to start with the harness reporting client did not start: terminated unexpectedly. On the dashboard this surfaces as flaky reqresp failures against whichever scenario's fresh client-boot happened to coincide with the peer being slow (a different scenario, or none, fails each run).

The container log shows the real cause:

Starting checkpoint sync url=http://172.17.0.3:5052/lean/v0/states/finalized
(+15.0s) WARN Checkpoint sync failed for this peer; no more URLs to try  err=error sending request ... operation timed out
ERROR All checkpoint sync attempts failed
ERROR Failed to initialize state
Error: operation timed out  @ bin/ethlambda/src/main.rs:208

Root cause

Checkpoint sync fails fast on any connection/timeout error. Hive supplies a single checkpoint URL, and try_checkpoint_url only retries the AnchorPairingMismatch case; any connection/timeout/HTTP error returns immediately. So one 15s miss to a peer that is still booting propagates up through fetch_initial_state (main.rs ?) and exits the whole process, well inside the harness's client-startup budget where a retry would have succeeded.

Fix

Wrap the anchor fetch in a bounded retry (fetch_anchor_with_retry) that reattempts a failed fetch for up to MAX_CHECKPOINT_ATTEMPTS attempts with a fixed CHECKPOINT_RETRY_BACKOFF delay between them. When each attempt fails fast at the connect timeout, the attempts stay within the harness's client-startup budget.

The per-attempt 15s connect/read timeouts and the internal AnchorPairingMismatch retry are intentionally left unchanged.

Testing

• cargo build -p ethlambda, cargo clippy -p ethlambda -- -D warnings, cargo fmt -p ethlambda -- --check — clean.
• cargo test -p ethlambda checkpoint_sync — existing checkpoint-sync unit tests pass (22).

[github-actions]

🤖 Codex Code Review

1. fetch_anchor_with_retry can abort too early when URL failures are mixed, because retry eligibility is based only on the last peer error. fetch_anchor_block_and_state returns the last failure it saw, and fetch_anchor_with_retry retries only if that final error is Http. With URLs like [transient-unreachable good peer, deterministic-bad fallback peer], the first peer is never retried: the fallback’s non-transient error wins and startup exits immediately. That makes behavior order-dependent and undermines the stated “retry instead of crashing on the first miss” goal. I’d keep a saw_transient flag across the whole URL pass, or move the retry policy down to per-URL handling instead of wrapping the entire URL list.

2. The advertised 90-second “total wall-clock budget” is not actually enforced. In fetch_anchor_with_retry, deadline only controls whether another sleep(backoff) is allowed; it does not bound the next fetch_anchor_block_and_state() call. Each pass can already spend multiple 15-second connect timeouts across URLs, and the client explicitly uses an inactivity timeout rather than a total response timeout in build_client. In practice this can exceed 90 seconds by a wide margin. If the startup budget matters, wrap each outer attempt in a timeout based on the remaining budget.

No consensus-state-transition, fork-choice, attestation, XMSS, or SSZ logic changed in this diff; the risk here is startup/liveness rather than consensus correctness.

I couldn’t run the Rust tests in this environment because cargo/rustup attempted to write under /home/runner/.rustup, which is read-only here.

---

Automated review by OpenAI Codex · gpt-5.4 · custom prompt

[greptile-apps]

Greptile Summary

This PR adds bounded retries for transient checkpoint-sync failures during startup. The main changes are:

• Adds transient HTTP error classification.
• Adds exponential backoff with a 90-second budget.
• Routes initial checkpoint fetching through the retry wrapper.

Confidence Score: 4/5

The checkpoint retry path can exceed its startup deadline and delay deterministic HTTP failures.

• The total budget does not constrain an active fetch.
• Client and configuration HTTP errors consume the retry budget instead of failing immediately.
• The caller change in main.rs is otherwise straightforward.

bin/ethlambda/src/checkpoint_sync.rs

Important Files Changed

Filename	Overview
bin/ethlambda/src/checkpoint_sync.rs	Adds error classification and retry logic, but active requests are not bounded by the total deadline and permanent HTTP failures are retried.
bin/ethlambda/src/main.rs	Switches initial checkpoint loading to the new retry wrapper.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Fetch initial state] --> B[Fetch checkpoint anchor]
    B --> C{Result}
    C -->|Success| D[Return state and block]
    C -->|Non-transient error| E[Return error]
    C -->|HTTP error| F{Backoff fits before deadline?}
    F -->|No| E
    F -->|Yes| G[Sleep with exponential backoff]
    G --> B

Loading

Prompt To Fix All With AI

Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 2
bin/ethlambda/src/checkpoint_sync.rs:384
**In-Flight Fetch Escapes Deadline**

The deadline only checks whether the backoff fits; it does not limit the fetch started after that sleep. A request that starts near the deadline can run past 90 seconds, and a peer that keeps sending data within each inactivity timeout can extend it much longer, so startup can still exceed the harness budget.

### Issue 2 of 2
bin/ethlambda/src/checkpoint_sync.rs:90-91
**Permanent HTTP Errors Are Retried**

`error_for_status()` represents responses such as 401, 403, and 404 as `reqwest::Error`, so this treats invalid credentials or a wrong endpoint as transient. With a single URL, startup repeats the same deterministic failure with backoff until the budget is consumed instead of returning it immediately.

_{Reviews (1): Last reviewed commit: "fix(checkpoint-sync): retry transient fa..." | Re-trigger Greptile}

[github-actions]

🤖 Kimi Code Review

The retry logic is well-structured with proper deadline tracking and exponential backoff capping, but there's a critical bug in the error classification:

Bug: Incomplete transient error detection (checkpoint_sync.rs:91-93)

The is_transient method only matches CheckpointSyncError::Http(_), but this likely represents HTTP-level failures (4xx/5xx), while transport-level failures (Reqwest for connection refused/DNS, Timeout for slow peers) are the actual transient conditions you want to retry when "peers boot concurrently."

// Current (incorrect)
fn is_transient(&self) -> bool {
    matches!(self, CheckpointSyncError::Http(_))
}

// Should be:
fn is_transient(&self) -> bool {
    matches!(self, 
        CheckpointSyncError::Reqwest(_) | 
        CheckpointSyncError::Timeout
    )
}

If Http is indeed for non-retryable HTTP errors (e.g., 404), consider excluding it. If it wraps transport errors, include it but clarify the distinction.

Consideration: Syncing peers (checkpoint_sync.rs:91)

MissingFinalizedBlock and MissingFinalizedState could be transient if the checkpoint peer hasn't finished syncing yet. Consider adding these to is_transient if your test harness scenarios involve concurrent booting where the checkpoint peer might lag slightly behind the finalized head.

Minor: Deadline edge case (checkpoint_sync.rs:381)

The condition Instant::now() + backoff < deadline means if you have 14 seconds remaining and the backoff is 15 seconds, you give up immediately rather than trying one final time. This is conservative but acceptable given the 90-second budget.

Positives:

• Correct use of Instant (monotonic clock) for deadline tracking
• Proper exponential backoff with min() capping
• Structured logging with the attempt counter is helpful for debugging
• The separation of fetch_anchor_with_retry from the core fetch logic maintains clean separation of concerns

Recommendation: Fix the is_transient match arms to include Reqwest and Timeout before merging.

---

Automated review by Kimi (Moonshot AI) · kimi-k2.5 · custom prompt

[github-actions]

🤖 Claude Code Review

Review: fix(checkpoint-sync): retry transient failures within startup budget (PR 537)

Overall the approach is sound — wrapping the existing single-shot fetch_anchor_block_and_state in a bounded, exponential-backoff retry is the right fix for the Hive flakiness described, and the implementation is clean (monotonic Instant deadline, capped backoff, non-transient errors still fail fast). A few issues worth addressing before merge:

1. is_transient() conflates network-level failures with permanent HTTP-status errors

checkpoint_sync.rs:90-92

fn is_transient(&self) -> bool {
    matches!(self, CheckpointSyncError::Http(_))
}

CheckpointSyncError::Http wraps any reqwest::Error, including the one produced by .error_for_status()? in fetch_ssz (checkpoint_sync.rs:121). That means a peer returning a deterministic 404/400 (wrong endpoint path, incompatible API version, misconfigured peer) gets classified as "transient" and retried with backoff for the full 90s budget before the real error ever surfaces — exactly the kind of config/deployment bug this PR wants to fail fast on for everything else. The comment on is_transient ("peer not yet reachable... not yet serving the finalized endpoints") describes a connect/timeout failure, not an HTTP status failure.

Suggest narrowing the check using reqwest::Error's own classifiers, e.g.:

fn is_transient(&self) -> bool {
    matches!(self, CheckpointSyncError::Http(e) if e.is_connect() || e.is_timeout())
}

(optionally also retry on 5xx via e.status().is_some_and(|s| s.is_server_error()), since a booting peer plausibly 500s). This keeps 4xx errors (wrong path, version mismatch, bad request) failing immediately as before.

2. Documentation is now stale

docs/checkpoint_sync.md:55 still states: "There is no automatic retry; restart the node to try again." This is no longer true after this change and should be updated to describe the bounded retry/backoff behavior (and roughly the 90s budget), otherwise operators debugging a slow checkpoint sync will be misled by the docs.

3. No test coverage for the new retry logic

The PR only re-runs the pre-existing checkpoint_sync unit tests; nothing exercises is_transient()'s classification or fetch_anchor_with_retry's retry/backoff/deadline behavior — which is the actual behavioral change being shipped. At minimum, a unit test asserting is_transient() returns false for representative non-Http variants (and, after Point 1's fix, false for a status-code-derived Http error) would guard against regressions here.

4. Minor: retry budget is looser than "90s" for multi-URL configs

fetch_anchor_with_retry retries the entire fetch_anchor_block_and_state call, which itself iterates over all checkpoint_urls and internally retries AnchorPairingMismatch up to MAX_ANCHOR_FETCH_ATTEMPTS times per URL. With more than one configured URL (or 15s connect/read timeouts stacking across peers), a single outer attempt can already consume a large fraction of the 90s budget before the outer loop's deadline check is even evaluated. Fine for the single-URL Hive case described in the PR, but worth a code comment noting the budget is a soft/best-effort bound once multiple URLs are configured, not a hard ceiling.

5. Minor: last partial retry window is wasted

Err(err) if err.is_transient() && Instant::now() + backoff < deadline bails without a final attempt whenever the remaining budget is less than the current backoff (up to 15s), even if a shorter wait would still fit. Not a bug, just leaves some budget on the table — could clamp backoff to deadline.saturating_duration_since(Instant::now()) instead of bailing outright, if squeezing out the last attempt matters.

None of these block the core fix from working for the reported Hive scenario (single URL, connect timeout), but Point 1 in particular changes the safety property the PR advertises ("deterministic validation/config error... still fails immediately") — right now that only holds for the hand-rolled CheckpointSyncError variants, not for HTTP-status errors funneled through Http.

---

Automated review by Claude (Anthropic) · sonnet · custom prompt