network: detect a wedged discv5 socket (operator health check + boot-node /healthz) - #2982
network: detect a wedged discv5 socket (operator health check + boot-node /healthz)#2982iurii-ssv wants to merge 3 commits into
Conversation
Greptile SummaryThe PR adds socket-read tracking so operator and boot-node health checks can detect wedged discv5 listeners.
Confidence Score: 5/5The PR appears safe to merge because no blocking failure remains established. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| network/discovery/timed_conn.go | Adds an atomic timestamping wrapper around successful discv5 UDP reads. |
| network/discovery/dv5_service.go | Routes the post-fork listener through TimedConn and exposes discovery staleness. |
| network/p2p/p2p.go | Extends operator health reporting to reject stale discovery sockets. |
| utils/boot_node/health.go | Implements boot-node health evaluation from routing-table occupancy and socket-read freshness. |
| utils/boot_node/node.go | Wires TimedConn into the boot-node listener and serves the new health endpoint. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
UDP["discv5 UDP socket"] --> TC["TimedConn records successful reads"]
TC --> OP["Operator DiscoveryStale"]
OP --> PH["p2p Healthy"]
PH --> OW["Operator watchdog restart"]
TC --> BH["Boot-node health check"]
RT["Routing table state"] --> BH
BH --> HZ["/healthz"]
HZ --> BL["Boot-node liveness restart"]
Reviews (2): Last reviewed commit: "utils/boot_node: add /healthz tied to di..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is
☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
7bc0d99 to
c9d0cc1
Compare
|
@greptile pls re-review |
There was a problem hiding this comment.
Sound, well-reasoned design: the socket-read timestamp is a genuinely cause-agnostic wedge signal, it is wired to the only listener that actually drains the socket, and greptile's 'quiet socket' P1 is properly fixed by the populated-table gate (verified against geth's 3s revalidation interval, not just the comment thread). The one thing worth changing before merge is the operator path, which cannot distinguish 'wedged' from 'never received any UDP' and will crash-loop a node that restarts while its bootnodes are unreachable; the rest are minor robustness/observability/test-wiring points. [verdict: with_fixes]
| // A wedged discv5 socket leaves discovery silently dead while bootstrap keeps | ||
| // looping; surface it so the hprobe watchdog restarts the node. n.disc is nil | ||
| // in tests and briefly at startup, hence the guard. | ||
| if n.disc != nil && n.disc.DiscoveryStale(discoveryStaleGrace) { |
There was a problem hiding this comment.
Finding 1 · [IMPORTANT but debatable] Arm the operator staleness check only after the first successful read — otherwise a cold start with unreachable bootnodes becomes a self-amplifying restart loop
Mechanism. NewTimedConn seeds lastReadUnixNano at construction time (network/discovery/timed_conn.go:34), which happens inside initDiscV5Listener during p2pNetwork.Setup(). From that instant the 3-minute clock is running, and StaleFor cannot tell apart:
- the wedge — the socket was being drained, then the read loop stopped; and
- never drained anything — the socket is fine and the read loop is healthy, but no inbound UDP has arrived at all.
Both produce an identical lastRead that stops advancing. The boot node explicitly guards against this by gating on a populated routing table (utils/boot_node/health.go:68); the operator deliberately does not, so case (2) reads as a wedge.
When case (2) actually happens. An operator's socket stays fresh as long as its discv5 table has any peer, because those peers are revalidated every ~3s (geth p2p/discover/common.go:72, table_reval.go:50-53) and their pongs are reads. So the exposure is precisely: an operator whose discv5 table is empty and whose FINDNODE/PING traffic gets no answer — i.e. a node that has just (re)started while the bootnodes it is configured with are unreachable, or a node whose UDP is fully blocked by a firewall.
Why it matters — the timing makes it a loop, not a blip. Walking the actual bring-up path in cli/operator/node.go: startNetwork runs p2pNetwork.Setup() (line 668) and registers the p2p prober component (line 674), then startHealthProber is spawned (line 572) and probes immediately. So:
T+0— socket bound,lastReadseeded, first probe passes.T+3min—DiscoveryStaleflips true. This is a level signal, so it does not recover acrossprobeComponent's retries.~T+5min— the 6 attempts × 10s + 5 gaps × 10s burn down,startHealthProberreturns an error, and the node exits non-zero for the orchestrator to restart (cli/operator/prober.go:57-60).- restart → table empty again → still no inbound UDP → repeat.
The loop is self-amplifying: every restart throws away whatever table the node had, guaranteeing the next cycle also starts from empty. And it is correlated across the fleet — a bootnode outage (the very failure class #2979 is about) would now take down every operator that happens to restart during it, and keep them down, where previously they would have come up with degraded discovery and recovered on their own once a bootnode returned. A node with an established libp2p peer set that loses only UDP is also killed, dropping working TCP connections and its duty participation, even though the restart cannot fix an upstream network block.
Note this does not weaken the detection you want. #2980's wedge (blocking send on Unhandled) can only trigger after packets have been received and forwarded — the channel has to fill first. The same is true of the boot-node incident in #2979 ("the application stopped draining it"). So requiring at least one successful read before arming the check keeps every wedge in scope while removing the entire false-positive class.
Suggested fix: Track whether the socket has ever been read, and only treat staleness as a wedge once it has:
// timed_conn.go
type TimedConn struct {
discover.UDPConn
lastReadUnixNano atomic.Int64
reads atomic.Uint64 // successful reads since start
now func() time.Time
}
func (c *TimedConn) ReadFromUDPAddrPort(b []byte) (int, netip.AddrPort, error) {
n, addr, err := c.UDPConn.ReadFromUDPAddrPort(b)
if err == nil {
c.lastReadUnixNano.Store(c.nowFn().UnixNano())
c.reads.Add(1)
}
return n, addr, err
}
// StaleFor reports a wedge: the socket was being drained and stopped. A socket
// that has never been read is not a wedge - it's a node that has never had
// inbound discv5 traffic (unreachable bootnodes, blocked UDP), which a restart
// cannot fix.
func (c *TimedConn) StaleFor(d time.Duration) bool {
if c.reads.Load() == 0 {
return false
}
return c.nowFn().Sub(c.LastRead()) > d
}The boot node is unaffected (its empty-table rule already covers "never worked"). If you'd rather keep StaleFor purely time-based, expose Reads() and put the guard in DiscV5Service.DiscoveryStale instead. Either way, add a test case: seeded-but-never-read + clock advanced past grace → not stale.
There was a problem hiding this comment.
Done — TimedConn now tracks whether the socket has ever been read (a read flag set on every successful ReadFromUDPAddrPort), and StaleFor returns false until that first read.
A wedge can only arise after draining has started, so this keeps every real wedge in scope while removing the false-positive class: a node that has simply had no inbound UDP yet (unreachable bootnodes / blocked UDP), which a restart can't fix. Kept the guard inside StaleFor via a new ReadStaleness() returning (age, ok) so it stays the single source of truth — the boot node inherits it too, though its populated-table gate already covered the never-worked case.
Added the seeded-but-never-read regression test you suggested, and updated TestDiscV5Service_DiscoveryStale, which had encoded the old assumption.
| } | ||
|
|
||
| dv5PostForkListener, err := listenV5(udpConn, localNode, *dv5PostForkCfg) | ||
| dv5PostForkListener, err := listenV5(socketConn, localNode, *dv5PostForkCfg) |
There was a problem hiding this comment.
Finding 2 · [MINOR] The wiring that makes the whole feature work is untested, and it fails open
Every test in this PR exercises TimedConn, DiscoveryStale and check() against hand-built structs. Nothing asserts the one fact the entire feature rests on: that the conn handed to ListenV5 is the wrapped one.
If a future refactor of initDiscV5Listener passes udpConn instead of socketConn at dv5_service.go:456 — easy to do, since both are in scope and the pre-fork call two lines down deliberately takes a different conn — then dvs.socketConn is still non-nil and still seeded, StaleFor still returns a plausible answer, and every test in this PR still passes. The detector just silently reports "healthy" forever. Same for the boot node at utils/boot_node/node.go:179. This is exactly the fail-open, silent-for-20-days failure mode #2979 is about, reintroduced one level up.
The package already has the machinery to pin this cheaply: listenV5 is an injectable package-level var precisely so tests can wrap it, and TestInitDiscV5Listener_CleansUpOnError (shared_conn_test.go:199) already stubs it and counts calls.
Suggested fix: Add a wiring assertion using the existing stub pattern:
// TestInitDiscV5Listener_WrapsPostForkConn pins the wiring the wedge detector
// depends on: the post-fork listener (the only one that drains the socket) must
// get the TimedConn, and the pre-fork one must get the SharedUDPConn.
func TestInitDiscV5Listener_WrapsPostForkConn(t *testing.T) {
orig := listenV5
t.Cleanup(func() { listenV5 = orig })
var conns []discover.UDPConn
listenV5 = func(conn discover.UDPConn, ln *enode.LocalNode, cfg discover.Config) (Listener, error) {
conns = append(conns, conn)
return orig(conn, ln, cfg)
}
dvs := testingDiscovery(t)
t.Cleanup(func() { require.NoError(t, dvs.Close()) })
require.Len(t, conns, 2)
require.Same(t, dvs.socketConn, conns[0], "post-fork listener must read through TimedConn")
require.IsType(t, &SharedUDPConn{}, conns[1])
}And a one-line addition to the existing TestNewDiscV5Service: assert.NotNil(t, dvs.socketConn). The boot node's createListener can be covered the same way by asserting the returned *discovery.TimedConn is non-nil.
There was a problem hiding this comment.
Done — added TestInitDiscV5Listener_WrapsPostForkConn, using the existing injectable listenV5 stub: it asserts the post-fork listener is handed the service's own *TimedConn (socketConn) and the pre-fork listener the *SharedUDPConn, so a refactor that passed the wrong conn now fails the suite instead of silently reporting healthy forever. Also added assert.NotNil(t, dvs.socketConn) to TestNewDiscV5Service.
Left the boot-node createListener path uncovered for now: it Fatals and binds a real socket, so covering it cheaply would need a testability refactor (return errors + inject the listen fn, like listenV5 here). Flagging as a follow-up rather than folding it into this PR.
|
|
||
| // StaleFor reports whether the socket has gone unread for longer than d — the | ||
| // signal that discovery has wedged. | ||
| func (c *TimedConn) StaleFor(d time.Duration) bool { |
There was a problem hiding this comment.
Finding 3 · [MINOR] No metric or log for read staleness — the only evidence of a wedge is the process exiting
The signal is consumed purely as a boolean at the moment of a restart decision. Nothing exports the underlying quantity, so:
- You cannot alert before the kill. A socket that has been unread for 2m50s is indistinguishable from one unread for 2s. There is no way to page on "discovery is degrading" — only to discover after the fact that a node restarted.
- You cannot confirm the fix works. After deploying this, there's no way to answer "how often does read-staleness approach the grace across the fleet?", which is exactly the data needed to tell whether 3 minutes is the right threshold or whether the false-positive class (see the other finding on the operator staleness check) is being hit in production.
- Post-mortem evidence is thin. On the operator, the wedge produces one error string on the way out; the pre-restart state (table size, last-read age,
SharedUDPConn.Dropped()) is lost. Given boot-node: discv5 can wedge silently — no health signal tied to discovery actually working #2979 went undiagnosed for 20 days, the diagnostic trail matters as much as the restart.
The package already has network/discovery/observability.go with registered instruments, and SharedUDPConn already exports Dropped(), so there's an established place for this.
Suggested fix: Export the age as an observable gauge in network/discovery/observability.go and record it wherever the health check reads it:
discoveryReadStalenessGauge = observability.NewMetric(
meter.Float64ObservableGauge(
metricName("discovery.socket.read_staleness"),
metric.WithUnit("s"),
metric.WithDescription("seconds since the discv5 socket was last read"),
),
)Plus a Warn on the operator path when the check trips, carrying the table size alongside the staleness, so the exit reason and the surrounding state land in the same log line.
There was a problem hiding this comment.
Done, scoped to the operator. Added a synchronous Int64Gauge ssv.p2p.discovery.socket.read_staleness (seconds; 0 until the first read), sampled at the operator poll point in DiscoveryStale, plus a Warn in p2pNetwork.Healthy() when the check trips — so the wedge shows up in the logs and isn't only inferable from a restart.
Went with a synchronous gauge rather than an ObservableGauge to match the existing instruments in this package. No boot-node gauge: the boot-node binary wires no OTel exporter, so it would never be scraped — /healthz already surfaces that state.
c9d0cc1 to
b9f0d08
Compare
b9f0d08 to
a16e6da
Compare
| // loads seed nodes from the persistent enode DB straight into the table, so | ||
| // AllNodes() can be >0 before the socket has been read once — and StaleFor stays | ||
| // disarmed until that first read. Those seeds then fail revalidation and age out, | ||
| // the table empties, and the empty-table grace trips instead. A runtime wedge |
There was a problem hiding this comment.
I fear the fallback described here doesn't hold under a dispatch-loop wedge.
Table entries are only deleted in tableRevalidation.handleResponse, and revalidation itself goes through the wedged dispatch goroutine (initCall blocks on t.callCh, which that goroutine drains), so the seeds never age out, AllNodes() stays >0, lastNonEmpty keeps refreshing, and /healthz stays green indefinitely.
The dispatch-wedge case is still caught (a prior read has armed StaleFor), but a stall where the socket never yields a read (kernel/conntrack-level) lands exactly in populated-table + disarmed-socket = healthy forever. Worth correcting the comment at minimum, or considering a self-probe: write a junk packet to the socket's own LocalAddr() and require LastRead() to advance, which covers it deterministically and independently of peers.
| // A wedged discv5 socket leaves discovery silently dead while bootstrap keeps | ||
| // looping; surface it so the hprobe watchdog restarts the node. n.disc is nil | ||
| // in tests and briefly at startup, hence the guard. | ||
| if n.disc != nil && n.disc.DiscoveryStale(discoveryStaleGrace) { |
There was a problem hiding this comment.
Could this lead to a restart loop on a healthy node that just has no inbound UDP?
After a single successful read ever, 3 minutes of inbound silence fails Healthy: e.g. inbound UDP blocked upstream (firewall/NAT/conntrack change) while the node keeps operating fine over its established libp2p TCP peers; the restart drops every peer connection and can't fix the network.
A total blackhole self-limits (the arm resets on restart), but a partial one the odd scan packet or NAT-refreshed reply arriving each boot re-arms every cycle and loops indefinitely.
Maybe stamp writes in TimedConn too and only declare a wedge when reads AND writes are both stale: all v5 sends funnel through the same dispatch goroutine, so a real wedge freezes both, while an inbound blackhole leaves writes flowing (nursery bootnodes are re-pinged every refresh).
| func (h *bootNodeHealth) check() error { | ||
| now := h.now() | ||
| if len(h.lister.AllNodes()) > 0 { | ||
| h.lastNonEmpty.Store(now.UnixNano()) |
There was a problem hiding this comment.
Seems that the empty-table clock is driven by probe arrivals rather than table state: if probing pauses (kubelet restart, or the livenessProbe manifest lands in a later rollout than the image), the first resumed check compares now against a stale observation and can fail with no effective grace.
Restarting a healthy pod. Conversely, a table flapping in and out of empty faster than the probe period resets the clock every time, so the grace never accumulates. Might be worth sampling the table from a small owned ticker and having check() read only the sampled state.
| return false | ||
| } | ||
| age, ok := dvs.socketConn.ReadStaleness() | ||
| recordDiscoveryReadStaleness(dvs.ctx, int64(age.Seconds())) |
There was a problem hiding this comment.
Should we distinguish the never-read state in the gauge?
Right now "read 200ms ago" and "never read since boot" both record 0 and never-read is precisely the state the health check deliberately ignores, so it's the one you'd want visible to a human for alerting.
A sentinel (-1) or a companion read-ever metric would keep it distinguishable. Also, since this only records when Healthy() reaches the staleness check, the series goes quiet whenever isReady/discoveryFailed short-circuit earlier: i.e. exactly when discovery is broken.
| // shows up as a last-read timestamp that stops advancing. StaleFor turns that | ||
| // into a liveness signal, used by both the operator and boot nodes. | ||
| // | ||
| // Only ReadFromUDPAddrPort is overridden; writes, Close and LocalAddr fall |
There was a problem hiding this comment.
Might be worth stating the scope here: only the post-fork listener reads through TimedConn, the pre-fork listener reads SharedUDPConn's buffer, so a pre-fork dispatch stall moves neither health signal.
The known blocking cause is gone post-#2980, but as written the doc reads as if the wedge signal covers discovery generally.
| func (h *bootNodeHealth) handler() http.HandlerFunc { | ||
| return func(w http.ResponseWriter, _ *http.Request) { | ||
| if err := h.check(); err != nil { | ||
| http.Error(w, err.Error(), http.StatusServiceUnavailable) |
There was a problem hiding this comment.
Do we need /healthz unauthenticated on the public ENR-advertised port?
Every request takes the discv5 table mutex via AllNodes() and allocates the full node slice, there's no method filtering, and the error body discloses internal state.
Maybe memoize check() for ~1s, reject non-GET/HEAD with 405, and return a fixed body with the reason logged instead — or serve it on a pod-internal listener.
3 participants
Problem
A discv5 socket can wedge — stay bound but stop being drained — leaving discovery silently dead while the process still looks healthy. #2979 documents a boot node that sat like this for ~20 days undetected. Neither node type surfaces it today: the operator's
p2pNetwork.Healthy()only checks a discovery-bootstrap flag (a runtime wedge never trips it, since the bootstrap loop keeps running and just yields nothing), and the boot node's HTTP handler always returns200.#2980 removes one cause of the wedge on the operator (the blocking
Unhandledsend); this PR adds the missing detection, so a wedge from any cause becomes a self-correcting restart.Approach
One shared, cause-agnostic signal — "how long since the discv5 socket was last read" — with actuation scoped per node type.
TimedConn(network/discovery): wraps the socket and stamps the last successful read;StaleFor(d)is the wedge signal. Both node types wrap the conn they hand toListenV5.DiscoveryStalefeedsp2pNetwork.Healthy(), so the existinghprobewatchdog restarts the node on a wedge. No routing-table check here — a stale-but-populated table would mask it on an operator./healthzreturns non-200 when the routing table has been empty past a cold-start grace, or — while the table is populated — the socket has gone unread (discv5 revalidates its peers, so a populated-but-unread socket is a wedge; an empty/quiet table produces no such traffic and is judged only by the grace). Empty-table is the boot node's definitional health (the 0-vs-72/102 datapoint in boot-node: discv5 can wedge silently — no health signal tied to discovery actually working #2979).Grace values: 3 min read-staleness (both node types); 10 min empty-table cold-start (boot node only).
Notes
fix/discv5-unhandled-wedge), because the operator change wraps the socket ininitDiscV5Listener, which network/discovery: stop undecodable packets from wedging discv5 #2980 rewrites. Retarget tostageonce network/discovery: stop undecodable packets from wedging discv5 #2980 merges./debug/pprof, and a secondsepoliaboot node.livenessProbeon/healthzis added by the downstream PRs below.Tests
TimedConn: seeded-not-stale, stale boundary (injected clock, no real sleeps), read-stamps, errored-read-doesn't-stamp.DiscoveryStale;TestP2PNetwork_Healthygainsdiscovery wedged/ready with live discoverycases — the existing nil-disccases stay green, proving the guard./healthzacross populated+fresh, populated+wedged, empty-within-grace (incl. the quiet-socket regression), and empty-past-grace.Closes #2979. Full discovery + p2p + boot_node suites pass, discovery also under
-race.Downstream infra PRs for the boot-node side of this (what makes the boot node self-healing)
livenessProbeto theboot-nodechart (inert by default)./healthz).Merge order: #2980 → this → build image → ssvlabs/charts#183 → ssvlabs/gitops-production#881.