Fix Windows detached AppHost launcher - #16561
Conversation
Use the Windows DETACHED_PROCESS creation flag when launching the detached child CLI process so aspire start is not tied to the launching console lifetime. Keep the existing new process group and restricted handle inheritance behavior. Co-authored-by: Copilot <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 -- 16561Or
iex "& { $(irm https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 16561" |
There was a problem hiding this comment.
Pull request overview
Fixes a Windows-specific detach bug in the Aspire CLI where the background (“detached”) AppHost could still be tied to the launching console and get terminated when the launching shell exits.
Changes:
- Adds the Windows
DETACHED_PROCESScreation flag to theCreateProcessWlaunch flags used for detached execution. - Centralizes the Windows detached creation flags into a single constant to preserve existing flag behavior while adding detach.
- Adds a regression unit test to ensure the Windows detached creation flags include both
DETACHED_PROCESSandCREATE_NEW_PROCESS_GROUP.
Show a summary per file
| File | Description |
|---|---|
| tests/Aspire.Cli.Tests/Processes/DetachedProcessLauncherTests.cs | Adds a regression test verifying the Windows detached process creation flags include the expected bits. |
| src/Aspire.Cli/Processes/DetachedProcessLauncher.Windows.cs | Adds DETACHED_PROCESS to the CreateProcess creation flags (via a shared constant) to ensure true console detachment on Windows. |
| src/Aspire.Cli/Processes/DetachedProcessLauncher.cs | Updates the high-level design comment to reflect the Windows detachment approach using DETACHED_PROCESS. |
Copilot's findings
- Files reviewed: 3/3 changed files
- Comments generated: 0
Eric Erhardt (eerhardt)
left a comment
Eric Erhardt (eerhardt)
left a comment
There was a problem hiding this comment.
Seems fine - if it works.
| const uint detachedProcess = 0x00000008; | ||
| const uint createNewProcessGroup = 0x00000200; | ||
|
|
||
| Assert.NotEqual(0u, DetachedProcessLauncher.WindowsDetachedProcessCreationFlags & detachedProcess); |
There was a problem hiding this comment.
This test is a little jank.
There was a problem hiding this comment.
The test just isnt required
Document that the detached Windows flag combination follows established daemonization patterns used by libuv/Node.js and GitHub CLI, without over-claiming Docker parity. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Only concern would be that |
aspire stop does want to send ctrl+c. Though it does fallback to process kill now I believe. |
Just checked the code; it looks like we DO try to send a signal, but we do it in a way that probably won't actually work (trying to send CTRL+BREAK to a process when we don't necessarily share the same console group). Long term if we want to try for graceful shutdown, we should do |
Remove the unit test coverage for the Windows detached process creation flags while keeping the implementation change and manual repro validation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
I added a followup issue to get |
|
Re-running the failed jobs in the CI workflow for this pull request because 1 job was identified as retry-safe transient failures in the CI run attempt.
|
|
🎬 CLI E2E Test Recordings — 76 recordings uploaded (commit View all recordings
📹 Recordings uploaded automatically from CI run #25126570121 |
|
No documentation PR is required for this change. This is an internal bug fix that adds the
|
|
/backport to release/13.3 |
|
Started backporting to |
* Fix Windows detached AppHost launcher Use the Windows DETACHED_PROCESS creation flag when launching the detached child CLI process so aspire start is not tied to the launching console lifetime. Keep the existing new process group and restricted handle inheritance behavior. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Clarify Windows detach launcher comment Document that the detached Windows flag combination follows established daemonization patterns used by libuv/Node.js and GitHub CLI, without over-claiming Docker parity. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Remove Windows detach unit test Remove the unit test coverage for the Windows detached process creation flags while keeping the implementation change and manual repro validation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Description
Fixes #16559
aspire startandaspire run --detachon Windows already launch the detached child CLI withCREATE_NEW_PROCESS_GROUPand restricted handle inheritance, but they did not setDETACHED_PROCESS. That can leave the child attached to the launching console, so the AppHost can be terminated when the launcher shell exits.This change adds the Windows
DETACHED_PROCESScreation flag while preserving the existingCREATE_NEW_PROCESS_GROUP, Unicode environment, extended startup info, and NUL stdout/stderr handle behavior.Validation:
Checklist
<remarks />and<code />elements on your triple slash comments?aspire.devissue: