Merge branch 'main' into fix-file-purger-kills-tray

Author: SimonCropp

claude.md

@@ -12,19 +12,23 @@ This file provides guidance to Claude Code (claude.ai/code) when working with co
 # Build (from repo root). Also packs: ProjectDefaults sets GeneratePackageOnBuild in Release.
 dotnet build src --configuration Release
 
-# Run all tests
-dotnet test --project src/DiffEngine.Tests --configuration Release
-dotnet test --project src/DiffEngineTray.Tests --configuration Release
-dotnet test --project src/DiffEngineViewer.Tests --configuration Release
+# Run all tests, which is what CI runs
+dotnet test --solution src/DiffEngine.slnx --configuration Release --no-build --no-restore
 
-# Run a single test project with filter
-dotnet test --project src/DiffEngine.Tests --configuration Release --filter "FullyQualifiedName~ClassName"
+# Run one test project
+dotnet test --project src/DiffEngine.Tests/DiffEngine.Tests.csproj --configuration Release --no-build --no-restore
 
-# Run a specific test
-dotnet test --project src/DiffEngine.Tests --configuration Release --filter "FullyQualifiedName=DiffEngine.Tests.ClassName.TestMethod"
+# Run one class, then one test
+dotnet test --project src/DiffEngine.Tests/DiffEngine.Tests.csproj --configuration Release --no-build --no-restore -- --treenode-filter "/*/*/ClassName/*"
+dotnet test --project src/DiffEngine.Tests/DiffEngine.Tests.csproj --configuration Release --no-build --no-restore -- --treenode-filter "/*/*/ClassName/MethodName"
+
+# Or run the test project directly, which is the fastest loop and takes the same filter
+src/DiffEngine.Tests/bin/Debug/net10.0/DiffEngine.Tests.exe --treenode-filter "/*/*/ClassName/*"
 ```
 
-**SDK Requirements:** .NET 10 SDK (see `src/global.json`). The project uses preview/prerelease SDK features.
+**Test runner:** TUnit runs on Microsoft.Testing.Platform rather than VSTest, which changes two things about the commands above. Filters are treenode paths given after `--`, as `/Assembly/Namespace/Class/Test` with `*` for any segment; VSTest's `--filter "FullyQualifiedName~ClassName"` matches nothing and exits 5, so a filtered run that reports no failures may have run no tests. And `--nologo` makes any run report "Zero tests ran" and exit 5, whatever else is on the command line, so leave it off.
+
+**SDK Requirements:** .NET 10 SDK (see `global.json`, at the repository root rather than under `src`). It also carries `"test": { "runner": "Microsoft.Testing.Platform" }`, which is what puts `dotnet test` on the runner described above. The project uses preview/prerelease SDK features.
 
 **Target Frameworks:**
 - DiffEngine library: net462, net472, net48, net6.0, net7.0, net8.0, net9.0, net10.0 (Windows also includes .NET Framework targets)

src/DiffEngine.Tests/GlobalUsings.cs

@@ -1,6 +1,8 @@
 global using EmptyFiles;
 global using System.Collections.Concurrent;
 global using System.Diagnostics;
+global using System.Net;
+global using System.Net.Sockets;
 global using System.Reflection;
 global using System.Text;
 global using Polyfills;

src/DiffEngine.Tests/InlineApplierTests.cs

@@ -205,9 +205,19 @@ public async Task ContentIsNeverObservedHalfWritten()
 
             using var cancellation = new CancelSource();
             var seen = new ConcurrentDictionary<long, byte>();
+            // Signalled from inside the delegate, so the apply cannot run and finish before the
+            // reader is looking. Without it a reader that starts late observes nothing and the
+            // assertion below passes on an empty set, which is a pass that checked nothing
+            var reading = new TaskCompletionSource<bool>(TaskCreationOptions.RunContinuationsAsynchronously);
+            // No token on Task.Run. It cancels the scheduling rather than the delegate, so a pool
+            // that had not yet picked this up when the cancel lands leaves the task Canceled and
+            // `await reader` throwing TaskCanceledException - which is what made this fail on CI,
+            // on all three OSes, while passing on any machine with a spare core. The loop already
+            // exits on the token, which is the only cancellation this ever wanted
             var reader = Task.Run(
                 () =>
                 {
+                    reading.SetResult(true);
                     while (!cancellation.IsCancellationRequested)
                     {
                         try
@@ -224,7 +234,9 @@ public async Task ContentIsNeverObservedHalfWritten()
                             // The swap is in flight. Not an observation of the content
                         }
                     }
-                }, cancellation.Token);
+                });
+
+            await reading.Task;
 
             // Long enough that the two whole files differ in length, which is what makes a
             // half written one tell itself apart
@@ -236,6 +248,10 @@ public async Task ContentIsNeverObservedHalfWritten()
             var after = new FileInfo(path).Length;
             await Assert.That(after).IsNotEqualTo(before);
 
+            // That the reader looked at all. Everything below is a statement about what it saw,
+            // and an empty set satisfies all of it
+            await Assert.That(seen.Keys).IsNotEmpty();
+
             var partial = seen.Keys.Where(_ => _ != before && _ != after).ToList();
             await Assert.That(partial).IsEmpty();
         }

src/DiffEngine.Tests/ViewerProtocolTests.cs

@@ -673,6 +673,48 @@ public async Task ASecondBindIsRefused()
         await Assert.That(second).IsNull();
     }
 
+    /// <summary>
+    /// An owner that accepts the connection and then says nothing. There used to be no bound on
+    /// this at all: SendTimeout and ReceiveTimeout apply only to synchronous calls, and the token
+    /// the async path was handed is the caller's, which is default from DiffRunner.AddInlineAsync.
+    /// A failing test waited for the owner for the rest of its life.
+    /// </summary>
+    [Test]
+    public async Task AnUnresponsiveOwnerTimesOutRatherThanHanging()
+    {
+        // Stop rather than Dispose: TcpListener is only IDisposable on the modern frameworks, and
+        // this test compiles for net48 too
+        var listener = new TcpListener(IPAddress.Loopback, 0);
+        listener.Start();
+        try
+        {
+            var port = ((IPEndPoint) listener.LocalEndpoint).Port;
+
+            // Accepted and then held, which is what a viewer inside the applier mutex looks like.
+            // Kept in scope so the connection is not collected and closed under the client
+            var accepted = listener.AcceptTcpClientAsync();
+
+            var watch = Stopwatch.StartNew();
+            var sent = await ViewerClient.TrySendAsync(
+                new(ViewerVerb.List),
+                default,
+                port,
+                TimeSpan.FromSeconds(1));
+            watch.Stop();
+
+            await Assert.That(sent).IsFalse();
+            await Assert.That(watch.Elapsed).IsLessThan(TimeSpan.FromSeconds(15));
+
+            if (accepted.Status == TaskStatus.RanToCompletion)
+            {
+                accepted.Result.Close();
+            }
+        }
+        finally
+        {
+            listener.Stop();
+        }
+    }
     [Test]
     public async Task AnAbsentOwnerIsNotAnError()
     {

src/DiffEngine/Protocol/ViewerClient.cs

@@ -36,6 +36,17 @@ public static int Port
 
     static readonly TimeSpan timeout = TimeSpan.FromSeconds(3);
 
+    /// <summary>
+    /// The deadline for the async exchange. Longer than the synchronous one because the owner
+    /// answers on its listener thread, so a connection can sit behind an accept that is itself
+    /// waiting up to ten seconds on <see cref="InlineApplier"/>'s cross process mutex. Shorter
+    /// than forever because there was no bound at all: SendTimeout and ReceiveTimeout apply only
+    /// to synchronous calls, and the token every async call was given is the caller's, which is
+    /// default from DiffRunner.AddInlineAsync - Verify passes none. An owner that accepted the
+    /// connection and then stopped answering hung the failing test for good.
+    /// </summary>
+    static readonly TimeSpan asyncTimeout = TimeSpan.FromSeconds(30);
+
     /// <summary>
     /// For callers on a clock or an interactive path, such as the tray's scan timer and its menu.
     /// The exchange is loopback to a local process, so anything slower than this is a wedged owner
@@ -95,40 +106,71 @@ public static bool TrySend(
     /// Fully async, including the read. A blocking read here would tie up a thread pool thread for
     /// the whole exchange, and a parallel test run calling this once per failing snapshot would
     /// starve the pool on a small machine.
+    /// <para>
+    /// <paramref name="port"/> and <paramref name="wait"/> override <see cref="Port"/> and
+    /// <see cref="asyncTimeout"/> for a single call, as they do on the synchronous overload. Tests
+    /// pass their own ephemeral port rather than mutating anything static, so they can run in
+    /// parallel.
+    /// </para>
     /// </summary>
-    public static async Task<bool> TrySendAsync(ViewerMessage message, Cancel cancel)
+    public static async Task<bool> TrySendAsync(
+        ViewerMessage message,
+        Cancel cancel,
+        int? port = null,
+        TimeSpan? wait = null)
     {
+        var endpointPort = port ?? Port;
+        var timeToWait = wait ?? asyncTimeout;
+        using var deadline = CancelSource.CreateLinkedTokenSource(cancel);
+        deadline.CancelAfter(timeToWait);
+        var token = deadline.Token;
         try
         {
             using var client = new TcpClient();
+            // Closing the socket is the only thing that unblocks every framework: the pre-net7
+            // ReadToEndAsync takes no token at all, and net462 has no cancellable connect or
+            // write either. Registered after the client and so disposed before it, which is what
+            // stops the callback firing on a disposed object
+            using var abort = token.Register(() => Abort(client));
 #if NET6_0_OR_GREATER
-            await client.ConnectAsync(IPAddress.Loopback, Port, cancel);
+            await client.ConnectAsync(IPAddress.Loopback, endpointPort, token);
 #else
-            cancel.ThrowIfCancellationRequested();
-            using (cancel.Register(client.Close))
-            {
-                await client.ConnectAsync(IPAddress.Loopback, Port);
-            }
+            token.ThrowIfCancellationRequested();
+            await client.ConnectAsync(IPAddress.Loopback, endpointPort);
 #endif
-            Configure(client, timeout);
+            Configure(client, timeToWait);
             var stream = client.GetStream();
             var bytes = Encoding.UTF8.GetBytes(message.Build());
 #if NET6_0_OR_GREATER
-            await stream.WriteAsync(bytes, cancel);
+            await stream.WriteAsync(bytes, token);
 #else
-            await stream.WriteAsync(bytes, 0, bytes.Length, cancel);
+            await stream.WriteAsync(bytes, 0, bytes.Length, token);
 #endif
-            await stream.FlushAsync(cancel);
+            await stream.FlushAsync(token);
             HalfClose(client);
             using var reader = new StreamReader(stream, Encoding.UTF8);
 #if NET7_0_OR_GREATER
-            var text = await reader.ReadToEndAsync(cancel);
+            var text = await reader.ReadToEndAsync(token);
 #else
             var text = await reader.ReadToEndAsync();
 #endif
             return ViewerResponse.TryParse(text, out var response) &&
                    response.Ok;
         }
+        // The deadline, rather than the caller cancelling. Whatever the abort surfaced as - a
+        // cancellation, a closed socket, a torn down stream - the owner is present but not
+        // answering. Reported as absence because that is the recoverable answer: the caller
+        // launches a viewer or stages the patch, rather than waiting on a process that has
+        // stopped listening. Logged so the two are still tellable apart afterwards
+        catch (Exception exception)
+            when (!cancel.IsCancellationRequested && token.IsCancellationRequested)
+        {
+            // Trace rather than Logging, because this file is linked into the viewer too
+            Trace.WriteLine(
+                $"Timed out after {timeToWait} waiting for the inline queue owner on port {endpointPort}. " +
+                $"Verb: {message.Verb}. The owner is present but unresponsive. {exception.GetType().Name}");
+            return false;
+        }
         // Cancellation is the caller's business; a missing owner is not.
         catch (Exception exception)
             when (exception is not OperationCanceledException && Ignorable(exception))
@@ -137,6 +179,21 @@ public static async Task<bool> TrySendAsync(ViewerMessage message, Cancel cancel
         }
     }
 
+    /// <summary>
+    /// Unblocks whatever the exchange is waiting on. Swallowing here rather than letting it out:
+    /// this runs on the timer that fired the deadline, where a throw has nowhere to go.
+    /// </summary>
+    static void Abort(TcpClient client)
+    {
+        try
+        {
+            client.Close();
+        }
+        catch (Exception exception)
+            when (Ignorable(exception))
+        {
+        }
+    }
     static void Configure(TcpClient client, TimeSpan wait)
     {
         client.SendTimeout = (int) wait.TotalMilliseconds;