From e876cd7ef82aa056c2c3891c1b65ce2051c653c0 Mon Sep 17 00:00:00 2001 From: Tyrie Vella Date: Mon, 13 Apr 2026 08:17:30 -0700 Subject: [PATCH] GVFS: fix silent mount failure on concurrent worktree add MountNewWorktree() in the post-command hook silently discarded the exit code of 'gvfs mount', leaving worktrees unprojected when concurrent adds caused mount contention. Also discarded the git checkout exit code. Product fix: - Check git checkout exit code; skip mount on failure - Check gvfs mount exit code with retry (2 retries, 100ms/250ms) - Extract and check --exit_code from hook args to skip post-processing when the git command itself failed - Return int? from GetHookExitCode so callers decide null semantics - Apply same TryMountWithRetry to MountMovedWorktree - Remount old worktree when 'git worktree move' fails (pre-hook unmounts) - Remove maxRetries parameter; retry count driven by delay array length - Emit actionable warning to stderr on mount failure with recovery command Test fix: - Increase concurrent worktrees from 2 to ProcessorCount for reliable mount contention - Use CountdownEvent barrier for tight synchronization of launches - Add pipe-based mount verification as primary assertion (probes the worktree-specific named pipe directly instead of relying on File.Exists) - Add diagnostic capture on failure (directory listing, .git contents) - Use dynamic branch names with GUID suffixes to avoid collisions - Use numeric indices instead of char arithmetic for labels/paths to support >26 concurrent worktrees - Only retry at lower concurrency when ALL failures are overload-related; mixed failure sets (overload + real regression) are reported immediately Bug: https://dev.azure.com/microsoft/OS/_workitems/edit/61784115 Assisted-by: Claude Opus 4.6 Signed-off-by: Tyrie Vella --- .../EnlistmentPerFixture/WorktreeTests.cs | 368 ++++++++++++++---- GVFS/GVFS.Hooks/Program.Worktree.cs | 105 ++++- GVFS/GVFS.Hooks/Program.cs | 22 ++ 3 files changed, 405 insertions(+), 90 deletions(-) diff --git a/GVFS/GVFS.FunctionalTests/Tests/EnlistmentPerFixture/WorktreeTests.cs b/GVFS/GVFS.FunctionalTests/Tests/EnlistmentPerFixture/WorktreeTests.cs index 376796350..5d277d238 100644 --- a/GVFS/GVFS.FunctionalTests/Tests/EnlistmentPerFixture/WorktreeTests.cs +++ b/GVFS/GVFS.FunctionalTests/Tests/EnlistmentPerFixture/WorktreeTests.cs @@ -1,9 +1,15 @@ -using GVFS.FunctionalTests.Tools; +using GVFS.Common; +using GVFS.Common.NamedPipes; +using GVFS.FunctionalTests.Tools; using GVFS.Tests.Should; using NUnit.Framework; using System; using System.Diagnostics; using System.IO; +using System.Linq; +using System.Text; +using System.Threading; +using ProcessResult = GVFS.FunctionalTests.Tools.ProcessResult; namespace GVFS.FunctionalTests.Tests.EnlistmentPerFixture { @@ -11,102 +17,298 @@ namespace GVFS.FunctionalTests.Tests.EnlistmentPerFixture [Category(Categories.GitCommands)] public class WorktreeTests : TestsWithEnlistmentPerFixture { - private const string WorktreeBranchA = "worktree-test-branch-a"; - private const string WorktreeBranchB = "worktree-test-branch-b"; + private const int MinWorktreeCount = 4; [TestCase] public void ConcurrentWorktreeAddCommitRemove() { - string worktreePathA = Path.Combine(this.Enlistment.EnlistmentRoot, "test-wt-a-" + Guid.NewGuid().ToString("N").Substring(0, 8)); - string worktreePathB = Path.Combine(this.Enlistment.EnlistmentRoot, "test-wt-b-" + Guid.NewGuid().ToString("N").Substring(0, 8)); + int count = Math.Max(Environment.ProcessorCount, MinWorktreeCount); + string[] worktreePaths; + string[] branchNames; + + // Adaptively scale down if concurrent adds overwhelm the primary + // GVFS mount. CI runners with fewer resources may not handle as + // many concurrent git operations as a developer workstation. + while (true) + { + this.InitWorktreeArrays(count, out worktreePaths, out branchNames); + ProcessResult[] addResults = this.ConcurrentWorktreeAdd(worktreePaths, branchNames, count); + + bool overloaded = addResults.Any(r => + r.ExitCode != 0 && + r.Errors != null && + r.Errors.Contains("does not appear to be mounted")); + + // Only retry if ALL failures are overload-related. If any + // failure has a different cause, it's a real regression and + // must not be masked by retrying at lower concurrency. + bool hasNonOverloadFailure = addResults.Any(r => + r.ExitCode != 0 && + !(r.Errors != null && r.Errors.Contains("does not appear to be mounted"))); + + if (hasNonOverloadFailure) + { + // Fall through to the assertion loop below which will + // report the specific failure(s). + } + else if (overloaded) + { + this.CleanupAllWorktrees(worktreePaths, branchNames, count); + int reduced = count / 2; + if (reduced < MinWorktreeCount) + { + Assert.Fail( + $"Primary GVFS mount overloaded even at count={count}. " + + $"Cannot reduce below {MinWorktreeCount}."); + } + + count = reduced; + continue; + } + + // Non-overload failures are real errors + for (int i = 0; i < count; i++) + { + addResults[i].ExitCode.ShouldEqual(0, + $"worktree add [{i}] failed: {addResults[i].Errors}"); + } + + break; + } try { - // 1. Create both worktrees in parallel - ProcessResult addResultA = null; - ProcessResult addResultB = null; - System.Threading.Tasks.Parallel.Invoke( - () => addResultA = GitHelpers.InvokeGitAgainstGVFSRepo( - this.Enlistment.RepoRoot, - $"worktree add -b {WorktreeBranchA} \"{worktreePathA}\""), - () => addResultB = GitHelpers.InvokeGitAgainstGVFSRepo( - this.Enlistment.RepoRoot, - $"worktree add -b {WorktreeBranchB} \"{worktreePathB}\"")); - - addResultA.ExitCode.ShouldEqual(0, $"worktree add A failed: {addResultA.Errors}"); - addResultB.ExitCode.ShouldEqual(0, $"worktree add B failed: {addResultB.Errors}"); - - // 2. Verify both have projected files - Directory.Exists(worktreePathA).ShouldBeTrue("Worktree A directory should exist"); - Directory.Exists(worktreePathB).ShouldBeTrue("Worktree B directory should exist"); - File.Exists(Path.Combine(worktreePathA, "Readme.md")).ShouldBeTrue("Readme.md should be projected in A"); - File.Exists(Path.Combine(worktreePathB, "Readme.md")).ShouldBeTrue("Readme.md should be projected in B"); - - // 3. Verify git status is clean in both - ProcessResult statusA = GitHelpers.InvokeGitAgainstGVFSRepo(worktreePathA, "status --porcelain"); - ProcessResult statusB = GitHelpers.InvokeGitAgainstGVFSRepo(worktreePathB, "status --porcelain"); - statusA.ExitCode.ShouldEqual(0, $"git status A failed: {statusA.Errors}"); - statusB.ExitCode.ShouldEqual(0, $"git status B failed: {statusB.Errors}"); - statusA.Output.Trim().ShouldBeEmpty("Worktree A should have clean status"); - statusB.Output.Trim().ShouldBeEmpty("Worktree B should have clean status"); - - // 4. Verify worktree list shows all three + // 2. Primary assertion: verify GVFS mount is running for each + // worktree by probing the worktree-specific named pipe. + for (int i = 0; i < count; i++) + { + this.AssertWorktreeMounted(worktreePaths[i], $"worktree [{i}]"); + } + + // 3. Verify projected files are visible (secondary assertion) + for (int i = 0; i < count; i++) + { + Directory.Exists(worktreePaths[i]).ShouldBeTrue( + $"Worktree [{i}] directory should exist"); + File.Exists(Path.Combine(worktreePaths[i], "Readme.md")).ShouldBeTrue( + $"Readme.md should be projected in [{i}]"); + } + + // 4. Verify git status is clean in each worktree + for (int i = 0; i < count; i++) + { + ProcessResult status = GitHelpers.InvokeGitAgainstGVFSRepo( + worktreePaths[i], "status --porcelain"); + status.ExitCode.ShouldEqual(0, + $"git status [{i}] failed: {status.Errors}"); + status.Output.Trim().ShouldBeEmpty( + $"Worktree [{i}] should have clean status"); + } + + // 5. Verify worktree list shows all entries ProcessResult listResult = GitHelpers.InvokeGitAgainstGVFSRepo( this.Enlistment.RepoRoot, "worktree list"); listResult.ExitCode.ShouldEqual(0, $"worktree list failed: {listResult.Errors}"); string listOutput = listResult.Output; - Assert.IsTrue(listOutput.Contains(worktreePathA.Replace('\\', '/')), - $"worktree list should contain A. Output: {listOutput}"); - Assert.IsTrue(listOutput.Contains(worktreePathB.Replace('\\', '/')), - $"worktree list should contain B. Output: {listOutput}"); - - // 5. Make commits in both worktrees - File.WriteAllText(Path.Combine(worktreePathA, "from-a.txt"), "created in worktree A"); - GitHelpers.InvokeGitAgainstGVFSRepo(worktreePathA, "add from-a.txt") - .ExitCode.ShouldEqual(0); - GitHelpers.InvokeGitAgainstGVFSRepo(worktreePathA, "commit -m \"commit from A\"") - .ExitCode.ShouldEqual(0); - - File.WriteAllText(Path.Combine(worktreePathB, "from-b.txt"), "created in worktree B"); - GitHelpers.InvokeGitAgainstGVFSRepo(worktreePathB, "add from-b.txt") - .ExitCode.ShouldEqual(0); - GitHelpers.InvokeGitAgainstGVFSRepo(worktreePathB, "commit -m \"commit from B\"") - .ExitCode.ShouldEqual(0); - - // 6. Verify commits are visible from all worktrees (shared objects) - GitHelpers.InvokeGitAgainstGVFSRepo(this.Enlistment.RepoRoot, $"log -1 --format=%s {WorktreeBranchA}") - .Output.ShouldContain(expectedSubstrings: new[] { "commit from A" }); - GitHelpers.InvokeGitAgainstGVFSRepo(this.Enlistment.RepoRoot, $"log -1 --format=%s {WorktreeBranchB}") - .Output.ShouldContain(expectedSubstrings: new[] { "commit from B" }); - - // A can see B's commit and vice versa - GitHelpers.InvokeGitAgainstGVFSRepo(worktreePathA, $"log -1 --format=%s {WorktreeBranchB}") - .Output.ShouldContain(expectedSubstrings: new[] { "commit from B" }); - GitHelpers.InvokeGitAgainstGVFSRepo(worktreePathB, $"log -1 --format=%s {WorktreeBranchA}") - .Output.ShouldContain(expectedSubstrings: new[] { "commit from A" }); - - // 7. Remove both in parallel - ProcessResult removeA = null; - ProcessResult removeB = null; - System.Threading.Tasks.Parallel.Invoke( - () => removeA = GitHelpers.InvokeGitAgainstGVFSRepo( - this.Enlistment.RepoRoot, - $"worktree remove --force \"{worktreePathA}\""), - () => removeB = GitHelpers.InvokeGitAgainstGVFSRepo( - this.Enlistment.RepoRoot, - $"worktree remove --force \"{worktreePathB}\"")); - - removeA.ExitCode.ShouldEqual(0, $"worktree remove A failed: {removeA.Errors}"); - removeB.ExitCode.ShouldEqual(0, $"worktree remove B failed: {removeB.Errors}"); - - // 8. Verify cleanup - Directory.Exists(worktreePathA).ShouldBeFalse("Worktree A directory should be deleted"); - Directory.Exists(worktreePathB).ShouldBeFalse("Worktree B directory should be deleted"); + for (int i = 0; i < count; i++) + { + Assert.IsTrue( + listOutput.Contains(worktreePaths[i].Replace('\\', '/')), + $"worktree list should contain [{i}]. Output: {listOutput}"); + } + + // 6. Make commits in all worktrees + for (int i = 0; i < count; i++) + { + File.WriteAllText( + Path.Combine(worktreePaths[i], $"from-{i}.txt"), + $"created in worktree {i}"); + GitHelpers.InvokeGitAgainstGVFSRepo(worktreePaths[i], $"add from-{i}.txt") + .ExitCode.ShouldEqual(0); + GitHelpers.InvokeGitAgainstGVFSRepo( + worktreePaths[i], $"commit -m \"commit from {i}\"") + .ExitCode.ShouldEqual(0); + } + + // 7. Verify commits are visible from main repo + for (int i = 0; i < count; i++) + { + GitHelpers.InvokeGitAgainstGVFSRepo( + this.Enlistment.RepoRoot, $"log -1 --format=%s {branchNames[i]}") + .Output.ShouldContain(expectedSubstrings: new[] { $"commit from {i}" }); + } + + // 8. Verify cross-worktree commit visibility (shared objects) + for (int i = 0; i < count; i++) + { + int other = (i + 1) % count; + GitHelpers.InvokeGitAgainstGVFSRepo( + worktreePaths[i], $"log -1 --format=%s {branchNames[other]}") + .Output.ShouldContain(expectedSubstrings: new[] { $"commit from {other}" }); + } + + // 9. Remove all worktrees in parallel + ProcessResult[] removeResults = new ProcessResult[count]; + using (CountdownEvent barrier = new CountdownEvent(count)) + { + Thread[] threads = new Thread[count]; + for (int i = 0; i < count; i++) + { + int idx = i; + threads[idx] = new Thread(() => + { + barrier.Signal(); + barrier.Wait(); + removeResults[idx] = GitHelpers.InvokeGitAgainstGVFSRepo( + this.Enlistment.RepoRoot, + $"worktree remove --force \"{worktreePaths[idx]}\""); + }); + threads[idx].Start(); + } + + foreach (Thread t in threads) + { + t.Join(); + } + } + + for (int i = 0; i < count; i++) + { + removeResults[i].ExitCode.ShouldEqual(0, + $"worktree remove [{i}] failed: {removeResults[i].Errors}"); + } + + // 10. Verify cleanup + for (int i = 0; i < count; i++) + { + Directory.Exists(worktreePaths[i]).ShouldBeFalse( + $"Worktree [{i}] directory should be deleted"); + } } finally { - this.ForceCleanupWorktree(worktreePathA, WorktreeBranchA); - this.ForceCleanupWorktree(worktreePathB, WorktreeBranchB); + this.CleanupAllWorktrees(worktreePaths, branchNames, count); + } + } + + private void InitWorktreeArrays(int count, out string[] paths, out string[] branches) + { + paths = new string[count]; + branches = new string[count]; + for (int i = 0; i < count; i++) + { + string suffix = Guid.NewGuid().ToString("N").Substring(0, 8); + paths[i] = Path.Combine(this.Enlistment.EnlistmentRoot, $"test-wt-{i}-{suffix}"); + branches[i] = $"worktree-test-branch-{i}-{suffix}"; + } + } + + private ProcessResult[] ConcurrentWorktreeAdd(string[] paths, string[] branches, int count) + { + ProcessResult[] results = new ProcessResult[count]; + using (CountdownEvent barrier = new CountdownEvent(count)) + { + Thread[] threads = new Thread[count]; + for (int i = 0; i < count; i++) + { + int idx = i; + threads[idx] = new Thread(() => + { + barrier.Signal(); + barrier.Wait(); + results[idx] = GitHelpers.InvokeGitAgainstGVFSRepo( + this.Enlistment.RepoRoot, + $"worktree add -b {branches[idx]} \"{paths[idx]}\""); + }); + threads[idx].Start(); + } + + foreach (Thread t in threads) + { + t.Join(); + } + } + + return results; + } + + /// + /// Asserts that the GVFS mount for a worktree is running by probing + /// the worktree-specific named pipe. This is the definitive signal + /// that ProjFS projection is active — much stronger than File.Exists + /// which depends on projection timing. + /// + private void AssertWorktreeMounted(string worktreePath, string label) + { + string basePipeName = GVFSPlatform.Instance.GetNamedPipeName( + this.Enlistment.EnlistmentRoot); + string suffix = GVFSEnlistment.GetWorktreePipeSuffix(worktreePath); + + Assert.IsNotNull(suffix, + $"Could not determine pipe suffix for {label} at {worktreePath}. " + + $"The worktree .git file may be missing or malformed."); + + string pipeName = basePipeName + suffix; + + using (NamedPipeClient client = new NamedPipeClient(pipeName)) + { + if (!client.Connect(10000)) + { + string diagnostics = this.CaptureWorktreeDiagnostics(worktreePath); + Assert.Fail( + $"GVFS mount is NOT running for {label}.\n" + + $"Path: {worktreePath}\n" + + $"Pipe: {pipeName}\n" + + $"This indicates the post-hook 'gvfs mount' failed silently.\n" + + $"Diagnostics:\n{diagnostics}"); + } + } + } + + private string CaptureWorktreeDiagnostics(string worktreePath) + { + StringBuilder sb = new StringBuilder(); + + sb.AppendLine($" Directory exists: {Directory.Exists(worktreePath)}"); + if (Directory.Exists(worktreePath)) + { + string dotGit = Path.Combine(worktreePath, ".git"); + sb.AppendLine($" .git file exists: {File.Exists(dotGit)}"); + if (File.Exists(dotGit)) + { + try + { + sb.AppendLine($" .git contents: {File.ReadAllText(dotGit).Trim()}"); + } + catch (Exception ex) + { + sb.AppendLine($" .git read failed: {ex.Message}"); + } + } + + try + { + string[] entries = Directory.GetFileSystemEntries(worktreePath); + sb.AppendLine($" Directory listing ({entries.Length} entries):"); + foreach (string entry in entries) + { + sb.AppendLine($" {Path.GetFileName(entry)}"); + } + } + catch (Exception ex) + { + sb.AppendLine($" Directory listing failed: {ex.Message}"); + } + } + + return sb.ToString(); + } + + private void CleanupAllWorktrees(string[] paths, string[] branches, int count) + { + for (int i = 0; i < count; i++) + { + this.ForceCleanupWorktree(paths[i], branches[i]); } } diff --git a/GVFS/GVFS.Hooks/Program.Worktree.cs b/GVFS/GVFS.Hooks/Program.Worktree.cs index 325532a37..40c768ce0 100644 --- a/GVFS/GVFS.Hooks/Program.Worktree.cs +++ b/GVFS/GVFS.Hooks/Program.Worktree.cs @@ -51,22 +51,72 @@ private static void RunWorktreePreCommand(string[] args) private static void RunWorktreePostCommand(string[] args) { string subcommand = GetWorktreeSubcommand(args); + int? gitExitCode = GetHookExitCode(args); + + // Treat null (missing arg) the same as 0 — older Git versions + // may not pass --exit_code, and we should run post-processing + // in that case for backward compatibility. + bool gitSucceeded = gitExitCode == null || gitExitCode == 0; + switch (subcommand) { case "add": - MountNewWorktree(args); + if (gitSucceeded) + { + MountNewWorktree(args); + } + break; case "remove": + // Always run cleanup regardless of git exit code — need to + // remount if remove failed, and clean markers either way. RemountWorktreeIfRemoveFailed(args); CleanupSkipCleanCheckMarker(args); break; case "move": - // Mount at the new location after git moved the directory - MountMovedWorktree(args); + if (gitSucceeded) + { + MountMovedWorktree(args); + } + else + { + // Move failed — the pre-hook already unmounted the old + // location. Remount so the worktree remains usable. + RemountWorktreeIfMoveFailed(args); + } + break; } } + /// + /// Attempts to mount GVFS for a worktree, retrying on transient failures. + /// The first attempt shows output to the console; retries are quiet. + /// Returns true if mount succeeded. + /// + private static bool TryMountWithRetry(string fullPath) + { + int[] retryDelaysMs = { 100, 250 }; + + ProcessResult result = ProcessHelper.Run("gvfs", $"mount \"{fullPath}\"", redirectOutput: false); + if (result.ExitCode == 0) + { + return true; + } + + for (int retry = 0; retry < retryDelaysMs.Length; retry++) + { + System.Threading.Thread.Sleep(retryDelaysMs[retry]); + result = ProcessHelper.Run("gvfs", $"mount \"{fullPath}\"", redirectOutput: true); + if (result.ExitCode == 0) + { + return true; + } + } + + return false; + } + private static void UnmountWorktreeByArg(string[] args) { string worktreePath = GetWorktreePathArg(args); @@ -106,6 +156,27 @@ private static void RemountWorktreeIfRemoveFailed(string[] args) } } + /// + /// If git worktree move failed, remount at the original location. + /// The pre-hook unmounted the worktree before the move attempt; + /// on failure, the directory hasn't moved so we remount in place. + /// + private static void RemountWorktreeIfMoveFailed(string[] args) + { + string worktreePath = GetWorktreePathArg(args); + if (string.IsNullOrEmpty(worktreePath)) + { + return; + } + + string fullPath = ResolvePath(worktreePath); + string dotGitFile = Path.Combine(fullPath, ".git"); + if (Directory.Exists(fullPath) && File.Exists(dotGitFile)) + { + ProcessHelper.Run("gvfs", $"mount \"{fullPath}\"", redirectOutput: false); + } + } + /// /// Remove the skip-clean-check marker if it still exists after /// worktree remove completes (e.g., if the remove failed and the @@ -335,22 +406,32 @@ private static void MountNewWorktree(string[] args) // Disable hooks via core.hookspath — the worktree's GVFS mount // doesn't exist yet, so post-index-change would fail trying // to connect to a pipe that hasn't been created. + bool checkoutSucceeded = false; string emptyVfsHook = Path.Combine(fullPath, ".vfs-empty-hook"); try { File.WriteAllText(emptyVfsHook, "#!/bin/sh\nprintf \".gitattributes\\n\"\n"); string emptyVfsHookGitPath = emptyVfsHook.Replace('\\', '/'); - ProcessHelper.Run( + ProcessResult checkoutResult = ProcessHelper.Run( "git", $"-C \"{fullPath}\" -c core.virtualfilesystem=\"'{emptyVfsHookGitPath}'\" -c core.hookspath= checkout -f HEAD", redirectOutput: false); + checkoutSucceeded = checkoutResult.ExitCode == 0; } finally { File.Delete(emptyVfsHook); } + if (!checkoutSucceeded) + { + Console.Error.WriteLine( + $"warning: worktree checkout failed for '{fullPath}'.\n" + + $"The worktree may not be fully initialized. Run 'gvfs mount \"{fullPath}\"' to recover."); + return; + } + // Hydrate .gitattributes — copy from the primary enlistment. if (wtInfo?.SharedGitDir != null) { @@ -363,8 +444,13 @@ private static void MountNewWorktree(string[] args) } } - // Now mount GVFS — the index exists for GitIndexProjection - ProcessHelper.Run("gvfs", $"mount \"{fullPath}\"", redirectOutput: false); + // Mount GVFS with retry for transient contention (e.g. concurrent adds) + if (!TryMountWithRetry(fullPath)) + { + Console.Error.WriteLine( + $"warning: failed to mount GVFS for worktree '{fullPath}' after multiple attempts.\n" + + $"Files may not be visible. Run 'gvfs mount \"{fullPath}\"' to recover."); + } } } @@ -383,7 +469,12 @@ private static void MountMovedWorktree(string[] args) string dotGitFile = Path.Combine(fullPath, ".git"); if (File.Exists(dotGitFile)) { - ProcessHelper.Run("gvfs", $"mount \"{fullPath}\"", redirectOutput: false); + if (!TryMountWithRetry(fullPath)) + { + Console.Error.WriteLine( + $"warning: failed to mount GVFS for moved worktree '{fullPath}' after multiple attempts.\n" + + $"Files may not be visible. Run 'gvfs mount \"{fullPath}\"' to recover."); + } } } } diff --git a/GVFS/GVFS.Hooks/Program.cs b/GVFS/GVFS.Hooks/Program.cs index c04f0c778..e9e3fb537 100644 --- a/GVFS/GVFS.Hooks/Program.cs +++ b/GVFS/GVFS.Hooks/Program.cs @@ -508,6 +508,28 @@ private static bool IsAlias(string command) return !string.IsNullOrEmpty(result.Output); } + /// + /// Extracts the git exit code from hook args. Git appends --exit_code=N + /// to post-command hook arguments. Returns null if the argument is + /// missing or unparseable — callers decide what "no exit code" means + /// for their use case. + /// + private static int? GetHookExitCode(string[] args) + { + for (int i = args.Length - 1; i >= 0; i--) + { + if (args[i].StartsWith("--exit_code=")) + { + if (int.TryParse(args[i].Substring("--exit_code=".Length), out int code)) + { + return code; + } + } + } + + return null; + } + private static string GetGitCommandSessionId() { try