add tests and check for resume limit

schemas/JSON/settings/settings.schema.0.2.json

@@ -149,10 +149,10 @@
           "type": "string",
           "maxLength": "32767"
         },
-        "maxReboots": {
-          "description": "The maximum number of reboots allowed when performing a command. This is to prevent continuous reboots if a install that requires a reboot is not properly detected.",
+        "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": 3,
+          "default": 4,
           "minimum": 1
         }
       }

src/AppInstallerCLICore/CheckpointManager.cpp

@@ -82,6 +82,8 @@ namespace AppInstaller::Checkpoints
                 automaticCheckpoint.SetMany(AutomaticCheckpointData::Arguments, argument, values);
             }
         }
+
+        automaticCheckpoint.Set(AutomaticCheckpointData::ResumeCount, {}, std::to_string(0));
     }
 
     void LoadCommandArgsFromAutomaticCheckpoint(CLI::Execution::Context& context, Checkpoint<AutomaticCheckpointData>& automaticCheckpoint)

src/AppInstallerCLICore/Commands/ResumeCommand.cpp

@@ -99,15 +99,16 @@ namespace AppInstaller::CLI
             AICLI_TERMINATE_CONTEXT(APPINSTALLER_CLI_ERROR_CLIENT_VERSION_MISMATCH);
         }
 
-        int currentRebootCount = std::stoi(automaticCheckpoint.Get(AutomaticCheckpointData::ResumeCount, {}));
-        if (currentRebootCount > Settings::User().Get<Settings::Setting::MaxReboots>())
+        const auto& resumeCountString = automaticCheckpoint.Get(AutomaticCheckpointData::ResumeCount, {});
+        int resumeCount = std::stoi(resumeCountString);
+        if (resumeCount >= Settings::User().Get<Settings::Setting::MaxResumes>())
         {
-            context.Reporter.Error() << "Maximum number of reboot encountered. Terminating installation." << std::endl;
-            AICLI_TERMINATE_CONTEXT(APPINSTALLER_CLI_ERROR_INSTALLER_HASH_MISMATCH);
+            context.Reporter.Error() << Resource::String::ResumeLimitExceeded(Utility::LocIndView{ resumeCountString }) << std::endl;
+            AICLI_TERMINATE_CONTEXT(APPINSTALLER_CLI_ERROR_RESUME_LIMIT_EXCEEDED);
         }
         else
         {
-            automaticCheckpoint.Set(AutomaticCheckpointData::ResumeCount, {}, std::to_string(currentRebootCount + 1));
+            automaticCheckpoint.Update(AutomaticCheckpointData::ResumeCount, {}, std::to_string(resumeCount + 1));
         }
 
         const auto& checkpointCommand = automaticCheckpoint.Get(AutomaticCheckpointData::Command, {});

src/AppInstallerCLICore/Resources.h

@@ -424,6 +424,7 @@ namespace AppInstaller::CLI::Resource
         WINGET_DEFINE_RESOURCE_STRINGID(ResumeCommandShortDescription);
         WINGET_DEFINE_RESOURCE_STRINGID(ResumeIdArgumentDescription);
         WINGET_DEFINE_RESOURCE_STRINGID(ResumeIdNotFoundError);
+        WINGET_DEFINE_RESOURCE_STRINGID(ResumeLimitExceeded);
         WINGET_DEFINE_RESOURCE_STRINGID(ResumeStateDataNotFoundError);
         WINGET_DEFINE_RESOURCE_STRINGID(RetroArgumentDescription);
         WINGET_DEFINE_RESOURCE_STRINGID(SearchCommandLongDescription);

src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw

@@ -2622,4 +2622,8 @@ Please specify one of them using the --source option to proceed.</value>
   <data name="FailedToRegisterReboot" xml:space="preserve">
     <value>Failed to register winget for reboot.</value>
   </data>
+  <data name="ResumeLimitExceeded" xml:space="preserve">
+    <value>Resume operation exceeds the allowable limit of {0} resume(s).</value>
+    <comment>{Locked="{0}"} {0} is a placeholder that is replaced by an integer number of the number of allowed resumes.</comment>
+  </data>
 </root>

src/AppInstallerCLITests/CheckpointDatabase.cpp

@@ -34,7 +34,7 @@ TEST_CASE("CheckpointDatabaseCreateLatestAndReopen", "[checkpointDatabase]")
     }
 }
 
-TEST_CASE("CheckpointDatabase_WriteMetadata", "[checkpointDatabase]")
+TEST_CASE("CheckpointDatabase_WriteAndRemoveMetadata", "[checkpointDatabase]")
 {
     TempFile tempFile{ "repolibtest_tempdb"s, ".db"s };
     INFO("Using temporary file named: " << tempFile.GetPath());
@@ -58,6 +58,18 @@ TEST_CASE("CheckpointDatabase_WriteMetadata", "[checkpointDatabase]")
         auto checkpointId = checkpointIds[0];
         REQUIRE(testCommand == database->GetDataFieldSingleValue(checkpointId, AutomaticCheckpointData::Command, {}));
         REQUIRE(testClientVersion == database->GetDataFieldSingleValue(checkpointId, AutomaticCheckpointData::ClientVersion, {}));
+
+        database->RemoveDataType(checkpointId, AutomaticCheckpointData::Command);
+        database->RemoveDataType(checkpointId, AutomaticCheckpointData::ClientVersion);
+    }
+
+    {
+        std::shared_ptr<CheckpointDatabase> database = CheckpointDatabase::Open(tempFile);
+        const auto& checkpointIds = database->GetCheckpointIds();
+        REQUIRE_FALSE(checkpointIds.empty());
+
+        auto checkpointId = checkpointIds[0];
+        REQUIRE(database->GetDataTypes(checkpointId).empty());
     }
 }
 

src/AppInstallerCLITests/ResumeFlow.cpp

@@ -9,6 +9,7 @@
 #include <AppInstallerStrings.h>
 #include <AppInstallerVersions.h>
 #include <CheckpointManager.h>
+#include <Workflows/ShellExecuteInstallerHandler.h>
 
 using namespace std::string_literals;
 using namespace AppInstaller::CLI;
@@ -186,7 +187,7 @@ TEST_CASE("ResumeFlow_InstallFailure", "[Resume]")
     REQUIRE(std::filesystem::is_empty(tempCheckpointRecordDirectoryPath));
 }
 
-TEST_CASE("ResumeFlow_WindowsFeature_RebootRequired", "[Resume]")
+TEST_CASE("ResumeFlow_WindowsFeature_RebootFailures", "[Resume][windowsFeature]")
 {
     if (!AppInstaller::Runtime::IsRunningAsAdmin())
     {
@@ -204,17 +205,19 @@ TEST_CASE("ResumeFlow_WindowsFeature_RebootRequired", "[Resume]")
     testSettings.Set<Setting::EFReboot>(true);
     testSettings.Set<Setting::EFWindowsFeature>(true);
 
+    std::ostringstream installOutput;
+    TestContext context{ installOutput, std::cin };
+    auto previousThreadGlobals = context.SetForCurrentThread();
+    OverrideOpenDependencySource(context);
+
     // Override with reboot required HRESULT.
     auto doesFeatureExistOverride = TestHook::SetDoesWindowsFeatureExistResult_Override(ERROR_SUCCESS);
     auto setEnableFeatureOverride = TestHook::SetEnableWindowsFeatureResult_Override(ERROR_SUCCESS_REBOOT_REQUIRED);
-    TestHook::SetRegisterForRestartResult_Override registerForRestartResultOverride(false);
-    TestHook::SetInitiateRebootResult_Override initiateRebootResultOverride(true);
 
+    SECTION("Register for reboot fails")
     {
-        std::ostringstream installOutput;
-        TestContext context{ installOutput, std::cin };
-        auto previousThreadGlobals = context.SetForCurrentThread();
-        OverrideOpenDependencySource(context);
+        TestHook::SetRegisterForRestartResult_Override registerForRestartResultOverride(false);
+        TestHook::SetInitiateRebootResult_Override initiateRebootResultOverride(true);
 
         const auto& testManifestPath = TestDataFile("InstallFlowTest_WindowsFeatures.yaml").GetPath().u8string();
         context.Args.AddArg(Execution::Args::Type::Manifest, testManifestPath);
@@ -224,8 +227,96 @@ TEST_CASE("ResumeFlow_WindowsFeature_RebootRequired", "[Resume]")
         install.Execute(context);
         INFO(installOutput.str());
 
-        // Verify unsupported arguments error message is shown 
         REQUIRE(context.GetTerminationHR() == APPINSTALLER_CLI_ERROR_INSTALL_REBOOT_REQUIRED_TO_INSTALL);
         REQUIRE(installOutput.str().find(Resource::LocString(Resource::String::FailedToRegisterReboot).get()) != std::string::npos);
     }
+    SECTION("Initiate reboot fails")
+    {
+        TestHook::SetRegisterForRestartResult_Override registerForRestartResultOverride(true);
+        TestHook::SetInitiateRebootResult_Override initiateRebootResultOverride(false);
+
+        const auto& testManifestPath = TestDataFile("InstallFlowTest_WindowsFeatures.yaml").GetPath().u8string();
+        context.Args.AddArg(Execution::Args::Type::Manifest, testManifestPath);
+        context.Args.AddArg(Execution::Args::Type::AllowReboot);
+
+        InstallCommand install({});
+        install.Execute(context);
+        INFO(installOutput.str());
+
+        REQUIRE(context.GetTerminationHR() == APPINSTALLER_CLI_ERROR_INSTALL_REBOOT_REQUIRED_TO_INSTALL);
+        REQUIRE(installOutput.str().find(Resource::LocString(Resource::String::FailedToInitiateReboot).get()) != std::string::npos);
+    }
+}
+
+TEST_CASE("ResumeFlow_ResumeLimitExceeded", "[Resume]")
+{
+    if (!AppInstaller::Runtime::IsRunningAsAdmin())
+    {
+        WARN("Test requires admin privilege. Skipped.");
+        return;
+    }
+
+    TestCommon::TempDirectory tempCheckpointRecordDirectory("TempCheckpointRecordDirectory", false);
+
+    const auto& tempCheckpointRecordDirectoryPath = tempCheckpointRecordDirectory.GetPath();
+    TestHook_SetPathOverride(PathName::CheckpointsLocation, tempCheckpointRecordDirectoryPath);
+
+    TestCommon::TestUserSettings testSettings;
+    testSettings.Set<Setting::EFResume>(true);
+    testSettings.Set<Setting::EFReboot>(true);
+    testSettings.Set<Setting::EFWindowsFeature>(true);
+    testSettings.Set<Setting::MaxResumes>(1);
+
+    std::ostringstream installOutput;
+    TestContext context{ installOutput, std::cin };
+    auto previousThreadGlobals = context.SetForCurrentThread();
+
+    // Override with reboot required HRESULT.
+    auto doesFeatureExistOverride = TestHook::SetDoesWindowsFeatureExistResult_Override(ERROR_SUCCESS);
+    auto setEnableFeatureOverride = TestHook::SetEnableWindowsFeatureResult_Override(ERROR_SUCCESS_REBOOT_REQUIRED);
+
+    TestHook::SetRegisterForRestartResult_Override registerForRestartResultOverride(true);
+    TestHook::SetInitiateRebootResult_Override initiateRebootResultOverride(true);
+
+    const auto& testManifestPath = TestDataFile("InstallFlowTest_WindowsFeatures.yaml").GetPath().u8string();
+    context.Args.AddArg(Execution::Args::Type::Manifest, testManifestPath);
+    context.Args.AddArg(Execution::Args::Type::AllowReboot);
+    context.Args.AddArg(Execution::Args::Type::AcceptSourceAgreements);
+
+    InstallCommand install({});
+    context.SetExecutingCommand(&install);
+    install.Execute(context);
+    INFO(installOutput.str());
+
+    std::string resumeId;
+    for (const auto& entry : std::filesystem::directory_iterator(tempCheckpointRecordDirectoryPath))
+    {
+        // There should only be one checkpoint folder created.
+        resumeId = entry.path().filename().u8string();
+    }
+
+    {
+        // Execute resume once.
+        std::ostringstream resumeOutput;
+        TestContext resumeContext{ resumeOutput, std::cin };
+        previousThreadGlobals = resumeContext.SetForCurrentThread();
+        resumeContext.Args.AddArg(Execution::Args::Type::ResumeId, resumeId);
+
+        ResumeCommand resume({});
+        resume.Execute(resumeContext);
+    }
+
+    {
+        // Resume should fail as the resume limit has been exceeded.
+        std::ostringstream resumeOutput;
+        TestContext resumeContext{ resumeOutput, std::cin };
+        previousThreadGlobals = resumeContext.SetForCurrentThread();
+        resumeContext.Args.AddArg(Execution::Args::Type::ResumeId, resumeId);
+
+        ResumeCommand resume({});
+        resume.Execute(resumeContext);
+        INFO(resumeOutput.str());
+        REQUIRE(resumeContext.IsTerminated());
+        REQUIRE(resumeContext.GetTerminationHR() == APPINSTALLER_CLI_ERROR_RESUME_LIMIT_EXCEEDED);
+    }
 }

src/AppInstallerCommonCore/Public/winget/UserSettings.h

@@ -89,7 +89,7 @@ namespace AppInstaller::Settings
         DisableInstallNotes,
         PortablePackageUserRoot,
         PortablePackageMachineRoot,
-        MaxReboots,
+        MaxResumes,
         // Network
         NetworkDownloader,
         NetworkDOProgressTimeoutInSeconds,
@@ -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::MaxReboots, uint32_t, int, {}, ".installBehavior.maxReboots"sv);
+        SETTINGMAPPING_SPECIALIZATION(Setting::MaxResumes, uint32_t, int, {}, ".installBehavior.maxResumes"sv);
         // Uninstall behavior
         SETTINGMAPPING_SPECIALIZATION(Setting::UninstallPurgePortablePackage, bool, bool, false, ".uninstallBehavior.purgePortablePackage"sv);
         // Download behavior

src/AppInstallerCommonCore/UserSettings.cpp

@@ -269,7 +269,7 @@ namespace AppInstaller::Settings
         WINGET_VALIDATE_PASS_THROUGH(DisableInstallNotes)
         WINGET_VALIDATE_PASS_THROUGH(UninstallPurgePortablePackage)
         WINGET_VALIDATE_PASS_THROUGH(NetworkWingetAlternateSourceURL)
-        WINGET_VALIDATE_PASS_THROUGH(MaxReboots)
+        WINGET_VALIDATE_PASS_THROUGH(MaxResumes)
 
 #ifndef AICLI_DISABLE_TEST_HOOKS
         WINGET_VALIDATE_PASS_THROUGH(EnableSelfInitiatedMinidump)

src/AppInstallerRepositoryCore/Microsoft/CheckpointDatabase.cpp

@@ -77,6 +77,19 @@ namespace AppInstaller::Repository::Microsoft
         savepoint.Commit();
     }
 
+    void CheckpointDatabase::RemoveDataType(IdType checkpointId, int dataType)
+    {
+        std::lock_guard<std::mutex> lockInterface{ *m_interfaceLock };
+        AICLI_LOG(Repo, Verbose, << "Removing checkpoint data [" << dataType << "]");
+
+        SQLite::Savepoint savepoint = SQLite::Savepoint::Create(m_dbconn, "CheckpointDatabase_removeDataValue");
+
+        m_interface->RemoveCheckpointDataType(m_dbconn, checkpointId, dataType);
+
+        SetLastWriteTime();
+        savepoint.Commit();
+    }
+
     std::string CheckpointDatabase::GetDataFieldSingleValue(IdType checkpointId, int dataType, const std::string& field)
     {
         const auto& values = m_interface->GetCheckpointDataFieldValues(m_dbconn, checkpointId, dataType, field);

src/AppInstallerRepositoryCore/Microsoft/CheckpointDatabase.h

@@ -56,6 +56,9 @@ namespace AppInstaller::Repository::Microsoft
         // Gets multiple values for a data type field.
         std::vector<std::string> GetDataFieldMultiValue(IdType checkpointId, int dataType, const std::string& field);
 
+        // Removes the value(s) for a data type.
+        void RemoveDataType(IdType checkpointId, int dataType);
+
     private:
         // Constructor used to open an existing index.
         CheckpointDatabase(const std::string& target, SQLiteStorageBase::OpenDisposition disposition, Utility::ManagedFile&& indexFile);

src/AppInstallerRepositoryCore/Microsoft/Schema/Checkpoint_1_0/CheckpointDataTable.cpp

@@ -199,7 +199,7 @@ namespace AppInstaller::Repository::Microsoft::Schema::Checkpoint_V1_0
         }
     }
 
-    void CheckpointDataTable::RemoveData(SQLite::Connection& connection, SQLite::rowid_t checkpointId, int contextData)
+    void CheckpointDataTable::RemoveDataType(SQLite::Connection& connection, SQLite::rowid_t checkpointId, int contextData)
     {
         SQLite::Builder::StatementBuilder builder;
         builder.DeleteFrom(s_CheckpointDataTable_Table_Name).Where(s_CheckpointDataTable_CheckpointId_Column).Equals(checkpointId)

src/AppInstallerRepositoryCore/Microsoft/Schema/Checkpoint_1_0/CheckpointDataTable.h

@@ -34,7 +34,7 @@ namespace AppInstaller::Repository::Microsoft::Schema::Checkpoint_V1_0
         static bool HasDataField(SQLite::Connection& connection, SQLite::rowid_t checkpointId, int type, std::string_view name);
 
         // Removes the context data by checkpoint id.
-        static void RemoveData(SQLite::Connection& connection, SQLite::rowid_t checkpointId, int contextData);
+        static void RemoveDataType(SQLite::Connection& connection, SQLite::rowid_t checkpointId, int contextData);
 
         // Gets a single data value for a context data.
         static std::string GetDataValue(SQLite::Connection& connection, SQLite::rowid_t checkpointId, int type);

src/AppInstallerRepositoryCore/Microsoft/Schema/Checkpoint_1_0/CheckpointDatabaseInterface.h

@@ -19,5 +19,6 @@ namespace AppInstaller::Repository::Microsoft::Schema::Checkpoint_V1_0
         std::vector<std::string> GetCheckpointDataFields(SQLite::Connection& connection, SQLite::rowid_t checkpointId, int dataType) override;
         void SetCheckpointDataValues(SQLite::Connection& connection, SQLite::rowid_t checkpointId, int dataType, std::string_view name, const std::vector<std::string>& values) override;
         std::optional<std::vector<std::string>> GetCheckpointDataFieldValues(SQLite::Connection& connection, SQLite::rowid_t checkpointId, int dataType, std::string_view name) override;
+        void RemoveCheckpointDataType(SQLite::Connection& connection, SQLite::rowid_t checkpointId, int dataType) override;
     };
 }

src/AppInstallerRepositoryCore/Microsoft/Schema/Checkpoint_1_0/CheckpointDatabaseInterface_1_0.cpp

@@ -74,4 +74,9 @@ namespace AppInstaller::Repository::Microsoft::Schema::Checkpoint_V1_0
     {
         return CheckpointDataTable::GetDataValuesByFieldName(connection, checkpointId, dataType, name);
     }
+
+    void CheckpointDatabaseInterface::RemoveCheckpointDataType(SQLite::Connection& connection, SQLite::rowid_t checkpointId, int dataType)
+    {
+        CheckpointDataTable::RemoveDataType(connection, checkpointId, dataType);
+    }
 }

src/AppInstallerRepositoryCore/Microsoft/Schema/ICheckpointDatabase.h

@@ -37,5 +37,8 @@ namespace AppInstaller::Repository::Microsoft::Schema
 
         // Gets all field values for a checkpoint data type.
         virtual std::optional<std::vector<std::string>> GetCheckpointDataFieldValues(SQLite::Connection& connection, SQLite::rowid_t checkpointId, int dataType, std::string_view name) = 0;
+
+        // Removes all values for a checkpoint data type.
+        virtual void RemoveCheckpointDataType(SQLite::Connection& connection, SQLite::rowid_t checkpointId, int dataType) = 0;
     };
 }

src/AppInstallerRepositoryCore/Public/winget/Checkpoint.h

@@ -54,6 +54,13 @@ namespace AppInstaller::Checkpoints
             m_checkpointDatabase->SetDataValue(m_checkpointId, dataType, fieldName, values);
         }
 
+        // Update a single existing field value for a data type.
+        void Update(T dataType, const std::string& fieldName, const std::string& value)
+        {
+            m_checkpointDatabase->RemoveDataType(m_checkpointId, dataType);
+            m_checkpointDatabase->SetDataValue(m_checkpointId, dataType, fieldName, { value });
+        }
+
         // Gets a single field value for a data type.
         std::string Get(T dataType, const std::string& fieldName)
         {

src/AppInstallerSharedLib/Public/AppInstallerErrors.h

@@ -130,7 +130,7 @@
 #define APPINSTALLER_CLI_ERROR_CLIENT_VERSION_MISMATCH              ((HRESULT)0x8A15006F)
 #define APPINSTALLER_CLI_ERROR_INVALID_RESUME_STATE                 ((HRESULT)0x8A150070)
 #define APPINSTALLER_CLI_ERROR_CANNOT_OPEN_CHECKPOINT_INDEX         ((HRESULT)0x8A150071)
-#define APPINSTALLER_CLI_ERROR_RESUME_EXCEEDS_LIMIT                 ((HRESULT)0x8A150072)
+#define APPINSTALLER_CLI_ERROR_RESUME_LIMIT_EXCEEDED                ((HRESULT)0x8A150072)
 // Install errors.
 #define APPINSTALLER_CLI_ERROR_INSTALL_PACKAGE_IN_USE                   ((HRESULT)0x8A150101)
 #define APPINSTALLER_CLI_ERROR_INSTALL_INSTALL_IN_PROGRESS              ((HRESULT)0x8A150102)