Handle SIGTERM in terminal host and GC orphaned sockets (#19302) - #19344
Conversation
WithTerminal() leaked two per-replica Unix-domain-socket files ({id}.dcp.sock producer and {id}.host.sock consumer) under ~/.aspire/trmnl/ on a graceful `aspire stop`.
Root cause: the aspire.terminalhost child only handled SIGINT (Console.CancelKeyPress). DCP stops the resource with SIGTERM and waits a grace period before SIGKILL, so the child's cooperative teardown (which unbinds and deletes the sockets) never ran; the recycle loop re-bound the producer/consumer UDS after the AppHost's ApplicationStopped cleanup, then SIGKILL left both files bound on disk.
Fix (two parts):
1. Program.cs now routes SIGINT + SIGTERM (and SIGQUIT on Windows) through PosixSignalRegistration into the existing graceful teardown, with a Console.CancelKeyPress fallback for platforms that lack PosixSignalRegistration. A clean stop now unlinks both sockets before exit.
2. MaterializeTerminalHosts sweeps the shared trmnl directory on startup and reclaims files whose owning AppHost PID is provably dead (keyed off the sidecar's appHostPid), cleaning up orphans left by ungraceful exits. PID-liveness makes it safe: live AppHosts are never touched.
Also corrects the now-inaccurate ApplicationStopped backstop comment. Adds a child-side graceful-cleanup test and two AppHost-side startup-GC tests.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: c3b3d047-2db6-43af-a787-315dd291a16f
github-actions
Bot
added
the
needs-area-label
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 19344Or
iex "& { $(irm https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 19344" |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull request overview
Adds graceful terminal-host shutdown and startup cleanup for orphaned Unix socket artifacts.
Changes:
- Handles SIGINT/SIGTERM through cooperative cancellation.
- Sweeps terminal files owned by dead AppHost processes.
- Adds shutdown and orphan-cleanup regression tests.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/Aspire.TerminalHost/Program.cs |
Registers process signal handlers. |
src/Aspire.Hosting/TerminalResourceBuilderExtensions.cs |
Adds startup orphan cleanup. |
tests/Aspire.TerminalHost.Tests/TerminalHostAppTests.cs |
Tests socket cleanup after cancellation. |
tests/Aspire.Hosting.Tests/WithTerminalTests.cs |
Tests dead/live owner sweeping. |
PR Testing ReportPR Information
Artifact Version Verification
Changes AnalyzedFiles Changed
Change Categories
Test Scenarios ExecutedScenario 1: PR artifact identityObjective: Ensure runtime testing uses the latest PR artifact. Coverage Type: Artifact integrity Status: PASS The dogfood CLI reported Evidence:
Scenario 2: Graceful
|
| Scenario | Status | Notes |
|---|---|---|
| PR artifact identity | PASS | Dogfood version matches current PR head |
Graceful aspire stop cleanup |
FAIL | .dcp.sock and .host.sock remain |
| Direct bundled SIGTERM diagnostic | FAIL | Exit 143; all three sockets remain |
| Crash-orphan startup recovery | PASS | Dead-owner sidecar and sockets swept |
| Live-owner safety boundary | PASS | Live peer files preserved |
Overall Result
FAIL - ISSUES FOUND
The startup-GC half of the PR works for orphans that retain a metadata sidecar, and it correctly protects live AppHosts. The primary graceful-shutdown fix does not affect the bundled executable used by dogfood and normal CLI bundle execution, so the original aspire stop leak remains.
Recommendations
- Apply the SIGINT/SIGTERM registration to
Aspire.Managed.RunTerminalHost, preferably by sharing one signal-to-cancellation helper with the standalone entrypoint. - Add a process-level test that launches the bundled
aspire-managed terminalhost, sends SIGTERM, and verifies exit code 0 plus removal of producer, consumer, and control sockets. - Keep the existing cancellation test as teardown coverage, but do not treat it as signal-delivery coverage.
David Pine (IEvangelist)
left a comment
David Pine (IEvangelist)
left a comment
There was a problem hiding this comment.
3 issues found: 2 correctness issues (the bundled SIGTERM path is unchanged, and metadata schema versions are ignored) and 1 regression-coverage gap for live peer ownership.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: ec34791b-0507-43c6-b846-916aa51f7f80
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
src/Aspire.Hosting/TerminalResourceBuilderExtensions.cs:565
- The startup GC cannot reclaim orphans created by existing releases. Those sidecars use schema v1 and omit the newly required
appHostProcessStartTimeUnixMilliseconds, so deserialization throws here (and the later current-schema check would reject them anyway). This contradicts the PR's stated goal of reclaiming the reporter's already-stale sidecar. Add an explicit v1 compatibility path that validates the filename/metadata ID and uses PID-only liveness (a reused live PID should conservatively skip cleanup), plus a regression test using the actual v1 JSON shape.
var json = await File.ReadAllTextAsync(candidatePath, cancellationToken).ConfigureAwait(false);
metadata = JsonSerializer.Deserialize<TerminalHostMetadata>(json);
src/Aspire.Hosting/TerminalResourceBuilderExtensions.cs:446
- This makes every per-replica lock file permanent:
DeleteReplicaFilesomits it, and both shutdown cleanup and startup GC call that method. Each distinct terminal replica therefore still leaves an artifact forever, andDirectory.GetFilesmakes every later startup scan the growing set of abandoned locks. Add a safe lock-reclamation strategy (for example, serialize lock-file deletion under a directory-wide lock) or use synchronization that does not require permanent per-replica files.
// Keep the lock file persistent. Deleting a locked file on Unix would let another
// process create a new inode at the same path and acquire a second, independent lock.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: ec34791b-0507-43c6-b846-916aa51f7f80
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/Aspire.Hosting/TerminalResourceBuilderExtensions.cs:351
- Lock acquisition is inside the best-effort sidecar-write
try, whose catch also handlesUnauthorizedAccessException. If opening{id}.lockfails due to permissions, materialization therefore continues without synchronization or a new ownership sidecar, despite the collision-safety requirement documented below. Acquire the lock before entering the write-onlytryso permission failures propagate just like the lock timeout.
using var replicaLock = await AcquireReplicaLockAsync(
trmnlDirectory,
metadata.ReplicaId,
cancellationToken).ConfigureAwait(false);
src/Aspire.Hosting/TerminalResourceBuilderExtensions.cs:595
- The startup GC cannot reclaim the leaks created by the already-shipped schema-v1 sidecars. Those files omit
AppHostProcessStartTimeUnixMilliseconds, so deserialization can fail on the required member, and this condition rejects schema 1 regardless. This means the stale files from #19302—including the reporter's existing sidecar—survive every startup. Treat schema 1 as a known legacy shape: validate the filename/content ID, delete only when its PID no longer exists, and preserve it when the PID is live/reused; add a v1 regression test.
if (metadata.SchemaVersion != TerminalHostMetadata.CurrentSchemaVersion
|| !string.Equals(metadata.ReplicaId, replicaId, StringComparison.Ordinal)
|| metadata.AppHostPid <= 0
|| metadata.AppHostProcessStartTimeUnixMilliseconds <= 0)
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/Aspire.Hosting/TerminalResourceBuilderExtensions.cs:302
ApplicationStoppeddoes not guarantee that DCP's terminal-host children have exited; that false assumption is the root condition documented in #19302. The new child teardown and random IDs make this callback safe despite the overlap, so describe that actual guarantee rather than preserving the disproven ordering assumption.
// Why ApplicationStopped (not ApplicationStopping): deleting after the children have fully
// exited avoids racing a child that is still mid-drain.
src/Aspire.Hosting/TerminalResourceBuilderExtensions.cs:364
- Creating/truncating the sidecar before the cancellable write can leave an empty or partially written JSON file when
WriteAsyncis canceled or fails mid-write. The orphan sweep treats malformed JSON as permanently uncollectable, so if the host later creates sockets (for example after a partial I/O failure that is swallowed below), those artifacts can never be reclaimed. Write to a temporary file and atomically move it into place, or delete the incomplete sidecar on every unsuccessful/canceled write.
using (var fs = new FileStream(
metadataPath,
FileMode.Create,
FileAccess.Write,
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
Karol Zadora-Przylecki (karolz-ms)
left a comment
Karol Zadora-Przylecki (karolz-ms)
left a comment
There was a problem hiding this comment.
Reviewed the current head of this branch locally (built clean, and the new WithTerminalTests / TerminalHostSignalTests all pass on macOS). The redesign around per-run replica IDs and the background collector reads well and the earlier threads look genuinely resolved.
Four findings, all new against the current state:
- 1 correctness/robustness bug: SIGTERM is now intercepted with no bounded force-exit backstop.
- 1 durability gap: the metadata sidecar write is not atomic, and any unreadable sidecar becomes permanently unreclaimable.
- 1 log-noise issue: the sweep is scheduled before all resources have written their sidecars.
- 1 test-maintainability nit: fragile assembly lookup in the new process tests.
PR Testing ReportPR Information
Artifact Version Verification
The PR head was rechecked after testing and had not changed. Test Environment
Changes AnalyzedFiles Changed
Change Categories
Test Scenarios ExecutedScenario 1: Targeted source regression coverageObjective: Exercise the changed hosting cleanup, signal handling, watchdog, metadata, compatibility, and path behavior at the exact PR head. Coverage Type: Unit/process integration Status: ✅ Passed for PR-related coverage Results:
The sole failure was Evidence:
Scenario 2: Dogfood happy path and graceful cleanupObjective: Verify a generated PR app can run an executable with Coverage Type: Happy path Status: ✅ Passed Observations:
Evidence:
Scenario 3: Bundled SIGTERM and parent-death cleanupObjective: Test the installed PR's bundled Coverage Type: Signal and unhappy path Status: ✅ Passed Results:
Evidence:
Scenario 4: Forced AppHost death and orphan recoveryObjective: Verify an exact owner crash does not leak terminal-host sockets and that the next same-scope AppHost reclaims the discoverable metadata. Coverage Type: Unhappy path and recovery Status: ✅ Passed Steps and observations:
Expected Unhappy-Path Outcome: Socket cleanup occurs immediately after owner death; metadata remains discoverable until the next safe sweep. Evidence:
Scenario 5: Concurrent owner safety and unsupported metadataObjective: Ensure one AppHost never deletes another live AppHost's terminal artifacts and unknown metadata is preserved conservatively. Coverage Type: Boundary and negative Status: ✅ Passed Observations:
Expected Unhappy-Path Outcome: Unknown metadata must be retained with a warning rather than deleted. Evidence:
Diagnostic Controls and Environment Notes
Summary
Overall Result✅ PR VERIFIED No PR-caused failures or blocking issues were found. |
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
src/Aspire.Hosting/Lifecycle/TerminalHostOrphanCleanupService.cs:239
- The sweep only enumerates metadata sidecars, and there is no code that checks or reclaims the legacy per-replica lock files promised by the PR description. In particular, an active schema-v2 owner in another PID namespace can appear dead to this process and have its sockets deleted even while it holds the legacy lock; stale lock files are never reclaimed either. Honor the legacy lock before deleting v1/v2 artifacts and remove it only after ownership is safely acquired.
foreach (var candidatePath in Directory.GetFiles(trmnlDirectory, $"*.{TerminalHostPaths.MetadataSuffix}"))
src/Aspire.Hosting/TerminalResourceBuilderExtensions.cs:328
- A hard process crash can leave exactly this
.tmpfile, but the startup sweep only enumerates*.metadata.json, and exact-path shutdown cleanup also omits the temporary path. These files therefore accumulate permanently. Include stale metadata temp files in bounded startup cleanup, or use a crash-safe temporary-file mechanism that cannot leave persistent artifacts.
// Write and chmod a sibling temporary file before atomically replacing the sidecar.
// A crash during serialization can then leave only an undiscoverable .tmp file, never
// a truncated metadata document that permanently blocks orphan recovery.
|
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>
Tests selector (audit mode)The full test matrix and all jobs still run in audit mode. The tests and jobs below are what selective CI would run under enforcement. 56 / 101 test projects · 5 jobs, from 20 changed files. Selected test projects (56 / 101)
Selected jobs (5)
How these were chosen — grouped by what changed
🔧 show 39
🔧 🧪 📦 affected project 📦 affected project 🧪 🧪 🧪 🧪 🧪 Job reasons
Selection computed for commit |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/Aspire.Hosting/TerminalResourceBuilderExtensions.cs:267
- If startup is cancelled while writing replica N, this exception exits before
RegisterReplicaArtifactsruns. Sidecars already written for replicas 0..N-1 are then absent from theApplicationStoppedcleanup list and remain orphaned. Register each generated replica ID before the cancellable write (or clean the partially materialized IDs in a cancellation/failure path) so interrupted startup cannot leak artifacts.
await WriteMetadataSidecarAsync(
src/Aspire.Hosting/Lifecycle/TerminalHostOrphanCleanupService.cs:120
- The background sweep is awaited without a timeout during DI disposal. If enumeration or file I/O under a network-mounted home directory stalls, AppHost shutdown can still hang indefinitely despite the bounded
ApplicationStoppedcleanup above. Bound this wait (and log a warning on timeout) just as the shutdown deletion path is bounded.
await cleanupTask.ConfigureAwait(false);
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
|
Pull request created: #1503
|
|
📝 Documentation has been drafted in microsoft/aspire.dev#1503 targeting Added a new "Terminal cleanup after a crash or forced stop" section to Note This draft PR needs human review before merging. |
Description
WithTerminal()could leave per-replica Unix-domain sockets under~/.aspire/trmnl/when a terminal host was stopped with SIGTERM or when its owning AppHost exited unexpectedly.This change makes terminal lifecycle cleanup resilient across graceful stops, forced termination, process crashes, PID reuse, reboots, containers, and concurrent AppHosts:
Aspire.TerminalHostexecutable and the bundledaspire-managed terminalhostpath now use the same runner. It handles SIGINT/SIGTERM (plus Windows Ctrl+Break), cancelsTerminalHostApp, and lets its existing teardown unlink the sockets.On Windows,
TerminateProcesscannot be intercepted. Graceful console signals use the shared runner, while exact-path AppHost cleanup, the parent watchdog, and the next startup sweep provide the cleanup backstops for forced termination.Tests
Coverage includes process-level SIGTERM for the bundled host, parent-death cleanup for bundled and standalone hosts, random identity isolation, schema compatibility, PID reuse, process-scope and Linux boot-ID handling, metadata-last deletion, legacy-lock interoperability, singleton background sweeping, and publish-mode behavior.
Fixes #19302
Checklist
<remarks />and<code />elements on your triple slash comments?