Files
meshcore-analyzer/cmd/server
efitenandClaude Opus 5 56d6d4c722 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-<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>
2026-09-04 15:15:16 +02:00
..