Fix DownloadFile to only log errors when all retries are exhausted #16212
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem
The
DownloadWithRetriesAsync
method inDownloadFile.cs
was callingLog.LogErrorFromException
during retry attempts, even when the download would eventually succeed. This caused MSBuild to mark builds as failed despite successful downloads after retrying.For example, in https://dev.azure.com/dnceng/internal/_build/results?buildId=2792361, a transient network error was logged as an error:
The download succeeded on a subsequent retry and produced the expected MSI, but the build was still marked as a failure due to the logged error.
Solution
Moved
Log.LogErrorFromException
from the retry loop to only execute after all retries are exhausted. Now:Log.LogMessage
Changes
Single line change in
src/Microsoft.DotNet.Arcade.Sdk/src/DownloadFile.cs
:Log.LogErrorFromException(e, true, true, null)
from inside the retry loop to inside theif (attempt > Retries)
failure blockThis ensures errors are only logged when the operation ultimately fails, not during intermediate retry attempts that may succeed.
Fixes the issue originally reported by @mmitche where transient network errors were incorrectly failing builds.
cc: @lewing @joeloff
Original prompt
This section details on the original issue you should resolve
<issue_title>DownloadWithRetriesAsync seems to retry, but still log an error?</issue_title>
<issue_description>Seems to be a retryable exception in https://dev.azure.com/dnceng/internal/_build/results?buildId=2792361&view=logs&j=89eea50f-59b5-5921-c3ce-ce40b36fa819&t=483e1309-93e4-5a7a-74de-93b8d658274f&l=11782
I assume it retried and succeeded since I see the build produce an MSI cc @joeloff
The build was still marked as a failure, most likely because of the logged exception:
arcade/src/Microsoft.DotNet.Arcade.Sdk/src/DownloadFile.cs
Line 222 in c32cd13
Pretty sure that LogError should only be in the
return false
path. @lewing you added in 025a2b3