fix(alarms): atomic snapshot+truncation read; direct tests for the flag plumbing (review)
Review found AlarmDispatcher.SnapshotActiveAlarms reading the snapshot and the truncation verdict through two independent lock acquisitions, defended by a comment claiming read-order made a race "widen only, never narrow". That claim was false: a not-truncated -> truncated poll landing between the two reads pairs a stale false with a capped snapshot, which is exactly the false all-clear the feature exists to prevent. It was safe only because AlarmCommandHandler STA-serializes consumer calls — an accident of the call graph, not an invariant. Made the invariant structural instead of documented. IMxAccessAlarmConsumer now exposes ONE accessor, `IReadOnlyList<MxAlarmSnapshotRecord> SnapshotActiveAlarms( out bool truncated)`, which implementations must satisfy from a single acquisition of the lock guarding the retained snapshot — mirroring the write side, where FoldFetch already updates snapshot and verdict together. The separate LastSnapshotTruncated property is gone from every layer, so there is no second read left to pair badly. `out` over a result struct follows the file's established idiom (FoldFetch, ParseSnapshotXml). The same threading applies one level up: IAlarmCommandHandler.QueryActive now carries `out bool snapshotTruncated`, so MxAccessCommandExecutor stamps the reply payload from the value the records were stamped with rather than reading the state a second time. Direct tests for the three hops that were only covered end-to-end: - AlarmDispatcherTests: truncated consumer snapshot stamps FromTruncatedSnapshot on every mapped record, with a complete-snapshot control, plus an assertion that the independent per-record Degraded flag is not dragged along. - AlarmCommandHandlerTests: the verdict delegates through the dispatcher (Theory over both values), and survives a prefix filter that removes every record — the case the per-record flag cannot cover. - AlarmCommandExecutorTests: the reply payload's SnapshotTruncated comes from the handler (Theory over both values), including the zero-record case. The WnWrapAlarmConsumer truncation tests now assert through SnapshotActiveAlarms(out ...) rather than an internal field, because the pairing is the contract. Also: GatewayAlarmMonitor's _snapshotTruncated comment now says "as of the last full reconcile" rather than implying it tracks the current _alarms contents, which live transitions keep moving via ApplyTransition between passes. Detection heuristic still untouched (fetchedRecordCount >= maxAlarmsPerFetch); no @COUNT parsing, per docs/AlarmProbeFindings.md. Still additive gateway metadata about our fetch mechanics, not MXAccess behavior — not a parity deviation, and no event is synthesized. Gateway: NonWindows.slnx builds clean (0 warnings); ~Alarm filter 107/107 pass. Worker + Worker.Tests are windev-gated; the signature change was reviewed by inspection across all 7 IMxAccessAlarmConsumer implementers, all 3 IAlarmCommandHandler implementers, and every call site.
This commit is contained in:
@@ -36,20 +36,6 @@ public interface IMxAccessAlarmConsumer : IDisposable
|
||||
/// </summary>
|
||||
event EventHandler<MxAlarmTransitionEvent>? AlarmTransitionEmitted;
|
||||
|
||||
/// <summary>
|
||||
/// Whether the most recent fetch that reached the retained snapshot came
|
||||
/// back holding the per-fetch cap. While this is <see langword="true"/>
|
||||
/// the snapshot returned by <see cref="SnapshotActiveAlarms"/> is
|
||||
/// authoritative about presence only: the provider may hold actives it
|
||||
/// had no room to report, and the consumer has suspended the
|
||||
/// absence-implies-Clear inference. Not latched — the first sub-cap fetch
|
||||
/// after a run of capped ones clears it, because that fetch is complete
|
||||
/// and the snapshot it produced is again authoritative about absence.
|
||||
/// Consumers with no per-fetch cap (the subtag fallback, which is
|
||||
/// event-driven) always report <see langword="false"/>.
|
||||
/// </summary>
|
||||
bool LastSnapshotTruncated { get; }
|
||||
|
||||
/// <summary>
|
||||
/// Initializes the AVEVA alarm-client connection, registers as a
|
||||
/// consumer, and subscribes to the supplied alarm-provider expression.
|
||||
@@ -111,12 +97,43 @@ public interface IMxAccessAlarmConsumer : IDisposable
|
||||
|
||||
/// <summary>
|
||||
/// Returns the consumer's most recently parsed snapshot of currently
|
||||
/// active alarms. Used by the gateway's QueryActiveAlarms (PR A.7)
|
||||
/// active alarms, together with whether the fetch that produced it hit
|
||||
/// the per-fetch cap. Used by the gateway's QueryActiveAlarms (PR A.7)
|
||||
/// ConditionRefresh path — operator clients call this after reconnect
|
||||
/// to seed local Part 9 state.
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// <para>
|
||||
/// The verdict is an <c>out</c> parameter rather than a separate
|
||||
/// property on purpose, and the reason is a correctness one.
|
||||
/// Implementations must produce both values from a single acquisition
|
||||
/// of whatever lock guards the retained snapshot, mirroring the write
|
||||
/// side (<c>WnWrapAlarmConsumer.FoldFetch</c> updates snapshot and
|
||||
/// verdict together). Two separate reads could straddle a poll that
|
||||
/// flips not-truncated → truncated and pair a stale
|
||||
/// <see langword="false"/> with a capped snapshot — a snapshot that
|
||||
/// reads as complete while missing actives, which is exactly the
|
||||
/// false all-clear this signal exists to prevent. Making the pair
|
||||
/// inseparable in the signature removes the possibility rather than
|
||||
/// relying on callers, or on the STA serializing them.
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// While <paramref name="truncated"/> is <see langword="true"/> the
|
||||
/// returned snapshot is authoritative about presence only: the
|
||||
/// provider may hold actives it had no room to report, and the
|
||||
/// consumer has suspended the absence-implies-Clear inference. Not
|
||||
/// latched — the first sub-cap fetch clears it, because that fetch is
|
||||
/// complete and its snapshot is again authoritative about absence.
|
||||
/// Consumers with no per-fetch cap (the subtag fallback, which is
|
||||
/// advise-driven) always report <see langword="false"/>.
|
||||
/// </para>
|
||||
/// </remarks>
|
||||
/// <param name="truncated">
|
||||
/// Receives whether the fetch behind the returned snapshot hit the
|
||||
/// per-fetch cap.
|
||||
/// </param>
|
||||
/// <returns>The most recently parsed snapshot of currently active alarms.</returns>
|
||||
IReadOnlyList<MxAlarmSnapshotRecord> SnapshotActiveAlarms();
|
||||
IReadOnlyList<MxAlarmSnapshotRecord> SnapshotActiveAlarms(out bool truncated);
|
||||
|
||||
/// <summary>
|
||||
/// Drives a single synchronous poll of the underlying alarm source.
|
||||
|
||||
Reference in New Issue
Block a user