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.
P7-6 (behaviour fix, plan decision 1): DeploySiteAsync redeployed EVERY
deployable instance unconditionally. A redeploy is not idempotent at the site —
it resets static attribute overrides and clears native alarm state by design — so
one site-wide push destroyed live operational state on every unchanged instance
in the batch, which is normally most of them. Bulk deploy now pre-filters with
IStaleInstanceProbe (the same seam the bundle importer and Deployments page use;
its memo timing was fixed by the Medium wave's P7-4): an instance whose freshly
computed revision hash still matches its DeployedConfigSnapshot is reported in the
existing additive SkippedInstances list with reason "up-to-date" and never
flattened, staged or sent. The filter is deliberately conservative — every
uncertainty deploys: only an Enabled instance qualifies, a missing snapshot
disqualifies, a prior record that ShouldQuerySiteBeforeRedeploy flags goes through
prepare so the reconcile that HEALS it still runs, and a null/faulting probe means
"indeterminate". New additive MgmtDeploySiteCommand.Force (default false = the new
filtered behaviour) plus CLI `deploy site --force` restores the unconditional push
for the case central cannot see (restored volume, rebuilt node). Composes with the
Medium wave's P7-2: Disabled wins — force suppresses the up-to-date filter only,
never the Disabled skip.
P7-5 (verified, then fixed): the cascade REPRODUCES. All three bulk phases share
one scoped DbContext, and a failed SaveChanges does not untrack the offending
entity — EF re-attempts it on the next save, including MarkDeploymentFailedAsync's
cleanup write. Measured on a real SQLite-backed ScadaBridgeDbContext: a permanent
save failure on instance 1's post-success side effects failed instances 2, 3 and 4
and left all three deployment records stranded InProgress. MarkDeploymentFailedAsync
now detaches the pending changes (new IDeploymentManagerRepository.DetachPendingChanges)
when the cleanup save fails and retries the Failed write once on the clean tracker,
so later instances save cleanly and the record reaches a terminal status. The
DeployFailed audit row moved outside the status-write try (best-effort, must not be
lost because the status write failed).
Tests: 5 new DeploySiteAsyncTests (skip + reason + no site round-trip; --force
deploys; Disabled skipped under both force values; the three not-enough-to-skip
guards), 2 new IntegrationTests over a real EF context (the poisoned-context
cascade, and the narrow EF tracking fact the fix rests on), CLI --force flag
parse test, and a MgmtDeploySite wire-default test. Proof of failure: both P7-6
behaviour tests and both P7-5 assertions fail against the unfixed code.
Six adversarial-review findings, each verified against the code first.
F1 (HIGH) CLI HttpClient capped every call at min(30s, caller timeout),
silently truncating deploy site's 5-minute BulkDeployTimeout and the
5-minute bundle export/preview/import calls — which printed a fake
"504 Request timed out" while the server kept working. HttpClient.Timeout
is now Timeout.InfiniteTimeSpan (the per-call CTS is the single overall
deadline, connect included) with the connect phase bounded separately on
SocketsHttpHandler.ConnectTimeout. The env override is renamed to
SCADABRIDGE_HTTP_CONNECT_TIMEOUT_SECONDS to match its new meaning.
F2 (HIGH) StaleInstanceProbe's process-static memo served stale hashes
because nothing bumped the watermark on three paths:
(a) BundleImporter commits through the raw DbContext, so no import ever
moved the watermark — a second import overwriting the same template
could be OMITTED from ImportResult.StaleInstanceIds. It now bumps
once per apply ATTEMPT: after the commit, and after the rollback too
(the probe runs pre-commit, so a rolled-back attempt leaves memos for
state that never landed; bumping on both paths is the simplest
correct shape, versus threading transaction awareness through a
process-static cache).
(b) CollectWatermarkBumps' default: arm silently no-op'd, contradicting
its own doc. It now sets unattributed=true — an over-broad bump costs
extra work, a missed one produces stale work.
(c) DataConnection edits route through SiteRepository.SaveChangesAsync,
which had no watermark at all, yet Protocol/Primary+Backup config/
FailoverRetryCount are revision-hash inputs. It now bumps (BumpAll —
a connection has no owning template) after a commit that touched one.
F3 (MED) CLI TemplateTableProjection read child ARRAYS, but ListTemplates
now returns database-projected TemplateSummary rows, so template list
printed all zeros. It now prefers the *Count scalars and falls back to
array length (template get still returns full entities). --detail help
text and README corrected: a listing cannot yield definitions, so --detail
renders the raw summary payload and template get --id is the full dump.
F4 (MED) DeploySiteAsync staged every PendingDeployment in phase 1 against
a 5-min TTL while phase 2 reached them one batch at a time, so tail
instances' fetch tokens could expire before their command was sent.
Staging moved into phase 2, immediately before each send; prepare keeps
its flatten/validate/record work. The staging write is the phase's only
repository touch and is serialised behind a 1-permit semaphore, so the
non-thread-safe DbContext constraint holds and the sends stay concurrent.
F5 (MED, latent) DeploySiteAsync leaked every held operation lock if
cancelled — a wedged per-instance semaphore is permanent for the process.
Phase 2 no longer throws (cancellation is recorded as a per-instance
outcome so phase 3 still runs), and an escape from phase 1 or 3 now
unwinds every unfinalised entry: Failed status + lock release.
F6 (LOW) ScriptCompileVerdictCache's promotion wrote hot directly,
bypassing SegmentCapacity (true ceiling 3x against a documented 2x).
Promotion now goes through Store, keeping generational semantics; _hot
and _cold are volatile.
Tests: CLI 396, DeploymentManager 133, ManagementService 494,
TemplateEngine 478, ScriptAnalysis 60, Transport 157, Transport
integration 106, ConfigurationDatabase 366 — all green, 0 build warnings.
The F2/F4/F5 regression tests were each confirmed to FAIL with their fix
reverted.
A NotDeployed instance has no live Instance Actor at any site, so Delete is a pure
central-side record cleanup. Transport-imported instances always land NotDeployed
(often against an uncommissioned/unreachable site); a site round-trip made them
undeletable until the site came online. Short-circuit the site call for NotDeployed.
Claude-Session: https://claude.ai/code/session_01MtdgwpEeCUn6cUA5f1LMPj
Route all post-terminal-status lifecycle audit writes (Deploy/Disable/Enable/Delete/DeleteOrphaned) through a swallow-and-warn TryLogAuditAsync helper so a failed best-effort audit write can neither reach the outer catch (flipping Success to Failed) nor propagate to the caller.
Claude-Session: https://claude.ai/code/session_01MtdgwpEeCUn6cUA5f1LMPj
Remove project bookkeeping citations from shipped code comments across the
solution: hyphenated task IDs (WP-14, StoreAndForward-025), milestone/task/
issue refs (M3, Task 4, Audit Log #23, #21), Bundle X task-bundle labels,
and C/D/K/S/T phase labels.
Comment text only — no code logic, string/log literals, or XML-doc structure
changed. Genuine descriptions are preserved (only the citation is stripped),
and technical lookalikes are retained (UTF-8, SHA-256, T00:00:00, M365,
UTC-5, pre-C4/pre-C5 schema versions). Flagged by the new CommentChecker
TaskReferenceInComment / TrackingReferenceInComment checks plus targeted
grep passes; full solution builds clean, append-only guard tests pass.
Notify-and-fetch follow-ups:
- PendingDeploymentPurgeActor: a central cluster singleton (not
readiness-gated, best-effort) that sweeps expired PendingDeployment
staging rows on CommunicationOptions.PendingDeploymentPurgeInterval
(default 1h). Modeled on the kpi-history-recorder pattern: self-scheduling
timer, per-tick DI scope -> IDeploymentManagerRepository, continue-on-error.
Wired in AkkaHostedService.RegisterCentralActors (manager + proxy + drain);
resolves the deferred TODO in DeploymentService. Correctness never depends
on it (supersession bounds rows to <=1/instance; the fetch endpoint enforces
the TTL), so it is deliberately absent from RequiredSingletonsHealthCheck.
- SQL Server integration test for StagePendingIfAbsentAsync re-staging an
instance's OWN DeploymentId over an expired row against the real UNIQUE
index on DeploymentId — confirms EF orders DELETE before INSERT in one
SaveChanges (SQLite's constraint timing differs from SQL Server's). Plus
a same-instance supersession variant on real SQL Server.
Tests: 2 TestKit actor tests + 2 SQL Server integration tests (both ran
green against the infra MSSQL container); 235 Communication + 15
PendingDeployment tests pass; Host builds 0 warnings.
Akka.Actor.AskTimeoutException does not derive from System.TimeoutException,
so the isTimeout check in DeployInstanceAsync's catch block missed it and
routed it to the generic "Deployment error:" branch. This broke the
DeploymentManager-006 reconciliation query (query-before-redeploy), which
keys off the "Communication failure:" prefix to detect a prior timeout-induced
failure. Add AskTimeoutException to the pattern; add a covering regression test.
Closes the four remaining items in the 2026-06-24 template-inheritance/CLI
follow-up tracker.
#4 — CLI `instance set-bindings` can now set DataSourceReferenceOverride.
`--bindings` accepts an optional 3rd element per entry:
[attributeName, dataConnectionId, dataSourceReferenceOverride]. A string
sets the override; a JSON null or an omitted 3rd element leaves it unset
(template default). TryParseBindings accepts 2- or 3-element entries and
rejects a non-string/non-null 3rd element or 4+ elements with a clean
error. Previously the CLI sent the override as null and silently wiped any
existing one (only a raw POST /management could set it).
#5 — `template update` is partial, not full-replace (fixed server-side so all
clients benefit). UpdateTemplateAsync now uses leave-unchanged semantics:
a null description keeps the stored value (pass "" to clear); a null
parentTemplateId keeps the existing parent. Parent stays immutable — a
non-null differing value is still rejected — but omitting --parent-id is
now a no-op instead of failing every derived-template update.
#6 — compact `template list`/`get` table output + `--detail`. Table output is
now id/name/description/parent/derived + member counts (#attrs/#alarms/
#scripts/#comps/#nativeAlarms) via TemplateTableProjection, fed through a
new optional tableProjector seam on CommandHelpers. `--detail` restores the
full dump. JSON output is left untouched (always full) so machine consumers
are unaffected — the projector only runs on the table path.
#8 — structured deploy-time validation error. New ValidationResult.SummarizeErrors()
(Commons) returns a grouped, capped summary: leading total count, one line
per ValidationCategory, and a per-module rollup (canonical name up to its
last dot) with counts + "... and N more module(s)" caps. DeploymentService
uses it for the "Pre-deployment validation failed" message and logs the full
per-entry list via LogWarning. Replaces the flat semicolon-joined dump that
became a wall of text for instances with 50-194 unbound attributes.
Tests: +8 Commons (SummarizeErrors), +8 CLI (4 binding 3-element / 4 table
projection), +2 net TemplateEngine (partial-update). Affected suites green:
Commons 587, CLI 341, TemplateEngine 447, DeploymentManager 101,
ManagementService 230, CentralUI 866; full solution builds 0/0.
Docs: Component-DeploymentManager.md "Validation Error Reporting"; CLI README
(set-bindings 3-element form, template update leave-unchanged, list/get
--detail); UpdateTemplateCommand doc; known-issues tracker #4/#5/#6/#8 resolved
(all 8 items now closed).
Resolve all 622 issues flagged by the enhanced CommentChecker: add missing
<returns> tags (incl. the standard phrasing on non-generic Task methods),
add missing <summary> tags, and replace misused/redundant <inheritdoc/> on
members that override or implement nothing with real documentation.
Documentation-only — no behavior change; solution builds clean.
Joseph Doherty