fix: choose our own home DERP from netcheck, and announce before moving - #36
fix: choose our own home DERP from netcheck, and announce before moving#36timmills wants to merge 14 commits into
Conversation
…ntrol plane
My boards are in Australia and were sitting on the Frankfurt relay at 254-927 ms
while Sydney answered in 10-52 ms, across reboots, with no way to move them. I
went looking for why netcheck was not helping, and I think there is a loop here.
NetInfo.PreferredDERP is populated from ml->derp_region_default, but that value
originates from the control plane, which echoes back whatever we send as
Node.HomeDERP - as the log line at the register path already points out. So we
report X, control echoes X, we adopt X and report it again. Whichever region we
start on is self-sustaining, and since ML_DERP_REGION is 4, that is Frankfurt
for every device that has not been given an explicit region.
Netcheck measures every region correctly the whole time; the result just has no
route into the decision. And with netcheck_override enabled it changed only the
local home region - NetInfo.PreferredDERP still reported the old one, so peers
were being told to reach us at a region we were not using.
The Go client has no notion of a control-assigned home region: it picks from its
own report and tells control (magicsock/derp.go:197).
preferredDERP = report.PreferredDERP
if preferredDERP == 0 { preferredDERP = c.pickDERPFallback() }
Selection now follows netcheck.go:1427-1500:
- choose on best-recent latency across the last few netchecks rather than the
newest probe (netcheck.go:1525), so one lossy sample cannot move home;
- require both an absolute and a proportional improvement before switching
(preferredDERPAbsoluteDiff = 10 ms, and new <= old * 2/3), since a home
change has to be announced to every peer;
- fall through to the best reachable region if the current one stops
answering.
On my bench board this moved 4 (254 ms) -> 5 (52 ms) on its own and then stayed
put - one change in a 1600-line log, no reconnect.
Happy to split the selection quality changes out from the reporting fix if you
would rather take them separately.
A home-region change is rare and has to be announced to every peer, so it is worth seeing without enabling debug logging. The much more frequent 'staying on the current region' line remains at INFO.
The no-hysteresis path (previous region unreachable -> take the best available)
was reachable from a single lost STUN round, which is the most plausible way
home-DERP selection could churn.
The reference treats the old region as accessible if it has EITHER a latency
this round OR has been heard from by a non-STUN route recently
(net/netcheck/netcheck.go:1475-1484, PreferredDERPFrameTime = 8s):
heardFromOldRegionRecently = prevRegionLastHeard.After(rs.start) ||
prevRegionLastHeard.After(now.Add(-PreferredDERPFrameTime))
oldRegionIsAccessible := oldRegionCurLatency != 0 || heardFromOldRegionRecently
Our equivalent is an established DERP session to that same region which has
received traffic within 8s - the same signal the liveness watchdog already
uses, and one that cannot self-satisfy because DERP servers send keepalives.
When the old region is alive but has no measured latency, the margin test sees
a negative difference and keeps it, matching the reference (netcheck.go:1487,
where oldRegionCurLatency of 0 makes the absolute-difference test true).
The previous commit fixed what we REPORT (NetInfo.PreferredDERP now comes from our own netcheck), but the self-node parse still adopted Node.HomeDERP unconditionally. Node.HomeDERP is the control plane echoing back the PreferredDERP we sent - not an independent suggestion, as the code's own log line at the register path already says. So after netcheck picked a nearer region, the very next netmap still carried the old value (control has not processed our report yet) and flipped the ACTIVE region - ml->derp_home_region, the value ml_derp.c connects with - back again. That reproduces the exact mismatch this series set out to remove, only inverted: we would report the good region while connecting to the stale one. Now treated as a seed when we have no region at all, matching the guard the register path already uses, and logged when it disagrees with our choice. Note this still only takes effect on the next DERP connection; the reference actively moves its home connection when the preference changes (magicsock/derp.go:203+). That is a separate change.
derp_rtt_hist slots are indexed by position in derp_regions[], so they are only meaningful while that array's contents are unchanged. A DERPMap that adds, removes or reorders regions would silently attribute one region's latency to another - and that history is what home selection is based on. Checksum the region IDs on each netcheck and clear the history when it changes. The reference does not have this problem because it prunes by time and keys by region ID (netcheck.go:1405-1410, :1525); this is the cheap equivalent for a fixed-size array. Also softens a claim in the previous commit: control cannot dictate the home region, but it can bias the choice via DERPMap.HomeParams.RegionScore, which the reference multiplies into every latency (netcheck.go:1430-1439). We do not implement that yet, and the comment now says so.
…ction
Found the hard way. A relay-only board picked a nearer region on boot, and
became unreachable to every peer for as long as I watched it - not because the
choice was wrong, but because peers reach a node at the HomeDERP the control
plane advertises for it. A home change we cannot report does not improve
connectivity, it removes it.
The reference refuses in exactly this situation
(wgengine/magicsock/derp.go:177-190):
connectedToControl = c.health.GetInPollNetMap()
if !connectedToControl && !force {
if myDerp != 0 { return myDerp } // keep what we have
// else fall through: any DERP beats none
}
GetInPollNetMap is documented as 'whether the client has an open HTTP long poll
to the control plane' (health/health.go:770-779). ctrl_stream_rx_ms is our
equivalent - it is fed by every frame on the map stream and already backs the
stream-liveness watchdog. Having no home region at all remains the documented
exception: any relay beats none.
Second half: when the region does change, report it immediately rather than
waiting for the next STUN completion to trigger an endpoint update. Until
control has the new PreferredDERP, every peer is still dialling the old region,
so the delay is measured in lost reachability. Retries on the next loop
iteration if the update fails.
I had noted this guard as 'not implemented' when adding home selection. That
was the wrong call: on a device whose only path is a relay, it is the
difference between a latency improvement and losing the device.
Two frames the relay sends us that we were dropping. PeerGone (0x08) carries a 32-byte peer key AND a one-byte reason (derp.go:88). We logged four bytes of the key and ignored the rest. Reason 0x01 is PeerGoneReasonNotHere - the relay has no path to that peer at all - and yet the WireGuard fallback loop keeps driving a handshake through that same relay every 30 s indefinitely. On a board whose peer is genuinely unreachable I watched this reach 53 attempts with lastrx never advancing: pure load on the relay and on a device that has little to spare. The reference removes its DERP route for the peer on any PeerGone reason (magicsock/derp.go:652-665). There is no route table here, so the equivalent is to stop pushing handshakes through a relay that has just said it cannot deliver them: a 60 s backoff, cleared immediately if we hear from the peer, so a peer that comes back is picked up at once rather than waiting out a timer. FrameHealth (0x14) was falling through to the default case. Its only current use upstream is duplicate-connection detection (derp.go:118-123) - two clients sharing a node key and fighting over one relay session, which from the outside looks exactly like an unreachable node. That is worth seeing, so it is logged rather than dropped.
…e fault Two problems that only appear on a relay-only device, which is why a bench board with direct paths never showed them. Every known peer is probed forever regardless of whether we are talking to it. Where a peer is reachable only through a relay, each probe costs a full relay round trip, so the 32-slot probe table saturates permanently: I measured active_probes pinned at 32/32 with just 4 peers, oldest_probe_age sitting above the 5 s timeout, and returning pongs unmatchable. The reference does not do this. It ends heartbeats for an idle session (magicsock/endpoint.go:835, sessionActiveTimeout = 45 s) rather than probing peers it has no traffic with. WireGuard already tracks last_rx/last_tx per peer, so the same gate is cheap here: no probes for a session with no traffic in 45 s. Trust-expiry bookkeeping still runs; this gates new probes only. The second problem is worse in practice. With the table saturated, the 'probe table full' warning fires many times a second, and on this device the logging itself blocked the main loop - 'logger took a long time (3050 ms)' - which starves the ESPHome API and makes the node look unreachable while it is running fine. The diagnostic became the outage. Now rate-limited to one line per 10 s with a suppressed count. Sizing the probe table larger would not have fixed either: the load is proportional to peers times round-trip time, and the reference's answer is to stop generating it.
Missing prototype - ml_derp.c called it with only an implicit declaration.
The caller and prototype landed but the function body did not, so the link failed. Restored.
Two problems found reviewing the branches together rather than separately. netcheck_override is exposed in the YAML by the DERP retry series and documented as opt-in, but this branch had removed its only consumer - so the flag became a documented safety knob that does nothing. The branches merge cleanly, so nothing failed; a user setting netcheck_override: false simply got netcheck-driven home selection anyway. That is the worst shape a defect can take. The flag now gates the CHOOSING again, as it always did. What is deliberately NOT gated is the REPORTING: NetInfo.PreferredDERP always carries the region we are actually using rather than the control plane's echo of it, because that echo loop is a bug in either mode - with the override off it would still pin every device permanently to whatever region it first landed on. netcheck_override_threshold_ms is superseded by the reference's two-part margin (absolute >= 10 ms AND proportional <= 2/3), which is strictly more conservative than one absolute threshold. Also: ML_DISCO_SESSION_ACTIVE_MS was defined twice. It turns out v0.5.4 already defined it - unused, exactly like derp.last_recv_ms before the liveness watchdog - and my edit added a second copy rather than annotating the existing one. Duplicate removed, and the comment now notes it was already there, since that supports the same 'intended and never finished' reading.
derp.go:118-123 -> 117-122. And the idle gate is not equivalent to the reference: it gates on max(last_rx, last_tx) where the reference uses lastSendExt, time since we last sent (endpoint.go:835). A peer that keeps sending to us stays probed here. That is more conservative, but the previous wording implied equivalence.
Ordering bug, found by watching a relay-only board go dark for minutes after
picking a nearer region.
Peers reach a node at the HomeDERP the control plane advertises, which control
derives from the PreferredDERP we report. The previous code set
derp_home_region immediately and reported afterwards, so the device stopped
listening on the region everyone was still calling and only announced the new
one later. The improvement in latency was real but it cost a multi-minute
outage every time the region changed.
The Go client can move immediately because magicsock holds connections to
several DERP regions and prunes idle ones later. We hold exactly one, so the
equivalent is to keep the old relay until control has been told:
1. selection decides a new region -> derp_preferred_region, and a pending
report is flagged. derp_home_region is deliberately NOT touched.
2. the coord loop sends the endpoint update carrying the new PreferredDERP.
3. only on success does derp_home_region move and a DERP reconnect fire.
If the update fails we stay put and retry, because moving without announcing is
strictly worse than staying on a slower relay that peers can still reach.
|
Build-tested against v0.5.5 before you spend time on it, since CI can't run on a fork PR without your approval: The rebase onto v0.5.5 was conflict-free, and I checked your v0.5.5 work survived it rather than trusting that: One thing worth knowing when you read the diff: the ~35 lines this removes from Also deliberate: this does not add a |
|
Full review + hardware cycle done on this one too — and unlike #35, I can't merge it yet. The DERP/netcheck half holds up well (source-checked against the reference; a build of the first three commits survived everything I threw at it), but the series as a whole has two independent boot-killers on a SaaS tailnet with a populated peer table, plus one wiring problem on this repo's side. Everything below is measured, not speculated. The crashRepro environment: ESP32-S3, ordinary SaaS tailnet, ~14 NVS-cached peers, boot straight into a PeerGone burst (we connect to our home relay at ~7–8 s and immediately learn most peers aren't on it). In every capture the first
So the two changes are independently fatal, and it's not the flood: We didn't finish root-causing the exact starvation mechanism (timing coincides with the boot window where the NVS peer-cache is being written heavily; flash-write vs PSRAM/cache contention is our working theory). Happy to dig further together, but the isolation is solid either way. Requests for the respin
The home-DERP selection logic itself, the echo-loop break, the announce-before-move ordering, the RTT-history design, PeerGone reason parsing and the HEALTH frame handler all read correctly against the reference and I'd like all of them in. The bench + repro harness is standing — send the respin and it goes straight through the full cycle, including your symmetric-NAT field test for the selection half. |
Two layers, both bench-measured under sustained fire from one aggressive peer (which reboot-looped an unpatched v0.5.5 within ~5 minutes): - Rate-limit the per-event 'DISCO PONG unmatched' and 'DISCO probe table full' warnings to one line per 10 s with a suppressed-event count (throttle pattern adopted from timmills' #36 series). - Demote the hot-path per-packet chatter (PONG sent, DERP TX: SendPacket, Probe registered) from INFO to DEBUG - it fires for every relayed packet during a storm and only matters with the debug switch on. This removes the log-driven part of the starvation and measurably extends survival; a residual stall in the component status path under the same extreme conditions is identified and tracked separately.
…uard_device Addresses the two boot-killers found in review (task-watchdog starvation on a warm-cache SaaS tailnet booting into a PeerGone burst). 1. PeerGone -> dedicated derp_gone_queue (DERP RX task enqueues a 32-byte key, no peer scan / no log on that task). wg_mgr, the sole owner of ml->peers, drains it, applies the backoff, and logs rate-limited (one line / 10s with a suppressed count, like the DISCO probe-table-full log). Drop-if-full: the backoff is advisory and the next PeerGone re-arms it. 2. DISCO idle-gate now reads wg_mgr's OWN bookkeeping instead of struct wireguard_peer from the probe loop: a new last_data_recv_ms stamped in process_wg_packet (peer resolved via pkt->src_pubkey) plus the existing last_pong_recv_ms. Gate = idle if we have not HEARD from the peer (data or pong) in ML_DISCO_SESSION_ACTIVE_MS. Deliberate deviation from "max(last_send_ms, last_recv_ms)": wg_mgr is not on the data-tx path, and stamping our own DISCO sends would make the gate self-sustaining (a peer we keep probing would never look idle), so it gates on last-heard. At boot all timestamps are 0 => nothing is idle (the gate is blind then; the rate-limited log is the boot-storm protection). Compile-verified on esp32-s3-devkitc-1 / esp-idf (esphome 2026.6.5). NOT yet validated on hardware with a warm cache + default watchdog timeout. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Hardware validation of the respin, on the warm-cache repro you described. Setup: esp32-s3-devkitc-1 on an ordinary SaaS tailnet (~7 NVS-cached peers), booting straight into a PeerGone burst (most cached peers live off the home relay), behind symmetric NAT. Built with this branch and, crucially, Result: it survives at the stock watchdog. 90s+ uptime, 0 task-watchdog resets, So request #1 (PeerGone routed to wg_mgr via a dedicated queue, drained + rate-limited on the wg_mgr task, off the DERP RX task) and request #2 (idle-gate reads wg_mgr's own bookkeeping — a new One honest caveat: this run had the DERP selector off (the Re request #3: acknowledged, |
|
Ran the respin through the bench. The starvation fix works — confirmed independently on my side, and the queue/bookkeeping shapes are right. Two things still block the merge, one of them a real hole in the new idle-gate. The good part, measured hereSame SaaS repro that killed the previous revision (warm NVS cache, boot straight into a PeerGone burst):
Worth stating explicitly: this branch is based on v0.5.5, so it does not include the log-throttle work now on Routing PeerGone through its own queue and draining it on wg_mgr is the right shape, and so is deriving idleness from wg_mgr's own state instead of Blocker: the idle-gate can't see traffic on a direct path
That closes a loop you were careful to avoid in the other direction:
I could not reproduce this on the bench: my direct peer's pong chain never broke ( Two ways to close it, either is fine by me:
Belt and braces would be both. Note Minor: the new queue isn't in the creation check
Merge mechanicsThis branch is based on v0.5.5 and conflicts with current Once the direct-path stamp lands I'll rerun the full cycle, and then wire the component-side |
2 participants
Rebased onto v0.5.5. This is findings #1 and #2 combined, as you asked in #33.
#1 — the home DERP region is chosen by a loop that echoes itself
NetInfo.PreferredDERPwas populated fromderp_region_default, which is a value that came from the control plane — and the control plane echoes back whatever the client sends. So the first region is self-sustaining:ML_DERP_REGION 4(Frankfurt) is a compile-time constant and the only fallback, so a device that never breaks the loop stays on Frankfurt permanently, everywhere in the world.Netcheck measures every region correctly the whole time. The measurements simply had no path to the decision — the override plumbing exists in the C but nothing exposed a way to reach it, and
config.preferred_derp_regionwas always 0.Observed here: two ESP32-S3 boards in Australia sat on
fraat 682-927 ms while Sydney answered in ~10 ms, surviving reboots. The practical cost was an OTA taking 498 s that takes 25.8 s on the correct region.This PR reports our own netcheck choice and selects on best-recent latency across several netchecks rather than a single probe. Movement requires both an absolute margin (>=10 ms) and a proportional one (<=2/3 of the incumbent), so near-equal regions do not ping-pong — modelled on
net/netcheck/netcheck.go:1427-1500andwgengine/magicsock/derp.go:197.Verified on hardware:
Home DERP changing 4 (254ms) -> 5 (52ms), one change then stable.Related fixes in the series, each its own commit:
#2 — announce the change before moving to it
The ordering matters and is easy to get backwards: if the device moves to the new home region and then tells the control plane, there is a window where peers are still being told to reach it at the old region while it is no longer listening there. Announce first, then move.
I have put that reasoning in a comment at the site rather than only in the commit message, since it is exactly the kind of thing that gets "simplified" back out by someone reading the code alone.
Please note — this branch also carries finding #6
The idle-peer probe gating (#6) is entangled with the DERP work in this series: it shares
ml_wg_mgr.cchanges with the DERPPeerGonehandling and the probe-table saturation logging. I have left it in rather than attempt a split that I could not build-test cleanly. Happy to separate it if you would rather review it on its own — say the word and I will rebase it out.Also included
netcheck_overrideis honoured again (it had been left unreachable), and a duplicate define is dropped.Testing
Two ESP32-S3 boards, ~85-node Tailscale SaaS tailnet, Australia — which is what made the Frankfurt pinning so visible. The offer stands to field-test against the symmetric-NAT site for this one specifically, since you mentioned your bench can't produce honest cross-continent latency.