Skip to content

[release/13.6] Fix Windows AppHost cleanup job routing - #20514

Merged
Jose Perez Rodriguez (joperezr) merged 1 commit into
release/13.6from
danegsta-backport-windows-cleanup
Sep 30, 2026
Merged

Jose Perez Rodriguez (joperezr) merged 1 commit into
release/13.6from
danegsta-backport-windows-cleanup

Conversation

@danegsta

@danegsta David Negstad (danegsta) commented Sep 26, 2026 •

Copy link
Copy Markdown
Member

Description

Backport of #20494 to release/13.6 (cherry-picked patch from e62dceba954899a33326297b4669e226c27dcdce). On Windows, .NET AppHosts launched through dotnet run or dotnet watch do not enter the Aspire CLI's outer kill-on-close job, allowing DCP to finish removing session-scoped resources during shutdown. Direct .NET launches and TypeScript/polyglot AppHost servers retain their job protection. Includes the associated CLI tests and extension E2E test stabilization. No new dependencies.

Related: #20495 (already closed by #20494).

Checklist

  • Is this feature complete?
    • Yes. Ready to ship.
    • No. Follow-up changes expected.
  • Are you including unit tests for the changes and scenario tests if relevant?
    • Yes
    • No
  • Did you add public API?
    • Yes
      • If yes, did you have an API Review for it?
        • Yes
        • No
      • Did you add <remarks /> and <code /> elements on your triple slash comments?
        • Yes
        • No
    • No
  • Does the change make any security assumptions or guarantees?
    • Yes
      • If yes, have you done a threat model and had a security review?
        • Yes
        • No
    • No

Customer Impact

On Windows, stopping a CLI-launched .NET AppHost can leave session-scoped containers running instead of removing them; persistent resources should remain running.

Testing

Main PR: manual C# normal/forced shutdown and TypeScript cleanup, focused CLI tests (181 passed, 3 platform-skipped), and passing Windows edge-cases CI. Release branch: targeted CLI tests (172 passed, 3 Windows platform-skipped), CLI/extension build, extension test and E2E type-check, and changed-file lint. Full VS Code E2E UI run was not performed on the release branch.

Risk

Medium. Changes Windows process ownership for dotnet run/dotnet watch; direct .NET and TypeScript/polyglot paths retain their kill-on-close protection, and CLI-parent detection remains enabled.

Regression?

Unknown. Reproduced on a 13.6 preview build; the exact introducing version/change is not established.

Backport of #20494 to release/13.6. Preserve .NET fallback cleanup while retaining direct-launch and AppHost server job protection.

(cherry picked from commit e62dceb)

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@github-actions

Copy link
Copy Markdown
Contributor

🚀 Dogfood this PR with:

⚠️ WARNING: Do not do this without first carefully reviewing the code of this PR to satisfy yourself it is safe.

curl -fsSL https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 20514

Or

  • Run remotely in PowerShell:
iex "& { $(irm https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 20514"

@github-actions

Copy link
Copy Markdown
Contributor

Tests selector

2 / 99 PR test projects · 5 PR jobs, from 5 changed files.

Selected PR test projects (2 / 99)

Aspire.Cli.EndToEnd.Tests, Aspire.Cli.Tests

Selected PR jobs (5)

cli-starter-validation, extension-e2e, extension-unit, polyglot, typescript-api-compat


How these were chosen — grouped by what changed

📦 affected project Aspire.Cli
→ 1 test: Aspire.Cli.EndToEnd.Tests

🧪 tests/Aspire.Cli.Tests/Projects/AppHostServerSessionTests.cs (changed test)
→ 1 directly: Aspire.Cli.Tests

🧪 tests/Aspire.Cli.Tests/Projects/DotNetAppHostProjectTests.cs (changed test)
→ 1 directly: Aspire.Cli.Tests

Job reasons

Job Triggered by
cli-starter-validation affected project Aspire.Cli
extension-e2e • extension/src/test-e2e/edgeCases.e2e.test.ts, src/Aspire.Cli/Processes/WindowsConsoleProcessJob.cs, src/Aspire.Cli/Projects/DotNetAppHostProject.cs
• affected project Aspire.Cli
extension-unit extension/src/test-e2e/edgeCases.e2e.test.ts
polyglot affected project Aspire.Cli
typescript-api-compat affected project Aspire.Cli

Selection computed for commit bce23f3.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟢 Approval recommended

The launch-path routing matches the reported Windows failure and has focused regression coverage.

Review effort: Balanced
Findings: None

What changed in this PR

Backports the Windows AppHost cleanup fix by adjusting kill-on-close job routing while preserving protection for direct and polyglot launches.

Changes:

  • Disables the outer Windows job for dotnet run/watch paths.
  • Retains job protection for direct .NET and AppHost server launches.
  • Adds focused tests and stabilizes extension E2E targeting.
File Description
src/​Aspire.Cli/​Projects/​DotNetAppHostProject.cs Selects job protection based on launch path.
src/​Aspire.Cli/​Processes/​WindowsConsoleProcessJob.cs Documents nested-job constraints.
tests/​Aspire.Cli.Tests/​Projects/​DotNetAppHostProjectTests.cs Verifies fallback and direct-launch options.
tests/​Aspire.Cli.Tests/​Projects/​AppHostServerSessionTests.cs Verifies server job protection.
extension/​src/​test-e2e/​edgeCases.e2e.test.ts Targets the permanent AppHost fixture explicitly.

💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.

@joperezr Jose Perez Rodriguez (joperezr) added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Sep 28, 2026
@joperezr

Copy link
Copy Markdown
Member

Chatted with David Negstad (@danegsta) offline and decided to push this to .1 patch.

@joperezr Jose Perez Rodriguez (joperezr) removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Sep 30, 2026
@joperezr
Jose Perez Rodriguez (joperezr) merged commit d6b50f5 into release/13.6 Sep 30, 2026
199 checks passed
@joperezr

Copy link
Copy Markdown
Member

Given the risk, when we have a new staging build, can you please do some e2e testing for this David Negstad (@danegsta) ?

@aspire-repo-bot

Copy link
Copy Markdown
Contributor

✅ No documentation update needed.

Step 5 branch taken: excluded → base_branch_is_release, title_release_prefix, body_backport_marker

Triggered signals: none (signal_count: 0, triggered_signals: []); recommendation was docs_optional but is irrelevant here since excluded == true overrides it.

Exclusion reasons (from signals.json):

  • base_branch_is_release — PR base ref is release/13.6
  • title_release_prefix — title is [release/13.6] Fix Windows AppHost cleanup job routing
  • body_backport_marker — body states "Backport of Fix Windows AppHost cleanup job routing #20494 to release/13.6 (cherry-picked patch from e62dceba954899a33326297b4669e226c27dcdce)"

This is a backport PR; per policy its user-facing documentation (if any) is authored against the original forward PR (#20494) on the default branch, so drafting a duplicate docs PR here would be pure noise. No docs PR created.

This was referenced Oct 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants