From c272f6e3dc069d21e30295054c29418195a9c14c Mon Sep 17 00:00:00 2001 From: efiten Date: Fri, 4 Sep 2026 15:15:16 +0200 Subject: [PATCH] fix(#1856): stop the hash migration reporting success it never achieved (#1958) Part 2 of #1856. **Part 1 is deliberately not fixed here** and the issue stays open for it; reasoning at the end. ## The bug `migrateContentHashesAsync` set `store.hashMigrationComplete` in a deferred func that ran unconditionally. Every DB failure inside the loop takes a `continue` (begin tx, prepare, commit), so the loop always reaches that defer, **including when not a single batch was written**. That is not hypothetical. The server has held a `mode=ro` handle since #1283, so `Begin`, `Prepare` and `Commit` all fail, every batch is skipped, and `/api/stats` then answers `hashMigrationComplete: true` after migrating nothing. The migration is started unconditionally on every boot at `main.go:546`. ## The fix The three failure paths now count, and the defer only claims completion when the count is zero. When it is not, it logs once, naming the read-only handle as the expected cause and pointing at this issue, so an operator can tell "no work to do" apart from "could not do the work". **Nothing waits on the flag.** The only reader is `routes.go:828`, which reports it in `/api/stats`. Leaving it false on failure blocks nothing; it just stops the endpoint from lying. The in-memory index is untouched on failure. That was already true, because the index update runs only after a successful commit, and the test now asserts it so memory and disk cannot drift apart. ## Verification The regression test **fails on unmodified master**: ``` hash_migrate_test.go:115: hashMigrationComplete must stay false when no batch could be written; reporting true here is what #1856 called self-reported success ``` It closes the DB handle to make writes fail. That is deterministic and exercises the identical path as a read-only handle (`Begin` errors, batch skipped); the in-memory test DB cannot be reopened read-only. The existing happy-path test still passes, so the flag still turns true on a real migration. `gofmt` clean, `go vet` clean, `cmd/server` suite ok in 59.7s. ## Why part 1 is not in here `handlePostPacket` writes to the same read-only handle and therefore always answers 500. I checked the error path before assuming it was misleading: it already returns `"transmission insert: attempt to write a readonly database"`, so the message is accurate. The endpoint is not confusing, it is simply dead. The issue asks maintainers directly: *"is this endpoint still wanted? If ingestion is MQTT-only now, deleting it is simpler than routing it through a handoff."* That is a product decision, not a fix, and inventing a middle answer would only add code without settling it. Worth noting the repository already has a precedent for the handoff shape: the server writes `request-.json` and the ingestor consumes it (`cmd/ingestor/prune_geofilter.go`). Two things a decision should account for: the endpoint is documented in `openapi.go:69` and guarded by `requireAPIKey`, and `routes_test.go:4850` asserts it writes an observation row using the v3 schema, which passes only because the test DB is read-write. Co-authored-by: Claude Opus 5 (1M context) (cherry picked from commit 56d6d4c722da6bddf3b6b176f2207b309fd3a8e4) Co-Authored-By: Claude Opus 5 --- cmd/server/hash_migrate.go | 17 +++++++++++++ cmd/server/hash_migrate_test.go | 44 +++++++++++++++++++++++++++++++++ 2 files changed, 61 insertions(+) diff --git a/cmd/server/hash_migrate.go b/cmd/server/hash_migrate.go index cd5929221..a85d58bc1 100644 --- a/cmd/server/hash_migrate.go +++ b/cmd/server/hash_migrate.go @@ -10,9 +10,23 @@ import ( // both the DB and the in-memory byHash index. The migration is idempotent: // once all hashes match the current formula it completes instantly. func migrateContentHashesAsync(store *PacketStore, batchSize int, yieldDuration time.Duration) { + // #1856: every DB failure below continues to the next batch, so the loop + // always reaches the deferred completion. Setting the flag unconditionally + // therefore reported success after migrating nothing, which is exactly what + // happens on the read-only handle the server holds since #1283: begin, + // prepare and commit all fail, every batch is skipped, and /api/stats then + // answers hashMigrationComplete: true. + failedBatches := 0 defer func() { if r := recover(); r != nil { log.Printf("[hash-migrate] panic recovered: %v", r) + failedBatches++ + } + if failedBatches > 0 { + log.Printf("[hash-migrate] INCOMPLETE: %d batch(es) could not be written; "+ + "hashMigrationComplete stays false. On a read-only DB handle this is expected "+ + "and the migration belongs in the ingestor (#1856).", failedBatches) + return } store.hashMigrationComplete.Store(true) }() @@ -57,12 +71,14 @@ func migrateContentHashesAsync(store *PacketStore, batchSize int, yieldDuration dbTx, err := store.db.conn.Begin() if err != nil { log.Printf("[hash-migrate] begin tx: %v", err) + failedBatches++ continue } stmt, err := dbTx.Prepare("UPDATE transmissions SET hash = ? WHERE id = ?") if err != nil { log.Printf("[hash-migrate] prepare: %v", err) dbTx.Rollback() + failedBatches++ continue } @@ -83,6 +99,7 @@ func migrateContentHashesAsync(store *PacketStore, batchSize int, yieldDuration if err := dbTx.Commit(); err != nil { log.Printf("[hash-migrate] commit: %v", err) + failedBatches++ continue } diff --git a/cmd/server/hash_migrate_test.go b/cmd/server/hash_migrate_test.go index f1c816de9..0cc7f21e6 100644 --- a/cmd/server/hash_migrate_test.go +++ b/cmd/server/hash_migrate_test.go @@ -76,3 +76,47 @@ func TestMigrateContentHashesAsync_NoOp(t *testing.T) { t.Error("hash should remain in index") } } + +// #1856: a migration that could not write anything must not report completion. +// +// In production the server holds a mode=ro handle (#1283), so Begin, Prepare and +// Commit all fail, every batch takes a `continue`, and the loop reaches the +// deferred completion having migrated nothing. Before this fix the flag was set +// unconditionally there, so /api/stats answered hashMigrationComplete: true +// after doing no work at all. +// +// Closing the handle stands in for the read-only one: it is deterministic and it +// exercises the identical failure path (Begin returns an error, batch skipped). +func TestMigrateContentHashesAsyncDoesNotClaimCompletionWhenWritesFail(t *testing.T) { + db := setupTestDBv2(t) + store := NewPacketStore(db, nil) + + rawHex := "0A00D69FD7A5A7475DB07337749AE61FA53A4788E976" + wrongHash := "deadbeef12345678" + if _, err := db.conn.Exec(`INSERT INTO transmissions (raw_hex, hash, first_seen, route_type, payload_type) + VALUES (?, ?, datetime('now'), 0, 2)`, rawHex, wrongHash); err != nil { + t.Fatal(err) + } + if err := store.Load(); err != nil { + t.Fatal(err) + } + if store.byHash[wrongHash] == nil { + t.Fatal("expected packet under the wrong hash before migration") + } + + // Make every write fail, the way a read-only handle does in production. + if err := db.conn.Close(); err != nil { + t.Fatal(err) + } + + migrateContentHashesAsync(store, 100, time.Millisecond) + + if store.hashMigrationComplete.Load() { + t.Error("hashMigrationComplete must stay false when no batch could be written; " + + "reporting true here is what #1856 called self-reported success") + } + if store.byHash[wrongHash] == nil { + t.Error("the in-memory index must be left alone when the DB write failed, " + + "otherwise memory and disk disagree") + } +}