Skip to content

fix(reach): do not cache interrupted query results - #603

Merged
dborup merged 3 commits into
masterfrom
fix/593-reach-interrupted-cache
Oct 25, 2026
Merged

dborup merged 3 commits into
masterfrom
fix/593-reach-interrupted-cache

Conversation

@adminopenclaw8-sketch

@adminopenclaw8-sketch adminopenclaw8-sketch commented Oct 25, 2026 •

Copy link
Copy Markdown
Collaborator

Relates to #593.

Summary

Audited the Reach implementation (admission limiter #566, 429 retry #547, singleflight) for the bug upstream fixed in PR 2277: a query interrupted partway (context cancelled, timeout, a DB error) being cached as if it were a complete result.

The specific root causes upstream found were a reachNodeScopes follow-up query that turned any error into empty strings, and a degree-snapshot loader that didn't check rows.Err() after its scan loop. Neither exists in that exact form here — there is no scopes feature, and reach_rank.go's degree-snapshot loader (loadDegreeSnapshot/scanRankRows) already checks rows.Err() after every loop and only publishes the snapshot on full success, confirmed by the existing TestReachRank_PartialReadIsNotPublished/TestReachRank_ColdDBErrorIsNotAnEmptyLeaderboard. scanReachRows (the main Reach observation scan) also already checks rows.Err(), and the cold-build admission limiter (#566) and the rank/visibility re-derivation on every serve (currentReachRankView, visibleReach) are safe — a failure there is never baked permanently into the cache, since both are recomputed fresh on every serve, hit or miss.

The same bug class does exist in three places one layer down, in the shared node cache that computeNodeReach's prefix-map resolver and buildNodeInfoMap (used by several endpoints) depend on. The third was found in round 2 of review and is a close analogue of the upstream reachNodeScopes cause:

  1. getAllNodes (cmd/server/store.go) discarded each row's Scan error and never checked rows.Err() after its loop — the one scan loop in this file that didn't follow that pattern (reach_rank.go's scanRankRows and two other spots in store.go do). A mid-iteration DB error (lock contention, I/O) ends the rows.Next() loop exactly the way "no more rows" does, so without that check a genuine failure silently became a truncated-but-reported-successful node list.
  2. getCachedNodesAndPM published whatever getAllNodes returned, unconditionally, into the 30-second node cache — so a transient failure could replace a complete snapshot with a partial (or on a cold start, empty) one for the rest of that window, and any Reach response built from it during that window cached reliable_tokens/links that silently undercount or go empty, for the Reach response cache's full 5-minute TTL.
  3. buildNodeInfoMap's observer-enrichment query (cmd/server/neighbor_api.go) ignored the error from its follow-up SELECT id, name FROM observers and never checked rows.Err() — a second query in the same build whose failure silently becomes empty strings, i.e. the same shape as the upstream reachNodeScopes cause. An interrupted enrichment cached a Reach report with observer names blanked for 5 minutes, still served after the DB recovered.

Fix

  • getAllNodes returns (nodes, ok). ok=false reports an interrupted read: the deepest schema rung's Query error, or a non-nil rows.Err() after the loop. A row that will not Scan is a different thing — a malformed row (nodes.public_key is a bare TEXT PRIMARY KEY, so SQLite admits one NULL; a non-numeric lat/lon fails Scan the same way) — and is skipped, logged and counted, the way scanReachRows already discards malformed rows. One bad row must not disable the shared node cache for every caller.
  • getCachedNodesAndPM keeps serving the previously-cached (complete) snapshot on a failed refresh rather than replacing it, and retries on the next call. On a cold start with nothing cached it falls back to buildPrefixMap(nil) — empty but valid, since every caller assumes a non-nil *prefixMap.
  • New getCachedNodesAndPMOK / buildNodeInfoMapOK variants additionally report whether what they returned is complete. computeNodeReach uses them and turns an incomplete read into an error → 500, nothing cached, instead of publishing a report built on a placeholder prefix map or on blanked observer names. The ok-dropping getCachedNodesAndPM / buildNodeInfoMap wrappers keep their old shape for the ~30 callers that render live and cannot act on a failure, where a degraded field beats a 500.
  • The observers enrichment now handles its Query error and checks rows.Err(); a row that won't Scan stays a skipped malformed row (the observers table admits a NULL name, and reach_rank.go's roster scan is pinned to skipping those too).

While wiring this up, go test ./... on cmd/server surfaced a real (not synthetic) nil-pointer panic in TestObsDedup411_PathsKeepTheSameObservationsAndIndexes: a genuine transient scan failure during that test's concurrent ingest, previously masked by the swallow-the-error bug. That's what motivated the cold-start buildPrefixMap(nil) fallback rather than returning a bare nil prefix map.

Requirements

Requirement Test Mutant
A mid-scan getAllNodes failure must report ok=false, never a truncated slice TestGetAllNodes_MidScanFailureDoesNotTruncate Revert the rows.Err() check to _ = rows.Err() — red: ok=true with 1/3 rows
A row that will not Scan is a malformed row: skipped, with the rest of the table loaded and the cache published TestGetAllNodes_MalformedRowIsSkippedNotFatal scanErr != nil → return nil, false — red: ok=false, cache never published
getCachedNodesAndPM must keep the prior complete snapshot on a failed refresh, not replace it TestGetCachedNodesAndPM_MidScanFailureKeepsStaleCompleteSnapshot Same rows.Err() mutant — red: cache replaced with the 1-node partial read
A cold-start failure must return a non-nil, usable empty prefix map and report ok=false TestGetCachedNodesAndPM_ColdStartFailureReportsNotOKWithUsableEmptyPM (a) fallback returns a nil pm; (b) fallback reports ok=true — both red
A Reach response built while the node cache fails mid-refresh must not cache a degraded report TestNodeReach_NodeCacheRefreshFailureDoesNotCacheADegradedReport Same rows.Err() mutant — red: links=0, lost 01FA token
A cold-start node-cache failure must reach the handler as 500, with nothing cached TestNodeReach_ColdStartNodeCacheFailureIsA500AndNotCached Fallback reports ok=true — red: 200 with reliable_tokens: [], links: [], cached
An interrupted observer enrichment must fail the Reach build, not cache blanked names TestNodeReach_ObserverEnrichmentInterruptedIsA500AndNotCached (Query error and mid-scan subcases) (a) Query error → return m, true; (b) drop the observers rows.Err() check — one subcase red each

Fault injection: a database/sql/driver wrapper around the real modernc.org/sqlite driver (same technique as ping_score_bulk_test.go's countingConn) whose driver.Rows.Next fails after a chosen number of real rows, plus a variant that fails the query itself. That exercises an actual mid-iteration failure, not only a pre-query error. The observers faults match the enrichment query's exact text, so scanReachRows' LEFT JOIN observers obs and reach_rank's own roster scan stay healthy and the test isolates the enrichment read.

Scope / residuals

  • No new map[string]interface{} outside tests; cmd/server stays read-only (no new writes — only read-path error handling).
  • The fix adds no backoff for a persistently failing node-table scan: every getCachedNodesAndPM call during such an outage retries the query. Scoped out as disproportionate to this issue; not filed.
  • computeHashCollisions and computeMultiByteCapability ignore ok and degrade to an empty node list on an interrupted read. Correcting the round-1 description: computeHashCollisions is cached — in collisionCache (collisionCacheTTL, default 1 h) and by the analytics recomputer — so an interrupted node read during a cold recompute caches an empty collision list for that TTL. Before this change it cached a partial list instead, so the class of problem is pre-existing rather than introduced here, but it is not fixed: threading the failure out would change computeHashCollisions' signature, GetHashCollisions' cache-put and the recomputer closure, which is more than this issue's Reach scope. Not filed.
  • Also correcting the round-1 description's "pre-query error (already handled before this change)": before this change a Query failure on every schema rung returned nil, which getCachedNodesAndPM then published as an empty nodePM. With this PR the stale snapshot is kept instead (and a cold start reports ok=false). That is a deliberate behaviour change in line with the issue's purpose, not a no-op.

getAllNodes silently truncated the nodes-table scan on a mid-iteration
DB error (lock contention, I/O) instead of checking rows.Err() and each
Scan's error, unlike every other scan loop in this file. The resulting
partial list was published unconditionally into the 30s node cache that
buildNodeInfoMap and computeNodeReach's prefixMap depend on, so a Reach
response built during that window could cache an incomplete report for
its 5-minute TTL.

getAllNodes now reports ok=false on any Scan/iteration failure instead
of returning a truncated slice; getCachedNodesAndPM keeps serving the
prior complete snapshot on a failed refresh rather than replacing it,
falling back to an empty-but-valid prefixMap only on a cold start with
nothing cached yet.

Relates to #593.
Round-2 review of #603 (#593).

F1 (blocking): a per-row Scan failure in getAllNodes made the whole read
report ok=false, permanently. nodes.public_key is a bare TEXT PRIMARY KEY,
so SQLite admits one NULL row (and a non-numeric lat/lon fails Scan the
same way); one such row disabled the shared node cache for every caller
for the life of the process — on a cold start the cache is never published,
so every call re-runs the full nodes SELECT and Reach 404s a normal
repeater. A row that will not Scan is a malformed row, not an interruption:
skip, log and count it the way scanReachRows already discards malformed
rows. ok=false is kept for what this issue is about — the deepest schema
rung's Query error and rows.Err().

F2: a cold-start node-cache failure was never propagated to Reach. With
nothing cached yet and the target also an observer, buildNodeInfoMap still
found it through the observer enrichment, so a report with zero reliable
tokens and zero links was cached for the full 5-minute TTL and kept being
served after the DB recovered. getCachedNodesAndPMOK now reports whether
the snapshot it returns is the node table or the empty-but-valid fallback,
and computeNodeReach turns the latter into an error (500, nothing cached).
The ok-dropping getCachedNodesAndPM wrapper keeps its shape for the ~30
callers that cannot act on a failure.

F3: buildNodeInfoMap's follow-up "SELECT id, name FROM observers" ignored
its Query error and never checked rows.Err() — the local analogue of the
upstream reachNodeScopes root cause, a second query in the same build whose
error becomes empty strings. buildNodeInfoMapOK now reports that failure
and Reach fails the build instead of caching blanked observer names for 5
minutes; a row that will not Scan stays a skipped malformed row.

F4: the cold-start buildPrefixMap(nil) fallback now has its own test.

Tests: TestGetAllNodes_MalformedRowIsSkippedNotFatal,
TestGetCachedNodesAndPM_ColdStartFailureReportsNotOKWithUsableEmptyPM,
TestNodeReach_ColdStartNodeCacheFailureIsA500AndNotCached and
TestNodeReach_ObserverEnrichmentInterruptedIsA500AndNotCached (query error
and mid-scan subcases). All four are red on the previous head.

Relates to #593.
@adminopenclaw8-sketch

Copy link
Copy Markdown
Collaborator Author

Rapport — CS-Macmini PR#603 runde2 — head fdf3cd1

Review feedback addressed (commit eaf4d240a13912c51245d3d07e1819a122f8aafa, merged with origin/master c682051a in fdf3cd10).

Evidence labels: [T] I ran it, [K] from reading the code at head, [A] from someone else's report, not re-verified.

1. F1 (blocking) — a per-row Scan error is a malformed row, not an interrupted read

Fixed as suggested. getAllNodes now skips, logs and counts a row that will not Scan, the way scanReachRows already discards malformed rows, and keeps ok=false for exactly the two things this issue is about: the deepest schema rung's Query error, and a non-nil rows.Err() after the loop [K].

The old scanErr != nil → return nil, false was the regression you measured: with one NULL public_key row the cache was never published, so every caller re-ran the full nodes SELECT forever and Reach 404'd a normal repeater. computeHashCollisions/computeMultiByteCapability went permanently empty with it.

New test TestGetAllNodes_MalformedRowIsSkippedNotFatal: three well-formed rows plus one with NULL public_key. Asserts ok=true, 3 nodes, getCachedNodesAndPM returns 3, store.nodeCache/store.nodePM are both non-nil (the cache is published), and uniqueResolve(pm, "aa") resolves — i.e. the snapshot is usable, not just non-empty [T].

The count is log-only ([store] getAllNodes discarded N malformed nodes rows (kept M)), with no exported counter — matching scanReachRows, which also only logs its skipped. The behavioural half (skip, don't fail, publish) is what the test pins.

2. F2 — a cold-start node-cache failure now reaches Reach as a 500, not a cached empty report

Fixed, not scoped out. Your probe P3 is now a test and the shape is exactly what you suggested.

  • New getCachedNodesAndPMOK() ([]nodeInfo, *prefixMap, bool) reports whether what it returns is the node table or the empty-but-valid buildPrefixMap(nil) placeholder. ok=false only on the cold-start branch (failed refresh with no complete snapshot to fall back on); a stale-but-complete snapshot still reports ok=true, since it is complete [K].
  • getCachedNodesAndPM is now a thin wrapper that drops ok, so the ~30 callers that render live and cannot act on a failure are byte-for-byte unchanged in behaviour [K].
  • computeNodeReach uses the OK variants and turns an incomplete read into an error, which handleNodeReach already maps to 500 with nothing cached (the reach: scanReachRows DB error surfaces as 404 instead of 500 Kpa-clawbot/CoreScope#1631 path) [K].
  • The singleflight waiters you flagged (nodeCacheLoad handing the same empty snapshot to everyone with no error) get the ok flag too — it travels in the nodeSnapshot the closure returns [K].

New test TestNodeReach_ColdStartNodeCacheFailureIsA500AndNotCached: the target is both a node and an observer (your P3 setup — the case where the empty node cache stays invisible to the Reach build because the observer enrichment still finds the target). Before the fix the handler answered 200 with reliable_tokens: [], links: [], role: "observer" and cached it; now 500, len(srv.reach.cache) == 0, and the very next request after the fault clears is a 200 with the 01FA token and both links — no TTL to wait out [T].

In production this path is only reachable before the first successful load: InvalidateNodeCache still has no non-test callers [T].

3. F3 — the observer enrichment is the local analogue, and is now handled

Fixed here rather than scoped out, and the PR description no longer claims the analogue is absent. buildNodeInfoMapOK handles the Query error from SELECT id, name FROM observers and checks rows.Err() after the loop. A row that will not Scan stays a skipped malformed row — the observers table admits a NULL name, and reach_rank.go:398 is pinned to buildNodeInfoMap skipping exactly those — but it is now counted and logged instead of silently dropped [K]. buildNodeInfoMap keeps its old signature as the ok-dropping wrapper for the three live-rendering call sites (neighbor_api.go:183, :354, neighbor_debug.go:83), where a degraded name beats a 500 [K].

New test TestNodeReach_ObserverEnrichmentInterruptedIsA500AndNotCached, two subcases — the query itself failing, and failing on its first Next(). Both: 500, reach cache empty, 200 again once the fault clears [T]. Red on the previous head in both subcases (200 with a full report, cached) [T].

The fault injector matches the enrichment query's exact text, so scanReachRows' LEFT JOIN observers obs and reach_rank's SELECT id, name FROM observers WHERE id IS NOT NULL stay healthy — the test isolates the enrichment read rather than breaking the whole build [K].

4. F4 — the cold-start buildPrefixMap(nil) fallback has its own test

TestGetCachedNodesAndPM_ColdStartFailureReportsNotOKWithUsableEmptyPM asserts, on a cold start with the first scan failing: ok=false, no nodes, a non-nil pm, and that the pm is usable — uniqueResolve returns "" and pm.without(...) does not nil-deref (the two calls Reach makes). It also pins the ok-dropping wrapper's shape [T]. No longer dependent on TestObsDedup411… killing M4 incidentally.

5. F5 — PR description corrected

Both points fixed in the body:

  • (a) computeHashCollisions is no longer called "uncached analytics". It is cached in collisionCache (collisionCacheTTL, default 1 h) and by the analytics recomputer [T, store.go:9823, analytics_recomputer.go:501], so an interrupted node read during a cold recompute caches an empty collision list for that TTL. Before this PR it cached a partial list instead, so the class is pre-existing rather than introduced — but it is not fixed, and the description now says so. I scoped the code fix out deliberately: threading the failure out changes computeHashCollisions' signature, GetHashCollisions' cache-put and the recomputer closure, which is more than this issue's Reach scope. Not filed. If you want it in this PR, say so and I will do it.
  • (b) The "pre-query error (already handled before this change)" line is gone. The body now states that before this change a Query failure on every schema rung returned nil and getCachedNodesAndPM published it as an empty nodePM, that the stale snapshot is now kept instead (and a cold start reports ok=false), and that this is a deliberate behaviour change in line with the issue's purpose.

The description also now has a row per requirement for the four new tests, and the getCachedNodesAndPMOK/buildNodeInfoMapOK split is written up.

Not disputed

Nothing. All five findings are accepted; F5(a)'s code fix is the only thing scoped out, with the reasoning above.

Tests

Merged tree: origin/master c682051a merged into the branch as fdf3cd10 (merge commit, no rebase/amend/force-push).

  • cmd/server: go build ./..., go vet ./..., go test -count=1 ./... — ok [T].
  • cmd/server go test -count=1 -race on the Reach/store/ObsDedup/neighbour/SQL-lock subset — ok [T].
  • cmd/ingestor go test -count=1 ./... — ok [T].
  • sh test-all.sh — 273 passed, 0 failed [T].
  • node test-frontend-helpers.js — 731 passed, 0 failed [T].
  • scripts/check-xss-sinks.sh --diff origin/master — no public/** changes to scan (backend-only) [T].
  • E2E against a local Go server on e2e-fixture.db, prepared as in CI (freshen, the 1486/1791 seed SQL, corescope-migrate, then seeds 2073, 199, 245, 122), port 13805, stopped by port afterwards [T]:
    • test-node-reach-e2e.js OK
    • test-node-reach-429-e2e.js OK
    • test-reach-rank-e2e.js 14/14
    • test-issue-1630-reach-mobile-e2e.js 7/7
    • test-issue-1306-collisions-terminology-e2e.js 23/23
    • test-issue-353-path-hash-warn-e2e.js 25/25
    • test-observer-direct-neighbors-panel.js 21/21, test-show-neighbors.js 4/4 (the other buildNodeInfoMap consumers)
    • test-e2e-playwright.js 156/159 with 3 skipped
    • test-node-reach-coverage-e2e.js SKIP (clientRxCoverage disabled on the fixture)
  • Live smoke on that server: /api/nodes 200, /api/nodes/{pk}/reach?days=30 200 with tokens, /api/reach-rank 200, /api/analytics/hash-collisions 200 [T].

Red before / green after. All four new tests fail on the previous head 6bf3d868 with the symptoms the review described [T]:

Test Symptom on 6bf3d868
TestGetAllNodes_MalformedRowIsSkippedNotFatal ok=false on one NULL-pubkey row
TestGetCachedNodesAndPM_ColdStartFailureReportsNotOKWithUsableEmptyPM ok=true reported for the placeholder map
TestNodeReach_ColdStartNodeCacheFailureIsA500AndNotCached 200 with reliable_tokens: [], links: [], cached
TestNodeReach_ObserverEnrichmentInterruptedIsA500AndNotCached (both subcases) 200 with a full report, cached

For F2/F3/F4 the "before" was produced by first landing the …OK plumbing with base semantics (always ok=true, computeNodeReach unchanged) so the package compiled, running the tests red, then implementing the real semantics — not by a signature-only stub.

Mutants

One per finding, on the merged tree.

Mutant Result
M-F1: scanErr != nil → return nil, false (the previous head's behaviour) killed by TestGetAllNodes_MalformedRowIsSkippedNotFatal [T]
M-F2: cold-start fallback reports ok=true killed by the cold-start cache test and TestNodeReach_ColdStartNodeCacheFailureIsA500AndNotCached [T]
M-F3a: observers Query error → return m, true killed by the query fails subcase [T]
M-F3b: drop the observers rows.Err() check killed by the fails mid-scan subcase [T]
M-F4: cold-start fallback returns a nil pm killed by the cold-start cache test [T]

Regression checks that the round-1 mutants are still killed after the F1 change:

Mutant Result
M1: drop getAllNodes' rows.Err() killed by all 5 of the relevant tests [T]
M2: getCachedNodesAndPM ignores ok and publishes killed by 4 tests [T]

M3 from round 1 ("skip the bad row") is now the intended behaviour, and its inverse M-F1 is killed. M4 is killed directly rather than incidentally.

CI per job

Run 40214788791 on fdf3cd10, attempt 1 [T]:

Job Result
✅ Go Server Tests (1/3), (2/3), (3/3) pass
✅ Go Ingestor Tests pass
✅ Go Libraries & CLI pass
✅ Go Build & Test pass
✅ JavaScript Unit Tests & Lint pass
✅ Script & Container Unit Checks pass
📋 Verify Go Test Assignments pass
🧱 Validate Docker Image pass
🎭 Playwright E2E Tests (1/3), (3/3) pass
🎭 Playwright E2E Tests (2/3) fail — see below
📊 Verify E2E Shards & Coverage fail in 5 s (downstream gate on shard 2)
🏗️ Build & Publish Docker Image fail in 6 s (downstream gate on shard 2)

Attempt 2 (gh run rerun --failed, one re-run only) — all three re-run jobs pass, so every job in the run is green [T]:

Job Result
🎭 Playwright E2E Tests (2/3) pass
📊 Verify E2E Shards & Coverage pass
🏗️ Build & Publish Docker Image pass

The shard-2 failure is a NEW CI flake, not #271 or #301, and not this PR. Shard 2's test-e2e-playwright.js stopped fail-fast on Scope Audit transport: deep link, scope, region, limitations and 390px layout: precondition: link without region — tools/scope-audit-e2e.js:133, which came in with the origin/master merge, not from this branch [T, shard-2 receipt result.json + e2e-output.txt]. Everything before it in that suite passed; 14 of the shard's 15 suites passed.

Why it cannot be this change [K]: the assertion is purely a frontend one on location.hash, this PR has zero public/** changes (check-xss-sinks.sh --diff origin/master reports nothing to scan), and cmd/server/scope_audit_transport.go references neither getAllNodes, getCachedNodesAndPM nor buildNodeInfoMap.

What the race is [K]: the test does localStorage.removeItem('meshcore-region-filter') on the page where region=SJC is still the live selection, then navigates to a link without region= and reloads, asserting the URL gained no region=. public/region-filter.js keeps the selection in the in-memory _selected and writes it back with saveToStorage() (region-filter.js:22), while _selected = loadFromStorage() runs once at script load (:30). Any saveToStorage() that lands between the test's removeItem and the navigation — e.g. a still-pending regions-load/apply callback — restores SJC to storage, and the reload then puts region=SJC back in the URL. A slower runner widens that window, which is why it reproduces on CI and not here.

Local repro attempt [T]: 12/12 clean on the merged tree, driving tools/scope-audit-e2e.js on its own against a local instrumented server (public-instrumented, e2e-fixture.db prepared as in CI) — so it does not reproduce on this machine. Not filed; it belongs to the Scope Audit transport work, not here.

@dborup
dborup marked this pull request as ready for review October 25, 2026 18:21
@dborup
dborup merged commit b79a776 into master Oct 25, 2026
33 of 36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants