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	6

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	Open
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

Unresolved.

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.