port(upstream#1882): nodes region filter reads the indexed from_pubkey column

[adminopenclaw8-sketch]

Split out of #25 (commit 9334d1a7 there). This branch holds exactly one upstream change so it can be reviewed, tested and reverted on its own.

Upstream

• PR: Kpa-clawbot/CoreScope#1882 by @Jonher937, merged 2026-09-02
• Upstream commit: eb8f376c6c29c8e703deebc1eb2a3dd8467a7276
• Applied with git cherry-pick -x onto master fda24ca5; upstream authorship kept, and the commit message carries the (cherry picked from commit …) line.
• Patch-id identical to the port(upstream): 26 clean upstream fixes — prune batching, /ws limits, observer liveness, watchdog race #25 commit and to the upstream commit.
• Scope: cmd/server/db.go (GetNodes region filter).

Problem

The region subquery in GetNodes extracted the advert pubkey with JSON_EXTRACT(t.decoded_json, '$.pubKey') for every joined row, although Kpa-clawbot#1143 added the dedicated, indexed transmissions.from_pubkey column that the other node queries already use.

Change

Uses t.from_pubkey in the subquery.

Adaptation to this fork

None in content. The changed lines are identical to upstream; only the surrounding diff context differs because this fork's files have diverged around them.

Notes for review

AGENTS.md rule 0 (perf proof): the upstream PR has no measurement. Locally checked on the E2E fixture DB (no production data was used):

• EXPLAIN QUERY PLAN is identical before and after (idx_transmissions_payload_type → idx_observations_tx_ts → observers rowid). The change only removes the per-row JSON_EXTRACT/JSON parse.
• Results are identical: 55 nodes for regions SJC+OAK both ways, and 0 advert rows where from_pubkey differs from decoded_json.pubKey.
• The fixture is too small to show a timing difference (both ~2 ms per query), so the performance benefit is unproven at production scale.
• Correctness relies on from_pubkey being populated for all adverts, which this fork's from_pubkey_v1 migration backfills. That was not checked against production data.

Dependencies and merge order

• Independent. Branched directly from master fda24ca5 and needs no other PR from this split.
• A merge simulation of all 24 split PRs gives the same tree in upstream order and in reverse order.
• The same simulation also merges cleanly with each of the open API: expose on-wire channelHashHex on channel messages #11, feat(map): support CARTO basemap API keys #13 and chore(deps): bump to golang:1.27.1-alpine3.24 and alpine:3.24 #22, and with test(ingestor): make neighbor metrics pruning deterministic #33 (opened during this split; it makes TestPruneOldNeighborMetrics deterministic). If test(ingestor): make neighbor metrics pruning deterministic #33 lands first, the expected CI failure named below disappears; nothing in this PR depends on it.
• Recommended position: 12 of 24, following upstream's merge order (the order port(upstream): 26 clean upstream fixes — prune batching, /ws limits, observer liveness, watchdog race #25 was assembled in).

Verification

Local run of the same commands as CI's “Go Build & Test” job (server tests with -race), on this branch and on master fda24ca5 under the same conditions (same machine, run one after another):

Check	master fda24ca5	this branch	verdict
go-server-build-vet	PASS	PASS
go-server-test-race	PASS	PASS
channel-lib-test	PASS	PASS
decrypt-cli-build-test	PASS	PASS
dockerfile-copy-invariants	FAIL	FAIL	not runnable locally: script needs bash ≥4 (declare -A), macOS has 3.2; identical on master
staging-disk-monitor	PASS	PASS
css-vars-lint	PASS	PASS

Baseline failures (fail identically on master; not introduced or changed here): see rows marked baseline failure, unchanged.

Browser validation (local, fixture DB, no staging/production): Not applicable (no frontend change).

Not run:

• Playwright E2E suites (no local Playwright install); CI's E2E job will also be skipped, see below.
• eslint (not installed locally; CI installs it on the fly).
• Frontend JS suites (no frontend change).
• Browser tests against staging/production (deliberately none).

Expected GitHub CI: “Go Build & Test” is expected to fail on TestPruneOldNeighborMetrics, which already fails on master (see #25's run). Downstream jobs (Playwright, image build) are therefore skipped. “Deploy Staging” and all GHCR publish steps only run on push to master and cannot run for this PR.
Two further ingestor tests have failed intermittently in this split's CI on branches whose cmd/ingestor tree is byte-identical to master (#27, #28), so they can also appear here without being caused by this change:

• TestBackfillTxLastSeen_ResolvesFromMaxObservationTimestamp: also reproduced locally on unmodified master.
• TestMQTTStallWatchdog_DisconnectedEscalationThrottled_1749: the suite flake that upstream test(ingestor): join the watchdog loop goroutine instead of only asking it to stop Kpa-clawbot/CoreScope#2003 (also split out of port(upstream): 26 clean upstream fixes — prune batching, /ws limits, observer liveness, watchdog race #25) addresses.

GitHub CI result: run 34751044665 on 5afba186. Go Build & Test: failure; all downstream jobs incl. Deploy Staging skipped. Failed tests:

• TestPruneOldNeighborMetrics: fails on master, documented baseline

🤖 Generated with Claude Code

[dborup]

Parked: correct only while from_pubkey is fully populated

Independent review, with measurements on a purpose-built 120k-transmission database, found this swap is not result-equivalent in production. It is equivalent only when every ADVERT row has from_pubkey set, and two paths break that. The failure is silent: the region filter returns fewer nodes, never an error.

The equivalence argument itself holds where the column is populated, and I verified it by construction rather than by sampling: Payload.PubKey is assigned in exactly one place, decodeAdvert (cmd/ingestor/decoder.go:336), reached only from case PayloadADVERT. So decoded_json.pubKey exists precisely for the rows from_pubkey is meant to cover. Case matches too — the same Go string feeds both, and both columns are BINARY-collated.

Where it breaks

1. POST /api/packets never writes the column. cmd/server/routes.go:1524 inserts (hash, raw_hex, route_type, payload_type, payload_version, decoded_json, first_seen) — no from_pubkey. An ADVERT posted there has payload_type=4 and decoded_json.pubKey but from_pubkey NULL. The old expression found it; the new one does not, permanently — BackfillFromPubkey is a one-shot at ingestor startup (cmd/ingestor/main.go:93), so only an ingestor restart repairs it.

2. The mid-backfill window, measured. 120,000 transmissions / 359,766 observations / 36,189 ADVERTs / 8,000 nodes / 60 observers across 10 IATA codes:

from_pubkey NULL	dense region	sparse region
25%	7830 → 7552 (−3.5%)	5588 → 4707 (−15.8%)
100%	7830 → 0	5588 → 0

The loss is superlinear in region sparsity: a node survives only if at least one of its ADVERTs in that region is already backfilled. The backfill can also abandon rows permanently — cmd/ingestor/maintenance.go:181,205,221 bare-return on select/begin/commit errors.

3. The one operator signal for that window is hardcoded green. cmd/server/from_pubkey_migration.go:18 sets fromPubkeyBackfillDone = true and nothing ever writes it; the ingestor passes nil for the progress callback. /api/healthz therefore always reports done: true, and public/warmup-banner.js:79-85 auto-dismisses on that — so the banner built to warn "attribution is incomplete right now" never shows. Pre-existing, but this change is what makes a user-visible query depend on that dead signal.

The existing tests cannot catch any of it

TestGetNodesFiltering/region_filter_* and TestGetNodesRegionFilterV2 do cover the changed line, and they pass. But all three helpers (db_test.go:142, db_test.go:1951, coverage_test.go:54) install trigger test_from_pubkey_advert, which auto-populates from_pubkey on every ADVERT insert — so in the test world the two expressions are equal by construction and the suite passes identically with either.

A test that would earn its keep: add a second node + ADVERT observed by the SJC observer, then UPDATE transmissions SET from_pubkey = NULL for it (the trigger is INSERT-only, so it sticks), and assert ?region=SJC returns both nodes. Passes on master, fails on this branch.

The perf case is weaker than stated, and the real argument is elsewhere

I measured 2.5x on the 500-row fixture; that does not survive scale. At 120k rows it is 1.17–1.31x (75–90 ms absolute either way, dominated by the transmissions→observations→observers fan-out). And idx_transmissions_from_pubkey is not used by this query — dropping it gives byte-identical EXPLAIN QUERY PLAN and timings within noise. The query drives off idx_transmissions_payload_type; from_pubkey is only projected, never searched. So the comment's "dedicated, indexed column" misattributes the win, which is really ~110k avoided JSON_EXTRACT calls.

The stronger argument the PR does not make: the old expression is a latent 500. SELECT JSON_EXTRACT('NOT JSON','$.pubKey') raises malformed JSON, so a single ADVERT with corrupt decoded_json in the queried region makes the whole subquery error and /api/nodes?region= return 500. The new expression is immune. This also removes the last $.pubKey SQL extraction in non-test code repo-wide.

Suggested resolution

COALESCE(t.from_pubkey, JSON_EXTRACT(t.decoded_json, '$.pubKey')) would keep nearly all of the win (JSON parsed only for the NULL minority) while being correct in every case above, including during backfill. Alternatively: populate from_pubkey in the POST /api/packets insert, and revive the backfill progress signal so the window is at least visible. Either way this wants the NULL-row test above.

Branch is synced with current master; go test -race ./... is clean (268s, 0 failures, 0 races).

[dborup]

Korrektion: min tidligere blocker holdt ikke — målt mod jeres rigtige database

Jeg parkerede tidligere denne PR som en bevist blocker med påstanden om "15,8 % nodetab". Det var forkert, og jeg trækker det tilbage. Tallet stammede fra et syntetisk backfill-scenarie med 25 % NULL, ikke fra denne installation. Jeg burde have målt mod den rigtige database, før jeg kaldte den en blocker.

Hvad de rigtige data siger

Målt read-only mod stagings 4,9 GB meshcore.db:

ADVERT-rækker i alt	62.104
med from_pubkey udfyldt	62.079
NULL	25 — 0,04 %
noder matchet, GAMMEL (JSON_EXTRACT)	1080 af 1084
noder matchet, NY (from_pubkey)	1080 — forskel 0
resultat for region AAR	877 begge veje, identisk

Formatet er også identisk — begge giver lowercase hex, der matcher nodes.public_key tegn for tegn.

Og de 25 NULL-rækker er ikke tabt data: ingen af dem har en pubKey i decoded_json heller. De er NULL, fordi der aldrig var en pubkey at gemme — hvilket stemmer med garden i cmd/ingestor/db.go, der kun sætter kolonnen, når decoded.Payload.PubKey != "".

Performance — målt ordentligt

Første måling svingede for meget til en konklusion (boksen ingester samtidig). Gentaget med 9 interleaved runder, så samtidig trafik ikke kan favorisere én variant:

variant	median	mod GAMMEL
GAMMEL (JSON_EXTRACT)	934,3 ms	—
NY (from_pubkey)	835,0 ms	1,12×
COALESCE-fallback	887,5 ms	1,05×

Alle tre giver 877. Så gevinsten er reel, men mindre end en størrelsesorden.

To ting rettet i 9eac67f3 — SQL'en er urørt

1. Kommentaren påstod, at gevinsten kom fra indekset. Det gør den ikke. EXPLAIN QUERY PLAN er byte-identisk før og efter — begge driver på idx_transmissions_payload_type, og idx_transmissions_from_pubkey indgår slet ikke. Gevinsten er udelukkende den sparede JSON-parsning per række. Titlens ord "indexed" er derfor misvisende, selvom ændringen er god.

2. Fælden var udokumenteret. from_pubkey skrives kun for ADVERTs, der bærer en pubkey, og en NULL falder lydløst ud af IN (...). Det er sikkert i dag, fordi det er målt — men det holder op med at være sikkert i samme øjeblik en skrivesti udfylder decoded_json uden at udfylde from_pubkey. Den advarsel står nu i koden, med tallene der gør den efterprøvelig.

COALESCE-fallback: skrevet, målt, forkastet

Jeg byggede COALESCE(t.from_pubkey, JSON_EXTRACT(t.decoded_json, '$.pubKey')) som sikkerhedssele. Den blev forkastet på målinger: den reddede 0 rækker på rigtige data og kostede halvdelen af gevinsten (1,12× → 1,05×). Den beslutning er skrevet ind i kommentaren, så den næste læser ikke gentager eksperimentet.

Branchen er synkroniseret med master 167ad111 (almindelig merge-commit 8855540d). go build, go vet og de målrettede node-/region-tests er grønne.

🤖 Generated with Claude Code

[dborup-agent]

Rapport — CS-pve-agent2 PR#38 runde 2 — head fd0b8a9

Review feedback addressed (commit fd0b8a97)

Evidence labels: [T] tested or measured in this round · [K] verified by reading code · [A] assumption or inference, not verified.

Commits this round (no rebase, amend or force-push):

• 8a21ce1c merge of origin/master 6444294d (clean, no conflicts). The diff against master is still the one GetNodes line plus its comment, and the tests below. [T]
• 3f2d4455 test(nodes): lock the region filter's result set to the JSON_EXTRACT one
• 8d258f89 test(ingestor): pin the values BackfillFromPubkey writes
• fd0b8a97 docs(nodes): give the measured reason COALESCE was rejected (comment only, SQL untouched)

The production SQL is unchanged from 9eac67f3: SELECT DISTINCT t.from_pubkey.

Findings, blocking first

1. POST /api/packets never writes from_pubkey (blocking in the first review). Resolved by master, so no change here. That endpoint no longer exists: /api/packets is registered GET only (cmd/server/routes.go:355), and cmd/server/ has no INSERT INTO transmissions outside tests, per the read-only invariant (bug(db): vacuumOnStartup fails with SQLITE_BUSY when ingestor + server share DB (auto_vacuum migration #919 broken in single-container topology) Kpa-clawbot/CoreScope#1283). [K] The only transmission writer is the ingestor's InsertTransmission (cmd/ingestor/db.go:872). It takes from_pubkey from BuildPacketData, which sets it for every ADVERT with a non-empty PubKey (cmd/ingestor/db.go:2665-2670). [K] Mutant M5 shows that a regression there is already caught.

2. Mid-backfill window: the filter loses nodes while legacy ADVERTs still have NULL from_pubkey (blocking in the first review). The window is real, and I measured it, but I disagree that it blocks this PR:

   • Before the backfill, on the 4.3 GB synthetic DB (15% of ADVERTs legacy NULL, correlated per node, so this is a worst case): AAR 609 → 509, CPH 944 → 776, BIL 132 → 99, all regions 2000 → 1700. [T]
   • The real BackfillFromPubkey then filled all 9,270 legacy rows in about 1 s, and afterwards every region is identical. [T]
   • The window only exists on a DB with un-backfilled legacy ADVERTs. Staging has 0 such rows that carry a pubKey (round 1 measurement). [A, not re-measured: no stg access this round]
   • The backfill runs at every ingestor start (cmd/ingestor/main.go:98) and only selects from_pubkey IS NULL, so a run that bare-returns on an error is retried at the next start. [K]
   • The real gap was that the backfill's output was untested. Mutant M6 (write "" for every row) left the whole ingestor suite green. New test TestBackfillFromPubkey_CopiesDecodedPubKey_PR38 now covers it, including across batch boundaries.

3. /api/healthz reports from_pubkey_backfill.done=true unconditionally. Not changed. This predates the PR, still holds on master (cmd/server/from_pubkey_migration.go:17), and fixing it means a cross-process progress signal from ingestor to server, which is outside "no other changes". I recommend a separate issue. Not a blocker for this PR: with finding 2's test in place, the window it would show is seconds long and only occurs at ingestor startup on a legacy DB. [K] [A]

4. The existing tests cannot tell the two expressions apart (trigger test_from_pubkey_advert). Fixed in cmd/server/nodes_region_from_pubkey_test.go. These tests drop the trigger and compare the real GetNodes with the pre-PR JSON_EXTRACT expression as an oracle:

   • TestGetNodesRegionFilter_FixtureMatchesJSONExtract_PR38: the committed e2e fixture (real staging data), each of MRY/OAK/SFO/SJC, all four combined, " mry , OAK", and ZZZ. It requires a non-vacuous result.
   • TestGetNodesRegionFilter_WritePathShapesMatchJSONExtract_PR38: one row of each shape the write paths produce (ingest-time pubkey, pubkey-less ADVERT left NULL, backfilled "" sentinel, non-ADVERT). It checks the oracle and an explicit expected set.

5. The comment credited the gain to the index. Already fixed in 9eac67f3. I re-verified it this round: EXPLAIN QUERY PLAN is identical for both expressions on the e2e fixture and on the 4.3 GB DB, and idx_transmissions_from_pubkey is not used (plan below). [T] The PR title still says "indexed". I kept it because port PRs carry the upstream title verbatim. It is the maintainer's call whether to rename it.

6. The perf case is weaker than stated. Re-measured below. The gain is real but modest: 1.10–1.21× in dense regions and 1.01× in the sparsest one, because the plan walks every ADVERT for every region and the JSON parse only runs for rows that pass the region filter. [T]

7. The old expression is a latent 500 (the stronger argument). Now locked by TestGetNodesRegionFilter_MalformedAdvertJSONDoesNotFailQuery_PR38. One ADVERT with decoded_json = 'NOT JSON' in the region makes the old SQL fail with SQL logic error: malformed JSON (1) (mutant M0). [T]

8. Round-1 claim: "COALESCE cost half the win (1.12× → 1.05×)". Not reproduced. On the 4.3 GB DB, COALESCE times within noise of from_pubkey (AAR 733 vs 746 ms, CPH 764 vs 752 ms), because only the NULL minority is parsed. The staging figure came from a box that was ingesting at the same time. COALESCE is still the wrong choice, but for a different reason: mutant M2 shows it brings back the malformed-JSON failure for every NULL row the backfill has not reached. The comment in db.go now gives that reason and names the guarding tests. [T]

Equivalence (task item 2)

DB	ADVERTs	from_pubkey NULL	NULL with pubKey in decoded_json	"" sentinel	from_pubkey ≠ decoded_json.pubKey	node sets old vs new
e2e fixture (committed)	102	0	0	0	0	identical, all 7 region cases [T]
e2e fixture, prepared as in CI (freshen, seeds, migrate), via HTTP /api/nodes?region= on a branch server and a master server side by side	—	—	—	—	—	identical for SJC, OAK, SFO, MRY, SJC+OAK, all 4, sjc, ZZZ (count and SHA-256 of the sorted keys) [T]
synthetic 4.3 GB after the real backfill	61,603	25	0	40	0	identical, 5 region cases × 11 rounds × 3 variants [T]
staging 4.9 GB (round 1, not re-run)	62,104	25	0	—	—	877 = 877 for AAR [A]

The synthetic DB has 1,600,065 transmissions, 6,400,111 observations, 2,300 nodes (300 never advertised) and 60 observers over 10 IATA codes, skewed from AAR (16 observers) to BIL (2). It was built from the migrated fixture schema with no ANALYZE, matching production (cmd/server/db.go:3130). Of its ADVERTs, 85% got from_pubkey at "ingest" time, and 15% were left as legacy NULL and filled by the real cmd/ingestor BackfillFromPubkey. The 25 pubkey-less live ADVERTs (NULL) and the 40 legacy pubkey-less ones ("" after backfill) mirror staging's shape. The generator SQL is reproducible on request.

Perf (task item 3, AGENTS.md rule 0)

Method:

• The real db.GetNodes(50, …, region) path, with the same driver (modernc) and OpenDB (mode=ro). Each variant is a separate test binary compiled from db.go: master (JSON_EXTRACT), branch (from_pubkey), and a COALESCE mutant.
• 11 rounds. The variant order rotates every round (old/new/coalesce → new/coalesce/old → coalesce/old/new). Each sample is the median of 3 calls after a warm call. The table shows the median over the 11 rounds.
• Synthetic 4.3 GB DB, 4-core / 5 GB RAM VM.

region	old JSON_EXTRACT	new from_pubkey	speed-up	COALESCE	rows (all variants)
AAR (dense)	827.7 ms	745.8 ms	1.11×	733.4 ms	609
CPH	858.3 ms	752.1 ms	1.14×	764.2 ms	944
ODE	781.9 ms	711.8 ms	1.10×	714.8 ms	665
BIL (sparse)	641.4 ms	636.8 ms	1.01×	629.9 ms	132
AAR,CPH	1019.2 ms	842.6 ms	1.21×	840.8 ms	1010

EXPLAIN QUERY PLAN is identical for both expressions on the 4.3 GB DB and on the fixture. Only the opcode address of the last line differs (40 vs 39):

SEARCH nodes USING COVERING INDEX sqlite_autoindex_nodes_1 (public_key=?)
LIST SUBQUERY 1
  SEARCH t USING INDEX idx_transmissions_payload_type (payload_type=?)
  SEARCH o USING INDEX idx_observations_tx_ts (transmission_id=?)
  SEARCH obs USING INTEGER PRIMARY KEY (rowid=?)
  USE TEMP B-TREE FOR DISTINCT

The ~630 ms floor that is the same for all variants (BIL) is the plan walking all ~61k ADVERTs and their observations for every region. That is the region-cache work in #176, which this PR does not touch.

Tests first, then mutants

The red evidence is the new tests run against master's db.go (M0). Only the malformed-JSON test is red there. The two identity tests are deliberately variant-agnostic and stay green against either expression, because the old and new expressions return the same set. That is the claim they lock.

#	mutant	caught by	result
M0	pre-PR SQL (origin/master db.go, JSON_EXTRACT)	…MalformedAdvertJSONDoesNotFailQuery_PR38	red: malformed JSON (1). Identity tests green, as intended [T]
M1	the query drops NULL rows outright (AND t.from_pubkey IS NOT NULL)	—	survives, equivalent mutant: NULL never matches IN (…), so dropping those rows changes nothing. This is the direct evidence that the 25 NULL rows carry no node [T]
M2	COALESCE fallback	…MalformedAdvertJSON…	red: malformed JSON (1) [T]
M3	data: a write path leaves from_pubkey NULL on a pubkey-bearing ADVERT (synthetic shapes)	…WritePathShapesMatchJSONExtract_PR38	red: missing [cc00000000000003] in SJC, SFO and SJC+SFO [T]
M4	data: 25% of the fixture's ADVERTs lose from_pubkey (forgotten write path or mid-backfill)	…FixtureMatchesJSONExtract_PR38	red in every region case: SJC 16 vs 20, OAK 23 vs 37, SFO 21 vs 25, MRY 11 vs 13, all 68 vs 90 [T]
M5	ingestor BuildPacketData stops setting FromPubkey	existing TestBuildPacketData_PopulatesFromPubkey	red [T]
M6	backfill writes "" for every legacy ADVERT	new TestBackfillFromPubkey_CopiesDecodedPubKey_PR38	red: legacy-adv-1/2: from_pubkey = "". Before this round the whole ingestor suite was green under this mutant [T]
M7	backfill abandons after the first batch	new backfill test	red: legacy-adv-nokey/corrupt still NULL [T]
M8	backfill also stamps non-ADVERTs	new backfill test	red: legacy-txt gets a pubkey [T]

Local runs (head fd0b8a97; the code under test was identical at 3f2d4455+)

• cmd/server: go vet clean. Full suite with -race -timeout 20m hit the 20 min timeout on this 4-core VM while in TestNodeAdvertRoutes_StreamMatchesWindowOracle, with 0 FAIL and 0 DATA RACE before that. Full suite without -race (-timeout 45m): ok (622.7 s). Targeted -race run (_PR38$|TestGetNodes|RegionFilter|TestHandleNodes|Nodes): ok. [T]
• cmd/ingestor: go vet clean. Full suite -timeout 20m: ok (863.6 s). [T]
• sh test-all.sh: 226/227. test-channels-client-state-152.js failed once on "N1: a remap mid-decrypt restarts loading … must show Decrypting…" while the Go race suite was loading the CPU. It passed 3/3 rerun on its own. No frontend file is touched by this PR. It is neither flaky: #180 packets URL E2E 'reload shows the same list' depends on fixture age (default 15-min window) #271 nor test(server): data race between startBackgroundIndexBuilds and PacketStore.Load under -race (TestHashMigrate_LogsMaxWriteLockHold_215) #301, so it may be a new load-sensitive flake. [T]
• node test-frontend-helpers.js: 709 passed, 0 failed. [T]
• E2E against a local Go server with e2e-fixture.db prepared as in CI (freshen, Packets page collapse button in the left column of the table opens the dialog. Kpa-clawbot/CoreScope#1486/"Group Data" message type missing from packet view window's "message type" filter Kpa-clawbot/CoreScope#1791 seed, corescope-migrate, the [feat] node detail, separate flood-adv from zero-hop adv Kpa-clawbot/CoreScope#2073/Active observers vanish as nodes after retention.nodeDays without an advert; observer link dead-ends on 'Node not found' #199/feat(nodes): estimated advert intervals (flood / zero-hop) per node, then mesh-wide settings analytics #245 seeds):
  • test-e2e-playwright.js: 132/135 passed, 3 skipped by the suite itself.
  • test-map-nodes-pagination-e2e.js: 3/3.
  • Branch vs master /api/nodes?region=: identical (table above). [T]
• No visual change, so there is no screenshot. The browser path was exercised by the E2E runs above.

CI per job (head fd0b8a97)

Run 37599294114, first attempt, no reruns needed: [T]

job	result
✅ Go Build & Test	pass (15m48s)
🎭 Playwright E2E Tests	pass (25m56s)
🏗️ Build & Publish Docker Image	pass (45s)
📦 Release Artifacts	skipped (version tags on upstream only)
🚀 Deploy Staging	skipped (does not run for PRs)
📝 Publish Badges & Summary	skipped (upstream push only)

Neither of the known flakes (#271, #301) appeared.

Notes

• The PR is currently not a draft (isDraft: false). The brief said it stays a draft. I did not change its state either way.
• Out of scope and left alone: the healthz signal (finding 3) and perf(nodes): port upstream region membership cache (#2102) #176.

🤖 Generated with Claude Code

[dborup-agent]

Review — CS-MacBook PR#38 — head fd0b8a9

Dom: APPROVE with nits

Independent, read-only review. Evidence labels: [T] tested/measured by me this round · [K] verified by reading code · [A] inferred or relied on other evidence, not run here.

Findings

#	Area	Severity	Finding
1	Correctness / equivalence	none	Node set is identical to JSON_EXTRACT on both the committed fixture and a purpose-built 110 MB DB; the only NULL from_pubkey rows carry no pubKey in decoded_json, so nothing is lost. [T]
2	Perf	nit	Gain is real but modest (~1.0–1.1×), and EXPLAIN QUERY PLAN is identical — idx_transmissions_from_pubkey does not participate. The win is the avoided per-row JSON parse, exactly as the (corrected) comment now says. [T]
3	PR title	nit	Title still says "indexed", which the author's own measurement contradicts. Kept verbatim as the upstream port title — maintainer's call. [T][K]
4	Test design	nit	The two identity tests are deliberately variant-agnostic (green against either expression). The only genuinely red-before/green-after test is the malformed-JSON one. This is documented honestly, and the dangerous data-shape regression is still caught (see mutants). [T][K]
5	/api/healthz	nit (pre-existing)	from_pubkey_backfill.done is hardcoded true; the warmup banner that would flag the mid-backfill window never shows. Predates this PR, out of scope; worth a separate issue. [K]
6	Commit identity	nit	Round-2 commits (3f2d4455, 8d258f89, fd0b8a97) are dborup <kontakt@meshview.dk> ✓. The round-1 doc commit 9eac67f3 is Dennis Jakobsen <dennisjakobsen@MiniMax.local>; the SQL cherry-pick keeps the upstream author (correct for -x). Not a blocker. [K]

No blocking findings. The one-line swap is correct and safe on the data as it exists, in scope, and well guarded by the new tests.

Answers to the review points

1. Identical result (on a large DB). [T]

• Committed fixture (test-fixtures/e2e-fixture.db): 102 ADVERTs, 0 NULL from_pubkey, 0 NULL-with-pubKey-in-JSON, 0 mismatches. Per-region node sets identical — MRY 13=13, OAK 37=37, SFO 25=25, SJC 20=20, only_old/only_new = 0 everywhere.
• Synthetic 110 MB DB (300k ADVERTs / 570k observations / 8000 nodes / 55 observers over 10 IATA regions, skewed dense→sparse, no ANALYZE): 0 NULL from_pubkey; per-region AAR/CPH/BLL/KRP/RNN all 800=800, only_old=only_new=0.
• So: the NULL rows that exist are exactly the ones with no pubKey to record; no node is lost by the swap.

2. Perf proof. [T] 11 interleaved rounds (order alternated each round), read-only, warmed, median of each series. C-sqlite 3.54:

• AAR 134.6 → 127.3 ms (1.06×); CPH 144.5 → 136.0 ms (1.06×); KRP 133.6 → 128.1 ms (1.04×).
• EXPLAIN QUERY PLAN byte-identical for both expressions (both drive off idx_transmissions_payload_type → idx_observations_transmission_id → observers rowid → TEMP B-TREE). Dropping idx_transmissions_from_pubkey yields the same plan and timings within noise → the index is not used. This supports the corrected comment and refutes the PR's original "2.5× / indexed" framing. Caveat: C-sqlite, not the server's modernc driver — a proxy for the JSON-parse cost, not an absolute number.

3. Test — does it pin the result set and kill the NULL-drop mutant? [T]

• The result-set claim is pinned against a hand-written JSON_EXTRACT oracle (regionNodesByJSONExtract), through the real db.GetNodes path, on real staging fixture data and on constructed write-path shapes.
• Red-before/green-after: only …MalformedAdvertJSONDoesNotFailQuery_PR38 is red on master (M0 below). The two identity tests pass against either expression by design — honestly documented in the test header and the author's report. That is a fair limitation, not a defect: the result is genuinely identical for well-formed data.
• The dangerous NULL-drop — a write path leaving from_pubkey NULL on a pubkey-bearing ADVERT — is caught: my mutant fix(frontend): replace legacy scope confirmation glyphs #2 (below) turns …WritePathShapesMatchJSONExtract_PR38 red with missing [aa00000000000001].

4. Scope. [K][T] Diff is exactly 3 files: cmd/server/db.go (one SQL line JSON_EXTRACT(...)→t.from_pubkey plus a comment), cmd/server/nodes_region_from_pubkey_test.go, cmd/ingestor/from_pubkey_test.go. cmd/server stays read-only — no INSERT INTO transmissions outside tests. No new map[string]interface{} in the diff. isV3() fork-guard count unchanged (19 = 19). No frontend/color change; scripts/check-xss-sinks.sh --diff origin/master is clean. No closing keywords in commits or PR body.

Tests (merged tree: git merge-tree of head onto origin/master 6444294)

Suite	Result
cmd/server full (go test .)	ok, 46 s [T]
cmd/server nodes/region -race	ok, 65 s [T]
cmd/ingestor full	ok, 108 s [T]
PR38 targeted (server + ingestor)	pass [T]
node test-frontend-helpers.js	709 / 0 [T]
sh test-all.sh	227 / 0 [T]
GitHub CI on head fd0b8a9	Go Build & Test pass, Playwright E2E pass (25m56s), Docker build pass [A]

Mutants

#	Mutant	Expected guard	Result
M0	Revert production SQL to JSON_EXTRACT (= master)	malformed-JSON test	RED malformed JSON (1); identity tests GREEN (variant-agnostic, as designed) [T]
mine#1	Data: NULL from_pubkey on 25% of pubkey-bearing ADVERTs (110 MB DB)	— (scale demo)	new query loses up to 400 of 800 nodes per region vs old; the loss is real and large, proving the swap's dependence on the write-path contract [T]
mine#2	Data shape: one pubkey-bearing ADVERT left NULL in WritePathShapes	…WritePathShapesMatchJSONExtract_PR38	RED missing [aa00000000000001] in SJC and SJC+SFO [T]
M6	Backfill writes "" for every row	…BackfillFromPubkey_CopiesDecodedPubKey_PR38	RED legacy-adv-1/2: from_pubkey = "" [T]

mine#1 and mine#2 together confirm the task's concern: a forgotten from_pubkey on a pubkey-bearing ADVERT silently drops nodes, and a committed test now catches that shape.

What I did not verify

• HTTP / Playwright E2E locally. I exercised equivalence through the real db.GetNodes code path (the FixtureMatches test on committed staging data) rather than over HTTP; the HTTP layer adds nothing to the region-filter logic. For the full E2E I relied on the green CI run on this exact head. [A]
• Real staging / production DB (no access). The "25 of 62104 ADVERTs NULL, all without pubKey" figure is the author's round-1 staging measurement, not re-run here. [A]
• modernc timing. My perf numbers are C-sqlite; a proxy for the JSON-parse cost, not the server's driver. [A]
• My synthetic DB is 110 MB with uniform per-region node counts — smaller than the author's 4.3 GB and with less region-size skew, so my speed-ups are a lower bound.