From 7faca593c10afe9f2203286044cc77a215434c28 Mon Sep 17 00:00:00 2001 From: dborup Date: Sun, 4 Oct 2026 13:10:17 +0000 Subject: [PATCH 1/2] test(server): wait for the reach-rank refresh a test leaves running (#224) TestReachRank_ExpiredSnapshotAlwaysRefreshes hammers the expired snapshot from 16 goroutines. A caller that read the expired snapshot just before the refresh published can register one more singleflight refresh before the callers return. That refresh runs after the test, finds the snapshot cleared by resetReachState's cleanup, and calls onDegreeSnapshotLoad with no happens-before edge to the next test, which swaps the hook: the -race report in TestReach Rank_RefreshHonoursBackoffAfterQueuedFailure. awaitDegreeRefresh joins the "degree" key once more at the end of the test and receives its result: it waits for any refresh still in flight (or runs one that returns the fresh snapshot without a load), which orders that refresh before the test returns. Test-only: an atomic or per-server hook would hide the report but still let the leftover refresh call the next test's hook. Co-Authored-By: Claude Opus 5.5 --- cmd/server/reach_rank_test.go | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/cmd/server/reach_rank_test.go b/cmd/server/reach_rank_test.go index 5136fd8b5..b8cb8cef3 100644 --- a/cmd/server/reach_rank_test.go +++ b/cmd/server/reach_rank_test.go @@ -953,6 +953,25 @@ func TestReachRank_ExpiredSnapshotAlwaysRefreshes(t *testing.T) { t.Fatalf("round %d: expired snapshot never refreshed", round) } } + awaitDegreeRefresh(t, srv) +} + +// awaitDegreeRefresh waits for the singleflight snapshot refresh a test may +// have left running (#224). A caller that read the expired snapshot just +// before the refresh published can register one more refresh before the +// test's callers return. That refresh runs after the test, finds the +// snapshot cleared by resetReachState's cleanup, and calls +// onDegreeSnapshotLoad, unsynchronised with the next test that swaps the +// hook. Joining the key here waits for it (or, with none left, runs one that +// returns the fresh snapshot without a load); the receive orders its end +// before the test returns. +func awaitDegreeRefresh(t *testing.T, srv *Server) { + t.Helper() + select { + case <-srv.refreshDegreeSnapshot(context.Background()): + case <-time.After(10 * time.Second): + t.Fatal("snapshot refresh still running 10s after the test") + } } // A caller that queued behind a failing rebuild must not start another load From 7862f67ae672f4a4eb9678a990854e1327b0f516 Mon Sep 17 00:00:00 2001 From: dborup Date: Sun, 4 Oct 2026 13:10:24 +0000 Subject: [PATCH 2/2] test(server): hold the subpath index build open for the #1008 503 test (#227) TestIssue1008_HandlerReturns503WhileSubpathIndexLoading checked the not-ready window right after Load(), racing the background subpath index build of a 6-transmission fixture; under CPU load the build won and the handler answered 200. PacketStore gets subpathBuildGate, a func field that is nil in production. startBackgroundIndexBuilds reads it before starting the subpath builder goroutine, which calls it first when it is set. The test sets it before Load() to block the build, asserts the 503, then releases it, waits for WaitIndexesReady and asserts 200. A per-store field rather than a package variable: background builds of other tests' stores never read it, so it cannot race the way #224's package hook did. Production cost is one nil check per Load(); readiness behaviour is unchanged. Co-Authored-By: Claude Opus 5.5 --- cmd/server/index_ready_1008.go | 4 ++++ cmd/server/index_ready_1008_test.go | 32 ++++++++++++++++++++++++++--- cmd/server/store.go | 4 ++++ 3 files changed, 37 insertions(+), 3 deletions(-) diff --git a/cmd/server/index_ready_1008.go b/cmd/server/index_ready_1008.go index 70256c9e7..c075a62bc 100644 --- a/cmd/server/index_ready_1008.go +++ b/cmd/server/index_ready_1008.go @@ -159,7 +159,11 @@ func (s *PacketStore) maybeCloseIndexReadyCh() { // parallel halves the pathHop-not-ready window since the two builders // are independent of each other. func (s *PacketStore) startBackgroundIndexBuilds() { + gate := s.subpathBuildGate // read before the goroutine starts go func() { + if gate != nil { + gate() + } t0 := time.Now() s.mu.Lock() s.buildSubpathIndex() diff --git a/cmd/server/index_ready_1008_test.go b/cmd/server/index_ready_1008_test.go index e8a602efd..f5d8027a8 100644 --- a/cmd/server/index_ready_1008_test.go +++ b/cmd/server/index_ready_1008_test.go @@ -14,6 +14,7 @@ import ( "encoding/json" "net/http" "net/http/httptest" + "sync" "testing" "time" ) @@ -50,16 +51,30 @@ func TestIssue1008_PathHopIndexReadyFalseImmediatelyAfterLoad(t *testing.T) { // TestIssue1008_HandlerReturns503WhileSubpathIndexLoading asserts the // analytics/subpaths handler returns 503 + Retry-After: 5 + a JSON body -// matching the triage spec while the subpath index is still building. +// matching the triage spec while the subpath index is still building, +// and 200 once it is built. +// +// The background build of the small fixture used to finish before the +// request under CPU load, so the 503 was a race the test could lose (#227). +// subpathBuildGate now holds the build until the 503 has been asserted. func TestIssue1008_HandlerReturns503WhileSubpathIndexLoading(t *testing.T) { db := setupRichTestDB(t) defer db.Close() store := NewPacketStore(db, nil) + release := make(chan struct{}) + var releaseOnce sync.Once + releaseBuild := func() { releaseOnce.Do(func() { close(release) }) } + t.Cleanup(releaseBuild) // a failed assertion must not strand the builder + building := make(chan struct{}) + store.subpathBuildGate = func() { close(building); <-release } if err := store.Load(); err != nil { t.Fatalf("Load() error: %v", err) } - // Don't wait for the background build — we want to observe the - // not-ready window. + select { + case <-building: // the build has started and is held + case <-time.After(5 * time.Second): + t.Fatal("background subpath index build never started") + } cfg := &Config{} cfg.applyListLimitsDefaults() srv := &Server{store: store, cfg: cfg} @@ -81,6 +96,17 @@ func TestIssue1008_HandlerReturns503WhileSubpathIndexLoading(t *testing.T) { if body["error"] != "index loading" { t.Errorf(`body["error"] = %v, want "index loading"`, body["error"]) } + + // Released, the build completes and the same request is served. + releaseBuild() + if !store.WaitIndexesReady(5 * time.Second) { + t.Fatal("indexes not ready within 5s of releasing the build") + } + rec = httptest.NewRecorder() + srv.handleAnalyticsSubpaths(rec, httptest.NewRequest("GET", "/api/analytics/subpaths?minLen=2&maxLen=4&limit=10", nil)) + if rec.Code != http.StatusOK { + t.Fatalf("status after the build = %d, want 200", rec.Code) + } } // TestIssue1008_HandlerRecoversAfterIndexReady asserts that, once the diff --git a/cmd/server/store.go b/cmd/server/store.go index a2ba63611..c366bd70c 100644 --- a/cmd/server/store.go +++ b/cmd/server/store.go @@ -309,6 +309,10 @@ type PacketStore struct { // indexReadyCh / maybeCloseIndexReadyCh in index_ready_1008.go. indexReadyChMu sync.Mutex indexReadyChan chan struct{} + // subpathBuildGate, nil in production, runs at the start of the + // background subpath index build that Load() starts. A test sets it + // before Load() to hold the not-ready window open (#227). + subpathBuildGate func() // Precomputed distance analytics: hop distances and path totals. // Built LAZILY on first /api/analytics/distance request (#1011) — // previously eager in Load() at startup, which was O(n²) work for