fix(nodes): keep active observers as nodes and explain inactive ones (#199)

[dborup-agent]

Relates to #199

Problem

MoveStaleNodes retires every node whose last_seen (last advert) is older than retention.nodeDays. Observer activity never counts, so a repeater that is online as an observer can vanish as a node. The Observers page still lists it, and its "View node detail" link then dead-ends on a bare "Node not found".

Plan

1. Tests first (commit 1, red on master): ingestor retention, the server 404 body, frontend unit, and an E2E with a fixture seed.
2. A, ingestor: keep nodes whose pubkey is an observer seen within nodeDays.
3. B, server and UI: the node page explains an inactive or observer-only device instead of dead-ending.
4. Browser check of observer detail and the node page for an inactive observer, with a local server on the seeded e2e-fixture.db.

Nothing becomes configurable. retention.nodeDays and the map status filter are untouched.

A. Ingestor: active observers are not retired

MoveStaleNodes (cmd/ingestor/db.go) uses a single shared predicate for both the INSERT … inactive_nodes and the DELETE:

last_seen < ?1 AND lower(public_key) NOT IN (
  SELECT lower(id) FROM observers WHERE id IS NOT NULL AND last_seen >= ?1)

• Observer ids are upper-case pubkeys and node keys are lower-case, so both sides are normalised.
• last_seen is not touched, so the node page still shows the last advert.
• When the observer goes quiet (no upload within nodeDays), the node is retired as before.
• The id IS NOT NULL guard is needed because a single NULL id would make NOT IN false for every node and stop retention entirely. A test covers this.
• Perf: this runs at start-up and once a day. The subquery is materialised once over the observers table (hundreds of rows). It does not run on a hot path.

B. Node page explains instead of dead-ending (option 1)

Why option 1: it covers every route to #/nodes/<pubkey>, not only the observer link (hop links and shared URLs too). It also covers observers that never had a node row at all; hiding the link (option 2) would need the observer page to probe the node table first. It needs no extra request: the explanation travels in the 404 body the page already gets.

• Server (read-only): a miss in handleNodeDetail now answers 404 with the named struct nodeNotFoundResponse (cmd/server/node_not_found.go), shaped {error, inactive_node?, observer?}.
  • inactive_node is the inactive_nodes row: name, role, last_seen (the last advert), and first_seen.
  • observer is the observers row (id, name, last_seen).
  • The lookups are two small selects that run only on this 404 path. hasInactiveNodesTable from identity_visibility.go is reused.
  • Blacklisted (node or observer) and hidden-name identities keep the bare {"error":"Not found"}, using the same identityHidden rule as Reach. A failed lookup also falls back to the bare 404.
  • No map[string]interface{} is added. OpenAPI documents the 404 body.
• Frontend:
  • api() attaches a JSON error body as err.body; a non-JSON body leaves it unset.
  • nodes.js builds the view with missingNodeView(pubkey, body), a pure function exposed for tests.
  • Inactive node: "No advert heard since ; this device is inactive", with name, role and last advert from the inactive row, the last observer upload, and a "View observer →" link.
  • Observer-only: "No node record", with the reason (no advert heard).
  • Unknown key: keeps the existing "Node not found" (bug(node-detail): full-page header title stuck on "Loading…" when /api/nodes/{pubkey} returns 404 Kpa-clawbot/CoreScope#1150).

Tests

• cmd/ingestor/issue199_test.go:
  • an active observer (upper-case id) keeps its node, and last_seen is unchanged;
  • a quiet observer and a plain stale node are retired;
  • a NULL observer id does not block retention.
• cmd/server/issue199_node_not_found_test.go:
  • the 404 carries the inactive row and the observer;
  • observer-only;
  • an unknown key stays bare;
  • works without an inactive_nodes table;
  • node blacklist, observer blacklist and both hidden-name cases stay bare.
• test-issue-199-missing-node.js (in test-all.sh): api() err.body with JSON and non-JSON bodies, both views, the unknown key, and escaping.
• test-issue-199-inactive-observer-e2e.js and test-fixtures/seed-199-inactive-observer.sql (wired into deploy.yml after the fixture migration): Observers → "View node detail" → explanation, plus the observer-only node page.
  • The seed reuses a fixture observer that has no node row, so no observer counts change for other E2E tests.

Out of scope

• Nodes that were already retired before this change return to nodes on their next advert. The node page explains them until then. This PR does not move rows back out of inactive_nodes.

🤖 Generated with Claude Code

[dborup-agent]

Rapport — CS-pve-agent2 PR#203 #199 — head 2c2544b

Status: Draft; all acceptance criteria met; CI green on every job that ran. Not merged, not marked ready.

Evidence tags: [T] verified by a test or a run, [K] verified by reading the code, [A] assumption, not verified.

Commits

1. 4cd187d4 test: red on master. Ingestor test, server 404 test, frontend unit test, E2E plus seed, and the wiring in test-all.sh and deploy.yml.
2. 2c2544be fix: A and B, plus one stronger assertion in the unit test (see mutant B5).

Acceptance criteria

1. An online observer whose last advert is older than nodeDays stays a node.
   • [T] TestMoveStaleNodesKeepsActiveObserverNode: the observer id is upper-case and the node key lower-case. The node stays in nodes, is not copied to inactive_nodes, and last_seen keeps the old advert time (nothing is faked). It was red on master: moved=1, want 0.
   • [K] Map and node page read the nodes table, so a node that stays in nodes stays visible there. The map status filter was not touched.
2. A node in neither nodes nor the active observer set is retired as before.
   • [T] TestMoveStaleNodesRetiresQuietObserverAndPlainNode: an observer last seen 8 days ago (nodeDays 7) and a plain stale node are both moved.
   • [T] TestMoveStaleNodesNullObserverIDDoesNotBlockRetention: a NULL observer id does not stop retention (the NOT IN + NULL trap).
   • [T] The existing TestMoveStaleNodes* tests still pass.
3. From the Observers page, a device without a nodes row never dead-ends on a bare "Node not found".
   • [T] Option 1 is implemented. The 404 body of /api/nodes/{pubkey} carries inactive_node and/or observer, and the node page explains the state.
   • [T] E2E test-issue-199-inactive-observer-e2e.js on the seeded fixture: Observers detail → "View node detail" → "No advert heard since ; this device is inactive", with name, role and an observer link. An observer with no node record at all gets "No node record" with the reason.
   • [T] The unit test test-issue-199-missing-node.js covers the same views plus escaping.
   • Why option 1: it covers every route to #/nodes/<pk> (hop links, shared URLs), not just the observer link. It also needs no extra request, because the explanation rides on the 404 the page already receives.
   • [T] Unknown keys keep the bug(node-detail): full-page header title stuck on "Loading…" when /api/nodes/{pubkey} returns 404 Kpa-clawbot/CoreScope#1150 "Node not found" (test-issue-1150-404-state-e2e.js 5/5 locally).
4. No new per-item API calls, and the server stays read-only.
   • [K] No new request was added. The data comes in the existing 404 response. The server only runs two SELECTs, and only on the 404 path.
   • [K] All writes stay in cmd/ingestor. The read-only invariant test passes in the server suite.
   • [K] The response uses named structs (nodeNotFoundResponse, inactiveNodeInfo, observerRef), with no new map[string]interface{}.
   • [T] Blacklisted (node or observer) and hidden-name identities keep the bare 404 (TestNodeDetail404HiddenIdentityStaysBare, 4 subtests).

Tests (local)

• [T] cd cmd/ingestor && go test ./...: ok (616 s).
  • The affected tests also pass with go test -race -run MoveStaleNodes.
  • The first attempt, with three suites running in parallel on 4 cores, hit the default 10-minute timeout with no failing test. A sequential rerun with -timeout 40m passed. CI uses -timeout 20m.
• [T] cd cmd/server && go test ./...: ok (375 s, sequential rerun, same reason).
• [T] sh test-all.sh: 202 passed, 0 failed (202 files).
• [T] E2E against a local Go server on a copy of e2e-fixture.db, prepared with freshen, migrate and the 2073 and 199 seeds:
  • Active observers vanish as nodes after retention.nodeDays without an advert; observer link dead-ends on 'Node not found' #199: red on master, 0/3. After the fix, 3/3.
  • bug(node-detail): full-page header title stuck on "Loading…" when /api/nodes/{pubkey} returns 404 Kpa-clawbot/CoreScope#1150: 5/5.
  • The server was stopped by pid afterwards, and the port was confirmed free.
• [T] scripts/check-xss-sinks.sh --diff: clean.

Mutants (14 run, all killed in the final state)

Part A (ingestor):

#	Mutant	Killed by
A1	no lower() on the observer id	KeepsActiveObserver
A2	no id IS NOT NULL	NullObserverID
A3	any observer protects, ignoring last_seen	RetiresQuietObserver
A4	exclusion only on the INSERT, not on the DELETE	KeepsActiveObserver

Part B (server):

#	Mutant	Killed by
B7	no identityHidden check	HiddenIdentity, 3 subtests
B8	case-sensitive observer lookup	4 tests
B9	hidden-name check ignores the names found	2 subtests
B10	handler back to the bare 404	3 tests

Part B (frontend):

#	Mutant	Killed by
B2	api() drops err.body	unit
B3	name not escaped	unit (XSS check)
B4	catch block ignores the view	E2E, 2 checks
B6	no observer link	unit + E2E
B5	observer-only explanation without the reason	unit, after the fix commit

• B5 initially survived: the headline "No node record" alone satisfied the assertion. In the fix commit the unit test now also requires the reason ("no advert from it has been heard"), and the rerun killed it.
• A first B5 variant was an equivalent mutant (the next statement overwrote it), so it was discarded and is not counted.

Browser

• [T] Local server on the seeded fixture, Chromium:
  • Observer detail: an online observer with "View node detail →".
  • The link opens the node page with the title set to the inactive name and the card "Inactive node". It shows "No advert heard since ; this device is inactive", the name, the role, the last advert, the last observer upload, and the buttons "View observer →" and "← Back to Nodes". "View observer" goes back to the observer.
  • The observer-only node page shows "No node record" with the reason.
  • Mobile at 390 px in the light theme: the card wraps cleanly.
  • An unknown key still shows "Node not found — …".
  • No page errors.
• [A] Dark theme was not screenshotted. The card uses only existing CSS variables and classes.

CI (run 37180838265, head 2c2544b)

• [T] Go Build & Test: pass.
  • server ok, 89.8% coverage; ingestor ok, 79.5%.
  • test-all.sh 202/202, including test-issue-199-missing-node.js.
  • The preflight XSS gate passed.
• [T] Playwright E2E Tests: pass.
  • The seed-199 step ran.
  • test-issue-199-inactive-observer-e2e.js passed 3/3; aggregate PASS=26 FAIL=0.
• [T] Build & Publish Docker Image: pass.
• Release Artifacts, Deploy Staging and Publish Badges & Summary were skipped, as expected on a PR.

Remaining items

• [K] Nodes retired before this change stay in inactive_nodes until their next advert. The node page explains them in the meantime. Moving them back to nodes is out of scope; it could be a follow-up if wanted.
• [K] The protection uses observers.last_seen only, as the issue proposes. A soft-deleted observer (inactive = 1) whose last_seen is still inside nodeDays keeps its node until that window passes. That can only happen when observerDays < nodeDays.
• [A] Not verified on any real deployment. There was no staging or production access, as required.

[dborup]

Review — CS-cloud PR#203 observer-node-retention — head 2c2544b

Dom: APPROVE with nits

Independent, read-only review of head 2c2544be and of the merge result against current origin/master (33b0dfe5; git merge-tree is clean). Head was 2c2544be in git ls-remote both before and after the review.

Evidence tags: [T] test, run or CI I checked; [A] my analysis of the code; [K] taken from the author's report and not re-run.

Findings

#	Prio	Where	Description
1	P3 nit	cmd/server/node_not_found.go, writeNodeNotFound	A failed lookupMissingNode falls back to the bare 404, which is right. But the error is dropped silently, so a broken lookup (for example a schema drift on inactive_nodes) would just look like "Node not found" with no trace. Suggest a log.Printf on that branch. [A]
2	P3 nit	test-issue-199-missing-node.js / E2E	My mutant M8 (drop the Last advert row from missingNodeView) survives the unit test and the E2E, because the same date also appears in the explanation sentence. Optional: assert the Last advert label as well. [T]
3	P3 nit	public/nodes.js, missingNodeView copy	An observer can be uploading right now while its node row is still in inactive_nodes: it was retired before this change, or while the observer was quiet. The card then says "this device is inactive" next to a fresh "Last upload as observer" time, which reads as a contradiction. Consider "inactive as a node" or similar. The issue proposed the current wording, so this is optional. [A, seen in browser]
4	Info	scope	Nodes that are already in inactive_nodes for active observers stay there until their next advert, which can be up to 168 h with the maximum flood.advert.interval. The author lists this as out of scope. That is fine, and the node page explains the state meanwhile. [A]
5	Info	cmd/server/routes.go prefix path	A short-URL prefix (8–63 hex) of a retired node still gets the bare 404, because the enrichment only matches full keys. The observer link always uses the full key, so the issue's path is covered. [T via curl]
6	Info	node page	On any 404, the node page fetches /api/nodes/<pk>?include=advertRoutes and /health twice. I measured the same request pattern for an unknown key, so this is existing behaviour and not caused by this PR. No new request was added. [T]

There are no P1 or P2 findings.

Verification points

1. Part A, ingestor (MoveStaleNodes)

• [A] One shared predicate, staleNodesWhere, is used for both the INSERT … inactive_nodes and the DELETE. So the copy and the delete cannot drift apart.

• [A] Case is normalised on both sides: lower(public_key) NOT IN (SELECT lower(id) …). id IS NOT NULL avoids the NOT IN/NULL trap.

• [A] last_seen is never written. The observer bound is observers.last_seen >= cutoff, and both values are RFC3339 UTC strings from the ingestor (UpsertObserverAt stamps the ingest time), so comparing them as strings is sound. Retained MQTT status messages do not advance observers.last_seen (UpsertObserverRetained), so a dead observer cannot keep its node alive through broker replays.

• [T] A quiet observer (8 days, nodeDays 7) is still retired: TestMoveStaleNodesRetiresQuietObserverAndPlainNode, plus my mutant M2.

• [T] Perf probe. I wrote a scratch test (not committed) that calls the real MoveStaleNodes on a migrated store.

  Nodes	Observers	Stale nodes	Active observers protecting a stale node	Moved	Time
  2,000	60	1,000	30	970 (expected 970)	13.6 ms
  20,000	600	10,000	300	9,700 (expected 9,700)	139 ms

  • The cost grows linearly with size.
  • EXPLAIN QUERY PLAN: SEARCH nodes USING INDEX idx_nodes_last_seen (last_seen<?), then LIST SUBQUERY 1 (built once, not correlated), then SEARCH observers USING INDEX idx_observers_last_seen.
  • The lookup is not O(n²). It runs at start-up and from the hourly ticker only.

2. Part B, server and UI

• [A] The server stays read-only. node_not_found.go adds only two SELECTs, and only on the node-detail 404 path. The Exec calls added under cmd/server are all in _test.go seed code.
  • [T] The server suite, including TestServerDBConnIsReadOnly, passes.
• [A] No per-item API calls. The explanation travels in the 404 body the page already receives.
  • [T] Browser network log: no new endpoints are requested.
• [A] Named structs nodeNotFoundResponse, inactiveNodeInfo and observerRef. The diff adds no interface{} at all (0 added lines).
• [A] XSS:
  • The inactive name, inactive role, observer name and pubkey all go through escapeHtml, which escapes & < > " '.
  • Dates are escaped too.
  • The observer id goes through encodeURIComponent inside a double-quoted href.
  • [T] My mutants M6 (role unescaped) and M7 (raw observer id in href) are both killed by the escaping test.
• [A] Visibility: identityHidden is applied to the lower-cased key and to both names found, so node blacklist, observer blacklist and hidden-name prefixes all give the bare 404.
  • [T] TestNodeDetail404HiddenIdentityStaysBare passes, with 4 subtests.
  • [T] My mutant M5 (observer name left out of the hidden check) is killed.

3. Acceptance criteria of #199

Criterion	Status	Test
An online observer with an advert older than nodeDays stays a node	Met	[T] TestMoveStaleNodesKeepsActiveObserverNode. It checks the row stays in nodes, is not copied to inactive_nodes, and keeps last_seen. [A] The map and node page read nodes, and the map has no default age filter (lastHeard is opt-in).
A node in neither nodes nor the active observer set is retired as before	Met	[T] TestMoveStaleNodesRetiresQuietObserverAndPlainNode and TestMoveStaleNodesNullObserverIDDoesNotBlockRetention. The existing TestMoveStaleNodes* tests still pass.
No bare "Node not found" from the Observers page	Met (option 1)	[T] test-issue-199-missing-node.js, the E2E test-issue-199-inactive-observer-e2e.js (3/3 locally), and the server 404 tests.
No new per-item calls; server read-only	Met	[T] Network log and the read-only test. [A] Code review.

6. Rules

• [T] Fork guards are unchanged: 9 github.repository == 'Kpa-clawbot/CoreScope' guards (8 in deploy.yml, 1 in release-fast-path.yml), identical in head, merge base and origin/master. The PR's own .github change only adds the seed-199 step and the E2E line.
• [T] No closing keywords. The PR body says "Relates to Active observers vanish as nodes after retention.nodeDays without an advert; observer link dead-ends on 'Node not found' #199", and neither commit message contains a closing keyword followed by an issue reference.
• [T] retention.nodeDays is not changed anywhere. Every added line that mentions it is a comment, a doc string or a test message, and no config file is in the diff.

Tests and mutants

All runs below are on the merged tree (head merged into current origin/master).

• [T] cd cmd/ingestor && go test ./...: ok (160 s).
• [T] cd cmd/server && go test ./...: ok (79 s).
• [T] go test -race:
  • ingestor -run MoveStaleNodes: 6/6 pass, no races;
  • server -run 'NodeDetail404|ReadOnly': all pass, no races.
• [T] sh test-all.sh: 212 passed, 0 failed (212 files). This includes test-issue-199-missing-node.js and master's new test-registry check, which confirms the E2E file is registered in deploy.yml.
• [T] CI on head (run 37180838265): Go Build & Test, Playwright E2E, and Docker build pass. Release, Staging and Badges are skipped, as expected for a PR.

My own mutants. The author's set is [K] and was not re-run.

#	Mutant	Result
M1	No lower() on either side of the NOT IN	Killed by KeepsActiveObserverNode
M2	Observer subquery without the last_seen >= ?1 bound	Killed by RetiresQuietObserverAndPlainNode
M3	lower() only on the node side	Killed by KeepsActiveObserverNode
M4	Server: path key not lower-cased	Killed by CarriesInactiveNodeAndObserver (upper-case path)
M5	Server: observer name left out of the identityHidden names	Killed by HiddenIdentityStaysBare/hidden_observer_name
M6	Frontend: role unescaped	Killed by the escaping test
M7	Frontend: raw observer id in href	Killed by the escaping test
M8	Frontend: Last advert row dropped	Survives (see finding 2)

Browser

• [T] Local Go server built from the merged tree, on a copy of e2e-fixture.db prepared like CI: freshen, corescope-migrate, then the seed-199 SQL. Chromium. The server was stopped by its pid, looked up from the port, and the port was confirmed free afterwards.
• [T] Observer detail. The observer with the inactive node shows "View node detail →" with href="#/nodes/0d3b3f38…" (lower-case).
• [T] Inactive node page. Clicking the link opens the node page:
  • title "Inactive Observer E2E";
  • "No advert heard since 2026-09-24 …; this device is inactive";
  • name, role, last advert, and last upload as observer;
  • "View observer →" leads back to the observer.
  • Checked in both light and dark theme. This closes the author's open [A] on the dark theme, and the card uses theme variables correctly.
• [T] Other node pages.
  • Observer-only key: "No node record", with the reason.
  • Unknown key: "Node not found — ffff00000000…".
• [T] API answers (curl).
  • Inactive key: 404 with inactive_node and observer.
  • Observer-only key: 404 with observer only.
  • Unknown key and 8-character prefix: bare {"error":"Not found"}.
• [T] The author's E2E passed 3/3 against this server.
• The only page errors (L is not defined, Chart is not defined) come from the sandbox blocking the unpkg CDN for Leaflet and Chart.js. They are unrelated to this PR.

Not verified

• No staging or production run. The fix has not been observed on real data.
• The map was not checked in the browser: Leaflet could not load in the sandbox. That a kept node stays on the map is [A], based on the nodes table and the absence of a default age filter.
• Mobile width (390 px): [K] from the author's report; not re-checked.
• The author's 14 mutants: [K]; I ran my own 8 instead.
• The full Playwright E2E suite and coverage were not run locally; I rely on CI for those.
• The behaviour of a soft-deleted observer (inactive = 1) whose last_seen is still inside nodeDays: [K] from the author's report, and [A] that it only matters when observerDays < nodeDays.

---

Generated by Claude Code