verify restart and resume behavior

ryfu-msft · Nov 6, 2023 · d58ee56 · d58ee56
1 parent 76e6a7c
commit d58ee56
Show file tree

Hide file tree

Showing 7 changed files with 55 additions and 51 deletions.
diff --git a/schemas/JSON/settings/settings.schema.0.2.json b/schemas/JSON/settings/settings.schema.0.2.json
@@ -152,7 +152,7 @@
         "maxResumes": {
           "description": "The maximum number of resumes allowed for a single resume id. This is to prevent continuous reboots if an install that requires a reboot is not properly detected.",
           "type": "integer",
-          "default": 4,
+          "default": 3,
           "minimum": 1
         }
       }

diff --git a/src/AppInstallerCLICore/Command.cpp b/src/AppInstallerCLICore/Command.cpp
@@ -6,6 +6,7 @@
 #include <winget/UserSettings.h>
 #include <AppInstallerRuntime.h>
 #include <winget/Locale.h>
+#include <winget/Reboot.h>
 
 using namespace std::string_view_literals;
 using namespace AppInstaller::Utility::literals;
@@ -870,16 +871,45 @@ namespace AppInstaller::CLI
             ExecuteInternal(context);
         }
 
-        if (context.Args.Contains(Execution::Args::Type::OpenLogs))
-        {   
-            // TODO: Consider possibly adding functionality that if the context contains 'Execution::Args::Type::Log' to open the path provided for the log
-            // 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) &&
+            context.Args.Contains(Execution::Args::Type::AllowReboot) &&
+            WI_IsFlagSet(context.GetFlags(), Execution::ContextFlag::RebootRequired))
+        {
+            context.Reporter.Warn() << Resource::String::InitiatingReboot << std::endl;
+
+            if (context.Args.Contains(Execution::Args::Type::Wait))
+            {
+                context.Reporter.PromptForEnter();
+            }
+
+            context.ClearFlags(Execution::ContextFlag::RebootRequired);
+
+            if (!Reboot::InitiateReboot())
+            {
+                context.Reporter.Error() << Resource::String::FailedToInitiateReboot << std::endl;
+            }
+            else if (WI_IsFlagSet(context.GetFlags(), Execution::ContextFlag::RegisterResume))
+            {
+                context.ClearFlags(Execution::ContextFlag::RegisterResume);
 
-        if (context.Args.Contains(Execution::Args::Type::Wait))
+                // For Windows Error Reporting to restart this process, the process must be rebooted while it is still running.
+                // Workaround is to have the program sleep while the reboot process is kicked off. Only applies to resume.
+                Sleep(5000);
+            }
+        }
+        else
         {
-            context.Reporter.PromptForEnter();
+            if (context.Args.Contains(Execution::Args::Type::OpenLogs))
+            {
+                // TODO: Consider possibly adding functionality that if the context contains 'Execution::Args::Type::Log' to open the path provided for the log
+                // 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 (context.Args.Contains(Execution::Args::Type::Wait))
+            {
+                context.Reporter.PromptForEnter();
+            }
         }
     }
 

diff --git a/src/AppInstallerCLICore/Workflows/InstallFlow.cpp b/src/AppInstallerCLICore/Workflows/InstallFlow.cpp
@@ -597,8 +597,7 @@ namespace AppInstaller::CLI::Workflow
             Workflow::InstallDependencies <<
             Workflow::DownloadInstaller <<
             Workflow::InstallPackageInstaller <<
-            Workflow::RegisterForReboot() <<
-            Workflow::InitiateRebootIfApplicable();
+            Workflow::RegisterForReboot();
     }
 
     void EnsureSupportForInstall(Execution::Context& context)

diff --git a/src/AppInstallerCLICore/Workflows/ResumeFlow.cpp b/src/AppInstallerCLICore/Workflows/ResumeFlow.cpp
@@ -16,34 +16,6 @@ namespace AppInstaller::CLI::Workflow
         context.Checkpoint(m_checkpointName, m_contextData);
     }
 
-    void InitiateRebootIfApplicable::operator()(Execution::Context& context) const
-    {
-        if (!Settings::ExperimentalFeature::IsEnabled(Settings::ExperimentalFeature::Feature::Reboot))
-        {
-            return;
-        }
-
-        if (!context.Args.Contains(Execution::Args::Type::AllowReboot))
-        {
-            AICLI_LOG(CLI, Info, << "No reboot flag found; skipping reboot flow.");
-            return;
-        }
-
-        if (WI_IsFlagSet(context.GetFlags(), Execution::ContextFlag::RebootRequired))
-        {
-            context.ClearFlags(Execution::ContextFlag::RebootRequired);
-
-            if (Reboot::InitiateReboot())
-            {
-                context.Reporter.Warn() << Resource::String::InitiatingReboot << std::endl;
-            }
-            else
-            {
-                context.Reporter.Error() << Resource::String::FailedToInitiateReboot << std::endl;
-            }
-        }
-    }
-
     void RegisterForReboot::operator()(Execution::Context& context) const
     {
         if (!Settings::ExperimentalFeature::IsEnabled(Settings::ExperimentalFeature::Feature::Resume) && 

diff --git a/src/AppInstallerCLICore/Workflows/ResumeFlow.h b/src/AppInstallerCLICore/Workflows/ResumeFlow.h
@@ -23,17 +23,6 @@ namespace AppInstaller::CLI::Workflow
         std::vector<Execution::Data> m_contextData;
     };
 
-    // Initiates a reboot if applicable. This task always executes even if context terminates.
-    // Required Args: None
-    // Inputs: None
-    // Outputs: None
-    struct InitiateRebootIfApplicable : public WorkflowTask
-    {
-        InitiateRebootIfApplicable() : WorkflowTask("InitiateRebootIfApplicable", /* executeAlways */ true) {}
-
-        void operator()(Execution::Context& context) const override;
-    };
-
     // Registers the resume command to execute upon reboot if applicable. This task always executes even if context terminates.
     // Required Args: None
     // Inputs: None

diff --git a/src/AppInstallerCLITests/UserSettings.cpp b/src/AppInstallerCLITests/UserSettings.cpp
@@ -562,3 +562,17 @@ TEST_CASE("SettingsInstallScope", "[settings]")
         REQUIRE(userSettingTest.Get<Setting::InstallScopeRequirement>() == AppInstaller::Manifest::ScopeEnum::Machine);
     }
 }
+
+TEST_CASE("SettingsMaxResumes", "[settings]")
+{
+    auto again = DeleteUserSettingsFiles();
+
+    SECTION("Modify max number of resumes")
+    {
+        std::string_view json = R"({ "installBehavior": { "maxResumes": 5 } })";
+        SetSetting(Stream::PrimaryUserSettings, json);
+        UserSettingsTest userSettingTest;
+
+        REQUIRE(userSettingTest.Get<Setting::MaxResumes>() == 5);
+    }
+}
diff --git a/src/AppInstallerCommonCore/Public/winget/UserSettings.h b/src/AppInstallerCommonCore/Public/winget/UserSettings.h
@@ -168,7 +168,7 @@ namespace AppInstaller::Settings
         SETTINGMAPPING_SPECIALIZATION(Setting::PortablePackageUserRoot, std::string, std::filesystem::path, {}, ".installBehavior.portablePackageUserRoot"sv);
         SETTINGMAPPING_SPECIALIZATION(Setting::PortablePackageMachineRoot, std::string, std::filesystem::path, {}, ".installBehavior.portablePackageMachineRoot"sv);
         SETTINGMAPPING_SPECIALIZATION(Setting::InstallDefaultRoot, std::string, std::filesystem::path, {}, ".installBehavior.defaultInstallRoot"sv);
-        SETTINGMAPPING_SPECIALIZATION(Setting::MaxResumes, uint32_t, int, {}, ".installBehavior.maxResumes"sv);
+        SETTINGMAPPING_SPECIALIZATION(Setting::MaxResumes, uint32_t, int, 3, ".installBehavior.maxResumes"sv);
         // Uninstall behavior
         SETTINGMAPPING_SPECIALIZATION(Setting::UninstallPurgePortablePackage, bool, bool, false, ".uninstallBehavior.purgePortablePackage"sv);
         // Download behavior