diff --git a/src/Aspire.Cli/Commands/StopCommand.cs b/src/Aspire.Cli/Commands/StopCommand.cs index 710c2654f8c..21daf122592 100644 --- a/src/Aspire.Cli/Commands/StopCommand.cs +++ b/src/Aspire.Cli/Commands/StopCommand.cs @@ -206,6 +206,12 @@ private async Task StopAppHostAsync(IAppHostAuxiliaryBackchannel connection if (stopped) { + // ProcessShutdownService only reports success once it has confirmed the AppHost process has + // terminated, so the socket's owner is gone and the file is safe to remove by exact path. Doing + // it here is the primary guard against a stale socket tripping up later commands: the AppHost's own + // cleanup is skipped if it crashes hard, and the orphan-pruning backstop misfires on Windows when the + // dead PID is reused (https://github.com/microsoft/aspire/issues/17587). + AppHostHelper.TryDeleteSocketFile(connection.SocketPath, _logger); InteractionService.DisplaySuccess(string.Format(CultureInfo.CurrentCulture, StopCommandStrings.AppHostStoppedSuccessfully, appHostIdentifier)); return CompleteStopActivity(activity, CliExitCodes.Success); } diff --git a/src/Aspire.Cli/Projects/RunningInstanceManager.cs b/src/Aspire.Cli/Projects/RunningInstanceManager.cs index 5f41d68a26a..e728b231928 100644 --- a/src/Aspire.Cli/Projects/RunningInstanceManager.cs +++ b/src/Aspire.Cli/Projects/RunningInstanceManager.cs @@ -7,6 +7,7 @@ using Aspire.Cli.Interaction; using Aspire.Cli.Resources; using Aspire.Cli.Telemetry; +using Aspire.Cli.Utils; using Microsoft.Extensions.Logging; namespace Aspire.Cli.Projects; @@ -69,6 +70,9 @@ public async Task StopRunningInstanceAsync(string socketPath, Cancellation if (stopped) { _interactionService.DisplaySuccess(RunCommandStrings.RunningInstanceStopped); + // Clean up the socket file now that the instance has been stopped so a later command does not + // rediscover it and try to connect to a dead process (https://github.com/microsoft/aspire/issues/17587). + AppHostHelper.TryDeleteSocketFile(socketPath, _logger); } else { diff --git a/src/Aspire.Cli/Utils/AppHostHelper.cs b/src/Aspire.Cli/Utils/AppHostHelper.cs index 6048efe355c..08250b7df5a 100644 --- a/src/Aspire.Cli/Utils/AppHostHelper.cs +++ b/src/Aspire.Cli/Utils/AppHostHelper.cs @@ -144,7 +144,7 @@ internal static string[] FindMatchingNonOrphanedSockets( // off the physical path (for example "/private/tmp/..." on macOS). var resolvedPath = PathNormalizer.ResolveSymlinks(appHostPath); var matchingSockets = BackchannelConstants.FindMatchingSockets(resolvedPath, homeDirectory); - var remainingSockets = PruneOrphanedSockets(matchingSockets, currentPid, out var deletedCount); + var remainingSockets = PruneOrphanedSockets(matchingSockets, currentPid, logger, out var deletedCount); if (deletedCount > 0) { logger.LogDebug("Cleaned up {Count} orphaned AppHost socket(s).", deletedCount); @@ -153,10 +153,50 @@ internal static string[] FindMatchingNonOrphanedSockets( return remainingSockets; } + /// + /// Best-effort deletion of an auxiliary backchannel socket file whose owning AppHost instance is no longer running. + /// + /// + /// This is the single CLI-side socket-cleanup path. It is used both at stop time (aspire stop and + /// , once the process is confirmed terminated) and by the orphan-pruning + /// backstop in (once the owning PID is observed to be dead). Leaving the socket + /// behind causes a later command (for example aspire add or aspire stop) to rediscover it via + /// and attempt to connect to a now-dead process. This is most visible on + /// Windows, where the dead AppHost's PID can be reused so the orphan-pruning heuristic still believes the process is + /// alive. Deleting by exact path at stop time sidesteps that PID heuristic entirely. The caller must only invoke this + /// once it knows the owning process has terminated. The AppHost-side socket cleanup in Aspire.Hosting + /// (BackchannelService/AuxiliaryBackchannelService) deliberately does not share this method: it lives in a + /// different assembly and deletes a socket the AppHost itself owns, so it is not exposed to the PID-reuse problem. See + /// https://github.com/microsoft/aspire/issues/17587. + /// + /// The path to the auxiliary backchannel socket file. + /// Logger used for diagnostic output. + /// if the socket file was deleted; otherwise . + internal static bool TryDeleteSocketFile(string socketPath, ILogger logger) + { + try + { + if (File.Exists(socketPath)) + { + File.Delete(socketPath); + logger.LogDebug("Cleaned up backchannel socket file: {SocketPath}", socketPath); + return true; + } + } + catch (Exception ex) when (ex is IOException or UnauthorizedAccessException) + { + // A failed delete is non-fatal: the next command's orphan-pruning pass will attempt cleanup again. We swallow + // the same exception types as the other socket-cleanup sites for consistency. + logger.LogDebug(ex, "Failed to clean up backchannel socket file (this may be safe to ignore): {SocketPath}", socketPath); + } + + return false; + } + /// /// Deletes PID-qualified socket files whose owning process has exited and returns sockets that should still be probed. /// - private static string[] PruneOrphanedSockets(string[] socketPaths, int currentPid, out int deletedCount) + private static string[] PruneOrphanedSockets(string[] socketPaths, int currentPid, ILogger logger, out int deletedCount) { deletedCount = 0; var remainingSockets = new List(socketPaths.Length); @@ -173,14 +213,10 @@ private static string[] PruneOrphanedSockets(string[] socketPaths, int currentPi // auxi.sock.{hash}.{instanceHash}.{pid} // After a crash or reboot these files can remain, and connecting to them // reports "connection refused" even though there is no AppHost to stop. - try + if (TryDeleteSocketFile(socketPath, logger)) { - File.Delete(socketPath); deletedCount++; } - catch (Exception ex) when (ex is IOException or UnauthorizedAccessException) - { - } continue; } diff --git a/tests/Aspire.Cli.Tests/Commands/StopCommandTests.cs b/tests/Aspire.Cli.Tests/Commands/StopCommandTests.cs index 60225e904bb..b20bd44c3d3 100644 --- a/tests/Aspire.Cli.Tests/Commands/StopCommandTests.cs +++ b/tests/Aspire.Cli.Tests/Commands/StopCommandTests.cs @@ -319,6 +319,44 @@ public async Task StopCommand_NoRunningAppHosts_ReturnsSuccess(string commandLin Assert.Equal(SharedCommandStrings.AppHostNotRunning, displayedMessage.Message); } + [Fact] + public async Task StopCommand_DeletesSocketFile_AfterSuccessfulStop() + { + // Regression test for https://github.com/microsoft/aspire/issues/17587: 'aspire stop' is the command + // that leaks the socket, so it must delete the socket file once the AppHost has been confirmed stopped. + // Otherwise a later command rediscovers the stale socket and tries to connect to a dead process. + using var workspace = TemporaryWorkspace.Create(outputHelper); + var interactionService = new TestInteractionService(); + + // The AppHost is reported with a process id that does not exist, so ProcessShutdownService observes + // termination immediately and the stop reaches the socket-cleanup branch. + var appHostPath = Path.Combine(workspace.WorkspaceRoot.FullName, "App1", "App1.AppHost.csproj"); + var socketPath = Path.Combine(workspace.WorkspaceRoot.FullName, "a.sock"); + File.WriteAllText(socketPath, ""); + Assert.True(File.Exists(socketPath)); + + var connection = CreateConnection(appHostPath, int.MaxValue - 9); + connection.SocketPath = socketPath; + + var monitor = new TestAuxiliaryBackchannelMonitor(); + monitor.AddConnection("hash1", socketPath, connection); + + var services = CliTestHelper.CreateServiceCollection(workspace, outputHelper, options => + { + options.InteractionServiceFactory = _ => interactionService; + options.AuxiliaryBackchannelMonitorFactory = _ => monitor; + }); + using var provider = services.BuildServiceProvider(); + + var command = provider.GetRequiredService(); + var result = command.Parse("stop"); + + var exitCode = await result.InvokeAsync().DefaultTimeout(); + + Assert.Equal(CliExitCodes.Success, exitCode); + Assert.False(File.Exists(socketPath)); + } + private static TestAppHostAuxiliaryBackchannel CreateConnection(string appHostPath, int processId, bool isInScope = true) { return new TestAppHostAuxiliaryBackchannel diff --git a/tests/Aspire.Cli.Tests/Projects/RunningInstanceManagerTests.cs b/tests/Aspire.Cli.Tests/Projects/RunningInstanceManagerTests.cs new file mode 100644 index 00000000000..f948b5db2de --- /dev/null +++ b/tests/Aspire.Cli.Tests/Projects/RunningInstanceManagerTests.cs @@ -0,0 +1,163 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System.Net.Sockets; +using Aspire.Cli.Backchannel; +using Aspire.Cli.Projects; +using Aspire.Cli.Telemetry; +using Aspire.Cli.Tests.TestServices; +using Microsoft.AspNetCore.InternalTesting; +using Microsoft.Extensions.Configuration; +using Microsoft.Extensions.Logging.Abstractions; +using StreamJsonRpc; + +namespace Aspire.Cli.Tests.Projects; + +public class RunningInstanceManagerTests +{ + [Fact] + public async Task StopRunningInstanceAsync_DeletesSocketFile_WhenStopSucceeds() + { + // Regression test for https://github.com/microsoft/aspire/issues/17587. + // + // After a successful stop, the CLI must delete the auxiliary backchannel socket file. If the + // file is left behind, a later command (e.g. 'aspire add' or 'aspire stop') rediscovers it via + // FindMatchingNonOrphanedSockets and tries to connect to a now-defunct process, failing with + // "Unable to stop one or more running Aspire AppHost instances". This is most visible on Windows + // where the dead AppHost's PID can be reused (so the orphan-pruning heuristic believes the + // process is still alive), which is why a unit test is the deterministic, cross-platform guard. + + // The AppHost process reported by the fake backchannel must be one MonitorProcessesForTerminationAsync + // observes as already terminated so StopRunningInstanceAsync reaches the socket-cleanup branch. A fabricated, + // guaranteed-nonexistent PID makes Process.GetProcessById throw ArgumentException, which the monitor treats as + // "terminated". We deliberately do NOT start a real process and reuse its exited PID: the OS can recycle that + // exact PID for an unrelated live process before the monitor checks, which would make the monitor loop for the + // full timeout, return stopped == false, and flake the cleanup assertion. + const int exitedProcessId = int.MaxValue - 9; + + var socketDirectory = Directory.CreateTempSubdirectory("aspire-rim-"); + try + { + // Keep the socket path short: a UnixDomainSocketEndPoint is limited to ~104 bytes on macOS. + var socketPath = Path.Combine(socketDirectory.FullName, "a.sock"); + using var server = TestAuxiliaryBackchannelUdsServer.Start(socketPath, exitedProcessId); + + // Binding the server creates the socket file on disk. + Assert.True(File.Exists(socketPath)); + + var manager = new RunningInstanceManager(NullLogger.Instance, new TestInteractionService(), TimeProvider.System, new ProfilingTelemetry(new ConfigurationBuilder().Build())); + + var stopped = await manager.StopRunningInstanceAsync(socketPath, CancellationToken.None).DefaultTimeout(); + + Assert.True(stopped); + Assert.True(server.Target.StopRequested); + // The fix: the socket file must be removed once the instance has been stopped. + Assert.False(File.Exists(socketPath)); + } + finally + { + socketDirectory.Delete(recursive: true); + } + } + + private sealed class TestAuxiliaryBackchannelUdsServer : IDisposable + { + private readonly Socket _listener; + private readonly CancellationTokenSource _cts = new(); + private readonly List _disposables = []; + + private TestAuxiliaryBackchannelUdsServer(string socketPath, int appHostProcessId) + { + Target = new StoppableAppHostRpcTarget(appHostProcessId); + _listener = new Socket(AddressFamily.Unix, SocketType.Stream, ProtocolType.Unspecified); + _listener.Bind(new UnixDomainSocketEndPoint(socketPath)); + _listener.Listen(1); + } + + public StoppableAppHostRpcTarget Target { get; } + + public static TestAuxiliaryBackchannelUdsServer Start(string socketPath, int appHostProcessId) + { + var server = new TestAuxiliaryBackchannelUdsServer(socketPath, appHostProcessId); + _ = server.AcceptConnectionAsync(); + return server; + } + + private async Task AcceptConnectionAsync() + { + try + { + var serverSocket = await _listener.AcceptAsync(_cts.Token).ConfigureAwait(false); + var serverStream = new NetworkStream(serverSocket, ownsSocket: true); + var messageHandler = new HeaderDelimitedMessageHandler(serverStream, serverStream, BackchannelJsonSerializerContext.CreateRpcMessageFormatter()); + var rpc = new JsonRpc(messageHandler, Target); + rpc.StartListening(); + _disposables.Add(rpc); + _disposables.Add(messageHandler); + _disposables.Add(serverStream); + } + catch (Exception ex) when (ex is OperationCanceledException or ObjectDisposedException) + { + // Expected when the server is disposed before/while a connection is accepted. + } + } + + public void Dispose() + { + _cts.Cancel(); + foreach (var disposable in _disposables) + { + disposable.Dispose(); + } + + _listener.Dispose(); + _cts.Dispose(); + } + } + + private sealed class StoppableAppHostRpcTarget + { + private readonly int _processId; + private readonly string[] _capabilities = + [ + AuxiliaryBackchannelCapabilities.V1, + AuxiliaryBackchannelCapabilities.V2 + ]; + + public StoppableAppHostRpcTarget(int processId) + { + _processId = processId; + } + + public bool StopRequested { get; private set; } + + public Task GetAppHostInformationAsync(CancellationToken cancellationToken = default) + { + _ = cancellationToken; + + return Task.FromResult(new AppHostInformation + { + AppHostPath = "/path/to/AppHost.csproj", + ProcessId = _processId + }); + } + + public Task GetCapabilitiesAsync(GetCapabilitiesRequest? request = null, CancellationToken cancellationToken = default) + { + _ = request; + _ = cancellationToken; + + return Task.FromResult(new GetCapabilitiesResponse + { + Capabilities = _capabilities + }); + } + + public Task StopAppHostAsync(CancellationToken cancellationToken = default) + { + _ = cancellationToken; + StopRequested = true; + return Task.CompletedTask; + } + } +}