feat: separate "Last Status Update" from "Last Packet Observation" for observers (rebased) by Kpa-clawbot · Pull Request #968 · Kpa-clawbot/CoreScope

Kpa-clawbot · 2026-05-02T17:06:14Z

Rebased version of #905 — resolves merge conflicts with current master.

Changes from #905 rebase

Conflict resolutions

cmd/server/db.go — GetObservers query: master added WHERE inactive IS NULL OR inactive = 0 filter; PR feat: separate "Last Status Update" from "Last Packet Observation" for observers #905 added last_packet_at to SELECT. Combined both.
cmd/server/db_test.go — test schema: master added inactive column; PR feat: separate "Last Status Update" from "Last Packet Observation" for observers #905 added last_packet_at. Combined both columns.

Additional fix (not in original #905)

cmd/server/types.go + cmd/server/routes.go — ObserverResp struct was extracted to types.go after PR feat: separate "Last Status Update" from "Last Packet Observation" for observers #905 was created. Added LastPacketAt field to ObserverResp and wired it through both handleObservers and handleObserverDetail handlers.

Verification

All Go tests pass (cmd/server + cmd/ingestor)
Frontend tests pass (test-packets.js, test-hash-color.js)
Built server, hit /api/observers and /api/observers/{id} — last_packet_at field present in JSON response
No behavioral changes — PR feat: separate "Last Status Update" from "Last Packet Observation" for observers #905's intent preserved verbatim

Original PR

See #905 for full description, review comments, and context.

Add a new 'last_packet_at' column to the observers table that is only bumped when an actual packet observation lands (InsertTransmission path), while 'last_seen' continues to be bumped on both status updates and packets. This allows the UI to distinguish between an observer that is alive (sending status pings) vs one that is actively forwarding packets. Schema migration backfills last_packet_at = last_seen for observers with packet_count > 0. Server API now returns last_packet_at in the Observer JSON response.

…vers - observers.js: rename 'Last Seen' column to 'Last Status', add 'Last Packet' column with a warning badge when no packets observed or packets lag behind status by >10min - observer-detail.js: add 'Last Status Update' and 'Last Packet Observation' stat cards with relative + absolute timestamps

- Ingestor: verify last_packet_at is NULL after UpsertObserver (status path), set after InsertTransmission, and unchanged by subsequent UpsertObserver calls - Server: verify last_packet_at reads back through GetObservers and GetObserverByID

The addition of the Last Packet column brings the table to 8 columns. The previous min-width of 640px was tight for 7 columns; 720px prevents cramped rendering and ensures the horizontal scroll trigger is appropriate on narrow viewports.

…istic, test comment 1. Migration ALTER error no longer swallowed: check error from ALTER TABLE and return if it fails (unless column already exists). Migration is not marked complete on failure. 2. Backfill heuristic fixed: use observations table JOIN instead of packet_count > 0, since UpsertObserver sets packet_count = 1 on INSERT even for status-only observers. 3. Test clarifying comment: document that InsertTransmission uses data.Timestamp (not time.Now()) as source-of-truth for last_packet_at, so the hardcoded assertion is correct.

…905)

Kpa-clawbot · 2026-05-02T17:26:15Z

Greybeard Review — PR #968

Rebased version of #905 (separate "Last Status Update" from "Last Packet Observation").

BLOCKER — Missing server-side migration for `last_packet_at`

The ingestor (cmd/ingestor/db.go) has the observers_last_packet_at_v1 migration that adds the column. But the server (cmd/server/) has no equivalent ensureLastPacketAtColumn startup migration.

This is the exact same pattern that PR #961 solved for the inactive column — ensureObserverInactiveColumn in cmd/server/main.go runs at server startup to add the column if it's missing, because the server and ingestor may be deployed independently.

Reproduction (against e2e-fixture.db, which lacks the column):

$ /tmp/cs968v2 -port 13909 -db test-fixtures/e2e-fixture.db ...
$ curl http://localhost:13909/api/observers
{"error":"SQL logic error: no such column: last_packet_at (1)"}

Both GET /api/observers and GET /api/observers/{id} return 500. The entire observers page is broken unless the ingestor has already migrated the DB.

Fix: Add ensureLastPacketAtColumn(dbPath) in cmd/server/main.go alongside the existing ensureObserverInactiveColumn call. Pattern:

if err := ensureLastPacketAtColumn(dbPath); err != nil {
    log.Printf("[store] warning: could not add observers.last_packet_at column: %v", err)
}

Rebase correctness — ✅ GOOD

cmd/server/db.go: master's WHERE inactive IS NULL OR inactive = 0 combined with feat: separate "Last Status Update" from "Last Packet Observation" for observers #905's last_packet_at SELECT — correct.
cmd/server/db_test.go: test schema includes both inactive and last_packet_at — correct.
cmd/server/types.go + routes.go: LastPacketAt added to ObserverResp and wired through both handleObservers and handleObserverDetail — correct adaptation for the post-feat: separate "Last Status Update" from "Last Packet Observation" for observers #905 ObserverResp extraction.

Migration & backfill — ✅ GOOD (ingestor side)

Idempotent, gated by _migrations table
ALTER error handling: aborts on non-"duplicate column" errors
Backfill uses subquery against observations table (not unreliable packet_count) — addresses original review feedback from feat: separate "Last Status Update" from "Last Packet Observation" for observers #905
Status-only observers stay NULL — correct semantics

Test coverage — ✅ GOOD

TestLastPacketAtUpdatedOnPacketOnly: verifies NULL after UpsertObserver, set after InsertTransmission, unchanged by subsequent UpsertObserver. Non-tautological — tests the actual separation of concerns.
TestGetObserverLastPacketAt: verifies read-back through both GetObservers and GetObserverByID. Exercises the column after manual UPDATE.
Existing TestGetObservers and TestGetObserverByID verify NULL default.

Frontend — ✅ GOOD

packetBadge() logic: correct 10-minute gap threshold for warning badge
Observer detail: two stat cards with relative + absolute timestamps
Table min-width bump (640→720px) accommodates new column
All colors use CSS variables

Summary

Severity	Count	Items
BLOCKER	1	Missing `ensureLastPacketAtColumn` in server startup
MAJOR	0	—
MINOR	0	—
NIT	0	—

Verdict: BLOCKER — do not merge until the server-side column migration is added. The fix is mechanical (copy the ensureObserverInactiveColumn pattern), but without it the observers page is a 500 error for any deployment where the server starts before/without the ingestor.

Mirror the ensureObserverInactiveColumn pattern (PR #961) for the last_packet_at column added by the ingestor migration. Without this, the server SELECTs last_packet_at but never adds it — causing 500 errors on /api/observers when running against DBs the ingestor has not yet touched (e.g. the e2e fixture). Adds TestEnsureLastPacketAtColumn for correctness + idempotency.

Kpa-clawbot · 2026-05-02T17:48:46Z

BLOCKER fixed — `ensureLastPacketAtColumn` added

Commit 37a2b71 adds the server-side migration, mirroring the ensureObserverInactiveColumn pattern from PR #961:

cmd/server/neighbor_persist.go: ensureLastPacketAtColumn(dbPath) — opens RW connection, checks PRAGMA table_info(observers) for last_packet_at, adds ALTER TABLE observers ADD COLUMN last_packet_at TEXT if missing, idempotent.
cmd/server/main.go: Called right after ensureObserverInactiveColumn at startup.
cmd/server/neighbor_persist_test.go: TestEnsureLastPacketAtColumn — creates DB without column, runs migration, verifies column exists, runs again for idempotency.

Verification

Before fix (fixture DB lacks column):

$ sqlite3 e2e-fixture.db "SELECT last_packet_at FROM observers LIMIT 1"
Error: no such column: last_packet_at

After fix (server auto-migrates at startup):

[store] Added last_packet_at column to observers
$ curl http://localhost:13909/api/observers
{"observers":[{"id":"...","last_packet_at":null,...},...]}  # 200 OK

Test passes:

=== RUN   TestEnsureLastPacketAtColumn
[store] Added last_packet_at column to observers
--- PASS: TestEnsureLastPacketAtColumn (0.00s)

Kpa-clawbot · 2026-05-02T17:55:31Z

Final greybeard review (re-verify post gap-fix)

PR HEAD: 37a2b71 · Prior HEAD reviewed: 9ba45e6

BLOCKER re-verification: ✅ RESOLVED

The prior review flagged a BLOCKER: server SELECTs last_packet_at but never adds the column — 500 on /api/observers against DBs the ingestor never touched (e.g. the e2e fixture).

Reproduced and verified the fix hands-on:

Built 37a2b71 clean
Copied e2e fixture → freshened timestamps → confirmed last_packet_at column absent
Started server → log: [store] Added last_packet_at column to observers

curl /api/observers → 200, response shape:

{"observers":[{"id":"...","name":"Kennedy Repeater",...,"last_packet_at":null,...}]}

PRAGMA table_info(observers) → 14|last_packet_at|TEXT|0||0 ✓
Restarted server → no re-add log (idempotency confirmed) ✓
go test ./... → all pass (19.9s)

Gap-fix code review (`9ba45e6..37a2b71`)

File	Change	Assessment
`main.go`	Calls `ensureLastPacketAtColumn` after `ensureObserverInactiveColumn`	✅ Correct placement, consistent error handling
`neighbor_persist.go`	New `ensureLastPacketAtColumn` — PRAGMA check → ALTER → log	✅ Mirrors `ensureObserverInactiveColumn` (PR #961) exactly
`neighbor_persist_test.go`	`TestEnsureLastPacketAtColumn` — real DB, add + idempotency	✅ Non-tautological, covers both paths

Other items (carried forward)

ObserverResp wiring — LastPacketAt in both list and detail handlers ✅
Ingestor migration — unchanged by gap-fix ✅
Rebase correctness — no new conflicts ✅

Summary

Severity	Count
BLOCKER	0
MAJOR	0
MINOR	0
NIT	0

Verdict: MERGE READY ✅

Kpa-clawbot · 2026-05-02T18:52:12Z

Superseded by #969 (rebased onto current master after #906 landed).

…r observers (v3 rebase) (#969) Rebased version of #968 (which was itself a rebase of #905) — resolves merge conflict with #906 (clock-skew UI) that landed on master. ## Conflict resolution **`public/observers.js`** — master (#906) added "Clock Offset" column to observer table; #968 split "Last Seen" into "Last Status" + "Last Packet" columns. Combined both: the table now has Status | Name | Region | Last Status | Last Packet | Packets | Packets/Hour | Clock Offset | Uptime. ## What this PR adds (unchanged from #968/#905) - `last_packet_at` column in observers DB table - Separate "Last Status Update" and "Last Packet Observation" display in observers list and detail page - Server-side migration to add the column automatically - Backfill heuristic for existing data - Tests for ingestor and server ## Verification - All Go tests pass (`cmd/server`, `cmd/ingestor`) - Frontend tests pass (`test-packets.js`, `test-hash-color.js`) - Built server, hit `/api/observers` — `last_packet_at` field present in JSON - Observer table header has all 9 columns including both Last Packet and Clock Offset ## Prior PRs - #905 — original (conflicts with master) - #968 — first rebase (conflicts after #906 landed) - This PR — second rebase, resolves #906 conflict Supersedes #968. Closes #905. --------- Co-authored-by: you <you@example.com>

you added 6 commits May 2, 2026 17:01

fix: add last_packet_at to ObserverResp (types.go extracted after PR #…

9ba45e6

…905)

Kpa-clawbot mentioned this pull request May 2, 2026

feat: separate "Last Status Update" from "Last Packet Observation" for observers (v3 rebase) #969

Merged

Kpa-clawbot closed this May 2, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: separate "Last Status Update" from "Last Packet Observation" for observers (rebased)#968

feat: separate "Last Status Update" from "Last Packet Observation" for observers (rebased)#968
Kpa-clawbot wants to merge 7 commits intomasterfrom
feat/observer-last-packet-at-rebased

Kpa-clawbot commented May 2, 2026

Uh oh!

Kpa-clawbot commented May 2, 2026

Uh oh!

Kpa-clawbot commented May 2, 2026

Uh oh!

Kpa-clawbot commented May 2, 2026

Uh oh!

Kpa-clawbot commented May 2, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

Kpa-clawbot commented May 2, 2026

Changes from #905 rebase

Conflict resolutions

Additional fix (not in original #905)

Verification

Original PR

Uh oh!

Kpa-clawbot commented May 2, 2026

Greybeard Review — PR #968

BLOCKER — Missing server-side migration for last_packet_at

Rebase correctness — ✅ GOOD

Migration & backfill — ✅ GOOD (ingestor side)

Test coverage — ✅ GOOD

Frontend — ✅ GOOD

Summary

Uh oh!

Kpa-clawbot commented May 2, 2026

BLOCKER fixed — ensureLastPacketAtColumn added

Verification

Uh oh!

Kpa-clawbot commented May 2, 2026

Final greybeard review (re-verify post gap-fix)

BLOCKER re-verification: ✅ RESOLVED

Gap-fix code review (9ba45e6..37a2b71)

Other items (carried forward)

Summary

Verdict: MERGE READY ✅

Uh oh!

Kpa-clawbot commented May 2, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

BLOCKER — Missing server-side migration for `last_packet_at`

BLOCKER fixed — `ensureLastPacketAtColumn` added

Gap-fix code review (`9ba45e6..37a2b71`)