Hold a group's deletes until its snapshots are written, from an attached viewer

src/DiffEngine.Tests/InlineQueueClientTests.cs

@@ -412,16 +412,17 @@ bool IQueueOwner.Has(string key)
             }
         }
 
-        (bool ok, string? message) IQueueOwner.Accept(string key, string? origin)
+        (bool ok, string? message, bool written) IQueueOwner.Accept(string key, string? origin)
         {
             lock (gate)
             {
                 if (queue.Find(key) is null)
                 {
-                    return (false, null);
+                    return (false, null, false);
                 }
 
                 var before = queue;
+                var applied = Applied.Count;
                 queue = origin is null
                     ? queue.Accept(key, Record, out var message)
                     : queue.Accept(key, origin, Record, out message);
@@ -430,10 +431,11 @@ bool IQueueOwner.Has(string key)
                 // refusal rather than an attempt and goes on the wire as an error.
                 if (ReferenceEquals(before, queue))
                 {
-                    return (false, message);
+                    return (false, message, false);
                 }
 
-                return (true, message);
+                // Record keeps only what landed
+                return (true, message, Applied.Count > applied);
             }
         }
 

src/DiffEngine.Tests/ViewerProtocolTests.cs

@@ -447,6 +447,38 @@ public async Task ARefusedAcceptGoesOnTheWireAsAnError()
         await Assert.That(done.Message).IsEqualTo("Applied Tests.cs:42");
     }
 
+    /// <summary>
+    /// Whether an accept left its snapshot in the source, which ok cannot say: a patch whose call
+    /// site moved is attempted, so ok, and dropped unwritten. A surface accepting a group from
+    /// someone else's queue sends the group's deletes on it. A reply without it, from an owner
+    /// that predates it, reads as null, which that surface takes as not written.
+    /// </summary>
+    [Test]
+    public async Task AnAcceptSaysWhetherTheSnapshotWasWritten()
+    {
+        static ViewerResponse RoundTrip(ViewerResponse response)
+        {
+            if (!ViewerResponse.TryParse(response.Build(), out var parsed))
+            {
+                throw new("Unreadable response.");
+            }
+
+            return parsed;
+        }
+
+        var written = RoundTrip(ViewerMessageHandler.Handle(new FakeOwner((true, "Applied Tests.cs:42"), written: true), new(ViewerVerb.Accept, "key")));
+        await Assert.That(written.Written).IsTrue();
+
+        var stale = RoundTrip(ViewerMessageHandler.Handle(new FakeOwner((true, "Not written")), new(ViewerVerb.Accept, "key")));
+        await Assert.That(stale.Ok).IsTrue();
+        await Assert.That(stale.Written).IsFalse();
+
+        var discarded = RoundTrip(ViewerMessageHandler.Handle(new FakeOwner((true, null), written: true), new(ViewerVerb.Discard, "key")));
+        await Assert.That(discarded.Written).IsNull();
+
+        await Assert.That(RoundTrip(ViewerResponse.Success("Applied Tests.cs:42")).Written).IsNull();
+    }
+
     /// <summary>
     /// A pending file with no tray running. The paths ride key and body rather than an encoded
     /// payload, because that is all a tracked move or delete is.
@@ -539,7 +571,7 @@ public async Task AnAcceptForwardsItsOriginToTheOwner()
         await Assert.That(owner.AcceptedOrigin).IsEqualTo("net9.0");
     }
 
-    class FakeOwner((bool ok, string? message) act) :
+    class FakeOwner((bool ok, string? message) act, bool written = false) :
         IQueueOwner
     {
         public string? AcceptedOrigin { get; private set; }
@@ -562,10 +594,10 @@ public void TrackDelete(string file) =>
 
         public bool Has(string key) => true;
 
-        public (bool ok, string? message) Accept(string key, string? origin)
+        public (bool ok, string? message, bool written) Accept(string key, string? origin)
         {
             AcceptedOrigin = origin;
-            return act;
+            return (act.ok, act.message, written);
         }
 
         public (bool ok, string? message) Discard(string key) => act;

src/DiffEngine/Protocol/IQueueOwner.cs

@@ -49,8 +49,14 @@ interface IQueueOwner
     /// nothing was attempted, and the message says why — a conflicted entry with no origin to
     /// pick, or a locked tracked move. True means attempted, including a retryable apply failure,
     /// whose message says what went wrong while the entry stays pending.
+    /// <para>
+    /// Written is whether a snapshot is in the source now: applied, or found already there. False
+    /// for anything else, a tracked file included, since that has no snapshot. It is what a surface
+    /// accepting a group from someone else's queue waits on before sending the group's deletes,
+    /// because ok cannot say it: a patch whose call site moved is attempted, and dropped unwritten.
+    /// </para>
     /// </summary>
-    (bool ok, string? message) Accept(string key, string? origin);
+    (bool ok, string? message, bool written) Accept(string key, string? origin);
 
     (bool ok, string? message) Discard(string key);
 

src/DiffEngine/Protocol/ViewerMessageHandler.cs

@@ -141,16 +141,33 @@ static ViewerResponse Act(IQueueOwner owner, string? key, string? body, ViewerVe
             return ViewerResponse.Error($"{verb} requires a key");
         }
 
+        if (verb == ViewerVerb.Discard)
+        {
+            return Reply(key, owner.Discard(key));
+        }
+
         // The body is the variant origin a reviewer picked, and only an accept carries one.
-        var (ok, message) = verb == ViewerVerb.Accept
-            ? owner.Accept(key, body)
-            : owner.Discard(key);
+        var (ok, message, written) = owner.Accept(key, body);
+        var reply = Reply(key, (ok, message));
         if (!ok)
         {
-            return ViewerResponse.Error(message ?? $"No pending snapshot for {key}");
+            return reply;
+        }
+
+        return reply with
+        {
+            Written = written
+        };
+    }
+
+    static ViewerResponse Reply(string key, (bool ok, string? message) result)
+    {
+        if (!result.ok)
+        {
+            return ViewerResponse.Error(result.message ?? $"No pending snapshot for {key}");
         }
 
-        return ViewerResponse.Success(message);
+        return ViewerResponse.Success(result.message);
     }
 
     static ViewerResponse Focus(IQueueOwner owner, string? key)

src/DiffEngine/Protocol/ViewerResponse.cs

@@ -72,6 +72,13 @@ record ViewerResponse(
     /// </summary>
     public AcceptProgress? Progress { get; init; }
 
+    /// <summary>
+    /// On an accept: whether a snapshot is in the source now (see <see cref="IQueueOwner.Accept"/>).
+    /// Null on every other reply, and on any reply from an owner that predates it, which a reader
+    /// needing an answer takes as not written: what waits on it is a delete.
+    /// </summary>
+    public bool? Written { get; init; }
+
     public static ViewerResponse Success(string? message = null) =>
         new(true, message, []);
 
@@ -111,6 +118,11 @@ public string Build()
             builder.Append($"progress: {Progress.Build()}\n");
         }
 
+        if (Written is { } written)
+        {
+            builder.Append($"written: {(written ? "true" : "false")}\n");
+        }
+
         foreach (var item in Items)
         {
             var status = item.Status is null ? "" : ViewerPayload.Encode(item.Status);
@@ -159,6 +171,7 @@ public static bool TryParse(string text, [NotNullWhen(true)] out ViewerResponse?
         WindowCommand? window = null;
         string? windowKey = null;
         AcceptProgress? progress = null;
+        bool? written = null;
         var items = new List<ViewerResponseItem>();
         var moves = new List<ViewerResponseMove>();
         var deletes = new List<ViewerResponseDelete>();
@@ -191,6 +204,9 @@ public static bool TryParse(string text, [NotNullWhen(true)] out ViewerResponse?
                         return false;
                     }
 
+                    continue;
+                case "written":
+                    written = value == "true";
                     continue;
                 case "message":
                     if (!ViewerPayload.TryDecode(value, out message))
@@ -272,7 +288,8 @@ public static bool TryParse(string text, [NotNullWhen(true)] out ViewerResponse?
         {
             Moves = moves,
             Deletes = deletes,
-            Progress = progress
+            Progress = progress,
+            Written = written
         };
         return true;
     }

src/DiffEngineTray.Tests/TrayViewerSyncTest.cs

@@ -242,6 +242,45 @@ public async Task ViewerAcceptOfOneSnapshotReachesTheTray()
         await Assert.That(pair.Applied.Select(_ => _.LineHint)).IsEquivalentTo([1]);
     }
 
+    /// <summary>
+    /// "Accept all in" a group, from a window attached to the tray, whose patch the tray cannot
+    /// write because the call site moved since the run. The tray takes each accept as asked and a
+    /// stale patch goes as an applied one does, so only the reply says it was not written - and
+    /// the verified file the delete would remove is the one copy of that snapshot left.
+    /// </summary>
+    [Test]
+    public async Task AViewerGroupAcceptHoldsItsDeletesWhenTheTrayCouldNotWriteASnapshot()
+    {
+        await using var pair = new TrayOwned(_ => InlineApplyResult.NotFound("Could not locate the call"));
+        pair.Queue(sample, 1);
+        var delete = pair.AddDelete();
+        pair.Pump();
+
+        pair.Link.PostAcceptGroup([], [Key(sample, 1)], [delete.Key]);
+
+        var viewer = pair.Pump();
+        await Assert.That(File.Exists(delete.File)).IsTrue();
+        await Assert.That(pair.Tracker.Deletes).HasSingleItem();
+        await Assert.That(viewer.Message).IsEqualTo(OwnerLink.DeletesHeld);
+    }
+
+    [Test]
+    public async Task AViewerGroupAcceptCarriesOutItsDeletesOnceTheTrayWroteTheSnapshots()
+    {
+        await using var pair = new TrayOwned();
+        pair.Queue(sample, 1);
+        var move = pair.AddMove();
+        var delete = pair.AddDelete();
+        pair.Pump();
+
+        pair.Link.PostAcceptGroup([move.Key], [Key(sample, 1)], [delete.Key]);
+
+        await Assert.That(pair.Pump().Queue).IsEmpty();
+        await Assert.That(pair.Applied.Select(_ => _.LineHint)).IsEquivalentTo([1]);
+        await Assert.That(File.Exists(delete.File)).IsFalse();
+        await Assert.That(await File.ReadAllTextAsync(move.Target)).IsEqualTo("received");
+    }
+
     [Test]
     public async Task ViewerDiscardOfOneSnapshotReachesTheTray()
     {

src/DiffEngineTray/OwnedInlineHost.cs

@@ -297,34 +297,34 @@ bool IQueueOwner.Has(string key)
         }
     }
 
-    (bool ok, string? message) IQueueOwner.Accept(string key, string? origin)
+    (bool ok, string? message, bool written) IQueueOwner.Accept(string key, string? origin)
     {
         if (TrackedKeys.IsTracked(key))
         {
-            var result = TrackedFiles?.Accept(key) ?? (false, null);
-            if (result.ok)
+            var (ok, text) = TrackedFiles?.Accept(key) ?? (false, null);
+            if (ok)
             {
                 Changed?.Invoke();
             }
 
-            return result;
+            return (ok, text, false);
         }
 
         var (outcome, message, refused) = AcceptOne(key, origin);
         if (outcome == AcceptOutcome.Unknown)
         {
-            return (false, null);
+            return (false, null, false);
         }
 
         if (refused)
         {
             // Nothing changed and nothing was attempted; the message says what a reviewer has to
             // do, and it goes on the wire as an error so a remote surface shows it as one.
-            return (false, message);
+            return (false, message, false);
         }
 
         Changed?.Invoke();
-        return (true, message);
+        return (true, message, outcome == AcceptOutcome.Applied);
     }
 
     (bool ok, string? message) IQueueOwner.Discard(string key)