From 1a31e3f584e2a0edada0cd7c04bdb8a3cf83847e Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Wed, 11 Nov 2026 10:12:30 -0500 Subject: [PATCH 1/5] fix(galaxy): park data-change events that overtake their SubscribeBulk reply (#859) mxaccessgw gives no reply-before-event ordering: the worker completes the STA command via a RunContinuationsAsynchronously TCS, so the reply is framed on a thread-pool continuation while the STA already pumps the OnDataChange Advise queued (drained/framed by a separate loop), and the reply (unary Invoke) and the event (StreamEvents) are separate HTTP/2 streams. The driver then also registers handles concurrently with the pump's dispatch loop. MXAccess's initial value could therefore be dropped as "stale after unsubscribe", leaving the node at its pre-subscribe value until the next change. SubscriptionRegistry gains an early-event window: every SubscribeBulk call site (SubscribeAsync, ResubscribeAllAsync, PerPlatformProbeWatcher) opens BeginPendingSubscribe() before the RPC and closes it after registering. Inside the window unknown-handle events are parked (bounded, 100k); the Register/Rebind that binds a handle promotes them, and the EventPump replays them on its single dispatch thread before any later event for that handle (woken by a sentinel when no further event arrives). Closing the last window discards unclaimed events, so the stale drop is unchanged outside it. New counters galaxy.events.early_{parked,replayed,overflow}. Claude-Session: https://claude.ai/code/session_01GT67AD99zQQqpLCVm3p7xk --- docs/drivers/Galaxy.md | 1 + docs/v2/Galaxy.Performance.md | 3 + .../GalaxyDriver.cs | 14 + .../Health/PerPlatformProbeWatcher.cs | 4 + .../Runtime/EventPump.cs | 85 ++++- .../Runtime/SubscriptionRegistry.cs | 181 ++++++++++ .../Runtime/EarlyEventWindowTests.cs | 315 ++++++++++++++++++ 7 files changed, 599 insertions(+), 4 deletions(-) create mode 100644 tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Tests/Runtime/EarlyEventWindowTests.cs diff --git a/docs/drivers/Galaxy.md b/docs/drivers/Galaxy.md index b768aaed8..c7674793e 100644 --- a/docs/drivers/Galaxy.md +++ b/docs/drivers/Galaxy.md @@ -172,6 +172,7 @@ Every (re)open of `StreamAlarms` replays the gateway's active-alarm snapshot. `G - **Replay gaps** (`galaxy.events.replay_gaps`): the gateway signals that events were evicted from its replay ring before we could read them. The driver responds by re-issuing `SubscribeBulk` for every tracked subscription — MXAccess delivers an initial value per item on subscribe, so re-subscribing *is* the re-snapshot the contract asks for. It does **not** restart the event pump (the stream carrying the sentinel is healthy) and it honours `Reconnect:ReplayOnSessionLost=false` by logging and standing down, on the grounds that an operator who delegated recovery to the gateway did not ask for a re-subscribe on a different trigger. ⚠️ This cannot currently fire: the sentinel is only emitted on a stream opened with `after_worker_sequence > 0`, and the driver opens with `0`, which the gateway treats as "fresh stream, not a resume". Enabling resume is a bigger change than it looks — the sequence is only meaningful **within one gateway session**, and the reconnect path recreates the session, so a remembered sequence replayed against a fresh session would set the server's live watermark *above* every event it is about to send, silencing the stream rather than recovering it. - **Undecodable values** (`galaxy.events.undecodable`): a value kind this driver has no CLR representation for is published as **`BadTypeMismatch` with a null value**, and the first occurrence of each distinct kind is logged. It is *not* published as a null carrying the event's own quality: an MXAccess event can be Good-quality while the payload is one we cannot map, and a good null is indistinguishable from a genuine null reading — a client would consume a value it never received as a real one, with the failure visible only in a log. Non-zero here means the gateway is sending a wire type `MxValueDecoder` has no mapping for and those readings are being lost; the fix is a decoder mapping, not a quality change. +- **Initial values can overtake the subscribe reply (#859)** (`galaxy.events.early_parked` / `early_replayed` / `early_overflow`): mxaccessgw gives no reply-before-event ordering — the worker frames the SubscribeBulk reply on a thread-pool continuation while the STA is already pumping the OnDataChange that Advise queued, and the reply (unary Invoke) and the event (StreamEvents) are separate HTTP/2 streams anyway. An event for an unregistered handle is therefore only "stale after unsubscribe" when no subscribe is in flight. Every SubscribeBulk call site (`SubscribeAsync`, the reconnect/replay-gap `ResubscribeAllAsync`, `PerPlatformProbeWatcher`) opens `SubscriptionRegistry.BeginPendingSubscribe()` before the RPC and closes it after registering; inside the window unknown-handle events are parked (bounded at 100 000), promoted by the `Register`/`Rebind` that binds the handle, and replayed on the pump's dispatch thread before any later event for that handle. When the last window closes, unclaimed parked events are discarded — the stale drop is unchanged outside it. - **Duplicate `attributeRef` (#605 / 06-S-13)**: two authored RawPaths dialling the same Galaxy attribute are both dialled, but the reverse map is first-wins, so value fan-out and alarm `ConditionId` surface under the first RawPath only. The loser is logged at Warning and recorded in the skipped-tag ledger with the winner path in the reason. - **Subscribe-once reads are bounded (#605 / 06-S-3)**: `ReadAsync` on the production path (SubscribeBulk, first event, Unsubscribe) carries an internal 30 s deadline in addition to the caller token; slots that never publish come back `BadTimeout`, so `CancellationToken.None` can no longer hang on a silent tag. - **Supervisory advise is retried after a thrown failure (#605 / 06-S-12)**: the writer marks an item handle advised before the call; a thrown advise (not just a non-OK status) now unmarks it, fails the write closed and re-advises on the next write. diff --git a/docs/v2/Galaxy.Performance.md b/docs/v2/Galaxy.Performance.md index 7fabe0a2c..21aec092b 100644 --- a/docs/v2/Galaxy.Performance.md +++ b/docs/v2/Galaxy.Performance.md @@ -40,6 +40,9 @@ the metrics surface instead. | `galaxy.events.dropped` | `{event}` | MxEvents discarded because the bounded channel was full (newest-dropped) | | `galaxy.events.replay_gaps` | `{gap}` | Replay-gap sentinels received — the gateway evicted events we can never see | | `galaxy.events.undecodable` | `{event}` | Data-change events carrying a value kind this driver cannot represent | +| `galaxy.events.early_parked` | `{event}` | Data-change events that overtook their SubscribeBulk reply and were parked for replay (#859) | +| `galaxy.events.early_replayed` | `{event}` | Parked early events replayed once the reply registered their handle (#859) | +| `galaxy.events.early_overflow` | `{event}` | Early events dropped because the parking bound (100 000) was reached (#859) | The invariant is `received = dispatched + dropped + replay_gaps + (in-flight in the channel)`. A replay-gap sentinel is counted as diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/GalaxyDriver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/GalaxyDriver.cs index d1be1bb04..7263d5c45 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/GalaxyDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/GalaxyDriver.cs @@ -253,6 +253,9 @@ public sealed class GalaxyDriver /// Test seam: the number of driver subscriptions the registry still tracks. internal int TrackedSubscriptionCountForTests => _subscriptions.TrackedSubscriptionCount; + /// Test seam — the registry's parked early-event count (#859). + internal int ParkedEarlyEventCountForTests => _subscriptions.ParkedEventCount; + /// /// Server-pushed data-change notification. Fires from the /// 's background loop; handlers should be cheap (or queue @@ -993,6 +996,10 @@ public sealed class GalaxyDriver CancellationToken cancellationToken) { var tagCount = 0; + + // #859: the re-issued handles' initial values (the whole point of a re-snapshot) can + // overtake each SubscribeBulk reply; park them until the matching Rebind promotes them. + using var earlyEventWindow = _subscriptions.BeginPendingSubscribe(); foreach (var (subscriptionId, oldBindings) in entries) { if (Volatile.Read(ref _runtimeGeneration) != generation) @@ -1646,6 +1653,13 @@ public sealed class GalaxyDriver // and infrastructure callers (probe watcher, deploy watcher) hit the fallback. var requested = (int)Math.Max(0, publishingInterval.TotalMilliseconds); var bufferedIntervalMs = requested > 0 ? requested : _options.MxAccess.PublishingIntervalMs; + + // #859: MXAccess's initial value for a new handle can reach the event pump before this + // reply is registered (the gateway gives no reply-before-event ordering, and the reply and + // the event ride separate RPCs). Open the early-event window BEFORE the RPC so such events + // are parked rather than dropped as stale, and close it only after TryRegisterForGeneration + // (method scope), which promotes them for in-order replay. + using var earlyEventWindow = _subscriptions.BeginPendingSubscribe(); var results = await subscriber .SubscribeBulkAsync(dialledRefs, bufferedIntervalMs, cancellationToken) .ConfigureAwait(false); diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/Health/PerPlatformProbeWatcher.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/Health/PerPlatformProbeWatcher.cs index 0e9433b77..2173a5a00 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/Health/PerPlatformProbeWatcher.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/Health/PerPlatformProbeWatcher.cs @@ -138,6 +138,10 @@ public sealed class PerPlatformProbeWatcher : IDisposable private async Task SubscribeAndRecordAsync(IReadOnlyList platforms, CancellationToken cancellationToken) { var probeAddresses = platforms.Select(p => p + ProbeSuffix).ToArray(); + + // #859: the initial ScanState value can overtake the SubscribeBulk reply; park it until + // PublishBindingsLocked registers the handles below (window closes at method exit). + using var earlyEventWindow = _registry?.BeginPendingSubscribe(); // PR 6.3 — use the configured bufferedUpdateIntervalMs (defaults to 0 = gw cadence // when the driver hasn't overridden MxAccess.PublishingIntervalMs). Probe ScanState // changes are rare so a coarser interval is usually fine; deployments that need diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/Runtime/EventPump.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/Runtime/EventPump.cs index 7eb952343..701ab73a5 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/Runtime/EventPump.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/Runtime/EventPump.cs @@ -53,6 +53,15 @@ internal sealed class EventPump : IAsyncDisposable private static readonly Counter ValuesUndecodable = Meter.CreateCounter("galaxy.events.undecodable", unit: "{event}", description: "Data-change events carrying a value kind this driver cannot represent."); + private static readonly Counter EarlyEventsParked = + Meter.CreateCounter("galaxy.events.early_parked", unit: "{event}", + description: "Data-change events that arrived before their SubscribeBulk reply registered the item handle, parked for replay (#859)."); + private static readonly Counter EarlyEventsReplayed = + Meter.CreateCounter("galaxy.events.early_replayed", unit: "{event}", + description: "Parked early data-change events replayed once their item handle was registered (#859)."); + private static readonly Counter EarlyEventsOverflowed = + Meter.CreateCounter("galaxy.events.early_overflow", unit: "{event}", + description: "Early data-change events dropped because the parking bound was reached (#859)."); private readonly IGalaxySubscriber _subscriber; private readonly SubscriptionRegistry _registry; @@ -124,11 +133,28 @@ internal sealed class EventPump : IAsyncDisposable // value consistent). FullMode = BoundedChannelFullMode.Wait, SingleReader = true, - SingleWriter = true, + // Not single-writer: besides the stream read loop, a registration that promotes + // parked early events (#859) writes the WakeSentinel from the subscribing thread. + SingleWriter = false, }); _clientTag = new KeyValuePair("galaxy.client", clientName ?? ""); + _registry.ParkedEventsReady += WakeForReadyEvents; } + /// + /// Marker written into the channel to wake the dispatch loop after a registration promoted + /// parked early events (#859). Never dispatched itself; compared by reference. + /// + private static readonly MxEvent WakeSentinel = new(); + + /// + /// Wakes the dispatch loop so promoted early events replay even if no further event arrives. + /// A failed write is harmless: the channel is either full (the loop is busy and replays the + /// ready queue before its next data-change dispatch) or completed (the pump is dead, and the + /// next pump on this registry replays them). + /// + private void WakeForReadyEvents() => _channel.Writer.TryWrite(WakeSentinel); + /// /// Start consuming the event stream on a background task. Idempotent — second /// calls are no-ops while the loop is running. @@ -281,6 +307,11 @@ internal sealed class EventPump : IAsyncDisposable { await foreach (var ev in _channel.Reader.ReadAllAsync(ct).ConfigureAwait(false)) { + if (ReferenceEquals(ev, WakeSentinel)) + { + DispatchReadyEvents(); + continue; + } Dispatch(ev); EventsDispatched.Add(1, _clientTag); } @@ -313,16 +344,61 @@ internal sealed class EventPump : IAsyncDisposable // — alarms come from the gateway's session-less StreamAlarms feed // (GatewayGalaxyAlarmFeed). OnWriteComplete / OperationComplete / // OnBufferedDataChange are filtered out: write callers get their reply - // via the InvokeAsync round-trip, not via the event stream. + // via the InvokeAsync round-trip, not via the event stream. Still replay any + // promoted early events: a wake lost to a full channel relies on the next item. + DispatchReadyEvents(); return; } } private void DispatchDataChange(MxEvent ev) { - var subscribers = _registry.ResolveSubscribers(ev.ItemHandle); - if (subscribers.Count == 0) return; // stale event after unsubscribe — drop quietly + var outcome = _registry.ResolveOrPark(ev, out var subscribers); + // Replay promoted early events (#859) AFTER the resolve and BEFORE fanning this event out: + // any registration that let this event resolve has already moved its parked (older) events + // to the ready queue, so they reach the handler first and per-handle order is preserved. + DispatchReadyEvents(); + + switch (outcome) + { + case EarlyEventOutcome.Resolved: + break; + case EarlyEventOutcome.Parked: + // The handle's SubscribeBulk reply has not been registered yet — MXAccess's initial + // value can overtake it. Replayed when the registration lands. + EarlyEventsParked.Add(1, _clientTag); + return; + case EarlyEventOutcome.Overflow: + EarlyEventsOverflowed.Add(1, _clientTag); + return; + default: + return; // stale event after unsubscribe (no subscribe in flight) — drop quietly + } + + FanOut(ev, subscribers); + } + + /// + /// Dispatch every early event a registration promoted (#859). Runs only on the dispatch + /// thread, so it is serialised with live dispatch. A promoted event whose subscription was + /// removed again before the replay resolves to nobody and is dropped like any stale event. + /// + private void DispatchReadyEvents() + { + var ready = _registry.TakeReadyEvents(); + if (ready.Count == 0) return; + foreach (var early in ready) + { + var subscribers = _registry.ResolveSubscribers(early.ItemHandle); + if (subscribers.Count == 0) continue; + EarlyEventsReplayed.Add(1, _clientTag); + FanOut(early, subscribers); + } + } + + private void FanOut(MxEvent ev, IReadOnlyList<(long SubscriptionId, string FullReference)> subscribers) + { var snapshot = ToSnapshot(ev); foreach (var (subscriptionId, fullReference) in subscribers) { @@ -377,6 +453,7 @@ internal sealed class EventPump : IAsyncDisposable { if (_disposed) return; _disposed = true; + _registry.ParkedEventsReady -= WakeForReadyEvents; _cts.Cancel(); _channel.Writer.TryComplete(); if (_loop is not null) diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/Runtime/SubscriptionRegistry.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/Runtime/SubscriptionRegistry.cs index 1a6ac2ff9..1bb5da9c4 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/Runtime/SubscriptionRegistry.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/Runtime/SubscriptionRegistry.cs @@ -1,5 +1,6 @@ using System.Collections.Concurrent; using System.Collections.Immutable; +using ZB.MOM.WW.MxGateway.Contracts.Proto; namespace ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Runtime; @@ -79,6 +80,7 @@ internal sealed class SubscriptionRegistry _ => [subscriptionId], (_, set) => set.Add(subscriptionId)); } + PromoteParkedEvents(bindings); } /// @@ -138,6 +140,168 @@ internal sealed class SubscriptionRegistry { _bySubscriptionId.Clear(); _subscribersByItemHandle.Clear(); + lock (_parkGate) + { + _parkedByItemHandle.Clear(); + _parkedCount = 0; + _ready.Clear(); + } + } + + // ===== Early-event window (#859) ===== + // + // MXAccess delivers an initial value on Advise, and nothing orders that OnDataChange behind the + // SubscribeBulk reply the driver registers the new item handles from: + // * mxaccessgw's worker completes the STA command through a TaskCompletionSource created with + // RunContinuationsAsynchronously, so the reply is framed by a thread-pool continuation while + // the STA loop is already pumping the OnDataChange that Advise queued; the event queue is + // drained by a separate loop that frames events as soon as it is signalled + // (mxaccessgw Worker/Sta/StaCommandDispatcher.cs, Worker/Ipc/WorkerPipeSession.cs + // RunEventDrainLoopAsync). The writer's control-before-event priority only orders frames + // that are queued at the same time. + // * the reply travels back on the unary Invoke RPC and the event on the StreamEvents RPC — + // separate HTTP/2 streams with no cross-stream ordering at the client; + // * and even after the reply lands, the pump's dispatch loop runs concurrently with the code + // that turns the reply into bindings. + // So an event whose handle is not yet registered is NOT necessarily "stale after unsubscribe". + // While at least one subscribe is in flight such events are parked here (bounded) instead of + // dropped; a registration that binds a parked handle promotes its events to a ready queue that + // the EventPump's dispatch loop replays — on that same single thread, BEFORE any later event for + // the handle — so per-handle value order is preserved. When the last in-flight subscribe ends, + // whatever is still parked belonged to no subscription and is discarded, so the genuine + // stale-after-unsubscribe drop is unchanged outside the window. + + /// + /// Upper bound on events parked across all in-flight subscribes. Sized to hold the initial + /// values of a large bulk subscribe (MXAccess sends one per advised item); overflow is + /// dropped and counted by the pump, i.e. it degrades to the pre-#859 behaviour. + /// + public const int MaxParkedEvents = 100_000; + + private readonly Lock _parkGate = new(); + private readonly Dictionary> _parkedByItemHandle = new(); + private readonly Queue _ready = new(); + private int _parkedCount; + private int _pendingSubscribes; + + /// + /// Raised (outside any lock) when a registration promoted parked events to the ready queue, + /// so the can wake its dispatch loop even if no further event arrives. + /// + public event Action? ParkedEventsReady; + + /// Gets the number of subscribes currently inside their reply window. + public int PendingSubscribeCount => Volatile.Read(ref _pendingSubscribes); + + /// Gets the number of events currently parked for not-yet-registered item handles. + public int ParkedEventCount + { + get { lock (_parkGate) return _parkedCount; } + } + + /// + /// Opens the early-event window for one subscribe. Call BEFORE issuing SubscribeBulk + /// and dispose AFTER the returned handles are registered (or the attempt was abandoned): + /// while any window is open, events for unknown handles are parked rather than dropped. + /// + /// A token that closes the window when disposed (idempotent). + public IDisposable BeginPendingSubscribe() + { + Interlocked.Increment(ref _pendingSubscribes); + return new PendingSubscribeScope(this); + } + + private void EndPendingSubscribe() + { + if (Interlocked.Decrement(ref _pendingSubscribes) != 0) return; + lock (_parkGate) + { + // Re-checked under the gate: a subscribe that began after the decrement may already + // have parked its own early events, which must survive. + if (Volatile.Read(ref _pendingSubscribes) != 0) return; + _parkedByItemHandle.Clear(); + _parkedCount = 0; + } + } + + /// + /// Resolve the subscribers for 's item handle, or — when the handle is + /// unknown while a subscribe is in flight — park the event for replay. Atomic with the + /// promotion a registration performs, so an event is never parked after its handle's + /// promotion already ran (it would otherwise be stranded until the window closed). + /// + /// The data-change event. + /// The subscribers when the outcome is . + /// What happened to the event. + public EarlyEventOutcome ResolveOrPark( + MxEvent ev, out IReadOnlyList<(long SubscriptionId, string FullReference)> subscribers) + { + subscribers = ResolveSubscribers(ev.ItemHandle); + if (subscribers.Count > 0) return EarlyEventOutcome.Resolved; + if (Volatile.Read(ref _pendingSubscribes) == 0) return EarlyEventOutcome.Stale; + + lock (_parkGate) + { + // A registration may have bound the handle between the lookup above and the gate. + subscribers = ResolveSubscribers(ev.ItemHandle); + if (subscribers.Count > 0) return EarlyEventOutcome.Resolved; + if (Volatile.Read(ref _pendingSubscribes) == 0) return EarlyEventOutcome.Stale; + if (_parkedCount >= MaxParkedEvents) return EarlyEventOutcome.Overflow; + + if (!_parkedByItemHandle.TryGetValue(ev.ItemHandle, out var list)) + { + list = []; + _parkedByItemHandle[ev.ItemHandle] = list; + } + list.Add(ev); + _parkedCount++; + return EarlyEventOutcome.Parked; + } + } + + /// + /// Dequeue every event a registration promoted, in arrival order. Called only from the + /// EventPump's single dispatch thread. + /// + /// The promoted events; empty when none are waiting. + public IReadOnlyList TakeReadyEvents() + { + lock (_parkGate) + { + if (_ready.Count == 0) return []; + var taken = _ready.ToArray(); + _ready.Clear(); + return taken; + } + } + + private void PromoteParkedEvents(IReadOnlyList bindings) + { + var promoted = false; + lock (_parkGate) + { + if (_parkedCount == 0) return; + foreach (var binding in bindings) + { + if (binding.ItemHandle <= 0) continue; + if (!_parkedByItemHandle.Remove(binding.ItemHandle, out var list)) continue; + foreach (var ev in list) _ready.Enqueue(ev); + _parkedCount -= list.Count; + promoted = true; + } + } + if (promoted) ParkedEventsReady?.Invoke(); + } + + private sealed class PendingSubscribeScope(SubscriptionRegistry owner) : IDisposable + { + private int _disposed; + + /// Closes this subscribe's early-event window (once). + public void Dispose() + { + if (Interlocked.Exchange(ref _disposed, 1) == 0) owner.EndPendingSubscribe(); + } } /// Snapshot every active binding for diagnostic output. @@ -189,6 +353,7 @@ internal sealed class SubscriptionRegistry _ => [subscriptionId], (_, set) => set.Add(subscriptionId)); } + PromoteParkedEvents(newBindings); } /// @@ -227,6 +392,22 @@ internal sealed class SubscriptionRegistry } } +/// Outcome of . +internal enum EarlyEventOutcome +{ + /// The handle is registered; dispatch to the returned subscribers. + Resolved, + + /// No subscribe in flight — a genuinely stale event (after unsubscribe); drop it. + Stale, + + /// Parked until the in-flight subscribe registers the handle. + Parked, + + /// Would have been parked but was reached; dropped. + Overflow, +} + /// /// One (full reference, gw item handle) pair returned by SubscribeBulk. Item handle is /// zero or negative when the gateway rejected this individual tag (bad name, duplicate); diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Tests/Runtime/EarlyEventWindowTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Tests/Runtime/EarlyEventWindowTests.cs new file mode 100644 index 000000000..f835d7491 --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Tests/Runtime/EarlyEventWindowTests.cs @@ -0,0 +1,315 @@ +using System.Collections.Concurrent; +using System.Runtime.CompilerServices; +using System.Threading.Channels; +using Google.Protobuf.WellKnownTypes; +using Shouldly; +using Xunit; +using ZB.MOM.WW.MxGateway.Contracts.Proto; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; +using ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Config; +using ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Runtime; + +namespace ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Tests.Runtime; + +/// +/// #859 — MXAccess's initial value for a freshly advised item can reach the event stream +/// before the SubscribeBulk reply is registered: mxaccessgw frames the reply on a thread-pool +/// continuation while the STA is already pumping the OnDataChange, and the reply and the event +/// ride separate RPCs. Such an event used to be dropped as "stale after unsubscribe", leaving +/// the node at its pre-subscribe value until the next change. These tests deliver the event +/// strictly BEFORE the reply and pin that it is parked and replayed in order, while a genuinely +/// stale event (no subscribe in flight) is still dropped. +/// +public sealed class EarlyEventWindowTests +{ + private const int WaitMs = 10_000; + + private static MxEvent DataChange(int handle, double value) => new() + { + Family = MxEventFamily.OnDataChange, + ItemHandle = handle, + Value = new MxValue { DoubleValue = value }, + Quality = 192, + SourceTimestamp = Timestamp.FromDateTime(DateTime.UtcNow), + }; + + private static async Task WaitUntilAsync(Func predicate, string because) + { + var deadline = DateTime.UtcNow.AddMilliseconds(WaitMs); + while (!predicate()) + { + if (DateTime.UtcNow > deadline) throw new TimeoutException(because); + await Task.Delay(5); + } + } + + // ===== Registry ===== + + /// With no subscribe in flight an unknown handle is stale — the pre-#859 drop is kept. + [Fact] + public void Unknown_handle_without_a_pending_subscribe_is_stale() + { + var registry = new SubscriptionRegistry(); + + registry.ResolveOrPark(DataChange(7, 1), out _).ShouldBe(EarlyEventOutcome.Stale); + registry.ParkedEventCount.ShouldBe(0); + } + + /// Parked events are promoted, in arrival order, when the handle is registered. + [Fact] + public void Parked_events_are_promoted_in_order_on_register() + { + var registry = new SubscriptionRegistry(); + var woken = 0; + registry.ParkedEventsReady += () => woken++; + + using (registry.BeginPendingSubscribe()) + { + registry.ResolveOrPark(DataChange(7, 1), out _).ShouldBe(EarlyEventOutcome.Parked); + registry.ResolveOrPark(DataChange(7, 2), out _).ShouldBe(EarlyEventOutcome.Parked); + registry.ResolveOrPark(DataChange(8, 9), out _).ShouldBe(EarlyEventOutcome.Parked); + registry.TakeReadyEvents().ShouldBeEmpty(); + + registry.Register(1, [new TagBinding("Tank.Level", 7)]); + } + + woken.ShouldBe(1); + registry.TakeReadyEvents().Select(e => e.Value.DoubleValue).ShouldBe([1.0, 2.0]); + registry.TakeReadyEvents().ShouldBeEmpty(); + // Handle 8 was never registered: closing the last window discarded it. + registry.ParkedEventCount.ShouldBe(0); + } + + /// A reconnect/re-snapshot Rebind promotes the new handles' early events too. + [Fact] + public void Rebind_promotes_parked_events_for_the_new_handles() + { + var registry = new SubscriptionRegistry(); + registry.Register(1, [new TagBinding("Tank.Level", 7)]); + + using (registry.BeginPendingSubscribe()) + { + registry.ResolveOrPark(DataChange(70, 5), out _).ShouldBe(EarlyEventOutcome.Parked); + registry.Rebind(1, [new TagBinding("Tank.Level", 70)]); + } + + registry.TakeReadyEvents().Single().ItemHandle.ShouldBe(70); + } + + /// Closing one of two overlapping windows keeps the other subscribe's parked events. + [Fact] + public void Closing_one_of_two_windows_keeps_parked_events() + { + var registry = new SubscriptionRegistry(); + var first = registry.BeginPendingSubscribe(); + var second = registry.BeginPendingSubscribe(); + + registry.ResolveOrPark(DataChange(7, 1), out _).ShouldBe(EarlyEventOutcome.Parked); + first.Dispose(); + first.Dispose(); // idempotent — must not close the second window + registry.PendingSubscribeCount.ShouldBe(1); + registry.ParkedEventCount.ShouldBe(1); + + second.Dispose(); + registry.ParkedEventCount.ShouldBe(0); + registry.ResolveOrPark(DataChange(7, 1), out _).ShouldBe(EarlyEventOutcome.Stale); + } + + /// Parking is bounded; past the bound the event is dropped as overflow. + [Fact] + public void Parking_is_bounded() + { + var registry = new SubscriptionRegistry(); + using var window = registry.BeginPendingSubscribe(); + var ev = DataChange(7, 1); + for (var i = 0; i < SubscriptionRegistry.MaxParkedEvents; i++) + { + registry.ResolveOrPark(ev, out _).ShouldBe(EarlyEventOutcome.Parked); + } + + registry.ResolveOrPark(ev, out _).ShouldBe(EarlyEventOutcome.Overflow); + registry.ParkedEventCount.ShouldBe(SubscriptionRegistry.MaxParkedEvents); + } + + /// A registered handle resolves normally even while a window is open. + [Fact] + public void Registered_handle_resolves_while_a_window_is_open() + { + var registry = new SubscriptionRegistry(); + registry.Register(1, [new TagBinding("Tank.Level", 7)]); + using var window = registry.BeginPendingSubscribe(); + + registry.ResolveOrPark(DataChange(7, 1), out var subs).ShouldBe(EarlyEventOutcome.Resolved); + subs.Single().FullReference.ShouldBe("Tank.Level"); + } + + // ===== EventPump ===== + + private sealed class ChannelSubscriber : IGalaxySubscriber + { + private readonly Channel _events = Channel.CreateUnbounded(); + + public Task> SubscribeBulkAsync( + IReadOnlyList fullReferences, int bufferedUpdateIntervalMs, CancellationToken cancellationToken) + => Task.FromResult>([]); + + public Task UnsubscribeBulkAsync(IReadOnlyList itemHandles, CancellationToken cancellationToken) => Task.CompletedTask; + + public async IAsyncEnumerable StreamEventsAsync([EnumeratorCancellation] CancellationToken cancellationToken) + { + await foreach (var ev in _events.Reader.ReadAllAsync(cancellationToken).ConfigureAwait(false)) + { + yield return ev; + } + } + + public ValueTask EmitAsync(MxEvent ev) => _events.Writer.WriteAsync(ev); + } + + /// + /// An initial value delivered before registration reaches the handler once the handle is + /// registered — with NO further event on the stream (the wake path). + /// + [Fact] + public async Task Pump_replays_an_early_event_once_the_handle_registers() + { + var subscriber = new ChannelSubscriber(); + var registry = new SubscriptionRegistry(); + var delivered = new ConcurrentQueue(); + await using var pump = new EventPump(subscriber, registry, channelCapacity: 16, clientName: "Early"); + pump.OnDataChange += (_, e) => delivered.Enqueue(e); + pump.Start(); + + using (registry.BeginPendingSubscribe()) + { + await subscriber.EmitAsync(DataChange(7, 42)); + await WaitUntilAsync(() => registry.ParkedEventCount == 1, "the early event must be parked, not dropped"); + delivered.ShouldBeEmpty(); + + registry.Register(1, [new TagBinding("Tank.Level", 7)]); + } + + await WaitUntilAsync(() => !delivered.IsEmpty, "the parked initial value must be replayed"); + var args = delivered.Single(); + args.FullReference.ShouldBe("Tank.Level"); + ((double)args.Snapshot.Value!).ShouldBe(42.0); + } + + /// The replayed early value precedes a later live value for the same handle. + [Fact] + public async Task Pump_replays_early_events_before_later_live_events() + { + var subscriber = new ChannelSubscriber(); + var registry = new SubscriptionRegistry(); + var delivered = new ConcurrentQueue(); + await using var pump = new EventPump(subscriber, registry, channelCapacity: 16, clientName: "Early"); + pump.OnDataChange += (_, e) => delivered.Enqueue((double)e.Snapshot.Value!); + pump.Start(); + + using (registry.BeginPendingSubscribe()) + { + await subscriber.EmitAsync(DataChange(7, 1)); + await subscriber.EmitAsync(DataChange(7, 2)); + await WaitUntilAsync(() => registry.ParkedEventCount == 2, "both early events must be parked"); + registry.Register(1, [new TagBinding("Tank.Level", 7)]); + await subscriber.EmitAsync(DataChange(7, 3)); + } + + await WaitUntilAsync(() => delivered.Count >= 3, "all three values must be delivered"); + delivered.ShouldBe([1.0, 2.0, 3.0]); + } + + /// + /// A genuinely stale event (no subscribe in flight) is still dropped; the positive control + /// that follows it proves the pump processed it rather than not having got to it yet. + /// + [Fact] + public async Task Pump_still_drops_a_stale_event_outside_any_window() + { + var subscriber = new ChannelSubscriber(); + var registry = new SubscriptionRegistry(); + registry.Register(1, [new TagBinding("Tank.Level", 7)]); + var delivered = new ConcurrentQueue(); + await using var pump = new EventPump(subscriber, registry, channelCapacity: 16, clientName: "Early"); + pump.OnDataChange += (_, e) => delivered.Enqueue(e); + pump.Start(); + + await subscriber.EmitAsync(DataChange(99, 5)); // unsubscribed handle + await subscriber.EmitAsync(DataChange(7, 6)); // positive control, strictly after + + await WaitUntilAsync(() => !delivered.IsEmpty, "the positive control must arrive"); + delivered.Single().FullReference.ShouldBe("Tank.Level"); + registry.ParkedEventCount.ShouldBe(0); + + // Registering 99 now must not resurrect the stale event. + registry.Register(2, [new TagBinding("Other", 99)]); + registry.TakeReadyEvents().ShouldBeEmpty(); + } + + // ===== GalaxyDriver end-to-end ===== + + /// + /// A gateway fake that writes each new handle's initial value to the event stream and only + /// returns the SubscribeBulk reply once the driver's pump has consumed that event — the + /// event deterministically precedes the reply. + /// + private sealed class EventBeforeReplySubscriber(Func parkedCount) : IGalaxySubscriber + { + private readonly Channel _events = Channel.CreateUnbounded(); + private int _nextHandle = 100; + + public async Task> SubscribeBulkAsync( + IReadOnlyList fullReferences, int bufferedUpdateIntervalMs, CancellationToken cancellationToken) + { + var results = new List(); + var before = parkedCount(); + foreach (var fullRef in fullReferences) + { + var handle = Interlocked.Increment(ref _nextHandle); + await _events.Writer.WriteAsync(DataChange(handle, handle * 10.0), cancellationToken); + results.Add(new SubscribeResult { TagAddress = fullRef, ItemHandle = handle, WasSuccessful = true }); + } + + await WaitUntilAsync(() => parkedCount() >= before + fullReferences.Count, + "the initial values must reach the pump (and be parked) before the reply is returned"); + return results; + } + + public Task UnsubscribeBulkAsync(IReadOnlyList itemHandles, CancellationToken cancellationToken) => Task.CompletedTask; + + public async IAsyncEnumerable StreamEventsAsync([EnumeratorCancellation] CancellationToken cancellationToken) + { + await foreach (var ev in _events.Reader.ReadAllAsync(cancellationToken).ConfigureAwait(false)) + { + yield return ev; + } + } + } + + /// The driver publishes MXAccess's initial value even when it overtook the subscribe reply. + [Fact] + public async Task Driver_publishes_an_initial_value_that_arrived_before_the_subscribe_reply() + { + GalaxyDriver? driverRef = null; + var subscriber = new EventBeforeReplySubscriber(() => driverRef!.ParkedEarlyEventCountForTests); + using var driver = new GalaxyDriver( + "g", + new GalaxyDriverOptions( + new GalaxyGatewayOptions("https://mxgw.test:5001", "key"), + new GalaxyMxAccessOptions("OtOpcUa-A"), + new GalaxyRepositoryOptions(), + new GalaxyReconnectOptions()), + hierarchySource: null, dataReader: null, dataWriter: null, subscriber: subscriber); + driverRef = driver; + var delivered = new ConcurrentQueue(); + driver.OnDataChange += (_, e) => delivered.Enqueue(e); + + var handle = await driver.SubscribeAsync( + ["Tank.Level", "Tank.Temp"], TimeSpan.FromSeconds(1), TestContext.Current.CancellationToken); + + await WaitUntilAsync(() => delivered.Count >= 2, "both initial values must be published"); + delivered.Select(e => e.FullReference).OrderBy(r => r).ShouldBe(["Tank.Level", "Tank.Temp"]); + delivered.ShouldAllBe(e => e.SubscriptionHandle.Equals(handle)); + driver.ParkedEarlyEventCountForTests.ShouldBe(0); + } +} From f89e0cc04cbc5e3828b96680b643da7b051dba0f Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Wed, 11 Nov 2026 10:26:28 -0500 Subject: [PATCH 2/5] =?UTF-8?q?fix(galaxy):=20#859=20review=20=E2=80=94=20?= =?UTF-8?q?read=20path=20window,=20atomic=20promote,=20session=20epoch,=20?= =?UTF-8?q?per-call=20windows?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 1. ReadAsync (subscribe-once) now opens the early-event window around its SubscribeBulk + registration; an initial value that overtook the reply was dropped and the read returned BadTimeout. 2. Register/Rebind write the reverse map and promote parked events in ONE critical section under the park gate, and ResolveOrPark resolves under that gate whenever anything is held (lock-free fast path only when nothing is), so a later live event can no longer overtake the parked initial value. A resolved handle that still holds a parked list queues behind it. 3. Held events are fenced by a session epoch: each new EventPump calls StartNewEpoch(), discarding parked AND promoted-but-unreplayed events; an older pump can neither park nor replay. Item-handle reuse across a reconnect can no longer put an old-session value on a new tag. 4. ResubscribeAllAsync opens one window per subscription, not across the whole replay. 5. Promoted-but-unreplayed (ready) events count toward MaxParkedEvents. 6. Begin/End of a window run under the park gate, so N->0 discard and 0->1 open are atomic (no inherited leftovers). Documented that ParkedEventsReady may run under _registryGate/_syncLock and relies on the channel's AllowSynchronousContinuations=false. Tests use TestWait (#858); each new test was checked red against a mutation of the fix it pins. Claude-Session: https://claude.ai/code/session_01GT67AD99zQQqpLCVm3p7xk --- docs/drivers/Galaxy.md | 2 +- .../GalaxyDriver.cs | 148 +++++---- .../Runtime/EventPump.cs | 26 +- .../Runtime/SubscriptionRegistry.cs | 238 +++++++++----- .../Runtime/EarlyEventWindowTests.cs | 294 ++++++++++++++---- 5 files changed, 505 insertions(+), 203 deletions(-) diff --git a/docs/drivers/Galaxy.md b/docs/drivers/Galaxy.md index c7674793e..cc6dce90b 100644 --- a/docs/drivers/Galaxy.md +++ b/docs/drivers/Galaxy.md @@ -172,7 +172,7 @@ Every (re)open of `StreamAlarms` replays the gateway's active-alarm snapshot. `G - **Replay gaps** (`galaxy.events.replay_gaps`): the gateway signals that events were evicted from its replay ring before we could read them. The driver responds by re-issuing `SubscribeBulk` for every tracked subscription — MXAccess delivers an initial value per item on subscribe, so re-subscribing *is* the re-snapshot the contract asks for. It does **not** restart the event pump (the stream carrying the sentinel is healthy) and it honours `Reconnect:ReplayOnSessionLost=false` by logging and standing down, on the grounds that an operator who delegated recovery to the gateway did not ask for a re-subscribe on a different trigger. ⚠️ This cannot currently fire: the sentinel is only emitted on a stream opened with `after_worker_sequence > 0`, and the driver opens with `0`, which the gateway treats as "fresh stream, not a resume". Enabling resume is a bigger change than it looks — the sequence is only meaningful **within one gateway session**, and the reconnect path recreates the session, so a remembered sequence replayed against a fresh session would set the server's live watermark *above* every event it is about to send, silencing the stream rather than recovering it. - **Undecodable values** (`galaxy.events.undecodable`): a value kind this driver has no CLR representation for is published as **`BadTypeMismatch` with a null value**, and the first occurrence of each distinct kind is logged. It is *not* published as a null carrying the event's own quality: an MXAccess event can be Good-quality while the payload is one we cannot map, and a good null is indistinguishable from a genuine null reading — a client would consume a value it never received as a real one, with the failure visible only in a log. Non-zero here means the gateway is sending a wire type `MxValueDecoder` has no mapping for and those readings are being lost; the fix is a decoder mapping, not a quality change. -- **Initial values can overtake the subscribe reply (#859)** (`galaxy.events.early_parked` / `early_replayed` / `early_overflow`): mxaccessgw gives no reply-before-event ordering — the worker frames the SubscribeBulk reply on a thread-pool continuation while the STA is already pumping the OnDataChange that Advise queued, and the reply (unary Invoke) and the event (StreamEvents) are separate HTTP/2 streams anyway. An event for an unregistered handle is therefore only "stale after unsubscribe" when no subscribe is in flight. Every SubscribeBulk call site (`SubscribeAsync`, the reconnect/replay-gap `ResubscribeAllAsync`, `PerPlatformProbeWatcher`) opens `SubscriptionRegistry.BeginPendingSubscribe()` before the RPC and closes it after registering; inside the window unknown-handle events are parked (bounded at 100 000), promoted by the `Register`/`Rebind` that binds the handle, and replayed on the pump's dispatch thread before any later event for that handle. When the last window closes, unclaimed parked events are discarded — the stale drop is unchanged outside it. +- **Initial values can overtake the subscribe reply (#859)** (`galaxy.events.early_parked` / `early_replayed` / `early_overflow`): mxaccessgw gives no reply-before-event ordering — the worker frames the SubscribeBulk reply on a thread-pool continuation while the STA is already pumping the OnDataChange that Advise queued, and the reply (unary Invoke) and the event (StreamEvents) are separate HTTP/2 streams anyway. An event for an unregistered handle is therefore only "stale after unsubscribe" when no subscribe is in flight. Every SubscribeBulk call site (`SubscribeAsync`, the subscribe-once `ReadAsync` — which waits only for the initial value and would otherwise read `BadTimeout` — the reconnect/replay-gap `ResubscribeAllAsync` (one window per subscription, not across the whole replay), `PerPlatformProbeWatcher`) opens `SubscriptionRegistry.BeginPendingSubscribe()` before the RPC and closes it right after registering, never later; inside the window unknown-handle events are parked, promoted by the `Register`/`Rebind` that binds the handle — map writes and promotion are one critical section, so no later event can resolve in between — and replayed on the pump's dispatch thread before any later event for that handle. Held events (parked + promoted-but-unreplayed) are bounded at 100 000 together. When the last window closes, unclaimed parked events are discarded — the stale drop is unchanged outside it. Item handles are per gateway session and reused by the next one, so held events carry the pump's **epoch**: each new `EventPump` (one per session) discards everything held and fences the old pump, so an old-session value can never land on the tag that now owns a reused handle number. - **Duplicate `attributeRef` (#605 / 06-S-13)**: two authored RawPaths dialling the same Galaxy attribute are both dialled, but the reverse map is first-wins, so value fan-out and alarm `ConditionId` surface under the first RawPath only. The loser is logged at Warning and recorded in the skipped-tag ledger with the winner path in the reason. - **Subscribe-once reads are bounded (#605 / 06-S-3)**: `ReadAsync` on the production path (SubscribeBulk, first event, Unsubscribe) carries an internal 30 s deadline in addition to the caller token; slots that never publish come back `BadTimeout`, so `CancellationToken.None` can no longer hang on a silent tag. - **Supervisory advise is retried after a thrown failure (#605 / 06-S-12)**: the writer marks an item handle advised before the call; a thrown advise (not just a non-OK status) now unmarks it, fails the write closed and re-advises on the next write. diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/GalaxyDriver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/GalaxyDriver.cs index 7263d5c45..05f7f9c12 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/GalaxyDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/GalaxyDriver.cs @@ -996,10 +996,6 @@ public sealed class GalaxyDriver CancellationToken cancellationToken) { var tagCount = 0; - - // #859: the re-issued handles' initial values (the whole point of a re-snapshot) can - // overtake each SubscribeBulk reply; park them until the matching Rebind promotes them. - using var earlyEventWindow = _subscriptions.BeginPendingSubscribe(); foreach (var (subscriptionId, oldBindings) in entries) { if (Volatile.Read(ref _runtimeGeneration) != generation) @@ -1023,6 +1019,12 @@ public sealed class GalaxyDriver .ToArray(); if (refs.Length == 0) continue; + // #859: the re-issued handles' initial values (the whole point of a re-snapshot) can + // overtake this SubscribeBulk reply; park them until the Rebind below promotes them. + // Scoped to this one iteration (the using is disposed at the end of the loop body): a + // window held across all N round-trips would park every unrelated unknown-handle event + // for the whole replay and could exhaust the bound before the genuine early values. + using var earlyEventWindow = _subscriptions.BeginPendingSubscribe(); var results = await _subscriber! .SubscribeBulkAsync(refs, _options.MxAccess.PublishingIntervalMs, cancellationToken) .ConfigureAwait(false); @@ -1457,75 +1459,89 @@ public sealed class GalaxyDriver var bufferedIntervalMs = _options.MxAccess.PublishingIntervalMs; IReadOnlyList results; - // The RPC gets only the CALLER's token: the internal deadline bounds our wait, not the RPC, so a slow - // gateway can never surface an OperationCanceledException to a caller that did not cancel, and the - // handles it eventually creates are known (and unsubscribed below) rather than lost to a cancelled call. - var subscribeTask = readSubscriber.SubscribeBulkAsync(fullReferences, bufferedIntervalMs, callerToken); + List bindings; + + // #859: a read waits ONLY for the initial value, and that value can overtake the SubscribeBulk + // reply — dropped as stale, a slow-changing attribute would read BadTimeout. Open the early-event + // window before the RPC and close it right after registration (the finally below), not after the + // value wait: a window held for the whole wait would park every unrelated unknown-handle event. + var earlyEventWindow = _subscriptions.BeginPendingSubscribe(); try { - results = await subscribeTask.WaitAsync(cancellationToken).ConfigureAwait(false); - } - catch (OperationCanceledException) when (!callerToken.IsCancellationRequested) - { - // Internal deadline fired while the subscribe RPC was still in flight. - pump.OnDataChange -= handler; - var subscriber = readSubscriber; - var logger = _logger; - _ = subscribeTask.ContinueWith(t => + // The RPC gets only the CALLER's token: the internal deadline bounds our wait, not the RPC, so a slow + // gateway can never surface an OperationCanceledException to a caller that did not cancel, and the + // handles it eventually creates are known (and unsubscribed below) rather than lost to a cancelled call. + var subscribeTask = readSubscriber.SubscribeBulkAsync(fullReferences, bufferedIntervalMs, callerToken); + try { - if (!t.IsCompletedSuccessfully) return Task.CompletedTask; - var late = t.Result.Where(r => r.WasSuccessful && r.ItemHandle > 0).Select(r => r.ItemHandle).ToArray(); - return late.Length == 0 - ? Task.CompletedTask - : subscriber.UnsubscribeBulkAsync(late, CancellationToken.None).ContinueWith( - u => logger.LogWarning(u.Exception, - "GalaxyDriver.ReadViaSubscribeOnceAsync late UnsubscribeBulk failed for {Count} handle(s).", late.Length), - TaskContinuationOptions.OnlyOnFaulted); - }, TaskScheduler.Default).Unwrap(); - var now = DateTime.UtcNow; - var timedOut = new DataValueSnapshot[fullReferences.Count]; - for (var i = 0; i < timedOut.Length; i++) - timedOut[i] = new DataValueSnapshot(null, StatusCodeMap.BadTimeout, null, now); - return timedOut; - } - catch - { - pump.OnDataChange -= handler; - throw; - } - - // Register bindings so the pump knows to dispatch events for these handles. - // Index the SubscribeBulk results once and correlate to references in O(1) - // instead of FirstOrDefault per element (O(n²) over the batch). On the - // 50k-tag soak path this turns a 2.5G-comparison loop into a single - // Dictionary build + linear scan. - var resultIndex = BuildResultIndex(results); - var bindings = new List(fullReferences.Count); - for (var i = 0; i < fullReferences.Count; i++) - { - var fullRef = fullReferences[i]; - var itemHandle = resultIndex.TryGetValue(fullRef, out var match) && match is { WasSuccessful: true } - ? match.ItemHandle - : 0; - bindings.Add(new TagBinding(fullRef, itemHandle)); - - // Tags the gw rejected up front — complete with Bad status now so the - // wait below doesn't time out on them. - if (itemHandle <= 0 - && pendingByRef.TryGetValue(fullRef, out var rejectedTcs)) + results = await subscribeTask.WaitAsync(cancellationToken).ConfigureAwait(false); + } + catch (OperationCanceledException) when (!callerToken.IsCancellationRequested) { - rejectedTcs.TrySetResult(new DataValueSnapshot( - Value: null, - StatusCode: StatusCodeMap.Bad, - SourceTimestampUtc: null, - ServerTimestampUtc: DateTime.UtcNow)); + // Internal deadline fired while the subscribe RPC was still in flight. + pump.OnDataChange -= handler; + var subscriber = readSubscriber; + var logger = _logger; + _ = subscribeTask.ContinueWith(t => + { + if (!t.IsCompletedSuccessfully) return Task.CompletedTask; + var late = t.Result.Where(r => r.WasSuccessful && r.ItemHandle > 0).Select(r => r.ItemHandle).ToArray(); + return late.Length == 0 + ? Task.CompletedTask + : subscriber.UnsubscribeBulkAsync(late, CancellationToken.None).ContinueWith( + u => logger.LogWarning(u.Exception, + "GalaxyDriver.ReadViaSubscribeOnceAsync late UnsubscribeBulk failed for {Count} handle(s).", late.Length), + TaskContinuationOptions.OnlyOnFaulted); + }, TaskScheduler.Default).Unwrap(); + var now = DateTime.UtcNow; + var timedOut = new DataValueSnapshot[fullReferences.Count]; + for (var i = 0; i < timedOut.Length; i++) + timedOut[i] = new DataValueSnapshot(null, StatusCodeMap.BadTimeout, null, now); + return timedOut; + } + catch + { + pump.OnDataChange -= handler; + throw; + } + + // Register bindings so the pump knows to dispatch events for these handles. + // Index the SubscribeBulk results once and correlate to references in O(1) + // instead of FirstOrDefault per element (O(n²) over the batch). On the + // 50k-tag soak path this turns a 2.5G-comparison loop into a single + // Dictionary build + linear scan. + var resultIndex = BuildResultIndex(results); + bindings = new List(fullReferences.Count); + for (var i = 0; i < fullReferences.Count; i++) + { + var fullRef = fullReferences[i]; + var itemHandle = resultIndex.TryGetValue(fullRef, out var match) && match is { WasSuccessful: true } + ? match.ItemHandle + : 0; + bindings.Add(new TagBinding(fullRef, itemHandle)); + + // Tags the gw rejected up front — complete with Bad status now so the + // wait below doesn't time out on them. + if (itemHandle <= 0 + && pendingByRef.TryGetValue(fullRef, out var rejectedTcs)) + { + rejectedTcs.TrySetResult(new DataValueSnapshot( + Value: null, + StatusCode: StatusCodeMap.Bad, + SourceTimestampUtc: null, + ServerTimestampUtc: DateTime.UtcNow)); + } + } + if (!TryRegisterForGeneration(subscriptionId, bindings, generation)) + { + pump.OnDataChange -= handler; + await ReleaseOrphanedHandlesAsync(readSubscriber, bindings).ConfigureAwait(false); + throw TornDownDuringSubscribe(); } } - if (!TryRegisterForGeneration(subscriptionId, bindings, generation)) + finally { - pump.OnDataChange -= handler; - await ReleaseOrphanedHandlesAsync(readSubscriber, bindings).ConfigureAwait(false); - throw TornDownDuringSubscribe(); + earlyEventWindow.Dispose(); } try diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/Runtime/EventPump.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/Runtime/EventPump.cs index 701ab73a5..a7d5388b8 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/Runtime/EventPump.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/Runtime/EventPump.cs @@ -138,9 +138,16 @@ internal sealed class EventPump : IAsyncDisposable SingleWriter = false, }); _clientTag = new KeyValuePair("galaxy.client", clientName ?? ""); + + // One pump per gateway session: a new epoch discards whatever an earlier session left held + // (#859 review — item handles are reused across sessions), and this pump never parks or + // replays under any other epoch. + _epoch = _registry.StartNewEpoch(); _registry.ParkedEventsReady += WakeForReadyEvents; } + private readonly int _epoch; + /// /// Marker written into the channel to wake the dispatch loop after a registration promoted /// parked early events (#859). Never dispatched itself; compared by reference. @@ -149,9 +156,12 @@ internal sealed class EventPump : IAsyncDisposable /// /// Wakes the dispatch loop so promoted early events replay even if no further event arrives. + /// Runs on the registering thread, possibly under the driver's _registryGate or the probe + /// watcher's _syncLock — safe because the channel keeps the default + /// AllowSynchronousContinuations = false, so the write never runs the dispatch loop inline. /// A failed write is harmless: the channel is either full (the loop is busy and replays the - /// ready queue before its next data-change dispatch) or completed (the pump is dead, and the - /// next pump on this registry replays them). + /// ready queue before its next dispatch) or completed (the pump is dead; its held events + /// belong to a session the next pump's new epoch discards). /// private void WakeForReadyEvents() => _channel.Writer.TryWrite(WakeSentinel); @@ -353,11 +363,13 @@ internal sealed class EventPump : IAsyncDisposable private void DispatchDataChange(MxEvent ev) { - var outcome = _registry.ResolveOrPark(ev, out var subscribers); + var outcome = _registry.ResolveOrPark(ev, _epoch, out var subscribers); - // Replay promoted early events (#859) AFTER the resolve and BEFORE fanning this event out: - // any registration that let this event resolve has already moved its parked (older) events - // to the ready queue, so they reach the handler first and per-handle order is preserved. + // Replay promoted early events (#859) AFTER the resolve and BEFORE fanning this event out. + // A registration writes the reverse map and promotes its parked events in ONE critical + // section under the registry's park gate, and ResolveOrPark resolves under that gate + // whenever anything is held. So if this event resolved, every older parked event for its + // handle is already in the ready queue, and replaying it first preserves per-handle order. DispatchReadyEvents(); switch (outcome) @@ -386,7 +398,7 @@ internal sealed class EventPump : IAsyncDisposable /// private void DispatchReadyEvents() { - var ready = _registry.TakeReadyEvents(); + var ready = _registry.TakeReadyEvents(_epoch); if (ready.Count == 0) return; foreach (var early in ready) { diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/Runtime/SubscriptionRegistry.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/Runtime/SubscriptionRegistry.cs index 1bb5da9c4..ad65f06d3 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/Runtime/SubscriptionRegistry.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/Runtime/SubscriptionRegistry.cs @@ -70,17 +70,27 @@ internal sealed class SubscriptionRegistry /// The tag bindings for the subscription. public void Register(long subscriptionId, IReadOnlyList bindings) { - var entry = new SubscriptionEntry(subscriptionId, bindings); - _bySubscriptionId[subscriptionId] = entry; - foreach (var binding in bindings) + bool promoted; + // The reverse-map writes and the early-event promotion are ONE critical section under the + // park gate (#859 review): a dispatch that resolves a handle through the map must never + // run between the write and the promotion, or a later live event would overtake the + // parked initial value. ResolveOrPark takes the same gate whenever anything is held. + lock (_parkGate) { - if (binding.ItemHandle <= 0) continue; // failed gw subscribe — no events expected - _subscribersByItemHandle.AddOrUpdate( - binding.ItemHandle, - _ => [subscriptionId], - (_, set) => set.Add(subscriptionId)); + var entry = new SubscriptionEntry(subscriptionId, bindings); + _bySubscriptionId[subscriptionId] = entry; + foreach (var binding in bindings) + { + if (binding.ItemHandle <= 0) continue; // failed gw subscribe — no events expected + _subscribersByItemHandle.AddOrUpdate( + binding.ItemHandle, + _ => [subscriptionId], + (_, set) => set.Add(subscriptionId)); + } + AfterMapWriteForTests?.Invoke(); + promoted = PromoteParkedEventsLocked(bindings); } - PromoteParkedEvents(bindings); + if (promoted) ParkedEventsReady?.Invoke(); } /// @@ -143,8 +153,8 @@ internal sealed class SubscriptionRegistry lock (_parkGate) { _parkedByItemHandle.Clear(); - _parkedCount = 0; _ready.Clear(); + Volatile.Write(ref _heldCount, 0); } } @@ -170,60 +180,110 @@ internal sealed class SubscriptionRegistry // the handle — so per-handle value order is preserved. When the last in-flight subscribe ends, // whatever is still parked belonged to no subscription and is discarded, so the genuine // stale-after-unsubscribe drop is unchanged outside the window. + // + // Item handles are only meaningful within one gateway session, and a new session reuses small + // integers. Every held event therefore belongs to an EPOCH: each EventPump (one per session — + // reconnect replay recreates it) starts a new epoch, which discards everything held, and an + // event parked or replayed by a pump of an older epoch is dropped. Otherwise an old-session + // value could be promoted onto whichever tag now owns the reused handle number. /// - /// Upper bound on events parked across all in-flight subscribes. Sized to hold the initial - /// values of a large bulk subscribe (MXAccess sends one per advised item); overflow is - /// dropped and counted by the pump, i.e. it degrades to the pre-#859 behaviour. + /// Upper bound on events held (parked + promoted-but-not-yet-replayed) across all in-flight + /// subscribes. Sized to hold the initial values of a large bulk subscribe (MXAccess sends + /// one per advised item); overflow is dropped and counted by the pump, i.e. it degrades to + /// the pre-#859 behaviour. /// public const int MaxParkedEvents = 100_000; private readonly Lock _parkGate = new(); private readonly Dictionary> _parkedByItemHandle = new(); private readonly Queue _ready = new(); - private int _parkedCount; + + // Parked + ready. Written only under _parkGate; read lock-free (Volatile) by the dispatch fast path. + private int _heldCount; + + // Open windows. Written only under _parkGate (so the 0→N / N→0 transitions and the discard of + // the previous window's leftovers are atomic); read lock-free by the dispatch fast path. private int _pendingSubscribes; + private int _epoch; /// - /// Raised (outside any lock) when a registration promoted parked events to the ready queue, - /// so the can wake its dispatch loop even if no further event arrives. + /// Test seam (#859 review): invoked inside the registration critical section, between the + /// reverse-map writes and the promotion of parked events. + /// + internal Action? AfterMapWriteForTests { get; set; } + + /// + /// Raised when a registration promoted parked events to the ready queue, so the + /// can wake its dispatch loop even if no further event arrives. + /// Raised after the park gate is released but still on the registering thread, which may hold + /// the driver's _registryGate or the probe watcher's _syncLock: a handler must + /// only signal (the pump's handler is a channel TryWrite, safe there because the + /// channel keeps the default AllowSynchronousContinuations = false, so the dispatch + /// loop never runs inline under those locks). /// public event Action? ParkedEventsReady; /// Gets the number of subscribes currently inside their reply window. public int PendingSubscribeCount => Volatile.Read(ref _pendingSubscribes); - /// Gets the number of events currently parked for not-yet-registered item handles. - public int ParkedEventCount + /// Gets the number of events currently held (parked or awaiting replay). + public int ParkedEventCount => Volatile.Read(ref _heldCount); + + /// + /// Starts a new session epoch: discards every held event and makes events from any earlier + /// epoch unparkable and unreplayable. Called by each new . + /// + /// The new epoch. + public int StartNewEpoch() { - get { lock (_parkGate) return _parkedCount; } + lock (_parkGate) + { + _parkedByItemHandle.Clear(); + _ready.Clear(); + Volatile.Write(ref _heldCount, 0); + return ++_epoch; + } } /// /// Opens the early-event window for one subscribe. Call BEFORE issuing SubscribeBulk - /// and dispose AFTER the returned handles are registered (or the attempt was abandoned): - /// while any window is open, events for unknown handles are parked rather than dropped. + /// and dispose AFTER the returned handles are registered (or the attempt was abandoned) — + /// not later: while any window is open, events for unknown handles are parked, not dropped. /// /// A token that closes the window when disposed (idempotent). public IDisposable BeginPendingSubscribe() { - Interlocked.Increment(ref _pendingSubscribes); + lock (_parkGate) + { + // 0→1: anything still parked belongs to an earlier, closed window. EndPendingSubscribe + // already discards on N→0 under this same gate, so this is belt-and-braces. + if (_pendingSubscribes == 0) DiscardParkedLocked(); + Volatile.Write(ref _pendingSubscribes, _pendingSubscribes + 1); + } return new PendingSubscribeScope(this); } private void EndPendingSubscribe() { - if (Interlocked.Decrement(ref _pendingSubscribes) != 0) return; lock (_parkGate) { - // Re-checked under the gate: a subscribe that began after the decrement may already - // have parked its own early events, which must survive. - if (Volatile.Read(ref _pendingSubscribes) != 0) return; - _parkedByItemHandle.Clear(); - _parkedCount = 0; + Volatile.Write(ref _pendingSubscribes, _pendingSubscribes - 1); + // N→0: what is still parked was claimed by no registration. Promoted (ready) events are + // kept — they belong to a registered handle and are awaiting replay. + if (_pendingSubscribes == 0) DiscardParkedLocked(); } } + private void DiscardParkedLocked() + { + if (_parkedByItemHandle.Count == 0) return; + var parked = 0; + foreach (var list in _parkedByItemHandle.Values) parked += list.Count; + _parkedByItemHandle.Clear(); + Volatile.Write(ref _heldCount, _heldCount - parked); + } + /// /// Resolve the subscribers for 's item handle, or — when the handle is /// unknown while a subscribe is in flight — park the event for replay. Atomic with the @@ -231,22 +291,49 @@ internal sealed class SubscriptionRegistry /// promotion already ran (it would otherwise be stranded until the window closed). /// /// The data-change event. + /// The calling pump's epoch (). /// The subscribers when the outcome is . /// What happened to the event. public EarlyEventOutcome ResolveOrPark( - MxEvent ev, out IReadOnlyList<(long SubscriptionId, string FullReference)> subscribers) + MxEvent ev, int epoch, out IReadOnlyList<(long SubscriptionId, string FullReference)> subscribers) { - subscribers = ResolveSubscribers(ev.ItemHandle); - if (subscribers.Count > 0) return EarlyEventOutcome.Resolved; - if (Volatile.Read(ref _pendingSubscribes) == 0) return EarlyEventOutcome.Stale; - - lock (_parkGate) + // Lock-free fast path ONLY while nothing is held: then no older event for this handle can be + // waiting, so resolving through the map cannot overtake one. While anything is held, the + // resolve happens under the gate, i.e. never inside a registration's map-write→promotion + // critical section. + if (Volatile.Read(ref _heldCount) == 0) { - // A registration may have bound the handle between the lookup above and the gate. subscribers = ResolveSubscribers(ev.ItemHandle); if (subscribers.Count > 0) return EarlyEventOutcome.Resolved; if (Volatile.Read(ref _pendingSubscribes) == 0) return EarlyEventOutcome.Stale; - if (_parkedCount >= MaxParkedEvents) return EarlyEventOutcome.Overflow; + } + + lock (_parkGate) + { + if (epoch != _epoch) + { + subscribers = []; + return EarlyEventOutcome.Stale; // a dying pump of an earlier session + } + + subscribers = ResolveSubscribers(ev.ItemHandle); + if (subscribers.Count > 0) + { + // Defensive: registration promotes atomically with its map writes, so a resolved + // handle should hold no parked list — but if one ever does, queue behind it rather + // than overtake it (promotion moves the whole list in order). Not bounded: dropping + // this newer value would leave the older parked one as the node's last word. + if (_parkedByItemHandle.TryGetValue(ev.ItemHandle, out var pending)) + { + pending.Add(ev); + Volatile.Write(ref _heldCount, _heldCount + 1); + subscribers = []; + return EarlyEventOutcome.Parked; + } + return EarlyEventOutcome.Resolved; + } + if (_pendingSubscribes == 0) return EarlyEventOutcome.Stale; + if (_heldCount >= MaxParkedEvents) return EarlyEventOutcome.Overflow; if (!_parkedByItemHandle.TryGetValue(ev.ItemHandle, out var list)) { @@ -254,43 +341,42 @@ internal sealed class SubscriptionRegistry _parkedByItemHandle[ev.ItemHandle] = list; } list.Add(ev); - _parkedCount++; + Volatile.Write(ref _heldCount, _heldCount + 1); return EarlyEventOutcome.Parked; } } /// /// Dequeue every event a registration promoted, in arrival order. Called only from the - /// EventPump's single dispatch thread. + /// EventPump's single dispatch thread. A pump of an earlier epoch gets nothing. /// + /// The calling pump's epoch. /// The promoted events; empty when none are waiting. - public IReadOnlyList TakeReadyEvents() + public IReadOnlyList TakeReadyEvents(int epoch) { + if (Volatile.Read(ref _heldCount) == 0) return []; lock (_parkGate) { - if (_ready.Count == 0) return []; + if (epoch != _epoch || _ready.Count == 0) return []; var taken = _ready.ToArray(); _ready.Clear(); + Volatile.Write(ref _heldCount, _heldCount - taken.Length); return taken; } } - private void PromoteParkedEvents(IReadOnlyList bindings) + private bool PromoteParkedEventsLocked(IReadOnlyList bindings) { + if (_parkedByItemHandle.Count == 0) return false; var promoted = false; - lock (_parkGate) + foreach (var binding in bindings) { - if (_parkedCount == 0) return; - foreach (var binding in bindings) - { - if (binding.ItemHandle <= 0) continue; - if (!_parkedByItemHandle.Remove(binding.ItemHandle, out var list)) continue; - foreach (var ev in list) _ready.Enqueue(ev); - _parkedCount -= list.Count; - promoted = true; - } + if (binding.ItemHandle <= 0) continue; + if (!_parkedByItemHandle.Remove(binding.ItemHandle, out var list)) continue; + foreach (var ev in list) _ready.Enqueue(ev); // stays counted in _heldCount until replayed + promoted = true; } - if (promoted) ParkedEventsReady?.Invoke(); + return promoted; } private sealed class PendingSubscribeScope(SubscriptionRegistry owner) : IDisposable @@ -328,32 +414,38 @@ internal sealed class SubscriptionRegistry /// The new tag bindings after reconnection. public void Rebind(long subscriptionId, IReadOnlyList newBindings) { - if (!_bySubscriptionId.TryGetValue(subscriptionId, out var oldEntry)) return; - - // Drop this subscription from every reverse-map set it currently appears in. The - // pre-reconnect item handles are stale once the gw re-issues fresh ones. - foreach (var binding in oldEntry.Bindings) + bool promoted; + lock (_parkGate) // map writes + promotion atomic — see Register { - if (binding.ItemHandle <= 0) continue; - if (!_subscribersByItemHandle.TryGetValue(binding.ItemHandle, out var set)) continue; - var remaining = set.Remove(subscriptionId); - if (remaining.IsEmpty) + if (!_bySubscriptionId.TryGetValue(subscriptionId, out var oldEntry)) return; + + // Drop this subscription from every reverse-map set it currently appears in. The + // pre-reconnect item handles are stale once the gw re-issues fresh ones. + foreach (var binding in oldEntry.Bindings) { - _subscribersByItemHandle.TryRemove(binding.ItemHandle, out _); + if (binding.ItemHandle <= 0) continue; + if (!_subscribersByItemHandle.TryGetValue(binding.ItemHandle, out var set)) continue; + var remaining = set.Remove(subscriptionId); + if (remaining.IsEmpty) + { + _subscribersByItemHandle.TryRemove(binding.ItemHandle, out _); + } + else _subscribersByItemHandle[binding.ItemHandle] = remaining; } - else _subscribersByItemHandle[binding.ItemHandle] = remaining; - } - _bySubscriptionId[subscriptionId] = new SubscriptionEntry(subscriptionId, newBindings); - foreach (var binding in newBindings) - { - if (binding.ItemHandle <= 0) continue; // failed gw subscribe — no events expected - _subscribersByItemHandle.AddOrUpdate( - binding.ItemHandle, - _ => [subscriptionId], - (_, set) => set.Add(subscriptionId)); + _bySubscriptionId[subscriptionId] = new SubscriptionEntry(subscriptionId, newBindings); + foreach (var binding in newBindings) + { + if (binding.ItemHandle <= 0) continue; // failed gw subscribe — no events expected + _subscribersByItemHandle.AddOrUpdate( + binding.ItemHandle, + _ => [subscriptionId], + (_, set) => set.Add(subscriptionId)); + } + AfterMapWriteForTests?.Invoke(); + promoted = PromoteParkedEventsLocked(newBindings); } - PromoteParkedEvents(newBindings); + if (promoted) ParkedEventsReady?.Invoke(); } /// diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Tests/Runtime/EarlyEventWindowTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Tests/Runtime/EarlyEventWindowTests.cs index f835d7491..e970b6661 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Tests/Runtime/EarlyEventWindowTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Tests/Runtime/EarlyEventWindowTests.cs @@ -7,6 +7,7 @@ using Xunit; using ZB.MOM.WW.MxGateway.Contracts.Proto; using ZB.MOM.WW.OtOpcUa.Core.Abstractions; using ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Config; +using ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Health; using ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Runtime; namespace ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Tests.Runtime; @@ -22,8 +23,6 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Tests.Runtime; /// public sealed class EarlyEventWindowTests { - private const int WaitMs = 10_000; - private static MxEvent DataChange(int handle, double value) => new() { Family = MxEventFamily.OnDataChange, @@ -33,15 +32,11 @@ public sealed class EarlyEventWindowTests SourceTimestamp = Timestamp.FromDateTime(DateTime.UtcNow), }; - private static async Task WaitUntilAsync(Func predicate, string because) - { - var deadline = DateTime.UtcNow.AddMilliseconds(WaitMs); - while (!predicate()) - { - if (DateTime.UtcNow > deadline) throw new TimeoutException(because); - await Task.Delay(5); - } - } + private static GalaxyDriverOptions Opts() => new( + new GalaxyGatewayOptions("https://mxgw.test:5001", "key"), + new GalaxyMxAccessOptions("OtOpcUa-A"), + new GalaxyRepositoryOptions(), + new GalaxyReconnectOptions()); // ===== Registry ===== @@ -50,8 +45,9 @@ public sealed class EarlyEventWindowTests public void Unknown_handle_without_a_pending_subscribe_is_stale() { var registry = new SubscriptionRegistry(); + var epoch = registry.StartNewEpoch(); - registry.ResolveOrPark(DataChange(7, 1), out _).ShouldBe(EarlyEventOutcome.Stale); + registry.ResolveOrPark(DataChange(7, 1), epoch, out _).ShouldBe(EarlyEventOutcome.Stale); registry.ParkedEventCount.ShouldBe(0); } @@ -60,22 +56,23 @@ public sealed class EarlyEventWindowTests public void Parked_events_are_promoted_in_order_on_register() { var registry = new SubscriptionRegistry(); + var epoch = registry.StartNewEpoch(); var woken = 0; registry.ParkedEventsReady += () => woken++; using (registry.BeginPendingSubscribe()) { - registry.ResolveOrPark(DataChange(7, 1), out _).ShouldBe(EarlyEventOutcome.Parked); - registry.ResolveOrPark(DataChange(7, 2), out _).ShouldBe(EarlyEventOutcome.Parked); - registry.ResolveOrPark(DataChange(8, 9), out _).ShouldBe(EarlyEventOutcome.Parked); - registry.TakeReadyEvents().ShouldBeEmpty(); + registry.ResolveOrPark(DataChange(7, 1), epoch, out _).ShouldBe(EarlyEventOutcome.Parked); + registry.ResolveOrPark(DataChange(7, 2), epoch, out _).ShouldBe(EarlyEventOutcome.Parked); + registry.ResolveOrPark(DataChange(8, 9), epoch, out _).ShouldBe(EarlyEventOutcome.Parked); + registry.TakeReadyEvents(epoch).ShouldBeEmpty(); registry.Register(1, [new TagBinding("Tank.Level", 7)]); } woken.ShouldBe(1); - registry.TakeReadyEvents().Select(e => e.Value.DoubleValue).ShouldBe([1.0, 2.0]); - registry.TakeReadyEvents().ShouldBeEmpty(); + registry.TakeReadyEvents(epoch).Select(e => e.Value.DoubleValue).ShouldBe([1.0, 2.0]); + registry.TakeReadyEvents(epoch).ShouldBeEmpty(); // Handle 8 was never registered: closing the last window discarded it. registry.ParkedEventCount.ShouldBe(0); } @@ -85,15 +82,16 @@ public sealed class EarlyEventWindowTests public void Rebind_promotes_parked_events_for_the_new_handles() { var registry = new SubscriptionRegistry(); + var epoch = registry.StartNewEpoch(); registry.Register(1, [new TagBinding("Tank.Level", 7)]); using (registry.BeginPendingSubscribe()) { - registry.ResolveOrPark(DataChange(70, 5), out _).ShouldBe(EarlyEventOutcome.Parked); + registry.ResolveOrPark(DataChange(70, 5), epoch, out _).ShouldBe(EarlyEventOutcome.Parked); registry.Rebind(1, [new TagBinding("Tank.Level", 70)]); } - registry.TakeReadyEvents().Single().ItemHandle.ShouldBe(70); + registry.TakeReadyEvents(epoch).Single().ItemHandle.ShouldBe(70); } /// Closing one of two overlapping windows keeps the other subscribe's parked events. @@ -101,10 +99,11 @@ public sealed class EarlyEventWindowTests public void Closing_one_of_two_windows_keeps_parked_events() { var registry = new SubscriptionRegistry(); + var epoch = registry.StartNewEpoch(); var first = registry.BeginPendingSubscribe(); var second = registry.BeginPendingSubscribe(); - registry.ResolveOrPark(DataChange(7, 1), out _).ShouldBe(EarlyEventOutcome.Parked); + registry.ResolveOrPark(DataChange(7, 1), epoch, out _).ShouldBe(EarlyEventOutcome.Parked); first.Dispose(); first.Dispose(); // idempotent — must not close the second window registry.PendingSubscribeCount.ShouldBe(1); @@ -112,7 +111,26 @@ public sealed class EarlyEventWindowTests second.Dispose(); registry.ParkedEventCount.ShouldBe(0); - registry.ResolveOrPark(DataChange(7, 1), out _).ShouldBe(EarlyEventOutcome.Stale); + registry.ResolveOrPark(DataChange(7, 1), epoch, out _).ShouldBe(EarlyEventOutcome.Stale); + } + + /// + /// Review #6: a window opening right after the previous one closed never inherits its + /// leftovers (the N→0 discard and the 0→1 open are serialised under the park gate). + /// + [Fact] + public void A_new_window_never_inherits_the_previous_windows_leftovers() + { + var registry = new SubscriptionRegistry(); + var epoch = registry.StartNewEpoch(); + var first = registry.BeginPendingSubscribe(); + registry.ResolveOrPark(DataChange(7, 1), epoch, out _).ShouldBe(EarlyEventOutcome.Parked); + first.Dispose(); + + using var second = registry.BeginPendingSubscribe(); + registry.ParkedEventCount.ShouldBe(0); + registry.Register(1, [new TagBinding("Reused", 7)]); + registry.TakeReadyEvents(epoch).ShouldBeEmpty("the closed window's event must not reach the new owner of handle 7"); } /// Parking is bounded; past the bound the event is dropped as overflow. @@ -120,29 +138,77 @@ public sealed class EarlyEventWindowTests public void Parking_is_bounded() { var registry = new SubscriptionRegistry(); + var epoch = registry.StartNewEpoch(); using var window = registry.BeginPendingSubscribe(); var ev = DataChange(7, 1); for (var i = 0; i < SubscriptionRegistry.MaxParkedEvents; i++) { - registry.ResolveOrPark(ev, out _).ShouldBe(EarlyEventOutcome.Parked); + registry.ResolveOrPark(ev, epoch, out _).ShouldBe(EarlyEventOutcome.Parked); } - registry.ResolveOrPark(ev, out _).ShouldBe(EarlyEventOutcome.Overflow); + registry.ResolveOrPark(ev, epoch, out _).ShouldBe(EarlyEventOutcome.Overflow); registry.ParkedEventCount.ShouldBe(SubscriptionRegistry.MaxParkedEvents); } + /// Review #5: promoted-but-not-yet-replayed events still count toward the bound. + [Fact] + public void Ready_events_count_toward_the_bound() + { + var registry = new SubscriptionRegistry(); + var epoch = registry.StartNewEpoch(); + using var window = registry.BeginPendingSubscribe(); + var ev = DataChange(7, 1); + for (var i = 0; i < SubscriptionRegistry.MaxParkedEvents; i++) registry.ResolveOrPark(ev, epoch, out _); + registry.Register(1, [new TagBinding("Tank.Level", 7)]); // all promoted to ready + + registry.ResolveOrPark(DataChange(8, 1), epoch, out _).ShouldBe(EarlyEventOutcome.Overflow); + + registry.TakeReadyEvents(epoch).Count.ShouldBe(SubscriptionRegistry.MaxParkedEvents); + registry.ParkedEventCount.ShouldBe(0); + registry.ResolveOrPark(DataChange(8, 1), epoch, out _).ShouldBe(EarlyEventOutcome.Parked, + "positive control: once replayed, the room is back"); + } + /// A registered handle resolves normally even while a window is open. [Fact] public void Registered_handle_resolves_while_a_window_is_open() { var registry = new SubscriptionRegistry(); + var epoch = registry.StartNewEpoch(); registry.Register(1, [new TagBinding("Tank.Level", 7)]); using var window = registry.BeginPendingSubscribe(); - registry.ResolveOrPark(DataChange(7, 1), out var subs).ShouldBe(EarlyEventOutcome.Resolved); + registry.ResolveOrPark(DataChange(7, 1), epoch, out var subs).ShouldBe(EarlyEventOutcome.Resolved); subs.Single().FullReference.ShouldBe("Tank.Level"); } + /// + /// Review #3: held events belong to a session epoch. A new epoch (a new pump after a + /// reconnect) discards parked AND promoted-but-unreplayed events, and an older pump can + /// neither park nor replay — so a reused handle number never receives an old-session value. + /// + [Fact] + public void A_new_epoch_discards_held_events_and_fences_the_old_epoch() + { + var registry = new SubscriptionRegistry(); + var oldEpoch = registry.StartNewEpoch(); + using var window = registry.BeginPendingSubscribe(); + registry.ResolveOrPark(DataChange(7, 111), oldEpoch, out _).ShouldBe(EarlyEventOutcome.Parked); + registry.ResolveOrPark(DataChange(8, 112), oldEpoch, out _).ShouldBe(EarlyEventOutcome.Parked); + registry.Register(1, [new TagBinding("Old.A", 8)]); // 8 promoted to ready, never replayed + + var newEpoch = registry.StartNewEpoch(); + registry.ParkedEventCount.ShouldBe(0); + registry.TakeReadyEvents(newEpoch).ShouldBeEmpty(); + registry.Register(2, [new TagBinding("New.B", 7)]); // new session reuses handle 7 + registry.TakeReadyEvents(newEpoch).ShouldBeEmpty("the old session's value must not reach New.B"); + + registry.ResolveOrPark(DataChange(9, 1), oldEpoch, out _).ShouldBe(EarlyEventOutcome.Stale); + registry.TakeReadyEvents(oldEpoch).ShouldBeEmpty(); + registry.ResolveOrPark(DataChange(9, 1), newEpoch, out _).ShouldBe(EarlyEventOutcome.Parked, + "positive control: the current epoch still parks"); + } + // ===== EventPump ===== private sealed class ChannelSubscriber : IGalaxySubscriber @@ -163,7 +229,7 @@ public sealed class EarlyEventWindowTests } } - public ValueTask EmitAsync(MxEvent ev) => _events.Writer.WriteAsync(ev); + public void Emit(MxEvent ev) => _events.Writer.TryWrite(ev).ShouldBeTrue(); } /// @@ -182,14 +248,14 @@ public sealed class EarlyEventWindowTests using (registry.BeginPendingSubscribe()) { - await subscriber.EmitAsync(DataChange(7, 42)); - await WaitUntilAsync(() => registry.ParkedEventCount == 1, "the early event must be parked, not dropped"); + subscriber.Emit(DataChange(7, 42)); + await TestWait.UntilAsync(() => registry.ParkedEventCount == 1, "the early event must be parked, not dropped"); delivered.ShouldBeEmpty(); registry.Register(1, [new TagBinding("Tank.Level", 7)]); } - await WaitUntilAsync(() => !delivered.IsEmpty, "the parked initial value must be replayed"); + await TestWait.UntilAsync(() => !delivered.IsEmpty, "the parked initial value must be replayed"); var args = delivered.Single(); args.FullReference.ShouldBe("Tank.Level"); ((double)args.Snapshot.Value!).ShouldBe(42.0); @@ -208,17 +274,56 @@ public sealed class EarlyEventWindowTests using (registry.BeginPendingSubscribe()) { - await subscriber.EmitAsync(DataChange(7, 1)); - await subscriber.EmitAsync(DataChange(7, 2)); - await WaitUntilAsync(() => registry.ParkedEventCount == 2, "both early events must be parked"); + subscriber.Emit(DataChange(7, 1)); + subscriber.Emit(DataChange(7, 2)); + await TestWait.UntilAsync(() => registry.ParkedEventCount == 2, "both early events must be parked"); registry.Register(1, [new TagBinding("Tank.Level", 7)]); - await subscriber.EmitAsync(DataChange(7, 3)); + subscriber.Emit(DataChange(7, 3)); } - await WaitUntilAsync(() => delivered.Count >= 3, "all three values must be delivered"); + await TestWait.UntilAsync(() => delivered.Count >= 3, "all three values must be delivered", + () => string.Join(",", delivered)); delivered.ShouldBe([1.0, 2.0, 3.0]); } + /// + /// Review #2: a live event arriving while a registration has written the reverse map but not + /// yet promoted (a 50k-binding Register is milliseconds of work) must NOT overtake the parked + /// initial value. The hook runs inside that window, emits the newer value and gives the pump + /// ample time to (wrongly) dispatch it; the pump must instead wait for the promotion. + /// + [Fact] + public async Task Pump_does_not_let_a_live_event_overtake_a_registration_in_progress() + { + var subscriber = new ChannelSubscriber(); + var registry = new SubscriptionRegistry(); + var delivered = new ConcurrentQueue(); + await using var pump = new EventPump(subscriber, registry, channelCapacity: 16, clientName: "Early"); + pump.OnDataChange += (_, e) => delivered.Enqueue((double)e.Snapshot.Value!); + pump.Start(); + + var deliveredDuringRegistration = -1; + using (registry.BeginPendingSubscribe()) + { + subscriber.Emit(DataChange(7, 1)); + await TestWait.UntilAsync(() => registry.ParkedEventCount == 1, "the initial value must be parked"); + + registry.AfterMapWriteForTests = () => + { + subscriber.Emit(DataChange(7, 2)); + Thread.Sleep(300); + deliveredDuringRegistration = delivered.Count; + }; + registry.Register(1, [new TagBinding("Tank.Level", 7)]); + registry.AfterMapWriteForTests = null; + } + + await TestWait.UntilAsync(() => delivered.Count >= 2, "both values must be delivered", + () => string.Join(",", delivered)); + deliveredDuringRegistration.ShouldBe(0, "nothing may dispatch for the handle mid-registration"); + delivered.ShouldBe([1.0, 2.0]); + } + /// /// A genuinely stale event (no subscribe in flight) is still dropped; the positive control /// that follows it proves the pump processed it rather than not having got to it yet. @@ -234,35 +339,72 @@ public sealed class EarlyEventWindowTests pump.OnDataChange += (_, e) => delivered.Enqueue(e); pump.Start(); - await subscriber.EmitAsync(DataChange(99, 5)); // unsubscribed handle - await subscriber.EmitAsync(DataChange(7, 6)); // positive control, strictly after + subscriber.Emit(DataChange(99, 5)); // unsubscribed handle + subscriber.Emit(DataChange(7, 6)); // positive control, strictly after - await WaitUntilAsync(() => !delivered.IsEmpty, "the positive control must arrive"); + await TestWait.UntilAsync(() => !delivered.IsEmpty, "the positive control must arrive"); delivered.Single().FullReference.ShouldBe("Tank.Level"); registry.ParkedEventCount.ShouldBe(0); // Registering 99 now must not resurrect the stale event. registry.Register(2, [new TagBinding("Other", 99)]); - registry.TakeReadyEvents().ShouldBeEmpty(); + registry.ParkedEventCount.ShouldBe(0); } - // ===== GalaxyDriver end-to-end ===== + /// + /// Review #3, pump level: an event the old session's pump parked is never replayed by the + /// next session's pump onto the tag that now owns the reused handle number. + /// + [Fact] + public async Task Pump_after_reconnect_never_replays_an_old_session_event_onto_a_reused_handle() + { + var registry = new SubscriptionRegistry(); + var delivered = new ConcurrentQueue(); + using var window = registry.BeginPendingSubscribe(); // e.g. a subscribe hanging across the reconnect + + var oldStream = new ChannelSubscriber(); + var oldPump = new EventPump(oldStream, registry, channelCapacity: 16, clientName: "Old"); + oldPump.Start(); + oldStream.Emit(DataChange(7, 111)); + await TestWait.UntilAsync(() => registry.ParkedEventCount == 1, "the old session's event must be parked"); + await oldPump.DisposeAsync(); + + var newStream = new ChannelSubscriber(); + await using var newPump = new EventPump(newStream, registry, channelCapacity: 16, clientName: "New"); + newPump.OnDataChange += (_, e) => delivered.Enqueue(e); + newPump.Start(); + + registry.Register(2, [new TagBinding("New.Tag", 7)]); // new session reuses handle 7 + newStream.Emit(DataChange(7, 222)); // positive control on the new session + + await TestWait.UntilAsync(() => !delivered.IsEmpty, "the new session's value must arrive"); + await TestWait.UntilAsync(() => registry.ParkedEventCount == 0, "nothing may stay held"); + delivered.Select(e => (double)e.Snapshot.Value!).ShouldBe([222.0]); + } + + // ===== GalaxyDriver / probe watcher end-to-end ===== /// /// A gateway fake that writes each new handle's initial value to the event stream and only - /// returns the SubscribeBulk reply once the driver's pump has consumed that event — the + /// returns the SubscribeBulk reply once the consumer's pump has parked that event — the /// event deterministically precedes the reply. /// - private sealed class EventBeforeReplySubscriber(Func parkedCount) : IGalaxySubscriber + private sealed class EventBeforeReplySubscriber : IGalaxySubscriber { private readonly Channel _events = Channel.CreateUnbounded(); private int _nextHandle = 100; + /// Gets or sets the consumer's held-event count, polled before replying. + public Func ParkedCount { get; set; } = () => 0; + + /// Gets the handles unsubscribed so far. + public ConcurrentQueue Unsubscribed { get; } = new(); + public async Task> SubscribeBulkAsync( IReadOnlyList fullReferences, int bufferedUpdateIntervalMs, CancellationToken cancellationToken) { var results = new List(); - var before = parkedCount(); + var before = ParkedCount(); foreach (var fullRef in fullReferences) { var handle = Interlocked.Increment(ref _nextHandle); @@ -270,12 +412,17 @@ public sealed class EarlyEventWindowTests results.Add(new SubscribeResult { TagAddress = fullRef, ItemHandle = handle, WasSuccessful = true }); } - await WaitUntilAsync(() => parkedCount() >= before + fullReferences.Count, - "the initial values must reach the pump (and be parked) before the reply is returned"); + await TestWait.UntilAsync(() => ParkedCount() >= before + fullReferences.Count, + "the initial values must reach the pump (and be parked) before the reply is returned", + () => $"held={ParkedCount()} before={before}"); return results; } - public Task UnsubscribeBulkAsync(IReadOnlyList itemHandles, CancellationToken cancellationToken) => Task.CompletedTask; + public Task UnsubscribeBulkAsync(IReadOnlyList itemHandles, CancellationToken cancellationToken) + { + foreach (var h in itemHandles) Unsubscribed.Enqueue(h); + return Task.CompletedTask; + } public async IAsyncEnumerable StreamEventsAsync([EnumeratorCancellation] CancellationToken cancellationToken) { @@ -290,26 +437,61 @@ public sealed class EarlyEventWindowTests [Fact] public async Task Driver_publishes_an_initial_value_that_arrived_before_the_subscribe_reply() { - GalaxyDriver? driverRef = null; - var subscriber = new EventBeforeReplySubscriber(() => driverRef!.ParkedEarlyEventCountForTests); + var subscriber = new EventBeforeReplySubscriber(); using var driver = new GalaxyDriver( - "g", - new GalaxyDriverOptions( - new GalaxyGatewayOptions("https://mxgw.test:5001", "key"), - new GalaxyMxAccessOptions("OtOpcUa-A"), - new GalaxyRepositoryOptions(), - new GalaxyReconnectOptions()), - hierarchySource: null, dataReader: null, dataWriter: null, subscriber: subscriber); - driverRef = driver; + "g", Opts(), hierarchySource: null, dataReader: null, dataWriter: null, subscriber: subscriber); + subscriber.ParkedCount = () => driver.ParkedEarlyEventCountForTests; var delivered = new ConcurrentQueue(); driver.OnDataChange += (_, e) => delivered.Enqueue(e); var handle = await driver.SubscribeAsync( ["Tank.Level", "Tank.Temp"], TimeSpan.FromSeconds(1), TestContext.Current.CancellationToken); - await WaitUntilAsync(() => delivered.Count >= 2, "both initial values must be published"); + await TestWait.UntilAsync(() => delivered.Count >= 2, "both initial values must be published"); delivered.Select(e => e.FullReference).OrderBy(r => r).ShouldBe(["Tank.Level", "Tank.Temp"]); delivered.ShouldAllBe(e => e.SubscriptionHandle.Equals(handle)); driver.ParkedEarlyEventCountForTests.ShouldBe(0); } + + /// + /// Review #1: the subscribe-once read waits ONLY for the initial value, so an initial value + /// that overtakes the reply must still complete the read Good — not BadTimeout. + /// + [Fact] + public async Task Driver_read_returns_an_initial_value_that_arrived_before_the_subscribe_reply() + { + var subscriber = new EventBeforeReplySubscriber(); + using var driver = new GalaxyDriver( + "g", Opts(), hierarchySource: null, dataReader: null, dataWriter: null, subscriber: subscriber); + subscriber.ParkedCount = () => driver.ParkedEarlyEventCountForTests; + + var snapshots = await TestWait.ForAsync( + driver.ReadAsync(["Tank.Level"], TestContext.Current.CancellationToken), + "the read must complete"); + + var snapshot = snapshots.ShouldHaveSingleItem(); + snapshot.StatusCode.ShouldBe(StatusCodeMap.Good); + ((double)snapshot.Value!).ShouldBe(1010.0); + driver.ParkedEarlyEventCountForTests.ShouldBe(0); + } + + /// The probe watcher's initial ScanState overtaking its reply still reaches the fan-out. + [Fact] + public async Task Probe_watcher_initial_value_that_arrived_before_the_reply_is_dispatched() + { + var registry = new SubscriptionRegistry(); + var subscriber = new EventBeforeReplySubscriber { ParkedCount = () => registry.ParkedEventCount }; + var delivered = new ConcurrentQueue(); + await using var pump = new EventPump(subscriber, registry, channelCapacity: 16, clientName: "Probe"); + pump.OnDataChange += (_, e) => delivered.Enqueue(e); + pump.Start(); + using var watcher = new PerPlatformProbeWatcher(subscriber, new HostStatusAggregator(), registry); + + await watcher.SyncPlatformsAsync(["PlatformA"], TestContext.Current.CancellationToken); + + await TestWait.UntilAsync(() => !delivered.IsEmpty, "the initial ScanState must be dispatched"); + var args = delivered.Single(); + args.FullReference.ShouldBe("PlatformA.ScanState"); + ((GalaxySubscriptionHandle)args.SubscriptionHandle).SubscriptionId.ShouldBe(SubscriptionRegistry.ProbeSubscriptionId); + } } From d08e9002037fdb0334a215bbfd72b0afe8792e8c Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Wed, 11 Nov 2026 10:41:16 -0500 Subject: [PATCH 3/5] =?UTF-8?q?fix(galaxy):=20#859=20second=20review=20+?= =?UTF-8?q?=20#861=20=E2=80=94=20window-sequence=20trim,=20per-session=20h?= =?UTF-8?q?andles,=20straddling=20subscribes?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit I-2: unclaimed parked events are trimmed by window sequence. Each window gets a monotonically increasing sequence at Begin, each parked event is stamped with the newest one, and on every End events stamped before the oldest open window's sequence are dropped (amortised O(1) via a park-order queue). Under concurrent reads the open count rarely hit 0, so trailing events used to pile up to the bound and push genuine early values into overflow. I-1 / #861: SubscriptionRegistry.StartNewSession() — called FIRST in ReplayAsync on every reconnect — clears held events, invalidates the reverse map (subscriptions kept) and bumps a session number. New-session events whose number an old binding still held now park until the replay's per-subscription Rebind promotes them, instead of resolving to the wrong tag. TryRegister / TryRebind refuse a stale session atomically under the park gate: a SubscribeAsync whose reply straddled a reconnect is re-issued on the new session (max 3 attempts), a subscribe-once read returns BadCommunicationError, ResubscribeAllAsync stops if the session changes again. Old-session handle numbers are never unsubscribed (Remove reports whether the entry belongs to the current session; the read's late-reply path checks too), and an old session's pump can no longer resolve through the map (fast path checks the session). I-3 (interim): a completed UnsubscribeBulk purges events parked for its handles (driver + probe watcher). Full fix needs a worker sequence on the SubscribeBulk reply (mxaccessgw, optional) — documented. M-1: a resolved handle with a stranded parked list promotes it and resolves the live event (never parks it). M-2: ResolveOrPark returns the ready events to replay first, one gate acquisition per event while anything is held. M-3: SubscribeAsync/read dispose the window before the orphan-release RPC. M-5: ResolveOrPark remarks document the single-dispatching-pump invariant. Tests (TestWait): overlapping windows don't accumulate, purge, stranded list, session fence + map invalidation, stale-session registration refused, end-to-end replay with swapped handle numbers and event-before-reply, subscribe and read straddling a reconnect, read deadline closes its window. Each was checked red against a mutation of the fix it pins. Refs #859, #861. Claude-Session: https://claude.ai/code/session_01GT67AD99zQQqpLCVm3p7xk --- docs/drivers/Galaxy.md | 2 +- .../GalaxyDriver.cs | 188 ++++++-- .../Health/PerPlatformProbeWatcher.cs | 6 + .../Runtime/EventPump.cs | 31 +- .../Runtime/SubscriptionRegistry.cs | 431 ++++++++++++------ .../Runtime/EarlyEventWindowTests.cs | 362 +++++++++++++-- 6 files changed, 789 insertions(+), 231 deletions(-) diff --git a/docs/drivers/Galaxy.md b/docs/drivers/Galaxy.md index cc6dce90b..0022abed1 100644 --- a/docs/drivers/Galaxy.md +++ b/docs/drivers/Galaxy.md @@ -172,7 +172,7 @@ Every (re)open of `StreamAlarms` replays the gateway's active-alarm snapshot. `G - **Replay gaps** (`galaxy.events.replay_gaps`): the gateway signals that events were evicted from its replay ring before we could read them. The driver responds by re-issuing `SubscribeBulk` for every tracked subscription — MXAccess delivers an initial value per item on subscribe, so re-subscribing *is* the re-snapshot the contract asks for. It does **not** restart the event pump (the stream carrying the sentinel is healthy) and it honours `Reconnect:ReplayOnSessionLost=false` by logging and standing down, on the grounds that an operator who delegated recovery to the gateway did not ask for a re-subscribe on a different trigger. ⚠️ This cannot currently fire: the sentinel is only emitted on a stream opened with `after_worker_sequence > 0`, and the driver opens with `0`, which the gateway treats as "fresh stream, not a resume". Enabling resume is a bigger change than it looks — the sequence is only meaningful **within one gateway session**, and the reconnect path recreates the session, so a remembered sequence replayed against a fresh session would set the server's live watermark *above* every event it is about to send, silencing the stream rather than recovering it. - **Undecodable values** (`galaxy.events.undecodable`): a value kind this driver has no CLR representation for is published as **`BadTypeMismatch` with a null value**, and the first occurrence of each distinct kind is logged. It is *not* published as a null carrying the event's own quality: an MXAccess event can be Good-quality while the payload is one we cannot map, and a good null is indistinguishable from a genuine null reading — a client would consume a value it never received as a real one, with the failure visible only in a log. Non-zero here means the gateway is sending a wire type `MxValueDecoder` has no mapping for and those readings are being lost; the fix is a decoder mapping, not a quality change. -- **Initial values can overtake the subscribe reply (#859)** (`galaxy.events.early_parked` / `early_replayed` / `early_overflow`): mxaccessgw gives no reply-before-event ordering — the worker frames the SubscribeBulk reply on a thread-pool continuation while the STA is already pumping the OnDataChange that Advise queued, and the reply (unary Invoke) and the event (StreamEvents) are separate HTTP/2 streams anyway. An event for an unregistered handle is therefore only "stale after unsubscribe" when no subscribe is in flight. Every SubscribeBulk call site (`SubscribeAsync`, the subscribe-once `ReadAsync` — which waits only for the initial value and would otherwise read `BadTimeout` — the reconnect/replay-gap `ResubscribeAllAsync` (one window per subscription, not across the whole replay), `PerPlatformProbeWatcher`) opens `SubscriptionRegistry.BeginPendingSubscribe()` before the RPC and closes it right after registering, never later; inside the window unknown-handle events are parked, promoted by the `Register`/`Rebind` that binds the handle — map writes and promotion are one critical section, so no later event can resolve in between — and replayed on the pump's dispatch thread before any later event for that handle. Held events (parked + promoted-but-unreplayed) are bounded at 100 000 together. When the last window closes, unclaimed parked events are discarded — the stale drop is unchanged outside it. Item handles are per gateway session and reused by the next one, so held events carry the pump's **epoch**: each new `EventPump` (one per session) discards everything held and fences the old pump, so an old-session value can never land on the tag that now owns a reused handle number. +- **Initial values can overtake the subscribe reply (#859)** (`galaxy.events.early_parked` / `early_replayed` / `early_overflow`): mxaccessgw gives no reply-before-event ordering — the worker frames the SubscribeBulk reply on a thread-pool continuation while the STA is already pumping the OnDataChange that Advise queued, and the reply (unary Invoke) and the event (StreamEvents) are separate HTTP/2 streams anyway. An event for an unregistered handle is therefore only "stale after unsubscribe" when no subscribe is in flight. Every SubscribeBulk call site (`SubscribeAsync`, the subscribe-once `ReadAsync` — which waits only for the initial value and would otherwise read `BadTimeout` — the reconnect/replay-gap `ResubscribeAllAsync` (one window per subscription, not across the whole replay), `PerPlatformProbeWatcher`) opens `SubscriptionRegistry.BeginPendingSubscribe()` before the RPC and closes it right after registering, never later; inside the window unknown-handle events are parked, promoted by the `Register`/`Rebind` that binds the handle — map writes and promotion are one critical section, so no later event can resolve in between — and replayed on the pump's dispatch thread before any later event for that handle. Held events (parked + promoted-but-unreplayed) are bounded at 100 000 together. Unclaimed parked events are trimmed by **window sequence**, not only when every window has closed (concurrent reads keep some window open almost permanently): each window gets a monotonically increasing sequence when it opens, each parked event is stamped with the newest sequence, and once every window that opened at or before that stamp has closed the event is discarded — outside any window the stale drop is unchanged. **Sessions (#861):** item handles are per gateway session and the next session reuses the numbers. On reconnect `ReplayAsync` first calls `SubscriptionRegistry.StartNewSession()`, which discards everything held, **invalidates the reverse map** (subscriptions are kept; new-session events park until the replay's per-subscription `Rebind` promotes them, instead of resolving to the tag that owned the number before), and bumps a session number. Registration and rebind are refused for a stale session, so a `SubscribeAsync` whose reply straddled the reconnect is **re-issued on the new session** (up to 3 attempts), a subscribe-once read returns `BadCommunicationError`, and dead old-session handle numbers are never sent to `UnsubscribeBulk`; an old session's pump can no longer resolve through the map. **Interim (I-3):** a completed `UnsubscribeBulk` purges anything parked for its handles, so a trailing event of a removed item is not promoted onto a reused handle number within one session. It cannot catch an event still in flight after the unsubscribe, and it also drops a genuine early value for a number reused that fast (a timeout rather than another tag's value). The complete fix needs a worker sequence on the SubscribeBulk reply so the driver can tell pre-subscribe events from post-subscribe ones — an optional cross-repo change in mxaccessgw. - **Duplicate `attributeRef` (#605 / 06-S-13)**: two authored RawPaths dialling the same Galaxy attribute are both dialled, but the reverse map is first-wins, so value fan-out and alarm `ConditionId` surface under the first RawPath only. The loser is logged at Warning and recorded in the skipped-tag ledger with the winner path in the reason. - **Subscribe-once reads are bounded (#605 / 06-S-3)**: `ReadAsync` on the production path (SubscribeBulk, first event, Unsubscribe) carries an internal 30 s deadline in addition to the caller token; slots that never publish come back `BadTimeout`, so `CancellationToken.None` can no longer hang on a silent tag. - **Supervisory advise is retried after a thrown failure (#605 / 06-S-12)**: the writer marks an item handle advised before the call; a thrown advise (not just a non-OK status) now unmarks it, fails the write closed and re-advises on the next write. diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/GalaxyDriver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/GalaxyDriver.cs index 05f7f9c12..7e77a70ce 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/GalaxyDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/GalaxyDriver.cs @@ -256,6 +256,9 @@ public sealed class GalaxyDriver /// Test seam — the registry's parked early-event count (#859). internal int ParkedEarlyEventCountForTests => _subscriptions.ParkedEventCount; + /// Test seam — the driver's subscription registry (#859/#861 window + session tests). + internal SubscriptionRegistry SubscriptionsForTests => _subscriptions; + /// /// Server-pushed data-change notification. Fires from the /// 's background loop; handlers should be cheap (or queue @@ -644,13 +647,37 @@ public sealed class GalaxyDriver /// Register only while is still the /// live runtime. Atomic with the teardown's registry Clear (both under ). /// - private bool TryRegisterForGeneration(long subscriptionId, IReadOnlyList bindings, int generation) + private RegisterOutcome TryRegisterForGeneration( + long subscriptionId, IReadOnlyList bindings, int generation, int session) { lock (_registryGate) { - if (Volatile.Read(ref _runtimeGeneration) != generation) return false; - _subscriptions.Register(subscriptionId, bindings); - return true; + if (Volatile.Read(ref _runtimeGeneration) != generation) return RegisterOutcome.TornDown; + // #861: the reply may have straddled a reconnect — its handles then belong to the dead + // session and collide with the new session's numbers. Checked atomically with + // StartNewSession inside the registry. + return _subscriptions.TryRegister(subscriptionId, bindings, session) + ? RegisterOutcome.Registered + : RegisterOutcome.SessionChanged; + } + } + + private enum RegisterOutcome { Registered, TornDown, SessionChanged } + + /// + /// UnsubscribeBulk for , then purge any event parked for them + /// (#859 I-3 interim — a trailing event must not be promoted onto a reused handle number). + /// + private async Task UnsubscribeAndPurgeAsync( + IGalaxySubscriber subscriber, IReadOnlyList itemHandles, CancellationToken cancellationToken) + { + try + { + await subscriber.UnsubscribeBulkAsync(itemHandles, cancellationToken).ConfigureAwait(false); + } + finally + { + _subscriptions.PurgeParked(itemHandles); } } @@ -667,7 +694,7 @@ public sealed class GalaxyDriver { var live = bindings.Where(b => b.ItemHandle > 0).Select(b => b.ItemHandle).ToArray(); if (live.Length == 0) return; - try { await subscriber.UnsubscribeBulkAsync(live, CancellationToken.None).ConfigureAwait(false); } + try { await UnsubscribeAndPurgeAsync(subscriber, live, CancellationToken.None).ConfigureAwait(false); } catch (Exception ex) { _logger.LogWarning(ex, @@ -945,6 +972,15 @@ public sealed class GalaxyDriver // anyway would start a fresh pump underneath the shutdown. cancellationToken.ThrowIfCancellationRequested(); if (_subscriber is null) return; + + // The session was recreated: every item handle the registry holds is dead and its number + // may be reused by the new session (#859 I-1 / #861). Invalidate the reverse map (the + // subscriptions stay; the per-iteration Rebind below re-binds each one, and new-session + // events park meanwhile instead of resolving to the old owner of a reused number), discard + // held events, and fence any subscribe whose reply straddled the reconnect. Done FIRST and + // unconditionally — before the pump restart and even with nothing to replay. + _subscriptions.StartNewSession(); + var entries = _subscriptions.SnapshotEntries(); if (entries.Count == 0) return; @@ -1025,6 +1061,7 @@ public sealed class GalaxyDriver // window held across all N round-trips would park every unrelated unknown-handle event // for the whole replay and could exhaust the bound before the genuine early values. using var earlyEventWindow = _subscriptions.BeginPendingSubscribe(); + var session = _subscriptions.CurrentSession; var results = await _subscriber! .SubscribeBulkAsync(refs, _options.MxAccess.PublishingIntervalMs, cancellationToken) .ConfigureAwait(false); @@ -1045,7 +1082,15 @@ public sealed class GalaxyDriver lock (_registryGate) { if (Volatile.Read(ref _runtimeGeneration) != generation) return tagCount; - _subscriptions.Rebind(subscriptionId, newBindings); + // #861: another reconnect during this round-trip makes these handles dead too; the + // newer session's own replay re-subscribes everything, so stop here. + if (!_subscriptions.TryRebind(subscriptionId, newBindings, session)) + { + _logger.LogDebug( + "GalaxyDriver {InstanceId} re-subscribe abandoned — the gateway session changed again.", + _driverInstanceId); + return tagCount; + } } tagCount += refs.Length; } @@ -1466,6 +1511,7 @@ public sealed class GalaxyDriver // window before the RPC and close it right after registration (the finally below), not after the // value wait: a window held for the whole wait would park every unrelated unknown-handle event. var earlyEventWindow = _subscriptions.BeginPendingSubscribe(); + var session = _subscriptions.CurrentSession; // #861 — the session this SubscribeBulk is issued on try { // The RPC gets only the CALLER's token: the internal deadline bounds our wait, not the RPC, so a slow @@ -1485,10 +1531,13 @@ public sealed class GalaxyDriver _ = subscribeTask.ContinueWith(t => { if (!t.IsCompletedSuccessfully) return Task.CompletedTask; + // A late reply from a session that has since been replaced carries dead handle + // numbers the new session may reuse — never unsubscribe those (#861). + if (_subscriptions.CurrentSession != session) return Task.CompletedTask; var late = t.Result.Where(r => r.WasSuccessful && r.ItemHandle > 0).Select(r => r.ItemHandle).ToArray(); return late.Length == 0 ? Task.CompletedTask - : subscriber.UnsubscribeBulkAsync(late, CancellationToken.None).ContinueWith( + : UnsubscribeAndPurgeAsync(subscriber, late, CancellationToken.None).ContinueWith( u => logger.LogWarning(u.Exception, "GalaxyDriver.ReadViaSubscribeOnceAsync late UnsubscribeBulk failed for {Count} handle(s).", late.Length), TaskContinuationOptions.OnlyOnFaulted); @@ -1532,11 +1581,28 @@ public sealed class GalaxyDriver ServerTimestampUtc: DateTime.UtcNow)); } } - if (!TryRegisterForGeneration(subscriptionId, bindings, generation)) + var registered = TryRegisterForGeneration(subscriptionId, bindings, generation, session); + if (registered != RegisterOutcome.Registered) { pump.OnDataChange -= handler; - await ReleaseOrphanedHandlesAsync(readSubscriber, bindings).ConfigureAwait(false); - throw TornDownDuringSubscribe(); + earlyEventWindow.Dispose(); // before any further round-trip + if (registered == RegisterOutcome.TornDown) + { + await ReleaseOrphanedHandlesAsync(readSubscriber, bindings).ConfigureAwait(false); + throw TornDownDuringSubscribe(); + } + + // #861: the reply straddled a reconnect. Its handles died with the old session and + // their numbers may already belong to new-session items, so they are neither + // registered nor unsubscribed. A read is one-shot: report the lost session per tag. + _logger.LogInformation( + "GalaxyDriver {InstanceId} subscribe-once read of {Count} tag(s) straddled a gateway reconnect — returning BadCommunicationError.", + _driverInstanceId, fullReferences.Count); + var lostAt = DateTime.UtcNow; + var lost = new DataValueSnapshot[fullReferences.Count]; + for (var i = 0; i < lost.Length; i++) + lost[i] = new DataValueSnapshot(null, StatusCodeMap.BadCommunicationError, null, lostAt); + return lost; } } finally @@ -1575,13 +1641,16 @@ public sealed class GalaxyDriver // Drop the bindings + unsubscribe the live handles. UnsubscribeBulkAsync's // failure isn't fatal — the registry is already cleared, so any straggling // event from the gw would be a no-op fan-out. - _subscriptions.Remove(subscriptionId); + var removed = _subscriptions.Remove(subscriptionId, out var liveInCurrentSession); + // A reconnect during the wait left these handles in the dead session; their numbers may now + // be new-session items, so they must not be unsubscribed (#861). + var staleSession = removed is not null && !liveInCurrentSession; var liveHandles = bindings.Where(b => b.ItemHandle > 0).Select(b => b.ItemHandle).ToArray(); - if (liveHandles.Length > 0) + if (liveHandles.Length > 0 && !staleSession) { try { - await readSubscriber.UnsubscribeBulkAsync(liveHandles, CancellationToken.None) + await UnsubscribeAndPurgeAsync(readSubscriber, liveHandles, CancellationToken.None) .ConfigureAwait(false); } catch (Exception ex) @@ -1653,7 +1722,14 @@ public sealed class GalaxyDriver { // Empty subscriptions register but never bind anything — keeps Unsubscribe // symmetric for callers that conditionally add tags later. - if (!TryRegisterForGeneration(subscriptionId, [], generation)) throw TornDownDuringSubscribe(); + RegisterOutcome outcome; + do + { + // No handles, so a session change only means "read the session again". + outcome = TryRegisterForGeneration(subscriptionId, [], generation, _subscriptions.CurrentSession); + } + while (outcome == RegisterOutcome.SessionChanged); + if (outcome == RegisterOutcome.TornDown) throw TornDownDuringSubscribe(); return new GalaxySubscriptionHandle(subscriptionId); } @@ -1670,16 +1746,63 @@ public sealed class GalaxyDriver var requested = (int)Math.Max(0, publishingInterval.TotalMilliseconds); var bufferedIntervalMs = requested > 0 ? requested : _options.MxAccess.PublishingIntervalMs; - // #859: MXAccess's initial value for a new handle can reach the event pump before this - // reply is registered (the gateway gives no reply-before-event ordering, and the reply and - // the event ride separate RPCs). Open the early-event window BEFORE the RPC so such events - // are parked rather than dropped as stale, and close it only after TryRegisterForGeneration - // (method scope), which promotes them for in-order replay. - using var earlyEventWindow = _subscriptions.BeginPendingSubscribe(); - var results = await subscriber - .SubscribeBulkAsync(dialledRefs, bufferedIntervalMs, cancellationToken) - .ConfigureAwait(false); + for (var attempt = 1; ; attempt++) + { + // #859: MXAccess's initial value for a new handle can reach the event pump before this + // reply is registered (the gateway gives no reply-before-event ordering, and the reply and + // the event ride separate RPCs). Open the early-event window BEFORE the RPC so such events + // are parked rather than dropped as stale; it closes once the registration (which promotes + // them for in-order replay) is done. + var earlyEventWindow = _subscriptions.BeginPendingSubscribe(); + try + { + var session = _subscriptions.CurrentSession; // #861 — the session this SubscribeBulk is issued on + var results = await subscriber + .SubscribeBulkAsync(dialledRefs, bufferedIntervalMs, cancellationToken) + .ConfigureAwait(false); + var bindings = BuildSubscribeBindings(dialledRefs, results); + switch (TryRegisterForGeneration(subscriptionId, bindings, generation, session)) + { + case RegisterOutcome.Registered: + _ = pump; // keep the pump alive for the subscription's lifetime + return new GalaxySubscriptionHandle(subscriptionId); + + case RegisterOutcome.TornDown: + // The teardown cleared the registry while SubscribeBulk was in flight; registering + // now would carry dead handles into the next runtime. Release what we got and fail. + earlyEventWindow.Dispose(); // M-3: not held across the release round-trip + await ReleaseOrphanedHandlesAsync(subscriber, bindings).ConfigureAwait(false); + throw TornDownDuringSubscribe(); + + default: + // #861: the reply straddled a gateway reconnect, so its handles died with the old + // session (and are NOT unsubscribed — the new session may reuse the numbers). The + // reconnect replay only re-subscribes what was registered when it snapshotted, so + // this subscription would be lost: issue it again on the new session. + if (attempt >= MaxSubscribeSessionAttempts) + { + throw new InvalidOperationException( + $"Galaxy subscribe straddled {attempt} consecutive gateway reconnects — giving up; retry later."); + } + _logger.LogInformation( + "GalaxyDriver {InstanceId} subscribe of {Count} tag(s) straddled a gateway reconnect — re-issuing on the new session (attempt {Attempt}).", + _driverInstanceId, dialledRefs.Length, attempt + 1); + continue; + } + } + finally + { + earlyEventWindow.Dispose(); + } + } + } + + /// Bound on re-issuing a subscribe whose reply straddled a gateway reconnect (#861). + private const int MaxSubscribeSessionAttempts = 3; + + private List BuildSubscribeBindings(string[] dialledRefs, IReadOnlyList results) + { // Build the binding list in input order. Failed entries (gw rejected the tag) are // recorded with a non-positive ItemHandle so the caller can detect partial failure // by inspecting the returned handle's diagnostic context — full per-tag error @@ -1703,16 +1826,7 @@ public sealed class GalaxyDriver fullRef, match?.ErrorMessage ?? ""); } } - - if (!TryRegisterForGeneration(subscriptionId, bindings, generation)) - { - // The teardown cleared the registry while SubscribeBulk was in flight; registering now - // would carry dead handles into the next runtime. Release what we got and fail the call. - await ReleaseOrphanedHandlesAsync(subscriber, bindings).ConfigureAwait(false); - throw TornDownDuringSubscribe(); - } - _ = pump; // keep the pump alive for the subscription's lifetime - return new GalaxySubscriptionHandle(subscriptionId); + return bindings; } /// @@ -1727,15 +1841,19 @@ public sealed class GalaxyDriver nameof(handle)); } - var bindings = _subscriptions.Remove(gsh.SubscriptionId); + var bindings = _subscriptions.Remove(gsh.SubscriptionId, out var liveInCurrentSession); if (bindings is null) return; // already removed or never registered + // Not yet rebound after a reconnect: its handles died with the old session and the new one may + // have reused the numbers for other items — unsubscribing them would unadvise those (#861). + if (!liveInCurrentSession) return; + var liveItemHandles = bindings.Where(b => b.ItemHandle > 0).Select(b => b.ItemHandle).ToArray(); if (liveItemHandles.Length == 0 || _subscriber is null) return; try { - await _subscriber.UnsubscribeBulkAsync(liveItemHandles, cancellationToken).ConfigureAwait(false); + await UnsubscribeAndPurgeAsync(_subscriber, liveItemHandles, cancellationToken).ConfigureAwait(false); } catch (Exception ex) { diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/Health/PerPlatformProbeWatcher.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/Health/PerPlatformProbeWatcher.cs index 2173a5a00..d98bae7e0 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/Health/PerPlatformProbeWatcher.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/Health/PerPlatformProbeWatcher.cs @@ -260,6 +260,12 @@ public sealed class PerPlatformProbeWatcher : IDisposable "PerPlatformProbeWatcher unsubscribe failed for {Count} probe(s); aggregator entries already cleared.", toRemove.Count); } + finally + { + // #859 I-3 interim: no trailing event for a removed probe handle may be promoted onto + // a reused handle number. + _registry?.PurgeParked([.. toRemove.Select(t => t.ItemHandle)]); + } } if (toAdd.Count == 0) return; diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/Runtime/EventPump.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/Runtime/EventPump.cs index a7d5388b8..e2be7600e 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/Runtime/EventPump.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/Runtime/EventPump.cs @@ -139,14 +139,14 @@ internal sealed class EventPump : IAsyncDisposable }); _clientTag = new KeyValuePair("galaxy.client", clientName ?? ""); - // One pump per gateway session: a new epoch discards whatever an earlier session left held - // (#859 review — item handles are reused across sessions), and this pump never parks or - // replays under any other epoch. - _epoch = _registry.StartNewEpoch(); + // One pump per gateway session. The owner calls SubscriptionRegistry.StartNewSession() BEFORE + // creating the pump for a new session (reconnect); this pump binds to that session and never + // parks or replays under any other (#859 review — item handles are reused across sessions). + _session = _registry.CurrentSession; _registry.ParkedEventsReady += WakeForReadyEvents; } - private readonly int _epoch; + private readonly int _session; /// /// Marker written into the channel to wake the dispatch loop after a registration promoted @@ -161,7 +161,7 @@ internal sealed class EventPump : IAsyncDisposable /// AllowSynchronousContinuations = false, so the write never runs the dispatch loop inline. /// A failed write is harmless: the channel is either full (the loop is busy and replays the /// ready queue before its next dispatch) or completed (the pump is dead; its held events - /// belong to a session the next pump's new epoch discards). + /// belong to a session the next StartNewSession discards). /// private void WakeForReadyEvents() => _channel.Writer.TryWrite(WakeSentinel); @@ -363,14 +363,14 @@ internal sealed class EventPump : IAsyncDisposable private void DispatchDataChange(MxEvent ev) { - var outcome = _registry.ResolveOrPark(ev, _epoch, out var subscribers); + var outcome = _registry.ResolveOrPark(ev, _session, out var subscribers, out var replayFirst); - // Replay promoted early events (#859) AFTER the resolve and BEFORE fanning this event out. - // A registration writes the reverse map and promotes its parked events in ONE critical - // section under the registry's park gate, and ResolveOrPark resolves under that gate - // whenever anything is held. So if this event resolved, every older parked event for its - // handle is already in the ready queue, and replaying it first preserves per-handle order. - DispatchReadyEvents(); + // Replay promoted early events (#859) BEFORE fanning this event out. A registration writes + // the reverse map and promotes its parked events in ONE critical section under the + // registry's park gate, and ResolveOrPark resolves — and takes the ready queue — under that + // same gate whenever anything is held. So if this event resolved, every older parked event + // for its handle is in replayFirst, and replaying it first preserves per-handle order. + ReplayEarly(replayFirst); switch (outcome) { @@ -396,9 +396,10 @@ internal sealed class EventPump : IAsyncDisposable /// thread, so it is serialised with live dispatch. A promoted event whose subscription was /// removed again before the replay resolves to nobody and is dropped like any stale event. /// - private void DispatchReadyEvents() + private void DispatchReadyEvents() => ReplayEarly(_registry.TakeReadyEvents(_session)); + + private void ReplayEarly(IReadOnlyList ready) { - var ready = _registry.TakeReadyEvents(_epoch); if (ready.Count == 0) return; foreach (var early in ready) { diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/Runtime/SubscriptionRegistry.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/Runtime/SubscriptionRegistry.cs index ad65f06d3..303dde0c5 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/Runtime/SubscriptionRegistry.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/Runtime/SubscriptionRegistry.cs @@ -68,17 +68,70 @@ internal sealed class SubscriptionRegistry /// /// The subscription identifier. /// The tag bindings for the subscription. - public void Register(long subscriptionId, IReadOnlyList bindings) + public void Register(long subscriptionId, IReadOnlyList bindings) => + Bind(subscriptionId, bindings, requireExisting: false, expectedSession: null); + + /// + /// , but only if the gateway session is still + /// — the session the SubscribeBulk that produced + /// was issued on (#861). Item handles are per session and the next + /// session reuses the numbers, so registering a reply that straddled a reconnect would bind + /// dead handles that collide with live ones. + /// + /// The subscription identifier. + /// The tag bindings for the subscription. + /// The captured before the SubscribeBulk. + /// False (nothing registered) when the session changed in the meantime. + public bool TryRegister(long subscriptionId, IReadOnlyList bindings, int expectedSession) => + Bind(subscriptionId, bindings, requireExisting: false, expectedSession) == BindResult.Bound; + + /// + /// Replace an existing subscription's bindings with the item handles a post-reconnect + /// SubscribeBulk returned, rebuilding the reverse fan-out map so events on the new + /// handles dispatch and the now-dead pre-reconnect handles are dropped. No-op when the + /// subscription id is unknown (it was unsubscribed during the reconnect window). + /// + /// The subscription identifier. + /// The new tag bindings after reconnection. + public void Rebind(long subscriptionId, IReadOnlyList newBindings) => + Bind(subscriptionId, newBindings, requireExisting: true, expectedSession: null); + + /// , but only while the session is still (#861). + /// The subscription identifier. + /// The new tag bindings. + /// The session the SubscribeBulk was issued on. + /// False when the session changed (nothing rebound); true otherwise, including the unknown-id no-op. + public bool TryRebind(long subscriptionId, IReadOnlyList newBindings, int expectedSession) => + Bind(subscriptionId, newBindings, requireExisting: true, expectedSession) != BindResult.SessionChanged; + + private enum BindResult { Bound, UnknownId, SessionChanged } + + private BindResult Bind( + long subscriptionId, IReadOnlyList bindings, bool requireExisting, int? expectedSession) { bool promoted; // The reverse-map writes and the early-event promotion are ONE critical section under the // park gate (#859 review): a dispatch that resolves a handle through the map must never // run between the write and the promotion, or a later live event would overtake the - // parked initial value. ResolveOrPark takes the same gate whenever anything is held. + // parked initial value. ResolveOrPark takes the same gate whenever anything is held. The + // session check is in here too, so it is atomic with StartNewSession's invalidation. lock (_parkGate) { - var entry = new SubscriptionEntry(subscriptionId, bindings); - _bySubscriptionId[subscriptionId] = entry; + if (expectedSession is { } session && session != _session) return BindResult.SessionChanged; + + if (_bySubscriptionId.TryGetValue(subscriptionId, out var oldEntry)) + { + // Drop this subscription from every reverse-map set it currently appears in. Only + // this id is removed, so another subscription that now owns the same handle number + // (a reused handle in a new session) keeps its entry. + RemoveFromReverseMap(oldEntry); + } + else if (requireExisting) + { + return BindResult.UnknownId; + } + + _bySubscriptionId[subscriptionId] = new SubscriptionEntry(subscriptionId, bindings, _session); foreach (var binding in bindings) { if (binding.ItemHandle <= 0) continue; // failed gw subscribe — no events expected @@ -91,6 +144,22 @@ internal sealed class SubscriptionRegistry promoted = PromoteParkedEventsLocked(bindings); } if (promoted) ParkedEventsReady?.Invoke(); + return BindResult.Bound; + } + + private void RemoveFromReverseMap(SubscriptionEntry entry) + { + foreach (var binding in entry.Bindings) + { + if (binding.ItemHandle <= 0) continue; + if (!_subscribersByItemHandle.TryGetValue(binding.ItemHandle, out var set)) continue; + var remaining = set.Remove(entry.SubscriptionId); + if (remaining.IsEmpty) + { + _subscribersByItemHandle.TryRemove(binding.ItemHandle, out _); + } + else _subscribersByItemHandle[binding.ItemHandle] = remaining; + } } /// @@ -99,23 +168,27 @@ internal sealed class SubscriptionRegistry /// /// The subscription identifier. /// The bindings for the subscription, or null if not found. - public IReadOnlyList? Remove(long subscriptionId) + public IReadOnlyList? Remove(long subscriptionId) => Remove(subscriptionId, out _); + + /// + /// Remove a subscription, also reporting whether its handles belong to the CURRENT gateway + /// session. A subscription not yet rebound after a reconnect still carries the previous + /// session's handles, whose numbers the new session reuses — the caller must not send those + /// to UnsubscribeBulk, or it would unadvise some other live item. + /// + /// The subscription identifier. + /// True when the bindings' handles were issued by the current session. + /// The bindings for the subscription, or null if not found. + public IReadOnlyList? Remove(long subscriptionId, out bool liveInCurrentSession) { - if (!_bySubscriptionId.TryRemove(subscriptionId, out var entry)) return null; - - foreach (var binding in entry.Bindings) + lock (_parkGate) { - if (binding.ItemHandle <= 0) continue; - if (!_subscribersByItemHandle.TryGetValue(binding.ItemHandle, out var set)) continue; - var remaining = set.Remove(subscriptionId); - if (remaining.IsEmpty) - { - _subscribersByItemHandle.TryRemove(binding.ItemHandle, out _); - } - else _subscribersByItemHandle[binding.ItemHandle] = remaining; + liveInCurrentSession = false; + if (!_bySubscriptionId.TryRemove(subscriptionId, out var entry)) return null; + RemoveFromReverseMap(entry); + liveInCurrentSession = entry.Session == _session; + return entry.Bindings; } - - return entry.Bindings; } /// @@ -140,7 +213,6 @@ internal sealed class SubscriptionRegistry return result; } - /// /// Drop every subscription and the whole reverse map. Used by the driver's runtime teardown: /// once the gateway session is closed every item handle in here is dead, and the host @@ -148,13 +220,11 @@ internal sealed class SubscriptionRegistry /// public void Clear() { - _bySubscriptionId.Clear(); - _subscribersByItemHandle.Clear(); lock (_parkGate) { - _parkedByItemHandle.Clear(); - _ready.Clear(); - Volatile.Write(ref _heldCount, 0); + _bySubscriptionId.Clear(); + _subscribersByItemHandle.Clear(); + DiscardAllHeldLocked(); } } @@ -177,15 +247,21 @@ internal sealed class SubscriptionRegistry // While at least one subscribe is in flight such events are parked here (bounded) instead of // dropped; a registration that binds a parked handle promotes its events to a ready queue that // the EventPump's dispatch loop replays — on that same single thread, BEFORE any later event for - // the handle — so per-handle value order is preserved. When the last in-flight subscribe ends, - // whatever is still parked belonged to no subscription and is discarded, so the genuine - // stale-after-unsubscribe drop is unchanged outside the window. + // the handle — so per-handle value order is preserved. // - // Item handles are only meaningful within one gateway session, and a new session reuses small - // integers. Every held event therefore belongs to an EPOCH: each EventPump (one per session — - // reconnect replay recreates it) starts a new epoch, which discards everything held, and an - // event parked or replayed by a pump of an older epoch is dropped. Otherwise an old-session - // value could be promoted onto whichever tag now owns the reused handle number. + // Unclaimed parked events are trimmed by WINDOW SEQUENCE, not only when every window has + // closed (#859 second review — concurrent reads keep some window open almost permanently): + // each window gets a monotonically increasing sequence when it opens, and each parked event is + // stamped with the newest sequence at park time. An event belonging to window W is generated + // after W opened, so its stamp is >= W's sequence; once every window with a sequence <= the + // stamp has closed, nothing can claim it and it is discarded. + // + // Item handles are only meaningful within one gateway SESSION, and a new session reuses small + // integers. StartNewSession (called on reconnect, before the replay re-subscribes) discards + // every held event, invalidates the reverse map (so a new-session event can never resolve to + // the tag that owned that number in the old session — it parks until the replay's Rebind), + // and fences: an event parked or replayed by an older session's pump is dropped, and a + // subscribe whose reply straddled the reconnect cannot register its dead handles (#861). /// /// Upper bound on events held (parked + promoted-but-not-yet-replayed) across all in-flight @@ -196,16 +272,18 @@ internal sealed class SubscriptionRegistry public const int MaxParkedEvents = 100_000; private readonly Lock _parkGate = new(); - private readonly Dictionary> _parkedByItemHandle = new(); + private readonly Dictionary> _parkedByItemHandle = new(); + private readonly Queue _parkOrder = new(); // every parked event, in park order (trim index) private readonly Queue _ready = new(); + private readonly SortedSet _openWindows = new(); + private long _windowSequence; // Parked + ready. Written only under _parkGate; read lock-free (Volatile) by the dispatch fast path. private int _heldCount; - // Open windows. Written only under _parkGate (so the 0→N / N→0 transitions and the discard of - // the previous window's leftovers are atomic); read lock-free by the dispatch fast path. + // Open windows. Written only under _parkGate; read lock-free by the dispatch fast path. private int _pendingSubscribes; - private int _epoch; + private int _session; /// /// Test seam (#859 review): invoked inside the registration critical section, between the @@ -213,6 +291,27 @@ internal sealed class SubscriptionRegistry /// internal Action? AfterMapWriteForTests { get; set; } + /// + /// Test seam (M-1): parks unconditionally, producing the parked list for a + /// resolved handle that the atomic registration otherwise makes impossible. + /// + /// The event to park. + internal void ForceParkForTests(MxEvent ev) + { + lock (_parkGate) + { + var entry = new ParkedEvent(_windowSequence, ev.ItemHandle, ev); + if (!_parkedByItemHandle.TryGetValue(ev.ItemHandle, out var queue)) + { + queue = new Queue(); + _parkedByItemHandle[ev.ItemHandle] = queue; + } + queue.Enqueue(entry); + _parkOrder.Enqueue(entry); + Volatile.Write(ref _heldCount, _heldCount + 1); + } + } + /// /// Raised when a registration promoted parked events to the ready queue, so the /// can wake its dispatch loop even if no further event arrives. @@ -230,78 +329,141 @@ internal sealed class SubscriptionRegistry /// Gets the number of events currently held (parked or awaiting replay). public int ParkedEventCount => Volatile.Read(ref _heldCount); + /// Gets the current gateway-session number (bumped by ). + public int CurrentSession => Volatile.Read(ref _session); + /// - /// Starts a new session epoch: discards every held event and makes events from any earlier - /// epoch unparkable and unreplayable. Called by each new . + /// A new gateway session replaced the old one (reconnect). Discards every held event, + /// invalidates the reverse map — subscriptions are KEPT, but their old-session handles no + /// longer resolve, so new-session events park until the replay rebinds each subscription — + /// and bumps the session number that fences old pumps and straddling subscribes. NOT for the + /// same-session replay-gap re-snapshot, whose handles stay valid until rebound. /// - /// The new epoch. - public int StartNewEpoch() + /// The new session number. + public int StartNewSession() { lock (_parkGate) { - _parkedByItemHandle.Clear(); - _ready.Clear(); - Volatile.Write(ref _heldCount, 0); - return ++_epoch; + _subscribersByItemHandle.Clear(); + DiscardAllHeldLocked(); + // Volatile: the dispatch fast path reads it lock-free so a dying pump of the old session + // can never resolve an old-session event through the map once the new session exists. + Volatile.Write(ref _session, _session + 1); + return _session; } } /// /// Opens the early-event window for one subscribe. Call BEFORE issuing SubscribeBulk /// and dispose AFTER the returned handles are registered (or the attempt was abandoned) — - /// not later: while any window is open, events for unknown handles are parked, not dropped. + /// not later: while a window is open, events for unknown handles are parked, not dropped. /// /// A token that closes the window when disposed (idempotent). public IDisposable BeginPendingSubscribe() { + long sequence; lock (_parkGate) { - // 0→1: anything still parked belongs to an earlier, closed window. EndPendingSubscribe - // already discards on N→0 under this same gate, so this is belt-and-braces. - if (_pendingSubscribes == 0) DiscardParkedLocked(); - Volatile.Write(ref _pendingSubscribes, _pendingSubscribes + 1); + sequence = ++_windowSequence; + _openWindows.Add(sequence); + Volatile.Write(ref _pendingSubscribes, _openWindows.Count); } - return new PendingSubscribeScope(this); + return new PendingSubscribeScope(this, sequence); } - private void EndPendingSubscribe() + private void EndPendingSubscribe(long sequence) { lock (_parkGate) { - Volatile.Write(ref _pendingSubscribes, _pendingSubscribes - 1); - // N→0: what is still parked was claimed by no registration. Promoted (ready) events are - // kept — they belong to a registered handle and are awaiting replay. - if (_pendingSubscribes == 0) DiscardParkedLocked(); + _openWindows.Remove(sequence); + Volatile.Write(ref _pendingSubscribes, _openWindows.Count); + TrimUnclaimableLocked(); } } - private void DiscardParkedLocked() + /// + /// Discard parked events no open window can claim: those stamped before the oldest open + /// window's sequence (all of them once no window is open). Promoted (ready) events are kept — + /// they belong to a registered handle and await replay. Amortised O(1) per parked event. + /// + private void TrimUnclaimableLocked() { - if (_parkedByItemHandle.Count == 0) return; - var parked = 0; - foreach (var list in _parkedByItemHandle.Values) parked += list.Count; + var oldestOpen = _openWindows.Count == 0 ? long.MaxValue : _openWindows.Min; + while (_parkOrder.TryPeek(out var head) && head.Stamp < oldestOpen) + { + _parkOrder.Dequeue(); + if (head.Gone) continue; // already promoted or purged + // Per-handle queues are in park order too, so a live head of the global order is the + // head of its handle's queue. + var queue = _parkedByItemHandle[head.ItemHandle]; + queue.Dequeue(); + head.Gone = true; + if (queue.Count == 0) _parkedByItemHandle.Remove(head.ItemHandle); + Volatile.Write(ref _heldCount, _heldCount - 1); + } + } + + private void DiscardAllHeldLocked() + { + foreach (var parked in _parkOrder) parked.Gone = true; + _parkOrder.Clear(); _parkedByItemHandle.Clear(); - Volatile.Write(ref _heldCount, _heldCount - parked); + _ready.Clear(); + Volatile.Write(ref _heldCount, 0); + } + + /// + /// Drop parked events for handles that were just unsubscribed (#859 second review, I-3 + /// interim). A trailing event for a removed handle can be parked during another subscribe's + /// window; if MXAccess reuses that handle number for the other subscribe's item, promotion + /// would hand the old item's value to the new tag. Call when UnsubscribeBulk for + /// completes. It cannot catch an event still in flight + /// after that, nor tell a genuine early value for a reused number from a trailing one (it + /// drops both — a timeout is safer than another tag's value); a complete fix needs a + /// worker sequence on the SubscribeBulk reply (mxaccessgw, cross-repo). + /// + /// The handles whose unsubscribe completed. + public void PurgeParked(IReadOnlyList itemHandles) + { + if (Volatile.Read(ref _heldCount) == 0 || itemHandles.Count == 0) return; + lock (_parkGate) + { + foreach (var handle in itemHandles) + { + if (!_parkedByItemHandle.Remove(handle, out var queue)) continue; + foreach (var parked in queue) parked.Gone = true; + Volatile.Write(ref _heldCount, _heldCount - queue.Count); + } + } } /// /// Resolve the subscribers for 's item handle, or — when the handle is - /// unknown while a subscribe is in flight — park the event for replay. Atomic with the - /// promotion a registration performs, so an event is never parked after its handle's - /// promotion already ran (it would otherwise be stranded until the window closed). + /// unknown while a subscribe is in flight — park the event for replay. Also hands back the + /// promoted events that must be dispatched BEFORE this one (one gate acquisition per event + /// while anything is held). /// + /// + /// ⚠️ The lock-free fast path relies on a single-dispatcher invariant: only the ONE live + /// 's dispatch thread ever parks (increments _heldCount), and it + /// is the thread calling this. So when it reads _heldCount == 0, no older event for + /// this handle can be waiting anywhere — it would have to have been parked by this same + /// thread earlier — and resolving through the map cannot overtake one. Registrations only + /// move events from parked to ready (count unchanged) and the dispatch thread only removes + /// them. A second concurrently-dispatching pump on one registry would break this. + /// /// The data-change event. - /// The calling pump's epoch (). + /// The calling pump's session ( at its creation). /// The subscribers when the outcome is . + /// Promoted early events to dispatch before this one, in order. /// What happened to the event. public EarlyEventOutcome ResolveOrPark( - MxEvent ev, int epoch, out IReadOnlyList<(long SubscriptionId, string FullReference)> subscribers) + MxEvent ev, int session, + out IReadOnlyList<(long SubscriptionId, string FullReference)> subscribers, + out IReadOnlyList replayFirst) { - // Lock-free fast path ONLY while nothing is held: then no older event for this handle can be - // waiting, so resolving through the map cannot overtake one. While anything is held, the - // resolve happens under the gate, i.e. never inside a registration's map-write→promotion - // critical section. - if (Volatile.Read(ref _heldCount) == 0) + replayFirst = []; + if (Volatile.Read(ref _heldCount) == 0 && Volatile.Read(ref _session) == session) { subscribers = ResolveSubscribers(ev.ItemHandle); if (subscribers.Count > 0) return EarlyEventOutcome.Resolved; @@ -310,7 +472,7 @@ internal sealed class SubscriptionRegistry lock (_parkGate) { - if (epoch != _epoch) + if (session != _session) { subscribers = []; return EarlyEventOutcome.Stale; // a dying pump of an earlier session @@ -319,28 +481,34 @@ internal sealed class SubscriptionRegistry subscribers = ResolveSubscribers(ev.ItemHandle); if (subscribers.Count > 0) { - // Defensive: registration promotes atomically with its map writes, so a resolved - // handle should hold no parked list — but if one ever does, queue behind it rather - // than overtake it (promotion moves the whole list in order). Not bounded: dropping - // this newer value would leave the older parked one as the node's last word. - if (_parkedByItemHandle.TryGetValue(ev.ItemHandle, out var pending)) + // Defensive (M-1): registration promotes atomically with its map writes, so a + // resolved handle should hold no parked list. If one ever does, promote it now + // (fail safe) — parking this LIVE event behind it would lose it to a later trim if + // no registration ever claimed the list. + if (_parkedByItemHandle.Remove(ev.ItemHandle, out var stranded)) { - pending.Add(ev); - Volatile.Write(ref _heldCount, _heldCount + 1); - subscribers = []; - return EarlyEventOutcome.Parked; + foreach (var parked in stranded) + { + parked.Gone = true; + _ready.Enqueue(parked.Event); + } } + replayFirst = TakeReadyLocked(); return EarlyEventOutcome.Resolved; } + + replayFirst = TakeReadyLocked(); if (_pendingSubscribes == 0) return EarlyEventOutcome.Stale; if (_heldCount >= MaxParkedEvents) return EarlyEventOutcome.Overflow; - if (!_parkedByItemHandle.TryGetValue(ev.ItemHandle, out var list)) + var entry = new ParkedEvent(_windowSequence, ev.ItemHandle, ev); + if (!_parkedByItemHandle.TryGetValue(ev.ItemHandle, out var queue)) { - list = []; - _parkedByItemHandle[ev.ItemHandle] = list; + queue = new Queue(); + _parkedByItemHandle[ev.ItemHandle] = queue; } - list.Add(ev); + queue.Enqueue(entry); + _parkOrder.Enqueue(entry); Volatile.Write(ref _heldCount, _heldCount + 1); return EarlyEventOutcome.Parked; } @@ -348,23 +516,28 @@ internal sealed class SubscriptionRegistry /// /// Dequeue every event a registration promoted, in arrival order. Called only from the - /// EventPump's single dispatch thread. A pump of an earlier epoch gets nothing. + /// EventPump's single dispatch thread (the wake path). A pump of an earlier session gets nothing. /// - /// The calling pump's epoch. + /// The calling pump's session. /// The promoted events; empty when none are waiting. - public IReadOnlyList TakeReadyEvents(int epoch) + public IReadOnlyList TakeReadyEvents(int session) { if (Volatile.Read(ref _heldCount) == 0) return []; lock (_parkGate) { - if (epoch != _epoch || _ready.Count == 0) return []; - var taken = _ready.ToArray(); - _ready.Clear(); - Volatile.Write(ref _heldCount, _heldCount - taken.Length); - return taken; + return session != _session ? [] : TakeReadyLocked(); } } + private IReadOnlyList TakeReadyLocked() + { + if (_ready.Count == 0) return []; + var taken = _ready.ToArray(); + _ready.Clear(); + Volatile.Write(ref _heldCount, _heldCount - taken.Length); + return taken; + } + private bool PromoteParkedEventsLocked(IReadOnlyList bindings) { if (_parkedByItemHandle.Count == 0) return false; @@ -372,21 +545,37 @@ internal sealed class SubscriptionRegistry foreach (var binding in bindings) { if (binding.ItemHandle <= 0) continue; - if (!_parkedByItemHandle.Remove(binding.ItemHandle, out var list)) continue; - foreach (var ev in list) _ready.Enqueue(ev); // stays counted in _heldCount until replayed + if (!_parkedByItemHandle.Remove(binding.ItemHandle, out var queue)) continue; + foreach (var parked in queue) + { + parked.Gone = true; // stays counted in _heldCount until replayed + _ready.Enqueue(parked.Event); + } promoted = true; } return promoted; } - private sealed class PendingSubscribeScope(SubscriptionRegistry owner) : IDisposable + private sealed class ParkedEvent(long stamp, int itemHandle, MxEvent ev) + { + /// Gets the newest window sequence when the event was parked. + public long Stamp { get; } = stamp; + /// Gets the event's item handle. + public int ItemHandle { get; } = itemHandle; + /// Gets the event. + public MxEvent Event { get; } = ev; + /// Gets or sets whether the event has left the parked set (promoted, purged or trimmed). + public bool Gone { get; set; } + } + + private sealed class PendingSubscribeScope(SubscriptionRegistry owner, long sequence) : IDisposable { private int _disposed; /// Closes this subscribe's early-event window (once). public void Dispose() { - if (Interlocked.Exchange(ref _disposed, 1) == 0) owner.EndPendingSubscribe(); + if (Interlocked.Exchange(ref _disposed, 1) == 0) owner.EndPendingSubscribe(sequence); } } @@ -404,50 +593,6 @@ internal sealed class SubscriptionRegistry public IReadOnlyList<(long SubscriptionId, IReadOnlyList Bindings)> SnapshotEntries() => [.. _bySubscriptionId.Values.Select(entry => (entry.SubscriptionId, entry.Bindings))]; - /// - /// Replace an existing subscription's bindings with the item handles a post-reconnect - /// SubscribeBulk returned, rebuilding the reverse fan-out map so events on the new - /// handles dispatch and the now-dead pre-reconnect handles are dropped. No-op when the - /// subscription id is unknown (it was unsubscribed during the reconnect window). - /// - /// The subscription identifier. - /// The new tag bindings after reconnection. - public void Rebind(long subscriptionId, IReadOnlyList newBindings) - { - bool promoted; - lock (_parkGate) // map writes + promotion atomic — see Register - { - if (!_bySubscriptionId.TryGetValue(subscriptionId, out var oldEntry)) return; - - // Drop this subscription from every reverse-map set it currently appears in. The - // pre-reconnect item handles are stale once the gw re-issues fresh ones. - foreach (var binding in oldEntry.Bindings) - { - if (binding.ItemHandle <= 0) continue; - if (!_subscribersByItemHandle.TryGetValue(binding.ItemHandle, out var set)) continue; - var remaining = set.Remove(subscriptionId); - if (remaining.IsEmpty) - { - _subscribersByItemHandle.TryRemove(binding.ItemHandle, out _); - } - else _subscribersByItemHandle[binding.ItemHandle] = remaining; - } - - _bySubscriptionId[subscriptionId] = new SubscriptionEntry(subscriptionId, newBindings); - foreach (var binding in newBindings) - { - if (binding.ItemHandle <= 0) continue; // failed gw subscribe — no events expected - _subscribersByItemHandle.AddOrUpdate( - binding.ItemHandle, - _ => [subscriptionId], - (_, set) => set.Add(subscriptionId)); - } - AfterMapWriteForTests?.Invoke(); - promoted = PromoteParkedEventsLocked(newBindings); - } - if (promoted) ParkedEventsReady?.Invoke(); - } - /// /// Per-subscription bookkeeping. is an index /// over keyed by item handle so ResolveSubscribers @@ -463,14 +608,18 @@ internal sealed class SubscriptionRegistry public IReadOnlyList Bindings { get; } /// Gets the index of full references by item handle. public IReadOnlyDictionary FullRefByItemHandle { get; } + /// Gets the gateway session whose handles carries. + public int Session { get; } /// Initializes a new subscription entry. /// The subscription identifier. /// The tag bindings for the subscription. - public SubscriptionEntry(long subscriptionId, IReadOnlyList bindings) + /// The session the bindings' handles were issued by. + public SubscriptionEntry(long subscriptionId, IReadOnlyList bindings, int session) { SubscriptionId = subscriptionId; Bindings = bindings; + Session = session; var index = new Dictionary(bindings.Count); foreach (var binding in bindings) { diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Tests/Runtime/EarlyEventWindowTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Tests/Runtime/EarlyEventWindowTests.cs index e970b6661..b68651989 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Tests/Runtime/EarlyEventWindowTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Tests/Runtime/EarlyEventWindowTests.cs @@ -45,9 +45,9 @@ public sealed class EarlyEventWindowTests public void Unknown_handle_without_a_pending_subscribe_is_stale() { var registry = new SubscriptionRegistry(); - var epoch = registry.StartNewEpoch(); + var epoch = registry.CurrentSession; - registry.ResolveOrPark(DataChange(7, 1), epoch, out _).ShouldBe(EarlyEventOutcome.Stale); + registry.ResolveOrPark(DataChange(7, 1), epoch, out _, out _).ShouldBe(EarlyEventOutcome.Stale); registry.ParkedEventCount.ShouldBe(0); } @@ -56,15 +56,15 @@ public sealed class EarlyEventWindowTests public void Parked_events_are_promoted_in_order_on_register() { var registry = new SubscriptionRegistry(); - var epoch = registry.StartNewEpoch(); + var epoch = registry.CurrentSession; var woken = 0; registry.ParkedEventsReady += () => woken++; using (registry.BeginPendingSubscribe()) { - registry.ResolveOrPark(DataChange(7, 1), epoch, out _).ShouldBe(EarlyEventOutcome.Parked); - registry.ResolveOrPark(DataChange(7, 2), epoch, out _).ShouldBe(EarlyEventOutcome.Parked); - registry.ResolveOrPark(DataChange(8, 9), epoch, out _).ShouldBe(EarlyEventOutcome.Parked); + registry.ResolveOrPark(DataChange(7, 1), epoch, out _, out _).ShouldBe(EarlyEventOutcome.Parked); + registry.ResolveOrPark(DataChange(7, 2), epoch, out _, out _).ShouldBe(EarlyEventOutcome.Parked); + registry.ResolveOrPark(DataChange(8, 9), epoch, out _, out _).ShouldBe(EarlyEventOutcome.Parked); registry.TakeReadyEvents(epoch).ShouldBeEmpty(); registry.Register(1, [new TagBinding("Tank.Level", 7)]); @@ -82,12 +82,12 @@ public sealed class EarlyEventWindowTests public void Rebind_promotes_parked_events_for_the_new_handles() { var registry = new SubscriptionRegistry(); - var epoch = registry.StartNewEpoch(); + var epoch = registry.CurrentSession; registry.Register(1, [new TagBinding("Tank.Level", 7)]); using (registry.BeginPendingSubscribe()) { - registry.ResolveOrPark(DataChange(70, 5), epoch, out _).ShouldBe(EarlyEventOutcome.Parked); + registry.ResolveOrPark(DataChange(70, 5), epoch, out _, out _).ShouldBe(EarlyEventOutcome.Parked); registry.Rebind(1, [new TagBinding("Tank.Level", 70)]); } @@ -99,11 +99,11 @@ public sealed class EarlyEventWindowTests public void Closing_one_of_two_windows_keeps_parked_events() { var registry = new SubscriptionRegistry(); - var epoch = registry.StartNewEpoch(); + var epoch = registry.CurrentSession; var first = registry.BeginPendingSubscribe(); var second = registry.BeginPendingSubscribe(); - registry.ResolveOrPark(DataChange(7, 1), epoch, out _).ShouldBe(EarlyEventOutcome.Parked); + registry.ResolveOrPark(DataChange(7, 1), epoch, out _, out _).ShouldBe(EarlyEventOutcome.Parked); first.Dispose(); first.Dispose(); // idempotent — must not close the second window registry.PendingSubscribeCount.ShouldBe(1); @@ -111,7 +111,7 @@ public sealed class EarlyEventWindowTests second.Dispose(); registry.ParkedEventCount.ShouldBe(0); - registry.ResolveOrPark(DataChange(7, 1), epoch, out _).ShouldBe(EarlyEventOutcome.Stale); + registry.ResolveOrPark(DataChange(7, 1), epoch, out _, out _).ShouldBe(EarlyEventOutcome.Stale); } /// @@ -122,9 +122,9 @@ public sealed class EarlyEventWindowTests public void A_new_window_never_inherits_the_previous_windows_leftovers() { var registry = new SubscriptionRegistry(); - var epoch = registry.StartNewEpoch(); + var epoch = registry.CurrentSession; var first = registry.BeginPendingSubscribe(); - registry.ResolveOrPark(DataChange(7, 1), epoch, out _).ShouldBe(EarlyEventOutcome.Parked); + registry.ResolveOrPark(DataChange(7, 1), epoch, out _, out _).ShouldBe(EarlyEventOutcome.Parked); first.Dispose(); using var second = registry.BeginPendingSubscribe(); @@ -138,35 +138,104 @@ public sealed class EarlyEventWindowTests public void Parking_is_bounded() { var registry = new SubscriptionRegistry(); - var epoch = registry.StartNewEpoch(); + var epoch = registry.CurrentSession; using var window = registry.BeginPendingSubscribe(); var ev = DataChange(7, 1); for (var i = 0; i < SubscriptionRegistry.MaxParkedEvents; i++) { - registry.ResolveOrPark(ev, epoch, out _).ShouldBe(EarlyEventOutcome.Parked); + registry.ResolveOrPark(ev, epoch, out _, out _).ShouldBe(EarlyEventOutcome.Parked); } - registry.ResolveOrPark(ev, epoch, out _).ShouldBe(EarlyEventOutcome.Overflow); + registry.ResolveOrPark(ev, epoch, out _, out _).ShouldBe(EarlyEventOutcome.Overflow); registry.ParkedEventCount.ShouldBe(SubscriptionRegistry.MaxParkedEvents); } - /// Review #5: promoted-but-not-yet-replayed events still count toward the bound. + /// + /// Review #5 + M-2: promoted-but-not-yet-replayed events stay counted until the dispatch thread + /// takes them, and the next ResolveOrPark hands them back (in order) before anything else. + /// [Fact] - public void Ready_events_count_toward_the_bound() + public void Ready_events_stay_counted_until_handed_back_by_the_next_resolve() { var registry = new SubscriptionRegistry(); - var epoch = registry.StartNewEpoch(); + var session = registry.CurrentSession; using var window = registry.BeginPendingSubscribe(); - var ev = DataChange(7, 1); - for (var i = 0; i < SubscriptionRegistry.MaxParkedEvents; i++) registry.ResolveOrPark(ev, epoch, out _); - registry.Register(1, [new TagBinding("Tank.Level", 7)]); // all promoted to ready + registry.ResolveOrPark(DataChange(7, 1), session, out _, out _); + registry.ResolveOrPark(DataChange(7, 2), session, out _, out _); + registry.Register(1, [new TagBinding("Tank.Level", 7)]); // promoted to ready + registry.ParkedEventCount.ShouldBe(2); - registry.ResolveOrPark(DataChange(8, 1), epoch, out _).ShouldBe(EarlyEventOutcome.Overflow); + registry.ResolveOrPark(DataChange(8, 3), session, out _, out var replayFirst).ShouldBe(EarlyEventOutcome.Parked); + replayFirst.Select(e => e.Value.DoubleValue).ShouldBe([1.0, 2.0]); + registry.ParkedEventCount.ShouldBe(1, "only the newly parked event for handle 8 remains held"); + } - registry.TakeReadyEvents(epoch).Count.ShouldBe(SubscriptionRegistry.MaxParkedEvents); + /// + /// I-2: windows that overlap so the open count never reaches 0 (concurrent reads) must not + /// accumulate unclaimed events — each is trimmed once every window that could claim it closed. + /// + [Fact] + public void Overlapping_windows_that_never_all_close_do_not_accumulate_unclaimed_events() + { + var registry = new SubscriptionRegistry(); + var session = registry.CurrentSession; + var open = registry.BeginPendingSubscribe(); + for (var i = 0; i < 1_000; i++) + { + // An unknown-handle event nobody will ever claim (e.g. a trailing event after a read's Remove). + registry.ResolveOrPark(DataChange(10_000 + i, i), session, out _, out _).ShouldBe(EarlyEventOutcome.Parked); + var next = registry.BeginPendingSubscribe(); // overlap: the count never drops to 0 + open.Dispose(); + open = next; + registry.ParkedEventCount.ShouldBe(0, $"iteration {i}: the event's only possible owners have all closed"); + } + + // Positive control: an event parked inside the still-open window survives and is claimable. + registry.ResolveOrPark(DataChange(7, 42), session, out _, out _).ShouldBe(EarlyEventOutcome.Parked); + var later = registry.BeginPendingSubscribe(); + later.Dispose(); // a window opened AFTER the park closing must not trim it + registry.ParkedEventCount.ShouldBe(1); + registry.Register(1, [new TagBinding("Tank.Level", 7)]); + registry.TakeReadyEvents(session).Single().Value.DoubleValue.ShouldBe(42.0); + open.Dispose(); + } + + /// I-3 interim: an unsubscribe purges events parked for its handles. + [Fact] + public void Purge_drops_events_parked_for_unsubscribed_handles() + { + var registry = new SubscriptionRegistry(); + var session = registry.CurrentSession; + using var window = registry.BeginPendingSubscribe(); + registry.ResolveOrPark(DataChange(7, 1), session, out _, out _); // trailing event of a removed item + registry.ResolveOrPark(DataChange(8, 2), session, out _, out _); // positive control + + registry.PurgeParked([7]); + + registry.ParkedEventCount.ShouldBe(1); + registry.Register(1, [new TagBinding("Reused", 7), new TagBinding("Other", 8)]); + registry.TakeReadyEvents(session).Select(e => e.ItemHandle).ShouldBe([8]); + } + + /// + /// M-1: a live event for a RESOLVED handle that (defensively) still has a parked list is never + /// parked behind it — the list is promoted and the event resolves, so no trim can lose it. + /// + [Fact] + public void A_resolved_handle_with_a_stranded_parked_list_promotes_it_and_resolves() + { + var registry = new SubscriptionRegistry(); + var session = registry.CurrentSession; + var window = registry.BeginPendingSubscribe(); + registry.Register(1, [new TagBinding("Tank.Level", 7)]); + registry.ForceParkForTests(DataChange(7, 1)); // the "should be impossible" stranded list + + registry.ResolveOrPark(DataChange(7, 2), session, out var subs, out var replayFirst) + .ShouldBe(EarlyEventOutcome.Resolved); + subs.Single().FullReference.ShouldBe("Tank.Level"); + replayFirst.Select(e => e.Value.DoubleValue).ShouldBe([1.0]); + window.Dispose(); registry.ParkedEventCount.ShouldBe(0); - registry.ResolveOrPark(DataChange(8, 1), epoch, out _).ShouldBe(EarlyEventOutcome.Parked, - "positive control: once replayed, the room is back"); } /// A registered handle resolves normally even while a window is open. @@ -174,11 +243,11 @@ public sealed class EarlyEventWindowTests public void Registered_handle_resolves_while_a_window_is_open() { var registry = new SubscriptionRegistry(); - var epoch = registry.StartNewEpoch(); + var epoch = registry.CurrentSession; registry.Register(1, [new TagBinding("Tank.Level", 7)]); using var window = registry.BeginPendingSubscribe(); - registry.ResolveOrPark(DataChange(7, 1), epoch, out var subs).ShouldBe(EarlyEventOutcome.Resolved); + registry.ResolveOrPark(DataChange(7, 1), epoch, out var subs, out _).ShouldBe(EarlyEventOutcome.Resolved); subs.Single().FullReference.ShouldBe("Tank.Level"); } @@ -188,25 +257,71 @@ public sealed class EarlyEventWindowTests /// neither park nor replay — so a reused handle number never receives an old-session value. /// [Fact] - public void A_new_epoch_discards_held_events_and_fences_the_old_epoch() + public void A_new_session_discards_held_events_and_fences_the_old_session() { var registry = new SubscriptionRegistry(); - var oldEpoch = registry.StartNewEpoch(); + var oldSession = registry.CurrentSession; using var window = registry.BeginPendingSubscribe(); - registry.ResolveOrPark(DataChange(7, 111), oldEpoch, out _).ShouldBe(EarlyEventOutcome.Parked); - registry.ResolveOrPark(DataChange(8, 112), oldEpoch, out _).ShouldBe(EarlyEventOutcome.Parked); + registry.ResolveOrPark(DataChange(7, 111), oldSession, out _, out _).ShouldBe(EarlyEventOutcome.Parked); + registry.ResolveOrPark(DataChange(8, 112), oldSession, out _, out _).ShouldBe(EarlyEventOutcome.Parked); registry.Register(1, [new TagBinding("Old.A", 8)]); // 8 promoted to ready, never replayed - var newEpoch = registry.StartNewEpoch(); + var newSession = registry.StartNewSession(); registry.ParkedEventCount.ShouldBe(0); - registry.TakeReadyEvents(newEpoch).ShouldBeEmpty(); + registry.TakeReadyEvents(newSession).ShouldBeEmpty(); registry.Register(2, [new TagBinding("New.B", 7)]); // new session reuses handle 7 - registry.TakeReadyEvents(newEpoch).ShouldBeEmpty("the old session's value must not reach New.B"); + registry.TakeReadyEvents(newSession).ShouldBeEmpty("the old session's value must not reach New.B"); - registry.ResolveOrPark(DataChange(9, 1), oldEpoch, out _).ShouldBe(EarlyEventOutcome.Stale); - registry.TakeReadyEvents(oldEpoch).ShouldBeEmpty(); - registry.ResolveOrPark(DataChange(9, 1), newEpoch, out _).ShouldBe(EarlyEventOutcome.Parked, - "positive control: the current epoch still parks"); + registry.ResolveOrPark(DataChange(7, 1), oldSession, out _, out _).ShouldBe(EarlyEventOutcome.Stale, + "a dying old-session pump must not resolve through the map, even for a registered number"); + registry.ResolveOrPark(DataChange(9, 1), oldSession, out _, out _).ShouldBe(EarlyEventOutcome.Stale); + registry.TakeReadyEvents(oldSession).ShouldBeEmpty(); + registry.ResolveOrPark(DataChange(9, 1), newSession, out _, out _).ShouldBe(EarlyEventOutcome.Parked, + "positive control: the current session still parks"); + } + + /// + /// I-1: a new session invalidates the reverse map but keeps the subscriptions, so a new-session + /// event on a number an old binding still holds PARKS (instead of resolving to the old tag) and + /// is promoted by the replay's Rebind to the right tag. + /// + [Fact] + public void A_new_session_invalidates_old_handles_so_colliding_numbers_park_until_rebind() + { + var registry = new SubscriptionRegistry(); + registry.Register(1, [new TagBinding("Old.A", 7)]); + registry.Register(2, [new TagBinding("Old.B", 8)]); + + var session = registry.StartNewSession(); + registry.TrackedSubscriptionCount.ShouldBe(2, "subscriptions survive for the replay to rebind"); + using var window = registry.BeginPendingSubscribe(); + // New session: Old.B is re-issued as handle 7 — the number Old.A still holds until its own Rebind. + registry.ResolveOrPark(DataChange(7, 5), session, out _, out _).ShouldBe(EarlyEventOutcome.Parked); + registry.TryRebind(2, [new TagBinding("Old.B", 7)], session).ShouldBeTrue(); + + registry.TakeReadyEvents(session).ShouldHaveSingleItem().Value.DoubleValue.ShouldBe(5.0); + registry.ResolveSubscribers(7).ShouldBe([(2L, "Old.B")]); + } + + /// #861: a registration or rebind carrying a stale session is refused. + [Fact] + public void Registration_with_a_stale_session_is_refused() + { + var registry = new SubscriptionRegistry(); + var before = registry.CurrentSession; + registry.Register(1, [new TagBinding("Tank.Level", 7)]); + registry.StartNewSession(); + + registry.TryRegister(2, [new TagBinding("Late", 9)], before).ShouldBeFalse(); + registry.Contains(2).ShouldBeFalse(); + registry.TryRebind(1, [new TagBinding("Tank.Level", 9)], before).ShouldBeFalse(); + registry.ResolveSubscribers(9).ShouldBeEmpty(); + registry.TryRegister(3, [new TagBinding("Fresh", 9)], registry.CurrentSession).ShouldBeTrue("positive control"); + + registry.Remove(1, out var live).ShouldNotBeNull(); + live.ShouldBeFalse("never rebound in the new session — its handle 7 must not be unsubscribed"); + registry.Remove(3, out live).ShouldNotBeNull(); + live.ShouldBeTrue(); } // ===== EventPump ===== @@ -368,6 +483,7 @@ public sealed class EarlyEventWindowTests oldStream.Emit(DataChange(7, 111)); await TestWait.UntilAsync(() => registry.ParkedEventCount == 1, "the old session's event must be parked"); await oldPump.DisposeAsync(); + registry.StartNewSession(); // what GalaxyDriver.ReplayAsync does before recreating the pump var newStream = new ChannelSubscriber(); await using var newPump = new EventPump(newStream, registry, channelCapacity: 16, clientName: "New"); @@ -494,4 +610,172 @@ public sealed class EarlyEventWindowTests args.FullReference.ShouldBe("PlatformA.ScanState"); ((GalaxySubscriptionHandle)args.SubscriptionHandle).SubscriptionId.ShouldBe(SubscriptionRegistry.ProbeSubscriptionId); } + + // ===== Reconnect / session (#859 I-1, #861) ===== + + /// + /// A gateway fake whose SubscribeBulk calls follow a per-call script (default: fresh handles). + /// Events go on one stream that survives pump restarts. + /// + private sealed class ScriptedSubscriber : IGalaxySubscriber + { + private readonly Channel _events = Channel.CreateUnbounded(); + private int _nextHandle = 1_000; + + public ConcurrentQueue, CancellationToken, Task>>> Script { get; } = new(); + public ConcurrentQueue Unsubscribed { get; } = new(); + public int SubscribeCalls; + + public Task> SubscribeBulkAsync( + IReadOnlyList fullReferences, int bufferedUpdateIntervalMs, CancellationToken cancellationToken) + { + Interlocked.Increment(ref SubscribeCalls); + if (Script.TryDequeue(out var step)) return step(fullReferences, cancellationToken); + return Task.FromResult>( + [.. fullReferences.Select(r => Ok(r, Interlocked.Increment(ref _nextHandle)))]); + } + + public Task UnsubscribeBulkAsync(IReadOnlyList itemHandles, CancellationToken cancellationToken) + { + foreach (var h in itemHandles) Unsubscribed.Enqueue(h); + return Task.CompletedTask; + } + + public async IAsyncEnumerable StreamEventsAsync([EnumeratorCancellation] CancellationToken cancellationToken) + { + await foreach (var ev in _events.Reader.ReadAllAsync(cancellationToken).ConfigureAwait(false)) + { + yield return ev; + } + } + + public void Emit(MxEvent ev) => _events.Writer.TryWrite(ev).ShouldBeTrue(); + } + + private static SubscribeResult Ok(string tag, int handle) => + new() { TagAddress = tag, ItemHandle = handle, WasSuccessful = true }; + + /// + /// I-1 end to end: after a reconnect the new session hands Tank.A the number Tank.B held in the + /// old session (and vice versa), and each initial value arrives BEFORE its SubscribeBulk reply. + /// Every value must reach its own tag — not the tag that owned the number before the reconnect. + /// + [Fact] + public async Task Replay_with_colliding_handles_and_event_before_reply_delivers_each_value_to_its_own_tag() + { + var subscriber = new ScriptedSubscriber(); + using var driver = new GalaxyDriver( + "g", Opts(), hierarchySource: null, dataReader: null, dataWriter: null, subscriber: subscriber); + var registry = driver.SubscriptionsForTests; + var delivered = new ConcurrentQueue<(string Ref, double Value)>(); + driver.OnDataChange += (_, e) => delivered.Enqueue((e.FullReference, (double)e.Snapshot.Value!)); + + subscriber.Script.Enqueue((refs, _) => Task.FromResult>([Ok(refs[0], 1)])); + subscriber.Script.Enqueue((refs, _) => Task.FromResult>([Ok(refs[0], 2)])); + await driver.SubscribeAsync(["Tank.A"], TimeSpan.Zero, TestContext.Current.CancellationToken); + await driver.SubscribeAsync(["Tank.B"], TimeSpan.Zero, TestContext.Current.CancellationToken); + + // New session: swapped numbers; each initial value is on the stream (and parked) before the reply. + var newHandle = new Dictionary { ["Tank.A"] = 2, ["Tank.B"] = 1 }; + var newValue = new Dictionary { ["Tank.A"] = 10.0, ["Tank.B"] = 20.0 }; + for (var i = 0; i < 2; i++) + { + subscriber.Script.Enqueue(async (refs, _) => + { + var tag = refs[0]; + var before = registry.ParkedEventCount; + subscriber.Emit(DataChange(newHandle[tag], newValue[tag])); + await TestWait.UntilAsync(() => registry.ParkedEventCount > before, + $"{tag}'s new-session initial value must PARK, not resolve to the old owner of handle {newHandle[tag]}"); + return [Ok(tag, newHandle[tag])]; + }); + } + + await driver.InvokeReplayForTestAsync(TestContext.Current.CancellationToken); + + await TestWait.UntilAsync(() => delivered.Count >= 2, "both replayed initial values must be published", + () => string.Join(", ", delivered)); + delivered.OrderBy(d => d.Ref).ShouldBe([("Tank.A", 10.0), ("Tank.B", 20.0)]); + registry.ResolveSubscribers(1).Single().FullReference.ShouldBe("Tank.B"); + registry.ResolveSubscribers(2).Single().FullReference.ShouldBe("Tank.A"); + } + + /// + /// #861: a SubscribeAsync whose reply returns after a reconnect does not register the dead + /// old-session handles (nor unsubscribe their numbers) — it re-issues on the new session. + /// + [Fact] + public async Task Subscribe_whose_reply_straddles_a_reconnect_is_reissued_on_the_new_session() + { + var subscriber = new ScriptedSubscriber(); + using var driver = new GalaxyDriver( + "g", Opts(), hierarchySource: null, dataReader: null, dataWriter: null, subscriber: subscriber); + var registry = driver.SubscriptionsForTests; + var replyGate = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + subscriber.Script.Enqueue(async (refs, _) => + { + await replyGate.Task; + return [Ok(refs[0], 500)]; // an old-session handle + }); + subscriber.Script.Enqueue((refs, _) => Task.FromResult>([Ok(refs[0], 600)])); + + var subscribe = driver.SubscribeAsync(["Tank.X"], TimeSpan.Zero, TestContext.Current.CancellationToken); + await TestWait.UntilAsync(() => Volatile.Read(ref subscriber.SubscribeCalls) == 1, "the first SubscribeBulk must be in flight"); + await driver.InvokeReplayForTestAsync(TestContext.Current.CancellationToken); // the reconnect + replyGate.SetResult(); + + var handle = await TestWait.ForAsync(subscribe, "the subscribe must complete on the new session"); + Volatile.Read(ref subscriber.SubscribeCalls).ShouldBe(2); + registry.ResolveSubscribers(500).ShouldBeEmpty("the dead old-session handle must not be registered"); + registry.ResolveSubscribers(600).ShouldBe([(((GalaxySubscriptionHandle)handle).SubscriptionId, "Tank.X")]); + subscriber.Unsubscribed.ShouldNotContain(500, "its number may belong to a new-session item"); + } + + /// #861: a subscribe-once read whose reply straddles a reconnect reports the lost session. + [Fact] + public async Task Read_whose_reply_straddles_a_reconnect_returns_bad_communication_and_leaves_old_handles_alone() + { + var subscriber = new ScriptedSubscriber(); + using var driver = new GalaxyDriver( + "g", Opts(), hierarchySource: null, dataReader: null, dataWriter: null, subscriber: subscriber); + var registry = driver.SubscriptionsForTests; + var replyGate = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + subscriber.Script.Enqueue(async (refs, _) => + { + await replyGate.Task; + return [Ok(refs[0], 700)]; + }); + + var read = driver.ReadAsync(["Tank.Y"], TestContext.Current.CancellationToken); + await TestWait.UntilAsync(() => Volatile.Read(ref subscriber.SubscribeCalls) == 1, "the read's SubscribeBulk must be in flight"); + await driver.InvokeReplayForTestAsync(TestContext.Current.CancellationToken); + replyGate.SetResult(); + + var snapshot = (await TestWait.ForAsync(read, "the read must complete")).ShouldHaveSingleItem(); + snapshot.StatusCode.ShouldBe(StatusCodeMap.BadCommunicationError); + registry.ResolveSubscribers(700).ShouldBeEmpty(); + subscriber.Unsubscribed.ShouldBeEmpty(); + registry.PendingSubscribeCount.ShouldBe(0, "the read's early-event window must be closed"); + } + + /// The read's internal-deadline path closes its early-event window. + [Fact] + public async Task Read_deadline_path_closes_its_window() + { + var subscriber = new ScriptedSubscriber(); + using var driver = new GalaxyDriver( + "g", Opts(), hierarchySource: null, dataReader: null, dataWriter: null, subscriber: subscriber) + { + ReadViaSubscribeDeadline = TimeSpan.FromMilliseconds(100), + }; + var never = new TaskCompletionSource>(TaskCreationOptions.RunContinuationsAsynchronously); + subscriber.Script.Enqueue((_, _) => never.Task); + + var snapshot = (await TestWait.ForAsync( + driver.ReadAsync(["Tank.Z"], TestContext.Current.CancellationToken), "the read must time out")).ShouldHaveSingleItem(); + + snapshot.StatusCode.ShouldBe(StatusCodeMap.BadTimeout); + driver.SubscriptionsForTests.PendingSubscribeCount.ShouldBe(0); + never.SetResult([]); // let the abandoned RPC finish cleanly + } } From 133bc07da5b30b3998635cea546e78c81e143017 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Wed, 11 Nov 2026 11:02:55 -0500 Subject: [PATCH 4/5] =?UTF-8?q?fix(galaxy):=20#859/#861=20third=20review?= =?UTF-8?q?=20=E2=80=94=20read=20rebind=20release,=20probe=20session=20fen?= =?UTF-8?q?ce,=20reopen=20fence,=20pump=20restart?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit B-1: the subscribe-once read's cleanup unsubscribes what the registry holds at removal (the replay may have rebound it to new-session handles), not its original bindings; nothing when the entry still holds dead old-session handles. Found while testing it: reads were attached to the pump INSTANCE, so a read across a reconnect's pump restart never saw the replayed value — reads now listen on a driver-level ReadDataChange raised for whichever pump is live. I-A: PerPlatformProbeWatcher records the registry session with each probe handle. Publishing goes through the new fenced TryUpsert and binds only current-session handles; removal/stop unsubscribe and purge only those; a seed reply that straddled a reconnect records nothing and is re-issued (max 3). I-B: PurgeParked takes the session captured BEFORE the unsubscribe RPC and is a no-op under the park gate if the session changed. I-C: ReopenAsync starts a new registry session before AND after the gateway session swap; ReplayAsync does so only when no reopen ran (injected subscriber / tests) and replays only subscriptions still holding older-session handles, so a subscribe issued after the swap is live and not double-advised. P-1: ReplayAsync restarts the pump before any early return (no entries used to leave the dead pump in place). P-2: ReplayOnSessionLost=false logs a Warning; misleading comment about gateway-side replay fixed. M-a: TryRebind returns BindOutcome; UnknownId releases the handles just advised for a subscription removed mid-replay. M-b: the replay/re-snapshot pass reads the session once and stops if it changes. M-c: parked entries drop their MxEvent when they leave the parked set. M-d (retry a straddled read) not done. Tests (TestWait) for each, each checked red against a mutation of its fix. Refs #859, #861. Claude-Session: https://claude.ai/code/session_01GT67AD99zQQqpLCVm3p7xk --- docs/drivers/Galaxy.md | 2 +- .../GalaxyDriver.cs | 175 +++++++++++---- .../Health/PerPlatformProbeWatcher.cs | 98 +++++++-- .../Runtime/SubscriptionRegistry.cs | 88 ++++++-- .../Runtime/EarlyEventWindowTests.cs | 201 +++++++++++++++++- 5 files changed, 481 insertions(+), 83 deletions(-) diff --git a/docs/drivers/Galaxy.md b/docs/drivers/Galaxy.md index 0022abed1..303f7cb2a 100644 --- a/docs/drivers/Galaxy.md +++ b/docs/drivers/Galaxy.md @@ -172,7 +172,7 @@ Every (re)open of `StreamAlarms` replays the gateway's active-alarm snapshot. `G - **Replay gaps** (`galaxy.events.replay_gaps`): the gateway signals that events were evicted from its replay ring before we could read them. The driver responds by re-issuing `SubscribeBulk` for every tracked subscription — MXAccess delivers an initial value per item on subscribe, so re-subscribing *is* the re-snapshot the contract asks for. It does **not** restart the event pump (the stream carrying the sentinel is healthy) and it honours `Reconnect:ReplayOnSessionLost=false` by logging and standing down, on the grounds that an operator who delegated recovery to the gateway did not ask for a re-subscribe on a different trigger. ⚠️ This cannot currently fire: the sentinel is only emitted on a stream opened with `after_worker_sequence > 0`, and the driver opens with `0`, which the gateway treats as "fresh stream, not a resume". Enabling resume is a bigger change than it looks — the sequence is only meaningful **within one gateway session**, and the reconnect path recreates the session, so a remembered sequence replayed against a fresh session would set the server's live watermark *above* every event it is about to send, silencing the stream rather than recovering it. - **Undecodable values** (`galaxy.events.undecodable`): a value kind this driver has no CLR representation for is published as **`BadTypeMismatch` with a null value**, and the first occurrence of each distinct kind is logged. It is *not* published as a null carrying the event's own quality: an MXAccess event can be Good-quality while the payload is one we cannot map, and a good null is indistinguishable from a genuine null reading — a client would consume a value it never received as a real one, with the failure visible only in a log. Non-zero here means the gateway is sending a wire type `MxValueDecoder` has no mapping for and those readings are being lost; the fix is a decoder mapping, not a quality change. -- **Initial values can overtake the subscribe reply (#859)** (`galaxy.events.early_parked` / `early_replayed` / `early_overflow`): mxaccessgw gives no reply-before-event ordering — the worker frames the SubscribeBulk reply on a thread-pool continuation while the STA is already pumping the OnDataChange that Advise queued, and the reply (unary Invoke) and the event (StreamEvents) are separate HTTP/2 streams anyway. An event for an unregistered handle is therefore only "stale after unsubscribe" when no subscribe is in flight. Every SubscribeBulk call site (`SubscribeAsync`, the subscribe-once `ReadAsync` — which waits only for the initial value and would otherwise read `BadTimeout` — the reconnect/replay-gap `ResubscribeAllAsync` (one window per subscription, not across the whole replay), `PerPlatformProbeWatcher`) opens `SubscriptionRegistry.BeginPendingSubscribe()` before the RPC and closes it right after registering, never later; inside the window unknown-handle events are parked, promoted by the `Register`/`Rebind` that binds the handle — map writes and promotion are one critical section, so no later event can resolve in between — and replayed on the pump's dispatch thread before any later event for that handle. Held events (parked + promoted-but-unreplayed) are bounded at 100 000 together. Unclaimed parked events are trimmed by **window sequence**, not only when every window has closed (concurrent reads keep some window open almost permanently): each window gets a monotonically increasing sequence when it opens, each parked event is stamped with the newest sequence, and once every window that opened at or before that stamp has closed the event is discarded — outside any window the stale drop is unchanged. **Sessions (#861):** item handles are per gateway session and the next session reuses the numbers. On reconnect `ReplayAsync` first calls `SubscriptionRegistry.StartNewSession()`, which discards everything held, **invalidates the reverse map** (subscriptions are kept; new-session events park until the replay's per-subscription `Rebind` promotes them, instead of resolving to the tag that owned the number before), and bumps a session number. Registration and rebind are refused for a stale session, so a `SubscribeAsync` whose reply straddled the reconnect is **re-issued on the new session** (up to 3 attempts), a subscribe-once read returns `BadCommunicationError`, and dead old-session handle numbers are never sent to `UnsubscribeBulk`; an old session's pump can no longer resolve through the map. **Interim (I-3):** a completed `UnsubscribeBulk` purges anything parked for its handles, so a trailing event of a removed item is not promoted onto a reused handle number within one session. It cannot catch an event still in flight after the unsubscribe, and it also drops a genuine early value for a number reused that fast (a timeout rather than another tag's value). The complete fix needs a worker sequence on the SubscribeBulk reply so the driver can tell pre-subscribe events from post-subscribe ones — an optional cross-repo change in mxaccessgw. +- **Initial values can overtake the subscribe reply (#859)** (`galaxy.events.early_parked` / `early_replayed` / `early_overflow`): mxaccessgw gives no reply-before-event ordering — the worker frames the SubscribeBulk reply on a thread-pool continuation while the STA is already pumping the OnDataChange that Advise queued, and the reply (unary Invoke) and the event (StreamEvents) are separate HTTP/2 streams anyway. An event for an unregistered handle is therefore only "stale after unsubscribe" when no subscribe is in flight. Every SubscribeBulk call site (`SubscribeAsync`, the subscribe-once `ReadAsync` — which waits only for the initial value and would otherwise read `BadTimeout` — the reconnect/replay-gap `ResubscribeAllAsync` (one window per subscription, not across the whole replay), `PerPlatformProbeWatcher`) opens `SubscriptionRegistry.BeginPendingSubscribe()` before the RPC and closes it right after registering, never later; inside the window unknown-handle events are parked, promoted by the `Register`/`Rebind` that binds the handle — map writes and promotion are one critical section, so no later event can resolve in between — and replayed on the pump's dispatch thread before any later event for that handle. Held events (parked + promoted-but-unreplayed) are bounded at 100 000 together. Unclaimed parked events are trimmed by **window sequence**, not only when every window has closed (concurrent reads keep some window open almost permanently): each window gets a monotonically increasing sequence when it opens, each parked event is stamped with the newest sequence, and once every window that opened at or before that stamp has closed the event is discarded — outside any window the stale drop is unchanged. **Sessions (#861):** item handles are per gateway session and the next session reuses the numbers. On reconnect `ReopenAsync` calls `SubscriptionRegistry.StartNewSession()` both before and after the gateway session swap (`ReplayAsync` does it only when no reopen ran), and the replay re-subscribes only subscriptions still holding the previous session's handles (one registered after the swap is already live). The pump is always restarted, even with nothing to replay. `StartNewSession()` discards everything held, **invalidates the reverse map** (subscriptions are kept; new-session events park until the replay's per-subscription `Rebind` promotes them, instead of resolving to the tag that owned the number before), and bumps a session number. Registration and rebind are refused for a stale session, so a `SubscribeAsync` whose reply straddled the reconnect is **re-issued on the new session** (up to 3 attempts), a subscribe-once read returns `BadCommunicationError`, and dead old-session handle numbers are never sent to `UnsubscribeBulk` (a read rebound by the replay releases the handles the registry holds, not its original ones); an old session's pump can no longer resolve through the map. Reads listen on the driver, not on a pump instance, so a read survives the reconnect's pump restart. `PerPlatformProbeWatcher` records the session with each probe handle: only current-session handles are published (fenced `TryUpsert`), unsubscribed or purged, and a seed reply that straddled a reconnect is re-issued. Purges are fenced by the session captured before the unsubscribe RPC. **Interim (I-3):** a completed `UnsubscribeBulk` purges anything parked for its handles, so a trailing event of a removed item is not promoted onto a reused handle number within one session. It cannot catch an event still in flight after the unsubscribe, and it also drops a genuine early value for a number reused that fast (a timeout rather than another tag's value). The complete fix needs a worker sequence on the SubscribeBulk reply so the driver can tell pre-subscribe events from post-subscribe ones — an optional cross-repo change in mxaccessgw. - **Duplicate `attributeRef` (#605 / 06-S-13)**: two authored RawPaths dialling the same Galaxy attribute are both dialled, but the reverse map is first-wins, so value fan-out and alarm `ConditionId` surface under the first RawPath only. The loser is logged at Warning and recorded in the skipped-tag ledger with the winner path in the reason. - **Subscribe-once reads are bounded (#605 / 06-S-3)**: `ReadAsync` on the production path (SubscribeBulk, first event, Unsubscribe) carries an internal 30 s deadline in addition to the caller token; slots that never publish come back `BadTimeout`, so `CancellationToken.None` can no longer hang on a silent tag. - **Supervisory advise is retried after a thrown failure (#605 / 06-S-12)**: the writer marks an item handle advised before the call; a thrown advise (not just a non-OK status) now unmarks it, fails the write closed and re-advises on the next write. diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/GalaxyDriver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/GalaxyDriver.cs index 7e77a70ce..7399b7f92 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/GalaxyDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/GalaxyDriver.cs @@ -250,6 +250,9 @@ public sealed class GalaxyDriver /// Test seam: true while an is running. internal bool HasEventPumpForTests { get { lock (_pumpLock) return _eventPump is not null; } } + /// Test seam — the current pump instance (identity only), to observe a restart. + internal object? EventPumpInstanceForTests { get { lock (_pumpLock) return _eventPump; } } + /// Test seam: the number of driver subscriptions the registry still tracks. internal int TrackedSubscriptionCountForTests => _subscriptions.TrackedSubscriptionCount; @@ -671,13 +674,17 @@ public sealed class GalaxyDriver private async Task UnsubscribeAndPurgeAsync( IGalaxySubscriber subscriber, IReadOnlyList itemHandles, CancellationToken cancellationToken) { + // Captured BEFORE the RPC (#859 I-B): a slow or failed unsubscribe can finish after a reconnect, + // when these numbers may be the NEW session's — the purge is then a no-op instead of deleting + // their parked initial values. + var session = _subscriptions.CurrentSession; try { await subscriber.UnsubscribeBulkAsync(itemHandles, cancellationToken).ConfigureAwait(false); } finally { - _subscriptions.PurgeParked(itemHandles); + _subscriptions.PurgeParked(itemHandles, session); } } @@ -948,14 +955,49 @@ public sealed class GalaxyDriver /// private async Task ReopenAsync(CancellationToken cancellationToken) { - if (_ownedMxSession is null) return; - var clientOptions = await BuildClientOptionsAsync(_options.Gateway, cancellationToken).ConfigureAwait(false); - await _ownedMxSession.RecreateAsync(clientOptions, cancellationToken).ConfigureAwait(false); + if (_ownedMxSession is null && RecreateOverrideForTests is null) return; + + // #859 I-C: fence the registry around the swap itself, not later in ReplayAsync — a + // SubscribeBulk issued between RecreateAsync and the replay goes to the NEW gateway session. + // BEFORE: from here on nothing registered so far counts as live, so an UnsubscribeAsync during + // reopen/backoff never sends an old number (which the new session may reuse). + _subscriptions.StartNewSession(); + if (RecreateOverrideForTests is { } recreate) + { + await recreate(cancellationToken).ConfigureAwait(false); + } + else + { + var clientOptions = await BuildClientOptionsAsync(_options.Gateway, cancellationToken).ConfigureAwait(false); + await _ownedMxSession!.RecreateAsync(clientOptions, cancellationToken).ConfigureAwait(false); + } + // AFTER: a subscribe that captured the session between the two bumps may have dialled the old + // (dead) session, so it is refused and re-issued; one that captures it from here on dials the + // recreated session (GatewayGalaxySubscriber reads GalaxyMxSession.Session at call time), so + // it registers normally and the replay leaves it alone instead of re-subscribing it. + Volatile.Write(ref _sessionOpenedByReopen, _subscriptions.StartNewSession()); + // The recreated session invalidates every prior gw item handle; drop the writer's handle/advise // caches so the next write re-AddItems + re-AdviseSupervisory against the fresh session. _dataWriter?.InvalidateHandleCaches(); } + /// + /// The registry session opened for the recreated gateway session, or -1. + /// starts a new session itself only when no reopen did (the injected- + /// subscriber path, and tests); a second bump after a reopen would fence subscribes that already + /// registered against the recreated session. + /// + private int _sessionOpenedByReopen = -1; + + /// Test seam — replaces the gateway session swap inside . + internal Func? RecreateOverrideForTests { get; set; } + + /// Test-visible entry into . + /// Cancellation token. + /// A task that represents the asynchronous operation. + internal Task InvokeReopenForTestAsync(CancellationToken cancellationToken) => ReopenAsync(cancellationToken); + /// /// Replay callback. Walks every active subscription, re-issues SubscribeBulk for /// its tag list, and rebinds the @@ -977,32 +1019,38 @@ public sealed class GalaxyDriver // may be reused by the new session (#859 I-1 / #861). Invalidate the reverse map (the // subscriptions stay; the per-iteration Rebind below re-binds each one, and new-session // events park meanwhile instead of resolving to the old owner of a reused number), discard - // held events, and fence any subscribe whose reply straddled the reconnect. Done FIRST and - // unconditionally — before the pump restart and even with nothing to replay. - _subscriptions.StartNewSession(); - - var entries = _subscriptions.SnapshotEntries(); - if (entries.Count == 0) return; - - // Honor ReplayOnSessionLost. When operators opt out (false) we skip the - // per-tag SubscribeBulk fan-out — they're delegating to the gateway's - // session-level ReplaySubscriptions or accept post-reconnect tag loss. We - // still restart the EventPump so a future Subscribe call lands on a live - // consumer. - if (!_options.Reconnect.ReplayOnSessionLost) + // held events, and fence any subscribe whose reply straddled the reconnect. ReopenAsync + // already did this around the session swap; only when no reopen ran (injected subscriber, + // tests) is it done here — FIRST, before the pump restart and even with nothing to replay. + if (Interlocked.Exchange(ref _sessionOpenedByReopen, -1) != _subscriptions.CurrentSession) { - RestartEventPumpForReplay(); - _logger.LogInformation( - "GalaxyDriver {InstanceId} reconnect replay skipped — ReplayOnSessionLost=false ({SubCount} subscriptions tracked)", - _driverInstanceId, entries.Count); - return; + _subscriptions.StartNewSession(); } // The stream-fault that triggered this recovery left the old pump's RunAsync loop // exited and its channel completed; EventPump.Start() is a no-op on a non-null but - // completed loop. Recreate the pump so the replayed subscriptions have a consumer. + // completed loop. Recreate the pump BEFORE anything can return early (#859 P-1): with no + // subscriptions to replay the dead pump used to stay in _eventPump, so every later + // Subscribe/Read reused it and was silently deaf while health stayed Healthy. RestartEventPumpForReplay(); + // Only subscriptions still holding the previous session's handles: one registered after the + // reopen is already live on the recreated session (re-subscribing it would double-advise). + var entries = _subscriptions.SnapshotEntriesNotIn(_subscriptions.CurrentSession); + if (entries.Count == 0) return; + + // Honor ReplayOnSessionLost. When operators opt out (false) the per-tag SubscribeBulk fan-out + // is skipped, and the recreated gateway session carries NONE of the old advises — those + // subscriptions stay deaf (their handles are dead and no longer resolve) until re-subscribed. + if (!_options.Reconnect.ReplayOnSessionLost) + { + _logger.LogWarning( + "GalaxyDriver {InstanceId} reconnect replay skipped — ReplayOnSessionLost=false; {SubCount} subscription(s) " + + "receive no further data until they are re-subscribed (the recreated gateway session has none of their advises).", + _driverInstanceId, entries.Count); + return; + } + var tagCount = await ResubscribeAllAsync(entries, Volatile.Read(ref _runtimeGeneration), cancellationToken) .ConfigureAwait(false); @@ -1032,6 +1080,9 @@ public sealed class GalaxyDriver CancellationToken cancellationToken) { var tagCount = 0; + // M-b: the session this pass belongs to, read ONCE. Any later change fences the rest of the + // pass (the new session's own replay takes over). + var session = _subscriptions.CurrentSession; foreach (var (subscriptionId, oldBindings) in entries) { if (Volatile.Read(ref _runtimeGeneration) != generation) @@ -1039,6 +1090,7 @@ public sealed class GalaxyDriver _logger.LogDebug("GalaxyDriver {InstanceId} re-subscribe abandoned — runtime was torn down.", _driverInstanceId); return tagCount; } + if (_subscriptions.CurrentSession != session) return tagCount; // a newer session's replay owns the rest if (subscriptionId == SubscriptionRegistry.ProbeSubscriptionId && _probeWatcher is { } probeWatcher) { @@ -1061,7 +1113,6 @@ public sealed class GalaxyDriver // window held across all N round-trips would park every unrelated unknown-handle event // for the whole replay and could exhaust the bound before the genuine early values. using var earlyEventWindow = _subscriptions.BeginPendingSubscribe(); - var session = _subscriptions.CurrentSession; var results = await _subscriber! .SubscribeBulkAsync(refs, _options.MxAccess.PublishingIntervalMs, cancellationToken) .ConfigureAwait(false); @@ -1079,18 +1130,41 @@ public sealed class GalaxyDriver // Rebind so the EventPump fan-out reverse map points at the post-reconnect // handles; otherwise events on the new handles miss every subscription. Not after a // teardown: the fresh handles belong to a subscriber that is already being released. + SubscriptionRegistry.BindOutcome outcome; lock (_registryGate) { if (Volatile.Read(ref _runtimeGeneration) != generation) return tagCount; - // #861: another reconnect during this round-trip makes these handles dead too; the - // newer session's own replay re-subscribes everything, so stop here. - if (!_subscriptions.TryRebind(subscriptionId, newBindings, session)) + outcome = _subscriptions.TryRebind(subscriptionId, newBindings, session); + } + + if (outcome == SubscriptionRegistry.BindOutcome.SessionChanged) + { + // #861: another reconnect during the pass makes these handles dead too; the newer + // session's own replay re-subscribes everything, so stop here. + _logger.LogDebug( + "GalaxyDriver {InstanceId} re-subscribe abandoned — the gateway session changed again.", + _driverInstanceId); + return tagCount; + } + if (outcome == SubscriptionRegistry.BindOutcome.UnknownId) + { + // M-a: the subscription (or a subscribe-once read) was removed while its SubscribeBulk + // was in flight. Nothing will ever unsubscribe what was just advised — release it now. + var orphaned = newBindings.Where(b => b.ItemHandle > 0).Select(b => b.ItemHandle).ToArray(); + if (orphaned.Length > 0) { - _logger.LogDebug( - "GalaxyDriver {InstanceId} re-subscribe abandoned — the gateway session changed again.", - _driverInstanceId); - return tagCount; + try + { + await UnsubscribeAndPurgeAsync(_subscriber!, orphaned, CancellationToken.None).ConfigureAwait(false); + } + catch (Exception ex) + { + _logger.LogWarning(ex, + "GalaxyDriver {InstanceId} could not release {Count} handle(s) re-subscribed for a subscription removed during the pass.", + _driverInstanceId, orphaned.Length); + } } + continue; } tagCount += refs.Length; } @@ -1446,6 +1520,13 @@ public sealed class GalaxyDriver return ReadViaSubscribeOnceAsync(dialledRefs, cancellationToken); } + /// + /// Raw (attributeRef-domain) data changes for in-flight subscribe-once reads. Raised by + /// for whichever pump is current, so a read survives a reconnect's + /// pump restart. + /// + private event EventHandler? ReadDataChange; + /// Upper bound on one subscribe-once read; internal so tests can shrink it. internal TimeSpan ReadViaSubscribeDeadline { get; set; } = TimeSpan.FromSeconds(30); @@ -1500,7 +1581,7 @@ public sealed class GalaxyDriver tcs.TrySetResult(args.Snapshot); } }; - pump.OnDataChange += handler; + ReadDataChange += handler; var bufferedIntervalMs = _options.MxAccess.PublishingIntervalMs; IReadOnlyList results; @@ -1525,7 +1606,7 @@ public sealed class GalaxyDriver catch (OperationCanceledException) when (!callerToken.IsCancellationRequested) { // Internal deadline fired while the subscribe RPC was still in flight. - pump.OnDataChange -= handler; + ReadDataChange -= handler; var subscriber = readSubscriber; var logger = _logger; _ = subscribeTask.ContinueWith(t => @@ -1550,7 +1631,7 @@ public sealed class GalaxyDriver } catch { - pump.OnDataChange -= handler; + ReadDataChange -= handler; throw; } @@ -1584,7 +1665,7 @@ public sealed class GalaxyDriver var registered = TryRegisterForGeneration(subscriptionId, bindings, generation, session); if (registered != RegisterOutcome.Registered) { - pump.OnDataChange -= handler; + ReadDataChange -= handler; earlyEventWindow.Dispose(); // before any further round-trip if (registered == RegisterOutcome.TornDown) { @@ -1637,16 +1718,20 @@ public sealed class GalaxyDriver } finally { - pump.OnDataChange -= handler; + ReadDataChange -= handler; // Drop the bindings + unsubscribe the live handles. UnsubscribeBulkAsync's // failure isn't fatal — the registry is already cleared, so any straggling // event from the gw would be a no-op fan-out. var removed = _subscriptions.Remove(subscriptionId, out var liveInCurrentSession); - // A reconnect during the wait left these handles in the dead session; their numbers may now - // be new-session items, so they must not be unsubscribed (#861). - var staleSession = removed is not null && !liveInCurrentSession; - var liveHandles = bindings.Where(b => b.ItemHandle > 0).Select(b => b.ItemHandle).ToArray(); - if (liveHandles.Length > 0 && !staleSession) + // Unsubscribe what the registry held at removal, NOT the local `bindings` (#859 B-1): a + // reconnect replay during the wait may have rebound this read to new-session handles, and + // the original numbers may now be other tags' live items. If the entry still holds dead + // old-session handles (not yet rebound), release nothing (#861). Only when the entry was + // already gone (teardown cleared the registry) fall back to the original bindings. + IReadOnlyList source = removed ?? bindings; + if (removed is not null && !liveInCurrentSession) source = []; + var liveHandles = source.Where(b => b.ItemHandle > 0).Select(b => b.ItemHandle).ToArray(); + if (liveHandles.Length > 0) { try { @@ -2277,6 +2362,14 @@ public sealed class GalaxyDriver // supervisor Healthy transition, so a 90 s Degraded tripped a re-init under flowing data). StampLiveness(); + // Subscribe-once reads listen here (attributeRef domain), not on a pump instance: a reconnect + // replaces the pump, and a read attached to the dead one never saw the replayed value (#859 B-1). + try { ReadDataChange?.Invoke(this, args); } + catch (Exception ex) + { + _logger.LogWarning(ex, "GalaxyDriver {InstanceId} read listener threw — continuing.", _driverInstanceId); + } + // Probe traffic rides the reserved registry entry and stays in the attributeRef domain — // ScanState attributes are discovered platform attributes, never authored RawPaths. It is // the watcher's alone: the host never subscribed it and must not see it on OnDataChange. diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/Health/PerPlatformProbeWatcher.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/Health/PerPlatformProbeWatcher.cs index d98bae7e0..19448404f 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/Health/PerPlatformProbeWatcher.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/Health/PerPlatformProbeWatcher.cs @@ -46,7 +46,10 @@ public sealed class PerPlatformProbeWatcher : IDisposable // Tracked platform → gw item handle. Item handle 0 means the gw rejected the subscribe; // we keep the entry so SyncPlatformsAsync doesn't try to subscribe it again on every call. - private readonly ConcurrentDictionary _itemHandlesByPlatform = + // Each handle carries the registry session that issued it (#859 I-A): item handles are per gateway + // session and the next session reuses the numbers, so an old-session handle must never be published, + // unsubscribed or purged against the current session. + private readonly ConcurrentDictionary _itemHandlesByPlatform = new(StringComparer.OrdinalIgnoreCase); private readonly Lock _syncLock = new(); // Written under _syncLock by StopAsync; read under it wherever a write-back must not outlive a stop. @@ -99,7 +102,14 @@ public sealed class PerPlatformProbeWatcher : IDisposable internal IReadOnlyList CurrentBindings => BuildBindings(); private IReadOnlyList BuildBindings() => - [.. _itemHandlesByPlatform.Select(kvp => new TagBinding(kvp.Key + ProbeSuffix, kvp.Value))]; + [.. _itemHandlesByPlatform.Select(kvp => new TagBinding(kvp.Key + ProbeSuffix, kvp.Value.ItemHandle))]; + + /// The bindings to publish for : another session's handles bind as 0 (no events). + private IReadOnlyList BuildBindingsFor(int session) => + [.. _itemHandlesByPlatform.Select(kvp => new TagBinding( + kvp.Key + ProbeSuffix, kvp.Value.Session == session ? kvp.Value.ItemHandle : 0))]; + + private int CurrentSession => _registry?.CurrentSession ?? 0; /// /// Mirror the tracked handle set into the driver registry (no-op without one, or once stopped — @@ -107,15 +117,20 @@ public sealed class PerPlatformProbeWatcher : IDisposable /// private void PublishBindings() { - lock (_syncLock) PublishBindingsLocked(); + lock (_syncLock) PublishBindingsLocked(CurrentSession); } - private void PublishBindingsLocked() + /// False when the registry's session is no longer (nothing published). + private bool PublishBindingsLocked(int session) { - if (_registry is null || _disposed) return; - var bindings = BuildBindings(); - if (bindings.Count == 0) _registry.Remove(SubscriptionRegistry.ProbeSubscriptionId); - else _registry.Upsert(SubscriptionRegistry.ProbeSubscriptionId, bindings); + if (_registry is null || _disposed) return true; + if (_itemHandlesByPlatform.IsEmpty) + { + _registry.Remove(SubscriptionRegistry.ProbeSubscriptionId); + return true; + } + // Fenced (#859 I-A): only handles issued by `session` are bound, and only while it is current. + return _registry.TryUpsert(SubscriptionRegistry.ProbeSubscriptionId, BuildBindingsFor(session), session); } /// @@ -136,12 +151,31 @@ public sealed class PerPlatformProbeWatcher : IDisposable } private async Task SubscribeAndRecordAsync(IReadOnlyList platforms, CancellationToken cancellationToken) + { + // #861 / #859 I-A: a reply that straddled a gateway reconnect carries dead handles; re-issue. + for (var attempt = 1; ; attempt++) + { + if (await TrySubscribeAndRecordAsync(platforms, cancellationToken).ConfigureAwait(false)) return; + if (attempt >= MaxSessionAttempts) + { + _logger.LogWarning( + "PerPlatformProbeWatcher probe subscribe straddled {Attempts} gateway reconnects — giving up until the next replay/seed", attempt); + return; + } + } + } + + private const int MaxSessionAttempts = 3; + + /// False when the reply straddled a session change (nothing recorded; retry). + private async Task TrySubscribeAndRecordAsync(IReadOnlyList platforms, CancellationToken cancellationToken) { var probeAddresses = platforms.Select(p => p + ProbeSuffix).ToArray(); // #859: the initial ScanState value can overtake the SubscribeBulk reply; park it until // PublishBindingsLocked registers the handles below (window closes at method exit). using var earlyEventWindow = _registry?.BeginPendingSubscribe(); + var session = CurrentSession; // the session this SubscribeBulk is issued on // PR 6.3 — use the configured bufferedUpdateIntervalMs (defaults to 0 = gw cadence // when the driver hasn't overridden MxAccess.PublishingIntervalMs). Probe ScanState // changes are rare so a coarser interval is usually fine; deployments that need @@ -167,8 +201,15 @@ public sealed class PerPlatformProbeWatcher : IDisposable stopped = _disposed; if (!stopped) { - foreach (var (platform, itemHandle, _) in acquired) _itemHandlesByPlatform[platform] = itemHandle; - PublishBindingsLocked(); + if (_registry is not null && _registry.CurrentSession != session) + { + // Straddled a reconnect: these handles died with the old session and their numbers may + // be reused — neither record, publish nor unsubscribe them. + return false; + } + foreach (var (platform, itemHandle, _) in acquired) + _itemHandlesByPlatform[platform] = new ProbeHandle(itemHandle, session); + if (!PublishBindingsLocked(session)) return false; } } @@ -184,7 +225,7 @@ public sealed class PerPlatformProbeWatcher : IDisposable "PerPlatformProbeWatcher could not release {Count} probe handle(s) acquired after stop", orphans.Length); } } - return; + return true; } foreach (var (platform, itemHandle, error) in acquired) @@ -194,6 +235,7 @@ public sealed class PerPlatformProbeWatcher : IDisposable _logger.LogWarning("PerPlatformProbeWatcher subscribe failed for {Platform}: {Error}", platform, error); } } + return true; } /// @@ -228,12 +270,29 @@ public sealed class PerPlatformProbeWatcher : IDisposable // race on the membership view. List toAdd; List<(string Platform, int ItemHandle)> toRemove; + int removeSession; + bool droppedWithoutRelease; lock (_syncLock) { toAdd = [.. desired.Where(p => !_itemHandlesByPlatform.ContainsKey(p))]; + // Captured before the RPC (#859 I-B); only handles issued by the CURRENT session are released — + // an older session's numbers may now belong to unrelated live items (#859 I-A). + removeSession = CurrentSession; toRemove = [.. _itemHandlesByPlatform - .Where(kvp => !desired.Contains(kvp.Key) && kvp.Value > 0) - .Select(kvp => (kvp.Key, kvp.Value))]; + .Where(kvp => !desired.Contains(kvp.Key) + && kvp.Value.ItemHandle > 0 && kvp.Value.Session == removeSession) + .Select(kvp => (kvp.Key, kvp.Value.ItemHandle))]; + var deadRemovals = _itemHandlesByPlatform + .Where(kvp => !desired.Contains(kvp.Key) && !(kvp.Value.ItemHandle > 0 && kvp.Value.Session == removeSession)) + .Select(kvp => kvp.Key).ToArray(); + // Dropped platforms with no releasable handle (rejected, or an older session's dead number): + // forget them without any unsubscribe. + foreach (var platform in deadRemovals) + { + _itemHandlesByPlatform.TryRemove(platform, out _); + _aggregator.Remove(platform); + } + droppedWithoutRelease = deadRemovals.Length > 0; // Drop removed entries from the membership map up-front so a concurrent // OnProbeValueChanged for them is silently ignored. The unsubscribe RPC @@ -245,6 +304,8 @@ public sealed class PerPlatformProbeWatcher : IDisposable } } + if (droppedWithoutRelease && toRemove.Count == 0) PublishBindings(); // keep the fan-out map in step + if (toRemove.Count > 0) { // Drop the dead handles from the fan-out map before the (slow, fallible) unsubscribe. @@ -264,7 +325,7 @@ public sealed class PerPlatformProbeWatcher : IDisposable { // #859 I-3 interim: no trailing event for a removed probe handle may be promoted onto // a reused handle number. - _registry?.PurgeParked([.. toRemove.Select(t => t.ItemHandle)]); + _registry?.PurgeParked([.. toRemove.Select(t => t.ItemHandle)], removeSession); } } @@ -334,7 +395,9 @@ public sealed class PerPlatformProbeWatcher : IDisposable { if (_disposed) return; _disposed = true; - liveHandles = [.. _itemHandlesByPlatform.Values.Where(h => h > 0)]; + var session = CurrentSession; // never release an older session's (reusable) numbers + liveHandles = [.. _itemHandlesByPlatform.Values + .Where(h => h.ItemHandle > 0 && h.Session == session).Select(h => h.ItemHandle)]; _itemHandlesByPlatform.Clear(); _registry?.Remove(SubscriptionRegistry.ProbeSubscriptionId); } @@ -347,4 +410,9 @@ public sealed class PerPlatformProbeWatcher : IDisposable /// Disposes the probe watcher and unsubscribes all tracked platforms (blocking on the round-trip). public void Dispose() => StopAsync(CancellationToken.None).GetAwaiter().GetResult(); + + /// A probe item handle and the registry session that issued it. + /// The gateway item handle (0 = rejected). + /// The registry session the handle belongs to. + private readonly record struct ProbeHandle(int ItemHandle, int Session); } diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/Runtime/SubscriptionRegistry.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/Runtime/SubscriptionRegistry.cs index 303dde0c5..d7a76d37c 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/Runtime/SubscriptionRegistry.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/Runtime/SubscriptionRegistry.cs @@ -83,7 +83,7 @@ internal sealed class SubscriptionRegistry /// The captured before the SubscribeBulk. /// False (nothing registered) when the session changed in the meantime. public bool TryRegister(long subscriptionId, IReadOnlyList bindings, int expectedSession) => - Bind(subscriptionId, bindings, requireExisting: false, expectedSession) == BindResult.Bound; + Bind(subscriptionId, bindings, requireExisting: false, expectedSession) == BindOutcome.Bound; /// /// Replace an existing subscription's bindings with the item handles a post-reconnect @@ -100,13 +100,36 @@ internal sealed class SubscriptionRegistry /// The subscription identifier. /// The new tag bindings. /// The session the SubscribeBulk was issued on. - /// False when the session changed (nothing rebound); true otherwise, including the unknown-id no-op. - public bool TryRebind(long subscriptionId, IReadOnlyList newBindings, int expectedSession) => - Bind(subscriptionId, newBindings, requireExisting: true, expectedSession) != BindResult.SessionChanged; + /// + /// ; when the subscription was + /// removed meanwhile (the caller must release the handles it just acquired — nothing will + /// ever unsubscribe them, M-a); when the session moved on. + /// + public BindOutcome TryRebind(long subscriptionId, IReadOnlyList newBindings, int expectedSession) => + Bind(subscriptionId, newBindings, requireExisting: true, expectedSession); - private enum BindResult { Bound, UnknownId, SessionChanged } + /// + /// fenced by (the probe watcher, #859 I-A). + /// + /// The subscription identifier. + /// The tag bindings, all issued by . + /// The session the bindings' handles belong to. + /// False when the session changed (nothing published). + public bool TryUpsert(long subscriptionId, IReadOnlyList bindings, int expectedSession) => + Bind(subscriptionId, bindings, requireExisting: false, expectedSession) == BindOutcome.Bound; - private BindResult Bind( + /// Outcome of a fenced bind. + public enum BindOutcome + { + /// Bound. + Bound, + /// The subscription id is no longer tracked (rebind only). + UnknownId, + /// The gateway session changed since the handles were issued. + SessionChanged, + } + + private BindOutcome Bind( long subscriptionId, IReadOnlyList bindings, bool requireExisting, int? expectedSession) { bool promoted; @@ -117,7 +140,7 @@ internal sealed class SubscriptionRegistry // session check is in here too, so it is atomic with StartNewSession's invalidation. lock (_parkGate) { - if (expectedSession is { } session && session != _session) return BindResult.SessionChanged; + if (expectedSession is { } session && session != _session) return BindOutcome.SessionChanged; if (_bySubscriptionId.TryGetValue(subscriptionId, out var oldEntry)) { @@ -128,7 +151,7 @@ internal sealed class SubscriptionRegistry } else if (requireExisting) { - return BindResult.UnknownId; + return BindOutcome.UnknownId; } _bySubscriptionId[subscriptionId] = new SubscriptionEntry(subscriptionId, bindings, _session); @@ -144,7 +167,7 @@ internal sealed class SubscriptionRegistry promoted = PromoteParkedEventsLocked(bindings); } if (promoted) ParkedEventsReady?.Invoke(); - return BindResult.Bound; + return BindOutcome.Bound; } private void RemoveFromReverseMap(SubscriptionEntry entry) @@ -397,7 +420,7 @@ internal sealed class SubscriptionRegistry // head of its handle's queue. var queue = _parkedByItemHandle[head.ItemHandle]; queue.Dequeue(); - head.Gone = true; + head.Release(); if (queue.Count == 0) _parkedByItemHandle.Remove(head.ItemHandle); Volatile.Write(ref _heldCount, _heldCount - 1); } @@ -405,7 +428,7 @@ internal sealed class SubscriptionRegistry private void DiscardAllHeldLocked() { - foreach (var parked in _parkOrder) parked.Gone = true; + foreach (var parked in _parkOrder) parked.Release(); _parkOrder.Clear(); _parkedByItemHandle.Clear(); _ready.Clear(); @@ -423,15 +446,21 @@ internal sealed class SubscriptionRegistry /// worker sequence on the SubscribeBulk reply (mxaccessgw, cross-repo). /// /// The handles whose unsubscribe completed. - public void PurgeParked(IReadOnlyList itemHandles) + /// + /// The session the handles (and the unsubscribe) belong to, captured BEFORE the RPC. A slow or + /// failed unsubscribe can complete after a reconnect; purging then would delete the NEW + /// session's parked initial values for reused numbers, so it is a no-op (#859 I-B). + /// + public void PurgeParked(IReadOnlyList itemHandles, int session) { if (Volatile.Read(ref _heldCount) == 0 || itemHandles.Count == 0) return; lock (_parkGate) { + if (session != _session) return; foreach (var handle in itemHandles) { if (!_parkedByItemHandle.Remove(handle, out var queue)) continue; - foreach (var parked in queue) parked.Gone = true; + foreach (var parked in queue) parked.Release(); Volatile.Write(ref _heldCount, _heldCount - queue.Count); } } @@ -489,8 +518,7 @@ internal sealed class SubscriptionRegistry { foreach (var parked in stranded) { - parked.Gone = true; - _ready.Enqueue(parked.Event); + _ready.Enqueue(parked.Release()); } } replayFirst = TakeReadyLocked(); @@ -548,8 +576,7 @@ internal sealed class SubscriptionRegistry if (!_parkedByItemHandle.Remove(binding.ItemHandle, out var queue)) continue; foreach (var parked in queue) { - parked.Gone = true; // stays counted in _heldCount until replayed - _ready.Enqueue(parked.Event); + _ready.Enqueue(parked.Release()); // stays counted in _heldCount until replayed } promoted = true; } @@ -562,10 +589,19 @@ internal sealed class SubscriptionRegistry public long Stamp { get; } = stamp; /// Gets the event's item handle. public int ItemHandle { get; } = itemHandle; - /// Gets the event. - public MxEvent Event { get; } = ev; - /// Gets or sets whether the event has left the parked set (promoted, purged or trimmed). - public bool Gone { get; set; } + /// Gets the event; null once it has left the parked set (M-c — not pinned by the trim index). + public MxEvent? Event { get; private set; } = ev; + /// Gets whether the event has left the parked set (promoted, purged or trimmed). + public bool Gone => Event is null; + + /// Takes the event out of this entry, so the trim index no longer pins it. + /// The event. + public MxEvent Release() + { + var ev = Event!; + Event = null; + return ev; + } } private sealed class PendingSubscribeScope(SubscriptionRegistry owner, long sequence) : IDisposable @@ -593,6 +629,16 @@ internal sealed class SubscriptionRegistry public IReadOnlyList<(long SubscriptionId, IReadOnlyList Bindings)> SnapshotEntries() => [.. _bySubscriptionId.Values.Select(entry => (entry.SubscriptionId, entry.Bindings))]; + /// + /// limited to subscriptions whose handles were NOT issued by + /// — the reconnect replay's work list. A subscription registered after + /// the reconnect already holds live handles; re-subscribing it would double-advise (#859 I-C). + /// + /// The current session. + /// The stale subscriptions with their bindings. + public IReadOnlyList<(long SubscriptionId, IReadOnlyList Bindings)> SnapshotEntriesNotIn(int session) => + [.. _bySubscriptionId.Values.Where(e => e.Session != session).Select(e => (e.SubscriptionId, e.Bindings))]; + /// /// Per-subscription bookkeeping. is an index /// over keyed by item handle so ResolveSubscribers diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Tests/Runtime/EarlyEventWindowTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Tests/Runtime/EarlyEventWindowTests.cs index b68651989..69989c2ad 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Tests/Runtime/EarlyEventWindowTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Tests/Runtime/EarlyEventWindowTests.cs @@ -210,7 +210,7 @@ public sealed class EarlyEventWindowTests registry.ResolveOrPark(DataChange(7, 1), session, out _, out _); // trailing event of a removed item registry.ResolveOrPark(DataChange(8, 2), session, out _, out _); // positive control - registry.PurgeParked([7]); + registry.PurgeParked([7], session); registry.ParkedEventCount.ShouldBe(1); registry.Register(1, [new TagBinding("Reused", 7), new TagBinding("Other", 8)]); @@ -297,7 +297,7 @@ public sealed class EarlyEventWindowTests using var window = registry.BeginPendingSubscribe(); // New session: Old.B is re-issued as handle 7 — the number Old.A still holds until its own Rebind. registry.ResolveOrPark(DataChange(7, 5), session, out _, out _).ShouldBe(EarlyEventOutcome.Parked); - registry.TryRebind(2, [new TagBinding("Old.B", 7)], session).ShouldBeTrue(); + registry.TryRebind(2, [new TagBinding("Old.B", 7)], session).ShouldBe(SubscriptionRegistry.BindOutcome.Bound); registry.TakeReadyEvents(session).ShouldHaveSingleItem().Value.DoubleValue.ShouldBe(5.0); registry.ResolveSubscribers(7).ShouldBe([(2L, "Old.B")]); @@ -314,7 +314,7 @@ public sealed class EarlyEventWindowTests registry.TryRegister(2, [new TagBinding("Late", 9)], before).ShouldBeFalse(); registry.Contains(2).ShouldBeFalse(); - registry.TryRebind(1, [new TagBinding("Tank.Level", 9)], before).ShouldBeFalse(); + registry.TryRebind(1, [new TagBinding("Tank.Level", 9)], before).ShouldBe(SubscriptionRegistry.BindOutcome.SessionChanged); registry.ResolveSubscribers(9).ShouldBeEmpty(); registry.TryRegister(3, [new TagBinding("Fresh", 9)], registry.CurrentSession).ShouldBeTrue("positive control"); @@ -683,9 +683,10 @@ public sealed class EarlyEventWindowTests subscriber.Script.Enqueue(async (refs, _) => { var tag = refs[0]; - var before = registry.ParkedEventCount; + // The previous iteration's promoted value stays counted until the pump replays it. + await TestWait.UntilAsync(() => registry.ParkedEventCount == 0, "earlier promoted values must be replayed first"); subscriber.Emit(DataChange(newHandle[tag], newValue[tag])); - await TestWait.UntilAsync(() => registry.ParkedEventCount > before, + await TestWait.UntilAsync(() => registry.ParkedEventCount > 0, $"{tag}'s new-session initial value must PARK, not resolve to the old owner of handle {newHandle[tag]}"); return [Ok(tag, newHandle[tag])]; }); @@ -778,4 +779,194 @@ public sealed class EarlyEventWindowTests driver.SubscriptionsForTests.PendingSubscribeCount.ShouldBe(0); never.SetResult([]); // let the abandoned RPC finish cleanly } + + // ===== Third review (B-1, I-A, I-B, I-C, P-1, M-a) ===== + + /// + /// B-1: a read rebound by a reconnect replay while it waited unsubscribes its NEW handle, not the + /// original number (which may by now be another tag's live item). + /// + [Fact] + public async Task Read_rebound_by_a_replay_unsubscribes_the_new_handle_not_the_original() + { + var subscriber = new ScriptedSubscriber(); + using var driver = new GalaxyDriver( + "g", Opts(), hierarchySource: null, dataReader: null, dataWriter: null, subscriber: subscriber); + subscriber.Script.Enqueue((refs, _) => Task.FromResult>([Ok(refs[0], 1)])); + subscriber.Script.Enqueue((refs, _) => + { + subscriber.Emit(DataChange(9, 99.0)); // the replayed initial value (may overtake the reply) + return Task.FromResult>([Ok(refs[0], 9)]); + }); + + var read = driver.ReadAsync(["Tank.R"], TestContext.Current.CancellationToken); + await TestWait.UntilAsync(() => driver.TrackedSubscriptionCountForTests == 1, "the read must be registered and waiting"); + await driver.InvokeReplayForTestAsync(TestContext.Current.CancellationToken); + + var snapshot = (await TestWait.ForAsync(read, "the replayed value must complete the read")).ShouldHaveSingleItem(); + ((double)snapshot.Value!).ShouldBe(99.0); + subscriber.Unsubscribed.ToArray().ShouldBe([9], "the read must release its NEW handle, never the original number 1"); + } + + /// P-1: a reconnect with nothing to replay still replaces the dead pump; a later subscribe gets data. + [Fact] + public async Task Reconnect_with_no_subscriptions_restarts_the_pump() + { + var subscriber = new ScriptedSubscriber(); + using var driver = new GalaxyDriver( + "g", Opts(), hierarchySource: null, dataReader: null, dataWriter: null, subscriber: subscriber); + var delivered = new ConcurrentQueue(); + driver.OnDataChange += (_, e) => delivered.Enqueue(e); + var first = await driver.SubscribeAsync(["Tank.Old"], TimeSpan.Zero, TestContext.Current.CancellationToken); + await driver.UnsubscribeAsync(first, TestContext.Current.CancellationToken); + var deadPump = driver.EventPumpInstanceForTests.ShouldNotBeNull(); + + await driver.InvokeReplayForTestAsync(TestContext.Current.CancellationToken); + + driver.EventPumpInstanceForTests.ShouldNotBeSameAs(deadPump, "the faulted pump must be replaced even with no entries"); + subscriber.Script.Enqueue((refs, _) => Task.FromResult>([Ok(refs[0], 42)])); + await driver.SubscribeAsync(["Tank.New"], TimeSpan.Zero, TestContext.Current.CancellationToken); + subscriber.Emit(DataChange(42, 4.2)); + await TestWait.UntilAsync(() => !delivered.IsEmpty, "a subscribe after the reconnect must receive data"); + delivered.Single().FullReference.ShouldBe("Tank.New"); + } + + /// + /// I-C: the reopen fences the registry around the session swap. An unsubscribe during the + /// reopen/backoff never sends an old number; a subscribe issued after the swap registers normally + /// and the replay does not re-subscribe it (no double advise). + /// + [Fact] + public async Task Subscribe_after_the_session_swap_but_before_replay_is_live_and_not_replayed() + { + var subscriber = new ScriptedSubscriber(); + using var driver = new GalaxyDriver( + "g", Opts(), hierarchySource: null, dataReader: null, dataWriter: null, subscriber: subscriber) + { + RecreateOverrideForTests = _ => Task.CompletedTask, + }; + var delivered = new ConcurrentQueue<(string Ref, double Value)>(); + driver.OnDataChange += (_, e) => delivered.Enqueue((e.FullReference, (double)e.Snapshot.Value!)); + subscriber.Script.Enqueue((refs, _) => Task.FromResult>([Ok(refs[0], 1)])); + subscriber.Script.Enqueue((refs, _) => Task.FromResult>([Ok(refs[0], 3)])); + await driver.SubscribeAsync(["Tank.A"], TimeSpan.Zero, TestContext.Current.CancellationToken); + var c = await driver.SubscribeAsync(["Tank.C"], TimeSpan.Zero, TestContext.Current.CancellationToken); + + await driver.InvokeReopenForTestAsync(TestContext.Current.CancellationToken); // gateway session swapped + + await driver.UnsubscribeAsync(c, TestContext.Current.CancellationToken); + subscriber.Unsubscribed.ShouldNotContain(3, "an old-session number must never reach the new session"); + + subscriber.Script.Enqueue((refs, _) => Task.FromResult>([Ok(refs[0], 50)])); + await driver.SubscribeAsync(["Tank.B"], TimeSpan.Zero, TestContext.Current.CancellationToken); + var callsBeforeReplay = Volatile.Read(ref subscriber.SubscribeCalls); + + subscriber.Script.Enqueue((refs, _) => Task.FromResult>([Ok(refs[0], 51)])); + await driver.InvokeReplayForTestAsync(TestContext.Current.CancellationToken); + + (Volatile.Read(ref subscriber.SubscribeCalls) - callsBeforeReplay).ShouldBe(1, "only Tank.A (old session) is re-subscribed"); + var registry = driver.SubscriptionsForTests; + registry.ResolveSubscribers(50).Single().FullReference.ShouldBe("Tank.B"); + registry.ResolveSubscribers(51).Single().FullReference.ShouldBe("Tank.A"); + subscriber.Emit(DataChange(50, 5.0)); + await TestWait.UntilAsync(() => !delivered.IsEmpty, "the post-swap subscription must stay live across the replay"); + delivered.Single().ShouldBe(("Tank.B", 5.0)); + } + + /// + /// M-a: a subscription removed while the replay's SubscribeBulk for it was in flight gets the + /// freshly advised handles released (nothing else would ever unsubscribe them). + /// + [Fact] + public async Task Replay_releases_handles_for_a_subscription_removed_mid_replay() + { + var subscriber = new ScriptedSubscriber(); + using var driver = new GalaxyDriver( + "g", Opts(), hierarchySource: null, dataReader: null, dataWriter: null, subscriber: subscriber); + subscriber.Script.Enqueue((refs, _) => Task.FromResult>([Ok(refs[0], 1)])); + var handle = await driver.SubscribeAsync(["Tank.Gone"], TimeSpan.Zero, TestContext.Current.CancellationToken); + + subscriber.Script.Enqueue(async (refs, _) => + { + await driver.UnsubscribeAsync(handle, CancellationToken.None); // removed mid-replay + return [Ok(refs[0], 77)]; + }); + await driver.InvokeReplayForTestAsync(TestContext.Current.CancellationToken); + + subscriber.Unsubscribed.ToArray().ShouldBe([77], "the re-advised handle is released; the dead number 1 is not sent"); + driver.SubscriptionsForTests.ResolveSubscribers(77).ShouldBeEmpty(); + } + + /// I-B: a purge that completes after a reconnect does not delete the new session's parked values. + [Fact] + public void A_purge_for_an_older_session_is_a_no_op() + { + var registry = new SubscriptionRegistry(); + var oldSession = registry.CurrentSession; + var newSession = registry.StartNewSession(); + using var window = registry.BeginPendingSubscribe(); + registry.ResolveOrPark(DataChange(7, 1), newSession, out _, out _).ShouldBe(EarlyEventOutcome.Parked); + + registry.PurgeParked([7], oldSession); // a slow old-session unsubscribe finishing late + registry.ParkedEventCount.ShouldBe(1, "the new session's parked initial value must survive"); + + registry.PurgeParked([7], newSession); // positive control + registry.ParkedEventCount.ShouldBe(0); + } + + /// I-A: a probe seed whose reply straddles a reconnect records nothing dead and is re-issued. + [Fact] + public async Task Probe_seed_straddling_a_reconnect_is_reissued_and_never_publishes_the_dead_handle() + { + var registry = new SubscriptionRegistry(); + var subscriber = new ScriptedSubscriber(); + var gate = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + subscriber.Script.Enqueue(async (refs, _) => + { + await gate.Task; + return [Ok(refs[0], 5)]; // old-session handle + }); + subscriber.Script.Enqueue((refs, _) => Task.FromResult>([Ok(refs[0], 6)])); + using var watcher = new PerPlatformProbeWatcher(subscriber, new HostStatusAggregator(), registry); + + var sync = watcher.SyncPlatformsAsync(["P"], TestContext.Current.CancellationToken); + await TestWait.UntilAsync(() => Volatile.Read(ref subscriber.SubscribeCalls) == 1, "the seed's SubscribeBulk must be in flight"); + registry.StartNewSession(); + gate.SetResult(); + await TestWait.ForAsync(sync, "the seed must complete"); + + Volatile.Read(ref subscriber.SubscribeCalls).ShouldBe(2); + watcher.CurrentBindings.Single().ItemHandle.ShouldBe(6); + registry.ResolveSubscribers(5).ShouldBeEmpty(); + registry.ResolveSubscribers(6).Single().FullReference.ShouldBe("P.ScanState"); + subscriber.Unsubscribed.ShouldBeEmpty(); + } + + /// + /// I-A: after a reconnect (before the replay rebinds probes), a seed neither re-publishes the old + /// session's probe numbers nor unsubscribes a removed platform's old number. + /// + [Fact] + public async Task Probe_watcher_never_publishes_or_unsubscribes_old_session_numbers() + { + var registry = new SubscriptionRegistry(); + var subscriber = new ScriptedSubscriber(); + subscriber.Script.Enqueue((refs, _) => Task.FromResult>( + [.. refs.Select((r, i) => Ok(r, i + 1))])); // P → 1, Q → 2 + using var watcher = new PerPlatformProbeWatcher(subscriber, new HostStatusAggregator(), registry); + await watcher.SyncPlatformsAsync(["P", "Q"], TestContext.Current.CancellationToken); + + registry.StartNewSession(); + // The new session reuses number 1 for a user tag. + registry.Register(10, [new TagBinding("User.Tag", 1)]); + + await watcher.SyncPlatformsAsync(["P"], TestContext.Current.CancellationToken); // drops Q + + subscriber.Unsubscribed.ShouldBeEmpty("Q's handle 2 is an old-session number"); + registry.ResolveSubscribers(1).ShouldBe([(10L, "User.Tag")], "P's old-session number must not be re-published"); + + (await watcher.ResubscribeAsync(TestContext.Current.CancellationToken)).ShouldBe(1); // the replay + var fresh = watcher.CurrentBindings.Single().ItemHandle; + registry.ResolveSubscribers(fresh).Single().FullReference.ShouldBe("P.ScanState"); + } } From a8d3790b090a8c963f83e996e4076974781935c5 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Wed, 11 Nov 2026 11:07:59 -0500 Subject: [PATCH 5/5] =?UTF-8?q?fix(galaxy):=20#859=20final=20review=20?= =?UTF-8?q?=E2=80=94=20probe=20replay=20per=20platform=20session,=20teardo?= =?UTF-8?q?wn=20session=20bump?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit BLOCKER: the reconnect replay chose work from the registry entries' sessions, but ONE registry entry holds every probe handle and any probe publish on the new session (a seed in the reopen→replay gap, or the straddle retry) re-stamps it as current — the replay then skipped the probe watcher and the old platforms stayed bound to handle 0 (host status frozen); a publish after the snapshot made it re-subscribe everything (double advise, leaked handle). ReplayAsync now excludes the probe entry from its snapshot and calls the new PerPlatformProbeWatcher.ResubscribeStaleAsync on every reconnect, which re-subscribes exactly the platforms whose ProbeHandle.Session predates the current session. The replay-gap path keeps ResubscribeAsync (all platforms). Also: ClearRegistry (teardown) starts a new registry session, so a late PurgeParked from a teardown-orphaned call cannot reach the next runtime's parked values; the read path's unused pump local is gone (call kept). Tests go through the driver's reopen/replay: the probe old-session-number test is rewritten that way, plus the seed-in-the-gap scenario (only P and Q are re-subscribed, R keeps its new-session handle, all three resolve). Both checked red with the probe replay removed / re-subscribing all platforms. Refs #859, #861. Claude-Session: https://claude.ai/code/session_01GT67AD99zQQqpLCVm3p7xk --- .../GalaxyDriver.cs | 52 +++++++++--- .../Health/PerPlatformProbeWatcher.cs | 24 ++++++ .../Runtime/EarlyEventWindowTests.cs | 80 ++++++++++++++++--- 3 files changed, 136 insertions(+), 20 deletions(-) diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/GalaxyDriver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/GalaxyDriver.cs index 7399b7f92..e5887e7cb 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/GalaxyDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/GalaxyDriver.cs @@ -690,7 +690,13 @@ public sealed class GalaxyDriver private void ClearRegistry() { - lock (_registryGate) _subscriptions.Clear(); + lock (_registryGate) + { + _subscriptions.Clear(); + // A new session too: a late PurgeParked from a call the teardown orphaned (captured the old + // session) must never reach the next runtime's parked values. + _subscriptions.StartNewSession(); + } } /// @@ -1036,27 +1042,49 @@ public sealed class GalaxyDriver // Only subscriptions still holding the previous session's handles: one registered after the // reopen is already live on the recreated session (re-subscribing it would double-advise). - var entries = _subscriptions.SnapshotEntriesNotIn(_subscriptions.CurrentSession); - if (entries.Count == 0) return; + // The probe entry is excluded: ONE registry entry holds every probe handle and any probe publish + // on the new session re-stamps it, so its session says nothing about which platforms are stale. + // The probe watcher tracks the session per handle and is driven separately below. + var session = _subscriptions.CurrentSession; + var entries = _subscriptions.SnapshotEntriesNotIn(session) + .Where(e => e.SubscriptionId != SubscriptionRegistry.ProbeSubscriptionId) + .ToArray(); + var probeWatcher = _probeWatcher; + if (entries.Length == 0 && probeWatcher is null) return; // Honor ReplayOnSessionLost. When operators opt out (false) the per-tag SubscribeBulk fan-out // is skipped, and the recreated gateway session carries NONE of the old advises — those // subscriptions stay deaf (their handles are dead and no longer resolve) until re-subscribed. if (!_options.Reconnect.ReplayOnSessionLost) { - _logger.LogWarning( - "GalaxyDriver {InstanceId} reconnect replay skipped — ReplayOnSessionLost=false; {SubCount} subscription(s) " + - "receive no further data until they are re-subscribed (the recreated gateway session has none of their advises).", - _driverInstanceId, entries.Count); + if (entries.Length > 0) + { + _logger.LogWarning( + "GalaxyDriver {InstanceId} reconnect replay skipped — ReplayOnSessionLost=false; {SubCount} subscription(s) " + + "receive no further data until they are re-subscribed (the recreated gateway session has none of their advises).", + _driverInstanceId, entries.Length); + } return; } - var tagCount = await ResubscribeAllAsync(entries, Volatile.Read(ref _runtimeGeneration), cancellationToken) - .ConfigureAwait(false); + var generation = Volatile.Read(ref _runtimeGeneration); + var tagCount = entries.Length == 0 + ? 0 + : await ResubscribeAllAsync(entries, generation, cancellationToken).ConfigureAwait(false); + + // Probes on EVERY reconnect, whatever the registry entry's session: re-subscribes exactly the + // platforms whose handles predate the current session (a platform a seed already advised on the + // new session is left alone — no double advise), and republishes the fenced bindings. + if (probeWatcher is not null + && Volatile.Read(ref _runtimeGeneration) == generation + && _subscriptions.CurrentSession == session) + { + tagCount += await probeWatcher.ResubscribeStaleAsync(cancellationToken).ConfigureAwait(false); + } _logger.LogInformation( "GalaxyDriver {InstanceId} replay completed — {SubCount} subscriptions, {TagCount} tags re-subscribed", - _driverInstanceId, entries.Count, tagCount); + _driverInstanceId, entries.Length, tagCount); } /// @@ -1555,7 +1583,9 @@ public sealed class GalaxyDriver var generation = Volatile.Read(ref _runtimeGeneration); // see SubscribeAsync var readSubscriber = _subscriber!; - var pump = EnsureEventPumpStarted(); + // A live pump must exist (values arrive through it); the read listens on ReadDataChange, not + // on this instance, so it survives a reconnect's pump restart. + EnsureEventPumpStarted(); var subscriptionId = _subscriptions.NextSubscriptionId(); // Pre-allocate one TaskCompletionSource per full-reference so the OnDataChange diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/Health/PerPlatformProbeWatcher.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/Health/PerPlatformProbeWatcher.cs index 19448404f..fe887b084 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/Health/PerPlatformProbeWatcher.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/Health/PerPlatformProbeWatcher.cs @@ -150,6 +150,30 @@ public sealed class PerPlatformProbeWatcher : IDisposable return platforms.Length; } + /// + /// The reconnect replay: re-issue SubscribeBulk only for platforms whose handle predates the + /// registry's current session, then republish. A platform a seed already advised on the new + /// session is left alone (re-subscribing it would double-advise and leak a handle), and every + /// reconnect calls this regardless of the shared registry entry's session — that entry is + /// re-stamped by any probe publish, so it cannot say which platforms are stale (#859). + /// + /// A cancellation token to cancel the operation. + /// The number of probe references re-subscribed. + public async Task ResubscribeStaleAsync(CancellationToken cancellationToken) + { + ObjectDisposedException.ThrowIf(_disposed, this); + string[] stale; + lock (_syncLock) + { + var session = CurrentSession; + stale = [.. _itemHandlesByPlatform.Where(kvp => kvp.Value.Session != session).Select(kvp => kvp.Key)]; + } + if (stale.Length == 0) return 0; + + await SubscribeAndRecordAsync(stale, cancellationToken).ConfigureAwait(false); + return stale.Length; + } + private async Task SubscribeAndRecordAsync(IReadOnlyList platforms, CancellationToken cancellationToken) { // #861 / #859 I-A: a reply that straddled a gateway reconnect carries dead handles; re-issue. diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Tests/Runtime/EarlyEventWindowTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Tests/Runtime/EarlyEventWindowTests.cs index 69989c2ad..1d0ade328 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Tests/Runtime/EarlyEventWindowTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Tests/Runtime/EarlyEventWindowTests.cs @@ -949,24 +949,86 @@ public sealed class EarlyEventWindowTests [Fact] public async Task Probe_watcher_never_publishes_or_unsubscribes_old_session_numbers() { - var registry = new SubscriptionRegistry(); var subscriber = new ScriptedSubscriber(); + using var driver = await InitializedDriverAsync(subscriber); + var watcher = driver.ProbeWatcherForTests.ShouldNotBeNull(); + var registry = driver.SubscriptionsForTests; subscriber.Script.Enqueue((refs, _) => Task.FromResult>( [.. refs.Select((r, i) => Ok(r, i + 1))])); // P → 1, Q → 2 - using var watcher = new PerPlatformProbeWatcher(subscriber, new HostStatusAggregator(), registry); await watcher.SyncPlatformsAsync(["P", "Q"], TestContext.Current.CancellationToken); - registry.StartNewSession(); + await driver.InvokeReopenForTestAsync(TestContext.Current.CancellationToken); // gateway session swapped // The new session reuses number 1 for a user tag. - registry.Register(10, [new TagBinding("User.Tag", 1)]); + subscriber.Script.Enqueue((refs, _) => Task.FromResult>([Ok(refs[0], 1)])); + await driver.SubscribeAsync(["User.Tag"], TimeSpan.Zero, TestContext.Current.CancellationToken); - await watcher.SyncPlatformsAsync(["P"], TestContext.Current.CancellationToken); // drops Q + await watcher.SyncPlatformsAsync(["P"], TestContext.Current.CancellationToken); // a seed in the gap drops Q subscriber.Unsubscribed.ShouldBeEmpty("Q's handle 2 is an old-session number"); - registry.ResolveSubscribers(1).ShouldBe([(10L, "User.Tag")], "P's old-session number must not be re-published"); + registry.ResolveSubscribers(1).Single().FullReference.ShouldBe("User.Tag", "P's old-session number must not be re-published"); - (await watcher.ResubscribeAsync(TestContext.Current.CancellationToken)).ShouldBe(1); // the replay - var fresh = watcher.CurrentBindings.Single().ItemHandle; - registry.ResolveSubscribers(fresh).Single().FullReference.ShouldBe("P.ScanState"); + var replayed = new ConcurrentQueue(); + subscriber.Script.Enqueue((refs, _) => + { + foreach (var r in refs) replayed.Enqueue(r); + return Task.FromResult>([Ok(refs[0], 7)]); + }); + await driver.InvokeReplayForTestAsync(TestContext.Current.CancellationToken); // the driver's replay + + replayed.ToArray().ShouldBe(["P.ScanState"]); + watcher.CurrentBindings.Single().ItemHandle.ShouldBe(7); + registry.ResolveSubscribers(7).Single().FullReference.ShouldBe("P.ScanState"); + } + + /// + /// Final review blocker: a probe seed in the gap between the session swap and the replay + /// re-stamps the shared probe registry entry as current. The replay must still re-subscribe the + /// platforms whose handles predate the swap (they would otherwise freeze at handle 0) — and only + /// those: the platform the seed just advised on the new session is not advised twice. + /// + [Fact] + public async Task Replay_after_a_seed_in_the_gap_resubscribes_exactly_the_stale_platforms() + { + var subscriber = new ScriptedSubscriber(); + using var driver = await InitializedDriverAsync(subscriber); + var watcher = driver.ProbeWatcherForTests.ShouldNotBeNull(); + var registry = driver.SubscriptionsForTests; + subscriber.Script.Enqueue((refs, _) => Task.FromResult>( + [.. refs.Select((r, i) => Ok(r, i + 1))])); // P → 1, Q → 2 + await watcher.SyncPlatformsAsync(["P", "Q"], TestContext.Current.CancellationToken); + + await driver.InvokeReopenForTestAsync(TestContext.Current.CancellationToken); + subscriber.Script.Enqueue((refs, _) => Task.FromResult>([Ok(refs[0], 20)])); + await watcher.SyncPlatformsAsync(["P", "Q", "R"], TestContext.Current.CancellationToken); // seed adds R on the new session + registry.ResolveSubscribers(20).Single().FullReference.ShouldBe("R.ScanState"); + + var replayed = new ConcurrentQueue(); + subscriber.Script.Enqueue((refs, _) => + { + foreach (var r in refs) replayed.Enqueue(r); + return Task.FromResult>([.. refs.Select((r, i) => Ok(r, 30 + i))]); + }); + await driver.InvokeReplayForTestAsync(TestContext.Current.CancellationToken); + + replayed.OrderBy(r => r).ToArray().ShouldBe(["P.ScanState", "Q.ScanState"], "R was advised on the new session already"); + var bindings = watcher.CurrentBindings.ToDictionary(b => b.FullReference, b => b.ItemHandle); + bindings["R.ScanState"].ShouldBe(20); + bindings["P.ScanState"].ShouldBeGreaterThanOrEqualTo(30); + bindings["Q.ScanState"].ShouldBeGreaterThanOrEqualTo(30); + foreach (var (reference, handle) in bindings) + { + registry.ResolveSubscribers(handle).Single().FullReference.ShouldBe(reference); + } + } + + private static async Task InitializedDriverAsync(ScriptedSubscriber subscriber) + { + var driver = new GalaxyDriver( + "g", Opts(), hierarchySource: null, dataReader: null, dataWriter: null, subscriber: subscriber) + { + RecreateOverrideForTests = _ => Task.CompletedTask, + }; + await driver.InitializeAsync("{}", TestContext.Current.CancellationToken); + return driver; } }