diff --git a/src/Daqifi.Core.Tests/Device/Capabilities/DaqifiDeviceCapabilityDocumentTests.cs b/src/Daqifi.Core.Tests/Device/Capabilities/DaqifiDeviceCapabilityDocumentTests.cs index 918f9644..af71d37d 100644 --- a/src/Daqifi.Core.Tests/Device/Capabilities/DaqifiDeviceCapabilityDocumentTests.cs +++ b/src/Daqifi.Core.Tests/Device/Capabilities/DaqifiDeviceCapabilityDocumentTests.cs @@ -244,20 +244,33 @@ protected override async Task> ExecuteTextCommandAsync( int responseTimeoutMs = 1000, int completionTimeoutMs = 250, CancellationToken cancellationToken = default, - Func? prepareAsync = null) + Func? prepareAsync = null, + Func? finalizeAsync = null) { - cancellationToken.ThrowIfCancellationRequested(); - - // Honor the exchange's prepare phase the way the real device does: it runs first, - // before anything this exchange sends (#396). - if (prepareAsync != null) + try { - await prepareAsync(cancellationToken).ConfigureAwait(false); + cancellationToken.ThrowIfCancellationRequested(); + + // Honor the exchange's prepare phase the way the real device does: it runs first, + // before anything this exchange sends (#396). + if (prepareAsync != null) + { + await prepareAsync(cancellationToken).ConfigureAwait(false); + } + + var before = SentCommands.Count; + setupAction(); + return ResponsesSince(before); + } + finally + { + // Honor the exchange's finalize phase the way the real device does: it runs + // however the exchange ended, still inside the exchange (#407). + if (finalizeAsync != null) + { + await finalizeAsync().ConfigureAwait(false); + } } - - var before = SentCommands.Count; - setupAction(); - return ResponsesSince(before); } protected override async Task> ExecuteTextCommandAsync( diff --git a/src/Daqifi.Core.Tests/Device/DaqifiDeviceDrainErrorQueueTests.cs b/src/Daqifi.Core.Tests/Device/DaqifiDeviceDrainErrorQueueTests.cs index 847fea4f..d106e521 100644 --- a/src/Daqifi.Core.Tests/Device/DaqifiDeviceDrainErrorQueueTests.cs +++ b/src/Daqifi.Core.Tests/Device/DaqifiDeviceDrainErrorQueueTests.cs @@ -208,20 +208,33 @@ protected override async Task> ExecuteTextCommandAsync( int responseTimeoutMs = 1000, int completionTimeoutMs = 250, CancellationToken cancellationToken = default, - Func? prepareAsync = null) + Func? prepareAsync = null, + Func? finalizeAsync = null) { - // Honor the exchange's prepare phase the way the real device does: it runs first, - // before anything this exchange sends (#396). - if (prepareAsync != null) + try { - await prepareAsync(cancellationToken).ConfigureAwait(false); + // Honor the exchange's prepare phase the way the real device does: it runs first, + // before anything this exchange sends (#396). + if (prepareAsync != null) + { + await prepareAsync(cancellationToken).ConfigureAwait(false); + } + + cancellationToken.ThrowIfCancellationRequested(); + setupAction(); + ExecuteTextCommandCallCount++; + var reply = Replies.Count > 0 ? Replies.Dequeue() : Array.Empty(); + return reply; + } + finally + { + // Honor the exchange's finalize phase the way the real device does: it runs + // however the exchange ended, still inside the exchange (#407). + if (finalizeAsync != null) + { + await finalizeAsync().ConfigureAwait(false); + } } - - cancellationToken.ThrowIfCancellationRequested(); - setupAction(); - ExecuteTextCommandCallCount++; - var reply = Replies.Count > 0 ? Replies.Dequeue() : Array.Empty(); - return reply; } } } diff --git a/src/Daqifi.Core.Tests/Device/DaqifiDeviceInitializeTests.cs b/src/Daqifi.Core.Tests/Device/DaqifiDeviceInitializeTests.cs index 01192596..56e4d0b0 100644 --- a/src/Daqifi.Core.Tests/Device/DaqifiDeviceInitializeTests.cs +++ b/src/Daqifi.Core.Tests/Device/DaqifiDeviceInitializeTests.cs @@ -433,25 +433,38 @@ protected override async Task> ExecuteTextCommandAsync( int responseTimeoutMs = 1000, int completionTimeoutMs = 250, CancellationToken cancellationToken = default, - Func? prepareAsync = null) + Func? prepareAsync = null, + Func? finalizeAsync = null) { - // Honor the exchange's prepare phase the way the real device does: it runs first, - // before anything this exchange sends (#396). - if (prepareAsync != null) + try { - await prepareAsync(cancellationToken).ConfigureAwait(false); - } + // Honor the exchange's prepare phase the way the real device does: it runs first, + // before anything this exchange sends (#396). + if (prepareAsync != null) + { + await prepareAsync(cancellationToken).ConfigureAwait(false); + } - // Run the setup action so that Send() calls inside it are captured - setupAction(); - TextCommandAttemptCount++; + // Run the setup action so that Send() calls inside it are captured + setupAction(); + TextCommandAttemptCount++; - if (_failFirstAttempt && TextCommandAttemptCount == 1) + if (_failFirstAttempt && TextCommandAttemptCount == 1) + { + return new[] { "**ERROR: -200, \"Execution error\"\r\n" }; + } + + return _textCommandResponse; + } + finally { - return new[] { "**ERROR: -200, \"Execution error\"\r\n" }; + // Honor the exchange's finalize phase the way the real device does: it runs + // however the exchange ended, still inside the exchange (#407). + if (finalizeAsync != null) + { + await finalizeAsync().ConfigureAwait(false); + } } - - return _textCommandResponse; } } @@ -513,40 +526,53 @@ protected override async Task> ExecuteTextCommandAsync( int responseTimeoutMs = 1000, int completionTimeoutMs = 250, CancellationToken cancellationToken = default, - Func? prepareAsync = null) + Func? prepareAsync = null, + Func? finalizeAsync = null) { - // Honor the exchange's prepare phase the way the real device does: it runs first, - // before anything this exchange sends (#396). - if (prepareAsync != null) + try { - await prepareAsync(cancellationToken).ConfigureAwait(false); - } + // Honor the exchange's prepare phase the way the real device does: it runs first, + // before anything this exchange sends (#396). + if (prepareAsync != null) + { + await prepareAsync(cancellationToken).ConfigureAwait(false); + } - var before = _sent.Count; - setupAction(); - var sentThisCall = _sent.Skip(before).ToList(); - var isUsbStep = sentThisCall.Any(d => d.Contains("STReam:INTerface")); + var before = _sent.Count; + setupAction(); + var sentThisCall = _sent.Skip(before).ToList(); + var isUsbStep = sentThisCall.Any(d => d.Contains("STReam:INTerface")); - if (isUsbStep) - { - UsbStepAttemptCount++; - - switch (_usbStepBehavior) + if (isUsbStep) { - case UsbStepBehavior.ScpiError: - return new[] { "**ERROR: -200, \"Execution error\"\r\n" }; - case UsbStepBehavior.ScpiErrorThenSucceed: - if (UsbStepAttemptCount == 1) - { + UsbStepAttemptCount++; + + switch (_usbStepBehavior) + { + case UsbStepBehavior.ScpiError: return new[] { "**ERROR: -200, \"Execution error\"\r\n" }; - } - break; - case UsbStepBehavior.Cancel: - throw new OperationCanceledException(); + case UsbStepBehavior.ScpiErrorThenSucceed: + if (UsbStepAttemptCount == 1) + { + return new[] { "**ERROR: -200, \"Execution error\"\r\n" }; + } + break; + case UsbStepBehavior.Cancel: + throw new OperationCanceledException(); + } } - } - return Array.Empty(); + return Array.Empty(); + } + finally + { + // Honor the exchange's finalize phase the way the real device does: it runs + // however the exchange ended, still inside the exchange (#407). + if (finalizeAsync != null) + { + await finalizeAsync().ConfigureAwait(false); + } + } } } } diff --git a/src/Daqifi.Core.Tests/Device/DaqifiDeviceStaleTextLineTests.cs b/src/Daqifi.Core.Tests/Device/DaqifiDeviceStaleTextLineTests.cs index 010a98db..9a3cbf54 100644 --- a/src/Daqifi.Core.Tests/Device/DaqifiDeviceStaleTextLineTests.cs +++ b/src/Daqifi.Core.Tests/Device/DaqifiDeviceStaleTextLineTests.cs @@ -122,6 +122,230 @@ public async Task ExecuteTextCommand_CarryingAPreparePhase_IsStillCaughtByASubcl device.Disconnect(); } + // ── Finalize phase (#407) — the mirror of the prepare phase above. An exchange that + // switches shared device state on the way in has to switch it back before anything else + // runs, or only half the pairing is serialized. ───────────────────────────────────────── + + [Fact] + public async Task ExecuteTextCommandWithFinalize_RunsFinalizeAfterTheSetupAction() + { + using var transport = new ReleaseOnStreamAccessMockTransport("0,\"No error\"\r\n"); + using var device = new StaleLineTestableDevice("Finalized Device", transport); + + device.Connect(); + + var order = new List(); + await device.CallWithFinalizeAsync( + () => order.Add("setup"), + () => { order.Add("finalize"); return Task.CompletedTask; }, + prepareAsync: _ => { order.Add("prepare"); return Task.CompletedTask; }); + + Assert.Equal(new[] { "prepare", "setup", "finalize" }, order); + + device.Disconnect(); + } + + [Fact] + public async Task ExecuteTextCommandWithFinalize_RunsFinalizeInsideTheExchange() + { + // The property #407 is about: the finalize phase holds the same lock acquisition the + // prepare phase does, so nothing can run between this exchange's commands and the state + // it restores. Asserted through the exchange's own re-entrancy guard rather than by + // racing threads — a nested exchange started from the finalize must be refused, and if + // the restore were back outside the lock this would quietly succeed instead. + using var transport = new ReleaseOnStreamAccessMockTransport("0,\"No error\"\r\n"); + using var device = new StaleLineTestableDevice("Nested Finalize Device", transport); + + device.Connect(); + + var ex = await Assert.ThrowsAsync( + () => device.CallWithFinalizeAsync( + () => { }, + async () => await device.CallExecuteTextCommandAsync(() => { }))); + + Assert.Contains("not re-entrant", ex.Message); + + device.Disconnect(); + } + + [Fact] + public async Task ExecuteTextCommandWithFinalize_RunsFinalizeWhenTheExchangeThrows() + { + // The reason the finalize is a phase the exchange owns rather than "another prepare at + // the end": a failed exchange is exactly when the device most needs putting back. + using var transport = new ReleaseOnStreamAccessMockTransport("0,\"No error\"\r\n"); + using var device = new StaleLineTestableDevice("Failing Device", transport); + + device.Connect(); + + var finalized = false; + + var ex = await Assert.ThrowsAsync( + () => device.CallWithFinalizeAsync( + () => throw new InvalidTimeZoneException("the exchange failed"), + () => { finalized = true; return Task.CompletedTask; })); + + Assert.Equal("the exchange failed", ex.Message); + Assert.True(finalized, "The finalize phase did not run for a failed exchange."); + + device.Disconnect(); + } + + [Fact] + public async Task ExecuteTextCommandWithFinalize_WhenBothFail_SurfacesTheExchangeFailure() + { + // A cleanup failure must never hide the failure that caused the cleanup: the caller + // needs the original to diagnose anything at all. + using var transport = new ReleaseOnStreamAccessMockTransport("0,\"No error\"\r\n"); + using var device = new StaleLineTestableDevice("Doubly Failing Device", transport); + + device.Connect(); + + var ex = await Assert.ThrowsAsync( + () => device.CallWithFinalizeAsync( + () => throw new InvalidTimeZoneException("the exchange failed"), + () => throw new NotSupportedException("the restore failed too"))); + + Assert.Equal("the exchange failed", ex.Message); + + device.Disconnect(); + } + + [Fact] + public async Task ExecuteTextCommandWithFinalize_WhenOnlyTheFinalizeFails_SurfacesThatFailure() + { + // The complement, so "never throw from the finalize" isn't the rule: with nothing else + // unwinding, a failed restore is the only failure there is, and reporting success would + // hand the caller a device left in the prepared state. + using var transport = new ReleaseOnStreamAccessMockTransport("0,\"No error\"\r\n"); + using var device = new StaleLineTestableDevice("Failing Restore Device", transport); + + device.Connect(); + + var ex = await Assert.ThrowsAsync( + () => device.CallWithFinalizeAsync( + () => { }, + () => throw new NotSupportedException("the restore failed"))); + + Assert.Equal("the restore failed", ex.Message); + + device.Disconnect(); + } + + [Fact] + public async Task ExecuteTextCommandWithFinalize_WhenTheFinalizeFails_TheExchangeLockIsStillReleased() + { + // The finalize runs from the exchange's own finally, so a failure raised straight out of + // it would abandon the rest of that finally — the lock included — and every later exchange + // on the device would hang forever. Both outcomes are checked because they take different + // routes out: the restore failing alone, and the restore failing on top of a failed + // exchange. + using var transport = new ReleaseOnStreamAccessMockTransport("0,\"No error\"\r\n"); + using var device = new StaleLineTestableDevice("Leaky Restore Device", transport); + + device.Connect(); + + var restoreFailed = device.CallWithFinalizeAsync( + () => { }, + () => throw new NotSupportedException("the restore failed")); + await AssertCompletesAsync(restoreFailed); + await Assert.ThrowsAsync(() => restoreFailed); + + var bothFailed = device.CallWithFinalizeAsync( + () => throw new InvalidTimeZoneException("the exchange failed"), + () => throw new NotSupportedException("the restore failed too")); + await AssertCompletesAsync(bothFailed); + await Assert.ThrowsAsync(() => bothFailed); + + var next = device.CallExecuteTextCommandAsync(() => { }); + await AssertCompletesAsync(next); + await next; + + device.Disconnect(); + } + + /// + /// Waits for a call with a bound, so a leaked exchange lock fails the test that is looking for + /// it instead of hanging the whole run. + /// + private static async Task AssertCompletesAsync(Task call) + { + var winner = await Task.WhenAny(call, Task.Delay(TimeSpan.FromSeconds(15))); + Assert.Same(call, winner); + } + + [Fact] + public async Task ExecuteTextCommandWithFinalize_NoOtherExchangeRunsBeforeTheFinalize() + { + // The race from #407 stated directly: a competing exchange must not be able to run + // between one exchange's commands and its restore. The second call is launched as soon + // as the first has sent, and the first's finalize then dawdles — plenty of room for the + // second to slip in if the restore were outside the lock. + using var transport = new ReleaseOnStreamAccessMockTransport("0,\"No error\"\r\n"); + using var device = new StaleLineTestableDevice("Serialized Device", transport); + + device.Connect(); + + var order = new List(); + var gate = new object(); + void Record(string step) + { + lock (gate) + { + order.Add(step); + } + } + + using var firstHasSent = new ManualResetEventSlim(false); + + // The finalize dawdles on purpose. Recording a start and an end around the wait is what + // makes this a regression detector rather than a coincidence: with the restore outside + // the lock, the second exchange acquires it the moment the first exchange returns and its + // setup lands INSIDE that window. + var first = device.CallWithFinalizeAsync( + () => { Record("first.setup"); firstHasSent.Set(); }, + async () => + { + Record("first.finalize.start"); + await Task.Delay(300); + Record("first.finalize.end"); + }); + + Assert.True(firstHasSent.Wait(TimeSpan.FromSeconds(10)), "The first exchange never sent."); + + var second = Task.Run(() => device.CallExecuteTextCommandAsync(() => Record("second.setup"))); + + await Task.WhenAll(first, second); + + lock (gate) + { + Assert.Equal( + new[] { "first.setup", "first.finalize.start", "first.finalize.end", "second.setup" }, + order); + } + + device.Disconnect(); + } + + [Fact] + public async Task ExecuteTextCommandWithFinalize_WhenValidationRefusesTheExchange_DoesNotRunFinalize() + { + // The one case the finalize is skipped: the exchange never got past validation, so it + // never touched the device and there is nothing to put back. Running it here would only + // add a second failure on a device that is already gone. + using var transport = new ReleaseOnStreamAccessMockTransport("0,\"No error\"\r\n"); + using var device = new StaleLineTestableDevice("Unconnected Device", transport); + + var finalized = false; + + await Assert.ThrowsAsync( + () => device.CallWithFinalizeAsync( + () => { }, + () => { finalized = true; return Task.CompletedTask; })); + + Assert.False(finalized, "The finalize phase ran for an exchange that never started."); + } + /// /// Stands in for a downstream subclass or test double that intercepts the text exchange — /// the case the single-seam design protects. @@ -140,17 +364,30 @@ protected override async Task> ExecuteTextCommandAsync( int responseTimeoutMs = 1000, int completionTimeoutMs = 250, CancellationToken cancellationToken = default, - Func? prepareAsync = null) + Func? prepareAsync = null, + Func? finalizeAsync = null) { - Intercepted = true; + try + { + Intercepted = true; - if (prepareAsync != null) + if (prepareAsync != null) + { + await prepareAsync(cancellationToken).ConfigureAwait(false); + } + + setupAction(); + return new List { "from the override" }; + } + finally { - await prepareAsync(cancellationToken).ConfigureAwait(false); + // Honor the exchange's finalize phase the way the real device does: it runs + // however the exchange ended, still inside the exchange (#407). + if (finalizeAsync != null) + { + await finalizeAsync().ConfigureAwait(false); + } } - - setupAction(); - return new List { "from the override" }; } } @@ -177,6 +414,19 @@ public Task> CallWithPrepareAsync( completionTimeoutMs: 150, prepareAsync: prepareAsync); } + + public Task> CallWithFinalizeAsync( + Action setupAction, + Func finalizeAsync, + Func? prepareAsync = null) + { + return ExecuteTextCommandAsync( + setupAction, + responseTimeoutMs: 500, + completionTimeoutMs: 150, + prepareAsync: prepareAsync, + finalizeAsync: finalizeAsync); + } } /// diff --git a/src/Daqifi.Core.Tests/Device/Diagnostics/DeviceDiagnosticsTests.cs b/src/Daqifi.Core.Tests/Device/Diagnostics/DeviceDiagnosticsTests.cs index 40f72899..3033b6ab 100644 --- a/src/Daqifi.Core.Tests/Device/Diagnostics/DeviceDiagnosticsTests.cs +++ b/src/Daqifi.Core.Tests/Device/Diagnostics/DeviceDiagnosticsTests.cs @@ -307,18 +307,31 @@ protected override async Task> ExecuteTextCommandAsync( int responseTimeoutMs = 1000, int completionTimeoutMs = 250, CancellationToken cancellationToken = default, - Func? prepareAsync = null) + Func? prepareAsync = null, + Func? finalizeAsync = null) { - // Honor the exchange's prepare phase the way the real device does: it runs first, - // before anything this exchange sends (#396). - if (prepareAsync != null) + try { - await prepareAsync(cancellationToken).ConfigureAwait(false); + // Honor the exchange's prepare phase the way the real device does: it runs first, + // before anything this exchange sends (#396). + if (prepareAsync != null) + { + await prepareAsync(cancellationToken).ConfigureAwait(false); + } + + cancellationToken.ThrowIfCancellationRequested(); + setupAction(); + return CannedTextResponse.ToList(); + } + finally + { + // Honor the exchange's finalize phase the way the real device does: it runs + // however the exchange ended, still inside the exchange (#407). + if (finalizeAsync != null) + { + await finalizeAsync().ConfigureAwait(false); + } } - - cancellationToken.ThrowIfCancellationRequested(); - setupAction(); - return CannedTextResponse.ToList(); } protected override async Task> ExecuteTextCommandAsync( diff --git a/src/Daqifi.Core.Tests/Device/GetLanChipInfoAsyncTests.cs b/src/Daqifi.Core.Tests/Device/GetLanChipInfoAsyncTests.cs index d99c980f..8d779d77 100644 --- a/src/Daqifi.Core.Tests/Device/GetLanChipInfoAsyncTests.cs +++ b/src/Daqifi.Core.Tests/Device/GetLanChipInfoAsyncTests.cs @@ -95,18 +95,31 @@ protected override async Task> ExecuteTextCommandAsync( int responseTimeoutMs = 1000, int completionTimeoutMs = 250, CancellationToken cancellationToken = default, - Func? prepareAsync = null) + Func? prepareAsync = null, + Func? finalizeAsync = null) { - // Honor the exchange's prepare phase the way the real device does: it runs first, - // before anything this exchange sends (#396). - if (prepareAsync != null) + try { - await prepareAsync(cancellationToken).ConfigureAwait(false); + // Honor the exchange's prepare phase the way the real device does: it runs first, + // before anything this exchange sends (#396). + if (prepareAsync != null) + { + await prepareAsync(cancellationToken).ConfigureAwait(false); + } + + cancellationToken.ThrowIfCancellationRequested(); + setupAction(); + return CannedTextResponse.ToList(); + } + finally + { + // Honor the exchange's finalize phase the way the real device does: it runs + // however the exchange ended, still inside the exchange (#407). + if (finalizeAsync != null) + { + await finalizeAsync().ConfigureAwait(false); + } } - - cancellationToken.ThrowIfCancellationRequested(); - setupAction(); - return CannedTextResponse.ToList(); } protected override async Task> ExecuteTextCommandAsync( diff --git a/src/Daqifi.Core.Tests/Device/SdCard/SdCardOperationsTests.cs b/src/Daqifi.Core.Tests/Device/SdCard/SdCardOperationsTests.cs index c8481366..6e870ed9 100644 --- a/src/Daqifi.Core.Tests/Device/SdCard/SdCardOperationsTests.cs +++ b/src/Daqifi.Core.Tests/Device/SdCard/SdCardOperationsTests.cs @@ -256,6 +256,28 @@ public async Task GetSdCardFilesAsync_HonorsCancellationDuringSettleDelay() await Assert.ThrowsAnyAsync(() => opTask); } + [Fact] + public async Task GetSdCardFilesAsync_WhenCancelledDuringTheSettleDelay_StillRestoresTheLanInterface() + { + // By the time the settle wait is cancelled the prepare phase has already switched the + // bus, so the exchange unwinds with the device sitting on the SD card. The restore has + // to happen anyway — which is why it is a phase the exchange owns and runs from its own + // try/finally, rather than a step tacked on to the end of a successful exchange (#407). + var device = new TestableSdCardStreamingDevice("TestDevice"); + device.CannedTextResponse = new List { "Daqifi/test.bin" }; + device.Connect(); + + using var cts = new CancellationTokenSource(); + var opTask = device.GetSdCardFilesAsync(cts.Token); + cts.Cancel(); + + await Assert.ThrowsAnyAsync(() => opTask); + + var sentCommands = device.SentMessages.Select(m => m.Data).ToList(); + Assert.Contains("SYSTem:STORage:SD:ENAble 0", sentCommands); // DisableStorageSd + Assert.Contains("SYSTem:COMMunicate:LAN:ENAbled 1", sentCommands); // EnableNetworkLan + } + [Fact] public async Task DeleteSdCardFileAsync_HonorsCancellationDuringSettleDelay() { @@ -1649,6 +1671,41 @@ public async Task GetSdCardFilesAsync_OverUsb_TogglesLanInterface() Assert.Contains("SYSTem:COMMunicate:LAN:ENAbled 1", sent); // EnableNetworkLan (restore) } + [Fact] + public async Task SdCardTextOperations_HandTheLanRestoreToTheExchangeAsItsFinalizePhase() + { + // #407: the restore has to travel through the exchange's finalize phase, because that + // is what puts it under the same lock acquisition as the matching switch. This device + // drops the finalize phase on the floor — so if any restore still reaches the wire, it + // came from a caller-side finally running after the lock was already released, which is + // the defect. Every SD text operation is checked, since they share the pairing. + var device = new FinalizeDroppingSdCardDevice("TestDevice"); + device.CannedTextResponse = new List { "Daqifi/log.bin" }; + device.Connect(); + + await device.GetSdCardFilesAsync(); + await device.DeleteSdCardFileAsync("log.bin"); + + device.CannedTextResponse = new List { "1024,4096" }; + await device.GetSdCardStorageAsync(); + + var sent = device.SentMessages.Select(m => m.Data).ToList(); + + // The switch still happened — this is not a device that simply sent nothing. + Assert.Contains("SYSTem:COMMunicate:LAN:ENAbled 0", sent); // DisableNetworkLan + Assert.Contains("SYSTem:STORage:SD:ENAble 1", sent); // EnableStorageSd + + // And the restore did not, because the only route to it was the phase this device + // discarded. + Assert.DoesNotContain("SYSTem:COMMunicate:LAN:ENAbled 1", sent); // EnableNetworkLan + Assert.DoesNotContain("SYSTem:STORage:SD:ENAble 0", sent); // DisableStorageSd + + // Three operations, each of which offered the exchange a finalize phase (the listing + // and the delete send one exchange apiece here; storage the same). + Assert.Equal(3, device.ExchangesOfferedAFinalizePhase); + Assert.Equal(0, device.ExchangesWithoutAFinalizePhase); + } + [Theory] [InlineData("3.6.3")] [InlineData("3.5.0")] @@ -2167,6 +2224,54 @@ public async Task DownloadSdCardFileAsync_WhenTransferParksSynchronously_StillTi } } + [Fact] + public async Task DownloadSdCardFileAsync_WhenTheTransferIsAbandoned_DoesNotRestoreTheLanInterface() + { + // #407 / #399: an abandoned transfer is still running and still owns the transport — + // that is why the download gate stays held until it unwinds. Sending the LAN restore + // now would put commands onto a link that transfer is still reading from, on a device + // that has already stopped answering. The caller is told to reconnect or power-cycle, + // and both re-establish the interface anyway. + var device = new ParkedDownloadDevice(ParkMode.Asynchronous, TimeSpan.FromMilliseconds(300)); + device.Connect(); + using var destinationStream = new MemoryStream(); + + try + { + var opTask = 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); + + var sent = device.SentCommandsSnapshot(); + Assert.Contains("SYSTem:StopStreamData", sent); // the pre-flight stop did happen + Assert.DoesNotContain("SYSTem:STORage:SD:ENAble 0", sent); // DisableStorageSd + Assert.DoesNotContain("SYSTem:COMMunicate:LAN:ENAbled 1", sent); // EnableNetworkLan + } + finally + { + device.Release(); + } + } + + [Fact] + public async Task DownloadSdCardFileAsync_WhenTheTransferCompletes_StillRestoresTheLanInterface() + { + // The complement, so skipping the restore for an abandoned transfer cannot quietly + // become "the download never restores the interface". + var device = new TestableDownloadDevice("TestDevice"); + device.CannedFileData = Encoding.ASCII.GetBytes("hello sd card"); + device.Connect(); + using var destinationStream = new MemoryStream(); + + await device.DownloadSdCardFileAsync("data.bin", destinationStream); + + var sent = device.SentMessages.Select(m => m.Data).ToList(); + Assert.Contains("SYSTem:STORage:SD:ENAble 0", sent); // DisableStorageSd + Assert.Contains("SYSTem:COMMunicate:LAN:ENAbled 1", sent); // EnableNetworkLan + } + [Fact] public async Task DownloadSdCardFileAsync_WhenTransferParksIgnoringItsToken_CallerCancellationStillEndsTheCall() { @@ -2366,23 +2471,36 @@ protected override async Task> ExecuteTextCommandAsync( int responseTimeoutMs = 1000, int completionTimeoutMs = 250, CancellationToken cancellationToken = default, - Func? prepareAsync = null) + Func? prepareAsync = null, + Func? finalizeAsync = null) { - // Honor the exchange's prepare phase the way the real device does: it runs first, - // before anything this exchange sends (#396). - if (prepareAsync != null) + try { - await prepareAsync(cancellationToken).ConfigureAwait(false); - } + // Honor the exchange's prepare phase the way the real device does: it runs first, + // before anything this exchange sends (#396). + if (prepareAsync != null) + { + await prepareAsync(cancellationToken).ConfigureAwait(false); + } - var sentBefore = SentMessages.Count; - setupAction(); - ExecuteTextCommandCallCount++; - var response = ResponseSequence.Count > 0 - ? ResponseSequence.Dequeue() - : new List(); - return SdCardTestResponses.AnswerErrorQuery( - response, SentMessages, sentBefore, ExecuteTextCommandCallCount, UnterminatedAttempts); + var sentBefore = SentMessages.Count; + setupAction(); + ExecuteTextCommandCallCount++; + var response = ResponseSequence.Count > 0 + ? ResponseSequence.Dequeue() + : new List(); + return SdCardTestResponses.AnswerErrorQuery( + response, SentMessages, sentBefore, ExecuteTextCommandCallCount, UnterminatedAttempts); + } + finally + { + // Honor the exchange's finalize phase the way the real device does: it runs + // however the exchange ended, still inside the exchange (#407). + if (finalizeAsync != null) + { + await finalizeAsync().ConfigureAwait(false); + } + } } protected override async Task> ExecuteTextCommandAsync( @@ -2494,22 +2612,35 @@ protected override async Task> ExecuteTextCommandAsync( int responseTimeoutMs = 1000, int completionTimeoutMs = 250, CancellationToken cancellationToken = default, - Func? prepareAsync = null) + Func? prepareAsync = null, + Func? finalizeAsync = null) { - // Honor the exchange's prepare phase the way the real device does: it runs first, - // before anything this exchange sends (#396). - if (prepareAsync != null) + try { - await prepareAsync(cancellationToken).ConfigureAwait(false); - } + // Honor the exchange's prepare phase the way the real device does: it runs first, + // before anything this exchange sends (#396). + if (prepareAsync != null) + { + await prepareAsync(cancellationToken).ConfigureAwait(false); + } - var sentBefore = SentMessages.Count; + var sentBefore = SentMessages.Count; - // Execute the setup action so we can capture the SCPI commands - setupAction(); - _executeTextCommandCallCount++; - return SdCardTestResponses.AnswerErrorQuery( - CannedTextResponse, SentMessages, sentBefore, _executeTextCommandCallCount, UnterminatedAttempts); + // Execute the setup action so we can capture the SCPI commands + setupAction(); + _executeTextCommandCallCount++; + return SdCardTestResponses.AnswerErrorQuery( + CannedTextResponse, SentMessages, sentBefore, _executeTextCommandCallCount, UnterminatedAttempts); + } + finally + { + // Honor the exchange's finalize phase the way the real device does: it runs + // however the exchange ended, still inside the exchange (#407). + if (finalizeAsync != null) + { + await finalizeAsync().ConfigureAwait(false); + } + } } protected override async Task> ExecuteTextCommandAsync( @@ -2526,6 +2657,49 @@ protected override async Task> ExecuteTextCommandAsync( } } + /// + /// Records how many exchanges were handed a finalize phase and then discards it, so a test + /// can tell a restore that arrived through the exchange seam (#407) from one that arrived + /// from a caller's own finally after the lock was released. + /// + private sealed class FinalizeDroppingSdCardDevice : TestableSdCardStreamingDevice + { + public FinalizeDroppingSdCardDevice(string name, IPAddress? ipAddress = null) + : base(name, ipAddress) + { + } + + public int ExchangesOfferedAFinalizePhase { get; private set; } + + public int ExchangesWithoutAFinalizePhase { get; private set; } + + protected override Task> ExecuteTextCommandAsync( + Action setupAction, + int responseTimeoutMs = 1000, + int completionTimeoutMs = 250, + CancellationToken cancellationToken = default, + Func? prepareAsync = null, + Func? finalizeAsync = null) + { + if (finalizeAsync != null) + { + ExchangesOfferedAFinalizePhase++; + } + else + { + ExchangesWithoutAFinalizePhase++; + } + + return base.ExecuteTextCommandAsync( + setupAction, + responseTimeoutMs, + completionTimeoutMs, + cancellationToken, + prepareAsync, + finalizeAsync: null); + } + } + /// /// A testable version of DaqifiStreamingDevice that simulates a USB connection /// so DownloadSdCardFileAsync passes the USB transport check. @@ -2558,17 +2732,30 @@ protected override async Task> ExecuteTextCommandAsync( int responseTimeoutMs = 1000, int completionTimeoutMs = 250, CancellationToken cancellationToken = default, - Func? prepareAsync = null) + Func? prepareAsync = null, + Func? finalizeAsync = null) { - // Honor the exchange's prepare phase the way the real device does: it runs first, - // before anything this exchange sends (#396). - if (prepareAsync != null) + try { - await prepareAsync(cancellationToken).ConfigureAwait(false); - } + // Honor the exchange's prepare phase the way the real device does: it runs first, + // before anything this exchange sends (#396). + if (prepareAsync != null) + { + await prepareAsync(cancellationToken).ConfigureAwait(false); + } - setupAction(); - return CannedTextResponse; + setupAction(); + return CannedTextResponse; + } + finally + { + // Honor the exchange's finalize phase the way the real device does: it runs + // however the exchange ended, still inside the exchange (#407). + if (finalizeAsync != null) + { + await finalizeAsync().ConfigureAwait(false); + } + } } protected override async Task> ExecuteTextCommandAsync( @@ -2721,26 +2908,39 @@ protected override async Task> ExecuteTextCommandAsync( int responseTimeoutMs = 1000, int completionTimeoutMs = 250, CancellationToken cancellationToken = default, - Func? prepareAsync = null) + Func? prepareAsync = null, + Func? finalizeAsync = null) { - // Honor the exchange's prepare phase the way the real device does: it runs first, - // before anything this exchange sends (#396). - if (prepareAsync != null) + try { - await prepareAsync(cancellationToken).ConfigureAwait(false); - } + // Honor the exchange's prepare phase the way the real device does: it runs first, + // before anything this exchange sends (#396). + if (prepareAsync != null) + { + await prepareAsync(cancellationToken).ConfigureAwait(false); + } - var sentBefore = SentMessages.Count; + var sentBefore = SentMessages.Count; - setupAction(); - _executeTextCommandCallCount++; + setupAction(); + _executeTextCommandCallCount++; - // GetSdCardFilesAsync drives the listing through THIS overload (the SPI switch is - // the exchange's prepareAsync phase, #406), and terminates it with SYSTem:ERRor? - // (#396) — so the listing has to be served here and terminated the same way - // TestableSdCardStreamingDevice does, or Core reads it as incomplete. - return SdCardTestResponses.AnswerErrorQuery( - ListingLines, SentMessages, sentBefore, _executeTextCommandCallCount, UnterminatedAttempts); + // GetSdCardFilesAsync drives the listing through THIS overload (the SPI switch is + // the exchange's prepareAsync phase, #406), and terminates it with SYSTem:ERRor? + // (#396) — so the listing has to be served here and terminated the same way + // TestableSdCardStreamingDevice does, or Core reads it as incomplete. + return SdCardTestResponses.AnswerErrorQuery( + ListingLines, SentMessages, sentBefore, _executeTextCommandCallCount, UnterminatedAttempts); + } + finally + { + // Honor the exchange's finalize phase the way the real device does: it runs + // however the exchange ended, still inside the exchange (#407). + if (finalizeAsync != null) + { + await finalizeAsync().ConfigureAwait(false); + } + } } protected override async Task> ExecuteTextCommandAsync( @@ -2810,14 +3010,34 @@ public ParkedDownloadDevice(ParkMode mode, TimeSpan budget) internal override TimeSpan SdCardDownloadTimeout => _budget; + /// Commands this device was asked to send, in order. + public List SentCommands { get; } = new(); + public override void Send(IOutboundMessage message) { + if (message is IOutboundMessage stringMessage) + { + lock (SentCommands) + { + SentCommands.Add(stringMessage.Data); + } + } + // Otherwise swallowed: this device never gets as far as exchanging commands. var cancelSource = CancelOnNextSend; CancelOnNextSend = null; cancelSource?.Cancel(); } + /// Snapshot of , safe to read while the abandoned worker runs. + public IReadOnlyList SentCommandsSnapshot() + { + lock (SentCommands) + { + return SentCommands.ToList(); + } + } + protected override async Task ExecuteRawCaptureAsync( Func rawAction, CancellationToken cancellationToken = default) @@ -2990,19 +3210,32 @@ protected override async Task> ExecuteTextCommandAsync( int responseTimeoutMs = 1000, int completionTimeoutMs = 250, CancellationToken cancellationToken = default, - Func? prepareAsync = null) + Func? prepareAsync = null, + Func? finalizeAsync = null) { - // Honor the exchange's prepare phase the way the real device does: it runs first, - // before anything this exchange sends (#396). - if (prepareAsync != null) + try { - await prepareAsync(cancellationToken).ConfigureAwait(false); - } + // Honor the exchange's prepare phase the way the real device does: it runs first, + // before anything this exchange sends (#396). + if (prepareAsync != null) + { + await prepareAsync(cancellationToken).ConfigureAwait(false); + } - var sentBefore = SentMessages.Count; - setupAction(); - return SdCardTestResponses.AnswerErrorQuery( - CannedTextResponse, SentMessages, sentBefore, attemptNumber: 1, unterminatedAttempts: 0); + var sentBefore = SentMessages.Count; + setupAction(); + return SdCardTestResponses.AnswerErrorQuery( + CannedTextResponse, SentMessages, sentBefore, attemptNumber: 1, unterminatedAttempts: 0); + } + finally + { + // Honor the exchange's finalize phase the way the real device does: it runs + // however the exchange ended, still inside the exchange (#407). + if (finalizeAsync != null) + { + await finalizeAsync().ConfigureAwait(false); + } + } } protected override async Task> ExecuteTextCommandAsync( diff --git a/src/Daqifi.Core/Device/DaqifiDevice.cs b/src/Daqifi.Core/Device/DaqifiDevice.cs index 3e28d755..919541b2 100644 --- a/src/Daqifi.Core/Device/DaqifiDevice.cs +++ b/src/Daqifi.Core/Device/DaqifiDevice.cs @@ -14,6 +14,7 @@ using System.Globalization; using System.Linq; using System.Net; +using System.Runtime.ExceptionServices; using System.Threading; using System.Threading.Tasks; @@ -558,7 +559,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 @@ -807,6 +808,28 @@ private void RestartMessageConsumerAfterSwap() /// it did before this exchange began. /// /// + /// + /// Optional phase that undoes what established — the SD card + /// operations use it to hand the shared SPI bus back to the LAN interface. + /// + /// It is the mirror of the prepare phase and runs under the same lock acquisition, so + /// nothing can interleave between this exchange's commands and the state it restores (#407). + /// It runs after the protobuf consumer has been restarted, mirroring the prepare phase + /// running before the consumer was swapped out. + /// + /// + /// It runs whether the exchange succeeds or fails, so the device is never left in the + /// prepared state. It takes no cancellation token on purpose: it is cleanup, and a cancelled + /// or timed-out exchange still has to put the device back. Keep it short and non-blocking — + /// it holds the lock while it runs. + /// + /// + /// If the exchange failed and the finalize phase then fails too, the finalize failure is + /// logged and dropped and the exchange's original failure is what the caller sees — a + /// cleanup failure must never hide the failure that caused the cleanup. If the exchange + /// succeeded, a finalize failure is the only failure there is, and it propagates. + /// + /// /// /// A list of text lines received from the device. Lines that were already in flight when the /// exchange opened — late replies to earlier commands — are excluded: only what arrived once @@ -820,10 +843,10 @@ private void RestartMessageConsumerAfterSwap() /// Thrown when the device has no transport-based connection. /// Thrown when the underlying transport has dropped. /// Thrown when the operation is canceled. - // prepareAsync is added AFTER cancellationToken (technically violating CA1068 - // "CancellationToken should be last") to keep existing positional callers working, matching - // the convention established in IFirmwareUpdateService for the same reason. It is a - // parameter on this seam rather than a second virtual method deliberately: a parallel + // prepareAsync and finalizeAsync are added AFTER cancellationToken (technically violating + // CA1068 "CancellationToken should be last") to keep existing positional callers working, + // matching the convention established in IFirmwareUpdateService for the same reason. They + // are parameters on this seam rather than separate virtual methods deliberately: a parallel // method would be bypassed silently by any subclass that overrides only this one, which for // an instrumented device or a test double means the override quietly stops intercepting SD // operations with nothing to indicate it. Overriders must widen their signature — a compile @@ -834,10 +857,12 @@ protected virtual Task> ExecuteTextCommandAsync( int responseTimeoutMs = 1000, int completionTimeoutMs = 250, CancellationToken cancellationToken = default, - Func? prepareAsync = null) + Func? prepareAsync = null, + Func? finalizeAsync = null) { return ExecuteTextCommandCoreAsync( prepareAsync, + finalizeAsync, _ => { setupAction(); return Task.CompletedTask; }, responseTimeoutMs, completionTimeoutMs, @@ -846,7 +871,7 @@ protected virtual Task> ExecuteTextCommandAsync( #pragma warning restore CA1068 /// - /// Async overload of + /// 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. @@ -872,6 +897,7 @@ protected virtual Task> ExecuteTextCommandAsync( { return ExecuteTextCommandCoreAsync( prepareAsync: null, + finalizeAsync: null, setupActionAsync, responseTimeoutMs, completionTimeoutMs, @@ -880,6 +906,7 @@ protected virtual Task> ExecuteTextCommandAsync( private async Task> ExecuteTextCommandCoreAsync( Func? prepareAsync, + Func? finalizeAsync, Func setupActionAsync, int responseTimeoutMs, int completionTimeoutMs, @@ -924,6 +951,12 @@ private async Task> ExecuteTextCommandCoreAsync( } _isInsideTextExchange.Value = true; + + // Whether the exchange got past validation and so owes its finalize phase, and whether + // it is on its way out normally rather than with an exception unwinding. Both are read + // only by the finalize block in the outer finally below. + var exchangeStarted = false; + var completedNormally = false; try { // All validation runs INSIDE the lock so a competing thread @@ -961,6 +994,11 @@ private async Task> ExecuteTextCommandCoreAsync( "Device transport is no longer connected."); } + // Past validation: from here on the exchange acts on the device, so its finalize + // phase (if any) is owed however this ends — including a prepare phase that failed + // part-way and left the device half-way into the state it was establishing. + exchangeStarted = true; + var sw = Stopwatch.StartNew(); // Prepare phase, if any. Deliberately here: inside the lock, so no competing text @@ -1124,12 +1162,36 @@ private async Task> ExecuteTextCommandCoreAsync( // The text consumer is stopped by this point, so the list is no longer being // appended to concurrently and can be re-projected safely. - return staleLineCount > 0 + var result = staleLineCount > 0 ? collectedLines.Skip(staleLineCount).ToList() : collectedLines; + + completedNormally = true; + return result; } finally { + // Finalize phase, if any — the mirror of the prepare phase above, and deliberately + // still inside the lock: an exchange that switches shared device state on the way in + // has to switch it back before anything else can run, or the pairing is only half + // serialized (#407). It runs after the protobuf consumer has been restarted, just as + // the prepare phase ran before the consumer was swapped out. + // A failure here is never thrown from this point: doing so would abandon the rest of + // the finally, leaking the lock this exchange holds. It is held until after the + // release below and dealt with there. + Exception? finalizeFailure = null; + if (exchangeStarted && finalizeAsync != null) + { + try + { + await finalizeAsync().ConfigureAwait(false); + } + catch (Exception ex) + { + finalizeFailure = ex; + } + } + _isInsideTextExchange.Value = false; // Release can race with Dispose() — Dispose acquires the lock // before disposing it, but if that acquisition timed out and @@ -1144,6 +1206,29 @@ private async Task> ExecuteTextCommandCoreAsync( catch (ObjectDisposedException) { } + + if (finalizeFailure != null) + { + if (completedNormally) + { + // Nothing else is unwinding, so a failed restore is the only failure there + // is. Surface it rather than report a success the device never got back + // from — the caller's next command would run against the wrong state. + // Rethrown only now, with the lock already released, so a failed restore + // cannot also wedge the device. + ExceptionDispatchInfo.Capture(finalizeFailure).Throw(); + } + + // Otherwise an exception is already on its way to the caller, and it is the one + // that explains what went wrong. Replacing it with this one would lose the + // diagnosis, so the cleanup failure is logged instead: cleanup never hides the + // failure that caused the cleanup. + SafeLog(() => _logger.LogError( + finalizeFailure, + "The text exchange's finalize phase failed while another failure was already " + + "unwinding. The original failure is being surfaced to the caller; the device " + + "may be left in the state the prepare phase established.")); + } } } @@ -1166,7 +1251,7 @@ private async Task> ExecuteTextCommandCoreAsync( /// 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 d2fcaa7a..6b79a6f2 100644 --- a/src/Daqifi.Core/Device/DaqifiStreamingDevice.cs +++ b/src/Daqifi.Core/Device/DaqifiStreamingDevice.cs @@ -1698,9 +1698,9 @@ private void EnsureSdFileTransferSupportedOnTransport() /// /// The terminator is only meaningful if it cannot be confused with a late reply to an /// earlier command, so two things guard that boundary: the text exchange discards whatever - /// was already in flight when it opened, and this method does its SPI-bus switch and settle - /// delay before the exchange rather than inside it, leaving the exchange with no internal - /// gap for a stale reply to slip into. + /// was already in flight when it opened, and this method's SPI-bus switch and settle delay + /// run as the exchange's prepare phase, ahead of that boundary, leaving the exchange with no + /// internal gap for a stale reply to slip into. /// /// /// The terminator's error code is used only as a liveness marker, never for classification: @@ -1730,55 +1730,51 @@ public async Task> GetSdCardFilesAsync(Cancellatio IReadOnlyList lines = Array.Empty(); IReadOnlyList listing = Array.Empty(); var isComplete = false; - try + + // Attempt 0 plus SD_LIST_MAX_RETRIES retries. A SCPI error here is often a transient + // timing issue, and an unterminated response can be a one-off stall, so both are + // retried once after an additional settle delay before being surfaced. + for (var attempt = 0; attempt <= SD_LIST_MAX_RETRIES; attempt++) { - // Attempt 0 plus SD_LIST_MAX_RETRIES retries. A SCPI error here is often a transient - // timing issue, and an unterminated response can be a one-off stall, so both are - // retried once after an additional settle delay before being surfaced. - for (var attempt = 0; attempt <= SD_LIST_MAX_RETRIES; attempt++) + if (attempt > 0) { - if (attempt > 0) - { - cancellationToken.ThrowIfCancellationRequested(); + cancellationToken.ThrowIfCancellationRequested(); - await Task.Delay(SD_INTERFACE_SETTLE_DELAY_MS, cancellationToken).ConfigureAwait(false); - } + await Task.Delay(SD_INTERFACE_SETTLE_DELAY_MS, cancellationToken).ConfigureAwait(false); + } - // The SPI bus switch and its settle wait run as the exchange's prepare phase: - // inside the exchange lock, so a competing text exchange cannot restore the LAN - // interface between the switch and the LIST, and ahead of the stale-line - // boundary, so the settle wait does not become a window in which a late reply to - // an earlier command could pass for this listing's terminator. Querying the card - // too soon after the switch makes the device answer -200 (Execution error), so - // the wait itself is not optional. - lines = await ExecuteTextCommandAsync( - () => - { - Send(ScpiMessageProducer.GetSdFileList); + // The SPI bus switch and its settle wait run as the exchange's prepare phase, and + // the restore as its finalize phase: both inside the exchange lock, so a competing + // text exchange can neither restore the LAN interface between the switch and the + // LIST nor slip in between the LIST and the restore. The prepare also sits ahead of + // the stale-line boundary, so its settle wait does not become a window in which a + // late reply to an earlier command could pass for this listing's terminator. + // Querying the card too soon after the switch makes the device answer -200 + // (Execution error), so the wait itself is not optional. + // + // Each attempt therefore leaves the bus back on LAN, including across the retry + // delay above — the pairing is per exchange rather than per call so that gap, which + // is outside the lock, is not one in which the device sits switched to the card. + lines = await ExecuteTextCommandAsync( + () => + { + Send(ScpiMessageProducer.GetSdFileList); - // End-of-listing terminator — see this method's remarks. Sent inside - // the same text exchange so the ordering guarantee holds. - Send(ScpiMessageProducer.GetSystemError); - }, - responseTimeoutMs: 3000, - completionTimeoutMs: SD_LIST_COMPLETION_TIMEOUT_MS, - cancellationToken: cancellationToken, - prepareAsync: PrepareSdInterfaceAndSettleAsync); + // End-of-listing terminator — see this method's remarks. Sent inside + // the same text exchange so the ordering guarantee holds. + Send(ScpiMessageProducer.GetSystemError); + }, + responseTimeoutMs: 3000, + completionTimeoutMs: SD_LIST_COMPLETION_TIMEOUT_MS, + cancellationToken: cancellationToken, + prepareAsync: PrepareSdInterfaceAndSettleAsync, + finalizeAsync: RestoreLanInterfaceAsync); - isComplete = TrySplitAtSdListTerminator(lines, out listing); + isComplete = TrySplitAtSdListTerminator(lines, out listing); - if (isComplete && !ContainsScpiError(listing)) - { - break; - } - } - } - finally - { - // Restore LAN interface regardless of outcome - if (IsConnected) + if (isComplete && !ContainsScpiError(listing)) { - PrepareLanInterface(); + break; } } @@ -1800,7 +1796,7 @@ public async Task> GetSdCardFilesAsync(Cancellatio /// /// /// Passed as the prepareAsync phase of - /// + /// /// rather than run /// inline, so it executes inside the text-exchange lock — a competing exchange restoring the /// LAN interface between the switch and the commands that depend on it would leave them @@ -1816,6 +1812,33 @@ private async Task PrepareSdInterfaceAndSettleAsync(CancellationToken cancellati await Task.Delay(SD_INTERFACE_SETTLE_DELAY_MS, cancellationToken).ConfigureAwait(false); } + /// + /// Finalize phase shared by the SD card text exchanges: hands the shared SPI bus back to the + /// LAN interface. The mirror of . + /// + /// + /// Passed as the finalizeAsync phase of + /// + /// rather than run from the caller's own finally, so it holds the same lock + /// acquisition the matching prepare phase does. Restoring from outside the lock leaves a + /// window in which a competing exchange runs between this operation's commands and its + /// restore — the switch serialized, the restore not (#407). + /// + /// The connection check keeps a restore off a device that dropped mid-operation, where the + /// sends would only throw over the top of whatever + /// actually failed. Nothing to restore in that case: the link is gone. + /// + /// + private Task RestoreLanInterfaceAsync() + { + if (IsConnected) + { + PrepareLanInterface(); + } + + return Task.CompletedTask; + } + /// /// Splits a raw SD listing response at the SYSTem:ERRor? terminator reply that /// appends to the exchange. @@ -1913,53 +1936,42 @@ public async Task GetSdCardStorageAsync(CancellationToken can Send(ScpiMessageProducer.StopStreaming); IsStreaming = false; - IReadOnlyList lines; - try - { - lines = await ExecuteTextCommandAsync(() => + // Same prepare/finalize pairing as GetSdCardFilesAsync: the SPI bus switch and its + // settle wait are the exchange's prepare phase and the LAN restore is its finalize + // phase, so both halves are held by the one lock acquisition rather than only the + // switch (#407). The settle wait also moves ahead of the exchange's stale-line + // boundary instead of blocking a thread inside it. + var lines = await ExecuteTextCommandAsync( + () => Send(ScpiMessageProducer.GetSdSpace), + responseTimeoutMs: 3000, + cancellationToken: cancellationToken, + prepareAsync: PrepareSdInterfaceAndSettleAsync, + finalizeAsync: RestoreLanInterfaceAsync); + + // Only retry transient SCPI errors. A "No SD Card Detected" line + // is non-transient — retrying just delays the typed exception and + // risks misclassification if the marker isn't repeated on retry. + if (ContainsScpiError(lines) && !ContainsNoSdCardMarker(lines)) + { + for (var retry = 0; retry < SD_LIST_MAX_RETRIES; retry++) { - PrepareSdInterface(); + cancellationToken.ThrowIfCancellationRequested(); - // 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, cancellationToken).ConfigureAwait(false); - Send(ScpiMessageProducer.GetSdSpace); - }, responseTimeoutMs: 3000, cancellationToken: cancellationToken); + lines = await ExecuteTextCommandAsync( + () => Send(ScpiMessageProducer.GetSdSpace), + responseTimeoutMs: 3000, + cancellationToken: cancellationToken, + prepareAsync: PrepareSdInterfaceAndSettleAsync, + finalizeAsync: RestoreLanInterfaceAsync); - // Only retry transient SCPI errors. A "No SD Card Detected" line - // is non-transient — retrying just delays the typed exception and - // risks misclassification if the marker isn't repeated on retry. - if (ContainsScpiError(lines) && !ContainsNoSdCardMarker(lines)) - { - for (var retry = 0; retry < SD_LIST_MAX_RETRIES; retry++) + if (!ContainsScpiError(lines) || ContainsNoSdCardMarker(lines)) { - cancellationToken.ThrowIfCancellationRequested(); - - await Task.Delay(SD_INTERFACE_SETTLE_DELAY_MS, cancellationToken); - - lines = await ExecuteTextCommandAsync(() => - { - PrepareSdInterface(); - Thread.Sleep(SD_INTERFACE_SETTLE_DELAY_MS); - Send(ScpiMessageProducer.GetSdSpace); - }, responseTimeoutMs: 3000, cancellationToken: cancellationToken); - - if (!ContainsScpiError(lines) || ContainsNoSdCardMarker(lines)) - { - break; - } + break; } } } - finally - { - if (IsConnected) - { - PrepareLanInterface(); - } - } if (SdCardSpaceParser.TryParseLines(lines, out var storage)) { @@ -2221,56 +2233,49 @@ public async Task DeleteSdCardFileAsync(string fileName, CancellationToken cance Send(ScpiMessageProducer.StopStreaming); IsStreaming = false; - IReadOnlyList lines; - try - { - // Same prepare-phase treatment as GetSdCardFilesAsync, for the same two reasons — - // the SPI switch stays serialized against competing text exchanges, and its settle - // wait stays outside the stale-line boundary. The consequence of a stale line is - // milder here (delete keys off ContainsScpiError, so it would mean a pointless - // delete-and-relist retry rather than a bad listing) but it is the same defect. - lines = await ExecuteTextCommandAsync( - () => - { - Send(ScpiMessageProducer.DeleteSdFile(fileName)); - Send(ScpiMessageProducer.GetSdFileList); - }, - responseTimeoutMs: 3000, - cancellationToken: cancellationToken, - prepareAsync: PrepareSdInterfaceAndSettleAsync); + // Same prepare/finalize treatment as GetSdCardFilesAsync, for the same reasons — the + // SPI switch stays serialized against competing text exchanges, its settle wait stays + // outside the stale-line boundary, and the restore stays under the same lock as the + // switch instead of running from a finally after the lock has been dropped. The + // consequence of a stale line is milder here (delete keys off ContainsScpiError, so it + // would mean a pointless delete-and-relist retry rather than a bad listing) but it is + // the same defect. + var lines = await ExecuteTextCommandAsync( + () => + { + Send(ScpiMessageProducer.DeleteSdFile(fileName)); + Send(ScpiMessageProducer.GetSdFileList); + }, + responseTimeoutMs: 3000, + cancellationToken: cancellationToken, + prepareAsync: PrepareSdInterfaceAndSettleAsync, + finalizeAsync: RestoreLanInterfaceAsync); - if (ContainsScpiError(lines)) + if (ContainsScpiError(lines)) + { + for (var retry = 0; retry < SD_LIST_MAX_RETRIES; retry++) { - for (var retry = 0; retry < SD_LIST_MAX_RETRIES; retry++) - { - cancellationToken.ThrowIfCancellationRequested(); + cancellationToken.ThrowIfCancellationRequested(); - await Task.Delay(SD_INTERFACE_SETTLE_DELAY_MS, cancellationToken).ConfigureAwait(false); + await Task.Delay(SD_INTERFACE_SETTLE_DELAY_MS, cancellationToken).ConfigureAwait(false); - lines = await ExecuteTextCommandAsync( - () => - { - Send(ScpiMessageProducer.DeleteSdFile(fileName)); - Send(ScpiMessageProducer.GetSdFileList); - }, - responseTimeoutMs: 3000, - cancellationToken: cancellationToken, - prepareAsync: PrepareSdInterfaceAndSettleAsync); - - if (!ContainsScpiError(lines)) + lines = await ExecuteTextCommandAsync( + () => { - break; - } + Send(ScpiMessageProducer.DeleteSdFile(fileName)); + Send(ScpiMessageProducer.GetSdFileList); + }, + responseTimeoutMs: 3000, + cancellationToken: cancellationToken, + prepareAsync: PrepareSdInterfaceAndSettleAsync, + finalizeAsync: RestoreLanInterfaceAsync); + + if (!ContainsScpiError(lines)) + { + break; } } } - finally - { - if (IsConnected) - { - PrepareLanInterface(); - } - } _sdCardFiles = SdCardFileListParser.ParseFileList(lines); } @@ -2345,6 +2350,13 @@ public Task FormatSdCardAsync(CancellationToken cancellationToken = default) /// protobuf consumer stopped, so reconnecting (or power-cycling, if its SD subsystem is /// genuinely wedged) is the reliable way to resume normal operation. /// + /// The LAN interface is deliberately not restored in that case: the abandoned + /// transfer still owns the transport, and putting the restore commands onto a link it is + /// still reading would only add traffic to a device that has already stopped answering. The + /// reconnect the caller needs anyway re-establishes the interface. On every other outcome — + /// success, a stall, a cancellation the transfer did observe — the restore runs as before. + /// + /// /// 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 @@ -2391,6 +2403,10 @@ public async Task DownloadSdCardFileAsync( long fileSize = 0; var budget = SdCardDownloadTimeout; + // Set when the transfer was given up on and left running (#399/#401). Read only by the + // restore below, on this same async flow. + var workerAbandoned = false; + try { await RunWithHardDeadlineAsync(async token => @@ -2443,12 +2459,21 @@ await ExecuteRawCaptureAsync(async (stream, ct) => fileSize = bytesReceived; }, token).ConfigureAwait(false); - }, budget, fileName, cancellationToken).ConfigureAwait(false); + }, + budget, + fileName, + cancellationToken, + onWorkerAbandoned: () => workerAbandoned = true).ConfigureAwait(false); } finally { - // Restore LAN interface - if (IsConnected) + // Restore the LAN interface — but NOT when the transfer was abandoned. An abandoned + // worker is still alive and still owns the transport (that is why the download gate + // is not released until it finally unwinds), so sending the restore now would put + // SCPI commands onto a link a transfer is still reading, on top of a device that has + // already stopped answering. There is nothing to gain: the caller is told to + // reconnect or power-cycle, and both re-establish the interface anyway (#399/#401). + if (!workerAbandoned && IsConnected) { try { @@ -2584,6 +2609,11 @@ private static TimeSpan HardDeadlineFor(TimeSpan budget) /// 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. + /// + /// Invoked, before this method throws, when the worker is given up on while still running. + /// Lets the caller skip any cleanup that would touch the transport the abandoned worker + /// still owns. + /// /// /// Thrown when a previous download still owns — it is either /// genuinely in flight or was abandoned and is still parked on the transport. @@ -2592,7 +2622,8 @@ private async Task RunWithHardDeadlineAsync( Func operation, TimeSpan budget, string fileName, - CancellationToken cancellationToken) + CancellationToken cancellationToken, + Action? onWorkerAbandoned = null) { // Checked before taking the gate so a cancelled caller neither acquires it nor gets an // answer about some other transfer. @@ -2672,6 +2703,10 @@ void ReleaseGate() // it below honors that result instead of discarding it. if (winner != workerTask && !workerTask.IsCompleted) { + // Tell the caller before unwinding: the worker keeps running and keeps the + // transport, so any cleanup that would write to it has to be skipped. + onWorkerAbandoned?.Invoke(); + // 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 diff --git a/src/Daqifi.Core/Device/SdCard/ISdCardOperations.cs b/src/Daqifi.Core/Device/SdCard/ISdCardOperations.cs index 2e24dbd1..2d126a63 100644 --- a/src/Daqifi.Core/Device/SdCard/ISdCardOperations.cs +++ b/src/Daqifi.Core/Device/SdCard/ISdCardOperations.cs @@ -202,7 +202,9 @@ Task CheckSdCardSpaceAsync( /// 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). + /// then abandoned rather than awaited (#399). An abandoned transfer still owns the + /// transport, so the interface it switched is left as-is rather than restored over the top + /// of it — reconnect before using the device again. /// Task DownloadSdCardFileAsync( string fileName,