ebc55c5f729cbb43a874b257dee4faf638d600f3
1844 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Joseph Doherty
|
230c841d35 | merge: tag-resolution counts fixed on first subscribe (register row 55) — HandleSubscribeCompleted was the missing fourth UpdateTagResolution push site, so a perfectly healthy connection reported 0/0 forever | ||
|
Joseph Doherty
|
c26b52ad68 | merge: observability (register rows 33, 54, 56, 63, 66) — LocalDb backlog KPI history + sustained-threshold badge, honest stream-subscriber gauge rename + real data-connection gauges, central metrics port on the rig, ASPNETCORE_ENVIRONMENT on site containers, import-session rejection counter | ||
|
Joseph Doherty
|
d607011e5e | merge: small fixes (register rows 28, 35, 57) — confirm dialogs name their own action (9 mislabeled 'Delete' buttons fixed incl. secured-write Approve/Reject), DeploymentRecords composite (DeployedAt DESC, Id DESC) index, audit-config query CLI table projection | ||
|
Joseph Doherty
|
92fe5acca7 |
refactor(auditlog+sitecallaudit): delete the _injectedRepository test ctors — register row 61
AuditLogIngestActor and SiteCallAuditActor each exposed constructors taking a scoped
repository and caching it on a field for the actor's lifetime. Safe only because no
production call site picked those overloads — i.e. one call site away from re-introducing
the row-45 captive-dependency bug — which is why CaptiveDependencyGuard had to allowlist
both fields.
Preferred fix applied: the test fixtures fold onto the IServiceProvider overload with a
stub provider, and the constructors are gone.
Product deletions:
- AuditLogIngestActor: the (IAuditLogRepository, ILogger) ctor, the _injectedRepository
field and its branch in OnIngestAsync, and OnCachedTelemetryWithoutDualWriteAsync (the
"this ctor cannot dual-write" fallback handler that only that ctor installed).
_serviceProvider is now non-nullable and the null-forgiving operators are gone.
- SiteCallAuditActor: both test ctors (repo-only, and repo + reconciliation
collaborators), the _injectedRepository field and its four branches, and the private
SerializedRepository class. _backgroundTimersEnabled went with them: only the deleted
repo-only ctor ever set it false, so it was a constant and its two guards are removed.
The reconciliation tick is still gated — on the collaborators being resolvable, which
is how production reaches the same state.
Test fixtures (12 call sites): two small TestSupport helpers build the stub providers.
SiteCallAuditActor's production wiring resolves a FRESH repository per scope, so the
serializing decorator existed only to protect fixtures that share one DbContext-backed
instance across every scope — it moves into the test project as
SerializingSiteCallAuditRepository, which is where the hazard is actually created.
Fixtures that relied on it keep it. The relay test's ICentralAuditWriter now arrives via
the provider, exactly as production resolves it.
Behaviour is preserved on the paths under test: the deleted audit ctor passed
redactor:null / failureCounter:null, and a provider registering neither resolves both to
null, so the SafeDefaultAuditRedactor fallback and counter-free logging are unchanged. No
fixture on the folded sites sends IngestCachedTelemetryCommand, so swapping to the real
dual-write handler is inert. The SiteCallAudit purge timer now arms in fixtures that
previously had timers off, but every one of them uses default purge settings whose first
tick is 5 minutes out — nothing fires inside a test.
One reconciliation test was renamed and rewritten rather than deleted:
TestCtor_RepositoryOnly_DoesNotStartReconciliationTick became
NoReconciliationCollaboratorsRegistered_DoesNotStartReconciliationTick — the same
guarantee, expressed the way production actually reaches it.
Guard: the allowlist is now EMPTY. The staleness self-test (every entry must still match a
flagged field) is replaced by two stronger pins — Allowlist_IsEmpty asserts nothing is
exempt at all, and FormerlyAllowlistedActors_NowScanClean scans both actors with the
allowlist DISABLED, so the empty allowlist is evidence the fields are gone rather than
evidence the scan stopped looking.
Negative control: re-adding an ISiteCallAuditRepository field to SiteCallAuditActor fails
both FormerlyAllowlistedActors_NowScanClean and the real
Central_NoActorHoldsCaptiveScopedState scan.
Verification: solution builds clean. AuditLog.Tests 3/3 (383), SiteCallAudit.Tests 3/3
(66), Host.Tests 2/2 (505), IntegrationTests 2/2 (115), Communication.Tests 2/2 (717).
|
||
|
Joseph Doherty
|
b2cab59cb9 |
feat(cli): compact table projection for audit-config query (register row 57)
The command passed no `tableProjector`, so `--format table` handed the renderer
an `AuditLogQueryResult` OBJECT. That takes the generic Property/Value branch,
which printed the entire `entries` array as raw JSON inside a single cell. JSON
output was and remains correct, and is the primary format for this command.
Adds `AuditConfigTableProjection`, mirroring `TemplateTableProjection`:
string->string over the raw body, applied only on the table path, returning the
input verbatim when it is not JSON or not a recognised shape so the renderer's
own fallbacks still apply. It unwraps the envelope to a row array — id,
timestamp, user, action, entityType, entityName, entityId — and drops
`afterStateJson`, the full post-change entity snapshot, which is the audit
analogue of template's fat child arrays: one per row fills a terminal.
`bundleImportId` is dropped as mostly-null noise.
`totalCount` would be lost by a plain unwrap, so a truncated page gains a single
trailing row with a `note` cell ("showing 50 of 137 — use --page / --page-size
for the rest"). When the page IS the whole result set the column is not emitted
at all, so a small query's table stays exactly as narrow as its data, and the
note row carries no `id` so it cannot be mistaken for an entry.
A `--detail` flag skips the projection and renders the raw payload, matching
`template list` / `template get` — that plus `--format json` is how to reach
`afterStateJson`. A bare array root is also accepted, so the projection does not
regress if the payload is ever unwrapped upstream or the deprecated `audit-log`
alias meets an older server.
Tests build their fixtures by serialising the REAL AuditLogQueryResult /
AuditLogEntry types, so renaming a server property fails the test instead of
silently blanking a column. CLI README + Component-CLI.md document the table
shape, the omitted fields, and --detail.
|
||
|
Joseph Doherty
|
be29bf471e |
perf(configdb): index DeploymentRecords on the deployment-list sort key (register row 35)
The deployment list orders `DeployedAt DESC, Id DESC` and joins `Instance` to make SiteId filterable, but the only DeployedAt index was single-column and ascending, so the server sorted the filtered set on every page request. Adds `IX_DeploymentRecords_DeployedAt_Id (DeployedAt DESC, Id DESC)` — the exact sort key, both columns descending, so a forward scan feeds the paged Skip/Take directly. Id is in the key as the deterministic tie-break: a bulk site deploy stamps many rows in the same DeployedAt instant, and without it a row can repeat or vanish across page boundaries. Three reads share the key (QueryDeploymentListPageAsync, QueryDeploymentSummariesAsync, and the single-row GetCurrentDeploymentStatusAsync). DROPS the old `IX_DeploymentRecords_DeployedAt` rather than keeping both. It is strictly redundant, not merely overlapping: DeployedAt leads both, so every predicate it served the composite serves too (direction does not affect seekability — SQL Server scans either way), and the leaf rows are the same width, because Id is the clustered PK and was therefore already in the old nonclustered index's leaf as the row locator. All that changes is Id becoming a declared, ordered key column. Keeping it would maintain a second index on every deployment insert and status transition for no read benefit. Honest caveat: because the clustering key is implicitly ordered within a nonclustered index, the optimizer may already have been able to serve this ordering by scanning the old index backward. The explicit composite makes it deterministic and self-documenting rather than dependent on that inference, and costs nothing to declare. Migration 20260815165452_AddDeploymentRecordListSortIndex (scaffolded with a real build, non-empty, snapshot updated) plus the hand-written production script under docs/plans/sql/ following the AddAuditLogAndNotificationCoveringIndexes precedent. SystemArtifactDeploymentRecords has its own separate DeployedAt index and is deliberately untouched. |
||
|
Joseph Doherty
|
0db253b55f |
fix(centralui): confirm dialogs name their own action instead of always saying "Delete" (register row 28)
The Health dashboard's "Trigger failover" confirmation offered a red button
labelled *Delete*. The copy was right; the button was not. `DialogHost` derived
the confirm label solely from `danger`, so `danger: true` meant the verb was
literally "Delete" for every caller — and no caller could say otherwise, because
`IDialogService.ConfirmAsync` had no label parameter.
`danger` now selects styling only; a new optional `confirmLabel` names the action
and wins over the fallback. The kind/danger fallback is deliberately RETAINED
("Save" for a prompt, "Delete" for a bare danger confirm, "Confirm" otherwise):
around 20 call sites are genuine deletes where it is correct, and the Playwright
suites click the literal "Delete". Requiring the label everywhere would have
churned those 20 sites for no behaviour change, with the miss-detection resting
on Playwright runs that need a live rig.
Audited all 34 confirm call sites; 9 read "Delete" for something that is not a
delete and are relabelled:
Fail over CentralFailoverControl (central pair, site pair)
Approve SecuredWrites — approve (this one WRITES TO A DEVICE)
Reject SecuredWrites — reject
Disable Topology — disable instance
Discard SiteCallsReport, NotificationReport, ParkedMessages (bulk + single)
The remaining 25 are correct as they stand: 18 genuine deletes plus 7 non-danger
confirms ("Revert to base", the three Retry paths) whose button reads "Confirm".
Tests: FailoverConfirmLabelTests drives the real DialogService behind a real
DialogHost — a substitute would pin the argument but not what the operator reads
— covering both branches, that the button keeps btn-danger, and that an
unlabelled danger confirm still falls back to "Delete". Note its click is
deliberately not awaited: with a real dialog the handler parks on ConfirmAsync
until answered, so awaiting deadlocks. Existing IDialogService fakes and
NSubstitute matchers gain the 4th parameter (an optional param is baked into the
call site, so a 3-arg Arg.Any stub silently stops matching a labelled call).
|
||
|
Joseph Doherty
|
8da60f57ac |
feat(health): KPI history + sustained-backlog badge for LocalDb replication (row 33)
LocalDbOplogBacklog was reported and Prometheus-exported with nothing consuming it: not sampled into KPI history, so a purge-burst spike could not be reviewed after the fact, and no threshold treatment anywhere. Both halves shipped. KPI SAMPLING. SiteHealthKpiSampleSource now emits localDbOplogBacklog and localDbReplicationConnected (0/1), and the backlog is charted on the Health dashboard's per-site trend panel. Charting is the point: a point-in-time tile can never answer 'did that spike drain?', which is the only thing separating an expected purge burst from a stalled peer. Both are CONDITIONALLY emitted rather than coerced with ?? 0 like the auditBacklogPending metric beside them. Both health-report fields are nullable and null means 'no reading' — replication not configured (rig site-b/site-c) or a failed poll — never zero. A fabricated 0 would render a pair that cannot read its own oplog, or one deliberately unreplicated, as perfectly healthy once a minute forever, burying the real series. So a site emits 12 metrics when unreplicated and 14 when replicated, and a GAP in these two series truthfully means 'not replicated / not reporting'. Note false and null differ for the connected flag: a configured link that is DOWN samples as 0; no link samples as nothing. Both fold as Gauge (summing an oplog depth, or a 0/1 flag, per hour is meaningless). THRESHOLD SEMANTICS — sustained, not instantaneous. This is the design, not a refinement. The site_events retention purge is CDC-captured like any other write, so the backlog jumps by the deleted batch and drains as the peer acks it; a bare 'backlog > threshold' badge would fire on every replicated site every day and be ignored within a week. Healthy is defined by the backlog RETURNING to ~0, which only a dwell can express. CentralHealthAggregator holds LocalDbBacklogAboveSince — the instant the backlog first rose above LocalDbBacklogThreshold (default 1000) — and the badge appears only after LocalDbBacklogSustainedFor (default 15 min). A report at or below the threshold clears the anchor; one above leaves the ORIGINAL instant intact, so the window is sustained rather than sliding. The null case is deliberately a third branch: a null backlog leaves the anchor UNCHANGED rather than clearing it. Null is evidence neither of a drain nor of a backlog, so a node going blind mid-climb must not silently reset its own dwell, and an unreplicated site (null forever) never accumulates an anchor and can never be badged. Validation rejects a non-positive threshold (0 would badge every replicated site permanently, since an idle backlog already sits at it) and a dwell shorter than one ReportInterval (satisfiable by a single sample, which collapses the check back into the bare threshold it replaces). DOC CORRECTION worth flagging: this row's premise was 'mirror the SiteAuditBacklog configurable threshold badge'. That badge does not exist — Component-HealthMonitoring.md described it in two places but no option, no comparison and no markup were ever implemented. The two stale claims are corrected here rather than left to send the next reader looking for it; the LocalDb badge is built fresh on the Metrics-stale badge's pattern, which is a real configurable-threshold badge. Tests: HealthMonitoring 115 (9 new aggregator tests incl. purge-burst-drains, unreplicated-never-anchors, null-mid-climb, exact-threshold, stale-report; 5 new validator tests; 4 new sampler tests incl. the null-semantics and false-vs-null cases); CentralUI 1042 (5 new badge/chart tests incl. the purge-burst negative control); Commons Kpi 93; KpiHistory 54. |
||
|
Joseph Doherty
|
41a5ecf14a |
feat(external-systems): deploy per-system retry policy to sites + CLI/management surface (register row 27)
An external system's MaxRetries/RetryDelay existed on the central entity but never reached a site and had no management surface, so every ExternalSystem.CachedCall buffered with the S&F engine default (DefaultMaxRetries 50 x DefaultRetryInterval 30s ~= 25 min to park) whatever central had configured. The sibling DatabaseConnectionArtifact had always carried its equivalents. The consumption end was already wired: ExternalSystemClient.CachedCallAsync already read system.MaxRetries/RetryDelay and passed them to EnqueueAsync with the 0-as-unset convention. Only the transport and persistence were missing. Commons - ExternalSystemArtifact gains MaxRetries/RetryDelay as optional positional params (0 / TimeSpan.Zero = unset), matching DatabaseConnectionArtifact rather than going nullable, so it maps 1:1 onto the entity and the proto3 scalars. - Create/UpdateExternalSystemCommand gain nullable MaxRetries/RetryDelay, following the existing TimeoutSeconds "omit to preserve" convention. Both additive. Central - ManagementActor create maps both; update preserves-if-null. - ArtifactDeploymentService projects them onto the artifact. Wire - site_command.proto ExternalSystemArtifactDto gains max_retries=7 / retry_delay=8 (google.protobuf.Duration), mirroring DatabaseConnectionArtifactDto. Additive: a pre-existing site decodes them as 0/absent, which IS the unset value, so old behaviour is preserved. Vendored C# regenerated via docker/regen-proto.sh sitecommand; csproj Protobuf item re-commented, no message types added or removed. Site - external_systems gains max_retries / retry_delay_ms via AddColumnIfMissing, DEFAULT 0 = unset. Storage upsert, repository SELECT/mapping (new ordinals 6/7 appended so existing ones are untouched), apply path, and the legacy migrator's explicit column list all carry them. Replicated-table decision: external_systems IS in SiteLocalDbSetup.ReplicatedTables, so this was checked against the AckTime/native_alarm_state precedent. Real columns are correct here, not a JSON bag: the table has no general-purpose config column (auth_configuration and method_definitions are both semantically specific), timeout_seconds already set the additive-column precedent on this exact table, and sibling database_connections stores the same two values as plain columns. Verified in LocalDb source that RegisterReplicated re-reads PRAGMA table_info every boot and its install script is DROP-then-CREATE, so capture triggers rebuild over the new column set automatically, and SiteStorageSchema.Apply runs before registration on the same boot. The only cost is the ReplicatedTable.ComputeDigest change, which the sync handshake compares fail-closed - a mixed-version pair refuses to sync loudly instead of diverging silently. Both-nodes-together is already the standing site-pair rule, so no new operational constraint; recorded in topology-guide.md. Also fixed: ArtifactDiff.CompareExternalSystem never compared MaxRetries/RetryDelay, so a bundle import that changed a system's retry budget previewed as "Identical" - harmless while the values were inert, a live behaviour change now. No EF migration: the central ExternalSystemDefinitions columns already exist (InitialSchema). Central UI already had both form fields. Tests: site storage round-trip + unset-default, site apply path, central artifact projection, proto round-trip (via the shared sample), management create/preserve, CLI option parse + alias, and diff reporting. Fail-before verified on the two core tests (expected 7 got 0 on the read path; expected 9 got 0 on the apply path). Docs: Component-ExternalSystemGateway, Component-StoreAndForward, Component-CLI, components/StoreAndForward, components/SiteRuntime, deployment/topology-guide, CLI README (which was also missing --timeout-seconds entirely). |
||
|
Joseph Doherty
|
103f2f1931 |
feat(host+rig): give central a dedicated Prometheus scrape port (register row 56)
The row read 'central nodes expose no Prometheus endpoint'. That is not quite
what was wrong, and the difference matters. Central already maps /metrics
(Program.cs central branch) and telemetry is registered for both roles through
the shared BindSharedOptions, so the endpoint rides endpoint routing and has
always been served on central's web port (:5000 on the rig, anonymous — there
is no FallbackPolicy). What central lacked was the site branch's dedicated
MetricsPort, which is the port the established live-scrape recipe probes:
docker exec <node> bash -c 'exec 3<>/dev/tcp/localhost/8084 ...'
So a probe of central on 8084 failed and central-only series (KPI/outbox
counters, the bundle-import session gauge + its new rejection counter,
alarm-cache gauges) read as unscrapeable while they were in fact being
exported on a different port.
Central now binds a dedicated HTTP/1.1 listener on ScadaBridge:Node:MetricsPort,
so one probe recipe covers all eight rig nodes. HTTP/1.1-capable because a
standard Prometheus scraper cannot negotiate h2c, which is why this cannot
share the gRPC port.
Read as a PRESENCE check, not GetValue's defaulted read. NodeOptions defaults
MetricsPort to 8084, so a defaulted read would make every existing central node
start binding a new port on upgrade — and a port already in use is a hard boot
failure, not a degraded one. Central gains the listener only where an operator
asked for it; site nodes are unchanged and still bind theirs unconditionally.
The bind is also skipped when the port would collide with the gRPC port or an
ASPNETCORE_URLS port, since Kestrel throws on a duplicate ListenAnyIP and
/metrics is reachable on those ports anyway.
Config only — no host port mapping added, because the probe runs inside the
container. Applied to both docker/ centrals and both docker-env2/ centrals.
FILE EDITS ONLY: no docker command was run, and deploy.sh was not invoked.
|
||
|
Joseph Doherty
|
b5f120c370 |
feat(observability): honest stream-subscriber name + real data-connection gauges (row 54)
scadabridge_site_connection_up never counted data connections. It counts open central->site gRPC subscriber streams (Debug View + the site-wide alarm feed), so it correctly reads 0 on a healthy site nobody is watching — and the name invited reading it as device health. That misreading has already cost a live debugging session: register row 48's 'connection_up is 0 on all six site nodes' was healthy steady state, not the fault it was diagnosed as. Two halves, per the row: RENAME. scadabridge.site.connection.up -> scadabridge.site.stream.subscribers.active (helpers SiteConnectionOpened/Closed -> SiteStreamSubscriberOpened/Closed), and the instrument gains the unit:"1" the other gauges already carried. Every reference updated: the gRPC server, both test suites, Component-Communication.md, components/Commons.md, docker/README.md and docker/seed-live-probe.sh. A test now pins the new name AND asserts the old one is gone, so it cannot creep back. NEW. scadabridge.site.data_connections.connected / .total — the first exported data-connection signal (the state existed on the health report and in the CLI, but never as a metric). connected < total is the alert condition; neither number alone separates 'a device is down' from 'nothing is configured', which is why the denominator ships with the numerator. Sourcing is deliberate on two counts. It is fed by the DCL at its four state transitions rather than derived from the health report, so the metric survives a dead report pipeline — the exact failure the 'metrics stale' flag exists to catch, and a derived gauge would go blind at the same moment. And it uses a static registry reached through ScadaBridgeTelemetry (the SiteConnectionOpened precedent) rather than a DI-injected holder: the DCL actors already take ISiteHealthCollector through their constructors, and threading a second dependency through both would ripple into every test that builds them. It also keeps SiteHealthCollector untouched, which matters right now — another agent is concurrently editing that file for the tag-resolution path. Removal is wired too: an undeployed connection leaves the registry, so it stops counting toward the total instead of latching connected < total on forever. Both gauges read 0 on the standby (connections open on the active node only) — healthy, documented, and called out in the docs so it does not become the next site_connection_up. No ObservedMeters allowlist change: all three instruments ride the existing ZB.MOM.WW.ScadaBridge meter. (An unlisted meter exports nothing, silently.) Tests: Commons 711 passed (9 new, delta-based against a baseline with unique per-test connection names, so they do not inherit the register row 32 QueueDepthGaugeTests static-state flake); Communication 717; DCL 319. |
||
|
Joseph Doherty
|
5da27e05c7 |
fix(dcl): push tag-resolution counts on first subscribe (register row 55)
A site health report showed tagResolutionCounts 0/0 for a connection whose dataConnectionTagQuality on the SAME report read good:2 — reproduced live on the docker rig against site-a's rig-probe-1 instance (2 Good OPC UA tags on "OPC PLC Simulator"), on the ACTIVE node's own report. Not a standby-shadowing or keying/removal problem, and not a serialization problem: both dictionaries live on one collector instance on one node, and the CentralControlDtoMapper round-trip is correct in both directions. The actor's own derived counts (TotalSubscribedTagCount / ResolvedTagCount) were right the whole time — GetHealthReport replied 2/2. Root cause: HandleSubscribeCompleted — the path that registers tags on a first, healthy subscribe — mutated _instancesByTag and _subscriptionIds but never pushed to ISiteHealthCollector. The only pushes were BecomeConnected's (fires before any instance subscribes, so its 0/0 is legitimate), the batch apply (reconnect re-subscribe / resolution probe) and unsubscribe (teardown). A connection whose tags all resolve first time reaches none of those, and ScheduleTagResolutionRetry cancels the retry timer outright when nothing is unresolved — so the connect-time 0/0 was never superseded and stood forever. The blind spot was therefore worst on a perfectly healthy connection, which is exactly when the "device still booting / tag path unresolved" diagnostic should read clean rather than empty. Any reconnect masked it (the batch path pushes), which is why it presented only on a clean first deploy. Fix: push the derived counts at the end of the first-subscribe apply, mirroring the existing push site in HandleBatchSubscribeCompleted. No semantic change to the null/no-data convention — the map entry already existed, its values were simply stale zeros. Tests: fail-before regression test pins the exact mechanism (NSubstitute records a single UpdateTagResolution(name, 0, 0) pre-fix while the actor reports 2/2). DCL 320, HealthMonitoring 97, SiteRuntime 605, Communication 717 all green; slnx builds clean. Docs: Component-DataConnectionLayer.md said the counts were "derived at report time", which implies a pull that cannot go stale and papered over this bug. Corrected to state they are pushed, name the four push sites, and record why a healthy connection was the worst case. |
||
|
Joseph Doherty
|
482165cbac |
fix(siteruntime): startup reconcile starts the healed InstanceActor (register row 67)
SiteReconciliationActor's node-STARTUP fetch healed the config FILE but never the RUNTIME. Instance Actors are created from exactly two paths -- the Deployment Manager's boot-time SQLite load and an explicit deploy/enable command -- and the reconcile pass runs AFTER that boot load has already read storage. So a config reconcile restored started no actor: an enabled, successfully-deployed instance whose config was missing at boot (lost volume, first boot after a restore, rehomed data dir) stayed dead until the NEXT restart, whose load finally picked it up. Proven live on site-a 2026-08-15: "Reconcile: fetched + stored config" with no "InstanceActor started" after it, and a debug snapshot answering instanceNotFound. The pass now raises a fire-and-forget EnsureInstanceStartedCommand (new Commons Deployment message) for every ENABLED instance it healed, routed through the deployment-manager singleton PROXY so Instance Actors stay under the singleton no matter which node's reconcile found the gap. The handler is deliberately advisory: - idempotent -- no-op when the actor is already running, which both nodes' reconcile actors legitimately trigger for the same instance; - enabled-state-aware -- it re-reads the durable row, which is the authority on both the config body and enabled/disabled, so a disable landing after the fetch still wins and the instance stays stopped; - it defers to any in-flight deploy/redeploy/warm for the instance, which keeps the deploy gate's ordering guarantees and the all-or-nothing-per-instance rule intact -- this never applies a config, it only starts one already persisted; - a missing row is a quiet no-op, the normal case on an unreplicated site whose peer node healed its own file. The heal is no longer silent: a successful reconcile start emits a Warning log line and an operational `deployment` site event, because reaching it means the node booted without a config for an instance that should have been running. |
||
|
Joseph Doherty
|
90ce099928 |
feat(transport): counter for import-session cap rejections (register row 66)
Row 53 shipped the scadabridge.central.transport.import_sessions.open gauge
and a Warning log on a cap hit, but a log line is not something an operator
can alert on. Adds the companion counter
scadabridge.central.transport.import_sessions.rejected
on the existing ZB.MOM.WW.ScadaBridge meter, so no ObservedMeters allowlist
change is needed (an unlisted meter exports nothing, silently).
The emit point is deliberately narrow. BundleSessionStore.Open tests the cap
TWICE — once on entry and again after EvictExpired() — and only the second
test is a rejection: a store full of merely expired sessions reclaims a slot
and admits the caller. Counting the first would make the series fire
routinely on any node whose sweep had not yet run, turning a page-worthy
signal into background noise. Re-Opening an already-open session id skips
the cap block entirely and is likewise not counted.
The two instruments are diagnostic as a pair: rejections rising while the
gauge oscillates below the cap is genuine concurrency (raise the cap);
rejections rising while the gauge sits pinned at the cap is a session leak
(register row 43's signature), where raising the cap only defers the problem.
Tests assert a delta over a scoped action rather than an absolute value, so
they do not inherit the register row 32 static-state flake that makes
QueueDepthGaugeTests fail only under the full concurrent suite. Negative
controls cover under-cap, expired-reclaim, and duplicate-session-id.
Tests: 11 passed in BundleSessionObservabilityTests (6 existing + 5 new).
|
||
|
Joseph Doherty
|
cde4af77fd | merge: audit-config query fix + rig live-probe seed (register rows 46, 48) — ValueTuple serialized as {} fixed with AuditLogQueryResult record + ITuple guard; docker/seed-live-probe.sh | ||
|
Joseph Doherty
|
4785f3cbef | merge: transport hardening (register rows 51-53) — BundleLockedException curated to Commons/MapFault, fleet-wide MS SQL unlock-throttle window, import-session gauge + cap Warning | ||
|
Joseph Doherty
|
73f518d6a6 |
chore(transport): delete the dead BundleUnlockRateLimitedException
Superseded by BundleUnlockThrottledException when register row 44 wired the limiter live and moved the typed exception to Commons so ManagementService's MapFault could curate it. The original type has been unreferenced since — never thrown, never caught, only its own definition — and with row 51 moving BundleLockedException into the same Commons namespace there are now three similarly-named unlock exceptions, one of which does nothing. A future maintainer catching the dead one would silently get no coverage. Solution builds clean with it gone (zero references). Transport 173 passed. |
||
|
Joseph Doherty
|
00a804e6d9 |
guard(di): captive-dependency tripwires — scope validation + an actor field scan
Register row 50. Row 45 (fixed
|
||
|
Joseph Doherty
|
677263af57 |
feat(transport): fleet-wide bundle-unlock throttle window (register row 52)
The per-IP unlock throttle's counters were a process-local ConcurrentDictionary on each central node, so the effective fleet cap was N x MaxUnlockAttemptsPerIpPerHour per client (failing over to the other node handed the client a fresh budget) and a node restart cleared the window outright. The window now lives in the central MS SQL BundleUnlockAttempt table — the HA store both central nodes already share, so no new infrastructure, no new port, no consensus protocol: - Commons: BundleUnlockAttempt entity + IBundleUnlockAttemptRepository. - ConfigurationDatabase: entity config (one index, IX_BundleUnlockAttempt_Client_Attempted, serving both the windowed count and the prune), repository (AsNoTracking count, ExecuteDeleteAsync prune), DbSet, DI registration, migration AddBundleUnlockAttemptTable + the hand-written idempotent production script under docs/plans/sql/. - Transport: IBundleUnlockAttemptWindow seam + SqlBundleUnlockAttemptWindow (singleton; resolves the scoped repository from its own scope per call, like the central purge singletons). BundleUnlockRateLimiter keeps the policy and gains TryRegisterAttemptAsync; BundleImporter.LoadAsync calls it. Behaviour preserved from row 44: locked bundles spend no hourly budget (the lockout check still runs first), throttle denials burn no per-bundle strike and are NOT recorded so they cannot roll the window forward, unattributable clients share the stricter <unattributed> bucket, and the budget counts attempts rather than failures. Config keys and their semantics are unchanged, so the rig's 500 override stays valid. Three deliberate trades, documented in both Transport docs: - FAILS OPEN. A store error logs a Warning naming the client and cap, and permits the attempt. This is a best-effort rate cap over an already authenticated, Administrator-gated surface — the availability of legitimate imports outranks the cap, and a DB blip must not make the import wizard unusable. Falling back to the process-local window was considered and rejected: it would still block a legitimate operator's 11th import. - Approximate. Count-then-insert is not atomic across nodes, so simultaneous attempts can overshoot by the number in flight — bounded, self-correcting within the window, and not worth provider-specific raw SQL. - Pruned opportunistically on the unlock path (CAS-claimed at most every 15 min per process, bounded to 1000 rows, retaining 2x the window, best-effort try/catch) rather than by a new background service — Transport owns no central scheduler. Same shape as DeploymentService's terminal-record sweep. Tests: Transport.IntegrationTests 116 passed (+10 — two limiter instances over one store enforcing a single combined window, restart durability, denials burning no budget, sliding release, bucket isolation, store-failure fail-open, caller-cancellation not swallowed, no-shared-window fallback, prune and its negative control); ConfigurationDatabase 395 passed (+5). |
||
|
Joseph Doherty
|
372ea4df1b |
feat(transport): observability for the import-session cap (register row 53)
Nothing counted open import sessions and nothing logged a rejection at MaxConcurrentImportSessions (default 8), so the register-row-43 session leak could only be inferred from rotating test failures. - Gauge scadabridge.central.transport.import_sessions.open on the existing ZB.MOM.WW.ScadaBridge meter — already in SiteServiceRegistration.ObservedMeters (the ZbTelemetryOptions.Meters allowlist), so no telemetry config change is needed and the series is not silently dropped. Provider-backed, mirroring the StoreAndForward queue-depth gauge: registered by BundleSessionEvictionService .StartAsync (Transport's only lifecycle seam) and identity-checked-cleared on StopAsync so a late stop cannot clobber a newer registrant. - IBundleSessionStore.OpenSessionCount counts only UNEXPIRED sessions. The raw dictionary count would report the node as pinned at the cap for up to a one-minute sweep interval after a burst had drained — the exact false positive the instrument exists to rule out. - BundleSessionStore.Open logs Warning with open count, cap and TTL when it rejects. Fires only on a real cap hit: a store full of expired sessions is reclaimed first and logs nothing. - Docs: docs/components/Transport.md + docs/requirements/Component-Transport.md. Tests: Transport 173 passed (+6 new — Warning content, two negative controls, unexpired-only counting, gauge register/clear, no-clobber-on-late-stop). |
||
|
Joseph Doherty
|
09108018f1 |
fix(management): audit-config query returned {} for every filter — project the repo ValueTuple into a named result record
Deferred-work register row 46. HandleQueryAuditLog returned ICentralUiRepository's
(IReadOnlyList<AuditLogEntry> Entries, int TotalCount) ValueTuple straight to
ManagementActor.SerializeResult. System.Text.Json writes properties only, and a
ValueTuple exposes Item1/Item2 as public FIELDS, so the whole payload serialized to
the literal "{}" — the CLI faithfully printed an empty object no matter how many
AuditLogEntries rows matched, with no error anywhere in the stack.
Fixed at the handler (the layer that leaks the tuple), not the CLI: new Commons record
AuditLogQueryResult(Entries, TotalCount), mirroring the SecuredWriteListResult shape
already used by the other paged management query.
Also guards the whole class of bug: SerializeResult now throws on any ITuple result,
so the next handler that returns a tuple fails loudly instead of silently emitting {}.
Tests (fail-before verified — the query test asserted exactly "{}"):
- QueryAuditLogCommand_ReturnsEntriesAndTotalCount
- SerializeResult_TupleResult_ThrowsInsteadOfEmittingEmptyObject
- SerializeResult_AuditLogQueryResult_EmitsEntriesAndTotalCount
Docs: Component-CLI.md + CLI README document the response shape and why it is a record.
|
||
|
Joseph Doherty
|
b449fe201a |
fix(transport): curate BundleLockedException on the management surface (register row 51)
ManagementService references Commons but not Transport, so MapFault could not see BundleLockedException and the per-bundle 3-strike lockout reached the CLI as "An internal error occurred (CorrelationId=...)" — the operator got a correlation id instead of the reason, and no hint that further attempts on that bundle would be refused. Applies the pattern register row 44 established for BundleUnlockThrottledException: - Relocate BundleLockedException from Transport/Import/ to Commons/Types/Transport/, beside BundleUnlockThrottledException. Both consumers (BundleImporter, CentralUI TransportImport) already carried the Commons.Types.Transport using, so no call-site edits were needed. - Add curated catch arms in ManagementActor.HandlePreviewBundle and HandleImportBundle that rethrow as ManagementCommandException, so the message survives MapFault's disclosure filter. - Mirror the row-44 throttle fault-mapping test for both handlers. - Document the relocation in docs/components/Transport.md and docs/requirements/Component-Transport.md. Tests: ManagementService 499 passed (+2 new), Transport 167 passed. |
||
|
Joseph Doherty
|
5a1b4e30e6 | Merge branch 'transport-session-fixes' — abandoned-session release + per-IP unlock throttle enforced (register rows 43, 44) | ||
|
Joseph Doherty
|
5ab26a4233 |
fix(transport): enforce the documented per-IP bundle-unlock throttle (row 44)
BundleUnlockRateLimiter was registered (ServiceCollectionExtensions), injected into BundleImporter, unit-tested, documented on TransportOptions.MaxUnlockAttemptsPerIpPerHour = 10 and pinned in appsettings.Central.json — but _unlockRateLimiter was never invoked. A brute-force passphrase attacker against a stolen bundle was bounded only by the per-bundle 3-strike lockout, which a re-export side-steps (new ContentHash). LoadAsync now registers every unlock attempt against the limiter. Ordering is load-bearing: AFTER the per-bundle lockout check (hammering a locked bundle spends no hourly budget) and BEFORE the decrypt (so a denial burns no per-bundle strike — otherwise the throttle would be a way to lock a bundle someone is legitimately importing). Over budget throws the new BundleUnlockThrottledException, fail-closed with a clear message on both surfaces; it lives in Commons because the management surface (Commons-only references) must catch it to return a curated error rather than MapFault's generic internal-error text. Client key plumbing: - Central UI: new scoped CircuitClientAddressProvider snapshots Connection.RemoteIpAddress at circuit establishment — the same constraint and the same pattern as CookieAuthenticationStateProvider, since HttpContext is null by the time a circuit submits a passphrase. - CLI/management: additive ManagementEnvelope.ClientIp, stamped by ManagementEndpoints and threaded through DispatchCommand to the preview/import bundle handlers. - No attributable address => a single SHARED "<unattributed>" bucket, not a bypass: unknown clients get a stricter budget, never none. Per-client isolation is only real behind a ForwardedHeaders-trusted proxy, the same caveat the login throttle carries (Component-Security.md). Docs: Component-Transport.md + docs/components/Transport.md record the two-level ordering, the client-key sourcing, the shared-bucket fallback and a troubleshooting entry for the new error. Tests: 6 importer tests (cap enforced; denial != passphrase failure, no strike burned; per-client isolation; shared unattributed bucket; unencrypted bundles consume no budget; a locked bundle spends no hourly budget) — 4 of the 6 fail with the limiter call disabled, i.e. against the dead-code state. Plus ManagementActor tests pinning the ClientIp forwarding and the curated throttle error. |
||
|
Joseph Doherty
|
9e7697359e |
fix(outbox): resolve the delivery adapter per delivery, not per actor lifetime
Register row 45. NotificationOutboxActor.ResolveAdapters() cached adapter instances in an actor-lifetime IServiceScope (_adaptersScope), but both EmailNotificationDeliveryAdapter and SmsNotificationDeliveryAdapter are AddScoped and hold a scoped INotificationRepository over one EF DbContext. MaxParallelDeliveries (default 4) then drove up to four concurrent deliveries through that single adapter and single context, which central-b logged continuously on the rig as "InvalidOperationException: A second operation was started on this context instance before a previous operation completed". The collided delivery's row is never written and the wasted attempt counts against its retry budget, so sustained contention could park a legitimate notification. The per-delivery scope DeliverGatedAsync already opened for the outbox repository — added for exactly this hazard — did not cover the adapter. _adaptersCache, _adaptersScope and ResolveAdapters() are deleted. Delivery adapters are now resolved inside the per-delivery scope via a static ResolveAdapter(IServiceProvider, NotificationType), preserving DI's last-wins resolution semantics (the enumeration runs to completion rather than short-circuiting) and the null-adapter park path. Nothing shared is lost: the Email adapter builds a fresh SMTP client per attempt from an injected factory and the SMS adapter draws pooled connections from the singleton IHttpClientFactory, so no adapter carried state worth preserving across deliveries. The Central UI's NotificationChannelCatalog is an independent consumer of the same scoped registrations and is untouched, so the adapter-gated Type selector still enumerates exactly the deliverable channels. Tests: Dispatch_ResolvesAdaptersOnce_AcrossMultipleSweeps (which pinned the superseded caching behaviour) is replaced by ParallelDeliveries_EachGetTheirOwnAdapterAndDbContext — two parallel deliveries through a scoped repository whose simulated DbContext throws EF's exact second-operation exception on overlap, with a rendezvous that guarantees the overlap window — and Dispatch_ResolvesAdapterPerDelivery_NotOncePerActorLifetime. Both fail against the pre-fix actor (stash-verified: 1 of 2 rows persisted; 1 distinct adapter instead of 4) and pass after. Parallel-delivery ordering/throughput semantics are unchanged. Docs: Component-NotificationOutbox.md gains the scoped-adapter contract under Delivery Adapters (resolve per delivery; adapters carry no cross-delivery state; genuinely shared resources belong in a singleton/factory), and the NotificationOutbox-006 finding is marked SUPERSEDED with the reasoning. Solution build clean; NotificationOutbox (153), AuditLog (383), integration NotificationOutbox flow (2) and CentralUI notification (14) suites green. |
||
|
Joseph Doherty
|
708389829b |
fix(transport): release abandoned BundleSessions instead of leaking a slot (row 43)
Only BundleImporter.ApplyAsync released an import session, so every path that opened one and did NOT apply held one of MaxConcurrentImportSessions (8) — and the DECRYPTED bundle content that T-007 deliberately zeroes on the apply paths — for the full BundleSessionTtlMinutes (30). Eight abandonments locked out all imports on the active node; observed live (R6: 3 back-to-back suite runs held 6 of 8 slots until central-b was restarted). New IBundleImporter.Abandon(sessionId): the same zero-then-remove release ApplyAsync performs, idempotent (double release / unknown id / expired id are no-ops) and only ever given the random-GUID id its caller opened, so it can never evict another circuit's session. Wired into every abandonment path: - Central UI wizard — ReleaseSession() from new upload, TryLoadAsync (before it overwrites _session), Back-to-Upload / ResetSessionState, the 3-strike lockout reset, and Dispose (navigation away / circuit teardown). - CLI transport preview — a preview never applies, so nothing released it at all; now released in a finally. - CLI transport import — covers the pre-apply aborts (blocker rows, unresolved site/connection mappings). Class doc's "an abandoned wizard does not leak" claim is true again (it only ever covered the staged plaintext temp file). Component-Transport.md + docs/components/Transport.md now record the explicit-release contract and that BundleSessionEvictionService is a TTL backstop, not an abandonment path. Tests: 4 bUnit tests over a REAL BundleSessionStore (dispose / back-to-upload / apply-then-dispose idempotency / concurrent-circuit isolation) — 3 of the 4 fail against the pre-fix page; 4 importer tests (release + zeroing, idempotency, cross-session isolation, and the slot-cap regression). |
||
|
Joseph Doherty
|
c75c79f8ab | Merge branch 'grpc-event-batching' — additive site-stream event batching, negotiated, 100ev/25ms window (residual #3 / R2) | ||
|
Joseph Doherty
|
e6842c108a |
fix(comm): stop the duplicate-stream replacement disposing another handler's CTS
Root-caused from a full-suite-load-only failure of GrpcStreamIntegrationTests.Pipeline_DuplicateCorrelationId_ReplacesStream: ObjectDisposedException escaping SubscribeInstance. SubscribeInstance's duplicate-prevention path cancelled AND disposed the replaced stream's CancellationTokenSource. That CTS belongs to the replaced handler's own `using var streamCts`, which is still running and still has to read `streamCts.Token` — the Dispose raced that read. The race is pre-existing and independent of R2: the same Dispose sat against the same first-token-read when the handler still used `ReadAllAsync(streamCts.Token)`; it only ever loses under enough scheduling pressure to land the replacement inside the first stream's setup window. Cancel only. The owning handler's `using` still disposes it exactly once on every exit path, and cancellation is all replacement ever needed. The Cancel is wrapped for the converse race (owner already finished and disposed), mirroring CancelAllStreams(). Regression test makes the race deterministic by gating the first stream inside its setup window — the _activeStreams entry is registered before Subscribe is called, so the replacement always lands before the first stream reads its token. Verified fail-before (ObjectDisposedException) / pass-after by reinstating the Dispose as a negative control. |
||
|
Joseph Doherty
|
7b3e455c25 | Merge branch 'localdb-per-table-snapshot' — LocalDb 0.3.0 pin: per-table needs_snapshot baselining (residual #2 / R1) | ||
|
Joseph Doherty
|
a5ac309a94 |
chore(deps): bump ZB.MOM.WW.LocalDb to 0.3.0 — per-table snapshot resync
LocalDb 0.3.0 (scadaproj branch localdb-per-table-snapshot) narrows the snapshot-resync flag from one per-database bit to one per table, closing the residual 0.2.1 explicitly deferred. On-disk bookkeeping schema goes to v3 (__localdb_snapshot_state, upgraded in place on open); the wire stays compatible with 0.1.x/0.2.x, negotiated by capability rather than by a lib_schema_version bump — which is not available, since the handshake compares that field fail-closed for equality. SiteLocalDbSetup needed no restructuring: it already calls RegisterReplicated once per table, which is exactly what per-table flagging keys off. The comment now records that the loop shape is load-bearing, and what it buys — adding an eleventh table to ReplicatedTables on an already-replicating site snapshots that one table, where through 0.2.x the same edit re-streamed all eleven in full in both directions. First boot is unchanged (all ten seed at once, so the flagged set is every registered table and the library sends an ordinary full snapshot), and upgrading the rig in place is a no-op (the ten are already ledgered, so nothing seeds and nothing is flagged). Suites: Host 490/490, SiteRuntime 604/604, StoreAndForward 134/134, SiteEventLogging 76/76. Solution build clean. topology-guide + CLAUDE.md LocalDb bullet updated; the umbrella scadaproj CLAUDE.md travels in that repo's commit. |
||
|
Joseph Doherty
|
9b5cb3dd9d |
feat(comm): batch site→central stream events over gRPC (R2)
Every AttributeValueChanged/AlarmStateChanged rode its own gRPC message on
SiteStreamService; at target scale that is ~37.5k messages/s/site of pure
framing overhead. Coalesce them, additively, with no new RPC.
Wire (sitestream.proto, regenerated via docker/regen-proto.sh sitestream):
InstanceStreamRequest.batching_supported = 3
SiteStreamRequest.batching_supported = 2
SiteStreamEvent.batch = 4 (new oneof case)
SiteStreamEventBatch { repeated SiteStreamEvent events = 1 }
The proto3 default of batching_supported IS the negotiation, and it is
load-bearing: a batch frame reaches a pre-R2 central as EventOneofCase.None,
whose ConvertToDomainEvent returns null — the whole batch would vanish with
no error anywhere. An old central cannot set the flag so it never receives
one; an old site ignores the unknown request field and keeps sending
per-event frames, which the new client's ForEachEvent handles as the
single-event case. Both skew directions are covered by tests that go through
a real proto serialize/parse round-trip.
Server: SiteStreamEventBatcher, a per-subscriber pump replacing the handler's
await-foreach/WriteAsync loop and byte-identical to it at a cap of 1. It
never delays a lone event — it drains the already-queued backlog for free and
lingers only once a backlog is proven — emits a single-event buffer as a
plain frame, preserves order and per-event Timestamps exactly, and flushes
what is buffered when the send channel's writer completes. It sits DOWNSTREAM
of StreamRelayActor's bounded DropOldest channel, so it changes framing only
and does not move the burst ceiling (deferred register row 31, which lives in
the shared publish stage upstream of the BroadcastHub).
Client: sets the flag on both subscriptions and unpacks in order into the
existing per-event pipeline, so SiteAlarmAggregatorActor,
DebugStreamBridgeActor, consumer-keepalive/orphan logic,
reconnect-on-graceful-completion, generation fencing, the (siteId, endpoint)
factory key and IsLive semantics are untouched.
Options (validated): GrpcStreamBatchMaxEvents 100 (1 disables),
GrpcStreamBatchWindow 25 ms — validated strictly under 250 ms, the load
test's end-to-end P99 threshold. Measured worst case (trickle-with-backlog,
window-bound rather than cap-bound): P50 13.8 ms, P99 25.4 ms, max 25.8 ms;
cap-bound case sent 600 queued events in 6 frames.
Telemetry: histogram scadabridge.site.stream.batch_size tagged by stream
kind, recorded only on negotiated streams (per-event otherwise). It rides
ScadaBridgeTelemetry.MeterName, already in the ObservedMeters allowlist.
Docs: Component-Communication.md gains an Event Batching section; CLAUDE.md's
gRPC streaming bullet records the wire shape and the negotiation rationale.
|
||
|
Joseph Doherty
|
87f244508a | Merge branch 'deployments-server-paging' — Deployments page server-side paging + status counts (residual #4 / R3) | ||
|
Joseph Doherty
|
35ce14138c |
feat(centralui): server-side paging + server-computed status counts on Deployments (R3)
The Deployment Status page client-materialized the whole deployment list. It read EVERY DeploymentRecord — an insert-only table, one row per deploy attempt for the retention window — plus EVERY Instance, then site-scoped, sorted, counted the four status tiles and sliced a 25-row page in the Blazor circuit's memory. That ran on first render AND on every IDeploymentStatusNotifier push, so the cost scaled with the age of the system rather than the size of the page. All four jobs move into SQL: - `IDeploymentManagerRepository.QueryDeploymentListPageAsync(filter, page, size)` returns one page of `DeploymentListRow` — DeploymentRecord INNER JOINed to Instance, so the instance display name and site travel with the rows that need them — plus the total count of the filtered set. The join is exact: the FK is Restrict and DeleteInstanceAsync removes the records first, so no orphan exists. - `GetDeploymentStatusCountsAsync(filter)` returns the tile counts from ONE grouped aggregation, deliberately ignoring the filter's Status: the tiles are the status BREAKDOWN of the filtered set, so honouring it would zero three of four tiles the moment an operator clicked one. - Site scoping runs in the query as `SiteIdScope` resolved through the record's instance (DeploymentRecord has no SiteId of its own). An EMPTY grant stays a real filter matching nothing, never "unconstrained". - The now-callerless whole-table `GetAllDeploymentRecordsAsync` is deleted. OFFSET paging, not the Audit Log's keyset cursor, and deliberately so: this page's pager is numbered and jump-to-any-page, so it needs a page count, which only a total can give it — a keyset cursor can express neither, and the total is required for the tiles regardless. The deep-offset cost that pushes high-volume tables to keyset is bounded here by the terminal-record retention purge, unlike the 365-day AuditLog. This mirrors the Notification Outbox, offset-paged for the same reason. Ordering is DeployedAt DESC, Id DESC — the Id tie-break is load-bearing, because DeployedAt ties on rapid redeploys and an unstable sort key makes offset paging repeat or drop rows. UI: the four status tiles become the status filter (click to apply, click again to clear, aria-pressed, phrasing-only content so a <button> stays valid), plus a free-text search matched DB-side against instance name, deployment id, revision hash and initiating user. Search is TRAILING-edge debounced at 500ms — the same Timer + lock + _disposed idiom as the existing leading-edge push coalescer, minus the leading edge, because a search box must not query on the first keystroke. A filter change resets to page 1; a page past the end falls back to the last real page. Bootstrap only, existing PagerWindow pager retained. The WP2.4 push coalescing is unchanged and still earns its keep: server paging shrank what a reload costs, not how many arrive — it now bounds database round-trips rather than table scans. Tests: 19 new SQLite repository tests (paging slice + total, tie-break stability across pages, page/size clamping, past-the-end, the joined projection, every filter dimension incl. the empty-scope security case, and the grouped counts' status-blind contract); 15 new bUnit page tests (page-1 request, server total drives the pager, Next re-queries, tiles show server counts not page counts, tile filter + toggle, system-wide vs site-scoped scope push, debounce collapses a keystroke burst to one query, clear-filters, dispose with an armed timer). The two existing Deployments suites re-point their reload assertions at the new query. Doc: Component-CentralUI.md Deployment section — the "no server-side paging" known residual is replaced by the shipped design. |
||
|
Joseph Doherty
|
9d2834e30a |
docs+log(siteeventlogging): explain the site_events purge oplog-backlog burst (R7)
The daily site_events retention purge (and the storage-cap trim) is CDC-captured on a replication-enabled site node exactly like any other write — correct by design, since LocalDb Phase 2 deliberately has no purge-exemption path — so the backlog jumps by the deleted batch size at purge time. LocalDbOplogBacklog / localdb_oplog_depth spike, drain, and an operator watching the gauge with no context reads it as a replication fault. Documentation + one log line, no behaviour change: - topology-guide.md gains "Reading the replication backlog — the daily site_events purge burst": when it fires (PurgeInterval 24h, anchored to the active node's PROCESS START, not a wall-clock hour, so it moves after every failover), where it shows (replicated nodes only — not rig site-b/site-c), the healthy signature (LocalDbReplicationConnected stays true, backlog returns to ~0) and what a genuine fault looks like instead. - Component-SiteEventLogging.md Storage records the same under retention/purge; Component-HealthMonitoring.md gains the two previously-undocumented LocalDbReplicationConnected / LocalDbOplogBacklog metric rows carrying the caveat, with cross-references both ways. - EventLogPurgeService emits one Information line naming the row count and the expected transient backlog when a purge deleted rows on a replication-enabled node, so the spike is correlatable in the log. Replication-awareness comes in as a Host-supplied SiteEventLogReplicationCheck delegate, mirroring the existing SiteEventLogActiveNodeCheck seam: SiteLocalDbSetup.ReplicationIsConfigured goes internal so the PeerAddress-OR-ApiKey rule stays in one place and SiteEventLogging never learns to read LocalDb config. Unregistered ⇒ no note, matching the default that replication is opt-in and off. Both delete paths carry the note (a cap trim is usually the larger burst); the predicate is try/caught since a log-wording check must never break the purge. Tests: 5 new EventLogPurgeServiceTests cases (replicated logs it, unreplicated does not, zero-rows does not, cap purge logs it, throwing predicate still purges and swallows) via a local capturing ILogger. SiteEventLogging 81/81 green, Host 490/490 green, full solution build clean (0 warnings). |
||
|
Joseph Doherty
|
491df111ea |
fix(dcl): derive tag-resolution health counts from per-tag authoritative state
Closes arch-review remediation residual #1 (DCL unsubscribe-during-reconnect
count staleness).
DataConnectionActor tracked TotalSubscribedTags/ResolvedTags as two int fields
incremented and decremented at five independent sites. ReSubscribeAll clears the
very maps those decrements key off (_subscriptionIds, _unresolvedTags) while
deliberately preserving _subscriptionsByInstance, so an unsubscribe landing
inside a reconnect window matched NEITHER decrement branch: the total leaked +1
per subscribe/reconnect/unsubscribe churn cycle, permanently and cumulatively.
The
|
||
|
Joseph Doherty
|
a9ca51e008 | Merge branch 'worktree-agent-a465fb3cd6ec48cd3' into arch-review-remediation | ||
|
Joseph Doherty
|
fd5e023d08 |
fix(comms): review findings — consumer-based debug orphan net, foreign-cancel triad, honest onConnected, served-row-exact retirement, full-rate reconcile
F1 (HIGH) DebugStreamBridgeActor: the 5-minute orphan net measured the MAILBOX (SetReceiveTimeout), and once stream events were correctly marked INotInfluenceReceiveTimeout nothing recurring reset it — the snapshot lands once and GrpcStreamStable once — so every healthy session self-terminated at ~6 min with a false "Site disconnected". Replaced with a periodic self-tick (ConsumerLivenessCheckInterval, 30s) over a consumer-last-seen stamp renewed only by DebugStreamConsumerAlive, which DebugStreamService Tells on a shared timer to every session still in its registry (holding a session there IS "a consumer is attached" — both the Blazor view and the SignalR hub release it on dispose/disconnect, and it works headless). Reverting the wrapper was rejected: it would restore the quiet-instance orphan bug. F2 (MED) SiteStreamGrpcClient: the RpcException(Cancelled) filter now requires cts.IsCancellationRequested. A peer-originated / channel-dispose Cancelled fired none of onError/onCompleted/onConnected, leaving SiteAlarmAggregatorActor with _streamDown=false forever (IsLive stuck true, reconcile reopen guard never fired). F3 (MED) SiteStreamGrpcClient: a header TIMEOUT is no longer reported as connected — that shape is exactly what an unreachable site produces, and it cleared _streamDown, consumed _seedOnConnect and launched a full snapshot fan-out at a dead site. AwaitHeadersAsync returns bool; the first received event is the fallback connected signal, fired at most once from headers OR first event. F4 (LOW-MED) SqliteAuditWriter.MarkReconciledUpToAsync: the blanket below-cursor UPDATE retired late-stamped inserts that were never served (then age-purged — silent loss). The flip is now bounded by insertion order: a Pending row retires only if its rowid is at or below the high-water mark of rows this instance has served from ReadPendingSinceAsync (clamped on purge, since SQLite reuses rowids); Forwarded rows are exempt (central ACKed them over the push path). At-least-once is unchanged. F5 (LOW) Documented the liveness dependency (a served row never covered by a later cursor stays Pending forever; PurgeExpiredAsync never purges Pending) in ISiteAuditQueue + Component-AuditLog.md, and added a cheap site-health signal: SiteAuditBacklogReporter logs a rate-limited warning when the existing oldest-pending metric exceeds 24h. F6 (MED) SiteAlarmAggregatorActor: _fanoutSinceLastTick was armed by the reconcile's OWN fan-out, so steady state ran fan-out→skip→fan-out→skip — one reconcile per 2x interval (120s), halving the not-reporting refresh and the alarm reconcile backstop. The skip is now armed only by connect/failover-driven seeds (initial, _seedOnConnect, and a re-seed queued behind one). Tests: Communication.Tests 691 passed (+13), AuditLog.Tests 382 passed (+5). |
||
|
Joseph Doherty
|
11abda1d1f | Merge branch 'worktree-agent-aaec6546913f0beae' into arch-review-remediation | ||
|
Joseph Doherty
|
e0e4b24679 |
fix(deploy+cli): review findings — honest CLI timeouts, watermark-complete staleness, phase-2 staging, lock-safe cancellation
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.
|
||
|
Joseph Doherty
|
09e350ab0d | Merge branch 'worktree-agent-adf34e265d2dcae96' into arch-review-remediation | ||
|
Joseph Doherty
|
5d075f1374 |
fix(central): review findings — no client-side audit truncation, insert-first upsert, QI-safe scripts, honest operator replies
Six adversarial-review findings in the central SQL/ingest layer. F1 (AuditLogRepository.InsertChunkAsync) — the set-based ingest declared each string parameter at its COLUMN width (Actor/Target 256, Action 64, Outcome 16, Category 32, SourceNode 64), so SqlClient truncated an over-long value at bind time and committed the mutilated row — silent, in an append-only store, with no PayloadTruncated flag — while the per-row and reconciliation paths sent the same value in full and let the server reject it with 2628. Bind at the value's own length instead; explicit SqlDbType is kept (it fixes the VALUES constructor's derived column types and datetime2 precision). Design: reject everywhere, truncate nowhere — matching today's per-row behaviour. F2 (SiteCallAuditRepository.UpsertAsync) — the single-statement upsert ran the monotonic UPDATE first and INSERTed only if nothing matched. Two writers racing the first packet of one TrackedOperationId (the cached dual-write and the reconciliation pull carry DIFFERENT lifecycle states) both matched nothing, and the loser then skipped its INSERT or swallowed a 2627 — dropping its Status/RetryCount/HttpStatus/TerminalAtUtc. Legs swapped to `IF NOT EXISTS … INSERT; UPDATE <monotonic>` — still one round trip, and the loser's UPDATE now lands on the winner's row. The duplicate-key catch re-runs the monotonic UPDATE for the same reason. Moved to raw SQL with explicitly-typed parameters so the intricate rank predicate exists in exactly one place (an untyped DateTime would bind as `datetime` and round the freshness tiebreaker). F3 (docs/plans/sql/*.sql) — filtered-index DDL failed with error 1934 under the documented `docker exec … sqlcmd` path, which defaults QUOTED_IDENTIFIER OFF; once IX_Notifications_Delivered exists, QI-OFF DML on Notifications fails too. All four scripts now open with `SET QUOTED_IDENTIFIER ON; SET ANSI_NULLS ON; GO` (own batch, so it is in force when the next batch parses), and the migration convention in Component-ConfigurationDatabase.md documents `sqlcmd -I`. Verified live: the pre-fix script fails 1934 without -I, the fixed one applies. F4 (SiteCallAuditActor) — the off-mailbox reconciliation/purge passes reuse the injected repository, so tests drove one DbContext from the pass and a mailbox handler concurrently. Serialized at the CALL via a private SerializedRepository wrapper applied only by the test constructors, rather than running the pass on-mailbox: production keeps its PipeTo shape untouched, and the existing "a blocked drain does not stall ingest/query/KPI" regression tests stay meaningful (they would have been invalidated by suspending the mailbox). F5 (AuditLogIngestActor) — when the batch failed because the 20 s IngestBudget expired, the per-row fallback reused the same expired token: N instant failures, N counter bumps, zero accepted. The fallback now gets a fresh 5 s budget (inside the 30 s outer Ask), and a blown budget bumps the failure counter ONCE for the batch instead of once per row. F6 (NotificationOutboxRepository.UpdateAsync) — ExecuteUpdate's row count was discarded, so an operator Retry/Discard of a notification the retention purge had already deleted reported success (the pre-ExecuteUpdate code threw DbUpdateConcurrencyException). UpdateAsync now returns whether a row matched; the operator one-shots answer "notification not found" and emit no audit row for the action that did not happen, while the dispatcher logs a warning (its delivery already happened; nothing to retry). GetByIdAsync switched to AsNoTracking since the write is out-of-band. Tests: 5 new SQL-backed regressions (over-long Target rejected on both paths + boundary round-trip; concurrent first-write and already-created-by-another-writer upserts; vanished-row UpdateAsync), a token-identity pin on the ingest fallback, a repository-concurrency detector for the SiteCallAudit passes, and vanished-row operator-path tests. The F1/F2/F4 regressions were each confirmed failing against the pre-fix code. Suites: ConfigurationDatabase 369, AuditLog 378, SiteCallAudit 66, NotificationOutbox 152 — all green, solution builds with 0 warnings. |
||
|
Joseph Doherty
|
95f091cff8 | Merge branch 'worktree-agent-a6a0dffcb93fa5007' into arch-review-remediation | ||
|
Joseph Doherty
|
950c54c5fc | fix(runtime): review findings — recursion-safe run cap, atomic detach counter, summary edge cases, per-row event-log fallback | ||
|
Joseph Doherty
|
3913c6b2ac | Merge branch 'worktree-agent-a71052048b4ee11d4' into arch-review-remediation | ||
|
Joseph Doherty
|
56c99c92c3 |
fix(ops): wonder site config gains required audit DB path; explicit LocalDb read-page cap; rate-limited observer drop logging
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. |
||
|
Joseph Doherty
|
37f13e2eaa | fix(dcl): discard in-flight subscribe results for unsubscribed tags; release the orphaned handle | ||
|
Joseph Doherty
|
c254d0740e |
perf(sitelog): sampled per-run events; interval run summaries; site_events replication policy pinned
Implements WP3.2 stage (b) per docs/plans/2026-08-15-site-events-policy-design.md.
- Per-run instance-script Started/Completed Info site events are now off by
default (SiteRuntimeOptions.PerRunScriptEvents=false) instead of firing on
every run, closing the dominant site_events writer. Gated at the ScriptRunLauncher
call sites (moved there from ScriptExecutionActor by WP3.1). Error-level events
(timeout/failure/stuck-watchdog/recursion-limit) remain unconditional.
- ScriptRunSummaryRecorder accumulates per-(instance, script) run counters and a
new site-only ScriptRunSummaryFlushService emits one aggregate "script" Info
site event per ScriptRunSummaryIntervalSeconds (default 300s), top-50-script
breakdown with an "others" rollup, zero-activity intervals emit nothing.
- Per-script opt-in via PerRunScriptEventScripts ("Instance/Script" exact or
"Instance/*" wildcard), matched by the new pure ScriptRunEventPolicy. All three
options are read from IOptionsMonitor<SiteRuntimeOptions> per run, so the
policy is hot-togglable without a restart.
- Fixed the stale "event log is not replicated" comment at AkkaHostedService.cs
(~905): site_events IS registered in SiteLocalDbSetup.ReplicatedTables — the
singleton is what makes queries always hit the actively-written copy;
replication is what gives the singleton history to read after a failover
(memo Decision (b)). site_events replication itself is unchanged (still
registered) and already pinned by
tests/ZB.MOM.WW.ScadaBridge.Host.Tests/SiteLocalDbCdcRegistrationTests.cs.
- Updated Component-SiteEventLogging.md (Volume Policy section, corrected
Storage/replication rationale) and Component-SiteRuntime.md (Script Run
Launch + Error Handling sections).
|
||
|
Joseph Doherty
|
799fd041ec | Merge branch 'worktree-agent-a2b4268818a1b6201' into arch-review-remediation | ||
|
Joseph Doherty
|
c4fc1f8ecd | perf(runtime): split trigger evals from the blocking pool; bounded, deadline-aware execution | ||
|
Joseph Doherty
|
cca7f1786d | chore(deps): LocalDb 0.2.0 — dereg cleanup, late-opt-in baselining, byte-budget replication |