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.

Author: SimonCropp

src/DiffEngineTray.Tests/ProcessExHandleTests.cs

@@ -0,0 +1,53 @@
+/// <summary>
+/// TryGet holds an OS handle on the process it hands back.
+/// <para>
+/// 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.
+/// </para>
+/// <para>
+/// 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.
+/// </para>
+/// </summary>
+[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();
+        }
+    }
+}

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)