diff --git a/src/DiffEngineTray.Tests/ProcessExHandleTests.cs b/src/DiffEngineTray.Tests/ProcessExHandleTests.cs new file mode 100644 index 00000000..89e5f65f --- /dev/null +++ b/src/DiffEngineTray.Tests/ProcessExHandleTests.cs @@ -0,0 +1,53 @@ +/// +/// TryGet holds an OS handle on the process it hands back. +/// +/// Process.GetProcessById holds none of its own, so Kill, HasExited and MainWindowHandle each +/// re-open the id at the moment they are called. A tracked move outlives its diff tool by design - +/// HandleScanMove keeps it while the temp file is still there - so hours later that id may belong +/// to something else, and "Accept all" or "Open diff tool" would act on whatever it is. +/// +/// +/// Asserted through the exit code, which is only readable if a handle was open before the process +/// ended. That is the same thing as the id being held, since Windows will not hand out an id while +/// a handle to it exists. +/// +/// +[NotInParallel] +public class ProcessExHandleTests +{ + [Test] + public async Task TheProcessIsStillReadableAfterItExits() + { + // Any long lived process will do; this project has no fake tool of its own + var started = FileLockUtils.StartFileLockProcess(Path.GetTempFileName()); + + try + { + await Assert.That(ProcessEx.TryGet(started.Id, out var tracked)).IsTrue(); + + started.Kill(); + await Assert.That(started.WaitForExit(30000)).IsTrue(); + + // Readable only because TryGet opened a handle while it was still running + await Assert.That(tracked!.HasExited).IsTrue(); + await Assert.That(tracked.ExitCode).IsNotEqualTo(int.MinValue); + tracked.Dispose(); + } + finally + { + try + { + if (!started.HasExited) + { + started.Kill(); + } + } + catch + { + // Already gone + } + + started.Dispose(); + } + } +} diff --git a/src/DiffEngineTray/ProcessEx.cs b/src/DiffEngineTray/ProcessEx.cs index 454d0304..6ca118e2 100644 --- a/src/DiffEngineTray/ProcessEx.cs +++ b/src/DiffEngineTray/ProcessEx.cs @@ -16,9 +16,19 @@ public static bool TryGet(int id, [NotNullWhen(true)] out Process? process) } } + Process? opened = null; try { - process = Process.GetProcessById(id); + opened = Process.GetProcessById(id); + process = opened; + // Forces the OS handle open, and keeps it. GetProcessById holds none of its own, so + // Kill, HasExited and MainWindowHandle each re-open the id at the moment they are + // called - and a tracked move outlives its diff tool by design, since HandleScanMove + // keeps it while the temp file is still there. Hours later that id may belong to + // something else, and "Accept all" or "Open diff tool" would kill whatever it is. + // An open handle also stops Windows handing the id out again while this move is + // tracked, so there is nothing to confuse it with + _ = process.Handle; return true; } catch (ArgumentException) @@ -27,6 +37,17 @@ public static bool TryGet(int id, [NotNullWhen(true)] out Process? process) process = null; return false; } + catch (Exception exception) + when (exception is Win32Exception or InvalidOperationException) + { + // The handle could not be held - it exited between the probe above and here, or this + // account cannot open it. Without one there is no way to tell the process apart from a + // later holder of the same id, so it is better tracked as no process at all: the tool + // is then not killed, rather than something else being killed in its place + opened?.Dispose(); + process = null; + return false; + } } public static void KillAndDispose(this Process process)