diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index bb89963..1b09797 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -19,4 +19,5 @@ jobs: uses: CodingWithCalvin/.github/.github/workflows/vsix-build.yml@main with: extension-name: MCPServer + test-project: src/CodingWithCalvin.MCPServer.Tests/CodingWithCalvin.MCPServer.Tests.csproj secrets: inherit diff --git a/src/CodingWithCalvin.MCPServer.Tests/CodingWithCalvin.MCPServer.Tests.csproj b/src/CodingWithCalvin.MCPServer.Tests/CodingWithCalvin.MCPServer.Tests.csproj new file mode 100644 index 0000000..f6576a9 --- /dev/null +++ b/src/CodingWithCalvin.MCPServer.Tests/CodingWithCalvin.MCPServer.Tests.csproj @@ -0,0 +1,29 @@ + + + + net48 + latest + enable + false + CodingWithCalvin.MCPServer.Tests + + $(NoWarn);VSTHRD002;VSTHRD103 + + + + + + + runtime; build; native; contentfiles; analyzers; buildtransitive + all + + + + + + + + diff --git a/src/CodingWithCalvin.MCPServer.Tests/ProcessJobObjectTests.cs b/src/CodingWithCalvin.MCPServer.Tests/ProcessJobObjectTests.cs new file mode 100644 index 0000000..031b29e --- /dev/null +++ b/src/CodingWithCalvin.MCPServer.Tests/ProcessJobObjectTests.cs @@ -0,0 +1,122 @@ +using System; +using System.Diagnostics; +using CodingWithCalvin.MCPServer.Services; +using Xunit; + +namespace CodingWithCalvin.MCPServer.Tests; + +/// +/// Covers the kill-on-close backstop that stops the MCP server process from outliving Visual +/// Studio when devenv.exe terminates without running its normal shutdown path. +/// +public class ProcessJobObjectTests +{ + private const int ExitWaitMs = 5000; + + [Fact] + public void Create_ReturnsJobObject() + { + using var job = ProcessJobObject.Create(); + + Assert.NotNull(job); + } + + [Fact] + public void Dispose_TerminatesAssignedProcess() + { + var process = StartLongRunningProcess(); + + try + { + var job = ProcessJobObject.Create(); + Assert.NotNull(job); + + Assert.True(job!.TryAssign(process), "Failed to assign the process to the job object."); + Assert.False(process.HasExited, "The child process exited before the job was closed."); + + job.Dispose(); + + Assert.True( + process.WaitForExit(ExitWaitMs), + "Closing the job object did not terminate the assigned process."); + } + finally + { + KillIfRunning(process); + process.Dispose(); + } + } + + [Fact] + public void TryAssign_ReturnsFalse_AfterDispose() + { + var job = ProcessJobObject.Create(); + Assert.NotNull(job); + job!.Dispose(); + + var process = StartLongRunningProcess(); + + try + { + Assert.False(job.TryAssign(process)); + } + finally + { + KillIfRunning(process); + process.Dispose(); + } + } + + [Fact] + public void Dispose_IsIdempotent() + { + var job = ProcessJobObject.Create(); + Assert.NotNull(job); + + job!.Dispose(); + job.Dispose(); + } + + [Fact] + public void TryAssign_Throws_ForNullProcess() + { + using var job = ProcessJobObject.Create(); + + Assert.Throws(() => job!.TryAssign(null!)); + } + + /// + /// Starts a process that runs for long enough that any exit observed during a test is + /// attributable to the job object rather than to natural termination. + /// + private static Process StartLongRunningProcess() + { + var startInfo = new ProcessStartInfo("ping.exe", "-n 120 127.0.0.1") + { + UseShellExecute = false, + CreateNoWindow = true, + RedirectStandardOutput = true, + }; + + var process = Process.Start(startInfo); + Assert.NotNull(process); + + return process!; + } + + private static void KillIfRunning(Process process) + { + try + { + if (!process.HasExited) + { + process.Kill(); + process.WaitForExit(ExitWaitMs); + } + } + catch (InvalidOperationException) + { + // Already gone. + } + } +} diff --git a/src/CodingWithCalvin.MCPServer.Tests/ServerShutdownTests.cs b/src/CodingWithCalvin.MCPServer.Tests/ServerShutdownTests.cs new file mode 100644 index 0000000..37640a7 --- /dev/null +++ b/src/CodingWithCalvin.MCPServer.Tests/ServerShutdownTests.cs @@ -0,0 +1,114 @@ +using System; +using System.Collections.Generic; +using System.Threading; +using System.Threading.Tasks; +using CodingWithCalvin.MCPServer.Services; +using CodingWithCalvin.MCPServer.Shared.Models; +using Xunit; + +namespace CodingWithCalvin.MCPServer.Tests; + +/// +/// Regression tests for issue #97: package disposal blocked the Visual Studio UI thread waiting +/// on StopAsync, whose continuations were posted straight back to that same blocked +/// thread. devenv.exe never finished exiting and stayed resident in Task Manager. +/// +public class ServerShutdownTests +{ + private static readonly TimeSpan CompletionTimeout = TimeSpan.FromSeconds(5); + private static readonly TimeSpan ThreadJoinTimeout = TimeSpan.FromSeconds(30); + + [Fact] + public void ServerProcessManager_StopAsync_CompletesWhenCallerBlocksItsSynchronizationContext() + { + var completed = RunWithBlockedSynchronizationContext(() => + { + var manager = new ServerProcessManager(new StubRpcServer()); + return manager.StopAsync(); + }); + + Assert.True( + completed, + "ServerProcessManager.StopAsync did not complete. A continuation is being posted back to " + + "the caller's synchronization context, which is the deadlock from issue #97. " + + "Every await in the shutdown path needs ConfigureAwait(false)."); + } + + /// + /// Mirrors the shape of package disposal itself: a synchronous, blocking wait on the shutdown + /// path from a thread that cannot pump its own message queue. + /// + [Fact] + public void BlockingOnStopAsync_Returns_WhenCallerBlocksItsSynchronizationContext() + { + var completed = RunWithBlockedSynchronizationContext(() => + { + var manager = new ServerProcessManager(new StubRpcServer()); + manager.StopAsync().GetAwaiter().GetResult(); + return Task.CompletedTask; + }); + + Assert.True(completed, "Blocking on the shutdown path deadlocked the calling thread."); + } + + /// + /// Runs on a thread whose synchronization context silently drops + /// everything posted to it — the observable behaviour of a UI thread blocked inside + /// Dispose. Any continuation that tries to resume on the caller's context will never + /// run, so the returned task never completes and this reports . + /// + private static bool RunWithBlockedSynchronizationContext(Func operation) + { + var completed = false; + + var thread = new Thread(() => + { + SynchronizationContext.SetSynchronizationContext(new BlockedSynchronizationContext()); + completed = operation().Wait(CompletionTimeout); + }) + { + IsBackground = true, + }; + + thread.Start(); + thread.Join(ThreadJoinTimeout); + + return completed; + } + + private sealed class BlockedSynchronizationContext : SynchronizationContext + { + public override void Post(SendOrPostCallback d, object? state) + { + // Deliberately dropped: the thread that would pump this is blocked. + } + + public override void Send(SendOrPostCallback d, object? state) + => throw new NotSupportedException("The blocked thread cannot run work synchronously."); + } + + /// + /// Minimal whose async members complete on the thread pool, which is + /// what makes the captured-context bug observable. + /// + private sealed class StubRpcServer : IRpcServer + { + public string PipeName => string.Empty; + + public bool IsListening => true; + + public bool IsConnected => false; + + public Task StartAsync(string pipeName) => Task.CompletedTask; + + public async Task StopAsync() => await Task.Delay(25).ConfigureAwait(false); + + public Task> GetAvailableToolsAsync() => Task.FromResult(new List()); + + public async Task RequestShutdownAsync() => await Task.Delay(25).ConfigureAwait(false); + + public void Dispose() + { + } + } +} diff --git a/src/CodingWithCalvin.MCPServer.slnx b/src/CodingWithCalvin.MCPServer.slnx index aaa4ddd..8894d07 100644 --- a/src/CodingWithCalvin.MCPServer.slnx +++ b/src/CodingWithCalvin.MCPServer.slnx @@ -2,4 +2,5 @@ + diff --git a/src/CodingWithCalvin.MCPServer/CodingWithCalvin.MCPServer.csproj b/src/CodingWithCalvin.MCPServer/CodingWithCalvin.MCPServer.csproj index 991f5f6..8516506 100644 --- a/src/CodingWithCalvin.MCPServer/CodingWithCalvin.MCPServer.csproj +++ b/src/CodingWithCalvin.MCPServer/CodingWithCalvin.MCPServer.csproj @@ -5,10 +5,19 @@ latest enable CodingWithCalvin.MCPServer - - $(NoWarn);VSTHRD002;VSTHRD003;VSTHRD010;VSTHRD110;VSSDK007 + + $(NoWarn);VSTHRD003;VSTHRD010;VSTHRD110;VSSDK007 + + + + diff --git a/src/CodingWithCalvin.MCPServer/MCPServerPackage.cs b/src/CodingWithCalvin.MCPServer/MCPServerPackage.cs index e394f62..e456031 100644 --- a/src/CodingWithCalvin.MCPServer/MCPServerPackage.cs +++ b/src/CodingWithCalvin.MCPServer/MCPServerPackage.cs @@ -102,13 +102,68 @@ public void InitializeServices() } } + /// + /// Total time package disposal will spend shutting the server down before giving up and + /// letting Visual Studio finish exiting. + /// + private static readonly TimeSpan ShutdownTimeout = TimeSpan.FromSeconds(5); + + /// + /// Visual Studio calls this on the UI thread. Shutdown work is therefore pushed onto the + /// thread pool and waited on with a timeout: starts with + /// no synchronization context, so no continuation can need the UI thread back, and the + /// timeout bounds the damage if one ever does. Blocking the UI thread directly on + /// StopAsync deadlocked and left devenv.exe resident after the main window closed + /// (issue #97). + /// protected override void Dispose(bool disposing) { if (disposing) { - ServerManager?.StopAsync().GetAwaiter().GetResult(); - RpcServer?.Dispose(); + var serverManager = ServerManager; + + if (serverManager != null) + { + try + { + var stopTask = Task.Run(() => serverManager.StopAsync()); + + // VSTHRD002: Dispose cannot be async, so a blocking wait is unavoidable. It is + // safe here because Task.Run starts the work without a synchronization context + // and StopAsync uses ConfigureAwait(false) throughout, so no continuation can + // require this thread. The timeout guarantees VS exits regardless. +#pragma warning disable VSTHRD002 + if (!stopTask.Wait(ShutdownTimeout)) +#pragma warning restore VSTHRD002 + { + // The job object assigned at start-up still guarantees the server + // process dies when devenv.exe does, so exiting is safe here. + System.Diagnostics.Debug.WriteLine("MCPServer: server shutdown timed out during package disposal."); + } + } + catch (Exception ex) + { + // A failure to stop the server must never prevent Visual Studio from exiting. + System.Diagnostics.Debug.WriteLine($"MCPServer: error stopping server during package disposal: {ex}"); + } + } + + try + { + RpcServer?.Dispose(); + } + catch (Exception ex) + { + System.Diagnostics.Debug.WriteLine($"MCPServer: error disposing RPC server: {ex}"); + } + VsixTelemetry.Shutdown(); + + ServerManager = null; + RpcServer = null; + VsService = null; + OutputPaneService = null; + Settings = null; Instance = null; } diff --git a/src/CodingWithCalvin.MCPServer/Services/ProcessJobObject.cs b/src/CodingWithCalvin.MCPServer/Services/ProcessJobObject.cs new file mode 100644 index 0000000..df5ed3d --- /dev/null +++ b/src/CodingWithCalvin.MCPServer/Services/ProcessJobObject.cs @@ -0,0 +1,183 @@ +using System; +using System.Diagnostics; +using System.Runtime.InteropServices; +using Microsoft.Win32.SafeHandles; + +namespace CodingWithCalvin.MCPServer.Services; + +/// +/// Wraps a Windows job object configured with JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE. +/// Any process assigned to the job is terminated by the operating system as soon as the +/// job handle closes, which happens when this instance is disposed or when the owning +/// process (devenv.exe) exits for any reason. +/// +/// +/// This is the backstop for orphaned server processes. Cooperative shutdown handles the +/// normal case, but if Visual Studio crashes or is killed from Task Manager it never gets +/// the chance to run, and the server process would otherwise survive and keep holding its +/// HTTP port against the next Visual Studio session. +/// +internal sealed class ProcessJobObject : IDisposable +{ + private SafeJobHandle? _handle; + + private ProcessJobObject(SafeJobHandle handle) + { + _handle = handle; + } + + /// + /// Creates a kill-on-close job object, or returns if the + /// operating system refuses. Callers treat a null result as "backstop unavailable" + /// and continue without it. + /// + public static ProcessJobObject? Create() + { + var handle = NativeMethods.CreateJobObject(IntPtr.Zero, null); + + if (handle.IsInvalid) + { + handle.Dispose(); + return null; + } + + var extendedLimits = new NativeMethods.JOBOBJECT_EXTENDED_LIMIT_INFORMATION(); + extendedLimits.BasicLimitInformation.LimitFlags = NativeMethods.JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE; + + var length = Marshal.SizeOf(typeof(NativeMethods.JOBOBJECT_EXTENDED_LIMIT_INFORMATION)); + var buffer = Marshal.AllocHGlobal(length); + + try + { + Marshal.StructureToPtr(extendedLimits, buffer, false); + + var configured = NativeMethods.SetInformationJobObject( + handle, + NativeMethods.JobObjectExtendedLimitInformation, + buffer, + (uint)length); + + if (!configured) + { + handle.Dispose(); + return null; + } + } + finally + { + Marshal.FreeHGlobal(buffer); + } + + return new ProcessJobObject(handle); + } + + /// + /// Assigns to the job. Returns if the + /// assignment fails, in which case the caller simply loses the kill-on-close guarantee. + /// + public bool TryAssign(Process process) + { + if (process == null) + { + throw new ArgumentNullException(nameof(process)); + } + + var handle = _handle; + + if (handle == null || handle.IsInvalid || handle.IsClosed) + { + return false; + } + + try + { + return NativeMethods.AssignProcessToJobObject(handle, process.Handle); + } + catch (InvalidOperationException) + { + // The process exited before we could assign it. + return false; + } + } + + public void Dispose() + { + var handle = _handle; + _handle = null; + handle?.Dispose(); + } + + private sealed class SafeJobHandle : SafeHandleZeroOrMinusOneIsInvalid + { + public SafeJobHandle() : base(true) + { + } + + protected override bool ReleaseHandle() => NativeMethods.CloseHandle(handle); + } + + private static class NativeMethods + { + public const int JobObjectExtendedLimitInformation = 9; + public const uint JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE = 0x2000; + + [DllImport("kernel32.dll", CharSet = CharSet.Unicode, SetLastError = true)] + public static extern SafeJobHandle CreateJobObject(IntPtr lpJobAttributes, string? lpName); + + [DllImport("kernel32.dll", SetLastError = true)] + [return: MarshalAs(UnmanagedType.Bool)] + public static extern bool SetInformationJobObject( + SafeJobHandle hJob, + int jobObjectInfoClass, + IntPtr lpJobObjectInfo, + uint cbJobObjectInfoLength); + + [DllImport("kernel32.dll", SetLastError = true)] + [return: MarshalAs(UnmanagedType.Bool)] + public static extern bool AssignProcessToJobObject(SafeJobHandle hJob, IntPtr hProcess); + + [DllImport("kernel32.dll", SetLastError = true)] + [return: MarshalAs(UnmanagedType.Bool)] + public static extern bool CloseHandle(IntPtr hObject); + + // Fields on these interop structures are populated by the marshaller rather than by + // managed code, so the compiler cannot see them being assigned. +#pragma warning disable CS0649 + [StructLayout(LayoutKind.Sequential)] + public struct JOBOBJECT_BASIC_LIMIT_INFORMATION + { + public long PerProcessUserTimeLimit; + public long PerJobUserTimeLimit; + public uint LimitFlags; + public UIntPtr MinimumWorkingSetSize; + public UIntPtr MaximumWorkingSetSize; + public uint ActiveProcessLimit; + public UIntPtr Affinity; + public uint PriorityClass; + public uint SchedulingClass; + } + + [StructLayout(LayoutKind.Sequential)] + public struct IO_COUNTERS + { + public ulong ReadOperationCount; + public ulong WriteOperationCount; + public ulong OtherOperationCount; + public ulong ReadTransferCount; + public ulong WriteTransferCount; + public ulong OtherTransferCount; + } + + [StructLayout(LayoutKind.Sequential)] + public struct JOBOBJECT_EXTENDED_LIMIT_INFORMATION + { + public JOBOBJECT_BASIC_LIMIT_INFORMATION BasicLimitInformation; + public IO_COUNTERS IoInfo; + public UIntPtr ProcessMemoryLimit; + public UIntPtr JobMemoryLimit; + public UIntPtr PeakProcessMemoryUsed; + public UIntPtr PeakJobMemoryUsed; + } +#pragma warning restore CS0649 + } +} diff --git a/src/CodingWithCalvin.MCPServer/Services/RpcServer.cs b/src/CodingWithCalvin.MCPServer/Services/RpcServer.cs index cc1d194..b7259e2 100644 --- a/src/CodingWithCalvin.MCPServer/Services/RpcServer.cs +++ b/src/CodingWithCalvin.MCPServer/Services/RpcServer.cs @@ -14,6 +14,12 @@ namespace CodingWithCalvin.MCPServer.Services; [PartCreationPolicy(CreationPolicy.Shared)] public class RpcServer : IRpcServer, IVisualStudioRpc { + /// How long waits for the pipe listener loop to unwind. + private const int ListenerStopTimeoutMs = 2000; + + /// Upper bound on the blocking wait performed by . + private const int DisposeTimeoutMs = 3000; + private readonly IVisualStudioService _vsService; private NamedPipeServerStream? _pipeServer; private JsonRpc? _jsonRpc; @@ -60,11 +66,11 @@ private async Task ListenAsync(CancellationToken cancellationToken) PipeTransmissionMode.Byte, PipeOptions.Asynchronous); - await _pipeServer.WaitForConnectionAsync(cancellationToken); + await _pipeServer.WaitForConnectionAsync(cancellationToken).ConfigureAwait(false); _jsonRpc = JsonRpc.Attach(_pipeServer, this); _serverProxy = _jsonRpc.Attach(); - await _jsonRpc.Completion; + await _jsonRpc.Completion.ConfigureAwait(false); } catch (OperationCanceledException) { @@ -72,8 +78,22 @@ private async Task ListenAsync(CancellationToken cancellationToken) } catch (Exception) { - // Connection lost, restart listening - await Task.Delay(100, cancellationToken); + // Connection lost, restart listening — unless we are shutting down, in which + // case backing off would throw straight out of this catch block and fault the + // listener task. + if (cancellationToken.IsCancellationRequested) + { + break; + } + + try + { + await Task.Delay(100, cancellationToken).ConfigureAwait(false); + } + catch (Exception) + { + break; + } } finally { @@ -86,6 +106,10 @@ private async Task ListenAsync(CancellationToken cancellationToken) } } + /// + /// Called from package disposal, so every await must use ConfigureAwait(false) — + /// see the note on . + /// public async Task StopAsync() { if (!IsListening) @@ -94,38 +118,52 @@ public async Task StopAsync() } IsListening = false; - _cts?.Cancel(); + + var cts = _cts; + _cts = null; + cts?.Cancel(); // Dispose JsonRpc to break out of the Completion await _jsonRpc?.Dispose(); _pipeServer?.Dispose(); - if (_listenerTask != null) + var listenerTask = _listenerTask; + _listenerTask = null; + + var listenerStopped = true; + + if (listenerTask != null) { try { // Use a timeout to prevent hanging forever - var timeoutTask = Task.Delay(2000); - var completedTask = await Task.WhenAny(_listenerTask, timeoutTask); - if (completedTask == timeoutTask) - { - // Listener didn't stop in time, just continue - } + var timeoutTask = Task.Delay(ListenerStopTimeoutMs); + var completedTask = await Task.WhenAny(listenerTask, timeoutTask).ConfigureAwait(false); + listenerStopped = completedTask != timeoutTask; } catch (OperationCanceledException) { // Expected } - catch + catch (Exception) { // Ignore other exceptions during shutdown } } - _cts?.Dispose(); - _cts = null; + // Only dispose the token source once nothing can still be observing the token; + // disposing it out from under a running listener throws inside that loop. + if (listenerStopped) + { + cts?.Dispose(); + } } + /// + /// never resumes on the caller's synchronization context, so this + /// blocking wait cannot deadlock the UI thread during package disposal (issue #97). The + /// timeout is a second line of defence. + /// public void Dispose() { if (_disposed) @@ -134,7 +172,20 @@ public void Dispose() } _disposed = true; - StopAsync().GetAwaiter().GetResult(); + + try + { + // VSTHRD002: IDisposable.Dispose cannot be async. Safe for the same reasons as the + // wait in MCPServerPackage.Dispose — no synchronization context is captured anywhere + // in StopAsync — and bounded by a timeout besides. +#pragma warning disable VSTHRD002 + Task.Run(() => StopAsync()).Wait(DisposeTimeoutMs); +#pragma warning restore VSTHRD002 + } + catch (Exception) + { + // Disposal must not throw during Visual Studio shutdown. + } } public async Task> GetAvailableToolsAsync() diff --git a/src/CodingWithCalvin.MCPServer/Services/ServerProcessManager.cs b/src/CodingWithCalvin.MCPServer/Services/ServerProcessManager.cs index 7c32731..55c72c8 100644 --- a/src/CodingWithCalvin.MCPServer/Services/ServerProcessManager.cs +++ b/src/CodingWithCalvin.MCPServer/Services/ServerProcessManager.cs @@ -15,8 +15,18 @@ namespace CodingWithCalvin.MCPServer.Services; [PartCreationPolicy(CreationPolicy.Shared)] public class ServerProcessManager : IServerProcessManager { + /// How long to wait for the server to acknowledge the RPC shutdown request. + private const int RpcShutdownTimeoutMs = 1000; + + /// How long to wait for the server process to exit on its own afterwards. + private const int GracefulExitTimeoutMs = 1500; + + /// How long to wait for the process to die after being killed. + private const int ForcedExitTimeoutMs = 500; + private readonly IRpcServer _rpcServer; private Process? _serverProcess; + private ProcessJobObject? _jobObject; private string _pipeName = string.Empty; private StreamWriter? _logFileWriter; private string? _logFilePath; @@ -45,7 +55,7 @@ public async Task StartAsync(ServerStartSettings settings) _pipeName = $"vsmcp-{Process.GetCurrentProcess().Id}"; // Start the RPC server first - await _rpcServer.StartAsync(_pipeName); + await _rpcServer.StartAsync(_pipeName).ConfigureAwait(false); // Find the server executable var extensionDir = Path.GetDirectoryName(Assembly.GetExecutingAssembly().Location); @@ -82,6 +92,10 @@ public async Task StartAsync(ServerStartSettings settings) process.EnableRaisingEvents = true; process.Exited += OnProcessExited; + // Tie the server's lifetime to this process. Cooperative shutdown below is the + // normal path; the job object is what saves us when devenv.exe dies without it. + AssignToJobObject(process); + // Start reading output streams (server logs go to stderr by convention) _ = ReadOutputAsync(process.StandardOutput); _ = ReadOutputAsync(process.StandardError); @@ -91,7 +105,7 @@ public async Task StartAsync(ServerStartSettings settings) Log($"Log file: {_logFilePath}"); // Give the server a moment to start - await Task.Delay(500); + await Task.Delay(500).ConfigureAwait(false); // Check if process exited if (process.HasExited) @@ -100,52 +114,97 @@ public async Task StartAsync(ServerStartSettings settings) } } + /// + /// Every await in this method — and everything it calls — must use + /// ConfigureAwait(false). Package disposal blocks a thread waiting on this, so a + /// continuation that needs the caller's synchronization context back would deadlock and + /// leave devenv.exe running forever (issue #97). The total wall time is also bounded, so + /// an unresponsive server cannot stall Visual Studio's shutdown. + /// public async Task StopAsync() { // Capture reference to avoid race conditions during shutdown var process = _serverProcess; + _serverProcess = null; - if (process != null && !process.HasExited) + if (process != null) { try { - Log("Stopping server..."); - - // Unsubscribe from Exited event to prevent duplicate logging - process.Exited -= OnProcessExited; + if (!process.HasExited) + { + Log("Stopping server..."); - // Request graceful shutdown via RPC - await _rpcServer.RequestShutdownAsync(); + // Unsubscribe from Exited event to prevent duplicate logging + process.Exited -= OnProcessExited; - // Wait for process to exit gracefully (up to 5 seconds) - var exited = await Task.Run(() => process.WaitForExit(5000)); + await RequestGracefulShutdownAsync(process).ConfigureAwait(false); + } - if (!exited) + if (!process.HasExited) { // Force kill if graceful shutdown timed out Log("Graceful shutdown timed out, forcing termination..."); process.Kill(); - await Task.Run(() => process.WaitForExit(2000)); + await Task.Run(() => process.WaitForExit(ForcedExitTimeoutMs)).ConfigureAwait(false); } Log($"Server stopped (Code: {process.ExitCode})"); } - catch + catch (Exception ex) + { + // Never allow a shutdown failure to propagate into package disposal. + Log($"Error stopping server: {ex.Message}"); + } + finally { - // Process may have already exited + process.Dispose(); } } - _serverProcess?.Dispose(); - _serverProcess = null; + // Closing the job terminates the server process if it somehow outlived the above. + _jobObject?.Dispose(); + _jobObject = null; - await _rpcServer.StopAsync(); + await _rpcServer.StopAsync().ConfigureAwait(false); // Close log file _logFileWriter?.Dispose(); _logFileWriter = null; } + /// + /// Asks the server to shut down over RPC and waits briefly for it to exit. Both steps are + /// individually bounded because a half-open named pipe can leave an RPC call pending + /// indefinitely. + /// + private async Task RequestGracefulShutdownAsync(Process process) + { + // RequestShutdownAsync swallows its own errors, so the abandoned task on timeout is + // harmless and cannot surface as an unobserved exception. + var shutdownRequest = _rpcServer.RequestShutdownAsync(); + await Task.WhenAny(shutdownRequest, Task.Delay(RpcShutdownTimeoutMs)).ConfigureAwait(false); + + await Task.Run(() => process.WaitForExit(GracefulExitTimeoutMs)).ConfigureAwait(false); + } + + private void AssignToJobObject(Process process) + { + try + { + _jobObject ??= ProcessJobObject.Create(); + + if (_jobObject == null || !_jobObject.TryAssign(process)) + { + Log("Warning: could not assign the server to a job object; it may outlive Visual Studio if devenv.exe terminates abnormally."); + } + } + catch (Exception ex) + { + Log($"Warning: job object setup failed ({ex.Message}); the server may outlive Visual Studio if devenv.exe terminates abnormally."); + } + } + private void InitializeLogging(ServerStartSettings settings) { // Create log file in temp directory (daily rotation) @@ -215,7 +274,7 @@ private async Task ReadOutputAsync(StreamReader reader) { while (!reader.EndOfStream) { - var line = await reader.ReadLineAsync(); + var line = await reader.ReadLineAsync().ConfigureAwait(false); if (!string.IsNullOrEmpty(line)) { Log($"[SERVER] {line}"); diff --git a/src/CodingWithCalvin.MCPServer/source.extension.vsixmanifest b/src/CodingWithCalvin.MCPServer/source.extension.vsixmanifest index 5398478..64634ab 100644 --- a/src/CodingWithCalvin.MCPServer/source.extension.vsixmanifest +++ b/src/CodingWithCalvin.MCPServer/source.extension.vsixmanifest @@ -1,31 +1,31 @@ - + - - - MCP Server - Exposes Visual Studio features as an MCP (Model Context Protocol) server, enabling AI assistants like Claude to interact with Visual Studio programmatically. - https://github.com/CodingWithCalvin/VS-MCPServer - resources\LICENSE - resources\logo.png - resources\logo.png - MCP, AI, Claude, LLM, Automation - - - - amd64 - - - arm64 - - - - - - - - - - - - - + + + MCP Server + Exposes Visual Studio features as an MCP (Model Context Protocol) server, enabling AI assistants like Claude to interact with Visual Studio programmatically. + https://github.com/CodingWithCalvin/VS-MCPServer + resources\LICENSE + resources\logo.png + resources\logo.png + MCP, AI, Claude, LLM, Automation + + + + amd64 + + + arm64 + + + + + + + + + + + + + \ No newline at end of file