Tasks 4 and 5 of the plan land together: the card's state fields are write-only
until the markup consumes them, and the project treats unused-field warnings as
errors, so neither task builds on its own.
Claude-Session: https://claude.ai/code/session_01G2GHPnbbKGvyyfbr2XBRVH
Row 104: docker/vacuum-site-localdb.sh stops both nodes of a pair together, sets
auto_vacuum=INCREMENTAL + VACUUM on each node volume with integrity_check before/after,
then starts both (DRY_RUN=1 reports). Run on all three rig pairs: six files 0 -> 2, ok,
~11% smaller; site-a replication reconnected with backlog 0 and no watermark change —
VACUUM is replication-invisible (ledger keys by declared PK, oplog seq is INTEGER PRIMARY
KEY AUTOINCREMENT). Topology guide gains the recipe.
Row 105: SiteLocalDbReplicatedTableFkGuardTests builds the real site schema via OnReady
(unreplicated config so 0.3.3's own guard cannot mask the culprit) and asserts every
SiteLocalDbSetup.ReplicatedTables entry (now internal) exists and has no inbound
CASCADE/SET NULL/SET DEFAULT foreign key, with a negative control. The constraint is
enforced at build instead of at the first replicated boot.
Row 106 (found during 104's live verification, NOT caused by it — the identical 127k-row
full snapshot also landed at the earlier deploy restart): SnapshotStreamer.SendAsync stamps
as_of_seq = MAX(seq), which is 0 once a converged sender's oplog is ack-pruned to empty;
a zero-watermark receiver (rebuilt node) is never lifted, so the empty-oplog horizon rule
re-streams a full snapshot on every reconnect. Sender-side one-line fix for LocalDb 0.3.4:
as_of_seq = max(MAX(seq), last_acked_seq).
The T4 test asserted 'reflection Invoke returned AND the row is still present', but the
yielded continuation runs RunPurge on a pool thread and can delete the row before the
test thread reaches its assert — lost 2/2 full-suite sweeps on the integration branch,
green in isolation (row-40 class, introduced by this wave). The purge now blocks inside
the isActiveNode check (consulted before any delete) until the test releases it: fixed
code returns from Invoke while the gate is held; unfixed code blocks on the gate itself
and the bounded wait fails. Proof of failure re-run: with Task.Yield() removed the test
fails with the intended message after the 10 s bound; 3/3 green with it restored.
Cross-task regression caught by the combined sweep: TopologyLiveUpdatesTimerDisposeRaceTests
(T12) builds a real DeploymentService, whose constructor grew an IStaleInstanceProbe
parameter in T13 (P7-6 up-to-date pre-filter) — 4 DI-resolution failures on the
integration branch that neither task saw in isolation. Registered a substitute, matching
TopologyPageTests.
Bundle review Minor: ApiMethodForm/SharedScriptForm copied TemplateScriptDialog's sandbox
pattern exactly, whose finally re-renders unconditionally; the plan asked the continuation
to be guarded on disposal, so a _disposed flag now suppresses the post-cancel
StateHasChanged (the reference dialog keeps its original shape — out of T11's scope).
P8-7: docker/README.md and docker-env2/README.md still listed the retired
scadabridge-ldap container. Verified against the running config
(central-node-*/appsettings.Central.json): both rigs actually bind to the
local redundant GLAuth pair (zb-ldap-primary :3893 + zb-ldap-backup :3894 on
host.docker.internal, source of truth scadaproj/infra/glauth-redundant), not
the shared 10.100.0.35 GLAuth the infra/docker-compose.yml retirement comment
claimed — that binding is itself stale, its serviceaccount password having
been rotated away by SEC-36. Corrected all three docs to name the actual
config each rig uses.
P8-10: docker-env2 mapped no host port for the central gRPC control plane
(docker/ maps 9013/9014->8083); added 9113/9114->8083 on both env2 centrals,
matching the free 91XX slots, plus the README port table. teardown.sh's
"preserved in node directories" data note predates the named-volume
migration; rewrote it honestly and added an explicit `docker volume rm`
recipe scoped to the env2 volume prefix, clearly labeled destructive,
mirroring docker/teardown.sh's existing pattern.
Gate: bash docker-env2/deploy.sh from this worktree — both centrals + both
site-x nodes recreated and came up with the new 9113/9114 ports open. The
simultaneous force-recreate split both pairs into independent 1-node
clusters (expected: BootstrapGuard is default-OFF on docker-env2, per
CLAUDE.md); a single restart of the b-side of each pair joined it to its
already-formed peer and both pairs converged (central: memberCount=2,
leader=central-a; site-x: node-b Welcome + singleton Younger). docker/ (the
primary rig) was not touched.
P8-8: replace the remaining wall-clock sleep-then-assert instances outside
Communication.Tests' Medium-wave sweep with the same row-40 house pattern —
continuous polling (AssertHoldsFor) for ABSENCE assertions instead of a single
end-of-window sample, which cannot distinguish "stayed quiet throughout" from
"hadn't happened yet when the one sample landed" and cannot catch a value that
flips wrong and back before that sample.
- DataConnectionActorBatchTests: QualityCounters_ArePushedOnTransitionOnly_AndCoalesced
(:206/:214 class) converted to AssertHoldsFor; ResolutionProbes_BackOff...
(:181/:184, a genuine real-time backoff-RATE assertion, not a sync-primitive
stand-in) converted to AssertRoundsWithinWindow, which enforces the ceiling
continuously (fails fast on a no-backoff regression) while the floor still
necessarily waits out the window.
- SiteAlarmLiveCacheServiceTests (:452-454, :470-472): converted to the
project's own AssertHoldsFor (mirrors SiteAlarmAggregatorActorTests', same
assembly).
- StoreAndForwardServiceTests (:891-892): async AssertHoldsForAsync variant
(the class runs on the async test thread).
- KpiHistoryRecorderActorTests (:1043-1049): two skip-path FoldCount==0
absence checks converted.
Each helper is a private per-file copy, matching the Medium-wave T17
precedent (SiteAlarmAggregatorActorTests.AssertHoldsFor) rather than a shared
TestSupport utility. Proof of the mechanism: a standalone (non-repo) harness
run 100 trials of a real background thread flipping a boolean true then back
to false inside a 300ms window — Thread.Sleep(300)+one sample missed the
violation 100/100 times (false pass), continuous 20ms polling caught it
100/100 times. Most of the actual conversions here guard monotonic counters
(their single-sample variant would eventually see the final value too), so
each converted test carries an inline honesty note on what specifically the
conversion buys for that assertion (fail-fast vs. true flip-back detection).
P8-9: ScadaBridgeTelemetryTests.EmitHelpers_DoNotThrow called
SetQueueDepthProvider(() => 5) against the process-global row-32 provider
slot with no clear and no [Collection] — a landmine for any future
same-process reader of scadabridge.store_and_forward.queue.depth. Fixed with
compare-and-clear in a finally via the existing (production, untouched)
ClearQueueDepthProvider. Searched this assembly and the whole test tree for a
second same-process reader that asserts on the value: none exists today
(MetricsEndpointTests scrapes /metrics but only asserts on the response
shape, and the only value-asserting test, QueueDepthGaugeTests, lives in
StoreAndForward.Tests — a separate test-host process the leak cannot reach),
so no [Collection] was added; the test's doc comment records the search and
flags it for re-check if that changes.
Test-only change, zero production edits. Full suites green: DCL.Tests
342/342, Communication.Tests 729/729, StoreAndForward.Tests 135/135,
KpiHistory.Tests 54/54, Host.Tests 522/522. Each converted test additionally
looped 20x in isolation with zero flakes.
P7-6 (behaviour fix, plan decision 1): DeploySiteAsync redeployed EVERY
deployable instance unconditionally. A redeploy is not idempotent at the site —
it resets static attribute overrides and clears native alarm state by design — so
one site-wide push destroyed live operational state on every unchanged instance
in the batch, which is normally most of them. Bulk deploy now pre-filters with
IStaleInstanceProbe (the same seam the bundle importer and Deployments page use;
its memo timing was fixed by the Medium wave's P7-4): an instance whose freshly
computed revision hash still matches its DeployedConfigSnapshot is reported in the
existing additive SkippedInstances list with reason "up-to-date" and never
flattened, staged or sent. The filter is deliberately conservative — every
uncertainty deploys: only an Enabled instance qualifies, a missing snapshot
disqualifies, a prior record that ShouldQuerySiteBeforeRedeploy flags goes through
prepare so the reconcile that HEALS it still runs, and a null/faulting probe means
"indeterminate". New additive MgmtDeploySiteCommand.Force (default false = the new
filtered behaviour) plus CLI `deploy site --force` restores the unconditional push
for the case central cannot see (restored volume, rebuilt node). Composes with the
Medium wave's P7-2: Disabled wins — force suppresses the up-to-date filter only,
never the Disabled skip.
P7-5 (verified, then fixed): the cascade REPRODUCES. All three bulk phases share
one scoped DbContext, and a failed SaveChanges does not untrack the offending
entity — EF re-attempts it on the next save, including MarkDeploymentFailedAsync's
cleanup write. Measured on a real SQLite-backed ScadaBridgeDbContext: a permanent
save failure on instance 1's post-success side effects failed instances 2, 3 and 4
and left all three deployment records stranded InProgress. MarkDeploymentFailedAsync
now detaches the pending changes (new IDeploymentManagerRepository.DetachPendingChanges)
when the cleanup save fails and retries the Failed write once on the clean tracker,
so later instances save cleanly and the record reaches a terminal status. The
DeployFailed audit row moved outside the status-write try (best-effort, must not be
lost because the status write failed).
Tests: 5 new DeploySiteAsyncTests (skip + reason + no site round-trip; --force
deploys; Disabled skipped under both force values; the three not-enough-to-skip
guards), 2 new IntegrationTests over a real EF context (the poisoned-context
cascade, and the narrow EF tracking fact the fix rests on), CLI --force flag
parse test, and a MgmtDeploySite wire-default test. Proof of failure: both P7-6
behaviour tests and both P7-5 assertions fail against the unfixed code.
Mirrors the Medium wave's DebugView P6-6 fix (_sessionLock + _disposed,
publish-then-recheck-disposal under one lock) for the two TTL-bounded/
unbounded lesser instances the Medium wave deferred:
- TransportImport.razor.cs: TryLoadAsync awaits IBundleImporter.LoadAsync and
used to publish the result into _session unconditionally. A circuit torn
down mid-await left Dispose() with nothing to release (session still null),
so the freshly-opened server-side BundleSession stayed registered, stranding
one of MaxConcurrentImportSessions (register row 43) for the full session
TTL. Now publishes and re-checks disposal atomically under _sessionLock, so
exactly one side (TryLoadAsync or Dispose) abandons the session.
- Topology.razor: OnInitializedAsync awaits LoadDataAsync before calling
StartLiveUpdatesTimer. A circuit torn down mid-await left Dispose() with no
timer to stop yet, so the timer StartLiveUpdatesTimer creates once the await
resumes was never disposed — it ticked against a torn-down component every
15s until process restart. StartLiveUpdatesTimer/StopLiveUpdatesTimer/
Dispose now create/replace/take the timer atomically under _timerLock
together with a _disposed check, so a late-arriving (re)start refuses to
create a timer once disposed.
Tests (bUnit, both proven to fail against the unfixed code):
- TransportImportSessionDisposeRaceTests: dispose-during-load abandons the
just-opened session immediately; dispose-after-successful-load abandons
exactly once.
- TopologyLiveUpdatesTimerDisposeRaceTests: a late StartLiveUpdatesTimer call
after Dispose leaves no live timer; Dispose stops the timer exactly once
(idempotent double-dispose); OnLiveUpdatesToggled(off) + Dispose leaves no
live timer. All assert on deterministic field/call-order state, no sleeps.
Register row 103 stays open for integration to close per the plan's shared
constraints (register edits happen at integration only).
Neither Test Run form implemented IDisposable, so navigating away mid-run left
_runCts uncancelled: the sandbox kept the run alive until its own internal
script timeout, and the eventual continuation touched a torn-down component's
_running/_runResult fields. TemplateScriptDialog already got this right — both
forms now mirror it exactly: @implements IDisposable plus a Dispose() that
cancels + disposes _runCts.
ScriptAnalysisService.RunInSandboxAsync is marked virtual (test-only seam,
mirroring the existing virtual on DebugStreamService.StartStreamAsync/
StopStream) so a TaskCompletionSource-gated stub can hold a run open
deterministically. New bUnit tests (ScriptFormSandboxDisposalTests) prove, for
both forms: disposing mid-run cancels the token the sandbox observed, and the
cancelled continuation resolves with no unhandled exception and no _runResult/
_running mutation. Proof of failure: reverting only the two .razor changes
(keeping the virtual seam) reproduces all 4 failures — the token is never
cancelled and the no-op assertions time out.
Three fetch initiators (search debounce, paging, push/coalesce reload) could
each have a LoadDataAsync flight in flight at once; completion order was not
call order, so a slow, stale flight could resolve after a newer one and
clobber the grid the operator was already looking at. Add a monotonic
_requestGeneration counter, bumped at fetch start and checked at apply —
last-write-wins by generation, not by whichever await resolves first.
Separately, the filter-debounce timer wrote `_currentPage = 1` directly on
its raw System.Threading.Timer callback thread — a Blazor threading
violation, since component state may only be mutated on the renderer's
sync context — and that write was not covered by the generation guard at
all. LoadDataAsync now takes a resetToFirstPage flag, resolves the target
page into a local, and applies it only behind the same generation check as
every other piece of state, via a new DispatchFilterReloadAsync that
marshals through InvokeAsync like the existing push-reload path.
Tests (DeploymentsRequestGenerationTests, bUnit): a delayed stale flight
arriving after a newer one does not overwrite the grid; a stale
filter-debounce flight does not reset the page after a newer navigation.
Both driven deterministically via TaskCompletionSource-backed repository
stubs. Verified to fail against the unfixed code (swapped via
`git show HEAD:<path>`, not `git stash` — repo-wide and unsafe across the
parallel worktree wave).
ManagementActor's ParseDataType/ParseElementDataType (bare Enum.TryParse) and
the two AlarmTriggerType parse sites in HandleAddAlarm/HandleUpdateAlarm (raw
Enum.Parse) all admitted ordinals ("2"), undefined values ("99" — a
deploy-time bomb that saves fine and only detonates at deploy), and
comma-joined name lists (bitwise-ORed together regardless of [Flags], e.g.
"String,Int32" silently becoming DateType.DateTime). A typo against the raw
Enum.Parse sites also surfaced as a masked "An internal error occurred"
rather than a curated field-naming error.
Adds ParseEnumStrict<TEnum>, mirroring the existing ParseDeploymentStatus
guard (names only, defined values only, comma-token rejection per the
Medium-wave T8 nuance / AuditQueryParamParsers.ParseEnumListStrict pattern),
and routes all four call sites through it.
Tests: 22 new ManagementActorTests cases per parse site (ordinal, undefined
numeric, comma-joined, and typo all rejected with the curated field-naming
error; every defined name still parses through to the service layer).
Proof of failure recorded in each rejection test's doc comment; verified
live by stashing the ManagementActor.cs fix — exactly the 14 new rejection
tests fail pre-fix (13 acceptance tests pass unchanged), confirming the
defect and the fix.
SiteAuditReconciliationActor minted a fresh random EventId for the synthetic
ReconciliationAbandoned row every time a permanently-abandoned row was
re-abandoned. Because the reconciliation cursor is held at `since` by ANY
co-batch unresolved row (see PullSiteAsync's hasUnresolvedFailure gate), a
poisoned row's per-EventId attempt counter restarts every time it reappears
in a stuck batch, so it gets re-abandoned roughly every
MaxPermanentInsertAttempts ticks — minting an unbounded number of duplicate
central audit rows for the SAME lost event.
Fix: DeriveAbandonmentEventId(Guid lostEventId) derives the synthetic
EventId deterministically (SHA-256 over a fixed namespace GUID + the lost
row's bytes, RFC4122-v5-shaped). Re-abandonment of the same lost row now
always produces the same synthetic EventId, so
IAuditLogRepository.InsertIfNotExistsAsync's existing idempotency collapses
every repeat into the first insert. Also adds an in-memory _abandonedEventIds
set as a CHEAP SHORT-CIRCUIT ONLY (skips the wasted ~25 min retry-then-abandon
cycle for a row already known abandoned this process's lifetime) — it is not
the correctness mechanism; it resets on singleton restart/failover, at which
point the deterministic id alone still keeps re-abandonment idempotent.
Tests (SiteAuditReconciliationActorTests):
- ReAbandonment_SameLostEventId_YieldsExactlyOneCentralRow: end-to-end repro
of the defect trigger (poison row + a rotating-EventId co-batch row that
never itself reaches the threshold, holding the cursor forever) across two
full abandon cycles. Proof of failure: run against the unfixed source
(git stash of the source file only, pure-function tests temporarily
#if false'd to keep the file compiling), the test failed with exactly the
predicted defect shape — 2 distinct synthetic rows for one lost EventId.
Passes post-fix (1 row).
- DeriveAbandonmentEventId_DifferentLostRows_YieldDistinctSyntheticIds and
DeriveAbandonmentEventId_IsPureAndStableAcrossIndependentInvocations:
pure-function coverage of the derivation directly, including a pinned
golden-value regression guard proving it is a function of the input GUID's
value alone (no clock/randomness/process state).
Defect: the hourly KPI rollup fold (KpiHistoryRepository.FoldHourlyRollupsAsync)
sums every Rate-classified sample recorded in an hour, correct only if each
sample is a non-overlapping delta covering exactly one recorder tick
(KpiHistoryOptions.SampleInterval). That invariant was an unenforced silent
assumption across three independently-owned config keys — SampleInterval,
NotificationOutboxOptions.DeliveredKpiWindow, SiteCallAuditOptions.KpiInterval —
so widening DeliveredKpiWindow to e.g. 15 min would silently over-count the
rollup ~15x.
Robust fix: NotificationOutboxKpiSampleSource and SiteCallAuditKpiSampleSource
now rescale their Rate-classified samples (deliveredLastInterval,
failedLastInterval) by SampleInterval / (DeliveredKpiWindow or KpiInterval)
before recording, so the rollup's per-hour Sum is correct regardless of how
the other two keys are tuned — the live dashboard tile's own cutoff/window is
untouched. Both sources gain a project reference to KpiHistory to read
SampleInterval; no cycle (KpiHistory references only Commons).
Eager cross-validation (defense-in-depth): KpiRateFoldIntervalCrossValidator
(Host, the one composition root referencing all three component projects)
fails boot naming all three config keys when they diverge, protecting the
separate (still-real) invariant that the recorded/charted KPI-history sample
equals the live tile at the same instant — which the rescale trades away
whenever the scale factor isn't 1. Rides KpiHistoryOptions' existing
ValidateOnStart trigger.
Also: NotificationOutboxOptions had no registered validator at all before this
(the gap this task's "whichever options class owns DeliveredKpiWindow" file
scope pointed at) — added NotificationOutboxOptionsValidator mirroring
SiteCallAuditOptionsValidator's positive-duration/count convention, wired with
ValidateOnStart.
Tests: new rescale tests in both sample-source suites prove the fold is
numerically correct at a non-default SampleInterval (30/40 counted over a
15/20-minute window correctly divides down to a 1-minute-tick delta); new
KpiRateFoldIntervalCrossValidatorTests prove divergent config fails with all
three keys named; new NotificationOutboxOptionsValidatorTests. Every new test
proven to fail against the unfixed computation first (temporarily
short-circuited the scale factor / cross-check to the pre-fix behavior,
confirmed failure, restored) — not via git stash (repo-wide, shared across
concurrent worktrees this session) but via file-copy swap.
Full suites green: Host.Tests (527), ConfigurationDatabase.Tests (401),
KpiHistory.Tests (54), NotificationOutbox.Tests (163),
SiteCallAudit.Tests (67).
P5-3: add IX_Notifications_Created_NotificationId (CreatedAt DESC,
NotificationId DESC) — the Central UI Notification Outbox list page sorts on
exactly that pair, but no existing index leads unconditionally on CreatedAt,
so an unfiltered/lightly filtered page fell back to a full clustered-index
scan + sort on every COUNT + page request. Mirrors the Medium wave's
IX_SiteCalls_Created_TrackedOperationId migration + SQL twin pattern
faithfully, including the handwritten docs/plans/sql script.
P5-4: operator Retry/Discard was read-check-write keyed on NotificationId
only — a detached PipeTo task interleaving with the dispatcher's own write
(or a second concurrent operator action) could land between the read and
the write and get silently clobbered by the unconditional UpdateAsync,
stamping a Delivered row Discarded with stale fields. Adds
INotificationOutboxRepository.TryUpdateFromParkedAsync, a single-statement
compare-and-swap (WHERE Status == Parked) mirroring
SecuredWriteRepository.TryMarkApprovedAsync's CAS shape. NotificationOutboxActor's
RetryAsync/DiscardAsync now use the CAS write and, on a lost race, re-read
to report "notification already progressed to <Status>" instead of silently
overwriting whatever is there now.
Proof of failure: reverting only the actor's CAS usage while keeping the
repository addition reproduces the silent-overwrite bug — 7 tests fail,
including two new deterministic (non-sleep) race-proof tests that mock the
exact GetByIdAsync -> lost-CAS -> re-read sequence the finding describes.
P5-3 is perf-only; verified via migration/SQL-twin parity instead (no
runtime proof-of-failure is meaningful for an index).
P4-5: EventLogPurgeService.ExecuteAsync now yields (Task.Yield()) before running the
initial RunPurge(), making "the purge never runs synchronously on the caller's thread"
a property of the method itself rather than an artifact of .NET 8+'s BackgroundService
scheduling (verified empirically not to reproduce the literal "StartAsync blocks" symptom
on this repo's net10.0 target — a 1.5s synchronous Thread.Sleep in ExecuteAsync still lets
StartAsync return in ~2ms). Proven instead via a reflection-invoked ExecuteAsync test that
bypasses BackgroundService's Task.Run wrapper, which is timing-independent. Also: a
throwing isActive predicate now fails CLOSED (logged as inactive, at Warning) instead of
silently claiming "active" — the predicate only affects the tick's log line since P4-4,
but a throw must not masquerade as a successful check.
P4-6: PRAGMA auto_vacuum = INCREMENTAL only takes effect on a table-less database, and
LocalDbSchema.EnsureCreated (inside SqliteLocalDb's constructor) always creates LocalDb's
own bookkeeping tables before SiteEventLogSchema.Apply ever gets a connection — so the
pragma there had never once taken effect, in any real call path. Real fix per plan
decision 2: SiteLocalDbSetup.EnsureAutoVacuumOnFreshFile pre-creates a genuinely fresh
LocalDb file with the pragma set, called from SiteServiceRegistration.Configure BEFORE
AddZbLocalDb's lazy singleton ever constructs SqliteLocalDb — no LocalDb API change
needed. Only helps NEW site databases; an existing file (every node on the docker rig
today) is left untouched rather than VACUUM'd, per the plan's explicit rejection of an
unconditional VACUUM against a live ~1 GB file. The now-dead pragma call in
SiteEventLogSchema.Apply is removed and its comment corrected.
Tests proven to fail against the unfixed code (git-diff-and-restore, not stash — this repo
shares one stash ref across all worktrees): ExecuteAsync_YieldsBeforeRunningTheInitial-
PurgeSynchronously and RunPurge_ActiveNodeCheckThrows_TickLogsAsInactive_NotActive fail
without the EventLogPurgeService fix; SiteLocalDbAutoVacuumTests fails to compile without
EnsureAutoVacuumOnFreshFile. SiteEventLogging.Tests 112/112 and Host.Tests 526/526 green
with the fix applied.
P3-4 — SiteAlarmAggregatorActor pinned _openInFlight forever on a stream open
that produced none of its three peer-driven outcomes. SiteStreamGrpcClient gives
up on response headers after ConnectedHeaderTimeout (10s) and then treats the
first received EVENT as its fallback connected signal, so an alarm-only site
that stays quiet supplies neither — leaving _streamDown=true AND _openInFlight=
true, which makes OnReconcileTick's reopen guard (_streamDown && !_openInFlight)
skip every tick until the site's 4h max stream lifetime ends the call.
Fix: bound one open attempt with a generation-fenced single-shot deadline
(GrpcAlarmStreamOpenTimedOut, ctor-injected, production default 30s — above the
10s header timeout, below the 60s reconcile interval). Firing it releases the
flag and nothing else: no retry-budget spend, no node flip, no liveness
republish, and the abandoned subscription is left running so a merely-slow
connect still comes up normally. Self-healing rather than reachability analysis.
The deadline is cancelled on connect / error / graceful completion.
P3-5 — SiteStreamGrpcServer cancelled the incumbent stream on a duplicate
correlation id and only THEN checked the concurrency cap. Checking Count >= max
after the removal is right at exactly the cap but wrong above it, and above it
is reachable because the cap check is documented check-then-act: two subscribes
that both clear it before either registers overshoot by one. In that window a
routine reconnect killed a healthy stream AND was refused with
ResourceExhausted, leaving the correlation with ZERO streams until its next
retry.
Fix: claim the removal, consult the cap only for a genuinely NEW correlation id,
then cancel. A replacement is net-neutral on the count so it never consults the
cap; a refused new subscriber cancelled nothing (TryRemove found nothing), so
the incumbent survives. Newest-wins is unchanged — a fresh authenticated
subscribe still displaces a dead/wedged incumbent.
Tests (both shown failing against the unfixed code):
- OpenAttemptThatNeverResolves_ReleasesTheInFlightGuard_SoTheTickCanReopen —
fails (reopen never happens) with the deadline arming disabled.
- OpenDeadline_DoesNotFire_OnAStreamThatConnects_NorFlipNodeOrBurnBudget —
inertness lock on every healthy path.
- AtCapOvershoot_DuplicateReconnect_IsNotRefused_AndLeavesExactlyOneLiveStream —
fails with ResourceExhausted against the old ordering; the overshoot is driven
deterministically by parking both openers on the ServerCallContext token read,
which sits between the cap check and the _activeStreams insert.
- AtCap_AGenuinelyNewSubscriber_IsStillRefused_AndTheIncumbentSurvives.
The P3-4 test's doc comment carries the honesty note: the aggregator-side
starvation reproduces exactly, but how often the >10s header delay that gets you
there occurs in production is NOT demonstrated — the site flushes headers
explicitly as soon as its relay is attached, so the budget has to be consumed by
connect + handshake + server scheduling instead.
P2-5 — the tag-resolution PROBE was the only subscribe path that did not seed.
HandleSubscribe carries seeds back on SubscribeCompleted and ReSubscribeAll re-seeds
after a reconnect, because a fresh advise only yields a value on the source's next
CHANGE. HandleRetryTagResolution did neither, so a tag that resolved LATE (device still
booting, item created after deployment) and then never changed had no value path at all.
OPC UA masks it with the initial data-change notification on subscription creation; an
MxGateway advise can legitimately return VT_EMPTY, so a static tag stayed Uncertain for
the life of the connection. New SeedNewlyResolvedTags reads the round's newly resolved
tags and delivers them through the normal generation-guarded TagValueReceived path —
directly rather than on the completion message (the probe only covers tags already
registered in _instancesByTag, so the initial subscribe's ordering hazard does not
apply), and as its own fire-and-forget task so the seed reads never delay the
BatchSubscribeCompleted that re-arms the retry timer.
P2-6 — only the Connected state handled AdapterDisconnected; in Connecting and
Reconnecting it fell through to Unhandled. A connect that succeeds and drops again before
its piped ConnectResult is applied therefore delivered the disconnect FIRST, where it was
discarded, and ConnectResult(true) then declared the connection up on a dead adapter. An
adapter raises Disconnected at most once per successful connect (OpcUaDataConnection's
once-only _disconnectFired, reset only by the next successful ConnectAsync), so a
connection with no subscriptions — nothing to fail at connection level and drive the
reconnect machine — sat Connected on a dead adapter until the process restarted. Both
states now record the disconnect in _disconnectDuringConnectAttempt (cleared at the top of
HandleAttemptConnect, so it only ever refers to the attempt in flight) and both result
handlers demote such a "success" to the ordinary connect-FAILURE tail — counting toward
the failover budget and re-arming the retry timer. A flag rather than Stash: the stash has
no selective drop, so a stashed disconnect would be replayed after the next successful
connect and cost a spurious reconnect cycle.
AdapterDisconnected is now stamped with the adapter generation (hooked via a
per-adapter closure instead of a method group; SwapAdapter hooks AFTER the bump), and
IsStaleDisconnect drops a notification from a swapped-out adapter in all three states —
the disconnect twin of IsStaleConnectResult. SwapAdapter's unhook cannot stop an event
already executing on a background thread, so without the fence an endpoint the operator
has just reconfigured AWAY from could knock the healthy replacement into a reconnect.
Tests: DataConnectionProbeSeedTests + DataConnectionDisconnectRaceTests (4 new). The
race is reproduced by ORDERING, not timing — the connect is held on a gate, Disconnected
is raised while the actor sits idle, and a GetHealthReport round trip is the mailbox
barrier; no sleeps. All 4 were proved to fail against the pre-fix behaviour via a
targeted revert of the four behaviour sites (the stale-generation test cannot be proved
by a plain revert — the record carried no generation before this change, so the test
would not compile; noted in its doc comment). Full DCL suite 346/346 green.
HandleEnsureInstanceStarted, HandleDeploymentStateQuery, and HandleEnable each
called GetAllDeployedConfigsAsync — materializing every deployed config,
including every ConfigJson blob — just to FirstOrDefault one row. A
volume-restore reconcile healing hundreds of instances turned that into
hundreds of full-table reads.
Add SiteStorageService.GetDeployedConfigAsync(name) (single-row, full
projection incl. ConfigJson, for callers that (re)start an Instance Actor)
and GetDeployedConfigMetadataAsync(name) (single-row, ConfigJson-free
projection, for callers that only need DeploymentId/RevisionHash/IsEnabled).
Switch the three call sites; leave decisions/semantics identical.
HandleEnsureInstanceStarted gets its own _deployedConfigLoader seam,
deliberately separate from the existing _configLoader (still used by the
boot-time load, which legitimately needs every row) so a test overriding one
cannot accidentally gate the other. Generation fencing (StoreGenerationAtRead,
Medium wave T1) is untouched: the generation is still captured on the actor
thread before the read dispatches.
Tests: new SiteStorageServiceTests lock in that both projections return the
same values GetAllDeployedConfigsAsync().FirstOrDefault(...) did. The three
row-101 ensure-started race tests (ReconcileStartsInstanceTests) are ported
to the new seam and still pass, proving generation fencing survives the
projection. Proof of failure: since this is a pure I/O-cost fix with no
observable behavior change, the new/changed tests were confirmed to fail to
even compile against the unfixed code (stash/unstash) — GetDeployedConfigAsync,
GetDeployedConfigMetadataAsync, and the deployedConfigLoader ctor param don't
exist there.
P6-7 (CWE-1236): AuditEndpoints CSV export quoted cells on comma/quote/CR/LF
but never neutralized a leading =, +, -, @, TAB, or CR — inbound-API-caller
text landing in Target/ErrorMessage/RequestSummary/ResponseSummary/Extra
could execute as a formula/DDE trigger when opened in Excel. AppendField now
prefixes a literal single quote on free-text columns only (never the
GUID/enum/numeric/date/bool columns the exporter formats itself), applied
before RFC 4180 quoting. The CSV was also missing ExecutionId/
ParentExecutionId/SourceNode that the JSONL export already carried; both are
now 24 columns in the same AuditExportRow order. A second, independently
maintained CSV writer (CentralUI's AuditLogExportService, feeding the Audit
Log page's bulk-export button) had the identical gap and got the identical
fix + column alignment.
P6-8: `audit export` was the one CLI HTTP path with no deadline at all — a
residual gap from the ManagementHttpClient timeout-honesty fix, which made
HttpClient.Timeout infinite and the per-call token the single deadline
everywhere except this streamed GET. RunExportAsync now takes a headers-
received deadline (CancelAfter, reusing ManagementHttpClient.
DefaultConnectTimeout) around the initial GetAsync, an inactivity deadline
around the streaming copy (rearmed on every successful read, so large exports
are bounded by silence, not total size), and an external CancellationToken
wired to Console.CancelKeyPress in AuditCommands so Ctrl-C cancels promptly.
A stalled/faulted stream now produces a friendly error noting the partial
output file instead of hanging or dumping a stack trace.
TimeoutSeconds is the per-system HTTP timeout ExternalSystemClient reads at
call time and is deployed to sites (proto field 6), but Transport bundles
silently dropped it: the DTO never carried it, export never wrote it, the
importer's Add path reset it to 0, the Overwrite path left the target's
stale value untouched, and ArtifactDiff never compared it (so a bundle
whose only change was a tuned timeout previewed as Identical).
Adds TimeoutSeconds as an additive-optional trailing field on
ExternalSystemDto (schemaVersion stays 1.1, bundleFormatVersion unchanged)
and threads it through EntitySerializer (export + import), BundleImporter
(Add and Overwrite), and ArtifactDiff.CompareExternalSystem. Old-bundle
semantics pinned: an absent field (null) falls back to the entity default
on Add and leaves the target's existing value untouched on Overwrite/diff.
Beefed up the RoundTripEquivalenceTests external-system fixture with a
non-default TimeoutSeconds so the reflection round-trip guard actually
exercises the field instead of comparing default against default.
Swept the rest of the ExternalSystemDto shape against
ExternalSystemDefinition field-by-field; no other operative field is
missing.
LoginThrottle keyed failures only as {username}|{ip}, so a password spray
from one IP against many distinct usernames never accumulated
MaxLoginFailuresPerWindow failures on any single pair and was never
throttled, despite the class doc claiming spray protection.
Add a secondary per-IP counter (SecurityOptions.PerIpMaxFailures, default
25, validated strictly greater than MaxLoginFailuresPerWindow) that shares
the existing fixed window/lockout machinery but tracks failures by IP
alone. IsLockedOut now refuses a bind when either the per-pair or the
per-IP counter is locked. RecordSuccess clears only the per-pair counter
(never the per-IP one), so a spray that stumbles onto one valid account
cannot use that success to reset its IP budget and keep spraying; window
expiry is the only relief for the per-IP counter. Per-pair behavior for
normal (non-spray) flows is unchanged.
Updates the class doc and Component-Security.md to describe the two-tier
model instead of the single-tier spray claim.
A debug-stream session created after the consumer had already gone away was
leaked permanently. Both consumers awaited StartStreamAsync (up to 30s) and only
then published the session; a circuit teardown / client disconnect inside that
window ran the release path first, found nothing registered and stopped nothing.
The session stayed registered in the singleton DebugStreamService, whose
keepalive renews registered sessions forever (the 5-minute orphan reaper only
collects UNregistered ones), pinning the site gRPC subscription, the
DebugStreamBridgeActor and the dead consumer's object graph until process
restart.
Both call sites now publish the session and re-check liveness under a single
monitor, so exactly one side tears it down and neither can miss it:
- DebugView.Connect/Dispose/Disconnect share a _sessionLock; Dispose sets
_disposed and takes the session in one step, Connect publishes only if not
disposed and otherwise stops the session it just created. Dispose can run off
the renderer's thread, so the Blazor sync context is not relied on. Disconnect
and the onTerminated callback go through the same take-once helper, which also
makes a repeated Dispose stop-once instead of stop-again.
- DebugStreamHub uses the per-connection Items monitor: OnDisconnectedAsync marks
the connection gone before releasing, SubscribeInstance publishes only if
neither that flag nor ConnectionAborted is set, and UnsubscribeInstance takes
the id atomically.
DebugStreamService.StartStreamAsync/StopStream become virtual purely as a test
seam (no behaviour change, keepalive/orphan contract untouched) so the race can
be driven deterministically by a gated stub.
Tests: DebugViewConnectDisposeRaceTests (3) + DebugStreamHubSubscribeRaceTests
(3). Verified fail-unfixed: with the two source fixes stashed, both
dispose-mid-start tests report zero StopStream calls and the DebugView
double-dispose test reports two.
Joseph Doherty