fix(l1): bound snap sync retries on PeerTable request timeouts

[NikhilSharmaWe]

Closes #6853

Summary

After #6823, PeerTable RequestTimeout is recoverable. With a snap header-download checkpoint, the manager could retry immediately forever if that timeout kept firing.

This change:

• returns a cycle outcome from Syncer::start_sync
• backs off on recoverable failures in the snap manager loop (1s, doubling, capped at 30s)
• exits after 5 consecutive PeerTable RequestTimeouts with no successful cycle in between
• leaves full sync alone (still stops until the next FCU when there's no checkpoint)

Scope note: backoff fixes the immediate retry for every recoverable error. The consecutive-failure cap (and shutdown) applies only to RequestTimeout. A persistent non-timeout recoverable error can still retry indefinitely at the 30s cap - intentional for #6853, not a hard give-up for RecoverableOther.

Test plan

• cargo test -p ethrex-p2p --lib sync::tests
• cargo test -p ethrex-p2p --lib sync_manager::tests

[greptile-apps]

Greptile Summary

This PR adds bounded retry handling for recoverable snap-sync failures while preserving full-sync loop behavior.

• Classifies recoverable sync-cycle results into success, PeerTable timeout, and other recoverable failures.
• Applies exponential backoff from 1 to 30 seconds while a snap checkpoint remains.
• Terminates after five PeerTable request-timeout cycles without an intervening successful cycle.
• Adds unit coverage for classification, reset behavior, backoff, checkpoint gating, and fatal escalation.

Confidence Score: 5/5

The PR appears safe to merge; no concrete changed-code defect remains after checking timeout classification, retry-state transitions, checkpoint gating, and caller compatibility.

The new outcome is handled by the sole caller, retry and fatal actions are applied only while snap synchronization remains active, successful cycles reset policy state, and checkpoint-read error behavior is unchanged from the base branch.

Important Files Changed

Filename	Overview
crates/networking/p2p/sync.rs	Adds cycle outcomes and narrowly classifies direct and wrapped PeerTable request-timeout errors; the only caller correctly consumes the new return value.
crates/networking/p2p/sync_manager.rs	Adds checkpoint-gated retry state, capped exponential backoff, fatal timeout escalation, and focused policy tests without introducing an actionable regression.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
  A[Run sync cycle] --> B{Cycle outcome}
  B -->|Success| C[Reset timeout count and backoff]
  B -->|Recoverable timeout| D[Increment timeout count]
  B -->|Other recoverable| E[Increase backoff]
  D --> F{Snap checkpoint remains?}
  E --> F
  C --> F
  F -->|No| G[Leave manager loop]
  F -->|Yes, fifth timeout| H[Exit process]
  F -->|Yes, recoverable failure| I[Sleep with capped exponential backoff]
  F -->|Yes, success| A
  I --> A

Loading

_{Reviews (1): Last reviewed commit: "fix(l1): bound snap sync retries on Peer..." | Re-trigger Greptile}

[NikhilSharmaWe]

cc @ilitteri

[ElFantasma]

A hard process::exit from inside a spawned task skips the node's shutdown sequence entirely — no store flush, no recorded cause, destructors never run. On a node with a live RocksDB and a block-data buffer that is drained by an explicit flush, that means dropping whatever the buffer was holding, on a path that fires precisely when the network is already unhealthy.

The codebase has machinery for this. spawn_fatal (cmd/ethrex/initializers.rs:243) exists so a subsystem can declare the node dead: it logs loudly, records the cause, cancels the node, and main exits non-zero after the shutdown sequence runs. Its own doc comment describes exactly this situation.

The catch is layering — spawn_fatal is in cmd/ethrex, and crates/networking/p2p can't depend upward on the binary, so it isn't callable from here. That makes this a design question rather than a one-line swap:

• surface the fatal condition to the caller (the manager task could return it, or set a flag the node loop polls) and let cmd/ethrex invoke its existing path, or
• hand SyncManager a cancellation token / shutdown handle at construction and trigger that instead of exiting.

Either keeps the fatal semantics you want while going through the flush. There are already a few process::exit calls under crates/networking/p2p (snap_sync.rs has three), so this isn't unprecedented — but those predate this PR and adding a fourth on a newly-reachable path is worth pausing on, especially since exit code 2 here is otherwise undocumented.

[NikhilSharmaWe]

This was relevant. Switched to your second option:

• Keep the construction-time CancellationToken on SyncManager
• On the timeout cap, cancel() instead of process::exit

That lets L1 run server_shutdown and flush the store.

[ElFantasma]

RecoverableOther backs off but never increments consecutive_timeouts, and only Success resets the backoff — so a persistent non-timeout recoverable error retries forever at the 30s cap.

That's a deliberate choice and the comment says so, and it matches #6853's scope. Worth being explicit in the PR description though, because the stated problem is "could retry immediately forever": this fixes immediately for every recoverable error but only fixes forever for RequestTimeout. A node stuck on a persistent RecoverableOther still loops indefinitely, just at 30s intervals rather than hot.

Bounded resource use, so not a blocker. But if the intent is that any persistent recoverable failure eventually gives up, a separate cap on total consecutive failures (reset by Success, same as the timeout counter) would cover it without conflating the two error classes.

[NikhilSharmaWe]

Updated the PR description to make this explicit:

• Backoff applies to all recoverable errors
• The consecutive-failure shutdown applies only to RequestTimeout
• A persistent RecoverableOther can still retry at the 30s cap