diff --git a/PolyPilot.Tests/PolyPilot.Tests.csproj b/PolyPilot.Tests/PolyPilot.Tests.csproj index 1ec6dde9..eda779fb 100644 --- a/PolyPilot.Tests/PolyPilot.Tests.csproj +++ b/PolyPilot.Tests/PolyPilot.Tests.csproj @@ -91,6 +91,7 @@ + diff --git a/PolyPilot.Tests/ProcessHelperTests.cs b/PolyPilot.Tests/ProcessHelperTests.cs new file mode 100644 index 00000000..093e2aad --- /dev/null +++ b/PolyPilot.Tests/ProcessHelperTests.cs @@ -0,0 +1,242 @@ +using System.Diagnostics; +using PolyPilot.Services; + +namespace PolyPilot.Tests; + +/// +/// Tests for ProcessHelper — safe wrappers for Process lifecycle operations +/// that prevent InvalidOperationException / UnobservedTaskException crashes +/// when a process is disposed while background tasks are still monitoring it. +/// +public class ProcessHelperTests +{ + // ===== SafeHasExited ===== + + [Fact] + public void SafeHasExited_NullProcess_ReturnsTrue() + { + Assert.True(ProcessHelper.SafeHasExited(null)); + } + + [Fact] + public void SafeHasExited_DisposedProcess_ReturnsTrue() + { + // Start a short-lived process and dispose it immediately + var psi = new ProcessStartInfo + { + FileName = OperatingSystem.IsWindows() ? "cmd.exe" : "/bin/sh", + Arguments = OperatingSystem.IsWindows() ? "/c echo test" : "-c \"echo test\"", + UseShellExecute = false, + RedirectStandardOutput = true, + CreateNoWindow = true + }; + var process = Process.Start(psi)!; + process.WaitForExit(5000); + process.Dispose(); + + // After disposal, HasExited would throw InvalidOperationException. + // SafeHasExited must return true instead. + Assert.True(ProcessHelper.SafeHasExited(process)); + } + + [Fact] + public void SafeHasExited_ExitedProcess_ReturnsTrue() + { + var psi = new ProcessStartInfo + { + FileName = OperatingSystem.IsWindows() ? "cmd.exe" : "/bin/sh", + Arguments = OperatingSystem.IsWindows() ? "/c echo done" : "-c \"echo done\"", + UseShellExecute = false, + RedirectStandardOutput = true, + CreateNoWindow = true + }; + var process = Process.Start(psi)!; + process.WaitForExit(5000); + + Assert.True(ProcessHelper.SafeHasExited(process)); + process.Dispose(); + } + + [Fact] + public void SafeHasExited_RunningProcess_ReturnsFalse() + { + // Start a long-running process + var psi = new ProcessStartInfo + { + FileName = OperatingSystem.IsWindows() ? "cmd.exe" : "/bin/sh", + Arguments = OperatingSystem.IsWindows() ? "/c ping -n 30 127.0.0.1 > nul" : "-c \"sleep 30\"", + UseShellExecute = false, + CreateNoWindow = true + }; + var process = Process.Start(psi)!; + try + { + Assert.False(ProcessHelper.SafeHasExited(process)); + } + finally + { + try { process.Kill(true); } catch { } + process.Dispose(); + } + } + + // ===== SafeKill ===== + + [Fact] + public void SafeKill_NullProcess_DoesNotThrow() + { + ProcessHelper.SafeKill(null); + } + + [Fact] + public void SafeKill_DisposedProcess_DoesNotThrow() + { + var psi = new ProcessStartInfo + { + FileName = OperatingSystem.IsWindows() ? "cmd.exe" : "/bin/sh", + Arguments = OperatingSystem.IsWindows() ? "/c echo test" : "-c \"echo test\"", + UseShellExecute = false, + CreateNoWindow = true + }; + var process = Process.Start(psi)!; + process.WaitForExit(5000); + process.Dispose(); + + // Must not throw + ProcessHelper.SafeKill(process); + } + + [Fact] + public void SafeKill_RunningProcess_KillsIt() + { + var psi = new ProcessStartInfo + { + FileName = OperatingSystem.IsWindows() ? "cmd.exe" : "/bin/sh", + Arguments = OperatingSystem.IsWindows() ? "/c ping -n 30 127.0.0.1 > nul" : "-c \"sleep 30\"", + UseShellExecute = false, + CreateNoWindow = true + }; + var process = Process.Start(psi)!; + + ProcessHelper.SafeKill(process); + process.WaitForExit(5000); + Assert.True(process.HasExited); + process.Dispose(); + } + + // ===== SafeKillAndDispose ===== + + [Fact] + public void SafeKillAndDispose_NullProcess_DoesNotThrow() + { + ProcessHelper.SafeKillAndDispose(null); + } + + [Fact] + public void SafeKillAndDispose_AlreadyDisposed_DoesNotThrow() + { + var psi = new ProcessStartInfo + { + FileName = OperatingSystem.IsWindows() ? "cmd.exe" : "/bin/sh", + Arguments = OperatingSystem.IsWindows() ? "/c echo test" : "-c \"echo test\"", + UseShellExecute = false, + CreateNoWindow = true + }; + var process = Process.Start(psi)!; + process.WaitForExit(5000); + process.Dispose(); + + // Calling SafeKillAndDispose on already-disposed process must not throw + ProcessHelper.SafeKillAndDispose(process); + } + + [Fact] + public void SafeKillAndDispose_RunningProcess_KillsAndDisposes() + { + var psi = new ProcessStartInfo + { + FileName = OperatingSystem.IsWindows() ? "cmd.exe" : "/bin/sh", + Arguments = OperatingSystem.IsWindows() ? "/c ping -n 30 127.0.0.1 > nul" : "-c \"sleep 30\"", + UseShellExecute = false, + CreateNoWindow = true + }; + var process = Process.Start(psi)!; + var pid = process.Id; + + ProcessHelper.SafeKillAndDispose(process); + + // Verify the process is no longer running + try + { + var check = Process.GetProcessById(pid); + // Process might still be there for a moment — give it time + check.WaitForExit(2000); + } + catch (ArgumentException) + { + // Process already gone — expected + } + } + + // ===== Race condition regression test ===== + + [Fact] + public void SafeHasExited_ConcurrentDispose_NoUnobservedTaskException() + { + // Regression test: simulates the race condition where a background task + // checks HasExited while another thread disposes the process. + using var unobservedSignal = new ManualResetEventSlim(false); + Exception? unobservedException = null; + EventHandler handler = (sender, args) => + { + if (args.Exception?.InnerException is InvalidOperationException) + { + unobservedException = args.Exception; + unobservedSignal.Set(); + } + }; + + TaskScheduler.UnobservedTaskException += handler; + try + { + for (int i = 0; i < 5; i++) + { + var psi = new ProcessStartInfo + { + FileName = OperatingSystem.IsWindows() ? "cmd.exe" : "/bin/sh", + Arguments = OperatingSystem.IsWindows() ? "/c ping -n 10 127.0.0.1 > nul" : "-c \"sleep 10\"", + UseShellExecute = false, + CreateNoWindow = true + }; + var process = Process.Start(psi)!; + + // Background task monitoring HasExited (like DevTunnel's fire-and-forget tasks) + _ = Task.Run(() => + { + for (int j = 0; j < 50; j++) + { + if (ProcessHelper.SafeHasExited(process)) + break; + Thread.Sleep(10); + } + }); + + // Simulate concurrent disposal (like Stop() being called) + Thread.Sleep(50); + ProcessHelper.SafeKillAndDispose(process); + } + + // Force GC to surface any unobserved task exceptions + GC.Collect(); + GC.WaitForPendingFinalizers(); + GC.Collect(); + + unobservedSignal.Wait(TimeSpan.FromMilliseconds(500)); + Assert.Null(unobservedException); + } + finally + { + TaskScheduler.UnobservedTaskException -= handler; + } + } +} diff --git a/PolyPilot/Services/CodespaceService.cs b/PolyPilot/Services/CodespaceService.cs index 2c9f3095..51206ba7 100644 --- a/PolyPilot/Services/CodespaceService.cs +++ b/PolyPilot/Services/CodespaceService.cs @@ -32,6 +32,7 @@ public sealed class TunnelHandle : IAsyncDisposable public int LocalPort { get; } public bool IsSshTunnel { get; } private readonly Process _process; + private volatile bool _disposed; internal TunnelHandle(int localPort, Process process, bool isSshTunnel = false) { @@ -40,18 +41,19 @@ internal TunnelHandle(int localPort, Process process, bool isSshTunnel = false) IsSshTunnel = isSshTunnel; } - public bool IsAlive => !_process.HasExited; + public bool IsAlive => !_disposed && !ProcessHelper.SafeHasExited(_process); public async ValueTask DisposeAsync() { + _disposed = true; try { - if (!_process.HasExited) + if (!ProcessHelper.SafeHasExited(_process)) _process.Kill(entireProcessTree: true); await _process.WaitForExitAsync(CancellationToken.None).WaitAsync(TimeSpan.FromSeconds(3)); } catch { } - _process.Dispose(); + try { _process.Dispose(); } catch { } } } /// diff --git a/PolyPilot/Services/DevTunnelService.cs b/PolyPilot/Services/DevTunnelService.cs index 8a4a3f98..83cc383f 100644 --- a/PolyPilot/Services/DevTunnelService.cs +++ b/PolyPilot/Services/DevTunnelService.cs @@ -282,11 +282,7 @@ public async Task HostAsync(int copilotPort) private async Task TryHostTunnelAsync(ConnectionSettings settings) { // Kill any existing host process from a previous attempt - if (_hostProcess != null && !_hostProcess.HasExited) - { - try { _hostProcess.Kill(entireProcessTree: true); } catch { } - } - _hostProcess?.Dispose(); + ProcessHelper.SafeKillAndDispose(_hostProcess); _hostProcess = null; var hostArgs = _tunnelId != null @@ -310,6 +306,9 @@ private async Task TryHostTunnelAsync(ConnectionSettings settings) return false; } + // Capture in local variable — fire-and-forget tasks must not access _hostProcess + // field, which can be nulled/disposed by Stop() or a subsequent TryHostTunnelAsync(). + var process = _hostProcess; var urlFound = new TaskCompletionSource(); var lastErrorLine = ""; @@ -317,9 +316,9 @@ private async Task TryHostTunnelAsync(ConnectionSettings settings) { try { - while (!_hostProcess.HasExited) + while (!ProcessHelper.SafeHasExited(process)) { - var line = await _hostProcess.StandardOutput.ReadLineAsync(); + var line = await process.StandardOutput.ReadLineAsync(); if (line == null) break; Console.WriteLine($"[DevTunnel] {line}"); if (!string.IsNullOrWhiteSpace(line)) @@ -334,9 +333,9 @@ private async Task TryHostTunnelAsync(ConnectionSettings settings) { try { - while (!_hostProcess.HasExited) + while (!ProcessHelper.SafeHasExited(process)) { - var line = await _hostProcess.StandardError.ReadLineAsync(); + var line = await process.StandardError.ReadLineAsync(); if (line == null) break; Console.WriteLine($"[DevTunnel ERR] {line}"); if (!string.IsNullOrWhiteSpace(line)) @@ -462,12 +461,9 @@ public void Stop(bool cleanClose = true) _ = _auditLog?.LogSessionClosed(null, 0, cleanClose, cleanClose ? "DevTunnel stopped" : "DevTunnel stopped after error"); try { - if (_hostProcess != null && !_hostProcess.HasExited) - { - _hostProcess.Kill(entireProcessTree: true); + if (!ProcessHelper.SafeHasExited(_hostProcess)) Console.WriteLine("[DevTunnel] Host process killed"); - } - _hostProcess?.Dispose(); + ProcessHelper.SafeKillAndDispose(_hostProcess); } catch (Exception ex) { diff --git a/PolyPilot/Services/ProcessHelper.cs b/PolyPilot/Services/ProcessHelper.cs new file mode 100644 index 00000000..d215e23d --- /dev/null +++ b/PolyPilot/Services/ProcessHelper.cs @@ -0,0 +1,73 @@ +using System.Diagnostics; + +namespace PolyPilot.Services; + +/// +/// Safe wrappers for operations that can throw +/// when the process handle is +/// disposed or was never associated. +/// +public static class ProcessHelper +{ + /// + /// Returns true if the process has exited or the handle is invalid/disposed. + /// Unlike , this never throws. + /// A disposed or invalid process is treated as exited. + /// + public static bool SafeHasExited(Process? process) + { + if (process == null) + return true; + try + { + return process.HasExited; + } + catch (InvalidOperationException) + { + // "No process is associated with this object" — handle was disposed + return true; + } + catch (SystemException) + { + // Win32Exception, NotSupportedException, etc. + return true; + } + } + + /// + /// Attempts to kill the process tree. Swallows all exceptions — safe to call + /// on disposed or already-exited processes. + /// + public static void SafeKill(Process? process, bool entireProcessTree = true) + { + if (process == null) + return; + try + { + if (!process.HasExited) + process.Kill(entireProcessTree); + } + catch + { + // Process already exited, disposed, or access denied — nothing to do + } + } + + /// + /// Kills (if alive) and disposes the process. Safe to call multiple times. + /// + public static void SafeKillAndDispose(Process? process, bool entireProcessTree = true) + { + if (process == null) + return; + SafeKill(process, entireProcessTree); + try + { + process.Dispose(); + } + catch + { + // Already disposed — ignore + } + } +} diff --git a/PolyPilot/Services/ServerManager.cs b/PolyPilot/Services/ServerManager.cs index 2e4db8f5..cbebe83d 100644 --- a/PolyPilot/Services/ServerManager.cs +++ b/PolyPilot/Services/ServerManager.cs @@ -137,8 +137,7 @@ public void StopServer() try { var process = Process.GetProcessById(pid.Value); - process.Kill(); - process.Dispose(); + ProcessHelper.SafeKillAndDispose(process, entireProcessTree: false); Console.WriteLine($"[ServerManager] Killed server PID {pid}"); } catch (Exception ex)