From 309e7f02658795e180ecb28478deb76f651adcf3 Mon Sep 17 00:00:00 2001 From: Tyler Kron Date: Sat, 23 May 2026 16:02:18 -0600 Subject: [PATCH 1/2] fix: replace Thread.Sleep with Task.Delay in async SD paths (closes #221) The SD interface settle delay inside *Async methods of DaqifiStreamingDevice used Thread.Sleep, which blocked a thread-pool thread for the settle window and ignored the operation's CancellationToken. Added a Func overload of ExecuteTextCommandAsync (refactored alongside the existing Action overload into a private async core) so setup lambdas can await Task.Delay(..., ct). Converted the four affected call sites in GetSdCardFilesAsync and DeleteSdCardFileAsync. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../Device/SdCard/SdCardOperationsTests.cs | 77 +++++++++++++++++++ src/Daqifi.Core/Device/DaqifiDevice.cs | 50 ++++++++++-- .../Device/DaqifiStreamingDevice.cs | 16 ++-- 3 files changed, 130 insertions(+), 13 deletions(-) diff --git a/src/Daqifi.Core.Tests/Device/SdCard/SdCardOperationsTests.cs b/src/Daqifi.Core.Tests/Device/SdCard/SdCardOperationsTests.cs index 1910019..12b40aa 100644 --- a/src/Daqifi.Core.Tests/Device/SdCard/SdCardOperationsTests.cs +++ b/src/Daqifi.Core.Tests/Device/SdCard/SdCardOperationsTests.cs @@ -229,6 +229,49 @@ public async Task GetSdCardFilesAsync_UpdatesSdCardFilesProperty() Assert.Equal("test.bin", device.SdCardFiles[0].FileName); } + [Fact] + public async Task GetSdCardFilesAsync_HonorsCancellationDuringSettleDelay() + { + // Regression for #221: the SD interface settle wait used Thread.Sleep, + // which ignored the CancellationToken and blocked a thread-pool thread. + // After the fix the wait is await Task.Delay(..., ct), so cancelling + // mid-wait must propagate as OperationCanceledException promptly. + var device = new TestableSdCardStreamingDevice("TestDevice"); + device.CannedTextResponse = new List { "Daqifi/test.bin" }; + device.Connect(); + + using var cts = new CancellationTokenSource(TimeSpan.FromMilliseconds(20)); + + var stopwatch = System.Diagnostics.Stopwatch.StartNew(); + await Assert.ThrowsAnyAsync( + () => device.GetSdCardFilesAsync(cts.Token)); + stopwatch.Stop(); + + // SD_INTERFACE_SETTLE_DELAY_MS is 100ms; cancellation must fire before + // the delay completes. 80ms headroom for scheduler jitter on CI. + Assert.True(stopwatch.ElapsedMilliseconds < 80, + $"Expected cancellation before settle delay completed; took {stopwatch.ElapsedMilliseconds}ms."); + } + + [Fact] + public async Task DeleteSdCardFileAsync_HonorsCancellationDuringSettleDelay() + { + // Regression for #221 — symmetric with GetSdCardFilesAsync above. + var device = new TestableSdCardStreamingDevice("TestDevice"); + device.CannedTextResponse = new List { "Daqifi/other.bin" }; + device.Connect(); + + using var cts = new CancellationTokenSource(TimeSpan.FromMilliseconds(20)); + + var stopwatch = System.Diagnostics.Stopwatch.StartNew(); + await Assert.ThrowsAnyAsync( + () => device.DeleteSdCardFileAsync("data.bin", cts.Token)); + stopwatch.Stop(); + + Assert.True(stopwatch.ElapsedMilliseconds < 80, + $"Expected cancellation before settle delay completed; took {stopwatch.ElapsedMilliseconds}ms."); + } + [Fact] public async Task StartSdCardLoggingAsync_SendsCorrectCommandSequence() { @@ -1238,6 +1281,20 @@ protected override Task> ExecuteTextCommandAsync( : new List(); return Task.FromResult>(response); } + + protected override async Task> ExecuteTextCommandAsync( + Func setupActionAsync, + int responseTimeoutMs = 1000, + int completionTimeoutMs = 250, + CancellationToken cancellationToken = default) + { + await setupActionAsync(cancellationToken).ConfigureAwait(false); + ExecuteTextCommandCallCount++; + var response = ResponseSequence.Count > 0 + ? ResponseSequence.Dequeue() + : new List(); + return response; + } } /// @@ -1277,6 +1334,16 @@ protected override Task> ExecuteTextCommandAsync( setupAction(); return Task.FromResult>(CannedTextResponse); } + + protected override async Task> ExecuteTextCommandAsync( + Func setupActionAsync, + int responseTimeoutMs = 1000, + int completionTimeoutMs = 250, + CancellationToken cancellationToken = default) + { + await setupActionAsync(cancellationToken).ConfigureAwait(false); + return CannedTextResponse; + } } /// @@ -1316,6 +1383,16 @@ protected override Task> ExecuteTextCommandAsync( return Task.FromResult>(CannedTextResponse); } + protected override async Task> ExecuteTextCommandAsync( + Func setupActionAsync, + int responseTimeoutMs = 1000, + int completionTimeoutMs = 250, + CancellationToken cancellationToken = default) + { + await setupActionAsync(cancellationToken).ConfigureAwait(false); + return CannedTextResponse; + } + protected override async Task ExecuteRawCaptureAsync( Func rawAction, CancellationToken cancellationToken = default) diff --git a/src/Daqifi.Core/Device/DaqifiDevice.cs b/src/Daqifi.Core/Device/DaqifiDevice.cs index 6a2d3c5..8d7935e 100644 --- a/src/Daqifi.Core/Device/DaqifiDevice.cs +++ b/src/Daqifi.Core/Device/DaqifiDevice.cs @@ -221,7 +221,7 @@ public void Connect() /// /// Waits up to 10 seconds to acquire _textExchangeLock before /// tearing down the consumer / producer / transport. This prevents - /// a race where an in-flight + /// a race where an in-flight /// is mid-swap (text consumer running on the stream, protobuf /// consumer not yet restarted) and Disconnect rips the transport /// out from under it. If the wait times out, Disconnect proceeds @@ -392,11 +392,50 @@ protected virtual async Task ExecuteRawCaptureAsync( /// A list of text lines received from the device. /// Thrown when the device is not connected or has no transport. /// Thrown when the operation is canceled. - protected virtual async Task> ExecuteTextCommandAsync( + protected virtual Task> ExecuteTextCommandAsync( Action setupAction, int responseTimeoutMs = 1000, int completionTimeoutMs = 250, CancellationToken cancellationToken = default) + { + return ExecuteTextCommandCoreAsync( + _ => { setupAction(); return Task.CompletedTask; }, + responseTimeoutMs, + completionTimeoutMs, + cancellationToken); + } + + /// + /// Async overload of + /// that accepts an async setup action so callers can await cancellable operations + /// (e.g. ) between SCPI commands without + /// blocking the thread-pool thread. + /// + /// An async function that sends SCPI commands to the device while the text consumer is active. Receives the operation's cancellation token. + /// The time in milliseconds to wait for the first text response after sending commands. + /// The time in milliseconds of inactivity after the first response before considering the response complete. Defaults to 250ms. + /// A cancellation token to observe while waiting for the task to complete. + /// A list of text lines received from the device. + /// Thrown when the device is not connected or has no transport. + /// Thrown when the operation is canceled. + protected virtual Task> ExecuteTextCommandAsync( + Func setupActionAsync, + int responseTimeoutMs = 1000, + int completionTimeoutMs = 250, + CancellationToken cancellationToken = default) + { + return ExecuteTextCommandCoreAsync( + setupActionAsync, + responseTimeoutMs, + completionTimeoutMs, + cancellationToken); + } + + private async Task> ExecuteTextCommandCoreAsync( + Func setupActionAsync, + int responseTimeoutMs, + int completionTimeoutMs, + CancellationToken cancellationToken) { if (responseTimeoutMs <= 0) throw new ArgumentOutOfRangeException(nameof(responseTimeoutMs), responseTimeoutMs, "Timeout must be positive."); @@ -511,8 +550,9 @@ protected virtual async Task> ExecuteTextCommandAsync( Trace.WriteLine($"[ExecuteTextCommandAsync] Text consumer started at {sw.ElapsedMilliseconds}ms"); - // Execute the setup action (sends SCPI commands) - setupAction(); + // Execute the setup action (sends SCPI commands). ConfigureAwait(false) + // matches the surrounding lock-protected awaits. + await setupActionAsync(cancellationToken).ConfigureAwait(false); Trace.WriteLine($"[ExecuteTextCommandAsync] Setup action completed at {sw.ElapsedMilliseconds}ms"); @@ -627,7 +667,7 @@ protected virtual async Task> ExecuteTextCommandAsync( /// on hardware faults, or discard them if known-stale. /// /// - /// Each iteration uses , which + /// Each iteration uses , which /// pauses the protobuf consumer for the duration of the text exchange. /// Avoid calling this during active streaming or concurrently with /// other text commands. diff --git a/src/Daqifi.Core/Device/DaqifiStreamingDevice.cs b/src/Daqifi.Core/Device/DaqifiStreamingDevice.cs index bb61173..cab157e 100644 --- a/src/Daqifi.Core/Device/DaqifiStreamingDevice.cs +++ b/src/Daqifi.Core/Device/DaqifiStreamingDevice.cs @@ -340,14 +340,14 @@ public async Task> GetSdCardFilesAsync(Cancellatio IReadOnlyList lines; try { - lines = await ExecuteTextCommandAsync(() => + lines = await ExecuteTextCommandAsync(async ct => { PrepareSdInterface(); // Allow the device firmware to complete the SPI bus switch // before querying the SD card. Without this delay, the device // can return SCPI error -200 (Execution error). - Thread.Sleep(SD_INTERFACE_SETTLE_DELAY_MS); + await Task.Delay(SD_INTERFACE_SETTLE_DELAY_MS, ct).ConfigureAwait(false); Send(ScpiMessageProducer.GetSdFileList); }, responseTimeoutMs: 3000, cancellationToken: cancellationToken); @@ -362,10 +362,10 @@ public async Task> GetSdCardFilesAsync(Cancellatio await Task.Delay(SD_INTERFACE_SETTLE_DELAY_MS, cancellationToken); - lines = await ExecuteTextCommandAsync(() => + lines = await ExecuteTextCommandAsync(async ct => { PrepareSdInterface(); - Thread.Sleep(SD_INTERFACE_SETTLE_DELAY_MS); + await Task.Delay(SD_INTERFACE_SETTLE_DELAY_MS, ct).ConfigureAwait(false); Send(ScpiMessageProducer.GetSdFileList); }, responseTimeoutMs: 3000, cancellationToken: cancellationToken); @@ -546,10 +546,10 @@ public async Task DeleteSdCardFileAsync(string fileName, CancellationToken cance IReadOnlyList lines; try { - lines = await ExecuteTextCommandAsync(() => + lines = await ExecuteTextCommandAsync(async ct => { PrepareSdInterface(); - Thread.Sleep(SD_INTERFACE_SETTLE_DELAY_MS); + await Task.Delay(SD_INTERFACE_SETTLE_DELAY_MS, ct).ConfigureAwait(false); Send(ScpiMessageProducer.DeleteSdFile(fileName)); Send(ScpiMessageProducer.GetSdFileList); }, responseTimeoutMs: 3000, cancellationToken: cancellationToken); @@ -562,10 +562,10 @@ public async Task DeleteSdCardFileAsync(string fileName, CancellationToken cance await Task.Delay(SD_INTERFACE_SETTLE_DELAY_MS, cancellationToken); - lines = await ExecuteTextCommandAsync(() => + lines = await ExecuteTextCommandAsync(async ct => { PrepareSdInterface(); - Thread.Sleep(SD_INTERFACE_SETTLE_DELAY_MS); + await Task.Delay(SD_INTERFACE_SETTLE_DELAY_MS, ct).ConfigureAwait(false); Send(ScpiMessageProducer.DeleteSdFile(fileName)); Send(ScpiMessageProducer.GetSdFileList); }, responseTimeoutMs: 3000, cancellationToken: cancellationToken); From 5bb512f984867536f1f24999d5d09d813e4d5486 Mon Sep 17 00:00:00 2001 From: Tyler Kron Date: Sat, 23 May 2026 16:12:16 -0600 Subject: [PATCH 2/2] test: cancel deterministically in SD settle-delay regression tests Replace the timer-based CancellationTokenSource and <80ms wall-clock upper-bound assertions with deterministic cancellation: start the operation, let it suspend in the settle Task.Delay, then Cancel() and assert the OperationCanceledException propagates. Removes CI scheduler jitter flakiness flagged by Qodo on #226 without weakening the regression guarantee for #221. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../Device/SdCard/SdCardOperationsTests.cs | 37 +++++++++---------- 1 file changed, 17 insertions(+), 20 deletions(-) diff --git a/src/Daqifi.Core.Tests/Device/SdCard/SdCardOperationsTests.cs b/src/Daqifi.Core.Tests/Device/SdCard/SdCardOperationsTests.cs index 12b40aa..a184061 100644 --- a/src/Daqifi.Core.Tests/Device/SdCard/SdCardOperationsTests.cs +++ b/src/Daqifi.Core.Tests/Device/SdCard/SdCardOperationsTests.cs @@ -233,24 +233,25 @@ public async Task GetSdCardFilesAsync_UpdatesSdCardFilesProperty() public async Task GetSdCardFilesAsync_HonorsCancellationDuringSettleDelay() { // Regression for #221: the SD interface settle wait used Thread.Sleep, - // which ignored the CancellationToken and blocked a thread-pool thread. - // After the fix the wait is await Task.Delay(..., ct), so cancelling - // mid-wait must propagate as OperationCanceledException promptly. + // which ignored the CancellationToken. After the fix the wait is + // await Task.Delay(..., ct), so cancelling while the operation is + // suspended in the delay must propagate as OperationCanceledException. var device = new TestableSdCardStreamingDevice("TestDevice"); device.CannedTextResponse = new List { "Daqifi/test.bin" }; device.Connect(); - using var cts = new CancellationTokenSource(TimeSpan.FromMilliseconds(20)); + using var cts = new CancellationTokenSource(); - var stopwatch = System.Diagnostics.Stopwatch.StartNew(); - await Assert.ThrowsAnyAsync( - () => device.GetSdCardFilesAsync(cts.Token)); - stopwatch.Stop(); + // The sync portion of GetSdCardFilesAsync runs through the setup + // lambda's PrepareSdInterface and suspends at Task.Delay(..., ct). + // Once it returns a pending task, we cancel synchronously: Task.Delay + // observes the cancellation immediately. Under the old Thread.Sleep + // code the cancel would be ignored and the call would complete + // normally — no OperationCanceledException would be thrown. + var opTask = device.GetSdCardFilesAsync(cts.Token); + cts.Cancel(); - // SD_INTERFACE_SETTLE_DELAY_MS is 100ms; cancellation must fire before - // the delay completes. 80ms headroom for scheduler jitter on CI. - Assert.True(stopwatch.ElapsedMilliseconds < 80, - $"Expected cancellation before settle delay completed; took {stopwatch.ElapsedMilliseconds}ms."); + await Assert.ThrowsAnyAsync(() => opTask); } [Fact] @@ -261,15 +262,11 @@ public async Task DeleteSdCardFileAsync_HonorsCancellationDuringSettleDelay() device.CannedTextResponse = new List { "Daqifi/other.bin" }; device.Connect(); - using var cts = new CancellationTokenSource(TimeSpan.FromMilliseconds(20)); + using var cts = new CancellationTokenSource(); + var opTask = device.DeleteSdCardFileAsync("data.bin", cts.Token); + cts.Cancel(); - var stopwatch = System.Diagnostics.Stopwatch.StartNew(); - await Assert.ThrowsAnyAsync( - () => device.DeleteSdCardFileAsync("data.bin", cts.Token)); - stopwatch.Stop(); - - Assert.True(stopwatch.ElapsedMilliseconds < 80, - $"Expected cancellation before settle delay completed; took {stopwatch.ElapsedMilliseconds}ms."); + await Assert.ThrowsAnyAsync(() => opTask); } [Fact]