mirror of
https://github.com/Kpa-clawbot/meshcore-analyzer.git
synced 2026-09-26 00:33:38 +00:00
aabeda0f2ce173f51b3b42ea5e2bfa55817e18ea
465
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 efiten
|
aabeda0f2c |
test(server): set the #1239 lock-hold threshold from measurement, 150µs to 5ms (#2039)
TestComputeAnalyticsDistanceLockHoldDuration failed on two consecutive master commits ( |
||
|
Alex B
|
e6323ec587 |
fix(store): account path, decode-cache and dedup-key bytes so maxMemoryMB eviction triggers (#2035)
trackedBytes undercounted the packet store by about 2.5x, so packetStore.maxMemoryMB never triggered: a tx was charged at creation, before pickBestObservation set its path, so the byPathHop and spTxIndex costs were never added, and eviction then re-estimated with the path known and subtracted more than had been added, drifting the total downwards. The ParsedDecoded cache, the obsKeys dedup key and several per-observation strings were not estimated at all. StoreTx.accountedBytes now records what was charged, rechargeTx returns the delta after every pickBestObservation, and eviction subtracts accountedBytes instead of re-estimating. Measured by the author on a production database copy: trackedMB 151 against 402 MB of heap in use before, 351 against 396 MB after, with GC cycles dropping from ~0.71/s to ~0.011/s over 12 h on their instance. Reviewed by auditing the accounting rather than the arithmetic: all six production pickBestObservation sites recharge, all three subtraction sites read accountedBytes, every recharge site holds s.mu (Load from :857, the two ingest paths at :2778 and :3142 with deferred unlocks), observations are charged only after acceptance, and no charged tx is discarded during the chunk merge. That lock audit is the independent check, because CI's race job covers the ingestor only. Operator impact, both from the estimate growing rather than any limit moving: where maxMemoryMB is set, eviction now caps the real store size, and the cold load clamp drops about a third of the boot walk (124420 to 84374 packets at 650 MB). Where it is unset, which is the default, the change is inert. docs/go-migration.md claimed the Go server ignored the setting, which was never true, and is corrected here. Merged by the interim maintainer without a second human reviewer: CI (run 35258839322) plus the review above are the independent checks. |
||
|
efiten
|
5430bc7923 |
test(ingestor): anchor the RF-sample fixtures to now, not to a calendar date (#2034)
The three ClientRfDeltas tests seeded 2026-08-17T10:00:00.000Z and queried that window back. resolveRxTimeCore (cmd/ingestor/main.go:1527) replaces timestamps older than 30 days with the ingest time, so from 2026-09-16T10:00Z the seeds landed at time.Now() and every delta fell outside the queried window. Master and every open PR went red on it. Fixtures now derive from a package-level base two hours in the past, computed once per test binary so two seeds cannot straddle a second boundary and break the exact WallMillis assertion. Merged by the interim maintainer without a second human reviewer: CI is the only independent check (run 35222316327, ingestor tests ok in 97.033s, race detector ok, no --- FAIL). Fixed dates elsewhere in the ingestor tests are untouched, they assert row counts rather than querying by the seeded date. |
||
|
Sylvain Rabot
|
a2ea18f778 |
perf(sqlite): swap modernc.org/sqlite for mattn/go-sqlite3, cross-built with zig (#1992)
Swaps the SQLite driver from `modernc.org/sqlite` (pure Go, SQLite
3.46.0) to `github.com/mattn/go-sqlite3` (cgo, bundled SQLite 3.53.4),
and pays the resulting cross-compilation cost with `zig cc`.
Draft because the riskiest part of this deletes rows — see [Please
review this part first](#please-review-this-part-first) — and because
three things remain unverified at the bottom.
`modernc.org/sqlite` is a transpilation of the C amalgamation. This repo
is read-heavy: `cmd/server` chunk-loads a graph at startup and fans out
neighbour/topology/analytics queries per request, and it pays for that
transpilation on exactly those paths. Head-to-head on the same
120k-transmission / 240k-observation database, running our own hot-path
SQL under both drivers (Apple M4, `-count=5`, medians):
| workload | modernc | mattn | |
|---|---:|---:|---|
| chunk load (`chunked_load.go` v3 join, 20k tx) | 449ms | 196ms |
**2.3×** |
| aggregate scan (240k-row join + `GROUP BY`) | 276ms | 137ms | **2.0×**
|
| 1500 prepared-statement lookups | 512ms | 403ms | **1.3×** |
Allocations fall with it: 1.12M vs 1.64M allocs and 21MB vs 30MB on the
chunk load.
**Superseded by a production run.** @efiten measured both drivers on a
real instance — 11,077,038 observations, 9.7GB database, 4-core arm64 —
as server-only containers against the same live volume, one at a time,
with round 2 reversing the order so the page cache favours the old
driver:
| | audit 7d | audit 24h | background fill (13 chunks) | start →
/api/health |
|---|---:|---:|---:|---:|
| modernc, round 1 | 16.67s | 2.27s | 130.2s | 16.6s |
| mattn, round 1 | 7.87s | 1.34s | 93.8s | 13.5s |
| mattn, round 2 | 8.15s | 1.35s | 96.4s | 13.0s |
| modernc, round 2 | 13.46s | 2.29s | 137.8s | 15.5s |
Warm, the old driver improves to 13.46s on the 7d audit and still loses
by ~1.8×. Chunk load is ~1.4×. `/api/nodes?limit=500` is 0.039s against
0.037s — nothing.
**So the real gain is ~1.4–1.8× on the paths that matter, not 2–2.3×.**
The shape the harness predicted holds — scans and joins gain, small
lookups do not — which is more reassuring than the magnitude would have
been. Quote these numbers.
**The counterweight**, cold and native on that machine: a build goes
from **52s to 163s**. An instance that builds its own image pays that
per deploy.
## The build is cgo now, and one thing about that is a trap
**`CGO_ENABLED=0` still builds.** mattn links a stub, and the binary
dies on its first query with `go-sqlite3 requires cgo to work. This is a
stub`. A green build is not evidence of anything here, which is why
`AGENTS.md` now says so explicitly. `GOOS=linux go build` genuinely
cannot cross-compile any more.
A new root `Makefile` is the entry point. `make crossbuild` uses `zig cc
-target {x86_64,aarch64}-linux-musl` and links static, so each artifact
stays a single self-contained file and the `alpine:3.20` runtime no
longer depends on the base image's libc at all.
`-Wl,-s` is load-bearing: Go's own `-s -w` does not reach the musl
objects zig links in, and without it the server binary is 19.8MB instead
of 12.1MB.
The Dockerfile keeps its single `$BUILDPLATFORM` builder — still no QEMU
for compilation — and gains a checksum-pinned zig plus BuildKit cache
mounts. The mounts are not a nicety: without them an image build
recompiles the amalgamation from cold and takes over half an hour.
## Please review this part first
`internal/dbschema/dedup_index.go` **deletes observation rows**. It is
the one part of this change that can lose data, and it exists because
the migration exposed a real bug rather than causing one.
`stmtInsertObservation` resolves its `ON CONFLICT` against
`idx_observations_dedup`, which `cmd/ingestor/db.go` only ever created
inside the branch that creates the `observations` table for the first
time. Any database whose table predates that branch never got one, so
the UPSERT had no conflict target. modernc failed on the first insert;
mattn fails at `OpenStore`. Same bug, found earlier.
Creating the index unconditionally repairs it — but the index is what
was supposed to prevent duplicates, so a database that never had it can
already hold rows violating it. **`test-fixtures/e2e-fixture.db` in this
repo holds one.** So duplicates are collapsed first. Refusing is not the
safer option: without the index the ingestor cannot prepare its UPSERT,
so it cannot start at all.
Replaying that UPSERT faithfully is subtler than it looks, and a first
cut of this got it wrong twice:
- `COALESCE(excluded.x, x)` means the **incoming** value wins, so down a
group in id order the survivor keeps the **last** non-NULL value. Taking
the first silently discarded newer readings.
- The UPSERT names exactly five columns (`snr`, `rssi`, `score`,
`raw_hex`, `resolved_path`). Every other column must keep the surviving
row's own value; merging those too invents history the ingestor would
never have written.
Merge, delete and `CREATE UNIQUE INDEX` now share one transaction. Split
apart, a writer inserting a duplicate in the gap fails the index
creation while leaving the deletions committed — rows destroyed and no
index to show for it.
Cost, measured on 2.4M synthetic rows holding 5 duplicates: **4.1s**,
holding the write lock throughout, once, at ingestor startup before MQTT
subscribe. Materialising the duplicate-group scan once rather than per
column took that from 9.7s; the pathological case (400k of 600k rows
duplicated) is 5.7s, slightly worse than the 4.2s it was before that
change.
## Four more behavioural differences
Full detail in `docs/sqlite-driver-migration.md`. Briefly:
**Statement preparation is eager.** modernc's `newStmt` stored the SQL
and compiled lazily; mattn calls `sqlite3_prepare_v2` inside `Prepare`,
so SQL naming a missing table fails at *open*. 59 server tests failed on
this alone, all fixtures with partial schemas. `OpenDB` keeps failing
loudly (#1901; `main.go` gates on `dbschema.AssertReady` anyway) and the
fixtures now declare what they are prepared against via
`ensurePreparable`. This also exposed nine `nodes(pubkey …)`
declarations across seven files, where production has only ever had
`public_key` — lazy compilation had hidden the mismatch for as long as
it existed.
**`synchronous` silently dropped FULL → NORMAL.** mattn defaults it to
NORMAL and executes the pragma unconditionally, where SQLite's own
default (what modernc left alone) is FULL. In WAL mode that weakens
durability under power loss. Pinned in `dbschema.WriterDSN`, which both
writers now share — `cmd/migrate` kept a bare path at first and so
quietly wrote at NORMAL, which is what a second copy of a DSN buys you.
**The DSN dialects are mutually invisible.** modernc understood only
`_pragma=name(value)`, mattn only `_`-prefixed parameters, and neither
errors on the other's form — a driver-only rename would have dropped
every pragma in silence. `_journal_mode=WAL` is also gone from the
server's read handle: modernc ignored it, mattn honours it, and setting
`journal_mode` on a read-only connection is a write. Dropping
`_busy_timeout` with it costs nothing, since mattn already defaults to
5000ms — which means the read handle finally *gets* the busy timeout it
had silently lacked.
**`mode=ro` survives for a non-obvious reason.** mattn always passes
`READWRITE|CREATE` and its amalgamation has `SQLITE_USE_URI=0`; what
makes the URI work is its C wrapper ORing `SQLITE_OPEN_URI` in. So the
#1283/#1289 invariant holds with no build flags — but it depends on the
`file:` prefix. `cmd/decrypt` had been building its DSN without one, so
its `mode=ro` had never applied and a missing path was created
read-write. Fixed in passing; never a migration regression.
## What did not change
No modernc-specific API was in use: no `RegisterFunction`, no
`*sqlite.Conn`, no `sqlite/lib` error constants, no `sql.Register`. No
`time.Time` is ever bound as a query argument, so driver time handling
is not in play. Both drivers convert declared
`DATE`/`DATETIME`/`TIMESTAMP` columns to `time.Time`, so
`/api/dropped-packets` keeps emitting `dropped_at` as RFC3339 — an
earlier draft "fixed" that with a `CAST` and would have been the
regression.
## Tests and CI
New regression tests, each written because something got through without
it:
- `TestEnsureObservationsDedupIndexKeepsLatestValues` — the merge
ordering. The original test used complementary NULLs, which passes
whichever direction you pick, which is why the bug survived it.
- `TestCollapseDuplicatesAndIndexIsAtomic` — a failed index creation
must roll the deletions back.
- `TestOpenStorePragmas` / `TestWriterDSNPragmas` — every writer pragma,
read back through the store's own connection. A separate `sqlite3`
session or the startup log line would prove nothing.
- `TestOpenDBRefusesMissingDatabase` — the read-only invariant, which
now rests on a detail of the driver's C wrapper.
- `TestEnsurePreparableMatchesPrepareStatements` — fails when a new
prepared statement outgrows the fixture helper.
CI gains test execution for `cmd/migrate` and `internal/dbschema`, which
had none and both open the database. A PR-time two-arch build plus an
arm64 QEMU smoke gate is new: the GHCR push is push/tag-only, so without
it nothing on a PR would exercise zig, static musl linking or arm64, and
the first signal would arrive on master. `cache-dependency-path` widens
from 2 of the 5 tracked `go.sum` files to all of them.
`make test` passes across all 14 modules, `cmd/server` also under `-race
-count=2` with no failures and no races. `gofmt` and `go vet` clean.
Release-routing and Dockerfile COPY-invariant gates pass.
## Verified by running
- All 8 cross-builds static and correct-architecture; both arches of the
container image built, exported and run under QEMU, serving
`/api/health` and `/api/nodes` against a 2.9M-observation production
snapshot.
- The `migrate` binary repairing that snapshot's duplicate on bare
Alpine.
- `CGO_ENABLED=0` producing a binary that builds and then fails on first
query.
## Not verified
- ~~The 2–2.3× figures come from a standalone harness, not this load
under the old driver.~~ **Closed** by @efiten's production run above,
which also corrected the multiplier.
- SQLite 3.46.0 → 3.53.4 query-planner differences on queries with no
total `ORDER BY`.
- Sustained live ingest through the new writer DSN, and the duplicate
collapse against a database an ingestor is actively writing to. Verified
against a static snapshot only, and the collapse is measured at 4.1s on
2.4M synthetic rows with 5 duplicates — well short of an 11M-row
instance. @efiten has offered a staging instance taking real MQTT
traffic; **this is the item to close before the PR leaves draft.**
An earlier revision of this branch shipped the dedup merge in the wrong
direction with a green test suite, and review then found three more
things in the same file: the repair gated on an error string, a
non-atomic TEMP table drop aimed at the wrong connection, and a deletion
whose only record was a row count. All fixed in
|
||
|
efitenandClaude Opus 5
|
52b9474d7a |
feat(map): filter repeaters by region name (#1862) (#2022)
Fixes #1862 ## What Adds a **Region Scope** picker to the map controls: pick `#be` and the map keeps the nodes that declare `#be` or were seen carrying `#be` traffic. It combines with the #2006 scope-state filter and persists in localStorage the same way. While a region is picked, a small "Region: #be · reset" chip sits on the map itself, so the filter stays visible when the controls panel is collapsed (the default on phones) and can be cleared from there. ## API `/api/nodes` and `/api/nodes/{pubkey}` gain two fields on repeater/room rows: - `declared_regions`: named regions from the node's newest declared-regions answer, split by the same function the Scope Audit now uses for `declaredRegions` (`splitDeclaredRegions`, `cmd/server/scope_config_state.go`), so both pages list a repeater under the same names. `[]` means it answered and named no region. Absent means it never answered, other roles, no declared-regions source, or the declared-regions lookup failed. - `declared_regions_truncated`: present, and `true`, only when that answer was flagged as truncated, so the list is partial. Never `false`: the `nodes.configured_scope` source does not record truncation, so absence does not mean the list is complete. The observed side reuses `transported_scopes`. Documented in `docs/api-spec.md` and the served OpenAPI spec. ### Why no `?hashRegion=` query parameter The observed side lives in the in-memory store. Filtering it after the SQL `LIMIT`/`OFFSET` would corrupt `total` and paging, and the map pages through `/api/nodes`. Same reasoning as the Data path section of #2001. ## Map behaviour - Filtering is client-side over the nodes `fetchAllNodes` already loaded: no new request. One pass over the loaded nodes to build the picker counts, one Set lookup per node per render. The marker filter is `nodePassesMapFilters` (`public/map.js:219`) and the observer stand-down `observerLayerShown` (`:212`), both exported and tested. - The picker and hint count only nodes with a map position, the same test the marker filter applies first, so a count never promises markers the map cannot draw. - The observer layer stands down while a region is picked, for the same reason it does for the scope-state filter. - The popup lists declared and observed regions separately. A truncated declared answer carries the same `truncated` badge the Scope Audit shows. - Absence is not read as a finding: the hint under the picker says a node left off the map is not proof it lacks the region. ## Tests - Go: `node_declared_regions_api_test.go` covers `declared_regions` on list and detail endpoints, the no-source case, agreement with `/api/scope-audit`, `declared_regions_truncated` (truncated, truncated-empty, complete, configured_scope-only, newer untruncated answer, companion) and the OpenAPI schema. - JS: `test-issue-1862-map-region-filter.js` (30 tests) covers the pure pieces (evidence, counts, options, hint, popup rows, `nodePassesMapFilters`, `observerLayerShown`) and, at page level, runs the registered map page through `init()` and `loadNodes()` in a vm sandbox: picker built from loaded nodes, markers filtered by a stored region, observer pins standing down, popup rows, the change handler persisting, and the chip showing, resetting and rendering its text as text. - Mutation-checked: removing the region check, the observer stand-down, the picker build in `loadNodes`, the popup rows, the persist on change, the chip reset, or the truncated flag (Go or JS) each fails a test. `test-issue-2001-map-scope-state.js` still passes. - `go test ./...` in `cmd/server` passes; `check-css-vars` and `check-xss-sinks --diff` are clean. ## Staging validation Build `c646310f`, Chrome, no console errors: - Picking `#be`: chip "Region: #be · reset" at the top of the map, hint "221 nodes with a map position have evidence for #be: 129 declare it, 191 seen carrying its traffic. Absence here is not proof: ...". - Clicking reset: picker back to "All regions", stored choice cleared, chip hidden. - First version, same instance: `declared_regions` on 136 of 500 `/api/nodes` rows; returning to "All regions" blocked the main thread 5.4 s against 3.2 s for the existing Status filter returning to "All". The review measured the region filter's own added work at about 0.08 ms per render plus popup rows that are already built for every marker, so most of that time is the existing full re-render. ## Not verified - The chip placement at phone width, and on narrow desktop widths where it may sit under the expanded controls panel. - The truncated badge with real truncated answers (staging has none today). - `test-map-clustering.js` has one failing test on `upstream/master` too; untouched here. - Marker badges from the original issue body are not implemented; the popup rows are the per-node display. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
efitenandClaude Opus 5
|
efdb3ea0b3 |
feat(analytics): retransmission pressure over time (#1699) (#2023)
## Summary Adds `GET /api/analytics/retransmissions` and a "Retransmission Pressure (proxy)" chart on the Analytics Topology tab, implementing the metric agreed in #1699: for each flood, the number of distinct repeaters in the union of the paths of all its observations (`[A]`, `[A,B,C]`, `[A,D]` gives 4), averaged per time bucket. Topology is the tab that already shows hop counts and repeaters in paths, so the chart sits there instead of in a new tab. ## Definition - Flood routes only (`route_type` 0/1); TRACE excluded. Direct routes carry the route still to travel (firmware `src/Mesh.cpp:78-106,334-342`), zero-hop sends are direct (`src/Mesh.cpp:717-737`), TRACE path bytes are SNR values (`src/Mesh.cpp:59-61`, refused by `sendFlood` at `src/Mesh.cpp:637-641`). Firmware commit 0679dbef. - **Flood events, not hashes.** `transmissions.hash` is UNIQUE and the packet hash excludes the path (`src/Packet.cpp:41-50`), so when the same bytes flood again the observations land on the same transmission. Observations are sorted by time and split into events wherever two consecutive observations are more than 5 minutes apart. Each event is counted on its own and bucketed by its first observation. - Why 5 minutes: a node holds a flood for at most 32 s (`src/Dispatcher.cpp:11,243-251`) plus a random retransmit delay. On live over 7 days, 72,806 of 74,347 flood transmissions span 60 s or less, and of 1,372,283 consecutive observation gaps, 52 fall between 60 s and 300 s against 1,823 above 300 s. - Events that start before the store retention floor (now minus `retentionHours`) are left out for every request shape. The store keeps older observations only for hashes heard again recently, so they do not represent that period. Eviction of those transmissions is tracked in #2024. - A flood event heard only with an empty path counts as 0 repeaters. - **Prefixes are not resolved to nodes, and a prefix counts once per event**, whether it repeats across observations or inside one path. On live (7 days), a repeated 2-byte prefix inside one path occurs in 1.08% of flood transmissions and 6,178 of 6,596 such repeats match exactly one known node; for 3-byte it is 0.69% and 104 of 104. That is one node forwarding again after its 160-slot cyclic duplicate filter dropped the hash (`src/helpers/SimpleMeshTables.h:9,52-57`). A repeated 1-byte prefix (44.9% of 1-byte transmissions) is mostly two nodes; counting it once keeps the value a lower bound. `summary.one_byte_packets` reports how many events that affects. - Observations are stored once per observer and path per hash, so a later event of the same hash only holds pairs not stored before; its count is a lower bound too. On live these are 1,466 of 75,356 events (1.9%), and they are kept in the average. - Resolution was not used: on live, 1-byte observations nearly all have `resolved_path` NULL, and cold load refuses context-based resolution of history (`cmd/server/neighbor_persist.go:155-168`). - Buckets `5m|15m|1h|6h|1d`. `region` filters on observers like `/api/analytics/rf`, after the event split; a region with no known observers is not filtered, the same as the other analytics endpoints. `area` is not supported. ## Implementation - `cmd/server/retransmission_pressure.go:255` `addPath`: scans path JSON directly into a generation-stamped hash set, no allocation per observation. - `cmd/server/retransmission_pressure.go:367` `computeRetransmissionPressure`: one pass under `s.mu.RLock`. Per flood transmission it sorts the observations by cached parsed time into a reused scratch slice, splits events and counts each in `addEvent` (`:319`). O(T + O log k + H). - `cmd/server/retransmission_pressure.go:470` `GetRetransmissionPressure`: default shape from the recomputer (#1659 warm-up gate). Other shapes come from a typed TTL cache (max 64 entries) cleared on new paths and eviction (`cmd/server/store.go:2289,2336`); concurrent misses on one key share one compute through singleflight (`store.go:204`). - `cmd/server/retransmission_pressure.go:515` handler, `cmd/server/routes.go:331`, `cmd/server/openapi.go:108`, `docs/api-spec.md:1283`. - `public/analytics.js:761` card, `:855` `renderRetransmissionChart` (CSS variables only, lines break at missing buckets, caption states it is a proxy, names the observer coverage bias, the once-per-flood prefix rule and the 5 minute event split), `:829` loader with stale-response guard. ## Performance - `BenchmarkComputeRetransmissionPressure`, 50k transmissions x 20 observations, `-cpu 1`, i5-1335U: median 161 ms/op (132 ms/op before the event split); first pass after startup with timestamps not yet parsed 192 ms/op. About 22 KB and 281 allocations per op. - On staging the default shape is served from the recomputer in 0.3 s; the post-load recompute of this recomputer took 994 ms on a 121k-transmission store (log line quoted in #2025). A 336h store would be about twice that, every recompute interval, under the store read lock. ## Tests - `cmd/server/retransmission_pressure_test.go`: union counting (reporter example, overlaps, once per event for 1/2/3-byte, width/case, growth); route/TRACE/zero-hop filter, bucketing, window by event start; event split and the 5 minute settle gap (boundary, chained steps, unsorted input), retention floor; region filter, region applied after the split, unknown region, 1-byte share; recomputer read, TTL cache invalidation on new paths and on eviction, cache expiry, singleflight, recomputer gate wiring, handler, warm-up gate. - `test-issue-1699-retransmission-chart.js` (33 tests, registered in `test-all.sh` and `deploy.yml`). - Mutation-checked: 18 mutations of the event split, floor, prefix rule, bucketing, region order, cache clears, expiry, gate wiring and singleflight, all killed. - `go test ./...` in cmd/server passes; `scripts/check-css-vars.js` OK. ## Staging validation Build `c646310f` (this rework plus #2025 and the other review follow-ups), after a container restart and full load: default shape 74,974 flood events, average 27.08 repeaters, 169 hourly buckets from 2026-09-06 16:00 (the 168h floor) to the current hour, highest hourly average 53.3. Before the rework the same instance showed buckets back to 2026-07-18, averages up to 148, and for the first minutes after a restart only 5,911 packets. ## Merge order with #2025 #2025 fixes the recomputer startup for all analytics endpoints (the stale first snapshot seen here). Whichever of the two merges second has to add `recompRetransmissions` to `analyticsRecomputersLocked`, wire it to that PR's `loadedGate` instead of `LoadComplete`, bump the recomputer count in `TestAnalyticsRecomputers_PostLoadOrder` from 9 to 10, and make `TestStartAnalyticsRecomputers_RetransmissionsGatedOnLoadComplete` call `signalStartupLoadDone()`. That resolution is what ran on staging. ## Not verified - Recompute timing on a production-size (336h) store; only extrapolated. - Phone-width layout and dark theme of the reworked chart. - E2E Playwright suite. Fixes #1699 --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
|
914bd4cf0e |
feat(channels): show each message's region (#1851) (#2018)
## Show each channel message's region scope Each message in the Channels view now shows the region scope it was sent with, as a small chip in the meta line: the region name (for example `#be`), `unknown scope`, or nothing. This is the channel-message part of #1852 by @dborup, extracted as a focused change. #1852 was closed unmerged because it had grown to the whole fork diff. The implementation follows dborup's commits |
||
|
efitenandClaude Opus 5
|
a059299588 |
feat(node-analytics): hop-count statistics per node (#1812) (#2021)
## Summary
Adds per-node hop-count statistics so repeater operators can choose
`flood.max`, `flood.max.unscoped` and `flood.max.advert` from what their
node actually sees.
- New endpoint `GET /api/nodes/{pubkey}/hop_analytics?days=N`
(`cmd/server/routes.go:299`, `cmd/server/node_hop_analytics.go:312`),
separate from `/analytics` as requested in the issue.
- New card "Hop Count at This Node" on the node analytics page
(`public/node-hop-analytics.js`, wired at
`public/node-analytics.js:130,174`): histogram of hop counts with a box
plot on the same x axis, filters `flood.max` (default),
`flood.max.advert`, `flood.max.unscoped`, driven by the existing range
picker.
- The existing "Hop Distribution" chart is unchanged: it shows path
length at the observer, a different quantity.
- No `direct` tag, although the issue lists one: for DIRECT packets the
path is the remaining route and no flood limit applies, so there is no
hop count to report.
## Hop count definition (firmware 0679dbef)
- `src/helpers/RoutingPolicy.h:15-21`: limits compare
`getPathHashCount()`; `.unscoped` applies to route type FLOOD, `.advert`
to adverts.
- `src/Mesh.cpp:344-350`: `routeRecvPacket` checks with n hashes in the
path, then writes its own hash at index n. So hops = the node's
zero-based index in the path, no +1.
- `src/Mesh.cpp:265-285`: a node forwards a flood once;
`src/Mesh.cpp:651,680`: an originator never forwards its own flood.
- DIRECT packets are excluded: their path is the remaining route
(`src/Mesh.cpp:78-103,334-341`).
Response: `{timeRange, packets: [{hash, timestamp, hops, tags}],
ambiguous}`. Tags: `flood`, `scoped` or `unscoped`, `advert`. Documented
in `docs/api-spec.md:679` and `cmd/server/openapi.go:90`.
## Attribution
`cmd/server/node_hop_analytics.go:198-309`. The result depends only on
the observed paths, the prefix map and the neighbor graph, so it is the
same after a restart as after live ingest.
- Every observation of every flood packet in the window is read.
`byNode` holds the server resolver's pick at ingest and other picks
after a cold load; `byPathHop` indexes only each packet's longest path,
which for a busy relay often runs through another branch of the flood.
- A packet counts when the node's prefix sits at exactly one index
across its observations, and either the node is the only relay candidate
for that prefix (`prefixMap.relayCandidates`,
`cmd/server/store.go:6795`), or the hop resolves to the node under the
ingestor's strict rule (`cmd/ingestor/path_resolver.go:143-214`) in at
least one observation and to another node in none. Strict rule: earlier
hops identified without a tiebreak, exactly one candidate adjacent in
`neighbor_edges` to the previous hop (the originator for hop 0 of an
advert), nodes already on the path excluded.
- The server resolver's tiebreaks (affinity, GPS distance, advert count,
pubkey order) are not used.
- Everything else with the node's prefix goes to `ambiguous`. In
practice that is most packets with a colliding 1-byte path hash.
On a read-only 7-day dump of a 1,669-node mesh DB, for one busy
repeater: 23,081 packets attributed, 11,437 ambiguous. Taking candidates
from `byPathHop` instead gave 9,995 attributed, with the histogram mode
moved from 2 to 3-5 hops.
## Performance
Scans `s.packets` under the read lock, no SQL per packet. Per
observation: one substring test for the node's first prefix byte; the
hop scan only for observations containing it; the strict walk only for
colliding prefixes, with per-request caches for candidates and
adjacency. `BenchmarkNodeHopPackets` models one 7-day request at that
scale (73,782 flood packets, 1,430,280 observations): 44-87 ms/op, 13.4
MB, 40 allocs on a throttling laptop.
Response size for that repeater over 7 days: about 23k entries, 2.3 MB
JSON, 375 KB gzipped. `hash` and `timestamp` are 61% of the raw and 91%
of the gzipped bytes; they stay because the issue asks for them so a
client can join entries to packets and bin by time.
## Tests
- Go: `cmd/server/node_hop_analytics_test.go`: 12 unit tests, a
live-ingest test through `IngestNewFromDB` (a colliding prefix without
independent attribution goes to `ambiguous`, not to the node the
resolver picked), live ingest versus cold load of the same DB, route
test, benchmark. 15 mutations of the attribution logic each fail a test.
- JS: `test-node-hop-analytics.js` (filters, histogram, quartiles and
whiskers with a fixture that separates 1.5 IQR from 3 IQR, render),
registered in `test-all.sh` and `.github/workflows/deploy.yml`.
- `gofmt`, `go vet ./...`, `go test ./...` in `cmd/server`,
`scripts/check-css-vars.js` pass.
## Staging validation
Build `c646310f` (this PR's review follow-up together with the other
open follow-ups), after a container restart and full load, on a busy
Belgian repeater:
- `hop_analytics?days=7`: 23,302 packets, 11,548 ambiguous, median 4,
adverts never above hop 7 (matching the firmware default
`flood_max_advert = 8`, `examples/simple_repeater/MyMesh.cpp:922`), 1.2
s. The first version reported 23,035 packets and 86 ambiguous in 534 ms,
because it trusted the resolver's pick for colliding prefixes.
- The card rendered on the first version with no console errors; the
rework does not touch the frontend beyond a test fixture.
## Not verified
- Response time and lock hold for 30 days on the busiest node on a
14-day store.
- Server relay candidates exclude companions and listeners while the
ingestor's prefix index does not, so a few strict attributions can
differ from the ingestor's persisted `resolved_path`.
- Identical numbers across a second container restart were shown in a Go
test, not repeated on staging.
- Dark theme, phone width, and switching the range picker in the
browser.
- Filter state is not reflected in the URL hash (the range picker is not
either).
Fixes #1812
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
efitenandClaude Opus 5
|
2d4019f719 |
fix(analytics): recompute once the store has fully loaded (#2025)
Refs #2023, #1659, #1724 ### Problem `main.go:258` waits only for the first load chunk, then `main.go:402` starts the analytics recomputers. `Start()` computes immediately on that chunk (`analytics_recomputer.go:86` on master) and the next compute waits a full interval (`:93`, 5 min default). The chunk loader walks by ascending id, so that chunk holds the oldest transmissions. - RF, topology, channels: the #1659 gate checked `LoadComplete()` after the compute (`analytics_warmup_1659.go:122`). `LoadComplete` flips at the end of the hot window (`chunked_load.go:489`), before the background fill (`store.go:1455`), so the gate could open on a snapshot without the background fill, and the 60 s force timeout (`:73`, `:162`) opened it on the first-chunk snapshot. In an end-to-end test on master, `/api/analytics/rf` returned 200 with 8 of 100 packets before the background fill ran. - Distance, hash-collisions, hash-sizes, roles, observers-clock-skew, nodes-clock-skew: no gate, partial snapshot served from the start. - Distance additionally served a snapshot from the previous index for up to one interval after each lazy index build. On a staging instance, a default analytics request returned 5,911 packets with hours-old last buckets until the next recompute (about 74k). ### Change - `StartupLoadDone()` (`chunked_load.go:108`): closed when `RunStartupLoad` returns, on every path (`chunked_load.go:202`). Closing it drops the hash-size info cache (15 s TTL) and the clock-skew engine throttle (30 s, `clock_skew.go:225`), both read by the post-load computes. - `recomputeWhenLoaded` (`analytics_recomputer.go:172`): on that signal, recompute each recomputer once, sequentially, via `RecomputeNow` (`:154`), which runs on the recomputer's own loop and restarts its ticker (`:106`). Order (`:255`): rf, topology, channels, distance, hash-collisions, hash-sizes, observers-clock-skew, nodes-clock-skew, roles (roles reads the nodes-clock-skew snapshot). Logs one line with per-recomputer durations. - Warm-up gate: now the same signal (`:343-354`), sampled before the compute starts (`:129`), so a pass that began on partial data never opens it. 503 + `Retry-After: 5` and the force timeout are unchanged; a forced-open snapshot is replaced by the post-load recompute. - Ungated endpoints: no new 503s (their API has none); snapshot replaced right after the load. - Distance: the lazy index build refreshes the distance recomputer before reporting built (`store.go:4476`). - Recompute intervals and config unchanged. ### Performance One extra compute per recomputer per process start, run sequentially so they do not all hold the store read lock at once. Ticker phases afterwards are offset by the cumulative post-load compute durations instead of all starting within the first-chunk compute window (relevant to #1724; the effect on lock waves is not measured). ### Tests `analytics_recompute_after_load_test.go`: signal open during background fill, closed after success and failure; cache drops; immediate and ordered post-load recompute; gate not opened by a pass started before the load; forced-open snapshot replaced on load; ticker restart; distance refresh before 202 ends; end to end with recomputers started before the background fill (RF 503 until load, then `totalTransmissions` equals the full store; six ungated endpoints 200 during load; all nine recomputed after load). 9 of these failed on master with stubs; 8 single-line mutations each caught. `go test ./...` in `cmd/server` passes. ### Staging validation Deployed together with the review follow-ups of #2015-#2023 (build `c646310f`), container restart: ``` 16:35:20 [store] first chunk ready (chunkSize=10000) 16:35:25 [store] LoadChunked complete ... starting background fill loader 16:36:58 [store] background load complete: 121120/121282 packets in memory (coverage=99.9%) 16:37:03 [analytics-recompute] startup load done: recomputed 10 snapshots in 5.155s (rf=955ms topology=1.684s channels=43ms distance=49ms hash-collisions=30ms hash-sizes=338ms observers-clock-skew=369ms nodes-clock-skew=692ms roles=2ms retransmissions=994ms) ``` Right after that line, `/api/analytics/rf` reported `totalTransmissions` 121,121 against 120,700 packets in memory, and the retransmissions default shape from #2023 covered the full 7 days. Before this change both waited for the next 5 minute tick. ### Merge order with #2023 #2023 adds a tenth recomputer. Whichever of the two merges second has to add `recompRetransmissions` to `analyticsRecomputersLocked`, wire it to `loadedGate` instead of `LoadComplete`, and change 9 to 10 in `TestAnalyticsRecomputers_PostLoadOrder`; the retransmissions gate test then calls `signalStartupLoadDone()` instead of setting `loadComplete`. That resolution is what ran on staging above. ### Not verified - Repeater-enrich recomputer and the region/window TTL caches (hash-collisions region results have a 1 h TTL) may also keep partial results after the load; not changed here. - Recompute order is tested structurally, not with roles/clock-skew data. - Whether this reduces the #1724 stalls; not measured. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
efitenandClaude Opus 5
|
7f8f71e773 |
fix(live): reconnect when the websocket goes silent (#1074) (#2020)
## Problem #1074 reports that after a proxy dropped the WebSocket, live updates only came back 8 to 10 minutes later. The client only reconnects from `onclose` (`public/app.js:791` on master). A half-open connection (a proxy or NAT dropping state without a FIN reaching the browser, a laptop that slept) can keep a WebSocket OPEN for minutes without `onclose`, and nothing retries in the meantime. The server does ping every 30s (`cmd/server/websocket.go:252` on master), but ping frames are answered by the browser below the page and JS cannot observe them. The only app-level frames are packet broadcasts (`websocket.go:337, 343, 366`), which stop on a quiet mesh. So the client had no signal to tell a quiet mesh from a dead socket. ## Change Server (`cmd/server/websocket.go`): - On the existing ping tick, `writePump` also writes the text frame `{"type":"heartbeat"}` (`:283`, bytes at `:80`). One 20-byte frame per client per 30s, from the goroutine that already writes the ping, no hub lock. - The interval moves to `Hub.pingInterval` (default 30s, `:118`) so a test can shorten it. Client (`public/app.js`): - Every frame refreshes `wsLastMessageAt` (`:841`). One timer (`checkWSLiveness`, `:806`) fires at last message + `WS_STALE_MS` (75s, one late or lost heartbeat of slack) and replaces the socket if it is still silent. It is armed at socket creation, so a stuck handshake is covered too. - `dropWS` (`:796`) detaches the old socket's handlers before `close()`, so a late close event cannot schedule a second connection. - `connectWS` (`:818`) cancels a pending reconnect and drops the previous socket, so the watchdog, resume checks, `onclose` and pull-to-reconnect cannot stack sockets. Before this, `pullReconnect` on a non-open socket left a third socket 3s later. The 3s `WS_RECONNECT_MS` delay after `onclose` is unchanged (`:837`). - `visibilitychange` (to visible) and `online` run the check immediately (`:861`), because a hidden or sleeping tab's timers can run late. - Heartbeat frames are matched by exact bytes (`:842`) and are not pulsed or dispatched to `onWS` listeners. Compatibility: tabs loaded before the deploy dispatch heartbeats to their listeners until reloaded. Every current listener filters on `msg.type`, so the visible effect is a logo pulse and a `/stats` cache refresh every 30s. Perf: one `Date.now()` and one string compare per WS message on the client; one extra 20-byte write per client per 30s on the server. ## Tests - `test-ws-stale-watchdog-1074.js`: real `app.js` in a vm with a fake clock, timers and WebSocket. 12 tests: silence past the threshold replaces the socket exactly once; a handshake that never opens is replaced; heartbeats and packet traffic keep the socket; heartbeats are not dispatched; resume and `online` after silence reconnect immediately, with recent traffic they do not, and hiding does not trigger a check; repeated resume events open one socket; after `onclose` only the reconnect timer is pending; pull-to-reconnect leaves one socket. 9 of 12 fail on master. 12 of 12 source mutations (threshold, reconnect path, detaching, timer cleanup, resume wiring, heartbeat filter) are caught. Registered in `test-all.sh` and the deploy.yml unit step. - `TestWritePumpSendsAppHeartbeat`: fails with a read timeout without the heartbeat, even with pings every 20ms. `TestHubDefaultPingInterval` pins the 30s interval that `WS_STALE_MS` assumes. - `go test ./...` in `cmd/server` passes; gofmt and go vet are clean. ## Browser validation On a staging instance (build `139e484e`, together with #1979's branch), in Chrome, no console errors: - A `{"type":"heartbeat"}` frame arrived on the open socket within the observation window. - Silent socket: after `ws.onmessage = null`, the page replaced the socket after 76.2s (threshold 75s plus a 250ms poll); the old socket ended in CLOSED, the new one OPEN. - Normal close: `ws.close()` led to a new OPEN socket after 4.1s, and exactly one new `WebSocket` was constructed. ## Not verified - The reporter's proxy setup was not reproduced; that their delay was a half-open socket is a hypothesis consistent with the symptom. Hence `Refs`, not `Fixes`. - Laptop sleep and the `visibilitychange` / `online` resume path were only covered by the unit test, not in a browser. - Behaviour under Chrome's intensive background-timer throttling and mobile tab freezing was not measured; a frozen but healthy tab may do one unnecessary reconnect on resume. - Go tests were run without `-race`. Refs #1074 ## Review follow-up (commit `72e5e906`) An independent review found no blocking bug: all data writes stay on the write goroutine, pong-based dead-client detection still works, and no ordering of onclose, watchdog, resume checks and pull ends with two live sockets or none. It reproduced the silent-socket case in headless Chromium through a blackholing TCP proxy (replacement 75.0 s after the last frame). Changed: 1. **Startup wiring tested.** The first resume test now boots through the page's real `DOMContentLoaded` listeners, so removing `setupWSResumeCheck()` from startup makes it fail. 2. **Wall clock stepping back.** If the clock steps back after the last message, the watchdog no longer re-arms for the size of the step (a 1 h step used to delay detection by about an hour). A negative silence reading is treated as stale, so the socket is replaced within `WS_STALE_MS` of the step (`public/app.js:810-814`). A step in either direction costs at most one extra reconnect on a healthy socket. `Date.now()` stays the clock so a tab resumed after sleep is still checked against real elapsed time. 3. **Pull-to-reconnect at once.** On an OPEN socket, pull-to-reconnect now replaces it through `connectWS()` instead of closing it and waiting for onclose, which took 63 s on a half-open connection in the review's measurement (`public/app.js:927-934`). This was slow on master too; it is safe now that `connectWS()` detaches the old socket. Tests: 12 to 16 in `test-ws-stale-watchdog-1074.js`; `test-pull-to-reconnect.js`, `test-pull-to-reconnect-1091.js` and `test-live.js` pass. Correction to the compatibility note: tabs opened before the deploy treat the heartbeat like any other message. Besides the logo pulse, `app.js` runs `updateNavStats` on every message and invalidates the cached `/stats` and `/nodes` responses 5 s later; `packets.js` also pushes every message into `pauseBuffer` unfiltered (~1310-1313), so an old tab with Packets paused sees its counter rise by 2 per minute. Cosmetic: heartbeats are filtered out on replay, and a reload ends it. Not verified: real hidden-tab or mobile freeze behaviour, Firefox and Safari, and the reporter's proxy setup. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
efitenandClaude Opus 5
|
0fea3f2a75 |
feat(analytics): break scope adverts down by node role (#1979) (#2019)
## Summary Adds a breakdown of flood adverts by sender role to `/api/scope-stats` and the Scopes tab, in the descriptive shape agreed in #1979: per node role, how many flood adverts were unscoped, scoped with an unnamed region, or scoped with a named region. It reports what was sent, not why. ## Changes - `cmd/server/db.go:3114-3144`: one grouped query in `GetScopeStats`. ADVERT packets on flood routes (TRANSPORT_FLOOD 0, FLOOD 1) in the window, `LEFT JOIN nodes` on `from_pubkey`, split by the three `scope_name` states (NULL, empty string, name). A missing or empty role becomes `"unknown"`. Ordered by total descending, then role. Zero-hop adverts are excluded because firmware sends them as DIRECT/TRANSPORT_DIRECT (`src/Mesh.cpp:717-730`, `Mesh::sendZeroHop`), so they would inflate "unscoped". - `cmd/server/types.go:116-133`: `ScopeAdvertRoleCount` and `ScopeStatsResponse.AdvertsByRole` (`advertsByRole`, always an array). - `public/analytics.js:4760`: `scopeAdvertsByRoleHtml` renders a table under the time-series chart with the count per state and its share of the row. Role text is escaped. It reuses the existing `/scope-stats` response, so there is no extra request. - `docs/api-spec.md:1763-1786`: documents the new field. `/api/scope-stats` is in `openapi_known_gaps.json`, so there is no `openapi.go` entry to update. API addition (existing fields unchanged): "advertsByRole": [ { "role": "repeater", "unscoped": 7741, "unknownScope": 7, "named": 1562 } ] ## Performance The query runs inside `GetScopeStats`, so it shares the existing 30s cache per window. The unary `+` on `payload_type` keeps SQLite on the `first_seen` range index. Without it the planner picked the `payload_type` index and walked every stored advert whatever the window. Read-only timing on a production DB (1,063,345 transmissions, 161,634 adverts, sqlite3 CLI 3.45.1): | Window | payload_type index | first_seen index (this PR) | |---|---|---| | 7d | 0.231s | 0.059s | | 24h | 0.214s | 0.008s | | 1h | 0.213s | 0.001s | ## Tests - `TestGetScopeStatsAdvertsByRole` (`cmd/server/db_test.go:2295`): the three states, flood-only routes, non-advert and out-of-window exclusion, `unknown` for a missing node row, an empty role and a NULL `from_pubkey`, and ordering. Mutation checked: widening to routes 0-3 and dropping the empty-role fallback both fail it. - `TestGetScopeStatsAdvertsByRoleEmpty` (`:2372`): empty result is `[]`, not null. - `test-issue-1979-scope-adverts-by-role.js`: renders the real `analytics.js` helper in a vm sandbox. Covers row order, totals and shares, columns, escaping (mutation checked), the empty state, and the non-causal caption. Registered in `test-all.sh` and the deploy.yml unit step. - `go test ./...` in `cmd/server` passes, gofmt and go vet are clean, `check-css-vars.js` OK, `check-xss-sinks.sh --diff` exits 0. ## Browser validation On a staging instance with live traffic (build `139e484e`, together with #1074's branch), in Chrome, no console errors: `/#/analytics?tab=scopes` shows "Flood adverts by node role" under the time-series chart with its caption, and a table of 6 roles for the default window, for example `repeater 1.361 | 1.109 (81.5%) | 2 (0.1%) | 250 (18.4%)`. Shares in each row add up to 100%. `/api/scope-stats?window=7d` returns `advertsByRole` with the same six roles. ## Not verified - Timings are from the sqlite3 CLI, not the modernc driver in the server process. - Role is the sender's current `nodes.role`. A node whose advert type changed within the window is counted under its latest role. The data also contains a raw `type-13` role, shown as is. - Switching the window in the browser was not exercised; the API was checked for 7d. - No Playwright E2E added. Fixes #1979 ## Review follow-up (commit `43af46b8`) An independent review found no correctness, security or performance problem: counts are per transmission, the window matches the rest of the Scopes tab, zero-hop adverts are DIRECT per firmware, and the `+t.payload_type` hint holds on modernc SQLite 3.46.0. It found two test gaps and a docs gap. Changed: - **Ordering.** The Go fixture gave companion, repeater and unknown 3 adverts each, so ordering by total was never checked (`ORDER BY COUNT(*) ASC` still passed). The fixture now has repeater 4, unknown 3, companion 2, sensor 2, so the expected order differs from alphabetical and the companion/sensor tie checks the role-name tie-break. Reversing the count order, dropping it, or reversing the tie-break now fails the test. - **Column positions.** The JS test only checked that each cell string appeared somewhere in the row, so swapping two columns passed. It now compares each row's cells and the header cells by position; both swaps fail. - **Docs.** `docs/api-spec.md` and the `ScopeAdvertRoleCount` comment now name every source of `unknown`: a NULL `from_pubkey` (legacy rows the #1143 backfill has not reached), a sender with no `nodes` row, including one moved to `inactive_nodes` by node retention (inside the 7d window only with `retention.nodeDays` below 7), and an empty role. Not added: a query-plan test pinning the `+t.payload_type` hint. The SQL is inline in `GetScopeStats`, so the test would have to copy it or the query would have to move into a constant; left for a follow-up if wanted. The raw `type-13` role in the table is the ingestor's placeholder for reserved advert types 5-15 (`cmd/ingestor/decoder.go:1229`, #1279), shown as is. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
efitenandClaude Opus 5
|
5efe61eef2 |
fix(ingestor): set an explicit MQTT ClientID per source (#2013) (#2016)
## What `buildMQTTOpts` (`cmd/ingestor/main.go:591`) never called `SetClientID`, so with paho.mqtt.golang v1.5.0 every ingestor connected with a zero-length ClientID and `CleanSession=true`. The session identity then depended on the broker. This PR: - adds an optional `clientId` per `mqttSources` entry (`cmd/ingestor/config.go:29`) - when unset, uses `corescope-<name>-<6 hex chars>` (`cmd/ingestor/main.go:653`). The name is reduced to `[0-9A-Za-z-]`, with the broker host as fallback when the name is empty. The suffix comes from `crypto/rand` and changes on every ingestor start. - sets the ID once per source in `buildMQTTOpts` (`cmd/ingestor/main.go:616`). paho copies the options into the client and reuses them for every reconnect, and the watchdog force-reconnect reuses the same client, so the ID is stable for the life of the process. - logs the ID on connect: `MQTT [tag] connected to <broker> as client <id>` (`cmd/ingestor/main.go:150`) - documents the key in `config.example.json:210` as a `_comment_clientId` entry rather than a value, because `docker/entrypoint.sh:6` copies that file as a live config and a literal value would give every default deployment the same ID. Also listed in `cmd/ingestor/README.md:94`. ## paho behaviour - No client-side length limit. `SetClientID` only stores the value; the 65535 check in `packets/connect.go:156` is in `Validate()`, which the client never calls. - The default ID is longer than the MQTT 3.1 limit of 23 characters for most source names. paho falls back to MQTT 3.1 after any refused CONNACK when no protocol version is set (`client.go:412`), so on a broker that refuses the first 3.1.1 attempt, the retry may hit that limit. I did not cap the length because paho does not require it and the 3.1.1 path accepts it (see below). ## Tests `cmd/ingestor/mqtt_opts_test.go:53-107`: - default ID is non-empty, has the sanitized name prefix, and contains only `[0-9A-Za-z-]` - broker host is used when the name is empty - configured `clientId` is used verbatim - two unconfigured sources with the same name get different IDs - the client built from the options reports the same ID Mutation checks: removing the random bytes fails the "different IDs" test; removing sanitization fails the prefix and character-set tests. `go test ./...` in `cmd/ingestor` passes except `TestWriteStatsAtomic_SymlinkAtDestIsReplaced`, which fails locally on Windows for a symlink privilege reason. `gofmt` and `go vet` are clean. ## Validation against a real broker On a staging instance (build `e84d2da6`) connecting to a Mosquitto bridge: ``` MQTT [lincomatic] connection attempt #1 to tcp://mosquitto-bridge:1883 MQTT [lincomatic] connected to tcp://mosquitto-bridge:1883 as client corescope-lincomatic-71a6eb MQTT [lincomatic] subscribed to meshcore/# ``` The 27-character default was accepted on the first attempt and packets kept arriving afterwards. ## Not verified - Only one broker type (Mosquitto) was tried. - `-race` was not run locally (no cgo toolchain on the test machine). - The case where both the source name and the broker host are empty (ID becomes `corescope-<hex>`) has no test. Fixes #2013 ## Review follow-up (commit `a1d6709e`) An independent review found no bug in the ID handling, but the tests covered less than their names said. Changed, tests only (`cmd/ingestor/mqtt_opts_test.go`): - `TestBuildMQTTOpts_ClientIDSurvivesReconnects` replaces the old stability test, which only checked that paho copies the options. Against a loopback fake broker built on paho's `packets` codec, the first CONNECT, paho's auto-reconnect after the broker drops the socket, and the watchdog force-reconnect (`buildForceReconnectFn`) must all carry the same non-empty ID. It runs in about 0.01 s and passed `-count=30 -cpu 1,2,8`. - `TestBuildMQTTOpts_ClientIDDefaultShape` asserts full IDs: `^corescope-local-feed-1-[0-9a-f]{6}$`, `^corescope-mqtt-example-com-[0-9a-f]{6}$` for the broker host fallback (no port), and `^corescope-[0-9a-f]{6}$` with neither a name nor a host. - Mutations now caught: `SetClientID` removed, `u.Host` instead of `u.Hostname()`, a 1-byte suffix, the name guard dropped, sanitization removed. The "as client" log line has no test because it is logged from a closure inside `main()`. Corrections to the description: - **Fallback to MQTT 3.1.** paho falls back after any failed handshake once the socket is open, not only after a refused CONNACK: also a read error or timeout before any CONNACK, or a first packet that is not a CONNACK (`client.go:401-416`, `net.go:83-97`). A failed dial does not trigger it (`client.go:387-391`), and after the first successful connect the protocol version is locked in (`client.go:422-424`). Without this PR the 3.1 retry sent an empty ID, which MQTT 3.1 forbids as well, so nothing gets worse. - **Broker side.** On the EMQX broker we run, authorization has per-username and all-client rules and no client-ID rules (checked through its REST API). Two per-username rules use `${clientid}` in a topic, but both are publish rules and the ingestor only subscribes, so no rule can match it. A broker that caps IDs at 23 characters but accepted the empty ID before would now reject the default ID for source names of 7 or more characters; I have no evidence such a broker is in use. - The "Not verified" item about the empty name and host case no longer applies. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
efiten
|
296456f9c1 |
feat(map): colour and filter repeaters by scope-configuration state (#2006)
Closes #2001. Two review rounds plus a re-review; findings and evidence on the PR. Verified on a deployment against live data: the field over 1653 nodes, marker tints per filter state, the marker title and popup Scope row reaching the DOM, and the colorblind-preset cascade. The audit and the map are held to the same classification by an end-to-end test that fails when either side's wildcard handling drifts. |
||
|
liquidraver
|
fe37f1060c |
fix(ingestor): delete aged packets in bounded batches so prune stops stalling ingest (#2000)
Reviewed at
|
||
|
efiten
|
02feb2a88e |
test(ingestor): join the watchdog loop goroutine instead of only asking it to stop (#2003)
Verified before merging: on upstream/master `go test ./cmd/ingestor -run TestMQTTStallWatchdog -count=20` fails; on this branch the same command passes. The flake blocked CI on #2000. |
||
|
efiten
|
0c7f2306f6 |
feat(ingestor): keep the scope-match tally across restarts (#2002)
## What `scopeMatchCounters` (unique / explicit-over-derived / ambiguous / none) lives only in the ingestor process, and the only way to read it is the periodic log line. A restart zeroes the counters, and recreating the container removes that log with it, so a measurement in progress cannot be recovered afterwards. That happened here on 2026-09-10: a 24h ambiguity measurement completed, two deploys followed before it was read, and nothing on disk held the number. `/var/lib/docker/containers/*/*-json.log` had no earlier copy. The tally gates a real decision (whether the collision tie-break from the `autoRegionKeys` design is worth building), and that needs days of traffic, so it has to survive the process counting it. ## How A single-row table, `scope_match_totals`: - `OpenStore` restores the counters from it, so counting continues instead of restarting. - The 5-minute stats ticker writes them back, and so does the shutdown path, which is what a deploy triggers. - `since_unix` carries the window start across restarts, so the ratio keeps a denominator. The periodic log line now prints it. Saving rides the **stats** ticker, not the region-refresh ticker: matches are recorded for every transport-scoped packet, including on instances that never enable `autoRegionKeys` and so never start the refresh loop. The table is **not** in `internal/dbschema` on purpose. The server neither reads nor PRAGMA-detects it; putting it in `AssertReady` would make an older DB fail the server's startup check over data the server never looks at. A failed restore is logged and ingestion continues. Losing an observability counter is not a reason to stop ingesting. ## Tests Four, all in `cmd/ingestor/scope_match_tally_test.go`: - totals and `since_unix` restored across a close/reopen - a fresh DB gets its anchor row immediately, so the first window has a start time - recording after a restore adds to the carried total instead of counting from zero - repeated saves keep exactly one row Full `cmd/ingestor` suite green locally except `TestWriteStatsAtomic_SymlinkAtDestIsReplaced`, which fails on Windows only (`os.Symlink` needs a privilege this account lacks) and predates this branch. `go vet` and `gofmt` clean. The race detector was not run locally (no cgo toolchain here); the `race-test` job covers it, since this branch touches `cmd/ingestor`. ## Not done No UI or API surface for the tally. It is still read from the log line or straight from the table. |
||
|
efiten
|
6c92a8b612 |
test(ingestor): make the suite race-clean, and run the detector when it matters (#1994)
`go test -race ./...` on `cmd/ingestor` reports **six data races** on master. None is in production logic. All six come from test helpers that outlive the test that started them, which is why the detector blames whichever test happens to be running: two different tests failed on two consecutive runs of the same code. ## What was racing **`StartStatsFileWriter` had no way to stop.** Two tests start it at a 50ms interval, and its goroutine then runs for the rest of the process. It reads the package-level `readProcSelfIOFn` hook, which a later test replaces to inject a fake, so the write and the read race. The same leak explains the stray log lines about writing stats into temp directories that were already cleaned up. It now returns a stop function that closes the goroutine and waits for it to exit. Production ignores the return value and runs for the process lifetime exactly as before; the two tests call it through `t.Cleanup`. **The migration test read a log buffer while a goroutine wrote to it.** `log.Logger` serialises its own writes, but `logContains` read `buf.String()` outside that lock while `RunAsyncMigration` kept logging after the call that started it had returned. The capture helper now uses a mutex-protected buffer. That one is worth calling a real race rather than a test artefact: a concurrent read during a buffer grow can panic outright with "concurrent map read and map write"-class behaviour, not merely trip `-race`. ## Measured `go test -race ./...` on linux/arm64 under go1.27.1: **exit 0, zero races, 704s**. ## The CI job, and why it is shaped this way The server has had `-race` since #1208. This closes the same gap for the ingestor, which carries an `atomic.Pointer` snapshot (the region key set from #1989) whose safety has been an argument rather than a measurement. Two deliberate choices, because a check that costs too much gets switched off: - **Its own job, not a step inside "Go Build & Test".** Appending `-race` there puts its ten-odd minutes on the critical path, taking the pipeline from roughly 20 minutes to roughly 32. As a separate job it runs beside the E2E job (15-17 minutes) and hides inside that window. - **Only when `cmd/ingestor/**.go` changed**, decided by the existing change-scope job, which already gates the heavy jobs on documentation-only PRs. A frontend or docs PR cannot introduce a data race in the ingestor. Pushes to master always run it, as they already do for `code`. Nothing `needs:` the new job. Adding it to `build-and-publish` would mean a skipped race job skips everything downstream, which is the opposite of what a conditional check should do. It reports as its own check; whether that blocks a merge is a repository setting rather than workflow logic. ## Scope Test helpers, one production signature (`StartStatsFileWriter` now returns a stop function), and the workflow. No change to what the ingestor does at runtime. |
||
|
efiten
|
675c576fea |
fix(scope-audit): bound the verifier's payload, and fix two tests that proved less than they claimed (#1993)
Three leftovers from reviewing the scope-audit series (#1986, #1987, #1990). None is urgent; all three are the kind of thing that gets harder to explain the longer it sits. ## `scopeHMACInputs` accepted a payload `DecodePacket` rejects Its comment says it walks the same offsets as the decoder, and it does, minus the `maxPacketPayload` bound the decoder enforces. Unreachable in practice: such a packet never reaches the database with an empty `scope_name` in the first place, so the verifier never sees one. Worth closing anyway, because the comment claims the two agree. A verifier that accepts what the decoder refuses is a small divergence today and an hour of confusion on the day it matters. ## The corroboration test seeded the same packet twice The threshold of two rests on `code1` being two bytes: one match is 1/65536 by chance, two on the same region is (1/65536)². That argument needs two **independent** observations. The test seeded `realTransportFloodPacket` twice. Identical payloads derive identical codes, so it was one observation counted twice, and it would have passed just as happily against an implementation that counted rows rather than deriving anything. It now seeds the real captured packet plus a second one built for a different payload, both deriving to `#fm-112` on their own. **The feature was never wrong here.** `transmissions.hash` is unique and `ComputeContentHash` is path-independent, so two rows always mean two distinct payloads in production. Only the test failed to demonstrate the property it is named for. ## Naming at ingest and verifying at read time had no test together They were built separately, in #1990 and #1989, and the interaction between them is not exotic: with derived region keys enabled, a packet that used to be stored unnameable now arrives with a name. The audit must then report that region as observed by the ordinary route: - present in `agg.scopes` - absent from `notObserved` - **not** claimed by `regionEvidence`, which exists to explain regions that could only be established by verification Getting that wrong is quiet. The chip stays green while the reason underneath it is wrong, and a reader asking "how do we know this" gets the wrong story. ## Verification `cd cmd/server && go test ./...` passes (254s), `go vet` and `gofmt -l` clean. No production behaviour changes beyond the payload bound, which rejects input that cannot occur. |
||
|
efiten
|
8c164c5315 |
feat(scope-audit): verify a declared region against the repeater's own traffic (#1990)
Follow-up to #1987, and the point of counting that traffic in the first place. A region this instance holds no `hashRegions` key for is **unnameable, not absent**. #1987 says so with a caveat chip. This settles it wherever the evidence allows: derive `SHA256("#region")[:16]` from the repeater's own declaration and HMAC that repeater's own unmatched packets with it. Same computation the ingestor performs at ingest, with the candidate set narrowed from every configured key to this repeater's handful of declarations. Where it fires, a grey "declared but not observed" chip becomes a green one and the caveat count shrinks by the packets it explained. ## Two packets, not one `code1` is two bytes, so an unrelated name matches a given packet with probability 1/65536. Across ~400 unmatched packets and ~124 declared names, chance alone produces roughly one false match per refresh. Two matches on the same region for the same repeater is (1/65536)², about one in four billion. Lowering the threshold to one would not make this noisy, it would make it **unsound**, so the constant carries that arithmetic and a test rather than a comment. A region with exactly one hit stays grey and reports its single hit, so the page can say why it is still shown as not observed instead of leaving the reader to wonder. ## What it deliberately does not do **It writes nothing.** Read-time only. A wrong answer expires with the window instead of sitting in `transmissions.scope_name` until someone runs a repair, and `cmd/server` stays read-only per the invariant in AGENTS.md. **`notObserved` remains the single source of chip colour.** `regionEvidence` says only HOW a region was established. Two fields that can disagree about the same fact is how this column got confusing in the first place. ## Rule 0, including the part that was wrong at first The naive shape is `targets × names × packets` HMACs: 205 × 124 × 400 ≈ 10M. Caching per `(region, transmission)` pair cuts the HMACs to ~50k. **That measured 501ms**, because the HMACs had become a rounding error while the *iteration* stayed cubic at 10.2M map lookups. Re-keyed per region, holding the set of matching transmissions, it is **36ms** at the same worst-case shape: a region is HMACed over every packet once, and a target then asks one question per declared region instead of one per (region, packet). Most declared regions match nothing, so the common case is a single map lookup and no packet loop at all. `hmacCount` exists so a test can assert the first mistake cannot come back; the benchmark exists because only it caught the second. ## Both axes are bounded, because neither is bounded by the data The "~400 packets in a 7 day window" this was sized against is a property of one instance's configuration, not of the feature: the ingestor stores the unnameable state for every transport-scoped packet no configured key names, so an instance with few or no `hashRegions` entries — the stock state, and the one this helps most — has **every** scoped packet in that set. - the window query takes the 4096 most recent candidates and reports truncation, which the handler logs, so a grey chip on a sampled refresh is not read as "not forwarded" - the declared list is capped at 32 names per repeater: it arrives from a collector that validates each entry's shape but never how many entries there are - measured at the cap: **306ms** for 205 targets over 124 names, against 29ms for the shape a real network produces Because both caps make the evidence a sample, the response carries `observedUnmatchedSampled`. Without it a client subtracts a capped numerator from an uncapped total and overstates the unexplained traffic with no way to know it is doing so. The chip subtracts only evidence for regions **absent** from `notObserved` — a single-hit region the server refused to accept is not called explained either — and says "at most N" when the count was sampled. ## Verified on live data Six repeaters clear the threshold in a 7d window on a real instance. One of them: `nl-nb` green with 3 corroborating packets and the tooltip stating the count, `belml` still grey on 1, and the caveat chip reading 31 of 34 packets unexplained rather than 30. ## Tests `scope_verify_test.go` covers the HMAC-input walk against a real transport-flood packet captured from a live instance (a hand-built fixture would only prove the parser agrees with itself), that `regionCode` does not fold case, the threshold in both directions, the memo's HMAC count, both bounds with their truncation flag, and the benchmark at cap size. Handler-level tests cover a region verified into green, a single hit left grey with its count reported, and the sample-size field. `cd cmd/server && go test ./...` passes (77s), frontend 723 assertions pass, `go vet` and `gofmt -l` clean. |
||
|
efiten
|
0e607c1d01 |
feat(ingestor): derive region keys from what nodes declare, opt-in (#1989)
Follow-up to #1988, which made an ambiguous match deterministic. This adds the second tier of keys that ambiguity rule was needed for. A transport-scoped packet can only be named by a region key this instance holds. `hashRegions` is a hand-maintained list, so every region a node forwards that nobody typed into the config is stored unmatched, and everything downstream reports that region as **absent** rather than as **unnameable**. The instance already knows the names, though: `nodes.configured_scope` holds what the observer `/neighbors` ingestion (#1865) confirmed each node is configured for. This derives keys from those names, on top of the explicit list rather than instead of it. **Default off.** An absent config block leaves behaviour byte-for-byte unchanged, asserted by tests rather than argued. ## Sources Two, mirroring what the server's `AllCurrentDeclaredRegions` already merges, so the derived tier sees exactly what the Scope Audit sees: | source | availability | |---|---| | `nodes.configured_scope` | always — the column is part of the schema, written by the `/neighbors` path | | `node_declared_regions` | optional, where a deployment fills it by other means | The optional table is probed via `sqlite_master` before it is read. A stock install does not have it, and its absence must not abort a refresh the first source could answer on its own. ## The two spellings The sources spell the same region differently, and both are accepted: - `configured_scope` carries the leading `#` that `normalizeScopeList` adds, because every other stored scope value has one - an OTA answer in the optional table carries the bare name - `loadRegionKeys` already prefixes a missing `#` before hashing So `regionNameAcceptable` canonicalises before judging, and hands back the bare name the caller re-prefixes. What it rejects is what cannot be a region name at all: `*` (the flood wildcard, not a region), a comma (it would split the name on the next round-trip through a comma-separated column), a second `#`, whitespace, non-ASCII, NUL padding from a stale client, and anything past 32 characters. The rules are deliberately structural rather than about meaning. A real declared set contains entries that look like junk, but a blocklist on string values is unmaintainable, and the cost of one bad name is a single slot out of the cap plus a 1-in-65536 collision chance. `*` is skipped in **two** places on purpose: in the filter, so no key is ever derived for it, and in the source count, because nearly every node declares it and counting it would overstate both the cap arithmetic and the refresh log on every deployment. ## Rule 0 Each derived key costs one HMAC per transport-scoped packet and raises the random 2-byte collision rate by 1/65536. So: - the tier is **capped** (default 256) - over the cap, names are kept by how many distinct nodes declare them, so a one-off local name is dropped before a region half the network uses - benchmarked linear at ~0.65µs per key: at 314 keys that is 217µs per packet, 0.0008% of one core at the 0.037 transport-scoped packets/s this network produces There is no indexable shortcut to reach for, for the same reason as in #1988: `code1` is an HMAC over the payload, so nothing is payload-independent. The set is an `atomic.Pointer` to an immutable snapshot. A refresh builds the replacement off to the side and swaps the pointer, so the ingest path never blocks on a rebuild. `refreshDerived` is a load-then-store rather than a CAS loop, which is safe only because exactly one goroutine calls it: the refresh ticker, plus one synchronous call at startup. The comment says so, rather than leaving the type looking as though it tolerates concurrent refreshers. ## Tier 2, and the counters When several keys match one packet and exactly one of them is explicit operator config, the explicit one wins: an operator who typed a region into `hashRegions` outranks a name overheard on the air. Ambiguity between two equally-sourced keys still stores unmatched, unchanged from #1988. Counters tally how each packet was decided (unique / explicit-over-derived / ambiguous / none) and are logged on the refresh tick. They exist to answer one question with data rather than estimation: whether a third tier that breaks ties on path evidence is worth building at all. Measured on a live instance at 159 keys over 41.7 hours and 147,535 packets: **0.253% ambiguous**, with explicit-over-derived at zero because that instance has exactly one derived key. ## Rule 8 `maxDerived` and `refreshMinutes` are configurable values and belong in the customizer eventually. Documented in `config.example.json` for now, flagged here so it is tracked rather than forgotten. ## Tests - default off, and a refresh that is a no-op while disabled - the name filter across both spellings, the wildcard, and each structural rejection - ranking by declarer count, then recency, then name, so the result is deterministic rather than churning between refreshes - `configured_scope` as a source, including that a node counts once per name and the newest answer wins - both sources merged, neither dropped - the explicit-over-derived tie-break - the derived tier replaced rather than merged on refresh, so a region that stops being declared leaves the key set and the cap keeps meaning something `cd cmd/ingestor && go test ./...` passes apart from `TestWriteStatsAtomic_SymlinkAtDestIsReplaced`, which needs `SeCreateSymbolicLinkPrivilege` and fails on Windows on master too. `go vet` and `gofmt -l` clean, `config.example.json` still parses. |
||
|
efiten
|
fd825a779c |
fix(ingestor): name a region scope deterministically, or not at all (#1988)
`matchScope` returns the first configured region whose derived code equals the packet's `code1` and stops there. Go randomises map iteration order per `range`, so when two configured regions collide on a payload, the stored region name depends on which key the runtime happened to visit first. **The same packet can be named differently on two runs of the same binary**, and neither answer is evidence of anything. ## How often this actually happens `code1` is two bytes, so any two configured regions collide on a given payload with probability 1/65536. That is a curiosity at 5 configured regions and routine at 150. Measured on a live instance carrying 159 configured regions, over 41.7 hours and 147,535 transport-scoped packets: **374 collisions, 0.253% of decisions.** They concentrate on four key pairs rather than scattering, because `code1` is an HMAC over the payload: a payload that collides collides every time it is seen, and a flooded packet is seen by many observers. The existing comment sizes the function for "≤ 50 regions". A live BE/NL instance declares 126 distinct region names across its repeaters, so operators are already past that. ## The rule `matchingRegions` returns every match; `matchScope` applies one rule: - exactly one match names the packet - several matches name nothing The candidates are equally sourced, there is no principled winner between them, and storing a wrong region name is worse than storing none. `""` is already the ingestor's "transport-scoped but unnameable" state (`scopeNameForDB`), so an ambiguous packet lands in a state the rest of the system already understands rather than in a new one. Nothing downstream needs to learn a new value. The collision is logged, because it is otherwise invisible: such a packet is stored exactly like one whose region this instance holds no key for. An operator watching an unnameable count grow deserves to see which of their own configured regions are colliding, since the fix is theirs to make. ## Rule 0 Cost is unchanged: the same single pass over the same keys, it just no longer stops early. The early exit was worth nothing on the common path, where zero or one key matches and the loop runs to the end either way. Worst case is unchanged at one HMAC per key per transport-scoped packet. There is no indexable shortcut to reach for. `code1` is an HMAC over the packet payload, so nothing is payload-independent to index on, and the old comment suggesting a "pre-indexed lookup table" is removed rather than left as a false lead for the next reader. ## Tests Three, and the fixture matters: - an unambiguous packet still gets its region name - a genuinely colliding payload stores the unmatched state instead of a coin flip - the ambiguous case run 50 times, because a first-match matcher passes a single iteration roughly half the time The collision is **found by searching payloads** (~65k tries, fractions of a second) rather than asserting on a hand-picked `code1`. The case only exists when the matcher genuinely finds two names for one packet, and a fabricated code would only prove the test agrees with itself. `cd cmd/ingestor && go test ./...` passes apart from `TestWriteStatsAtomic_SymlinkAtDestIsReplaced`, which needs `SeCreateSymbolicLinkPrivilege` and fails on Windows on master too. `go vet` and `gofmt -l` clean. |
||
|
efiten
|
0605b1703a |
feat(scope-audit): count and surface the traffic this instance cannot name (#1987)
Follow-up to #1986, and the second half of the same problem. `ScopeAuditForwarding` drops rows whose `scope_name` is the empty string with a bare `continue`. That empty string is the ingestor's "transport-scoped, but no configured region key matched `code1`" state (`scopeNameForDB`), so those packets name no region and can never satisfy a declared one. The consequence is on the page: **a repeater forwarding a region this instance holds no `hashRegions` key for is reported exactly like a repeater forwarding nothing at all.** The audit presents a gap in the reader's own configuration as a finding about someone else's hardware. This counts them per target, exposes the count as `observedUnmatchedPackets`, and renders it as a caveat chip beside the scope chips. ## Why it is not a rare edge Measured on a live instance before this landed: of 613 `notObserved` entries across 205 repeaters, **260 named a region that never appeared under any name in the whole 7-day window**. Two of them (`behss`, `fm-112`) were hash-verified as genuinely forwarded traffic the instance simply could not name: packet `0a065d41d51f1f77` decodes to `code1=9209`, which is exactly the code `#fm-112` derives over that packet's own payload. That instance had 16 region keys configured against 124 distinct region names its repeaters declare. A stock install has fewer. ## What the counter is not It is deliberately **not** folded into `unscopedPackets`. The two are opposites: | | meaning | what governs it | |---|---|---| | `unscopedPackets` | the packet carried no scope at all (`scope_name` SQL NULL) | the `*` wildcard | | `observedUnmatchedPackets` | the packet IS scoped, this instance holds no key for that region | nothing the repeater declares | For the same reason the new count never feeds `wildcardContradiction`, which counts only plain unscoped floods. `scopeNameForDB` in the ingestor is the source of truth for that three-state encoding, and the comments point there rather than restating it. It is also distinct from `ambiguousHops`, and the distinction is the point of the chip: that one is a pubkey-prefix collision between two repeaters and is nobody's fault, this one is a missing entry in the reader's own configuration and they can act on it. Saying which is which is what stops someone investigating an innocent repeater. ## Frontend The chip reuses the muted dashed treatment of `.sa-chip-ambiguous` on purpose: both are caveats on the row's finding rather than findings themselves, and neither may compete visually with the red/green scope chips beside them. It renders nothing for a non-numeric count. The value is server-supplied, and a truthiness check would put the literal string `NaN forwarded packets` on the page if that ever stopped holding. ## Docs `docs/api-spec.md` had **no entry for `GET /api/scope-audit` at all**, so this adds one: query parameter, full response shape, and the notes a client needs (the three traps the per-node endpoint documents apply here identically, `*` is never a scope, and "never asked" is not "declared nothing"). The new field is documented there rather than in isolation. ## Tests - the counter on a last-hop and on a mid-path hop - an unmatched packet enters neither `agg.scopes` nor `unscopedPackets`, which is the confusion this field exists to prevent - the field on the API row - six frontend cases: zero renders nothing, a missing field renders nothing (older server), the chip carries its count and class, singular and plural are both grammatical, the title names the cause and the fix, and a non-numeric count renders nothing rather than `NaN` `cd cmd/server && go test ./...` passes, frontend 712 assertions pass, `go vet` and `gofmt -l` clean. Rule 0: the counter is one increment on a branch that already existed as a `continue`, inside a loop this PR does not change. No new query, no new pass over the data. |
||
|
efiten
|
079e73aa4c |
fix(scope-audit): attribute forwarding to every hop, and pay for the wider scan (#1986)
The Scope Audit credits a transmission to `path[last]` only. On a
flood-family route every forwarder appends its own hash to the END of
the path (`internal/packetpath/route.go`), so `path[last]` does not mean
"forwarded this packet", it means "was the transmission an uplinked
observer heard directly". Every earlier hop forwarded the same packet
and is discarded.
The last-hop rule is genuinely required for DIRECT routes, which consume
hops from the front, so their `path[last]` is the route’s far end rather
than the transmitter. But `scopeAuditForwarderScanQuery` already
restricts to `route_type IN (0, 1)` via
`scopeConformanceForwarderRouteTypesSQL`, where that hazard cannot
arise, so inside this query the restriction only throws evidence away.
## What it costs the page today
Measured on a live-shaped instance, 206 declared repeaters, 965k
transmissions, 7d window:
| | before | after |
|---|---|---|
| repeaters with no attributable evidence of any kind | 133 of 205 (65%)
| 30 of 206 (15%) |
On a 1000-packet flood sample the mean path length is 7.08 hops, so the
last-hop rule keeps 394 of 2789 hop observations (14%), and 85% of the
nodes seen forwarding never appear as a last hop at all. Those repeaters
have every region they declare reported as "declared, not observed",
which is the page presenting a gap in our own attribution as a finding
about someone else’s repeater.
## Rule 0: what widening it costs, and what pays for it
Reading every hop multiplies the rows the scan returns: a 7d window
yields **3,470,188 hop rows** from 1,368,761 observations carrying a
path. Cold cost before this change was 16.7s for 7d and 4.0s for 24h, of
which SQLite accounts for 2.7s. The rest was the Go side reading rows.
Three changes, in order of what they bought:
1. **The scan carried `scope_name` and `first_seen` on every hop row.**
Both are columns of `transmissions`, and at 43 hop rows per transmission
the same two values were re-read that many times. They now come from one
query over the same window keyed by transmission id, both inside one
read transaction so a transmission arriving between them cannot appear
in the hop scan with no metadata to attribute it by. The hop scan
carries two columns instead of four.
2. **The hop is lower-cased into a stack buffer** instead of through
`strings.ToLower`. 1,026,814 of the 1,284,897 hops in a 24h window are
stored uppercase, because `packetpath.DecodePathFromRawHex` writes them
that way, and the great majority match no declared target, so that
allocation was paid millions of times to answer "no". The `(target,
txID)` de-duplication key became a struct for the same reason.
3. **The compute ran outside the cache mutex**, so every request
arriving on a cold window ran its own full scan concurrently. It now
sits behind a singleflight, the same treatment `/api/observers` and
`/api/nodes/{pubkey}/reach` already have, and the 7d window gets a 5
minute TTL while 1h and 24h keep 30s. At 30s a single reader with 7d
open keeps the instance recomputing more than half the time, for an
aggregate that moves at the pace of a week of traffic.
Result, warm process:
| window | before | after |
|---|---|---|
| 1h | 0.155s | 0.140s |
| 24h | 4.04s | 2.79-2.89s across six samples |
| 7d | 16.7s | 11.6s |
| repeat inside TTL | ~1ms | ~1ms |
**Rejected alternatives, measured on the same database**, so the next
reader does not have to re-derive them:
| approach | rows returned | time in SQLite |
|---|---|---|
| the query as written | 3,470,188 | 2.7s |
| pre-filter on the declared targets’ first 4 hex chars | 1,971,126 |
20.9s |
| `GROUP BY t.id, hop` | 965,025 | 38.0s |
| `SELECT DISTINCT t.id, path_json` | 1,229,966 | 17.7s |
The query plan is already index-driven (`idx_transmissions_first_seen`,
then `idx_observations_tx_ts`), so there is no missing index behind
this: the rows are inherent to the data. Note for anyone attempting a
hop comparison in SQL: a case-sensitive comparison silently drops most
attributable hops, per the 80% figure above.
## Tests
- a mid-path hop is attributed (the case behind the 65% blind spot)
- a DIRECT transmission whose `path[last]` **is** the target is still
not attributed. With the last-hop rule gone this is the only thing
standing between the audit and misattribution, so it gets its own test
rather than relying on the route filter being obvious
- one transmission counted once per target even when it appears on
several hops of the same path, which the `(target, txID)` de-duplication
now carries alone
- a hop longer than the 4-char floor resolved by its own length, which
nothing pinned before: every other test seeds 4-char hops
- the per-window TTL, so collapsing it back to one constant has to
delete the reason
- a second request inside the TTL served from cache rather than
recomputed. The cache path had no test at all
`cd cmd/server && go test ./...` passes (168s), `go vet` and `gofmt -l`
clean. Server-side only, no API shape change, no frontend change.
Browser validation: run against a live instance carrying this change,
the Scope Audit renders 220 rows matching the API row for row, and the
per-node scopes page still answers with its route-type mix.
|
||
|
efiten
|
2c8c1161b5 |
feat(#1975): network-wide Scope Audit page, fed by confirmed scopes (#1976)
One row per repeater whose configured region list is known, answering a question no other view answers: you declare these regions, but were you seen forwarding them? default_scope says what a node's adverts were observed under and transported_scopes (#1751) says what it carried, but nothing lined the declared list up against observed forwarding. The declared side merges every confirmed-scope source the database carries, newest answer per node wins, rather than naming one. On a stock install only nodes.configured_scope (#1865/#1971) exists and it degrades to the one-source case; deployments that collect the same fact another way keep working. Reading a single hard-coded source would have rendered an empty page on the very instance the evidence came from. Declared and observed are compared through normScope, so a leading "#" and a bare region name are one region. Unobserved regions render neutral, not red: absence over a short window is weak evidence, which the page header already states in words. Ported from a long-running fork deployment with its 17 server tests, rewired to the upstream data source, plus 14 frontend cases asserting rendered markup. |
||
|
efitenandClaude Opus 5
|
1ffaad8eb1 |
feat(#1794): per-IP limits and a deny list on the /ws upgrade (#1974)
Closes #1794. Follow-up to #1793, decided **before** the upgrade because the handshake is the resource being protected. - Deny list of addresses and CIDRs → 403 - Per-IP concurrent connection cap → 403 - Per-IP upgrade rate limit over a rolling minute → **429**, not 403: a temporary refusal should not read as "never come back" - Rejection counters split by cause in `/api/stats` under `websocket` ### The decision this feature lives or dies on Most CoreScope installs sit behind nginx, Caddy, Traefik or an ingress. `cdn_detection.go` says so in as many words: it deliberately excludes `X-Forwarded-For` from its CDN signals precisely because *every* reverse-proxied install sets it. For those deployments `r.RemoteAddr` is the proxy, `127.0.0.1` for every visitor on earth. A per-IP cap keyed on that address protects nobody and hands the sixth legitimate browser tab a 403. That is a self-inflicted outage wearing the costume of hardening. So: - **`X-Forwarded-For` is believed only from an address listed in `webSocket.trustedProxies`.** From anywhere else it is attacker-supplied, and trusting it would let anyone mint a fresh source IP per connection, which is strictly worse than having no limit at all. - **When the peer looks like a local reverse proxy and no `trustedProxies` is set, the per-IP limits are skipped**, and one warning names the setting that fixes it. Silently refusing real users is the worse failure. - **The deny list still applies there**, because it is the operator's explicit instruction rather than an inference. That is the answer to @mcode6726's question on the thread: it is neither "always the socket address" nor "always the header", and the operator decides which by naming their proxy. ### Two deliberate departures from the issue body **`maxConnsPerIP` ships as 0 (off), not 5.** Carrier-grade NAT puts thousands of unrelated mobile subscribers behind a single public IPv4. A cap of 5 refuses real visitors on phones while a scraper simply rents more addresses: all of the cost, none of the benefit. `upgradesPerMinPerIP` ships at **30 and on**, because that one *is* safe under CGNAT: a real client upgrades a handful of times per minute even while reconnecting, so 30 leaves ordinary traffic untouched while flattening a reconnect loop. A pointer type distinguishes "unset" from an explicit `0` that turns it off. **The default deny list is not shipped.** The thread proposed seeding 44 CIDRs for one VPS provider after a single scraper was seen at `23.111.177.6`. I have left it out: blanket-blocking a hosting provider by default breaks legitimate operators who host there, is undiscoverable by the person locked out (they see a bare 403), and ages badly as ranges get reassigned. The mechanism is here and `config.example.json` shows exactly how to configure it, so any operator who wants that list can have it in one line. If you want it shipped as a default anyway, that is your call as maintainer and it is a one-line change. ### Verification 19 tests, including all five the issue specifies as TDD requirements, each marked with the issue's own wording. Beyond those five: - a **bare address** in the deny list works, not just CIDR form. Operators write `1.2.3.4`, and silently ignoring that would be the worst possible failure for a deny list: it looks configured and blocks nothing - an unparseable deny entry is skipped and logged, not fatal. One typo must not take the server down - one client behind a trusted proxy does **not** exhaust another client's budget behind the same proxy, which is the entire point of honouring XFF - changing a forged XFF from an untrusted peer buys no fresh budget - `release` frees a slot and is **idempotent**, because `Unregister` can run twice for one client and double-crediting would leak slots - a **rejected** upgrade does not consume rate budget, or a retrying client could never recover once its window cleared - limits skipped for loopback and private peers; deny list applies anyway - a nil limiter allows everything, so a `Hub` built without `ConfigureLimits` behaves exactly as before - idle per-IP state is collected, while a record with a live connection never is Full `cmd/server` suite green, `gofmt` clean. ### Not done - No runtime config reload; restart required. Listed as optional in the issue. - No `WS_DENY_IPS` env override. Also listed as optional. - From the OWASP expansion in the first comment: `maxPayload` and the idle/read timeout are **already in master** (`SetReadLimit`, `SetReadDeadline`). The ping/pong heartbeat is not, and is not in this PR either; it is a separate change to the read/write pumps and belongs in its own review. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
efitenandSaarMesh-Bot
|
9d6f08c144 |
feat(#1865): ingest observer /neighbors as confirmed scope evidence (#1971)
Carries SaarMesh-Bot's implementation from the closed #1867 forward onto current master, 67 commits later, and surfaces the result on the per-node Reach report. The declared region list a repeater answers with is now stored on the node as configured_scope, normalised to the same leading-# syntax default_scope already uses so the two are directly comparable. That normalisation is the point @cwichura raised on #1865 and @dborup agreed with before the original PR closed. Co-authored-by: SaarMesh-Bot <300107934+SaarMesh-Bot@users.noreply.github.com> |
||
|
n30nex
|
cf67a5e5ec |
fix: remove evicted resolved path hops and preserve relay snapshots (#1966)
Eviction removes raw wire hops but leaves resolved full-key entries in `byPathHop`, retaining expired transmissions and stale relay counts/scopes. Filter every hop bucket once per eviction batch using the existing evicted-ID set, remove duplicate references and empty buckets, and clear discarded pointer slots. Bulk relay aggregation now owns its bucket snapshots before releasing the read lock, so eviction and raw-path updates cannot mutate an in-flight reader. Three existing handler test fixtures also wait for index readiness or explicitly simulate not-ready state, preserving their original 200/503 assertions. Fixes #1908. Validation: - Regression commits fail before their corresponding fixes: resolved keys/counts/scopes remain after eviction, and saved relay snapshots change during eviction. - Targeted eviction, relay, scope, cache and concurrent-reader checks pass under `-race`; coverage includes time/cap eviction, missing resolved-path prefetch, disabled membership indexing, duplicate references, retained backing arrays and surviving entries. - Local browser smoke: nodes, node details/path attribution and analytics render using the fixture-backed Go server. - The last full Windows server race run, before the final snapshot-copy correction, had one remaining DB-only timing failure (`TestGetChannelMessagesPerfLargeChannel`: 2.198s against a 1.5s budget). The final correction was checked with focused race tests. The unchanged ingestor suite also cannot create one symlink without Windows privileges. These thresholds/assertions were preserved; full Linux Go/E2E results still require upstream CI approval. Performance tradeoff: cleanup is O(total indexed pointers) per nonempty eviction batch, under the existing write lock. The minute-based ticker pays for one sweep instead of repeated scans of shared raw buckets. No per-transmission string index or dependency is added. Synthetic benchmark medians (three single-iteration runs, shared Windows host): | Transmissions | Evicted | Before | After | |---:|---:|---:|---:| | 30,000 | 1 | 1.07 ms | 14.19 ms | | 30,000 | 3,000 | 56.27 ms | 61.68 ms | | 30,000 | 7,500 | 83.58 ms | 65.54 ms | | 100,000 | 1 | 0.30 ms | 56.95 ms | | 100,000 | 10,000 | 949.93 ms | 190.71 ms | | 100,000 | 25,000 | 1,834.48 ms | 320.38 ms | Fixture: eight raw plus eight resolved hops per transmission, two observations, 2,048 relays; 480,000/1,600,000 hop entries. Timing includes acquiring the store lock and omits unrelated secondary indexes. Small batches now pay for the complete sweep; shared-host timing is noisy. Owning the bulk reader's arrays also has a measured cost on cold/bulk recomputation, rather than cached hits. Snapshot medians from three samples of ten iterations: | Transmissions / relay nodes | Before time / bytes per operation | After time / bytes per operation | |---|---:|---:| | 30,000 / 50 | 0.0068 ms / 5,416 B | 23.65 ms / 4,101,435 B | | 30,000 / 2,000 | 0.1374 ms / 196,768 B | 26.77 ms / 4,274,336 B | | 100,000 / 2,000 | 0.1376 ms / 196,768 B | 27.89 ms / 13,959,337 B | These are total snapshot costs, comparing the unsafe header-only snapshot with owned pointer arrays. Cleanup guarantees here apply to `byPathHop`; other indexes and existing periodic bulk-cache freshness are outside this change. Following #1922, this runtime fix is separate from the release-routing and frontend-runner PRs. Current Go and E2E job results should be assessed separately from workflow-approval or staging-runner state. |
||
|
efitenandClaude Opus 5
|
eb3d71f8f6 |
perf(#1910): collapse concurrent /stats work and serve the count cache stale (#1963)
Addresses #1910. The Observers page hangs on "Loading..." for 10-20s; the reporter measured `/stats` at 10-17s under the mixed load that page produces, while the same endpoint stays under 70ms at 8x concurrency when it is the only one being hit. ## Cause Two cache layers guard the expensive work and **neither has single-flight**: | | | | |---|---|---| | `handleStats` | 10s cache | releases `statsMu` before rebuilding (`routes.go:774`) | | `GetStoreStats` | 30s cache | releases `statsCacheMu` before scanning (`store.go:2048`) | Both do check, release, then work. The moment either window expires, **every in-flight request does the whole thing itself**. The expensive part is a range scan over 24h of `observations` with two `SUM(CASE...)` over it. The column is indexed (`idx_observations_timestamp`), but the scan still visits every row in the window, and at 18k observers that is millions. The pool is `SetMaxOpenConns(4)` (`db.go:111`), and the page fires stats, observers, nodes, channels and clock-skew at once, so one cache miss turns a single scan into a queue of them. That is exactly the reported profile: fast alone, slow only when mixed. ## Changes 1. **Single-flight both layers.** Concurrent callers that miss the cache wait for the first one's result instead of each running the same queries. 2. **Serve the observation counts stale while refreshing in the background.** An expired cache answers from the previous value and kicks off one refresh, so a miss is never a wait. Single-flight alone would not have fixed the hang: the first caller still waits for the full scan. The second change is what removes it. ## Contract change, stated plainly `TestGetStoreStats_CacheExpiry` asserted that an expired cache returns **fresh DB values on the same call**. It no longer does. For `packetsLastHour` / `packetsLast24h` on a dashboard, answering with a value up to ~30s older instead of blocking for seconds looks like the right trade to me. But that is a judgement, not a bug fix, and **a reviewer should be able to reject it**. I did not quietly delete the test: it now asserts what still has to hold, that the refresh happens, and the new behaviour is pinned separately by `TestGetStoreStats_StaleCacheServedWithoutBlocking`. If you would rather keep the old contract, drop change 2 and keep change 1; the diff separates cleanly. ## Verification The new test **fails without the change**: ``` stale cache not served: got (0, 2), want (424242, 434343). An expired cache must answer from the previous value and refresh in the background, not block the request on the observations scan ``` `gofmt` clean, `go vet` clean, `cmd/server` suite ok in 267s. ## Two things I did not verify **The race detector.** It needs cgo and there is no gcc on this machine, so `go test -race` cannot run here. This change adds a background goroutine writing the cache under `statsCacheMu`, so that check matters. CI runs `go test -timeout 20m -race` for `cmd/server` (`deploy.yml:134`), which covers it before merge. **The 10-17s itself.** I have no database with 18k observers. The mechanism above explains the reported profile, including why the endpoint is fast in isolation, but I did not measure the figure. @dborup, if you can run a build from this branch, the number to watch is `/stats` under the same mixed-load command from your issue. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
efitenandClaude Opus 5
|
71b892ba95 |
fix(#1899): scope the channel preview to the region filter (#1961)
Closes #1899. The Channels sidebar preview (`lastMessage` / `lastSender`) ignored the active region filter, so an operator filtering on their own region saw a preview line from a message their observers never heard. ## Cause `GetChannels` scoped `msg_count` and `last_activity` correctly: the outer query joins `observations` and `observers` and filters on IATA. The `sample_json` subquery that feeds the preview joined neither, so it always returned the globally newest message on the channel. ## Fix Both region-filtered branches now scope the subquery the way the outer query does: v3 through `observations`/`observers` on `observer_idx`, v2 through the `EXISTS` on `observer_id`. The unfiltered branch is untouched, since there is no filter for it to respect. **One thing that is easy to get wrong here:** the subquery sits in the SELECT list, *ahead of* the WHERE, so its placeholders bind first. The region codes are appended twice, subquery set first, or every filtered call binds the wrong values. **Scope checked rather than assumed:** `GetEncryptedChannels` has the same shape and the same `regionPlaceholder` pattern, but selects no `sample_json`, so it does not have this bug and is left alone. ## Verification The regression test **fails on unmodified master**, with the reported symptom: ``` db_test.go:2465: preview sender = Bob, want Alice: SJC must not be shown Bob's message, which only SFO heard db_test.go:2469: preview message = heard in SFO, want "heard in SJC" ``` It asserts both directions, so it cannot pass by always picking the oldest row, and it asserts the unfiltered call still shows the globally newest message, which was never in question. `gofmt` clean, `go vet` clean, `cmd/server` suite ok in 98.5s. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
efitenandClaude Opus 5
|
e7b3a2e77f |
chore(#1856): remove POST /api/packets, which has never worked (#1959)
Part 1 of #1856. Part 2 (the hash migration reporting false success) is #1958. ## It has never worked `handlePostPacket` writes to the server's DB handle, and that handle is read-only. `cmd/server/db.go:106`: ```go dsn := fmt.Sprintf("file:%s?mode=ro&_journal_mode=WAL&_busy_timeout=5000", path) ``` Every call answered `500 attempt to write a readonly database`. **This is the second report.** #1196 raised it on 2026-06-13, a fix was merged that corrected v2 column names to v3, and the issue was closed. That fix could not have worked, because the column names were never why the write failed. Its comment is still sitting at `routes.go:1288`, next to code that has never executed successfully in production. ## Why remove rather than build a handoff **It cannot break a caller.** An endpoint that has only ever returned 500 has no working consumer. This is not a breaking API change, it is documentation catching up with reality. Nothing in `public/` calls it. **It was actively misleading.** `openapi.go` advertised it as "Ingest a packet" and it sits behind `requireAPIKey`, which reads as a live, protected write endpoint. **Its test hid the breakage.** `TestPostPacketPersistsV3Schema` asserted the observation row is written and passed for four months, because the test DB is opened read-write while production is not. That is how #1196 came to be closed as fixed. **Ingest is MQTT-only by design since #1283.** Re-adding an HTTP write path re-opens the invariant that change established. If manual injection is wanted later for testing or replay, it belongs on the ingestor side and deserves its own issue. The repository already has the handoff shape for that: the server writes `request-<id>.json` and the ingestor consumes it (`cmd/ingestor/prune_geofilter.go`). ## What went The route, `handlePostPacket` (103 lines), the now-unused `PacketIngestResponse` type, the `openapi.go` entry, the round-trip test, and the section plus table-of-contents line in `docs/api-spec.md`. The `packetpath` import in `routes.go` became unused and went with it. `+4/-225` across 6 files. ## The auth tests The four `requireAPIKey` tests used `"/api/packets"` only as a request path while building their own handler with `s.requireAPIKey(...)`, so they never touched the route. I checked that by **running them**, not by reading the code: ``` --- PASS: TestRequireAPIKey_RejectsWeakKey --- PASS: TestRequireAPIKey_AcceptsStrongKey --- PASS: TestRequireAPIKey_EmptyKeyDisablesEndpoints --- PASS: TestRequireAPIKey_WrongKeyUnauthorized ``` Their paths now point at `/api/admin/prune-geo-filter`, which still exists, so they no longer name a removed endpoint. Re-ran after that change: still 4 of 4. `/api/packets/observations` is a different endpoint and is untouched. Verified: `gofmt` clean, `go vet` clean, `cmd/server` suite ok in 62.9s. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
efitenandClaude Opus 5
|
56d6d4c722 |
fix(#1856): stop the hash migration reporting success it never achieved (#1958)
Part 2 of #1856. **Part 1 is deliberately not fixed here** and the issue stays open for it; reasoning at the end. ## The bug `migrateContentHashesAsync` set `store.hashMigrationComplete` in a deferred func that ran unconditionally. Every DB failure inside the loop takes a `continue` (begin tx, prepare, commit), so the loop always reaches that defer, **including when not a single batch was written**. That is not hypothetical. The server has held a `mode=ro` handle since #1283, so `Begin`, `Prepare` and `Commit` all fail, every batch is skipped, and `/api/stats` then answers `hashMigrationComplete: true` after migrating nothing. The migration is started unconditionally on every boot at `main.go:546`. ## The fix The three failure paths now count, and the defer only claims completion when the count is zero. When it is not, it logs once, naming the read-only handle as the expected cause and pointing at this issue, so an operator can tell "no work to do" apart from "could not do the work". **Nothing waits on the flag.** The only reader is `routes.go:828`, which reports it in `/api/stats`. Leaving it false on failure blocks nothing; it just stops the endpoint from lying. The in-memory index is untouched on failure. That was already true, because the index update runs only after a successful commit, and the test now asserts it so memory and disk cannot drift apart. ## Verification The regression test **fails on unmodified master**: ``` hash_migrate_test.go:115: hashMigrationComplete must stay false when no batch could be written; reporting true here is what #1856 called self-reported success ``` It closes the DB handle to make writes fail. That is deterministic and exercises the identical path as a read-only handle (`Begin` errors, batch skipped); the in-memory test DB cannot be reopened read-only. The existing happy-path test still passes, so the flag still turns true on a real migration. `gofmt` clean, `go vet` clean, `cmd/server` suite ok in 59.7s. ## Why part 1 is not in here `handlePostPacket` writes to the same read-only handle and therefore always answers 500. I checked the error path before assuming it was misleading: it already returns `"transmission insert: attempt to write a readonly database"`, so the message is accurate. The endpoint is not confusing, it is simply dead. The issue asks maintainers directly: *"is this endpoint still wanted? If ingestion is MQTT-only now, deleting it is simpler than routing it through a handoff."* That is a product decision, not a fix, and inventing a middle answer would only add code without settling it. Worth noting the repository already has a precedent for the handoff shape: the server writes `request-<id>.json` and the ingestor consumes it (`cmd/ingestor/prune_geofilter.go`). Two things a decision should account for: the endpoint is documented in `openapi.go:69` and guarded by `requireAPIKey`, and `routes_test.go:4850` asserts it writes an observation row using the v3 schema, which passes only because the test DB is read-write. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
|
40f664c587 |
chore(#1859): gofmt sweep + gofmt/go vet CI gate (rebase of #1881) (#1941)
Rebase of #1881 by @SaarMesh-Bot onto current master. Their three commits are preserved, two of them cherry-picked with authorship intact; the sweep itself had to be regenerated. Opened as a new PR rather than force-pushing their branch. Closes #1881 once merged. Addresses parts 1 and 3 of #1859; part 2 landed as #1937. ## Why regenerated rather than merged The sweep in #1881 was cut on 2026-09-02 07:13 and roughly forty PRs landed after it, so it went `CONFLICTING/DIRTY`. Re-running `gofmt` on current master is cheaper and less error-prone than resolving 72 conflicts that are all whitespace. The drift it fixes also grew in the meantime: 66 files now, against 72 then, but spread differently. ## The three commits 1. **`style(#1859)`** — `gofmt -w` across the 14 modules. 66 files. 2. **`test(#1859)`** — @SaarMesh-Bot's fix for the one `go vet` copylocks finding, `cmd/ingestor/coverage_boost_test.go`: the range variable copied a `Config` embedding `sync.Once`. Cherry-picked unchanged. 3. **`ci(#1859)`** — @SaarMesh-Bot's CI step that fails on gofmt drift or vet findings, plus `.git-blame-ignore-revs`. Cherry-picked with one change, noted in the commit message: the ignore file pointed at `04bc80ee`, the sweep commit on their branch, which does not exist on this base and would make `git blame --ignore-revs-file` error. Repointed at `d3a02599`, the sweep here. ## Verification The claim "formatting only" is checked twice rather than asserted: - Every changed file is byte-identical to `gofmt(previous content)`. 0 of 66 deviate. - With line comments and all whitespace stripped, 0 of 66 files differ, so no code outside comments changed. 14 of the 66 also show doc-comment reflow. Since Go 1.19 `gofmt` re-indents indented comment blocks to tabs and inserts a blank comment line before them; the behavior matrix above `resolveHopWithContext` in `cmd/ingestor/path_resolver.go` is a clear example. That is gofmt's own output, not an edit, but it is worth naming because it makes the diff look larger than "whitespace" suggests. The gate was run locally exactly as the workflow runs it: `gofmt` clean, and `go vet` clean in all 14 modules, including `cmd/ingestor` which is what commit 2 fixes. Suites: `cmd/server` ok (80.7s), `internal/packetpath` ok (2.3s), `cmd/ingestor` passes except `TestWriteStatsAtomic_SymlinkAtDestIsReplaced`, which fails identically on bare master with "A required privilege is not held by the client" (Windows symlink privilege on my host, not code). ## Sequencing This should go last in the queue. The sweep touches 66 files, so merging it before the remaining open Go PRs gives each of them a conflict about nothing but formatting. After it lands the gate is active, and any PR with drift fails CI until it runs `gofmt -w`. Excluded from the sweep: the misnamed `Dockerfile.go`, which is a Dockerfile that gofmt cannot parse (the workflow excludes it too), and `docs/DEPLOYMENT.md`, which a case-insensitive filesystem surfaces as a spurious modification against `docs/deployment.md` and is unrelated. --------- Co-authored-by: SaarMesh-Bot <300107934+SaarMesh-Bot@users.noreply.github.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
efitenandClaude Opus 5
|
9ae3387416 |
feat(ingestor): RF environment samples from mobile clients (#1906)
> **Stacked on #1905.** This branch contains #1905's commits; review and merge that one first. The diff unique to this PR is the `client_rf_samples` table, its handler, the delta query and its retention. ## What Everything CoreDrive RX records today is anchored to a *packet*. But a drive also passes through RF conditions that exist whether or not a packet arrives: the noise floor, how busy the channel is, how many receptions fail CRC. The radio measures all three and was never asked. This samples the companion's own counters along the GPS track and stores them, so the server can render a noise-floor map, a channel-utilisation map and a CRC-error-rate map. A fixed observer cannot produce those — it measures one point forever. **Zero airtime:** `CMD_GET_STATS` is a local Bluetooth query to the attached radio. Nothing is transmitted. ## Design points worth knowing - **Absolutes are stored; deltas are derived at query time.** A lost or reordered sample then costs one interval rather than corrupting a running total. `ClientRfDeltas` breaks the chain whenever `uptime_secs` fails to increase — that is the reboot and counter-wrap detector. - **Absent is not zero, end to end.** Firmware predating the `recv_errors` field cannot count CRC errors at all, and a stored `0` would read downstream as "a perfectly clean channel" — the opposite of "we don't know". Presence/absence is preserved through the app parser, the wire payload, a nullable column, and the delta view, which returns `nil` rather than `0` when either endpoint is unknown. Each of those five layers has its own test. - **`sampled_at` is millisecond precision, and it is load-bearing.** SQLite compares these strings lexicographically and `.` (0x2E) sorts before `Z` (0x5A), so a second-resolution retention cutoff would delete rows *inside* the window. The prune formats its cutoff with the same layout. ## Performance justification (touches the ingest hot path) - One INSERT per sample, gated behind an opt-in flag that defaults off. Sample rate is 15 s while moving and 5 min while parked, so roughly 240 rows per hour per active driver. - The delta query is a single `LAG(...) OVER` pass with no nested query inside the loop, so it cannot deadlock the single writer connection. Window functions are already used elsewhere in this codebase. - Retention has its own key and index (`sampled_at`); without it the table would grow unbounded, so `config.example.json` documents it inline. ## Safety for existing deployments Opt-in and default off on both sides (`clientRfSamples.enabled`, and `rfSampler` in the app). The coverage path is untouched — `Publisher.buildPayload` is byte-identical and a record with no `kind` field still routes to `/packets` unchanged. The MQTT dispatch was reshaped so that **anything on `meshcore/client/…` returns from that branch in every config state**, with the enable-gates inside rather than in the topic match. Previously a disabled gate let the message fall through to the observer path, where `parts[1]` — the literal string `client` — was read as a region and the phone's pubkey registered as an observer. The blacklist check now also runs ahead of the sub-topic switch, so it covers every present and future client sub-topic. ## Testing Full ingestor suite green. Notable coverage: a `/rf` message with the gate off writes nothing anywhere and does not fall through; a sample missing `uptime_secs` is rejected rather than stored as an unusable row; two samples 40 ms apart remain two rows; and the retention test seeds a row with a non-zero millisecond component inside the cutoff second, which is the only row that distinguishes a correct cutoff from an RFC3339 one. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Joel ClawandJoel Claw
|
ac6fbaf9f3 |
perf: reuse ctx buffer in resolvePathForObs, cache ReadMemStats per store (#1873)
## Problem Three hot-path inefficiencies causing excess CPU and memory allocations: ### 1. `filterTxSlice` starts with nil slice `filterTxSlice` is called on the full `s.packets` slice (50k+ packets) for every query that doesn't hit a fast-path index. Starting with `var result []*StoreTx` means Go's append does ~15 growth+copy cycles (1→2→4→8→...→32768→65536) before reaching steady state. ### 2. `resolvePathForObs` allocates per hop Each hop in the path resolution loop allocates a new `ctx` slice (`make([]string, len(contextPKs), len(contextPKs)+2)`). For a 5-hop path, that's 5 allocations per observation. With 500+ observations per ingest batch, that's 2500+ small allocations. ### 3. `estimatedMemoryMB` calls `runtime.ReadMemStats` without caching `runtime.ReadMemStats()` triggers a STW (stop-the-world) pause. It's called from stats/debug endpoints (`GetStoreStats`, `GetPerfStoreStats`) that may be polled frequently. The routes.go layer already caches this with a 5s TTL, but the store layer doesn't. ## Fix 1. **Pre-allocate `filterTxSlice`**: `make([]*StoreTx, 0, n/2)` — the 2x over-allocation is cheaper than repeated growth+copy. 2. **Reuse ctx buffer**: Allocate one `ctx` buffer before the hop loop, reset to base length each iteration with `ctx = ctx[:ctxLen]`. 3. **Cache `ReadMemStats`**: 5-second TTL cache matching the routes.go pattern. Uses a package-level mutex (not on `PacketStore`) to avoid adding a field. ## Testing - `go build` passes - No behavior change — same results, fewer allocations --------- Co-authored-by: Joel Claw <358739783+Joel-Claw@users.noreply.github.com> |
||
|
efitenandClaude Opus 5
|
9e13e0b05f |
feat(ingestor): full-packet RF observations from mobile clients (#1905)
## What A CoreDrive RX drive already carries far more RF information than reaches CoreScope, and it was being discarded twice: once in the mobile app (every packet it could not attribute to a directly-heard node was dropped before queueing) and once here (the ingestor decodes the *complete* packet, then keeps only `heard_key`/`snr`/`rssi`/`lat`/`lon`). This captures what was being thrown away, at **zero extra airtime** — nothing new is transmitted. - **`transmissions.code1` / `code2`** — the transport codes were decoded on every packet and used only to derive `scope_name`, then dropped. Storing them turns "which repeater forwards which scope" from a re-parse into a query. - **An async backfill** re-parses the `raw_hex` already on disk, so months of scope history become queryable with no new data collection. - **`client_rx_observations`** — a new diagnostic table holding every decodable packet a phone heard, with route type, transport codes, scope name, path-hash size, the full forwarder chain and the forwarder. ## Why it is safe for existing deployments Both halves are **opt-in and default off** (`clientRxObservations.enabled`, and `fullRfLog` on the app side), so an existing deployment sees no behaviour change and no volume change on upgrade. The coverage invariant is untouched: `client_receptions` keeps its rule — 0-hop advert pubkey or FLOOD `path[last]`, ≥2-byte hash — and an unattributable packet writes **zero** coverage rows. `deriveHeardKey`, `buildClientReception` and `InsertClientReception` are unmodified except for one guard described below. ## Performance justification (touches the ingest hot path) - **Backfill:** keyset-paginated by `id` in 5000-row batches, a single forward scan, `rows.Close()` before `Begin()` so it never deadlocks against `SetMaxOpenConns(1)`, and commits per batch so live ingest interleaves. Termination is driven by rows *scanned*, not rows decoded — an earlier count-based loop would have stopped at the first batch containing an undecodable row and then written its completion guard, permanently stranding the rest. - **Guard row is written if and only if the loop ran to genuine exhaustion.** Every error path leaves it unwritten so the next startup retries. - **Per-packet cost:** one extra INSERT on the client topic when enabled, gated behind an opt-in flag. No new work on the observer path. - **New indexes** cover the prune (`rx_at`), the flood-grouping (`pkt_hash, rx_at`), the per-repeater query (`forwarder, rx_at`) and the scope query (`scope_name, rx_at`). Retention has its own shorter window — this table is diagnostic, not archival. ## Two firmware-derived correctness points - **`pkt_hash` is `ComputeContentHash()`**, byte-identical to `transmissions.hash`, so dark-traffic queries are a plain equality join rather than a translation layer. - **TRACE packets are refused.** TRACE repurposes the header path bytes as per-hop SNR values, so deriving a `heard_key` from them invents a node that never existed. `packetpath.PathBytesAreHops` existed but was never wired into the client path; it became reachable only because the app half now publishes packets it previously dropped locally. ## Testing Full ingestor suite green. Notable coverage: a FLOOD-routed TRACE writes zero coverage rows and NULL `forwarder`; a `direction: "tx"` message writes no observation; a DIRECT route never sets `forwarder`; two forwarder copies of one flood remain two rows; the backfill's multi-batch path is exercised with an undecodable row in the first page; and a forced error asserts the migration guard stays unwritten. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
efitenandClaude Opus 5
|
376c3e9f4a |
fix(packets): surface the transport region scope — detail pane row and a sortable Scope column (#1894)
## Summary `transmissions.scope_name` (#899) reached the database but never reached the UI. Two problems, one dead feature and one missing surface. ## 1. The detail pane's Scope row was dead `public/packets.js:3279` has rendered a **Scope** row since #899, gated on `pkt.scope_name != null`. It never fires in practice. `/api/packets` and `/api/packets/{id}` are served from the in-memory `PacketStore`. The store reads `scope_name` out of SQLite fine (`store.go:888`, `chunked_load.go:551` → `StoreTx.ScopeName`), but `txToMap()` did not put it in the JSON. Only packets old enough to have been evicted from the store — and thus served by the SQLite fallback in `db.go`, which does emit it — could ever show a scope. Verified against a live instance before the fix: ``` GET /api/packets/552e9687f1525537 → packet keys: ['_parsedPath','decoded_json','direction','first_seen','hash','id', 'observation_count','observations','observer_iata','observer_id', 'observer_name','path_json','payload_type','raw_hex','route_type','rssi','snr','timestamp'] ``` No `scope_name`. ### The NULL / "" distinction `StoreTx.ScopeName` was typed `string`, which collapses the two states the frontend distinguishes: | DB value | Meaning | UI | |---|---|---| | `NULL` | not transport-scoped | row hidden | | `""` | transport-scoped, region matched no configured key | muted "unknown scope" | | `"#be"` | matched region | the region name | `route_type` is **not** a usable proxy for that distinction: the ingestor writes NULL for a transport route whose `transport_code_1` is `0000` (`cmd/ingestor/db.go:1576` — `IsTransportScoped = route_type IN (0,3) AND Code1 ≠ "0000"`). So the field is now `*string`, with `nullStrPtr` preserving what `nullStrVal` collapsed. The two internal consumers (`TransportedScopes` #1751, `relayEntry.scope`) only care about non-empty named scopes and are unchanged in behaviour. ## 2. New: a Scope column on the packets table The scope was only reachable one packet at a time by opening the detail pane. It now has its own sortable column between Type and Observer, visible by default. The default view is **Group by Hash**, served by mappers that did not carry `scope_name` at all — so the column would have been empty in exactly the view most people look at. Both grouped paths now select and emit it: `groupedTxsToPage` in the store, and the dedicated grouped query in the DB fallback (v3 and legacy shapes). Rendering lives in `scopeCellHtml` (`public/app.js`, next to `transportBadge`) and is used on all three row-render sites — group header, expanded children, flat rows — so the column and the detail pane cannot drift apart. **Sorting** pins the empties last in both directions, as the nodes table already does for `default_scope`. Only ~8% of packets carry a scope, so an ascending sort would otherwise bury every scoped row under a wall of dashes. **Filtering**: `packet-filter.js` gains a `scope` field, so the cell is click-to-filter like Type and Observer, and `scope == "#be"` works in the filter bar. **Column prefs**: a `packets-known-cols` companion key. The `packets-visible-cols` array alone cannot distinguish "this column did not exist when you saved" from "you unchecked it", so any new column arrives silently hidden for every returning visitor. Keys absent from `known-cols` get the default treatment; keys the visitor actually hid stay hidden — there is a test for that second half specifically. ## Tests Each watched fail first. **Go** (`cmd/server/packet_scope_name_test.go`) - `txToMap` unit tests for all three states, including a JSON round-trip so a typed nil `*string` cannot pass as `null` - end-to-end through `/api/packets/{hash}` - `groupedTxsToPage` unit + end-to-end through `/api/packets?groupByHash=true`, across **both** the store-backed and DB-fallback paths - `transported_scopes_1751_test.go`: the "no scope" guard now covers both non-values (nil and a pointer to `""`) **Frontend** - `test-frontend-helpers.js`: `scopeCellHtml` three states + escaping - `test-packet-filter.js`: `scope` matching, case-insensitivity, and `FIELDS` registration - `test-packets-scope-column.js` (new Playwright e2e): header position, default visibility, one cell per row, em dash on non-transport rows, empties-last sorting, the Columns toggle, and the prefs backfill ## Verification Deployed and checked against a live instance: ``` /api/packets?groupByHash=true&limit=500 → scope_name present on 500/500, 59 with a matched region, 1 unknown-scope test-packets-scope-column.js → 7 passed, 0 failed cd cmd/server && go test ./... → ok ``` Two pre-existing failures, unrelated and equally red on an unmodified checkout: `test-e2e-playwright.js` "Customizer open does not overwrite server home config" and `test-observer-iata-1188-e2e.js` (timeout on `[data-loaded="true"]`). 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
efitenandJonathan Herlin
|
89544b1d08 |
perf: index, cache, and deflake /api/channels queries (rebase of #1887) (#1936)
Continues #1887 by @Jonher937. The commit is theirs, authorship unchanged; I only rebased it onto master. It went CONFLICTING because #1934 (prepared statements, originally @Joel-Claw's #1878) landed in the same `DB` struct. Both PRs add fields there and this one also replaces the single-slot channels cache. Resolution: kept this PR's keyed caches (`channelsCache`, `encChannelsCache`, `msgCache` plus their entry types and TTL constants) and kept master's thirteen prepared-statement fields alongside them. The old single-slot `channelsCacheKey`/`channelsCacheRes`/`channelsCacheExp` trio is gone, which is the point of this PR. Nothing else touched. Verified: `cmd/server` builds and the **full suite passes**, not just the channel tests. My review stands: approve, with two questions that do not block and are worth a look at some point. 1. `msgCache` is keyed by `hash|limit|offset|region`, and `offset` grows without bound as someone pages through a channel. Each entry also holds a full page of message maps, so a full 256-entry cache at `limit=50` holds around 12,800 maps. The other two caches are keyed by region only and genuinely low-cardinality as your comment says; this one is the odd one out. 2. `getMsgCache` returns the cached slice directly, so every hit hands the caller the same message maps. If any handler mutates one before serialising, it corrupts the cache for the next ten seconds. Same class as the finding on #1871, which was fixed there by copying at the two broadcast sites. Co-authored-by: Jonathan Herlin <jonte@jherlin.se> |
||
|
Jonathan Herlin
|
eb8f376c6c |
fix: use index from_pubkey in nodes region filter (#1882)
The region subquery in GetNodes was pulling the advert pubkey out of decoded_json with JSON_EXTRACT for every row the join touched, instead of reading the from_pubkey column that #1143 already added and indexed It looks like buildPacketWhere, GetRecentTransmissionsForNode, QueryMultiNodePackets etc. moved to from_pubkey already, but not this. |
||
|
Joel ClawandJoel Claw
|
4a776454ca |
perf: remove dead relayTimes field (#1931)
The `relayTimes` field (`map[string][]int64`) on `PacketStore` is never written to and never read. Its only references are the declaration at `store.go:180` and the `make()` in `NewPacketStore` at `store.go:644`. `relay_liveness_test.go` looks like a user at a glance but builds its own local `idx := make(map[string][]int64)` and passes that to `addTxToRelayTimeIndex`; the string "relayTimes" there is only inside a `t.Error` message. This is the surviving fragment of #1872, which no longer compiles after #1855 removed `lastSeenTouched` and `touchRelayLastSeen` from master. Verified against current master: both references are gone, build passes, all tests pass (32.2s). Co-authored-by: Joel Claw <358739783+Joel-Claw@users.noreply.github.com> |
||
|
efitenandClaude Opus 5
|
f081f91b88 |
fix(#1904): keep resolved full-pubkey hops across a path-hop index rebuild (#1907)
Fixes #1904. ## The bug `buildPathHopIndex` reassigned `s.byPathHop` to a fresh map and refilled it from every packet's raw `path_json` hops: ```go func (s *PacketStore) buildPathHopIndex() { s.byPathHop = make(map[string][]*StoreTx, 4096) for _, tx := range s.packets { addTxToPathHopIndex(s.byPathHop, tx) // raw hops only } ... } ``` `byPathHop` carries two kinds of key, though: those raw wire hops, and the resolved full pubkeys fed per observation by `indexResolvedPathHops`. The pubkey strings behind the second kind are retained nowhere — #800 replaced the per-`StoreTx` `ResolvedPath` field with a hash-only membership index (`resolvedPubkeyIndex` stores FNV hashes, not strings) — so the rebuild could not reproduce them and dropped them. All three call sites run post-load: `LoadChunked` (`chunked_load.go:459`), the background fill loader (`store.go:1573`), and the deferred startup build (`index_ready_1008.go:177`). The `resolved_path` branch of the chunk scan populates the index and is then silently undone a few hundred lines later, while the `resolved_path IS NULL` fallback right beside it is explicitly documented as "byNode ONLY — the resolved_path/path-hop indexes must NOT be populated here". The two branches disagreed about who owns the index. Consequence: after a cold start every lookup keyed by a node's full pubkey missed, so `relay_count_1h/24h`, `last_relayed`, `unscoped_relay_count_24h`, `transported_scopes` (#1751) and the usefulness Traffic axis all read zero until live ingestion slowly refilled the index. ## Evidence Fixture built from live data: 2512 nodes, 17,056 transmissions, 528,891 observations, 123,057 of them carrying a non-NULL `resolved_path`. ``` before [store] Built path-hop index: 2924 unique keys /api/nodes → 0 of 2000 nodes with transported_scopes 0 with relay_count_24h > 0 after [store] Built path-hop index: 3881 unique keys (172181 resolved-hop entries retained) /api/nodes → 726 with transported_scopes 741 with relay_count_24h > 0 ``` The 957 extra keys are the full pubkeys. ## The change `retainResolvedPathHops` re-merges the pre-rebuild map's entries that the raw-hop pass cannot reproduce. Entries are carried over **only for transmissions still in `s.packets`**. That filter is load-bearing rather than defensive. `removeTxFromPathHopIndex` strips raw hops only — it derives them from `txGetParsedPath` — and its companion `removeFromResolvedPubkeyIndex` cleans the hash index, not `byPathHop`. So evicted transmissions linger under their resolved keys, and the wipe this PR removes was the only thing that ever cleared them. Filtering on liveness keeps the index bounded by the eviction policy instead of converting that gap into a permanent leak. `TestBuildPathHopIndex_DropsResolvedHopsOfEvictedTx_1904` pins it. (The eviction gap itself is pre-existing and outside this change: between rebuilds, an evicted transmission still stays referenced under its resolved keys. Filed separately.) ## Perf `O(entries in prev)` with one scratch map reused across keys (`clear()` per key, the same idiom as `hopsSeen`), plus one `map[*StoreTx]struct{}` over `s.packets` for the liveness check. It runs only where `buildPathHopIndex` already ran — cold load and background-fill completion — never on an ingest or request path. Measured on the fixture above: index build stayed within the same `LoadChunked` step, 15.2s total for 17k transmissions / 527k observations. Memory: the retained entries point at transmissions already held by `s.packets`, so no `StoreTx` is kept alive beyond eviction; the cost is map/slice overhead for keys that the feature is supposed to have. ## Tests `cmd/server/pathhop_rebuild_1904_test.go`, red before / green after: 1. `TestBuildPathHopIndex_RetainsResolvedHops_1904` — a resolved full-pubkey key survives the rebuild alongside the raw hop. 2. `TestBuildPathHopIndex_DropsResolvedHopsOfEvictedTx_1904` — a resolved key whose transmission is no longer in `s.packets` is dropped, and the now-empty key is not left behind. 3. `TestBuildPathHopIndex_NoDuplicateOnRepeatedBuild_1904` — building twice does not double-append (`indexResolvedPathHops` dedups within a call, not across the several observations of one transmission, so `prev` can legitimately contain duplicates). ``` cd cmd/server && go test ./... ok github.com/corescope/server 85.5s go vet ./... clean ``` Frontend and ingestor suites are untouched by this change (Go server only, no `public/` files). ## Interaction with #1903 Both touch `byPathHop` semantics, so I verified them composed on the same fixture. With #1904 alone the resolved keys come back and #1902's prefix collision is plainly visible again (51% of 1-byte prefix groups reporting an identical scope set). With both: ``` f79616 BE repeater ['#be','#de','#eu','#nl'] relay24h=542 f752c2 DE/NRW repeater ['#de','#de-nw'] relay24h=343 f788ad BE repeater none relay24h=383 ``` Identical-set prefix groups fall to 8%, relay counts stay intact, and each node's scopes match what its own `resolved_path` rows say. The two changes are independent and compose cleanly. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
efitenandJoel Claw
|
2f711eb851 |
perf: use prepared statements for frequently-called server DB queries (rebase of #1878) (#1934)
Continues #1878 by @Joel-Claw, at their request. Both commits are theirs, authorship unchanged; I only rebased them onto master and resolved the conflict with #1909. ## The conflict, and how it is resolved Exactly the two places I named in the review on #1878: `OpenDB` and `Close()`. Both PRs rewrite them, and #1909 went first because it is the correctness fix. **`OpenDB`** — kept #1909's pinned-connection `detectSchema` and added this PR's `prepareStatements()` after it: ```go derr := d.detectSchema(ctx, sc) _ = sc.Close() if derr != nil { conn.Close(); return nil, fmt.Errorf("schema detection failed: %w", derr) } // Statements are prepared after schema detection so they can never be // compiled against a schema mode that turned out to be wrong (#1901). if err := d.prepareStatements(); err != nil { ... } ``` The ordering matters and is not arbitrary: preparing before detection would compile statements against a schema mode that #1909 exists to stop trusting. **`Close()`** — kept this PR's statement closing and **did not** restore the WAL checkpoint. #1909 removed it deliberately: the handle is `mode=ro`, so `PRAGMA wal_checkpoint(TRUNCATE)` can only ever fail with "disk I/O error (778)" and was emitting a misleading storage-fault line on every shutdown. That reasoning survives; the statement closing is added in front of it. ## Verification - Both commits cherry-picked onto `e5595ad9` - `cmd/server` builds - **Full `cmd/server` suite: ok, 0 failures** (not just the targeted DB tests — after master briefly went red today from a two-PR interaction, a full local run seemed worth the two minutes) ## Review points still open, none blocking From my review on #1878, unchanged by the rebase: 1. Every SQL string now exists twice, once prepared and once as the `stmtQueryRow` fallback literal, with nothing keeping them in sync. The fallback is genuinely needed — twelve test helpers build `&DB{conn: ...}` directly and never call `prepareStatements` — but a constructor for those helpers would remove the duplication. 2. `stmtCountObsLastHour` and `stmtCountObsLastDay` are byte-identical SQL. 3. `OpenDB` now refuses to start rather than degrading when a Prepare fails. Contained today, since none of the 13 prepared queries touch a schema-conditional column, but the failure mode changed. @Joel-Claw — your work, your credit. Ping me if you would rather take it back. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01Wzwr3eXseyNM7Xj598djjE --------- Co-authored-by: Joel Claw <358739783+Joel-Claw@users.noreply.github.com> |
||
|
efitenandClaude Opus 5
|
d821d9a390 |
feat(retention): add observerPurgeDays hard-delete for long-inactive observers (#1886)
## Problem `RemoveStaleObservers` only soft-deletes — it sets `inactive = 1` and the row stays forever. On a long-running deployment those rows just accumulate: on a two-year-old instance roughly 25% of the `observers` table was rows nobody can ever see again. There is currently no way to reclaim them. ## Fix A second retention stage. `PurgeStaleObservers` hard-deletes rows that are: - already `inactive = 1` (so the soft-delete stage owns the decision of *when* an observer goes stale), **and** - older than `retention.observerPurgeDays`, **and** - referenced by nothing. New config field `retention.observerPurgeDays`, default `0` = disabled. Existing deployments are unaffected until they opt in. Set it above both `observerDays` and `packetDays` — below those the reference guards keep every candidate row anyway. ## Why the reference guards are the point `observations.observer_idx` is a bare rowid with no foreign key. Deleting a still-referenced observer silently orphans history — `packets_v` stops resolving the observer and those packets get mis-attributed. Nothing errors; the data just quietly goes wrong. So the statement guards on all three referencing tables: ```sql AND NOT EXISTS (SELECT 1 FROM observations o WHERE o.observer_idx = observers.rowid) AND NOT EXISTS (SELECT 1 FROM observer_metrics m WHERE m.observer_id = observers.id) AND NOT EXISTS (SELECT 1 FROM dropped_packets d WHERE d.observer_id = observers.id) ``` This is correctness, not defensive padding — it was found the hard way, by orphaning 280 observation rows during a manual purge that skipped one of these checks. Each guard has its own test. ## Performance Each `NOT EXISTS` is an index seek per candidate row (`idx_observations_observer_idx`, `idx_dropped_observer`, the `observer_metrics` PK), and `observers` is O(100). It runs on the existing daily retention tick alongside `RemoveStaleObservers`, never on the ingest path. ## Tests Eight tests in `cmd/ingestor/observer_purge_test.go`, written before the implementation: - deletes an unreferenced stale row - keeps a row referenced by `observations` — and asserts zero orphans afterwards - keeps a row referenced by `observer_metrics` - keeps a row referenced by `dropped_packets` - keeps a row that is old enough but still `inactive = 0` - keeps a row inside the retention window - no-ops when disabled (`0` and `-1`) - config accessor table test ## Invariant Writes stay in `cmd/ingestor` per #1283. `cmd/server/readonly_invariant_test.go` now also forbids `PurgeStaleObservers` as a method on the server's `*DB`. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
TeTeHackoandClaude Opus 5
|
97b6090344 |
fix(hash-size): key the zero-hop advert skip on the path byte, not the route type (#1913)
## Summary `computeNodeHashSizeInfo` skips zero-hop direct adverts by **route type**. It should skip them by the **content of the path byte**, because the two cases are no longer the same thing. A zero-hop direct advert carries no path, so its hop count is 0. Whether the two size bits next to it mean anything depends on the sender: - Firmware that predates [meshcore-dev/MeshCore#3293](https://github.com/meshcore-dev/MeshCore/pull/3293) does `packet->path_len = 0` in `Mesh::sendZeroHop()`, wiping the whole byte including the size bits. `0x00` genuinely says nothing about the node's `path.hash.mode` — skipping it is right, and #649 was right. - A sender that writes the size through `setPathHashSizeAndCount()` emits `0x40` (2 bytes) or `0x80` (3 bytes) with a zero hop count. On a zero-hop packet nothing else can set those bits, so they are a deliberate declaration. #653 landed the skip as `pathByte & 0x3F == 0`, which swallows the second case too. The diagnosis in #649 had actually proposed `pathByte == 0x00`; the review widened it on the reasoning that a zero hop count always implies zeroed size bits. That was true in April, when no firmware wrote them. It is not true now. On the Czech mesh (869.4 MHz), a 24h window of 10k packets holds **54 zero-hop direct adverts: 39 at `0x00` and 15 carrying a declared size** (14× `0x40`, 1× `0x80`). ## Why it matters for display, not just tidiness Measured on one node over a 7-day window. A companion was reconfigured from a 2-byte to a 3-byte path hash. Its first advert under the new setting was a zero-hop direct one on **24 Aug 15:36 UTC** declaring `0x80`. That packet was dropped, so the node kept reading as 2-byte until its next **flood** advert arrived on **25 Aug 10:18 UTC** — 18h42m serving a configuration the analyzer had already been told was stale, confirmed against both an unpatched and a patched instance. With local adverts typically every 2h and flood adverts every 25h, that gap is the normal case rather than a corner one. It bites hardest on an instance whose retention window is shorter than a flood advert interval: there the node has *no* countable advert at all and falls out of `hash_size` entirely (which is what #1912 is about on the rendering side). ## Change `(pathByte & 0x3F) == 0` → `pathByte == 0x00`, in `computeNodeHashSizeInfo` and in `computeAnalyticsHashSizes` so the two views agree. `isZeroHop` renamed to `isUndeclaredZeroHop` in the latter, since that is now what it means. No complexity change — same single byte comparison inside the existing scan. ## Measured A/B Two builds of the **same commit**, one with the change, both run read-only against the same copy of a real 181k-transmission / 973-node database: | | baseline | patched | |---|---|---| | nodes changed | — | **1** | | nodes regressed | — | **0** | | `hash_size_inconsistent` | 6 | **6** | | `multi_byte_status` split | 726 / 161 / 86 | unchanged | The flip-flop flag not moving is the point worth checking: a node that legitimately changes its mode mid-window is still handled by the recency decay from #1788, so reading these packets does not resurrect false "varies". ## Tests `cd cmd/server && go test ./...` → **ok**, 0 failures. Coverage 83.5%, unchanged from master. 5 new cases in `cmd/server/zerohop_hashsize_test.go`, two built from real off-air packets: - zero-hop DIRECT `0x40` → `HashSize 2` (was: dropped) - zero-hop DIRECT `0x80` → `HashSize 3` - zero-hop DIRECT `0x00` → still absent from the map, i.e. #649's behaviour preserved - TRANSPORT_DIRECT at path-byte offset 5, declared vs wiped - the declared size reaching `computeMultiByteCapability` as `confirmed`, which is what the map's multi-byte overlay reads **One existing test changed, flagging it explicitly:** `TestHashSizeTransportDirectZeroHopSkipped` used `0x40` as its "should be skipped" fixture. It now uses `0x00` — the case it was written to cover, since #747 was about the missing `RouteTransportDirect` skip rather than about the size bits. The `0x40` case is covered by the new tests with the opposite expectation. ## Deliberately not touched The decoders (`cmd/server/decoder.go:648`, `cmd/ingestor/decoder.go:1045`) still report `HashSize 0` for these packets, so per-packet views keep showing the size as unknown. Arguably they should follow the same rule, but that changes packet display rather than node attribution and felt like a separate call for you to make. ## Caveat worth stating This attributes a declared size to the pubkey inside the advert. That holds as long as the advert was transmitted by the node that owns it — the same assumption the existing zero-hop **flood** path already makes, so this change does not widen it. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
efitenandClaude Opus 5
|
e5595ad92f |
fix: unbreak master — decouple the pathTrust builder test from the default (#1932)
**master is currently red.** This is the fix.
```
--- FAIL: TestNeighborEdgesBuilderPathTrustExcludesOneByte
neighbor_builder_test.go:301: 1-byte hop must not produce an edge under
the default threshold, got 1
```
## What happened
Two PRs that were each green on their own:
- **#1929** moved `DefaultMinHashBytesForMapping` from 2 to 1.
- **#1930** carries `TestNeighborEdgesBuilderPathTrustExcludesOneByte`,
written when the default was 2.
Neither pipeline saw the other, because a `pull_request` run tests the
merge commit as it stood when that run started. Both merged, and the
combination fails. My mistake for merging them in the same batch without
re-running one against the other.
## The fix
The test passed `nil` for the trust config and leaned on the package
default being 2:
```go
// nil == package default (MinHashBytesForMapping = 2).
store.buildAndPersistNeighborEdges(nil)
```
That coupling is the real defect. The test is about what happens **at
threshold 2**, not about what the default happens to be. It now says so:
```go
trust := &packetpath.TrustConfig{MinHashBytesForMapping: 2}
store.buildAndPersistNeighborEdges(trust)
```
It keeps testing exactly what it was written to test, and stops breaking
when the default moves. The sibling
`TestNeighborEdgesBuilderPathTrustAllowsTwoByte` already passes its own
fixture explicitly, so this brings the two into line.
**No production code changed.** `cmd/ingestor` PathTrust and Neighbor
tests pass.
## Worth recording
This is the failure mode I have been flagging on other PRs all day, and
I walked into it myself: green CI on a PR is a statement about the base
it was tested against, not about master. Two PRs can each be green and
still be red together. Nothing about the review process would have
caught it — only re-running one against the other, or a merge queue,
would.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
https://claude.ai/code/session_01Wzwr3eXseyNM7Xj598djjE
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
|
e8f32df4dc |
feat(#1784): gate ingestor neighbor-edge creation on the path-trust threshold (rebase of #1863) (#1930)
Continues #1863. Three of the four commits are @Saarlandpower's and @SaarMesh-Bot's, authorship unchanged. The fourth is mine and is explained below. ## Why a rebase was needed #1863 was stacked on #1824, and #1841 merged instead. Both carried the same pathTrust base from different commits, which is why the two conflicted while each reported MERGEABLE against master. Cherry-picking #1863's own three commits onto master applied cleanly with no conflicts, which confirms its actual work was always independent of that duplicated base. ## The fourth commit, and a correction to something I got wrong The three commits do not build on master: ``` cmd/ingestor/main.go:455:23: cfg.GetPathTrust undefined (type *Config has no field or method GetPathTrust) ``` **#1824 added the pathTrust config and helper to both `cmd/server/config.go` and `cmd/ingestor/config.go`. #1841 carried only the server half** — one of its own commits is titled "remove ingestor side". I then closed #1824 as superseded by #1841, which is true for the server side and wrong for the ingestor side. Master has no pathTrust code in `cmd/ingestor/config.go` at all. The fourth commit restores that half, unchanged from `beae2c1c`: the `packetpath` import, the `PathTrust` field, the `PathTrustConfig` alias, `GetPathTrust`, and `cmd/ingestor/config_test.go` verbatim (28 lines covering the default, an explicit value, and a nil `*Config` receiver). That code is @Bjorkan's and @SaarMesh-Bot's from #1824, not mine; I only put it back. ## Verification - All three original commits cherry-picked onto `b3a306b8` with **no conflicts** - `cmd/ingestor` builds, and its `PathTrust|Neighbor|Config` tests pass - `cmd/server` `Neighbor|PathTrust|AnonReq|Edge` tests pass ## Interaction with #1929 #1929 moves `DefaultMinHashBytesForMapping` from 2 to 1. With that in, this PR's ingestor gate is a no-op by default and only takes effect when an operator sets `minHashBytesForMapping` to 2 or 3, which is the opt-in shape #1784 asks for. The two are complementary; merge order between them does not matter. @Saarlandpower @SaarMesh-Bot — your work, your credit. Say the word and I will close this and hand the rebase back, or push it to the #1863 branch if you would rather that stayed the vehicle. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01Wzwr3eXseyNM7Xj598djjE --------- Co-authored-by: Saarlandpower <Mail@mathiaskasper.de> Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: SaarMesh-Bot <300107934+SaarMesh-Bot@users.noreply.github.com> |
||
|
|
1720060284 |
fix(#1827): avoid per-observation SQL fetch in handleObserverAnalytics hot loop (#1829)
## Summary Fixes the CPU/DoS issue in #1827: observer detail pages were saturating CPU on busy observers — 6-7 concurrently loaded tabs pegged 12 cores for seconds, and auto-refresh made it self-sustaining. ## Root cause `handleObserverAnalytics` iterated every observation in the requested window and called `enrichObs()` per observation just to read `payload_type` and `decoded_json` for the `packetTypes`/`nodesTimeline` aggregates. `enrichObs()` also runs an on-demand SQL `SELECT resolved_path FROM observations WHERE id=?` (`fetchResolvedPathForObs`) and builds a full response map — both of which are unused by this aggregation loop. `resolved_path` is only actually consumed by the `<=20` kept `recentPackets` entries. Per the triage in #1827 (@carmack): *"Replacing `enrichObs(obs)` with a direct `s.store.byTxID[obs.TransmissionID].PayloadType` read (as sketched in the body) drops a map alloc + interface boxes per obs on the loop that saturated the operator's 12 cores. Byte-identical output. That's ~90% of the value."* This PR implements exactly that fast-path. ## Change - Aggregate loop (`packetTypes`, `nodesTimeline`): read `payload_type`/`decoded_json` directly off the transmission via `s.store.byTxID[obs.TransmissionID]` — no SQL, no per-obs map allocation. - `recentPackets` (`<=20` entries): unchanged, still calls `enrichObs()` since it needs `resolved_path`/`raw_hex`/etc. for display. - Output is unchanged: `packetTypes`/`nodesTimeline` are computed from the exact same underlying fields (`tx.PayloadType`, `tx.DecodedJSON`), just without the O(N) SQL round-trips. ## Scope This is the concrete hot-path fix from #1827's triage — not the broader `/api/observers/{id}/analytics` endpoint-split proposal in #1828, which (per that issue's discussion) is a separate P3 follow-up. #1828's own triage converged on this same `byTxID` fast-path as "the ground-work minimum" before any endpoint splitting. ## Testing - Existing `TestObserverAnalytics` passes unchanged. - Extended `TestObserverAnalytics/default` to assert `packetTypes` counts come out correct (`{"4":2,"5":1}` for the seeded fixture) via the new `byTxID` path, and that `recentPackets` still carries `resolved_path` where present (confirming the `enrichObs()` path for those 20 entries is untouched). - `go build ./...` and `go vet ./...` clean in `cmd/server`. - Full `go test ./...` in `cmd/server`: passes except 4 pre-existing test-order-dependent failures in `TestHandleNodePaths_*` (unrelated to this change — reproduced identically on a fresh, unpatched clone of `upstream/master`). --------- Co-authored-by: SaarMesh-Bot <300107934+SaarMesh-Bot@users.noreply.github.com> Co-authored-by: Claude <noreply@anthropic.com> |
||
|
|
1441734991 |
fix(#1901): detectSchema fails loud instead of caching wrong schema mode (#1909)
Fixes #1901. Thanks to @MarekWo for the exceptionally thorough report — root cause, repro, and a prioritised fix checklist in one. This implements it. ## Problem `detectSchema()` swallowed any probe-query error with a bare `return`, so a single transient failure of the first `PRAGMA table_info(observations)` at startup left `isV3` (and the feature flags) at their zero value **for the entire process lifetime**. The server then ran v2 SQL against a v3 DB: Packets page empty, `/api/channels/<name>/messages` → 500, logs full of `no such column: o.observer_id`, while the database was perfectly healthy. Nothing re-checked the flag, so only a manual restart recovered it. ## Fix Works through the report's checklist: - **Don't swallow the error.** `detectSchema` now returns `error` and `OpenDB` aborts on it. `main.go` already `log.Fatalf`s on an `OpenDB` failure, so the supervisord/Docker restart policy retries and a transient cause clears on the next attempt — strictly better than serving a broken read API. - **Log the mode unconditionally** — `[db] schema mode: v3 (observer_idx)` / `v2 (observer_id)`. A clean startup log is now positive evidence detection ran, not just an absence of errors. - **Run detection on a single pinned connection** (`conn.Conn(ctx)`) rather than an arbitrary pooled one, so the startup race in the report's hypothesis can't quietly hand detection a fresh, not-yet-openable handle — if the connection can't be acquired, we fail loud. - **`Close()` no longer checkpoints the read-only handle.** `PRAGMA wal_checkpoint(TRUNCATE)` on a `mode=ro` connection always failed with `disk I/O error (778)` and looked like a storage fault on every shutdown (the report's aside). The ingestor (the writer) owns WAL checkpointing. The three near-identical PRAGMA scan loops are consolidated into one `schemaColumns()` helper that returns errors instead of ignoring `Scan` failures. ### On the "single source of truth" item The report suggests deriving `isV3` from `dbschema.TableHasColumn(...)`. I kept the PRAGMA-scan structure here because `detectSchema` sets six flags from three tables in a single pass; swapping to `TableHasColumn` would mean six separate probe calls and wouldn't actually be cleaner. The goal it was aimed at — never cache a false negative — is met by making the existing scan fail loud. Happy to switch to the single-probe-per-column shape if you'd prefer it. ### Honest note on the connection `conn.Conn(ctx)` pins *a* single connection for all four probes and fails loud if it can't be acquired; it does not guarantee the literal connection `Ping()` validated (`database/sql` doesn't expose that). The fail-fast is what actually closes the bug — a mis-detected schema aborts startup instead of persisting for the process lifetime. ## Tests - `TestDetectSchemaFailsLoudOnProbeError` — injects a probe failure through a `rowQuerier` and asserts the error propagates and `isV3` stays unset (the invariant the old bare-`return` violated). - `TestDetectSchemaV3AndV2` — covers both schema shapes through `OpenDB`. `go vet ./cmd/server` and `go build` are clean; targeted `go test -run 'DetectSchema|OpenDB'` is green. Heads-up on the full `go test ./cmd/server` run: a handful of `TestHandleNodePaths_*` / `TestHandleAnalytics*` tests return `503 index loading`, plus one intentional panic test — these fail identically on pristine `master` (`a06ac8ac`) with this branch stashed, i.e. they're pre-existing/timing-related and untouched by this change. Out of scope (per the issue): frontend behaviour when the API 500s. 🤖 Authored with [Claude](https://claude.com) · Co-Authored-By trailer on the commit. Co-authored-by: SaarMesh-Bot <300107934+SaarMesh-Bot@users.noreply.github.com> Co-authored-by: Claude <noreply@anthropic.com> |
||
|
Jonathan Herlin
|
647841c990 |
fix: stop watchdog force-reconnect from racing paho's own retry loop (#1897)
Relates to #1335, which was already closed by PR #1336 shipping the naive `client.Disconnect(250); client.Connect()` force-reconnect. That fix has its own bug: liveness.IsConnectedFn (paho's IsConnected()) reports true for the entire time paho is actively retrying, not just when genuinely connected, so the watchdog's stall check cannot tell a half-open TCP socket (the original #1335 case) from a broker that paho is already correctly reconnecting to. Unconditionally calling Disconnect(250) then Connect() on that second, transitional case races paho's status machine and permanently kills its retry loop, requiring another watchdog trigger to recover, sometimes compounding into 100+ minute outages. This is a different failure mode from #1749/PR #1853: that bug is a blocking log.Print() write freezing the entire watchdog loop before ForceReconnectFn is ever called. This bug only manifests once ForceReconnectFn does fire, so the two fixes are independent and touch disjoint files. buildForceReconnectFn now gates Disconnect() on IsConnectionOpen() (true only when status is strictly connected) so it only tears down a genuinely open connection, and logs Connect()'s error token instead of discarding it. |
||
|
|
0d6f59ab2d |
fix(#1864): decode ANON_REQ source pubkey instead of treating it like REQUEST (#1866)
Fixes #1864. ## Problem `PAYLOAD_TYPE_ANON_REQ` was effectively treated like `REQUEST`. The two differ on the wire: ``` REQUEST : <dest hash 1B> <source hash 1B> <hmac 2B> <encrypted> ANON_REQ: <dest hash 1B> <source pubkey 32B, full> <hmac 2B> <encrypted> ``` The decoders read the right bytes but surfaced the sender key as `ephemeralPubKey`, which meant: - `store.go`'s node indexer keys on `pubKey`/`destPubKey`/`srcPubKey`, so ANON_REQ packets were **not** indexed — they didn't show up on a node's packet view; and - the packets list "details" rendered a bare `anon → <destHash>`, throwing away the sender identity the packet actually carries. - the detail side-view byte breakdown fell through the REQ catch-all, mislabelling a nonexistent 1-byte "Src Hash" and placing MAC/Encrypted-Data at the wrong offsets (`+2`/`+4` instead of `+33`/`+35`). ## Fix **Backend** (`cmd/ingestor` + `cmd/server` decoders) - Surface the ANON_REQ sender key as `srcPubKey` (json) so it's indexed and resolvable. The frontend keeps a legacy `ephemeralPubKey` reader so packets decoded before this rename still resolve — no DB migration needed. - `TestDecodeAnonReqValid` now asserts the full 32-byte `srcPubKey`. **Frontend** - `hop-resolver.js`: new O(1) `nameForKey(pubkey)` using the existing `pubkeyIdx` (all nodes). - `getDetailPreview`: resolve the source pubkey to a node **name** when known, else show the first 8 hex chars — no more bare `anon`. - Detail side-view: explicit ANON_REQ breakdown — `Dest Hash (1B)` | `Src Public Key (32B)` (node-linked) | `MAC @+33` | `Encrypted Data @+35`. - Detail header `srcLabel` falls back to the resolved ANON_REQ sender. All rendered names are `escapeHtml`-wrapped. ## Testing - `go test ./...` green for both `cmd/ingestor` and `cmd/server` (incl. strengthened `TestDecodeAnonReqValid`). - `node --check` on `packets.js`; brace/paren balance + markers verified on `hop-resolver.js`. - No HTML sink lines added → XSS preflight gate unaffected; every interpolated name is escaped. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: SaarMesh-Bot <300107934+SaarMesh-Bot@users.noreply.github.com> Co-authored-by: Claude <noreply@anthropic.com> |
||
|
|
52d08214bb |
fix(#1749): decouple watchdog emit from blocking I/O (root cause) (#1853)
Closes the gap left by #1810: that PR added defer/recover around the watchdog per-source work so a **panic** inside emit cannot kill the loop, but the actual production incident is caused by emit **blocking**, not panicking. ## Root cause In production `emit` is `log.Print`. `log.Print`'s underlying `write()` can block indefinitely if the sink is backpressured (Docker JSON-file log driver falling behind under load, a full stderr pipe, journald hiccups, etc.). A blocked syscall is not a panic -- `recover()` does nothing for it. Because emit was called **synchronously** inside the per-source work, a single stuck `write()` froze the entire tick loop forever -- no further source was ever checked and no further tick was ever processed again. This exactly reproduces the original #1749 incident even after #1810 landed: 3 independent MQTT sources going silent within ~60s of each other (one shared dependency -- the watchdog goroutine itself -- died, not 3 independent paho clients), zero WATCHDOG log lines for the rest of the 75-minute window, every other goroutine in the process continuing to run fine (a hang, not a crash), and only a full container restart recovering it. ## Fix `newAsyncEmit` decouples "decide to log" from "perform the write": the watchdog loop now only ever does a non-blocking channel send. A single background goroutine drains the channel and performs the (potentially blocking) write. If that goroutine itself gets stuck, the bounded queue (256) fills and further sends are dropped -- counted via the new `WatchdogLogDropCount`, surfaced through `/api/mqtt/status` and the ingestor stats snapshot alongside `WatchdogLastTickUnix` / `WatchdogPanicCount`. Worst case under a persistent backpressure event is now lost log lines (visible and counted), not a silently dead watchdog (invisible and undetectable -- the actual #1749 failure). ## Tests - `TestNewAsyncEmit_NeverBlocksWhenWriterStuck_1749` -- floods emit() past queue capacity while the writer is permanently blocked; every call must return immediately and drops must be counted. - `TestMQTTStallWatchdog_LoopSurvivesStuckWriter_1749` -- end-to-end, wires `runLivenessWatchdogLoop` exactly as production does (via `newAsyncEmit` around a permanently-blocking `realEmit`) with 3 registered sources, reproducing the incident shape and asserting the loop keeps ticking regardless. - `TestRunLivenessWatchdog_ProductionWiringUsesAsyncEmit_1749` -- smoke-tests the real entrypoint starts, ticks, and stops cleanly. - `WatchdogLogDropCount` round-trip tests in both the ingestor stats snapshot and the server's `/api/mqtt/status` handler, mirroring the existing `WatchdogPanicCount` coverage from #1810. All pre-existing watchdog/liveness tests (#1749, #1810 r1, force-reconnect) continue to pass unmodified; full ingestor suite green (verified 5x consecutive runs for flake-freedom). Note: the server package has pre-existing test-suite-wide flakiness in unrelated `TestHandleNodePaths_*` tests (confirmed reproducible on unmodified master too, non-deterministic which subset fails per run) -- unrelated to this change and out of scope here. --------- Co-authored-by: SaarMesh-Bot <300107934+SaarMesh-Bot@users.noreply.github.com> Co-authored-by: Claude <noreply@anthropic.com> |