Skip to content

feat(ingestor): resolve the last hop from the observer (#188) - #190

Merged
dborup merged 18 commits into
masterfrom
codex/issue-188-observer-anchor
Oct 4, 2026
Merged

dborup merged 18 commits into
masterfrom
codex/issue-188-observer-anchor

Conversation

@adminopenclaw8-sketch

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

Copy link
Copy Markdown
Collaborator

Relates to #188, #184, #182

This is a deliberate scoring change. On staging, every non-advert observation with 1-byte hop hashes is stored with resolved_path = NULL, so about half of the relayed traffic gives no relay credit wherever the server indexes from that column. This PR resolves most of those rows. Traffic share and usefulness change for most repeaters. The expected sum of traffic_share_score is about 1.47× today's persisted level (see the replay below).

Dependency on #182: now met. #182 is merged on master (9d29daec), and master is merged into this branch (486aa9ea).

What changes (cmd/ingestor only)

1. Observer anchor (path_resolver.go, db.go)

resolveObservationPath replaces the direct resolvePathWithContext call in InsertTransmission.

  • Forward chain: unchanged. It anchors hop 0 on fromPubkey (ADVERT only) and each next hop on the previous resolved hop.
  • Backward chain: runs only for flood route types (0, 1) and only if the forward chain left a hop nil. The last hop is the unique relay that neighbours the observer. Each earlier hop is the unique neighbour of the hop after it. A hop that does not resolve breaks the chain.
  • Shared rule: both chains use resolveHopWithContext, so a hop resolves only when exactly one candidate qualifies. There is no geo, GPS or count tie-break (fix(nodes): rebuild relay-hop history on startup from path_json Kpa-clawbot/CoreScope#1643).
  • The observer is never its own last hop in a flood path, because a radio does not receive its own transmission. Both chains leave that hop nil rather than name the observer, even when its prefix is unique. The observer can still be an earlier hop: it logs packets before de-duplication, so it can report the echo of a flood it forwarded.
  • Merge rule:
    • A hop keeps the forward value if there is one; otherwise it takes the backward value.
    • If the chains resolve the same hop to different nodes, or the merge would place one node at two positions, the backward result is dropped for the whole row and the forward result is stored unchanged. So the two chains never contradict each other in a row, and a forward-resolved hop (for example, from an ADVERT) is never changed.
  • Observer identity: the observer is the <observer_id> segment of the MQTT topic meshcore/<iata>/<observer_id>/packets, which is the observer node's pubkey.
    • The neighbour builder writes observer↔last-hop edges keyed by that id, lower-cased (neighbor_builder.go).
    • NeighborGraph lower-cases on lookup.
    • An id that is not a node (for example companion) has no edges and anchors nothing.

Firmware basis (MeshCore src/; line numbers verified at a366955):

  • A hop is a prefix of the forwarder's pubkey (Identity.h:19-31).
  • A flood forwarder appends its own hash before retransmitting (Mesh.cpp:344-356, routeRecvPacket, copyHashTo at :349).
  • A received packet is logged before routing (Dispatcher.cpp:238 logRx, before processRecvPacket at :246/:256).
  • So the last hash of a flood path that an observer reports is the node it heard: a direct neighbour.

Not anchored:

  • DIRECT routes: the path is the remaining planned route, and each forwarder strips itself from the front (Mesh.cpp:89, removeSelfFromPath :334-342).
  • TRACE: always DIRECT; sendFlood refuses it (Mesh.cpp:638). Its header path carries SNR bytes (Mesh.cpp:60-61), and the ingestor's decoder replaces the hops with the planned route from the payload.
  • Route types are defined in Packet.h:14-17,64-65.

2. Relay-role prefix index (neighbor_builder.go)

buildPrefixIndex keeps only nodes for which isRelayRole is true. That is the server's canAppearInPath (cmd/server/store.go): a role containing repeater or room_server, or equal to room. It is copied because the two binaries share no package, and its tests mirror the server's TestCanAppearInPath.

On staging, 64 companion pubkeys are stored as relays today.

Firmware: companions and sensors ship with forwarding off:

  • companion_radio/NodePrefs.h:91 disable_fwd = 1, and MyMesh.cpp:891;
  • simple_sensor/SensorMesh.cpp:728.

Forwarding is opt-in (companion_radio/MyMesh.cpp:1402). A hop through a companion that opted in stays unresolved, or resolves to the one relay that shares its prefix. That is the same trade-off the server makes.

The index also feeds the neighbour-edge builder, so edges to companions are no longer derived from hop prefixes.

2b. Observer↔last-hop edges from flood routes only (neighbor_builder.go)

The builder used to add an observer↔last-hop edge for every observation. For a DIRECT route the path is the remaining planned route:

  • each forwarder matches itself at the front and strips itself (Mesh.cpp:89, removeSelfFromPath :334-342);
  • only flood forwarders append their hash (routeRecvPacket :346-350).

So a DIRECT path's last hop is the far end of the route, not the node the observer heard, and the edge was false. Since the observer anchor relies on these edges, the builder now adds the observer edge only for route types 0 and 1. A NULL route type is skipped too. The ADVERT originator↔first-hop edge and the interior hop-to-hop edges are unchanged; consecutive hops of a DIRECT route are neighbours.

This changes neighbor_edges for every consumer: the Neighbors panel, bridge nodes and the estimates built on the graph. The server reads the table; its extractEdgesFromObs has the same rule but no caller outside tests. DIRECT-derived edges already persisted stay until the neighbour-edge prune removes them (5 days by default), so the old edges fade out over that window after deploy.

3. Start-up window (main.go ~318 / ~487): not in this PR

Deferred until #141 was merged, because #141 rewrites that part of main.go. #141 (41675d98) has since been merged; point 3 stays with the original author. Meanwhile, the backfill below resolves rows ingested in that window. git merge-tree of this branch with 41675d98 merges cleanly.

4. Backfill (resolved_path_backfill.go, config.go, main.go)

There is one pass per ingestor start. It is started after StartNeighborEdgesBuilder, and it runs only once a neighbour graph from a successful edge build is published (see the readiness guard). It covers observation ids from the persisted watermark up to MAX(id) at the start of the pass.

  • Each batch:

    • reads at most batchSize rows by primary key, without writerMu;
    • resolves them outside any lock;
    • commits one WriterTx that sets resolved_path only where it is still NULL and moves the watermark. The watermark lives in a one-row table, resolved_path_backfill_state.
  • Restarts: a restart repeats at most the one uncommitted batch. Rows that still do not resolve stay NULL and are not retried in a later pass.

  • Readiness guard: the pass and every batch run only when three things hold. The edge build counts as complete only once a call has caught up, meaning it read fewer observations than its 50,000-row cap. The warm-up loops on rows scanned, not on edges produced. A full batch that yields no edge cannot move the builder's watermark; the warm-up then stops, logs it, and leaves the graph unpublished, so the backfill waits. Otherwise they write nothing, not even the watermark, and the background pass waits for the next post-build graph and resumes from the persisted watermark. The three conditions:

    • the prefix index is non-empty;
    • the neighbour graph was loaded after a neighbor_edges build that succeeded and caught up: StartNeighborEdgesBuilder publishes it after such a warm-up, or after such a tick;
    • that graph has at least one edge.

    Without the guard, a pass on the pre-build snapshot or on an empty graph would resolve only unique prefixes and move the watermark past every other row for good.

  • Config (defaults):

    • resolvedPathBackfill.batchSize = 500;
    • resolvedPathBackfill.pauseMs = 250;
    • resolvedPathBackfill.disabled = false.

    They are documented in config.example.json and docs/user-guide/configuration.md. For batchSize and pauseMs, 0 or omitted means the default, so the pause cannot be turned off; the smallest pause is 1 ms. These are candidates for the customizer later (AGENTS.md rule 8).

  • Server visibility: on master, the server's relay-credit indexes read resolved_path only at start-up (Load); with fix(store): index live observations from the persisted resolved_path, as Load does (#158) #182 they also read it when a row is first polled. Either way a backfilled value reaches those indexes only on the server's next restart. The server is not changed. A possible follow-up is a periodic server-side re-read of rows whose resolved_path changed, for example keyed by the backfill watermark.

Replay on staging data (public API, read-only)

Staging build int-0b11401d. 3,000 transmissions in 10 windows over 44 h, with 73,275 observations that carry a path. The replay runs this branch's Go code. Two graphs:

  • proxy: /api/analytics/neighbor-graph; neighbor_edges itself is not public;
  • lower bound: built from the sample alone with the ingestor's builder rules.

The old resolver reproduces staging's persisted NULL/non-NULL status for 98.5 % of rows.

Persisted today New, proxy graph New, lower-bound graph
Rows with resolved_path NULL 32.9 % 5.0 % 4.7 %
1-byte flood rows NULL 87.4 % (old resolver replay) 13.3 % 12.5 %
Hops resolved 59.6 % 69.7 % 75.2 %
Distinct relays per non-advert tx (≈ sum of traffic_share_score) 17.52 25.82 26.32
(tx, prefix) pairs with more than one pubkey across observers 0.1 % 4.0 % 4.0 %
Rows where the backward result was dropped as a conflict – 643 (0.9 %) 541 (0.7 %)

Performance

Resolution per observation (InsertTransmission hot path): benchmark on the staging sample, one op = one observation, n = 6.

Before (forward only, all-roles index) After
Time 757 ns/op 2,345 ns/op
Memory 339 B/op 638 B/op
Allocations 10 16

That is +1.6 µs per observation; at staging's ~4–5 observations/s it is about 10 µs of CPU per second. No new maps outside the call, no work under locks, and no map[string]interface{}.

Backfill write-lock hold per 500-row batch:

DB size p50 p99 Max
5,000 rows – – 1.6 ms
200,000 rows 1.6 ms 6.4 ms 7.4 ms
  • Measured on a local SSD.
  • The stated budget, asserted in TestResolvedPathBackfill_WriteHoldUnderBudget_188, is 250 ms.
  • With the default pause, the pass processes at most about 2,000 rows/s.

Tests

  • Observer anchor:
    • TestObserverAnchor_LastHopResolvesViaObserver_188
    • TestObserverAnchor_BackwardWalkResolvesEarlierHops_188
    • TestObserverAnchor_TwoNeighborCandidatesStayNil_188
    • TestObserverAnchor_AdvertForwardResultUnchanged_188
    • TestObserverAnchor_ForwardBackwardMeetOrNil_188 (agree / disagree / same node twice)
    • TestObserverAnchor_NeverContradictsForward_Random_188 (2,000 random graphs)
    • TestObserverAnchor_HashSizes_188 (1/2/3-byte)
    • TestObserverAnchor_DirectRouteIsNotAnchored_188
    • TestObserverAnchor_UnknownObserverHasNoEffect_188
    • TestInsertTransmission_ObserverAnchorResolvesLastHop_188
  • Relay roles:
    • TestIsRelayRole_MatchesServerCanAppearInPath_188
    • TestBuildPrefixIndex_RelayRolesOnly_188
    • TestInsertTransmission_CompanionNeverStoredAsRelay_188
  • Backfill:
    • TestResolvedPathBackfill_ResolvesRowsIngestedBeforePriming_188
    • …_BatchBoundAndWatermark_188
    • …_Idempotent_188
    • …_ResumesAfterRestart_188
    • …_StopKeepsWatermark_188
    • …_WriteHoldUnderBudget_188
    • …_RefusesBeforePriming_188
    • TestResolvedPathBackfillSettings_Defaults_188
  • Red before the change:
    • The observer-anchor and relay-role tests fail by assertion against today's behaviour (a forward-only or pass-through stub).
    • The backfill tests fail against a no-op backfill; only the defaults test passes there.
  • Existing fixtures: fixtures that seeded hop nodes without a role now give them role = 'repeater'. Expectations are unchanged.

Mutants (each in a copy of the tree, all red)

Mutant Caught by (among others)
Observer anchor removed TestObserverAnchor_LastHopResolvesViaObserver_188
"At least one" neighbour instead of "exactly one" TestObserverAnchor_TwoNeighborCandidatesStayNil_188
Role filter removed TestBuildPrefixIndex_RelayRolesOnly_188, TestInsertTransmission_CompanionNeverStoredAsRelay_188
Backfill ignores the persisted watermark TestResolvedPathBackfill_ResumesAfterRestart_188
Backfill does not persist the watermark TestResolvedPathBackfill_BatchBoundAndWatermark_188
Backfill batch without LIMIT TestResolvedPathBackfill_BatchBoundAndWatermark_188
Backfill runs before priming TestResolvedPathBackfill_RefusesBeforePriming_188
Backfill runs on the pre-build graph or an empty graph (review follow-up, 7 variants) the finding 1 tests above
Observer allowed as its own last hop, forward or backward (review follow-up) TestObserverAnchor_ObserverIsNeverItsOwnLastHop_188
Backward walk keeps its anchor across a nil hop (m9) …_BackwardWalkBreaksOnNilHop_188
Backward walk without its exclusion set (m10) …_BackwardWalkExcludesResolvedHops_188
Backfill passes from_pubkey for non-ADVERT rows (m12) …_FromPubkeyOnlyForAdverts_188
Merge keeps the backward value on conflict TestObserverAnchor_ForwardBackwardMeetOrNil_188/disagree…
DIRECT routes anchored TestObserverAnchor_DirectRouteIsNotAnchored_188

The "priming after drain" mutant belongs to point 3, which is not in this PR.

Review follow-up (findings 1, 2, 4, 5, 8, and the second review's P2-1 and P2-2)

Finding Commits Tests
1. Backfill on a pre-build or empty graph f9bae200 (red), cbaa9ca6 (fix), f5add678 …_UsesGraphFromFirstEdgeBuild_188, …_EmptyGraphKeepsWatermark_188, …_StartWaitsForFirstEdgeBuild_188, TestResolvedPathBackfillReady_188, TestNeighborEdgesBuilder_TickPublishesBuiltGraphAfterFailedWarmUp_188
2. Config keys documented; pauseMs: 0 means the default d1db161c TestConfigExample_ResolvedPathBackfill_188, TestResolvedPathBackfillSettings_ZeroMeansDefault_188
4. The observer is never its own last hop (flood) e610dbb8 (red), 503ea758 (fix) TestObserverAnchor_ObserverIsNeverItsOwnLastHop_188, TestObserverAnchor_ObserverMayBeAnEarlierHop_188
5. Surviving mutants m9, m10, m12 0c623dba …_BackwardWalkBreaksOnNilHop_188, …_BackwardWalkExcludesResolvedHops_188, TestResolvedPathBackfill_FromPubkeyOnlyForAdverts_188
8. TRACE comment and firmware line numbers 2e2886af comments only
Second review P2-1: warm-up ends on rows scanned, not edges e369ae69 (red), da0494c4 (fix) TestNeighborEdgesBuilder_WarmUpScansPastFullBatchWithFewEdges_190, TestResolvedPathBackfill_WaitsWhileEdgeBuildCannotCatchUp_190
Second review P2-2: observer edges from flood routes only addccb88 (red), 194512d2 (fix), 0f4c167a TestNeighborEdgesBuilder_ObserverEdgeOnlyForFloodRoutes_190

The resolver benchmark is now in the repo: BenchmarkResolveObservationPath_188 (700 relays, 60 observers, 1–5 hop 1-byte flood paths). Finding 4 costs nothing measurable: median 3,875 → 3,890 ns/op, 119 B/op and 5 allocs/op on both sides (n = 12 each, interleaved, same machine).

Deploy notes and further effects (added at merge, from the review of 0f4c167a)

Two further effects of section 2b:

  • On observer detail pages, the "confirmed / not seen yet" state can change for neighbours whose only edge to that observer came from DIRECT observations.
  • The DIRECT-derived part of an edge's count never goes away for node pairs that keep getting real flood observations. Neighbour-prune only removes edges with no new observations for 5 days, so such counts stay somewhat inflated rather than being corrected.

Deploy:

  • On a large database, the first backfill pass scans every observation at no more than about 2,000 rows/s. Expect it to take from 1.5 to 4 hours on a 5 GB database. This is an estimate.
  • The server only sees backfilled rows after its next restart. Restart the server after the ingestor logs [resolved_path_backfill] done, and only then read the traffic-share sum.

Not verified

  • Staging after deploy: the sum should settle near the estimate and not grow with uptime. The lead or developer verifies this.
  • The replay uses a proxy graph and a lower-bound graph, not staging's neighbor_edges.
  • The backfill's hold time on the staging host and its total duration on the staging DB.
  • main.go ordering: the background pass now waits for the first post-build graph whatever the call order (…_StartWaitsForFirstEdgeBuild_188).

🤖 Generated with Claude Code

dborup and others added 4 commits October 3, 2026 10:47
#188)

A non-advert observation has no anchor for its first hop, so with 1-byte
hashes (about 7 nodes per prefix on staging) every hop stayed nil and the
row was stored as NULL. The observer is a better anchor: a flood forwarder
appends its hash before retransmitting (MeshCore Mesh.cpp routeRecvPacket)
and the observer logs the packet before its own routing step
(Dispatcher.cpp logRx), so the last hash is the node it heard, a direct
neighbour.

resolveObservationPath keeps the forward chain from fromPubkey and, for
flood route types only, adds a backward chain: the last hop is the unique
neighbour of the observer, each earlier hop the unique neighbour of the
hop after it. The merge keeps every forward hop and fills nil hops from the
backward chain; if the two disagree on a hop, or would place one node
twice, the backward result is dropped for the row. No geo, GPS or count
tie-break. DIRECT and TRACE paths are not anchored.

path_resolver.go is gofmt-ed as a whole (one comment table moved).

Relates to #188, #184

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The ingestor's prefix index held every node, so a prefix shared by one
repeater and a companion counted as ambiguous, and a prefix unique to a
companion resolved to it (64 companion pubkeys are stored as relays on
staging). buildPrefixIndex now keeps only nodes for which isRelayRole is
true: the server's canAppearInPath (repeater, room_server, room), with
the same test cases. Companions and sensors ship with forwarding off in
the firmware (companion_radio NodePrefs.h, simple_sensor SensorMesh.cpp);
a companion that opted in to repeating stays unresolved, as on the server.

The index also feeds the neighbour-edge builder, so edges to companions
are no longer derived from a hop prefix. Existing fixtures that seeded hop
nodes without a role now give them role 'repeater'; expectations are
unchanged.

Relates to #188, #184

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…rimed (#188)

Rows stored before the observer anchor and the relay-only index, and rows
drained from the start-up buffer before the prefix index is primed, keep
resolved_path = NULL. RunResolvedPathBackfill re-resolves them once per
ingestor start, with resolveObservationPath, for observation ids between
the persisted watermark and MAX(id) at the start of the pass.

Each batch reads at most batchSize rows by primary key without writerMu,
resolves them outside any lock, and commits one WriterTx that sets
resolved_path only where it is still NULL and moves the watermark
(resolved_path_backfill_state, one row). A restart repeats at most the
uncommitted batch. Batches are paced by a pause. Defaults: 500 rows,
250 ms, configurable as resolvedPathBackfill.{batchSize,pauseMs,disabled}.
main.go starts the pass right after StartNeighborEdgesBuilder has primed
the index and graph.

The server indexes a row when it first polls it or at Load, so backfilled
values reach its indexes on its next restart; the server is unchanged.

Relates to #188, #184

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… is primed (#188)

Started before StartNeighborEdgesBuilder had primed the prefix index and
graph, a pass would resolve nothing yet move its watermark past every row,
so those rows would never be retried. RunResolvedPathBackfill now returns
an error and leaves the watermark unchanged until both are loaded.

Relates to #188

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

dborup commented Oct 3, 2026

Copy link
Copy Markdown
Owner

Review — CS-MacBook review PR#190 observer-anker — head 1cfb42d

Dom: APPROVE med nits

Independent, read-only review of head 1cfb42d91eedf9f2c2527989e2f0e27d9f672668 (4 commits, 12 files, cmd/ingestor only), against master 751f8fc8. The head was unchanged on git ls-remote before and after the review. I read #188 and #184 first; #182 is still an open draft (see finding 3).

Evidence tags: [F] measured against the staging public API, [T] test or benchmark I ran, [A] analysis from reading code or firmware, [K] not checked.

Summary

I found no correctness bug in the resolver, the role filter or the backfill that blocks the merge. The firmware reasoning holds, the merge rule is sound for identity and duplicates, and live ingest is not starved. The deliberate scoring change behaves as described: my own replay on a fresh staging sample reproduces the PR's NULL-share numbers. Three things I would fix or document before merging (findings 1 to 3), plus smaller nits.

Findings

# Priority Where Finding
1 Medium (robustness) resolved_path_backfill.go:137-139 (guard), neighbor_builder.go:76-82 The priming guard only checks != nil. An empty but non-nil graph passes it, and StartNeighborEdgesBuilder loads the graph snapshot before its initial edge build, so the backfill starts on that pre-build snapshot. With empty or stale neighbor_edges (fresh or restored DB, or edges pruned) the pass resolves only unique-prefix hops, moves the watermark past every row, and never retries them. [T] My probe (batch size 10, 20 NULL rows, empty graph) ends with Resolved:0, watermark = last id. On staging and prod, neighbor_edges is persisted, so the risk is low there. Fix: refresh the graph after the warm-up loop, and/or require len(adj) > 0, and/or wait for the first tick.
2 Low-Medium (docs) config.example.json The three new keys resolvedPathBackfill.{disabled,batchSize,pauseMs} are not in config.example.json (checked with grep on the PR tree). Other ingestor options carry a _comment_* entry there. [T]
3 Medium (sequencing, informational) PR body The text says "Since #182, the server credits relays only from that column". #182 (head 504b5147) is an open draft and is not in master. [T] git merge-base --is-ancestor says no. From its title and diff, on master only the start-up load reads the persisted column. [A] So on master alone the score change shows up at server restart, not gradually. Please state the intended merge order (#182 first) or the dependency in the PR body.
4 Low path_resolver.go:275-277 (resolvePathBackward) The observer is not excluded from the candidates of the last hop. An observer cannot be its own last hop. [T] Probe: observer with a unique 1-byte relay prefix, path ["0b"], resolves to the observer itself. Master has the same flaw through the forward chain (unique prefix), so this is not a regression, but the observer is now known and the fix is one seen[observer] entry for i == len(hops)-1.
5 Low (test gap) new tests Three mutants survive the whole ingestor suite, not only the targeted tests (see Mutants): the backward walk not breaking on a nil hop, the backward walk without its exclusion set (practically equivalent), and the backfill passing from_pubkey for non-ADVERT rows. [T] I wrote a probe that kills the first one ([a1 b2 c3], hop 1 ambiguous, hop 0 only reachable through the stale anchor). The benchmark behind the perf table is not in the repo.
6 Low (design note) path_resolver.go:300 (mergeForwardBackward) The merge checks identity and duplicates, not adjacency at the seam between a forward hop and a backward hop. [T] Probe: forward resolves a1, backward resolves b2 and c3, the graph has no a1~b2 edge, and the stored path is [a1 b2 c3]. This is plausible (the graph is incomplete, not proof of absence) and not a contradiction, but the stored path asserts an adjacency the graph does not contain.
7 Nit resolved_path_backfill.go:85-93 ensureResolvedPathBackfillState runs s.db.Exec directly. The other backfill DDL (route_mask_backfill.go:~96-111) goes through writerMu + recordWriterTiming. With MaxOpenConns(1) it is safe, but it does not show up in /api/perf writer timings. The backfill itself is not registered via RunAsyncMigration (MIGRATIONS.md "Option 1, preferred for backfills"), so only the log lines show its progress. Acceptable because it is bounded and rate-limited. [A]
8 Nit comment at path_resolver.go ("TRACE: the path carries SNR bytes") The raw header path of a TRACE carries SNR bytes (Mesh.cpp:61), but the ingestor's decoder replaces path.Hops with the payload's planned-route hashes (decoder.go, TRACE branch). The conclusion (TRACE is DIRECT, so never anchored) is unchanged. Firmware line numbers in the comments are off by one in two places (removeSelfFromPath is Mesh.cpp:334, routeRecvPacket appends at :344-353). [A]

Answers to the review points

1. Resolver correctness

  • Flood only (route types 0 and 1): correct. [A] Dispatcher.cpp:238 calls logRx before processRecvPacket (:246/:256). For flood packets Mesh.cpp:344-353 (routeRecvPacket) appends the forwarder's own hash after it received the packet. So the last hash an observer reports is the node it heard directly. For a zero-hop flood the path is empty, so nothing is anchored.
  • Direct routes (2/3): correctly not anchored. [A] Mesh.cpp:89 matches self_id against the front of the path and removeSelfFromPath (:334-342) strips it before retransmit. The path is the remaining planned route, so its last hash is a node ahead, not a neighbour. TRACE is always direct (Mesh.cpp:50-64). TestObserverAnchor_DirectRouteIsNotAnchored_188 covers both direct types, and mutant m2 fails it. [T]
  • "Exactly one" in both directions: both chains share resolveHopWithContext, so the rule is identical. Mutant m1 (at least one) is caught by four tests. [T]
  • Merge rule: forward values are never changed, a conflict or a node at two positions drops the whole backward result. The 2,000-graph random test and the mutants m3a/m3b/m3c show these are enforced. Flood forwarders deduplicate with wasSeen, so a node cannot legitimately appear twice in one path, which makes the duplicate rule sound. [A][T] Impossible or self-contradicting results are therefore ruled out on identity; seam adjacency is not checked (finding 6).
  • Observer identity: the observer is the MQTT topic segment parts[2] (main.go:714, :811), stored in observers.id; the backfill joins observers.rowid = observation.observer_idx exactly like the neighbour builder. Graph lookups lower-case on both sides, so the upper-case topic id works (tested with an upper-case id). [A][T]
    • Unknown ids (companion, empty, not-a-node) have no edges and anchor nothing; tested.
    • Observers that are not repeaters still anchor: the builder writes observer↔last-hop edges for any observer id, whether or not it is in nodes (neighbor_builder.go, the observerPK != "" branch). The PR text "an id that is not a node has no edges" is therefore true only for ids that are not pubkeys. [A]
    • MQTT bridges that aggregate several radios have a wider neighbour set. That makes more candidates adjacent, so the effect is more nil, not wrong attribution. [A]
  • Stale graph / moved repeater: the exact-one rule means a stale edge to a moved node produces two survivors (nil) when the new neighbour has an edge too. The real misattribution mode is a genuine new neighbour Z whose edge does not exist yet and which shares a 1-byte prefix with a known neighbour X: the resolver picks X. Edges live for 5 days. To size it I truncated 13,126 staging flood observations that had 2-byte paths with persisted resolutions to 1 byte and re-resolved them with the PR's code and the staging graph proxy: 17,935 of 83,210 hops (21.6 %) were resolved, 591 of them (3.3 %) differ from the persisted 2-byte resolution. [F][T] This is an optimistic bound for the graph (it already contains the edges those 2-byte paths created) and the "truth" is itself a resolver output. It matches the 3.2-4.0 % cross-observer disagreement below. The backfill also resolves old rows with today's graph, so rows from before an edge change are attributed with later topology (bounded by the 5-day edge window).

2. Relay-role filter

  • Same definition as the server: isRelayRole is character for character canAppearInPath (cmd/server/store.go:7126-7129); TestIsRelayRole_MatchesServerCanAppearInPath_188 mirrors the server cases. [A][T] On staging every node has a role (1,522 repeaters + 72 rooms + 284 companions + 1 sensor = 1,879), so no role-less node is silently dropped, and the 1,594 relay roles are the nodes in my replay index. [F]
  • Firmware nuance: verified at MeshCore 0679dbe: NodePrefs.h:91 (disable_fwd = 1), companion MyMesh.cpp:891 setRepeatEn(false) and :1402 opt-in, sensor SensorMesh.cpp:728. [A] The residual risk is a companion or sensor that opted in to relaying: its hop stays nil or resolves to the single repeater sharing the prefix. It is documented in the PR body and the code comment, and it is the server's trade-off too. Its size on the real mesh is [K]. Acceptable.
  • Changed fixtures: five existing tests only gained role = 'repeater' on their INSERT INTO nodes; the assertions are untouched (diff reviewed line by line). Nothing weakens, but the tests no longer cover "a node with no role resolves", which is the intended behaviour change. [A]

3. Backfill

  • 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: all writes are in cmd/ingestor; cmd/server is untouched and does not reference the new table. [A]
  • State table: created lazily by the first pass (CREATE TABLE IF NOT EXISTS, PREFLIGHT annotation present and placed directly above the statement). The server opens read-only and its schema gate / AssertReady do not reference it. I cannot run the external check-async-migrations.sh (it is not on this machine). [A][K] See nit 7 for writerMu.
  • Batch, watermark, idempotence, resume: every batch is LIMIT ? by primary key, writes UPDATE … WHERE resolved_path IS NULL and the watermark in one WriterTx. …_BatchBoundAndWatermark, …_Idempotent, …_ResumesAfterRestart and …_StopKeepsWatermark pass, and mutants m5a, m5b, m7, m8, m11 are all caught. A restart repeats at most the uncommitted batch; rows above the starting ceiling are picked up on the next start. [T]
  • Live-ingest starvation: 200,000 NULL rows, a 1,500-insert live loop at one insert per ~2 ms, InsertTransmission latency: baseline p50 275 µs / p99 0.97 ms / max 4.8 ms; during a default backfill (500 rows / 250 ms) p50 268 µs / p99 1.2 ms / max 1.9 ms; worst case (500 rows / 1 ms pause) p50 285 µs / p99 4.5 ms / max 9.7 ms. The read phase also holds the single connection, which the PR's hold figures do not include, but the end-to-end figure above covers it. No starvation. Local SSD, in-process, not staging hardware. [T]
  • Can it skip rows? Yes in three cases, one of them accidental: (a) rows that do not resolve stay NULL by design; (b) rows whose node enters the index after the batch ran are not revisited; (c) an empty or stale graph (finding 1). The nil guard itself works (…_RefusesBeforePriming_188, mutant m6). [T]
  • Server visibility: documented in the PR body and in the file header. The consequence is worth spelling out in the deploy notes: the server and ingestor restart together, the backfill finishes minutes to hours later, and the server then only sees backfilled values after a second server restart. [A]
  • Config: defaults 500 rows / 250 ms / enabled give about 2,000 rows/s; that matches the 8,000 rows my 3.5 s loop reached. They are sensible. They are not in config.example.json (finding 2). pauseMs: 0 cannot be set (0 means default). [T]

4. Performance

  • Resolution: the PR reports 757 → 2,345 ns/observation. My synthetic benchmark (700 relays, 60 observers, 1-byte paths of 1-5 hops, Apple M-series) shows 160 → 1,290 ns/op, 29 → 100 B/op, 1 → 4 allocs/op. Same order of magnitude, +1.1 µs. The resolver runs under writerMu (as before), which adds microseconds against a millisecond insert. [T]
  • No new unbounded structure (per-call maps only) and no new map[string]interface{} (grep of the added lines). [A]
  • The checked-in tests contain no benchmark, so the table in the PR body is the only record of the 757/2,345 numbers (finding 5).

5. Effect (fresh replay)

I fetched the nodes list, the neighbour graph and 8 windows of 150 transmissions (about 21,940 path observations over ~36 h) from the staging public API, a handful of GET requests, and ran this branch's Go code over them with the graph proxy.

Persisted Old resolver replay New resolver replay
Rows with NULL resolved_path 28.0 % 27.6 % 3.9 %
Hops resolved 65.0 % 64.6 % 71.1 %
1-byte non-advert flood rows NULL – 100 % 13.7 %
Distinct relays per non-advert tx (n = 529) 33.12 32.76 44.74 (× 1.35)
(tx, prefix) with more than one pubkey across observers – 0.0 % 3.2 %

[F][T] The old-resolver replay reproduces the persisted NULL share (27.6 % vs 28.0 %), so the replay is credible, and the new-resolver numbers agree with the PR's (5.0 % NULL, 13.3 % for 1-byte, 4.0 % disagreement). My ratio is 1.35 instead of 1.47 because my sample is biased towards well-observed transmissions. Absolute levels differ from the PR (33 vs 17.5) for the same reason.

The expected Traffic Share sum of about 22-26 is plausible as an end state. computeRepeaterUsefulnessScoreMap (cmd/server/repeater_enrich_bulk.go:222) divides each node's relayed-transmission count by all non-advert transmissions in the in-memory store, so the sum equals the average distinct relays per transmission. It does grow with uptime until the store window has turned over: after the combined deploy, old transmissions in memory keep their old resolution, new ones get the new one, and backfilled rows only appear after a further server restart. Verify on staging after the backfill logs "done" and a server restart, or after one full window turnover; a rising sum during that period is the transition, not drift. [A]

6. Conflict with #141

git merge-tree --write-tree of this head with 41675d98 (#141) and with master 751f8fc8 are both clean (exit 0). I also built both merged trees: go vet is clean and the 188/resolved-path/neighbour-builder tests pass on each. [T]

Tests

  • cmd/ingestor go test ./...: ok. 703 top-level PASS, 0 FAIL, 1 SKIP, plus 157 passing subtests. 21 new _188 tests, all pass. [T]
  • cmd/server go test ./...: ok. 2,050 top-level PASS, 0 FAIL, 2 SKIP, 624 passing subtests. [T]
  • -race -count=10, affected tests (-run '188|ResolvedPath|NeighborEdges|NeighborGraph|PrefixIndex|Resolve'): ok, no data race reported (1,170 s). [T]
  • go vet clean; gofmt -l is clean for every file the PR adds or changes except neighbor_builder_test.go, which is also unformatted on master. [T]
  • Red on master: I put the 3 new test files on master 751f8fc8 behind a stub (forward-only resolveObservationPath, isRelayRole always true, no-op backfill). 19 of 21 tests fail by assertion. TestObserverAnchor_DirectRouteIsNotAnchored_188 and …_UnknownObserverHasNoEffect_188 pass on master because they assert a non-effect; the first is guarded by mutant m2; no mutant of mine targets the unknown-observer test. [T]

Mutants (each in a copy of the PR tree, then the targeted suite)

Mutant Result Caught by
m1 at least one neighbour instead of exactly one (both chains) caught TestObserverAnchor_TwoNeighborCandidatesStayNil_188, …_BackwardWalkResolvesEarlierHops_188, …_ForwardBackwardMeetOrNil_188/agree, TestResolveHopWithContext_OneByteCollision_AdjacencyResolves
m2 backward chain also for direct routes caught TestObserverAnchor_DirectRouteIsNotAnchored_188
m3a merge without the conflict check caught TestObserverAnchor_ForwardBackwardMeetOrNil_188/disagree…
m3b merge without the duplicate check caught …_ForwardBackwardMeetOrNil_188/same_node_at_two_positions…, …_NeverContradictsForward_Random_188
m3c backward wins on conflict caught …_ForwardBackwardMeetOrNil_188/disagree…, …_NeverContradictsForward_Random_188
m4 role filter removed caught TestBuildPrefixIndex_RelayRolesOnly_188, TestInsertTransmission_CompanionNeverStoredAsRelay_188
m5a backfill ignores the persisted watermark caught TestResolvedPathBackfill_Idempotent_188, …_ResumesAfterRestart_188
m5b backfill never persists the watermark caught …_BatchBoundAndWatermark_188, …_Idempotent_188, …_ResumesAfterRestart_188, …_StopKeepsWatermark_188
m6 priming guard removed caught TestResolvedPathBackfill_RefusesBeforePriming_188
m7 backfill overwrites non-NULL rows caught TestResolvedPathBackfill_Idempotent_188
m8 batch without LIMIT caught …_BatchBoundAndWatermark_188, …_Idempotent_188, …_ResumesAfterRestart_188, …_StopKeepsWatermark_188, …_WriteHoldUnderBudget_188
m11 ceiling ignored caught …_BatchBoundAndWatermark_188
m13 TRANSPORT_FLOOD (0) not anchored caught TestObserverAnchor_LastHopResolvesViaObserver_188
m9 backward walk does not break on a nil hop survived none (my probe kills it)
m10 backward walk without its exclusion set survived none (equivalent in practice: the merge's duplicate check drops the row)
m12 backfill passes from_pubkey for every payload type survived none (fixtures have no from_pubkey on non-adverts)

CI (head 1cfb42d9, run on the pull_request event)

Go Build & Test: pass (13m07s). Playwright E2E: pass (22m11s). Build & Publish Docker Image: pass (50s). Deploy Staging, Release Artifacts, Publish Badges & Summary: skipped, as expected for a PR. [F] .github/workflows/deploy.yml is not touched by the PR and contains 9 × github.repository == 'Kpa-clawbot/CoreScope' on both master and the head. [T]

Not verified

  • [K] Behaviour on staging after deploy (sum settling, spot checks), the backfill's total duration and hold time on the real DB and host.
  • [K] Real-mesh frequency of companions or sensors that opted in to relaying.
  • [K] The external check-async-migrations.sh preflight (not available here); the replay uses the API's neighbour graph, not neighbor_edges, and its accuracy figures are optimistic bounds.
  • [K] Whether a main.go ordering test is needed beyond the priming guard (the PR already lists it as untested); point 3 of feat(ingestor): resolve the last hop from the observer, relay-only prefix index, backfill NULL resolved_path #188 (start-up window) is deferred as agreed.

dborup and others added 8 commits October 3, 2026 13:32
…y graph (#188)

PR #190 review, finding 1. StartNeighborEdgesBuilder loads the graph
snapshot before its warm-up build, and the backfill only checks that the
graph is non-nil. With empty neighbor_edges the pass resolves only unique
prefixes and moves its watermark past every row for good.

Three tests, red by assertion on 1cfb42d:
- the pass right after StartNeighborEdgesBuilder uses the pre-build graph;
- a pass and a single batch on an empty graph commit a watermark;
- the background pass started before the first build runs at once.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QbNdgonR8kq7SPEU7LcmXD
…raph (#188)

PR #190 review, finding 1. The backfill could run on the graph snapshot
that StartNeighborEdgesBuilder loads before its warm-up build, or on an
empty graph. It then resolved only unique prefixes and moved its
watermark past every other row for good.

- StartNeighborEdgesBuilder publishes the graph again after a successful
  warm-up build, and after each successful tick, as a post-build snapshot
  (neighborGraphHolder.storeBuilt).
- The pass and every batch run only with a non-empty prefix index and a
  post-build graph that has at least one edge; otherwise they write
  nothing, not even the watermark (errResolvedPathBackfillNotReady).
- StartResolvedPathBackfill waits for the next post-build graph and
  retries; the retry resumes at the persisted watermark.
- Test setup primes the way the builder does: build, then publish.
- TestResolvedPathBackfillReady_188 pins each readiness condition.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QbNdgonR8kq7SPEU7LcmXD
…t rule (#188)

PR #190 review, finding 2. The three keys were missing from
config.example.json. They now sit in a resolvedPathBackfill block with a
_comment, like the other ingestor blocks, and in the configuration guide.

pauseMs: 0 cannot switch the pause off, because 0 means the default.
That is documented rather than changed: a pass with no pause at all would
hold the single write connection back to back. The smallest pause is 1 ms.

Tests:
- TestConfigExample_ResolvedPathBackfill_188 parses config.example.json and
  checks the block and its defaults (red before the block was added);
- TestResolvedPathBackfillSettings_ZeroMeansDefault_188 pins 0 and negative
  values to the defaults and 1 to 1 ms.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QbNdgonR8kq7SPEU7LcmXD
…188)

PR #190 review, finding 5. Three mutants survived the whole ingestor
suite. Each test passes on the code and fails on its mutant:

- m9, the backward walk keeps its anchor across a nil hop:
  TestObserverAnchor_BackwardWalkBreaksOnNilHop_188 (the reviewer's
  [a1 b2 c3] probe, hop 1 ambiguous).
- m10, the backward walk without its exclusion set. It was reported as
  practically equivalent, but it is not: when a prefix repeats, the
  exclusion leaves one candidate where there would be two.
  TestObserverAnchor_BackwardWalkExcludesResolvedHops_188.
- m12, the backfill passes from_pubkey for non-ADVERT rows:
  TestResolvedPathBackfill_FromPubkeyOnlyForAdverts_188. Today only
  ADVERTs carry from_pubkey, so on real data this mutant has no effect.
  The test keeps the backfill consistent with InsertTransmission if that
  changes.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QbNdgonR8kq7SPEU7LcmXD
…warm-up (#188)

Follow-up to the finding-1 fix. If the warm-up build fails, the backfill
waits for the first successful tick. The mutant "a tick never publishes a
post-build graph" survived the targeted suite; this test kills it.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QbNdgonR8kq7SPEU7LcmXD
…188)

PR #190 review, finding 4. In a flood path an observer reports, the last
hop is the node it heard, never itself. With the observer's prefix unique,
both chains name the observer: path ["0b"] resolves to obs188. With path
["c3","0b"], the backward walk then anchors hop 0 on it.

TestObserverAnchor_ObserverIsNeverItsOwnLastHop_188 is red by assertion.
TestObserverAnchor_ObserverMayBeAnEarlierHop_188 is green and pins the
scope: an observer logs a packet before de-duplication, so it can report an
echo of a flood it forwarded, with its own hash earlier in the path.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QbNdgonR8kq7SPEU7LcmXD
…#188)

PR #190 review, finding 4. A radio does not receive its own
transmission, so the last hop of a flood path an observer reports is
never the observer. Both chains now leave that hop nil instead:

- the forward chain (resolvePathForward, the walk behind
  resolvePathWithContext) does not take the observer as a candidate for the
  last hop of a flood path. With a unique prefix, the forward chain alone
  named the observer, so a fix in the backward walk alone would not
  change the stored row;
- the backward walk excludes the observer from the last hop only. It may
  still be an earlier hop: it logs a packet before de-duplication
  (Dispatcher.cpp logRx before processRecvPacket), so it can report the
  echo of a flood it forwarded.

DIRECT routes and resolvePathWithContext's other callers are unchanged.

Perf (hot path, InsertTransmission): BenchmarkResolveObservationPath_188 is
new, because the PR's benchmark was not in the repo. It uses 700 relays,
60 observers and 1-5 hop 1-byte flood paths; one op is one observation.
Run interleaved, n=12 each, on the same machine:
- before: median 3,875 ns/op, 119 B/op, 5 allocs/op;
- after: median 3,890 ns/op, 119 B/op, 5 allocs/op.
That is within noise. The change adds one map insert per flood
observation.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QbNdgonR8kq7SPEU7LcmXD
PR #190 review, finding 8. A TRACE's header path carries SNR bytes, but
the ingestor's decoder replaces the hops with the planned route from the
payload. TRACE is not anchored because it is always DIRECT: sendFlood
refuses it. The comment now says so.

Firmware references are checked against MeshCore a366955, the version
cloned here; the PR cited 0679dbe:
- routeRecvPacket is at Mesh.cpp:344-356, with copyHashTo at :349;
- removeSelfFromPath is at :334;
- the front-of-path match is at :89;
- the SNR append is at :60-61;
- logRx is at Dispatcher.cpp:238, before processRecvPacket at :246/:256.

Comments only.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QbNdgonR8kq7SPEU7LcmXD
@dborup

dborup commented Oct 3, 2026

Copy link
Copy Markdown
Owner

Review of head 2e2886af5dd741cc3aa5a5ec5a0edceeacd8b0e9: I found two issues to address before merging.

  1. [P2] The backfill can advance its watermark with an incomplete neighbour graph. StartNeighborEdgesBuilder reads at most 50,000 observations per batch, but its warm-up loop stops when buildAndPersistNeighborEdges returns fewer than 50,000 edges (neighbor_builder.go:83–103). That return value is len(edges), not the number of observations scanned (:318–324). A full 50,000-observation batch can produce only a few edges, causing warm-up to stop despite more observations remaining. The new backfill readiness check accepts any successful build with a non-empty graph (resolved_path_backfill.go:120–125), then permanently advances past rows it cannot resolve with that partial graph. Please make warm-up completion depend on the scanned-row count/exhaustion and test a full batch that produces fewer than 50,000 edges before allowing backfill to move its watermark.

  2. [P2] DIRECT traffic can create false observer-to-last-hop edges used by the new flood resolver. The neighbour builder adds observer ↔ last path hop for every observation, without reading or checking route_type (neighbor_builder.go:200–212, 246–250). For a DIRECT packet, the path contains the remaining planned route, not the relays already traversed; MeshCore removes the next hop from its front when forwarding. Thus the last listed hop need not be the transmitter heard by the observer. resolveObservationPath now uses those same edges to select a unique candidate for an ambiguous FLOOD hop (path_resolver.go:333–342, 354–374), so a false DIRECT-derived edge can assign relay credit to the wrong node. Please exclude unsupported route types from this endpoint-edge evidence (or otherwise distinguish trustworthy edges) and add a mixed DIRECT/FLOOD regression test.

These are code-path findings; I have not run a staging replay or changed the PR. The Go CI job passed on this head, while the Playwright job was still in progress at my last check. I also noticed that #182 has since landed on master, so the final integration should be verified against the current base.

Brings in #182 (9d29dae), #191, #194 and #196, so that the observer anchor
and the backfill are tested against the current base. Master's server now
indexes live observations from the persisted resolved_path (#182).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QbNdgonR8kq7SPEU7LcmXD
@dborup-agent

Copy link
Copy Markdown
Collaborator

Rapport — CS-pve-agent1 PR#190 review-rettelser — head 2e2886a

Status: Findings 1, 2, 3, 4, 5 and 8 are fixed, and findings 6 and 7 are assessed with no change. CI is green on 2e2886af. The two P2 findings in the later review of 2e2886af are handled in a follow-up round with its own report.

This follows up on the review comment "Review — CS-MacBook review PR#190 observer-anker — head 1cfb42d". I built on 1cfb42d9. git ls-remote showed that head before I started and again right before the push. I only fast-forwarded the branch with 8 new commits: no rebase, no amend, no force push. All changes are in cmd/ingestor, config.example.json and docs/; cmd/server is untouched.

Evidence tags:

  • [T] a test, benchmark or CI run;
  • [A] analysis of code or firmware;
  • [K] taken from the review and not run again.

Findings

# Result Commits Tests Evidence
1 Fixed f9bae200 (red), cbaa9ca6 (fix), f5add678 TestResolvedPathBackfill_UsesGraphFromFirstEdgeBuild_188, …_EmptyGraphKeepsWatermark_188, …_StartWaitsForFirstEdgeBuild_188, TestResolvedPathBackfillReady_188, TestNeighborEdgesBuilder_TickPublishesBuiltGraphAfterFailedWarmUp_188 [T] red before, green after; 7 mutants caught
2 Fixed (documented) d1db161c TestConfigExample_ResolvedPathBackfill_188, TestResolvedPathBackfillSettings_ZeroMeansDefault_188 [T]
3 Fixed (PR body) – – [A] master server code read
4 Fixed e610dbb8 (red), 503ea758 (fix) TestObserverAnchor_ObserverIsNeverItsOwnLastHop_188, TestObserverAnchor_ObserverMayBeAnEarlierHop_188 [T] red before, green after; 4 mutants caught; benchmark
5 Fixed 0c623dba TestObserverAnchor_BackwardWalkBreaksOnNilHop_188, …_BackwardWalkExcludesResolvedHops_188, TestResolvedPathBackfill_FromPubkeyOnlyForAdverts_188 [T] m9, m10 and m12 all caught
6 Assessed, no change – – [A]
7 Assessed, no change – – [A]
8 Fixed 2e2886af comments only [A] firmware a366955

1. Backfill on a pre-build or empty graph

Change:

  • StartNeighborEdgesBuilder publishes the graph again after a successful warm-up build, and after each successful tick, as a post-build snapshot (neighborGraphHolder.storeBuilt).
  • The pass, and every batch, run only when all three hold (resolvedPathBackfillReady):
    • the prefix index is non-empty;
    • the graph is a post-build snapshot;
    • the graph has at least one edge.
  • Otherwise they return errResolvedPathBackfillNotReady and write nothing, not even the watermark.
  • StartResolvedPathBackfill then waits on a channel that the next post-build graph closes, and retries from the persisted watermark. It does not poll, and it no longer depends on the call order in main.go. main.go itself is unchanged; feat(ingestor): resolve the last hop from the observer, relay-only prefix index, backfill NULL resolved_path #188 point 3 is not touched.

Red before, green after [T]. The three reproduction tests from f9bae200, on the PR code at 1cfb42d9:

--- FAIL: TestResolvedPathBackfill_UsesGraphFromFirstEdgeBuild_188    row 1 is still NULL
--- FAIL: TestResolvedPathBackfill_EmptyGraphKeepsWatermark_188       the pass ran on an empty neighbour graph
--- FAIL: TestResolvedPathBackfill_StartWaitsForFirstEdgeBuild_188    the pass committed watermark 11 before the first edge build

All three pass from cbaa9ca6.

  • The first test is the review's exact scenario: neighbor_edges is empty, the builder starts, then the pass runs. The builder derives the observer↔relay edge from a 2-byte path in the data, but the pass used the pre-build snapshot.
  • The empty-graph test also proves the next half: once a build finds the edge, the same rows resolve, because the watermark did not move.

Test-setup change: primeIndexAndGraph188 now primes the way the builder does: build, then publish the post-build graph. The existing backfill tests keep their assertions.

2. Config

  • config.example.json now has a resolvedPathBackfill block with disabled, batchSize and pauseMs at their defaults, and a _comment in the style of retention and db. docs/user-guide/configuration.md has a short table.
  • pauseMs: 0 is documented, not changed: 0 (or a negative value) means the default for both numbers, so the pause cannot be switched off; the smallest pause is 1 ms.
    • Reason: a pass with no pause holds the single write connection back to back. The review's worst case already used a 1 ms pause.
    • TestResolvedPathBackfillSettings_ZeroMeansDefault_188 pins 0, −1 and 1.
    • TestConfigExample_ResolvedPathBackfill_188 parses the example file. It was red before the block was added: config.example.json has no resolvedPathBackfill block.

3. PR body

What master actually does [A] (checked in origin/master:cmd/server/store.go):

  • The server's relay-credit indexes (indexResolvedPathHops → byPathHop → traffic share) read the persisted resolved_path only at start-up load (Load, loadChunk).
  • IngestNewFromDB and IngestNewObservations index live rows with the server's own resolvePathForObs. The code comment says so: "intentionally does NOT select resolved_path".
  • Some per-request queries read the column straight from the DB (resolved_index.go, one bucket query in db.go). They do not feed the relay indexes.

Body changes:

4. The observer as its own last hop

Why a backward-only fix is not enough [T]: the suggested seen[observer] entry in resolvePathBackward alone does not change the stored row for path ["0b"]. The forward chain resolves a unique prefix with no anchor at all, so it already returns [obs], and the backward walk never runs.

Fix:

  • Both chains exclude the observer from the last hop of a flood path:
    • forward, through resolvePathForward(…, notLast), which resolvePathWithContext calls with "", so its other callers are unchanged;
    • backward, through a seen[observer] entry that is removed after the last hop.
  • DIRECT routes are unchanged: there the observer is the next hop, at the front of the path (Mesh.cpp:89).
  • The observer stays a valid candidate for earlier hops. Dispatcher.cpp:238 logRx runs before de-duplication, so an observer can report the echo of a flood it forwarded itself. …_ObserverMayBeAnEarlierHop_188 pins this.

Red before, green after [T]. On e610dbb8, before the fix:

--- FAIL: TestObserverAnchor_ObserverIsNeverItsOwnLastHop_188
    hop 0 = "0b5e…01", want "" (path [0b5e])

It passes from 503ea758.

Perf [T]. The resolver is the InsertTransmission hot path, and the PR's benchmark was not in the repo, so I added BenchmarkResolveObservationPath_188: 700 relays, 60 observers, 1–5 hop 1-byte flood paths built as graph walks; one op is one observation. Before and after were run interleaved on the same machine, n = 12 each:

ns/op (median) min–max B/op allocs/op
before 503ea758 3,875 3,745–4,161 119 5
after 3,890 3,783–3,975 119 5

That is within noise; the fix adds one map insert per flood observation.

5. Surviving mutants

Mutant Result Killed by
m9: the backward walk keeps its anchor across a nil hop caught TestObserverAnchor_BackwardWalkBreaksOnNilHop_188 (the review's [a1 b2 c3] probe)
m10: the backward walk without its exclusion set caught TestObserverAnchor_BackwardWalkExcludesResolvedHops_188
m12: the backfill passes from_pubkey for every payload type caught TestResolvedPathBackfill_FromPubkeyOnlyForAdverts_188
  • m10 is not equivalent [T]. The probe is path c3b → b2a → c3a → observer, where hop 0 repeats the prefix c3. Both c3 nodes neighbour b2a, but c3a is already hop 2. With the exclusion set, c3b is the only candidate left; without it, hop 0 stays nil. The merge's duplicate check never sees a duplicate here, so it does not mask the difference.
  • m12 has no effect on today's data [A]. buildPacketData sets from_pubkey only for ADVERTs (db.go), so non-ADVERT rows have it NULL. The test keeps the backfill identical to InsertTransmission if that ever changes.

6. Seam adjacency: assessment, no change [A]

A seam without an edge occurs only when the forward chain found no neighbour of hop i among the candidates of hop i+1, and the backward chain then picked hop i+1 from its own anchor.

A seam check would be stricter than either chain's own rule, for three reasons:

  • The graph is incomplete by design. Interior hop-to-hop edges are built only from hops of 2 or more bytes (minInteriorEdgeHashBytes), so on a 1-byte mesh most real adjacencies between relays are missing. A missing edge is not evidence of non-adjacency.
  • Each chain already stores adjacencies the graph lacks. A hop with a unique prefix resolves with no adjacency check at all (case 1 in resolveHopWithContext).
  • The backward value is no weaker at the seam. It has the same evidence as any other backward hop: a unique neighbour of its anchor. The seam adds no contradiction; identity conflicts and duplicates already drop the whole backward result.

A seam check would mostly discard correct backward results on 1-byte meshes. I recommend leaving it as is and, if wanted, counting such rows on staging later.

7. ensureResolvedPathBackfillState and RunAsyncMigration: assessment, no change [A]

  • writerMu. writerMu is a Go mutex; SQLite sees a single connection (MaxOpenConns(1)). A CREATE TABLE IF NOT EXISTS outside writerMu waits for that connection in the pool, so it cannot cause SQLITE_BUSY against the ingestor's own writes. It runs once per pass, after the readiness check, and costs microseconds. The only loss is that it does not appear in the /api/perf writer timings. It could be wrapped for consistency, but that is cosmetic.

  • RunAsyncMigration. It does not fit this backfill:

    • It is run-once: status done is never run again (async_migration.go). This backfill is designed as one pass per start, from a watermark.
    • It is scheduled in OpenStore, before any neighbour graph exists, while this pass must wait for the first edge build.

    Registering it would need a per-start name or a status reset, which bends the helper's semantics. The batch writes themselves already go through WriterTx (writer timings, component resolved_path_backfill). Progress shows only in the logs, as before. A status entry for /api/perf would be a possible follow-up.

8. Comments [A]

Checked against MeshCore a366955, the version cloned here. 0679dbe, which the PR cites, is not in that clone.

Reference Location
routeRecvPacket Mesh.cpp:344-356, with copyHashTo at :349
removeSelfFromPath :334-342
front-of-path match :89
SNR append :60-61
sendFlood refuses TRACE :638
logRx Dispatcher.cpp:238, before processRecvPacket at :246/:256

These are now in the path_resolver.go comment and the PR body.

The TRACE comment now says what the ingestor actually does. The header path carries SNR bytes, but decoder.go replaces the hops with the planned route from the payload. TRACE is not anchored because it is always DIRECT.

Mutants (this follow-up)

Each mutant was applied to a copy of the tree and run against the affected tests (-run '188|ResolvedPath|NeighborEdges|NeighborGraph|PrefixIndex|Resolve'). All 14 were caught [T]. One more test failed in every copy, TestConfigExample_ResolvedPathBackfill_188: the copy had no config.example.json. That failure is not counted.

Mutant Caught by
f1a: no post-build refresh after the warm-up …_UsesGraphFromFirstEdgeBuild_188, …_EmptyGraphKeepsWatermark_188, …_StartWaitsForFirstEdgeBuild_188 (whole suite)
f1b: readiness ignores "post-build" TestResolvedPathBackfillReady_188, …_StartWaitsForFirstEdgeBuild_188
f1c: readiness ignores an empty graph TestResolvedPathBackfillReady_188, …_EmptyGraphKeepsWatermark_188
f1d: readiness ignores an empty index TestResolvedPathBackfillReady_188
f1e: no per-batch guard …_EmptyGraphKeepsWatermark_188
f1f: the background pass does not wait …_StartWaitsForFirstEdgeBuild_188
f1g: a tick never publishes a post-build graph TestNeighborEdgesBuilder_TickPublishesBuiltGraphAfterFailedWarmUp_188 (survived until that test was added)
f4a: no last-hop exclusion in the forward chain …_ObserverIsNeverItsOwnLastHop_188
f4b: no last-hop exclusion in the backward walk …_ObserverIsNeverItsOwnLastHop_188
f4c: the backward walk excludes the observer at every position …_ObserverMayBeAnEarlierHop_188
f4d: the exclusion is also applied to DIRECT routes …_ObserverIsNeverItsOwnLastHop_188
m9, m10, m12 see finding 5

Local runs

Run on 2e2886af [T]:

Command Result
cd cmd/ingestor && go test -count=1 ./... ok (548 s)
cd cmd/server && go test -count=1 ./... ok (314 s)
go test -race -count=3 -timeout 90m -run '188|ResolvedPath|NeighborEdges|NeighborGraph|PrefixIndex|Resolve' ok (705 s), no race reported
go vet ./... (ingestor and server) clean
gofmt -l on every changed Go file clean
sh test-all.sh all passed

The first -race -count=3 attempt hit go test's default 10-minute timeout inside the existing TestNeighborEdgesBuilderDeltaScan, with no race and no failure. The re-run with -timeout 90m passed.

CI (head 2e2886a)

Run 37129803920, pull_request event [T]:

Job Result Time
Go Build & Test success 21 min
Playwright E2E Tests success 19 min
Build & Publish Docker Image success <1 min
Release Artifacts skipped (PR) –
Deploy Staging skipped (PR) –
Publish Badges & Summary skipped (PR) –

Not verified

  • [K] Staging behaviour after deploy: the sum settling, the backfill's duration and hold time on the real DB. None of these changes alter what the review measured there.
  • [A] The tests show that the backfill waits for the post-build graph, but on a mesh where no build ever yields an edge, it waits indefinitely. It logs one "waiting" line, and cancelling it on shutdown is covered by the stop function; there is no separate test of that indefinite wait.
  • New observation, not changed [A]. Neither the observer anchor nor the neighbour builder checks observations.direction, while client_reception.go (deriveHeardKey) requires rx.
    • If an uploader sends tx observations of floods its node forwarded, the last hop of those paths is the observer itself.
    • With finding 4 that hop is never resolved to the observer itself. But if exactly one neighbour of the observer shares the observer's prefix, the backward walk picks that neighbour and anchors earlier hops on it: a mis-attribution.
    • I did not check whether any uploader sends tx. It could be a small follow-up: anchor only rx, or rows with no direction.
  • [A] Anomalous FLOOD-routed TRACE packets get their hops replaced with the planned route (decoder.go), and the anchor would treat them as a flood. The firmware never sends one (sendFlood refuses TRACE). Not changed.
  • [K] feat(ingestor): resolve the last hop from the observer, relay-only prefix index, backfill NULL resolved_path #188 point 3 (the start-up window in main.go) stays with the original author.

dborup and others added 5 commits October 3, 2026 15:12
…h few edges (#188)

PR #190, second review, P2-1. The warm-up loop in StartNeighborEdgesBuilder
stops once buildAndPersistNeighborEdges returns fewer than
neighborBuilderMaxBatch edges, but each call reads up to that many
observations. A full batch that yields few edges ends the warm-up with
observations left unscanned. The backfill then runs on that partial graph
and moves its watermark past rows it could not resolve.

Two tests, red by assertion:
- one full batch (50,000 rows) with a single edge, and the edge the rows
  need in the second batch: the warm-up stops early;
- a full batch with no edge newer than the watermark, so the build cannot
  move past it: the backfill runs anyway, because the graph holds an older
  edge.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QbNdgonR8kq7SPEU7LcmXD
…188)

PR #190, second review, P2-1. buildAndPersistNeighborEdges reads at most
neighborBuilderMaxBatch observations per call, but the warm-up loop
stopped once a call returned fewer edges than that. A full batch with few
edges ended the warm-up with observations unscanned, and the backfill then
ran on that partial graph.

- buildNeighborEdges reports both the edge upserts and the observation
  rows read; buildAndPersistNeighborEdges keeps its old result for
  existing callers.
- The warm-up continues while a call reads a full batch. It stops when a
  call reads fewer rows: it has caught up.
- A full batch with no edge cannot move the watermark (MAX(last_seen)), and
  the next call would read the same rows. The warm-up then stops, logs it,
  and leaves the graph unpublished.
- Only a build that caught up publishes a post-build graph, in the
  warm-up and in a tick. The backfill therefore never runs on a graph the
  builder has not finished, and waits instead.
- Comments, config.example.json and the guide now say "caught up".

TestNeighborEdgesBuilder_WarmUpScansPastFullBatchWithFewEdges_190 and
TestResolvedPathBackfill_WaitsWhileEdgeBuildCannotCatchUp_190 (red in
e369ae6) pass. The second one now also lets ticks run.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QbNdgonR8kq7SPEU7LcmXD
PR #190, second review, P2-2. The neighbour builder adds an
observer <-> last-hop edge for every observation, whatever its route type.
For a DIRECT packet the path is the remaining planned route: forwarders
strip themselves from the front (Mesh.cpp:89, removeSelfFromPath
:334-342), and only flood forwarders append their hash (routeRecvPacket,
:346-350). The last hop of a DIRECT path is therefore the route's far end,
not the node the observer heard.

TestNeighborEdgesBuilder_ObserverEdgeOnlyForFloodRoutes_190 mixes flood,
transport flood, DIRECT and TRANSPORT_DIRECT observations. It is red by
assertion: there are edges from DIRECT paths, and the flood resolver
leaves an ambiguous last hop nil because of the false edge.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QbNdgonR8kq7SPEU7LcmXD
…#188)

PR #190, second review, P2-2. On a DIRECT route the path is the remaining
planned route:
- each forwarder matches itself at the front and strips itself
  (MeshCore a366955, Mesh.cpp:89, removeSelfFromPath :334-342);
- only flood forwarders append their hash (routeRecvPacket :346-350).

So the last hop of a DIRECT path is the far end of the route, not the node
the observer heard. The edge it produced was false, and the flood
resolver relies on those edges to pick a unique candidate.

The builder now reads transmissions.route_type and adds the observer edge
only for TRANSPORT_FLOOD (0) and FLOOD (1). An unknown route type (NULL) is
skipped as well. Other edges are unchanged: the originator<->first-hop
edge of ADVERTs, and the interior hop-to-hop edges. Consecutive hops of a
DIRECT route are neighbours.

This changes neighbor_edges for every consumer: the Neighbors panel,
bridge nodes and the estimates built on them. The server reads the table
and has no other production edge source; its extractEdgesFromObs has the
same rule but no caller outside tests. DIRECT edges already persisted stay
until the neighbour-edge prune drops them (5 days by default).

TestNeighborEdgesBuilder_ObserverEdgeOnlyForFloodRoutes_190 (red in the
previous commit) passes.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QbNdgonR8kq7SPEU7LcmXD
In the P2-2 test the TRANSPORT_FLOOD row used the same path as the FLOOD
row, so a builder that dropped route type 0 still passed. The row now
names its own relay (a1a), and the test expects both observer edges.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QbNdgonR8kq7SPEU7LcmXD
@dborup-agent

Copy link
Copy Markdown
Collaborator

Rapport — CS-pve-agent1 PR#190 runde 2b — head 0f4c167

Status: P2-1 and P2-2 are fixed, with tests that were red before and green after; all 8 mutants are caught. Master (with #182) is merged in. CI is green on 0f4c167a.

This round addresses the two P2 findings in the review of 2e2886af.

Push: I built on 2e2886af. git ls-remote showed that head before work started and right before the push. The push was a fast-forward to 0f4c167a: no rebase, no amend, no force push.

Merge with master: I first merged origin/master (6f7a5e7c) into the branch with a merge commit, 486aa9ea. It brings in #182 (9d29daec), #191, #194 and #196. The merge was clean, and the targeted tests passed on the merged tree before any further change [T].

Evidence tags:

  • [T] a test, mutant or CI run;
  • [A] analysis of code or firmware.

Findings

Finding Result Commits Tests Evidence
P2-1: the warm-up ends on edges, not on rows scanned, so the backfill can run on a partial graph Fixed e369ae69 (red), da0494c4 (fix) TestNeighborEdgesBuilder_WarmUpScansPastFullBatchWithFewEdges_190, TestResolvedPathBackfill_WaitsWhileEdgeBuildCannotCatchUp_190 [T] red before, green after; 4 of 4 mutants caught
P2-2: observer↔last-hop edges come from DIRECT paths Fixed addccb88 (red), 194512d2 (fix), 0f4c167a (test hardening) TestNeighborEdgesBuilder_ObserverEdgeOnlyForFloodRoutes_190 [T] red before, green after; 4 of 4 mutants caught; [A] firmware

P2-1: warm-up completion

Change:

  • buildNeighborEdges now reports the edge upserts and the observation rows read. buildAndPersistNeighborEdges keeps its old result for existing callers.
  • The warm-up continues while a call reads a full batch (neighborBuilderMaxBatch, 50,000). A call that reads fewer rows has caught up, and only then is a post-build graph published.
  • Termination [A]: every row a call reads is newer than the builder's watermark (MAX(neighbor_edges.last_seen)). So any call that persists an edge moves the watermark forward, and the loop ends.
    • A full batch with no edge cannot move the watermark, and the next call would read the same rows.
    • The warm-up then stops, logs it, and does not publish a post-build graph.
  • The same rule applies to the ticks: only a tick that caught up publishes a post-build graph.
  • As a result, the backfill never runs on a graph the builder has not finished. It waits, as the readiness guard from round 1 already makes it do.

Red before, green after [T]. Both tests use a real 50,000-row batch, since neighborBuilderMaxBatch is a const. On e369ae69, with the round-1 code:

--- FAIL: TestNeighborEdgesBuilder_WarmUpScansPastFullBatchWithFewEdges_190
    the warm-up stopped after a full batch with one edge: no observer<->c3a edge from the second batch
--- FAIL: TestResolvedPathBackfill_WaitsWhileEdgeBuildCannotCatchUp_190
    the pass ran although the edge build has not caught up with the observations

Both pass from da0494c4.

  • First test: batch 1 holds 49,999 ambiguous rows plus one row that yields an edge; the edge the rows need is in batch 2. After the fix, the warm-up reaches batch 2 and the backfill resolves the rows.
  • Second test: a full batch yields no edge newer than an older persisted edge, so the graph is not empty. It also lets the builder tick every 100 ms for 2 s, and checks that no tick publishes a post-build graph.

Mutants [T]. Each was applied to a copy of the tree and run against the affected tests:

Mutant Caught by
The loop stops on edges < cap (the old rule) both P2-1 tests
The warm-up publishes a post-build graph when stuck …_WaitsWhileEdgeBuildCannotCatchUp_190
A tick publishes a post-build graph without catching up …_WaitsWhileEdgeBuildCannotCatchUp_190
caughtUp uses <= (always true) both P2-1 tests

P2-2: observer edges only for flood routes

Firmware [A], MeshCore a366955:

Function Location Behaviour
Mesh::routeRecvPacket Mesh.cpp:344-356 appends the forwarder's hash only when packet->isRouteFlood() (:346), via self_id.copyHashTo(&packet->path[n * …]) at :349
DIRECT front-of-path match Mesh.cpp:89 a forwarder matches itself at the front of the path (self_id.isHashMatch(pkt->path, …))
removeSelfFromPath :95 and :103; defined at :334-342 before retransmitting, the forwarder shifts the path left by one entry
isRouteFlood() Packet.h:64 route types 0 and 1

A DIRECT path is therefore the remaining planned route, and its last entry is the far end of the route, not the node the observer heard.

Change: the builder now selects COALESCE(t.route_type, -1) and adds the observer↔last-hop edge only when isFloodRoute(routeType). A NULL route type is skipped too. Two other edge kinds are unchanged:

  • the ADVERT originator↔first-hop edge;
  • the interior hop-to-hop edges, since consecutive hops of a DIRECT route are neighbours. The test pins this.

Red before, green after [T]. TestNeighborEdgesBuilder_ObserverEdgeOnlyForFloodRoutes_190 mixes FLOOD, TRANSPORT_FLOOD, DIRECT and TRANSPORT_DIRECT observations from one observer. On addccb88:

observer<->c366 edge built from a DIRECT path
observer<->b233 edge built from a DIRECT path
hop 0 = "", want "c355…aa" (path [nil])

The last line is the review's concern made concrete: the false DIRECT edge to c3b leaves the flood resolver with two neighbour candidates, so the ambiguous last hop c3 is not resolved. The test passes from 194512d2.

0f4c167a gives the TRANSPORT_FLOOD row its own relay. Before that, a mutant that dropped route type 0 still passed.

Mutants [T]:

Mutant Caught by
No route-type gate (the old behaviour) …_ObserverEdgeOnlyForFloodRoutes_190
Only route type 2 excluded (TRANSPORT_DIRECT allowed) …_ObserverEdgeOnlyForFloodRoutes_190
Only route type 1 allowed (TRANSPORT_FLOOD dropped) …_ObserverEdgeOnlyForFloodRoutes_190, TestNeighborEdgesBuilderDeltaScan
The gate also drops interior edges of DIRECT routes …_ObserverEdgeOnlyForFloodRoutes_190

Effect beyond this PR [A]. The PR body now states it:

  • The fix changes neighbor_edges for every consumer: the Neighbors panel, bridge nodes and the estimates built on the graph.
  • The server reads the table; its own extractEdgesFromObs follows the same old rule but has no caller outside tests.
  • DIRECT-derived edges already persisted stay until the neighbour-edge prune removes them (NeighborEdgesDaysOrDefault, 5 days by default).

PR body

Local runs (0f4c167a) [T]

Command Result
cd cmd/ingestor && go test -count=1 ./... ok (541 s)
cd cmd/server && go test -count=1 ./... ok (348 s)
go test -race -count=3 -timeout 90m -run '_190$|188|ResolvedPath|NeighborEdges|NeighborGraph|PrefixIndex|Resolve' ok (1,211 s), no race reported
go vet ./... (ingestor and server) clean
gofmt -l on the Go files changed in this round clean
sh test-all.sh 201 passed, 0 failed

On the whole merged diff, gofmt -l flags only cmd/ingestor/source_status.go, which comes from master unchanged. No new interface{} was added in cmd/ingestor, and cmd/server is untouched by this PR.

CI (head 0f4c167)

Run 37135018260, pull_request event [T]:

Job Result Time
Go Build & Test success 22 min
Playwright E2E Tests success 22 min
Build & Publish Docker Image success 1 min
Release Artifacts skipped (PR) –
Deploy Staging skipped (PR) –
Publish Badges & Summary skipped (PR) –

Not verified

  • [A] A builder that cannot catch up waits indefinitely. Its warm-up logs one line; ticks stay stuck silently, which is how the builder behaved before. The backfill then never runs. That is deliberate: no watermark moves on a partial graph. A builder whose 50,000-row batch yields no edge would need its own fix, for example a persisted scan watermark; that is outside this PR.
  • [A] More full batches without edges. Excluding DIRECT paths from observer edges means a backlog dominated by DIRECT traffic yields fewer edges per batch, so that case becomes slightly more likely. I have not measured its frequency on staging.
  • Staging (left to the lead or the developer): the new neighbor_edges contents, the Neighbors panel and bridge counts after the DIRECT edges age out, and the backfill's run time.
  • [A] Carried over from round 1, unchanged:

dborup commented Oct 4, 2026

Copy link
Copy Markdown
Owner

Review — CS-cloud PR#190 runde 2b — head 0f4c167

Dom: APPROVE med nits

Independent, read-only review of head 0f4c167a91dedf3e8a2339c93aabe1afc7621f37. git ls-remote showed this head before and after the review. I did not write any of this PR. I read #188, #184, the PR body and the four review/report comments in order. The firmware was cloned at MeshCore a366955.

origin/master (6f7a5e7c) is an ancestor of the head, and git merge-tree --write-tree origin/master <head> gives exactly the head's tree (d002721a). So the merged tree and the head are the same, and everything below was run on one git archive of the head. cmd/server and internal/ are byte-identical to master.

Evidence tags: [T] test, benchmark or CI; [A] analysis of code or firmware; [K] taken from earlier comments and not run again.

Findings

No blocking finding. All priorities are low.

# Priority Where Finding
1 Low (test gap) cmd/ingestor/neighbor_builder.go:138 The tick's err == nil && guard before publishing a post-build graph is not pinned. Mutant X1 drops it, and every targeted test still passes. After a failed tick, b.scanned is 0, so caughtUp() is true and the graph would be marked "built". That is the P2-1 situation (backfill on a graph whose build did not complete) reached through an error instead of a full batch. The code is correct; a test that makes one tick fail (for example a closed DB or a broken nodes table) while the warm-up is stuck would pin it. [T]
2 Low (test gap) cmd/ingestor/neighbor_builder.go:290 The PR text says "a NULL route type is skipped too". Mutant X3 (isFloodRoute(routeType) || routeType < 0) survives the targeted tests, so nothing pins it. One extra row with route_type = NULL in …_ObserverEdgeOnlyForFloodRoutes_190 would. [T]
3 Low (test gap) cmd/ingestor/path_resolver.go:358 The backward chain excludes fromPubkey from its candidates. Mutant X4 removes that and survives. fromPubkey is only set for ADVERTs, and the forward chain anchored on it usually fills the hop first. But when it leaves hop 0 nil, the backward walk could resolve it to the originator, which is never a hop of its own flood path. A flood-ADVERT case where the originator shares hop 0's prefix and neighbours hop 1 would pin it. [T][A]
4 Low (docs) PR body, section 2b Two P2-2 effects on neighbor_edges consumers are not mentioned (see point 2 below). (a) The observer-detail "confirmed / not seen yet" column (SeenViaPackets, db.go packetGraphNeighbors) can flip for neighbours that only had a DIRECT-derived edge. (b) An edge pair that also gets real flood evidence keeps refreshing last_seen, so the count it accumulated from DIRECT observations never ages out with the 5-day prune. That count is read by GPS sanity and by the "nearest positioned neighbours" ranking (ORDER BY count DESC). Both are small and in the right direction; a sentence in the PR body is enough. [A]
5 Info (perf) resolveObservationPath Confirmed in order of magnitude, larger factor here (point 6). [T]
6 Info (deploy) backfill, first start A first pass covers every observation id up to MAX(id), at most about 2,000 rows/s by default. On a 5 GB DB that is likely hours (point 6). The server only sees the results after its next restart. Worth a line in the v0.2.1 deploy note. [A]

1. Earlier findings: are they really fixed?

Finding Verdict Basis
MacBook 1: backfill on a pre-build, empty or partial graph Fixed. resolvedPathBackfillReady requires all three: built (set only by storeBuilt), a non-empty prefix index, and a graph with at least one edge. It is checked when the pass starts and again for every batch (resolvedPathBackfillBatch), on the same snapshot the batch then uses. A batch that is not ready returns before reading rows and writes nothing, not even the watermark. The watermark and the UPDATE … WHERE resolved_path IS NULL commit in one WriterTx. The background pass takes the wake-up channel before checking readiness, so a graph published in between is not missed. Gap: finding 1 (error path). [A]; X1 [T]; authors' f1a–f1g mutants [K]
MacBook 2: config keys Fixed. config.example.json has resolvedPathBackfill with disabled, batchSize and pauseMs plus a _comment. 0 means default, so the pause cannot be turned off. [T] (parsed the file on the head)
MacBook 3: dependency on #182 Fixed. The PR body now describes master as it is (#182 merged, live and Load both read the persisted column). [A]
MacBook 4: observer as its own last hop Fixed in both chains. Forward via resolvePathForward(…, notLast). Backward via seen[observer], removed after the last hop unless the observer was already excluded as fromPubkey. My mutant X2 (observer not lower-cased) is killed by …_ObserverIsNeverItsOwnLastHop_188. [A]; X2 [T]
MacBook 5: surviving mutants m9, m10, m12 Fixed. The three tests exist and pass. [T] (pass); kill status [K]
MacBook 8: TRACE comment and line numbers Fixed. At a366955: front-of-path match Mesh.cpp:89, removeSelfFromPath :334-342, routeRecvPacket :344-356 with copyHashTo at :349, logRx Dispatcher.cpp:238 before processRecvPacket. All match the comment. [A]
P2-1: warm-up stops on rows scanned, backfill waits Fixed. caughtUp() is scanned < neighborBuilderMaxBatch. A full batch with zero edges stops the warm-up without publishing. Ticks publish a post-build graph only after err == nil && caughtUp(). While nothing is published the backfill waits on the channel. Termination holds: every row read is newer than MAX(last_seen), so any persisted edge moves the watermark. [A]; tests pass [T]
P2-2: observer↔last-hop edge only for flood Fixed and correct against the firmware. routeRecvPacket appends the forwarder's hash only if isRouteFlood() (Packet.h:64: types 0 and 1). A DIRECT forwarder matches itself at the front and shifts the path left (Mesh.cpp:89, 95/103, 334-342), so a DIRECT path's last entry is the far end of the planned route. The builder selects COALESCE(t.route_type, -1) and gates only the observer edge. ADVERT-origin and interior edges are unchanged. [A]; finding 2 [T]

2. Effect of P2-2 on other neighbor_edges consumers

[A] The server reads neighbor_edges (table and in-memory graph) and does not derive edges itself: extractEdgesFromObs has no non-test caller (grep). Removing the DIRECT observer↔far-end edges deletes false RF adjacencies, so every consumer moves towards correct values. None of them has a code path that breaks on missing edges.

  • Bridge nodes, usefulness axes (bridge, coverage, redundancy), Reach: fewer spurious long-range edges, so observers and route endpoints that only had DIRECT edges lose some bridge, coverage or reach credit.
  • GPS sanity: false observer↔far-end edges inflated distances and could flag correct positions as suspicious; fewer false flags now. A node whose only edges were DIRECT-derived may drop to "not evaluated".
  • Ping score and position estimates (nearestPositionedNeighborsBulk): fewer neighbours to estimate from, so some nodes lose an approximate position.
  • Path Inspector: fewer wrong adjacency candidates.
  • Neighbors API / panel: fewer neighbours shown. Also observer-detail SeenViaPackets (finding 4a).

The PR body names the Neighbors panel, bridge nodes and graph estimates, and says the old edges stay until the 5-day neighbour prune (NeighborEdgesDaysOrDefault). So during the first 5 days after deploy, consumers see old and new edges mixed. Not mentioned: finding 4 (observer-detail flag, and count that does not decay).

3. Interaction with #182

[A]

  • InsertTransmission writes resolved_path in the same insert as the observation. The server's live poll (IngestNewFromDB/IngestNewObservations, fix(store): index live observations from the persisted resolved_path, as Load does (#158) #182) and Load therefore read the same value for every row written by the new resolver. Live and Load stay consistent, and the extra non-NULL rows simply give relay credit on both paths.
  • The one source of difference is the backfill. It changes rows the server may already have polled as NULL, which gave byNode credit only. Those rows reach byPathHop and traffic share only at the server's next Load. The traffic-share sum therefore steps up at the server restart after the backfill, as the PR body and the backfill header say.
  • Nothing in cmd/server needs to change for correctness: cmd/server is untouched (0 files in the diff) and stays read-only. A server-side re-read of backfilled rows would be a follow-up, not a fix.

4. Resolver correctness

[A], with tests [T]:

  • Backward walk only for flood: resolveObservationPath returns the forward chain unchanged for any route type other than 0/1. It also skips the backward walk when there is no graph, no index, no observer, or no nil hop.
  • Exactly one candidate: both chains call resolveHopWithContext. Two or more candidates need an anchor and exactly one adjacent survivor. An excluded candidate is skipped before counting, and a single candidate that is excluded gives nil, so an exclusion never turns an ambiguous prefix into a "unique" one.
  • Merge rule: forward wins. A different value at any position, or one node at two positions, drops the whole backward result and returns the forward chain unchanged. The 2,000-random-graph test passes.
  • Observer never its own last hop: see point 1.
  • Relay-role filter: isRelayRole is character for character the server's canAppearInPath (cmd/server/store.go:7284-7287).
  • Backfill inputs match InsertTransmission: observer = observers.id via observer_idx, route type and from_pubkey only for ADVERTs.

5. Tests and mutants

On the head (= merged tree):

Run Result
cd cmd/ingestor && go test -count=1 ./... ok (139 s) [T]
go test -race -count=3 -run '_190$|_188$|ResolvedPath|ObserverAnchor|PrefixIndex' . (ingestor) ok (691 s), no race [T]
cd cmd/server && go test -count=1 ./... first run: 1 failure, TestHandleNodePaths_PrefixCollision_1352 with 503 "index loading"; second run ok (60.5 s) [T]
sh test-all.sh 201 passed, 0 failed (rc 0) [T]
CI on the head Go Build & Test, Playwright E2E and Docker build: success; release, badges and staging deploy: skipped (PR) [T]

The _1352 failure is the known pre-existing flake, not this PR: cmd/server is identical to master. Reproduced with the same counts on both:

-run '_1352$' (4 tests) master 6f7a5e7c head
default GOMAXPROCS, -count=30 0 failures 0 failures
-cpu 1, -count=10 39 failures 40 failures

[T]

Own mutants (in addition to the authors'), each in a copy of the head and run with -run '_188$|_190$|ResolvedPath|ObserverAnchor|PrefixIndex|ResolveHop|NeighborGraph|ObserverEdge|TestNeighborEdgesBuilder_':

Mutant Result
X1 tick publishes a post-build graph even when the build errored survives (finding 1)
X2 observer id not lower-cased in resolveObservationPath killed by TestObserverAnchor_ObserverIsNeverItsOwnLastHop_188
X3 NULL route type gets the observer edge survives (finding 2)
X4 backward chain does not exclude fromPubkey survives (finding 3)
X5 backfill ignores the row's route type (treats every row as DIRECT) killed by 10 tests, including …_ResolvesRowsIngestedBeforePriming_188, …_UsesGraphFromFirstEdgeBuild_188, …_WarmUpScansPastFullBatchWithFewEdges_190

[T]

6. Performance and operations

  • Resolver cost [T]: I added a forward-only twin of BenchmarkResolveObservationPath_188 (same fixture: 700 relays, 60 observers, 1–5-hop 1-byte flood paths) in a scratch copy and ran both interleaved, n=6, on a linux/amd64 container (Xeon 2.1 GHz, 4 vCPU, load average under 1):

    • forward only: median 623 ns [612–657], 31 B/op, 1 alloc;
    • resolveObservationPath: median 5,640 ns [5,358–5,848], 119 B/op, 5 allocs.

    That is about +5 µs per flood observation on this worst-case fixture (every row is 1-byte and leaves a nil hop, so every row runs the backward walk). The PR's 757 → 2,345 ns is on a staging sample where many rows do not take the backward path. Same order of magnitude: microseconds against a millisecond insert. At staging's ~4–5 observations/s that is well under 0.1 ms of CPU per second.

  • Backfill vs. live ingest [A]: each batch reads by primary key without writerMu, resolves without a lock, and then holds the write transaction only for at most 500 single-row UPDATEs plus the watermark. That is milliseconds per 250 ms pause. MacBook's end-to-end measurement (live InsertTransmission p99 1.2 ms during a default backfill) [K] fits that. I did not re-measure.

  • First start on a 5 GB DB [A]:

    • Pass size: the pass covers (watermark, MAX(observations.id)], so on first start that is every stored observation, not only the NULL ones. At ≤2,000 rows/s, 10–30 M rows take about 1.5–4 h. It resumes from the persisted watermark after a restart.
    • Prerequisite: it starts only after the neighbour-edge warm-up has caught up. With a persisted neighbor_edges (staging) that is quick.
    • Risk: if neighbor_edges is empty or far behind (for example a restored copy), the warm-up scans from its old watermark in 50,000-row batches. If one full batch yields no edge, the backfill waits indefinitely, by design. The PR documents this; it logs "initial build cannot move past its watermark".
    • Deploy note: check for the [resolved_path_backfill] starting and done lines. Plan a server restart after done, then read the traffic-share sum; a rising sum before that is the transition, not drift.

7. Rules

Rule Result
All writes in cmd/ingestor; cmd/server read-only yes: cmd/server and internal/ unchanged; no new INSERT/UPDATE/DELETE outside cmd/ingestor [T]
No new map[string]interface{} 0 added lines [T]
config.example.json has the new keys yes [T]
Fork guards 9 × github.repository == 'Kpa-clawbot/CoreScope' on the head and on master [T]
No closing keywords in title or body none found (title: "feat(ingestor): resolve the last hop from the observer (#188)") [T]

Not verified

  • [K] Staging after deploy: the sum settling near the estimate, the backfill's real duration and lock-hold time on the staging host, and the Neighbors/bridge/GPS-sanity numbers after the DIRECT edges age out.
  • [K] The authors' mutant tables and the replay numbers; I did not rerun them.
  • [A] The real number of observation rows in the 5 GB DB; the duration above is an estimate.
  • Not run: the full cmd/server suite under -race, by request.

Generated by Claude Code

@dborup
dborup marked this pull request as ready for review October 4, 2026 05:21
@dborup
dborup merged commit c7798e0 into master Oct 4, 2026
6 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