fix shutdown tests - #129853
Conversation
|
Tagging subscribers to this area: @dotnet/area-extensions-hosting |
There was a problem hiding this comment.
Pull request overview
This PR updates the Microsoft.Extensions.Hosting functional-test deployment infrastructure to make shutdown-related tests more reliable on constrained/Helix environments by hardening process launch and shutdown handling.
Changes:
- Add a retry loop when launching the self-hosted test process, and surface early-exit failures more consistently.
- Resolve the
dotnetmuxer path relative to the currently running shared framework (with PATH fallback) for Helix environments that don’t provide a globaldotnet. - Avoid
Process.HasExitedthrowing during shutdown/cleanup when theProcessobject was never successfully started.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/libraries/Microsoft.Extensions.Hosting/tests/FunctionalTests/IntegrationTesting/src/Deployers/SelfHostDeployer.cs | Adds start retry logic and reorders startup flow to reduce flaky failures from transient process-launch issues. |
| src/libraries/Microsoft.Extensions.Hosting/tests/FunctionalTests/IntegrationTesting/src/Deployers/ApplicationDeployer.cs | Improves dotnet muxer resolution for Helix and hardens shutdown checks against unstarted Process instances. |
| var started = new TaskCompletionSource<object>(); | ||
| var hostExitTokenSource = new CancellationTokenSource(); | ||
|
|
| if (IsRunning(hostProcess)) | ||
| { | ||
| Logger.LogWarning("Unable to terminate the host process with process Id '{processId}", hostProcess.Id); | ||
| } |
| } | ||
| } | ||
|
|
||
| // Launching the host process can fail transiently on constrained CI/Helix machines (for example a |
There was a problem hiding this comment.
Is this really the case?
RemoteExecutor is launching a ton of process in CI/Helix machines. It does not have a retry loop like this and we do not see a problems with that.
There was a problem hiding this comment.
Hi, I'm trying to find out, the PR was generated by Copilot, please take the comments as WIP.
| hostProcess.KillTree(); | ||
| if (!hostProcess.HasExited) | ||
| if (IsRunning(hostProcess)) | ||
| { | ||
| Logger.LogWarning("Unable to terminate the host process with process Id '{processId}", hostProcess.Id); | ||
| } |
| catch (InvalidOperationException) | ||
| { | ||
| return false; | ||
| } |
| { | ||
| Logger.LogError("Host process {processName} {pid} exited with code {exitCode} or failed to start.", startInfo.FileName, HostProcess.Id, HostProcess.ExitCode); | ||
| throw new Exception("Failed to start host"); | ||
| Logger.LogWarning("Attempt {attempt} of {maxAttempts} to start the host process failed; retrying in {delaySeconds}s. Exception: {exception}", | ||
| attempt, MaxAttempts, retryDelay.TotalSeconds, ex.ToString()); | ||
| process.Dispose(); | ||
| await Task.Delay(retryDelay); |
|
I'm disabling these tests in crossgen2 and native AOT testing in #130066 because this has been broken for too long and makes it difficult to find real issues. Please remove the disabling and run |
|
/azp run runtime-nativeaot-outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Workflow state for the Holistic Review Orchestrator. {
"version": 5,
"last_dispatched_commit": "25d32f0c830cecd53dcdacbf88f21022a2e5f57b",
"last_dispatched_base_ref": "main",
"last_dispatched_base_sha": "e3f7eec1b9f39efe769c3cd316181110546500f8",
"last_reviewed_commit": "25d32f0c830cecd53dcdacbf88f21022a2e5f57b",
"last_reviewed_base_ref": "main",
"last_reviewed_base_sha": "e3f7eec1b9f39efe769c3cd316181110546500f8",
"last_recorded_worker_run_id": "29679143706",
"review_attempt_commit": "",
"review_attempt_base_ref": "",
"review_attempt_count": 0,
"max_review_attempts": 5,
"review_history_format": "holistic-review-disclosure-v1",
"review_history": [
{
"commit": "25d32f0c830cecd53dcdacbf88f21022a2e5f57b",
"review_id": 4730523149
}
]
} |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Holistic Review
Motivation: Justified. Linked issue #129832 shows a real, reproducible failure: ShutDownIfAnyHostProcess calls Process.HasExited on a Process that was created but never successfully started, throwing InvalidOperationException ("No process is associated with this object") during Dispose, which masks the real launch failure. The stack trace in the issue matches the code paths touched here.
Approach: Reasonable and targeted for test infrastructure. The IsRunning helper that swallows InvalidOperationException directly addresses the throwing HasExited call; adding throw; in the launch catch surfaces the real start failure instead of hitting the misleading HasExited/ExitCode access later; RunContinuationsAsynchronously avoids inline continuations on the Exited event thread; and GetHostDotNetExecutable resolves the muxer from the running host/shared-framework layout for Helix where dotnet isn't on PATH. These are consistent with the codebase and low-risk.
Summary:
Detailed Findings
✅ Correctness — Core fix is correct
IsRunning (ApplicationDeployer.cs) correctly treats a never-started/disposed Process as not running by catching InvalidOperationException, which is exactly the exception from the issue's stack trace. Replacing both !hostProcess.HasExited checks in ShutDownIfAnyHostProcess with IsRunning makes cleanup non-throwing without masking real state. Adding throw; in StartSelfHostAsync's catch is the right complement: it propagates the genuine launch error rather than letting the subsequent HostProcess.HasExited access throw a misleading InvalidOperationException.
⚠️ Documentation — PR description claims retry logic that is not in the diff
The PR description states: "Add retry logic for self-host process launch (3 attempts with short delay) to tolerate transient CI/Helix launch failures." The current base-to-head diff contains no retry loop — StartAndCaptureOutAndErrToLogger is still called exactly once inside a single try/catch that now rethrows. Please update the description (it notes it was AI-generated) so it matches the actual change, or add the retry if it was intended. As written, the description overstates the change.
💡 Robustness — GetHostDotNetExecutable fallback assumptions (low confidence, test-only)
Two minor, non-blocking observations on the new helper:
- The muxer fallback assumes
dotnetlives exactly three directories abovetypeof(object).Assembly.Location(shared/Microsoft.NETCore.App/<version>→ root). This holds for the standard testhost/dotnet layout and self-contained builds may differ, but the method falls back toDotnetCommandName, so worst case is the prior behavior. Fine as-is. Process.GetCurrentProcess().MainModule?.FileNamecan throw on some restricted platforms; in the normal test host it is fine. Given this is test infrastructure and the failure mode is only a fallback, not worth guarding, but flagging for visibility.
✅ Concurrency — RunContinuationsAsynchronously and reordering
Switching started to TaskCreationOptions.RunContinuationsAsynchronously avoids running the awaiting continuation inline on the process Exited/output-reader thread, which is a good default. Moving the hostExitTokenSource declaration earlier is a no-op reorder with no semantic impact.
Note
This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.
Generated by Holistic Review · 59 AIC · ⌖ 10.9 AIC · ⊞ 10K
Fixes #129832
Summary
This change makes the functional test deployer more robust in Helix environments where dotnet is not on PATH and where process start can fail transiently.
Testing
Note
This PR description was generated with AI assistance.