[LocalDb] INSERT OR REPLACE on apply deletes a different row via a secondary UNIQUE (permanent divergence, latent) #38

New Issue

2026-11-09T14:39:21-05:00

dohertj2 commented

2026-11-09 14:39:21 -05:00

From the scadaproj#15 shared-library review (code-reviews/LocalDb/findings.md, LocalDb-003), verified at 91fb2b4. Update the finding's Resolution row when fixed.


Severity	Medium
Priority	P2
Category	Correctness & logic bugs
Status	Open
Location	`ZB.MOM.WW.LocalDb/src/ZB.MOM.WW.LocalDb.Replication/Internal/LwwApplier.cs:340`

Description

BuildUpsert emits INSERT OR REPLACE INTO <table> (...). When the incoming row collides with a
local row on a non-PK UNIQUE constraint, REPLACE deletes that other row, which has a different
primary key. The deletion:

does not fire the capture triggers (they are guarded by the applying flag, and recursive_triggers is off anyway);
updates no __localdb_row_version row;
is never shipped to the peer.

LWW only ever compares versions of the key it was handed, so nothing detects the change.
RegisterReplicated (SqliteLocalDb.cs:170) checks PKs, BLOBs and cascading FKs, but not UNIQUE
indexes.

Failure scenario

Latent today: the ScadaBridge site tables registered for replication declare no secondary UNIQUE
constraint (checked 2026-11-09). Reproduced in a scratch test:

Create t(id INTEGER PRIMARY KEY, code TEXT UNIQUE) and register it on both nodes.
While disconnected, insert (1,'X') on A and (2,'X') on B, for example the same tag or
name created on each node during a partition.
Reconnect and exchange deltas.

A applies (2,'X'), and REPLACE drops row 1. B applies (1,'X'), and REPLACE drops row 2. Final
state: A = {2}, B = {1}. Each row is applied (applied=1, dl=0) and A's ledger still says
{"id":1} is live. The pair is permanently diverged. Any later snapshot only logs "ledger/table
divergence" and skips the row (SnapshotStreamer.cs:114).

Recommendation

Reject, or at least warn on, registration of tables that have non-PK UNIQUE indexes or
constraints (pragma_index_list where origin IN ('u') and the index is not the PK). Alternatively,
apply with an explicit INSERT … ON CONFLICT(<pk cols>) DO UPDATE SET …. A unique conflict on
another column then aborts and becomes a visible dead-letter instead of a silent delete. Document
that secondary unique keys must be globally unique across both nodes.

Resolution

Unresolved.

From the scadaproj#15 shared-library review (`code-reviews/LocalDb/findings.md`, **LocalDb-003**), verified at 91fb2b4. Update the finding's Resolution row when fixed. | | | |--|--| | Severity | Medium | | Priority | P2 | | Category | Correctness & logic bugs | | Status | Open | | Location | `ZB.MOM.WW.LocalDb/src/ZB.MOM.WW.LocalDb.Replication/Internal/LwwApplier.cs:340` | **Description** `BuildUpsert` emits `INSERT OR REPLACE INTO <table> (...)`. When the incoming row collides with a local row on a non-PK `UNIQUE` constraint, REPLACE deletes that other row, which has a different primary key. The deletion: - does not fire the capture triggers (they are guarded by the applying flag, and `recursive_triggers` is off anyway); - updates no `__localdb_row_version` row; - is never shipped to the peer. LWW only ever compares versions of the key it was handed, so nothing detects the change. `RegisterReplicated` (`SqliteLocalDb.cs:170`) checks PKs, BLOBs and cascading FKs, but not `UNIQUE` indexes. **Failure scenario** Latent today: the ScadaBridge site tables registered for replication declare no secondary `UNIQUE` constraint (checked 2026-11-09). Reproduced in a scratch test: 1. Create `t(id INTEGER PRIMARY KEY, code TEXT UNIQUE)` and register it on both nodes. 2. While disconnected, insert `(1,'X')` on A and `(2,'X')` on B, for example the same tag or name created on each node during a partition. 3. Reconnect and exchange deltas. A applies `(2,'X')`, and REPLACE drops row 1. B applies `(1,'X')`, and REPLACE drops row 2. Final state: A = `{2}`, B = `{1}`. Each row is applied (`applied=1, dl=0`) and A's ledger still says `{"id":1}` is live. The pair is permanently diverged. Any later snapshot only logs "ledger/table divergence" and skips the row (`SnapshotStreamer.cs:114`). **Recommendation** Reject, or at least warn on, registration of tables that have non-PK `UNIQUE` indexes or constraints (`pragma_index_list` where `origin IN ('u')` and the index is not the PK). Alternatively, apply with an explicit `INSERT … ON CONFLICT(<pk cols>) DO UPDATE SET …`. A unique conflict on another column then aborts and becomes a visible dead-letter instead of a silent delete. Document that secondary unique keys must be globally unique across both nodes. **Resolution** _Unresolved._

dohertj2 added the type:review-finding priority:p2 labels 2026-11-09 14:39:21 -05:00

dohertj2 commented

2026-11-09 15:03:19 -05:00

Fixed on main 15a6c7c (LocalDb-003 → Resolved): rows apply with INSERT … ON CONFLICT(<pk>) DO UPDATE; a clash on another UNIQUE column becomes a visible dead-letter instead of a silent delete (the pair still differs until someone resolves it — documented). Bonus: a remote update of a parent with ON DELETE RESTRICT children now applies (was dead-lettered); also de-flaked WipedPeer_SnapshotLiftsItsWatermark. Only effective once both nodes run 0.3.6. Evidence: LocalDb 236/236; Auth 344 passed / 78 skipped (live SQL/LDAP gated) after rebase; code-reviews --check consistent.

Fixed on main 15a6c7c (LocalDb-003 → Resolved): rows apply with `INSERT … ON CONFLICT(<pk>) DO UPDATE`; a clash on another UNIQUE column becomes a visible dead-letter instead of a silent delete (the pair still differs until someone resolves it — documented). Bonus: a remote update of a parent with ON DELETE RESTRICT children now applies (was dead-lettered); also de-flaked WipedPeer_SnapshotLiftsItsWatermark. Only effective once both nodes run 0.3.6. Evidence: LocalDb 236/236; Auth 344 passed / 78 skipped (live SQL/LDAP gated) after rebase; code-reviews --check consistent.

dohertj2 closed this issue

2026-11-09 15:03:20 -05:00

dohertj2 referenced this issue

2026-11-12 14:08:40 -05:00

[LocalDb] Publish 0.3.7 and adopt in ScadaBridge + OtOpcUa (tombstone fix #16) #53

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: dohertj2/scadaproj#38