Skip to content

Record checkpoint-phase diagnostics at recovery stall onset - #3904

Open
tomerweller wants to merge 2 commits into
mainfrom
do/issue-3902
Open

Record checkpoint-phase diagnostics at recovery stall onset#3904
tomerweller wants to merge 2 commits into
mainfrom
do/issue-3902

Conversation

@tomerweller

Copy link
Copy Markdown
Collaborator

Refs #3902

Intentionally uses Refs, not Closes — per the converged plan (Critic C), this PR ships diagnostic instrumentation only; the underlying critical near-tip-stall mechanism is still unestablished, so #3902 must remain open to track the follow-up mechanism fix once these fields confirm or kill the phase-lock hypothesis on live data.

Summary

Adds two checkpoint-phase diagnostic fields to the once-per-episode "Recovery stall onset — diagnostic snapshot" INFO log in crates/app/src/app/consensus.rs, via two file-scope pure helpers:

The archive value is read via the same non-blocking parking_lot cache read (get_cached_archive_checkpoint_nonblocking) the recovery routing already performs — no .await, no recovery-state side effect, safe on the event loop. This turns the checkpoint-boundary phase-lock and the archive-behind window into queryable log fields, so the long/short stall split can be confirmed or killed from a single day of live data instead of a 306 h offline log reconstruction (exactly the cheap next step the issue requests).

Plan reference

Converged Plan comment

Test plan

  • cargo fmt --check
  • cargo clippy --all -- -D warnings
  • cargo test -p henyey-app passes (lib + integration + doc-tests, exit 0)

Regression test (kind: bug-fix)

Deliverable is diagnostic, so the tests target the new pure phase-math helpers (a deterministic 340 s-stall reproduction is not writable while the mechanism is unestablished — see the plan's Risks).

  • Tests: crates/app/src/app/consensus.rs::test_ledgers_past_checkpoint_boundary_and_window, test_ledgers_past_checkpoint_period_tracks_frequency, test_ledgers_past_checkpoint_before_first_checkpoint, test_archive_checkpoint_lag_known_and_cold
  • Pre-fix: committed as 5e79d36 — verified FAILED (helpers do not exist: unresolved import super::ledgers_past_checkpoint / super::archive_checkpoint_lag)
  • Post-fix: verified PASS after 2d95539 (4/4 pass)

Parity considerations

n/a — no observable-surface change. Two fields on a henyey-only diagnostic INFO log (stellar-core emits no equivalent line); no hash, result/meta XDR, SCP/overlay wire, archive format, HTTP/RPC/CLI, or crypto output touched. Checkpoint math reads checkpoint_frequency()/checkpoint_containing() live for cadence parity.

Deviations from plan

  • test_ledgers_past_checkpoint_accelerated_frequencytest_ledgers_past_checkpoint_period_tracks_frequency. The plan proposed setting set_checkpoint_frequency(8) "restored via a guard". That is not safely writable: set_checkpoint_frequency is backed by a process-global OnceLock (first-write-wins, never resettable), so mutating it from a unit test cannot be guaranteed to take effect and would poison every other checkpoint-math test in the shared --lib binary. The replacement test proves frequency-coupling deterministically instead: it asserts the phase resets to 0 exactly every checkpoint_frequency() ledgers, referencing the live frequency symbolically (never the literal 64). Frequency-awareness is additionally guaranteed by construction — the helper delegates to the already-tested latest_checkpoint_before_or_at and contains no numeric literal.

🤖 Generated with Claude Code

Tomer Weller and others added 2 commits August 22, 2026 06:20
Add unit tests for the checkpoint-phase onset diagnostic helpers
(ledgers_past_checkpoint, archive_checkpoint_lag). They fail to compile
on main because the helpers do not yet exist, capturing the missing
instrumentation the issue requests.

Refs #3902

Co-authored-by: Claude Code <claude-code@anthropic.com>
Add two file-scope pure helpers in consensus.rs and wire their output
into the once-per-episode "Recovery stall onset — diagnostic snapshot"
INFO log:

- ledgers_past_checkpoint(current_ledger): ledgers elapsed since the most
  recent checkpoint boundary (boundary ledger -> 0), frequency-aware via
  latest_checkpoint_before_or_at (correct under accelerated freq 8, not a
  hardcoded % 64). The #3902 phase-lock band is 0..=7.
- archive_checkpoint_lag(next_cp, archive_latest): signed gap between the
  next checkpoint and the archive's latest known checkpoint; positive =
  archive behind the unpublished checkpoint, cold cache -> -1 sentinel.

The archive value comes from the same non-blocking parking_lot cache read
the recovery routing already performs (no .await, no recovery-state side
effect). This turns the checkpoint-boundary phase-lock and the archive-
behind window into queryable log fields, so the long/short stall split can
be confirmed or killed from a single day of live data instead of a 306 h
offline log reconstruction. Diagnostic instrumentation only; no observable
surface changes (parity-neutral).

Refs #3902

Co-authored-by: Claude Code <claude-code@anthropic.com>
@tomerweller tomerweller added the pdr-managed PR opened by the henyey project-tick pipeline /do skill label Aug 22, 2026
@tomerweller

Copy link
Copy Markdown
Collaborator Author

🔍 Reviewer: Correctness

Verdict: APPROVE

Summary: Two pure checkpoint-phase helpers plus four INFO-log fields at recovery-stall onset; helpers are correct, symbols resolve, and the regression tests cover the phase math with a documented pre-fix failing commit.

Full review

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

Correctness — verified, no findings:

  • ledgers_past_checkpoint(current_ledger) delegates to henyey_history::checkpoint::latest_checkpoint_before_or_at and subtracts the boundary. Semantics match the doc and the issue's own values: latest_checkpoint_before_or_at(64066751)=64066751 → phase 0 (boundary ledger); 64066755 → 4. The unwrap_or(current_ledger) pre-first-checkpoint branch cannot underflow (returns the raw ledger when seq < freq-1). The 0..=7 band ⟺ lcl mod 64 ∈ {0..6, 63} claim is arithmetically correct (boundary ≡ 63 mod 64 is phase 0; mod64 0..6 are phases 1..7).
  • archive_checkpoint_lag(next_cp, archive_latest) computes next_cp as i64 - latest as i64 (i64 avoids wraparound on archive-ahead) and maps None-1. The -1 sentinel is collision-safe: both operands are checkpoint boundaries, so any genuine lag is a multiple of checkpoint_frequency() (64 or 8) and can never equal -1.
  • next_cp = checkpoint_containing(current_ledger + 1) in the diagnostic snapshot is byte-identical to the routing's own definition at consensus.rs:1963, so the logged field matches what the recovery path acts on downstream (PR body claim confirmed). checkpoint_containing resolves via pub use checkpoint_ledger as checkpoint_containing.
  • Archive read via get_cached_archive_checkpoint_nonblocking() (defined in catchup_impl.rs:748) matched on Fresh|Stale|Cold — the same non-blocking parking_lot read pattern the routing already uses; no .await, no recovery-state side effect.

Test-verification gate (kind: bug-fix): satisfied. Four helper tests (test_ledgers_past_checkpoint_boundary_and_window, _period_tracks_frequency, _before_first_checkpoint, test_archive_checkpoint_lag_known_and_cold) exercise boundary/window/underflow/frequency-coupling/cold-cache paths with real mainnet-shaped values. PR body documents pre-fix commit 5e79d36 verified FAILED (unresolved imports) and post-fix 2d95539 PASS. The frequency test asserts the phase resets every checkpoint_frequency() symbolically rather than mutating the process-global OnceLock — the correct call, and the deviation is documented. A deterministic 340 s-stall repro is legitimately not writable while the mechanism is unestablished (this PR is diagnostic-only).

Visibility/dead-code gate: both helpers are fn (private, file-scope) and each has a live caller in the snapshot block; not dead. Green Clippy/Build/Doctest confirm compilation.

No blocking concerns.

@tomerweller

Copy link
Copy Markdown
Collaborator Author

🔍 Reviewer: Risk

Verdict: APPROVE

Summary: Diagnostic-only change on the consensus recovery path — adds two log fields and two pure helpers, no control-flow or observable-surface change; event-loop-safe non-blocking cache read.

Full review

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

Reviewer B lens = Risk (non-parity): the only changed file is crates/app/src/app/consensus.rs, which is not under any parity-critical prefix (crates/{scp,herder,ledger,tx,overlay}). Note the issue carries a crate:herder label, but Step 3 keys on changed paths, not labels — the diff is in crates/app.

Regression risk — none identified: the change is additive. It introduces two file-scope pure functions and four fields on the once-per-episode "Recovery stall onset — diagnostic snapshot" INFO line. No recovery decision, control flow, or state transition is altered.

Operational / event-loop safety: the archive read uses get_cached_archive_checkpoint_nonblocking() (non-blocking parking_lot read), matching the pattern the routing already performs a few hundred lines down; no .await, no lock held across suspension, no recovery-state mutation. Safe on the async consensus loop.

Performance: the fields are computed once per stall-onset episode (rate-limited by RECOVERY_STALL_ONSET_TOTAL increment site), not per ledger or per SCP message — negligible.

Parity / observable surface: n/a. This is a henyey-only diagnostic INFO log; stellar-core emits no equivalent line. No ledger/bucket hash, tx result/meta XDR, SCP/overlay wire bytes, history archive format, HTTP/RPC/CLI contract, or crypto output is touched. Checkpoint math reads checkpoint_frequency()/checkpoint_containing() live, so cadence stays correct under accelerated frequency.

Security: no new inputs, no unsafe, no external I/O.

No blocking concerns.

@tomerweller

Copy link
Copy Markdown
Collaborator Author

Review: Waiting on CI

Both agent reviewers APPROVE (Correctness, Risk) and no external reviewer has requested changes. CI is still running (checks in progress include: Test, and the test (local|testnet|pubnet, ...) integration matrix). 0 failing so far.

Not merging yet — auto-merge requires triple-green. Re-picking this PR on the next tick to verify CI completion.

CI age: 12 min / budget 60 min.
Bounce-back count: 0/3.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pdr-managed PR opened by the henyey project-tick pipeline /do skill

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant