diff --git a/src/Daqifi.Core.Tests/Communication/Transport/SerialStreamTransportTests.cs b/src/Daqifi.Core.Tests/Communication/Transport/SerialStreamTransportTests.cs index 489e36c..725b47a 100644 --- a/src/Daqifi.Core.Tests/Communication/Transport/SerialStreamTransportTests.cs +++ b/src/Daqifi.Core.Tests/Communication/Transport/SerialStreamTransportTests.cs @@ -53,6 +53,26 @@ public void SerialStreamTransport_Stream_WhenNotConnected_ThrowsTransportNotConn Assert.Contains("COM1", ex.Message); } + [Fact] + public void SerialStreamTransport_ApplyOperationalTimeouts_BoundsBothDirections() + { + // #399: only ReadTimeout used to be lowered after open, so WriteTimeout kept the connect + // timeout for the life of the port. SerialPort.Write takes no CancellationToken, so a + // device that stops draining its receive buffer parks the write for that whole duration + // with nothing able to interrupt it. Both directions must end up bounded and short. + using var port = new SerialPort("COM1") + { + ReadTimeout = 30000, + WriteTimeout = 30000 + }; + + SerialStreamTransport.ApplyOperationalTimeouts(port); + + Assert.Equal(500, port.ReadTimeout); + Assert.Equal(2000, port.WriteTimeout); + Assert.NotEqual(SerialPort.InfiniteTimeout, port.WriteTimeout); + } + [Fact] public void SerialStreamTransport_SetSerialPortForTesting_TakesOwnershipAndDisposesPreviousPort() { diff --git a/src/Daqifi.Core.Tests/Device/SdCard/SdCardFileReceiverTests.cs b/src/Daqifi.Core.Tests/Device/SdCard/SdCardFileReceiverTests.cs index 7130afe..bf5d050 100644 --- a/src/Daqifi.Core.Tests/Device/SdCard/SdCardFileReceiverTests.cs +++ b/src/Daqifi.Core.Tests/Device/SdCard/SdCardFileReceiverTests.cs @@ -102,6 +102,27 @@ await Assert.ThrowsAnyAsync( cancellationToken: cts.Token)); } + [Fact] + public async Task ReceiveAsync_WhenCancelledAndStreamIgnoresTheToken_ReportsCancellationNotTimeout() + { + // #399: System.IO.Ports' SerialStream ignores the token once a read is in flight, and on a + // device that is sending nothing it just returns 0 bytes when its (500 ms) ReadTimeout + // elapses. The loop translated that into "transport stream closed" — a timeout — even when + // the caller had already cancelled, which is precisely the case where a consumer's stall + // watchdog wants to see its own cancellation come back. Cancellation has to win. + using var sourceStream = new TokenIgnoringSilentStream(); + using var destinationStream = new MemoryStream(); + var receiver = new SdCardFileReceiver(sourceStream); + using var cts = new CancellationTokenSource(); + await cts.CancelAsync(); + + await Assert.ThrowsAnyAsync( + () => receiver.ReceiveAsync( + destinationStream, "test.bin", + timeout: TimeSpan.FromMinutes(5), + cancellationToken: cts.Token)); + } + [Fact] public async Task ReceiveAsync_ProgressReporting_BytesReceivedIncreases() { @@ -376,6 +397,37 @@ public override void Flush() { } public override void Write(byte[] buffer, int offset, int count) => throw new NotSupportedException(); } + /// + /// A silent device: every read returns 0 bytes (a serial read timeout) and the cancellation + /// token is accepted and then ignored, exactly as System.IO.Ports' SerialStream behaves for a + /// read already in flight. + /// + private sealed class TokenIgnoringSilentStream : Stream + { + public override bool CanRead => true; + public override bool CanSeek => false; + public override bool CanWrite => false; + public override long Length => throw new NotSupportedException(); + public override long Position + { + get => throw new NotSupportedException(); + set => throw new NotSupportedException(); + } + + public override int Read(byte[] buffer, int offset, int count) => 0; + + public override Task ReadAsync(byte[] buffer, int offset, int count, CancellationToken cancellationToken) + { + // Deliberately no ThrowIfCancellationRequested: the token is accepted, never acted on. + return Task.FromResult(0); + } + + public override void Flush() { } + public override long Seek(long offset, SeekOrigin origin) => throw new NotSupportedException(); + public override void SetLength(long value) => throw new NotSupportedException(); + public override void Write(byte[] buffer, int offset, int count) => throw new NotSupportedException(); + } + /// /// A stream that blocks on read until cancellation is requested. /// Used to test cancellation behavior. diff --git a/src/Daqifi.Core.Tests/Device/SdCard/SdCardOperationsTests.cs b/src/Daqifi.Core.Tests/Device/SdCard/SdCardOperationsTests.cs index 17fab92..9e98ade 100644 --- a/src/Daqifi.Core.Tests/Device/SdCard/SdCardOperationsTests.cs +++ b/src/Daqifi.Core.Tests/Device/SdCard/SdCardOperationsTests.cs @@ -5,6 +5,7 @@ using Daqifi.Core.Firmware; using System; using System.Collections.Generic; +using System.Diagnostics; using System.IO; using System.Linq; using System.Net; @@ -1932,6 +1933,230 @@ public async Task DownloadSdCardFileAsync_MarkerOnlyThenSuccess_RetriesAndSuccee Assert.Equal(2, getCommands); } + [Fact] + public async Task DownloadSdCardFileAsync_WhenTransferParksIgnoringItsToken_ThrowsTimeoutException() + { + // #399: against a wedged SD subsystem the transfer parks somewhere that never looks + // at the token it was handed, so the only thing that can end the call is a deadline + // the download enforces itself. + var device = new ParkedDownloadDevice(ParkMode.Asynchronous, TimeSpan.FromMilliseconds(300)); + device.Connect(); + using var destinationStream = new MemoryStream(); + + try + { + var elapsed = Stopwatch.StartNew(); + var opTask = device.DownloadSdCardFileAsync("data.bin", destinationStream); + + // Guard the assertion rather than the test run: if the deadline never fires this + // fails instead of hanging CI forever. + var winner = await Task.WhenAny(opTask, Task.Delay(TimeSpan.FromSeconds(30))); + Assert.Same((Task)opTask, winner); + + var ex = await Assert.ThrowsAsync(() => opTask); + elapsed.Stop(); + + Assert.Contains("data.bin", ex.Message); + + // It waited for the budget instead of failing early — the deadline is what ended + // it, not some unrelated fault. + Assert.True( + elapsed.Elapsed >= TimeSpan.FromMilliseconds(300), + $"Download failed after only {elapsed.ElapsedMilliseconds}ms, before the budget elapsed."); + } + finally + { + device.Release(); + } + } + + [Fact] + public async Task DownloadSdCardFileAsync_WhenTransferParksSynchronously_StillTimesOut() + { + // The suspected park in #399 (a blocking SerialPort.Write on a device that stopped + // draining) is in the transfer's SYNCHRONOUS prefix — before the first await. Unless + // that prefix runs on the worker task, it blocks the caller before any deadline can + // even be armed, and the call never returns a Task at all. Hence the Task.Run: it + // keeps the test thread free so a regression fails here instead of wedging the run. + var device = new ParkedDownloadDevice(ParkMode.Synchronous, TimeSpan.FromMilliseconds(300)); + device.Connect(); + using var destinationStream = new MemoryStream(); + + try + { + var opTask = Task.Run(() => device.DownloadSdCardFileAsync("data.bin", destinationStream)); + + var winner = await Task.WhenAny(opTask, Task.Delay(TimeSpan.FromSeconds(30))); + Assert.Same((Task)opTask, winner); + + await Assert.ThrowsAsync(() => opTask); + } + finally + { + device.Release(); + } + } + + [Fact] + public async Task DownloadSdCardFileAsync_WhenTransferParksIgnoringItsToken_CallerCancellationStillEndsTheCall() + { + // The consumer-side stall watchdog described in #399: it cancels its token at 90s and + // nothing happens, because the parked path never polls it. The download must observe + // the caller's token itself so cancelling actually ends the await — well before the + // (deliberately distant) deadline below. + var device = new ParkedDownloadDevice(ParkMode.Asynchronous, TimeSpan.FromSeconds(60)); + device.Connect(); + using var destinationStream = new MemoryStream(); + using var cts = new CancellationTokenSource(); + + try + { + var opTask = device.DownloadSdCardFileAsync("data.bin", destinationStream, null, cts.Token); + + // Only cancel once the transfer is genuinely parked, so the test proves the token + // reaches a call already in flight rather than the up-front guard. + Assert.True(device.Parked.Wait(TimeSpan.FromSeconds(30)), "Transfer never reached the park."); + cts.Cancel(); + + var winner = await Task.WhenAny(opTask, Task.Delay(TimeSpan.FromSeconds(30))); + Assert.Same((Task)opTask, winner); + + await Assert.ThrowsAnyAsync(() => opTask); + } + finally + { + device.Release(); + } + } + + [Fact] + public async Task DownloadSdCardFileAsync_WhileAPreviousTransferIsStillAbandoned_FailsFast() + { + // An abandoned transfer still owns the transport, so a retry must be refused rather + // than start a second reader on the same stream (and stack another blocked thread). + // A consumer looping over a wedged card's files gets one timeout, then cheap failures. + var device = new ParkedDownloadDevice(ParkMode.Asynchronous, TimeSpan.FromMilliseconds(300)); + device.Connect(); + using var firstDestination = new MemoryStream(); + using var secondDestination = new MemoryStream(); + + try + { + await Assert.ThrowsAsync( + () => device.DownloadSdCardFileAsync("data.bin", firstDestination)); + + // The first transfer is still parked, so the gate is still held. The exception TYPE + // is what proves the refusal came from the gate: waiting out another deadline would + // have produced TimeoutException, never InvalidOperationException. The wall-clock + // bound is only a hang guard, kept loose so a contended CI runner can't fail it. + var secondCall = device.DownloadSdCardFileAsync("other.bin", secondDestination); + var winner = await Task.WhenAny(secondCall, Task.Delay(TimeSpan.FromSeconds(30))); + Assert.Same((Task)secondCall, winner); + + var ex = await Assert.ThrowsAsync(() => secondCall); + Assert.Contains("abandoned", ex.Message, StringComparison.OrdinalIgnoreCase); + } + finally + { + device.Release(); + } + } + + [Fact] + public async Task DownloadSdCardFileAsync_WhenGateIsHeldAndCallerCancelled_ReportsCancellation() + { + // Cancellation outranks the gate: a caller who cancelled should get their own + // cancellation back, not a report about a different transfer they can do nothing about. + var device = new ParkedDownloadDevice(ParkMode.Asynchronous, TimeSpan.FromMilliseconds(300)); + device.Connect(); + using var firstDestination = new MemoryStream(); + using var secondDestination = new MemoryStream(); + + try + { + await Assert.ThrowsAsync( + () => device.DownloadSdCardFileAsync("data.bin", firstDestination)); + + // Cancel from inside the pre-flight StopStreaming send: that lands in the exact + // window this guards — after DownloadSdCardFileAsync's entry check, before the + // gate is examined. A token cancelled before the call would just trip the entry + // check and prove nothing. + using var cts = new CancellationTokenSource(); + device.CancelOnNextSend = cts; + + await Assert.ThrowsAnyAsync( + () => device.DownloadSdCardFileAsync("other.bin", secondDestination, null, cts.Token)); + } + finally + { + device.Release(); + } + } + + [Fact] + public async Task DownloadSdCardFileAsync_AfterAnAbandonedTransferUnwinds_IsAllowedAgain() + { + // The gate is a quarantine, not a one-shot latch: once the abandoned worker finally + // returns, the transport is free and downloads are accepted again. + var device = new ParkedDownloadDevice(ParkMode.Asynchronous, TimeSpan.FromMilliseconds(300)); + device.Connect(); + using var destination = new MemoryStream(); + + await Assert.ThrowsAsync( + () => device.DownloadSdCardFileAsync("data.bin", destination)); + + // Let the abandoned worker unwind, then wait for the gate to come back. + device.Release(); + + InvalidOperationException? lastRefusal = null; + var deadline = DateTime.UtcNow + TimeSpan.FromSeconds(10); + while (DateTime.UtcNow < deadline) + { + try + { + // The park device completes its raw capture without ever calling the transfer + // body, so an accepted download reports a zero-byte result rather than throwing. + await device.DownloadSdCardFileAsync("data.bin", destination); + lastRefusal = null; + break; + } + catch (InvalidOperationException ex) + { + lastRefusal = ex; + await Task.Delay(25); + } + } + + Assert.Null(lastRefusal); + } + + [Fact] + public async Task DownloadSdCardFileAsync_SlowButHealthyTransfer_IsNotAborted() + { + // The bounding must only end transfers that are stuck. A slow-but-progressing one — + // a large file, a busy device — has to run to completion untouched. + var fileData = new byte[240]; + new Random(399).NextBytes(fileData); + + var device = new SlowDownloadDevice( + fileData, + chunkSize: 16, + chunkDelay: TimeSpan.FromMilliseconds(40), + budget: TimeSpan.FromSeconds(10)); + device.Connect(); + using var destinationStream = new MemoryStream(); + + // Act + var result = await device.DownloadSdCardFileAsync("data.bin", destinationStream); + + // Assert — every byte arrived, and the transfer really did take its time getting here. + Assert.Equal(fileData, destinationStream.ToArray()); + Assert.Equal(fileData.Length, result.FileSize); + Assert.True( + result.Duration >= TimeSpan.FromMilliseconds(200), + $"Transfer completed in {result.Duration.TotalMilliseconds}ms — too fast to prove a slow transfer survives."); + } + #endregion /// @@ -2309,6 +2534,180 @@ protected override async Task ExecuteRawCaptureAsync( } } + /// + /// How simulates a wedged transfer. + /// + private enum ParkMode + { + /// Parks in an awaited task that never observes the token (a read that never returns). + Asynchronous, + + /// Blocks the calling thread outright, before the first await (a blocking write that never drains). + Synchronous + } + + /// + /// A device whose raw-capture path parks and never observes the cancellation token it was + /// handed — the #399 failure mode, where neither the caller's token nor a consumer-side + /// watchdog can reach whatever the transfer is stuck in. unblocks the + /// abandoned worker at the end of the test so it cannot outlive it. + /// + private sealed class ParkedDownloadDevice : DaqifiStreamingDevice + { + private readonly TaskCompletionSource _asyncPark = new(TaskCreationOptions.RunContinuationsAsynchronously); + private readonly ManualResetEventSlim _syncPark = new(false); + private readonly ParkMode _mode; + private readonly TimeSpan _budget; + + public ParkedDownloadDevice(ParkMode mode, TimeSpan budget) + : base("TestDevice") + { + _mode = mode; + _budget = budget; + } + + /// Set once the transfer has actually reached the park. + public ManualResetEventSlim Parked { get; } = new(false); + + /// + /// When set, the next cancels it. Lets a test cancel from inside + /// the download's pre-flight command, i.e. between its entry guard and the SD-download + /// gate check. + /// + public CancellationTokenSource? CancelOnNextSend { get; set; } + + public override bool IsUsbConnection => true; + + internal override TimeSpan SdCardDownloadTimeout => _budget; + + public override void Send(IOutboundMessage message) + { + // Otherwise swallowed: this device never gets as far as exchanging commands. + var cancelSource = CancelOnNextSend; + CancelOnNextSend = null; + cancelSource?.Cancel(); + } + + protected override async Task ExecuteRawCaptureAsync( + Func rawAction, + CancellationToken cancellationToken = default) + { + Parked.Set(); + + // Deliberately ignores cancellationToken — that is the whole point. + if (_mode == ParkMode.Synchronous) + { + _syncPark.Wait(); + return; + } + + await _asyncPark.Task.ConfigureAwait(false); + } + + /// Lets the abandoned transfer finish so no thread is left parked after the test. + public void Release() + { + _asyncPark.TrySetResult(); + _syncPark.Set(); + } + } + + /// + /// A device that serves a healthy file slowly — small chunks with a delay between them — + /// so tests can show that bounding a stuck download does not abort a progressing one. + /// + private sealed class SlowDownloadDevice : DaqifiStreamingDevice + { + private readonly SlowChunkStream _stream; + private readonly TimeSpan _budget; + + public SlowDownloadDevice(byte[] fileData, int chunkSize, TimeSpan chunkDelay, TimeSpan budget) + : base("TestDevice") + { + _stream = new SlowChunkStream(fileData, chunkSize, chunkDelay); + _budget = budget; + } + + public override bool IsUsbConnection => true; + + internal override TimeSpan SdCardDownloadTimeout => _budget; + + public override void Send(IOutboundMessage message) + { + // Not asserted on by the slow-transfer tests. + } + + protected override async Task ExecuteRawCaptureAsync( + Func rawAction, + CancellationToken cancellationToken = default) + { + await rawAction(_stream, cancellationToken).ConfigureAwait(false); + } + } + + /// + /// Serves file data plus the EOF marker in small chunks, pausing between them, to imitate + /// a large transfer trickling in over USB. + /// + private sealed class SlowChunkStream : Stream + { + private static readonly byte[] EofMarker = Encoding.ASCII.GetBytes("__END_OF_FILE__"); + + private readonly byte[] _data; + private readonly int _chunkSize; + private readonly TimeSpan _chunkDelay; + private int _position; + + public SlowChunkStream(byte[] fileData, int chunkSize, TimeSpan chunkDelay) + { + _data = new byte[fileData.Length + EofMarker.Length]; + Array.Copy(fileData, 0, _data, 0, fileData.Length); + Array.Copy(EofMarker, 0, _data, fileData.Length, EofMarker.Length); + _chunkSize = chunkSize; + _chunkDelay = chunkDelay; + } + + public override bool CanRead => true; + public override bool CanSeek => false; + public override bool CanWrite => false; + public override long Length => _data.Length; + public override long Position + { + get => _position; + set => throw new NotSupportedException(); + } + + public override int Read(byte[] buffer, int offset, int count) + { + Thread.Sleep(_chunkDelay); + var available = _data.Length - _position; + if (available <= 0) return 0; + + var toRead = Math.Min(Math.Min(count, _chunkSize), available); + Array.Copy(_data, _position, buffer, offset, toRead); + _position += toRead; + return toRead; + } + + public override async Task ReadAsync(byte[] buffer, int offset, int count, CancellationToken cancellationToken) + { + await Task.Delay(_chunkDelay, cancellationToken).ConfigureAwait(false); + + var available = _data.Length - _position; + if (available <= 0) return 0; + + var toRead = Math.Min(Math.Min(count, _chunkSize), available); + Array.Copy(_data, _position, buffer, offset, toRead); + _position += toRead; + return toRead; + } + + public override void Flush() { } + public override long Seek(long offset, SeekOrigin origin) => throw new NotSupportedException(); + public override void SetLength(long value) => throw new NotSupportedException(); + public override void Write(byte[] buffer, int offset, int count) => throw new NotSupportedException(); + } + /// /// A testable device that reports IsUsbConnection = false to verify /// that SD card operations reject non-USB connections. diff --git a/src/Daqifi.Core/Communication/Transport/SerialStreamTransport.cs b/src/Daqifi.Core/Communication/Transport/SerialStreamTransport.cs index bdb868b..d381044 100644 --- a/src/Daqifi.Core/Communication/Transport/SerialStreamTransport.cs +++ b/src/Daqifi.Core/Communication/Transport/SerialStreamTransport.cs @@ -19,6 +19,26 @@ public class SerialStreamTransport : IStreamTransport private SerialPort? _serialPort; private bool _disposed; + /// + /// Blocking-read timeout applied once the port is open. The connection timeout is only + /// needed for retry/backoff logic, not for reads during normal operation; a short value + /// keeps consumer threads stoppable (StopSafely). + /// + private const int OperationalReadTimeoutMs = 500; + + /// + /// Blocking-write timeout applied once the port is open. Everything written over this + /// transport is a short SCPI command line, so a healthy device drains a write in + /// milliseconds. Only used to be lowered after open, + /// leaving writes bounded by the caller's + /// for the life of the port — a value chosen for retry/backoff, not for how long a command + /// write may legitimately take, and one a consumer can set arbitrarily high. Since + /// accepts no , + /// nothing else can shorten that wait on a device that has stopped draining its receive + /// buffer (#399). + /// + private const int OperationalWriteTimeoutMs = 2000; + /// /// Initializes a new instance of the SerialStreamTransport class. /// @@ -171,11 +191,9 @@ await ConnectRetryExecutor.ExecuteAsync( _serialPort.Open(); - // After a successful open, lower the ReadTimeout to a short operational - // value. The connection timeout is only needed for retry/backoff logic, - // not for blocking reads during normal operation. A short ReadTimeout - // ensures consumer threads can be stopped promptly (StopSafely). - _serialPort.ReadTimeout = 500; + // After a successful open, swap the connect timeouts for the (shorter) + // operational ones — both directions, not just reads (#399). + ApplyOperationalTimeouts(_serialPort); return Task.CompletedTask; }, @@ -187,6 +205,19 @@ await ConnectRetryExecutor.ExecuteAsync( onStatusChanged: OnStatusChanged); } + /// + /// Replaces the connect-phase timeouts with the operational ones on an opened port. + /// Internal (not inlined at the call site) so the values can be asserted without a real + /// port: / are + /// settable while the port is closed and are carried into the handle on open. + /// + /// The port to configure. + internal static void ApplyOperationalTimeouts(SerialPort port) + { + port.ReadTimeout = OperationalReadTimeoutMs; + port.WriteTimeout = OperationalWriteTimeoutMs; + } + /// /// Closes the serial connection asynchronously. /// diff --git a/src/Daqifi.Core/Device/DaqifiStreamingDevice.cs b/src/Daqifi.Core/Device/DaqifiStreamingDevice.cs index 625386f..17a0430 100644 --- a/src/Daqifi.Core/Device/DaqifiStreamingDevice.cs +++ b/src/Daqifi.Core/Device/DaqifiStreamingDevice.cs @@ -85,6 +85,23 @@ public class DaqifiStreamingDevice : DaqifiDevice, IStreamingDevice, INetworkCon private bool _isLoggingToSdCard; private IReadOnlyList _sdCardFiles = Array.Empty(); + /// + /// Admits one SD download at a time. A download that hits its deadline is ABANDONED, not + /// stopped — its worker can still be parked in native I/O holding the transport stream — + /// so the gate is released only when that worker actually finishes, however long that + /// takes. Without it, a caller retrying against a device that stays wedged (an "import + /// all" loop, say) would start a second reader on the same stream, which is the framing + /// corruption already refuses to risk when restarting the + /// protobuf consumer, and would stack another permanently blocked thread each time (#399). + /// + /// + /// Deliberately not disposed: we only ever call and + /// , never , + /// so there is no handle to release — and an abandoned worker may release this long after + /// the device is disposed, which would otherwise fault a continuation nobody observes. + /// + private readonly SemaphoreSlim _sdDownloadGate = new(1, 1); + /// /// Reconstructs host timestamps from the device's rolling 32-bit tick counter during a /// streaming session. Scoped to this device instance, so a single fixed key suffices. @@ -2224,6 +2241,31 @@ public Task FormatSdCardAsync(CancellationToken cancellationToken = default) /// retry attempts, indicating its SD subsystem is not ready rather than the file being /// legitimately empty. /// + /// + /// Thrown when the download does not finish within . + /// The deadline is enforced by this method itself, so it still applies when the transfer + /// is parked in a call that cannot observe a cancellation token (#399). + /// + /// Thrown when is canceled. + /// + /// On a timeout — or a cancellation the parked transfer cannot itself observe — the + /// in-flight transfer is abandoned rather than awaited: it may be blocked in native + /// serial I/O that no token can interrupt, and waiting for it is the hang this method + /// exists to bound. The abandoned transfer's token is cancelled first, so it unwinds at + /// its next token check — but that check is only reached once whatever it is blocked in + /// returns, which may be never. Two consequences for callers: it can still write to + /// after this method has thrown, so the stream must + /// not be reused for anything else; and the device is left mid-SD:GET with the + /// protobuf consumer stopped, so reconnecting (or power-cycling, if its SD subsystem is + /// genuinely wedged) is the reliable way to resume normal operation. + /// + /// Until an abandoned transfer unwinds it still owns the transport, so a further download + /// on the same device fails fast with rather than + /// putting a second reader on the same stream. A caller looping over many files against a + /// wedged card therefore gets one timeout and then immediate, cheap failures — not a + /// growing pile of blocked threads. + /// + /// public async Task DownloadSdCardFileAsync( string fileName, Stream destinationStream, @@ -2261,49 +2303,56 @@ public async Task DownloadSdCardFileAsync( var stopwatch = Stopwatch.StartNew(); long fileSize = 0; + var budget = SdCardDownloadTimeout; try { - await ExecuteRawCaptureAsync(async (stream, ct) => + await RunWithHardDeadlineAsync(async token => { - // Prepare SD card interface - PrepareSdInterface(); + await ExecuteRawCaptureAsync(async (stream, ct) => + { + // Prepare SD card interface + PrepareSdInterface(); - // Small delay to let the interface switch settle - await Task.Delay(50, ct).ConfigureAwait(false); + // Small delay to let the interface switch settle + await Task.Delay(50, ct).ConfigureAwait(false); - // Send the SCPI command to request the file - Send(ScpiMessageProducer.GetSdFile(fileName)); + // Send the SCPI command to request the file + Send(ScpiMessageProducer.GetSdFile(fileName)); - // Receive the file data. A marker-only (0-byte) transfer means the device's - // SD subsystem wasn't ready when it opened the file - the same kind of - // transient condition GetSdCardFilesAsync's LIST retry already absorbs - so - // retry the GET a bounded number of times before giving up (see #264). - var receiver = new SdCardFileReceiver(stream); - long bytesReceived; - var attempt = 0; - while (true) - { - try - { - bytesReceived = await receiver.ReceiveAsync( - destinationStream, - fileName, - progress, - timeout: TimeSpan.FromMinutes(30), - cancellationToken: ct).ConfigureAwait(false); - break; - } - catch (SdCardEmptyTransferException) when (attempt < SD_LIST_MAX_RETRIES) + // Receive the file data. A marker-only (0-byte) transfer means the device's + // SD subsystem wasn't ready when it opened the file - the same kind of + // transient condition GetSdCardFilesAsync's LIST retry already absorbs - so + // retry the GET a bounded number of times before giving up (see #264). + var receiver = new SdCardFileReceiver(stream); + long bytesReceived; + var attempt = 0; + while (true) { - attempt++; - await Task.Delay(SD_INTERFACE_SETTLE_DELAY_MS, ct).ConfigureAwait(false); - Send(ScpiMessageProducer.GetSdFile(fileName)); + try + { + // Each attempt gets what is left of the overall budget, never a + // fresh full one: retries must not be able to push the total past + // the deadline the caller was promised. + bytesReceived = await receiver.ReceiveAsync( + destinationStream, + fileName, + progress, + timeout: RemainingBudget(budget, stopwatch), + cancellationToken: ct).ConfigureAwait(false); + break; + } + catch (SdCardEmptyTransferException) when (attempt < SD_LIST_MAX_RETRIES) + { + attempt++; + await Task.Delay(SD_INTERFACE_SETTLE_DELAY_MS, ct).ConfigureAwait(false); + Send(ScpiMessageProducer.GetSdFile(fileName)); + } } - } - fileSize = bytesReceived; - }, cancellationToken).ConfigureAwait(false); + fileSize = bytesReceived; + }, token).ConfigureAwait(false); + }, budget, fileName, cancellationToken).ConfigureAwait(false); } finally { @@ -2364,6 +2413,188 @@ public async Task DownloadSdCardFileAsync( } } + /// + /// Overall wall-clock budget for one + /// + /// call, covering every GET attempt. This is the per-transfer limit the receiver has + /// always applied; what #399 changed is that the download now enforces it itself instead + /// of trusting whatever the transfer is parked in to notice a cancellation token. + /// + /// Virtual only as a test seam — a 30-minute budget is not unit-testable. + internal virtual TimeSpan SdCardDownloadTimeout => TimeSpan.FromMinutes(30); + + /// + /// The part of not yet consumed, floored at zero (a negative + /// timeout is not a legal delay). + /// + private static TimeSpan RemainingBudget(TimeSpan budget, Stopwatch stopwatch) + { + var remaining = budget - stopwatch.Elapsed; + return remaining > TimeSpan.Zero ? remaining : TimeSpan.Zero; + } + + /// + /// The instant the download is given up on regardless of what it is doing. It sits just + /// past the cooperative so that a transfer which IS observing + /// its token still fails through the receiver's own timeout — which reports how many + /// bytes arrived — and the hard deadline only decides the case where it is not. + /// + private static TimeSpan HardDeadlineFor(TimeSpan budget) + { + var graceMs = Math.Clamp(budget.TotalMilliseconds * 0.1, 100, 5000); + return budget + TimeSpan.FromMilliseconds(graceMs); + } + + /// + /// Runs an SD download on a worker task and races it against a hard deadline, so neither + /// the deadline nor the caller's cancellation depends on the transfer being somewhere it + /// can observe a token (#399). On expiry the worker is abandoned rather than awaited. + /// + /// The transfer. Receives a token cancelled by caller cancellation or the deadline, whichever comes first. + /// The cooperative budget; the hard deadline is of it. + /// Used only in the message. + /// The caller's token, observed by the race itself and not only by the worker. + /// + /// Thrown when a previous download still owns — it is either + /// genuinely in flight or was abandoned and is still parked on the transport. + /// + private async Task RunWithHardDeadlineAsync( + Func operation, + TimeSpan budget, + string fileName, + CancellationToken cancellationToken) + { + // Checked before taking the gate so a cancelled caller neither acquires it nor gets an + // answer about some other transfer. + cancellationToken.ThrowIfCancellationRequested(); + + // Fail fast rather than becoming a second reader on a stream an abandoned transfer + // still holds. Wait(0) never blocks: this either takes the gate or reports the state. + if (!_sdDownloadGate.Wait(0)) + { + // Cancellation wins when it raced the gate check — the same precedence the abandon + // path below applies. The caller asked to stop; that is a truer answer than a + // report about a different download. + cancellationToken.ThrowIfCancellationRequested(); + + throw new InvalidOperationException( + "A previous SD card download is still in flight, or was abandoned after timing out and " + + "is still parked on the transport. Reconnect the device before retrying."); + } + + // Released exactly once, by whichever path is last to be done with the worker: the + // finally below in the normal case, or the abandon-path continuation when the worker + // finally unwinds. Interlocked because a worker that completes right at the deadline + // boundary can reach both. + var gateReleased = 0; + void ReleaseGate() + { + if (Interlocked.Exchange(ref gateReleased, 1) != 0) + { + return; + } + + try + { + _sdDownloadGate.Release(); + } + catch (ObjectDisposedException) + { + // The device was disposed while a transfer was still abandoned. Benign + // teardown, and this can run from a discarded continuation — never throw. + } + } + + var hardDeadline = HardDeadlineFor(budget); + + // hardDeadlineCts runs on its own timer, independent of the Task.Delay race below, so + // it still reaches the worker if the worker only returns long after the race was + // decided. linkedCts is what the worker observes: caller cancellation OR the deadline. + var hardDeadlineCts = new CancellationTokenSource(hardDeadline); + var linkedCts = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken, hardDeadlineCts.Token); + + // Stops the racing delay the moment the outcome is decided — without it, a download + // that finishes in a second would leave a 30-minute timer registered behind it. + var raceCts = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken); + + // LongRunning (a dedicated thread, not a pooled one): the transfer's synchronous + // prefix — the consumer stop-and-join, PrepareSdInterface's blocking writes — otherwise + // runs on the CALLING thread up to the first await, which on a UI thread means a + // wedged device freezes the window, and which would put that prefix outside the very + // deadline it needs to be inside. A pooled Task.Run would also tie up a worker for the + // transfer's full blocking duration. Pass CancellationToken.None to StartNew itself: + // the worker's own token still cancels its waits, and "cancelled before start" must + // not surface as an operation fault. (Mirrors WifiBridgeActivator, #294/#295/#326.) + var workerTask = Task.Factory.StartNew( + () => operation(linkedCts.Token), + CancellationToken.None, + TaskCreationOptions.LongRunning, + TaskScheduler.Default).Unwrap(); + + try + { + var winner = await Task.WhenAny( + workerTask, + Task.Delay(hardDeadline, raceCts.Token)).ConfigureAwait(false); + + // Only abandon when the worker is genuinely still running: WhenAny can hand back + // the delay even though the worker completed at that same boundary, and awaiting + // it below honors that result instead of discarding it. + if (winner != workerTask && !workerTask.IsCompleted) + { + // Cancel explicitly instead of relying on the deadline timer having fired: the + // delay above and hardDeadlineCts are two separate timers of the same duration, + // so the delay can win by a hair and leave a late-returning worker running one + // more state-changing step after the caller already threw. Idempotent. + hardDeadlineCts.Cancel(); + + // The worker may be parked in native serial I/O that no token can interrupt, so + // it is ABANDONED, not awaited — waiting for it is the hang being bounded here. + // Observe its eventual fault so it cannot resurface as an UnobservedTaskException, + // and dispose the sources only once it is done with them (disposing early would + // turn its pending waits into ObjectDisposedException instead of cancellation). + _ = workerTask.ContinueWith( + t => + { + _ = t.Exception; + linkedCts.Dispose(); + hardDeadlineCts.Dispose(); + + // Only now is the transport genuinely free for another download. + ReleaseGate(); + }, + CancellationToken.None, + TaskContinuationOptions.ExecuteSynchronously, + TaskScheduler.Default); + + // Prefer surfacing caller cancellation over a generic timeout when both raced. + cancellationToken.ThrowIfCancellationRequested(); + + throw new TimeoutException( + $"SD card download of '{fileName}' did not complete within " + + $"{hardDeadline.TotalSeconds:0.#}s and was abandoned. The device's SD " + + "subsystem is not responding; reconnect (or power-cycle) before retrying."); + } + + // Propagate success or the transfer's own exception unchanged. + await workerTask.ConfigureAwait(false); + } + finally + { + raceCts.Cancel(); + raceCts.Dispose(); + + // The abandon path hands disposal and the gate to its continuation instead; do it + // here only when the worker actually finished (the common, non-hung case). + if (workerTask.IsCompleted) + { + linkedCts.Dispose(); + hardDeadlineCts.Dispose(); + ReleaseGate(); + } + } + } + /// /// Checks whether any line in the response contains a SCPI error indicator. /// These errors (e.g., "**ERROR: -200") can occur transiently when the device diff --git a/src/Daqifi.Core/Device/SdCard/ISdCardOperations.cs b/src/Daqifi.Core/Device/SdCard/ISdCardOperations.cs index 884a04a..788e260 100644 --- a/src/Daqifi.Core/Device/SdCard/ISdCardOperations.cs +++ b/src/Daqifi.Core/Device/SdCard/ISdCardOperations.cs @@ -183,6 +183,12 @@ Task CheckSdCardSpaceAsync( /// Metadata about the downloaded file. /// Thrown when the device is not connected or is not using a USB/serial transport. /// Thrown when the filename is null, empty, or contains invalid characters. + /// + /// Thrown when the transfer does not finish within the implementation's download deadline. + /// The deadline is enforced by the download itself, so it holds even when the transfer is + /// parked in a call that cannot observe a cancellation token; the in-flight transfer is + /// then abandoned rather than awaited (#399). + /// Task DownloadSdCardFileAsync( string fileName, Stream destinationStream, @@ -198,6 +204,10 @@ Task DownloadSdCardFileAsync( /// Metadata about the downloaded file, including the local . /// Thrown when the device is not connected or is not using a USB/serial transport. /// Thrown when the filename is null, empty, or contains invalid characters. + /// + /// Thrown when the transfer does not finish within the implementation's download deadline + /// (see the overload above); the temporary file is removed. + /// Task DownloadSdCardFileAsync( string fileName, IProgress? progress = null, diff --git a/src/Daqifi.Core/Device/SdCard/SdCardFileReceiver.cs b/src/Daqifi.Core/Device/SdCard/SdCardFileReceiver.cs index 4c0084d..3326c48 100644 --- a/src/Daqifi.Core/Device/SdCard/SdCardFileReceiver.cs +++ b/src/Daqifi.Core/Device/SdCard/SdCardFileReceiver.cs @@ -99,6 +99,14 @@ public async Task ReceiveAsync( $"Received {totalBytesReceived} bytes before timeout."); } + // A read can complete without ever having observed the token: System.IO.Ports' + // SerialStream ignores the CancellationToken once a read is in flight, so a + // cancellation requested mid-read is only visible here. Checking it every + // iteration is what makes the accepted token actually honored rather than + // merely accepted (#399) — including on the zero-byte path below, which would + // otherwise report a cancelled transfer as a timeout. + token.ThrowIfCancellationRequested(); + if (bytesRead == 0) { // Stream ended without EOF marker — treat as timeout/error