Honor 429 Retry-After #3718

msftrubengu · 2023-09-30T17:14:13Z

Change

This pull request honors 429 Retry-After for pre indexed sources and throws ServiceUnavailableException when the response is 429 or 503.

If there's a Retry-After header and its value is more than 60s or the date is more than 60s into the future
- Save the next time it can be updated in the source details and not try again for that command.
- Subsequent updates or open that source will be skipped if the time lapsed hasn't passed.
If there's a Retry-After header and its value is less than 60s or the date value is less than 60 seconds into the future
- Retry once after that amount of time.
If no Retry-After header
- Retry after a random amount of time.

For open, the failed to update source message will show up.
For update, a warning with Unavailable will show up.
The next available time will show up in the logs.

Test

I ran manual validation using Fiddler to intercept winget's source.msix and return 429 or 503 with different Retry-Header

Microsoft Reviewers: Open in CodeFlow

JohnMcPMS

Code that could still use generating the exceptions:

Repo\Rest\Schema\HttpClientHelper.cpp :: ValidateAndExtractResponse
Common\HttpStream\HttpClientWrapper.cpp :: both PopulateInfoAsync and SendHttpRequestAsync
Common\DODownloader.cpp :: Somewhere in here after figuring out how they tell us about a 429

Code that could still be updated to handle the exception:

Manifest retrieval
Installer download

For this change I would request exception generation 1 and 2 be implemented. The DO downloader is probably already handling this reasonably well, so we can wait until we have actual issues. I would also request that we handle it in the installer download, in the event that the user has set us to use wininet. The manifest retrieval has no retry, nor would it be easy or make much sense to integrate manifest retrieval with the index update.

src/AppInstallerCLICore/Workflows/SourceFlow.cpp

src/AppInstallerCommonCore/Downloader.cpp

JohnMcPMS · 2023-10-02T17:00:57Z

src/AppInstallerRepositoryCore/RepositorySource.cpp

+            AddOrUpdateResult result;
+
+            // Do not update if we are still before the update block time.
+            if (IsUpdateSuppressed(details))


I had intentionally left it open for a manual update to still attempt this. We are caught in the middle of respecting the user's request and the server's. I wouldn't mind it being "harder" for the client to do (aka --force), but I feel like we should leave that option open.

This is still here...

src/AppInstallerRepositoryCore/RepositorySource.cpp

Co-authored-by: JohnMcPMS <johnmcp@microsoft.com>

msftrubengu · 2023-10-03T01:37:52Z

Updated for

Repo\Rest\Schema\HttpClientHelper.cpp :: ValidateAndExtractResponse
Common\HttpStream\HttpClientWrapper.cpp :: both PopulateInfoAsync and SendHttpRequestAsync

For SendHttpRequestAsync is going to be more complicated to get the RetryCount without weird things.

I believe the installer download was already covered, no? If the setting is wininet it will go to WinINetDownloadToStream which I already implemented.

~~Missing manifest retrieval and verify ValidateAndExtractResponse~~

JohnMcPMS

I meant catching the exception in DownloadInstallerFile and waiting for the appropriate amount of time rather than the arbitrary (and short) time being waited for in that retry.

JohnMcPMS · 2023-10-03T17:29:58Z

src/AppInstallerCommonCore/Downloader.cpp

+                if (hre.code() == APPINSTALLER_CLI_ERROR_SERVICE_UNAVAILABLE)
+                {
+                    auto retryAfter = httpRandomAccessStream->RetryAfter();
+                    THROW_EXCEPTION(AppInstaller::Utility::ServiceUnavailableException(retryAfter));


Did you verify this one?

src/AppInstallerCommonCore/MsixInfo.cpp

src/AppInstallerCommonCore/Public/AppInstallerProgress.h

JohnMcPMS · 2023-10-03T17:36:33Z

src/AppInstallerRepositoryCore/RepositorySource.cpp

+            AddOrUpdateResult result;
+
+            // Do not update if we are still before the update block time.
+            if (IsUpdateSuppressed(details))


This is still here...

JohnMcPMS · 2023-10-03T17:42:18Z

For SendHttpRequestAsync is going to be more complicated to get the RetryCount without weird things.

Ok, the HEAD request handling it should hopefully cover it, and it is no longer the primary case anyway.

src/AppInstallerSharedLib/Public/AppInstallerErrors.h

src/AppInstallerRepositoryCore/SourceList.h

src/AppInstallerCommonCore/Downloader.cpp

Change This pull request honors 429 Retry-After for pre indexed sources and throws ServiceUnavailableException when the response is 429 or 503. If there's a Retry-After header and its value is more than 60s or the date is more than 60s into the future Save the next time it can be updated in the source details and not try again for that command. Subsequent updates or open that source will be skipped if the time lapsed hasn't passed. If there's a Retry-After header and its value is less than 60s or the date value is less than 60 seconds into the future Retry once after that amount of time. If no Retry-After header Retry after a random amount of time. For open, the failed to update source message will show up. For update, a warning with Unavailable will show up. The next available time will show up in the logs.

JohnMcPMS and others added 4 commits September 28, 2023 13:49

Beginnings

48018d1

Implemented for GetHeaders

1e14f41

Finish download code path and test locally

10273a2

move to its own func

770e768

msftrubengu requested a review from a team as a code owner September 30, 2023 17:14

This comment has been minimized.

Sign in to view

spelling

785fc18

JohnMcPMS reviewed Oct 2, 2023

View reviewed changes

msftrubengu and others added 5 commits October 2, 2023 12:08

Update src/AppInstallerCommonCore/Downloader.cpp

3fd962b

Co-authored-by: JohnMcPMS <johnmcp@microsoft.com>

Update src/AppInstallerCommonCore/Downloader.cpp

538a84c

Co-authored-by: JohnMcPMS <johnmcp@microsoft.com>

Update src/AppInstallerRepositoryCore/RepositorySource.cpp

cc79973

Co-authored-by: JohnMcPMS <johnmcp@microsoft.com>

Some PR feedback, missing more tests

d8c23b9

skip for open

5502ea9

Verify rest

66adaee

JohnMcPMS reviewed Oct 3, 2023

View reviewed changes

msftrubengu added 2 commits October 3, 2023 10:53

fix test build

cb77351

Installer and manifest

4c352ff

yao-msft reviewed Oct 4, 2023

View reviewed changes

src/AppInstallerSharedLib/Public/AppInstallerErrors.h Show resolved Hide resolved

msftrubengu added 2 commits October 3, 2023 18:44

test

17c5465

return code

8971424

ryfu-msft previously approved these changes Oct 4, 2023

View reviewed changes

throw installer retry done

d11cc8d

msftrubengu dismissed ryfu-msft’s stale review via d11cc8d October 4, 2023 20:01

yao-msft reviewed Oct 4, 2023

View reviewed changes

src/AppInstallerRepositoryCore/SourceList.h Outdated Show resolved Hide resolved

src/AppInstallerCommonCore/Downloader.cpp Outdated Show resolved Hide resolved

msftrubengu added 2 commits October 4, 2023 14:02

PR comments

45a2afe

try again

d90bcae

yao-msft approved these changes Oct 4, 2023

View reviewed changes

msftrubengu merged commit 21de160 into microsoft:master Oct 4, 2023
8 checks passed

msftrubengu mentioned this pull request Oct 4, 2023

Cherry pick 21de to 1.6 #3730

Merged

marticliment mentioned this pull request Oct 23, 2023

[BUG] WingetUI does not report GodotEngine.GodotEngine as upgradeable, but Winget does marticliment/UniGetUI#1519

Closed

4 tasks

denelon mentioned this pull request Nov 13, 2023

winget causing DDOS like behavior #3879

Closed

msftrubengu deleted the honor-response branch November 14, 2023 00:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Honor 429 Retry-After #3718

Honor 429 Retry-After #3718

msftrubengu commented Sep 30, 2023 •

edited by microsoft-github-policy-service bot

Loading

This comment has been minimized.

JohnMcPMS left a comment

JohnMcPMS Oct 2, 2023

JohnMcPMS Oct 3, 2023

msftrubengu commented Oct 3, 2023 •

edited

Loading

JohnMcPMS left a comment

JohnMcPMS Oct 3, 2023

msftrubengu Oct 3, 2023

JohnMcPMS Oct 3, 2023

JohnMcPMS commented Oct 3, 2023

Honor 429 Retry-After #3718

Honor 429 Retry-After #3718

Conversation

msftrubengu commented Sep 30, 2023 • edited by microsoft-github-policy-service bot Loading

Change

Test

Microsoft Reviewers: Open in CodeFlow

This comment has been minimized.

JohnMcPMS left a comment

Choose a reason for hiding this comment

JohnMcPMS Oct 2, 2023

Choose a reason for hiding this comment

JohnMcPMS Oct 3, 2023

Choose a reason for hiding this comment

msftrubengu commented Oct 3, 2023 • edited Loading

JohnMcPMS left a comment

Choose a reason for hiding this comment

JohnMcPMS Oct 3, 2023

Choose a reason for hiding this comment

msftrubengu Oct 3, 2023

Choose a reason for hiding this comment

JohnMcPMS Oct 3, 2023

Choose a reason for hiding this comment

JohnMcPMS commented Oct 3, 2023

msftrubengu commented Sep 30, 2023 •

edited by microsoft-github-policy-service bot

Loading

msftrubengu commented Oct 3, 2023 •

edited

Loading