mirror of
https://github.com/Kpa-clawbot/meshcore-analyzer.git
synced 2026-09-25 23:13:49 +00:00
Sylvain Rabot
ac7e8d38a2
Review feedback on #1992, all three points on `internal/dbschema/dedup_index.go`. **The repair decision no longer hinges on prose.** `ensureObservationsDedupIndex` matched `strings.Contains(err.Error(), "UNIQUE constraint failed")` to decide whether to repair — a text match on driver output, introduced by the same change that swapped the driver. A reworded or wrapped message would skip the repair silently and resurface as an `OpenStore` failure with nothing pointing at the cause. It now checks `sqlite3.Error.Code == sqlite3.ErrConstraint` via `errors.As`. Confirmed against the driver: mattn returns `Code=19` (`ErrConstraint`), `ExtendedCode=2067` (`ErrConstraintUnique`) for `CREATE UNIQUE INDEX` over duplicates, and `errors.As` reaches it. `TestEnsureObservationsDedupIndexTakesRepairPathOnRealDriverError` covers the branch that *decides* to repair, which nothing did: the existing tests call `collapseDuplicatesAndIndex` directly, so a broken error check would have left all of them green. It asserts `isConstraintViolation` recognises the driver's own error and that the repair actually runs. **The TEMP table lifecycle now matches its comment.** The deferred cleanup ran `rw.Exec("DROP TABLE IF EXISTS temp.dedup_groups")`, but a TEMP table belongs to one connection and `database/sql` hands out pooled ones, so that drop could land on a different connection and leave the table on the one holding it. Harmless in `cmd/ingestor` at `SetMaxOpenConns(1)`, not guaranteed for `cmd/migrate`, which calls the same function with an unbounded pool. Every reference is now through `tx`: created, dropped before COMMIT, and removed by ROLLBACK on the error paths. Verified rather than assumed — a TEMP table created inside a transaction does not survive its rollback, so the deferred drop was both unnecessary and aimed at the wrong connection. The leading `DROP ... IF EXISTS` stays, since this package cannot prove nothing else left one behind. **The deletion is auditable now.** `collapsed N duplicate observation row(s)` was the whole record, and if the merge direction were ever wrong again that line is all anyone would have to work from. The group keys are sitting in `dedup_groups` at that moment, so they get logged — group count, rows to remove, then the first 20 groups with their key and the id being kept — before anything is destroyed. It also warns up front that the write lock is held, because on a large table the pause at ingestor startup is otherwise unexplained: [dbschema] idx_observations_dedup cannot be created: duplicate observations exist. Repairing now — this holds the write lock until it completes, and on a large observations table it can take tens of seconds. [dbschema] 1 duplicate observation group(s), 1 row(s) to remove [dbschema] transmission_id=4 observer_idx=4 path_json="[]" rows=2 keeping id=1 [dbschema] collapsed 1 duplicate observation row(s); idx_observations_dedup created **Docs carry the production numbers.** The PR's headline came from a standalone harness; @efiten ran both drivers against an 11M-observation, 9.7GB instance on 4-core arm64, with the order reversed in a second round so the page cache favoured the old driver. Warm, the 7d audit is ~1.8x and chunk load ~1.4x, and `/api/nodes?limit=500` is unchanged. So the real gain on the paths that matter is 1.4-1.8x, not the 2-2.3x the harness showed, and the doc now says to quote those instead. It also records the counterweight nobody had quantified: a cold native build goes from 52s to 163s, which an instance building its own image pays per deploy. **Also removes four files the previous commit should not have added.** `cmd/server/prune-requests/*.json` is runtime queue state written by `internal/prunequeue` and left behind by the server tests; a `git add -A` swept it in. They were never in master, so the branch's net diff was unaffected, but they had no business being committed. `prune-requests/` is now in `.gitignore` so it cannot happen again. Constraint: the repair must stay all-or-nothing — merge, delete and CREATE UNIQUE INDEX share one transaction Rejected: keeping the string match with a test pinning the message | the typed code is available and the text is the driver's to change Rejected: an explicit sql.Conn for the TEMP table | tx already pins one connection, and rollback already cleans up Rejected: logging every duplicate group | unbounded output at startup is its own operational problem, so it caps at 20 with a count of the rest Directive: nothing in this file may decide control flow from an error message; use the typed code Directive: every TEMP table reference goes through tx, never rw — the pool will hand you a different connection Confidence: high Scope-risk: narrow Not-tested: the collapse at 11M rows, or against a database an ingestor is actively writing to — @efiten has offered a staging instance taking live MQTT traffic, which is the remaining gap before this leaves draft