Repository navigation
port(upstream): 26 clean upstream fixes — prune batching, /ws limits, observer liveness, watchdog race - #25
Closed
adminopenclaw8-sketch wants to merge 26 commits into
Closed
adminopenclaw8-sketch wants to merge 26 commits into
adminopenclaw8-sketch wants to merge 26 commits into
Conversation
…a-clawbot#1885) ## Problem The broker replays every retained `status` message on subscribe, so each ingestor restart pushes all of them through the status path in `handleMessage`. That path stamps `last_seen` with `time.Now()` (deliberately, per Kpa-clawbot#1465). Observed on a live deployment on 2026-08-11: **23 observers all carried `last_seen = 2026-08-06T08:20:09Z`** — 15 seconds after container start — and 18 of them had sent no actual packet in over a month. Their retained publish dates lined up almost 1:1 with `last_packet_at`, i.e. the replay was their only sign of "life": | observer | last real packet | retained status published | |---|---|---| | ON8AR - Observer | never | 2026-03-20 | | BE-BGS-RRY120-RES | never | 2026-04-02 | | A3BEF374 | 2026-04-30 | 2026-04-30 | | BE-BGS-RRY120-RUDY | 2026-05-18 | 2026-05-18 | | BE-JBE-ETG-O1 | 2026-06-10 | 2026-06-10 | That makes dead observers immortal, three ways per restart: 1. `last_seen` jumps forward, so `RemoveStaleObservers` can never age them out as long as a restart happens inside `observerDays`. 2. The unconditional `inactive = 0` reactivation at the end of `UpsertObserverAt` undoes any soft-delete that did land. 3. A metrics sample is filed at ingest time, dating a months-old reading as a present-tense measurement. `UpsertObserverAt`'s docstring already claimed retained replays were a no-op for `last_seen` thanks to the `MAX` guard. That held only while the caller passed the envelope timestamp; Kpa-clawbot#1465 switched it to ingest time, which defeats the guard. ## Fix The retained path now updates metadata only, via a new `UpsertObserverRetained`: - no `last_seen` advance - no `inactive = 0` reactivation - no `packet_count` bump - no metrics sample - **no INSERT** — a retained-only observer the analyzer has never heard from live describes a past that may be months old and does not belong in the list. A live message from the same observer creates the row through the normal path moments later. Live status handling is unchanged. ## Tests Seven tests in `cmd/ingestor/retained_status_test.go`, written before the fix: - 4 that failed on the bug: `last_seen` advance, reactivation of a soft-deleted row, creation of a never-seen observer, metrics-sample insert - 2 regression guards pinning live (non-retained) behaviour: `last_seen` still advances, unknown observer still created - 1 asserting retained metadata is still applied — the snapshot is the observer's last known state, only the liveness signal is suppressed `mockMessage` gained a `retained` field so `Retained()` is controllable. Meta flattening is extracted to `observerMetaColumns` so both write paths bind identical args. Full `cmd/ingestor` suite passes. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> (cherry picked from commit 9bd5f5a) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…de (Kpa-clawbot#1748) (Kpa-clawbot#1821) Closes Kpa-clawbot#1748. ## Root cause `renderTableRows()` in `public/packets.js` re-filtered the already server-filtered `/api/packets` (grouped) results client-side, comparing `filters.observer` against each row's `observer_id`. But that field is only the **representative** observer chosen for display — `QueryGroupedPackets` (`cmd/server/db.go:554-606`) picks the observation with the longest observed path via a `LEFT JOIN ... ORDER BY length(path_json) DESC LIMIT 1`, purely for display purposes. The server-side filter (`buildTransmissionWhere`, `cmd/server/db.go:725-790`) is already correct: it uses an `EXISTS` subquery over **all** observations of a transmission, so any row it returns was genuinely seen by at least one selected observer. The client then discarded rows *again*, checking only the representative's `observer_id`, with a fallback to `p._children` — which is `undefined` on initial page load (only fetched lazily on row-expand or when the observer-sort dropdown changes, see the `obsSortSel` change handler). Net effect: a multi-observer transmission stayed visible under an observer filter only when the filtered observer happened to also be the representative (longest-path) observer — which matches the reported "works only for whichever observer logged it first" behavior in dense meshes, where longest-path and earliest-seen correlate. ## Fix Skip the client-side observer re-filter entirely when `groupByHash` is active — the server's `EXISTS` filter is authoritative for grouped rows and needs no client-side correction. The flat/expanded-mode path (single-observation rows, each with its own exact `observer_id` from `buildPacketWhere`) keeps the existing children-aware filter unchanged, which already had test coverage under Kpa-clawbot#537 for the case where `_children` is already populated. ## Tests Added 6 cases in `test-frontend-helpers.js` covering the specific gap Kpa-clawbot#537's tests didn't reach — grouped mode with `_children` still `undefined` (the actual initial-load state that triggers this bug). New tests confirm: - A multi-observer row whose representative doesn't match the filter is kept in grouped mode (the core bug). - Grouped mode never re-filters client-side (trusts the server). - Flat mode behavior is unchanged (matches by own `observer_id`, falls back to already-loaded `_children`). Full run: `test-frontend-helpers.js` 631 passed / 2 failed (same 2 pre-existing `favStar` failures reproduce identically on unmodified `master` — unrelated). `test-packet-filter.js` 92/92. `test-aging.js` 18/18. Operator context: running CoreScope for SaarMesh (SaarLorLux, DE/FR/LU, 800+ nodes, 14 observers) — this was hiding a large share of traffic whenever filtering by a non-primary observer in our dense mesh. Co-Authored-By: Claude <noreply@anthropic.com> --------- Co-authored-by: Claude <noreply@anthropic.com> (cherry picked from commit 02c2338) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… rx-coverage (Kpa-clawbot#1860) ## Summary `public/rx-coverage.js` still carried a literal `🗺️` (U+1F5FA) in the Mobile RX coverage page header — missed by the Kpa-clawbot#1648 emoji → Phosphor migration. Replaced with `ph-map-trifold` from the existing sprite, matching how every other page header renders (`analytics.js`, `home.js`, `node-analytics.js`, `customize-v2.js`). ```diff -'<h2 style="margin:4px 0 2px;font-size:18px">🗺️ Mobile RX coverage</h2>' + +'<h2 style="margin:4px 0 2px;font-size:18px"><svg class="ph-icon" aria-hidden="true"><use href="/icons/phosphor-sprite.svg#ph-map-trifold"/></svg> Mobile RX coverage</h2>' + ``` ## How it got through This is not a gap in the tooling — `test-issue-1648-m6-final-sweep.js` catches it correctly. On current `master` (`a06ac8ac`): ``` ✗ 1 emoji-as-icon violation(s): public/rx-coverage.js:29 [U+1F5FA] '<h2 style="margin:4px 0 2px;font-size:18px">🗺️ Mobile RX coverage</h2>' + ``` The gate is in `test-all.sh` but not in the CI test list in `deploy.yml`, so it never runs. That divergence is filed separately as Kpa-clawbot#1858 — this PR is the concrete defect it let through. ## Test plan - [x] `node test-issue-1648-m6-final-sweep.js` — `✓ lint gate: 0 violations across public/** and cmd/**` (was 1 violation before) - [x] `node test-issue-1648-m6-lint-self.js` — green, including the anti-tautology probe (it requires a clean repo to run at all, so it was failing on master purely as a cascade from the above) - [x] `eslint public/rx-coverage.js` — 0 errors (1 pre-existing `no-unused-vars` warning on `selectedName`, untouched) - [x] `ph-map-trifold` confirmed present in `public/icons/phosphor-sprite.svg` Single-line change, no behaviour change beyond the icon glyph. 🤖 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> (cherry picked from commit a3454e7) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…pa-clawbot#1897) Relates to Kpa-clawbot#1335, which was already closed by PR Kpa-clawbot#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 Kpa-clawbot#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 Kpa-clawbot#1749/PR Kpa-clawbot#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. (cherry picked from commit 647841c) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… on the instance (Kpa-clawbot#1893) Fixes Kpa-clawbot#1890. ## The problem `public/index.html:16` shipped this to every deployment: ```html <meta property="og:url" content="https://analyzer.00id.net"> ``` Open Graph consumers — Facebook and Messenger among them — treat `og:url` as the canonical destination. Clicking the preview of a link shared from *any* CoreScope instance navigated to that one host. The direct link text still resolved correctly, which is why this went unnoticed; the preview card and the surrounding message body did not. It is the only occurrence in the frontend. ## The change Remove the tag. `og:url` is optional — with no tag present, consumers fall back to the URL they crawled, which is correct for every deployment and needs no configuration. ## Why not the config-driven variant The issue also proposes deriving the URL from `config.json`. I did not take that shape, on purpose: `index.html` is pre-processed **once at startup** — `spaHandler` reads it and substitutes `__BUST__` (`cmd/server/main.go:565`), then serves the same byte slice for every request. A correct per-host `og:url` therefore needs either a new public-URL config key or per-request templating of the index. Both are decisions about config surface and request-path cost that belong to you, and neither is needed to stop the redirect. Happy to follow up with whichever shape you prefer — this PR is the part that is unambiguous. ## What is left alone `og:image` still points at `raw.githubusercontent.com/Kpa-clawbot/corescope/master/public/og-image.png`. That is the project's own asset, a shared project resource rather than a redirect target, so it is correct for every instance to reference it. ## Test `test-issue-1890-og-url.js`, a static scan, registered in `test-all.sh`: - no `og:url` meta tag - no `rel="canonical"` link - no `00id.net` reference anywhere in `index.html` - `og:title` / `og:description` / `og:image` still present That last assertion is deliberate: without it the guard could be satisfied by deleting the whole embed block. Watched fail first — 2 passed, 2 failed before the change, 4 passed after. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> (cherry picked from commit c5a71b3) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This adds two optional map tile providers: - OpenTopoMap - USGS They are disabled by default, but can be quite useful for visualizing repeater sites with terrain features visible. (cherry picked from commit 34b41fd) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…e route type (Kpa-clawbot#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](meshcore-dev/MeshCore#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 Kpa-clawbot#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. Kpa-clawbot#653 landed the skip as `pathByte & 0x3F == 0`, which swallows the second case too. The diagnosis in Kpa-clawbot#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 Kpa-clawbot#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 Kpa-clawbot#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. Kpa-clawbot#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 Kpa-clawbot#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> (cherry picked from commit 97b6090) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…observers (Kpa-clawbot#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 Kpa-clawbot#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> (cherry picked from commit d821d9a) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…op index rebuild (Kpa-clawbot#1907) Fixes Kpa-clawbot#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 — Kpa-clawbot#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` (Kpa-clawbot#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 Kpa-clawbot#1903 Both touch `byPathHop` semantics, so I verified them composed on the same fixture. With Kpa-clawbot#1904 alone the resolved keys come back and Kpa-clawbot#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> (cherry picked from commit f081f91) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ton history (Kpa-clawbot#1883) fix: use replaceState for traces/roles redirects to preserve back-button history The #/traces/<hash> and #/roles backward-compat redirects used location.hash = ..., which pushes a new history entry instead of replacing the current one. This trapped users navigating back from a trace view: the intermediate #/traces/<hash> entry would immediately re-redirect forward again on hashchange, so back button never reached the packets view they came from. (cherry picked from commit a46a6d5) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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 Kpa-clawbot#1872, which no longer compiles after Kpa-clawbot#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> (cherry picked from commit 4a77645) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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 Kpa-clawbot#1143 already added and indexed It looks like buildPacketWhere, GetRecentTransmissionsForNode, QueryMultiNodePackets etc. moved to from_pubkey already, but not this. (cherry picked from commit eb8f376) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…tore (Kpa-clawbot#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> (cherry picked from commit ac6fbaf) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…r-graph filter (Kpa-clawbot#1944) Closes Kpa-clawbot#1925. This is the flake that failed Kpa-clawbot#1942 and Kpa-clawbot#1871, neither of which touches the feature. It is not a test problem. ## Cause Every page load fires exactly one delayed, full re-render of the active analytics tab: 1. `app.js` starts `/api/config/theme` without gating navigation on it, deliberately. 2. When it resolves, `_customizerV2.init()` runs `applyCSS()`, which dispatches `theme-changed`. 3. `app.js:1188` debounces that by 300 ms and dispatches `theme-refresh`. 4. `analytics.js:230` answered it with `renderTab(_currentTab)`. For the neighbor-graph tab step 4 is destructive. `renderTab` replaces `el.innerHTML`, so the role checkboxes are recreated with their defaults and companion is silently re-checked, and `_ngState` is rebuilt from the full 1400-node graph. The count is back over the 1000 limit, so `#ngSkipMsg` returns and the canvas is hidden. The re-entrancy epoch guard cannot prevent it. That guard stops a superseded `tick()` loop; this is a legitimate new top-level render pass that resets the very inputs the guard protects downstream of. **One mechanism produces both documented failure modes**, decided only by where that single re-render lands: | lands | result | |---|---| | before the first uncheck | harmless, test passes | | between an uncheck and the next `waitForFunction` poll | mode 1, the 15 s timeout | | after `waitForFunction` succeeded, before the follow-up `evaluate` | mode 2, "expected #ngSkipMsg gone again" (Kpa-clawbot#1942) | Measured on an idle machine: the test's final evaluate at 542 ms, `theme-refresh` at 636 ms, `#ngSkipMsg` re-added at 667 ms. It passes locally by about 90 ms. On a loaded runner the test's Playwright round trips stretch while `theme-refresh` still lands at theme-fetch latency plus 300 ms, so it arrives mid-test. ## Fix On `theme-refresh`, restart the renderer instead of rebuilding the tab when the neighbor-graph tab is active and has state. Four lines. This stays theme-correct: node colors are read live per frame from `window.ROLE_COLORS`, role swatches use `.role-swatch--{role}` CSS tokens, stats and the skip message use CSS variables, and the one cached theme value, `_labelColor = cssVar('--text-primary')`, is re-read on restart at `analytics.js:3375`, inside `startGraphRenderer`. When `_ngState` is null it falls through to the old path. ## Verification Measured, not asserted: - **Deterministic reproduction** (hold `/api/config/theme` until just before the second filter-down, then stall 500 ms before the final evaluate; no synthetic events dispatched): **2 of 2 fail** on unmodified master with the exact Kpa-clawbot#1942 message, **3 of 3 pass** with this fix. - The **unmodified** E2E test passes against the fixed build. - With the tab open and a filter applied, a `theme-refresh` leaves the filter intact, the canvas present, and produces no page errors. - The **regression test added here fails on unmodified master** with `theme-refresh reset the role filter (companion re-checked)` and passes with the fix. It dispatches the event directly, so it tests the cause instead of waiting for the race to appear. ## What this does not cover The same startup re-render silently discards user interaction in the first second or so on **any** analytics tab, not just this one. A user who clicks quickly after load loses that click. This change covers the neighbor-graph tab, because that is what Kpa-clawbot#1925 is about and what is failing CI. The general fix, for example skipping the startup refresh when the effective config changed nothing, deserves its own issue rather than being smuggled in here. Side observation while tracing: `theme-changed` fires twice at startup, at about 311 ms and 323 ms. The debounce collapses them, so it is harmless, but it means the customizer pipeline runs twice. I did not identify the second dispatcher. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> (cherry picked from commit e2df9bb) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…yed packets (Kpa-clawbot#1957) Closes Kpa-clawbot#1898. Closes Kpa-clawbot#1900. Two issues, one omission. With a region filter active, VCR replay on the Live map rendered **nothing at all** (Kpa-clawbot#1898), and the Replay button on packet detail **silently did nothing** (Kpa-clawbot#1900). ## Cause `packetMatchesRegion` (`public/live.js:80-92`) matches a packet group by looking up `packets[i].observer_id` in the observer roster map. A packet whose `observer_id` is null is skipped, and when none match it returns `false` and the caller drops the whole group (`live.js:3374`). `dbPacketToLive()` returned `observer` (the resolved name) but never `observer_id`. So every replayed packet was skipped, and every group was dropped. The Replay button had the same gap: both branches passed `obsName(o.observer_id)` and threw the id itself away. **The value was there the whole time.** The VCR builds its entries with `Object.assign({}, p, obs, ...)`, so the observation's `observer_id` is on the input, and the server has returned `observer_id`, `observer_name` and `observer_iata` per packet since `cmd/server/db.go:345-347`. Only the object literal dropped it. ## Fix Carry `observer_id`, and `observer_iata` alongside it so `obsIataBadgeHtml` (`live.js:102-108`) can use the direct field for replayed packets instead of falling back to the roster map. Three lines of behaviour, in two files. ## Verification Four regression tests in `test-live-region-filter.js`. **Three fail without the fix**, checked by reverting `live.js` and re-running: ``` ❌ Kpa-clawbot#1898: dbPacketToLive carries observer_id through ❌ Kpa-clawbot#1898: a replayed packet survives an active region filter ✅ Kpa-clawbot#1898: dropping observer_id is what broke it (guards the regression) ❌ Kpa-clawbot#1898: observer_iata is carried so the badge needs no roster lookup ``` The one that passes either way does so on purpose: it asserts a packet carrying **no** `observer_id` is still dropped, pinning the mechanism so a future change cannot make the filter match everything. That test's sandbox needed `getParsedDecoded` and `getParsedPath`. `live.js:14` captures those from `packet-helpers.js` at load time and the sandbox does not load it, so they are stubbed in the sandbox definition rather than assigned afterwards. Assigning later is too late for that capture, which cost me two attempts. Other suites unaffected: `test-live.js` 95 passed, `test-packet-filter.js` 99, `test-frontend-helpers.js` 656. `test-1110-live-filter.js` fails identically on unmodified master with `ERR_CONNECTION_REFUSED`; it is an E2E test needing a server on port 13581. ## Note Both issues were filed separately and neither names the other. They are the same root cause in sibling code paths, which is why they are fixed together rather than in two PRs. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> (cherry picked from commit 9488e9c) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ever achieved (Kpa-clawbot#1958) Part 2 of Kpa-clawbot#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 Kpa-clawbot#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 Kpa-clawbot#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> (cherry picked from commit 56d6d4c) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…pa-clawbot#1962) Closes Kpa-clawbot#1896. The banner told operators their clock is naive but not that the notice goes away on its own, so people went looking for Kpa-clawbot#1480 to find out. One sentence: > Clock is naive — per-packet timing clamped to ingest time. **Clears itself 24h after the last skew event.** ## Verified before writing it into the UI The issue asserts the 24h self-clear. Rather than repeat that, I checked it: - `cmd/server/observer_naive_clock.go:8` — `const observerNaiveClockWindow = 24 * time.Hour` - `applyObserverNaiveClock` applies the decay at read time and leaves `clock_naive` false once the last event is older than the window - its own comment: *"any event older than observerNaiveClockWindow is treated as absent so the chip and banner clear automatically without a background sweep"* So "24h after the last skew event" is accurate, including the fact that it needs no sweep and no restart. No test pinned the old string. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> (cherry picked from commit 83134d6) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Red commit: `5a5ecb6` (local browser: 14 passed, 8 behavior assertion failures before the fix). Hash Issues links now restore `bytes=1|2|3` for the selected control and its matrix/collision data. Missing or malformed values default to one byte. Selector clicks, section/top links, tab-bar changes, filters and theme refreshes retain the chosen view through the existing URL helper. Fixes Kpa-clawbot#1914. - E2E assertion added: `test-issue-1306-collisions-terminology-e2e.js:242`. The existing CI-selected harness passes 23 checks, including distinct nonempty collision rows for each byte size. Its original assertions remain. - Browser verified: `http://127.0.0.1:55634` with the local fixture API, plus reviewed matrix/risk screenshots. Region refresh passed; area coverage skips because the fixture has no areas. - Required frontend checks pass: 99 filter, 18 aging, 666 helpers; URL helpers pass 18. Three independent reviews found no blocking issues; their coverage suggestion is included in `fb482fe`. - Added work parses URL state and updates six links. Rendering and bulk requests are reused; no backend, configuration, dependency or CI-list changes. - A broader smoke run timed out at Live autocomplete (Kpa-clawbot#1110); full-suite success is not established. ## Preflight overrides - The external `run-all.sh` is absent. Corresponding scope, PII, syntax, whitespace and CSS checks passed; no SQL, migration or image changes require those gates. - Red browser evidence is local. Upstream CI execution remains a separate approval gate, as discussed in Kpa-clawbot#1922. (cherry picked from commit eb1d733) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Red commit: `2bf9be8` (local Chromium: 3 passed, 3 intended assertion failures). CI: [run](https://github.com/Kpa-clawbot/CoreScope/actions/runs/33988112224) awaits maintainer approval (`action_required`); 0 jobs started. Remove the unqualified aggregate Avg SNR row from node side-panel Overview and full-detail stats, following option 3 in Kpa-clawbot#1149. Heard By retains each observer's SNR reading. Fixes Kpa-clawbot#1149. - E2E assertion added: `test-issue-1281-location-row-e2e.js:224`. Three new browser cases cover desktop side/full and mobile full views with a numeric aggregate and distinct positive/negative observer readings. Existing packet-location assertions remain intact. - Browser verified: local Chromium; 6 cases passed after push. Screenshots: `coverage/issue-1149/issue-1149-desktop-side-panel.png`, `coverage/issue-1149/issue-1149-desktop-full-detail.png`, and `coverage/issue-1149/issue-1149-mobile-full-detail.png`. - Validation: packet filter 99/99, aging 18/18, frontend helpers 666/666; XSS, CSS-variable, syntax, whitespace and PII checks passed. - Independent reviews: adversarial, lifecycle expert and TDD reviewers found no required changes. One initial browser navigation timed out; the unchanged parent rerun passed 6/6. - Performance/config: two production row deletions; no new requests, loops, timers, settings or customizer implications. Backend unchanged; Go suites were not rerun. Fix commit: `d7c68f3`. ## Preflight overrides - External `run-all.sh` is unavailable on this host. Scoped branch, red/green, PII, CSS, XSS and whitespace checks were run directly. The diff adds no migrations, SQL attribution or image markup. (cherry picked from commit 108ea02) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…t#1968) Red commit: `0988bc0` (local browser: 3 passed, 8 behavior assertion failures before the fix). The selected node now has a compact outline in long “Paths Through This Node” chains, in the side panel and full detail page. Matching uses complete public keys without case sensitivity; same-prefix siblings and unresolved hops stay unmarked. Existing links, escaped names, warnings and ambiguity underlines are preserved. Fixes Kpa-clawbot#1153. Its prerequisite Kpa-clawbot#1144 is already merged. - E2E assertion added: `test-issue-1146-path-link-contrast-e2e.js:220`. The existing CI-selected harness passes 11 checks across 18-hop paths, both themes, desktop/mobile, and the renderer fallback. Review follow-up `bd8118f` verifies the marked ambiguous hop's dashed underline. - Browser verified: `http://127.0.0.1:55635`; desktop/mobile path screenshots were inspected. The broader smoke runner exited successfully with fixture-dependent skips. - Required frontend checks pass: 99 filter, 18 aging, 666 helpers. CSS variables, seven CSS self-tests, 31 XSS sink checks, 17 XSS gate self-tests and XSS diff preflight pass. - Three independent reviews found no blocking issues. Traversal remains linear with no new requests, settings, dependencies or cache invalidation; styling uses the existing customizer token. ## Preflight overrides - The external preflight runner is absent; corresponding scoped gates passed. Red browser evidence is local, with upstream CI approval tracked separately under the process in Kpa-clawbot#1922. - Existing rapid-navigation map resize timer errors remain visible in browser logs and are outside this change. (cherry picked from commit a938176) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ade (Kpa-clawbot#1974) Closes Kpa-clawbot#1794. Follow-up to Kpa-clawbot#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> (cherry picked from commit 1ffaad8) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ing it (Kpa-clawbot#1982) Closes Kpa-clawbot#1977. Supersedes Kpa-clawbot#1952. Follow-up filed as Kpa-clawbot#1983. ## What §10.2 did ```bash q="SELECT COUNT(*) FROM transmissions WHERE from_node = '$TEST_PUBKEY';" qq=$(printf %q "$q") if ! count=$(ssh_t "docker exec … sqlite3 … $qq" 2>/dev/null); then count=$(ssh_t "sqlite3 … $qq" 2>/dev/null || echo "") fi ``` The injection is not reachable today — `TEST_PUBKEY` is hex-gated and the script `exit 2`s before the SQL is built. The problem is that the SQL layer's safety rests entirely on that outer gate rather than on the SQL layer itself. Kpa-clawbot#1952 proposed doubling embedded quotes; that is string escaping, not parameterisation, which is why it was withdrawn in favour of this. ## What this does Per the four points in the sign-off on Kpa-clawbot#1977: **1. Bind the value.** A constant `SELECT` and a bound `:pubkey`, fed to sqlite3 on stdin. The SQL no longer crosses the remote shell as a command word, so there is no `printf %q` on the query at all any more. **Why hex rather than `.parameter set :pk '<value>'`.** Dot-command arguments are split on whitespace, so a payload containing a space produces too many arguments — and sqlite3 responds by printing the `.parameter` help to **stdout**, exiting **0**, and leaving `:pk` **unbound**. `COUNT(*)` then returns 0, which reads exactly like a passing security fix. `-bail` does not catch it. Verified on 3.51.0: ``` $ printf ".parameter set :pk '' OR 1=1 --'\nSELECT COUNT(*) FROM transmissions WHERE from_node = :pk;\n" \ | sqlite3 -bail ptest.db .parameter CMD ... Manage SQL parameter bindings # <- help, on stdout … 0 # <- :pk never bound $ echo $? 0 ``` `.parameter set :pk 1+1` also binds the integer `2` — the value is evaluated as an SQL expression and only falls back to a text literal when evaluation fails. So interpolating into the `.parameter set` line trades one hazard for another. Hex-encoding removes the quoting layer instead of adding one: the value is bound as `cast(x'<hex>' as text)`, so its contribution to the SQL text is drawn from the alphabet `[0-9a-f]` only. Nothing to quote, no tokenizer arity hazard, and it holds for **arbitrary** input rather than only for hex-gated input — which is the point. Verified against a fixture table holding two rows, one of them `deadbeef`: | value | result | exit | |---|---|---| | `deadbeef`, bound as `cast(x'6465616462656566' as text)` | `1` | 0 | | `' OR 1=1 --`, bound the same way | `0` | 0 | | `' OR 1=1 --`, interpolated the current way | `2` (whole table) | 0 | | query against a DB with no `transmissions` table, `-bail` | `Parse error … no such table` on **stderr** | **1** | **2. Probe the capability, not a version.** `resolve_sqlite_runner` binds `corescope-probe-ok` and asserts it comes back — a round trip, not a bare `.parameter init`, so the positive control runs against the operator's actual binary rather than one we pin. If neither the container nor the host qualifies, it fails loudly and names what is needed: ``` ❌ retain-failed: no sqlite3 able to bind a parameter on the target tried: docker exec -i corescope-prod sqlite3, then sqlite3 on runner@example need: the sqlite3 CLI reachable over ssh, supporting '.parameter set' OCI runtime exec failed: exec: "sqlite3": executable file not found in $PATH bash: line 1: sqlite3: command not found ``` There is deliberately **no** interpolating fallback. That would leave the vulnerable path in place under a nicer name. **3. The hex gate is kept**, with its comment updated to say why: for the SQL layer it is now defence in depth rather than the only guard. Redundant is not the same as wrong. **4. The exit status and stderr survive.** `-batch -bail -init /dev/null -noheader -list` (stop at the first SQL error; ignore the operator's `~/.sqliterc`, where a stray `.mode` would make the count unparseable; stdout is exactly the number). Query stderr is captured and printed on failure rather than sent to `/dev/null`, so a broken query is distinguishable from a legitimately empty result. Probe stderr is collected too, and printed only if *both* probes fail — the container miss is the known-normal case, so surfacing it on every run would be noise. ## Also fixed An existing double-count in §10.2: the `TARGET_DB_PATH unset` branch incremented `$fails` and then left `count=""`, so the generic branch incremented it a **second** time for the same failure. `read_retain_count` now gives §10.2 exactly one increment point. Opportunistic cleanup in a file already being touched (AGENTS.md line 318). ## Tests New `qa/scripts/test-blacklist-sql.sh`, wired into the `go-test` job. 24 assertions, modelled on `scripts/staging/test-disk-monitor.sh`. Both directions are asserted, because a zero from a command that failed proves nothing: - **Positive control** — `deadbeef` still returns its row (`1`, exit 0), and so does `cafebabe`; an absent pubkey returns `0`. - **Negative** — `' OR 1=1 --` returns `0` while the table demonstrably holds 2 rows, and the old interpolated form is asserted to leak all `2`. That last assertion is what makes the `0` above worth something. - **Error surfacing** — the same SQL against a DB with no `transmissions` table exits non-zero with a message on stderr and nothing on stdout. - **Alphabet** — `sql_hex_literal` output matches `^x'[0-9a-f]*'$` for the SQL payloads, a backslash, `$(id)` / backticks, an embedded newline, `héllo`, and a 4096-byte repetitive string. That last one is a regression guard for `od -v`: without the flag `od` collapses repeated identical lines to `*`. - `run_sqlite` with no resolved runner refuses rather than guessing. Group 2 skips loudly (rather than silently) if `sqlite3` is not on PATH; group 1 needs no sqlite3 and always runs. **Mutation-tested** — each of these breaks the suite, so the assertions have teeth: | mutation | caught by | |---|---| | restore full interpolation | `injection payload → 0 rows — expected '0' got '2'` | | naive `.parameter set '%s'` | `expected '0' got '.parameter CMD ...'` | | drop `od -v` | alphabet failure on `*`, plus `expected '8192' got '33'` | Commit 1 is a behaviour-neutral refactor that moves the imperative body into `main()` behind a `BASH_SOURCE` guard, so the test can source the script and exercise individual helpers. Same idiom as `scripts/staging/disk-monitor.sh:99`. ## Verification - `bash qa/scripts/test-blacklist-sql.sh` → 24 passed, 0 failed - `bash -n` on both scripts - All three runtime paths exercised end to end with PATH shims for `ssh`/`docker`/`sqlite3` against a real fixture DB: success (`sqlite3 runner: host`, count 2), query failure (classified message + `Parse error … no such table`, `fails=1`), and no-capability (the loud block above, both probe stderrs, `fails=1` — not 2) - The new step lands inside `go-test`, which runs when `changes.outputs.code == 'true'`; `qa/scripts/*.sh` does not match that job's `^docs/|[.]md$|^LICENSE$` documentation filter, so it is not skipped ## Deliberately out of scope - **The `docker exec` branch is dead on current images** → filed as Kpa-clawbot#1983. The app container has no `sqlite3` at all: `Dockerfile:15` is pure-Go SQLite with no CGO, and the `apk add` installs only `mosquitto mosquitto-clients supervisor caddy wget`. So the host "fallback" is the only path that has ever executed, silently, because both branches discarded stderr. This change keeps both branches and merely makes the outcome visible (`sqlite3 runner: …` on every run). - **`-readonly` on the target DB.** Tempting, and verified compatible with `.parameter` (the binding table lives in the TEMP database), but a WAL database needing journal recovery can refuse a read-only open. Adding it here risks exactly the "trades an unreachable injection for a script that does not run" outcome flagged in the Kpa-clawbot#1952 thread. Worth its own issue. - **The other `2>/dev/null` sites** in this file, which also sit awkwardly with `qa/README.md`'s "Don't silence stderr". Only the §10.2 lines named in the sign-off are touched. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> (cherry picked from commit de237fc) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ng it to stop (Kpa-clawbot#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 Kpa-clawbot#2000. (cherry picked from commit 02feb2a) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…stalling ingest (Kpa-clawbot#2000) Reviewed at ecf0b37: query plans dumped and confirmed index-driven for all three statements, termination proven against concurrent ingest (first_seen is always time.Now()), FK child-first ordering required and correct, writer-stats assertions non-racy. Two low findings noted on the PR for follow-up: the dropped RowsAffected error now gates the loop, and ~0.53s batches will trip defaultSlowWriterMs=500. (cherry picked from commit fe37f10) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Verified: every value is SHA-256(name)[:16] per internal/channel.DeriveKey, 319/320 exact (Public is the fixed firmware default), no duplicate hashes, file parses at 320 entries. (cherry picked from commit c283f42) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…clawbot#2009) Verified against production: ingestor writes enc_<HH> (db.go:2282), store.go:3536 filters on it, live values are uppercase two-digit hex with thousands of packets (enc_28: 2708), /api/channels omits them, and /api/packets?channel=enc_28 returns rows. New test passes at 14 and fails when the dedupe guard is disabled. (cherry picked from commit a2f039d) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This was referenced Sep 13, 2026
This was referenced Sep 13, 2026
Owner
|
Closing this aggregate PR without merging. The 26 upstream fixes it bundles have been split into individual PRs so each one can be reviewed, tested, staged and merged on its own risk profile. This branch has since gone The work continues in those separate PRs; nothing is dropped. The branch is left in place, not deleted. |
|
Thanks for the clarification. That makes sense, especially given the overlap and the branch now conflicting with master. I’m glad the individual ports preserve the work and allow each upstream fix to be reviewed and tested independently. I’ll follow the dedicated PRs going forward and help address any review or testing issues there. Thanks for taking the time to split these out and keep the work moving. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
13 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Ports 26 PRs merged upstream in Kpa-clawbot/CoreScope between 2026-08-14 and 2026-09-12. They are the ones that cherry-pick onto our
masterwithout conflicts. Each commit is agit cherry-pick -xof the upstream squash commit. Upstream authorship is kept, and every message carries a(cherry picked from commit …)line.A full merge of
upstream/masteris not an option: we are 402 commits ahead and 93 behind, and a trial merge conflicts in 44 files. PRs that conflict, or that overlap our own scope work, are left out (see below).Highlights
PruneOldPacketsdeleted a whole retention day inside oneWriterTx, which blocked MQTT ingest for the entire delete. It now deletes in bounded batches./wsupgrade. Rejections are counted in/api/statsunderwebsocket.statusmessage replayed on (re)subscribe no longer counts as observer liveness.observer_idis carried into replayed packets, so VCR replay and the packet-detail Replay button work with a region filter active.retention.observerPurgeDays, which hard-deletes long-inactive observers. Opt-in.og:url(https://analyzer.00id.net) fromindex.html, so links shared from our instance no longer open that host.All 26 commits (upstream merge order)
Config and behaviour changes for operators
webSocketblock (feat(#1794): per-IP limits and a deny list on the /ws upgrade Kpa-clawbot/CoreScope#1974):maxConnsPerIP: 0(off),upgradesPerMinPerIP: 30(on),trustedProxies: [],deny: [].localhost:3000, so the server's peer is loopback. WithtrustedProxiesempty,ws_limits.gotreats clients as indistinguishable and skips the per-IP limits, logging one warning. The deny list still applies."trustedProxies": ["127.0.0.1", "::1"]. Only do that if Caddy'sX-Forwarded-Forreally carries the visitor IP. If a CDN such as Cloudflare sits in front of Caddy, XFF holds the CDN edge address unless Caddy is configured to trust the CDN. Many real visitors then share one edge IP and could be throttled.retention.observerPurgeDays(feat(retention): add observerPurgeDays hard-delete for long-inactive observers Kpa-clawbot/CoreScope#1886): default0(disabled). If enabled, set it above bothobserverDaysandpacketDays.opentopomapandusgs(Add topographic map layers Kpa-clawbot/CoreScope#1891): added,enabled: falseby default.og:urlremoved frompublic/index.html(fix(#1890): drop the hardcoded og:url so shared links stay on the instance Kpa-clawbot/CoreScope#1893).Deliberately not included
detectSchemaswallows probe errors).top-nav), observers page (.observers-page) and map controls (.map-controls) disagree on observer totals Kpa-clawbot/CoreScope#1888/fix(#1888): count only live observers in the store's /api/stats query Kpa-clawbot/CoreScope#1892, feat(nodes): export the visible node list as MeshCore companion contacts JSON Kpa-clawbot/CoreScope#1889, feat(map): Important Links overlay — B-weighted top routes Kpa-clawbot/CoreScope#1771/feat(map): Important Links overlay (rebase of #1771 onto master) Kpa-clawbot/CoreScope#1928, feat(analytics): repeater metric scatter tab Kpa-clawbot/CoreScope#1760, fix(frontend): relay-aware staleness for infra nodes + dim-not-delete (#1598, PR A) Kpa-clawbot/CoreScope#1815, fix(clock-skew): restrict per-node skew to self-originated adverts (#1816, #1818) Kpa-clawbot/CoreScope#1820, fix(#1749): decouple watchdog emit from blocking I/O (root cause) Kpa-clawbot/CoreScope#1853, fix(#1854): move relay last_seen touch to the ingestor — server writes have been no-ops since mode=ro Kpa-clawbot/CoreScope#1855, fix(#1864): decode ANON_REQ source pubkey instead of treating it like REQUEST Kpa-clawbot/CoreScope#1866, fix(nodes): dispose map timers with their owning view Kpa-clawbot/CoreScope#1970, fix(live): use the shared WebSocket instead of opening a second one Kpa-clawbot/CoreScope#1991, perf(#1910): collapse concurrent /stats work and serve the count cache stale Kpa-clawbot/CoreScope#1963 (step 1).Verification
go build ./...andgo vet ./...: OK incmd/serverandcmd/ingestor.go test ./...incmd/server:ok.go test ./...incmd/ingestor: the only failure isTestPruneOldNeighborMetrics(expected 1 row pruned, got 2).master(fda24ca) and comes from our own 27628fb, not from this PR.TestBackfillTxLastSeen_ResolvesFromMaxObservationTimestampfailed once in the first full run. It passes in isolation, and two further full runs did not reproduce it.test-issue-1890-og-url.js(4/4),test-live-region-filter.js,test-packets-local-channels.js(14/14).test-frontend-helpers.js: 695 passed / 2 failed. Onmasterit is 687 passed / 2 failed; the 2 are the same pre-existingfavStarassertions.CI/CD Pipelinerun on this fork, so there is nomasterbaseline to compare against. ExpectGo Build & Testto go red onTestPruneOldNeighborMetrics, because it already fails onmaster. Any other failure is new and worth a look.🤖 Generated with Claude Code