From 89961af6e660da35f0e54466a1f27f21da66ff1e Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 8 Oct 2026 20:18:03 -0400 Subject: [PATCH] =?UTF-8?q?fix(alarms):=20#647=20review=20=E2=80=94=20Rais?= =?UTF-8?q?e=20on=20an=20active=20condition=20is=20a=20re-statement;=20FOC?= =?UTF-8?q?AS=20outage=20quality=20+=20health?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit MAJOR-1: a deploy that changes a driver's config respawns it, and the fresh instance's empty told record re-sent Raise for every still-active alarm, un-acking acknowledged conditions and writing a duplicate "Activated" alerts row. NativeAlarmProjector now treats Raise on an already-active condition as a re-statement (keeps Acked; ForwardNativeAlarm skips the alerts row); only Retrigger (or inactive->active) un-acks. Sources whose upstream ack state is authoritative send Retrigger for an unacked re-statement: OpcUaClient on first sight (no prior told), Galaxy for an unacked active snapshot entry. Runtime E2E drives a FRESH FOCAS driver after a redeploy and asserts the ack survives. MAJOR-2: a failed FOCAS alarm read annotates each subscribed condition BadNoCommunication once per outage (#626 quality-only path) with one Warning (MINOR-6), lifted to Good on recovery; failed alarm reads degrade driver health with LastSuccessfulRead frozen (successful ones advance it), so an alarm-only driver trips the #527 comms-loss watchdog. MINOR-3 CLAUDE.md #630 bullet; MINOR-4 split line; MINOR-5 a non-empty cnc_rdalmmsg2 payload that fits neither stride throws; MINOR-7 value-tag + parsed-address tables are built and swapped, never cleared in place under the retained poll engine. Docs corrected (FOCAS.md, AlarmTracking.md, CLAUDE.md). Claude-Session: https://claude.ai/code/session_01Ef2ozxS7QPanGT8uoATbTe --- CLAUDE.md | 18 ++- docs/AlarmTracking.md | 4 +- docs/drivers/FOCAS.md | 26 ++-- .../FocasAlarmProjection.cs | 118 ++++++++++++++-- .../FocasDriver.cs | 50 +++++-- .../Wire/FocasWireClient.cs | 22 ++- .../Runtime/GatewayGalaxyAlarmFeed.cs | 8 +- .../OpcUaClientDriver.cs | 27 +++- .../Drivers/DriverHostActor.cs | 8 +- .../Drivers/NativeAlarmProjector.cs | 18 ++- .../FocasAlarmProjectionTests.cs | 130 +++++++++++++++++- .../FocasAlarmSelectorTests.cs | 14 ++ .../Runtime/GatewayGalaxyAlarmFeedTests.cs | 4 +- .../OpcUaClientAlarmTests.cs | 19 ++- .../OpcUaClientDriverWireTests.cs | 8 +- .../Drivers/FocasNativeAlarmEndToEndTests.cs | 85 +++++++++++- .../Drivers/NativeAlarmProjectorTests.cs | 23 ++++ 17 files changed, 516 insertions(+), 66 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 695e8123..71fe1cd6 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -1120,12 +1120,20 @@ host logger factory (wired in `DriverFactoryBootstrap`; the FOCAS and TwinCAT fa - **FOCAS alarms (#647):** an `ALARM:` address (`*`, ``, `/`, `/-`, `T`, `OTHER`) + the `alarm` object = ONE condition at the RawPath (either alone is skipped + reported). Overlapping selectors all fire; `OTHER` = what no type selector claims. ⚠️ A failed `cnc_rdalmmsg2` THROWS (it used to return `[]` ⇒ mass - clear + un-acked re-raise); the told record lives on the driver, so reconnect/re-init re-raise nothing. Type names - are the 16i-era table only (30i codes unconfirmed, not invented — use `T` there). Read-only; ack server-side. - `AlarmProjection.Enabled` is a no-op. E2E: `Runtime.Tests/FocasNativeAlarmEndToEndTests`. `docs/drivers/FOCAS.md`. + clear + un-acked re-raise) — and annotates its conditions `BadNoCommunication` once per outage (quality-only) and + degrades driver health with `LastSuccessfulRead` frozen, so an alarm-only driver trips the #527 watchdog. The told + record lives on the driver INSTANCE (reconnect / in-place re-init re-raise nothing); a deploy RESPAWNS the driver, + and its Raise for a still-active alarm is a host-side re-statement (below). Type names are the 16i-era table only + (30i codes unconfirmed, not invented — use `T` there). Read-only; ack server-side. `AlarmProjection.Enabled` + is a no-op. E2E: `Runtime.Tests/FocasNativeAlarmEndToEndTests` (incl. a respawn). `docs/drivers/FOCAS.md`. +- **A Raise on an already-active condition is a RE-STATEMENT (#647 review):** `NativeAlarmProjector` keeps its ack and + `ForwardNativeAlarm` writes no second "Activated" `/alerts` row — every native driver's told-record dies with its + instance, and a deploy respawns the driver. Only `Retrigger` (or inactive → active) un-acks, so a source whose + UPSTREAM is authoritative for the ack sends Retrigger for an unacked re-statement: OpcUaClient on first sight of a + condition (no prior told), Galaxy for an unacked active snapshot entry. - **Re-Initialize without teardown (#630):** FOCAS, AbLegacy, AbCip, TwinCAT, S7 and OpcUaClient `InitializeAsync` - first release the prior device/session state (cancel, ≤ 5 s join, dispose — incl. the AbCip/FOCAS alarm - projections) — the device-state half of `ShutdownAsync` only; the poll engine is kept and the actor re-subscribes + first release the prior device/session state (cancel, ≤ 5 s join, dispose — incl. the AbCip alarm projection; + FOCAS's is PAUSED, its told record kept, #647) — the device-state half of `ShutdownAsync` only; the poll engine is kept and the actor re-subscribes after every successful init. AbCip publishes fresh tag tables by reference swap (a second init used to throw "tag name collision" on `AdoptConfigDuringInit`); S7 swaps `Plc` under `_gate`. - **S7 (#632/#638):** STRING/WSTRING/CHAR/WCHAR and DTL/DATE/TOD/TIME/S5TIME codecs (`S7ValueCodecs`; TOD/TIME/S5TIME diff --git a/docs/AlarmTracking.md b/docs/AlarmTracking.md index f727f6c5..65906ac1 100644 --- a/docs/AlarmTracking.md +++ b/docs/AlarmTracking.md @@ -57,7 +57,9 @@ same condition is wired as an **event notifier of every referencing equipment fo - **Routing is an exact `ConditionId == RawPath` match** (`DriverHostActor.ForwardNativeAlarm`). A driver that decorates the id has every transition dropped as "no alarm condition" — AbCip did (`"{ref}#active"`, fixed under #641), and so did FOCAS (`"{host}#{Type}:{Number}"`, fixed under #647 — one condition per `ALARM:` tag, see - `drivers/FOCAS.md`). A native source must also set `Kind`: an + `drivers/FOCAS.md`). A `Raise` on a condition already projected Active is a **re-statement** (#647 review): the + ack is kept and no second "Activated" row is published — a respawned driver re-reading a still-active alarm sends + exactly that. Only `Retrigger` (or inactive → active) un-acks. A native source must also set `Kind`: an `Unspecified` kind projects no state change. ### Native condition state survives a full rebuild (Gitea #641) diff --git a/docs/drivers/FOCAS.md b/docs/drivers/FOCAS.md index 430e4046..5840ae7a 100644 --- a/docs/drivers/FOCAS.md +++ b/docs/drivers/FOCAS.md @@ -398,14 +398,24 @@ it, then author `ALARM:T`. Number ranges are series-independent. **Delivery and failure.** - **A failed read is not "no alarms".** `IFocasClient.ReadAlarmsAsync` throws on a down session, a non-OK - return code or a wire fault; that device is skipped for the tick and its conditions keep their state. (It - used to answer `[]`, which would have cleared every alarm on a comms blip and re-raised them un-acknowledged - on recovery.) Driver-wide comms loss is the host's `BadNoCommunication` quality annotation (#477) — Active - is kept. -- **Diffed against what the server was TOLD**, recorded only while a handler is attached, and held for the - driver's lifetime: a reconnect (same-set re-subscribe), a re-initialise and a shutdown/init cycle re-raise - nothing, so an acknowledgement survives them. A changed subscribed set (an alarm-bearing deploy) prunes only - the RawPaths that left (#641). + return code, a wire fault, or a non-empty reply that fits neither the 76- nor the 80-byte record layout (an + empty reply is "no alarms"). That device is skipped for the tick and its conditions keep their Active/Acked + state. (It used to answer `[]`, which would have cleared every alarm on a comms blip and re-raised them + un-acknowledged on recovery.) +- **…but an unreachable source says so.** The first failed read of an outage annotates every subscribed condition + on that device `BadNoCommunication` through the #626 quality-only path (no state change, no `/alerts` row) and + logs ONE Warning naming `cnc_rdalmmsg2`; the next successful read lifts it to Good (before any transition it + also finds, e.g. Good then Clear for an alarm that cleared during the outage). Failed alarm reads also reach + driver health: `Degraded` with `LastSuccessfulRead` frozen (only a successful read advances it), so a driver + whose only tags are `ALARM:` tags trips the comms-loss watchdog (#527) and the host's driver-wide #477 + annotation like any other. +- **Diffed against what the server was TOLD**, recorded only while a handler is attached. The record lives on the + driver instance: a reconnect (same-set re-subscribe) and an in-place re-initialise (Force Reconnect, #630) re-raise + nothing. **A deploy that changes the driver's config respawns it** (the planner stops + recreates the driver, a new + instance with an empty record), and its first poll sends `Raise` for every still-active alarm. The host treats a + `Raise` on a condition it already shows Active as a **re-statement**: the acknowledgement is kept and no second + "Activated" `/alerts` row is written (`NativeAlarmProjector`, #647 review). Only `Retrigger` — or inactive → + active — un-acknowledges, which is why a genuinely new CNC alarm in a matched set is sent as `Retrigger`. - **Polling:** one `cnc_rdalmmsg2` per `AlarmProjection.PollInterval` (default 2 s) for each device that owns a **subscribed** alarm tag; devices without one are never polled for alarms. - **Opt-in is the tag.** `AlarmProjection.Enabled` is retained so old configs bind, and has no effect. Before diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/FocasAlarmProjection.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/FocasAlarmProjection.cs index 3b835d14..7fb8e026 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/FocasAlarmProjection.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/FocasAlarmProjection.cs @@ -57,6 +57,22 @@ internal sealed class FocasAlarmProjection : IAsyncDisposable /// Devices whose last read came back at the per-read cap. Guarded by . private readonly HashSet _truncationWarned = new(StringComparer.OrdinalIgnoreCase); + /// Devices whose alarm read is currently failing (one Warning per episode). Guarded by . + private readonly HashSet _failingDevices = new(StringComparer.OrdinalIgnoreCase); + + /// + /// ConditionIds this projection has annotated BadNoCommunication and not yet lifted. Guarded by + /// . Kept apart from : a quality annotation never touches a + /// condition's Active/Acked state. + /// + private readonly HashSet _qualityBadTold = new(StringComparer.Ordinal); + + /// Canonical Opc.Ua.StatusCodes.Good. + private const uint QualityGood = 0x00000000u; + + /// Canonical Opc.Ua.StatusCodes.BadNoCommunication. + private const uint QualityBadNoCommunication = 0x80310000u; + /// Serializes loop start/stop. private readonly SemaphoreSlim _loopGate = new(1, 1); @@ -95,7 +111,11 @@ internal sealed class FocasAlarmProjection : IAsyncDisposable lock (_alarmLock) { // Forget only the conditions that LEFT the set (#641) — the SAME set (a reconnect) prunes nothing. - foreach (var gone in _told.Keys.Where(k => !refs.Contains(k)).ToList()) _told.Remove(gone); Volatile.Write(ref _current, sub); + foreach (var gone in _told.Keys.Where(k => !refs.Contains(k)).ToList()) _told.Remove(gone); + // A (re)subscribe follows a reconnect, where the host re-annotates every condition Good (#477); forget + // which conditions this projection marked Bad, so a still-failing device re-asserts its Bad. + _qualityBadTold.Clear(); + Volatile.Write(ref _current, sub); } await RestartLoopAsync().ConfigureAwait(false); return sub.Handle; @@ -225,10 +245,14 @@ internal sealed class FocasAlarmProjection : IAsyncDisposable catch (OperationCanceledException) when (ct.IsCancellationRequested) { throw; } catch (Exception ex) { - // Not "no alarms": keep every condition on this device exactly as the server was last told. - _logger.LogDebug(ex, "FOCAS alarm read failed for {Host}; its conditions keep their state", host); + // Not "no alarms": keep every condition on this device exactly as the server was last told, but say + // its source is unreachable — and let the failure reach driver health (an alarm-only driver has no + // other read to trip the comms-loss watchdog with). + _driver.NoteAlarmReadFailed(host, ex); + TickFailed(sub, host, tags, ex); continue; } + _driver.NoteAlarmReadSucceeded(); Tick(sub, host, alarms, tags); } } @@ -266,24 +290,92 @@ internal sealed class FocasAlarmProjection : IAsyncDisposable var handlers = _driver.AlarmHandlers; if (handlers is null) return; + if (_failingDevices.Remove(deviceHostAddress)) + _logger.LogInformation("FOCAS alarm read for {Host} recovered", deviceHostAddress); + foreach (var tag in deviceTags.Where(t => sub.References.Contains(t.RawPath))) { + // Lift a Bad this projection put there BEFORE the normal diff, so a recovery that also changed the + // alarm state (cleared during the outage) lands as Good then Clear. + if (_qualityBadTold.Remove(tag.RawPath)) + Deliver(handlers, QualityOnly(tag, sub.Handle, QualityGood, now), tag.RawPath); + var view = FocasAlarmMatcher.Evaluate(tag.Selector, alarms, deviceSelectors, truncated); _told.TryGetValue(tag.RawPath, out var told); var (evt, nowTold) = FocasAlarmConditionMapper.Transition(told, view, tag, sub.Handle, now); _told[tag.RawPath] = nowTold; if (evt is null) continue; + Deliver(handlers, evt, tag.RawPath); + } + } + } - foreach (var registration in handlers.GetInvocationList()) - { - try { ((EventHandler)registration).Invoke(_driver, evt); } - catch (Exception ex) - { - _logger.LogWarning(ex, - "FOCAS driver caught a fault from an alarm subscriber for {ConditionId}; delivery continues.", - tag.RawPath); - } - } + /// + /// A FAILED read of (Gitea #647 review): conditions keep their state, but + /// each subscribed one is annotated BadNoCommunication once per outage through the #626 quality-only + /// path (no Active/Acked change, no /alerts row), and the first failure of the episode logs one + /// Warning. The next successful read lifts the Bad in . + /// + /// The subscription the read was taken for; a superseded one emits nothing. + /// The device whose read failed. + /// The driver's alarm-tag table (RawPath → tag). + /// Why the read failed. + internal void TickFailed( + Subscription sub, string deviceHostAddress, IReadOnlyDictionary tags, Exception error) + { + var now = DateTime.UtcNow; + lock (_alarmLock) + { + if (!ReferenceEquals(Volatile.Read(ref _current), sub)) return; + + if (_failingDevices.Add(deviceHostAddress)) + _logger.LogWarning(error, + "FOCAS alarm read (cnc_rdalmmsg2) failed for {Host}; its alarm conditions keep their state and are " + + "marked BadNoCommunication until a read succeeds", deviceHostAddress); + else + _logger.LogDebug(error, "FOCAS alarm read for {Host} still failing", deviceHostAddress); + + // As for transitions: with nobody listening, record nothing, so the next failure tells it. + var handlers = _driver.AlarmHandlers; + if (handlers is null) return; + + foreach (var tag in tags.Values.Where(t => + string.Equals(t.DeviceHostAddress, deviceHostAddress, StringComparison.OrdinalIgnoreCase) + && sub.References.Contains(t.RawPath))) + { + if (!_qualityBadTold.Add(tag.RawPath)) continue; + Deliver(handlers, QualityOnly(tag, sub.Handle, QualityBadNoCommunication, now), tag.RawPath); + } + } + } + + /// A quality-only annotation (#626): + a status code. + private AlarmEventArgs QualityOnly(FocasAlarmTag tag, IAlarmSubscriptionHandle handle, uint quality, DateTime now) + { + _told.TryGetValue(tag.RawPath, out var told); + return new AlarmEventArgs(handle, + SourceNodeId: tag.DeviceHostAddress, + ConditionId: tag.RawPath, + AlarmType: told?.Winner is { } w ? FocasAlarmConditionMapper.TypeLabel(w.Type) : "CncAlarm", + Message: told?.Message ?? "", + Severity: told?.Severity ?? AlarmSeverity.Low, + SourceTimestampUtc: now, + Kind: AlarmTransitionKind.Unspecified, + OpcUaSeverity: told?.OpcUaSeverity, + QualityStatusCode: quality); + } + + /// Invokes every handler, isolating a faulting one. Caller holds the lock. + private void Deliver(EventHandler handlers, AlarmEventArgs evt, string conditionId) + { + foreach (var registration in handlers.GetInvocationList()) + { + try { ((EventHandler)registration).Invoke(_driver, evt); } + catch (Exception ex) + { + _logger.LogWarning(ex, + "FOCAS driver caught a fault from an alarm subscriber for {ConditionId}; delivery continues.", + conditionId); } } } diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/FocasDriver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/FocasDriver.cs index a97993e9..7245578e 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/FocasDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/FocasDriver.cs @@ -47,7 +47,9 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, private readonly Dictionary _devices = new(StringComparer.OrdinalIgnoreCase); // v3: RawPath → definition, case-sensitive ordinal. Built in InitializeAsync from _options.RawTags // via the pure FocasTagDefinitionFactory mapper (the reference is always a RawPath). - private readonly Dictionary _tagsByRawPath = new(StringComparer.Ordinal); + // Built whole in InitializeAsync and SWAPPED in (Gitea #647 review): the #630-retained poll engine keeps calling + // ReadAsync during a re-initialise, and a table cleared and refilled in place reads BadNodeIdUnknown in between. + private Dictionary _tagsByRawPath = new(StringComparer.Ordinal); // Resolves a read/write fullReference (always a RawPath in v3) to a tag definition by a single // authored-table hit. A miss is a miss — the driver surfaces BadNodeIdUnknown, never a blob-parse. private readonly EquipmentTagRefResolver _resolver; @@ -56,7 +58,7 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, // call. ConcurrentDictionary is required because the poll loop // (ReadAsync) and the host write thread (WriteAsync) both mutate this cache when resolver- // produced equipment tags are encountered for the first time. - private readonly ConcurrentDictionary _parsedAddressesByTagName = + private ConcurrentDictionary _parsedAddressesByTagName = new(StringComparer.OrdinalIgnoreCase); /// /// Whether this driver instance runs its per-device fixed-tree poll loop, and therefore @@ -128,7 +130,7 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, _logger = logger ?? NullLogger.Instance; _alarmProjection = new FocasAlarmProjection(this, _logger); _resolver = new EquipmentTagRefResolver( - r => _tagsByRawPath.TryGetValue(r, out var t) ? t : null); + r => Volatile.Read(ref _tagsByRawPath).TryGetValue(r, out var t) ? t : null); _poll = new PollGroupEngine( reader: ReadAsync, onChange: (handle, tagRef, snapshot) => @@ -234,8 +236,9 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, _skippedTags.BeginRebuild(_options.RawTags.Count); // A re-initialise without teardown (#630) rebuilds the tables from scratch: a tag the new config // removed — or moved between the value and alarm tables (#647) — must not survive in the old one. - _tagsByRawPath.Clear(); - _parsedAddressesByTagName.Clear(); + // Built into fresh tables and swapped in below, never cleared in place under a live reader. + var tagsByRawPath = new Dictionary(StringComparer.Ordinal); + var parsedAddresses = new ConcurrentDictionary(StringComparer.OrdinalIgnoreCase); var alarmTags = new Dictionary(StringComparer.Ordinal); foreach (var entry in _options.RawTags) { @@ -298,7 +301,7 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, entry.RawPath, why, _driverInstanceId); continue; } - _tagsByRawPath[entry.RawPath] = def with + tagsByRawPath[entry.RawPath] = def with { DeviceHostAddress = deviceHost, WriteIdempotent = entry.WriteIdempotent, @@ -306,12 +309,14 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, // Cache the parsed FocasAddress so ReadAsync / WriteAsync don't re-parse on every // hot-path call. The address string has already been validated by FocasAddress.TryParse // above; reusing the parsed record avoids per-tick allocs on subscription pollers. - _parsedAddressesByTagName[entry.RawPath] = parsed; + parsedAddresses[entry.RawPath] = parsed; } // Publish the skip tally only once the whole table is built. An unknown-device throw above // leaves the previous snapshot standing rather than a truncated one that would read as // "we now skip fewer tags". _skippedTags.Commit(); + Volatile.Write(ref _parsedAddressesByTagName, parsedAddresses); + Volatile.Write(ref _tagsByRawPath, tagsByRawPath); Volatile.Write(ref _alarmTags, alarmTags); if (_options.Probe.Enabled) @@ -465,7 +470,7 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, state.DisposeClient(); } _devices.Clear(); - _tagsByRawPath.Clear(); + Volatile.Write(ref _tagsByRawPath, new Dictionary(StringComparer.Ordinal)); Volatile.Write(ref _alarmTags, new Dictionary(StringComparer.Ordinal)); _resolver.Clear(); // no-op in v3 (the resolver reads the live table by closure); kept for symmetry _parsedAddressesByTagName.Clear(); @@ -1693,6 +1698,35 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, /// Gets the alarm poll cadence — one cnc_rdalmmsg2 per alarm-owning device per tick. internal TimeSpan AlarmPollInterval => _options.AlarmProjection.PollInterval; + + /// + /// A successful alarm read is a successful read (Gitea #647 review): it advances + /// exactly as a Good value read does, so an alarm-only driver's + /// health tracks its CNC. Never lifts a (a config-level signal). + /// + internal void NoteAlarmReadSucceeded() + { + var current = Volatile.Read(ref _health); + if (current.State != DriverState.Faulted) + Volatile.Write(ref _health, new DriverHealth(DriverState.Healthy, DateTime.UtcNow, null)); + } + + /// + /// A failed alarm read degrades driver health, PRESERVING — the + /// conjunction the comms-loss watchdog (#527) trips on. Without it a driver whose only tags are ALARM: + /// tags stayed Healthy through a total outage, the watchdog never fired, and the host's #477 + /// BadNoCommunication annotation never ran (Gitea #647 review). + /// + /// The device whose alarm read failed. + /// Why. + internal void NoteAlarmReadFailed(string hostAddress, Exception error) + { + var current = Volatile.Read(ref _health); + if (current.State != DriverState.Faulted) + Volatile.Write(ref _health, new DriverHealth(DriverState.Degraded, current.LastSuccessfulRead, + $"FOCAS alarm read failed for {hostAddress}: {error.Message}")); + } + /// /// Reads one device's active-alarm list. Throws on any failure — an unknown device, a connect that /// fails, a read the CNC refuses — because the projection must never mistake a failed read for an empty diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/Wire/FocasWireClient.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/Wire/FocasWireClient.cs index 097d210c..6c5e36a7 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/Wire/FocasWireClient.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/Wire/FocasWireClient.cs @@ -1101,10 +1101,24 @@ public sealed class FocasWireClient : IAsyncDisposable, IDisposable return new WireProgramName(name, number); } - private static IReadOnlyList ParseAlarms(byte[] payload, short count) - => payload.Length % 76 == 0 - ? ParseVendorAlarms(payload, count) - : ParseLegacyWireAlarms(payload, count); + /// + /// Decode a cnc_rdalmmsg2 payload: the 76-byte vendor stride, else the 80-byte legacy one. An empty + /// payload is "no alarms active". A non-empty payload that fits NEITHER stride is malformed and throws + /// (Gitea #647 review): it used to fall into the legacy parser, decode zero records, and so read as "no + /// alarms" — a mass Clear of every condition on the device, then an un-acknowledged re-raise next poll. + /// + /// The reply payload. + /// The requested record count. + /// The decoded alarms. + /// The payload length matches neither record layout. + internal static IReadOnlyList ParseAlarms(byte[] payload, short count) + { + if (payload.Length % 76 == 0) return ParseVendorAlarms(payload, count); + if (payload.Length % 80 == 0) return ParseLegacyWireAlarms(payload, count); + throw new FocasWireException( + $"cnc_rdalmmsg2 returned a {payload.Length}-byte payload, which fits neither the 76- nor the 80-byte " + + "record layout; refusing to read it as an empty alarm list.", rc: null); + } private static IReadOnlyList ParseVendorAlarms(byte[] payload, short count) { diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/Runtime/GatewayGalaxyAlarmFeed.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/Runtime/GatewayGalaxyAlarmFeed.cs index 0e45269d..c8c6c969 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/Runtime/GatewayGalaxyAlarmFeed.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/Runtime/GatewayGalaxyAlarmFeed.cs @@ -171,14 +171,18 @@ internal sealed class GatewayGalaxyAlarmFeed : IGalaxyAlarmFeed /// Decode one entry of the initial active-alarm snapshot. Each currently-active /// alarm is surfaced as a transition so the OPC UA Part 9 condition layer sees /// the alarm's present state on (re)connect: an unacknowledged active alarm as - /// a , an acknowledged one as a + /// a , an acknowledged one as a /// . + /// Retrigger, not Raise (Gitea #647 review): the host treats a Raise on a condition it already + /// shows Active as a re-statement that KEEPS the acknowledgement. A snapshot re-states upstream state and + /// the upstream's ack state is authoritative, so an unacked entry must un-ack a host condition still + /// holding an ack from before the reconnect. /// private void DispatchSnapshotEntry(ActiveAlarmSnapshot snapshot) { var kind = snapshot.CurrentState switch { - AlarmConditionState.Active => GalaxyAlarmTransitionKind.Raise, + AlarmConditionState.Active => GalaxyAlarmTransitionKind.Retrigger, AlarmConditionState.ActiveAcked => GalaxyAlarmTransitionKind.Acknowledge, AlarmConditionState.Inactive => GalaxyAlarmTransitionKind.Clear, _ => GalaxyAlarmTransitionKind.Unspecified, diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient/OpcUaClientDriver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient/OpcUaClientDriver.cs index 814c1f98..c677db2c 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient/OpcUaClientDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient/OpcUaClientDriver.cs @@ -1982,12 +1982,11 @@ public sealed class OpcUaClientDriver : IDriver, ITagDiscovery, IReadable, IWrit // Fresh prior = the host projector's own default (inactive, acked). var told = new AlarmToldState(target.Active, target.Acked, e.Message, e.OpcUaSeverity); - var prior = _alarmStateByRawPath.TryGetValue(e.ConditionId, out var p) - ? p - : new AlarmToldState(false, true, null, null); + var priorKnown = _alarmStateByRawPath.TryGetValue(e.ConditionId, out var p); + var prior = priorKnown ? p! : new AlarmToldState(false, true, null, null); _alarmStateByRawPath[e.ConditionId] = told; - var kinds = AlarmTransitions((prior.Active, prior.Acked), target); + var kinds = AlarmTransitions((prior.Active, prior.Acked), target, priorKnown); if (kinds.Count == 0) { // Same state: a message/severity change still reaches the host; an identical copy does not. @@ -2106,18 +2105,34 @@ public sealed class OpcUaClientDriver : IDriver, ITagDiscovery, IReadable, IWrit /// /// #619 review — the sequence that moves the host's - /// NativeAlarmProjector (which knows only kinds: Raise ⇒ active+unacked, Acknowledge ⇒ acked, + /// NativeAlarmProjector (which knows only kinds: Raise ⇒ active+unacked — or, on an already-active condition, a re-statement keeping the ack (#647) — Acknowledge ⇒ acked, /// Clear ⇒ inactive keeping acked) from to . A single /// upstream event can need two (an alarm that went active and was acknowledged between two notifications). /// No state change ⇒ empty (the caller decides whether a message/severity change warrants an Unspecified). /// /// The state last told to the host. /// The upstream event's ActiveState/Id + AckedState/Id. + /// + /// False when this driver instance has told the host nothing about the condition yet (first sight after a + /// start or a deploy respawn), so is only the projector's default. Gitea #647 review: + /// the host now treats a Raise on an already-active condition as a re-statement that KEEPS its ack, so an + /// upstream that says unacknowledged must then be told with + /// — the upstream's ack state is authoritative, and the host may be holding an ack from before the respawn. + /// /// The kinds to emit, in order. internal static IReadOnlyList AlarmTransitions( - (bool Active, bool Acked) prior, (bool Active, bool Acked) target) + (bool Active, bool Acked) prior, (bool Active, bool Acked) target, bool priorKnown = true) { if (prior == target) return []; + var kinds = Transitions(prior, target); + return priorKnown || target.Acked + ? kinds + : [.. kinds.Select(k => k == AlarmTransitionKind.Raise ? AlarmTransitionKind.Retrigger : k)]; + } + + private static IReadOnlyList Transitions( + (bool Active, bool Acked) prior, (bool Active, bool Acked) target) + { return target switch { // Active+unacked: a fresh raise, or a re-arm of an active-acked condition. diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Drivers/DriverHostActor.cs b/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Drivers/DriverHostActor.cs index c131024d..cc286208 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Drivers/DriverHostActor.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Drivers/DriverHostActor.cs @@ -1877,12 +1877,14 @@ public sealed class DriverHostActor : ReceiveActor, IWithTimers, IWithUnboundedS foreach (var n in nodeIds) { var nodeId = n.NodeId; - var snapshot = _nativeAlarmProjector.Project(nodeId, msg.Args); + var snapshot = _nativeAlarmProjector.Project(nodeId, msg.Args, out var restatement); _opcUaPublishActor.Tell(new ZB.MOM.WW.OtOpcUa.Runtime.OpcUa.OpcUaPublishActor.AlarmStateUpdate( nodeId, snapshot, msg.Args.SourceTimestampUtc, n.Realm)); - // Only the Primary publishes the cluster-wide alerts transition (see the gate above). - if (!serviceAlertsAsPrimary) continue; + // Only the Primary publishes the cluster-wide alerts transition (see the gate above). A re-statement + // (Gitea #647 review: a Raise on an already-active condition, sent by a driver a deploy respawned) is + // not a transition — the condition was already Active, so a second "Activated" row would be a duplicate. + if (!serviceAlertsAsPrimary || restatement) continue; var meta = _alarmMetaByNodeId.TryGetValue(nodeId, out var m) ? m : (EquipmentId: nodeId, Name: nodeId, AlarmType: "AlarmCondition", HistorizeToAveva: (bool?)null, diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Drivers/NativeAlarmProjector.cs b/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Drivers/NativeAlarmProjector.cs index 9e10f29b..b8392f77 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Drivers/NativeAlarmProjector.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Drivers/NativeAlarmProjector.cs @@ -18,13 +18,29 @@ public sealed class NativeAlarmProjector /// The materialised condition node's id (the projection's state key). /// The native alarm transition. /// The full Part 9 condition snapshot to write to the node. - public AlarmConditionSnapshot Project(string nodeId, AlarmEventArgs e) + public AlarmConditionSnapshot Project(string nodeId, AlarmEventArgs e) => Project(nodeId, e, out _); + + /// + /// Project an alarm transition, reporting whether it was a re-statement (Gitea #647 review): a + /// on a condition already projected Active. That is what a driver + /// whose told-record died with its instance sends after a deploy respawns it — the alarm it re-reads is the + /// one the server already shows, so the acknowledgement is KEPT and the host writes no second "Activated" + /// row. Only (or inactive → active) un-acknowledges; a source + /// that knows the alarm is a genuinely new statement must send Retrigger. + /// + /// The materialised condition node's id (the projection's state key). + /// The native alarm transition. + /// True when re-stated an already-active condition. + /// The full Part 9 condition snapshot to write to the node. + public AlarmConditionSnapshot Project(string nodeId, AlarmEventArgs e, out bool restatement) { var prev = _prior.TryGetValue(nodeId, out var p) ? p : (Active: false, Acked: true); // #626: a source with a finer scale than the four buckets supplies the exact Part 9 severity. var sev = e.OpcUaSeverity is { } exact ? (ushort)Math.Clamp(exact, 1, 1000) : MapSeverity(e.Severity); + restatement = e.Kind == AlarmTransitionKind.Raise && prev.Active; var (active, acked) = e.Kind switch { + AlarmTransitionKind.Raise when prev.Active => (true, prev.Acked), AlarmTransitionKind.Raise or AlarmTransitionKind.Retrigger => (true, false), AlarmTransitionKind.Acknowledge => (prev.Active, true), AlarmTransitionKind.Clear => (false, prev.Acked), diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasAlarmProjectionTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasAlarmProjectionTests.cs index 4859ec08..40d5801f 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasAlarmProjectionTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasAlarmProjectionTests.cs @@ -62,10 +62,14 @@ public sealed class FocasAlarmProjectionTests private static void Tick(FocasDriver drv, params FocasActiveAlarm[] alarms) => drv.AlarmProjection.Tick(drv.AlarmProjection.Current!, Host, alarms, drv.AlarmTags); - private static List Capture(FocasDriver drv) + private static List Capture(FocasDriver drv, bool transitionsOnly = false) { var events = new List(); - drv.OnAlarmEvent += (_, e) => { lock (events) events.Add(e); }; + drv.OnAlarmEvent += (_, e) => + { + if (transitionsOnly && e.IsQualityOnly) return; + lock (events) events.Add(e); + }; return events; } @@ -277,7 +281,8 @@ public sealed class FocasAlarmProjectionTests { var (drv, factory) = NewDriver(Tag(ServoPath, "ALARM:Servo")); await drv.InitializeAsync("{}", CancellationToken.None); - var events = Capture(drv); + // Transitions only: the outage's quality-only BadNoCommunication annotation is pinned separately. + var events = Capture(drv, transitionsOnly: true); await drv.SubscribeAlarmsAsync([ServoPath], CancellationToken.None); await WaitFor(() => factory.Clients.Count > 0); var client = ClientOf(factory); @@ -421,6 +426,125 @@ public sealed class FocasAlarmProjectionTests await drv.ShutdownAsync(CancellationToken.None); } + // ---- Gitea #647 review MAJOR-2: an unreachable alarm source must say so ---------------------------------- + + private const uint BadNoCommunication = 0x80310000u; + + private static void TickFailed(FocasDriver drv) => + drv.AlarmProjection.TickFailed(drv.AlarmProjection.Current!, Host, drv.AlarmTags, new InvalidOperationException("down")); + + /// + /// A failed read marks each subscribed condition BadNoCommunication ONCE per outage, quality-only (no Clear, + /// no state change); recovery lifts it to Good and, with the alarm unchanged, re-raises nothing. + /// + [Fact] + public async Task A_failed_read_annotates_quality_once_and_recovery_lifts_it_without_a_re_raise() + { + var (drv, factory) = NewDriver(Tag(ServoPath, "ALARM:Servo"), Tag(AnyPath, "ALARM:*")); + await InitAndSubscribeForTicksAsync(drv, factory, ServoPath, AnyPath); + var events = Capture(drv); + Tick(drv, Servo401()); + events.Clear(); + + TickFailed(drv); + TickFailed(drv); + events.Count.ShouldBe(2, "one Bad per condition for the whole outage"); + events.ShouldAllBe(e => e.IsQualityOnly && e.QualityStatusCode == BadNoCommunication); + events.Select(e => e.ConditionId).Order().ToArray().ShouldBe(new[] { AnyPath, ServoPath }); + + events.Clear(); + Tick(drv, Servo401()); + events.Count.ShouldBe(2); + events.ShouldAllBe(e => e.IsQualityOnly && e.QualityStatusCode == 0u, "recovery lifts the Bad and says nothing else"); + } + + [Fact] + public async Task A_recovery_that_finds_the_alarm_cleared_lifts_quality_then_clears() + { + var (drv, factory) = NewDriver(Tag(ServoPath, "ALARM:Servo")); + await InitAndSubscribeForTicksAsync(drv, factory, ServoPath); + var events = Capture(drv); + Tick(drv, Servo401()); + TickFailed(drv); + events.Clear(); + + Tick(drv); + + events.Select(e => (e.Kind, e.QualityStatusCode)).ToArray().ShouldBe(new (AlarmTransitionKind, uint?)[] + { + (AlarmTransitionKind.Unspecified, 0u), + (AlarmTransitionKind.Clear, null), + }); + } + + [Fact] + public async Task A_persistently_failing_alarm_read_warns_once_per_outage() + { + var logger = new ListLogger(); + var factory = new FakeFocasClientFactory(); + var drv = new FocasDriver(new FocasDriverOptions + { + Devices = [new FocasDeviceOptions(Host, DeviceName: "m1")], + RawTags = [Tag(ServoPath, "ALARM:Servo")], + Probe = new FocasProbeOptions { Enabled = false }, + }, "drv-1", factory, logger); + await InitAndSubscribeForTicksAsync(drv, factory, ServoPath); + Capture(drv); + var before = logger.Warnings(); + + TickFailed(drv); + TickFailed(drv); + TickFailed(drv); + logger.Warnings().ShouldBe(before + 1); + + Tick(drv); // recovery ends the episode… + TickFailed(drv); // …so the next outage warns again + logger.Warnings().ShouldBe(before + 2); + } + + /// + /// An alarm-only driver must be able to trip the comms-loss watchdog (#527): sustained alarm-read failure + /// goes non-Healthy with LastSuccessfulRead FROZEN; a successful alarm read advances it. + /// + [Fact] + public async Task Sustained_alarm_read_failure_degrades_health_with_LastSuccessfulRead_frozen() + { + var (drv, factory) = NewDriver(Tag(ServoPath, "ALARM:Servo")); + factory.Customise = () => new FakeFocasClient { ThrowOnReadAlarms = true }; + await drv.InitializeAsync("{}", CancellationToken.None); + var atInit = drv.GetHealth().LastSuccessfulRead; + await drv.SubscribeAlarmsAsync([ServoPath], CancellationToken.None); + await WaitFor(() => factory.Clients.Count > 0 && factory.Clients[0].AlarmReadCount >= 3); + + var failing = drv.GetHealth(); + failing.State.ShouldBe(DriverState.Degraded); + failing.LastSuccessfulRead.ShouldBe(atInit, "a failed read never advances LastSuccessfulRead"); + + factory.Clients[0].ThrowOnReadAlarms = false; + await WaitFor(() => drv.GetHealth().State == DriverState.Healthy); + drv.GetHealth().LastSuccessfulRead!.Value.ShouldBeGreaterThan(atInit!.Value); + await drv.ShutdownAsync(CancellationToken.None); + } + + private sealed class ListLogger : Microsoft.Extensions.Logging.ILogger + { + private int _warnings; + + public int Warnings() => Volatile.Read(ref _warnings); + + public IDisposable? BeginScope(TState state) where TState : notnull => null; + + public bool IsEnabled(Microsoft.Extensions.Logging.LogLevel logLevel) => true; + + public void Log(Microsoft.Extensions.Logging.LogLevel logLevel, Microsoft.Extensions.Logging.EventId eventId, + TState state, Exception? exception, Func formatter) + { + if (logLevel == Microsoft.Extensions.Logging.LogLevel.Warning + && formatter(state, exception).Contains("cnc_rdalmmsg2", StringComparison.Ordinal)) + Interlocked.Increment(ref _warnings); + } + } + private static int Count(List events) { lock (events) return events.Count; diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasAlarmSelectorTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasAlarmSelectorTests.cs index c22db67b..7a3ac0fa 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasAlarmSelectorTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasAlarmSelectorTests.cs @@ -133,6 +133,20 @@ public sealed class FocasAlarmSelectorTests c.Alarm.ShouldBeNull(); } + /// + /// Gitea #647 review MINOR-5 — a non-empty alarm payload that fits neither record stride is malformed, not + /// "no alarms" (which would mass-clear every condition on the device). Empty stays empty. + /// + [Fact] + public void A_malformed_alarm_payload_throws_while_an_empty_one_is_no_alarms() + { + FocasWireClient.ParseAlarms([], 32).ShouldBeEmpty(); + FocasWireClient.ParseAlarms(new byte[76], 32).Count.ShouldBe(1); + FocasWireClient.ParseAlarms(new byte[160], 32).Count.ShouldBe(2); + Should.Throw(() => FocasWireClient.ParseAlarms(new byte[77], 32)); + Should.Throw(() => FocasWireClient.ParseAlarms(new byte[12], 32)); + } + /// /// Gitea #647 defect 4 — a read that FAILED must not look like "no alarms": the projection would diff it /// to a mass clear and re-raise everything un-acknowledged on recovery. A disconnected wire client throws. diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Tests/Runtime/GatewayGalaxyAlarmFeedTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Tests/Runtime/GatewayGalaxyAlarmFeedTests.cs index e0482d60..090f7228 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Tests/Runtime/GatewayGalaxyAlarmFeedTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Tests/Runtime/GatewayGalaxyAlarmFeedTests.cs @@ -58,9 +58,9 @@ public sealed class GatewayGalaxyAlarmFeedTests observed.Count.ShouldBe(3); - // Active snapshot entry → Raise. + // Active (unacked) snapshot entry → Retrigger (#647 review: forces the upstream's unack on the host). observed[0].AlarmFullReference.ShouldBe("Tank01.Level.HiHi"); - observed[0].TransitionKind.ShouldBe(GalaxyAlarmTransitionKind.Raise); + observed[0].TransitionKind.ShouldBe(GalaxyAlarmTransitionKind.Retrigger); observed[0].SeverityBucket.ShouldBe(AlarmSeverity.Critical); observed[0].RawMxAccessSeverity.ShouldBe(750); diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Tests/OpcUaClientAlarmTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Tests/OpcUaClientAlarmTests.cs index 234e1c8c..f87b90f9 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Tests/OpcUaClientAlarmTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Tests/OpcUaClientAlarmTests.cs @@ -232,6 +232,21 @@ public sealed class OpcUaClientAlarmTests OpcUaClientDriver.AlarmTransitions((priorActive, priorAcked), (active, acked)).ShouldBe(expected); } + /// + /// Gitea #647 review — with no prior this driver told the host (first sight after start or a deploy respawn), + /// an unacknowledged upstream target goes out as Retrigger, never Raise: the host keeps an existing ack across + /// a Raise on an already-active condition, so only a Retrigger makes the host match the upstream's unack. + /// An acknowledged target keeps its Raise (a re-statement there is exactly right). + /// + [Theory] + [InlineData(true, false, new[] { AlarmTransitionKind.Retrigger })] + [InlineData(false, false, new[] { AlarmTransitionKind.Retrigger, AlarmTransitionKind.Clear })] + [InlineData(true, true, new[] { AlarmTransitionKind.Raise, AlarmTransitionKind.Acknowledge })] + public void An_unknown_prior_tells_an_unacked_upstream_with_Retrigger(bool active, bool acked, AlarmTransitionKind[] expected) + { + OpcUaClientDriver.AlarmTransitions((false, true), (active, acked), priorKnown: false).ShouldBe(expected); + } + // ---- #619 re-review: the transition table must reach EVERY target from EVERY prior. ---- /// The host projector's semantics, restated: the only thing the driver's kinds act upon. @@ -357,7 +372,9 @@ public sealed class OpcUaClientAlarmTests drv.RouteAlarmEvent(ConditionFields(true, false), TripIndex, handle); drv.RouteAlarmEvent(ConditionFields(true, true), TripIndex, handle); - seen.Select(e => e.Kind).ShouldBe([AlarmTransitionKind.Raise, AlarmTransitionKind.Acknowledge]); + // First sight of an UNACKED upstream condition is a Retrigger (#647 review: the host keeps an ack across a + // Raise on an already-active condition, and only the upstream knows it is unacknowledged). + seen.Select(e => e.Kind).ShouldBe([AlarmTransitionKind.Retrigger, AlarmTransitionKind.Acknowledge]); drv.RouteAlarmEvent(ConditionFields(true, true), TripIndex, handle); // the refresh copy seen.Count.ShouldBe(2); diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.IntegrationTests/OpcUaClientDriverWireTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.IntegrationTests/OpcUaClientDriverWireTests.cs index 23f8adeb..ccc1526f 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.IntegrationTests/OpcUaClientDriverWireTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.IntegrationTests/OpcUaClientDriverWireTests.cs @@ -257,7 +257,7 @@ public sealed class OpcUaClientDriverWireTests /// /// #619 — ConditionRefresh over the wire: a condition already active+unacked BEFORE the driver subscribes is - /// reported (Raise, keyed by the RawPath) right after subscribe, not only on its next transition. A second + /// reported (Retrigger since #647, keyed by the RawPath) right after subscribe, not only on its next transition. A second /// subscribe of the same set refreshes again and must emit nothing (no re-raise); a positive control then /// proves the feed is still live by acknowledging upstream and seeing exactly the Acknowledge arrive. /// @@ -293,9 +293,11 @@ public sealed class OpcUaClientDriverWireTests AlarmEventArgs[] Seen() { lock (seen) return [.. seen]; } var first = await drv.SubscribeAlarmsAsync([AlarmRef], ct); - await RecordedEvents.WaitUntilAsync(() => Seen().Any(e => e.Kind == AlarmTransitionKind.Raise)); + // First sight of an active+UNACKED upstream condition is a Retrigger (#647 review): the host keeps an ack + // across a Raise on an already-active condition, so only a Retrigger forces the upstream's unack. + await RecordedEvents.WaitUntilAsync(() => Seen().Any(e => e.Kind == AlarmTransitionKind.Retrigger)); - var raise = Seen().Single(e => e.Kind == AlarmTransitionKind.Raise); + var raise = Seen().Single(e => e.Kind == AlarmTransitionKind.Retrigger); raise.ConditionId.ShouldBe(AlarmRef); raise.Message.ShouldBe("already hot"); raise.OpcUaSeverity.ShouldBe(800); diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Drivers/FocasNativeAlarmEndToEndTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Drivers/FocasNativeAlarmEndToEndTests.cs index e415f019..0e3e20af 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Drivers/FocasNativeAlarmEndToEndTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Drivers/FocasNativeAlarmEndToEndTests.cs @@ -1,10 +1,12 @@ using System.Text.Json; using Akka.Actor; +using Akka.Cluster.Tools.PublishSubscribe; using Akka.TestKit; using Microsoft.EntityFrameworkCore; using Shouldly; using Xunit; using ZB.MOM.WW.OtOpcUa.Commons.Engines; +using ZB.MOM.WW.OtOpcUa.Commons.Messages.Alerts; using ZB.MOM.WW.OtOpcUa.Commons.Messages.Deploy; using ZB.MOM.WW.OtOpcUa.Commons.OpcUa; using ZB.MOM.WW.OtOpcUa.Commons.Types; @@ -15,6 +17,7 @@ using ZB.MOM.WW.OtOpcUa.Core.Abstractions; using ZB.MOM.WW.OtOpcUa.Driver.FOCAS; using ZB.MOM.WW.OtOpcUa.Runtime.Drivers; using ZB.MOM.WW.OtOpcUa.Runtime.OpcUa; +using ZB.MOM.WW.OtOpcUa.Runtime.ScriptedAlarms; using ZB.MOM.WW.OtOpcUa.Runtime.Tests.Harness; namespace ZB.MOM.WW.OtOpcUa.Runtime.Tests.Drivers; @@ -44,8 +47,8 @@ public sealed class FocasNativeAlarmEndToEndTests : RuntimeActorTestBase { var db = NewInMemoryDbFactory(); var blob = BuildArtifact(); - var deploymentId = Seed(db, blob); - var (host, publish) = SpawnHostAndApply(db, deploymentId); + var deploymentId = Seed(db, blob, Rev); + var (host, publish, _) = SpawnHostAndApply(db, deploymentId); // The raw tags exactly as the driver receives them: the artifact's merged DriverConfig RawTags array. var rawTags = RawTagsFor(blob, DriverId); @@ -89,6 +92,76 @@ public sealed class FocasNativeAlarmEndToEndTests : RuntimeActorTestBase } } + /// + /// Gitea #647 review (MAJOR-1) — a deploy that changes ANY of the driver's config respawns it, and the fresh + /// instance has no record of what the server was told. Its first poll re-reads the still-active alarm and + /// sends Raise; the host must treat that as a re-statement: the operator's acknowledgement survives and no + /// second "Activated" alerts row is written. Driven through the real host with a FRESH driver instance. + /// + [Fact] + public async Task A_respawned_driver_re_reading_an_acknowledged_active_alarm_keeps_the_ack() + { + var db = NewInMemoryDbFactory(); + var blob = BuildArtifact(); + var alerts = CreateTestProbe(); + DistributedPubSub.Get(Sys).Mediator.Tell(new Subscribe(ScriptedAlarmHostActor.AlertsTopic, alerts.Ref), alerts.Ref); + alerts.ExpectMsg(TimeSpan.FromSeconds(5)); + var (host, publish, coordinator) = SpawnHostAndApply(db, Seed(db, blob, Rev)); + var rawTags = RawTagsFor(blob, DriverId); + var cnc = new AlarmOnlyFocasClient(); + cnc.SetAlarms(new FocasActiveAlarm(401, FocasAlarmType.Servo, 1, "SERVO V-READY OFF")); + + var first = await StartDriverAsync(rawTags, cnc, host); + try + { + publish.FishForMessage(_ => true, TimeSpan.FromSeconds(5)) + .State.Acknowledged.ShouldBeFalse(); + alerts.ExpectMsg(TimeSpan.FromSeconds(5)); + } + finally + { + await first.ShutdownAsync(CancellationToken.None); + } + + // The operator acknowledges, then a deploy changes the driver's config => stop + respawn. + host.Tell(new DriverHostActor.RouteNativeAlarmAck(ExpectedRawPath, "seen", "op")); + var revB = RevisionHash.Parse(new string('e', 64)); + host.Tell(new DispatchDeployment(Seed(db, blob, revB), revB, CorrelationId.NewId())); + coordinator.FishForMessage(a => a.Outcome == ApplyAckOutcome.Applied, TimeSpan.FromSeconds(5)); + publish.FishForMessage(_ => true, TimeSpan.FromSeconds(5)); + + var respawned = await StartDriverAsync(rawTags, cnc, host); + try + { + var restated = publish.FishForMessage(_ => true, TimeSpan.FromSeconds(5)); + restated.AlarmNodeId.ShouldBe(ExpectedRawPath); + restated.State.Active.ShouldBeTrue(); + restated.State.Acknowledged.ShouldBeTrue("a respawned driver's re-statement must not un-acknowledge"); + alerts.ExpectNoMsg(TimeSpan.FromMilliseconds(500)); + } + finally + { + await respawned.ShutdownAsync(CancellationToken.None); + } + } + + private static async Task StartDriverAsync(List rawTags, IFocasClient cnc, IActorRef host) + { + var driver = new FocasDriver(new FocasDriverOptions + { + Devices = [new FocasDeviceOptions(Host, DeviceName: "m1")], + RawTags = rawTags, + Probe = new FocasProbeOptions { Enabled = false }, + AlarmProjection = new FocasAlarmProjectionOptions { PollInterval = TimeSpan.FromMilliseconds(20) }, + }, DriverId, new SingleClientFactory(cnc)); + driver.OnAlarmEvent += (_, e) => host.Tell(new DriverInstanceActor.AttributeAlarmPublished(DriverId, e)); + await driver.InitializeAsync("{}", CancellationToken.None); +#pragma warning disable OTOPCUA0001 // the routing is under test, not resilience wrapping + await driver.SubscribeAlarmsAsync([.. rawTags.Select(t => t.RawPath)], CancellationToken.None); +#pragma warning restore OTOPCUA0001 + return driver; + } + /// The seeded artifact's FOCAS driver raw tags, decoded through the production artifact reader. private static List RawTagsFor(byte[] blob, string driverInstanceId) { @@ -133,14 +206,14 @@ public sealed class FocasNativeAlarmEndToEndTests : RuntimeActorTestBase }, }); - private static DeploymentId Seed(IDbContextFactory db, byte[] blob) + private static DeploymentId Seed(IDbContextFactory db, byte[] blob, RevisionHash rev) { var id = DeploymentId.NewId(); using var ctx = db.CreateDbContext(); ctx.Deployments.Add(new Deployment { DeploymentId = id.Value, - RevisionHash = Rev.Value, + RevisionHash = rev.Value, Status = DeploymentStatus.Sealed, CreatedBy = "test", SealedAtUtc = DateTime.UtcNow, @@ -150,7 +223,7 @@ public sealed class FocasNativeAlarmEndToEndTests : RuntimeActorTestBase return id; } - private (IActorRef Actor, TestProbe Publish) SpawnHostAndApply( + private (IActorRef Actor, TestProbe Publish, TestProbe Coordinator) SpawnHostAndApply( IDbContextFactory db, DeploymentId deploymentId) { var coordinator = CreateTestProbe(); @@ -166,7 +239,7 @@ public sealed class FocasNativeAlarmEndToEndTests : RuntimeActorTestBase actor.Tell(new DispatchDeployment(deploymentId, Rev, CorrelationId.NewId())); coordinator.ExpectMsg(TimeSpan.FromSeconds(5)).Outcome.ShouldBe(ApplyAckOutcome.Applied); publish.ExpectMsg(TimeSpan.FromSeconds(5)); - return (actor, publish); + return (actor, publish, coordinator); } private sealed class SingleClientFactory(IFocasClient client) : IFocasClientFactory diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Drivers/NativeAlarmProjectorTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Drivers/NativeAlarmProjectorTests.cs index e3a2c876..12b878f8 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Drivers/NativeAlarmProjectorTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Drivers/NativeAlarmProjectorTests.cs @@ -224,6 +224,29 @@ public class NativeAlarmProjectorTests snap.StatusCode.ShouldBe(OpcUaStatus.BadNoCommunication); } + /// + /// Gitea #647 review — a Raise on a condition that is ALREADY active is a re-statement (a respawned driver + /// re-reading a still-active alarm it never told the server about): it keeps the acknowledgement and is + /// flagged so the host writes no duplicate "Activated" row. Only a Retrigger (or inactive → active) un-acks. + /// + [Fact] + public void A_Raise_on_an_already_active_condition_is_a_restatement_that_keeps_the_ack() + { + var sut = new NativeAlarmProjector(); + sut.Project("n1", Evt(AlarmTransitionKind.Raise), out var first).Active.ShouldBeTrue(); + first.ShouldBeFalse("inactive → active is a real transition"); + sut.MarkAcknowledged("n1"); + + var restated = sut.Project("n1", Evt(AlarmTransitionKind.Raise, msg: "again"), out var restatement); + restatement.ShouldBeTrue(); + restated.Active.ShouldBeTrue(); + restated.Acknowledged.ShouldBeTrue("a re-statement must not un-acknowledge"); + + var retriggered = sut.Project("n1", Evt(AlarmTransitionKind.Retrigger), out var retrigger); + retrigger.ShouldBeFalse(); + retriggered.Acknowledged.ShouldBeFalse("a Retrigger is a new statement and un-acks"); + } + private static AlarmEventArgs Evt( AlarmTransitionKind kind, AlarmSeverity sev = AlarmSeverity.High,