Review found AlarmDispatcher.SnapshotActiveAlarms reading the snapshot and the
truncation verdict through two independent lock acquisitions, defended by a
comment claiming read-order made a race "widen only, never narrow". That claim
was false: a not-truncated -> truncated poll landing between the two reads pairs
a stale false with a capped snapshot, which is exactly the false all-clear the
feature exists to prevent. It was safe only because AlarmCommandHandler
STA-serializes consumer calls — an accident of the call graph, not an invariant.
Made the invariant structural instead of documented. IMxAccessAlarmConsumer now
exposes ONE accessor, `IReadOnlyList<MxAlarmSnapshotRecord> SnapshotActiveAlarms(
out bool truncated)`, which implementations must satisfy from a single
acquisition of the lock guarding the retained snapshot — mirroring the write
side, where FoldFetch already updates snapshot and verdict together. The
separate LastSnapshotTruncated property is gone from every layer, so there is no
second read left to pair badly. `out` over a result struct follows the file's
established idiom (FoldFetch, ParseSnapshotXml).
The same threading applies one level up: IAlarmCommandHandler.QueryActive now
carries `out bool snapshotTruncated`, so MxAccessCommandExecutor stamps the reply
payload from the value the records were stamped with rather than reading the
state a second time.
Direct tests for the three hops that were only covered end-to-end:
- AlarmDispatcherTests: truncated consumer snapshot stamps FromTruncatedSnapshot
on every mapped record, with a complete-snapshot control, plus an assertion
that the independent per-record Degraded flag is not dragged along.
- AlarmCommandHandlerTests: the verdict delegates through the dispatcher
(Theory over both values), and survives a prefix filter that removes every
record — the case the per-record flag cannot cover.
- AlarmCommandExecutorTests: the reply payload's SnapshotTruncated comes from the
handler (Theory over both values), including the zero-record case.
The WnWrapAlarmConsumer truncation tests now assert through
SnapshotActiveAlarms(out ...) rather than an internal field, because the pairing
is the contract.
Also: GatewayAlarmMonitor's _snapshotTruncated comment now says "as of the last
full reconcile" rather than implying it tracks the current _alarms contents,
which live transitions keep moving via ApplyTransition between passes.
Detection heuristic still untouched (fetchedRecordCount >= maxAlarmsPerFetch);
no @COUNT parsing, per docs/AlarmProbeFindings.md. Still additive gateway
metadata about our fetch mechanics, not MXAccess behavior — not a parity
deviation, and no event is synthesized.
Gateway: NonWindows.slnx builds clean (0 warnings); ~Alarm filter 107/107 pass.
Worker + Worker.Tests are windev-gated; the signature change was reviewed by
inspection across all 7 IMxAccessAlarmConsumer implementers, all 3
IAlarmCommandHandler implementers, and every call site.
The truncation-cliff fix made alarm transitions truncation-safe but silent:
when GetXmlCurrentAlarms2 returns exactly maxAlmCnt records the worker
suppresses absence-implies-Clear inference and says so only in a rate-limited
stderr warning. No client and no operator could tell a complete active set
from a capped one.
Two additive proto3 booleans carry the verdict out:
- QueryActiveAlarmsReplyPayload.snapshot_truncated = 2 (worker IPC reply)
- ActiveAlarmSnapshot.from_truncated_snapshot = 16 (per record)
The per-record field is not an aesthetic choice. QueryActiveAlarms returns a
bare `stream ActiveAlarmSnapshot` with no envelope, header, or trailer, so a
per-record boolean is the only carrier that stays wire-compatible; an envelope
message would change every existing client's stream element type. The reply
payload states it too because a prefix filter can leave zero records and a
truncated fetch with nothing to report still has to say so. The flag means
"this set may be incomplete", never "this record is unreliable" — it is
independent of the subtag-fallback `degraded` field.
Detection is deliberately UNCHANGED: IsTruncatedFetch remains
`fetchedRecordCount >= maxAlarmsPerFetch`. The live probe (docs/AlarmProbeFindings.md,
ce5d8ae) could not verify whether ALARM_RECORDS/@COUNT reports the total active
count or only the records in the reply, so @COUNT is not parsed for detection;
switching to it stays blocked on probe evidence. The probe's comment
annotations in WnWrapAlarmConsumer.cs are preserved.
Reset semantics: not latched. WnWrapAlarmConsumer.FoldFetch replaces the
verdict on every poll under the same lock as the snapshot merge, so the first
sub-cap fetch clears it; GatewayAlarmMonitor.ClearCache drops it with the cache
generation it describes. A caveat that never turns off is one operators learn
to ignore.
Flow: WnWrapAlarmConsumer.LastSnapshotTruncated -> AlarmDispatcher (stamps every
record) / IAlarmCommandHandler (payload) -> MxAccessCommandExecutor reply ->
GatewayAlarmMonitor._snapshotTruncated -> IGatewayAlarmService.SnapshotTruncated
-> DashboardAlarmQueryResult -> AlarmsPage warning banner (render-side only; the
poll loop and DisposeAsync drain are untouched). The public QueryActiveAlarms
RPC forwards worker snapshots unmodified, so the per-record flag needed no
mapper change — a test pins that.
Parity: this describes OUR fetch mechanics — additive gateway metadata — not
MXAccess provider behavior. No event is synthesized and no MXAccess-observable
semantics change, so it is not a parity deviation.
Tests: worker LastSnapshotTruncated set/reset/consecutive-burst (windev-run);
gateway end-to-end truncated reply -> monitor -> public stream, with the
complete-reply control as the load-bearing assertion; AlarmsPage banner
present/absent. Docs: gateway.md alarm surface, docs/DesignDecisions.md entry.
A periodic reconcile can synthesize a repair transition whose matching live
transition is still buffered in the alarm lease; both then broadcast as
indistinguishable duplicates on StreamAlarms and the dashboard hub. Nothing
serializes the two paths, and a correct serialization needs a worker-side
high-water mark on QueryActiveAlarms (proto + worker change + a stall path),
so this closes the common case with a local best-effort dedup instead: both
paths already carry the same worker-derived identity — the worker stamps
record.TransitionTimestampUtc into both OnAlarmTransitionEvent's
transition_timestamp and ActiveAlarmSnapshot.last_transition_timestamp — so
ApplyTransition suppresses a live transition whose (timestamp, resulting
state) the cache already carries from a repair. The Clear leg has no cache
entry left to compare, so ApplyReconcile tombstones each synthesized Clear by
the instance's original_raise_timestamp for one reconcile generation; a
matching live Clear consumes the tombstone, while a new raise/clear cycle
carries a newer raise timestamp and passes. Suppression fires only on a
positive marker match — unset timestamps keep today's behavior — so the
documented at-least-once consumer contract stands (gateway.md, Sessions.md
updated in the same change).
Tests: two new regressions drive the exact race through the GWC-26 harness
(repair-then-buffered-live for Raise and for Clear, each with a genuine
follow-up transition proving no over-suppression); GatewayAlarmMonitor
suites 18/18.
Code-review follow-up on fix/gwc-26-27-alarm-attach.
ApplyReconcile's snapshot-derived feed repairs are at-least-once, not
exactly-once: a reconcile reads the worker's current state while the matching
live transition may still be buffered in the monitor's lease, so both broadcast
and the duplicates are indistinguishable on the alarm feed. This pre-dates the
acked-state delta — the Raise/Clear presence repair has always had it, since
nothing serializes a reconcile pass against the in-flight live stream — so
closing it (serialization or timestamp dedup) stays out of scope for a P2 fix.
Documented instead, with the consumer contract stated explicitly (apply
transitions idempotently, never as an increment or toggle):
- ApplyReconcile gains a "Delivery semantics" comment.
- gateway.md softens the "defense in depth" prose to state the semantics.
- docs/Sessions.md carries the same caveat on the alarm-feed description.
- Tracker change-log records it as a known pre-existing characteristic and a
candidate finding for the next review cycle.
Also hoists the ChannelWorkerClient fake — duplicated across the three alarm
test files — into TestSupport/, dropping the usings it took with it.
RunMonitorAsync issued SubscribeAlarms and the first reconcile before the
internal distributor subscriber was attached (via ISessionManager
.ReadAlarmEventsAsync). The pump has been running since MarkReady started the
dashboard mirror and only fans to subscribers registered at fan-out time, so
every transition raised in that two-round-trip window bypassed the alarm feed —
and a missed Acknowledge was never repaired, because ApplyReconcile broadcast
presence deltas only.
- The monitor now takes the internal lease directly from its session BEFORE
SubscribeAlarms and drains it after the first reconcile; window transitions
buffer in the lease's bounded channel. Processing them after ApplyReconcile is
order-safe (ApplyTransition handles alarms the snapshot already placed).
- ISessionManager.ReadAlarmEventsAsync removed — zero remaining callers.
- ApplyReconcile broadcasts an Acknowledge feed transition when a both-present
alarm's state advanced to ActiveAcked. This is a feed-level repair on the
AlarmFeedMessage/StreamAlarms surface rebuilt from the worker's own snapshot,
not MxEvent emission, so the "never synthesize events" rule is untouched;
the reasoning is recorded on ApplyReconcile.
The alarm-monitor test fakes now hand the monitor a real Ready GatewaySession
with a dashboard mirror, which is what makes the window reproducible.
Docs: docs/Sessions.md and gateway.md alarm-monitor ordering notes.
Refs: archreview/2026-07-12/remediation/10-gateway-core.md GWC-26
Resolve all CommentChecker findings across the gateway server, worker, tests,
and .NET client (314 -> 0 real issues): add missing <returns>/<summary>/<param>
on public and test members, convert Stream/interface overrides to <inheritdoc/>,
and remove internal task/issue tracking IDs (SEC-*, IPC-*, WRK-*, GWC-*, TST-*,
Client.Dotnet-*) from shipped code documentation while preserving the design
rationale prose. Shipped comments should not carry internal bookkeeping, and
complete XML docs keep the analyzer/TreatWarningsAsErrors gate and generated API
docs clean. The 6 remaining flags are heuristic false positives (MD5, UTC-4,
capacity-1, near-1601) left intact so real documentation is not corrupted.
Claude-Session: https://claude.ai/code/session_01DMXXvNuPekkkrTEyPNxEkW
Interlocking changes across the gateway server (shared GatewaySession.cs /
SessionManager.cs), committed together:
- GWC-01 (Critical): alarm monitor now attaches as an internal
(non-counted) distributor subscriber instead of a second raw drain of the
single worker event channel; WorkerClient._events -> SingleReader with a
claimed-once guard so a future dual-consumer regression throws loudly.
- GWC-02 (High): faulted sessions are swept in CloseExpiredLeasesAsync
(IsFaultedReapable + FaultedReason); new FaultedGraceSeconds (default 0).
- GWC-03 (High): configurable MaxSparseArrayLength (default 1_000_000)
enforced before allocation.
- TST-02 (High, security): StreamEvents attach now enforces the opening key
id -> PermissionDenied on owner mismatch.
- TST-12 (Medium): CLAUDE.md retention-defaults sentence corrected.
Verified: NonWindows build clean; targeted tests 135/135 on macOS, plus
WorkerClientTests 18/18 on the Windows host.
Add a nullable string? OwnerKeyId property to GatewaySession that captures
the API key identifier (KeyId) of the authenticated caller that opened the
session. Wire it through ISessionManager.OpenSessionAsync → SessionManager
→ GatewaySession constructor. The gRPC service passes identityAccessor
.Current?.KeyId; internal callers (GatewayAlarmMonitor, DashboardLiveDataService)
pass null. Covers the positive and null cases with two new TDD-first tests.
Render Fallback:Mode=ForceSubtag as a cyan 'Subtag monitoring (forced)'
badge, distinct from the amber failover 'degraded' badge, so an intentional
configuration isn't shown as a fault. Distinguished by the shared
AlarmProviderReasons.ForcedSubtag reason carried on the provider-status feed.
B1: add AlarmProviderSwitchCount to GatewayMetricsSnapshot so the switch total is
readable without scraping the OTEL counter.
B2: replace the free-text reason tag on mxgateway.alarms.provider_switches with a
bounded AlarmProviderSwitchReason enum (failover/failback/unknown); the human-readable
reason stays in the structured log.
Adds missing <summary>/<param> XML docs across 99 server, worker, and test
files so CommentChecker reports zero issues (TreatWarningsAsErrors needs the
analyzer clean). Bundles in WIP dashboard work: NavSection extraction,
MainLayout/site.css/js styling alignment, and DashboardOptions/Auth tweaks.
Apply the ZB.MOM.WW. prefix to all gateway-side projects, folders,
.csproj/.sln contents, C# namespaces, using directives, generated proto
C# (csharp_namespace + checked-in generated files), InternalsVisibleTo
attributes, project-name string literals (LoadProject, .sln lookups,
worker exe paths, staticwebassets manifest), and the install/script/doc
references that point at any of the above. Migrate the solution from
.sln to .slnx via `dotnet sln migrate` and delete the old file.
External-runtime identifiers are intentionally NOT prefixed so external
configuration keeps working:
- GatewayMetrics.cs MeterName ("MxGateway.Server")
- DashboardAuthenticationDefaults Scheme/Policy ("MxGateway.Dashboard")
- GatewayRequestLoggingMiddleware logger category ("MxGateway.Request")
- StaRuntime thread name ("MxGateway.Worker.STA")
- appsettings.json root section "MxGateway" + env-var prefix
MxGateway__... and secret-name MxGateway:ApiKeyPepper
- C:\ProgramData\MxGateway\ data dir paths
Also fixes two tests that were not rename-related but became visible
while validating the rename:
- WorkerLiveMxAccessSmokeTests.ShutDownAsync: cancellation that the
gateway service correctly maps to RpcException(Cancelled) per gRPC
convention was being misclassified as a stream fault. Added a sibling
catch on RpcException with StatusCode.Cancelled.
- IntegrationTestEnvironment.ResolveRepositoryRoot: extracted IsRepositoryRoot
and made it accept either a .git marker OR a .sln/.slnx next to src/
so the worker-exe walker works in non-git working copies.
clients/proto/proto-inputs.json's protoRoot updated to point at
src/ZB.MOM.WW.MxGateway.Contracts/Protos.
Verified by `dotnet build` and a full `dotnet test` of the .slnx with
MXGATEWAY_RUN_LIVE_{MXACCESS,LDAP,GALAXY}_TESTS=1:
Tests: 472/472 pass
Worker.Tests: 280/280 pass (4 dev-rig [Fact(Skip=...)] skipped)
IntegrationTests: 18/18 pass
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Joseph Doherty