Stop AppHost debug session before Aspire parent#18561
Conversation
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 -- 18561Or
iex "& { $(irm https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 18561" |
There was a problem hiding this comment.
Pull request overview
This PR fixes flaky VS Code extension E2E test global debug stop removes running apphost (#18524), which was turning rolling main red. The test timed out because, after a global/E2E debug stop, the synthetic Aspire parent debug session was stopped but the real AppHost child session (and its global registry entry) lingered until the test timed out. The fix reorders AspireDebugSession.stopDebugging() to stop the real AppHost session before the synthetic Aspire parent — mirroring the existing dispose() child-before-parent ordering — so the AppHost registry refresh observes the stopped AppHost promptly instead of relying on VS Code cascading termination.
Changes:
stopDebugging()now stops the AppHost child session first, then stops the parent in afinallyblock so the parent is stopped even if the AppHost stop request fails.- The tracked resource stop callback (
disposalFunction) now returns thevscode.debug.stopDebugging(session)promise (withstopSession()retyped toThenable<void>) so callers can await the child stop, whilecleanupRunstill runs synchronously. - Added two unit tests covering the new stop ordering and the parent-stop fallback when the AppHost stop fails.
Show a summary per file
| File | Description |
|---|---|
extension/src/debugger/AspireDebugSession.ts |
Reorders stopDebugging() to stop AppHost before parent; returns the child stop promise from the disposal callback. |
extension/src/dcp/types.ts |
Changes AspireResourceDebugSession.stopSession() return type from void to Thenable<void> so the child stop can be awaited. |
extension/src/test/aspireDebugSession.test.ts |
Adds unit tests verifying stop ordering and parent-stop fallback on AppHost stop failure. |
I verified the only non-test stopSession() call site (AspireDcpServer.ts DELETE /run_session/:id) remains a deliberate fire-and-forget call, and confirmed the extension's ESLint config does not enable no-floating-promises, so the return-type change introduces no new lint failures. The new unit tests are consistent with existing patterns in the file, and the ordering/fallback assertions are correct. I found no concrete issues to comment on.
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 0
- Review effort level: Medium
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Description
Related to #18524. This intentionally does not auto-close the failing-test tracking issue until the normal tracking policy says it is safe to close.
The VS Code extension E2E
global debug stop removes running apphosthas been timing out on rollingmain: the E2E state file showsdebugSessions: [], but the global AppHost registry still contains the stopped AppHost PID/dashboard URL until after the test times out.This makes the global stop path stop the real AppHost debug session before stopping the synthetic Aspire parent debug session. That matches the existing dispose ordering, but applies it to the explicit
stopDebugging()path used by the E2E bridge and global debug stop. The parent session is still stopped in afinallyblock if the AppHost stop request fails.I also made the tracked resource stop callback return the VS Code stop promise, so callers can await the child stop request, and added unit coverage for both the ordering and parent-stop fallback cases.
Validation:
./restore.shdotnet build src/Aspire.Cli/Aspire.Cli.csproj /p:SkipNativeBuild=true --verbosity minimalcd extension && COREPACK_HOME="$PWD/.corepack-cache" corepack yarn run compile-testscd extension && COREPACK_HOME="$PWD/.corepack-cache" corepack yarn run unit-test --config .vscode-test-short.mjs --run out/test/aspireDebugSession.test.js(20 passing)cd extension && COREPACK_HOME="$PWD/.corepack-cache" corepack yarn eslint src/debugger/AspireDebugSession.ts src/dcp/types.ts src/test/aspireDebugSession.test.tsI attempted the targeted E2E locally with
ASPIRE_EXTENSION_E2E_SPEC=out/test-e2e/test-e2e/debugDashboard.e2e.test.js. It did not reach the Aspire test: local macOS ExTester failed during VS Code/Chrome session creation withSessionNotCreatedError: Chrome instance exited. Before that, the local fixture restore also neededWarningsNotAsErrors=NU1507because this checkout has multiple NuGet feeds without source mapping in the generated E2E workspace.Checklist
<remarks />and<code />elements on your triple slash comments?