From bed7011ccd20d09e0332112e34c86f7399c81850 Mon Sep 17 00:00:00 2001 From: Tyler Kron Date: Sun, 2 Aug 2026 13:03:38 -0600 Subject: [PATCH 1/2] test(winc): make the abandoned-open tests deterministic instead of timing-based Both abandoned-open tests raced a 150ms fault against a 50ms openTimeout and asserted an exact exception type. Task.WaitAsync(timeout) only times out when its timer fires before the task completes, so a contended runner that slipped the 50ms timer past 150ms saw the fault land first and caught InvalidOperationException instead of TimeoutException. Gate the fault on an explicit signal the test releases only after it has observed the timeout, so the ordering is guaranteed rather than likely. The behaviour under test - observing the abandoned open's fault, and disposing the port even when the logger throws - is unchanged. Co-Authored-By: Claude Opus 5 --- .../Firmware/Winc/WincFlasherTests.cs | 42 +++++++++++++++---- 1 file changed, 35 insertions(+), 7 deletions(-) diff --git a/src/Daqifi.Core.Tests/Firmware/Winc/WincFlasherTests.cs b/src/Daqifi.Core.Tests/Firmware/Winc/WincFlasherTests.cs index 8b1bad0..8fce053 100644 --- a/src/Daqifi.Core.Tests/Firmware/Winc/WincFlasherTests.cs +++ b/src/Daqifi.Core.Tests/Firmware/Winc/WincFlasherTests.cs @@ -332,7 +332,7 @@ public async Task Inspector_ObservesTheFaultOfAnAbandonedOpen() // to hand to the logger. So a logger that captures the exception proves the read happened, // using a seam that exists for production reasons rather than a test-only hook. var logger = new CapturingLogger(); - var port = new FaultingAfterDelayPort(TimeSpan.FromMilliseconds(150)); + var port = new FaultingOnReleasePort(); var inspector = new WincModuleInspector( (_, _) => port, logger, @@ -341,6 +341,10 @@ public async Task Inspector_ObservesTheFaultOfAnAbandonedOpen() await Assert.ThrowsAsync(() => inspector.ReadIdentityAsync("COM1")); + // Only now let the abandoned open fault, so it provably lands after the timeout rather than + // racing it. + port.ReleaseFault(); + var observed = await logger.FirstException.WaitAsync(TimeSpan.FromSeconds(10)); Assert.IsType(observed); @@ -356,7 +360,7 @@ public async Task Inspector_DisposesTheAbandonedPort_EvenWhenObservingTheFaultTh // Releasing the handle is the whole reason the abandonment continuation exists. If a // throwing logger could skip past the disposal, the abandoned port would leak and break // every later open until the process exits — strictly worse than the fault being reported. - var port = new FaultingAfterDelayPort(TimeSpan.FromMilliseconds(150)); + var port = new FaultingOnReleasePort(); var inspector = new WincModuleInspector( (_, _) => port, new ThrowingLogger(), @@ -365,6 +369,10 @@ public async Task Inspector_DisposesTheAbandonedPort_EvenWhenObservingTheFaultTh await Assert.ThrowsAsync(() => inspector.ReadIdentityAsync("COM1")); + // Only now let the abandoned open fault, so it provably lands after the timeout rather than + // racing it. + port.ReleaseFault(); + // Disposal must still happen despite the observation throwing. await port.Disposed.WaitAsync(TimeSpan.FromSeconds(10)); } @@ -608,23 +616,43 @@ public void Log( } /// - /// A port whose blocks briefly and then throws, standing in for an open that - /// is abandoned on the timeout and only fails afterwards. + /// A port whose blocks until the test releases it and then throws, standing + /// in for an open that is abandoned on the timeout and only fails afterwards. /// - private sealed class FaultingAfterDelayPort(TimeSpan delay) : IWincSerialPort - { + /// + /// The fault is gated on a signal rather than a wall-clock delay because the ordering has to be + /// guaranteed, not merely likely. An earlier version slept 150 ms against a 50 ms + /// openTimeout and leaned on that margin, but only + /// times out when its timer fires before the task completes — so a runner contended + /// enough to slip a 50 ms timer past 150 ms saw the fault land first, and the assertion caught + /// instead of . That + /// reddened unrelated PRs under full-suite load while passing in isolation. Holding the fault + /// until the timeout has already been observed removes the timing assumption entirely. + /// + private sealed class FaultingOnReleasePort : IWincSerialPort + { + private readonly SemaphoreSlim _release = new(0, 1); + private readonly TaskCompletionSource _disposed = new(TaskCreationOptions.RunContinuationsAsynchronously); /// Completes when the port is disposed, so tests can await it deterministically. internal Task Disposed => _disposed.Task; + /// + /// Lets the blocked open proceed to its fault. Call only once the timeout has been observed, + /// which is what makes the abandoned-open ordering deterministic. + /// + internal void ReleaseFault() => _release.Release(); + public bool IsOpen => false; public int BaudRate { get; set; } = 115200; public void Open() { - Thread.Sleep(delay); + // Bounded so a test that fails before releasing surfaces its own assertion rather than + // parking a pool thread for the life of the run. + _release.Wait(TimeSpan.FromSeconds(30)); throw new InvalidOperationException("abandoned-open-fault"); } From 24ef41bddf6ad08371399f1aec698fd3cf19aeec Mon Sep 17 00:00:00 2001 From: Tyler Kron Date: Sun, 2 Aug 2026 15:24:41 -0600 Subject: [PATCH 2/2] test(winc): cap the gated open at the tests' own 10s wait The 30s fallback outlived the tests' 10s waits, so a test that failed before releasing left the abandoned open parked on a pool thread for ~20s after the test finished - extra contention in the suite whose load sensitivity is the reason for this change. Cap at 10s and fail loudly with a distinct message so a missing ReleaseFault is diagnosed rather than mistaken for the fault under test. Co-Authored-By: Claude Opus 5 --- .../Firmware/Winc/WincFlasherTests.cs | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/src/Daqifi.Core.Tests/Firmware/Winc/WincFlasherTests.cs b/src/Daqifi.Core.Tests/Firmware/Winc/WincFlasherTests.cs index 8fce053..a978a88 100644 --- a/src/Daqifi.Core.Tests/Firmware/Winc/WincFlasherTests.cs +++ b/src/Daqifi.Core.Tests/Firmware/Winc/WincFlasherTests.cs @@ -650,9 +650,16 @@ private sealed class FaultingOnReleasePort : IWincSerialPort public void Open() { - // Bounded so a test that fails before releasing surfaces its own assertion rather than - // parking a pool thread for the life of the run. - _release.Wait(TimeSpan.FromSeconds(30)); + // Bounded at the tests' own 10 s wait rather than above it. A test that fails before + // releasing surfaces its own assertion first either way, but anything longer leaves this + // parked on a pool thread after the test has finished — extra contention in the suite + // whose load sensitivity is the reason this class exists. + if (!_release.Wait(TimeSpan.FromSeconds(10))) + { + throw new InvalidOperationException( + "Test bug: ReleaseFault was never called, so the gated open timed out."); + } + throw new InvalidOperationException("abandoned-open-fault"); }