ScadaBridge/archreview/00-OVERALL.md

ScadaBridge Deep Architecture Review — Overall Report (Round 2)

Date: 2026-07-12 (round 2; round 1 dated 2026-07-08) Scope: Full system — all 27 components, docker/deploy topology, tests, and design docs, at HEAD 8c888f13 Method: 8 parallel domain re-reviews. Each re-read its round-1 report and fix plan (PLAN-01…08, all complete — 191/192 tasks merged by 2026-07-10), then verified every round-1 finding's disposition against current source (trusting code, not plan claims), then swept the ~268 commits since the pre-initiative baseline b910f5eb — the fix-plan code itself plus the two post-initiative features nobody had reviewed (aggregated live alarm stream, KPI hourly rollups) — for new findings. Dimensions: stability, performance, conventions, underdeveloped areas; all findings carry file:line citations in the domain reports.

Report Index

#	Report	Domain	Round-2 verdict (one line)
01	Cluster, Host & Failover	ClusterInfrastructure, Host, HealthMonitoring, docker/Traefik/deploy	Round 1's Critical (SBR never enabled) fixed and behaviorally proven; active-node model coherently unified — but the never-run failover drill kills the one node keep-oldest can't survive, and seed-order blocks the recovery loop when central-b is the survivor.
02	Communication & Store-and-Forward	ClusterClient, gRPC streaming, S&F engine, live alarm stream	Round 1's Critical and all three Highs genuinely fixed and test-backed; the new live alarm stream is solid — but the new anti-entropy peer resync was built on the wrong active-node predicate and an undeliverable-size Akka message: 1 new Critical + 1 new High, both confined to that feature.
03	Site Runtime & DCL	SiteRuntime, DataConnectionLayer, SiteEventLogging	All three round-1 risk concentrations (adapter reconnect, script containment, deployment honesty) genuinely fixed — 26/28 verified; one Medium residual on the MxGateway reconnect fault path.
04	Data & Audit Backbone	ConfigurationDatabase, AuditLog, NotificationOutbox, SiteCallAudit, KpiHistory	All round-1 time bombs genuinely defused and verified; the one new material risk is the brand-new KPI rollup backfill, which re-introduces at startup the exact unbounded year-scale single-pass shape the plan eliminated everywhere else.
05	Templates, Deployment & Transport	TemplateEngine, DeploymentManager, Transport, ScriptAnalysis	All 26 actionable round-1 findings — including the Critical inheritance-edge loss and the whole silent-data-loss family — fixed in code with zero regressions; residue is 1 Medium perf leftover and 5 Lows.
06	Edge Integrations	InboundAPI, ExternalSystemGateway, NotificationService, DelmiaNotifier	All round-1 risk genuinely fixed and source-verified (20/25 fixed, 0 regressions); new residual risk is 1 Medium + 4 Low — the Medium being the script-artifact invalidation bus publishing to zero subscribers.
07	UI, Management & Security	CentralUI, ManagementService, CLI, Security	Both round-1 Criticals dead, 17/24 fully fixed, 0 regressions; remaining new findings are Medium-and-below — mostly security seams the fixes themselves exposed (proxy-blind throttle keying, unthrottled hub bind, unscoped secured writes).
08	Conventions, Tests & Underdeveloped	Cross-cutting	Every actionable round-1 finding genuinely fixed and conventions held through 268 commits of churn; new residue is small and mostly hygiene — except a live production API key sitting in an untracked working-tree file.

Round-1 Finding Disposition (verified against source, not plan claims)

Report	Tracked	Fixed (verified)	Partial	Deferred/accepted	Not fixed	Regressed
01 Cluster/Host/Failover	28	18	3	6	1¹	0
02 Communication/S&F	26	22	2	2	0	0
03 Site Runtime/DCL	28	26	0	2	0	0
04 Data/Audit Backbone	26	19	0	7²	0	0
05 Templates/Deploy/Transport	32	26	1	5	0	0
06 Edge Integrations	25	20	2	3	0	0
07 UI/Management/Security	24	17	6	1³	0	0
08 Conventions/Tests	30	20	3	7	0	0
Total	~219	168	17	~33	1	0

¹ U5 (NodeName in the wonder-app-vd03 overlay) — deferred out-of-band with an owner, but see new finding 01-N2. ² Includes 1 won't-fix (S10). ³ UA7 was a false premise (No longer applicable).

Headline: zero regressions, zero false "fixed" claims found. Every disposition was re-derived from current source with file:line evidence. Several fixes exceeded their tickets (content-based inbound handler invalidation, two-signal deploy join, trigger-body trust coverage); PLAN-05's reflection round-trip guard structurally forecloses the entire DTO-drift class that caused round 1's Transport Critical.

New-Findings Severity Tally (round 2 only)

Report	Critical	High	Medium	Low
01 Cluster/Host/Failover	0	1	1	4
02 Communication/S&F	1	1	2	5
03 Site Runtime/DCL	0	0	1	5
04 Data/Audit Backbone	0	1	3	3
05 Templates/Deploy/Transport	0	0	1	5
06 Edge Integrations	0	0	1	4
07 UI/Management/Security	0	0	4	5
08 Conventions/Tests	0	1	2	5
Total	1	4	15	36

Round 1 closed with 7 Critical / 50 High / 85 Medium / 68 Low. Round 2 opens with 1 / 4 / 15 / 36 — an ~80% reduction at every severity, and the residue is concentrated almost entirely in code written after round 1, not in anything round 1 flagged.

Overall Verdict

The arch-review initiative worked: the fix plans delivered what they claimed, verified in source, with zero regressions. The system's round-1 risk profile — the failover crack, the Transport silent data loss, the data-layer time bombs, the cache-coherence family, the security projection leaks — is genuinely closed.

The round-2 risk profile is different in kind: the new findings live almost exclusively in the newest code — the anti-entropy resync (built by PLAN-02 itself), the KPI rollup backfill and the live alarm stream (both shipped post-initiative, 2026-07-10), and operational seams the fixes exposed. This is the expected shape for a healthy hardening cycle: the reviewed-and-fixed code held; the code that had never been reviewed is where the defects are.

Bottom line: the codebase graduated from "production-grade engineering, not production-proven HA" to "one new feature (peer resync) must not ship as-is, one secret must be rotated today, and the failover drill still needs to actually be run."

The Findings That Matter Most (fix before anything else)

C1 (new). The S&F anti-entropy resync can wipe the delivering node's live buffer. The delivery gate uses oldest-Up (SelfIsPrimary) but the resync authority check uses leader+Up (lowest address). After a routine rolling restart of the lower-address site node the two predicates diverge persistently: the delivering node requests a resync from the stale rejoined leader and ReplaceAllAsync-wipes its own live buffer — silently losing every notification/cached call buffered during the outage and resurrecting delivered rows as duplicates. (Report 02 N1; SiteReplicationActor.cs:190-198 vs AkkaHostedService.cs:881)

H1 (new). The resync snapshot is undeliverable at exactly the scale it exists for. Up to 10,000 full rows travel as ONE Akka remoting message with no maximum-frame-size override (default 128 KB) — silently dropped for any realistic backlog. (Report 02 N2; SiteReplicationActor.cs:40,407-410,460)

H2 (new). A live production Inbound API key sits in untracked test.txt at the repo root (sbk_eb5acc… for wonder-app-vd03.zmr.zimmer.com:8085) — one git add -A from history. Rotate the key and delete the file today. (Report 08 NF1; test.txt:3)

H3 (new). The failover drill codifies a recovery the topology cannot deliver. failover-drill.sh kills the ACTIVE(=oldest) central node — the one crash keep-oldest cannot survive (registered deferred decision) — and the restart recovery loop cannot converge when central-b is the survivor because both nodes list central-a first in SeedNodes (Akka's first-seed rule blocks a non-first seed from self-bootstrapping). The README admits the drill was never run live yet promises ~25s failover; the ~25s envelope also remains the last unmeasured headline design claim (the perf harness's skip precondition — the two-node rig — landed 2026-07-08). (Reports 01 N1, 08 NF2)

H4 (new). The KPI rollup backfill re-introduces the unbounded single-pass shape at startup. The one-shot backfill folds the full 90-day retention window in one tracked ToListAsync (potentially tens of millions of rows) + one giant SaveChanges, re-runs on every singleton failover, and blocks periodic folds while it runs. (Report 04 R1; KpiHistoryRecorderActor.cs:510-511, KpiHistoryRepository.cs:104-167)

Cross-Cutting Themes (round 2)

Theme 1 — The oldest-vs-leader predicate split is still alive, and it just caused the new Critical

PLAN-01 unified "active node" on oldest-member semantics, but the helper was never swapped into SiteCommunicationActor/SiteReplicationActor (a known partial), and the new resync feature then keyed its authority check on the leader predicate — producing C1. One shared IsActiveNode predicate, used by every gate, would close this class permanently. (Reports 01, 02)

Theme 2 — Post-initiative features carry the new risk

The aggregated live alarm stream and KPI rollups shipped after the review baseline with no adversarial review: the rollup backfill (H4), rate-unit discontinuity at the raw/rollup routing boundary (~60× magnitude jump) and a bucketer re-fold error on rate metrics, per-delta full-snapshot copies + per-circuit fan-out with no coalescing in the live alarm cache, sticky IsLive grafting frozen snapshots after aggregator death, and a stale-site poll race on the Alarm Summary page. None are Critical individually; together they say new features need the round-1 treatment before they accumulate. (Reports 02, 04, 07)

Theme 3 — The script-artifact invalidation bus publishes into the void

The PLAN-05→06 handoff never completed: BundleImporter publishes ScriptArtifactsChanged, but zero Subscribe call sites exist repo-wide — the Host comment claiming the Inbound API consumer "wired in plan 06" is factually wrong. Correctness currently self-heals via the revision-checked handler cache, so this is a Medium not a Critical — but the contract everyone believes exists, doesn't. Wire the consumer or delete the claim. (Reports 06 N1, 05 N3)

Theme 4 — The eager-options-validation convention has a tail

PLAN-08 validated 12 components, but knobs added by the fix plans themselves missed the sweep: TagSubscribeRetryIntervalMs (0 hot-loops), StuckScriptGraceMs (negative throws), MaxConcurrentImportSessions (0 permanently blocks imports), 6 AuditLog sub-options, and Host's NodeOptions (empty NodeName silently NULLs audit SourceNode). One follow-up batch closes it. (Reports 03 N3, 05 N6, 08 NF4)

Theme 5 — Security seams the fixes exposed

LoginThrottle landed everywhere except the DebugStreamHub bind; no ForwardedHeaders handling anywhere means behind Traefik the throttle key collapses to username|proxy-IP — anyone can lock out admin for all operators with 5 bad passwords; and none of the four secured-write handlers enforces site scope, so a site-scoped Operator/Verifier can submit/approve MxGateway device writes fleet-wide — the same class of gap C2 (round 1) just closed, on a higher-consequence path. (Report 07 N1–N3)

Theme 6 — Deferral tracking is fragmenting again

The canonical deferred-work register is substantially accurate (all 10 "Resolved 2026-07-10" claims spot-verified true), but the SBR oldest-crash gap and the wonder-app overlay edits live only in the master tracker, the perf-envelope row's revisit trigger fired unnoticed, an untracked deferred.md snapshot has drifted, and the CHANGELOG (last touched 2026-06-02) is now factually wrong. One tracking home, enforced. (Reports 01 N6, 08 NF3/NF5)

What's Genuinely Good (keep doing this)

• The fix plans held under adversarial re-verification — 168 findings re-proven fixed from source, zero regressions, zero false claims; several fixes exceeded scope.
• Structural guards over point fixes: the reflection round-trip suite (Transport), the frozen 138-command authz matrix + dispatch-coverage pair, contract-lock tests (which surfaced the real Newtonsoft default(T) wire behavior), and the design-invariant DI lock tests.
• The live alarm cache lifecycle engineering (ref-counting, linger, version-checked timers, fail-safe viewer cap, self-healing start retry) is exemplary — the defects around it are in the fan-out economics, not the lifecycle.
• NodeB-failover reconciliation pull clients and the composite keyset cursor dodge every classic trap (transport-fault-only failover, per-row fault isolation, DateTime.MinValue underflow).
• Conventions survived 268 commits of churn: UTC discipline, Commons purity, additive contracts, clean reference graph all re-verified.

Prioritized Recommendations

P0 — immediately

1. Rotate the exposed Inbound API key and delete test.txt. (H2)
2. Fix the resync predicate inversion — one shared oldest-member active-node predicate for delivery gate AND resync authority. (C1)
3. Chunk the anti-entropy snapshot (or page it) so it fits Akka remoting frames; add a delivery-confirmation check. (H1)

P1 — before relying on the affected features 4. Bound the rollup backfill (time-sliced like the purges, AsNoTracking, skip-on-failover-if-current) and fix the rate-metric unit discontinuity at the raw/rollup boundary. (H4, Report 04 R2/R3) 5. Make the failover drill honest: kill the younger node in the scripted drill (document the oldest-crash gap), fix seed-list ordering or document the constraint, then actually run it and measure the ~25s envelope with the now-landed two-node rig. (H3) 6. Enforce site scope in the four secured-write handlers; throttle the DebugStreamHub bind; add ForwardedHeaders handling so throttle keying survives Traefik. (Theme 5) 7. Wire the Inbound API subscriber to IScriptArtifactChangeBus (or remove the claim from Host and the tracker). (Theme 3)

P2 — hardening batch 8. Close the options-validation tail (5 spots, Theme 4); guard the MxGateway reconnect generic-catch with ct.IsCancellationRequested (Report 03 N1); add failure mapping to the expression-eval PipeTos (03 N2); batch the SiteCalls terminal purge (04 R6); cache Expression-trigger compiles on read-only validation paths (05 N1); coalesce live-alarm deltas (02 N6); fix the Alarm Summary stale-site poll race (07 N4). 9. Consolidate deferral tracking into the canonical register; refresh or delete the CHANGELOG; sweep the doc drift each report catalogues (stale "leader" comments, "off-thread" compile-gate claim, "throttling complete" claim, Host bus comment).

Reading Order

If you read only three reports this round: 02 (the new Critical, plus the live-alarm stream review), 01 (what the failover story still owes), 04 (the rollup backfill). Report 08 again carries the best overall-health picture, the refreshed test inventory (~30 projects, ~6,300 executed cases), and the deferred-register audit.