Fix: stop retrying stale snapshot on transient errors

[cliff0412]

When snapshot transmission hits a transient transport error, openraft currently keeps retrying inside the same snapshot task. This means the leader can keep retrying an old snapshot even after a newer snapshot has already been built.

In the worst case, if the follower is effectively crashed and the transport keeps failing quickly, the snapshot task can stay in a very tight retry loop. With the current chunked transport, that loop sleeps only 1ms between attempts, so the leader may keep hammering the same stale snapshot over and over.

This patch changes snapshot retry behavior so transient snapshot send errors return to ReplicationCore. The next retry then fetches the current snapshot again.

---

This change is 

[drmingdrmer]

Thank you for addressing this issue! But have you used the wrong branch in this PR? It shouldn't be so many changes.

[cliff0412]

Thank you for addressing this issue! But have you used the wrong branch in this PR? It shouldn't be so many changes.

i used release-0.9; it is 1 file change now. btw, if it is valid, possible making a quick minor release? this is affecting our production

[drmingdrmer]

Thank you for addressing this issue! But have you used the wrong branch in this PR? It shouldn't be so many changes.

i used release-0.9; it is 1 file change now. btw, if it is valid, possible making a quick minor release? this is affecting our production

Yes, make sense.

[cliff0412]

any update on this?

[drmingdrmer]

Thanks — the stale-snapshot loop is a real issue, but I think the fix is too coarse.

Returning Timeout / Network to ReplicationCore discards all chunk progress and restarts from offset 0 with a fresh snapshot. For a large snapshot on a flaky link, a single blip near the end means starting over.

A more proportionate fix:

1. Incremental backoff between chunk retries inside the send loop — the current 1ms sleep is what turns a failing transport into a hammering loop.
2. Bounded retry count per snapshot — only bail out to ReplicationCore after N consecutive failures, so transient errors resume from the last offset but a truly stuck transfer still yields to a refreshed snapshot.
3. Keep the RemoteError<Fatal> bail-out as-is — that one is correct.

Also, the test asserts 2 distinct snapshot IDs but not retry cadence. Worth bounding the number of InstallSnapshot RPCs within the window to guard against regressions back toward tight-loop retries.

[cliff0412]

Thanks — the stale-snapshot loop is a real issue, but I think the fix is too coarse.

Returning Timeout / Network to ReplicationCore discards all chunk progress and restarts from offset 0 with a fresh snapshot. For a large snapshot on a flaky link, a single blip near the end means starting over.

A more proportionate fix:

1. Incremental backoff between chunk retries inside the send loop — the current 1ms sleep is what turns a failing transport into a hammering loop.
2. Bounded retry count per snapshot — only bail out to ReplicationCore after N consecutive failures, so transient errors resume from the last offset but a truly stuck transfer still yields to a refreshed snapshot.
3. Keep the RemoteError<Fatal> bail-out as-is — that one is correct.

Also, the test asserts 2 distinct snapshot IDs but not retry cadence. Worth bounding the number of InstallSnapshot RPCs within the window to guard against regressions back toward tight-loop retries.

that makes sense; i will make change later on

[cliff0412]

updated as per review

[drmingdrmer]

@drmingdrmer reviewed all commit messages and made 3 comments.
Reviewable status: 0 of 3 files reviewed, 3 unresolved discussions (waiting on cliff0412).

---

openraft/src/network/snapshot_transport.rs line 102 at r2 (raw file):

const SNAPSHOT_CHUNK_MAX_RETRIES: u64 = 5;
const SNAPSHOT_CHUNK_RETRY_BASE_MILLIS: u64 = 10;

use Duration::from_millis() to define a typed time duration, instead of using millis in u64:
const SNAPSHOT_CHUNK_RETRY_BASE: Duration = Duration::from_millis(10).

The same for SNAPSHOT_CHUNK_RETRY_MAX_MILLIS

---

openraft/src/network/snapshot_transport.rs line 109 at r2 (raw file):

    let shift = consecutive_failures.saturating_sub(1).min(4) as u32;
    let millis = SNAPSHOT_CHUNK_RETRY_BASE_MILLIS.saturating_mul(1u64 << shift);

1u64<<shift may panic if shift is too large

---

openraft/src/network/snapshot_transport.rs line 132 at r2 (raw file):

        let end = snapshot.snapshot.seek(SeekFrom::End(0)).await.sto_res(subject_verb)?;
        let mut consecutive_failures = 0;
        let mut unreachable_backoff = None::<Backoff>;

consecutive_failures and Backoff seems to be duplicated.
Keep just one of these two: either increase consecutive_failures and then use it to calculate the sleep time, or just use Backoff to set up incremental sleep interval series.

Code quote:

        let mut consecutive_failures = 0;
        let mut unreachable_backoff = None::<Backoff>;

[cliff0412]

@drmingdrmer reviewed all commit messages and made 3 comments.
Reviewable status: 0 of 3 files reviewed, 3 unresolved discussions (waiting on cliff0412).

openraft/src/network/snapshot_transport.rs line 102 at r2 (raw file):

const SNAPSHOT_CHUNK_MAX_RETRIES: u64 = 5;
const SNAPSHOT_CHUNK_RETRY_BASE_MILLIS: u64 = 10;

use Duration::from_millis() to define a typed time duration, instead of using millis in u64: const SNAPSHOT_CHUNK_RETRY_BASE: Duration = Duration::from_millis(10).

The same for SNAPSHOT_CHUNK_RETRY_MAX_MILLIS

openraft/src/network/snapshot_transport.rs line 109 at r2 (raw file):

    let shift = consecutive_failures.saturating_sub(1).min(4) as u32;
    let millis = SNAPSHOT_CHUNK_RETRY_BASE_MILLIS.saturating_mul(1u64 << shift);

1u64<<shift may panic if shift is too large

openraft/src/network/snapshot_transport.rs line 132 at r2 (raw file):

        let end = snapshot.snapshot.seek(SeekFrom::End(0)).await.sto_res(subject_verb)?;
        let mut consecutive_failures = 0;
        let mut unreachable_backoff = None::<Backoff>;

consecutive_failures and Backoff seems to be duplicated. Keep just one of these two: either increase consecutive_failures and then use it to calculate the sleep time, or just use Backoff to set up incremental sleep interval series.

Code quote:

        let mut consecutive_failures = 0;
        let mut unreachable_backoff = None::<Backoff>;

regarding first 2 points; already made the changes;
for the third one, for Unreachable error, at file network/network.rs line 161

 /// Build a backoff instance if the target node is temporarily(or permanently) unreachable.
    ///
    /// When a [`Unreachable`](`crate::error::Unreachable`) error is returned from the `Network`
    /// methods, Openraft does not retry connecting to a node immediately. Instead, it sleeps
    /// for a while and retries. The duration of the sleep is determined by the backoff
    /// instance.
    ///
    /// The backoff is an infinite iterator that returns the ith sleep interval before the ith
    /// retry. The returned instance will be dropped if a successful RPC is made.
    ///
    /// By default it returns a constant backoff of 500 ms.
    fn backoff(&self) -> Backoff {
        Backoff::new(std::iter::repeat(Duration::from_millis(500)))
    }

this mentioned that need to use this from Unreachable node, which defaults to 500ms. i think we can stick to that and use longer retry delay; but for Timeout and Network error, we can use the shorter retry delay as defined in this PR

[drmingdrmer]

@drmingdrmer reviewed all commit messages and resolved 1 discussion.
Reviewable status: 0 of 3 files reviewed, 2 unresolved discussions (waiting on cliff0412).

[cliff0412]

@cliff0412 made 2 comments.
Reviewable status: 0 of 7 files reviewed, 2 unresolved discussions (waiting on drmingdrmer).

---

openraft/src/network/snapshot_transport.rs line 109 at r2 (raw file):

Previously, drmingdrmer (张炎泼) wrote…

1u64<<shift may panic if shift is too large

Done.

---

openraft/src/network/snapshot_transport.rs line 132 at r2 (raw file):

Previously, cliff0412 (cliff.li) wrote…

regarding first 2 points; already made the changes;
for the third one, for Unreachable error, at file network/network.rs line 161

 /// Build a backoff instance if the target node is temporarily(or permanently) unreachable.
    ///
    /// When a [`Unreachable`](`crate::error::Unreachable`) error is returned from the `Network`
    /// methods, Openraft does not retry connecting to a node immediately. Instead, it sleeps
    /// for a while and retries. The duration of the sleep is determined by the backoff
    /// instance.
    ///
    /// The backoff is an infinite iterator that returns the ith sleep interval before the ith
    /// retry. The returned instance will be dropped if a successful RPC is made.
    ///
    /// By default it returns a constant backoff of 500 ms.
    fn backoff(&self) -> Backoff {
        Backoff::new(std::iter::repeat(Duration::from_millis(500)))
    }

this mentioned that need to use this from Unreachable node, which defaults to 500ms. i think we can stick to that and use longer retry delay; but for Timeout and Network error, we can use the shorter retry delay as defined in this PR

DONE

[drmingdrmer]

@drmingdrmer reviewed 7 files and all commit messages, and resolved 2 discussions.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on cliff0412).

[ariesdevil]

Why not just use _ => err

[drmingdrmer]

in case there are new error variants added.

[ariesdevil]

I don't like this style, there is only one branch continue, others all return error.

[drmingdrmer]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on cliff0412).