From 9ec7abfeb73b5ecbcbd0158829f617339f03f2c0 Mon Sep 17 00:00:00 2001 From: Tyrie Vella Date: Thu, 9 Jul 2026 12:53:37 -0700 Subject: [PATCH] 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; + } + } } }