fix(node): reset a peer's reachability gate when an announce changes its http_url (#270)

[beardthelion]

upsert_peer's ON CONFLICT branch rewrote http_url and said nothing about last_ping_ok, so an unsigned announce could repoint an existing peer's URL and inherit the reachable = true the previous host earned. The federated repo fan-out (crates/gitlawb-node/src/api/repos.rs:1451) filters on that flag and then stamps each entry from the peer's response body with that peer's DID, so the inheritance is what made the injection instant rather than probe-delayed. Reaching the branch needs no credential: require_signed_peer_writes defaults false (config.rs:48) and the announce handler's unsigned path only logs a warning.

The change

One clause on the existing statement:

ON CONFLICT(did) DO UPDATE SET
  http_url = $2,
  last_seen = $3,
  last_ping_ok = CASE
    WHEN peers.http_url IS DISTINCT FROM $2 THEN FALSE
    ELSE peers.last_ping_ok
  END

In the DO UPDATE branch peers.http_url is the existing pre-update row and $2 is the proposed value, so the comparison reads the stored URL, and it evaluates under the conflict row lock instead of a read-then-write that could lose an update. Both writers funnel through upsert_peer (the announce handler and the bootstrap announce-back in main.rs), so one clause covers both with no per-caller opt-out. No schema change, no migration.

What this bounds, and what it does not

Worth being precise, since the flag is easy to over-trust:

Only the federated repo fan-out gates on last_ping_ok. Four consumers act on a repointed http_url regardless of it (sync.rs's origin resolve, the post-receive notify fan-out, trigger_sync, and the public resolve route), and two read surfaces republish it as reachable. So this removes the automatic inheritance; it does not close the rewrite. Binding a DID to its first-seen announcing key is what closes it, which is #273 and a federation-model decision rather than a patch.

Until #248 lands, the reset is also bypassable in one unauthenticated request: GET /api/v1/peers/{did}/ping carries no auth layer and no IP brake, and writes mark_peer_ping from the stored URL's own probe response. #248 already deletes that write and is approved, so this branch deliberately leaves ping_peer alone rather than editing the same three lines.

Comparison is exact, which cuts both ways. A cosmetic difference such as a trailing slash also clears the flag, and an unauthenticated caller can use that to hold a legitimate peer out of the flag-gated fan-out at roughly 12 requests per hour per victim against the 600/hour/IP default brake. That cost is accepted here because the alternative is silent trust inheritance, and it goes away with #273. Normalizing instead would mean canonicalizing the stored value, this comparison, and the existing trim_end_matches call sites.

All of the above is recorded in a comment at the fix site so the next reader does not mistake the mitigation for a closure.

Verification

Tests landed before the fix, in their own commit, and the failure was observed rather than assumed:

	before the fix	after
url_change_clears_reachability	FAILED on the last_ping_ok assertion, with the http_url assertion passing	ok
the other five	ok	ok

Three of the six are green in both directions by design. They are regression pins, and each one was confirmed load-bearing by applying the wrong implementation it targets and watching it go red:

Wrong implementation	Caught only by
CASE clause removed	url_change_clears_reachability
last_ping_ok = (peers.http_url IS NOT DISTINCT FROM $2)	same_url_reannounce_does_not_grant_reachability
condition hoisted to a statement-level WHERE	same_url_reannounce_still_advances_last_seen
rtrim(...) normalizing comparison	cosmetic_url_difference_counts_as_a_change

Each of those four passes at least three of the other tests, which is why the set is six and not one. The second would be the dangerous regression: it lets any unsigned same-URL re-announce set the flag TRUE with no probe at all, which is worse than the bug under repair. The third silently stops last_seen advancing on a liveness re-announce, so the peer ages out.

cargo test -p gitlawb-node: 516 passed, 0 failed, no pre-existing test flipped. cargo clippy --all-targets -- -D warnings clean workspace-wide. cargo fmt --check clean.

Scope

Issue #270 was narrowed to this mitigation on 2026-07-28, with the rest split out: #272 (unvalidated repo slug on the same unsigned route, shipped as #274) and #273 (bind a DID to its first-seen key). Nothing here touches sync.rs, api/repos.rs, api/resolve.rs, main.rs, or server.rs.

Closes #270

Summary by CodeRabbit

• Bug Fixes
  • Improved peer reachability tracking when a peer’s announced URL changes.
  • Preserved reachability status when peers re-announce the same URL.
  • Continued updating the last-seen timestamp for repeated announcements.
  • Correctly distinguishes URL changes based on exact string matching, including trailing-slash differences.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: e6852174-0a1c-476b-b507-9a44c52894ca

📥 Commits

Reviewing files that changed from the base of the PR and between 111cff7 and 811ad50.

📒 Files selected for processing (1)

• crates/gitlawb-node/src/db/mod.rs

---

📝 Walkthrough

Walkthrough

Peer upserts now clear last_ping_ok when a peer announces a different HTTP URL, while preserving it for identical URLs. New database tests cover URL changes, re-announcements, inserts, exact comparisons, and last_seen updates.

Changes

Peer reachability semantics

Layer / File(s)	Summary
URL-aware peer upsert and reachability coverage crates/gitlawb-node/src/db/mod.rs	upsert_peer clears reachability on URL changes, preserves it for identical announcements, and continues updating last_seen; tests cover these transitions and exact URL string comparisons.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related issues

• Gitlawb/node issue 273 — Directly addresses resetting last_ping_ok when http_url changes.

Suggested reviewers: kevincodex1

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly names the main behavior change: resetting reachability when an announce changes http_url.
Description check	✅ Passed	It covers summary, motivation, changed behavior, verification, and scope, though it doesn't follow the template verbatim.
Linked Issues check	✅ Passed	The code matches #270 by clearing last_ping_ok on URL changes, preserving same-URL reannounces, and keeping new peers unreachable.
Out of Scope Changes check	✅ Passed	The added comment and tests support the fix, and no unrelated code paths or public APIs were changed.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/270-reset-peer-reachability-on-url-change

---

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