Second slice of the 2026-09-09 whole-system review remediation
(docs/plans/2026-09-09-arch-review.md §10), on top of the High slice 146a1323.
Runtime/devices: SR-2 alarm on-trigger integration scope; SR-3 correlated NativeAlarmActor
subscribe retry; SR-4 immutable audited DB target; SR-5 ordered InstanceActor stop/flush;
SR-12 rehydration fence; SR-13 S&F gate recheck per delivery; DC-4 no OPC UA security
downgrade; DC-5 alarm snapshot for every new subscriber; ORCH-1 sync-dispose joins the pump.
Management/UI: MG-2 site scope on audit tree + Flow Debugger Replay; MG-3 stale-probe memo
removed; MG-4 server-owned bulk-deploy deadline; UI-5 sandbox runs in a killable child
process (new CentralUI.SandboxWorker project); UI-14 bounded sandbox output/IPC.
Edge/audit: EI-1 relay audit ownership; EI-2 SkipBodyCapture suppresses error text; EI-3/EI-11
error-field redaction + caps; EI-4 single relay deadline, pre-execution-only retry; EI-5
authenticated relay provenance; EI-10 body cap before audit read.
Persistence/contracts: DL-1 global Instance.UniqueName unique index + conflict translation;
DL-2 DataConnection protocol config encrypted at rest (migration
20260909203046_EnforceGlobalInstanceNamesAndProtectProtocolConfiguration — coordinated
central upgrade, old binaries cannot read protected rows); DL-3 bounded Replay paging;
DL-6 cross-site Overwrite of a deployed instance rejected; XC-4 proto descriptor contract test.
Config/hygiene: XC-1 central listener isolation by local port; XC-2/XC-3 inert SMTP + cron
keys removed from rig settings; CM-2 env2 bootstrap guard enabled; DC-6 deploy site exit code;
DC-7 notifier retries only explicit standby rejection; XC-5 ClusterClient purged from docs.
Gates recorded in the report: build 0/0, 9 397 passed / 0 failed, rig redeployed on all
eight nodes with the migration applied. 46 Low findings remain open.
P8-8: replace the remaining wall-clock sleep-then-assert instances outside
Communication.Tests' Medium-wave sweep with the same row-40 house pattern —
continuous polling (AssertHoldsFor) for ABSENCE assertions instead of a single
end-of-window sample, which cannot distinguish "stayed quiet throughout" from
"hadn't happened yet when the one sample landed" and cannot catch a value that
flips wrong and back before that sample.
- DataConnectionActorBatchTests: QualityCounters_ArePushedOnTransitionOnly_AndCoalesced
(:206/:214 class) converted to AssertHoldsFor; ResolutionProbes_BackOff...
(:181/:184, a genuine real-time backoff-RATE assertion, not a sync-primitive
stand-in) converted to AssertRoundsWithinWindow, which enforces the ceiling
continuously (fails fast on a no-backoff regression) while the floor still
necessarily waits out the window.
- SiteAlarmLiveCacheServiceTests (:452-454, :470-472): converted to the
project's own AssertHoldsFor (mirrors SiteAlarmAggregatorActorTests', same
assembly).
- StoreAndForwardServiceTests (:891-892): async AssertHoldsForAsync variant
(the class runs on the async test thread).
- KpiHistoryRecorderActorTests (:1043-1049): two skip-path FoldCount==0
absence checks converted.
Each helper is a private per-file copy, matching the Medium-wave T17
precedent (SiteAlarmAggregatorActorTests.AssertHoldsFor) rather than a shared
TestSupport utility. Proof of the mechanism: a standalone (non-repo) harness
run 100 trials of a real background thread flipping a boolean true then back
to false inside a 300ms window — Thread.Sleep(300)+one sample missed the
violation 100/100 times (false pass), continuous 20ms polling caught it
100/100 times. Most of the actual conversions here guard monotonic counters
(their single-sample variant would eventually see the final value too), so
each converted test carries an inline honesty note on what specifically the
conversion buys for that assertion (fail-fast vs. true flip-back detection).
P8-9: ScadaBridgeTelemetryTests.EmitHelpers_DoNotThrow called
SetQueueDepthProvider(() => 5) against the process-global row-32 provider
slot with no clear and no [Collection] — a landmine for any future
same-process reader of scadabridge.store_and_forward.queue.depth. Fixed with
compare-and-clear in a finally via the existing (production, untouched)
ClearQueueDepthProvider. Searched this assembly and the whole test tree for a
second same-process reader that asserts on the value: none exists today
(MetricsEndpointTests scrapes /metrics but only asserts on the response
shape, and the only value-asserting test, QueueDepthGaugeTests, lives in
StoreAndForward.Tests — a separate test-host process the leak cannot reach),
so no [Collection] was added; the test's doc comment records the search and
flags it for re-check if that changes.
Test-only change, zero production edits. Full suites green: DCL.Tests
342/342, Communication.Tests 729/729, StoreAndForward.Tests 135/135,
KpiHistory.Tests 54/54, Host.Tests 522/522. Each converted test additionally
looped 20x in isolation with zero flakes.
QueueDepthGaugeTests failed intermittently whenever a sibling test class started a
StoreAndForwardService concurrently. `scadabridge.store_and_forward.queue.depth` is an
ObservableGauge on ScadaBridgeTelemetry's static meter reading ONE process-global
provider slot; every StartAsync writes it, last registrant wins. A concurrent StartAsync
clobbered the gauge mid-assertion and the test read the sibling's depth (observed:
expected 0, actual 6 — 2 runs in 5 with parallel collections enabled).
The global slot cannot be swapped for a per-test meter: three of the six facts assert
the slot-ownership semantics themselves (compare-and-clear identity, stop/restart
re-seed), so the slot IS their subject. The correct isolation is mutual exclusion —
QueueDepthGaugeTests, StoreAndForwardServiceTests and CachedCallAttemptEmissionTests
(the three classes that call StartAsync) now share the QueueDepthGaugeCollection, which
xUnit runs serially; the other eight classes stay parallel.
With real isolation in place the assembly-wide `parallelizeTestCollections: false`
blanket switch is no longer needed and is flipped back on, so the isolation is actually
exercised rather than papered over.
QueueDepthGaugeCollectionGuardTests is a staleness self-test: it scans this project's
sources (anchored by [CallerFilePath], not the output dir) for files that both construct
and start a service, and fails if one omits the collection attribute — so a new test
class cannot silently reintroduce the flake. Negative-controlled: dropping the attribute
from StoreAndForwardServiceTests makes the guard fail with the offending file name, and
the original gauge flake reappears in the same run.
Verification: 8 consecutive full-suite runs green with parallel collections ON (135
tests), plus 5 more after the negative control was reverted. Test-only change; no src/
involvement.
F1: deploy/wonder-app-vd03/appsettings.Site.json (outside git, WP1.2's
StartupValidator gate applies live on next install/upgrade) was missing the
now-required AuditLog:SiteWriter:DatabasePath, added pointing at
E:\ApiInstall\ScadaBridge\site\data\auditlog.db alongside the file's
existing SiteEventLog/LocalDb paths; scanned deploy/ for other Site-role
appsettings with the same gap (none) and confirmed wonder does not pin
LocalDb:Replication:MaxBatchSize (F2 doesn't apply there).
F2: re-pin an explicit LocalDb:Replication:MaxBatchSize=64 on docker/site-a
node-a and node-b. MaxBatchBytes (2 MB default) only bounds the wire
message via the per-message split in SyncSession.PumpLoopAsync;
MaxBatchSize separately bounds the DB read page in
OplogStore.ReadBatchAboveAsync/SnapshotStreamer, which materializes the
whole page into memory before that split runs. Left at the 500 default, a
reconnect drain of worst-case config_json rows could transiently allocate
~35 MB per read even though every wire message stayed within budget.
Updated the CLAUDE.md LocalDb bullet to stop implying the row cap is fully
redundant with the byte budget (topology-guide.md has no matching claim).
F3: StoreAndForwardService's observer-queue onDropped callback logged a
Warning per dropped item, flooding logs at sweep rate for a stuck observer
with a large queue. LogObserverQueueDrop now logs once immediately on the
first drop of an episode, then throttles to at most one rollup Warning per
minute while drops continue, reporting the count dropped since the last
log; the cumulative ObserverQueueDroppedCount counter is unaffected.
Extended StoreAndForwardServiceTests with
ObserverQueue_ManyDropsInOneEpisode_LogsExactlyOneWarning, which floods the
bounded queue and pins exactly one drop-related Warning log for the
episode via a small CapturingLogger test double.
dotnet build ZB.MOM.WW.ScadaBridge.slnx: 0 warnings, 0 errors.
dotnet test StoreAndForward.Tests: 134/134 passed.
dotnet test Host.Tests: 490/490 passed.
WP2.6 (arch-review remediation, cross-cutting misc):
- SiteExternalSystemRepository: name/ID-indexed ExternalSystemDefinitionCache replaces
the fetch-all + reverse-map scan on every by-ID/method lookup; loaded once per
redeploy, invalidated by DeploymentManagerActor after HandleDeployArtifacts applies
external-system changes. Static JsonSerializerOptions for method-list parsing.
- Inbound API: short-TTL ApiMethodCache fronts the per-request ApiMethod repository
fetch; invalidated by name via the existing ScriptArtifactChangeSubscriber/
IScriptArtifactChangeBus pipeline, self-healing via TTL for changes the bus
doesn't cover (e.g. Management API edits).
- StoreAndForward: the cached-call audit-observer queue — the one unbounded channel
left in the system — is now bounded (ObserverQueueCapacity, default 10,000) with
DropOldest overflow and a dropped-notification counter.
- SiteStreamManager: alarm state changes now travel a dedicated publish
source/broadcast hub, isolated from the (far higher-volume) attribute path, so an
attribute storm can no longer evict a pending alarm transition; the alarm hand-off
queue is bounded with a drop counter surfaced on the site health report
(SiteStreamAlarmDropCount via the new SiteStreamAlarmDropReporter), and publishing
is skipped entirely at zero subscribers on either path.
- CLI ManagementHttpClient: explicit 30s HttpClient.Timeout on the shared
construction (was the 100s framework default), overridable via
SCADABRIDGE_HTTP_TIMEOUT_SECONDS.
Deviation: the failback-probe heartbeat item is NOT included — its only viable
surface (CentralChannelProvider.cs / heartbeat consumers) lives entirely in the
Communication project, explicitly off-limits to this work package this phase.
Tests: SiteRuntime.Tests (550), InboundAPI.Tests (278), StoreAndForward.Tests (133),
CLI.Tests (390), HealthMonitoring.Tests (97) — all green after full solution build.
Tasks 5 and 6 of the Phase 2 plan, committed together because their test
fallout is entangled — several fixtures construct both stores.
StoreAndForwardStorage and SiteStorageService now take ILocalDb. Connections
come from ILocalDb.CreateConnection(), which hands out an already-open,
pragma-configured connection carrying the zb_hlc_next() UDF the capture triggers
call; a raw connection would lack the UDF and every write to a replicated table
would fail closed. Deleted with the connection strings: S&F's
EnsureDatabaseDirectoryExists and its per-open busy_timeout pragma, and the site
service's BusyTimeoutFloorSeconds normalization — LocalDb owns all of it now.
DI: AddSiteRuntime's string overload is gone (nothing left to supply), so the
Host calls the no-arg form. ScadaBridge:Database:SiteDbPath and
StoreAndForwardOptions.SqliteDbPath survive only as the migrator's source
locations in Tasks 8/9.
Two things the plan did not anticipate, both worth reading:
1. FOUND A REAL LATENT DEFECT, from Phase 1, now fixed. The plan assumed
directory creation simply moved to LocalDb along with file ownership. It did
not: the LocalDb library never creates the parent directory, and
SqliteLocalDb opens the file eagerly in its constructor — so a missing
directory is a hard boot failure ("SQLite Error 14: unable to open database
file"), not a degraded start. The default site config points at the RELATIVE
path ./data/site-localdb.db, so any site node without a pre-existing data/
directory fails to boot. The docker rig escapes only because its volume mount
happens to create /app/data — a coincidence that would have hidden this until
a bare-metal or fresh deployment. This has been latent since Phase 1 made
LocalDb:Path required; deleting S&F's EnsureDatabaseDirectoryExists here
would have widened it. Re-established the guarantee at the layer that now
owns the path (SiteLocalDbDirectory.Ensure, called before AddZbLocalDb) and
pinned it with SiteLocalDbDirectoryTests. Non-vacuity is not assumed: two
tests written against the wrong assumption failed with exactly this
SQLite Error 14 before the fix existed.
2. Test fallout was ~7x the plan's estimate. The plan named "fixtures" in one
project; the constructor change actually reaches 40 files across 7 test
projects, and most used Mode=Memory;Cache=Shared — which LocalDb has no
equivalent for, so every one had to move to a real temp file. Rather than
copy the Phase 1 TestLocalDb fixture into 7 projects, added a shared
tests/ZB.MOM.WW.ScadaBridge.TestSupport library (not a test project) so the
WAL-sidecar cleanup and the "real, not stubbed" rationale live in one place.
Retargeted rather than deleted, in both directions: the S&F WAL test now asserts
against the LocalDb-backed store (WAL genuinely is LocalDb's job), while the
directory-creation test moved to Host.Tests (that guarantee is NOT LocalDb's).
SiteStorageServiceTests.Initialize_EnablesWalJournalMode got the same treatment.
DeploymentManagerMediumFindingsTests induced a persistence failure via an
unopenable path, which no longer reaches the assertion since the fixture now
throws first; it induces the same failure shape via an uninitialized store.
Verified: full solution build 0 warnings; SiteRuntime 532, Host 318,
AuditLog 355, ExternalSystemGateway 142, HealthMonitoring 97,
StoreAndForward 153 — 1597 passed, 0 failed.
Claude-Session: https://claude.ai/code/session_01BL2Vu1ESDQ9SCN4gVKkdts
Joseph Doherty