fix(mesh): fleet census defects — ghost punches, NAT-blind coordination, advertise egress, two data races - #26
Merged
Conversation
A datagram session registered the moment its Noise handshake crossed — which only proves the path at handshake time. The far side may have refused its half of the session (capacity, duplicate yield, allowlist) and closed it, and a datagram carrier never signals the refusal, so the dialer registered, pinged into the void for six unanswered probes and died "one-way path" at ~37s. In one 150s field census that was 20 of 68 session closes, all origin holepunch_udp with inbound_packets=0, plus a graft/prune war over each ghost until its death. Registration now waits for a mesh-level reply through the same candidate: - Dial side (connectPeerUDPWithHint): after DialPeerContext, ping probes run every 250ms and the first inbound envelope on the session proves the path before registerPeerFrom. Silence returns a dial error, so the punch plan keeps hunting instead of stopping at a ghost and the dial budget charges the candidate — no resetHostDialState for a one-way path. - Accept side (acceptUDPLoop): the same confirm runs per accepted session in its own bounded goroutine, replacing the inline registration that let one slow candidate stall every later handshake. - registerPeerFrom reports whether the session became a peer so the confirm phase can treat a silent refusal (capacity, duplicate, stopped) exactly like a dead path. - The confirming packet is replayed through the normal dispatch path, so the far side's ping gets its pong and the two confirm phases converge in one round trip. Both sides send the first probe at once, which keeps pre-fix peers (registered on handshake, pongs only) confirming in about one RTT. - Stream sessions (TCP, masq, veil, webrtc) are untouched: their close is a signal the peer across the wire can observe. Census after the fix (150s, public mesh): zero one-way closes, zero holepunch_udp closes, 184 opens with live long-hold sessions; the remaining close class is the TCP glare oscillation tracked as bug 4.
Every early punch in the field census logged target_nat:"" — 17 of 18 punches in one 150s run — because a peer's NAT type only ever arrived with the signed announce flood, on its own cadence, while the punch coordination offer and reply, the only envelopes a fresh node exchanges with a punch target before that flood lands, carried an address and nothing else. The result was a punch layer that spent its first rounds NAT-blind: telemetry could not say why a punch failed, and the relay preference (shouldPreferRelayForTarget) could not classify a symmetric-on-symmetric pair to skip a punch that cannot complete. The coordination offer and reply now carry the sender's self profile — the same facts a signed supernode-status announce carries (NAT type, public reachability, relay capability) — signed under a new moss-punch-coord payload domain, distinct from supernode status so a signature from one envelope type never verifies as another. The receiver applies a profile only from a valid self-signed claim naming the coordinating peer, into an existing directory entry, with the same natTrusted semantics as the supernode announce path; an unsigned or forged envelope changes nothing beyond the address, which is exactly the behaviour of a legacy peer on the wire today. A node that has not classified its own NAT sends an unsigned envelope rather than a signed "unknown" that would overwrite better knowledge. Punch attempt and result events now read the target's NAT type from the directory at emit time, so a reply that lands mid-punch classifies the same round's result instead of waiting for the next one. Legacy compatibility is total: fields are omitempty, receivers gate on the signature, and a fleet on the previous version teaches nothing and is taught the same as before. The census after this change shows the mechanism working against the first fixed peer it punches (t+142s target_nat:port_restricted_cone) while legacy targets keep their addresses flowing as before.
…ions The census kept ~150 "duplicate connection" closes per 150 seconds, all origin dial_tcp, held times of 0.0002-0.12s, inbound_packets:0 — a self-sustaining glare against the same handful of live masq hosts. Two mechanisms fed it, and the dial budget never learned either: - The kick and seed passes selected targets by exact address, so a host with a live session kept offering its OTHER port records — stale binds, or the same masq listener, which terminates every port at one node. The pass spent both dial slots on full handshakes into instant duplicate yields, every round. Selection now skips all port records of a host that has a live session; the skip is live-session-dependent, so a host whose session dies is dialable again, and a host running several distinct nodes stays reachable through the announced directory, which dials by peer identity. - A dial reported success the moment registration completed. A full peer closes the session microseconds later, before a single packet crosses, and the success reset deleted the cooldown the death was about to write — two racing writers on one verdict, and the reset kept winning: the same live host was re-dialled every announce round for the whole run. The race is closed on both ends now. removePeer charges a session that died inside the refusal window without any packet as a dial failure, for the peer and its host, exactly like the ping-death path; and the kick/seed outcome charges success only after the session has outlived that same window, so whatever the far side is going to do has already happened by the time the answer is read. The refusal signature is safe to charge: a replaced session is closed after the directory already points at its successor, so removePeer's identity guard never runs for it, and a confirmed datagram session always has inbound packets. The window is a var so tests can compress it. After the fix the glare is self-limiting instead of flat: the census runs show 151, then 79, then 52 duplicate closes per 150s with the per-peer gap doubling (10s, 20s, 40s, 80s...) as backoff accumulates, while live peers climb 12 -> 18 -> 22 and the known directory stays ~450. The residual closes are the fleet's own churn against this node — their side, on the version before these fixes.
A static peer dial is operator intent, but it entered the network through two gates that did not treat it that way: - The transport choice was gated on address rank: anything loopback or private went to a TCP-only dial, both at Start and on every seed retry. A peer whose TCP was refused but whose UDP ear was alive never formed — the idlepair repro ran a full 60 seconds against a live UDP mesh endpoint and ended with zero peers and nothing but "connection refused" dial lines. Static peers now dial both transports on every path: the initial dial, the kick, and the seed retries. - The initial dial at Start ran on the root context — no handshake budget. A static peer that was down at that moment held the transport's client slot for its address forever: every later retry got "udp handshake is already in progress" without sending a packet, so a temporarily dead static peer could never recover, and the UDP fallback never got a chance either. The initial dial now carries the same bounded handshake budget as the kick and seed paths, and static peers dial in parallel instead of serially through the paths they share with discovery. The fallback is sequential, not a race: a parallel UDP leg churned the peer slot on LAN topologies that budget exactly one session, so TCP goes first and UDP fires only when TCP produced nothing. Public static peers keep the parallel dual dial the bootstrap path already used for them. After the fix the harness pair forms in under a second when the static peer's TCP is refused (OPEN origin holepunch_udp, peers_final 1), where it formed not at all before. The dead-both-transports case stays at zero, and the bounded initial dial frees the address for the seed retries.
… write overlayLookup fanned its alpha query goroutines out to write results[i] while the outer select could already have given up (ctx.Done or the 12s batch bound); the reader loop then ranged over the same slice the parked stragglers still owned. The race census caught this twice on the public fleet and once on clean e43d572 (worktree), but CI stayed blind because the harness test is not in the tree. Results now flow through a buffered channel sized to the batch: the reader drains only what has arrived, and a straggler's late send lands in the buffer instead of racing the loop. The 12s batch bound stays, the per-query 4s deadlines stay, and a timed-out contact is still never dropped from the table. The new unit pins the defect deterministically against silent transport-only contacts, so the CI Race job can see it.
…wn-endpoint observations Bug №5. A public box with a docker user-defined bridge advertised the bridge to the whole mesh: interface preference is deliberately private-before-global (LAN pairs depend on it), so br-<id> (172.26.0.1) beat the public eth0, and only the first STUN correction pulled the advertise back — never, on a UDP-blackholed host. The same preference fed the NAT profile: a vantage point reporting the host's own address:port back is no NAT evidence, but the binding classifier graded the stable observation as a cone mapping, pinning both directly public stand hosts at port_restricted_cone forever (the public label only ever promotes from Unknown). Three changes: - the default route's source address (a bound UDP dial — no packet leaves — via the node's bindIfIndex) leads the advertise chain for a public host, ahead of peer-subnet and interface preference; a non-global source (LAN egress, CGNAT, loopback) falls back to the old chain unchanged, so LAN behaviour is untouched. Seam: defaultRouteAdvertiseHostFn, overridable per test. - isVirtualOverlayInterfaceName now covers docker's br-<id> bridges, veth/calico pair ends, virbr, podman, netavark, weave, flannel, cilium, cni0 and ztnet — none of them may ever win an advertise or a NAT observation walk again. - observations whose host is the node's own egress address at its own listen port no longer reach the binding classifier (applyObservation and freshObservedUDPAddr): they are recorded as external addresses — which they are — and classification waits for genuine NAT evidence. A host behind NAT reports the router's address and classifies exactly as before (pinned by the non-egress control test). Stand census before/after: nat=port_restricted_cone on both public hosts before; after, the local stand promotes honestly to public once a peer confirms inbound, and neither host mints the cone verdict anymore. Advertised address is the public egress from t+0, not a bridge.
Stop() does not join probePortMapping: it is deliberately wg-untracked, bounded by its own STUN/mapping timeouts, and its contexts are Background-derived, so cancelling rootCtx does not interrupt it either. A restart's Start() then assigns a fresh n.udpListener to a plain field while the previous run's probe is still inside its STUN windows — the race census caught that unsynchronized read-while-write pair (TestPublishIsDeliveredAfterRestart under -race). The field becomes an atomic pointer, following the masqDialer precedent in node_types.go for exactly this reader class (goroutines Stop does not track). Start stores; every reader — the probe's STUN helpers, the UDP accept loop, punch/bootstrap dials — loads once into a snapshot and works on it. A late probe's load sees the previous (closed) listener and its STUN calls fail cleanly; nothing else changes. The new restart unit pins the pair deterministically: a TEST-NET tracker turns the STUN bootstrap on with no network, three Stop+Start rounds produce the race before the fix and silence after.
A standalone diagnostic binary for pair experiments over the real transport: --dial runs a node against one static peer, --trackers joins the built-in public trackers, --secs bounds the run. It prints the peer/known/relay counts on change, the advertised address and NAT type on change, and a JSON verdict carrying every session close from the debug ring — the census shape that pinned bugs №2 through №7. Kept as a command, not a test: by design it can join the real public mesh, so CI must never run it.
🤖 CodeAnt AI — Review Status
|
Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
codeant-ai
Bot
added
the
size:XXL
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| env.AdvertisedNATType = string(info.natType) | ||
| env.AdvertisedReachable = info.publicReachable | ||
| env.AdvertisedRelayCapable = info.relayCapable | ||
| return n.signHolePunchCoordEnvelope(env) |
There was a problem hiding this comment.
Suggestion: This newly trusted coordination signature does not bind the request or timestamp, allowing an old signed NAT profile to be replayed in later punch exchanges.
Assessment: 🟠 Major · 🔁 Occurrence: Rarely · 🏷️ Security
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** internal/mesh/node_holepunch.go
**Line:** 194:194
**Comment:**
*Security: This newly trusted coordination signature does not bind the request or timestamp, allowing an old signed NAT profile to be replayed in later punch exchanges.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Contributor
Author
There was a problem hiding this comment.
Resolving without a change. The signature binds sender + address + NAT profile + reachability under a punch-specific domain, so the claim cannot be forged — the finding is about replaying an old but authentic profile. Freshness would require a nonce/echo protocol (or a timestamp field plus a clock-skew policy) in the coordination envelope: a wire-format change across a deployed fleet, not a localized patch. The stale-profile worst case is a suboptimal punch decision (an outdated NAT classification), not privilege — and the receiver still re-learns the profile from the signed announce flood within seconds. Tracked as a hardening candidate for the next envelope version rather than a defect in this one.
There was a problem hiding this comment.
✅ Customized review instruction saved!
Instruction:
Do not flag the lack of replay freshness or nonce binding in the current punch coordination envelope; stale signed NAT profiles only affect punch optimization and are re-learned from announce floods.
Applied to:
internal/mesh/node_holepunch.go
💡 To manage or update this instruction, visit: CodeAnt AI Settings
CodeAnt Nitpicks5 code suggestions1.
|
DeterministicAddr hashes identity into the pool, so two peers tie with 1-in-pool odds per pair — a /24 makes that one collision per ~250 pairs. CI proved it live: TestLanNodePresenceRoundTrip failed on windows with both nodes claiming 10.66.0.92, while the comments called the odds astronomically unlikely and documented last-writer-wins as the resolution. Last-writer-wins untangles nothing: both nodes keep heartbeating from the same address and the LAN stays broken. The presence layer now detects the tie (a remote heartbeat claiming OUR self address) and both sides apply the same deterministic rule: the greater peer ID keeps the address, the lesser re-derives itself under a salt and announces the new address on its next beat. Exactly one side moves, no negotiation, and the relocation chain terminates because each retry hashes a different input. The routing table follows through the regular registration path. Pinned at three layers: the odds are real (a brute-force colliding pair in a /24), the collision hook fires only on a genuine self-address claim (not on different addresses, own echoes, or stale envelopes), and the product path re-homes a live LanNode — new self IP, salt advanced, presence retargeted, routing table updated, stable against repeat claims of the abandoned address. The heartbeat now reads the announced IP under the table lock so a concurrent retarget cannot race it.
…aths Four findings from the PR review, each a real defect the review bot caught on the census fixes: - connectStaticPeer treated connectPeer's nil as 'connected', but clientHandshakeAndRegister ignores registerPeerFrom's verdict, so a session the node refused (capacity, allowlist) suppressed the UDP fallback exactly where it was needed. The fallback now keys on hasPeerAddr — a real session at the addr — not on the return code. - awaitUDPConfirm confirmed on ANY decrypted packet: the transport layer only promises Noise decryption, and a malformed or unrelated datagram registered a session the far side never spoke on. Confirmation now requires a packet that parses as a mesh envelope — every legitimate reply is one, so a live peer is never delayed. - bootstrapDialSucceeded keyed its refusal-window wait on the dial's context: a dial whose budget expired before the window closed read as success while the peer was still about to refuse, resetting the host's backoff seconds before the refusal charged it. The window now runs on its own timer, always to completion. - a register-then-die-in-window verdict was charged twice: removePeer's instant-refusal path charged the host, then the outcome note charged it again, doubling the machine's backoff for one refusal event. The verdict is now tri-state (alive / refused / plain failure) and the refused shape charges only the addr, never the host. - the egress probe's 2s dial timeout ran inside mapping observations whose caller budgets are a few seconds; a wedged sandbox connect could eat most of one. The probe is a local route lookup — 250ms is three orders of headroom on it.
…oker Pre-existing flake, untouched by this branch, caught by CI on the PR: TestMqttLinkPingKeepalive asserted PingsSent immediately after the broker-side PINGREQs arrived, but the pinger counts a ping only AFTER writePacket returns — the broker observes the packet the moment the write hands it off, so on a loaded runner the assertion could read the counter before the increment landed (CI read 1 with two PINGREQs already delivered). The assertion now waits for the counter to catch up, bounded like every other wait in the file.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
User description
Problem
The stand census — 150s pair runs against the public fleet — pinned seven
defects on top of the dial-budget fix (#25): one-way ghost punch sessions,
NAT-blind hole-punch coordination, dial churn against live hosts, static
peers that could never meet, a lookup data race, a public box advertising
its docker bridge address, and a restart race on the UDP listener.
How it's solved
One commit per defect, each landing test-first: the regression tests are
deterministic (fabricated interfaces, ghost transport listeners, TEST-NET
routing seams), were red before their fix, and now run under the CI race
detector.
after a mesh-level reply through the same candidate. A silent candidate
fails the dial and charges the budget instead of squatting a peer slot
until its 37s one-way death.
profile (NAT type, reachable, relay-capable) rides offer/reply, so early
punches can classify symmetric pairs instead of firing NAT-blind.
Unsigned legacy envelopes change nothing.
duplicate-connection glare cycle becomes self-limiting; seed and kick
passes no longer spend both slots on other ports of a host that already
holds a live session.
peer now meets over UDP fallback, and a dead static releases its dial
slot instead of blocking every later dial.
alpha batch reports through a buffered channel; an abandoned round
drains only what arrived. Pre-existing on main; previously only
reproducible under
-raceagainst the live fleet.observations — a public host advertises the address its default route
leaves from instead of a container bridge; docker/CNI/veth interfaces
are excluded by name; and an observation of the node's own egress
endpoint no longer mints a
port_restricted_coneverdict that locks adirectly public host out of the public label forever.
NAT probe can outlive
Stopinto the nextStart; the listener fieldbecomes an atomic pointer (the same pattern as masqDialer) and every
reader works on a snapshot.
numbers. Kept as a command, never a test: by design it can join the
real public mesh, so CI must not run it.
Verification
go test ./internal/mesh/green (~300s); vet/gofmt clean;-racegreen on every touched path, including the two races thisbatch pinned for CI (overlay lookup batch, restart listener swap).
duplicate churn down, and both directly public stand hosts advertise
their real address from t+0 instead of a bridge, with the NAT label
promoting honestly to
publiconce inbound reachability is confirmed.Produced with MiniMax Code (GLM-5.3) under the Mavis terminal agent
harness, working from the fleet-census handoff in the mosh-flutter
workspace.
CodeAnt-AI Description
Prevent ghost UDP peers and improve mesh connectivity
What Changed
idlepairexperiment command and regression coverage for UDP confirmation, static fallback, NAT coordination, address selection, dial backoff, restart safety, and lookup cancellationImpact
✅ Fewer one-way UDP ghost peers✅ Static peers recover through UDP fallback✅ Fewer duplicate handshakes against connected hosts💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.