From e024f430df5c9eca5475fd7902bca43736e0b0a1 Mon Sep 17 00:00:00 2001 From: Simon Cropp Date: Sat, 15 Aug 2026 17:23:22 +1000 Subject: [PATCH 01/16] Escape line terminators rather than writing them into a literal U+0085, U+2028 and U+2029 are line terminators to the C# lexer, and RenderRegular passed all three through as themselves: its escape branch covers the C0 range and DEL, none of which they are. Snapshot content holding one rendered to source that does not compile - CS1010 in a regular literal, and CS8999 in a raw one, where the line the terminator starts no longer carries the closing delimiter's indentation. Both confirmed against the compiler. No delimiter width helps with the raw form, so content holding one falls back to a regular literal, which is the shape F# already falls back to for a quote run it cannot widen past. The fallback sits in RenderRaw so a caller asking for the raw form directly gets it too. F# is held to the same rule. Nothing says its lexer reads these as line breaks, but the two languages writing the one shape is what RenderMultiLineMatchesCSharp asserts and what the readers are written against. The fsi round trip covers the escaped form, so the belief that F# reads \uXXXX back as the one character is checked rather than assumed. --- src/DiffEngine.Tests/CsStringLiteralTests.cs | 58 ++++++++++++++++++- .../FsCompilerRoundTripTests.cs | 9 ++- src/DiffEngine.Tests/FsStringLiteralTests.cs | 28 ++++++++- src/DiffEngine/Inline/CsStringLiteral.cs | 14 ++++- src/DiffEngine/Inline/FsStringLiteral.cs | 17 ++++-- src/DiffEngine/Inline/StringLiteral.cs | 39 ++++++++++++- 6 files changed, 154 insertions(+), 11 deletions(-) diff --git a/src/DiffEngine.Tests/CsStringLiteralTests.cs b/src/DiffEngine.Tests/CsStringLiteralTests.cs index f5af28cf..cc22ca29 100644 --- a/src/DiffEngine.Tests/CsStringLiteralTests.cs +++ b/src/DiffEngine.Tests/CsStringLiteralTests.cs @@ -22,9 +22,21 @@ public class CsStringLiteralTests "a\n \nb", "trailing space \nnext", "emoji 🎈 and unicode β˜‚", - "line1\n indented\nline3" + "line1\n indented\nline3", + "x" + nextLine + "y", + "x" + lineSeparator + "y", + "x" + paragraphSeparator + "y", + "a\nb" + lineSeparator + "c", + lineSeparator + "leading", + "trailing" + paragraphSeparator ]; + // Line terminators to the C# lexer, past \n and \r. Named by code point rather than written + // into a literal, so nothing between here and the compiler can normalize them away + const char nextLine = (char) 0x85; + const char lineSeparator = (char) 0x2028; + const char paragraphSeparator = (char) 0x2029; + [Test] public async Task RenderSimple() { @@ -61,6 +73,50 @@ public async Task RenderQuoteRunEscalatesDelimiter() await Assert.That(rendered).IsEqualTo("\"\"\"\"\nhas \"\"\" inside\n\"\"\"\""); } + [Test] + [Arguments(0x85)] + [Arguments(0x2028)] + [Arguments(0x2029)] + public async Task RenderEscapesLineTerminator(int codePoint) + { + var terminator = (char) codePoint; + var rendered = CsStringLiteral.Render($"x{terminator}y", " ", "\n"); + + // Carried as an escape. Left as itself it ends the literal, and the file stops compiling + // (CS1010) + await Assert.That(rendered).IsEqualTo($"\"x\\u{codePoint:x4}y\""); + await Assert.That(rendered).DoesNotContain(terminator.ToString()); + } + + [Test] + [Arguments(0x85)] + [Arguments(0x2028)] + [Arguments(0x2029)] + public async Task RenderFallsBackToRegularWhenRawCannotHoldLineTerminator(int codePoint) + { + var terminator = (char) codePoint; + var content = $"a\nb{terminator}c"; + var rendered = CsStringLiteral.Render(content, " ", "\n"); + + // Content over several lines is otherwise raw, but no delimiter width makes a raw string + // able to hold this: the compiler reads the terminator as a line break, and the line it + // starts does not carry the closing delimiter's indentation (CS8999) + await Assert.That(rendered.StartsWith("\"\"\"", StringComparison.Ordinal)).IsFalse(); + await Assert.That(rendered).DoesNotContain(terminator.ToString()); + await Assert.That(CsStringLiteral.TryParse(rendered, out var value)).IsTrue(); + await Assert.That(value).IsEqualTo(content); + } + + [Test] + public async Task RenderRawFallsBackWhenContentHoldsLineTerminator() + { + // The fallback sits in RenderRaw rather than in Render, so asking for the raw form + // directly still produces source that compiles + var content = $"a\nb{lineSeparator}c"; + await Assert.That(CsStringLiteral.RenderRaw(content, " ", "\n")) + .IsEqualTo(StringLiteral.RenderRegular(content)); + } + [Test] [Arguments("a\r\nb")] [Arguments("a\rb")] diff --git a/src/DiffEngine.Tests/FsCompilerRoundTripTests.cs b/src/DiffEngine.Tests/FsCompilerRoundTripTests.cs index 34dc7f53..e94fd564 100644 --- a/src/DiffEngine.Tests/FsCompilerRoundTripTests.cs +++ b/src/DiffEngine.Tests/FsCompilerRoundTripTests.cs @@ -45,7 +45,14 @@ public class FsCompilerRoundTripTests "has \"\"\" inside\nsecond", "(* not a comment *)\nsecond", "// not a comment\nsecond", - "'ticked'\nsecond" + "'ticked'\nsecond", + // Line terminators, which are rendered as escapes rather than written into a + // triple-quoted literal. What is being asked of fsi is that it reads the escape back as + // the one character, since nothing on this side can tell whether it did + "next line" + (char) 0x85 + "inside", + "separator" + (char) 0x2028 + "inside", + "paragraph" + (char) 0x2029 + "inside", + "a\nb" + (char) 0x2028 + "c" ]; [Test] diff --git a/src/DiffEngine.Tests/FsStringLiteralTests.cs b/src/DiffEngine.Tests/FsStringLiteralTests.cs index 701457dc..b8322c87 100644 --- a/src/DiffEngine.Tests/FsStringLiteralTests.cs +++ b/src/DiffEngine.Tests/FsStringLiteralTests.cs @@ -25,9 +25,17 @@ public class FsStringLiteralTests "line1\n indented\nline3", "back\\slash\nsecond", "tab\there\nsecond", - "{\n \"name\": \"value\"\n}" + "{\n \"name\": \"value\"\n}", + "x" + lineSeparator + "y", + "a\nb" + lineSeparator + "c", + "a\nb" + nextLine + "c", + "a\nb" + paragraphSeparator + "c" ]; + const char nextLine = (char) 0x85; + const char lineSeparator = (char) 0x2028; + const char paragraphSeparator = (char) 0x2029; + // The same shape C# writes: the content indented under the call, with the first line and the // closing delimiter's indentation there to be taken back off [Test] @@ -56,6 +64,24 @@ public async Task RenderMultiLineMatchesCSharp() } } + [Test] + [Arguments(0x85)] + [Arguments(0x2028)] + [Arguments(0x2029)] + public async Task RenderFallsBackToRegularWhenContentHoldsLineTerminator(int codePoint) + { + var terminator = (char) codePoint; + var content = $"a\nb{terminator}c"; + var rendered = FsStringLiteral.Render(content, " ", "\n"); + + // Kept off the triple-quoted form for the reason C# is: the two write the one shape, and + // that is what RenderMultiLineMatchesCSharp is asserting over these same cases + await Assert.That(rendered.StartsWith("\"\"\"", StringComparison.Ordinal)).IsFalse(); + await Assert.That(rendered).DoesNotContain(terminator.ToString()); + await Assert.That(FsStringLiteral.TryParse(rendered, out var value)).IsTrue(); + await Assert.That(value).IsEqualTo(content); + } + [Test] public async Task RenderBlankLineHasNoTrailingWhitespace() { diff --git a/src/DiffEngine/Inline/CsStringLiteral.cs b/src/DiffEngine/Inline/CsStringLiteral.cs index 6f892a7c..c584ab61 100644 --- a/src/DiffEngine/Inline/CsStringLiteral.cs +++ b/src/DiffEngine/Inline/CsStringLiteral.cs @@ -14,7 +14,8 @@ public static class CsStringLiteral /// /// Renders (\n newlines) as a C# string literal expression: a /// regular literal when it is a single line, since a raw string spends three lines and an - /// indentation rule to say the same thing, and a multi-line raw literal otherwise. + /// indentation rule to say the same thing, and a multi-line raw literal otherwise - except + /// where the raw form cannot hold the content at all, which answers. /// /// Snapshot text with \n newlines. /// Whitespace prefix for content lines and the closing delimiter. @@ -42,8 +43,17 @@ public static string RenderRaw(string content, string indent, string eol) return "\"\""; } + if (StringLiteral.HasLineTerminator(content)) + { + // No delimiter can hold one of these, however wide: the compiler reads it as a line + // break, and the line it starts does not carry the closing delimiter's indentation + // (CS8999). Only a regular literal can say it, as an escape + return StringLiteral.RenderRegular(content); + } + // Three quotes, or one more than the longest run in the content, which is the widening - // F# does not have and the reason it needs a fallback where C# does not + // F# does not have and the reason a quote run sends it to a regular literal where C# stays + // raw var delimiter = new string('"', Math.Max(3, StringLiteral.LongestQuoteRun(content) + 1)); return StringLiteral.RenderMultiLine(content, indent, eol, delimiter); } diff --git a/src/DiffEngine/Inline/FsStringLiteral.cs b/src/DiffEngine/Inline/FsStringLiteral.cs index 8ede64d1..b18c57f5 100644 --- a/src/DiffEngine/Inline/FsStringLiteral.cs +++ b/src/DiffEngine/Inline/FsStringLiteral.cs @@ -38,9 +38,10 @@ public static string Render(string content, string indent, string eol) if (!CanTripleQuote(content)) { - // F# cannot widen a delimiter the way C# can (FS1232), so content that runs into one - // has no multi-line form at all. A regular literal on one source line always works, - // whatever it costs in escapes + // Content the multi-line form cannot hold: a quote run, which F# cannot widen a + // delimiter past the way C# can (FS1232), or a line terminator, which no delimiter + // helps with. A regular literal on one source line always works, whatever it costs in + // escapes return StringLiteral.RenderRegular(SourceLanguage.NormalizeNewlines(content)); } @@ -51,11 +52,19 @@ public static string Render(string content, string indent, string eol) /// Whether a triple-quoted literal can hold this content. A quote at either end would sit /// against the delimiter and be read as part of it, and a run of three anywhere would close /// the literal early. + /// + /// A line terminator rules it out as well, for the reason + /// gives. F# is stricter here than it has to be: + /// what sends C# to a regular literal is its own lexer reading one as a line break, and + /// nothing says F# does. It keeps the two languages writing the one shape, which is what + /// everything downstream of a render is written against. + /// /// static bool CanTripleQuote(string content) => content[0] != '"' && content[content.Length - 1] != '"' && - content.IndexOf("\"\"\"", StringComparison.Ordinal) == -1; + content.IndexOf("\"\"\"", StringComparison.Ordinal) == -1 && + !StringLiteral.HasLineTerminator(content); /// /// The snapshot a triple-quoted literal was written to hold: the value F# produced for it, diff --git a/src/DiffEngine/Inline/StringLiteral.cs b/src/DiffEngine/Inline/StringLiteral.cs index 6070ec38..dc62e31d 100644 --- a/src/DiffEngine/Inline/StringLiteral.cs +++ b/src/DiffEngine/Inline/StringLiteral.cs @@ -59,8 +59,12 @@ public static string RenderRegular(string content) continue; } - // Everything else a literal cannot carry as itself - if (ch < ' ' || ch == '\u007f') + // Everything else a literal cannot carry as itself. Past the C0 range that is the + // three line terminators recognised beyond \n and \r: left as themselves they end the + // literal rather than sit in it + if (ch < ' ' || + ch == '\u007f' || + IsLineTerminator(ch)) { builder.Append("\\u"); builder.Append(((int) ch).ToString("x4")); @@ -74,6 +78,37 @@ public static string RenderRegular(string content) return builder.ToString(); } + /// + /// The line terminators a lexer recognises beyond \n and \r: next line, line + /// separator and paragraph separator. + /// + /// A C# literal cannot hold one as itself. In a regular literal it ends the literal (CS1010), + /// and in a raw one it reads as a line break, so the line after it no longer starts with the + /// whitespace the closing delimiter defines (CS8999). Only an escape carries one, and only a + /// regular literal has escapes, which is why content holding one is written as a regular + /// literal whatever else it holds. + /// + /// + public static bool IsLineTerminator(char ch) => + ch is (char) 0x85 or (char) 0x2028 or (char) 0x2029; + + /// + /// True when the content holds a terminator describes, which is + /// what sends content that would otherwise be written multi-line to a regular literal instead. + /// + public static bool HasLineTerminator(string content) + { + foreach (var ch in content) + { + if (IsLineTerminator(ch)) + { + return true; + } + } + + return false; + } + /// /// Renders (\n newlines) as a multi-line literal delimited by /// . The result starts with the opening delimiter (no leading From 60b2f33cf4de2a1413cfbe09a5eec1eb641aca36 Mon Sep 17 00:00:00 2001 From: Simon Cropp Date: Sat, 15 Aug 2026 17:26:36 +1000 Subject: [PATCH 02/16] Return false rather than throwing for a code point a string cannot hold The \U escape checked only the top of the Unicode range, so a lone surrogate reached char.ConvertFromUtf32 and threw. Nothing between there and the process applying the patch catches, and a TryParse that throws is not one that failed: the callers handle false. Both languages had it, and both now ask IsScalarValue. Also read @"""x""" as the verbatim literal it is. The quote run was measured before the @ was considered, so a verbatim literal opening on an escaped quote looked like a raw string carrying an @ and was rejected. F# already asked in that order, with the reasoning written out; C# now matches it, which is why the F# tests already covered the case. Verified against the compiler: @"""x""" is valid C# worth "x". --- src/DiffEngine.Tests/CsStringLiteralTests.cs | 26 ++++++++++++++++++++ src/DiffEngine.Tests/FsStringLiteralTests.cs | 13 ++++++++++ src/DiffEngine/Inline/CsStringLiteral.cs | 21 ++++++++-------- src/DiffEngine/Inline/FsStringLiteral.cs | 2 +- src/DiffEngine/Inline/StringLiteral.cs | 14 +++++++++++ 5 files changed, 64 insertions(+), 12 deletions(-) diff --git a/src/DiffEngine.Tests/CsStringLiteralTests.cs b/src/DiffEngine.Tests/CsStringLiteralTests.cs index cc22ca29..2a7b3aa5 100644 --- a/src/DiffEngine.Tests/CsStringLiteralTests.cs +++ b/src/DiffEngine.Tests/CsStringLiteralTests.cs @@ -229,6 +229,32 @@ public async Task Parse(string expression, string expected) await Assert.That(value).IsEqualTo(expected); } + // A verbatim literal opening on an escaped quote. The quote run is not a delimiter here, and + // reading it as one rejected a literal the compiler accepts + [Test] + [Arguments("@\"\"\"x\"\"\"", "\"x\"")] + [Arguments("@\"\"\"\"", "\"")] + [Arguments("@\"\"\"a\"\"b\"\"\"", "\"a\"b\"")] + public async Task ParseVerbatimOpeningOnQuote(string expression, string expected) + { + var parsed = CsStringLiteral.TryParse(expression, out var value); + await Assert.That(parsed).IsTrue(); + await Assert.That(value).IsEqualTo(expected); + } + + // Half a surrogate pair, and a code point past the Unicode range. ConvertFromUtf32 throws on + // both, and a throw here reaches the process applying the patch + [Test] + [Arguments("\"\\U0000D800\"")] + [Arguments("\"\\U0000DFFF\"")] + [Arguments("\"\\U00110000\"")] + [Arguments("\"\\UFFFFFFFF\"")] + public async Task ParseRejectsUnreadableCodePoint(string expression) + { + var parsed = CsStringLiteral.TryParse(expression, out _); + await Assert.That(parsed).IsFalse(); + } + [Test] public async Task ParseMultiLineVerbatim() { diff --git a/src/DiffEngine.Tests/FsStringLiteralTests.cs b/src/DiffEngine.Tests/FsStringLiteralTests.cs index b8322c87..e6841543 100644 --- a/src/DiffEngine.Tests/FsStringLiteralTests.cs +++ b/src/DiffEngine.Tests/FsStringLiteralTests.cs @@ -239,6 +239,19 @@ public async Task Parse(string expression, string expected) await Assert.That(value).IsEqualTo(expected); } + // Half a surrogate pair, and a code point past the Unicode range. ConvertFromUtf32 throws on + // both, and a throw here reaches the process applying the patch + [Test] + [Arguments("\"\\U0000D800\"")] + [Arguments("\"\\U0000DFFF\"")] + [Arguments("\"\\U00110000\"")] + [Arguments("\"\\UFFFFFFFF\"")] + public async Task ParseRejectsUnreadableCodePoint(string expression) + { + var parsed = FsStringLiteral.TryParse(expression, out _); + await Assert.That(parsed).IsFalse(); + } + [Test] public async Task ParseMultiLineTakesTheLayoutOff() { diff --git a/src/DiffEngine/Inline/CsStringLiteral.cs b/src/DiffEngine/Inline/CsStringLiteral.cs index c584ab61..58490598 100644 --- a/src/DiffEngine/Inline/CsStringLiteral.cs +++ b/src/DiffEngine/Inline/CsStringLiteral.cs @@ -117,20 +117,19 @@ static bool TryScanLiteral(string text, int start, out string? value, out int en return false; } - var quotes = StringLiteral.QuoteRunLength(text, index); - if (quotes >= 3) + if (verbatim) { - if (verbatim) - { - return false; - } - - return StringLiteral.TryScanMultiLine(text, index, quotes, out value, out end); + // Asked before the quote run is measured, because there is no verbatim raw form: a + // run of quotes after @" is an escaped quote and the start of the content, not a + // delimiter. Measuring first read @"""x""" as a raw string that happened to carry an + // @, and rejected a literal C# is perfectly happy with + return StringLiteral.TryScanVerbatim(text, index + 1, out value, out end); } - if (verbatim) + var quotes = StringLiteral.QuoteRunLength(text, index); + if (quotes >= 3) { - return StringLiteral.TryScanVerbatim(text, index + 1, out value, out end); + return StringLiteral.TryScanMultiLine(text, index, quotes, out value, out end); } if (quotes == 2) @@ -241,7 +240,7 @@ static bool TryScanRegular(string text, int start, out string? value, out int en return false; } - if (codePoint > 0x10FFFF) + if (!StringLiteral.IsScalarValue(codePoint)) { return false; } diff --git a/src/DiffEngine/Inline/FsStringLiteral.cs b/src/DiffEngine/Inline/FsStringLiteral.cs index b18c57f5..20202261 100644 --- a/src/DiffEngine/Inline/FsStringLiteral.cs +++ b/src/DiffEngine/Inline/FsStringLiteral.cs @@ -254,7 +254,7 @@ static bool TryScanRegular(string text, int start, out string? value, out int en return false; } - if (codePoint > 0x10FFFF) + if (!StringLiteral.IsScalarValue(codePoint)) { return false; } diff --git a/src/DiffEngine/Inline/StringLiteral.cs b/src/DiffEngine/Inline/StringLiteral.cs index dc62e31d..040a2984 100644 --- a/src/DiffEngine/Inline/StringLiteral.cs +++ b/src/DiffEngine/Inline/StringLiteral.cs @@ -320,6 +320,20 @@ public static bool TryScanVerbatim(string text, int start, out string? value, ou return false; } + /// + /// Whether a code point is one a string can hold: inside the Unicode range, and not half of a + /// surrogate pair written on its own. + /// + /// Asked ahead of , which throws on either. A parse + /// that throws is not a parse that failed: callers handle false, and between here and the + /// process hosting the queue nothing catches. Source holding one of these does not compile, so + /// the answer being looked for is that this is not a literal that can be read. + /// + /// + public static bool IsScalarValue(uint codePoint) => + codePoint <= 0x10FFFF && + codePoint is < 0xD800 or > 0xDFFF; + public static bool TryReadHex(string text, ref int index, int min, int max, out uint result) { result = 0; From b532a778767d5012393e9db806c61a3ac781d94d Mon Sep 17 00:00:00 2001 From: Simon Cropp Date: Sat, 15 Aug 2026 17:39:25 +1000 Subject: [PATCH 03/16] Swap the patched source in rather than writing over it Writing in place truncates the file and fills it back in, so a process killed partway through, or a disk that fills up, cost the caller the rest of their source file. The mutex keeps two appliers apart but says nothing about one that stops halfway. Everything else here is careful because the whole file is rewritten rather than the patched span; the write was the step that could still lose it. The new content goes to a sibling temporary and File.Replace renames it over the original, which keeps the destination attributes and is on every framework this targets, unlike the overwriting File.Move. That moved the File.Exists precondition. Replace takes the path away for the instant it renames over it, and the check sat outside the lock, so an applier waiting its turn on a file another was finishing with reported it missing - which ParallelAppliesToSameFile went red on, intermittently, once the rest of the class was loading the machine. It is now asked inside the lock, with every other operation on the file. ContentIsNeverObservedHalfWritten reads alongside an apply and fails only if a length that belongs to neither whole file is seen; it goes red on the old write. The contention test pins the contract rather than the window, which is too narrow to reproduce on demand. --- src/DiffEngine.Tests/GlobalUsings.cs | 1 + src/DiffEngine.Tests/InlineApplierTests.cs | 150 ++++++++++++++++++++- src/DiffEngine/Inline/InlineApplier.cs | 63 ++++++++- 3 files changed, 207 insertions(+), 7 deletions(-) diff --git a/src/DiffEngine.Tests/GlobalUsings.cs b/src/DiffEngine.Tests/GlobalUsings.cs index 1a351c1f..49fe999c 100644 --- a/src/DiffEngine.Tests/GlobalUsings.cs +++ b/src/DiffEngine.Tests/GlobalUsings.cs @@ -1,4 +1,5 @@ global using EmptyFiles; +global using System.Collections.Concurrent; global using System.Diagnostics; global using System.Reflection; global using System.Text; diff --git a/src/DiffEngine.Tests/InlineApplierTests.cs b/src/DiffEngine.Tests/InlineApplierTests.cs index 310ad86c..19cc431d 100644 --- a/src/DiffEngine.Tests/InlineApplierTests.cs +++ b/src/DiffEngine.Tests/InlineApplierTests.cs @@ -1,4 +1,4 @@ -ο»Ώpublic class InlineApplierTests +public class InlineApplierTests { static string WriteTemp(byte[] bytes, string extension = ".cs") { @@ -7,6 +7,14 @@ static string WriteTemp(byte[] bytes, string extension = ".cs") return path; } + // A directory of its own, for the tests that are about what is left in one + static string NewDirectory() + { + var path = Path.Combine(Path.GetTempPath(), $"InlineApplierTests_{Guid.NewGuid():N}"); + Directory.CreateDirectory(path); + return path; + } + static byte[] Utf8(string text, bool bom) { var encoding = new UTF8Encoding(bom); @@ -95,6 +103,146 @@ public async Task Utf16Preserved() } } + // The swap goes through a sibling temporary. It is gone by the time the patch is applied, + // whatever else is true, because a stray file in a source directory is the caller's problem + [Test] + public async Task LeavesNoTemporaryBehind() + { + var directory = NewDirectory(); + try + { + var path = Path.Combine(directory, "Sample.cs"); + File.WriteAllBytes(path, Utf8(source, bom: false)); + + var result = InlineApplier.Apply(Patch(path, 3, "\"old\"", "new")); + + await Assert.That(result.Status).IsEqualTo(InlineApplyStatus.Applied); + 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 + [Test] + public async Task ContentIsNeverObservedHalfWritten() + { + var padding = string.Join("\n", Enumerable.Repeat(" // padding, to widen the window a truncating write would open", 30000)); + var text = $"class C\n{{\n void M() => Verify(value).Snapshot(\"old\");\n{padding}\n}}"; + var directory = NewDirectory(); + try + { + var path = Path.Combine(directory, "Big.cs"); + File.WriteAllBytes(path, Utf8(text, bom: false)); + var before = new FileInfo(path).Length; + + using var cancellation = new CancellationTokenSource(); + var seen = new ConcurrentDictionary(); + var reader = Task.Run( + () => + { + while (!cancellation.IsCancellationRequested) + { + try + { + // Shared every way, so watching the file never blocks the write being + // watched. Holding it any other way tests which of the two wins the + // file rather than what a reader of it sees + using var stream = new FileStream(path, FileMode.Open, FileAccess.Read, FileShare.ReadWrite | FileShare.Delete); + seen.TryAdd(stream.Length, 0); + } + catch (Exception exception) + when (exception is IOException or UnauthorizedAccessException) + { + // The swap is in flight. Not an observation of the content + } + } + }); + + // Long enough that the two whole files differ in length, which is what makes a + // half written one tell itself apart + var result = InlineApplier.Apply(Patch(path, 3, "\"old\"", "a replacement longer than what it replaces")); + cancellation.Cancel(); + await reader; + + await Assert.That(result.Status).IsEqualTo(InlineApplyStatus.Applied); + var after = new FileInfo(path).Length; + await Assert.That(after).IsNotEqualTo(before); + + var partial = seen.Keys.Where(_ => _ != before && _ != after).ToList(); + await Assert.That(partial).IsEmpty(); + } + finally + { + Directory.Delete(directory, true); + } + } + + // Swapping the file in takes the path away for the instant it takes to rename over it. An + // applier waiting its turn must not read that as a file that is not there, which is what + // asking before taking the lock did + [Test] + public async Task ConcurrentAppliesNeverReportAMissingFile() + { + const int writers = 4; + const int rounds = 60; + var directory = NewDirectory(); + try + { + // A snapshot each, flipped back and forth, so every apply is a real write and the + // file is swapped out from under the others hundreds of times. The window this is + // about is microseconds wide and no arrangement here reproduces it on demand - it + // showed up as ParallelAppliesToSameFile going red under the load of the rest of this + // class. What is pinned is the contract: contended appliers all get their write, and + // none of them reports the file as gone + var methods = Enumerable.Range(0, writers) + .Select(_ => $" void M{_}() => Verify(v).Snapshot(\"a{_}\");"); + var path = Path.Combine(directory, "Contended.cs"); + File.WriteAllBytes(path, Utf8($"class C\n{{\n{string.Join("\n", methods)}\n}}", bom: false)); + + var results = await Task.WhenAll( + Enumerable + .Range(0, writers) + .Select( + index => Task.Run( + () => + { + var outcomes = new List(); + for (var round = 0; round < rounds; round++) + { + var (from, to) = round % 2 == 0 + ? ($"a{index}", $"b{index}") + : ($"b{index}", $"a{index}"); + outcomes.Add(InlineApplier.Apply(Patch(path, index + 3, $"\"{from}\"", to))); + } + + return outcomes; + }))); + + var failures = results + .SelectMany(_ => _) + .Where(_ => _.Status == InlineApplyStatus.Failed) + .Select(_ => _.Message) + .ToList(); + await Assert.That(failures).IsEmpty(); + + // And none of them lost another's line along the way + var text = await File.ReadAllTextAsync(path); + for (var index = 0; index < writers; index++) + { + await Assert.That(text).Contains($"void M{index}()"); + } + } + finally + { + Directory.Delete(directory, true); + } + } + // The whole file is rewritten, not just the patched span, so a byte that does not decode // would come back as a replacement character everywhere it appears [Test] diff --git a/src/DiffEngine/Inline/InlineApplier.cs b/src/DiffEngine/Inline/InlineApplier.cs index 9b2dd380..12d1e6a2 100644 --- a/src/DiffEngine/Inline/InlineApplier.cs +++ b/src/DiffEngine/Inline/InlineApplier.cs @@ -35,11 +35,6 @@ public static InlineApplyResult Apply(InlinePatch patch) return InlineApplyResult.Failed($"Invalid InlinePatch.SourceFile: {patch.SourceFile}", exception); } - if (!File.Exists(fullPath)) - { - return InlineApplyResult.Failed($"Source file does not exist: {fullPath}"); - } - var newContent = SourceLanguage.NormalizeNewlines(patch.NewContent); var normalizedPath = fullPath.ToLowerInvariant(); lock (gates.GetOrAdd(normalizedPath, static _ => new())) @@ -76,6 +71,15 @@ public static InlineApplyResult Apply(InlinePatch patch) static InlineApplyResult LockedApply(string fullPath, InlinePatch patch, string newContent) { + // Asked here rather than before the lock, because the swap at the end of this method takes + // the path away for the instant it takes to rename over it. Asked outside, an applier + // waiting its turn on a file another one was finishing with saw the file as missing and + // reported it, which is neither true nor the sort of thing a retry was going to fix + if (!File.Exists(fullPath)) + { + return InlineApplyResult.Failed($"Source file does not exist: {fullPath}"); + } + byte[] bytes; try { @@ -139,7 +143,7 @@ static InlineApplyResult LockedApply(string fullPath, InlinePatch patch, string output = content; } - File.WriteAllBytes(fullPath, output); + WriteThroughTemporary(fullPath, output); } catch (Exception exception) { @@ -149,6 +153,53 @@ static InlineApplyResult LockedApply(string fullPath, InlinePatch patch, string return InlineApplyResult.Applied; } + /// + /// Writes the patched source through a temporary file beside it and swaps that in, so the file + /// on disk is either what it was or what the patch made it and never half of either. + /// + /// Writing in place truncates the file first and fills it back in, which leaves a window where + /// a killed process or a full disk costs the caller the rest of their source file. The mutex + /// above keeps two appliers apart but says nothing about a process that stops partway. Every + /// other care taken here - strict decoders, the BOM round trip, refusing a file that will not + /// decode - is because the whole file is rewritten rather than the patched span, and the write + /// itself was the step that could still lose it. + /// + /// + /// The temporary is a sibling so the swap stays on one volume, where it is a rename rather + /// than a copy. Replace rather than a move that overwrites, because it keeps the attributes + /// the destination already had, and because the overwriting move does not exist on every + /// framework this targets. + /// + /// + static void WriteThroughTemporary(string fullPath, byte[] output) + { + var directory = Path.GetDirectoryName(fullPath)!; + // Named after the file it replaces, so anything left by a process that died between the + // write and the swap says what it was for. The extension keeps it out of a *.cs glob + var temporary = Path.Combine(directory, $"{Path.GetFileName(fullPath)}.{Guid.NewGuid():N}.tmp"); + try + { + File.WriteAllBytes(temporary, output); + File.Replace(temporary, fullPath, null); + } + 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 + try + { + if (File.Exists(temporary)) + { + File.Delete(temporary); + } + } + catch (Exception exception) + when (exception is IOException or UnauthorizedAccessException) + { + } + } + } + /// /// 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 From 6b01698b7f25e56dc9cd2572f11499ec3483431b Mon Sep 17 00:00:00 2001 From: Simon Cropp Date: Sat, 15 Aug 2026 17:44:02 +1000 Subject: [PATCH 04/16] Wait for an accept the owner is still applying Every verb the remote host sends used the short wait, which is sized for the scan timer and the menu. An accept is not: the owner applies through InlineApplier, which waits up to ten seconds on its cross process mutex, and an owning viewer does that inside its session. Anything slower than half a second was reported as "The snapshot viewer is not running." while the owner was in the middle of writing the source file - and the snapshot then left the menu a scan later, contradicting the balloon. Accept and AcceptAll now use the same fifteen seconds InlineQueueClient and OwnerLink already use, and for the reason both of them state. Safe because accepts run on a worker, which is what Tracker.Accept exists to arrange; the listing and menu verbs keep the short wait. Both tests drive a real socket with an applier that sleeps past the old wait, and both go red on it. --- .../TrayViewerSyncTest.cs | 46 ++++++++++++++++++ src/DiffEngineTray/RemoteInlineHost.cs | 47 ++++++++++++++----- 2 files changed, 80 insertions(+), 13 deletions(-) diff --git a/src/DiffEngineTray.Tests/TrayViewerSyncTest.cs b/src/DiffEngineTray.Tests/TrayViewerSyncTest.cs index b1b4977e..dae09165 100644 --- a/src/DiffEngineTray.Tests/TrayViewerSyncTest.cs +++ b/src/DiffEngineTray.Tests/TrayViewerSyncTest.cs @@ -456,6 +456,52 @@ public async Task TrayAcceptAllReportsWhatTheOwningViewerKept() await Assert.That(pair.Failures.Single()).Contains("the file is locked"); } + /// + /// 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. + /// The tray has to wait for the answer: reporting the viewer as gone while it is in the middle + /// of writing the source file is wrong twice over, since the snapshot then leaves the menu a + /// scan later and contradicts the balloon. + /// + [Test] + public async Task ASlowAcceptIsWaitedForRatherThanCalledAMissingViewer() + { + await using var pair = new ViewerOwned( + _ => + { + Thread.Sleep(TimeSpan.FromSeconds(1.5)); + return ViewerSideApplyResult.Applied; + }); + var snapshot = pair.Snapshot(sample, 1); + + await pair.Tracker.Accept(snapshot); + + await Assert.That(pair.Failures).IsEmpty(); + await Assert.That(pair.Viewer.Queue).IsEmpty(); + await Assert.That(pair.Tracker.Snapshots).IsEmpty(); + } + + /// + [Test] + public async Task ASlowAcceptAllIsWaitedFor() + { + // Every entry applied inside the one exchange, so this outlasts a lone accept + await using var pair = new ViewerOwned( + _ => + { + Thread.Sleep(TimeSpan.FromSeconds(0.8)); + return ViewerSideApplyResult.Applied; + }); + pair.Queue(sample, 1); + pair.Queue(other, 7); + + await pair.Tracker.AcceptAll(); + + await Assert.That(pair.Failures).IsEmpty(); + await Assert.That(pair.Viewer.Queue).IsEmpty(); + await Assert.That(pair.Applied.Count).IsEqualTo(2); + } + [Test] public async Task AConflictIsRefusedTheSameWayFromTheTray() { diff --git a/src/DiffEngineTray/RemoteInlineHost.cs b/src/DiffEngineTray/RemoteInlineHost.cs index 31197ddd..af494847 100644 --- a/src/DiffEngineTray/RemoteInlineHost.cs +++ b/src/DiffEngineTray/RemoteInlineHost.cs @@ -2,8 +2,12 @@ /// The queue belongs to a viewer that bound the port before this tray started, so every call is a /// short loopback round trip and the tray is a remote control. /// -/// All of them use ViewerClient.ShortTimeout. These run from the 2 second scan timer and from the -/// menu opening, so a slow exchange must not outlast the timer period or block the UI. +/// The listing and the menu verbs use ViewerClient.ShortTimeout. Those run from the 2 second scan +/// timer and from the menu opening, so a slow exchange must not outlast the timer period or block +/// the UI. +/// +/// +/// Accepting does not, for the reason gives. /// /// /// A refused connection means the viewer has gone, which is the same as nothing pending. The queue @@ -12,11 +16,26 @@ /// class RemoteInlineHost : IInlineHost { + /// + /// The one verb that can legitimately take this long: the owner applies through + /// , which waits up to ten seconds on its cross process mutex, and + /// an owning viewer does that inside its session. The short wait read a busy owner as an + /// absent one and told the user the viewer was not running while it was in the middle of + /// writing their source file - and then the snapshot left the menu a scan later, contradicting + /// the balloon. + /// + /// The same wait and OwnerLink use, and safe here for the same + /// reason it is there: accepts run on a worker rather than the timer or the UI thread, which + /// is what exists to arrange. + /// + /// + static readonly TimeSpan acceptWait = TimeSpan.FromSeconds(15); + public string Description => $"owned by another process on port {ViewerClient.Port}"; public IReadOnlyList List() { - if (!Exchange(new(ViewerVerb.List), out var response) || + if (!Exchange(new(ViewerVerb.List), ViewerClient.ShortTimeout, out var response) || !response.Ok) { return []; @@ -50,7 +69,7 @@ public IReadOnlyList List() /// public AcceptOutcome Accept(PendingSnapshot snapshot, out string? message) { - if (!Send(ViewerVerb.Accept, snapshot.Key, out message)) + if (!Send(ViewerVerb.Accept, snapshot.Key, acceptWait, out message)) { return AcceptOutcome.Failed; } @@ -61,30 +80,32 @@ public AcceptOutcome Accept(PendingSnapshot snapshot, out string? message) } public bool Discard(PendingSnapshot snapshot, out string? message) => - Send(ViewerVerb.Discard, snapshot.Key, out message); + Send(ViewerVerb.Discard, snapshot.Key, ViewerClient.ShortTimeout, out 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. /// + // One accept per entry inside a single exchange, so this outlasts a lone accept rather than + // matching it public bool AcceptAll(out string? message) => - Send(ViewerVerb.AcceptAll, null, out message) && + Send(ViewerVerb.AcceptAll, null, acceptWait, out message) && List().Count == 0; public void DiscardAll() => - Send(ViewerVerb.DiscardAll, null, out _); + Send(ViewerVerb.DiscardAll, null, ViewerClient.ShortTimeout, out _); public void Focus(PendingSnapshot snapshot) => - Send(ViewerVerb.Focus, snapshot.Key, out _); + Send(ViewerVerb.Focus, snapshot.Key, ViewerClient.ShortTimeout, out _); public void Close() => - Send(ViewerVerb.Quit, null, out _); + Send(ViewerVerb.Quit, null, ViewerClient.ShortTimeout, out _); - static bool Send(ViewerVerb verb, string? key, out string? message) + static bool Send(ViewerVerb verb, string? key, TimeSpan wait, out string? message) { message = null; - if (!Exchange(new(verb, key), out var response)) + if (!Exchange(new(verb, key), wait, out var response)) { message = "The snapshot viewer is not running."; return false; @@ -94,6 +115,6 @@ static bool Send(ViewerVerb verb, string? key, out string? message) return response.Ok; } - static bool Exchange(ViewerMessage message, [NotNullWhen(true)] out ViewerResponse? response) => - ViewerClient.TrySend(message, out response, wait: ViewerClient.ShortTimeout); + static bool Exchange(ViewerMessage message, TimeSpan wait, [NotNullWhen(true)] out ViewerResponse? response) => + ViewerClient.TrySend(message, out response, wait: wait); } From 0b5b66d78433d018a45c281c83defd4e54a79685 Mon Sep 17 00:00:00 2001 From: Simon Cropp Date: Sat, 15 Aug 2026 17:46:52 +1000 Subject: [PATCH 05/16] Do not call a snapshot that was already gone a failed accept The tray menu is built from the last scan, so an item outlives its entry whenever the test re-ran and passed or another surface accepted it first. Clicking one of those returns Unknown, which fell through to the failure branch and raised "Could not accept the snapshot for X. " - naming a snapshot that is already in the source, and with a null message leaving the sentence hanging. Unknown is now its own case and says nothing, which is what accepting a group has always done with it. The failure text is built by a helper so an owner with nothing to add does not produce the trailing full stop and space either. --- .../TrayViewerSyncTest.cs | 22 +++++++++++++++++++ src/DiffEngineTray/Tracker.cs | 18 +++++++++++++-- 2 files changed, 38 insertions(+), 2 deletions(-) diff --git a/src/DiffEngineTray.Tests/TrayViewerSyncTest.cs b/src/DiffEngineTray.Tests/TrayViewerSyncTest.cs index dae09165..882c6c9b 100644 --- a/src/DiffEngineTray.Tests/TrayViewerSyncTest.cs +++ b/src/DiffEngineTray.Tests/TrayViewerSyncTest.cs @@ -251,6 +251,28 @@ public async Task ViewerAcceptOfATrackedMoveReachesTheTray() await Assert.That(await File.ReadAllTextAsync(move.Target)).IsEqualTo("received"); } + /// + /// The tray menu is built from the last scan, so an item can outlive its entry: the test + /// re-ran and passed, or the viewer accepted it first. Clicking it accepts nothing, and there + /// is nothing to tell the user β€” a failure balloon there names a snapshot that is already in + /// the source. Accepting a group has always skipped these; accepting one of them said the + /// accept had failed. + /// + [Test] + public async Task AcceptingASnapshotThatIsAlreadyGoneSaysNothing() + { + await using var pair = new TrayOwned(); + var snapshot = pair.Queue(sample, 1); + pair.Queue(other, 7); + pair.Tracker.Discard(snapshot); + + await pair.Tracker.Accept(snapshot); + + await Assert.That(pair.Failures).IsEmpty(); + await Assert.That(pair.Applied).IsEmpty(); + await Assert.That(pair.Tracker.Snapshots.Select(_ => _.Key)).IsEquivalentTo([Key(other, 7)]); + } + /// /// A failed apply keeps its entry so it can be retried, and both surfaces have to say the same /// thing about it β€” the tray in a balloon, the viewer in the entry's status. diff --git a/src/DiffEngineTray/Tracker.cs b/src/DiffEngineTray/Tracker.cs index fe60f541..0aedf2e3 100644 --- a/src/DiffEngineTray/Tracker.cs +++ b/src/DiffEngineTray/Tracker.cs @@ -245,11 +245,18 @@ public Task Accept(PendingSnapshot snapshot) => // Gone, but not accepted. Told rather than logged, because the snapshot // vanishing from the menu otherwise reads as success. Log.Warning("Inline snapshot stale for `{Name}`: {Message}", snapshot.Name, message); - inlineFailed?.Invoke($"Could not accept the snapshot for '{snapshot.Name}'. {message}"); + inlineFailed?.Invoke(CouldNotAccept(snapshot.Name, message)); + break; + case AcceptOutcome.Unknown: + // No entry left to accept: it settled, or another surface got to it first. + // The menu is built from the last scan, so an item outliving its entry is + // ordinary rather than a failure, and a balloon here names a snapshot that + // is already in the source. The bulk path has always skipped these + Log.Information("Inline snapshot for `{Name}` was no longer pending.", snapshot.Name); break; default: Log.Warning("Inline snapshot accept failed for `{Name}`: {Message}", snapshot.Name, message); - inlineFailed?.Invoke($"Could not accept the snapshot for '{snapshot.Name}'. {message}"); + inlineFailed?.Invoke(CouldNotAccept(snapshot.Name, message)); break; } @@ -261,6 +268,13 @@ public Task Accept(PendingSnapshot snapshot) => } }); + // The owner does not always have something to add, and a balloon ending in a bare full stop + // and a space reads as a message that went missing + static string CouldNotAccept(string name, string? message) => + message is { Length: > 0 } + ? $"Could not accept the snapshot for '{name}'. {message}" + : $"Could not accept the snapshot for '{name}'."; + public void Discard(PendingSnapshot snapshot) { if (!inline.Discard(snapshot, out var message)) From 544dedd4117d8b673d85ddb81b38571fc15c1b1a Mon Sep 17 00:00:00 2001 From: Simon Cropp Date: Sat, 15 Aug 2026 17:52:31 +1000 Subject: [PATCH 06/16] Do not read a listing that failed as an empty queue Both accept clients decide what became of an accept by asking, in a second round trip, whether the entry survived. A listing that could not be made returned no items, and no items was read as "not pending", so an owner that took the accept and then went away - or answered with an error - came back as Accepted. A surface told that stops offering a snapshot nothing ever confirmed landed. TryList and TryListKeys now report false for an error response as well as for an absent owner: an empty item list on an error is not a statement about what is pending. StillPending answers three ways instead of two, and the case it gained maps to Unknown, which is the outcome that claims nothing. RemoteInlineHost does the same, and keeps flattening it to nothing pending for List, which is what the menu wants. The residual is the other direction and is inherent to inferring from a listing: a re-run that re-enqueues the same key between the accept and the listing still reads as Failed. Carrying the apply status on the wire would remove that and both round trips, but that is a protocol change rather than a fix. --- .../InlineQueueClientTests.cs | 50 +++++++++++++++++++ src/DiffEngine/Inline/InlineQueueClient.cs | 48 +++++++++++++----- src/DiffEngineTray.Tests/FakeViewer.cs | 14 ++++++ .../TrackerSnapshotTest.cs | 20 ++++++++ src/DiffEngineTray/RemoteInlineHost.cs | 25 ++++++++-- 5 files changed, 140 insertions(+), 17 deletions(-) diff --git a/src/DiffEngine.Tests/InlineQueueClientTests.cs b/src/DiffEngine.Tests/InlineQueueClientTests.cs index d938a7b7..18cbb237 100644 --- a/src/DiffEngine.Tests/InlineQueueClientTests.cs +++ b/src/DiffEngine.Tests/InlineQueueClientTests.cs @@ -183,6 +183,45 @@ public async Task AcceptOfAnUnknownCallSiteDoesNothing() await Assert.That(InlineQueueClient.Find(InlineKey.For("Sample.cs", 42))).IsNotNull(); } + /// + /// Whether the entry survived is what says an accept applied, and that takes a second round + /// trip. When it cannot be made β€” the owner exited, or answered with an error β€” there is no + /// answer to give, and the one that used to be given was Accepted: an empty item list read as + /// an empty queue, so a surface stopped offering a snapshot nothing had confirmed. + /// + [Test] + public async Task AnAcceptThatCannotBeConfirmedIsNotCalledAccepted() + { + using var owner = new Owner(); + owner.Enqueue(Patch()); + owner.ListingFails = true; + + var outcome = InlineQueueClient.Accept(InlineKey.For("Sample.cs", 42), out _); + + await Assert.That(outcome).IsEqualTo(InlineAcceptOutcome.Unknown); + // The owner did carry it out; only the confirmation was unavailable + await Assert.That(owner.Applied.Single().NewContent).IsEqualTo("new content"); + } + + /// + /// The same shape one level down: an error listing is not a statement that nothing is pending, + /// so a caller that falls back when no owner answered has to fall back here too. + /// + [Test] + public async Task AnErrorListingIsNotAnEmptyQueue() + { + using var owner = new Owner + { + ListingFails = true + }; + owner.Enqueue(Patch()); + + await Assert.That(InlineQueueClient.TryList(out var pending)).IsFalse(); + await Assert.That(pending).IsEmpty(); + await Assert.That(InlineQueueClient.TryListKeys(out var keys)).IsFalse(); + await Assert.That(keys).IsEmpty(); + } + [Test] public async Task DiscardDropsWithoutApplying() { @@ -317,8 +356,19 @@ void IQueueOwner.TrackDelete(string file) { } + /// + /// When set, every listing comes back as an error, which is how an owner that cannot say + /// what it holds is arranged without racing its own shutdown. + /// + public bool ListingFails { get; set; } + ViewerResponse IQueueOwner.Listing(bool withPatches) { + if (ListingFails) + { + return ViewerResponse.Error("the owner is going away"); + } + lock (gate) { return ViewerResponse.Listing(ViewerListing.Items(queue.Items, withPatches)); diff --git a/src/DiffEngine/Inline/InlineQueueClient.cs b/src/DiffEngine/Inline/InlineQueueClient.cs index 3068094a..aaac0098 100644 --- a/src/DiffEngine/Inline/InlineQueueClient.cs +++ b/src/DiffEngine/Inline/InlineQueueClient.cs @@ -6,8 +6,14 @@ namespace DiffEngine; public enum InlineAcceptOutcome { /// - /// Nothing happened: no owner answered, or the one that did holds no entry for that key β€” - /// it settled, or another surface got to it first. + /// There is nothing to report. No owner answered, or the one that did holds no entry for that + /// key β€” it settled, or another surface got to it first. + /// + /// Also the answer when an owner took the accept and then could not be asked what became of + /// it. That is rare and it is not nothing, but the two outcomes it sits between are worse + /// guesses: stops a caller offering a snapshot that may still be + /// pending, and invites a retry of one that is probably already applied. + /// /// Unknown, @@ -74,7 +80,10 @@ public static class InlineQueueClient /// public static bool TryList(out IReadOnlyList pending) { - if (!Exchange(new(ViewerVerb.ListFull), ViewerClient.ShortTimeout, out var response)) + // An owner that answered with an error has not told us what it holds, and an empty item + // list on an error is not the same statement as an empty queue + if (!Exchange(new(ViewerVerb.ListFull), ViewerClient.ShortTimeout, out var response) || + !response.Ok) { pending = []; return false; @@ -95,7 +104,8 @@ public static bool TryList(out IReadOnlyList pending) /// public static bool TryListKeys(out IReadOnlyList keys) { - if (!Exchange(new(ViewerVerb.List), ViewerClient.ShortTimeout, out var response)) + if (!Exchange(new(ViewerVerb.List), ViewerClient.ShortTimeout, out var response) || + !response.Ok) { keys = []; return false; @@ -117,8 +127,8 @@ public static bool TryListKeys(out IReadOnlyList keys) /// /// Asks the owner to apply the patch for a call site and drop it from the queue. /// is the owner's own account of what happened, suitable to show a - /// user as it stands, and null when it had nothing to say β€” always so for - /// , where nothing happened. + /// user as it stands, and null when it had nothing to say β€” which is most of the time for + /// , since nothing usually happened. /// public static InlineAcceptOutcome Accept(string key, out string? message) { @@ -133,7 +143,7 @@ public static InlineAcceptOutcome Accept(string key, out string? message) { // One error shape covers both "no entry for that key" and a refusal on a live one β€” a // conflicted entry β€” so which it was is asked rather than read out of the text. - if (StillPending(key)) + if (StillPending(key) == true) { return InlineAcceptOutcome.Failed; } @@ -147,9 +157,15 @@ public static InlineAcceptOutcome Accept(string key, out string? message) // Attempted, but attempted is not applied: an owner keeps an entry that failed to write so // it can be retried. Whether the entry survived is the answer, and it has to be asked for // rather than read off `ok`. - return StillPending(key) - ? InlineAcceptOutcome.Failed - : InlineAcceptOutcome.Accepted; + return StillPending(key) switch + { + true => InlineAcceptOutcome.Failed, + false => InlineAcceptOutcome.Accepted, + // The owner took the accept and then could not be asked what became of it. Reading + // that as applied is a guess, and the one that loses a snapshot: a caller told it was + // accepted stops offering it + null => InlineAcceptOutcome.Unknown + }; } /// @@ -177,9 +193,15 @@ public static bool Focus(string key) => Exchange(new(ViewerVerb.Focus, key), ViewerClient.ShortTimeout, out var response) && response.Ok; - static bool StillPending(string key) => - TryListKeys(out var keys) && - keys.Contains(key); + /// + /// Whether the owner still holds the entry, or null when it could not be asked - it went away, + /// or answered with an error. Three answers rather than two because "it is not pending" and + /// "there is no telling" lead to opposite reports, and a failed listing used to give the first. + /// + static bool? StillPending(string key) => + TryListKeys(out var keys) + ? keys.Contains(key) + : null; static string? Text(string? message) => message is { Length: > 0 } ? message : null; diff --git a/src/DiffEngineTray.Tests/FakeViewer.cs b/src/DiffEngineTray.Tests/FakeViewer.cs index 883ef4b0..0a480ab1 100644 --- a/src/DiffEngineTray.Tests/FakeViewer.cs +++ b/src/DiffEngineTray.Tests/FakeViewer.cs @@ -39,6 +39,12 @@ public FakeViewer(params string[] names) public string? FailureMessage { get; set; } = "the file is locked"; + /// + /// When true, listings come back as an error, which stands in for an owner that took a verb + /// and then could not be asked what it holds. + /// + public bool ListingFails { get; set; } + async Task Listen() { while (!cancel.IsCancellationRequested) @@ -68,6 +74,14 @@ string Respond(string request) Verbs.Add(key == null ? verb : $"{verb}:{key}"); var builder = new StringBuilder("version: 1\n"); + if (verb == "list" && + ListingFails) + { + builder.Append("status: error\n"); + Append(builder, "message", "the owner is going away"); + return builder.ToString(); + } + if (verb is "accept" or "discard" or "acceptall" or "discardall" && !Succeed) { diff --git a/src/DiffEngineTray.Tests/TrackerSnapshotTest.cs b/src/DiffEngineTray.Tests/TrackerSnapshotTest.cs index beaaea7a..ebab44ec 100644 --- a/src/DiffEngineTray.Tests/TrackerSnapshotTest.cs +++ b/src/DiffEngineTray.Tests/TrackerSnapshotTest.cs @@ -40,6 +40,26 @@ public async Task AcceptForwardsTheKeyAndRefreshes() await Assert.That(tracker.Snapshots[0].Name).IsEqualTo("Other.cs:1"); } + /// + /// Whether the entry survived is what tells an accept from a failure here, and that is a + /// second round trip. An owner that took the accept and then could not answer used to come + /// back as applied, because a listing that failed returned no items and no items read as an + /// empty queue. + /// + [Test] + public async Task AnAcceptThatCannotBeConfirmedIsNotCalledApplied() + { + using var viewer = new FakeViewer("Sample.cs:1"); + var host = new RemoteInlineHost(); + var snapshot = host.List().Single(); + viewer.ListingFails = true; + + var outcome = host.Accept(snapshot, out _); + + await Assert.That(outcome).IsEqualTo(AcceptOutcome.Unknown); + await Assert.That(viewer.Verbs).Contains($"accept:{snapshot.Key}"); + } + [Test] public async Task DiscardForwardsTheKey() { diff --git a/src/DiffEngineTray/RemoteInlineHost.cs b/src/DiffEngineTray/RemoteInlineHost.cs index af494847..5abf3997 100644 --- a/src/DiffEngineTray/RemoteInlineHost.cs +++ b/src/DiffEngineTray/RemoteInlineHost.cs @@ -33,17 +33,27 @@ class RemoteInlineHost : IInlineHost public string Description => $"owned by another process on port {ViewerClient.Port}"; - public IReadOnlyList List() + public IReadOnlyList List() => + TryList(out var pending) ? pending : []; + + /// + /// False when the owner could not be asked, which flattens to nothing + /// pending β€” right for a menu, and wrong for anything reading the answer as a statement about + /// a particular entry. + /// + static bool TryList(out IReadOnlyList pending) { if (!Exchange(new(ViewerVerb.List), ViewerClient.ShortTimeout, out var response) || !response.Ok) { - return []; + pending = []; + return false; } - return response.Items + pending = response.Items .Select(_ => new PendingSnapshot(_.Key, _.Name, _.Status)) .ToList(); + return true; } public IReadOnlyList? Queued() => @@ -74,7 +84,14 @@ public AcceptOutcome Accept(PendingSnapshot snapshot, out string? message) return AcceptOutcome.Failed; } - return List().Any(_ => _.Key == snapshot.Key) + if (!TryList(out var pending)) + { + // The owner took the accept and then could not be asked what became of it. Applied is + // a guess, and the one that tells the user a snapshot landed that may not have + return AcceptOutcome.Unknown; + } + + return pending.Any(_ => _.Key == snapshot.Key) ? AcceptOutcome.Failed : AcceptOutcome.Applied; } From c1bec3dbc56fd36cb788a5295c58d8890fc759ed Mon Sep 17 00:00:00 2001 From: Simon Cropp Date: Sat, 15 Aug 2026 17:55:26 +1000 Subject: [PATCH 07/16] Give a file to the nearest solution above it, not the first one cached Reuse was a scan of every already resolved directory for one the file sat under, taking the longest. Nothing looked between the file and that directory, so a file inside a nested solution was handed the outer one: the outer directory encloses it too, and only the order patches happened to arrive in decided which was cached first. The entry then showed under the wrong solution in the tray and the viewer. The cache is now consulted level by level on the way up, keyed by directory rather than by file, so the nearest solution is always the one that answers and a known directory still stops the walk without touching the disk. --- .../Inline/SolutionDirectoryFinder.cs | 61 ++++++------------- .../SolutionDirectoryFinderTests.cs | 29 +++++++++ 2 files changed, 47 insertions(+), 43 deletions(-) diff --git a/src/DiffEngine/Inline/SolutionDirectoryFinder.cs b/src/DiffEngine/Inline/SolutionDirectoryFinder.cs index 4cb9b04e..127098af 100644 --- a/src/DiffEngine/Inline/SolutionDirectoryFinder.cs +++ b/src/DiffEngine/Inline/SolutionDirectoryFinder.cs @@ -6,7 +6,13 @@ class Result(string directory, string name) public string Name { get; } = name; } - static ConcurrentDictionary cache = new(StringComparer.OrdinalIgnoreCase); + static readonly ConcurrentDictionary cache = new(StringComparer.OrdinalIgnoreCase); + + /// + /// Directories already known to hold a solution, which is what lets the walk stop early + /// without asking the disk again. + /// + static readonly ConcurrentDictionary directories = new(StringComparer.OrdinalIgnoreCase); /// /// The solution a file belongs to, or null when it has none. @@ -37,29 +43,6 @@ class Result(string directory, string name) static Result? Walk(string file) { - // Reuse an already resolved solution when the file sits inside its directory. - // Prefer the nearest (longest) enclosing directory so nested solutions resolve correctly. - Result? nearest = null; - foreach (var result in cache.Values) - { - if (result == null || - !IsInDirectory(file, result.Directory)) - { - continue; - } - - if (nearest == null || - result.Directory.Length > nearest.Directory.Length) - { - nearest = result; - } - } - - if (nearest != null) - { - return nearest; - } - var currentDirectory = Path.GetDirectoryName(file); if (string.IsNullOrEmpty(currentDirectory)) { @@ -68,6 +51,16 @@ class Result(string directory, string name) do { + // Asked level by level on the way up, so the nearest solution is always the one that + // answers. Reuse used to be a scan for any cached directory the file sat under, which + // took whichever had been resolved first: a file in a nested solution was given the + // one above it, because that directory encloses it too and nothing had yet looked + // between the two + if (directories.TryGetValue(currentDirectory, out var known)) + { + return known; + } + if (TryFind(currentDirectory, "*.slnx", out var result)) { return result; @@ -88,25 +81,6 @@ class Result(string directory, string name) } while (true); } - // True when file is directory itself or sits below it, requiring a directory-separator - // boundary so that a sibling like "AppTests" is not treated as being inside "App". - static bool IsInDirectory(string file, string directory) - { - if (!file.StartsWith(directory, StringComparison.OrdinalIgnoreCase)) - { - return false; - } - - if (file.Length == directory.Length) - { - return true; - } - - var boundary = file[directory.Length]; - return boundary == Path.DirectorySeparatorChar || - boundary == Path.AltDirectorySeparatorChar; - } - static bool TryFind(string directory, string searchPattern, [NotNullWhen(true)] out Result? result) { string[] solutions; @@ -127,6 +101,7 @@ static bool TryFind(string directory, string searchPattern, [NotNullWhen(true)] if (solutions.Length != 0) { result = new(directory, Path.GetFileNameWithoutExtension(solutions.First())); + directories[directory] = result; return true; } diff --git a/src/DiffEngineTray.Tests/SolutionDirectoryFinderTests.cs b/src/DiffEngineTray.Tests/SolutionDirectoryFinderTests.cs index 7f3d271f..37473d24 100644 --- a/src/DiffEngineTray.Tests/SolutionDirectoryFinderTests.cs +++ b/src/DiffEngineTray.Tests/SolutionDirectoryFinderTests.cs @@ -42,6 +42,35 @@ public async Task MissingDirectoryIsWalkedPast() public async Task UnusablePathIsNotFound() => await Assert.That(SolutionDirectoryFinder.Find("no\0such\\Sample.verified.txt")).IsNull(); + /// + /// A solution inside another one owns the files under it. Reuse used to be a scan of every + /// resolved directory for one the file sat under, so once the outer solution was cached it + /// answered for the inner one too β€” the outer directory encloses those files, and nothing had + /// looked between the file and it. Whichever was resolved first decided, which made the bug + /// depend on the order patches happened to arrive in. + /// + [Test] + public async Task NestedSolutionOwnsItsOwnFiles() + { + var root = Path.Combine(Path.GetTempPath(), "DiffEngineSlnFinder", Guid.NewGuid().ToString("N")); + var nested = Path.Combine(root, "submodule", "Tests"); + Directory.CreateDirectory(nested); + try + { + await File.WriteAllTextAsync(Path.Combine(root, "Outer.sln"), ""); + await File.WriteAllTextAsync(Path.Combine(root, "submodule", "Inner.slnx"), ""); + + // The outer one first, so its directory is the cached one + await Assert.That(SolutionDirectoryFinder.Find(Path.Combine(root, "Class.cs"))).IsEqualTo("Outer"); + + await Assert.That(SolutionDirectoryFinder.Find(Path.Combine(nested, "Tests.cs"))).IsEqualTo("Inner"); + } + finally + { + Directory.Delete(root, true); + } + } + [Test] public async Task SiblingWithSharedPrefixIsNotMatched() { From df9e70db293b70373482a853e8c264e7cceb3c72 Mon Sep 17 00:00:00 2001 From: Simon Cropp Date: Sat, 15 Aug 2026 17:59:26 +1000 Subject: [PATCH 08/16] Let an accept complete when a re-run repeated itself meanwhile Applying happens outside the host's lock and can wait ten seconds on the cross process mutex, and Accept throws the completion away if the entry changed while that was going on. But every fold rebuilt the entry, so a still failing test re-sending the byte identical patch inside that window counted as a change: the patch reached the file and the entry stayed pending with nothing reported, until something else settled it. A fold that adds nothing now hands the same variants back, so the accept recognises the entry it started on. The status of the last attempt is still dropped, which is what rebuilding did and what a re-run should do. --- src/DiffEngine.Tests/InlineQueueTests.cs | 54 ++++++++++++++++++++++++ src/DiffEngine/Inline/InlineQueue.cs | 51 +++++++++++++++++++--- 2 files changed, 98 insertions(+), 7 deletions(-) diff --git a/src/DiffEngine.Tests/InlineQueueTests.cs b/src/DiffEngine.Tests/InlineQueueTests.cs index 9b68a84c..7f976b02 100644 --- a/src/DiffEngine.Tests/InlineQueueTests.cs +++ b/src/DiffEngine.Tests/InlineQueueTests.cs @@ -181,6 +181,60 @@ public async Task CompletingAfterAReplaceLeavesTheNewEntry() await Assert.That(after.Items[0].Patch.NewContent).IsEqualTo("second"); } + /// + /// The other side of . Applying takes up + /// to ten seconds on the cross process mutex, and a test that is still failing re-runs and + /// re-sends the identical patch inside that window. That is not a replace: the entry says what + /// it said before, so the accept it is in the middle of still completes. + /// + /// Rebuilding the entry regardless made every one of those look like a change, so the patch + /// reached the file and the entry stayed pending with nothing said about it. + /// + /// + [Test] + public async Task CompletingAfterAnIdenticalReRunStillApplies() + { + var queue = InlineQueue.Empty.Enqueue(Patch(content: "same")); + var entry = queue.Find(InlineKey.For("Sample.cs", 42))!; + queue = queue.Enqueue(Patch(content: "same")); + + var after = queue.Accept(entry, InlineApplyResult.Applied, out var message); + + await Assert.That(message).IsEqualTo("Applied Sample.cs:42"); + await Assert.That(after.Items).IsEmpty(); + } + + /// + [Test] + public async Task CompletingAfterAnIdenticalReRunFromTheSameFrameworkStillApplies() + { + var queue = InlineQueue.Empty.Enqueue(Patch(content: "same", framework: "net8.0")); + var entry = queue.Find(InlineKey.For("Sample.cs", 42))!; + queue = queue.Enqueue(Patch(content: "same", framework: "net8.0")); + + var after = queue.Accept(entry, InlineApplyResult.Applied, out var message); + + await Assert.That(message).IsEqualTo("Applied Sample.cs:42"); + await Assert.That(after.Items).IsEmpty(); + } + + /// + /// A re-run still drops what the last attempt failed with, which is what rebuilding the entry + /// did: the content has arrived again and nothing has retried it. + /// + [Test] + public async Task AnIdenticalReRunClearsTheFailureStatus() + { + var queue = InlineQueue.Empty.Enqueue(Patch(content: "same")); + var entry = queue.Find(InlineKey.For("Sample.cs", 42))!; + queue = queue.Accept(entry, InlineApplyResult.Failed("the file is locked"), out _); + await Assert.That(queue.Items.Single().Status).IsEqualTo("the file is locked"); + + queue = queue.Enqueue(Patch(content: "same")); + + await Assert.That(queue.Items.Single().Status).IsNull(); + } + [Test] public async Task CompletingAfterASettleDoesNothing() { diff --git a/src/DiffEngine/Inline/InlineQueue.cs b/src/DiffEngine/Inline/InlineQueue.cs index 496f6a2e..be5bf755 100644 --- a/src/DiffEngine/Inline/InlineQueue.cs +++ b/src/DiffEngine/Inline/InlineQueue.cs @@ -58,6 +58,15 @@ public InlineQueue Enqueue(InlinePatch patch) return new(items); } + /// + /// A fold that added nothing. The variants are handed back as they are, so an accept applying + /// against this entry still recognises it as the one it started on; only the status of the + /// last attempt goes, which is what rebuilding the entry used to do anyway - the content + /// arrived again, and what failed before has not been retried. + /// + static PendingInline Unchanged(PendingInline entry) => + entry.Status is null ? entry : entry with { Status = null }; + static PendingInline Fold(PendingInline entry, InlinePatch patch) { var origin = patch.Framework; @@ -67,6 +76,17 @@ static PendingInline Fold(PendingInline entry, InlinePatch patch) if (origin is null || entry.Variants.All(_ => _.Origins.Count == 0)) { + // A re-run that repeats itself has changed nothing, and saying so matters: Accept + // throws away the completion of an accept whose entry changed identity while the + // patch was applying, and a still failing test re-sending the same patch is exactly + // what happens during those ten seconds + if (entry.Variants is [var only] && + only.Origins.Count == 0 && + only.Patch.Matches(patch)) + { + return Unchanged(entry); + } + return new(patch); } @@ -79,6 +99,9 @@ static PendingInline Fold(PendingInline entry, InlinePatch patch) if (matching >= 0) { var folded = new List(); + // Nothing to say when this framework already held this content and no other variant + // gave a label up, which is the shape a re-run repeating itself arrives in + var changed = false; for (var index = 0; index < variants.Count; index++) { var variant = variants[index]; @@ -86,24 +109,38 @@ static PendingInline Fold(PendingInline entry, InlinePatch patch) { // The existing patch instance, deliberately: the content is identical, and a // reader caching per patch keeps its work. - folded.Add(variant.Origins.Contains(origin) - ? variant - : variant with { Origins = [.. variant.Origins, origin] }); + if (variant.Origins.Contains(origin)) + { + folded.Add(variant); + } + else + { + folded.Add(variant with { Origins = [.. variant.Origins, origin] }); + changed = true; + } + continue; } var stripped = variant.Origins.Where(_ => _ != origin).ToList(); if (stripped.Count == 0) { + changed = true; continue; } - folded.Add(stripped.Count == variant.Origins.Count - ? variant - : variant with { Origins = stripped }); + if (stripped.Count == variant.Origins.Count) + { + folded.Add(variant); + } + else + { + folded.Add(variant with { Origins = stripped }); + changed = true; + } } - return new(folded); + return changed ? new(folded) : Unchanged(entry); } // This framework previously produced different content: its variant updates in place when From 6382f8a19d8c4cb210b5d313379051c8932896f1 Mon Sep 17 00:00:00 2001 From: Simon Cropp Date: Sat, 15 Aug 2026 18:04:43 +1000 Subject: [PATCH 09/16] Make the mid-apply replace test replace something AReplacedEntrySurvivesTheAcceptItInterrupted re-queued the call site with the content it already had, so it was asserting the guard against a re-run that had not in fact replaced anything - which is the case the previous commit deliberately changed. It now re-queues different content, which is what its own summary describes, and the identical arrival gets its own test beside it. Amends the previous commit rather than reporting green over it: that commit left this red. --- .../OwnedInlineHostTest.cs | 45 +++++++++++++++++-- 1 file changed, 42 insertions(+), 3 deletions(-) diff --git a/src/DiffEngineTray.Tests/OwnedInlineHostTest.cs b/src/DiffEngineTray.Tests/OwnedInlineHostTest.cs index 94305cff..5cb58fd8 100644 --- a/src/DiffEngineTray.Tests/OwnedInlineHostTest.cs +++ b/src/DiffEngineTray.Tests/OwnedInlineHostTest.cs @@ -318,13 +318,15 @@ public async Task AReplacedEntrySurvivesTheAcceptItInterrupted() }); try { - owner.Queue(); + owner.Queue(content: "first"); var snapshot = owner.Host.List().Single(); var accepting = Task.Run(() => owner.Send(new(ViewerVerb.Accept, snapshot.Key))); applying.Wait(TimeSpan.FromSeconds(5)); - // The same call site again, mid apply, as a re-run of the test would send it. - owner.Queue(); + // The same call site again, mid apply, saying something else: a re-run of a test + // whose result moved on. What was applied is the old content, so the outcome + // describes nothing that is pending now + owner.Queue(content: "second"); release.Set(); var accepted = await accepting; @@ -337,6 +339,43 @@ public async Task AReplacedEntrySurvivesTheAcceptItInterrupted() } } + /// + /// The same window, and the far more common arrival in it: a still failing test re-running and + /// producing what it produced before. Nothing was replaced, so the accept completes and the + /// entry goes - rather than the patch reaching the file while the entry stays pending with + /// nothing said about it. + /// + [Test] + public async Task AnIdenticalReRunDoesNotInterruptTheAcceptItLandsIn() + { + using var applying = new ManualResetEventSlim(); + using var release = new ManualResetEventSlim(); + using var owner = new Owner(_ => + { + applying.Set(); + release.Wait(TimeSpan.FromSeconds(10)); + return InlineApplyResult.Applied; + }); + try + { + owner.Queue(content: "same"); + var snapshot = owner.Host.List().Single(); + var accepting = Task.Run(() => owner.Send(new(ViewerVerb.Accept, snapshot.Key))); + applying.Wait(TimeSpan.FromSeconds(5)); + + owner.Queue(content: "same"); + release.Set(); + var accepted = await accepting; + + await Assert.That(accepted.Ok).IsTrue(); + await Assert.That(owner.Host.List()).IsEmpty(); + } + finally + { + release.Set(); + } + } + /// /// The ownership gate. A tray that starts while a viewer holds the port does not own the /// queue, and drives it remotely for the rest of its life instead. From 2590487e4eefc3981f089e18ee7e142014686e6e Mon Sep 17 00:00:00 2001 From: Simon Cropp Date: Sat, 15 Aug 2026 18:10:10 +1000 Subject: [PATCH 10/16] Show a patch that arrives, and stop closing the window taking the queue Two ways an owning viewer behaved unlike a tray owner, both of them the window. A patch arriving asked a tray owner for the window - "with no window open starts one; with one, this is the focus" - and asked an owning viewer for nothing. That window is hidden whenever a tray is running and the queue last emptied, so a newly failing test landed somewhere nobody could see and surfaced only as a tray icon on the next scan. "Close snapshot viewer" sent Quit, which exits the process. For a tray owned queue that is a display closing. For an owning viewer the queue is in that process's memory, so one menu item quietly threw away every pending snapshot - and they were then simply missing from the menu, a refused connection listing the same as nothing pending. It asks to hide instead, which leaves the process serving and the user where the other arrangement leaves them. IInlineHost.Close already said the queue was unaffected either way; now it is. --- .../TrayViewerSyncTest.cs | 38 +++++++++++++++++++ src/DiffEngineTray/IInlineHost.cs | 5 ++- src/DiffEngineTray/RemoteInlineHost.cs | 14 ++++++- src/DiffEngineViewer/Ipc/MessageHandler.cs | 12 +++++- 4 files changed, 64 insertions(+), 5 deletions(-) diff --git a/src/DiffEngineTray.Tests/TrayViewerSyncTest.cs b/src/DiffEngineTray.Tests/TrayViewerSyncTest.cs index 882c6c9b..644643cd 100644 --- a/src/DiffEngineTray.Tests/TrayViewerSyncTest.cs +++ b/src/DiffEngineTray.Tests/TrayViewerSyncTest.cs @@ -361,6 +361,24 @@ public async Task TrayAcceptAllEmptiesTheOwningViewer() await Assert.That(pair.Applied.Count).IsEqualTo(2); } + /// + /// A tray owner asks for the window on every patch that arrives, and an owning viewer has to + /// do the same. Its window is hidden whenever a tray is running and the queue last emptied, so + /// without this a newly failing test landed in a window nobody could see and showed up only as + /// a tray icon on the next scan. + /// + [Test] + public async Task AQueuedPatchBringsTheOwningViewerForward() + { + await using var pair = new ViewerOwned(); + + pair.Queue(sample, 1); + + await Assert.That(pair.Windows).Contains(ViewerSideWindowCommand.Focus); + // On the entry that arrived, the same as a focus verb lands + await Assert.That(pair.Viewer.Queue[pair.Viewer.Selected].Key).IsEqualTo(Key(sample, 1)); + } + [Test] public async Task TrayAcceptOfOneSnapshotLeavesTheRestInTheOwningViewer() { @@ -524,6 +542,26 @@ public async Task ASlowAcceptAllIsWaitedFor() await Assert.That(pair.Applied.Count).IsEqualTo(2); } + /// + /// "Close snapshot viewer" closes a window. Against an owning viewer it used to quit the + /// process, and the queue is in that process's memory, so the same menu item threw away every + /// pending snapshot without asking - and they were simply absent from the menu afterwards, + /// since a refused connection lists the same as nothing pending. + /// + [Test] + public async Task ClosingAnOwningViewerLeavesItsQueue() + { + await using var pair = new ViewerOwned(); + pair.Queue(sample, 1); + pair.Queue(other, 7); + + new RemoteInlineHost().Close(); + + await Assert.That(pair.Windows).Contains(ViewerSideWindowCommand.Hide); + await Assert.That(pair.Windows).DoesNotContain(ViewerSideWindowCommand.Close); + await Assert.That(pair.Viewer.Keys()).IsEquivalentTo([Key(sample, 1), Key(other, 7)]); + } + [Test] public async Task AConflictIsRefusedTheSameWayFromTheTray() { diff --git a/src/DiffEngineTray/IInlineHost.cs b/src/DiffEngineTray/IInlineHost.cs index 5e64d665..8ceec101 100644 --- a/src/DiffEngineTray/IInlineHost.cs +++ b/src/DiffEngineTray/IInlineHost.cs @@ -34,8 +34,9 @@ interface IInlineHost void Focus(PendingSnapshot snapshot); /// - /// Close the window. The queue is unaffected either way: an owning viewer exits with it, and a - /// tray owned queue simply loses its display until something reopens one. + /// Close the window. The queue is unaffected either way: a tray owned queue loses its display + /// until something reopens one, and an owning viewer is asked to hide rather than to quit, + /// since quitting would take the queue it holds with it. /// void Close(); } diff --git a/src/DiffEngineTray/RemoteInlineHost.cs b/src/DiffEngineTray/RemoteInlineHost.cs index 5abf3997..26397e5b 100644 --- a/src/DiffEngineTray/RemoteInlineHost.cs +++ b/src/DiffEngineTray/RemoteInlineHost.cs @@ -116,8 +116,20 @@ public void DiscardAll() => public void Focus(PendingSnapshot snapshot) => Send(ViewerVerb.Focus, snapshot.Key, ViewerClient.ShortTimeout, out _); + /// + /// Hidden rather than quit, because this owner is holding the queue. Quit exits the process, + /// and the queue is in its memory, so one menu item meant "close the window" in the arrangement + /// where the tray owns the queue and "throw away every pending snapshot, without asking" in + /// this one - after which they were simply gone from the menu, a refused connection being + /// indistinguishable from nothing pending. + /// + /// Hiding leaves the process serving, which it has to be for the queue to survive at all, and + /// leaves the user where the other arrangement leaves them: no window, and everything still + /// pending. Focus brings it back. + /// + /// public void Close() => - Send(ViewerVerb.Quit, null, ViewerClient.ShortTimeout, out _); + Send(ViewerVerb.Hide, null, ViewerClient.ShortTimeout, out _); static bool Send(ViewerVerb verb, string? key, TimeSpan wait, out string? message) { diff --git a/src/DiffEngineViewer/Ipc/MessageHandler.cs b/src/DiffEngineViewer/Ipc/MessageHandler.cs index f699702e..1ddc9f54 100644 --- a/src/DiffEngineViewer/Ipc/MessageHandler.cs +++ b/src/DiffEngineViewer/Ipc/MessageHandler.cs @@ -10,8 +10,16 @@ class MessageHandler(SessionHost host, ViewerActions actions, Action ViewerMessageHandler.Handle(this, message); - int IQueueOwner.Enqueue(InlinePatch patch) => - host.Mutate(_ => ViewerSession.EnqueueInline(_, patch)).Queue.Count; + int IQueueOwner.Enqueue(InlinePatch patch) + { + var count = host.Mutate(_ => ViewerSession.EnqueueInline(_, patch)).Queue.Count; + // Brought forward on the entry that arrived, which is what a tray owner does with one of + // these. Without it a patch landing in a window that is hidden - which this one is + // whenever a tray is running and the queue last emptied - showed up only as a tray icon + // on the next scan, and one landing in a window behind the editor showed up not at all + ((IQueueOwner) this).Window(WindowCommand.Focus, InlineKey.For(patch.SourceFile, patch.LineHint)); + return count; + } void IQueueOwner.Settle(string key, string? origin) => host.Mutate(_ => ViewerSession.Settle(_, key, origin)); From d81b5007542a40ebd8b6e3b3f79253c812a00231 Mon Sep 17 00:00:00 2001 From: Simon Cropp Date: Sat, 15 Aug 2026 18:11:58 +1000 Subject: [PATCH 11/16] Update the IPC tests the enqueue focus changed Three of them encoded the old behaviour incidentally rather than on purpose: they queued patches as setup and then asserted over a window command list that queueing no longer leaves empty, or over a selection that queueing no longer leaves at zero. FocusSelectsAndRaises and Quit clear the window list after their setup, so each still asserts exactly what its verb raised. AcceptTargetsTheKey- NotTheSelection now accepts the entry that is not selected - with the newest arrival selected, accepting it was no longer the case the test exists for. Amends the previous commit, which left these red. --- src/DiffEngineViewer.Tests/IpcTests.cs | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/src/DiffEngineViewer.Tests/IpcTests.cs b/src/DiffEngineViewer.Tests/IpcTests.cs index efd2f1ce..12179326 100644 --- a/src/DiffEngineViewer.Tests/IpcTests.cs +++ b/src/DiffEngineViewer.Tests/IpcTests.cs @@ -138,12 +138,14 @@ public async Task AcceptTargetsTheKeyNotTheSelection() using var fixture = new ServerFixture(); fixture.Send(Inline(Fixtures.Patch())); fixture.Send(Inline(Fixtures.Patch("OtherTests.cs", 7, null, "new"))); - await Assert.That(fixture.Host.State.Selected).IsEqualTo(0); + // A patch that arrives is selected, so the newest is what is in view + await Assert.That(fixture.Host.State.Selected).IsEqualTo(1); - fixture.Send(new(ViewerVerb.Accept, QueueEntry.KeyForInline("OtherTests.cs", 7))); + // The one that is not + fixture.Send(new(ViewerVerb.Accept, QueueEntry.KeyForInline("SampleTests.cs", 42))); await Assert.That(fixture.Host.State.Queue.Count).IsEqualTo(1); - await Assert.That(fixture.Host.State.Queue[0].Name).IsEqualTo("SampleTests.cs:42"); + await Assert.That(fixture.Host.State.Queue[0].Name).IsEqualTo("OtherTests.cs:7"); } [Test] @@ -197,13 +199,15 @@ public async Task DiscardAll() public async Task FocusSelectsAndRaises() { using var fixture = new ServerFixture(); - fixture.Send(Inline(Fixtures.Patch())); fixture.Send(Inline(Fixtures.Patch("OtherTests.cs", 7, null, "new"))); + fixture.Send(Inline(Fixtures.Patch())); + // Each of those asked for the window as well, and this is about what the verb does + fixture.Windows.Clear(); var response = fixture.Send(new(ViewerVerb.Focus, QueueEntry.KeyForInline("OtherTests.cs", 7))); await Assert.That(response.Ok).IsTrue(); - await Assert.That(fixture.Host.State.Selected).IsEqualTo(1); + await Assert.That(fixture.Host.State.Selected).IsEqualTo(0); await Assert.That(fixture.Windows).IsEquivalentTo([WindowCommand.Focus]); } @@ -228,6 +232,8 @@ public async Task Quit() { using var fixture = new ServerFixture(); fixture.Send(Inline(Fixtures.Patch())); + // The patch asked for the window on its way in + fixture.Windows.Clear(); fixture.Send(new(ViewerVerb.Quit)); From 2049947c780a13a5600fa21c9b7c61c30dddc392 Mon Sep 17 00:00:00 2001 From: Simon Cropp Date: Sat, 15 Aug 2026 18:16:02 +1000 Subject: [PATCH 12/16] Refuse a patch payload that cannot be written or read honestly Two ways the line oriented format could be taken at its word when it should not have been. sourceFile and framework ride the payload as themselves rather than base64, so a line break in either ends its line and everything after it is read as more of the payload: the six fixed lines shift, or one of the tolerantly read trailing fields is forged. Both are public settable properties, and a path may hold a line break off Windows. Build refuses rather than emitting one, since no shape of this format carries it - base64 there would be a version bump and a reader that predates it. mode went through Enum.TryParse, which takes a number as readily as a name, so "mode: 7" arrived as an InlinePatchMode that is none of them and then fell through every mode check in the patcher to behave as a Set. IsDefined settles it; the numbers that do name a mode still read. --- src/DiffEngine.Tests/InlineApplierTests.cs | 44 ++++++++++++++++++++++ src/DiffEngine/Inline/InlinePatchFile.cs | 21 +++++++++++ 2 files changed, 65 insertions(+) diff --git a/src/DiffEngine.Tests/InlineApplierTests.cs b/src/DiffEngine.Tests/InlineApplierTests.cs index 19cc431d..4db3df15 100644 --- a/src/DiffEngine.Tests/InlineApplierTests.cs +++ b/src/DiffEngine.Tests/InlineApplierTests.cs @@ -688,6 +688,50 @@ public async Task UnknownModeFails() await Assert.That(read).IsFalse(); } + // A number parses as an enum as readily as a name, so this used to arrive as an + // InlinePatchMode that is none of them and behave as a Set + [Test] + [Arguments("7")] + [Arguments("-1")] + [Arguments("99")] + public async Task UndefinedNumericModeFails(string mode) + { + var read = InlinePatchFile.TryParse($"version: 2\nsourceFile: x\nlineHint: 1\nmode: {mode}\noriginalExpression:\nnewContent: YQ==\n", out _); + await Assert.That(read).IsFalse(); + } + + // The numbers that do name a mode still read, since that is how the enum has always been + // written on the wire + [Test] + public async Task DefinedNumericModeReads() + { + var read = InlinePatchFile.TryParse($"version: 2\nsourceFile: x\nlineHint: 1\nmode: {(int) InlinePatchMode.Append}\noriginalExpression:\nnewContent: YQ==\n", out var patch); + await Assert.That(read).IsTrue(); + await Assert.That(patch!.Mode).IsEqualTo(InlinePatchMode.Append); + } + + // Both ride the payload as themselves, so a line break in either ends the line and the rest + // is read as more of the payload: the fixed lines shift, or a trailing field is forged + [Test] + public async Task ALineBreakInAFieldThatIsNotEncodedIsRefused() + { + await Assert.That( + () => InlinePatchFile.Build( + new("a\nb.cs", 1, null, "new", InlinePatchMode.Set) + { + TestName = null + })) + .Throws(); + await Assert.That( + () => InlinePatchFile.Build( + new("a.cs", 1, null, "new", InlinePatchMode.Set) + { + TestName = null, + Framework = "net8.0\noriginalValue: forged" + })) + .Throws(); + } + [Test] public async Task MissingFileFails() { diff --git a/src/DiffEngine/Inline/InlinePatchFile.cs b/src/DiffEngine/Inline/InlinePatchFile.cs index b9f2bc30..58ddc4e8 100644 --- a/src/DiffEngine/Inline/InlinePatchFile.cs +++ b/src/DiffEngine/Inline/InlinePatchFile.cs @@ -19,6 +19,14 @@ public static void Write(string path, InlinePatch patch) public static string Build(InlinePatch patch) { + // The two fields that ride the payload as themselves rather than base64, so a line break + // in either would end its line and the rest would be read as more of the payload - + // shifting the fixed lines, or forging one of the trailing ones. Both are public settable + // properties, and a path may legally hold a line break off Windows. Refused rather than + // written, because there is no shape of this format that can carry one + AgainstLineBreak(patch.SourceFile, nameof(InlinePatch.SourceFile)); + AgainstLineBreak(patch.Framework, nameof(InlinePatch.Framework)); + var expression = patch.OriginalExpression is null ? "" : Convert.ToBase64String(Encoding.UTF8.GetBytes(patch.OriginalExpression)); @@ -78,7 +86,11 @@ public static bool TryParse(string text, [NotNullWhen(true)] out InlinePatch? pa !TryValue(lines[2], "lineHint", out var lineText) || !int.TryParse(lineText, out var lineHint) || !TryValue(lines[3], "mode", out var modeText) || + // IsDefined as well, because TryParse takes a number as readily as a name: "mode: 7" + // parsed to an InlinePatchMode that is none of them, and then fell through every mode + // check in the patcher to behave as a Set !Enum.TryParse(modeText, out var mode) || + !Enum.IsDefined(typeof(InlinePatchMode), mode) || !TryValue(lines[4], "originalExpression", out var expressionBase64) || !TryValue(lines[5], "newContent", out var contentBase64)) { @@ -147,6 +159,15 @@ public static bool TryParse(string text, [NotNullWhen(true)] out InlinePatch? pa return true; } + static void AgainstLineBreak(string? value, string name) + { + if (value is not null && + (value.IndexOf('\n') != -1 || value.IndexOf('\r') != -1)) + { + throw new ArgumentException($"InlinePatch.{name} cannot contain a line break. Value: {value}"); + } + } + static bool TryValue(string line, string key, out string value) { value = ""; From 4f259e0148fda976ddc2eb83d2b42faffd654873 Mon Sep 17 00:00:00 2001 From: Simon Cropp Date: Sat, 15 Aug 2026 18:21:05 +1000 Subject: [PATCH 13/16] Fold an inline key only where the file system folds Two paths differing just in case are one file on Windows and two on Linux, and the key folded unconditionally: on Linux the second patch took over the first's entry, and settling either settled both. Folded on Windows and macOS, left alone elsewhere. Every process addressing a queue is on the one machine, so they agree about which applies. KeyFormat pinned the folded form with mixed case inputs, which would now be wrong on the Linux leg of CI rather than right everywhere. Its cases are lower case now, so it pins the separator and the line, and the folding has a test per platform beside it. --- src/DiffEngine.Tests/ViewerProtocolTests.cs | 34 ++++++++++++++++++--- src/DiffEngine/Inline/InlineKey.cs | 16 ++++++++-- 2 files changed, 43 insertions(+), 7 deletions(-) diff --git a/src/DiffEngine.Tests/ViewerProtocolTests.cs b/src/DiffEngine.Tests/ViewerProtocolTests.cs index 38ef40a2..64bf1818 100644 --- a/src/DiffEngine.Tests/ViewerProtocolTests.cs +++ b/src/DiffEngine.Tests/ViewerProtocolTests.cs @@ -97,7 +97,8 @@ public async Task EveryVerbRoundTrips() [Test] public async Task SettleCarriesTheKey() { - var payload = new ViewerMessage(ViewerVerb.Settle, InlineKey.For("Tests.cs", 42)).Build(); + // Already lower case, so what the key survives is the round trip rather than the folding + var payload = new ViewerMessage(ViewerVerb.Settle, InlineKey.For("tests.cs", 42)).Build(); await Assert.That(ViewerMessage.TryParse(payload, out var message)).IsTrue(); await Assert.That(message!.Verb).IsEqualTo(ViewerVerb.Settle); @@ -108,14 +109,39 @@ public async Task SettleCarriesTheKey() /// Settling only works if the sender and the queue owner derive the same key from the same /// call site, so the format is pinned rather than left to whatever ToLower happens to do. /// + /// These are already lower case, so they say the same thing wherever they run. [Test] + [Arguments("tests.cs", 42, "tests.cs|42")] + [Arguments(@"c:\repo\some.tests\sample.cs", 1, @"c:\repo\some.tests\sample.cs|1")] + [Arguments("/home/user/sample.cs", 9999, "/home/user/sample.cs|9999")] + public async Task KeyFormat(string sourceFile, int line, string expected) => + await Assert.That(InlineKey.For(sourceFile, line)).IsEqualTo(expected); + + /// + /// A Windows path reaches here from several sources with several casings, and every one of + /// them is the same call site. + /// + [Test] + [RunOn(TUnit.Core.Enums.OS.Windows)] [Arguments("Tests.cs", 42, "tests.cs|42")] [Arguments(@"C:\Repo\Some.Tests\Sample.cs", 1, @"c:\repo\some.tests\sample.cs|1")] - [Arguments("/home/user/Sample.cs", 9999, "/home/user/sample.cs|9999")] [Arguments("MiXeDCase.CS", 7, "mixedcase.cs|7")] - public async Task KeyFormat(string sourceFile, int line, string expected) => + public async Task KeyIsFoldedWhereThePathsAre(string sourceFile, int line, string expected) => await Assert.That(InlineKey.For(sourceFile, line)).IsEqualTo(expected); + /// + /// And not where they are not. On Linux these are two files, and one key for both meant the + /// second patch took over the first's entry and settling either settled both. + /// + [Test] + [RunOn(TUnit.Core.Enums.OS.Linux)] + public async Task KeysDifferingOnlyInCaseStayApartWhereTheFilesDo() + { + await Assert.That(InlineKey.For("/home/user/Sample.cs", 1)).IsEqualTo("/home/user/Sample.cs|1"); + await Assert.That(InlineKey.For("/home/user/sample.cs", 1)) + .IsNotEqualTo(InlineKey.For("/home/user/Sample.cs", 1)); + } + /// /// A newer sender can add a field without breaking an older owner. /// @@ -219,7 +245,7 @@ public async Task MetadataRidesTheInlineBody() [Test] public async Task SettleCarriesTheOriginInTheBody() { - var payload = new ViewerMessage(ViewerVerb.Settle, InlineKey.For("Tests.cs", 42), "net9.0").Build(); + var payload = new ViewerMessage(ViewerVerb.Settle, InlineKey.For("tests.cs", 42), "net9.0").Build(); await Assert.That(ViewerMessage.TryParse(payload, out var message)).IsTrue(); await Assert.That(message!.Key).IsEqualTo("tests.cs|42"); diff --git a/src/DiffEngine/Inline/InlineKey.cs b/src/DiffEngine/Inline/InlineKey.cs index 420f8fa6..465d3e5c 100644 --- a/src/DiffEngine/Inline/InlineKey.cs +++ b/src/DiffEngine/Inline/InlineKey.cs @@ -12,9 +12,19 @@ namespace DiffEngine; public static class InlineKey { /// - /// Case folded, because Windows paths reach here from different sources with different casing - /// and the same call site must produce one entry. + /// Case folded where the file system is, because a Windows path reaches here from different + /// sources with different casing and the same call site has to produce one entry. + /// + /// Not where it is not. On Linux two paths differing only in case are two files, and folding + /// them gave both one key: the second patch took over the first's entry, and settling either + /// settled both. Every process addressing a queue is on the one machine, so they agree about + /// which of these applies. + /// /// public static string For(string sourceFile, int line) => - $"{sourceFile.ToLowerInvariant()}|{line}"; + $"{(caseInsensitivePaths ? sourceFile.ToLowerInvariant() : sourceFile)}|{line}"; + + static readonly bool caseInsensitivePaths = + RuntimeInformation.IsOSPlatform(OSPlatform.Windows) || + RuntimeInformation.IsOSPlatform(OSPlatform.OSX); } From 509604116208a607fa98ae15ddd4842936252cf3 Mon Sep 17 00:00:00 2001 From: Simon Cropp Date: Sat, 15 Aug 2026 18:21:05 +1000 Subject: [PATCH 14/16] End three lifetimes that were being left to the finaliser or to luck ProcessViewerLauncher overwrote its Process on every relaunch without disposing the one before it, over a tray lifetime measured in weeks. Program disposed the server task with a using. Task.Dispose throws for a task that has not completed, so anything thrown between starting it and the await at the end of the method came out as an InvalidOperationException about the wrong thing. A task needs no disposal; cancelling ends it. OwnerLink.Run had no catch, and it runs on a task nothing awaits until shutdown. A throw faulted it unobserved and left a live window showing a queue that had stopped being read - which looks exactly like a quiet queue, so it is the worst of the outcomes available. It now reports through the same channel an owner that went away uses. No test: the known throw sources inside the pump are already defended, and there is no seam to inject one through that would not be testing the seam. --- src/DiffEngineTray/IViewerLauncher.cs | 5 +++++ src/DiffEngineTray/Program.cs | 6 +++++- src/DiffEngineViewer/Ipc/OwnerLink.cs | 21 +++++++++++++++++++++ 3 files changed, 31 insertions(+), 1 deletion(-) diff --git a/src/DiffEngineTray/IViewerLauncher.cs b/src/DiffEngineTray/IViewerLauncher.cs index 487c92de..db802332 100644 --- a/src/DiffEngineTray/IViewerLauncher.cs +++ b/src/DiffEngineTray/IViewerLauncher.cs @@ -22,7 +22,12 @@ sealed class ProcessViewerLauncher : IViewerLauncher public bool Launch() { + // The one it replaces has exited - Launch is only reached when Running said so - but the + // handle it was read through has not gone anywhere. The tray runs for weeks, and every + // relaunch over that time used to leave one behind for a finaliser to get to eventually + var previous = viewer; viewer = ViewerLauncher.LaunchAttached(); + previous?.Dispose(); return viewer is not null; } } diff --git a/src/DiffEngineTray/Program.cs b/src/DiffEngineTray/Program.cs index 1bb2ab9c..c5684f14 100644 --- a/src/DiffEngineTray/Program.cs +++ b/src/DiffEngineTray/Program.cs @@ -90,7 +90,11 @@ void Warn(string message) => owned.Start(); } - using var task = StartServer(tracker, cancel); + // Not a using. Anything throwing between here and the await below would dispose a task + // that is still running, and Task.Dispose throws for one that has not completed - which + // would replace whatever actually went wrong with an InvalidOperationException. A task + // needs no disposal anyway; cancelling it is what ends it + var task = StartServer(tracker, cancel); using var keyRegister = new KeyRegister(icon.Handle()); ReBindKeys(settings, keyRegister, tracker); diff --git a/src/DiffEngineViewer/Ipc/OwnerLink.cs b/src/DiffEngineViewer/Ipc/OwnerLink.cs index 02929d14..1d800e2c 100644 --- a/src/DiffEngineViewer/Ipc/OwnerLink.cs +++ b/src/DiffEngineViewer/Ipc/OwnerLink.cs @@ -87,6 +87,27 @@ public bool Pump(out bool sent) } public void Run(Cancel cancel) + { + try + { + Pump(cancel); + } + catch (Exception exception) + when (exception is not OperationCanceledException) + { + // This runs on a task nothing awaits until shutdown, so a throw here used to fault it + // unobserved and leave a live window showing a queue that had stopped being read - + // the worst of the available outcomes, since it looks exactly like a quiet queue. Said + // out loud instead, through the same channel an owner that went away uses + host.Mutate(_ => _ with + { + Message = $"The queue owner could not be read: {exception.Message}", + Exit = true + }); + } + } + + void Pump(Cancel cancel) { while (!cancel.IsCancellationRequested) { From de9d6edd1c20bb0056ba48f1487c6b6986c91634 Mon Sep 17 00:00:00 2001 From: Simon Cropp Date: Sat, 15 Aug 2026 18:25:44 +1000 Subject: [PATCH 15/16] Stop one wedged piper client holding up every other process Connections were accepted and read one at a time, and the read ran until the client closed its stream. A test process that connected and stopped partway through writing therefore held the accept loop for as long as it stayed that way, and every move and delete from every other process on the machine went nowhere. Nothing timed it out. Each connection is now handled on its own task, which is how ViewerServer has always taken its connections, and the read has ten seconds to finish. Concurrency is safe here: the callbacks land in the tracker's concurrent collections, which the viewer port already writes to off this thread. Nothing awaits the per-connection task, so it reports what it catches rather than throwing into nowhere. --- src/DiffEngineTray.Tests/PiperTest.cs | 26 +++++++ src/DiffEngineTray/PiperServer.cs | 104 +++++++++++++++++++------- 2 files changed, 104 insertions(+), 26 deletions(-) diff --git a/src/DiffEngineTray.Tests/PiperTest.cs b/src/DiffEngineTray.Tests/PiperTest.cs index 7a64104b..32e17ec2 100644 --- a/src/DiffEngineTray.Tests/PiperTest.cs +++ b/src/DiffEngineTray.Tests/PiperTest.cs @@ -130,6 +130,32 @@ public async Task ClientDisconnectsAbruptly() await Assert.That(received!.File).IsEqualTo("Foo"); } + /// + /// A client that connects and never finishes sending β€” a test process wedged mid write β€” used + /// to hold the one accept loop for as long as it stayed that way, so every move and delete + /// from every other process on the machine went nowhere and nothing timed the wait out. + /// + [Test] + public async Task AClientThatStopsSendingDoesNotBlockTheNextOne() + { + DeletePayload? received = null; + using var source = new CancelSource(); + var task = PiperServer.Start(_ => { }, _ => received = _, source.Token); + + // Connected, wrote nothing, and holds the stream open for the rest of the test + using var wedged = new TcpClient(); + await wedged.ConnectAsync(IPAddress.Loopback, PiperClient.Port, source.Token); + await using var held = wedged.GetStream(); + + await PiperClient.SendDeleteAsync("Foo", source.Token); + await Task.Delay(1000, source.Token); + await source.CancelAsync(); + await task; + + await Assert.That(received).IsNotNull(); + await Assert.That(received!.File).IsEqualTo("Foo"); + } + [Test] public async Task SendOnly() { diff --git a/src/DiffEngineTray/PiperServer.cs b/src/DiffEngineTray/PiperServer.cs index e751441e..4a77cae9 100644 --- a/src/DiffEngineTray/PiperServer.cs +++ b/src/DiffEngineTray/PiperServer.cs @@ -30,6 +30,9 @@ public static async Task Start( { listener = new(IPAddress.Loopback, PiperClient.Port); listener.Start(); + // Kept from when the accept lived inside the per-connection method: cancelling stops + // the listener, which is what brings a pending accept down with it + await using var registration = cancel.Register(listener.Stop); while (true) { @@ -40,7 +43,13 @@ public static async Task Start( try { - await Handle(listener, move, delete, cancel); + var client = await listener.AcceptTcpClientAsync(cancel); + // On its own task, the way ViewerServer takes its connections. Handled in + // turn, one client that connected and never closed its stream held up every + // move and delete from every other process for as long as it stayed that way, + // with nothing to end the wait. The callbacks are the tracker's concurrent + // collections, which the viewer port already writes to off this thread + _ = Handle(client, move, delete, cancel); } catch (TaskCanceledException) { @@ -73,39 +82,82 @@ public static async Task Start( } } - static async Task Handle(TcpListener listener, Action move, Action delete, Cancel cancel) + /// + /// How long one client gets to send its payload and close. A client is expected to write and + /// go, so anything near this is one that has stopped rather than one that is slow, and the + /// read has to end by itself: nothing else here will end it. + /// + static readonly TimeSpan readTimeout = TimeSpan.FromSeconds(10); + + /// + /// Nothing awaits this, so it reports rather than throws β€” an unobserved throw on the + /// finaliser thread is not a way to hear about a dropped move. + /// + static async Task Handle(TcpClient client, Action move, Action delete, Cancel cancel) { - await using (cancel.Register(listener.Stop)) + try { - using var client = await listener.AcceptTcpClientAsync(cancel); - using var reader = new StreamReader(client.GetStream()); - - var payload = await reader.ReadToEndAsync(cancel); - - if (payload.Contains("\"Type\":\"Move\"") || - payload.Contains("\"Type\": \"Move\"")) - { - move(Serializer.Deserialize(payload)); - } - else if (payload.Contains("\"Type\":\"Delete\"") || - payload.Contains("\"Type\": \"Delete\"")) - { - delete(Serializer.Deserialize(payload)); - } - else + using (client) { - if (payload.Length > 0) + using var reader = new StreamReader(client.GetStream()); + using var deadline = CancellationTokenSource.CreateLinkedTokenSource(cancel); + deadline.CancelAfter(readTimeout); + + string payload; + try { - // Tolerate payloads from newer clients so future additions dont - // surface an error dialog on this tray version - Log.Error("Received unknown payload type. Ignoring. Payload: {payload}", payload); + payload = await reader.ReadToEndAsync(deadline.Token); + } + catch (OperationCanceledException) + when (!cancel.IsCancellationRequested) + { + Log.Error("A client connected and did not finish sending within {timeout}. Ignoring it.", readTimeout); + return; } - } - if (client.Connected) + Dispatch(payload, move, delete); + } + } + catch (Exception exception) + when (exception is OperationCanceledException or ObjectDisposedException) + { + // Shutting down, or the socket went with it + } + catch (IOException exception) + when (exception.InnerException is SocketException {SocketErrorCode: SocketError.ConnectionReset}) + { + //client disconnected abruptly, e.g. test was canceled + } + catch (Exception exception) + { + if (!cancel.IsCancellationRequested) { - client.Close(); + ExceptionHandler.Handle("Failed to receive payload", exception); } } } + + static void Dispatch(string payload, Action move, Action delete) + { + if (payload.Contains("\"Type\":\"Move\"") || + payload.Contains("\"Type\": \"Move\"")) + { + move(Serializer.Deserialize(payload)); + return; + } + + if (payload.Contains("\"Type\":\"Delete\"") || + payload.Contains("\"Type\": \"Delete\"")) + { + delete(Serializer.Deserialize(payload)); + return; + } + + if (payload.Length > 0) + { + // Tolerate payloads from newer clients so future additions dont + // surface an error dialog on this tray version + Log.Error("Received unknown payload type. Ignoring. Payload: {payload}", payload); + } + } } \ No newline at end of file From 4a51b35727fa590e83cafaab1277aeccb331dbc5 Mon Sep 17 00:00:00 2001 From: Simon Cropp Date: Sat, 15 Aug 2026 18:31:12 +1000 Subject: [PATCH 16/16] Step over an interpolation hole in a raw string rather than through it The raw string scan looked for a closing run of quotes and treated holes as content, so a literal inside one ended the outer string at its delimiter. Where the runs happen to pair up evenly the rest re-pairs and lands back on its feet, which is why this took a four-quoted literal holding a run of three to show: the last run then opens a string that runs to the end of the file, and the Snapshot call after it is inside that string as far as every search is concerned. Holes are now skipped whole, through the TrySkipHole the regular interpolated branch already uses - it lexes nested literals, char literals and comments properly. A run of fewer braces than dollars is still content, which is how a raw interpolated string carries a literal brace. F# is left alone deliberately. Its triple-quoted delimiter is exactly three and cannot hold a run of three, so its runs always pair and the mis-pairing this fixes has no way to start. --- src/DiffEngine.Tests/InlinePatcherTests.cs | 46 ++++++++++++++++++++++ src/DiffEngine/Inline/CsLanguage.cs | 42 ++++++++++++++++++-- 2 files changed, 85 insertions(+), 3 deletions(-) diff --git a/src/DiffEngine.Tests/InlinePatcherTests.cs b/src/DiffEngine.Tests/InlinePatcherTests.cs index 3075c91b..60df1557 100644 --- a/src/DiffEngine.Tests/InlinePatcherTests.cs +++ b/src/DiffEngine.Tests/InlinePatcherTests.cs @@ -31,6 +31,52 @@ public async Task ReplaceRawLiteral() await Assert.That(newSource.EndsWith(");\n }\n}")).IsTrue(); } + /// + /// An interpolated raw string whose hole holds a literal of its own. Skipping holes as content + /// ended the outer string at the inner delimiter, after which the rest of the line lexed as + /// code and a stray delimiter opened a string that ran on and swallowed the real call. + /// + [Test] + public async Task ARawInterpolatedStringWithALiteralInItsHoleIsSteppedOverWhole() + { + var source = Method($"{rawInterpolated}\n await Snapshot(\"old\");"); + var status = TryApply(source, 6, InlinePatchMode.Set, "\"old\"", "new", out var newSource, out var reason); + + await Assert.That(status).IsEqualTo(PatchStatus.Applied); + await Assert.That(reason).IsEmpty(); + await Assert.That(newSource).Contains("Snapshot(\"new\")"); + // And the literal it stepped over is untouched + await Assert.That(newSource).Contains(rawInterpolated); + } + + /// + /// The same shape with the call before it, so the scan has to get past the literal rather than + /// stop short of it. + /// + [Test] + public async Task ASnapshotBeforeARawInterpolatedStringIsStillFound() + { + var source = Method($" await Snapshot(\"old\");\n{rawInterpolated}"); + var status = TryApply(source, 5, InlinePatchMode.Set, "\"old\"", "new", out var newSource, out _); + + await Assert.That(status).IsEqualTo(PatchStatus.Applied); + await Assert.That(newSource).Contains("Snapshot(\"new\")"); + } + + // Written by concatenation because it holds runs of three and four quotes, which no raw string + // in this file can carry without a wider delimiter than the thing being described. + // + // The literal in the hole is four-quoted and holds a run of three, so the runs no longer pair + // up evenly. That matters: with a hole skipped as content, the blind scan ends the outer + // string at the first run of three or more and then re-pairs the rest, which for evenly + // matched runs lands back on its feet and hides the bug. Here the last run opens a string that + // runs to the end of the file, taking the Snapshot call with it + const string rawInterpolated = + " var text = $" + q3 + "{Render(" + q4 + "has " + q3 + " inside" + q4 + ")}" + q3 + ";"; + + const string q3 = "\"\"\""; + const string q4 = "\"\"\"\""; + [Test] public async Task ReplaceRegularLiteral() { diff --git a/src/DiffEngine/Inline/CsLanguage.cs b/src/DiffEngine/Inline/CsLanguage.cs index 313d3f1f..cf09d69c 100644 --- a/src/DiffEngine/Inline/CsLanguage.cs +++ b/src/DiffEngine/Inline/CsLanguage.cs @@ -210,8 +210,11 @@ static bool TrySkipStringLike(string source, ref int index) var quotes = QuoteRun(source, cursor); if (quotes >= 3) { - // Raw string (interpolated or not): skip blindly to a closing run of >= quotes. - // Interpolation holes are skipped as part of the content. + // Raw string: scan to a closing run of >= quotes, stepping over any interpolation + // hole whole. Skipping holes as content read the quotes of a literal inside one - + // $"""{Render("""x""")}""" - as the end of the outer string, after which the rest of + // the line lexed as code and a stray delimiter opened a string that could swallow a + // real call var search = cursor + quotes; while (true) { @@ -221,7 +224,28 @@ static bool TrySkipStringLike(string source, ref int index) return true; } - if (source[search] != '"') + var ch = source[search]; + if (dollars > 0 && ch == '{') + { + // A run of fewer than one brace per dollar is content, which is how a raw + // interpolated string carries a literal brace + var braces = BraceRun(source, search); + if (braces < dollars) + { + search += braces; + continue; + } + + if (!TrySkipHole(source, ref search)) + { + index = source.Length; + return true; + } + + continue; + } + + if (ch != '"') { search++; continue; @@ -362,6 +386,18 @@ static bool TrySkipHole(string source, ref int cursor) return false; } + static int BraceRun(string source, int index) + { + var count = 0; + while (index + count < source.Length && + source[index + count] == '{') + { + count++; + } + + return count; + } + static int QuoteRun(string source, int index) { var count = 0;