From 5d11095b7937e61ba3c98fdb8903059f696ebd93 Mon Sep 17 00:00:00 2001 From: "Calvin A. Allen" Date: Wed, 5 Aug 2026 17:16:31 -0400 Subject: [PATCH 1/2] fix(server): prevent devenv.exe from hanging on shutdown Package disposal blocked the Visual Studio UI thread on ServerProcessManager .StopAsync(), whose awaits captured that thread's synchronization context and posted their continuations straight back to it. The shutdown never completed, so devenv.exe stayed resident after the main window closed. The deadlock only reproduced with the server running: with no child process and no listening pipe, every await in the path completed synchronously and nothing was ever posted back. - Use ConfigureAwait(false) throughout the shutdown path in ServerProcessManager and RpcServer. - Run shutdown via Task.Run and wait with a timeout in package disposal, so no continuation can need the UI thread and VS exits regardless. - Bound the cooperative shutdown. A half-open pipe could leave the RPC shutdown call pending forever; the previous 5s + 2s waits were also unbounded in the worst case. - Assign the server process to a kill-on-close job object so it dies with Visual Studio even when devenv.exe terminates abnormally, rather than surviving to hold its HTTP port against the next session. - Stop suppressing VSTHRD002 project-wide. That analyzer flagged this exact deadlock; it is now suppressed only at the two Dispose call sites that genuinely require a blocking wait, each with a justification. - Fix the pipe listener faulting its task when cancelled during the retry backoff, and avoid disposing the cancellation source out from under it. Adds a test project with regression coverage that drives the shutdown path from a thread whose synchronization context cannot run posted callbacks. Both new tests fail against the previous code and pass against this change. --- .github/workflows/test.yml | 25 +++ .../CodingWithCalvin.MCPServer.Tests.csproj | 29 +++ .../ProcessJobObjectTests.cs | 122 ++++++++++++ .../ServerShutdownTests.cs | 114 +++++++++++ src/CodingWithCalvin.MCPServer.slnx | 1 + .../CodingWithCalvin.MCPServer.csproj | 13 +- .../MCPServerPackage.cs | 59 +++++- .../Services/ProcessJobObject.cs | 183 ++++++++++++++++++ .../Services/RpcServer.cs | 83 ++++++-- .../Services/ServerProcessManager.cs | 97 ++++++++-- 10 files changed, 687 insertions(+), 39 deletions(-) create mode 100644 .github/workflows/test.yml create mode 100644 src/CodingWithCalvin.MCPServer.Tests/CodingWithCalvin.MCPServer.Tests.csproj create mode 100644 src/CodingWithCalvin.MCPServer.Tests/ProcessJobObjectTests.cs create mode 100644 src/CodingWithCalvin.MCPServer.Tests/ServerShutdownTests.cs create mode 100644 src/CodingWithCalvin.MCPServer/Services/ProcessJobObject.cs diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml new file mode 100644 index 0000000..2806a60 --- /dev/null +++ b/.github/workflows/test.yml @@ -0,0 +1,25 @@ +name: Test + +on: + workflow_dispatch: + pull_request: + types: [opened, reopened, synchronize] + paths: + - 'src/**' + - '.github/workflows/test.yml' + push: + branches: + - main + paths: + - 'src/**' + - '.github/workflows/test.yml' + +jobs: + test: + runs-on: windows-latest + + steps: + - uses: actions/checkout@v4 + + - name: Run Tests + run: dotnet test src/CodingWithCalvin.MCPServer.Tests/CodingWithCalvin.MCPServer.Tests.csproj -c Release 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}"); From ca8fca75911af7923104d334e015624dfdd74b56 Mon Sep 17 00:00:00 2001 From: "Calvin A. Allen" Date: Wed, 5 Aug 2026 17:26:55 -0400 Subject: [PATCH 2/2] ci: run tests from the build workflow instead of a separate one The test project references the extension project, so a standalone test workflow rebuilt the extension and its self-contained server publish a second time on every PR. Passing test-project to the shared vsix-build workflow runs the tests in the same job, reusing the build output. Requires CodingWithCalvin/.github#76. --- .github/workflows/build.yml | 1 + .github/workflows/test.yml | 25 -------- .../source.extension.vsixmanifest | 60 +++++++++---------- 3 files changed, 31 insertions(+), 55 deletions(-) delete mode 100644 .github/workflows/test.yml 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/.github/workflows/test.yml b/.github/workflows/test.yml deleted file mode 100644 index 2806a60..0000000 --- a/.github/workflows/test.yml +++ /dev/null @@ -1,25 +0,0 @@ -name: Test - -on: - workflow_dispatch: - pull_request: - types: [opened, reopened, synchronize] - paths: - - 'src/**' - - '.github/workflows/test.yml' - push: - branches: - - main - paths: - - 'src/**' - - '.github/workflows/test.yml' - -jobs: - test: - runs-on: windows-latest - - steps: - - uses: actions/checkout@v4 - - - name: Run Tests - run: dotnet test src/CodingWithCalvin.MCPServer.Tests/CodingWithCalvin.MCPServer.Tests.csproj -c Release 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