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.

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

Register restart for resume #3858

Merged
merged 25 commits into from Dec 6, 2023
Merged

Conversation

ryfu-msft
Copy link
Contributor

@ryfu-msft ryfu-msft commented Nov 6, 2023

Related to: #3165

Changes:

  • This PR calls RegisterApplicationRestart to register winget to restart upon a reboot. The resume command and the resume id are passed as arguments to the winget process so that it can resume from where it left off prior to reboot.
  • Registering for a restart will only take place if the result of the installation is REBOOT_REQUIRED_FOR_INSTALL.
  • The checkpoint database will only persist if the result of the installation is REBOOT_REQUIRED_FOR_INSTALL.
  • Added a new settings value called maxResumes that limits the number of times a resume state can be "resumed". This is to ensure that continuous reboots don't occur.
  • In order for the process to be restarted, the reboot must be kicked off while the process is still running. The winget process cannot be terminated normally or else Windows Error Reporting will not initiate the restart. My workaround is to have the process sleep while the reboot process is kicked off.
  • Moved the InitiateReboot() flow to take place after --wait argument is processed so that the user is prompted to continue before a reboot is kicked off.
Microsoft Reviewers: Open in CodeFlow

@ryfu-msft ryfu-msft marked this pull request as ready for review November 14, 2023 20:12
@ryfu-msft ryfu-msft requested a review from a team as a code owner November 14, 2023 20:12

This comment has been minimized.

@microsoft microsoft deleted a comment from github-actions bot Nov 16, 2023
// The above was omitted initially as a security precaution to ensure that user input to '--log' wouldn't be passed directly to ShellExecute
ShellExecute(NULL, NULL, Runtime::GetPathTo(Runtime::PathName::DefaultLogLocation).wstring().c_str(), NULL, NULL, SW_SHOWNORMAL);
}
if (Settings::ExperimentalFeature::IsEnabled(Settings::ExperimentalFeature::Feature::Reboot) &&
Copy link
Member

Choose a reason for hiding this comment

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

If the context is terminated (not with an exception, but "normally"), this code will still run. That may or may not be correct, but I want to make sure it is considered. It feels ok, but then I worry that it won't happen for an exception. I would say that we haven't been strict on the precise method for failures to propagate, which means that this code is likely to not run in some situations when it probably should.

Since this is experimental, it is fine to live here for now. But it will be good to keep this in mind.

src/AppInstallerCLICore/Workflows/ResumeFlow.cpp Outdated Show resolved Hide resolved
src/AppInstallerCLICore/Workflows/ResumeFlow.cpp Outdated Show resolved Hide resolved
src/AppInstallerCommonCore/Reboot.cpp Outdated Show resolved Hide resolved
src/AppInstallerCommonCore/Reboot.cpp Outdated Show resolved Hide resolved
src/AppInstallerCommonCore/Reboot.cpp Outdated Show resolved Hide resolved
src/AppInstallerCommonCore/Reboot.cpp Outdated Show resolved Hide resolved
@ryfu-msft
Copy link
Contributor Author

@microsoft-github-policy-service rerun

@ryfu-msft ryfu-msft merged commit 8964245 into microsoft:master Dec 6, 2023
8 checks passed
@ryfu-msft ryfu-msft deleted the registerRestart branch December 6, 2023 23:08
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.

None yet

2 participants