Workflow improvements to increase winget command success rate (#648)

.github/actions/spelling/expect.txt

@@ -326,3 +326,4 @@ Wunused
 WZDNCRFJ
 xf
 xl
+INET

src/AppInstallerCLICore/Resources.h

@@ -74,6 +74,9 @@ namespace AppInstaller::CLI::Resource
         WINGET_DEFINE_RESOURCE_STRINGID(InstallationRequiresHigherWindows);
         WINGET_DEFINE_RESOURCE_STRINGID(InstallCommandLongDescription);
         WINGET_DEFINE_RESOURCE_STRINGID(InstallCommandShortDescription);
+        WINGET_DEFINE_RESOURCE_STRINGID(InstallerBlockedByPolicy);
+        WINGET_DEFINE_RESOURCE_STRINGID(InstallerFailedSecurityCheck);
+        WINGET_DEFINE_RESOURCE_STRINGID(InstallerFailedVirusScan);
         WINGET_DEFINE_RESOURCE_STRINGID(InstallerHashMismatchAdminBlock);
         WINGET_DEFINE_RESOURCE_STRINGID(InstallerHashMismatchOverridden);
         WINGET_DEFINE_RESOURCE_STRINGID(InstallerHashMismatchOverrideRequired);
@@ -177,6 +180,7 @@ namespace AppInstaller::CLI::Resource
         WINGET_DEFINE_RESOURCE_STRINGID(SourceNameArgumentDescription);
         WINGET_DEFINE_RESOURCE_STRINGID(SourceOpenFailedSuggestion);
         WINGET_DEFINE_RESOURCE_STRINGID(SourceOpenPredefinedFailedSuggestion);
+        WINGET_DEFINE_RESOURCE_STRINGID(SourceOpenWithFailedUpdate);
         WINGET_DEFINE_RESOURCE_STRINGID(SourceRemoveAll);
         WINGET_DEFINE_RESOURCE_STRINGID(SourceRemoveCommandLongDescription);
         WINGET_DEFINE_RESOURCE_STRINGID(SourceRemoveCommandShortDescription);

src/AppInstallerCLICore/Workflows/InstallFlow.cpp

@@ -101,11 +101,40 @@ namespace AppInstaller::CLI::Workflow
 
         context.Reporter.Info() << "Downloading " << Execution::UrlEmphasis << installer.Url << std::endl;
 
-        auto hash = context.Reporter.ExecuteWithProgress(std::bind(Utility::Download,
-            installer.Url,
-            tempInstallerPath,
-            std::placeholders::_1,
-            true));
+        std::optional<std::vector<BYTE>> hash;
+
+        const int MaxRetryCount = 2;
+        for (int retryCount = 0; retryCount < MaxRetryCount; ++retryCount)
+        {
+            bool success = false;
+            try
+            {
+                hash = context.Reporter.ExecuteWithProgress(std::bind(Utility::Download,
+                    installer.Url,
+                    tempInstallerPath,
+                    std::placeholders::_1,
+                    true));
+
+                success = true;
+            }
+            catch (...)
+            {
+                if (retryCount < MaxRetryCount - 1)
+                {
+                    AICLI_LOG(CLI, Info, << "Failed to download, waiting a bit and retry. Url: " << installer.Url);
+                    Sleep(500);
+                }
+                else
+                {
+                    throw;
+                }
+            }
+
+            if (success)
+            {
+                break;
+            }
+        }
 
         if (!hash)
         {
@@ -135,9 +164,10 @@ namespace AppInstaller::CLI::Workflow
         }
         catch (const winrt::hresult_error& e)
         {
-            if (e.code() == HRESULT_FROM_WIN32(ERROR_NO_RANGES_PROCESSED))
+            if (e.code() == HRESULT_FROM_WIN32(ERROR_NO_RANGES_PROCESSED) ||
+                HRESULT_FACILITY(e.code()) == FACILITY_HTTP)
             {
-                // Server does not support range request, use download
+                // Failed to get signature hash through HttpStream, use download
                 downloadInstead = true;
             }
             else
@@ -210,7 +240,26 @@ namespace AppInstaller::CLI::Workflow
             else if (WI_IsFlagSet(context.GetFlags(), Execution::ContextFlag::InstallerHashMatched))
             {
                 const auto& installer = context.Get<Execution::Data::Installer>();
-                Utility::ApplyMotwUsingIAttachmentExecuteIfApplicable(context.Get<Execution::Data::InstallerPath>(), installer.value().Url);
+                HRESULT hr = Utility::ApplyMotwUsingIAttachmentExecuteIfApplicable(context.Get<Execution::Data::InstallerPath>(), installer.value().Url);
+
+                // Not using SUCCEEDED(hr) to check since there are cases file is missing after a successful scan
+                if (hr != S_OK)
+                {
+                    switch (hr)
+                    {
+                    case INET_E_SECURITY_PROBLEM:
+                        context.Reporter.Error() << Resource::String::InstallerBlockedByPolicy << std::endl;
+                        break;
+                    case E_FAIL:
+                        context.Reporter.Error() << Resource::String::InstallerFailedVirusScan << std::endl;
+                        break;
+                    default:
+                        context.Reporter.Error() << Resource::String::InstallerFailedSecurityCheck << std::endl;
+                    }
+
+                    AICLI_LOG(Fail, Error, << "Installer failed security check. Url: " << installer.value().Url << " Result: " << WINGET_OSTREAM_FORMAT_HRESULT(hr));
+                    AICLI_TERMINATE_CONTEXT(APPINSTALLER_CLI_ERROR_INSTALLER_SECURITY_CHECK_FAILED);
+                }
             }
         }
     }

src/AppInstallerCLICore/Workflows/WorkflowBase.cpp

@@ -105,7 +105,14 @@ namespace AppInstaller::CLI::Workflow
         std::shared_ptr<Repository::ISource> source;
         try
         {
-            source = context.Reporter.ExecuteWithProgress(std::bind(Repository::OpenSource, sourceName, std::placeholders::_1), true);
+            auto result = context.Reporter.ExecuteWithProgress(std::bind(Repository::OpenSource, sourceName, std::placeholders::_1), true);
+            source = result.Source;
+
+            // We'll only report the source update failure as warning and continue
+            for (const auto& s : result.SourcesWithUpdateFailure)
+            {
+                context.Reporter.Warn() << Resource::String::SourceOpenWithFailedUpdate << ' ' << s.Name << std::endl;
+            }
         }
         catch (...)
         {

src/AppInstallerCLIE2ETests/Constants.cs

@@ -52,6 +52,7 @@ public class ErrorCode
             public const int ERROR_NO_RANGES_PROCESSED = unchecked((int)0x80070138);
             public const int OPC_E_ZIP_MISSING_END_OF_CENTRAL_DIRECTORY = unchecked((int)0x8051100f);
             public const int ERROR_OLD_WIN_VERSION = unchecked((int)0x8007047e);
+            public const int HTTP_E_STATUS_NOT_FOUND = unchecked((int)0x80190194);
 
             // AICLI custom HRESULTs
             public const int ERROR_INTERNAL_ERROR = unchecked((int)0x8A150001);

src/AppInstallerCLIE2ETests/SourceCommand.cs

@@ -30,7 +30,7 @@ public void SourceAddWithInvalidURL()
         {
             // Add source with invalid url should fail
             var result = TestCommon.RunAICLICommand("source add", "AnotherSource https://microsoft.com");
-            Assert.AreEqual(Constants.ErrorCode.ERROR_NO_RANGES_PROCESSED, result.ExitCode);
+            Assert.AreEqual(Constants.ErrorCode.HTTP_E_STATUS_NOT_FOUND, result.ExitCode);
             Assert.True(result.StdOut.Contains("An unexpected error occurred while executing the command"));
         }
 

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

@@ -722,4 +722,16 @@ They can be configured through the settings file 'winget settings'.</value>
     <value>Logs</value>
     <comment>Diagnostic files containing information about application use.</comment>
   </data>
+  <data name="InstallerBlockedByPolicy" xml:space="preserve">
+    <value>The installer is blocked by policy</value>
+  </data>
+  <data name="InstallerFailedSecurityCheck" xml:space="preserve">
+    <value>The installer failed security check</value>
+  </data>
+  <data name="InstallerFailedVirusScan" xml:space="preserve">
+    <value>An anti-virus product reports an infection in the installer</value>
+  </data>
+  <data name="SourceOpenWithFailedUpdate" xml:space="preserve">
+    <value>Failed in attempting to update the source:</value>
+  </data>
 </root>

src/AppInstallerCLITests/Sources.cpp

@@ -510,7 +510,7 @@ TEST_CASE("RepoSources_UpdateOnOpen", "[sources]")
     SetSetting(Streams::UserSources, s_SingleSource);
 
     ProgressCallback progress;
-    auto source = OpenSource(name, progress);
+    auto source = OpenSource(name, progress).Source;
 
     REQUIRE(updateCalledOnFactory);
 
@@ -574,7 +574,7 @@ TEST_CASE("RepoSources_SearchAcrossMultipleSources", "[sources]")
     SetSetting(Streams::UserSources, s_TwoSource_AggregateSourceTest);
 
     ProgressCallback progress;
-    auto source = OpenSource("", progress);
+    auto source = OpenSource("", progress).Source;
 
     SearchRequest request;
     auto result = source->Search(request);

src/AppInstallerCommonCore/Downloader.cpp

@@ -1,6 +1,7 @@
 // Copyright (c) Microsoft Corporation.
 // Licensed under the MIT License.
 #include "pch.h"
+#include "Public/AppInstallerErrors.h"
 #include "Public/AppInstallerRuntime.h"
 #include "Public/AppInstallerDownloader.h"
 #include "Public/AppInstallerSHA256.h"
@@ -109,6 +110,12 @@ namespace AppInstaller::Utility
 
         dest.flush();
 
+        // Check download size matches if content length is provided in response header
+        if (contentLength > 0)
+        {
+            THROW_HR_IF(APPINSTALLER_CLI_ERROR_DOWNLOAD_SIZE_MISMATCH, bytesDownloaded != contentLength);
+        }
+
         std::vector<BYTE> result;
         if (computeHash)
         {
@@ -192,24 +199,26 @@ namespace AppInstaller::Utility
         AICLI_LOG(Core, Info, << "Finished applying motw");
     }
 
-    void ApplyMotwUsingIAttachmentExecuteIfApplicable(const std::filesystem::path& filePath, const std::string& source)
+    HRESULT ApplyMotwUsingIAttachmentExecuteIfApplicable(const std::filesystem::path& filePath, const std::string& source)
     {
         AICLI_LOG(Core, Info, << "Started applying motw using IAttachmentExecute to " << filePath);
 
         if (!IsNTFS(filePath))
         {
             AICLI_LOG(Core, Info, << "File system is not NTFS. Skipped applying motw");
-            return;
+            return S_OK;
         }
 
         // Attachment execution service needs STA to succeed, so we'll create a new thread and CoInitialize with STA.
+        HRESULT aesSaveResult = S_OK;
         auto updateMotw = [&]() -> HRESULT
         {
             Microsoft::WRL::ComPtr<IAttachmentExecute> attachmentExecute;
             RETURN_IF_FAILED(CoCreateInstance(CLSID_AttachmentServices, nullptr, CLSCTX_INPROC_SERVER, IID_PPV_ARGS(&attachmentExecute)));
             RETURN_IF_FAILED(attachmentExecute->SetLocalPath(filePath.c_str()));
             RETURN_IF_FAILED(attachmentExecute->SetSource(Utility::ConvertToUTF16(source).c_str()));
-            RETURN_IF_FAILED(attachmentExecute->Save());
+            aesSaveResult = attachmentExecute->Save();
+            RETURN_IF_FAILED(aesSaveResult);
             return S_OK;
         };
 
@@ -229,6 +238,8 @@ namespace AppInstaller::Utility
 
         aesThread.join();
 
-        AICLI_LOG(Core, Info, << "Finished applying motw using IAttachmentExecute. Result: " << hr);
+        AICLI_LOG(Core, Info, << "Finished applying motw using IAttachmentExecute. Result: " << hr << " IAttachmentExecute::Save() result: " << aesSaveResult);
+
+        return aesSaveResult;
     }
 }

src/AppInstallerCommonCore/Errors.cpp

@@ -103,6 +103,10 @@ namespace AppInstaller
                 return "No applicable update found";
             case APPINSTALLER_CLI_ERROR_UPDATE_ALL_HAS_FAILURE:
                 return "winget upgrade --all completed with failures";
+            case APPINSTALLER_CLI_ERROR_INSTALLER_SECURITY_CHECK_FAILED:
+                return "Installer failed security check";
+            case APPINSTALLER_CLI_ERROR_DOWNLOAD_SIZE_MISMATCH:
+                return "Download size does not match expected content length";
             default:
                 return "Unknown Error Code";
             }

src/AppInstallerCommonCore/HttpStream/HttpClientWrapper.cpp

@@ -45,7 +45,9 @@ namespace AppInstaller::Utility::HttpStream
 
         HttpResponseMessage response = co_await m_httpClient.SendRequestAsync(request, HttpCompletionOption::ResponseHeadersRead);
 
-        THROW_HR_IF(HRESULT_FROM_WIN32(ERROR_NO_RANGES_PROCESSED), response.StatusCode() != HttpStatusCode::Ok);
+        THROW_HR_IF(
+            MAKE_HRESULT(SEVERITY_ERROR, FACILITY_HTTP, response.StatusCode()),
+            response.StatusCode() != HttpStatusCode::Ok);
 
         // Get the length from the response
         if (response.Content().Headers().HasKey(L"Content-Length"))

src/AppInstallerCommonCore/Public/AppInstallerDownloader.h

@@ -45,5 +45,6 @@ namespace AppInstaller::Utility
 
     // Apply Mark of the web using IAttachmentExecute::Save if the target file is on NTFS, otherwise does nothing.
     // This method only does a best effort since Attachment Execution Service may be disabled.
-    void ApplyMotwUsingIAttachmentExecuteIfApplicable(const std::filesystem::path& filePath, const std::string& source);
+    // If IAttachmentExecute::Save is successfully invoked and the scan failed, the failure HRESULT is returned.
+    HRESULT ApplyMotwUsingIAttachmentExecuteIfApplicable(const std::filesystem::path& filePath, const std::string& source);
 }

src/AppInstallerCommonCore/Public/AppInstallerErrors.h

@@ -57,6 +57,8 @@
 #define APPINSTALLER_CLI_ERROR_INVALID_MANIFEST                 ((HRESULT)0x8A15002A)
 #define APPINSTALLER_CLI_ERROR_UPDATE_NOT_APPLICABLE            ((HRESULT)0x8A15002B)
 #define APPINSTALLER_CLI_ERROR_UPDATE_ALL_HAS_FAILURE           ((HRESULT)0x8A15002C)
+#define APPINSTALLER_CLI_ERROR_INSTALLER_SECURITY_CHECK_FAILED  ((HRESULT)0x8A15002D)
+#define APPINSTALLER_CLI_ERROR_DOWNLOAD_SIZE_MISMATCH           ((HRESULT)0x8A15002E)
 
 namespace AppInstaller
 {

src/AppInstallerCommonCore/Public/AppInstallerSynchronization.h

@@ -33,11 +33,14 @@ namespace AppInstaller::Synchronization
 
         static CrossProcessReaderWriteLock LockForWrite(std::string_view name);
 
+        bool WasAbandoned() { return m_wasAbandoned; }
+
     private:
         CrossProcessReaderWriteLock(std::string_view name);
 
         wil::unique_mutex m_mutex;
         wil::unique_semaphore m_semaphore;
         ResetWhenMovedFrom<LONG> m_semaphoreReleases{ 0 };
+        bool m_wasAbandoned = false;
     };
 }

src/AppInstallerCommonCore/Synchronization.cpp

@@ -46,7 +46,8 @@ namespace AppInstaller::Synchronization
 
         DWORD status = 0;
         auto lock = result.m_mutex.acquire(&status);
-        THROW_HR_IF(E_UNEXPECTED, status != WAIT_OBJECT_0);
+        THROW_HR_IF_NULL(E_UNEXPECTED, lock);
+        result.m_wasAbandoned = (status == WAIT_ABANDONED);
 
         for (LONG i = 0; i < s_CrossProcessReaderWriteLock_MaxReaders; ++i)
         {

src/AppInstallerRepositoryCore/Microsoft/SQLiteIndexSource.cpp

@@ -106,7 +106,34 @@ namespace AppInstaller::Repository::Microsoft
 
                     AICLI_LOG(Repo, Info, << "Downloading manifest");
                     ProgressCallback emptyCallback;
-                    (void)Utility::DownloadToStream(fullPath, manifestStream, emptyCallback);
+
+                    const int MaxRetryCount = 2;
+                    for (int retryCount = 0; retryCount < MaxRetryCount; ++retryCount)
+                    {
+                        bool success = false;
+                        try
+                        {
+                            (void)Utility::DownloadToStream(fullPath, manifestStream, emptyCallback);
+                            success = true;
+                        }
+                        catch (...)
+                        {
+                            if (retryCount < MaxRetryCount - 1)
+                            {
+                                AICLI_LOG(Repo, Info, << "Downloading manifest failed, waiting a bit and retrying: " << fullPath);
+                                Sleep(500);
+                            }
+                            else
+                            {
+                                throw;
+                            }
+                        }
+
+                        if (success)
+                        {
+                            break;
+                        }
+                    }
 
                     std::string manifestContents = manifestStream.str();
                     AICLI_LOG(Repo, Verbose, << "Manifest contents: " << manifestContents);

src/AppInstallerRepositoryCore/Public/AppInstallerRepositorySource.h

@@ -97,9 +97,18 @@ namespace AppInstaller::Repository
     // Adds a new source for the user.
     void AddSource(std::string_view name, std::string_view type, std::string_view arg, IProgressCallback& progress);
 
+    struct OpenSourceResult
+    {
+        // The ISource returned by OpenSource
+        std::shared_ptr<ISource> Source;
+
+        // List of SourceDetails that failed to update
+        std::vector<SourceDetails> SourcesWithUpdateFailure;
+    };
+
     // Opens an existing source.
     // Passing an empty string as the name of the source will return a source that aggregates all others.
-    std::shared_ptr<ISource> OpenSource(std::string_view name, IProgressCallback& progress);
+    OpenSourceResult OpenSource(std::string_view name, IProgressCallback& progress);
 
     // A predefined source.
     // These sources are not under the direct control of the user, such as packages installed on the system.