Comment/XML-doc changes only:
- add summaries to ~10k test methods, fixtures, fakes and helpers
(+ <param> on theories, <returns> on async Task methods,
<inheritdoc/> on interface implementations the checker can resolve)
- strip ~2.6k project-tracking references (WP*, P*-*, row-NN, Task N,
#NN, Component-0NN) from comments, keeping the explanatory text;
reword false positives in comments only
- real docs on fakes implementing BCL/library interfaces
(IDisposable, ILogger, IOptionsMonitor, IAsyncLifetime)
- reword the last 5 src/ false positives (CWE-1236, failures-1,
year-0001, M365)
Only non-comment edits: four one-line nested classes split across lines
so members can carry docs. Build 0 errors / 0 warnings.
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.
The gauge test polls baseline+1, but the process-global gauge's baseline is
captured once: a parallel class's streams closing after capture lowers the
gauge below baseline+1 forever. Survey confirmed only SiteStreamGrpcServerTests
and CleanupVerificationTests drive the real SubscribeInstance handler that
moves the counter; both now share the SiteStreamSubscriberGauge collection
(row-96 pattern). 4/4 full-suite runs green, durations unchanged.
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.
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.
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.
Adds an additive, site-wide, alarm-only gRPC stream backing the aggregated
Alarm Summary. Proto: new SiteStreamRequest { correlation_id } message + rpc
SubscribeSite(SiteStreamRequest) returns (stream SiteStreamEvent) on
SiteStreamService — purely additive, no field renumbering. Regenerated the
checked-in SiteStreamGrpc/*.cs.
Server: SubscribeInstance and the new SubscribeSite now delegate to a shared
RunSubscriptionStreamAsync helper (readiness/shutdown guards, correlation-id
validation, duplicate replacement, concurrency cap, bounded DropOldest channel,
relay actor, SiteConnectionOpened/Closed telemetry, guaranteed cleanup). The
only variation is the subscribe delegate: SubscribeSite calls
ISiteStreamSubscriber.SubscribeSiteAlarms (no per-instance filter). Added
SubscribeSiteAlarms to the ISiteStreamSubscriber contract (SiteStreamManager
already implements it from T1). StreamRelayActor reused unchanged — it already
drops IsConfiguredPlaceholder rows and maps the enriched AlarmStateUpdate.
Tests: SubscribeSite subscribes site alarms + removes on cancel, rejects unsafe
correlation ids, and relays a domain AlarmStateChanged as a proto
AlarmStateUpdate on the stream.
Claude-Session: https://claude.ai/code/session_01MtdgwpEeCUn6cUA5f1LMPj
Joseph Doherty