Enable rollback on install failures during update#40524
Conversation
There was a problem hiding this comment.
Pull request overview
This PR hardens WSL against unrecoverable failures when an MSI major upgrade fails mid-install by (1) moving old-product removal into the MSI transaction for rollback safety and (2) adding a dedicated, localized error path when required packaged VHD files (e.g., system.vhd, modules.vhd) are missing so users get an actionable message instead of a generic HCS failure.
Changes:
- Adjust WiX
MajorUpgradescheduling soRemoveExistingProductsruns inside the MSI transaction (rollback restores the previous install on failure). - Introduce
WSL_E_SYSTEM_DISTRO_MISSINGand a localized message for missing packaged files. - Add runtime existence checks in VM startup paths and wire the new HRESULT into common error-string handling.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| msipackage/package.wix.in | Schedules MajorUpgrade removal to occur inside the MSI transaction for rollback protection. |
| src/windows/service/inc/wslservice.idl | Adds new HRESULT WSL_E_SYSTEM_DISTRO_MISSING (0x33). |
| src/windows/service/exe/WslCoreVm.cpp | Replaces debug-only asserts with production checks that throw a user-facing localized error when packaged VHDs are missing. |
| src/windows/service/exe/HcsVirtualMachine.cpp | Adds packaged-file existence validation before attaching boot VHDs (currently without setting a user-facing message). |
| src/windows/common/wslutil.cpp | Adds the new HRESULT to common error mappings and returns a localized fallback string (currently hard-coded to system.vhd). |
| localization/strings/en-US/Resources.resw | Adds MessageSystemDistroMissing localized string resource. |
|
Thanks for investigating, would it be possible to try to root cause the issue instead of a band-aid? A slightly better error message isn’t going to help users that get into this state. |
2ad3626 to
05c4925
Compare
05c4925 to
c1f0d2c
Compare
|
Apologies for the earlier noise — I've cleaned this up. The PR is now a single-line MSI fix (the root cause), no runtime checks. The previous defense-in-depth changes have been removed to keep this focused on what actually prevents the data loss. |
ptrivedi
left a comment
There was a problem hiding this comment.
Seems to be a righteous fix targeted towards preventing data loss. It would be good to keep an eye out at figuring out why some of these installs fail as we look at more issues.
OneBlue
left a comment
There was a problem hiding this comment.
Reading through https://docs.firegiant.com/wix3/xsd/wix/majorupgrade/, this appears to be low risk and should be a direct improvement in case of an upgrade failure.
Change LGTM, although if it all possible I would recommend adding a test failure that artificially fails the upgrade to validate the rollback (potentially doing something similar to what the MsixUpgradeFails() test case does)
|
Azure Pipelines successfully started running 1 pipeline(s). |
eec7985 to
1995224
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
1995224 to
1c778d3
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Hi @benhillis @ptrivedi @OneBlue — sorry for the delay on this! I've added the e2e rollback test as suggested. The latest commit:
All CI checks are green. Would appreciate a re-review when you get a chance — thanks! |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
OneBlue
left a comment
There was a problem hiding this comment.
LGTM, two minor comments and
Add WslTestForceInstallFailure deferred CA that returns ERROR_INSTALL_FAILURE when WSL_TEST_FORCE_INSTALL_FAILURE=1 is passed to msiexec. Sequenced after FinalizeInstall inside the transaction so MSI rollback restores files. New MsiUpgradeFailureRestoresFiles test installs the MSI, attempts an upgrade with the forced failure, then asserts wsl.exe and wslservice.exe survived. Also fixes MsiRemoveExistingProductsScheduledInsideTransaction per review: - Use MSI_NULL_INTEGER instead of -1 for null sequence detection - Drop redundant InstallFinalize bound check Co-authored-by: Copilot <[email protected]>
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Thanks @OneBlue! Both nits addressed:
Tests pass locally. CI triggered. |
Fix: all installed files lost on failed MSI upgrade
Fixes #40488
Problem
MajorUpgradewith noScheduleattribute (our previous state) uses the WiX defaultafterInstallValidate, which removes the old product outside the MSI transaction. If the new install fails afterward, all old files (~30 files, ~1.1GB including system.vhd) are gone with no rollback.Even Worse: once files are lost, reinstalling does not recover them. Running
msiexec /iagain reports success but does nothing -- MSI thinks the product is already installed and skips all file operations. Recovery requiresmsiexec /fa(repair),REINSTALL=ALL, or full uninstall + reinstall -- none of which a typical user would know to try.Fix
Add
Schedule="afterInstallInitialize"-- this moves old product removal inside the transaction. On failure, MSI restores all files from.rbfbackups automatically. The unrecoverable state is never reached.Tradeoff: ~700MB extra temporary disk during upgrade (freed on commit). No other downsides identified -- all custom actions are guarded by
(not UPGRADINGPRODUCTCODE).Not in scope
Locked-file reboot-pending deletes (kernel-mode lock holders that
MSIRMSHUTDOWNcan't kill). That's a separate issue.Test results
afterInstallInitialize)How the fix works (observed via filesystem monitoring):
During upgrade, old files are atomically renamed to
.rbfbackups (not deleted). On failure, MSI restores them from the.rbffiles automatically.