Fix .NET SDK CLI download timeout#493
Conversation
Increase the default timeout for downloading the Copilot CLI tarball from 100s (the HttpClient default) to 600s (10 minutes) to handle slow or unreliable network conditions. The timeout is configurable via the CopilotCliDownloadTimeout MSBuild property. Fixes #451 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
SDK Consistency Review: ✅ No Action RequiredThis PR adds a configurable download timeout (default 10 minutes) to the .NET SDK's MSBuild CLI download process, addressing issue #451 where slow networks could cause build failures. Cross-SDK AnalysisThe change is .NET-specific and appropriate because each SDK has a different CLI acquisition strategy:
Finding: Potential Enhancement OpportunityWhile this PR correctly addresses the .NET SDK's needs, it does reveal that Python and Go build scripts lack explicit timeout configuration for their CLI downloads. This is not a blocking issue for this PR, but could be considered for future hardening:
Recommendation: Approve this PR as-is. The .NET SDK's configurable timeout makes it more resilient to network issues, which is valuable for enterprise build environments with proxies/firewalls. The other SDKs can consider similar improvements independently if network timeout issues are reported. This review checked for cross-SDK feature parity and API consistency. No changes needed to other SDKs.
|
There was a problem hiding this comment.
Pull request overview
This pull request attempts to fix issue #451 where the .NET SDK CLI download times out on slow or unreliable networks. The MSBuild DownloadFile task defaults to HttpClient's 100-second timeout, which can be insufficient for downloading the Copilot CLI tarball in some network conditions.
Changes:
- Added a new MSBuild property
CopilotCliDownloadTimeoutwith a default value of 600 (intended to be 10 minutes) - Made the timeout configurable via user's .csproj or Directory.Build.props
- Applied the timeout to the DownloadFile task using MSBuild property functions
Comments suppressed due to low confidence (1)
dotnet/src/build/GitHub.Copilot.SDK.targets:53
- The comment states "Timeout in seconds" but users might be confused about whether to specify seconds or milliseconds when overriding this property. Since the MSBuild DownloadFile task expects milliseconds, and this code will multiply the property value by 1000, the comment is correct. However, consider adding a clarifying note in the comment that the SDK automatically converts seconds to milliseconds internally, so users should always specify values in seconds. For example: "Specify the timeout in seconds (the SDK converts to milliseconds internally)."
<!-- Timeout in seconds for downloading the Copilot CLI tarball.
Override CopilotCliDownloadTimeout in your .csproj or Directory.Build.props if needed.
Default is 600 seconds (10 minutes) to handle slow or unreliable network conditions. -->
| <MakeDir Directories="$(_CopilotCacheDir)" Condition="!Exists('$(_CopilotCliBinaryPath)')" /> | ||
| <Message Importance="high" Text="Downloading Copilot CLI $(CopilotCliVersion) for $(_CopilotPlatform)..." Condition="!Exists('$(_CopilotCliBinaryPath)')" /> | ||
| <DownloadFile SourceUrl="$(_CopilotDownloadUrl)" DestinationFolder="$(_CopilotCacheDir)" DestinationFileName="copilot.tgz" | ||
| Timeout="$([System.Int32]::Parse('$(CopilotCliDownloadTimeout)'))" |
There was a problem hiding this comment.
The MSBuild DownloadFile task's Timeout parameter expects milliseconds, not seconds. The property is documented as "600 seconds (10 minutes)" but passing 600 directly to the Timeout parameter will be interpreted as 600 milliseconds (0.6 seconds), which will cause downloads to fail almost immediately.
The value should be multiplied by 1000 to convert from seconds to milliseconds. For example:
Timeout="$([System.Int32]::Parse('$(CopilotCliDownloadTimeout)') * 1000)"
This ensures that a value of 600 results in a 600,000 millisecond (600 second) timeout.
This issue also appears on line 51 of the same file.
| Timeout="$([System.Int32]::Parse('$(CopilotCliDownloadTimeout)'))" | |
| Timeout="$([System.Int32]::Parse('$(CopilotCliDownloadTimeout)') * 1000)" |
| <PropertyGroup> | ||
| <CopilotCliDownloadTimeout Condition="'$(CopilotCliDownloadTimeout)' == ''">600</CopilotCliDownloadTimeout> | ||
| </PropertyGroup> |
There was a problem hiding this comment.
Consider adding input validation for the CopilotCliDownloadTimeout property to ensure it's a positive integer. Invalid values (negative numbers, zero, or non-numeric strings) could cause build failures with cryptic error messages. You could add a validation target or use MSBuild's Error condition, for example:
<Error Condition="$([System.Int32]::Parse('$(CopilotCliDownloadTimeout)')) <= 0" Text="CopilotCliDownloadTimeout must be a positive integer (in seconds)." />
This would provide users with a clearer error message if they misconfigure the property.
Summary
Fixes #451.
The \DownloadFile\ MSBuild task defaults to 100s timeout (the .NET HttpClient default), which can cause downloads to fail on slow or unreliable networks.
Changes
Testing
Verified the file still parses as valid XML and the timeout property is correctly applied to the \DownloadFile\ task.