Fix launch canKill inversion and tray bugs

src/DiffEngine/DiffRunner.cs

@@ -156,11 +156,12 @@ static LaunchResult InnerLaunch(TryResolveTool tryResolveTool, string tempFile,
 
         tool.CommandAndArguments(tempFile, targetFile, out var arguments, out var command);
 
+        var canKill = !tool.IsMdi;
         if (ProcessCleanup.TryGetProcessInfo(command, out var processCommand))
         {
             if (tool.AutoRefresh)
             {
-                DiffEngineTray.AddMove(tempFile, targetFile, tool.ExePath, arguments, tool.IsMdi, processCommand.Process);
+                DiffEngineTray.AddMove(tempFile, targetFile, tool.ExePath, arguments, canKill, processCommand.Process);
                 return LaunchResult.AlreadyRunningAndSupportsRefresh;
             }
 
@@ -169,13 +170,13 @@ static LaunchResult InnerLaunch(TryResolveTool tryResolveTool, string tempFile,
 
         if (MaxInstance.Reached())
         {
-            DiffEngineTray.AddMove(tempFile, targetFile, tool.ExePath, arguments, tool.IsMdi, null);
+            DiffEngineTray.AddMove(tempFile, targetFile, tool.ExePath, arguments, canKill, null);
             return LaunchResult.TooManyRunningDiffTools;
         }
 
         var processId = LaunchProcess(tool, arguments);
 
-        DiffEngineTray.AddMove(tempFile, targetFile, tool.ExePath, arguments, !tool.IsMdi, processId);
+        DiffEngineTray.AddMove(tempFile, targetFile, tool.ExePath, arguments, canKill, processId);
 
         return LaunchResult.StartedNewInstance;
     }

src/DiffEngine/Tray/DiffEngineTray.cs

@@ -19,7 +19,7 @@ static DiffEngineTray()
         }
     }
 
-    public static bool IsRunning { get; }
+    public static bool IsRunning { get; internal set; }
 
     public static void AddDelete(string file)
     {

src/DiffEngine/Tray/PiperClient.cs

@@ -10,7 +10,7 @@ public static Task SendDeleteAsync(
         Cancel cancel = default)
     {
         var payload = BuildDeletePayload(file);
-        return SendAsync(payload);
+        return SendAsync(payload, cancel);
     }
 
     static string BuildDeletePayload(string file) =>
@@ -41,7 +41,7 @@ public static Task SendMoveAsync(
         Cancel cancel = default)
     {
         var payload = BuildMovePayload(tempFile, targetFile, exe, arguments, canKill, processId);
-        return SendAsync(payload);
+        return SendAsync(payload, cancel);
     }
 
     public static string BuildMovePayload(string tempFile, string targetFile, string? exe, string? arguments, bool canKill, int? processId)
@@ -91,13 +91,14 @@ static void Send(string payload)
         }
     }
 
-    static async Task SendAsync(string payload)
+    static async Task SendAsync(string payload, Cancel cancel)
     {
         try
         {
-            await InnerSendAsync(payload);
+            await InnerSendAsync(payload, cancel);
         }
-        catch (Exception exception)
+        // Let cancellation surface to the caller; only genuine send failures are swallowed.
+        catch (Exception exception) when (exception is not OperationCanceledException)
         {
             HandleSendException(payload, exception);
         }
@@ -132,16 +133,28 @@ static void InnerSend(string payload)
         }
     }
 
-    static async Task InnerSendAsync(string payload)
+    static async Task InnerSendAsync(string payload, Cancel cancel)
     {
         using var client = new TcpClient();
         var endpoint = GetEndpoint();
         try
         {
-            await client.ConnectAsync(endpoint.Address, endpoint.Port);
+#if NET6_0_OR_GREATER
+            await client.ConnectAsync(endpoint.Address, endpoint.Port, cancel);
             using var stream = client.GetStream();
             using var writer = new StreamWriter(stream);
-            await writer.WriteAsync(payload);
+            await writer.WriteAsync(payload.AsMemory(), cancel);
+#else
+            cancel.ThrowIfCancellationRequested();
+            // Older frameworks lack cancellable Connect/Write, so abort by closing the client.
+            using (cancel.Register(client.Close))
+            {
+                await client.ConnectAsync(endpoint.Address, endpoint.Port);
+                using var stream = client.GetStream();
+                using var writer = new StreamWriter(stream);
+                await writer.WriteAsync(payload);
+            }
+#endif
         }
         finally
         {

src/DiffEngineTray.Tests/DiffRunnerCanKillTest.cs

@@ -0,0 +1,118 @@
+using System.Net;
+using System.Net.Sockets;
+using DiffEngine;
+
+#pragma warning disable CS0618 // DiffEngineTray is obsolete; the test drives it directly to enable the send path.
+
+// Regression test for the sync launch path sending the wrong `canKill` value.
+// MDI tools host every diff in one shared window, so the tray must never kill them (CanKill=false);
+// non-MDI tools get their own process, which the tray may kill (CanKill=true).
+public class DiffRunnerCanKillTest :
+    IDisposable
+{
+    string tempFile = Path.GetTempFileName();
+    bool originalDisabled = DiffRunner.Disabled;
+    string? originalMaxInstances = Environment.GetEnvironmentVariable("DiffEngine_MaxInstances");
+
+    public DiffRunnerCanKillTest()
+    {
+        PiperClient.Port = GetFreePort();
+        DiffEngine.DiffEngineTray.IsRunning = true;
+        DiffRunner.Disabled = false;
+        // Force the "too many running" branch so no real process is launched, while a move
+        // payload is still sent to the tray. The env var takes precedence over the app-domain
+        // value, so set it too; MaxInstancesToLaunch resets the cached lookup.
+        Environment.SetEnvironmentVariable("DiffEngine_MaxInstances", "0");
+        DiffRunner.MaxInstancesToLaunch(0);
+    }
+
+    [Test]
+    public async Task Sync_launch_of_mdi_tool_marks_move_as_not_killable()
+    {
+        var received = await CaptureMove(() => Task.FromResult(DiffRunner.Launch(MdiTool(), tempFile, "target.txt")));
+
+        await Assert.That(received.CanKill).IsFalse();
+    }
+
+    [Test]
+    public async Task Async_launch_of_mdi_tool_marks_move_as_not_killable()
+    {
+        var received = await CaptureMove(() => DiffRunner.LaunchAsync(MdiTool(), tempFile, "target.txt"));
+
+        await Assert.That(received.CanKill).IsFalse();
+    }
+
+    [Test]
+    public async Task Sync_launch_of_non_mdi_tool_marks_move_as_killable()
+    {
+        var received = await CaptureMove(() => Task.FromResult(DiffRunner.Launch(NonMdiTool(), tempFile, "target.txt")));
+
+        await Assert.That(received.CanKill).IsTrue();
+    }
+
+    static async Task<MovePayload> CaptureMove(Func<Task<LaunchResult>> launch)
+    {
+        MovePayload? received = null;
+        var source = new CancelSource();
+        var server = PiperServer.Start(move => received = move, _ => { }, source.Token);
+        try
+        {
+            var result = await launch();
+            await Assert.That(result).IsEqualTo(LaunchResult.TooManyRunningDiffTools);
+
+            for (var i = 0; received == null && i < 50; i++)
+            {
+                await Task.Delay(100);
+            }
+        }
+        finally
+        {
+            await source.CancelAsync();
+            await server;
+        }
+
+        await Assert.That(received).IsNotNull();
+        return received!;
+    }
+
+    static ResolvedTool MdiTool() => Tool(isMdi: true);
+
+    static ResolvedTool NonMdiTool() => Tool(isMdi: false);
+
+    static ResolvedTool Tool(bool isMdi) =>
+        new(
+            name: "FakeCanKillTool",
+            exePath: Environment.ProcessPath!,
+            launchArguments: new(
+                Left: (temp, target) => $"\"{temp}\" \"{target}\"",
+                Right: (temp, target) => $"\"{target}\" \"{temp}\""),
+            isMdi: isMdi,
+            autoRefresh: false,
+            binaryExtensions: [],
+            requiresTarget: false,
+            supportsText: true,
+            useShellExecute: false);
+
+    static int GetFreePort()
+    {
+        var probe = new TcpListener(IPAddress.Loopback, 0);
+        probe.Start();
+        try
+        {
+            return ((IPEndPoint) probe.LocalEndpoint).Port;
+        }
+        finally
+        {
+            probe.Stop();
+        }
+    }
+
+    public void Dispose()
+    {
+        DiffEngine.DiffEngineTray.IsRunning = false;
+        DiffRunner.Disabled = originalDisabled;
+        Environment.SetEnvironmentVariable("DiffEngine_MaxInstances", originalMaxInstances);
+        DiffRunner.MaxInstancesToLaunch(5);
+        File.Delete(tempFile);
+    }
+}

src/DiffEngineTray.Tests/FileExTest.cs

@@ -0,0 +1,31 @@
+public class FileExTest
+{
+    [Test]
+    public async Task SafeDeleteDirectoryRemovesDirectoryWithOnlyEmptySubDirectories()
+    {
+        var root = Path.Combine(Path.GetTempPath(), "DiffEngineSafeDelete", Guid.NewGuid().ToString("N"));
+        Directory.CreateDirectory(Path.Combine(root, "sub", "nested"));
+
+        FileEx.SafeDeleteDirectory(root);
+
+        await Assert.That(Directory.Exists(root)).IsFalse();
+    }
+
+    [Test]
+    public async Task SafeDeleteDirectoryKeepsDirectoryThatContainsAFile()
+    {
+        var root = Path.Combine(Path.GetTempPath(), "DiffEngineSafeDelete", Guid.NewGuid().ToString("N"));
+        Directory.CreateDirectory(root);
+        await File.WriteAllTextAsync(Path.Combine(root, "keep.txt"), "data");
+        try
+        {
+            FileEx.SafeDeleteDirectory(root);
+
+            await Assert.That(Directory.Exists(root)).IsTrue();
+        }
+        finally
+        {
+            Directory.Delete(root, true);
+        }
+    }
+}

src/DiffEngineTray.Tests/PiperTest.SendOnly.verified.txt

@@ -14,9 +14,7 @@ Exception:
 System.Net.Sockets.SocketException (10061): No connection could be made because the target machine actively refused it.
    at System.Net.Sockets.Socket.AwaitableSocketAsyncEventArgs.ThrowException(SocketError error, CancellationToken cancellationToken)
    at System.Net.Sockets.Socket.AwaitableSocketAsyncEventArgs.System.Threading.Tasks.Sources.IValueTaskSource.GetResult(Int16 token)
-   at System.Threading.Tasks.ValueTask.ValueTaskSourceAsTask.<>c.<.cctor>b__4_0(Object state)
---- End of stack trace from previous location ---
-   at System.Net.Sockets.TcpClient.CompleteConnectAsync(Task task),
+   at System.Net.Sockets.TcpClient.CompleteConnectAsync(ValueTask task),
   Failed to send payload to DiffEngineTray.
 
 Payload:
@@ -29,7 +27,5 @@ Exception:
 System.Net.Sockets.SocketException (10061): No connection could be made because the target machine actively refused it.
    at System.Net.Sockets.Socket.AwaitableSocketAsyncEventArgs.ThrowException(SocketError error, CancellationToken cancellationToken)
    at System.Net.Sockets.Socket.AwaitableSocketAsyncEventArgs.System.Threading.Tasks.Sources.IValueTaskSource.GetResult(Int16 token)
-   at System.Threading.Tasks.ValueTask.ValueTaskSourceAsTask.<>c.<.cctor>b__4_0(Object state)
---- End of stack trace from previous location ---
-   at System.Net.Sockets.TcpClient.CompleteConnectAsync(Task task)
+   at System.Net.Sockets.TcpClient.CompleteConnectAsync(ValueTask task)
 ]

src/DiffEngineTray.Tests/PiperTest.cs

@@ -84,6 +84,25 @@ public async Task Move()
         await Verify(received);
     }
 
+    [Test]
+    public async Task SendMoveAsyncHonorsCancellation()
+    {
+        using var source = new CancelSource();
+        source.Cancel();
+
+        var cancelled = false;
+        try
+        {
+            await PiperClient.SendMoveAsync("Foo", "Bar", "theExe", "TheArguments", true, 10, source.Token);
+        }
+        catch (OperationCanceledException)
+        {
+            cancelled = true;
+        }
+
+        await Assert.That(cancelled).IsTrue();
+    }
+
     [Test]
     public async Task ClientDisconnectsAbruptly()
     {

src/DiffEngineTray.Tests/ProcessExTest.cs

@@ -14,4 +14,16 @@ public async Task TryGetMissing()
         await Assert.That(ProcessEx.TryGet(40000, out var found)).IsFalse();
         await Assert.That(found).IsNull();
     }
+
+    [Test]
+    public async Task DescribeIsSafeForDisposedProcess()
+    {
+        var process = Process.GetCurrentProcess();
+        process.Dispose();
+
+        // Id/MainModule can throw on a disposed process; Describe must swallow that.
+        var description = ProcessEx.Describe(process);
+
+        await Assert.That(description).IsNotNull();
+    }
 }

src/DiffEngineTray.Tests/ProgramKeyBindingsTest.cs

@@ -0,0 +1,40 @@
+public class ProgramKeyBindingsTest
+{
+    [Test]
+    public async Task Each_configured_hotkey_uses_a_distinct_binding_id()
+    {
+        var settings = new Settings
+        {
+            DiscardAllHotKey = new() { Key = "A" },
+            AcceptAllHotKey = new() { Key = "B" },
+            AcceptOpenHotKey = new() { Key = "C" },
+        };
+        await using var tracker = new RecordingTracker();
+
+        var ids = Program.BuildKeyBindings(settings, tracker)
+            .Select(_ => _.Id)
+            .ToList();
+
+        await Assert.That(ids.Count).IsEqualTo(3);
+        // A duplicate id would make KeyRegister unregister and overwrite an earlier hot key.
+        await Assert.That(ids.Distinct().Count()).IsEqualTo(3);
+        await Assert.That(ids.Contains(KeyBindingIds.DiscardAll)).IsTrue();
+        await Assert.That(ids.Contains(KeyBindingIds.AcceptAll)).IsTrue();
+        await Assert.That(ids.Contains(KeyBindingIds.AcceptOpen)).IsTrue();
+    }
+
+    [Test]
+    public async Task AcceptOpen_hotkey_maps_to_the_AcceptOpen_binding()
+    {
+        var settings = new Settings
+        {
+            AcceptOpenHotKey = new() { Key = "C" },
+        };
+        await using var tracker = new RecordingTracker();
+
+        var binding = Program.BuildKeyBindings(settings, tracker)
+            .Single();
+
+        await Assert.That(binding.Id).IsEqualTo(KeyBindingIds.AcceptOpen);
+    }
+}