Skip to content

feat(regions): direct-evidence placement aggregate + operator dry-run (#488 M2) - #612

Merged
dborup merged 2 commits into
masterfrom
feat-488-m2-region-placement-dryrun
Oct 25, 2026
Merged

dborup merged 2 commits into
masterfrom
feat-488-m2-region-placement-dryrun

Conversation

@dborup-agent

Copy link
Copy Markdown
Collaborator

Relates to #488

Milestone M2 of the plan on #488 (§9), built on M1 (#609): a global direct-evidence region-placement aggregate in cmd/server behind the existing nodes-region cache machinery, plus a read-only dry-run report for operators. The default stays today's heard rule, so nothing changes for users. M3 (API field, ?regionBasis=), M4 (UI) and M5 (default flip) are not part of this PR.

Plan

  1. Red (7989ded0): tests plus compiling stubs (types and signatures only). Every new test fails except the default-mode guard, which pins "no behaviour change" and is green by design.
  2. Green (e6222c8d): the aggregate, the cache wiring, the config block, the dry-run flag, and the two M1 review nits.
  3. Mutation run: one or more mutants per requirement, each applied alone and then restored.
  4. Measure cold, warm and delta cost against the heard cache on a generated 1.07M-observation database (generator ported from nodes: investigate direct-evidence regional placement (#488) #601).

#601 is not merged. Its prototype aggregate, the RP488 cache tests and the generator are ported under new names (rpm2*, TestRegionPlacement488_*, TestPerf_RegionPlacementM2*_488), so the two PRs never collide in package main.

Changes

Where What
cmd/server/region_placement.go regionEvidenceAgg: one entry per (node, region), each a fixed 80-byte regionEvidenceStats (newest verified-direct hearing, newest hearing per hop count 1-7, oldest direct hearing, newest legacy '[]'). Every observation is classified by M1's packetpath.ClassifyAdvert (its own frame first, then path_json). There is no second classifier. placeNode applies the rule. regionPlacementSnapshot is an immutable region → sorted keys index
cmd/server/nodes_region_cache.go In placed mode, refreshNodeRegion cuts the per-set entry from the snapshot. The entry keeps the existing fresh TTL, stale-while-revalidate, max-stale wait, LRU cap and per-set singleflight. In heard mode the code path is unchanged
cmd/server/config.go, main.go, config.example.json regionPlacement.mode/maxHops/lifetimeDays/persistSpanHours, documented in config.example.json. Noted for the customizer in a later milestone (AGENTS rule 8). New flag -region-placement-dryrun
cmd/server/region_placement_dryrun.go The operator report, Markdown on stdout, counts only
cmd/server/advert_evidence.go, store.go N2 (review on #609): StoreObs.advertEv becomes a 2-byte advertEvidence{code, hops}, so advertHearing() returns the real hop count for relayed frames instead of 0. StoreObs stays 168 bytes (TestStoreObsLayout* green). N7: the caller comment at the computeNodeHomeRegions call site now describes the M1 rule

Rule (placed mode)

  1. direct: every region that heard the node's own transmission (hash count 0 on the observation's own frame, on FLOOD, TRANSPORT_FLOOD or DIRECT) is a home region. Ties form a set.
  2. fewest_hops: if there is no direct evidence anywhere, use the regions that heard the node in the fewest relays, up to maxHops. Ties form a set. Owner decision: a legacy row without a stored frame counts an explicit '[]' only as a 0-hop fallback, never as direct evidence.
  3. unknown: the node is in no region filter, but it is still listed unfiltered. Contact re-shares, unknown paths and hearings past 7 hops never place a node.

Evidence older than lifetimeDays before the node's own newest hearing is ignored. This keeps quiet nodes placed and lets a node that moved drop its old region. Owner decision: transient regions are deferred, so persistSpanHours defaults to 0 (off). The rule is implemented and tested, but it only runs when an operator sets the value.

Config (every default keeps today's behaviour)

Key Default Bounds
mode heard (today's rule; the aggregate is never built) heard or placed; anything else logs a warning and stays heard
maxHops 3 (working default, to be checked with the dry-run) 0-7
lifetimeDays 30 (matches retention.packetDays) 0 = no window; a negative value falls back to the default
persistSpanHours 0 = off (transient rule deferred) ≥ 0

All four values are startup-only and are meant for the customizer later.

Cache semantics kept

  • Watermark and delta. A refresh scans only observations.id > watermark, into a separate aggregate. The result is merged only on success, so a failed delta leaves the aggregate and the published snapshot untouched. The stats merge by min/max, so delta-then-merge equals a full build (deep-equal test).
  • Rebuild. A full rebuild runs every nodeRegionRebuildInterval (30 min). It picks up retention pruning, observer region edits and late from_pubkey backfills, which all sit below the watermark. Full builds share nodeRegionFullMu with the heard cache.
  • Non-monotone. Every refresh re-derives membership from the snapshot; the union-only extend is not used in placed mode. A new direct hearing removes the node from its fallback region.
  • Startup. The first requests for different region sets share one full scan: per-set singleflight, then coalescing on regionAggMu.
  • No full scan per request. A warm request is the same cached keys-JSON lookup as today.
  • Read-only. There are only SELECTs on the mode=ro handle and nothing is persisted. PacketStore.mu is never taken; this code does not touch the store.
  • Bounded. The aggregate grows with distinct (node, region) pairs, never with hearings or refreshes. The snapshot holds at most one key per pair, and the per-set cache keeps its 32-entry LRU cap.

Performance

Measured on an i7-1260P with 4 vCPU and go1.27.1. Medians of 7 runs, with the range over 3 runs. The generated DB has 1,066,840 observations (66,840 ADVERT), 2,340 nodes, 4 regions and 5,400 (node, region) pairs.

Operation heard (today) placed (this PR)
Cold, serving all 4 regions 745-751 ms (each set scans separately) 305-320 ms (one scan for all sets)
Warm hit 0.42-0.59 µs 0.37-0.50 µs
Delta, newest 5,000 obs (mostly non-advert) 2.58-2.67 ms (refresh) 1.96-2.04 ms (scan, merge, placement, entries)
Delta, 5,000 advert obs (worst case) 5.6-5.8 ms (scan only) 12.6-13.4 ms (scan, classify, merge, placement, entries)
Placement recompute only — 0.71-0.74 ms
Heap 285 KiB (4 entries) 1,372 KiB (aggregate, snapshot and entries; ≈ 260 B/pair)

On the e2e fixture: cold 1.1 ms vs 0.58-0.68 ms; delta 0.36 ms vs 0.16-0.28 ms.

  • Complexity. A refresh is O(new observations) for the scan plus O(nodes × regions heard) for re-deriving placements, which is under 1 ms at 2.3k nodes. Requests are O(1) cache hits.
  • Worst case. An advert-dense delta costs about 2× the heard scan. It runs at most once per 30 s, in the background, and only in placed mode.
  • Default mode. In heard mode the cost is exactly today's: the new code is never reached (TestRegionPlacement488_DefaultModeIsHeard).

To reproduce:

cd cmd/server
CORESCOPE_PERF_488_GEN=/tmp/big488.db CORESCOPE_PERF_488_SCALE=60 \
  go test -run '^TestPerf_RegionPlacementM2Generate_488$' -timeout 1h .
CORESCOPE_PERF_488_DB=/tmp/big488.db \
  go test -run '^TestPerf_RegionPlacementM2_488$' -v -timeout 1h .

Operator dry-run

The dry-run reads a copy of the database and never writes to it. It prints counts per evidence kind, basis and region, plus the unknown share for maxHops 0-5 and 7. It prints no public keys, names or observer ids.

  1. Make a consistent copy of the live database while the ingestor keeps running. SQLite's online backup is safe for this:
    mkdir -p /tmp/488 && sqlite3 /path/to/meshcore.db ".backup '/tmp/488/meshcore-copy.db'"
    Keep the copy in a writable directory. The server opens it with mode=ro, but SQLite may need to create -wal/-shm files next to a WAL-mode copy.
  2. Run the report with the same binary as the deployment, either from the image or from source:
    # from the image (overrides the image's entrypoint; serves nothing, exits when done)
    docker run --rm --entrypoint /app/corescope-server -v /tmp/488:/data <corescope-image> \
      -db /data/meshcore-copy.db -region-placement-dryrun > region-dryrun.md
    
    # or from a checkout of this branch
    cd cmd/server && go run . -db /tmp/488/meshcore-copy.db -region-placement-dryrun > region-dryrun.md
    To evaluate non-default values, put a config.json holding only a regionPlacement block into a directory and add -config-dir <dir>. The sweep table already compares maxHops 0-5 and 7.
  3. Review region-dryrun.md: it contains counts only. It can then be pasted into nodes: investigate direct-evidence regional placement without regressing historical cache #488 to choose the maxHops default before M5. Delete the copy afterwards.

On the CI-prepared e2e fixture, the report shows no direct evidence: the fixture has no per-observation frames, so every zero-hop hearing is a legacy '[]' fallback. At maxHops 3, 73 of 97 nodes are unknown; at maxHops 5 the count is 59, and at maxHops 7 it is 28. Those figures match the #488 analysis. The fixture is a worst case, with about one hearing per advert, so the real numbers have to come from a production copy.

Tests

Test Covers
TestRegionPlacement488_PlaceRule (22 cases) Relayed-only leakage, multi-region direct, transient on and off, no persistent region, fewest-hop ties, cap boundary, cap 0, hops past the slots, shares and unknown, undated, quiet node, moved node, lifetime 0, stale fallback, direct beats fallback, legacy '[]' fallback-only (4 cases)
…_StatsAreBounded, …_BoundedMemory Fixed 80-byte stats; the aggregate grows with pairs, not hearings or refreshes; snapshot ≤ pairs; LRU cap in placed mode
…_DeltaEqualsFullBuild Delta plus merge deep-equals a full build at 4 split points
…_DefaultModeIsHeard No config: heard membership, and the aggregate is never built
…_PlacedModeDropsRelayedOnly, …_NewObservationCanRemoveRegion The leak is closed; a refresh is non-monotone and runs as a delta, not a rebuild
…_RebuildNeededForEditsPruneBackfill Observer edit, prune and late backfill: invisible to a delta, picked up by the rebuild
…_QuietNodePlacedFromSQLHistory A node heard 120 d ago is placed from SQLite alone
…_StartupSingleflight 6 concurrent first requests over 4 sets → exactly 1 full scan; a warm request does not scan
…_FailedDeltaLeavesAggregateIntact A scan failing after it folded its rows changes nothing; recovery equals a full build
…_V2Schema Legacy schema (observer_id, ISO timestamps)
…_SyntheticAccuracy 39-node ground-truth scenario from #601: placed is exact for all 39, heard for 7; placed ⊆ heard
…_DryRunReport, …_DryRunEmptyDB, …_FixtureDryRun Pinned counts on the scenario; no key-shaped hex in the output; DB file hash unchanged; heard per region equals today's filter
…_DryRunFlag (process) The real main() with the flag prints the report for the configured settings and exits 0
…_ServerUsesConfiguredMode (process) The real server with mode: placed serves /api/nodes?region=OAK total 5 (heard: 37)
…_Config, …_ConfigExample Defaults, explicit values, bounds; config.example.json documents the block with values equal to the defaults
TestAdvertHearingKeepsRelayHops_488, TestAdvertEvidence_SetOnEveryObservationPath_488 (extended) N2: hop counts 1, 2, 3 and 63 survive the stored class; relayed hops 1 and 3 on all five load and ingest paths

Constraints

  • No SQL under PacketStore.mu. cmd/server stays read-only: only SELECTs, and the mode=ro handle.
  • No new map[string]interface{} in production files. The counts per touched file are unchanged (store.go 197, config.go 26, db.go 67) and the new files have 0.
  • No frontend or workflow changes. scripts/check-xss-sinks.sh --diff origin/master has nothing to scan. No colours. Fork guards are unchanged: 9 in deploy.yml, 1 in release-fast-path.yml.
  • node scripts/run-go-server-shard.js --validate passes. The new tests run in catch-all shard 3.

🤖 Generated with Claude Code

dborup and others added 2 commits October 25, 2026 18:50
Red commit. Tests for the direct-evidence region placement aggregate
behind the nodes-region cache, the operator dry-run report, the
regionPlacement config block and the relayed hop count that M1's
advertHearing dropped (review N2 on #609). Production files carry the
types and signatures only, so every new test except the default-mode
guard fails.

The placement rule, aggregate and cache tests are ported from the
research PR #601 under new names (rpm2*, TestRegionPlacement488_*), so
the two never collide in package main.

Relates to #488

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…#488 M2)

Green commit for #488 milestone M2.

- region_placement.go: one global (node, region) evidence aggregate,
  classified per observation by M1's packetpath.ClassifyAdvert, behind
  the nodes-region cache machinery (watermark, delta scans merged only on
  success, full rebuild every 30 min, per-set singleflight, fresh/stale
  ages). Rule: verified direct evidence first, then the fewest-hops
  fallback capped at maxHops (a legacy '[]' without a frame is a 0-hop
  fallback only), else unknown. Membership is re-derived on every
  refresh, never extended.
- regionPlacement config (mode/maxHops/lifetimeDays/persistSpanHours);
  every default keeps today's behaviour: mode "heard" never builds the
  aggregate. Noted for the customizer later.
- corescope-server -region-placement-dryrun: read-only report (counts
  per evidence kind, basis and region, unknown share for several maxHops
  values), no node keys.
- Review N2 on #609: StoreObs.advertEv keeps a relayed frame's hop count,
  so advertHearing no longer returns Hops 0 for relayed hearings.
- Review N7 on #609: the computeNodeHomeRegions caller comment describes
  the M1 rule.

Relates to #488

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@dborup-agent

Copy link
Copy Markdown
Collaborator Author

Rapport — CS-pve-agent1 PR#612 #488 — head e6222c8

Status: #488 M2 is implemented on a draft PR. Every requirement has a test that fails on the red commit and passes on green, and all 27 mutants are killed. CI is green on every job without a rerun. The PR is not merged and not marked ready.

Evidence tags: [T] = a test or command run in this session; [K] = read from code or firmware source; [A] = analysis or inference.

Commits

Commit Role Result
7989ded0 Red: tests plus compiling stubs (types and signatures only) Each new test was run alone against the red production code, using the final test files. 21 fail. The default-mode guard passes by design, the dry-run process helper and the env-gated perf tests skip, and ConfigExample fails on 7989ded0 itself [T]
e6222c8d Green: aggregate, cache wiring, config, dry-run flag, N2, N7 All pass [T]

Requirements

Requirement Test(s) Mutant(s), all killed Evidence
Global aggregate built on M1's classifier (no duplicate) DeltaEqualsFullBuild (deep-equal stats), SyntheticAccuracy A1: merge overwrites instead of max. A2: path_json preferred over the frame [T]; the scan calls packetpath.ClassifyAdvert [K]
Rule: direct first, capped fewest-hops fallback, else unknown; ties form sets PlaceRule (22 cases) P2: ties cut to one region. P3: cap off by one. P4: lifetime anchored at now. P6: fallback ignores direct evidence elsewhere [T]
Owner decision: legacy '[]' is fallback only PlaceRule (4 legacy cases), StatsAreBounded P1: legacy treated as verified [T]
Owner decision: transient regions deferred (persistSpanHours 0 = off) PlaceRule (transient on and off), Config P5: transient rule always on [T]
Behind the cache machinery: watermark and delta NewObservationCanRemoveRegion (1 full + 1 delta), FailedDeltaLeavesAggregateIntact C4: delta scanned into the live aggregate. C5: delta from id 0 [T]
Non-monotone refresh NewObservationCanRemoveRegion C1: union with the previous keys [T]
Rebuild: edits, prune, backfill RebuildNeededForEditsPruneBackfill (3 cases) C2: never rebuild [T]
Quiet node from SQL history QuietNodePlacedFromSQLHistory P4 [T]
Startup: one shared full scan StartupSingleflight (6 requests, 4 sets → 1 full scan; warm → 0) C3: refresh without regionAggMu [T]
Behind the heard default (no behaviour change) DefaultModeIsHeard (guard), Config D1: default placed. D2: placed branch on any mode [T]; /api/nodes?region= on the CI-prepared fixture is identical to an origin/master server for 7 region sets (total + sha256 of the keys) [T]
Config regionPlacement.mode/maxHops/lifetimeDays/persistSpanHours Config (7 cases), ConfigExample, ServerUsesConfiguredMode (process: real server, OAK 37 → 5) F1: maxHops unbounded. F2: lifetimeDays 0 not honoured. F3: config not wired into the server [T]; noted for the customizer in config.example.json and the PR body [K]
Dry-run: counts per basis and region, unknown share at several maxHops DryRunReport (pinned scenario counts), FixtureDryRun, DryRunEmptyDB R1: heard derived from placements. R2: sweep ignores maxHops [T]
Dry-run: no pubkeys DryRunReport (no seeded key, no ≥12-char hex in the output) R3: a node key written into the report [T]
Dry-run: read-only, exits DryRunReport (DB sha256 unchanged), DryRunFlag (process) R4: flag ignores the config. R5: no exit after the report [T]; the built binary on a CI-prepared copy: exit 0, DB sha256 unchanged [T]
N2: advertHearing() keeps the hop count AdvertHearingKeepsRelayHops_488 (1, 2, 3, 63 hops), AdvertEvidence_SetOnEveryObservationPath_488 (5 paths) N1: hops not stored. N2: hops not returned [T]; StoreObs stays 168 bytes (TestStoreObsLayout*) [T]
N7: stale caller comment — (comment only; no test possible) — [K] store.go at the computeNodeHomeRegions call
Bounded memory BoundedMemory (20 refreshes: pairs stay 15; snapshot ≤ pairs; LRU ≤ 32), StatsAreBounded (80 B fixed) B1: a new pair on every hearing [T]
Legacy v2 schema V2Schema V1: no ISO-timestamp conversion [T]
Cold/warm/delta benchmark with the generator TestPerf_RegionPlacementM2_488 (env-gated), FixtureCost — [T] see below

Mutants were applied one at a time with the file restored afterwards; the final run against the green tree killed 27/27 [T]. Two earlier survivors led to test changes. A1 led to the deep-equal stats check. C3 first targeted a redundant inner fresh-check, which was equivalent (a queued refresh finds no new rows and does not scan), so that check and a redundant inner singleflight were removed; the mutant now removes the coalescing mutex [T].

Benchmark (i7-1260P, 4 vCPU, go1.27.1; medians of 7, ranges over 3 runs) [T]

The generated DB has 1,066,840 observations, 2,340 nodes, 4 regions and 5,400 pairs.

Operation heard placed
Cold, all 4 regions 745-751 ms 305-320 ms
Warm hit 0.42-0.59 µs 0.37-0.50 µs
Delta, newest 5,000 obs 2.58-2.67 ms 1.96-2.04 ms
Delta, 5,000 advert obs 5.6-5.8 ms (scan only) 12.6-13.4 ms (end to end)
Placement recompute — 0.71-0.74 ms
Heap 285 KiB 1,372 KiB (≈ 260 B/pair)

Invariants

Check Result
No SQL under PacketStore.mu The new code never takes the store lock and does not touch the store [K]
cmd/server read-only SELECTs only, on the mode=ro handle; readonly_invariant_test.go green [T][K]
map[string]interface{} outside tests Unchanged per touched file (store.go 197, config.go 26, db.go 67); 0 in the new files [T]
Hardcoded colours / XSS No frontend change; scripts/check-xss-sinks.sh --diff origin/master exit 0, nothing to scan [T]
Fork guards grep -c: 9 in deploy.yml, 1 in release-fast-path.yml; .github unchanged [T]
Go shard manifest node scripts/run-go-server-shard.js --validate OK (2 / 9 / 2968) [T]

Local runs

Suite Result
cd cmd/server && go vet ./... && go test ./... ok, 72 s [T]
-race on the region, cache, home-region, layout and #597 lock tests ok [T]
cd cmd/ingestor && go test ./... ok, 225 s [T]
internal/* 12 ok, 3 without tests [T]
sh test-all.sh 273 passed, 0 failed [T]
node test-frontend-helpers.js 731 passed, 0 failed [T]
E2E against a local Go server on a CI-prepared e2e-fixture.db copy (freshen, inline seeds, corescope-migrate, 2073/199/245/122 seeds); stopped by pid test-e2e-playwright.js 156/159 (3 skipped); 531: 31/31; 416: 17/17; 560: 6/6; 245: 7/7; channels 154-155: 8/8; map-nodes pagination: 3/3; 521 nodes URL race: 5/5 [T]

A full go test -race ./... of cmd/server hit Go's default 10-minute timeout in an unrelated test (TestNodeAdvertRoutes_FloodCountMatchesCountFloodAdvertsForNode, 28 s into its run) while the perf DB generator was running on the same machine. CI shards that suite; the targeted race run above is clean [T].

CI (head e6222c8d)

Run 40227012172 (head e6222c8d, attempt 1):

Job Result
Go Build & Test pass
Go Server Tests (1/3), (2/3), (3/3) pass, pass, pass
Go Ingestor Tests pass
Go Libraries & CLI pass
JavaScript Unit Tests & Lint pass
Script & Container Unit Checks pass
Verify Go Test Assignments pass
Playwright E2E Tests (1/3), (2/3), (3/3) pass, pass, pass
Verify E2E Shards & Coverage pass
Validate Docker Image / Build & Publish Docker Image pass / pass
Publish Badges & Summary, Release Artifacts, Deploy Staging skipping (fork guards)

No reruns were needed; the known flaky test (#271) did not fail [T].

Operator: run the dry-run on a DB copy

# 1. consistent copy while the ingestor runs (online backup), in a writable dir
mkdir -p /tmp/488 && sqlite3 /path/to/meshcore.db ".backup '/tmp/488/meshcore-copy.db'"

# 2a. with the deployed image (serves nothing, exits when done)
docker run --rm --entrypoint /app/corescope-server -v /tmp/488:/data <corescope-image> \
  -db /data/meshcore-copy.db -region-placement-dryrun > region-dryrun.md
# 2b. or from a checkout of this branch
cd cmd/server && go run . -db /tmp/488/meshcore-copy.db -region-placement-dryrun > region-dryrun.md

# optional: evaluate other values with a config.json that holds only
#   {"regionPlacement": {"maxHops": 5, "lifetimeDays": 30}}
# and add: -config-dir <dir containing it>

# 3. review region-dryrun.md (counts only), paste it into #488, then delete /tmp/488

The copy must stay in a writable directory: the server opens it mode=ro, but SQLite may create -wal/-shm files next to a WAL-mode copy. The report contains counts only: no public keys, names or observer ids.

Remaining

  1. The default maxHops is unvalidated [A]. The CI-prepared fixture has no per-observation frames, so it shows 0 direct, and 73 of 97 nodes unknown at maxHops 3 [T]. A real instance's dry-run is needed before M5.
  2. persistSpanHours is implemented and tested but off by default, as the owner deferred transient regions. Transient regions are not exposed anywhere [K].
  3. Lifetime overflow. A lifetimeDays value above about 106,000 overflows time.Duration, and the window then behaves like "no window". The effect is harmless, but no explicit cap is enforced [A].
  4. Redundant fast path. The lock-free fresh check in currentRegionPlacement is a fast path only. Removing it is an equivalent mutant (a queued refresh finds no new rows and only re-stamps the snapshot), so no test pins it [T][A].
  5. Out of scope. M3 (region_placement API field, ?regionBasis=), M4 (UI, customizer entries) and M5 (default flip) are not part of this PR [K].
  6. Single machine. All timings come from one machine. The advert-dense delta costs about 2× the heard scan; it runs only in placed mode, at most once per 30 s, in the background [T][A].

@adminopenclaw8-sketch

Copy link
Copy Markdown
Collaborator

Review — CS-Macmini PR#612 — head e6222c8

Dom: APPROVE med nits

Independent, read-only review on the merged tree (git merge-tree --write-tree origin/master e6222c8d = cbe940a3) against origin/master 4c6a12cc. git ls-remote gave e6222c8d before and after. Evidence tags: [T] run in this session, [A] analysis, [K] read from code.

No correctness bug found. The default mode is provably byte-identical to master, the aggregate reuses M1's classifier with no second copy, the watermark/delta/rebuild/singleflight semantics hold, the dry-run is read-only and names no node, and the perf claims reproduce. One real test gap, the rest are documentation and config-polish nits.

Findings

# Sev Where Finding
F1 nit (test gap) cmd/server/region_placement.go:310 The ts != 0 half of within() is unpinned. Dropping it (return ts >= ws) survives the entire cmd/server suite [T], yet it is a large behaviour change under the documented lifetimeDays: 0: ws becomes math.MinInt64, so LastAt[0] == 0 ("never heard directly here") satisfies within() and every heard region is reported direct. Probe: placeNode on AAA@1-hop + BBB@3-hop with Lifetime = 0 returns {fewest_hops [AAA] 1} on head and {direct [AAA BBB] 0} with the mutant [T]. Cause of the gap: noWindow appears in exactly one PlaceRule case, and there both regions have direct evidence, so the mutant agrees. One extra PlaceRule row with noWindow + relayed-only evidence closes it.
F2 nit cmd/server/advert_evidence.go:11-13 The N2 comment says the hop count is stored because "the fewest-hops fallback needs it", but no production code reads it. advertHearing()'s only production caller is computeNodeHomeRegions (store.go:4713), which branches on Kind/Verified only; the M2 aggregate classifies straight from SQL via packetpath.ClassifyAdvert and never goes through StoreObs [K]. The field is correct and tested, but it is forward-looking for M3/M4 — worth saying so, so the next reader does not look for the consumer.
F3 nit PR body, "Operator dry-run" "the fixture has no per-observation frames, so every zero-hop hearing is a legacy '[]' fallback" is not what the fixture does. On a CI-prepared fixture the report itself prints per-observation frames (observations.raw_hex): yes and classifies 81 of 103 advert observations from frames (51 relayed 1-7, 30 relayed 8+); only 22 rows are frameless legacy [T]. The accurate statement is "no frame carries direct evidence". Same paragraph: "73 of 97 nodes are unknown" — the denominator is 90 (73 unknown + 17 placed), and the 73/59/28 triple at maxHops 3/5/7 matches my run exactly [T].
F4 nit cmd/server/config.go:1763-1783 maxHops is clamped both ways and logs, negative lifetimeDays is rejected and logs — but lifetimeDays and persistSpanHours have no upper bound, so above ~106,751 days (resp. ~2.56e15 hours) time.Duration(...) overflows and the window silently changes meaning [A]. The author discloses this (Remaining 3); a min() clamp would make the block uniformly bounded with no new behaviour.
F5 nit cmd/server/config.go:1781 A negative persistSpanHours is dropped silently, while negative maxHops/lifetimeDays both log a warning. Inconsistent operator feedback for the same class of mistake.
F6 nit cmd/server/region_placement_dryrun.go:21 regionDryRunSweep = {0,1,2,3,4,5,7} skips 6 — the one cap value an operator cannot read off the table they are meant to choose the default from.

Nothing above blocks the draft; none changes default-mode behaviour.

The review points

M2 scope, default mode unchanged. Verified end to end, not just by unit test: master (4c6a12cc) and head servers were run in turn on one CI-prepared e2e-fixture.db, and /api/nodes?region= is identical for 7 region sets — MRY 13, OAK 37, SFO 25, SJC 20, OAK,SFO 61, MRY,SJC 32, all four 90 — with the same sha256 over the sorted public keys in every case [T]. With {"regionPlacement":{"mode":"placed"}} the same binary serves 2/5/6/4 and 17 for the union, and logs one full build, 90 nodes, 95 (node, region) pairs in 1ms [T]. TestRegionPlacement488_DefaultModeIsHeard additionally pins that the aggregate is never built (0 full, 0 delta scans) in the default mode [T].

Aggregate reuses M1's classifier. scanRegionEvidence calls packetpath.ClassifyAdvert(raw, path) once per row (region_placement.go:281) [K]. There is one classifier in the tree (internal/packetpath/advert_evidence.go); nothing in cmd/server re-derives hop counts or route semantics. Mutant A2's premise (frame vs path_json order) lives entirely in that package. Confirmed by MINE-3 below.

Watermark / delta / rebuild / failed-delta. The delta scans into a separate aggregate and agg.merge(delta) runs only after the scan returns nil; scanRegionEvidence raises its own watermark after rows.Err(), so a scan that fails part-way leaves watermark and pairs untouched [K]. FailedDeltaLeavesAggregateIntact pins watermark 1 / pairs 1 after an injected failure and then pins that recovery deep-equals a full build [T]. DeltaEqualsFullBuild deep-equals per-pair stats, not just placements, at 4 split points [T]. RebuildNeededForEditsPruneBackfill covers the three sub-watermark mutations the issue names (observer edit, prune, late backfill) [T].

Singleflight and no full scan per request. StartupSingleflight blocks inside the scan hook, fires 6 concurrent requests over 4 distinct sets, and pins exactly 1 full and 0 delta scans plus correct membership for each set; a later new set on a fresh snapshot scans 0 times [T]. Lock audit: nodeRegionFullMu is taken in exactly two places (buildNodeRegion, the placement full build) and regionAggMu in one; the only ordering is regionAggMu → nodeRegionFullMu, and buildNodeRegion never takes regionAggMu, so there is no inverse order to deadlock against [K].

Bounded memory. regionEvidenceStats is a fixed 80 bytes (pinned by unsafe.Sizeof), one per (node, region); BoundedMemory runs 20 refreshes over 300 hearings and pins pairs 15 / nodes 5, snapshot keys ≤ pairs, and the 32-entry LRU cap in placed mode [T]. I measured 455 KiB for 1800 pairs on a generated 322k-observation DB = 259 B/pair, reproducing the body's "≈ 260 B/pair" [T].

No SQL under PacketStore.mu; cmd/server read-only. The new files contain no INSERT/UPDATE/DELETE/CREATE/ALTER/DROP [T], never touch PacketStore [K], and run on OpenDB's file:...?mode=ro handle with no journal_mode pragma (db.go:236) [K]. readonly_invariant_test.go is green as part of the suite [T]. The one /api/nodes?region= call site (db.go:1274, inside GetNodes) is a *DB method and holds no store lock [K].

Dry-run read-only and private. Ran the built binary on a copy of a CI-prepared fixture: exit 0, DB sha256 byte-identical before and after, no side files, and the report contains no public key of the 200 in the fixture and no hex run of ≥12 chars [T]. main() returns before OpenDB for the server, the schema wait, the packet store and the listener, so nothing in the dry-run path can write [K].

Config defaults off and documented. heard / 3 / 30 / 0, with Config's 7 cases covering absent file, absent block, empty block, explicit, over-cap, negatives and an unknown mode; ConfigExample parses config.example.json through the real struct and asserts its values equal the defaults [T]. config.example.json carries the _comment with the dry-run instruction, the bounds and the "meant for the customizer" note.

N2 and N7. N2: advertEv is now a 2-byte struct, advertHearing() returns the real hop count, StoreObs stays 168 bytes (TestStoreObsLayout* green), and AdvertEvidence_SetOnEveryObservationPath_488 now pins relayed hops 1 and 3 across all five load/ingest paths [T]. See F2 on the missing consumer. N7: the computeNodeHomeRegions call-site comment now describes the M1 rule (TRANSPORT_DIRECT {0,0} never counts, legacy '[]' is fallback only) [K].

No new map[string]interface{}. Per-file counts identical to origin/master (store.go 197, config.go 26, db.go 67); the 2 new production files have 0 [T].

Colours / XSS / fork guards / authorship. The diff touches no public/, .js, .html or .css file, so check-xss-sinks.sh --diff origin/master has nothing in scope [T]; .github/ is unchanged and the fork guards still count 9 in deploy.yml and 1 in release-fast-path.yml [T]. Both commits are authored and committed by dborup <kontakt@meshview.dk> [T]. The body says "Relates to #488" — no closing keyword [T].

Operator instructions — checked, and the warning is load-bearing. I reproduced the recipe on a WAL database with a writer attached: .backup succeeds and yields a WAL-mode copy; the dry-run on that copy in a writable directory produces the report and creates copy.db-shm / copy.db-wal; in a read-only directory it fails fast with [region-placement] dry-run: max observation id: attempt to write a readonly database (1544) and a non-zero exit [T]. So the "keep the copy in a writable directory" note is necessary, accurate, and the failure mode is a clear message rather than a wrong report. I also tested the obvious hardening — sqlite3 -readonly <live> ".backup ..." and sqlite3 -readonly <live> "VACUUM INTO ..." — and both fail on a WAL source ("unable to open database file"), because a read-only connection cannot create the -shm. The documented read-write .backup is the right call; no change wanted [T].

Acceptance criteria, red before / green after. I rebuilt the red tree (7989ded0 production code) and dropped the final head test files onto it: 21 of the 22 new tests fail, and the only green one is DefaultModeIsHeard, the "no behaviour change" guard that is green by design [T]. This matches the author's claim exactly. The test-file changes between red and green are additive (a deep-equal stats assertion, the ServerUsesConfiguredMode process test, an advert-dense perf window); the one non-additive edit fixes a test that passed a non-normalised lowercase "aaa" code — normalizeRegionCodes upper-cases every code before it reaches the cache, and both the heard SQL IN and members() are exact matches on the normalised value, so this is a test fix, not a weakening [K]. Issue AC 1's "NULL versus empty paths / every supported route type" is carried by M1 (#609) and exercised here through PlaceRule and the frame fixtures; AC 2 is QuietNodePlacedFromSQLHistory + RebuildNeededForEditsPruneBackfill; AC 3 is the transient-on/off PlaceRule cases plus the report's "inferred from reception, not a verified location" line; AC 4 is the above plus the measurements; AC 5 holds — no staging or prod was touched.

Perf plausibility. Reproduced on my own generated DB (scale 20, 322,280 observations, 780 nodes, 1800 pairs), medians of 7 [T]:

Operation heard placed
Cold, all 4 regions 190.7 ms 66.3 ms
Warm hit 250 ns 250 ns
Delta, newest 5,000 obs 2.227 ms 784 µs
Delta, 5,000 advert obs 3.952 ms 5.204 ms
Placement recompute only — 147 µs
Heap 101 KiB 455 KiB (259 B/pair)

Same shape as the body at a third of the scale: placed is ~2.9× faster cold, equal warm, faster on an ordinary delta, and slower only on the advert-dense delta (1.3× here vs the body's 2.3× at 4× the advert density — consistent with a cost that is linear in classified rows [A]). On the e2e fixture, cold 1.016 ms heard vs 382 µs placed [T]. The claim that the regression is bounded to one background scan per 30 s in placed mode only follows from currentRegionPlacement's fresh-TTL short-circuit [K].

Tests and mutants

All on the merged tree, go1.27.0, macOS. CI run 40227012172 on e6222c8d was green on every job at attempt 1 — I re-checked each job myself; neither known flake (#256 Hash Stats sort, #267 backfill write-hold) appeared [T].

Suite Result
cd cmd/server && go vet ./... && go test ./... vet clean; ok 74.9 s, 0 failures [T]
cd cmd/ingestor && go test ./... ok 109.3 s [T]
internal/* (12 modules with tests) all ok [T]
sh test-all.sh 273 passed, 0 failed [T]
node test-frontend-helpers.js 731 passed, 0 failed [T]
node scripts/run-go-server-shard.js --validate ok (2 / 9 / 2979 runnable entries, each in exactly one shard) [T]
E2E vs a local Go server on a CI-prepared fixture (freshen → the workflow's inline seed SQL → corescope-migrate), port 13805, stopped by pid test-issue-531-region-url 31/31; test-issue-416-region-quick-picks 17/17; test-issue-560-role-breakdown 6/6; test-e2e-playwright 156/159 (3 skipped) [T]
Mutant Applied to Result
A1 (author) — merge overwrites instead of max regionEvidenceStats.merge killed — DeltaEqualsFullBuild [T]
C4 (author) — delta scanned into the live aggregate refreshRegionPlacement killed — FailedDeltaLeavesAggregateIntact [T]
R3 (author) — a real node key carried into the report region_placement_dryrun.go killed — DryRunReport [T]
MINE-1 — within() drops the ts != 0 guard placeNode SURVIVED the whole cmd/server suite → F1 [T]
MINE-2 — minNonZero returns the larger value region_placement.go killed — PlaceRule, SyntheticAccuracy, DryRunReport [T]
MINE-3 — ignore observations.raw_hex, always classify by path_json scanRegionEvidence killed — BoundedMemory, SyntheticAccuracy, DryRunReport [T]
MINE-4 — members() uses only the first region code regionPlacementSnapshot.members killed — StartupSingleflight [T]

Each mutant was applied alone to a throwaway copy and the file restored and diffed against the clean tree afterwards [T].

Not verified

  • The maxHops default. The fixture has zero direct evidence, so 73 of 90 nodes are unknown at maxHops 3 [T]. The default stays unexercised until a dry-run on a real instance, as the author says. Nothing here argues 3 is right or wrong.
  • The exact published timings. My numbers come from a 322k-observation DB on one Apple-silicon machine, not the author's 1.07M-observation DB on an i7-1260P. I reproduced the shape, the direction of every comparison and the per-pair heap figure, not the absolute milliseconds.
  • Long-run behaviour. The 30-minute rebuild interval, the 30-minute max-stale wait and the LRU under real traffic were only exercised by the tests' time manipulation, never by a clock.
  • persistSpanHours in production. Implemented and unit-tested, off by default, and nothing exposes transient regions anywhere — reviewed as code, not as behaviour.
  • M3/M4/M5. The API field, ?regionBasis=, the UI, the customizer entries and the default flip are out of scope here and were not assessed.
  • A -race run of the full cmd/server suite. I ran the suite without -race; CI's sharded jobs cover it.
  • Fixture-prep detail. I prepared the fixture exactly as the current deploy.yml does (freshen, the one inline seed step covering Packets page collapse button in the left column of the table opens the dialog. Kpa-clawbot/CoreScope#1486 and "Group Data" message type missing from packet view window's "message type" filter Kpa-clawbot/CoreScope#1791, corescope-migrate). The review brief also mentioned seeds "2073" and "199"; there is no such step in the workflow today, so none was applied.

@dborup
dborup marked this pull request as ready for review October 25, 2026 20:49
@dborup
dborup merged commit 1270fb3 into master Oct 25, 2026
18 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.

3 participants