scadaproj/code-reviews/LocalDb/findings.md

# Code Review — LocalDb

| Field | Value |
|-------|-------|
| Library | `ZB.MOM.WW.LocalDb/` |
| Packages | `ZB.MOM.WW.LocalDb`, `ZB.MOM.WW.LocalDb.Replication`, `ZB.MOM.WW.LocalDb.Contracts` |
| Component spec | `docs/plans/2026-07-17-localdb-design.md` (design doc; there is no `components/localdb/` directory, so no SPEC.md exists) |
| Shared contract | None (no `components/localdb/shared-contract/`); the wire contract is `src/ZB.MOM.WW.LocalDb.Contracts/Protos/localdb_sync.proto` |
| Status | Reviewed |
| Last reviewed | 2026-11-09 |
| Reviewer | Claude |
| Commit reviewed | `91fb2b4` |
| Open findings | 5 |

## Summary

The sync engine is carefully built, and its watermark and session machinery holds up under a close
read. The parts checked and found sound:

- Seq assignment matches commit order, because SQLite has a single writer and the oplog is AUTOINCREMENT.
- Watermarks are MAX-guarded, the applier runs one `BEGIN IMMEDIATE` transaction per batch with a
  savepoint per entry, and the applying guard is a row in the same transaction.
- Prune gaps fault the live session, and a rebuilt peer is detected (both directions).
- Snapshot debt is cleared only on the peer's ack, the inbox is bounded and the writer has two lanes.
- Takeover is newest-wins, and the DI extensions are idempotent.

The DI wiring is covered by tests that build the container: `DependencyInjectionTests`,
`GrpcAdapterTests.AddZbLocalDbReplication_*` and `MaintenanceTests.AddZbLocalDbReplication_RegistersMaintenance_BothRoles`
build a provider and resolve the hosted services. No TryAdd-ordering or factory-cycle defect was found.

The findings fall into three themes:

1. **Shapes the apply path cannot converge on.** The applier writes with `INSERT OR REPLACE` and
   treats every `SqliteException` as per-entry poison. That holds for the narrow schemas the two
   consumers use today, but registration accepts several shapes that diverge or lose rows silently:
   - FK-linked tables during a snapshot (LocalDb-002);
   - secondary `UNIQUE` constraints (LocalDb-003);
   - rowid-alias keys assigned on both nodes (LocalDb-005);
   - column names containing `'` (LocalDb-006).
2. **A fail-closed option the snapshot path skips.** `FailClosedOnDrift` is never checked on the
   snapshot path (LocalDb-004).
3. **The endpoint is unauthenticated, and the option docs suggest otherwise.** The library maps an
   unauthenticated sync endpoint, and `ApiKey`'s doc says the key is "expected from the peer"
   (LocalDb-001). Both current consumers (ScadaBridge, OtOpcUa `LocalDbSyncAuthInterceptor`) close
   the hole with their own interceptors.

Every finding except LocalDb-001 and LocalDb-007 was reproduced with a throwaway test against a
scratch copy of the library (not committed). The suite at `91fb2b4` passes: 216/216 offline (223
with the 7 probes added). The known open items #16 (tombstone resurrection past `TombstoneRetention`)
and #26 (Microsoft.Data.Sqlite pool bump) are excluded.

## Checklist coverage

| # | Category | Examined | Notes |
|---|----------|----------|-------|
| 1 | Correctness & logic bugs | ☑ | Watermarks, ack and prune, horizon gap, rebuilt-peer detection, LWW tie-break and per-entry savepoints are correct. Defects in how the apply path handles some schema shapes: LocalDb-002, LocalDb-003, LocalDb-005, LocalDb-006 |
| 2 | Public API surface & compatibility | ☑ | Proto evolution is additive, with capability negotiation (`supports_partial_snapshot`) and `lib_schema_version` left at 1. On-disk schema upgrades v1→v3 are idempotent. No issues found |
| 3 | Concurrency & thread safety | ☑ | One writer task over two lanes, `_master` guarded by a lock, Interlocked/Volatile on the status fields, takeover is cancel-then-await. HLC `Next`/`Observe` are locked. No issues found |
| 4 | Error handling & resilience | ☑ | Fault teardown flushes the control lane, keepalive pings, backoff with a fast path for schema mismatch, maintenance never dies. The poison catch is broad enough to turn FK-ordering failures into permanent dead-letters: LocalDb-002 |
| 5 | Security & secret handling | ☑ | Table names are regex-gated, all values are parameterized, and `Synchronous` is allow-listed. The sync endpoint is unauthenticated by default and `ApiKey` is documented misleadingly: LocalDb-001. Column-name quoting is incomplete (local schema only, not peer-controlled): LocalDb-006 |
| 6 | Performance & resource management | ☑ | Channel, CTS, call and reader-task disposal checked on every exit path. Statements are prepared once per batch, depth is O(1), batches are byte-budgeted, the inbox is bounded. No issues found |
| 7 | Spec & shared-contract adherence | ☑ | Design §Auth says "ZB.MOM.WW.Auth.ApiKeys keyId/Bearer on the channel", but the lib only sends a header (LocalDb-001). Design §Error handling ("Clock skew … optionally fail-closed") is not applied to snapshots (LocalDb-004) |
| 8 | Packaging, dependencies & project layout | ☑ | Three packages, central package management, TreatWarningsAsErrors, InternalsVisibleTo limited to Replication and Tests. Version 0.3.5 is unpublished (#26). No issues found |
| 9 | Testing coverage | ☑ | Broad, and DI is covered by tests that build the container. No test has a schema with a secondary `UNIQUE`, a non-cascading FK under snapshot, a quoted column name, or a drifted snapshot row. Those are the gaps behind LocalDb-002 through LocalDb-006 |
| 10 | Documentation & XML docs | ☑ | XML docs are thorough. The README is stale in places (LocalDb-007) and the `ApiKey` XML doc is wrong (LocalDb-001) |

## Findings

### LocalDb-001 — Sync endpoint is unauthenticated by default, and `ApiKey` is documented as "expected from the peer"

| | |
|--|--|
| Severity | Medium |
| Priority | P2 |
| Category | Security & secret handling |
| Status | Resolved |
| Location | `ZB.MOM.WW.LocalDb/src/ZB.MOM.WW.LocalDb.Replication/LocalDbSyncService.cs:85`, `ZB.MOM.WW.LocalDb/src/ZB.MOM.WW.LocalDb.Replication/ReplicationOptions.cs:11`, `ZB.MOM.WW.LocalDb/src/ZB.MOM.WW.LocalDb.Replication/DependencyInjection/ReplicationServiceCollectionExtensions.cs:77` |

**Description**

`ReplicationOptions.ApiKey` is documented as "API key presented to / expected from the peer; null
disables key auth". Only the first half is true:

- `SyncBackgroundService` attaches `Authorization: Bearer <key>` to the outbound call.
- `LocalDbSyncService.Sync` never reads request headers.
- `MapZbLocalDbSync` maps the service with no authorization metadata.
- The library ships no interceptor or validator.

So setting `ApiKey` on both nodes, the natural reading of the option, leaves the passive endpoint
open. The README's Ops notes say this, but the XML doc does not. The design doc (§Auth: "`ZB.MOM.WW.Auth.ApiKeys` keyId/Bearer on
the channel") promises enforcement on the channel. Both current consumers wrote their own
fail-closed interceptor (ScadaBridge, and OtOpcUa's `LocalDbSyncAuthInterceptor`). This is the
duplication the shared library exists to remove, and every future consumer has to rediscover it.

The handshake makes an open endpoint easy to exploit. `SyncSession.HandshakeAsync` sends the
server's own `Handshake`, including every registered table name and its schema digest
(`SyncSession.cs:180`), before validating the caller's. A caller can therefore echo the digests
back and pass the fail-closed schema check without knowing the schema.

**Failure scenario**

A third consumer (or a new OtOpcUa/ScadaBridge host profile) calls `AddZbLocalDbReplication` +
`MapZbLocalDbSync`. It sets `LocalDb:Replication:ApiKey` on both nodes and does not add an
interceptor. Any client that can reach the gRPC port can then:

1. Open `Sync`, read the server's Handshake, and reply with the same digests and
   `last_applied_remote_seq = 0`.
2. Receive a full snapshot of every replicated table, which is complete data exfiltration.
3. Push `DeltaBatch` entries with a high HLC that overwrite or tombstone any replicated row. They
   win LWW and are then replicated onward by nothing, because the 2-node topology does not
   re-forward.
4. Repeatedly displace the legitimate peer's stream, because the guard is newest-wins
   (`LocalDbSyncService.cs:152`).

A different `node_id` in step 1 also makes the server reset its inbound watermark
(`SyncSession.cs:199`).

**Recommendation**

Move the interceptor both consumers already wrote into the library. When `ApiKey` is set, the
service should verify `Bearer` with a constant-time compare (`CryptographicOperations.FixedTimeEquals`)
and refuse with `Unauthenticated` otherwise. Make the endpoint fail closed when no key is
configured, unless the host opts out explicitly, for example with `AllowUnauthenticated = true`. Fix
the `ApiKey` XML doc either way. Both consumers could then delete their copies.

**Resolution**

Resolved 2026-11-09 (scadaproj#36, LocalDb `0.3.6`, unpublished). Re-verified: at `91fb2b4` nothing in the library reads
request headers. `LocalDbSyncService.Sync` now calls `IsAuthorized` before the takeover and before the handshake. When
`ApiKey` is set, the stream must carry `Authorization: Bearer <key>`, compared with `CryptographicOperations.FixedTimeEquals`
over UTF-8; anything else is refused with `Unauthenticated`. The check sits before the takeover, so an unauthenticated
caller can neither read the Handshake (table names and digests) nor displace the real peer.

As the task requires, a blank `ApiKey` keeps the old behaviour (accept), and the service logs a startup warning. That
rules out the review's fail-closed-by-default suggestion: a key-less pair must keep syncing. Wire compatibility holds,
because the initiator has sent this header whenever the key is set since `0.1.0`. The ScadaBridge and OtOpcUa
interceptors check the same key and the same header, so they are unaffected. The `ApiKey` XML doc, the README config
table and the README Ops notes are corrected.

Tests (`GrpcAdapterTests`): two-node convergence with the key on both sides. Missing, wrong, scheme-less and
over-long tokens are each refused before the handshake. A correct token gets the Handshake. An unauthenticated
stream does not displace a live peer: there is no redial, and replication continues. A key on the initiator only
still syncs, and a blank, empty or whitespace key admits every caller. With the check disabled, the 5 refusal and
non-displacement cases fail.

### LocalDb-002 — Snapshot streams tables by name and rows by `pk_json` text, so FK children land before parents and are dead-lettered for good

| | |
|--|--|
| Severity | Medium |
| Priority | P2 |
| Category | Correctness & logic bugs |
| Status | Resolved |
| Location | `ZB.MOM.WW.LocalDb/src/ZB.MOM.WW.LocalDb.Replication/Internal/SnapshotStreamer.cs:79` |

**Description**

`SnapshotStreamer.SendAsync` streams tables in ordinal name order
(`.OrderBy(t => t.Name, StringComparer.Ordinal)`, line 79–81). Within a table it pages
`ORDER BY rv.pk_json` (line 196), which is text order: `{"id":10}` sorts before `{"id":2}`.

The receiver applies through `LwwApplier` on a connection with `PRAGMA foreign_keys=ON`. An
immediate FK violation is a `SqliteException`, and `LwwApplier.cs:116` treats it as per-entry
poison: dead-letter, keep going. `SnapshotApplier.OnCompleteAsync` then moves the watermark to
`as_of_seq`, and the sender clears its snapshot debt on the `SnapshotAck`. Nothing ever re-sends the
dead-lettered rows.

Registration explicitly allows these schemas. `EnsureNoInboundActionOnDelete` rejects only
CASCADE / SET NULL / SET DEFAULT, and README §Limitations recommends `NO ACTION`/`RESTRICT`. So a
replicated parent/child pair with a default FK is a supported configuration that the delta path
handles (seq order matches commit order) but the snapshot path does not.

**Failure scenario**

Reproduced in a scratch test:

1. Register `z_parent(id PK)` and `a_child(id PK, parent_id REFERENCES z_parent(id))` on both nodes.
2. Insert `(1)` into `z_parent` and `(10, 1)` into `a_child` on A.
3. Run a full snapshot from A to an empty B, the rebuilt-peer or cap-prune path.

Result on B: `parents=1 children=0`, one dead-letter `a_child: SQLite Error 19: 'FOREIGN KEY
constraint failed'`, watermark advanced to 2. A self-referencing table (`node(id, parent_id
REFERENCES node(id))` with rows 2 and 10→2) loses row 10 the same way. The child rows are gone from
B permanently, and the pair reports itself converged.

No current consumer has an FK on a replicated table (per the 0.3.3 notes), so this is latent. The
first consumer to add one loses data on every snapshot resync.

**Recommendation**

Choose one:

- Apply snapshot batches with `PRAGMA defer_foreign_keys = ON` inside the apply transaction, which
  checks FKs at commit. This only helps if parent and child land in the same transaction, which
  they do not today across batches. More robustly, apply the whole snapshot with FK enforcement off
  (`PRAGMA foreign_keys=OFF` on the snapshot apply connection, set outside a transaction) and run
  `PRAGMA foreign_key_check` at `SnapshotComplete` to log or dead-letter any true orphans.
- Order the snapshot topologically by FK dependency, parents first. Even then a self-referencing
  table still needs deferral.

Also add a snapshot test with a non-cascading FK, and consider not dead-lettering
`SQLITE_CONSTRAINT_FOREIGNKEY` silently on the snapshot path.

**Resolution**

Resolved 2026-11-09 (scadaproj#37, LocalDb `0.3.6`, unpublished). Re-verified with a 2-node test: a wiped peer
received `a_child` before `z_parent` and `node {"id":10}` before `node {"id":2}`. Both children were dead-lettered
(`SQLite Error 19: 'FOREIGN KEY constraint failed'`) and the watermark advanced past them. Fixed on both sides, and
the wire is unchanged:
- **Sender.** `SnapshotStreamer.ParentsFirstAsync` orders the tables in scope topologically by
  `pragma_foreign_key_list` (Kahn's algorithm, ordinal name tie-break). Self-references are ignored, and a cycle
  falls back to name order.
- **Receiver.** `LwwApplier.ApplyBatchAsync` gained `deferForeignKeyViolations`, used only by `SnapshotApplier`. An
  entry that fails with SQLITE_CONSTRAINT_FOREIGNKEY (787) is rolled back to its savepoint and returned as deferred,
  not dead-lettered. The applier retries deferred rows after every batch that applied something, until a pass makes
  no progress. At `SnapshotComplete` it makes one final pass with deferral off, which dead-letters the true orphans
  (with a warning). The delta path is unchanged, because commit order already puts parents first.

Tests (`SnapshotForeignKeyTests`, all 4 fail pre-fix):
- 2-node wiped-peer snapshot with `MaxBatchSize = 1`, cross-table and self-referencing;
- streamer table order;
- a receiver fed child-first in the old sender order;
- a true orphan dead-lettered only at complete.

README §Limitations documents the mixed-version caveat: a pre-0.3.6 receiver still loses child-first self-references.

### LocalDb-003 — `INSERT OR REPLACE` apply silently deletes a different row on a secondary `UNIQUE` conflict, so the pair diverges

| | |
|--|--|
| 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._

### LocalDb-004 — `FailClosedOnDrift` is bypassed on the snapshot path, so a future-stamped snapshot row moves the local HLC forward permanently

| | |
|--|--|
| Severity | Medium |
| Priority | P2 |
| Category | Spec & shared-contract adherence |
| Status | Open |
| Location | `ZB.MOM.WW.LocalDb/src/ZB.MOM.WW.LocalDb.Replication/Internal/SnapshotStreamer.cs:249` |

**Description**

The drift guard (`MaxHlcDriftAhead` + `FailClosedOnDrift`) lives only in
`SyncSession.ApplyDeltaEntriesAsync` (`SyncSession.cs:599-628`). `SnapshotApplier.OnBatchAsync`
passes every snapshot row straight to `LwwApplier.ApplyBatchAsync`. It takes no
`ReplicationOptions`, so it cannot check drift. The applier then calls
`db.Clock.Observe(maxHlc)` (`LwwApplier.cs:155`), and `OnCompleteAsync` persists `last_seen_hlc`.
That value feeds clock recovery on every later open (`SqliteLocalDb.cs:69-75`).

This contradicts README §Semantics ("With `FailClosedOnDrift`, each over-drift entry is
dead-lettered") and design §Error handling ("configurable max-drift guard (warn, optionally
fail-closed)").

**Failure scenario**

Reproduced in a scratch test with `FailClosedOnDrift = true`. A peer whose clock was a year ahead
when it wrote a row serves a snapshot after a cap prune or rebuild. The receiving node writes 0
dead-letters, applies the row, and its HLC jumps about 365 days ahead. Every later local write on
that node is stamped a year in the future, and the stamp survives restarts through `last_seen_hlc`.
From then on those writes beat every genuine write on the peer under LWW until wall-clock time
catches up. The operator enabled `FailClosedOnDrift` specifically to prevent this.

**Recommendation**

Give `SnapshotApplier` the options and the clock, and run the same per-row drift check as the delta
path: dead-letter over-drift rows when `FailClosedOnDrift`, otherwise warn once. Better, move the
check into `LwwApplier` so both paths share it. Add a snapshot-drift test.

**Resolution**

_Unresolved._

### LocalDb-005 — Auto-assigned rowid / AUTOINCREMENT keys collide across nodes and LWW silently drops one row

| | |
|--|--|
| Severity | Low |
| Priority | P3 |
| Category | Documentation & XML docs |
| Status | Open |
| Location | `ZB.MOM.WW.LocalDb/src/ZB.MOM.WW.LocalDb/Internal/SqliteLocalDb.cs:197` |

**Description**

Registration accepts any explicit PK, including an `INTEGER PRIMARY KEY [AUTOINCREMENT]` rowid
alias that each node assigns on its own. LWW keys on `pk_json`, so two distinct rows inserted with
`NULL` ids on the two nodes are treated as two versions of the same row, and one is discarded. This
is inherent to LWW with locally-assigned keys. However, neither the README §Limitations nor the
`RegisterReplicated` docs say that replicated keys must be globally unique (GUIDs or node-scoped).
The README quickstart even uses `id INTEGER PRIMARY KEY`.

**Failure scenario**

Reproduced in a scratch test:

1. Create `ev(id INTEGER PRIMARY KEY AUTOINCREMENT, msg TEXT)` and register it on both nodes.
2. Insert `('from A')` on A and `('from B')` on B, for example when both nodes of a pair log events
   during a split.
3. Exchange deltas.

Both nodes end with `1:from A` only. B's row is lost on both nodes, with no dead-letter or metric.

**Recommendation**

Document in README §Limitations and the `RegisterReplicated` XML docs that replicated PKs must be
globally unique, and that a rowid alias assigned on both nodes loses rows. Optionally log a warning
at registration when the sole PK column is an `INTEGER PRIMARY KEY` rowid alias.

**Resolution**

_Unresolved._

### LocalDb-006 — Column names containing `'` are captured but break the apply and snapshot SQL

| | |
|--|--|
| Severity | Low |
| Priority | P3 |
| Category | Correctness & logic bugs |
| Status | Open |
| Location | `ZB.MOM.WW.LocalDb/src/ZB.MOM.WW.LocalDb.Replication/Internal/LwwApplier.cs:349` |

**Description**

`TriggerSqlGenerator` escapes both quote styles (`Ident` doubles `"`, `Literal` doubles `'`), so
capture works for any column name. The replication side builds JSON paths with
`JsonPath(column) => "'$.\"" + column.Replace("\"", "\\\"") + "\"'"` (`LwwApplier.cs:349`,
`SnapshotStreamer.cs:205`). That code escapes `"` but not `'`, so a column name containing an
apostrophe ends the SQL string literal early. Only table names are regex-gated at registration
(`SqliteLocalDb.cs:172`); column names come from `PRAGMA table_info` unchecked. The input is the
local schema rather than the peer, so this is a robustness issue, not an injection vector.

**Failure scenario**

Reproduced in a scratch test. For a table with a column `"it's"`, capture on A succeeds. Applying on
B fails `Prepare` with `SQLite Error 1: near "s": syntax error`. That is a `SqliteException` inside
the per-entry try, so every row of the table is dead-lettered and acked (`applied=0 dl=1`). If the
column is part of the PK, the snapshot page query also throws and faults the session on every
reconnect.

**Recommendation**

Escape `'` in `JsonPath`, for example by building the path text and then passing it through the same
`Literal`-style doubling. Alternatively, validate column names at registration with the same
identifier regex used for table names, so registration fails loudly. Add a quoted-column round-trip
test.

**Resolution**

_Unresolved._

### LocalDb-007 — README is stale on version, adoption, test count, `Synchronous` values and baseline scope

| | |
|--|--|
| Severity | Low |
| Priority | P3 |
| Category | Documentation & XML docs |
| Status | Open |
| Location | `ZB.MOM.WW.LocalDb/README.md:13` |

**Description**

Several README statements no longer match the code or the library's own `CLAUDE.md`:

- line 13: "Packages (all `0.3.4`)". `Directory.Build.props` is `0.3.5`.
- line 21: "adopted by … OtOpcUa (Phase 1)". OtOpcUa has adopted Phases 1+2.
- line 228: `Synchronous` lists `OFF` / `NORMAL` / `FULL`. The validator also accepts `EXTRA`
  (`LocalDbOptionsValidator.cs:14`).
- line 373: "see Limitations: baselining flags a full, all-tables snapshot". This contradicts the
  0.3.0 per-table behaviour described in the same README at lines 264–266 and 315.
- line 390: "199 tests". There are 216 at this commit.

**Failure scenario**

An operator re-enabling a table follows line 373 and trims every high-volume table, expecting an
all-tables snapshot. That is unnecessary work, and it obscures the real rule: only the baselined
table, an old peer, or a cap prune streams in full. A reader checking which version is current gets
0.3.4 from the README and 0.3.5 from the build.

**Recommendation**

Refresh those five lines when 0.3.5 is published.

**Resolution**

_Unresolved._