fix(node): re-pin connected UDP sockets when the host changes medium - #144
fix(node): re-pin connected UDP sockets when the host changes medium#144fr34aky wants to merge 9 commits into
Conversation
Does this help Android?Checked, since Wi-Fi↔cellular is the handover this exists for. It half does, and the The reaction is already correct. Android is not one of the two platforms with the The signal is where it goes wrong. Android is So on the platform that changes medium most often:
Detection latency is the poll interval, reported as though it were milliseconds. Verified The fix is a cfg widening in Two further Android notes, neither verified on a device:
None of this is covered by |
|
Thanks for this, and thanks for the measurements. The change is good and I want it in. Answering your question first, then the review. Your question: does the Android fix ride along, or follow?It rides along, in this pull request, as its own commit. Your diagnosis is right and I checked every part of it that can be checked without a device. Android is The data plane is fine. Detection still happens on the timer, and the immediate heartbeat that makes the peer re-pin still runs. What breaks is the report: The reason it belongs here rather than in a follow-up is that widening cannot make Android worse. If the socket or the group bind is refused, Action. One commit that changes five
Then update the platform table at One thing to be clear about: this path lands unexercised either way. The The two other Android items you raised should not ride along, because both are real platform work with a design in them: the bionic Please retarget this pull request to
|
A watcher that resolves when the kernel reports a change to the host's network links, so callers can react to interface state in sub-second time instead of polling for it. Netlink `RTNLGRP_LINK` on Linux, `PF_ROUTE` on the BSDs. Self-contained and unused by anything yet: it lands separately because more than one caller wants it, and a second netlink socket beside this one would be the wrong answer to that. Three properties are the reason it is worth sharing rather than reimplementing, and each has a test: - **A sourceless watcher parks rather than fires.** A kernel or sandbox that refuses the socket yields a watcher whose `changed()` never resolves, which is what makes it safe to `select!` against a poll ticker — the ticker simply always wins and the caller degrades to polling with no special case. - **A zero-length read does not spin.** `try_io` clears readiness only on `WouldBlock`, so breaking out of a zero-length read leaves `readable()` instantly ready with nothing to read, and the loop never returns `Pending`. That starves the caller's `select!` of every other arm — it is not a busy loop in the watcher, it is a livelock in whatever owns it. - **Giving up is sticky.** `changed()` is constructed fresh on every pass of a caller's `select!` and dropped whenever another arm wins, so a `pending()` inside the future parks nothing beyond the current pass. Without a flag on the watcher itself, the next pass re-reads the dead socket, re-counts the error and re-logs the give-up warning — once per wake-up, forever. The messages are deliberately not parsed. An event is a hint to re-ask whatever question the caller actually has, which is cheap and authoritative; decoding `nlmsghdr`/`ifinfomsg` to reach the same answer would add a parser whose bugs would become the caller's bugs. `RTNLGRP_LINK` carries link state only. A caller needing route or address events should extend `open_link_socket` with a group mask rather than opening its own socket — the note is in the module docs so the next caller finds it.
LinkWatcher hardcoded RTMGRP_LINK. That is the right subscription for the question it was built to answer — "is this interface here?" — and it stays the default: new() is unchanged in behaviour, and so is the interface binder behind it. It is the wrong subscription for a different question. A default route moving between two interfaces that both stay up produces no link message at all; `ip monitor` reports zero events in the link group for that change and two in the route group. A caller watching for a change of *egress path* rather than of interface presence therefore hears nothing, and falls back to its poll without any sign that the event source is not covering it. So open_link_socket takes the mask, new() passes the link group, and with_groups() takes an explicit one. The groups module names the five values and EGRESS_PATH combines them. Off Linux nothing changes and nothing can: PF_ROUTE has no group selection and delivers every routing message to every reader, so the mask is accepted and ignored there rather than being made conditional at every call site. The added tests pin that the wider mask binds — these are all read-only NETLINK_ROUTE groups needing no privilege, so a failure to bind means a wrong constant rather than a restricted sandbox, and would otherwise surface only as a silent downgrade to polling.
An established UDP peer gets its own connected socket for the send fast path. `open_connected_fd` binds the wildcard and then calls `connect(2)`, which makes the kernel resolve the route once and auto-bind the local source address to whichever interface was carrying it at that instant. It never re-evaluates, and the only code that drops those sockets is gated on the *peer's* address rotating, observed in the data plane. So a local medium change had no signal at all. A laptop moving between WLAN and LAN, a phone between Wi-Fi and cellular: every established peer went on transmitting from an address the routing table had abandoned, and the peer, which re-pins to whatever source address it last heard from, kept answering to that same dead address. The peering stayed marked connected the whole time and carried nothing, until `link_dead_timeout_secs` reaped it and the reconnect redid the Noise handshake and the tree position from scratch. On a live node, unpatched: route moved at 20:43:54, reaped at 20:44:24, reconnected at 20:45:16 — 82s, and the node lost its place in the tree on the way through. A local move is invisible in the data plane, which is exactly why it went unhandled: there was no event to hang a fix off. So this adds the missing signal and the reaction to it. The signal is a coarse fingerprint of the host's network attachment — the source addresses the routing table would pick for an off-link destination, plus the set of up, non-loopback interface addresses. A handover is not atomic, so a short debounce coalesces the burst into one event and a fingerprint that settles back where it started reports nothing. A backend only decides *when to look* — the comparison, the debounce and the settled-back suppression are shared — so platforms land behind the same seam without touching the reaction. The kernel backend is `transport::watcher`, the same one the interface binder uses, asked for the egress-path group mask rather than the link mask. Opening a second netlink socket beside it would have been two subscriptions to the same multicast groups, in one process, for the same reason. The shared watcher is also the better of the two by some distance: it survives a zero-length read that would otherwise spin a core flat, and it backs off and gives up on a persistently failing socket rather than re-logging once per wake-up forever, with a give-up state that outlives the future that decided it — which matters here, because `wait` is a fresh `select!` on every pass. Two consequences of taking it. macOS and FreeBSD get event-driven detection for free, because `PF_ROUTE` has no group selection and already delivered what this needs; the detector would have polled there only because it had no way to ask. And the fallback is simpler rather than more careful: `changed()` parks forever when it has no source and after it abandons a broken one, so selecting it against the poll timer degrades to the timer with no bookkeeping at all. The timer runs underneath the kernel source in any case, as a backstop — a netlink socket drops messages under memory pressure and the subscription can be refused outright in a restricted sandbox. The reaction is to drop the pinned sockets and heartbeat every peer at once. Dropping is self-healing rather than disruptive: the wildcard listen socket resolves a route per packet, so sends keep working immediately and a correctly-bound connected socket is reinstalled on a later tick. The heartbeat is what fixes the reverse direction — it carries the node's new source address, so the far side re-pins on receipt instead of waiting out its own heartbeat interval. Nothing here tears a peering down. Measured after: a WLAN/LAN switch in either direction costs no reconnection at all — Noise session, tree position and routes all survive. A scripted 8s LAN outage over 186 pings lost 5, in two gaps totalling 1.0s. The socket drop is Linux and macOS, the two platforms with the connected fast path; elsewhere the heartbeat alone carries the new address. Bluetooth is out of scope — an adapter's state is not an IP attachment and this detector cannot see it.
Reference entries for the `node.netmon.*` block and its three keys, plus the matching lines in the complete-reference listing so the block is discoverable by reading down the reference rather than only by knowing to look for it. Says what the poll interval actually means on each platform, since it is the detection-latency floor where there is no event-driven backend and only a backstop period where there is — the same knob meaning two different things, which is worth stating outright rather than leaving to be inferred. The platform table names both kernel sources: `NETLINK_ROUTE` multicast on Linux and the `PF_ROUTE` socket on macOS and FreeBSD.
The connected-socket fix had unit coverage and a hand-run on real hardware, neither of which a reviewer can re-run. This adds the integration suite: a multi-homed node whose default route moves between two live access paths while mesh traffic is in flight, with the far peer behind a router. Three things about the topology are load-bearing, and each of them was arrived at by watching the suite pass when it should not have. The far node has to be off-link. Reachable only through the router, the route to it follows node-a's default route, which is what the suite moves. The first version also installed an explicit /24 to the far segment on node-a "so it could get there" — that route outranks the default, so moving the default left the path to the peer exactly where it was. Detection fired, the sockets were dropped, and every assertion passed against a peer that had never moved. The router runs strict reverse-path filtering. Without it the suite proves nothing: both of node-a's interfaces stay up and both stay routable, so a packet still sourced from the abandoned path is forwarded and answered quite happily. The stale pin is real but harmless, and the negative control passes. A real gateway drops that packet as spoofed because the reverse route for its source points out a different interface — that is *why* a medium change black-holes traffic in the field, so it is the part the lab has to model. The outage metric counts the end of the observation window as a boundary. `ping -D` writes a line only for a reply, so an outage that never recovers simply stops producing lines, and the largest interval between two surviving replies stays one ping apart. A total blackout scored as zero. The control run is what surfaced it: 16 replies, all of them from before the move, reported as 0.20s of disruption. The assertions are about continuity rather than reachability, because a peering the liveness reaper tore down and a re-dial rebuilt also ends with traffic flowing. Unchanged link_id and authenticated_at_ms on the near node say no second handshake ran; a changed transport_addr on the far node says it re-pinned to the new source; a bounded ping gap says the data plane genuinely carried through. The third is what stops the first two passing vacuously on a topology where nothing moved. Phase 3 repeats the move with node.netmon.enabled = false and requires the outage, so the suite demonstrates the regression instead of asserting it from a changelog entry. Measured on this branch: 0.21s and 0.41s for the two directions with detection on, against a control that stayed dark for the whole 12s window. Wired into both runners, so `ci-local.sh --only medium-change` and the GitHub matrix leg stay the same claim; check-ci-parity.sh reports 13 suites and 23 legs on each side.
69f128e to
a4620eb
Compare
Android is `target_os = "android"`, not `target_os = "linux"`, so it took the non-Linux arm of `open_link_socket` and opened a `PF_ROUTE` socket. That is a no-op on a platform with no route socket. On Android it is worse than a no-op, because libc's shared `linux_like` module defines `PF_ROUTE = AF_ROUTE = AF_NETLINK` (16) and the protocol argument `AF_UNSPEC` is `0`, which is `NETLINK_ROUTE`. The call is therefore `socket(AF_NETLINK, SOCK_RAW, NETLINK_ROUTE)` with no group subscription. It succeeds, so `inner` is `Some`, `is_event_driven()` returns true, and the node logs "Network-change detection: kernel events" while nothing but the poll backstop ever fires. Nothing was broken in the data plane by that: detection still happened on the timer, and the immediate heartbeat that makes the peer re-pin still ran. What was broken was the report. The reference says `poll_interval_secs` is only a backstop where an event-driven backend exists, so an Android operator read 5 seconds as a ceiling when it was the floor — on the platform that changes medium most often, and the one this detector exists for. So the five predicates that decide which arm a platform takes widen to `any(target_os = "linux", target_os = "android")`: the `groups` module, both `open_link_socket` arms, `LinkWatcher::new`'s inner gate, and the backend choice in `build_wake_source`. The platform tables in the module docs and in the configuration reference are corrected to match. Widening cannot make Android worse. If the socket or the group bind is refused — SELinux policy for an untrusted app is the obvious case — `open_link_socket` returns an error, `with_groups` leaves `inner` as `None`, and `build_wake_source` falls to the timer. That is exactly today's behaviour, with an honest log instead of a wrong one. This lands unexercised either way. The `android-check` CI leg is a `cargo check`, so it compiles the code and cannot see a socket that opens and stays silent, and nothing in CI runs on a device. Verified as far as that allows: the widened arm needs `AF_NETLINK`, `SOCK_RAW`, `SOCK_NONBLOCK`, `SOCK_CLOEXEC`, `NETLINK_ROUTE` and `sockaddr_nl`, and all six are defined for Android in the pinned libc 0.2.189, the last three in the android module itself. Not run on a device. Two Android items are deliberately not here, both tracked separately: bionic's `getifaddrs` restriction for apps targeting API 30+, which likely degrades the fingerprint to the source-address probe alone, and a `ConnectivityManager` push, which is the durable signal under Doze where a timer is not reliable.
The reaction to a medium change ran on the rx loop and awaited a heartbeat to every peer in turn. For a peer on TCP, Tor, Nym or BLE that reaches `writer.lock().await` and then a bare `write_all(data).await`. The connect above it is wrapped in a timeout; the write and the lock are not. A medium change is precisely the condition that leaves a send window full against a path that has just gone away, so that write could hold the rx loop — every other arm of the select with it — for as long as the stranded socket took to fail. The fan-out now covers connectionless transports only. That is not an optimisation, it is the whole point: a UDP send completes without awaiting the wire, because the fast path hands the frame to the encrypt workers and returns, and a raw datagram write does not wait for a peer. Neither obvious alternative works. A timeout around the send is unsafe, not merely insufficient: dropping a partial `write_all` leaves a half-written frame on the stream, which the peer cannot resynchronise from. And the loop cannot simply be spawned, because the send needs `&mut self` for the session counter and the MMP sender record, and neither `Node` nor `TransportHandle` is shared or cloneable. A connection-oriented peer is not stranded by the omission. It keeps the periodic heartbeat it had before this detector existed, its transport re-dials on send, and `link_dead_timeout_secs` remains the backstop — so this is the behaviour that shipped in v0.5.0 for those peers, unchanged, rather than a regression introduced here. Doing better for them means dropping the stale connection rather than writing into it, which is a different change with a real cost behind it: a Tor peer would pay a fresh circuit every time the fingerprint moved, and the fingerprint currently moves for things like a docker bridge appearing. The handler's `info!` now reports how many heartbeats actually went out, rather than leaving it inferred from the peer count. Covered by a regression test that re-pins an established peer onto a TCP transport and asserts its heartbeat timestamp does not move across a change. Verified by mutation: with the filter replaced by `true`, the test fails.
|
All four actions are in, and the two Android follow-ups are filed as #145 and What the address set is forShort answer: less than the module docs claim, and one of the two jobs I wrote The docs at What genuinely remains is one case, and it is real: a peer whose path is not So the address set is a proxy for "could the local end of one of our Why I would not narrow it the way you suggestedDropping The filter is not narrow in the wrong dimension. It is enumerating the wrong What I think the right shape isProbe per peer instead of enumerating interfaces: for each peer with an IP That is strictly better on every axis you raised:
Costs and caveats, so this is not oversold. It is two syscalls per peer per I have not built this. Happy to do it here or as a follow-up — it is a One note on the cost you measured, which is now slightly lower: The four actions
I did not extend the fan-out to dropping those peers' stale connections, though
|
…is for One of the two jobs the module docs gave it does not exist. "The address a link-local Ethernet transport peers over" is wrong: that transport is pure L2 — `AF_PACKET`/`SOCK_DGRAM` on Linux, BPF on macOS, peers addressed by MAC — and an Ethernet peering neither has nor needs an IP address on the interface. Its addresses appearing or vanishing say nothing about that peering. What remains is one case, and it is the real one. A peer on the same LAN is reached by the on-link subnet route, while `preferred_source` probes an off-link destination and so follows the *default* route by construction. It looks straight past that path. Unplug a LAN cable on a host whose default route is cellular: the source addresses do not move, and every connected socket to that peer is stranded. Also names the set for what it is — a proxy for "could the local end of one of our peerings have moved" — and says plainly that it is a broad one, since it enumerates every address the host has rather than the local end of the peerings the node holds. A container bridge or a tunnel interface appearing moves it too.
`test.sh` renders the node configs from the topology before compose starts and leaves them in place for post-mortem, under a directory named with the run suffix — so every run leaves another one behind in the working tree. Same reason `/testing/docker-*/` is already ignored.
|
The doc line I flagged above as wrong is fixed in |
|
Landed on I want to be straight about the basis for merging it. Your per-peer probe removes the whole category, since a virtual interface appearing does not change the source address the kernel picks for any actual peer. It also closes the on-link case you found yourself. So I am treating the probe as owed rather than optional, and this lands on the understanding that it follows. Detail at the end. Three things changed as it went in. I squashed the nine commits to two, one per author, so Arjen's watcher stays his and yours is one commit; that consumes his One thing to flag: Six smaller follow-ups, none of which held the merge:
On the probe itself: the premise holds and it does subsume both current signals, so I am not asking you to narrow the shape, only to build it. Two conditions when you do. Compare the intersection of consecutive samples over a per-peer map rather than a set, so peer churn cannot fire the fan-out on its own. And reach the peer table through That also settles #145: the probe deletes |
3 participants
Fixes #142.
Targets
masterand carries @Origami74's watcher (dfc1dd2, his authorshipintact) as its first commit, per review — so
LinkWatcherlands with itsconsumer rather than entering the public library API with no caller on
master. Rebased ontof149cba; the only file of his this touches is thewatcher itself.
Commits reordered so
src/node/netmon/linux.rsnever exists in any tree: thewatcher-groups change now precedes the fix, and the fix is one commit that was
already driven from the shared watcher. Verified commit by commit.
The Android
cfgwidening is in as its own commit (f769414): five predicatesto
any(target_os = "linux", target_os = "android"), plus the three platformtables that claimed Android polls. Confirmed the libc chain rather than assuming
it —
PF_ROUTE = AF_ROUTE = AF_NETLINK(16) andAF_UNSPEC = 0 = NETLINK_ROUTEin the pinned 0.2.189, so the old call really was an unbound
NETLINK_ROUTEsocket — and all six symbols the widened arm needs are defined for Android, the
last three in the android module itself. Not run on a device, and I have no
NDK here, so the CI
android-checkleg is the only compile evidence.The rx-loop hold is addressed in
b1a1043, but not by spawning — that doesnot compile. The send needs
&mut selffor the session counter and the MMPsender record, and neither
NodenorTransportHandleis shared or cloneable,so the fan-out cannot be moved to a
'statictask without restructuringownership. A timeout around the send is worse than insufficient: dropping a
partial
write_allleaves a half-written frame the peer cannot resynchronisefrom.
What it does instead is skip the peers whose send can block. A connectionless
send never awaits the wire — the UDP fast path hands the frame to the encrypt
workers and returns — so the unbounded
write_allis simply not reachable fromthis path any more. A peer on TCP, Tor, Nym or BLE keeps the periodic heartbeat
it had before this detector existed, so for those peers this is v0.5.0
behaviour unchanged rather than a regression introduced here.
I did not extend it to dropping their stale connections, though that is the
honest analogue of dropping a stranded UDP socket, because the cost is not
symmetric: a Tor peer would pay a fresh circuit every time the fingerprint
moved — and per your
interface_addrs()note, the fingerprint currently movesfor things like a docker bridge appearing. That seems worth settling together
with the filter question rather than deciding here.
Covered by a regression test that re-pins an established peer onto a TCP
transport and asserts its heartbeat timestamp does not move. Mutation-checked:
replace the filter with
trueand the test fails.All four review actions are now in. Outstanding on my side: the two Android
follow-up issues you asked for.
Symptom
Every established UDP peering black-holes for ~80s when the host changes
transport medium — WLAN↔LAN, Wi-Fi↔cellular — and
fipsctl show peersreports the peer as
connectedfor the first 30s of it, with MMP reportingloss=0.0%. To an operator it presents as "connected but dead", not as adisconnect. When the reaped peer is the node's only peer it also loses its
tree position and every routed destination goes with it.
Measured unpatched on a live node: route moved at 20:43:54, reaped at
20:44:24, reconnected at 20:45:16 — 82s, tree position lost on the way
through.
Root cause
open_connected_fdbinds the wildcard and then callsconnect(2), whichmakes the kernel resolve the route once and auto-bind the local source
address to whichever interface carries it at that instant. It never
re-evaluates. The only site that drops those sockets is gated on the
peer's address rotating, observed in the data plane — the mirror-image
case, handled correctly.
A local move produces no data-plane event, so there was nothing to hang a
handler off. That is why it went unnoticed rather than unhandled.
Fix shape
Two halves, because the missing piece was the trigger rather than the
reaction.
The signal is a coarse fingerprint of the host's network attachment: the
source addresses the routing table would pick for an off-link destination,
plus the set of up, non-loopback interface addresses. A handover is not
atomic, so a short debounce coalesces the burst into one event and a
fingerprint that settles back where it started reports nothing. Linux reacts
in milliseconds off
NETLINK_ROUTEmulticast; every other platform sampleson a timer, which also runs underneath netlink as a backstop — a netlink
socket drops messages under memory pressure and the subscription can be
refused outright in a restricted sandbox. A backend only decides when to
look; the comparison, debounce and settled-back suppression are shared.
The reaction is to drop the pinned sockets and heartbeat every peer at
once. Dropping is self-healing rather than disruptive: the wildcard listen
socket resolves a route per packet, so sends keep working immediately and a
correctly-bound connected socket is reinstalled on a later tick. The
heartbeat fixes the reverse direction — it carries the new source address, so
the far side re-pins on receipt instead of waiting out its own heartbeat
interval. Nothing here tears a peering down.
The socket drop is Linux and macOS, the two platforms with the connected fast
path; elsewhere the heartbeat alone carries the new address. Bluetooth is out
of scope — an adapter's state is not an IP attachment and this detector
cannot see it.
What this needed from the watcher
One additive change.
LinkWatcherhardcodedRTMGRP_LINK, which is rightfor the question it was built to answer — "is this interface here?" — and
stays the default:
new()is unchanged in behaviour, and so is the interfacebinder behind it. It is the wrong subscription here, because a default route
moving between two interfaces that both stay up produces no link message at
all.
open_link_socketnow takes the mask,new()passes the link group,and
with_groups()takes an explicit one.Two consequences worth naming:
PF_ROUTEhasno group selection and already delivered what this needs; the detector was
polling there only because it had no way to ask. Reasoned, not measured —
I have run this on Linux only and have no BSD host.
changed()parksforever when it has no source and after it abandons a broken one, so
selecting it against the poll timer degrades to the timer with no
bookkeeping.
Measurements
Same hardware, LAN and cellular both up, crossing genuinely different public
IPs (fixed line vs cellular):
link_idandauthenticated_at_mswere unchanged across both, so thepeering was absorbed rather than torn down and rebuilt. That distinction is
the point: a reconnect fast enough to look similar is not the same outcome,
and the suite asserts against it rather than against "traffic came back".
Test coverage
New integration suite
testing/medium-change/: a multi-homed node whosedefault route moves between two live access paths, far peer behind a router.
Wired into both runners —
check-ci-parity.shreports 13 suites, 23 legseach side.
Three things about the topology are load-bearing, each arrived at by watching
the suite pass when it should not have:
to the far segment, which outranks the default — moving the default left
the path to the peer exactly where it was, and every assertion passed
against a peer that had never moved.
node's interfaces stay routable, a packet from the abandoned path is
answered happily, and the stale pin is real but harmless. A real gateway
drops that packet as spoofed — that is why a medium change black-holes
traffic in the field, so the lab has to model it.
ping -Dwrites a line only for a reply, so an outage that never recovers stops
producing lines and a total blackout scored as zero disruption.
Phase 3 repeats the move with
node.netmon.enabled = falseand requiresthe outage, so the suite demonstrates the regression rather than asserting it
from a changelog entry.
Unit coverage sits in
src/node/netmon/tests.rsandsrc/node/tests/netmon.rs(20 tests). One is
#[ignore]-gated onCAP_NET_ADMIN:a_route_change_alone_reaches_the_watcheradds a route with no link changealongside it, and under
unshare -rnpasses on the egress-path mask andtimes out on
RTMGRP_LINK. No CI leg currently runs it underunshare, soit documents the link-vs-route distinction rather than enforcing it — say the
word if you want a leg for it.
Docs
docs/reference/configuration.mdgains thenode.netmon.*block and itsthree keys, plus the matching lines in the complete-reference listing. The
poll interval means two different things by platform — a detection-latency
floor where there is no event-driven backend, a backstop period where there
is — which the entry states outright.
Gates
cargo fmt --check,cargo build,cargo clippy --all-targets -- -D warnings,cargo clippy --all-targets --all-features -- -D warnings,cargo test(2424 passed, 0 failed), and
./testing/ci-local.sh --only medium-change(17/17, suite itself 8/8).
Note
Related but distinct: #130 (peer re-handshaking from a new transport address
stalls). The teardown-and-redial this bug forces is one way to reach #130's
precondition, so fixing this removes a common route into that issue without
addressing it.