From c28a20b9a1a1329fb6720ce90f71fc6448d1dc17 Mon Sep 17 00:00:00 2001 From: efiten Date: Wed, 2 Sep 2026 09:11:05 +0200 Subject: [PATCH 01/26] fix(ingestor): a retained status message is not observer liveness (#1885) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Problem The broker replays every retained `status` message on subscribe, so each ingestor restart pushes all of them through the status path in `handleMessage`. That path stamps `last_seen` with `time.Now()` (deliberately, per #1465). Observed on a live deployment on 2026-08-11: **23 observers all carried `last_seen = 2026-08-06T08:20:09Z`** β€” 15 seconds after container start β€” and 18 of them had sent no actual packet in over a month. Their retained publish dates lined up almost 1:1 with `last_packet_at`, i.e. the replay was their only sign of "life": | observer | last real packet | retained status published | |---|---|---| | ON8AR - Observer | never | 2026-03-20 | | BE-BGS-RRY120-RES | never | 2026-04-02 | | A3BEF374 | 2026-04-30 | 2026-04-30 | | BE-BGS-RRY120-RUDY | 2026-05-18 | 2026-05-18 | | BE-JBE-ETG-O1 | 2026-06-10 | 2026-06-10 | That makes dead observers immortal, three ways per restart: 1. `last_seen` jumps forward, so `RemoveStaleObservers` can never age them out as long as a restart happens inside `observerDays`. 2. The unconditional `inactive = 0` reactivation at the end of `UpsertObserverAt` undoes any soft-delete that did land. 3. A metrics sample is filed at ingest time, dating a months-old reading as a present-tense measurement. `UpsertObserverAt`'s docstring already claimed retained replays were a no-op for `last_seen` thanks to the `MAX` guard. That held only while the caller passed the envelope timestamp; #1465 switched it to ingest time, which defeats the guard. ## Fix The retained path now updates metadata only, via a new `UpsertObserverRetained`: - no `last_seen` advance - no `inactive = 0` reactivation - no `packet_count` bump - no metrics sample - **no INSERT** β€” a retained-only observer the analyzer has never heard from live describes a past that may be months old and does not belong in the list. A live message from the same observer creates the row through the normal path moments later. Live status handling is unchanged. ## Tests Seven tests in `cmd/ingestor/retained_status_test.go`, written before the fix: - 4 that failed on the bug: `last_seen` advance, reactivation of a soft-deleted row, creation of a never-seen observer, metrics-sample insert - 2 regression guards pinning live (non-retained) behaviour: `last_seen` still advances, unknown observer still created - 1 asserting retained metadata is still applied β€” the snapshot is the observer's last known state, only the liveness signal is suppressed `mockMessage` gained a `retained` field so `Retained()` is controllable. Meta flattening is extracted to `observerMetaColumns` so both write paths bind identical args. Full `cmd/ingestor` suite passes. πŸ€– Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 (1M context) (cherry picked from commit 9bd5f5a3a2647f42091d8cb7a04ac98b7f018d17) Co-Authored-By: Claude Opus 5 --- cmd/ingestor/db.go | 79 ++++++++--- cmd/ingestor/main.go | 16 +++ cmd/ingestor/main_test.go | 7 +- cmd/ingestor/retained_status_test.go | 171 ++++++++++++++++++++++ cmd/ingestor/status_liveness.go | 50 +++++++ cmd/ingestor/status_liveness_test.go | 203 +++++++++++++++++++++++++++ 6 files changed, 507 insertions(+), 19 deletions(-) create mode 100644 cmd/ingestor/retained_status_test.go create mode 100644 cmd/ingestor/status_liveness.go create mode 100644 cmd/ingestor/status_liveness_test.go diff --git a/cmd/ingestor/db.go b/cmd/ingestor/db.go index e37094e00..a6604389d 100644 --- a/cmd/ingestor/db.go +++ b/cmd/ingestor/db.go @@ -1413,8 +1413,68 @@ func (s *Store) UpsertObserverAt(id, name, iata string, meta *ObserverMeta, last } normalizedIATA := strings.TrimSpace(strings.ToUpper(iata)) - var model, firmware, clientVersion, radio interface{} - var batteryMv, uptimeSecs, noiseFloor, canRelay interface{} + model, firmware, clientVersion, radio, batteryMv, uptimeSecs, noiseFloor, canRelay := observerMetaColumns(meta) + + _, err := s.stmtUpsertObserver.Exec( + id, name, normalizedIATA, lastSeen, lastSeen, model, firmware, clientVersion, radio, batteryMv, uptimeSecs, noiseFloor, canRelay, canRelay, + name, normalizedIATA, ingestNow, lastSeen, model, firmware, clientVersion, radio, batteryMv, uptimeSecs, noiseFloor, canRelay, canRelay, + ) + if err != nil { + s.Stats.WriteErrors.Add(1) + return err + } + s.Stats.ObserverUpserts.Add(1) + + // Reactivate if this observer was previously marked inactive + s.db.Exec(`UPDATE observers SET inactive = 0 WHERE id = ? AND inactive = 1`, id) + return nil +} + +// UpsertObserverRetained applies the metadata from a RETAINED status message +// without treating it as a sign of life. The broker replays retained messages +// on every subscribe, so an ingestor restart would otherwise stamp last_seen +// with the restart time for every observer that ever published one β€” making +// dead observers permanently un-ageable by RemoveStaleObservers, and undoing +// any inactive flag it did manage to set. +// +// Deliberately narrower than UpsertObserverAt: it updates metadata columns on +// an existing row only. It does not advance last_seen, does not clear +// inactive, does not bump packet_count, and does not INSERT β€” a retained-only +// observer the analyzer has never heard from live describes a past that may be +// months old and does not belong in the list. A live message from the same +// observer arrives moments later and creates the row through the normal path. +func (s *Store) UpsertObserverRetained(id, name, iata string, meta *ObserverMeta) error { + normalizedIATA := strings.TrimSpace(strings.ToUpper(iata)) + model, firmware, clientVersion, radio, batteryMv, uptimeSecs, noiseFloor, canRelay := observerMetaColumns(meta) + + _, err := s.db.Exec(` + UPDATE observers SET + name = COALESCE(?, name), + iata = COALESCE(?, iata), + model = COALESCE(?, model), + firmware = COALESCE(?, firmware), + client_version = COALESCE(?, client_version), + radio = COALESCE(?, radio), + battery_mv = COALESCE(?, battery_mv), + uptime_secs = COALESCE(?, uptime_secs), + noise_floor = COALESCE(?, noise_floor), + can_relay = COALESCE(?, can_relay), + can_relay_seen = CASE WHEN ? IS NULL THEN can_relay_seen ELSE 1 END + WHERE id = ?`, + name, normalizedIATA, model, firmware, clientVersion, radio, + batteryMv, uptimeSecs, noiseFloor, canRelay, canRelay, id, + ) + if err != nil { + s.Stats.WriteErrors.Add(1) + return err + } + return nil +} + +// observerMetaColumns flattens an *ObserverMeta into the driver args the +// observer upserts bind. A nil field stays nil so the COALESCE in the SQL +// leaves the existing column untouched (#1290). +func observerMetaColumns(meta *ObserverMeta) (model, firmware, clientVersion, radio, batteryMv, uptimeSecs, noiseFloor, canRelay interface{}) { if meta != nil { if meta.Model != nil { model = *meta.Model @@ -1449,20 +1509,7 @@ func (s *Store) UpsertObserverAt(id, name, iata string, meta *ObserverMeta, last } } } - - _, err := s.stmtUpsertObserver.Exec( - id, name, normalizedIATA, lastSeen, lastSeen, model, firmware, clientVersion, radio, batteryMv, uptimeSecs, noiseFloor, canRelay, canRelay, - name, normalizedIATA, ingestNow, lastSeen, model, firmware, clientVersion, radio, batteryMv, uptimeSecs, noiseFloor, canRelay, canRelay, - ) - if err != nil { - s.Stats.WriteErrors.Add(1) - return err - } - s.Stats.ObserverUpserts.Add(1) - - // Reactivate if this observer was previously marked inactive - s.db.Exec(`UPDATE observers SET inactive = 0 WHERE id = ? AND inactive = 1`, id) - return nil + return model, firmware, clientVersion, radio, batteryMv, uptimeSecs, noiseFloor, canRelay } // Close checkpoints the WAL and closes the database. diff --git a/cmd/ingestor/main.go b/cmd/ingestor/main.go index ac102da01..12f96355b 100644 --- a/cmd/ingestor/main.go +++ b/cmd/ingestor/main.go @@ -632,6 +632,22 @@ func handleMessage(store *Store, tag string, source MQTTSource, m mqtt.Message, name, _ := msg["origin"].(string) iata := parts[1] meta := extractObserverMeta(msg) + // A replayed status message is the broker handing us the observer's + // last published snapshot β€” it is not evidence the observer is alive + // now. Stamping last_seen from it resurrects dead observers, so the + // replay path only refreshes metadata. + // + // Two ways to recognise one: the retain flag (our own subscribe), and + // the payload itself (a replay that reached us through the mosquitto + // bridge, where the flag does not survive the hop β€” see + // statusIsLiveness). + if m.Retained() || !statusIsLiveness(msg, time.Now().UTC()) { + if err := store.UpsertObserverRetained(observerID, name, iata, meta); err != nil { + log.Printf("MQTT [%s] retained observer status error: %v", tag, err) + } + log.Print(formatStatusLog(tag, firstNonEmpty(name, observerID), iata)) + return + } // observer.last_seen is "when did the analyzer last hear from this // observer" β€” fundamentally an ingest-time question. Passing "" makes // UpsertObserverAt use time.Now(), independent of the envelope timestamp diff --git a/cmd/ingestor/main_test.go b/cmd/ingestor/main_test.go index c3a2e0095..817afe7b0 100644 --- a/cmd/ingestor/main_test.go +++ b/cmd/ingestor/main_test.go @@ -105,13 +105,14 @@ func TestUnixTime(t *testing.T) { // mockMessage implements mqtt.Message for testing handleMessage type mockMessage struct { - topic string - payload []byte + topic string + payload []byte + retained bool } func (m *mockMessage) Duplicate() bool { return false } func (m *mockMessage) Qos() byte { return 0 } -func (m *mockMessage) Retained() bool { return false } +func (m *mockMessage) Retained() bool { return m.retained } func (m *mockMessage) Topic() string { return m.topic } func (m *mockMessage) MessageID() uint16 { return 0 } func (m *mockMessage) Payload() []byte { return m.payload } diff --git a/cmd/ingestor/retained_status_test.go b/cmd/ingestor/retained_status_test.go new file mode 100644 index 000000000..64a7ffc47 --- /dev/null +++ b/cmd/ingestor/retained_status_test.go @@ -0,0 +1,171 @@ +package main + +import ( + "testing" + "time" +) + +// A retained MQTT status message is a snapshot the broker replays on every +// subscribe β€” it says what the observer last published, not that the observer +// is alive now. Treating the replay as liveness makes dead observers immortal: +// last_seen jumps to the ingestor's restart time, RemoveStaleObservers can +// never age them out, and any row it did mark gets reactivated on the next +// restart. Observed on live 2026-08-11: 23 observers, all stamped 15s after +// container start, 18 of them with no real packet in over a month. + +const retainedStatusTopic = "meshcore/LAX/obs-retained/status" + +func retainedStatusMsg(topic string, payload string) *mockMessage { + return &mockMessage{topic: topic, payload: []byte(payload), retained: true} +} + +// seedObserver inserts an observer and forces last_seen to ageDays ago. +func seedObserver(t *testing.T, s *Store, id string, ageDays int) string { + t.Helper() + if err := s.UpsertObserver(id, id, "LAX", nil); err != nil { + t.Fatal(err) + } + old := time.Now().UTC().AddDate(0, 0, -ageDays).Format(time.RFC3339) + if _, err := s.db.Exec(`UPDATE observers SET last_seen = ? WHERE id = ?`, old, id); err != nil { + t.Fatal(err) + } + return old +} + +func observerLastSeen(t *testing.T, s *Store, id string) string { + t.Helper() + var got string + if err := s.db.QueryRow(`SELECT last_seen FROM observers WHERE id = ?`, id).Scan(&got); err != nil { + t.Fatal(err) + } + return got +} + +func TestRetainedStatusDoesNotAdvanceLastSeen(t *testing.T) { + store := newTestStore(t) + want := seedObserver(t, store, "obs-retained", 60) + + handleMessage(store, "test", MQTTSource{Name: "test"}, + retainedStatusMsg(retainedStatusTopic, `{"origin":"obs-retained","noise_floor":-95.5}`), + nil, nil, &Config{}) + + if got := observerLastSeen(t, store, "obs-retained"); got != want { + t.Errorf("last_seen = %s, want %s (retained replay is not liveness)", got, want) + } +} + +func TestRetainedStatusDoesNotReactivateInactiveObserver(t *testing.T) { + store := newTestStore(t) + seedObserver(t, store, "obs-retained", 60) + if _, err := store.db.Exec(`UPDATE observers SET inactive = 1 WHERE id = ?`, "obs-retained"); err != nil { + t.Fatal(err) + } + + handleMessage(store, "test", MQTTSource{Name: "test"}, + retainedStatusMsg(retainedStatusTopic, `{"origin":"obs-retained","noise_floor":-95.5}`), + nil, nil, &Config{}) + + var inactive int + if err := store.db.QueryRow(`SELECT inactive FROM observers WHERE id = ?`, "obs-retained").Scan(&inactive); err != nil { + t.Fatal(err) + } + if inactive != 1 { + t.Errorf("inactive = %d, want 1 (retained replay must not resurrect a soft-deleted observer)", inactive) + } +} + +// A retained snapshot from an observer the analyzer has never heard from live +// describes a past that may be months old. Creating a row for it puts a +// permanently dead observer in the list. +func TestRetainedStatusDoesNotCreateUnknownObserver(t *testing.T) { + store := newTestStore(t) + + handleMessage(store, "test", MQTTSource{Name: "test"}, + retainedStatusMsg("meshcore/LAX/obs-never-seen/status", `{"origin":"Ghost","noise_floor":-95.5}`), + nil, nil, &Config{}) + + var count int + if err := store.db.QueryRow(`SELECT COUNT(*) FROM observers WHERE id = ?`, "obs-never-seen").Scan(&count); err != nil { + t.Fatal(err) + } + if count != 0 { + t.Errorf("observers row count = %d, want 0 (retained-only observer must not be created)", count) + } +} + +// The metrics sample is stamped with ingest time, so a replay would file a +// months-old reading as a present-tense measurement. +func TestRetainedStatusDoesNotInsertMetricsSample(t *testing.T) { + store := newTestStore(t) + seedObserver(t, store, "obs-retained", 60) + + handleMessage(store, "test", MQTTSource{Name: "test"}, + retainedStatusMsg(retainedStatusTopic, `{"origin":"obs-retained","noise_floor":-95.5,"battery_mv":3900}`), + nil, nil, &Config{}) + + var count int + if err := store.db.QueryRow( + `SELECT COUNT(*) FROM observer_metrics WHERE observer_id = ?`, "obs-retained", + ).Scan(&count); err != nil { + t.Fatal(err) + } + if count != 0 { + t.Errorf("observer_metrics rows = %d, want 0 (retained replay is not a new sample)", count) + } +} + +// The retained snapshot is still the observer's last known state, so its +// metadata is worth keeping β€” only the liveness signal is suppressed. +func TestRetainedStatusStillUpdatesMetadata(t *testing.T) { + store := newTestStore(t) + seedObserver(t, store, "obs-retained", 60) + + handleMessage(store, "test", MQTTSource{Name: "test"}, + retainedStatusMsg(retainedStatusTopic, `{"origin":"obs-retained","firmware":"v1.2.3","noise_floor":-95.5}`), + nil, nil, &Config{}) + + var firmware string + var noiseFloor float64 + if err := store.db.QueryRow( + `SELECT firmware, noise_floor FROM observers WHERE id = ?`, "obs-retained", + ).Scan(&firmware, &noiseFloor); err != nil { + t.Fatal(err) + } + if firmware != "v1.2.3" { + t.Errorf("firmware = %q, want %q", firmware, "v1.2.3") + } + if noiseFloor != -95.5 { + t.Errorf("noise_floor = %v, want -95.5", noiseFloor) + } +} + +// Regression guard for #1465: a live status message must still stamp last_seen +// with ingest time. Only the retained flag changes the behaviour. +func TestLiveStatusStillAdvancesLastSeen(t *testing.T) { + store := newTestStore(t) + before := seedObserver(t, store, "obs-live", 60) + + handleMessage(store, "test", MQTTSource{Name: "test"}, + &mockMessage{topic: "meshcore/LAX/obs-live/status", payload: []byte(`{"origin":"obs-live","noise_floor":-95.5}`)}, + nil, nil, &Config{}) + + if got := observerLastSeen(t, store, "obs-live"); got == before { + t.Errorf("last_seen = %s, want it advanced past %s (live status is liveness)", got, before) + } +} + +func TestLiveStatusStillCreatesUnknownObserver(t *testing.T) { + store := newTestStore(t) + + handleMessage(store, "test", MQTTSource{Name: "test"}, + &mockMessage{topic: "meshcore/LAX/obs-fresh/status", payload: []byte(`{"origin":"Fresh","noise_floor":-95.5}`)}, + nil, nil, &Config{}) + + var count int + if err := store.db.QueryRow(`SELECT COUNT(*) FROM observers WHERE id = ?`, "obs-fresh").Scan(&count); err != nil { + t.Fatal(err) + } + if count != 1 { + t.Errorf("observers row count = %d, want 1 (live status creates the observer)", count) + } +} diff --git a/cmd/ingestor/status_liveness.go b/cmd/ingestor/status_liveness.go new file mode 100644 index 000000000..9b9ef2893 --- /dev/null +++ b/cmd/ingestor/status_liveness.go @@ -0,0 +1,50 @@ +package main + +import ( + "strings" + "time" +) + +// statusLivenessMaxAge is how far a status payload's own timestamp may lag +// before the message stops counting as evidence the observer is alive now. +// +// Deliberately generous: observer clocks are naive (#1478) and can sit a whole +// timezone off, so anything under a day has to be accepted. The replays this +// guards against are months stale, so the margin costs nothing. +const statusLivenessMaxAge = 24 * time.Hour + +// statusIsLiveness reports whether a status message proves the observer was +// alive when we received it. +// +// The retain flag alone cannot answer that. Between EMQX and the ingestor sits +// a mosquitto bridge, and MQTT only sets RETAIN on messages delivered in +// response to a *new* subscription. The bridge runs cleansession, so every +// reconnect resubscribes and pulls EMQX's entire retained set β€” which it then +// republishes into its own broker as ordinary traffic. The ingestor is already +// subscribed there, so those months-old snapshots arrive with RETAIN cleared, +// indistinguishable from live status. On 2026-08-13 an EMQX restart replayed +// them and recreated 17 long-dead observers in two seconds. +// +// So judge the payload instead of the transport: +// - a status that is not "online" is the observer's own offline notice (its +// LWT, or a replay of it) and is never proof of life; +// - a timestamp older than statusLivenessMaxAge describes a past that may be +// months old. +// +// A payload with no timestamp, or one we cannot parse, is unjudgeable and +// keeps the previous behaviour β€” this guard removes false liveness, it does +// not invent new grounds to discard observers. +func statusIsLiveness(msg map[string]interface{}, now time.Time) bool { + if s, ok := msg["status"].(string); ok && s != "" && !strings.EqualFold(s, "online") { + return false + } + raw, _ := msg["timestamp"].(string) + if raw == "" { + return true + } + t, _, err := parseEnvelopeTime(raw) + if err != nil { + return true + } + return !t.Before(now.Add(-statusLivenessMaxAge)) +} diff --git a/cmd/ingestor/status_liveness_test.go b/cmd/ingestor/status_liveness_test.go new file mode 100644 index 000000000..6f96b07d1 --- /dev/null +++ b/cmd/ingestor/status_liveness_test.go @@ -0,0 +1,203 @@ +package main + +import ( + "fmt" + "testing" + "time" +) + +// The retain flag alone does not identify a replay. Our live topology puts a +// mosquitto bridge between EMQX and the ingestor, and MQTT only sets RETAIN on +// messages delivered in response to a *new* subscription. When the bridge +// reconnects (it runs cleansession, so every reconnect resubscribes), EMQX +// replays its whole retained set to the bridge, which republishes it into its +// own broker β€” and the ingestor, already subscribed, receives those months-old +// snapshots with RETAIN cleared, indistinguishable from live traffic. +// +// Observed on live 2026-08-13: EMQX restarted at 13:09 UTC and 17 long-dead +// observers were recreated inside two seconds, among them BE-BGS-RRY120-RES +// whose retained payload was published 2026-04-02 and says "offline". +// +// So judge the payload, not the transport: a status message is evidence of +// life only if it claims to be online and carries a timestamp from roughly now. + +func liveStatusMsg(topic, payload string) *mockMessage { + return &mockMessage{topic: topic, payload: []byte(payload), retained: false} +} + +func statusPayload(origin, status string, ts time.Time) string { + return fmt.Sprintf(`{"origin":%q,"status":%q,"timestamp":%q,"noise_floor":-95.5}`, + origin, status, ts.Format("2006-01-02T15:04:05.000000")) +} + +func TestStaleStatusPayloadDoesNotAdvanceLastSeen(t *testing.T) { + store := newTestStore(t) + want := seedObserver(t, store, "obs-zombie", 60) + + handleMessage(store, "test", MQTTSource{Name: "test"}, + liveStatusMsg("meshcore/LAX/obs-zombie/status", + statusPayload("obs-zombie", "online", time.Now().UTC().AddDate(0, -4, 0))), + nil, nil, &Config{}) + + if got := observerLastSeen(t, store, "obs-zombie"); got != want { + t.Errorf("last_seen = %s, want %s (a 4-month-old payload is a replay, not liveness)", got, want) + } +} + +// The exact live case: an observer purged from the DB must not come back when +// the bridge replays its months-old retained snapshot with RETAIN cleared. +func TestStaleStatusPayloadDoesNotCreateObserver(t *testing.T) { + store := newTestStore(t) + + handleMessage(store, "test", MQTTSource{Name: "test"}, + liveStatusMsg("meshcore/OST/obs-purged/status", + statusPayload("BE-BGS-RRY120-RES", "offline", time.Now().UTC().AddDate(0, -4, 0))), + nil, nil, &Config{}) + + var count int + if err := store.db.QueryRow(`SELECT COUNT(*) FROM observers WHERE id = ?`, "obs-purged").Scan(&count); err != nil { + t.Fatal(err) + } + if count != 0 { + t.Errorf("observers row count = %d, want 0 (replayed snapshot must not resurrect a purged observer)", count) + } +} + +// An offline notice is the observer's own death certificate β€” the broker +// replaying it, or the LWT firing, is never proof of life. +func TestOfflineStatusIsNotLiveness(t *testing.T) { + store := newTestStore(t) + want := seedObserver(t, store, "obs-offline", 60) + + handleMessage(store, "test", MQTTSource{Name: "test"}, + liveStatusMsg("meshcore/LAX/obs-offline/status", + statusPayload("obs-offline", "offline", time.Now().UTC())), + nil, nil, &Config{}) + + if got := observerLastSeen(t, store, "obs-offline"); got != want { + t.Errorf("last_seen = %s, want %s (an offline status is not liveness)", got, want) + } +} + +func TestStaleStatusPayloadDoesNotReactivateInactiveObserver(t *testing.T) { + store := newTestStore(t) + seedObserver(t, store, "obs-zombie", 60) + if _, err := store.db.Exec(`UPDATE observers SET inactive = 1 WHERE id = ?`, "obs-zombie"); err != nil { + t.Fatal(err) + } + + handleMessage(store, "test", MQTTSource{Name: "test"}, + liveStatusMsg("meshcore/LAX/obs-zombie/status", + statusPayload("obs-zombie", "online", time.Now().UTC().AddDate(0, -4, 0))), + nil, nil, &Config{}) + + var inactive int + if err := store.db.QueryRow(`SELECT inactive FROM observers WHERE id = ?`, "obs-zombie").Scan(&inactive); err != nil { + t.Fatal(err) + } + if inactive != 1 { + t.Errorf("inactive = %d, want 1 (a replay must not undo the soft-delete)", inactive) + } +} + +// ─── The guard must not swallow real observers ────────────────────────────── + +func TestFreshStatusStillCountsAsLiveness(t *testing.T) { + store := newTestStore(t) + stale := seedObserver(t, store, "obs-live", 60) + + handleMessage(store, "test", MQTTSource{Name: "test"}, + liveStatusMsg("meshcore/LAX/obs-live/status", + statusPayload("obs-live", "online", time.Now().UTC())), + nil, nil, &Config{}) + + if got := observerLastSeen(t, store, "obs-live"); got == stale { + t.Errorf("last_seen = %s, want it advanced (a fresh online status is liveness)", got) + } +} + +// Observer clocks are naive and can sit a whole timezone off (#1478). A UTC-12 +// observer must not be mistaken for a replay. +func TestNaiveClockSkewStillCountsAsLiveness(t *testing.T) { + store := newTestStore(t) + stale := seedObserver(t, store, "obs-skewed", 60) + + handleMessage(store, "test", MQTTSource{Name: "test"}, + liveStatusMsg("meshcore/LAX/obs-skewed/status", + statusPayload("obs-skewed", "online", time.Now().UTC().Add(-12*time.Hour))), + nil, nil, &Config{}) + + if got := observerLastSeen(t, store, "obs-skewed"); got == stale { + t.Errorf("last_seen = %s, want it advanced (12h naive skew is a timezone, not a replay)", got) + } +} + +func TestStatusWithoutTimestampStillCountsAsLiveness(t *testing.T) { + store := newTestStore(t) + stale := seedObserver(t, store, "obs-nots", 60) + + handleMessage(store, "test", MQTTSource{Name: "test"}, + liveStatusMsg("meshcore/LAX/obs-nots/status", `{"origin":"obs-nots","noise_floor":-95.5}`), + nil, nil, &Config{}) + + if got := observerLastSeen(t, store, "obs-nots"); got == stale { + t.Errorf("last_seen = %s, want it advanced (no timestamp to judge β€” keep the old behaviour)", got) + } +} + +func TestStatusWithUnparseableTimestampStillCountsAsLiveness(t *testing.T) { + store := newTestStore(t) + stale := seedObserver(t, store, "obs-badts", 60) + + handleMessage(store, "test", MQTTSource{Name: "test"}, + liveStatusMsg("meshcore/LAX/obs-badts/status", + `{"origin":"obs-badts","status":"online","timestamp":"not-a-time","noise_floor":-95.5}`), + nil, nil, &Config{}) + + if got := observerLastSeen(t, store, "obs-badts"); got == stale { + t.Errorf("last_seen = %s, want it advanced (unjudgeable timestamp β€” keep the old behaviour)", got) + } +} + +// A replayed snapshot is still the observer's last known state: metadata is +// kept, only the liveness signal is suppressed. +func TestStaleStatusPayloadStillUpdatesMetadata(t *testing.T) { + store := newTestStore(t) + seedObserver(t, store, "obs-zombie", 60) + + handleMessage(store, "test", MQTTSource{Name: "test"}, + liveStatusMsg("meshcore/LAX/obs-zombie/status", + fmt.Sprintf(`{"origin":"obs-zombie","status":"online","timestamp":%q,"firmware":"v1.14.1","noise_floor":-95.5}`, + time.Now().UTC().AddDate(0, -4, 0).Format("2006-01-02T15:04:05.000000"))), + nil, nil, &Config{}) + + var firmware string + if err := store.db.QueryRow(`SELECT firmware FROM observers WHERE id = ?`, "obs-zombie").Scan(&firmware); err != nil { + t.Fatal(err) + } + if firmware != "v1.14.1" { + t.Errorf("firmware = %q, want %q (metadata from a replay is still the last known state)", firmware, "v1.14.1") + } +} + +// No metrics sample either β€” a months-old reading filed at ingest time reads +// as a present-tense measurement. +func TestStaleStatusPayloadDoesNotInsertMetricsSample(t *testing.T) { + store := newTestStore(t) + seedObserver(t, store, "obs-zombie", 60) + + handleMessage(store, "test", MQTTSource{Name: "test"}, + liveStatusMsg("meshcore/LAX/obs-zombie/status", + statusPayload("obs-zombie", "online", time.Now().UTC().AddDate(0, -4, 0))), + nil, nil, &Config{}) + + var count int + if err := store.db.QueryRow( + `SELECT COUNT(*) FROM observer_metrics WHERE observer_id = ?`, "obs-zombie", + ).Scan(&count); err != nil { + t.Fatal(err) + } + if count != 0 { + t.Errorf("observer_metrics rows = %d, want 0 (a replayed reading is not a new sample)", count) + } +} From a3f983a20f1b64732ae4c43ac220b2289766e873 Mon Sep 17 00:00:00 2001 From: Saarlandpower Date: Wed, 2 Sep 2026 09:50:22 +0200 Subject: [PATCH 02/26] fix(packets): observer filter hides multi-observer rows in grouped mode (#1748) (#1821) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --------- Co-authored-by: Claude (cherry picked from commit 02c2338d649b11f27b5e2e008438fd25e22b9660) Co-Authored-By: Claude Opus 5 --- public/packets.js | 51 ++++++++++++++++++---- test-frontend-helpers.js | 91 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 134 insertions(+), 8 deletions(-) diff --git a/public/packets.js b/public/packets.js index fd157240a..ff8c80f5a 100644 --- a/public/packets.js +++ b/public/packets.js @@ -2775,6 +2775,47 @@ }); } + // applyObserverFilter decides which already-loaded packets remain visible + // under the current observer filter. Extracted into its own function + // (rather than left inline in renderTableRows) specifically so tests can + // exercise the real production logic instead of a hand-copied + // reimplementation β€” see #1748 PR review (kent-beck): a test that only + // checks a copy of this logic doesn't fail if this function regresses. + // + // #1748: In grouped mode, the server already filters transmissions + // correctly (buildTransmissionWhere emits an EXISTS subquery over ALL + // observations of the transmission, not just the displayed one β€” see + // cmd/server/db.go). Each row's `observer_id` here is only the + // *representative* observer chosen for display (longest observed path), + // which may legitimately differ from the observer that satisfied the + // filter. Re-filtering client-side against that single representative β€” + // with `_children` still unpopulated at this point (only fetched lazily + // on row-expand or observer-sort-change) β€” hid every multi-observer + // transmission whose representative happened not to be one of the + // selected observers. In practice this meant a transmission only stayed + // visible when the filtered observer was also the one with the longest + // path (which is why the report described it as "works only for + // whichever observer logged it first" in dense meshes, where + // longest-path and earliest-seen correlate). The server-side EXISTS + // filter is authoritative for grouped rows, so no client-side + // re-filtering is needed or correct here. + // + // Flat/expanded mode (groupByHash === false) has no such + // representative-vs-actual mismatch β€” buildPacketWhere filters each + // observation row by its own exact observer_id β€” but we keep the + // defensive re-filter for that path since it costs nothing and guards + // against any future flat-mode server change. + function applyObserverFilter(displayPackets, filters, groupByHash, hashOnly) { + if (hashOnly || !filters.observer) return displayPackets; + if (groupByHash) return displayPackets; + const obsIds = new Set(filters.observer.split(',')); + return displayPackets.filter(p => { + if (obsIds.has(p.observer_id)) return true; + if (p._children) return p._children.some(c => obsIds.has(String(c.observer_id))); + return false; + }); + } + async function renderTableRows() { const tbody = document.getElementById('pktBody'); if (!tbody) return; @@ -2817,14 +2858,7 @@ const types = filters.type.split(',').map(Number); displayPackets = displayPackets.filter(p => types.includes(p.payload_type)); } - if (!hashOnly && filters.observer) { - const obsIds = new Set(filters.observer.split(',')); - displayPackets = displayPackets.filter(p => { - if (obsIds.has(p.observer_id)) return true; - if (p._children) return p._children.some(c => obsIds.has(String(c.observer_id))); - return false; - }); - } + displayPackets = applyObserverFilter(displayPackets, filters, groupByHash, hashOnly); // Packet Filter Language const pfCount = document.getElementById('packetFilterCount'); @@ -4074,6 +4108,7 @@ buildFlatRowHtml, _calcVisibleRange, buildPacketsParams, + applyObserverFilter, renderTableRows, _setPackets: function(p) { packets = p; }, _setFilter: function(k, v) { filters[k] = v; }, diff --git a/test-frontend-helpers.js b/test-frontend-helpers.js index 5dbd47e57..57373501d 100644 --- a/test-frontend-helpers.js +++ b/test-frontend-helpers.js @@ -3608,6 +3608,97 @@ console.log('\n=== packets.js: savedTimeWindowMin defaults ==='); }); } +// ===== Packets page: observer filter in grouped mode (issue #1748) ===== +{ + console.log('\n--- Observer filter β€” grouped vs. flat mode (#1748) ---'); + + // Load the REAL applyObserverFilter from packets.js via sandbox, rather + // than testing a hand-copied reimplementation of its logic. Per #1748 PR + // review (kent-beck): a test that only checks a copy doesn't fail if the + // actual production function regresses. Mirrors the same loadInCtx + // pattern used below for _calcVisibleRange. + const obsFilterCtx = makeSandbox(); + obsFilterCtx.registerPage = (name, handlers) => {}; + obsFilterCtx.onWS = () => {}; + obsFilterCtx.offWS = () => {}; + obsFilterCtx.api = () => Promise.resolve({}); + obsFilterCtx.window.getParsedPath = () => []; + obsFilterCtx.window.getParsedDecoded = () => ({}); + loadInCtx(obsFilterCtx, 'public/packets.js'); + const applyObserverFilter = obsFilterCtx.window._packetsTestAPI.applyObserverFilter; + + // A transmission the server correctly returned for observer "B" (it was + // one of several observers of this hash) but whose displayed + // "representative" observer is "A" (longest path) β€” exactly the shape + // QueryGroupedPackets returns. `_children` is undefined, matching the + // state on initial load (only fetched lazily on expand/obs-sort-change). + const groupedRowRepresentativeNotFiltered = { + hash: 'hash1', observer_id: 'A', observer_count: 3, _children: undefined, + }; + // A single-observer transmission whose only observer is NOT in the filter β€” + // must still be excluded (it genuinely wasn't observed by the filtered + // observer; the server wouldn't have returned it in the first place, but + // the client function must not accidentally let everything through). + const groupedRowGenuinelyExcludedByServer = { + hash: 'hash2', observer_id: 'C', observer_count: 1, _children: undefined, + }; + // A flat/expanded-mode row: single observation, own observer_id. + const flatRowMatching = { hash: 'hash3', observer_id: 'B' }; + const flatRowNonMatching = { hash: 'hash4', observer_id: 'A' }; + // A flat/expanded-mode row with already-loaded children, only one of + // which matches β€” the pre-existing children-aware fallback path. + const flatRowWithMatchingChild = { + hash: 'hash5', observer_id: 'A', + _children: [{ observer_id: 'A' }, { observer_id: 'B' }], + }; + + test('grouped mode: keeps a multi-observer row whose representative is not the filtered observer (#1748 core bug)', () => { + const result = applyObserverFilter([groupedRowRepresentativeNotFiltered], { observer: 'B' }, true, false); + assert.strictEqual(result.length, 1, 'server already guaranteed observer B saw this transmission'); + }); + + test('grouped mode: does not need _children to keep a valid server-filtered row', () => { + // The defining regression: _children is undefined (not yet fetched), + // and observer_id (the representative) does not match β€” pre-fix this + // returned zero rows. + const result = applyObserverFilter([groupedRowRepresentativeNotFiltered], { observer: 'B' }, true, false); + assert.strictEqual(result.length, 1); + }); + + test('grouped mode: passes through rows unfiltered (server EXISTS filter is authoritative)', () => { + // The client no longer second-guesses the server in grouped mode at all β€” + // this row would never have been returned by the server if observer C + // weren't a match, so the client trusts it. + const result = applyObserverFilter([groupedRowGenuinelyExcludedByServer], { observer: 'B' }, true, false); + assert.strictEqual(result.length, 1, 'grouped mode does not re-filter; server already applied the correct filter'); + }); + + test('flat mode: keeps a row whose own observer_id matches', () => { + const result = applyObserverFilter([flatRowMatching], { observer: 'B' }, false, false); + assert.strictEqual(result.length, 1); + }); + + test('flat mode: excludes a row whose own observer_id does not match and has no children', () => { + const result = applyObserverFilter([flatRowNonMatching], { observer: 'B' }, false, false); + assert.strictEqual(result.length, 0); + }); + + test('flat mode: falls back to matching children when representative does not match', () => { + const result = applyObserverFilter([flatRowWithMatchingChild], { observer: 'B' }, false, false); + assert.strictEqual(result.length, 1, 'observer B is among the already-loaded children'); + }); + + test('hashOnly bypasses the observer filter entirely (pinned-hash view)', () => { + const result = applyObserverFilter([groupedRowGenuinelyExcludedByServer, flatRowNonMatching], { observer: 'B' }, true, true); + assert.strictEqual(result.length, 2, 'hashOnly must return every row unfiltered, matching renderTableRows()'); + }); + + test('no observer filter set: all rows pass through unchanged', () => { + const result = applyObserverFilter([groupedRowGenuinelyExcludedByServer, flatRowNonMatching], {}, false, false); + assert.strictEqual(result.length, 2); + }); +} + // ===== Packets page: virtual scroll infrastructure ===== { console.log('\nPackets page β€” virtual scroll:'); From 37be79a2f4ea1a6b5ba93520d7ba32ac753e108b Mon Sep 17 00:00:00 2001 From: SaarMesh-Bot Date: Wed, 2 Sep 2026 10:27:52 +0200 Subject: [PATCH 03/26] fix(#1858): replace emoji map icon with Phosphor sprite in rx-coverage (#1860) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## 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 -'

πŸ—ΊοΈ Mobile RX coverage

' + +'

Mobile RX coverage

' + ``` ## 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] '

πŸ—ΊοΈ Mobile RX coverage

' + ``` 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 (cherry picked from commit a3454e75080800d4ef545e21d2ff987b4563794c) Co-Authored-By: Claude Opus 5 --- public/rx-coverage.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/public/rx-coverage.js b/public/rx-coverage.js index b2e3824eb..a8f1b2c0b 100644 --- a/public/rx-coverage.js +++ b/public/rx-coverage.js @@ -26,7 +26,7 @@ function pageHtml() { return '
' + - '

πŸ—ΊοΈ Mobile RX coverage

' + + '

Mobile RX coverage

' + '
Where roaming CoreScope-RX clients heard nodes. Colour = best signal per cell. Get the companion app β†’
' + '
' + dayBtn(1) + dayBtn(7) + dayBtn(14) + dayBtn(30) + '
' + '
strongmediumweakno signal
' + From 43863a0902f32f4a6ecd5096d8190c1646166788 Mon Sep 17 00:00:00 2001 From: Jonathan Herlin Date: Wed, 2 Sep 2026 10:28:00 +0200 Subject: [PATCH 04/26] 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. (cherry picked from commit 647841c99033c3574897f288b144f5e82721b4c4) Co-Authored-By: Claude Opus 5 --- cmd/ingestor/main.go | 60 ++++++- .../mqtt_force_reconnect_race_test.go | 150 ++++++++++++++++++ 2 files changed, 202 insertions(+), 8 deletions(-) create mode 100644 cmd/ingestor/mqtt_force_reconnect_race_test.go diff --git a/cmd/ingestor/main.go b/cmd/ingestor/main.go index 12f96355b..c00b60544 100644 --- a/cmd/ingestor/main.go +++ b/cmd/ingestor/main.go @@ -201,14 +201,10 @@ func main() { // half-open TCP socket and re-dial when paho.IsConnected==true // but no messages have flowed past the stall threshold. Throttled // per source by the watchdog itself (forceReconnectThrottle). - // Disconnect(250) gives in-flight publishes 250ms to drain; - // Connect() returns immediately and paho's reconnect machinery - // takes over from there. Captured-by-value `client` is the same - // pointer used everywhere else for this source. - liveness.ForceReconnectFn = func() { - client.Disconnect(250) - client.Connect() - } + // Captured-by-value `client` is the same pointer used everywhere + // else for this source. See buildForceReconnectFn for why this is + // NOT simply "Disconnect(250) then Connect()". + liveness.ForceReconnectFn = buildForceReconnectFn(client, tag) // PR #1216 r2 item 3: tag collisions used to log.Fatalf, which // killed the entire ingestor over one config typo and recreated // the #1212 total-ingest-stop class this PR exists to prevent. @@ -554,6 +550,54 @@ func buildMQTTOpts(source MQTTSource) *mqtt.ClientOptions { return opts } +// buildForceReconnectFn builds the watchdog's forced-reconnect action for a +// source (#1335, hardened against a race found while investigating a 100+ +// minute reconnect failure). +// +// paho's own client.IsConnected() β€” used as liveness.IsConnectedFn β€” reports +// true not only when genuinely connected but for the ENTIRE time paho's +// background AutoReconnect/ConnectRetry loop is retrying (status +// reconnecting/connecting). So the watchdog's LivenessStalled classification +// (IsConnected==true, no messages) fires just as often for "paho is actively, +// correctly retrying a still-down broker" as it does for the true #1335 +// half-open-TCP case. Naively doing Disconnect(250) then Connect() in the +// first case is actively harmful: paho's Disconnecting() must block until the +// CURRENT in-flight connection attempt plus its backoff sleep unwind (up to +// ConnectTimeout+MaxReconnectInterval, tens of seconds) before status +// actually reaches `disconnected`. Disconnect(250) returns to the caller +// after the 250ms quiesce regardless, so the following Connect() usually runs +// while status is still the transitional `disconnecting` state β€” paho then +// returns an error token (silently discarded by the old code) AND, because +// Disconnect() was called at all, tears down paho's own retry loop for good +// ("user requested no auto reconnection"). The client is left with nothing +// retrying until the watchdog's next trigger fires, which can repeat the same +// race β€” compounding into very long outages. +// +// client.IsConnectionOpen() (unlike IsConnected()) is strictly status == +// connected β€” never true while paho is reconnecting/connecting β€” so it +// reliably distinguishes "genuinely connected, maybe half-open" (safe to +// Disconnect then Connect; Disconnecting() does not need to wait on any +// in-flight retry loop from status connected, so it completes well within +// the 250ms quiesce) from "paho is already retrying on its own" (must NOT +// call Disconnect; Connect() alone is a safe no-op per paho when a retry is +// already under way, and properly starts a fresh attempt on the rare +// occasion status has actually settled to disconnected). +func buildForceReconnectFn(client mqtt.Client, tag string) func() { + return func() { + if client.IsConnectionOpen() { + client.Disconnect(250) + } + // Connect() resolves synchronously (Error() readable immediately, + // no Wait() needed) for both error returns and the "already + // retrying, treated as a safe no-op" success case β€” only a genuine + // fresh connection attempt leaves the token pending in the + // background, and we must not block this call on that. + if token := client.Connect(); token.Error() != nil { + log.Printf("MQTT [%s] WATCHDOG force-reconnect Connect() failed: %v", tag, token.Error()) + } + } +} + func handleMessage(store *Store, tag string, source MQTTSource, m mqtt.Message, channelKeys map[string]string, regionKeys map[string][]byte, cfg *Config) { // Liveness watchdog (#1212): record receipt before any processing so a // slow handler still counts as "source is alive". Cheap atomic store. diff --git a/cmd/ingestor/mqtt_force_reconnect_race_test.go b/cmd/ingestor/mqtt_force_reconnect_race_test.go new file mode 100644 index 000000000..8697cd3ea --- /dev/null +++ b/cmd/ingestor/mqtt_force_reconnect_race_test.go @@ -0,0 +1,150 @@ +package main + +import ( + "bytes" + "errors" + "log" + "strings" + "testing" + "time" + + mqtt "github.com/eclipse/paho.mqtt.golang" +) + +// fakeToken is a minimal mqtt.Token whose Error() is resolvable immediately +// without calling Wait(), matching how paho's real ConnectToken behaves on +// both of its synchronous return paths (the "already retrying, treated as a +// safe no-op" success case and any error case) β€” see buildForceReconnectFn's +// doc comment for why the caller must not need to Wait() before reading +// Error() (doing so would block indefinitely on a genuine, still in-flight +// reconnect attempt). +type fakeToken struct{ err error } + +func (t *fakeToken) Wait() bool { return true } +func (t *fakeToken) WaitTimeout(time.Duration) bool { return true } +func (t *fakeToken) Done() <-chan struct{} { ch := make(chan struct{}); close(ch); return ch } +func (t *fakeToken) Error() error { return t.err } + +// fakeClient implements mqtt.Client, recording calls to the three methods +// buildForceReconnectFn actually uses (IsConnectionOpen, Disconnect, +// Connect). Every other method panics β€” buildForceReconnectFn must never +// touch subscriptions, publishes, or options, so a call there indicates the +// fix drifted from its intended scope. +type fakeClient struct { + isConnectionOpen bool + connectErr error + + disconnectCalled bool + connectCalled bool + callOrder []string +} + +func (c *fakeClient) IsConnected() bool { panic("not used by buildForceReconnectFn") } +func (c *fakeClient) IsConnectionOpen() bool { + c.callOrder = append(c.callOrder, "IsConnectionOpen") + return c.isConnectionOpen +} +func (c *fakeClient) Connect() mqtt.Token { + c.connectCalled = true + c.callOrder = append(c.callOrder, "Connect") + return &fakeToken{err: c.connectErr} +} +func (c *fakeClient) Disconnect(quiesce uint) { + c.disconnectCalled = true + c.callOrder = append(c.callOrder, "Disconnect") +} +func (c *fakeClient) Publish(topic string, qos byte, retained bool, payload any) mqtt.Token { + panic("not used by buildForceReconnectFn") +} +func (c *fakeClient) Subscribe(topic string, qos byte, callback mqtt.MessageHandler) mqtt.Token { + panic("not used by buildForceReconnectFn") +} +func (c *fakeClient) SubscribeMultiple(filters map[string]byte, callback mqtt.MessageHandler) mqtt.Token { + panic("not used by buildForceReconnectFn") +} +func (c *fakeClient) Unsubscribe(topics ...string) mqtt.Token { + panic("not used by buildForceReconnectFn") +} +func (c *fakeClient) AddRoute(topic string, callback mqtt.MessageHandler) { + panic("not used by buildForceReconnectFn") +} +func (c *fakeClient) OptionsReader() mqtt.ClientOptionsReader { + panic("not used by buildForceReconnectFn") +} + +// Case 1 (#1335, the genuine half-open-TCP case): paho reports the +// connection as strictly open. This is the one case where Disconnect() is +// safe β€” status==connected means Disconnecting() does not need to wait on +// any in-flight retry loop, so it completes well within the quiesce window. +// buildForceReconnectFn must still Disconnect before Connect here. +func TestBuildForceReconnectFn_DisconnectsWhenConnectionOpen(t *testing.T) { + c := &fakeClient{isConnectionOpen: true} + fn := buildForceReconnectFn(c, "half-open-source") + fn() + + if !c.disconnectCalled { + t.Error("expected Disconnect to be called when IsConnectionOpen()==true (half-open TCP case, #1335)") + } + if !c.connectCalled { + t.Error("expected Connect to be called") + } + want := []string{"IsConnectionOpen", "Disconnect", "Connect"} + if len(c.callOrder) != len(want) { + t.Fatalf("expected call order %v, got %v", want, c.callOrder) + } + for i, name := range want { + if c.callOrder[i] != name { + t.Errorf("expected call order %v, got %v", want, c.callOrder) + break + } + } +} + +// Case 2 (the race this fix closes): paho is already mid-retry +// (reconnecting, or ConnectRetry's connecting), so IsConnectionOpen() is +// false even though the looser IsConnected() (used elsewhere for liveness +// classification) would report true. Calling Disconnect() here is what +// caused the original bug: it races paho's internal status transition and +// can permanently kill an in-flight retry loop via a botched, silently +// swallowed reconnect. buildForceReconnectFn must skip Disconnect and just +// call Connect(), which paho treats as a safe no-op if a retry is already +// under way. +func TestBuildForceReconnectFn_SkipsDisconnectWhenAlreadyRetrying(t *testing.T) { + c := &fakeClient{isConnectionOpen: false} + fn := buildForceReconnectFn(c, "mid-retry-source") + fn() + + if c.disconnectCalled { + t.Error("Disconnect must NOT be called while paho is already retrying (IsConnectionOpen()==false) β€” this races paho's status machine and can kill the in-flight retry loop") + } + if !c.connectCalled { + t.Error("expected Connect to still be called (safe no-op per paho when already retrying, or a real reconnect if paho had actually settled to disconnected)") + } +} + +// Case 3: Connect() returns an error token (e.g. the errStatusMustBeDisconnected +// class this whole bug hinged on β€” Connect() called while paho's status is +// transitionally "disconnecting"). The old code discarded the returned token +// entirely; the fix must surface it via log output instead of silently +// dropping it. +func TestBuildForceReconnectFn_LogsConnectError(t *testing.T) { + c := &fakeClient{isConnectionOpen: false, connectErr: errors.New("status can only transition to connecting from disconnected")} + fn := buildForceReconnectFn(c, "erroring-source") + + var buf bytes.Buffer + origOut := log.Writer() + origFlags := log.Flags() + log.SetOutput(&buf) + log.SetFlags(0) + defer func() { + log.SetOutput(origOut) + log.SetFlags(origFlags) + }() + + fn() + + logged := buf.String() + if !strings.Contains(logged, "erroring-source") || !strings.Contains(logged, "status can only transition") { + t.Errorf("expected Connect() error to be logged with the source tag, got: %q", logged) + } +} From 4be0c596727591732895d5715b60ff25f4720db2 Mon Sep 17 00:00:00 2001 From: efiten Date: Wed, 2 Sep 2026 10:28:04 +0200 Subject: [PATCH 05/26] fix(#1890): drop the hardcoded og:url so shared links stay on the instance (#1893) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes #1890. ## The problem `public/index.html:16` shipped this to every deployment: ```html ``` 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) (cherry picked from commit c5a71b34ec948f69c17956c4deebffe7e8a5051e) Co-Authored-By: Claude Opus 5 --- public/index.html | 5 +++- test-all.sh | 1 + test-issue-1890-og-url.js | 61 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 66 insertions(+), 1 deletion(-) create mode 100644 test-issue-1890-og-url.js diff --git a/public/index.html b/public/index.html index 73a0a9b6a..3e6e91dcb 100644 --- a/public/index.html +++ b/public/index.html @@ -13,7 +13,10 @@ - + diff --git a/test-all.sh b/test-all.sh index b831175e7..ca4c3847d 100755 --- a/test-all.sh +++ b/test-all.sh @@ -50,6 +50,7 @@ node test-issue-1648-m2-emoji-scan.js node test-issue-1648-m3-emoji-scan.js node test-issue-1648-m6-final-sweep.js node test-issue-1648-m6-lint-self.js +node test-issue-1890-og-url.js node test-traces.js node test-live-multibyte-filter.js diff --git a/test-issue-1890-og-url.js b/test-issue-1890-og-url.js new file mode 100644 index 000000000..8db009a2e --- /dev/null +++ b/test-issue-1890-og-url.js @@ -0,0 +1,61 @@ +#!/usr/bin/env node +/* Issue #1890 β€” index.html must not hardcode one instance's URL in its + * Open Graph tags. + * + * `` shipped to + * every self-hosted CoreScope. Facebook, Messenger and other OG consumers treat + * og:url as the canonical destination, so clicking a shared preview from ANY + * instance navigated to that one host instead of the instance the link came + * from. With no og:url present, consumers fall back to the URL they crawled, + * which is correct for every deployment without any configuration. + * + * og:image is deliberately NOT covered: it points at the project's own asset on + * raw.githubusercontent.com, which is a shared project resource, not a + * redirect target. + */ +'use strict'; + +const fs = require('fs'); +const path = require('path'); +const assert = require('assert'); + +const INDEX = path.resolve(__dirname, 'public', 'index.html'); +const html = fs.readFileSync(INDEX, 'utf8'); + +let passed = 0, failed = 0; +function test(name, fn) { + try { + fn(); + passed++; + console.log(` βœ… ${name}`); + } catch (e) { + failed++; + console.log(` ❌ ${name}: ${e.message}`); + } +} + +console.log('\n=== #1890: index.html carries no instance-specific canonical URL ==='); + +test('no og:url meta tag pinning a single instance', () => { + const m = html.match(/]*property=["']og:url["'][^>]*>/i); + assert.ok(!m, `og:url must not be hardcoded; found: ${m && m[0]}`); +}); + +test('no rel="canonical" pinning a single instance', () => { + const m = html.match(/]*rel=["']canonical["'][^>]*>/i); + assert.ok(!m, `canonical link must not be hardcoded; found: ${m && m[0]}`); +}); + +test('the analyzer.00id.net host appears nowhere in index.html', () => { + assert.ok(!/00id\.net/i.test(html), 'index.html still references 00id.net'); +}); + +test('og:title and og:description are still present', () => { + // The fix removes one tag, not the embed. Guard against over-deletion. + assert.ok(/property=["']og:title["']/i.test(html), 'og:title missing'); + assert.ok(/property=["']og:description["']/i.test(html), 'og:description missing'); + assert.ok(/property=["']og:image["']/i.test(html), 'og:image missing'); +}); + +console.log(`\n #1890 og:url: ${passed} passed, ${failed} failed\n`); +process.exit(failed ? 1 : 0); From 34173fb332fd26cc4863b79d2af2c3ec2a651d54 Mon Sep 17 00:00:00 2001 From: h4badger <204420093+h4badger@users.noreply.github.com> Date: Wed, 2 Sep 2026 02:07:13 -0700 Subject: [PATCH 06/26] 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. (cherry picked from commit 34b41fd5b6df5b685e69482d9b74ff00528641db) Co-Authored-By: Claude Opus 5 --- config.example.json | 8 ++++++++ public/map-tile-providers.js | 7 +++++++ 2 files changed, 15 insertions(+) diff --git a/config.example.json b/config.example.json index 327e32d91..e169a6aa5 100644 --- a/config.example.json +++ b/config.example.json @@ -89,10 +89,18 @@ "provider": "", "token": "" }, + "_comment_opentopomap": "OpenTopoMap topographical tile provider. Limited to zoom level 17.", + "opentopomap": { + "enabled": false + }, "_comment_stamen": "Stamen (hosted by Stadia). WARNING: Tokens are sent to the browser. Apply origin/referrer restrictions.", "stamen": { "enabled": false, "token": "" + }, + "_comment_usgs": "USGS topographical tile provider. Limited to zoom level 16.", + "usgs": { + "enabled": false } } } diff --git a/public/map-tile-providers.js b/public/map-tile-providers.js index 6266b81f9..fca6d1d0a 100644 --- a/public/map-tile-providers.js +++ b/public/map-tile-providers.js @@ -49,8 +49,11 @@ 'positron-dark': { provider: 'carto', label: 'Carto Positron', url: function() { return _getCartoBase() + '/light_all/{z}/{x}/{y}{r}.png'; }, invertFilter: INVERT_CSS, type: 'dark', attribution: 'Β© OpenStreetMap Β© CartoDB', maxZoom: 19 }, 'osm-standard': { provider: 'osm', label: 'OSM Standard', url: _getOsmUrl, invertFilter: null, type: 'light', attribution: 'Β© OpenStreetMap contributors, Maps Β© Mapbox/Thunderforest/MapTiler', maxZoom: 18 }, 'osm-dark': { provider: 'osm', label: 'OSM Standard', url: _getOsmUrl, invertFilter: INVERT_CSS, type: 'dark', attribution: 'Β© OpenStreetMap contributors, Maps Β© Mapbox/Thunderforest/MapTiler', maxZoom: 18 }, + 'opentopomap': { provider: 'opentopomap', label: 'OpenTopoMap', url: function() { return 'https://{s}.tile.opentopomap.org/{z}/{x}/{y}.png'; }, invertFilter: null, type: 'light', attribution: 'Β© OpenStreetMap contributors, SRTM, Β© OpenTopoMap (CC-BY-SA)', maxZoom: 17 }, 'stamen-toner-lite': { provider: 'stamen', label: 'Stamen Toner Lite', url: _getStamenUrl, invertFilter: null, type: 'light', attribution: 'Β© Stadia Maps Β© Stamen Design Β© OpenStreetMap', maxZoom: 20 }, 'stamen-toner-dark': { provider: 'stamen', label: 'Stamen Toner Lite', url: _getStamenUrl, invertFilter: INVERT_CSS, type: 'dark', attribution: 'Β© Stadia Maps Β© Stamen Design Β© OpenStreetMap', maxZoom: 20 }, + 'usgs-topo': { provider: 'usgs', label: 'USGS Topographic', url: function() { return 'https://basemap.nationalmap.gov/arcgis/rest/services/USGSTopo/MapServer/tile/{z}/{y}/{x}'; }, invertFilter: null, type: 'light', attribution: 'U.S. Geological Survey, National Geospatial Program', maxZoom: 16 }, + 'usgs-imagery': { provider: 'usgs', label: 'USGS Imagery', url: function() { return 'https://basemap.nationalmap.gov/arcgis/rest/services/USGSImageryTopo/MapServer/tile/{z}/{y}/{x}'; }, invertFilter: null, type: 'light', attribution: 'U.S. Geological Survey, National Geospatial Program', maxZoom: 16 }, 'esri-darkgray-labels': { provider: 'esri', label: 'Esri Dark Gray Canvas', url: function() { return 'https://server.arcgisonline.com/ArcGIS/rest/services/Canvas/World_Dark_Gray_Base/MapServer/tile/{z}/{y}/{x}'; }, invertFilter: null, type: 'dark', attribution: 'Tiles Β© Esri', maxZoom: 19 } }; @@ -65,6 +68,8 @@ var HAS_CARTO = !_cfg || !_cfg.providers || !_cfg.providers.carto || _cfg.providers.carto.enabled !== false; var HAS_OSM = _cfg && _cfg.providers && _cfg.providers.osm && _cfg.providers.osm.enabled; + var HAS_OPENTOPOMAP = _cfg && _cfg.providers && _cfg.providers.opentopomap && _cfg.providers.opentopomap.enabled; + var HAS_USGS = _cfg && _cfg.providers && _cfg.providers.usgs && _cfg.providers.usgs.enabled; var HAS_STAMEN = _cfg && _cfg.providers && _cfg.providers.stamen && _cfg.providers.stamen.enabled && !!_cfg.providers.stamen.token; var HAS_ESRI = true; // Kept for backwards compatibility @@ -73,7 +78,9 @@ var style = BASE_STYLES[key]; if (style.provider === 'carto' && HAS_CARTO) REGISTRY[key] = style; if (style.provider === 'osm' && HAS_OSM) REGISTRY[key] = style; + if (style.provider === 'opentopomap' && HAS_OPENTOPOMAP) REGISTRY[key] = style; if (style.provider === 'stamen' && HAS_STAMEN) REGISTRY[key] = style; + if (style.provider === 'usgs' && HAS_USGS) REGISTRY[key] = style; if (style.provider === 'esri' && HAS_ESRI) REGISTRY[key] = style; } From 67b7033bb14907adaddc186958011dc2a01da26c Mon Sep 17 00:00:00 2001 From: TeTeHacko Date: Wed, 2 Sep 2026 14:21:42 +0200 Subject: [PATCH 07/26] fix(hash-size): key the zero-hop advert skip on the path byte, not the route type (#1913) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## 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) (cherry picked from commit 97b6090344a09ca22d806279d5f515ff2109c510) Co-Authored-By: Claude Opus 5 --- cmd/server/coverage_test.go | 15 ++- cmd/server/store.go | 27 ++-- cmd/server/zerohop_hashsize_test.go | 187 ++++++++++++++++++++++++++++ 3 files changed, 214 insertions(+), 15 deletions(-) create mode 100644 cmd/server/zerohop_hashsize_test.go diff --git a/cmd/server/coverage_test.go b/cmd/server/coverage_test.go index d4bba14df..4be9a2845 100644 --- a/cmd/server/coverage_test.go +++ b/cmd/server/coverage_test.go @@ -3341,17 +3341,20 @@ func TestHashSizeTransportDirectZeroHopSkipped(t *testing.T) { db.conn.Exec(`INSERT INTO observers (id, name, iata, last_seen, first_seen, packet_count) VALUES ('obs1', 'Obs', 'SJC', ?, '2026-01-01T00:00:00Z', 10)`, recent) - // RouteDirect (2) zero-hop: path byte 0x40 β†’ hop_count=0, hash_size bits=01 - // Should be skipped (existing behavior) + // RouteDirect (2) zero-hop with an all-zero path byte: the sender wiped the + // size bits, so there is nothing to read. Should be skipped. + // (This fixture used to be 0x40. A zero hop count with the size bits SET is + // a deliberate declaration β€” see TestZeroHopDirectAdvertDeclaredSizeIsEvidence + // β€” so the skip is keyed on the byte's content, not on the route type.) db.conn.Exec(`INSERT INTO transmissions (raw_hex, hash, first_seen, route_type, payload_type, decoded_json) - VALUES ('1240aabbccdd', 'direct_zh', ?, 2, 4, '{"pubKey":"bbbb000000000001","name":"Direct-ZH","type":"ADVERT"}')`, recent) + VALUES ('1200aabbccdd', 'direct_zh', ?, 2, 4, '{"pubKey":"bbbb000000000001","name":"Direct-ZH","type":"ADVERT"}')`, recent) db.conn.Exec(`INSERT INTO observations (transmission_id, observer_idx, snr, rssi, path_json, timestamp) VALUES (1, 1, 10.0, -90, '[]', ?)`, recentEpoch) - // RouteTransportDirect (3) zero-hop: 4 transport bytes + path byte 0x40 β†’ hop_count=0 - // Should ALSO be skipped (this was the missing case) + // RouteTransportDirect (3) zero-hop: 4 transport bytes + all-zero path byte + // Should ALSO be skipped (this was the missing case in #747) db.conn.Exec(`INSERT INTO transmissions (raw_hex, hash, first_seen, route_type, payload_type, decoded_json) - VALUES ('130102030440aabb', 'tdirect_zh', ?, 3, 4, '{"pubKey":"bbbb000000000002","name":"TDirect-ZH","type":"ADVERT"}')`, recent) + VALUES ('130102030400aabb', 'tdirect_zh', ?, 3, 4, '{"pubKey":"bbbb000000000002","name":"TDirect-ZH","type":"ADVERT"}')`, recent) db.conn.Exec(`INSERT INTO observations (transmission_id, observer_idx, snr, rssi, path_json, timestamp) VALUES (2, 1, 10.0, -90, '[]', ?)`, recentEpoch) diff --git a/cmd/server/store.go b/cmd/server/store.go index 7a4a6ba86..7e9ecf604 100644 --- a/cmd/server/store.go +++ b/cmd/server/store.go @@ -8196,14 +8196,17 @@ func (s *PacketStore) computeAnalyticsHashSizes(region, area string) map[string] name = pk } } - // Skip zero-hop direct adverts for hash_size β€” the - // path byte is locally generated and unreliable. - // Still count the packet and update lastSeen. - isZeroHop := (routeType == uint64(RouteDirect) || routeType == uint64(RouteTransportDirect)) && (actualPathByte&0x3F) == 0 + // Skip zero-hop direct adverts whose path byte is entirely zero β€” + // there the size bits were wiped by the sender and say nothing. + // A non-zero byte with a zero hop count (0x40 / 0x80) is a + // deliberate size declaration; keep it. Same rule as + // computeNodeHashSizeInfo. Skipped packets still count and still + // update lastSeen. + isUndeclaredZeroHop := (routeType == uint64(RouteDirect) || routeType == uint64(RouteTransportDirect)) && actualPathByte == 0x00 if byNode[pk] == nil { role := nodeRoleByPK[pk] // empty if unknown initHS := hashSize - if isZeroHop { + if isUndeclaredZeroHop { initHS = 0 } byNode[pk] = map[string]interface{}{ @@ -8213,7 +8216,7 @@ func (s *PacketStore) computeAnalyticsHashSizes(region, area string) map[string] } } byNode[pk]["packets"] = byNode[pk]["packets"].(int) + 1 - if !isZeroHop { + if !isUndeclaredZeroHop { byNode[pk]["hashSize"] = hashSize } byNode[pk]["lastSeen"] = tx.FirstSeen @@ -8811,9 +8814,15 @@ func (s *PacketStore) computeNodeHashSizeInfo() map[string]*hashSizeNodeInfo { if err != nil { continue } - // Direct zero-hop adverts (route types 2 and 3) use path byte 0x00 - // locally and can misreport multibyte hash mode as 1-byte. - if (routeType == RouteDirect || routeType == RouteTransportDirect) && (pathByte&0x3F) == 0 { + // Direct zero-hop adverts carry no path, so the hop count is 0. Whether + // the SIZE bits are meaningful depends on the sender: firmware that + // predates meshcore-dev/MeshCore#3293 does `path_len = 0`, wiping the + // whole byte including the two size bits, so 0x00 says nothing about + // the node's path.hash.mode. A sender that writes the size through + // setPathHashSizeAndCount() emits 0x40 / 0x80 with a zero hop count β€” + // that is a deliberate declaration and the only way those bits can be + // non-zero here. Skip on the byte's content, not on the route type. + if (routeType == RouteDirect || routeType == RouteTransportDirect) && pathByte == 0x00 { continue } hs := int((pathByte>>6)&0x3) + 1 diff --git a/cmd/server/zerohop_hashsize_test.go b/cmd/server/zerohop_hashsize_test.go new file mode 100644 index 000000000..6509989e6 --- /dev/null +++ b/cmd/server/zerohop_hashsize_test.go @@ -0,0 +1,187 @@ +package main + +import ( + "fmt" + "testing" + "time" +) + +// Zero-hop direct adverts carry no path, so the hop count is always 0. Whether +// the two SIZE bits next to it mean anything depends on the sender: +// +// - Firmware predating meshcore-dev/MeshCore#3293 does `path_len = 0`, which +// wipes the whole byte. 0x00 therefore says nothing about path.hash.mode +// and must not be read as "1 byte". +// - A sender that writes the size through setPathHashSizeAndCount() emits +// 0x40 (2 bytes) or 0x80 (3 bytes) with a zero hop count. Nothing else can +// produce those bits here, so they are a deliberate declaration. +// +// Before this fix the skip was keyed on the route type and swallowed both +// cases. These tests pin the distinction. The two multi-byte fixtures are real +// packets off the Czech mesh (869.4 MHz), captured 2026-08-24/25. + +// insertAdvert adds one ADVERT transmission plus a matching observation. +func insertAdvert(t *testing.T, db *DB, id int, rawHex, hash, pubKey, name string, routeType int, seen string, seenEpoch int64) { + t.Helper() + if _, err := db.conn.Exec( + `INSERT INTO transmissions (raw_hex, hash, first_seen, route_type, payload_type, decoded_json) + VALUES (?, ?, ?, ?, 4, ?)`, + rawHex, hash, seen, routeType, + fmt.Sprintf(`{"pubKey":"%s","name":"%s","type":"ADVERT"}`, pubKey, name), + ); err != nil { + t.Fatalf("insert transmission %s: %v", hash, err) + } + if _, err := db.conn.Exec( + `INSERT INTO observations (transmission_id, observer_idx, snr, rssi, path_json, timestamp) + VALUES (?, 1, 10.0, -90, '[]', ?)`, id, seenEpoch, + ); err != nil { + t.Fatalf("insert observation %s: %v", hash, err) + } +} + +func zeroHopStore(t *testing.T) (*DB, string, int64) { + t.Helper() + db := setupTestDB(t) + now := time.Now().UTC().Add(-1 * time.Hour) + recent := now.Format(time.RFC3339) + db.conn.Exec(`INSERT INTO observers (id, name, iata, last_seen, first_seen, packet_count) + VALUES ('obs1', 'Obs', 'PRG', ?, '2026-01-01T00:00:00Z', 10)`, recent) + return db, recent, now.Unix() +} + +// TestZeroHopDirectAdvertDeclaredSizeIsEvidence: a zero-hop DIRECT advert whose +// path byte is 0x40 declares a 2-byte path hash and must be counted. +// Fixture: tth-hrebecna, 2026-08-25T09:29:40Z, header 0x12 (route 2, ADVERT). +func TestZeroHopDirectAdvertDeclaredSizeIsEvidence(t *testing.T) { + db, recent, epoch := zeroHopStore(t) + defer db.Close() + + const hrebecnaPK = "5361aa08929b6248bb61cb1fab0fdb97d6dba8ae44f53a44427f63e7798a6b6b" + insertAdvert(t, db, 1, + "1240"+hrebecnaPK+"83608d6a3d38eeb187a94a745daf2ffb3191d1ae49fb82f30a083ba081ffbb20"+ + "266331879c931267408acd04bcb8c8aec558d106ab40e942c70ce8e214af68529faa580b9243bd000322a5c300"+ + "7474682d6872656265636e61", + "zh_direct_2b", hrebecnaPK, "tth-hrebecna", 2, recent, epoch) + + store := NewPacketStore(db, nil) + store.Load() + info := store.GetNodeHashSizeInfo() + + ni, ok := info[hrebecnaPK] + if !ok { + t.Fatal("node with a 0x40 zero-hop advert missing from hash size info β€” the declared size was dropped") + } + if ni.HashSize != 2 { + t.Errorf("want HashSize=2 from path byte 0x40, got %d", ni.HashSize) + } + if ni.Inconsistent { + t.Error("single advert must not be flagged inconsistent") + } +} + +// TestZeroHopDirectAdvertThreeByteDeclaration: same rule at 3 bytes (0x80). +// Fixture: TTH-L1, 2026-08-24T15:36:45Z. +func TestZeroHopDirectAdvertThreeByteDeclaration(t *testing.T) { + db, recent, epoch := zeroHopStore(t) + defer db.Close() + + const pk = "e11e55684170982339efe2266a423c9040f909339646ee2adeea19c21ed2b02a" + insertAdvert(t, db, 1, + "1280"+pk+"0c658c6afcf586f0935c87b563dc0770f1ccb558126d16e4805eb4c556d511bf"+ + "c1f5d832a760bbe2f08d73b62c068c1a269425faadb2f575a69bcfc53ebae0cb65b71d0981"+ + "5454482d4c31", + "zh_direct_3b", pk, "TTH-L1", 2, recent, epoch) + + store := NewPacketStore(db, nil) + store.Load() + info := store.GetNodeHashSizeInfo() + + ni, ok := info[pk] + if !ok { + t.Fatal("node with a 0x80 zero-hop advert missing from hash size info") + } + if ni.HashSize != 3 { + t.Errorf("want HashSize=3 from path byte 0x80, got %d", ni.HashSize) + } +} + +// TestZeroHopDirectAdvertAllZeroByteStillSkipped: 0x00 carries no information β€” +// the sender wiped the size bits β€” so the node must stay absent rather than be +// recorded as 1 byte. This is the behaviour #649 asked for and it must survive. +func TestZeroHopDirectAdvertAllZeroByteStillSkipped(t *testing.T) { + db, recent, epoch := zeroHopStore(t) + defer db.Close() + + const pk = "aaaa000000000000000000000000000000000000000000000000000000000001" + insertAdvert(t, db, 1, "1200"+pk+"cafe", "zh_direct_zero", pk, "Legacy-Node", 2, recent, epoch) + + store := NewPacketStore(db, nil) + store.Load() + + if ni, ok := store.GetNodeHashSizeInfo()[pk]; ok { + t.Errorf("node seen only via a 0x00 zero-hop advert must have no hash size evidence, got %d", ni.HashSize) + } +} + +// TestZeroHopTransportDirectDeclaredSize: route type 3 puts the path byte at +// offset 5, behind four transport code bytes. The same content rule applies. +func TestZeroHopTransportDirectDeclaredSize(t *testing.T) { + db, recent, epoch := zeroHopStore(t) + defer db.Close() + + const declared = "aaaa000000000000000000000000000000000000000000000000000000000002" + const wiped = "aaaa000000000000000000000000000000000000000000000000000000000003" + + // header 0x13 = payload_type 4 (ADVERT), route type 3; 4 transport bytes; path byte 0x40. + insertAdvert(t, db, 1, "1301020304"+"40"+declared+"cafe", "zh_td_2b", declared, "TD-Declared", 3, recent, epoch) + // Same shape, path byte 0x00 β†’ no evidence. + insertAdvert(t, db, 2, "1305060708"+"00"+wiped+"cafe", "zh_td_zero", wiped, "TD-Wiped", 3, recent, epoch) + + store := NewPacketStore(db, nil) + store.Load() + info := store.GetNodeHashSizeInfo() + + if ni, ok := info[declared]; !ok { + t.Error("transport-direct node with 0x40 at offset 5 missing from hash size info") + } else if ni.HashSize != 2 { + t.Errorf("transport-direct: want HashSize=2, got %d", ni.HashSize) + } + if _, ok := info[wiped]; ok { + t.Error("transport-direct node with an all-zero path byte must have no evidence") + } +} + +// TestZeroHopDeclaredSizeReachesMultiByteCapability: the capability view derives +// "confirmed" from advert evidence, so a node heard only via zero-hop adverts +// should now be confirmed rather than left unknown. This is the user-visible +// half β€” it is what the map's multi-byte overlay reads. +func TestZeroHopDeclaredSizeReachesMultiByteCapability(t *testing.T) { + db, recent, epoch := zeroHopStore(t) + defer db.Close() + + const pk = "5361aa08929b6248bb61cb1fab0fdb97d6dba8ae44f53a44427f63e7798a6b6b" + db.conn.Exec(`INSERT INTO nodes (public_key, name, role, last_seen, first_seen, advert_count) + VALUES (?, 'tth-hrebecna', 'repeater', ?, ?, 1)`, pk, recent, recent) + insertAdvert(t, db, 1, "1240"+pk+"cafe", "zh_cap", pk, "tth-hrebecna", 2, recent, epoch) + + store := NewPacketStore(db, nil) + store.Load() + + var found *MultiByteCapEntry + for _, e := range store.computeMultiByteCapability(nil) { + if e.PublicKey == pk { + entry := e + found = &entry + break + } + } + if found == nil { + t.Fatal("node missing from multi-byte capability output") + } + if found.Status != "confirmed" { + t.Errorf("want status=confirmed from a declared zero-hop advert, got %q (evidence %q)", found.Status, found.Evidence) + } + if found.MaxHashSize != 2 { + t.Errorf("want MaxHashSize=2, got %d", found.MaxHashSize) + } +} From 61efd416389ac7fcd7cbc226227ff6d5e60a7932 Mon Sep 17 00:00:00 2001 From: efiten Date: Wed, 2 Sep 2026 14:22:30 +0200 Subject: [PATCH 08/26] feat(retention): add observerPurgeDays hard-delete for long-inactive observers (#1886) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## 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) (cherry picked from commit d821d9a390422179b6a5689cfdd33a77dd42b56e) Co-Authored-By: Claude Opus 5 --- cmd/ingestor/config.go | 17 +- cmd/ingestor/db.go | 36 +++++ cmd/ingestor/main.go | 10 ++ cmd/ingestor/observer_purge_test.go | 222 ++++++++++++++++++++++++++ cmd/server/readonly_invariant_test.go | 1 + config.example.json | 3 +- 6 files changed, 287 insertions(+), 2 deletions(-) create mode 100644 cmd/ingestor/observer_purge_test.go diff --git a/cmd/ingestor/config.go b/cmd/ingestor/config.go index be1d55f4f..3c022a71f 100644 --- a/cmd/ingestor/config.go +++ b/cmd/ingestor/config.go @@ -144,7 +144,13 @@ func (c *Config) ClientRxCoverageEnabled() bool { type RetentionConfig struct { NodeDays int `json:"nodeDays"` ObserverDays int `json:"observerDays"` - MetricsDays int `json:"metricsDays"` + // ObserverPurgeDays hard-deletes observers that observerDays already + // soft-deleted, once they are older than this and no observation, + // metric or dropped-packet row still references them; 0 disables. + // Only meaningful above observerDays and packetDays β€” below those the + // reference guards keep every candidate row anyway. + ObserverPurgeDays int `json:"observerPurgeDays"` + MetricsDays int `json:"metricsDays"` // PacketDays is the retention window for transmissions (#1283). // Ownership moved from cmd/server to cmd/ingestor; 0 disables. PacketDays int `json:"packetDays"` @@ -163,6 +169,15 @@ func (c *Config) PacketDaysOrZero() int { return 0 } +// ObserverPurgeDaysOrZero returns the configured retention.observerPurgeDays +// or 0 (disabled) if not set. +func (c *Config) ObserverPurgeDaysOrZero() int { + if c.Retention != nil && c.Retention.ObserverPurgeDays > 0 { + return c.Retention.ObserverPurgeDays + } + return 0 +} + // ClientRxDaysOrZero returns the configured retention.clientRxDays or 0 // (disabled) if not set. func (c *Config) ClientRxDaysOrZero() int { diff --git a/cmd/ingestor/db.go b/cmd/ingestor/db.go index a6604389d..1cf4bf408 100644 --- a/cmd/ingestor/db.go +++ b/cmd/ingestor/db.go @@ -1886,6 +1886,42 @@ func (s *Store) RemoveStaleObservers(observerDays int) (int64, error) { return removed, nil } +// PurgeStaleObservers hard-deletes rows RemoveStaleObservers already soft-deleted, +// once they are older than purgeDays and nothing references them any more. It is +// the second stage of observer retention: the soft-delete hides the observer, +// this reclaims the row after its packets have aged out via packetDays. +// +// observations.observer_idx is a bare rowid with no foreign key, so deleting a +// still-referenced observer silently orphans history β€” packets_v stops resolving +// the observer and the packets are mis-attributed. The three NOT EXISTS guards +// are what make the delete safe; they are correctness, not defensive padding. +// Each is an index seek per candidate row (observers is O(100)), so the whole +// statement stays cheap enough for the daily retention tick. +// +// purgeDays <= 0 disables the purge (the default). +func (s *Store) PurgeStaleObservers(purgeDays int) (int64, error) { + if purgeDays <= 0 { + return 0, nil // disabled + } + cutoff := time.Now().UTC().AddDate(0, 0, -purgeDays).Format(time.RFC3339) + // Tagged for /api/perf writer-lock visibility (#1340). + result, err := s.instrumentedExec("purge_observers", ` + DELETE FROM observers + WHERE inactive = 1 + AND last_seen < ? + 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)`, cutoff) + if err != nil { + return 0, fmt.Errorf("purge stale observers: %w", err) + } + purged, _ := result.RowsAffected() + if purged > 0 { + log.Printf("Purged %d inactive observer(s) with no remaining data (not seen in %d days)", purged, purgeDays) + } + return purged, nil +} + // DroppedPacket holds data for a packet rejected during ingest. type DroppedPacket struct { Hash string diff --git a/cmd/ingestor/main.go b/cmd/ingestor/main.go index c00b60544..8138d8457 100644 --- a/cmd/ingestor/main.go +++ b/cmd/ingestor/main.go @@ -260,6 +260,14 @@ func main() { observerDays := cfg.ObserverDaysOrDefault() store.RemoveStaleObservers(observerDays) + // Observer purge: second stage, hard-deletes long-inactive observers whose + // packets have already aged out. Always runs after the soft-delete so a row + // crossing both thresholds is finalised in a single pass. 0 = disabled. + observerPurgeDays := cfg.ObserverPurgeDaysOrZero() + if _, err := store.PurgeStaleObservers(observerPurgeDays); err != nil { + log.Printf("[prune] error: %v", err) + } + // Metrics retention: prune old metrics on startup metricsDays := cfg.MetricsRetentionDays() store.PruneOldMetrics(metricsDays) @@ -319,9 +327,11 @@ func main() { go func() { time.Sleep(90 * time.Second) // stagger after metrics prune store.RemoveStaleObservers(observerDays) + store.PurgeStaleObservers(observerPurgeDays) store.RunIncrementalVacuum(vacuumPages) for range observerRetentionTicker.C { store.RemoveStaleObservers(observerDays) + store.PurgeStaleObservers(observerPurgeDays) store.RunIncrementalVacuum(vacuumPages) } }() diff --git a/cmd/ingestor/observer_purge_test.go b/cmd/ingestor/observer_purge_test.go new file mode 100644 index 000000000..8a3f6a6a6 --- /dev/null +++ b/cmd/ingestor/observer_purge_test.go @@ -0,0 +1,222 @@ +package main + +import ( + "testing" + "time" +) + +// staleObserver inserts an observer already soft-deleted (inactive = 1) with +// last_seen ageDays in the past, and returns its rowid. +func staleObserver(t *testing.T, s *Store, id string) int64 { + t.Helper() + if err := s.UpsertObserver(id, id, "LAX", nil); err != nil { + t.Fatal(err) + } + old := time.Now().UTC().AddDate(0, 0, -90).Format(time.RFC3339) + if _, err := s.db.Exec(`UPDATE observers SET last_seen = ?, inactive = 1 WHERE id = ?`, old, id); err != nil { + t.Fatal(err) + } + var rowid int64 + if err := s.db.QueryRow(`SELECT rowid FROM observers WHERE id = ?`, id).Scan(&rowid); err != nil { + t.Fatal(err) + } + return rowid +} + +func observerExists(t *testing.T, s *Store, id string) bool { + t.Helper() + var n int + if err := s.db.QueryRow(`SELECT COUNT(*) FROM observers WHERE id = ?`, id).Scan(&n); err != nil { + t.Fatal(err) + } + return n > 0 +} + +func TestPurgeStaleObserversDeletesUnreferencedRow(t *testing.T) { + store := newTestStore(t) + staleObserver(t, store, "obs-gone") + + purged, err := store.PurgeStaleObservers(30) + if err != nil { + t.Fatal(err) + } + if purged != 1 { + t.Errorf("purged=%d, want 1", purged) + } + if observerExists(t, store, "obs-gone") { + t.Error("obs-gone still present, want hard-deleted") + } +} + +// Regression: an observer whose rowid is still referenced by observations must +// survive the purge β€” deleting it orphans observations.observer_idx, which +// breaks the packets_v join and mis-attributes historical packets. +func TestPurgeStaleObserversKeepsObserverWithObservations(t *testing.T) { + store := newTestStore(t) + rowid := staleObserver(t, store, "obs-referenced") + + if _, err := store.db.Exec( + `INSERT INTO transmissions (id, raw_hex, hash, first_seen) VALUES (1, 'AA', 'h1', ?)`, + time.Now().UTC().Format(time.RFC3339), + ); err != nil { + t.Fatal(err) + } + if _, err := store.db.Exec( + `INSERT INTO observations (transmission_id, observer_idx, timestamp) VALUES (1, ?, 0)`, rowid, + ); err != nil { + t.Fatal(err) + } + + purged, err := store.PurgeStaleObservers(30) + if err != nil { + t.Fatal(err) + } + if purged != 0 { + t.Errorf("purged=%d, want 0 (row is referenced by observations)", purged) + } + if !observerExists(t, store, "obs-referenced") { + t.Error("obs-referenced was deleted, want kept") + } + + var orphans int + if err := store.db.QueryRow( + `SELECT COUNT(*) FROM observations WHERE observer_idx IS NOT NULL + AND observer_idx NOT IN (SELECT rowid FROM observers)`, + ).Scan(&orphans); err != nil { + t.Fatal(err) + } + if orphans != 0 { + t.Errorf("orphaned observations=%d, want 0", orphans) + } +} + +func TestPurgeStaleObserversKeepsObserverWithMetrics(t *testing.T) { + store := newTestStore(t) + staleObserver(t, store, "obs-metrics") + + if _, err := store.db.Exec( + `INSERT INTO observer_metrics (observer_id, timestamp) VALUES ('obs-metrics', ?)`, + time.Now().UTC().Format(time.RFC3339), + ); err != nil { + t.Fatal(err) + } + + purged, err := store.PurgeStaleObservers(30) + if err != nil { + t.Fatal(err) + } + if purged != 0 { + t.Errorf("purged=%d, want 0 (row is referenced by observer_metrics)", purged) + } + if !observerExists(t, store, "obs-metrics") { + t.Error("obs-metrics was deleted, want kept") + } +} + +func TestPurgeStaleObserversKeepsObserverWithDroppedPackets(t *testing.T) { + store := newTestStore(t) + staleObserver(t, store, "obs-dropped") + + if _, err := store.db.Exec( + `INSERT INTO dropped_packets (reason, observer_id) VALUES ('bad-sig', 'obs-dropped')`, + ); err != nil { + t.Fatal(err) + } + + purged, err := store.PurgeStaleObservers(30) + if err != nil { + t.Fatal(err) + } + if purged != 0 { + t.Errorf("purged=%d, want 0 (row is referenced by dropped_packets)", purged) + } + if !observerExists(t, store, "obs-dropped") { + t.Error("obs-dropped was deleted, want kept") + } +} + +// An observer old enough to purge but not yet soft-deleted must be left to +// RemoveStaleObservers β€” the hard purge only ever finalises an inactive row. +func TestPurgeStaleObserversKeepsActiveObserver(t *testing.T) { + store := newTestStore(t) + if err := store.UpsertObserver("obs-active", "Active", "LAX", nil); err != nil { + t.Fatal(err) + } + old := time.Now().UTC().AddDate(0, 0, -90).Format(time.RFC3339) + if _, err := store.db.Exec(`UPDATE observers SET last_seen = ? WHERE id = ?`, old, "obs-active"); err != nil { + t.Fatal(err) + } + + purged, err := store.PurgeStaleObservers(30) + if err != nil { + t.Fatal(err) + } + if purged != 0 { + t.Errorf("purged=%d, want 0 (inactive = 0)", purged) + } + if !observerExists(t, store, "obs-active") { + t.Error("obs-active was deleted, want kept") + } +} + +func TestPurgeStaleObserversKeepsObserverInsideWindow(t *testing.T) { + store := newTestStore(t) + if err := store.UpsertObserver("obs-recent", "Recent", "LAX", nil); err != nil { + t.Fatal(err) + } + recent := time.Now().UTC().AddDate(0, 0, -20).Format(time.RFC3339) + if _, err := store.db.Exec( + `UPDATE observers SET last_seen = ?, inactive = 1 WHERE id = ?`, recent, "obs-recent", + ); err != nil { + t.Fatal(err) + } + + purged, err := store.PurgeStaleObservers(30) + if err != nil { + t.Fatal(err) + } + if purged != 0 { + t.Errorf("purged=%d, want 0 (last_seen inside the 30-day window)", purged) + } + if !observerExists(t, store, "obs-recent") { + t.Error("obs-recent was deleted, want kept") + } +} + +func TestPurgeStaleObserversDisabled(t *testing.T) { + store := newTestStore(t) + staleObserver(t, store, "obs-kept") + + for _, days := range []int{0, -1} { + purged, err := store.PurgeStaleObservers(days) + if err != nil { + t.Fatal(err) + } + if purged != 0 { + t.Errorf("PurgeStaleObservers(%d) purged=%d, want 0 (disabled)", days, purged) + } + } + if !observerExists(t, store, "obs-kept") { + t.Error("obs-kept was deleted, want kept while purge is disabled") + } +} + +func TestObserverPurgeDaysOrZero(t *testing.T) { + tests := []struct { + name string + cfg *Config + want int + }{ + {"nil retention", &Config{}, 0}, + {"unset", &Config{Retention: &RetentionConfig{}}, 0}, + {"configured", &Config{Retention: &RetentionConfig{ObserverPurgeDays: 30}}, 30}, + {"negative disables", &Config{Retention: &RetentionConfig{ObserverPurgeDays: -1}}, 0}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := tt.cfg.ObserverPurgeDaysOrZero(); got != tt.want { + t.Errorf("ObserverPurgeDaysOrZero() = %d, want %d", got, tt.want) + } + }) + } +} diff --git a/cmd/server/readonly_invariant_test.go b/cmd/server/readonly_invariant_test.go index cc2fdde70..896621826 100644 --- a/cmd/server/readonly_invariant_test.go +++ b/cmd/server/readonly_invariant_test.go @@ -92,6 +92,7 @@ func TestServerDBHasNoWriteMethods(t *testing.T) { "PruneOldPackets", "PruneOldMetrics", "RemoveStaleObservers", + "PurgeStaleObservers", // #738 / one-click geo-prune: the DELETE must live on the // ingestor's *Store. The server's HTTP handler now enqueues a // marker file (see internal/prunequeue); it does not write. diff --git a/config.example.json b/config.example.json index e169a6aa5..5a72ae2ac 100644 --- a/config.example.json +++ b/config.example.json @@ -10,9 +10,10 @@ "retention": { "nodeDays": 7, "observerDays": 14, + "observerPurgeDays": 0, "packetDays": 30, "clientRxDays": 30, - "_comment": "nodeDays: nodes not seen in N days moved to inactive_nodes (default 7). observerDays: observers not sending data in N days are removed (-1 = keep forever, default 14). packetDays: transmissions older than N days are deleted (0 = disabled). clientRxDays: mobile client-RX coverage rows (client_receptions/client_observers) older than N days are deleted (0 = disabled) β€” bounds the opt-in coverage tables (#1727). NOTE (#1283): all retention fields are consumed by the INGESTOR process. The server is read-only and never prunes." + "_comment": "nodeDays: nodes not seen in N days moved to inactive_nodes (default 7). observerDays: observers not sending data in N days are removed (-1 = keep forever, default 14). observerPurgeDays: observers already marked inactive AND not seen in N days are hard-deleted, but only when no observation, observer_metrics or dropped_packets row still references them (0 = disabled, the default). Set it above both observerDays and packetDays β€” below those the reference guards keep every row anyway. packetDays: transmissions older than N days are deleted (0 = disabled). clientRxDays: mobile client-RX coverage rows (client_receptions/client_observers) older than N days are deleted (0 = disabled) β€” bounds the opt-in coverage tables (#1727). NOTE (#1283): all retention fields are consumed by the INGESTOR process. The server is read-only and never prunes." }, "db": { "vacuumOnStartup": false, From b251f2994d4f90842c3c8279196499362e107903 Mon Sep 17 00:00:00 2001 From: efiten Date: Wed, 2 Sep 2026 14:34:20 +0200 Subject: [PATCH 09/26] fix(#1904): keep resolved full-pubkey hops across a path-hop index rebuild (#1907) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) (cherry picked from commit f081f91b88ff698286f201e67f2442ffac78a4da) Co-Authored-By: Claude Opus 5 --- cmd/server/pathhop_rebuild_1904_test.go | 105 ++++++++++++++++++++++++ cmd/server/store.go | 73 +++++++++++++++- 2 files changed, 175 insertions(+), 3 deletions(-) create mode 100644 cmd/server/pathhop_rebuild_1904_test.go diff --git a/cmd/server/pathhop_rebuild_1904_test.go b/cmd/server/pathhop_rebuild_1904_test.go new file mode 100644 index 000000000..7848cb6af --- /dev/null +++ b/cmd/server/pathhop_rebuild_1904_test.go @@ -0,0 +1,105 @@ +package main + +import ( + "sync" + "testing" +) + +// Issue #1904: buildPathHopIndex() rebuilds byPathHop from raw path_json +// hops and opens by discarding the map. But byPathHop also holds resolved +// full-pubkey keys fed by indexResolvedPathHops, and those pubkey strings +// are retained nowhere (#800 dropped the per-StoreTx ResolvedPath field in +// favour of a hash-only index), so a plain rebuild silently drops every +// resolved attribution. On a cold start that leaves relay_count_*, +// last_relayed and transported_scopes empty for every node until live +// ingestion refills the index. +// +// The rebuild must therefore RETAIN resolved entries β€” but only for +// transmissions still in s.packets. Eviction's removeTxFromPathHopIndex +// only strips raw hops (it derives them from txGetParsedPath), so evicted +// transmissions linger under their resolved keys; the rebuild is where +// they get dropped. + +const ( + hop1904Raw = "a3" + hop1904Resolved = "a3f19c2b7d4e5081aa22bb33cc44dd55ee66ff778899000112233445566778899" + hop1904Evicted = "b7002211ffeeddccbbaa99887766554433221100aabbccddeeff001122334455" +) + +// pathHopTx builds a transmission whose wire path is a single raw hop. +func pathHopTx(id int, path string) *StoreTx { + return &StoreTx{ID: id, Hash: "pathhop-tx-" + path, PathJSON: `["` + hop1904Raw + `"]`} +} + +func newPathHopStore(packets []*StoreTx, idx map[string][]*StoreTx) *PacketStore { + return &PacketStore{packets: packets, byPathHop: idx, mu: sync.RWMutex{}} +} + +func hopKeyHas(idx map[string][]*StoreTx, key string, tx *StoreTx) bool { + for _, t := range idx[key] { + if t == tx { + return true + } + } + return false +} + +func TestBuildPathHopIndex_RetainsResolvedHops_1904(t *testing.T) { + tx := pathHopTx(1, hop1904Raw) + // Shape produced by a chunk scan: raw hop from path_json PLUS the + // resolved full pubkey that indexResolvedPathHops added. + store := newPathHopStore([]*StoreTx{tx}, map[string][]*StoreTx{ + hop1904Raw: {tx}, + hop1904Resolved: {tx}, + }) + + store.buildPathHopIndex() + + if !hopKeyHas(store.byPathHop, hop1904Raw, tx) { + t.Fatalf("raw hop %q lost by rebuild", hop1904Raw) + } + if !hopKeyHas(store.byPathHop, hop1904Resolved, tx) { + t.Fatalf("resolved pubkey hop %q dropped by rebuild β€” #1904", hop1904Resolved) + } +} + +func TestBuildPathHopIndex_DropsResolvedHopsOfEvictedTx_1904(t *testing.T) { + live := pathHopTx(1, hop1904Raw) + evicted := pathHopTx(2, hop1904Raw) // NOT in s.packets any more + + store := newPathHopStore([]*StoreTx{live}, map[string][]*StoreTx{ + hop1904Raw: {live}, + hop1904Resolved: {live}, + hop1904Evicted: {evicted}, + }) + + store.buildPathHopIndex() + + if hopKeyHas(store.byPathHop, hop1904Evicted, evicted) { + t.Fatal("evicted transmission retained under its resolved key β€” the rebuild must not turn eviction's known gap into a permanent leak") + } + if _, ok := store.byPathHop[hop1904Evicted]; ok { + t.Fatalf("empty key %q left behind after dropping its only entry", hop1904Evicted) + } + if !hopKeyHas(store.byPathHop, hop1904Resolved, live) { + t.Fatal("live resolved hop dropped while pruning evicted ones") + } +} + +func TestBuildPathHopIndex_NoDuplicateOnRepeatedBuild_1904(t *testing.T) { + tx := pathHopTx(1, hop1904Raw) + store := newPathHopStore([]*StoreTx{tx}, map[string][]*StoreTx{ + hop1904Raw: {tx}, + hop1904Resolved: {tx}, + }) + + store.buildPathHopIndex() + store.buildPathHopIndex() + + if got := len(store.byPathHop[hop1904Raw]); got != 1 { + t.Fatalf("raw hop bucket has %d entries after two builds, want 1", got) + } + if got := len(store.byPathHop[hop1904Resolved]); got != 1 { + t.Fatalf("resolved hop bucket has %d entries after two builds, want 1", got) + } +} diff --git a/cmd/server/store.go b/cmd/server/store.go index 7e9ecf604..118b4ccfc 100644 --- a/cmd/server/store.go +++ b/cmd/server/store.go @@ -4155,14 +4155,81 @@ func (s *PacketStore) buildSubpathIndex() { len(s.spIndex), s.spTotalPaths) } -// buildPathHopIndex scans all packets and populates byPathHop. +// buildPathHopIndex rebuilds byPathHop: raw wire hops from every packet's +// path_json, plus the resolved full-pubkey hops carried over from the +// previous index (see retainResolvedPathHops). // Must be called with s.mu held. func (s *PacketStore) buildPathHopIndex() { - s.byPathHop = make(map[string][]*StoreTx, 4096) + prev := s.byPathHop + s.byPathHop = make(map[string][]*StoreTx, max(4096, len(prev))) for _, tx := range s.packets { addTxToPathHopIndex(s.byPathHop, tx) } - log.Printf("[store] Built path-hop index: %d unique keys", len(s.byPathHop)) + retained := s.retainResolvedPathHops(prev) + log.Printf("[store] Built path-hop index: %d unique keys (%d resolved-hop entries retained)", + len(s.byPathHop), retained) +} + +// retainResolvedPathHops re-merges the entries of a pre-rebuild byPathHop +// that the raw-hop pass above cannot reproduce: the resolved full-pubkey +// keys fed by indexResolvedPathHops. Their pubkey strings are retained +// nowhere β€” #800 dropped the per-StoreTx ResolvedPath field in favour of a +// hash-only membership index β€” so a plain rebuild silently discarded every +// resolved relay attribution, leaving relay counts and transported scopes +// empty after each cold load until live ingestion refilled them (#1904). +// +// Only transmissions still in s.packets are carried over. This matters: +// eviction's removeTxFromPathHopIndex strips raw hops only (it derives them +// from txGetParsedPath), so evicted transmissions linger in prev under their +// resolved keys. Filtering them here is what keeps the index bounded by the +// eviction policy instead of turning that gap into a permanent leak. +// +// Cost is O(entries in prev) with one reused scratch map, and it runs only +// where buildPathHopIndex already runs β€” cold load and background-fill +// completion β€” never on an ingest or request path. +// +// Returns the number of entries carried over. Must be called with s.mu held, +// after s.byPathHop has been rebuilt from raw hops. +func (s *PacketStore) retainResolvedPathHops(prev map[string][]*StoreTx) int { + if len(prev) == 0 { + return 0 + } + live := make(map[*StoreTx]struct{}, len(s.packets)) + for _, tx := range s.packets { + live[tx] = struct{}{} + } + + // Reused across keys (cleared per key) so a large index does not churn + // one map allocation per key. Guards against both a key that the raw + // pass already produced and repeated appends of the same tx in prev β€” + // indexResolvedPathHops dedups within a call, not across the several + // observations of one transmission. + seen := make(map[*StoreTx]struct{}, 16) + retained := 0 + for key, list := range prev { + if len(list) == 0 { + continue + } + clear(seen) + for _, tx := range s.byPathHop[key] { + seen[tx] = struct{}{} + } + for _, tx := range list { + if tx == nil { + continue + } + if _, ok := live[tx]; !ok { + continue + } + if _, dup := seen[tx]; dup { + continue + } + seen[tx] = struct{}{} + s.byPathHop[key] = append(s.byPathHop[key], tx) + retained++ + } + } + return retained } // addTxToPathHopIndex indexes a transmission under each unique raw hop key. From fa90d88757fcfda3d3073b0388d54e8afbf8c976 Mon Sep 17 00:00:00 2001 From: Jonathan Herlin Date: Wed, 2 Sep 2026 14:51:30 +0200 Subject: [PATCH 10/26] 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/ 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/ entry would immediately re-redirect forward again on hashchange, so back button never reached the packets view they came from. (cherry picked from commit a46a6d55eb7912e1f917af35c33f6de28429e09e) Co-Authored-By: Claude Opus 5 --- public/app.js | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/public/app.js b/public/app.js index 52bab2ce8..0674d5d1e 100644 --- a/public/app.js +++ b/public/app.js @@ -1170,16 +1170,20 @@ function navigate() { closeNav(); // Backward-compat redirect: #/traces/ β†’ #/tools/trace/ (issue #944). + // Uses replaceState (not location.hash =) so this redirect doesn't add its + // own history entry, otherwise the back button gets stuck bouncing off it. if (location.hash.startsWith('#/traces/')) { - location.hash = location.hash.replace('#/traces/', '#/tools/trace/'); + history.replaceState(null, '', location.hash.replace('#/traces/', '#/tools/trace/')); + navigate(); return; } // Backward-compat redirect: #/roles β†’ #/analytics?tab=roles (issue #1085). // The Roles page was folded into the Analytics tab strip; old links and - // bookmarks must keep working. + // bookmarks must keep working. Uses replaceState for the same reason as above. if (location.hash === '#/roles' || location.hash.startsWith('#/roles?') || location.hash.startsWith('#/roles/')) { - location.hash = '#/analytics?tab=roles'; + history.replaceState(null, '', '#/analytics?tab=roles'); + navigate(); return; } From d205f46861ed5e83dc3b562477d9ab6a81ef0dc3 Mon Sep 17 00:00:00 2001 From: Joel Claw <274497724+Joel-Claw@users.noreply.github.com> Date: Wed, 2 Sep 2026 14:53:16 +0200 Subject: [PATCH 11/26] 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> (cherry picked from commit 4a776454ca96c36ba85f2ca807120f3ff0686a66) Co-Authored-By: Claude Opus 5 --- cmd/server/store.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/cmd/server/store.go b/cmd/server/store.go index 118b4ccfc..78b02b961 100644 --- a/cmd/server/store.go +++ b/cmd/server/store.go @@ -178,7 +178,6 @@ type PacketStore struct { byNode map[string][]*StoreTx // pubkey β†’ transmissions nodeHashes map[string]map[string]bool // pubkey β†’ Set byPathHop map[string][]*StoreTx // lowercase hop/pubkey β†’ transmissions with that hop in path - relayTimes map[string][]int64 // lowercase pubkey β†’ sorted unix-millis of relay events (full pubkeys only) byPayloadType map[int][]*StoreTx // payload_type β†’ transmissions loaded bool totalObs int @@ -646,7 +645,6 @@ func NewPacketStore(db *DB, cfg *PacketStoreConfig, cacheTTLs ...map[string]inte byObserver: make(map[string][]*StoreObs), byNode: make(map[string][]*StoreTx), byPathHop: make(map[string][]*StoreTx), - relayTimes: make(map[string][]int64), nodeHashes: make(map[string]map[string]bool), byPayloadType: make(map[int][]*StoreTx), rfCache: make(map[string]*cachedResult), From 9334d1a76533bf51dae7d7c84f106a8463eb929a Mon Sep 17 00:00:00 2001 From: Jonathan Herlin Date: Wed, 2 Sep 2026 15:03:41 +0200 Subject: [PATCH 12/26] 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. (cherry picked from commit eb8f376c6c29c8e703deebc1eb2a3dd8467a7276) Co-Authored-By: Claude Opus 5 --- cmd/server/db.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/cmd/server/db.go b/cmd/server/db.go index 0fbd4b3c4..872656067 100644 --- a/cmd/server/db.go +++ b/cmd/server/db.go @@ -1041,8 +1041,11 @@ func (db *DB) GetNodes(limit, offset int, role, search, before, lastHeard, sortB if !db.isV3() { joinCond = "obs.id = o.observer_id" } + // #1143: from_pubkey is a dedicated, indexed column populated at + // ingest (and backfilled) for ADVERT rows specifically so pubkey + // lookups don't need to JSON_EXTRACT + parse decoded_json per row. subq := fmt.Sprintf(`public_key IN ( - SELECT DISTINCT JSON_EXTRACT(t.decoded_json, '$.pubKey') + SELECT DISTINCT t.from_pubkey FROM transmissions t JOIN observations o ON o.transmission_id = t.id JOIN observers obs ON %s From 9ef31367f5ba65f33394251df20cac7173d8cd20 Mon Sep 17 00:00:00 2001 From: Joel Claw <274497724+Joel-Claw@users.noreply.github.com> Date: Wed, 2 Sep 2026 23:20:39 +0200 Subject: [PATCH 13/26] perf: reuse ctx buffer in resolvePathForObs, cache ReadMemStats per store (#1873) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Problem Three hot-path inefficiencies causing excess CPU and memory allocations: ### 1. `filterTxSlice` starts with nil slice `filterTxSlice` is called on the full `s.packets` slice (50k+ packets) for every query that doesn't hit a fast-path index. Starting with `var result []*StoreTx` means Go's append does ~15 growth+copy cycles (1β†’2β†’4β†’8β†’...β†’32768β†’65536) before reaching steady state. ### 2. `resolvePathForObs` allocates per hop Each hop in the path resolution loop allocates a new `ctx` slice (`make([]string, len(contextPKs), len(contextPKs)+2)`). For a 5-hop path, that's 5 allocations per observation. With 500+ observations per ingest batch, that's 2500+ small allocations. ### 3. `estimatedMemoryMB` calls `runtime.ReadMemStats` without caching `runtime.ReadMemStats()` triggers a STW (stop-the-world) pause. It's called from stats/debug endpoints (`GetStoreStats`, `GetPerfStoreStats`) that may be polled frequently. The routes.go layer already caches this with a 5s TTL, but the store layer doesn't. ## Fix 1. **Pre-allocate `filterTxSlice`**: `make([]*StoreTx, 0, n/2)` β€” the 2x over-allocation is cheaper than repeated growth+copy. 2. **Reuse ctx buffer**: Allocate one `ctx` buffer before the hop loop, reset to base length each iteration with `ctx = ctx[:ctxLen]`. 3. **Cache `ReadMemStats`**: 5-second TTL cache matching the routes.go pattern. Uses a package-level mutex (not on `PacketStore`) to avoid adding a field. ## Testing - `go build` passes - No behavior change β€” same results, fewer allocations --------- Co-authored-by: Joel Claw <358739783+Joel-Claw@users.noreply.github.com> (cherry picked from commit ac6fbaf9f37b75b985aa96412bbc7cf639127984) Co-Authored-By: Claude Opus 5 --- cmd/server/neighbor_persist.go | 9 +++++++-- cmd/server/store.go | 24 +++++++++++++++++++++--- 2 files changed, 28 insertions(+), 5 deletions(-) diff --git a/cmd/server/neighbor_persist.go b/cmd/server/neighbor_persist.go index 5204147be..134b6b530 100644 --- a/cmd/server/neighbor_persist.go +++ b/cmd/server/neighbor_persist.go @@ -132,9 +132,14 @@ func resolvePathForObs(pathJSON, observerID string, tx *StoreTx, pm *prefixMap, contextPKs = append(contextPKs, strings.ToLower(fromNode)) } resolved := make([]*string, len(hops)) + // Reuse a single ctx buffer across hops instead of allocating per hop. + // Capacity +2 for the previous-hop resolved PK that may be appended. + ctx := make([]string, len(contextPKs), len(contextPKs)+2) + copy(ctx, contextPKs) + ctxLen := len(ctx) for i, hop := range hops { - ctx := make([]string, len(contextPKs), len(contextPKs)+2) - copy(ctx, contextPKs) + // Reset to base context (trim any previously appended resolved PK) + ctx = ctx[:ctxLen] if i > 0 && resolved[i-1] != nil { ctx = append(ctx, *resolved[i-1]) } diff --git a/cmd/server/store.go b/cmd/server/store.go index 78b02b961..d6efc649c 100644 --- a/cmd/server/store.go +++ b/cmd/server/store.go @@ -475,6 +475,13 @@ type PacketStore struct { trackedBytes int64 // running total of estimated packet store memory memoryEstimator func() float64 // injectable for tests; nil = use runtime.ReadMemStats (stats only) + // Per-store ReadMemStats cache (5s TTL). Fields (not package-level vars) so + // that test helpers constructing &PacketStore{...} directly get independent + // cache state, avoiding order-dependent test failures. + estMemMu sync.Mutex + estMemVal float64 + estMemAt time.Time + // Short-lived cache for the observations aggregate in GetStoreStats (30s TTL). // Avoids a per-/api/stats full-table scan; values accurate to ~30s which is // sufficient for dashboard display. @@ -4652,13 +4659,24 @@ func estimateStoreObsBytes(obs *StoreObs) int64 { // estimatedMemoryMB returns current Go heap allocation in MB. // Kept for stats/debug endpoints only β€” NOT used in eviction decisions. // In tests, memoryEstimator can be set to inject a deterministic value. +// Caches the result for 5 seconds because runtime.ReadMemStats() stops the +// world and this is called from stats/debug endpoints that may be polled. +// The cache is per-store (not package-level) so that test helpers constructing +// &PacketStore{...} directly get independent cache state, avoiding +// order-dependent test failures from a shared global cache. func (s *PacketStore) estimatedMemoryMB() float64 { if s.memoryEstimator != nil { return s.memoryEstimator() } - var ms runtime.MemStats - runtime.ReadMemStats(&ms) - return float64(ms.HeapAlloc) / 1048576.0 + s.estMemMu.Lock() + defer s.estMemMu.Unlock() + if time.Since(s.estMemAt) > 5*time.Second { + var ms runtime.MemStats + runtime.ReadMemStats(&ms) + s.estMemVal = float64(ms.HeapAlloc) / 1048576.0 + s.estMemAt = time.Now() + } + return s.estMemVal } // trackedMemoryMB returns the self-accounted packet store memory in MB. From a80759aa3473882603f29065895adceaee2c9c1b Mon Sep 17 00:00:00 2001 From: efiten Date: Thu, 3 Sep 2026 09:41:36 +0200 Subject: [PATCH 14/26] 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) (cherry picked from commit e2df9bbd3e5a28445eea42774713189af14e18af) Co-Authored-By: Claude Opus 5 --- public/analytics.js | 15 +++++++++++- test-issue-1758-ng-filter-rerenders-e2e.js | 28 ++++++++++++++++++++++ 2 files changed, 42 insertions(+), 1 deletion(-) diff --git a/public/analytics.js b/public/analytics.js index 846ca04bb..74ad68fa1 100644 --- a/public/analytics.js +++ b/public/analytics.js @@ -246,7 +246,20 @@ } // Re-render when distance unit or theme changes - _themeRefreshHandler = function () { renderTab(_currentTab); }; + _themeRefreshHandler = function () { + // #1925: never full-rebuild the neighbor-graph tab on theme-refresh. + // Every page load fires one theme-refresh ~300ms after + // /api/config/theme resolves (app.js dispatches 'theme-changed', then + // debounces 300ms). renderTab() here replaced el.innerHTML, which reset + // the role checkboxes to their defaults and rebuilt _ngState from the + // full graph, discarding any filtering the user had applied in the + // meantime. Restarting the renderer keeps the current filter state and + // still picks up the new theme: node colors are read live per frame + // from window.ROLE_COLORS, role swatches use CSS tokens, and the one + // cached value (_labelColor) is re-read on restart. + if (_currentTab === 'neighbor-graph' && _ngState) { startGraphRenderer(); return; } + renderTab(_currentTab); + }; window.addEventListener('theme-refresh', _themeRefreshHandler); loadAnalytics(); diff --git a/test-issue-1758-ng-filter-rerenders-e2e.js b/test-issue-1758-ng-filter-rerenders-e2e.js index 1a75fdb43..f51e323ea 100644 --- a/test-issue-1758-ng-filter-rerenders-e2e.js +++ b/test-issue-1758-ng-filter-rerenders-e2e.js @@ -204,6 +204,34 @@ function buildFixture() { if (reFiltered.canvasDisplay === 'none') fail('second filter-down: expected #ngCanvas visible again'); console.log(' βœ“ second filter-down: render restored across repeated cycles'); + // --- #1925: a theme-refresh must not discard the applied filter. + // Every page load fires one 'theme-refresh' about 300ms after + // /api/config/theme resolves. It used to call renderTab(), which replaced + // el.innerHTML (resetting the role checkboxes to their defaults) and + // rebuilt _ngState from the full graph, putting #ngSkipMsg back. On an + // idle machine the test finished ~90ms before that landed; under CI load + // it landed mid-test, which is both failure modes of #1925. + // Dispatching the event directly makes that deterministic, so this asserts + // the cause rather than waiting for the race to show up. + await page.evaluate(() => window.dispatchEvent(new CustomEvent('theme-refresh'))); + await page.waitForTimeout(500); + const afterTheme = await page.evaluate(() => { + const canvas = document.getElementById('ngCanvas'); + const cb = document.querySelector('#ngRoleChecks input[data-role="companion"]'); + return { + hasSkip: !!document.getElementById('ngSkipMsg'), + canvasDisplay: canvas ? getComputedStyle(canvas).display : '(no canvas)', + companionChecked: cb ? cb.checked : '(no checkbox)', + }; + }); + if (afterTheme.companionChecked !== false) { + fail('theme-refresh reset the role filter (companion re-checked): ' + + JSON.stringify(afterTheme)); + } + if (afterTheme.hasSkip) fail('theme-refresh brought #ngSkipMsg back: ' + JSON.stringify(afterTheme)); + if (afterTheme.canvasDisplay === 'none') fail('theme-refresh hid #ngCanvas again'); + console.log(' βœ“ theme-refresh preserves the applied filter (#1925)'); + await browser.close(); console.log('\nPASS: #1758 neighbor-graph filter re-render lifecycle holds'); From 5c93ebfb5e182208d529dad86b6509c7382aec2d Mon Sep 17 00:00:00 2001 From: efiten Date: Fri, 4 Sep 2026 15:03:08 +0200 Subject: [PATCH 15/26] fix(#1898, #1900): carry observer_id into replayed packets (#1957) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) (cherry picked from commit 9488e9c31cc9b5d6c4e07ca6fdfb8266f9a407b8) Co-Authored-By: Claude Opus 5 --- public/live.js | 8 ++++++- public/packets.js | 9 ++++++-- test-live-region-filter.js | 43 +++++++++++++++++++++++++++++++++++++- 3 files changed, 56 insertions(+), 4 deletions(-) diff --git a/public/live.js b/public/live.js index 266cf7342..536ecf80b 100644 --- a/public/live.js +++ b/public/live.js @@ -856,7 +856,13 @@ resolved_path: pkt.resolved_path, _ts: new Date(pkt.timestamp || pkt.created_at).getTime(), decoded: { header: { payloadTypeName: typeName }, payload: raw, path: { hops } }, - snr: pkt.snr, rssi: pkt.rssi, observer: pkt.observer_name + snr: pkt.snr, rssi: pkt.rssi, observer: pkt.observer_name, + // #1898: the region filter matches on observer_id (packetMatchesRegion, + // line ~85). Without it every replayed packet has observer_id undefined, + // so the filter skips them all and drops the whole group. observer_iata + // is carried too so obsIataBadgeHtml does not have to fall back to the + // roster map for replayed packets. + observer_id: pkt.observer_id, observer_iata: pkt.observer_iata }; } diff --git a/public/packets.js b/public/packets.js index ff8c80f5a..e1ae22d9d 100644 --- a/public/packets.js +++ b/public/packets.js @@ -3537,7 +3537,11 @@ id: o.id, hash: pkt.hash, raw: o.raw_hex || pkt.raw_hex, _ts: new Date(o.timestamp).getTime(), decoded: { header: { payloadTypeName: typeName }, payload: oDec, path: { hops: oPath } }, - snr: o.snr, rssi: o.rssi, observer: obsName(o.observer_id) + snr: o.snr, rssi: o.rssi, observer: obsName(o.observer_id), + // #1900: carry the id itself, not just the resolved name. The Live + // region filter matches on observer_id, so without it the replay + // silently renders nothing whenever a region is selected. + observer_id: o.observer_id, observer_iata: o.observer_iata }); } } else { @@ -3545,7 +3549,8 @@ id: pkt.id, hash: pkt.hash, raw: pkt.raw_hex, _ts: new Date(pkt.timestamp).getTime(), decoded: { header: { payloadTypeName: typeName }, payload: decoded, path: { hops: pathHops } }, - snr: pkt.snr, rssi: pkt.rssi, observer: obsName(pkt.observer_id) + snr: pkt.snr, rssi: pkt.rssi, observer: obsName(pkt.observer_id), + observer_id: pkt.observer_id, observer_iata: pkt.observer_iata }); } sessionStorage.setItem('replay-packet', JSON.stringify(replayPackets)); diff --git a/test-live-region-filter.js b/test-live-region-filter.js index cf914727e..5a5126368 100644 --- a/test-live-region-filter.js +++ b/test-live-region-filter.js @@ -14,7 +14,13 @@ function test(name, fn) { function makeSandbox() { const ctx = { - window: { addEventListener: () => {}, dispatchEvent: () => {}, devicePixelRatio: 1 }, + window: { + addEventListener: () => {}, dispatchEvent: () => {}, devicePixelRatio: 1, + // live.js:14 captures these at load time from packet-helpers.js, which + // this sandbox does not load. They must exist before live.js is evaluated. + getParsedDecoded: (p) => { try { return p && p.decoded_json ? JSON.parse(p.decoded_json) : {}; } catch (e) { return {}; } }, + getParsedPath: (p) => { try { return p && p.path_json ? JSON.parse(p.path_json) : []; } catch (e) { return []; } }, + }, document: { readyState: 'complete', createElement: () => ({ style:{}, classList:{add(){},remove(){},contains(){return false;}}, setAttribute(){}, addEventListener(){}, getContext: () => ({clearRect(){},fillRect(){},beginPath(){},arc(){},fill(){},scale(){},fillText(){}}) }), @@ -135,7 +141,42 @@ test('observer iata map can be updated and used by filter', () => { assert.strictEqual(fn([{observer_id:'newobs'}], { 'newobs': 'LAX' }, ['LAX']), true); }); +// --- #1898 / #1900: replayed packets must carry observer_id --- +// dbPacketToLive() used to return only observer (the resolved name), so every +// VCR-replayed packet had observer_id undefined. packetMatchesRegion skips a +// packet whose observer_id is null and returns false when none match, so with a +// region selected the replay rendered nothing at all, and the Replay button on +// packet detail silently no-opped. Both issues are the same omission. +const toLive = ctx.window._liveDbPacketToLive; +assert.ok(toLive, '_liveDbPacketToLive must be exposed'); + +test('#1898: dbPacketToLive carries observer_id through', () => { + const live = toLive({ id: 1, hash: 'h', timestamp: '2026-01-01T00:00:00Z', + observer_id: 'obs1', observer_name: 'Obs One' }); + assert.strictEqual(live.observer_id, 'obs1', + 'observer_id must survive; the region filter matches on it'); +}); + +test('#1898: a replayed packet survives an active region filter', () => { + const live = toLive({ id: 1, hash: 'h', timestamp: '2026-01-01T00:00:00Z', + observer_id: 'obs1', observer_name: 'Obs One' }); + assert.strictEqual(fn([live], { obs1: 'BRU' }, ['BRU']), true, + 'with observer_id carried the group matches and is rendered'); +}); + +test('#1898: dropping observer_id is what broke it (guards the regression)', () => { + assert.strictEqual(fn([{ observer: 'Obs One' }], { obs1: 'BRU' }, ['BRU']), false, + 'a packet with no observer_id is skipped, so the group is dropped'); +}); + +test('#1898: observer_iata is carried so the badge needs no roster lookup', () => { + const live = toLive({ id: 1, hash: 'h', timestamp: '2026-01-01T00:00:00Z', + observer_id: 'obs1', observer_iata: 'BRU' }); + assert.strictEqual(live.observer_iata, 'BRU'); +}); + console.log(`\n${'═'.repeat(40)}`); console.log(` live region filter tests: ${passed} passed, ${failed} failed`); + console.log(`${'═'.repeat(40)}\n`); if (failed > 0) process.exit(1); From 758aabeb83ae7431f30b3d0c0aefd9f04eeca421 Mon Sep 17 00:00:00 2001 From: efiten Date: Fri, 4 Sep 2026 15:15:16 +0200 Subject: [PATCH 16/26] 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-.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) (cherry picked from commit 56d6d4c722da6bddf3b6b176f2207b309fd3a8e4) Co-Authored-By: Claude Opus 5 --- cmd/server/hash_migrate.go | 17 +++++++++++++ cmd/server/hash_migrate_test.go | 44 +++++++++++++++++++++++++++++++++ 2 files changed, 61 insertions(+) diff --git a/cmd/server/hash_migrate.go b/cmd/server/hash_migrate.go index cd5929221..a85d58bc1 100644 --- a/cmd/server/hash_migrate.go +++ b/cmd/server/hash_migrate.go @@ -10,9 +10,23 @@ import ( // both the DB and the in-memory byHash index. The migration is idempotent: // once all hashes match the current formula it completes instantly. func migrateContentHashesAsync(store *PacketStore, batchSize int, yieldDuration time.Duration) { + // #1856: every DB failure below continues to the next batch, so the loop + // always reaches the deferred completion. Setting the flag unconditionally + // therefore reported success after migrating nothing, which is exactly what + // happens on the read-only handle the server holds since #1283: begin, + // prepare and commit all fail, every batch is skipped, and /api/stats then + // answers hashMigrationComplete: true. + failedBatches := 0 defer func() { if r := recover(); r != nil { log.Printf("[hash-migrate] panic recovered: %v", r) + failedBatches++ + } + if failedBatches > 0 { + log.Printf("[hash-migrate] INCOMPLETE: %d batch(es) could not be written; "+ + "hashMigrationComplete stays false. On a read-only DB handle this is expected "+ + "and the migration belongs in the ingestor (#1856).", failedBatches) + return } store.hashMigrationComplete.Store(true) }() @@ -57,12 +71,14 @@ func migrateContentHashesAsync(store *PacketStore, batchSize int, yieldDuration dbTx, err := store.db.conn.Begin() if err != nil { log.Printf("[hash-migrate] begin tx: %v", err) + failedBatches++ continue } stmt, err := dbTx.Prepare("UPDATE transmissions SET hash = ? WHERE id = ?") if err != nil { log.Printf("[hash-migrate] prepare: %v", err) dbTx.Rollback() + failedBatches++ continue } @@ -83,6 +99,7 @@ func migrateContentHashesAsync(store *PacketStore, batchSize int, yieldDuration if err := dbTx.Commit(); err != nil { log.Printf("[hash-migrate] commit: %v", err) + failedBatches++ continue } diff --git a/cmd/server/hash_migrate_test.go b/cmd/server/hash_migrate_test.go index f1c816de9..0cc7f21e6 100644 --- a/cmd/server/hash_migrate_test.go +++ b/cmd/server/hash_migrate_test.go @@ -76,3 +76,47 @@ func TestMigrateContentHashesAsync_NoOp(t *testing.T) { t.Error("hash should remain in index") } } + +// #1856: a migration that could not write anything must not report completion. +// +// In production the server holds a mode=ro handle (#1283), so Begin, Prepare and +// Commit all fail, every batch takes a `continue`, and the loop reaches the +// deferred completion having migrated nothing. Before this fix the flag was set +// unconditionally there, so /api/stats answered hashMigrationComplete: true +// after doing no work at all. +// +// Closing the handle stands in for the read-only one: it is deterministic and it +// exercises the identical failure path (Begin returns an error, batch skipped). +func TestMigrateContentHashesAsyncDoesNotClaimCompletionWhenWritesFail(t *testing.T) { + db := setupTestDBv2(t) + store := NewPacketStore(db, nil) + + rawHex := "0A00D69FD7A5A7475DB07337749AE61FA53A4788E976" + wrongHash := "deadbeef12345678" + if _, err := db.conn.Exec(`INSERT INTO transmissions (raw_hex, hash, first_seen, route_type, payload_type) + VALUES (?, ?, datetime('now'), 0, 2)`, rawHex, wrongHash); err != nil { + t.Fatal(err) + } + if err := store.Load(); err != nil { + t.Fatal(err) + } + if store.byHash[wrongHash] == nil { + t.Fatal("expected packet under the wrong hash before migration") + } + + // Make every write fail, the way a read-only handle does in production. + if err := db.conn.Close(); err != nil { + t.Fatal(err) + } + + migrateContentHashesAsync(store, 100, time.Millisecond) + + if store.hashMigrationComplete.Load() { + t.Error("hashMigrationComplete must stay false when no batch could be written; " + + "reporting true here is what #1856 called self-reported success") + } + if store.byHash[wrongHash] == nil { + t.Error("the in-memory index must be left alone when the DB write failed, " + + "otherwise memory and disk disagree") + } +} From 2a745f4c92cf5a020b9de0b7c442e3d42b2179e5 Mon Sep 17 00:00:00 2001 From: efiten Date: Fri, 4 Sep 2026 17:05:01 +0200 Subject: [PATCH 17/26] fix(#1896): say that the naive-clock notice clears itself (#1962) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #1896. The banner told operators their clock is naive but not that the notice goes away on its own, so people went looking for #1480 to find out. One sentence: > Clock is naive β€” per-packet timing clamped to ingest time. **Clears itself 24h after the last skew event.** ## Verified before writing it into the UI The issue asserts the 24h self-clear. Rather than repeat that, I checked it: - `cmd/server/observer_naive_clock.go:8` β€” `const observerNaiveClockWindow = 24 * time.Hour` - `applyObserverNaiveClock` applies the decay at read time and leaves `clock_naive` false once the last event is older than the window - its own comment: *"any event older than observerNaiveClockWindow is treated as absent so the chip and banner clear automatically without a background sweep"* So "24h after the last skew event" is accurate, including the fact that it needs no sweep and no restart. No test pinned the old string. Co-authored-by: Claude Opus 5 (1M context) (cherry picked from commit 83134d6e7022bb1575d04c2b6809514a614ded5c) Co-Authored-By: Claude Opus 5 --- public/observers.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/public/observers.js b/public/observers.js index ceeb270e1..54c9a90f1 100644 --- a/public/observers.js +++ b/public/observers.js @@ -538,7 +538,7 @@ window.preserveCompareSelection = function preserveCompareSelection(prevIds, tbo var content = window.SlideOver.open({ title: (o.name || o.id) }); var naiveChipHTML = window.ObserversNaiveChip.render(o); content.innerHTML = - (naiveChipHTML ? '
' + naiveChipHTML + ' Clock is naive β€” per-packet timing clamped to ingest time.
' : '') + + (naiveChipHTML ? '
' + naiveChipHTML + ' Clock is naive β€” per-packet timing clamped to ingest time. Clears itself 24h after the last skew event.
' : '') + '
' + '
Status
' + h.label + '
' + '
Region
' + (o.iata ? '' + o.iata + '' : 'β€”') + '
' + From 0500c0cd3bc03388305e6e435782c711bd457bc7 Mon Sep 17 00:00:00 2001 From: n30nex Date: Sun, 6 Sep 2026 15:02:14 -0400 Subject: [PATCH 18/26] fix(analytics): preserve selected hash size in links (#1967) Red commit: `5a5ecb6` (local browser: 14 passed, 8 behavior assertion failures before the fix). Hash Issues links now restore `bytes=1|2|3` for the selected control and its matrix/collision data. Missing or malformed values default to one byte. Selector clicks, section/top links, tab-bar changes, filters and theme refreshes retain the chosen view through the existing URL helper. Fixes #1914. - E2E assertion added: `test-issue-1306-collisions-terminology-e2e.js:242`. The existing CI-selected harness passes 23 checks, including distinct nonempty collision rows for each byte size. Its original assertions remain. - Browser verified: `http://127.0.0.1:55634` with the local fixture API, plus reviewed matrix/risk screenshots. Region refresh passed; area coverage skips because the fixture has no areas. - Required frontend checks pass: 99 filter, 18 aging, 666 helpers; URL helpers pass 18. Three independent reviews found no blocking issues; their coverage suggestion is included in `fb482fe`. - Added work parses URL state and updates six links. Rendering and bulk requests are reused; no backend, configuration, dependency or CI-list changes. - A broader smoke run timed out at Live autocomplete (#1110); full-suite success is not established. ## Preflight overrides - The external `run-all.sh` is absent. Corresponding scope, PII, syntax, whitespace and CSS checks passed; no SQL, migration or image changes require those gates. - Red browser evidence is local. Upstream CI execution remains a separate approval gate, as discussed in #1922. (cherry picked from commit eb1d733998ac9c2f3254b7a309a67593c78d66d9) Co-Authored-By: Claude Opus 5 --- public/analytics.js | 26 ++- test-issue-1306-collisions-terminology-e2e.js | 164 +++++++++++++++++- 2 files changed, 179 insertions(+), 11 deletions(-) diff --git a/public/analytics.js b/public/analytics.js index 74ad68fa1..4139f49ed 100644 --- a/public/analytics.js +++ b/public/analytics.js @@ -1527,18 +1527,18 @@ async function renderCollisionTab(el, data, collisionData) { el.innerHTML = `

Collisions actually observed in packet traffic β€” among repeaters grouped by their configured hash size. For theoretical address conflicts that would occur if all repeaters used a given hash size, see the Prefix Tool tab.

-

Inconsistent Hash Sizes

↑ top
+

Inconsistent Hash Sizes

↑ top

Repeaters and room servers sending adverts with varying hash sizes in the last 7 days. Originally caused by a firmware bug where automatic adverts ignored the configured multibyte path setting, fixed in repeater v1.14.1. Companion nodes are excluded.

Loading…
@@ -1546,7 +1546,7 @@

Hash Usage Matrix

- ↑ top + ↑ top
@@ -1560,7 +1560,7 @@
-

Collision Risk

↑ top
+

Collision Risk

↑ top
Loading…
`; @@ -1597,9 +1597,15 @@ // Repeaters and routing nodes no longer needed β€” collision data is server-computed - let currentBytes = 1; function refreshHashViews(bytes) { - currentBytes = bytes; + // #1914: keep both the bookmark and section links on this byte size. + if (window.URLState) { + const newHash = URLState.updateHashParams({ bytes }, location.hash); + if (newHash !== location.hash) history.replaceState(null, '', newHash); + el.querySelectorAll('[data-hash-section]').forEach(link => { + link.href = URLState.updateHashParams({ section: link.dataset.hashSection }, newHash); + }); + } hideMatrixTip(); // Update selector button states document.querySelectorAll('.hash-byte-btn').forEach(b => { @@ -1628,7 +1634,9 @@ btn.addEventListener('click', () => refreshHashViews(Number(btn.dataset.bytes))); }); - refreshHashViews(1); + // Read on every render so tab, filter and theme refreshes retain the view. + const urlBytes = new URLSearchParams(location.hash.split('?')[1] || '').get('bytes'); + refreshHashViews(['1', '2', '3'].includes(urlBytes) ? Number(urlBytes) : 1); } function renderHashTimeline(hourly) { diff --git a/test-issue-1306-collisions-terminology-e2e.js b/test-issue-1306-collisions-terminology-e2e.js index 1905b25f3..688ce2aca 100644 --- a/test-issue-1306-collisions-terminology-e2e.js +++ b/test-issue-1306-collisions-terminology-e2e.js @@ -15,6 +15,8 @@ * 3. The Collisions (Hash Issues) tab body contains the reverse * cross-reference link back to `#/analytics?tab=prefix-tool` * framed around "actually observed" / packet traffic wording. + * 4. #1914: shared Hash Issues links restore the selected byte size and + * its server data; selection survives navigation and refreshes. * * Usage: BASE_URL=http://localhost:13581 node test-issue-1306-collisions-terminology-e2e.js */ @@ -32,6 +34,7 @@ function assert(c, m) { if (!c) throw new Error(m || 'assertion failed'); } async function openPrefixTool(page) { await page.goto(BASE + '/#/analytics?tab=prefix-tool', { waitUntil: 'domcontentloaded' }); + await page.waitForFunction(() => window.__collisionsThemeReady); await page.waitForSelector('#ptOverview', { timeout: 15000 }); // expand the overview body (collapsed by default) await page.evaluate(() => { @@ -40,9 +43,45 @@ async function openPrefixTool(page) { }); } -async function openCollisions(page) { - await page.goto(BASE + '/#/analytics?tab=collisions', { waitUntil: 'domcontentloaded' }); +async function openCollisions(page, query = '') { + await page.goto('about:blank'); + await page.goto(BASE + '/#/analytics?tab=collisions' + query, { waitUntil: 'domcontentloaded' }); await page.waitForSelector('#hashMatrixSection', { timeout: 15000 }); + await page.waitForFunction(() => document.querySelector('#hashMatrix .analytics-stat-value')); +} + +function hashParams(page) { + return new URLSearchParams(new URL(page.url()).hash.split('?')[1] || ''); +} + +async function refreshCollisions(page, action) { + const previous = await page.locator('#hashMatrix').elementHandle(); + await action(); + await page.waitForFunction(el => !el.isConnected, previous); + await previous.dispose(); +} + +async function assertHashView(page, bytes, collisionData) { + await page.waitForFunction(() => window.__collisionsThemeReady); + await page.waitForFunction(() => document.querySelector('#hashMatrix .analytics-stat-value')); + const selected = page.locator('#hashByteSelector .hash-byte-btn.active'); + assert(await selected.count() === 1, 'Exactly one byte-size button should be selected'); + const actual = await selected.getAttribute('data-bytes'); + assert(actual === String(bytes), `Expected ${bytes}-byte selection, got ${actual}`); + const riskTitle = await page.locator('#collisionRiskTitle').textContent(); + assert(riskTitle.includes(bytes + '-Byte Collision Risk'), 'Risk title does not match selected byte size'); + const usingCard = page.locator('#hashMatrix .analytics-stat-card').nth(1); + assert((await usingCard.textContent()).includes('Using ' + bytes + '-byte ID'), 'Matrix uses the wrong byte-size data'); + const expected = collisionData.by_size[String(bytes)]; + assert(!!expected, 'Fixture must include ' + bytes + '-byte collision data'); + const usingCount = Number((await usingCard.locator('.analytics-stat-value').textContent()).replace(/,/g, '')); + assert(usingCount === (expected.stats.using_this_size || 0), 'Matrix count differs from the selected server data'); + assert(await page.locator('#hashMatrix .hash-matrix-table').count() === (bytes === 3 ? 0 : 1), 'Expected a grid only for 1-byte and 2-byte views'); + const prefixes = await page.locator('#collisionList tbody tr > td:first-child').allTextContents(); + assert(JSON.stringify(prefixes) === JSON.stringify((expected.collisions || []).map(c => c.prefix)), 'Collision risk rows differ from the selected server data'); + if (!prefixes.length) { + assert((await page.locator('#collisionList').textContent()).includes('No ' + bytes + '-byte'), 'Empty collision result should name the selected byte size'); + } } (async () => { @@ -64,6 +103,11 @@ async function openCollisions(page) { } const ctx = await browser.newContext({ viewport: { width: 1400, height: 1200 } }); + // Initial theme loading rebuilds analytics once; wait for it before + // interacting with an expanded panel or asserting the restored view. + await ctx.addInitScript(() => { + window.addEventListener('theme-refresh', () => { window.__collisionsThemeReady = true; }, { once: true }); + }); const page = await ctx.newPage(); page.setDefaultTimeout(15000); page.on('pageerror', (e) => console.error('[pageerror]', e.message)); @@ -108,6 +152,122 @@ async function openCollisions(page) { assert(xref >= 1, 'Collisions tab missing cross-reference link to #/analytics?tab=prefix-tool'); }); + const collisionResponse = await page.request.get(BASE + '/api/analytics/hash-collisions'); + assert(collisionResponse.ok(), 'Local collision API must respond successfully'); + const collisionData = await collisionResponse.json(); + + for (const section of ['hashMatrixSection', 'collisionRiskSection']) { + for (const bytes of [1, 2, 3]) { + await step(`#1914: ${bytes}-byte ${section} deep link restores selection and data`, async () => { + await openCollisions(page, '§ion=' + section + '&bytes=' + bytes); + await assertHashView(page, bytes, collisionData); + assert(hashParams(page).get('section') === section, 'Deep link must retain the target section'); + }); + } + } + + await step('#1914: byte clicks update a reloadable URL without losing other parameters', async () => { + await openCollisions(page, '§ion=collisionRiskSection&window=7d&filter=kept'); + for (const bytes of [2, 3, 1]) { + await page.locator(`.hash-byte-btn[data-bytes="${bytes}"]`).click(); + const params = hashParams(page); + assert(params.get('bytes') === String(bytes), `Byte click should write bytes=${bytes}, got ${params.get('bytes')}`); + assert(params.get('section') === 'collisionRiskSection' && params.get('window') === '7d' && params.get('filter') === 'kept', 'Byte click must preserve other parameters'); + await page.reload({ waitUntil: 'domcontentloaded' }); + await assertHashView(page, bytes, collisionData); + } + }); + + await step('#1914: section and top links preserve byte size and other parameters', async () => { + await openCollisions(page, '&bytes=3&window=7d&filter=kept'); + for (const section of ['hashMatrixSection', 'collisionRiskSection', 'inconsistentHashSection']) { + const link = page.locator(`#hashIssuesToc a[href*="section=${section}"]`); + const params = new URLSearchParams((await link.getAttribute('href')).split('?')[1]); + assert(params.get('bytes') === '3' && params.get('window') === '7d' && params.get('filter') === 'kept', 'Section href must carry the selected view for copying or opening'); + await link.click(); + await assertHashView(page, 3, collisionData); + assert(hashParams(page).get('section') === section, 'Section click must update its URL target'); + } + await page.locator('#collisionRiskSection a', { hasText: 'top' }).click(); + await assertHashView(page, 3, collisionData); + assert(!hashParams(page).has('section') && hashParams(page).get('bytes') === '3', 'Top link should clear only the section'); + }); + + await step('#1914: time-window, tab and theme refreshes retain the chosen byte size', async () => { + await openCollisions(page); + await page.locator('.hash-byte-btn[data-bytes="2"]').click(); + await refreshCollisions(page, () => page.locator('#analyticsTimeWindow').selectOption('24h')); + await page.waitForFunction(() => new URLSearchParams(location.hash.split('?')[1]).get('window') === '24h'); + await assertHashView(page, 2, collisionData); + await page.locator('#analyticsTabs [data-tab="hashsizes"]').click(); + await page.locator('#analyticsTabs [data-tab="collisions"]').click(); + await assertHashView(page, 2, collisionData); + await page.evaluate(() => window.dispatchEvent(new Event('theme-refresh'))); + await assertHashView(page, 2, collisionData); + assert(hashParams(page).get('bytes') === '2' && hashParams(page).get('window') === '24h', 'Refreshes must preserve the view URL'); + }); + + for (const value of [null, '', '0', '4', '-1', '2.5', '2bytes', '02', 'NaN']) { + await step(`#1914: ${value === null ? 'missing' : JSON.stringify(value)} byte size safely defaults to 1`, async () => { + await openCollisions(page, value === null ? '' : '&bytes=' + encodeURIComponent(value)); + await assertHashView(page, 1, collisionData); + }); + } + + for (const filter of ['region', 'area']) { + await openCollisions(page, '&bytes=3'); + const container = page.locator(filter === 'region' ? '#analyticsRegionFilter' : '#analyticsAreaFilter'); + const option = container.locator(`[data-${filter}]:not([data-${filter}="__all__"])`).first(); + const config = await (await page.request.get(BASE + '/api/config/' + filter + 's')).json(); + if (filter === 'region' ? Object.keys(config).length < 2 : config.length === 0) { + console.log(` #1914 ${filter} refresh: SKIP (local fixture has no selectable ${filter} filter)`); + continue; + } + await step(`#1914: ${filter} filter refresh retains byte size and selects its server data`, async () => { + await option.waitFor({ state: 'attached' }); + const selected = await option.getAttribute('data-' + filter); + const dropdown = container.locator('.region-dropdown-trigger'); + if (await dropdown.count()) await dropdown.click(); + await refreshCollisions(page, () => option.click()); + const response = await page.request.get(BASE + '/api/analytics/hash-collisions?' + filter + '=' + encodeURIComponent(selected)); + assert(response.ok(), 'Filtered collision API must respond successfully'); + await assertHashView(page, 3, await response.json()); + assert(hashParams(page).get('bytes') === '3', 'Filter refresh must preserve the byte-size URL'); + if (await dropdown.count()) await dropdown.click(); + await refreshCollisions(page, () => container.locator(`[data-${filter}="__all__"]`).click()); + await assertHashView(page, 3, collisionData); + }); + } + + await step('#1914: nonempty risk rows follow deep-linked and clicked byte sizes', async () => { + const prefixesBySize = { 1: ['A1'], 2: ['B2C3', 'B2D4'], 3: ['D4E5F6'] }; + const routePattern = '**/api/analytics/hash-collisions*'; + await page.route(routePattern, async route => { + const response = await route.fetch(); + const json = await response.json(); + // Keep the real matrix data, but distinguish each risk list even + // when the local fixture has no observed collisions. + for (const [bytes, prefixes] of Object.entries(prefixesBySize)) { + json.by_size[bytes].collisions = prefixes.map(prefix => ({ + prefix, appearances: 1, with_coords: 0, classification: 'unknown', nodes: [] + })); + } + await route.fulfill({ response, json }); + }); + try { + await openCollisions(page, '§ion=collisionRiskSection&bytes=2'); + await page.waitForFunction(() => window.__collisionsThemeReady); + for (const bytes of [2, 3, 1]) { + if (bytes !== 2) await page.locator(`.hash-byte-btn[data-bytes="${bytes}"]`).click(); + const prefixes = await page.locator('#collisionList tbody tr > td:first-child').allTextContents(); + assert(JSON.stringify(prefixes) === JSON.stringify(prefixesBySize[bytes]), + `Expected ${bytes}-byte risk prefixes ${prefixesBySize[bytes]}, got ${prefixes}`); + } + } finally { + await page.unroute(routePattern); + } + }); + await browser.close(); console.log('\n' + passed + ' passed, ' + failed + ' failed'); From 224e27198d61cf8acc26840a8969c6e8e4419925 Mon Sep 17 00:00:00 2001 From: n30nex Date: Sun, 6 Sep 2026 15:03:58 -0400 Subject: [PATCH 19/26] fix(nodes): remove misleading aggregate SNR headlines (#1969) Red commit: `2bf9be8` (local Chromium: 3 passed, 3 intended assertion failures). CI: [run](https://github.com/Kpa-clawbot/CoreScope/actions/runs/33988112224) awaits maintainer approval (`action_required`); 0 jobs started. Remove the unqualified aggregate Avg SNR row from node side-panel Overview and full-detail stats, following option 3 in #1149. Heard By retains each observer's SNR reading. Fixes #1149. - E2E assertion added: `test-issue-1281-location-row-e2e.js:224`. Three new browser cases cover desktop side/full and mobile full views with a numeric aggregate and distinct positive/negative observer readings. Existing packet-location assertions remain intact. - Browser verified: local Chromium; 6 cases passed after push. Screenshots: `coverage/issue-1149/issue-1149-desktop-side-panel.png`, `coverage/issue-1149/issue-1149-desktop-full-detail.png`, and `coverage/issue-1149/issue-1149-mobile-full-detail.png`. - Validation: packet filter 99/99, aging 18/18, frontend helpers 666/666; XSS, CSS-variable, syntax, whitespace and PII checks passed. - Independent reviews: adversarial, lifecycle expert and TDD reviewers found no required changes. One initial browser navigation timed out; the unchanged parent rerun passed 6/6. - Performance/config: two production row deletions; no new requests, loops, timers, settings or customizer implications. Backend unchanged; Go suites were not rerun. Fix commit: `d7c68f3`. ## Preflight overrides - External `run-all.sh` is unavailable on this host. Scoped branch, red/green, PII, CSS, XSS and whitespace checks were run directly. The diff adds no migrations, SQL attribution or image markup. (cherry picked from commit 108ea020f74a9d0a460b3e426ef0d20d0e9f0de9) Co-Authored-By: Claude Opus 5 --- public/nodes.js | 2 - test-issue-1281-location-row-e2e.js | 79 +++++++++++++++++++++++++++++ 2 files changed, 79 insertions(+), 2 deletions(-) diff --git a/public/nodes.js b/public/nodes.js index db73c94b4..ff0ed6602 100644 --- a/public/nodes.js +++ b/public/nodes.js @@ -786,7 +786,6 @@ First Seen${renderNodeTimestampHtml(n.first_seen)} Total Packets${stats.totalTransmissions || stats.totalPackets || n.advert_count || 0}${stats.totalObservations && stats.totalObservations !== (stats.totalTransmissions || stats.totalPackets) ? ' (seen ' + stats.totalObservations + 'Γ—)' : ''} Packets Today${stats.packetsToday || 0} - ${stats.avgSnr != null ? `Avg SNR${Number(stats.avgSnr).toFixed(1)} dB` : ''} ${stats.avgHops ? `Avg Hops${stats.avgHops}` : ''} ${hasLoc ? `Location${Number(n.lat).toFixed(5)}, ${Number(n.lon).toFixed(5)}` : ''} ${hasEstLoc ? `${hasLoc ? 'Neighbor Estimate' : 'Location'} (estimated)~${Number(n.estimated_lat).toFixed(5)}, ~${Number(n.estimated_lon).toFixed(5)} (from ${n.estimated_contributor_count} neighbor${n.estimated_contributor_count === 1 ? '' : 's'}${hasLoc ? ', ' + Number(n.estimated_distance_km).toFixed(1) + ' km from reported position' : ', no real GPS fix'})` : ''} @@ -1838,7 +1837,6 @@
First Seen
${renderNodeTimestampHtml(n.first_seen)}
Total Packets
${totalPackets}
Packets Today
${stats.packetsToday || 0}
- ${stats.avgSnr != null ? `
Avg SNR
${Number(stats.avgSnr).toFixed(1)} dB
` : ''} ${stats.avgHops ? `
Avg Hops
${stats.avgHops}
` : ''} ${hasLoc ? `
Location
${Number(n.lat).toFixed(5)}, ${Number(n.lon).toFixed(5)}
` : ''} ${hasEstLoc ? `
${hasLoc ? 'Neighbor Estimate' : 'Location'} (estimated)
~${Number(n.estimated_lat).toFixed(5)}, ~${Number(n.estimated_lon).toFixed(5)} (from ${n.estimated_contributor_count} neighbor${n.estimated_contributor_count === 1 ? '' : 's'}${hasLoc ? ', ' + Number(n.estimated_distance_km).toFixed(1) + ' km from reported position' : ', no real GPS fix'})
` : ''} diff --git a/test-issue-1281-location-row-e2e.js b/test-issue-1281-location-row-e2e.js index a92be9a4d..ffde5cc6f 100644 --- a/test-issue-1281-location-row-e2e.js +++ b/test-issue-1281-location-row-e2e.js @@ -14,6 +14,8 @@ * 3. The πŸ“map link uses class="loc-map-link" with a themed link color * (was --accent pre-M5; now --link-color after #1668/PR #1696 AA-contrast * fix). The hard guarantee is: NOT the default UA blue rgb(0,0,238). + * 4. #1149: node side/full stats omit aggregate SNR, retaining Heard By + * readings on desktop and mobile. Node API fixtures keep SNR non-null. * * Usage: BASE_URL=http://localhost:13581 node test-issue-1281-location-row-e2e.js */ @@ -157,6 +159,83 @@ async function findPacketDetailByType(page, predicate, maxRows = 40) { 'Link color is UA-default blue β€” class is missing or CSS rule does not match'); }); + const pubkey = 'ab'.repeat(32); + const now = new Date().toISOString(); + const node = { + public_key: pubkey, name: 'SNR fixture node', role: 'repeater', + first_seen: now, last_seen: now, advert_count: 1005, + }; + const health = { + stats: { totalPackets: 1005, packetsToday: 7, avgSnr: 6.3, avgHops: 2, lastHeard: now }, + observers: [ + { observer_id: 'observer-a', observer_name: 'Observer A', packetCount: 1000, avgSnr: 12.5, avgRssi: -45 }, + { observer_id: 'observer-b', observer_name: 'Observer B', packetCount: 5, avgSnr: -8.5, avgRssi: -110 }, + ], + recentPackets: [], + }; + + for (const view of [ + { name: 'desktop side panel', width: 1280, full: false }, + { name: 'desktop full detail', width: 1280, full: true }, + { name: 'mobile full detail', width: 390, full: true }, + ]) { + await step('#1149 ' + view.name + ' retains observer SNR without an aggregate headline', async () => { + const nodePage = await browser.newPage({ viewport: { width: view.width, height: 900 } }); + try { + await nodePage.route('**/api/nodes**', async (route) => { + const pathname = new URL(route.request().url()).pathname; + const fixtures = { + '/api/nodes': { nodes: [node], total: 1, counts: { repeaters: 1 } }, + '/api/nodes/clock-skew': [], + ['/api/nodes/' + pubkey]: { node, recentAdverts: [] }, + ['/api/nodes/' + pubkey + '/health']: health, + ['/api/nodes/' + pubkey + '/neighbors']: { neighbors: [] }, + ['/api/nodes/' + pubkey + '/paths']: { paths: [] }, + ['/api/nodes/' + pubkey + '/clock-skew']: {}, + }; + if (Object.prototype.hasOwnProperty.call(fixtures, pathname)) { + await route.fulfill({ json: fixtures[pathname] }); + } else { + await route.continue(); + } + }); + await nodePage.goto(BASE + '/#/nodes', { waitUntil: 'domcontentloaded' }); + await nodePage.locator('#nodesBody tr[data-key="' + pubkey + '"]').click(); + if (view.full && view.width > 640) { + await nodePage.locator('#nodesRight .node-detail-btn').click(); + } + + const stats = nodePage.locator(view.full ? '#node-stats' : '#nodesRight .detail-meta'); + const readings = nodePage.locator(view.full ? '#node-observers tbody tr' : '#nodesRight .observer-row'); + await readings.first().waitFor({ state: 'visible' }); + assert(await readings.count() === 2, 'Heard By must retain both observers'); + for (const observer of health.observers) { + const row = readings.filter({ hasText: observer.observer_name }); + const text = (await row.innerText()).replace(/\s+/g, ' '); + const snr = observer.avgSnr.toFixed(1) + (view.full ? ' dB' : 'dB'); + assert(text.includes(snr), observer.observer_name + ' must retain SNR ' + snr + ', got: ' + text); + } + for (const [label, value] of [['Total Packets', '1005'], ['Packets Today', '7'], ['Avg Hops', '2']]) { + const metric = stats.getByText(label, { exact: true }); + const actual = await metric.evaluate(el => el.nextElementSibling.textContent.trim()); + assert(actual === value, label + ' must remain ' + value + ', got: ' + actual); + } + const headlineCount = await stats.getByText('Avg SNR', { exact: true }).count(); + assert(headlineCount === 0, + 'Expected no aggregate Avg SNR headline with stats.avgSnr=6.3, got ' + headlineCount); + if (process.env.SCREENSHOT_DIR) { + await stats.scrollIntoViewIfNeeded(); + await nodePage.screenshot({ + path: require('path').join(process.env.SCREENSHOT_DIR, 'issue-1149-' + view.name.replace(/ /g, '-') + '.png'), + fullPage: true, + }); + } + } finally { + await nodePage.close(); + } + }); + } + await browser.close(); console.log(`\n${passed} passed, ${failed} failed`); From 6ca5964d6f43ee324269c7bc75a675702eaf2b29 Mon Sep 17 00:00:00 2001 From: n30nex Date: Sun, 6 Sep 2026 15:04:03 -0400 Subject: [PATCH 20/26] fix(nodes): clearly mark the selected node in path chains (#1968) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Red commit: `0988bc0` (local browser: 3 passed, 8 behavior assertion failures before the fix). The selected node now has a compact outline in long β€œPaths Through This Node” chains, in the side panel and full detail page. Matching uses complete public keys without case sensitivity; same-prefix siblings and unresolved hops stay unmarked. Existing links, escaped names, warnings and ambiguity underlines are preserved. Fixes #1153. Its prerequisite #1144 is already merged. - E2E assertion added: `test-issue-1146-path-link-contrast-e2e.js:220`. The existing CI-selected harness passes 11 checks across 18-hop paths, both themes, desktop/mobile, and the renderer fallback. Review follow-up `bd8118f` verifies the marked ambiguous hop's dashed underline. - Browser verified: `http://127.0.0.1:55635`; desktop/mobile path screenshots were inspected. The broader smoke runner exited successfully with fixture-dependent skips. - Required frontend checks pass: 99 filter, 18 aging, 666 helpers. CSS variables, seven CSS self-tests, 31 XSS sink checks, 17 XSS gate self-tests and XSS diff preflight pass. - Three independent reviews found no blocking issues. Traversal remains linear with no new requests, settings, dependencies or cache invalidation; styling uses the existing customizer token. ## Preflight overrides - The external preflight runner is absent; corresponding scoped gates passed. Red browser evidence is local, with upstream CI approval tracked separately under the process in #1922. - Existing rapid-navigation map resize timer errors remain visible in browser logs and are outside this change. (cherry picked from commit a938176f831f979ef0442594ab33286e2585724f) Co-Authored-By: Claude Opus 5 --- public/nodes.js | 10 +- public/style.css | 10 +- test-issue-1146-path-link-contrast-e2e.js | 145 ++++++++++++++++++++-- 3 files changed, 149 insertions(+), 16 deletions(-) diff --git a/public/nodes.js b/public/nodes.js index ff0ed6602..2b33921ae 100644 --- a/public/nodes.js +++ b/public/nodes.js @@ -1069,17 +1069,18 @@ } document.querySelector('#fullPathsSection h4').textContent = `Paths Through This Node (${pathData.totalPaths} unique, ${pathData.totalTransmissions} transmissions)`; const COLLAPSE_LIMIT = 10; + const currentPubkey = n.public_key.toLowerCase(); function renderPaths(paths) { return paths.map(p => { const chain = p.hops.map(h => { - const isThis = h.pubkey === n.public_key; + const isThis = h.pubkey && h.pubkey.toLowerCase() === currentPubkey; if (window.HopDisplay) { const entry = { name: h.name, pubkey: h.pubkey, ambiguous: h.ambiguous, conflicts: h.conflicts, totalGlobal: h.totalGlobal, totalRegional: h.totalRegional, globalFallback: h.globalFallback, unreliable: h.unreliable }; const html = HopDisplay.renderHop(h.prefix, entry); return isThis ? html.replace('class="', 'class="hop-current ') : html; } const name = escapeHtml(h.name || h.prefix); - const link = h.pubkey ? `${name}` : `${name}`; + const link = h.pubkey ? `${name}` : `${name}`; return link; }).join(' β†’ '); return `
@@ -1988,12 +1989,13 @@ document.querySelector('#pathsSection h4').textContent = `Paths Through This Node (${pathData.totalPaths} unique path${pathData.totalPaths !== 1 ? 's' : ''}, ${pathData.totalTransmissions} transmissions)`; const COLLAPSE_LIMIT = 10; const showAll = pathData.paths.length <= COLLAPSE_LIMIT; + const currentPubkey = n.public_key.toLowerCase(); function renderPaths(paths) { return paths.map(p => { const chain = p.hops.map(h => { - const isThis = h.pubkey === n.public_key; + const isThis = h.pubkey && h.pubkey.toLowerCase() === currentPubkey; const name = escapeHtml(h.name || h.prefix); - const link = h.pubkey ? `${name}` : `${name}`; + const link = h.pubkey ? `${name}` : `${name}`; return link; }).join(' β†’ '); return `
diff --git a/public/style.css b/public/style.css index 6642eb74d..4205b5331 100644 --- a/public/style.css +++ b/public/style.css @@ -3119,7 +3119,15 @@ button.ch-item.ch-item-encrypted .ch-badge { filter: grayscale(0.6); } .hop-unreliable-btn { background: none; border: none; color: var(--status-yellow, #f59e0b); font-size: 13px; cursor: help; vertical-align: middle; margin-left: 2px; padding: 0 2px; line-height: 1; } .hop-global-fallback { border-bottom: 1px dashed var(--status-red); } -.hop-current { font-weight: 700 !important; color: var(--link-color) !important; } +.hop-current { + display: inline-block; + padding: 0 4px; + /* Outline preserves the shared renderer's dashed ambiguity border. */ + outline: 1px solid var(--link-color); + border-radius: 4px; + font-weight: 700 !important; + color: var(--link-color) !important; +} /* Self-loop subpath rows */ .subpath-selfloop { opacity: 0.6; } diff --git a/test-issue-1146-path-link-contrast-e2e.js b/test-issue-1146-path-link-contrast-e2e.js index 77a465fa6..7c496611b 100644 --- a/test-issue-1146-path-link-contrast-e2e.js +++ b/test-issue-1146-path-link-contrast-e2e.js @@ -14,15 +14,21 @@ * theme, then asserts the computed link colour vs. its background * yields a contrast ratio β‰₯ 4.5:1. * - * Currently FAILS (link color resolves to rgb(0,0,238)). - * After the style.css fix it PASSES. + * #1153 also checks that the selected node has a visible enclosing marker + * in an 18-hop chain, without marking a different node with the same prefix. + * Covers the desktop side/full views and mobile full view in both themes. * * Usage: BASE_URL=http://localhost:13581 node test-issue-1146-path-link-contrast-e2e.js + * Optional SCREENSHOT_DIR saves PNG evidence; use an ignored output directory. */ 'use strict'; const { chromium } = require('playwright'); const BASE = process.env.BASE_URL || 'http://localhost:13581'; +assert(['localhost', '127.0.0.1', '[::1]'].includes(new URL(BASE).hostname), + 'Browser regression tests must use a local server'); +const SCREENSHOT_DIR = process.env.SCREENSHOT_DIR; +if (SCREENSHOT_DIR) require('fs').mkdirSync(SCREENSHOT_DIR, { recursive: true }); let passed = 0, failed = 0; async function step(name, fn) { @@ -83,20 +89,41 @@ async function effectiveBgFor(page, selector) { console.log(`\n=== #1146 path-link contrast E2E against ${BASE} ===`); - const hopPubkey = 'a1b2c3d4e5f60718293a4b5c6d7e8f9001122334455667788990aabbccddeeff'; + const hopPubkey = 'a1'.repeat(32); + const targetName = 'Selected & relay'; + let selectedPubkey; + let targetHopKey; + let siblingKey; // Mock paths API for ANY node so test is deterministic. await page.route('**/api/nodes/*/paths*', (route) => { - route.fulfill({ + selectedPubkey = decodeURIComponent(new URL(route.request().url()).pathname.split('/')[3]); + // Full public-key identity must survive hex casing; a shared prefix alone + // must never qualify. Neither fixture node requires resolver changes. + targetHopKey = selectedPubkey === selectedPubkey.toUpperCase() + ? selectedPubkey.toLowerCase() : selectedPubkey.toUpperCase(); + siblingKey = targetHopKey.slice(0, -1) + (targetHopKey.endsWith('0') ? '1' : '0'); + const hops = Array.from({ length: 18 }, (_, i) => ({ + pubkey: hopPubkey.slice(0, -2) + i.toString(16).padStart(2, '0'), + prefix: 'a1', name: 'Relay ' + (i + 1), + })); + hops[8] = { pubkey: targetHopKey, prefix: targetHopKey.slice(0, 2), name: targetName, unreliable: true, ambiguous: true }; + hops[9] = { + pubkey: siblingKey, prefix: targetHopKey.slice(0, 2), name: 'Same-prefix sibling', ambiguous: true, + conflicts: [ + { pubkey: targetHopKey, name: targetName, regional: true }, + { pubkey: siblingKey, name: 'Same-prefix sibling', regional: true }, + ], + }; + hops[10] = { prefix: targetHopKey.slice(0, 2), name: 'Unresolved prefix' }; + return route.fulfill({ status: 200, contentType: 'application/json', body: JSON.stringify({ totalPaths: 1, totalTransmissions: 5, paths: [{ - hops: [ - { pubkey: hopPubkey, prefix: 'a1', name: 'TestHop' }, - ], + hops, count: 5, lastSeen: new Date().toISOString(), sampleHash: 'deadbeef00', @@ -107,10 +134,10 @@ async function effectiveBgFor(page, selector) { await step('Load nodes page and force dark theme', async () => { await page.goto(BASE + '/#/nodes', { waitUntil: 'domcontentloaded' }); - await page.evaluate(() => { - document.documentElement.setAttribute('data-theme', 'dark'); - }); await page.waitForSelector('#nodesBody tr[data-key]', { timeout: 15000 }); + if (await page.locator('html').getAttribute('data-theme') !== 'dark') { + await page.locator('#darkModeToggle').click(); + } }); await step('Open side panel for first node and wait for paths', async () => { @@ -121,7 +148,7 @@ async function effectiveBgFor(page, selector) { const el = document.getElementById('pathsContent'); return el && el.querySelector('a[href^="#/nodes/"]'); }, - { timeout: 15000 } + null, { timeout: 15000 } ); }); @@ -145,6 +172,102 @@ async function effectiveBgFor(page, selector) { `covered by the .data-table td a rule and inherit UA blue.`); }); + async function assertCurrentHop(selector, hasHopDisplay) { + await page.waitForFunction((sel) => document.querySelector(sel)?.textContent.includes('Relay 18'), selector); + const targetSelector = selector + ' a[href="#/nodes/' + targetHopKey + '"]'; + const result = await page.evaluate(({ selector, targetHopKey, siblingKey }) => { + const container = document.querySelector(selector); + const chain = container.firstElementChild.firstElementChild; + const target = chain.querySelector('a[href="#/nodes/' + targetHopKey + '"]'); + const sibling = chain.querySelector('a[href="#/nodes/' + siblingKey + '"]'); + target.scrollIntoView({ block: 'center' }); + const style = getComputedStyle(target); + const rect = target.getBoundingClientRect(); + const chainRect = chain.getBoundingClientRect(); + const outline = parseFloat(style.outlineWidth) >= 1 && style.outlineStyle === 'solid'; + const border = ['Top', 'Right', 'Bottom', 'Left'].every(side => + parseFloat(style['border' + side + 'Width']) >= 1 && style['border' + side + 'Style'] === 'solid'); + return { + marked: Array.from(chain.querySelectorAll('.hop-current')).map(el => el.getAttribute('href')), + ring: outline || border, ringColor: outline ? style.outlineColor : style.borderTopColor, + outline, dashedBottom: style.borderBottomStyle === 'dashed' && parseFloat(style.borderBottomWidth) >= 1, + textColor: style.color, padding: parseFloat(style.paddingLeft), + text: target.textContent, injectedElements: target.childElementCount, + siblingMarked: sibling.classList.contains('hop-current'), + fits: rect.left >= chainRect.left - 1 && rect.right <= chainRect.right + 1, + fragments: target.getClientRects().length, + warning: !!chain.querySelector('.hop-unreliable-btn'), + conflict: !!chain.querySelector('.hop-conflict-btn'), + siblingAmbiguous: sibling.classList.contains('hop-ambiguous'), + hopCount: chain.children.length - chain.querySelectorAll('button').length, + }; + }, { selector, targetHopKey, siblingKey }); + assert(result.hopCount === 18, 'Expected all 18 hops in the rendered chain'); + assert(result.ring, 'Selected hop must have a visible enclosing border or outline, beyond bold/color'); + assert(result.marked.length === 1 && result.marked[0] === '#/nodes/' + targetHopKey, + 'Exactly the selected full public key must be marked, including mixed-case hex'); + assert(!result.siblingMarked, 'Same-prefix sibling must not be marked as this node'); + assert(result.padding > 0 && result.fits && result.fragments === 1, + 'Selected hop marker must enclose the name and fit within the wrapping chain'); + assert(result.text === targetName && result.injectedElements === 0, 'Selected hop name must stay escaped'); + const bg = parseRgb(await effectiveBgFor(page, targetSelector)); + assert(contrast(parseRgb(result.ringColor), bg) >= 3, 'Selected hop ring must contrast with the active theme'); + assert(contrast(parseRgb(result.textColor), bg) >= 4.5, 'Selected hop text must remain readable'); + if (hasHopDisplay) { + assert(result.warning && result.conflict && result.siblingAmbiguous, + 'Shared hop rendering must preserve unreliable and ambiguous indicators'); + assert(result.outline && result.dashedBottom, + 'The selected ambiguous hop must keep its dashed bottom border alongside the enclosing outline'); + } + if (SCREENSHOT_DIR) { + const theme = await page.locator('html').getAttribute('data-theme'); + const filename = `${selector.slice(1)}-${page.viewportSize().width}-${theme}${hasHopDisplay ? '' : '-plain'}.png`; + await page.locator(selector).screenshot({ path: require('path').join(SCREENSHOT_DIR, filename) }); + } + } + + await step('#1153 desktop side panel: selected hop stands out in an 18-hop dark chain', + () => assertCurrentHop('#pathsContent', false)); + + for (const theme of ['light', 'dark']) { + if (await page.locator('html').getAttribute('data-theme') !== theme) { + await page.locator('#darkModeToggle').click(); + } + await step('#1153 desktop side panel: ' + theme + ' theme', async () => { + await page.goto(BASE + '/#/nodes', { waitUntil: 'domcontentloaded' }); + assert(await page.locator('html').getAttribute('data-theme') === theme, 'Expected the selected theme'); + await page.locator('#nodesBody tr[data-key]').first().click(); + await assertCurrentHop('#pathsContent', false); + }); + await step('#1153 desktop full view: ' + theme + ' theme', async () => { + await page.locator('#nodesRight .node-detail-btn').click(); + await assertCurrentHop('#fullPathsContent', true); + }); + } + + for (const theme of ['light', 'dark']) { + // The theme toggle lives in the desktop header; exercise the same theme + // at mobile width without depending on the unrelated navigation drawer. + await page.setViewportSize({ width: 1280, height: 900 }); + if (await page.locator('html').getAttribute('data-theme') !== theme) { + await page.locator('#darkModeToggle').click(); + } + await page.setViewportSize({ width: 390, height: 844 }); + await step('#1153 mobile row click opens a readable full chain: ' + theme + ' theme', async () => { + await page.goto(BASE + '/#/nodes', { waitUntil: 'domcontentloaded' }); + assert(await page.locator('html').getAttribute('data-theme') === theme, 'Expected the selected theme'); + await page.locator('#nodesBody tr[data-key]').first().click(); + await assertCurrentHop('#fullPathsContent', true); + }); + } + + await step('#1153 full-view fallback also marks only the selected node', async () => { + await page.route('**/hop-display.js*', route => route.fulfill({ contentType: 'text/javascript', body: '' })); + await page.reload({ waitUntil: 'domcontentloaded' }); + await assertCurrentHop('#fullPathsContent', false); + assert(await page.evaluate(() => !window.HopDisplay), 'Expected the real full-view fallback'); + }); + await browser.close(); console.log(`\n${passed} passed, ${failed} failed`); From c489d70b9e270c8ba626ab2946b21fc45204fb93 Mon Sep 17 00:00:00 2001 From: efiten Date: Sun, 6 Sep 2026 21:11:29 +0200 Subject: [PATCH 21/26] feat(#1794): per-IP limits and a deny list on the /ws upgrade (#1974) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #1794. Follow-up to #1793, decided **before** the upgrade because the handshake is the resource being protected. - Deny list of addresses and CIDRs β†’ 403 - Per-IP concurrent connection cap β†’ 403 - Per-IP upgrade rate limit over a rolling minute β†’ **429**, not 403: a temporary refusal should not read as "never come back" - Rejection counters split by cause in `/api/stats` under `websocket` ### The decision this feature lives or dies on Most CoreScope installs sit behind nginx, Caddy, Traefik or an ingress. `cdn_detection.go` says so in as many words: it deliberately excludes `X-Forwarded-For` from its CDN signals precisely because *every* reverse-proxied install sets it. For those deployments `r.RemoteAddr` is the proxy, `127.0.0.1` for every visitor on earth. A per-IP cap keyed on that address protects nobody and hands the sixth legitimate browser tab a 403. That is a self-inflicted outage wearing the costume of hardening. So: - **`X-Forwarded-For` is believed only from an address listed in `webSocket.trustedProxies`.** From anywhere else it is attacker-supplied, and trusting it would let anyone mint a fresh source IP per connection, which is strictly worse than having no limit at all. - **When the peer looks like a local reverse proxy and no `trustedProxies` is set, the per-IP limits are skipped**, and one warning names the setting that fixes it. Silently refusing real users is the worse failure. - **The deny list still applies there**, because it is the operator's explicit instruction rather than an inference. That is the answer to @mcode6726's question on the thread: it is neither "always the socket address" nor "always the header", and the operator decides which by naming their proxy. ### Two deliberate departures from the issue body **`maxConnsPerIP` ships as 0 (off), not 5.** Carrier-grade NAT puts thousands of unrelated mobile subscribers behind a single public IPv4. A cap of 5 refuses real visitors on phones while a scraper simply rents more addresses: all of the cost, none of the benefit. `upgradesPerMinPerIP` ships at **30 and on**, because that one *is* safe under CGNAT: a real client upgrades a handful of times per minute even while reconnecting, so 30 leaves ordinary traffic untouched while flattening a reconnect loop. A pointer type distinguishes "unset" from an explicit `0` that turns it off. **The default deny list is not shipped.** The thread proposed seeding 44 CIDRs for one VPS provider after a single scraper was seen at `23.111.177.6`. I have left it out: blanket-blocking a hosting provider by default breaks legitimate operators who host there, is undiscoverable by the person locked out (they see a bare 403), and ages badly as ranges get reassigned. The mechanism is here and `config.example.json` shows exactly how to configure it, so any operator who wants that list can have it in one line. If you want it shipped as a default anyway, that is your call as maintainer and it is a one-line change. ### Verification 19 tests, including all five the issue specifies as TDD requirements, each marked with the issue's own wording. Beyond those five: - a **bare address** in the deny list works, not just CIDR form. Operators write `1.2.3.4`, and silently ignoring that would be the worst possible failure for a deny list: it looks configured and blocks nothing - an unparseable deny entry is skipped and logged, not fatal. One typo must not take the server down - one client behind a trusted proxy does **not** exhaust another client's budget behind the same proxy, which is the entire point of honouring XFF - changing a forged XFF from an untrusted peer buys no fresh budget - `release` frees a slot and is **idempotent**, because `Unregister` can run twice for one client and double-crediting would leak slots - a **rejected** upgrade does not consume rate budget, or a retrying client could never recover once its window cleared - limits skipped for loopback and private peers; deny list applies anyway - a nil limiter allows everything, so a `Hub` built without `ConfigureLimits` behaves exactly as before - idle per-IP state is collected, while a record with a live connection never is Full `cmd/server` suite green, `gofmt` clean. ### Not done - No runtime config reload; restart required. Listed as optional in the issue. - No `WS_DENY_IPS` env override. Also listed as optional. - From the OWASP expansion in the first comment: `maxPayload` and the idle/read timeout are **already in master** (`SetReadLimit`, `SetReadDeadline`). The ping/pong heartbeat is not, and is not in this PR either; it is a separate change to the read/write pumps and belongs in its own review. Co-authored-by: Claude Opus 5 (1M context) (cherry picked from commit 1ffaad8eb1d98d8ac2bff48ff3f7f74a93abb175) Co-Authored-By: Claude Opus 5 --- cmd/server/config.go | 79 ++++++++++ cmd/server/main.go | 2 + cmd/server/routes.go | 7 +- cmd/server/types.go | 7 + cmd/server/websocket.go | 57 ++++++- cmd/server/ws_limits.go | 274 ++++++++++++++++++++++++++++++++ cmd/server/ws_limits_test.go | 296 +++++++++++++++++++++++++++++++++++ config.example.json | 10 ++ 8 files changed, 727 insertions(+), 5 deletions(-) create mode 100644 cmd/server/ws_limits.go create mode 100644 cmd/server/ws_limits_test.go diff --git a/cmd/server/config.go b/cmd/server/config.go index 40b463623..b25add72a 100644 --- a/cmd/server/config.go +++ b/cmd/server/config.go @@ -279,6 +279,10 @@ type Config struct { // so browsers enforce same-origin policy. Set to ["*"] to allow all origins. CORSAllowedOrigins []string `json:"corsAllowedOrigins,omitempty"` + // WebSocket carries the /ws transport limits from #1794. Omitted entirely + // means: rate limit at its default, connection cap off, no deny list. + WebSocket *WebSocketConfig `json:"webSocket,omitempty"` + DebugAffinity bool `json:"debugAffinity,omitempty"` // MapDarkTileProvider selects the default dark-mode basemap provider for @@ -619,6 +623,81 @@ func LoadConfig(baseDirs ...string) (*Config, error) { return cfg, nil // defaults } +// WebSocketConfig holds the /ws transport limits (#1794). These sit behind +// the CheckOrigin allowlist (#1793) and catch non-browser clients, which can +// omit or forge Origin. +type WebSocketConfig struct { + // MaxConnsPerIP caps concurrent /ws connections from one client address. + // DEFAULT 0, meaning OFF, and that is a deliberate departure from the + // value floated on #1794. + // + // A cap of 5 is safe only when one address means one household. It does + // not on mobile: carrier-grade NAT puts thousands of unrelated subscribers + // behind a single public IPv4, so a low cap would refuse real visitors on + // phones while barely inconveniencing a scraper that can rent more + // addresses. Operators who know their audience can set it; we must not + // pick it for them. + MaxConnsPerIP int `json:"maxConnsPerIP,omitempty"` + + // UpgradesPerMinPerIP caps handshakes per minute from one client address. + // DEFAULT 30, on. Unlike the connection cap this is safe under CGNAT: a + // real client upgrades a handful of times per minute even while + // reconnecting, so 30 leaves ordinary traffic untouched while flattening + // the reconnect loop that makes scrapers expensive. + UpgradesPerMinPerIP *int `json:"upgradesPerMinPerIP,omitempty"` + + // TrustedProxies lists the addresses or CIDRs of reverse proxies whose + // X-Forwarded-For may be believed. REQUIRED for the limits to do anything + // behind nginx/Caddy/Traefik/ingress: without it every visitor shares the + // proxy's address, so the limits are skipped and a warning is logged. + // Never list an address you do not control; anyone reaching the server + // from a trusted address can name any client IP they like. + TrustedProxies []string `json:"trustedProxies,omitempty"` + + // Deny blocks addresses outright at the upgrade, before the handshake. + // Accepts both bare addresses ("1.2.3.4") and CIDRs ("1.2.3.0/24"). + // Applies even when clients cannot be told apart, because it is an + // explicit instruction rather than an inference. + Deny []string `json:"deny,omitempty"` +} + +// WSMaxConnsPerIP returns the configured concurrent-connection cap, 0 = off. +func (c *Config) WSMaxConnsPerIP() int { + if c.WebSocket == nil || c.WebSocket.MaxConnsPerIP < 0 { + return 0 + } + return c.WebSocket.MaxConnsPerIP +} + +// WSUpgradesPerMinPerIP returns the upgrade-rate cap, defaulting to 30 when +// unset. A pointer distinguishes "not configured" (use the default) from an +// explicit 0, which an operator uses to turn the limit off. +func (c *Config) WSUpgradesPerMinPerIP() int { + if c.WebSocket == nil || c.WebSocket.UpgradesPerMinPerIP == nil { + return 30 + } + if v := *c.WebSocket.UpgradesPerMinPerIP; v > 0 { + return v + } + return 0 +} + +// WSTrustedProxies returns the configured reverse-proxy addresses, if any. +func (c *Config) WSTrustedProxies() []string { + if c.WebSocket == nil { + return nil + } + return c.WebSocket.TrustedProxies +} + +// WSDeny returns the configured deny entries, if any. +func (c *Config) WSDeny() []string { + if c.WebSocket == nil { + return nil + } + return c.WebSocket.Deny +} + func (c *Config) applyListLimitsDefaults() { if c.ListLimits == nil { c.ListLimits = &ListLimitsConfig{} diff --git a/cmd/server/main.go b/cmd/server/main.go index 1c31a277a..7928f73c2 100644 --- a/cmd/server/main.go +++ b/cmd/server/main.go @@ -349,6 +349,8 @@ func main() { // WebSocket hub hub := NewHub() hub.SetAllowedOrigins(cfg.CORSAllowedOrigins) + hub.ConfigureLimits(cfg.WSMaxConnsPerIP(), cfg.WSUpgradesPerMinPerIP(), + cfg.WSTrustedProxies(), cfg.WSDeny()) // #1794 hub.upgrader.EnableCompression = cfg.WSCompressionEnabled() // HTTP server diff --git a/cmd/server/routes.go b/cmd/server/routes.go index 6928c698e..cc2514627 100644 --- a/cmd/server/routes.go +++ b/cmd/server/routes.go @@ -886,8 +886,10 @@ func (s *Server) handleHealth(w http.ResponseWriter, r *http.Request) { uptime := time.Since(s.startedAt).Seconds() wsClients := 0 + var wsDeny, wsRate, wsConnCap int64 if s.hub != nil { wsClients = s.hub.ClientCount() + wsDeny, wsRate, wsConnCap = s.hub.limits.counts() // #1794; nil-safe } // Real packet store stats @@ -959,8 +961,9 @@ func (s *Server) handleHealth(w http.ResponseWriter, r *http.Request) { P95Ms: round(percentile(sortedPauses, 0.95), 1), P99Ms: round(percentile(sortedPauses, 0.99), 1), }, - Cache: cs, - WebSocket: WebSocketStatsResp{Clients: wsClients}, + Cache: cs, + WebSocket: WebSocketStatsResp{Clients: wsClients, + RejectedDeny: wsDeny, RejectedRate: wsRate, RejectedConnCap: wsConnCap}, PacketStore: HealthPacketStoreStats{ Packets: pktCount, EstimatedMB: pktEstMB, diff --git a/cmd/server/types.go b/cmd/server/types.go index c24b5341b..b3d81c7df 100644 --- a/cmd/server/types.go +++ b/cmd/server/types.go @@ -586,6 +586,13 @@ type PerfCacheStats struct { type WebSocketStatsResp struct { Clients int `json:"clients"` + // #1794: upgrade rejections since boot, split by cause so an operator can + // tell a deny-list hit from a client that is merely reconnecting too fast. + // omitempty: an install with no limits configured sends none of these + // rather than three permanent zeroes. + RejectedDeny int64 `json:"rejectedDeny,omitempty"` + RejectedRate int64 `json:"rejectedRate,omitempty"` + RejectedConnCap int64 `json:"rejectedConnCap,omitempty"` } type HealthPacketStoreStats struct { diff --git a/cmd/server/websocket.go b/cmd/server/websocket.go index ba4adc90d..a8b177305 100644 --- a/cmd/server/websocket.go +++ b/cmd/server/websocket.go @@ -17,7 +17,8 @@ type Hub struct { mu sync.RWMutex clients map[*Client]bool upgrader websocket.Upgrader - allowedOrigins []string // exact-match allowlist for /ws CheckOrigin (see SetAllowedOrigins) + allowedOrigins []string // exact-match allowlist for /ws CheckOrigin (see SetAllowedOrigins) + limits *wsLimiter // #1794: per-IP caps and deny list; nil allows everything } // SetAllowedOrigins configures the exact-match origin allowlist consulted by @@ -78,6 +79,32 @@ type Client struct { conn *websocket.Conn send chan []byte closeOnce sync.Once + // release returns this connection's slot to the per-IP concurrent cap + // (#1794). Always non-nil so Unregister needs no nil check; it is a no-op + // when limits are disabled. + release func() +} + +// ConfigureLimits installs the #1794 transport limits. Called once at +// startup, before the listener binds, so no upgrade can race a half-built +// limiter. Passing zero values for both caps leaves only the deny list +// active, which is a legitimate configuration. +func (h *Hub) ConfigureLimits(maxConnsPerIP, upgradesPerMin int, trustedProxies, deny []string) { + l := newWSLimiter() + l.maxConnsPerIP = maxConnsPerIP + l.upgradesPerMin = upgradesPerMin + l.trustedProxies = parseCIDRList(trustedProxies, "webSocket.trustedProxies") + l.denyNets = parseCIDRList(deny, "webSocket.deny") + h.mu.Lock() + h.limits = l + h.mu.Unlock() + if len(l.denyNets) > 0 { + log.Printf("[ws] deny list active: %d network(s)", len(l.denyNets)) + } + if len(l.trustedProxies) == 0 && (maxConnsPerIP > 0 || upgradesPerMin > 0) { + log.Printf("[ws] per-IP limits configured with no trustedProxies: they apply only to " + + "directly-connected clients. Behind a reverse proxy, set webSocket.trustedProxies.") + } } func NewHub() *Hub { @@ -112,6 +139,12 @@ func (h *Hub) Unregister(c *Client) { c.closeOnce.Do(func() { close(c.send) }) } h.mu.Unlock() + // #1794: outside the lock β€” release takes the limiter's own mutex, and + // holding two locks in one order here and the other order elsewhere is how + // deadlocks are made. Idempotent, so a double Unregister cannot over-credit. + if c.release != nil { + c.release() + } log.Printf("[ws] client disconnected (%d total)", h.ClientCount()) } @@ -151,15 +184,33 @@ func (h *Hub) Broadcast(msg interface{}) { // ServeWS handles the WebSocket upgrade and runs the client. func (h *Hub) ServeWS(w http.ResponseWriter, r *http.Request) { + // #1794: decide BEFORE the upgrade. Rejecting afterwards would already + // have allocated the connection and completed the handshake, which is the + // resource this is meant to protect. + ok, reason, key, release := h.limits.allow(r) + if !ok { + status := http.StatusForbidden + if reason == wsRejectRate { + // 429 tells a well-behaved client to back off; 403 would read as + // "never come back" for what is a temporary refusal. + status = http.StatusTooManyRequests + } + log.Printf("[ws] reject ip=%s reason=%s", key, reason) + http.Error(w, "websocket upgrade refused", status) + return + } + conn, err := h.upgrader.Upgrade(w, r, nil) if err != nil { log.Printf("[ws] upgrade error: %v", err) + release() // the slot was charged above and this connection never happened return } client := &Client{ - conn: conn, - send: make(chan []byte, 256), + conn: conn, + send: make(chan []byte, 256), + release: release, } h.Register(client) diff --git a/cmd/server/ws_limits.go b/cmd/server/ws_limits.go new file mode 100644 index 000000000..9638b9e33 --- /dev/null +++ b/cmd/server/ws_limits.go @@ -0,0 +1,274 @@ +package main + +// Issue #1794: per-IP limits and a deny list for the /ws upgrade, as defence +// in depth behind the CheckOrigin allowlist from #1793. +// +// CheckOrigin only stops browsers. A Go, Python or curl client can omit the +// Origin header entirely or forge one, connect, and sit in the hub. These +// limits work on the transport instead: who is connecting, how often, and how +// many at once. +// +// THE THING THAT MAKES THIS DANGEROUS TO SHIP NAIVELY, stated up front: +// most CoreScope installs run behind nginx, Caddy, Traefik or a k8s ingress +// (cdn_detection.go says so in as many words). For those, r.RemoteAddr is the +// proxy, 127.0.0.1 for every visitor on earth. A per-IP cap keyed on that +// address protects nobody; it counts the whole internet as one client and +// hands the sixth legitimate browser tab a 403. That is a self-inflicted +// outage, not hardening. +// +// So enforcement is conditional on being able to tell clients apart: +// - X-Forwarded-For is honoured ONLY when the request arrives from an +// address the operator listed in trustedProxies. Otherwise it is an +// attacker-supplied header, and trusting it would let anyone forge a +// fresh source IP per connection, which is worse than no limit at all. +// - If the peer looks like a local reverse proxy and no trustedProxies is +// configured, clients cannot be distinguished, so the per-IP limits are +// SKIPPED and one warning is logged telling the operator what to set. +// The deny list still applies, because an explicit block is the +// operator's own instruction rather than an inference. +// +// Not shipped: a default deny list of hosting-provider ranges. It was +// proposed on the issue (44 CIDRs for one VPS host, after a single scraper +// was seen). Blanket-blocking a provider by default breaks legitimate +// operators who host there, is undiscoverable by the person locked out, and +// ages badly as ranges are reassigned. Operators who want it can configure it. + +import ( + "log" + "net" + "net/http" + "strings" + "sync" + "sync/atomic" + "time" +) + +// wsUpgradeWindow is the span the upgrade budget is counted over. One minute +// matches the config field name (upgradesPerMinPerIP) so an operator reading +// the config can predict the behaviour without reading this file. +const wsUpgradeWindow = time.Minute + +// wsBucketIdleGC is how long an idle per-IP record is kept before collection. +// Longer than the window, so a client that pauses briefly cannot earn a fresh +// budget by being forgotten. +const wsBucketIdleGC = 10 * time.Minute + +// wsRejectReason is the label used both in the log line and in the +// /api/stats counters, so an operator can match one to the other. +type wsRejectReason string + +const ( + wsRejectDeny wsRejectReason = "deny" + wsRejectRate wsRejectReason = "rate" + wsRejectConnCap wsRejectReason = "conncap" +) + +type wsIPState struct { + conns int + upgrades []time.Time // timestamps inside the current window + lastSeenAt time.Time +} + +// wsLimiter holds the parsed configuration and the live per-IP state. A nil +// *wsLimiter allows everything, so a server that never configures limits +// behaves exactly as it did before this change. +type wsLimiter struct { + mu sync.Mutex + + maxConnsPerIP int // 0 disables the concurrent-connection cap + upgradesPerMin int // 0 disables the upgrade-rate limit + + trustedProxies []*net.IPNet + denyNets []*net.IPNet + + state map[string]*wsIPState + + rejectedDeny atomic.Int64 + rejectedRate atomic.Int64 + rejectedConnCap atomic.Int64 + + // warnedIndistinct fires the "set trustedProxies" warning once, rather + // than on every upgrade from behind an unconfigured proxy. + warnedIndistinct sync.Once +} + +func newWSLimiter() *wsLimiter { + return &wsLimiter{state: make(map[string]*wsIPState)} +} + +// parseCIDRList turns operator strings into networks. A bare address is +// accepted and treated as a single-host network, because "1.2.3.4" is what an +// operator naturally writes and silently ignoring it would be the worst +// possible failure mode for a deny list. An unparseable entry is logged and +// skipped rather than aborting startup: one typo must not take the server +// down, but it must not pass unnoticed either. +func parseCIDRList(entries []string, what string) []*net.IPNet { + out := make([]*net.IPNet, 0, len(entries)) + for _, raw := range entries { + e := strings.TrimSpace(raw) + if e == "" { + continue + } + if _, n, err := net.ParseCIDR(e); err == nil { + out = append(out, n) + continue + } + if ip := net.ParseIP(e); ip != nil { + bits := 32 + if ip.To4() == nil { + bits = 128 + } + out = append(out, &net.IPNet{IP: ip, Mask: net.CIDRMask(bits, bits)}) + continue + } + log.Printf("[ws] WARNING: %s entry %q is neither an IP nor a CIDR, ignored", what, e) + } + return out +} + +func ipInAny(ip net.IP, nets []*net.IPNet) bool { + if ip == nil { + return false + } + for _, n := range nets { + if n.Contains(ip) { + return true + } + } + return false +} + +// clientIP resolves the address the limits are counted against, and reports +// whether that address actually identifies a client. +// +// distinct is false when the peer is a loopback or private address and no +// trustedProxies is configured: the request has almost certainly crossed a +// reverse proxy whose X-Forwarded-For we are not allowed to believe, so every +// visitor looks identical and per-IP limits would punish the wrong people. +func (l *wsLimiter) clientIP(r *http.Request) (ip net.IP, distinct bool) { + host, _, err := net.SplitHostPort(r.RemoteAddr) + if err != nil { + host = r.RemoteAddr + } + peer := net.ParseIP(strings.TrimSpace(host)) + if peer == nil { + return nil, false + } + if ipInAny(peer, l.trustedProxies) { + // First hop is the client; later hops were appended by intermediaries + // we have no reason to trust individually. + if xff := r.Header.Get("X-Forwarded-For"); xff != "" { + first := strings.TrimSpace(strings.Split(xff, ",")[0]) + if fwd := net.ParseIP(first); fwd != nil { + return fwd, true + } + } + // A trusted proxy that forwarded no XFF still identifies itself. + return peer, true + } + if peer.IsLoopback() || peer.IsPrivate() || peer.IsLinkLocalUnicast() { + return peer, false + } + return peer, true +} + +// allow decides whether an upgrade may proceed. On rejection it returns the +// reason. On success it returns a release func the caller MUST invoke when +// the connection ends, so the concurrent-connection count comes back down. +func (l *wsLimiter) allow(r *http.Request) (ok bool, reason wsRejectReason, key string, release func()) { + noop := func() {} + if l == nil { + return true, "", "", noop + } + ip, distinct := l.clientIP(r) + + // The deny list is an explicit operator instruction, so it applies even + // when clients cannot be told apart. Behind an unconfigured proxy it + // simply never matches, which is visible rather than silently wrong. + if ipInAny(ip, l.denyNets) { + l.rejectedDeny.Add(1) + return false, wsRejectDeny, ip.String(), noop + } + if l.maxConnsPerIP <= 0 && l.upgradesPerMin <= 0 { + return true, "", "", noop + } + if !distinct { + l.warnedIndistinct.Do(func() { + log.Printf("[ws] WARNING: per-IP limits are configured but every request arrives from %s, "+ + "so clients cannot be told apart and the limits are NOT enforced. "+ + "Set webSocket.trustedProxies to your reverse proxy's address to enable them.", ip) + }) + return true, "", "", noop + } + + key = ip.String() + now := time.Now() + + l.mu.Lock() + defer l.mu.Unlock() + l.gcLocked(now) + + st := l.state[key] + if st == nil { + st = &wsIPState{} + l.state[key] = st + } + st.lastSeenAt = now + + if l.upgradesPerMin > 0 { + cutoff := now.Add(-wsUpgradeWindow) + kept := st.upgrades[:0] + for _, t := range st.upgrades { + if t.After(cutoff) { + kept = append(kept, t) + } + } + st.upgrades = kept + if len(st.upgrades) >= l.upgradesPerMin { + l.rejectedRate.Add(1) + return false, wsRejectRate, key, noop + } + } + if l.maxConnsPerIP > 0 && st.conns >= l.maxConnsPerIP { + l.rejectedConnCap.Add(1) + return false, wsRejectConnCap, key, noop + } + + // Both budgets have room, so charge them together: a rejected upgrade + // must never consume rate budget for a connection it did not get. + if l.upgradesPerMin > 0 { + st.upgrades = append(st.upgrades, now) + } + st.conns++ + + var once sync.Once + return true, "", key, func() { + once.Do(func() { + l.mu.Lock() + defer l.mu.Unlock() + if s := l.state[key]; s != nil && s.conns > 0 { + s.conns-- + s.lastSeenAt = time.Now() + } + }) + } +} + +// gcLocked drops records with no live connections and no recent activity, so +// the map cannot grow without bound when the server is scanned from many +// addresses. Caller must hold l.mu. +func (l *wsLimiter) gcLocked(now time.Time) { + for k, st := range l.state { + if st.conns == 0 && now.Sub(st.lastSeenAt) > wsBucketIdleGC { + delete(l.state, k) + } + } +} + +// counts reports the rejection tallies for /api/stats. +func (l *wsLimiter) counts() (deny, rate, connCap int64) { + if l == nil { + return 0, 0, 0 + } + return l.rejectedDeny.Load(), l.rejectedRate.Load(), l.rejectedConnCap.Load() +} diff --git a/cmd/server/ws_limits_test.go b/cmd/server/ws_limits_test.go new file mode 100644 index 000000000..cf025770f --- /dev/null +++ b/cmd/server/ws_limits_test.go @@ -0,0 +1,296 @@ +package main + +// Tests for the #1794 /ws transport limits. The five cases the issue lists as +// TDD requirements are marked with the exact wording from the issue body, so a +// reviewer can match them off without reading the implementation. + +import ( + "net/http" + "net/http/httptest" + "testing" + "time" +) + +func req(remoteAddr, xff string) *http.Request { + r := httptest.NewRequest(http.MethodGet, "/ws", nil) + r.RemoteAddr = remoteAddr + if xff != "" { + r.Header.Set("X-Forwarded-For", xff) + } + return r +} + +func limiter(maxConns, perMin int, trusted, deny []string) *wsLimiter { + l := newWSLimiter() + l.maxConnsPerIP = maxConns + l.upgradesPerMin = perMin + l.trustedProxies = parseCIDRList(trusted, "trusted") + l.denyNets = parseCIDRList(deny, "deny") + return l +} + +// "Deny CIDR match β†’ 403, hub count unchanged." +func TestWSDeny_CIDRMatchRejects(t *testing.T) { + l := limiter(0, 0, nil, []string{"203.0.113.0/24"}) + ok, reason, _, _ := l.allow(req("203.0.113.7:1234", "")) + if ok || reason != wsRejectDeny { + t.Fatalf("denied CIDR: ok=%v reason=%q, want rejected with %q", ok, reason, wsRejectDeny) + } + if ok, _, _, _ := l.allow(req("203.0.114.7:1234", "")); !ok { + t.Error("an address outside the denied range must still connect") + } + if d, _, _ := l.counts(); d != 1 { + t.Errorf("rejectedDeny = %d, want 1", d) + } +} + +func TestWSDeny_BareAddressIsAcceptedAsSingleHost(t *testing.T) { + // Operators write "1.2.3.4", not "1.2.3.4/32". Silently ignoring the bare + // form would be the worst possible failure for a deny list: it looks + // configured and blocks nothing. + l := limiter(0, 0, nil, []string{"198.51.100.9"}) + if ok, _, _, _ := l.allow(req("198.51.100.9:5000", "")); ok { + t.Error("bare address in the deny list must block") + } + if ok, _, _, _ := l.allow(req("198.51.100.10:5000", "")); !ok { + t.Error("the neighbouring address must not be blocked") + } +} + +func TestWSDeny_UnparseableEntryIsSkippedNotFatal(t *testing.T) { + // One typo must not take the deny list, or the server, down. + nets := parseCIDRList([]string{"not-an-ip", "203.0.113.0/24"}, "deny") + if len(nets) != 1 { + t.Fatalf("parsed %d networks, want 1 (the valid entry)", len(nets)) + } +} + +// "6th conn from same IP with cap=5 β†’ 403." +func TestWSConnCap_SixthConnectionRejected(t *testing.T) { + l := limiter(5, 0, nil, nil) + for i := 1; i <= 5; i++ { + if ok, _, _, _ := l.allow(req("203.0.113.5:1", "")); !ok { + t.Fatalf("connection %d of 5 was rejected", i) + } + } + ok, reason, _, _ := l.allow(req("203.0.113.5:1", "")) + if ok || reason != wsRejectConnCap { + t.Fatalf("6th connection: ok=%v reason=%q, want rejected with %q", ok, reason, wsRejectConnCap) + } + if _, _, c := l.counts(); c != 1 { + t.Errorf("rejectedConnCap = %d, want 1", c) + } +} + +func TestWSConnCap_ReleaseFreesTheSlot(t *testing.T) { + l := limiter(1, 0, nil, nil) + _, _, _, release := l.allow(req("203.0.113.5:1", "")) + if ok, _, _, _ := l.allow(req("203.0.113.5:1", "")); ok { + t.Fatal("cap=1 must refuse the second concurrent connection") + } + release() + if ok, _, _, _ := l.allow(req("203.0.113.5:1", "")); !ok { + t.Error("after release the slot must be reusable") + } +} + +func TestWSConnCap_ReleaseIsIdempotent(t *testing.T) { + // Unregister can run twice for one client; double-crediting would let an + // IP accumulate slots it never gave back. + l := limiter(1, 0, nil, nil) + _, _, _, release := l.allow(req("203.0.113.5:1", "")) + release() + release() + if ok, _, _, _ := l.allow(req("203.0.113.5:1", "")); !ok { + t.Fatal("first reconnect must be allowed") + } + if ok, _, _, _ := l.allow(req("203.0.113.5:1", "")); ok { + t.Error("cap=1 must still hold: the double release must not have credited two slots") + } +} + +// "31 upgrades/min from same IP β†’ 31st rejected." +func TestWSRate_ThirtyFirstUpgradeRejected(t *testing.T) { + l := limiter(0, 30, nil, nil) + for i := 1; i <= 30; i++ { + if ok, _, _, _ := l.allow(req("203.0.113.8:1", "")); !ok { + t.Fatalf("upgrade %d of 30 was rejected", i) + } + } + ok, reason, _, _ := l.allow(req("203.0.113.8:1", "")) + if ok || reason != wsRejectRate { + t.Fatalf("31st upgrade: ok=%v reason=%q, want rejected with %q", ok, reason, wsRejectRate) + } + if _, r, _ := l.counts(); r != 1 { + t.Errorf("rejectedRate = %d, want 1", r) + } +} + +func TestWSRate_BudgetIsPerAddress(t *testing.T) { + l := limiter(0, 2, nil, nil) + l.allow(req("203.0.113.1:1", "")) + l.allow(req("203.0.113.1:1", "")) + if ok, _, _, _ := l.allow(req("203.0.113.1:1", "")); ok { + t.Fatal("the first address should be out of budget") + } + if ok, _, _, _ := l.allow(req("203.0.113.2:1", "")); !ok { + t.Error("a different address must have its own budget") + } +} + +func TestWSRate_OldAttemptsLeaveTheWindow(t *testing.T) { + l := limiter(0, 2, nil, nil) + key := "203.0.113.3" + l.state[key] = &wsIPState{ + upgrades: []time.Time{time.Now().Add(-2 * wsUpgradeWindow), time.Now().Add(-90 * time.Second)}, + lastSeenAt: time.Now(), + } + if ok, _, _, _ := l.allow(req(key+":1", "")); !ok { + t.Error("attempts older than the window must not count against the budget") + } +} + +func TestWSRate_RejectedUpgradeDoesNotConsumeBudget(t *testing.T) { + // A refusal must not push the client further into debt, or a client that + // keeps retrying could never recover even once its window cleared. + l := limiter(1, 5, nil, nil) + l.allow(req("203.0.113.4:1", "")) // takes the only conn slot, 1 upgrade used + for i := 0; i < 3; i++ { + l.allow(req("203.0.113.4:1", "")) // all rejected on the conn cap + } + l.mu.Lock() + used := len(l.state["203.0.113.4"].upgrades) + l.mu.Unlock() + if used != 1 { + t.Errorf("upgrades charged = %d, want 1 (rejections must not be charged)", used) + } +} + +// "Trusted proxy + XFF spoofing β†’ rate counted against the XFF IP, not the proxy." +func TestWSClientIP_TrustedProxyUsesForwardedFor(t *testing.T) { + l := limiter(0, 1, []string{"10.0.0.0/8"}, nil) + if ok, _, key, _ := l.allow(req("10.0.0.2:1", "203.0.113.20, 10.0.0.2")); !ok || key != "203.0.113.20" { + t.Fatalf("key = %q (ok=%v), want the first XFF hop 203.0.113.20", key, ok) + } + // Budget of 1 is now spent for that client, but a different client behind + // the SAME proxy must be unaffected. This is the whole point of honouring + // XFF: one visitor must not exhaust the budget for everyone else. + if ok, _, _, _ := l.allow(req("10.0.0.2:1", "203.0.113.21, 10.0.0.2")); !ok { + t.Error("a second client behind the same trusted proxy must have its own budget") + } + if ok, _, _, _ := l.allow(req("10.0.0.2:1", "203.0.113.20, 10.0.0.2")); ok { + t.Error("the first client's budget should still be spent") + } +} + +// "Untrusted RemoteAddr + XFF spoofing β†’ XFF ignored, rate counted against RemoteAddr." +func TestWSClientIP_UntrustedPeerIgnoresForwardedFor(t *testing.T) { + l := limiter(0, 1, []string{"10.0.0.0/8"}, nil) + // A public peer that is not a configured proxy. If XFF were believed here, + // an attacker would mint a fresh identity per request and the limit would + // be worthless. + if ok, _, key, _ := l.allow(req("203.0.113.30:1", "198.51.100.1")); !ok || key != "203.0.113.30" { + t.Fatalf("key = %q (ok=%v), want the peer address, not the forged header", key, ok) + } + if ok, _, _, _ := l.allow(req("203.0.113.30:1", "198.51.100.2")); ok { + t.Error("changing the forged header must not buy a fresh budget") + } +} + +func TestWSClientIP_TrustedProxyWithoutForwardedForUsesPeer(t *testing.T) { + l := limiter(0, 5, []string{"10.0.0.0/8"}, nil) + if _, _, key, _ := l.allow(req("10.0.0.2:1", "")); key != "10.0.0.2" { + t.Errorf("key = %q, want the proxy's own address when it forwards nothing", key) + } +} + +// The safety property this design exists for: behind an unconfigured reverse +// proxy every visitor shares one address, so enforcing a per-IP cap would +// refuse real users. Limits must be skipped, not applied. +func TestWSLimits_SkippedWhenClientsCannotBeToldApart(t *testing.T) { + l := limiter(1, 1, nil, nil) // caps of 1, and no trustedProxies + for i := 0; i < 10; i++ { + if ok, reason, _, _ := l.allow(req("127.0.0.1:1", "203.0.113.9")); !ok { + t.Fatalf("request %d refused (%s): loopback peer with no trustedProxies must not be limited", i, reason) + } + } + if _, r, c := l.counts(); r != 0 || c != 0 { + t.Errorf("rejections rate=%d conncap=%d, want 0 and 0", r, c) + } +} + +func TestWSLimits_PrivatePeerAlsoCountsAsIndistinct(t *testing.T) { + l := limiter(1, 1, nil, nil) + for i := 0; i < 5; i++ { + if ok, _, _, _ := l.allow(req("192.168.1.10:1", "")); !ok { + t.Fatalf("request %d refused: a private peer with no trustedProxies must not be limited", i) + } + } +} + +// The deny list is an explicit operator instruction, so unlike the caps it +// still applies when clients cannot be told apart. +func TestWSDeny_AppliesEvenWhenIndistinct(t *testing.T) { + l := limiter(0, 0, nil, []string{"127.0.0.1"}) + if ok, reason, _, _ := l.allow(req("127.0.0.1:1", "")); ok || reason != wsRejectDeny { + t.Errorf("ok=%v reason=%q, want the deny list to apply regardless", ok, reason) + } +} + +func TestWSLimits_NilLimiterAllowsEverything(t *testing.T) { + // A Hub built without ConfigureLimits must behave exactly as before. + var l *wsLimiter + ok, _, _, release := l.allow(req("203.0.113.99:1", "")) + if !ok { + t.Fatal("a nil limiter must allow the upgrade") + } + release() // must not panic + if d, r, c := l.counts(); d != 0 || r != 0 || c != 0 { + t.Errorf("nil limiter counts = %d/%d/%d, want zeroes", d, r, c) + } +} + +func TestWSLimits_IdleStateIsCollected(t *testing.T) { + // Without collection, scanning the server from many addresses would grow + // the map forever. + l := limiter(0, 5, nil, nil) + l.state["203.0.113.50"] = &wsIPState{conns: 0, lastSeenAt: time.Now().Add(-2 * wsBucketIdleGC)} + l.state["203.0.113.51"] = &wsIPState{conns: 1, lastSeenAt: time.Now().Add(-2 * wsBucketIdleGC)} + l.mu.Lock() + l.gcLocked(time.Now()) + _, idleKept := l.state["203.0.113.50"] + _, liveKept := l.state["203.0.113.51"] + l.mu.Unlock() + if idleKept { + t.Error("an idle record with no connections should be collected") + } + if !liveKept { + t.Error("a record with a live connection must never be collected") + } +} + +func TestWSConfig_Defaults(t *testing.T) { + // The connection cap is deliberately OFF by default: carrier-grade NAT + // puts many unrelated subscribers behind one address, so a low cap would + // refuse real visitors on mobile networks. + var c Config + if got := c.WSMaxConnsPerIP(); got != 0 { + t.Errorf("default maxConnsPerIP = %d, want 0 (off)", got) + } + if got := c.WSUpgradesPerMinPerIP(); got != 30 { + t.Errorf("default upgradesPerMinPerIP = %d, want 30", got) + } + zero := 0 + c.WebSocket = &WebSocketConfig{UpgradesPerMinPerIP: &zero} + if got := c.WSUpgradesPerMinPerIP(); got != 0 { + t.Errorf("explicit 0 = %d, want 0: an operator must be able to turn the rate limit off", got) + } + five := 5 + c.WebSocket = &WebSocketConfig{UpgradesPerMinPerIP: &five, MaxConnsPerIP: 3} + if got := c.WSUpgradesPerMinPerIP(); got != 5 { + t.Errorf("configured rate = %d, want 5", got) + } + if got := c.WSMaxConnsPerIP(); got != 3 { + t.Errorf("configured cap = %d, want 3", got) + } +} diff --git a/config.example.json b/config.example.json index 5a72ae2ac..8b33967c8 100644 --- a/config.example.json +++ b/config.example.json @@ -36,6 +36,16 @@ "_comment_ingestorStats": "Ingestor publishes a 1-Hz stats snapshot consumed by the server's /api/perf/io and /api/perf/write-sources endpoints (#1120). Path is configured via the CORESCOPE_INGESTOR_STATS environment variable on the INGESTOR process. Default: /tmp/corescope-ingestor-stats.json. The writer uses O_NOFOLLOW + 0o600, so a pre-planted symlink in /tmp cannot be used to clobber an arbitrary file. SECURITY: in shared-tmp environments (multi-tenant hosts), point CORESCOPE_INGESTOR_STATS at a private directory like /var/lib/corescope/ingestor-stats.json that only the corescope user can write to.", "corsAllowedOrigins": [], "_comment_corsAllowedOrigins": "Cross-origin allowlist for embed scenarios (#1369) AND for /ws WebSocket upgrades (#1793 β€” CSWSH defense per OWASP WebSocket Security Cheat Sheet). Exact-match origins, e.g. [\"https://blog.example.com\", \"https://embed.example.com\"]. Same-origin requests (Origin host == request Host) and non-browser clients (no Origin header) are always allowed for /ws. When empty (default), no Access-Control-* headers are sent and browsers enforce same-origin; cross-origin /ws upgrades are rejected. When non-empty, only the listed origins receive CORS headers, and Access-Control-Allow-Methods is limited to GET, HEAD, OPTIONS (the cross-domain surface is read-only β€” same-origin admin writes are unaffected). Use [\"*\"] to allow any origin for CORS XHR (NOT recommended for write-capable deployments); note that \"*\" is deliberately NOT honored for /ws upgrades β€” list explicit origins to permit cross-origin WebSocket clients (OWASP guidance: avoid wildcards in CSWSH defense). Operators can override per-deployment with the CORS_ALLOWED_ORIGINS environment variable (comma-separated). No credentialed CORS is enabled. To embed the map or channels pages cross-domain, add the embedding origin here and use the URL pattern '/#/map?embed=1' or '/#/channels?embed=1' β€” embed mode hides the top-nav, bottom-nav, and side drawer for full-bleed iframe rendering.", + "webSocket": { + "maxConnsPerIP": 0, + "_comment_maxConnsPerIP": "#1794 - concurrent /ws connections allowed from one client address. 0 = off, and that is the shipped default ON PURPOSE: carrier-grade NAT puts thousands of unrelated mobile subscribers behind one public IPv4, so a low cap refuses real visitors while a scraper simply rents more addresses. Set it only if you know your audience is not behind shared NAT.", + "upgradesPerMinPerIP": 30, + "_comment_upgradesPerMinPerIP": "#1794 - handshakes per minute from one client address. On by default. Safe under CGNAT: a real client upgrades a handful of times per minute even while reconnecting, so 30 leaves ordinary traffic alone while flattening a scraper reconnect loop. Set 0 to disable.", + "trustedProxies": [], + "_comment_trustedProxies": "#1794 - addresses or CIDRs of YOUR reverse proxy. REQUIRED for the per-IP limits to do anything behind nginx/Caddy/Traefik/ingress: without it every visitor shares the proxy's address, so the limits are skipped and one warning is logged at startup. X-Forwarded-For is believed only from these addresses, because anyone else can forge it. Never list an address you do not control.", + "deny": [], + "_comment_deny": "#1794 - addresses or CIDRs refused at the /ws upgrade, before the handshake. Accepts both \"1.2.3.4\" and \"1.2.3.0/24\". Applies even behind an unconfigured proxy, since it is your explicit instruction. Rejections are counted in /api/stats under websocket.rejectedDeny." + }, "https": { "cert": "/path/to/cert.pem", "key": "/path/to/key.pem", From f93de01012454da4c30de7b4f781d9deaa093983 Mon Sep 17 00:00:00 2001 From: Anupam Mediratta Date: Wed, 9 Sep 2026 19:07:40 +0530 Subject: [PATCH 22/26] fix(qa): bind TEST_PUBKEY as a SQLite parameter instead of interpolating it (#1982) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #1977. Supersedes #1952. Follow-up filed as #1983. ## What Β§10.2 did ```bash q="SELECT COUNT(*) FROM transmissions WHERE from_node = '$TEST_PUBKEY';" qq=$(printf %q "$q") if ! count=$(ssh_t "docker exec … sqlite3 … $qq" 2>/dev/null); then count=$(ssh_t "sqlite3 … $qq" 2>/dev/null || echo "") fi ``` The injection is not reachable today β€” `TEST_PUBKEY` is hex-gated and the script `exit 2`s before the SQL is built. The problem is that the SQL layer's safety rests entirely on that outer gate rather than on the SQL layer itself. #1952 proposed doubling embedded quotes; that is string escaping, not parameterisation, which is why it was withdrawn in favour of this. ## What this does Per the four points in the sign-off on #1977: **1. Bind the value.** A constant `SELECT` and a bound `:pubkey`, fed to sqlite3 on stdin. The SQL no longer crosses the remote shell as a command word, so there is no `printf %q` on the query at all any more. **Why hex rather than `.parameter set :pk ''`.** Dot-command arguments are split on whitespace, so a payload containing a space produces too many arguments β€” and sqlite3 responds by printing the `.parameter` help to **stdout**, exiting **0**, and leaving `:pk` **unbound**. `COUNT(*)` then returns 0, which reads exactly like a passing security fix. `-bail` does not catch it. Verified on 3.51.0: ``` $ printf ".parameter set :pk '' OR 1=1 --'\nSELECT COUNT(*) FROM transmissions WHERE from_node = :pk;\n" \ | sqlite3 -bail ptest.db .parameter CMD ... Manage SQL parameter bindings # <- help, on stdout … 0 # <- :pk never bound $ echo $? 0 ``` `.parameter set :pk 1+1` also binds the integer `2` β€” the value is evaluated as an SQL expression and only falls back to a text literal when evaluation fails. So interpolating into the `.parameter set` line trades one hazard for another. Hex-encoding removes the quoting layer instead of adding one: the value is bound as `cast(x'' as text)`, so its contribution to the SQL text is drawn from the alphabet `[0-9a-f]` only. Nothing to quote, no tokenizer arity hazard, and it holds for **arbitrary** input rather than only for hex-gated input β€” which is the point. Verified against a fixture table holding two rows, one of them `deadbeef`: | value | result | exit | |---|---|---| | `deadbeef`, bound as `cast(x'6465616462656566' as text)` | `1` | 0 | | `' OR 1=1 --`, bound the same way | `0` | 0 | | `' OR 1=1 --`, interpolated the current way | `2` (whole table) | 0 | | query against a DB with no `transmissions` table, `-bail` | `Parse error … no such table` on **stderr** | **1** | **2. Probe the capability, not a version.** `resolve_sqlite_runner` binds `corescope-probe-ok` and asserts it comes back β€” a round trip, not a bare `.parameter init`, so the positive control runs against the operator's actual binary rather than one we pin. If neither the container nor the host qualifies, it fails loudly and names what is needed: ``` ❌ retain-failed: no sqlite3 able to bind a parameter on the target tried: docker exec -i corescope-prod sqlite3, then sqlite3 on runner@example need: the sqlite3 CLI reachable over ssh, supporting '.parameter set' OCI runtime exec failed: exec: "sqlite3": executable file not found in $PATH bash: line 1: sqlite3: command not found ``` There is deliberately **no** interpolating fallback. That would leave the vulnerable path in place under a nicer name. **3. The hex gate is kept**, with its comment updated to say why: for the SQL layer it is now defence in depth rather than the only guard. Redundant is not the same as wrong. **4. The exit status and stderr survive.** `-batch -bail -init /dev/null -noheader -list` (stop at the first SQL error; ignore the operator's `~/.sqliterc`, where a stray `.mode` would make the count unparseable; stdout is exactly the number). Query stderr is captured and printed on failure rather than sent to `/dev/null`, so a broken query is distinguishable from a legitimately empty result. Probe stderr is collected too, and printed only if *both* probes fail β€” the container miss is the known-normal case, so surfacing it on every run would be noise. ## Also fixed An existing double-count in Β§10.2: the `TARGET_DB_PATH unset` branch incremented `$fails` and then left `count=""`, so the generic branch incremented it a **second** time for the same failure. `read_retain_count` now gives Β§10.2 exactly one increment point. Opportunistic cleanup in a file already being touched (AGENTS.md line 318). ## Tests New `qa/scripts/test-blacklist-sql.sh`, wired into the `go-test` job. 24 assertions, modelled on `scripts/staging/test-disk-monitor.sh`. Both directions are asserted, because a zero from a command that failed proves nothing: - **Positive control** β€” `deadbeef` still returns its row (`1`, exit 0), and so does `cafebabe`; an absent pubkey returns `0`. - **Negative** β€” `' OR 1=1 --` returns `0` while the table demonstrably holds 2 rows, and the old interpolated form is asserted to leak all `2`. That last assertion is what makes the `0` above worth something. - **Error surfacing** β€” the same SQL against a DB with no `transmissions` table exits non-zero with a message on stderr and nothing on stdout. - **Alphabet** β€” `sql_hex_literal` output matches `^x'[0-9a-f]*'$` for the SQL payloads, a backslash, `$(id)` / backticks, an embedded newline, `hΓ©llo`, and a 4096-byte repetitive string. That last one is a regression guard for `od -v`: without the flag `od` collapses repeated identical lines to `*`. - `run_sqlite` with no resolved runner refuses rather than guessing. Group 2 skips loudly (rather than silently) if `sqlite3` is not on PATH; group 1 needs no sqlite3 and always runs. **Mutation-tested** β€” each of these breaks the suite, so the assertions have teeth: | mutation | caught by | |---|---| | restore full interpolation | `injection payload β†’ 0 rows β€” expected '0' got '2'` | | naive `.parameter set '%s'` | `expected '0' got '.parameter CMD ...'` | | drop `od -v` | alphabet failure on `*`, plus `expected '8192' got '33'` | Commit 1 is a behaviour-neutral refactor that moves the imperative body into `main()` behind a `BASH_SOURCE` guard, so the test can source the script and exercise individual helpers. Same idiom as `scripts/staging/disk-monitor.sh:99`. ## Verification - `bash qa/scripts/test-blacklist-sql.sh` β†’ 24 passed, 0 failed - `bash -n` on both scripts - All three runtime paths exercised end to end with PATH shims for `ssh`/`docker`/`sqlite3` against a real fixture DB: success (`sqlite3 runner: host`, count 2), query failure (classified message + `Parse error … no such table`, `fails=1`), and no-capability (the loud block above, both probe stderrs, `fails=1` β€” not 2) - The new step lands inside `go-test`, which runs when `changes.outputs.code == 'true'`; `qa/scripts/*.sh` does not match that job's `^docs/|[.]md$|^LICENSE$` documentation filter, so it is not skipped ## Deliberately out of scope - **The `docker exec` branch is dead on current images** β†’ filed as #1983. The app container has no `sqlite3` at all: `Dockerfile:15` is pure-Go SQLite with no CGO, and the `apk add` installs only `mosquitto mosquitto-clients supervisor caddy wget`. So the host "fallback" is the only path that has ever executed, silently, because both branches discarded stderr. This change keeps both branches and merely makes the outcome visible (`sqlite3 runner: …` on every run). - **`-readonly` on the target DB.** Tempting, and verified compatible with `.parameter` (the binding table lives in the TEMP database), but a WAL database needing journal recovery can refuse a read-only open. Adding it here risks exactly the "trades an unreachable injection for a script that does not run" outcome flagged in the #1952 thread. Worth its own issue. - **The other `2>/dev/null` sites** in this file, which also sit awkwardly with `qa/README.md`'s "Don't silence stderr". Only the Β§10.2 lines named in the sign-off are touched. πŸ€– Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 (cherry picked from commit de237fc29cb1d1b682d8b91519c05c6506148232) Co-Authored-By: Claude Opus 5 --- .github/workflows/deploy.yml | 3 + qa/scripts/blacklist-test.sh | 342 +++++++++++++++++++++---------- qa/scripts/test-blacklist-sql.sh | 135 ++++++++++++ 3 files changed, 369 insertions(+), 111 deletions(-) create mode 100755 qa/scripts/test-blacklist-sql.sh diff --git a/.github/workflows/deploy.yml b/.github/workflows/deploy.yml index 1e8be8f62..9082db010 100644 --- a/.github/workflows/deploy.yml +++ b/.github/workflows/deploy.yml @@ -86,6 +86,9 @@ jobs: - name: Staging disk-monitor unit tests (issue #1684) run: bash scripts/staging/test-disk-monitor.sh + - name: QA SQL parameter-binding unit tests (issue #1977) + run: bash qa/scripts/test-blacklist-sql.sh + - name: Lint CSS variables (issue #1128) run: | set -e diff --git a/qa/scripts/blacklist-test.sh b/qa/scripts/blacklist-test.sh index 94b5b4127..535f477f0 100755 --- a/qa/scripts/blacklist-test.sh +++ b/qa/scripts/blacklist-test.sh @@ -25,65 +25,24 @@ # ssh-failed β†’ cannot reach/control target # restart-stuck β†’ /api/stats not 200 within RESTART_WAIT_S # hide-failed β†’ blacklisted pubkey still surfaced via API (Β§10.1 fail) -# retain-failed β†’ blacklisted pubkey absent from DB (Β§10.2 fail) +# retain-failed β†’ blacklisted pubkey absent from DB (Β§10.2 fail), or the +# Β§10.2 probe could not run at all β€” no sqlite3 on the target +# able to bind a parameter. The message names what is needed; +# there is no fallback to interpolated SQL. # teardown-failedβ†’ post-test removal did not restore listing # # Exit code = number of failures (0 = pass). # PUBLIC repo: zero PII β€” no real pubkeys, IPs, or hostnames as defaults. +# +# Structure: helpers live at top level and the imperative body lives in main(), +# so test-blacklist-sql.sh can source this file and exercise individual helpers +# without running the suite. Same idiom as scripts/staging/disk-monitor.sh. set -uo pipefail -BASELINE_URL="${1:-}" -TARGET_URL="${2:-}" -if [[ -z "$BASELINE_URL" || -z "$TARGET_URL" ]]; then - echo "usage: $0 BASELINE_URL TARGET_URL (TEST_NODE_PUBKEY+TARGET_* via env)" >&2 - exit 2 -fi - -TEST_PUBKEY="${TEST_NODE_PUBKEY:-}" -TARGET_SSH_HOST="${TARGET_SSH_HOST:-}" -TARGET_SSH_KEY="${TARGET_SSH_KEY:-/root/.ssh/id_ed25519}" -TARGET_CONFIG_PATH="${TARGET_CONFIG_PATH:-}" -TARGET_CONTAINER="${TARGET_CONTAINER:-}" -TARGET_DB_PATH="${TARGET_DB_PATH:-}" -ADMIN_API_TOKEN="${ADMIN_API_TOKEN:-}" - -if [[ -z "$TEST_PUBKEY" || -z "$TARGET_SSH_HOST" || -z "$TARGET_CONFIG_PATH" || -z "$TARGET_CONTAINER" ]]; then - echo "error: TEST_NODE_PUBKEY, TARGET_SSH_HOST, TARGET_CONFIG_PATH, TARGET_CONTAINER are required" >&2 - exit 2 -fi - -# Hard input validation β€” these strings are interpolated into remote shell/SQL. -# Pubkey must be hex (MeshCore pubkeys are hex-encoded ed25519 prefixes). -if ! [[ "$TEST_PUBKEY" =~ ^[0-9a-fA-F]+$ ]]; then - echo "error: TEST_NODE_PUBKEY must be hex (got: redacted)" >&2 - exit 2 -fi -# Container name must match docker's allowed chars: [a-zA-Z0-9][a-zA-Z0-9_.-]* -if ! [[ "$TARGET_CONTAINER" =~ ^[a-zA-Z0-9][a-zA-Z0-9_.-]*$ ]]; then - echo "error: TARGET_CONTAINER has illegal chars" >&2 - exit 2 -fi -# Config path must be an absolute, sane path (no spaces, quotes, $, ;, etc.). -if ! [[ "$TARGET_CONFIG_PATH" =~ ^/[A-Za-z0-9_./-]+$ ]]; then - echo "error: TARGET_CONFIG_PATH must be a sane absolute path" >&2 - exit 2 -fi -if [[ -n "$TARGET_DB_PATH" ]] && ! [[ "$TARGET_DB_PATH" =~ ^/[A-Za-z0-9_./-]+$ ]]; then - echo "error: TARGET_DB_PATH must be a sane absolute path" >&2 - exit 2 -fi - -CURL_TIMEOUT="${CURL_TIMEOUT:-60}" -RESTART_WAIT_S="${RESTART_WAIT_S:-120}" - -SSH_OPTS=(-i "$TARGET_SSH_KEY" -o StrictHostKeyChecking=accept-new -o ConnectTimeout=15 -o BatchMode=yes) +SSH_OPTS=() # populated by main() from TARGET_SSH_KEY ssh_t() { ssh "${SSH_OPTS[@]}" "$TARGET_SSH_HOST" "$@"; } -TMP=$(mktemp -d) -fails=0 -TEARDOWN_DONE=0 - # ----------------------------------------------------------------------------- # Teardown β€” MANDATORY in all exit paths. # ----------------------------------------------------------------------------- @@ -106,7 +65,6 @@ teardown() { rm -rf "$TMP" exit "$rc" } -trap teardown EXIT INT TERM # ----------------------------------------------------------------------------- # Helpers @@ -197,75 +155,237 @@ node_visible() { } # ----------------------------------------------------------------------------- -# Β§10.1 β€” hide +# Β§10.2 DB probe β€” bind the pubkey, do not interpolate it (issue #1977) # ----------------------------------------------------------------------------- -echo "=== Β§10.1 add $TEST_PUBKEY to nodeBlacklist ===" -if ! add_to_blacklist; then fails=$((fails+1)); exit "$fails"; fi -if ! restart_target; then fails=$((fails+1)); exit "$fails"; fi -if ! wait_for_stats; then fails=$((fails+1)); exit "$fails"; fi +# Batch flags, all in service of "the count is parseable and errors are visible": +# -bail stop at the first SQL error instead of running on +# -init /dev/null ignore the operator's ~/.sqliterc β€” a stray .mode there +# would make the count unparseable +# -noheader -list stdout is exactly the number, nothing else +SQLITE_ARGS=(-batch -bail -init /dev/null -noheader -list) +# Round-trip probe token. The value is arbitrary; it only has to come back intact. +SQLITE_PROBE_TOKEN="corescope-probe-ok" +SQLITE_RUNNER="" # "container" | "host", set by resolve_sqlite_runner +RETAIN_COUNT="" # set by read_retain_count -detail_code=$(fetch_code "$TARGET_URL/api/nodes/$TEST_PUBKEY" "$TMP/detail.json") -list_code=$(fetch_code "$TARGET_URL/api/nodes?limit=10000" "$TMP/list.json") -in_list=0 -if [[ "$list_code" == "200" ]] && grep -qF -- "\"$TEST_PUBKEY\"" "$TMP/list.json"; then - in_list=1 -fi -if [[ "$detail_code" == "404" || "$in_list" == "0" ]]; then - echo " βœ… hide ok: detail=$detail_code in_list=$in_list" -else - echo " ❌ hide-failed: detail=$detail_code in_list=$in_list β€” pubkey still surfaced" - fails=$((fails+1)) -fi +# Hex-encode a value for embedding in SQL as a blob literal. +# +# Why hex rather than quoting: the output alphabet is [0-9a-f], so no byte the +# caller passes can terminate a string literal or add a dot-command argument. +# That holds for arbitrary input, which is the point β€” the SQL layer stops +# depending on main()'s hex gate in order to be safe. +# +# `od -v` is load-bearing: without it od collapses runs of identical lines to +# '*' and long repetitive values encode wrongly. +sql_hex_literal() { + printf "x'%s'" "$(printf '%s' "$1" | od -An -v -tx1 | tr -d ' \n')" +} -topo_code=$(fetch_code "$TARGET_URL/api/topology" "$TMP/topo.json") -if [[ "$topo_code" != "200" ]]; then - echo " ⚠️ /api/topology HTTP $topo_code β€” skipping topology assertion" -elif grep -qF -- "$TEST_PUBKEY" "$TMP/topo.json"; then - echo " ❌ hide-failed: /api/topology references blacklisted pubkey" - fails=$((fails+1)) -else - echo " βœ… topology clean" -fi +# SQL for the Β§10.2 count, fed to sqlite3 on stdin. The SELECT text is a +# constant; the pubkey arrives as a bound parameter. +# +# Note the nested cast rather than `.parameter set :pubkey ''`: +# dot-command arguments are split on whitespace, so a value containing a space +# (e.g. "' OR 1=1 --") makes sqlite3 print the .parameter help to STDOUT, exit +# 0, and leave :pubkey unbound. COUNT(*) then returns 0 β€” which reads exactly +# like a passing security fix. -bail does not catch it either. +transmission_count_sql() { + printf '.parameter init\n' + printf '.parameter set :pubkey "cast(%s as text)"\n' "$(sql_hex_literal "$1")" + printf 'SELECT COUNT(*) FROM transmissions WHERE from_node = :pubkey;\n' +} -# ----------------------------------------------------------------------------- -# Β§10.2 β€” DB retain -# ----------------------------------------------------------------------------- -echo "=== Β§10.2 verify packets retained in DB ===" -count="" -if [[ -n "$ADMIN_API_TOKEN" ]]; then - # Read auth header from stdin so the token never enters argv (ps-safe). - code=$(printf 'header = "Authorization: Bearer %s"\n' "$ADMIN_API_TOKEN" | \ - curl -s -m "$CURL_TIMEOUT" -K - -o "$TMP/admin.json" -w "%{http_code}" \ - "$TARGET_URL/api/admin/transmissions?from_node=$TEST_PUBKEY&count=1" 2>/dev/null || echo "000") - if [[ "$code" == "200" ]]; then - count=$(jq -r '.count // ((.transmissions // []) | length)' "$TMP/admin.json" 2>/dev/null || echo "") +# Capability probe: bind a known value and read it back. A version number only +# implies that .parameter works; binding something and getting it back proves it +# on the binary actually in front of us, which is the operator's, not ours. +sqlite_probe_sql() { + printf '.parameter init\n' + printf '.parameter set :probe "cast(%s as text)"\n' "$(sql_hex_literal "$SQLITE_PROBE_TOKEN")" + printf 'SELECT :probe;\n' +} + +# Find a sqlite3 that can bind a parameter β€” in the container first, then on the +# host. Sets SQLITE_RUNNER; returns 1 if neither qualifies. There is deliberately +# no interpolating fallback: that would leave the vulnerable path in place under +# a nicer name. +# +# Probe stderr is collected rather than discarded, but only printed if BOTH +# probes fail. The container miss is the known-normal case β€” the app image has +# no sqlite3 (pure-Go driver, no CGO; Dockerfile:15) β€” so surfacing it on every +# run would be noise. +resolve_sqlite_runner() { + local probe out + probe=$(sqlite_probe_sql) + SQLITE_RUNNER="" + out=$(ssh_t "docker exec -i $(printf %q "$TARGET_CONTAINER") sqlite3 ${SQLITE_ARGS[*]} :memory:" \ + <<<"$probe" 2>>"$TMP/sqlite-probe.err") + if [[ "$out" == "$SQLITE_PROBE_TOKEN" ]]; then SQLITE_RUNNER="container"; return 0; fi + out=$(ssh_t "sqlite3 ${SQLITE_ARGS[*]} :memory:" <<<"$probe" 2>>"$TMP/sqlite-probe.err") + if [[ "$out" == "$SQLITE_PROBE_TOKEN" ]]; then SQLITE_RUNNER="host"; return 0; fi + return 1 +} + +# Run SQL from stdin against TARGET_DB_PATH via the resolved runner. Stderr is +# left alone so the caller can capture it, and the exit status is sqlite3's. +# The SQL crosses on stdin, so only the container name and db path still need +# printf %q for the remote shell. docker exec needs -i to attach stdin. +run_sqlite() { + case "$SQLITE_RUNNER" in + container) ssh_t "docker exec -i $(printf %q "$TARGET_CONTAINER") sqlite3 ${SQLITE_ARGS[*]} $(printf %q "$TARGET_DB_PATH")" ;; + host) ssh_t "sqlite3 ${SQLITE_ARGS[*]} $(printf %q "$TARGET_DB_PATH")" ;; + *) echo "run_sqlite: no runner resolved" >&2; return 127 ;; + esac +} + +# Read the retained-transmission count into RETAIN_COUNT. Prints a classified +# "retain-failed" line and returns 1 on failure, so Β§10.2 has exactly one place +# that increments $fails. +read_retain_count() { + RETAIN_COUNT="" + local code + if [[ -n "$ADMIN_API_TOKEN" ]]; then + # Read auth header from stdin so the token never enters argv (ps-safe). + code=$(printf 'header = "Authorization: Bearer %s"\n' "$ADMIN_API_TOKEN" | \ + curl -s -m "$CURL_TIMEOUT" -K - -o "$TMP/admin.json" -w "%{http_code}" \ + "$TARGET_URL/api/admin/transmissions?from_node=$TEST_PUBKEY&count=1" 2>/dev/null || echo "000") + if [[ "$code" == "200" ]]; then + RETAIN_COUNT=$(jq -r '.count // ((.transmissions // []) | length)' "$TMP/admin.json" 2>/dev/null || echo "") + fi + if [[ -n "$RETAIN_COUNT" ]]; then return 0; fi fi -fi -if [[ -z "$count" ]]; then + if [[ -z "$TARGET_DB_PATH" ]]; then echo " ❌ retain-failed: TARGET_DB_PATH unset and no ADMIN_API_TOKEN β€” cannot probe" + return 1 + fi + if ! resolve_sqlite_runner; then + echo " ❌ retain-failed: no sqlite3 able to bind a parameter on the target" + echo " tried: docker exec -i $TARGET_CONTAINER sqlite3, then sqlite3 on $TARGET_SSH_HOST" + echo " need: the sqlite3 CLI reachable over ssh, supporting '.parameter set'" + cat "$TMP/sqlite-probe.err" >&2 + return 1 + fi + echo " sqlite3 runner: $SQLITE_RUNNER" + if ! RETAIN_COUNT=$(run_sqlite <<<"$(transmission_count_sql "$TEST_PUBKEY")" 2>"$TMP/sqlite.err"); then + echo " ❌ retain-failed: sqlite3 query failed via $SQLITE_RUNNER" + cat "$TMP/sqlite.err" >&2 + RETAIN_COUNT="" + return 1 + fi + return 0 +} + +# ----------------------------------------------------------------------------- +# main +# ----------------------------------------------------------------------------- +main() { + BASELINE_URL="${1:-}" + TARGET_URL="${2:-}" + if [[ -z "$BASELINE_URL" || -z "$TARGET_URL" ]]; then + echo "usage: $0 BASELINE_URL TARGET_URL (TEST_NODE_PUBKEY+TARGET_* via env)" >&2 + exit 2 + fi + + TEST_PUBKEY="${TEST_NODE_PUBKEY:-}" + TARGET_SSH_HOST="${TARGET_SSH_HOST:-}" + TARGET_SSH_KEY="${TARGET_SSH_KEY:-/root/.ssh/id_ed25519}" + TARGET_CONFIG_PATH="${TARGET_CONFIG_PATH:-}" + TARGET_CONTAINER="${TARGET_CONTAINER:-}" + TARGET_DB_PATH="${TARGET_DB_PATH:-}" + ADMIN_API_TOKEN="${ADMIN_API_TOKEN:-}" + + if [[ -z "$TEST_PUBKEY" || -z "$TARGET_SSH_HOST" || -z "$TARGET_CONFIG_PATH" || -z "$TARGET_CONTAINER" ]]; then + echo "error: TEST_NODE_PUBKEY, TARGET_SSH_HOST, TARGET_CONFIG_PATH, TARGET_CONTAINER are required" >&2 + exit 2 + fi + + # Hard input validation β€” these strings are interpolated into the remote shell. + # Β§10.2's SQL binds TEST_PUBKEY as a parameter rather than interpolating it, so + # for the SQL layer this gate is defence in depth rather than the only guard + # (issue #1977). Keep it: redundant is not the same as wrong. + # Pubkey must be hex (MeshCore pubkeys are hex-encoded ed25519 prefixes). + if ! [[ "$TEST_PUBKEY" =~ ^[0-9a-fA-F]+$ ]]; then + echo "error: TEST_NODE_PUBKEY must be hex (got: redacted)" >&2 + exit 2 + fi + # Container name must match docker's allowed chars: [a-zA-Z0-9][a-zA-Z0-9_.-]* + if ! [[ "$TARGET_CONTAINER" =~ ^[a-zA-Z0-9][a-zA-Z0-9_.-]*$ ]]; then + echo "error: TARGET_CONTAINER has illegal chars" >&2 + exit 2 + fi + # Config path must be an absolute, sane path (no spaces, quotes, $, ;, etc.). + if ! [[ "$TARGET_CONFIG_PATH" =~ ^/[A-Za-z0-9_./-]+$ ]]; then + echo "error: TARGET_CONFIG_PATH must be a sane absolute path" >&2 + exit 2 + fi + if [[ -n "$TARGET_DB_PATH" ]] && ! [[ "$TARGET_DB_PATH" =~ ^/[A-Za-z0-9_./-]+$ ]]; then + echo "error: TARGET_DB_PATH must be a sane absolute path" >&2 + exit 2 + fi + + CURL_TIMEOUT="${CURL_TIMEOUT:-60}" + RESTART_WAIT_S="${RESTART_WAIT_S:-120}" + + SSH_OPTS=(-i "$TARGET_SSH_KEY" -o StrictHostKeyChecking=accept-new -o ConnectTimeout=15 -o BatchMode=yes) + + TMP=$(mktemp -d) + fails=0 + TEARDOWN_DONE=0 + trap teardown EXIT INT TERM + + # --------------------------------------------------------------------------- + # Β§10.1 β€” hide + # --------------------------------------------------------------------------- + echo "=== Β§10.1 add $TEST_PUBKEY to nodeBlacklist ===" + if ! add_to_blacklist; then fails=$((fails+1)); exit "$fails"; fi + if ! restart_target; then fails=$((fails+1)); exit "$fails"; fi + if ! wait_for_stats; then fails=$((fails+1)); exit "$fails"; fi + + detail_code=$(fetch_code "$TARGET_URL/api/nodes/$TEST_PUBKEY" "$TMP/detail.json") + list_code=$(fetch_code "$TARGET_URL/api/nodes?limit=10000" "$TMP/list.json") + in_list=0 + if [[ "$list_code" == "200" ]] && grep -qF -- "\"$TEST_PUBKEY\"" "$TMP/list.json"; then + in_list=1 + fi + if [[ "$detail_code" == "404" || "$in_list" == "0" ]]; then + echo " βœ… hide ok: detail=$detail_code in_list=$in_list" + else + echo " ❌ hide-failed: detail=$detail_code in_list=$in_list β€” pubkey still surfaced" + fails=$((fails+1)) + fi + + topo_code=$(fetch_code "$TARGET_URL/api/topology" "$TMP/topo.json") + if [[ "$topo_code" != "200" ]]; then + echo " ⚠️ /api/topology HTTP $topo_code β€” skipping topology assertion" + elif grep -qF -- "$TEST_PUBKEY" "$TMP/topo.json"; then + echo " ❌ hide-failed: /api/topology references blacklisted pubkey" fails=$((fails+1)) else - # TEST_PUBKEY is hex-validated β†’ safe to inline single-quoted in SQL. - # Container/db path also validated; printf %q for defense in depth. - q="SELECT COUNT(*) FROM transmissions WHERE from_node = '$TEST_PUBKEY';" - qq=$(printf %q "$q") - if ! count=$(ssh_t "docker exec $(printf %q "$TARGET_CONTAINER") sqlite3 $(printf %q "$TARGET_DB_PATH") $qq" 2>/dev/null); then - count=$(ssh_t "sqlite3 $(printf %q "$TARGET_DB_PATH") $qq" 2>/dev/null || echo "") - fi + echo " βœ… topology clean" fi -fi -if [[ -z "$count" ]]; then - echo " ❌ retain-failed: could not read transmissions count" - fails=$((fails+1)) -elif [[ "$count" =~ ^[0-9]+$ ]] && (( count > 0 )); then - echo " βœ… DB retains $count packets from $TEST_PUBKEY" -else - echo " ❌ retain-failed: count=$count (expected > 0)" - fails=$((fails+1)) -fi + # --------------------------------------------------------------------------- + # Β§10.2 β€” DB retain + # --------------------------------------------------------------------------- + echo "=== Β§10.2 verify packets retained in DB ===" + if ! read_retain_count; then + # read_retain_count already printed the classified reason. Counting here and + # nowhere else: the old code incremented $fails for the "TARGET_DB_PATH + # unset" case and then again for the empty count it left behind. + fails=$((fails+1)) + elif [[ "$RETAIN_COUNT" =~ ^[0-9]+$ ]] && (( RETAIN_COUNT > 0 )); then + echo " βœ… DB retains $RETAIN_COUNT packets from $TEST_PUBKEY" + else + echo " ❌ retain-failed: count=$RETAIN_COUNT (expected > 0)" + fails=$((fails+1)) + fi + + echo "=== summary: $fails failure(s) before teardown ===" + # trap handles teardown + exit + exit "$fails" +} -echo "=== summary: $fails failure(s) before teardown ===" -# trap handles teardown + exit -exit "$fails" +# Only run main when executed directly (not when sourced by tests). +if [ "${BASH_SOURCE[0]}" = "${0}" ]; then + main "$@" +fi diff --git a/qa/scripts/test-blacklist-sql.sh b/qa/scripts/test-blacklist-sql.sh new file mode 100755 index 000000000..770d6e065 --- /dev/null +++ b/qa/scripts/test-blacklist-sql.sh @@ -0,0 +1,135 @@ +#!/usr/bin/env bash +# test-blacklist-sql.sh β€” unit tests for the Β§10.2 SQL construction in +# qa/scripts/blacklist-test.sh (issue #1977). Sources the script and exercises +# its pure helpers, plus a real local sqlite3 against a throwaway fixture DB. +# +# Run: bash qa/scripts/test-blacklist-sql.sh +# Exits non-zero if any case fails. +# +# The point of the sqlite3 group is that BOTH directions are asserted. A test +# that only checks "the injection payload returns 0" passes just as happily when +# the query is silently broken and returns 0 for everything, so the legitimate +# pubkey must be shown to still return the row it should. + +set -uo pipefail + +SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd)" +# shellcheck source=blacklist-test.sh +. "$SCRIPT_DIR/blacklist-test.sh" + +PASS=0 +FAIL=0 + +assert_eq() { + local label="$1" expected="$2" actual="$3" + if [ "$expected" = "$actual" ]; then + PASS=$((PASS + 1)) + else + FAIL=$((FAIL + 1)) + echo "FAIL: $label β€” expected '$expected' got '$actual'" >&2 + fi +} + +assert_match() { + local label="$1" pattern="$2" actual="$3" + if [[ "$actual" =~ $pattern ]]; then + PASS=$((PASS + 1)) + else + FAIL=$((FAIL + 1)) + echo "FAIL: $label β€” '$actual' does not match /$pattern/" >&2 + fi +} + +# ----- sql_hex_literal ------------------------------------------------------ +# The security property: whatever goes in, the SQL text it produces is drawn +# from [0-9a-f] only. No caller-supplied byte can close a string literal or add +# a dot-command argument. Needs no sqlite3, so this group always runs. +assert_eq "hex of deadbeef" "x'6465616462656566'" "$(sql_hex_literal deadbeef)" +assert_eq "hex of empty" "x''" "$(sql_hex_literal "")" + +HEX_ONLY="^x'[0-9a-f]*'\$" +assert_match "alphabet: sql quote payload" "$HEX_ONLY" "$(sql_hex_literal "' OR 1=1 --")" +assert_match "alphabet: drop table" "$HEX_ONLY" "$(sql_hex_literal '"; DROP TABLE transmissions; --')" +assert_match "alphabet: backslash" "$HEX_ONLY" "$(sql_hex_literal 'a\b')" +assert_match "alphabet: dollar and backtick" "$HEX_ONLY" "$(sql_hex_literal '$(id) `id`')" +assert_match "alphabet: embedded newline" "$HEX_ONLY" "$(sql_hex_literal "$(printf 'a\nb')")" +assert_match "alphabet: multibyte" "$HEX_ONLY" "$(sql_hex_literal 'hΓ©llo')" + +# `od` without -v collapses runs of identical lines to '*'. A long repetitive +# value is the case that catches losing the flag. +LONG=$(printf 'x%.0s' $(seq 1 4096)) +LONG_HEX=$(sql_hex_literal "$LONG") +assert_match "alphabet: 4096 repeated bytes" "$HEX_ONLY" "$LONG_HEX" +# 4096 bytes β†’ 8192 hex digits, plus the 3 chars of x''. A collapsed run would +# be far shorter and would also fail the alphabet check on '*'. +assert_eq "no od line-collapse in 4096-byte value" "8192" "$(( ${#LONG_HEX} - 3 ))" + +# ----- against a real sqlite3 ---------------------------------------------- +if ! command -v sqlite3 >/dev/null 2>&1; then + echo "SKIP: sqlite3 not on PATH β€” skipping the ${#SQLITE_ARGS[@]}-flag query group" >&2 + echo " (the alphabet assertions above still ran)" >&2 +else + FIXTURE_DIR=$(mktemp -d) + trap 'rm -rf "$FIXTURE_DIR"' EXIT + DB="$FIXTURE_DIR/fixture.db" + EMPTY_DB="$FIXTURE_DIR/no-table.db" + sqlite3 "$DB" \ + "CREATE TABLE transmissions(from_node TEXT); INSERT INTO transmissions VALUES('deadbeef'),('cafebabe');" + sqlite3 "$EMPTY_DB" "CREATE TABLE unrelated(x);" + + run_local() { sqlite3 "${SQLITE_ARGS[@]}" "$1"; } + + # The capability probe must round-trip on this machine, or the assertions + # below would be testing nothing. + assert_eq "probe round-trips" "$SQLITE_PROBE_TOKEN" "$(sqlite_probe_sql | run_local :memory:)" + + # POSITIVE CONTROL: a legitimate pubkey still returns its row. Without this, + # a silently broken query looks like a passing security fix. + out=$(transmission_count_sql deadbeef | run_local "$DB"); rc=$? + assert_eq "legit pubkey β†’ its row" "1" "$out" + assert_eq "legit pubkey β†’ exit 0" "0" "$rc" + assert_eq "other legit pubkey" "1" "$(transmission_count_sql cafebabe | run_local "$DB")" + assert_eq "absent pubkey β†’ 0" "0" "$(transmission_count_sql abc123 | run_local "$DB")" + + # NEGATIVE: the payload binds as a literal that matches nothing. The table + # holds 2 rows, so a structural injection would return 2, not 0. + out=$(transmission_count_sql "' OR 1=1 --" | run_local "$DB"); rc=$? + assert_eq "injection payload β†’ 0 rows" "0" "$out" + assert_eq "injection payload β†’ exit 0" "0" "$rc" + assert_eq "table really does hold 2 rows" "2" \ + "$(run_local "$DB" <<<'SELECT COUNT(*) FROM transmissions;')" + + # Interpolating the same payload the old way returns the whole table. This is + # the behaviour the change removes; asserting it keeps the test honest about + # what "0" above is worth. + legacy="SELECT COUNT(*) FROM transmissions WHERE from_node = '' OR 1=1 --';" + assert_eq "old interpolated form leaked the table" "2" "$(run_local "$DB" <<<"$legacy")" + + # Multibyte and whitespace values bind as themselves rather than erroring. + sqlite3 "$DB" "INSERT INTO transmissions VALUES('hΓ©llo wΓΆrld');" + assert_eq "multibyte value with a space binds" "1" \ + "$(transmission_count_sql 'hΓ©llo wΓΆrld' | run_local "$DB")" + + # ERROR SURFACING: a broken query must be distinguishable from an empty + # result β€” non-zero exit and something on stderr, not a silent "". + err_file="$FIXTURE_DIR/err" + out=$(transmission_count_sql deadbeef | run_local "$EMPTY_DB" 2>"$err_file"); rc=$? + if [ "$rc" -ne 0 ]; then PASS=$((PASS + 1)); else + FAIL=$((FAIL + 1)); echo "FAIL: missing table β€” expected non-zero exit, got $rc" >&2 + fi + if [ -s "$err_file" ]; then PASS=$((PASS + 1)); else + FAIL=$((FAIL + 1)); echo "FAIL: missing table β€” expected a message on stderr" >&2 + fi + assert_eq "missing table β†’ no count on stdout" "" "$out" + + # run_sqlite with no resolved runner must refuse rather than guess. + SQLITE_RUNNER="" + if run_sqlite /dev/null 2>&1; then + FAIL=$((FAIL + 1)); echo "FAIL: run_sqlite with no runner β€” expected non-zero exit" >&2 + else + PASS=$((PASS + 1)) + fi +fi + +echo "test-blacklist-sql.sh: $PASS passed, $FAIL failed" +[ "$FAIL" -eq 0 ] From 6e4929b290d757957c37646f5507d7e2b1031d63 Mon Sep 17 00:00:00 2001 From: efiten Date: Fri, 11 Sep 2026 10:58:56 +0200 Subject: [PATCH 23/26] test(ingestor): join the watchdog loop goroutine instead of only asking it to stop (#2003) Verified before merging: on upstream/master `go test ./cmd/ingestor -run TestMQTTStallWatchdog -count=20` fails; on this branch the same command passes. The flake blocked CI on #2000. (cherry picked from commit 02feb2a88ef05284445f022eb87aca78273fbb4c) Co-Authored-By: Claude Opus 5 --- cmd/ingestor/mqtt_watchdog_1749_test.go | 23 ++++----------- cmd/ingestor/mqtt_watchdog_1810_test.go | 6 ++-- .../mqtt_watchdog_testhelpers_test.go | 29 +++++++++++++++++++ 3 files changed, 37 insertions(+), 21 deletions(-) diff --git a/cmd/ingestor/mqtt_watchdog_1749_test.go b/cmd/ingestor/mqtt_watchdog_1749_test.go index 105c18ad2..43e407829 100644 --- a/cmd/ingestor/mqtt_watchdog_1749_test.go +++ b/cmd/ingestor/mqtt_watchdog_1749_test.go @@ -58,15 +58,8 @@ func TestMQTTStallWatchdog_EscalateOnPersistentDisconnect_1749(t *testing.T) { t.Fatalf("setup: %v", err) } - tick := make(chan time.Time) - done := make(chan struct{}) - defer close(done) - - exited := make(chan struct{}) - go func() { - runLivenessWatchdogLoop(tick, done, threshold, func(args ...any) {}) - close(exited) - }() + tick, stopLoop := startWatchdogTestLoop(t, threshold, func(args ...any) {}) + defer stopLoop() // Feed ticks spanning > (multiplier Γ— threshold) of wall clock so // the escalation path fires. We control the `now` parameter by @@ -114,10 +107,8 @@ func TestMQTTStallWatchdog_DisconnectedEscalationThrottled_1749(t *testing.T) { t.Fatalf("setup: %v", err) } - tick := make(chan time.Time) - done := make(chan struct{}) - defer close(done) - go runLivenessWatchdogLoop(tick, done, threshold, func(args ...any) {}) + tick, stopLoop := startWatchdogTestLoop(t, threshold, func(args ...any) {}) + defer stopLoop() base := time.Now() // Pre-stamp DisconnectedSinceUnix so that the first tick is @@ -259,10 +250,8 @@ func TestMQTTStallWatchdog_LastTickUnixExposed_1749(t *testing.T) { watchdogLastTickUnix.Store(before) }) - tick := make(chan time.Time) - done := make(chan struct{}) - defer close(done) - go runLivenessWatchdogLoop(tick, done, time.Minute, func(args ...any) {}) + tick, stopLoop := startWatchdogTestLoop(t, time.Minute, func(args ...any) {}) + defer stopLoop() stamp := time.Now().Add(48 * time.Hour) // guaranteed > before select { diff --git a/cmd/ingestor/mqtt_watchdog_1810_test.go b/cmd/ingestor/mqtt_watchdog_1810_test.go index aeefd4a0f..b68fa75d3 100644 --- a/cmd/ingestor/mqtt_watchdog_1810_test.go +++ b/cmd/ingestor/mqtt_watchdog_1810_test.go @@ -212,10 +212,8 @@ func TestWatchdog_EscalationWarnThrottled_1810(t *testing.T) { } } - tick := make(chan time.Time) - done := make(chan struct{}) - defer close(done) - go runLivenessWatchdogLoop(tick, done, threshold, emit) + tick, stopLoop := startWatchdogTestLoop(t, threshold, emit) + defer stopLoop() base := time.Now() // First tick: stamps DisconnectedSinceUnix (no escalation yet). diff --git a/cmd/ingestor/mqtt_watchdog_testhelpers_test.go b/cmd/ingestor/mqtt_watchdog_testhelpers_test.go index 348c2f2d1..1db9b99a5 100644 --- a/cmd/ingestor/mqtt_watchdog_testhelpers_test.go +++ b/cmd/ingestor/mqtt_watchdog_testhelpers_test.go @@ -1,6 +1,7 @@ package main import ( + "sync" "testing" "time" ) @@ -45,3 +46,31 @@ func sendTickOrFail(t *testing.T, tick chan<- time.Time, stamp time.Time, timeou t.Fatalf("%s: tick blocked after %s β€” loop dead?", label, timeout) } } + +// startWatchdogTestLoop is setupWatchdogTestLoop for the common case: a test +// that wants the loop running for its own duration and nothing more. The +// returned stop closes done AND waits for the loop goroutine to return. It is +// safe to call more than once. +// +// Waiting is the part that must not be skipped. Closing done only asks the +// loop to stop; the goroutine can still be inside a scan, and that scan walks +// the package-level livenessRegistry. The next test registers its own source +// there, so a loop that has not returned yet will process that source on a +// tick carrying the previous test's fabricated clock. +// +// That is measured, not theoretical. With four call sites closing done and +// walking away, TestMQTTStallWatchdog_DisconnectedEscalationThrottled_1749 +// failed 2 to 3 times per 20 runs of the watchdog tests and never once in 50 +// runs on its own. Debug tracing showed two loops reaching maybeForceReconnect +// for the same source with tick clocks 420s apart, both reading +// LastForceReconnectUnix as 0 before either wrote it, so the throttle the test +// asserts on let both through. +func startWatchdogTestLoop(t *testing.T, threshold time.Duration, emit func(...any)) (tick chan time.Time, stop func()) { + t.Helper() + tick, done, exited := setupWatchdogTestLoop(t, threshold, emit) + var once sync.Once + return tick, func() { + once.Do(func() { close(done) }) + <-exited + } +} From dcac010e263917bdd00f231dcadae27224753651 Mon Sep 17 00:00:00 2001 From: liquidraver <504870+liquidraver@users.noreply.github.com> Date: Fri, 11 Sep 2026 11:29:49 +0200 Subject: [PATCH 24/26] fix(ingestor): delete aged packets in bounded batches so prune stops stalling ingest (#2000) Reviewed at ecf0b371: query plans dumped and confirmed index-driven for all three statements, termination proven against concurrent ingest (first_seen is always time.Now()), FK child-first ordering required and correct, writer-stats assertions non-racy. Two low findings noted on the PR for follow-up: the dropped RowsAffected error now gates the loop, and ~0.53s batches will trip defaultSlowWriterMs=500. (cherry picked from commit fe37f1060cdf3f260943ad1cd15d3ed4ad176e48) Co-Authored-By: Claude Opus 5 --- cmd/ingestor/maintenance.go | 96 ++++++++-- cmd/ingestor/prune_chunked_test.go | 286 +++++++++++++++++++++++++++++ 2 files changed, 362 insertions(+), 20 deletions(-) create mode 100644 cmd/ingestor/prune_chunked_test.go diff --git a/cmd/ingestor/maintenance.go b/cmd/ingestor/maintenance.go index fea189fe5..fbcbda716 100644 --- a/cmd/ingestor/maintenance.go +++ b/cmd/ingestor/maintenance.go @@ -10,42 +10,98 @@ import ( "github.com/meshcore-analyzer/dbschema" ) +// pruneBatchTransmissions bounds how many transmissions (and their child +// observations) one prune transaction deletes before it commits and +// releases the writer lock. +// +// Releasing between batches is the entire point. writerMu serialises every +// wrapped writer call, so while the prune holds it the MQTT ingest path is +// blocked. Go's sync.Mutex switches to FIFO handoff once a waiter has been +// blocked for 1ms (starvation mode), so an ingest goroutine queued behind +// the prune is served at the next batch boundary instead of after the whole +// retention day. +// +// The bound is on transmissions, but hold time scales with the rows actually +// deleted, and each transmission carries an unbounded number of observations. +// At ~16 observations per transmission a batch is ~4k row deletes and a few +// hundred milliseconds; an instance with a denser observation ratio gets a +// proportionally longer hold from the same batch size. 250 also keeps the +// commit count low (a 16k-transmission day is 64 transactions, not 16k). +const pruneBatchTransmissions = 250 + +// pruneAgedTransmissionIDs selects the next batch of transmissions older than +// the cutoff. Both statements of a batch embed it, so they resolve the same +// set: nothing modifies `transmissions` between them inside the transaction. +// +// The ORDER BY must be satisfiable from idx_transmissions_first_seen. That +// index carries the rowid as its tiebreaker, so "first_seen, id" is walked +// straight off it and the LIMIT stays deterministic even when timestamps tie. +// Ordering by id alone looks equivalent but makes SQLite abandon the index for +// a rowid SCAN. That is harmless while rows are being deleted β€” the oldest +// rows have the lowest rowids and match at once β€” but the batch that finds +// nothing, which is the steady state whenever nothing has aged out, walks the +// whole table under writerMu. TestPruneAgedTransmissionIDsUsesFirstSeenIndex +// pins the plan. +const pruneAgedTransmissionIDs = `SELECT id FROM transmissions WHERE first_seen < ? ORDER BY first_seen, id LIMIT ?` + +// The two statements of one prune batch. Child observations go first (no +// CASCADE in SQLite). +const ( + pruneObservationsBatch = `DELETE FROM observations WHERE transmission_id IN (` + pruneAgedTransmissionIDs + `)` + pruneTransmissionsBatch = `DELETE FROM transmissions WHERE id IN (` + pruneAgedTransmissionIDs + `)` +) + // PruneOldPackets deletes transmissions (and their child observations) // older than `days`. Returns count of transmissions deleted. // // Owned by the ingestor per #1283: the writer process is the only one // allowed to hold the DB write lock; previously this lived in // cmd/server/db.go and raced ingestor INSERTs (SQLITE_BUSY). +// +// Deletion is chunked into bounded transactions so ingest is never blocked +// for longer than a single batch. Deleting a whole retention day in one +// transaction held the writer lock for ~35s on a mesh doing ~260k +// observations/day, stalling MQTT ingest for the same duration. +// +// On error the transmissions deleted by already-committed batches are +// returned alongside it β€” those rows are gone, so reporting 0 would be +// wrong. func (s *Store) PruneOldPackets(days int) (int64, error) { if days <= 0 { return 0, nil } cutoff := time.Now().UTC().AddDate(0, 0, -days).Format(time.RFC3339) - // Tagged for writer-perf visibility (#1340). - var n int64 - err := s.WriterTx("prune_packets", func(tx *sql.Tx) error { - // Delete child observations first (no CASCADE in SQLite). - if _, err := tx.Exec(`DELETE FROM observations WHERE transmission_id IN ( - SELECT id FROM transmissions WHERE first_seen < ? - )`, cutoff); err != nil { - return fmt.Errorf("prune observations: %w", err) - } - - res, err := tx.Exec(`DELETE FROM transmissions WHERE first_seen < ?`, cutoff) + var total int64 + for { + var batch int64 + // Tagged for writer-perf visibility (#1340). + err := s.WriterTx("prune_packets", func(tx *sql.Tx) error { + if _, err := tx.Exec(pruneObservationsBatch, cutoff, pruneBatchTransmissions); err != nil { + return fmt.Errorf("prune observations: %w", err) + } + res, err := tx.Exec(pruneTransmissionsBatch, cutoff, pruneBatchTransmissions) + if err != nil { + return fmt.Errorf("prune transmissions: %w", err) + } + batch, _ = res.RowsAffected() + return nil + }) if err != nil { - return fmt.Errorf("prune transmissions: %w", err) + return total, err + } + total += batch + // A short batch proves nothing is left below the cutoff: the subquery + // found fewer rows than it was allowed to take. Only a batch that came + // back exactly full needs another pass. + if batch < pruneBatchTransmissions { + break } - n, _ = res.RowsAffected() - return nil - }) - if err != nil { - return 0, err } - if n > 0 { - log.Printf("[prune] deleted %d transmissions older than %d days", n, days) + if total > 0 { + log.Printf("[prune] deleted %d transmissions older than %d days", total, days) } - return n, nil + return total, nil } // PruneOldClientReceptions deletes mobile client-RX coverage rows older than diff --git a/cmd/ingestor/prune_chunked_test.go b/cmd/ingestor/prune_chunked_test.go new file mode 100644 index 000000000..a54130b00 --- /dev/null +++ b/cmd/ingestor/prune_chunked_test.go @@ -0,0 +1,286 @@ +package main + +import ( + "fmt" + "path/filepath" + "strings" + "testing" + "time" + + _ "modernc.org/sqlite" +) + +// seedAgedTransmissions inserts n transmissions stamped `age` days in the +// past, each with obsPerTx child observations, and returns nothing β€” the +// caller asserts against the store. Kept local to this file so the batching +// tests own their fixture shape. +func seedAgedTransmissions(t *testing.T, store *Store, n, obsPerTx, ageDays int) { + t.Helper() + ts := time.Now().UTC().AddDate(0, 0, -ageDays).Format(time.RFC3339) + for i := 0; i < n; i++ { + res, err := store.db.Exec( + `INSERT INTO transmissions (raw_hex, hash, first_seen, route_type, payload_type, payload_version, decoded_json) + VALUES (?, ?, ?, 0, 1, 1, '{}')`, + "AA", fmt.Sprintf("h%d-%d", ageDays, i), ts, + ) + if err != nil { + t.Fatalf("seed tx %d: %v", i, err) + } + txID, err := res.LastInsertId() + if err != nil { + t.Fatalf("seed tx %d LastInsertId: %v", i, err) + } + for j := 0; j < obsPerTx; j++ { + if _, err := store.db.Exec( + `INSERT INTO observations (transmission_id, observer_idx, direction, snr, rssi, score, path_json, timestamp) + VALUES (?, ?, 'rx', 1.0, -100, 0, '[]', ?)`, + txID, j, time.Now().Unix(), + ); err != nil { + t.Fatalf("seed obs %d/%d: %v", i, j, err) + } + } + } +} + +func countRows(t *testing.T, store *Store, table string) int { + t.Helper() + var n int + if err := store.db.QueryRow(`SELECT COUNT(*) FROM ` + table).Scan(&n); err != nil { + t.Fatalf("count %s: %v", table, err) + } + return n +} + +func openPruneStore(t *testing.T, name string) *Store { + t.Helper() + store, err := OpenStore(filepath.Join(t.TempDir(), name)) + if err != nil { + t.Fatalf("OpenStore: %v", err) + } + t.Cleanup(func() { store.Close() }) + return store +} + +// TestPruneOldPacketsDeletesInBoundedBatches is the perf assertion for the +// chunked prune: it enforces that a retention day is deleted across many +// bounded transactions rather than one long-held writer lock. +// +// The old implementation issued exactly ONE WriterTx regardless of row +// count, holding writerMu β€” and therefore blocking MQTT ingest β€” for the +// whole delete. Measured on a production instance doing ~260k +// observations/day that was prune_packets hold_ms_p50 = 33905, with +// mqtt_handler wait_ms_max = 35797. +// +// Counting prune_packets transactions is the deterministic proxy for that +// characteristic: N transactions means the lock was released N-1 times, so +// the worst-case ingest stall is one batch rather than the full day. +func TestPruneOldPacketsDeletesInBoundedBatches(t *testing.T) { + store := openPruneStore(t, "prune-batched.db") + + // Two full batches plus a partial one. The partial batch is what ends the + // loop, so no terminating empty batch should run. + const aged = pruneBatchTransmissions*2 + 37 + seedAgedTransmissions(t, store, aged, 2, 10) + + ResetWriterStatsForTest() + + n, err := store.PruneOldPackets(5) + if err != nil { + t.Fatalf("PruneOldPackets: %v", err) + } + if n != aged { + t.Fatalf("expected %d transmissions pruned, got %d", aged, n) + } + + // 2 full batches + 1 partial batch, which proves nothing is left. + wantTx := int64(3) + got := store.WriterStatsSnapshot()["prune_packets"] + if got.Count != wantTx { + t.Fatalf("expected %d prune_packets transactions for %d rows at batch size %d, got %d "+ + "(1 means the delete was not chunked and holds the writer lock for the whole retention day)", + wantTx, aged, pruneBatchTransmissions, got.Count) + } + + if remaining := countRows(t, store, "transmissions"); remaining != 0 { + t.Fatalf("expected all aged transmissions gone, %d remain", remaining) + } + if remaining := countRows(t, store, "observations"); remaining != 0 { + t.Fatalf("expected all child observations gone, %d remain", remaining) + } +} + +// TestPruneOldPacketsExactMultipleTerminates covers the one case the +// short-batch exit cannot catch on its own: when the aged rows are an exact +// multiple of the batch size, the last batch comes back full, so the loop +// must run one more β€” empty β€” batch to learn that nothing is left. +func TestPruneOldPacketsExactMultipleTerminates(t *testing.T) { + store := openPruneStore(t, "prune-exact.db") + + const aged = pruneBatchTransmissions * 2 + seedAgedTransmissions(t, store, aged, 1, 10) + + ResetWriterStatsForTest() + + n, err := store.PruneOldPackets(5) + if err != nil { + t.Fatalf("PruneOldPackets: %v", err) + } + if n != aged { + t.Fatalf("expected %d pruned, got %d", aged, n) + } + + // 2 full batches + 1 empty batch that finds nothing and ends the loop. + if got := store.WriterStatsSnapshot()["prune_packets"].Count; got != 3 { + t.Fatalf("expected 3 prune_packets transactions for an exact multiple of the batch size, got %d", got) + } + if remaining := countRows(t, store, "transmissions"); remaining != 0 { + t.Fatalf("expected all aged transmissions gone, %d remain", remaining) + } + if remaining := countRows(t, store, "observations"); remaining != 0 { + t.Fatalf("expected all child observations gone, %d remain", remaining) + } +} + +// TestPruneOldPacketsSpansBatchesAndKeepsFreshRows covers the correctness +// risk the batching introduces: with a LIMIT on both statements, a cutoff +// that straddles several batches must still delete every aged row and no +// fresh one, and must not orphan observations. +func TestPruneOldPacketsSpansBatchesAndKeepsFreshRows(t *testing.T) { + store := openPruneStore(t, "prune-mixed.db") + + const aged = pruneBatchTransmissions + 11 + const fresh = 17 + const obsPerTx = 3 + seedAgedTransmissions(t, store, aged, obsPerTx, 10) + seedAgedTransmissions(t, store, fresh, obsPerTx, 0) + + n, err := store.PruneOldPackets(5) + if err != nil { + t.Fatalf("PruneOldPackets: %v", err) + } + if n != aged { + t.Fatalf("expected %d pruned, got %d", aged, n) + } + + if remaining := countRows(t, store, "transmissions"); remaining != fresh { + t.Fatalf("expected %d fresh transmissions kept, got %d", fresh, remaining) + } + if remaining := countRows(t, store, "observations"); remaining != fresh*obsPerTx { + t.Fatalf("expected %d observations kept, got %d", fresh*obsPerTx, remaining) + } + + // No observation may reference a transmission that is gone. + var orphans int + if err := store.db.QueryRow(` + SELECT COUNT(*) FROM observations o + LEFT JOIN transmissions t ON t.id = o.transmission_id + WHERE t.id IS NULL`).Scan(&orphans); err != nil { + t.Fatalf("orphan check: %v", err) + } + if orphans != 0 { + t.Fatalf("expected no orphaned observations, got %d", orphans) + } +} + +// TestPruneOldPacketsDisabledTakesNoWriterLock pins the 0/negative +// short-circuit: retention disabled must not open a transaction at all. +func TestPruneOldPacketsDisabledTakesNoWriterLock(t *testing.T) { + store := openPruneStore(t, "prune-disabled.db") + seedAgedTransmissions(t, store, 5, 1, 10) + + ResetWriterStatsForTest() + + for _, days := range []int{0, -1} { + n, err := store.PruneOldPackets(days) + if err != nil { + t.Fatalf("PruneOldPackets(%d): %v", days, err) + } + if n != 0 { + t.Fatalf("PruneOldPackets(%d) = %d, want 0", days, n) + } + } + + if got := store.WriterStatsSnapshot()["prune_packets"].Count; got != 0 { + t.Fatalf("expected no prune_packets transactions when retention is disabled, got %d", got) + } + if remaining := countRows(t, store, "transmissions"); remaining != 5 { + t.Fatalf("expected 5 transmissions untouched, got %d", remaining) + } +} + +// TestPruneOldPacketsNothingToDeleteRunsOneEmptyBatch documents the +// steady-state cost when nothing has aged out yet: a single empty batch, then +// the loop exits. That batch is only cheap because the subquery is walked off +// idx_transmissions_first_seen β€” see +// TestPruneAgedTransmissionIDsUsesFirstSeenIndex. +func TestPruneOldPacketsNothingToDeleteRunsOneEmptyBatch(t *testing.T) { + store := openPruneStore(t, "prune-noop.db") + seedAgedTransmissions(t, store, 9, 2, 0) + + ResetWriterStatsForTest() + + n, err := store.PruneOldPackets(5) + if err != nil { + t.Fatalf("PruneOldPackets: %v", err) + } + if n != 0 { + t.Fatalf("expected 0 pruned, got %d", n) + } + if got := store.WriterStatsSnapshot()["prune_packets"].Count; got != 1 { + t.Fatalf("expected exactly 1 empty prune_packets transaction, got %d", got) + } + if remaining := countRows(t, store, "transmissions"); remaining != 9 { + t.Fatalf("expected 9 transmissions kept, got %d", remaining) + } +} + +// TestPruneAgedTransmissionIDsUsesFirstSeenIndex pins the query plan of the +// batch subquery and of both statements that embed it. +// +// Ordering the batch by id instead of first_seen makes SQLite drop +// idx_transmissions_first_seen for a rowid SCAN. On a 1M-row table that took +// the terminating, nothing-left batch from ~10Β΅s to ~73ms β€” once per statement, +// under writerMu, in the state an instance is in whenever nothing has aged out. +// Transaction counts cannot see that regression, so the plan is the assertion. +func TestPruneAgedTransmissionIDsUsesFirstSeenIndex(t *testing.T) { + store := openPruneStore(t, "prune-plan.db") + seedAgedTransmissions(t, store, 20, 2, 10) + + cutoff := time.Now().UTC().AddDate(0, 0, -5).Format(time.RFC3339) + for name, q := range map[string]string{ + "batch subquery": pruneAgedTransmissionIDs, + "observations delete": pruneObservationsBatch, + "transmissions delete": pruneTransmissionsBatch, + } { + rows, err := store.db.Query("EXPLAIN QUERY PLAN "+q, cutoff, pruneBatchTransmissions) + if err != nil { + t.Fatalf("%s: EXPLAIN QUERY PLAN: %v", name, err) + } + var steps []string + for rows.Next() { + var id, parent, notused int + var detail string + if err := rows.Scan(&id, &parent, ¬used, &detail); err != nil { + rows.Close() + t.Fatalf("%s: scan plan row: %v", name, err) + } + steps = append(steps, detail) + } + if err := rows.Err(); err != nil { + rows.Close() + t.Fatalf("%s: plan rows: %v", name, err) + } + rows.Close() + plan := strings.Join(steps, " | ") + + if !strings.Contains(plan, "idx_transmissions_first_seen") { + t.Errorf("%s: plan does not use idx_transmissions_first_seen: %s", name, plan) + } + if strings.Contains(plan, "SCAN transmissions") { + t.Errorf("%s: plan scans transmissions, so the empty terminating batch walks the whole table under writerMu: %s", name, plan) + } + if strings.Contains(plan, "TEMP B-TREE") { + t.Errorf("%s: plan sorts in a temp b-tree instead of walking the index in order: %s", name, plan) + } + } +} From abef6d3ad207c1a4f440a5eb131f0dd6fcec7e39 Mon Sep 17 00:00:00 2001 From: nullrouten0 Date: Sat, 12 Sep 2026 02:49:27 -0700 Subject: [PATCH 25/26] channels: 23 more names in the rainbow guess list (#2007) Verified: every value is SHA-256(name)[:16] per internal/channel.DeriveKey, 319/320 exact (Public is the fixed firmware default), no duplicate hashes, file parses at 320 entries. (cherry picked from commit c283f42c1fd1e43fc3dbd78ae6ed4ef4178bd067) Co-Authored-By: Claude Opus 5 --- channel-rainbow.json | 25 ++++++++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-) diff --git a/channel-rainbow.json b/channel-rainbow.json index 73ad3a237..23e1e7829 100644 --- a/channel-rainbow.json +++ b/channel-rainbow.json @@ -295,5 +295,28 @@ "#bogota": "6d0864985b64350ce4cbfebf4979e970", "#peru": "7e6fc347bf29a4c128ac3156865bd521", "#lima": "5f167ce354eca08ab742463df10ef255", - "Public": "8b3387e9c5cdea6ac9e5edbaa115cd72" + "Public": "8b3387e9c5cdea6ac9e5edbaa115cd72", + "#Public": "7534fe292203ccdce93db79377d4b566", + "#newbie": "ff2ecc59f59dbf051a10d880f0b4308d", + "#power": "c4d23a51329aeaee767234644d52cff8", + "#garden": "56ff6fe3500872c2765dbc178350005c", + "#jokes": "abe610d61dee508c0ac18793c36c41c3", + "#space": "5caa27568ba7a216bcc09fcb69c77ea1", + "#path": "29eed6c759d96346b30c869e23fd86f8", + "#hamradio": "83c8b01997654265938da8765cbc7db9", + "#amrron": "68ec397a1dd4f3416911d00211ed0ef6", + "#family": "7d1b805d4bf059c2ef06ed221794663b", + "#testing": "cde5e82cf515647dcb547a79a4f065d1", + "#wicked": "05e990d48943ee03c943c41dea0ddfe1", + "#gay": "3a612fa3bfc9f3b439cc230b01cbbc00", + "#t": "7b5336e85005be805b496bc27c722ff3", + "#doom": "7034910637d6b9b6c2cc3bcd8dfe9bb5", + "#empty": "3351cfa0dad99e1777cce210c983b561", + "#puppy": "c20cf85aab9561e6e65ac4a19a852198", + "#naked": "f423f23ddcf9e472c2ace38dce7591bc", + "#cruising": "550857053360e3492755251f9142e74c", + "#nightowls": "1411c5c3732d3df1b855644f78c47417", + "#worldcup": "d5786cc7bcee5a4828bbb20c3825c5a9", + "#nofilter": "cab31f589959debcd1721b7d252259b1", + "#politics": "64cc9fdaad67f03f23b4cec8c9a4468f" } From 44568c50b9d4f0c42b708a0a2ce0b31a3364c651 Mon Sep 17 00:00:00 2001 From: nullrouten0 Date: Sat, 12 Sep 2026 03:40:44 -0700 Subject: [PATCH 26/26] fix(packets): show browser-added channels in the channel filter (#2009) Verified against production: ingestor writes enc_ (db.go:2282), store.go:3536 filters on it, live values are uppercase two-digit hex with thousands of packets (enc_28: 2708), /api/channels omits them, and /api/packets?channel=enc_28 returns rows. New test passes at 14 and fails when the dedupe guard is disabled. (cherry picked from commit a2f039d4c7b212038a060f3b10412fdd4920b7ba) Co-Authored-By: Claude Opus 5 --- .github/workflows/deploy.yml | 1 + public/packets.js | 118 ++++++++++++++++-- test-all.sh | 1 + test-packets-local-channels.js | 222 +++++++++++++++++++++++++++++++++ 4 files changed, 329 insertions(+), 13 deletions(-) create mode 100644 test-packets-local-channels.js diff --git a/.github/workflows/deploy.yml b/.github/workflows/deploy.yml index 9082db010..567b418ae 100644 --- a/.github/workflows/deploy.yml +++ b/.github/workflows/deploy.yml @@ -103,6 +103,7 @@ jobs: node test-confidence-indicator.js node test-1659-analytics-warmup.js node test-channels-merge-1498-unit.js + node test-packets-local-channels.js node test-issue-1518-home-url.js node test-channel-decrypt-insecure-context.js node test-live-region-filter.js diff --git a/public/packets.js b/public/packets.js index e1ae22d9d..68c061a02 100644 --- a/public/packets.js +++ b/public/packets.js @@ -1067,6 +1067,92 @@ else _docColMenuCloseHandler = handler; } + // --- Locally-added channels in the channel filter --------------------- + // Channels the operator adds in their own browser (Channels page β†’ "Add + // channel") live only in localStorage β€” the keys never leave the client + // (channel-decrypt.js). The server therefore cannot decrypt their traffic + // and files it under the synthetic channel hash "enc_", which + // /api/channels omits. Result: a channel you just added is invisible in + // the packets channel picker. + // + // Fix: derive the same "enc_" value client-side from the stored key + // (SHA-256(key)[0], exactly what the ingestor writes) and offer those + // channels as extra options. /api/packets?channel=enc_ already + // filters on that value server-side, so no backend change is needed. + + /** + * Read the browser's stored channel keys and map each to the server-side + * channel-hash value its packets are stored under. + * Cost: one SHA-256 per stored key (a handful), once per page init. + * @returns {Promise>} + */ + async function collectLocalChannels() { + const CD = window.ChannelDecrypt; + if (!CD || typeof CD.getStoredKeys !== 'function') return []; + let keys; + try { keys = CD.getStoredKeys() || {}; } catch (e) { return []; } + const out = []; + for (const name of Object.keys(keys)) { + const keyHex = keys[name]; + if (!keyHex || typeof keyHex !== 'string') continue; + let hashByte; + try { + const keyBytes = CD.hexToBytes(keyHex); + if (!keyBytes || keyBytes.length !== 16) continue; + hashByte = await CD.computeChannelHash(keyBytes); + } catch (e) { continue; } + if (typeof hashByte !== 'number') continue; + const label = (typeof CD.getLabel === 'function' && CD.getLabel(name)) || name; + out.push({ + // Uppercase 2-digit hex β€” matches cmd/ingestor/decoder.go ("%02X"). + value: 'enc_' + hashByte.toString(16).padStart(2, '0').toUpperCase(), + name: name, + label: label + }); + } + return out; + } + + /** + * Merge the server channel list with locally-added ones into the two + * option groups the picker renders. Pure β€” unit-tested. + * A local channel is dropped when the server already exposes it (same + * hash value, or same name because the server holds the key too). + * @returns {{server: Array<{value:string,label:string}>, local: Array<{value:string,label:string}>}} + */ + function buildChannelOptions(serverChannels, localChannels) { + const byLabel = (a, b) => { + const an = (a.label || '').toLowerCase(); + const bn = (b.label || '').toLowerCase(); + return an < bn ? -1 : an > bn ? 1 : 0; + }; + const server = []; + const seenValues = new Set(); + const seenNames = new Set(); + for (const ch of serverChannels || []) { + const value = (ch && (ch.hash || ch.name)) || ''; + if (!value || seenValues.has(value)) continue; + seenValues.add(value); + seenNames.add(String(ch.name || value).toLowerCase()); + server.push({ value: value, label: ch.name || value }); + } + const local = []; + for (const lc of localChannels || []) { + if (!lc || !lc.value) continue; + if (seenValues.has(lc.value)) continue; + if (seenNames.has(String(lc.name || '').toLowerCase())) continue; + seenValues.add(lc.value); + local.push({ value: lc.value, label: lc.label || lc.name }); + } + return { server: server.sort(byLabel), local: local.sort(byLabel) }; + } + + // Exported for test-packets-local-channels.js (no DOM required). + if (typeof window !== 'undefined') { + window._packetsBuildChannelOptionsForTest = buildChannelOptions; + window._packetsCollectLocalChannelsForTest = collectLocalChannels; + } + function renderTimestampCell(isoString) { if (typeof formatTimestampWithTooltip !== 'function' || typeof getTimestampMode !== 'function') { return escapeHtml(typeof timeAgo === 'function' ? timeAgo(isoString) : 'β€”'); @@ -1851,31 +1937,37 @@ opt.selected = true; channelSel.appendChild(opt); } - api('/channels').then(data => { + Promise.all([ + api('/channels').catch(() => null), + collectLocalChannels() + ]).then(([data, localChannels]) => { const channels = (data && data.channels) || []; // Build options via DOM API: channel names are network-supplied // and must NOT be interpolated into innerHTML (XSS, #812). // Sort alphabetically (case-insensitive) for predictable picker order; // the API returns last-activity order which is unstable for a dropdown. - const sorted = channels.slice().sort((a, b) => { - const an = (a.name || a.hash || '').toLowerCase(); - const bn = (b.name || b.hash || '').toLowerCase(); - return an < bn ? -1 : an > bn ? 1 : 0; - }); + const groups = buildChannelOptions(channels, localChannels); channelSel.textContent = ''; const allOpt = document.createElement('option'); allOpt.value = ''; allOpt.textContent = 'All Channels'; channelSel.appendChild(allOpt); let matched = false; - for (const ch of sorted) { - const v = ch.hash || ch.name || ''; - if (!v) continue; + const addOption = (o, parent) => { const opt = document.createElement('option'); - opt.value = v; - opt.textContent = ch.name || v; - if (v === filters.channel) { opt.selected = true; matched = true; } - channelSel.appendChild(opt); + opt.value = o.value; + opt.textContent = o.label; + if (o.value === filters.channel) { opt.selected = true; matched = true; } + parent.appendChild(opt); + }; + for (const o of groups.server) addOption(o, channelSel); + // Browser-added channels the server can't see, grouped so it's + // obvious they come from keys stored in this browser only. + if (groups.local.length) { + const grp = document.createElement('optgroup'); + grp.label = 'My Channels (this browser)'; + for (const o of groups.local) addOption(o, grp); + channelSel.appendChild(grp); } // If current filter isn't in the list (encrypted hash, stale, or // race with cache), keep it as a selected option so the UI reflects state. diff --git a/test-all.sh b/test-all.sh index ca4c3847d..3262dc089 100755 --- a/test-all.sh +++ b/test-all.sh @@ -11,6 +11,7 @@ echo "" echo "── Unit Tests ──" node test-packet-filter.js node test-packet-filter-ux.js +node test-packets-local-channels.js node test-aging.js node test-issue-1065-gesture-hints-gates.js node test-frontend-helpers.js diff --git a/test-packets-local-channels.js b/test-packets-local-channels.js new file mode 100644 index 000000000..c73914a90 --- /dev/null +++ b/test-packets-local-channels.js @@ -0,0 +1,222 @@ +/** + * Unit tests for the packets channel-picker option merge. + * + * Locally-added channels (keys stored in the browser only) are invisible to + * the server, so /api/channels never lists them. packets.js derives their + * server-side "enc_" channel hash client-side and offers them as an + * extra option group. buildChannelOptions() does that merge; it is pure and + * exported as window._packetsBuildChannelOptionsForTest. + * + * Covers: sorting, server-list passthrough, local append, dedupe by hash + * value, dedupe by name (server holds the key too), and junk input. + */ +'use strict'; +const vm = require('vm'); +const fs = require('fs'); +const assert = require('assert'); + +// packets.js expects a browser at call time; the merge helper touches none +// of it. Load in a tolerant sandbox and grab the exported helper β€” it is +// assigned before any DOM-dependent code runs. +const noop = () => {}; +const fakeEl = { + addEventListener: noop, removeEventListener: noop, appendChild: noop, remove: noop, + querySelector: () => null, querySelectorAll: () => [], setAttribute: noop, + getAttribute: () => null, classList: { add: noop, remove: noop, toggle: noop, contains: () => false }, + style: {}, dataset: {}, textContent: '', innerHTML: '', +}; +const doc = { + readyState: 'complete', createElement: () => ({ ...fakeEl }), head: fakeEl, body: fakeEl, + getElementById: () => null, querySelector: () => null, querySelectorAll: () => [], + addEventListener: noop, removeEventListener: noop, +}; +const win = { + addEventListener: noop, removeEventListener: noop, + matchMedia: () => ({ matches: false, addEventListener: noop, removeEventListener: noop }), + // packets.js fails loud if this module is missing (#1799). + PayloadLabels: { SHORT_BY_ID: {} }, +}; +const ctx = { + window: win, document: doc, console, Date, Math, JSON, Set, Map, Array, Object, Promise, Error, + setTimeout, clearTimeout, setInterval, clearInterval, + history: { replaceState: noop, pushState: noop }, + location: { hash: '', href: '', pathname: '/' }, + navigator: { userAgent: 'node' }, + localStorage: { getItem: () => null, setItem: noop, removeItem: noop }, + registerPage: noop, + api: () => Promise.resolve({}), + fetch: () => Promise.resolve({ json: () => Promise.resolve({}) }), +}; +ctx.self = ctx; +vm.createContext(ctx); +try { + vm.runInContext(fs.readFileSync('public/packets.js', 'utf8'), ctx); +} catch (e) { + // Downstream init may throw on the stub DOM β€” fine, the helper is exported + // near the top of the module. +} + +const build = win._packetsBuildChannelOptionsForTest; +assert.strictEqual(typeof build, 'function', 'buildChannelOptions must be exported for tests'); + +// buildChannelOptions returns objects created inside the vm realm, whose +// prototypes differ from this realm's β€” deepStrictEqual would reject them on +// identity alone. Compare structurally. +const plain = (v) => JSON.parse(JSON.stringify(v)); + +let passed = 0; +function test(name, fn) { + fn(); + passed++; + console.log(' ok - ' + name); +} + +console.log('packets channel-picker option merge'); + +test('empty inputs produce empty groups', () => { + const r = build([], []); + assert.deepStrictEqual(plain(r), { server: [], local: [] }); + const r2 = build(null, undefined); + assert.deepStrictEqual(plain(r2), { server: [], local: [] }); +}); + +test('server channels are sorted case-insensitively by label', () => { + const r = build([{ hash: 'zeta', name: 'zeta' }, { hash: 'Alpha', name: 'Alpha' }, { hash: 'mid', name: 'mid' }], []); + assert.deepStrictEqual(plain(r.server.map(o => o.value)), ['Alpha', 'mid', 'zeta']); + assert.strictEqual(r.local.length, 0); +}); + +test('server channel without a name falls back to its hash', () => { + const r = build([{ hash: 'enc_A5' }], []); + assert.deepStrictEqual(plain(r.server), [{ value: 'enc_A5', label: 'enc_A5' }]); +}); + +test('local channels are appended as their own group', () => { + const r = build( + [{ hash: 'public', name: 'public' }], + [{ value: 'enc_A5', name: '#mytown', label: '#mytown' }] + ); + assert.deepStrictEqual(plain(r.server), [{ value: 'public', label: 'public' }]); + assert.deepStrictEqual(plain(r.local), [{ value: 'enc_A5', label: '#mytown' }]); +}); + +test('local channel is dropped when the server already lists that hash', () => { + const r = build( + [{ hash: 'enc_A5', name: 'enc_A5' }], + [{ value: 'enc_A5', name: '#mytown', label: '#mytown' }] + ); + assert.strictEqual(r.local.length, 0); +}); + +test('local channel is dropped when the server decrypts it under the same name', () => { + const r = build( + [{ hash: '#MyTown', name: '#MyTown' }], + [{ value: 'enc_A5', name: '#mytown', label: '#mytown' }] + ); + assert.strictEqual(r.local.length, 0, 'name match is case-insensitive'); +}); + +test('duplicate local entries collapse to one option', () => { + const r = build([], [ + { value: 'enc_A5', name: '#a', label: '#a' }, + { value: 'enc_A5', name: '#b', label: '#b' }, + ]); + assert.deepStrictEqual(plain(r.local), [{ value: 'enc_A5', label: '#a' }]); +}); + +test('local options are sorted and use their display label', () => { + const r = build([], [ + { value: 'enc_02', name: 'psk:deadbeef', label: 'Work PSK' }, + { value: 'enc_01', name: '#alpha', label: '#alpha' }, + ]); + assert.deepStrictEqual(plain(r.local), [ + { value: 'enc_01', label: '#alpha' }, + { value: 'enc_02', label: 'Work PSK' }, + ]); +}); + +test('valueless / junk entries are skipped', () => { + const r = build([{ name: '' }, null, {}], [null, { name: '#x' }, { value: '' }]); + assert.deepStrictEqual(plain(r), { server: [], local: [] }); +}); + +// --- collectLocalChannels(): stored keys β†’ "enc_" ------------------- +// Stubs ChannelDecrypt with the real crypto (node) so the derived value is +// checked against a genuine SHA-256(key)[0], not a fixture. + +const crypto = require('crypto'); +const collect = win._packetsCollectLocalChannelsForTest; +assert.strictEqual(typeof collect, 'function', 'collectLocalChannels must be exported for tests'); + +function fakeChannelDecrypt(keys, labels) { + return { + getStoredKeys: () => keys, + getLabel: (name) => (labels && labels[name]) || '', + hexToBytes: (hex) => Uint8Array.from(Buffer.from(hex, 'hex')), + computeChannelHash: async (keyBytes) => crypto.createHash('sha256').update(keyBytes).digest()[0], + }; +} + +function deriveKeyHex(channelName) { + return crypto.createHash('sha256').update(channelName).digest().subarray(0, 16).toString('hex'); +} + +function expectedEncValue(channelName) { + const key = crypto.createHash('sha256').update(channelName).digest().subarray(0, 16); + const hashByte = crypto.createHash('sha256').update(key).digest()[0]; + return 'enc_' + hashByte.toString(16).padStart(2, '0').toUpperCase(); +} + +async function asyncTest(name, fn) { + await fn(); + passed++; + console.log(' ok - ' + name); +} + +(async () => { + console.log('\ncollectLocalChannels'); + + await asyncTest('no ChannelDecrypt module β†’ no local channels', async () => { + win.ChannelDecrypt = undefined; + assert.deepStrictEqual(plain(await collect()), []); + }); + + await asyncTest('derives the server-side enc_ value from the stored key', async () => { + // '#alpha' and '#zulu' hash to different bytes; both must round-trip. + const keys = { '#alpha': deriveKeyHex('#alpha'), '#zulu': deriveKeyHex('#zulu') }; + win.ChannelDecrypt = fakeChannelDecrypt(keys); + const out = plain(await collect()); + assert.strictEqual(out.length, 2); + assert.deepStrictEqual(out[0], { value: expectedEncValue('#alpha'), name: '#alpha', label: '#alpha' }); + assert.deepStrictEqual(out[1], { value: expectedEncValue('#zulu'), name: '#zulu', label: '#zulu' }); + // Uppercase, zero-padded β€” matches cmd/ingestor/decoder.go's "%02X". + for (const o of out) assert.ok(/^enc_[0-9A-F]{2}$/.test(o.value), 'bad value ' + o.value); + }); + + await asyncTest('uses the stored display label when present', async () => { + win.ChannelDecrypt = fakeChannelDecrypt( + { 'psk:deadbeef': deriveKeyHex('#work') }, + { 'psk:deadbeef': 'Work PSK' } + ); + const out = plain(await collect()); + assert.deepStrictEqual(out, [{ value: expectedEncValue('#work'), name: 'psk:deadbeef', label: 'Work PSK' }]); + }); + + await asyncTest('skips malformed keys instead of throwing', async () => { + win.ChannelDecrypt = fakeChannelDecrypt({ + '#short': 'aabb', // not 16 bytes + '#empty': '', + '#null': null, + '#good': deriveKeyHex('#good'), + }); + const out = plain(await collect()); + assert.deepStrictEqual(out, [{ value: expectedEncValue('#good'), name: '#good', label: '#good' }]); + }); + + await asyncTest('a throwing getStoredKeys degrades to no local channels', async () => { + win.ChannelDecrypt = { getStoredKeys: () => { throw new Error('quota'); } }; + assert.deepStrictEqual(plain(await collect()), []); + }); + + console.log(`\n${passed} tests passed`); +})().catch((e) => { console.error(e); process.exit(1); });