Skip to content

Fix firmware flash reporting success when it actually failed#190

Merged
mikecarr merged 1 commit into
OpenIPC:devfrom
wkumik:fix/silent-flash-failure
Jul 13, 2026
Merged

Fix firmware flash reporting success when it actually failed#190
mikecarr merged 1 commit into
OpenIPC:devfrom
wkumik:fix/silent-flash-failure

Conversation

@wkumik

@wkumik wkumik commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

Problem

A failed firmware flash is reported to the user as a success. The device is left on its old firmware while the UI shows 100% and "Upgrade Complete!! Device has been flashed!"

Root cause is a chain of swallowed exceptions:

  1. SshClientService.UploadFileAsync catches upload errors and does not rethrow — a failed SCP upload returns as success.
  2. SysUpgradeService.PerformSysupgradeAsync catches upload + wc -c size-verification + sysupgrade in one try/catch that only logs — no rethrow. So even when the size check detects the file never landed, it is swallowed.
  3. FirmwareTabViewModel.UpgradeFirmwareFromFileAsync then unconditionally sets ProgressValue = 100 and shows the success dialog without checking whether the flash happened.

Especially easy to hit on dropbear cameras where the Renci.SshNet ScpClient upload can fail/truncate: upload silently no-ops, device never gets the files, UI still claims success (unchanged GITHUB_VERSION banner, no rootfs change).

Fix

  • UploadFileAsync: rethrow after logging.
  • PerformSysupgradeAsync: rethrow after logging; add UploadAndVerifyWithRetryAsync (3 attempts, 2s backoff) so flaky-link upload failures recover instead of proceeding against a missing file.
  • UpgradeFirmwareFromFileAsync: show explicit "Upgrade failed" dialog on error.

Testing

  • dotnet build — 0 errors (pre-existing MVVMTK0034 warnings only).
  • dotnet test — all 60 tests pass.

The local-file / by-SoC / dropdown firmware upgrade paths all funnel through
PerformSysupgradeAsync, which wrapped upload + size-verification + sysupgrade
in a try/catch that only logged the error and returned normally. The caller
(UpgradeFirmwareFromFileAsync) then unconditionally set ProgressValue = 100
and showed "Upgrade Complete!! Device has been flashed!" — so a failed flash
looked identical to a successful one and the device was left on its old
firmware. UploadFileAsync had the same swallow, so even a failed SCP upload
returned as success.

This is especially likely on dropbear targets where the Renci.SshNet ScpClient
upload can fail/truncate: the upload silently no-ops, the device never gets the
files, and the UI still claims success.

Changes:
- SshClientService.UploadFileAsync: rethrow instead of swallowing, so a failed
  upload surfaces to the caller.
- SysUpgradeService.PerformSysupgradeAsync: rethrow after logging so the caller
  can tell a failed flash from a successful one; add UploadAndVerifyWithRetryAsync
  which retries the upload + on-device size check (3 attempts) before giving up,
  so flaky links recover instead of silently proceeding against a missing file.
- FirmwareTabViewModel.UpgradeFirmwareFromFileAsync: show an explicit "Upgrade
  failed" dialog when the upgrade throws, instead of silently doing nothing.

Build clean (0 errors); all 60 existing tests pass.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@wkumik

wkumik commented Jul 11, 2026

Copy link
Copy Markdown
Contributor Author

Rebased mentally against current master — this is still worth taking, and I think the case for it is actually stronger now than when I opened it.

Since this PR went up, master gained real failure detection: ValidateRemoteFileSizeAsync (throws on Failed to verify … upload / upload size mismatch) and WaitForDeviceRecoveryAsync (throws if the device never comes back). That's the right instinct, and it means we already agree a failed flash should be noticed.

The problem is that the signal never escapes. Every one of those new throws lands in the outer catch in SysUpgradeService.PerformSysupgradeAsync, which only logs and calls updateProgress("Error: …") — it doesn't rethrow:

catch (Exception ex)
{
    _logger.Error(ex, "Error during sysupgrade.");
    updateProgress($"Error: {ex.Message}");
    // no rethrow
}

and FirmwareTabViewModel.UpgradeFirmwareFromFileAsync then runs on regardless:

ProgressValue = 100;
SetProgressBarBrush(ProgressCompleteBrush);
var result = await _messageBoxService.ShowCustomMessageBox(
    "Upgrade Complete!!",
    $"Device has been flashed!\n\n...",

So today Companion can detect that the flash failed, discard that fact, and still tell the user "Device has been flashed!" — the new validators are effectively dead code in the failure path. A completely failed flash remains pixel-identical to a successful one.

This is not hypothetical. It's why testers flash via the Local-file flow, see the green "Upgrade Complete", and stay on their old firmware — then spend days debugging a device they believe was reflashed. SshClientService.UploadFileAsync swallows its SCP error the same way, which is where it usually starts.

This PR is small and does exactly one thing: let the failure reach the user. Rethrow at the three swallow points, retry-with-verify on the upload, and show a real "Upgrade failed — the device is still on its previous firmware" dialog. Build is green, dotnet test 60/60.

Related, for context: OpenIPC/firmware#2220 fixes the underlying sysupgrade bug (the verify-mount runs after the kernel is already written, and can hang indefinitely, leaving a half-flashed device). But that only helps devices once they've taken new firmware — which they can't do while flashing is the thing that's broken. Companion telling the truth about a failed flash is what people need in the meantime.

Happy to rebase or split this further if that makes review easier.

@mikecarr
mikecarr changed the base branch from master to dev July 13, 2026 12:56
@mikecarr

Copy link
Copy Markdown
Member

Retargeted this PR to dev. For this repo, feature/fix PRs should land in dev first; master should receive release/integration changes after dev has been validated.

Validation against current dev:

  • Patch applies cleanly to dev.
  • dotnet build Companion.Desktop/Companion.Desktop.csproj -c Release succeeds.
  • dotnet test is currently blocked by an existing dev test compile issue in WfbTabViewModelTest missing the new IMessageBoxService constructor parameter, so that is not caused by this PR.

The README and docs/development.md document the local build/test commands; the branch-flow expectation is: PR to dev first, then promote/release from there.

@wkumik

wkumik commented Jul 16, 2026

Copy link
Copy Markdown
Contributor Author

Thank you for merging this, and for retargeting it to dev — noted on the branch flow, and I've rebased #191 onto dev accordingly so it follows the same path.

One small question, asked with no expectation and entirely at your discretion: is a release cut from dev on the horizon at some point? No urgency intended — I know releases carry work and risk that a merge doesn't, and you're the best judge of when dev has been validated enough to promote.

The only reason I ask is that this particular fix is one whose value is fully in users' hands and zero in the repo. release-v0.9.4 is from 19 April and dev has 12 commits on it since. Until there's a build that includes this one, people flashing with Companion still see the green "Upgrade Complete!! / Device has been flashed!" over a flash that silently failed, and then spend days debugging a device they reasonably believe was reflashed — which is the exact loop this change exists to break. Testers are currently recovering air units by hand over SSH with wget + flashcp because the GUI told them the flash worked.

So: no pressure at all, and I'm not asking you to rush dev on my account. If it's simply "not yet, dev needs more validation first," that's a completely fair answer and I'd rather that than a hurried release. If there's validation work that would help get dev to a releasable state, I'm happy to take some of it on — testing on real SSC338Q hardware, or anything else that's useful. Just let me know what would actually help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants