Stop losing snapshots and source files on eight paths

.github/workflows/build-native.yml

@@ -47,22 +47,33 @@ jobs:
       - name: Checkout
         uses: actions/checkout@v4
 
-      - name: Install build dependencies
+      # In a manylinux_2_28 container rather than on the runner, for its glibc 2.28: the floor
+      # .NET 10 itself supports, which is RHEL 8. A library records the glibc symbol versions of
+      # the headers it was compiled against, and the loader refuses it anywhere older - built on
+      # the runner's own Ubuntu 24.04 it needed GLIBC_2.38, so the viewer could not start on
+      # Ubuntu 22.04, Debian 12, or RHEL 8 or 9, and every inline snapshot sent to it was lost.
+      - name: Build
         if: runner.os == 'Linux'
-        run: |
-          sudo apt-get update
-          sudo apt-get install -y --no-install-recommends \
-            cmake ninja-build \
-            libx11-dev libxrandr-dev libxi-dev libxcursor-dev libxinerama-dev \
-            libgl1-mesa-dev libglu1-mesa-dev libwayland-dev libxkbcommon-dev
-
-      - name: Configure
-        if: matrix.rid != 'osx'
-        run: cmake -S native -B build -G Ninja -DCMAKE_BUILD_TYPE=Release
+        run: docker run --rm -v "$PWD:/src" -w /src "quay.io/pypa/manylinux_2_28_$(uname -m)" bash native/build-linux.sh
 
-      - name: Build
-        if: matrix.rid != 'osx'
-        run: cmake --build build --config Release
+      # The floor the container is there to hold, checked rather than trusted, since nothing else
+      # would notice until a user on an older distribution did.
+      - name: Check glibc floor
+        if: runner.os == 'Linux'
+        env:
+          GLIBC_FLOOR: GLIBC_2.28
+        run: |
+          highest=$(objdump -T build/libdiffengine_viewer.so | grep -o 'GLIBC_[0-9][0-9.]*' | sort -uV | tail -n 1)
+          echo "Highest glibc symbol version required: $highest"
+          if [ "$(printf '%s\n' "$GLIBC_FLOOR" "$highest" | sort -V | tail -n 1)" != "$GLIBC_FLOOR" ]; then
+            echo "::error::libdiffengine_viewer.so requires $highest, above the $GLIBC_FLOOR floor"
+            exit 1
+          fi
+          # libGL.so.1 is on every machine with a GL driver; libOpenGL.so.0 is not
+          if objdump -p build/libdiffengine_viewer.so | grep -q 'NEEDED.*libOpenGL'; then
+            echo "::error::libdiffengine_viewer.so links libOpenGL.so.0 rather than libGL.so.1"
+            exit 1
+          fi
 
       # macOS draws with AppKit and Core Text rather than raylib and ImGui, so it is a Swift
       # package rather than a CMake project. Both --arch flags in one invocation produce a
@@ -85,7 +96,7 @@ jobs:
           }
           case "${{ matrix.rid }}" in
             linux-*)
-              strip build/libdiffengine_viewer.so
+              # Already stripped, inside the container that owns the build directory
               collect DiffEngineViewer.Linux "${{ matrix.rid }}" build/libdiffengine_viewer.so
               ;;
             osx)

native/build-linux.sh

@@ -0,0 +1,32 @@
+#!/usr/bin/env bash
+# Builds build/libdiffengine_viewer.so inside a manylinux_2_28 container, whose glibc 2.28 is the
+# floor the library then needs - see the Linux build step in .github/workflows/build-native.yml.
+# Runs from the repository root:
+#
+#   docker run --rm -v "$PWD:/src" -w /src "quay.io/pypa/manylinux_2_28_$(uname -m)" bash native/build-linux.sh
+set -euo pipefail
+
+# What raylib's bundled GLFW builds against, for both its X11 and Wayland backends: the set the
+# runner used to install, under this distribution's names.
+dnf install -y \
+  git pkgconfig \
+  libX11-devel libXext-devel libXrandr-devel libXi-devel libXcursor-devel libXinerama-devel \
+  mesa-libGL-devel wayland-devel libxkbcommon-devel
+
+# native/CMakeLists.txt needs CMake 3.24, newer than the distribution's own. The image carries
+# current releases through pipx, so these are only installed where it does not.
+export PATH="$HOME/.local/bin:$PATH"
+for tool in cmake ninja; do
+  if ! command -v "$tool" > /dev/null 2>&1; then
+    pipx install "$tool"
+  fi
+done
+
+# LEGACY links libGL.so.1, which every GL driver ships. This distribution's CMake otherwise
+# prefers the GLVND split and links libOpenGL.so.0, which a machine with only libgl1 lacks.
+cmake -S native -B build -G Ninja -DCMAKE_BUILD_TYPE=Release -DOpenGL_GL_PREFERENCE=LEGACY
+cmake --build build --config Release
+
+# Here rather than in the workflow's collect step: the build directory belongs to this
+# container's root, and the runner cannot rewrite what is in it.
+strip build/libdiffengine_viewer.so

src/DiffEngine.Tests/InlineApplierTests.cs

@@ -188,6 +188,63 @@ public async Task LeavesNoTemporaryBehind()
         }
     }
 
+    // ReplaceFile can fail after the destination has already gone: with no backup name,
+    // ERROR_UNABLE_TO_MOVE_REPLACEMENT leaves the original deleted and the replacement under its
+    // temporary name. The temporary is then the only copy of the source there is, and deleting it
+    // on the way out lost the file outright
+    [Test]
+    public async Task AReplaceThatFailsAfterRemovingTheSourceStillLeavesOne()
+    {
+        var directory = NewDirectory();
+        try
+        {
+            var path = Path.Combine(directory, "Sample.cs");
+            await File.WriteAllTextAsync(path, "original");
+
+            InlineApplier.WriteThroughTemporary(
+                path,
+                Encoding.UTF8.GetBytes("patched"),
+                (_, destination) =>
+                {
+                    File.Delete(destination);
+                    throw new IOException("Unable to move the replacement file to the file to be replaced.");
+                });
+
+            await Assert.That(await File.ReadAllTextAsync(path)).IsEqualTo("patched");
+            await Assert.That(Directory.GetFileSystemEntries(directory)).IsEquivalentTo([path]);
+        }
+        finally
+        {
+            Directory.Delete(directory, true);
+        }
+    }
+
+    // The ordinary failure, where the swap gives up before touching either file: reported, with the
+    // source as it was and nothing left beside it
+    [Test]
+    public async Task AReplaceThatFailsCleanlyLeavesTheSourceAsItWas()
+    {
+        var directory = NewDirectory();
+        try
+        {
+            var path = Path.Combine(directory, "Sample.cs");
+            await File.WriteAllTextAsync(path, "original");
+
+            Assert.Throws<IOException>(
+                () => InlineApplier.WriteThroughTemporary(
+                    path,
+                    Encoding.UTF8.GetBytes("patched"),
+                    (_, _) => throw new IOException("The process cannot access the file.")));
+
+            await Assert.That(await File.ReadAllTextAsync(path)).IsEqualTo("original");
+            await Assert.That(Directory.GetFileSystemEntries(directory)).IsEquivalentTo([path]);
+        }
+        finally
+        {
+            Directory.Delete(directory, true);
+        }
+    }
+
     // Writing in place truncates first, so a reader - or a process that stops partway, which is
     // the case this stands in for - could see a file with its tail missing. Reading alongside the
     // apply can only fail when that window is real, so it never goes red on timing alone

src/DiffEngine.Tests/InlinePatcherTests.cs

@@ -1155,17 +1155,101 @@ public async Task RemoveWhenTheCallIsNotChained()
         await Assert.That(reason).Contains("not a chained call");
     }
 
+    /// <summary>
+    /// A verify call with no Snapshot left is what a Remove leaves behind, and what the same
+    /// Remove finds when a second framework's test process applies it. It is done, and saying so
+    /// is what stops the search going on to strip a Snapshot call from somewhere else.
+    /// </summary>
     [Test]
-    public async Task RemoveWithNoSnapshotCall()
+    public async Task RemoveWithNoSnapshotCallIsAlreadyDone()
     {
         var source = Method("        await Verify(value);");
 
+        var status = TryApply(source, 5, InlinePatchMode.Remove, null, "", out _, out _);
+
+        await Assert.That(status).IsEqualTo(PatchStatus.AlreadyApplied);
+    }
+
+    /// <summary>
+    /// Nothing at the recorded line at all, and nothing anywhere else either, is still reported.
+    /// </summary>
+    [Test]
+    public async Task RemoveWithNoCallAtAll()
+    {
+        var source = Method("        var value = 1;");
+
         var status = TryApply(source, 5, InlinePatchMode.Remove, null, "", out _, out var reason);
 
         await Assert.That(status).IsEqualTo(PatchStatus.NotFound);
         await Assert.That(reason).Contains("Could not find a Snapshot call");
     }
 
+    const string twoIdenticalSnapshots =
+        "class Tests\n{\n    async Task Test()\n    {\n        await Verify(a).Snapshot(\"dup\");\n        await Verify(b).Snapshot(\"dup\");\n    }\n}\n";
+
+    /// <summary>
+    /// A patch applied a second time - a second framework's identical patch reaching the queue
+    /// after the first was accepted. The anchor has gone from the call it named, and the content
+    /// search used to find it in the sibling instead and rewrite that one.
+    /// </summary>
+    [Test]
+    public async Task ReapplyingASetLeavesASiblingWithTheSameLiteral()
+    {
+        TryApply(twoIdenticalSnapshots, 6, InlinePatchMode.Set, "\"dup\"", "new", out var once, out _, memberName: "Test");
+        await Assert.That(once).Contains("Verify(a).Snapshot(\"dup\")");
+
+        var status = TryApply(once, 6, InlinePatchMode.Set, "\"dup\"", "new", out _, out _, memberName: "Test");
+
+        // Done, so the caller writes nothing and the sibling keeps its literal
+        await Assert.That(status).IsEqualTo(PatchStatus.AlreadyApplied);
+    }
+
+    /// <inheritdoc cref="ReapplyingASetLeavesASiblingWithTheSameLiteral"/>
+    [Test]
+    public async Task ReapplyingASetByValueLeavesASiblingWithTheSameLiteral()
+    {
+        TryApply(twoIdenticalSnapshots, 6, InlinePatchMode.Set, null, "new", out var once, out _, originalValue: "dup", memberName: "Test");
+        await Assert.That(once).Contains("Verify(a).Snapshot(\"dup\")");
+
+        var status = TryApply(once, 6, InlinePatchMode.Set, null, "new", out _, out _, originalValue: "dup", memberName: "Test");
+
+        // Done, so the caller writes nothing and the sibling keeps its literal
+        await Assert.That(status).IsEqualTo(PatchStatus.AlreadyApplied);
+    }
+
+    /// <summary>
+    /// The same for a Remove, which each framework's test process applies itself: the second one
+    /// stripped the sibling's Snapshot call.
+    /// </summary>
+    [Test]
+    public async Task ReapplyingARemoveLeavesASiblingWithTheSameLiteral()
+    {
+        TryApply(twoIdenticalSnapshots, 6, InlinePatchMode.Remove, "\"dup\"", "", out var once, out _, memberName: "Test");
+        await Assert.That(once).Contains("Verify(a).Snapshot(\"dup\")");
+
+        var status = TryApply(once, 6, InlinePatchMode.Remove, "\"dup\"", "", out _, out _, memberName: "Test");
+
+        // Done, so the caller writes nothing and the sibling keeps its literal
+        await Assert.That(status).IsEqualTo(PatchStatus.AlreadyApplied);
+    }
+
+    /// <summary>
+    /// A Snapshot call on a line of its own leaves the recorded line holding whatever followed it
+    /// once it is removed, so the statement it named ends on the line above.
+    /// </summary>
+    [Test]
+    public async Task ReapplyingARemoveOfAChainedLineLeavesASiblingWithTheSameLiteral()
+    {
+        var source = Method("        await Verify(a).Snapshot(\"dup\");\n        await Verify(b)\n            .Snapshot(\"dup\");");
+        TryApply(source, 7, InlinePatchMode.Remove, "\"dup\"", "", out var once, out _, memberName: "Test");
+        await Assert.That(once).Contains("await Verify(b);");
+
+        var status = TryApply(once, 7, InlinePatchMode.Remove, "\"dup\"", "", out _, out _, memberName: "Test");
+
+        // Done, so the caller writes nothing and the sibling keeps its literal
+        await Assert.That(status).IsEqualTo(PatchStatus.AlreadyApplied);
+    }
+
     [Test]
     public async Task TabIndentedFileUsesTabUnit()
     {

src/DiffEngine.Tests/ViewerLaunchGateTests.cs

@@ -170,6 +170,20 @@ public async Task NoSlotMeansNoViewerIsStartedAsync()
         await Assert.That(viewer.Starts).IsEqualTo(0);
     }
 
+    /// <summary>
+    /// What AddInlineAsync tells its caller about each outcome. Capped is the one that matters:
+    /// nothing was started and nothing took the patch, and reporting that as queued meant the
+    /// caller staged nothing either, so the snapshot was pending nowhere.
+    /// </summary>
+    [Test]
+    public async Task OnlyALaunchOrAHandoverIsQueued()
+    {
+        await Assert.That(DiffRunner.InlineResultFor(ViewerLaunchOutcome.Launched)).IsEqualTo(InlineResult.Queued);
+        await Assert.That(DiffRunner.InlineResultFor(ViewerLaunchOutcome.Taken)).IsEqualTo(InlineResult.Queued);
+        await Assert.That(DiffRunner.InlineResultFor(ViewerLaunchOutcome.Capped)).IsEqualTo(InlineResult.NoViewerFound);
+        await Assert.That(DiffRunner.InlineResultFor(ViewerLaunchOutcome.Failed)).IsEqualTo(InlineResult.NoViewerFound);
+    }
+
     /// <summary>
     /// A slot is spent on a window, not on a pair. So the cap is asked only once the ownership
     /// probe has said there is no window - otherwise the nineteen callers that find the one their

src/DiffEngine.Tests/ViewerLauncherTests.cs

@@ -0,0 +1,47 @@
+/// <summary>
+/// A viewer started with nowhere to draw bound the port, failed to open its window and exited, and
+/// the bind read to its launcher as a viewer that had taken the snapshot. On Linux with no display
+/// none is started, so the caller hears that no viewer was found and keeps what it sent.
+/// </summary>
+public class ViewerLauncherTests
+{
+    [Test]
+    public async Task LinuxWithNoDisplayStartsNoViewer() =>
+        await Assert.That(ViewerLauncher.HasDisplay(linux: true, Variables())).IsFalse();
+
+    [Test]
+    public async Task LinuxWithAnXDisplayStartsOne() =>
+        await Assert.That(ViewerLauncher.HasDisplay(linux: true, Variables(("DISPLAY", ":0")))).IsTrue();
+
+    [Test]
+    public async Task LinuxWithAWaylandDisplayStartsOne() =>
+        await Assert.That(ViewerLauncher.HasDisplay(linux: true, Variables(("WAYLAND_DISPLAY", "wayland-0")))).IsTrue();
+
+    /// <summary>
+    /// Set but empty is how a shell unsets a variable it cannot remove, and names no display.
+    /// </summary>
+    [Test]
+    public async Task AnEmptyDisplayIsNone() =>
+        await Assert.That(ViewerLauncher.HasDisplay(linux: true, Variables(("DISPLAY", "")))).IsFalse();
+
+    [Test]
+    public async Task OtherPlatformsAreTakenToHaveADesktop() =>
+        await Assert.That(ViewerLauncher.HasDisplay(linux: false, Variables())).IsTrue();
+
+    static Func<string, string?> Variables(params (string Name, string Value)[] set)
+    {
+        var variables = set.ToDictionary(_ => _.Name, _ => _.Value);
+
+        string? Read(string name)
+        {
+            if (variables.TryGetValue(name, out var value))
+            {
+                return value;
+            }
+
+            return null;
+        }
+
+        return Read;
+    }
+}

src/DiffEngine/DiffRunner_Inline.cs

@@ -88,9 +88,27 @@ public static async Task<InlineResult> AddInlineAsync(InlinePatch patch, Cancel
             async () => await ViewerClient.SendAsync(new(ViewerVerb.Inline, Body: payload), cancel) == SendOutcome.Accepted,
             () => ViewerLauncher.LaunchAsync(patch, payload, cancel),
             cancel);
-        return launched == ViewerLaunchOutcome.Failed ? InlineResult.NoViewerFound : InlineResult.Queued;
+        return InlineResultFor(launched);
     }
 
+    /// <summary>
+    /// Queued only where something now holds the snapshot: the viewer this call started, or an
+    /// owner that turned up while it waited at the gate.
+    /// <para>
+    /// A capped launch started nothing, and nobody was there to take the patch, so it is pending
+    /// nowhere - the same position as a viewer that could not be found, and answered the same way
+    /// so the caller stages it. Everything but Failed used to read as queued, which was right until
+    /// Capped existed and wrong from then on: with no tray running, every inline snapshot failing
+    /// after the fifth diff tool of a run was reported as handed over and staged by nobody.
+    /// </para>
+    /// </summary>
+    internal static InlineResult InlineResultFor(ViewerLaunchOutcome outcome) =>
+        outcome switch
+        {
+            ViewerLaunchOutcome.Launched or ViewerLaunchOutcome.Taken => InlineResult.Queued,
+            _ => InlineResult.NoViewerFound
+        };
+
     /// <summary>
     /// Drops a pending inline snapshot from the viewer's queue, for when a previously failing test
     /// starts passing. Does nothing when no viewer is running - and cheaply, since this is called