fix(nsis): use atomic rmdir on update #6551
Conversation
Previously uninstaller would run `RMDir /r $INSTDIR` to remove the currently installed version, but this operation is not atomic and if some of the files will fail to delete it will leave them in the directory while erasing the rest. If we are updating, however, this leads us to a tricky situation where we cannot update these files, but cannot also cancel the installation. Because of the erased files the app won't be able to start if the installation won't completely at least partially. However, the downside of that is that the app can have new asar.unpacked files along with the old asar, executable, and bindings. The approach of this change is to recursive use `Rename` instead of a single `RMDir /r` in uninstaller (when `isUpdated` is true) to move the whole app directory file by file to a temporary folder. If this operation fails due to busy files - we use `CopyFiles` to restore all files that we managed to move so far. Because the whole uninstallation process becomes interrupted - the app shortcut and file associations have to be removed only *after* the successful recursive `Rename`. This error is caught by installer running in an update mode (see `installUtil.nsh`) and presented to user in a dialog. If this erro happen the installation does not proceed normally. In addition to all of the above, this patch simplifies the last resort measure in `extractAppPackage` which should now only run when old uninstaller (that still uses `RMDir /r`) leaves busy files behind.
🦋 Changeset detectedLatest commit: f8fe5ff The changes in this PR will be included in the next version bump. This PR includes changesets to release 8 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
✔️ Deploy Preview for car-park-attendant-cleat-11576 ready! 🔨 Explore the source changes: f8fe5ff 🔍 Inspect the deploy log: https://app.netlify.com/sites/car-park-attendant-cleat-11576/deploys/61e25152115705000774080c 😎 Browse the preview: https://deploy-preview-6551--car-park-attendant-cleat-11576.netlify.app |
indutny-signal
force-pushed
the
fix/partial-updates-with-nsis-2
branch
from
bc9eb99 to
dd8770f
Compare
| DetailPrint `Uninstall was not successful. Uninstaller error code: $R0.` | ||
| SetErrorLevel 2 | ||
| Quit |
There was a problem hiding this comment.
I think ideally we should retry running uninstaller (with user's confirmation) in such case instead of failing whole installation immediately. Let me know if you want me to add this to the current patch.
There was a problem hiding this comment.
Oh, and btw. Abort doesn't quite work here for some reason (the installer just hangs), so I had to resort to Quit.
There was a problem hiding this comment.
@indutny-signal This sounds like a great idea. Is it complex to add?
we should retry running uninstaller (with user's confirmation) in such case instead of failing whole installation immediately
There was a problem hiding this comment.
I actually already pushed it 😉
| @@ -28,6 +28,109 @@ Function un.onInit | |||
| !endif | |||
| FunctionEnd | |||
|
|
|||
| Function un.atomicRMDir | |||
There was a problem hiding this comment.
I'm not entirely proud of this, but it implements RMDir /r that preserves backups of all files and directories that it removed in $PLUGINSDIR\old-install.
| @@ -38,6 +141,34 @@ Section "un.install" | |||
|
|
|||
| !insertmacro setLinkVars | |||
|
|
|||
| # delete the installed files | |||
There was a problem hiding this comment.
The order of operations changed as stated in the commit message. We try to keep as much state as possible in case we will need to restore the files on failure. The users would be really frustrated to see their shortcuts and file associations go if uninstall.exe --updated fails while restoring all app's files.
There was a problem hiding this comment.
I just went ahead and did it. Makes for a much better user experience!
|
Awesome! Thanks for merging it! |
|
Thanks for the contribution @indutny! It'll make it in our upcoming v23 alpha release :) |
Previously uninstaller would run
RMDir /r $INSTDIRto remove thecurrently installed version, but this operation is not atomic and if
some of the files will fail to delete it will leave them in the
directory while erasing the rest. If we are updating, however, this
leads us to a tricky situation where we cannot update these files, but
cannot also cancel the installation. Because of the erased files the app
won't be able to start if the installation won't completely at least
partially. However, the downside of that is that the app can have new
asar.unpacked files along with the old asar, executable, and bindings.
The approach of this change is to recursive use
Renameinstead of asingle
RMDir /rin uninstaller (whenisUpdatedis true) to move thewhole app directory file by file to a temporary folder. If this
operation fails due to busy files - we use
CopyFilesto restore allfiles that we managed to move so far. Because the whole uninstallation
process becomes interrupted - the app shortcut and file associations
have to be removed only after the successful recursive
Rename.This error is caught by installer running in an update mode (see
installUtil.nsh) and presented to user in a dialog. If this errohappen the installation does not proceed normally.
In addition to all of the above, this patch simplifies the last resort
measure in
extractAppPackagewhich should now only run when olduninstaller (that still uses
RMDir /r) leaves busy files behind.Testing
Tested by doing normal updates and keeping tmp.exe open as in #6547
electron-builder-demo.mp4