From 82f77f48ee5dda8e6011e966afddfcb70bd0a63f Mon Sep 17 00:00:00 2001 From: Tyrie Vella Date: Wed, 10 Jun 2026 15:49:38 -0700 Subject: [PATCH 1/2] Fix flaky ModifiedPathsTests by replaying the on-disk log The modified-paths database file is an append-only log of "A path" / "D path" entries that ModifiedPathsDatabase compacts at the end of each background-op batch via WriteAllEntriesAndFlush in PostBackgroundOperation. BackgroundOperationCount, however, drops to 0 inside DequeueAndFlush for the last task -- before the post-callback runs. WaitForBackgroundOperations polls until count == 0 and can therefore return between dequeue and compaction, leaving the file in a state like: A temp.txt D temp.txt ModifiedPathsShouldNotContain matched both lines, fed two results into EnumerableShouldExtensions.ShouldNotContain, and the SingleOrDefault call threw "Sequence contains more than one matching element" -- masking the underlying race with a confusing exception. The product code is correct: FileBasedCollection.TryLoadFromDisk replays the A/D log on mount, so the on-disk state is always recoverable. This change fixes the tests: * GVFSHelpers.ModifiedPathsShouldContain / ModifiedPathsShouldNotContain now build the current set by replaying the A/D log (same algorithm as TryLoadFromDisk) and check membership in that set. This matches the semantic intent of every existing caller and is robust to the flush-race window. * ModifiedPathsContentsShouldEqual is renamed to ModifiedPathsRawFileContentsShouldEqual and gains an XML doc comment pointing callers at the semantic helpers unless they specifically need to validate the compacted file layout. The two existing callers in CheckoutTests are updated. * EnumerableShouldExtensions.ShouldNotContain uses Where(predicate) instead of SingleOrDefault(predicate) so multi-match cases produce a useful Assert.Fail message instead of InvalidOperationException. * A new ModifiedPathsDatabaseTests.AddFollowedByDeleteIsRecoveredOnLoad unit test pins down the recovery contract -- loading "A temp.txt\r\nD temp.txt\r\n" produces an empty set (only the auto-added .gitattributes entry). Assisted-by: Claude Opus 4.7 Signed-off-by: Tyrie Vella --- .../Tests/GitCommands/CheckoutTests.cs | 4 +- .../GVFS.FunctionalTests/Tools/GVFSHelpers.cs | 57 +++++++++++++++---- .../Should/EnumerableShouldExtensions.cs | 11 +++- .../Common/ModifiedPathsDatabaseTests.cs | 18 ++++++ 4 files changed, 74 insertions(+), 16 deletions(-) diff --git a/GVFS/GVFS.FunctionalTests/Tests/GitCommands/CheckoutTests.cs b/GVFS/GVFS.FunctionalTests/Tests/GitCommands/CheckoutTests.cs index e6de487b7..6a2c61c33 100644 --- a/GVFS/GVFS.FunctionalTests/Tests/GitCommands/CheckoutTests.cs +++ b/GVFS/GVFS.FunctionalTests/Tests/GitCommands/CheckoutTests.cs @@ -248,7 +248,7 @@ public void CheckoutBranchAfterReadingFileAndVerifyContentsCorrect() this.FilesShouldMatchCheckoutOfSourceBranch(); // Verify modified paths contents - GVFSHelpers.ModifiedPathsContentsShouldEqual(this.Enlistment, this.FileSystem, "A .gitattributes" + GVFSHelpers.ModifiedPathsNewLine); + GVFSHelpers.ModifiedPathsRawFileContentsShouldEqual(this.Enlistment, this.FileSystem, "A .gitattributes" + GVFSHelpers.ModifiedPathsNewLine); } [TestCase] @@ -266,7 +266,7 @@ public void CheckoutBranchAfterReadingAllFilesAndVerifyContentsCorrect() .WithDeepStructure(this.FileSystem, this.ControlGitRepo.RootPath, compareContent: true, withinPrefixes: this.pathPrefixes); // Verify modified paths contents - GVFSHelpers.ModifiedPathsContentsShouldEqual(this.Enlistment, this.FileSystem, "A .gitattributes" + GVFSHelpers.ModifiedPathsNewLine); + GVFSHelpers.ModifiedPathsRawFileContentsShouldEqual(this.Enlistment, this.FileSystem, "A .gitattributes" + GVFSHelpers.ModifiedPathsNewLine); } [TestCase] diff --git a/GVFS/GVFS.FunctionalTests/Tools/GVFSHelpers.cs b/GVFS/GVFS.FunctionalTests/Tools/GVFSHelpers.cs index d943035fb..5d73fe490 100644 --- a/GVFS/GVFS.FunctionalTests/Tools/GVFSHelpers.cs +++ b/GVFS/GVFS.FunctionalTests/Tools/GVFSHelpers.cs @@ -165,7 +165,17 @@ public static string ReadAllTextFromWriteLockedFile(string filename) } } - public static void ModifiedPathsContentsShouldEqual(GVFSFunctionalTestEnlistment enlistment, FileSystemRunner fileSystem, string contents) + /// + /// Asserts that the on-disk modified-paths file's raw contents are + /// exactly equal to , including the A/D + /// log prefixes and line terminators. Most callers want + /// / + /// instead, which compare + /// against the semantic set after replaying the log. Only use this + /// helper when the test specifically needs to validate the compacted + /// file layout (e.g. confirming a checkout left no stray entries). + /// + public static void ModifiedPathsRawFileContentsShouldEqual(GVFSFunctionalTestEnlistment enlistment, FileSystemRunner fileSystem, string contents) { string modifedPathsContents = GetModifiedPathsContents(enlistment, fileSystem); modifedPathsContents.ShouldEqual(contents); @@ -173,26 +183,19 @@ public static void ModifiedPathsContentsShouldEqual(GVFSFunctionalTestEnlistment public static void ModifiedPathsShouldContain(GVFSFunctionalTestEnlistment enlistment, FileSystemRunner fileSystem, params string[] gitPaths) { - string modifedPathsContents = GetModifiedPathsContents(enlistment, fileSystem); - string[] modifedPathLines = modifedPathsContents.Split(new[] { ModifiedPathsNewLine }, StringSplitOptions.None); + HashSet currentPaths = GetCurrentModifiedPaths(enlistment, fileSystem); foreach (string gitPath in gitPaths) { - modifedPathLines.ShouldContain(path => path.Equals(ModifedPathsLineAddPrefix + gitPath, FileSystemHelpers.PathComparison)); + currentPaths.ShouldContain(path => path.Equals(gitPath, FileSystemHelpers.PathComparison)); } } public static void ModifiedPathsShouldNotContain(GVFSFunctionalTestEnlistment enlistment, FileSystemRunner fileSystem, params string[] gitPaths) { - string modifedPathsContents = GetModifiedPathsContents(enlistment, fileSystem); - string[] modifedPathLines = modifedPathsContents.Split(new[] { ModifiedPathsNewLine }, StringSplitOptions.None); + HashSet currentPaths = GetCurrentModifiedPaths(enlistment, fileSystem); foreach (string gitPath in gitPaths) { - modifedPathLines.ShouldNotContain( - path => - { - return path.Equals(ModifedPathsLineAddPrefix + gitPath, FileSystemHelpers.PathComparison) || - path.Equals(ModifedPathsLineDeletePrefix + gitPath, FileSystemHelpers.PathComparison); - }); + currentPaths.ShouldNotContain(path => path.Equals(gitPath, FileSystemHelpers.PathComparison)); } } @@ -230,6 +233,36 @@ private static string GetModifiedPathsContents(GVFSFunctionalTestEnlistment enli return GVFSHelpers.ReadAllTextFromWriteLockedFile(modifiedPathsDatabase); } + /// + /// Returns the set of currently-modified paths by replaying the on-disk + /// modified paths log. The file is append-only between background-op + /// batches; + /// compacts it after each batch finishes. Because + /// + /// can return after the last task is dequeued but before that compaction + /// completes, callers must replay the A/D log entries the same way + /// does on mount to + /// observe a consistent state. + /// + private static HashSet GetCurrentModifiedPaths(GVFSFunctionalTestEnlistment enlistment, FileSystemRunner fileSystem) + { + string contents = GetModifiedPathsContents(enlistment, fileSystem); + HashSet paths = new HashSet(FileSystemHelpers.PathComparer); + foreach (string line in contents.Split(new[] { ModifiedPathsNewLine }, StringSplitOptions.RemoveEmptyEntries)) + { + if (line.StartsWith(ModifedPathsLineAddPrefix, StringComparison.Ordinal)) + { + paths.Add(line.Substring(ModifedPathsLineAddPrefix.Length)); + } + else if (line.StartsWith(ModifedPathsLineDeletePrefix, StringComparison.Ordinal)) + { + paths.Remove(line.Substring(ModifedPathsLineDeletePrefix.Length)); + } + } + + return paths; + } + private static T RunSqliteCommand(string sqliteDbPath, Func runCommand) { string connectionString = $"data source={sqliteDbPath}"; diff --git a/GVFS/GVFS.Tests/Should/EnumerableShouldExtensions.cs b/GVFS/GVFS.Tests/Should/EnumerableShouldExtensions.cs index efe65bb0a..c8a28fd42 100644 --- a/GVFS/GVFS.Tests/Should/EnumerableShouldExtensions.cs +++ b/GVFS/GVFS.Tests/Should/EnumerableShouldExtensions.cs @@ -47,8 +47,15 @@ public static T ShouldContainSingle(this IEnumerable group, Func public static void ShouldNotContain(this IEnumerable group, Func predicate) { - T item = group.SingleOrDefault(predicate); - item.ShouldEqual(default(T), "Unexpected matching entry found in {" + string.Join(",", group) + "}"); + List matches = group.Where(predicate).ToList(); + if (matches.Count != 0) + { + Assert.Fail("Unexpected matching {0} {1}: {2} found in {{{3}}}", + matches.Count, + matches.Count == 1 ? "entry" : "entries", + string.Join(",", matches), + string.Join(",", group)); + } } public static IEnumerable ShouldNotContain(this IEnumerable group, IEnumerable unexpectedValues, Func predicate) diff --git a/GVFS/GVFS.UnitTests/Common/ModifiedPathsDatabaseTests.cs b/GVFS/GVFS.UnitTests/Common/ModifiedPathsDatabaseTests.cs index 0f7682830..9037592f0 100644 --- a/GVFS/GVFS.UnitTests/Common/ModifiedPathsDatabaseTests.cs +++ b/GVFS/GVFS.UnitTests/Common/ModifiedPathsDatabaseTests.cs @@ -135,6 +135,24 @@ public void EntryNotAddedIfParentDirectoryExists() modifiedPathsDatabase.Contains("dir2/dir", isFolder: true).ShouldBeTrue(); } + [TestCase] + public void AddFollowedByDeleteIsRecoveredOnLoad() + { + // Simulates the on-disk state during the window between a background + // operation completing and PostBackgroundOperation calling + // WriteAllEntriesAndFlush. The append log contains both the add and + // delete entries; a subsequent load must replay them and end with + // the path NOT in the modified-paths set. + const string AddThenDelete = "A temp.txt\r\nD temp.txt\r\n"; + + ModifiedPathsDatabase modifiedPathsDatabase = CreateModifiedPathsDatabase(AddThenDelete); + + // Only the auto-added .gitattributes default entry should remain. + modifiedPathsDatabase.Count.ShouldEqual(1); + modifiedPathsDatabase.Contains(DefaultEntry, isFolder: false).ShouldBeTrue(); + modifiedPathsDatabase.Contains("temp.txt", isFolder: false).ShouldBeFalse(); + } + [TestCase] public void RemoveEntriesWithParentFolderEntry() { From 448c12b4700781c6c62ccde1463d1e1527e1ae54 Mon Sep 17 00:00:00 2001 From: Tyrie Vella Date: Thu, 11 Jun 2026 09:45:44 -0700 Subject: [PATCH 2/2] Address PR feedback: drop raw-file helper for semantic set check Per Keith's review, the two CheckoutTests callsites that used ModifiedPathsRawFileContentsShouldEqual were exposed to the same compaction race the rest of this PR fixes -- WaitForBackgroundOperations can return before PostBackgroundOperation rewrites the file, so any transient "A path / D path" cycle (today: none; future: easy to introduce) would break the exact-equals raw-bytes assertion. Those callers' real intent is semantic: "after this checkout sequence, the only modified path is .gitattributes". Replace the raw helper with a new ModifiedPathsShouldOnlyContain that compares against the replayed A/D log as a set. The raw helper now has zero callers and is removed. Assisted-by: Claude Opus 4.7 Signed-off-by: Tyrie Vella --- .../Tests/GitCommands/CheckoutTests.cs | 4 ++-- .../GVFS.FunctionalTests/Tools/GVFSHelpers.cs | 20 +++++++++---------- 2 files changed, 11 insertions(+), 13 deletions(-) diff --git a/GVFS/GVFS.FunctionalTests/Tests/GitCommands/CheckoutTests.cs b/GVFS/GVFS.FunctionalTests/Tests/GitCommands/CheckoutTests.cs index 6a2c61c33..94f92a40b 100644 --- a/GVFS/GVFS.FunctionalTests/Tests/GitCommands/CheckoutTests.cs +++ b/GVFS/GVFS.FunctionalTests/Tests/GitCommands/CheckoutTests.cs @@ -248,7 +248,7 @@ public void CheckoutBranchAfterReadingFileAndVerifyContentsCorrect() this.FilesShouldMatchCheckoutOfSourceBranch(); // Verify modified paths contents - GVFSHelpers.ModifiedPathsRawFileContentsShouldEqual(this.Enlistment, this.FileSystem, "A .gitattributes" + GVFSHelpers.ModifiedPathsNewLine); + GVFSHelpers.ModifiedPathsShouldOnlyContain(this.Enlistment, this.FileSystem, ".gitattributes"); } [TestCase] @@ -266,7 +266,7 @@ public void CheckoutBranchAfterReadingAllFilesAndVerifyContentsCorrect() .WithDeepStructure(this.FileSystem, this.ControlGitRepo.RootPath, compareContent: true, withinPrefixes: this.pathPrefixes); // Verify modified paths contents - GVFSHelpers.ModifiedPathsRawFileContentsShouldEqual(this.Enlistment, this.FileSystem, "A .gitattributes" + GVFSHelpers.ModifiedPathsNewLine); + GVFSHelpers.ModifiedPathsShouldOnlyContain(this.Enlistment, this.FileSystem, ".gitattributes"); } [TestCase] diff --git a/GVFS/GVFS.FunctionalTests/Tools/GVFSHelpers.cs b/GVFS/GVFS.FunctionalTests/Tools/GVFSHelpers.cs index 5d73fe490..8fdf7fe7f 100644 --- a/GVFS/GVFS.FunctionalTests/Tools/GVFSHelpers.cs +++ b/GVFS/GVFS.FunctionalTests/Tools/GVFSHelpers.cs @@ -166,19 +166,17 @@ public static string ReadAllTextFromWriteLockedFile(string filename) } /// - /// Asserts that the on-disk modified-paths file's raw contents are - /// exactly equal to , including the A/D - /// log prefixes and line terminators. Most callers want - /// / - /// instead, which compare - /// against the semantic set after replaying the log. Only use this - /// helper when the test specifically needs to validate the compacted - /// file layout (e.g. confirming a checkout left no stray entries). + /// Asserts that the modified-paths set, after replaying the on-disk + /// A/D log, contains exactly -- no more, + /// no fewer. Use this when a test wants to prove that some sequence + /// of operations produced no spurious modified-paths entries. /// - public static void ModifiedPathsRawFileContentsShouldEqual(GVFSFunctionalTestEnlistment enlistment, FileSystemRunner fileSystem, string contents) + public static void ModifiedPathsShouldOnlyContain(GVFSFunctionalTestEnlistment enlistment, FileSystemRunner fileSystem, params string[] gitPaths) { - string modifedPathsContents = GetModifiedPathsContents(enlistment, fileSystem); - modifedPathsContents.ShouldEqual(contents); + HashSet currentPaths = GetCurrentModifiedPaths(enlistment, fileSystem); + HashSet expectedPaths = new HashSet(gitPaths, FileSystemHelpers.PathComparer); + currentPaths.SetEquals(expectedPaths).ShouldBeTrue( + $"Expected modified paths {{{string.Join(",", expectedPaths)}}} but got {{{string.Join(",", currentPaths)}}}"); } public static void ModifiedPathsShouldContain(GVFSFunctionalTestEnlistment enlistment, FileSystemRunner fileSystem, params string[] gitPaths)