fix(node): use readiness for peer liveness - #248
Conversation
github-actions
Bot
added
the
needs-tests
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughPeer liveness checks now use DB-aware ChangesPeer readiness probing
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
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
|
Thanks for the contribution. A couple of things will help us review this faster:
See CONTRIBUTING.md. Update the PR and these notes will clear automatically. |
beardthelion
added
crate:node
|
For reviewer context, the This PR adds regression coverage inside the existing The triage workflow currently recognizes newly added literal Validation completed locally:
|
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
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/healthto/ready. Released v0.5.0 nodes expose/healthbut not/ready, and #170 deliberately left the peer probe on/healthfor exactly that rolling-upgrade case. Their/readyresponse is therefore 404, which this helper maps tofalse; both the periodic job and the manual ping persist that value. Since federated repository listing filters onlast_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/healthfallback that still treats a real/ready503 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/healthis 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/healthonly 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/healthresponse. 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.
|
Addressed all three findings in
Validation:
|
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
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/healthrequest. A legacy (or attacker-controlled public) peer can therefore hold the serial gossip loop for nearly 20 seconds by delaying/readybefore 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-/healthoperation (or pass the remaining budget to the fallback), and cover a delayed-404 case.
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
@beardthelion LGTM, requires your manual review.
beardthelion
left a comment
beardthelion
left a comment
There was a problem hiding this comment.
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/healthprobe 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 returnGET /api/v1/peers/{did}/pingto 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 tolast_ping_okunconditionally, andapi/repos.rs:1451filters the federated fan-out on that flag, so one failed sample removes a peer's repos from/api/v1/repos/federateduntil 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 notracingon 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 atcrates/gitlawb-node/src/server.rs:292
peer_read_routescarries no auth layer and no per-IP brake, whilesync_trigger_routesandpeer_write_routesin 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 callingmark_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.
github-actions
Bot
removed
the
needs-tests
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/gitlawb-node/src/main.rs (1)
941-962: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
failed_oncenever sheds entries for peers that disappear.A DID is removed from the set only on a readiness success. Peers pruned from the
peerstable (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
📒 Files selected for processing (2)
crates/gitlawb-node/src/api/peers.rscrates/gitlawb-node/src/main.rs
Superseded: re-reviewed the new head (e8a000c).
beardthelion
left a comment
beardthelion
left a comment
There was a problem hiding this comment.
Both of my round-three asks landed, and both are now pinned by a test that fails without them. I re-ran the mutation matrix on this head rather than carrying last round's results forward: pointing the probe at /health reddens four tests, widening the fallback trigger past 404 reddens one, giving the fallback a fresh budget reddens one, flipping the catch-all to true reddens two, and letting the client follow redirects reddens two. Re-adding mark_peer_ping to ping_peer, or putting that handler back on an inline /health probe, each redden the new handler test.
The new hysteresis is the exception, and it is the one thing I want before merge.
Findings
-
[P2] Pin the gossip loop's use of the hysteresis, not just the helper
crates/gitlawb-node/src/main.rs:956
I replaced the wholematch peer_ping_db_update(...)block with the pre-PRlet _ = state.db.mark_peer_ping(&peer.did, ok).await;, which deletes the feature outright, and everything stayed green: 8 tests ingossip_ssrf_tests, 20 inapi::peers::tests. The unit test covers the pure function and nothing covers its only call site, so this round's work is removable without CI noticing. Same shape as the manual-ping hole from last round, which you closed well. Deleting thefailed_once.retainprune is green too, so one test can cover both. I built the fix before asking for it: extract the tick body intoasync fn gossip_ping_round(db: &crate::db::Db, client: &reqwest::Client, failed_once: &mut HashSet<String>, peers: Vec<crate::db::PeerRecord>), call it from theselect!arm, and drive it from a#[sqlx::test]against a mockito peer answering/ready503: after round onelast_ping_okis still TRUE, after round two it is FALSE. That is green as written and RED once the hysteresis inside the extracted function is deleted. Seed the peer with a raw INSERT the way yourmanual_ping...test does;upsert_peerrejects a loopbackhttp_urland mockito binds on 127.0.0.1. -
[P3] Give the gossip interval
MissedTickBehavior::Delay
crates/gitlawb-node/src/main.rs:940
The hysteresis is counted in ticks, but its value is wall-clock: the second sample is meant to be 300s after the first.tokio::time::intervaldefaults toMissedTickBehavior::Burstin the pinned tokio 1.50, so a sweep that overruns the period is followed by catch-up ticks and the two samples can land seconds apart, which is the mass-failure case the guard exists for. A sequential sweep at up to 10s per peer needs roughly 30 unreachable peers to get there. Addinginterval.set_missed_tick_behavior(tokio::time::MissedTickBehavior::Delay);after the constructor is the whole change and it compiles clean. Worst case today is the guard degrading to the old behavior rather than doing something wrong, hence P3.
Not asks, recorded
- The anonymous ping route still has no per-IP brake, and
/readycosts the target a database query where/healthdid not. I checked the delta, not the state: the pre-PR handler made the same unauthenticated outbound request and additionally wrotelast_ping_ok, and the probe now sits inside a single 10s budget covering both legs. This PR moves that surface in the right direction; the brake is a separate question and not something I want in this PR. last_seenis no longer refreshed on a first-failure tick, since that tick skips the write. Every reader is display or ordering and none gates on freshness. No action.- A peer alternating ready/unready is never persisted unreachable, because one success clears the set. That is the tradeoff behind the invariant I gave last round: survive one transient failure, drop out on a sustained one. Settled as intended, not open.
Scope is closed after this. The two commits since my last round have never been through CI: PR Checks is awaiting approval on both, and the last green run was a1072e8, before the hysteresis and the prune.
|
Addressed the final review findings in
Validation:
|
Superseded: both round-four asks landed and are now pinned by tests that fail without them. Re-reviewing head 32e4578.
There was a problem hiding this comment.
Approving. The premise is pinned now: pointing the probe back at /health reddens six tests across both modules, and reverting gossip_ping_round's match to the pre-PR let _ = db.mark_peer_ping(&peer.did, ok).await; fails gossip_ping_round_requires_two_failures_before_persisting_unreachable. That was my round-four finding and it is closed by execution rather than by reading the new test.
I re-ran the whole matrix on this head instead of carrying earlier results forward. Deleting the hysteresis in peer_ping_db_update reddens two tests, deleting the failed_once.retain prune reddens one, widening the fallback trigger past 404 reddens two, unbinding the shared /ready+/health deadline reddens one, letting the client follow redirects reddens two, flipping the transport-error arm to true reddens one, and re-adding mark_peer_ping(&did, ok) to the manual handler or putting it back on an inline /health probe each redden the handler test.
CI is green on this head across all nine jobs. Your note about the MSRV lane failing on the AWS crate drift is stale: the base is current with main and MSRV (Rust 1.91) passes.
Findings
None blocking.
Recorded, not asks
- I overstated one thing last round. I wrote that re-adding
mark_peer_pingtoping_peerreddens the handler test. That holds for the faithful revert, which writes the probe result:mark_peer_ping(&did, ok)with/readyat 503 writes false and the test fails. It does not hold for a write of a hardcodedtrue, which stays green because the test seedslast_ping_ok = TRUEand asserts the same value.mark_peer_pingalso stampslast_seen(db/mod.rs:2106), so assertinglast_seenis byte-identical would pin the no-write property in both directions. Not asking for it. gossip_taskhas no test, so the cross-tick lifetime offailed_onceis unpinned: movinglet mut failed_once(main.rs:942) inside theinterval.tick()arm turns the hysteresis into a no-op and all 29 tests stay green, as does deleting theMissedTickBehavior::Delayline. This is the next layer of the shape I asked about last round, and pinning it needs the loop extracted the way the round was. Not asking for that here: the feature is pinned at the pure function and at the round, and the residue is loop scaffolding.- Dropping the
Some(true)arm's write leaves all 29 green. A regression there strands a peer as permanently unreachable with no operator path back, since the manual ping is read-only by design now. Recording it, not asking. - The two-tick guard is spaced in ticks but argued in wall-clock. Probes are serial at up to 10s each and
list_peersorders bylast_seen DESC(db/mod.rs:2118), so a peer probed last in one round and first in the next can have its samples land seconds apart. That is traced from the code rather than measured. It takes roughly 30 unreachable peers to get there and the cost is one peer missing from/api/v1/repos/federatedfor a cycle, so I am leaving it. Keying first-failure on anInstantwould make it wall-clock by construction if it ever bites. peer_read_routes(server.rs:290) still carries no auth layer and no per-IP brake, unlikesync_trigger_routesandpeer_write_routesin the same file. What changes here is the shape of the probe, not its cost to us: I checked the pre-PR handler and it already made one outbound request under the same 10s client timeout and additionally wrotemark_peer_ping, so our own per-request DB work goes down. What is new is that a peer answering 404 draws a second outbound leg, and the probe lands on/ready, which runs a query on the target where/health(server.rs:497) ran none. Filing that separately rather than growing this PR.- Not yours:
upsert_peer(db/mod.rs:2093) rewriteshttp_urlon conflict and leaveslast_ping_okalone, and announce accepts unsigned bodies whilerequire_signed_peer_writesdefaults false, so a repointed peer URL inheritsreachable=true. Pre-existing, I will file it.
Scope is closed.
32e4578 to
e9bf1ea
Compare
…its http_url (Gitlawb#270) The ON CONFLICT branch rewrote http_url and left last_ping_ok alone, so an unsigned announce could repoint an existing peer's URL and inherit the reachable=true that the previous host earned. api/repos.rs filters the federated fan-out on that flag and then stamps each entry from the peer's response body with the peer's DID, so the inheritance is what made the injection instant rather than probe-delayed. Clear the flag when the URL actually changes, and leave it alone when it does not. In the DO UPDATE branch peers.http_url is the existing pre-update row and the comparison runs under the conflict row lock, so this needs no second round trip and does not race a read-then-write. Both writers funnel through upsert_peer, so the announce handler and the bootstrap announce-back are both covered. Observed transition on the tests from the previous commit: before url_change_clears_reachability ... FAILED (last_ping_ok assertion) after url_change_clears_reachability ... ok the other three ... ok throughout Both guards were confirmed load-bearing by reverting, not by reading a green suite. Removing the CASE clause entirely turns url_change_clears_reachability RED again. Flattening it to last_ping_ok = (peers.http_url IS NOT DISTINCT FROM $2) still passes three of the four cases and is caught only by same_url_reannounce_does_not_grant_reachability, which is why that case exists. This bounds the automatic inheritance; it does not close the rewrite. Four other http_url consumers never read this flag, and until Gitlawb#248 lands the unauthenticated ping route can write it back from the attacker's own host. Gitlawb#273 is the closure. Closes Gitlawb#270
|
@jatmn this was force-push rebased onto c83cbc5 after you approved, so the approval state you see is still the one from a1072e8 and GitHub carried it forward on its own. The rebase itself is clean. The branch's own diff is unchanged, and the head-to-head delta is exactly main's, which I checked over the whole tree rather than just the touched files. Nothing came in with it. What your approval predates is the commits after a1072e8, and that is the part worth your time. CI is green on the rebased head apart from |
3 participants
Summary
Use the database-aware
/readyendpoint for periodic and manual peer checks, while falling back to/healthonly 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
/healthintentionally 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
What changed
/healthto/ready, with a 404-only/healthfallback for nodes released before/readyexisted.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 --workspaceThe focused regressions can also be run with:
Before you request review
cargo test --workspacepasses locallycargo fmt --allandcargo clippy --workspace --all-targets -- -D warningsare cleanfeat(...),fix(...),docs(...)).env.exampleupdated if behavior or config changed (N/A)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
/ready) to determine peer reachability, with legacy fallback to/healthwhen/readyis unavailable.400, without scheduling sync work.GET /healthandGET /ready.