mirror of
https://github.com/Kpa-clawbot/meshcore-analyzer.git
synced 2026-09-13 15:05:35 +00:00
Fixes #1888. ## The mismatch `/api/stats.totalObservers` and `/api/observers` counted different sets: | Source | Predicate | |---|---| | `cmd/server/store.go:2089` (store path) | `SELECT COUNT(*) FROM observers` — every row | | `cmd/server/db.go:336` (DB fallback) | `WHERE inactive IS NULL OR inactive = 0` | | `db.GetObservers()` → `/api/observers` | `WHERE inactive IS NULL OR inactive = 0` | `handleStats` uses the store path whenever a `PacketStore` exists (`routes.go:785`), which is every normal deployment. So the header count came from the unfiltered query while the Observers page listed the filtered set. The two stats implementations also disagreed with each other for the same database, which is a bug on its own. ## Reproduction The gap is exactly the observers the `observerDays` retention sweep has soft-deleted. On the instance I reproduced against: ``` GET /api/stats → totalObservers: 79 GET /api/observers → observers.length == 51 ``` ```sql SELECT 'all', COUNT(*) FROM observers -- 79 UNION ALL SELECT 'active', COUNT(*) FROM observers WHERE inactive IS NULL OR inactive = 0 -- 51 UNION ALL SELECT 'inactive', COUNT(*) FROM observers WHERE inactive = 1; -- 28 ``` 79 − 28 = 51. Same shape as the 82 vs 51 in the issue. ## The change One line: the store's stats query gets the same predicate the other two already use, so all three agree. ## Deliberately out of scope Two things the issue raises that this does **not** fix, called out so they are not mistaken for done: - **Config blacklist.** `buildObserversDefaultResponse` drops blacklisted observers in the handler loop (`routes.go:2752`), which no SQL count can see. A deployment with a non-empty `observerBlacklist` will still show a stats count higher than the list, by the number of blacklisted-but-live observers. Closing that needs config plumbing into the count and is a separate change — happy to follow up if wanted. - **Map controls.** The third surface named in the issue derives its count from node role aggregates (`roleCounts`), not from the observer set at all. That is a frontend concern and untouched here. ## Tests `cmd/server/observer_count_1888_test.go`, three cases, each watched fail first: 1. `TestStoreStatsTotalObserversExcludesSoftDeleted` — `TotalObservers = 5, want 4` 2. `TestStoreStatsTotalObserversMatchesObserverList` — `stats totalObservers = 5 but /api/observers lists 4` 3. `TestStoreAndDBStatsAgreeOnTotalObservers` — `store path reports 5 observers, DB fallback reports 4` The fixture includes a row with `inactive = NULL` alongside `inactive = 0` and `inactive = 1`, since `GetObservers` treats NULL as live and only the `1` may be excluded. `cd cmd/server && go test ./...` → ok (87s). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -0,0 +1,115 @@
|
||||
package main
|
||||
|
||||
import (
|
||||
"testing"
|
||||
"time"
|
||||
)
|
||||
|
||||
// #1888: /api/stats.totalObservers and /api/observers disagreed because they
|
||||
// counted different sets. GetObservers() (and therefore the Observers page)
|
||||
// returns only rows the retention sweep has not soft-deleted
|
||||
// (inactive IS NULL OR inactive = 0), while the store's stats query counted
|
||||
// every row in the table. On a deployment with observerDays retention the two
|
||||
// drift apart by exactly the number of soft-deleted observers — 79 vs 51 on the
|
||||
// instance this was reproduced against.
|
||||
//
|
||||
// The two stats implementations did not even agree with each other: the DB
|
||||
// fallback (DB.GetStats) already applied the inactive filter, the store path
|
||||
// did not.
|
||||
|
||||
// seedObserversForCount inserts 3 live observers and 2 soft-deleted ones.
|
||||
func seedObserversForCount(t *testing.T, db *DB) {
|
||||
t.Helper()
|
||||
now := time.Now().UTC().Format(time.RFC3339)
|
||||
if _, err := db.conn.Exec(`DELETE FROM observers`); err != nil {
|
||||
t.Fatalf("clear observers: %v", err)
|
||||
}
|
||||
live := []string{"live1", "live2", "live3"}
|
||||
for _, id := range live {
|
||||
if _, err := db.conn.Exec(`INSERT INTO observers (id, name, last_seen, first_seen, packet_count, inactive)
|
||||
VALUES (?, ?, ?, ?, 10, 0)`, id, "Observer "+id, now, now); err != nil {
|
||||
t.Fatalf("insert %s: %v", id, err)
|
||||
}
|
||||
}
|
||||
// One explicitly soft-deleted, one with a NULL flag — GetObservers treats
|
||||
// NULL as live, so only the inactive=1 row must be excluded.
|
||||
if _, err := db.conn.Exec(`INSERT INTO observers (id, name, last_seen, first_seen, packet_count, inactive)
|
||||
VALUES ('gone1', 'Gone One', ?, ?, 5, 1)`, now, now); err != nil {
|
||||
t.Fatalf("insert gone1: %v", err)
|
||||
}
|
||||
if _, err := db.conn.Exec(`INSERT INTO observers (id, name, last_seen, first_seen, packet_count, inactive)
|
||||
VALUES ('nullflag', 'Null Flag', ?, ?, 5, NULL)`, now, now); err != nil {
|
||||
t.Fatalf("insert nullflag: %v", err)
|
||||
}
|
||||
}
|
||||
|
||||
func TestStoreStatsTotalObserversExcludesSoftDeleted(t *testing.T) {
|
||||
db := setupTestDB(t)
|
||||
seedTestData(t, db)
|
||||
seedObserversForCount(t, db)
|
||||
|
||||
store := NewPacketStore(db, nil)
|
||||
if err := store.Load(); err != nil {
|
||||
t.Fatalf("store.Load: %v", err)
|
||||
}
|
||||
|
||||
st, err := store.GetStoreStats()
|
||||
if err != nil {
|
||||
t.Fatalf("GetStoreStats: %v", err)
|
||||
}
|
||||
if st.TotalObservers != 4 {
|
||||
t.Errorf("TotalObservers = %d, want 4 (3 live + 1 NULL flag, excluding 1 soft-deleted)", st.TotalObservers)
|
||||
}
|
||||
}
|
||||
|
||||
// The count must equal what the Observers page actually lists — that is the
|
||||
// whole point of the issue: three UI surfaces showing three numbers.
|
||||
func TestStoreStatsTotalObserversMatchesObserverList(t *testing.T) {
|
||||
db := setupTestDB(t)
|
||||
seedTestData(t, db)
|
||||
seedObserversForCount(t, db)
|
||||
|
||||
store := NewPacketStore(db, nil)
|
||||
if err := store.Load(); err != nil {
|
||||
t.Fatalf("store.Load: %v", err)
|
||||
}
|
||||
|
||||
st, err := store.GetStoreStats()
|
||||
if err != nil {
|
||||
t.Fatalf("GetStoreStats: %v", err)
|
||||
}
|
||||
observers, err := db.GetObservers()
|
||||
if err != nil {
|
||||
t.Fatalf("GetObservers: %v", err)
|
||||
}
|
||||
if st.TotalObservers != len(observers) {
|
||||
t.Errorf("stats totalObservers = %d but /api/observers lists %d",
|
||||
st.TotalObservers, len(observers))
|
||||
}
|
||||
}
|
||||
|
||||
// The store path and the DB fallback must not report different totals for the
|
||||
// same database.
|
||||
func TestStoreAndDBStatsAgreeOnTotalObservers(t *testing.T) {
|
||||
db := setupTestDB(t)
|
||||
seedTestData(t, db)
|
||||
seedObserversForCount(t, db)
|
||||
|
||||
store := NewPacketStore(db, nil)
|
||||
if err := store.Load(); err != nil {
|
||||
t.Fatalf("store.Load: %v", err)
|
||||
}
|
||||
|
||||
storeStats, err := store.GetStoreStats()
|
||||
if err != nil {
|
||||
t.Fatalf("GetStoreStats: %v", err)
|
||||
}
|
||||
dbStats, err := db.GetStats()
|
||||
if err != nil {
|
||||
t.Fatalf("GetStats: %v", err)
|
||||
}
|
||||
if storeStats.TotalObservers != dbStats.TotalObservers {
|
||||
t.Errorf("store path reports %d observers, DB fallback reports %d",
|
||||
storeStats.TotalObservers, dbStats.TotalObservers)
|
||||
}
|
||||
}
|
||||
+1
-1
@@ -2086,7 +2086,7 @@ func (s *PacketStore) GetStoreStats() (*Stats, error) {
|
||||
`SELECT
|
||||
(SELECT COUNT(*) FROM nodes WHERE last_seen > ?) AS active_nodes,
|
||||
(SELECT COUNT(*) FROM nodes) AS all_nodes,
|
||||
(SELECT COUNT(*) FROM observers) AS observers`,
|
||||
(SELECT COUNT(*) FROM observers WHERE inactive IS NULL OR inactive = 0) AS observers`,
|
||||
sevenDaysAgo,
|
||||
).Scan(&st.TotalNodes, &st.TotalNodesAllTime, &st.TotalObservers)
|
||||
}()
|
||||
|
||||
Reference in New Issue
Block a user