Skip to content

Add explicit-start shutdown regression coverage (startup-token cancellation and process exit) - #21397

Open
David Fowler (davidfowl) wants to merge 2 commits into
mainfrom
agent/fix-explicit-start-lifecycle-budget
Open

David Fowler (davidfowl) wants to merge 2 commits into
mainfrom
agent/fix-explicit-start-lifecycle-budget

Conversation

@davidfowl

@davidfowl David Fowler (davidfowl) commented Oct 29, 2026 •

Copy link
Copy Markdown
Collaborator

Description

Update after merging main (2026-10-30): #21360 has since landed on main with its own fix that gives this test a separate stopCts for StopAsync and moves it to LongTimeoutTimeSpan budgets. The cancellation-ownership repair this PR originally made is therefore already on main. After resolving the merge conflict, this PR adds regression coverage only on top of main's version of the test:

  • The test becomes a [Theory] that also covers an explicitly cancelled startup token after startup has completed (cancelStartupAfterStart: true).
  • The test awaits its actual StartAsync task instead of discarding it.
  • It asserts that the ServiceB root process is running before shutdown and has exited afterwards, and that ApplicationStopped fires.

The timeouts and token ownership come unchanged from main. The before/after evidence below was gathered against the original pre-#21360 base. It shows why phase-owned shutdown is needed, but it is no longer this PR's diff. Both theory cases pass locally on the merged head.

Original description (pre-merge): give BeforeResourceStartedEvent_NotFiredForExplicitStartOnInitialCreation separate startup and shutdown cancellation budgets. A slow startup previously consumed most of the same 30-second CI token subsequently passed to StopAsync, causing cleanup cancellation even when shutdown itself completed within the standard bound.

Related: #21026, #21328. These issues are intentionally left open for continued observation.

The test now observes its actual startup task, creates the shutdown token only when entering shutdown, and runs both natural and explicitly cancelled completed-startup cases. All existing resource/event assertions, waits, ports, and timeout values are preserved. Both cases verify actual host shutdown and ServiceB root-process exit. No product, shared fixture, workflow, dependency, or policy changes are included.

Native before/after evidence

Run Actual selected execution evidence
Unmodified baseline 8 passed / 1 failed. Failure was cancellation in the original line-365 StopAsync after approximately 21 seconds of startup consumed the shared 30-second budget.
Paired causal experiment Shared-token/cancelled-completed-startup controls failed 9/9; phase-owned shutdown with the same startup cancellation passed 9/9, with actual shutdown/root exit within the unchanged bound. Diagnostic controls and instrumentation are not in this PR.
Clean repair qualification 60 actual TRX executions passed, 0 failed, 0 skipped on Windows runners 2 and 3, each completing all three fixed iterations. Includes both repaired rows six times each, 12 adjacent event cases, and 36 broader lifecycle/cancellation/explicit-start cases.

The clean repaired source at d28c36fe8091ae1308fbf5691e36b71c3b51b0e2 is byte-identical to the test source executed at validation head 7c6ad6b7d8d99b06eea1224f853ea45f4ad5f822. Only the separate validation branch contains workflow selection/TRX-capture configuration.

Qualification gap: 90 executions were planned. Runner 1's build succeeded, but its check reports “The hosted runner lost communication with the server.” It retained neither logs nor TRX; its 30 planned outcomes are unknown, not passes or demonstrated test failures. That qualification workflow remains failed; subsequent normal PR CI does not fill those missing outcomes. Local native execution was unavailable after the prescribed restore failed with NU1900/NU1905 resolving configured NuGet audit metadata; the supported hosted Windows path was used without policy overrides.

Normal PR CI status (2026-10-29 18:28 UTC)

Normal CI executed both repaired theory rows and both adjacent event facts successfully on Windows and Ubuntu. Hosting-1 completed 28 passed / 0 failed / 0 skipped on Windows and 63 passed / 0 failed / 0 skipped on Ubuntu.

Attempt 1 failed in the unchanged Ubuntu AzureFunctionsTests.AddAzureFunctionsProject_WorksWithMultipleProjects at its line-201 parameterless host.StopAsync() (TaskCanceledException, 32.300-second test; suite 2,109 passed / 1 failed / 5 skipped). That separate test does not use this PR's repaired method or startup token; its root cause remains unproven.

The repository's automatic CI rerun started attempt 2 without a source change. Its Azure suite passed 2,110 / 0 failed / 5 skipped, including that method in 28.566 seconds. The macOS ARM64 build also succeeded. Latest applicable CI is now green on d28c36fe8091ae1308fbf5691e36b71c3b51b0e2 (46 successful checks, 29 skipped); skipped checks are not passing test executions. The first Azure failure and the separate qualification gap above remain part of the evidence. Copilot review identified no active issues. This is ready for human review, not approved for merge.

Scope and limits

This is not a revival of #20406's broad shared-fixture/shutdown-timeout/DCP-log-forwarding changes. That PR was closed after an out-of-order DCP API-server update bug was identified. Here the actual selected host path separately demonstrates that passing an already-cancelled completed-startup token to shutdown causes cancellation; giving shutdown its own unchanged budget completes host shutdown and root exit.

The distinct original line-353 resource-startup failure associated with Docker/WSLc discovery stalls is not fixed or claimed fixed. Root-process exit does not prove that every descendant process exited.

Created using the repository's fix-flaky-test skill, with existing before/after native evidence and deterministic cancellation coverage rather than arbitrary delays or larger timeouts.

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

🤖 Posted by AspireAgent (AI Agent)

Observe startup completion and give shutdown its own standard cancellation budget. Exercise both natural and expired completed-startup budgets and verify the host and running service stop.

Co-authored-by: Copilot <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 -- 21397

Or

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

@aspire-repo-bot
aspire-repo-bot Bot requested a balanced review from Copilot October 29, 2026 17:34
@github-actions github-actions Bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Oct 29, 2026
@github-actions

This comment has been minimized.

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.

🟢 Approval recommended

No active review issues were identified for the test-only changes separating startup and shutdown cancellation budgets.

No open issues 🎉

What changed in this PR

This PR addresses shutdown cancellation in the explicit-start hosting test by separating startup and shutdown budgets, without changing production code or timeout values. It targets the teardown failures reported in #21026 and #21328, not the separate resource-startup failure.

Changes:

  • Observes the actual startup task before initiating shutdown.
  • Covers both normal completion and explicit cancellation of the completed startup token.
  • Gives shutdown a fresh cancellation budget and verifies host shutdown and ServiceB root-process exit.
File Description
tests/​Aspire.Hosting.Tests/​DistributedApplicationTests.cs Separates lifecycle cancellation budgets and adds cancellation-case coverage and shutdown assertions.

Review effort: Balanced

@github-actions

Copy link
Copy Markdown
Contributor

Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt.

# Conflicts:
#	tests/Aspire.Hosting.Tests/DistributedApplicationTests.cs
@github-actions

Copy link
Copy Markdown
Contributor

Tests selector

6 / 100 PR test projects · 0 PR jobs, from 1 changed file.

Selected PR test projects (6 / 100)

Aspire.Hosting.Azure.Kubernetes.Tests, Aspire.Hosting.Azure.Tests, Aspire.Hosting.Blazor.Tests, Aspire.Hosting.Dotnet.Tests, Aspire.Hosting.Radius.Tests, Aspire.Hosting.Tests

Selected PR jobs (0)

none


How these were chosen — grouped by what changed

🧪 tests/Aspire.Hosting.Tests/DistributedApplicationTests.cs (changed test)
→ 1 directly: Aspire.Hosting.Tests
→ 5 via the project graph: Aspire.Hosting.Azure.Kubernetes.Tests (2 hops), Aspire.Hosting.Azure.Tests, Aspire.Hosting.Blazor.Tests, Aspire.Hosting.Dotnet.Tests, Aspire.Hosting.Radius.Tests

Job reasons

none


Selection computed for commit 12f4c50.

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.

🟡 Changes recommended

The title and description claim a cancellation-budget separation that already exists, misrepresenting this PR’s actual contribution of regression coverage and startup-completion observation.

1 open issue

Review effort: Balanced

Copy link
Copy Markdown
Collaborator Author

Merged main into this branch to resolve the conflict in DistributedApplicationTests.cs. The PR's theory and process-exit assertions are kept, now using main's LongTimeoutTimeSpan startup/stop budgets. Both theory cases pass locally.

On the new head, Hosting-2 (windows-latest) failed in an unrelated test, ExecutableResourceFailureLoggingTests.ExecutableDoesNotExist. DCP retried the missing-executable start, so the "Failed to start a process" line was logged twice where the test expects it once (job). This PR doesn't touch that code path, so I'm rerunning the failed job once.

🤖 Posted by AspireAgent (AI Agent)

@github-actions

Copy link
Copy Markdown
Contributor

Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt.

@davidfowl David Fowler (davidfowl) changed the title Fix explicit-start test shutdown cancellation ownership Add explicit-start shutdown regression coverage (startup-token cancellation and process exit) Oct 31, 2026

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

flaky-test needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants