fix(net): survive edge connection-table flushes — DERP/disco resilience

[iliabaranov]

DERP/disco resilience — survive office-edge connection-table flushes

Status: DRAFT — one fix still pending. Base main, HEAD 674f457.

The bug

The bench protective-stop dropped green every few hours. Root cause (nailed via a live wire+journal capture from the DUT-host, over a session-to-session bridge): the office edge firewall periodically flushes its connection table — RSTing DERP TCP across regions and rebinding NAT in the same instant. One flush kills the machine↔remote direct path (a NAT hairpin off the edge's public IP, not LAN) and the DERP relay at once, so the 5 Hz heartbeat loses both paths together.

What's fixed and validated

Commit	Fix
a175361	Demote-verification — WG data rx vetoes a spurious direct-path teardown
fd95b83	Hitless re-ingest — don't tear down an actively-authenticating safety session
1874ba1	Rekey black-hole + endpoint-table wedge + WG-session instrumentation
38fca7d	Disco-reset recovery for a relay-stuck safety peer
f2f883e	Immediate DERP reconnect on RST (was a hardcoded 1 s wall)
7162a9b	Relay-rx instrumentation

The keypair-expiry mechanism that killed earlier soaks is proven fixed — wg_tx_keypair_expired stayed 0 across 8 h including two flushes. f2f883e's reconnect works on the wire (936 ms).

The one remaining bug (identified, code-confirmed, fix held)

Fast reconnect alone didn't hold green. During a flush the edge also RSTs machn's standby DERP conn, and derp_manage_aux does a blocking ~2 s TLS reconnect inside the same loop that polls home-rx — so machn stops processing the relayed heartbeat for ~2 s (home conn never dropped → reconnects=0) → disarm. The fix (defer/async the standby reconnect) touches the safety-critical DERP loop with a failover trade-off, so it's held for one more flush's independent wire confirmation + review before landing.

Do not merge until

run-23 shows green held through a real edge flush on the corrective build.

🤖 Generated with Claude Code

https://claude.ai/code/session_01UJz4ZB7WcPErDqKLWDq3ht

[iliabaranov]

@claude please review this PR for correctness, clarity, and especially any safety concerns in the DERP/disco changes (the safety heartbeat rides these paths). Context: chasing an intermittent protective-stop green-drop root-caused to office-edge firewall connection-table flushes; the last residual fix (defer the blocking standby-DERP reconnect that starves home-rx) is intentionally NOT yet in this PR pending live wire confirmation. Feedback on the committed fixes + instrumentation is welcome.

[iliabaranov]

@claude review always

[claude]

🔴 In derp_manage_aux() the new home-rx guard defers opening ANY aux DERP connection while ANY safety peer is relay-bound, but the aux want-set (ml_wg_collect_safety_regions) is drawn from that exact same safety-peer set. For a cross-region safety peer, this is self-sustaining: it stays relay-bound because its aux never opens, and the aux never opens because it's relay-bound — with derp_route_conn silently black-holing its heartbeat via the home fallback in the meantime. This reintroduces the safety-heartbeat wedge this PR is meant to fix, but for a cross-region symmetric-NAT peer instead of the home-region one. Scope the deferral to already-connected/serving conns, or allow at least one aux-open attempt for a region a relay-bound safety peer actually needs.

Extended reasoning...

The bug. derp_manage_aux() (components/microlink/src/ml_derp.c:698-711) adds a guard before the 'ensure a conn for each wanted region' loop:

if (ml_wg_any_safety_relay_bound(ml)) {
  s_diag_aux_connect_deferred++;
  return;
}

ml_wg_any_safety_relay_bound() returns true if any active safety peer (pinned/priority/health-tracked) currently has has_direct_path == false. The comment's premise is 'Home carries the heartbeat; the standby is redundancy' — true only when the relay-bound peer is homed on the same DERP region as the chip's home slot.

Why it self-deadlocks for a cross-region peer. The aux want-set that the guarded loop would otherwise service comes from ml_wg_collect_safety_regions(), which draws from the exact same safety-peer set the guard inspects. So a cross-region safety peer that goes relay-bound needs precisely the aux connection for its region to receive anything — derp_route_conn() only reaches a peer homed on a non-home region via a connected aux for that region; otherwise it silently falls back to the home conn, which the code's own comment documents as a server-side black-hole ('DERP server only delivers to clients connected to IT'). But that peer being relay-bound is itself sufficient to make the guard return early and block the very aux connect it needs. The state is self-sustaining: relay-bound → guard blocks aux → aux never opens → relayed heartbeat black-holes via home → peer stays relay-bound forever. Since this is a symmetric-NAT peer (the class this PR is about), there's no autonomous UDP path recovery either.

Why nothing else prevents it. Step 1 (reap of now-unwanted aux slots) runs unconditionally before the guard, so an existing warm-standby aux can be torn down by the reap and never rebuilt once the peer trips into relay-bound. There's also cross-contamination: a same-region priority peer going relay-bound (safe on its own, since home covers it) still trips the global guard and blocks a completely different cross-region peer's aux.

Concrete walkthrough (matches this PR's own target scenario). 1) Steady state: peer B is a cross-region, symmetric-NAT safety peer, direct, with a warm standby aux open on its home region. 2) An edge connection-table flush RSTs the direct hairpin and the aux DERP TCP simultaneously (this PR's whole premise). 3) B goes relay-bound (has_direct_path=false); its aux is now down. 4) derp_manage_aux() runs: ml_wg_any_safety_relay_bound() is true (because of B itself) → guard fires → returns before the aux-open loop. 5) B's relayed heartbeat and DISCO/CallMeMaybe now route via derp_route_conn, which finds no connected aux for B's region and falls back to home — silently eaten server-side since B isn't homed there. 6) B can never regain a direct path (no CMM gets through) and its aux can never reopen (guard still true), so the loop in steps 4-5 repeats indefinitely. Net result: a non-recovering loss of the safety heartbeat for peer B, requiring a reboot — the exact class of failure (a machine STOP that doesn't self-clear) this PR was written to eliminate, just shifted from the home-region case to the cross-region case.

Fix direction. Scope the deferral to conns that are already connected/serving a peer with a direct path (i.e. don't defer opening a NEEDED aux for a region a relay-bound safety peer is homed on), or allow at least one aux-open attempt per pass even while relay-bound, reserving the 'defer' behavior for the home-region case the comment actually describes.

[iliabaranov]

Resolved — this is a false positive against 6832794. The defer-aux guard this describes was removed in this exact commit; line 711 now anchors to the comment explaining the removal (714–719). The for (w…) loop at 720 attempts aux connects unconditionally (one per call, the pre-existing hard cap), and ml_derp_connect calls derp_pump_home_rx(ml) during each aux TLS handshake (ml_derp.c:1635) so the relay-bound safety peer keeps draining while a standby reconnects. No self-deadlock path remains.

[claude]

🟡 s_diag_relay_disco_resets (declared ml_wg_mgr.c:855, incremented at :3602 in the disco-reset-v2 block) is never read by any getter or surfaced in /admin/api/monitor, unlike every other diagnostic counter this PR adds. A bench run can never observe whether the relay-stuck disco-reset path fired. Either add it to ml_wg_get_reingest_diag (or a new getter) and expose it in handler_monitor's handler, or drop the dead counter.

Extended reasoning...

This PR's own stated design pattern (the reingest-diag comment block just above the affected code) is counters-not-logs, read via /admin/api/monitor, precisely so a log-less bench unit can be diagnosed after the fact. Every other diagnostic counter this PR introduces follows that pattern end-to-end: s_diag_rekey_retires, s_diag_rekey_retire_vetoes, s_diag_peer_removes, s_diag_remove_vetoes, and s_diag_evict_safety_skips are all returned by ml_wg_get_reingest_diag() and surfaced in handler_monitor() (ml_config_httpd.c); s_diag_ep_learn_evictions goes out through ml_wg_get_session_diag(); s_diag_demote_vetoes goes out through ml_wg_get_disco_obs_diag(); and s_diag_aux_connect_deferred/the DERP reconnect counters go out through their own ml_derp_get_* getters.

s_diag_relay_disco_resets is the one counter that breaks this chain. It's declared at ml_wg_mgr.c:855 and incremented at ml_wg_mgr.c:3602, inside the disco-reset-v2 escalation block that clears a relay-stuck safety peer's disco timers/backoff so the next sweep re-runs a from-scratch hole-punch. That escalation path is exactly the kind of rare, safety-relevant event this PR's instrumentation effort is meant to make visible — but there is no getter function anywhere in the tree that reads this static, and it does not appear in handler_monitor()'s JSON output. A grep for the symbol turns up exactly two hits (the declaration and the increment) and nothing else.

The practical effect: on a bench unit with no live serial log, there is no way to tell whether the disco-reset-v2 escalation ever fired, or how many times, during a soak. If a relay-stuck safety peer needed this from-scratch reset to recover (or needed it repeatedly, which would itself be a signal worth noticing), that information is silently discarded. This is the same visibility gap the rest of the PR is explicitly trying to close for the other new recovery paths (rekey-retire vetoes, teardown vetoes, evict-safety-skips, demote-vetoes, DERP reconnects, etc.) — it just wasn't wired up for this one counter.

To confirm this isn't just a naming coincidence or a case where the value is read some other way, I checked whether the increment could be dead code entirely (e.g. behind a condition that never fires) — it isn't; the increment sits inside the normal periodic disco-probe sweep gated only by p->relay_retry_count >= 3u and a per-peer rate limit, so it is very plausibly the kind of counter that would climb slowly during a real edge-flush/relay-stuck episode and matter for exactly the kind of forensics this PR's description talks about (run-20/21 style investigation).

Fix is small and low-risk: add out[5] = s_diag_relay_disco_resets; to ml_wg_get_reingest_diag() (bumping it to a uint32_t[6] and updating the one caller in handler_monitor()), or give it a one-off getter and one more cJSON_AddNumberToObject() line, following the exact pattern already used for every sibling counter. Alternatively, if the value isn't actually expected to be useful, it should be dropped rather than left silently uncollectable. Since this is a pure instrumentation/observability gap — no incorrect behavior, no safety-path change — it doesn't block merge, but it undercuts the PR's own diagnostic goal for the one path that arguably needs it most (a relay-stuck safety peer).