port(upstream#1958): hash migration no longer reports success it never achieved

[adminopenclaw8-sketch]

Split out of #25 (commit 758aabeb there). This branch holds exactly one upstream change so it can be reviewed, tested and reverted on its own.

Upstream

• PR: Kpa-clawbot/CoreScope#1958 by @efiten, merged 2026-09-04
• Upstream commit: 56d6d4c722da6bddf3b6b176f2207b309fd3a8e4
• Applied with git cherry-pick -x onto master fda24ca5; upstream authorship kept, and the commit message carries the (cherry picked from commit …) line.
• Patch-id identical to the port(upstream): 26 clean upstream fixes — prune batching, /ws limits, observer liveness, watchdog race #25 commit and to the upstream commit.
• Scope: cmd/server/hash_migrate.go, cmd/server/hash_migrate_test.go.

Problem

migrateContentHashesAsync set hashMigrationComplete in an unconditional defer. On the server's read-only DB handle (Kpa-clawbot#1283) every batch fails at begin/prepare/commit and continues, so the loop always reached the defer and /api/stats answered hashMigrationComplete: true although nothing was migrated.

Change

Counts failed batches (including a recovered panic). If any failed, the flag stays false and one [hash-migrate] INCOMPLETE … line is logged.

Adaptation to this fork

None. The cherry-pick applied without conflicts and the changed lines are identical to upstream.

Notes for review

Visible change on this fork: our server opens SQLite mode=ro, so after merge /api/stats will report hashMigrationComplete: false and the INCOMPLETE line appears at startup. That is the truthful value. In this tree the field is only exposed through /api/stats (routes.go); nothing under public/ reads it.

Dependencies and merge order

• Independent. Branched directly from master fda24ca5 and needs no other PR from this split.
• A merge simulation of all 24 split PRs gives the same tree in upstream order and in reverse order.
• The same simulation also merges cleanly with each of the open API: expose on-wire channelHashHex on channel messages #11, feat(map): support CARTO basemap API keys #13 and chore(deps): bump to golang:1.27.1-alpine3.24 and alpine:3.24 #22, and with test(ingestor): make neighbor metrics pruning deterministic #33 (opened during this split; it makes TestPruneOldNeighborMetrics deterministic). If test(ingestor): make neighbor metrics pruning deterministic #33 lands first, the expected CI failure named below disappears; nothing in this PR depends on it.
• Recommended position: 16 of 24, following upstream's merge order (the order port(upstream): 26 clean upstream fixes — prune batching, /ws limits, observer liveness, watchdog race #25 was assembled in).

Verification

Local run of the same commands as CI's “Go Build & Test” job (server tests with -race), on this branch and on master fda24ca5 under the same conditions (same machine, run one after another):

Check	master fda24ca5	this branch	verdict
go-server-build-vet	PASS	PASS
go-server-test-race	PASS	PASS
channel-lib-test	PASS	PASS
decrypt-cli-build-test	PASS	PASS
dockerfile-copy-invariants	FAIL	FAIL	not runnable locally: script needs bash ≥4 (declare -A), macOS has 3.2; identical on master
staging-disk-monitor	PASS	PASS
css-vars-lint	PASS	PASS

Baseline failures (fail identically on master; not introduced or changed here): see rows marked baseline failure, unchanged.

Browser validation (local, fixture DB, no staging/production): Not applicable (no frontend change).

Not run:

• Playwright E2E suites (no local Playwright install); CI's E2E job will also be skipped, see below.
• eslint (not installed locally; CI installs it on the fly).
• Frontend JS suites (no frontend change).
• Browser tests against staging/production (deliberately none).

Expected GitHub CI: “Go Build & Test” is expected to fail on TestPruneOldNeighborMetrics, which already fails on master (see #25's run). Downstream jobs (Playwright, image build) are therefore skipped. “Deploy Staging” and all GHCR publish steps only run on push to master and cannot run for this PR.
Two further ingestor tests have failed intermittently in this split's CI on branches whose cmd/ingestor tree is byte-identical to master (#27, #28), so they can also appear here without being caused by this change:

• TestBackfillTxLastSeen_ResolvesFromMaxObservationTimestamp: also reproduced locally on unmodified master.
• TestMQTTStallWatchdog_DisconnectedEscalationThrottled_1749: the suite flake that upstream test(ingestor): join the watchdog loop goroutine instead of only asking it to stop Kpa-clawbot/CoreScope#2003 (also split out of port(upstream): 26 clean upstream fixes — prune batching, /ws limits, observer liveness, watchdog race #25) addresses.

GitHub CI result: run 34751729111 on c272f6e3. Go Build & Test: failure; all downstream jobs incl. Deploy Staging skipped. Failed tests:

• TestPruneOldNeighborMetrics: fails on master, documented baseline

🤖 Generated with Claude Code

[dborup]

Parked: this does not fix the case it targets

Independent review found, and I then reproduced independently, that the premise in the code comment is not what SQLite does. The comment says that on the read-only handle the server has held since Kpa-clawbot#1283 "begin, prepare and commit all fail, every batch is skipped". Probing a real mode=ro handle with the driver this repo uses:

Begin()   err = <nil>                                    => SUCCEEDS
Prepare() err = <nil>                                    => SUCCEEDS
Exec()    err = attempt to write a readonly database (8) => FAILS
Commit()  err = <nil>                                    => SUCCEEDS
on-disk value afterwards: unchanged

That is expected: the DSN sets no _txlock, so BEGIN is deferred and takes no write lock, sqlite3_prepare compiles without writing, and a transaction that wrote nothing commits cleanly. Only stmt.Exec fails — and that is the one error path this PR does not count.

Consequence: all three new failedBatches++ sites are unreachable in the deployment the PR names, so /api/stats still answers hashMigrationComplete: true after migrating nothing. Run end to end against a read-only handle, the patched function still logs "Migrated 1 content hashes" and reports complete, with the old hash still on disk.

The test does not catch this because it closes the handle rather than opening it read-only. Close() makes Begin() return sql: database is closed — a database/sql-level error raised before the driver is consulted — so it exercises a different path from SQLITE_READONLY. The test comment asserts the two are "the identical failure path"; they diverge at the first call. Mutation testing agrees: removing the prepare or commit counter leaves the test green, and only the begin counter is covered.

Two further things surfaced, both pre-existing but in the same function and relevant to any real fix:

• A failed stmt.Exec is treated as a UNIQUE-collision. On a read-only handle the fallback SELECT finds no survivor, so the in-memory index is moved to the new hash while disk keeps the old one, and the migrated counter reports rows attempted, not rows written. So there are two more "success" over-reports in the same place.
• readonly_invariant_test.go enforces "zero writer call sites in cmd/server", but its DML patterns only match the nodes table, so UPDATE transmissions / DELETE FROM transmissions here were never flagged. That is why this write path survived bug(db): vacuumOnStartup fails with SQLITE_BUSY when ingestor + server share DB (auto_vacuum migration #919 broken in single-container topology) Kpa-clawbot/CoreScope#1283/refactor(db): finish #1283 — Option 4: ingestor owns neighbor-graph + schema migrations; server is read-only (fixes #1287) Kpa-clawbot/CoreScope#1289.

For what it is worth: hashMigrationComplete has no consumer beyond the /api/stats field — the contract comment claiming it "gates content-hash-dependent code paths (dedup correctness on the write side)" describes a gate that does not exist, and cannot, since the server has no write side. So flipping the flag carries no behaviour risk; the value is purely in reporting honestly.

Why parked rather than patched

The minimal correction (count stmt.Exec) makes every production start log an error-shaped INCOMPLETE line forever, for a condition that is expected and cannot be fixed in this process. That is a design decision, not a mechanical fix, and the alternatives differ in kind:

1. Detect the read-only handle up front (it is known at open time) and skip the migration with an INFO line.
2. Count stmt.Exec and accept a permanent per-boot warning.
3. Move the migration to the ingestor, which this PR's own log message names as where it belongs — that would resolve the misclassification and the invariant-test gap at the same time.

The branch is synced with current master and otherwise ready; it needs a decision on which of those to take.

[dborup]

Closing as superseded: the content-hash migration now runs as a one-time ingestor migration (#222), so this port of upstream#1958 no longer applies. The branch is also ~690 commits behind and conflicts.