From 5bec14222a84d928fa887917232ebf4db348260e Mon Sep 17 00:00:00 2001 From: you Date: Fri, 24 Apr 2026 15:38:05 +0000 Subject: [PATCH] =?UTF-8?q?fix:=20address=20PR=20#905=20review=20=E2=80=94?= =?UTF-8?q?=20migration=20error=20handling,=20backfill=20heuristic,=20test?= =?UTF-8?q?=20comment?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- cmd/ingestor/db.go | 13 ++++++++++--- cmd/ingestor/db_test.go | 2 ++ 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/cmd/ingestor/db.go b/cmd/ingestor/db.go index c416e19..0fe99d7 100644 --- a/cmd/ingestor/db.go +++ b/cmd/ingestor/db.go @@ -423,9 +423,16 @@ func applySchema(db *sql.DB) error { row = db.QueryRow("SELECT 1 FROM _migrations WHERE name = 'observers_last_packet_at_v1'") if row.Scan(&migDone) != nil { log.Println("[migration] Adding last_packet_at column to observers...") - db.Exec(`ALTER TABLE observers ADD COLUMN last_packet_at TEXT DEFAULT NULL`) - // Backfill: set last_packet_at = last_seen only for observers that have received packets - res, err := db.Exec(`UPDATE observers SET last_packet_at = last_seen WHERE packet_count > 0`) + _, alterErr := db.Exec(`ALTER TABLE observers ADD COLUMN last_packet_at TEXT DEFAULT NULL`) + if alterErr != nil && !strings.Contains(alterErr.Error(), "duplicate column") { + return fmt.Errorf("observers last_packet_at ALTER: %w", alterErr) + } + // Backfill: set last_packet_at = last_seen only for observers that actually have + // observation rows (packet_count alone is unreliable — UpsertObserver sets it to 1 + // on INSERT even for status-only observers). + res, err := db.Exec(`UPDATE observers SET last_packet_at = last_seen + WHERE last_packet_at IS NULL + AND rowid IN (SELECT DISTINCT observer_idx FROM observations WHERE observer_idx IS NOT NULL)`) if err == nil { n, _ := res.RowsAffected() log.Printf("[migration] Backfilled last_packet_at for %d observers with packets", n) diff --git a/cmd/ingestor/db_test.go b/cmd/ingestor/db_test.go index 12c2da6..957b7bd 100644 --- a/cmd/ingestor/db_test.go +++ b/cmd/ingestor/db_test.go @@ -606,6 +606,8 @@ func TestLastPacketAtUpdatedOnPacketOnly(t *testing.T) { if !lastPacketAt.Valid { t.Fatal("expected last_packet_at to be non-NULL after InsertTransmission") } + // InsertTransmission uses `now = data.Timestamp || time.Now()`, so last_packet_at + // should match the packet's Timestamp when provided (same source-of-truth as last_seen). if lastPacketAt.String != "2026-04-24T12:00:00Z" { t.Errorf("expected last_packet_at=2026-04-24T12:00:00Z, got %s", lastPacketAt.String) }