GO-7467 Rework quic degradation fallback - #775
Merged
Merged
Conversation
Follow-up to #774, which shipped the feature with a detection signal that could not fire and a policy that could not be undone. Reviewed along six lenses; every fix below has a test that fails without it. Structure. The demotion state moves out of peerservice into its own net/quicdemotion component, wired like peerobserver: registering it is the opt-in, so a server node leaves it out and dialing behaves exactly as before. That removes the five methods bolted onto the public PeerService interface and the EnableQuicDemotion call, which had to happen at a precise point in the component lifecycle - and did not, since clients enable it from their config component, whose Init runs before peerservice's. It dereferenced a nil penalty state and panicked on every account start. peerservice now only reports what a dial did and asks whether to reorder. Signal. A blackholed UDP path gives up with an idle timeout, never a handshake timeout: with nothing received, quic-go measures the idle deadline from connection start and it fires at half the handshake deadline. The old check therefore never matched the case it was written for, so a fully blocked UDP path scored nothing. Connection lifetime moves to the wall clock, because the monotonic clock pauses while the machine sleeps and made a slept-through connection look seconds old - the exact false positive the age threshold exists to prevent. A connection that carried bytes now counts as healthy: the pool closes idle connections after about a minute, so short-RPC peers never reached the age threshold and their strikes could only ever climb. A stateless reset is evidence the path works, not against it. Policy. Strikes decay after an hour and peers that can no longer affect a decision are pruned, so the state tracks recent trouble instead of growing for the life of the install. A demoted peer is dialed quic-first once every ten minutes: dialing stops at the first working scheme, so without a probe a demotion suppressed the only evidence that could lift it. Demotion is suspended while yamux itself is failing, and a dial-time strike requires that another scheme carried the same dial - otherwise an ordinary outage would demote every peer, and on a censored network the adversary would get to choose our transport. TTLs are jittered, since a deterministic ladder leaks how many episodes an install has seen and lines devices up. Also: the network-wide verdict is cached instead of rescanned under the lock on every dial, the conn observer is stored atomically and watch is a no-op without one (a server node has none, and a nil call in a detached goroutine would take the process down), and snapshots carry a schema version so a shape change here cannot be misread by a client that persisted an older one.
scheme("") returns yamux, so deriving the winning scheme from the connected
address - an obvious-looking tidy-up of the dial loop - silently reports a
yamux success for a dial where nothing connected. That would clear the
fallback-failing signal and let an ordinary outage strike every peer, which
is the false positive the working-fallback rule exists to prevent. Verified
the test fails against that refactor.
The suspend-while-yamux-fails rule was a global one-way latch. Any failed yamux dial set it - including the sleeping LAN peers heart re-dials every twenty seconds - and only a dial that finished on yamux could clear it. In the case this feature exists for the quic dial keeps succeeding, so yamux is never dialed again and nothing ever clears it: one offline blip early in a session silently disabled demotion for the rest of the process while strikes piled up unused. reset() did not clear it either, so a network change did not recover it. Date the verdict instead of latching it, expire it after five minutes since it is evidence about the network we are on right now, count only network nodes for the same reason they alone drive the network-wide demotion, and clear it - with the global probe clock - on reset.
…port Two ways a dial could teach the wrong lesson. A connection that is opened and then rejected - a stale address that now points at a different peer - was reported before the peer id was checked, so it counted as the fallback working and, if a quic address had timed out earlier in the same loop, as a strike against a peer we demonstrably never reached. Report once the dial is fully resolved instead. And any non-quic scheme counted as proof that the path works without udp, but webtransport and iroh are quic-based themselves: succeeding on one of them means udp works, the opposite of what a strike records. Only yamux qualifies.
The lifetime shortcut ran before the cause check, so a connection that lived past five minutes and then died of an idle timeout was scored healthy - which deletes the peer's entry outright, backoff included. An idle timeout always means the path went black, since keepalives are on and a working path never reaches one; when it happens late it is usually sleep or a network change, which is no evidence either way and must not wipe a demotion. Also covers three behaviours mutation testing found undefended: seed must restore the network-wide verdict (the reason persistence exists at all), reset must clear it, and a failing quic dial must not be read as the yamux fallback failing. Each new test was checked against its mutant.
…cuments globalDemotionMinPeers appeared only in comments - the rule was hard-coded as a first/second-latest scan, so changing the constant changed nothing. Take the Nth-latest deadline instead, which is both what the constant says and what the rule means: the moment fewer than N nodes are still demoted. Arming the global probe now also requires a demotion that is actually in effect; an expired entry that prune deliberately keeps could previously push the deadline forward and arm the clock with nothing demoted. Drops the enabled flag left over from the pre-component design, an IsDialDegraded re-export with no callers anywhere, and an orphaned doc comment from the package move; and says plainly that quicDemoted is the state query while dialing goes through demoteDial.
cheggaaa
approved these changes
Sep 1, 2026
Coverage provided by https://github.com/seriousben/go-patch-cover-action |
penalty.go moved to net/quicdemotion but penalty_test.go stayed behind, so its name pointed at a file no longer in the package. What it actually covers is the seam between dialing and the demotion component.
The peerservice-side tests were reaching through Dial into the demotion component - seeding a snapshot, then asserting on Snapshot() - so they restated policy that net/quicdemotion already covers and made the wrong package responsible for it. Assert what this layer actually decides instead: the DialOutcome it reports and whether it reorders the schemes, against a stub component. Policy stays where it is implemented. The file is named for the peerservice concept it covers, since penalty.go and quicdemotion.go both live elsewhere.
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
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.
Follow-up to #774, which shipped the feature with a detection signal that could not fire and a policy that could not be undone. Reviewed along six lenses, then three more after the rework; every fix here has a test that fails without it.
Structure
The demotion state moves out of
peerserviceinto its ownnet/quicdemotioncomponent, wired likepeerobserver: registering it is the opt-in, so a server node leaves it out and dialing behaves exactly as before. That removes the five methods bolted onto the publicPeerServiceinterface and theEnableQuicDemotioncall, which had to happen at a precise point in the component lifecycle — and did not, since clients enable it from their config component, whoseInitruns before peerservice's. It dereferenced a nil penalty state and panicked on every account start.peerservicenow only reports what a dial did and asks whether to reorder.Signal
Policy
Also
The network-wide verdict is cached instead of rescanned under the lock on every dial, and
globalDemotionMinPeersnow governs the rule it documents rather than being hard-coded. The conn observer is stored atomically andwatchis a no-op without one — a server node has none, and a nil call in a detached goroutine would take the process down. Snapshots carry a schema version so a shape change here cannot be misread by a client that persisted an older one.Testing
go build ./...,go test ./...andgo test -race ./net/...all pass. Mutation testing over the new code killed 19 of 22 injected mutants in the previous round; the survivors are covered here and each new test was verified against its mutant.Notes for the reviewer
PenaltySnapshot/PeerPenalty, orIsHandshakeTimeout, and nothing mocksPeerService.quicdemotion.CName.quic.Config.Versionschange that production nodes already accept) looks like the better counter for that case and is worth measuring separately.