From 4c69e85cf7d1f011ea02dc23b11709b713c69108 Mon Sep 17 00:00:00 2001 From: Tyrie Vella Date: Tue, 19 May 2026 13:35:43 -0700 Subject: [PATCH 1/5] hooks: resolve enlistment root for worktrees outside enlistment tree MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit All native hooks (virtual-filesystem, read-object, post-index-changed) and managed hooks (GVFS.Hooks) resolved the primary enlistment root by walking up from CWD looking for a .gvfs/ directory. This meant worktrees placed outside the enlistment directory tree (e.g. in a temp directory or as a sibling) could not use git commands — the hooks would fail with 'must be run from inside a GVFS enlistment'. Fix by adding a worktree fallback path: when .gvfs walk-up fails, walk up looking for a .git file (indicating a linked worktree), then resolve the primary enlistment root through the gitdir chain — first from the gvfs-enlistment-root marker file, then by deriving from commondir. The resolved root is validated by checking that .gvfs/ exists there. Native hooks (common.windows.cpp): - Extract ReadFirstLine, Utf8ToWide, TryParseGitFile helpers - Refactor GetWorktreePipeSuffix to use shared helpers - Add TryResolveFromWorktree that returns both enlistment root and pipe suffix - GetGVFSPipeName tries .gvfs walk-up first, then worktree fallback Managed hooks (Program.cs): - After TryGetGVFSEnlistmentRoot fails, try TryGetWorktreeInfo -> GetEnlistmentRoot() before exiting - Validate .gvfs exists at the resolved root Tests: - Unit tests for GetEnlistmentRoot with marker file, SharedGitDir fallback, and marker-preferred scenarios - Functional test creating worktree in temp directory, verifying git status from root and subdirectory, file projection, commits Assisted-by: Claude Opus 4.6 Signed-off-by: Ty Larrabee --- .../EnlistmentPerFixture/WorktreeTests.cs | 77 +++++ GVFS/GVFS.Hooks/Program.cs | 19 +- .../common.windows.cpp | 296 ++++++++++++++---- .../Common/WorktreeInfoTests.cs | 68 ++++ 4 files changed, 402 insertions(+), 58 deletions(-) diff --git a/GVFS/GVFS.FunctionalTests/Tests/EnlistmentPerFixture/WorktreeTests.cs b/GVFS/GVFS.FunctionalTests/Tests/EnlistmentPerFixture/WorktreeTests.cs index 5d277d238..1f8501043 100644 --- a/GVFS/GVFS.FunctionalTests/Tests/EnlistmentPerFixture/WorktreeTests.cs +++ b/GVFS/GVFS.FunctionalTests/Tests/EnlistmentPerFixture/WorktreeTests.cs @@ -191,6 +191,83 @@ public void ConcurrentWorktreeAddCommitRemove() } } + [TestCase] + public void WorktreeOutsideEnlistmentTree() + { + string suffix = Guid.NewGuid().ToString("N").Substring(0, 8); + string tempDir = Path.Combine(Path.GetTempPath(), $"gvfs-remote-wt-{suffix}"); + string worktreePath = Path.Combine(tempDir, "wt"); + string branchName = $"remote-wt-test-{suffix}"; + + try + { + Directory.CreateDirectory(tempDir); + + // 1. Create worktree outside the enlistment tree + ProcessResult addResult = GitHelpers.InvokeGitAgainstGVFSRepo( + this.Enlistment.RepoRoot, + $"worktree add -b {branchName} \"{worktreePath}\""); + addResult.ExitCode.ShouldEqual(0, + $"worktree add failed: {addResult.Errors}"); + + // 2. Verify GVFS mount is running + this.AssertWorktreeMounted(worktreePath, "remote worktree"); + + // 3. Verify git status works from the worktree root + ProcessResult statusResult = GitHelpers.InvokeGitAgainstGVFSRepo( + worktreePath, "status --porcelain"); + statusResult.ExitCode.ShouldEqual(0, + $"git status from worktree root failed: {statusResult.Errors}"); + statusResult.Output.Trim().ShouldBeEmpty( + "Remote worktree should have clean status"); + + // 4. Verify projected files are visible + File.Exists(Path.Combine(worktreePath, "Readme.md")).ShouldBeTrue( + "Readme.md should be projected in remote worktree"); + + // 5. Verify git status works from a subdirectory + string subDir = Path.Combine(worktreePath, "GVFS"); + Directory.Exists(subDir).ShouldBeTrue( + "Subdirectory GVFS should be projected"); + ProcessResult subDirStatus = GitHelpers.InvokeGitAgainstGVFSRepo( + subDir, "status --porcelain"); + subDirStatus.ExitCode.ShouldEqual(0, + $"git status from subdirectory failed: {subDirStatus.Errors}"); + + // 6. Verify commits work + File.WriteAllText( + Path.Combine(worktreePath, "remote-test.txt"), + "created in remote worktree"); + GitHelpers.InvokeGitAgainstGVFSRepo(worktreePath, "add remote-test.txt") + .ExitCode.ShouldEqual(0); + GitHelpers.InvokeGitAgainstGVFSRepo( + worktreePath, "commit -m \"commit from remote worktree\"") + .ExitCode.ShouldEqual(0); + + // 7. Verify commit is visible from primary repo + GitHelpers.InvokeGitAgainstGVFSRepo( + this.Enlistment.RepoRoot, $"log -1 --format=%s {branchName}") + .Output.ShouldContain(expectedSubstrings: new[] { "commit from remote worktree" }); + + // 8. Remove worktree + ProcessResult removeResult = GitHelpers.InvokeGitAgainstGVFSRepo( + this.Enlistment.RepoRoot, + $"worktree remove --force \"{worktreePath}\""); + removeResult.ExitCode.ShouldEqual(0, + $"worktree remove failed: {removeResult.Errors}"); + Directory.Exists(worktreePath).ShouldBeFalse( + "Remote worktree directory should be deleted"); + } + finally + { + this.ForceCleanupWorktree(worktreePath, branchName); + if (Directory.Exists(tempDir)) + { + try { Directory.Delete(tempDir, recursive: true); } catch { } + } + } + } + private void InitWorktreeArrays(int count, out string[] paths, out string[] branches) { paths = new string[count]; diff --git a/GVFS/GVFS.Hooks/Program.cs b/GVFS/GVFS.Hooks/Program.cs index e9e3fb537..aee260928 100644 --- a/GVFS/GVFS.Hooks/Program.cs +++ b/GVFS/GVFS.Hooks/Program.cs @@ -3,6 +3,7 @@ using GVFS.Common.NamedPipes; using GVFS.Common.Tracing; using GVFS.Hooks.HooksPlatform; +using GVFS.Platform.Windows; using System; using System.Collections.Generic; using System.IO; @@ -47,9 +48,21 @@ public static void Main(string[] args) if (!GVFSHooksPlatform.TryGetGVFSEnlistmentRoot(Environment.CurrentDirectory, out enlistmentRoot, out errorMessage)) { - // Nothing to hook when being run outside of a GVFS repo. - // This is also the path when run with --git-dir outside of a GVFS directory, see Story #949665 - Environment.Exit(0); + // .gvfs walk-up failed — this may be a worktree placed + // outside the primary enlistment tree. Try resolving + // the enlistment root through the worktree chain. + GVFSEnlistment.WorktreeInfo wtInfo = GVFSEnlistment.TryGetWorktreeInfo(normalizedCurrentDirectory); + if (wtInfo != null) + { + enlistmentRoot = wtInfo.GetEnlistmentRoot(); + } + + if (enlistmentRoot == null || + !Directory.Exists(Path.Combine(enlistmentRoot, WindowsPlatform.DotGVFSRoot))) + { + // Not in a GVFS repo or worktree. Nothing to hook. + Environment.Exit(0); + } } enlistmentPipename = GVFSHooksPlatform.GetNamedPipeName(enlistmentRoot); diff --git a/GVFS/GVFS.NativeHooks.Common/common.windows.cpp b/GVFS/GVFS.NativeHooks.Common/common.windows.cpp index 35c7db8d4..f837f4585 100644 --- a/GVFS/GVFS.NativeHooks.Common/common.windows.cpp +++ b/GVFS/GVFS.NativeHooks.Common/common.windows.cpp @@ -3,6 +3,7 @@ #include #include #include +#include #include "common.h" PATH_STRING GetFinalPathName(const PATH_STRING& path) @@ -52,6 +53,112 @@ PATH_STRING GetFinalPathName(const PATH_STRING& path) return finalPath; } +// Reads the first line of a UTF-8 text file into a std::string. +// Returns false if the file cannot be opened or read. +static bool ReadFirstLine(const PATH_STRING& filePath, std::string& line) +{ + FILE* file = NULL; + errno_t err = _wfopen_s(&file, filePath.c_str(), L"r"); + if (err != 0 || file == NULL) + return false; + + char buffer[4096]; + if (fgets(buffer, sizeof(buffer), file) == NULL) + { + fclose(file); + return false; + } + fclose(file); + + line = buffer; + + // Trim trailing whitespace / newlines + while (!line.empty() && (line.back() == '\n' || line.back() == '\r' || line.back() == ' ')) + line.pop_back(); + + return true; +} + +// Converts a UTF-8 string to a wide string. +static PATH_STRING Utf8ToWide(const std::string& utf8) +{ + if (utf8.empty()) + return PATH_STRING(); + + int wideLen = MultiByteToWideChar(CP_UTF8, 0, utf8.c_str(), -1, NULL, 0); + if (wideLen <= 0) + return PATH_STRING(); + + PATH_STRING wide(wideLen, L'\0'); + MultiByteToWideChar(CP_UTF8, 0, utf8.c_str(), -1, &wide[0], wideLen); + wide.resize(wideLen - 1); + return wide; +} + +// Checks if a directory exists at the given path. +static bool DirectoryExists(const PATH_STRING& path) +{ + DWORD attrs = GetFileAttributesW(path.c_str()); + return attrs != INVALID_FILE_ATTRIBUTES && (attrs & FILE_ATTRIBUTE_DIRECTORY); +} + +// Resolves a potentially relative path against a base directory. +static PATH_STRING ResolvePath(const PATH_STRING& basePath, const PATH_STRING& relativePath) +{ + PATH_STRING combined; + if (relativePath.length() >= 2 && relativePath[1] == L':') + { + combined = relativePath; + } + else + { + combined = basePath; + if (!combined.empty() && combined.back() != L'\\') + combined += L'\\'; + combined += relativePath; + } + + wchar_t resolved[MAX_PATH]; + DWORD len = GetFullPathNameW(combined.c_str(), MAX_PATH, resolved, NULL); + if (len == 0 || len >= MAX_PATH) + return combined; + + return PATH_STRING(resolved); +} + +// Parses a .git file to extract the resolved gitdir path and +// worktree name (last component of gitdir path). +static bool TryParseGitFile( + const PATH_STRING& dotGitFilePath, + const PATH_STRING& containingDir, + PATH_STRING& resolvedGitdir, + std::string& worktreeName) +{ + std::string gitdirLine; + if (!ReadFirstLine(dotGitFilePath, gitdirLine)) + return false; + + const char* prefix = "gitdir: "; + if (gitdirLine.compare(0, 8, prefix) != 0) + return false; + + std::string gitdirPath = gitdirLine.substr(8); + if (gitdirPath.empty()) + return false; + + std::replace(gitdirPath.begin(), gitdirPath.end(), '/', '\\'); + + size_t lastSep = gitdirPath.find_last_of('\\'); + if (lastSep == std::string::npos || lastSep == gitdirPath.length() - 1) + return false; + + worktreeName = gitdirPath.substr(lastSep + 1); + + PATH_STRING wideGitdir = Utf8ToWide(gitdirPath); + resolvedGitdir = ResolvePath(containingDir, wideGitdir); + return true; +} + // Checks if the given directory is a git worktree by looking for a // ".git" file (not directory). If found, reads it to extract the // worktree name and returns a pipe name suffix like "_WT_NAME". @@ -70,53 +177,101 @@ PATH_STRING GetWorktreePipeSuffix(const wchar_t* directory) return PATH_STRING(); } - // .git is a file — this is a worktree. Read it to find the - // worktree git directory (format: "gitdir: ") - FILE* gitFile = NULL; - errno_t fopenResult = _wfopen_s(&gitFile, dotGitPath.c_str(), L"r"); - if (fopenResult != 0 || gitFile == NULL) + PATH_STRING resolvedGitdir; + std::string worktreeName; + if (!TryParseGitFile(dotGitPath, PATH_STRING(directory), resolvedGitdir, worktreeName)) return PATH_STRING(); - char gitdirLine[4096]; - if (fgets(gitdirLine, sizeof(gitdirLine), gitFile) == NULL) - { - fclose(gitFile); - return PATH_STRING(); - } - fclose(gitFile); - - char* gitdirPath = gitdirLine; - if (strncmp(gitdirPath, "gitdir: ", 8) == 0) - gitdirPath += 8; - - // Trim trailing whitespace - size_t lineLen = strlen(gitdirPath); - while (lineLen > 0 && (gitdirPath[lineLen - 1] == '\n' || - gitdirPath[lineLen - 1] == '\r' || - gitdirPath[lineLen - 1] == ' ')) - gitdirPath[--lineLen] = '\0'; - - // Extract worktree name — last path component - // e.g., from ".git/worktrees/my-worktree" extract "my-worktree" - char* lastSep = strrchr(gitdirPath, '/'); - if (!lastSep) - lastSep = strrchr(gitdirPath, '\\'); - - if (lastSep == NULL) + // Verify this is actually a worktree (has commondir file) + PATH_STRING commondirFile = resolvedGitdir + L"\\commondir"; + std::string commondirContent; + if (!ReadFirstLine(commondirFile, commondirContent)) return PATH_STRING(); - std::string nameUtf8(lastSep + 1); - int wideLen = MultiByteToWideChar(CP_UTF8, 0, nameUtf8.c_str(), -1, NULL, 0); - if (wideLen <= 0) - return PATH_STRING(); + PATH_STRING suffix = L"_WT_" + Utf8ToWide(worktreeName); + return suffix; +} - std::wstring wtName(wideLen, L'\0'); - MultiByteToWideChar(CP_UTF8, 0, nameUtf8.c_str(), -1, &wtName[0], wideLen); - wtName.resize(wideLen - 1); // remove null terminator from string +// Walks up from startDirectory looking for a ".git" file (not directory) +// indicating a git worktree. If found, resolves the primary GVFS +// enlistment root through the worktree's gitdir chain: +// 1. Read gvfs-enlistment-root marker (preferred) +// 2. Fall back to commondir -> shared .git dir -> parent -> parent +// Validates that the resolved root contains a .gvfs directory. +static bool TryResolveFromWorktree( + const PATH_STRING& startDirectory, + PATH_STRING& enlistmentRoot, + PATH_STRING& pipeSuffix) +{ + PATH_STRING current = startDirectory; + while (true) + { + PATH_STRING dotGitPath = current + L"\\.git"; + DWORD attrs = GetFileAttributesW(dotGitPath.c_str()); - PATH_STRING suffix = L"_WT_"; - suffix += wtName; - return suffix; + if (attrs != INVALID_FILE_ATTRIBUTES && !(attrs & FILE_ATTRIBUTE_DIRECTORY)) + { + PATH_STRING resolvedGitdir; + std::string worktreeName; + if (!TryParseGitFile(dotGitPath, current, resolvedGitdir, worktreeName)) + return false; + + PATH_STRING commondirFile = resolvedGitdir + L"\\commondir"; + std::string commondirContent; + if (!ReadFirstLine(commondirFile, commondirContent)) + return false; + + // Try gvfs-enlistment-root marker first (written during + // git worktree add by the managed hooks) + PATH_STRING markerFile = resolvedGitdir + L"\\gvfs-enlistment-root"; + std::string markerContent; + if (ReadFirstLine(markerFile, markerContent) && !markerContent.empty()) + { + std::replace(markerContent.begin(), markerContent.end(), '/', '\\'); + enlistmentRoot = ResolvePath(resolvedGitdir, Utf8ToWide(markerContent)); + } + else + { + // Fall back: commondir -> shared .git dir -> src/ -> enlistment root + std::replace(commondirContent.begin(), commondirContent.end(), '/', '\\'); + PATH_STRING sharedGitDir = ResolvePath(resolvedGitdir, Utf8ToWide(commondirContent)); + + // SharedGitDir = /src/.git + size_t sep = sharedGitDir.find_last_of(L'\\'); + if (sep == std::wstring::npos) + return false; + PATH_STRING srcDir = sharedGitDir.substr(0, sep); + + sep = srcDir.find_last_of(L'\\'); + if (sep == std::wstring::npos) + return false; + enlistmentRoot = srcDir.substr(0, sep); + } + + // Validate: the resolved root must contain .gvfs + if (!DirectoryExists(enlistmentRoot + L"\\.gvfs")) + return false; + + pipeSuffix = L"_WT_" + Utf8ToWide(worktreeName); + return true; + } + + if (attrs != INVALID_FILE_ATTRIBUTES && (attrs & FILE_ATTRIBUTE_DIRECTORY)) + { + // Found a .git directory - primary repo, not a worktree + return false; + } + + size_t sep = current.find_last_of(L'\\'); + if (sep == std::wstring::npos || sep == 0) + return false; + + PATH_STRING parent = current.substr(0, sep); + if (parent == current) + return false; + + current = parent; + } } PATH_STRING GetGVFSPipeName(const char *appName) @@ -125,18 +280,26 @@ PATH_STRING GetGVFSPipeName(const char *appName) // Start in the current directory and walk up the directory tree // until we find a folder that contains the ".gvfs" folder. // For worktrees, a suffix is appended to target the worktree's mount. + // + // If .gvfs walk-up fails, fall back to worktree detection: walk up + // looking for a .git file, then resolve the primary enlistment root + // through the worktree's gitdir chain. const size_t dotGVFSRelativePathLength = sizeof(L"\\.gvfs") / sizeof(wchar_t); // TODO 640838: Support paths longer than MAX_PATH - wchar_t enlistmentRoot[MAX_PATH]; - DWORD currentDirResult = GetCurrentDirectoryW(MAX_PATH - dotGVFSRelativePathLength, enlistmentRoot); + wchar_t currentDir[MAX_PATH]; + DWORD currentDirResult = GetCurrentDirectoryW(MAX_PATH - dotGVFSRelativePathLength, currentDir); if (currentDirResult == 0 || currentDirResult > MAX_PATH - dotGVFSRelativePathLength) { die(ReturnCode::GetCurrentDirectoryFailure, "GetCurrentDirectory failed (%d)\n", GetLastError()); } - PATH_STRING finalRootPath(GetFinalPathName(enlistmentRoot)); + PATH_STRING finalRootPath(GetFinalPathName(currentDir)); + + // Phase 1: Try .gvfs walk-up (the common case for primary enlistments + // and worktrees placed under the enlistment root) + wchar_t enlistmentRoot[MAX_PATH]; errno_t copyResult = wcscpy_s(enlistmentRoot, finalRootPath.c_str()); if (copyResult != 0) { @@ -150,7 +313,7 @@ PATH_STRING GetGVFSPipeName(const char *appName) enlistmentRootLength++; } - // Walk up enlistmentRoot looking for a folder named .gvfs + bool foundGvfs = false; wchar_t* lastslash = enlistmentRoot + enlistmentRootLength - 1; WIN32_FIND_DATAW findFileData; HANDLE dotGVFSHandle; @@ -163,6 +326,7 @@ PATH_STRING GetGVFSPipeName(const char *appName) FindClose(dotGVFSHandle); if (findFileData.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY) { + foundGvfs = true; break; } } @@ -175,28 +339,50 @@ PATH_STRING GetGVFSPipeName(const char *appName) if (enlistmentRoot == lastslash) { - die(ReturnCode::NotInGVFSEnlistment, "%s must be run from inside a GVFS enlistment\n", appName); + break; } *(lastslash + 1) = 0; - }; + } + + if (foundGvfs) + { + *(lastslash) = 0; - *(lastslash) = 0; + PATH_STRING namedPipe(CharUpperW(enlistmentRoot)); + std::replace(namedPipe.begin(), namedPipe.end(), L':', L'_'); + PATH_STRING pipeName = L"\\\\.\\pipe\\GVFS_" + namedPipe; - PATH_STRING namedPipe(CharUpperW(enlistmentRoot)); - std::replace(namedPipe.begin(), namedPipe.end(), L':', L'_'); - PATH_STRING pipeName = L"\\\\.\\pipe\\GVFS_" + namedPipe; + PATH_STRING worktreeSuffix = GetWorktreePipeSuffix(finalRootPath.c_str()); + if (!worktreeSuffix.empty()) + { + std::transform(worktreeSuffix.begin(), worktreeSuffix.end(), + worktreeSuffix.begin(), ::towupper); + pipeName += worktreeSuffix; + } - // Append worktree suffix if running in a worktree - PATH_STRING worktreeSuffix = GetWorktreePipeSuffix(finalRootPath.c_str()); - if (!worktreeSuffix.empty()) + return pipeName; + } + + // Phase 2: .gvfs not found - try worktree fallback + PATH_STRING resolvedRoot; + PATH_STRING worktreeSuffix; + if (TryResolveFromWorktree(finalRootPath, resolvedRoot, worktreeSuffix)) { + std::transform(resolvedRoot.begin(), resolvedRoot.end(), + resolvedRoot.begin(), ::towupper); + std::replace(resolvedRoot.begin(), resolvedRoot.end(), L':', L'_'); + PATH_STRING pipeName = L"\\\\.\\pipe\\GVFS_" + resolvedRoot; + std::transform(worktreeSuffix.begin(), worktreeSuffix.end(), worktreeSuffix.begin(), ::towupper); pipeName += worktreeSuffix; + + return pipeName; } - return pipeName; + die(ReturnCode::NotInGVFSEnlistment, "%s must be run from inside a GVFS enlistment\n", appName); + return PATH_STRING(); } PIPE_HANDLE CreatePipeToGVFS(const PATH_STRING& pipeName) diff --git a/GVFS/GVFS.UnitTests/Common/WorktreeInfoTests.cs b/GVFS/GVFS.UnitTests/Common/WorktreeInfoTests.cs index 9ebe56963..558493dfa 100644 --- a/GVFS/GVFS.UnitTests/Common/WorktreeInfoTests.cs +++ b/GVFS/GVFS.UnitTests/Common/WorktreeInfoTests.cs @@ -183,5 +183,73 @@ public void ReturnsNullForPrimaryFromSubdirectory() GVFSEnlistment.WorktreeInfo info = GVFSEnlistment.TryGetWorktreeInfo(subDir); info.ShouldBeNull(); } + + [TestCase] + public void GetEnlistmentRootReadsMarkerFile() + { + string enlistmentRoot = Path.Combine(this.testRoot, "enlistment"); + string primaryGitDir = Path.Combine(enlistmentRoot, "src", ".git"); + string worktreeGitDir = Path.Combine(primaryGitDir, "worktrees", "remote-wt"); + Directory.CreateDirectory(worktreeGitDir); + File.WriteAllText(Path.Combine(worktreeGitDir, "commondir"), "../.."); + + // Write the marker file (same as hooks do during git worktree add) + File.WriteAllText( + Path.Combine(worktreeGitDir, GVFSEnlistment.WorktreeInfo.EnlistmentRootFileName), + enlistmentRoot); + + // Worktree placed outside the enlistment tree + string worktreeDir = Path.Combine(this.testRoot, "remote-location", "remote-wt"); + Directory.CreateDirectory(worktreeDir); + File.WriteAllText(Path.Combine(worktreeDir, ".git"), "gitdir: " + worktreeGitDir); + + GVFSEnlistment.WorktreeInfo info = GVFSEnlistment.TryGetWorktreeInfo(worktreeDir); + info.ShouldNotBeNull(); + info.GetEnlistmentRoot().ShouldEqual(enlistmentRoot); + } + + [TestCase] + public void GetEnlistmentRootFallsBackToSharedGitDir() + { + string enlistmentRoot = Path.Combine(this.testRoot, "enlistment"); + string primaryGitDir = Path.Combine(enlistmentRoot, "src", ".git"); + string worktreeGitDir = Path.Combine(primaryGitDir, "worktrees", "fallback-wt"); + Directory.CreateDirectory(worktreeGitDir); + File.WriteAllText(Path.Combine(worktreeGitDir, "commondir"), "../.."); + + // No marker file — fallback should derive from SharedGitDir + string worktreeDir = Path.Combine(this.testRoot, "elsewhere", "fallback-wt"); + Directory.CreateDirectory(worktreeDir); + File.WriteAllText(Path.Combine(worktreeDir, ".git"), "gitdir: " + worktreeGitDir); + + GVFSEnlistment.WorktreeInfo info = GVFSEnlistment.TryGetWorktreeInfo(worktreeDir); + info.ShouldNotBeNull(); + + // SharedGitDir = /src/.git → parent = src → parent = enlistmentRoot + info.GetEnlistmentRoot().ShouldEqual(enlistmentRoot); + } + + [TestCase] + public void GetEnlistmentRootPrefersMarkerOverFallback() + { + string actualRoot = Path.Combine(this.testRoot, "actual-root"); + string primaryGitDir = Path.Combine(this.testRoot, "different-structure", ".git"); + string worktreeGitDir = Path.Combine(primaryGitDir, "worktrees", "marker-wt"); + Directory.CreateDirectory(worktreeGitDir); + File.WriteAllText(Path.Combine(worktreeGitDir, "commondir"), "../.."); + + // Write marker pointing to a different root than what SharedGitDir would derive + File.WriteAllText( + Path.Combine(worktreeGitDir, GVFSEnlistment.WorktreeInfo.EnlistmentRootFileName), + actualRoot); + + string worktreeDir = Path.Combine(this.testRoot, "marker-wt"); + Directory.CreateDirectory(worktreeDir); + File.WriteAllText(Path.Combine(worktreeDir, ".git"), "gitdir: " + worktreeGitDir); + + GVFSEnlistment.WorktreeInfo info = GVFSEnlistment.TryGetWorktreeInfo(worktreeDir); + info.ShouldNotBeNull(); + info.GetEnlistmentRoot().ShouldEqual(actualRoot); + } } } From 6c01575f245335fdc9562c28085f86eb29ae3259 Mon Sep 17 00:00:00 2001 From: Nathan Baird Date: Fri, 22 May 2026 13:21:44 -0700 Subject: [PATCH 2/5] Eliminate conhost.exe from git.exe and hooks process launches GVFS launches git.exe processes with CreateNoWindow=true (or CREATE_NO_WINDOW in native code). Despite the name, this flag tells Windows to create a new hidden console for each child process, which allocates a conhost.exe instance. For frequent, small git operations (e.g., during prefetch), the per-process conhost creation/teardown overhead is disproportionately large. Changes: - GitProcess.cs: Set CreateNoWindow=false. With UseShellExecute=false and stdout/stderr redirected to pipes, the child inherits the parent's console state. Since GVFS.Mount runs as a service with no console, the child gets no console and no conhost. Also remove the unused redirectStandardError parameter (all callers pass true). - ProcessHelper.cs: Set CreateNoWindow=false unconditionally. When redirectOutput is true, I/O goes through pipes so no console is needed. When redirectOutput is false, the child inherits the parent's console handles, which is correct for terminal contexts and harmless in service contexts (output was already going to an invisible hidden console). - GitHooksLoader.cpp: Use DETACHED_PROCESS instead of CREATE_NO_WINDOW in the no-console branch. Explicitly detaches from any console without allocating a new one. The console branch (user terminal) is unchanged. Verified with edge-case tests across terminal, hidden-console, and fully-detached (DETACHED_PROCESS) parent contexts. All git status, git fetch (prefetch hook), and gvfs health scenarios pass. Assisted-by: Tyrie Vella Assisted-by: Claude Opus 4.6 Signed-off-by: Tyrie Vella --- GVFS/GVFS.Common/Git/GitProcess.cs | 16 ++++++++++++---- GVFS/GVFS.Common/ProcessHelper.cs | 11 ++++++++++- GVFS/GitHooksLoader/GitHooksLoader.cpp | 2 +- 3 files changed, 23 insertions(+), 6 deletions(-) diff --git a/GVFS/GVFS.Common/Git/GitProcess.cs b/GVFS/GVFS.Common/Git/GitProcess.cs index caca4df64..b818fd915 100644 --- a/GVFS/GVFS.Common/Git/GitProcess.cs +++ b/GVFS/GVFS.Common/Git/GitProcess.cs @@ -815,16 +815,24 @@ public Result MultiPackIndexRepack(string gitObjectDirectory, string batchSize) return this.InvokeGitAgainstDotGitFolder($"-c pack.threads=1 -c repack.packKeptObjects=true multi-pack-index repack --object-dir=\"{gitObjectDirectory}\" --batch-size={batchSize} --no-progress"); } - public Process GetGitProcess(string command, string workingDirectory, string dotGitDirectory, bool useReadObjectHook, bool redirectStandardError, string gitObjectsDirectory, bool usePreCommandHook) + public Process GetGitProcess(string command, string workingDirectory, string dotGitDirectory, bool useReadObjectHook, string gitObjectsDirectory, bool usePreCommandHook) { ProcessStartInfo processInfo = new ProcessStartInfo(this.gitBinPath); processInfo.WorkingDirectory = workingDirectory; processInfo.UseShellExecute = false; processInfo.RedirectStandardInput = true; processInfo.RedirectStandardOutput = true; - processInfo.RedirectStandardError = redirectStandardError; + processInfo.RedirectStandardError = true; processInfo.WindowStyle = ProcessWindowStyle.Hidden; - processInfo.CreateNoWindow = true; + + // CreateNoWindow=false avoids allocating a hidden conhost.exe per child + // process. This is safe because both stdout and stderr are redirected via + // pipes, so the child never needs a console for I/O. If a future change + // stops redirecting either stream (to forward output to the parent console + // instead), CreateNoWindow must be set to true for that case — otherwise + // the non-redirected stream inherits the parent's console handle, which + // may be absent when running as a service, causing lost output. + processInfo.CreateNoWindow = false; processInfo.StandardOutputEncoding = UTF8NoBOM; processInfo.StandardErrorEncoding = UTF8NoBOM; @@ -903,7 +911,7 @@ protected virtual Result InvokeGitImpl( // From https://msdn.microsoft.com/en-us/library/system.diagnostics.process.standardoutput.aspx // To avoid deadlocks, use asynchronous read operations on at least one of the streams. // Do not perform a synchronous read to the end of both redirected streams. - using (this.executingProcess = this.GetGitProcess(command, workingDirectory, dotGitDirectory, useReadObjectHook, redirectStandardError: true, gitObjectsDirectory: gitObjectsDirectory, usePreCommandHook: usePreCommandHook)) + using (this.executingProcess = this.GetGitProcess(command, workingDirectory, dotGitDirectory, useReadObjectHook, gitObjectsDirectory: gitObjectsDirectory, usePreCommandHook: usePreCommandHook)) { StringBuilder output = new StringBuilder(); StringBuilder errors = new StringBuilder(); diff --git a/GVFS/GVFS.Common/ProcessHelper.cs b/GVFS/GVFS.Common/ProcessHelper.cs index a9731d6d5..a67f4159e 100644 --- a/GVFS/GVFS.Common/ProcessHelper.cs +++ b/GVFS/GVFS.Common/ProcessHelper.cs @@ -18,7 +18,16 @@ public static ProcessResult Run(string programName, string args, bool redirectOu processInfo.RedirectStandardOutput = redirectOutput; processInfo.RedirectStandardError = redirectOutput; processInfo.WindowStyle = ProcessWindowStyle.Hidden; - processInfo.CreateNoWindow = redirectOutput; + + // CreateNoWindow=false avoids allocating a hidden conhost.exe per child + // process. When redirectOutput is true, I/O goes through pipes so no + // console is needed. When redirectOutput is false, the child inherits the + // parent's console handles — this works when the parent has a console + // (e.g., GVFS.Hooks invoked from a terminal), but output is silently lost + // when the parent has no console (e.g., service context). This is + // acceptable because CreateNoWindow=true would only send that output to + // an invisible hidden console instead. + processInfo.CreateNoWindow = false; processInfo.Arguments = args; return Run(processInfo); diff --git a/GVFS/GitHooksLoader/GitHooksLoader.cpp b/GVFS/GitHooksLoader/GitHooksLoader.cpp index dfb259b87..9f1619d0a 100644 --- a/GVFS/GitHooksLoader/GitHooksLoader.cpp +++ b/GVFS/GitHooksLoader/GitHooksLoader.cpp @@ -129,7 +129,7 @@ int ExecuteHook(const std::wstring &applicationName, wchar_t *hookName, int argc /* Git disallows stdin from hooks */ si.dwFlags = STARTF_USESTDHANDLES; - creationFlags |= CREATE_NO_WINDOW; + creationFlags |= DETACHED_PROCESS; } ZeroMemory(&pi, sizeof(pi)); From 249b895d8c86285b9921ca74dd36fed34f382a06 Mon Sep 17 00:00:00 2001 From: Tyrie Vella Date: Tue, 26 May 2026 11:46:59 -0700 Subject: [PATCH 3/5] Fix flaky CreatePlaceholderTests stderr comparison Git's "Updating files" progress output on stderr is timing-dependent, causing ErrorsShouldMatch to fail when one repo emits progress and the other does not. Set GIT_PROGRESS_DELAY=3600 on all test git invocations so progress output is never emitted for commands completing under an hour. This eliminates the flake at the source without skipping error validation. Assisted-by: Claude Opus 4.6 Signed-off-by: Tyrie Vella --- GVFS/GVFS.FunctionalTests/Tools/GitProcess.cs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/GVFS/GVFS.FunctionalTests/Tools/GitProcess.cs b/GVFS/GVFS.FunctionalTests/Tools/GitProcess.cs index bc2135465..abc94f06d 100644 --- a/GVFS/GVFS.FunctionalTests/Tools/GitProcess.cs +++ b/GVFS/GVFS.FunctionalTests/Tools/GitProcess.cs @@ -35,6 +35,9 @@ public static ProcessResult InvokeProcess( } processInfo.EnvironmentVariables["GIT_TERMINAL_PROMPT"] = "0"; + // Suppress progress output (e.g. "Updating files: 100%") which is timing-dependent + // and causes flaky stderr comparisons between control and GVFS repos. + processInfo.EnvironmentVariables["GIT_PROGRESS_DELAY"] = "3600"; if (environmentVariables != null) { From abe5da6eefd862018262cb95b5fccd61cdb8d559 Mon Sep 17 00:00:00 2001 From: Tyrie Vella Date: Wed, 27 May 2026 11:39:15 -0700 Subject: [PATCH 4/5] Fix PID-recycling race in orphan-lock detection GVFS's orphan-lock check (GVFSLock.LockHolder.GetExternalHolder) only asked the OS "is there a process at this PID?" via IsProcessActive. If the original lock-holder exited and Windows recycled its PID to an unrelated process before the next git command arrived, GVFS saw a live process at that PID and concluded the lock was still held. The git command then either waited the full 300s timeout or printed a spurious "Waiting for '' to release the lock" message. This is a real product bug, not just a test issue. It surfaced as flakiness in the OrphanedGVFSLockIsCleanedUp functional test on busy CI agents (two failure modes: 300s timeout, and assertion failure with non-empty stderr), but the same race can affect users when a PID is quickly reassigned by the OS scheduler. The fix is process-identity tracking. When the lock is granted to an external requestor, capture that process's creation timestamp (GetProcessTimes, an opaque 100-ns FILETIME value) alongside its PID. On every orphan check, look up the current creation timestamp at that PID and compare. If the process is gone the lookup fails; if a different process is now at the PID the timestamps differ. Either way the lock is correctly recognized as orphaned. Implementation notes: * New abstract GVFSPlatform.TryGetActiveProcessStartTime returns the raw 64-bit creation time. It is documented as identity-only; the value is never decoded as a date. * The Windows implementation combines GetExitCodeProcess (STILL_ACTIVE check) and GetProcessTimes. GetProcessTimes alone is not sufficient because it still returns a creation time for terminated processes whose kernel object is kept alive by an outstanding handle elsewhere. * Acquisition does not fail if start-time capture fails. The existing IsProcessActiveImplementation has a Process.GetProcessById fallback for cross-integrity callers that OpenProcess(QueryLimitedInformation) cannot open, and we preserve the same compatibility surface here. A HasStartTime flag is stored alongside the PID; when it is false the orphan check falls back to the legacy IsProcessActive call. The fallback is recorded in telemetry (StartTimeUnavailable=true on the TryAcquireLockExternal event) so we can see if it ever becomes common in the field. * The ExternalLockHolderExited telemetry event now carries an ExternalHolderTerminationReason field (ProcessNotActive | PidRecycled | Unknown) to make future post-mortems straightforward. * The OrphanedGVFSLockIsCleanedUp functional test is unchanged. It polls Process.GetProcessById to detect when the lock holder has exited, then runs git status. With this product fix, git status succeeds cleanly even if the PID has already been recycled to an unrelated process by the time mount processes the request, so the test passes naturally without any test-side workaround. * Two new unit tests cover the new identity-check path: TryAcquireLockForExternalRequestor_WhenHolderPidRecycled (verifies that an orphan is detected when start times differ at the same PID) and TryAcquireLockForExternalRequestor_WhenHolderStartTimeMatches (the inverse: a still-alive holder is correctly seen as active and a new acquisition is denied). Assisted-by: Claude Opus 4.7 Signed-off-by: Tyrie Vella --- GVFS/GVFS.Common/GVFSLock.cs | 71 ++++++++++++++--- GVFS/GVFS.Common/GVFSPlatform.cs | 13 ++++ GVFS/GVFS.Common/NativeMethods.Shared.cs | 12 +++ .../WindowsPlatform.Shared.cs | 38 +++++++++ GVFS/GVFS.Platform.Windows/WindowsPlatform.cs | 5 ++ GVFS/GVFS.UnitTests/Common/GVFSLockTests.cs | 78 +++++++++++++++++++ .../Mock/Common/MockPlatform.cs | 23 ++++++ 7 files changed, 230 insertions(+), 10 deletions(-) diff --git a/GVFS/GVFS.Common/GVFSLock.cs b/GVFS/GVFS.Common/GVFSLock.cs index 5bfaa5976..c8cac11ff 100644 --- a/GVFS/GVFS.Common/GVFSLock.cs +++ b/GVFS/GVFS.Common/GVFSLock.cs @@ -41,6 +41,20 @@ public bool TryAcquireLockForExternalRequestor( existingExternalHolder = null; + // Capture the requestor's process start time so we can later distinguish the + // genuine holder from an unrelated process that happens to be reusing the same + // PID after the holder exits. If we cannot read the start time (e.g. permission + // failure on OpenProcess for a different-integrity caller) we still accept the + // lock and fall back to the legacy PID-only orphan check; record the fallback in + // telemetry so we can spot if it becomes common. + long? requestorStartTime = GVFSPlatform.Instance.TryGetActiveProcessStartTime(requestor.PID, out long startTime) + ? startTime + : (long?)null; + if (requestorStartTime == null) + { + metadata.Add("StartTimeUnavailable", true); + } + try { lock (this.acquisitionLock) @@ -65,7 +79,7 @@ public bool TryAcquireLockForExternalRequestor( metadata.Add("Result", "Accepted"); eventLevel = EventLevel.Informational; - this.currentLockHolder.AcquireForExternalRequestor(requestor); + this.currentLockHolder.AcquireForExternalRequestor(requestor, requestorStartTime); this.Stats = new ActiveGitCommandStats(); return true; @@ -190,12 +204,14 @@ private bool IsLockAvailable(bool checkExternalHolderOnly, out NamedPipeMessages } bool externalHolderTerminatedWithoutReleasingLock; + string terminationReason; existingExternalHolder = this.currentLockHolder.GetExternalHolder( - out externalHolderTerminatedWithoutReleasingLock); + out externalHolderTerminatedWithoutReleasingLock, + out terminationReason); if (externalHolderTerminatedWithoutReleasingLock) { - this.ReleaseLockForTerminatedProcess(existingExternalHolder.PID); + this.ReleaseLockForTerminatedProcess(existingExternalHolder.PID, terminationReason); this.tracer.SetGitCommandSessionId(string.Empty); existingExternalHolder = null; } @@ -204,11 +220,11 @@ private bool IsLockAvailable(bool checkExternalHolderOnly, out NamedPipeMessages } } - private bool ReleaseExternalLock(int pid, string eventName) + private bool ReleaseExternalLock(int pid, string eventName, EventMetadata extraMetadata = null) { lock (this.acquisitionLock) { - EventMetadata metadata = new EventMetadata(); + EventMetadata metadata = extraMetadata ?? new EventMetadata(); try { @@ -251,9 +267,11 @@ private bool ReleaseExternalLock(int pid, string eventName) } } - private void ReleaseLockForTerminatedProcess(int pid) + private void ReleaseLockForTerminatedProcess(int pid, string terminationReason) { - this.ReleaseExternalLock(pid, "ExternalLockHolderExited"); + EventMetadata metadata = new EventMetadata(); + metadata.Add("ExternalHolderTerminationReason", terminationReason ?? "Unknown"); + this.ReleaseExternalLock(pid, "ExternalLockHolderExited", metadata); } // The lock release event is a convenient place to record stats about things that happened while a git command was running, @@ -383,6 +401,7 @@ public void AddStatsToTelemetry(EventMetadata metadata) private class LockHolder { private NamedPipeMessages.LockData externalLockHolder; + private long? externalLockHolderStartTime; public bool IsFree { @@ -404,7 +423,7 @@ public void AcquireForGVFS() this.IsGVFS = true; } - public void AcquireForExternalRequestor(NamedPipeMessages.LockData externalLockHolder) + public void AcquireForExternalRequestor(NamedPipeMessages.LockData externalLockHolder, long? startTime) { if (this.IsGVFS || this.externalLockHolder != null) @@ -413,12 +432,14 @@ public void AcquireForExternalRequestor(NamedPipeMessages.LockData externalLockH } this.externalLockHolder = externalLockHolder; + this.externalLockHolderStartTime = startTime; } public void Release() { this.IsGVFS = false; this.externalLockHolder = null; + this.externalLockHolderStartTime = null; } public NamedPipeMessages.LockData GetExternalHolder() @@ -426,14 +447,44 @@ public NamedPipeMessages.LockData GetExternalHolder() return this.externalLockHolder; } - public NamedPipeMessages.LockData GetExternalHolder(out bool externalHolderTerminatedWithoutReleasingLock) + public NamedPipeMessages.LockData GetExternalHolder(out bool externalHolderTerminatedWithoutReleasingLock, out string terminationReason) { externalHolderTerminatedWithoutReleasingLock = false; + terminationReason = null; if (this.externalLockHolder != null) { int pid = this.externalLockHolder.PID; - externalHolderTerminatedWithoutReleasingLock = !GVFSPlatform.Instance.IsProcessActive(pid); + + if (this.externalLockHolderStartTime is long capturedStartTime) + { + // Identity check: confirm the same process still owns this PID by comparing + // the OS-supplied process start time we captured at acquisition with the + // current one. A mismatch means the original holder exited and Windows + // recycled the PID to a different process (the bug this code fixes). + if (!GVFSPlatform.Instance.TryGetActiveProcessStartTime(pid, out long currentStartTime)) + { + externalHolderTerminatedWithoutReleasingLock = true; + terminationReason = "ProcessNotActive"; + } + else if (currentStartTime != capturedStartTime) + { + externalHolderTerminatedWithoutReleasingLock = true; + terminationReason = "PidRecycled"; + } + } + else + { + // Fallback for the rare case where we could not capture a start time at + // acquisition time (e.g. cross-integrity OpenProcess denial). Use the + // legacy PID-only liveness check, which is vulnerable to PID recycling + // but matches pre-fix behavior. + if (!GVFSPlatform.Instance.IsProcessActive(pid)) + { + externalHolderTerminatedWithoutReleasingLock = true; + terminationReason = "ProcessNotActive"; + } + } } return this.externalLockHolder; diff --git a/GVFS/GVFS.Common/GVFSPlatform.cs b/GVFS/GVFS.Common/GVFSPlatform.cs index 579e94955..d5132066b 100644 --- a/GVFS/GVFS.Common/GVFSPlatform.cs +++ b/GVFS/GVFS.Common/GVFSPlatform.cs @@ -66,6 +66,19 @@ public static void Register(GVFSPlatform platform) public abstract void PrepareProcessToRunInBackground(); public abstract bool IsProcessActive(int processId); + + /// + /// Returns true and writes an opaque, OS-supplied process-identity token (e.g. process + /// creation time on Windows) when the process with the given PID is currently active. + /// The token has no meaning beyond identity comparison: two calls for the same underlying + /// process yield equal tokens, and a call after the OS has recycled the PID to a different + /// process yields a different token. Returns false (and startTime = 0) if the process + /// is no longer running, or if it could not be identified for any reason (e.g. permission + /// failure). Callers must treat a false return as "no identity information available" and + /// fall back to if they still need a liveness check. + /// + public abstract bool TryGetActiveProcessStartTime(int processId, out long startTime); + public abstract void IsServiceInstalledAndRunning(string name, out bool installed, out bool running); public abstract string GetNamedPipeName(string enlistmentRoot); public abstract string GetGVFSServiceNamedPipeName(string serviceName); diff --git a/GVFS/GVFS.Common/NativeMethods.Shared.cs b/GVFS/GVFS.Common/NativeMethods.Shared.cs index 7a796b15e..43aca72f8 100644 --- a/GVFS/GVFS.Common/NativeMethods.Shared.cs +++ b/GVFS/GVFS.Common/NativeMethods.Shared.cs @@ -158,6 +158,18 @@ public static extern SafeFileHandle OpenProcess( [return: MarshalAs(UnmanagedType.Bool)] public static extern bool GetExitCodeProcess(SafeFileHandle hProcess, out uint lpExitCode); + // GetProcessTimes writes four FILETIME values (each two DWORDs / 8 bytes). + // We marshal each as `out long` because we only ever compare the raw 64-bit value + // (creation time) for identity purposes; we never decode it as a date. + [DllImport("kernel32.dll", SetLastError = true)] + [return: MarshalAs(UnmanagedType.Bool)] + public static extern bool GetProcessTimes( + SafeFileHandle hProcess, + out long lpCreationTime, + out long lpExitTime, + out long lpKernelTime, + out long lpUserTime); + [DllImport("kernel32.dll", SetLastError = true, CharSet = CharSet.Unicode)] private static extern SafeFileHandle CreateFile( [In] string lpFileName, diff --git a/GVFS/GVFS.Platform.Windows/WindowsPlatform.Shared.cs b/GVFS/GVFS.Platform.Windows/WindowsPlatform.Shared.cs index 80cc09f68..99def6841 100644 --- a/GVFS/GVFS.Platform.Windows/WindowsPlatform.Shared.cs +++ b/GVFS/GVFS.Platform.Windows/WindowsPlatform.Shared.cs @@ -70,6 +70,44 @@ public static bool IsProcessActiveImplementation(int processId, bool tryGetProce } } + /// + /// Returns true if a process with the given PID is currently active AND writes its + /// creation timestamp (raw FILETIME, 100-ns ticks since 1601) to . + /// The returned value is intended for identity comparison only -- two calls for the + /// same underlying process (no PID reuse) always yield equal values; if the OS recycles + /// a PID to a new process the value will differ. Returns false (and startTime = 0) + /// if the process is gone, has terminated (but its kernel object lingers due to an + /// outstanding handle elsewhere), or cannot be opened for QueryLimitedInformation. + /// + public static bool TryGetActiveProcessStartTimeImplementation(int processId, out long startTime) + { + startTime = 0; + + using (SafeFileHandle process = NativeMethods.OpenProcess(NativeMethods.ProcessAccessFlags.QueryLimitedInformation, false, processId)) + { + if (process.IsInvalid) + { + return false; + } + + // GetProcessTimes succeeds for terminated processes whose kernel object still + // exists (e.g., an outstanding handle elsewhere). Confirm the process is still + // running before trusting the creation time as an identity marker. + if (!NativeMethods.GetExitCodeProcess(process, out uint exitCode) || exitCode != StillActive) + { + return false; + } + + if (!NativeMethods.GetProcessTimes(process, out long creationTime, out _, out _, out _)) + { + return false; + } + + startTime = creationTime; + return true; + } + } + public static string GetNamedPipeNameImplementation(string enlistmentRoot) { return "GVFS_" + enlistmentRoot.ToUpper().Replace(':', '_'); diff --git a/GVFS/GVFS.Platform.Windows/WindowsPlatform.cs b/GVFS/GVFS.Platform.Windows/WindowsPlatform.cs index d262a7953..b8593f417 100644 --- a/GVFS/GVFS.Platform.Windows/WindowsPlatform.cs +++ b/GVFS/GVFS.Platform.Windows/WindowsPlatform.cs @@ -205,6 +205,11 @@ public override bool IsProcessActive(int processId) return WindowsPlatform.IsProcessActiveImplementation(processId, tryGetProcessById: true); } + public override bool TryGetActiveProcessStartTime(int processId, out long startTime) + { + return WindowsPlatform.TryGetActiveProcessStartTimeImplementation(processId, out startTime); + } + public override void IsServiceInstalledAndRunning(string name, out bool installed, out bool running) { ServiceController service = ServiceController.GetServices().FirstOrDefault(s => s.ServiceName.Equals(name, StringComparison.Ordinal)); diff --git a/GVFS/GVFS.UnitTests/Common/GVFSLockTests.cs b/GVFS/GVFS.UnitTests/Common/GVFSLockTests.cs index fb0a7c5a4..91173fd7b 100644 --- a/GVFS/GVFS.UnitTests/Common/GVFSLockTests.cs +++ b/GVFS/GVFS.UnitTests/Common/GVFSLockTests.cs @@ -182,6 +182,84 @@ public void TryAcquireLockForExternalRequestor_WhenExternalHolderTerminated() mockTracer.VerifyAll(); } + /// + /// Regression test for OrphanedGVFSLockIsCleanedUp flake: simulates the original lock holder + /// exiting and the OS recycling its PID to a different, unrelated process before the orphan + /// check fires. Identity is preserved via process start time, so the orphan must be detected + /// even though the (now-different) process at the holder's PID still appears active. + /// + [TestCase] + public void TryAcquireLockForExternalRequestor_WhenHolderPidRecycled() + { + Mock mockTracer = new Mock(MockBehavior.Strict); + mockTracer.Setup(x => x.RelatedEvent(EventLevel.Informational, "TryAcquireLockExternal", It.IsAny())); + mockTracer.Setup(x => x.RelatedEvent(EventLevel.Informational, "ExternalLockHolderExited", It.IsAny(), Keywords.Telemetry)); + mockTracer.Setup(x => x.SetGitCommandSessionId(string.Empty)); + MockPlatform mockPlatform = (MockPlatform)GVFSPlatform.Instance; + + // Acquire as DefaultLockData.PID with an explicit start time so the new identity-check + // path (not the legacy fallback) runs. + mockPlatform.ActiveProcesses.Add(DefaultLockData.PID); + mockPlatform.ProcessStartTimes[DefaultLockData.PID] = 1000; + GVFSLock gvfsLock = new GVFSLock(mockTracer.Object); + NamedPipeMessages.LockData existingExternalHolder; + gvfsLock.TryAcquireLockForExternalRequestor(DefaultLockData, out existingExternalHolder).ShouldBeTrue(); + existingExternalHolder.ShouldBeNull(); + this.ValidateLockHeld(gvfsLock, DefaultLockData); + + // Simulate PID recycling: the PID is still reported active (a different process now owns it), + // but the start time differs from the one we captured at acquisition. + mockPlatform.ProcessStartTimes[DefaultLockData.PID] = 2000; + + NamedPipeMessages.LockData newLockData = new NamedPipeMessages.LockData(4321, false, false, "git new", "456"); + mockPlatform.ActiveProcesses.Add(newLockData.PID); + mockPlatform.ProcessStartTimes[newLockData.PID] = 3000; + gvfsLock.TryAcquireLockForExternalRequestor(newLockData, out existingExternalHolder).ShouldBeTrue(); + existingExternalHolder.ShouldBeNull(); + this.ValidateLockHeld(gvfsLock, newLockData); + + mockPlatform.ActiveProcesses.Remove(DefaultLockData.PID); + mockPlatform.ActiveProcesses.Remove(newLockData.PID); + mockPlatform.ProcessStartTimes.Remove(DefaultLockData.PID); + mockPlatform.ProcessStartTimes.Remove(newLockData.PID); + mockTracer.VerifyAll(); + } + + /// + /// Inverse of the PID-recycle test: when start time still matches, the original holder + /// must still be considered the active holder and a new acquisition must be denied. + /// Guards against an over-eager orphan detector that would release any lock on every check. + /// + [TestCase] + public void TryAcquireLockForExternalRequestor_WhenHolderStartTimeMatches() + { + Mock mockTracer = new Mock(MockBehavior.Strict); + mockTracer.Setup(x => x.RelatedEvent(EventLevel.Informational, "TryAcquireLockExternal", It.IsAny())); + mockTracer.Setup(x => x.RelatedEvent(EventLevel.Verbose, "TryAcquireLockExternal", It.IsAny())); + MockPlatform mockPlatform = (MockPlatform)GVFSPlatform.Instance; + + mockPlatform.ActiveProcesses.Add(DefaultLockData.PID); + mockPlatform.ProcessStartTimes[DefaultLockData.PID] = 1000; + GVFSLock gvfsLock = new GVFSLock(mockTracer.Object); + NamedPipeMessages.LockData existingExternalHolder; + gvfsLock.TryAcquireLockForExternalRequestor(DefaultLockData, out existingExternalHolder).ShouldBeTrue(); + this.ValidateLockHeld(gvfsLock, DefaultLockData); + + // Start time is unchanged -- holder is genuinely still alive; new acquire must be denied. + NamedPipeMessages.LockData newLockData = new NamedPipeMessages.LockData(4321, false, false, "git new", "456"); + mockPlatform.ActiveProcesses.Add(newLockData.PID); + mockPlatform.ProcessStartTimes[newLockData.PID] = 3000; + gvfsLock.TryAcquireLockForExternalRequestor(newLockData, out existingExternalHolder).ShouldBeFalse(); + this.ValidateExistingExternalHolder(DefaultLockData, existingExternalHolder); + this.ValidateLockHeld(gvfsLock, DefaultLockData); + + mockPlatform.ActiveProcesses.Remove(DefaultLockData.PID); + mockPlatform.ActiveProcesses.Remove(newLockData.PID); + mockPlatform.ProcessStartTimes.Remove(DefaultLockData.PID); + mockPlatform.ProcessStartTimes.Remove(newLockData.PID); + mockTracer.VerifyAll(); + } + private GVFSLock AcquireDefaultLock(MockPlatform mockPlatform, ITracer mockTracer) { GVFSLock gvfsLock = new GVFSLock(mockTracer); diff --git a/GVFS/GVFS.UnitTests/Mock/Common/MockPlatform.cs b/GVFS/GVFS.UnitTests/Mock/Common/MockPlatform.cs index 8c153a424..41876f953 100644 --- a/GVFS/GVFS.UnitTests/Mock/Common/MockPlatform.cs +++ b/GVFS/GVFS.UnitTests/Mock/Common/MockPlatform.cs @@ -45,6 +45,14 @@ public override bool SupportsSystemInstallLog public HashSet ActiveProcesses { get; } = new HashSet(); + /// + /// Optional per-PID start-time overrides used by tests that exercise PID-reuse + /// detection. When a PID is in but not in this + /// dictionary, returns the PID itself + /// as a synthetic non-zero start time. + /// + public Dictionary ProcessStartTimes { get; } = new Dictionary(); + public override void ConfigureVisualStudio(string gitBinPath, ITracer tracer) { throw new NotSupportedException(); @@ -138,6 +146,21 @@ public override bool IsProcessActive(int processId) return this.ActiveProcesses.Contains(processId); } + public override bool TryGetActiveProcessStartTime(int processId, out long startTime) + { + if (this.ActiveProcesses.Contains(processId)) + { + // If the test populated an explicit start time use that, otherwise fall back + // to a deterministic non-zero value so test scenarios that don't care about + // PID identity (the existing majority of unit tests) keep working. + startTime = this.ProcessStartTimes.TryGetValue(processId, out long stored) ? stored : processId; + return true; + } + + startTime = 0; + return false; + } + public override void IsServiceInstalledAndRunning(string name, out bool installed, out bool running) { throw new NotSupportedException(); From 452e2fb3381efee37adb0894601d5f6154544ddc Mon Sep 17 00:00:00 2001 From: Tyrie Vella Date: Wed, 27 May 2026 13:03:46 -0700 Subject: [PATCH 5/5] Release pipeline: route vcpkg through Terrapin asset cache The official release pipeline has been failing since libgit2 was statically linked via vcpkg, because the pool's build agents do not have direct public-internet access. vcpkg was reaching out to sourceforge.net (and other origins) for libgit2 transitive dependencies (pcre, zlib, http_parser, ...) which the network layer blocked, causing WinHttpSendRequest 12029 errors. Route vcpkg asset downloads through the internal Terrapin cache (the canonical CI asset-caching service). This is opt-in. External contributors and the public GitHub Actions PR-validation workflow (.github/workflows/build.yaml) continue to use the standard vcpkg flow with direct downloads from public origins. No internal feed authentication is required to build VFS for Git from a fresh clone, and the repo source code intentionally contains no msbuild SDK references that would pull internal packages into a default restore. Changes: Directory.Build.props Add IsLocalBuild and UseTerrapinAssetCache properties, both defaulted to false. IsLocalBuild controls Terrapin upload-on-fetch (allowed on dev boxes only); UseTerrapinAssetCache opts the build into the Terrapin asset-cache machinery (release pipeline only). Directory.Build.targets Build a --x-asset-sources string that combines the Terrapin retrieval script with x-block-origin, gated on UseTerrapinAssetCache=true, and append it to both vcpkg install invocations. Add a guard that requires the pipeline to supply TerrapinRetrievalToolPath up front when asset caching is enabled. Also validate static git2.lib (was only checking dynamic git2.dll). .azure-pipelines/official-release-nuget.config (new) Pipeline-only NuGet config that points at an internal feed. Used solely by a one-off `nuget install` of Microsoft.Build.Vcpkg, not by any csproj restore. Not consulted by external contributors or by GitHub Actions. .azure-pipelines/release.yml Add NuGetAuthenticate@1 task. Add a NuGetCommand@2 task that out-of-band installs the internal Microsoft.Build.Vcpkg package (which ships TerrapinRetrievalTool.exe) into $(Agent.TempDirectory). Add an explicit "Restore vcpkg native libraries (Terrapin cache)" step that invokes _RestoreVcpkgDependencies on GVFS.Common.csproj with -p:UseTerrapinAssetCache=true and TerrapinRetrievalToolPath pointing at the extracted tool. Build.bat's own vcpkg install step then skips because the libs are already present. Assisted-by: Claude Opus 4.7 Signed-off-by: Tyrie Vella --- .../official-release-nuget.config | 29 ++++++++++++++++ .azure-pipelines/release.yml | 34 +++++++++++++++++++ Directory.Build.props | 19 +++++++++++ Directory.Build.targets | 34 +++++++++++++++++-- 4 files changed, 113 insertions(+), 3 deletions(-) create mode 100644 .azure-pipelines/official-release-nuget.config diff --git a/.azure-pipelines/official-release-nuget.config b/.azure-pipelines/official-release-nuget.config new file mode 100644 index 000000000..4df1aaa4c --- /dev/null +++ b/.azure-pipelines/official-release-nuget.config @@ -0,0 +1,29 @@ + + + + + + + + diff --git a/.azure-pipelines/release.yml b/.azure-pipelines/release.yml index 84b9a2b80..9433cb2f5 100644 --- a/.azure-pipelines/release.yml +++ b/.azure-pipelines/release.yml @@ -111,6 +111,9 @@ extends: inputs: versionSpec: '6.x' + - task: NuGetAuthenticate@1 + displayName: 'Authenticate to internal NuGet feed (for Microsoft.Build.Vcpkg)' + - task: PowerShell@2 displayName: 'Install VS C++ workload (NativeAOT prerequisite)' inputs: @@ -121,6 +124,37 @@ extends: inputs: filePath: $(Build.SourcesDirectory)\.azure-pipelines\scripts\enable-projfs.ps1 + # Download the Microsoft.Build.Vcpkg NuGet package out-of-band so we + # can hand the build a path to TerrapinRetrievalTool.exe via + # -p:TerrapinRetrievalToolPath. The package is pulled from an + # internal NuGet feed (see .azure-pipelines/official-release-nuget.config). + # Downloading it this way -- rather than as an msbuild Sdk import -- + # keeps the internal feed out of the root nuget.config that + # external contributors and the public GitHub Actions workflow see. + - task: NuGetCommand@2 + displayName: 'Download Microsoft.Build.Vcpkg package (Terrapin retrieval tool)' + inputs: + command: custom + arguments: 'install Microsoft.Build.Vcpkg -Version 2026.5.25.434-aa40adda53 -ConfigFile $(Build.SourcesDirectory)\.azure-pipelines\official-release-nuget.config -OutputDirectory $(Agent.TempDirectory)\nuget-internal -ExcludeVersion -DirectDownload -NonInteractive' + + # Restore vcpkg native dependencies through the Terrapin asset + # cache (the release pipeline's build agents have x-block-origin + # enforced and cannot download from the public internet). Runs the + # _RestoreVcpkgDependencies MSBuild target with + # UseTerrapinAssetCache=true and TerrapinRetrievalToolPath pointing + # at the binary extracted by the previous step. vcpkg downloads + # then route through https://vcpkg.storage.devpackages.microsoft.io. + # Build.bat's own vcpkg install step then skips because the libs + # are already present. + - script: | + dotnet build "$(Build.SourcesDirectory)\GVFS\GVFS.Common\GVFS.Common.csproj" ^ + /t:_RestoreVcpkgDependencies ^ + -c $(BuildConfiguration) ^ + -p:UseTerrapinAssetCache=true ^ + -p:TerrapinRetrievalToolPath=$(Agent.TempDirectory)\nuget-internal\Microsoft.Build.Vcpkg\trt\TerrapinRetrievalTool.exe ^ + -v:detailed + displayName: 'Restore vcpkg native libraries (Terrapin cache)' + - script: | $(Build.SourcesDirectory)\scripts\Build.bat ^ $(BuildConfiguration) ^ diff --git a/Directory.Build.props b/Directory.Build.props index 89953442c..b7752b80f 100644 --- a/Directory.Build.props +++ b/Directory.Build.props @@ -18,6 +18,25 @@ but our .NET projects do. --> true + + + false + + + false diff --git a/Directory.Build.targets b/Directory.Build.targets index d04b30b4f..f78140643 100644 --- a/Directory.Build.targets +++ b/Directory.Build.targets @@ -32,6 +32,22 @@ <_VcpkgManifestFile>$(RepoSrcPath)vcpkg.json <_VcpkgConfigFile>$(RepoSrcPath)vcpkg-configuration.json <_VcpkgStampFile>$(RepoOutPath)vcpkg_installed\.msbuildstamp + + + <_VcpkgAssetSources Condition="'$(UseTerrapinAssetCache)' == 'true'">"--x-asset-sources=x-script,$(TerrapinRetrievalToolPath) -b https://vcpkg.storage.devpackages.microsoft.io/artifacts/ -a $(IsLocalBuild) -p {url} -s {sha512} -d {dst};x-block-origin" @@ -70,13 +86,25 @@ - + + + - + Text="vcpkg install completed but git2.dll (dynamic) is missing. Check vcpkg output above for errors." /> +