Recalibrate slow consensus trigger WARN thresholds

[tomerweller]

Closes #3839

Summary

The slow consensus trigger WARN in trigger_next_ledger shared a single 50ms literal across both build_value_ms and nominate_ms. Measured over 115h/74k closes (see issue), that bar sits below build_value_ms's own p50 (~60ms), so the WARN fires on ~9% of ledgers and tracks transaction volume (r=0.931) rather than slowness. This raises the build-value bar to 250ms (above the ~166ms p99) and gives nominate_ms its own independent 50ms bar. The predicate is extracted into a unit-testable pure helper is_slow_consensus_trigger. The nominate bar is decoupled but deliberately kept at 50ms to preserve the genuine sole-trigger nomination stalls (nominate 51-61ms with build_value 4-7ms) documented in the issue.

Plan reference

Converged Plan comment

Test plan

• cargo fmt --check
• cargo clippy --all -- -D warnings
• cargo test -p henyey-herder passes (1235 + new unit tests green)

New unit coverage for is_slow_consensus_trigger: typical p50 stays quiet (60,0), build-value outlier warns (251,0), strict boundary (250,0) quiet, nominate sole-trigger warns (4,53), nominate boundary (0,50) quiet, both-healthy (60,22) quiet.

Deviations from plan

None.

🤖 Generated with Claude Code

[tomerweller]

🔍 Reviewer: Correctness

Verdict: APPROVE

Summary: Clean, well-tested extraction of the slow-consensus-trigger predicate. The build-value bar is raised 50→250ms and nominate gets its own independent 50ms bar, exactly as the converged plan specifies. Strictly-greater semantics are preserved and boundary-tested.

Full review

Cycle 1 — no prior ## 🔍 Reviewer: Correctness comment on this PR, so this is a COMPLETE class-labeled change-list.

Correctness

• The single slow-trigger WARN site (crates/herder/src/herder.rs:3250) is the only place the old build_value_ms > 50 || nominate_ms > 50 predicate lived; it is replaced by is_slow_consensus_trigger(build_value_ms, nominate_ms). The observer-path build_value_ms (line 3129) only feeds a tracing::debug!, not the WARN, so there is no parallel site left un-updated.
• New behavior: build_value_ms > 250 || nominate_ms > 50. Build-value bar raised above the observed p99 (~166ms); nominate bar decoupled and kept at 50ms to preserve genuine sole-trigger nomination stalls. Matches the plan and the issue's measured distributions.
• Strictly-greater (>) semantics preserved on both arms.

test-coverage — the new is_slow_consensus_trigger pure helper has 6 unit tests: typical p50 quiet (60,0), build-value outlier warns (251,0), build-value boundary exclusive (250,0 quiet), nominate sole-trigger warns (4,53), nominate boundary exclusive (0,50 quiet), both-healthy quiet (60,22). Both arms and both boundaries covered; strictly-greater exclusivity asserted. The behavioral regression is captured: test_slow_consensus_trigger_typical_build_value_no_warn asserts !is_slow(60,0), which is exactly the pre-fix false-positive (60 > 50 → warned) that motivated the issue — it would fail against the old logic.

readability / error-handling — the two threshold constants carry thorough rationale comments with issue references; the helper doc explains why the two arms need independent bars. No error-handling surface touched (pure predicate over already-computed millis). Nothing to flag.

No blocking concerns.

[tomerweller]

🔍 Reviewer: Parity

Verdict: APPROVE

Summary: The change touches only WARN-vs-debug log-level selection thresholds in trigger_next_ledger. It does not alter any observable/interop surface (SCP/overlay wire bytes, tx result/meta XDR, hashes, history archive, HTTP/RPC/CLI, crypto). Logging is an explicitly allowed internal deviation per docs/PARITY.md.

Full review

Cycle 1 — no prior ## 🔍 Reviewer: Parity comment on this PR; COMPLETE class-labeled change-list.

Reviewed strictly against the observable/interop surface defined in docs/PARITY.md.

• No observable-surface impact. The diff changes which tracing level (warn! vs debug!) a diagnostic line is emitted at, based on two millisecond thresholds. It does not change nomination inputs, SCP message contents/sequencing, ledger/bucket hashes, tx result/meta XDR, history archive format, or any HTTP/RPC/CLI contract. self.scp.nominate(...) and all control flow around it are unchanged.
• Timing/logging are internal. Metrics, logging, and internal helpers are areas where deviation from stellar-core is explicitly permitted. Extracting the predicate into is_slow_consensus_trigger is an internal refactor with no wire/format effect.
• No parity gap. No change to any file under the observable surface beyond a log threshold.

No blocking concerns.

[tomerweller]

Review: Bounce-Back Cycle 1

Reason: CI failed (unrelated, will rebase)

Reviewer A: APPROVE — clean, well-tested predicate extraction; strictly-greater semantics preserved and boundary-tested.
Reviewer B: APPROVE — parity lens: change is WARN-vs-debug log-level thresholds only; no observable/interop-surface impact.
External reviewers: none
CI: red

Both agent reviewers approve the code. The only blocker is CI: two Quickstart integration jobs failed and the run cannot recover to green:

• test (local, rpc, test_stellar_rpc_up.go test_stellar_rpc_healthy.go test_friendbot.go) — FAILURE
• test (local, core,rpc,horizon, test_core.go test_horizon_up.go test_horizon_core_up.go ...) — FAILURE

These are classified unrelated to the diff:

• The diff touches only crates/herder/src/herder.rs, changing the WARN-vs-debug threshold for the slow consensus trigger diagnostic. There is no control-flow path by which a log-level threshold change can affect whether stellar-rpc / horizon / friendbot containers come up healthy.
• The core-only combos passed on this same run: test (local, core, test_core.go) SUCCESS, test (pubnet, core,rpc,horizon, ...) SUCCESS. Only the rpc/horizon/friendbot external-dependency combos failed — the classically flaky Quickstart integration path.
• (Failed-job logs were not yet retrievable because the overall run still had a testnet job in progress; classification is from diff scope + the passing core combos.)

Routing back to ready-for-doing for /do Mode B to rebase on origin/main and re-trigger a fresh CI run. If the same combos are still red after a clean rebase, the next /review-pr will mark this blocked for operator investigation.

[tomerweller]

Addressed the bounce-back. Both agent reviewers already APPROVED; the sole blocker was CI red on the flaky Quickstart/testnet integration combos (test (local, rpc, ...), test (local, core,rpc,horizon, ...), test (testnet, core,horizon, ...)).

The PR branch is already based on the current origin/main (129190c), so a rebase produces no new commit — there is no code change to make. I re-ran the failed jobs on run 33018467064 to get a fresh CI result. All henyey-relevant jobs (Clippy, Format, Test, Doctest, Build, Agent Worktree Contract) are already green. Ready for re-review once the re-run settles.

[tomerweller]

Addressed the bounce-back. Both agent reviewers already APPROVED; the sole blocker was CI red on flaky Quickstart/testnet integration combos. The PR branch is already on current origin/main (129190c), so a rebase yields no new commit — no code change needed. Re-ran the failed jobs on run 33018467064 for a fresh CI result; all henyey-relevant jobs (Clippy, Format, Test, Doctest, Build) are green. Ready for re-review once the re-run settles.

[tomerweller]

Review: Bounce-Back Cycle 1

Reason: CI failed (unrelated, will rebase)

Reviewer A: APPROVE — clean, well-tested extraction of the slow-consensus-trigger predicate; build-value bar 50→250ms, nominate decoupled at its own 50ms bar; strictly-greater semantics preserved and boundary-tested.
Reviewer B: APPROVE (Parity) — change only affects WARN-vs-debug log-level threshold selection in trigger_next_ledger; no observable/interop-surface impact (logging is an allowed internal deviation per docs/PARITY.md).
External reviewers: none
CI: red

The two failing checks are both quickstart integration shards that timed out on test_friendbot.go (exit 124, 720s probe budget), after the core / horizon / rpc up+healthy probes all passed:

test (local, rpc, ...)             -> test_friendbot.go: === Failed (exit 124), not retryable ===
test (local, core,rpc,horizon,...) -> test_friendbot.go: === Failed (exit 124), not retryable ===

Friendbot is a separate external account-funding service in the quickstart harness; the PR only changes a herder log-emission threshold (crates/herder/src/herder.rs). The failure is not diff-attributable — it is a friendbot-funding timeout flake in the quickstart harness.

Routing back to ready-for-doing for /do Mode B to rebase on origin/main and re-run CI. If the friendbot shards are still red after a clean rebase, the next /review-pr will mark this blocked for operator investigation.

[tomerweller]

Do: Re-review

Both agent reviewers already APPROVED this PR (Correctness + Parity). The only blocker was CI: the two test (local, ...) quickstart Docker integration jobs (test_core.go, test_horizon_up.go, test_stellar_rpc_up.go, etc.) failed on probe timeouts — infrastructure flakiness unrelated to this change, which only touches WARN-vs-debug log-level thresholds in herder::trigger_next_ledger.

The branch head (e6280aa) is already on top of current origin/main (129190c), so there is nothing to rebase and no code change to make. I re-ran the failed jobs on run 33018467064. Ready for re-review once CI is green.

[tomerweller]

Review: Cycle Cap Reached / CI Stuck

Bounce count (head-scoped, actual on PR): 2 → this tick makes the 3rd review of an unchanged head
Status: blocked

Reviewer verdicts (unchanged code, re-affirmed this tick):

• Reviewer A (Correctness): APPROVE — clean extraction of the pure is_slow_consensus_trigger predicate; build-value bar 50→250ms, nominate arm byte-identical (>50); strictly-greater boundary semantics preserved and unit-tested (typical/outlier/boundary/sole-trigger/both-healthy).
• Reviewer B (Parity): APPROVE — the predicate feeds only a tracing::warn! guard in trigger_next_ledger. Pure logging; no observable/interop-surface impact (logging is an allowed internal deviation per docs/PARITY.md). No hash/XDR/wire/consensus-behavior change.

Pattern: Both agent reviewers have APPROVED the code on all three reviews. The sole blocker is CI, which is red on two Quickstart integration shards that both fail on the identical test_friendbot.go timeout (exit 124), after the core / horizon / rpc up+healthy probes pass:

test (local, rpc, ...)              -> test_friendbot.go: === Failed (exit 124), not retryable ===
test (local, core,rpc,horizon, ...) -> test_friendbot.go: === Failed (exit 124), not retryable ===

This failure is not diff-attributable: the PR changes only a herder WARN-emission threshold (crates/herder/src/herder.rs); there is no control-flow path by which a log-level threshold can affect whether the friendbot account-funding service comes up. The core-only and pubnet combos passed on the same run.

Why block instead of bounce again: the PR head has not changed across the two prior bounces (pushed once at 22:05:30Z; no new commit at either 23:33Z or 02:48Z). /do Mode B has no code fix to make — a rebase cannot cure a friendbot-funding infra flake in the Quickstart harness. The prior bounce comment explicitly committed that "if the friendbot shards are still red after a clean rebase, the next /review-pr will mark this blocked for operator investigation." That condition is met. Continuing to bounce is a known runaway loop (which the literal issue-scoped bounce counter fails to catch only because bounce comments are posted on the PR, not issue #3839).

Operator action required: the change itself is merge-ready (triple-would-be-green but for the flake). Please either:

• Re-run / stabilize the flaky test_friendbot.go Quickstart shards and, once CI is green, post ## Review: Reset on herder: 'slow consensus trigger' WARN fires on 7.0% of ledgers — 50ms threshold is below the p50 build_value_ms (60ms), and the nominate_ms arm has never fired #3839 to let the pipeline re-attempt (and merge), or
• Merge manually if the friendbot flake is a known-infra issue.