mirror of
https://github.com/Kpa-clawbot/meshcore-analyzer.git
synced 2026-09-16 10:42:41 +00:00
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-<id>.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) <noreply@anthropic.com>
123 lines
3.7 KiB
Go
123 lines
3.7 KiB
Go
package main
|
|
|
|
import (
|
|
"testing"
|
|
"time"
|
|
)
|
|
|
|
func TestMigrateContentHashesAsync(t *testing.T) {
|
|
db := setupTestDBv2(t)
|
|
store := NewPacketStore(db, nil)
|
|
|
|
// Insert a packet with a manually wrong hash (simulating old formula).
|
|
rawHex := "0A00D69FD7A5A7475DB07337749AE61FA53A4788E976"
|
|
correctHash := ComputeContentHash(rawHex)
|
|
wrongHash := "deadbeef12345678"
|
|
|
|
_, err := db.conn.Exec(`INSERT INTO transmissions (raw_hex, hash, first_seen, route_type, payload_type)
|
|
VALUES (?, ?, datetime('now'), 0, 2)`, rawHex, wrongHash)
|
|
if err != nil {
|
|
t.Fatal(err)
|
|
}
|
|
|
|
if err := store.Load(); err != nil {
|
|
t.Fatal(err)
|
|
}
|
|
|
|
if store.byHash[wrongHash] == nil {
|
|
t.Fatal("expected packet under wrong hash before migration")
|
|
}
|
|
|
|
migrateContentHashesAsync(store, 100, time.Millisecond)
|
|
|
|
if !store.hashMigrationComplete.Load() {
|
|
t.Error("expected hashMigrationComplete to be true")
|
|
}
|
|
if store.byHash[wrongHash] != nil {
|
|
t.Error("old hash should be removed from index")
|
|
}
|
|
if store.byHash[correctHash] == nil {
|
|
t.Error("new hash should be in index")
|
|
}
|
|
|
|
var dbHash string
|
|
err = db.conn.QueryRow("SELECT hash FROM transmissions WHERE raw_hex = ?", rawHex).Scan(&dbHash)
|
|
if err != nil {
|
|
t.Fatal(err)
|
|
}
|
|
if dbHash != correctHash {
|
|
t.Errorf("DB hash = %s, want %s", dbHash, correctHash)
|
|
}
|
|
}
|
|
|
|
func TestMigrateContentHashesAsync_NoOp(t *testing.T) {
|
|
db := setupTestDBv2(t)
|
|
store := NewPacketStore(db, nil)
|
|
|
|
rawHex := "0A00D69FD7A5A7475DB07337749AE61FA53A4788E976"
|
|
correctHash := ComputeContentHash(rawHex)
|
|
|
|
_, err := db.conn.Exec(`INSERT INTO transmissions (raw_hex, hash, first_seen, route_type, payload_type)
|
|
VALUES (?, ?, datetime('now'), 0, 2)`, rawHex, correctHash)
|
|
if err != nil {
|
|
t.Fatal(err)
|
|
}
|
|
|
|
if err := store.Load(); err != nil {
|
|
t.Fatal(err)
|
|
}
|
|
|
|
migrateContentHashesAsync(store, 100, time.Millisecond)
|
|
|
|
if !store.hashMigrationComplete.Load() {
|
|
t.Error("expected hashMigrationComplete to be true")
|
|
}
|
|
if store.byHash[correctHash] == nil {
|
|
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")
|
|
}
|
|
}
|