_migrate_to_v3: deterministic dedupe, explicit column list, and LMC foreign-key sweep #128

New Issue

jabber.developer2 · 2026-05-27T11:13:58Z

jabber.developer2 commented

2026-05-27 11:13:58 +00:00

`_migrate_to_v3`: deterministic dedupe, explicit column list, and LMC foreign-key sweep

Labels: database · correctness
Severity: medium (silent data loss / dangling LMC links on v2 → v3 upgrade)

Split out of #112 (items #7 and #11). Both concern a single statement in
_migrate_to_v3 (src/database_sqlite.c) and are fixed by one change, so they
are tracked together here.

Problem

The v2 → v3 migration copies and deduplicates rows with:

INSERT INTO ChatLogs_v3_migration
SELECT * FROM ChatLogs WHERE archive_id IS NULL
UNION ALL
SELECT * FROM (SELECT * FROM ChatLogs WHERE archive_id IS NOT NULL GROUP BY archive_id)

Three defects:

Non-deterministic dedupe (was #7). SQLite's bare-column-with-GROUP BY extension picks an implementation-defined row per archive_id group. In practice it keeps the first row scanned, not the latest — verified: for a duplicate group {id=2 (old), id=3 (MAM rebroadcast)} the migration keeps id=2 and drops id=3. The choice is arbitrary and can change across SQLite versions.
Dangling LMC foreign keys (was #7). When a dropped duplicate is referenced by an XEP-0308 correction chain, the surviving rows' replaces_db_id / replaced_by_db_id are left pointing at ids that no longer exist. The migration produces exactly the "Broken LMC reference" condition that _sqlite_verify_integrity already flags as an integrity error. The migration runs once per upgrade and is irreversible without a backup.
Positional SELECT * (was #11). The copy relies on the v2 and v3 column lists being identical in order. Any out-of-band schema change between v2 and v3 (e.g. a side branch adding a column to ChatLogs) would silently mis-bind columns at migration time.

Real-world incidence of (1)/(2) is low — MAM rebroadcast of the same archive_id is rare on well-behaved servers — but the blast radius is the user's whole chat history, applied silently on first launch after upgrade.

Acceptance criteria

Deduplicate deterministically: keep MAX(id) per archive_id group ("latest insert wins") instead of relying on GROUP BY's arbitrary pick.
After dedupe, sweep replaces_db_id / replaced_by_db_id and NULL out references to rows that no longer exist, so the migrated table carries no dangling local FKs (and verify_integrity reports clean).
Replace SELECT * with an explicit column list matching the ChatLogs_v3_migration schema, in every branch of the copy.
Add a comment near the live ChatLogs schema noting that column changes must be mirrored in the migration's explicit column list.
Cover with a migration test that seeds a v2 schema with deliberate archive_id duplicates plus an LMC chain spanning the duplicates, runs the migration, and asserts: deterministic survivor, no dangling FKs, valid FKs preserved.

Out of scope

Changing the dedupe strategy for live inserts (already handled by the explicit duplicate_check_query and the new archive_id UNIQUE).
Remapping dangling correction links to the surviving duplicate (would better preserve the LMC chain across a rebroadcast duplicate, but adds an old-id → survivor-id mapping pass; NULL-out is sufficient to remove the dangling FK). Track separately if desired.

Status / notes

A fix and a local-only regression test are already prepared (branch fix/issue-112-followups, to be submitted as its own PR):

src/database_sqlite.c _migrate_to_v3: explicit column list, id IN (SELECT MAX(id) ... GROUP BY archive_id), and two UPDATE ... SET <fk> = NULL WHERE <fk> NOT IN (SELECT id ...) sweep statements before the DROP/RENAME.
tests/migration/test_migrate_v3_dedup.sh — sqlite3-CLI test, deliberately not wired into Makefile.am/CI (one-shot migration). Passes on the fix; fails 4 assertions on the original SQL (wrong survivor + dangling FKs), confirming it guards the regression.

# `_migrate_to_v3`: deterministic dedupe, explicit column list, and LMC foreign-key sweep **Labels:** database · correctness **Severity:** medium (silent data loss / dangling LMC links on v2 → v3 upgrade) Split out of #112 (items #7 and #11). Both concern a single statement in `_migrate_to_v3` (`src/database_sqlite.c`) and are fixed by one change, so they are tracked together here. ## Problem The v2 → v3 migration copies and deduplicates rows with: ```sql INSERT INTO ChatLogs_v3_migration SELECT * FROM ChatLogs WHERE archive_id IS NULL UNION ALL SELECT * FROM (SELECT * FROM ChatLogs WHERE archive_id IS NOT NULL GROUP BY archive_id) ``` Three defects: 1. **Non-deterministic dedupe (was #7).** SQLite's bare-column-with-`GROUP BY` extension picks an *implementation-defined* row per `archive_id` group. In practice it keeps the *first* row scanned, not the latest — verified: for a duplicate group `{id=2 (old), id=3 (MAM rebroadcast)}` the migration keeps `id=2` and drops `id=3`. The choice is arbitrary and can change across SQLite versions. 2. **Dangling LMC foreign keys (was #7).** When a dropped duplicate is referenced by an XEP-0308 correction chain, the surviving rows' `replaces_db_id` / `replaced_by_db_id` are left pointing at ids that no longer exist. The migration produces exactly the "Broken LMC reference" condition that `_sqlite_verify_integrity` already flags as an integrity *error*. The migration runs once per upgrade and is irreversible without a backup. 3. **Positional `SELECT *` (was #11).** The copy relies on the v2 and v3 column lists being identical in order. Any out-of-band schema change between v2 and v3 (e.g. a side branch adding a column to `ChatLogs`) would silently mis-bind columns at migration time. Real-world incidence of (1)/(2) is low — MAM rebroadcast of the same `archive_id` is rare on well-behaved servers — but the blast radius is the user's whole chat history, applied silently on first launch after upgrade. ## Acceptance criteria - [ ] Deduplicate deterministically: keep `MAX(id)` per `archive_id` group ("latest insert wins") instead of relying on `GROUP BY`'s arbitrary pick. - [ ] After dedupe, sweep `replaces_db_id` / `replaced_by_db_id` and NULL out references to rows that no longer exist, so the migrated table carries no dangling local FKs (and `verify_integrity` reports clean). - [ ] Replace `SELECT *` with an explicit column list matching the `ChatLogs_v3_migration` schema, in every branch of the copy. - [ ] Add a comment near the live `ChatLogs` schema noting that column changes must be mirrored in the migration's explicit column list. - [ ] Cover with a migration test that seeds a v2 schema with deliberate `archive_id` duplicates plus an LMC chain spanning the duplicates, runs the migration, and asserts: deterministic survivor, no dangling FKs, valid FKs preserved. ## Out of scope - Changing the dedupe strategy for live inserts (already handled by the explicit `duplicate_check_query` and the new `archive_id UNIQUE`). - Remapping dangling correction links to the surviving duplicate (would better preserve the LMC chain across a rebroadcast duplicate, but adds an old-id → survivor-id mapping pass; NULL-out is sufficient to remove the dangling FK). Track separately if desired. ## Status / notes A fix and a local-only regression test are already prepared (branch `fix/issue-112-followups`, to be submitted as its own PR): - `src/database_sqlite.c` `_migrate_to_v3`: explicit column list, `id IN (SELECT MAX(id) ... GROUP BY archive_id)`, and two `UPDATE ... SET <fk> = NULL WHERE <fk> NOT IN (SELECT id ...)` sweep statements before the `DROP`/`RENAME`. - `tests/migration/test_migrate_v3_dedup.sh` — `sqlite3`-CLI test, deliberately **not** wired into Makefile.am/CI (one-shot migration). Passes on the fix; fails 4 assertions on the original SQL (wrong survivor + dangling FKs), confirming it guards the regression.

jabber.developer2 referenced this issue

2026-05-27 11:15:55 +00:00

Follow-up issues for PR #105 #112

jabber.developer2 referenced this issue from a commit

2026-05-27 11:22:24 +00:00

fix(database): make v2->v3 chatlog migration deterministic and FK-safe

jabber.developer2 referenced a pull request that will close this issue

2026-05-27 11:23:27 +00:00

WIP: fix(database): make v2->v3 chatlog migration deterministic and FK-safe #129

jabber.developer2 referenced this issue

2026-05-27 11:30:53 +00:00

Fix: issue #112 follow-ups — OTR strip, presence UAF, OMEMO load #130