From 09ccd9561f0e1ec3a47e664bb263d9592a93ab89 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Fri, 7 Aug 2026 06:00:16 -0400 Subject: [PATCH] docs(GWC-26): record alarm feed repairs as at-least-once; share the channel worker fake MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../2026-07-12/remediation/00-tracking.md | 1 + docs/Sessions.md | 2 + gateway.md | 19 ++++-- .../Alarms/GatewayAlarmMonitor.cs | 8 +++ .../Alarms/AlarmFailoverEndToEndTests.cs | 49 +------------- .../GatewayAlarmMonitorAttachOrderTests.cs | 48 -------------- .../GatewayAlarmMonitorProviderModeTests.cs | 49 +------------- .../TestSupport/ChannelWorkerClient.cs | 66 +++++++++++++++++++ 8 files changed, 93 insertions(+), 149 deletions(-) create mode 100644 src/ZB.MOM.WW.MxGateway.Tests/TestSupport/ChannelWorkerClient.cs diff --git a/archreview/2026-07-12/remediation/00-tracking.md b/archreview/2026-07-12/remediation/00-tracking.md index 2ced6a3..c0af6f6 100644 --- a/archreview/2026-07-12/remediation/00-tracking.md +++ b/archreview/2026-07-12/remediation/00-tracking.md @@ -162,3 +162,4 @@ Sequence these together rather than piecemeal — several are one change set spa | 2026-07-13 | Ran the TST-25 acceptance checks (scripts/ci/README.md) — they caught **two real CI defects, both fixed** on `fix/tst-25-ci-key-log-leak`: (1) **CI SSH key leaked in cleartext** in the `windows-x86` step env echo (Gitea's line-oriented masker missed the multiline PEM) — rotated the CI key on windev (old pubkey revoked), stored the key **base64-encoded** so the masker redacts it to `***` (confirmed on run #38), taught `run-windev-ci.sh` to decode, dropped the redundant public known-hosts secret from the job env; (2) **bootstrap lock race** — `run-windev-ci.sh`'s pre-hand-off `git fetch`/`checkout` ran outside the worktree lock, so concurrent runs collided on `.git/index.lock`; the bootstrap now holds the lock (ps1 re-uses it via `MXGW_CI_LOCK_HELD`), retest confirmed clean serialization. Also **deflaked** `SessionManagerTests` fail-fast timing assertions (absolute `<100ms` wall-clock bound flaked under CI load; now anchored to the configured timeout / dropped for the zero-timeout case). Checks passed: unreachable-host fast-fail (exit 255/15s), deliberate-red propagation (Worker.Tests failure → exit 1), lock concurrency (2nd run waits), no-key-in-logs (masked). Merge target `df7e20d` verified GREEN via the local windev path (Worker build + 356 tests); merged to `main` `19cbf7b`. Check 6 (forced-failure nightly issue): issue endpoint+token proven live at bring-up (#124); in-CI forced-failure probe abandoned to shared-runner congestion (residual `if: failure()` gating is standard Actions). | | 2026-07-13 | New finding **TST-30** (`Low`/`P2`) added — surfaced during TST-25 acceptance: CI runs on a single shared `gitea-runner` (`maxParallel=1`, co-located `10.100.0.35`) interleaved with `dohertj2/lmxopcua`, and Gitea 1.26 exposes no run cancel/delete, so queue latency is unbounded under cross-repo contention and the runner is a single point of failure. Design: add a second/labelled runner + document the no-cancel reality and the `run-windev-ci.sh` bypass. Roll-ups updated (Testing Low 2→3, total 47→48; P2 9→10). | | 2026-08-07 | **GWC-27 → `Done`, GWC-26 → `Done`** (branch `fix/gwc-26-27-alarm-attach`). GWC-27: `GatewaySession.AttachInternalEventSubscriber` now mirrors `AttachEventSubscriber`'s readiness gate under `_syncRoot`, before `EnsureDistributorCreated`, so a premature attach can no longer latch a poisoned distributor. GWC-26: the alarm monitor takes its internal lease directly from the session **before** `SubscribeAlarms` and drains it after the first reconcile; `ISessionManager.ReadAlarmEventsAsync` removed (zero remaining callers); `ApplyReconcile` now broadcasts an `Acknowledge` feed transition for a both-present alarm whose state advanced to `ActiveAcked` (feed-level repair on `AlarmFeedMessage`, not `MxEvent` synthesis). New tests `GatewaySessionTests.AttachInternalEventSubscriberBeforeReadyThrowsAndDoesNotPoisonDistributor` and `GatewayAlarmMonitorAttachOrderTests` (`TransitionsDuringSubscribeWindow_StillReachTheAlarmFeed`, `ApplyReconcileBroadcastsAcknowledgeDelta`); the alarm-monitor fakes now hand the monitor a real Ready `GatewaySession` with a dashboard mirror so the window is actually reproducible. Verification: NonWindows build 0 warnings/0 errors; `GatewayAlarmMonitor` 16 passed, `SessionManagerTests` 38 passed, `GatewaySessionTests` 19 passed, `AlarmFailoverEndToEndTests` 2 passed. | +| 2026-08-07 | Code review of `fix/gwc-26-27-alarm-attach` surfaced a **known pre-existing characteristic, now documented**: the alarm monitor's reconcile-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 internal lease, so both broadcast and the duplicates are indistinguishable on the alarm feed (`StreamAlarms` + dashboard alarm hub). This pre-dates GWC-26 — the Raise/Clear presence repair has always had it, since nothing serializes a reconcile pass against the in-flight live stream — so closing it (reconcile/live serialization or transition-timestamp dedup) was ruled out of scope for a P2 fix. Documented instead in `GatewayAlarmMonitor.ApplyReconcile`, `gateway.md`, and `docs/Sessions.md`, with the consumer-side contract stated explicitly (apply transitions idempotently — "set this alarm to this state", never increment/toggle). **Candidate finding for the next review cycle.** | diff --git a/docs/Sessions.md b/docs/Sessions.md index 6d76a17..65e26fe 100644 --- a/docs/Sessions.md +++ b/docs/Sessions.md @@ -201,6 +201,8 @@ The single worker event channel has exactly one direct reader: the `SessionEvent The monitor takes that lease **before** it issues `SubscribeAlarms`, so no transition window is missed: the pump has been running since `MarkReady` started the dashboard mirror, and the distributor only fans to subscribers registered at fan-out time, so a lease taken after the subscribe + first-reconcile round trips would lose every transition raised inside that window. Transitions arriving while the monitor subscribes and reconciles buffer in the lease's bounded channel and are applied after the reconcile, which is order-safe because a live transition can update an alarm the reconciled snapshot already holds. +The repair transitions the monitor's reconcile broadcasts on the alarm feed (Raise/Clear presence deltas and the acked-state delta) are **at-least-once**: a reconcile reads the worker's current state while the matching live transition may still be buffered in the lease, so both can be broadcast and the duplicates are indistinguishable — alarm-feed consumers must apply transitions idempotently. This is a property of the reconcile design, not of the buffering above. + `AttachInternalEventSubscriber` enforces the same readiness gate as `AttachEventSubscriber` — a session (or worker) that is not `Ready` throws `SessionNotReady` *before* the distributor is constructed. A premature attach would otherwise start the pump against a source that throws, completing every subscriber with that error and latching the distributor for the session's whole lifetime. Sessions open with `MxGateway:Sessions:DefaultLeaseSeconds` (default 1800) added to the open timestamp. Unary client activity refreshes the lease by the same duration. `ExtendLease` and `IsLeaseExpired` cooperate with `SessionManager.CloseExpiredLeasesAsync`, which iterates a registry snapshot and closes any session whose lease has expired with `LeaseExpiredReason`. `SessionLeaseMonitorHostedService` runs that sweep every `MxGateway:Sessions:LeaseSweepIntervalSeconds` seconds (default 30). diff --git a/gateway.md b/gateway.md index f978614..1c15cd1 100644 --- a/gateway.md +++ b/gateway.md @@ -161,13 +161,22 @@ subscribers registered at fan-out time, so attaching after the subscribe + reconcile round trips would drop every transition raised in that window — including an `Acknowledge`, which the presence-only reconcile deltas would never repair. Transitions arriving during the window buffer in the lease's bounded -channel instead. As defense in depth for any window this ordering cannot cover -(worker restart, internal-subscriber overflow disconnect), a reconcile that finds -a known alarm now reported `ActiveAcked` broadcasts an `Acknowledge` transition on -the alarm feed. That is a feed-level repair rebuilt from the worker's own -snapshot on the `StreamAlarms` surface — it is not an `MxEvent` and never reaches +channel instead. As a backstop for any window this ordering cannot cover (worker +restart, internal-subscriber overflow disconnect), a reconcile that finds a known +alarm now reported `ActiveAcked` broadcasts an `Acknowledge` transition on the +alarm feed. That is a feed-level repair rebuilt from the worker's own snapshot on +the `StreamAlarms` surface — it is not an `MxEvent` and never reaches `StreamEvents`, so the "never synthesize events" rule is untouched. +**Feed repair transitions are at-least-once, not exactly-once.** A reconcile reads +the worker's current state while the corresponding live transition may still be +buffered in the monitor's lease, so both can broadcast and the two are +indistinguishable on the feed. This applies to the acked-state delta and equally +to the older Raise/Clear presence repair: nothing serializes a reconcile pass +against the in-flight live stream. Alarm-feed consumers (`StreamAlarms` clients +and the dashboard alarm hub) must apply transitions idempotently — treat one as +"set this alarm to this state", never as an increment or a toggle. + ### Alarm providers and failover The alarm feed has two providers, both implemented worker-side: diff --git a/src/ZB.MOM.WW.MxGateway.Server/Alarms/GatewayAlarmMonitor.cs b/src/ZB.MOM.WW.MxGateway.Server/Alarms/GatewayAlarmMonitor.cs index d6312ba..0f1e1cc 100644 --- a/src/ZB.MOM.WW.MxGateway.Server/Alarms/GatewayAlarmMonitor.cs +++ b/src/ZB.MOM.WW.MxGateway.Server/Alarms/GatewayAlarmMonitor.cs @@ -530,6 +530,14 @@ public sealed class GatewayAlarmMonitor : BackgroundService, IGatewayAlarmServic // from the worker's own authoritative snapshot to repair what the live feed missed. They are // not MxEvents and never reach the gRPC StreamEvents path, so this feed-level repair does not // breach the "never synthesize events" rule, which governs MxEvent emission. + // + // Delivery semantics: feed repair transitions are AT-LEAST-ONCE, not exactly-once. A reconcile + // reads the worker's current state while the corresponding live transition may still be + // buffered in the alarm lease's channel; both then broadcast, and the two are indistinguishable + // on the feed. This is inherent to the reconcile design and pre-dates the acked-state delta + // (the Raise/Clear repair has always had it), since nothing serializes a reconcile against the + // in-flight live stream. Consumers must therefore treat alarm state idempotently — apply a + // transition as "set the alarm to this state", never as an increment or a toggle. private void ApplyReconcile(IEnumerable snapshots) { Dictionary next = new(StringComparer.Ordinal); diff --git a/src/ZB.MOM.WW.MxGateway.Tests/Alarms/AlarmFailoverEndToEndTests.cs b/src/ZB.MOM.WW.MxGateway.Tests/Alarms/AlarmFailoverEndToEndTests.cs index 5ed3994..dd0813e 100644 --- a/src/ZB.MOM.WW.MxGateway.Tests/Alarms/AlarmFailoverEndToEndTests.cs +++ b/src/ZB.MOM.WW.MxGateway.Tests/Alarms/AlarmFailoverEndToEndTests.cs @@ -1,5 +1,4 @@ using System.Diagnostics.CodeAnalysis; -using System.Runtime.CompilerServices; using System.Threading.Channels; using Google.Protobuf.WellKnownTypes; using Microsoft.Extensions.Logging.Abstractions; @@ -8,7 +7,7 @@ using ZB.MOM.WW.MxGateway.Server.Alarms; using ZB.MOM.WW.MxGateway.Server.Configuration; using ZB.MOM.WW.MxGateway.Server.Metrics; using ZB.MOM.WW.MxGateway.Server.Sessions; -using ZB.MOM.WW.MxGateway.Server.Workers; +using ZB.MOM.WW.MxGateway.Tests.TestSupport; namespace ZB.MOM.WW.MxGateway.Tests.Alarms; @@ -495,50 +494,4 @@ public sealed class AlarmFailoverEndToEndTests /// public Task ShutdownAsync(CancellationToken cancellationToken) => Task.CompletedTask; } - - /// Ready worker client whose event stream is the fake session manager's channel. - private sealed class ChannelWorkerClient(string sessionId, ChannelReader events) : IWorkerClient - { - /// - public string SessionId { get; } = sessionId; - - /// - public int? ProcessId { get; } = 4321; - - /// - public WorkerClientState State { get; } = WorkerClientState.Ready; - - /// - public DateTimeOffset LastHeartbeatAt { get; } = DateTimeOffset.UtcNow; - - /// - public Task StartAsync(CancellationToken cancellationToken) => Task.CompletedTask; - - /// - public Task InvokeAsync( - WorkerCommand command, - TimeSpan timeout, - CancellationToken cancellationToken) => Task.FromResult(new WorkerCommandReply()); - - /// - public async IAsyncEnumerable ReadEventsAsync( - [EnumeratorCancellation] CancellationToken cancellationToken) - { - await foreach (WorkerEvent workerEvent in events.ReadAllAsync(cancellationToken).ConfigureAwait(false)) - { - yield return workerEvent; - } - } - - /// - public Task ShutdownAsync(TimeSpan timeout, CancellationToken cancellationToken) => Task.CompletedTask; - - /// - public void Kill(string reason) - { - } - - /// - public ValueTask DisposeAsync() => ValueTask.CompletedTask; - } } diff --git a/src/ZB.MOM.WW.MxGateway.Tests/Alarms/GatewayAlarmMonitorAttachOrderTests.cs b/src/ZB.MOM.WW.MxGateway.Tests/Alarms/GatewayAlarmMonitorAttachOrderTests.cs index 48ad14d..061931b 100644 --- a/src/ZB.MOM.WW.MxGateway.Tests/Alarms/GatewayAlarmMonitorAttachOrderTests.cs +++ b/src/ZB.MOM.WW.MxGateway.Tests/Alarms/GatewayAlarmMonitorAttachOrderTests.cs @@ -1,5 +1,4 @@ using System.Diagnostics.CodeAnalysis; -using System.Runtime.CompilerServices; using System.Threading.Channels; using Google.Protobuf.WellKnownTypes; using Microsoft.Extensions.Logging.Abstractions; @@ -9,7 +8,6 @@ using ZB.MOM.WW.MxGateway.Server.Configuration; using ZB.MOM.WW.MxGateway.Server.Grpc; using ZB.MOM.WW.MxGateway.Server.Metrics; using ZB.MOM.WW.MxGateway.Server.Sessions; -using ZB.MOM.WW.MxGateway.Server.Workers; using ZB.MOM.WW.MxGateway.Tests.TestSupport; namespace ZB.MOM.WW.MxGateway.Tests.Alarms; @@ -467,50 +465,4 @@ public sealed class GatewayAlarmMonitorAttachOrderTests return gate; } } - - /// Worker client whose event stream is a test-driven channel. - private sealed class ChannelWorkerClient(string sessionId, ChannelReader events) : IWorkerClient - { - /// - public string SessionId { get; } = sessionId; - - /// - public int? ProcessId { get; } = 4321; - - /// - public WorkerClientState State { get; } = WorkerClientState.Ready; - - /// - public DateTimeOffset LastHeartbeatAt { get; } = DateTimeOffset.UtcNow; - - /// - public Task StartAsync(CancellationToken cancellationToken) => Task.CompletedTask; - - /// - public Task InvokeAsync( - WorkerCommand command, - TimeSpan timeout, - CancellationToken cancellationToken) => Task.FromResult(new WorkerCommandReply()); - - /// - public async IAsyncEnumerable ReadEventsAsync( - [EnumeratorCancellation] CancellationToken cancellationToken) - { - await foreach (WorkerEvent workerEvent in events.ReadAllAsync(cancellationToken).ConfigureAwait(false)) - { - yield return workerEvent; - } - } - - /// - public Task ShutdownAsync(TimeSpan timeout, CancellationToken cancellationToken) => Task.CompletedTask; - - /// - public void Kill(string reason) - { - } - - /// - public ValueTask DisposeAsync() => ValueTask.CompletedTask; - } } diff --git a/src/ZB.MOM.WW.MxGateway.Tests/Alarms/GatewayAlarmMonitorProviderModeTests.cs b/src/ZB.MOM.WW.MxGateway.Tests/Alarms/GatewayAlarmMonitorProviderModeTests.cs index 13eaa66..eef1fa3 100644 --- a/src/ZB.MOM.WW.MxGateway.Tests/Alarms/GatewayAlarmMonitorProviderModeTests.cs +++ b/src/ZB.MOM.WW.MxGateway.Tests/Alarms/GatewayAlarmMonitorProviderModeTests.cs @@ -1,6 +1,5 @@ using System.Diagnostics.CodeAnalysis; using System.Diagnostics.Metrics; -using System.Runtime.CompilerServices; using System.Threading.Channels; using Google.Protobuf.WellKnownTypes; using Microsoft.Extensions.Logging.Abstractions; @@ -10,7 +9,7 @@ using ZB.MOM.WW.MxGateway.Server.Alarms; using ZB.MOM.WW.MxGateway.Server.Configuration; using ZB.MOM.WW.MxGateway.Server.Metrics; using ZB.MOM.WW.MxGateway.Server.Sessions; -using ZB.MOM.WW.MxGateway.Server.Workers; +using ZB.MOM.WW.MxGateway.Tests.TestSupport; namespace ZB.MOM.WW.MxGateway.Tests.Alarms; @@ -808,50 +807,4 @@ public sealed class GatewayAlarmMonitorProviderModeTests /// public Task ShutdownAsync(CancellationToken cancellationToken) => Task.CompletedTask; } - - /// Ready worker client whose event stream is the fake session manager's channel. - private sealed class ChannelWorkerClient(string sessionId, ChannelReader events) : IWorkerClient - { - /// - public string SessionId { get; } = sessionId; - - /// - public int? ProcessId { get; } = 4321; - - /// - public WorkerClientState State { get; } = WorkerClientState.Ready; - - /// - public DateTimeOffset LastHeartbeatAt { get; } = DateTimeOffset.UtcNow; - - /// - public Task StartAsync(CancellationToken cancellationToken) => Task.CompletedTask; - - /// - public Task InvokeAsync( - WorkerCommand command, - TimeSpan timeout, - CancellationToken cancellationToken) => Task.FromResult(new WorkerCommandReply()); - - /// - public async IAsyncEnumerable ReadEventsAsync( - [EnumeratorCancellation] CancellationToken cancellationToken) - { - await foreach (WorkerEvent workerEvent in events.ReadAllAsync(cancellationToken).ConfigureAwait(false)) - { - yield return workerEvent; - } - } - - /// - public Task ShutdownAsync(TimeSpan timeout, CancellationToken cancellationToken) => Task.CompletedTask; - - /// - public void Kill(string reason) - { - } - - /// - public ValueTask DisposeAsync() => ValueTask.CompletedTask; - } } diff --git a/src/ZB.MOM.WW.MxGateway.Tests/TestSupport/ChannelWorkerClient.cs b/src/ZB.MOM.WW.MxGateway.Tests/TestSupport/ChannelWorkerClient.cs new file mode 100644 index 0000000..378d125 --- /dev/null +++ b/src/ZB.MOM.WW.MxGateway.Tests/TestSupport/ChannelWorkerClient.cs @@ -0,0 +1,66 @@ +using System.Runtime.CompilerServices; +using System.Threading.Channels; +using ZB.MOM.WW.MxGateway.Contracts.Proto; +using ZB.MOM.WW.MxGateway.Server.Workers; + +namespace ZB.MOM.WW.MxGateway.Tests.TestSupport; + +/// +/// Always- whose event +/// stream is a channel the test writes to. Lets a test attach a real +/// GatewaySession to a worker it drives by hand — the session can be marked Ready, +/// its SessionEventDistributor pump then drains this channel, and the test controls +/// exactly when each event is fanned. +/// +/// +/// Commands are not scripted here: returns an empty reply, because +/// the consumers of this fake route commands through their own ISessionManager double +/// rather than through the worker client. Use a purpose-built worker client instead when a +/// test needs command behavior. +/// +/// Session identifier the client reports. +/// Channel whose events yields, in order. +public sealed class ChannelWorkerClient(string sessionId, ChannelReader events) : IWorkerClient +{ + /// + public string SessionId { get; } = sessionId; + + /// + public int? ProcessId { get; } = 4321; + + /// + public WorkerClientState State { get; } = WorkerClientState.Ready; + + /// + public DateTimeOffset LastHeartbeatAt { get; } = DateTimeOffset.UtcNow; + + /// + public Task StartAsync(CancellationToken cancellationToken) => Task.CompletedTask; + + /// + public Task InvokeAsync( + WorkerCommand command, + TimeSpan timeout, + CancellationToken cancellationToken) => Task.FromResult(new WorkerCommandReply()); + + /// + public async IAsyncEnumerable ReadEventsAsync( + [EnumeratorCancellation] CancellationToken cancellationToken) + { + await foreach (WorkerEvent workerEvent in events.ReadAllAsync(cancellationToken).ConfigureAwait(false)) + { + yield return workerEvent; + } + } + + /// + public Task ShutdownAsync(TimeSpan timeout, CancellationToken cancellationToken) => Task.CompletedTask; + + /// + public void Kill(string reason) + { + } + + /// + public ValueTask DisposeAsync() => ValueTask.CompletedTask; +}