fix: address PR #905 review — migration error handling, backfill heuristic, 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.
This commit is contained in:
you
2026-04-24 15:38:05 +00:00
parent e9f977cd70
commit 5bec14222a
2 changed files with 12 additions and 3 deletions

View File

@@ -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)

View File

@@ -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)
}