Bound Aspire CLI process termination wait - #19367
Bound Aspire CLI process termination wait#19367Karol Zadora-Przylecki (karolz-ms) wants to merge 6 commits into
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 19367Or
iex "& { $(irm https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 19367" |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull request overview
Bounds Aspire CLI process cleanup after command timeouts to prevent indefinite MSBuild hangs.
Changes:
- Adds a five-second post-termination wait and cleanup failure message.
- Extends the full-framework timeout assertion.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/Aspire.Hosting.Tasks/RunAspireCliCommand.cs |
Bounds process termination waits. |
tests/Aspire.Hosting.Sdk.Tests/AppHostSdkTargetsTests.cs |
Verifies the timeout failure message. |
Co-authored-by: karolz-ms <15271049+karolz-ms@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
Co-authored-by: karolz-ms <15271049+karolz-ms@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/Aspire.Hosting.Tasks/RunAspireCliCommand.cs:110
- This bounds only the root process wait. If the root exits while a descendant still owns the redirected stdout/stderr handles,
WaitForExitreturnstrueand the code immediately performs unbounded waits on bothReadToEndAsynctasks below.Kill(entireProcessTree: true)does not make the root's exit status represent descendant completion, so this can still leave MSBuild waiting indefinitely. Please include stream draining in the cleanup deadline (or stop draining/close the streams when that deadline expires).
if (!WaitForExit(process, ProcessTerminationTimeoutMilliseconds))
This comment has been minimized.
This comment has been minimized.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 851efffb-92bd-49d5-b3de-0d5a91446df8
This comment has been minimized.
This comment has been minimized.
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
PR Testing ReportPR Information
Artifact Version Verification
Changes AnalyzedFiles Changed
Change Categories
Test Scenarios ExecutedScenario 1: Dogfood artifact and fresh AppHost smoke testObjective: Verify the tested artifact matches the PR and can create, start, inspect, and stop a fresh AppHost. Coverage type: Happy path Status: PASS Steps and results:
Evidence:
Scenario 2: Multi-target task buildObjective: Verify the changed task compiles for both affected target frameworks. Coverage type: Build validation Status: PASS Results:
Evidence:
Scenario 3: Bounded timeout cleanup and normal command pathsObjective: Verify timeout cleanup cannot block on process exit or inherited output pipes, while successful setup paths still work. Coverage type: Happy path and unhappy path Status: PASS - 4 of 4 focused tests passed. Tests:
Expected unhappy-path outcomes confirmed:
Evidence:
Scenario 4: Real setup timeout and ASPIRE009 propagationObjective: Verify a real timed-out CLI bundle setup returns and emits the expected diagnostic rather than hanging. Coverage type: Unhappy path Status: PASS Test: Result: 1 of 1 test passed. The build reported ASPIRE009 and included the command timeout failure. Evidence:
Scenario 5: Hanging CLI setup followed by DNX fallbackObjective: Verify fallback can run after terminating a hanging PATH-resolved Aspire CLI. Coverage type: Recovery path and timing boundary Status: PASS WITH NON-BLOCKING TIMING OBSERVATION
The same test failed 5 of 5 times at the PR base commit with the same fake-DNX timeout. This demonstrates that the intermittent result is not introduced by PR #19367; the PR materially improves completion on this macOS host. Evidence:
Scenario 6: Full-framework Windows process-tree cleanupObjective: Cover the Coverage type: Platform-specific CI validation Status: PASS IN CI / SKIPPED LOCALLY AS DESIGNED The local test All PR checks reported by GitHub were passing: 362 passed and 2 skipped. Additional ObservationTwo extra Evidence:
Summary
Overall ResultPR VERIFIED The changed timeout behavior is bounded and reports useful failures, normal setup paths remain functional, both task target frameworks build, and the PR artifact runs a fresh AppHost successfully. No regression attributable to PR #19367 was found. RecommendationConsider increasing or separating the test-only 100 ms timeout in |
Karol Zadora-Przylecki (karolz-ms)
left a comment
Karol Zadora-Przylecki (karolz-ms)
left a comment
There was a problem hiding this comment.
Code review of the bounded process-termination change. 6 findings: 2 correctness/behavior regressions in RunAspireCliCommand, 1 test-coverage gap on the new bound, 1 environment-dependent test asset, 1 test resource leak, and 1 diagnosability regression.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6bddb089-22c9-491a-99d4-a566f5c3f3c4
This comment has been minimized.
This comment has been minimized.
PR Testing ReportPR Information
Artifact Version Verification
Changes AnalyzedFiles Changed
Change Categories
Test Scenarios ExecutedScenario 1: Exact PR artifact and normal AppHost smokeObjective: Verify the dogfood artifact matches the PR head and can create, build, start, inspect, and stop a fresh AppHost. Coverage Type: Happy path Status: PASS Steps:
Evidence:
Observations:
Scenario 2: Hanging CLI setup falls back to DNXObjective: Confirm a hanging path-based Aspire CLI setup is terminated and the build recovers through the DNX fallback without waiting indefinitely. Coverage Type: Unhappy path / recovery Status: PASS with local baseline sensitivity noted Steps:
Evidence:
Observations:
Expected Unhappy-Path Outcome: The hanging CLI setup is terminated, DNX creates the bundle, and the build succeeds. Scenario 3: Bounded failure reports ASPIRE009Objective: Confirm the no-usable-bundle path returns instead of hanging and reports a clear timeout diagnostic. Coverage Type: Unhappy path / diagnostic Status: PASS Steps:
Evidence:
Observations:
Expected Unhappy-Path Outcome: MSBuild returns a bounded failure containing Scenario 4: Full-framework process-tree terminationObjective: Confirm a timed-out Windows command shim and child process tree are terminated, a failure message is emitted, and inherited pipe handles do not leave MSBuild blocked. Coverage Type: Boundary / platform-specific unhappy path Status: PASS - Windows CI-backed Steps:
Evidence:
Observations:
Expected Unhappy-Path Outcome: The process tree exits, the timeout failure message is populated, and MSBuild completes. Supporting Build Validation
Summary
Overall ResultPASS - PR VERIFIED No PR-specific failure was found. The full PR CI run was still in progress when this report was written, but both relevant Hosting SDK jobs had completed successfully. Recommendations
|
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 288bc591-90b0-439d-85c2-562d720b0720
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. 44 / 101 test projects · 2 jobs, from 4 changed files. Selected test projects (44 / 101)
Selected jobs (2)
How these were chosen — grouped by what changed
🔧 show 36
🧪 📦 affected project 🧪 🧪 Job reasons
Selection computed for commit |
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
Description
Aspire CLI bundle setup commands that exceed their timeout should not be able to leave MSBuild waiting indefinitely while process termination completes asynchronously.
This change adds a fixed five-second post-termination wait. If the process still has not exited, the task reports the cleanup timeout through
FailureMessagewhile preserving the existing DNX fallback flow. The full-framework timeout regression now also verifies the emitted failure message.Validation:
Aspire.Hosting.Tasksfornet8.0andnet472.BuildFallsBackToDnxWhenPathAspireCliSetupTimesOut.RunAspireCliCommandKillsCommandShimProcessTreeOnTimeoutInFullFrameworkMsBuild.Fixes # (issue)
Checklist
<remarks />and<code />elements on your triple slash comments?