diff --git a/docs/plans/2026-07-19-localdb-adoption-phase1.md b/docs/plans/2026-07-19-localdb-adoption-phase1.md index a3a93254..c974d352 100644 --- a/docs/plans/2026-07-19-localdb-adoption-phase1.md +++ b/docs/plans/2026-07-19-localdb-adoption-phase1.md @@ -18,8 +18,8 @@ | 1 | Task 1 (packages) | | 2 | Task 2 (schema helpers) | | 3 | Task 3 (composition root) | -| 4 | Task 4 (tracking store) ∥ Task 5 (event logger) ∥ Task 7 (replication wiring) | -| 5 | Task 6 (migrator) ∥ Task 8 (auth interceptor) ∥ Task 9 (health signal) | +| 4 | Task 4 (tracking store) ∥ Task 5a (event writer) ∥ Task 7 (replication wiring) | +| 5 | Task 5b (event read path) ∥ Task 6 (migrator) ∥ Task 8 (auth interceptor) ∥ Task 9 (health signal) | | 6 | Task 10 (convergence test) ∥ Task 11 (rig config) | | 7 | Task 12 (live gate — serial, on the real rig) | | 8 | Task 13 (docs) ∥ Task 14 (Phase 2 planning) | @@ -181,8 +181,9 @@ services.AddZbLocalDb(config, SiteLocalDbSetup.OnReady); **Files:** - Modify: `src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Tracking/OperationTrackingStore.cs` -- Modify: wherever `AddSiteRuntime` registers the store (follow `services.AddSiteRuntime` from `SiteServiceRegistration.cs:51` into the SiteRuntime SCE) +- Modify: `src/ZB.MOM.WW.ScadaBridge.SiteRuntime/ServiceCollectionExtensions.cs:74` (the `IOperationTrackingStore` registration; `AddSiteRuntime`'s connection-string parameter becomes vestigial for this store — leave the parameter alone, it still feeds the site config DB) - Test: `tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Tracking/` (existing store tests) +- Test: `tests/ZB.MOM.WW.ScadaBridge.Host.Tests/CompositionRootTests.cs:433` (`Site_Resolves_IOperationTrackingStore` — it overrides the connection string to a temp path because the store opens eagerly in its ctor; that override changes shape once the store takes `ILocalDb`) **Step 1:** Constructor gains `ILocalDb localDb`; `_writeConnection = localDb.CreateConnection()` replaces `new SqliteConnection(_connectionString)` + `Open()` (verified in the lib, `SqliteLocalDb.cs:83-98`: CreateConnection returns an **open** connection with per-connection pragmas — `synchronous`, `busy_timeout`, `foreign_keys=ON` — and the `zb_hlc_next` UDF registered; do NOT call `Open()` on it again). Reader paths that open ad-hoc connections from `_connectionString` switch to `localDb.CreateConnection()` too (reads don't fire triggers, but one connection source is one less config knob). `OperationTrackingOptions.ConnectionString` becomes unused by the store — keep the options class but drop the store's dependency; leave a validator relaxation only if the DI-graph test demands it. @@ -194,25 +195,70 @@ services.AddZbLocalDb(config, SiteLocalDbSetup.OnReady); --- -### Task 5: Rewire SiteEventLogger + EventLogQueryService (GUID ids) +### Task 5a: Rewire SiteEventLogger onto ILocalDb + mint GUID ids **Classification:** standard -**Estimated implement time:** ~5 min +**Estimated implement time:** ~4 min **Parallelizable with:** Task 4, Task 7 +> **Amended 2026-07-19 (execution review).** The original Task 5 said "any `ORDER BY id` +> becomes `ORDER BY timestamp`" and listed 3 files. Code recon found the integer `id` is +> load-bearing in three separate places beyond the writer — a keyset cursor, the +> storage-cap purge, and `long`-typed DTOs that cross the site↔central Akka boundary. +> Task 5 is therefore split: **5a = the writer** (this task, self-contained), **5b = the +> read/purge/contract change** (high-risk, its own task). Do not fold 5b back into 5a. + **Files:** - Modify: `src/ZB.MOM.WW.ScadaBridge.SiteEventLogging/SiteEventLogger.cs` -- Modify: `src/ZB.MOM.WW.ScadaBridge.SiteEventLogging/EventLogQueryService.cs` - Modify: `src/ZB.MOM.WW.ScadaBridge.SiteEventLogging/ServiceCollectionExtensions.cs` (AddSiteEventLogging registration) -- Test: `tests/ZB.MOM.WW.ScadaBridge.SiteEventLogging.Tests/` (existing suite) +- Test: `tests/ZB.MOM.WW.ScadaBridge.SiteEventLogging.Tests/` (writer-path tests only) -**Step 1:** `SiteEventLogger` constructor gains `ILocalDb localDb`; its single owned `_connection` becomes `localDb.CreateConnection()`. Keep the `connectionStringOverride` test seam by preferring the override when non-null (tests may still use raw files for non-replicated scenarios) — but note overridden connections lack the HLC UDF, so tests that write must use the ILocalDb path once tables are registered; simplest is to switch the test fixture to a real `ILocalDb` and delete the override parameter if nothing else uses it. +**Step 1:** `SiteEventLogger` constructor gains `ILocalDb localDb`; its single owned `_connection` (`SiteEventLogger.cs:34,62-65`) becomes `localDb.CreateConnection()` — already open, pragma-configured, HLC-UDF-registered; do **not** call `Open()` again. Delete the `connectionStringOverride` seam (`:50`) rather than preserving it: a raw connection lacks `zb_hlc_next()`, so any test writing through the override against a registered table now **fails closed**. Switch the test fixture to a real `ILocalDb` over a temp file. -**Step 2:** The insert statement gains an `id` column bound to `Guid.NewGuid().ToString("N")` (minted in the writer loop per event). `EventLogQueryService` (read-only) switches its connection source the same way; any `ORDER BY id` becomes `ORDER BY timestamp` (GUIDs don't sort chronologically — check the query paths and the UI paging contract). +**Step 2:** Preserve the `internal WithConnection` seam (`:104`, `:123`) — `EventLogQueryService` and `EventLogPurgeService` both depend on it deliberately (their ctors take the concrete `SiteEventLogger`, not the interface). Its body changes connection source only; the signature and locking semantics stay identical, so 5b's consumers keep compiling. -**Step 3:** Fix remaining writer-path tests from Task 2. Full `dotnet test tests/ZB.MOM.WW.ScadaBridge.SiteEventLogging.Tests` — PASS. +**Step 3:** The INSERT (`:237-240`) gains an `id` column bound to `Guid.NewGuid().ToString("N")`, minted per event in the writer loop. `ISiteEventLogger.LogEventAsync`'s signature is unchanged — all 18 fire-and-forget call sites are untouched. -**Step 4:** Commit. +**Step 4:** Fix writer-path tests from Task 2. Run `dotnet test tests/ZB.MOM.WW.ScadaBridge.SiteEventLogging.Tests` — writer tests PASS. Query/purge tests may be red here; 5b closes them. Note that in the commit message. + +**Step 5:** Commit. + +--- + +### Task 5b: Migrate the event-log read path off integer ids + +**Classification:** high-risk (cross-cluster data contract + silent-data-loss paging bug) +**Estimated implement time:** ~5 min +**Parallelizable with:** Task 6, Task 8, Task 9 +**Blocked by:** Task 5a + +The GUID PK breaks three integer-id assumptions. All three must move together — shipping any one alone is a live bug. + +**Files:** +- Modify: `src/ZB.MOM.WW.ScadaBridge.SiteEventLogging/EventLogQueryService.cs` (cursor `:68-71`, `ORDER BY id ASC` `:141`, `GetInt64(0)` `:153`, token mint `:181`) +- Modify: `src/ZB.MOM.WW.ScadaBridge.SiteEventLogging/EventLogPurgeService.cs` (storage-cap delete `:162-166`) +- Modify: `src/ZB.MOM.WW.ScadaBridge.Commons/Messages/RemoteQuery/EventLogQueryResponse.cs` (`EventLogEntry.Id` `long`→`string`; `ContinuationToken` `long?`→`string?`) +- Modify: `src/ZB.MOM.WW.ScadaBridge.Commons/Messages/RemoteQuery/EventLogQueryRequest.cs` (`ContinuationToken` `long?`→`string?`) +- Modify: `src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs:3127` (passes `null` today — verify it still compiles) +- Modify: `src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Pages/Monitoring/EventLogs.razor:247,268,271` (`_continuationToken` type) +- Test: `tests/ZB.MOM.WW.ScadaBridge.SiteEventLogging.Tests/` (query + purge) + +**Step 1: Write the failing tests first.** +- Paging: insert N events with known timestamps, page through with `PageSize` < N, assert **every** row is returned exactly once across pages and in chronological order. This test fails loudly against a naive `id > $afterId` GUID cursor — that is the point. +- Ordering: rows come back oldest-first regardless of GUID values. +- Purge: with the storage cap tripped, the **oldest** rows are deleted, not arbitrary ones. + +**Step 2:** Run — FAIL. + +**Step 3: Implement.** +- **Cursor** → composite keyset on `(timestamp, id)`, since `timestamp` alone is not unique: + `WHERE (timestamp > $afterTs) OR (timestamp = $afterTs AND id > $afterId)`, `ORDER BY timestamp ASC, id ASC`. The continuation token encodes both — use `"{timestamp}|{id}"` as the `string?` token; parse defensively and treat a malformed token as "start from the beginning" rather than throwing. +- **Purge** (`:162-166`) → `ORDER BY timestamp ASC` instead of `ORDER BY id ASC`. (The retention purge at `:128` already filters on `timestamp` and is unaffected.) +- **DTOs** → `Id` and both `ContinuationToken`s become `string`/`string?`; reader uses `GetString(0)`. + +**Step 4: Wire-compat check.** These DTOs cross the site↔central Akka boundary (`SiteCommunicationActor.cs:193` → `CommunicationService.cs:304` → ManagementActor / CentralUI). Both sides ship in the same deployable and the rig redeploys as a unit, so no rolling-upgrade shim is needed — **but state that explicitly in the commit message**, and confirm no serializer manifest/binding pins these types by name+type (grep the Akka serialization setup). If a binding exists, stop and surface it. + +**Step 5:** `dotnet test tests/ZB.MOM.WW.ScadaBridge.SiteEventLogging.Tests` + full solution build (0 warnings) — PASS. Commit. --- @@ -228,7 +274,8 @@ services.AddZbLocalDb(config, SiteLocalDbSetup.OnReady); - Test: `tests/ZB.MOM.WW.ScadaBridge.Host.Tests/SiteLocalDbLegacyMigratorTests.cs` Semantics (write the tests first, one per bullet): -- Legacy files located from the OLD config keys (`ScadaBridge:OperationTracking:ConnectionString`, `ScadaBridge:SiteEventLog` DatabasePath); missing file ⇒ no-op. +- Legacy files located from the OLD config keys (`ScadaBridge:OperationTracking:ConnectionString`, `ScadaBridge:SiteEventLog:DatabasePath`); missing file ⇒ no-op. +- **Both keys are unset in every docker appsettings today**, so they fall back to the code defaults `"Data Source=site-tracking.db"` (`OperationTrackingOptions.cs:14`) and `"site_events.db"` (`SiteEventLogOptions.cs:10`) — **CWD-relative**, i.e. `/app/site-tracking.db` and `/app/site_events.db`, NOT `/app/data/`. Resolve relative paths against the process CWD; do not assume the data volume. (Consequence worth knowing: both DBs live outside the mounted volume today and are already lost on every container recreate — so on the rig the migrator will usually find nothing, and that is a legitimate no-op, not a failure. Phase 1 incidentally fixes that data-loss bug by consolidating into `/app/data/site-localdb.db`.) - Runs **after** `RegisterReplicated` so migrated rows enter the oplog and replicate. - Tracking rows copy with `INSERT OR IGNORE` (same TEXT PK ⇒ idempotent). - Legacy `site_events` rows get **deterministic** ids — `"mig-{NodeName}-{legacyId}"` — NOT fresh GUIDs, so a crashed-then-rerun migration cannot duplicate events (`INSERT OR IGNORE` on the deterministic key). NodeName from `ScadaBridge:Node:NodeName`. @@ -312,6 +359,7 @@ Bridge `ISyncStatus` (`Connected`, `OplogBacklog` — **nullable**, null must su **Classification:** high-risk **Estimated implement time:** ~5 min **Parallelizable with:** Task 11 +**Blocked by:** Task 6, Task 8, **Task 5b** (scenario 3 reads events back through the query path) **Files:** - Create: `tests/ZB.MOM.WW.ScadaBridge.IntegrationTests/LocalDbSitePairConvergenceTests.cs` diff --git a/docs/plans/2026-07-19-localdb-adoption-phase1.md.tasks.json b/docs/plans/2026-07-19-localdb-adoption-phase1.md.tasks.json index 5b857656..239a6171 100644 --- a/docs/plans/2026-07-19-localdb-adoption-phase1.md.tasks.json +++ b/docs/plans/2026-07-19-localdb-adoption-phase1.md.tasks.json @@ -4,19 +4,28 @@ "mode": "parallel-waves", "implementerModel": "opus", "isolation": "worktree", + "branch": "feat/localdb-phase1", "note": "Dispatch every unblocked task concurrently (waves in the plan header). Parallel implementers MUST use worktree isolation - concurrent git in one worktree races destructively." }, + "amendments": [ + { + "date": "2026-07-19", + "by": "execution review", + "change": "Task 5 split into 5a (id 5, writer) and 5b (id 15, read path). Code recon found the integer site_events.id load-bearing in the keyset cursor (EventLogQueryService:70,141,153), the storage-cap purge (EventLogPurgeService:164), and long-typed DTOs crossing the site<->central Akka boundary (EventLogEntry.Id, both ContinuationTokens) - none of which were in the original Task 5 file set. 5b is high-risk. Task 10 gains a blockedBy on 15." + } + ], "tasks": [ {"id": 1, "subject": "Task 1: Add LocalDb packages to the build", "status": "pending"}, {"id": 2, "subject": "Task 2: Shared schema helpers (tracking + events, GUID PK)", "status": "pending", "blockedBy": [1]}, {"id": 3, "subject": "Task 3: Register LocalDb in the site composition root", "status": "pending", "blockedBy": [2]}, {"id": 4, "subject": "Task 4: Rewire OperationTrackingStore onto ILocalDb connections", "status": "pending", "blockedBy": [3]}, - {"id": 5, "subject": "Task 5: Rewire SiteEventLogger + EventLogQueryService (GUID ids)", "status": "pending", "blockedBy": [3]}, + {"id": 5, "subject": "Task 5a: Rewire SiteEventLogger onto ILocalDb + mint GUID ids", "status": "pending", "blockedBy": [3]}, + {"id": 15, "subject": "Task 5b: Migrate the event-log read path off integer ids", "status": "pending", "blockedBy": [5]}, {"id": 6, "subject": "Task 6: One-time legacy data migrator", "status": "pending", "blockedBy": [4, 5]}, {"id": 7, "subject": "Task 7: Replication registration + sync endpoint (default OFF)", "status": "pending", "blockedBy": [3]}, {"id": 8, "subject": "Task 8: Bearer auth interceptor for the sync endpoint", "status": "pending", "blockedBy": [7]}, {"id": 9, "subject": "Task 9: Surface ISyncStatus as site health signal", "status": "pending", "blockedBy": [7]}, - {"id": 10, "subject": "Task 10: Two-node in-process convergence test", "status": "pending", "blockedBy": [6, 8]}, + {"id": 10, "subject": "Task 10: Two-node in-process convergence test", "status": "pending", "blockedBy": [6, 8, 15]}, {"id": 11, "subject": "Task 11: Docker rig enablement (site-a pair)", "status": "pending", "blockedBy": [8]}, {"id": 12, "subject": "Task 12: Live gate on the docker rig", "status": "pending", "blockedBy": [9, 10, 11]}, {"id": 13, "subject": "Task 13: Documentation truth pass", "status": "pending", "blockedBy": [12]},