From b9a096c53ab72f00b075f2ad8ed827ad061e79f9 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 9 Nov 2026 15:13:24 -0500 Subject: [PATCH] docs(localdb): refresh README, record 0.3.6 #46/#50 notes (LocalDb-007) README: 0.3.6 version heading and changelog entries (0.3.5, 0.3.6), OtOpcUa Phases 1+2, Synchronous EXTRA, per-table baseline wording in Ops notes, test count 249. CLAUDE.md records LocalDb-005/006/007 and the #50 items, with the cross-repo LocalDbSyncAuthInterceptor decision left as a note. LocalDb findings now 0 open; code-reviews/README.md regenerated. (scadaproj#46) Claude-Session: https://claude.ai/code/session_01GT67AEHd2Acv1YPwGfXEoV --- ZB.MOM.WW.LocalDb/CLAUDE.md | 6 +++++- ZB.MOM.WW.LocalDb/README.md | 27 ++++++++++++++++++++++----- code-reviews/LocalDb/findings.md | 17 ++++++++++++++--- code-reviews/README.md | 13 +++++-------- 4 files changed, 46 insertions(+), 17 deletions(-) diff --git a/ZB.MOM.WW.LocalDb/CLAUDE.md b/ZB.MOM.WW.LocalDb/CLAUDE.md index ed09401..6ff118b 100644 --- a/ZB.MOM.WW.LocalDb/CLAUDE.md +++ b/ZB.MOM.WW.LocalDb/CLAUDE.md @@ -6,11 +6,15 @@ Shared embedded SQLite cache + 2-node LWW sync library for the **ZB.MOM.WW SCADA **`0.3.5` (unpublished, scadaproj#26):** `Microsoft.Data.Sqlite` 10.0.7 -> 10.0.13 (efcore#39009 pool fault, which a LocalDb consumer is exposed to); `SQLitePCLRaw.lib.e_sqlite3` transitive pin 2.1.12 -> 2.1.13 (10.0.13 floors 2.1.13). 216/216; no wire/schema change. -**`0.3.6` (unpublished, scadaproj#36–#39):** code-review fixes, all wire-compatible (no proto, `lib_schema_version` or on-disk schema change). +**`0.3.6` (unpublished, scadaproj#36–#39, #46, #50):** code-review fixes, all wire-compatible (no proto, `lib_schema_version` or on-disk schema change). - **#36 LocalDb-001:** the passive `LocalDbSyncService` now enforces `LocalDb:Replication:ApiKey` itself (Bearer, constant-time, checked before the handshake and before newest-wins takeover; `Unauthenticated` otherwise). Blank key = unchanged behaviour plus a startup warning. ScadaBridge's and OtOpcUa's own `LocalDbSyncAuthInterceptor`s use the same key and header, so they keep working; they are now redundant except for their fail-closed-when-blank rule. - **#37 LocalDb-002:** snapshots deliver FK-linked rows in any order. `SnapshotStreamer` sends referenced tables first (Kahn order, name tie-break); `SnapshotApplier` defers rows that fail only SQLITE_CONSTRAINT_FOREIGNKEY (`LwwApplier.ApplyBatchAsync(..., deferForeignKeyViolations: true)`), retries them as rows land, and dead-letters true orphans at `SnapshotComplete`. Before, a child that arrived first was dead-lettered and acked over permanently. - **#38 LocalDb-003:** the applier upserts with `INSERT … ON CONFLICT() DO UPDATE` (`DO NOTHING` when every column is PK) instead of `INSERT OR REPLACE`. A secondary-`UNIQUE` clash is now a dead-letter instead of a silent delete of another row; as a side effect a remote update of a parent with `ON DELETE RESTRICT` children applies instead of being dead-lettered. - **#39 LocalDb-004:** `SnapshotApplier` now takes `ReplicationOptions` (wired by `SyncSessionFactory`) and runs the delta path's drift guard per snapshot row (shared `HlcDriftGuard` dead-letter); a dead-lettered row's HLC is kept out of `last_seen_hlc`. Before, `FailClosedOnDrift` was skipped on snapshots and one future-stamped row moved the receiver's HLC ahead across restarts. +- **#46 LocalDb-005:** `RegisterReplicated` **refuses an `AUTOINCREMENT` primary key** (`LocalDbRegistrationException`, detected from SQLite column metadata). Each node assigns its own, so two different rows shared a `pk_json` and LWW kept one on both nodes. Plain `INTEGER PRIMARY KEY` is still accepted (app-assigned ids); replicated keys must be unique across both nodes — documented. ScadaBridge's 12 and OtOpcUa's 5 replicated tables are all TEXT/composite keys (checked 2026-11-09), so neither boot is affected. This is the only 0.3.6 change that can refuse a schema an earlier version accepted. +- **#46 LocalDb-006:** apply/snapshot JSON paths double `'` (shared `JsonPathSql.For`); a column named `it's` used to dead-letter every row of its table on the peer. **LocalDb-007:** README refreshed (version, OtOpcUa Phases 1+2, `EXTRA`, per-table baseline wording, test count). +- **#50:** snapshot-path dead-letters (poison, drift, FK orphan, cap spill) now count in `localdb.sync.dead_lettered` (`SnapshotApplier` takes `LocalDbMetrics`, wired by `SyncSessionFactory`). New option **`LocalDb:Replication:MaxSnapshotDeferredRows`** (default 100,000, validated > 0) caps snapshot rows held for a missing FK parent; past it the oldest are re-applied once and dead-lettered if still orphaned. **Open note (cross-repo, #50):** once ScadaBridge and OtOpcUa pin 0.3.6, their own `LocalDbSyncAuthInterceptor`s duplicate the library check — they differ only in refusing every stream when no key is set. Each app decides whether to keep that stricter rule or delete its interceptor; nothing in this library changes for either choice. +249/249 tests. ## Index history (migrated from the scadaproj root CLAUDE.md) diff --git a/ZB.MOM.WW.LocalDb/README.md b/ZB.MOM.WW.LocalDb/README.md index d62a293..70be1e1 100644 --- a/ZB.MOM.WW.LocalDb/README.md +++ b/ZB.MOM.WW.LocalDb/README.md @@ -10,7 +10,7 @@ Change capture uses SQLite triggers writing a single HLC-stamped oplog inside th consumer's own write transaction (crash-consistent, non-bypassable). Convergence is last-writer-wins over a hybrid logical clock. All timestamps are UTC. -## Packages (all `0.3.4`) +## Packages (all `0.3.6`; the feed carries up to `0.3.4` — `0.3.5` and `0.3.6` are unpublished) | Package | Contents | |---|---| @@ -18,8 +18,24 @@ last-writer-wins over a hybrid logical clock. All timestamps are UTC. | `ZB.MOM.WW.LocalDb.Contracts` | `localdb_sync.v1` proto + generated gRPC types (packable; a future peer could be non-.NET). | | `ZB.MOM.WW.LocalDb.Replication` | gRPC sync engine: passive `LocalDbSyncService`, initiating `SyncBackgroundService`, `MaintenanceBackgroundService`, LWW apply, watermark acks + pruning, snapshot resync, `ISyncStatus`, `LocalDbMetrics`, `AddZbLocalDbReplication()` / `MapZbLocalDbSync()`. | -> Status: **published to the Gitea feed; adopted by ScadaBridge (Phases 1+2) and OtOpcUa (Phase 1).** +> Status: **published to the Gitea feed (through `0.3.4`); adopted by ScadaBridge (Phases 1+2) and +> OtOpcUa (Phases 1+2), both pinned to `0.3.4`.** > +> - `0.3.6` (unpublished) — code-review fixes (scadaproj#36–#39, #46, #50). Wire-compatible: no proto +> change, no `lib_schema_version` bump, no on-disk schema change. +> - The passive endpoint enforces `ApiKey` itself (Bearer, constant-time, before the handshake). +> - Snapshots deliver FK-linked rows in any order (parents first; FK-failing rows deferred and +> retried; true orphans dead-lettered at completion), with the held-back rows capped by the new +> `MaxSnapshotDeferredRows`. +> - Remote rows are upserted on the primary key (`ON CONFLICT() DO UPDATE`), not +> `INSERT OR REPLACE`, so a secondary-`UNIQUE` clash is a dead-letter, not a silent delete. +> - `FailClosedOnDrift` covers snapshot rows. +> - Snapshot-path dead-letters are counted in `localdb.sync.dead_lettered`. +> - Column names containing `'` replicate (they broke the apply/snapshot SQL). +> - **`RegisterReplicated` refuses an `AUTOINCREMENT` primary key** (see Limitations) — the one +> change that can refuse a schema an earlier version accepted. +> - `0.3.5` (unpublished) — `Microsoft.Data.Sqlite` 10.0.7 → 10.0.13 (efcore#39009 pool fault) and +> the `SQLitePCLRaw.lib.e_sqlite3` transitive pin 2.1.12 → 2.1.13. No wire or schema change. > - `0.3.4` (2026-08-20) — **a converged pair no longer re-streams a full snapshot on every > reconnect once the sender's oplog has been ack-pruned to empty.** Found on the ScadaBridge > docker rig (its register row 106): a rebuilt node (watermark 0) was healed by a snapshot @@ -227,7 +243,7 @@ node still pushes its own deltas back. Only one side needs `PeerAddress`. |---|---|---| | `Path` | `""` (required) | Filesystem path to the SQLite database file. | | `BusyTimeoutMs` | `5000` | SQLite `busy_timeout` pragma, in milliseconds. | -| `Synchronous` | `"NORMAL"` | SQLite `synchronous` pragma (`OFF` / `NORMAL` / `FULL`). | +| `Synchronous` | `"NORMAL"` | SQLite `synchronous` pragma (`OFF` / `NORMAL` / `FULL` / `EXTRA`, case-insensitive). | ### `LocalDb:Replication` section (`ReplicationOptions`) @@ -405,7 +421,8 @@ provider is wired or the poll threw, so a DB failure never reads as "0 backlog") replicates stops syncing with a schema-mismatch error. Deregister on both nodes, or on neither. Re-enabling later should pass `baselineExistingRows: true`, or the rows that accumulated while capture was off stay invisible to the peer — but trim high-volume - tables first (see Limitations: baselining flags a full, all-tables snapshot). The HLC's + that table first (see Limitations: baselining streams that one table in full, not every + table — only an oplog cap prune or a pre-`0.3.0` peer streams everything). The HLC's durable anchor is flushed in the same transaction, so a crash after a deregister boot cannot bring the clock back below stamps peers already hold. - **On-disk schema upgrades** are applied in place when the file is opened, in one @@ -434,7 +451,7 @@ provider is wired or the poll threw, so a DB failure never reads as "0 backlog") ## Testing -199 tests, all offline / no live dependencies — unit (HLC ordering + restart +249 tests, all offline / no live dependencies — unit (HLC ordering + restart persistence, deregistration's HLC anchor surviving an ungraceful reopen, trigger SQL generation, LWW decision table, watermark/pruning, digest computation, schema-version upgrade, deregistration idempotency, baseline-floor LWW safety, O(1) depth vs a seeded diff --git a/code-reviews/LocalDb/findings.md b/code-reviews/LocalDb/findings.md index efe1bc8..dd51be0 100644 --- a/code-reviews/LocalDb/findings.md +++ b/code-reviews/LocalDb/findings.md @@ -10,7 +10,7 @@ | Last reviewed | 2026-11-09 | | Reviewer | Claude | | Commit reviewed | `91fb2b4` | -| Open findings | 1 | +| Open findings | 0 | ## Summary @@ -473,7 +473,7 @@ the quoted PK) and a wiped-peer snapshot through a quoted PK column. Both fail p | Severity | Low | | Priority | P3 | | Category | Documentation & XML docs | -| Status | Open | +| Status | Resolved | | Location | `ZB.MOM.WW.LocalDb/README.md:13` | **Description** @@ -501,4 +501,15 @@ Refresh those five lines when 0.3.5 is published. **Resolution** -_Unresolved._ +Resolved 2026-11-09 (scadaproj#46, LocalDb `0.3.6`, unpublished). Re-verified: all five statements were still +stale at `c207573`, and the version had since moved to `0.3.6`. README changes: +- **Packages heading.** It now reads `0.3.6`, and notes that the feed carries up to `0.3.4` and that `0.3.5` and + `0.3.6` are unpublished. +- **Status.** OtOpcUa is listed as adopting Phases 1+2, both apps are shown pinned to `0.3.4`, and there are new + `0.3.6` and `0.3.5` entries. +- **`Synchronous`.** It lists `OFF` / `NORMAL` / `FULL` / `EXTRA`, matching `LocalDbOptionsValidator.AllowedSynchronous`. +- **Ops notes.** The "baselining flags a full, all-tables snapshot" cross-reference now says that baselining streams + that one table in full, and that only a cap prune or a pre-`0.3.0` peer streams everything. +- **Test count.** It is 249, the count after the #46/#50 fixes. + +Documentation only; no test applies. diff --git a/code-reviews/README.md b/code-reviews/README.md index 8a6dc57..b6b641f 100644 --- a/code-reviews/README.md +++ b/code-reviews/README.md @@ -40,7 +40,7 @@ code-reviews/ ## Summary -10 of 10 libraries reviewed. 19 pending findings across all libraries. +10 of 10 libraries reviewed. 16 pending findings across all libraries. Coverage: Secrets, LocalDb, GalaxyRepository and Overview first reviewed, and Auth, Health, Theme, Audit, Configuration and Telemetry re-reviewed, on 2026-11-09 (#15). Each finding also @@ -51,8 +51,8 @@ carries a P1/P2/P3 priority (P1 = Critical/High, P2 = Medium, P3 = Low). | Critical | 0 | | High | 0 | | Medium | 0 | -| Low | 19 | -| **Total** | **19** | +| Low | 16 | +| **Total** | **16** | ## Library Status @@ -63,7 +63,7 @@ carries a P1/P2/P3 priority (P1 = Critical/High, P2 = Medium, P3 = Low). | [Configuration](Configuration/findings.md) | 2026-11-09 | `91fb2b4` | 0/0/0/0 | 0 | 4 | | [GalaxyRepository](GalaxyRepository/findings.md) | 2026-11-09 | `91fb2b4` | 0/0/0/0 | 0 | 8 | | [Health](Health/findings.md) | 2026-11-09 | `91fb2b4` | 0/0/0/0 | 0 | 7 | -| [LocalDb](LocalDb/findings.md) | 2026-11-09 | `91fb2b4` | 0/0/0/3 | 3 | 7 | +| [LocalDb](LocalDb/findings.md) | 2026-11-09 | `91fb2b4` | 0/0/0/0 | 0 | 7 | | [Overview](Overview/findings.md) | 2026-11-09 | `91fb2b4` | 0/0/0/0 | 0 | 5 | | [Secrets](Secrets/findings.md) | 2026-11-09 | `91fb2b4` | 0/0/0/13 | 13 | 19 | | [Telemetry](Telemetry/findings.md) | 2026-11-09 | `91fb2b4` | 0/0/0/0 | 0 | 11 | @@ -88,16 +88,13 @@ _None open._ _None open._ -### Low (19) +### Low (16) | ID | Library | Title | |----|---------|-------| | Auth-008 | [Auth](Auth/findings.md) | SQL Server store silently truncates over-length values and matches key ids case-insensitively, diverging from SQLite | | Auth-009 | [Auth](Auth/findings.md) | Canonical username is the first value of a possibly multi-valued attribute, not the value the user matched | | Auth-010 | [Auth](Auth/findings.md) | `ApiKeyVerifier` lets store exceptions escape despite its "only exception path is cancellation" contract | -| LocalDb-005 | [LocalDb](LocalDb/findings.md) | Auto-assigned rowid / AUTOINCREMENT keys collide across nodes and LWW silently drops one row | -| LocalDb-006 | [LocalDb](LocalDb/findings.md) | Column names containing `'` are captured but break the apply and snapshot SQL | -| LocalDb-007 | [LocalDb](LocalDb/findings.md) | README is stale on version, adoption, test count, `Synchronous` values and baseline scope | | Secrets-004 | [Secrets](Secrets/findings.md) | Corrupt or wrong-length crypto fields escape `Decrypt` as `ArgumentException`: not audited, and breaks the cipher contract | | Secrets-005 | [Secrets](Secrets/findings.md) | Store-path rules are lexical: drive-relative paths pass "rooted", and `\\?\` / UNC / 8.3 / symlink spellings bypass "under content root" | | Secrets-006 | [Secrets](Secrets/findings.md) | Resolver cache can re-cache a stale value after `Invalidate`, and expired plaintext is never evicted |