Consolidate connect-backoff schedule into henyey-common

[tomerweller]

Closes #3871

Summary

The peer connect-backoff schedule was implemented three times with independent copies of the same spec-cited constants and ceiling formula (SECONDS_PER_BACKOFF=10, MAX_BACKOFF_EXPONENT=10, random_uniform(1, 2^min(n,10)*10)):

• crates/overlay/src/peer_manager.rs::compute_backoff
• crates/overlay/src/manager/tick.rs::compute_connect_backoff (the live dial-gate path, Overlay: escalating jittered backoff for outbound connect retries #3771)
• crates/app/src/app/types.rs::compute_peer_backoff_secs (live + persisted to the peers table)

A change to one copy would silently leave the others stale, with no test to catch it. This PR adds a single leaf module henyey_common::peer_backoff owning the two constants and the deterministic ceiling connect_backoff_ceiling_secs(num_failures), and routes all three sites through it. The unbiased uniform sampling (gen_range(1..=ceiling)) stays at each site so henyey-common stays rand-free. The sampled distribution is bit-for-bit identical to before, so this is a pure dedup with no behavior change.

Plan reference

See the Converged Plan comment on #3871. Per the converged plan, this is scoped as a refactor (dedup); the residual preferred-peer restart-backoff behavioral gap is tracked separately as #3887.

Test plan

• cargo fmt --check
• cargo clippy -p henyey-common -p henyey-overlay -p henyey-app --all-targets (clean under -Dwarnings)
• cargo test -p henyey-common peer_backoff — 3 new tests pass
• cargo test -p henyey-overlay backoff — 8 preserved backoff/escalation tests pass
• cargo test -p henyey-app --lib peer — 81 peer-record/related tests pass

New coverage

• peer_backoff::tests::test_ceiling_schedule — ceiling yields 10,20,…,10240 for n=0..=10 and clamps at 10240 for n=11, 1000, u32::MAX (exercises the min(n, MAX_BACKOFF_EXPONENT) cap).
• peer_backoff::tests::test_schedule_constants_pinned — pins both constants to their spec values; the load-bearing drift guard.
• peer_backoff::tests::test_ceiling_never_below_one — ceiling is always >= 1 so gen_range(1..=ceiling) is never empty.

Deviations from plan

None. (Per the plan's note, henyey-common is kept rand-free, so the sampled-bounds check lives at the call sites — the existing overlay tick-loop backoff-bounds test covers it — rather than in the shared module.)

🤖 Generated with Claude Code

[tomerweller]

🔍 Reviewer: Correctness

Verdict: APPROVE

Summary: Clean, behavior-preserving dedup — the triplicated connect-backoff schedule (constants + ceiling) is collapsed into one henyey_common::peer_backoff source read by all three call sites, with a load-bearing drift-guard test. Matches the converged plan exactly.

Full review

Cycle 1 (no prior Correctness verdict on this PR) — complete class-labeled review.

Correctness — verified equivalent, no behavior change:

• New connect_backoff_ceiling_secs(n) = (1u64 << min(n, 10)) * 10. For n=0..=10 → 10,20,…,10240, clamped at 10240 beyond. Bit-identical schedule to all three prior copies.
• app/src/app/types.rs: old used saturating_mul + .max(1) guard; new uses plain * and no guard. Safe — max exponent 10 ⇒ 1<<10 * 10 = 10240, no overflow, and ceiling is always >= 10 so gen_range(1..=ceiling) is never empty. Equivalent.
• overlay/manager/tick.rs: ceiling type widens u32 -> u64. Harmless — max 10240 fits u32; the sampled value set is identical, so the RNG distribution is unchanged. The as u64 cast is correctly dropped since from_secs takes u64. The max_connect_backoff_secs test helper is repointed to the same u64 fn — straight delegation, no cast churn.
• overlay/peer_manager.rs: same substitution; the now-unused file-local SECONDS_PER_BACKOFF/MAX_BACKOFF_EXPONENT consts are deleted. Repo builds under -Dwarnings, so a stray const would be a hard error — removal is required and correct. No live symbol removed, no visibility narrowed; only-reachability action is deleting dead consts. dead-code build-verify gate: satisfied (CI green + reasoned, not a bare grep).

Test coverage (Kind: refactor per converged plan):

• No fail-on-main regression test is owed — this is a pure single-source dedup; the behavioral persistence-across-restart item is out of scope and tracked in already-filed Overlay preferred-peer dial gate and tick-loop retry_after ignore persisted next_attempt on restart #3887.
• New coverage present and load-bearing: test_ceiling_schedule (full 10..10240 schedule + clamp at n=11,1000,u32::MAX), test_schedule_constants_pinned (the drift guard — one definition now, all sites read it), test_ceiling_never_below_one (empty-range safety). Every new pub surface (connect_backoff_ceiling_secs, the two consts) is tested.
• Existing overlay/app backoff tests preserved and green in CI (Test, Clippy, Doctest, Build all SUCCESS).

No concerns. APPROVE.

[tomerweller]

🔍 Reviewer: Parity

Verdict: APPROVE

Summary: No observable/interop surface is touched. Connect-retry timing carries no wire framing and is explicitly on the freely-divergeable side of docs/PARITY.md; the sampled distribution is bit-for-bit unchanged, so even internal behavior is identical.

Full review

Cycle 1 (no prior Parity verdict on this PR) — complete class-labeled review.

Parity surface (per docs/PARITY.md) — nothing observable changes:

• Connect-backoff num_failures/next_attempt are node-local peer-DB state and in-memory dial gating. No SCP/overlay wire bytes, no ledger/bucket hash input, no tx result/meta XDR, no history archive format, no HTTP/RPC/CLI contract, no crypto output is affected. The issue and all three planning critics independently confirmed retry timing is on the freely-divergeable side of the boundary.
• The change is a mechanical dedup of the ceiling arithmetic + two constants into henyey-common; the min(n, MAX_BACKOFF_EXPONENT) cap and (1<<n)*10 schedule are preserved exactly, and the unbiased gen_range(1..=ceiling) sampling stays at each site. Sampled distribution is unchanged ⇒ no internal behavioral drift either.
• Doc comments correctly cite OVERLAY_SPEC §10.3-1 and stellar-core/src/overlay/PeerManager.cpp:365-410; centralizing pins the spec-cited schedule so future edits can no longer silently diverge across the three sites (the parity-hygiene win of the PR).
• MAX_FAILURES (the distinct peer-advertisement filter) is untouched, as planned.

No parity concern. APPROVE.

[tomerweller]

Review: Bounce-Back Cycle 1

Reason: CI failed (unrelated, will rebase)

Reviewer A (Correctness): APPROVE — clean behavior-preserving dedup of the triplicated backoff schedule into henyey_common::peer_backoff; drift-guard tests present; matches the converged plan exactly.
Reviewer B (Parity): APPROVE — no observable/interop surface touched; sampled distribution bit-for-bit unchanged; retry timing is on the freely-divergeable side of docs/PARITY.md.
External reviewers: none.
CI: red — 40/41 checks SUCCESS/SKIPPED; the sole failure is test (testnet, core,horizon, …) with conclusion CANCELLED.

Why unrelated (not diff-attributable): the diff is a pure internal Rust refactor (extract two constants + a ceiling function into henyey-common, route three call sites through it). The cancelled job is a live-testnet Quickstart integration probe (Run probes through wrapper) that ran from 02:07:22Z and was cancelled ~55 min later at 03:02:23Z — an external-network/workflow-timeout cancellation with no relation to the change. Every code-level check passed green: Test, Clippy, Doctest, Build (release-ci), CodeQL, and all local/pubnet integration jobs.

Routing back to ready-for-doing for /do Mode B to rebase on origin/main and re-trigger CI. If the testnet probe cancels again after rebase, the next /review-pr will mark this blocked for operator investigation of testnet-probe flakiness.

Bounce-back count: 1/3 (head-scoped), 1/6 (lifetime).

[tomerweller]

Rebased do/issue-3871 onto latest origin/main (now on top of 2b198d37) and force-pushed as 827b2278 to re-trigger CI. No code changes — the prior CI red was solely the unrelated test (testnet, core,horizon, …) Quickstart probe CANCELLED (external testnet/timeout), while all code-level checks (Test, Clippy, Doctest, Build, CodeQL, local/pubnet integration) were green. Both prior reviewer verdicts (Correctness, Parity) were APPROVE.

Local verification on the rebased tree: cargo fmt --check clean, cargo clippy --all -- -D warnings clean, and henyey-common/henyey-overlay/henyey-app tests pass — including the three drift-guard tests (test_ceiling_schedule, test_schedule_constants_pinned, test_ceiling_never_below_one).

Ready for re-review.

[tomerweller]

🔍 Reviewer: Correctness

Verdict: APPROVE

Summary: Clean, behavior-preserving dedup of the triplicated connect-backoff schedule into henyey_common::peer_backoff; drift-guard tests present and load-bearing; matches the converged plan.

Full review

Reviewed against current origin/main (head do/issue-3871).

• The new connect_backoff_ceiling_secs(n) = 2^min(n, MAX_BACKOFF_EXPONENT) * SECONDS_PER_BACKOFF is value-identical to all three prior in-line copies. Exponent is capped at 10, so max ceiling is 1<<10 * 10 = 10240 — no overflow in either the old u32 (tick.rs) or u64 (peer_manager.rs / app types.rs) arithmetic, and the two representations yield the same numeric result.
• The removed defensive guards are provably unnecessary: the old max.max(1) (app) and the "max_seconds always >= 1" comment (peer_manager) are subsumed because the ceiling is always >= SECONDS_PER_BACKOFF (>= 10). The gen_range(1..=ceiling) range is therefore never empty. Unbiased sampling stays at each call site, so henyey-common stays rand-free — good separation.
• Test coverage: test_ceiling_schedule (full doubling table + clamp at exponent 10, incl. u32::MAX), test_schedule_constants_pinned (the drift guard), test_ceiling_never_below_one. The overlay tick-loop bounds test still routes its max_connect_backoff_secs helper through the shared function, so the sampling path remains exercised.
• Linked issue is a refactor/enhancement; no regression-test-first gate applies. Persistence-of-backoff-across-restart (the issue's "suggested direction") is explicitly out of the converged plan's scope (the plan chose the drift-guard consolidation, which is the issue's stated minimum acceptable outcome); not a blocker here.

No correctness concerns.

[tomerweller]

🔍 Reviewer: Parity

Verdict: APPROVE

Summary: No observable/interop surface touched; connect-retry timing is on the freely-divergeable side of docs/PARITY.md; sampled distribution is bit-for-bit unchanged.

Full review

Reviewed against current origin/main (head do/issue-3871). Parity lens (overlay is parity-critical).

• The change touches only the internal connect-backoff schedule ceiling computation. Per docs/PARITY.md, the observable/interop surface is ledger/bucket hashes, tx result & meta XDR, SCP/overlay wire bytes, history archive format, HTTP/RPC/CLI contracts, and crypto outputs. Connect-retry timing has no wire framing and is not part of any of these — it is explicitly on the may-deviate side.
• The random draw gen_range(1..=ceiling) and the ceiling values are unchanged, so even the (non-observable) retry distribution is identical to before consolidation.
• No XDR, hash, archive, RPC, CLI, or crypto path is in the diff.

No parity concerns.

[tomerweller]

Review: Blocked — Unrelated CI Red Persists After Rebase

Reviewer A (Correctness): APPROVE — behavior-preserving dedup of the triplicated backoff schedule into henyey_common::peer_backoff; drift-guard tests present; matches the converged plan.
Reviewer B (Parity): APPROVE — no observable/interop surface touched; sampled distribution bit-for-bit unchanged; retry timing is freely-divergeable per docs/PARITY.md.
External reviewers: none.
CI: red — 40/41 code-level checks SUCCESS/SKIPPED; the sole non-green is test (testnet, core,horizon, …) with conclusion CANCELLED.

Why blocked (not another bounce): Bounce-Back Cycle 1 already bounced this PR to /do for the identical failure — the live-testnet Quickstart integration shard cancelling after ~55 min (environmental testnet-sync timeout, #3272 soft-timeout machinery). /do rebased on origin/main and re-pushed (head do/issue-3871, committed 2026-08-20T03:48:15Z); CI re-ran (Quickstart run 32329802048) and the same test (testnet, core,horizon, …) shard ran 04:06:35Z → 05:01:35Z and was CANCELLED again. The job log confirms the testnet shard times out on testnet sync probe — environmental, not a henyey failure — and this shard did not convert to a soft-skip success.

The diff is a pure internal Rust refactor (extract two constants + a ceiling fn into henyey-common, route three call sites through it). It has zero relation to a live-testnet network probe. Every code-level check is green: Test, Clippy, Doctest, Build (release-ci), CodeQL, and all local/pubnet integration jobs.

Per /review-pr Step 6 (APPROVE | APPROVE | red unrelated → bounce once, then block if still red after rebase), and as Cycle 1 explicitly forewarned, this is now an operator-investigation item: the testnet-probe shard cannot go green through the pipeline's own retry mechanism. Operator options:

• Investigate testnet-probe flakiness / cancellation (workflow timeout budget for the testnet shard), or make that shard soft-skip to success like the sibling shards, then re-trigger CI.
• If the testnet-probe cancellation is confirmed environmental and non-gating, post ## Review: Reset with a one-line reason and re-queue; the code and both reviewers are already green.

Bounce-back count: 0/3 (head-scoped, reset by the fresh push), 1/6 (lifetime).