Open the tray menu without a socket round trip

src/DiffEngineTray.Tests/TrayViewerSyncTest.cs

@@ -79,7 +79,7 @@ public async Task TrayAcceptOfOneSnapshotLeavesTheRestInTheAttachedViewer()
         await pair.Tracker.Accept(snapshot);
 
         await Assert.That(pair.Pump().Keys()).IsEquivalentTo([Key(other, 7)]);
-        await Assert.That(pair.Tracker.Snapshots.Select(_ => _.Key)).IsEquivalentTo([Key(other, 7)]);
+        await Assert.That(pair.Listing.Select(_ => _.Key)).IsEquivalentTo([Key(other, 7)]);
     }
 
     [Test]
@@ -160,7 +160,7 @@ public async Task ASettleReachesTheAttachedViewer()
         pair.Send(new(ViewerVerb.Settle, Key(sample, 1)));
 
         await Assert.That(pair.Pump().Keys()).IsEquivalentTo([Key(other, 7)]);
-        await Assert.That(pair.Tracker.Snapshots.Select(_ => _.Key)).IsEquivalentTo([Key(other, 7)]);
+        await Assert.That(pair.Listing.Select(_ => _.Key)).IsEquivalentTo([Key(other, 7)]);
     }
 
     /// <summary>
@@ -181,7 +181,7 @@ public async Task ViewerAcceptAllEmptiesTheTray()
 
         var viewer = pair.Pump();
         await Assert.That(viewer.Queue).IsEmpty();
-        await Assert.That(pair.Tracker.Snapshots).IsEmpty();
+        await Assert.That(pair.Listing).IsEmpty();
         await Assert.That(pair.Tracker.Moves).IsEmpty();
         await Assert.That(pair.Tracker.Deletes).IsEmpty();
         await Assert.That(await File.ReadAllTextAsync(move.Target)).IsEqualTo("received");
@@ -199,7 +199,7 @@ public async Task ViewerAcceptOfOneSnapshotReachesTheTray()
         pair.Link.Post(ViewerSideVerb.Accept, Key(sample, 1));
 
         await Assert.That(pair.Pump().Keys()).IsEquivalentTo([Key(other, 7)]);
-        await Assert.That(pair.Tracker.Snapshots.Select(_ => _.Key)).IsEquivalentTo([Key(other, 7)]);
+        await Assert.That(pair.Listing.Select(_ => _.Key)).IsEquivalentTo([Key(other, 7)]);
         await Assert.That(pair.Applied.Select(_ => _.LineHint)).IsEquivalentTo([1]);
     }
 
@@ -214,7 +214,7 @@ public async Task ViewerDiscardOfOneSnapshotReachesTheTray()
         pair.Link.Post(ViewerSideVerb.Discard, Key(sample, 1));
 
         await Assert.That(pair.Pump().Keys()).IsEquivalentTo([Key(other, 7)]);
-        await Assert.That(pair.Tracker.Snapshots.Select(_ => _.Key)).IsEquivalentTo([Key(other, 7)]);
+        await Assert.That(pair.Listing.Select(_ => _.Key)).IsEquivalentTo([Key(other, 7)]);
         await Assert.That(pair.Applied).IsEmpty();
     }
 
@@ -270,7 +270,7 @@ public async Task AcceptingASnapshotThatIsAlreadyGoneSaysNothing()
 
         await Assert.That(pair.Failures).IsEmpty();
         await Assert.That(pair.Applied).IsEmpty();
-        await Assert.That(pair.Tracker.Snapshots.Select(_ => _.Key)).IsEquivalentTo([Key(other, 7)]);
+        await Assert.That(pair.Listing.Select(_ => _.Key)).IsEquivalentTo([Key(other, 7)]);
     }
 
     /// <summary>
@@ -289,7 +289,7 @@ public async Task AFailedAcceptStaysPendingOnBothSides()
         var viewer = pair.Pump();
         await Assert.That(viewer.Queue.Single().Status).IsEqualTo("the file is locked");
         await Assert.That(viewer.Exit).IsFalse();
-        await Assert.That(pair.Tracker.Snapshots.Single().Status).IsEqualTo("the file is locked");
+        await Assert.That(pair.Listing.Single().Status).IsEqualTo("the file is locked");
         await Assert.That(pair.Failures.Single()).Contains("the file is locked");
     }
 
@@ -314,7 +314,7 @@ public async Task TrayAcceptAllReportsWhatStayedPending()
         var viewer = pair.Pump();
         await Assert.That(viewer.Keys()).IsEquivalentTo([Key(sample, 1)]);
         await Assert.That(viewer.Queue.Single().Status).IsEqualTo("the file is locked");
-        await Assert.That(pair.Tracker.Snapshots.Single().Status).IsEqualTo("the file is locked");
+        await Assert.That(pair.Listing.Single().Status).IsEqualTo("the file is locked");
         await Assert.That(pair.Failures.Single()).Contains("the file is locked");
     }
 
@@ -357,7 +357,7 @@ public async Task TrayAcceptAllEmptiesTheOwningViewer()
         await pair.Tracker.AcceptAll();
 
         await Assert.That(pair.Viewer.Queue).IsEmpty();
-        await Assert.That(pair.Tracker.Snapshots).IsEmpty();
+        await Assert.That(pair.Listing).IsEmpty();
         await Assert.That(pair.Applied.Count).IsEqualTo(2);
     }
 
@@ -389,7 +389,7 @@ public async Task TrayAcceptOfOneSnapshotLeavesTheRestInTheOwningViewer()
         await pair.Tracker.Accept(snapshot);
 
         await Assert.That(pair.Viewer.Keys()).IsEquivalentTo([Key(other, 7)]);
-        await Assert.That(pair.Tracker.Snapshots.Select(_ => _.Key)).IsEquivalentTo([Key(other, 7)]);
+        await Assert.That(pair.Listing.Select(_ => _.Key)).IsEquivalentTo([Key(other, 7)]);
     }
 
     [Test]
@@ -415,7 +415,7 @@ public async Task TrayDiscardAllEmptiesTheOwningViewer()
         pair.Tracker.Clear();
 
         await Assert.That(pair.Viewer.Queue).IsEmpty();
-        await Assert.That(pair.Tracker.Snapshots).IsEmpty();
+        await Assert.That(pair.Listing).IsEmpty();
         await Assert.That(pair.Applied).IsEmpty();
     }
 
@@ -429,7 +429,7 @@ public async Task ViewerAcceptReachesTheTrayListing()
         pair.Act(CommandKind.Accept, Key(sample, 1));
 
         await Assert.That(pair.Viewer.Keys()).IsEquivalentTo([Key(other, 7)]);
-        await Assert.That(pair.Tracker.Snapshots.Select(_ => _.Key)).IsEquivalentTo([Key(other, 7)]);
+        await Assert.That(pair.Listing.Select(_ => _.Key)).IsEquivalentTo([Key(other, 7)]);
     }
 
     [Test]
@@ -442,7 +442,7 @@ public async Task ViewerAcceptAllEmptiesTheTrayListing()
         pair.Act(CommandKind.AcceptAll, null);
 
         await Assert.That(pair.Viewer.Queue).IsEmpty();
-        await Assert.That(pair.Tracker.Snapshots).IsEmpty();
+        await Assert.That(pair.Listing).IsEmpty();
         await Assert.That(pair.Tracker.TrackingAny).IsFalse();
     }
 
@@ -456,7 +456,7 @@ public async Task ViewerDiscardReachesTheTrayListing()
         pair.Act(CommandKind.Discard, Key(sample, 1));
 
         await Assert.That(pair.Viewer.Keys()).IsEquivalentTo([Key(other, 7)]);
-        await Assert.That(pair.Tracker.Snapshots.Select(_ => _.Key)).IsEquivalentTo([Key(other, 7)]);
+        await Assert.That(pair.Listing.Select(_ => _.Key)).IsEquivalentTo([Key(other, 7)]);
         await Assert.That(pair.Applied).IsEmpty();
     }
 
@@ -473,7 +473,7 @@ public async Task AFailedAcceptStaysPendingOnBothSidesOfAnOwningViewer()
         await pair.Tracker.Accept(snapshot);
 
         await Assert.That(pair.Viewer.Queue.Single().Status).IsEqualTo("the file is locked");
-        await Assert.That(pair.Tracker.Snapshots.Single().Status).IsEqualTo("the file is locked");
+        await Assert.That(pair.Listing.Single().Status).IsEqualTo("the file is locked");
         await Assert.That(pair.Failures.Single()).Contains("the file is locked");
     }
 
@@ -492,7 +492,7 @@ public async Task TrayAcceptAllReportsWhatTheOwningViewerKept()
 
         await Assert.That(pair.Viewer.Keys()).IsEquivalentTo([Key(sample, 1)]);
         await Assert.That(pair.Viewer.Queue.Single().Status).IsEqualTo("the file is locked");
-        await Assert.That(pair.Tracker.Snapshots.Single().Status).IsEqualTo("the file is locked");
+        await Assert.That(pair.Listing.Single().Status).IsEqualTo("the file is locked");
         await Assert.That(pair.Failures.Single()).Contains("the file is locked");
     }
 
@@ -518,7 +518,7 @@ public async Task ASlowAcceptIsWaitedForRatherThanCalledAMissingViewer()
 
         await Assert.That(pair.Failures).IsEmpty();
         await Assert.That(pair.Viewer.Queue).IsEmpty();
-        await Assert.That(pair.Tracker.Snapshots).IsEmpty();
+        await Assert.That(pair.Listing).IsEmpty();
     }
 
     /// <inheritdoc cref="ASlowAcceptIsWaitedForRatherThanCalledAMissingViewer"/>
@@ -821,6 +821,24 @@ public TrayOwned(Func<InlinePatch, InlineApplyResult>? applier = null)
         public List<string> Warnings { get; } = [];
         public List<string> Failures { get; } = [];
 
+        /// <summary>
+        /// What the tray menu is built from. <see cref="Tracker.Snapshots"/> is the last listing
+        /// seen rather than a live read, so this refreshes first - standing in for the two second
+        /// scan, and for <see cref="OwnedInlineHost.Changed"/> where that is what keeps it current.
+        /// <para>
+        /// Reading the property alone would assert against whatever the cache happened to hold,
+        /// which for an empty expectation is a test that cannot fail.
+        /// </para>
+        /// </summary>
+        public IReadOnlyList<PendingSnapshot> Listing
+        {
+            get
+            {
+                Tracker.Refresh();
+                return Tracker.Snapshots;
+            }
+        }
+
         readonly string root = TempRoot();
 
         /// <summary>
@@ -858,7 +876,7 @@ public PendingSnapshot Queue(string source, int line, string content = "new", st
                 throw new($"The owner refused the patch. {response.Message}");
             }
 
-            return Tracker.Snapshots.Single(_ => _.Key == Key(source, line));
+            return Listing.Single(_ => _.Key == Key(source, line));
         }
 
         public TrackedMoveFiles AddMove()
@@ -982,6 +1000,20 @@ public TrackedMoveFiles StageMove()
 
         public SessionState Viewer => Window.State;
 
+        /// <summary>
+        /// What the tray menu is built from, refreshed first - see
+        /// <see cref="TrayOwned.Listing"/>. It matters more on this side: with the queue in the
+        /// other process, the cache only moves when something refreshes it.
+        /// </summary>
+        public IReadOnlyList<PendingSnapshot> Listing
+        {
+            get
+            {
+                Tracker.Refresh();
+                return Tracker.Snapshots;
+            }
+        }
+
         public void Queue(string source, int line, string content = "new", string? framework = null)
         {
             var message = new ViewerMessage(ViewerVerb.Inline, Body: Payload(source, line, content, framework));
@@ -995,7 +1027,7 @@ public void Queue(string source, int line, string content = "new", string? frame
         public PendingSnapshot Snapshot(string source, int line, string content = "new", string? framework = null)
         {
             Queue(source, line, content, framework);
-            return Tracker.Snapshots.Single(_ => _.Key == Key(source, line));
+            return Listing.Single(_ => _.Key == Key(source, line));
         }
 
         public ViewerResponse Send(ViewerMessage message)

src/DiffEngineTray/RemoteInlineHost.cs

@@ -1,17 +1,24 @@
+using System.Net.NetworkInformation;
+
 /// <summary>
 /// The queue belongs to a viewer that bound the port before this tray started, so every call is a
 /// short loopback round trip and the tray is a remote control.
 /// <para>
-/// The listing and the menu verbs use ViewerClient.ShortTimeout. Those run from the 2 second scan
-/// timer and from the menu opening, so a slow exchange must not outlast the timer period or block
-/// the UI.
+/// The listing and the menu verbs use ViewerClient.ShortTimeout. The listing runs from the 2
+/// second scan timer, so a slow exchange must not outlast the timer period. It is no longer what
+/// the menu is built from - see <see cref="Tracker.Snapshots"/> - so nothing here is waited on
+/// from the UI thread except a verb the user clicked.
 /// </para>
 /// <para>
 /// Accepting does not, for the reason <see cref="acceptWait"/> gives.
 /// </para>
 /// <para>
-/// A refused connection means the viewer has gone, which is the same as nothing pending. The queue
-/// went with it, and this tray does not take ownership: it was decided at startup.
+/// No owner means the viewer has gone, which is the same as nothing pending. The queue went with
+/// it, and this tray does not take ownership: it was decided at startup.
+/// </para>
+/// <para>
+/// "No owner" is asked of the OS rather than found out by connecting - see
+/// <see cref="PortIsHeld"/>.
 /// </para>
 /// </summary>
 class RemoteInlineHost : IInlineHost
@@ -156,6 +163,50 @@ static bool Send(ViewerVerb verb, string? key, TimeSpan wait, out string? messag
         return response.Ok;
     }
 
-    static bool Exchange(ViewerMessage message, TimeSpan wait, [NotNullWhen(true)] out ViewerResponse? response) =>
-        ViewerClient.TrySend(message, out response, wait: wait);
+    static bool Exchange(ViewerMessage message, TimeSpan wait, [NotNullWhen(true)] out ViewerResponse? response)
+    {
+        if (!PortIsHeld())
+        {
+            response = null;
+            return false;
+        }
+
+        return ViewerClient.TrySend(message, out response, wait: wait);
+    }
+
+    /// <summary>
+    /// Whether anything holds the port, asked of the OS rather than found out by connecting to it.
+    /// <para>
+    /// Connecting to a port nothing is listening on is supposed to be refused at once, and every
+    /// caller here was written expecting that. It is not refused at once on every machine: where
+    /// the SYN is dropped rather than answered with a reset, the connect runs to its timeout
+    /// instead. Once the owning viewer exits, that is the full
+    /// <see cref="ViewerClient.ShortTimeout"/> per call - half a second on the two second scan,
+    /// and half a second on every menu verb - for the rest of this tray's life, because ownership
+    /// is decided at startup and this host is never replaced.
+    /// </para>
+    /// <para>
+    /// The listener table is a local kernel query costing well under a millisecond, and it answers
+    /// the only question worth asking first. Matched on the port alone: a listener on any address
+    /// accepts a loopback connection, so the round trip is skipped only when nothing at all holds
+    /// the port. Racing it is harmless either way - an owner that binds just after the check is
+    /// found by the next call, and one that exits just after it costs the timeout exactly as
+    /// before.
+    /// </para>
+    /// </summary>
+    static bool PortIsHeld()
+    {
+        var port = ViewerClient.Port;
+        try
+        {
+            return IPGlobalProperties.GetIPGlobalProperties()
+                .GetActiveTcpListeners()
+                .Any(_ => _.Port == port);
+        }
+        catch (NetworkInformationException)
+        {
+            // No table to read, so let the connect decide as it always did
+            return true;
+        }
+    }
 }

src/DiffEngineTray/Tracker.cs

@@ -10,8 +10,9 @@ class Tracker :
     ConcurrentDictionary<string, TrackedMove> moves = new(StringComparer.OrdinalIgnoreCase);
     ConcurrentDictionary<string, TrackedDelete> deletes = new(StringComparer.OrdinalIgnoreCase);
     IInlineHost inline;
-    // The last listing seen, used for the icon state. Free when this tray owns the queue, and a
-    // loopback round trip when a viewer does, which is why the menu re-reads live when it opens.
+    // The last listing seen, used for the icon state and for the menu. Free when this tray owns
+    // the queue, and a loopback round trip when a viewer does, which is why nothing re-reads it
+    // from a click.
     IReadOnlyList<PendingSnapshot> snapshots = [];
     AsyncTimer timer;
     int lastScanCount;
@@ -31,6 +32,11 @@ public Tracker(Action active, Action inactive, LockedFilesResolver? lockedFilesR
             {
                 ExceptionHandler.Handle("Failed to scan files", exception);
             });
+
+        // Seeded rather than left empty until the first scan two seconds later. The menu reads
+        // this cache now, so without it a tray that has just started shows none of what a viewer
+        // already had queued - and the icon stays dark for the same two seconds.
+        Refresh();
     }
 
     Task ScanFiles(Cancel cancel)
@@ -316,19 +322,20 @@ public Task Discard(PendingSnapshot snapshot) =>
             }
         });
 
-    public Task AcceptAllSnapshots()
-    {
-        // Live read, not the scan cache: this can be called before the first scan, and acting on
-        // a stale empty cache would silently do nothing.
-        if (Snapshots.Count == 0)
-        {
-            return Task.CompletedTask;
-        }
-
-        return Task.Run(() =>
+    public Task AcceptAllSnapshots() =>
+        Task.Run(() =>
         {
             try
             {
+                // Live read, not the scan cache: this can be called before the first scan, and
+                // acting on a stale empty cache would silently do nothing. Inside the worker
+                // rather than in front of it, because the caller is a menu click or a hot key and
+                // the read is a round trip whenever a viewer owns the queue.
+                if (inline.List().Count == 0)
+                {
+                    return;
+                }
+
                 if (!inline.AcceptAll(out var message))
                 {
                     inlineFailed?.Invoke($"Could not accept the pending snapshots. {message}");
@@ -341,7 +348,6 @@ public Task AcceptAllSnapshots()
                 ExceptionHandler.Handle("Failed to accept the pending snapshots", exception);
             }
         });
-    }
 
     /// <summary>
     /// Accepts just these snapshots, for a group header: unlike <see cref="AcceptAllSnapshots"/>,
@@ -936,17 +942,24 @@ int ITrackedFiles.DiscardAll()
     }
 
     /// <summary>
-    /// Read live rather than from the scan cache, so the menu shows the viewer's current queue at
-    /// the moment it opens.
+    /// The last listing seen, rather than a fresh one.
+    /// <para>
+    /// This is what the menu is built from, and building it runs on the UI thread inside
+    /// <c>ContextMenuStrip.Opening</c>. Reading live there put a loopback round trip between the
+    /// right click and the menu whenever a viewer owned the queue. Worse, a connection to a port
+    /// nothing is listening on is only refused at once on some machines - where the SYN is dropped
+    /// instead, an owner that had exited cost the whole of
+    /// <see cref="ViewerClient.ShortTimeout"/>, so every menu open took half a second for the rest
+    /// of the tray's life.
+    /// </para>
+    /// <para>
+    /// Nothing is lost where the queue is held here: <see cref="OwnedInlineHost.Changed"/> runs
+    /// <see cref="Refresh"/> on every mutation, and the tray's own accepts and discards refresh
+    /// too, so the cache is the live queue. Where a viewer holds it, the listing is at most one
+    /// scan old - which is what <see cref="TrackingAny"/> and the icon have always shown.
+    /// </para>
     /// </summary>
-    public IReadOnlyList<PendingSnapshot> Snapshots
-    {
-        get
-        {
-            snapshots = inline.List();
-            return snapshots;
-        }
-    }
+    public IReadOnlyList<PendingSnapshot> Snapshots => snapshots;
 
     /// <summary>
     /// Deliberately not <see cref="Clear"/>: exiting is not discarding. The diff tools this tray