mirror of
https://github.com/Kpa-clawbot/meshcore-analyzer.git
synced 2026-09-13 19:25:40 +00:00
e7b3a2e77f61deeacf4030f892aab705ca7ff010
2883
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
|
|
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> |
||
|
|
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> |
||
|
|
9488e9c31c |
fix(#1898, #1900): carry observer_id into replayed packets (#1957)
Closes #1898. Closes #1900. Two issues, one omission. With a region filter active, VCR replay on the Live map rendered **nothing at all** (#1898), and the Replay button on packet detail **silently did nothing** (#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: ``` ❌ #1898: dbPacketToLive carries observer_id through ❌ #1898: a replayed packet survives an active region filter ✅ #1898: dropping observer_id is what broke it (guards the regression) ❌ #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> |
||
|
|
cb6d230573 |
docs: renumber the release to v3.10.1 (#1953)
v3.10.0 was tagged and then withdrawn. **Nothing was ever available under that number**: no container image and no release asset was ever published, so no user could have pulled it. This renames the notes and the CHANGELOG section. No product code changes. ## Why it had to be renumbered Three things, in the order they bit. **1. The image never built.** `release-fast-path.yml` re-tags `:edge` to `:vX.Y.Z` when the `:edge` revision label matches the tagged commit, and dispatches `deploy.yml` when it does not. The tagged commit was documentation-only, so the `paths-ignore` from #1949 meant no `:edge` existed for it and the fallback ran. That part behaved correctly. The fallback then published nothing, because every GHCR step was gated on `github.event_name == 'push'` and a dispatch is not a push. It built locally, reported `success`, and pushed nothing. Fixed in #1951, but that fix is not in the `v3.10.0` tag, and a `workflow_dispatch` runs the workflow file **from the ref it targets**. So the existing tag could not be made to publish. **2. The assets never uploaded.** I created the GitHub release by hand before the workflow reached it, and `action-gh-release` cannot update an immutable release. The correct procedure is to push the tag and let the workflow create the release. **3. The tag name cannot be reused.** GitHub's immutable releases keep a tag name reserved even after the release is deleted: ``` remote: - Cannot create ref due to creations being restricted. ``` I established that only after deleting the release, which is the wrong order. The lesson, written into the commit message so it survives: check whether a tag can be rewritten before removing anything that depends on it. ## What is in v3.10.1 The same 111 commits, plus the three CI fixes that landed after the v3.10.0 tag (#1949, #1950, #1951). Those are listed in their own section in the notes. **No product code differs** from what was tagged as v3.10.0. All 69 SHA references in the notes were re-verified after the rename. ## Procedure for this tag Push the tag and stop. The workflow creates the release and attaches the assets. Do not create it by hand. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>v3.10.1 |
||
|
|
a3ee37011a |
ci: scope docs-only skipping to jobs, so required checks still report (#1955)
Same change as #1954, opened from a branch on this repository instead of from a fork. #1954 never received a workflow run: zero runs and zero check suites for its head commit, and closing and reopening it changed nothing. A manual `workflow_dispatch` on master ran immediately, so Actions itself is working; the `pull_request` event from the fork is what produces nothing. See the note at the end. Replaces the trigger-level `paths-ignore` from #1949 and #1950. That approach was wrong, and it is currently blocking #1953 from merging. ## What was wrong GitHub documents the distinction I had backwards: > a workflow skipped by path filtering keeps its checks **pending** and blocks the merge, while a **job** skipped by an `if:` conditional reports **Success** and does not. So the filtering has to live on the jobs, not on the trigger. I compounded it by claiming, in both the commit and the description of #1949, that master had no required checks: *"verified: the branch protection endpoint returns 404"*. **That verification was invalid.** A 404 there means the token cannot read protection details, not that none exist. The branch reports `protected=true`, and #1953 was refused with `the base branch policy prohibits the merge`. ## What this does A `🔎 Change scope` job computes whether anything outside `docs/`, `*.md` and `LICENSE` changed. `go-test`, `e2e-test`, `build-and-publish` and `release-artifacts` are gated on its output. A documentation-only pull request skips those jobs, they report Success, and the PR can merge. **Only pull requests are scoped.** A push or a dispatch always runs the full pipeline. That second part is deliberate and it fixes a separate failure. `release-fast-path.yml` re-tags `:edge` to `:vX.Y.Z` only when the `:edge` revision label matches the tagged commit. A master commit with no image breaks tagging, which is what happened to the v3.10.0 tag: the tagged commit was documentation-only, the fast path could not re-tag, it fell back to a dispatch, and the dispatch published nothing (#1951). Master pushes now always produce an image. The cost is running the pipeline on documentation commits to master; pull requests are where the queue pressure was. Two conservative defaults in the scope check: a non-`pull_request` event and an empty diff both count as code, so an unexpected shape runs everything rather than silently skipping. ## Note for whoever has repository settings access Fork pull requests stopped getting workflow runs between 22:36 and 05:40. #1949, #1950 and #1951 all came from the same fork and each got a run; #1954 got none, with no check suite created at all, which is different from a skipped run. `repos/.../actions/permissions` returns 403 for a non-admin token, so this could not be confirmed from the API. If the "Fork pull request workflows from outside collaborators" setting was tightened, that would explain it, and it would affect every outside contributor, not just this branch. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
|
bb8634312d |
ci: publish images on a tag ref, not only on a push event (#1951)
**v3.10.0 produced no container image.** The build job reported `success` and pushed nothing. ## What happened `release-fast-path.yml` re-tags `:edge` to `:vX.Y.Z` when the `:edge` revision label matches the tagged commit, and dispatches `deploy.yml` when it does not. For v3.10.0 the tagged commit was documentation-only. The `paths-ignore` added in #1949 means documentation-only commits skip `deploy.yml`, so no `:edge` image was ever built for that commit, the labels did not match, and the fallback ran. **That part worked exactly as designed** and correctly refused to re-tag an image built from a different commit. Then `deploy.yml` skipped all five GHCR steps, because each was gated on `github.event_name == 'push'` and a `workflow_dispatch` is not a push: ``` 4. Build Go Docker image (local staging): success 5. Set up Docker Buildx: skipped 7. Log in to GHCR: skipped 9. Build and push to GHCR: skipped ``` **So the fallback has never been able to publish an image.** It dispatches a pipeline that cannot push. That stayed invisible for as long as the fast path kept succeeding, which it did until a release note happened to be the last commit before the tag. ## Fix Gate those five steps on a push **or** a tag ref: ```yaml if: ${{ github.event_name == 'push' || startsWith(github.ref, 'refs/tags/v') }} ``` A dispatch aimed at a tag now publishes. A dispatch aimed at a branch still does not, so this does not turn every manual run into a release. ## The version stamp needed no change Verified rather than assumed. `Compute build metadata` keys on `GITHUB_REF`, not on the event: ```bash if [[ "$GITHUB_REF" == refs/tags/v* ]]; then APP_VERSION="${GITHUB_REF#refs/tags/}"; else APP_VERSION="edge"; fi ``` The failed v3.10.0 run already logged `Build: version=v3.10.0 commit=5bad23b`. Only the publishing was missing. ## Not covered here `Release Artifacts` failed on the same run for an unrelated reason: the GitHub release had been created by hand before the workflow reached it, and `action-gh-release` cannot update an immutable release. That one is process, not code. Push the tag and let the workflow create the release. Once this merges, re-dispatching `deploy.yml` against `v3.10.0` publishes the images for the existing tag. No re-tagging needed. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
|
5bad23b46a |
docs: release notes for v3.10.0 (#1948)
Release notes for the first tag since `v3.9.2` on 2026-06-13. **111 commits**, and no auto-generated coverage bumps fall in this range, so all 111 are substantive. Nothing here changes behaviour. It is `docs/release-notes/v3.10.0.md` plus a `CHANGELOG.md` section. ## Verification The header promises that every bullet ends with a SHA you can `git show`. That is checked mechanically rather than trusted: all **69** references were confirmed to point at a commit that exists and whose subject line contains the issue or PR number cited beside it. Zero mismatches. ## Two things operators need, and both are silent failures The urgency line leads with the first one on purpose. 1. **CARTO requires an API key** on its raster basemaps since 2026-08. Without one every tile is served watermarked with HTTP 200. Nothing errors, no healthcheck fires, and the only way to notice is to look at a tile. Anyone upgrading needs to set `map.tiles.providers.carto.key`. 2. **`pathTrust.minHashBytesForMapping` ships at 1**, which is the existing behaviour, so an upgrade changes nothing on its own. The note states what raising it to 2 would actually cost, with numbers from a live instance (56% of path-hop observations are 1-byte, 41% of repeaters use a 1-byte hash), because there is no UI to undo it. The relay `last_seen` fix is quantified the same way rather than described as "improved": for repeaters that relayed within the last hour, the gap between `last_relayed` and `last_seen` drops from a median of 12,062 s to 193 s, and the share more than five minutes behind falls from 96% to 39%. ## A theme worth naming Three of the highlights are the same defect in three places: something is operable before its own setup has finished. The Live view toggles are inert for about 100 ms after paint, the colour picker's deferred focus undid arrow-key navigation so Enter assigned the wrong colour, and an analytics theme-refresh discarded the filter you had just applied. All three were first written off as flaky tests, twice by me. Each is now fixed with a regression test that fails on the previous commit. ## Sequencing This should land, and the `v3.10.0` tag be cut, **before** the Go 1.27 upgrade in #1946. A toolchain bump changes the compiler, the runtime and `gofmt` for everything at once; landing it on top of 111 unpublished commits means a later regression cannot be separated from the toolchain. #1946 itself says no 1.27-only features are being adopted, so there is no cost to waiting one release. A tag first also gives a known-good bisect point. ## Not done The CHANGELOG has no `3.9.2` section and did not have one before this change. I left that gap alone rather than reconstructing it retroactively. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
|
6ccef5053b |
ci: match root-level markdown in the docs paths-ignore (#1950)
Follow-up to #1949, correcting a pattern I did not check before proposing it. #1949 used `'**/*.md'`. That reads as requiring a directory component, so it covers `docs/release-notes/v3.10.0.md` but not `CHANGELOG.md` or `README.md` at the repository root. Since `paths-ignore` skips only when **every** changed file matches, one uncovered root file is enough to run the whole pipeline anyway. GitHub's own example for "any file with this extension" is `'**.js'`, with no slash. `'**/*.md'` does not appear in their documentation at all. `'**.md'` covers root and subdirectories both. ## What this does not establish #1948 (`CHANGELOG.md` plus a release note) did start a full run after #1949 landed, and that is what prompted this. But there is a second candidate explanation I did not rule out: that PR's branch predates #1949, so its workflow file may simply not have carried the filter yet. I am not claiming to have proven which one it was. The fix is correct either way, and shipping the documented pattern is better than defending the first cause I noticed. The real test is the next docs-only PR opened from a branch that already contains the filter. The reasoning is in a comment above the `on:` block, not only in this description. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
|
55aabaa325 |
ci: skip the pipeline for documentation-only changes (#1949)
Two documentation-only PRs were running the full pipeline simultaneously this afternoon: #1948 (`CHANGELOG.md` plus a release note) and #1947 (deleting a stale `docs/DEPLOYMENT.md`). Each spends about 12 minutes on Go Build & Test and about 16 minutes on Playwright to establish that a text file does not break a browser. **The cost is the queue, not the minutes.** On the same afternoon a `pull_request` run was created at 12:13 and its first job did not start until 16:19. Four hours in the queue. Every unnecessary run pushes the ones that matter further back, and this repository has been merging heavily today. ## Checked before adding the filter Rather than assumed: - **Nothing reads markdown at build or test time.** Grepping every Go and JS source for a runtime read (`ReadFile`, `readFileSync`, `os.Open`) of a `.md` path returns nothing. The `docs/` matches in `cmd/` and `test-*.js` are all comments pointing at documentation. - `/api/docs` serves Swagger UI generated from `cmd/server/openapi.go`, not from `docs/`. - `docs/` holds markdown plus screenshots (`png`, `gif`) and no build input. - **This workflow has no tag trigger**, so release tagging is unaffected; that runs from `release-fast-path.yml`. Worth stating explicitly given a `v3.10.0` tag is imminent. `paths-ignore` skips only when **every** changed file matches, so a PR touching both code and documentation still runs the full pipeline. ## The trap, stated in the file If required status checks are ever enabled on master, a skipped workflow never reports, and a docs-only PR would wait forever on a check that cannot arrive. At that point this needs to become a change-detection job with conditional heavy jobs rather than a trigger filter. Master has no required checks today. Verified: the branch protection endpoint returns 404. That caveat is in a comment above the `on:` block, not just in this description, because the person who enables required checks in six months will be reading the workflow and not this PR. ## Note This PR itself changes only `.github/workflows/deploy.yml`, so it is not documentation-only and will run the full pipeline, as it should. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
|
b5dfac85dd |
fix(docs): remove stale docs/DEPLOYMENT.md duplicate (#1947)
## Summary - `docs/DEPLOYMENT.md` and `docs/deployment.md` were both tracked in git, colliding into a single file on case-insensitive filesystems (default on macOS/Windows) and causing `git status` to report spurious modifications. - `docs/deployment.md` is the actively maintained guide (linked from `README.md` and `docs/deployment-behind-cdn.md`); `docs/DEPLOYMENT.md` was a stale duplicate untouched since the MeshCore → CoreScope rename. - Removed `docs/DEPLOYMENT.md` from the index, keeping `docs/deployment.md`. ## Test plan - [x] `git status` is clean on a case-insensitive checkout with no spurious modification - [x] Confirmed no remaining references to `docs/DEPLOYMENT.md` in the repo |
||
|
|
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> |
||
|
|
f167d6f338 |
test(#1923 follow-up): pin the packets window in the munger slide-over step (#1942)
Follow-up to #1923/#1924 — one of the row-dependent packets navigations the pin sweep did not reach. ## The gap `test-slideover-1168-munger-e2e.js` navigates to bare `#/packets` and waits for `#pktTable tbody tr[data-action]` with an 8 s budget, on the default client-side window (`since = now − 15 min`). It is one of the row-dependent packets navigations #1924 did not reach. Most are already immune: #1924 pinned `?timeWindow=1440` in `test-slideover-1056-e2e.js`, `test-e2e-playwright.js` sets `meshcore-time-window=525600` in `gotoPackets()` and in the #1791 Group-Data step, and `test-issue-1122`/`1128` widen the window through the UI dropdown. Three other row-dependent packets navigations in the same job share the exposure and are **not** in this PR, to keep it to one file: - `test-gestures-1062-e2e.js` and `test-touch-gestures-coverage-e2e.js` also assert on the URL after in-app navigation, so a bare `?timeWindow=` query leaks into those assertions (`#/packets?hash=…` becomes `#/packets?timeWindow=1440&hash=…`); they want the localStorage window path. - the mobile branch of `test-observer-iata-1188-e2e.js` pins via localStorage, which `packets.js:736` clamps back to 15 min above 180 on a mobile viewport; switching it to the URL-param idiom this PR uses (which is applied after that clamp) would fix it, but it is a separate file. I can send those separately. (This step itself runs at an 800px viewport — `isMobile`, since the breakpoint is `innerWidth <= 1024` — and the pin still works precisely because the URL param is read after the mobile clamp, at `packets.js:1091-1094`, not from the clamped localStorage value.) ## Why it matters now, with numbers On two of the four master runs of 2026-09-02 this step executed at **freshen+8:06** (run 33678488159) and **freshen+10:13** (run 33684614144) — margins of 6:54 and 4:47 before the fixture's newest rows age out of the window. Every test added ahead of it shrinks that. The suite as a whole is closer still: in run 33684614144 the third repetition of the #1616 flake-gate ran **21:48:21→21:48:45 = freshen+14:45→+15:09**, i.e. already past the 15-minute mark — it survives only because of the #1924 pin. ## Verification Reproduced without waiting for the clock: shift the freshened fixture 20 minutes back (`first_seen` and `observations.timestamp`) and start the server on it — | | aged fixture | fresh fixture | |---|---|---| | step without the pin (master) | **fails** (selector timeout) | passes | | step with the pin (this PR) | passes | passes 3/3 | Same idiom and value as #1924, same caveat baked into the comment: the value must be > 0, because `packets.js` only reads the param under `_urlTimeWindow > 0`, so `timeWindow=0` silently keeps the default. One observation from the same investigation, offered separately from this PR: `tools/freshen-fixture.sh` computes its shift as `now − MAX(first_seen)`; if the max ever sits in the future, the offset goes negative and `printf('+%d seconds')` produces the invalid `'+-N seconds'`. On the NOT NULL `transmissions.first_seen` that aborts the script (set -e); on the nullable columns (`nodes.last_seen`, observers, neighbor_edges) `strftime` silently returns NULL. A one-line clamp to ≥0 would make it safe to run around an insert. Happy to send that separately if wanted. |
||
|
|
ab62e86d2d |
fix(live): wire the view toggles before init() awaits — they are inert for ~100 ms (#1940)
Follow-up to the #1939 discussion, where @efiten asked for this PR. The multibyte E2E assertion that has been failing intermittently on master is a symptom of this; with this change the unmodified test passes reliably (3/3 idle, 8/8 under a 24-core load run that previously failed it 2 in 6). ## The defect `init()` writes the whole controls panel with `app.innerHTML` and only restores toggle state and attaches the `change` listeners ~330 lines later, behind two awaits (line numbers on master, as verified in the #1939 thread): | line | | |---|---| | 1104 | `app.innerHTML = …` — the checkboxes are in the DOM, clickable | | 1256 | `await (await fetch('/api/config/map')).json()` | | 1543 | `await loadNodes()` | | 1612–1614 | `.checked = <pref>` and `addEventListener('change', …)` | **A click inside that window is silently lost.** Measured on master, localhost, clicking `#liveMultibyteToggle` on the first animation frame in which it exists: | run | click at | immediately after | after 2.5 s | |---|---|---|---| | 1 | 481 ms | `checked=true`, `localStorage=null` | `checked=false`, `localStorage=null` | | 2 | 505 ms | `checked=true`, `localStorage=null` | `checked=false`, `localStorage=null` | | 3 | 438 ms | `checked=true`, `localStorage=null` | `checked=false`, `localStorage=null` | No handler runs, nothing reaches localStorage, and the later `.checked = <pref>` reverts the click with no feedback. Separately, the restored state itself appears only **93–112 ms (3–5 rendered frames)** after the control is painted — `ghost` and `colorHash` default ON, so they visibly flick on for every visitor. ## The fix - **`wireLiveControls()`** — synchronous, right after `app.innerHTML`: restores `.checked` and attaches listeners for the eight persisted toggles, as one table instead of eight near-identical blocks. The matrix↔heat interlock applies from the first paint too. - **`applyLiveControlEffects()`** — after the awaits: applies the effects that need state built there (matrix theme, rain canvas). - **`syncHeatToggleToMatrix()`** — the interlock, extracted; it previously existed as two identical copies. - Heat gets a module-level mirror (`heatEnabled`) like the other seven toggles, so the layer is only built when wanted. Previously it was built unconditionally and torn down ~270 lines later — invisible (no await in between, so no frame composited; the cost is only ~9 ms at 1000 nodes), but any throw between the two calls left the layer visible against the stored preference. `showHeatMap()` now guards on the map existing instead of relying on `nodeData` being empty at that moment. ## Verification - Click on the first painted frame now persists, 3/3 (`localStorage` written, survives). - Restored state present on the first painted frame: 0 unchecked frames in 5 runs (was 3–5). - All four heat×matrix load combinations render identically to master (layer present/absent, checked, disabled). - `test-live-multibyte-only-e2e.js` unmodified: 3/3, plus 8/8 under CPU load. - With stored matrix ON, the heat toggle is `checked=false, disabled=true` from the first frame. ## The probe (as requested) <details><summary>~30-line Playwright harness that demonstrates the inert control</summary> ```js const { chromium } = require('playwright'); (async () => { const b = await chromium.launch(); for (let run = 0; run < 3; run++) { const ctx = await b.newContext({ viewport: { width: 1400, height: 900 } }); const p = await ctx.newPage(); await p.addInitScript(() => { window.__r = { clickedAt: null, afterClick: null, lsAfterClick: null, final: null, lsFinal: null }; const tick = () => { const el = document.getElementById('liveMultibyteToggle'); if (el && window.__r.clickedAt === null) { window.__r.clickedAt = performance.now(); el.click(); window.__r.afterClick = el.checked; window.__r.lsAfterClick = localStorage.getItem('live-multibyte-only'); return; } requestAnimationFrame(tick); }; requestAnimationFrame(tick); }); await p.goto('http://localhost:13581/#/live', { waitUntil: 'domcontentloaded' }); await p.waitForTimeout(2500); const r = await p.evaluate(() => { const el = document.getElementById('liveMultibyteToggle'); window.__r.final = el ? el.checked : null; window.__r.lsFinal = localStorage.getItem('live-multibyte-only'); return window.__r; }); console.log(`run ${run+1}: click at ${Math.round(r.clickedAt)}ms -> checked=${r.afterClick}, ls=${r.lsAfterClick}` + ` || after 2.5s: checked=${r.final}, ls=${r.lsFinal}`); await ctx.close(); } await b.close(); })(); ``` </details> ## Deliberately out of scope (each verified, none regressed here) - `#liveAudioToggle` has the same window (MeshAudio persists `live-audio-enabled`), but its restore runs through `MeshAudio.restore()` and a slider panel — its own change. - `#liveGeoFilterToggle` stays hidden until its own config fetch, so its window is not user-reachable; the fullscreen control is created by Leaflet after the map exists. - Pre-existing: `clearNodeMarkers()` (VCR resume path) drops the heat layer and nothing rebuilds it. `heatEnabled` is the right gate for fixing that, but it is a separate behaviour change. Happy to also submit the deterministic version of the multibyte test (it forces the window open by delaying `/api/config/map`, so it fails on this bug 3/3 instead of intermittently) as a follow-up if wanted. |
||
|
|
5d2e14aba2 |
fix(#1943): cancel the deferred swatch focus so arrow keys are not undone (#1945)
Closes #1943. The colour picker's keyboard navigation is broken, and the E2E flake that has been failing unrelated PRs (#1940, #1941, and master pushes `589fa987` and `859173f1`) was reporting it correctly. ## Cause `showPopover` deferred focusing the first swatch with an uncancellable `setTimeout(..., 0)` at `channel-color-picker.js:146`, and nothing cleared it on hide. The file contained **zero** `clearTimeout` calls. Reopen the popover while a swatch still holds focus and that timer lands after the user has already pressed an arrow key, pulling focus back to the first swatch. Proven, not argued. Instrumenting `HTMLElement.prototype.focus` with a stack trace, on one open: ``` focus(#f97316) @10205ms <- the keydown handler focus(#ef4444) @10208ms <- channel-color-picker.js:146:58 ``` Three milliseconds apart. ## The user-visible bug Worse than a flaky test. **Open the picker, arrow to a colour, press Enter, and the first colour is assigned instead of the one you chose.** Holding the timing still made the existing suite say so directly: ``` ✗ Enter should assign focused color (#f97316), got #ef4444 ``` ## Why the test looked flaky The revert happens on **every** open. Only whether the assertion reads before or after it varies, which is why an idle machine passes and a loaded runner does not. #1939 (mine) assumed the opposite: a race in which the handler had not yet moved focus, cured by waiting for it. #1943 has the measurement that disproves it. The failing step took **16 ms** while that wait has a **3 second** budget, so the wait was resolving successfully and then the value was reverted underneath it. It never helped. Its comment is corrected in this PR rather than left to mislead the next reader. ## Fix Keep a handle for the timer, cancel a pending one on both show and hide, and inside it do nothing when the popover has since been hidden or when focus already sits inside it. A fresh open still focuses the first swatch, which is what the accessibility behaviour is for. An open that inherits focus, or a user who has already navigated, is left alone. ## Verification - The **regression test added here fails on unmodified master** with `a late focus timer must not move focus after the user did` and passes with the fix. - It is **deterministic, not load-dependent**: it reproduces the exact sequence the stack trace identified (open, Escape, reopen, ArrowRight before the timer lands) rather than waiting for contention. It also asserts the Enter path, so the user-visible half is covered and not just focus position. - Full suite: 10 of 10, three consecutive runs. ## Note on the other flake This is one of two E2E failures blocking the queue. The other, #1925, is a different mechanism in a different file and is fixed separately in #1944. Together they should leave the E2E suite deterministic again. Same shape as @TeTeHacko's finding in #1940: something is operable before its setup has finished. That is now three instances in this codebase, so it may be worth a look as a pattern rather than three separate fixes. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
|
e2df9bbd3e |
fix(#1925): stop theme-refresh from discarding the neighbor-graph filter (#1944)
Closes #1925. This is the flake that failed #1942 and #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" (#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 #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 #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> |
||
|
|
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> |
||
|
|
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> |
||
|
|
a8e8449170 |
test(#1616): wait for swatch focus to move instead of reading it immediately (#1939)
This is the test that has been keeping master red on both sides of today's queue run. ## The failure ``` ✗ ArrowRight cycles focus across swatches: ArrowRight should move focus to next swatch (was #ef4444, now #ef4444) ``` Observed on: | where | when | |---|---| | master push `589fa987` | 2026-08-31 — the last completed master run before today | | master push `859173f1` | 2026-09-02 — the first completed master run after #1938 | | PR #1884 | 2026-09-02, passed unchanged on a re-run | **Two out of two completed master runs.** Master has produced exactly two finished pipelines since 2026-08-31 and this test failed both, which is why the branch has had no green badge either side of a day of merges. ## The cause ```js await page.keyboard.press('ArrowRight'); const nextColor = await page.evaluate(() => document.activeElement.getAttribute('data-color')); ``` It reads `document.activeElement` on the tick after the key press. The keydown handler moves focus, but under CI load that can land after the evaluate has already run, so the assertion compares the swatch against itself and reports the same colour twice. **This file already knows about this.** The "outside click" step below carries a long comment about exactly this macrotask race for #1317, and the conclusion there was to wait on the real condition instead of a proxy. That step got the treatment and this one did not. ## The fix Wait for `activeElement` to be a `.cc-swatch` whose `data-color` differs from the one focused before the key press, with a 3s budget. The wait is wrapped so that a timeout falls through to the original assertion, which then reports the value actually observed rather than a bare Playwright timeout — a failing test should still say what it saw. **No product code changed.** One file, +18 lines, all of it the wait and the reasoning. ## What this does not claim It does not prove the focus handler is correct, only that the test stops racing it. If ArrowRight is ever genuinely broken, this still fails, and now with a useful message. 🤖 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> |
||
|
|
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> |
||
|
|
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> |
||
|
|
c598abd210 |
chore(#1859): pin eslint@8 + the lock file it needs (continues #1880) (#1937)
Continues #1880 by @SaarMesh-Bot. Their commit is unchanged and keeps their authorship; I added the lock file it was missing. ## What was wrong #1880 added `eslint@^8.57.1` to `devDependencies` but not to `package-lock.json`. CI runs `npm ci --production=false`, which requires the two to be in sync, so the pipeline died at **Install npm dependencies** before a single test ran: ``` npm error code EUSAGE npm error `npm ci` can only install packages when your package.json and npm error package-lock.json are in sync. npm error Missing: eslint@8.57.1 from lock file npm error Missing: @eslint-community/eslint-utils@4.10.1 from lock file ``` I approved that PR on 2026-08-30 on the grounds that it was two lines of devDependency with no runtime effect. I checked that `.eslintrc.json` exists so `npm run lint` would resolve, and did not check the lock file. That was the miss. ## What this adds One commit: the regenerated `package-lock.json`. Generated with `npm install --package-lock-only`, so `node_modules` was never touched and the change is confined to the lock file. The 13 removed lines are npm reorganising the existing `find-up` / `find-cache-dir` entries because eslint brings its own versions of them; nothing unrelated was bumped. ## Verification | command | result | |---|---| | `npm ci --production=false` | **added 394 packages**, exit 0 | | `npm run lint` | exit 0, **92 problems (0 errors, 92 warnings)** | The warnings are pre-existing unused-variable reports across `public/*.js`. Not addressed here: the point of this PR is that the command runs at all, and whether to act on 92 warnings is a decision for #1859 rather than something to smuggle in behind a lock file. ## Why this matters beyond itself #1881 is the other half of #1859 and adds the gofmt/go vet CI gate. It has been sitting at the end of the merge order all day because it forces a rebase on every open Go PR. It also should not land before this one: the `lint` script it complements is not installable until the lock file is right. @SaarMesh-Bot — your work, your credit. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01Wzwr3eXseyNM7Xj598djjE --------- Co-authored-by: SaarMesh-Bot <300107934+SaarMesh-Bot@users.noreply.github.com> Co-authored-by: Claude <noreply@anthropic.com> |
||
|
|
859173f145 |
ci: unblock the master pipeline — gate the staging deploy, detach the badges job (#1938)
Fixes the problem @sylr reported on #1922. ## What is broken **master has produced no completed pipeline result since 2026-08-31.** Thirty-plus runs today alone were cancelled or left queued, while `go-test`, `e2e-test` and `build-and-publish` were passing inside them. The `deploy` job runs on `[self-hosted, meshcore-runner-2]`, and no such runner has picked up a job since at least 2026-08-31. It has no `timeout-minutes`, so it sits queued indefinitely. That job holds its run open, the run holds the concurrency group `ci-refs/heads/master`, and GitHub then cancels every subsequent master push while one waits. Two runs showing it, one from yesterday and one from right now: ``` |
||
|
|
25090230f2 |
fix(map): render the Esri labels overlay it was already named for (rebase of #1917) (#1935)
Continues #1917 by @nullrouten0. The commit is theirs, authorship unchanged; I only rebased it onto master. It went CONFLICTING because #1891 (the OpenTopoMap and USGS layers) landed in the same `BASE_STYLES` block, and both PRs also add cases to `test-issue-1420-tile-providers.js`. Resolution: kept #1891's two `usgs-*` entries and took this PR's `esri-darkgray-labels` line, which is the one that adds `refUrl`. Both test suites kept in full. Nothing else touched. Verified: `test-issue-1420-tile-providers.js` 47 passed, 0 failed, which includes this PR's four Esri cases and the Carto key cases that landed since. My review stands: approve. The id `esri-darkgray-labels` promised labels the layer control never stacked, and the test asserting that single-layer providers stay bare tile layers is the part that makes this safe to merge. Co-authored-by: nullrouten <nullrouten@gmail.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
|
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> |
||
|
|
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. |
||
|
|
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> |
||
|
|
a46a6d55eb |
fix: use replaceState for traces/roles redirects to preserve back-button history (#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. |
||
|
|
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> |
||
|
|
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> |
||
|
|
aabbd50d27 |
feat(nodes): export the visible node list as MeshCore companion contacts JSON (#1889)
## What
Adds an **Export JSON** button to the Nodes topbar that downloads the
currently visible node list as a MeshCore companion-app config file:
```json
{
"contacts": [
{
"type": 2,
"name": "BE-MGU-RP03 | ON7YT",
"custom_name": null,
"public_key": "7a7a37d4819fb27440ed1439ca7d281fdecceb83b27e61a69745feb004d726a4",
"flags": 0,
"latitude": "51.07307",
"longitude": "5.5796",
"last_advert": 1786545431,
"last_modified": 1786545431,
"out_path_list": null
}
]
}
```
That is the exact shape the companion app itself writes, so the file
imports straight back into a companion app as contacts. The practical
use case is per-area: pick an area in the area filter, export, and hand
someone the repeaters for that region instead of having them wait for
adverts.
## Scope of the export
WYSIWYG — the button exports the rows the table is showing, in the
table's current order, so area, region, role tab, search, last-heard and
status filters all carry over. The button label shows how many contacts
the file will contain and is disabled when that count is zero.
## Field mapping
| JSON field | Source | Notes |
|---|---|---|
| `type` | `node.role` | repeater→2, companion→1, room→3, sensor→4;
unknown/empty→1 |
| `name` | `node.name` | unchanged, emoji included |
| `custom_name` | — | always `null` |
| `public_key` | `node.public_key` | full 64-hex |
| `flags` | — | always `0` |
| `latitude` / `longitude` | `node.lat` / `node.lon` | stringified, as
the format expects |
| `last_advert` | `node.last_seen` | RFC3339 → unix seconds |
| `last_modified` | mirrors `last_advert` | no separate source exists |
| `out_path_list` | — | always `null`; the companion app discovers
routes itself |
Nodes are skipped when they have no name, a pubkey shorter than 64 hex
chars, or no usable position (missing, non-numeric, or null island).
Filename: `corescope_nodes_<area|all>_YYYY-MM-DD-HHMMSS.json`.
## Shape of the change
The mapping lives in a self-contained `public/nodes-export.js`
(`window.NodesExport.buildContacts/filename/download`); `nodes.js` only
gains the button markup, a click handler and a small
`updateExportBtn()`. No backend change, no new API call — the export
reuses the already-fetched node list, so there is nothing per-node to
fetch.
## Tests
- `test-nodes-export.js` — field mapping and key order, role→type table,
skip rules, order preservation, filename format (added to
`test-all.sh`).
- `test-nodes-export-wiring.js` — `index.html` loads the module before
`nodes.js`; the button lives in the topbar and hands the filtered
`nodes` array plus `AreaFilter.getSelected()` to
`NodesExport.download()` (added to `test-all.sh`).
- `test-nodes-export-e2e.js` — Playwright: downloads the file, validates
the JSON shape per contact, asserts the button count matches the file,
and that narrowing the search shrinks the export set.
## Browser validation
Deployed to staging and checked in Chromium:
- Desktop 1400×900 — button renders at the right of the topbar next to
the count pills, `Export JSON (1962)`.
- Mobile 390×844 — topbar stacks, button visible, no horizontal overflow
(`scrollWidth == clientWidth`).
- E2E against staging: 1761 contacts exported,
`corescope_nodes_all_2026-08-12-164349.json`, shape validated, search
narrowing confirmed.
- With the `BE-LIM` area filter active: 44 contacts,
`corescope_nodes_BE-LIM_2026-08-12-164445.json`, type histogram `{1: 1,
2: 42, 3: 1}`.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
|
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> |
||
|
|
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> |
||
|
|
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>
|
||
|
|
0fd22039cb |
feat(map): Important Links overlay (rebase of #1771 onto master) (#1928)
Continues #1771 by @ArcanConsulting. Both commits are theirs, authorship unchanged; I only rebased them onto current master. Opening it here rather than force-pushing to someone else's branch. ## Why the rebase was needed #1771 went CONFLICTING through no fault of its author: #1760 landed first and both PRs append a line to `test-all.sh` at the same spot. That was the entire conflict. ## What I changed One line, and it is the conflict resolution: `test-all.sh` now runs **both** test files rather than either. ``` node test-repeater-metric-scatter.js # from #1760 node test-top-routes-overlay.js # from this PR ``` Nothing else was touched. `public/map.js` and `test-issue-1329-map-controls-accordion-e2e.js` are byte-for-byte as the author wrote them. ## Verification on the rebased tree | | result | |---|---| | `test-top-routes-overlay.js` (this PR's own) | 20 passed, 0 failed | | `test-repeater-metric-scatter.js` (#1760's, must still pass) | 31 passed, 0 failed | | `test-frontend-helpers.js` | 627 passed, 0 failed | ## The one review point that still stands From my review on #1771, unchanged by the rebase and not something I fixed on the author's behalf: `test-top-routes-overlay.js` extracts the ranking core by `indexOf`-slicing `public/map.js` between the literals `const TOP_ROUTES_AXES` and `function clearTopRoutes`, then `new Function`s the result. There is a guard assertion for the rename case, which is thoughtful, but it still breaks on any reordering of map.js and it tests a string rather than the module. Two PRs in this same queue do it properly and are worth copying: #1821 exports `applyObserverFilter` through `_packetsTestAPI`, and #1912 puts `hashPrefixInfo` on `window`. Happy to take that as a follow-up rather than block the overlay on it. @ArcanConsulting — this is your work and the credit is yours. Say the word and I will close this and hand the rebase back, or push it to your branch instead if you would rather #1771 stayed the vehicle. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01Wzwr3eXseyNM7Xj598djjE --------- Co-authored-by: Arcan Consulting - Michael J. Arcan <github@arcan-it.de> |
||
|
|
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> |
||
|
|
34b41fd5b6 |
Add topographic map layers (#1891)
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. |
||
|
|
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> |
||
|
|
c5a71b34ec |
fix(#1890): drop the hardcoded og:url so shared links stay on the instance (#1893)
Fixes #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> |
||
|
|
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> |
||
|
|
a3454e7508 |
fix(#1858): replace emoji map icon with Phosphor sprite in rx-coverage (#1860)
## Summary `public/rx-coverage.js` still carried a literal `🗺️` (U+1F5FA) in the Mobile RX coverage page header — missed by the #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 #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> |
||
|
|
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> |
||
|
|
4fc42d30a7 |
fix(frontend): relay-aware staleness for infra nodes + dim-not-delete (#1598, PR A) (#1815)
Implements **PR A** from the #1598 triage fix path (r6 — all three operator decisions locked: confidence ≥0.75 [PR B], **dim-not-delete**, **in-place `getNodeStatus` signature extension**). ## Changes **`public/roles.js` — `getNodeStatus()`** - Now accepts a full node object (preferred); the legacy `(role, lastSeenMs)` signature keeps working unchanged. - For infra roles (repeater/room), freshness = `max(advert-based timestamp, last_relayed)`. Freshness precedence mirrors existing call sites: `_liveSeen` > `_lastHeard` > `last_heard` > `last_seen`. - `last_relayed` is only consulted for infra — companions keep pure advert/heard-based staleness (per @liquidraver's collision caveat; the ≥0.75-confidence `_liveSeen` refresh is PR B). **`public/live.js` — `pruneStaleNodes()`** - Repeater/room markers are **dimmed, never deleted**, regardless of `_fromAPI` origin. WS-only non-infra nodes are still removed to prevent unbounded memory growth. **Call sites** — all seven (`nodes.js` ×3, `map.js` ×3, `live.js` ×1) now pass the node object. The Nodes-page status explanation shows "Last relayed …" when relay participation is the fresher signal, so an Active badge next to an old "last heard" isn't confusing. **Tests** — 16 new `getNodeStatus` unit tests incl. the triage's backbone-repeater fixture (`last_seen`=25h, `last_relayed`=5min → `active`); `pruneStaleNodes` tests updated for dim-not-delete plus a new relay-aware prune test. ## Test results `node test-frontend-helpers.js`: **641 passed, 2 failed** — the 2 failures (favStar ★ assertions) are pre-existing on current master (verified on a clean upstream clone). ## Validation offer live.saarmesh.de currently has **160 infra nodes past `infraSilentMs`=72h while actively relaying** (incl. KatS disaster-relief repeaters) — a ready-made test population. Happy to run this branch there and report before/after node visibility. Refs #1598 (PR A of two; PR B = `_liveSeen` refresh on `resolved_path` ≥0.75 confidence). --------- Co-authored-by: Mathias Kasper <fallisaar@gmail.com> Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: SaarMesh-Bot <bot@saarmesh.de> |
||
|
|
6528c7ba3e |
fix(#1854): move relay last_seen touch to the ingestor — server writes have been no-ops since mode=ro (#1855)
Fixes #1854. Refs #1598, #1611, #1845.
## The bug
`cmd/server/db.go:54` opens SQLite `mode=ro` (#1283/#1289).
`touchRelayLastSeen` → `TouchNodeLastSeen` issues `UPDATE nodes SET
last_seen` on that handle. It has failed on every call since, with the
error discarded at the call site:
```go
if err := s.db.TouchNodeLastSeen(pk, ts); err == nil {
s.lastSeenTouched[pk] = now
}
```
`nodes.last_seen` has therefore tracked ADVERT arrivals only. Verified
on live.saarmesh.de (1388 nodes): 1362 have `last_seen` within one
minute of their own most recent ADVERT. Reproduced directly with the
server's DSN in #1854.
Secondary effect: `lastSeenTouched` is populated only in the success
branch, so the debounce never engaged — the server retried the failing
UPDATE for every resolved pubkey in every decode window.
## The fix
The writer moves to `cmd/ingestor`, which owns `nodes` per #1283/#1287
and since #1547 already resolves hop prefixes to full pubkeys for
`observations.resolved_path`. The touch hooks into that existing
resolution point, so there is no new IPC surface and no second resolver.
Only unambiguously resolved hops qualify — a 1-byte prefix collision
cannot keep a silent node alive.
I considered the `internal/mbcapqueue` snapshot handoff used for
#903/#1324 and did not need it: that pattern exists because the
capability computation lives in the server's analytics cycle. Path
resolution already happens in the ingestor, so a file handoff would add
a hop for nothing.
`Store.TouchRelayNodes`:
- monotonic guard in SQL (`last_seen IS NULL OR last_seen < ?`) —
out-of-order ingest never rewinds
- 5-minute debounce keyed on `rxTime`, matching the interval the server
intended
- UPDATE only — unknown pubkeys never create rows
- unparsable `rxTime` is a no-op rather than writing garbage into the
node directory
- `Stats.RelayTouches` for `/api/perf` visibility
- debounce records the *attempt*, not the row match, so an unknown
pubkey is not retried per observation
## Server-side removal
`touchRelayLastSeen`, `DB.TouchNodeLastSeen`, the `lastSeenTouched` map
and the now-unused `allResolvedPKs` decode-window map are deleted.
`readonly_invariant_test.go` gains `UPDATE\s+nodes\s+SET\s+last_seen`.
`cmd/server/touch_last_seen_test.go` and two tests in
`resolved_index_test.go` go with it. Worth stating why they were green
for months: they build their `PacketStore` on `setupTestDB`, which opens
read-write. The production constraint is the one thing they did not
reproduce, which is why the added invariant regex — not a replacement
unit test — is the right guard here.
## Tests
Five tests in `cmd/ingestor/relay_touch_test.go`, committed red first
(
|
||
|
|
176bb53335 |
fix(#1784): ship pathTrust default 1, not 2 (#1929)
Follows #1841. Moves the pathTrust default from 2 back to 1. ## Why #1784's first acceptance criterion is **"Default behaviour remains backward-compatible"**, and its example config shows `minHashBytesForMapping: 1`. What shipped is 2. The problem is not the value. It is that **there is no way to undo it from the UI.** #1841 adds no control for the threshold: it is `config.json` only, and changing it needs a restart. The customizer gains a hint that says exactly that. So an instance that upgrades without touching config switches to the stricter rule, and the only visible symptom is that the neighbour graph and the resolved paths quietly get smaller. The existing "Hide 1-byte path hops" toggle (#1633) is a *display* filter and does not change what counts as evidence, so it is not an escape hatch either. The two are easy to confuse. ## How much this actually moves Measured on a live instance via `/api/analytics/hash-sizes`, not estimated: | path-hop observations | count | share | |---|---|---| | 1-byte prefix | 116,923 | **56.0%** | | 2-byte prefix | 86,031 | 41.2% | | 3-byte prefix | 5,753 | 2.8% | | repeaters by observed hash size | count | |---|---| | 1-byte | **645 (41%)** | | 2-byte | 865 | | 3-byte | 63 | At threshold 2 the 1-byte column stops counting as mapping evidence. `MeetsPathTrust` also drops the legacy bucket-0 observations with it (pre-#1638 persisted neighbor edges that carry no per-mode breakdown), so already-stored edges lose their evidence status on upgrade too. ## What this does not change The knob works and is untouched. Operators who want the stricter behaviour set `minHashBytesForMapping` to 2 or 3, which is the opt-in #1784 describes. Only the default moves. Nothing about storage changes; packets and paths were never affected either way. ## Also fixes an inconsistency inside #1841 Five frontend consumers already fall back to **1** when `MC_getPathTrustThreshold()` is unavailable: `analytics.js`, `live.js`, `map.js`, `nodes.js`, `route-view.js`. Two fell back to **2**: `hop-filter.js` (the getter itself) and `customize-v2.js`. They now all agree. ## Tests - `internal/packetpath`: `TestMeetsPathTrust_ZeroValueOptIn` was asserting the old default *through behaviour*, so it would need rewriting on any future default change. It now asserts the property instead: an absent JSON field resolves to `DefaultMinHashBytesForMapping`, behaves identically to naming that value outright, and an explicit stricter setting still wins. Package tests pass. - `test-issue-1633-hide-1byte-hops.js`: the case pinning the getter's default is updated, with the reasoning in a comment so the next person sees why it is 1. **37 passed, 0 failed** (master baseline: 37 passed, 0 failed). - `cmd/server` config tests pass. ## One thing I want to flag rather than paper over The test I changed was named `default is 2 (operator-confirmed)`. I am overriding something that was confirmed with an operator, and I am not claiming that confirmation was wrong. My reading is that it was about the threshold being a *useful* value, which it is, rather than about it being the default in a build with no UI to change it. If the intent really was "2 out of the box for everyone", say so and I will close this. @Bjorkan as the issue author, @nullrouten0 and @Saarlandpower since you have touched adjacent code. 🤖 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> |
||
|
|
b3a306b81f |
fix(#1888): count only live observers in the store's /api/stats query (#1892)
Fixes #1888. ## The mismatch `/api/stats.totalObservers` and `/api/observers` counted different sets: | Source | Predicate | |---|---| | `cmd/server/store.go:2089` (store path) | `SELECT COUNT(*) FROM observers` — every row | | `cmd/server/db.go:336` (DB fallback) | `WHERE inactive IS NULL OR inactive = 0` | | `db.GetObservers()` → `/api/observers` | `WHERE inactive IS NULL OR inactive = 0` | `handleStats` uses the store path whenever a `PacketStore` exists (`routes.go:785`), which is every normal deployment. So the header count came from the unfiltered query while the Observers page listed the filtered set. The two stats implementations also disagreed with each other for the same database, which is a bug on its own. ## Reproduction The gap is exactly the observers the `observerDays` retention sweep has soft-deleted. On the instance I reproduced against: ``` GET /api/stats → totalObservers: 79 GET /api/observers → observers.length == 51 ``` ```sql SELECT 'all', COUNT(*) FROM observers -- 79 UNION ALL SELECT 'active', COUNT(*) FROM observers WHERE inactive IS NULL OR inactive = 0 -- 51 UNION ALL SELECT 'inactive', COUNT(*) FROM observers WHERE inactive = 1; -- 28 ``` 79 − 28 = 51. Same shape as the 82 vs 51 in the issue. ## The change One line: the store's stats query gets the same predicate the other two already use, so all three agree. ## Deliberately out of scope Two things the issue raises that this does **not** fix, called out so they are not mistaken for done: - **Config blacklist.** `buildObserversDefaultResponse` drops blacklisted observers in the handler loop (`routes.go:2752`), which no SQL count can see. A deployment with a non-empty `observerBlacklist` will still show a stats count higher than the list, by the number of blacklisted-but-live observers. Closing that needs config plumbing into the count and is a separate change — happy to follow up if wanted. - **Map controls.** The third surface named in the issue derives its count from node role aggregates (`roleCounts`), not from the observer set at all. That is a frontend concern and untouched here. ## Tests `cmd/server/observer_count_1888_test.go`, three cases, each watched fail first: 1. `TestStoreStatsTotalObserversExcludesSoftDeleted` — `TotalObservers = 5, want 4` 2. `TestStoreStatsTotalObserversMatchesObserverList` — `stats totalObservers = 5 but /api/observers lists 4` 3. `TestStoreAndDBStatsAgreeOnTotalObservers` — `store path reports 5 observers, DB fallback reports 4` The fixture includes a row with `inactive = NULL` alongside `inactive = 0` and `inactive = 1`, since `GetObservers` treats NULL as live and only the `1` may be excluded. `cd cmd/server && go test ./...` → ok (87s). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
|
02c2338d64 |
fix(packets): observer filter hides multi-observer rows in grouped mode (#1748) (#1821)
Closes #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 #537 for the case where `_children` is already populated. ## Tests Added 6 cases in `test-frontend-helpers.js` covering the specific gap #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> |
||
|
|
0352c9a287 |
fix(clock-skew): restrict per-node skew to self-originated adverts (#1816, #1818) (#1820)
Closes #1816. Closes #1818 (confirmed duplicate of #1816 by the triage bot). ## Root cause `byNode` is an involvement index (`indexResolvedPathHops`, `store.go:1696-1705`, #1558/#1352): a transmission is indexed under every relay-hop pubkey found in an observation's `resolved_path`, not just its originator. `getNodeClockSkewLocked` (`clock_skew.go:489`) iterated every ADVERT transaction under a pubkey without checking who actually signed it, so a relay inherited the clock skew of every broken-clock node it forwarded as if it were its own. This produced: - Fleet-wide false `no_clock`/`bimodal_clock` classifications on healthy relays whose only "bad" samples were adverts they merely relayed. - Bit-identical `RecentMedianSkewSec` "clusters" across unrelated relays that all forwarded the same broken-clock originator. - Single relays showing a multi-day skew even though their own self-adverts are healthy, because 1-2 relayed adverts from a broken originator landed in the tail of their small recent-window sample (the #1818 "island" repro from @cwichura). ## Fix Add `txOriginatedBy(tx, pubkey)`: ADVERTs are self-signed, so `decoded["pubKey"]` is the originator per protocol (case-insensitive compare as a defensive measure). Apply it as a guard in both the main skew-aggregation loop and the per-hash evidence loop in `getNodeClockSkewLocked`. `byNode` itself is untouched — #1558/#1352 still rely on the broader involvement index for other consumers. ## Tests - Existing `clock_skew_test.go` / `clock_skew_issue1094_test.go` / `clock_skew_issue1285_test.go` fixtures built synthetic ADVERT transactions without a `pubKey` field and seeded `s.byNode` directly, bypassing the normal `indexByNode` path where every real ADVERT carries `pubKey`. Added `pubKey` to each fixture so it reflects a self-originated advert, which is what these tests already intended to represent. All pre-existing tests pass unchanged in behavior. - New `clock_skew_issue1816_test.go`: - `TestTxOriginatedBy` — unit coverage of the new guard (self, foreign, missing pubKey, case-insensitivity). - `TestIssue1816_RelayDoesNotInheritOriginatorSkew` — a relay with healthy self-adverts plus relayed adverts from a broken-clock originator (matching the report's +100.5k s band) must report `ok` severity based only on its own adverts. - `TestIssue1816_PureRelaysReportNoSkew_NoBitIdenticalCluster` — five relay pubkeys that only ever forward a broken originator's advert (never self-advert) must report `nil`, not a bit-identical copy of the originator's skew. - `TestIssue1818_TwoForeignAdvertsDoNotPoisonIslandNode` — reproduces the cwichura island scenario: 8 healthy self-adverts + 2 foreign adverts at ~10 days skew must not flip severity or pollute `RecentMedianSkewSec`. Full suite: `go test ./...` passes (one pre-existing, unrelated flaky test — `TestHandleNodePaths_PrefixCollision_1352`, an index-loading race — reproduces intermittently on unmodified `master` too). Operator context: running CoreScope for SaarMesh (SaarLorLux, DE/FR/LU, 800+ nodes); this bug was surfacing as fleet-wide clock-skew false positives on our infra nodes. Co-Authored-By: Claude <noreply@anthropic.com> Co-authored-by: Claude <noreply@anthropic.com> |
||
|
|
9ef4179ef1 |
fix(release): report correct version on fast-path retagged images (#1807) (#1814)
Fixes #1807. Implements the fix path from the triage (env → image-version file → baked version, zero rebuild cost): ## Changes **`cmd/server/main.go` — `resolveVersion()` fallback chain** 1. `CORESCOPE_VERSION` env (operator override) 2. `.image-version` file in the working dir (`/app` in the container) — mirrors the existing `.git-commit` pattern in `resolveCommit()` 3. ldflags-baked `Version` 4. `"unknown"` Edge builds are unaffected: no env, no file → baked `"edge"` as before. **`.github/workflows/release-fast-path.yml` — retag step** Instead of a plain `crane tag :edge → :vX.Y.Z`, the fast path now runs `crane mutate` on `:edge` with: - `--append` of a deterministic one-file layer containing `/app/.image-version` = `vX.Y.Z` - `--label org.opencontainers.image.version=vX.Y.Z` - `--tag :vX.Y.Z` `vX.Y`, `vX` and `latest` are then pointed at the mutated image. Still no rebuild — the mutation is a manifest + single ~100-byte layer operation. ## Notes - The release tags no longer share the exact digest with `:edge` (they carry one extra layer); the fallback SHA check is unaffected since it compares the `org.opencontainers.image.revision` label against `github.sha`. - The layer tar uses `--owner=0 --group=0 --mtime='UTC 2020-01-01'` for reproducibility. - Operators can also fix existing deployments immediately with `-e CORESCOPE_VERSION=v3.9.2`, no image change needed. ## Testing - `go build ./cmd/server` + `go vet` clean (golang:1.24) - Workflow YAML validated - Reporter context: running the affected v3.9.2 fast-path image in production (live.saarmesh.de), happy to verify the next tagged release end-to-end. --------- Co-authored-by: Mathias Kasper <fallisaar@gmail.com> Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: SaarMesh-Bot <bot@saarmesh.de> |