diff --git a/docs/plans/2026-07-20-localdb-adoption-phase2.md.tasks.json b/docs/plans/2026-07-20-localdb-adoption-phase2.md.tasks.json index 7cbfdc9a..2f31a474 100644 --- a/docs/plans/2026-07-20-localdb-adoption-phase2.md.tasks.json +++ b/docs/plans/2026-07-20-localdb-adoption-phase2.md.tasks.json @@ -1,15 +1,78 @@ { "planPath": "docs/plans/2026-07-20-localdb-adoption-phase2.md", "tasks": [ - { "id": 0, "subject": "Task 0: Recon — sink schema, seam, drain lifecycle, role-view bridge (STOP conditions)", "status": "pending" }, - { "id": 1, "subject": "Task 1: alarm_sf_events schema + registration (+ exact-set pin update)", "status": "pending", "blockedBy": [0] }, - { "id": 2, "subject": "Task 2: Rewire sink onto ILocalDb + delete bespoke file management (cutover 1/2)", "status": "pending", "blockedBy": [1] }, - { "id": 3, "subject": "Task 3: Primary-gated drain via PrimaryGatePolicy (cutover 2/2 — may co-commit with Task 2)", "status": "pending", "blockedBy": [2] }, - { "id": 4, "subject": "Task 4: One-time alarm-historian.db legacy migrator", "status": "pending", "blockedBy": [3] }, - { "id": 5, "subject": "Task 5: Convergence + failover scenarios in the pair harness (+ positive control)", "status": "pending", "blockedBy": [3] }, - { "id": 6, "subject": "Task 6: Rig config + docs", "status": "pending", "blockedBy": [3] }, - { "id": 7, "subject": "Task 7: DoD sweep (offline) — STOP and report after this", "status": "pending", "blockedBy": [4, 5, 6] }, - { "id": 8, "subject": "Task 8: Live gate on the docker-dev rig (needs explicit user go-ahead)", "status": "pending", "blockedBy": [7] } + { + "id": 0, + "subject": "Task 0: Recon \u2014 sink schema, seam, drain lifecycle, role-view bridge (STOP conditions)", + "status": "completed", + "note": "STOP condition does NOT fire (no BLOB; PayloadJson TEXT). Recon doc: docs/plans/2026-07-20-localdb-phase2-recon.md. Key finding: the drain worker is an internal Timer inside the sink (not a hosted service/actor), so the gate is a Func ctor param; and HistorianAdapterActor already primary-gates ENQUEUE with a different policy (ShouldHistorize) - which is why today's ungated drain is safe and why replication breaks it. Deviations D-1..D-4 recorded." + }, + { + "id": 1, + "subject": "Task 1: alarm_sf_events schema + registration (+ exact-set pin update)", + "status": "pending", + "blockedBy": [ + 0 + ] + }, + { + "id": 2, + "subject": "Task 2: Rewire sink onto ILocalDb + delete bespoke file management (cutover 1/2)", + "status": "pending", + "blockedBy": [ + 1 + ] + }, + { + "id": 3, + "subject": "Task 3: Primary-gated drain via PrimaryGatePolicy (cutover 2/2 \u2014 may co-commit with Task 2)", + "status": "pending", + "blockedBy": [ + 2 + ] + }, + { + "id": 4, + "subject": "Task 4: One-time alarm-historian.db legacy migrator", + "status": "pending", + "blockedBy": [ + 3 + ] + }, + { + "id": 5, + "subject": "Task 5: Convergence + failover scenarios in the pair harness (+ positive control)", + "status": "pending", + "blockedBy": [ + 3 + ] + }, + { + "id": 6, + "subject": "Task 6: Rig config + docs", + "status": "pending", + "blockedBy": [ + 3 + ] + }, + { + "id": 7, + "subject": "Task 7: DoD sweep (offline) \u2014 STOP and report after this", + "status": "pending", + "blockedBy": [ + 4, + 5, + 6 + ] + }, + { + "id": 8, + "subject": "Task 8: Live gate on the docker-dev rig (needs explicit user go-ahead)", + "status": "pending", + "blockedBy": [ + 7 + ] + } ], - "lastUpdated": "2026-07-20T00:00:00Z" + "lastUpdated": "2026-07-21T00:00:00Z" } diff --git a/docs/plans/2026-07-20-localdb-phase2-recon.md b/docs/plans/2026-07-20-localdb-phase2-recon.md new file mode 100644 index 00000000..97fad21b --- /dev/null +++ b/docs/plans/2026-07-20-localdb-phase2-recon.md @@ -0,0 +1,257 @@ +# LocalDb Phase 2 — Recon findings + +**Date:** 2026-07-21 · **Branch:** `feat/localdb-phase2` · **Base:** `master` `2e46d054` + +Answers Task 0 of `2026-07-20-localdb-adoption-phase2.md`. Every claim carries a `file:line` +citation against the base commit. Read the **Deviations** section last — two of them change the +shape of Tasks 2/3 from what the plan assumed. + +--- + +## 1. The sink's table schema + +**STOP condition does NOT fire** — confirmed by re-reading the executed DDL, as the plan's +pre-answer instructed. + +Executed DDL: `SqliteStoreAndForwardSink.cs:652-670` (`InitializeSchema`). It matches the class +doc-comment at `:17-26` exactly — no drift between documented and executed schema. + +```sql +CREATE TABLE IF NOT EXISTS Queue ( + RowId INTEGER PRIMARY KEY AUTOINCREMENT, + AlarmId TEXT NOT NULL, + EnqueuedUtc TEXT NOT NULL, + PayloadJson TEXT NOT NULL, + AttemptCount INTEGER NOT NULL DEFAULT 0, + LastAttemptUtc TEXT NULL, + LastError TEXT NULL, + DeadLettered INTEGER NOT NULL DEFAULT 0 +); +CREATE INDEX IF NOT EXISTS IX_Queue_Drain ON Queue (DeadLettered, RowId); +``` + +| Question | Answer | +|---|---| +| BLOB columns? | **None.** All 8 columns are TEXT/INTEGER. Payload is `PayloadJson TEXT NOT NULL`. No base64 conversion needed; the 171 KB chunk sizing is moot. | +| PK autoincrement? | **Yes** — `RowId INTEGER PRIMARY KEY AUTOINCREMENT`. The migrator must mint deterministic `mig-{node}-{legacyId}` ids and the new table needs a TEXT PK, as the plan specified. | +| Indices to carry | One: `IX_Queue_Drain (DeadLettered, RowId)` — the drain's covering index. `alarm_sf_events` wants the same shape over (status, insertion order). | +| Largest realistic payload | `PayloadJson` is a serialized `AlarmHistorianEvent` — 10 scalar fields (`AlarmHistorianEvent.cs`), no collections, no nesting. Low single-digit KB worst case; `Comment` is the only loosely-bounded field. | + +**Legacy column list** for the migrator's `pragma_table_info` intersection check: +`RowId, AlarmId, EnqueuedUtc, PayloadJson, AttemptCount, LastAttemptUtc, LastError, DeadLettered`. + +`RegisterReplicated` will accept this table: no BLOB, and the PK will be app-minted TEXT. + +## 2. The public seam + +`IAlarmHistorianSink` (`IAlarmHistorianSink.cs:23-34`) is **two members**: + +```csharp +Task EnqueueAsync(AlarmHistorianEvent evt, CancellationToken cancellationToken); +HistorianSinkStatus GetStatus(); +``` + +The concrete `SqliteStoreAndForwardSink` additionally exposes, and these are all consumed +somewhere, so the rewire must keep them: + +| Member | Cited | Consumer | +|---|---|---| +| `StartDrainLoop(TimeSpan)` | `:189` | `ServiceCollectionExtensions.cs:105` (DI factory) | +| `DrainOnceAsync(CancellationToken)` | `:320` | tests only — the deterministic drive | +| `RetryDeadLettered()` | `:511` | operator action (AdminUI) | +| `CurrentBackoff` | `:676` | tests | +| `DebugCapacityProbeCount` | `:99` | tests | +| `DefaultCapacity` / `DefaultMaxAttempts` consts | `:49`,`:53` | `AlarmHistorianOptions.cs:38,45` | +| `IDisposable` | `:679` | DI + tests; **disposes the injected writer too** | + +There is **no separate enqueue/dequeue/markDelivered/deadLetter interface** — the plan's item 2 +guessed at one. Dequeue, delivered-marking and dead-lettering are all *private* to the sink +(`ReadBatch :532`, `DeleteRow :564`, `DeadLetterRow :573`, `BumpAttempt :587`). That is good news: +the rewire is entirely internal to one class and the public seam is untouched. + +## 3. Semantics to preserve + +All defaults live in **two** places that must stay in agreement: + +| Semantic | Sink default | Options default | Enforced at | +|---|---|---|---| +| `Capacity` 1,000,000 | `:49` | `AlarmHistorianOptions.cs:38` | `EnforceCapacityFastPathAsync :271` → `EnforceCapacityAsync :602` | +| `MaxAttempts` 10 | `:53` | `:45` | drain loop `:429` | +| Dead-letter retention 30 d | `:50` | `:41` | `PurgeAgedDeadLetters :638`, run once per drain tick | +| `BatchSize` 100 | ctor arg `:116` | `:31` | `ReadBatch :542` | +| `DrainIntervalSeconds` 5 | — | `:34` | `StartDrainLoop` via `ServiceCollectionExtensions.cs:105` | + +Behaviours that are **not** in the plan's list but are load-bearing, and which the cutover must +not silently drop: + +- **Backoff ladder** 1s→2s→5s→15s→60s (`:57-64`), applied to the timer's *next due time* + (`RescheduleDrain :224`) so an outage genuinely slows the cadence. +- **Overflow evicts oldest non-dead-lettered rows** with a WARN and a lifetime + `EvictedCount` counter (`:630-635`) — the durability guarantee is explicitly bounded. +- **Corrupt-payload rows dead-letter immediately** for their own RowId so they cannot stall the + queue head (`:344-361`). +- **Cardinality guard**: a writer returning ≠1 outcome per event is treated as a batch retry + (`:393-404`). +- **In-memory row counter with periodic resync** every 10,000 enqueues (`:89-96`, `:271-290`) — + a perf optimisation that avoids `COUNT(*)` on the hot enqueue path. +- **Post-dispose throwing contract**: `GetStatus`/`RetryDeadLettered`/`EnqueueAsync`/`StartDrainLoop` + throw `ObjectDisposedException`; `DrainOnceAsync` returns quietly (`:322`). Pinned by a + regression test (`SqliteStoreAndForwardSinkTests.cs:854-865`). + +## 4. Drain-worker lifecycle — **the finding that reshapes Task 3** + +> The plan asked "hosted service or actor?". **Neither.** + +The drain worker is a **self-rescheduling one-shot `System.Threading.Timer` owned by the sink +itself** (`_drainTimer`, `:76`; started by `StartDrainLoop :189`; callback `DrainTimerCallback :199`; +re-armed by `RescheduleDrain :224`). It is started from inside the DI singleton factory at +`ServiceCollectionExtensions.cs:105`, so it begins ticking the moment the first consumer resolves +`IAlarmHistorianSink`. + +Consequences for the Primary gate: + +1. **The gate has to live inside `DrainOnceAsync`**, not in a wrapper. There is no external + scheduler to gate. +2. **`Core.AlarmHistorian` cannot see `PrimaryGatePolicy`.** The policy is in + `Runtime/Drivers/PrimaryGatePolicy.cs`, and Runtime → Core is the reference direction. + `Core.AlarmHistorian` references only `Core.Abstractions` (`.csproj`). So the sink takes a + **`Func` gate delegate** (default: always-drain) and Runtime supplies the lambda that + calls `PrimaryGatePolicy`. No new project reference, no new interface in Core. +3. **A role view is still needed** because the timer fires outside any actor. See the next point + for who should own it. + +### There is already a Primary gate — on the *enqueue* side, with a different policy + +`HistorianAdapterActor` (`Historian/HistorianAdapterActor.cs`) already subscribes to +`RedundancyStateChanged` (`:86`, handler `:145`), caches `_localRole` (`:42`), and gates the +enqueue on `ShouldHistorize()` (`:97`): + +```csharp +private bool ShouldHistorize() => _localRole is not (RedundancyRole.Secondary or RedundancyRole.Detached); +``` + +This is **not** `PrimaryGatePolicy` — it ignores driver-member count and defaults to *write* while +the role is unknown. Its comment (`:68-71`) explains why it exists: DPS fans the Primary's single +`alerts` publish to **every** node's adapter, so without the gate both nodes of a pair would +double-write. + +**This is why today's ungated drain is safe, and why Phase 2 breaks that.** Today the Secondary +never enqueues, so its queue is empty and an ungated drain has nothing to send. Once +`alarm_sf_events` replicates, the Primary's rows land in the Secondary's table — and an ungated +drain there would double-deliver *every* event, continuously, not just at failover. **The drain +gate is not a nicety; it is required by the same commit that registers the table.** + +### Who owns the role view + +`HistorianAdapterActor`, not `DriverHostActor`. Both are spawned under +`WithOtOpcUaRuntimeActors` (`ServiceCollectionExtensions.cs:353`) which `Program.cs:133` calls only +under `hasDriver` — as are `AddAlarmHistorian` (`:175`) and `AddOtOpcUaLocalDb` (`:185`). So sink, +LocalDb and adapter are co-located by construction. The adapter wins over `DriverHostActor` +because it **already** holds the role for exactly this purpose; using it adds no new subscription +and no new way for the view to go stale while the sink keeps draining. + +`DriverMemberCount` is read off Akka cluster state in `DriverHostActor.DriverMemberCount() :1446` +(try/catch → 0 on a non-cluster provider ⇒ single-node posture). Task 3 will lift that into a +small shared helper rather than copy it. + +## 5. Other `DatabasePath` construction sites + +- `ServiceCollectionExtensions.cs:98` — the only production site. +- `appsettings.json:57` — `"DatabasePath": "alarm-historian.db"`. +- `AlarmHistorianOptions.Validate() :54-55` — warns when the path is relative. **This warning + disappears with the key**; the validator loses one of its five checks. +- `SqliteStoreAndForwardSinkTests.cs` — ~30 construction sites, all passing the per-test temp + `_dbPath` seeded at `:25` and deleted in `Dispose :32`. These are the tests Task 2 rewires onto + a temp-file `ILocalDb`. +- **`docker-dev/docker-compose.yml` has no `AlarmHistorian` keys at all** — the rig runs with + `Enabled=false` today. Task 6 is therefore an *addition*, not an edit. + +No tooling or CLI constructs the sink. + +## 6. How `Enabled=false` short-circuits + +`AddAlarmHistorian` returns before registering anything when the section is absent or +`Enabled != true` (`ServiceCollectionExtensions.cs:87`), leaving the `NullAlarmHistorianSink` +seeded by `AddOtOpcUaRuntime :49`. The Null sink is a pure no-op (`IAlarmHistorianSink.cs:37-52`). + +Confirming the plan's own answer: **the tables are created unconditionally in `OnReady`** (cheap, +empty) because `AddOtOpcUaLocalDb` is independent of the `AlarmHistorian` gate. Only the +sink/drain stay Null. This is also required for correctness — `RegisterReplicated` must run on +both pair members regardless of their historian config, or a node that later enables the sink +would write rows that are never captured (see the CDC note below). + +## 7. Library constraints re-confirmed (README, `ZB.MOM.WW.LocalDb` 0.1.3) + +- **BLOB columns are rejected at registration.** Not a problem here (§1). +- **CDC semantics** — rows that existed *before* `RegisterReplicated` are neither captured nor + snapshotted. This is the mechanical reason the migrator must run **after** registration, and + the reason `LocalDbSetup.OnReady`'s ordering comment is load-bearing (`LocalDbSetup.cs:23-31`). +- **LWW keyed on the PK** — two nodes minting the same PK for *different* rows silently overwrite + each other; two nodes minting the same PK for the *same* row converge. Decision D-1 below turns + that second property into a feature. +- `ILocalDb` surface in use: `ExecuteAsync` / `QueryAsync(sql, rowMapper, params, ct)` / + `BeginTransactionAsync` / `CreateConnection` / `RegisterReplicated` + (`Deployment/LocalDbDeploymentArtifactCache.cs:77,82,205`). + +--- + +## Deviations and decisions + +Recorded here rather than raised as questions, per the plan's follow-up convention. + +### D-1 — Enqueue ids are a deterministic content hash, not `Guid.NewGuid()` + +**The plan says** GUIDs minted at enqueue (Task 2 step 2). + +**The problem.** The enqueue gate (`ShouldHistorize`, default-**write** on unknown role) and the +drain gate the plan specifies (`PrimaryGatePolicy`, default-**deny** on unknown role when paired) +disagree during the boot window. On a two-node pair before the first redundancy snapshot arrives, +**both** adapters enqueue the same DPS-fanned transition. With random GUIDs those are two distinct +rows that replicate into each other's tables — the buffer *duplicates* instead of converging, and +whichever node becomes Primary later delivers both copies. + +**The decision.** Mint `id` as a hash over the serialized payload. Two nodes independently +enqueuing the same fanned event produce the same PK, so LWW converges them to one row for free. +Two genuinely distinct events cannot collide: `AlarmHistorianEvent` carries a full-precision +`TimestampUtc` plus alarm id, kind, message and user, so an identical hash means an identical +event. + +This costs one `SHA256.HashData` per enqueue and removes a whole duplicate-row class. It also +slightly *improves* today's behaviour, where the same boot-window fan-out is already +double-delivered because the drain is ungated on both nodes. + +`ShouldHistorize` itself is left unchanged — out of scope, and with D-1 in place the two gates +disagreeing is no longer harmful. + +### D-2 — Gate denial must be observable + +`PrimaryGatePolicy` defaults to **deny** when the role is unknown on a multi-driver cluster. That +is the correct safety posture, but it means a redundancy-snapshot identity mismatch — the +documented 03/S5 shape that `DriverHostActor.cs:1485-1491` already warns about once — would +silently stop alarm history on *both* nodes, degrading only as retry/eviction growth minutes later. + +Task 3 will therefore: log the skip once per gate-state transition (not per tick — the drain runs +every 5 s), and surface it in `HistorianSinkStatus` so `/healthz` and the AdminUI can see +"buffering, not draining, because not Primary" rather than an unexplained rising queue depth. +Adding a `HistorianDrainState` member is additive; Task 3 will check every `switch` over it. + +### D-3 — Tasks 2 and 3 will land as one commit + +The plan anticipated this. Confirmed necessary: the gate delegate is a constructor parameter of +the rewritten sink, so the sink cannot compile in its final shape without it, and an intermediate +commit with a rewired-but-ungated sink would be a commit in which a replicated table is drained by +both nodes. Not a state worth having in history. + +### D-4 — The live gate needs `MaxAttempts` raised on the rig + +Not actionable until Task 8, recorded now so it is not rediscovered there. The rig has no +HistorianGateway, so every drain attempt fails. With `MaxAttempts: 10` and the backoff ladder +capping at 60 s, buffered rows dead-letter roughly five minutes into the outage — which would make +the gate's "dead letters 0" check (step 2) fail for reasons that have nothing to do with +replication. Task 6 should set a high `MaxAttempts` on the rig, or Task 8 should assert on +dead-letter *timing* instead. + +Related: `Program.cs:170-173` shows an `AlarmHistorian:Enabled=true` / +`ServerHistorian:Enabled=false` deployment is explicitly supported and warned about — which is +exactly the rig's shape. Task 6 must confirm `ServerHistorianOptionsValidator` does not fail host +start for a disabled section with an empty `Endpoint`.