Bound aspire doctor checks with timeouts - #18969
Conversation
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 18969Or
iex "& { $(irm https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 18969" |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull request overview
Bounds aspire doctor environment checks and installation discovery, preventing indefinite hangs and orphaned child processes.
Changes:
- Adds per-check, aggregate, and discovery timeouts with warning metadata.
- Terminates canceled
dotnetand Unixcertutilprocess trees. - Adds timeout orchestration tests and localized messages.
Reviewed changes
Copilot reviewed 28 out of 29 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
tests/Aspire.Cli.Tests/Utils/EnvironmentCheckerTests.cs |
Tests check and aggregate timeouts. |
tests/Aspire.Cli.Tests/TestServices/TestWindowsRegistryReader.cs |
Adds registry test fake. |
tests/Aspire.Cli.Tests/TestServices/TestEnvironmentCheck.cs |
Adds configurable check fake. |
tests/Aspire.Cli.Tests/TestServices/TestCertificateToolRunner.cs |
Supports cancellation. |
tests/Aspire.Cli.Tests/TestServices/FakeInstallationDiscovery.cs |
Adds discovery callback seam. |
tests/Aspire.Cli.Tests/Commands/InstallationInfoOutputTests.cs |
Tests non-cooperative discovery timeout. |
src/Aspire.Cli/Utils/EnvironmentChecker/EnvironmentChecker.cs |
Implements bounded check orchestration. |
src/Aspire.Cli/Utils/EnvironmentChecker/DevCertsCheck.cs |
Propagates cancellation to certificate checks. |
src/Aspire.Cli/Resources/xlf/DoctorCommandStrings.zh-Hant.xlf |
Adds timeout resources. |
src/Aspire.Cli/Resources/xlf/DoctorCommandStrings.zh-Hans.xlf |
Adds timeout resources. |
src/Aspire.Cli/Resources/xlf/DoctorCommandStrings.tr.xlf |
Adds timeout resources. |
src/Aspire.Cli/Resources/xlf/DoctorCommandStrings.ru.xlf |
Adds timeout resources. |
src/Aspire.Cli/Resources/xlf/DoctorCommandStrings.pt-BR.xlf |
Adds timeout resources. |
src/Aspire.Cli/Resources/xlf/DoctorCommandStrings.pl.xlf |
Adds timeout resources. |
src/Aspire.Cli/Resources/xlf/DoctorCommandStrings.ko.xlf |
Adds timeout resources. |
src/Aspire.Cli/Resources/xlf/DoctorCommandStrings.ja.xlf |
Adds timeout resources. |
src/Aspire.Cli/Resources/xlf/DoctorCommandStrings.it.xlf |
Adds timeout resources. |
src/Aspire.Cli/Resources/xlf/DoctorCommandStrings.fr.xlf |
Adds timeout resources. |
src/Aspire.Cli/Resources/xlf/DoctorCommandStrings.es.xlf |
Adds timeout resources. |
src/Aspire.Cli/Resources/xlf/DoctorCommandStrings.de.xlf |
Adds timeout resources. |
src/Aspire.Cli/Resources/xlf/DoctorCommandStrings.cs.xlf |
Adds timeout resources. |
src/Aspire.Cli/Resources/DoctorCommandStrings.resx |
Defines timeout messages. |
src/Aspire.Cli/Resources/DoctorCommandStrings.Designer.cs |
Exposes generated resource properties. |
src/Aspire.Cli/DotNet/DotNetSdkInstaller.cs |
Kills canceled SDK probe trees. |
src/Aspire.Cli/Commands/InstallationInfoOutput.cs |
Bounds installation discovery. |
src/Aspire.Cli/Certificates/NativeCertificateToolRunner.cs |
Passes cancellation through trust checks. |
src/Aspire.Cli/Certificates/ICertificateToolRunner.cs |
Adds cancellation to the contract. |
src/Aspire.Cli/Certificates/CertificateService.cs |
Propagates caller cancellation. |
src/Aspire.Cli/Certificates/CertificateGeneration/UnixCertificateManager.cs |
Kills canceled certutil trees. |
Files not reviewed (1)
- src/Aspire.Cli/Resources/DoctorCommandStrings.Designer.cs: Generated file
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 31 out of 32 changed files in this pull request and generated no new comments.
Files not reviewed (1)
- src/Aspire.Cli/Resources/DoctorCommandStrings.Designer.cs: Generated file
Suppressed comments (6)
src/Aspire.Cli/DotNet/DotNetSdkInstaller.cs:65
- If process-tree termination throws
Win32ExceptionorNotSupportedException, this catch does not preserve the original cancellation; the outer catch at line 114 converts the cleanup failure into an SDK-not-found result. Cleanup is best-effort and must not mask cancellation, so handle the documented kill failures here and then rethrow the originalOperationCanceledException.
catch (InvalidOperationException)
{
// The process exited between cancellation and the kill attempt.
}
src/Aspire.Cli/Utils/EnvironmentChecker/EnvironmentChecker.cs:64
- The check is invoked even if the linked timeout token was already canceled (for example, caller cancellation before entry or the aggregate deadline expiring between checks).
WaitAsyncnotices only afterTask.Runhas scheduled the callback, so external work can start after cancellation and the aggregate path does not reliably stop scheduling remaining checks. Check the linked token inside thistrybefore invoking the check so the existing catch filters handle it.
var checkTask = Task.Run(() => check.CheckAsync(checkTimeoutCts.Token), CancellationToken.None);
src/Aspire.Cli/Commands/InstallationInfoOutput.cs:43
- A pre-canceled caller token still schedules
DiscoverAllCoreAsync, which runs the synchronous winget probe before discovery observes cancellation. This can mutate the install sidecar after the command was canceled. Check the linked token before scheduling the task, inside the existingtry, so caller cancellation propagates without starting discovery.
var discoveryTask = Task.Run(
() => DiscoverAllCoreAsync(discovery, wingetFirstRunProbe, logger, timeoutCts.Token),
CancellationToken.None);
src/Aspire.Cli/Certificates/NativeCertificateToolRunner.cs:19
- Cancellation is checked only while enumerating certificates. When the store is empty, the selector never runs and a pre-canceled call returns normally, so the newly added cancellation contract is not honored. Check the token before accessing/enumerating the certificate store.
public CertificateTrustResult CheckHttpCertificate(CancellationToken cancellationToken = default)
{
var availableCertificates = certificateManager.ListCertificates(
StoreName.My, StoreLocation.CurrentUser, isValid: true);
src/Aspire.Cli/DotNet/DotNetSdkInstaller.cs:60
- The timeout path becomes unbounded again while waiting for the process to exit after
Kill. A termination request does not guarantee prompt exit, so a stuck process can keepCheckAsyncat this await indefinitely despite cancellation. Use a separately bounded post-kill wait and always rethrow the original cancellation when that bound expires.
This issue also appears on line 62 of the same file.
process.Kill(entireProcessTree: true);
await process.WaitForExitAsync(CancellationToken.None);
src/Aspire.Cli/Certificates/CertificateGeneration/UnixCertificateManager.cs:747
- When the cancellation callback cannot kill
certutil(a case explicitly swallowed above), this blocking wait still has no bound and the caller never reachesThrowIfCancellationRequested. Cancellation can therefore hang indefinitely and leave the process tree running. Make the exit wait cancellation-aware or separately bounded so kill failure cannot block return.
process.WaitForExit();
cancellationToken.ThrowIfCancellationRequested();
Tests selector (audit mode)The full test matrix and all jobs still run in audit mode. The tests and jobs below are what selective CI would run under enforcement. 2 / 100 test projects · 5 jobs, from 32 changed files. Selected test projects (2 / 100)
Selected jobs (5)
How these were chosen — grouped by what changed📦 affected project 🧪 🧪 🧪 🧪 🧪 🧪 🧪 🧪 🧪 Job reasons
Selection computed for commit |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 31 out of 32 changed files in this pull request and generated no new comments.
Files not reviewed (1)
- src/Aspire.Cli/Resources/DoctorCommandStrings.Designer.cs: Generated file
Suppressed comments (2)
src/Aspire.Cli/Utils/EnvironmentChecker/EnvironmentChecker.cs:64
- Scheduling with
CancellationToken.Noneallows a queued check to begin after either deadline has already fired andCheckAllAsynchas reported the timeout. Under thread-pool starvation, that late check can continue running or spawn processes afteraspire doctorreturns. Pass the linked check token toTask.Runso work that has not started by the deadline is canceled rather than invoked.
var checkTask = Task.Run(() => check.CheckAsync(checkTimeoutCts.Token), CancellationToken.None);
src/Aspire.Cli/Commands/InstallationInfoOutput.cs:38
- Using
CancellationToken.Nonemeans installation discovery can still start after the 30-second timeout has returned a failed row. This is especially problematic during thread-pool starvation: the late task can enumerate installations and launch peer probes afteraspire doctorhas completed. Schedule it with the linked timeout token so queued work is suppressed once the deadline expires.
var discoveryTask = Task.Run(
() => DiscoverAllCoreAsync(discovery, wingetFirstRunProbe, logger, timeoutCts.Token),
CancellationToken.None);
PR Testing ReportPR Information
Artifact Version Verification
The official dogfood installer was attempted first with Changes AnalyzedChange Categories
Test Scenarios ExecutedScenario 1: Full Aspire CLI unit suiteObjective: Validate the current PR source across the complete CLI test project. Command: dotnet test --project .\tests\Aspire.Cli.Tests\Aspire.Cli.Tests.csproj --no-launch-profile --no-restore -- --filter-not-trait "quarantined=true" --filter-not-trait "outerloop=true"Result: 4,693 total; 4,643 passed; 50 platform-specific skipped; 0 failed. Duration 1m 39s. Scenario 2: Normal doctor text and JSON outputObjective: Verify normal doctor execution remains successful and structured output is valid. Results:
Evidence:
Scenario 3: Blocked dotnet probe timeout and process cleanupObjective: Verify a blocking Steps:
Results:
Expected Unhappy-Path Outcome: A warning result, continued doctor completion, and no orphaned parent or child process. Evidence:
Scenario 4: Linux certutil cancellationObjective: Verify a blocked NSS probe kills its certutil process tree. The user selected local Windows execution. This Linux-only product scenario cannot run on that target. The corresponding test compiles on Windows and is exercised by Linux CI. Scenario 5: Fresh project smokeObjective: Create, start, describe, and stop an The official installer stalled before creating the PR hive, and the verified native CLI archive contains only Summary
Overall ResultPARTIALLY VERIFIED The changed Windows doctor behavior passed both happy-path and targeted blocked-process testing, and the full CLI test project passed. Full cross-platform verification remains pending for the Linux-only certutil scenario, and the template smoke scenario could not run because the official installer did not complete hive creation. |
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
|
Docs were required for the See the workflow run for details: https://github.com/microsoft/aspire/actions/runs/30862723856 |
|
The CI build failed due to test failure(s) that appear unrelated to the PR changes. These may be flaky tests. Suspected flaky test(s):
Suggested actions:
You can re-run the failed jobs from the workflow run page. |
Description
aspire doctorcould remain atChecking Aspire environment...until an external job timeout when an environment check or installation discovery operation blocked indefinitely.This change bounds individual environment checks and installation discovery to 30 seconds, with a two-minute aggregate limit for environment checks. Individual timeouts are reported as warnings and later checks continue; reaching the aggregate limit reports a warning and stops scheduling remaining checks. Timeout details are also included in JSON output metadata.
Cancellation now terminates owned
dotnet --list-sdksand Unixcertutilprocess trees so timed-out checks do not leave child processes running.User-facing usage
When an individual check exceeds its deadline,
aspire doctorcontinues and reports:When the aggregate deadline is reached, it reports:
Validation:
dotnet test --project .\tests\Aspire.Cli.Tests\Aspire.Cli.Tests.csproj --no-launch-profile --no-restore -- --filter-class "*.DoctorCommandTests" --filter-class "*.EnvironmentCheckerTests" --filter-class "*.InstallationInfoOutputTests" --filter-class "*.DotNetSdkInstallerTests" --filter-class "*.DevCertsCheckTests" --filter-class "*.NativeCertificateToolRunnerTests" --filter-class "*.UnixCertificateManagerTests" --filter-class "*.CertificateServiceTests" --filter-not-trait "quarantined=true" --filter-not-trait "outerloop=true"(91 passed, 4 platform-specific tests skipped on Windows)dotnet build .\src\Aspire.Cli\Aspire.Cli.csproj --no-restoreFixes #18961
Checklist
<remarks />and<code />elements on your triple slash comments?