From 41582cef3a208ab26ea360a599c0a73ed5f99dd1 Mon Sep 17 00:00:00 2001 From: Simon Cropp Date: Sat, 22 Aug 2026 10:17:55 +1000 Subject: [PATCH] Tell a refused inline apart from an absent owner TrySendAsync collapsed an error reply and a refused connection into the same false, and AddInlineAsync read that as "nobody owns the queue" and launched a viewer. But an owner that answered is still there and still holds the port, so the launched viewer cannot bind it - and AddInlineAsync returned Queued anyway, as soon as stdin was written. So a snapshot the owner declined - an older owner that does not understand the payload, or a handler that threw - was reported to the caller as queued while being held by nothing at all. Nothing was staged either, because staging is what NoViewerFound is for. The snapshot simply vanished, and the test that produced it went on reporting a failure with nowhere to review it. SendAsync returns three outcomes instead of two: NoOwner, Accepted, Refused. TrySendAsync stays as it was, as a wrapper, so PendingFiles and the rest are untouched. AddInlineAsync launches a viewer only on NoOwner, and reports NoViewerFound on a refusal - which is accurate from the caller's side, since in both cases the snapshot is pending nowhere and staging is the right answer. The rest of that item - RunInline ignoring response.Ok, and persisting through InlineStaging on a forward failure - is in DiffEngineViewer and not touched here. --- src/DiffEngine.Tests/ViewerProtocolTests.cs | 58 +++++++++++++++++++++ src/DiffEngine/DiffRunner_Inline.cs | 17 +++++- src/DiffEngine/Protocol/ViewerClient.cs | 49 +++++++++++++++-- 3 files changed, 118 insertions(+), 6 deletions(-) diff --git a/src/DiffEngine.Tests/ViewerProtocolTests.cs b/src/DiffEngine.Tests/ViewerProtocolTests.cs index 7ed60f8e..5b4f4d8e 100644 --- a/src/DiffEngine.Tests/ViewerProtocolTests.cs +++ b/src/DiffEngine.Tests/ViewerProtocolTests.cs @@ -733,6 +733,64 @@ public async Task AnUnresponsiveOwnerTimesOutRatherThanHanging() listener.Stop(); } } + /// + /// An owner that answers with an error is not an absent one. Collapsing the two into false + /// meant a refused inline was read as "nobody is there", so a second viewer was launched, it + /// could not bind the port, and the snapshot was reported as Queued while being held by + /// nothing at all. + /// + [Test] + public async Task ARefusedExchangeIsToldApartFromAnAbsentOwner() + { + await Assert.That(ViewerServer.TryBind(0, out var bound)).IsTrue(); + using var server = bound!; + using var cancel = new CancelSource(); + var listening = server.Listen(_ => ViewerResponse.Error("no thanks"), cancel.Token); + + try + { + var refused = await ViewerClient.SendAsync(new(ViewerVerb.List), default, server.Port); + + await Assert.That(refused).IsEqualTo(SendOutcome.Refused); + } + finally + { + await cancel.CancelAsync(); + await Wait(listening); + } + } + + [Test] + public async Task AnAcceptedExchangeReportsAccepted() + { + await Assert.That(ViewerServer.TryBind(0, out var bound)).IsTrue(); + using var server = bound!; + using var cancel = new CancelSource(); + var listening = server.Listen(_ => ViewerResponse.Success("fine"), cancel.Token); + + try + { + await Assert.That(await ViewerClient.SendAsync(new(ViewerVerb.List), default, server.Port)) + .IsEqualTo(SendOutcome.Accepted); + } + finally + { + await cancel.CancelAsync(); + await Wait(listening); + } + } + + [Test] + public async Task AnAbsentOwnerReportsNoOwner() + { + ViewerServer.TryBind(0, out var server); + var port = server!.Port; + server.Dispose(); + + await Assert.That(await ViewerClient.SendAsync(new(ViewerVerb.List), default, port, TimeSpan.FromSeconds(2))) + .IsEqualTo(SendOutcome.NoOwner); + } + [Test] public async Task AnAbsentOwnerIsNotAnError() { diff --git a/src/DiffEngine/DiffRunner_Inline.cs b/src/DiffEngine/DiffRunner_Inline.cs index c01cef60..6c247b25 100644 --- a/src/DiffEngine/DiffRunner_Inline.cs +++ b/src/DiffEngine/DiffRunner_Inline.cs @@ -14,7 +14,9 @@ public enum InlineResult Disabled, /// - /// No DiffEngineViewer could be resolved. Callers that want a fallback should use it here. + /// Nothing has the snapshot. No DiffEngineViewer could be resolved, or the owner of the queue + /// declined the payload - which is the same thing from the caller's side, since in both cases + /// the snapshot is pending nowhere. Callers that want a fallback should use it here. /// NoViewerFound } @@ -61,11 +63,22 @@ public static async Task AddInlineAsync(InlinePatch patch, Cancel // Onto the payload rather than onto the patch, which belongs to the caller and may be // held or sent again var payload = InlinePatchFile.Build(patch, patch.Framework ?? RuntimeMoniker.Current); - if (await ViewerClient.TrySendAsync(new(ViewerVerb.Inline, Body: payload), cancel)) + var outcome = await ViewerClient.SendAsync(new(ViewerVerb.Inline, Body: payload), cancel); + if (outcome == SendOutcome.Accepted) { return InlineResult.Queued; } + if (outcome == SendOutcome.Refused) + { + // An owner that is there and said no - an older one that does not understand the + // payload, or a handler that threw. Launching a second viewer cannot change that + // answer, and it would bind nothing, so reporting Queued would say the snapshot is + // somewhere when it is nowhere. Report it as no viewer, which is the answer that + // makes the caller stage the files instead + return InlineResult.NoViewerFound; + } + var launched = await ViewerLauncher.LaunchAsync(patch, payload, cancel); return launched ? InlineResult.Queued : InlineResult.NoViewerFound; } diff --git a/src/DiffEngine/Protocol/ViewerClient.cs b/src/DiffEngine/Protocol/ViewerClient.cs index 9f36941a..3d0aaa3a 100644 --- a/src/DiffEngine/Protocol/ViewerClient.cs +++ b/src/DiffEngine/Protocol/ViewerClient.cs @@ -5,6 +5,31 @@ namespace DiffEngine; /// caller turns into a launch (DiffEngine), "nothing pending" (the tray), or "the owner has gone" /// (an attached viewer). /// +/// +/// What came back from an exchange with the queue owner. Three outcomes rather than two, because +/// "nobody is there" and "the owner said no" call for opposite responses: the first is fixed by +/// launching a viewer, and the second is not. +/// +enum SendOutcome +{ + /// + /// Nobody answered. No owner, or one present but unresponsive - the caller cannot tell, and + /// for its purposes they are the same. + /// + NoOwner, + + /// + /// The owner answered and took it. + /// + Accepted, + + /// + /// The owner answered and declined it: a version it does not understand, or a handler that + /// threw. Launching another viewer will not change that answer. + /// + Refused +} + static class ViewerClient { public const int DefaultPort = 3493; @@ -114,6 +139,18 @@ public static bool TrySend( /// /// public static async Task TrySendAsync( + ViewerMessage message, + Cancel cancel, + int? port = null, + TimeSpan? wait = null) => + await SendAsync(message, cancel, port, wait) == SendOutcome.Accepted; + + /// + /// As , but says which of the two failures happened. A caller that + /// would launch a viewer on absence needs that: launching one because the owner refused the + /// payload leaves two processes and still no snapshot. + /// + public static async Task SendAsync( ViewerMessage message, Cancel cancel, int? port = null, @@ -154,8 +191,12 @@ public static async Task TrySendAsync( #else var text = await reader.ReadToEndAsync(); #endif - return ViewerResponse.TryParse(text, out var response) && - response.Ok; + if (!ViewerResponse.TryParse(text, out var response)) + { + return SendOutcome.NoOwner; + } + + return response.Ok ? SendOutcome.Accepted : SendOutcome.Refused; } // 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 @@ -169,13 +210,13 @@ public static async Task TrySendAsync( 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; + return SendOutcome.NoOwner; } // Cancellation is the caller's business; a missing owner is not. catch (Exception exception) when (exception is not OperationCanceledException && Ignorable(exception)) { - return false; + return SendOutcome.NoOwner; } }