Skip to content

Accept natural ps leader exit racing a failed Windows taskkill (Fixes #20869) - #21595

Merged
Ella Hathaway (ellahathaway) merged 1 commit into
microsoft:mainfrom
davidfowl:aspireagent/fix-20869-ps-follower
Nov 2, 2026
Merged

Ella Hathaway (ellahathaway) merged 1 commit into
microsoft:mainfrom
davidfowl:aspireagent/fix-20869-ps-follower

Conversation

@davidfowl

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

Copy link
Copy Markdown
Collaborator

Description

Fixes the intermittent VS Code extension E2E (Windows, apphost-tree) failure owns at most one ps follower…. The test reports Timed out after 30000ms waiting for 1 owned ps follower(s) and the state error Failed to stop the previous AppHost watcher: Error: taskkill for aspire ps exited with code 255 while PID N remained live.

Denominator (ci.yml, 2026-10-29 13:00Z → 10-31 13:23Z, 388 runs)

  • The VS Code E2E (Windows, apphost-tree) job failed 13 times and succeeded 379 times.
  • 7 of the failure logs were readable. 6 of those match this signature, on several branches including main (run 40905807668, job 123572993753).
  • The other 6 failure logs were empty: runner loss / BlobNotFound.

Root cause (from the artifact extension-e2e-diagnostics-win-x64-apphost-tree-attempt1 of run 40905807668)

  • At 16:17:46.591 the extension spawns a one-shot aspire ps --format json (PID 7120). The poller stops at .746 and forces taskkill /pid 7120 /t /f.
  • The CLI log for PID 7120 shows Exit code: 0 at 46.800 and Hosting stopped at 46.809. The process finished on its own while taskkill was running.
  • Windows refuses to terminate a process that is already exiting, so taskkill returns 255 (46.881).
  • libuv maps ACCESS_DENIED for an already-exited process to ESRCH, so child.kill('SIGKILL') returns false.
  • killLeaderAsFallback then threw the taskkill error 3ms later. AppHostPsPoller._terminatePsProcess keeps that failure permanently, so a replacement follower never starts and the test times out.

Fix

In killLeaderAsFallback, when the fallback kill() returns false, wait for close within the remaining deadline.

  • If the leader exited cleanly (exit code 0, no signal), the termination is treated as already satisfied. This is the same outcome as the existing "exited before taskkill" early return.
  • A forced taskkill terminates with exit code 1, so exit code 0 means the process exited on its own.
  • Any other outcome keeps the original taskkill error.

There are no sleeps or retries, and no assertions were relaxed.

Regression tests (extension/src/test/cliSpawn.test.ts)

  • accepts a failed taskkill when the leader already finished on its own before its exit was observed: fails before the fix with the exact CI error (taskkill for test Aspire CLI exited with code 255 while PID 5100 remained live) and passes after.
  • preserves a failed taskkill when the leader was terminated rather than finishing on its own: exit code 1 still rejects.
  • I ran the existing taskkill/termination tests locally with mocha and a vscode module stub, because the sandbox can't download VS Code. They all pass. CI runs the full extension unit and E2E suites on Windows.

CI evidence (head 6c3bb2e, run 41211446478)

Checklist

  • Is this feature complete?
    • Yes. Ready to ship.
  • Are you including unit tests for the changes and scenario tests if relevant?
    • Yes
  • Did you add public API?
    • No
  • Does the change make any security assumptions or guarantees?
    • No
  • Does the change require an update in our Aspire docs?
    • No

🤖 Posted by AspireAgent (AI Agent)

When a short-lived CLI child (e.g. one-shot aspire ps) exits on its own while
forced taskkill is running, Windows denies termination (taskkill exit 255) and
libuv's kill() returns false because the process already exited. The fallback
path threw immediately, and AppHostPsPoller permanently retained that failure,
so no replacement ps follower started.

Wait for close within the remaining deadline and accept a clean exit 0 (forced
termination yields exit 1); otherwise preserve the taskkill error.

Fixes microsoft#20869

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@aspire-repo-bot
aspire-repo-bot Bot requested a balanced review from Copilot October 31, 2026 14:43
@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 -- 21595

Or

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

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 are recorded for the Windows process-termination race fix.

No open issues 🎉

What changed in this PR

Fixes a Windows process-termination race in the VS Code extension that could prevent a replacement AppHost watcher from starting after an aspire ps command exits naturally.

Changes:

  • Waits within the remaining deadline for the CLI leader to close when fallback termination returns false.
  • Accepts a clean, signal-free exit while preserving the original taskkill error for other outcomes.
  • Adds regression tests for natural and forced exits, including listener and timer cleanup.
File Description
extension/​src/​utils/​process/​cliProcess.ts Handles natural CLI exits racing with failed Windows taskkill.
extension/​src/​test/​cliSpawn.test.ts Tests acceptance of clean exits and preservation of termination errors.

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.

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.

I reviewed the Windows termination fallback, AppHost poller cleanup and restart flow, and regression coverage. I found no blocking correctness issues or nonblocking design concerns.

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

Labels

area-vscode-extension ci-health Fixes CI flakiness or CI infrastructure (AspireAgent queue)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants