Stop bufferbloat readings being fabricated by dropped handshakes

Bufferbloat is measured by timing TCP handshakes to a fixed target, once
while the link is quiet and again while a speedtest saturates it; the
difference is the queueing delay. When a handshake's first packet is lost,
the system waits about a second and tries again, so the sample comes back
reading roughly 1000ms whatever the link is really doing.

That was poisoning the baseline. A quiet-link burst that collected enough
of those retransmits produced a baseline near a second, and because the
reported figure is the loaded time minus the baseline, floored at zero,
every honest loaded reading then came out as exactly zero bufferbloat -
not missing, but confidently wrong. Retransmit-shaped samples are now
discarded before the baseline is taken, and a burst with no honest sample
left in it reports nothing at all rather than a fabricated number. The
test cares about slow links specifically: a satellite connection whose
real samples sit near 600ms keeps every one of them, because a sample is
only judged against the fastest one in its own burst.

The reason so many handshakes were being lost is that the target is
reached over whichever address family answers first, and on a connection
where one family is losing packets that family still wins the race about
half the time - then stays cached for as long as the daemon runs. It is
now measured with a short burst before it is trusted, and if it looks
lossy the other family is measured too and the cleaner one is used. On the
connection this was found on, the IPv6 path was dropping about 44% of
handshakes while IPv4 was clean.

The measuring cannot be cancelled once started, since a half-finished
burst would judge a link on evidence its own caller destroyed - so
stopping a speedtest now abandons it instead of waiting for it, and
nothing is remembered from an abandoned one.

Co-Authored-By: Claude <noreply@anthropic.com>

Author: cansofgrease

README.md

@@ -640,8 +640,12 @@ or upload-only run has no figures for the direction it skipped, packet loss is
 optional and not always measurable, family and probe direction are recorded
 only when the run really established them (the engine notes below say when
 that is), and bufferbloat is absent when a transfer
-phase was too short to sample, returned too few samples, or the latency target
-was unreachable. Missing is stored as missing rather than as a zero, so charts
+phase was too short to sample, returned too few samples, the latency target
+was unreachable, or too few idle probes survived the retransmit filter to leave a
+baseline. That last case drops the figure on purpose: a single retransmit in the
+idle number is worth about a second, enough on its own to cancel real bloat down
+to zero, so a polluted baseline would report a clean link instead of an
+unmeasurable one. Missing is stored as missing rather than as a zero, so charts
 and thresholds can tell "not measured" from "measured, and it was bad". A run
 that failed outright isn't a measurement at all: it is kept only as a flagged
 data-usage row, which every measurement view filters out (see the data-usage
@@ -678,14 +682,26 @@ flowchart LR
   queued --> bloat["bufferbloat = 190 - 24<br/>= +166 ms under load"]
 ```
 
-Both figures are **medians** of their probes, and the headline bufferbloat number
-- the one the tiles show and the one your **max bufferbloat** threshold is
+Both figures are **medians** of their probes - the idle one over only the probes
+that survive a retransmit filter, since on an unloaded link a sample more than
+500 ms above that burst's own minimum is an OS retry rather than latency. A burst
+whose own fastest probe is already at or above a second holds no honest sample to
+measure the rest against, and yields no baseline at all. The loaded phases keep
+theirs, where a near-second sample is the bloat itself. The headline bufferbloat
+number - the one the tiles show and the one your **max bufferbloat** threshold is
 compared against - is `median(loaded) - median(idle)`. The chart also plots a
 **p95** per direction, the sustained bad end of the distribution. p95 is
 deliberately not the maximum: these are TCP-connect probes, and a single worst
 sample on one is usually a SYN retransmission (a fixed ~1000 ms OS retry, and
 ~2000 ms for a second one) rather than queue delay, so a max-based number
-reports round figures that say more about packet loss than about buffering.
+reports round figures that say more about packet loss than about buffering. The
+probes go to a fixed dual-stack name, and the address family that wins their
+connection race is not taken on trust - a path that drops half its handshakes
+still wins races constantly, and its retries would land in the baseline. So the
+winner is graded with a short burst first, and only if that burst comes back
+lossy is the other family resolved and measured, taking the job only if it grades
+cleaner. A host reachable in just one family keeps it however lossy: lossy data
+beats none.
 
 There are two engines, picked in the settings drawer:
 

internal/speedtest/iperf.go

@@ -584,7 +584,7 @@ func (i *Iperf) Run(ctx context.Context) (Result, error) {
 	// reject) - version caveats are surfaced to the UI via IperfVersion, not gated
 	// here (see iperfArgs).
 	res := Result{Engine: "iperf3", Server: name}
-	probeAddr := lulRunEndpoint()
+	probeAddr := lulRunEndpoint(ctx)
 	res.IdleMS = measureIdleLatency(ctx, probeAddr)
 	// Unloaded ping to the server, taken now while the link is idle (see measureServerRTT
 	// for why iperf3's own min_rtt isn't trusted here).

internal/speedtest/lul.go

@@ -29,8 +29,11 @@ func TestMedian(t *testing.T) {
 // The loaded sampler must return nil when the phase is too short or yields too
 // few samples - a number from an unsaturated instant would be misleading.
 func TestLoadSamplerGating(t *testing.T) {
+	origFails := lulFails
+	t.Cleanup(func() { lulFails = origFails })
 	// Real network unavailable in tests isn't assumed; we only exercise the
-	// gating path: stop immediately, far under both thresholds.
+	// gating path: stop immediately, far under both thresholds. The empty address
+	// is the offline part - the dial fails without touching a resolver.
 	stop := startLoadSampler(context.Background(), "")
 	time.Sleep(50 * time.Millisecond)
 	if got := stop(); got != nil {
@@ -43,11 +46,23 @@ func TestLoadSamplerGating(t *testing.T) {
 // (The old sync.Once cached the miss and then dialed the hostname - paying DNS in
 // every timed handshake - for the whole process.)
 func TestLulDialAddrRetriesAfterResolveFailure(t *testing.T) {
-	origTarget, origResolved, origDial := lulTarget, lulResolved, lulResolveDial
-	t.Cleanup(func() { lulTarget, lulResolved, lulResolveDial = origTarget, origResolved, origDial })
+	origTarget, origResolved, origFails, origDial := lulTarget, lulResolved, lulFails, lulResolveDial
+	origBurst, origFam := lulProbeBurst, lulFamilyDial
+	t.Cleanup(func() {
+		lulTarget, lulResolved, lulFails, lulResolveDial = origTarget, origResolved, origFails, origDial
+		lulProbeBurst, lulFamilyDial = origBurst, origFam
+	})
 
 	lulTarget = "lul.invalid:443" // a hostname, so lulDialAddr takes the resolve path
-	lulResolved = ""
+	lulResolved, lulFails = "", 0
+	// Selection is not under test here: report the candidate clean so the
+	// caching semantics stay the subject, and keep both seams off the network.
+	lulProbeBurst = func(context.Context, string, int, time.Duration) ([]float64, int) {
+		return []float64{1, 1, 1, 1, 1}, 0
+	}
+	lulFamilyDial = func(context.Context, string, string) (net.Conn, error) {
+		return nil, errors.New("not under test")
+	}
 	calls := 0
 	lulResolveDial = func(context.Context, string) (net.Conn, error) {
 		calls++
@@ -58,47 +73,111 @@ func TestLulDialAddrRetriesAfterResolveFailure(t *testing.T) {
 	}
 
 	// 1st: resolve fails -> hostname fallback, nothing cached.
-	if got := lulDialAddr(); got != lulTarget {
+	if got := lulDialAddr(context.Background()); got != lulTarget {
 		t.Fatalf("after failed resolve = %q, want hostname fallback %q", got, lulTarget)
 	}
 	// 2nd: the miss was NOT cached, so the resolve retries and now succeeds.
-	if got, want := lulDialAddr(), "203.0.113.7:443"; got != want {
+	if got, want := lulDialAddr(context.Background()), "203.0.113.7:443"; got != want {
 		t.Fatalf("after recovered resolve = %q, want resolved literal %q", got, want)
 	}
 	// 3rd: served from cache, no further dial.
-	if got, want := lulDialAddr(), "203.0.113.7:443"; got != want {
+	if got, want := lulDialAddr(context.Background()), "203.0.113.7:443"; got != want {
 		t.Fatalf("cached resolve = %q, want %q", got, want)
 	}
 	if calls != 2 {
 		t.Fatalf("resolve dialed %d times, want 2 (one failed retry, then one cached success)", calls)
 	}
 }
 
-// A cached resolve must not outlive the stack it belongs to: after
-// lulFailInvalidate consecutive failed connects the literal is dropped so the
-// next sample re-resolves (the first resolve picked IPv6, IPv6 later died while
-// IPv4 still works). A successful connect resets the streak.
-func TestLulNoteConnectDropsDeadCache(t *testing.T) {
-	origResolved, origFails := lulResolved, lulFails
-	t.Cleanup(func() { lulResolved, lulFails = origResolved, origFails })
+// A RUN PINS ONE ENDPOINT EVEN WHEN THE RESOLVE FAILS. Handing back "" would
+// leave every sample to resolve for itself: on an idle link that is a whole
+// family selection per probe, so the idle burst spends its budget on failed
+// selections instead of probes and reports no baseline at all; inside a load
+// phase it is a DNS lookup in every timed handshake, inflating the bufferbloat
+// number being measured. The hostname is the honest fallback - worse than a
+// literal, but one name for every sample of the run.
+func TestLulRunEndpointPinsHostnameWhenResolveFails(t *testing.T) {
+	lulSelectSeams(t)
+	resolves := 0
+	lulResolveDial = func(context.Context, string) (net.Conn, error) {
+		resolves++
+		return nil, errors.New("simulated resolve failure")
+	}
+	lulProbeBurst = func(context.Context, string, int, time.Duration) ([]float64, int) {
+		t.Error("validated a candidate although the resolve failed")
+		return nil, lulSelectProbes
+	}
+	lulFamilyDial = func(context.Context, string, string) (net.Conn, error) {
+		return nil, errors.New("no candidate: must not be reached")
+	}
+
+	if got := lulRunEndpoint(context.Background()); got != lulTarget {
+		t.Fatalf("lulRunEndpoint = %q with the resolve failing, want the hostname %q: \"\" "+
+			"leaves every sample to resolve for itself", got, lulTarget)
+	}
+	// And a sample dials what it was handed, whatever the cache holds: no
+	// selection, no DNS, nothing but the timed handshake.
+	before := resolves
+	if _, ok := connectRTT(context.Background(), ""); ok {
+		t.Fatal("connectRTT reported a successful sample without an address to dial")
+	}
+	if resolves != before {
+		t.Fatalf("one sample ran %d extra resolve dials, want 0: a sample must dial the "+
+			"endpoint the run pinned, never re-enter the selection", resolves-before)
+	}
+}
+
+// A CACHED LITERAL IS DROPPED AT A RUN BOUNDARY, NOT MID-RUN. The cache must not
+// outlive the stack it belongs to: after lulFailInvalidate consecutive failed
+// connects the literal is dead and the next run must re-resolve (the first
+// resolve picked IPv6, IPv6 later died while IPv4 still works). But dropping it
+// the moment the streak lands drops it INSIDE a saturated phase - congestion
+// produces those failures too - and the phases after it are then left dialing a
+// hostname, paying DNS inside the very handshakes being timed. So lulNoteConnect
+// only counts and lulDialAddr, reached once per run on an idle link, does the
+// dropping. A successful connect clears the streak: a path that recovered is not
+// dead.
+func TestLulDeadLiteralDroppedAtRunBoundary(t *testing.T) {
+	lulSelectSeams(t)
+	lulResolved = "203.0.113.7:443"
+	resolves := 0
+	lulResolveDial = func(context.Context, string) (net.Conn, error) {
+		resolves++
+		return stubConn{remote: &net.TCPAddr{IP: net.ParseIP("2001:db8::7"), Port: 443}}, nil
+	}
+	lulProbeBurst = func(context.Context, string, int, time.Duration) ([]float64, int) {
+		return []float64{7.0, 7.2, 6.9, 7.1, 7.0}, 0
+	}
+	lulFamilyDial = func(context.Context, string, string) (net.Conn, error) {
+		return nil, errors.New("clean winner: must not consult the other family")
+	}
 
-	lulResolved, lulFails = "203.0.113.7:443", 0
 	for i := 0; i < lulFailInvalidate-1; i++ {
 		lulNoteConnect(false)
 	}
-	if lulResolved == "" {
-		t.Fatalf("cache dropped after %d failures, want it kept below the threshold", lulFailInvalidate-1)
-	}
-	lulNoteConnect(true) // success resets the streak
+	lulNoteConnect(true) // a success clears the streak
 	for i := 0; i < lulFailInvalidate-1; i++ {
 		lulNoteConnect(false)
 	}
+	if got := lulRunEndpoint(context.Background()); got != "203.0.113.7:443" {
+		t.Fatalf("lulRunEndpoint = %q below the failure threshold, want the cached literal: "+
+			"only a streak that reaches lulFailInvalidate=%d means dead", got, lulFailInvalidate)
+	}
+	if resolves != 0 {
+		t.Fatalf("re-resolved %d times below the failure threshold, want 0", resolves)
+	}
+
+	lulNoteConnect(false) // the streak lands, mid-run
 	if lulResolved == "" {
-		t.Fatal("a successful connect must reset the failure streak")
+		t.Fatal("the cached literal was dropped the moment the streak landed; want it held to " +
+			"the run boundary, so no later phase is left dialing a hostname mid-run")
+	}
+	if got := lulRunEndpoint(context.Background()); got != v6Winner {
+		t.Fatalf("lulRunEndpoint = %q on the next run, want the re-selected literal %q: a dead "+
+			"literal must not survive a run boundary either", got, v6Winner)
 	}
-	lulNoteConnect(false) // streak reaches the threshold
-	if lulResolved != "" {
-		t.Fatalf("cache = %q after %d consecutive failures, want dropped for re-resolve", lulResolved, lulFailInvalidate)
+	if resolves != 1 {
+		t.Fatalf("re-resolved %d times at the run boundary, want exactly 1", resolves)
 	}
 }