From ffc1923532f36a8368af4116ca1b899ae5bb7029 Mon Sep 17 00:00:00 2001 From: Simon Cropp Date: Sat, 22 Aug 2026 10:21:23 +1000 Subject: [PATCH] Fall through when the tray has gone PendingFiles routes on DiffEngineTray.IsRunning, which a type initialiser reads once. CLAUDE.md documents the late-starting tray - a tray that appears after the test host, which the queue owner branch already handles. This is the mirror case, and nothing handled it. A tray that exits while a long lived host keeps running leaves that cached answer saying a tray is there. The piper send then went to a port nobody was listening on, and returned void: the refusal was swallowed into a trace line and the caller carried on believing it had handed the file over. Every later move and delete for the life of that process was pending in nothing at all - no fallback to the queue owner, and no LaunchDelete either. Have the piper send report whether it connected, and treat a refusal the same as no tray. That is the branch that already exists and already knows what to do. --- .../PendingFilesFallbackTests.cs | 83 +++++++++++++++++++ src/DiffEngine/Tray/PendingFiles.cs | 23 +++-- src/DiffEngine/Tray/PiperClient.cs | 21 +++-- 3 files changed, 113 insertions(+), 14 deletions(-) create mode 100644 src/DiffEngine.Tests/PendingFilesFallbackTests.cs diff --git a/src/DiffEngine.Tests/PendingFilesFallbackTests.cs b/src/DiffEngine.Tests/PendingFilesFallbackTests.cs new file mode 100644 index 00000000..79c7b77e --- /dev/null +++ b/src/DiffEngine.Tests/PendingFilesFallbackTests.cs @@ -0,0 +1,83 @@ +// DiffEngineTray is the obsolete public shim, but its IsRunning is still where the tray check +// lives, and this test has to move it. +#pragma warning disable CS0618 + +/// +/// Where a pending file goes when the tray check is stale. +/// +/// DiffEngineTray.IsRunning is read once, when the type initialises. A tray that exits while a +/// long lived host keeps running leaves that answer saying a tray is there, so the piper send went +/// to a port nobody was listening on - and, returning nothing, was swallowed into a trace line. +/// The move or delete was then pending in nothing at all: no fallback to the queue owner, no +/// LaunchDelete. +/// +/// +[NotInParallel] +public class PendingFilesFallbackTests +{ + [Test] + public async Task ADeadPiperFallsThroughToTheQueueOwner() + { + await Assert.That(ViewerServer.TryBind(0, out var bound)).IsTrue(); + using var server = bound!; + using var cancel = new CancelSource(); + + var heard = new ConcurrentBag(); + var listening = server.Listen( + _ => + { + heard.Add($"{_.Verb}:{_.Key}"); + return ViewerResponse.Success(); + }, + cancel.Token); + + var previousPort = PiperClient.Port; + var previousViewerPort = Environment.GetEnvironmentVariable(ViewerClient.PortVariable); + var previousRunning = DiffEngineTray.IsRunning; + try + { + // A tray that says it is running, on a port nothing is listening on + PiperClient.Port = DeadPort(); + DiffEngineTray.IsRunning = true; + Environment.SetEnvironmentVariable(ViewerClient.PortVariable, server.Port.ToString()); + + PendingFiles.AddMove("temp.txt", "target.txt", null, null, false, null); + await PendingFilesAddDelete("gone.txt"); + + await Assert.That(heard).Contains(_ => _.StartsWith("Move:", StringComparison.Ordinal)); + await Assert.That(heard).Contains(_ => _.StartsWith("Delete:", StringComparison.Ordinal)); + } + finally + { + PiperClient.Port = previousPort; + DiffEngineTray.IsRunning = previousRunning; + Environment.SetEnvironmentVariable(ViewerClient.PortVariable, previousViewerPort); + await cancel.CancelAsync(); + try + { + await listening.WaitAsync(TimeSpan.FromSeconds(5)); + } + catch (Exception exception) + when (exception is OperationCanceledException or TimeoutException) + { + } + } + } + + // The delete path launches a viewer when nothing answers, so it is only safe to exercise with + // an owner bound - which is the point of the test. + static Task PendingFilesAddDelete(string file) + { + PendingFiles.AddDelete(file); + return Task.CompletedTask; + } + + static int DeadPort() + { + var listener = new TcpListener(IPAddress.Loopback, 0); + listener.Start(); + var port = ((IPEndPoint) listener.LocalEndpoint).Port; + listener.Stop(); + return port; + } +} diff --git a/src/DiffEngine/Tray/PendingFiles.cs b/src/DiffEngine/Tray/PendingFiles.cs index ba936813..3272c00c 100644 --- a/src/DiffEngine/Tray/PendingFiles.cs +++ b/src/DiffEngine/Tray/PendingFiles.cs @@ -24,14 +24,21 @@ namespace DiffEngine; /// process's life, and its moves and deletes arrive here instead — which a tray that owns the /// queue answers, so they end up tracked either way. /// +/// +/// The mirror of that case is a tray that exits while a long lived host keeps running, and it is +/// why the piper send is asked whether it connected rather than told to get on with it. The cached +/// answer still says a tray is there, so every later move and delete went to a port nobody was +/// listening on and was swallowed into a trace line: pending in nothing, with no fallback and no +/// LaunchDelete. A refused piper send now falls through to the same branch as no tray at all. +/// /// static class PendingFiles { public static void AddDelete(string file) { - if (DiffEngineTray.IsRunning) + if (DiffEngineTray.IsRunning && + PiperClient.SendDelete(file)) { - PiperClient.SendDelete(file); return; } @@ -45,9 +52,9 @@ public static void AddDelete(string file) public static async Task AddDeleteAsync(string file, Cancel cancel) { - if (DiffEngineTray.IsRunning) + if (DiffEngineTray.IsRunning && + await PiperClient.SendDeleteAsync(file, cancel)) { - await PiperClient.SendDeleteAsync(file, cancel); return; } @@ -67,9 +74,9 @@ public static void AddMove( bool canKill, int? processId) { - if (DiffEngineTray.IsRunning) + if (DiffEngineTray.IsRunning && + PiperClient.SendMove(tempFile, targetFile, exe, arguments, canKill, processId)) { - PiperClient.SendMove(tempFile, targetFile, exe, arguments, canKill, processId); return; } @@ -85,9 +92,9 @@ public static async Task AddMoveAsync( int? processId, Cancel cancel) { - if (DiffEngineTray.IsRunning) + if (DiffEngineTray.IsRunning && + await PiperClient.SendMoveAsync(tempFile, targetFile, exe, arguments, canKill, processId, cancel)) { - await PiperClient.SendMoveAsync(tempFile, targetFile, exe, arguments, canKill, processId, cancel); return; } diff --git a/src/DiffEngine/Tray/PiperClient.cs b/src/DiffEngine/Tray/PiperClient.cs index 5f52320a..0ce08d00 100644 --- a/src/DiffEngine/Tray/PiperClient.cs +++ b/src/DiffEngine/Tray/PiperClient.cs @@ -2,10 +2,10 @@ static class PiperClient { public static int Port = 3492; - public static void SendDelete(string file) => + public static bool SendDelete(string file) => Send(BuildDeletePayload(file)); - public static Task SendDeleteAsync( + public static Task SendDeleteAsync( string file, Cancel cancel = default) { @@ -22,7 +22,7 @@ static string BuildDeletePayload(string file) => """; - public static void SendMove( + public static bool SendMove( string tempFile, string targetFile, string? exe, @@ -31,7 +31,7 @@ public static void SendMove( int? processId) => Send(BuildMovePayload(tempFile, targetFile, exe, arguments, canKill, processId)); - public static Task SendMoveAsync( + public static Task SendMoveAsync( string tempFile, string targetFile, string? exe, @@ -79,28 +79,37 @@ public static string BuildMovePayload(string tempFile, string targetFile, string return builder.ToString(); } - static void Send(string payload) + /// + /// True when the tray took it. False is not fatal on its own - the payload is traced either + /// way - but it is what lets the caller send the pending file somewhere else instead of + /// dropping it, which is what happened when this returned nothing. + /// + static bool Send(string payload) { try { InnerSend(payload); + return true; } catch (Exception exception) { HandleSendException(payload, exception); + return false; } } - static async Task SendAsync(string payload, Cancel cancel) + static async Task SendAsync(string payload, Cancel cancel) { try { await InnerSendAsync(payload, cancel); + return true; } // Let cancellation surface to the caller; only genuine send failures are swallowed. catch (Exception exception) when (exception is not OperationCanceledException) { HandleSendException(payload, exception); + return false; } }