From 2cb61fb3d03010500682e7303d25524c0c0526ba Mon Sep 17 00:00:00 2001 From: Tyler Kron Date: Fri, 31 Jul 2026 13:51:07 -0600 Subject: [PATCH] fix(sdcard): run the SD->LAN restore as a finalize phase inside the exchange lock The SD operations switched the shared SPI bus to the card inside the text-exchange lock (the prepare phase added in #406) but restored it to LAN from each method's own finally, after the lock had been released. The switch was serialized; the matching restore was not, so a competing exchange could run between an SD command and its restore, or observe the bus mid-restore. ExecuteTextCommandAsync's Action overload gains a symmetric finalizeAsync phase. It runs under the same lock acquisition as the prepare phase, after the protobuf consumer has been restarted, and the exchange owns a try/finally around it so it runs however the exchange ended. If the exchange failed and the finalize fails too, the finalize failure is logged and the exchange's original failure is what the caller sees. If the exchange succeeded, the finalize failure is the only failure and it propagates - but only after the lock has been released, so a failed restore cannot also wedge the device. GetSdCardFilesAsync, DeleteSdCardFileAsync and GetSdCardStorageAsync now pass the restore as that phase; the storage query's switch also moves from its setup action into the prepare phase, matching its siblings and dropping a blocking Thread.Sleep. DownloadSdCardFileAsync runs on the raw-capture path, not the exchange. There the restore is now skipped when the transfer was abandoned on its deadline: the abandoned worker is still alive and still owns the transport, so the restore would write onto a link it is still reading (#399/#401). Closes #407. Co-Authored-By: Claude Opus 5 --- .../DaqifiDeviceCapabilityDocumentTests.cs | 35 +- .../DaqifiDeviceDrainErrorQueueTests.cs | 35 +- .../Device/DaqifiDeviceInitializeTests.cs | 104 ++++-- .../Device/DaqifiDeviceStaleTextLineTests.cs | 264 ++++++++++++- .../Diagnostics/DeviceDiagnosticsTests.cs | 31 +- .../Device/GetLanChipInfoAsyncTests.cs | 31 +- .../Device/SdCard/SdCardOperationsTests.cs | 351 +++++++++++++++--- src/Daqifi.Core/Device/DaqifiDevice.cs | 103 ++++- .../Device/DaqifiStreamingDevice.cs | 295 ++++++++------- .../Device/SdCard/ISdCardOperations.cs | 4 +- 10 files changed, 968 insertions(+), 285 deletions(-) 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,