524bdbbe9023f8c38d5669c4547e52b0b2f9800d
1556 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Joseph Doherty
|
8f2c2813ea |
fix(siteruntime): self-healed and newly-enabled instances now get their DCL connections (register row 85)
An Instance Actor started by a self-heal came up with NO DCL connection actors, so every tag it subscribed was answered "Unknown connection: <name>" by the DCL manager indefinitely. Proven live 2026-08-16 by the row-78 close-out probe: the sweep correctly started the replication-seeded instances, then every subscribe failed for 6+ min until a second pair restart. Root cause is narrower than the register assumed. A DCL connection actor is never created from data_connection_definitions -- that table has no production reader at all (GetAllDataConnectionDefinitionsAsync is called only by a test). It exists only as a side effect of starting an instance, from the Connections map inside that instance's own flattened config, via EnsureDclConnections. That ensure was the CALLER's job and only two of the four callers made it: CreateInstanceActorBatch (boot) ensured OK ApplyDeployment (deploy) ensured OK StartStoredInstance MISSING <- row-67 ensure-started + row-78 sweep HandleEnableResult MISSING <- enabling a disabled instance The two misses are the two paths added after the original pair -- the signature of a convention living at call sites instead of in the funnel. It was never replication-specific: the boot load reads only ENABLED configs, so a node booting with an instance disabled never creates its connections either, and enabling it afterwards hit the identical dead end on an ordinary unreplicated site. Fix: EnsureDclConnections moves into CreateInstanceActor, already documented as the single Instance Actor creation funnel (it is where the running revision is stamped for the same reason), and the two caller-side calls are removed. Placed before the already-exists guard so the two paths that already called it keep their exact previous semantics. No new message, option or DCL change -- it reuses EnsureDclConnection's existing per-name hash guard, so steady state is silent. Structural rather than another guard: a fifth start path cannot forget. Deliberately NOT done, with rationale in the memo: a sweep pass over data_connection_definitions. It would not have fixed row 85 (that table is empty on a site that has only ever received instance deploys), and re-hashing from it would fight the deploy path over the same per-name hash slot, recreating connection actors and dropping live subscriptions on a loop. Two residual sibling gaps (connection-only artifact deploy replicating to a peer that never adopts it, and the mirror-image removal case) are recorded in the memo and the component docs as open, not silently skipped. Tests: SelfHealCreatesDclConnectionTests, 7 tests asserting on the CreateConnectionCommand messages sent to a recording DCL stand-in. Verified fail-before against a behavioral revert (call removed from the funnel, restored at the two original sites): 4 fail / 3 pass, the 3 being steady-state silence plus two refactor guards that must pass on both sides. Docs: sweep design memo gains section 7; Component-SiteRuntime, Component-DataConnectionLayer, docs/components/SiteRuntime and docs/components/DataConnectionLayer updated (the deploy-flow step ordering in docs/components/SiteRuntime was stale as written). |
||
|
Joseph Doherty
|
f46cc19736 | Merge rows 78+72: DeploymentManager singleton instance sweep — starts replication-seeded enabled instances and converges stale revisions through the real HandleDeploy path; one repair attempt per (instance, revision) | ||
|
Joseph Doherty
|
4d39ad3cd7 |
Merge rows 80+73+71: nullable LocalDb health fields on unreplicated sites (Host supplies ReplicationIsConfigured), StartupReconcilerActive on the health report (proto field 29) with central No-self-heal badge, standby active-node ERR downgraded via narrowly-keyed Serilog filter
# Conflicts: # CLAUDE.md |
||
|
Joseph Doherty
|
9f418a6aea | Merge row 34: site_events purge burst measured (2.73M-row worst case = 2.7x MaxOplogRows) and paced — shared 250k/tick budget, 5-min catch-up, ceiling-hit counter | ||
|
Joseph Doherty
|
e606b24a55 |
fix(siteruntime): instance reconcile sweep closes config/runtime divergence (register rows 78, 72)
The site's deployed_configurations rows are the durable truth about what should be
running; the set of live Instance Actors is what IS running. Nothing kept the two in
agreement after boot, and nothing surfaced a disagreement.
Row 78 (availability, proven live 2026-08-15) -- deployed_configurations is one of
LocalDb's ten replicated tables, so on a replicated pair a config can appear in a
node's file with no local actor involved at all: the CDC sync applier writes the row
directly. Live sequence: site-a-b's reconcile fetched 4 configs at its solo boot; the
pair was then restarted with a fresh site-a-a file and snapshot replication seeded
site-a-a's table within ~5s -- AFTER its boot-time load had read an empty table, and
invisibly to its own reconcile (central rightly reported "0 fetched", because central's
view of the site was correct). Enabled instances sat dead with their configs present
until yet another restart. Row 67's EnsureInstanceStartedCommand only covers configs
the reconcile itself FETCHES; this is the same gap through a side door.
Row 72 (silent divergence) -- for a STALE ReconcileGapItem the pass writes the newer
config via StoreDeployedConfigIfNewerAsync, a pure file write, while the running
Instance Actor keeps executing the previously compiled config until the next restart
or redeploy. Nothing said "stored revision != running revision".
Both are closed by one periodic sweep owned by the Deployment Manager singleton
(ScadaBridge:SiteRuntime:InstanceSweepIntervalSeconds, default 60, validated >= 0;
0 disables). Each tick re-reads the durable rows off-thread and, on the actor thread,
for every ENABLED stored config either starts a missing Instance Actor (row 78) or --
when the running one's revision no longer matches the stored one -- re-applies the
stored config through the ordinary redeploy path (row 72).
A sweep was chosen over a LocalDb change hook deliberately: a hook covers only the
replication arrival path, and only on the half of the fleet running with replication
enabled, whereas the sweep is arrival-path-agnostic (replication seeding, post-boot
snapshot resync, a hand-restored file, any future path). It is also not available --
ILocalDb 0.3.0 exposes no change-notification or applied-rows callback, so a hook would
mean a new upstream capability in scadaproj plus an in-process bus into this singleton,
a cross-repo change to close a site-local gap.
Row 72's decision is to CONVERGE, not merely warn. The stored row is what central
successfully deployed; an actor serving the config central believes was replaced is the
actual fault, and a surface-only warning leaves the operator to issue by hand exactly
the redeploy the sweep would have performed. Reusing HandleDeploy means the compile
gate, warm-then-gate ordering, redeploy-supersede, the static-override / native-alarm
reset and all-or-nothing-per-instance are the production ones, not a second
implementation -- and a stop still does NOT cancel an in-flight script run, exactly as
for a central redeploy.
Guards, shared with row 67 via the extracted InstanceIsOwnedByADeployPipeline predicate
and the shared StartStoredInstance helper:
- a DISABLED instance is never started (config row, no actor is its correct state);
- the sweep defers to any deploy pipeline owning the instance (compile warm in flight,
predecessor still terminating, deployment awaiting its persistence/init join), so it
can never interleave with a real redeploy -- that window has the config stored and no
actor, which an unguarded sweep would answer by creating a child whose actor name is
still registered;
- inert until boot-time Instance Actor creation has finished, so a tick between
staggered batches does not "heal" instances the boot load is about to create;
- ONE repair attempt per (instance, revision) pair -- this is what stops a periodic
self-heal becoming a restart loop for an actor that dies during init, or for a stored
config the compile gate rejects (a rejection leaves the running instance untouched:
the gate returns before anything is stopped);
- reads never overlap, so a slow read cannot stack ticks into duplicate repairs.
A sweep that finds nothing is completely silent -- no log above Trace, no site event.
A repair emits the same Warning `deployment` site-event pattern as row 67 with the
trigger named ("deployment-manager sweep self-heal") so it is distinguishable from a
startup-reconcile heal; a stale-revision restart names BOTH revisions and a rejected
one adds an Error event. The synthetic redeploy replies to the singleton itself rather
than NoSender, because dead letters are a monitored site health metric.
CreateInstanceActor gains a revisionHash parameter and is the single creation funnel
that records it, so the stale check covers every path that can start an Instance Actor;
a missing entry reads as "unknown", never "stale", so a future path bypassing the funnel
degrades to no-op rather than to spurious restarts.
Fail-before by behavioral revert (sweep timer left unarmed): 5 of 10 new tests fail,
including both live symptoms -- the row-78 repro ("A replication-seeded config for an
enabled instance left no running Instance Actor") and the row-72 repro, whose snapshot
still shows the rev-1 value 111 while the store holds rev-2's 222. The 5 negative
guards (disabled / in-flight redeploy / matching revisions / silent-when-healthy /
interval-zero) are green in both states, as they must be.
SiteRuntime 626/626, Host 505/505, AuditLog 383/383.
Design note: docs/plans/2026-08-15-instance-sweep-self-heal-design.md
|
||
|
Joseph Doherty
|
d9a35846d9 |
perf(siteeventlog): pace the site_events purge burst — per-tick delete ceiling + accelerated catch-up (row 34)
Measured the unbounded purge burst and found it can force a full ten-table
snapshot resync, then bounded it. No purge-exemption path was added — the
deletes are still CDC-captured like any other write, which stays ruled out
by design; what changed is how many a single tick performs.
MEASURED (harness: real EventLogPurgeService over a real LocalDb file with
site_events genuinely RegisterReplicated, plus a two-node loopback gRPC
replication pair; docker rig untouched):
393 bytes/row incl. indexes => a 1 GB event log holds ~2,731,947 rows.
Expiring all of them in ONE tick: 285 s, 2,731,947 oplog rows,
284 MB oplog payload, 2.73 M row-version tombstones (7-day retention),
i.e. 2.7x LocalDb's MaxOplogRows default of 1,000,000. Crossing that makes
the 60 s maintenance tick prune the oplog and set needs_snapshot — a FULL
snapshot of all ten replicated tables, bought by a routine retention purge.
The resident backlog is produced-minus-drained, so a healthy peer blunts it:
on loopback the same burst peaked at 391,424 and cleared in 6.0 min. That is
the problem, not the reassurance — the old safety margin was a function of
peer throughput and vanished with the peer down. A bounded tick is safe at
any peer speed.
Drain is not a bottleneck: 7.6k-19.8k tombstones/s (20k / 500k / 2.73M runs).
SHIPPED:
- SiteEventLogOptions.MaxPurgedRowsPerTick (default 250,000; 0 = unlimited
escape hatch, negatives rejected). One SHARED budget across the retention
purge and the cap trim — two independently-capped phases would let a tick
emit twice the intended burst. 4x margin under MaxOplogRows, which is also
shared with every other replicated table's ordinary traffic.
- SiteEventLogOptions.PurgeCatchUpInterval (default 5 min, validated > 0 and
<= PurgeInterval). Without it pacing would make retention converge in 11
DAYS at the daily cadence; with it the worst case converges in ~11 ticks
(~55 min), every burst bounded. 5 min rather than 1 so ticks cannot outrun
the peer's drain and re-accumulate the backlog.
- A ceiling is claimed only when work genuinely remains (bounded EXISTS probe),
so an expired set landing exactly on the budget does not arm a no-op retry.
- Visibility: rate-limited (15 min) Warning naming the ceiling and the
accelerated interval, plus an unthrottled counter
scadabridge.site.event_log.purge_ceiling_hits — its SHAPE distinguishes a
backlog converging from a site whose event rate outruns one paced tick.
- The purge loop is a delay loop rather than a PeriodicTimer, so its cadence
now anchors to the END of the previous purge. Documented.
ROW-33 INTERPLAY VERIFIED: pacing lowers each spike and lengthens total purge
duration, but a full 250k tick drains in ~23 s (~4 min even at 10x a real
link's penalty) against the 15-minute sustained dwell and a 5-minute gap to
the next tick. The UNPACED 2.73M burst had only 2.5x of margin (6.0 min), so
pacing makes the badge assumption robust rather than merely preserving it.
Docs travel together: Component-SiteEventLogging.md, topology-guide.md
("Reading the replication backlog" + the row-33 badge note), CLAUDE.md's
purge-burst sentence, and a dated measurement/decision note under docs/plans/.
Fail-before: the new behavioural tests do not compile against the pre-fix
service (RunPurge was void); with PurgeBudget neutralised to always-unlimited,
6 of the new tests fail and all 19 pre-existing ones pass.
Tests: SiteEventLogging 93/93, Commons 715/715, HealthMonitoring 115/115,
Host 505/505. Solution build clean.
|
||
|
Joseph Doherty
|
8b761841bc |
fix(logging): row 71 — stop logging the standby's by-design active-node verdict at ERROR
A healthy standby logged "Health check active-node with status Unhealthy … 'not the site pair primary (standby)'" at Error on every probe — every 10s on a site node, and (discovered while fixing this) every 5s on a central standby, which Traefik health-probes at that cadence. Correct-by-design state at an alarming severity: it floods the log and trains operators to ignore ERR. Mechanism: DefaultHealthCheckService logs one line per check per probe and picks the level from the verdict — Debug/Warning/Error for Healthy/Degraded/Unhealthy. Both active-node checks answer Unhealthy on the standby because 503 IS the contract /health/active publishes. There is no IHealthCheckPublisher in this repo, so each line is one inbound HTTP probe. Fix: one Serilog exclusion filter in LoggerConfigurationFactory. A level override could not work — Serilog's MinimumLevel.Override only RAISES a per-source floor, and a LogEvent's Level is immutable, so neither an override nor an enricher can rewrite Error as Information. Changing the check to report Degraded was rejected: it would alter the HTTP status that /health/active publishes, which is load-bearing for Traefik and the family dashboard. The constraint (genuine failures must still log at Error) is preserved by keying the predicate on the check NAME rather than the logger category: localdb, akka-cluster, database and required-singletons keep their Error lines, and the active-node check has no error path of its own — the active node always answers Healthy — so the only event this can ever suppress is a standby reporting that it is a standby. The verdict stays fully observable via /health/active's own HTTP status, the readiness probe's cluster view, and the health dashboard. Also corrects a stale CLAUDE.md claim found while verifying what consumes this check: "/health/active is central-only (site nodes map no /health/* at all)" is wrong — SiteServiceRegistration registers akka-cluster + localdb + the role-scoped SitePairActiveNodeHealthCheck, and Program.cs's Site branch calls MapZbHealth() on the HTTP/1.1 listener. Fail-before evidence: with the filter behaviourally disabled, StandbyActiveNodeVerdict_IsNotLogged fails "Collection was not empty" while the other 9 tests in the file — the must-still-log constraints — pass either way, which is what proves the filter is narrow rather than a blanket suppression. Tests: Host 518 green (10 new). |
||
|
Joseph Doherty
|
a2f0086fa1 |
fix(health): rows 80+73 — unconfigured replication reports null, reconciler presence surfaced
ROW 80 (observability correctness). An unreplicated site published
LocalDbReplicationConnected=false / LocalDbOplogBacklog=0, contradicting the
nullable semantics every doc and the proto already state ("null means no
reading, NOT disconnected with an empty backlog").
Root cause: the replication engine is registered unconditionally and idles
without a peer, so ISyncStatus answers false/0 on a node that was never meant to
replicate, and LocalDbReplicationStatusReporter passed that straight through.
Two consequences: an operator read connected:false on a healthy site as a fault,
and central's null branches — row 33's conditional KPI emission and
sustained-badge anchor exclusion — never received the null they exist for, so
rig site-b/site-c emitted a permanently misleading
localDbReplicationConnected=0 series.
Fix mirrors the SiteEventLogReplicationCheck precedent: the Host evaluates
SiteLocalDbSetup.ReplicationIsConfigured ONCE and passes it to
AddLocalDbReplicationHealthBridge as a third provider; unconfigured
short-circuits the probe to (null, null). HealthMonitoring never reads LocalDb
config itself, and the same captured value now also feeds
SiteEventLogReplicationCheck so the two surfaces cannot disagree.
ISiteHealthCollector.SetLocalDbReplicationStatus widens to bool? — the
non-nullable bool was the type-level root cause (null was inexpressible).
false now means exactly one thing: configured, peer session down = a real fault.
Wire/central/UI needed no change: the proto already carries BoolValue/Int64Value
wrappers and the aggregator + KPI source already branch on null. This makes code
match doc, so no doc claim was weakened.
Watch the trap that caused this: configured-ness is PeerAddress OR ApiKey (only
the dialling half sets a peer address). The pre-existing SiteLocalDbWiringTests
fixture sets ApiKey and IS a replicating node, so its false/0 assertion is
correct and is kept, with a comment saying so; the new
UnreplicatedSiteLocalDbHealthTests fixture is the genuinely-unreplicated one.
ROW 73 (silent loss of self-heal). AkkaHostedService skips
SiteReconciliationActor when no IDeploymentConfigFetcher is resolvable, and that
skip was a lone boot Warning — a node with no self-heal path at all went on
reporting itself perfectly healthy forever.
Adds tri-state SiteHealthReport.StartupReconcilerActive (null = not reported,
i.e. the central self-report or a pre-row-73 site build; true = running;
false = booted without it), latched once at boot by the Host in both branches.
Additive proto field 29 (BoolValue, vendored C# regenerated via
docker/regen-proto.sh centralcontrol; csproj restored, verified empty diff).
The skip branch also raises its log to Error — no configuration makes the
fetcher legitimately absent on a site node — and writes a best-effort
deployment/Error site event so the evidence outlives the boot log.
Central treatment: a "No self-heal" WARNING badge on the site tile, not a
failure. Config still reaches the node via LocalDb replication while it is up;
what it lost is recovery from a deploy that landed while it was down — degraded,
not offline. Only the explicit false is badged (badging null would light up the
whole fleet the moment central is upgraded ahead of the sites), and offline
sites are skipped, matching the replication-backlog badge.
Fail-before evidence: with the row-80 gate behaviourally reverted,
UnreplicatedSite_HealthReport_ReportsNullForBothLocalDbFields fails
"Expected: null / Actual: False" and 3 of the 8 new reporter unit tests fail.
Tests: HealthMonitoring 128, Host 518, CentralUI 1051, Communication 717,
Commons 715, SiteEventLogging 81 — all green.
|
||
|
Joseph Doherty
|
40944a0d04 |
feat(centralui): row 75 — Approve is caution amber, not destructive red
The Secured Writes page rendered Approve and Reject as IDENTICAL red buttons.
Row 28 fixed the labels but left the styling, and the styling was the worse
half: at the moment of a two-person-control decision the colour carried no
discriminating information at all, and the affirmative half of the control wore
the delete affordance.
Root cause is that `danger` was a two-state flag for a three-state concept, so a
consequential-but-not-destructive action had to choose between looking like a
delete and looking like the safe default. Adds the missing middle rung:
DialogSeverity { Normal -> btn-primary, Caution -> btn-warning,
Destructive -> btn-danger }
DECISION. Approve -> Caution (amber): approving executes a real write to live
plant equipment, so it is not a benign primary action, but it is not destroying
anything either. Reject KEEPS Destructive (red): it is terminal and irreversible
for the request — the Operator must resubmit — and holding it red is what keeps
the two opposite decisions visually distinct now that Approve is amber. Dropping
Reject to neutral was considered and rejected: it would invert the bias, making
declining look like the safe default. Colour is never the only signal — both
buttons carry their verb.
Only Destructive inherits the row-28 "Delete" label fallback; an unlabelled
Caution reads "Confirm", since a caution action is by definition not a delete —
otherwise row 28 would come back through the new door.
ADDITIVE OVERLOAD, deliberately. The boolean form stays the signature for all
~34 existing call sites, which are genuinely two-state, so nothing in src/
outside SecuredWrites.razor changed. Adding a fifth optional parameter to the
boolean form instead would have silently un-matched every existing four-argument
NSubstitute stub — the exact row-28 gotcha (null Task, caller NREs, no compile
error). The overload cannot: the only breakage was six hand-written IDialogService
fakes failing to compile, which is the loud failure mode. That gotcha did land
once, in SecuredWritesTests' own stub, and is now stubbed on both overloads with
a comment saying why.
DialogState's `bool Danger` became `DialogSeverity Severity` rather than gaining
a second field — it is read only by DialogHost, and one field cannot disagree
with itself.
Fail-before, both halves:
- Approve call site reverted to `danger: true` -> both new Approve pins fail.
- Caution remapped to btn-danger in DialogHost -> the amber styling pin fails.
Tests: DialogSeverityStylingTests renders the REAL DialogService behind a real
DialogHost (FailoverConfirmLabelTests' harness), so it pins what the user sees,
not what an argument was — all three severities, both label fallbacks, and the
boolean adapter. SecuredWritesTests pins that the page asks for Caution, with a
negative control that it did NOT also go through the boolean form (without which
the assertion would pass on a call site left at danger:true). Reject's red is
pinned too, so a later "make this page less alarming" pass cannot flatten both
to one colour.
No Playwright selectors needed updating — the suite has no Secured Writes
coverage, and no `.btn-danger` selector elsewhere targets this page.
Docs: Component-CentralUI.md gains the severity contract under Technology (next
to the row-28 label contract) plus the decision on the Secured Writes section.
CentralUI.Tests: 1055/1055 green.
|
||
|
Joseph Doherty
|
4ae57bdb6b |
docs(cli): row 76 — audit-config query JSON sample shows afterStateJson + bundleImportId
The `entries` array is a serialized `AuditLogEntry`, so both fields are always on the wire; the README sample showed neither, which read as if the payload were the seven columns the table projection happens to keep. Row 57 added that projection and the table section already names both omitted fields — the JSON sample above it was the stale half. Added both to the sample and documented their nullability, which is the part a script author actually needs: `afterStateJson` is the post-change snapshot embedded as a string and is null for deletes (no after-state), `bundleImportId` is the import session id when the change came from `transport import` and null for a direct change. Also grew the sample from one entry to two so it now mirrors the table-output sample directly below it row for row — the same 412 Template/Created and 413 ExternalSystem/Deleted pair — which is what makes the "the table drops these two fields" sentence checkable against the JSON above it. Docs only. |
||
|
Joseph Doherty
|
744ff36a7b | merge: startup reconcile starts the healed InstanceActor (register row 67) — EnsureInstanceStartedCommand to the DM singleton, idempotent, disable-wins, Warning site event on self-heal | ||
|
Joseph Doherty
|
e8a90f18dc | merge: external-system retry policy end-to-end (register row 27) — MaxRetries/RetryDelay now travel artifact→gRPC→site schema→S&F enqueue, CLI --max-retries/--retry-delay-seconds, ArtifactDiff compares them | ||
|
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 |