Fix CLI Ctrl+C/SIGTERM shutdown: responsive cancellation and double-signal bug#17588
Fix CLI Ctrl+C/SIGTERM shutdown: responsive cancellation and double-signal bug#17588JamesNK wants to merge 7 commits into
Conversation
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 17588Or
iex "& { $(irm https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 17588" |
There was a problem hiding this comment.
Pull request overview
Improves Aspire CLI termination responsiveness when users press Ctrl+C during run/AppHost startup, aiming to avoid multi-second waits and provide a “second signal” forced termination path.
Changes:
- Refactors
ConsoleCancellationManagerso the first signal triggers non-blocking cancellation and schedules forced termination asynchronously; subsequent signals trigger immediate forced-termination signaling. - Threads the command cancellation token into
RunCommand.CancelAppHostStartupAsyncso its startup-cancellation wait exits promptly on Ctrl+C. - Adds unit coverage for the cancellation manager behavior and a regression test ensuring
runexits promptly when cancelled during the startup-timeout cancellation path.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Aspire.Cli.Tests/ConsoleCancellationManagerTests.cs | Adds tests for first/second signal behavior, async timeout forcing, and non-blocking cancel. |
| tests/Aspire.Cli.Tests/Commands/RunCommandTests.cs | Adds regression test to ensure Ctrl+C cancels out of the startup-cancellation timeout promptly. |
| src/Aspire.Cli/Program.cs | Wires a logger into cancellation handling for diagnostics. |
| src/Aspire.Cli/ConsoleCancellationManager.cs | Makes signal handling non-blocking; adds async forced-termination timeout and second-signal behavior; adds logging hook. |
| src/Aspire.Cli/Commands/RunCommand.cs | Passes the command cancellation token through to WaitAsync during startup cancellation. |
- Make ConsoleCancellationManager.Cancel() non-blocking so Ctrl+C handler returns immediately - Pass cancellation token through to WaitAsync in CancelAppHostStartupAsync so Ctrl+C exits promptly instead of waiting for the full 5s timeout - Support second Ctrl+C for immediate force exit (Environment.Exit) - Add logging support to ConsoleCancellationManager via SetLogger() - Add comprehensive unit tests for ConsoleCancellationManager - Add integration test for RunCommand cancellation during startup timeout Fixes #17569
06a5d7d to
754458f
Compare
Replace WaitForSuccessPromptAsync with WaitForSuccessPromptFailFastAsync across all CLI E2E tests. The fail-fast variant detects error prompts immediately and throws instead of hanging for up to 500s waiting for a success prompt that will never arrive. This prevents 10+ minute CI timeouts when a command fails with a non-zero exit code.
- Fix ConsoleCancellationManager double-signal bug: move Console.CancelKeyPress to else branch so it only registers on platforms without PosixSignalRegistration. Previously both handlers fired for the same SIGINT, causing immediate force-kill. - Add SIGQUIT/Ctrl+Break registration for Windows parity. - Remove old WaitForSuccessPromptAsync (no fail-fast) and rename WaitForSuccessPromptFailFastAsync to WaitForSuccessPromptAsync. - Remove duplicate RunCommandFailFastAsync (identical to RunCommandAsync).
PR Testing ReportPR Information
CLI Version Verification
Test Scenarios ExecutedScenario 1: Normal Start and StopObjective: Verify basic Steps:
Observations:
Scenario 2: ConsoleCancellationManager Unit TestsObjective: Verify all signal handling logic — first signal cooperative cancellation, second signal force-kill, timeout behavior Tests executed:
Scenario 3: Ctrl+C During Startup TimeoutObjective: Verify that when Ctrl+C fires during AppHost startup, the CLI exits promptly rather than blocking for the full 5-second Test: Observations:
Scenario 4: Startup Timeout Regression TestsObjective: Ensure existing startup timeout behavior is preserved Tests executed:
Summary
Overall Result✅ PR VERIFIED — All 13 targeted tests pass. The normal start/stop lifecycle works correctly. Signal handling changes are well-covered by unit tests that directly exercise the |
| /// Sets the logger instance used for diagnostic messages during signal handling. | ||
| /// Call this once the logging infrastructure is available. | ||
| /// </summary> | ||
| internal void SetLogger(ILogger logger) => Volatile.Write(ref _logger, logger); |
There was a problem hiding this comment.
Why is this a volatile write?
There was a problem hiding this comment.
AI was worried about it being used at the same time it is set, and memory barrier issues from ARM. I humoured it.
| // Schedule the forced-completion timeout asynchronously so the signal handler | ||
| // returns immediately. This allows Program.Main's Task.WhenAny to observe | ||
| // handlerTask completion without being blocked by the signal handler thread. | ||
| _ = ForceTerminationAfterTimeoutAsync(forcedTerminationExitCode); |
There was a problem hiding this comment.
Are you sure returned Task is running (hot)?
There was a problem hiding this comment.
It either completes immediately or Task.Delay is called. It won't sync block.
|
❓ CLI E2E Tests unknown — 107 passed, 0 failed, 2 unknown (commit View all recordings
📹 Recordings uploaded automatically from CI run #26584075410 |
|
Tried this, still takes forever (5 seconds is forever): Screen.Recording.2026-05-28.104823.mp4It also feels like forever because there's no feedback. |
|
@davidfowl |
|
This is purely for the CLI itself, nothing to do with graceful shutdown of resources, which happens when you stop a resource from the dashboard or cli, but isn't graceful in the debugger in VS. |
but it should be supported... We should be able to trigger a stop cmd that would send a sigterm to a resource when testing our distributed app.. You should at least rename KnownRessourcesCommand to Kill instead of Stop.. since this is what you truly are doing.. killing the process |
aspire resource stop. Lets not use this pr to discuss feature requests though |
|
Will this fix #17625? |
Description
Fixes the CLI's Ctrl+C/SIGTERM handling to be responsive during all phases and prevents a double-signal bug that caused immediate force-kill instead of graceful shutdown.
Problems Fixed
Sluggish Ctrl+C during AppHost startup: When a user pressed Ctrl+C during AppHost startup (before the AppHost connects back to the CLI), the CLI would wait for the full 5-second startup timeout before exiting.
Double-signal bug causing force-kill: Both
PosixSignalRegistration(SIGINT)andConsole.CancelKeyPresswere registered simultaneously. On Linux, when SIGINT arrives both handlers fire for the same signal, each callingCancel(), so_cancelCalledreaches 2 immediately — triggering the force-kill path instead of graceful shutdown.No way to force-exit: If graceful shutdown was taking too long, there was no mechanism for a second Ctrl+C to force immediate termination.
Changes
src/Aspire.Cli/ConsoleCancellationManager.cs(new file in this PR):CancellationTokenSourceProcessTerminationCompletionSourceConsole.CancelKeyPressis now only registered on platforms withoutPosixSignalRegistration(Android, iOS, tvOS, Browser) — eliminates the double-signal bugSetLogger()for diagnostic observability andSetStartedHandler()for timeout trackingsrc/Aspire.Cli/Program.cs:ConsoleCancellationManagerearly in startup and wires it through the command pipelineTask.WhenAny(handlerTask, processTerminationCompletionSource.Task)so forced termination is observed immediatelysrc/Aspire.Cli/Commands/RunCommand.cs:CancelAppHostStartupAsyncnow receives the cancellation token and passes it toWaitAsync, so the startup-timeout wait exits immediately on Ctrl+Ctests/Aspire.Cli.Tests/ConsoleCancellationManagerTests.cs(new):tests/Aspire.Cli.Tests/Commands/RunCommandTests.cs:tests/Shared/Hex1bAutomatorTestHelpers.cs:WaitForSuccessPromptAsync(which would hang on errors instead of failing fast)WaitForSuccessPromptFailFastAsync→WaitForSuccessPromptAsync(now the only variant, always fails fast on ERR prompts)RunCommandFailFastAsync(was identical toRunCommandAsync)E2E test files (59 files):
WaitForSuccessPromptFailFastAsync→WaitForSuccessPromptAsyncRunCommandFailFastAsync→RunCommandAsyncFixes #17569
Checklist