Restore phase=3 broadcast watchdog stamp on the SCP-relay path

[tomerweller]

Closes #3723

Summary

The event-loop freeze this issue tracks (~101 s on 2026-07-12, phase=3 broadcast) landed in the SCP-relay broadcast path, but on current main that path carries no phase stamp: the generic relay arm (fetching_relay_rx.recv(), the sole relay path for all fetched-ready SCP envelopes) broadcasts without calling set_phase, while the loop top stamps set_phase(0) before every select!. A freeze there is therefore misattributed to phase=0 "waiting", defeating the diagnostics the issue depends on.

This restores the stamp by extracting the arm body into App::relay_ready_scp_envelope (single source of truth, unit-testable without the select! loop). Inside: set_phase(3) then set_phase_sub(PHASE_3_1_OVERLAY_READ) before overlay().await (so even the overlay-unset path stamps a non-zero sub), then set_phase_sub(PHASE_3_2_BROADCAST) + warn_if_slow("scp_relay_broadcast", ...) around overlay.broadcast(...).await. The 3.1/3.2 split lets the next occurrence name whether the loop parked on the overlay RwLock read or inside broadcast. Relay semantics (message construction, henyey_scp_receive_to_relay_seconds guard/label, Failed to relay warn) are moved verbatim — diagnostics only.

Also corrects the stale #3218 cross-reference in the monitor-tick (3e) docs to point at #3723.

Per the plan, this does not eliminate the freeze (an unreproduced 28-day singleton); the confirmed-root-cause remediation is deferred and tracked in #3899.

Plan reference

Converged Plan comment

Test plan

• cargo fmt --check
• cargo clippy --all -- -D warnings
• cargo test -p henyey-app — 1244 lib + integration/doc tests pass

Regression test (kind: bug-fix)

• Test: crates/app/src/app/lifecycle.rs → scp_dedup_pipeline_tests::test_relay_ready_scp_envelope_stamps_broadcast_phase
• Pre-fix: committed as 37e71d7 — verified FAILED (compile error: no method named relay_ready_scp_envelope; the relay arm never stamped phase=3, so the invariant did not hold on main)
• Post-fix: verified PASSES after b3a4cff (asserts phase_snapshot_for_test() == (3, sub) with sub != 0)
• Added test_phase_3_constants_distinct_and_dense mirroring the existing phase-6/13 distinctness tests.

Deviations from plan

None.

🤖 Generated with Claude Code

[tomerweller]

🔍 Reviewer: Correctness

Verdict: APPROVE

Summary: The relay-arm extraction into App::relay_ready_scp_envelope is behavior-preserving; the phase-3 stamps and warn_if_slow timing are diagnostics-only, and the regression test correctly pins phase=(3, non-zero) for the overlay-unset path. One minor doc-comment misplacement noted inline (non-blocking).

Full review

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

Correctness (relay extraction) — the moved arm body is verbatim: slot/received_at/ready_path reads, StellarMessage::ScpMessage construction, if let Some(overlay) = self.overlay().await, the henyey_scp_receive_to_relay_seconds guard/label, and the Failed to relay warn are all preserved. The only additions are set_phase(3), set_phase_sub(PHASE_3_1_OVERLAY_READ) before the overlay guard, set_phase_sub(PHASE_3_2_BROADCAST) inside it, and a warn_if_slow(broadcast_start.elapsed(), ...) around the awaited broadcast (let result = ...; warn_if_slow(...); match result). No await occurs between set_phase(3) and set_phase_sub(1) — only cheap synchronous field reads/enum construction — so the sub-phase is stamped promptly. No behavior change.

test-coverage (bug-fix gate) — kind:bug-fix ⇒ regression test required that provably fails pre-fix. test_relay_ready_scp_envelope_stamps_broadcast_phase builds a fresh App (overlay unset → overlay().await is None), calls relay_ready_scp_envelope, and asserts phase_snapshot_for_test() == (3, non-zero). Pre-fix the method does not exist (compile failure = failing test); the PR records pre-fix commit 37e71d7 as verified-FAILED. Gate satisfied. test_phase_3_constants_distinct_and_dense mirrors the existing phase-6/13 distinctness tests and pins dense 1..=N numbering.

doc-drift (minor, non-blocking — inline) — in crates/app/src/app/mod.rs the new test_phase_3_constants_distinct_and_dense was inserted between test_phase_13_constants_distinct_and_dense's doc comment and its #[test] attribute. Result: the /// All PHASE_13_* ... doc block is now misattached to test_phase_3 (which gets a doubled, self-contradictory doc comment), and test_phase_13_constants_distinct_and_dense is left with no doc comment. Compiles and tests pass — purely cosmetic. APPROVE at top level; flagging so it can be cleaned up on the re-push (see the CI-driven bounce). Suggested fix: move the new test (with its own doc block) to after the closing brace of test_phase_13_constants_distinct_and_dense, or insert it before the phase-13 doc block.

No blocking concerns.

[tomerweller]

🔍 Reviewer: Risk

Verdict: APPROVE

Summary: Diagnostics-only change in a non-parity crate (crates/app). No observable/interop-surface impact, metric preserved verbatim, verbatim relay semantics — negligible regression/operational risk.

Full review

Cycle 1 (no prior Risk verdict on this PR) — complete class-labeled change-list below.

parity-gap (none) — files are under crates/app/ (not scp/herder/ledger/tx/overlay). Per docs/PARITY.md the observable/interop surface is untouched: no ledger/bucket hash, tx result/meta XDR, SCP/overlay wire byte, history-archive, HTTP/RPC/CLI, or crypto change. set_phase/set_phase_sub/warn_if_slow are internal watchdog/logging state, explicitly in the "MAY deviate freely" bucket. The henyey_scp_receive_to_relay_seconds histogram (name, path label values, count > 0 sampling guard) is moved verbatim — no metric-contract drift.

regression-risk (low) — the relay arm body is a verbatim move into a private async method; the only runtime additions are internal phase stamps and a slow-broadcast warn. overlay.broadcast(...).await is now bound to let result before the match, which is semantically identical. Overlay-unset early-return path unchanged (now additionally stamps phase 3.1, which is desired).

operational (positive) — restores the intended phase=3 broadcast watchdog attribution that main had lost, splitting overlay-RwLock-read vs in-broadcast parks. This is the diagnostic the linked incident depends on. Follow-up root-cause work is correctly deferred to #3899.

No blocking concerns.

[tomerweller]

Addressed the Correctness reviewer's non-blocking doc-drift note in 022b660: moved the PHASE_13_* distinctness-test doc block back onto test_phase_13_constants_distinct_and_dense, leaving test_phase_3_constants_distinct_and_dense with only its own doc comment. No behavior change; fmt/clippy/cargo test -p henyey-app phase all green. Ready for re-review.

[tomerweller]

🔍 Reviewer: Correctness

Verdict: APPROVE

Summary: Cycle 2 re-review. The verbatim relay-arm extraction into App::relay_ready_scp_envelope is behavior-preserving, the phase-3 stamps + warn_if_slow are diagnostics-only, and the bug-fix regression test pins phase=(3, non-zero) on the overlay-unset path. The one cycle-1 concern (doc-drift) is resolved. No blocking concerns.

Full review

Cycle N≥2 — prior classes raised in cycle 1: correctness, test-coverage, doc-drift. Sticking to those classes; no NEW CLASS DISCOVERED.

correctness (relay extraction) — the arm body moved into relay_ready_scp_envelope is verbatim: slot/received_at/ready_path reads, StellarMessage::ScpMessage construction, if let Some(overlay) = self.overlay().await, the henyey_scp_receive_to_relay_seconds count>0 guard + immediate|deferred label, and the Failed to relay warn are all preserved. The only additions are set_phase(3), set_phase_sub(PHASE_3_1_OVERLAY_READ) before the overlay guard, set_phase_sub(PHASE_3_2_BROADCAST) inside it, and the warn_if_slow timer. The rewrite of match overlay.broadcast(...).await { .. } into let result = overlay.broadcast(...).await; warn_if_slow(...); match result { .. } is semantically identical — the only interleaved statement is a synchronous Instant::elapsed() + warn. No await occurs between set_phase(3) and the first set_phase_sub, so the sub-phase is stamped promptly. No behavior change.

test-coverage (bug-fix gate) — kind:bug-fix ⇒ regression test that provably fails pre-fix. test_relay_ready_scp_envelope_stamps_broadcast_phase builds a fresh App (overlay unset ⇒ overlay().await is None), calls relay_ready_scp_envelope, and asserts phase_snapshot_for_test() == (3, non-zero). Pre-fix the method does not exist (compile failure = failing test); PR records pre-fix commit 37e71d7 verified-FAILED. Gate satisfied. test_phase_3_constants_distinct_and_dense mirrors the phase-6/13 distinctness tests and pins dense 1..=N numbering (helper max_defined_phase_3_sub_phase() gated #[cfg(test)]).

doc-drift (RESOLVED) — the cycle-1 note (new phase-3 test inserted between test_phase_13's doc block and its #[test]) is fixed in the re-push: test_phase_3_constants_distinct_and_dense now carries its own /// All PHASE_3_* ... doc block and sits before the /// All PHASE_13_* ... block, so the phase-13 doc is correctly reattached to its own test. Confirmed in the current diff (mod.rs).

No blocking concerns.

[tomerweller]

🔍 Reviewer: Risk

Verdict: APPROVE

Summary: Cycle 2 re-review. Diagnostics-only change in a non-parity crate (crates/app). No observable/interop-surface impact, henyey_scp_receive_to_relay_seconds metric moved verbatim, verbatim relay semantics — negligible regression/operational risk. No blocking concerns.

Full review

Cycle N≥2 — prior classes raised in cycle 1: parity-gap (none), regression-risk (low), operational (positive). Sticking to those classes; no NEW CLASS DISCOVERED.

parity-gap (none) — changed files are under crates/app/ (not scp/herder/ledger/tx/overlay). Per docs/PARITY.md the observable/interop surface is untouched: no ledger/bucket hash, tx result/meta XDR, SCP/overlay wire byte, history-archive, HTTP/RPC/CLI, or crypto change. set_phase/set_phase_sub/warn_if_slow are internal watchdog/logging state, explicitly in the "MAY deviate freely" bucket. The henyey_scp_receive_to_relay_seconds histogram (name, path label values, count>0 sampling guard) is moved verbatim — no metric-contract drift.

regression-risk (low) — the relay arm body is a verbatim move into a private async method; the only runtime additions are internal phase stamps and a slow-broadcast warn. The overlay-unset early-return path is unchanged (now additionally stamps phase 3.1, which is the intended fix). let result = ... .await; match result is semantically identical to the prior inline match.

operational (positive) — restores the phase=3 broadcast watchdog attribution main had lost, splitting overlay-RwLock-read (3.1) vs in-broadcast (3.2) parks — the diagnostic the linked incident depends on. Root-cause remediation correctly deferred to #3899. The .claude/skills/monitor-tick/SKILL.md edit is a stale #3218 → #3723 cross-reference fix (docs only; not a gated pipeline path).

No blocking concerns.

[tomerweller]

Review: Bounce-Back Cycle 1

Reason: CI failed (unrelated, will rebase)

Reviewer A (Correctness): APPROVE — verbatim relay extraction, bug-fix regression test pins phase=(3, non-zero), cycle-1 doc-drift resolved.
Reviewer B (Risk): APPROVE — diagnostics-only, non-parity crate, metric moved verbatim; negligible regression/operational risk.
External reviewers: none
CI: red

Both agent reviewers APPROVE and there are no reviewer-blocking concerns. The only red signal is CI, and it is not diff-attributable:

• The sole non-green check is test (testnet, core,horizon, ...) in the Quickstart workflow — conclusion=CANCELLED after ~55 min (started 01:47:33Z, cancelled 02:42:34Z).
• All 40 other checks are green, including CI Test, Build (release-ci), Clippy, Doctest, and every local/pubnet Quickstart integration test.
• The identical test (testnet, ...) job was also cancelled on the immediately preceding run for this branch — a recurring live-testnet infra/timeout dependency, not a regression from this diagnostics-only phase-stamp diff (crates/app only).

Per the /review-pr matrix (APPROVE / APPROVE / CI red-unrelated), routing back to ready-for-doing for /do Mode B to rebase on origin/main and re-trigger CI. No code changes are required — a clean CI run should flip this to a triple-green auto-merge. If the testnet job still cancels after rebase, the next /review-pr will mark this blocked for operator investigation of the flaky testnet check.

Bounce-back count (head-scoped): 0/3. Lifetime: 1/6.

[tomerweller]

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

Addressing the Cycle-1 bounce-back (CI red, unrelated).

• Both agent reviewers APPROVE; no actionable inline review comments since the last push.
• Rebased do/issue-3723 onto origin/main — no-op: main is still at f1aa690, unchanged since the branch was cut, so there is no new commit to push.
• Confirmed the only red check is test (testnet, core,horizon, …) (the live-testnet Quickstart job, fail after 55m); all 40+ other checks — CI Test, Build (release-ci), Clippy, Doctest, Format, and every local/pubnet Quickstart integration test — are green. Not diff-attributable (this PR is a diagnostics-only crates/app phase-stamp change).
• Re-triggered CI via gh run rerun 32542484419 --failed. The testnet job is now re-running (pending).

No code changes required. Routing back to in-review for /review-pr to re-evaluate once CI settles.

[tomerweller]

Re-triggered the flaky live-testnet CI job (no code changes — rebase was a no-op since main is unchanged). Ready for re-review once CI settles.

[tomerweller]

Do: Mode B — no code changes; CI retrigger exhausted

Re-picked from ready-for-doing after the Cycle-1 bounce. Assessment:

• No actionable feedback. Both agent reviewers (Correctness + Risk) APPROVE; there are zero inline review comments since the last push. Nothing to implement.
• Rebase is a no-op. origin/main is still at f1aa690 (unchanged since the branch was cut); PR head 022b660 is MERGEABLE. No new commit to push.
• CI retrigger already exhausted. The only non-green check is test (testnet, core,horizon, …) in the Quickstart workflow. It has now CANCELLED on attempt 3 — the previous Mode B gh run rerun --failed (which reuses the green build artifacts and re-runs only the cancelled testnet job, the cheapest possible retry) cancelled again at the same ~55-min timeout mark (04:02:21Z → 04:57:21Z). A full re-run cannot change live-testnet infra behavior, so it would only burn another ~55 min with no new signal.
• Not diff-attributable. This PR is a diagnostics-only crates/app phase-stamp change; the testnet job is a live-network integration dependency, not a regression from this diff. All 40+ other checks — CI Test, Build (release-ci), Clippy, Doctest, Format, and every local/pubnet Quickstart test — are green.

Per the Cycle-1 bounce-back's own stated policy ("If the testnet job still cancels after rebase, the next /review-pr will mark this blocked for operator investigation of the flaky testnet check"), this is now that case. Routing back to in-review for /review-pr to make the blocked determination — no further /do retrigger loop, since retriggering has been shown not to help.

Co-authored-by: Claude Code claude-code@anthropic.com