From f22daabcae8be07a1baa34091818270ff1cc5581 Mon Sep 17 00:00:00 2001 From: Tyrie Vella Date: Thu, 27 Aug 2026 09:44:02 -0700 Subject: [PATCH 01/11] FunctionalTests: capture mount dumps and preserve logs on failure When a functional test fails because its GVFS.Mount is unreachable (a hang or silent exit), we currently have no post-mortem data. Per-test enlistment inlined into the console by TestResultsHelper.OutputGVFSLogs -- lossy under parallel fixtures and empty when the mount hung. There is no process dump, so a mount deadlock leaves zero diagnostic signal. On test failure only, into a CI-uploadable diagnostics directory: GVFSFunctionalTestEnlistment.CaptureFailureDiagnostics runs first in DeleteEnlistment, gated on TestStatus.Failed, before the enlistment directory is deleted. The mount-process PID discovery is extracted from KillMountProcess into a shared GetMountProcessIds helper. CI: functional-tests.yaml sets GVFS_TEST_DIAGNOSTICS_DIR and uploads it as a FailureDiagnostics artifact with if: always(). Review follow-ups: - GetMountProcessIds doubled the backslashes in the enlistment path before using it in a PowerShell -like wildcard. In -like, '\' is a literal (not an escape), so the doubled pattern never matched a real single-backslash command line: CaptureFailureDiagnostics found no live mount and wrote no minidump, and KillMountProcess silently killed nothing. Match on the enlistment's unique leaf folder id instead -- present on the GVFS.Mount command line (launched with PrimaryEnlistmentRoot), unique, and free of path separators or wildcard metacharacters, so it needs no escaping. - WaitForExit(int) only guarantees the helper process has exited -- it does not guarantee the async OutputDataReceived callbacks (raised on the thread pool as data arrives) have all run yet. Reading the output buffer immediately afterward could race the last callback and intermittently drop a trailing PID, making GetMountProcessIds miss a live mount process. Call the parameterless WaitForExit() right after the timed wait succeeds to drain any pending async output callbacks before parsing. Assisted-by: Claude Sonnet 5 Signed-off-by: Tyrie Vella --- .github/workflows/functional-tests.yaml | 10 + .../Tests/TestResultsHelper.cs | 83 ++++++++ .../Tools/GVFSFunctionalTestEnlistment.cs | 190 ++++++++++++++++-- GVFS/GVFS.FunctionalTests/Tools/MiniDump.cs | 88 ++++++++ 4 files changed, 357 insertions(+), 14 deletions(-) create mode 100644 GVFS/GVFS.FunctionalTests/Tools/MiniDump.cs diff --git a/.github/workflows/functional-tests.yaml b/.github/workflows/functional-tests.yaml index 60f61d3088..7274f97656 100644 --- a/.github/workflows/functional-tests.yaml +++ b/.github/workflows/functional-tests.yaml @@ -204,8 +204,18 @@ jobs: run: | SET PATH=C:\Program Files\VFS for Git;%PATH% SET GIT_TRACE2_PERF=C:\temp\git-trace2.log + SET GVFS_TEST_DIAGNOSTICS_DIR=C:\temp\gvfs-ft-diagnostics ft\GVFS.FunctionalTests.exe /result:TestResult.xml --ci --slice=${{ matrix.nr }},12 + - name: Upload failure diagnostics (mount dumps + logs) + if: always() && steps.skip.outputs.result != 'true' + uses: actions/upload-artifact@v7 + continue-on-error: true + with: + name: ${{ env.ARTIFACT_PREFIX }}FailureDiagnostics_${{ env.FT_MATRIX_NAME }} + path: C:\temp\gvfs-ft-diagnostics + if-no-files-found: ignore + - name: Upload functional test results if: always() && steps.skip.outputs.result != 'true' uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 diff --git a/GVFS/GVFS.FunctionalTests/Tests/TestResultsHelper.cs b/GVFS/GVFS.FunctionalTests/Tests/TestResultsHelper.cs index e70adca78d..3c197a1ecd 100644 --- a/GVFS/GVFS.FunctionalTests/Tests/TestResultsHelper.cs +++ b/GVFS/GVFS.FunctionalTests/Tests/TestResultsHelper.cs @@ -69,5 +69,88 @@ public static IEnumerable GetAllFilesInDirectory(string folderName) return directory.GetFiles().Select(file => file.FullName); } + + /// + /// Root directory under which per-failure diagnostics (preserved logs and + /// mount process dumps) are written so CI can upload them as an artifact. + /// Honors the GVFS_TEST_DIAGNOSTICS_DIR environment variable; otherwise + /// falls back to a folder under the temp path. + /// + public static string DiagnosticsRoot + { + get + { + string configured = Environment.GetEnvironmentVariable("GVFS_TEST_DIAGNOSTICS_DIR"); + return string.IsNullOrWhiteSpace(configured) + ? Path.Combine(Path.GetTempPath(), "gvfs_ft_diagnostics") + : configured; + } + } + + /// + /// Copies every file in into + /// . A mount that hung or exited + /// abnormally may still hold its log file open, so a plain copy can fail + /// with a sharing violation. In that case we fall back to opening the file + /// with a read-only shared handle (FileShare.ReadWrite | Delete) and copy + /// out whatever has been flushed so far — partial content is still useful. + /// Best-effort: never throws. + /// + public static void CopyFilesWithFallback(string sourceFolder, string destinationFolder) + { + try + { + Directory.CreateDirectory(destinationFolder); + } + catch (Exception ex) + { + Console.Error.WriteLine($"[DIAGNOSTICS] Unable to create '{destinationFolder}': {ex.Message}"); + return; + } + + foreach (string sourceFile in GetAllFilesInDirectory(sourceFolder)) + { + string destinationFile = Path.Combine(destinationFolder, Path.GetFileName(sourceFile)); + + try + { + File.Copy(sourceFile, destinationFile, overwrite: true); + } + catch (Exception copyException) when (copyException is IOException || copyException is UnauthorizedAccessException) + { + // The file is likely locked by a still-running (possibly hung) + // mount process. Fall back to a shared read-only handle and copy + // what we can. + if (!TryCopyWithSharedReadHandle(sourceFile, destinationFile)) + { + Console.Error.WriteLine($"[DIAGNOSTICS] Failed to copy '{sourceFile}' (locked): {copyException.Message}"); + } + } + catch (Exception ex) + { + Console.Error.WriteLine($"[DIAGNOSTICS] Failed to copy '{sourceFile}': {ex.Message}"); + } + } + } + + private static bool TryCopyWithSharedReadHandle(string sourceFile, string destinationFile) + { + try + { + using (FileStream source = new FileStream(sourceFile, FileMode.Open, FileAccess.Read, FileShare.ReadWrite | FileShare.Delete)) + using (FileStream destination = new FileStream(destinationFile, FileMode.Create, FileAccess.Write, FileShare.None)) + { + source.CopyTo(destination); + } + + Console.Error.WriteLine($"[DIAGNOSTICS] Copied '{sourceFile}' via shared read handle (may be partial)"); + return true; + } + catch (Exception ex) + { + Console.Error.WriteLine($"[DIAGNOSTICS] Shared-handle copy of '{sourceFile}' failed: {ex.Message}"); + return false; + } + } } } diff --git a/GVFS/GVFS.FunctionalTests/Tools/GVFSFunctionalTestEnlistment.cs b/GVFS/GVFS.FunctionalTests/Tools/GVFSFunctionalTestEnlistment.cs index 4d653f7e23..d352a53191 100644 --- a/GVFS/GVFS.FunctionalTests/Tools/GVFSFunctionalTestEnlistment.cs +++ b/GVFS/GVFS.FunctionalTests/Tools/GVFSFunctionalTestEnlistment.cs @@ -2,6 +2,8 @@ using GVFS.FunctionalTests.Should; using GVFS.FunctionalTests.Tests; using GVFS.Tests.Should; +using NUnit.Framework; +using NUnit.Framework.Interfaces; using System; using System.Collections.Generic; using System.IO; @@ -179,10 +181,102 @@ public string GetPackRoot(FileSystemRunner fileSystem) public void DeleteEnlistment() { + this.CaptureFailureLogs(); TestResultsHelper.OutputGVFSLogs(this); RepositoryHelpers.DeleteTestDirectory(this.EnlistmentRoot); } + /// + /// When the current test has failed, writes a full-memory minidump of each still-running + /// GVFS.Mount process for this enlistment, so a mount *hang* can be diagnosed after the fact. + /// Must be called before the mount is unmounted or killed - once the process is gone (whether + /// cleanly unmounted or force-killed) there is nothing left to dump. Written under + /// so CI can upload it. Best-effort: never + /// throws, so it cannot break teardown. + /// + public void CaptureFailureDiagnostics() + { + try + { + if (!this.TryGetFailureDiagnosticsFolder(out string destinationFolder)) + { + return; + } + + List mountProcessIds = this.GetMountProcessIds(); + if (mountProcessIds.Count == 0) + { + Console.Error.WriteLine("[DIAGNOSTICS] No live GVFS.Mount process for this enlistment (already exited/crashed)"); + return; + } + + Console.Error.WriteLine($"[DIAGNOSTICS] Test failed; capturing mount dump(s) to '{destinationFolder}'"); + Directory.CreateDirectory(destinationFolder); + foreach (int pid in mountProcessIds) + { + MiniDump.TryWrite(pid, Path.Combine(destinationFolder, $"GVFS.Mount_{pid}.dmp")); + } + } + catch (Exception ex) + { + Console.Error.WriteLine($"[DIAGNOSTICS] CaptureFailureDiagnostics failed: {ex.Message}"); + } + } + + /// + /// When the current test has failed, preserves the enlistment's .gvfs/logs folder (robust to + /// locked / partially-flushed files) under + /// before the enlistment directory is deleted. Best-effort: never throws. + /// + private void CaptureFailureLogs() + { + try + { + if (!this.TryGetFailureDiagnosticsFolder(out string destinationFolder)) + { + return; + } + + Console.Error.WriteLine($"[DIAGNOSTICS] Test failed; capturing logs to '{destinationFolder}'"); + TestResultsHelper.CopyFilesWithFallback(this.GVFSLogsRoot, Path.Combine(destinationFolder, "logs")); + } + catch (Exception ex) + { + Console.Error.WriteLine($"[DIAGNOSTICS] CaptureFailureLogs failed: {ex.Message}"); + } + } + + private bool TryGetFailureDiagnosticsFolder(out string destinationFolder) + { + destinationFolder = null; + if (TestContext.CurrentContext.Result.Outcome.Status != TestStatus.Failed) + { + return false; + } + + destinationFolder = Path.Combine( + TestResultsHelper.DiagnosticsRoot, + SanitizeForPath(TestContext.CurrentContext.Test.Name) + "_" + Path.GetFileName(this.EnlistmentRoot)); + return true; + } + + private static string SanitizeForPath(string name) + { + if (string.IsNullOrEmpty(name)) + { + return "test"; + } + + foreach (char invalid in Path.GetInvalidFileNameChars()) + { + name = name.Replace(invalid, '_'); + } + + // Flatten characters that are legal in file names but noisy in NUnit + // test names (parameterized cases, spaces). + return name.Replace('(', '_').Replace(')', '_').Replace(' ', '_').Replace(',', '_').Replace('"', '_'); + } + public void CloneAndMount(bool skipPrefetch) { Console.Error.WriteLine("[CI-DEBUG] CloneAndMount: starting clone of " + this.RepoUrl); @@ -303,6 +397,10 @@ public string SetCacheServer(string arg) public void UnmountAndDeleteAll() { + // Capture the mount dump before unmounting or killing anything - once the mount process is + // gone (whether it unmounts cleanly or is force-killed below) there is nothing left to dump. + this.CaptureFailureDiagnostics(); + try { this.UnmountGVFS(); @@ -320,12 +418,39 @@ public void UnmountAndDeleteAll() public void KillMountProcess() { + foreach (int pid in this.GetMountProcessIds()) + { + Console.Error.WriteLine($"[TEARDOWN] Killing GVFS.Mount (PID {pid}) for {this.EnlistmentRoot}"); + try + { + System.Diagnostics.Process.GetProcessById(pid)?.Kill(); + } + catch (Exception ex) + { + Console.Error.WriteLine($"[TEARDOWN] Failed to kill PID {pid}: {ex.Message}"); + } + } + } + + /// + /// Returns the process ids of the GVFS.Mount processes whose command line + /// references this enlistment root. Uses PowerShell's Get-CimInstance to + /// read command lines without requiring System.Management. Best-effort: + /// returns an empty list on any failure (e.g. non-Windows). + /// + private List GetMountProcessIds() + { + List processIds = new List(); + try { - // Find GVFS.Mount processes whose command line contains this - // enlistment root. Uses PowerShell's Get-CimInstance to read - // command lines without requiring System.Management. - string filter = this.EnlistmentRoot.Replace("\\", "\\\\"); + // Match on the enlistment's unique leaf folder id rather than the + // full path. PowerShell's -like treats '\' as a literal (not an + // escape), so doubling backslashes in the full path would produce a + // pattern that never matches a real (single-backslash) command line. + // The leaf id is unique and free of path separators and wildcard + // metacharacters, so it needs no escaping. + string filter = Path.GetFileName(this.EnlistmentRoot.TrimEnd('\\', '/')); var psi = new System.Diagnostics.ProcessStartInfo("powershell.exe") { Arguments = $"-NoProfile -Command \"Get-CimInstance Win32_Process -Filter \\\"Name='GVFS.Mount.exe'\\\" | Where-Object {{ $_.CommandLine -like '*{filter}*' }} | ForEach-Object {{ $_.ProcessId }}\"", @@ -333,30 +458,67 @@ public void KillMountProcess() UseShellExecute = false, CreateNoWindow = true, }; - var proc = System.Diagnostics.Process.Start(psi); - string output = proc.StandardOutput.ReadToEnd(); - proc.WaitForExit(10000); - foreach (string line in output.Split('\n', StringSplitOptions.RemoveEmptyEntries)) + var output = new System.Text.StringBuilder(); + + // Read output asynchronously via the event, rather than a blocking ReadToEnd() before + // WaitForExit(): ReadToEnd() blocks until the process closes its stdout handle, so if the + // helper itself hangs, the later WaitForExit(10000) timeout is never reached at all. With + // async reads, WaitForExit is the only blocking call, so it enforces a real timeout and we + // can kill the helper if it does not exit in time. + using (var proc = new System.Diagnostics.Process { StartInfo = psi }) { - if (int.TryParse(line.Trim(), out int pid)) + proc.OutputDataReceived += (sender, args) => { - Console.Error.WriteLine($"[TEARDOWN] Killing GVFS.Mount (PID {pid}) for {this.EnlistmentRoot}"); + if (args.Data != null) + { + output.AppendLine(args.Data); + } + }; + + proc.Start(); + proc.BeginOutputReadLine(); + + if (!proc.WaitForExit(10000)) + { + Console.Error.WriteLine("[TEARDOWN] GetMountProcessIds helper timed out; killing it"); try { - System.Diagnostics.Process.GetProcessById(pid)?.Kill(); + proc.Kill(); + proc.WaitForExit(2000); } - catch (Exception ex) + catch (Exception killEx) { - Console.Error.WriteLine($"[TEARDOWN] Failed to kill PID {pid}: {ex.Message}"); + Console.Error.WriteLine($"[TEARDOWN] Failed to kill GetMountProcessIds helper: {killEx.Message}"); } } + else + { + // WaitForExit(int) only guarantees the process has exited - it does NOT guarantee + // that all queued OutputDataReceived callbacks have run yet, since those fire on + // the thread pool as data arrives. Reading `output` right here could race the last + // callback(s) and silently drop a trailing PID line. The parameterless WaitForExit() + // is documented to block until the redirected stream's async reads have completed, + // so calling it again (a no-op once the process has exited) drains any pending + // callbacks before we parse output below. + proc.WaitForExit(); + } + } + + foreach (string line in output.ToString().Split('\n', StringSplitOptions.RemoveEmptyEntries)) + { + if (int.TryParse(line.Trim(), out int pid)) + { + processIds.Add(pid); + } } } catch (Exception ex) { - Console.Error.WriteLine($"[TEARDOWN] KillMountProcess failed: {ex.Message}"); + Console.Error.WriteLine($"[TEARDOWN] GetMountProcessIds failed: {ex.Message}"); } + + return processIds; } public string GetVirtualPathTo(string path) diff --git a/GVFS/GVFS.FunctionalTests/Tools/MiniDump.cs b/GVFS/GVFS.FunctionalTests/Tools/MiniDump.cs new file mode 100644 index 0000000000..57356f44cd --- /dev/null +++ b/GVFS/GVFS.FunctionalTests/Tools/MiniDump.cs @@ -0,0 +1,88 @@ +using System; +using System.Diagnostics; +using System.IO; +using System.Runtime.InteropServices; + +namespace GVFS.FunctionalTests.Tools +{ + /// + /// Best-effort process minidump writer used to capture post-mortem state of a + /// (potentially hung) GVFS.Mount process when a functional test fails. Windows + /// only; a no-op that returns false on other platforms. Never throws. + /// + public static class MiniDump + { + [Flags] + private enum MiniDumpType : uint + { + Normal = 0x00000000, + WithFullMemory = 0x00000002, + WithHandleData = 0x00000004, + WithFullMemoryInfo = 0x00000800, + WithThreadInfo = 0x00001000, + } + + /// + /// Writes a full-memory minidump of the process with the given id to + /// . A full-memory dump is required so + /// that managed call stacks are resolvable in WinDbg/SOS, which is what we + /// need to diagnose a mount deadlock. Returns true on success. + /// + public static bool TryWrite(int processId, string destinationPath) + { + if (!RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) + { + Console.Error.WriteLine($"[DIAGNOSTICS] MiniDump skipped (non-Windows) for PID {processId}"); + return false; + } + + try + { + using (Process process = Process.GetProcessById(processId)) + using (FileStream dumpFile = new FileStream(destinationPath, FileMode.Create, FileAccess.ReadWrite, FileShare.Write)) + { + MiniDumpType dumpType = + MiniDumpType.WithFullMemory | + MiniDumpType.WithHandleData | + MiniDumpType.WithThreadInfo | + MiniDumpType.WithFullMemoryInfo; + + bool succeeded = MiniDumpWriteDump( + process.Handle, + (uint)process.Id, + dumpFile.SafeFileHandle, + dumpType, + IntPtr.Zero, + IntPtr.Zero, + IntPtr.Zero); + + if (!succeeded) + { + int error = Marshal.GetLastWin32Error(); + Console.Error.WriteLine($"[DIAGNOSTICS] MiniDumpWriteDump failed for PID {processId} (Win32 error {error})"); + return false; + } + + Console.Error.WriteLine($"[DIAGNOSTICS] Wrote minidump for GVFS.Mount PID {processId} to {destinationPath}"); + return true; + } + } + catch (Exception ex) + { + Console.Error.WriteLine($"[DIAGNOSTICS] Failed to write minidump for PID {processId}: {ex.Message}"); + return false; + } + } + + [DllImport("dbghelp.dll", SetLastError = true)] + [return: MarshalAs(UnmanagedType.Bool)] + private static extern bool MiniDumpWriteDump( + IntPtr hProcess, + uint processId, + SafeHandle hFile, + MiniDumpType dumpType, + IntPtr exceptionParam, + IntPtr userStreamParam, + IntPtr callbackParam); + } +} From d2adb5aadefcdc46c50902ebcbf87daf044e97df Mon Sep 17 00:00:00 2001 From: Tyrie Vella Date: Wed, 5 Aug 2026 13:25:38 -0700 Subject: [PATCH 02/11] Split enumeration-miss cause and de-duplicate the error telemetry GetDirectoryEnumeration logs "Failed to find active enumeration ID" when an enumeration ID is absent. Every non-eviction miss carried the reason Unknown, which hid two different causes: - EndedRecently: ProjFS delivered a Get that raced or followed the End for the same enumeration (a benign kernel close/query race). - NeverSeen: GVFS never held the ID (it never started, or it predates a provider restart). The classification, the once-per-ID error de-duplication, and the bounded tracking maps are extracted into a new EnumerationFailureTracker class (mirroring the MissingTreeTracker pattern) so the policy is cohesive and unit testable on its own. The tracker owns all three "why is this ID absent" maps - recently evicted, recently ended, and recently reported - and exposes RecordEvicted, RecordEnded, ClassifyMiss, and TryReserveReport. The virtualizer records an end (and an eviction) before removing the ID from activeEnumerations, so a racing Get always finds the ID in one collection or the other; ClassifyMiss attributes Evicted (most actionable), then EndedRecently, then NeverSeen. The old Unknown value is renamed NeverSeen. When an End removes nothing from the active set and the ID was not evicted, GVFS never actually held it, so the ended marker recorded before the removal is undone (UndoEnded) - keeping the record-before-remove ordering for the normal path while avoiding a later Get being skewed to EndedRecently for an ID that was never seen. De-duplicate the error: a caller that re-enumerates a lost handle can emit the same error a very large number of times on one machine. The tracker emits the full error once per ID within a window; the first occurrence still logs at Error, so the machine-based signal stays intact. The returned HResult does not change. The tracker prunes its maps on a throttle from every record point, so no map can grow unbounded when one callback (e.g. End) stops arriving - the never-ended scenario this instrumentation targets. It keeps lock-free ConcurrentDictionary state; a coarse lock would serialize the hot enumeration path. The EnumerationFailureReason values are a case-sensitive contract consumed by the release-readiness telemetry dashboard; its cause bucketing must add EndedRecently and NeverSeen. Tests: EnumerationFailureTracker is unit-tested directly (classification, Evicted-over-EndedRecently precedence, eviction undo, ended undo, dedup, prune/retention); the virtualizer tests cover the wiring, the record-before-remove ordering, the Evicted-over-EndedRecently precedence, and that an End for a never-held ID does not skew a later Get to EndedRecently. Assisted-by: Claude Opus 4.8 Signed-off-by: Tyrie Vella --- .../EnumerationFailureTracker.cs | 230 ++++++++++++++++++ .../WindowsFileSystemVirtualizer.cs | 115 +++++---- .../Windows/EnumerationFailureTrackerTests.cs | 190 +++++++++++++++ .../WindowsFileSystemVirtualizerTests.cs | 116 ++++++++- 4 files changed, 596 insertions(+), 55 deletions(-) create mode 100644 GVFS/GVFS.Platform.Windows/EnumerationFailureTracker.cs create mode 100644 GVFS/GVFS.UnitTests/Windows/EnumerationFailureTrackerTests.cs diff --git a/GVFS/GVFS.Platform.Windows/EnumerationFailureTracker.cs b/GVFS/GVFS.Platform.Windows/EnumerationFailureTracker.cs new file mode 100644 index 0000000000..04d02d700b --- /dev/null +++ b/GVFS/GVFS.Platform.Windows/EnumerationFailureTracker.cs @@ -0,0 +1,230 @@ +using System; +using System.Collections.Concurrent; +using System.Collections.Generic; +using System.Threading; + +namespace GVFS.Platform.Windows +{ + /// + /// Why a directory-enumeration Get failed to find its enumeration ID. Recorded on the failure + /// telemetry so self-inflicted causes can be told apart from ProjFS races outside gvfs.exe's + /// control. These values are a case-sensitive contract consumed by the release-readiness + /// telemetry dashboard; keep them in sync with its cause bucketing. + /// + public enum EnumerationFailureReason + { + NeverSeen = 0, // ProjFS delivered an ID GVFS never held: never started, or from before a provider restart (outside gvfs.exe's control). + Evicted, // GVFS's own stale-enumeration eviction removed a live enumeration (self-inflicted). + EndedRecently, // ProjFS delivered a Get racing or following the End for the same enumeration - a benign close/query race (outside gvfs.exe's control). + } + + /// + /// Tracks the state needed to classify and rate-limit "Failed to find active enumeration ID" + /// failures, keyed by ProjFS enumeration GUID: + /// + /// - Recently ended IDs, so a Get that races or follows the End for the same enumeration is + /// attributed to a benign close/query race () + /// rather than an ID GVFS never held. + /// - Recently reported IDs, so the error is emitted once per ID within a window instead of once + /// per retry when a caller re-enumerates a lost handle in a loop. + /// + /// Eviction (a separate concern owned by the virtualizer) is passed in to + /// as a flag rather than tracked here. + /// + /// Thread-safety: the enumeration callbacks run concurrently on many ProjFS worker threads, so + /// this deliberately uses lock-free state rather + /// than a coarse lock, which would serialize the hot enumeration path. GUIDs are never reused, so + /// entries are bounded purely by age. + /// + public class EnumerationFailureTracker + { + // ProjFS can deliver a Get that races the End for the same handle (a query in flight while the + // directory handle is closing, or the querying process dying mid-enumeration). Ended IDs are + // retained this long so such a Get is attributed to a recently-ended enumeration. + private static readonly TimeSpan DefaultRecentlyEndedRetention = TimeSpan.FromSeconds(30); + + // A Get miss for the same ID can repeat in a tight loop (a caller re-enumerating a handle + // whose Start GVFS lost, e.g. across a provider restart). The error is emitted once per ID + // within this window so the machine-based signal survives without the per-machine event storm. + private static readonly TimeSpan DefaultReportedMissingRetention = TimeSpan.FromMinutes(5); + + // GVFS's own stale-enumeration eviction removes a live enumeration that ProjFS never ended. + // Evicted IDs are retained this long so a later Get for one is attributed to eviction rather + // than a never-held ID. This is twice the default stale-enumeration timeout (5 minutes), the + // window the virtualizer's eviction sweep uses to decide an enumeration is stale. + private static readonly TimeSpan DefaultRecentlyEvictedRetention = TimeSpan.FromMinutes(10); + + // Throttle for the age-based prune. The prune runs from every record point (RecordEnded, + // RecordEvicted, TryReserveReport), so the maps stay bounded even if one callback (e.g. End) + // stops arriving. + private static readonly TimeSpan DefaultPruneInterval = TimeSpan.FromSeconds(30); + + // Key: the ProjFS enumeration GUID that was ended. Value: the Environment.TickCount64 + // (monotonic milliseconds) at which EndDirectoryEnumeration recorded it. + private readonly ConcurrentDictionary recentlyEnded = new ConcurrentDictionary(); + + // Key: the ProjFS enumeration GUID for which a miss error was already emitted. Value: the + // Environment.TickCount64 (monotonic milliseconds) of that first report. + private readonly ConcurrentDictionary recentlyReportedMissing = new ConcurrentDictionary(); + + // Key: the ProjFS enumeration GUID that GVFS's stale-enumeration eviction removed. Value: the + // Environment.TickCount64 (monotonic milliseconds) at which it was evicted. + private readonly ConcurrentDictionary recentlyEvicted = new ConcurrentDictionary(); + + private readonly TimeSpan recentlyEndedRetention; + private readonly TimeSpan reportedMissingRetention; + private readonly TimeSpan recentlyEvictedRetention; + private readonly TimeSpan pruneInterval; + + // Monotonic (Environment.TickCount64, milliseconds) timestamp of the last prune. + private long lastPruneTickCount = Environment.TickCount64; + + public EnumerationFailureTracker() + : this(DefaultRecentlyEndedRetention, DefaultReportedMissingRetention, DefaultRecentlyEvictedRetention, DefaultPruneInterval) + { + } + + public EnumerationFailureTracker( + TimeSpan recentlyEndedRetention, + TimeSpan reportedMissingRetention, + TimeSpan recentlyEvictedRetention, + TimeSpan pruneInterval) + { + this.recentlyEndedRetention = recentlyEndedRetention; + this.reportedMissingRetention = reportedMissingRetention; + this.recentlyEvictedRetention = recentlyEvictedRetention; + this.pruneInterval = pruneInterval; + } + + /// + /// Records that an enumeration has ended. The caller MUST call this before removing the ID from + /// its active-enumeration collection, so a Get that races the removal always finds the ID in + /// one collection or the other and is never mis-attributed to a never-held ID. + /// + public void RecordEnded(Guid enumerationId) + { + this.MaybePrune(); + this.recentlyEnded[enumerationId] = Environment.TickCount64; + } + + /// + /// Undoes a when the End did not actually remove a live enumeration + /// (GVFS never held the ID), so a later miss is classified + /// rather than skewed to . + /// + public void UndoEnded(Guid enumerationId) + { + this.recentlyEnded.TryRemove(enumerationId, out _); + } + + /// + /// Records that GVFS's stale-enumeration eviction removed . + /// The caller MUST call this before removing the ID from its active-enumeration collection so a + /// racing Get always finds the ID in one collection or the other; if the removal then loses the + /// race (e.g. a normal End removed it first), call to undo. + /// + public void RecordEvicted(Guid enumerationId) + { + this.MaybePrune(); + this.recentlyEvicted[enumerationId] = Environment.TickCount64; + } + + /// + /// Undoes a when the eviction lost the race to remove the ID from + /// the active collection, so a miss is not mis-attributed to eviction. + /// + public void UndoEvicted(Guid enumerationId) + { + this.recentlyEvicted.TryRemove(enumerationId, out _); + } + + /// + /// Whether is currently tracked as recently evicted. + /// + public bool IsRecentlyEvicted(Guid enumerationId) + { + return this.recentlyEvicted.ContainsKey(enumerationId); + } + + /// + /// Classifies why a Get failed to find in the active + /// collection. Eviction is the most actionable (self-inflicted) cause and wins; otherwise a + /// recently-ended ID is a benign close/query race, and anything else was never held. + /// + public EnumerationFailureReason ClassifyMiss(Guid enumerationId) + { + if (this.recentlyEvicted.ContainsKey(enumerationId)) + { + return EnumerationFailureReason.Evicted; + } + + if (this.recentlyEnded.ContainsKey(enumerationId)) + { + return EnumerationFailureReason.EndedRecently; + } + + return EnumerationFailureReason.NeverSeen; + } + + /// + /// Reserves the single error report allowed for within the + /// reporting window. Returns true the first time the ID is seen missing and false for repeats, + /// so a caller's retry loop cannot produce a telemetry storm. + /// + public bool TryReserveReport(Guid enumerationId) + { + this.MaybePrune(); + return this.recentlyReportedMissing.TryAdd(enumerationId, Environment.TickCount64); + } + + // Prunes all three maps if the throttle interval has elapsed. Called from every record point so + // the maps stay bounded regardless of which callback is active. + private void MaybePrune() + { + if (this.recentlyEnded.IsEmpty && this.recentlyReportedMissing.IsEmpty && this.recentlyEvicted.IsEmpty) + { + return; + } + + long now = Environment.TickCount64; + long last = Interlocked.Read(ref this.lastPruneTickCount); + if (now - last < (long)this.pruneInterval.TotalMilliseconds) + { + return; + } + + if (Interlocked.CompareExchange(ref this.lastPruneTickCount, now, last) != last) + { + // Another thread just claimed this prune interval. + return; + } + + PruneByAge(this.recentlyEnded, now - (long)this.recentlyEndedRetention.TotalMilliseconds); + PruneByAge(this.recentlyReportedMissing, now - (long)this.reportedMissingRetention.TotalMilliseconds); + PruneByAge(this.recentlyEvicted, now - (long)this.recentlyEvictedRetention.TotalMilliseconds); + } + + private static void PruneByAge(ConcurrentDictionary map, long cutoffTickCount) + { + foreach (KeyValuePair tracked in map) + { + if (tracked.Value < cutoffTickCount) + { + map.TryRemove(tracked.Key, out _); + } + } + } + + /// + /// Test-only: runs the prune immediately, bypassing the throttle, so retention behavior can be + /// exercised deterministically. + /// + internal void PruneForTest() + { + Interlocked.Exchange( + ref this.lastPruneTickCount, + Environment.TickCount64 - (long)this.pruneInterval.TotalMilliseconds - 1); + this.MaybePrune(); + } + } +} diff --git a/GVFS/GVFS.Platform.Windows/WindowsFileSystemVirtualizer.cs b/GVFS/GVFS.Platform.Windows/WindowsFileSystemVirtualizer.cs index c99a602056..cc4ff66863 100644 --- a/GVFS/GVFS.Platform.Windows/WindowsFileSystemVirtualizer.cs +++ b/GVFS/GVFS.Platform.Windows/WindowsFileSystemVirtualizer.cs @@ -59,23 +59,15 @@ public class WindowsFileSystemVirtualizer : FileSystemVirtualizer, IRequiredCall // the throttle cannot be disturbed by wall-clock adjustments. private long lastEnumerationEvictionSweepTickCount = Environment.TickCount64; - // Enumeration IDs recently removed by EvictStaleEnumerations, mapped to the monotonic tick at - // which they were evicted. Retained briefly so a later GetDirectoryEnumeration for an evicted - // ID can be attributed to GVFS eviction (self-inflicted) rather than a ProjFS unknown-ID - // delivery. Bounded by pruning during each sweep; empty while eviction is disabled (the default). - private readonly ConcurrentDictionary recentlyEvictedEnumerations = new ConcurrentDictionary(); + // Classifies and rate-limits "Failed to find active enumeration ID" failures (evicted vs + // recently-ended vs never-seen, and once-per-ID error de-duplication). The eviction sweep in + // this class records evictions into it via RecordEvicted. + private readonly EnumerationFailureTracker enumerationFailureTracker = new EnumerationFailureTracker(); - /// - /// Why a GetDirectoryEnumeration failed to find its enumeration ID. Recorded on the failure - /// telemetry so a self-inflicted eviction can be told apart from a ProjFS unknown-ID delivery. - /// Kept in sync with the telemetry bucketing in devprod.git.telemetry - /// (gvfs-regression-signatures.kql). - /// - public enum EnumerationFailureReason - { - Unknown = 0, // ProjFS delivered an ID GVFS never held or already ended (outside gvfs.exe's control). - Evicted, // GVFS's own stale-enumeration eviction removed a live enumeration (self-inflicted). - } + // Test-only seam: invoked inside EndDirectoryEnumerationCallback after the ended ID is + // recorded but before it is removed from activeEnumerations, so a test can interleave a + // GetDirectoryEnumeration and verify the record-before-remove ordering. Null in production. + private Action enumerationEndBeforeRemoveHookForTest; public WindowsFileSystemVirtualizer(GVFSContext context, GVFSGitObjects gitObjects) : this( @@ -207,24 +199,6 @@ private void EvictStaleEnumerations() { long now = Environment.TickCount64; - // Prune the eviction-tracking map on every sweep, independent of whether an eviction - // happens this pass, so entries never outlive the window in which a stale - // GetDirectoryEnumeration could still arrive for an evicted ID. (If this ran only when - // Count > max below, the last evicted batch would linger once activity subsided.) Guids - // are never reused, so there is no need to prune on re-add. Cheap no-op while empty - // (the default, since eviction is off). - if (!this.recentlyEvictedEnumerations.IsEmpty) - { - long trackingCutoff = now - (long)(2 * this.activeEnumerationStaleTimeout.TotalMilliseconds); - foreach (KeyValuePair tracked in this.recentlyEvictedEnumerations) - { - if (tracked.Value < trackingCutoff) - { - this.recentlyEvictedEnumerations.TryRemove(tracked.Key, out _); - } - } - } - if (this.activeEnumerations.Count <= this.maxActiveEnumerations) { return; @@ -237,9 +211,9 @@ private void EvictStaleEnumerations() if (entry.Value.LastActivityTickCount < cutoff) { // Record the eviction BEFORE removing from activeEnumerations so a concurrent - // GetDirectoryEnumeration for this ID always finds it in one map or the other, - // and is never mis-attributed to a ProjFS unknown-ID delivery. - this.recentlyEvictedEnumerations[entry.Key] = now; + // GetDirectoryEnumeration for this ID always finds it in one collection or the + // other, and is never mis-attributed to a ProjFS unknown-ID delivery. + this.enumerationFailureTracker.RecordEvicted(entry.Key); if (this.activeEnumerations.TryRemove(entry.Key, out _)) { evictedCount++; @@ -248,7 +222,7 @@ private void EvictStaleEnumerations() { // Lost the race (e.g. a normal EndDirectoryEnumeration removed it first); // it was not evicted by us, so undo the tracking entry. - this.recentlyEvictedEnumerations.TryRemove(entry.Key, out _); + this.enumerationFailureTracker.UndoEvicted(entry.Key); } } } @@ -278,6 +252,18 @@ internal int MaxActiveEnumerationsForTest set { this.maxActiveEnumerations = value; } } + internal Action EnumerationEndBeforeRemoveHookForTest + { + set { this.enumerationEndBeforeRemoveHookForTest = value; } + } + + internal bool ActiveEnumerationsContainsForTest(Guid enumerationId) + { + return this.activeEnumerations.ContainsKey(enumerationId); + } + + internal EnumerationFailureTracker EnumerationFailureTrackerForTest => this.enumerationFailureTracker; + /// /// Test-only: resets the sweep throttle and runs the same eviction path the enumeration hot /// callback runs, so eviction behavior can be exercised deterministically. @@ -511,20 +497,25 @@ public HResult GetDirectoryEnumerationCallback( ActiveEnumeration activeEnumeration = null; if (!this.activeEnumerations.TryGetValue(enumerationId, out activeEnumeration)) { - EventMetadata metadata = this.CreateEventMetadata(enumerationId); - metadata.Add("filterFileName", filterFileName); - metadata.Add("restartScan", restartScan); - - // Distinguish a failure caused by GVFS's own stale-enumeration eviction - // (self-inflicted, fixable) from ProjFS delivering an ID GVFS never held or - // already ended (outside gvfs.exe's control). Kept in sync with the telemetry - // bucketing in devprod.git.telemetry (gvfs-regression-signatures.kql). - EnumerationFailureReason enumerationFailureReason = this.recentlyEvictedEnumerations.ContainsKey(enumerationId) - ? EnumerationFailureReason.Evicted - : EnumerationFailureReason.Unknown; - metadata.Add(nameof(EnumerationFailureReason), enumerationFailureReason.ToString()); - - this.Context.Tracer.RelatedError(metadata, nameof(this.GetDirectoryEnumerationCallback) + ": Failed to find active enumeration ID"); + // Distinguish why the ID is absent so self-inflicted causes can be told apart from + // ProjFS races outside gvfs.exe's control. The tracker attributes eviction (the + // only cause GVFS can act on), a recent End (a benign close/query race), or a + // never-held ID. + EnumerationFailureReason enumerationFailureReason = this.enumerationFailureTracker.ClassifyMiss(enumerationId); + + // Emit the full error only the first time a given ID is seen missing; the tracker + // suppresses the duplicate telemetry a caller's retry loop would otherwise generate + // (a single stuck enumeration has produced a very large number of events per machine + // in the field). The machine-based regression signal is preserved because the first + // occurrence still logs at Error. + if (this.enumerationFailureTracker.TryReserveReport(enumerationId)) + { + EventMetadata metadata = this.CreateEventMetadata(enumerationId); + metadata.Add("filterFileName", filterFileName); + metadata.Add("restartScan", restartScan); + metadata.Add(nameof(EnumerationFailureReason), enumerationFailureReason.ToString()); + this.Context.Tracer.RelatedError(metadata, nameof(this.GetDirectoryEnumerationCallback) + ": Failed to find active enumeration ID"); + } return HResult.InternalError; } @@ -597,9 +588,29 @@ public HResult EndDirectoryEnumerationCallback(Guid enumerationId) { try { + // Record the end BEFORE removing from activeEnumerations so a GetDirectoryEnumeration + // that races this end - ProjFS can deliver an in-flight Get concurrently with the + // handle-close End for the same enumeration - is attributed to a recently-ended + // enumeration rather than an ID GVFS never held. RecordEnded also prunes the tracking + // maps on a throttle, so they stay bounded from this path. + this.enumerationFailureTracker.RecordEnded(enumerationId); + + this.enumerationEndBeforeRemoveHookForTest?.Invoke(); + ActiveEnumeration activeEnumeration; if (!this.activeEnumerations.TryRemove(enumerationId, out activeEnumeration)) { + // This End removed nothing from the active set. If GVFS's own eviction removed the + // ID, keep the ended marker (Evicted wins classification regardless). Otherwise GVFS + // never actually held this ID, so undo the marker recorded above, so a later Get is + // classified NeverSeen rather than skewed to EndedRecently. The record-before-remove + // ordering still holds for the normal successful-remove path, so a Get racing a real + // End stays race-safe. + if (!this.enumerationFailureTracker.IsRecentlyEvicted(enumerationId)) + { + this.enumerationFailureTracker.UndoEnded(enumerationId); + } + this.Context.Tracer.RelatedWarning( this.CreateEventMetadata(enumerationId), nameof(this.EndDirectoryEnumerationCallback) + ": Failed to remove enumeration ID from active collection", diff --git a/GVFS/GVFS.UnitTests/Windows/EnumerationFailureTrackerTests.cs b/GVFS/GVFS.UnitTests/Windows/EnumerationFailureTrackerTests.cs new file mode 100644 index 0000000000..7db8a6f9c5 --- /dev/null +++ b/GVFS/GVFS.UnitTests/Windows/EnumerationFailureTrackerTests.cs @@ -0,0 +1,190 @@ +using System; +using GVFS.Platform.Windows; +using GVFS.Tests.Should; +using NUnit.Framework; + +namespace GVFS.UnitTests.Windows +{ + [TestFixture] + public class EnumerationFailureTrackerTests + { + // Retention/interval used by the classification and dedup tests, where entries must survive + // for the duration of the test (the default 30s throttle keeps the auto-prune from firing). + private static EnumerationFailureTracker CreateTracker() + { + return new EnumerationFailureTracker(); + } + + // Retention set to already-expired so a forced prune reclaims every entry deterministically, + // without any Thread.Sleep. The interval is left at a normal value; PruneForTest bypasses it. + private static EnumerationFailureTracker CreateImmediatelyExpiringTracker() + { + return new EnumerationFailureTracker( + recentlyEndedRetention: TimeSpan.FromMilliseconds(-1), + reportedMissingRetention: TimeSpan.FromMilliseconds(-1), + recentlyEvictedRetention: TimeSpan.FromMilliseconds(-1), + pruneInterval: TimeSpan.FromSeconds(30)); + } + + [TestCase] + public void ClassifyMiss_UnknownIdIsNeverSeen() + { + EnumerationFailureTracker tracker = CreateTracker(); + + tracker.ClassifyMiss(Guid.NewGuid()).ShouldEqual(EnumerationFailureReason.NeverSeen); + } + + [TestCase] + public void ClassifyMiss_RecordedEndIsEndedRecently() + { + EnumerationFailureTracker tracker = CreateTracker(); + Guid id = Guid.NewGuid(); + + tracker.RecordEnded(id); + + tracker.ClassifyMiss(id).ShouldEqual(EnumerationFailureReason.EndedRecently); + } + + [TestCase] + public void ClassifyMiss_RecordedEvictionIsEvicted() + { + EnumerationFailureTracker tracker = CreateTracker(); + Guid id = Guid.NewGuid(); + + tracker.RecordEvicted(id); + + tracker.ClassifyMiss(id).ShouldEqual(EnumerationFailureReason.Evicted); + } + + [TestCase] + public void ClassifyMiss_EvictionWinsOverRecordedEnd() + { + EnumerationFailureTracker tracker = CreateTracker(); + Guid id = Guid.NewGuid(); + + // Same ID present as both evicted and ended (the real case: eviction removed it, then a + // late End recorded it). Eviction is the more actionable cause and must win. + tracker.RecordEvicted(id); + tracker.RecordEnded(id); + + tracker.ClassifyMiss(id).ShouldEqual(EnumerationFailureReason.Evicted); + } + + [TestCase] + public void UndoEvicted_UndoesEviction() + { + EnumerationFailureTracker tracker = CreateTracker(); + Guid id = Guid.NewGuid(); + + // Eviction recorded the ID before removing it from the active set, then lost the race, so + // it undoes the record. The ID must no longer be attributed to eviction. + tracker.RecordEvicted(id); + tracker.UndoEvicted(id); + + tracker.ClassifyMiss(id).ShouldEqual(EnumerationFailureReason.NeverSeen); + } + + [TestCase] + public void UndoEnded_ReclassifiesAsNeverSeen() + { + EnumerationFailureTracker tracker = CreateTracker(); + Guid id = Guid.NewGuid(); + + // End recorded the ID before removing it, but the removal found nothing (GVFS never held + // it), so it undoes the record. The ID must classify NeverSeen, not EndedRecently. + tracker.RecordEnded(id); + tracker.ClassifyMiss(id).ShouldEqual(EnumerationFailureReason.EndedRecently); + + tracker.UndoEnded(id); + tracker.ClassifyMiss(id).ShouldEqual(EnumerationFailureReason.NeverSeen); + } + + [TestCase] + public void IsRecentlyEvicted_TrueOnlyAfterRecordEvicted() + { + EnumerationFailureTracker tracker = CreateTracker(); + Guid id = Guid.NewGuid(); + + tracker.IsRecentlyEvicted(id).ShouldBeFalse(); + tracker.RecordEvicted(id); + tracker.IsRecentlyEvicted(id).ShouldBeTrue(); + tracker.UndoEvicted(id); + tracker.IsRecentlyEvicted(id).ShouldBeFalse(); + } + + [TestCase] + public void TryReserveReport_ReturnsTrueOnceThenFalseForSameId() + { + EnumerationFailureTracker tracker = CreateTracker(); + Guid id = Guid.NewGuid(); + + tracker.TryReserveReport(id).ShouldBeTrue(); + tracker.TryReserveReport(id).ShouldBeFalse(); + tracker.TryReserveReport(id).ShouldBeFalse(); + } + + [TestCase] + public void TryReserveReport_IndependentPerId() + { + EnumerationFailureTracker tracker = CreateTracker(); + + tracker.TryReserveReport(Guid.NewGuid()).ShouldBeTrue(); + tracker.TryReserveReport(Guid.NewGuid()).ShouldBeTrue(); + } + + [TestCase] + public void PruneRemovesAgedEntries() + { + EnumerationFailureTracker tracker = CreateImmediatelyExpiringTracker(); + Guid id = Guid.NewGuid(); + + // Populate the maps. The default-interval throttle keeps the auto-prune inside RecordEnded, + // RecordEvicted and TryReserveReport from firing yet, so the entries are present. + tracker.RecordEnded(id); + tracker.TryReserveReport(id).ShouldBeTrue(); + tracker.ClassifyMiss(id).ShouldEqual(EnumerationFailureReason.EndedRecently); + tracker.TryReserveReport(id).ShouldBeFalse(); + + // Force the prune past the throttle: both aged entries are reclaimed. + tracker.PruneForTest(); + + // The ended entry is gone (now NeverSeen) and the dedup entry is gone (can report again). + tracker.ClassifyMiss(id).ShouldEqual(EnumerationFailureReason.NeverSeen); + tracker.TryReserveReport(id).ShouldBeTrue(); + } + + [TestCase] + public void PruneRemovesAgedEviction() + { + EnumerationFailureTracker tracker = CreateImmediatelyExpiringTracker(); + Guid id = Guid.NewGuid(); + + tracker.RecordEvicted(id); + tracker.ClassifyMiss(id).ShouldEqual(EnumerationFailureReason.Evicted); + + tracker.PruneForTest(); + + tracker.ClassifyMiss(id).ShouldEqual(EnumerationFailureReason.NeverSeen); + } + + [TestCase] + public void PruneKeepsEntriesWithinRetention() + { + // Long retention: a forced prune must NOT remove fresh entries. + EnumerationFailureTracker tracker = new EnumerationFailureTracker( + recentlyEndedRetention: TimeSpan.FromMinutes(10), + reportedMissingRetention: TimeSpan.FromMinutes(10), + recentlyEvictedRetention: TimeSpan.FromMinutes(10), + pruneInterval: TimeSpan.FromSeconds(30)); + Guid id = Guid.NewGuid(); + + tracker.RecordEnded(id); + tracker.TryReserveReport(id).ShouldBeTrue(); + + tracker.PruneForTest(); + + tracker.ClassifyMiss(id).ShouldEqual(EnumerationFailureReason.EndedRecently); + tracker.TryReserveReport(id).ShouldBeFalse(); + } + } +} diff --git a/GVFS/GVFS.UnitTests/Windows/Virtualization/WindowsFileSystemVirtualizerTests.cs b/GVFS/GVFS.UnitTests/Windows/Virtualization/WindowsFileSystemVirtualizerTests.cs index bfd4a0e094..3a98649b7e 100644 --- a/GVFS/GVFS.UnitTests/Windows/Virtualization/WindowsFileSystemVirtualizerTests.cs +++ b/GVFS/GVFS.UnitTests/Windows/Virtualization/WindowsFileSystemVirtualizerTests.cs @@ -328,7 +328,7 @@ public void StaleEnumerationsAreEvictedWhenEnabledButLiveOnesAreKept() } [TestCase] - public void GetDirectoryEnumerationTagsEvictedVersusUnknownId() + public void GetDirectoryEnumerationTagsMissReasonAndDeduplicates() { using (WindowsFileSystemVirtualizerTester tester = new WindowsFileSystemVirtualizerTester(this.Repo, new[] { "test" })) { @@ -355,11 +355,121 @@ public void GetDirectoryEnumerationTagsEvictedVersusUnknownId() mockTracker.RelatedErrorEvents.Any( e => e.Contains("Failed to find active enumeration ID") && e.Contains("\"EnumerationFailureReason\":\"Evicted\"")).ShouldBeTrue(); - // A Get for an ID GVFS never held is attributed to a ProjFS unknown-ID delivery. + // A Get for an ID GVFS never held is attributed to a never-seen delivery. Guid neverSeenId = Guid.NewGuid(); tester.MockVirtualization.RequiredCallbacks.GetDirectoryEnumerationCallback(4, neverSeenId, string.Empty, false, null).ShouldEqual(HResult.InternalError); mockTracker.RelatedErrorEvents.Any( - e => e.Contains("Failed to find active enumeration ID") && e.Contains("\"EnumerationFailureReason\":\"Unknown\"")).ShouldBeTrue(); + e => e.Contains("Failed to find active enumeration ID") && e.Contains("\"EnumerationFailureReason\":\"NeverSeen\"")).ShouldBeTrue(); + + // A Get that races/follows the End for the same enumeration is attributed to a benign + // close/query race, not a never-seen delivery. + Guid endedId = Guid.NewGuid(); + tester.MockVirtualization.RequiredCallbacks.StartDirectoryEnumerationCallback(5, endedId, "test", TriggeringProcessId, TriggeringProcessImageFileName).ShouldEqual(HResult.Ok); + tester.MockVirtualization.RequiredCallbacks.EndDirectoryEnumerationCallback(endedId).ShouldEqual(HResult.Ok); + tester.MockVirtualization.RequiredCallbacks.GetDirectoryEnumerationCallback(6, endedId, string.Empty, false, null).ShouldEqual(HResult.InternalError); + mockTracker.RelatedErrorEvents.Any( + e => e.Contains("Failed to find active enumeration ID") && e.Contains("\"EnumerationFailureReason\":\"EndedRecently\"")).ShouldBeTrue(); + + // Repeated Gets for the same missing ID are de-duplicated: the error is emitted once, + // so a caller's retry loop cannot produce a telemetry storm. + int errorsForNeverSeenId = mockTracker.RelatedErrorEvents.Count(e => e.Contains("\"EnumerationFailureReason\":\"NeverSeen\"")); + tester.MockVirtualization.RequiredCallbacks.GetDirectoryEnumerationCallback(7, neverSeenId, string.Empty, false, null).ShouldEqual(HResult.InternalError); + tester.MockVirtualization.RequiredCallbacks.GetDirectoryEnumerationCallback(8, neverSeenId, string.Empty, false, null).ShouldEqual(HResult.InternalError); + mockTracker.RelatedErrorEvents.Count(e => e.Contains("\"EnumerationFailureReason\":\"NeverSeen\"")).ShouldEqual(errorsForNeverSeenId); + } + } + + [TestCase] + public void EndDirectoryEnumerationRecordsEndedBeforeRemovingFromActive() + { + using (WindowsFileSystemVirtualizerTester tester = new WindowsFileSystemVirtualizerTester(this.Repo, new[] { "test" })) + { + tester.GitIndexProjection.EnumerationInMemory = true; + MockTracer mockTracker = this.Repo.Context.Tracer as MockTracer; + + Guid endedId = Guid.NewGuid(); + tester.MockVirtualization.RequiredCallbacks.StartDirectoryEnumerationCallback(1, endedId, "test", TriggeringProcessId, TriggeringProcessImageFileName).ShouldEqual(HResult.Ok); + + // Capture the state at the exact interleaving point a concurrent Get would observe: + // after End records the ended ID but before it removes it from activeEnumerations. This + // is the ordering the fix guarantees; a remove-before-record regression would fail it. + bool activeAtHook = false; + bool recentlyEndedAtHook = false; + tester.WindowsVirtualizer.EnumerationEndBeforeRemoveHookForTest = () => + { + activeAtHook = tester.WindowsVirtualizer.ActiveEnumerationsContainsForTest(endedId); + recentlyEndedAtHook = tester.WindowsVirtualizer.EnumerationFailureTrackerForTest.ClassifyMiss(endedId) == EnumerationFailureReason.EndedRecently; + }; + + tester.MockVirtualization.RequiredCallbacks.EndDirectoryEnumerationCallback(endedId).ShouldEqual(HResult.Ok); + + // The end was recorded before the removal, and the entry was still live at that point, + // so there is no window where the ID is absent from BOTH maps - a racing Get can never + // be misclassified NeverSeen. + recentlyEndedAtHook.ShouldBeTrue(); + activeAtHook.ShouldBeTrue(); + + // After End completes the ID is out of the active set but still tracked as recently ended. + tester.WindowsVirtualizer.ActiveEnumerationsContainsForTest(endedId).ShouldBeFalse(); + tester.WindowsVirtualizer.EnumerationFailureTrackerForTest.ClassifyMiss(endedId).ShouldEqual(EnumerationFailureReason.EndedRecently); + mockTracker.RelatedErrorEvents.Any(e => e.Contains("Failed to find active enumeration ID")).ShouldBeFalse(); + } + } + + [TestCase] + public void GetDirectoryEnumerationPrefersEvictedWhenIdIsBothEvictedAndEnded() + { + using (WindowsFileSystemVirtualizerTester tester = new WindowsFileSystemVirtualizerTester(this.Repo, new[] { "test" })) + { + tester.GitIndexProjection.EnumerationInMemory = true; + 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); + + Thread.Sleep(200); + + Guid freshId = Guid.NewGuid(); + tester.MockVirtualization.RequiredCallbacks.StartDirectoryEnumerationCallback(2, freshId, "test", TriggeringProcessId, TriggeringProcessImageFileName).ShouldEqual(HResult.Ok); + + // Evict staleId: it lands in the recently-evicted map and leaves the active set. + tester.WindowsVirtualizer.ForceEnumerationEvictionSweepForTest(); + + // A late End for the same ID also records it in the recently-ended map (the removal + // itself fails because eviction already removed it), so the ID is now in BOTH maps. + tester.MockVirtualization.RequiredCallbacks.EndDirectoryEnumerationCallback(staleId).ShouldEqual(HResult.InternalError); + + MockTracer mockTracker = this.Repo.Context.Tracer as MockTracer; + + // The classifier checks eviction first, so the more actionable self-inflicted cause wins. + tester.MockVirtualization.RequiredCallbacks.GetDirectoryEnumerationCallback(3, staleId, string.Empty, false, null).ShouldEqual(HResult.InternalError); + mockTracker.RelatedErrorEvents.Any( + e => e.Contains("Failed to find active enumeration ID") && e.Contains("\"EnumerationFailureReason\":\"Evicted\"")).ShouldBeTrue(); + mockTracker.RelatedErrorEvents.Any( + e => e.Contains("Failed to find active enumeration ID") && e.Contains("\"EnumerationFailureReason\":\"EndedRecently\"")).ShouldBeFalse(); + } + } + + [TestCase] + public void EndForNeverHeldIdDoesNotSkewLaterGetToEndedRecently() + { + using (WindowsFileSystemVirtualizerTester tester = new WindowsFileSystemVirtualizerTester(this.Repo, new[] { "test" })) + { + tester.GitIndexProjection.EnumerationInMemory = true; + MockTracer mockTracker = this.Repo.Context.Tracer as MockTracer; + + // End arrives for an ID GVFS never held (no prior Start), so the removal finds nothing + // and it was not evicted. The ended marker recorded before the removal must be undone. + Guid neverHeldId = Guid.NewGuid(); + tester.MockVirtualization.RequiredCallbacks.EndDirectoryEnumerationCallback(neverHeldId).ShouldEqual(HResult.InternalError); + + // A later Get for that ID is therefore classified NeverSeen, not skewed to EndedRecently. + tester.MockVirtualization.RequiredCallbacks.GetDirectoryEnumerationCallback(1, neverHeldId, string.Empty, false, null).ShouldEqual(HResult.InternalError); + mockTracker.RelatedErrorEvents.Any( + e => e.Contains("Failed to find active enumeration ID") && e.Contains("\"EnumerationFailureReason\":\"NeverSeen\"")).ShouldBeTrue(); + mockTracker.RelatedErrorEvents.Any( + e => e.Contains("Failed to find active enumeration ID") && e.Contains("\"EnumerationFailureReason\":\"EndedRecently\"")).ShouldBeFalse(); } } From 6ba0bb2ac30cccadd560ef294836e1ffcc74c694 Mon Sep 17 00:00:00 2001 From: Tyrie Vella Date: Fri, 7 Aug 2026 15:51:24 -0700 Subject: [PATCH 03/11] Share transient libgit2 config lookup helper Add LibGit2Repo.GetConfigBoolOrDefault(...) (instance + static overloads) for one-off boolean config reads, replacing scattered short-lived LibGit2Repo/LibGit2RepoInvoker usage at 4 call sites: - GVFS/CommandLine/CloneVerb.cs (gvfs.trust-pack-indexes) - GVFS.Hooks/Program.cs (gvfs.show-hydration-status) - GVFS.Mount/InProcessMount.cs (gvfs.background-cache-auth) - GVFS/CommandLine/PrefetchVerb.cs (gvfs.prefetch-offload) LibGit2RepoInvoker.InitializeSharedRepo() intentionally forces an object-store probe so long-lived/shared callers can amortize object-store load costs. That is wasted work for one-off config reads that immediately dispose the repo. The helper methods live directly on LibGit2Repo rather than a separate extension class, matching the repo.GetConfigBoolOrDefault(name, default) convention already documented in AGENTS.md, and avoiding unnecessary indirection for a class the team owns in the same assembly. Both methods fall back to defaultValue and log a RelatedWarning on any failure, matching the "default on any failure" contract each call site previously implemented independently. Added a protected LibGit2Repo(ITracer tracer) constructor to support test doubles that inject a mock tracer without opening a real repo. Surveyed master for other short-lived config-only LibGit2Repo/ LibGit2RepoInvoker usage; PrefetchStep.cs, GitStatusCache.cs, and GitRepo.cs were left alone because they use shared/long-lived repo access, not the transient anti-pattern this change addresses. Reviewed with an internal 6-lens review-swarm pass (correctness, security, design, tests, async-parallelism, risk-rollout); addressed all actionable findings: - Widened the shared helper's exception handling to a plain catch (Exception), restoring the "default on any failure" guarantee InProcessMount/PrefetchVerb relied on before this refactor. - Fixed a double-RelatedWarning log on the repo-open-failure path. - Replaced a hardcoded, non-portable "Z:\..." path in a unit test with a GUID-suffixed temp path. - Added test coverage for the unset-key (null-coalescing) branch and the InvalidDataException catch arm. - Simplified the parameterless constructor to delegate to the tracer-accepting one. Full unit test suite: 891 passed, 0 failed, 11 skipped (pre-existing, unrelated). Assisted-by: Claude Opus 4.8 Signed-off-by: Tyrie Vella --- GVFS/GVFS.Common/Git/LibGit2Repo.cs | 56 +++++++- GVFS/GVFS.Hooks/GVFS.Hooks.csproj | 1 - GVFS/GVFS.Hooks/Program.cs | 9 +- GVFS/GVFS.Mount/InProcessMount.cs | 26 +--- .../Common/LibGit2RepoConfigLookupTests.cs | 136 ++++++++++++++++++ GVFS/GVFS/CommandLine/CloneVerb.cs | 11 +- GVFS/GVFS/CommandLine/PrefetchVerb.cs | 18 +-- 7 files changed, 213 insertions(+), 44 deletions(-) create mode 100644 GVFS/GVFS.UnitTests/Common/LibGit2RepoConfigLookupTests.cs diff --git a/GVFS/GVFS.Common/Git/LibGit2Repo.cs b/GVFS/GVFS.Common/Git/LibGit2Repo.cs index 00bc55e73e..b21eb83b9e 100644 --- a/GVFS/GVFS.Common/Git/LibGit2Repo.cs +++ b/GVFS/GVFS.Common/Git/LibGit2Repo.cs @@ -39,8 +39,13 @@ public LibGit2Repo(ITracer tracer, string repoPath) } protected LibGit2Repo() + : this(NullTracer.Instance) { - this.Tracer = NullTracer.Instance; + } + + protected LibGit2Repo(ITracer tracer) + { + this.Tracer = tracer; } ~LibGit2Repo() @@ -327,6 +332,55 @@ public virtual string GetConfigString(string name) } } + /// + /// Reads a boolean config value from this already-open repo, falling back to + /// if the key is unset or the read fails for any reason + /// (e.g. a corrupt/unreadable config). + /// + public bool GetConfigBoolOrDefault(string key, bool defaultValue) + { + try + { + return this.GetConfigBool(key) ?? defaultValue; + } + catch (Exception e) + { + this.Tracer.RelatedWarning($"Failed to read {key} config, using default: {e.Message}"); + return defaultValue; + } + } + + /// + /// Reads a single boolean config value from the repo at , + /// opening and disposing a transient for the lookup. Prefer + /// this over for one-off config reads: + /// LibGit2RepoInvoker.InitializeSharedRepo forces the object store to load, which is + /// wasted work when all that's needed is a single config value. Falls back to + /// if the repo can't be opened or the read fails for any + /// reason. + /// + public static bool GetConfigBoolOrDefault(ITracer tracer, string repoPath, string key, bool defaultValue) + { + try + { + using (LibGit2Repo repo = new LibGit2Repo(tracer, repoPath)) + { + return repo.GetConfigBoolOrDefault(key, defaultValue); + } + } + catch (InvalidDataException) + { + // The LibGit2Repo constructor already logged a RelatedWarning with the native + // failure reason before throwing; avoid logging the same failure twice. + return defaultValue; + } + catch (Exception e) + { + tracer.RelatedWarning($"Failed to read {key} config, using default: {e.Message}"); + return defaultValue; + } + } + public void ForEachMultiVarConfig(string key, MultiVarConfigCallback callback) { if (Native.Config.GetConfig(out IntPtr configHandle, this.RepoHandle) != Native.ResultCode.Success) diff --git a/GVFS/GVFS.Hooks/GVFS.Hooks.csproj b/GVFS/GVFS.Hooks/GVFS.Hooks.csproj index 69988ac802..3b996578e7 100644 --- a/GVFS/GVFS.Hooks/GVFS.Hooks.csproj +++ b/GVFS/GVFS.Hooks/GVFS.Hooks.csproj @@ -118,4 +118,3 @@ - diff --git a/GVFS/GVFS.Hooks/Program.cs b/GVFS/GVFS.Hooks/Program.cs index 00db23872f..940b2cf37d 100644 --- a/GVFS/GVFS.Hooks/Program.cs +++ b/GVFS/GVFS.Hooks/Program.cs @@ -171,10 +171,11 @@ private static bool HasShortFlag(string arg, string flag) private static bool ConfigurationAllowsHydrationStatus() { - using (LibGit2RepoInvoker repo = new LibGit2RepoInvoker(NullTracer.Instance, normalizedCurrentDirectory)) - { - return repo.GetConfigBoolOrDefault(GVFSConstants.GitConfig.ShowHydrationStatus, GVFSConstants.GitConfig.ShowHydrationStatusDefault); - } + return LibGit2Repo.GetConfigBoolOrDefault( + NullTracer.Instance, + normalizedCurrentDirectory, + GVFSConstants.GitConfig.ShowHydrationStatus, + GVFSConstants.GitConfig.ShowHydrationStatusDefault); } /// diff --git a/GVFS/GVFS.Mount/InProcessMount.cs b/GVFS/GVFS.Mount/InProcessMount.cs index 8cf3c386fc..629be66138 100644 --- a/GVFS/GVFS.Mount/InProcessMount.cs +++ b/GVFS/GVFS.Mount/InProcessMount.cs @@ -486,27 +486,11 @@ private GVFSContext CreateContext() 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; - } + return LibGit2Repo.GetConfigBoolOrDefault( + this.tracer, + this.enlistment.WorkingDirectoryBackingRoot, + GVFSConstants.GitConfig.BackgroundCacheAuth, + GVFSConstants.GitConfig.BackgroundCacheAuthDefault); } private void ValidateMountPoints() diff --git a/GVFS/GVFS.UnitTests/Common/LibGit2RepoConfigLookupTests.cs b/GVFS/GVFS.UnitTests/Common/LibGit2RepoConfigLookupTests.cs new file mode 100644 index 0000000000..843d423b5f --- /dev/null +++ b/GVFS/GVFS.UnitTests/Common/LibGit2RepoConfigLookupTests.cs @@ -0,0 +1,136 @@ +using GVFS.Common.Git; +using GVFS.Tests.Should; +using GVFS.UnitTests.Mock.Common; +using NUnit.Framework; +using System; +using System.IO; + +namespace GVFS.UnitTests.Common +{ + [TestFixture] + public class LibGit2RepoConfigLookupTests + { + [TestCase] + public void GetConfigBoolOrDefaultOnRepoReturnsConfiguredValue() + { + MockTracer tracer = new MockTracer(); + + using (MockConfigRepo repo = new MockConfigRepo(tracer, true)) + { + bool value = repo.GetConfigBoolOrDefault("gvfs.test", false); + + value.ShouldEqual(true); + tracer.RelatedWarningEvents.Count.ShouldEqual(0); + } + } + + [TestCase] + public void GetConfigBoolOrDefaultOnRepoReturnsDefaultWhenKeyIsUnset() + { + MockTracer tracer = new MockTracer(); + + using (MockConfigRepo repo = new MockConfigRepo(tracer, (bool?)null)) + { + bool value = repo.GetConfigBoolOrDefault("gvfs.test", true); + + value.ShouldEqual(true); + tracer.RelatedWarningEvents.Count.ShouldEqual(0); + } + } + + [TestCase] + public void GetConfigBoolOrDefaultOnRepoReturnsDefaultOnLibGit2ExceptionAndLogsOnce() + { + MockTracer tracer = new MockTracer(); + + using (MockConfigRepo repo = new MockConfigRepo(tracer, new LibGit2Exception("boom"))) + { + bool value = repo.GetConfigBoolOrDefault("gvfs.test", false); + + value.ShouldEqual(false); + tracer.RelatedWarningEvents.Count.ShouldEqual(1); + tracer.RelatedWarningEvents[0].ShouldContain("Failed to read gvfs.test config, using default: boom"); + } + } + + [TestCase] + public void GetConfigBoolOrDefaultOnRepoReturnsDefaultOnInvalidDataExceptionAndLogsOnce() + { + MockTracer tracer = new MockTracer(); + + using (MockConfigRepo repo = new MockConfigRepo(tracer, new InvalidDataException("corrupt config"))) + { + bool value = repo.GetConfigBoolOrDefault("gvfs.test", false); + + value.ShouldEqual(false); + tracer.RelatedWarningEvents.Count.ShouldEqual(1); + tracer.RelatedWarningEvents[0].ShouldContain("Failed to read gvfs.test config, using default: corrupt config"); + } + } + + [TestCase] + public void GetConfigBoolOrDefaultOnPathReturnsDefaultForMissingRepoAndLogsExactlyOnce() + { + MockTracer tracer = new MockTracer(); + + // A GUID-suffixed path under the OS temp directory is guaranteed not to exist and + // does not depend on any particular drive letter being unmapped (unlike a + // hardcoded "Z:\..." path, which could resolve on a host with that drive mapped). + string missingRepoPath = Path.Combine( + Path.GetTempPath(), + "LibGit2RepoConfigLookupTests_" + Guid.NewGuid().ToString("N")); + + bool value = LibGit2Repo.GetConfigBoolOrDefault( + tracer, + missingRepoPath, + "gvfs.test", + false); + + value.ShouldEqual(false); + + // The LibGit2Repo constructor logs a RelatedWarning with the native open-failure + // reason before throwing InvalidDataException; the static helper's catch does not + // log a second time for that case (see LibGit2Repo.GetConfigBoolOrDefault), so + // exactly one warning is expected here. + tracer.RelatedWarningEvents.Count.ShouldEqual(1); + tracer.RelatedWarningEvents[0].ShouldContain("Couldn't open repo at"); + } + + private class MockConfigRepo : LibGit2Repo + { + private readonly bool? value; + private readonly Exception exceptionToThrow; + + public MockConfigRepo(MockTracer tracer, bool? value) + : base(tracer) + { + this.value = value; + } + + public MockConfigRepo(MockTracer tracer, Exception exceptionToThrow) + : base(tracer) + { + this.exceptionToThrow = exceptionToThrow; + } + + public override bool? GetConfigBool(string name) + { + if (this.exceptionToThrow != null) + { + throw this.exceptionToThrow; + } + + return this.value; + } + + // This mock never calls the base LibGit2Repo(tracer, repoPath) constructor, so it + // never initializes native libgit2 state or a real RepoHandle. Override Dispose(bool) + // to skip the base implementation's native Free/Shutdown calls, which would otherwise + // run on an uninitialized handle without a matching Init (same pattern as + // LibGit2RepoInvokerTests.MockLibGit2Repo and LibGit2RepoSafeDirectoryTests.MockSafeDirectoryRepo). + protected override void Dispose(bool disposing) + { + } + } + } +} diff --git a/GVFS/GVFS/CommandLine/CloneVerb.cs b/GVFS/GVFS/CommandLine/CloneVerb.cs index 977c5b0823..1277355ab6 100644 --- a/GVFS/GVFS/CommandLine/CloneVerb.cs +++ b/GVFS/GVFS/CommandLine/CloneVerb.cs @@ -152,7 +152,7 @@ public override void Execute() CacheServerInfo cacheServer = null; ServerGVFSConfig serverGVFSConfig = null; - bool trustPackIndexes; + bool trustPackIndexes = GVFSConstants.GitConfig.TrustPackIndexesDefault; using (JsonTracer tracer = new JsonTracer(GVFSConstants.GVFSEtwProviderName, "GVFSClone")) { @@ -248,10 +248,13 @@ public override void Execute() { tracer.RelatedError(cloneResult.ErrorMessage); } - - using (var repo = new LibGit2RepoInvoker(tracer, enlistment.WorkingDirectoryBackingRoot)) + else { - trustPackIndexes = repo.GetConfigBoolOrDefault(GVFSConstants.GitConfig.TrustPackIndexes, GVFSConstants.GitConfig.TrustPackIndexesDefault); + trustPackIndexes = LibGit2Repo.GetConfigBoolOrDefault( + tracer, + enlistment.WorkingDirectoryBackingRoot, + GVFSConstants.GitConfig.TrustPackIndexes, + GVFSConstants.GitConfig.TrustPackIndexesDefault); } } diff --git a/GVFS/GVFS/CommandLine/PrefetchVerb.cs b/GVFS/GVFS/CommandLine/PrefetchVerb.cs index 945984d62c..6fa0d91f42 100644 --- a/GVFS/GVFS/CommandLine/PrefetchVerb.cs +++ b/GVFS/GVFS/CommandLine/PrefetchVerb.cs @@ -700,19 +700,11 @@ private string GetCacheServerDisplay(CacheServerInfo cacheServer, string repoUrl private bool IsPrefetchOffloadEnabled(ITracer tracer, GVFSEnlistment enlistment) { - try - { - 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; - } + return LibGit2Repo.GetConfigBoolOrDefault( + tracer, + enlistment.WorkingDirectoryBackingRoot, + GVFSConstants.GitConfig.PrefetchOffload, + GVFSConstants.GitConfig.PrefetchOffloadDefault); } /// From 82a228d0a71447715c201df86d221085e6d4cbaf Mon Sep 17 00:00:00 2001 From: Tyrie Vella Date: Thu, 27 Aug 2026 11:58:08 -0700 Subject: [PATCH 04/11] FunctionalTests: make shared control-repo cache setup resilient The GitCommands functional tests compare a GVFS repo against a plain "control" git repo. Every control repo fetches from one machine-global bare cache. The cache was set up in a static constructor that checked Directory.Exists and then either cloned or fetched, and it discarded every git exit code. That setup was not atomic and not verified. Fixtures run in parallel and the cache path is shared by concurrent test processes on the same machine, so a process could observe a half-built clone directory (Directory.Exists is true before the clone finishes) and fetch from an incomplete repo. A transient clone or fetch failure had the same effect. The cache was then left missing branches, the failure was swallowed, and every GitCommands test failed its setup checkout with: error: pathspec 'FunctionalTests/20201014' did not match any file(s) known to git A local repro confirmed the cause: against a healthy cache 0/50 control-repo builds fail; against a cache missing the branch 50/50 fail with that exact error. Make the setup robust: - Serialize cache creation and refresh across processes with a system-wide mutex. - Build the cache atomically: clone into a temporary directory, verify the base branch is present, then move it into place so no other process sees a partial cache. - Retry transient clone and fetch failures, and rebuild the cache when verification fails. - Fail loudly with a clear message when a control repo cannot fetch or check out its branch, and retry the whole control-repo build, instead of producing a broken repo that fails 20+ tests with a confusing cascade. With the fix, a control repo that starts from a broken cache self-heals and 0/50 builds fail. Assisted-by: Claude Opus 4.8 Signed-off-by: Tyrie Vella --- .../Tools/ControlGitRepo.cs | 196 +++++++++++++++++- 1 file changed, 185 insertions(+), 11 deletions(-) diff --git a/GVFS/GVFS.FunctionalTests/Tools/ControlGitRepo.cs b/GVFS/GVFS.FunctionalTests/Tools/ControlGitRepo.cs index 807c09efdc..89272301c8 100644 --- a/GVFS/GVFS.FunctionalTests/Tools/ControlGitRepo.cs +++ b/GVFS/GVFS.FunctionalTests/Tools/ControlGitRepo.cs @@ -1,20 +1,18 @@ -using System; +using System; using System.IO; +using System.Threading; namespace GVFS.FunctionalTests.Tools { public class ControlGitRepo { + // Serializes creation and refresh of the machine-global shared cache across every + // functional-test process running on this machine. + private const string CacheMutexName = @"Global\GVFS.FunctionalTests.ControlGitRepoCache"; + static ControlGitRepo() { - if (!Directory.Exists(CachePath)) - { - GitProcess.Invoke(Environment.SystemDirectory, "clone " + GVFSTestConfig.RepoToClone + " " + CachePath + " --bare"); - } - else - { - GitProcess.Invoke(CachePath, "fetch origin +refs/*:refs/*"); - } + EnsureSharedCache(); } private ControlGitRepo(string repoUrl, string rootPath, string commitish) @@ -46,6 +44,28 @@ public static ControlGitRepo Create(string commitish = null) // IMPORTANT! These must parallel the settings in GVFSVerb:TrySetRequiredGitConfigSettings // public void Initialize() + { + const int MaxAttempts = 3; + for (int attempt = 1; attempt <= MaxAttempts; attempt++) + { + try + { + this.InitializeCore(); + return; + } + catch (Exception ex) when (attempt < MaxAttempts) + { + // Building the control repo hit a transient failure (for example the shared + // cache was being rebuilt by another process). Discard the partial repo and + // retry from a clean directory. + Console.WriteLine($"ControlGitRepo.Initialize attempt {attempt} of {MaxAttempts} failed: {ex.Message}"); + RepositoryHelpers.DeleteTestDirectory(this.RootPath); + Thread.Sleep(TimeSpan.FromSeconds(attempt)); + } + } + } + + private void InitializeCore() { Directory.CreateDirectory(this.RootPath); GitProcess.Invoke(this.RootPath, "init"); @@ -65,7 +85,15 @@ public void Initialize() GitProcess.Invoke(this.RootPath, "remote add origin " + CachePath); this.Fetch(this.Commitish); GitProcess.Invoke(this.RootPath, "branch --set-upstream " + this.Commitish + " origin/" + this.Commitish); - GitProcess.Invoke(this.RootPath, "checkout " + this.Commitish); + + ProcessResult checkoutResult = GitProcess.InvokeProcess(this.RootPath, "checkout " + this.Commitish); + if (checkoutResult.ExitCode != 0) + { + throw new InvalidOperationException( + $"Control repo failed to checkout '{this.Commitish}'. The shared control-repo cache at '{CachePath}' is likely missing the branch. " + + $"git exit code {checkoutResult.ExitCode}: {checkoutResult.Errors}"); + } + GitProcess.Invoke(this.RootPath, "branch --unset-upstream"); // Enable the ORT merge strategy @@ -74,7 +102,153 @@ public void Initialize() public void Fetch(string commitish) { - GitProcess.Invoke(this.RootPath, "fetch origin " + commitish); + ProcessResult result = InvokeGitWithRetry(this.RootPath, "fetch origin " + commitish); + if (result.ExitCode != 0) + { + throw new InvalidOperationException( + $"Control repo failed to fetch '{commitish}' from the shared cache '{CachePath}'. " + + $"git exit code {result.ExitCode}: {result.Errors}"); + } + } + + /// + /// Creates or refreshes the shared bare cache that every control repo fetches from. + /// + /// + /// The cache path is machine-global and is shared by every functional-test fixture (fixtures + /// run in parallel) and by concurrent test processes on the same machine. The previous + /// implementation checked and then either cloned or + /// fetched, and swallowed every git failure. That produced a flaky cascade: a transient + /// clone or fetch failure, or a concurrent process that observed a half-built clone + /// directory, left the cache missing branches. Every GitCommands test then failed its setup + /// checkout with "pathspec ... did not match any file(s) known to git". + /// + /// This method serializes setup across processes with a system-wide mutex, builds the cache + /// atomically (clone into a temporary directory, then move it into place), verifies the + /// required base branch is present, and rebuilds the cache when verification fails. + /// + private static void EnsureSharedCache() + { + using (Mutex mutex = new Mutex(initiallyOwned: false, name: CacheMutexName)) + { + bool mutexHeld = false; + try + { + try + { + mutexHeld = mutex.WaitOne(TimeSpan.FromMinutes(10)); + } + catch (AbandonedMutexException) + { + // A previous process exited while holding the mutex. The cache is verified + // below regardless, so it is safe to proceed. + mutexHeld = true; + } + + if (!mutexHeld) + { + throw new TimeoutException($"Timed out waiting to initialize the control-repo cache at '{CachePath}'."); + } + + string baseBranch = Properties.Settings.Default.Commitish; + + if (CacheHasBranch(CachePath, baseBranch)) + { + // Refresh the existing cache so newly-added test branches are available. + // Only rebuild if the refresh leaves the cache invalid. + ProcessResult refresh = InvokeGitWithRetry(CachePath, "fetch origin +refs/*:refs/*"); + if (refresh.ExitCode != 0 || !CacheHasBranch(CachePath, baseBranch)) + { + RebuildCache(baseBranch); + } + } + else + { + RebuildCache(baseBranch); + } + } + finally + { + if (mutexHeld) + { + mutex.ReleaseMutex(); + } + } + } + } + + private static void RebuildCache(string baseBranch) + { + string root = Properties.Settings.Default.ControlGitRepoRoot; + Directory.CreateDirectory(root); + + string tempCache = Path.Combine(root, "cache.tmp." + Guid.NewGuid().ToString("N")); + + ProcessResult clone = null; + for (int attempt = 1; attempt <= 3; attempt++) + { + if (Directory.Exists(tempCache)) + { + RepositoryHelpers.DeleteTestDirectory(tempCache); + } + + clone = GitProcess.InvokeProcess( + Environment.SystemDirectory, + "clone " + GVFSTestConfig.RepoToClone + " " + tempCache + " --bare"); + + if (clone.ExitCode == 0 && CacheHasBranch(tempCache, baseBranch)) + { + break; + } + + if (attempt == 3) + { + throw new InvalidOperationException( + $"Failed to build the control-repo cache from '{GVFSTestConfig.RepoToClone}' after {attempt} attempts. " + + $"git exit code {clone.ExitCode}: {clone.Errors}"); + } + + Thread.Sleep(TimeSpan.FromSeconds(attempt * 2)); + } + + // Move the fully-built cache into place so no other process observes a partial directory. + if (Directory.Exists(CachePath)) + { + RepositoryHelpers.DeleteTestDirectory(CachePath); + } + + Directory.Move(tempCache, CachePath); + } + + private static bool CacheHasBranch(string cachePath, string branch) + { + if (!Directory.Exists(cachePath)) + { + return false; + } + + ProcessResult result = GitProcess.InvokeProcess(cachePath, "rev-parse --verify --quiet refs/heads/" + branch); + return result.ExitCode == 0 && !string.IsNullOrWhiteSpace(result.Output); + } + + private static ProcessResult InvokeGitWithRetry(string workingDirectory, string command, int attempts = 3) + { + ProcessResult result = null; + for (int attempt = 1; attempt <= attempts; attempt++) + { + result = GitProcess.InvokeProcess(workingDirectory, command); + if (result.ExitCode == 0) + { + return result; + } + + if (attempt < attempts) + { + Thread.Sleep(TimeSpan.FromSeconds(attempt)); + } + } + + return result; } } } From 183f5eb943b7f3fba7eb05c1f76278cc9cacfb8c Mon Sep 17 00:00:00 2001 From: Tyrie Vella Date: Thu, 27 Aug 2026 13:18:19 -0700 Subject: [PATCH 05/11] FunctionalTests: build control-repo cache with --mirror, not --bare The previous commit rebuilt the shared control-repo cache with "git clone --bare". A --bare clone copies only refs/heads/* and tags. Some tests fetch commits by SHA that live outside refs/heads (for example RebaseTests fetches the tip of FunctionalTests/RebaseTestsSource_20170130). Those objects were absent from a --bare cache, so the control repo's fetch failed with: fatal: git upload-pack: not our ref and, now that Fetch throws on a non-zero exit, RebaseSmallOneFileConflict failed on functional-test slice 4 (both architectures). Rebuild the cache with "git clone --mirror" instead. --mirror maps refs/*:refs/*, so the cache carries the complete ref set, matching the refresh path's "fetch origin +refs/*:refs/*". A local test confirms a --bare clone omits a non-refs/heads ref while a --mirror clone retains it. Assisted-by: Claude Opus 4.8 Signed-off-by: Tyrie Vella --- GVFS/GVFS.FunctionalTests/Tools/ControlGitRepo.cs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/GVFS/GVFS.FunctionalTests/Tools/ControlGitRepo.cs b/GVFS/GVFS.FunctionalTests/Tools/ControlGitRepo.cs index 89272301c8..cb97caaea4 100644 --- a/GVFS/GVFS.FunctionalTests/Tools/ControlGitRepo.cs +++ b/GVFS/GVFS.FunctionalTests/Tools/ControlGitRepo.cs @@ -192,9 +192,14 @@ private static void RebuildCache(string baseBranch) RepositoryHelpers.DeleteTestDirectory(tempCache); } + // Use --mirror (not --bare) so the cache carries the complete ref set. A --bare + // clone copies only refs/heads/* and tags; some tests fetch commits (by SHA) that + // live outside refs/heads, so a --bare cache would be missing those objects and the + // fetch would fail with "not our ref". --mirror maps refs/*:refs/*, matching the + // refresh path's "fetch origin +refs/*:refs/*". clone = GitProcess.InvokeProcess( Environment.SystemDirectory, - "clone " + GVFSTestConfig.RepoToClone + " " + tempCache + " --bare"); + "clone " + GVFSTestConfig.RepoToClone + " " + tempCache + " --mirror"); if (clone.ExitCode == 0 && CacheHasBranch(tempCache, baseBranch)) { From 6d4d4d7331856c4042f2c5f8ad6ae20c5b1810fb Mon Sep 17 00:00:00 2001 From: Tyrie Vella Date: Thu, 27 Aug 2026 14:12:16 -0700 Subject: [PATCH 06/11] FunctionalTests: serve control-repo cache SHAs, stop rebuilding cache The prior commits regressed functional-test slice 4: RebaseTests fetch a specific commit by SHA (for example 5d29951...), and those fetches failed with "fatal: git upload-pack: not our ref". Two problems caused this. 1. The shared cache is machine-global and persistent on CI runners. It can hold commits reachable only from branches that upstream no longer advertises, which some tests still fetch by SHA. My EnsureSharedCache could rebuild (replace) that cache when a branch check or refresh looked wrong, which drops those commits. Decide fresh-build vs refresh by Directory.Exists (matching the original code), refresh an existing cache best-effort, and never delete or rebuild it. 2. upload-pack refuses to serve a SHA that is not an advertised ref tip unless the served repo allows it. Enable uploadpack.allowAnySHA1InWant (plus reachable and tip) on the cache so control repos can fetch any commit present in the cache by SHA. Also make ControlGitRepo.Fetch tolerant again: it retries, but on final failure it logs instead of throwing. Whether the cache can serve a given SHA is a property of the cache, not the test; the test's own ValidateGitCommand (control repo vs GVFS repo) remains the correctness gate. The loud failure for a genuinely broken cache stays on the base-branch checkout in Initialize, which is what caused the original swallowed cascade. Fresh caches are still built with clone --mirror (complete ref set) into a temporary directory and moved into place under a system-wide mutex, so no process observes a half-built cache. Assisted-by: Claude Opus 4.8 Signed-off-by: Tyrie Vella --- .../Tools/ControlGitRepo.cs | 49 +++++++++++++------ 1 file changed, 33 insertions(+), 16 deletions(-) diff --git a/GVFS/GVFS.FunctionalTests/Tools/ControlGitRepo.cs b/GVFS/GVFS.FunctionalTests/Tools/ControlGitRepo.cs index cb97caaea4..bd884d9797 100644 --- a/GVFS/GVFS.FunctionalTests/Tools/ControlGitRepo.cs +++ b/GVFS/GVFS.FunctionalTests/Tools/ControlGitRepo.cs @@ -105,9 +105,12 @@ public void Fetch(string commitish) ProcessResult result = InvokeGitWithRetry(this.RootPath, "fetch origin " + commitish); if (result.ExitCode != 0) { - throw new InvalidOperationException( - $"Control repo failed to fetch '{commitish}' from the shared cache '{CachePath}'. " + - $"git exit code {result.ExitCode}: {result.Errors}"); + // Do not throw here. Some tests fetch a specific commit by SHA; whether the shared + // cache can serve that SHA is a property of the cache, not of the test. The test's + // own ValidateGitCommand (which compares the control repo against the GVFS repo) + // is the correctness gate. Log for diagnosis and continue. + Console.WriteLine( + $"ControlGitRepo.Fetch: 'fetch origin {commitish}' returned {result.ExitCode} from cache '{CachePath}': {result.Errors}"); } } @@ -123,9 +126,13 @@ public void Fetch(string commitish) /// directory, left the cache missing branches. Every GitCommands test then failed its setup /// checkout with "pathspec ... did not match any file(s) known to git". /// - /// This method serializes setup across processes with a system-wide mutex, builds the cache - /// atomically (clone into a temporary directory, then move it into place), verifies the - /// required base branch is present, and rebuilds the cache when verification fails. + /// This method serializes setup across processes with a system-wide mutex. It builds a + /// missing cache atomically (clone into a temporary directory, verify the base branch, then + /// move it into place). It never rebuilds or deletes an existing cache: on CI runners the + /// cache is persistent and can hold commits from branches that no longer exist upstream + /// (some tests fetch those commits by SHA), so replacing it with a fresh clone would drop + /// those objects. An existing cache is only refreshed, best-effort. Finally it enables + /// uploadpack.allowAnySHA1InWant so control repos can fetch any commit in the cache by SHA. /// private static void EnsureSharedCache() { @@ -140,7 +147,7 @@ private static void EnsureSharedCache() } catch (AbandonedMutexException) { - // A previous process exited while holding the mutex. The cache is verified + // A previous process exited while holding the mutex. The cache is handled // below regardless, so it is safe to proceed. mutexHeld = true; } @@ -152,20 +159,23 @@ private static void EnsureSharedCache() string baseBranch = Properties.Settings.Default.Commitish; - if (CacheHasBranch(CachePath, baseBranch)) + if (Directory.Exists(CachePath)) { // Refresh the existing cache so newly-added test branches are available. - // Only rebuild if the refresh leaves the cache invalid. - ProcessResult refresh = InvokeGitWithRetry(CachePath, "fetch origin +refs/*:refs/*"); - if (refresh.ExitCode != 0 || !CacheHasBranch(CachePath, baseBranch)) - { - RebuildCache(baseBranch); - } + // Do this best-effort and never delete/rebuild: the persistent cache can + // hold commits that upstream no longer advertises (fetched by SHA by some + // tests), which a fresh clone would not restore. + InvokeGitWithRetry(CachePath, "fetch origin +refs/*:refs/*"); } else { - RebuildCache(baseBranch); + BuildFreshCache(baseBranch); } + + // Allow control repos to fetch any commit present in the cache by its SHA + // (some tests fetch specific commits directly). Without this, upload-pack + // rejects a SHA that is not an advertised ref tip with "not our ref". + ConfigureCacheForShaFetch(CachePath); } finally { @@ -177,7 +187,7 @@ private static void EnsureSharedCache() } } - private static void RebuildCache(string baseBranch) + private static void BuildFreshCache(string baseBranch) { string root = Properties.Settings.Default.ControlGitRepoRoot; Directory.CreateDirectory(root); @@ -225,6 +235,13 @@ private static void RebuildCache(string baseBranch) Directory.Move(tempCache, CachePath); } + private static void ConfigureCacheForShaFetch(string cachePath) + { + GitProcess.InvokeProcess(cachePath, "config uploadpack.allowAnySHA1InWant true"); + GitProcess.InvokeProcess(cachePath, "config uploadpack.allowReachableSHA1InWant true"); + GitProcess.InvokeProcess(cachePath, "config uploadpack.allowTipSHA1InWant true"); + } + private static bool CacheHasBranch(string cachePath, string branch) { if (!Directory.Exists(cachePath)) From d2215eb130844be04027a8d50d82fc9c2bcb70b2 Mon Sep 17 00:00:00 2001 From: Derrick Stolee Date: Wed, 2 Sep 2026 11:11:13 -0400 Subject: [PATCH 07/11] feat: Route GVFS endpoints to dedicated cache servers Context: The microsoft/git GVFS helper supports endpoint-specific cache servers so cache infrastructure can be migrated independently. VFS for Git previously sent every protocol request to one global URL. Justification: Use the same gvfs..cache-server keys and clone option names as Scalar. Keeping endpoint preferences on CacheServerInfo centralizes precedence and lets mount-time cache resolution preserve the configured routes. Implementation: Load, persist, and validate overrides for prefetch, object GET, object POST, and sizes requests. Add matching clone options, retain the global cache as the default, preserve overrides while resolving cache identity, and cover configuration, CLI parsing, and mount resolution. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- GVFS/FastFetch/FastFetchVerb.cs | 4 +- .../GvfsMainCliTests.cs | 18 +++- GVFS/GVFS.Common/GVFSConstants.cs | 4 + GVFS/GVFS.Common/Http/CacheServerInfo.cs | 71 +++++++++++++- GVFS/GVFS.Common/Http/CacheServerResolver.cs | 37 +++++++- .../Http/GitObjectsHttpRequestor.cs | 6 +- GVFS/GVFS.Mount/InProcessMount.cs | 5 +- .../Common/CacheServerResolverTests.cs | 94 ++++++++++++++++++- GVFS/GVFS/CommandLine/CloneVerb.cs | 38 ++++++++ GVFS/GVFS/CommandLine/GVFSVerb.cs | 3 +- GVFS/GVFS/CommandLine/PrefetchVerb.cs | 4 +- 11 files changed, 272 insertions(+), 12 deletions(-) diff --git a/GVFS/FastFetch/FastFetchVerb.cs b/GVFS/FastFetch/FastFetchVerb.cs index 737b31ffe3..cf6add3aad 100644 --- a/GVFS/FastFetch/FastFetchVerb.cs +++ b/GVFS/FastFetch/FastFetchVerb.cs @@ -237,7 +237,9 @@ private int ExecuteWithExitCode() string fastfetchLogFile = Enlistment.GetNewLogFileName(enlistment.FastFetchLogRoot, "fastfetch"); tracer.AddLogFileEventListener(fastfetchLogFile, EventLevel.Informational, Keywords.Any); - CacheServerInfo cacheServer = new CacheServerInfo(this.GetRemoteUrl(enlistment), null); + CacheServerInfo cacheServer = string.IsNullOrWhiteSpace(this.CacheServerUrl) + ? CacheServerResolver.GetCacheServerFromConfig(enlistment) + : new CacheServerInfo(this.GetRemoteUrl(enlistment), null); tracer.WriteStartEvent( enlistment.PrimaryEnlistmentRoot, diff --git a/GVFS/GVFS.CommandLine.Tests/GvfsMainCliTests.cs b/GVFS/GVFS.CommandLine.Tests/GvfsMainCliTests.cs index 4eb360808e..27e117d216 100644 --- a/GVFS/GVFS.CommandLine.Tests/GvfsMainCliTests.cs +++ b/GVFS/GVFS.CommandLine.Tests/GvfsMainCliTests.cs @@ -215,6 +215,10 @@ public void Clone_FullCommandLine_ParsesCorrectly() { "clone", "https://example.com/repo", @"C:\Users\test\repo", "--cache-server-url", "https://cache.test", + "--prefetch-cache-server-url", "https://prefetch-cache.test", + "--get-cache-server-url", "https://get-cache.test", + "--post-cache-server-url", "https://post-cache.test", + "--sizes-cache-server-url", "https://sizes-cache.test", "-b", "develop", "--single-branch", "--no-mount", @@ -342,7 +346,19 @@ public void Repair_FullCommandLine_ParsesCorrectly() [Test] public void Clone_HasAllExpectedOptions() { - var expected = new[] { "--cache-server-url", "--branch", "--single-branch", "--no-mount", "--no-prefetch", "--local-cache-path" }; + var expected = new[] + { + "--cache-server-url", + "--prefetch-cache-server-url", + "--get-cache-server-url", + "--post-cache-server-url", + "--sizes-cache-server-url", + "--branch", + "--single-branch", + "--no-mount", + "--no-prefetch", + "--local-cache-path", + }; foreach (var optName in expected) { Assert.That(FindOptionOnCommand("clone", optName), Is.Not.Null, diff --git a/GVFS/GVFS.Common/GVFSConstants.cs b/GVFS/GVFS.Common/GVFSConstants.cs index 143b59e693..088d771957 100644 --- a/GVFS/GVFS.Common/GVFSConstants.cs +++ b/GVFS/GVFS.Common/GVFSConstants.cs @@ -33,6 +33,10 @@ public static class GitConfig public const string MountId = GVFSPrefix + "mount-id"; public const string EnlistmentId = GVFSPrefix + "enlistment-id"; public const string CacheServer = GVFSPrefix + "cache-server"; + public const string PrefetchCacheServer = GVFSPrefix + "prefetch.cache-server"; + public const string GetCacheServer = GVFSPrefix + "get.cache-server"; + public const string PostCacheServer = GVFSPrefix + "post.cache-server"; + public const string SizesCacheServer = GVFSPrefix + "sizes.cache-server"; public const string DeprecatedCacheEndpointSuffix = ".cache-server-url"; public const string HooksPrefix = GitConfig.GVFSPrefix + "clone.default-"; public const string GVFSTelemetryId = GitConfig.GVFSPrefix + "telemetry-id"; diff --git a/GVFS/GVFS.Common/Http/CacheServerInfo.cs b/GVFS/GVFS.Common/Http/CacheServerInfo.cs index 0ec929b0dc..33e1b61e70 100644 --- a/GVFS/GVFS.Common/Http/CacheServerInfo.cs +++ b/GVFS/GVFS.Common/Http/CacheServerInfo.cs @@ -11,27 +11,89 @@ public class CacheServerInfo [JsonConstructor] public CacheServerInfo(string url, string name, bool globalDefault = false) + : this(url, name, globalDefault, null, null, null, null) + { + } + + public CacheServerInfo( + string url, + string name, + bool globalDefault, + string prefetchCacheServerUrl, + string getCacheServerUrl, + string postCacheServerUrl, + string sizesCacheServerUrl) { this.Url = url; this.Name = name; this.GlobalDefault = globalDefault; + this.PrefetchCacheServerUrl = prefetchCacheServerUrl; + this.GetCacheServerUrl = getCacheServerUrl; + this.PostCacheServerUrl = postCacheServerUrl; + this.SizesCacheServerUrl = sizesCacheServerUrl; if (this.Url != null) { this.ObjectsEndpointUrl = this.Url + ObjectsEndpointSuffix; - this.PrefetchEndpointUrl = this.Url + PrefetchEndpointSuffix; - this.SizesEndpointUrl = this.Url + SizesEndpointSuffix; } + + this.PrefetchEndpointUrl = GetEndpointUrl(prefetchCacheServerUrl ?? this.Url, PrefetchEndpointSuffix); + this.ObjectsGetEndpointUrl = GetEndpointUrl(getCacheServerUrl ?? this.Url, ObjectsEndpointSuffix); + this.ObjectsPostEndpointUrl = GetEndpointUrl(postCacheServerUrl ?? this.Url, ObjectsEndpointSuffix); + this.SizesEndpointUrl = GetEndpointUrl(sizesCacheServerUrl ?? this.Url, SizesEndpointSuffix); } public string Url { get; } public string Name { get; } public bool GlobalDefault { get; } + [JsonIgnore] + public string PrefetchCacheServerUrl { get; } + + [JsonIgnore] + public string GetCacheServerUrl { get; } + + [JsonIgnore] + public string PostCacheServerUrl { get; } + + [JsonIgnore] + public string SizesCacheServerUrl { get; } + public string ObjectsEndpointUrl { get; } public string PrefetchEndpointUrl { get; } public string SizesEndpointUrl { get; } + [JsonIgnore] + public string ObjectsGetEndpointUrl { get; } + + [JsonIgnore] + public string ObjectsPostEndpointUrl { get; } + + public CacheServerInfo WithEndpointOverrides( + string prefetchCacheServerUrl, + string getCacheServerUrl, + string postCacheServerUrl, + string sizesCacheServerUrl) + { + return new CacheServerInfo( + this.Url, + this.Name, + this.GlobalDefault, + prefetchCacheServerUrl, + getCacheServerUrl, + postCacheServerUrl, + sizesCacheServerUrl); + } + + public CacheServerInfo WithEndpointOverridesFrom(CacheServerInfo cacheServer) + { + return this.WithEndpointOverrides( + cacheServer.PrefetchCacheServerUrl, + cacheServer.GetCacheServerUrl, + cacheServer.PostCacheServerUrl, + cacheServer.SizesCacheServerUrl); + } + public bool HasValidUrl() { return Uri.IsWellFormedUriString(this.Url, UriKind.Absolute); @@ -64,5 +126,10 @@ public static class ReservedNames public const string Default = "Default"; public const string UserDefined = "User Defined"; } + + private static string GetEndpointUrl(string cacheServerUrl, string endpointSuffix) + { + return cacheServerUrl == null ? null : cacheServerUrl + endpointSuffix; + } } } diff --git a/GVFS/GVFS.Common/Http/CacheServerResolver.cs b/GVFS/GVFS.Common/Http/CacheServerResolver.cs index bc1df9727b..26037c0a75 100644 --- a/GVFS/GVFS.Common/Http/CacheServerResolver.cs +++ b/GVFS/GVFS.Common/Http/CacheServerResolver.cs @@ -20,10 +20,16 @@ public CacheServerResolver( public static CacheServerInfo GetCacheServerFromConfig(Enlistment enlistment) { + GitProcess git = enlistment.CreateGitProcess(); string url = GetUrlFromConfig(enlistment); return new CacheServerInfo( url, - url == enlistment.RepoUrl ? CacheServerInfo.ReservedNames.None : null); + url == enlistment.RepoUrl ? CacheServerInfo.ReservedNames.None : null, + globalDefault: false, + GetValueFromConfig(git, GVFSConstants.GitConfig.PrefetchCacheServer, localOnly: true), + GetValueFromConfig(git, GVFSConstants.GitConfig.GetCacheServer, localOnly: true), + GetValueFromConfig(git, GVFSConstants.GitConfig.PostCacheServer, localOnly: true), + GetValueFromConfig(git, GVFSConstants.GitConfig.SizesCacheServer, localOnly: true)); } public static string GetUrlFromConfig(Enlistment enlistment) @@ -129,6 +135,22 @@ public bool TrySaveUrlToLocalConfig(CacheServerInfo cache, out string error) return result.ExitCodeIsSuccess; } + public bool TrySaveEndpointUrlsToLocalConfig(CacheServerInfo cache, out string error) + { + GitProcess git = this.enlistment.CreateGitProcess(); + + if (!TrySaveEndpointUrl(git, GVFSConstants.GitConfig.PrefetchCacheServer, cache.PrefetchCacheServerUrl, out error) || + !TrySaveEndpointUrl(git, GVFSConstants.GitConfig.GetCacheServer, cache.GetCacheServerUrl, out error) || + !TrySaveEndpointUrl(git, GVFSConstants.GitConfig.PostCacheServer, cache.PostCacheServerUrl, out error) || + !TrySaveEndpointUrl(git, GVFSConstants.GitConfig.SizesCacheServer, cache.SizesCacheServerUrl, out error)) + { + return false; + } + + error = null; + return true; + } + private static string GetValueFromConfig(GitProcess git, string configName, bool localOnly) { GitProcess.ConfigResult result = @@ -144,6 +166,19 @@ private static string GetValueFromConfig(GitProcess git, string configName, bool return value; } + private static bool TrySaveEndpointUrl(GitProcess git, string configName, string url, out string error) + { + error = null; + if (url == null) + { + return true; + } + + GitProcess.Result result = git.SetInLocalConfig(configName, url, replaceAll: true); + error = result.Errors; + return result.ExitCodeIsSuccess; + } + private static string GetDeprecatedCacheConfigSettingName(Enlistment enlistment) { string sectionUrl = diff --git a/GVFS/GVFS.Common/Http/GitObjectsHttpRequestor.cs b/GVFS/GVFS.Common/Http/GitObjectsHttpRequestor.cs index 2cdffcb8da..45e4ba8c4f 100644 --- a/GVFS/GVFS.Common/Http/GitObjectsHttpRequestor.cs +++ b/GVFS/GVFS.Common/Http/GitObjectsHttpRequestor.cs @@ -149,7 +149,7 @@ public virtual RetryWrapper.InvocationResult TryDownloadLoo onSuccess, eArgs => this.HandleDownloadAndSaveObjectError(retryOnFailure, requestId, eArgs), HttpMethod.Get, - new Uri(this.CacheServer.ObjectsEndpointUrl + "/" + objectId), + new Uri(this.CacheServer.ObjectsGetEndpointUrl + "/" + objectId), cancellationToken, requestBody: null, acceptType: null, @@ -170,7 +170,7 @@ public virtual RetryWrapper.InvocationResult TryDownloadObj onSuccess, onFailure, HttpMethod.Post, - new Uri(this.CacheServer.ObjectsEndpointUrl), + new Uri(this.CacheServer.ObjectsPostEndpointUrl), CancellationToken.None, () => this.ObjectIdsJsonGenerator(requestId, objectIdGenerator), preferBatchedLooseObjects ? CustomLooseObjectsHeader : null); @@ -204,7 +204,7 @@ public virtual RetryWrapper.InvocationResult TryDownloadObj onSuccess, onFailure, HttpMethod.Post, - new Uri(this.CacheServer.ObjectsEndpointUrl), + new Uri(this.CacheServer.ObjectsPostEndpointUrl), CancellationToken.None, objectIdsJson, preferBatchedLooseObjects ? CustomLooseObjectsHeader : null); diff --git a/GVFS/GVFS.Mount/InProcessMount.cs b/GVFS/GVFS.Mount/InProcessMount.cs index 629be66138..0a5c930b5f 100644 --- a/GVFS/GVFS.Mount/InProcessMount.cs +++ b/GVFS/GVFS.Mount/InProcessMount.cs @@ -353,7 +353,10 @@ private void MountWithLockAcquired(EventLevel verbosity, Keywords keywords) this.mountProgressMessage = "Resolving cache server"; CacheServerResolver cacheServerResolver = new CacheServerResolver(this.tracer, this.enlistment); - this.cacheServer = cacheServerResolver.ResolveNameFromRemote(this.cacheServer.Url, serverGVFSConfig); + CacheServerInfo cacheServerFromConfig = this.cacheServer; + this.cacheServer = cacheServerResolver + .ResolveNameFromRemote(cacheServerFromConfig.Url, serverGVFSConfig) + .WithEndpointOverridesFrom(cacheServerFromConfig); this.tracer.RelatedEvent( EventLevel.Informational, diff --git a/GVFS/GVFS.UnitTests/Common/CacheServerResolverTests.cs b/GVFS/GVFS.UnitTests/Common/CacheServerResolverTests.cs index 852ecb908a..651e05343b 100644 --- a/GVFS/GVFS.UnitTests/Common/CacheServerResolverTests.cs +++ b/GVFS/GVFS.UnitTests/Common/CacheServerResolverTests.cs @@ -13,6 +13,10 @@ public class CacheServerResolverTests { private const string CacheServerUrl = "https://cache/server"; private const string CacheServerName = "TestCacheServer"; + private const string PrefetchCacheServerUrl = "https://prefetch-cache/server"; + private const string GetCacheServerUrl = "https://get-cache/server"; + private const string PostCacheServerUrl = "https://post-cache/server"; + private const string SizesCacheServerUrl = "https://sizes-cache/server"; [TestCase] public void CanGetCacheServerFromNewConfig() @@ -43,6 +47,76 @@ public void CanGetCacheServerWithNoConfig() CacheServerResolver.GetUrlFromConfig(enlistment).ShouldEqual(enlistment.RepoUrl); } + [TestCase] + public void EndpointSpecificCacheServersOverrideGlobalCacheServer() + { + MockGVFSEnlistment enlistment = this.CreateEnlistment( + CacheServerUrl, + prefetchCacheServerUrl: PrefetchCacheServerUrl, + getCacheServerUrl: GetCacheServerUrl, + postCacheServerUrl: PostCacheServerUrl, + sizesCacheServerUrl: SizesCacheServerUrl); + + CacheServerInfo cacheServer = CacheServerResolver.GetCacheServerFromConfig(enlistment); + + cacheServer.PrefetchEndpointUrl.ShouldEqual(PrefetchCacheServerUrl + "/gvfs/prefetch"); + cacheServer.ObjectsGetEndpointUrl.ShouldEqual(GetCacheServerUrl + "/gvfs/objects"); + cacheServer.ObjectsPostEndpointUrl.ShouldEqual(PostCacheServerUrl + "/gvfs/objects"); + cacheServer.SizesEndpointUrl.ShouldEqual(SizesCacheServerUrl + "/gvfs/sizes"); + } + + [TestCase] + public void EndpointSpecificCacheServersFallBackToGlobalCacheServer() + { + CacheServerInfo cacheServer = CacheServerResolver.GetCacheServerFromConfig(this.CreateEnlistment(CacheServerUrl)); + + cacheServer.PrefetchEndpointUrl.ShouldEqual(CacheServerUrl + "/gvfs/prefetch"); + cacheServer.ObjectsGetEndpointUrl.ShouldEqual(CacheServerUrl + "/gvfs/objects"); + cacheServer.ObjectsPostEndpointUrl.ShouldEqual(CacheServerUrl + "/gvfs/objects"); + cacheServer.SizesEndpointUrl.ShouldEqual(CacheServerUrl + "/gvfs/sizes"); + } + + [TestCase] + public void EndpointSpecificCacheServersArePreservedWhenGlobalCacheServerIsResolved() + { + CacheServerInfo configuredCacheServer = new CacheServerInfo(CacheServerUrl, CacheServerName) + .WithEndpointOverrides(PrefetchCacheServerUrl, GetCacheServerUrl, PostCacheServerUrl, SizesCacheServerUrl); + CacheServerInfo resolvedCacheServer = new CacheServerInfo("https://resolved-cache/server", "ResolvedCache") + .WithEndpointOverridesFrom(configuredCacheServer); + + resolvedCacheServer.PrefetchCacheServerUrl.ShouldEqual(PrefetchCacheServerUrl); + resolvedCacheServer.GetCacheServerUrl.ShouldEqual(GetCacheServerUrl); + resolvedCacheServer.PostCacheServerUrl.ShouldEqual(PostCacheServerUrl); + resolvedCacheServer.SizesCacheServerUrl.ShouldEqual(SizesCacheServerUrl); + resolvedCacheServer.HasValidUrl().ShouldEqual(true); + } + + [TestCase] + public void CanSaveEndpointSpecificCacheServers() + { + MockGVFSEnlistment enlistment = this.CreateEnlistment(); + MockGitProcess git = (MockGitProcess)enlistment.CreateGitProcess(); + git.SetExpectedCommandResult( + "config --local --replace-all \"gvfs.prefetch.cache-server\" \"https://prefetch-cache/server\"", + () => new GitProcess.Result(string.Empty, string.Empty, GitProcess.Result.SuccessCode)); + git.SetExpectedCommandResult( + "config --local --replace-all \"gvfs.get.cache-server\" \"https://get-cache/server\"", + () => new GitProcess.Result(string.Empty, string.Empty, GitProcess.Result.SuccessCode)); + git.SetExpectedCommandResult( + "config --local --replace-all \"gvfs.post.cache-server\" \"https://post-cache/server\"", + () => new GitProcess.Result(string.Empty, string.Empty, GitProcess.Result.SuccessCode)); + git.SetExpectedCommandResult( + "config --local --replace-all \"gvfs.sizes.cache-server\" \"https://sizes-cache/server\"", + () => new GitProcess.Result(string.Empty, string.Empty, GitProcess.Result.SuccessCode)); + + CacheServerInfo cacheServer = new CacheServerInfo(CacheServerUrl, CacheServerName) + .WithEndpointOverrides(PrefetchCacheServerUrl, GetCacheServerUrl, PostCacheServerUrl, SizesCacheServerUrl); + + new CacheServerResolver(new MockTracer(), enlistment) + .TrySaveEndpointUrlsToLocalConfig(cacheServer, out string error) + .ShouldEqual(true, error); + } + [TestCase] public void CanResolveUrlForKnownName() { @@ -190,7 +264,13 @@ private void ValidateIsNone(Enlistment enlistment, CacheServerInfo cacheServer) cacheServer.Name.ShouldEqual(CacheServerInfo.ReservedNames.None); } - private MockGVFSEnlistment CreateEnlistment(string newConfigValue = null, string oldConfigValue = null) + private MockGVFSEnlistment CreateEnlistment( + string newConfigValue = null, + string oldConfigValue = null, + string prefetchCacheServerUrl = null, + string getCacheServerUrl = null, + string postCacheServerUrl = null, + string sizesCacheServerUrl = null) { MockGitProcess gitProcess = new MockGitProcess(); gitProcess.SetExpectedCommandResult( @@ -199,6 +279,18 @@ private MockGVFSEnlistment CreateEnlistment(string newConfigValue = null, string gitProcess.SetExpectedCommandResult( "config gvfs.mock:..repourl.cache-server-url", () => new GitProcess.Result(oldConfigValue ?? string.Empty, string.Empty, oldConfigValue != null ? GitProcess.Result.SuccessCode : GitProcess.Result.GenericFailureCode)); + gitProcess.SetExpectedCommandResult( + "config --local gvfs.prefetch.cache-server", + () => new GitProcess.Result(prefetchCacheServerUrl ?? string.Empty, string.Empty, prefetchCacheServerUrl != null ? GitProcess.Result.SuccessCode : GitProcess.Result.GenericFailureCode)); + gitProcess.SetExpectedCommandResult( + "config --local gvfs.get.cache-server", + () => new GitProcess.Result(getCacheServerUrl ?? string.Empty, string.Empty, getCacheServerUrl != null ? GitProcess.Result.SuccessCode : GitProcess.Result.GenericFailureCode)); + gitProcess.SetExpectedCommandResult( + "config --local gvfs.post.cache-server", + () => new GitProcess.Result(postCacheServerUrl ?? string.Empty, string.Empty, postCacheServerUrl != null ? GitProcess.Result.SuccessCode : GitProcess.Result.GenericFailureCode)); + gitProcess.SetExpectedCommandResult( + "config --local gvfs.sizes.cache-server", + () => new GitProcess.Result(sizesCacheServerUrl ?? string.Empty, string.Empty, sizesCacheServerUrl != null ? GitProcess.Result.SuccessCode : GitProcess.Result.GenericFailureCode)); return new MockGVFSEnlistment(gitProcess); } diff --git a/GVFS/GVFS/CommandLine/CloneVerb.cs b/GVFS/GVFS/CommandLine/CloneVerb.cs index 1277355ab6..370f40b30a 100644 --- a/GVFS/GVFS/CommandLine/CloneVerb.cs +++ b/GVFS/GVFS/CommandLine/CloneVerb.cs @@ -24,6 +24,14 @@ public class CloneVerb : GVFSVerb public string CacheServerUrl { get; set; } + public string PrefetchCacheServerUrl { get; set; } + + public string GetCacheServerUrl { get; set; } + + public string PostCacheServerUrl { get; set; } + + public string SizesCacheServerUrl { get; set; } + public string Branch { get; set; } public bool SingleBranch { get; set; } @@ -56,6 +64,18 @@ public static System.CommandLine.Command CreateCommand() System.CommandLine.Option cacheServerOption = new System.CommandLine.Option("--cache-server-url") { Description = "The url or friendly name of the cache server" }; cmd.Add(cacheServerOption); + System.CommandLine.Option prefetchCacheServerOption = new System.CommandLine.Option("--prefetch-cache-server-url") { Description = "The cache server URL for the prefetch endpoint" }; + cmd.Add(prefetchCacheServerOption); + + System.CommandLine.Option getCacheServerOption = new System.CommandLine.Option("--get-cache-server-url") { Description = "The cache server URL for the objects GET endpoint" }; + cmd.Add(getCacheServerOption); + + System.CommandLine.Option postCacheServerOption = new System.CommandLine.Option("--post-cache-server-url") { Description = "The cache server URL for the objects POST endpoint" }; + cmd.Add(postCacheServerOption); + + System.CommandLine.Option sizesCacheServerOption = new System.CommandLine.Option("--sizes-cache-server-url") { Description = "The cache server URL for the sizes endpoint" }; + cmd.Add(sizesCacheServerOption); + System.CommandLine.Option branchOption = new System.CommandLine.Option("--branch", new[] { "-b" }) { Description = "Branch to checkout after clone" }; cmd.Add(branchOption); @@ -86,6 +106,10 @@ public static System.CommandLine.Command CreateCommand() } verb.CacheServerUrl = result.GetValue(cacheServerOption); + verb.PrefetchCacheServerUrl = result.GetValue(prefetchCacheServerOption); + verb.GetCacheServerUrl = result.GetValue(getCacheServerOption); + verb.PostCacheServerUrl = result.GetValue(postCacheServerOption); + verb.SizesCacheServerUrl = result.GetValue(sizesCacheServerOption); verb.Branch = result.GetValue(branchOption); verb.SingleBranch = result.GetValue(singleBranchOption); verb.NoMount = result.GetValue(noMountOption); @@ -144,6 +168,10 @@ public override void Execute() this.CheckKernelDriverSupported(normalizedEnlistmentRootPath); this.CheckNotInsideExistingRepo(normalizedEnlistmentRootPath); this.BlockEmptyCacheServerUrl(this.CacheServerUrl); + this.BlockEmptyCacheServerUrl(this.PrefetchCacheServerUrl); + this.BlockEmptyCacheServerUrl(this.GetCacheServerUrl); + this.BlockEmptyCacheServerUrl(this.PostCacheServerUrl); + this.BlockEmptyCacheServerUrl(this.SizesCacheServerUrl); try { @@ -231,6 +259,11 @@ public override void Execute() } cacheServer = this.ResolveCacheServer(tracer, cacheServer, cacheServerResolver, serverGVFSConfig); + cacheServer = cacheServer.WithEndpointOverrides( + this.PrefetchCacheServerUrl, + this.GetCacheServerUrl, + this.PostCacheServerUrl, + this.SizesCacheServerUrl); this.ValidateClientVersions(tracer, enlistment, serverGVFSConfig, showWarnings: true); @@ -643,6 +676,11 @@ private Result CreateClone( return new Result("Unable to configure cache server: " + errorMessage); } + if (!cacheServerResolver.TrySaveEndpointUrlsToLocalConfig(objectRequestor.CacheServer, out errorMessage)) + { + return new Result("Unable to configure endpoint-specific cache servers: " + errorMessage); + } + GitProcess git = new GitProcess(enlistment); string originBranchName = "origin/" + branch; GitProcess.Result createBranchResult = git.CreateBranchWithUpstream(branch, originBranchName); diff --git a/GVFS/GVFS/CommandLine/GVFSVerb.cs b/GVFS/GVFS/CommandLine/GVFSVerb.cs index 51b693578d..84a4c678bf 100644 --- a/GVFS/GVFS/CommandLine/GVFSVerb.cs +++ b/GVFS/GVFS/CommandLine/GVFSVerb.cs @@ -475,6 +475,7 @@ protected CacheServerInfo ResolveCacheServer( resolvedCacheServer = cacheServerResolver.ResolveNameFromRemote(cacheServer.Url, serverGVFSConfig); } + resolvedCacheServer = resolvedCacheServer.WithEndpointOverridesFrom(cacheServer); this.Output.WriteLine("Using cache server: " + resolvedCacheServer); return resolvedCacheServer; } @@ -526,7 +527,7 @@ protected bool TryDownloadCommit( if (!gitObjects.TryDownloadCommit(commitId)) { - error = "Could not download commit " + commitId + " from: " + Uri.EscapeDataString(objectRequestor.CacheServer.ObjectsEndpointUrl); + error = "Could not download commit " + commitId + " from: " + Uri.EscapeDataString(objectRequestor.CacheServer.ObjectsPostEndpointUrl); return false; } diff --git a/GVFS/GVFS/CommandLine/PrefetchVerb.cs b/GVFS/GVFS/CommandLine/PrefetchVerb.cs index 6fa0d91f42..f82f4c0898 100644 --- a/GVFS/GVFS/CommandLine/PrefetchVerb.cs +++ b/GVFS/GVFS/CommandLine/PrefetchVerb.cs @@ -340,7 +340,9 @@ private void InitializeServerConnection( CacheServerResolver cacheServerResolver = new CacheServerResolver(tracer, enlistment); - resolvedCacheServer = cacheServerResolver.ResolveNameFromRemote(cacheServerFromConfig.Url, serverGVFSConfig); + resolvedCacheServer = cacheServerResolver + .ResolveNameFromRemote(cacheServerFromConfig.Url, serverGVFSConfig) + .WithEndpointOverridesFrom(cacheServerFromConfig); if (!this.SkipVersionCheck) { From d423ed1e208a38e1db7d39ed77f7ba29f72527ff Mon Sep 17 00:00:00 2001 From: Derrick Stolee Date: Wed, 2 Sep 2026 11:11:59 -0400 Subject: [PATCH 08/11] fix: Fall back safely from dedicated cache endpoints Context: Endpoint-specific cache servers are preferences above the global cache, but failures previously terminated requests instead of using the healthy fallback route. Early fallback handling also charged abandoned attempts to the process-wide circuit breaker, confused cancellation with transport failure, and exposed excess URI data in telemetry. Justification: Treat route failover separately from transient retry accounting. Cancellation remains control flow, local processing errors stay on the active route, and network-body failures alone can move a request to the global cache. Authority-only metadata preserves diagnostics without exposing credentials or request details. Implementation: Fall back prefetch, object GET, object POST, and sizes requests through the global cache, with sizes retaining its final origin fallback. Track response-stream failures, preserve circuit-breaker budget across route transitions, propagate cancellation unchanged, validate endpoint URLs, and emit redacted fallback telemetry. Add focused coverage for HTTP, transport, body-read, local-write, cancellation, telemetry, and terminal failures. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../GvfsMainCliTests.cs | 20 + GVFS/GVFS.Common/Git/GitObjects.cs | 12 +- GVFS/GVFS.Common/Http/CacheServerInfo.cs | 15 +- GVFS/GVFS.Common/Http/CacheServerResolver.cs | 23 +- .../Http/GitEndPointResponseData.cs | 91 +++- .../Http/GitObjectsHttpRequestor.cs | 353 +++++++++++-- GVFS/GVFS.Common/Http/HttpRequestor.cs | 9 +- GVFS/GVFS.Common/RetryWrapper.cs | 10 +- .../Common/CacheServerResolverTests.cs | 14 + .../Http/GitObjectsHttpRequestorTests.cs | 485 ++++++++++++++++++ .../GVFS.UnitTests/Http/HttpRequestorTests.cs | 11 +- GVFS/GVFS.UnitTests/Mock/Common/MockTracer.cs | 8 + GVFS/GVFS/CommandLine/CloneVerb.cs | 29 ++ 13 files changed, 1020 insertions(+), 60 deletions(-) create mode 100644 GVFS/GVFS.UnitTests/Http/GitObjectsHttpRequestorTests.cs diff --git a/GVFS/GVFS.CommandLine.Tests/GvfsMainCliTests.cs b/GVFS/GVFS.CommandLine.Tests/GvfsMainCliTests.cs index 27e117d216..5cb6be2407 100644 --- a/GVFS/GVFS.CommandLine.Tests/GvfsMainCliTests.cs +++ b/GVFS/GVFS.CommandLine.Tests/GvfsMainCliTests.cs @@ -225,6 +225,26 @@ public void Clone_FullCommandLine_ParsesCorrectly() "--no-prefetch" }); Assert.That(parseResult.Errors, Is.Empty, "Full clone command should parse without errors"); + Assert.Multiple(() => + { + Assert.That(parseResult.GetValue((Option)FindOptionOnCommand("clone", "--cache-server-url")), Is.EqualTo("https://cache.test")); + Assert.That(parseResult.GetValue((Option)FindOptionOnCommand("clone", "--prefetch-cache-server-url")), Is.EqualTo("https://prefetch-cache.test")); + Assert.That(parseResult.GetValue((Option)FindOptionOnCommand("clone", "--get-cache-server-url")), Is.EqualTo("https://get-cache.test")); + Assert.That(parseResult.GetValue((Option)FindOptionOnCommand("clone", "--post-cache-server-url")), Is.EqualTo("https://post-cache.test")); + Assert.That(parseResult.GetValue((Option)FindOptionOnCommand("clone", "--sizes-cache-server-url")), Is.EqualTo("https://sizes-cache.test")); + }); + } + + [TestCase("--prefetch-cache-server-url")] + [TestCase("--get-cache-server-url")] + [TestCase("--post-cache-server-url")] + [TestCase("--sizes-cache-server-url")] + public void Clone_EndpointCacheServerUrl_RejectsInvalidUrl(string optionName) + { + var parseResult = rootCommand.Parse(new[] { "clone", "https://example.com/repo", optionName, "not-a-url" }); + + Assert.That(parseResult.Errors, Has.Count.EqualTo(1)); + Assert.That(parseResult.Errors[0].Message, Does.Contain("requires an absolute URL")); } [Test] diff --git a/GVFS/GVFS.Common/Git/GitObjects.cs b/GVFS/GVFS.Common/Git/GitObjects.cs index a9b0f2851a..19c1a0e608 100644 --- a/GVFS/GVFS.Common/Git/GitObjects.cs +++ b/GVFS/GVFS.Common/Git/GitObjects.cs @@ -180,6 +180,11 @@ public virtual bool TryDownloadPrefetchPacks(GitProcess gitProcess, long latestT "{0}?lastPackTimestamp={1}", this.GitObjectRequestor.CacheServer.PrefetchEndpointUrl, latestTimestamp)), + fallbackEndPointGenerator: () => new Uri( + string.Format( + "{0}?lastPackTimestamp={1}", + this.GitObjectRequestor.CacheServer.GlobalPrefetchEndpointUrl, + latestTimestamp)), requestBodyGenerator: () => null, cancellationToken: CancellationToken.None, acceptType: new MediaTypeWithQualityHeaderValue(GVFSConstants.MediaTypes.PrefetchPackFilesAndIndexesMediaType)); @@ -188,18 +193,21 @@ public virtual bool TryDownloadPrefetchPacks(GitProcess gitProcess, long latestT if (!result.Succeeded) { + Uri requestUri = result.Result?.RequestUri + ?? new Uri(this.GitObjectRequestor.CacheServer.PrefetchEndpointUrl); + string requestAuthority = HttpRequestor.GetAuthorityForTelemetry(requestUri); if (result.Result != null && result.Result.HttpStatusCodeResult == HttpStatusCode.NotFound) { EventMetadata warning = CreateEventMetadata(); warning.Add(TracingConstants.MessageKey.WarningMessage, "The server does not support " + GVFSConstants.Endpoints.GVFSPrefetch); - warning.Add(nameof(this.GitObjectRequestor.CacheServer.PrefetchEndpointUrl), this.GitObjectRequestor.CacheServer.PrefetchEndpointUrl); + warning.Add("PrefetchEndpointUrl", requestAuthority); activity.RelatedEvent(EventLevel.Warning, "CommandNotSupported", warning); } else { EventMetadata error = CreateEventMetadata(result.Error); error.Add("latestTimestamp", latestTimestamp); - error.Add(nameof(this.GitObjectRequestor.CacheServer.PrefetchEndpointUrl), this.GitObjectRequestor.CacheServer.PrefetchEndpointUrl); + error.Add("PrefetchEndpointUrl", requestAuthority); activity.RelatedWarning(error, "DownloadPrefetchPacks failed.", Keywords.Telemetry); } } diff --git a/GVFS/GVFS.Common/Http/CacheServerInfo.cs b/GVFS/GVFS.Common/Http/CacheServerInfo.cs index 33e1b61e70..9df0399e42 100644 --- a/GVFS/GVFS.Common/Http/CacheServerInfo.cs +++ b/GVFS/GVFS.Common/Http/CacheServerInfo.cs @@ -37,6 +37,8 @@ public CacheServerInfo( this.ObjectsEndpointUrl = this.Url + ObjectsEndpointSuffix; } + this.GlobalPrefetchEndpointUrl = GetEndpointUrl(this.Url, PrefetchEndpointSuffix); + this.GlobalSizesEndpointUrl = GetEndpointUrl(this.Url, SizesEndpointSuffix); this.PrefetchEndpointUrl = GetEndpointUrl(prefetchCacheServerUrl ?? this.Url, PrefetchEndpointSuffix); this.ObjectsGetEndpointUrl = GetEndpointUrl(getCacheServerUrl ?? this.Url, ObjectsEndpointSuffix); this.ObjectsPostEndpointUrl = GetEndpointUrl(postCacheServerUrl ?? this.Url, ObjectsEndpointSuffix); @@ -69,6 +71,12 @@ public CacheServerInfo( [JsonIgnore] public string ObjectsPostEndpointUrl { get; } + [JsonIgnore] + public string GlobalPrefetchEndpointUrl { get; } + + [JsonIgnore] + public string GlobalSizesEndpointUrl { get; } + public CacheServerInfo WithEndpointOverrides( string prefetchCacheServerUrl, string getCacheServerUrl, @@ -96,7 +104,12 @@ public CacheServerInfo WithEndpointOverridesFrom(CacheServerInfo cacheServer) public bool HasValidUrl() { - return Uri.IsWellFormedUriString(this.Url, UriKind.Absolute); + return IsValidUrl(this.Url); + } + + public static bool IsValidUrl(string url) + { + return Uri.IsWellFormedUriString(url, UriKind.Absolute); } public bool IsNone(string repoUrl) diff --git a/GVFS/GVFS.Common/Http/CacheServerResolver.cs b/GVFS/GVFS.Common/Http/CacheServerResolver.cs index 26037c0a75..7a0c6e1a73 100644 --- a/GVFS/GVFS.Common/Http/CacheServerResolver.cs +++ b/GVFS/GVFS.Common/Http/CacheServerResolver.cs @@ -22,14 +22,18 @@ public static CacheServerInfo GetCacheServerFromConfig(Enlistment enlistment) { GitProcess git = enlistment.CreateGitProcess(); string url = GetUrlFromConfig(enlistment); + string prefetchCacheServerUrl = GetEndpointUrlFromConfig(git, GVFSConstants.GitConfig.PrefetchCacheServer); + string getCacheServerUrl = GetEndpointUrlFromConfig(git, GVFSConstants.GitConfig.GetCacheServer); + string postCacheServerUrl = GetEndpointUrlFromConfig(git, GVFSConstants.GitConfig.PostCacheServer); + string sizesCacheServerUrl = GetEndpointUrlFromConfig(git, GVFSConstants.GitConfig.SizesCacheServer); return new CacheServerInfo( url, url == enlistment.RepoUrl ? CacheServerInfo.ReservedNames.None : null, globalDefault: false, - GetValueFromConfig(git, GVFSConstants.GitConfig.PrefetchCacheServer, localOnly: true), - GetValueFromConfig(git, GVFSConstants.GitConfig.GetCacheServer, localOnly: true), - GetValueFromConfig(git, GVFSConstants.GitConfig.PostCacheServer, localOnly: true), - GetValueFromConfig(git, GVFSConstants.GitConfig.SizesCacheServer, localOnly: true)); + prefetchCacheServerUrl, + getCacheServerUrl, + postCacheServerUrl, + sizesCacheServerUrl); } public static string GetUrlFromConfig(Enlistment enlistment) @@ -166,6 +170,17 @@ private static string GetValueFromConfig(GitProcess git, string configName, bool return value; } + private static string GetEndpointUrlFromConfig(GitProcess git, string configName) + { + string url = GetValueFromConfig(git, configName, localOnly: true); + if (url != null && !CacheServerInfo.IsValidUrl(url)) + { + throw new InvalidRepoException($"Invalid value for {configName}: '{url}' is not an absolute URL."); + } + + return url; + } + private static bool TrySaveEndpointUrl(GitProcess git, string configName, string url, out string error) { error = null; diff --git a/GVFS/GVFS.Common/Http/GitEndPointResponseData.cs b/GVFS/GVFS.Common/Http/GitEndPointResponseData.cs index 0d450bf9d1..9eb6164219 100644 --- a/GVFS/GVFS.Common/Http/GitEndPointResponseData.cs +++ b/GVFS/GVFS.Common/Http/GitEndPointResponseData.cs @@ -4,6 +4,8 @@ using System.IO; using System.Net; using System.Net.Http; +using System.Threading; +using System.Threading.Tasks; namespace GVFS.Common.Http { @@ -30,7 +32,7 @@ public GitEndPointResponseData(HttpStatusCode statusCode, Exception error, bool public GitEndPointResponseData(HttpStatusCode statusCode, string contentType, Stream responseStream, HttpResponseMessage message, Action onResponseDisposed) : this(statusCode, null, false, message, onResponseDisposed) { - this.Stream = responseStream; + this.Stream = responseStream == null ? null : new ReadErrorTrackingStream(responseStream); this.ContentType = MapContentType(contentType); } @@ -42,6 +44,11 @@ public GitEndPointResponseData(HttpStatusCode statusCode, string contentType, St public Stream Stream { get; private set; } + public bool StreamReadFailed + { + get { return this.Stream is ReadErrorTrackingStream trackingStream && trackingStream.ReadFailed; } + } + public bool HasErrors { get { return this.StatusCode != HttpStatusCode.OK; } @@ -70,7 +77,7 @@ public string RetryableReadToEnd() { return contentStreamReader.ReadToEnd(); } - catch (Exception ex) + catch (Exception ex) when (!(ex is OperationCanceledException)) { // All exceptions potentially from network should be retried throw new RetryableException("Exception while reading stream. See inner exception for details.", ex); @@ -99,7 +106,7 @@ public List RetryableReadAllLines() line = contentStreamReader.ReadLine(); } - catch (Exception ex) + catch (Exception ex) when (!(ex is OperationCanceledException)) { // All exceptions potentially from network should be retried throw new RetryableException("Exception while reading stream. See inner exception for details.", ex); @@ -159,5 +166,83 @@ private static GitObjectContentType MapContentType(string contentType) return GitObjectContentType.None; } } + + private sealed class ReadErrorTrackingStream : Stream + { + private readonly Stream innerStream; + + public ReadErrorTrackingStream(Stream innerStream) + { + this.innerStream = innerStream; + } + + public bool ReadFailed { get; private set; } + + public override bool CanRead => this.innerStream.CanRead; + + public override bool CanSeek => this.innerStream.CanSeek; + + public override bool CanWrite => this.innerStream.CanWrite; + + public override long Length => this.innerStream.Length; + + public override long Position + { + get { return this.innerStream.Position; } + set { this.innerStream.Position = value; } + } + + public override void Flush() => this.innerStream.Flush(); + + public override int Read(byte[] buffer, int offset, int count) => + this.TrackRead(() => this.innerStream.Read(buffer, offset, count)); + + public override int ReadByte() => this.TrackRead(this.innerStream.ReadByte); + + public override Task ReadAsync(byte[] buffer, int offset, int count, CancellationToken cancellationToken) => + this.TrackReadAsync(() => this.innerStream.ReadAsync(buffer, offset, count, cancellationToken)); + + public override long Seek(long offset, SeekOrigin origin) => this.innerStream.Seek(offset, origin); + + public override void SetLength(long value) => this.innerStream.SetLength(value); + + public override void Write(byte[] buffer, int offset, int count) => this.innerStream.Write(buffer, offset, count); + + protected override void Dispose(bool disposing) + { + if (disposing) + { + this.innerStream.Dispose(); + } + + base.Dispose(disposing); + } + + private T TrackRead(Func read) + { + try + { + return read(); + } + catch (Exception ex) when (!(ex is OperationCanceledException)) + { + this.ReadFailed = true; + throw; + } + } + + private async Task TrackReadAsync(Func> read) + { + try + { + return await read().ConfigureAwait(false); + } + catch (Exception ex) when (!(ex is OperationCanceledException)) + { + this.ReadFailed = true; + throw; + } + } + } } } diff --git a/GVFS/GVFS.Common/Http/GitObjectsHttpRequestor.cs b/GVFS/GVFS.Common/Http/GitObjectsHttpRequestor.cs index 45e4ba8c4f..278351b8ee 100644 --- a/GVFS/GVFS.Common/Http/GitObjectsHttpRequestor.cs +++ b/GVFS/GVFS.Common/Http/GitObjectsHttpRequestor.cs @@ -2,6 +2,7 @@ using GVFS.Common.Tracing; using System; using System.Collections.Generic; +using System.IO; using System.Text.Json.Serialization; using System.Linq; using System.Net; @@ -34,8 +35,12 @@ public virtual List QueryForFileSizes(IEnumerable objectI long requestId = HttpRequestor.GetNewRequestId(); string objectIdsJson = ToJsonList(objectIds); - Uri cacheServerEndpoint = new Uri(this.CacheServer.SizesEndpointUrl); + Uri preferredCacheServerEndpoint = new Uri(this.CacheServer.SizesEndpointUrl); + Uri globalCacheServerEndpoint = new Uri(this.CacheServer.GlobalSizesEndpointUrl); Uri originEndpoint = new Uri(this.enlistment.RepoUrl + GVFSConstants.Endpoints.GVFSSizes); + bool hasEndpointOverride = preferredCacheServerEndpoint != globalCacheServerEndpoint; + bool useGlobalCacheServer = !hasEndpointOverride; + bool useOrigin = this.nextCacheServerAttemptTime >= DateTime.Now; EventMetadata metadata = new EventMetadata(); metadata.Add("RequestId", requestId); @@ -51,38 +56,91 @@ public virtual List QueryForFileSizes(IEnumerable objectI this.Tracer.RelatedEvent(EventLevel.Informational, "QueryFileSizes", metadata, Keywords.Network); - RetryWrapper> retrier = new RetryWrapper>(this.RetryConfig.MaxAttempts, cancellationToken); + RetryWrapper> retrier = new RetryWrapper>( + this.RetryConfig.MaxAttempts + (hasEndpointOverride && !useOrigin ? 2 : 0), + cancellationToken); retrier.OnFailure += RetryWrapper>.StandardErrorHandler(this.Tracer, requestId, "QueryFileSizes"); RetryWrapper>.InvocationResult requestTask = retrier.Invoke( tryCount => { Uri gvfsEndpoint; - if (this.nextCacheServerAttemptTime < DateTime.Now) + if (useOrigin) + { + gvfsEndpoint = originEndpoint; + } + else if (useGlobalCacheServer) { - gvfsEndpoint = cacheServerEndpoint; + gvfsEndpoint = globalCacheServerEndpoint; } else { - gvfsEndpoint = originEndpoint; + gvfsEndpoint = preferredCacheServerEndpoint; } - using (GitEndPointResponseData response = this.SendRequest(requestId, gvfsEndpoint, HttpMethod.Post, objectIdsJson, cancellationToken)) + try { - if (response.StatusCode == HttpStatusCode.NotFound) + using (GitEndPointResponseData response = this.SendProtocolRequest(requestId, gvfsEndpoint, HttpMethod.Post, objectIdsJson, cancellationToken)) { - this.nextCacheServerAttemptTime = DateTime.Now.AddDays(1); - return new RetryWrapper>.CallbackResult(response.Error, true); + if (response.HasErrors && !useGlobalCacheServer && !useOrigin) + { + this.TraceCacheServerFallback( + requestId, + preferredCacheServerEndpoint, + globalCacheServerEndpoint, + "EndpointSpecific", + "Global"); + useGlobalCacheServer = true; + return new RetryWrapper>.CallbackResult( + response.Error, + shouldRetry: true, + result: null, + shouldRecordFailure: false); + } + + if (response.StatusCode == HttpStatusCode.NotFound) + { + if (!useOrigin) + { + this.TraceCacheServerFallback( + requestId, + globalCacheServerEndpoint, + originEndpoint, + "Global", + "Origin"); + } + + this.nextCacheServerAttemptTime = DateTime.Now.AddDays(1); + useOrigin = true; + return new RetryWrapper>.CallbackResult( + response.Error, + shouldRetry: true, + result: null, + shouldRecordFailure: false); + } + + if (response.HasErrors) + { + return new RetryWrapper>.CallbackResult(response.Error, response.ShouldRetry); + } + + string objectSizesString = response.RetryableReadToEnd(); + List objectSizes = GVFSJsonOptions.Deserialize>(objectSizesString); + return new RetryWrapper>.CallbackResult(objectSizes); } - - if (response.HasErrors) - { - return new RetryWrapper>.CallbackResult(response.Error, response.ShouldRetry); - } - - string objectSizesString = response.RetryableReadToEnd(); - List objectSizes = GVFSJsonOptions.Deserialize>(objectSizesString); - return new RetryWrapper>.CallbackResult(objectSizes); + } + catch (Exception e) when ( + (e is HttpRequestException || e is IOException || e is RetryableException) && + !useGlobalCacheServer && + !useOrigin) + { + this.TraceCacheServerFallback(requestId, preferredCacheServerEndpoint, globalCacheServerEndpoint, "EndpointSpecific", "Global"); + useGlobalCacheServer = true; + return new RetryWrapper>.CallbackResult( + e, + shouldRetry: true, + result: null, + shouldRecordFailure: false); } }); @@ -109,7 +167,7 @@ public virtual GitRefs QueryInfoRefs(string branch) RetryWrapper.InvocationResult output = retrier.Invoke( tryCount => { - using (GitEndPointResponseData response = this.SendRequest( + using (GitEndPointResponseData response = this.SendProtocolRequest( requestId, infoRefsEndpoint, HttpMethod.Get, @@ -150,6 +208,7 @@ public virtual RetryWrapper.InvocationResult TryDownloadLoo eArgs => this.HandleDownloadAndSaveObjectError(retryOnFailure, requestId, eArgs), HttpMethod.Get, new Uri(this.CacheServer.ObjectsGetEndpointUrl + "/" + objectId), + new Uri(this.CacheServer.ObjectsEndpointUrl + "/" + objectId), cancellationToken, requestBody: null, acceptType: null, @@ -170,10 +229,11 @@ public virtual RetryWrapper.InvocationResult TryDownloadObj onSuccess, onFailure, HttpMethod.Post, - new Uri(this.CacheServer.ObjectsPostEndpointUrl), - CancellationToken.None, - () => this.ObjectIdsJsonGenerator(requestId, objectIdGenerator), - preferBatchedLooseObjects ? CustomLooseObjectsHeader : null); + () => new Uri(this.CacheServer.ObjectsPostEndpointUrl), + requestBodyGenerator: () => this.ObjectIdsJsonGenerator(requestId, objectIdGenerator), + cancellationToken: CancellationToken.None, + acceptType: preferBatchedLooseObjects ? CustomLooseObjectsHeader : null, + fallbackEndPointGenerator: () => new Uri(this.CacheServer.ObjectsEndpointUrl)); } public virtual RetryWrapper.InvocationResult TryDownloadObjects( @@ -205,11 +265,37 @@ public virtual RetryWrapper.InvocationResult TryDownloadObj onFailure, HttpMethod.Post, new Uri(this.CacheServer.ObjectsPostEndpointUrl), + new Uri(this.CacheServer.ObjectsEndpointUrl), CancellationToken.None, objectIdsJson, preferBatchedLooseObjects ? CustomLooseObjectsHeader : null); } + public virtual RetryWrapper.InvocationResult TrySendProtocolRequest( + long requestId, + Func.CallbackResult> onSuccess, + Action.ErrorEventArgs> onFailure, + HttpMethod method, + Uri endPoint, + Uri fallbackEndPoint, + CancellationToken cancellationToken, + string requestBody = null, + MediaTypeWithQualityHeaderValue acceptType = null, + bool retryOnFailure = true) + { + return this.TrySendProtocolRequest( + requestId, + onSuccess, + onFailure, + method, + () => endPoint, + requestBodyGenerator: () => requestBody, + cancellationToken: cancellationToken, + acceptType: acceptType, + retryOnFailure: retryOnFailure, + fallbackEndPointGenerator: () => fallbackEndPoint); + } + public virtual RetryWrapper.InvocationResult TrySendProtocolRequest( long requestId, Func.CallbackResult> onSuccess, @@ -227,10 +313,11 @@ public virtual RetryWrapper.InvocationResult TrySendProtoco onFailure, method, endPoint, - cancellationToken, - () => requestBody, - acceptType, - retryOnFailure); + fallbackEndPoint: null, + cancellationToken: cancellationToken, + requestBody: requestBody, + acceptType: acceptType, + retryOnFailure: retryOnFailure); } public virtual RetryWrapper.InvocationResult TrySendProtocolRequest( @@ -250,10 +337,10 @@ public virtual RetryWrapper.InvocationResult TrySendProtoco onFailure, method, () => endPoint, - requestBodyGenerator, - cancellationToken, - acceptType, - retryOnFailure); + requestBodyGenerator: requestBodyGenerator, + cancellationToken: cancellationToken, + acceptType: acceptType, + retryOnFailure: retryOnFailure); } public virtual RetryWrapper.InvocationResult TrySendProtocolRequest( @@ -265,10 +352,16 @@ public virtual RetryWrapper.InvocationResult TrySendProtoco Func requestBodyGenerator, CancellationToken cancellationToken, MediaTypeWithQualityHeaderValue acceptType = null, - bool retryOnFailure = true) + bool retryOnFailure = true, + Func fallbackEndPointGenerator = null) { + Uri endPoint = endPointGenerator(); + Uri fallbackEndPoint = fallbackEndPointGenerator?.Invoke(); + bool hasFallbackEndPoint = fallbackEndPoint != null && endPoint != fallbackEndPoint; + bool useFallbackEndPoint = false; + RetryWrapper retrier = new RetryWrapper( - retryOnFailure ? this.RetryConfig.MaxAttempts : 1, + (retryOnFailure ? this.RetryConfig.MaxAttempts : 1) + (hasFallbackEndPoint ? 1 : 0), cancellationToken); if (onFailure != null) { @@ -278,24 +371,182 @@ public virtual RetryWrapper.InvocationResult TrySendProtoco return retrier.Invoke( tryCount => { - using (GitEndPointResponseData response = this.SendRequest( - requestId, - endPointGenerator(), - method, - requestBodyGenerator(), - cancellationToken, - acceptType)) + Uri requestEndPoint = useFallbackEndPoint ? fallbackEndPointGenerator() : endPointGenerator(); + GitEndPointResponseData response; + + try + { + response = this.SendProtocolRequest( + requestId, + requestEndPoint, + method, + requestBodyGenerator(), + cancellationToken, + acceptType); + } + catch (HttpRequestException e) + { + return this.HandleProtocolException( + requestId, + e, + requestEndPoint, + fallbackEndPoint, + hasFallbackEndPoint, + ref useFallbackEndPoint, + retryOnFailure); + } + catch (IOException e) + { + return this.HandleProtocolException( + requestId, + e, + requestEndPoint, + fallbackEndPoint, + hasFallbackEndPoint, + ref useFallbackEndPoint, + retryOnFailure); + } + catch (RetryableException e) + { + return this.HandleProtocolException( + requestId, + e, + requestEndPoint, + fallbackEndPoint, + hasFallbackEndPoint, + ref useFallbackEndPoint, + retryOnFailure); + } + + using (response) { if (response.HasErrors) { - return new RetryWrapper.CallbackResult(response.Error, response.ShouldRetry, new GitObjectTaskResult(response.StatusCode)); + bool shouldFallBack = hasFallbackEndPoint && !useFallbackEndPoint; + if (shouldFallBack) + { + this.TraceCacheServerFallback( + requestId, + requestEndPoint, + fallbackEndPoint, + "EndpointSpecific", + "Global"); + } + + useFallbackEndPoint |= shouldFallBack; + return new RetryWrapper.CallbackResult( + response.Error, + shouldFallBack || response.ShouldRetry, + new GitObjectTaskResult(response.StatusCode, requestEndPoint), + shouldRecordFailure: response.ShouldRetry && !shouldFallBack); } - return onSuccess(tryCount, response); + RetryWrapper.CallbackResult result; + try + { + result = onSuccess(tryCount, response); + } + catch (Exception e) + { + if (response.StreamReadFailed) + { + return this.HandleProtocolException( + requestId, + e, + requestEndPoint, + fallbackEndPoint, + hasFallbackEndPoint, + ref useFallbackEndPoint, + retryOnFailure); + } + + throw; + } + + if (result.HasErrors) + { + bool shouldFallBack = response.StreamReadFailed && hasFallbackEndPoint && !useFallbackEndPoint; + if (shouldFallBack) + { + this.TraceCacheServerFallback( + requestId, + requestEndPoint, + fallbackEndPoint, + "EndpointSpecific", + "Global"); + useFallbackEndPoint = true; + } + + GitObjectTaskResult requestResult = result.Result == null + ? new GitObjectTaskResult(success: false, requestEndPoint) + : result.Result.WithRequestUri(requestEndPoint); + return new RetryWrapper.CallbackResult( + result.Error, + shouldFallBack || result.ShouldRetry, + requestResult, + shouldRecordFailure: result.ShouldRecordFailure && !shouldFallBack); + } + + return result; } }); } + private RetryWrapper.CallbackResult HandleProtocolException( + long requestId, + Exception error, + Uri requestEndPoint, + Uri fallbackEndPoint, + bool hasFallbackEndPoint, + ref bool useFallbackEndPoint, + bool retryOnFailure) + { + bool shouldFallBack = hasFallbackEndPoint && !useFallbackEndPoint; + if (shouldFallBack) + { + this.TraceCacheServerFallback( + requestId, + requestEndPoint, + fallbackEndPoint, + "EndpointSpecific", + "Global"); + useFallbackEndPoint = true; + } + + return new RetryWrapper.CallbackResult( + error, + shouldFallBack || retryOnFailure, + new GitObjectTaskResult(success: false, requestEndPoint), + shouldRecordFailure: retryOnFailure && !shouldFallBack); + } + + private void TraceCacheServerFallback( + long requestId, + Uri source, + Uri target, + string sourceRoute, + string targetRoute) + { + EventMetadata metadata = new EventMetadata(); + metadata.Add("RequestId", requestId); + metadata.Add("SourceRoute", sourceRoute); + metadata.Add("SourceAuthority", GetAuthorityForTelemetry(source)); + metadata.Add("TargetRoute", targetRoute); + metadata.Add("TargetAuthority", GetAuthorityForTelemetry(target)); + this.Tracer.RelatedEvent(EventLevel.Informational, "CacheServerFallback", metadata, Keywords.Network | Keywords.Telemetry); + } + + protected virtual GitEndPointResponseData SendProtocolRequest( + long requestId, + Uri requestUri, + HttpMethod httpMethod, + string requestContent, + CancellationToken cancellationToken, + MediaTypeWithQualityHeaderValue acceptType = null) + { + return this.SendRequest(requestId, requestUri, httpMethod, requestContent, cancellationToken, acceptType); + } + private static string ToJsonList(IEnumerable strings) { return "[\"" + string.Join("\",\"", strings) + "\"]"; @@ -356,19 +607,29 @@ public GitObjectSize(string id, long size) public class GitObjectTaskResult { - public GitObjectTaskResult(bool success) + public GitObjectTaskResult(bool success, Uri requestUri = null) { this.Success = success; + this.RequestUri = requestUri; } - public GitObjectTaskResult(HttpStatusCode statusCode) - : this(statusCode == HttpStatusCode.OK) + public GitObjectTaskResult(HttpStatusCode statusCode, Uri requestUri = null) + : this(statusCode == HttpStatusCode.OK, requestUri) { this.HttpStatusCodeResult = statusCode; } public bool Success { get; } - public HttpStatusCode HttpStatusCodeResult { get; } + public HttpStatusCode HttpStatusCodeResult { get; private set; } + public Uri RequestUri { get; } + + public GitObjectTaskResult WithRequestUri(Uri requestUri) + { + return new GitObjectTaskResult(this.Success, requestUri) + { + HttpStatusCodeResult = this.HttpStatusCodeResult, + }; + } } } } \ No newline at end of file diff --git a/GVFS/GVFS.Common/Http/HttpRequestor.cs b/GVFS/GVFS.Common/Http/HttpRequestor.cs index 435e52c2b1..ee1558abf2 100644 --- a/GVFS/GVFS.Common/Http/HttpRequestor.cs +++ b/GVFS/GVFS.Common/Http/HttpRequestor.cs @@ -312,11 +312,11 @@ protected GitEndPointResponseData SendRequest( } private static bool ShouldRetry(HttpStatusCode statusCode) - { + { // Retry timeout, Unauthorized, 429 (Too Many Requests), and 5xx errors int statusInt = (int)statusCode; if (statusCode == HttpStatusCode.RequestTimeout || - statusCode == HttpStatusCode.Unauthorized || + statusCode == HttpStatusCode.Unauthorized || statusInt == 429 || (statusInt >= 500 && statusInt < 600)) { @@ -378,6 +378,11 @@ internal static bool ShouldRejectCredentials(HttpStatusCode statusCode, string r return false; } + internal static string GetAuthorityForTelemetry(Uri uri) + { + return uri.Authority; + } + private static string GetSingleHeaderOrEmpty(HttpHeaders headers, string headerName) { IEnumerable values; diff --git a/GVFS/GVFS.Common/RetryWrapper.cs b/GVFS/GVFS.Common/RetryWrapper.cs index 4d6a0ccd84..4d56ccc1fa 100644 --- a/GVFS/GVFS.Common/RetryWrapper.cs +++ b/GVFS/GVFS.Common/RetryWrapper.cs @@ -88,7 +88,7 @@ public InvocationResult Invoke(Func toInvoke) CallbackResult result = toInvoke(tryCount); if (result.HasErrors) { - if (result.ShouldRetry) + if (result.ShouldRecordFailure) { RetryCircuitBreaker.RecordFailure(); } @@ -224,6 +224,7 @@ public CallbackResult(Exception error, bool shouldRetry) this.HasErrors = true; this.Error = error; this.ShouldRetry = shouldRetry; + this.ShouldRecordFailure = shouldRetry; } public CallbackResult(Exception error, bool shouldRetry, T result) @@ -232,9 +233,16 @@ public CallbackResult(Exception error, bool shouldRetry, T result) this.Result = result; } + public CallbackResult(Exception error, bool shouldRetry, T result, bool shouldRecordFailure) + : this(error, shouldRetry, result) + { + this.ShouldRecordFailure = shouldRecordFailure; + } + public bool HasErrors { get; } public Exception Error { get; } public bool ShouldRetry { get; } + public bool ShouldRecordFailure { get; } public T Result { get; } } } diff --git a/GVFS/GVFS.UnitTests/Common/CacheServerResolverTests.cs b/GVFS/GVFS.UnitTests/Common/CacheServerResolverTests.cs index 651e05343b..f338fedadf 100644 --- a/GVFS/GVFS.UnitTests/Common/CacheServerResolverTests.cs +++ b/GVFS/GVFS.UnitTests/Common/CacheServerResolverTests.cs @@ -91,6 +91,20 @@ public void EndpointSpecificCacheServersArePreservedWhenGlobalCacheServerIsResol resolvedCacheServer.HasValidUrl().ShouldEqual(true); } + [TestCase] + public void InvalidEndpointSpecificCacheServerIsRejected() + { + MockGVFSEnlistment enlistment = this.CreateEnlistment( + CacheServerUrl, + prefetchCacheServerUrl: "not-a-url"); + + InvalidRepoException exception = Assert.Throws( + () => CacheServerResolver.GetCacheServerFromConfig(enlistment)); + + exception.Message.ShouldContain(GVFSConstants.GitConfig.PrefetchCacheServer); + exception.Message.ShouldContain("not an absolute URL"); + } + [TestCase] public void CanSaveEndpointSpecificCacheServers() { diff --git a/GVFS/GVFS.UnitTests/Http/GitObjectsHttpRequestorTests.cs b/GVFS/GVFS.UnitTests/Http/GitObjectsHttpRequestorTests.cs new file mode 100644 index 0000000000..ee89a68ae2 --- /dev/null +++ b/GVFS/GVFS.UnitTests/Http/GitObjectsHttpRequestorTests.cs @@ -0,0 +1,485 @@ +using GVFS.Common; +using GVFS.Common.Git; +using GVFS.Common.Http; +using GVFS.Common.Tracing; +using GVFS.Tests.Should; +using GVFS.UnitTests.Mock.Common; +using NUnit.Framework; +using System; +using System.Collections.Generic; +using System.IO; +using System.Net; +using System.Net.Http; +using System.Net.Http.Headers; +using System.Text; +using System.Threading; + +namespace GVFS.UnitTests.Http +{ + [TestFixture] + public class GitObjectsHttpRequestorTests + { + private const string GlobalCacheServerUrl = "https://global-cache/server"; + private const string EndpointCacheServerUrl = "https://endpoint-cache/server"; + + [SetUp] + public void SetUp() + { + RetryCircuitBreaker.Reset(); + } + + [TearDown] + public void TearDown() + { + RetryCircuitBreaker.Reset(); + } + + [TestCase] + public void LooseObjectFallsBackToGlobalCacheServerWhenEndpointRequestFails() + { + TestGitObjectsHttpRequestor requestor = this.CreateRequestor(maxRetries: 0); + requestor.EnqueueResponse(HttpStatusCode.NotFound); + requestor.EnqueueResponse(HttpStatusCode.OK); + + RetryWrapper.InvocationResult result = + requestor.TryDownloadLooseObject( + "0123456789abcdef", + retryOnFailure: false, + CancellationToken.None, + requestSource: "test", + onSuccess: SuccessfulRequest); + + result.Succeeded.ShouldEqual(true); + requestor.RequestUris.Count.ShouldEqual(2); + requestor.RequestUris[0].AbsoluteUri.ShouldEqual(EndpointCacheServerUrl + "/gvfs/objects/0123456789abcdef"); + requestor.RequestUris[1].AbsoluteUri.ShouldEqual(GlobalCacheServerUrl + "/gvfs/objects/0123456789abcdef"); + requestor.TestTracer.RelatedEventNames.ShouldContain(name => name == "CacheServerFallback"); + requestor.TestTracer.RelatedEventKeywords.ShouldContain( + keywords => (keywords & Keywords.Telemetry) == Keywords.Telemetry); + } + + [TestCase] + public void BatchedObjectRequestFallsBackToGlobalCacheServer() + { + TestGitObjectsHttpRequestor requestor = this.CreateRequestor(maxRetries: 0); + requestor.EnqueueResponse(HttpStatusCode.ServiceUnavailable, shouldRetry: true); + requestor.EnqueueResponse(HttpStatusCode.NotFound); + + RetryWrapper.InvocationResult result = + requestor.TryDownloadObjects( + new[] { "0123456789abcdef" }, + onSuccess: SuccessfulRequest, + onFailure: null, + preferBatchedLooseObjects: false); + + result.Succeeded.ShouldEqual(false); + requestor.RequestUris.Count.ShouldEqual(2); + requestor.RequestUris[0].AbsoluteUri.ShouldEqual(EndpointCacheServerUrl + "/gvfs/objects"); + requestor.RequestUris[1].AbsoluteUri.ShouldEqual(GlobalCacheServerUrl + "/gvfs/objects"); + RetryCircuitBreaker.ConsecutiveFailures.ShouldEqual(0); + } + + [TestCase] + public void PrefetchRequestFallsBackToGlobalCacheServer() + { + TestGitObjectsHttpRequestor requestor = this.CreateRequestor(maxRetries: 0); + requestor.EnqueueResponse(HttpStatusCode.BadRequest); + requestor.EnqueueResponse(HttpStatusCode.OK); + + RetryWrapper.InvocationResult result = + requestor.TrySendProtocolRequest( + requestId: 1, + onSuccess: SuccessfulRequest, + onFailure: null, + method: HttpMethod.Get, + endPointGenerator: () => new Uri(EndpointCacheServerUrl + "/gvfs/prefetch?lastPackTimestamp=0"), + fallbackEndPointGenerator: () => new Uri(GlobalCacheServerUrl + "/gvfs/prefetch?lastPackTimestamp=0"), + requestBodyGenerator: () => null, + cancellationToken: CancellationToken.None); + + result.Succeeded.ShouldEqual(true); + requestor.RequestUris.Count.ShouldEqual(2); + requestor.RequestUris[0].AbsoluteUri.ShouldEqual(EndpointCacheServerUrl + "/gvfs/prefetch?lastPackTimestamp=0"); + requestor.RequestUris[1].AbsoluteUri.ShouldEqual(GlobalCacheServerUrl + "/gvfs/prefetch?lastPackTimestamp=0"); + } + + [TestCase] + public void TransportExceptionFallsBackToGlobalCacheServer() + { + TestGitObjectsHttpRequestor requestor = this.CreateRequestor(maxRetries: 0); + requestor.EnqueueException(new HttpRequestException("Test failure")); + requestor.EnqueueResponse(HttpStatusCode.NotFound); + + RetryWrapper.InvocationResult result = + requestor.TryDownloadObjects( + new[] { "0123456789abcdef" }, + onSuccess: SuccessfulRequest, + onFailure: null, + preferBatchedLooseObjects: false); + + result.Succeeded.ShouldEqual(false); + requestor.RequestUris.Count.ShouldEqual(2); + requestor.RequestUris[0].AbsoluteUri.ShouldEqual(EndpointCacheServerUrl + "/gvfs/objects"); + requestor.RequestUris[1].AbsoluteUri.ShouldEqual(GlobalCacheServerUrl + "/gvfs/objects"); + RetryCircuitBreaker.ConsecutiveFailures.ShouldEqual(0); + } + + [TestCase] + public void ResponseBodyReadFailureFallsBackToGlobalCacheServer() + { + TestGitObjectsHttpRequestor requestor = this.CreateRequestor(maxRetries: 0); + requestor.EnqueueResponse(new ThrowingReadStream()); + requestor.EnqueueResponse(HttpStatusCode.NotFound); + + RetryWrapper.InvocationResult result = + requestor.TryDownloadObjects( + new[] { "0123456789abcdef" }, + onSuccess: (tryCount, response) => + { + response.Stream.ReadByte(); + return SuccessfulRequest(tryCount, response); + }, + onFailure: null, + preferBatchedLooseObjects: false); + + result.Succeeded.ShouldEqual(false); + requestor.RequestUris.Count.ShouldEqual(2); + requestor.RequestUris[0].AbsoluteUri.ShouldEqual(EndpointCacheServerUrl + "/gvfs/objects"); + requestor.RequestUris[1].AbsoluteUri.ShouldEqual(GlobalCacheServerUrl + "/gvfs/objects"); + RetryCircuitBreaker.ConsecutiveFailures.ShouldEqual(0); + } + + [TestCase] + public void ResponseBodyReadFailureReportedByHandlerFallsBackToGlobalCacheServer() + { + TestGitObjectsHttpRequestor requestor = this.CreateRequestor(maxRetries: 0); + requestor.EnqueueResponse(new ThrowingReadStream()); + requestor.EnqueueResponse(HttpStatusCode.OK); + + RetryWrapper.InvocationResult result = + requestor.TryDownloadObjects( + new[] { "0123456789abcdef" }, + onSuccess: (tryCount, response) => + { + try + { + response.Stream.ReadByte(); + return SuccessfulRequest(tryCount, response); + } + catch (IOException e) + { + return new RetryWrapper.CallbackResult( + e, + shouldRetry: true); + } + }, + onFailure: null, + preferBatchedLooseObjects: false); + + result.Succeeded.ShouldEqual(true); + requestor.RequestUris.Count.ShouldEqual(2); + requestor.RequestUris[0].AbsoluteUri.ShouldEqual(EndpointCacheServerUrl + "/gvfs/objects"); + requestor.RequestUris[1].AbsoluteUri.ShouldEqual(GlobalCacheServerUrl + "/gvfs/objects"); + RetryCircuitBreaker.ConsecutiveFailures.ShouldEqual(0); + } + + [TestCase] + public void ResponseBodyCancellationIsNotRetriedOrReportedAsFallback() + { + TestGitObjectsHttpRequestor requestor = this.CreateRequestor(maxRetries: 1); + requestor.EnqueueResponse(new CancelingReadStream()); + + Assert.Throws( + () => requestor.TryDownloadObjects( + new[] { "0123456789abcdef" }, + onSuccess: (tryCount, response) => + { + response.RetryableReadToEnd(); + return SuccessfulRequest(tryCount, response); + }, + onFailure: null, + preferBatchedLooseObjects: false)); + + requestor.RequestUris.Count.ShouldEqual(1); + requestor.TestTracer.RelatedEventNames.ShouldNotContain(name => name == "CacheServerFallback"); + RetryCircuitBreaker.ConsecutiveFailures.ShouldEqual(0); + } + + [TestCase] + public void FallbackTelemetryExcludesCredentialsAndRequestPath() + { + const string CredentialedGlobalUrl = "https://global-user:global-secret@global-cache:8443/server"; + const string CredentialedEndpointUrl = "https://endpoint-user:endpoint-secret@endpoint-cache:9443/server"; + TestGitObjectsHttpRequestor requestor = this.CreateRequestor( + maxRetries: 0, + globalCacheServerUrl: CredentialedGlobalUrl, + endpointCacheServerUrl: CredentialedEndpointUrl); + requestor.EnqueueResponse(HttpStatusCode.NotFound); + requestor.EnqueueResponse(HttpStatusCode.OK); + + requestor.TryDownloadLooseObject( + "0123456789abcdef", + retryOnFailure: false, + CancellationToken.None, + requestSource: "test", + onSuccess: SuccessfulRequest); + + int fallbackEventIndex = requestor.TestTracer.RelatedEventNames.IndexOf("CacheServerFallback"); + EventMetadata metadata = requestor.TestTracer.RelatedEventMetadata[fallbackEventIndex]; + metadata["SourceAuthority"].ShouldEqual("endpoint-cache:9443"); + metadata["TargetAuthority"].ShouldEqual("global-cache:8443"); + } + + [TestCase] + public void NoEndpointOverrideUsesNormalGlobalCacheRetries() + { + TestGitObjectsHttpRequestor requestor = this.CreateRequestor(maxRetries: 1, endpointOverrides: false); + requestor.EnqueueResponse(HttpStatusCode.ServiceUnavailable, shouldRetry: true); + requestor.EnqueueResponse(HttpStatusCode.OK); + + RetryWrapper.InvocationResult result = + requestor.TryDownloadObjects( + new[] { "0123456789abcdef" }, + onSuccess: SuccessfulRequest, + onFailure: null, + preferBatchedLooseObjects: false); + + result.Succeeded.ShouldEqual(true); + requestor.RequestUris.Count.ShouldEqual(2); + requestor.RequestUris[0].AbsoluteUri.ShouldEqual(GlobalCacheServerUrl + "/gvfs/objects"); + requestor.RequestUris[1].AbsoluteUri.ShouldEqual(GlobalCacheServerUrl + "/gvfs/objects"); + requestor.TestTracer.RelatedEventNames.ShouldNotContain(name => name == "CacheServerFallback"); + } + + [TestCase] + public void TerminalFallbackFailureReportsGlobalCacheServer() + { + TestGitObjectsHttpRequestor requestor = this.CreateRequestor(maxRetries: 0); + requestor.EnqueueResponse(HttpStatusCode.ServiceUnavailable); + requestor.EnqueueResponse(HttpStatusCode.NotFound); + + RetryWrapper.InvocationResult result = + requestor.TryDownloadObjects( + new[] { "0123456789abcdef" }, + onSuccess: SuccessfulRequest, + onFailure: null, + preferBatchedLooseObjects: false); + + result.Succeeded.ShouldEqual(false); + result.Attempts.ShouldEqual(2); + result.Result.HttpStatusCodeResult.ShouldEqual(HttpStatusCode.NotFound); + result.Result.RequestUri.AbsoluteUri.ShouldEqual(GlobalCacheServerUrl + "/gvfs/objects"); + RetryCircuitBreaker.ConsecutiveFailures.ShouldEqual(0); + } + + [TestCase] + public void SuccessHandlerFailureRetriesTheEndpointSpecificServer() + { + TestGitObjectsHttpRequestor requestor = this.CreateRequestor(maxRetries: 1); + requestor.EnqueueResponse(HttpStatusCode.OK); + requestor.EnqueueResponse(HttpStatusCode.OK); + int successHandlerCalls = 0; + + RetryWrapper.InvocationResult result = + requestor.TryDownloadObjects( + new[] { "0123456789abcdef" }, + onSuccess: (tryCount, response) => + { + if (++successHandlerCalls == 1) + { + throw new RetryableException("Local write failed"); + } + + return SuccessfulRequest(tryCount, response); + }, + onFailure: null, + preferBatchedLooseObjects: false); + + result.Succeeded.ShouldEqual(true); + requestor.RequestUris.Count.ShouldEqual(2); + requestor.RequestUris[0].AbsoluteUri.ShouldEqual(EndpointCacheServerUrl + "/gvfs/objects"); + requestor.RequestUris[1].AbsoluteUri.ShouldEqual(EndpointCacheServerUrl + "/gvfs/objects"); + requestor.TestTracer.RelatedEventNames.ShouldNotContain(name => name == "CacheServerFallback"); + } + + [TestCase] + public void SizesRequestFallsBackThroughGlobalCacheServerToOrigin() + { + TestGitObjectsHttpRequestor requestor = this.CreateRequestor(maxRetries: 0); + requestor.EnqueueResponse(HttpStatusCode.ServiceUnavailable); + requestor.EnqueueResponse(HttpStatusCode.NotFound); + requestor.EnqueueResponse(HttpStatusCode.OK, "[]"); + + requestor.QueryForFileSizes(new[] { "0123456789abcdef" }, CancellationToken.None); + + requestor.RequestUris.Count.ShouldEqual(3); + requestor.RequestUris[0].AbsoluteUri.ShouldEqual(EndpointCacheServerUrl + "/gvfs/sizes"); + requestor.RequestUris[1].AbsoluteUri.ShouldEqual(GlobalCacheServerUrl + "/gvfs/sizes"); + requestor.RequestUris[2].AbsoluteUri.ShouldEqual("mock://repourl/gvfs/sizes"); + RetryCircuitBreaker.ConsecutiveFailures.ShouldEqual(0); + } + + [TestCase] + public void SizesHttpFallbackDoesNotChargeCircuitBreaker() + { + TestGitObjectsHttpRequestor requestor = this.CreateRequestor(maxRetries: 0); + requestor.EnqueueResponse(HttpStatusCode.ServiceUnavailable, shouldRetry: true); + requestor.EnqueueResponse(HttpStatusCode.ServiceUnavailable); + + requestor.QueryForFileSizes(new[] { "0123456789abcdef" }, CancellationToken.None); + + requestor.RequestUris.Count.ShouldEqual(2); + requestor.RequestUris[0].AbsoluteUri.ShouldEqual(EndpointCacheServerUrl + "/gvfs/sizes"); + requestor.RequestUris[1].AbsoluteUri.ShouldEqual(GlobalCacheServerUrl + "/gvfs/sizes"); + RetryCircuitBreaker.ConsecutiveFailures.ShouldEqual(0); + } + + [TestCase] + public void SizesTransportFallbackDoesNotChargeCircuitBreaker() + { + TestGitObjectsHttpRequestor requestor = this.CreateRequestor(maxRetries: 0); + requestor.EnqueueException(new HttpRequestException("Test failure")); + requestor.EnqueueResponse(HttpStatusCode.ServiceUnavailable); + + requestor.QueryForFileSizes(new[] { "0123456789abcdef" }, CancellationToken.None); + + requestor.RequestUris.Count.ShouldEqual(2); + requestor.RequestUris[0].AbsoluteUri.ShouldEqual(EndpointCacheServerUrl + "/gvfs/sizes"); + requestor.RequestUris[1].AbsoluteUri.ShouldEqual(GlobalCacheServerUrl + "/gvfs/sizes"); + RetryCircuitBreaker.ConsecutiveFailures.ShouldEqual(0); + } + + private static RetryWrapper.CallbackResult SuccessfulRequest( + int tryCount, + GitEndPointResponseData response) + { + return new RetryWrapper.CallbackResult( + new GitObjectsHttpRequestor.GitObjectTaskResult(true)); + } + + private TestGitObjectsHttpRequestor CreateRequestor( + int maxRetries, + bool endpointOverrides = true, + string globalCacheServerUrl = GlobalCacheServerUrl, + string endpointCacheServerUrl = EndpointCacheServerUrl) + { + CacheServerInfo cacheServer = new CacheServerInfo(globalCacheServerUrl, "global"); + if (endpointOverrides) + { + cacheServer = cacheServer.WithEndpointOverrides( + endpointCacheServerUrl, + endpointCacheServerUrl, + endpointCacheServerUrl, + endpointCacheServerUrl); + } + + return new TestGitObjectsHttpRequestor( + new MockGVFSEnlistment(), + cacheServer, + new RetryConfig(maxRetries)); + } + + private class TestGitObjectsHttpRequestor : GitObjectsHttpRequestor + { + private readonly Queue responses = new Queue(); + + public TestGitObjectsHttpRequestor( + Enlistment enlistment, + CacheServerInfo cacheServer, + RetryConfig retryConfig) + : this(new MockTracer(), enlistment, cacheServer, retryConfig) + { + } + + private TestGitObjectsHttpRequestor( + MockTracer tracer, + Enlistment enlistment, + CacheServerInfo cacheServer, + RetryConfig retryConfig) + : base(tracer, enlistment, cacheServer, retryConfig) + { + this.TestTracer = tracer; + this.RequestUris = new List(); + } + + public MockTracer TestTracer { get; } + public List RequestUris { get; } + + public void EnqueueResponse(HttpStatusCode statusCode, string body = "", bool shouldRetry = false) + { + this.responses.Enqueue(Tuple.Create(statusCode, body, shouldRetry)); + } + + public void EnqueueException(Exception exception) + { + this.responses.Enqueue(exception); + } + + public void EnqueueResponse(Stream stream) + { + this.responses.Enqueue(stream); + } + + protected override GitEndPointResponseData SendProtocolRequest( + long requestId, + Uri requestUri, + HttpMethod httpMethod, + string requestContent, + CancellationToken cancellationToken, + MediaTypeWithQualityHeaderValue acceptType = null) + { + this.RequestUris.Add(requestUri); + object nextResponse = this.responses.Dequeue(); + if (nextResponse is Exception exception) + { + throw exception; + } + + if (nextResponse is Stream stream) + { + return new GitEndPointResponseData( + HttpStatusCode.OK, + "application/json", + stream, + message: null, + onResponseDisposed: null); + } + + Tuple response = (Tuple)nextResponse; + + if (response.Item1 == HttpStatusCode.OK) + { + return new GitEndPointResponseData( + response.Item1, + "application/json", + new MemoryStream(Encoding.UTF8.GetBytes(response.Item2)), + message: null, + onResponseDisposed: null); + } + + return new GitEndPointResponseData( + response.Item1, + new GitObjectsHttpException(response.Item1, "Test failure"), + shouldRetry: response.Item3, + message: null, + onResponseDisposed: null); + } + } + + private class ThrowingReadStream : MemoryStream + { + public override int ReadByte() + { + throw new IOException("Response body read failed"); + } + } + + private class CancelingReadStream : MemoryStream + { + public override int Read(byte[] buffer, int offset, int count) + { + throw new OperationCanceledException("Response body read canceled"); + } + } + } +} diff --git a/GVFS/GVFS.UnitTests/Http/HttpRequestorTests.cs b/GVFS/GVFS.UnitTests/Http/HttpRequestorTests.cs index 33692dd03b..e81b81170d 100644 --- a/GVFS/GVFS.UnitTests/Http/HttpRequestorTests.cs +++ b/GVFS/GVFS.UnitTests/Http/HttpRequestorTests.cs @@ -1,7 +1,8 @@ -using System.Net; using GVFS.Common.Http; using GVFS.Tests.Should; using NUnit.Framework; +using System; +using System.Net; namespace GVFS.UnitTests.Http { @@ -75,5 +76,13 @@ public void CommonNonAuthStatusesDoNotRejectCredentials() HttpRequestor.ShouldRejectCredentials(HttpStatusCode.RequestTimeout, responseBody: null) .ShouldEqual(false, "A 408 must NOT reject credentials"); } + + [TestCase] + public void AuthorityForTelemetryExcludesCredentialsAndRequestPath() + { + Uri uri = new Uri("https://alice:secret@cache.example.com:8443/private/path?token=sensitive#fragment"); + + HttpRequestor.GetAuthorityForTelemetry(uri).ShouldEqual("cache.example.com:8443"); + } } } diff --git a/GVFS/GVFS.UnitTests/Mock/Common/MockTracer.cs b/GVFS/GVFS.UnitTests/Mock/Common/MockTracer.cs index d933584e94..47ad66e295 100644 --- a/GVFS/GVFS.UnitTests/Mock/Common/MockTracer.cs +++ b/GVFS/GVFS.UnitTests/Mock/Common/MockTracer.cs @@ -17,6 +17,8 @@ public MockTracer() this.RelatedWarningEvents = new List(); this.RelatedErrorEvents = new List(); this.RelatedEventNames = new List(); + this.RelatedEventKeywords = new List(); + this.RelatedEventMetadata = new List(); } public MockTracer StartActivityTracer { get; private set; } @@ -29,6 +31,8 @@ public MockTracer() // Names of events reported via RelatedEvent (which, unlike RelatedInfo/Warning/Error, // do not otherwise get recorded). Lets tests assert a specific diagnostic event fired. public List RelatedEventNames { get; } + public List RelatedEventKeywords { get; } + public List RelatedEventMetadata { get; } public void WaitForRelatedEvent() { @@ -38,6 +42,8 @@ public void WaitForRelatedEvent() public void RelatedEvent(EventLevel error, string eventName, EventMetadata metadata) { this.RelatedEventNames.Add(eventName); + this.RelatedEventKeywords.Add(Keywords.None); + this.RelatedEventMetadata.Add(metadata); if (eventName == this.WaitRelatedEventName) { this.waitEvent.Set(); @@ -47,6 +53,8 @@ public void RelatedEvent(EventLevel error, string eventName, EventMetadata metad public void RelatedEvent(EventLevel error, string eventName, EventMetadata metadata, Keywords keyword) { this.RelatedEventNames.Add(eventName); + this.RelatedEventKeywords.Add(keyword); + this.RelatedEventMetadata.Add(metadata); if (eventName == this.WaitRelatedEventName) { this.waitEvent.Set(); diff --git a/GVFS/GVFS/CommandLine/CloneVerb.cs b/GVFS/GVFS/CommandLine/CloneVerb.cs index 370f40b30a..2e9bb52762 100644 --- a/GVFS/GVFS/CommandLine/CloneVerb.cs +++ b/GVFS/GVFS/CommandLine/CloneVerb.cs @@ -65,15 +65,19 @@ public static System.CommandLine.Command CreateCommand() cmd.Add(cacheServerOption); System.CommandLine.Option prefetchCacheServerOption = new System.CommandLine.Option("--prefetch-cache-server-url") { Description = "The cache server URL for the prefetch endpoint" }; + AddEndpointCacheServerUrlValidator(prefetchCacheServerOption); cmd.Add(prefetchCacheServerOption); System.CommandLine.Option getCacheServerOption = new System.CommandLine.Option("--get-cache-server-url") { Description = "The cache server URL for the objects GET endpoint" }; + AddEndpointCacheServerUrlValidator(getCacheServerOption); cmd.Add(getCacheServerOption); System.CommandLine.Option postCacheServerOption = new System.CommandLine.Option("--post-cache-server-url") { Description = "The cache server URL for the objects POST endpoint" }; + AddEndpointCacheServerUrlValidator(postCacheServerOption); cmd.Add(postCacheServerOption); System.CommandLine.Option sizesCacheServerOption = new System.CommandLine.Option("--sizes-cache-server-url") { Description = "The cache server URL for the sizes endpoint" }; + AddEndpointCacheServerUrlValidator(sizesCacheServerOption); cmd.Add(sizesCacheServerOption); System.CommandLine.Option branchOption = new System.CommandLine.Option("--branch", new[] { "-b" }) { Description = "Branch to checkout after clone" }; @@ -131,6 +135,19 @@ public static System.CommandLine.Command CreateCommand() return cmd; } + private static void AddEndpointCacheServerUrlValidator(System.CommandLine.Option option) + { + option.Validators.Add( + result => + { + string url = result.GetValueOrDefault(); + if (url != null && !CacheServerInfo.IsValidUrl(url)) + { + result.AddError($"Option '{option.Name}' requires an absolute URL."); + } + }); + } + protected override string VerbName { get { return CloneVerbName; } @@ -172,6 +189,10 @@ public override void Execute() this.BlockEmptyCacheServerUrl(this.GetCacheServerUrl); this.BlockEmptyCacheServerUrl(this.PostCacheServerUrl); this.BlockEmptyCacheServerUrl(this.SizesCacheServerUrl); + this.BlockInvalidEndpointCacheServerUrl("--prefetch-cache-server-url", this.PrefetchCacheServerUrl); + this.BlockInvalidEndpointCacheServerUrl("--get-cache-server-url", this.GetCacheServerUrl); + this.BlockInvalidEndpointCacheServerUrl("--post-cache-server-url", this.PostCacheServerUrl); + this.BlockInvalidEndpointCacheServerUrl("--sizes-cache-server-url", this.SizesCacheServerUrl); try { @@ -629,6 +650,14 @@ private bool TryDetermineLocalCacheAndInitializePaths( return true; } + private void BlockInvalidEndpointCacheServerUrl(string optionName, string url) + { + if (url != null && !CacheServerInfo.IsValidUrl(url)) + { + this.ReportErrorAndExit($"Option '{optionName}' requires an absolute URL."); + } + } + private Result CreateClone( ITracer tracer, GVFSEnlistment enlistment, From 63f8dc02f1ebd7f4401fee5b0ba6a36dc9a03ec1 Mon Sep 17 00:00:00 2001 From: Derrick Stolee Date: Wed, 2 Sep 2026 11:12:07 -0400 Subject: [PATCH 09/11] docs: Explain endpoint-specific cache routing Context: Administrators need to understand how dedicated GVFS endpoint caches interact with the existing global cache and with gvfs cache-server commands. Justification: Documenting precedence and fallback behavior alongside the configuration keys makes staged cache migrations predictable and preserves the distinction between global and endpoint-specific settings. Implementation: Describe the clone options, local Git config keys, endpoint-to-global fallback order, the sizes-to-origin fallback, and troubleshooting guidance for inspecting or changing endpoint overrides. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- docs/getting-started.md | 14 ++++++++++++++ docs/troubleshooting.md | 20 ++++++++++++++++++++ 2 files changed, 34 insertions(+) diff --git a/docs/getting-started.md b/docs/getting-started.md index aee8b93844..75899bb0ae 100644 --- a/docs/getting-started.md +++ b/docs/getting-started.md @@ -38,6 +38,20 @@ These options allow a user to customize their initial enlistment. cache servers via the `/gvfs/config` endpoint, then the `clone` command will select a nearby cache server from that list. +* `--prefetch-cache-server-url=`, + `--get-cache-server-url=`, `--post-cache-server-url=`, and + `--sizes-cache-server-url=`: Prefer the specified absolute cache server + URL for `/gvfs/prefetch`, loose-object GET requests, batched-object POST + requests, or `/gvfs/sizes`, respectively. If a dedicated server fails, VFS + for Git retries the request against the server selected by + `--cache-server-url`. Sizes requests retain their additional fallback to the + origin server when the global cache does not support `/gvfs/sizes`. + + These values are saved in the local Git configuration as + `gvfs.prefetch.cache-server`, `gvfs.get.cache-server`, + `gvfs.post.cache-server`, and `gvfs.sizes.cache-server`. They continue to + apply to later mount, hydration, and prefetch operations. + * `--branch=`: Specify the branch to checkout after clone. * `--local-cache-path=`: Use this option to override the path for the diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md index 44fa175482..8b91ed346a 100644 --- a/docs/troubleshooting.md +++ b/docs/troubleshooting.md @@ -237,6 +237,26 @@ Run `gvfs cache-server --list` to see the available cache server URLs. Run `gvfs cache-server --set=` to set your cache server to ``. +Individual GVFS protocol endpoints can prefer dedicated cache servers through +these local Git configuration values: + +| Configuration | Requests | +| --- | --- | +| `gvfs.prefetch.cache-server` | `/gvfs/prefetch` | +| `gvfs.get.cache-server` | Loose-object GET requests under `/gvfs/objects` | +| `gvfs.post.cache-server` | Batched-object POST requests to `/gvfs/objects` | +| `gvfs.sizes.cache-server` | `/gvfs/sizes` | + +Each value must be an absolute URL. A dedicated endpoint server is attempted +before `gvfs.cache-server`; if that request fails, VFS for Git falls back to +the global cache server. Sizes requests also fall back from the global cache +to the origin server when `/gvfs/sizes` is not supported. + +`gvfs cache-server --get` and `--set` operate on the global +`gvfs.cache-server` value. Setting the global server does not clear the four +endpoint-specific values. Inspect or change those values with `git config +--local []`. + ### System-wide Config The `gvfs config` command allows customizing some behavior. From f2480964a103990f9bbcec5a8661248a3b21dc9c Mon Sep 17 00:00:00 2001 From: Derrick Stolee Date: Wed, 2 Sep 2026 11:47:51 -0400 Subject: [PATCH 10/11] test: Cover prefetch failure telemetry redaction Context: The prefetch entry point now reports only URI authority when a request fails, but requestor-level tests did not execute the warning and unsupported-command telemetry paths that consume the terminal request URI. Justification: Exercise the production composition directly so future changes cannot reintroduce credentials, paths, queries, or fragments into prefetch failure diagnostics. These focused cases also raise changed-line coverage above the repository threshold without relying on incidental functional-test execution. Implementation: Add a deterministic prefetch requestor that returns terminal HTTP failures. Verify both general failure warnings and not-supported events emit only the host and port from a credential-bearing request URI. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- GVFS/GVFS.UnitTests/Git/GitObjectsTests.cs | 96 ++++++++++++++++++++++ 1 file changed, 96 insertions(+) diff --git a/GVFS/GVFS.UnitTests/Git/GitObjectsTests.cs b/GVFS/GVFS.UnitTests/Git/GitObjectsTests.cs index 88be433806..78236a37bc 100644 --- a/GVFS/GVFS.UnitTests/Git/GitObjectsTests.cs +++ b/GVFS/GVFS.UnitTests/Git/GitObjectsTests.cs @@ -1,13 +1,19 @@ using GVFS.Common; using GVFS.Common.Git; +using GVFS.Common.Http; using GVFS.Common.Tracing; using GVFS.Tests.Should; using GVFS.UnitTests.Mock.Common; using GVFS.UnitTests.Mock.FileSystem; using NUnit.Framework; +using System; using System.Collections.Generic; using System.IO; +using System.Net; +using System.Net.Http; +using System.Net.Http.Headers; using System.Security; +using System.Threading; namespace GVFS.UnitTests.Git { @@ -124,6 +130,61 @@ public void WriteLooseObject_Success() moved.ShouldBeTrue("File was not moved"); } + [TestCase] + public void PrefetchFailureTelemetryReportsOnlyRequestAuthority() + { + const string RequestUrl = "https://user:secret@cache.example:8443/gvfs/prefetch?token=sensitive"; + MockTracer tracer = new MockTracer(); + MockGVFSEnlistment enlistment = new MockGVFSEnlistment(); + TestPrefetchRequestor requestor = new TestPrefetchRequestor( + tracer, + enlistment, + HttpStatusCode.ServiceUnavailable, + new Uri(RequestUrl)); + GitObjects gitObjects = new GVFSGitObjects( + new GVFSContext(tracer, new MockFileSystemWithCallbacks(), null, enlistment), + requestor); + + gitObjects.TryDownloadPrefetchPacks( + gitProcess: null, + latestTimestamp: 0, + trustPackIndexes: false, + out List _) + .ShouldEqual(false); + + tracer.StartActivityTracer.RelatedWarningEvents.Count.ShouldEqual(1); + tracer.StartActivityTracer.RelatedWarningEvents[0].ShouldContain("\"PrefetchEndpointUrl\":\"cache.example:8443\""); + tracer.StartActivityTracer.RelatedWarningEvents[0].IndexOf("user", StringComparison.Ordinal).ShouldEqual(-1); + tracer.StartActivityTracer.RelatedWarningEvents[0].IndexOf("secret", StringComparison.Ordinal).ShouldEqual(-1); + tracer.StartActivityTracer.RelatedWarningEvents[0].IndexOf("sensitive", StringComparison.Ordinal).ShouldEqual(-1); + } + + [TestCase] + public void UnsupportedPrefetchTelemetryReportsOnlyRequestAuthority() + { + const string RequestUrl = "https://user:secret@cache.example:8443/gvfs/prefetch?token=sensitive"; + MockTracer tracer = new MockTracer(); + MockGVFSEnlistment enlistment = new MockGVFSEnlistment(); + TestPrefetchRequestor requestor = new TestPrefetchRequestor( + tracer, + enlistment, + HttpStatusCode.NotFound, + new Uri(RequestUrl)); + GitObjects gitObjects = new GVFSGitObjects( + new GVFSContext(tracer, new MockFileSystemWithCallbacks(), null, enlistment), + requestor); + + gitObjects.TryDownloadPrefetchPacks( + gitProcess: null, + latestTimestamp: 0, + trustPackIndexes: false, + out List _) + .ShouldEqual(false); + + EventMetadata metadata = tracer.StartActivityTracer.RelatedEventMetadata[0]; + metadata["PrefetchEndpointUrl"].ShouldEqual("cache.example:8443"); + } + private Stream OnOpenFileStream(string path, FileMode mode, FileAccess access) { this.openedPaths.Add(path); @@ -144,5 +205,40 @@ private bool OnFileExists(string path) { return this.pathsToData.TryGetValue(path, out _); } + + private class TestPrefetchRequestor : GitObjectsHttpRequestor + { + private readonly HttpStatusCode statusCode; + private readonly Uri requestUri; + + public TestPrefetchRequestor( + ITracer tracer, + Enlistment enlistment, + HttpStatusCode statusCode, + Uri requestUri) + : base(tracer, enlistment, new CacheServerInfo("https://cache.example/server", "cache"), new RetryConfig(0)) + { + this.statusCode = statusCode; + this.requestUri = requestUri; + } + + public override RetryWrapper.InvocationResult TrySendProtocolRequest( + long requestId, + Func.CallbackResult> onSuccess, + Action.ErrorEventArgs> onFailure, + HttpMethod method, + Func endPointGenerator, + Func requestBodyGenerator, + CancellationToken cancellationToken, + MediaTypeWithQualityHeaderValue acceptType = null, + bool retryOnFailure = true, + Func fallbackEndPointGenerator = null) + { + return new RetryWrapper.InvocationResult( + tryCount: 1, + new GitObjectsHttpException(this.statusCode, "Test failure"), + new GitObjectTaskResult(this.statusCode, this.requestUri)); + } + } } } From 35272aaf16b06e22fa36bab49fdb5d0b94e512e6 Mon Sep 17 00:00:00 2001 From: Tyrie Vella Date: Mon, 14 Sep 2026 10:07:58 -0700 Subject: [PATCH 11/11] Disable Git fsmonitor in virtual repositories Assisted-by: Auto Signed-off-by: Tyrie Vella --- GVFS/GVFS.Common/Git/RequiredGitConfig.cs | 3 +++ GVFS/GVFS.FunctionalTests/Tools/ControlGitRepo.cs | 1 + 2 files changed, 4 insertions(+) diff --git a/GVFS/GVFS.Common/Git/RequiredGitConfig.cs b/GVFS/GVFS.Common/Git/RequiredGitConfig.cs index a9159e6638..8f40e2dd37 100644 --- a/GVFS/GVFS.Common/Git/RequiredGitConfig.cs +++ b/GVFS/GVFS.Common/Git/RequiredGitConfig.cs @@ -183,6 +183,9 @@ public static Dictionary GetRequiredSettings(GVFSEnlistment enli // Disable the builtin FS Monitor in case it was enabled globally. { "core.useBuiltinFSMonitor", "false" }, + + // Disable the FS Monitor in case it was enabled globally. + { "core.fsmonitor", "false" }, }; } } diff --git a/GVFS/GVFS.FunctionalTests/Tools/ControlGitRepo.cs b/GVFS/GVFS.FunctionalTests/Tools/ControlGitRepo.cs index bd884d9797..11e1d972d6 100644 --- a/GVFS/GVFS.FunctionalTests/Tools/ControlGitRepo.cs +++ b/GVFS/GVFS.FunctionalTests/Tools/ControlGitRepo.cs @@ -77,6 +77,7 @@ private void InitializeCore() GitProcess.Invoke(this.RootPath, "config core.abbrev 40"); GitProcess.Invoke(this.RootPath, "config checkout.workers 0"); GitProcess.Invoke(this.RootPath, "config core.useBuiltinFSMonitor false"); + GitProcess.Invoke(this.RootPath, "config core.fsmonitor false"); GitProcess.Invoke(this.RootPath, "config pack.useSparse true"); GitProcess.Invoke(this.RootPath, "config reset.quiet true"); GitProcess.Invoke(this.RootPath, "config status.aheadbehind false");