From 8ee659d4973f3046dbbc14635e24c523d2e4fd8c Mon Sep 17 00:00:00 2001 From: Simon Cropp Date: Sat, 22 Aug 2026 14:44:32 +1000 Subject: [PATCH] Hold a handle on a tracked process Process.GetProcessById holds no OS handle, 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 entirely, and "Accept all" or "Open diff tool" would act on whatever that is. Touching Handle at track time opens one and keeps it, and Windows will not hand out an id while a handle to it exists, so there is nothing left to confuse the tracked process with. When the handle cannot be held - it exited between the probe and here, or this account cannot open it - TryGet now reports no process rather than one it cannot identify. The tool then goes unkilled, which is better than something else being killed in its place. The test asserts through the exit code, which is readable only if a handle was open before the process ended. That is the same fact as the id being held. --- .../ProcessExHandleTests.cs | 53 +++++++++++++++++++ src/DiffEngineTray/ProcessEx.cs | 23 +++++++- 2 files changed, 75 insertions(+), 1 deletion(-) create mode 100644 src/DiffEngineTray.Tests/ProcessExHandleTests.cs 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)