Source resilience #1470

JohnMcPMS · 2021-09-15T07:13:21Z

Fixes #1349, #1437 (although #1437 seems to have been re-opened more as a "the store is down" than "the client doesn't handle that well").

Change

Improves handling of errors from one source when multiple are configured by capturing the errors and including them as part of the source open and search results. Some commands will continue to fail if one of the sources has an error to prevent said error from affecting a change to the system. The commands that still fail are install, import, and upgrade --all|<specific> and they now indicate which source failed.

Examples of new behavior:

> wingetdev search terminal
Failed when searching source; results will not be included: msstore
Name                          Id                               Version     Match             Source
---------------------------------------------------------------------------------------------------
Windows Terminal              Microsoft.WindowsTerminal        1.10.2383.0 Moniker: terminal winget
...

> wingetdev install terminal
Failed when searching source: msstore
An unexpected error occurred while executing the command:
WinHttpSendRequest: 12007: The server name or address could not be resolved

The following packages were found among the working sources.
Please specify one of them using the `--source` option to proceed.
Name             Id                        Source
-------------------------------------------------
Windows Terminal Microsoft.WindowsTerminal winget

This change also replaces the experimental msstore source with the more official but still green msstore source (previously called storepreview). This source is now capable of providing basic search functionality against the entire Store catalog. It does not provide any correlation for installed packages however. In addition, the store source is being moved out of experimental.

Validation

Added test source type that can be configured to fail in specific ways to enable easier validation of changes.
Unit tests added to ensure proper error handling.
E2E test added using test source type.

Microsoft Reviewers: Open in CodeFlow

jedieaston · 2021-09-15T11:13:39Z

It seems to me, as a user, winget install should still work without adding a source argument even if one of the sources is down. If I understand the PR description correctly, if I do winget install terminal, and the Microsoft Store is down (as an example), it doesn't matter if there's a great version of Windows Terminal available from the default repo, I just get an error message saying that the msstore is down, which I then use as a hint to start doing winget search so I can find a repo that is up and has it.

I would think the expected behavior (coming from other package managers), would be that winget still installs whatever the closest thing it can find from available sources, since I presumably trust all of the sources I've added equally (or else I wouldn't have them).

If that's too unsafe, then maybe a hint under the error would be nice, like:

> wingetdev install terminal
Failed to open a source; try the 'source reset' command if the problem persists: msstore
An unexpected error occurred while executing the command:
WinHttpSendRequest: 12007: The server name or address could not be resolved

Note: A match was found in the "winget" source. To install it, run winget install terminal --source winget.
(or, if multiple sources have a match, it shows the table)

which would save the trouble of me having to search for someone with the package myself, since presumably winget install already knows everyone who should have it.

denelon · 2021-09-15T15:25:34Z

@JohnMcPMS what do you think about the suggestion from @jedieaston?

JohnMcPMS · 2021-09-15T16:06:52Z

If that's too unsafe

I don't think of it as a security issue, but rather a conservative take that we can't know what the user intent was. So if there is a terminal in multiple sources, but one of those is down, how are we to know that the user didn't actually want the unavailable one? Rather than performing a change to the system, I would rather fail "closed" for now. We can consider changes to that based on broad feedback when it is in everyone's hands.

maybe a hint under the error would be nice

This is a good idea and I will add it.

JohnMcPMS · 2021-09-15T18:27:32Z

This is a good idea and I will add it.

Ok, it is going to require a bit more work than I expected, but I think it will end up being a better experience overall anyway.

…able through project, add exploratory E2E test

JohnMcPMS · 2021-09-16T04:55:20Z

@jedieaston , I updated the PR description with the behavior of showing the search results from the not failing sources.

jedieaston · 2021-09-16T11:46:23Z

That looks awesome! Thanks!

yao-msft · 2021-09-17T03:15:32Z

nit, unrelated: Dependencies is missing

Refers to: schemas/JSON/settings/settings.schema.0.2.json:121 in c360e5d. [](commit_id = c360e5d, deletion_comment = False)

yao-msft · 2021-09-17T03:39:53Z

            Workflow::SearchSourceUsingManifest <<

I think this also performs a search against the sources, this is also used in upgrade command

but maybe it's ok we do not perform sophisticated handling for -m cases

Refers to: src/AppInstallerCLICore/Commands/UninstallCommand.cpp:119 in c360e5d. [](commit_id = c360e5d, deletion_comment = False)

yao-msft · 2021-09-17T03:44:55Z

src/AppInstallerRepositoryCore/Public/AppInstallerRepositorySource.h

        // Custom header for Rest sources
        std::optional<std::string> CustomHeader;

        // Source information containing source agreements, required/unsupported match fields.
        SourceInformation Information;
+
+        // Prevent correlation against this source if true.
+        bool DoNotCorrelateAgainst = false;


DoNotCorrelateAgainst

SupportsCorrelation?

yao-msft · 2021-09-17T03:49:29Z

src/AppInstallerCLICore/Commands/ExportCommand.cpp

        context <<
            Workflow::ReportExecutionStage(Workflow::ExecutionStage::Discovery) <<
            Workflow::OpenSource <<
            Workflow::OpenCompositeSource(Repository::PredefinedSource::Installed) <<
            Workflow::SearchSourceForMany <<
+            Workflow::HandleSearchResultFailures <<


HandleSearchResultFailures

This is called after every SearchSourceForMany or SearchSourceForSingle, shall we consolidate them inside?

I had considered that, but I'm not sure. I think that there is probably a case to be made to have an uber search task (or two) that takes a few well defined behavior flags and roll search + handle failure + ensure* since they are very much linked.

But given our timeframe I'm going to put that on the debt rather than change it here.

yao-msft · 2021-09-17T03:57:50Z

src/AppInstallerCLICore/Workflows/WorkflowBase.cpp

+                    error << Resource::String::SearchFailureError << ' ' << failure.Source->GetDetails().Name << std::endl;
+                    HRESULT failureHR = HandleException(context, failure.Exception);
+
+                    // Just take first failure for now


Just take first failure for now

Should we have a new hr for this instead of taking the first one?

I don't want to create a new failure bucket that is "multiple failures" yet.

yao-msft · 2021-09-17T04:03:54Z

src/AppInstallerCLIE2ETests/InstallCommand.cs

+                Assert.True(result.StdOut.Contains("Failed when searching source: failSearch"));
+                Assert.True(result.StdOut.Contains("AppInstallerTest.TestExeInstaller"));
+                Assert.False(result.StdOut.Contains("Successfully installed"));
+                Assert.False(VerifyTestExeInstalled(installDir, "/execustom"));


"/execustom"

not needed, we expect the installed file does not exist

yao-msft · 2021-09-17T05:39:49Z

src/AppInstallerCLICore/AppInstallerCLICore.vcxproj

@@ -226,6 +226,11 @@
      <SubSystem Condition="'$(Configuration)|$(Platform)'=='Release|x64'">Windows</SubSystem>
    </Link>
  </ItemDefinitionGroup>
+  <ItemDefinitionGroup Condition="'$(WingetDisableTestHooks)'=='true'">


WingetDisableTestHooks

Is this turned on in our official AppInstaller release build?

It will be after this is pulled in.

yao-msft · 2021-09-17T05:50:38Z

src/AppInstallerRepositoryCore/CompositeSource.cpp

+                        }
+
+                        // Move failures into the single result
+                        for (SearchResult::Failure& failure : availableResult.Failures)


availableResult.Failures

I think this is from a single source(not composite), curious in what scenario will this have Failures entries?

Ah, I saw this carries the OpenSource failure. Any reason not adding these failures to OpenSourceResult and handle there?

I had originally done that, but this pattern made it cleaner to show the suggestions on a failed install command.

yao-msft · 2021-09-17T05:51:38Z

src/AppInstallerRepositoryCore/CompositeSource.cpp

+            }
+
+            // Move failures into the single result
+            std::move(availableResult.Failures.begin(), availableResult.Failures.end(), std::back_inserter(result.Failures));


std::move

Should this use AddFailureIfSourceNotPresent to check duplicates?

Probably, although I put that in mostly as a guard against unbounded correlation failures.

yao-msft · 2021-09-17T06:01:19Z

src/AppInstallerRepositoryCore/RepositorySource.cpp

+                    {
+                        LOG_CAUGHT_EXCEPTION();
+                        AICLI_LOG(Repo, Warning, << "Failed to open source: " << source.get().Name);
+                        openExceptionProxies.emplace_back(std::make_shared<OpenExceptionProxy>(source, std::current_exception()));


openExceptionProxies.emplace_back(std::make_shared(source, std::current_exception()));

nit: I think we can just directly add the proxy to the aggregatedSource directly here instead of saving to a list and add all later? #ByDesign

never mind, there's a none available check below

yao-msft · 2021-09-17T06:05:16Z

src/AppInstallerCLIE2ETests/InstallCommand.cs

+        [Test]
+        public void InstallExeWithAlternateSourceFailure()
+        {
+            TestCommon.RunAICLICommand("source add", "failSearch \"{ \"\"SearchHR\"\": \"\"0x80070002\"\" }\" Microsoft.Test.Configurable --header \"{}\"");


""

I may be missing something but why double quotes for json key and value here, I thought it's regular json

The double double-quotes are to convince cmd to actually pass in a single double-quote to the process.

yao-msft · 2021-09-17T06:17:58Z

src/AppInstallerRepositoryCore/RepositorySource.cpp

                }

                if (sourceUpdated)
                {
                    sourceList.SaveMetadata();
                }

+                // If all sources failed to open, then throw an exception that is specific to this case.
+                THROW_HR_IF(APPINSTALLER_CLI_ERROR_FAILED_TO_OPEN_ALL_SOURCES, !aggregatedSource->HasAvailableSource());


HasAvailableSource

nit: there is GetAvailableSources().empty()

It returns a copy of the vector and I didn't need that.

yao-msft

yao-msft · 2021-09-17T21:06:00Z

src/AppInstallerRepositoryCore/RepositorySource.cpp

@@ -94,7 +93,7 @@ namespace AppInstaller::Repository
                details.Arg = s_Source_MSStoreDefault_Arg;
                details.Identifier = s_Source_MSStoreDefault_Identifier;
                details.TrustLevel = SourceTrustLevel::Trusted;
-                details.Restricted = true;
+                details.SupportCorrelation = true;


true

should be false here

revoking review

JohnMcPMS added 7 commits September 13, 2021 18:51

CC and building

83be626

Add task to workflows, add error message that names the failing source

2b671f9

Merge from master

10d28f6

Add tests

a30fbc0

Revert manual test helper

7b058a0

Revert more

c12ec33

Put store before winget

7669f3a

JohnMcPMS requested a review from a team as a code owner September 15, 2021 07:13

ghost added the Issue-Bug It either shouldn't be doing this or needs an investigation. label Sep 15, 2021

JohnMcPMS added 2 commits September 15, 2021 18:46

Refactor open failures and give the user more data on search failures

dd7ad86

Remove store experimental, update tests to account, add test hook dis…

6fa9975

…able through project, add exploratory E2E test

This comment has been minimized.

Sign in to view

JohnMcPMS added 2 commits September 15, 2021 22:46

Fix spelling, forgotten resource removed, fix tests, add test

4bd49ec

Clean up test source

07fa44f

denelon linked an issue Sep 16, 2021 that may be closed by this pull request

0x801901f4 : Internal server error (500) when I try to execute winget list and winget install (Source is down) #1437

Closed

Fix upgrade and import flows

c360e5d

yao-msft reviewed Sep 17, 2021

View reviewed changes

PR feedback

adf8539

yao-msft previously approved these changes Sep 17, 2021

View reviewed changes

yao-msft reviewed Sep 17, 2021

View reviewed changes

Fix semantic flip bug

5b4826a

yao-msft approved these changes Sep 17, 2021

View reviewed changes

JohnMcPMS merged commit 2e285f1 into microsoft:master Sep 18, 2021

JohnMcPMS deleted the sourceresilence branch September 18, 2021 06:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Source resilience #1470

Source resilience #1470

JohnMcPMS commented Sep 15, 2021 •

edited

Loading

jedieaston commented Sep 15, 2021 •

edited

Loading

denelon commented Sep 15, 2021

JohnMcPMS commented Sep 15, 2021

JohnMcPMS commented Sep 15, 2021

This comment has been minimized.

JohnMcPMS commented Sep 16, 2021

jedieaston commented Sep 16, 2021

yao-msft commented Sep 17, 2021

yao-msft commented Sep 17, 2021

yao-msft Sep 17, 2021

yao-msft Sep 17, 2021

JohnMcPMS Sep 17, 2021

yao-msft Sep 17, 2021

JohnMcPMS Sep 17, 2021

yao-msft Sep 17, 2021

yao-msft Sep 17, 2021

JohnMcPMS Sep 17, 2021

yao-msft Sep 17, 2021 •

edited

Loading

yao-msft Sep 17, 2021 •

edited

Loading

JohnMcPMS Sep 17, 2021

yao-msft Sep 17, 2021

JohnMcPMS Sep 17, 2021

yao-msft Sep 17, 2021 •

edited

Loading

yao-msft Sep 17, 2021

yao-msft Sep 17, 2021

JohnMcPMS Sep 17, 2021

yao-msft Sep 17, 2021

JohnMcPMS Sep 17, 2021

yao-msft left a comment

yao-msft Sep 17, 2021

Source resilience #1470

Source resilience #1470

Conversation

JohnMcPMS commented Sep 15, 2021 • edited Loading

Change

Validation

Microsoft Reviewers: Open in CodeFlow

jedieaston commented Sep 15, 2021 • edited Loading

denelon commented Sep 15, 2021

JohnMcPMS commented Sep 15, 2021

JohnMcPMS commented Sep 15, 2021

This comment has been minimized.

JohnMcPMS commented Sep 16, 2021

jedieaston commented Sep 16, 2021

yao-msft commented Sep 17, 2021

yao-msft commented Sep 17, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yao-msft Sep 17, 2021 • edited Loading

Choose a reason for hiding this comment

yao-msft Sep 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yao-msft Sep 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yao-msft left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JohnMcPMS commented Sep 15, 2021 •

edited

Loading

jedieaston commented Sep 15, 2021 •

edited

Loading

yao-msft Sep 17, 2021 •

edited

Loading

yao-msft Sep 17, 2021 •

edited

Loading

yao-msft Sep 17, 2021 •

edited

Loading