fix: retry catch-up batch fetches instead of panicking on a transient miss

[procdump]

The problem

A CvvInactive node catching up fetches each committed subdag's batches from peers.
When that fetch fails transiently — a PeerNotConnected while the worker network is
still re-meshing right after a mode transition — the pipeline retried only 5 times
(~10s), then surfaced the error as fatal and panicked the subscriber. Because that's
a critical task, the panic tears down the whole epoch task stack; the relaunch
re-enters the same catch-up and hits the same wall.

The batch is never actually lost: it belongs to a committed subdag, so it's held by
peers (and, for a rejoining node, usually on local disk too). The node can't skip it
(skipping a committed subdag forks) and can't progress without it — so the only correct
response to a momentary unavailability is to wait, not to crash. Reproduced by a chaos
test (bombard an observer, kill a validator): the node executed the very same outputs a
minute later from local disk, after a gratuitous crash-and-relaunch.

What's changed

• Extract fetch_batches_retrying, which retries is_batch_fetch_error failures with
  capped backoff until success or shutdown, and route both catch-up sites (the pipelined
  fetch and the startup missing-header fetch) through it.
• Logging stays loud for the first few attempts, then a throttled heartbeat so a
  prolonged outage stays visible without flooding.
• Genuine non-transient errors remain fatal; the live CvvActive path is unchanged (its
  batches are local). Shutdown is the only exit from the retry loop.

Follow-up (separate PR): after a retry threshold, escalate to a targeted worker-network
respawn rather than retrying forever — for the case where a wedged worker swarm only
clears on respawn.

[github-actions]

Claude finished @procdump's task in 2m 31s —— View job

---

PR Review

• Gather context
• Read changed file and diff
• Post review feedback

---

Summary

The fix is correct and well-reasoned. Replacing the 5-attempt cap with an unbounded retry (shutdown-only exit) is the right behaviour for committed subdag batches: the batch cannot be skipped without forking, and the only alternative to waiting is crashing and re-entering the same wall. The extraction of fetch_batches_retrying is clean, and routing both catch-up sites through it closes the gap at the startup missing-header path.

No blocking issues. A few things worth calling out:

---

Medium: No test for fetch_batches_retrying itself

subscriber.rs:1405-1411 has test_is_batch_fetch_error, which tests the helper but not the retry loop. A unit test for fetch_batches_retrying (mock fetch_batches that fails N times with a ClientRequestsFailed then succeeds; verify return value, retry count, shutdown exit) would meaningfully strengthen confidence in the new logic — especially since the retry count cap was deliberately removed.

Fix this →

---

Minor: rx_shutdown subscribed inside the retry arm

subscriber.rs:387 — shutdown.subscribe() is called on every retry iteration, after fetch_batches returns an error. If shutdown fires in the window between the error return and the subscribe() call, the signal is caught only after the next delay sleep (up to 2 s). If shutdown() returns a watch handle, subscribe() reads the current value and would catch an already-fired shutdown instantly — but if it's a broadcast or a one-shot notifier, there's a real (if small) delay. Subscribing once before the outer loop and passing a &mut rx_shutdown into the select would eliminate the window:

let mut rx_shutdown = shutdown.subscribe();
loop {
    match self.fetch_batches(deliver.clone(), parent_hash, number).await {
        ...
        Err(e) if e.is_batch_fetch_error() => {
            ...
            tokio::select! {
                biased;
                _ = rx_shutdown => { ... }
                _ = tokio::time::sleep(delay) => continue,
            }
        }
        ...
    }
}

(The old code had the same pattern, so this is not a regression — but it's a good moment to fix it while touching this section.)

---

Minor: Silent gap between throttle levels (retries 6–14)

subscriber.rs:378 — if retry_count <= 5 || retry_count % 15 == 0. With a 2 s cap on delay, retries 6–14 produce no log at all: ~20 s of silence between the last loud warning (retry 5) and the first throttled heartbeat (retry 15). An operator monitoring a prolonged outage would see nothing during that window. retry_count % 10 == 0 (every 20 s) or even using elapsed time instead of a count-based gate would tighten this gap without flooding the log.

---

Nit: retry_count.min(5) is a necessary overflow guard — worth a comment

subscriber.rs:375:

let delay = Duration::from_secs(2)
    .min(Duration::from_millis(500) * 2u32.saturating_pow(retry_count.min(5)));

saturating_pow caps the u32 at u32::MAX, but Duration * u32::MAX panics on overflow in Rust. The .min(5) prevents that (2^5 = 32; 500 ms × 32 = 16 s, safely within Duration range). This is load-bearing and not obvious — a short inline comment (// cap exponent to avoid Duration overflow) would help the next reader.

---

Confirmed correct

• Live CvvActive path (subscriber.rs:1072) correctly keeps fetch_batches directly — no change to the hot path.
• deliver.clone() on each retry is pre-existing and necessary since fetch_batches takes ownership. Acceptable on the catch-up path.
• Logging throttle overall is a good pattern for a prolonged outage — the concern above is only about the gap size.
• is_batch_fetch_error coverage (MissingFetchedBatch | ClientRequestsFailed) correctly distinguishes transient peer-availability failures from non-transient errors.

[github-actions]

Claude finished @procdump's task in 1m 43s —— View job

---

• Gather context
• Read CLAUDE.md
• Read changed file and diff
• Post review feedback

---

PR Review

The fix is correct and well-reasoned. Replacing the 5-attempt cap with a shutdown-only exit is the right behaviour for committed subdag batches: they cannot be skipped without forking, and crashing re-enters the same wall. The extraction of fetch_batches_retrying is clean, and routing both catch-up sites through it closes the gap at the startup missing-header path.

One thing the previous review got wrong — corrected below. Two issues remain worth addressing.

---

Correction: shutdown.subscribe() inside the retry arm is safe

The previous review flagged subscriber.rs:387 as a potential missed-shutdown window. After reading crates/infrastructure/types/src/notifier.rs, this is not an issue: Notifier::subscribe() checks notified atomically at call time (line 59) and returns a pre-resolved Noticer if notify() already fired (lines 65–67). So if shutdown fires in the window between fetch_batches returning and the subscribe() call, the select! arm resolves immediately. The concern in the prior review assumed broadcast/oneshot semantics; the actual implementation guarantees no missed signal regardless of ordering.

---

Medium: No test for fetch_batches_retrying

subscriber.rs:1405–1411 tests is_batch_fetch_error but not the retry loop. Given the retry-count cap was deliberately removed and the loop is now unbounded, a unit test would meaningfully strengthen confidence:

• (a) ClientRequestsFailed retried N times then succeeds → correct output returned
• (b) non-transient error propagates without retrying
• (c) shutdown fires mid-sleep → returns ClosedChannel

Fix this →

---

Minor: ~18 s silent gap between throttle levels (retries 6–14)

subscriber.rs:378:

if retry_count <= 5 || retry_count % 15 == 0 {

With a 2 s cap on delay, retries 6–14 produce no log at all — roughly 18 s of silence between the last loud warning (retry 5) and the first throttled heartbeat (retry 15). retry_count % 10 == 0 would tighten this to ~10 s without flooding:

if retry_count <= 5 || retry_count % 10 == 0 {

---

Nit: .min(5) is a load-bearing overflow guard — worth a comment

subscriber.rs:375:

let delay = Duration::from_secs(2)
    .min(Duration::from_millis(500) * 2u32.saturating_pow(retry_count.min(5)));

Without .min(5), at high retry counts 2u32.saturating_pow(retry_count) saturates to u32::MAX and Duration::from_millis(500) * u32::MAX panics (overflow). The .min(5) caps the exponent at 32 (500 ms × 32 = 16 s), safely within range. This is non-obvious; a short inline comment would help:

let delay = Duration::from_secs(2)
    // cap exponent: Duration * u32::MAX panics on overflow
    .min(Duration::from_millis(500) * 2u32.saturating_pow(retry_count.min(5)));

---

Confirmed correct

• Live CvvActive path (subscriber.rs:1072) correctly keeps fetch_batches directly — no change to the hot path.
• deliver.clone() on each retry is pre-existing and necessary since fetch_batches takes ownership. Acceptable on the catch-up path.
• is_batch_fetch_error coverage (MissingFetchedBatch | ClientRequestsFailed) correctly distinguishes transient peer-availability failures from non-transient errors.
• Logging throttle is a good pattern for a prolonged outage — the concern above is only about the gap size.
• Noticer resource cleanup: dropped Noticers from previous loop iterations are cleaned up by the retain check in the next subscribe() call, so repeated subscription in the retry loop doesn't cause unbounded accumulation.

[github-actions]

Claude finished @procdump's task in 1m 45s —— View job

---

• Gather context
• Read CLAUDE.md
• Read changed file and diff
• Post review feedback

---

PR Review

The fix is correct and well-reasoned. Replacing the 5-attempt cap with a shutdown-only exit is the right response for committed subdag batches: they cannot be skipped without forking, crashing just re-enters the same wall, and the batch is never actually lost. The extraction of fetch_batches_retrying is clean, and closing the gap at the startup missing-header path (subscriber.rs:813) is a good catch — that site was equally vulnerable to transient failures.

Two issues remain worth addressing before merge.

---

Medium: No test for fetch_batches_retrying

subscriber.rs:1405–1411 tests is_batch_fetch_error in isolation but not the retry loop. Given the retry cap was deliberately removed and the loop is now unbounded, a unit test would meaningfully strengthen confidence:

• (a) ClientRequestsFailed returned N times then success → correct output returned
• (b) non-transient error (ClosedChannel, ExecutorConnectionDropped) propagates immediately without retrying
• (c) shutdown fires during the sleep window → returns ClosedChannel

Fix this →

---

Minor: ~18 s silent gap between throttle levels

subscriber.rs:378:

if retry_count <= 5 || retry_count % 15 == 0 {

With the 2 s delay cap, retries 6–14 produce no log at all — roughly 18 s of silence between the last loud warning (retry 5) and the first throttled heartbeat (retry 15). Tightening to % 10 would cut that to ~10 s without flooding:

if retry_count <= 5 || retry_count % 10 == 0 {

---

Nit: .min(5) is a load-bearing overflow guard — worth a comment

subscriber.rs:374–375:

let delay = Duration::from_secs(2)
    .min(Duration::from_millis(500) * 2u32.saturating_pow(retry_count.min(5)));

Without .min(5), at high retry counts saturating_pow saturates to u32::MAX and Duration::from_millis(500) * u32::MAX panics (overflow in multiplication). The .min(5) caps the exponent at 32 (500 ms × 32 = 16 s), safely within range — but it's non-obvious. A short inline comment would help the next reader:

let delay = Duration::from_secs(2)
    // cap exponent: Duration * u32::MAX overflows and panics
    .min(Duration::from_millis(500) * 2u32.saturating_pow(retry_count.min(5)));

---

Confirmed correct

• Live CvvActive path (subscriber.rs:1072) correctly keeps fetch_batches directly — no change to the hot path.
• shutdown.subscribe() inside the retry arm is safe: Notifier::subscribe() checks notified atomically at call time and returns a pre-resolved Noticer if notify() already fired — no missed-signal window regardless of ordering.
• deliver.clone() on each retry is necessary since fetch_batches takes ownership. Acceptable on the catch-up path.
• is_batch_fetch_error coverage (MissingFetchedBatch | ClientRequestsFailed) correctly distinguishes transient peer-availability failures from non-transient errors.
• biased; in the select ensures shutdown is always polled before the sleep future — correct.
• Startup missing-header path (subscriber.rs:813) now correctly retries through fetch_batches_retrying, closing a gap that would have hit the same wall as the pipelined path.