fix(node): use readiness for peer liveness

[MikeTomlin19]

Summary

Use the database-aware /ready endpoint for periodic and manual peer checks, while falling back to /health only when an older peer returns 404 for /ready. Periodic federation state now requires two consecutive failed samples before marking a peer unreachable; the anonymous manual-ping endpoint is read-only.

Motivation & context

/health intentionally reports process liveness and remains successful during a database outage. Using it for peer reachability lets a node continue treating a peer as healthy even though that peer cannot serve database-backed requests. A single transient readiness failure also should not remove a peer from federated repository results for the next five-minute interval.

Closes #176

Kind of change

• Bug fix
• Feature
• Security fix
• Docs
• Tests / CI
• Refactor (no behavior change)
• Breaking or protocol change (issue required first)

What changed

• Changed the shared peer probe from /health to /ready, with a 404-only /health fallback for nodes released before /ready existed.
• Reused that helper from the manual peer-ping API so both paths have identical readiness and no-redirect behavior.
• Kept the anonymous manual-ping endpoint read-only; only the periodic gossip loop updates persisted federation reachability.
• Required two consecutive periodic failures before marking a peer unreachable, while restoring reachability immediately after a successful sample.
• Added structured logs for readiness failures, legacy fallback, fallback failures, transport failures, and timeout exhaustion.
• Added regression coverage proving the manual handler uses readiness without mutating the federation gate, one transient failure preserves the gate, sustained failure clears it, success restores it, legacy peers remain compatible, and neither request path follows redirects.
• Updated the operator guide and degraded-router comment to distinguish liveness from readiness.

How a reviewer can verify

DATABASE_URL=postgres://postgres:postgres@localhost:5432/gitlawb_test cargo test --workspace
cargo fmt --all -- --check
cargo clippy --workspace --all-targets -- -D warnings
cargo build --release --workspace

The focused regressions can also be run with:

cargo test -p gitlawb-node gossip_ssrf_tests
DATABASE_URL=postgres://postgres:postgres@localhost:5432/gitlawb_test \
  cargo test -p gitlawb-node manual_ping_uses_readiness_without_mutating_federation_gate

Before you request review

• Scope is one logical change; no unrelated churn
• cargo test --workspace passes locally
• New behavior is covered by tests (required for fixes)
• cargo fmt --all and cargo clippy --workspace --all-targets -- -D warnings are clean
• Commit titles use Conventional Commits (feat(...), fix(...), docs(...))
• Docs / .env.example updated if behavior or config changed (N/A)
• Checked existing PRs so this isn't a duplicate

Protocol & signing impact

N/A — this does not change identity, signatures, UCANs, ref certificates, or P2P wire formats.

Notes for reviewers

The repository's current MSRV lane fails before compilation because newly resolved AWS crates require Rust 1.91.1 while CI installs 1.91 (currently 1.91.0). That existing manifest/lock/dependency drift is tracked in #242 and #243; stable tests, clippy, formatting, and the release build are clean for this change.

Summary by CodeRabbit

• New Features
  • Peer monitoring now checks readiness, including database-aware status, rather than process liveness alone.
  • Legacy peers without a readiness endpoint continue to be supported through a safe health-check fallback.
• Bug Fixes
  • Improved handling of failed, unavailable, non-ready, and redirected peer checks.
• Documentation
  • Operational guidance now requires both /health and /ready endpoints for public URLs.

[coderabbitai]

Warning

Review limit reached

@MikeTomlin19, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 54 minutes

Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab.

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 739c6079-14a9-43ce-ad4a-7bdbd728c095

📥 Commits

Reviewing files that changed from the base of the PR and between 9ace2c6 and e8a000c.

📒 Files selected for processing (1)

• crates/gitlawb-node/src/main.rs

📝 Walkthrough

Walkthrough

Peer liveness checks now use DB-aware /ready semantics, fall back to /health for legacy peers, share timeout budgets, and preserve redirect protections. Gossip checks require two consecutive failures before persistence changes, while API pings use the shared helper.

Changes

Peer readiness probing

Layer / File(s)	Summary
Readiness probe and validation crates/gitlawb-node/src/main.rs	The shared probe requests /ready, treats only 2xx responses as ready, falls back to /health on 404, shares timeout handling, rejects redirects, and maps failures to false.
Peer check integration crates/gitlawb-node/src/main.rs, crates/gitlawb-node/src/api/peers.rs	Gossip checks and API peer pings use the readiness helper; gossip persistence requires two consecutive failures, and manual pings do not rewrite the federation gate.
Operational readiness contract crates/gitlawb-node/src/main.rs, docs/RUN-A-NODE.md	Degraded-server documentation covers /health and /ready, and the public URL checklist requires both endpoints with DB-aware readiness.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant GossipTask
  participant ping_peer_readiness
  participant PeerReadyEndpoint
  participant PeerHealthEndpoint
  participant PeerDatabase
  GossipTask->>ping_peer_readiness: Probe peer URL
  ping_peer_readiness->>PeerReadyEndpoint: GET /ready
  PeerReadyEndpoint-->>ping_peer_readiness: Readiness response
  ping_peer_readiness->>PeerHealthEndpoint: GET /health after 404
  PeerHealthEndpoint-->>ping_peer_readiness: Legacy response
  ping_peer_readiness-->>GossipTask: Ready boolean
  GossipTask->>PeerDatabase: Persist after success or second failure

Loading

Possibly related PRs

• Gitlawb/node#78 — Related peer ping redirect and SSRF handling.
• Gitlawb/node#140 — Related shared client, ping helper, and redirect protections.
• Gitlawb/node#170 — Adds the node-side /ready endpoint used by this probe.

Suggested reviewers: jatmn, kevincodex1, beardthelion

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title is concise and accurately summarizes the main change: switching peer checks to readiness.
Description check	✅ Passed	The description matches the template and covers summary, motivation, change details, verification, and review notes.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[github-actions]

Thanks for the contribution. A couple of things will help us review this faster:

• This changes Rust source but no tests changed. Tests are required for fixes and strongly encouraged for features.

See CONTRIBUTING.md. Update the PR and these notes will clear automatically.

[MikeTomlin19]

For reviewer context, the needs-tests signal is a triage false positive rather than missing coverage.

This PR adds regression coverage inside the existing #[cfg(test)] module in crates/gitlawb-node/src/main.rs, using Tokio's #[tokio::test] attribute. In particular, ping_peer_readiness_ignores_liveness_only_health proves that 200 /health plus 503 /ready is treated as unready and that /health is never requested.

The triage workflow currently recognizes newly added literal #[test] and #[cfg(test)] lines, but not #[tokio::test], so it misses these inline async tests.

Validation completed locally:

• cargo test -p gitlawb-node gossip_ssrf_tests — 4 passed
• cargo test --workspace against PostgreSQL 16 — passed
• cargo fmt --all -- --check — passed
• cargo clippy --workspace --all-targets -- -D warnings — passed

[jatmn]

I found issues that need to be addressed before this is ready.

Findings

• [P1] Preserve compatibility with peers that do not implement /ready
crates/gitlawb-node/src/main.rs:985
  This unconditionally changes the peer wire contract from /health to /ready. Released v0.5.0 nodes expose /health but not /ready, and #170 deliberately left the peer probe on /health for exactly that rolling-upgrade case. Their /ready response is therefore 404, which this helper maps to false; both the periodic job and the manual ping persist that value. Since federated repository listing filters on last_ping_ok, a healthy v0.5.0 peer disappears from federation after the first probe. Please add a version/capability transition (for example, a 404-only legacy /health fallback that still treats a real /ready 503 as unready) and cover the mixed-version case, or provide the required coordinated rollout and compatibility window.

• [P2] Update the operator peer-endpoint requirement
  docs/RUN-A-NODE.md:180
  The operator guide currently says that serving /health is sufficient because peers ping it. After this change peers instead require /ready, so an operator following the documented contract can be reported unreachable. Update the guidance to require /ready (and retain /health only for its liveness purpose).

• [P3] Correct the degraded-router peer-ping comment
  crates/gitlawb-node/src/main.rs:735
  This comment still says peer pings use a successful /health response. The changed callers now use /ready, so the comment gives the wrong rationale for the degraded response and is likely to mislead future outage-routing work.

[MikeTomlin19]

Addressed all three findings in f9b4ce3:

• /ready now falls back to /health only on a 404, preserving compatibility with pre-/ready nodes while keeping real readiness failures such as 503 fail-closed.
• Added mixed-version coverage (404 /ready + 200 /health) and redirect-safety coverage for the legacy fallback. The existing test still proves 503 /ready never falls back to a successful /health.
• Updated the operator checklist to require both liveness /health and DB-aware /ready.
• Corrected the degraded-router comment to describe peer readiness probes rather than 2xx /health pings.

Validation:

• cargo test -p gitlawb-node gossip_ssrf_tests — 6 passed
• cargo test --workspace against PostgreSQL 16 — passed
• cargo fmt --all -- --check — passed
• cargo clippy --workspace --all-targets -- -D warnings — passed

[jatmn]

I found issues that need to be addressed before this is ready.

Findings

• [P2] Keep the legacy fallback within one peer-probe deadline
  crates/gitlawb-node/src/main.rs:988
  The shared client gives each request a 10-second timeout, but the new 404 fallback starts a second independently timed /health request. A legacy (or attacker-controlled public) peer can therefore hold the serial gossip loop for nearly 20 seconds by delaying /ready before returning 404 and delaying /health; the prior probe had a single 10-second budget. Since the peer table is unbounded and the loop updates one row at a time, enough such rows cause sweeps to overrun the five-minute interval and leave peer reachability stale. Apply one deadline to the complete /ready-then-/health operation (or pass the remaining budget to the fallback), and cover a delayed-404 case.

[jatmn]

@beardthelion LGTM, requires your manual review.

[beardthelion]

The readiness switch is the right call and all four findings from the earlier rounds landed properly. I confirmed the compatibility story by execution rather than by reading it: an axum router carrying only /health and no fallback, which is v0.5.0's shape, answers 404 for GET /ready, and the probe falls through to /health and returns ready. /ready first shipped in v0.5.1, so the peers that arm exists for really do return 404. A 403 and a 500 both fail closed without touching /health.

I also mutated each line inside ping_peer_readiness_with_timeout one at a time against the seven tests. Pointing the probe at /health, widening the fallback trigger past 404, giving the fallback its own timeout budget, letting the client follow redirects, flipping the catch-all to true, and deleting the 404 arm each turn at least one test red. That part is well covered. Four things below.

Findings

• [P2] Bind the manual ping path to readiness with a test
  crates/gitlawb-node/src/api/peers.rs:442
  Reverting this line to the pre-PR inline /health probe leaves all seven tests green. Every line inside the helper reddens under mutation, so the harness works and this is a real hole: the handler's readiness behavior is unpinned, and a later edit can quietly return GET /api/v1/peers/{did}/ping to liveness-only while CI stays green. A test that drives the handler (or at minimum asserts the call site's behavior) closes it.

• [P2] Do not let a single probe sample drop a peer from federation
  crates/gitlawb-node/src/main.rs:948-949
  The probe result is written to last_ping_ok unconditionally, and api/repos.rs:1451 filters the federated fan-out on that flag, so one failed sample removes a peer's repos from /api/v1/repos/federated until the next 300s tick. The single-sample write predates this PR; what changes is the set of states that flip it, since a peer whose database hiccups for two seconds now drops out where previously only an unreachable process did. Two consecutive failures before writing false, or one re-probe inside the existing budget, would fix it. Whichever shape you pick, the property to preserve is that a peer survives one transient failure and still drops out on a sustained one.

• [P3] Log the probe outcome
  crates/gitlawb-node/src/main.rs:1000-1017
  Five materially different exits collapse into one bool with no tracing on any arm: ready, database-degraded 503, legacy 404 then healthy, refused redirect, and budget exhaustion. From the logs an operator cannot tell a peer whose database is down from one that is unreachable, which is the distinction this PR exists to draw. The bootstrap announce in the same function logs both its error and its timeout arm, so this is a gap against the file's own habit. The 503 arm and the legacy-404 downgrade are the two worth recording; the downgrade also gives you the list of peers still on the old build, which is what tells you when the fallback can be dropped.

• [P3] Reconsider the anonymous ping route writing the federation gate
  crates/gitlawb-node/src/api/peers.rs:444, route wired at crates/gitlawb-node/src/server.rs:292
peer_read_routes carries no auth layer and no per-IP brake, while sync_trigger_routes and peer_write_routes in the same file each carry one with a comment explaining that outbound fan-out needs it. This handler now drives a database-backed probe on the target and writes the flag that gates federation, from a single unauthenticated sample. Same lineage as the finding above, and the cheaper fix is probably to report the probe result without calling mark_peer_ping, leaving the flag owned solely by the gossip loop.

Not asks, just recording them

The probe URL is built by string concatenation, so a peer that announces https://host/?x=1 gets probed at / rather than /ready. I ran it: the /ready mock is never requested, / answers 200, and the peer reads ready even though /ready would have said 503. is_public_http_url validates scheme and host only, and upsert_peer stores the URL verbatim, so that shape survives announce. It is the same concatenation the pre-PR code used, and a hostile peer controls its own answer regardless, so it only really bites an honestly misconfigured operator. Url::join would be the tidy follow-up, separate from this PR.

README.md:276 and docs/OSS-READINESS-AUDIT.md:69 still list /health alone. Both predate this change and neither claims to state the peer contract, so I am not asking you to touch them here.

[coderabbitai]

🧹 Nitpick comments (1)

crates/gitlawb-node/src/main.rs (1)

941-962: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

failed_once never sheds entries for peers that disappear.

A DID is removed from the set only on a readiness success. Peers pruned from the peers table (or permanently down) leave their DID behind for the process lifetime, and DIDs are announce-influenceable. Retaining against the current snapshot each tick keeps the set bounded.

♻️ Prune the failure set against the current peer list

                 let peers = match state.db.list_peers().await {
                     Ok(p) => p,
                     Err(_) => continue,
                 };
+                // Drop bookkeeping for peers that no longer exist so the set
+                // cannot grow unbounded over the process lifetime.
+                let current: HashSet<&str> = peers.iter().map(|p| p.did.as_str()).collect();
+                failed_once.retain(|did: &String| current.contains(did.as_str()));
                 for peer in peers {

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/gitlawb-node/src/main.rs` around lines 941 - 962, Update the peer
polling loop around failed_once and the list_peers result to prune failure
entries against the current peer snapshot on every tick. Retain only DIDs
present in peers, so removed or permanently unavailable peers cannot accumulate
in the HashSet while preserving the existing readiness update behavior.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@crates/gitlawb-node/src/main.rs`:
- Around line 941-962: Update the peer polling loop around failed_once and the
list_peers result to prune failure entries against the current peer snapshot on
every tick. Retain only DIDs present in peers, so removed or permanently
unavailable peers cannot accumulate in the HashSet while preserving the existing
readiness update behavior.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 80a159f2-3246-4a44-ac4e-42951e15b196

📥 Commits

Reviewing files that changed from the base of the PR and between a1072e8 and 9ace2c6.

📒 Files selected for processing (2)

• crates/gitlawb-node/src/api/peers.rs
• crates/gitlawb-node/src/main.rs