Skip to content

Commit d13dff2

Browse files
cansofgreaseclaude
andcommitted
Fix upload data-usage counting, Quick Setup retries, and large downloads
- A speed test whose upload fails now records the data it actually used again (a library update had quietly stopped counting it). - Retrying the first-run Quick Setup after it turns on a login now succeeds instead of showing an error. - Downloading oversized logs or CSV exports shows a clear "too large" message (and points to the database file) instead of failing silently. - The log-download link always goes through the app's own login, never the browser's basic-auth popup. - Accessibility touch-ups on the first-run setup dialog. - Internal test and CI hardening; no other change in behavior. Co-Authored-By: Claude <noreply@anthropic.com>
1 parent 53540a6 commit d13dff2

14 files changed

Lines changed: 489 additions & 29 deletions

.github/workflows/deep-test.yml

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -245,7 +245,7 @@ jobs:
245245
# No host networking here: under QEMU we only care that the arm64
246246
# binary starts, opens its port and serves - the measurements it takes
247247
# inside an emulated container are meaningless either way.
248-
docker run -d --name ping-a64 -p 9111:9000 ping-arm64
248+
docker run -d --name ping-a64 -e PINGULARITY_ACCESS=network -p 9111:9000 ping-arm64
249249
ok=""
250250
for i in $(seq 1 30); do
251251
curl -fsS --max-time 3 http://127.0.0.1:9111/metrics >/dev/null 2>&1 && { ok=1; break; }
@@ -308,6 +308,8 @@ jobs:
308308
image: ghcr.io/pingular/pingularity:0.7.0
309309
container_name: pingularity
310310
network_mode: host
311+
environment:
312+
- PINGULARITY_ACCESS=network
311313
cap_add:
312314
- NET_RAW
313315
volumes:
@@ -357,7 +359,8 @@ jobs:
357359
# (-metrics-token) and must never surface in any log; the allow-host
358360
# part is what the inertness 403 is asserted against. Do NOT sentinel
359361
# on the domain - the Host guard legitimately logs rejected Hosts.
360-
sed -i 's/^ command: \["-allow-host=ping.example.com"\]$/ environment:\n - PINGULARITY_OPTS=-allow-host=ping.example.com -metrics-token=DEEP_SECRET_SENTINEL/' compose.yaml
362+
sed -i '/^ command: \["-allow-host=ping.example.com"\]$/d' compose.yaml
363+
sed -i 's/^ - PINGULARITY_ACCESS=network$/ - PINGULARITY_ACCESS=network\n - PINGULARITY_OPTS=-allow-host=ping.example.com -metrics-token=DEEP_SECRET_SENTINEL/' compose.yaml
361364
docker compose up -d
362365
for i in $(seq 1 15); do curl -fsS --max-time 3 http://127.0.0.1:9000/metrics >/dev/null 2>&1 && break; sleep 2; done
363366
eh=$(curl -s -o /dev/null -w '%{http_code}' --max-time 5 -H 'Host: ping.example.com' http://127.0.0.1:9000/ || echo FAIL)
@@ -408,7 +411,7 @@ jobs:
408411
[ "$perm" = "700" ] || note "iperf image data dir is $perm, want 700"
409412
docker run --rm --entrypoint iperf3 ping-iperf --version \
410413
|| note "iperf3 does not run in the image that exists to carry it"
411-
docker run -d --name ping-iperf -p 9112:9000 ping-iperf
414+
docker run -d --name ping-iperf -e PINGULARITY_ACCESS=network -p 9112:9000 ping-iperf
412415
for i in $(seq 1 15); do curl -fsS --max-time 3 http://127.0.0.1:9112/metrics >/dev/null 2>&1 && break; sleep 2; done
413416
im=$(curl -s -o /dev/null -w '%{http_code}' --max-time 5 http://127.0.0.1:9112/metrics || echo FAIL)
414417
echo "iperf image metrics -> $im"
@@ -419,7 +422,7 @@ jobs:
419422
echo "--- iperf container logs ---"; tail -15 <<<"$ilogs"
420423
docker rm -fv ping-iperf
421424
echo "--- issue #16 parity: same Host-guard pair on the variant the report used ---"
422-
docker run -d --name ping-iperf-cmd -p 9113:9000 ping-iperf -allow-host=ping.example.com
425+
docker run -d --name ping-iperf-cmd -e PINGULARITY_ACCESS=network -p 9113:9000 ping-iperf -allow-host=ping.example.com
423426
for i in $(seq 1 15); do curl -fsS --max-time 3 http://127.0.0.1:9113/metrics >/dev/null 2>&1 && break; sleep 2; done
424427
iah=$(curl -s -o /dev/null -w '%{http_code}' --max-time 5 -H 'Host: ping.example.com' http://127.0.0.1:9113/ || echo FAIL)
425428
ioh=$(curl -s -o /dev/null -w '%{http_code}' --max-time 5 -H 'Host: other.example.com' http://127.0.0.1:9113/ || echo FAIL)
@@ -429,7 +432,7 @@ jobs:
429432
[ "$ioh" = "403" ] || note "iperf image: unlisted public Host not rejected (got $ioh, want 403)"
430433
case "$iargs" in *"-allow-host=ping.example.com"*) ;; *) note "iperf image: run-arg flag missing from container argv";; esac
431434
docker rm -fv ping-iperf-cmd
432-
docker run -d --name ping-iperf-env -p 9113:9000 -e "PINGULARITY_OPTS=-allow-host=ping.example.com -metrics-token=DEEP_SECRET_SENTINEL" ping-iperf
435+
docker run -d --name ping-iperf-env -p 9113:9000 -e PINGULARITY_ACCESS=network -e "PINGULARITY_OPTS=-allow-host=ping.example.com -metrics-token=DEEP_SECRET_SENTINEL" ping-iperf
433436
for i in $(seq 1 15); do curl -fsS --max-time 3 http://127.0.0.1:9113/metrics >/dev/null 2>&1 && break; sleep 2; done
434437
ieh=$(curl -s -o /dev/null -w '%{http_code}' --max-time 5 -H 'Host: ping.example.com' http://127.0.0.1:9113/ || echo FAIL)
435438
echo "iperf env-var: Host -> $ieh (must stay 403)"

firstrun_wiring_test.go

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,8 @@ import (
44
"context"
55
"io"
66
"log/slog"
7+
"os"
8+
"strings"
79
"testing"
810
"time"
911

@@ -110,3 +112,60 @@ func TestFirstRunReadyFnComposition(t *testing.T) {
110112
t.Errorf("iperf3 engine readiness = %v, want %v (IperfAvailable on this host)", got, want)
111113
}
112114
}
115+
116+
// The pending->PUBLISHED half of the readiness truth table. TestFirstRunReadyFnComposition
117+
// only pins the pending (false) side, so a mutation that leaves readiness
118+
// permanently false AFTER netinfo publishes slips past it. This drives a real
119+
// publish (even a failed lookup stamps UpdatedAt) and asserts readiness flips.
120+
func TestFirstRunReadyFnReleasesAfterPublish(t *testing.T) {
121+
ctx, cancel := context.WithCancel(context.Background())
122+
defer cancel()
123+
st, err := store.Open(":memory:")
124+
if err != nil {
125+
t.Fatalf("open store: %v", err)
126+
}
127+
defer st.Close()
128+
set, err := settings.New(ctx, st, settings.Values{Monitoring: true, NetinfoEnabled: true})
129+
if err != nil {
130+
t.Fatalf("settings: %v", err)
131+
}
132+
ni := netinfo.NewManager(slog.New(slog.NewTextHandler(io.Discard, nil)))
133+
ni.EnabledFn = func() bool { return set.Monitoring() && set.NetinfoEnabled() }
134+
ready := newFirstRunReadyFn(set, ni)
135+
136+
if ready() {
137+
t.Fatal("must NOT be ready before netinfo publishes (UpdatedAt still 0)")
138+
}
139+
loopCtx, loopCancel := context.WithCancel(ctx)
140+
done := make(chan struct{})
141+
defer func() { loopCancel(); <-done }()
142+
go func() { defer close(done); ni.Loop(loopCtx, time.Hour) }()
143+
144+
deadline := time.Now().Add(15 * time.Second)
145+
for !ready() {
146+
if time.Now().After(deadline) {
147+
t.Fatalf("readiness never released after netinfo published (UpdatedAt=%d)", ni.Get().UpdatedAt)
148+
}
149+
time.Sleep(20 * time.Millisecond)
150+
}
151+
}
152+
153+
// The wiring lives in main.go's run() assembly, which isn't unit-testable, and
154+
// the tests above re-create it locally - so deleting it from main.go leaves them
155+
// green (the exact gap that shipped). This source-presence guard fails if the
156+
// production wiring is removed or renamed. It is deliberately literal.
157+
func TestMainWiresFirstRunAndOptsHooks(t *testing.T) {
158+
src, err := os.ReadFile("main.go")
159+
if err != nil {
160+
t.Fatal(err)
161+
}
162+
for _, want := range []string{
163+
"ni.WakeFn = set.Changed", // #3: first speedtest must not race an empty netinfo
164+
"sched.ReadyFn = newFirstRunReadyFn(set, ni)", // #3: selection readiness predicate
165+
"replayIgnoredOpts(", // #7: ignored-PINGULARITY_OPTS warning is replayed at boot
166+
} {
167+
if !strings.Contains(string(src), want) {
168+
t.Errorf("main.go no longer wires %q - package tests won't catch it; the behavior is silently dead", want)
169+
}
170+
}
171+
}

internal/netinfo/looptimer_test.go

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,59 @@
1+
package netinfo
2+
3+
import (
4+
"context"
5+
"io"
6+
"log/slog"
7+
"testing"
8+
"time"
9+
)
10+
11+
// Loop sizes its backstop timer from the staleness cap, and the cap is recomputed
12+
// AFTER the refresh (netinfo.go:435-443): a boot/refresh can flip the snapshot
13+
// healthy->error, and the error path wants the fast errRetryStale retry, not the
14+
// full maxStale the pre-refresh healthy read implied. This drives that exact flip
15+
// with no network - the boot snapshot is empty (Error==""), and the first refresh
16+
// fails every lookup deterministically ("ip lookup failed"), which CarriedIdentity
17+
// short-circuits before any traceroute. The armed wait must be the short
18+
// errRetryStale-based cap, not maxStale. Deleting the post-refresh recompute makes
19+
// wait ~maxStale (this test fails).
20+
func TestLoopArmsFastRetryWhenRefreshFlipsToError(t *testing.T) {
21+
oldV4, oldV6, oldRE, oldAfter := ipv4Client, ipv6Client, resolverEgress, afterFn
22+
defer func() { ipv4Client, ipv6Client, resolverEgress, afterFn = oldV4, oldV6, oldRE, oldAfter }()
23+
ipv4Client = canned(500, "")
24+
ipv6Client = canned(500, "")
25+
resolverEgress = func(context.Context) string { return "" } // no DNS egress -> no real lookups
26+
27+
waits := make(chan time.Duration, 4)
28+
block := make(chan time.Time) // never fires: Loop parks in the select until ctx cancel
29+
afterFn = func(d time.Duration) <-chan time.Time { waits <- d; return block }
30+
31+
m := NewManager(slog.New(slog.NewTextHandler(io.Discard, nil)))
32+
m.http = canned(500, "") // cfColo / geo answer nothing
33+
34+
const maxStale = time.Hour // >> errRetryStale (5m), so the two caps are unmistakable
35+
ctx, cancel := context.WithCancel(context.Background())
36+
done := make(chan struct{})
37+
go func() { defer close(done); m.Loop(ctx, maxStale) }()
38+
39+
var wait time.Duration
40+
select {
41+
case wait = <-waits:
42+
case <-time.After(3 * time.Second):
43+
cancel()
44+
<-done
45+
t.Fatal("Loop never armed its backstop timer")
46+
}
47+
cancel()
48+
<-done
49+
50+
// The refresh flipped the snapshot to an error; the timer must be armed off
51+
// that post-refresh error state (errRetryStale ~5m), not the pre-refresh
52+
// healthy maxStale (1h).
53+
if got := m.Get().Error; got == "" {
54+
t.Fatalf("boot refresh did not flip the snapshot to an error; got Error=%q", got)
55+
}
56+
if wait > 10*time.Minute {
57+
t.Fatalf("post-error retry armed at %v; want the short errRetryStale-based cap (~%v), not the pre-refresh maxStale (%v) - the post-refresh cap recompute (netinfo.go:440-443) is gone", wait, errRetryStale, maxStale)
58+
}
59+
}

internal/netinfo/netinfo.go

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -360,6 +360,11 @@ func (m *Manager) RefreshNow(ctx context.Context) Info {
360360
// shrink it.
361361
var refreshRetryDelay = 2 * time.Second
362362

363+
// afterFn is Loop's backstop sleep timer, injectable so a test can read the wait
364+
// Loop computes from the POST-refresh staleness cap without sleeping it out.
365+
// Production is the real timer.
366+
var afterFn = func(d time.Duration) <-chan time.Time { return time.After(d) }
367+
363368
// enabled reports whether connection-info lookups may run. Nil means yes, so a
364369
// Manager built without the hook (tests, callers that always want it) behaves
365370
// as it always did.
@@ -463,7 +468,7 @@ func (m *Manager) Loop(ctx context.Context, maxStale time.Duration) {
463468
// refresh NOW.
464469
case <-m.nudge:
465470
// A broadcast-less enable edge (see Nudge) - same re-evaluation.
466-
case <-time.After(wait):
471+
case <-afterFn(wait):
467472
}
468473
}
469474
}

internal/speedtest/iperf.go

Lines changed: 11 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -130,14 +130,17 @@ func congestionForOS(requested, goos string) (effective string, dropped bool) {
130130
return requested, false
131131
}
132132

133-
func AvailableCongestionControl() []string {
134-
// Only Linux iperf3 has the -C socket option; on macOS and Windows passing
135-
// it aborts the whole run at startup ("an option you are trying to set is
136-
// not implemented yet"). Offering a curated list there advertised choices
137-
// that BREAK every test, so offer nothing and let the field mean "system
138-
// default". (setCongestionSkipped mirrors this at run time for a value that
139-
// arrived via an imported backup from a Linux box.)
140-
switch runtime.GOOS {
133+
func AvailableCongestionControl() []string { return availableCongestionControlFor(runtime.GOOS) }
134+
135+
// availableCongestionControlFor is AvailableCongestionControl with the OS as a
136+
// parameter, so the FreeBSD sysctl-reading + table-parsing branch is testable on
137+
// ANY host (through the injectable ccSysctl seam) rather than only when
138+
// GOOS==freebsd. Only Linux iperf3 has the -C socket option; on macOS and Windows
139+
// passing it aborts the whole run at startup, so offer nothing there and let the
140+
// field mean "system default". (setCongestionSkipped mirrors this at run time for
141+
// a value that arrived via an imported backup from a Linux box.)
142+
func availableCongestionControlFor(goos string) []string {
143+
switch goos {
141144
case "linux":
142145
b, err := os.ReadFile("/proc/sys/net/ipv4/tcp_allowed_congestion_control")
143146
if err != nil {

internal/speedtest/iperf_test.go

Lines changed: 37 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -831,10 +831,8 @@ func TestParseFreeBSDCC(t *testing.T) {
831831
}
832832
}
833833

834-
// TestAvailableCongestionControlFreeBSDInjected drives the FreeBSD branch's
835-
// sysctl reader through the injectable ccSysctl seam so the table-parsing path
836-
// is covered on any host (the switch only reaches it when GOOS==freebsd, so we
837-
// test the reader+parser composition directly here).
834+
// Parser-only: pins that table noise never leaks (see TestAvailableCongestionControlFreeBSD
835+
// for the reader+branch composition through the ccSysctl seam).
838836
func TestParseFreeBSDCC_DropsAllTableNoise(t *testing.T) {
839837
got := parseFreeBSDCC("CCmod D PCB\nnewreno * 1\ncubic 0\n")
840838
for _, bad := range []string{"CCmod", "D", "PCB", "*", "1", "0"} {
@@ -848,3 +846,38 @@ func TestParseFreeBSDCC_DropsAllTableNoise(t *testing.T) {
848846
t.Fatalf("want 2 algorithms, got %v", got)
849847
}
850848
}
849+
850+
// TestAvailableCongestionControlFreeBSD actually drives the FreeBSD branch
851+
// (reader + table parser) through the injectable ccSysctl seam, on any host - the
852+
// coverage the "injected" comment previously claimed but never delivered.
853+
// Disconnecting the freebsd case now fails here instead of leaving the suite green.
854+
func TestAvailableCongestionControlFreeBSD(t *testing.T) {
855+
orig := ccSysctl
856+
defer func() { ccSysctl = orig }()
857+
858+
// 14.x table shape, via the seam -> parsed algorithm names, noise dropped.
859+
ccSysctl = func() ([]byte, error) {
860+
return []byte("CCmod D PCB\nnewreno * 1\ncubic 0\nhtcp 0\n"), nil
861+
}
862+
got := availableCongestionControlFor("freebsd")
863+
want := []string{"newreno", "cubic", "htcp"}
864+
if len(got) != len(want) {
865+
t.Fatalf("freebsd branch: got %v, want %v", got, want)
866+
}
867+
for i := range want {
868+
if got[i] != want[i] {
869+
t.Fatalf("freebsd branch: got %v, want %v", got, want)
870+
}
871+
}
872+
873+
// A read failure yields no dropdown, not a crash.
874+
ccSysctl = func() ([]byte, error) { return nil, errors.New("sysctl unavailable") }
875+
if got := availableCongestionControlFor("freebsd"); got != nil {
876+
t.Errorf("read error: got %v, want nil", got)
877+
}
878+
879+
// macOS/Windows offer nothing (the -C option aborts there).
880+
if got := availableCongestionControlFor("darwin"); got != nil {
881+
t.Errorf("darwin: got %v, want nil", got)
882+
}
883+
}

internal/speedtest/ookla.go

Lines changed: 21 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -939,6 +939,24 @@ var (
939939
}
940940
)
941941

942+
// ooklaPing is the ranking latency probe, a swap-a-var seam (like ooklaDownload/
943+
// ooklaUpload) so rankedServers' selection logic - which reachable/failed server
944+
// wins - is testable without a live server.
945+
var ooklaPing = func(ctx context.Context, srv *ookla.Server, cb func(time.Duration)) error {
946+
return srv.PingTestContext(ctx, cb)
947+
}
948+
949+
// uploadSpent is the data an upload attempt actually pushed across the (possibly
950+
// metered) link, for data-usage accounting. It is confirmed bytes (GetTotalUpload)
951+
// PLUS the backlog (GetUploadBacklog): bytes read into the socket but not
952+
// server-confirmed. speedtest-go v1.7.11 counts only confirmed bytes in
953+
// GetTotalUpload - correct for the RATE (ULSpeed) but it drops the bytes a FAILED
954+
// or aborted attempt already sent, which "data used" must still include. Download
955+
// has no equivalent gap (GetTotalDownload counts bytes actually received).
956+
func uploadSpent(srv *ookla.Server) int64 {
957+
return srv.Context.GetTotalUpload() + srv.Context.GetUploadBacklog()
958+
}
959+
942960
// errTransferAbandoned marks a transfer we walked away from because the context
943961
// died first (see runTransfer). It travels alongside the context error - both are
944962
// wrapped, so callers testing for context.Canceled still match - and it is what
@@ -1125,7 +1143,7 @@ func (o *Ookla) measure(ctx context.Context, srv *ookla.Server, dir string, retr
11251143
// Same ten samples the library already sends - the callback only keeps the
11261144
// fastest alongside the mean it returns, so this costs no extra probe.
11271145
var bestPing time.Duration
1128-
if err := srv.PingTestContext(ctx, keepFastestPing(&bestPing)); err != nil {
1146+
if err := ooklaPing(ctx, srv, keepFastestPing(&bestPing)); err != nil {
11291147
return Result{}, fmt.Errorf("ping: %w", err)
11301148
}
11311149
// Idle baseline for latency-under-load: same method/target as the loaded
@@ -1187,7 +1205,7 @@ func (o *Ookla) measure(ctx context.Context, srv *ookla.Server, dir string, retr
11871205
if !finished { // abandoned: srv belongs to the orphan now (see above)
11881206
return e
11891207
}
1190-
upBytes += srv.Context.GetTotalUpload() // count this attempt before the next Reset zeroes it
1208+
upBytes += uploadSpent(srv) // confirmed + backlog: a FAILED attempt's pushed bytes aren't in GetTotalUpload alone
11911209
if e == nil && ctx.Err() != nil {
11921210
e = ctx.Err()
11931211
}
@@ -1524,7 +1542,7 @@ func rankedServers(ctx context.Context, servers ookla.Servers, isp string) (ookl
15241542
// value - so "err == nil && Latency > 0" would record a one-shot
15251543
// echo as an answered ten-sample ranking ping.
15261544
sampled := false
1527-
err := s.PingTestContext(ctx, func(time.Duration) { sampled = true }) // sets s.Latency on success
1545+
err := ooklaPing(ctx, s, func(time.Duration) { sampled = true }) // sets s.Latency on success
15281546
pings[i] = applyRankPing(s, err, sampled)
15291547
}(i, s)
15301548
}

internal/speedtest/rankping_test.go

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,9 @@
11
package speedtest
22

33
import (
4+
"context"
45
"errors"
6+
"fmt"
57
"testing"
68
"time"
79

@@ -68,3 +70,34 @@ func TestApplyRankPingAndRanking(t *testing.T) {
6870
t.Error("an unreachable server must never rank ahead of a reachable one")
6971
}
7072
}
73+
74+
// rankedServers (the call site) must not let a candidate whose ranking ping
75+
// FAILED win on its stale discovery latency. Drives the site through the ooklaPing
76+
// seam: a reachable server measured at 11ms vs an unreachable one holding a stale
77+
// 1ms. The fix ranks the reachable one first; reverting applyRankPing's zeroing
78+
// lets the stale 1ms win (test fails) - the call-site coverage the helper-only
79+
// tests were missing.
80+
func TestRankedServersDropsUnreachableStaleLatency(t *testing.T) {
81+
orig := ooklaPing
82+
defer func() { ooklaPing = orig }()
83+
ooklaPing = func(ctx context.Context, srv *ookla.Server, cb func(time.Duration)) error {
84+
if srv.ID == "reachable" {
85+
srv.Latency = 11 * time.Millisecond // the library sets the measured mean on success
86+
cb(srv.Latency)
87+
return nil
88+
}
89+
return errors.New("connection refused") // no sample -> failed ranking ping
90+
}
91+
servers := ookla.Servers{
92+
{ID: "unreachable", Sponsor: "A", Name: "near", Distance: 0, Latency: 1 * time.Millisecond}, // stale, low
93+
{ID: "reachable", Sponsor: "B", Name: "far", Distance: 1, Latency: 0},
94+
}
95+
out, _ := rankedServers(context.Background(), servers, "")
96+
if len(out) == 0 || out[0].ID != "reachable" {
97+
var ids []string
98+
for _, s := range out {
99+
ids = append(ids, fmt.Sprintf("%s(%v)", s.ID, s.Latency))
100+
}
101+
t.Fatalf("reachable must rank first; a failed ping's stale 1ms must not win. order=%v", ids)
102+
}
103+
}

0 commit comments

Comments
 (0)