Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix app can not restart after an unexpected exit during the apply releases #1862

Merged
merged 4 commits into from
Aug 1, 2023
Merged

Fix app can not restart after an unexpected exit during the apply releases #1862

merged 4 commits into from
Aug 1, 2023

Conversation

DamonYu6
Copy link
Contributor

@DamonYu6 DamonYu6 commented Jul 28, 2023 •
edited
Loading

Problem Description: During the use of Squirrel's automatic upgrade feature, if the app is quit or unexpectedly exits while installing the new version, it may result in unable to restart the app. Upon reviewing the source code, it was found that the stubExe would find the latest version and create the path to launch it. However, due to missing files in the latest app-xxx directory, the app fails to start.

Solution: To address this issue, during the application of releases, the new version is first extracted to a temp-app-xxxx directory. After the extraction is complete, it is then renamed to the final app-xxxx directory. This approach ensures that even if the app unexpectedly exits during the installation process and leaves the temp directory, it will not affect the next app launch.

@DamonYu6
Fix app can not restart after an unexpected exit during the apply rel…
63e2a4b 
…eases

Use a temp directory to apply the releases and then rename the temp directory to the final release directory
@DamonYu6 DamonYu6 mentioned this pull request Jul 28, 2023
Open
@anaisbetts
Copy link
Contributor

anaisbetts commented Jul 28, 2023 •
edited
Loading

Instead of doing this, write a .not-finished file in the app folder, and delete it when the update successfully completes. Then, make the stubs ignore folders with .not-finished and delete any folders on update check that do have .not-finished. This seems more complicated but it avoids moving executables around or renaming folders, which often triggers 3rd party antivirus

Also, don't quit your application while updates are running - instead of exiting, hide the window, await the update, then close once it finishes

@damonyu666
Fix the app can not restart after an unexpected exit during the upgrade
0a8a9ee 
Use the .not-finished file to indicate whether the installation of the new version was successful and make stubs ignore those folders which have the .not-finished file
@DamonYu6
Copy link
Contributor Author

Instead of doing this, write a .not-finished file in the app folder, and delete it when the update successfully completes. Then, make the stubs ignore folders with .not-finished and delete any folders on update check that do have .not-finished. This seems more complicated but it avoids moving executables around or renaming folders, which often triggers 3rd party antivirus

Agreed, I have submitted another commit, plz help review it, thanks. About delete any folders on update check that do have .not-finished, I don't need to add extra logic to handle this part since it already covers in

Squirrel.Windows/src/Squirrel/UpdateManager.ApplyReleases.cs

Line 306 in 0ee9447

await Utility.DeleteDirectory(target.FullName);

and

Squirrel.Windows/src/Squirrel/UpdateManager.ApplyReleases.cs

Line 96 in 0ee9447

await cleanDeadVersions(currentVersion, newVersion);

Also, don't quit your application while updates are running - instead of exiting, hide the window, await the update, then close once it finishes

Yes, during the upgrade process, we don't recommend users to exit the app. But we can't guarantee whether the app will be exited due to other reasons, such as crash or system shutdown. So we need this part logic to make sure users won't be able to launch the app due to the uncompleted upgrade.

anaisbetts
anaisbetts requested changes Jul 29, 2023
View reviewed changes
Copy link
Contributor

@anaisbetts anaisbetts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few small things but this looks good

src/Squirrel/UpdateManager.ApplyReleases.cs Outdated Show resolved Hide resolved
src/StubExecutable/StubExecutable.cpp Outdated Show resolved Hide resolved
@DamonYu6 DamonYu6 requested a review from anaisbetts July 30, 2023 15:14
@DamonYu6
Copy link
Contributor Author

DamonYu6 commented Aug 1, 2023

A few small things but this looks good

Hi @anaisbetts , plz review it again, thx

@anaisbetts anaisbetts merged commit 5e44cb4 into Squirrel:develop Aug 1, 2023
1 check passed
@KennyTK KennyTK mentioned this pull request Oct 25, 2023
Closed
@caesay
Copy link

caesay commented Nov 4, 2023

Not sure if I missed something here, but I don't think this covers the Update.exe --processStart case, which will seemingly still launch the broken directory.

@damonyu666
Copy link

@caesay In fact, when launching the app through "Update.exe --processStart," it initiates the latest version recorded in the local RELEASES file. However, if the last upgrade attempt failed, the RELEASES file remains unchanged, and as a result, it won't launch the broken directory.

@beyondkmp beyondkmp mentioned this pull request Jul 20, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anaisbetts anaisbetts anaisbetts approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@DamonYu6 @anaisbetts @caesay @damonyu666