From 9ab464e6fcec35d8d2308f1267cf10ca1b5f6e5d Mon Sep 17 00:00:00 2001 From: Tyrie Vella Date: Wed, 1 Jul 2026 10:46:25 -0700 Subject: [PATCH 01/24] Include executable extension in core.virtualfilesystem path git invokes core.virtualfilesystem via the shell (use_shell=1 in virtualfilesystem.c), so on Windows the shell has to probe PATHEXT candidates (.COM, .EXE, ...) to resolve the bare "virtual-filesystem" path to the actual "virtual-filesystem.exe" file that HooksInstaller writes to disk. Since VFSForGit controls both the config value and the file it points to, append GVFSPlatform.Instance.Constants.ExecutableExtension so the path matches the file exactly, avoiding repeated PATHEXT probing on every hook invocation. This is a no-op on platforms where ExecutableExtension is empty (macOS/Linux). Assisted-by: Claude Sonnet 5 Signed-off-by: Tyrie Vella --- GVFS/GVFS.Common/Git/RequiredGitConfig.cs | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/GVFS/GVFS.Common/Git/RequiredGitConfig.cs b/GVFS/GVFS.Common/Git/RequiredGitConfig.cs index 3ab6cd0b6..a9159e663 100644 --- a/GVFS/GVFS.Common/Git/RequiredGitConfig.cs +++ b/GVFS/GVFS.Common/Git/RequiredGitConfig.cs @@ -24,8 +24,14 @@ public static Dictionary GetRequiredSettings(GVFSEnlistment enli // path would split the command. Git's config parser strips double quotes // but preserves single quotes, and bash treats single-quoted strings as // a single token. + // + // Include the platform's executable extension (matching the file actually + // written to disk by HooksInstaller) so the shell can exec it directly + // instead of probing PATHEXT candidates (.COM, .EXE, ...) on every hook + // invocation. string virtualFileSystemPath = "'" + Paths.ConvertPathToGitFormat( - Path.Combine(enlistment.DotGitRoot, GVFSConstants.DotGit.Hooks.RootName, GVFSConstants.DotGit.Hooks.VirtualFileSystemName)) + "'"; + Path.Combine(enlistment.DotGitRoot, GVFSConstants.DotGit.Hooks.RootName, GVFSConstants.DotGit.Hooks.VirtualFileSystemName) + + GVFSPlatform.Instance.Constants.ExecutableExtension) + "'"; string gitStatusCachePath = null; if (!GVFSEnlistment.IsUnattended(tracer: null) && GVFSPlatform.Instance.IsGitStatusCacheSupported()) From e14475c234a78da0d8e83fac332a095107f0a2bf Mon Sep 17 00:00:00 2001 From: Tyrie Vella Date: Tue, 30 Jun 2026 15:12:33 -0700 Subject: [PATCH 02/24] fix: tighten FT slicer grouping regex to class-level MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The greedy bucket slicer keeps consecutive tests from the same fixture class together when they share state. The grouping regex was overly broad, using whole namespaces instead of specific class names: OLD: ^.*\.(?:EnlistmentPerFixture|MultiEnlistmentTests)\..+\. NEW: ^.*\.(?:EnlistmentPerFixture\..+|MultiEnlistmentTests\.ConfigVerbTests)\. Two problems fixed: 1. GitCommands namespace (previous PR run): included all GitCommands tests, causing all 38 CheckoutTests(Full) methods (~16 min) to land in one slice. GitCommands tests are safe to split: they use either a per-test enlistment or a git-checkout reset in [SetUp]. 2. MultiEnlistmentTests namespace (this change): included all classes in the namespace, keeping SharedCacheTests (10 tests, ~7 min) and ServiceVerbTests together unnecessarily. - SharedCacheTests: [SetUp] generates a Guid-based cache path per test — no shared state across test methods. Safe to split. - ServiceVerbTests: [NonParallelizable] blocks within-process concurrency; different slices run on different machines. Safe. - ConfigVerbTests: uses [Order(1..8)] with each test reading config written by the previous. Must stay together — kept explicitly. - EnlistmentPerFixture: still grouped per-class as before. From PR #2028 baseline: critical path was 18.9 min. After GitCommands fix: 9.85 min. Expected after this change: ~6-7 min. Assisted-by: Claude Sonnet 4.6 Signed-off-by: Tyrie Vella --- GVFS/GVFS.Tests/NUnitRunner.cs | 29 +++++++++++++++++++++-------- 1 file changed, 21 insertions(+), 8 deletions(-) diff --git a/GVFS/GVFS.Tests/NUnitRunner.cs b/GVFS/GVFS.Tests/NUnitRunner.cs index 701aac4d4..34d30f83b 100644 --- a/GVFS/GVFS.Tests/NUnitRunner.cs +++ b/GVFS/GVFS.Tests/NUnitRunner.cs @@ -98,16 +98,29 @@ public void PrepareTestSlice(string filters, (uint, uint) testSlice) } // Now distribute the tests into the buckets. - // Tests from the same fixture class must stay in the same bucket - // when the fixture shares a single enlistment across tests (both - // EnlistmentPerFixture classes, MultiEnlistmentTests with [Order], - // and GitCommands fixture classes like GitCommandsTests, CheckoutTests, - // etc. use a shared enlistment). - // The regex captures "everything up to and including the class name" - // so that SomeClass.TestA and SomeClass.TestB share a prefix. + // + // Some test classes share state across test methods and must land in the + // same slice. The regex captures a per-class prefix so that all methods of + // those classes are grouped together. + // + // Classes that MUST stay together: + // EnlistmentPerFixture.* — share a single mounted enlistment for the + // whole fixture; tests may be order-dependent. + // MultiEnlistmentTests.ConfigVerbTests — uses [Order(1..8)]; each test + // reads/writes GVFS config set by the previous. + // + // Classes that are safe to split (do NOT add to this regex): + // GitCommands.* — each test resets state via git-checkout in [SetUp] + // or gets a fresh per-test enlistment. + // SharedCacheTests — [SetUp] generates a new Guid-based cache path per + // test; no shared state across test methods. + // ServiceVerbTests — [NonParallelizable] prevents within-process + // concurrency; different slices run on different + // machines so cross-slice isolation is guaranteed. Regex fixtureRegex = new Regex( - @"^.*\.(?:EnlistmentPerFixture|MultiEnlistmentTests|GitCommands)\..+\.", + @"^.*\.(?:EnlistmentPerFixture\..+|MultiEnlistmentTests\.ConfigVerbTests)\.", RegexOptions.CultureInvariant | RegexOptions.IgnoreCase); + for (uint i = 0; i < list.Length; i++) { var test = list[i].Trim(); From 6ee0ba2a2a2fa7c47781b24e24ed76b1e25de220 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" Date: Fri, 3 Jul 2026 08:19:29 +0000 Subject: [PATCH 03/24] Update default Microsoft Git version to v2.54.0.vfs.0.4 --- .github/workflows/build.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/build.yaml b/.github/workflows/build.yaml index a4ca46cfb..bce5e1c6b 100644 --- a/.github/workflows/build.yaml +++ b/.github/workflows/build.yaml @@ -24,7 +24,7 @@ permissions: checks: read env: - GIT_VERSION: ${{ github.event.inputs.git_version || 'v2.54.0.vfs.0.2' }} + GIT_VERSION: ${{ github.event.inputs.git_version || 'v2.54.0.vfs.0.4' }} jobs: validate: From 3cbc75f978159d7c4320c5e85709424368a51491 Mon Sep 17 00:00:00 2001 From: Tyrie Vella Date: Thu, 2 Jul 2026 20:29:26 -0700 Subject: [PATCH 04/24] Fix clone ownership under Windows Administrator Protection MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Under Windows "Administrator protection" (AP), an elevated process runs as a hidden, profile-separated shadow admin account (MACHINE\admin_) whose SID is a local account (S-1-5-21-...) that differs from the real (non-elevated) user's SID. When `gvfs clone` runs elevated, the enlistment's `src` and `.git` directories are created owned by the Administrators group. EnsureDirectoryIsOwnedByCurrentUser then reassigned ownership to the current user to satisfy git's dubious-ownership check. Under AP the "current user" is the shadow admin, so ownership was set to admin_ — which the real user does not match. The real (non-elevated) user then hit `fatal: detected dubious ownership`, because git's Administrators-group membership grace does not cover another specific user's SID, and the shadow admin cannot SetOwner to the real user's SID either. Detect the AP shadow admin from the running process's own token — its effective elevation identity — and, when present, leave the directory owner as the Administrators group instead of reassigning it to the shadow admin. An Administrators-owned directory is accepted by modern git and the libgit2 non-elevated-admin-owner overlay for any Administrators member — the real user, the shadow admin, and SYSTEM (for automount) alike. Detection inspects the token rather than the TypeOfAdminApprovalMode policy value so it stays correct across an AP policy change that has not yet been rebooted (where the policy value is pending but elevation still yields a shadow admin). A shadow admin is identified as a local account SID (S-1-5-21-...) whose ProfileList profile directory is ADMIN_-prefixed; the real interactive user (a domain or Entra ID account) is excluded. Assisted-by: Claude Opus 4.8 Signed-off-by: Tyrie Vella --- .../WindowsFileSystem.cs | 23 ++++++++++ GVFS/GVFS.Platform.Windows/WindowsPlatform.cs | 44 +++++++++++++++++++ 2 files changed, 67 insertions(+) diff --git a/GVFS/GVFS.Platform.Windows/WindowsFileSystem.cs b/GVFS/GVFS.Platform.Windows/WindowsFileSystem.cs index 7ba732a13..0fe4d781f 100644 --- a/GVFS/GVFS.Platform.Windows/WindowsFileSystem.cs +++ b/GVFS/GVFS.Platform.Windows/WindowsFileSystem.cs @@ -327,6 +327,10 @@ public bool IsFileSystemSupported(string path, out string error) /// Also, even if the fix were in place, automount would still fail because it runs under SYSTEM account. /// /// This method ensures that the directory is owned by the current user (which is verified to work for SYSTEM account for automount). + /// + /// Exception: under Windows "Administrator protection" (AP), an elevated process runs as a profile-separated shadow admin + /// account (MACHINE\admin_<user>) whose SID differs from the real (non-elevated) user's. In that case the directory owner is + /// left as the Administrators group rather than being reassigned to the shadow admin (see the AP note inline). /// public void EnsureDirectoryIsOwnedByCurrentUser(string directoryPath) { @@ -339,6 +343,25 @@ public void EnsureDirectoryIsOwnedByCurrentUser(string directoryPath) SecurityIdentifier administratorsSid = new SecurityIdentifier(WellKnownSidType.BuiltinAdministratorsSid, null); if (directoryOwner == administratorsSid) { + // Under Windows "Administrator protection" (AP), an elevated process runs as a profile-separated shadow + // admin account (MACHINE\admin_) whose SID differs from the real (non-elevated) user's. Reassigning + // ownership to the current user here would set the owner to that shadow admin, causing the real user to hit + // "fatal: detected dubious ownership" — git's Administrators-membership grace does not cover another specific + // user's SID. The shadow admin also cannot SetOwner to the real user's SID. Leaving the owner as the + // Administrators group is accepted by modern git and the libgit2 non-elevated-admin-owner overlay for any + // Administrators member (real user, shadow admin, and SYSTEM for automount alike), so skip the reassignment. + // + // Note: a libgit2 consumer that lacks the non-elevated-admin-owner patch (i.e. stock libgit2 without the + // Administrators-membership fix) will not be able to open an Administrators-owned repo. Addressing that would + // require setting git's safe.directory for the real user, but safe.directory is only honored from global/system + // config (never repo-local), and an elevated clone runs as the shadow admin — so covering the real user would + // mean mutating the real user's global gitconfig or machine-wide system config from the shadow admin. After + // consideration, GVFS is not the right place to do that; unpatched third-party libgit2 consumers are out of scope. + if (WindowsPlatform.IsCurrentUserAdminProtectionShadowAccount()) + { + return; + } + WindowsIdentity currentUser = WindowsIdentity.GetCurrent(); directorySecurity.SetOwner(currentUser.User); directoryInfo.SetAccessControl(directorySecurity); diff --git a/GVFS/GVFS.Platform.Windows/WindowsPlatform.cs b/GVFS/GVFS.Platform.Windows/WindowsPlatform.cs index 37949e6d6..9d26a71ce 100644 --- a/GVFS/GVFS.Platform.Windows/WindowsPlatform.cs +++ b/GVFS/GVFS.Platform.Windows/WindowsPlatform.cs @@ -24,6 +24,10 @@ public partial class WindowsPlatform : GVFSPlatform private const string BuildLabRegistryValue = "BuildLab"; private const string BuildLabExRegistryValue = "BuildLabEx"; + private const string ProfileListRegistryKey = "SOFTWARE\\Microsoft\\Windows NT\\CurrentVersion\\ProfileList"; + private const string ProfileImagePathRegistryValue = "ProfileImagePath"; + private const string AdminProtectionProfilePrefix = "ADMIN_"; + public WindowsPlatform() : base(underConstruction: new UnderConstructionFlags()) { } @@ -76,6 +80,46 @@ public static object GetValueFromRegistry(RegistryHive registryHive, string key, return value; } + /// + /// Detects whether the current process token belongs to a Windows "Administrator protection" (AP) + /// shadow admin account. + /// + /// Under AP, elevating produces a token whose user is a hidden, system-managed local account + /// (MACHINE\admin_<user>, SID S-1-5-21-...) with its own user profile named ADMIN_<user>. That + /// account's SID differs from the real (interactive) user's SID, so treating it as "the current user" + /// for file-ownership purposes is wrong. + /// + /// This inspects the running process's own token (its effective elevation identity) rather than the + /// TypeOfAdminApprovalMode policy value, so it is correct even across an AP policy change that has not + /// yet been rebooted (where the policy value is pending but elevation still yields a shadow admin). + /// + public static bool IsCurrentUserAdminProtectionShadowAccount() + { + using (WindowsIdentity currentUser = WindowsIdentity.GetCurrent()) + { + SecurityIdentifier userSid = currentUser.User; + + // AP shadow accounts are local machine accounts (S-1-5-21-...); the real interactive user is a + // domain or Entra ID account (e.g. S-1-12-1-...), which is not an "account SID" in this sense. + if (userSid == null || !userSid.IsAccountSid()) + { + return false; + } + + string profilePath = GetStringFromRegistry( + $"{ProfileListRegistryKey}\\{userSid.Value}", + ProfileImagePathRegistryValue); + if (string.IsNullOrEmpty(profilePath)) + { + return false; + } + + string profileFolderName = Path.GetFileName( + profilePath.TrimEnd(Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar)); + return profileFolderName.StartsWith(AdminProtectionProfilePrefix, StringComparison.OrdinalIgnoreCase); + } + } + public static bool TrySetDWordInRegistry(RegistryHive registryHive, string key, string valueName, uint value) { RegistryKey localKey = RegistryKey.OpenBaseKey(registryHive, RegistryView.Registry64); From 776aea6f4bc3cdbe0a58af0154789b57674c6654 Mon Sep 17 00:00:00 2001 From: Tyrie Vella Date: Mon, 6 Jul 2026 14:34:54 -0700 Subject: [PATCH 05/24] Re-sync libgit2 overlay admin-owner patch with merged upstream MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `non-elevated-admin-owner.diff` overlay patch was copied verbatim from libgit2 PR #7200. That PR version had four bugs that maintainers fixed before merging to libgit2 `main`: - `linked_token` HANDLE from TokenLinkedToken was never CloseHandle'd, leaking a handle in the long-lived GVFS mount service (SYSTEM). - `current_user_sid` used `static HANDLE linked_token` — not thread-safe and retained a stale handle across calls; replaced with a per-call local initialized to NULL by the caller. - The TokenLinkedToken failure path nulled the local pointer parameter (`linked_token = NULL`) instead of `*linked_token = NULL` (+CloseHandle). - `current_user_sid()` was only invoked in the CURRENT_USER branch, but the admin-membership branch also reads `linked_token`; a caller passing only USER_IS_ADMINISTRATOR would read an uninitialized handle. It is now called unconditionally before either branch. Regenerate the patch so that, applied to the pinned libgit2 source, it produces the same corrected `src/util/fs_path.c` as upstream `main` (commits cc477ee + f9f36a6 + 44c05e5, merge e805a16, merged 2026-06-07). The patch is not yet in any libgit2 release, so the overlay remains necessary. Also bump the pinned libgit2 version 1.9.3 -> 1.9.4 (matching the official vcpkg port). fs_path.c is byte-identical between 1.9.3 and 1.9.4, so the patched region is unaffected and the diff applies cleanly to both. Verified: Build.bat Debug rebuilds libgit2 for both x64-windows-static-aot and x64-windows-dynamic triplets, applying non-elevated-admin-owner.diff with no hunk failures, and the full solution builds to an installer. Complements microsoft/VFSForGit#2039 (the GVFS-side AP clone-ownership fix). Work item: AB#62918022. Assisted-by: Claude Opus 4.8 Signed-off-by: Tyrie Vella --- overlays/libgit2/README.md | 12 +- .../libgit2/non-elevated-admin-owner.diff | 161 ++++++++++-------- overlays/libgit2/portfile.cmake | 2 +- overlays/libgit2/vcpkg.json | 2 +- 4 files changed, 102 insertions(+), 75 deletions(-) diff --git a/overlays/libgit2/README.md b/overlays/libgit2/README.md index 716eb81fb..255ec41d6 100644 --- a/overlays/libgit2/README.md +++ b/overlays/libgit2/README.md @@ -5,8 +5,8 @@ version and optionally carry patches ahead of upstream releases. ## Current version -**libgit2 v1.9.3** — includes C4703 warning fixes (libgit2/libgit2#7154) -that were missing from v1.9.1. +**libgit2 v1.9.4** — latest v1.9.x release (2026-05-22). Matches the version +pinned by the official vcpkg port. v1.9.x is the last of the libgit2 1.x line. ## Patches @@ -17,6 +17,12 @@ that were missing from v1.9.1. Allows non-elevated processes run by Administrators group members to be considered the owner of repos owned by that group. Related to [libgit2/libgit2#6279](https://github.com/libgit2/libgit2/issues/6279). + This overlay carries the version that was **merged to libgit2 `main`** + (2026-06-07), which fixes handle-leak, static-handle, and + uninitialized-token bugs present in the original PR #7200 diff. See the + provenance table below. The patch is **not yet in any libgit2 release** + (latest is v1.9.4; expected to ship upstream in v2.0, no ETA), so it + remains necessary until the pinned port moves to a libgit2 that includes it. Additional patches can be added to the `PATCHES` list in `portfile.cmake` to apply fixes that haven't shipped in an official libgit2 release yet. @@ -32,7 +38,7 @@ and then modified as noted below. | `vcpkg.json` | Official vcpkg port | Unchanged | | `dependencies.diff` | Official vcpkg port | Unchanged | | `portfile.cmake` | Official vcpkg port | Removed patches not needed for MSVC x64: `c-standard.diff` (C99 inline keyword — MSVC handles natively), `cli-include-dirs.diff` (CLI tool build — we set `BUILD_CLI=OFF`), `mingw-winhttp.diff` (MinGW only) | -| `non-elevated-admin-owner.diff` | [libgit2/libgit2#7200](https://github.com/libgit2/libgit2/pull/7200) | PR diff, verbatim | +| `non-elevated-admin-owner.diff` | libgit2 `main` (commits `cc477ee` + `f9f36a6` + `44c05e5`, merge `e805a16`) | Matches the version merged to `main` on 2026-06-07, not the original [PR #7200](https://github.com/libgit2/libgit2/pull/7200) diff. Maintainer follow-ups fixed a `linked_token` handle leak, replaced a non-thread-safe `static HANDLE` with a per-call local, corrected the `*linked_token = NULL` failure path (with `CloseHandle`), and hoisted `current_user_sid()` so `linked_token` is always populated before both branches use it. | | `README.md` | New | VFSForGit-specific documentation | When updating to a new libgit2 version, compare these files against the diff --git a/overlays/libgit2/non-elevated-admin-owner.diff b/overlays/libgit2/non-elevated-admin-owner.diff index f8e016ea1..223e2a5dd 100644 --- a/overlays/libgit2/non-elevated-admin-owner.diff +++ b/overlays/libgit2/non-elevated-admin-owner.diff @@ -1,70 +1,91 @@ -diff --git a/src/util/fs_path.c b/src/util/fs_path.c -index ff0836ff874..5be2da35b34 100644 ---- a/src/util/fs_path.c -+++ b/src/util/fs_path.c -@@ -1853,12 +1853,16 @@ static PSID *sid_dup(PSID sid) - return dup; - } - --static int current_user_sid(PSID *out) -+static int current_user_sid(PSID *sid, HANDLE *linked_token) - { - TOKEN_USER *info = NULL; - HANDLE token = NULL; - DWORD len = 0; - int error = -1; -+ TOKEN_ELEVATION_TYPE elevation_type; -+ DWORD size; -+ -+ *linked_token = NULL; - - if (!OpenProcessToken(GetCurrentProcess(), TOKEN_QUERY, &token)) { - git_error_set(GIT_ERROR_OS, "could not lookup process information"); -@@ -1879,9 +1883,19 @@ static int current_user_sid(PSID *out) - goto done; - } - -- if ((*out = sid_dup(info->User.Sid))) -+ if ((*sid = sid_dup(info->User.Sid))) - error = 0; - -+ if (GetTokenInformation(token, TokenElevationType, &elevation_type, sizeof(elevation_type), &size) && -+ elevation_type == TokenElevationTypeLimited) { -+ /* -+ * The current process is run by a member of the Administrators group -+ * but is not running elevated. -+ */ -+ if (!GetTokenInformation(token, TokenLinkedToken, linked_token, sizeof(HANDLE), &size)) { -+ linked_token = NULL; -+ } -+ } - done: - if (token) - CloseHandle(token); -@@ -1926,6 +1940,7 @@ int git_fs_path_owner_is( - git_fs_path_owner_t owner_type) - { - PSID owner_sid = NULL, user_sid = NULL; -+ static HANDLE linked_token; - BOOL is_admin, admin_owned; - int error; - -@@ -1938,7 +1953,7 @@ int git_fs_path_owner_is( - goto done; - - if ((owner_type & GIT_FS_PATH_OWNER_CURRENT_USER) != 0) { -- if ((error = current_user_sid(&user_sid)) < 0) -+ if ((error = current_user_sid(&user_sid, &linked_token)) < 0) - goto done; - - if (EqualSid(owner_sid, user_sid)) { -@@ -1959,7 +1974,8 @@ int git_fs_path_owner_is( - - if (admin_owned && - (owner_type & GIT_FS_PATH_USER_IS_ADMINISTRATOR) != 0 && -- CheckTokenMembership(NULL, owner_sid, &is_admin) && -+ (CheckTokenMembership(NULL, owner_sid, &is_admin) && -+ CheckTokenMembership(linked_token, owner_sid, &is_admin)) && - is_admin) { - *out = true; - goto done; +diff --git a/src/util/fs_path.c b/src/util/fs_path.c +index 5b00b1b..976a9f0 100644 +--- a/src/util/fs_path.c ++++ b/src/util/fs_path.c +@@ -1853,12 +1853,14 @@ static PSID *sid_dup(PSID sid) + return dup; + } + +-static int current_user_sid(PSID *out) ++static int current_user_sid(PSID *sid, HANDLE *linked_token) + { + TOKEN_USER *info = NULL; + HANDLE token = NULL; + DWORD len = 0; + int error = -1; ++ TOKEN_ELEVATION_TYPE elevation_type; ++ DWORD size; + + if (!OpenProcessToken(GetCurrentProcess(), TOKEN_QUERY, &token)) { + git_error_set(GIT_ERROR_OS, "could not lookup process information"); +@@ -1879,7 +1881,19 @@ static int current_user_sid(PSID *out) + goto done; + } + +- if ((*out = sid_dup(info->User.Sid))) ++ if (GetTokenInformation(token, TokenElevationType, &elevation_type, sizeof(elevation_type), &size) && ++ elevation_type == TokenElevationTypeLimited) { ++ /* ++ * The current process is run by a member of the Administrators group ++ * but is not running elevated. ++ */ ++ if (!GetTokenInformation(token, TokenLinkedToken, linked_token, sizeof(HANDLE), &size)) { ++ CloseHandle(*linked_token); ++ *linked_token = NULL; ++ } ++ } ++ ++ if ((*sid = sid_dup(info->User.Sid))) + error = 0; + + done: +@@ -1926,6 +1940,7 @@ int git_fs_path_owner_is( + git_fs_path_owner_t owner_type) + { + PSID owner_sid = NULL, user_sid = NULL; ++ HANDLE linked_token = NULL; + BOOL is_admin, admin_owned; + int error; + +@@ -1934,17 +1949,14 @@ int git_fs_path_owner_is( + return 0; + } + +- if ((error = file_owner_sid(&owner_sid, path)) < 0) ++ if ((error = file_owner_sid(&owner_sid, path)) < 0 || ++ (error = current_user_sid(&user_sid, &linked_token)) < 0) + goto done; + +- if ((owner_type & GIT_FS_PATH_OWNER_CURRENT_USER) != 0) { +- if ((error = current_user_sid(&user_sid)) < 0) +- goto done; +- +- if (EqualSid(owner_sid, user_sid)) { +- *out = true; +- goto done; +- } ++ if ((owner_type & GIT_FS_PATH_OWNER_CURRENT_USER) != 0 && ++ EqualSid(owner_sid, user_sid)) { ++ *out = true; ++ goto done; + } + + admin_owned = +@@ -1960,6 +1972,7 @@ int git_fs_path_owner_is( + if (admin_owned && + (owner_type & GIT_FS_PATH_USER_IS_ADMINISTRATOR) != 0 && + CheckTokenMembership(NULL, owner_sid, &is_admin) && ++ CheckTokenMembership(linked_token, owner_sid, &is_admin) && + is_admin) { + *out = true; + goto done; +@@ -1968,6 +1981,9 @@ int git_fs_path_owner_is( + *out = false; + + done: ++ if (linked_token) ++ CloseHandle(linked_token); ++ + git__free(owner_sid); + git__free(user_sid); + return error; diff --git a/overlays/libgit2/portfile.cmake b/overlays/libgit2/portfile.cmake index ddf650e17..bc47e5f91 100644 --- a/overlays/libgit2/portfile.cmake +++ b/overlays/libgit2/portfile.cmake @@ -2,7 +2,7 @@ vcpkg_from_github( OUT_SOURCE_PATH SOURCE_PATH REPO libgit2/libgit2 REF "v${VERSION}" - SHA512 5359d07b85b691b92784d5ef52825be2c1f1616bab8e969e51177bf8f5e767edbaf267943296407f62df1f3d7fa08e48c802374289046b71da2cc1ac7d43a15f + SHA512 85036ade3aca33c5283605ae9de21a7948ec5952bc8cd468aa024ca873851a56ab0ebe62f95c0da109df9163875e9687a377d3df69728d434cc258b3f845ef0c HEAD_REF main PATCHES dependencies.diff diff --git a/overlays/libgit2/vcpkg.json b/overlays/libgit2/vcpkg.json index 700a09255..4d3a5d3b2 100644 --- a/overlays/libgit2/vcpkg.json +++ b/overlays/libgit2/vcpkg.json @@ -1,6 +1,6 @@ { "name": "libgit2", - "version-semver": "1.9.3", + "version-semver": "1.9.4", "description": "A C library implementing the Git core methods with a solid API", "homepage": "https://github.com/libgit2/libgit2", "license": null, From d9d0a1420f847da5bb32667d64a13f1b95e6a17f Mon Sep 17 00:00:00 2001 From: Tyrie Vella Date: Thu, 9 Jul 2026 11:03:39 -0700 Subject: [PATCH 06/24] Bound git stdout/stderr capture to prevent GVFS.Mount OOM GVFS.Mount could crash with OutOfMemoryException. GitProcess.InvokeGitImpl buffered all of a git process's stdout and stderr into unbounded StringBuilders via the DataReceived handlers. A corrupt or truncated packfile in the shared object cache makes 'git multi-pack-index' stream a large volume of "could not load pack" errors to stderr, growing the buffer until the mount OOM-crashes. Bound each captured stream with its own cap: - stderr: ~20 MB (10M chars). stderr is diagnostic, so truncating it is safe; this is the cap that stops the corrupt-packfile crash. - stdout: ~256 MB (128M chars) when the caller buffers the whole result. Sized to hold the machine-readable (-z) output of the largest real repositories - on the order of 2.8M status/diff records on the biggest os.2020 branches - with headroom, while staying far below the .NET maximum array/string size so the buffer itself cannot OOM. Commands that stream their output line-by-line are unaffected. Truncation is surfaced on GitProcess.Result via OutputTruncated and ErrorsTruncated so callers can react: - stdout carries correctness-critical data for two commands, which now fail safe on truncation rather than act on a partial result: - AddStagedFilesToModifiedPaths (git diff --cached --name-status -z): refuses to update ModifiedPaths from a partial staged-file list, which would otherwise leave staged files with skip-worktree set and stale placeholders. - SparseVerb's git status check (status --porcelain -z): aborts sparse rather than risk proceeding over uncommitted changes it failed to see. Both emit telemetry on truncation. - stderr truncation is expected and non-fatal; GitMaintenanceStep.RunGitCommand logs it for diagnostics but does not fail the command for it. This is the first of a series: a follow-up converts those two -z commands to stream their output (removing the truncation exposure entirely), and packfile corruption auto-recovery is a separate change. Assisted-by: Claude Opus 4.8 Signed-off-by: Tyrie Vella --- GVFS/GVFS.Common/Git/GitProcess.cs | 120 +++++++++++++++++- .../Maintenance/GitMaintenanceStep.cs | 11 ++ GVFS/GVFS.UnitTests/Git/GitProcessTests.cs | 82 ++++++++++++ .../FileSystemCallbacks.cs | 14 ++ GVFS/GVFS/CommandLine/SparseVerb.cs | 15 +++ 5 files changed, 236 insertions(+), 6 deletions(-) diff --git a/GVFS/GVFS.Common/Git/GitProcess.cs b/GVFS/GVFS.Common/Git/GitProcess.cs index cf666bc64..03cc27b41 100644 --- a/GVFS/GVFS.Common/Git/GitProcess.cs +++ b/GVFS/GVFS.Common/Git/GitProcess.cs @@ -15,6 +15,28 @@ public class GitProcess : ICredentialStore { private const int HResultEHANDLE = -2147024890; // 0x80070006 E_HANDLE + /// + /// Upper bound on the number of characters buffered from a git process's stderr. Bounding + /// stderr prevents a pathologically noisy git command (e.g. 'multi-pack-index write' repeatedly + /// reporting "could not load pack N" against a corrupt packfile) from growing the capture buffer + /// until GVFS.Mount hits an OutOfMemoryException. stderr is a diagnostic stream, so truncating it + /// is safe: callers can observe it via and should log it, + /// but must not fail solely because of it. + /// + private const int MaxCapturedStdErrChars = 10 * 1024 * 1024; // ~20 MB of UTF-16 + + /// + /// Upper bound on the number of characters buffered from a git process's stdout when the caller + /// buffers the whole result (i.e. does not stream it line-by-line). Sized to hold the + /// machine-readable (-z) output of the largest real repositories - on the order of 2.8M + /// status/diff records on the biggest os.2020 branches - with headroom, while staying far below + /// the .NET maximum array/string size so the buffer itself cannot OOM. stdout can carry + /// correctness-critical data (e.g. the full staged-file list), so truncation here is surfaced via + /// and those callers fail safe rather than act on a partial + /// result. + /// + private const int MaxCapturedStdOutChars = 128 * 1024 * 1024; // ~256 MB of UTF-16 + private static readonly Encoding UTF8NoBOM = new UTF8Encoding(false); private static bool failedToSetEncoding = false; private static string expireTimeDateString; @@ -967,14 +989,19 @@ protected virtual Result InvokeGitImpl( // Do not perform a synchronous read to the end of both redirected streams. using (this.executingProcess = this.GetGitProcess(command, workingDirectory, dotGitDirectory, useReadObjectHook, gitObjectsDirectory: gitObjectsDirectory, usePreCommandHook: usePreCommandHook)) { - StringBuilder output = new StringBuilder(); - StringBuilder errors = new StringBuilder(); + // Bound how much stdout/stderr we buffer so a pathologically noisy git command + // cannot grow these buffers without limit until GVFS.Mount hits an + // OutOfMemoryException. stderr gets a tight cap (it is diagnostic); stdout gets a + // large cap sized for the biggest real repositories. Truncation is surfaced on the + // Result so callers of correctness-critical commands can fail safe. + BoundedGitOutputBuffer output = new BoundedGitOutputBuffer(MaxCapturedStdOutChars); + BoundedGitOutputBuffer errors = new BoundedGitOutputBuffer(MaxCapturedStdErrChars); this.executingProcess.ErrorDataReceived += (sender, args) => { if (args.Data != null) { - errors.Append(args.Data + "\n"); + errors.AppendLine(args.Data); } }; this.executingProcess.OutputDataReceived += (sender, args) => @@ -987,7 +1014,7 @@ protected virtual Result InvokeGitImpl( } else { - output.Append(args.Data + "\n"); + output.AppendLine(args.Data); } } }; @@ -1026,11 +1053,11 @@ protected virtual Result InvokeGitImpl( { this.executingProcess.Kill(); - return new Result(output.ToString(), "Operation timed out: " + errors.ToString(), Result.GenericFailureCode); + return new Result(output.ToString(), "Operation timed out: " + errors.ToString(), Result.GenericFailureCode, output.Truncated, errors.Truncated); } } - return new Result(output.ToString(), errors.ToString(), this.executingProcess.ExitCode); + return new Result(output.ToString(), errors.ToString(), this.executingProcess.ExitCode, output.Truncated, errors.Truncated); } } catch (Win32Exception e) @@ -1151,16 +1178,38 @@ public class Result public const int GenericFailureCode = 1; public Result(string stdout, string stderr, int exitCode) + : this(stdout, stderr, exitCode, outputTruncated: false, errorsTruncated: false) + { + } + + public Result(string stdout, string stderr, int exitCode, bool outputTruncated, bool errorsTruncated) { this.Output = stdout; this.Errors = stderr; this.ExitCode = exitCode; + this.OutputTruncated = outputTruncated; + this.ErrorsTruncated = errorsTruncated; } public string Output { get; } public string Errors { get; } public int ExitCode { get; } + /// + /// True if the git process produced more stdout than the capture buffer allowed, so + /// is incomplete. Callers whose correctness depends on the full output + /// (e.g. the complete list of staged files) must treat this as a failure rather than acting + /// on a partial result. + /// + public bool OutputTruncated { get; } + + /// + /// True if the git process produced more stderr than the capture buffer allowed, so + /// is incomplete. This is expected for pathologically noisy commands and + /// is safe: callers should log it for diagnostics but should not fail solely because of it. + /// + public bool ErrorsTruncated { get; } + public bool ExitCodeIsSuccess { get { return this.ExitCode == Result.SuccessCode; } @@ -1184,6 +1233,65 @@ public bool StderrContainsErrors() } } + /// + /// Accumulates lines from a git process's redirected stdout or stderr while enforcing a hard + /// upper bound on the number of characters buffered. Once the cap is reached, further input is + /// dropped and a single truncation marker is appended. This prevents a noisy git command from + /// growing an unbounded StringBuilder until GVFS.Mount hits an OutOfMemoryException. Whether + /// truncation occurred is exposed via . + /// + public class BoundedGitOutputBuffer + { + private readonly StringBuilder builder = new StringBuilder(); + private readonly int maxChars; + private bool truncated; + + public BoundedGitOutputBuffer(int maxChars) + { + this.maxChars = maxChars; + } + + /// + /// True once output has exceeded the cap and been truncated. + /// + public bool Truncated => this.truncated; + + /// + /// Appends a single line (followed by a newline) unless doing so would exceed the cap. The + /// first time the cap is reached, as much of the line as fits is kept and a truncation marker + /// is appended; all subsequent lines are dropped. + /// + public void AppendLine(string line) + { + if (this.truncated) + { + return; + } + + // +1 accounts for the trailing newline we append after each line. + if (this.builder.Length + line.Length + 1 > this.maxChars) + { + int remaining = this.maxChars - this.builder.Length; + if (remaining > 0) + { + this.builder.Append(line, 0, Math.Min(remaining, line.Length)); + } + + this.builder.Append("\n[... GVFS truncated git output after " + this.maxChars + " characters to avoid OutOfMemoryException ...]\n"); + this.truncated = true; + return; + } + + this.builder.Append(line); + this.builder.Append('\n'); + } + + public override string ToString() + { + return this.builder.ToString(); + } + } + public class ConfigResult { private readonly Result result; diff --git a/GVFS/GVFS.Common/Maintenance/GitMaintenanceStep.cs b/GVFS/GVFS.Common/Maintenance/GitMaintenanceStep.cs index 1df648fc4..a39cf33bf 100644 --- a/GVFS/GVFS.Common/Maintenance/GitMaintenanceStep.cs +++ b/GVFS/GVFS.Common/Maintenance/GitMaintenanceStep.cs @@ -193,6 +193,17 @@ protected GitProcess.Result RunGitCommand(Func wo throw new StoppingException(); } + if (result?.ErrorsTruncated == true) + { + // stderr was too large to buffer fully and was truncated to avoid an + // OutOfMemoryException (e.g. a corrupt packfile producing a flood of errors). This is + // safe and non-fatal - record it for diagnostics but do not fail the command for it. + this.Context.Tracer.RelatedWarning( + metadata: null, + message: $"{this.Area}: Git process {gitCommand} stderr was truncated to bound memory use", + keywords: Keywords.Telemetry); + } + if (result?.ExitCodeIsFailure == true) { string errorMessage = result?.Errors == null ? string.Empty : result.Errors; diff --git a/GVFS/GVFS.UnitTests/Git/GitProcessTests.cs b/GVFS/GVFS.UnitTests/Git/GitProcessTests.cs index 74ddde8ee..8182f8dfb 100644 --- a/GVFS/GVFS.UnitTests/Git/GitProcessTests.cs +++ b/GVFS/GVFS.UnitTests/Git/GitProcessTests.cs @@ -9,6 +9,88 @@ namespace GVFS.UnitTests.Git [TestFixture] public class GitProcessTests { + [TestCase] + public void BoundedGitOutputBuffer_KeepsShortOutput() + { + GitProcess.BoundedGitOutputBuffer buffer = new GitProcess.BoundedGitOutputBuffer(1000); + buffer.AppendLine("line one"); + buffer.AppendLine("line two"); + + buffer.Truncated.ShouldBeFalse(); + buffer.ToString().ShouldEqual("line one\nline two\n"); + } + + [TestCase] + public void BoundedGitOutputBuffer_TruncatesBeyondCapWithoutUnboundedGrowth() + { + // Simulate a flood of output (e.g. 'multi-pack-index write' spewing "could not load pack" + // against a corrupt packfile). Before the cap this grew an unbounded StringBuilder until + // GVFS.Mount hit OutOfMemoryException. + const int Cap = 4096; + GitProcess.BoundedGitOutputBuffer buffer = new GitProcess.BoundedGitOutputBuffer(Cap); + + const string NoisyLine = "error: could not load pack 2"; + for (int i = 0; i < 1_000_000; i++) + { + buffer.AppendLine(NoisyLine); + } + + buffer.Truncated.ShouldBeTrue(); + + string result = buffer.ToString(); + + // We appended ~28 million characters; the buffer must stay bounded near the cap + // (cap + a single one-time truncation marker), nowhere near the raw input size. + (result.Length < Cap + 200).ShouldBeTrue("Buffer grew beyond its cap: " + result.Length); + result.Contains("truncated").ShouldBeTrue(); + } + + [TestCase] + public void BoundedGitOutputBuffer_KeepsPartialLineThatCrossesCap() + { + GitProcess.BoundedGitOutputBuffer buffer = new GitProcess.BoundedGitOutputBuffer(10); + buffer.AppendLine("123456789012345"); + + buffer.Truncated.ShouldBeTrue(); + string result = buffer.ToString(); + + // The portion of the line that fit under the cap is retained, then the marker. + result.StartsWith("1234567890").ShouldBeTrue(); + result.Contains("truncated").ShouldBeTrue(); + } + + [TestCase] + public void BoundedGitOutputBuffer_ExactlyAtCapIsNotTruncated() + { + // "abcd" + "\n" == 5 chars == cap; must fit without tripping truncation. + GitProcess.BoundedGitOutputBuffer buffer = new GitProcess.BoundedGitOutputBuffer(5); + buffer.AppendLine("abcd"); + + buffer.Truncated.ShouldBeFalse(); + buffer.ToString().ShouldEqual("abcd\n"); + } + + [TestCase] + public void Result_TruncationFlagsDefaultToFalse() + { + GitProcess.Result result = new GitProcess.Result("out", "err", 0); + + result.OutputTruncated.ShouldBeFalse(); + result.ErrorsTruncated.ShouldBeFalse(); + } + + [TestCase] + public void Result_TruncationFlagsRoundTripThroughConstructor() + { + GitProcess.Result outputTruncated = new GitProcess.Result("out", "err", 0, outputTruncated: true, errorsTruncated: false); + outputTruncated.OutputTruncated.ShouldBeTrue(); + outputTruncated.ErrorsTruncated.ShouldBeFalse(); + + GitProcess.Result errorsTruncated = new GitProcess.Result("out", "err", 0, outputTruncated: false, errorsTruncated: true); + errorsTruncated.OutputTruncated.ShouldBeFalse(); + errorsTruncated.ErrorsTruncated.ShouldBeTrue(); + } + [TestCase] public void TryKillRunningProcess_NeverRan() { diff --git a/GVFS/GVFS.Virtualization/FileSystemCallbacks.cs b/GVFS/GVFS.Virtualization/FileSystemCallbacks.cs index ab2ff36d7..f2a45623c 100644 --- a/GVFS/GVFS.Virtualization/FileSystemCallbacks.cs +++ b/GVFS/GVFS.Virtualization/FileSystemCallbacks.cs @@ -436,6 +436,20 @@ public bool AddStagedFilesToModifiedPaths(string messageBody, out int addedCount // Query all staged files in one call using --name-status -z. // Output format: "A\0path1\0M\0path2\0D\0path3\0" GitProcess.Result result = gitProcess.DiffCachedNameStatus(pathspecs, pathspecFromFile, pathspecFileNul); + + if (result.OutputTruncated) + { + // The staged-file list exceeded the capture buffer. Acting on a partial list would leave + // some staged files out of ModifiedPaths (skip-worktree not cleared, stale placeholders), + // which is worse than failing. Fail safe and let the caller retry. + EventMetadata metadata = new EventMetadata(); + metadata.Add("ExitCode", result.ExitCode); + this.context.Tracer.RelatedError( + metadata, + nameof(this.AddStagedFilesToModifiedPaths) + ": git diff --cached output was truncated; refusing to update ModifiedPaths from a partial staged-file list"); + return false; + } + if (result.ExitCodeIsSuccess && !string.IsNullOrEmpty(result.Output)) { string[] parts = result.Output.Split(new[] { '\0' }, StringSplitOptions.RemoveEmptyEntries); diff --git a/GVFS/GVFS/CommandLine/SparseVerb.cs b/GVFS/GVFS/CommandLine/SparseVerb.cs index 1ea6e5aed..334e4d1ca 100644 --- a/GVFS/GVFS/CommandLine/SparseVerb.cs +++ b/GVFS/GVFS/CommandLine/SparseVerb.cs @@ -639,6 +639,17 @@ private void CheckGitStatus(ITracer tracer, GVFSEnlistment enlistment, HashSet Date: Thu, 9 Jul 2026 12:03:15 -0700 Subject: [PATCH 07/24] Narrow IsTransientError to BUSY+LOCKED only; exclude IOERR SQLITE_IOERR is excluded from the retry-eligible set because it can fire during connection disposal *after* a write has already committed to the WAL. In ExecuteReadThenWrite (used by RemoveAllEntriesForFolder), this produces a stale empty result on retry: the SELECT finds zero rows (DELETE already committed), so the returned list of removed placeholders is wrong. If the dehydrate operation subsequently fails, the rollback loop re-adds nothing and placeholder entries are silently lost. BUSY and LOCKED are safe to retry: both mean the operation was blocked before executing and was never committed. SQLite guarantees this. IOERR on a genuinely failed write is also rolled back atomically by SQLite, but we cannot distinguish a post-commit IOERR (from Dispose) from a pre-commit IOERR (from ExecuteNonQuery) at the catch site. The safe choice is to surface IOERR immediately and let the caller decide. The root cause of the production LOCKED errors (#2031) was Cache=Shared, which is already removed. IOERR tolerance was speculative; removing it eliminates a real (if rare) correctness hazard with no practical loss. Assisted-by: Claude Sonnet 4.6 Signed-off-by: Tyrie Vella --- GVFS/GVFS.Common/Database/SqliteErrorCodes.cs | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/GVFS/GVFS.Common/Database/SqliteErrorCodes.cs b/GVFS/GVFS.Common/Database/SqliteErrorCodes.cs index 703074a91..9efd7e940 100644 --- a/GVFS/GVFS.Common/Database/SqliteErrorCodes.cs +++ b/GVFS/GVFS.Common/Database/SqliteErrorCodes.cs @@ -22,13 +22,21 @@ public static class SqliteErrorCodes public const int NotADatabase = 26; /// - /// Returns true if the error code represents a transient condition - /// that may resolve on retry (I/O errors, locking contention). + /// Returns true if the error code represents a transient locking condition + /// that is safe to retry. Only BUSY and LOCKED qualify: both mean the operation + /// was blocked before executing and was never committed, so retrying is idempotent. + /// + /// SQLITE_IOERR is intentionally excluded. An I/O error can fire during connection + /// disposal AFTER a write has already committed (e.g. WAL flush on close). Retrying + /// in that case would re-execute the read phase on an already-empty table and return + /// stale results — a risk that exists in ExecuteReadThenWrite (RemoveAllEntriesForFolder). + /// IOERR on a genuinely failed write will have been rolled back by SQLite atomically, + /// but we cannot distinguish that case at the catch site without inspecting whether + /// the write had already committed. The safe choice is to surface IOERR immediately. /// public static bool IsTransientError(int sqliteErrorCode) { - return sqliteErrorCode == DiskIOError - || sqliteErrorCode == Busy + return sqliteErrorCode == Busy || sqliteErrorCode == Locked; } } From c01248deda83560ad2ade3e8bbce0fa68f722e0d Mon Sep 17 00:00:00 2001 From: Tyrie Vella Date: Thu, 9 Jul 2026 12:05:31 -0700 Subject: [PATCH 08/24] ProjFS: only tolerate FilterAttach ACCESS_DENIED for unelevated callers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR #1980 made TryAttachToVolume tolerate ACCESS_DENIED from FilterAttach whenever the ProjFS service is running. Because both callers share this method, the elevated GVFS.Service (running as LocalSystem) also began swallowing ACCESS_DENIED — but the service holds SE_LOAD_DRIVER_PRIVILEGE, so ACCESS_DENIED there is never the benign "unprivileged caller" case. It signals a real problem (AV/EDR lock, Group Policy, or driver corruption), and tolerating it lets a broken mount proceed instead of surfacing an actionable error. Before #1980 the service called raw TryAttach and reported that failure. FilterAttach's ACCESS_DENIED is a privilege gate, not a transient contention error: an unelevated caller always gets it (regardless of the actual attach state), while an elevated caller only gets it when something is genuinely wrong. Narrow the tolerance to unelevated callers so the unelevated `gvfs mount` fix is preserved while the elevated service once again surfaces genuine attach failures. This also aligns the code with the existing comment's stated intent ("when the caller lacks this privilege"). Assisted-by: Claude Opus 4.8 Signed-off-by: Tyrie Vella --- GVFS/GVFS.Platform.Windows/ProjFSFilter.cs | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/GVFS/GVFS.Platform.Windows/ProjFSFilter.cs b/GVFS/GVFS.Platform.Windows/ProjFSFilter.cs index ec092e984..3d4996818 100644 --- a/GVFS/GVFS.Platform.Windows/ProjFSFilter.cs +++ b/GVFS/GVFS.Platform.Windows/ProjFSFilter.cs @@ -87,7 +87,7 @@ public static bool TryAttach(string enlistmentRoot, out string errorMessage) /// /// Attempts to attach the ProjFS filter driver to the volume containing the enlistment. - /// If FilterAttach returns ACCESS_DENIED but the ProjFS service is already running, + /// If an unelevated caller gets ACCESS_DENIED but the ProjFS service is already running, /// the filter is presumed to already be attached and the call succeeds. /// public static bool TryAttachToVolume(string enlistmentRoot, ITracer tracer, out string errorMessage) @@ -98,14 +98,22 @@ public static bool TryAttachToVolume(string enlistmentRoot, ITracer tracer, out } // FilterAttach requires SE_LOAD_DRIVER_PRIVILEGE, which is typically only - // granted to administrators. When the caller lacks this privilege but the - // ProjFS service is confirmed running, the filter is probably already - // attached to the volume — treat ACCESS_DENIED as success in that case. + // granted to administrators. An UNELEVATED caller therefore always gets + // ACCESS_DENIED regardless of whether the filter is actually attached, so + // when the ProjFS service is confirmed running we presume the (already + // elevated) service attached the filter and treat this as success. + // + // We intentionally do NOT tolerate ACCESS_DENIED for an ELEVATED caller + // (e.g. GVFS.Service, which runs as LocalSystem): an elevated caller holds + // the privilege, so ACCESS_DENIED there is a genuine failure (AV/EDR lock, + // Group Policy, or driver corruption) that must surface rather than be + // silently swallowed and leave the mount in a broken state. if (errorMessage != null && errorMessage.Contains(AccessDeniedResult.ToString()) + && !WindowsPlatform.IsElevatedImplementation() && IsServiceRunning(tracer)) { - tracer.RelatedInfo($"{nameof(TryAttachToVolume)}: FilterAttach returned ACCESS_DENIED, but ProjFS service is running. Proceeding."); + tracer.RelatedInfo($"{nameof(TryAttachToVolume)}: FilterAttach returned ACCESS_DENIED to an unelevated caller, but ProjFS service is running. Proceeding."); errorMessage = null; return true; } From e1dc57af39d323fc26686faea8583b50d46d520e Mon Sep 17 00:00:00 2001 From: Tyrie Vella Date: Thu, 9 Jul 2026 12:04:41 -0700 Subject: [PATCH 09/24] Mount: rethrow after logging in HandleRequest exception catch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Change the outer try-catch in HandleRequest from swallowing exceptions to catch-log-rethrow. The original catch (PR #2021) suppressed all exceptions except OutOfMemoryException, which could mask corruption in state-mutating handlers like PostIndexChanged, DehydrateFolders, and ReleaseLock — leaving the mount serving in an inconsistent state. The catch now logs the message header and exception (context that OnNewConnection's generic catch in NamedPipeServer does not capture), then rethrows so fail-fast via Environment.Exit still occurs for unhandled errors in state-mutating handlers. The inner try-catch in HandleDownloadObjectRequest remains unchanged — it correctly catches and returns DownloadFailed for transient network/disk errors in the download path, which is safe because no critical mount state is mutated. Assisted-by: Claude Opus 4.6 Signed-off-by: Tyrie Vella --- GVFS/GVFS.Mount/InProcessMount.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/GVFS/GVFS.Mount/InProcessMount.cs b/GVFS/GVFS.Mount/InProcessMount.cs index b88af37fc..d8e7b409a 100644 --- a/GVFS/GVFS.Mount/InProcessMount.cs +++ b/GVFS/GVFS.Mount/InProcessMount.cs @@ -707,6 +707,7 @@ private void HandleRequest(ITracer tracer, string request, NamedPipeServer.Conne metadata.Add("Header", message.Header); metadata.Add("Exception", e.ToString()); this.tracer.RelatedError(metadata, "HandleRequest: Unhandled exception in request handler"); + throw; } } From 34666c4f3661c39ec217c039fa06cd9a51431d9b Mon Sep 17 00:00:00 2001 From: Tyrie Vella Date: Thu, 9 Jul 2026 14:56:43 -0700 Subject: [PATCH 10/24] Gate CLI mount-progress display behind gvfs.mount-progress config PR #2005 restructured mount startup so the named pipe server starts before the parallel auth+validation wait. That early-pipe move is a reliability improvement -- it widens the WaitUntilMounted connect window, avoiding "GVFS.Mount process is not responding" failures when auth is slow (including the SYSTEM-service automount path) -- and is separable from the progress-display feature layered on top of it. Add a disabled-by-default gvfs.mount-progress config flag that gates only the display layer. When off, the mount process no longer surfaces MountProgress phase strings over the named pipe, and the CLI falls back to its existing static spinner (ConsoleHelper already renders just the base message when progress is empty). The reliability infrastructure -- early pipe start, HandleRequest Mounting-state guard, volatile currentState, and null-safe HandleGetStatusRequest -- is always on regardless of the flag, so stabilization builds keep the automount robustness without shipping the newer progress-display behavior. Assisted-by: Claude Opus 4.8 Signed-off-by: Tyrie Vella --- GVFS/GVFS.Common/GVFSConstants.cs | 8 +++++++ GVFS/GVFS.Mount/InProcessMount.cs | 38 ++++++++++++++++++++++++++++++- 2 files changed, 45 insertions(+), 1 deletion(-) diff --git a/GVFS/GVFS.Common/GVFSConstants.cs b/GVFS/GVFS.Common/GVFSConstants.cs index 8f135786a..72e8c060a 100644 --- a/GVFS/GVFS.Common/GVFSConstants.cs +++ b/GVFS/GVFS.Common/GVFSConstants.cs @@ -47,6 +47,14 @@ public static class GitConfig public const string ShowHydrationStatus = GVFSPrefix + "show-hydration-status"; public const bool ShowHydrationStatusDefault = false; + /* Gates the CLI mount-progress display layer (progress phase strings surfaced + * over the named pipe during `gvfs mount`). Disabled by default so stabilization + * builds keep the reliability-relevant early-pipe infrastructure without shipping + * the newer progress-display behavior. The early pipe, HandleRequest Mounting + * guard, volatile currentState, and null-safe GetStatus are always on regardless. */ + public const string MountProgress = GVFSPrefix + "mount-progress"; + public const bool MountProgressDefault = false; + public const string MaxHttpConnectionsConfig = GVFSPrefix + "max-http-connections"; public const string PrefetchUseIdx = GVFSPrefix + "prefetch-use-idx"; diff --git a/GVFS/GVFS.Mount/InProcessMount.cs b/GVFS/GVFS.Mount/InProcessMount.cs index b88af37fc..baa9ac90e 100644 --- a/GVFS/GVFS.Mount/InProcessMount.cs +++ b/GVFS/GVFS.Mount/InProcessMount.cs @@ -58,6 +58,12 @@ public class InProcessMount private volatile MountState currentState; private volatile string mountProgressMessage; + + // When false (default), the mount process does not surface progress phase + // strings over the named pipe, so the CLI falls back to its static spinner. + // This gates only the display layer; the early-pipe reliability infrastructure + // (early pipe start, HandleRequest Mounting guard, null-safe GetStatus) is unaffected. + private bool reportMountProgress; private HeartbeatThread heartbeat; private ManualResetEvent unmountEvent; @@ -233,6 +239,11 @@ private void MountWithLockAcquired(EventLevel verbosity, Keywords keywords) this.enlistment.InitializeCachePaths(localCacheRoot, gitObjectsRoot, blobSizesRoot); + // Read the display-layer flag once before the pipe opens so the very first + // GetStatus request is served consistently. Only gates the progress strings; + // the pipe still starts early regardless (reliability infrastructure). + this.reportMountProgress = this.ShouldReportMountProgress(); + // Start the pipe server early so MountVerb can connect and poll progress // during the parallel validation phase. Only GetStatus requests are // handled while currentState == Mounting (see HandleRequest guard). @@ -581,6 +592,27 @@ private NamedPipeServer StartNamedPipe() } } + private bool ShouldReportMountProgress() + { + // Read via a transient LibGit2Repo rather than the context's shared + // LibGit2RepoInvoker: the invoker isn't created until CreateContext (after the + // pipe starts), and its constructor force-loads the object store, which we must + // not do on the pre-pipe critical path. A raw LibGit2Repo opens only the repo + // handle + config (no object store) and gives native git-bool parsing. + try + { + using (LibGit2Repo repo = new LibGit2Repo(this.tracer, this.enlistment.WorkingDirectoryBackingRoot)) + { + return repo.GetConfigBool(GVFSConstants.GitConfig.MountProgress) ?? GVFSConstants.GitConfig.MountProgressDefault; + } + } + catch (Exception e) + { + this.tracer.RelatedWarning($"Failed to read {GVFSConstants.GitConfig.MountProgress} config, using default: {e.Message}"); + return GVFSConstants.GitConfig.MountProgressDefault; + } + } + private void FailMountAndExit(string error, params object[] args) { this.FailMountAndExit(ReturnCode.GenericError, error, args); @@ -1377,7 +1409,11 @@ private void HandleGetStatusRequest(NamedPipeServer.Connection connection) { case MountState.Mounting: response.MountStatus = NamedPipeMessages.GetStatus.Mounting; - response.MountProgress = this.mountProgressMessage; + if (this.reportMountProgress) + { + response.MountProgress = this.mountProgressMessage; + } + break; case MountState.Ready: From 75adf9cd50bd664ba3c66e74249487b8ce6c7664 Mon Sep 17 00:00:00 2001 From: Tyrie Vella Date: Thu, 9 Jul 2026 15:54:22 -0700 Subject: [PATCH 11/24] docs(agents): document release-shipping scope and feature-flag convention MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add two sections to AGENTS.md capturing repo facts that routinely mislead agents and aren't obvious from a fresh clone: - "What ships in a public release": FastFetch is built/signed but NOT attached to the GitHub release, and Git is not bundled — "default Microsoft Git version" bumps only change CI GIT_VERSION, while the shipped floor is MinimumGitVersion in Version.props. Both matter when scoping a release or writing changelog notes. - "Feature flags": product flags are git config keys under the gvfs. prefix, declared in GVFSConstants.cs and read via GetConfigBoolOrDefault, mirroring gvfs.show-hydration-status. Gate the runtime entry point, default false. Assisted-by: Claude Opus 4.8 Signed-off-by: Tyrie Vella --- AGENTS.md | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/AGENTS.md b/AGENTS.md index 4fafb53e9..e5dd769f3 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -166,6 +166,39 @@ Do not manually delete or re-run vcpkg unless you've changed an overlay port. If you've copied `out\` from another enlistment as a build-time shortcut, vcpkg results come along with it. +## What ships in a public release + +The signed release pipeline builds several artifacts, but the **public GitHub +release only carries the installer and symbols**: `SetupGVFS..exe` +(per arch) and `Symbols.zip`. + +- **FastFetch is NOT shipped publicly.** `FastFetch.exe` is built and + ESRP-signed as a pipeline artifact, but it is not attached to the GitHub + release. Treat FastFetch-only changes as **non-shipping** when scoping a + release or writing changelog notes — they don't reach the installer end + users get. +- **Git is NOT bundled.** VFS for Git does not ship Microsoft Git. PRs titled + "update default Microsoft Git version" change only the CI `GIT_VERSION` in + `.github/workflows/build.yaml` (used to install Git for build + functional + test runs) — they are **non-shipping**. The only shipped Git constraint is + the compiled `MinimumGitVersion` constant in `Version.props`, which is + changed separately and rarely. + +## Feature flags + +Product feature flags are **git config** keys under the `gvfs.` prefix (not a +separate config system). To add one, mirror `gvfs.show-hydration-status`: + +- Declare the key name and default in `GVFS.Common/GVFSConstants.cs` under + `GitConfig` (e.g. `ShowHydrationStatus = GVFSPrefix + "show-hydration-status"` + and `ShowHydrationStatusDefault = false`). +- Read it via `repo.GetConfigBoolOrDefault(name, default)` (or + `LibGit2RepoInvoker.GetConfigBoolOrDefault`) at the point of use. + +Default new gates to `false` and gate the **runtime entry point** into a +feature, not its build, so the code still compiles and ships (and keeps +getting exercised) while its behavior stays off by default. + ## Coding standards See [CONTRIBUTING.md](CONTRIBUTING.md) for StyleCop rules, error-handling From c8eb49cf8a6dd4cf44981632ea86d0fd4423c9ea Mon Sep 17 00:00:00 2001 From: Tyrie Vella Date: Thu, 9 Jul 2026 14:18:01 -0700 Subject: [PATCH 12/24] Bound the leaked ActiveEnumeration collection behind an off-by-default flag ProjFS does not always deliver EndDirectoryEnumeration for a StartDirectoryEnumeration (for example when an enumeration is cancelled), which leaks the corresponding ActiveEnumeration - and the projected item list it pins - in this.activeEnumerations. Over long-lived GVFS.Mount processes this unbounded growth is a suspected contributor to the memory pressure behind the native ProjFS command-completion crashes that surface downstream as STATUS_FILE_SYSTEM_VIRTUALIZATION_UNAVAILABLE (0xC000CE01). "Suspected" is the operative word: Watson shows radar_high_memory in projectedfslib.dll!prjcompletecommand, but we do not yet have direct evidence that the never-ended-enumeration leak is a dominant driver of that memory. This change is therefore split into two parts so we can measure before we change the enumeration hot path anywhere: 1. Observation telemetry (always on). ActiveEnumeration records a monotonic LastActivityTickCount (Environment.TickCount64) on creation and on every GetDirectoryEnumeration read. WindowsFileSystemVirtualizer now contributes ActiveEnumerationCount and ActiveCommandCount to the periodic Heartbeat event via a new FileSystemVirtualizer.AddHeartbeatMetadata hook. This ships the signal needed to confirm (or refute) that activeEnumerations grows without bound on real machines - independent of whether eviction is enabled. 2. Eviction (off by default). When the gvfs.max-active-enumerations git config is set to a positive value, a throttled sweep (at most once per minute, and only once the collection exceeds the configured count) evicts enumerations idle longer than a 5 minute timeout. Config unset or <= 0 disables eviction entirely, so the enumeration hot path is unchanged by default. Monotonic clocks are used for both the throttle and the staleness cutoff so wall-clock adjustments cannot disturb them, and the throttle claim uses an Interlocked.CompareExchange keyed on the previously-read tick so two threads entering the same interval cannot both sweep. Evicting a live-but-idle enumeration is safe: the next GetDirectoryEnumeration for that id misses activeEnumerations and returns HResult.InternalError - it fails that one directory listing loudly rather than returning truncated results as if complete - and every eviction is reported via telemetry. This complements the stabilization-safe "do not crash the mount on a native ProjFS failure" change (#2042), which is intentionally kept separate and does not touch the enumeration hot path. Eviction stays behind the flag until the Heartbeat telemetry confirms the leak is worth acting on. Tests: ActiveEnumeration records activity time; Heartbeat metadata reports the live enumeration count; with eviction disabled stale enumerations are retained; with eviction enabled stale enumerations are evicted (and their subsequent End fails with InternalError) while freshly-active ones are kept. Full unit suite passes. Assisted-by: Claude Opus 4.8 Signed-off-by: Tyrie Vella --- GVFS/GVFS.Common/GVFSConstants.cs | 1 + GVFS/GVFS.Common/Git/GitRepo.cs | 22 +++ .../ActiveEnumeration.cs | 19 ++ .../InternalsVisibleTo.cs | 3 + .../WindowsFileSystemVirtualizer.cs | 172 +++++++++++++++++- .../Virtualization/ActiveEnumerationTests.cs | 14 ++ .../WindowsFileSystemVirtualizerTests.cs | 76 ++++++++ .../FileSystem/FileSystemVirtualizer.cs | 8 + .../FileSystemCallbacks.cs | 2 + 9 files changed, 315 insertions(+), 2 deletions(-) create mode 100644 GVFS/GVFS.Platform.Windows/InternalsVisibleTo.cs diff --git a/GVFS/GVFS.Common/GVFSConstants.cs b/GVFS/GVFS.Common/GVFSConstants.cs index 8f135786a..8b67e6fb9 100644 --- a/GVFS/GVFS.Common/GVFSConstants.cs +++ b/GVFS/GVFS.Common/GVFSConstants.cs @@ -29,6 +29,7 @@ public static class GitConfig public const string MaxRetriesConfig = GVFSPrefix + "max-retries"; public const string TimeoutSecondsConfig = GVFSPrefix + "timeout-seconds"; public const string GitStatusCacheBackoffConfig = GVFSPrefix + "status-cache-backoff-seconds"; + public const string MaxActiveEnumerationsConfig = GVFSPrefix + "max-active-enumerations"; public const string MountId = GVFSPrefix + "mount-id"; public const string EnlistmentId = GVFSPrefix + "enlistment-id"; public const string CacheServer = GVFSPrefix + "cache-server"; diff --git a/GVFS/GVFS.Common/Git/GitRepo.cs b/GVFS/GVFS.Common/Git/GitRepo.cs index 1dc9550d4..302b0e13e 100644 --- a/GVFS/GVFS.Common/Git/GitRepo.cs +++ b/GVFS/GVFS.Common/Git/GitRepo.cs @@ -180,6 +180,28 @@ public virtual bool TryGetMissingSubTrees(string treeSha, out string[] subtrees) return succeeded; } + /// + /// Reads a git config value through the in-process libgit2 repo (no git.exe spawn). + /// + /// Config setting name (e.g. "gvfs.max-active-enumerations"). + /// + /// The config value, or null if the setting is unset. Only meaningful when this returns true. + /// + /// + /// true if the libgit2 repo was available and the lookup ran (the setting may still be unset, + /// in which case is null); false if no libgit2 repo is available. + /// + public virtual bool TryGetConfigValue(string name, out string value) + { + value = null; + if (this.libgit2RepoInvoker == null) + { + return false; + } + + return this.libgit2RepoInvoker.TryInvoke(repo => repo.GetConfigString(name), out value); + } + public void Dispose() { if (this.libgit2RepoInvoker != null) diff --git a/GVFS/GVFS.Platform.Windows/ActiveEnumeration.cs b/GVFS/GVFS.Platform.Windows/ActiveEnumeration.cs index 0baa27a4c..6fdb6a6d3 100644 --- a/GVFS/GVFS.Platform.Windows/ActiveEnumeration.cs +++ b/GVFS/GVFS.Platform.Windows/ActiveEnumeration.cs @@ -1,6 +1,7 @@ using GVFS.Common; using GVFS.Virtualization.Projection; using Microsoft.Windows.ProjFS; +using System; using System.Collections.Generic; namespace GVFS.Platform.Windows @@ -20,10 +21,28 @@ public ActiveEnumeration(List fileInfos) this.fileInfoEnumerator = new ProjectedFileInfoEnumerator(fileInfos); this.ResetEnumerator(); this.MoveNext(); + this.RecordActivity(); } public delegate bool FileNamePatternMatcher(string name, string pattern); + /// + /// Monotonic tick count (, milliseconds) of the most + /// recent activity (creation or enumeration read) on this enumeration. Used to identify + /// enumerations that ProjFS never ended, so their memory can be reclaimed. A monotonic + /// source is used (rather than wall-clock time) so that NTP/clock adjustments cannot cause a + /// live-but-idle enumeration to be misjudged as stale. + /// + public long LastActivityTickCount { get; private set; } + + /// + /// Records that this enumeration was just touched, so it is not treated as abandoned. + /// + public void RecordActivity() + { + this.LastActivityTickCount = Environment.TickCount64; + } + /// /// true if Current refers to an element in the enumeration, false if Current is past the end of the collection /// diff --git a/GVFS/GVFS.Platform.Windows/InternalsVisibleTo.cs b/GVFS/GVFS.Platform.Windows/InternalsVisibleTo.cs new file mode 100644 index 000000000..9fd1d13ff --- /dev/null +++ b/GVFS/GVFS.Platform.Windows/InternalsVisibleTo.cs @@ -0,0 +1,3 @@ +using System.Runtime.CompilerServices; + +[assembly: InternalsVisibleTo("GVFS.UnitTests")] diff --git a/GVFS/GVFS.Platform.Windows/WindowsFileSystemVirtualizer.cs b/GVFS/GVFS.Platform.Windows/WindowsFileSystemVirtualizer.cs index f39128e7a..eb2f9980d 100644 --- a/GVFS/GVFS.Platform.Windows/WindowsFileSystemVirtualizer.cs +++ b/GVFS/GVFS.Platform.Windows/WindowsFileSystemVirtualizer.cs @@ -33,12 +33,39 @@ public class WindowsFileSystemVirtualizer : FileSystemVirtualizer, IRequiredCall private ConcurrentDictionary activeEnumerations; private ConcurrentDictionary activeCommands; + // ProjFS does not always deliver EndDirectoryEnumeration for a StartDirectoryEnumeration + // (for example when an enumeration is cancelled), which leaks the corresponding + // ActiveEnumeration - and the projected item list it pins - in this.activeEnumerations. Over + // long-lived mounts this unbounded growth is a suspected contributor to the memory pressure + // that ultimately crashes GVFS.Mount in the native ProjFS command-completion path. + // + // Eviction of stale (never-ended) enumerations is OFF by default and enabled only when the + // gvfs.max-active-enumerations git config is set to a positive value: once activeEnumerations + // grows past that threshold, a throttled sweep evicts entries idle longer than a timeout. + // The count telemetry below is always emitted on the Heartbeat event so the leak can be + // observed in the field before eviction is turned on anywhere. + private static readonly TimeSpan ActiveEnumerationStaleTimeout = TimeSpan.FromMinutes(5); + private static readonly TimeSpan ActiveEnumerationEvictionSweepInterval = TimeSpan.FromMinutes(1); + + // Effective staleness cutoff for a never-ended enumeration. Defaults to the constant above; + // overridable by tests via ActiveEnumerationStaleTimeoutForTest. + private TimeSpan activeEnumerationStaleTimeout = ActiveEnumerationStaleTimeout; + + // 0 disables eviction (the default). When > 0, it is the count of live enumerations above + // which a sweep evicts stale ones. Read from git config (gvfs.max-active-enumerations). + private int maxActiveEnumerations; + + // Monotonic (Environment.TickCount64, milliseconds) timestamp of the last eviction sweep, so + // the throttle cannot be disturbed by wall-clock adjustments. + private long lastEnumerationEvictionSweepTickCount = Environment.TickCount64; + public WindowsFileSystemVirtualizer(GVFSContext context, GVFSGitObjects gitObjects) : this( context, gitObjects, virtualizationInstance: null, - numWorkerThreads: FileSystemVirtualizer.DefaultNumWorkerThreads) + numWorkerThreads: FileSystemVirtualizer.DefaultNumWorkerThreads, + maxActiveEnumerations: ReadMaxActiveEnumerationsFromConfig(context)) { } @@ -46,7 +73,8 @@ public WindowsFileSystemVirtualizer( GVFSContext context, GVFSGitObjects gitObjects, IVirtualizationInstance virtualizationInstance, - int numWorkerThreads) + int numWorkerThreads, + int maxActiveEnumerations = 0) : base(context, gitObjects, numWorkerThreads) { List notificationMappings = new List() @@ -72,10 +100,146 @@ public WindowsFileSystemVirtualizer( this.activeEnumerations = new ConcurrentDictionary(); this.activeCommands = new ConcurrentDictionary(); + + this.maxActiveEnumerations = maxActiveEnumerations; } protected override string EtwArea => ClassName; + public override void AddHeartbeatMetadata(EventMetadata metadata) + { + // Always emitted, even when eviction is disabled, so the size of the activeEnumerations + // collection (and thus whether the never-ended-enumeration leak is a real memory driver) + // can be observed in the field before enabling eviction anywhere. + metadata.Add("ActiveEnumerationCount", this.activeEnumerations.Count); + metadata.Add("ActiveCommandCount", this.activeCommands.Count); + } + + /// + /// Reads the gvfs.max-active-enumerations git config value (0 = eviction disabled, the + /// default). Uses the in-process libgit2 repo (no git.exe spawn). Never throws: any failure + /// reading git config leaves eviction disabled. + /// + private static int ReadMaxActiveEnumerationsFromConfig(GVFSContext context) + { + try + { + if (context?.Repository != null && + context.Repository.TryGetConfigValue(GVFSConstants.GitConfig.MaxActiveEnumerationsConfig, out string rawValue)) + { + if (string.IsNullOrWhiteSpace(rawValue)) + { + // Setting is unset: eviction disabled. + return 0; + } + + if (int.TryParse(rawValue.Trim(), out int value) && value > 0) + { + return value; + } + + if (context.Tracer != null) + { + EventMetadata metadata = new EventMetadata(); + metadata.Add("Area", ClassName); + metadata.Add("configValue", rawValue); + context.Tracer.RelatedWarning(metadata, nameof(ReadMaxActiveEnumerationsFromConfig) + ": could not parse " + GVFSConstants.GitConfig.MaxActiveEnumerationsConfig + " as a positive int, leaving enumeration eviction disabled"); + } + } + } + catch (Exception e) + { + if (context?.Tracer != null) + { + EventMetadata metadata = new EventMetadata(); + metadata.Add("Area", ClassName); + metadata.Add("Exception", e.ToString()); + context.Tracer.RelatedWarning(metadata, nameof(ReadMaxActiveEnumerationsFromConfig) + ": exception reading git config, leaving enumeration eviction disabled"); + } + } + + return 0; + } + + private void MaybeEvictStaleEnumerations() + { + if (this.maxActiveEnumerations <= 0) + { + // Eviction disabled (gvfs.max-active-enumerations unset or non-positive). + return; + } + + long now = Environment.TickCount64; + long last = Interlocked.Read(ref this.lastEnumerationEvictionSweepTickCount); + if (now - last < (long)ActiveEnumerationEvictionSweepInterval.TotalMilliseconds) + { + return; + } + + if (Interlocked.CompareExchange(ref this.lastEnumerationEvictionSweepTickCount, now, last) != last) + { + // Another thread just claimed this sweep interval. + return; + } + + this.EvictStaleEnumerations(); + } + + private void EvictStaleEnumerations() + { + if (this.activeEnumerations.Count <= this.maxActiveEnumerations) + { + return; + } + + long cutoff = Environment.TickCount64 - (long)this.activeEnumerationStaleTimeout.TotalMilliseconds; + int evictedCount = 0; + foreach (KeyValuePair entry in this.activeEnumerations) + { + if (entry.Value.LastActivityTickCount < cutoff && + this.activeEnumerations.TryRemove(entry.Key, out _)) + { + evictedCount++; + } + } + + if (evictedCount > 0) + { + EventMetadata metadata = this.CreateEventMetadata(); + metadata.Add("evictedCount", evictedCount); + metadata.Add("remainingCount", this.activeEnumerations.Count); + metadata.Add("maxActiveEnumerations", this.maxActiveEnumerations); + metadata.Add("staleTimeoutMinutes", this.activeEnumerationStaleTimeout.TotalMinutes); + metadata.Add("gcTotalMemoryBytes", GC.GetTotalMemory(forceFullCollection: false)); + metadata.Add( + TracingConstants.MessageKey.WarningMessage, + nameof(this.EvictStaleEnumerations) + ": evicted stale directory enumerations that ProjFS never ended, to bound memory usage"); + this.Context.Tracer.RelatedEvent(EventLevel.Warning, nameof(this.EvictStaleEnumerations), metadata, Keywords.Telemetry); + } + } + + internal TimeSpan ActiveEnumerationStaleTimeoutForTest + { + set { this.activeEnumerationStaleTimeout = value; } + } + + internal int MaxActiveEnumerationsForTest + { + set { this.maxActiveEnumerations = value; } + } + + /// + /// Test-only: resets the sweep throttle and runs the same eviction path the enumeration hot + /// callback runs, so eviction behavior can be exercised deterministically. + /// + internal void ForceEnumerationEvictionSweepForTest() + { + Interlocked.Exchange( + ref this.lastEnumerationEvictionSweepTickCount, + Environment.TickCount64 - (long)ActiveEnumerationEvictionSweepInterval.TotalMilliseconds - 1); + this.MaybeEvictStaleEnumerations(); + } + /// /// Public for unit testing /// @@ -211,6 +375,8 @@ public HResult StartDirectoryEnumerationCallback(int commandId, Guid enumeration { try { + this.MaybeEvictStaleEnumerations(); + List projectedItems; if (this.FileSystemCallbacks.GitIndexProjection.TryGetProjectedItemsFromMemory(virtualPath, out projectedItems)) { @@ -287,6 +453,8 @@ public HResult GetDirectoryEnumerationCallback( return HResult.InternalError; } + activeEnumeration.RecordActivity(); + if (restartScan) { activeEnumeration.RestartEnumeration(filterFileName); diff --git a/GVFS/GVFS.UnitTests/Windows/Virtualization/ActiveEnumerationTests.cs b/GVFS/GVFS.UnitTests/Windows/Virtualization/ActiveEnumerationTests.cs index 6770d2e1a..e914a00f8 100644 --- a/GVFS/GVFS.UnitTests/Windows/Virtualization/ActiveEnumerationTests.cs +++ b/GVFS/GVFS.UnitTests/Windows/Virtualization/ActiveEnumerationTests.cs @@ -45,6 +45,20 @@ public void EnumerationHandlesEmptyList() activeEnumeration.Current.ShouldEqual(null); } + [TestCase] + public void RecordActivityUpdatesLastActivityTime() + { + long beforeCreate = Environment.TickCount64; + ActiveEnumeration activeEnumeration = CreateActiveEnumeration(new List()); + + // The constructor records initial activity. + (activeEnumeration.LastActivityTickCount >= beforeCreate).ShouldBeTrue(); + + long beforeRecord = Environment.TickCount64; + activeEnumeration.RecordActivity(); + (activeEnumeration.LastActivityTickCount >= beforeRecord).ShouldBeTrue(); + } + [TestCase] public void EnumerateSingleEntryList() { diff --git a/GVFS/GVFS.UnitTests/Windows/Virtualization/WindowsFileSystemVirtualizerTests.cs b/GVFS/GVFS.UnitTests/Windows/Virtualization/WindowsFileSystemVirtualizerTests.cs index 5ca3f4115..8de5895cc 100644 --- a/GVFS/GVFS.UnitTests/Windows/Virtualization/WindowsFileSystemVirtualizerTests.cs +++ b/GVFS/GVFS.UnitTests/Windows/Virtualization/WindowsFileSystemVirtualizerTests.cs @@ -11,7 +11,9 @@ using System; using System.Collections.Generic; using System.IO; +using System.Threading; using System.Threading.Tasks; +using GVFS.Common.Tracing; namespace GVFS.UnitTests.Windows.Virtualization { @@ -181,6 +183,80 @@ public void OnStartDirectoryEnumerationReturnsSuccessWhenResultsInMemory() } } + [TestCase] + public void HeartbeatMetadataReportsActiveEnumerationCount() + { + using (WindowsFileSystemVirtualizerTester tester = new WindowsFileSystemVirtualizerTester(this.Repo, new[] { "test" })) + { + tester.GitIndexProjection.EnumerationInMemory = true; + + tester.MockVirtualization.RequiredCallbacks.StartDirectoryEnumerationCallback(1, Guid.NewGuid(), "test", TriggeringProcessId, TriggeringProcessImageFileName).ShouldEqual(HResult.Ok); + tester.MockVirtualization.RequiredCallbacks.StartDirectoryEnumerationCallback(2, Guid.NewGuid(), "test", TriggeringProcessId, TriggeringProcessImageFileName).ShouldEqual(HResult.Ok); + + EventMetadata metadata = new EventMetadata(); + tester.WindowsVirtualizer.AddHeartbeatMetadata(metadata); + + metadata.ContainsKey("ActiveEnumerationCount").ShouldBeTrue(); + ((int)metadata["ActiveEnumerationCount"]).ShouldEqual(2); + metadata.ContainsKey("ActiveCommandCount").ShouldBeTrue(); + } + } + + [TestCase] + public void StaleEnumerationsAreNotEvictedWhenDisabled() + { + using (WindowsFileSystemVirtualizerTester tester = new WindowsFileSystemVirtualizerTester(this.Repo, new[] { "test" })) + { + tester.GitIndexProjection.EnumerationInMemory = true; + + // Eviction is disabled by default (gvfs.max-active-enumerations unset => 0). Even with + // a zero stale timeout - which would make every enumeration eligible - nothing is evicted. + tester.WindowsVirtualizer.MaxActiveEnumerationsForTest = 0; + tester.WindowsVirtualizer.ActiveEnumerationStaleTimeoutForTest = TimeSpan.Zero; + + Guid firstId = Guid.NewGuid(); + Guid secondId = Guid.NewGuid(); + tester.MockVirtualization.RequiredCallbacks.StartDirectoryEnumerationCallback(1, firstId, "test", TriggeringProcessId, TriggeringProcessImageFileName).ShouldEqual(HResult.Ok); + Thread.Sleep(20); + tester.MockVirtualization.RequiredCallbacks.StartDirectoryEnumerationCallback(2, secondId, "test", TriggeringProcessId, TriggeringProcessImageFileName).ShouldEqual(HResult.Ok); + + tester.WindowsVirtualizer.ForceEnumerationEvictionSweepForTest(); + + // Both enumerations are still present (a missing id would return InternalError). + tester.MockVirtualization.RequiredCallbacks.EndDirectoryEnumerationCallback(firstId).ShouldEqual(HResult.Ok); + tester.MockVirtualization.RequiredCallbacks.EndDirectoryEnumerationCallback(secondId).ShouldEqual(HResult.Ok); + } + } + + [TestCase] + public void StaleEnumerationsAreEvictedWhenEnabledButLiveOnesAreKept() + { + using (WindowsFileSystemVirtualizerTester tester = new WindowsFileSystemVirtualizerTester(this.Repo, new[] { "test" })) + { + tester.GitIndexProjection.EnumerationInMemory = true; + + // Enable eviction above 1 live enumeration, treating anything idle longer than 20ms as stale. + tester.WindowsVirtualizer.MaxActiveEnumerationsForTest = 1; + tester.WindowsVirtualizer.ActiveEnumerationStaleTimeoutForTest = TimeSpan.FromMilliseconds(20); + + Guid staleId = Guid.NewGuid(); + tester.MockVirtualization.RequiredCallbacks.StartDirectoryEnumerationCallback(1, staleId, "test", TriggeringProcessId, TriggeringProcessImageFileName).ShouldEqual(HResult.Ok); + + // Let the first enumeration age well past the stale timeout before adding a fresh one. + Thread.Sleep(200); + + Guid freshId = Guid.NewGuid(); + tester.MockVirtualization.RequiredCallbacks.StartDirectoryEnumerationCallback(2, freshId, "test", TriggeringProcessId, TriggeringProcessImageFileName).ShouldEqual(HResult.Ok); + + tester.WindowsVirtualizer.ForceEnumerationEvictionSweepForTest(); + + // The stale enumeration was evicted: its End now fails to find it (fails loudly, does + // not silently return partial results). The fresh enumeration is retained. + tester.MockVirtualization.RequiredCallbacks.EndDirectoryEnumerationCallback(staleId).ShouldEqual(HResult.InternalError); + tester.MockVirtualization.RequiredCallbacks.EndDirectoryEnumerationCallback(freshId).ShouldEqual(HResult.Ok); + } + } + [TestCase] public void GetPlaceholderInformationHandlerPathNotProjected() { diff --git a/GVFS/GVFS.Virtualization/FileSystem/FileSystemVirtualizer.cs b/GVFS/GVFS.Virtualization/FileSystem/FileSystemVirtualizer.cs index 41e596401..b3d0c0d3d 100644 --- a/GVFS/GVFS.Virtualization/FileSystem/FileSystemVirtualizer.cs +++ b/GVFS/GVFS.Virtualization/FileSystem/FileSystemVirtualizer.cs @@ -111,6 +111,14 @@ public abstract FileSystemResult UpdatePlaceholderIfNeeded( public abstract FileSystemResult DehydrateFolder(string relativePath); + /// + /// Adds platform-specific virtualization diagnostics (for example live directory-enumeration + /// counts) to the periodic Heartbeat telemetry event. The default implementation adds nothing. + /// + public virtual void AddHeartbeatMetadata(EventMetadata metadata) + { + } + public void Dispose() { if (this.fileAndNetworkRequests != null) diff --git a/GVFS/GVFS.Virtualization/FileSystemCallbacks.cs b/GVFS/GVFS.Virtualization/FileSystemCallbacks.cs index ab2ff36d7..ac25431cf 100644 --- a/GVFS/GVFS.Virtualization/FileSystemCallbacks.cs +++ b/GVFS/GVFS.Virtualization/FileSystemCallbacks.cs @@ -278,6 +278,8 @@ public EventMetadata GetAndResetHeartBeatMetadata(out bool logToFile) metadata.Add("FilePlaceholderCount", this.placeholderDatabase.GetFilePlaceholdersCount()); metadata.Add("FolderPlaceholderCount", this.placeholderDatabase.GetFolderPlaceholdersCount()); + this.fileSystemVirtualizer?.AddHeartbeatMetadata(metadata); + if (this.gitStatusCache.WriteTelemetryandReset(metadata)) { logToFile = true; From 0dcc2e81a272a8c30b5547a885d88216cacae9c3 Mon Sep 17 00:00:00 2001 From: Tyrie Vella Date: Thu, 9 Jul 2026 16:13:13 -0700 Subject: [PATCH 13/24] Remove dead org auto-upgrade code The in-product org auto-upgrade mechanism (OrgInfo server + upgrade ring + NuGet feed) has been dead for years: `gvfs upgrade` is already a no-op stub and nothing consumes these paths. Fleet upgrades are driven externally from GitHub Releases. Remove the dead code while keeping the `gvfs upgrade` verb in its current no-op stub state: - OrgInfoApiClient / VersionResponse (and its GVFSJsonContext registration) - AzDevOpsOrgFromNuGetFeed (NuGet-feed org-parse helper, only referenced by the dead upgrader path and its own test) - LocalGVFSConfig keys upgrade.ring / upgrade.feedpackagename / upgrade.feedurl / upgrade.orgInfoServerUrl - the now-unreferenced GVFSConstants.UpgradeVerbMessages class - MockLocalGVFSConfigBuilder and the removed types' unit tests - installer ring->NuGet-feed plumbing (SetNuGetFeedIfNecessary, GetConfiguredUpgradeRing, IsConfigured, SetIfNotConfigured, UpgradeRing) Build (including the Inno Setup installer) and all unit tests pass. Assisted-by: Claude Opus 4.8 Signed-off-by: Tyrie Vella --- GVFS/GVFS.Common/AzDevOpsOrgFromNuGetFeed.cs | 51 -------- GVFS/GVFS.Common/GVFSConstants.cs | 16 --- GVFS/GVFS.Common/GVFSJsonContext.cs | 1 - GVFS/GVFS.Common/OrgInfoApiClient.cs | 78 ------------- GVFS/GVFS.Common/VersionResponse.cs | 13 --- GVFS/GVFS.Installers/Setup.iss | 84 ------------- .../Common/AzDevOpsOrgFromNuGetFeedTests.cs | 35 ------ .../Common/OrgInfoApiClientTests.cs | 110 ------------------ .../Mock/Common/MockLocalGVFSConfigBuilder.cs | 91 --------------- 9 files changed, 479 deletions(-) delete mode 100644 GVFS/GVFS.Common/AzDevOpsOrgFromNuGetFeed.cs delete mode 100644 GVFS/GVFS.Common/OrgInfoApiClient.cs delete mode 100644 GVFS/GVFS.Common/VersionResponse.cs delete mode 100644 GVFS/GVFS.UnitTests/Common/AzDevOpsOrgFromNuGetFeedTests.cs delete mode 100644 GVFS/GVFS.UnitTests/Common/OrgInfoApiClientTests.cs delete mode 100644 GVFS/GVFS.UnitTests/Mock/Common/MockLocalGVFSConfigBuilder.cs diff --git a/GVFS/GVFS.Common/AzDevOpsOrgFromNuGetFeed.cs b/GVFS/GVFS.Common/AzDevOpsOrgFromNuGetFeed.cs deleted file mode 100644 index aaafb682f..000000000 --- a/GVFS/GVFS.Common/AzDevOpsOrgFromNuGetFeed.cs +++ /dev/null @@ -1,51 +0,0 @@ -using System.Text.RegularExpressions; - -namespace GVFS.Common -{ - public class AzDevOpsOrgFromNuGetFeed - { - /// - /// Given a URL for a NuGet feed hosted on Azure DevOps, - /// return the organization that hosts the feed. - /// - public static bool TryParseOrg(string packageFeedUrl, out string orgName) - { - // We expect a URL of the form https://pkgs.dev.azure.com/{org} - // and want to convert it to a URL of the form https://{org}.visualstudio.com - Regex packageUrlRegex = new Regex( - @"^https://pkgs.dev.azure.com/(?.+?)/", - RegexOptions.CultureInvariant | RegexOptions.IgnoreCase); - Match urlMatch = packageUrlRegex.Match(packageFeedUrl); - - if (!urlMatch.Success) - { - orgName = null; - return false; - } - - orgName = urlMatch.Groups["org"].Value; - return true; - } - - /// - /// Given a URL for a NuGet feed hosted on Azure DevOps, - /// return a URL that Git Credential Manager can use to - /// query for a credential that is valid for use with the - /// NuGet feed. - /// - public static bool TryCreateCredentialQueryUrl(string packageFeedUrl, out string azureDevOpsUrl, out string error) - { - if (!TryParseOrg(packageFeedUrl, out string org)) - { - azureDevOpsUrl = null; - error = $"Input URL {packageFeedUrl} did not match expected format for an Azure DevOps Package Feed URL"; - return false; - } - - azureDevOpsUrl = $"https://{org}.visualstudio.com"; - error = null; - - return true; - } - } -} diff --git a/GVFS/GVFS.Common/GVFSConstants.cs b/GVFS/GVFS.Common/GVFSConstants.cs index 8f135786a..7c159fd50 100644 --- a/GVFS/GVFS.Common/GVFSConstants.cs +++ b/GVFS/GVFS.Common/GVFSConstants.cs @@ -55,10 +55,6 @@ public static class GitConfig public static class LocalGVFSConfig { - public const string UpgradeRing = "upgrade.ring"; - public const string UpgradeFeedPackageName = "upgrade.feedpackagename"; - public const string UpgradeFeedUrl = "upgrade.feedurl"; - public const string OrgInfoServerUrl = "upgrade.orgInfoServerUrl"; public const string USNJournalUpdates = "usn.updateDirectories"; } @@ -268,17 +264,5 @@ public static class Unmount public const string SkipLock = "skip-wait-for-lock"; } } - - public static class UpgradeVerbMessages - { - public const string GVFSUpgrade = "`gvfs upgrade`"; - public const string GVFSUpgradeDryRun = "`gvfs upgrade --dry-run`"; - public const string NoUpgradeCheckPerformed = "No upgrade check was performed."; - public const string NoneRingConsoleAlert = "Upgrade ring set to \"None\". " + NoUpgradeCheckPerformed; - public const string NoRingConfigConsoleAlert = "Upgrade ring is not set. " + NoUpgradeCheckPerformed; - public const string InvalidRingConsoleAlert = "Upgrade ring set to unknown value. " + NoUpgradeCheckPerformed; - public const string SetUpgradeRingCommand = "To set or change upgrade ring, run `gvfs config " + LocalGVFSConfig.UpgradeRing + " [\"Fast\"|\"Slow\"|\"None\"]` from a command prompt."; - public const string UnmountRepoWarning = "Upgrade will unmount and remount gvfs repos, ensure you are at a stopping point."; - } } } diff --git a/GVFS/GVFS.Common/GVFSJsonContext.cs b/GVFS/GVFS.Common/GVFSJsonContext.cs index 13b35b27c..555da7ca5 100644 --- a/GVFS/GVFS.Common/GVFSJsonContext.cs +++ b/GVFS/GVFS.Common/GVFSJsonContext.cs @@ -23,7 +23,6 @@ namespace GVFS.Common [JsonSerializable(typeof(List))] [JsonSerializable(typeof(List))] [JsonSerializable(typeof(ServerGVFSConfig))] - [JsonSerializable(typeof(VersionResponse))] [JsonSerializable(typeof(InternalVerbParameters))] [JsonSerializable(typeof(CacheServerInfo))] [JsonSerializable(typeof(NamedPipeMessages.GetStatus.Response), TypeInfoPropertyName = "GetStatusResponse")] diff --git a/GVFS/GVFS.Common/OrgInfoApiClient.cs b/GVFS/GVFS.Common/OrgInfoApiClient.cs deleted file mode 100644 index 93dbd97d9..000000000 --- a/GVFS/GVFS.Common/OrgInfoApiClient.cs +++ /dev/null @@ -1,78 +0,0 @@ -using System; -using System.Collections.Generic; -using System.Linq; -using System.Net; -using System.Net.Http; -using System.Text; - -namespace GVFS.Common -{ - /// - /// Class that handles communication with a server that contains version information. - /// - public class OrgInfoApiClient - { - private const string VersionApi = "/api/GetLatestVersion"; - - private HttpClient client; - private string baseUrl; - - public OrgInfoApiClient(HttpClient client, string baseUrl) - { - this.client = client; - this.baseUrl = baseUrl; - } - - private string VersionUrl - { - get - { - return this.baseUrl + VersionApi; - } - } - - public Version QueryNewestVersion(string orgName, string platform, string ring) - { - Dictionary queryParams = new Dictionary() - { - { "Organization", orgName }, - { "Platform", platform }, - { "Ring", ring }, - }; - - string responseString = this.client.GetStringAsync(this.ConstructRequest(this.VersionUrl, queryParams)).GetAwaiter().GetResult(); - VersionResponse versionResponse = VersionResponse.FromJsonString(responseString); - - if (string.IsNullOrEmpty(versionResponse.Version)) - { - return null; - } - - return new Version(versionResponse.Version); - } - - private string ConstructRequest(string baseUrl, Dictionary queryParams) - { - StringBuilder sb = new StringBuilder(baseUrl); - - if (queryParams.Any()) - { - sb.Append("?"); - } - - bool isFirst = true; - foreach (KeyValuePair kvp in queryParams) - { - if (!isFirst) - { - sb.Append("&"); - } - - isFirst = false; - sb.Append($"{WebUtility.UrlEncode(kvp.Key)}={WebUtility.UrlEncode(kvp.Value)}"); - } - - return sb.ToString(); - } - } -} diff --git a/GVFS/GVFS.Common/VersionResponse.cs b/GVFS/GVFS.Common/VersionResponse.cs deleted file mode 100644 index 08864f5c0..000000000 --- a/GVFS/GVFS.Common/VersionResponse.cs +++ /dev/null @@ -1,13 +0,0 @@ - -namespace GVFS.Common -{ - public class VersionResponse - { - public string Version { get; set; } - - public static VersionResponse FromJsonString(string jsonString) - { - return GVFSJsonOptions.Deserialize(jsonString); - } - } -} diff --git a/GVFS/GVFS.Installers/Setup.iss b/GVFS/GVFS.Installers/Setup.iss index 4393da41f..a2e31a75d 100644 --- a/GVFS/GVFS.Installers/Setup.iss +++ b/GVFS/GVFS.Installers/Setup.iss @@ -624,89 +624,6 @@ begin Result := True; end; -type - UpgradeRing = (urUnconfigured, urNone, urFast, urSlow); - -function GetConfiguredUpgradeRing(): UpgradeRing; -var - ResultCode: integer; - ResultString: ansiString; -begin - Result := urUnconfigured; - if ExecWithResult('gvfs.exe', 'config upgrade.ring', '', SW_HIDE, ewWaitUntilTerminated, ResultCode, ResultString) then begin - if ResultCode = 0 then begin - ResultString := AnsiLowercase(Trim(ResultString)); - Log('GetConfiguredUpgradeRing: upgrade.ring is ' + ResultString); - if CompareText(ResultString, 'none') = 0 then begin - Result := urNone; - end else if CompareText(ResultString, 'fast') = 0 then begin - Result := urFast; - end else if CompareText(ResultString, 'slow') = 0 then begin - Result := urSlow; - end else begin - Log('GetConfiguredUpgradeRing: Unknown upgrade ring: ' + ResultString); - end; - end else begin - Log('GetConfiguredUpgradeRing: Call to gvfs config upgrade.ring failed with ' + SysErrorMessage(ResultCode)); - end; - end else begin - Log('GetConfiguredUpgradeRing: Call to gvfs config upgrade.ring failed with ' + SysErrorMessage(ResultCode)); - end; -end; - -function IsConfigured(ConfigKey: String): Boolean; -var - ResultCode: integer; - ResultString: ansiString; -begin - Result := False - if ExecWithResult('gvfs.exe', Format('config %s', [ConfigKey]), '', SW_HIDE, ewWaitUntilTerminated, ResultCode, ResultString) then begin - ResultString := AnsiLowercase(Trim(ResultString)); - Log(Format('IsConfigured(%s): value is %s', [ConfigKey, ResultString])); - Result := Length(ResultString) > 1 - end -end; - -procedure SetIfNotConfigured(ConfigKey: String; ConfigValue: String); -var - ResultCode: integer; - ResultString: ansiString; -begin - if IsConfigured(ConfigKey) = False then begin - if ExecWithResult('gvfs.exe', Format('config %s %s', [ConfigKey, ConfigValue]), '', SW_HIDE, ewWaitUntilTerminated, ResultCode, ResultString) then begin - Log(Format('SetIfNotConfigured: Set %s to %s', [ConfigKey, ConfigValue])); - end else begin - Log(Format('SetIfNotConfigured: Failed to set %s with %s', [ConfigKey, SysErrorMessage(ResultCode)])); - end; - end else begin - Log(Format('SetIfNotConfigured: %s is configured, not overwriting', [ConfigKey])); - end; -end; - -procedure SetNuGetFeedIfNecessary(); -var - ConfiguredRing: UpgradeRing; - RingName: String; - TargetFeed: String; - FeedPackageName: String; -begin - ConfiguredRing := GetConfiguredUpgradeRing(); - if ConfiguredRing = urFast then begin - RingName := 'Fast'; - end else if (ConfiguredRing = urSlow) or (ConfiguredRing = urNone) then begin - RingName := 'Slow'; - end else begin - Log('SetNuGetFeedIfNecessary: No upgrade ring configured. Not configuring NuGet feed.') - exit; - end; - - TargetFeed := Format('https://pkgs.dev.azure.com/microsoft/_packaging/VFSForGit-%s/nuget/v3/index.json', [RingName]); - FeedPackageName := 'Microsoft.VfsForGitEnvironment'; - - SetIfNotConfigured('upgrade.feedurl', TargetFeed); - SetIfNotConfigured('upgrade.feedpackagename', FeedPackageName); -end; - // Below are EVENT FUNCTIONS -> The main entry points of InnoSetup into the code region // Documentation : http://www.jrsoftware.org/ishelp/index.php?topic=scriptevents @@ -905,7 +822,6 @@ begin NeedsRestart := False; KeepMountsRunning := False; Result := ''; - SetNuGetFeedIfNecessary(); // Check for mounted repos by querying the service, and also check for // running GVFS processes (a mount can be running without being registered diff --git a/GVFS/GVFS.UnitTests/Common/AzDevOpsOrgFromNuGetFeedTests.cs b/GVFS/GVFS.UnitTests/Common/AzDevOpsOrgFromNuGetFeedTests.cs deleted file mode 100644 index 74dbdbb66..000000000 --- a/GVFS/GVFS.UnitTests/Common/AzDevOpsOrgFromNuGetFeedTests.cs +++ /dev/null @@ -1,35 +0,0 @@ -using GVFS.Common; -using GVFS.Tests.Should; -using NUnit.Framework; - -namespace GVFS.UnitTests.Common -{ - [TestFixture] - public class NuGetUpgraderTests - { - [TestCase("https://pkgs.dev.azure.com/test-pat/_packaging/Test-GVFS-Installers-Custom/nuget/v3/index.json", "https://test-pat.visualstudio.com")] - [TestCase("https://PKGS.DEV.azure.com/test-pat/_packaging/Test-GVFS-Installers-Custom/nuget/v3/index.json", "https://test-pat.visualstudio.com")] - [TestCase("https://dev.azure.com/test-pat/_packaging/Test-GVFS-Installers-Custom/nuget/v3/index.json", null)] - [TestCase("http://pkgs.dev.azure.com/test-pat/_packaging/Test-GVFS-Installers-Custom/nuget/v3/index.json", null)] - public void CanConstructAzureDevOpsUrlFromPackageFeedUrl(string packageFeedUrl, string expectedAzureDevOpsUrl) - { - bool success = AzDevOpsOrgFromNuGetFeed.TryCreateCredentialQueryUrl( - packageFeedUrl, - out string azureDevOpsUrl, - out string error); - - if (expectedAzureDevOpsUrl != null) - { - success.ShouldBeTrue(); - azureDevOpsUrl.ShouldEqual(expectedAzureDevOpsUrl); - error.ShouldBeNull(); - } - else - { - success.ShouldBeFalse(); - azureDevOpsUrl.ShouldBeNull(); - error.ShouldNotBeNull(); - } - } - } -} diff --git a/GVFS/GVFS.UnitTests/Common/OrgInfoApiClientTests.cs b/GVFS/GVFS.UnitTests/Common/OrgInfoApiClientTests.cs deleted file mode 100644 index ced645d08..000000000 --- a/GVFS/GVFS.UnitTests/Common/OrgInfoApiClientTests.cs +++ /dev/null @@ -1,110 +0,0 @@ -using GVFS.Common; -using GVFS.Tests.Should; -using Moq; -using Moq.Protected; -using NUnit.Framework; -using System; -using System.Collections.Generic; -using System.Net; -using System.Net.Http; -using System.Threading; -using System.Threading.Tasks; - -namespace GVFS.UnitTests.Common -{ - [TestFixture] - public class OrgInfoServerTests - { - public static List TestOrgInfo = new List() - { - new OrgInfo() { OrgName = "org1", Platform = "windows", Ring = "fast", Version = "1.2.3.1" }, - new OrgInfo() { OrgName = "org1", Platform = "windows", Ring = "slow", Version = "1.2.3.2" }, - new OrgInfo() { OrgName = "org1", Platform = "macOS", Ring = "fast", Version = "1.2.3.3" }, - new OrgInfo() { OrgName = "org1", Platform = "macOS", Ring = "slow", Version = "1.2.3.4" }, - new OrgInfo() { OrgName = "org2", Platform = "windows", Ring = "fast", Version = "1.2.3.5" }, - new OrgInfo() { OrgName = "org2", Platform = "windows", Ring = "slow", Version = "1.2.3.6" }, - new OrgInfo() { OrgName = "org2", Platform = "macOS", Ring = "fast", Version = "1.2.3.7" }, - new OrgInfo() { OrgName = "org2", Platform = "macOS", Ring = "slow", Version = "1.2.3.8" }, - }; - - private string baseUrl = "https://www.contoso.com"; - - private interface IHttpMessageHandlerProtectedMembers - { - Task SendAsync(HttpRequestMessage message, CancellationToken token); - } - - [TestCaseSource("TestOrgInfo")] - public void QueryNewestVersionWithParams(OrgInfo orgInfo) - { - Mock handlerMock = new Mock(MockBehavior.Strict); - - handlerMock.Protected().As() - .Setup(m => m.SendAsync(It.Is(request => this.UriMatches(request.RequestUri, this.baseUrl, orgInfo.OrgName, orgInfo.Platform, orgInfo.Ring)), It.IsAny())) - .ReturnsAsync(new HttpResponseMessage() - { - StatusCode = HttpStatusCode.OK, - Content = new StringContent(this.ConstructResponseContent(orgInfo.Version)) - }); - - HttpClient httpClient = new HttpClient(handlerMock.Object); - - OrgInfoApiClient upgradeChecker = new OrgInfoApiClient(httpClient, this.baseUrl); - Version version = upgradeChecker.QueryNewestVersion(orgInfo.OrgName, orgInfo.Platform, orgInfo.Ring); - - version.ShouldEqual(new Version(orgInfo.Version)); - - handlerMock.VerifyAll(); - } - - private bool UriMatches(Uri uri, string baseUrl, string expectedOrgName, string expectedPlatform, string expectedRing) - { - bool hostMatches = uri.Host.Equals(baseUrl); - - Dictionary queryParams = new Dictionary(StringComparer.OrdinalIgnoreCase); - foreach (string param in uri.Query.Substring(1).Split('&')) - { - string[] fields = param.Split('='); - string key = fields[0]; - string value = fields[1]; - - queryParams.Add(key, value); - } - - if (queryParams.Count != 3) - { - return false; - } - - if (!queryParams.TryGetValue("Organization", out string orgName) || !string.Equals(orgName, expectedOrgName, StringComparison.OrdinalIgnoreCase)) - { - return false; - } - - if (!queryParams.TryGetValue("platform", out string platform) || !string.Equals(platform, expectedPlatform, StringComparison.OrdinalIgnoreCase)) - { - return false; - } - - if (!queryParams.TryGetValue("ring", out string ring) || !string.Equals(ring, expectedRing, StringComparison.OrdinalIgnoreCase)) - { - return false; - } - - return true; - } - - private string ConstructResponseContent(string version) - { - return $"{{\"version\" : \"{version}\"}} "; - } - - public class OrgInfo - { - public string OrgName { get; set; } - public string Ring { get; set; } - public string Platform { get; set; } - public string Version { get; set; } - } - } -} diff --git a/GVFS/GVFS.UnitTests/Mock/Common/MockLocalGVFSConfigBuilder.cs b/GVFS/GVFS.UnitTests/Mock/Common/MockLocalGVFSConfigBuilder.cs deleted file mode 100644 index 469067513..000000000 --- a/GVFS/GVFS.UnitTests/Mock/Common/MockLocalGVFSConfigBuilder.cs +++ /dev/null @@ -1,91 +0,0 @@ -using GVFS.Common; -using System.Collections.Generic; - -namespace GVFS.UnitTests.Mock.Common -{ - public class MockLocalGVFSConfigBuilder - { - private string defaultRing; - private string defaultUpgradeFeedUrl; - private string defaultUpgradeFeedPackageName; - private string defaultOrgServerUrl; - - private Dictionary entries; - - public MockLocalGVFSConfigBuilder( - string defaultRing, - string defaultUpgradeFeedUrl, - string defaultUpgradeFeedPackageName, - string defaultOrgServerUrl) - { - this.defaultRing = defaultRing; - this.defaultUpgradeFeedUrl = defaultUpgradeFeedUrl; - this.defaultUpgradeFeedPackageName = defaultUpgradeFeedPackageName; - this.defaultOrgServerUrl = defaultOrgServerUrl; - this.entries = new Dictionary(); - } - - public MockLocalGVFSConfigBuilder WithUpgradeRing(string value = null) - { - return this.With(GVFSConstants.LocalGVFSConfig.UpgradeRing, value ?? this.defaultRing); - } - - public MockLocalGVFSConfigBuilder WithNoUpgradeRing() - { - return this.WithNo(GVFSConstants.LocalGVFSConfig.UpgradeRing); - } - - public MockLocalGVFSConfigBuilder WithUpgradeFeedPackageName(string value = null) - { - return this.With(GVFSConstants.LocalGVFSConfig.UpgradeFeedPackageName, value ?? this.defaultUpgradeFeedPackageName); - } - - public MockLocalGVFSConfigBuilder WithNoUpgradeFeedPackageName() - { - return this.WithNo(GVFSConstants.LocalGVFSConfig.UpgradeFeedPackageName); - } - - public MockLocalGVFSConfigBuilder WithUpgradeFeedUrl(string value = null) - { - return this.With(GVFSConstants.LocalGVFSConfig.UpgradeFeedUrl, value ?? this.defaultUpgradeFeedUrl); - } - - public MockLocalGVFSConfigBuilder WithNoUpgradeFeedUrl() - { - return this.WithNo(GVFSConstants.LocalGVFSConfig.UpgradeFeedUrl); - } - - public MockLocalGVFSConfigBuilder WithOrgInfoServerUrl(string value = null) - { - return this.With(GVFSConstants.LocalGVFSConfig.OrgInfoServerUrl, value ?? this.defaultUpgradeFeedUrl); - } - - public MockLocalGVFSConfigBuilder WithNoOrgInfoServerUrl() - { - return this.WithNo(GVFSConstants.LocalGVFSConfig.OrgInfoServerUrl); - } - - public MockLocalGVFSConfig Build() - { - MockLocalGVFSConfig gvfsConfig = new MockLocalGVFSConfig(); - foreach (KeyValuePair kvp in this.entries) - { - gvfsConfig.TrySetConfig(kvp.Key, kvp.Value, out _); - } - - return gvfsConfig; - } - - private MockLocalGVFSConfigBuilder With(string key, string value) - { - this.entries.Add(key, value); - return this; - } - - private MockLocalGVFSConfigBuilder WithNo(string key) - { - this.entries.Remove(key); - return this; - } - } -} From 62c879f2402f99a4543570abb77bc6a8ae23e823 Mon Sep 17 00:00:00 2001 From: Tyrie Vella Date: Tue, 7 Jul 2026 10:48:51 -0700 Subject: [PATCH 14/24] Harden GVFS.Mount against native ProjFS failures under memory pressure (0xC000CE01) GVFS.Mount crashes are being reported that manifest downstream as STATUS_FILE_SYSTEM_VIRTUALIZATION_UNAVAILABLE (0xC000CE01): once the mount process dies, the ProjFS virtualization root is orphaned and every subsequent placeholder access fails (for example GetOfficialBranch/libgit2 reading the virtualized .git). Watson attributes the crashes to radar_high_memory projectedfslib.dll!prjcompletecommand, with the dominant managed signatures being NullReferenceException surfaced from the native ProjFS command-completion (PrjCompleteCommand) and delete (PrjDeleteFile) paths under memory pressure. VFS for Git is already on the latest Microsoft.Windows.ProjFS package, so the native projectedfslib.dll fault cannot be fixed here directly. This is not a v2.0 regression: the unguarded call sites date to the 2018 .NET Framework era. The v2.0 pure-C# ProjFS rewrite made the same latent native fault legible as a managed NRE in GVFS telemetry, which is why reports increased recently. Do not crash the mount on a native ProjFS failure. A single boundary helper, TryInvokeProjFS, wraps each GVFS-initiated native ProjFS call: it catches the exception, emits high-signal *_NativeFailure telemetry (including live enumeration/command counts and GC memory), and returns HResult.InternalError so the caller maps it to a failure result and keeps serving the mount. Routed through it: CompleteCommand (via TryCompleteCommand, covering the completion leg of all async callbacks), DeleteFile, WritePlaceholderFile, WritePlaceholderDirectory, UpdatePlaceholderIfNeeded, and ClearNegativePathCache. MarkDirectoryAsPlaceholder mutates projection state, so swallowing is unsafe; it uses a sibling helper InvokeProjFSOrThrow that emits the same telemetry and then rethrows, preserving fail-fast. GetFileStreamHandlerAsyncHandler already fails a single hydration on any exception, so it is left as-is. When a native CompleteCommand fault is swallowed for a directory-enumeration start, ProjFS never accepted the completion and will not send the matching EndDirectoryEnumeration callback. TryCompleteCommand now reports the command as not completed in that case, so StartDirectoryEnumerationAsyncHandler removes the ActiveEnumeration it just registered instead of leaking it. A companion change to bound the activeEnumerations leak (a suspected contributor to the memory pressure) is deliberately kept out of this stabilization-safe fix and tracked separately behind an off-by-default config flag, so that the enumeration hot path is not altered until telemetry confirms the leak is a real driver. Tests: DeleteFile and the other outbound operations return IOError when the virtualization instance throws; a directory-enumeration start whose native completion faults removes its ActiveEnumeration rather than leaking it. Full unit suite passes. Assisted-by: Claude Opus 4.8 Signed-off-by: Tyrie Vella --- .../ActiveEnumeration.cs | 2 +- .../WindowsFileSystemVirtualizer.cs | 199 ++++++++++++++---- .../Mock/MockVirtualizationInstance.cs | 37 ++++ .../WindowsFileSystemVirtualizerTests.cs | 97 +++++++++ 4 files changed, 289 insertions(+), 46 deletions(-) diff --git a/GVFS/GVFS.Platform.Windows/ActiveEnumeration.cs b/GVFS/GVFS.Platform.Windows/ActiveEnumeration.cs index 0baa27a4c..2c3222e7b 100644 --- a/GVFS/GVFS.Platform.Windows/ActiveEnumeration.cs +++ b/GVFS/GVFS.Platform.Windows/ActiveEnumeration.cs @@ -1,4 +1,4 @@ -using GVFS.Common; +using GVFS.Common; using GVFS.Virtualization.Projection; using Microsoft.Windows.ProjFS; using System.Collections.Generic; diff --git a/GVFS/GVFS.Platform.Windows/WindowsFileSystemVirtualizer.cs b/GVFS/GVFS.Platform.Windows/WindowsFileSystemVirtualizer.cs index f39128e7a..1a14ef944 100644 --- a/GVFS/GVFS.Platform.Windows/WindowsFileSystemVirtualizer.cs +++ b/GVFS/GVFS.Platform.Windows/WindowsFileSystemVirtualizer.cs @@ -119,14 +119,21 @@ public override void Stop() public override FileSystemResult ClearNegativePathCache(out uint totalEntryCount) { - HResult result = this.virtualizationInstance.ClearNegativePathCache(out totalEntryCount); + uint entryCount = 0; + HResult result = this.TryInvokeProjFS( + () => this.virtualizationInstance.ClearNegativePathCache(out entryCount), + nameof(this.ClearNegativePathCache)); + totalEntryCount = entryCount; return new FileSystemResult(HResultToFSResult(result), unchecked((int)result)); } public override FileSystemResult DeleteFile(string relativePath, UpdatePlaceholderType updateFlags, out UpdateFailureReason failureReason) { UpdateFailureCause failureCause = UpdateFailureCause.NoFailure; - HResult result = this.virtualizationInstance.DeleteFile(relativePath, (UpdateType)updateFlags, out failureCause); + HResult result = this.TryInvokeProjFS( + () => this.virtualizationInstance.DeleteFile(relativePath, (UpdateType)updateFlags, out failureCause), + nameof(this.DeleteFile), + relativePath); failureReason = (UpdateFailureReason)failureCause; return new FileSystemResult(HResultToFSResult(result), unchecked((int)result)); } @@ -137,17 +144,20 @@ public override FileSystemResult WritePlaceholderFile( string sha) { FileProperties properties = this.FileSystemCallbacks.GetLogsHeadFileProperties(); - HResult result = this.virtualizationInstance.WritePlaceholderInfo( - relativePath, - properties.CreationTimeUTC, - properties.LastAccessTimeUTC, - properties.LastWriteTimeUTC, - changeTime: properties.LastWriteTimeUTC, - fileAttributes: FileAttributes.Archive, - endOfFile: endOfFile, - isDirectory: false, - contentId: FileSystemVirtualizer.ConvertShaToContentId(sha), - providerId: PlaceholderVersionId); + HResult result = this.TryInvokeProjFS( + () => this.virtualizationInstance.WritePlaceholderInfo( + relativePath, + properties.CreationTimeUTC, + properties.LastAccessTimeUTC, + properties.LastWriteTimeUTC, + changeTime: properties.LastWriteTimeUTC, + fileAttributes: FileAttributes.Archive, + endOfFile: endOfFile, + isDirectory: false, + contentId: FileSystemVirtualizer.ConvertShaToContentId(sha), + providerId: PlaceholderVersionId), + nameof(this.WritePlaceholderFile), + relativePath); return new FileSystemResult(HResultToFSResult(result), unchecked((int)result)); } @@ -155,17 +165,20 @@ public override FileSystemResult WritePlaceholderFile( public override FileSystemResult WritePlaceholderDirectory(string relativePath) { FileProperties properties = this.FileSystemCallbacks.GetLogsHeadFileProperties(); - HResult result = this.virtualizationInstance.WritePlaceholderInfo( - relativePath, - properties.CreationTimeUTC, - properties.LastAccessTimeUTC, - properties.LastWriteTimeUTC, - changeTime: properties.LastWriteTimeUTC, - fileAttributes: FileAttributes.Directory, - endOfFile: 0, - isDirectory: true, - contentId: FolderContentId, - providerId: PlaceholderVersionId); + HResult result = this.TryInvokeProjFS( + () => this.virtualizationInstance.WritePlaceholderInfo( + relativePath, + properties.CreationTimeUTC, + properties.LastAccessTimeUTC, + properties.LastWriteTimeUTC, + changeTime: properties.LastWriteTimeUTC, + fileAttributes: FileAttributes.Directory, + endOfFile: 0, + isDirectory: true, + contentId: FolderContentId, + providerId: PlaceholderVersionId), + nameof(this.WritePlaceholderDirectory), + relativePath); return new FileSystemResult(HResultToFSResult(result), unchecked((int)result)); } @@ -183,18 +196,21 @@ public override FileSystemResult UpdatePlaceholderIfNeeded( out UpdateFailureReason failureReason) { UpdateFailureCause failureCause = UpdateFailureCause.NoFailure; - HResult result = this.virtualizationInstance.UpdateFileIfNeeded( - relativePath, - creationTime, - lastAccessTime, - lastWriteTime, - changeTime, - fileAttributes, - endOfFile, - ConvertShaToContentId(shaContentId), - PlaceholderVersionId, - (UpdateType)updateFlags, - out failureCause); + HResult result = this.TryInvokeProjFS( + () => this.virtualizationInstance.UpdateFileIfNeeded( + relativePath, + creationTime, + lastAccessTime, + lastWriteTime, + changeTime, + fileAttributes, + endOfFile, + ConvertShaToContentId(shaContentId), + PlaceholderVersionId, + (UpdateType)updateFlags, + out failureCause), + nameof(this.UpdatePlaceholderIfNeeded), + relativePath); failureReason = (UpdateFailureReason)failureCause; return new FileSystemResult(HResultToFSResult(result), unchecked((int)result)); } @@ -652,13 +668,101 @@ private bool TryCompleteCommand(int commandId, HResult result) CancellationTokenSource cancellationSource; if (this.activeCommands.TryRemove(commandId, out cancellationSource)) { - this.virtualizationInstance.CompleteCommand(commandId, result); - return true; + // Track whether the native completion actually ran. If CompleteCommand faults under + // memory pressure, TryInvokeProjFS swallows the exception so the mount survives, but + // ProjFS never accepted the completion and so will not drive the matching + // EndDirectoryEnumeration callback. In that case we must report the command as not + // completed so callers clean up any state they registered (for example the active + // enumeration) instead of leaking it. + bool completionHandedOffToProjFS = false; + this.TryInvokeProjFS( + () => + { + HResult completionResult = this.virtualizationInstance.CompleteCommand(commandId, result); + completionHandedOffToProjFS = true; + return completionResult; + }, + nameof(this.TryCompleteCommand), + addDetails: metadata => + { + metadata.Add("commandId", commandId); + metadata.Add("completionResult", result.ToString("X") + "(" + result.ToString("G") + ")"); + }); + + return completionHandedOffToProjFS; } return false; } + private void AddMemoryPressureData(EventMetadata metadata) + { + metadata.Add("activeEnumerations", this.activeEnumerations.Count); + metadata.Add("activeCommands", this.activeCommands.Count); + metadata.Add("gcTotalMemoryBytes", GC.GetTotalMemory(forceFullCollection: false)); + } + + /// + /// Invokes a GVFS-initiated native ProjFS operation, converting a native failure into a + /// failed instead of a process crash. + /// + /// + /// A native ProjFS call can fault under memory pressure (for example a NullReferenceException + /// surfaced from projectedfslib.dll). If that exception escaped, it would tear down + /// GVFS.Mount, orphaning the virtualization root and causing every subsequent placeholder + /// access to fail with STATUS_FILE_SYSTEM_VIRTUALIZATION_UNAVAILABLE (0xC000CE01). Callers + /// map the returned non-Ok HResult to a failure result and continue serving the mount. + /// + /// This is used for outbound operations that are safe to fail individually. Operations that + /// mutate projection state use instead, which logs the same + /// telemetry but rethrows. Callback completions funnel through + /// (which uses this guard); the file-stream handler already fails a single hydration on any + /// exception. + /// + private HResult TryInvokeProjFS(Func nativeCall, string operationName, string relativePath = null, Action addDetails = null) + { + try + { + return nativeCall(); + } + catch (Exception e) + { + this.LogProjFSNativeFailure(operationName, e, relativePath, addDetails); + return HResult.InternalError; + } + } + + /// + /// Invokes a GVFS-initiated native ProjFS operation for which swallowing a failure is unsafe + /// (for example because it mutates projection state). Emits the same high-signal + /// *_NativeFailure telemetry as for the memory-pressure + /// diagnostics, then rethrows so the mount fails fast rather than continuing in an + /// inconsistent state. + /// + private HResult InvokeProjFSOrThrow(Func nativeCall, string operationName, string relativePath = null, Action addDetails = null) + { + try + { + return nativeCall(); + } + catch (Exception e) + { + this.LogProjFSNativeFailure(operationName, e, relativePath, addDetails); + throw; + } + } + + private void LogProjFSNativeFailure(string operationName, Exception e, string relativePath, Action addDetails) + { + EventMetadata metadata = this.CreateEventMetadata(relativePath, e); + addDetails?.Invoke(metadata); + this.AddMemoryPressureData(metadata); + metadata.Add( + TracingConstants.MessageKey.ErrorMessage, + operationName + ": native ProjFS call threw under suspected memory pressure"); + this.Context.Tracer.RelatedError(metadata, operationName + "_NativeFailure", Keywords.Telemetry); + } + private void StartDirectoryEnumerationAsyncHandler( CancellationToken cancellationToken, BlobSizes.BlobSizesConnection blobSizesConnection, @@ -720,10 +824,12 @@ private void StartDirectoryEnumerationAsyncHandler( if (!this.TryCompleteCommand(commandId, result)) { - // Command has already been canceled, and no EndDirectoryEnumeration callback will be received + // Either the command was already canceled or the native completion failed under + // memory pressure; in both cases no EndDirectoryEnumeration callback will arrive, so + // remove the enumeration we just registered rather than leaking it. EventMetadata metadata = this.CreateEventMetadata(virtualPath); - metadata.Add(TracingConstants.MessageKey.InfoMessage, $"{nameof(this.StartDirectoryEnumerationAsyncHandler)}: TryCompleteCommand returned false, command already canceled"); + metadata.Add(TracingConstants.MessageKey.InfoMessage, $"{nameof(this.StartDirectoryEnumerationAsyncHandler)}: TryCompleteCommand returned false; command canceled or native completion failed"); metadata.Add("commandId", commandId); metadata.Add("enumerationId", enumerationId); metadata.Add(nameof(result), result.ToString("X") + "(" + result.ToString("G") + ")"); @@ -731,7 +837,7 @@ private void StartDirectoryEnumerationAsyncHandler( ActiveEnumeration activeEnumeration; bool activeEnumerationsUpdated = this.activeEnumerations.TryRemove(enumerationId, out activeEnumeration); metadata.Add("activeEnumerationsUpdated", activeEnumerationsUpdated); - this.Context.Tracer.RelatedEvent(EventLevel.Informational, $"{nameof(this.StartDirectoryEnumerationAsyncHandler)}_CommandAlreadyCanceled", metadata); + this.Context.Tracer.RelatedEvent(EventLevel.Informational, $"{nameof(this.StartDirectoryEnumerationAsyncHandler)}_CommandNotCompleted", metadata); } } @@ -1070,10 +1176,13 @@ private void MarkDirectoryAsPlaceholder( string triggeringProcessImageFileName) { string directoryPath = Path.Combine(this.Context.Enlistment.WorkingDirectoryRoot, virtualPath); - HResult hr = this.virtualizationInstance.MarkDirectoryAsPlaceholder( - directoryPath, - FolderContentId, - PlaceholderVersionId); + HResult hr = this.InvokeProjFSOrThrow( + () => this.virtualizationInstance.MarkDirectoryAsPlaceholder( + directoryPath, + FolderContentId, + PlaceholderVersionId), + nameof(this.MarkDirectoryAsPlaceholder), + virtualPath); if (hr == HResult.Ok) { diff --git a/GVFS/GVFS.UnitTests/Windows/Mock/MockVirtualizationInstance.cs b/GVFS/GVFS.UnitTests/Windows/Mock/MockVirtualizationInstance.cs index 16659ddda..95147d9bb 100644 --- a/GVFS/GVFS.UnitTests/Windows/Mock/MockVirtualizationInstance.cs +++ b/GVFS/GVFS.UnitTests/Windows/Mock/MockVirtualizationInstance.cs @@ -62,6 +62,18 @@ public HResult WriteFileReturnResult public HResult DeleteFileResult { get; set; } public UpdateFailureCause DeleteFileUpdateFailureCause { get; set; } + // When set, DeleteFile throws this to simulate a native ProjFS failure (e.g. a + // NullReferenceException surfaced from projectedfslib.dll under memory pressure). + public Exception DeleteFileException { get; set; } + + // When set, the other GVFS-initiated native operations (ClearNegativePathCache, + // WritePlaceholderInfo, UpdateFileIfNeeded) throw this to simulate a native ProjFS failure. + public Exception NativeCallException { get; set; } + + // When set, CompleteCommand throws this to simulate the native ProjFS completion faulting + // under memory pressure (e.g. a NullReferenceException surfaced from projectedfslib.dll). + public Exception CompleteCommandException { get; set; } + public HResult UpdateFileIfNeededResult { get; set; } public UpdateFailureCause UpdateFileIfNeededFailureCase { get; set; } @@ -82,6 +94,11 @@ public HResult DetachDriver() public HResult ClearNegativePathCache(out uint totalEntryNumber) { + if (this.NativeCallException != null) + { + throw this.NativeCallException; + } + totalEntryNumber = this.NegativePathCacheCount; this.NegativePathCacheCount = 0; return HResult.Ok; @@ -89,12 +106,22 @@ public HResult ClearNegativePathCache(out uint totalEntryNumber) public HResult DeleteFile(string relativePath, UpdateType updateFlags, out UpdateFailureCause failureReason) { + if (this.DeleteFileException != null) + { + throw this.DeleteFileException; + } + failureReason = this.DeleteFileUpdateFailureCause; return this.DeleteFileResult; } public HResult UpdateFileIfNeeded(string relativePath, DateTime creationTime, DateTime lastAccessTime, DateTime lastWriteTime, DateTime changeTime, FileAttributes fileAttributes, long endOfFile, byte[] contentId, byte[] providerId, UpdateType updateFlags, out UpdateFailureCause failureReason) { + if (this.NativeCallException != null) + { + throw this.NativeCallException; + } + failureReason = this.UpdateFileIfNeededFailureCase; return this.UpdateFileIfNeededResult; } @@ -121,6 +148,11 @@ public HResult WritePlaceholderInfo( byte[] contentId, byte[] epochId) { + if (this.NativeCallException != null) + { + throw this.NativeCallException; + } + this.CreatedPlaceholders.Add(relativePath); this.placeholderCreated.Set(); return HResult.Ok; @@ -169,6 +201,11 @@ public HResult CompleteCommand(int commandId, IDirectoryEnumerationResults resul public HResult CompleteCommand(int commandId, HResult completionResult) { + if (this.CompleteCommandException != null) + { + throw this.CompleteCommandException; + } + this.completionResult = completionResult; this.commandCompleted.Set(); return HResult.Ok; diff --git a/GVFS/GVFS.UnitTests/Windows/Virtualization/WindowsFileSystemVirtualizerTests.cs b/GVFS/GVFS.UnitTests/Windows/Virtualization/WindowsFileSystemVirtualizerTests.cs index 5ca3f4115..caf6f884a 100644 --- a/GVFS/GVFS.UnitTests/Windows/Virtualization/WindowsFileSystemVirtualizerTests.cs +++ b/GVFS/GVFS.UnitTests/Windows/Virtualization/WindowsFileSystemVirtualizerTests.cs @@ -95,6 +95,75 @@ public void DeleteFile() } } + [TestCase] + public void DeleteFileReturnsIOErrorWhenVirtualizationInstanceThrows() + { + using (MockVirtualizationInstance mockVirtualization = new MockVirtualizationInstance()) + using (WindowsFileSystemVirtualizer virtualizer = new WindowsFileSystemVirtualizer(this.Repo.Context, this.Repo.GitObjects, mockVirtualization, numWorkThreads)) + { + // A native ProjFS failure (for example a NullReferenceException surfaced from + // projectedfslib.dll under memory pressure) must fail this single delete rather than + // propagate out and crash the mount. + mockVirtualization.DeleteFileException = new NullReferenceException("simulated native ProjFS failure"); + + UpdateFailureReason failureReason = UpdateFailureReason.DirtyData; + virtualizer + .DeleteFile("test.txt", UpdatePlaceholderType.AllowReadOnly, out failureReason) + .ShouldEqual(new FileSystemResult(FSResult.IOError, (int)HResult.InternalError)); + failureReason.ShouldEqual(UpdateFailureReason.NoFailure); + } + } + + [TestCase] + public void OutboundProjFSOperationsReturnFailureWhenVirtualizationInstanceThrows() + { + using (MockVirtualizationInstance mockVirtualization = new MockVirtualizationInstance()) + using (WindowsFileSystemVirtualizer virtualizer = new WindowsFileSystemVirtualizer(this.Repo.Context, this.Repo.GitObjects, mockVirtualization, numWorkThreads)) + { + // Every GVFS-initiated native ProjFS operation must fail the single call rather than + // propagate a native exception (e.g. a NullReferenceException from projectedfslib.dll + // under memory pressure) and crash the mount. + mockVirtualization.NativeCallException = new NullReferenceException("simulated native ProjFS failure"); + + virtualizer + .ClearNegativePathCache(out uint _) + .ShouldEqual(new FileSystemResult(FSResult.IOError, (int)HResult.InternalError)); + + UpdateFailureReason failureReason = UpdateFailureReason.DirtyData; + virtualizer + .UpdatePlaceholderIfNeeded( + "test.txt", + DateTime.Now, + DateTime.Now, + DateTime.Now, + DateTime.Now, + 0, + 15, + string.Empty, + UpdatePlaceholderType.AllowReadOnly, + out failureReason) + .ShouldEqual(new FileSystemResult(FSResult.IOError, (int)HResult.InternalError)); + failureReason.ShouldEqual(UpdateFailureReason.NoFailure); + } + } + + [TestCase] + public void WritePlaceholderOperationsReturnFailureWhenVirtualizationInstanceThrows() + { + using (WindowsFileSystemVirtualizerTester tester = new WindowsFileSystemVirtualizerTester(this.Repo)) + { + tester.MockVirtualization.NativeCallException = new NullReferenceException("simulated native ProjFS failure"); + + tester.WindowsVirtualizer + .WritePlaceholderFile("test.txt", endOfFile: 15, sha: new string('0', 40)) + .ShouldEqual(new FileSystemResult(FSResult.IOError, (int)HResult.InternalError)); + + tester.WindowsVirtualizer + .WritePlaceholderDirectory("testDir") + .ShouldEqual(new FileSystemResult(FSResult.IOError, (int)HResult.InternalError)); + } + } + [TestCase] public void UpdatePlaceholderIfNeeded() { @@ -473,5 +542,33 @@ public void OnGetFileStreamHandlesHResultHandleResult() mockTracker.RelatedErrorEvents.ShouldBeEmpty(); } } + + [TestCase] + public void OnStartDirectoryEnumerationCleansUpEnumerationWhenNativeCompletionFails() + { + using (WindowsFileSystemVirtualizerTester tester = new WindowsFileSystemVirtualizerTester(this.Repo)) + { + MockTracer mockTracker = this.Repo.Context.Tracer as MockTracer; + mockTracker.WaitRelatedEventName = "StartDirectoryEnumerationAsyncHandler_CommandNotCompleted"; + + // Simulate the native ProjFS completion faulting under memory pressure. TryInvokeProjFS + // must swallow the exception so the mount survives, and because ProjFS never accepted the + // completion no EndDirectoryEnumeration callback will arrive. The enumeration registered by + // the async handler must therefore be removed here rather than leaked. + tester.MockVirtualization.CompleteCommandException = new NullReferenceException("simulated native ProjFS failure"); + + Guid enumerationGuid = Guid.NewGuid(); + tester.GitIndexProjection.EnumerationInMemory = false; + tester.MockVirtualization.RequiredCallbacks.StartDirectoryEnumerationCallback(1, enumerationGuid, "test", TriggeringProcessId, TriggeringProcessImageFileName).ShouldEqual(HResult.Pending); + + // Deterministically wait for the async handler to reach its cleanup path (the enumeration is + // removed immediately before this event is emitted). If the native failure were reported as a + // successful completion, this event would never fire. + mockTracker.WaitForRelatedEvent(); + + // The enumeration must already have been removed, so End cannot find it. + tester.MockVirtualization.RequiredCallbacks.EndDirectoryEnumerationCallback(enumerationGuid).ShouldEqual(HResult.InternalError); + } + } } } From 646346c67caba7b251ae5886681c103882ed19aa Mon Sep 17 00:00:00 2001 From: Tyrie Vella Date: Fri, 10 Jul 2026 09:55:46 -0700 Subject: [PATCH 15/24] Split ServiceVerb repo-state check into intent-specific predicates `gvfs service --unmount-all` intermittently failed with exit code 3 and "Already unmounting, please wait" when the service registry still listed a repo whose GVFS.Mount process was in the transient Unmounting state (a concurrent or prior unmount still shutting it down). The single IsRepoMounted helper returned true whenever the mount pipe merely answered a GetStatus request, regardless of the actual MountState, so --unmount-all would attempt to unmount a repo already on its way out and treat that transient state as a hard failure. Replace IsRepoMounted with three intent-specific predicates built on a TryGetRepoMountStatus primitive that reads the actual MountState: - IsRepoReady (Ready) -> --list-mounted - IsRepoAvailableToMount (no live process) -> --mount-all - IsRepoAvailableToUnmount (Ready|MountFailed) -> --unmount-all --unmount-all now skips repos that are Unmounting (already reaching the desired state) or Mounting (an unmount request would be rejected anyway), eliminating the spurious failure. --list-mounted now reports only fully-Ready repos. --mount-all behavior is unchanged (it still mounts only when no live mount process is answering). StatusVerb exposes the "Mount status: " output prefix as a shared constant so ServiceVerb recovers the MountState without re-implementing the pipe protocol. Fixes the flaky GVFS.FunctionalTests.Tests.MultiEnlistmentTests.ServiceVerbTests.ServiceCommandsWithNoRepos. Assisted-by: Claude Opus 4.8 Signed-off-by: Tyrie Vella --- GVFS/GVFS/CommandLine/ServiceVerb.cs | 50 ++++++++++++++++++++++++---- GVFS/GVFS/CommandLine/StatusVerb.cs | 6 +++- 2 files changed, 49 insertions(+), 7 deletions(-) diff --git a/GVFS/GVFS/CommandLine/ServiceVerb.cs b/GVFS/GVFS/CommandLine/ServiceVerb.cs index 842521fa7..0741aa40e 100644 --- a/GVFS/GVFS/CommandLine/ServiceVerb.cs +++ b/GVFS/GVFS/CommandLine/ServiceVerb.cs @@ -73,7 +73,7 @@ public override void Execute() { foreach (string repoRoot in repoList) { - if (this.IsRepoMounted(repoRoot)) + if (this.IsRepoReady(repoRoot)) { this.Output.WriteLine(repoRoot); } @@ -93,7 +93,7 @@ public override void Execute() foreach (string repoRoot in repoList) { - if (!this.IsRepoMounted(repoRoot)) + if (this.IsRepoAvailableToMount(repoRoot)) { this.Output.WriteLine("\r\nMounting repo at " + repoRoot); ReturnCode result = this.Execute(repoRoot); @@ -118,7 +118,7 @@ public override void Execute() foreach (string repoRoot in repoList) { - if (this.IsRepoMounted(repoRoot)) + if (this.IsRepoAvailableToUnmount(repoRoot)) { this.Output.WriteLine("\r\nUnmounting repo at " + repoRoot); ReturnCode result = this.Execute( @@ -199,8 +199,35 @@ private bool TryGetRepoList(out List repoList, out string errorMessage) } } - private bool IsRepoMounted(string repoRoot) + private bool IsRepoReady(string repoRoot) { + // For --list-mounted: only a repo whose mount is fully Ready is reported as mounted. + return this.TryGetRepoMountStatus(repoRoot, out string mountStatus) + && mountStatus.Equals(NamedPipeMessages.GetStatus.Ready, StringComparison.Ordinal); + } + + private bool IsRepoAvailableToMount(string repoRoot) + { + // For --mount-all: a repo can be mounted only when no live mount process is + // answering for it (i.e. it is not already Mounting, Ready, Unmounting, or MountFailed). + return !this.TryGetRepoMountStatus(repoRoot, out _); + } + + private bool IsRepoAvailableToUnmount(string repoRoot) + { + // For --unmount-all: only unmount a repo whose mount is in a state that accepts an + // unmount request. A repo already Unmounting is reaching the desired state on its own, + // and one still Mounting rejects the request, so both are skipped to avoid spurious + // "Already unmounting" / "not mounted" failures during concurrent teardown. + return this.TryGetRepoMountStatus(repoRoot, out string mountStatus) + && (mountStatus.Equals(NamedPipeMessages.GetStatus.Ready, StringComparison.Ordinal) + || mountStatus.Equals(NamedPipeMessages.GetStatus.MountFailed, StringComparison.Ordinal)); + } + + private bool TryGetRepoMountStatus(string repoRoot, out string mountStatus) + { + mountStatus = null; + // Hide the output of status StringWriter statusOutput = new StringWriter(); ReturnCode result = this.Execute( @@ -210,9 +237,20 @@ private bool IsRepoMounted(string repoRoot) verb.Output = statusOutput; }); - if (result == ReturnCode.Success) + if (result != ReturnCode.Success) + { + // No live mount process is answering for this repo. + return false; + } + + foreach (string line in statusOutput.ToString().Split('\n')) { - return true; + string trimmedLine = line.Trim(); + if (trimmedLine.StartsWith(StatusVerb.MountStatusOutputPrefix, StringComparison.Ordinal)) + { + mountStatus = trimmedLine.Substring(StatusVerb.MountStatusOutputPrefix.Length).Trim(); + return true; + } } return false; diff --git a/GVFS/GVFS/CommandLine/StatusVerb.cs b/GVFS/GVFS/CommandLine/StatusVerb.cs index ead87842b..569a625e5 100644 --- a/GVFS/GVFS/CommandLine/StatusVerb.cs +++ b/GVFS/GVFS/CommandLine/StatusVerb.cs @@ -23,6 +23,10 @@ public static System.CommandLine.Command CreateCommand() private const string StatusVerbName = "status"; + // Prefix used when writing the mount status line to output. ServiceVerb parses + // this to recover a repo's MountState without re-implementing the pipe protocol. + public const string MountStatusOutputPrefix = "Mount status: "; + protected override string VerbName { get { return StatusVerbName; } @@ -47,7 +51,7 @@ protected override void Execute(GVFSEnlistment enlistment) this.Output.WriteLine("Repo URL: " + getStatusResponse.RepoUrl); this.Output.WriteLine("Cache Server: " + getStatusResponse.CacheServer); this.Output.WriteLine("Local Cache: " + getStatusResponse.LocalCacheRoot); - this.Output.WriteLine("Mount status: " + getStatusResponse.MountStatus); + this.Output.WriteLine(MountStatusOutputPrefix + getStatusResponse.MountStatus); this.Output.WriteLine("GVFS Lock: " + getStatusResponse.LockStatus); this.Output.WriteLine("Background operations: " + getStatusResponse.BackgroundOperationCount); this.Output.WriteLine("Disk layout version: " + getStatusResponse.DiskLayoutVersion); From b84ed5280fadcafa3193880092e1c7f5d25cfb5a Mon Sep 17 00:00:00 2001 From: Tyrie Vella Date: Fri, 10 Jul 2026 10:53:28 -0700 Subject: [PATCH 16/24] Harden orphan-lock detection against false-release on transient failures PR #1989 fixed a PID-recycling race in orphan-lock detection by capturing the holder's process start time at acquire and comparing it at each orphan check. That fix is correct for the recycling case, but it collapsed every failure to read the start time into a single bool "false" that the caller treats as "the holder is gone" and releases the lock. The dangerous consequence is on the transient-failure path. The Windows implementation returns false the moment OpenProcess(QUERY_LIMITED_INFORMATION) yields an invalid handle. Under momentary resource pressure that OpenProcess can fail for a process that is genuinely still alive, so the orphan check can release a lock that is still legitimately held -- admitting a second writer to the index. That is a worse failure class (potential corruption) than the flaky hang #1989 set out to fix, which is exactly the kind of regression a stabilization release must not ship. (Note the pre-#1989 code did not have this exposure: its liveness check fell back to Process.GetProcessById when OpenProcess failed; #1989 dropped that fallback on the identity path.) This change makes the release decision reason-aware so that a lock is only released on positive evidence that the holder is gone: * TryGetActiveProcessStartTime now returns a ProcessStartTimeResult enum (Success / ProcessNotFound / Inaccessible / Indeterminate) instead of bool. The Windows implementation classifies an invalid OpenProcess handle by the Win32 error: ERROR_INVALID_PARAMETER -> ProcessNotFound (no such PID), ERROR_ACCESS_DENIED -> Inaccessible, anything else (e.g. ERROR_NOT_ENOUGH_MEMORY / ERROR_NO_SYSTEM_RESOURCES) -> Indeterminate. An opened-but-exited process (exit code != STILL_ACTIVE) maps to ProcessNotFound. * The orphan check in GVFSLock releases the lock only for: - Success with a different start time (PID recycled -- the #1989 case) - ProcessNotFound (positive: holder is gone) - Inaccessible (see gate argument below) and deliberately KEEPS the lock for Indeterminate, letting the existing 250 ms wait-loop poll re-evaluate. A transient read failure can no longer release a live holder's lock. * Inaccessible is safe to treat as "holder gone" because of an acquire-time gate: we only enter the identity-check path for a holder whose start time we successfully read at acquire, i.e. one this mount could open. OpenProcess access is a stable function of the caller token and the target's protection level / DACL (git and the hooks never rewrite their own DACL), so a live original holder we could open before cannot later become inaccessible. An access-denied result therefore means the PID now refers to a different process, so releasing is correct. The pre-existing null-start-time fallback (used when we could not read the start time at acquire) is unchanged. Telemetry: the Indeterminate hold emits an ExternalHolderLivenessIndeterminate event so we can measure whether these transient failures ever occur in the field. This is in-memory mount-side state only; the named-pipe lock protocol is unchanged, so there is no cross-version or wire-format impact. Tests: adds unit coverage for the new outcomes -- ProcessNotFound and Inaccessible each release the orphaned lock, and Indeterminate keeps it (the anti-false-release guard). Existing PID-recycle and start-time-match tests are retained. Full GVFS.UnitTests suite passes (885 passed, 0 failed). Assisted-by: Claude Opus 4.7 Signed-off-by: Tyrie Vella --- GVFS/GVFS.Common/GVFSLock.cs | 60 +++++++-- GVFS/GVFS.Common/GVFSPlatform.cs | 23 ++-- GVFS/GVFS.Common/ProcessStartTimeResult.cs | 38 ++++++ GVFS/GVFS.Hooks/GVFS.Hooks.csproj | 3 + .../WindowsPlatform.Shared.cs | 57 +++++++-- GVFS/GVFS.Platform.Windows/WindowsPlatform.cs | 2 +- GVFS/GVFS.UnitTests/Common/GVFSLockTests.cs | 119 ++++++++++++++++++ .../Mock/Common/MockPlatform.cs | 27 +++- 8 files changed, 291 insertions(+), 38 deletions(-) create mode 100644 GVFS/GVFS.Common/ProcessStartTimeResult.cs diff --git a/GVFS/GVFS.Common/GVFSLock.cs b/GVFS/GVFS.Common/GVFSLock.cs index c8cac11ff..9cc680fb0 100644 --- a/GVFS/GVFS.Common/GVFSLock.cs +++ b/GVFS/GVFS.Common/GVFSLock.cs @@ -47,7 +47,8 @@ public bool TryAcquireLockForExternalRequestor( // 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) + long? requestorStartTime = + GVFSPlatform.Instance.TryGetActiveProcessStartTime(requestor.PID, out long startTime) == ProcessStartTimeResult.Success ? startTime : (long?)null; if (requestorStartTime == null) @@ -215,6 +216,17 @@ private bool IsLockAvailable(bool checkExternalHolderOnly, out NamedPipeMessages this.tracer.SetGitCommandSessionId(string.Empty); existingExternalHolder = null; } + else if (terminationReason == "StartTimeIndeterminate" && existingExternalHolder != null) + { + // We could not determine whether the holder is still alive, so we deliberately + // kept the lock rather than risk releasing one that is still held. Record it so + // we can see whether these transient failures ever happen in the field (and, + // if a holder ever gets stuck this way, why the lock was not released). + EventMetadata metadata = new EventMetadata(); + metadata.Add("CurrentLockHolder", existingExternalHolder.ToString()); + metadata.Add("Reason", terminationReason); + this.tracer.RelatedEvent(EventLevel.Verbose, "ExternalHolderLivenessIndeterminate", metadata); + } return existingExternalHolder == null; } @@ -460,17 +472,43 @@ public NamedPipeMessages.LockData GetExternalHolder(out bool externalHolderTermi { // 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)) + // current one. We only conclude the holder is gone (and release its lock) + // when we have positive evidence, so that a transient failure to read the + // start time can never release a lock that is still legitimately held. + ProcessStartTimeResult result = GVFSPlatform.Instance.TryGetActiveProcessStartTime(pid, out long currentStartTime); + switch (result) { - externalHolderTerminatedWithoutReleasingLock = true; - terminationReason = "ProcessNotActive"; - } - else if (currentStartTime != capturedStartTime) - { - externalHolderTerminatedWithoutReleasingLock = true; - terminationReason = "PidRecycled"; + case ProcessStartTimeResult.Success when currentStartTime == capturedStartTime: + // Same process, still alive. Keep the lock. + break; + + case ProcessStartTimeResult.Success: + // A different, still-running process now owns this PID: the original + // holder exited and Windows recycled the PID (the bug this fixes). + externalHolderTerminatedWithoutReleasingLock = true; + terminationReason = "PidRecycled"; + break; + + case ProcessStartTimeResult.ProcessNotFound: + // Positive evidence the holder is gone (no such PID, or it exited). + externalHolderTerminatedWithoutReleasingLock = true; + terminationReason = "ProcessExited"; + break; + + case ProcessStartTimeResult.Inaccessible: + // We could open the holder at acquire time, and that access is stable + // for the process's lifetime, so an access-denied result now means the + // PID refers to a different process: the original holder is gone. + externalHolderTerminatedWithoutReleasingLock = true; + terminationReason = "HolderInaccessible"; + break; + + case ProcessStartTimeResult.Indeterminate: + default: + // We could not determine liveness (e.g. transient resource error). + // Do NOT release: keep the lock and let the next poll re-evaluate. + terminationReason = "StartTimeIndeterminate"; + break; } } else diff --git a/GVFS/GVFS.Common/GVFSPlatform.cs b/GVFS/GVFS.Common/GVFSPlatform.cs index 8935c72b6..5b232628a 100644 --- a/GVFS/GVFS.Common/GVFSPlatform.cs +++ b/GVFS/GVFS.Common/GVFSPlatform.cs @@ -68,16 +68,21 @@ public static void Register(GVFSPlatform platform) 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. + /// Attempts to read an opaque, OS-supplied process-identity token (e.g. process creation + /// time on Windows) for the process with the given PID. 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. /// - public abstract bool TryGetActiveProcessStartTime(int processId, out long startTime); + /// The PID to inspect. + /// The identity token, valid only when the result is . + /// + /// A describing the outcome. Callers making a + /// release-versus-hold decision about a lock holder must only treat + /// and + /// as evidence the original holder is gone; + /// means "unknown" and must not trigger a release. + /// + public abstract ProcessStartTimeResult TryGetActiveProcessStartTime(int processId, out long startTime); public abstract void IsServiceInstalledAndRunning(string name, out bool installed, out bool running); public abstract string GetNamedPipeName(string enlistmentRoot); diff --git a/GVFS/GVFS.Common/ProcessStartTimeResult.cs b/GVFS/GVFS.Common/ProcessStartTimeResult.cs new file mode 100644 index 000000000..dbb1c6fa6 --- /dev/null +++ b/GVFS/GVFS.Common/ProcessStartTimeResult.cs @@ -0,0 +1,38 @@ +namespace GVFS.Common +{ + /// + /// Outcome of attempting to read a process's start time for lock-holder identity checks. + /// The distinction between the failure values matters: the orphan-lock detector must only + /// release a lock when it has positive evidence that the original holder is gone, so it + /// treats "we could not determine anything" () differently from + /// evidence that the holder has exited or been replaced. + /// + public enum ProcessStartTimeResult + { + /// + /// The process is active and its start time was read successfully. The out start time is valid. + /// + Success, + + /// + /// Positive evidence that no live process with the requested identity exists: either there is + /// no process at that PID, or a process was opened but has already exited. Safe to treat the + /// original holder as gone. + /// + ProcessNotFound, + + /// + /// A process exists at the PID but we could not open it (access denied). Because we only + /// identity-track holders we successfully opened at acquire time (and that access relationship + /// is stable for a given process's lifetime), an access-denied result here means the PID now + /// refers to a different process than the original holder. + /// + Inaccessible, + + /// + /// A transient or unclassified failure (e.g. resource exhaustion). We genuinely do not know + /// whether the original holder is alive, so callers must NOT treat this as evidence of exit. + /// + Indeterminate, + } +} diff --git a/GVFS/GVFS.Hooks/GVFS.Hooks.csproj b/GVFS/GVFS.Hooks/GVFS.Hooks.csproj index c960ac430..69988ac80 100644 --- a/GVFS/GVFS.Hooks/GVFS.Hooks.csproj +++ b/GVFS/GVFS.Hooks/GVFS.Hooks.csproj @@ -71,6 +71,9 @@ Common\ProcessResult.cs + + Common\ProcessStartTimeResult.cs + Common\WorktreeCommandParser.cs diff --git a/GVFS/GVFS.Platform.Windows/WindowsPlatform.Shared.cs b/GVFS/GVFS.Platform.Windows/WindowsPlatform.Shared.cs index 59088bc88..766c05957 100644 --- a/GVFS/GVFS.Platform.Windows/WindowsPlatform.Shared.cs +++ b/GVFS/GVFS.Platform.Windows/WindowsPlatform.Shared.cs @@ -3,6 +3,7 @@ using System; using System.Diagnostics; using System.IO; +using System.Runtime.InteropServices; using System.Security.Principal; namespace GVFS.Platform.Windows @@ -14,6 +15,9 @@ public partial class WindowsPlatform private const int StillActive = 259; /* from Win32 STILL_ACTIVE */ + private const int ErrorAccessDenied = 5; /* ERROR_ACCESS_DENIED */ + private const int ErrorInvalidParameter = 87; /* ERROR_INVALID_PARAMETER */ + public static bool IsElevatedImplementation() { using (WindowsIdentity id = WindowsIdentity.GetCurrent()) @@ -54,15 +58,14 @@ 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. + /// Attempts to read a process's creation timestamp (raw FILETIME, 100-ns ticks since 1601) + /// for identity comparison. The value written to is only valid + /// when the return value is ; two calls for the + /// same underlying process yield equal values, and a recycled PID yields a different value. + /// The failure values are deliberately distinct so that the orphan-lock detector can release + /// a lock only on positive evidence (see ). /// - public static bool TryGetActiveProcessStartTimeImplementation(int processId, out long startTime) + public static ProcessStartTimeResult TryGetActiveProcessStartTimeImplementation(int processId, out long startTime) { startTime = 0; @@ -70,24 +73,52 @@ public static bool TryGetActiveProcessStartTimeImplementation(int processId, out { if (process.IsInvalid) { - return false; + // Classify the failure by the Win32 error so callers can distinguish "the holder + // is gone" from "we could not tell". OpenProcess sets last error on failure and + // the P/Invoke is declared SetLastError=true, so the value is preserved here. + int error = Marshal.GetLastWin32Error(); + switch (error) + { + case ErrorInvalidParameter: + // No process exists with this PID. + return ProcessStartTimeResult.ProcessNotFound; + + case ErrorAccessDenied: + // A process exists but we cannot open it. Because we only reach the + // identity check for holders we successfully opened at acquire time, + // and OpenProcess(QueryLimitedInformation) access is stable for a + // process's lifetime, this means the PID now refers to a different + // process than the original holder. + return ProcessStartTimeResult.Inaccessible; + + default: + // Transient/unclassified (e.g. ERROR_NOT_ENOUGH_MEMORY). We do not know + // whether the original holder is alive. + return ProcessStartTimeResult.Indeterminate; + } } // 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) + if (!NativeMethods.GetExitCodeProcess(process, out uint exitCode)) + { + return ProcessStartTimeResult.Indeterminate; + } + + if (exitCode != StillActive) { - return false; + // The process was opened but has already exited. + return ProcessStartTimeResult.ProcessNotFound; } if (!NativeMethods.GetProcessTimes(process, out long creationTime, out _, out _, out _)) { - return false; + return ProcessStartTimeResult.Indeterminate; } startTime = creationTime; - return true; + return ProcessStartTimeResult.Success; } } diff --git a/GVFS/GVFS.Platform.Windows/WindowsPlatform.cs b/GVFS/GVFS.Platform.Windows/WindowsPlatform.cs index 9d26a71ce..91da42f76 100644 --- a/GVFS/GVFS.Platform.Windows/WindowsPlatform.cs +++ b/GVFS/GVFS.Platform.Windows/WindowsPlatform.cs @@ -255,7 +255,7 @@ public override bool IsProcessActive(int processId) return WindowsPlatform.IsProcessActiveImplementation(processId, tryGetProcessById: true); } - public override bool TryGetActiveProcessStartTime(int processId, out long startTime) + public override ProcessStartTimeResult TryGetActiveProcessStartTime(int processId, out long startTime) { return WindowsPlatform.TryGetActiveProcessStartTimeImplementation(processId, out startTime); } diff --git a/GVFS/GVFS.UnitTests/Common/GVFSLockTests.cs b/GVFS/GVFS.UnitTests/Common/GVFSLockTests.cs index 91173fd7b..3d18fd15c 100644 --- a/GVFS/GVFS.UnitTests/Common/GVFSLockTests.cs +++ b/GVFS/GVFS.UnitTests/Common/GVFSLockTests.cs @@ -260,6 +260,125 @@ public void TryAcquireLockForExternalRequestor_WhenHolderStartTimeMatches() mockTracer.VerifyAll(); } + /// + /// When the start-time read returns positive evidence that the holder is gone + /// (ProcessNotFound), the orphaned lock must be released. + /// + [TestCase] + public void TryAcquireLockForExternalRequestor_WhenHolderProcessNotFound() + { + 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; + + 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); + + // Positive evidence the holder is gone. + mockPlatform.ProcessStartTimeResults[DefaultLockData.PID] = ProcessStartTimeResult.ProcessNotFound; + + 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); + mockPlatform.ProcessStartTimeResults.Remove(DefaultLockData.PID); + mockTracer.VerifyAll(); + } + + /// + /// When the holder's PID is now occupied by a process we cannot open (Inaccessible), the + /// original holder must be gone: we successfully opened it at acquire time, and that access + /// is stable for a process's lifetime, so an access-denied result means the PID was recycled + /// to a different process. The orphaned lock must be released. + /// + [TestCase] + public void TryAcquireLockForExternalRequestor_WhenHolderInaccessible() + { + 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; + + 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); + + // The PID is now occupied by a process we cannot open. + mockPlatform.ProcessStartTimeResults[DefaultLockData.PID] = ProcessStartTimeResult.Inaccessible; + + 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); + mockPlatform.ProcessStartTimeResults.Remove(DefaultLockData.PID); + mockTracer.VerifyAll(); + } + + /// + /// Anti-false-release guard: when the start-time read is Indeterminate (e.g. a transient + /// resource failure), we do NOT know whether the holder is alive, so the lock must be KEPT. + /// Releasing here could hand the lock to a second writer while the original git process is + /// still running -- the worst-case regression this hardening prevents. + /// + [TestCase] + public void TryAcquireLockForExternalRequestor_WhenHolderIndeterminateKeepsLock() + { + 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())); + mockTracer.Setup(x => x.RelatedEvent(EventLevel.Verbose, "ExternalHolderLivenessIndeterminate", 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); + + // We cannot determine whether the holder is alive. + mockPlatform.ProcessStartTimeResults[DefaultLockData.PID] = ProcessStartTimeResult.Indeterminate; + + // A new requestor must be denied -- the lock is still held by the (possibly-alive) holder. + 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); + mockPlatform.ProcessStartTimeResults.Remove(DefaultLockData.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 e7dca80cc..3ee7425de 100644 --- a/GVFS/GVFS.UnitTests/Mock/Common/MockPlatform.cs +++ b/GVFS/GVFS.UnitTests/Mock/Common/MockPlatform.cs @@ -53,6 +53,13 @@ public override bool SupportsSystemInstallLog /// public Dictionary ProcessStartTimes { get; } = new Dictionary(); + /// + /// Optional per-PID result overrides for , used by + /// tests that exercise the non-Success outcomes (Inaccessible / Indeterminate). When a PID is + /// present here, the specified result is returned regardless of . + /// + public Dictionary ProcessStartTimeResults { get; } = new Dictionary(); + public override void ConfigureVisualStudio(string gitBinPath, ITracer tracer) { throw new NotSupportedException(); @@ -141,19 +148,31 @@ public override bool IsProcessActive(int processId) return this.ActiveProcesses.Contains(processId); } - public override bool TryGetActiveProcessStartTime(int processId, out long startTime) + public override ProcessStartTimeResult TryGetActiveProcessStartTime(int processId, out long startTime) { + startTime = 0; + + // Explicit result override wins, so tests can exercise Inaccessible / Indeterminate. + if (this.ProcessStartTimeResults.TryGetValue(processId, out ProcessStartTimeResult overrideResult)) + { + if (overrideResult == ProcessStartTimeResult.Success) + { + startTime = this.ProcessStartTimes.TryGetValue(processId, out long overrideTime) ? overrideTime : processId; + } + + return overrideResult; + } + 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; + return ProcessStartTimeResult.Success; } - startTime = 0; - return false; + return ProcessStartTimeResult.ProcessNotFound; } public override void IsServiceInstalledAndRunning(string name, out bool installed, out bool running) From f034ec7a8448a387946a155ac7102d74c9f33206 Mon Sep 17 00:00:00 2001 From: Tyrie Vella Date: Fri, 10 Jul 2026 09:43:59 -0700 Subject: [PATCH 17/24] test(functional): scope ServiceVerbTests to fixture service Run ServiceVerbTests against a dedicated GVFS service name so --mount-all/--unmount-all only touch repos created in that fixture. This wires service-name overrides through functional-test helpers and adds fixture-local service lifecycle setup/teardown for ServiceVerbTests. Assisted-by: GPT-5.3-Codex Signed-off-by: Tyrie Vella --- .../MultiEnlistmentTests/ServiceVerbTests.cs | 32 ++++- .../TestsWithMultiEnlistment.cs | 6 +- .../Tools/GVFSFunctionalTestEnlistment.cs | 35 +++-- .../GVFS.FunctionalTests/Tools/GVFSHelpers.cs | 8 +- .../GVFS.FunctionalTests/Tools/GVFSProcess.cs | 18 ++- .../Tools/GVFSServiceProcess.cs | 120 +++++++++++------- 6 files changed, 146 insertions(+), 73 deletions(-) diff --git a/GVFS/GVFS.FunctionalTests/Tests/MultiEnlistmentTests/ServiceVerbTests.cs b/GVFS/GVFS.FunctionalTests/Tests/MultiEnlistmentTests/ServiceVerbTests.cs index b2a53cd91..72a049407 100644 --- a/GVFS/GVFS.FunctionalTests/Tests/MultiEnlistmentTests/ServiceVerbTests.cs +++ b/GVFS/GVFS.FunctionalTests/Tests/MultiEnlistmentTests/ServiceVerbTests.cs @@ -1,6 +1,7 @@ using GVFS.FunctionalTests.Tools; using GVFS.Tests.Should; using NUnit.Framework; +using System; namespace GVFS.FunctionalTests.Tests.MultiEnlistmentTests { @@ -9,6 +10,19 @@ namespace GVFS.FunctionalTests.Tests.MultiEnlistmentTests public class ServiceVerbTests : TestsWithMultiEnlistment { private static readonly string[] EmptyRepoList = new string[] { }; + private readonly string fixtureServiceName = "Test.GVFS.Service.ServiceVerbTests." + Guid.NewGuid().ToString("N"); + + [OneTimeSetUp] + public void StartFixtureService() + { + GVFSServiceProcess.InstallService(this.fixtureServiceName); + } + + [OneTimeTearDown] + public void StopFixtureService() + { + GVFSServiceProcess.UninstallService(this.fixtureServiceName); + } [TestCase] public void ServiceCommandsWithNoRepos() @@ -21,20 +35,22 @@ public void ServiceCommandsWithNoRepos() [TestCase] public void ServiceCommandsWithMultipleRepos() { - GVFSFunctionalTestEnlistment enlistment1 = this.CreateNewEnlistment(); - GVFSFunctionalTestEnlistment enlistment2 = this.CreateNewEnlistment(); + GVFSFunctionalTestEnlistment enlistment1 = this.CreateNewEnlistment(serviceName: this.fixtureServiceName); + GVFSFunctionalTestEnlistment enlistment2 = this.CreateNewEnlistment(serviceName: this.fixtureServiceName); string[] repoRootList = new string[] { enlistment1.EnlistmentRoot, enlistment2.EnlistmentRoot }; GVFSProcess gvfsProcess1 = new GVFSProcess( GVFSTestConfig.PathToGVFS, enlistment1.EnlistmentRoot, - enlistment1.LocalCacheRoot); + enlistment1.LocalCacheRoot, + this.fixtureServiceName); GVFSProcess gvfsProcess2 = new GVFSProcess( GVFSTestConfig.PathToGVFS, enlistment2.EnlistmentRoot, - enlistment2.LocalCacheRoot); + enlistment2.LocalCacheRoot, + this.fixtureServiceName); this.RunServiceCommandAndCheckOutput("--list-mounted", expectedRepoRoots: repoRootList); this.RunServiceCommandAndCheckOutput("--unmount-all", expectedRepoRoots: repoRootList); @@ -57,14 +73,15 @@ public void ServiceCommandsWithMultipleRepos() [TestCase] public void ServiceCommandsWithMountAndUnmount() { - GVFSFunctionalTestEnlistment enlistment1 = this.CreateNewEnlistment(); + GVFSFunctionalTestEnlistment enlistment1 = this.CreateNewEnlistment(serviceName: this.fixtureServiceName); string[] repoRootList = new string[] { enlistment1.EnlistmentRoot }; GVFSProcess gvfsProcess1 = new GVFSProcess( GVFSTestConfig.PathToGVFS, enlistment1.EnlistmentRoot, - enlistment1.LocalCacheRoot); + enlistment1.LocalCacheRoot, + this.fixtureServiceName); this.RunServiceCommandAndCheckOutput("--list-mounted", expectedRepoRoots: repoRootList); @@ -88,7 +105,8 @@ private void RunServiceCommandAndCheckOutput(string argument, string[] expectedR GVFSProcess gvfsProcess = new GVFSProcess( GVFSTestConfig.PathToGVFS, enlistmentRoot: null, - localCacheRoot: null); + localCacheRoot: null, + serviceName: this.fixtureServiceName); string result = gvfsProcess.RunServiceVerb(argument); result.ShouldContain(expectedRepoRoots); diff --git a/GVFS/GVFS.FunctionalTests/Tests/MultiEnlistmentTests/TestsWithMultiEnlistment.cs b/GVFS/GVFS.FunctionalTests/Tests/MultiEnlistmentTests/TestsWithMultiEnlistment.cs index 3c9a35327..fd7c37132 100644 --- a/GVFS/GVFS.FunctionalTests/Tests/MultiEnlistmentTests/TestsWithMultiEnlistment.cs +++ b/GVFS/GVFS.FunctionalTests/Tests/MultiEnlistmentTests/TestsWithMultiEnlistment.cs @@ -31,13 +31,15 @@ protected virtual void OnTearDownEnlistmentsDeleted() protected GVFSFunctionalTestEnlistment CreateNewEnlistment( string localCacheRoot = null, string branch = null, - bool skipPrefetch = false) + bool skipPrefetch = false, + string serviceName = null) { GVFSFunctionalTestEnlistment output = GVFSFunctionalTestEnlistment.CloneAndMount( GVFSTestConfig.PathToGVFS, branch, localCacheRoot, - skipPrefetch); + skipPrefetch, + serviceName); this.enlistmentsToDelete.Add(output); return output; } diff --git a/GVFS/GVFS.FunctionalTests/Tools/GVFSFunctionalTestEnlistment.cs b/GVFS/GVFS.FunctionalTests/Tools/GVFSFunctionalTestEnlistment.cs index 15880551b..4d653f7e2 100644 --- a/GVFS/GVFS.FunctionalTests/Tools/GVFSFunctionalTestEnlistment.cs +++ b/GVFS/GVFS.FunctionalTests/Tools/GVFSFunctionalTestEnlistment.cs @@ -20,12 +20,20 @@ public class GVFSFunctionalTestEnlistment private static readonly string ZeroBackgroundOperations = "Background operations: 0" + Environment.NewLine; private GVFSProcess gvfsProcess; + private readonly string serviceName; - private GVFSFunctionalTestEnlistment(string pathToGVFS, string enlistmentRoot, string repoUrl, string commitish, string localCacheRoot = null) + private GVFSFunctionalTestEnlistment( + string pathToGVFS, + string enlistmentRoot, + string repoUrl, + string commitish, + string localCacheRoot = null, + string serviceName = null) { this.EnlistmentRoot = enlistmentRoot; this.RepoUrl = repoUrl; this.Commitish = commitish; + this.serviceName = serviceName; if (localCacheRoot == null) { @@ -43,7 +51,7 @@ private GVFSFunctionalTestEnlistment(string pathToGVFS, string enlistmentRoot, s } this.LocalCacheRoot = localCacheRoot; - this.gvfsProcess = new GVFSProcess(pathToGVFS, this.EnlistmentRoot, this.LocalCacheRoot); + this.gvfsProcess = new GVFSProcess(pathToGVFS, this.EnlistmentRoot, this.LocalCacheRoot, this.serviceName); } public string EnlistmentRoot @@ -57,6 +65,7 @@ public string RepoUrl } public string LocalCacheRoot { get; } + public string ServiceName => this.serviceName; public string RepoBackingRoot { @@ -102,24 +111,25 @@ public static GVFSFunctionalTestEnlistment CloneAndMountWithPerRepoCache(string { string enlistmentRoot = GVFSFunctionalTestEnlistment.GetUniqueEnlistmentRoot(); string localCache = GVFSFunctionalTestEnlistment.GetRepoSpecificLocalCacheRoot(enlistmentRoot); - return CloneAndMount(pathToGvfs, enlistmentRoot, null, localCache, skipPrefetch); + return CloneAndMount(pathToGvfs, enlistmentRoot, null, localCache, skipPrefetch, serviceName: null); } public static GVFSFunctionalTestEnlistment CloneAndMount( string pathToGvfs, string commitish = null, string localCacheRoot = null, - bool skipPrefetch = false) + bool skipPrefetch = false, + string serviceName = null) { string enlistmentRoot = GVFSFunctionalTestEnlistment.GetUniqueEnlistmentRoot(); - return CloneAndMount(pathToGvfs, enlistmentRoot, commitish, localCacheRoot, skipPrefetch); + return CloneAndMount(pathToGvfs, enlistmentRoot, commitish, localCacheRoot, skipPrefetch, serviceName); } - public static GVFSFunctionalTestEnlistment CloneAndMountEnlistmentWithSpacesInPath(string pathToGvfs, string commitish = null) + public static GVFSFunctionalTestEnlistment CloneAndMountEnlistmentWithSpacesInPath(string pathToGvfs, string commitish = null, string serviceName = null) { string enlistmentRoot = GVFSFunctionalTestEnlistment.GetUniqueEnlistmentRootWithSpaces(); string localCache = GVFSFunctionalTestEnlistment.GetRepoSpecificLocalCacheRoot(enlistmentRoot); - return CloneAndMount(pathToGvfs, enlistmentRoot, commitish, localCache); + return CloneAndMount(pathToGvfs, enlistmentRoot, commitish, localCache, skipPrefetch: false, serviceName: serviceName); } public static string GetUniqueEnlistmentRoot() @@ -387,14 +397,21 @@ public string GetObjectPathTo(string objectHash) objectHash.Substring(2)); } - private static GVFSFunctionalTestEnlistment CloneAndMount(string pathToGvfs, string enlistmentRoot, string commitish, string localCacheRoot, bool skipPrefetch = false) + private static GVFSFunctionalTestEnlistment CloneAndMount( + string pathToGvfs, + string enlistmentRoot, + string commitish, + string localCacheRoot, + bool skipPrefetch = false, + string serviceName = null) { GVFSFunctionalTestEnlistment enlistment = new GVFSFunctionalTestEnlistment( pathToGvfs, enlistmentRoot ?? GetUniqueEnlistmentRoot(), GVFSTestConfig.RepoToClone, commitish ?? Properties.Settings.Default.Commitish, - localCacheRoot ?? GVFSTestConfig.LocalCacheRoot); + localCacheRoot ?? GVFSTestConfig.LocalCacheRoot, + serviceName); try { diff --git a/GVFS/GVFS.FunctionalTests/Tools/GVFSHelpers.cs b/GVFS/GVFS.FunctionalTests/Tools/GVFSHelpers.cs index 8fdf7fe7f..d1b356f3d 100644 --- a/GVFS/GVFS.FunctionalTests/Tools/GVFSHelpers.cs +++ b/GVFS/GVFS.FunctionalTests/Tools/GVFSHelpers.cs @@ -197,9 +197,13 @@ public static void ModifiedPathsShouldNotContain(GVFSFunctionalTestEnlistment en } } - public static string GetInternalParameter(string maintenanceJob = "null", string packfileMaintenanceBatchSize = "null") + public static string GetInternalParameter( + string maintenanceJob = "null", + string packfileMaintenanceBatchSize = "null", + string serviceName = null) { - return $"\"{{\\\"ServiceName\\\":\\\"{GVFSServiceProcess.TestServiceName}\\\"," + + string effectiveServiceName = string.IsNullOrWhiteSpace(serviceName) ? GVFSServiceProcess.TestServiceName : serviceName; + return $"\"{{\\\"ServiceName\\\":\\\"{effectiveServiceName}\\\"," + "\\\"StartedByService\\\":false," + $"\\\"MaintenanceJob\\\":{maintenanceJob}," + $"\\\"PackfileMaintenanceBatchSize\\\":{packfileMaintenanceBatchSize}}}\""; diff --git a/GVFS/GVFS.FunctionalTests/Tools/GVFSProcess.cs b/GVFS/GVFS.FunctionalTests/Tools/GVFSProcess.cs index 5d7f415d7..7e632a279 100644 --- a/GVFS/GVFS.FunctionalTests/Tools/GVFSProcess.cs +++ b/GVFS/GVFS.FunctionalTests/Tools/GVFSProcess.cs @@ -14,17 +14,23 @@ public class GVFSProcess private readonly string pathToGVFS; private readonly string enlistmentRoot; private readonly string localCacheRoot; + private readonly string serviceName; public GVFSProcess(GVFSFunctionalTestEnlistment enlistment) - : this(GVFSTestConfig.PathToGVFS, enlistment.EnlistmentRoot, Path.Combine(enlistment.EnlistmentRoot, GVFSTestConfig.DotGVFSRoot)) + : this( + GVFSTestConfig.PathToGVFS, + enlistment.EnlistmentRoot, + Path.Combine(enlistment.EnlistmentRoot, GVFSTestConfig.DotGVFSRoot), + enlistment.ServiceName) { } - public GVFSProcess(string pathToGVFS, string enlistmentRoot, string localCacheRoot) + public GVFSProcess(string pathToGVFS, string enlistmentRoot, string localCacheRoot, string serviceName = null) { this.pathToGVFS = pathToGVFS; this.enlistmentRoot = enlistmentRoot; this.localCacheRoot = localCacheRoot; + this.serviceName = serviceName; } public void Clone(string repositorySource, string branchToCheckout, bool skipPrefetch) @@ -141,13 +147,13 @@ public string LooseObjectStep() return this.CallGVFS( "dehydrate \"" + this.enlistmentRoot + "\"", expectedExitCode: SuccessExitCode, - internalParameter: GVFSHelpers.GetInternalParameter("\\\"LooseObjects\\\"")); + internalParameter: GVFSHelpers.GetInternalParameter("\\\"LooseObjects\\\"", serviceName: this.serviceName)); } public string PackfileMaintenanceStep(long? batchSize) { string sizeString = batchSize.HasValue ? $"\\\"{batchSize.Value}\\\"" : "null"; - string internalParameter = GVFSHelpers.GetInternalParameter("\\\"PackfileMaintenance\\\"", sizeString); + string internalParameter = GVFSHelpers.GetInternalParameter("\\\"PackfileMaintenance\\\"", sizeString, this.serviceName); return this.CallGVFS( "dehydrate \"" + this.enlistmentRoot + "\"", expectedExitCode: SuccessExitCode, @@ -156,7 +162,7 @@ public string PackfileMaintenanceStep(long? batchSize) public string PostFetchStep() { - string internalParameter = GVFSHelpers.GetInternalParameter("\\\"PostFetch\\\""); + string internalParameter = GVFSHelpers.GetInternalParameter("\\\"PostFetch\\\"", serviceName: this.serviceName); return this.CallGVFS( "dehydrate \"" + this.enlistmentRoot + "\"", expectedExitCode: SuccessExitCode, @@ -246,7 +252,7 @@ private string CallGVFS(string args, int expectedExitCode = DoNotCheckExitCode, if (internalParameter == null) { - internalParameter = GVFSHelpers.GetInternalParameter(); + internalParameter = GVFSHelpers.GetInternalParameter(serviceName: this.serviceName); } processInfo.Arguments = args + " " + TestConstants.InternalUseOnlyFlag + " " + internalParameter; diff --git a/GVFS/GVFS.FunctionalTests/Tools/GVFSServiceProcess.cs b/GVFS/GVFS.FunctionalTests/Tools/GVFSServiceProcess.cs index 2ac384629..022504abb 100644 --- a/GVFS/GVFS.FunctionalTests/Tools/GVFSServiceProcess.cs +++ b/GVFS/GVFS.FunctionalTests/Tools/GVFSServiceProcess.cs @@ -2,6 +2,7 @@ using System; using System.Diagnostics; using System.IO; +using System.Collections.Generic; using System.Linq; using System.Runtime.InteropServices; using System.ServiceProcess; @@ -11,8 +12,8 @@ namespace GVFS.FunctionalTests.Tools { public static class GVFSServiceProcess { - private static readonly string ServiceNameArgument = "--servicename=" + TestServiceName; - private static Process consoleServiceProcess; + private static readonly Dictionary ConsoleServiceProcesses = + new Dictionary(System.StringComparer.OrdinalIgnoreCase); public static string TestServiceName { @@ -24,77 +25,98 @@ public static string TestServiceName } public static void InstallService() + { + InstallService(TestServiceName); + } + + public static void InstallService(string serviceName) { if (GVFSTestConfig.IsDevMode) { - StartServiceAsConsoleProcess(); + StartServiceAsConsoleProcess(serviceName); } else { - InstallWindowsService(); + InstallWindowsService(serviceName); } } public static void UninstallService() + { + UninstallService(TestServiceName); + } + + public static void UninstallService(string serviceName) { if (GVFSTestConfig.IsDevMode) { - StopConsoleServiceProcess(); - CleanupServiceData(); + StopConsoleServiceProcess(serviceName); + CleanupServiceData(serviceName); } else { - UninstallWindowsService(); + UninstallWindowsService(serviceName); } } public static void StartService() + { + StartService(TestServiceName); + } + + public static void StartService(string serviceName) { if (GVFSTestConfig.IsDevMode) { - StartServiceAsConsoleProcess(); + StartServiceAsConsoleProcess(serviceName); } else { - StartWindowsService(); + StartWindowsService(serviceName); } } public static void StopService() + { + StopService(TestServiceName); + } + + public static void StopService(string serviceName) { if (GVFSTestConfig.IsDevMode) { - StopConsoleServiceProcess(); + StopConsoleServiceProcess(serviceName); } else { - StopWindowsService(); + StopWindowsService(serviceName); } } - private static void StartServiceAsConsoleProcess() + private static void StartServiceAsConsoleProcess(string serviceName) { - StopConsoleServiceProcess(); + StopConsoleServiceProcess(serviceName); string pathToService = GetPathToService(); Console.WriteLine("Starting test service in console mode: " + pathToService); ProcessStartInfo startInfo = new ProcessStartInfo(pathToService); - startInfo.Arguments = $"--console {ServiceNameArgument}"; + startInfo.Arguments = $"--console --servicename={serviceName}"; startInfo.UseShellExecute = false; startInfo.CreateNoWindow = true; startInfo.RedirectStandardOutput = true; startInfo.RedirectStandardError = true; - consoleServiceProcess = Process.Start(startInfo); + Process consoleServiceProcess = Process.Start(startInfo); consoleServiceProcess.ShouldNotBeNull("Failed to start test service process"); + ConsoleServiceProcesses[serviceName] = consoleServiceProcess; // Consume output asynchronously to prevent buffer deadlock consoleServiceProcess.BeginOutputReadLine(); consoleServiceProcess.BeginErrorReadLine(); // Wait for the service to start listening on its named pipe - string pipeName = TestServiceName + ".pipe"; + string pipeName = serviceName + ".pipe"; int retries = 50; while (retries-- > 0) { @@ -116,39 +138,43 @@ private static void StartServiceAsConsoleProcess() throw new System.TimeoutException("Timed out waiting for test service pipe: " + pipeName); } - private static void StopConsoleServiceProcess() + private static void StopConsoleServiceProcess(string serviceName) { - if (consoleServiceProcess != null && !consoleServiceProcess.HasExited) + Process consoleServiceProcess; + if (ConsoleServiceProcesses.TryGetValue(serviceName, out consoleServiceProcess)) { - try - { - Console.WriteLine("Stopping test service console process (PID: " + consoleServiceProcess.Id + ")"); - consoleServiceProcess.Kill(); - consoleServiceProcess.WaitForExit(5000); - } - catch (InvalidOperationException) + if (consoleServiceProcess != null && !consoleServiceProcess.HasExited) { - // Process already exited + try + { + Console.WriteLine("Stopping test service console process (PID: " + consoleServiceProcess.Id + ")"); + consoleServiceProcess.Kill(); + consoleServiceProcess.WaitForExit(5000); + } + catch (InvalidOperationException) + { + // Process already exited + } } - consoleServiceProcess = null; + ConsoleServiceProcesses.Remove(serviceName); } } - private static void CleanupServiceData() + private static void CleanupServiceData(string serviceName) { string commonAppDataRoot = Environment.GetEnvironmentVariable("GVFS_COMMON_APPDATA_ROOT"); string serviceData; if (!string.IsNullOrEmpty(commonAppDataRoot)) { - serviceData = Path.Combine(commonAppDataRoot, TestServiceName); + serviceData = Path.Combine(commonAppDataRoot, serviceName); } else { serviceData = Path.Combine( Environment.GetFolderPath(Environment.SpecialFolder.CommonApplicationData), "GVFS", - TestServiceName); + serviceName); } DirectoryInfo serviceDataDir = new DirectoryInfo(serviceData); @@ -158,14 +184,14 @@ private static void CleanupServiceData() } } - private static void InstallWindowsService() + private static void InstallWindowsService(string serviceName) { - Console.WriteLine("Installing " + TestServiceName); + Console.WriteLine("Installing " + serviceName); - UninstallWindowsService(); + UninstallWindowsService(serviceName); // Wait for delete to complete. If the services control panel is open, this will never complete. - while (RunScCommand("query", TestServiceName).ExitCode == 0) + while (RunScCommand("query", serviceName).ExitCode == 0) { Thread.Sleep(1000); } @@ -178,23 +204,23 @@ private static void InstallWindowsService() string createServiceArguments = string.Format( "{0} binPath= \"{1}\"", - TestServiceName, + serviceName, pathToService); ProcessResult result = RunScCommand("create", createServiceArguments); result.ExitCode.ShouldEqual(0, "Failure while running sc create " + createServiceArguments + "\r\n" + result.Output); - StartWindowsService(); + StartWindowsService(serviceName); } - private static void UninstallWindowsService() + private static void UninstallWindowsService(string serviceName) { - StopWindowsService(); + StopWindowsService(serviceName); - RunScCommand("delete", TestServiceName); + RunScCommand("delete", serviceName); // Make sure to delete any test service data state - string serviceData = Path.Combine(Environment.GetFolderPath(Environment.SpecialFolder.CommonApplicationData), "GVFS", TestServiceName); + string serviceData = Path.Combine(Environment.GetFolderPath(Environment.SpecialFolder.CommonApplicationData), "GVFS", serviceName); DirectoryInfo serviceDataDir = new DirectoryInfo(serviceData); if (serviceDataDir.Exists) { @@ -202,24 +228,24 @@ private static void UninstallWindowsService() } } - private static void StartWindowsService() + private static void StartWindowsService(string serviceName) { - ServiceController testService = ServiceController.GetServices().SingleOrDefault(service => service.ServiceName == TestServiceName); - testService.ShouldNotBeNull($"{TestServiceName} does not exist as a service"); + ServiceController testService = ServiceController.GetServices().SingleOrDefault(service => service.ServiceName == serviceName); + testService.ShouldNotBeNull($"{serviceName} does not exist as a service"); - using (ServiceController controller = new ServiceController(TestServiceName)) + using (ServiceController controller = new ServiceController(serviceName)) { - controller.Start(new[] { ServiceNameArgument }); + controller.Start(new[] { "--servicename=" + serviceName }); controller.WaitForStatus(ServiceControllerStatus.Running, TimeSpan.FromSeconds(10)); controller.Status.ShouldEqual(ServiceControllerStatus.Running); } } - private static void StopWindowsService() + private static void StopWindowsService(string serviceName) { try { - ServiceController testService = ServiceController.GetServices().SingleOrDefault(service => service.ServiceName == TestServiceName); + ServiceController testService = ServiceController.GetServices().SingleOrDefault(service => service.ServiceName == serviceName); if (testService != null) { if (testService.Status == ServiceControllerStatus.Running) From 63223f8a5cb897bea0388ed7b0fe1c58090faeae Mon Sep 17 00:00:00 2001 From: Tyrie Vella Date: Fri, 10 Jul 2026 11:33:54 -0700 Subject: [PATCH 18/24] HealthTests: wait for background ops before gvfs health HealthTests read placeholder/modified-path counts via 'gvfs health' immediately after hydrating files via File.OpenWrite(). Those writes trigger ProjFS callbacks that GVFS processes on a background queue, so the modified-paths database may not yet reflect the full set of transitions when the health verb samples it. The resulting race manifests as: fast-file count is still 5 (some Scripts/*.bat entries still counted as placeholders) when the test expects 3 after TurnPlaceholdersIntoModifiedPaths. Fix: call WaitForBackgroundOperations() in ValidateHealthOutputValues before invoking the health verb, consistent with every other functional test that asserts modified-path state after a file operation (e.g. GitFilesTests, ModifiedPathsTests, CheckoutTests). Assisted-by: Claude Sonnet 4.6 Signed-off-by: Tyrie Vella --- .../Tests/EnlistmentPerFixture/HealthTests.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/GVFS/GVFS.FunctionalTests/Tests/EnlistmentPerFixture/HealthTests.cs b/GVFS/GVFS.FunctionalTests/Tests/EnlistmentPerFixture/HealthTests.cs index 1e9b6926d..3e4d7ec84 100644 --- a/GVFS/GVFS.FunctionalTests/Tests/EnlistmentPerFixture/HealthTests.cs +++ b/GVFS/GVFS.FunctionalTests/Tests/EnlistmentPerFixture/HealthTests.cs @@ -160,6 +160,7 @@ private void ValidateHealthOutputValues( List directoryHydrationLevels, string enlistmentHealthStatus) { + this.Enlistment.WaitForBackgroundOperations(); string rawOutput = this.Enlistment.Health(directory); List healthOutputLines = new List(rawOutput.Split(new[] { '\n', '\r' }, StringSplitOptions.RemoveEmptyEntries)); From 52e7edddbd177dc3a78d3777994c71149fe8c327 Mon Sep 17 00:00:00 2001 From: Tyrie Vella Date: Mon, 13 Jul 2026 11:31:08 -0700 Subject: [PATCH 19/24] Mount: don't crash when auth is used before background init completes When a cache server is configured locally, mount starts virtualization without waiting for the background auth/config query (PR #2034). During that window, TryInitializeAndQueryGVFSConfig has already published IsAnonymous=false but has not yet set isInitialized (the credential fetch runs in between). A directory enumeration that arrives in this window flows through HttpRequestor.SendRequest -> GitAuthentication.TryGetCredentials, which threw InvalidOperationException("must be initialized"). Thrown into the ProjFS StartDirectoryEnumerationAsyncHandler callback, this crashed the mount process: StartDirectoryEnumerationAsyncHandler caught unhandled exception, exiting process Fix: instead of throwing when called before initialization, TryGetCredentials now waits (bounded by InitializationWaitTimeoutMs, defaulting to the background credential timeout) for initialization to complete, then proceeds normally. If initialization never completes within the timeout it returns a retryable failure rather than crashing. All initialization terminal paths signal a ManualResetEventSlim via a new MarkInitialized() helper. This is isolated to GitAuthentication.cs so it can land independently of the separate change that will gate the background cache-auth behavior behind a config flag. Adds deterministic unit tests that reproduce the race: reintroducing the throw fails them; the fix makes them pass. Assisted-by: Claude Opus 4.8 Signed-off-by: Tyrie Vella --- GVFS/GVFS.Common/Git/GitAuthentication.cs | 46 +++++++++++-- .../Git/GitAuthenticationTests.cs | 69 +++++++++++++++++++ 2 files changed, 109 insertions(+), 6 deletions(-) diff --git a/GVFS/GVFS.Common/Git/GitAuthentication.cs b/GVFS/GVFS.Common/Git/GitAuthentication.cs index 96f47673d..37c3563b5 100644 --- a/GVFS/GVFS.Common/Git/GitAuthentication.cs +++ b/GVFS/GVFS.Common/Git/GitAuthentication.cs @@ -21,6 +21,14 @@ public class GitAuthentication private readonly ICredentialStore credentialStore; private readonly string repoUrl; + // Signaled when initialization completes (success or failure). Callers of + // TryGetCredentials that arrive before initialization is done wait on this + // rather than throwing. This matters when mount starts virtualization while + // auth initialization is still running in the background (cache-server path): + // a directory enumeration can call TryGetCredentials before the background + // TryInitializeAndQueryGVFSConfig has set isInitialized. + private readonly ManualResetEventSlim initializationComplete = new ManualResetEventSlim(false); + private int numberOfAttempts = 0; private DateTime lastAuthAttempt = DateTime.MinValue; @@ -50,6 +58,14 @@ public bool IsBackingOff public bool IsAnonymous { get; private set; } = true; + /// + /// How long a caller of will wait for + /// initialization to complete before giving up with a retryable error. + /// Defaults to the maximum time initialization itself can take (a + /// background credential fetch). Overridable for tests. + /// + internal int InitializationWaitTimeoutMs { get; set; } = BackgroundCredentialTimeoutMs; + private GitSsl GitSsl { get; } public void ApproveCredentials(ITracer tracer, string credentialString) @@ -157,7 +173,17 @@ public bool TryGetCredentials(ITracer tracer, out string credentialString, out s { if (!this.isInitialized) { - throw new InvalidOperationException("This auth instance must be initialized before it can be used"); + // Initialization may still be running in the background (mount can + // start virtualization before the background auth/config query has + // finished when a cache server is configured). Wait for it to + // complete rather than throwing an unhandled exception into the + // virtualization callback, which would crash the mount process. + if (!this.initializationComplete.Wait(this.InitializationWaitTimeoutMs)) + { + credentialString = null; + errorMessage = "Timed out waiting for authentication to initialize"; + return false; + } } credentialString = this.cachedCredentialString; @@ -243,14 +269,14 @@ public bool TryInitializeAndQueryGVFSConfig( if (configRequestor.TryQueryGVFSConfig(false, out serverGVFSConfig, out httpStatus, out _)) { this.IsAnonymous = true; - this.isInitialized = true; + this.MarkInitialized(); tracer.RelatedInfo("{0}: Anonymous access succeeded, config obtained in one request", nameof(this.TryInitializeAndQueryGVFSConfig)); return true; } if (httpStatus != HttpStatusCode.Unauthorized) { - this.isInitialized = true; + this.MarkInitialized(); errorMessage = "Unable to query /gvfs/config"; tracer.RelatedWarning("{0}: Config query failed with status {1}", nameof(this.TryInitializeAndQueryGVFSConfig), httpStatus?.ToString() ?? "None"); return false; @@ -265,12 +291,12 @@ public bool TryInitializeAndQueryGVFSConfig( // Mark initialized even on failure so TryGetCredentials can // retry later (e.g., when mount proceeds with a cache server // and object downloads need auth). - this.isInitialized = true; + this.MarkInitialized(); tracer.RelatedWarning("{0}: Credential fetch failed: {1}", nameof(this.TryInitializeAndQueryGVFSConfig), errorMessage); return false; } - this.isInitialized = true; + this.MarkInitialized(); // Retry with credentials using the same ConfigHttpRequestor (reuses HttpClient/connection) HttpStatusCode? retryHttpStatus; @@ -303,7 +329,7 @@ internal bool TryInitializeAndRequireAuth(ITracer tracer, out string errorMessag if (this.TryCallGitCredential(tracer, out errorMessage)) { - this.isInitialized = true; + this.MarkInitialized(); return true; } @@ -385,6 +411,14 @@ private void UpdateBackoff() this.numberOfAttempts++; } + private void MarkInitialized() + { + // Publish isInitialized before signaling so a waiter that wakes up + // observes the initialized state. + this.isInitialized = true; + this.initializationComplete.Set(); + } + private bool TryCallGitCredential(ITracer tracer, out string errorMessage, int timeoutMs = -1) { // Serialize credential fetches so only one git-credential-fill diff --git a/GVFS/GVFS.UnitTests/Git/GitAuthenticationTests.cs b/GVFS/GVFS.UnitTests/Git/GitAuthenticationTests.cs index c083ac586..25aa675b7 100644 --- a/GVFS/GVFS.UnitTests/Git/GitAuthenticationTests.cs +++ b/GVFS/GVFS.UnitTests/Git/GitAuthenticationTests.cs @@ -1,5 +1,7 @@ using System; using System.Linq; +using System.Threading; +using System.Threading.Tasks; using GVFS.Common.Git; using GVFS.Tests; using GVFS.Tests.Should; @@ -273,6 +275,73 @@ public void RejectionShouldNotBeSentIfUnderlyingTokenHasChanged() gitProcess.CredentialRejections.ShouldBeEmpty(); } + [TestCase] + public void TryGetCredentialsBeforeInitializationDoesNotThrow() + { + // Regression test for a mount crash: when a cache server is configured, + // mount starts virtualization before the background auth/config query + // finishes initializing. A directory enumeration then calls + // TryGetCredentials while IsAnonymous has already been set false but + // initialization has not completed. The previous behavior threw an + // InvalidOperationException straight into the ProjFS callback, crashing + // the mount process. It must instead fail gracefully (retryable). + MockTracer tracer = new MockTracer(); + MockGitProcess gitProcess = this.GetGitProcess(); + + GitAuthentication dut = new GitAuthentication(gitProcess, "mock://repoUrl"); + + // Keep the wait short so an uninitialized instance reports failure + // quickly instead of blocking for the full background timeout. + dut.InitializationWaitTimeoutMs = 10; + + string authString = null; + string error = null; + bool result = true; + + Assert.DoesNotThrow(() => result = dut.TryGetCredentials(tracer, out authString, out error)); + + result.ShouldBeFalse("TryGetCredentials should fail (not throw) before initialization completes"); + error.ShouldNotBeNull("A retryable error message should be returned"); + } + + [TestCase] + public void TryGetCredentialsWaitsForBackgroundInitializationThenSucceeds() + { + // A caller that arrives before initialization completes should block + // until initialization finishes, then return the fetched credentials - + // this is the correct behavior for the background cache-server auth path. + MockTracer tracer = new MockTracer(); + MockGitProcess gitProcess = this.GetGitProcess(); + + GitAuthentication dut = new GitAuthentication(gitProcess, "mock://repoUrl"); + dut.InitializationWaitTimeoutMs = 30_000; + + string authString = null; + string error = null; + bool result = false; + + using (ManualResetEventSlim consumerStarted = new ManualResetEventSlim(false)) + { + Task consumer = Task.Run(() => + { + consumerStarted.Set(); + result = dut.TryGetCredentials(tracer, out authString, out error); + }); + + // Ensure the consumer has begun waiting on initialization before we + // complete it, so we exercise the wait path rather than a no-op. + consumerStarted.Wait(); + Thread.Sleep(50); + + dut.TryInitializeAndRequireAuth(tracer, out _); + + consumer.Wait(TimeSpan.FromSeconds(10)).ShouldBeTrue("Consumer should unblock once initialization completes"); + } + + result.ShouldBeTrue("TryGetCredentials should succeed after initialization: " + error); + authString.ShouldNotBeNull("A credential string should be returned"); + } + private MockGitProcess GetGitProcess() { MockGitProcess gitProcess = new MockGitProcess(); From 067b1069eb24b6f1daaafc72ba78f4365429bed5 Mon Sep 17 00:00:00 2001 From: Tyrie Vella Date: Mon, 13 Jul 2026 11:44:28 -0700 Subject: [PATCH 20/24] Mount: gate background cache-server auth behind gvfs.background-cache-auth MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR #2034 changed mount so that, when a cache server URL is already configured in local git config, authentication and the /gvfs/config query run as a fire-and-forget background task instead of blocking mount startup. As part of GVFS 2.0 stabilization we want that behavioral change to be off by default and opt-in, while keeping the associated diagnostic/reliability improvements (the distinct startup exit codes, the bounded credential timeout, and the cache-server fallback) always on. This adds a git-config flag, gvfs.background-cache-auth (default false), that gates only the fire-and-forget behavior: - Flag off (default): mount blocks on auth + /gvfs/config synchronously before starting virtualization, even when a cache server is configured. Because the network task is awaited, the fetched config is used and a synchronous (Default) credential timeout applies. - Flag on: the #2034 background behavior — mount proceeds without waiting on auth, using the local cache server URL, with the longer background credential timeout. The gate is a single derived condition, useBackgroundAuth = hasCacheServer && flag, applied to the wait decision, the credential-timeout selection, and the source of serverGVFSConfig. The pre-existing cache-server fallback, the startup exit codes, and ValidateGVFSVersion leniency remain keyed on hasCacheServer and are unchanged. The flag is read via libgit2 in-process (a short-lived LibGit2Repo, since the GVFSContext is not created until later in mount), defaulting to off on any failure. Isolated to GVFSConstants.cs and InProcessMount.cs; does not touch GitAuthentication.cs, so it does not conflict with the separate fix that hardens TryGetCredentials against being called before initialization completes. Assisted-by: Claude Opus 4.8 Signed-off-by: Tyrie Vella --- GVFS/GVFS.Common/GVFSConstants.cs | 7 +++++ GVFS/GVFS.Mount/InProcessMount.cs | 50 ++++++++++++++++++++++++++----- 2 files changed, 50 insertions(+), 7 deletions(-) diff --git a/GVFS/GVFS.Common/GVFSConstants.cs b/GVFS/GVFS.Common/GVFSConstants.cs index 8f135786a..842d143ad 100644 --- a/GVFS/GVFS.Common/GVFSConstants.cs +++ b/GVFS/GVFS.Common/GVFSConstants.cs @@ -51,6 +51,13 @@ public static class GitConfig public const string PrefetchUseIdx = GVFSPrefix + "prefetch-use-idx"; public const bool PrefetchUseIdxDefault = false; + + /* Gates the background (fire-and-forget) auth + /gvfs/config behavior on + * mount when a cache server is configured locally. When false (default), + * mount blocks on auth/config synchronously before starting virtualization. + * When true, auth runs in the background so mount is not delayed. */ + public const string BackgroundCacheAuth = GVFSPrefix + "background-cache-auth"; + public const bool BackgroundCacheAuthDefault = false; } public static class LocalGVFSConfig diff --git a/GVFS/GVFS.Mount/InProcessMount.cs b/GVFS/GVFS.Mount/InProcessMount.cs index b88af37fc..1e2c69ffb 100644 --- a/GVFS/GVFS.Mount/InProcessMount.cs +++ b/GVFS/GVFS.Mount/InProcessMount.cs @@ -132,19 +132,27 @@ private void MountWithLockAcquired(EventLevel verbosity, Keywords keywords) Stopwatch parallelTimer = Stopwatch.StartNew(); bool hasCacheServer = this.cacheServer != null && !string.IsNullOrWhiteSpace(this.cacheServer.Url); + // Fire-and-forget background auth is gated behind gvfs.background-cache-auth + // (default off). When the flag is off, mount blocks on auth/config + // synchronously even if a cache server is configured (pre-existing behavior). + bool useBackgroundAuth = hasCacheServer && this.IsBackgroundCacheAuthEnabled(); + this.tracer.RelatedEvent( EventLevel.Informational, "MountPhase", new EventMetadata { { "Phase", "ParallelMountStarted" }, + { "HasCacheServer", hasCacheServer }, + { "UseBackgroundAuth", useBackgroundAuth }, { "ElapsedMs", mountPhaseTimer.ElapsedMilliseconds }, }, Keywords.Telemetry); - // When a cache server is configured locally, auth/config is best-effort: - // mount can proceed without it. We still attempt it so GCM can pop up a - // renewal prompt for stale tokens, but we don't block mount on the result. + // With background auth enabled, auth/config is best-effort: mount can + // proceed without it. We still attempt it so GCM can pop up a renewal + // prompt for stale tokens, but we don't block mount on the result. A + // longer credential timeout is acceptable only because we don't block. var networkTask = Task.Run(() => { Stopwatch sw = Stopwatch.StartNew(); @@ -155,7 +163,7 @@ private void MountWithLockAcquired(EventLevel verbosity, Keywords keywords) if (!this.enlistment.Authentication.TryInitializeAndQueryGVFSConfig( this.tracer, this.enlistment, this.retryConfig, out config, out authConfigError, out isAuthFailure, - credentialTimeoutMs: hasCacheServer ? GitAuthentication.BackgroundCredentialTimeoutMs : GitAuthentication.DefaultCredentialTimeoutMs)) + credentialTimeoutMs: useBackgroundAuth ? GitAuthentication.BackgroundCredentialTimeoutMs : GitAuthentication.DefaultCredentialTimeoutMs)) { if (hasCacheServer) { @@ -291,9 +299,9 @@ private void MountWithLockAcquired(EventLevel verbosity, Keywords keywords) try { - if (hasCacheServer) + if (useBackgroundAuth) { - // With a cache server, don't block mount on the network task. + // With background auth, don't block mount on the network task. // Auth runs in the background to warm credentials / pop GCM, // but mount proceeds immediately using the local cache server URL. localTask.Wait(); @@ -327,7 +335,10 @@ private void MountWithLockAcquired(EventLevel verbosity, Keywords keywords) }, Keywords.Telemetry); - ServerGVFSConfig serverGVFSConfig = hasCacheServer ? null : networkTask.Result; + // Only background auth leaves the network task potentially unfinished; + // in that case we resolve the cache server from local config (null). + // Otherwise we waited on the network task and can use its result. + ServerGVFSConfig serverGVFSConfig = useBackgroundAuth ? null : networkTask.Result; this.mountProgressMessage = "Resolving cache server"; CacheServerResolver cacheServerResolver = new CacheServerResolver(this.tracer, this.enlistment); @@ -462,6 +473,31 @@ private GVFSContext CreateContext() return new GVFSContext(this.tracer, fileSystem, gitRepo, this.enlistment); } + private bool IsBackgroundCacheAuthEnabled() + { + // Read the flag via libgit2 (in-process) rather than spawning git.exe. + // The GVFSContext (and its shared libgit2 repo) is not created until + // later in mount, so open a short-lived repo here just for the config + // read. Default to off on any failure. + try + { + using (LibGit2Repo repo = new LibGit2Repo(this.tracer, this.enlistment.WorkingDirectoryBackingRoot)) + { + return repo.GetConfigBool(GVFSConstants.GitConfig.BackgroundCacheAuth) + ?? GVFSConstants.GitConfig.BackgroundCacheAuthDefault; + } + } + catch (Exception e) + { + this.tracer.RelatedWarning( + "Failed to read {0} config, defaulting to {1}: {2}", + GVFSConstants.GitConfig.BackgroundCacheAuth, + GVFSConstants.GitConfig.BackgroundCacheAuthDefault, + e.Message); + return GVFSConstants.GitConfig.BackgroundCacheAuthDefault; + } + } + private void ValidateMountPoints() { DirectoryInfo workingDirectoryRootInfo = new DirectoryInfo(this.enlistment.WorkingDirectoryBackingRoot); From 1f34e9aee712782dff6281965a02dae8a48b2c7b Mon Sep 17 00:00:00 2001 From: Tyrie Vella Date: Thu, 9 Jul 2026 14:43:32 -0700 Subject: [PATCH 21/24] Gate prefetch offload behind config flag Default gvfs.prefetch-offload to false and skip mount offload runtime paths unless enabled. Add heartbeat FeatureFlags.PrefetchOffload telemetry and unit assertions. Assisted-by: GPT-5.3-Codex Copilot-Session: 85dced45-8638-4b33-a1d1-8af4c8e37fb0 Signed-off-by: Tyrie Vella --- GVFS/GVFS.Common/GVFSConstants.cs | 3 +++ GVFS/GVFS.Common/Git/GitRepo.cs | 5 ++++ .../FileSystemCallbacksTests.cs | 12 +++++++--- .../FileSystemCallbacks.cs | 12 ++++++++++ GVFS/GVFS/CommandLine/PrefetchVerb.cs | 23 +++++++++++++++---- 5 files changed, 48 insertions(+), 7 deletions(-) diff --git a/GVFS/GVFS.Common/GVFSConstants.cs b/GVFS/GVFS.Common/GVFSConstants.cs index 2f6886fdf..c14ec8a93 100644 --- a/GVFS/GVFS.Common/GVFSConstants.cs +++ b/GVFS/GVFS.Common/GVFSConstants.cs @@ -59,6 +59,9 @@ public static class GitConfig * When true, auth runs in the background so mount is not delayed. */ public const string BackgroundCacheAuth = GVFSPrefix + "background-cache-auth"; public const bool BackgroundCacheAuthDefault = false; + + public const string PrefetchOffload = GVFSPrefix + "prefetch-offload"; + public const bool PrefetchOffloadDefault = false; } public static class LocalGVFSConfig diff --git a/GVFS/GVFS.Common/Git/GitRepo.cs b/GVFS/GVFS.Common/Git/GitRepo.cs index 302b0e13e..f66ab2bc1 100644 --- a/GVFS/GVFS.Common/Git/GitRepo.cs +++ b/GVFS/GVFS.Common/Git/GitRepo.cs @@ -77,6 +77,11 @@ public bool TryGetObjectType(string sha, out Native.ObjectTypes? objectType) return this.libgit2RepoInvoker.TryInvoke(repo => repo.GetObjectType(sha), out objectType); } + public bool GetConfigBoolOrDefault(string key, bool defaultValue) + { + return this.libgit2RepoInvoker?.GetConfigBoolOrDefault(key, defaultValue) ?? defaultValue; + } + public virtual bool TryCopyBlobContentStream(string blobSha, Action writeAction) { LooseBlobState state = this.GetLooseBlobState(blobSha, writeAction, out long size); diff --git a/GVFS/GVFS.UnitTests/Virtualization/FileSystemCallbacksTests.cs b/GVFS/GVFS.UnitTests/Virtualization/FileSystemCallbacksTests.cs index 6ebd817fd..a8f278504 100644 --- a/GVFS/GVFS.UnitTests/Virtualization/FileSystemCallbacksTests.cs +++ b/GVFS/GVFS.UnitTests/Virtualization/FileSystemCallbacksTests.cs @@ -34,7 +34,7 @@ public void IsPathInsideDotGitIsTrueForDotGitPath() { FileSystemCallbacks.IsPathInsideDotGit(@".git" + Path.DirectorySeparatorChar).ShouldEqual(true); FileSystemCallbacks.IsPathInsideDotGit(Path.Combine(".git", "test_file.txt")).ShouldEqual(true); - FileSystemCallbacks.IsPathInsideDotGit(Path.Combine(".git", "test_folder", "test_file.txt")).ShouldEqual(true); + FileSystemCallbacks.IsPathInsideDotGit(Path.Combine(".git", "test_folder", "test_file.txt")).ShouldEqual(true); } [TestCase] @@ -159,11 +159,14 @@ public void GetMetadataForHeartBeatDoesSetsEventLevelToInformationalWhenPlacehol eventLevel.ShouldEqual(EventLevel.Informational); // "ModifiedPathsCount" should be 1 because ".gitattributes" is always present - metadata.Count.ShouldEqual(8); + metadata.Count.ShouldEqual(9); metadata.ContainsKey("FilePlaceholderCreation").ShouldBeTrue(); metadata.TryGetValue("FilePlaceholderCreation", out object fileNestedMetadata); GVFSJsonOptions.Serialize(fileNestedMetadata).ShouldContain("\"ProcessName1\":\"GVFS.UnitTests.exe\""); GVFSJsonOptions.Serialize(fileNestedMetadata).ShouldContain("\"ProcessCount1\":1"); + metadata.ContainsKey("FeatureFlags").ShouldBeTrue(); + metadata.TryGetValue("FeatureFlags", out object featureFlagsMetadata); + GVFSJsonOptions.Serialize(featureFlagsMetadata).ShouldContain("\"PrefetchOffload\":false"); metadata.ShouldContain("ModifiedPathsCount", 1); metadata.ShouldContain("FilePlaceholderCount", 1); metadata.ShouldContain("FolderPlaceholderCount", 0); @@ -182,13 +185,16 @@ public void GetMetadataForHeartBeatDoesSetsEventLevelToInformationalWhenPlacehol eventLevel = writeToLogFile2 ? EventLevel.Informational : EventLevel.Verbose; eventLevel.ShouldEqual(EventLevel.Informational); - metadata.Count.ShouldEqual(8); + metadata.Count.ShouldEqual(9); // Only processes that have created placeholders since the last heartbeat should be named metadata.ContainsKey("FilePlaceholderCreation").ShouldBeTrue(); metadata.TryGetValue("FilePlaceholderCreation", out object fileNestedMetadata2); GVFSJsonOptions.Serialize(fileNestedMetadata2).ShouldContain("\"ProcessName1\":\"GVFS.UnitTests.exe2\""); GVFSJsonOptions.Serialize(fileNestedMetadata2).ShouldContain("\"ProcessCount1\":2"); + metadata.ContainsKey("FeatureFlags").ShouldBeTrue(); + metadata.TryGetValue("FeatureFlags", out object featureFlagsMetadata2); + GVFSJsonOptions.Serialize(featureFlagsMetadata2).ShouldContain("\"PrefetchOffload\":false"); metadata.ContainsKey("FolderPlaceholderCreation").ShouldBeTrue(); metadata.TryGetValue("FolderPlaceholderCreation", out object folderNestedMetadata2); GVFSJsonOptions.Serialize(folderNestedMetadata2).ShouldContain("\"ProcessName1\":\"GVFS.UnitTests.exe2\""); diff --git a/GVFS/GVFS.Virtualization/FileSystemCallbacks.cs b/GVFS/GVFS.Virtualization/FileSystemCallbacks.cs index 91e627dd2..7cc57392e 100644 --- a/GVFS/GVFS.Virtualization/FileSystemCallbacks.cs +++ b/GVFS/GVFS.Virtualization/FileSystemCallbacks.cs @@ -277,6 +277,7 @@ public EventMetadata GetAndResetHeartBeatMetadata(out bool logToFile) metadata.Add("ModifiedPathsCount", this.modifiedPaths.Count); metadata.Add("FilePlaceholderCount", this.placeholderDatabase.GetFilePlaceholdersCount()); metadata.Add("FolderPlaceholderCount", this.placeholderDatabase.GetFolderPlaceholdersCount()); + metadata.Add("FeatureFlags", this.GetFeatureFlagMetadata()); this.fileSystemVirtualizer?.AddHeartbeatMetadata(metadata); @@ -295,6 +296,17 @@ public EventMetadata GetAndResetHeartBeatMetadata(out bool logToFile) return metadata; } + private EventMetadata GetFeatureFlagMetadata() + { + bool prefetchOffloadEnabled = this.context.Repository.GetConfigBoolOrDefault( + GVFSConstants.GitConfig.PrefetchOffload, + GVFSConstants.GitConfig.PrefetchOffloadDefault); + + EventMetadata featureFlags = new EventMetadata(); + featureFlags.Add("PrefetchOffload", prefetchOffloadEnabled); + return featureFlags; + } + public bool TryDehydrateFolder(string relativePath, out string errorMessage) { List removedPlaceholders = null; diff --git a/GVFS/GVFS/CommandLine/PrefetchVerb.cs b/GVFS/GVFS/CommandLine/PrefetchVerb.cs index ef6d3ddf9..00bea8524 100644 --- a/GVFS/GVFS/CommandLine/PrefetchVerb.cs +++ b/GVFS/GVFS/CommandLine/PrefetchVerb.cs @@ -157,6 +157,8 @@ protected override void Execute(GVFSEnlistment enlistment) metadata.Add("FilesFromStdIn", this.FilesFromStdIn); metadata.Add("FoldersFromStdIn", this.FoldersFromStdIn); metadata.Add("HydrateFiles", this.HydrateFiles); + bool prefetchOffloadEnabled = this.IsPrefetchOffloadEnabled(tracer, enlistment); + metadata.Add("PrefetchOffloadEnabled", prefetchOffloadEnabled); tracer.RelatedEvent(EventLevel.Informational, "PerformPrefetch", metadata); if (this.Commits) @@ -181,7 +183,7 @@ protected override void Execute(GVFSEnlistment enlistment) // has its own spinner. We don't wrap this in ShowStatusWhileRunning // because a false return (mount unavailable) would print "Failed" // to the console, which is misleading for an expected fallback. - bool offloadSucceeded = this.TryPrefetchCommitsViaMountProcess(tracer, enlistment); + bool offloadSucceeded = prefetchOffloadEnabled && this.TryPrefetchCommitsViaMountProcess(tracer, enlistment); if (!offloadSucceeded) { @@ -220,9 +222,12 @@ protected override void Execute(GVFSEnlistment enlistment) // (without hydration), then hydrate locally in the verb process. // This avoids the mount process writing to ProjFS-virtualized files // (self-callback risk) while still benefiting from warm auth. - if (!this.TryPrefetchBlobsViaMountProcess(tracer, enlistment, filesList, foldersList, headCommitId)) + bool offloadSucceeded = prefetchOffloadEnabled && + this.TryPrefetchBlobsViaMountProcess(tracer, enlistment, filesList, foldersList, headCommitId); + + if (!offloadSucceeded) { - // Mount unavailable — fall back to direct auth for download + // Mount unavailable/offload disabled — fall back to direct auth for download GitObjectsHttpRequestor objectRequestor; CacheServerInfo resolvedCacheServer; this.InitializeServerConnection( @@ -242,7 +247,8 @@ protected override void Execute(GVFSEnlistment enlistment) BlobPrefetcher.UpdateNoopCache(prefetchCache, prefetchCacheSize, headCommitId, filesList, foldersList, this.HydrateFiles); } } - else if (!this.TryPrefetchBlobsViaMountProcess(tracer, enlistment, filesList, foldersList, headCommitId)) + else if (!prefetchOffloadEnabled || + !this.TryPrefetchBlobsViaMountProcess(tracer, enlistment, filesList, foldersList, headCommitId)) { GitObjectsHttpRequestor objectRequestor; CacheServerInfo resolvedCacheServer; @@ -692,6 +698,15 @@ private string GetCacheServerDisplay(CacheServerInfo cacheServer, string repoUrl return "from origin (no cache server)"; } + private bool IsPrefetchOffloadEnabled(ITracer tracer, GVFSEnlistment enlistment) + { + using (LibGit2Repo repo = new LibGit2Repo(tracer, enlistment.WorkingDirectoryBackingRoot)) + { + bool? enabled = repo.GetConfigBool(GVFSConstants.GitConfig.PrefetchOffload); + return enabled ?? GVFSConstants.GitConfig.PrefetchOffloadDefault; + } + } + /// /// Hydrates files matching the file/folder filters by reading 1 byte from each. /// Runs in the verb process (not the mount) to avoid ProjFS self-callbacks. From e642590d563874358655d4bec3e2cc845fe0dee6 Mon Sep 17 00:00:00 2001 From: Tyrie Vella Date: Mon, 13 Jul 2026 11:37:40 -0700 Subject: [PATCH 22/24] Remove redundant heartbeat FeatureFlags telemetry The gvfs.prefetch-offload flag state is already captured fleet-wide by the git Trace2 collector via trace2.configparams (scalar.*,gvfs.*,...), surfaced as Summary.payload.params. Re-emitting it in VFS.Heartbeat is redundant and client-side telemetry adopts slowly across the GVFS fleet. Drop the FeatureFlags heartbeat metadata and the GitRepo.GetConfigBoolOrDefault wrapper added only to feed it. The gvfs.prefetch-offload gate in PrefetchVerb is unaffected. Assisted-by: Claude Opus 4.8 Signed-off-by: Tyrie Vella --- GVFS/GVFS.Common/Git/GitRepo.cs | 5 ----- .../Virtualization/FileSystemCallbacksTests.cs | 10 ++-------- GVFS/GVFS.Virtualization/FileSystemCallbacks.cs | 12 ------------ 3 files changed, 2 insertions(+), 25 deletions(-) diff --git a/GVFS/GVFS.Common/Git/GitRepo.cs b/GVFS/GVFS.Common/Git/GitRepo.cs index f66ab2bc1..302b0e13e 100644 --- a/GVFS/GVFS.Common/Git/GitRepo.cs +++ b/GVFS/GVFS.Common/Git/GitRepo.cs @@ -77,11 +77,6 @@ public bool TryGetObjectType(string sha, out Native.ObjectTypes? objectType) return this.libgit2RepoInvoker.TryInvoke(repo => repo.GetObjectType(sha), out objectType); } - public bool GetConfigBoolOrDefault(string key, bool defaultValue) - { - return this.libgit2RepoInvoker?.GetConfigBoolOrDefault(key, defaultValue) ?? defaultValue; - } - public virtual bool TryCopyBlobContentStream(string blobSha, Action writeAction) { LooseBlobState state = this.GetLooseBlobState(blobSha, writeAction, out long size); diff --git a/GVFS/GVFS.UnitTests/Virtualization/FileSystemCallbacksTests.cs b/GVFS/GVFS.UnitTests/Virtualization/FileSystemCallbacksTests.cs index a8f278504..3a515c3b5 100644 --- a/GVFS/GVFS.UnitTests/Virtualization/FileSystemCallbacksTests.cs +++ b/GVFS/GVFS.UnitTests/Virtualization/FileSystemCallbacksTests.cs @@ -159,14 +159,11 @@ public void GetMetadataForHeartBeatDoesSetsEventLevelToInformationalWhenPlacehol eventLevel.ShouldEqual(EventLevel.Informational); // "ModifiedPathsCount" should be 1 because ".gitattributes" is always present - metadata.Count.ShouldEqual(9); + metadata.Count.ShouldEqual(8); metadata.ContainsKey("FilePlaceholderCreation").ShouldBeTrue(); metadata.TryGetValue("FilePlaceholderCreation", out object fileNestedMetadata); GVFSJsonOptions.Serialize(fileNestedMetadata).ShouldContain("\"ProcessName1\":\"GVFS.UnitTests.exe\""); GVFSJsonOptions.Serialize(fileNestedMetadata).ShouldContain("\"ProcessCount1\":1"); - metadata.ContainsKey("FeatureFlags").ShouldBeTrue(); - metadata.TryGetValue("FeatureFlags", out object featureFlagsMetadata); - GVFSJsonOptions.Serialize(featureFlagsMetadata).ShouldContain("\"PrefetchOffload\":false"); metadata.ShouldContain("ModifiedPathsCount", 1); metadata.ShouldContain("FilePlaceholderCount", 1); metadata.ShouldContain("FolderPlaceholderCount", 0); @@ -185,16 +182,13 @@ public void GetMetadataForHeartBeatDoesSetsEventLevelToInformationalWhenPlacehol eventLevel = writeToLogFile2 ? EventLevel.Informational : EventLevel.Verbose; eventLevel.ShouldEqual(EventLevel.Informational); - metadata.Count.ShouldEqual(9); + metadata.Count.ShouldEqual(8); // Only processes that have created placeholders since the last heartbeat should be named metadata.ContainsKey("FilePlaceholderCreation").ShouldBeTrue(); metadata.TryGetValue("FilePlaceholderCreation", out object fileNestedMetadata2); GVFSJsonOptions.Serialize(fileNestedMetadata2).ShouldContain("\"ProcessName1\":\"GVFS.UnitTests.exe2\""); GVFSJsonOptions.Serialize(fileNestedMetadata2).ShouldContain("\"ProcessCount1\":2"); - metadata.ContainsKey("FeatureFlags").ShouldBeTrue(); - metadata.TryGetValue("FeatureFlags", out object featureFlagsMetadata2); - GVFSJsonOptions.Serialize(featureFlagsMetadata2).ShouldContain("\"PrefetchOffload\":false"); metadata.ContainsKey("FolderPlaceholderCreation").ShouldBeTrue(); metadata.TryGetValue("FolderPlaceholderCreation", out object folderNestedMetadata2); GVFSJsonOptions.Serialize(folderNestedMetadata2).ShouldContain("\"ProcessName1\":\"GVFS.UnitTests.exe2\""); diff --git a/GVFS/GVFS.Virtualization/FileSystemCallbacks.cs b/GVFS/GVFS.Virtualization/FileSystemCallbacks.cs index 7cc57392e..91e627dd2 100644 --- a/GVFS/GVFS.Virtualization/FileSystemCallbacks.cs +++ b/GVFS/GVFS.Virtualization/FileSystemCallbacks.cs @@ -277,7 +277,6 @@ public EventMetadata GetAndResetHeartBeatMetadata(out bool logToFile) metadata.Add("ModifiedPathsCount", this.modifiedPaths.Count); metadata.Add("FilePlaceholderCount", this.placeholderDatabase.GetFilePlaceholdersCount()); metadata.Add("FolderPlaceholderCount", this.placeholderDatabase.GetFolderPlaceholdersCount()); - metadata.Add("FeatureFlags", this.GetFeatureFlagMetadata()); this.fileSystemVirtualizer?.AddHeartbeatMetadata(metadata); @@ -296,17 +295,6 @@ public EventMetadata GetAndResetHeartBeatMetadata(out bool logToFile) return metadata; } - private EventMetadata GetFeatureFlagMetadata() - { - bool prefetchOffloadEnabled = this.context.Repository.GetConfigBoolOrDefault( - GVFSConstants.GitConfig.PrefetchOffload, - GVFSConstants.GitConfig.PrefetchOffloadDefault); - - EventMetadata featureFlags = new EventMetadata(); - featureFlags.Add("PrefetchOffload", prefetchOffloadEnabled); - return featureFlags; - } - public bool TryDehydrateFolder(string relativePath, out string errorMessage) { List removedPlaceholders = null; From ee65930bac971df14d7d43000c2ffd42301001de Mon Sep 17 00:00:00 2001 From: tyrielv Date: Tue, 14 Jul 2026 15:41:08 -0700 Subject: [PATCH 23/24] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- GVFS/GVFS/CommandLine/PrefetchVerb.cs | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/GVFS/GVFS/CommandLine/PrefetchVerb.cs b/GVFS/GVFS/CommandLine/PrefetchVerb.cs index 00bea8524..945984d62 100644 --- a/GVFS/GVFS/CommandLine/PrefetchVerb.cs +++ b/GVFS/GVFS/CommandLine/PrefetchVerb.cs @@ -700,11 +700,19 @@ private string GetCacheServerDisplay(CacheServerInfo cacheServer, string repoUrl private bool IsPrefetchOffloadEnabled(ITracer tracer, GVFSEnlistment enlistment) { - using (LibGit2Repo repo = new LibGit2Repo(tracer, enlistment.WorkingDirectoryBackingRoot)) + try { - bool? enabled = repo.GetConfigBool(GVFSConstants.GitConfig.PrefetchOffload); - return enabled ?? GVFSConstants.GitConfig.PrefetchOffloadDefault; + using (LibGit2Repo repo = new LibGit2Repo(tracer, enlistment.WorkingDirectoryBackingRoot)) + { + bool? enabled = repo.GetConfigBool(GVFSConstants.GitConfig.PrefetchOffload); + return enabled ?? GVFSConstants.GitConfig.PrefetchOffloadDefault; + } } + catch (Exception ex) + { + tracer.RelatedWarning($"Failed to read '{GVFSConstants.GitConfig.PrefetchOffload}' config; defaulting to {GVFSConstants.GitConfig.PrefetchOffloadDefault}: {ex.GetType().Name}: {ex.Message}"); + return GVFSConstants.GitConfig.PrefetchOffloadDefault; + } } /// From c0a95bc7515b32bacbda54adbdefe6a7b235bf45 Mon Sep 17 00:00:00 2001 From: Tyrie Vella Date: Tue, 14 Jul 2026 16:48:06 -0700 Subject: [PATCH 24/24] Retrigger CI: NuGet feed pull-through completed