diff --git a/.github/workflows/build-native.yml b/.github/workflows/build-native.yml index ced50cb6..1e1d16c4 100644 --- a/.github/workflows/build-native.yml +++ b/.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) diff --git a/native/build-linux.sh b/native/build-linux.sh new file mode 100755 index 00000000..0184de62 --- /dev/null +++ b/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 diff --git a/src/DiffEngine.Tests/InlineApplierTests.cs b/src/DiffEngine.Tests/InlineApplierTests.cs index 034b01da..95403e0c 100644 --- a/src/DiffEngine.Tests/InlineApplierTests.cs +++ b/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( + () => 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 diff --git a/src/DiffEngine.Tests/InlinePatcherTests.cs b/src/DiffEngine.Tests/InlinePatcherTests.cs index 6f92ec5a..ff3a2bfc 100644 --- a/src/DiffEngine.Tests/InlinePatcherTests.cs +++ b/src/DiffEngine.Tests/InlinePatcherTests.cs @@ -1155,17 +1155,101 @@ public async Task RemoveWhenTheCallIsNotChained() await Assert.That(reason).Contains("not a chained call"); } + /// + /// 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. + /// [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); + } + + /// + /// Nothing at the recorded line at all, and nothing anywhere else either, is still reported. + /// + [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"; + + /// + /// 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. + /// + [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); + } + + /// + [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); + } + + /// + /// The same for a Remove, which each framework's test process applies itself: the second one + /// stripped the sibling's Snapshot call. + /// + [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); + } + + /// + /// 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. + /// + [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() { diff --git a/src/DiffEngine.Tests/ViewerLaunchGateTests.cs b/src/DiffEngine.Tests/ViewerLaunchGateTests.cs index f3eae6f7..ffc02490 100644 --- a/src/DiffEngine.Tests/ViewerLaunchGateTests.cs +++ b/src/DiffEngine.Tests/ViewerLaunchGateTests.cs @@ -170,6 +170,20 @@ public async Task NoSlotMeansNoViewerIsStartedAsync() await Assert.That(viewer.Starts).IsEqualTo(0); } + /// + /// 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. + /// + [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); + } + /// /// 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 diff --git a/src/DiffEngine.Tests/ViewerLauncherTests.cs b/src/DiffEngine.Tests/ViewerLauncherTests.cs new file mode 100644 index 00000000..295dc1f4 --- /dev/null +++ b/src/DiffEngine.Tests/ViewerLauncherTests.cs @@ -0,0 +1,47 @@ +/// +/// 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. +/// +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(); + + /// + /// Set but empty is how a shell unsets a variable it cannot remove, and names no display. + /// + [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 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; + } +} diff --git a/src/DiffEngine/DiffRunner_Inline.cs b/src/DiffEngine/DiffRunner_Inline.cs index 42a64f45..0d1f28c3 100644 --- a/src/DiffEngine/DiffRunner_Inline.cs +++ b/src/DiffEngine/DiffRunner_Inline.cs @@ -88,9 +88,27 @@ public static async Task 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); } + /// + /// 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. + /// + /// 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. + /// + /// + internal static InlineResult InlineResultFor(ViewerLaunchOutcome outcome) => + outcome switch + { + ViewerLaunchOutcome.Launched or ViewerLaunchOutcome.Taken => InlineResult.Queued, + _ => InlineResult.NoViewerFound + }; + /// /// 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 diff --git a/src/DiffEngine/Inline/InlineApplier.cs b/src/DiffEngine/Inline/InlineApplier.cs index 0972b2a3..4512a9a4 100644 --- a/src/DiffEngine/Inline/InlineApplier.cs +++ b/src/DiffEngine/Inline/InlineApplier.cs @@ -288,7 +288,17 @@ static void CopyMode(string destination, string temporary) } #endif - static void WriteThroughTemporary(string fullPath, byte[] output) + static void WriteThroughTemporary(string fullPath, byte[] output) => + WriteThroughTemporary(fullPath, output, static (temporary, destination) => File.Replace(temporary, destination, null)); + + /// The source file, which the patched bytes replace. + /// The whole patched file, preamble included. + /// + /// The swap. Supplied by the tests, because the failure it has to survive is one ReplaceFile + /// produces on its own schedule - an antivirus or sync client holding the file it was just + /// handed - and cannot be arranged on demand. + /// + internal static void WriteThroughTemporary(string fullPath, byte[] output, Action replace) { var directory = Path.GetDirectoryName(fullPath)!; // Named after the file it replaces, so anything left by a process that died between the @@ -300,15 +310,33 @@ static void WriteThroughTemporary(string fullPath, byte[] output) #if NET7_0_OR_GREATER CopyMode(fullPath, temporary); #endif - File.Replace(temporary, fullPath, null); + try + { + replace(temporary, fullPath); + } + catch (Exception exception) + when (!File.Exists(fullPath) && + File.Exists(temporary)) + { + // ReplaceFile can fail after it has already taken the destination away: with no + // backup name, ERROR_UNABLE_TO_MOVE_REPLACEMENT means the original no longer + // exists and the replacement is still under its temporary name. The temporary is + // then the only copy of the source anywhere, and the finally below used to delete + // it. It is the whole patched file, so finishing the swap by hand is the write + // having happened. + MoveIntoPlace(temporary, fullPath, exception); + } } finally { // Replace consumed it. Anything still there is this method's litter, and failing an - // applied patch over a temporary file that could not be deleted helps nobody + // applied patch over a temporary file that could not be deleted helps nobody - unless + // the destination is gone, when it is the source file and is left for the reader of + // the failure to find try { - if (File.Exists(temporary)) + if (File.Exists(temporary) && + File.Exists(fullPath)) { File.Delete(temporary); } @@ -320,6 +348,23 @@ static void WriteThroughTemporary(string fullPath, byte[] output) } } + static void MoveIntoPlace(string temporary, string fullPath, Exception replaceFailure) + { + try + { + File.Move(temporary, fullPath); + } + catch (Exception exception) + when (exception is IOException or UnauthorizedAccessException) + { + // Named, because this is the one failure a person has to act on: the file they edit is + // not where it was, and this is where it went + throw new IOException( + $"Replacing {fullPath} failed after the original had been removed, and the patched source could not be moved back into place. It is in {temporary}.", + new AggregateException(replaceFailure, exception)); + } + } + /// /// The encoding to read and write the file with. Every one of them throws rather than /// substituting: the applier rewrites the whole file, not just the patched span, so a diff --git a/src/DiffEngine/Inline/InlinePatcher.cs b/src/DiffEngine/Inline/InlinePatcher.cs index 0ef27c84..f5986d46 100644 --- a/src/DiffEngine/Inline/InlinePatcher.cs +++ b/src/DiffEngine/Inline/InlinePatcher.cs @@ -124,7 +124,7 @@ public static PatchStatus TryApply( if (mode == InlinePatchMode.Remove) { - return TryRemove(language, source, scan, lineStarts, lineHint, memberLine, originalExpression, originalValue, eol, ref newSource, ref failReason); + return TryRemove(language, source, scan, lineStarts, lineHint, memberLine, EntryPoints(entryPoints), originalExpression, originalValue, eol, ref newSource, ref failReason); } var fileUnit = DetectIndentUnit(source, scan, lineStarts); @@ -144,11 +144,20 @@ public static PatchStatus TryApply( // still unaccepted. // ReSharper disable once RedundantSuppressNullableWarningExpression var needle = NormalizeTo(originalExpression!, eol); - foreach (var (_, openParen) in FindCalls(source, scan, lineStarts, lineHint, memberLine, snapshotName, false)) + var appliedAtHint = false; + foreach (var (nameStart, openParen) in FindCalls(source, scan, lineStarts, lineHint, memberLine, snapshotName, false)) { + var onHint = IsOnHint(lineStarts, nameStart, lineHint); + if (appliedAtHint && + !onHint) + { + return PatchStatus.AlreadyApplied; + } + if (!TryReadArguments(source, scan, openParen, out var expected) || !expected.Matches(source, needle)) { + appliedAtHint |= onHint && HoldsContent(source, scan, openParen, newContent); continue; } @@ -174,8 +183,16 @@ public static PatchStatus TryApply( // the same outcome when nothing matches: report, rather than rewrite whichever call // the hint happens to land on. var previous = SourceLanguage.NormalizeNewlines(originalValue); - foreach (var (_, openParen) in FindCalls(source, scan, lineStarts, lineHint, memberLine, snapshotName, false)) + var appliedAtHint = false; + foreach (var (nameStart, openParen) in FindCalls(source, scan, lineStarts, lineHint, memberLine, snapshotName, false)) { + var onHint = IsOnHint(lineStarts, nameStart, lineHint); + if (appliedAtHint && + !onHint) + { + return PatchStatus.AlreadyApplied; + } + if (!TryReadArguments(source, scan, openParen, out var expected) || expected.IsAbsent || expected.BlockedByName) @@ -187,6 +204,7 @@ public static PatchStatus TryApply( if (!language.TryParse(argument, out var value) || value != previous) { + appliedAtHint |= onHint && value == newContent; continue; } @@ -206,6 +224,23 @@ public static PatchStatus TryApply( return InsertOrCheck(source, scan, lineStarts, lineHint, memberLine, newContent, eol, fileUnit, alreadyOnly: false, ref newSource, ref failReason); } + /// + /// Whether a call is on the recorded line, which yields before anything + /// else and never again. + /// + /// It matters to the content search above because a patch can arrive a second time after it + /// has been applied: a second target framework's identical patch reaching the queue after the + /// first was accepted, or each framework's test process applying the same Remove. The anchor + /// has gone from the call it named by then, so the search went looking for it elsewhere - and a + /// sibling holding the same literal, which is ordinary for a member verifying two values that + /// serialise alike, is exactly where it found it, and rewrote that one. So once the call at + /// the recorded line turns out to already hold what the patch would write, the search stops + /// there and reports it done, rather than carrying on to the next call that matches. + /// + /// + static bool IsOnHint(List lineStarts, int nameStart, int lineHint) => + LineOf(lineStarts, nameStart) == Clamp(lineHint, lineStarts.Count); + static PatchStatus InsertOrCheck( string source, SourceScan scan, @@ -502,12 +537,18 @@ static PatchStatus TryRemove( List lineStarts, int lineHint, int? memberLine, + string[] entryPoints, string? originalExpression, string? originalValue, string eol, ref string newSource, ref string failReason) { + if (RemovedAtHint(source, scan, lineStarts, lineHint, memberLine, entryPoints)) + { + return PatchStatus.AlreadyApplied; + } + var anchored = !string.IsNullOrEmpty(originalExpression) || originalValue != null; if (!TryFindAnchoredCall(language, source, scan, lineStarts, lineHint, memberLine, originalExpression, originalValue, eol, out var nameStart, out var openParen)) { @@ -566,6 +607,70 @@ static PatchStatus TryRemove( return PatchStatus.Applied; } + /// + /// Whether the Snapshot call the recorded line names has already been removed: the line holds + /// no Snapshot call, and the verify statement it belongs to has none chained onto it. + /// + /// A Remove is applied by the test process itself rather than queued, so a multi-targeted run + /// applies the same one once per framework. Every one after the first found the anchor gone + /// from the call it named and went looking for it elsewhere, and a sibling holding the same + /// literal is exactly where it found it: that snapshot was stripped instead, the way + /// describes for a Set. + /// + /// + /// The statement is the nearest verify call at or above the line, provided its chain still + /// reaches the line or the one above it. Removing a Snapshot call that had a line of its own + /// pulls the rest of its statement up onto the line above, so the recorded line then holds + /// whatever followed, and the statement it named ends just before it. + /// + /// + static bool RemovedAtHint(string source, SourceScan scan, List lineStarts, int lineHint, int? memberLine, string[] entryPoints) + { + var lineCount = lineStarts.Count; + if (lineHint < 1 || + lineHint > lineCount) + { + return false; + } + + var floor = memberLine is null ? 1 : Clamp(memberLine.Value, lineCount); + var ceiling = memberLine is null ? lineCount + 1 : NextMemberLine(source, scan, lineStarts, floor); + // A hint outside the member has gone stale, and names nothing + if (lineHint < floor || + lineHint >= ceiling) + { + return false; + } + + // A Snapshot call still on the line is one to remove, whatever it hangs off + if (CallsOnLine(source, scan, lineStarts, lineHint, snapshotName, false).Any()) + { + return false; + } + + for (var line = lineHint; line >= floor; line--) + { + var calls = CallsOnLine(source, scan, lineStarts, line, entryPoints, true).ToList(); + if (calls.Count == 0) + { + continue; + } + + // The last on the line is the nearest one above the hint + var (_, openParen) = calls[calls.Count - 1]; + if (!TryScanArguments(source, scan, openParen, out var closeParen, out _)) + { + return false; + } + + var end = WalkChain(source, scan, closeParen + 1, methodName, out var chained); + return chained < 0 && + LineOf(lineStarts, end - 1) >= lineHint - 1; + } + + return false; + } + /// /// Walks the calls chained onto an invocation and returns where a call should be appended: /// the end of the chain, or the point in front of the language's diff --git a/src/DiffEngine/Viewer/ViewerLauncher.cs b/src/DiffEngine/Viewer/ViewerLauncher.cs index f09a455b..7dc5b206 100644 --- a/src/DiffEngine/Viewer/ViewerLauncher.cs +++ b/src/DiffEngine/Viewer/ViewerLauncher.cs @@ -87,6 +87,15 @@ public static string DiffArguments(string temp, string target) => static Process? Start(string arguments, bool stdin = false) { + // With nowhere to draw, a viewer binds the port, fails to open its window and exits, and + // to whoever launched it the bind reads as a viewer that took the work. Not starting one + // tells the caller no viewer was found instead, which is the answer that has it keep what + // it sent. + if (!HasDisplay(RuntimeInformation.IsOSPlatform(OSPlatform.Linux), Environment.GetEnvironmentVariable)) + { + return null; + } + if (!DiffTools.TryFindByName(DiffTool.DiffEngineViewer, out var tool)) { return null; @@ -111,4 +120,15 @@ public static string DiffArguments(string temp, string target) => return null; } } + + /// + /// Whether a window started from this process has anywhere to go. Only Linux can be asked: an + /// SSH session or a container has neither variable, while a desktop session, a forwarded X + /// connection and WSLg each set one. Windows and macOS are taken to have a desktop, since + /// nothing in the environment says otherwise. + /// + internal static bool HasDisplay(bool linux, Func variable) => + !linux || + !string.IsNullOrEmpty(variable("DISPLAY")) || + !string.IsNullOrEmpty(variable("WAYLAND_DISPLAY")); } diff --git a/src/DiffEngineTray.Tests/OwnedInlineHostTest.cs b/src/DiffEngineTray.Tests/OwnedInlineHostTest.cs index 66c3af1f..dfd55124 100644 --- a/src/DiffEngineTray.Tests/OwnedInlineHostTest.cs +++ b/src/DiffEngineTray.Tests/OwnedInlineHostTest.cs @@ -617,8 +617,14 @@ public bool Has(string key) => return (true, "Discarded tracked"); } - public (int accepted, int kept) AcceptAll(Action? advanced = null) + /// + /// What the last sweep was told about its deletes, null before there was one. + /// + public bool? HeldDeletes { get; private set; } + + public (int accepted, int kept) AcceptAll(bool holdDeletes, Action? advanced = null) { + HeldDeletes = holdDeletes; // One step per file the sweep reports, which is what the real tracker calls it for for (var file = 0; file < SweepResult.accepted + SweepResult.kept; file++) { @@ -787,12 +793,91 @@ public async Task AnAcceptAllCountsTheFilesIntoItsProgress() var accepting = Task.Run(() => owner.Send(new(ViewerVerb.AcceptAll), TimeSpan.FromSeconds(30))); held.WaitUntilHeld(); - await Assert.That(owner.Send(new(ViewerVerb.ListFull)).Progress).IsEqualTo(new AcceptProgress(2, 3)); + // Snapshots first, so none of the files has been dealt with while one is applying, and + // the total still counts them + await Assert.That(owner.Send(new(ViewerVerb.ListFull)).Progress).IsEqualTo(new AcceptProgress(0, 3)); held.Release(); await Assert.That((await accepting).Message).IsEqualTo("Accepted 1, plus 2 files"); } + /// + /// A snapshot moving inline arrives as a patch plus a delete of the verified file it replaces. + /// The files went first, so a patch refused after them had already cost its snapshot the file: + /// in neither place. Now the patches go first, and a refusal holds the deletes. + /// + [Test] + public async Task AnAcceptAllWhosePatchIsRefusedHoldsTheDeletes() + { + using var owner = new Owner(_ => InlineApplyResult.NotFound("The source changed since the test run.")); + var tracked = new FakeTracked + { + DeleteList = [new(@"delete:c:\code\b.verified.txt", "b.verified.txt", null, @"c:\code\b.verified.txt")], + SweepResult = (0, 1) + }; + owner.Host.TrackedFiles = tracked; + owner.Queue(); + + var response = owner.Send(new(ViewerVerb.AcceptAll), TimeSpan.FromSeconds(30)); + + await Assert.That(tracked.HeldDeletes).IsTrue(); + await Assert.That(response.Message).Contains(Tracker.DeletesHeld); + } + + /// + /// Every patch written, nothing to protect: the deletes go ahead. + /// + [Test] + public async Task AnAcceptAllWhosePatchesAllLandCarriesOutTheDeletes() + { + using var owner = new Owner(_ => InlineApplyResult.Applied); + var tracked = new FakeTracked + { + DeleteList = [new(@"delete:c:\code\b.verified.txt", "b.verified.txt", null, @"c:\code\b.verified.txt")], + SweepResult = (1, 0) + }; + owner.Host.TrackedFiles = tracked; + owner.Queue(); + + var response = owner.Send(new(ViewerVerb.AcceptAll), TimeSpan.FromSeconds(30)); + + await Assert.That(tracked.HeldDeletes).IsFalse(); + await Assert.That(response.Message).IsEqualTo("Accepted 1, plus 1 files"); + } + + /// + /// An accept-all runs for as long as the queue is long, and the queue moves meanwhile. It + /// applied from a copy taken at the start, so an entry discarded while an earlier one was + /// applying was still written into the source. + /// + [Test] + public async Task AnEntryDiscardedDuringAnAcceptAllIsNotWritten() + { + using var held = new HeldApply(1); + var applied = new List(); + using var owner = new Owner( + patch => + { + lock (applied) + { + applied.Add(patch); + } + + return held.Apply(patch); + }); + owner.Queue(line: 1); + owner.Queue(line: 2); + + var accepting = Task.Run(() => owner.Send(new(ViewerVerb.AcceptAll), TimeSpan.FromSeconds(30))); + held.WaitUntilHeld(); + owner.Send(new(ViewerVerb.Discard, InlineKey.For(@"c:\repo\SampleTests.cs", 2))); + held.Release(); + await accepting; + + await Assert.That(applied).HasSingleItem(); + await Assert.That(applied[0].LineHint).IsEqualTo(1); + } + /// /// The menu's accept-all reaches the queue through the tray's own host rather than the wire, /// and a viewer displaying the queue follows that one the same way. @@ -804,7 +889,7 @@ public async Task TheMenusAcceptAllReportsProgressToo() using var owner = new Owner(held.Apply); owner.Queue(); - var accepting = Task.Run(() => owner.Host.AcceptAll(out _)); + var accepting = Task.Run(() => owner.Host.AcceptAll(out _, out _)); held.WaitUntilHeld(); await Assert.That(owner.Send(new(ViewerVerb.ListFull)).Progress).IsEqualTo(new AcceptProgress(0, 1)); diff --git a/src/DiffEngineTray.Tests/StubInlineHost.cs b/src/DiffEngineTray.Tests/StubInlineHost.cs index 852a1cd1..d8793ec4 100644 --- a/src/DiffEngineTray.Tests/StubInlineHost.cs +++ b/src/DiffEngineTray.Tests/StubInlineHost.cs @@ -53,9 +53,22 @@ public bool Discard(PendingSnapshot snapshot, out string? message) public string? AcceptAllMessage { get; init; } - public bool AcceptAll(out string? message) + /// + /// Whether the sweep reports a snapshot it could not write, which is what holds the tray's + /// pending deletes back. + /// + public bool AcceptAllRefuses { get; init; } + + /// + /// Run as the sweep starts, so a test can look at what else has or has not happened by then. + /// + public Action? AcceptingAll { get; init; } + + public bool AcceptAll(out string? message, out bool refused) { + AcceptingAll?.Invoke(); message = AcceptAllMessage; + refused = AcceptAllRefuses; return AcceptAllSucceeds; } diff --git a/src/DiffEngineTray.Tests/TrackerDeleteTest.cs b/src/DiffEngineTray.Tests/TrackerDeleteTest.cs index 8e8a60d9..8e98ab29 100644 --- a/src/DiffEngineTray.Tests/TrackerDeleteTest.cs +++ b/src/DiffEngineTray.Tests/TrackerDeleteTest.cs @@ -122,6 +122,54 @@ public async Task AcceptAllContinuesPastAnUndeletableFile() await Assert.That(tracker.Deletes).HasSingleItem(); } + /// + /// A snapshot moving inline arrives as a patch plus a delete of the verified file it replaces, + /// and "Accept all" used to delete first. The snapshots go first now, while that file is still + /// there to fall back on. + /// + [Test] + public async Task AcceptAllAppliesTheSnapshotsBeforeTheDeletes() + { + bool? existedWhileAccepting = null; + await using var tracker = new RecordingTracker( + inline: new StubInlineHost(new PendingSnapshot(@"c:\repo\sample.cs|12", "Sample.cs:12", null)) + { + AcceptingAll = () => existedWhileAccepting = File.Exists(file1) + }); + tracker.AddDelete(file1); + + await tracker.AcceptAll(); + + await Assert.That(existedWhileAccepting).IsTrue(); + await Assert.That(File.Exists(file1)).IsFalse(); + } + + /// + /// A patch was refused, so the file a delete would remove may be the only copy of that snapshot + /// left. The delete stays pending, the file stays where it is, and the balloon says why. + /// + [Test] + public async Task AcceptAllHoldsTheDeletesWhenASnapshotWasNotWritten() + { + var warnings = new List(); + await using var tracker = new RecordingTracker( + inlineFailed: warnings.Add, + inline: new StubInlineHost(new PendingSnapshot(@"c:\repo\sample.cs|12", "Sample.cs:12", null)) + { + AcceptAllSucceeds = false, + AcceptAllRefuses = true, + AcceptAllMessage = "Accepted 0, 1 not written" + }); + tracker.AddDelete(file1); + + await tracker.AcceptAll(); + + await Assert.That(File.Exists(file1)).IsTrue(); + await Assert.That(tracker.Deletes).HasSingleItem(); + await Assert.That(warnings).IsEquivalentTo( + [$"Could not accept the pending snapshots. Accepted 0, 1 not written {Tracker.DeletesHeld}"]); + } + public void Dispose() { File.Delete(file1); diff --git a/src/DiffEngineTray.Tests/TrackerTrackedFilesTest.cs b/src/DiffEngineTray.Tests/TrackerTrackedFilesTest.cs index deae12ea..44fb92ac 100644 --- a/src/DiffEngineTray.Tests/TrackerTrackedFilesTest.cs +++ b/src/DiffEngineTray.Tests/TrackerTrackedFilesTest.cs @@ -153,7 +153,7 @@ public async Task AcceptAllSweepsAndCountsWhatStayed() await File.WriteAllTextAsync(temp, "content"); tracker.AddMove(temp, target, null, null, false, null); - var (accepted, kept) = tracked.AcceptAll(); + var (accepted, kept) = tracked.AcceptAll(holdDeletes: false); await Assert.That(accepted).IsEqualTo(2); await Assert.That(kept).IsEqualTo(0); @@ -161,6 +161,29 @@ public async Task AcceptAllSweepsAndCountsWhatStayed() await Assert.That(await File.ReadAllTextAsync(target)).IsEqualTo("content"); } + /// + /// A snapshot swept alongside was not written, so the file a delete would remove may be the + /// only copy of it left. The delete stays pending, and the file stays where it is; a move is + /// the snapshot arriving rather than the last copy leaving, so it goes ahead. + /// + [Test] + public async Task AcceptAllHoldingDeletesLeavesThemPending() + { + await using var tracker = new RecordingTracker(); + ITrackedFiles tracked = tracker; + tracker.AddDelete(file); + await File.WriteAllTextAsync(temp, "content"); + tracker.AddMove(temp, target, null, null, false, null); + + var (accepted, kept) = tracked.AcceptAll(holdDeletes: true); + + await Assert.That(accepted).IsEqualTo(1); + await Assert.That(kept).IsEqualTo(1); + await Assert.That(File.Exists(file)).IsTrue(); + await Assert.That(tracker.Deletes).HasSingleItem(); + await Assert.That(await File.ReadAllTextAsync(target)).IsEqualTo("content"); + } + [Test] public async Task DiscardAllUntracksDeletesAndDropsMoveTemps() { diff --git a/src/DiffEngineTray.Tests/TrayViewerSyncTest.cs b/src/DiffEngineTray.Tests/TrayViewerSyncTest.cs index df69ce9d..30c9ee24 100644 --- a/src/DiffEngineTray.Tests/TrayViewerSyncTest.cs +++ b/src/DiffEngineTray.Tests/TrayViewerSyncTest.cs @@ -535,6 +535,42 @@ public async Task TrayAcceptAllReportsWhatTheOwningViewerKept() await Assert.That(pair.Failures.Single()).Contains("the file is locked"); } + /// + /// A snapshot moving inline while a viewer holds the queue sends its patch there and the + /// delete of its verified file here. The tray reads what the batch refused back out of the + /// viewer's listing, and holds its delete rather than removing the file under a patch that + /// was never written. + /// + [Test] + public async Task TrayAcceptAllHoldsItsDeletesWhenTheOwningViewerRefusedAPatch() + { + await using var pair = new ViewerOwned(_ => ViewerSideApplyResult.NotFound("The source changed since the test run.")); + pair.Queue(sample, 1); + var stale = pair.StageStaleFile(); + pair.Tracker.AddDelete(stale); + + await pair.Tracker.AcceptAll(); + + await Assert.That(File.Exists(stale)).IsTrue(); + await Assert.That(pair.Tracker.Deletes).HasSingleItem(); + await Assert.That(pair.Failures.Single()).Contains(Tracker.DeletesHeld); + } + + /// + [Test] + public async Task TrayAcceptAllCarriesOutItsDeletesWhenTheOwningViewerWroteEveryPatch() + { + await using var pair = new ViewerOwned(); + pair.Queue(sample, 1); + var stale = pair.StageStaleFile(); + pair.Tracker.AddDelete(stale); + + await pair.Tracker.AcceptAll(); + + await Assert.That(File.Exists(stale)).IsFalse(); + await Assert.That(pair.Tracker.Deletes).IsEmpty(); + } + /// /// An owning viewer applies inside its session, and InlineApplier waits up to ten seconds on /// its cross process mutex, so an accept legitimately outlasts the wait the listing verbs use. diff --git a/src/DiffEngineTray/IInlineHost.cs b/src/DiffEngineTray/IInlineHost.cs index ca9c5156..73581358 100644 --- a/src/DiffEngineTray/IInlineHost.cs +++ b/src/DiffEngineTray/IInlineHost.cs @@ -25,7 +25,18 @@ interface IInlineHost AcceptOutcome Accept(PendingSnapshot snapshot, out string? message); bool Discard(PendingSnapshot snapshot, out string? message); - bool AcceptAll(out string? message); + + /// + /// True when nothing is pending afterwards. + /// + /// What the owner said the sweep did. + /// + /// A snapshot this sweep tried was not written, or the owner could not be asked. What holds + /// the tray's pending deletes back, since a snapshot moving inline arrives as a patch plus a + /// delete of the verified file it replaces, and that file may be the only copy of it left. + /// + bool AcceptAll(out string? message, out bool refused); + /// /// False when the queue owner could not be asked, so a caller clearing its own state knows not /// to. diff --git a/src/DiffEngineTray/ITrackedFiles.cs b/src/DiffEngineTray/ITrackedFiles.cs index a296d852..a7ae565e 100644 --- a/src/DiffEngineTray/ITrackedFiles.cs +++ b/src/DiffEngineTray/ITrackedFiles.cs @@ -26,14 +26,18 @@ interface ITrackedFiles (bool ok, string? message) Discard(string key); /// - /// Accept every tracked delete and move without prompting. Kept is what stayed pending — - /// locked moves, undeletable files. + /// Accept every tracked move and delete without prompting. Kept is what stayed pending — + /// locked moves, undeletable files, and deletes held back. /// + /// + /// Leave every delete pending rather than carrying it out, because a snapshot swept alongside + /// was not written, and the file a delete removes may be the only copy of it left. + /// /// /// Called as each file is dealt with, whichever way it went, so the owner can say how far an /// accept-all has got while a locked move is still being retried. /// - (int accepted, int kept) AcceptAll(Action? advanced = null); + (int accepted, int kept) AcceptAll(bool holdDeletes, Action? advanced = null); /// /// Track a pending move or delete that arrived over the viewer port rather than the piper one. diff --git a/src/DiffEngineTray/OwnedInlineHost.cs b/src/DiffEngineTray/OwnedInlineHost.cs index 5a173739..f17df644 100644 --- a/src/DiffEngineTray/OwnedInlineHost.cs +++ b/src/DiffEngineTray/OwnedInlineHost.cs @@ -145,14 +145,14 @@ public bool Discard(PendingSnapshot snapshot, out string? message) } } - public bool AcceptAll(out string? message) + public bool AcceptAll(out string? message, out bool refused) { lock (accepting) { StartProgress(0); try { - message = AcceptEvery(); + message = AcceptEvery(0, out refused); } finally { @@ -357,9 +357,17 @@ bool IQueueOwner.Has(string key) /// /// The wire's accept-all sweeps everything this owner shows a viewer: tracked deletes and - /// moves as well as the snapshots, mirroring the tray menu's own "Accept all". Files first, - /// the order that menu has always used, and never through , - /// whose snapshot half would re-enter this host and whose move path can prompt. + /// moves as well as the snapshots, mirroring the tray menu's own "Accept all". Never through + /// , whose snapshot half would re-enter this host and whose move + /// path can prompt. + /// + /// Snapshots first, and the deletes held when one of them was not written. A snapshot moving + /// inline arrives as two unrelated entries - the patch that writes the literal, and a delete + /// of the verified file it replaces - and files first, the order this used to take, deleted + /// that file before finding out the patch would be refused: the snapshot lost both copies at + /// once. The viewer's own batch has always run this way round, for that reason; this is the + /// same rule for the arrangement where the tray holds the queue, which is the usual one. + /// /// /// The files count towards the progress a listing reports, since a move that is being retried /// while a diff tool lets go of it is as much of the wait as any snapshot. @@ -369,14 +377,17 @@ string IQueueOwner.AcceptAll() { (int accepted, int kept)? tracked; string message; + var held = false; lock (accepting) { - var files = TrackedFiles is { } trackedFiles ? trackedFiles.Moves().Count + trackedFiles.Deletes().Count : 0; - StartProgress(files); + var moves = TrackedFiles?.Moves().Count ?? 0; + var deletes = TrackedFiles?.Deletes().Count ?? 0; + StartProgress(moves + deletes); try { - tracked = TrackedFiles?.AcceptAll(Advance); - message = AcceptEvery(); + message = AcceptEvery(moves + deletes, out var refused); + tracked = TrackedFiles?.AcceptAll(refused, Advance); + held = refused && deletes > 0; } finally { @@ -394,6 +405,11 @@ string IQueueOwner.AcceptAll() var clause = swept.kept == 0 ? $"{swept.accepted} files" : $"{swept.accepted} files ({swept.kept} kept)"; + if (held) + { + return $"{message}, plus {clause}. {Tracker.DeletesHeld}"; + } + return $"{message}, plus {clause}"; } @@ -510,34 +526,63 @@ void IQueueOwner.Window(WindowCommand command, string? key) /// /// Every snapshot pending when it starts, applied outside the gate and completed one at a - /// time. The list is immutable, so applying over it is safe, and each completion skips an - /// entry that changed underneath it. Conflicted entries are never applied: they are counted - /// into the message at the end. + /// time. Conflicted entries are never applied: they are counted into the message at the end. + /// + /// Taken as keys, and each looked up again when its turn comes, rather than applied from a + /// copy of the queue taken at the start. A batch holds an apply per entry, each of which can + /// wait on InlineApplier's mutex, and the queue does not stand still meanwhile: a test that + /// started passing settles its entry, a discard empties the queue, a re-run replaces a patch, + /// a second framework makes a conflict of one. Applying from the copy wrote every one of those + /// into the source regardless - the old failing content over a test that now passed, snapshots + /// just discarded - and then ignored the outcome because the entry had changed. The viewer's + /// batch claims its entries the same way. + /// /// /// Completed as each lands rather than all together at the end, so a displaying viewer's next /// listing shows the queue shrinking and says how far the batch has got. Together they left /// the window showing an untouched queue for as long as the batch took. /// /// - string AcceptEvery() + /// + /// The tracked files the caller sweeps once the snapshots are done, for the progress total. + /// + /// + /// A snapshot in this batch was not written, which is what holds the deletes swept after it. + /// + string AcceptEvery(int files, out bool refused) { - List pending; + List keys; lock (gate) { - pending = queue.Items + keys = queue.Items .Where(_ => !_.Conflicted) + .Select(_ => _.Key) .ToList(); - // Exact now, where the start could only estimate it: the files swept first gave - // snapshots time to arrive or settle + // Exact now, where the start could only estimate it: anything can arrive or settle + // between the two if (progress is not null) { - progress = progress with { Total = progress.Done + pending.Count }; + progress = progress with { Total = progress.Done + keys.Count + files }; } } var tally = new AcceptAllTally(); - foreach (var entry in pending) + foreach (var key in keys) { + PendingInline? entry; + lock (gate) + { + entry = queue.Find(key); + if (entry is null || + entry.Conflicted) + { + // Settled, discarded or made a conflict of since the batch began. Nothing to + // apply, and one fewer to wait for + progress = progress?.Advance(); + continue; + } + } + var result = applier(entry.Patch); // Together, so no listing can show the entry gone and the count not yet moved past it lock (gate) @@ -551,6 +596,7 @@ string AcceptEvery() lock (gate) { + refused = tally.Refused; return tally.Message(queue.Conflicts); } } diff --git a/src/DiffEngineTray/RemoteInlineHost.cs b/src/DiffEngineTray/RemoteInlineHost.cs index 035bf618..092d0757 100644 --- a/src/DiffEngineTray/RemoteInlineHost.cs +++ b/src/DiffEngineTray/RemoteInlineHost.cs @@ -124,10 +124,31 @@ public bool Discard(PendingSnapshot snapshot, out string? message) => /// True only when the queue is empty afterwards, for the reason gives — /// and matching what an owning tray reports, which is also "is anything still pending". A /// conflict counts as not accepted, which is right: it is what a reviewer still has to resolve. + /// + /// Refused is read back the same way, out of the full listing that follows, since the wire + /// carries a message rather than a tally. The owner keeps an entry it could not write and says + /// why on it, while one that arrived during the batch carries nothing and a conflict is never + /// tried - so a non-conflicted entry with a status is one this batch refused. An owner that + /// could not be asked, before or after, counts as refused: what waits on the answer is a + /// delete, and a delete is the one thing not safe to guess about. + /// /// - public bool AcceptAll(out string? message) => - Send(ViewerVerb.AcceptAll, null, acceptAllWait, out message) && - List().Count == 0; + public bool AcceptAll(out string? message, out bool refused) + { + if (!Send(ViewerVerb.AcceptAll, null, acceptAllWait, out message) || + !Exchange(new(ViewerVerb.ListFull), ViewerClient.ShortTimeout, out var response) || + !response.Ok) + { + refused = true; + return false; + } + + // A full listing lists a conflicted entry's other variants, and an entry has them exactly + // when it is conflicted + refused = response.Items.Any(_ => _.Variants.Count == 0 && + _.Status is not null); + return response.Items.Count == 0; + } /// /// As , and the outcome is returned rather than dropped. Discarded on a diff --git a/src/DiffEngineTray/Tracker.cs b/src/DiffEngineTray/Tracker.cs index 7b20899f..d4a853f8 100644 --- a/src/DiffEngineTray/Tracker.cs +++ b/src/DiffEngineTray/Tracker.cs @@ -345,18 +345,49 @@ public Task AcceptAllSnapshots() => { 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) + SweepSnapshots(out var failure); + if (failure is not null) { - return; + inlineFailed?.Invoke(failure); } - if (!inline.AcceptAll(out var message)) + Refresh(); + } + catch (Exception exception) + { + ExceptionHandler.Handle("Failed to accept the pending snapshots", exception); + } + }); + + /// + /// The second half of an accept-all: the snapshots, then the deletes, on a worker for the + /// reason gives. + /// + /// Deletes after the snapshots, and not at all when one of those was not written. A snapshot + /// moving inline arrives as a patch plus a delete of the verified file it replaces, and nothing + /// ties the two together. Deleting first, which is what this used to do, removed that file + /// before finding out the patch would be refused, so the snapshot was in neither place: not in + /// the source, and not on disk. The viewer's own accept-all has always held its deletes this + /// way, for the same reason. + /// + /// + Task AcceptSnapshotsThenDeletes() => + Task.Run(() => + { + try + { + if (!SweepSnapshots(out var failure)) { - inlineFailed?.Invoke($"Could not accept the pending snapshots. {message}"); + AcceptAllDeletes(); + } + else if (!deletes.IsEmpty) + { + failure = failure is null ? DeletesHeld : $"{failure} {DeletesHeld}"; + } + + if (failure is not null) + { + inlineFailed?.Invoke(failure); } Refresh(); @@ -367,6 +398,35 @@ public Task AcceptAllSnapshots() => } }); + /// + /// What a user is told about the deletes an accept-all left pending, from either surface. + /// + public const string DeletesHeld = "Pending deletes were kept, since a snapshot in this batch was not written and a file being deleted may be the only copy of it left. Accept them on their own to delete them anyway."; + + /// + /// Accepts every pending snapshot, and returns whether one it tried was not written. + /// + /// What to tell the user, when something is still pending afterwards. + bool SweepSnapshots(out string? failure) + { + failure = null; + // 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 false; + } + + if (!inline.AcceptAll(out var message, out var refused)) + { + failure = $"Could not accept the pending snapshots. {message}"; + } + + return refused; + } + /// /// Accepts just these snapshots, for a group header: unlike , /// solution A's header must not accept solution B's queue. @@ -720,14 +780,14 @@ public void Clear() } /// - /// The returned task covers the snapshot half, which runs on a worker for the reason - /// gives. The menu and the hot keys discard it; tests - /// await it so what the other surface should now be showing is settled rather than in flight. + /// The moves here, on the calling thread, because a locked one can prompt. The returned task + /// covers the rest - the snapshots, then the deletes, which have to wait for them - and runs + /// on a worker for the reason gives. The menu and the + /// hot keys discard it; tests await it so what the other surface should now be showing is + /// settled rather than in flight. /// public Task AcceptOpen() { - AcceptAllDeletes(); - AcceptMoves( moves.Values .Where(_ => _.IsOpen) @@ -735,24 +795,22 @@ public Task AcceptOpen() // Every pending snapshot is open by definition: the viewer only stays running while it // has something to show. - return AcceptAllSnapshots(); + return AcceptSnapshotsThenDeletes(); } /// public Task AcceptAll() { - AcceptAllDeletes(); - AcceptMoves(moves.Values); - return AcceptAllSnapshots(); + return AcceptSnapshotsThenDeletes(); } void AcceptAllDeletes() { // One at a time, and no Clear afterwards: a delete that fails re-tracks itself, and - // clearing would throw that away. Unguarded, the first bad one also took AcceptMoves and - // AcceptAllSnapshots with it, so "Accept all" stopped at the first read-only file + // clearing would throw that away. Unguarded, the first bad one also took the rest of the + // sweep with it, so "Accept all" stopped at the first read-only file foreach (var delete in deletes.Values.ToList()) { Accept(delete); @@ -866,13 +924,13 @@ bool ITrackedFiles.Untrack(string key) return (false, null); } - (int accepted, int kept) ITrackedFiles.AcceptAll(Action? advanced) + (int accepted, int kept) ITrackedFiles.AcceptAll(bool holdDeletes, Action? advanced) { var accepted = 0; var kept = 0; - foreach (var delete in deletes.Values.ToList()) + foreach (var move in moves.Values.ToList()) { - if (AcceptTracked(delete).ok) + if (AcceptWithoutPrompting(move).ok) { accepted++; } @@ -884,9 +942,12 @@ bool ITrackedFiles.Untrack(string key) advanced?.Invoke(); } - foreach (var move in moves.Values.ToList()) + foreach (var delete in deletes.Values.ToList()) { - if (AcceptWithoutPrompting(move).ok) + // Held rather than tried, and left tracked, so it can still be accepted on its own by + // anyone who knows the file is redundant + if (!holdDeletes && + AcceptTracked(delete).ok) { accepted++; } diff --git a/src/DiffEngineViewer.Linux/runtimes/linux-arm64/native/libdiffengine_viewer.so b/src/DiffEngineViewer.Linux/runtimes/linux-arm64/native/libdiffengine_viewer.so index 79c8d649..4453961b 100644 Binary files a/src/DiffEngineViewer.Linux/runtimes/linux-arm64/native/libdiffengine_viewer.so and b/src/DiffEngineViewer.Linux/runtimes/linux-arm64/native/libdiffengine_viewer.so differ diff --git a/src/DiffEngineViewer.Linux/runtimes/linux-x64/native/libdiffengine_viewer.so b/src/DiffEngineViewer.Linux/runtimes/linux-x64/native/libdiffengine_viewer.so index 183f04e5..f4e18c99 100644 Binary files a/src/DiffEngineViewer.Linux/runtimes/linux-x64/native/libdiffengine_viewer.so and b/src/DiffEngineViewer.Linux/runtimes/linux-x64/native/libdiffengine_viewer.so differ diff --git a/src/DiffEngineViewer.Tests/IpcTests.cs b/src/DiffEngineViewer.Tests/IpcTests.cs index ccc8e8dd..d67a607a 100644 --- a/src/DiffEngineViewer.Tests/IpcTests.cs +++ b/src/DiffEngineViewer.Tests/IpcTests.cs @@ -48,6 +48,27 @@ public async Task InlineWithoutABodyIsRejected() await Assert.That(response.Ok).IsFalse(); } + /// + /// A viewer on its way out refuses rather than answering. It used to acknowledge a patch or a + /// pair and then exit with it, so the sender believed it queued and staged nothing, and a + /// pending file was in no window and no tray. + /// + [Test] + public async Task AClosingViewerRefusesWhatWouldJoinTheQueue() + { + using var fixture = new ServerFixture(); + fixture.Host.Mutate(_ => _ with { Closing = true }); + + var inline = fixture.Send(Inline(Fixtures.Patch())); + var diff = fixture.Send(new(ViewerVerb.Diff, "temp/sample.received.txt", "code/sample.verified.txt")); + var delete = fixture.Send(new(ViewerVerb.Delete, "code/extra.verified.txt")); + + await Assert.That(inline.Ok).IsFalse(); + await Assert.That(diff.Ok).IsFalse(); + await Assert.That(delete.Ok).IsFalse(); + await Assert.That(fixture.Host.State.Queue).IsEmpty(); + } + [Test] public async Task SettleDropsTheEntry() { diff --git a/src/DiffEngineViewer.Tests/ViewerProgramTests.cs b/src/DiffEngineViewer.Tests/ViewerProgramTests.cs index 040652f3..64584d3f 100644 --- a/src/DiffEngineViewer.Tests/ViewerProgramTests.cs +++ b/src/DiffEngineViewer.Tests/ViewerProgramTests.cs @@ -35,6 +35,85 @@ public async Task AnAttachedViewerPersistsNothing() await Assert.That(project.StagedFiles()).IsEmpty(); } + /// + /// The port is bound before the window is asked for, so whoever launched the viewer was told + /// what it sent had been taken. A window that could not open - no display, a native library + /// that would not load - returned before anything was written, and the snapshot existed + /// nowhere. + /// + [Test] + public async Task AViewerWithNoWindowStillStagesWhatItHolds() + { + using var project = new TempProject(); + var source = project.Source("SampleTests.cs"); + var state = Fixtures.Inline(Fixtures.Patch(source: source, framework: "net10.0")); + + var code = ViewerProgram.Run(new(state), server: null, link: null, NoWindow); + + await Assert.That(code).IsEqualTo(4); + await Assert.That(project.StagedFiles().Count(_ => _.EndsWith(".inlinepatch"))).IsEqualTo(1); + } + + /// + /// A loop that throws ends the way one that returns does. The throw used to unwind straight to + /// Main's catch, past the persist, and the queue went with the process. + /// + [Test] + public async Task AViewerWhoseLoopThrowsStillStagesWhatItHolds() + { + using var project = new TempProject(); + var source = project.Source("SampleTests.cs"); + var state = Fixtures.Inline(Fixtures.Patch(source: source, framework: "net10.0")); + + Assert.Throws( + () => + { + ViewerProgram.Run(new(state), server: null, link: null, ThrowingWindow.Open); + }); + + await Assert.That(project.StagedFiles().Count(_ => _.EndsWith(".inlinepatch"))).IsEqualTo(1); + } + + static IViewerWindow? NoWindow(string title, int width, int height, bool hidden, out string? error) + { + error = "No display."; + return null; + } + + sealed class ThrowingWindow : IViewerWindow + { + public static IViewerWindow? Open(string title, int width, int height, bool hidden, out string? error) + { + error = null; + return new ThrowingWindow(); + } + + public bool Present(Screen screen) => + throw new InvalidOperationException("The renderer failed."); + + public ViewerInput Poll() => + default; + + public void SetHidden(bool hidden) + { + } + + public void Focus() + { + } + + public void SetClipboard(string text) + { + } + + public bool Capture(Screen screen, int width, int height, string pngPath) => + false; + + public void Dispose() + { + } + } + sealed class TempProject : IDisposable { readonly string directory = Path.Combine( diff --git a/src/DiffEngineViewer.Tests/ViewerSessionTests.cs b/src/DiffEngineViewer.Tests/ViewerSessionTests.cs index b6e70524..afc37b5c 100644 --- a/src/DiffEngineViewer.Tests/ViewerSessionTests.cs +++ b/src/DiffEngineViewer.Tests/ViewerSessionTests.cs @@ -780,6 +780,56 @@ public async Task AQueueChangeClosesTheMenu() await Assert.That(synced.Menu).IsNull(); } + /// + /// A settle that empties the queue sets Exit, and the loop acts on it a frame later. An arrival + /// in between is a reason to stay: carried across, Exit took the new entry out with the window. + /// + [Test] + public async Task AnArrivalAfterTheQueueEmptiedKeepsTheWindow() + { + var state = Fixtures.Inline(Fixtures.Patch()); + var settled = ViewerSession.Settle(state, state.Queue[0].Key); + await Assert.That(settled.Exit).IsTrue(); + + var inline = ViewerSession.EnqueueInline(settled, Fixtures.Patch("OtherTests.cs", 7)); + var tracked = ViewerSession.EnqueueTracked(settled, Fixtures.Move()); + + await Assert.That(inline.Queue).HasSingleItem(); + await Assert.That(inline.Exit).IsFalse(); + await Assert.That(tracked.Queue).HasSingleItem(); + await Assert.That(tracked.Exit).IsFalse(); + } + + /// + /// Once the loop has committed to leaving, nothing joins the queue: the handler answering the + /// wire sees the state come back unchanged and refuses, rather than acknowledging something + /// that is about to leave with the process. + /// + [Test] + public async Task NothingJoinsAQueueThatHasCommittedToLeaving() + { + var state = Fixtures.Inline(Fixtures.Patch()); + var closing = ViewerSession.CommitExit(ViewerSession.Settle(state, state.Queue[0].Key)); + await Assert.That(closing.Closing).IsTrue(); + + await Assert.That(ViewerSession.EnqueueInline(closing, Fixtures.Patch("OtherTests.cs", 7))).IsSameReferenceAs(closing); + await Assert.That(ViewerSession.EnqueueTracked(closing, Fixtures.Move())).IsSameReferenceAs(closing); + } + + /// + /// Only an Exit that is still standing commits: one an arrival has already cleared leaves the + /// window open. + /// + [Test] + public async Task AnArrivalBeforeTheCommitCancelsIt() + { + var state = Fixtures.Inline(Fixtures.Patch()); + var settled = ViewerSession.Settle(state, state.Queue[0].Key); + var arrived = ViewerSession.EnqueueInline(settled, Fixtures.Patch("OtherTests.cs", 7)); + + await Assert.That(ViewerSession.CommitExit(arrived).Closing).IsFalse(); + } + /// /// Owner-mode operations rebuild the inline queue from the display list, and tracked entries /// must never leak into it. diff --git a/src/DiffEngineViewer/Ipc/MessageHandler.cs b/src/DiffEngineViewer/Ipc/MessageHandler.cs index 11017b39..a4809719 100644 --- a/src/DiffEngineViewer/Ipc/MessageHandler.cs +++ b/src/DiffEngineViewer/Ipc/MessageHandler.cs @@ -25,8 +25,9 @@ int IQueueOwner.Enqueue(InlinePatch patch) // Inline entries only, which is what a tray owner counts. This queue also holds tracked // moves and deletes, so counting all of it had the two owners answering the same verb // with different numbers - var count = host - .Mutate(_ => ViewerSession.EnqueueInline(_, patch)) + var state = host.Mutate(_ => ViewerSession.EnqueueInline(_, patch)); + RefuseWhenClosing(state); + var count = state .Queue .Count(_ => _.Kind == QueueEntryKind.Inline); // Brought forward on the entry that arrived, which is what a tray owner does with one of @@ -42,13 +43,35 @@ void IQueueOwner.Settle(string key, string? origin, string? member) => /// /// The files are read here, on the listener thread, so the session stays IO free — the same - /// seam materializes the tray's tracked files through. + /// seam materializes the tray's tracked files through. Before the lock + /// rather than inside it: building an entry reads both files and diffs them, and the render + /// loop takes the same lock every frame. /// - void IQueueOwner.TrackMove(string temp, string target) => - host.Mutate(_ => ViewerSession.EnqueueTracked(_, TrackedEntry.ForMove(temp, target))); + void IQueueOwner.TrackMove(string temp, string target) + { + var entry = TrackedEntry.ForMove(temp, target); + RefuseWhenClosing(host.Mutate(_ => ViewerSession.EnqueueTracked(_, entry))); + } + + void IQueueOwner.TrackDelete(string file) + { + var entry = TrackedEntry.ForDelete(file); + RefuseWhenClosing(host.Mutate(_ => ViewerSession.EnqueueTracked(_, entry))); + } - void IQueueOwner.TrackDelete(string file) => - host.Mutate(_ => ViewerSession.EnqueueTracked(_, TrackedEntry.ForDelete(file))); + /// + /// Thrown rather than returned, because has no refusal to return for + /// these verbs, and a throwing handler is answered with an error: the sender then stages or + /// relaunches instead of believing a window that is on its way out took what it sent. See + /// . + /// + static void RefuseWhenClosing(SessionState state) + { + if (state.Closing) + { + throw new InvalidOperationException("This viewer is closing and can take nothing more. Send it again once it has gone."); + } + } /// /// With patches, each item carries the payloads it was queued from — every variant of it — diff --git a/src/DiffEngineViewer/SessionState.cs b/src/DiffEngineViewer/SessionState.cs index 6b28ba9b..3fbb486f 100644 --- a/src/DiffEngineViewer/SessionState.cs +++ b/src/DiffEngineViewer/SessionState.cs @@ -24,6 +24,17 @@ record SessionState( /// public bool QuitRequested { get; init; } + /// + /// The loop has committed to leaving, so nothing more may join the queue. Set under the host's + /// lock, which is the point of it: a window leaving because its queue emptied used to read + /// without the lock and keep answering while it went, so a patch or a pair + /// that arrived in between was acknowledged to its sender and then left with the process. + /// Arrivals that land before this is set clear and keep the window; ones + /// that land after are refused, and the sender stages what it had rather than believing it + /// queued. + /// + public bool Closing { get; init; } + /// /// The group headers that are folded, by . /// diff --git a/src/DiffEngineViewer/ViewerProgram.cs b/src/DiffEngineViewer/ViewerProgram.cs index 134bc5a2..5959ce89 100644 --- a/src/DiffEngineViewer/ViewerProgram.cs +++ b/src/DiffEngineViewer/ViewerProgram.cs @@ -71,9 +71,14 @@ static int RunInline(OpenWindow open) { // Something else holds the queue, a tray or another viewer, so hand the patch over and // get out of the way. Whichever it is will show it. - var forwarded = ViewerClient.TrySend(new(ViewerVerb.Inline, Body: payload), out _, port); - if (!forwarded) + if (!ViewerClient.TrySend(new(ViewerVerb.Inline, Body: payload), out var response, port) || + !response.Ok) { + // Refused - an owner on its way out, or one too old for the payload - or gone + // between the bind and the send. Whoever launched this was told the patch was + // taken, and a refusal used to be read as a hand over, so it is staged rather + // than dropped: this process is the only place it exists. + InlineStaging.Persist([new PendingInline(patch)]); Console.Error.WriteLine("A viewer holds the port but did not accept the patch."); return 1; } @@ -203,14 +208,21 @@ static int RunFile(ViewerRequest request, OpenWindow open) /// /// A non null means this window is displaying someone else's queue, so - /// commands that change it are forwarded rather than applied here. + /// commands that change it are forwarded rather than applied here. Internal so + /// ViewerProgramTests can hand it a window that will not open, or one that throws. /// - static int Run(SessionHost host, ViewerServer? server, OwnerLink? link, OpenWindow open) + internal static int Run(SessionHost host, ViewerServer? server, OwnerLink? link, OpenWindow open) { var window = open("DiffEngineViewer", 1100, 700, false, out var error); if (window is null) { Console.Error.WriteLine(error); + // The port was bound before the window was asked for, so whoever launched this saw an + // owner and was told what it sent had been taken - and that is only in this process's + // memory. Staged instead, where accept tooling finds it. A display that is not there + // or a native library that will not load are both ordinary on Linux, and each used to + // cost every inline snapshot of the run. + PersistOwned(host.State, link); return 4; } @@ -232,30 +244,41 @@ static int Run(SessionHost host, ViewerServer? server, OwnerLink? link, OpenWind ? null : Task.Run(() => new TrackedWatch(host).Run(cancel.Token), Cancel.None); - using (window) - { - Loop(host, window, link, windowCommands, runner); - } - - // Closing the window mid batch does not abandon it: clicking Accept all and then closing - // used to mean both happened, because the click held the window until it was done. Before - // the listener stops, so a drive the tray started finishes answering it. - runner?.Finish(); - - cancel.Cancel(); + // Finally, so a loop that throws still ends the way one that returns does. The throw used + // to unwind straight past all of this to Main's catch, and the queue went with it. try { - listening?.Wait(TimeSpan.FromSeconds(2)); - polling?.Wait(TimeSpan.FromSeconds(2)); - watching?.Wait(TimeSpan.FromSeconds(2)); + using (window) + { + Loop(host, window, link, windowCommands, runner); + } } - catch (AggregateException) + finally { - // Cancellation unwinds through both; nothing to report. - } + // However the loop ended, nothing arriving from here on has a window to be shown in, + // and the listener keeps answering until it is cancelled below + host.Mutate(_ => _ with { Closing = true }); - // After the listener has stopped, so what is written is the final queue. - PersistOwned(host.State, link); + // Closing the window mid batch does not abandon it: clicking Accept all and then + // closing used to mean both happened, because the click held the window until it was + // done. Before the listener stops, so a drive the tray started finishes answering it. + runner?.Finish(); + + cancel.Cancel(); + try + { + listening?.Wait(TimeSpan.FromSeconds(2)); + polling?.Wait(TimeSpan.FromSeconds(2)); + watching?.Wait(TimeSpan.FromSeconds(2)); + } + catch (AggregateException) + { + // Cancellation unwinds through both; nothing to report. + } + + // After the listener has stopped, so what is written is the final queue. + PersistOwned(host.State, link); + } return 0; } @@ -306,12 +329,17 @@ static void Loop( window.SetHidden(command == WindowCommand.Hide); } - var state = host.State; - if (state.Exit) + // Committed under the lock rather than read and acted on. Between reading Exit and the + // listener stopping, an arrival used to be answered as queued and then leave with the + // window. One landing first clears Exit and keeps the window open; one landing after + // finds the viewer closing and is refused, so its sender stages it instead. + if (host.State.Exit && + host.Mutate(ViewerSession.CommitExit).Closing) { return; } + var state = host.State; if (!window.Present(ScreenBuilder.Build(state))) { return; diff --git a/src/DiffEngineViewer/ViewerSession.cs b/src/DiffEngineViewer/ViewerSession.cs index 51cc76fb..13c6a27c 100644 --- a/src/DiffEngineViewer/ViewerSession.cs +++ b/src/DiffEngineViewer/ViewerSession.cs @@ -28,6 +28,13 @@ public static SessionState Resize(SessionState state, int columns, int rows) => /// public static SessionState EnqueueInline(SessionState state, InlinePatch patch) { + // Nothing joins a queue whose window has committed to leaving. Returned as it is, and the + // caller answering the wire refuses when it sees that + if (state.Closing) + { + return state; + } + var key = InlineKey.For(patch.SourceFile, patch.LineHint); var current = state.Current; var queue = Rebuild(state, Pending(state).Enqueue(patch)); @@ -52,7 +59,10 @@ public static SessionState EnqueueInline(SessionState state, InlinePatch patch) Queue = queue, Selected = selected, // The open menu indexes the queue it was opened over, which just changed. - Menu = null + Menu = null, + // Something to show again. A settle that emptied the queue a moment ago set this, and + // carrying it across the arrival took the new entry out with the window + Exit = false }; // Nothing on screen before means nobody has been reading this one yet either. @@ -133,6 +143,12 @@ public static SessionState Settle(SessionState state, string key, string? origin /// public static SessionState EnqueueTracked(SessionState state, QueueEntry entry) { + // As EnqueueInline: refused, by the caller, once the window has committed to leaving + if (state.Closing) + { + return state; + } + var replacedCurrent = state.Current?.Key == entry.Key; var kept = state.Queue.Where(_ => _.Key != entry.Key); var queue = QueueProjection.Order([..kept, entry]); @@ -142,7 +158,9 @@ public static SessionState EnqueueTracked(SessionState state, QueueEntry entry) { Queue = queue, Selected = selected < 0 ? 0 : selected, - Menu = null + Menu = null, + // As EnqueueInline: an arrival is a reason to stay + Exit = false }; if (currentKey is null || @@ -251,6 +269,21 @@ public static SessionState Refresh( return Remove(state, queue, state.Message); } + /// + /// The loop's decision to leave, taken under the host's lock so it cannot cross an arrival: + /// still set means nothing has joined the queue since it + /// emptied, and from here nothing can. See . + /// + public static SessionState CommitExit(SessionState state) + { + if (!state.Exit) + { + return state; + } + + return state with { Closing = true }; + } + /// /// Selects by key rather than index, for a queue owner asking that a particular item be the /// one on screen. A key that is not here leaves the selection alone, because a listing and the