Deduplicate watchdog pre-abort threshold derivation (0.75x floored)

[tomerweller]

Follow-up from PR #3911 (issue #3767). Non-blocking simplification concern raised by the correctness reviewer; reviewer APPROVEd rather than requesting changes, so filing as backlog.

Concern

crates/app/src/app/mod.rs — the pre-abort threshold derivation (abort_threshold_secs * 3 / 4).max(1) (0.75x floored at 1, 0 when disabled) is duplicated in two places:

• WatchdogSnapshot::pre_abort_threshold_secs() (method)
• emit_watchdog_started_line() (free fn, boot-diagnostics line)

Both are documented as needing to stay in sync and both are test-pinned (the boot-line test asserts pre=90 for abort=120), so there is no current drift. Extracting a single shared const fn (e.g. derive_pre_abort_threshold(abort: u64) -> u64) called from both sites would remove the drift risk entirely.

Severity

Low / non-blocking — maintainability nit, no behavioral defect.

[tomerweller]

Triage Report

Verdict: ACCEPT

Type: enhancement
Kind: refactor
Severity: low
Crate: crate:app
Size: S

Summary: Extract the duplicated pre-abort threshold formula (abort_threshold_secs * 3 / 4).max(1) to a shared const fn in crates/app/src/app/mod.rs. The formula appears in both WatchdogSnapshot::pre_abort_threshold_secs() and emit_watchdog_started_line() with existing test pinning; consolidation removes drift risk.

Test obligation: Existing tests must keep passing; no new tests required. The boot-line test already asserts the pre/abort relationship (pre=90 for abort=120).

[tomerweller]

📝 Plan Draft (Round 1)

Summary: Extract the duplicated (abort * 3 / 4).max(1)-else-0 pre-abort threshold formula into a single module-level const fn and call it from both WatchdogSnapshot::pre_abort_threshold_secs() and emit_watchdog_started_line(), removing the documented drift risk.

Files to modify:

• crates/app/src/app/mod.rs — add const fn derive_pre_abort_threshold_secs(abort_threshold_secs: u64) -> u64; call it from both existing sites (method at ~L4909 and free fn at ~L5007).

Approach:
Introduce one private module-level const fn near the two existing definitions in crates/app/src/app/mod.rs:

/// Derive the pre-abort near-miss threshold in seconds from the armed abort
/// threshold: 0.75× floored at 1s, or 0 when auto-abort is disabled
/// (`abort_threshold_secs == 0`). The `.max(1)` floor prevents a degenerate
/// trip at `stale_secs == 0` for tiny abort thresholds (1–3), where integer
/// `* 3 / 4` would round to 0 (#3767). Single source of truth for both the
/// runtime tier ([`WatchdogSnapshot::pre_abort_threshold_secs`]) and the boot
/// diagnostics line ([`emit_watchdog_started_line`]).
const fn derive_pre_abort_threshold_secs(abort_threshold_secs: u64) -> u64 {
    if abort_threshold_secs > 0 {
        (abort_threshold_secs * 3 / 4).max(1)
    } else {
        0
    }
}

Then replace the two inline copies:

• WatchdogSnapshot::pre_abort_threshold_secs() body becomes derive_pre_abort_threshold_secs(self.abort_threshold_secs).
• emit_watchdog_started_line() computes let pre_abort_threshold_secs = derive_pre_abort_threshold_secs(abort_threshold_secs);.

Keep WatchdogSnapshot::pre_abort_threshold_secs() as the pub(crate) method (it is called in should_warn_pre_abort() and emit_pre_abort_warn()), delegating to the new helper — so no call-site churn beyond the two formula copies. const fn is correct here (no allocation/floats) and lets it be usable in const contexts if ever needed. Both callers are in the same module, so the helper stays private with no visibility concerns.

Test plan:

• Regression tests: n/a (kind: refactor, not bug-fix). The formula is unchanged; no new behavior.
• New coverage: none required — the helper is exercised transitively by existing pinned tests. Optionally add a tiny #[test] asserting derive_pre_abort_threshold_secs(120) == 90, (0) == 0, (1) == 1, (2) == 1 to lock the const fn directly (nice-to-have; the existing tests already cover these values).
• Existing tests preserved (all in cargo test -p henyey-app):
  • watchdog_pre_abort_threshold_routing (asserts pre_abort_threshold_secs() == 90 for abort=120, == 0 when disabled) — L9124.
  • the boot-line test asserting pre_abort_threshold_secs=90 for emit_watchdog_started_line(120) — L9262, and =0 for (0) — L9294.
  • the emit_pre_abort_warn field test asserting pre_abort_threshold_secs=90 — L9228.

Parity considerations:
n/a — does not change the observable surface. Watchdog logging is internal operator diagnostics (tracing INFO/WARN lines), not part of the ledger/XDR/SCP/overlay/archive/RPC surface in docs/PARITY.md. The rendered log field values are byte-for-byte identical before and after (same formula).

Risks:
Minimal. The only risk is a typo in the extracted formula; the existing pinned tests (pre=90 for abort=120, 0 when disabled) catch any divergence. Under -Dwarnings the helper must have a live caller — it has two, so no dead_code risk.

[tomerweller]

🔍 Critic A (correctness) — Round 1

Verdict: APPROVE

Key concerns:

• Extracted const fn body is byte-identical to both inline copies (if abort>0 { (abort*3/4).max(1) } else { 0 }) — no logic change; verified against L4909–4915 and L5008–5012.
• Kind is refactor, so no regression test is required; the existing pinned tests (watchdog_pre_abort_threshold_routing L9124, boot-line tests L9262/L9294, emit_pre_abort_warn field test L9228) fully cover the 90/0/1 values and will catch any transcription typo.
• No visibility narrowing and no dead-code removal — the helper is newly added with two live in-module callers, so the -Dwarnings/dead_code gate is satisfied; grep-vs-build concern is n/a here.
• Keeping pre_abort_threshold_secs() as the pub(crate) method delegating to the helper preserves its two internal callers (should_warn_pre_abort, emit_pre_abort_warn) with zero call-site churn.

Full review

The plan is a pure consolidation. The new const fn derive_pre_abort_threshold_secs reproduces the exact conditional and arithmetic of both existing sites. const fn is valid (only integer ops, no floats/alloc). Both replacement sites feed the same input (self.abort_threshold_secs / the abort_threshold_secs param) so semantics are identical. The optional direct unit test on the const fn (120→90, 0→0, 1→1, 2→1) is a reasonable nice-to-have but not required since existing tests transitively pin these. No edge cases are lost. Sound.

[tomerweller]

🔍 Critic B (parity) — Round 1

Verdict: APPROVE

Key concerns:

• Change touches only watchdog operator-diagnostics logging (tracing INFO/WARN), which is not part of the observable/interop surface in docs/PARITY.md (ledger/bucket hashes, result/meta XDR, SCP/overlay wire, archive format, HTTP/RPC/CLI, crypto).
• The rendered pre_abort_threshold_secs log-field values are byte-identical before/after because the extracted formula is unchanged.
• No stellar-core counterpart exists for this henyey-specific watchdog; nothing to mirror.

Full review

This is an internal refactor of a henyey-only diagnostic. No serialization, hashing, wire bytes, or RPC/CLI contract is affected. The plan's "n/a — does not change the observable surface" is correct. Approve.

[tomerweller]

🔍 Critic C (scope) — Round 1

Verdict: APPROVE

Key concerns:

• Single atomic concern (deduplicate one formula), one file, well under ~500 lines net change — "just right."
• Addresses the root cause named in the issue (two-site drift risk) directly via a single source of truth, not a band-aid.
• No unrelated changes bundled in; the optional const-fn unit test stays within scope.

Full review

Scope matches the triage sizing (S). The plan does exactly what the follow-up issue asks and nothing more. No split-out or expansion needed. Approve.

[tomerweller]

✅ Converged Plan

Summary: Extract the duplicated (abort * 3 / 4).max(1)-else-0 pre-abort threshold formula into a single module-level const fn and call it from both WatchdogSnapshot::pre_abort_threshold_secs() and emit_watchdog_started_line(), removing the documented drift risk.

Files to modify:

• crates/app/src/app/mod.rs — add a private const fn derive_pre_abort_threshold_secs(abort_threshold_secs: u64) -> u64; call it from the method (~L4909) and the free fn (~L5007).

Approach:
Add one private module-level helper near the two existing definitions:

/// Derive the pre-abort near-miss threshold in seconds from the armed abort
/// threshold: 0.75× floored at 1s, or 0 when auto-abort is disabled
/// (`abort_threshold_secs == 0`). The `.max(1)` floor prevents a degenerate
/// trip at `stale_secs == 0` for tiny abort thresholds (1–3) (#3767). Single
/// source of truth for both the runtime tier
/// ([`WatchdogSnapshot::pre_abort_threshold_secs`]) and the boot diagnostics
/// line ([`emit_watchdog_started_line`]).
const fn derive_pre_abort_threshold_secs(abort_threshold_secs: u64) -> u64 {
    if abort_threshold_secs > 0 {
        (abort_threshold_secs * 3 / 4).max(1)
    } else {
        0
    }
}

Replace the two inline copies: WatchdogSnapshot::pre_abort_threshold_secs() returns derive_pre_abort_threshold_secs(self.abort_threshold_secs); emit_watchdog_started_line() computes let pre_abort_threshold_secs = derive_pre_abort_threshold_secs(abort_threshold_secs);. Keep pre_abort_threshold_secs() as the pub(crate) method (still called by should_warn_pre_abort and emit_pre_abort_warn), now delegating — so no call-site churn beyond the two formula bodies. const fn is valid (integer-only). Both callers are in the same module, so the helper stays private and has two live callers (no dead_code under -Dwarnings).

Kind: refactor (from triage)

Test plan:

• Regression tests: n/a (refactor; formula unchanged, no behavior change).
• New coverage: optional nice-to-have — a small #[test] pinning derive_pre_abort_threshold_secs: 120→90, 0→0, 1→1, 2→1. Not required (existing tests transitively pin these).
• Existing tests preserved (all cargo test -p henyey-app): watchdog_pre_abort_threshold_routing (L9124), the boot-line tests asserting pre_abort_threshold_secs=90 for emit_watchdog_started_line(120) (L9262) and =0 for (0) (L9294), and the emit_pre_abort_warn field test asserting pre_abort_threshold_secs=90 (L9228).

Parity considerations:
n/a — does not change the observable surface. Watchdog tracing lines are internal operator diagnostics, not in docs/PARITY.md. Rendered field values are byte-identical before/after (same formula).

Minor items to consider during implementation:

• Consider adding the optional direct const-fn unit test to lock the single source of truth.
• Run cargo build -p henyey-app --all-targets + workspace build under -Dwarnings to confirm the helper is not flagged dead (it has two live callers).

Convergence: Round 1, verdicts: A=APPROVE, B=APPROVE, C=APPROVE.