From 9ec7abfeb73b5ecbcbd0158829f617339f03f2c0 Mon Sep 17 00:00:00 2001 From: Tyrie Vella Date: Thu, 9 Jul 2026 12:53:37 -0700 Subject: [PATCH 1/6] Auto-recover corrupt packfiles in packfile maintenance When a packfile in the shared object cache is corrupt or truncated (e.g. from a past disk-full event), 'git multi-pack-index write/verify' fails with "could not load pack N". The existing self-heal only deletes and rewrites the multi-pack-index (MIDX), which does not fix the underlying pack: the rewrite re-scans the same bad pack and keeps failing. The corruption then recurs indefinitely. PackfileMaintenanceStep now routes write/verify failures through a recovery path that, when git reports a pack-load failure: - Detection (always runs, even with recovery disabled): verifies each pack in the object cache with 'git verify-pack' and reports every unreadable pack via telemetry (Operation=FoundCorruptPack). The "could not load pack N" ordinal is an internal MIDX position, not a filename, so per-pack verification is how we find the actual bad file. - Removal (gated, see kill switch below): deletes each corrupt pack's files (.pack/.idx/.keep/.rev; Operation=DeletedCorruptPack), then deletes and regenerates the MIDX from the packs that remain (fast path, no full repack). Missing objects are re-fetched on demand. - Corrupt prefetch pack (special case): prefetch packs are incremental and ordered by timestamp, so a corrupt one invalidates every later prefetch pack too - leaving a hole would let the newest surviving timestamp advance past it so a later prefetch never backfills the gap. Recovery removes the corrupt prefetch pack and every later prefetch pack (Operation=DeletedHealthyPrefetchPack for the healthy ones removed purely due to ordering), then requests a prefetch (via a callback GitMaintenanceScheduler wires to a PrefetchStep, only when using a cache server) to re-download them and rebuild the commit-graph. Kill switch: the destructive pack removal is gated by a new git config, gvfs.enable-packfile-recovery (default true). When false, GVFS still detects and reports corrupt packs (Operation=FoundCorruptPack, then CorruptPackRecoverySkipped) but deletes nothing and does not request a prefetch; the non-destructive MIDX rewrite still runs, so behavior degrades to today's. This gives a field kill switch without a redeploy if the destructive path ever misbehaves. This is stacked on the git-output bounding change: recovery runs additional git commands (verify-pack, MIDX rewrites) against the corrupt repo, so it relies on that change to keep a noisy stderr from OOM-ing the mount mid-recovery. Review follow-ups: - prefetchRestoreNeeded is now set only after a corrupt prefetch pack is actually removed (RemovePackFileSet returns whether the .pack file was deleted), instead of as soon as one is detected. If deletion is blocked, the restore no longer runs while the corrupt pack is still present. - DetectAndRemoveCorruptPacks now remembers, for the lifetime of a single maintenance run, that it already reported corrupt packs with recovery disabled, and skips the redundant per-pack verify-pack rescan on later MIDX failures in that same run. - DetectAndRemoveCorruptPacks now parses the corrupt pack's filename directly out of the write/verify failure's stderr when git includes it (e.g. "packfile pack-1234.pack does not match index" / "wrong index v2 file size in pack-1234.idx"), and verifies only that candidate instead of every pack in the object cache. This only helps when git actually names the file, which it does for the verify-triggered failures this code mostly handles (not for the rarer write-path "could not load pack N", which is genuinely an unresolvable internal ordinal - confirmed by reading git's midx-write.c). Falls back to verifying every pack whenever no candidate can be parsed, or the parsed candidate turns out to be healthy, so detection is never less thorough than before. Tests: - A verify failure reporting a pack-load error removes the corrupt pack and rewrites the MIDX from the remaining good packs (recovery enabled). - With recovery disabled, the same failure still verifies each pack and reports the corrupt one but deletes nothing. - A corrupt prefetch pack removes it and every later prefetch pack, keeps the earlier healthy one, and requests a prefetch. - A verify failure that names the corrupt pack directly verifies only that pack (fast path). - A verify failure that names a pack which turns out to be healthy falls back to verifying every pack (fallback path). Assisted-by: Claude Sonnet 5 Signed-off-by: Tyrie Vella --- GVFS/GVFS.Common/GVFSConstants.cs | 7 + GVFS/GVFS.Common/Git/GitProcess.cs | 11 + .../Maintenance/GitMaintenanceScheduler.cs | 14 +- .../Maintenance/PackfileMaintenanceStep.cs | 470 ++++++++++++++++-- .../PackfileMaintenanceStepTests.cs | 280 +++++++++++ 5 files changed, 750 insertions(+), 32 deletions(-) diff --git a/GVFS/GVFS.Common/GVFSConstants.cs b/GVFS/GVFS.Common/GVFSConstants.cs index 8f135786aa..6463fcd81b 100644 --- a/GVFS/GVFS.Common/GVFSConstants.cs +++ b/GVFS/GVFS.Common/GVFSConstants.cs @@ -44,6 +44,13 @@ public static class GitConfig public const string TrustPackIndexes = GVFSPrefix + "trust-pack-indexes"; public const bool TrustPackIndexesDefault = true; + /* Kill switch for the destructive part of packfile-maintenance corruption recovery: when + * false, GVFS still detects and reports corrupt packs but does not delete them (or later + * prefetch packs) and does not request a restoring prefetch. Detection/telemetry is + * unaffected; the non-destructive multi-pack-index rewrite still runs. */ + public const string EnablePackfileRecovery = GVFSPrefix + "enable-packfile-recovery"; + public const bool EnablePackfileRecoveryDefault = true; + public const string ShowHydrationStatus = GVFSPrefix + "show-hydration-status"; public const bool ShowHydrationStatusDefault = false; diff --git a/GVFS/GVFS.Common/Git/GitProcess.cs b/GVFS/GVFS.Common/Git/GitProcess.cs index cf666bc646..d27d7a7498 100644 --- a/GVFS/GVFS.Common/Git/GitProcess.cs +++ b/GVFS/GVFS.Common/Git/GitProcess.cs @@ -784,6 +784,17 @@ public Result VerifyMultiPackIndex(string objectDir) return this.InvokeGitAgainstDotGitFolder("-c core.multiPackIndex=true multi-pack-index verify --object-dir=\"" + objectDir + "\" --no-progress"); } + /// + /// Verifies the integrity of a single packfile via its .idx. Returns a failure exit code if the + /// pack is truncated or otherwise unreadable. Used by pack maintenance recovery to determine + /// which pack is corrupt - the "could not load pack N" ordinal reported by the multi-pack-index + /// is an internal position, not a filename, so it cannot be mapped to a file directly. + /// + public Result VerifyPack(string packIndexPath) + { + return this.InvokeGitAgainstDotGitFolder("verify-pack \"" + packIndexPath + "\""); + } + public Result RemoteAdd(string remoteName, string url) { return this.InvokeGitAgainstDotGitFolder("remote add " + remoteName + " " + url); diff --git a/GVFS/GVFS.Common/Maintenance/GitMaintenanceScheduler.cs b/GVFS/GVFS.Common/Maintenance/GitMaintenanceScheduler.cs index 760f803291..2759306ff5 100644 --- a/GVFS/GVFS.Common/Maintenance/GitMaintenanceScheduler.cs +++ b/GVFS/GVFS.Common/Maintenance/GitMaintenanceScheduler.cs @@ -54,7 +54,9 @@ private void ScheduleRecurringSteps() return; } - if (this.gitObjects.IsUsingCacheServer()) + bool usingCacheServer = this.gitObjects.IsUsingCacheServer(); + + if (usingCacheServer) { TimeSpan prefetchPeriod = TimeSpan.FromMinutes(15); this.stepTimers.Add(new Timer( @@ -70,8 +72,16 @@ private void ScheduleRecurringSteps() dueTime: this.looseObjectsDueTime, period: this.looseObjectsPeriod)); + // When packfile-maintenance recovery removes a corrupt prefetch pack (and the later prefetch + // packs that depend on it), it needs a prefetch to re-download them and rebuild the + // commit-graph. This is only meaningful when a cache server is in use; otherwise the objects + // are restored on demand. + Action requestPrefetch = usingCacheServer + ? () => this.queue.TryEnqueue(new PrefetchStep(this.context, this.gitObjects)) + : (Action)null; + this.stepTimers.Add(new Timer( - (state) => this.queue.TryEnqueue(new PackfileMaintenanceStep(this.context)), + (state) => this.queue.TryEnqueue(new PackfileMaintenanceStep(this.context, requestPrefetch: requestPrefetch)), state: null, dueTime: this.packfileDueTime, period: this.packfilePeriod)); diff --git a/GVFS/GVFS.Common/Maintenance/PackfileMaintenanceStep.cs b/GVFS/GVFS.Common/Maintenance/PackfileMaintenanceStep.cs index a5fc5b54a6..e51f11a2a8 100644 --- a/GVFS/GVFS.Common/Maintenance/PackfileMaintenanceStep.cs +++ b/GVFS/GVFS.Common/Maintenance/PackfileMaintenanceStep.cs @@ -5,6 +5,7 @@ using System.Collections.Generic; using System.IO; using System.Linq; +using System.Text.RegularExpressions; namespace GVFS.Common.Maintenance { @@ -27,21 +28,30 @@ namespace GVFS.Common.Maintenance public class PackfileMaintenanceStep : GitMaintenanceStep { public const string PackfileLastRunFileName = "pack-maintenance.time"; - public const string DefaultBatchSize = "2g"; + public const string DefaultBatchSize = "2g"; private const string MultiPackIndexLock = "multi-pack-index.lock"; private readonly bool forceRun; private readonly string batchSize; + private readonly Action requestPrefetch; + + // Set once corrupt packs have been detected and reported with recovery disabled. Recovery leaves + // the corrupt packs in place, so 'git multi-pack-index write/verify' keeps failing on them for + // the rest of this maintenance run - once reported, skip re-verifying every pack on each + // subsequent failure in the same run rather than repeating an identical, already-known result. + private bool reportedCorruptPacksWithRecoveryDisabled; public PackfileMaintenanceStep( GVFSContext context, bool requireObjectCacheLock = true, bool forceRun = false, string batchSize = DefaultBatchSize, - GitProcessChecker gitProcessChecker = null) + GitProcessChecker gitProcessChecker = null, + Action requestPrefetch = null) : base(context, requireObjectCacheLock, gitProcessChecker) { this.forceRun = forceRun; this.batchSize = batchSize; + this.requestPrefetch = requestPrefetch; } public override string Area => nameof(PackfileMaintenanceStep); @@ -116,41 +126,54 @@ protected override void PerformMaintenance() return; } - string multiPackIndexLockPath = Path.Combine(this.Context.Enlistment.GitPackRoot, MultiPackIndexLock); - this.Context.FileSystem.TryDeleteFile(multiPackIndexLockPath); - - this.RunGitCommand((process) => process.WriteMultiPackIndex(this.Context.Enlistment.GitObjectsRoot), nameof(GitProcess.WriteMultiPackIndex)); - - // If a LibGit2Repo is active, then it may hold handles to the .idx and .pack files we want - // to delete during the 'git multi-pack-index expire' step. If one starts during the step, - // then it can still block those deletions, but we will clean them up in the next run. By - // running CloseActiveRepos() here, we ensure that we do not run twice with the same - // LibGit2Repo active across two calls. A "new" repo should not hold handles to .idx files - // that do not have corresponding .pack files, so we will clean them up in CleanStaleIdxFiles(). - this.Context.Repository.CloseActiveRepo(); - - GitProcess.Result expireResult = this.RunGitCommand((process) => process.MultiPackIndexExpire(this.Context.Enlistment.GitObjectsRoot), nameof(GitProcess.MultiPackIndexExpire)); - - this.Context.Repository.OpenRepo(); - + string multiPackIndexLockPath = Path.Combine(this.Context.Enlistment.GitPackRoot, MultiPackIndexLock); + this.Context.FileSystem.TryDeleteFile(multiPackIndexLockPath); + + // Read the recovery kill switch while the repo is open. When disabled, we still detect and + // report corrupt packs but do not delete anything. + bool recoveryEnabled = this.IsPackfileRecoveryEnabled(); + + // A corrupt or truncated packfile in the shared object cache (e.g. introduced by a + // disk-full event) makes 'git multi-pack-index write' fail with "could not load pack N". + // The existing self-heal only ran after a later verify failed - but the write is first, + // so recover on the write path too rather than pressing on with a broken cache. + GitProcess.Result writeResult = this.RunGitCommand((process) => process.WriteMultiPackIndex(this.Context.Enlistment.GitObjectsRoot), nameof(GitProcess.WriteMultiPackIndex)); + + if (!this.Stopping && writeResult.ExitCodeIsFailure) + { + this.RepairMultiPackIndex(activity, writeResult, recoveryEnabled); + } + + // If a LibGit2Repo is active, then it may hold handles to the .idx and .pack files we want + // to delete during the 'git multi-pack-index expire' step. If one starts during the step, + // then it can still block those deletions, but we will clean them up in the next run. By + // running CloseActiveRepos() here, we ensure that we do not run twice with the same + // LibGit2Repo active across two calls. A "new" repo should not hold handles to .idx files + // that do not have corresponding .pack files, so we will clean them up in CleanStaleIdxFiles(). + this.Context.Repository.CloseActiveRepo(); + + GitProcess.Result expireResult = this.RunGitCommand((process) => process.MultiPackIndexExpire(this.Context.Enlistment.GitObjectsRoot), nameof(GitProcess.MultiPackIndexExpire)); + + this.Context.Repository.OpenRepo(); + List staleIdxFiles = this.CleanStaleIdxFiles(out int numDeletionBlocked); - this.GetPackFilesInfo(out int expireCount, out long expireSize, out hasKeep); - + this.GetPackFilesInfo(out int expireCount, out long expireSize, out hasKeep); + GitProcess.Result verifyAfterExpire = this.RunGitCommand((process) => process.VerifyMultiPackIndex(this.Context.Enlistment.GitObjectsRoot), nameof(GitProcess.VerifyMultiPackIndex)); - if (!this.Stopping && verifyAfterExpire.ExitCodeIsFailure) - { - this.LogErrorAndRewriteMultiPackIndex(activity); + if (!this.Stopping && verifyAfterExpire.ExitCodeIsFailure) + { + this.RepairMultiPackIndex(activity, verifyAfterExpire, recoveryEnabled); } GitProcess.Result repackResult = this.RunGitCommand((process) => process.MultiPackIndexRepack(this.Context.Enlistment.GitObjectsRoot, this.batchSize), nameof(GitProcess.MultiPackIndexRepack)); - this.GetPackFilesInfo(out int afterCount, out long afterSize, out hasKeep); - - GitProcess.Result verifyAfterRepack = this.RunGitCommand((process) => process.VerifyMultiPackIndex(this.Context.Enlistment.GitObjectsRoot), nameof(GitProcess.VerifyMultiPackIndex)); + this.GetPackFilesInfo(out int afterCount, out long afterSize, out hasKeep); - if (!this.Stopping && verifyAfterRepack.ExitCodeIsFailure) - { - this.LogErrorAndRewriteMultiPackIndex(activity); + GitProcess.Result verifyAfterRepack = this.RunGitCommand((process) => process.VerifyMultiPackIndex(this.Context.Enlistment.GitObjectsRoot), nameof(GitProcess.VerifyMultiPackIndex)); + + if (!this.Stopping && verifyAfterRepack.ExitCodeIsFailure) + { + this.RepairMultiPackIndex(activity, verifyAfterRepack, recoveryEnabled); } EventMetadata metadata = new EventMetadata(); @@ -171,5 +194,392 @@ protected override void PerformMaintenance() this.SaveLastRunTimeToFile(); } } + + /// + /// Reads the gvfs.enable-packfile-recovery kill switch. Virtual so unit tests can + /// override it; the LibGit2 invoker is null in tests, in which case recovery defaults to enabled. + /// + protected virtual bool IsPackfileRecoveryEnabled() + { + LibGit2RepoInvoker repoInvoker = this.Context.Repository.LibGit2RepoInvoker; + if (repoInvoker == null) + { + return GVFSConstants.GitConfig.EnablePackfileRecoveryDefault; + } + + return repoInvoker.GetConfigBoolOrDefault( + GVFSConstants.GitConfig.EnablePackfileRecovery, + GVFSConstants.GitConfig.EnablePackfileRecoveryDefault); + } + + private static bool ResultIndicatesCorruptPack(GitProcess.Result result) { + string errors = result?.Errors; + if (string.IsNullOrEmpty(errors)) + { + return false; + } + + // 'git multi-pack-index write/verify' reports an unreadable underlying packfile with + // messages like "could not load pack N" or "failed to load pack in position N". Both mean + // a packfile - not just the multi-pack-index - is corrupt. + return errors.IndexOf("could not load pack", StringComparison.OrdinalIgnoreCase) >= 0 + || errors.IndexOf("failed to load pack", StringComparison.OrdinalIgnoreCase) >= 0; + } + + /// + /// Returns the prefetch timestamp encoded in a prefetch pack file name + /// (prefetch-<timestamp>-<uniqueId>.pack), or null if the file is not a prefetch pack. + /// + private static long? GetPrefetchTimestamp(string packFileName) + { + if (!packFileName.StartsWith(GVFSConstants.PrefetchPackPrefix, StringComparison.OrdinalIgnoreCase)) + { + return null; + } + + string[] parts = packFileName.Split('-'); + if (parts.Length > 1 && long.TryParse(parts[1], out long timestamp)) + { + return timestamp; + } + + return null; + } + + /// + /// Recovers from a failed multi-pack-index write or verify. When git reports it could not load a + /// pack, a packfile itself is corrupt (e.g. truncated by a past disk-full event) and regenerating + /// the multi-pack-index (MIDX) alone keeps failing because the rewrite re-scans the same bad pack. + /// Detect the corrupt pack(s) - and, when recovery is enabled, remove them - then delete and + /// regenerate the MIDX from the packs that remain (fast path, no full repack). + /// + private void RepairMultiPackIndex(ITracer activity, GitProcess.Result failure, bool recoveryEnabled) + { + bool prefetchRestoreNeeded = false; + + if (!this.Stopping && ResultIndicatesCorruptPack(failure)) + { + this.DetectAndRemoveCorruptPacks(activity, recoveryEnabled, out prefetchRestoreNeeded, failure.Errors); + } + + // Delete the (now stale) multi-pack-index and rebuild it from the packs that remain. This is + // non-destructive and runs regardless of the recovery kill switch. + this.LogErrorAndRewriteMultiPackIndex(activity); + + if (prefetchRestoreNeeded && !this.Stopping) + { + this.RequestPrefetchRestore(activity); + } + } + + /// + /// Verifies each packfile in the object cache with 'git verify-pack' and reports every unreadable + /// pack via telemetry (this detection runs even when recovery is disabled). When + /// is true, also removes each corrupt pack's files + /// (.pack/.idx/.keep/.rev). A corrupt prefetch pack additionally forces removal of every + /// later (higher-timestamp) prefetch pack and sets , + /// because prefetch packs are incremental - leaving a hole would let the newest surviving + /// timestamp advance past it so a later prefetch never backfills the gap. + /// + // public only for unit tests + public void DetectAndRemoveCorruptPacks(ITracer activity, bool recoveryEnabled, out bool prefetchRestoreNeeded, string failureErrors = null) + { + prefetchRestoreNeeded = false; + + if (!recoveryEnabled && this.reportedCorruptPacksWithRecoveryDisabled) + { + // Already verified every pack and reported the corrupt ones earlier in this maintenance + // run. Recovery is disabled, so nothing has changed on disk - skip the redundant rescan. + return; + } + + List packDirContents = this.Context + .FileSystem + .ItemsInDirectory(this.Context.Enlistment.GitPackRoot) + .ToList(); + + // Phase 1 - detection (read-only, always runs). git's own 'multi-pack-index verify' failure + // text usually already names the specific unreadable pack (e.g. "packfile pack-1234.pack does + // not match index"), so try verifying just those named packs first - much faster than + // verify-pack'ing every pack in the object cache. Fall back to the full scan whenever no + // candidate can be parsed, or when every candidate turns out to verify successfully (the + // write/verify failure that got us here must then be explained by some other pack). + HashSet candidateIdxPaths = this.ExtractCandidateCorruptIdxPaths(failureErrors); + List idxItemsToVerify = candidateIdxPaths.Count > 0 + ? packDirContents.Where(info => candidateIdxPaths.Contains(info.FullName)).ToList() + : packDirContents; + + long? minCorruptPrefetchTimestamp; + List corruptNonPrefetchIdxPaths; + HashSet corruptIdxPaths = this.VerifyPacksAndReportCorruption( + activity, + recoveryEnabled, + idxItemsToVerify, + out corruptNonPrefetchIdxPaths, + out minCorruptPrefetchTimestamp); + + if (this.Stopping) + { + return; + } + + if (corruptIdxPaths.Count == 0 && idxItemsToVerify != packDirContents) + { + // The candidate(s) parsed from the failure text turned out to be healthy - fall back to + // verifying every pack so a real corruption elsewhere is not missed. + corruptIdxPaths = this.VerifyPacksAndReportCorruption( + activity, + recoveryEnabled, + packDirContents, + out corruptNonPrefetchIdxPaths, + out minCorruptPrefetchTimestamp); + + if (this.Stopping) + { + return; + } + } + + if (corruptIdxPaths.Count == 0) + { + return; + } + + if (!recoveryEnabled) + { + EventMetadata skippedMetadata = this.CreateEventMetadata(); + skippedMetadata["Operation"] = "CorruptPackRecoverySkipped"; + skippedMetadata["CorruptPackCount"] = corruptIdxPaths.Count; + activity.RelatedWarning( + skippedMetadata, + $"Found {corruptIdxPaths.Count} corrupt packfile(s) but {GVFSConstants.GitConfig.EnablePackfileRecovery} is disabled; leaving packs in place.", + Keywords.Telemetry); + this.reportedCorruptPacksWithRecoveryDisabled = true; + return; + } + + // Phase 2 - deletion (gated). Build the set of prefetch packs to remove: the corrupt one and + // every later (>= timestamp) prefetch pack, whether or not those later packs are themselves + // corrupt, because prefetch packs are incremental. + List laterPrefetchIdxPaths = new List(); + if (minCorruptPrefetchTimestamp.HasValue) + { + foreach (DirectoryItemInfo info in packDirContents) + { + if (!string.Equals(Path.GetExtension(info.Name), ".pack", GVFSPlatform.Instance.Constants.PathComparison)) + { + continue; + } + + long? prefetchTimestamp = GetPrefetchTimestamp(info.Name); + if (prefetchTimestamp.HasValue && prefetchTimestamp.Value >= minCorruptPrefetchTimestamp.Value) + { + laterPrefetchIdxPaths.Add(Path.ChangeExtension(info.FullName, ".idx")); + } + } + } + + // Only request a prefetch restore once a corrupt prefetch pack is actually removed. If + // deletion is blocked (e.g. a handle is still open), the corrupt pack is still present, so + // running the restore now would just re-download around a cache that is still broken. + bool corruptPrefetchPackRemoved = false; + + // Close the LibGit2 repo so the .idx files can be deleted, then remove each pack set. + this.Context.Repository.CloseActiveRepo(); + try + { + foreach (string idxPath in corruptNonPrefetchIdxPaths) + { + if (this.Stopping) + { + return; + } + + this.RemovePackFileSet(activity, idxPath, "DeletedCorruptPack", $"Deleted corrupt packfile {Path.GetFileName(Path.ChangeExtension(idxPath, ".pack"))} during pack maintenance recovery."); + } + + foreach (string idxPath in laterPrefetchIdxPaths) + { + if (this.Stopping) + { + return; + } + + if (corruptIdxPaths.Contains(idxPath)) + { + bool removed = this.RemovePackFileSet(activity, idxPath, "DeletedCorruptPack", $"Deleted corrupt prefetch packfile {Path.GetFileName(Path.ChangeExtension(idxPath, ".pack"))} during pack maintenance recovery."); + corruptPrefetchPackRemoved = corruptPrefetchPackRemoved || removed; + } + else + { + this.RemovePackFileSet(activity, idxPath, "DeletedHealthyPrefetchPack", $"Deleted healthy prefetch packfile {Path.GetFileName(Path.ChangeExtension(idxPath, ".pack"))} because an earlier prefetch pack was corrupt; incremental prefetch packs after the corruption must be removed and re-fetched."); + } + } + } + finally + { + this.Context.Repository.OpenRepo(); + } + + prefetchRestoreNeeded = corruptPrefetchPackRemoved; + } + + /// + /// Matches pack/idx file names (e.g. "pack-<hash>.pack", "prefetch-123-abc.idx") as they + /// appear embedded in git's own error text - see packfile.c's "packfile %s does not match + /// index" / "packfile %s index unavailable" and "wrong index v2 file size in %s" messages. Pack + /// file names only ever contain word characters, hyphens, and dots, so this is precise and won't + /// pick up unrelated substrings. + /// + private static readonly Regex CorruptPackFileNamePattern = new Regex(@"[\w\-]+\.(?:pack|idx)", RegexOptions.Compiled); + + /// + /// Parses candidate corrupt pack file names directly out of a 'multi-pack-index write/verify' + /// failure's stderr, returning their .idx paths under . + /// Git's own error text usually already names the specific unreadable packfile, so this lets the + /// caller skip a full verify-pack scan of every pack in the object cache. Only paths that + /// actually exist on disk are returned, since the parsed text could (rarely) reference a pack + /// from a different object-dir or a message format this pattern doesn't recognize (e.g. the + /// ordinal-only "could not load pack N" from the write path, which names no file at all). + /// + private HashSet ExtractCandidateCorruptIdxPaths(string failureErrors) + { + HashSet idxPaths = new HashSet(GVFSPlatform.Instance.Constants.PathComparer); + if (string.IsNullOrEmpty(failureErrors)) + { + return idxPaths; + } + + string packRoot = this.Context.Enlistment.GitPackRoot; + foreach (Match match in CorruptPackFileNamePattern.Matches(failureErrors)) + { + string idxFileName = Path.GetFileNameWithoutExtension(match.Value) + ".idx"; + string idxPath = Path.Combine(packRoot, idxFileName); + if (this.Context.FileSystem.FileExists(idxPath)) + { + idxPaths.Add(idxPath); + } + } + + return idxPaths; + } + + /// + /// Runs 'git verify-pack' against each .idx in that has a + /// matching .pack on disk, and reports (via telemetry) every one that fails to verify. verify-pack + /// is an external git process, so it is safe to run with the LibGit2 repo open. + /// + private HashSet VerifyPacksAndReportCorruption( + ITracer activity, + bool recoveryEnabled, + List idxItemsToVerify, + out List corruptNonPrefetchIdxPaths, + out long? minCorruptPrefetchTimestamp) + { + minCorruptPrefetchTimestamp = null; + corruptNonPrefetchIdxPaths = new List(); + HashSet corruptIdxPaths = new HashSet(GVFSPlatform.Instance.Constants.PathComparer); + + foreach (DirectoryItemInfo info in idxItemsToVerify) + { + if (this.Stopping) + { + return corruptIdxPaths; + } + + if (!string.Equals(Path.GetExtension(info.Name), ".idx", GVFSPlatform.Instance.Constants.PathComparison)) + { + continue; + } + + string idxPath = info.FullName; + string packPath = Path.ChangeExtension(idxPath, ".pack"); + + // A dangling .idx with no matching .pack is handled by CleanStaleIdxFiles; here we only + // care about packs that exist on disk but cannot be read. + if (!this.Context.FileSystem.FileExists(packPath)) + { + continue; + } + + GitProcess.Result verifyPackResult = this.RunGitCommand((process) => process.VerifyPack(idxPath), nameof(GitProcess.VerifyPack)); + + if (this.Stopping) + { + return corruptIdxPaths; + } + + if (verifyPackResult.ExitCodeIsSuccess) + { + continue; + } + + long? prefetchTimestamp = GetPrefetchTimestamp(info.Name); + bool isPrefetchPack = prefetchTimestamp.HasValue; + corruptIdxPaths.Add(idxPath); + + EventMetadata foundMetadata = this.CreateEventMetadata(); + foundMetadata["Operation"] = "FoundCorruptPack"; + foundMetadata["Pack"] = info.Name; + foundMetadata["IsPrefetchPack"] = isPrefetchPack; + foundMetadata["RecoveryEnabled"] = recoveryEnabled; + activity.RelatedWarning(foundMetadata, $"Found corrupt packfile {info.Name} during pack maintenance.", Keywords.Telemetry); + + if (isPrefetchPack) + { + if (!minCorruptPrefetchTimestamp.HasValue || prefetchTimestamp.Value < minCorruptPrefetchTimestamp.Value) + { + minCorruptPrefetchTimestamp = prefetchTimestamp.Value; + } + } + else + { + corruptNonPrefetchIdxPaths.Add(idxPath); + } + } + + return corruptIdxPaths; + } + + /// + /// True if the packfile itself was deleted. The .pack file is what actually contains the corrupt + /// (or, for a later prefetch pack, stale) data, so its deletion result - not the sidecar + /// .idx/.keep/.rev files - is what determines whether recovery for this pack set succeeded. + /// + private bool RemovePackFileSet(ITracer activity, string idxPath, string operation, string message) + { + string packPath = Path.ChangeExtension(idxPath, ".pack"); + bool packDeleted = this.Context.FileSystem.TryDeleteFile(packPath); + + EventMetadata metadata = this.CreateEventMetadata(); + metadata["Operation"] = operation; + metadata["Pack"] = Path.GetFileName(packPath); + metadata["DeletePackResult"] = packDeleted; + metadata["DeleteIdxResult"] = this.Context.FileSystem.TryDeleteFile(idxPath); + metadata["DeleteKeepResult"] = this.Context.FileSystem.TryDeleteFile(Path.ChangeExtension(idxPath, ".keep")); + metadata["DeleteRevResult"] = this.Context.FileSystem.TryDeleteFile(Path.ChangeExtension(idxPath, ".rev")); + activity.RelatedWarning(metadata, message, Keywords.Telemetry); + + return packDeleted; + } + + private void RequestPrefetchRestore(ITracer activity) + { + if (this.requestPrefetch == null) + { + // No prefetch is available (e.g. not using a cache server). The removed prefetch packs' + // objects will be re-fetched on demand through normal virtualization. + EventMetadata metadata = this.CreateEventMetadata(); + metadata["Operation"] = "PrefetchRestoreUnavailable"; + activity.RelatedWarning( + metadata, + "Removed prefetch pack(s) but no prefetch restore is available. Missing objects will be re-fetched on demand.", + Keywords.Telemetry); + return; + } + + activity.RelatedInfo("Requesting a prefetch to restore removed prefetch packs and rebuild the commit-graph."); + this.requestPrefetch(); + } } } diff --git a/GVFS/GVFS.UnitTests/Maintenance/PackfileMaintenanceStepTests.cs b/GVFS/GVFS.UnitTests/Maintenance/PackfileMaintenanceStepTests.cs index 811b55e15b..7f7aab9076 100644 --- a/GVFS/GVFS.UnitTests/Maintenance/PackfileMaintenanceStepTests.cs +++ b/GVFS/GVFS.UnitTests/Maintenance/PackfileMaintenanceStepTests.cs @@ -11,6 +11,7 @@ using System; using System.Collections.Generic; using System.IO; +using System.Linq; namespace GVFS.UnitTests.Maintenance { @@ -28,6 +29,8 @@ public class PackfileMaintenanceStepTests private string WriteCommand => $"-c core.multiPackIndex=true multi-pack-index write --object-dir=\"{this.context.Enlistment.GitObjectsRoot}\" --no-progress"; private string RepackCommand => $"-c pack.threads=1 -c repack.packKeptObjects=true multi-pack-index repack --object-dir=\"{this.context.Enlistment.GitObjectsRoot}\" --batch-size=2g --no-progress"; + private string VerifyPackCommand(string idxName) => $"verify-pack \"{Path.Combine(this.context.Enlistment.GitPackRoot, idxName)}\""; + [TestCase] public void PackfileMaintenanceIgnoreTimeRestriction() { @@ -142,6 +145,177 @@ public void PackfileMaintenanceRewriteOnBadVerify() commands[6].ShouldEqual(this.WriteCommand); } + [TestCase] + public void PackfileMaintenanceRemovesCorruptPackWhenVerifyReportsPackLoadFailure() + { + this.TestSetup(DateTime.UtcNow); + this.SetupVerifyFailsOnceWithPackLoadError(); + + // Per-pack verification: pack-2 is the corrupt one, the rest are healthy. + this.gitProcess.SetExpectedCommandResult( + "verify-pack ", + () => new GitProcess.Result(string.Empty, string.Empty, GitProcess.Result.SuccessCode), + matchPrefix: true); + this.gitProcess.SetExpectedCommandResult( + this.VerifyPackCommand("pack-2.idx"), + () => new GitProcess.Result(string.Empty, "error: could not load pack\n", GitProcess.Result.GenericFailureCode)); + + PackfileMaintenanceStep step = new TestablePackfileMaintenanceStep(this.context, recoveryEnabled: true); + step.Execute(); + + this.tracer.StartActivityTracer.RelatedErrorEvents.Count.ShouldEqual(0); + + List commands = this.gitProcess.CommandsRun; + commands.Count(c => c.StartsWith("verify-pack ")).ShouldEqual(3); + + string packRoot = this.context.Enlistment.GitPackRoot; + this.context.FileSystem.FileExists(Path.Combine(packRoot, "pack-2.pack")).ShouldBeFalse(); + this.context.FileSystem.FileExists(Path.Combine(packRoot, "pack-2.idx")).ShouldBeFalse(); + this.context.FileSystem.FileExists(Path.Combine(packRoot, "pack-1.pack")).ShouldBeTrue(); + this.context.FileSystem.FileExists(Path.Combine(packRoot, "pack-3.pack")).ShouldBeTrue(); + + this.WarningsContain("FoundCorruptPack").ShouldBeTrue(); + this.WarningsContain("DeletedCorruptPack").ShouldBeTrue(); + } + + [TestCase] + public void PackfileMaintenanceFastPathVerifiesOnlyNamedPackWhenErrorNamesIt() + { + this.TestSetup(DateTime.UtcNow); + this.SetupVerifyFailsOnceWithNamedPackError("pack-2.pack"); + + // Only pack-2 should be verified via the fast path - no other pack's verify-pack result is + // even registered, so the test would fail with an unexpected-command error if the fallback + // full scan ran instead. + this.gitProcess.SetExpectedCommandResult( + this.VerifyPackCommand("pack-2.idx"), + () => new GitProcess.Result(string.Empty, "error: could not load pack\n", GitProcess.Result.GenericFailureCode)); + + PackfileMaintenanceStep step = new TestablePackfileMaintenanceStep(this.context, recoveryEnabled: true); + step.Execute(); + + this.tracer.StartActivityTracer.RelatedErrorEvents.Count.ShouldEqual(0); + + List commands = this.gitProcess.CommandsRun; + commands.Count(c => c.StartsWith("verify-pack ")).ShouldEqual(1); + commands.Where(c => c.StartsWith("verify-pack ")).Single().ShouldEqual(this.VerifyPackCommand("pack-2.idx")); + + string packRoot = this.context.Enlistment.GitPackRoot; + this.context.FileSystem.FileExists(Path.Combine(packRoot, "pack-2.pack")).ShouldBeFalse(); + this.context.FileSystem.FileExists(Path.Combine(packRoot, "pack-1.pack")).ShouldBeTrue(); + this.context.FileSystem.FileExists(Path.Combine(packRoot, "pack-3.pack")).ShouldBeTrue(); + + this.WarningsContain("FoundCorruptPack").ShouldBeTrue(); + this.WarningsContain("DeletedCorruptPack").ShouldBeTrue(); + } + + [TestCase] + public void PackfileMaintenanceFallsBackToFullScanWhenNamedPackIsHealthy() + { + this.TestSetup(DateTime.UtcNow); + + // The verify failure text names pack-1, but pack-1 turns out to verify successfully; the + // real corrupt pack (pack-2) is only found once the code falls back to the full scan. + this.SetupVerifyFailsOnceWithNamedPackError("pack-1.pack"); + + this.gitProcess.SetExpectedCommandResult( + "verify-pack ", + () => new GitProcess.Result(string.Empty, string.Empty, GitProcess.Result.SuccessCode), + matchPrefix: true); + this.gitProcess.SetExpectedCommandResult( + this.VerifyPackCommand("pack-2.idx"), + () => new GitProcess.Result(string.Empty, "error: could not load pack\n", GitProcess.Result.GenericFailureCode)); + + PackfileMaintenanceStep step = new TestablePackfileMaintenanceStep(this.context, recoveryEnabled: true); + step.Execute(); + + this.tracer.StartActivityTracer.RelatedErrorEvents.Count.ShouldEqual(0); + + List commands = this.gitProcess.CommandsRun; + + // 1 fast-path verify-pack (pack-1, healthy) + 3 full-scan verify-pack (pack-1/2/3). + commands.Count(c => c.StartsWith("verify-pack ")).ShouldEqual(4); + + string packRoot = this.context.Enlistment.GitPackRoot; + this.context.FileSystem.FileExists(Path.Combine(packRoot, "pack-2.pack")).ShouldBeFalse(); + this.context.FileSystem.FileExists(Path.Combine(packRoot, "pack-1.pack")).ShouldBeTrue(); + this.context.FileSystem.FileExists(Path.Combine(packRoot, "pack-3.pack")).ShouldBeTrue(); + + this.WarningsContain("FoundCorruptPack").ShouldBeTrue(); + this.WarningsContain("DeletedCorruptPack").ShouldBeTrue(); + } + + [TestCase] + public void PackfileMaintenanceDetectsButDoesNotDeleteWhenRecoveryDisabled() + { + this.TestSetup(DateTime.UtcNow); + this.SetupVerifyFailsOnceWithPackLoadError(); + + this.gitProcess.SetExpectedCommandResult( + "verify-pack ", + () => new GitProcess.Result(string.Empty, string.Empty, GitProcess.Result.SuccessCode), + matchPrefix: true); + this.gitProcess.SetExpectedCommandResult( + this.VerifyPackCommand("pack-2.idx"), + () => new GitProcess.Result(string.Empty, "error: could not load pack\n", GitProcess.Result.GenericFailureCode)); + + PackfileMaintenanceStep step = new TestablePackfileMaintenanceStep(this.context, recoveryEnabled: false); + step.Execute(); + + this.tracer.StartActivityTracer.RelatedErrorEvents.Count.ShouldEqual(0); + + List commands = this.gitProcess.CommandsRun; + + // Detection still runs (verify-pack on each pack), but nothing is deleted. + commands.Count(c => c.StartsWith("verify-pack ")).ShouldEqual(3); + + string packRoot = this.context.Enlistment.GitPackRoot; + this.context.FileSystem.FileExists(Path.Combine(packRoot, "pack-2.pack")).ShouldBeTrue(); + this.context.FileSystem.FileExists(Path.Combine(packRoot, "pack-2.idx")).ShouldBeTrue(); + + this.WarningsContain("FoundCorruptPack").ShouldBeTrue(); + this.WarningsContain("CorruptPackRecoverySkipped").ShouldBeTrue(); + this.WarningsContain("DeletedCorruptPack").ShouldBeFalse(); + } + + [TestCase] + public void PackfileMaintenanceRemovesLaterPrefetchPacksAndRequestsPrefetch() + { + this.PrefetchTestSetup(DateTime.UtcNow); + this.SetupVerifyFailsOnceWithPackLoadError(); + + this.gitProcess.SetExpectedCommandResult( + "verify-pack ", + () => new GitProcess.Result(string.Empty, string.Empty, GitProcess.Result.SuccessCode), + matchPrefix: true); + this.gitProcess.SetExpectedCommandResult( + this.VerifyPackCommand("prefetch-2-bbb.idx"), + () => new GitProcess.Result(string.Empty, "error: could not load pack\n", GitProcess.Result.GenericFailureCode)); + + bool prefetchRequested = false; + PackfileMaintenanceStep step = new TestablePackfileMaintenanceStep( + this.context, + recoveryEnabled: true, + requestPrefetch: () => prefetchRequested = true); + step.Execute(); + + this.tracer.StartActivityTracer.RelatedErrorEvents.Count.ShouldEqual(0); + + string packRoot = this.context.Enlistment.GitPackRoot; + + // The corrupt prefetch pack and every later prefetch pack are removed; the earlier healthy + // prefetch pack is kept. + this.context.FileSystem.FileExists(Path.Combine(packRoot, "prefetch-2-bbb.pack")).ShouldBeFalse(); + this.context.FileSystem.FileExists(Path.Combine(packRoot, "prefetch-3-ccc.pack")).ShouldBeFalse(); + this.context.FileSystem.FileExists(Path.Combine(packRoot, "prefetch-3-ccc.keep")).ShouldBeFalse(); + this.context.FileSystem.FileExists(Path.Combine(packRoot, "prefetch-1-aaa.pack")).ShouldBeTrue(); + this.context.FileSystem.FileExists(Path.Combine(packRoot, "prefetch-1-aaa.idx")).ShouldBeTrue(); + + this.WarningsContain("DeletedCorruptPack").ShouldBeTrue(); + this.WarningsContain("DeletedHealthyPrefetchPack").ShouldBeTrue(); + prefetchRequested.ShouldBeTrue(); + } + [TestCase] public void CountPackFiles() { @@ -240,5 +414,111 @@ private void TestSetup(DateTime lastRun, bool failOnVerify = false) this.RepackCommand, () => new GitProcess.Result(string.Empty, string.Empty, GitProcess.Result.SuccessCode)); } + + private void PrefetchTestSetup(DateTime lastRun) + { + string lastRunTime = EpochConverter.ToUnixEpochSeconds(lastRun).ToString(); + + this.gitProcess = new MockGitProcess(); + GVFSEnlistment enlistment = new MockGVFSEnlistment(this.gitProcess); + + MockFile timeFile = new MockFile(Path.Combine(enlistment.GitObjectsRoot, "info", PackfileMaintenanceStep.PackfileLastRunFileName), lastRunTime); + MockDirectory info = new MockDirectory( + Path.Combine(enlistment.GitObjectsRoot, "info"), + null, + new List() { timeFile }); + + // Three prefetch packs in ascending timestamp order, newest .keep'd (as GVFS does). + MockDirectory pack = new MockDirectory( + enlistment.GitPackRoot, + null, + new List() + { + new MockFile(Path.Combine(enlistment.GitPackRoot, "prefetch-1-aaa.pack"), "one"), + new MockFile(Path.Combine(enlistment.GitPackRoot, "prefetch-1-aaa.idx"), "1"), + new MockFile(Path.Combine(enlistment.GitPackRoot, "prefetch-2-bbb.pack"), "two"), + new MockFile(Path.Combine(enlistment.GitPackRoot, "prefetch-2-bbb.idx"), "2"), + new MockFile(Path.Combine(enlistment.GitPackRoot, "prefetch-3-ccc.pack"), "three"), + new MockFile(Path.Combine(enlistment.GitPackRoot, "prefetch-3-ccc.idx"), "3"), + new MockFile(Path.Combine(enlistment.GitPackRoot, "prefetch-3-ccc.keep"), string.Empty), + }); + + MockDirectory gitObjectsRoot = new MockDirectory(enlistment.GitObjectsRoot, new List() { info, pack }, null); + List directories = new List() { gitObjectsRoot }; + PhysicalFileSystem fileSystem = new MockFileSystem(new MockDirectory(enlistment.PrimaryEnlistmentRoot, directories, null)); + + this.tracer = new MockTracer(); + MockGitRepo repository = new MockGitRepo(this.tracer, enlistment, fileSystem); + this.context = new GVFSContext(this.tracer, fileSystem, repository, enlistment); + + this.gitProcess.SetExpectedCommandResult( + this.WriteCommand, + () => new GitProcess.Result(string.Empty, string.Empty, GitProcess.Result.SuccessCode)); + this.gitProcess.SetExpectedCommandResult( + this.ExpireCommand, + () => new GitProcess.Result(string.Empty, string.Empty, GitProcess.Result.SuccessCode)); + this.gitProcess.SetExpectedCommandResult( + this.RepackCommand, + () => new GitProcess.Result(string.Empty, string.Empty, GitProcess.Result.SuccessCode)); + } + + /// + /// Makes the multi-pack-index verify fail the first time with a "could not load pack" error + /// (the corrupt-pack signature) and succeed afterwards. + /// + private void SetupVerifyFailsOnceWithPackLoadError() + { + int verifyCount = 0; + this.gitProcess.SetExpectedCommandResult( + this.VerifyCommand, + () => + { + verifyCount++; + return verifyCount == 1 + ? new GitProcess.Result(string.Empty, "failed to load pack in position 0\n", GitProcess.Result.GenericFailureCode) + : new GitProcess.Result(string.Empty, string.Empty, GitProcess.Result.SuccessCode); + }); + } + + /// + /// Makes the multi-pack-index verify fail the first time with an error that names + /// directly (as real git verify failures do - e.g. "failed to + /// load pack entry for oid[0] = ..." followed by "packfile pack-2.pack does not match index"), + /// and succeed afterwards. + /// + private void SetupVerifyFailsOnceWithNamedPackError(string packFileName) + { + int verifyCount = 0; + this.gitProcess.SetExpectedCommandResult( + this.VerifyCommand, + () => + { + verifyCount++; + return verifyCount == 1 + ? new GitProcess.Result(string.Empty, $"failed to load pack entry for oid[0] = abc\nerror: packfile {packFileName} does not match index\n", GitProcess.Result.GenericFailureCode) + : new GitProcess.Result(string.Empty, string.Empty, GitProcess.Result.SuccessCode); + }); + } + + private bool WarningsContain(string operation) + { + return this.tracer.StartActivityTracer.RelatedWarningEvents.Any(e => e.Contains(operation)); + } + + private class TestablePackfileMaintenanceStep : PackfileMaintenanceStep + { + private readonly bool recoveryEnabled; + + public TestablePackfileMaintenanceStep(GVFSContext context, bool recoveryEnabled, Action requestPrefetch = null) + : base(context, requireObjectCacheLock: false, forceRun: true, requestPrefetch: requestPrefetch) + { + this.recoveryEnabled = recoveryEnabled; + } + + protected override bool IsPackfileRecoveryEnabled() + { + return this.recoveryEnabled; + } + } } } From 9b0787adee48d21e54d49cf61492d2c460d8d9cb Mon Sep 17 00:00:00 2001 From: Tyrie Vella Date: Thu, 13 Aug 2026 10:49:23 -0700 Subject: [PATCH 2/6] HttpRequestor: do not reject credentials on HTTP 400 GVFS erased a valid credential when an object-download response was HTTP 400, which triggered a storm of Git Credential Manager popups. A 400 is a request or formatting problem, not an authentication failure. An expired or invalid credential always returns 401 (Unauthorized) or 302 (the Azure DevOps sign-in redirect), never 400. Treating 400 as an auth failure erased good credentials and produced the misleading "Your PAT may be expired" message. Remove BadRequest (400) from the credential-rejection branch in SendRequest. Only 401 and 302 now reject credentials; 400 flows through the generic, non-auth error path (unchanged retry / circuit-breaker behavior, and 400 remains non-retryable). Extract the decision into ShouldRejectCredentials so it is unit tested: 400 does not reject credentials, while 401 and 302 still do. Assisted-by: Claude Opus 4.8 Signed-off-by: Tyrie Vella --- GVFS/GVFS.Common/Http/HttpRequestor.cs | 20 +++++++- .../GVFS.UnitTests/Http/HttpRequestorTests.cs | 46 +++++++++++++++++++ 2 files changed, 65 insertions(+), 1 deletion(-) create mode 100644 GVFS/GVFS.UnitTests/Http/HttpRequestorTests.cs diff --git a/GVFS/GVFS.Common/Http/HttpRequestor.cs b/GVFS/GVFS.Common/Http/HttpRequestor.cs index 0f9767dde1..869b56a082 100644 --- a/GVFS/GVFS.Common/Http/HttpRequestor.cs +++ b/GVFS/GVFS.Common/Http/HttpRequestor.cs @@ -232,7 +232,7 @@ protected GitEndPointResponseData SendRequest( shouldRetry = false; errorMessage = "Anonymous request was rejected with a 401"; } - else if (response.StatusCode == HttpStatusCode.Unauthorized || response.StatusCode == HttpStatusCode.BadRequest || response.StatusCode == HttpStatusCode.Redirect) + else if (ShouldRejectCredentials(response.StatusCode)) { this.authentication.RejectCredentials(this.Tracer, authString); if (!this.authentication.IsBackingOff) @@ -326,6 +326,24 @@ private static bool ShouldRetry(HttpStatusCode statusCode) return false; } + /// + /// Determines whether an HTTP status code indicates an authentication failure + /// that warrants rejecting (erasing) the stored credential. + /// + /// + /// Only 401 (Unauthorized) and 302 (Redirect to the Azure DevOps sign-in page) + /// are genuine authentication failures. A 400 (Bad Request) is a request/formatting + /// problem (e.g. a malformed object URL), NOT an expired credential - an expired or + /// invalid credential always returns 401 or 302. Rejecting credentials on 400 erased + /// valid credentials and caused a storm of credential-manager popups, so 400 must NOT + /// reject credentials. + /// + internal static bool ShouldRejectCredentials(HttpStatusCode statusCode) + { + return statusCode == HttpStatusCode.Unauthorized || + statusCode == HttpStatusCode.Redirect; + } + private static string GetSingleHeaderOrEmpty(HttpHeaders headers, string headerName) { IEnumerable values; diff --git a/GVFS/GVFS.UnitTests/Http/HttpRequestorTests.cs b/GVFS/GVFS.UnitTests/Http/HttpRequestorTests.cs new file mode 100644 index 0000000000..b4ddeb4c86 --- /dev/null +++ b/GVFS/GVFS.UnitTests/Http/HttpRequestorTests.cs @@ -0,0 +1,46 @@ +using System.Net; +using GVFS.Common.Http; +using GVFS.Tests.Should; +using NUnit.Framework; + +namespace GVFS.UnitTests.Http +{ + [TestFixture] + public class HttpRequestorTests + { + [TestCase] + public void Unauthorized401RejectsCredentials() + { + HttpRequestor.ShouldRejectCredentials(HttpStatusCode.Unauthorized) + .ShouldEqual(true, "A 401 is a definitive auth failure and must reject credentials"); + } + + [TestCase] + public void Redirect302RejectsCredentials() + { + HttpRequestor.ShouldRejectCredentials(HttpStatusCode.Redirect) + .ShouldEqual(true, "A 302 is the Azure DevOps sign-in redirect and must reject credentials"); + } + + [TestCase] + public void BadRequest400DoesNotRejectCredentials() + { + // A 400 is a request/formatting problem, not an expired credential. + // An expired or invalid credential always returns 401 or 302, never 400. Rejecting + // credentials on 400 erased valid credentials and caused a credential-popup storm. + HttpRequestor.ShouldRejectCredentials(HttpStatusCode.BadRequest) + .ShouldEqual(false, "A 400 is not an auth failure and must NOT reject credentials"); + } + + [TestCase] + public void CommonNonAuthStatusesDoNotRejectCredentials() + { + HttpRequestor.ShouldRejectCredentials(HttpStatusCode.NotFound) + .ShouldEqual(false, "A 404 must NOT reject credentials"); + HttpRequestor.ShouldRejectCredentials(HttpStatusCode.InternalServerError) + .ShouldEqual(false, "A 500 must NOT reject credentials"); + HttpRequestor.ShouldRejectCredentials(HttpStatusCode.RequestTimeout) + .ShouldEqual(false, "A 408 must NOT reject credentials"); + } + } +} From 227c58d09bed73ad54766606d1defa1d38a3c5de Mon Sep 17 00:00:00 2001 From: Tyrie Vella Date: Fri, 14 Aug 2026 08:31:45 -0700 Subject: [PATCH 3/6] HttpRequestor: only treat a 400 as auth failure for the cache-server "auth required" body An earlier change dropped HTTP 400 from the credential-rejection branch entirely, on the premise that a 400 is never an authentication failure. That premise is incomplete. The Azure DevOps GVFS cache server returns a 400 (not a 401) in one genuine authentication case: when the request carried no parseable Basic Authorization header. Its response body is "A valid Basic Authorization header is required." microsoft/git's git-gvfs-helper maps that same cache-server 400 to a 401 for this reason, and its own TODO says to confirm the response body - which is what this change does. A present-but-expired or invalid credential still returns 401, and a malformed request (for example a corrupt object SHA in the loose-object URL) returns a 400 that has nothing to do with credentials. So the decision is now body-aware: - 401 and 302 always reject credentials. - 400 rejects credentials only when the body matches the cache server's auth-required message (case-insensitive substring). - Every other 400 (and 404/5xx/timeouts) does not reject credentials. This stops the credential-manager popup storm caused by rejecting a valid credential on a non-auth 400, while preserving credential refresh for the one 400 that really does mean "authentication required", keeping the behavior consistent with git-gvfs-helper. Tests updated for the new body-aware signature and cases. Assisted-by: Claude Opus 4.8 Signed-off-by: Tyrie Vella --- GVFS/GVFS.Common/Http/HttpRequestor.cs | 56 +++++++++++++++---- .../GVFS.UnitTests/Http/HttpRequestorTests.cs | 55 ++++++++++++++---- 2 files changed, 89 insertions(+), 22 deletions(-) diff --git a/GVFS/GVFS.Common/Http/HttpRequestor.cs b/GVFS/GVFS.Common/Http/HttpRequestor.cs index 869b56a082..435e52c2b1 100644 --- a/GVFS/GVFS.Common/Http/HttpRequestor.cs +++ b/GVFS/GVFS.Common/Http/HttpRequestor.cs @@ -232,7 +232,7 @@ protected GitEndPointResponseData SendRequest( shouldRetry = false; errorMessage = "Anonymous request was rejected with a 401"; } - else if (ShouldRejectCredentials(response.StatusCode)) + else if (ShouldRejectCredentials(response.StatusCode, errorMessage)) { this.authentication.RejectCredentials(this.Tracer, authString); if (!this.authentication.IsBackingOff) @@ -327,21 +327,55 @@ private static bool ShouldRetry(HttpStatusCode statusCode) } /// - /// Determines whether an HTTP status code indicates an authentication failure + /// The message the Azure DevOps GVFS cache server returns in a 400 (Bad Request) + /// body when the request carried no parseable Basic Authorization header - i.e. + /// the one 400 that genuinely means "authentication required". + /// + /// + /// Mirrors the cache server's own message, emitted by + /// GvfsHttpHandler.PrepareContextAsync as + /// $"A valid {scheme} {header} header is required." with scheme="Basic" and + /// header="Authorization". Kept as a literal (not a format) so a substring match + /// stays robust if the server text is wrapped or prefixed. + /// + internal const string CacheServerAuthRequiredBadRequestMessage = "A valid Basic Authorization header is required."; + + /// + /// Determines whether an HTTP response indicates an authentication failure /// that warrants rejecting (erasing) the stored credential. /// /// - /// Only 401 (Unauthorized) and 302 (Redirect to the Azure DevOps sign-in page) - /// are genuine authentication failures. A 400 (Bad Request) is a request/formatting - /// problem (e.g. a malformed object URL), NOT an expired credential - an expired or - /// invalid credential always returns 401 or 302. Rejecting credentials on 400 erased - /// valid credentials and caused a storm of credential-manager popups, so 400 must NOT - /// reject credentials. + /// 401 (Unauthorized) and 302 (Redirect to the Azure DevOps sign-in page) are + /// always genuine authentication failures. A 400 (Bad Request) is usually NOT an + /// auth failure - a present-but-expired/invalid credential returns 401, and a + /// malformed request (e.g. a corrupt object SHA in the loose-object URL) returns a + /// 400 that has nothing to do with credentials. Rejecting credentials on every 400 + /// erased valid credentials and caused a storm of credential-manager popups. + /// + /// The one exception: the GVFS cache server returns a 400 (instead of a 401) when + /// the request carried no parseable Basic Authorization header. That single 400 is + /// genuinely "authentication required", and microsoft/git's git-gvfs-helper maps it + /// to a 401 for the same reason (its normalize step notes the cache server "sends a + /// somewhat bogus 400 instead of the normal 401 when AUTH is required", and its TODO + /// asks to confirm the response body - which is exactly what we do here). We only + /// treat a 400 as an auth failure when the body matches that specific message. /// - internal static bool ShouldRejectCredentials(HttpStatusCode statusCode) + internal static bool ShouldRejectCredentials(HttpStatusCode statusCode, string responseBody) { - return statusCode == HttpStatusCode.Unauthorized || - statusCode == HttpStatusCode.Redirect; + if (statusCode == HttpStatusCode.Unauthorized || + statusCode == HttpStatusCode.Redirect) + { + return true; + } + + if (statusCode == HttpStatusCode.BadRequest && + responseBody != null && + responseBody.IndexOf(CacheServerAuthRequiredBadRequestMessage, StringComparison.OrdinalIgnoreCase) >= 0) + { + return true; + } + + return false; } private static string GetSingleHeaderOrEmpty(HttpHeaders headers, string headerName) diff --git a/GVFS/GVFS.UnitTests/Http/HttpRequestorTests.cs b/GVFS/GVFS.UnitTests/Http/HttpRequestorTests.cs index b4ddeb4c86..33692dd03b 100644 --- a/GVFS/GVFS.UnitTests/Http/HttpRequestorTests.cs +++ b/GVFS/GVFS.UnitTests/Http/HttpRequestorTests.cs @@ -11,35 +11,68 @@ public class HttpRequestorTests [TestCase] public void Unauthorized401RejectsCredentials() { - HttpRequestor.ShouldRejectCredentials(HttpStatusCode.Unauthorized) + HttpRequestor.ShouldRejectCredentials(HttpStatusCode.Unauthorized, responseBody: null) .ShouldEqual(true, "A 401 is a definitive auth failure and must reject credentials"); } [TestCase] public void Redirect302RejectsCredentials() { - HttpRequestor.ShouldRejectCredentials(HttpStatusCode.Redirect) + HttpRequestor.ShouldRejectCredentials(HttpStatusCode.Redirect, responseBody: null) .ShouldEqual(true, "A 302 is the Azure DevOps sign-in redirect and must reject credentials"); } [TestCase] - public void BadRequest400DoesNotRejectCredentials() + public void BadRequest400WithAuthRequiredMessageRejectsCredentials() { - // A 400 is a request/formatting problem, not an expired credential. - // An expired or invalid credential always returns 401 or 302, never 400. Rejecting - // credentials on 400 erased valid credentials and caused a credential-popup storm. - HttpRequestor.ShouldRejectCredentials(HttpStatusCode.BadRequest) - .ShouldEqual(false, "A 400 is not an auth failure and must NOT reject credentials"); + // The GVFS cache server returns a 400 (instead of a 401) when the request carried + // no parseable Basic Authorization header. That single 400 genuinely means + // "authentication required", so it must reject credentials. We recognize it by the + // cache server's response body. + HttpRequestor.ShouldRejectCredentials( + HttpStatusCode.BadRequest, + HttpRequestor.CacheServerAuthRequiredBadRequestMessage) + .ShouldEqual(true, "A 400 whose body is the cache server's auth-required message must reject credentials"); + } + + [TestCase] + public void BadRequest400AuthRequiredMessageMatchIsCaseInsensitiveAndSubstring() + { + // The match is a case-insensitive substring so it stays robust if the server + // wraps or prefixes the text. + HttpRequestor.ShouldRejectCredentials( + HttpStatusCode.BadRequest, + "Error: a valid basic authorization header is required. (request 123)") + .ShouldEqual(true, "The auth-required message match must be a case-insensitive substring"); + } + + [TestCase] + public void BadRequest400WithNonAuthBodyDoesNotRejectCredentials() + { + // The storm case: a corrupt placeholder SHA makes the cache server return a 400 + // with an "Invalid ObjectId" body. That is NOT an auth failure and must not erase + // a valid credential. + HttpRequestor.ShouldRejectCredentials( + HttpStatusCode.BadRequest, + "Error processing GVFS request: Invalid ObjectId in the URI.") + .ShouldEqual(false, "A non-auth 400 (e.g. invalid object id) must NOT reject credentials"); + } + + [TestCase] + public void BadRequest400WithNullBodyDoesNotRejectCredentials() + { + HttpRequestor.ShouldRejectCredentials(HttpStatusCode.BadRequest, responseBody: null) + .ShouldEqual(false, "A 400 with no body must NOT reject credentials"); } [TestCase] public void CommonNonAuthStatusesDoNotRejectCredentials() { - HttpRequestor.ShouldRejectCredentials(HttpStatusCode.NotFound) + HttpRequestor.ShouldRejectCredentials(HttpStatusCode.NotFound, responseBody: null) .ShouldEqual(false, "A 404 must NOT reject credentials"); - HttpRequestor.ShouldRejectCredentials(HttpStatusCode.InternalServerError) + HttpRequestor.ShouldRejectCredentials(HttpStatusCode.InternalServerError, responseBody: null) .ShouldEqual(false, "A 500 must NOT reject credentials"); - HttpRequestor.ShouldRejectCredentials(HttpStatusCode.RequestTimeout) + HttpRequestor.ShouldRejectCredentials(HttpStatusCode.RequestTimeout, responseBody: null) .ShouldEqual(false, "A 408 must NOT reject credentials"); } } From a0f5fc3806e503c479acf763ff2e8f8584cd71c5 Mon Sep 17 00:00:00 2001 From: Dan Fiedler Date: Tue, 18 Aug 2026 12:14:40 -0400 Subject: [PATCH 4/6] Pin GitHub Actions to full-length commit SHAs --- .github/dependabot.yml | 2 ++ .github/workflows/build.yaml | 20 ++++++++++---------- .github/workflows/functional-tests.yaml | 22 +++++++++++----------- .github/workflows/upgrade-tests.yaml | 12 ++++++------ 4 files changed, 29 insertions(+), 27 deletions(-) diff --git a/.github/dependabot.yml b/.github/dependabot.yml index 22d5376407..7b2eaaf9f0 100644 --- a/.github/dependabot.yml +++ b/.github/dependabot.yml @@ -11,3 +11,5 @@ updates: directory: "/" # Location of package manifests schedule: interval: "weekly" + cooldown: + default-days: 7 diff --git a/.github/workflows/build.yaml b/.github/workflows/build.yaml index d0b4a4509a..a2778fb5a1 100644 --- a/.github/workflows/build.yaml +++ b/.github/workflows/build.yaml @@ -37,7 +37,7 @@ jobs: - name: Look for prior successful runs id: check if: github.event.inputs.git_version == '' - uses: actions/github-script@v9 + uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 with: github-token: ${{secrets.GITHUB_TOKEN}} result-encoding: string @@ -199,7 +199,7 @@ jobs: - name: Checkout source if: steps.check.outputs.result == '' - uses: actions/checkout@v7 + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 - name: Validate Microsoft Git version if: steps.check.outputs.result == '' @@ -249,7 +249,7 @@ jobs: - name: Upload microsoft/git installers if: steps.check.outputs.result == '' - uses: actions/upload-artifact@v7 + uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 with: name: MicrosoftGit path: MicrosoftGit @@ -269,7 +269,7 @@ jobs: - name: Skip this job if there is a previous successful run if: needs.validate.outputs.skip != '' id: skip - uses: actions/github-script@v9 + uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 with: script: | core.info(`Skipping: There already is a successful run: ${{ needs.validate.outputs.skip }}`) @@ -277,19 +277,19 @@ jobs: - name: Checkout source if: steps.skip.outputs.result != 'true' - uses: actions/checkout@v7 + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: path: src - name: Install .NET SDK if: steps.skip.outputs.result != 'true' - uses: actions/setup-dotnet@v6 + uses: actions/setup-dotnet@a98b56852c35b8e3190ac28c8c2271da59106c68 # v6.0.0 with: global-json-file: src/global.json - name: Add MSBuild to PATH if: steps.skip.outputs.result != 'true' - uses: microsoft/setup-msbuild@v3.0.0 + uses: microsoft/setup-msbuild@30375c66a4eea26614e0d39710365f22f8b0af57 # v3.0.0 - name: Build VFS for Git if: steps.skip.outputs.result != 'true' @@ -308,21 +308,21 @@ jobs: - name: Upload functional tests drop if: steps.skip.outputs.result != 'true' - uses: actions/upload-artifact@v7 + uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 with: name: FunctionalTests_${{ matrix.configuration }}_${{ matrix.architecture }} path: artifacts\GVFS.FunctionalTests - name: Upload FastFetch drop if: steps.skip.outputs.result != 'true' - uses: actions/upload-artifact@v7 + uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 with: name: FastFetch_${{ matrix.configuration }}_${{ matrix.architecture }} path: artifacts\FastFetch - name: Upload GVFS installer if: steps.skip.outputs.result != 'true' - uses: actions/upload-artifact@v7 + uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 with: name: GVFS_${{ matrix.configuration }}_${{ matrix.architecture }} path: artifacts\GVFS.Installers diff --git a/.github/workflows/functional-tests.yaml b/.github/workflows/functional-tests.yaml index 9b14047aab..60f61d3088 100644 --- a/.github/workflows/functional-tests.yaml +++ b/.github/workflows/functional-tests.yaml @@ -70,7 +70,7 @@ jobs: - name: Skip this job if there is a previous successful run if: inputs.skip != '' id: skip - uses: actions/github-script@v9 + uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 with: script: | core.info(`Skipping: There already is a successful run: ${{ inputs.skip }}`) @@ -80,7 +80,7 @@ jobs: id: download-git if: steps.skip.outputs.result != 'true' continue-on-error: true - uses: actions/download-artifact@v8 + uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8.0.1 with: name: ${{ inputs.git_artifact_name }} path: git @@ -90,7 +90,7 @@ jobs: - name: Download Git installer (retry) if: steps.skip.outputs.result != 'true' && steps.download-git.outcome == 'failure' - uses: actions/download-artifact@v8 + uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8.0.1 with: name: ${{ inputs.git_artifact_name }} path: git @@ -102,7 +102,7 @@ jobs: id: download-gvfs if: steps.skip.outputs.result != 'true' continue-on-error: true - uses: actions/download-artifact@v8 + uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8.0.1 with: name: GVFS_${{ matrix.configuration }}_${{ matrix.architecture }} path: gvfs @@ -112,7 +112,7 @@ jobs: - name: Download GVFS installer (retry) if: steps.skip.outputs.result != 'true' && steps.download-gvfs.outcome == 'failure' - uses: actions/download-artifact@v8 + uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8.0.1 with: name: GVFS_${{ matrix.configuration }}_${{ matrix.architecture }} path: gvfs @@ -124,7 +124,7 @@ jobs: id: download-ft if: steps.skip.outputs.result != 'true' continue-on-error: true - uses: actions/download-artifact@v8 + uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8.0.1 with: name: FunctionalTests_${{ matrix.configuration }}_${{ matrix.architecture }} path: ft @@ -134,7 +134,7 @@ jobs: - name: Download functional tests drop (retry) if: steps.skip.outputs.result != 'true' && steps.download-ft.outcome == 'failure' - uses: actions/download-artifact@v8 + uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8.0.1 with: name: FunctionalTests_${{ matrix.configuration }}_${{ matrix.architecture }} path: ft @@ -145,7 +145,7 @@ jobs: - name: Download FastFetch drop if: steps.skip.outputs.result != 'true' continue-on-error: true - uses: actions/download-artifact@v8 + uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8.0.1 with: name: FastFetch_${{ matrix.configuration }}_${{ matrix.architecture }} path: ft @@ -189,7 +189,7 @@ jobs: - name: Upload installation logs if: always() && steps.skip.outputs.result != 'true' - uses: actions/upload-artifact@v7 + uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 continue-on-error: true with: name: ${{ env.ARTIFACT_PREFIX }}InstallationLogs_${{ env.FT_MATRIX_NAME }} @@ -208,14 +208,14 @@ jobs: - name: Upload functional test results if: always() && steps.skip.outputs.result != 'true' - uses: actions/upload-artifact@v7 + uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 with: name: ${{ env.ARTIFACT_PREFIX }}FunctionalTests_Results_${{ env.FT_MATRIX_NAME }} path: TestResult.xml - name: Upload Git trace2 output if: always() && steps.skip.outputs.result != 'true' - uses: actions/upload-artifact@v7 + uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 with: name: ${{ env.ARTIFACT_PREFIX }}GitTrace2_${{ env.FT_MATRIX_NAME }} path: C:\temp\git-trace2.log diff --git a/.github/workflows/upgrade-tests.yaml b/.github/workflows/upgrade-tests.yaml index fe33976f0f..01bd1e4761 100644 --- a/.github/workflows/upgrade-tests.yaml +++ b/.github/workflows/upgrade-tests.yaml @@ -40,7 +40,7 @@ jobs: - name: Skip this job if there is a previous successful run if: inputs.skip != '' id: skip - uses: actions/github-script@v9 + uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 with: script: | core.info(`Skipping: There already is a successful run: ${{ inputs.skip }}`) @@ -66,14 +66,14 @@ jobs: id: download-git if: steps.skip.outputs.result != 'true' continue-on-error: true - uses: actions/download-artifact@v8 + uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8.0.1 with: name: MicrosoftGit path: git - name: Download Git installer (retry) if: steps.skip.outputs.result != 'true' && steps.download-git.outcome == 'failure' - uses: actions/download-artifact@v8 + uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8.0.1 with: name: MicrosoftGit path: git @@ -82,14 +82,14 @@ jobs: id: download-gvfs if: steps.skip.outputs.result != 'true' continue-on-error: true - uses: actions/download-artifact@v8 + uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8.0.1 with: name: GVFS_${{ matrix.configuration }}_x64 path: gvfs-new - name: Download current GVFS installer (retry) if: steps.skip.outputs.result != 'true' && steps.download-gvfs.outcome == 'failure' - uses: actions/download-artifact@v8 + uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8.0.1 with: name: GVFS_${{ matrix.configuration }}_x64 path: gvfs-new @@ -370,7 +370,7 @@ jobs: - name: Upload service logs if: always() && steps.skip.outputs.result != 'true' - uses: actions/upload-artifact@v7 + uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 continue-on-error: true with: name: UpgradeTest_Logs_${{ matrix.scenario }} From f7babbd3988671da5829b87861cd2081f1e3c15f Mon Sep 17 00:00:00 2001 From: Tyrie Vella Date: Wed, 26 Aug 2026 13:39:23 -0700 Subject: [PATCH 5/6] Fix upgrade tests when the LKG release has multiple installers The upgrade test job selected the last-known-good installer with `(Get-ChildItem gvfs-lkg\SetupGVFS*.exe).FullName`. Releases now publish both an x64 and an arm64 installer, so the glob matches two files and `.FullName` returns an array. `Start-Process -FilePath` then fails with "Cannot convert 'System.Object[]' to the type 'System.String'". Select the x64 installer explicitly. The x64 installer has no architecture suffix; the arm64 one is named `SetupGVFS.-arm64.exe`. These tests run on an x64 runner and download the x64 "new" installer, so the x64 LKG installer is the correct match. Apply the same guard to the "new" installer selection and throw a clear error if no x64 installer is present. Assisted-by: Claude Opus 4.8 Signed-off-by: Tyrie Vella --- .github/workflows/upgrade-tests.yaml | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/.github/workflows/upgrade-tests.yaml b/.github/workflows/upgrade-tests.yaml index 01bd1e4761..4b48a255e6 100644 --- a/.github/workflows/upgrade-tests.yaml +++ b/.github/workflows/upgrade-tests.yaml @@ -118,8 +118,18 @@ jobs: run: | $ErrorActionPreference = 'Stop' - $lkgInstaller = (Get-ChildItem gvfs-lkg\SetupGVFS*.exe).FullName - $newInstaller = (Get-ChildItem gvfs-new\SetupGVFS*.exe).FullName + # Releases now publish both x64 and arm64 installers. These tests run + # on an x64 runner and download the x64 "new" installer, so select the + # x64 LKG installer. The x64 installer has no architecture suffix; the + # arm64 one is named "SetupGVFS.-arm64.exe". + $lkgInstaller = (Get-ChildItem gvfs-lkg\SetupGVFS*.exe | + Where-Object { $_.Name -notmatch '-arm64\.exe$' } | + Select-Object -First 1).FullName + if (-not $lkgInstaller) { throw "No x64 LKG installer found in gvfs-lkg" } + $newInstaller = (Get-ChildItem gvfs-new\SetupGVFS*.exe | + Where-Object { $_.Name -notmatch '-arm64\.exe$' } | + Select-Object -First 1).FullName + if (-not $newInstaller) { throw "No x64 installer found in gvfs-new" } $installDir = "C:\Program Files\VFS for Git" $testRepo = "https://dev.azure.com/gvfs/ci/_git/ForTests" $enlistment = "C:\gvfs-upgrade-test" From c24182d3a28e54015f7a05fe1a540af643f0507d Mon Sep 17 00:00:00 2001 From: Tyrie Vella Date: Wed, 26 Aug 2026 14:23:26 -0700 Subject: [PATCH 6/6] Harden LKG installer selection to the x64 asset by name Address self-review feedback on the multi-installer fix. Replace the `-arm64` denylist plus `Select-Object -First 1` with a shared `Select-X64Installer` helper that positively matches the x64 asset by its suffix-less name (`SetupGVFS..exe`) and requires exactly one match. The denylist would still pass a future non-x64 asset (for example a `-x86` or `-arm` installer) and `-First 1` would then pick an arbitrary file. The positive allowlist matches the documented x64 naming contract and fails loudly when the directory holds an unexpected number of installers. The helper also removes the duplicated filter across the LKG and new installer selection, and its error message reports the directory and the files found. Note in a comment that arm64 upgrade is not exercised here because the runner is x64. Assisted-by: Claude Opus 4.8 Signed-off-by: Tyrie Vella --- .github/workflows/upgrade-tests.yaml | 31 +++++++++++++++++----------- 1 file changed, 19 insertions(+), 12 deletions(-) diff --git a/.github/workflows/upgrade-tests.yaml b/.github/workflows/upgrade-tests.yaml index 4b48a255e6..04cb26f6b1 100644 --- a/.github/workflows/upgrade-tests.yaml +++ b/.github/workflows/upgrade-tests.yaml @@ -118,18 +118,25 @@ jobs: run: | $ErrorActionPreference = 'Stop' - # Releases now publish both x64 and arm64 installers. These tests run - # on an x64 runner and download the x64 "new" installer, so select the - # x64 LKG installer. The x64 installer has no architecture suffix; the - # arm64 one is named "SetupGVFS.-arm64.exe". - $lkgInstaller = (Get-ChildItem gvfs-lkg\SetupGVFS*.exe | - Where-Object { $_.Name -notmatch '-arm64\.exe$' } | - Select-Object -First 1).FullName - if (-not $lkgInstaller) { throw "No x64 LKG installer found in gvfs-lkg" } - $newInstaller = (Get-ChildItem gvfs-new\SetupGVFS*.exe | - Where-Object { $_.Name -notmatch '-arm64\.exe$' } | - Select-Object -First 1).FullName - if (-not $newInstaller) { throw "No x64 installer found in gvfs-new" } + # Releases publish both x64 and arm64 installers. The x64 installer + # keeps the historical suffix-less name "SetupGVFS..exe"; other + # architectures add a suffix (e.g. "SetupGVFS.-arm64.exe"). + # These tests run on an x64 runner, so select the x64 installer by its + # suffix-less name and require exactly one match, rather than picking an + # arbitrary file when the directory holds more than one installer. + # NOTE: arm64 upgrade is not exercised here because the runner is x64; + # arm64 upgrade coverage is a known gap for when arm64 runners exist. + function Select-X64Installer($directory) { + $installers = @(Get-ChildItem "$directory\SetupGVFS*.exe" | + Where-Object { $_.Name -match '^SetupGVFS\.[\d.]+\.exe$' }) + if ($installers.Count -ne 1) { + throw "Expected exactly one x64 installer in '$directory', found $($installers.Count): $($installers.Name -join ', ')" + } + return $installers[0].FullName + } + + $lkgInstaller = Select-X64Installer "gvfs-lkg" + $newInstaller = Select-X64Installer "gvfs-new" $installDir = "C:\Program Files\VFS for Git" $testRepo = "https://dev.azure.com/gvfs/ci/_git/ForTests" $enlistment = "C:\gvfs-upgrade-test"