[tylerkron]

Not merging — for review.

closes #234

What was wrong

Cancel a WiFi-module-only firmware update and the status line changes to "Canceling firmware update…", the Cancel button stays enabled — and the flash keeps running to completion. Nothing is cancelled and nothing is logged. A user who believes they stopped the flash and then unplugs the board is unplugging mid-flash.

Closing the window was no escape hatch either: the flash carried on against a torn-down view model.

Why

Two different fields held the same idea — "the cancellation source for the in-flight firmware upload" — and the Cancel button was wired to one of them while one of the two producers wrote the other.

	who created it	who got the token
FirmwareUpdateCoordinator._firmwareUploadCts	the combined PIC32 + WiFi update, which nulls it when it finishes	the combined flash
DaqifiViewModel._firmwareUploadCts	UpdateWifiFirmwareOnly	the WiFi-only flash

Cancel routes to FirmwareUpdateCoordinator.CancelUpload(), which cancels the coordinator's field. On the WiFi-only path that field is null, so the ?. swallowed the call. Its guard made the lie complete: it gated on _host.IsFirmwareUploading — a flag the view model had just set — so it passed the guard, wrote "Canceling firmware update…" into the UI, and then cancelled nothing.

The fix

Sole ownership, in the coordinator.

• UpdateWifiModuleOnlyAsync is the WiFi-only entry point. It creates the run's CancellationTokenSource exactly as UploadFirmwareAsync does. DaqifiViewModel no longer holds one — the field, and its create/dispose/null lifecycle in three places, are deleted.
• BeginUpload / EndUpload make "adopt the source" and "mark the host uploading" one step, in that order, so the Cancel button is never enabled over a source that does not exist yet.
• CancelUpload gates on the source, not on the host's flag. It can no longer announce a cancellation it has no way to deliver. That guard is what made the original bug silent.
• The source is never disposed (see the note on EndUpload), which is how the repo settled the same Cancel()-versus-Dispose() race in feat(mobile): logged-session plot viewer + delete/delete-all (#7 phase-2) #26. Nothing reads its WaitHandle or gives it a timer, so there is nothing unmanaged to release, and with no Dispose a reference already captured by CancelUpload can only ever signal a live-or-finished source — a harmless no-op.
• UpdateWifiModuleAsync is now private — no caller outside the coordinator hands it a token any more — and it and FlashWifiPackageAsync drop the coreDevice parameter, which was threaded through both and dereferenced by neither.

Both call sites now read the same way: the coordinator opens the run, hands its own token down, and closes the run in a finally.

Verification

Firmware cancellation had no test coverage at all before this — no test in the repo constructed a FirmwareUpdateCoordinator. Daqifi.Avalonia.Tests/Device/Firmware/FirmwareUpdateCancellationTests.cs (5 tests) is new. It needs no hardware: the cancellation is observed at the coordinator's own injected seams — the download service and the host's WiFi-probe quiesce both park on the token they were handed, so a test asserts on exactly the token the flash received.

Run against the unfixed coordinator first. Commit 770031c is the test file as written against unchanged origin/main product code; 2 of its 4 tests fail there, and they are the two that describe the defect:

• Canceling_a_wifi_only_flash_cancels_the_flash — fails with TimeoutException after 5 s. The flash never unwinds, because the token it is running on is never signalled. This is issue Cancel during a WiFi-only firmware update cancels the wrong CancellationTokenSource — the UI says "Canceling" and the flash runs on #234 reproduced.
• Cancel_does_not_claim_to_be_canceling_when_nothing_is_cancelable — fails: the status line reads "Canceling firmware update…" with no source to cancel.

The other two (Canceling_a_combined_update_cancels_the_flash, Cancel_after_a_wifi_only_flash_has_finished_is_a_no_op) pass on origin/main and exist as regression guards for the combined path, which already routed Cancel correctly and must keep doing so now that both paths share one owner.

The final file has 5 tests, all passing — the fifth (A_wifi_only_flash_marks_the_host_uploading_while_it_runs) pins that the coordinator now raises the flag the Cancel button's CanExecute reads. Three of the five call the new entry point and so cannot be expressed against main at all.

Gate, at head 8fec8d2:

• Desktop build: green.
• iOS Release iossimulator-arm64: green.
• dotnet test: 472 passed, 0 failed.
• First-party warnings: measured on this branch and on my own merge-base (a74933d) in the same worktree, same --no-incremental discipline, third_party/ excluded, compared as a set of (file, warning code). Identical on both heads — none added, none removed (65 distinct, desktop and iOS Release alike).
• packages.lock.json untouched across the repo.
• Android is not buildable on this machine (workload absent) — left to CI.

No hardware was involved and no firmware was flashed: this is a CancellationTokenSource wiring defect, fixed and verified in code.

Disclosures

• The product diff is net +10 lines of code (+36/−26 excluding comments and blanks; the raw +84/−28 is mostly XML doc). The additions are the new entry point and the two small lifecycle helpers; what they buy is the deletion of a duplicated field and its three-site lifecycle from the view model. The comments that survive explain why the guards are shaped as they are, since getting that wrong is the bug.
• The Cancel-versus-Dispose race is closed by deleting the Dispose calls, not by locking. That means it is a structural property of the code — there is no disposed source for a captured reference to touch — rather than something a unit test pins, since deterministically interleaving CancelUpload with EndUpload would need a seam that exists only for the test. Cancel_after_a_wifi_only_flash_has_finished_is_a_no_op covers the observable contract.
• Closing the window still does not cancel a running flash. DaqifiViewModel.Dispose never cancelled the old field and this PR does not add a cancel there. Aborting a firmware flash on window close is a device-safety decision, not a wiring fix, and slipping it in here would be the wrong place for it. Worth its own ticket.
• Behaviour change on one early-return path: when the WiFi-only command is invoked on a disconnected device, IsFirmwareUploading is no longer set true and then immediately back to false. Nothing observes the transient, and the ordering relative to CancelWifiFirmwareCheck() is unchanged in effect — everything from the command's re-entrancy guard to BeginUpload() runs in one synchronous stretch with no await, so no dispatcher work can interleave.
• No bound view-model member was renamed or removed; the only removed field was private, and UpdateWifiModuleAsync is not a binding target. Nothing in the .axaml refers to any of it.

🤖 Generated with Claude Code

[tylerkron]

/agentic_review

[qodo-code-review]

PR Summary by Qodo

Fix WiFi-only firmware cancellation token ownership

🐞 Bug fix 🧪 Tests 🕐 20-40 Minutes

AI Description

• Centralizes cancellation-source ownership for combined and WiFi-only firmware updates in the
 coordinator.
• Prevents cancellation status changes when no active upload token can be signaled.
• Adds regression coverage for active, completed, WiFi-only, and combined cancellation paths.

Diagram

sequenceDiagram
    actor User
    participant VM as View Model
    participant Coord as Firmware Coordinator
    participant CTS as Upload Token
    participant Flash as Firmware Flash
    participant Host as Upload State
    User->>VM: Start WiFi update
    VM->>Coord: Update WiFi only
    Coord->>CTS: Create source
    Coord->>Host: Mark uploading
    Coord->>Flash: Run with token
    User->>VM: Cancel update
    VM->>Coord: CancelUpload
    Coord->>CTS: Cancel source
    CTS-->>Flash: Signal cancellation
    Flash-->>Coord: Operation canceled
    Coord->>Host: Clear upload state

Loading

High-Level Assessment

Coordinator ownership is the strongest approach because firmware execution, cancellation, and upload-state transitions form one lifecycle. Keeping a second source in the view model or forwarding cancellation between owners would preserve synchronization risks, while a separate cancellation manager would add unnecessary indirection for one active operation.

Files changed (3) +426 / -28

Bug fix (2) +69 / -28

FirmwareUpdateCoordinator.csUnify firmware cancellation lifecycle in the coordinator +64/-16

Unify firmware cancellation lifecycle in the coordinator

• Introduces a coordinator-owned WiFi-only entry point and shared BeginUpload/EndUpload lifecycle helpers. CancelUpload now gates on the active source, safely handles completion races, and signals the same token passed through download and flash operations.

Daqifi.Avalonia/Daqifi.Desktop/Device/Firmware/FirmwareUpdateCoordinator.cs

DaqifiViewModel.csDelegate WiFi-only cancellation ownership to the coordinator +5/-12

Delegate WiFi-only cancellation ownership to the coordinator

• Removes the view model’s independent firmware cancellation source and its lifecycle management. The WiFi-only command now delegates the complete cancellable run to FirmwareUpdateCoordinator while retaining UI notifications and error handling.

Daqifi.Avalonia/Daqifi.Desktop/ViewModels/DaqifiViewModel.cs

Tests (1) +357 / -0

FirmwareUpdateCancellationTests.csAdd firmware cancellation regression coverage +357/-0

Add firmware cancellation regression coverage

• Adds hardware-free tests proving cancellation reaches WiFi-only and combined update tokens. Covers upload-state transitions, cancellation without an active source, and cancellation after completion using deterministic fakes.

Daqifi.Avalonia.Tests/Device/Firmware/FirmwareUpdateCancellationTests.cs

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (0)

1. Cancel races source disposal ✓ Resolved 🐞 Bug ☼ Reliability

Description

CancelUpload retains a CTS reference that EndUpload can dispose before cts.Cancel() executes,
causing an ObjectDisposedException when cancellation arrives as an upload finishes. Clearing the
shared field before disposal does not protect a reference already read by another thread.

Code

Daqifi.Avalonia/Daqifi.Desktop/Device/Firmware/FirmwareUpdateCoordinator.cs[R321-322]

        _host.FirmwareUpdateStatusText = "Canceling firmware update...";
-        _firmwareUploadCts?.Cancel();
+        cts.Cancel();

Relevance

●●● Strong

PR #26 explicitly accepted synchronizing CancellationTokenSource cancellation and disposal to
prevent ObjectDisposedException races.

PR-#26

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

CancelUpload reads the shared source at lines 315-316 and invokes it later at line 322, while
EndUpload independently clears and disposes the same source at lines 343-348 without
synchronization. This is the same cancel-versus-dispose CTS race previously accepted in PR #26.

Daqifi.Avalonia/Daqifi.Desktop/Device/Firmware/FirmwareUpdateCoordinator.cs[310-348]
PR-#26

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`CancelUpload` can copy `_firmwareUploadCts`, race with `EndUpload` disposing that source, and then throw `ObjectDisposedException` from `Cancel()`. Synchronize cancellation, replacement, clearing, and disposal so no source can be disposed while cancellation is using it.

## Issue Context
The field being cleared before disposal only protects future readers; it does not protect a CTS reference already captured by `CancelUpload`. Preserve the intended no-op behavior when an upload has already ended, and ensure cleanup remains associated with the source owned by that run.

## Fix Focus Areas
- Daqifi.Avalonia/Daqifi.Desktop/Device/Firmware/FirmwareUpdateCoordinator.cs[310-348]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Context sources

Review mode: ⚖️ Balanced: This is a localized but behavior-changing firmware cancellation and lifecycle fix with concurrency and device-safety implications, warranting a careful single-pass review.

Tip of the day

💡 Did you know, you can keep summaries lean with Finding overflow, which tucks the rest behind 'View more'

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[tylerkron]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit 8fec8d2

[tylerkron]

Qodo-clean, CI green — ready for review at 8fec8d2.

Verification behind this note is CI's run on 8fec8d2 (33913933462), read from the log — not a local build. I pushed nothing, so the head has not moved and that green covers the current tree. 6 SUCCESS + 1 SKIPPED, including Android head, which cannot be built on this Mac.

Qodo. Round 1 on 97424de raised one High ("Cancel races source disposal"); 8fec8d2 answered it. Round 2 genuinely re-reviewed 8fec8d2 (not a Skipped doc-only round — the review's Previous reviews fold shows 97424de as prior, 8fec8d2 as current) and returned Bugs (0) / Rule violations (0) / Skill insights (0). Inline reviewThreads: 1 qodo thread, 0 unresolved. Re-checked both surfaces after a settle interval against the same SHA — still clean, summary comment unedited since 20:01:31Z.

Claims re-derived rather than repeated

• 472/472 and "+5 tests". Closed from both ends against CI's own logs: main at the merge-base a74933d reports Passed: 467, Total: 467; this head reports Passed: 472, Total: 472. 472 − 467 = 5, matching the [Fact] count in the one new test file, and git diff --numstat origin/main...HEAD touches exactly three files with no other test file among them. The 4→5 denominator move is real, not a figure true at no commit.
• "2 of 4 fail on unchanged main." Verified by inspection, not by running — CI never ran 770031c. That commit's file has exactly 4 [Fact]s over unchanged product code. Cancel_does_not_claim_to_be_canceling_when_nothing_is_cancelable is deterministic: main's CancelUpload gates on _host.IsFirmwareUploading, the test sets that flag with nothing in flight, so main writes the status line and the Assert.Equal(string.Empty, …) fails. Canceling_a_wifi_only_flash_cancels_the_flash: on main the coordinator's field is null on that path, ?.Cancel() no-ops, the parked download never unwinds, and WaitAsync(5s) throws TimeoutException — not an OperationCanceledException. Both fail as described.
• Warning parity. Parsed (job, file, code) triples out of the CI logs for a74933d and 8fec8d2, excluding third_party/: 263 distinct on each, empty symmetric difference, identical per-code histograms and identical per-job counts (Android 350, Desktop macOS 344, Library+capture 346, iOS 344). Covers Android, which a local measurement cannot.
• Merge safety. git merge-base origin/main HEAD == origin/main tip == a74933d: main has not moved since this branch cut, so this PR's own green already covers the exact merge result. No fix(layering): a view-model was hand-writing SCPI bytes at the transport #189-shaped surprise available here.
• packages.lock.json untouched; working tree clean.

The disposal removal holds up

• The feat(mobile): logged-session plot viewer + delete/delete-all (#7 phase-2) #26 citation is accurate. LoggedSessionsMobileViewModel._viewCts is deliberately not disposed, for the same stated reason, and feat(mobile): logged-session plot viewer + delete/delete-all (#7 phase-2) #26 carried a Qodo "Cts cancel/dispose race" thread resolved that way. Qodo's Relevance blurb here — that feat(mobile): logged-session plot viewer + delete/delete-all (#7 phase-2) #26 "explicitly accepted synchronizing cancellation and disposal" — mischaracterises it; feat(mobile): logged-session plot viewer + delete/delete-all (#7 phase-2) #26 synchronizes nothing, it removes the disposal. The PR body has this right and Qodo's citation had it backwards.
• No accumulation. The coordinator holds exactly one reference and EndUpload() nulls it, so at most one source is ever live and retired ones are immediately unreachable. A repeated-attempt leak isn't available.
• The precondition actually holds. Repo-wide, CreateLinkedTokenSource / CancelAfter / .WaitHandle appear only in SdCardSessionImporter and NativeWiFiDiscovery — neither on the firmware path, both already using-scoped. So "nothing unmanaged to release" is checked, not asserted.

One finding, not a blocker

No Avalonia view surfaces the firmware Cancel affordance at all. Across all 31 .axaml files, nothing binds CancelFirmwareUploadCommand, CancelUploadFirmwareCommand, FirmwareUpdateStatusText, or DaqifiViewModel.IsFirmwareUploading. (FirmwareDialog.axaml's IsFirmwareUploading / IsUploadComplete bindings resolve against FirmwareDialogViewModel, which has its own copies.) The WPF app does have it — Daqifi.Desktop/View/Flyouts/FirmwareFlyout.xaml:90, Command="{Binding CancelFirmwareUploadCommand}" IsEnabled="{Binding IsFirmwareUploading}" — and Avalonia has no FirmwareFlyout equivalent.

So the description's "the Cancel button stays enabled" / "wrote … into the UI" overstates today's user-visible impact: the wiring defect and this fix are real at the view-model layer, but the user-facing half is currently unreachable because the control was never ported. That is pre-existing and untouched by this PR — the plumbing here is correct and ready (IsFirmwareUploading already carries [NotifyCanExecuteChangedFor(CancelFirmwareUploadCommand)]), only the view is missing. It does contradict the "parity is complete" ledger from #229 and deserves its own ticket.

It also settles the screenshot question: no screenshot. This PR changes nothing that is drawn (no .axaml in the diff) and there is no bound surface for firmware-cancel state to capture, so one would have been evidence for nothing.

Also checked

• XAML binding sweep (the deletions are invisible to the build): _firmwareUploadCts was private and UpdateWifiModuleAsync is a coordinator method, so neither is a binding target. Every surviving bound member (Shell.UpdateWifiFirmwareOnlyCommand, Shell.UploadWiFiProgress, Shell.UploadFirmwareCommand, SelectedDeviceSupportsFirmwareUpdate) is present and unrenamed.
• Re-entrancy. Both entry points guard on _host.IsFirmwareUploading before BeginUpload(), and that stretch is synchronous in both paths (CancelWifiFirmwareCheck() is a plain void, and BeginUpload() is UpdateWifiModuleOnlyAsync's first statement, so it runs before the method's first await). BeginUpload() cannot clobber a live run's source.
• Device safety. On the WiFi-only path IsFirmwareUploading now goes false one dispatcher turn before the VM clears DeviceBeingUpdated, where both previously happened in one synchronous finally. The WiFi-probe hard stops also test deviceBeingUpdated per device, so the device stays excluded across that window — and the flash has already returned by the time EndUpload() runs. No regression.

Not merging.