Deduplicate watchdog pre-abort threshold derivation

[tomerweller]

Closes #3912

Summary

The pre-abort near-miss threshold formula (abort_threshold_secs * 3 / 4).max(1)-else-0 was duplicated in WatchdogSnapshot::pre_abort_threshold_secs() and emit_watchdog_started_line(), with both documented as needing to stay in sync. This extracts a single private module-level const fn derive_pre_abort_threshold_secs as the source of truth and delegates both call sites to it, removing the drift risk. No behavior change — the rendered field values are byte-identical.

Plan reference

Converged Plan comment

Test plan

• cargo fmt --check
• cargo clippy -p henyey-app --all-targets -- -D warnings
• cargo test -p henyey-app (watchdog suite + new test) passes

Deviations from plan

• The plan's helper body used .max(1), but Ord::max is not yet const-stable, so const fn compilation fails with it. The floor is expressed with an explicit if pre > 1 { pre } else { 1 } instead — same value, still a const fn.
• Added the optional direct const-fn unit test (derive_pre_abort_threshold_secs_formula: 120->90, 0->0, 1->1, 2->1) the plan flagged as a nice-to-have, locking the single source of truth.

🤖 Generated with Claude Code

[tomerweller]

🔍 Reviewer: Correctness

Verdict: APPROVE

Summary: Provably-equivalent DRY refactor extracting a duplicated pre-abort threshold formula into one const fn; new direct unit test pins the single source of truth. No behavior change.

Full review

Cycle 1 (no prior Correctness verdict) — complete review.

Correctness / equivalence

• Original: if x > 0 { (x*3/4).max(1) } else { 0 }. New: if x > 0 { let pre = x*3/4; if pre > 1 { pre } else { 1 } } else { 0 }. Equivalent for all u64: pre ∈ {0,1} → 1, pre ≥ 2 → pre, x == 0 → 0. Ord::max(pre,1) yields the identical mapping.
• Both call sites (WatchdogSnapshot::pre_abort_threshold_secs and emit_watchdog_started_line) now delegate to the shared helper, removing the documented drift risk.
• Deviation from plan (.max(1) → explicit if) is justified: Ord::max is not const-stable; the explicit form keeps the fn const and is value-identical.

Test coverage (kind: refactor / cleanup)

• New derive_pre_abort_threshold_secs_formula covers the boundary cases the drift risk lived at: 120→90 (normal), 0→0 (disabled), 1→1 and 2→1 (the #3767 degenerate-floor cases). Existing watchdog suite remains green in CI, exercising both delegating call sites.

Readability / error handling

• Doc comment on the new helper is accurate and cross-references both call sites and the relevant issues. No error paths involved (pure integer fn).

No concerns.

[tomerweller]

🔍 Reviewer: Risk

Verdict: APPROVE

Summary: Pure internal refactor of a single non-parity crate (crates/app); no observable-surface, API, data-format, security, or operational change. Rendered field values are byte-identical.

Full review

Cycle 1 (no prior Risk verdict) — complete review.

Regression risk: None. The extracted const fn reproduces the exact prior arithmetic; both call sites produce identical outputs. The boot-diagnostics INFO line and the runtime pre-abort tier emit the same values as before.

Perf: Negligible/none — a const fn over u64, if anything more optimizable.

API / data-format: No public API change. derive_pre_abort_threshold_secs is private (const fn, module-private); the two touched fns retain pub(crate) visibility. No wire/XDR/HTTP/CLI surface touched.

Parity note: crates/app is not a parity-critical crate; the watchdog threshold is internal observability (logging/diagnostics), explicitly outside the observable/interop surface per docs/PARITY.md.

Security / operational: None.

No concerns.

[tomerweller]

Review: Bounce-Back Cycle 1

Reason: CI failed (unrelated, will rebase)

Reviewer A: APPROVE — provably-equivalent DRY refactor; boundary cases pinned by a new direct unit test.
Reviewer B: APPROVE — pure internal refactor of a non-parity crate; no observable-surface/API/data-format/security change, byte-identical rendered values.
External reviewers: none
CI: red

Both agent reviewers approve. The only non-green check is a single environmental cancellation, unrelated to this Rust-only diff:

• CANCELLED :: test (testnet, core,horizon, test_core.go test_horizon_up.go ...) — the Quickstart testnet Go integration shard ran ~55 min against live testnet and was cancelled by its per-shard timeout budget. The harness itself classifies testnet sync-probe timeouts as "environmental, not a henyey failure" (CI: Quickstart testnet shard cancelled on ≥2 consecutive main commits — probe wrapper hangs at step 8 (systematic, not flaky) #3272). The diff touches only crates/app/src/app/mod.rs (a const fn extraction with no wire/RPC/CLI surface), so this cannot be diff-attributable.
• All 40 other checks — CI (Test, Clippy, Doctest, Build), CodeQL, Quickstart local/pubnet shards, contract checks — are SUCCESS.

Routing back to ready-for-doing for /do Mode B to rebase on origin/main and re-trigger CI. The refactor itself needs no code changes; a fresh CI run should clear the transient testnet cancellation. If CI is still red after rebase, the next /review-pr will mark this blocked.

[tomerweller]

Do: Mode B — CI re-trigger (no code change)

Both agent reviewers APPROVED. The only red check was the test (testnet, core,horizon, ...) Quickstart shard, which ran ~55 min against live testnet and hit its per-shard timeout — the environmental testnet sync-probe cancellation class (#3272), not diff-attributable (this PR touches only a const fn extraction in crates/app, no wire/RPC/CLI surface).

Rebase onto origin/main is a no-op — main has not moved since the branch was cut, so there is nothing to rebase and no code change is warranted. Re-ran the failed testnet job in run 32602231531 to clear the transient cancellation. All 40 other checks are green.

Ready for re-review.

[tomerweller]

Do: Re-trigger CI (Mode B, bounce cycle 1)

Both agent reviewers APPROVEd; the only non-green check was the environmental testnet Quickstart shard cancellation (unrelated to this crates/app const fn refactor).

• Branch do/issue-3912 (108bdf6) is already on top of latest origin/main (5ac2f16) — rebase was a no-op, no code change required.
• Re-ran the failed jobs of workflow run 32602231531 to clear the transient testnet cancellation; the testnet shard is running again.

Ready for re-review once CI settles.

[tomerweller]

Addressed bounce reason (re-triggered transient testnet CI; no code change needed). Ready for re-review.

[tomerweller]

Review: Unrelated CI Red — Blocked

Reviewer A (Correctness): APPROVE — const fn extraction is a faithful, behavior-preserving dedup; new direct unit test locks the single source of truth.
Reviewer B (Risk): APPROVE — no observable/interop surface touched (crates/app only); no regression, perf, or API-shape risk.
CI: red — 40/41 checks green; the sole failure is the test (testnet, core,horizon, ...) Quickstart shard.

Why blocked (not bounced again): The failing shard is environmental, not diff-attributable. It was killed by The runner has received a shutdown signal ... exit code 143 while waiting for live-testnet Horizon/stellar-core to sync — the #3272 testnet sync-probe cancellation class. This PR only extracts a private const fn in crates/app/src/app/mod.rs with no quickstart/wire/RPC/CLI surface, so the diff cannot have caused it.

This same shard already triggered Bounce-Back Cycle 1, after which /do Mode B re-triggered CI (rebase onto origin/main is a no-op — main has not moved since the branch was cut), and the shard failed again for the same environmental reason. Per the unrelated-CI-red rule, the pipeline's one retry has been consumed and CI is still red, so this /review-pr tick marks the issue blocked rather than looping /do on an infra failure it cannot fix.

To clear: an operator can re-run only the failed testnet shard (run 32602231531), or post ## Review: Reset with a one-line reason to reset the bounce counters and let the pipeline re-attempt once testnet infra is healthy. No code change is warranted.