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; } }