Repository navigation
fix(reach): do not cache interrupted query results - #603
Conversation
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.
Rapport — CS-Macmini PR#603 runde2 — head fdf3cd1Review feedback addressed (commit 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
|
| 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.
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
reachNodeScopesfollow-up query that turned any error into empty strings, and a degree-snapshot loader that didn't checkrows.Err()after its scan loop. Neither exists in that exact form here — there is no scopes feature, andreach_rank.go's degree-snapshot loader (loadDegreeSnapshot/scanRankRows) already checksrows.Err()after every loop and only publishes the snapshot on full success, confirmed by the existingTestReachRank_PartialReadIsNotPublished/TestReachRank_ColdDBErrorIsNotAnEmptyLeaderboard.scanReachRows(the main Reach observation scan) also already checksrows.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 andbuildNodeInfoMap(used by several endpoints) depend on. The third was found in round 2 of review and is a close analogue of the upstreamreachNodeScopescause:getAllNodes(cmd/server/store.go) discarded each row'sScanerror and never checkedrows.Err()after its loop — the one scan loop in this file that didn't follow that pattern (reach_rank.go'sscanRankRowsand two other spots instore.godo). A mid-iteration DB error (lock contention, I/O) ends therows.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.getCachedNodesAndPMpublished whatevergetAllNodesreturned, 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 cachedreliable_tokens/linksthat silently undercount or go empty, for the Reach response cache's full 5-minute TTL.buildNodeInfoMap's observer-enrichment query (cmd/server/neighbor_api.go) ignored the error from its follow-upSELECT id, name FROM observersand never checkedrows.Err()— a second query in the same build whose failure silently becomes empty strings, i.e. the same shape as the upstreamreachNodeScopescause. An interrupted enrichment cached a Reach report with observer names blanked for 5 minutes, still served after the DB recovered.Fix
getAllNodesreturns(nodes, ok).ok=falsereports an interrupted read: the deepest schema rung'sQueryerror, or a non-nilrows.Err()after the loop. A row that will notScanis a different thing — a malformed row (nodes.public_keyis a bareTEXT PRIMARY KEY, so SQLite admits one NULL; a non-numericlat/lonfailsScanthe same way) — and is skipped, logged and counted, the wayscanReachRowsalready discards malformed rows. One bad row must not disable the shared node cache for every caller.getCachedNodesAndPMkeeps 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 tobuildPrefixMap(nil)— empty but valid, since every caller assumes a non-nil*prefixMap.getCachedNodesAndPMOK/buildNodeInfoMapOKvariants additionally report whether what they returned is complete.computeNodeReachuses 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-droppinggetCachedNodesAndPM/buildNodeInfoMapwrappers keep their old shape for the ~30 callers that render live and cannot act on a failure, where a degraded field beats a 500.Queryerror and checksrows.Err(); a row that won'tScanstays a skipped malformed row (the observers table admits a NULL name, andreach_rank.go's roster scan is pinned to skipping those too).While wiring this up,
go test ./...oncmd/serversurfaced a real (not synthetic) nil-pointer panic inTestObsDedup411_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-startbuildPrefixMap(nil)fallback rather than returning a barenilprefix map.Requirements
getAllNodesfailure must reportok=false, never a truncated sliceTestGetAllNodes_MidScanFailureDoesNotTruncaterows.Err()check to_ = rows.Err()— red:ok=truewith 1/3 rowsScanis a malformed row: skipped, with the rest of the table loaded and the cache publishedTestGetAllNodes_MalformedRowIsSkippedNotFatalscanErr != nil → return nil, false— red:ok=false, cache never publishedgetCachedNodesAndPMmust keep the prior complete snapshot on a failed refresh, not replace itTestGetCachedNodesAndPM_MidScanFailureKeepsStaleCompleteSnapshotrows.Err()mutant — red: cache replaced with the 1-node partial readok=falseTestGetCachedNodesAndPM_ColdStartFailureReportsNotOKWithUsableEmptyPMpm; (b) fallback reportsok=true— both redTestNodeReach_NodeCacheRefreshFailureDoesNotCacheADegradedReportrows.Err()mutant — red:links=0, lost01FAtokenTestNodeReach_ColdStartNodeCacheFailureIsA500AndNotCachedok=true— red: 200 withreliable_tokens: [],links: [], cachedTestNodeReach_ObserverEnrichmentInterruptedIsA500AndNotCached(Queryerror and mid-scan subcases)Queryerror →return m, true; (b) drop the observersrows.Err()check — one subcase red eachFault injection: a
database/sql/driverwrapper around the realmodernc.org/sqlitedriver (same technique asping_score_bulk_test.go'scountingConn) whosedriver.Rows.Nextfails 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, soscanReachRows'LEFT JOIN observers obsandreach_rank's own roster scan stay healthy and the test isolates the enrichment read.Scope / residuals
map[string]interface{}outside tests;cmd/serverstays read-only (no new writes — only read-path error handling).getCachedNodesAndPMcall during such an outage retries the query. Scoped out as disproportionate to this issue; not filed.computeHashCollisionsandcomputeMultiByteCapabilityignoreokand degrade to an empty node list on an interrupted read. Correcting the round-1 description:computeHashCollisionsis cached — incollisionCache(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 changecomputeHashCollisions' signature,GetHashCollisions' cache-put and the recomputer closure, which is more than this issue's Reach scope. Not filed.Queryfailure on every schema rung returnednil, whichgetCachedNodesAndPMthen published as an emptynodePM. With this PR the stale snapshot is kept instead (and a cold start reportsok=false). That is a deliberate behaviour change in line with the issue's purpose, not a no-op.