4480a7d755
D-6 (payload-hash migrator ids over mig-{node}-{legacyId}) and D-7 (the
plan's Host.Tests project does not exist) were captured only in commit
messages and the tasks file. They belong with the other deviations, where
the next reader looks.
Claude-Session: https://claude.ai/code/session_01GASWkNEi68FSCtvr6rLoEW
303 lines
17 KiB
Markdown
303 lines
17 KiB
Markdown
# 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<bool>` 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-5 — The table keeps the legacy delete-on-ack shape, not a `status` column
|
|
|
|
**The plan proposes** `status TEXT NOT NULL DEFAULT 'pending'` with values `pending | delivered |
|
|
dead`, and drops the legacy `LastError` column.
|
|
|
|
**Two problems.** A `delivered` status means acknowledged rows accumulate in the table forever —
|
|
the plan specifies no sweeper for them, and they would also have to be excluded from the capacity
|
|
count, changing a semantic §3 says to preserve. And `LastError` is not dead weight: `DeadLetterRow`
|
|
writes the failure reason into it and `RetryDeadLettered` clears it, so it is the only operator-
|
|
facing record of *why* a row was dead-lettered.
|
|
|
|
**The decision.** Keep the legacy shape: DELETE on acknowledgement, `dead_lettered` as the same 0/1
|
|
flag, and retain `last_error`. Deleting keeps the table bounded with no second sweeper, and the
|
|
replication engine carries the delete as a tombstone (pruned after `TombstoneRetention`, 7 d), so
|
|
the peer drops its copy too — which is the property the plan wanted the `delivered` status for.
|
|
Columns then map 1:1 onto the legacy table, making the Task-4 migrator a straight copy.
|
|
|
|
One genuine change: the drain's `ORDER BY` moves from `RowId ASC` to `enqueued_at_utc, id`, because
|
|
a hashed TEXT id carries no insertion order. Round-trip ("O") timestamps sort lexicographically in
|
|
chronological order, and `id` makes the ordering total; the index covers both.
|
|
|
|
### D-6 — The migrator keys on the payload hash, not `mig-{node}-{legacyId}`
|
|
|
|
**The plan says** deterministic ids of the form `mig-{NodeName}-{legacyId}`, mirroring ScadaBridge.
|
|
|
|
**Why that is not enough here.** Node-prefixing does solve the collision the legacy AUTOINCREMENT
|
|
key would otherwise cause — node A's `RowId 7` and node B's `RowId 7` are different alarms — but it
|
|
*preserves* a duplication that ought to be collapsed. A warm pair's two legacy files **overlap**:
|
|
`HistorianAdapterActor` default-writes while its redundancy role is unknown, so both nodes accepted
|
|
the same fanned transitions during every boot window they ever had. Prefixed ids would carry every
|
|
one of those duplicates into the merged buffer permanently, for the eventual Primary to deliver
|
|
twice.
|
|
|
|
**The decision.** Reuse D-1's `AlarmSfSchema.DeriveId` (lifted out of the sink so both call sites
|
|
share one identity rule). Distinct events cannot collide; identical ones converge. It also removes
|
|
the migrator's only need for node identity from configuration, and makes a crash between commit and
|
|
rename harmless under `INSERT OR IGNORE` — which the plan's own idempotency requirement wanted
|
|
anyway.
|
|
|
|
### D-7 — The migrator's tests live in `Host.IntegrationTests`
|
|
|
|
The plan names `tests/Server/ZB.MOM.WW.OtOpcUa.Host.Tests`. **That project does not exist.** The
|
|
tests sit beside `LocalDbSetupTests` in `Host.IntegrationTests`, which is where every other
|
|
LocalDb-facing test already lives. They are offline — a temp file and no external services.
|
|
|
|
### 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`.
|