Files
lmxopcua/docs/plans/2026-07-20-localdb-phase2-recon.md
T
Joseph Doherty 2e4ccf7fe9 chore(localdb): phase-2 DoD sweep
Build: 0 errors solution-wide, and 0 warnings from every project this branch
touches. The ~816 solution-wide warnings are pre-existing xUnit1051 /
OTOPCUA0001 / CS86xx in untouched driver + client test projects.

Tests: full solution run compared against a full run on a detached worktree at
the pre-branch baseline 2e46d054. The two failure SETS are identical -- all 13
tests, same names, zero new failures. Net +26 tests: +3 Core.AlarmHistorian
(drain gate), +6 Runtime (role view), +17 Host.IntegrationTests (migrator +
convergence). Set comparison rather than counts, because the suite carries
standing environment- and load-dependent failures a count would hide.

The greps found real drift Task 6 missed -- eight live sites still naming the
deleted SqliteStoreAndForwardSink, including the AdminUI /alarms/historian
panel text, which is user-visible, and a <see cref> in HistorianAdapterActor
that resolved to nothing without warning. All repointed at
LocalDbStoreAndForwardSink; CLAUDE.md's alarm-history paragraph now also
records the LocalDb buffer and the primary-gated drain, and drops DatabasePath
from the knob list. docs/AlarmTracking.md still promised an
AlarmHistorianOptions.Validate() startup warning for a relative DatabasePath
and an empty SharedSecret; both branches are gone, so it now says so.

Code references to AlarmHistorian:DatabasePath reduce to exactly two intentional
ones: AlarmSfLegacyMigrator.LegacyPathKey and its test. No `new SqliteConnection`
remains anywhere in Core.AlarmHistorian.

Recon doc gains the durable verification record: guard-deletion evidence for
both vacuous passes, the two exact-set replicated-table pins (both assert set
equality, so an added or a dropped registration fails), and the baseline test
comparison with a per-failure account of why each of the 13 is not this
branch's.

Stops here per the plan. Task 8's live gate needs explicit go-ahead; nothing on
this branch is to be merged.

Claude-Session: https://claude.ai/code/session_01GASWkNEi68FSCtvr6rLoEW
2026-07-21 05:03:57 -04:00

21 KiB
Raw Blame History

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.

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? YesRowId 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:

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 :271EnforceCapacityAsync :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):

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.


Guard-deletion evidence (recorded at the Task 7 DoD sweep)

Every guard this phase adds was proved by deleting it and watching the tests go red. Two of those runs found tests that would have passed over a broken implementation. Both are recorded here because the shape of the vacuity is reusable, not just the fix.

Control 1 — remove IRedundancyRoleView.Publish from DriverHostActor

3 of 6 DriverHostActorRoleViewTests went red. The other three assert ShouldServiceAsPrimary == true, which is exactly the value the view is seeded with — so they cannot distinguish "published true" from "never published at all". That split is expected and acceptable given the three that do go red pin the false cases, which only a real publish can produce. Worth knowing before someone reads a future 3/6 result as a regression.

Control 2 — remove RegisterReplicated("alarm_sf_events") from LocalDbSetup.OnReady

3 of 4 AlarmSfConvergenceTests went red immediately. The fourth, TheSameEventOnBothNodes_ConvergesToOneRow, passed vacuously: with replication switched off each node trivially held its own single row, and "count == 1 on both nodes" was satisfied by two completely unconverged databases. The assertion could not tell convergence from isolation.

Fix: enqueue a second, distinct event on node B and require both nodes to hold two rows. Isolation now yields 1 and 2; only convergence yields 2 and 2. Re-run under the control: red. Control restored: all 4 green.

The general trap — an assertion whose expected value is also what a disabled system produces — is the same one recorded in the #485/#486 unreadable-artifact defect class.

Exact-set replicated-table pins

Two, not one, and both assert set equality (ordered ShouldBe against the literal three-table list, so an added or dropped registration fails):

  • LocalDbSetupTests — drives the production OnReady callback directly.
  • LocalDbWiringTests — resolves ILocalDb from the full driver DI graph, which is what catches a registration that never reaches a running host.

Test-suite evidence (Task 7 DoD sweep)

Full solution run on the branch, compared against a full solution run on a detached worktree at the pre-branch baseline 2e46d054. Comparing failure sets, not counts, because the suite has a standing set of environment- and load-dependent failures that a raw count would hide.

The two failure sets are identical — all 13 tests, same names, both runs. Zero new failures.

Assembly Baseline Branch Delta
Core.AlarmHistorian.Tests 0 failed / 29 passed 0 failed / 32 passed +3 (the drain-gate tests; 29 ported from the old sink's suite)
Runtime.Tests 1 failed / 418 passed 1 failed / 424 passed +6 (DriverHostActorRoleViewTests)
Host.IntegrationTests 2 failed / 169 passed 2 failed / 186 passed +17 (migrator + convergence + pin updates)

The 13 standing failures, and why each is not this branch's:

  • 3 × DriverTypeNamesGuardTestsArgumentNullException (secretResolver) out of GalaxyDriverFactoryExtensions.Register, reached by reflection. A real pre-existing defect in a guard test, reproduced exactly on the baseline worktree. Unrelated to this phase.
  • 4 × AbLegacy.IntegrationTests, 3 × OpcUaClient.IntegrationTests, 1 × DriverProbeHandshakeE2eTests.AbCip_Green_AgainstSim — driver fixtures on the 10.100.0.35 docker host are not running.
  • 1 × RoslynVirtualTagEvaluatorTests.Evaluate_racing_ClearCompiledScripts_never_fails_with_disposed — times out under full-suite load; passes in isolation.
  • 1 × ContinuousHistorizationRecorderTests.Retry_after_writer_failure_eventually_acks — fails under full-suite load ("writer must have been called at least twice"), and Runtime.Tests passes 425/425 when run alone. Also fails on the baseline. Worth noting for anyone reading a future run: this one is newly observed here only because the previous sweep's log was truncated, not because the branch introduced it.