fix(sdcard): run the SD→LAN restore inside the exchange lock, not after it (closes #407)

[tylerkron]

Why

The SD card operations turn a switch on the way in and turn it back off on the way out. The DAQiFi hardware shares one SPI bus between the SD card and the network interface, so before Core can talk to the card it has to hand the bus over, and afterwards it has to hand it back.

#406 made the hand-over safe: it now happens while the operation holds the device's text-command lock, so nothing else can run in the middle of it. The hand-back was left where it was — in each method's own cleanup block, which runs after the lock has already been let go. So half the pair was protected and half was not, and another command could slip in between an SD operation and its own hand-back, or run while it was happening.

What

The text exchange gained a matching "finalize" step, so the hand-back now runs under the same lock as the hand-over. The listing, the delete, and the storage-space query all use it. Nothing about the commands sent to the device changes on the normal path.

Two decisions worth stating plainly:

• If the restore fails while something else has already gone wrong, the original problem is what you see. The restore failure is written to the log and dropped, because a cleanup failure that hides the real failure leaves you with nothing to diagnose. If the operation itself went fine, a failed restore is the only thing that went wrong, so it is reported normally.
• A download that was given up on does not get a restore. When a download blows through its deadline, Core stops waiting for it but the transfer is still running and still holding the connection (reliability: SD download against a wedged card hangs indefinitely and ignores cancellation — no consumer-side watchdog can rescue it #399/fix(sdcard): bound the SD download path so a wedged card cannot hang or ignore cancellation (closes #399) #401). Sending it more commands would just add traffic to a device that has already stopped answering, and the caller is told to reconnect anyway — which puts the interface right. Every other outcome still restores as before.

How

ExecuteTextCommandAsync takes an optional finalize step alongside the prepare step added in #406. The exchange wraps the caller's work in a try/finally around it, so the finalize runs whether the work succeeded or blew up, and always before the lock is released. Anything the finalize throws is held until after the lock is released and only then reported, so a bad restore can't also wedge the device — that one bit me while writing this, and there's a test for it.

Subclasses that override ExecuteTextCommandAsync have to widen their signature by one parameter, exactly as in #406, and for the same reason: a compile error beats an override that quietly stops seeing SD traffic.

Side effects worth knowing about:

• The storage-space query's bus switch moved into the prepare step to match its siblings, which also removes a blocking sleep from an async path. Same commands, same order.
• On a retry, the bus is now handed back and re-taken between attempts instead of staying on the card across the gap. That gap is outside the lock, so leaving the device switched over during it was the same defect in miniature. The device already sees this exact enable/disable pattern between any two back-to-back SD calls, so it is not a new sequence for the firmware — but it is a real change on the retry path, and the retry path is rare enough that the bench cannot exercise it on demand.

Tests

Full suite green on net9.0 and net10.0 (2203 passed, 2 skipped each), plus the MCP project (23), build clean with 0 warnings.

New coverage, each checked to fail with its fix disabled:

• the restore runs inside the exchange (proved through the exchange's own re-entrancy guard, so it is deterministic rather than a thread race);
• no competing exchange can run between an operation's commands and its restore (the restore deliberately dawdles, so a competitor would land inside the window);
• the restore still runs when the exchange throws — at the exchange level and end-to-end through GetSdCardFilesAsync;
• a failed restore does not replace a failure already unwinding, and does surface when it is the only failure;
• a failed restore still releases the lock (this one hung the run before the fix, which is how I found it);
• the SD operations hand the restore to the exchange rather than doing it themselves — a device that discards the finalize step sees no restore commands at all;
• an abandoned download sends no restore, and a completed one still does.

Bench

DAQiFi Nyquist 1, firmware 3.7.2, example CLI built against this branch. Nothing destructive — no format, delete, download, flash, reboot or WiFi reconfiguration.

• USB --sd-list: 32 files. --sd-storage: 7.44 GiB total, parsed fine — that is the query whose prepare step moved.
• USB streaming right after the SD work: 50 Hz, two channels, clean.
• The real proof — WiFi. The SD operations disable the LAN interface over USB, so if the restore had not run the device would have dropped off the network. It streamed over 192.168.1.30 immediately afterwards, and a final USB listing returned all 32 files again.

Refs #406, #396, #399. Narrower than #342 by design — this is one asymmetry inside a path that already has a lock, and does not settle whether Core should serialize all mutating operations per device.

closes #407

Not merging — for review.

🤖 Generated with Claude Code

[tylerkron]

/agentic_review

[qodo-code-review]

PR Summary by Qodo

Serialize SD→LAN restore by adding a finalize phase to text exchanges

🐞 Bug fix 🧪 Tests 📝 Documentation 🕐 40+ Minutes

AI Description

• Add a finalize phase to text exchanges so SD→LAN restore runs under the same lock.
• Move SD card text operations to use exchange prepare/finalize instead of caller finally blocks.
• Skip LAN restore when an SD download is abandoned, and add regression tests for all cases.

Diagram

graph TD
  A["SD card operation"] --> B["DaqifiStreamingDevice"] --> C[["ExecuteTextCommandCoreAsync"]] --> D["prepareAsync: switch to SD + settle"] --> E["SCPI SD commands"] --> F["finalizeAsync: restore LAN"]
  G["DownloadSdCardFileAsync"] --> H[["RunWithHardDeadlineAsync"]] --> I{"worker abandoned?"} --> J["skip restore"]
  I --> K["restore LAN in finally"]
  subgraph Legend
    direction LR
    _op["Operation"] ~~~ _core[["Core exchange"]] ~~~ _dec{"Decision"}
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Introduce an IAsyncDisposable "exchange scope"

• ➕ Makes the prepare/finalize pairing explicit via async using and structured lifetime
• ➕ Avoids additional parameters on ExecuteTextCommandAsync overloads
• ➖ Would likely require a new public abstraction and wider refactoring of exchange call sites
• ➖ Does not naturally force override signature widening; subclasses could still bypass the scope

2. Add a separate virtual hook (e.g., OnExchangeFinalizeAsync)

• ➕ Keeps ExecuteTextCommandAsync signature stable for most callers
• ➕ Centralizes cleanup behavior
• ➖ Risk of silent bypass if subclasses override only the original exchange seam (the same problem cited for prepare)
• ➖ Harder to pass per-call cleanup (SD restore) without additional state

Recommendation: Keep the PR’s approach: an explicit finalizeAsync parameter on the existing exchange seam is the most robust way to guarantee the restore runs under the same lock acquisition while still forcing downstream overrides/test doubles to recompile and consciously handle SD traffic. The deferred-throw/logging behavior also correctly prevents lock leaks while preserving the primary failure signal.

Files changed (10) +968 / -285

Bug fix (2) +259 / -139

DaqifiDevice.csAdd finalizeAsync to text exchange core and defer finalize exceptions past lock release +94/-9

Add finalizeAsync to text exchange core and defer finalize exceptions past lock release

• Extends ExecuteTextCommandAsync overloads and core exchange implementation to accept an optional finalizeAsync that runs under the same lock as prepareAsync. Tracks whether validation passed and whether the exchange completed normally; finalize failures are captured, then either rethrown after releasing the lock (if exchange succeeded) or logged and suppressed (if another failure is already unwinding). Uses ExceptionDispatchInfo to preserve stack when rethrowing.

src/Daqifi.Core/Device/DaqifiDevice.cs

DaqifiStreamingDevice.csRun SD interface restore via exchange finalize; refine storage query and download cleanup +165/-130

Run SD interface restore via exchange finalize; refine storage query and download cleanup

• Routes SD card text operations (list, delete, storage query) through prepare/finalize pairing so the SD switch and LAN restore are both serialized by the text-exchange lock. Introduces a shared RestoreLanInterfaceAsync finalize method and moves the storage query’s settle delay into prepare (removing a blocking Thread.Sleep). Updates download to skip LAN restore when the worker was abandoned due to hard deadline, and adds an onWorkerAbandoned callback to the hard-deadline runner.

src/Daqifi.Core/Device/DaqifiStreamingDevice.cs

Tests (7) +706 / -145

DaqifiDeviceCapabilityDocumentTests.csUpdate test device override to honor exchange finalize phase +24/-11

Update test device override to honor exchange finalize phase

• Widens the ExecuteTextCommandAsync override signature to accept finalizeAsync. Ensures finalizeAsync executes in a finally block to mirror real device behavior during tests.

src/Daqifi.Core.Tests/Device/Capabilities/DaqifiDeviceCapabilityDocumentTests.cs

DaqifiDeviceDrainErrorQueueTests.csAdd finalizeAsync support to drain-error-queue test double +24/-11

Add finalizeAsync support to drain-error-queue test double

• Extends the overridden ExecuteTextCommandAsync signature with finalizeAsync. Runs finalizeAsync from a finally to reflect the new exchange contract.

src/Daqifi.Core.Tests/Device/DaqifiDeviceDrainErrorQueueTests.cs

DaqifiDeviceInitializeTests.csPropagate finalize phase through initialization/USB-step test overrides +65/-39

Propagate finalize phase through initialization/USB-step test overrides

• Updates multiple ExecuteTextCommandAsync overrides to accept finalizeAsync and execute it in finally. Preserves existing setup/behavior logic while matching the new exchange lifecycle.

src/Daqifi.Core.Tests/Device/DaqifiDeviceInitializeTests.cs

DaqifiDeviceStaleTextLineTests.csAdd comprehensive tests for finalize phase semantics and locking +257/-7

Add comprehensive tests for finalize phase semantics and locking

• Introduces a suite of tests verifying finalize ordering, execution under the exchange lock, behavior when exchange/finalize throw, and ensuring the lock is still released. Adds a helper method and a new testable call path that supplies finalizeAsync.

src/Daqifi.Core.Tests/Device/DaqifiDeviceStaleTextLineTests.cs

DeviceDiagnosticsTests.csUpdate diagnostics test double to support exchange finalize phase +22/-9

Update diagnostics test double to support exchange finalize phase

• Widens ExecuteTextCommandAsync override to accept finalizeAsync and ensures it runs in finally. Keeps prepareAsync behavior intact while adding the finalize mirror.

src/Daqifi.Core.Tests/Device/Diagnostics/DeviceDiagnosticsTests.cs

GetLanChipInfoAsyncTests.csUpdate LAN chip info test device to run finalize phase +22/-9

Update LAN chip info test device to run finalize phase

• Updates the overridden ExecuteTextCommandAsync signature to include finalizeAsync. Ensures finalizeAsync runs regardless of exchange outcome in tests.

src/Daqifi.Core.Tests/Device/GetLanChipInfoAsyncTests.cs

SdCardOperationsTests.csAdd SD restore/finalize regression coverage and abandon-download expectations +292/-59

Add SD restore/finalize regression coverage and abandon-download expectations

• Adds tests asserting LAN restore still happens on cancellation during settle delay (prepare already switched the bus). Adds a device that discards finalizeAsync to prove restores are routed through the exchange finalize seam. Adds download tests verifying restore is skipped when the transfer is abandoned but still performed on normal completion, plus test-device plumbing to snapshot sent commands safely.

src/Daqifi.Core.Tests/Device/SdCard/SdCardOperationsTests.cs

Documentation (1) +3 / -1

ISdCardOperations.csDocument that abandoned downloads do not restore the interface +3/-1

Document that abandoned downloads do not restore the interface

• Updates the DownloadSdCardFileAsync exception remarks to clarify that an abandoned transfer still owns the transport and the interface is left as-is. Advises callers to reconnect before further device use.

src/Daqifi.Core/Device/SdCard/ISdCardOperations.cs

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)

Great, no issues found!

Qodo reviewed your code and found no material issues that require review

_{To customize comments, go to the Qodo configuration screen, or learn more in the docs.}