docs(localdb): amend Phase 1 plan - split Task 5 (event-log id change)
Execution review against the code found the integer site_events.id is
load-bearing in three places the original Task 5 did not list:
- keyset cursor EventLogQueryService.cs:70,141,153 (id > $afterId)
- storage-cap purge EventLogPurgeService.cs:164 (ORDER BY id ASC)
- wire contracts EventLogEntry.Id, both ContinuationTokens (long)
cross the site<->central Akka boundary
A GUID PK against the existing cursor silently drops rows, and against the
purge deletes arbitrary rows instead of the oldest. Task 5 is therefore split
into 5a (writer, standard) and 5b (read path + DTOs, high-risk), with 5b's
full file set spelled out and Task 10 gated on it.
Also recorded: Task 4's real registration site, and that both legacy DB paths
are CWD-relative and unset in docker today (so they live outside the data
volume and are already lost on container recreate - Phase 1 fixes that).
Claude-Session: https://claude.ai/code/session_01BL2Vu1ESDQ9SCN4gVKkdts
This commit is contained in:
Joseph Doherty
@@ -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`
|
||||
|
||||
Reference in New Issue
Block a user