fix(#1888): count only live observers in the store's /api/stats query (#1892)

Fixes #1888.

## The mismatch

`/api/stats.totalObservers` and `/api/observers` counted different sets:

| Source | Predicate |
|---|---|
| `cmd/server/store.go:2089` (store path) | `SELECT COUNT(*) FROM
observers` — every row |
| `cmd/server/db.go:336` (DB fallback) | `WHERE inactive IS NULL OR
inactive = 0` |
| `db.GetObservers()` → `/api/observers` | `WHERE inactive IS NULL OR
inactive = 0` |

`handleStats` uses the store path whenever a `PacketStore` exists
(`routes.go:785`), which is every normal deployment. So the header count
came from the unfiltered query while the Observers page listed the
filtered set. The two stats implementations also disagreed with each
other for the same database, which is a bug on its own.

## Reproduction

The gap is exactly the observers the `observerDays` retention sweep has
soft-deleted. On the instance I reproduced against:

```
GET /api/stats     → totalObservers: 79
GET /api/observers → observers.length == 51
```

```sql
SELECT 'all',      COUNT(*) FROM observers                                    -- 79
UNION ALL SELECT 'active',   COUNT(*) FROM observers WHERE inactive IS NULL OR inactive = 0  -- 51
UNION ALL SELECT 'inactive', COUNT(*) FROM observers WHERE inactive = 1;      -- 28
```

79 − 28 = 51. Same shape as the 82 vs 51 in the issue.

## The change

One line: the store's stats query gets the same predicate the other two
already use, so all three agree.

## Deliberately out of scope

Two things the issue raises that this does **not** fix, called out so
they are not mistaken for done:

- **Config blacklist.** `buildObserversDefaultResponse` drops
blacklisted observers in the handler loop (`routes.go:2752`), which no
SQL count can see. A deployment with a non-empty `observerBlacklist`
will still show a stats count higher than the list, by the number of
blacklisted-but-live observers. Closing that needs config plumbing into
the count and is a separate change — happy to follow up if wanted.
- **Map controls.** The third surface named in the issue derives its
count from node role aggregates (`roleCounts`), not from the observer
set at all. That is a frontend concern and untouched here.

## Tests

`cmd/server/observer_count_1888_test.go`, three cases, each watched fail
first:

1. `TestStoreStatsTotalObserversExcludesSoftDeleted` — `TotalObservers =
5, want 4`
2. `TestStoreStatsTotalObserversMatchesObserverList` — `stats
totalObservers = 5 but /api/observers lists 4`
3. `TestStoreAndDBStatsAgreeOnTotalObservers` — `store path reports 5
observers, DB fallback reports 4`

The fixture includes a row with `inactive = NULL` alongside `inactive =
0` and `inactive = 1`, since `GetObservers` treats NULL as live and only
the `1` may be excluded.

`cd cmd/server && go test ./...` → ok (87s).

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
efiten
2026-09-02 09:50:26 +02:00
committed by GitHub
co-authored by Claude Opus 5
parent 02c2338d64
commit b3a306b81f
2 changed files with 116 additions and 1 deletions
+115
View File
@@ -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
View File
@@ -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)
}()