From 3e7a3d7e31974fab9b5744ab6729b64bdac3d5b3 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Sat, 16 May 2026 20:58:03 -0400 Subject: [PATCH] =?UTF-8?q?fix(commons):=20resolve=20Commons-001..004=20?= =?UTF-8?q?=E2=80=94=20stale-fire=20race,=20JsonDocument=20lifetime,=20Get?= =?UTF-8?q?Nullable=20strictness,=20registry=20symmetry?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- code-reviews/Commons/findings.md | 52 ++++++-- .../Management/ManagementCommandRegistry.cs | 43 ++++++- .../Types/DynamicJsonElement.cs | 11 +- .../Types/ScriptParameters.cs | 20 +-- .../Types/StaleTagMonitor.cs | 87 +++++++++++-- .../ManagementCommandRegistryTests.cs | 78 ++++++++++++ .../Types/DynamicJsonElementTests.cs | 103 +++++++++++++++ .../Types/ScriptParametersTests.cs | 25 +++- .../Types/StaleTagMonitorRaceTests.cs | 119 ++++++++++++++++++ 9 files changed, 501 insertions(+), 37 deletions(-) create mode 100644 tests/ScadaLink.Commons.Tests/Messages/ManagementCommandRegistryTests.cs create mode 100644 tests/ScadaLink.Commons.Tests/Types/DynamicJsonElementTests.cs create mode 100644 tests/ScadaLink.Commons.Tests/Types/StaleTagMonitorRaceTests.cs diff --git a/code-reviews/Commons/findings.md b/code-reviews/Commons/findings.md index f6e6f56..ad5e1f0 100644 --- a/code-reviews/Commons/findings.md +++ b/code-reviews/Commons/findings.md @@ -8,7 +8,7 @@ | Last reviewed | 2026-05-16 | | Reviewer | claude-agent | | Commit reviewed | `9c60592` | -| Open findings | 12 | +| Open findings | 8 | ## Summary @@ -55,7 +55,7 @@ wire command. |--|--| | Severity | Medium | | Category | Concurrency & thread safety | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.Commons/Types/StaleTagMonitor.cs:42-46`, `:62-67` | **Description** @@ -81,7 +81,14 @@ only then reschedule the timer. **Resolution** -_Unresolved._ +Resolved 2026-05-16 (commit pending) — confirmed the race against the source. Replaced +the `volatile bool` guard with a lock-protected monotonic generation token: `Start`, +`OnValueReceived` and `Stop` each bump the generation under a gate, and the timer +callback only raises `Stale` if its scheduled generation still matches. `OnValueReceived` +now recreates the timer (rather than `Change`-ing it) so the rescheduled callback carries +the new token. A superseded or stopped period can no longer emit a spurious staleness +signal. Regression tests added in `StaleTagMonitorRaceTests` (deterministic via an +internal `CallbackEnteredHook` test seam). ### Commons-002 — `DynamicJsonElement` retains a `JsonElement` whose `JsonDocument` lifetime it does not own @@ -89,7 +96,7 @@ _Unresolved._ |--|--| | Severity | Medium | | Category | Performance & resource management | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.Commons/Types/DynamicJsonElement.cs:10-17` | **Description** @@ -113,7 +120,13 @@ regardless. **Resolution** -_Unresolved._ +Resolved 2026-05-16 (commit pending) — confirmed the hazard: `ExternalCallResult.Response` +constructs the wrapper from `JsonDocument.Parse(...).RootElement` with no reference kept +to the document, so deferred script-time access could fault. Fixed at the root by cloning +the element with `JsonElement.Clone()` in the `DynamicJsonElement` constructor, detaching +it from the owning document; the public constructor signature is unchanged. Added a +remarks block documenting the lifetime contract. Regression tests added in +`DynamicJsonElementTests` (access after the source document is disposed / GC-collected). ### Commons-003 — `ScriptParameters.GetNullable` silently swallows conversion failures @@ -121,7 +134,7 @@ _Unresolved._ |--|--| | Severity | Medium | | Category | Error handling & resilience | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.Commons/Types/ScriptParameters.cs:72-86` | **Description** @@ -146,7 +159,15 @@ element handling. If the swallowing must stay for compatibility, at minimum surf **Resolution** -_Unresolved._ +Resolved 2026-05-16 (commit pending) — confirmed the silent-swallow path against the +source. Removed the `catch (ScriptParameterException)` block in `GetNullable`: an +absent or explicitly-null parameter still returns `null`, but a parameter that is +*present but holds an unconvertible value* now throws `ScriptParameterException` with a +descriptive message, consistent with `Get()` and the array/list element paths. The +`Get` XML doc was corrected accordingly. This is a deliberate behavioral change toward +correctness — the previous behavior masked caller/script bugs; the type-level public +contract is unchanged. Regression tests added in `ScriptParametersTests` +(`Get_NullableInt_PresentButUnparsable_Throws` and siblings). ### Commons-004 — `ManagementCommandRegistry` name mapping is asymmetric and namespace-scoped @@ -154,7 +175,7 @@ _Unresolved._ |--|--| | Severity | Medium | | Category | Code organization & conventions | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.Commons/Messages/Management/ManagementCommandRegistry.cs:14-35` | **Description** @@ -187,7 +208,20 @@ scope, and reconsider whether the `Mgmt*` prefixed duplicates are still needed. **Resolution** -_Unresolved._ +Resolved 2026-05-16 (commit pending) — confirmed the asymmetry: `GetCommandName` stripped +`Command` from any type while `BuildRegistry` only registered the `Messages.Management` +namespace. In practice no defect was observed because every command type the CLI and +ManagementService actually use is in `Messages.Management` (a round-trip test over all +registered commands confirms no name collision). Closed the asymmetry by making +`GetCommandName` registry-bound: it now looks up a reverse `Type→name` frozen dictionary +built from the same registry and throws `ArgumentException` for any unregistered type, so +`Resolve(GetCommandName(t)) == t` holds for every type it accepts. Added an XML remarks +block documenting the registry scope and the symmetry guarantee. The `Mgmt*` prefixed +records were left in place — they are the genuine Management-namespace command types the +CLI constructs and renaming them would change wire command names (out of scope for a +behavior-preserving fix; noted for a future cleanup). CLI, ManagementService, and +SiteRuntime all build clean against the change. Regression tests added in +`ManagementCommandRegistryTests`. ### Commons-005 — `OpcUaEndpointConfigSerializer.Deserialize` discards malformed legacy input and over-reports `IsLegacy` diff --git a/src/ScadaLink.Commons/Messages/Management/ManagementCommandRegistry.cs b/src/ScadaLink.Commons/Messages/Management/ManagementCommandRegistry.cs index 002eef5..7c9acb3 100644 --- a/src/ScadaLink.Commons/Messages/Management/ManagementCommandRegistry.cs +++ b/src/ScadaLink.Commons/Messages/Management/ManagementCommandRegistry.cs @@ -2,21 +2,56 @@ using System.Collections.Frozen; namespace ScadaLink.Commons.Messages.Management; +/// +/// Bidirectional name registry for management command records. The registry contains +/// exactly the non-abstract *Command types declared in the +/// ScadaLink.Commons.Messages.Management namespace; these are the commands that +/// travel over the HTTP / ClusterClient management boundary. +/// +/// +/// and are symmetric: +/// Resolve(GetCommandName(t)) returns t for every type +/// accepts. rejects any type +/// the registry does not contain rather than computing an unresolvable name. +/// public static class ManagementCommandRegistry { private static readonly FrozenDictionary Commands = BuildRegistry(); + /// + /// Names keyed by command type, for the reverse lookup. Keeps + /// in lock-step with the forward registry. + /// + private static readonly FrozenDictionary NamesByType = + Commands.ToFrozenDictionary(kv => kv.Value, kv => kv.Key); + public static Type? Resolve(string commandName) { return Commands.GetValueOrDefault(commandName); } + /// + /// Returns the registered wire name for a management command type. + /// + /// + /// Thrown when is not a registered management + /// command — i.e. not a non-abstract *Command type in the + /// ScadaLink.Commons.Messages.Management namespace. This keeps the method + /// symmetric with : it never yields a name that + /// cannot turn back into the same type. + /// public static string GetCommandName(Type commandType) { - var name = commandType.Name; - return name.EndsWith("Command", StringComparison.Ordinal) - ? name[..^"Command".Length] - : name; + ArgumentNullException.ThrowIfNull(commandType); + + if (NamesByType.TryGetValue(commandType, out var name)) + return name; + + throw new ArgumentException( + $"'{commandType.FullName}' is not a registered management command. " + + $"Management commands must be non-abstract '*Command' records declared in " + + $"the '{typeof(ManagementEnvelope).Namespace}' namespace.", + nameof(commandType)); } private static FrozenDictionary BuildRegistry() diff --git a/src/ScadaLink.Commons/Types/DynamicJsonElement.cs b/src/ScadaLink.Commons/Types/DynamicJsonElement.cs index 07e0ece..1124178 100644 --- a/src/ScadaLink.Commons/Types/DynamicJsonElement.cs +++ b/src/ScadaLink.Commons/Types/DynamicJsonElement.cs @@ -7,13 +7,22 @@ namespace ScadaLink.Commons.Types; /// Wraps a JsonElement as a dynamic object for convenient property access in scripts. /// Supports property access (obj.name), indexing (obj.items[0]), and ToString(). /// +/// +/// The element passed to the constructor is cloned +/// so the wrapper owns a self-contained copy. This decouples its lifetime from the +/// the element originated from: a wrapper built from an +/// element inside a using block remains valid for deferred (e.g. script-time) +/// access after that document has been disposed. +/// public class DynamicJsonElement : DynamicObject { private readonly JsonElement _element; public DynamicJsonElement(JsonElement element) { - _element = element; + // Clone detaches the element from its owning JsonDocument so accessing it + // later cannot throw ObjectDisposedException once that document is disposed. + _element = element.Clone(); } public override bool TryGetMember(GetMemberBinder binder, out object? result) diff --git a/src/ScadaLink.Commons/Types/ScriptParameters.cs b/src/ScadaLink.Commons/Types/ScriptParameters.cs index 741a0b9..46a1e6a 100644 --- a/src/ScadaLink.Commons/Types/ScriptParameters.cs +++ b/src/ScadaLink.Commons/Types/ScriptParameters.cs @@ -24,7 +24,8 @@ public class ScriptParameters : IReadOnlyDictionary /// Gets a parameter value with typed conversion. /// /// Get<int>("key") — throws if missing, null, or unconvertible. - /// Get<int?>("key") — returns null if missing, null, or unconvertible. + /// Get<int?>("key") — returns null if the parameter is missing or null; + /// throws if it is present but holds an unconvertible value. /// Get<int[]>("key") — converts list to typed array; throws on first bad element. /// Get<List<int>>("key") — converts list to typed List; throws on first bad element. /// @@ -71,18 +72,17 @@ public class ScriptParameters : IReadOnlyDictionary private T GetNullable(string key, Type underlyingType) { + // Absent or explicitly-null parameter — the caller did not supply a value. if (!_inner.TryGetValue(key, out var value) || value is null) return default!; // null for Nullable - try - { - var converted = ConvertScalar(value, underlyingType, key); - return (T)converted; - } - catch (ScriptParameterException) - { - return default!; // null on conversion failure for nullable - } + // A parameter that is *present but non-null* must be convertible. A value + // that cannot be converted is a caller/script bug, not "not supplied": + // throw with a descriptive message rather than silently returning null + // (which a script would misread as absent). This mirrors Get() and the + // array/list element paths. See Commons-003. + var converted = ConvertScalar(value, underlyingType, key); + return (T)converted; } private Array ConvertToArray(string key, Type elementType) diff --git a/src/ScadaLink.Commons/Types/StaleTagMonitor.cs b/src/ScadaLink.Commons/Types/StaleTagMonitor.cs index c188086..d11ed1b 100644 --- a/src/ScadaLink.Commons/Types/StaleTagMonitor.cs +++ b/src/ScadaLink.Commons/Types/StaleTagMonitor.cs @@ -1,3 +1,7 @@ +using System.Runtime.CompilerServices; + +[assembly: InternalsVisibleTo("ScadaLink.Commons.Tests")] + namespace ScadaLink.Commons.Types; /// @@ -5,11 +9,29 @@ namespace ScadaLink.Commons.Types; /// within , the event fires. /// Composable into any IDataConnection adapter. /// +/// +/// Thread-safe: , and +/// may be called from any thread and race the internal timer callback. Each call to +/// or begins a new monitoring period +/// identified by a generation token; a timer callback only raises +/// if it still belongs to the current period. A fresh value, a restart, or a +/// arriving while a previous-period callback is in flight bumps the +/// generation, so that callback observes the mismatch and declines to fire — no spurious +/// staleness signal is emitted after the period it was scheduled for has ended. +/// public sealed class StaleTagMonitor : IDisposable { private readonly TimeSpan _maxSilence; + private readonly object _gate = new(); private Timer? _timer; - private volatile bool _staleFired; + + /// + /// Monotonically increasing token identifying the current monitoring period. + /// Bumped on every , and + /// so that a timer callback scheduled for an earlier period + /// can detect that it is stale and decline to fire. + /// + private long _generation; public StaleTagMonitor(TimeSpan maxSilence) { @@ -26,14 +48,25 @@ public sealed class StaleTagMonitor : IDisposable public TimeSpan MaxSilence => _maxSilence; + /// + /// Test-only seam invoked by the timer callback after it has been entered but + /// before it acquires the synchronization gate. Allows a test to deterministically + /// interleave a / with an in-flight + /// callback to exercise the stale-fire race. Never set in production. + /// + internal Action? CallbackEnteredHook { get; set; } + /// /// Start monitoring. The timer begins counting from now. /// public void Start() { - _staleFired = false; - _timer?.Dispose(); - _timer = new Timer(OnTimerElapsed, null, _maxSilence, Timeout.InfiniteTimeSpan); + lock (_gate) + { + _generation++; + _timer?.Dispose(); + _timer = new Timer(OnTimerElapsed, _generation, _maxSilence, Timeout.InfiniteTimeSpan); + } } /// @@ -41,8 +74,20 @@ public sealed class StaleTagMonitor : IDisposable /// public void OnValueReceived() { - _staleFired = false; - _timer?.Change(_maxSilence, Timeout.InfiniteTimeSpan); + lock (_gate) + { + // No active monitoring — nothing to reset. + if (_timer is null) + return; + + // Bump the generation: any timer callback for the previous period that + // has already been entered will see a generation mismatch and decline to + // raise Stale. The timer is recreated rather than re-armed with + // Change(...) so the new callback carries the new generation token. + _generation++; + _timer.Dispose(); + _timer = new Timer(OnTimerElapsed, _generation, _maxSilence, Timeout.InfiniteTimeSpan); + } } /// @@ -50,8 +95,14 @@ public sealed class StaleTagMonitor : IDisposable /// public void Stop() { - _timer?.Dispose(); - _timer = null; + lock (_gate) + { + // Bumping the generation invalidates any in-flight callback so a stopped + // monitor cannot deliver a Stale signal. + _generation++; + _timer?.Dispose(); + _timer = null; + } } public void Dispose() @@ -61,8 +112,24 @@ public sealed class StaleTagMonitor : IDisposable private void OnTimerElapsed(object? state) { - if (_staleFired) return; - _staleFired = true; + var scheduledGeneration = (long)state!; + + CallbackEnteredHook?.Invoke(); + + // Only fire if this callback still represents the current period. The check + // and the generation bump happen under the gate, so a concurrent + // OnValueReceived / Stop / Start either completes before this guard (its + // generation bump makes this callback decline) or serializes after it. + lock (_gate) + { + if (_generation != scheduledGeneration) + return; + + // Consume this period so a duplicate callback for the same generation + // cannot fire twice; the next Start/OnValueReceived issues a new token. + _generation++; + } + Stale?.Invoke(); } } diff --git a/tests/ScadaLink.Commons.Tests/Messages/ManagementCommandRegistryTests.cs b/tests/ScadaLink.Commons.Tests/Messages/ManagementCommandRegistryTests.cs new file mode 100644 index 0000000..ea4ee5c --- /dev/null +++ b/tests/ScadaLink.Commons.Tests/Messages/ManagementCommandRegistryTests.cs @@ -0,0 +1,78 @@ +using System.Reflection; +using ScadaLink.Commons.Messages.Management; + +namespace ScadaLink.Commons.Tests.Messages; + +/// +/// Tests for , including the Commons-004 +/// regression: GetCommandName and Resolve must be symmetric — every +/// type for which GetCommandName yields a name must round-trip back to the +/// same type via Resolve. +/// +public class ManagementCommandRegistryTests +{ + private static IEnumerable RegisteredCommandTypes() => + typeof(ManagementEnvelope).Assembly.GetTypes() + .Where(t => t.Namespace == typeof(ManagementEnvelope).Namespace + && t.Name.EndsWith("Command", StringComparison.Ordinal) + && !t.IsAbstract); + + [Fact] + public void GetCommandName_Resolve_RoundTrips_ForEveryRegisteredCommand() + { + foreach (var type in RegisteredCommandTypes()) + { + var name = ManagementCommandRegistry.GetCommandName(type); + var resolved = ManagementCommandRegistry.Resolve(name); + Assert.Equal(type, resolved); + } + } + + [Fact] + public void Resolve_KnownCommand_ReturnsType() + { + var type = ManagementCommandRegistry.Resolve("CreateSite"); + Assert.Equal(typeof(CreateSiteCommand), type); + } + + [Fact] + public void Resolve_UnknownCommand_ReturnsNull() + { + Assert.Null(ManagementCommandRegistry.Resolve("NoSuchCommand")); + } + + [Fact] + public void Resolve_IsCaseInsensitive() + { + Assert.Equal(typeof(CreateSiteCommand), ManagementCommandRegistry.Resolve("createsite")); + } + + /// + /// Commons-004: GetCommandName previously stripped a Command suffix + /// from any type, producing names the registry cannot resolve. It must + /// only return a name for a command type the registry actually contains. + /// + [Fact] + public void GetCommandName_UnregisteredCommandType_Throws() + { + // A *Command type that is not in the Messages.Management namespace. + Assert.Throws( + () => ManagementCommandRegistry.GetCommandName(typeof(UnregisteredFakeCommand))); + } + + [Fact] + public void GetCommandName_NonCommandType_Throws() + { + Assert.Throws( + () => ManagementCommandRegistry.GetCommandName(typeof(string))); + } + + [Fact] + public void GetCommandName_RegisteredCommand_ReturnsStrippedName() + { + Assert.Equal("CreateSite", ManagementCommandRegistry.GetCommandName(typeof(CreateSiteCommand))); + } + + /// A *Command record outside the Management namespace, for the negative test. + private record UnregisteredFakeCommand(int Id); +} diff --git a/tests/ScadaLink.Commons.Tests/Types/DynamicJsonElementTests.cs b/tests/ScadaLink.Commons.Tests/Types/DynamicJsonElementTests.cs new file mode 100644 index 0000000..f3e75ca --- /dev/null +++ b/tests/ScadaLink.Commons.Tests/Types/DynamicJsonElementTests.cs @@ -0,0 +1,103 @@ +using System.Text.Json; +using ScadaLink.Commons.Types; + +namespace ScadaLink.Commons.Tests.Types; + +/// +/// Tests for , including the Commons-002 regression: +/// a wrapped element must remain valid for deferred (script-time) access even after +/// the it was parsed from has been disposed. +/// +public class DynamicJsonElementTests +{ + private static DynamicJsonElement Wrap(string json) + { + using var doc = JsonDocument.Parse(json); + return new DynamicJsonElement(doc.RootElement); + // doc is disposed here — a wrapper that retained a non-cloned element would + // now throw ObjectDisposedException on the first member access. + } + + // ── Commons-002 regression: lifetime independence from the source document ── + + [Fact] + public void MemberAccess_WorksAfterSourceDocumentDisposed() + { + dynamic obj = Wrap("""{ "name": "pump", "id": 7 }"""); + + Assert.Equal("pump", (string)obj.name); + Assert.Equal(7, (int)obj.id); + } + + [Fact] + public void IndexAccess_WorksAfterSourceDocumentDisposed() + { + dynamic obj = Wrap("""{ "items": [ "a", "b", "c" ] }"""); + + Assert.Equal("b", obj.items[1]); + } + + [Fact] + public void NestedAccess_WorksAfterSourceDocumentDisposed() + { + dynamic obj = Wrap("""{ "outer": { "inner": { "value": 42 } } }"""); + + Assert.Equal(42, (int)obj.outer.inner.value); + } + + [Fact] + public void ToString_WorksAfterSourceDocumentDisposed() + { + dynamic obj = Wrap("""{ "label": "site-1" }"""); + + Assert.Equal("site-1", obj.label.ToString()); + } + + [Fact] + public void Access_SurvivesGarbageCollection_OfSourceDocument() + { + // No reference to the source document is held anywhere; force collection + // and finalization to prove the wrapper does not depend on it. + var obj = MakeWrapperAndDropDocument(); + GC.Collect(); + GC.WaitForPendingFinalizers(); + GC.Collect(); + + dynamic d = obj; + Assert.Equal("ok", (string)d.status); + } + + private static DynamicJsonElement MakeWrapperAndDropDocument() + { + var doc = JsonDocument.Parse("""{ "status": "ok" }"""); + var wrapper = new DynamicJsonElement(doc.RootElement); + doc.Dispose(); + return wrapper; + } + + // ── Basic conversion / access behavior ────────────────────────── + + [Fact] + public void Convert_NumberToInt() + { + dynamic obj = Wrap("""{ "n": 123 }"""); + Assert.Equal(123, (int)obj.n); + } + + [Fact] + public void Convert_BoolFromJson() + { + dynamic obj = Wrap("""{ "flag": true }"""); + Assert.True((bool)obj.flag); + } + + [Fact] + public void MissingMember_Throws() + { + // TryGetMember returns false for an absent property, so the dynamic binder + // surfaces a RuntimeBinderException — the standard DynamicObject contract. + dynamic obj = Wrap("""{ "a": 1 }"""); + Assert.Throws( + () => { var _ = obj.doesNotExist; }); + } +} diff --git a/tests/ScadaLink.Commons.Tests/Types/ScriptParametersTests.cs b/tests/ScadaLink.Commons.Tests/Types/ScriptParametersTests.cs index ac7f60a..b25845d 100644 --- a/tests/ScadaLink.Commons.Tests/Types/ScriptParametersTests.cs +++ b/tests/ScadaLink.Commons.Tests/Types/ScriptParametersTests.cs @@ -148,11 +148,30 @@ public class ScriptParametersTests Assert.Equal(42, p.Get("x")); } + // Commons-003: a parameter that is *present but unconvertible* is a caller/script + // bug and must throw — not be silently mapped to null (which a script would + // misread as "not supplied"). Genuinely absent/null still returns null. [Fact] - public void Get_NullableInt_Unparsable_ReturnsNull() + public void Get_NullableInt_PresentButUnparsable_Throws() { - var p = new ScriptParameters(new Dictionary { ["x"] = "abc" }); - Assert.Null(p.Get("x")); + var p = new ScriptParameters(new Dictionary { ["x"] = "banana" }); + var ex = Assert.Throws(() => p.Get("x")); + Assert.Contains("could not be parsed as Int32", ex.Message); + } + + [Fact] + public void Get_NullableInt_PresentButOverflowing_Throws() + { + var p = new ScriptParameters(new Dictionary { ["x"] = long.MaxValue }); + var ex = Assert.Throws(() => p.Get("x")); + Assert.Contains("could not be parsed as Int32", ex.Message); + } + + [Fact] + public void Get_NullableDateTime_PresentButUnparsable_Throws() + { + var p = new ScriptParameters(new Dictionary { ["dt"] = "not-a-date" }); + Assert.Throws(() => p.Get("dt")); } [Fact] diff --git a/tests/ScadaLink.Commons.Tests/Types/StaleTagMonitorRaceTests.cs b/tests/ScadaLink.Commons.Tests/Types/StaleTagMonitorRaceTests.cs new file mode 100644 index 0000000..89ef9f8 --- /dev/null +++ b/tests/ScadaLink.Commons.Tests/Types/StaleTagMonitorRaceTests.cs @@ -0,0 +1,119 @@ +using ScadaLink.Commons.Types; + +namespace ScadaLink.Commons.Tests.Types; + +/// +/// Regression tests for Commons-001: the check-then-act race between the timer +/// callback (OnTimerElapsed) and OnValueReceived / Stop / Start. +/// +/// The original implementation guarded firing with a single volatile bool that +/// was both read by the callback and reset by the caller threads. Because the +/// check-then-set was not atomic with the timer reschedule, a callback that had +/// already entered could raise Stale after the period it was scheduled for +/// had been cancelled or restarted — a spurious staleness signal that, for a +/// connection-health monitor, triggers an unnecessary reconnect. +/// +/// These tests use the internal CallbackEnteredHook seam to deterministically +/// interleave a caller-thread operation with an in-flight callback. +/// +public class StaleTagMonitorRaceTests +{ + /// + /// A value arrives (OnValueReceived) while a previous-period timer callback + /// is in flight, before that callback decides whether to fire. The old period has + /// been superseded, so the in-flight callback must not raise Stale + /// immediately; Stale may only fire later, for the fresh period, after a + /// full MaxSilence with no further values. + /// + /// With the original single-volatile-bool guard the in-flight callback fired + /// Stale right after the value arrived (a spurious, wrong-moment signal); + /// this test detects that by checking how soon the fire lands after the value. + /// + [Fact] + public void Stale_DoesNotFirePromptly_WhenValueArrivesWhileCallbackInFlight() + { + var maxSilence = TimeSpan.FromMilliseconds(60); + using var monitor = new StaleTagMonitor(maxSilence); + + DateTime? valueArrivedAt = null; + DateTime? staleFiredAt = null; + monitor.Stale += () => staleFiredAt ??= DateTime.UtcNow; + + // When the (old-period) callback is entered, simulate a fresh value arriving + // on another thread before the callback's fire decision. + monitor.CallbackEnteredHook = () => + { + monitor.CallbackEnteredHook = null; // only intercept the first callback + valueArrivedAt = DateTime.UtcNow; + monitor.OnValueReceived(); + }; + + monitor.Start(); + + // Wait well past the intercepted callback and the fresh period's deadline. + Thread.Sleep(300); + monitor.Stop(); + + // The fresh period legitimately goes stale, so a fire is expected — but it + // must land roughly MaxSilence after the value, not immediately. A spurious + // wrong-moment fire from the superseded callback would land within a few ms. + Assert.NotNull(valueArrivedAt); + Assert.NotNull(staleFiredAt); + var delay = staleFiredAt.Value - valueArrivedAt.Value; + Assert.True(delay >= maxSilence * 0.5, + $"Stale fired only {delay.TotalMilliseconds:F0}ms after the value arrived; " + + $"expected at least {maxSilence.TotalMilliseconds * 0.5:F0}ms — the in-flight " + + "callback fired spuriously for the superseded period."); + } + + /// + /// Stop races an in-flight timer callback. Once monitoring is stopped no + /// Stale signal may be delivered, even for a callback that had already + /// been entered. + /// + [Fact] + public void Stale_DoesNotFire_WhenStopRacesInFlightCallback() + { + using var monitor = new StaleTagMonitor(TimeSpan.FromMilliseconds(30)); + var staleCount = 0; + monitor.Stale += () => Interlocked.Increment(ref staleCount); + + monitor.CallbackEnteredHook = () => + { + monitor.CallbackEnteredHook = null; + monitor.Stop(); + }; + + monitor.Start(); + Thread.Sleep(200); + + Assert.Equal(0, staleCount); + } + + /// + /// Start (a restart) races an in-flight callback from the prior run. The + /// old callback belongs to a superseded period and must not fire; the new period + /// fires exactly once on its own deadline. + /// + [Fact] + public void Stale_FiresOnceForNewPeriod_WhenRestartRacesInFlightCallback() + { + using var monitor = new StaleTagMonitor(TimeSpan.FromMilliseconds(30)); + var staleCount = 0; + monitor.Stale += () => Interlocked.Increment(ref staleCount); + + monitor.CallbackEnteredHook = () => + { + monitor.CallbackEnteredHook = null; + monitor.Start(); // restart — supersedes the in-flight callback's period + }; + + monitor.Start(); + + // Old callback must be suppressed; the restarted period fires exactly once. + Thread.Sleep(250); + monitor.Stop(); + + Assert.Equal(1, staleCount); + } +}