fix(store): lazy distance build can crash (sync.Once reset during Do) and deadlock (lock order)

[dborup]

Summary

The lazy distance-index build can crash the server with fatal error: sync: unlock of unlocked mutex. PacketStore.distLazyOnce (a sync.Once) is reassigned to a zero value while Do may be running on it. sync.Once.Do holds its internal mutex for the whole callback; overwriting the struct zeroes that mutex, so the deferred unlock at the end of the build fails fatally (not recoverable, process exits).

Found by the independent review of #145 (which re-runs analytics right after the startup load and so makes the window larger). A second review reproduced the sync.Once mechanism deterministically (fatal error: sync: unlock of unlocked mutex from sync.(*Once).doSlow) and found a lock-order deadlock in the same state handling (see below). Analysis against master 5f493f1d.

Where

cmd/server/store.go:

• TriggerDistanceIndexBuild() (around lines 4658–4707): under distLazyMu it may reset s.distLazyOnce = sync.Once{} (around line 4680), then starts go s.distLazyOnce.Do(func() { ... }) (around line 4688). distLazyBuilding is only set inside the callback.
• The background chunk-load completion path (around lines 1677–1680) does s.distLazyBuilt = false; s.distLazyOnce = sync.Once{} under distLazyMu, without checking distLazyBuilding.

Failure scenario

1. A /api/analytics/distance request calls TriggerDistanceIndexBuild(); the goroutine enters Do, which locks the Once's mutex and runs the build (buildDistanceIndex under s.mu, which can take a while on a large store).
2. The background chunk load finishes and resets s.distLazyOnce = sync.Once{} while step 1 is still inside Do.
3. The build finishes; Do's deferred m.Unlock() runs on the zeroed mutex → fatal error: sync: unlock of unlocked mutex, server exits.

Holding distLazyMu around the reset does not help: Do's own mutex is what gets overwritten. It is also a data race on the Once's fields (done, m), which -race should report.

Proposed fix

Stop resetting a sync.Once. The state already has distLazyBuilding/distLazyBuilt under distLazyMu, which is enough:

• TriggerDistanceIndexBuild: under distLazyMu, return if distLazyBuilding; apply the existing debounce if distLazyBuilt; otherwise set distLazyBuilding = true before starting the goroutine, and run the build without a sync.Once.
• Chunk-load path: set distLazyBuilt = false (so the next request rebuilds) and bump a data generation; never touch a Once. The build records the generation it actually read and rebuilds again only if the dataset changed after that snapshot (see the lock-order section).
• Remove the distLazyOnce field.

Acceptance criteria

• Reproduce first: a test that holds the build open via the existing distanceBuildHook seam, triggers the chunk-load reset path meanwhile, and shows the failure (a -race report or the fatal error in a subprocess) on master.
• See the additional criteria in the lock-order section below.
• With the fix, the same test passes under -race -count=20; no sync.Once is reassigned anywhere in cmd/server (grep guard test is fine).
• Existing behaviour stays: at most one build at a time, debounce (Δobs > 5 % or 5 min), 202 while building, rebuild after the background load.
• Full cmd/server suite under -race.

Lock-order deadlock (same code, separate bug)

Removing the sync.Once alone does not fix this:

• TriggerDistanceIndexBuild() takes distLazyMu, then s.mu.RLock() to read totalObs (debounce path, when a build has completed before).
• The background-load completion path holds s.mu.Lock() and then takes distLazyMu (around lines 1669–1681).

Opposite lock order: a distance request holding distLazyMu waits for s.mu.RLock() while the loader holds s.mu and waits for distLazyMu → both block forever.

Added requirements

• Never acquire s.mu (Lock or RLock) while holding distLazyMu. Read totalObs before taking distLazyMu (or keep it atomic), or restructure so the locks are never nested in opposite orders.
• Prefer a generation/dirty counter over a plain rebuildRequested flag: record the generation the build actually read; rebuild again only if the dataset changed after that snapshot. This avoids a redundant expensive build when the loader finishes before the queued build gets s.mu.

Added acceptance criteria

• Deterministic test of the sync.Once crash (subprocess) or a real -race reproduction on master.
• Deterministic test of the lock-order deadlock on master, passing with the fix.
• A guard that no code path takes s.mu while holding distLazyMu.
• Many concurrent triggers start exactly one build.
• Loader finishes before the build acquires s.mu: no extra build.
• Loader changes the dataset after the build's snapshot: exactly one new build.
• 202 + Retry-After, debounce and post-load invalidation unchanged; full cmd/server suite under -race.

Order

Fix this issue first, then sync #145 with it (or land it as a clearly separate first commit in the same series).

[dborup]

Fixed by #151 (merged as 0bce43c0): distLazyOnce removed, s.mu is never taken while distLazyMu is held, and a data generation replaces the Once reset. Known, documented residual: the "generation read inside the build's s.mu section" invariant is pinned by a comment only (no deterministic test seam). #145 still has to be synced with this before it merges.