diff --git a/.github/dependabot.yml b/.github/dependabot.yml index 22d537640..7b2eaaf9f 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 d0b4a4509..a2778fb5a 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 9b14047aa..60f61d308 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 fe33976f0..04cb26f6b 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 @@ -118,8 +118,25 @@ jobs: run: | $ErrorActionPreference = 'Stop' - $lkgInstaller = (Get-ChildItem gvfs-lkg\SetupGVFS*.exe).FullName - $newInstaller = (Get-ChildItem gvfs-new\SetupGVFS*.exe).FullName + # 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" @@ -370,7 +387,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 }} diff --git a/GVFS/GVFS.Common/GVFSConstants.cs b/GVFS/GVFS.Common/GVFSConstants.cs index 3dd946bff..143b59e69 100644 --- a/GVFS/GVFS.Common/GVFSConstants.cs +++ b/GVFS/GVFS.Common/GVFSConstants.cs @@ -45,6 +45,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 03cc27b41..43a40ced0 100644 --- a/GVFS/GVFS.Common/Git/GitProcess.cs +++ b/GVFS/GVFS.Common/Git/GitProcess.cs @@ -806,6 +806,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/Http/HttpRequestor.cs b/GVFS/GVFS.Common/Http/HttpRequestor.cs index 0f9767dde..435e52c2b 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, errorMessage)) { this.authentication.RejectCredentials(this.Tracer, authString); if (!this.authentication.IsBackingOff) @@ -326,6 +326,58 @@ private static bool ShouldRetry(HttpStatusCode statusCode) return false; } + /// + /// 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. + /// + /// + /// 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, string responseBody) + { + 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) { IEnumerable values; diff --git a/GVFS/GVFS.Common/Maintenance/GitMaintenanceScheduler.cs b/GVFS/GVFS.Common/Maintenance/GitMaintenanceScheduler.cs index 760f80329..2759306ff 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 a5fc5b54a..e51f11a2a 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/Http/HttpRequestorTests.cs b/GVFS/GVFS.UnitTests/Http/HttpRequestorTests.cs new file mode 100644 index 000000000..33692dd03 --- /dev/null +++ b/GVFS/GVFS.UnitTests/Http/HttpRequestorTests.cs @@ -0,0 +1,79 @@ +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, responseBody: null) + .ShouldEqual(true, "A 401 is a definitive auth failure and must reject credentials"); + } + + [TestCase] + public void Redirect302RejectsCredentials() + { + HttpRequestor.ShouldRejectCredentials(HttpStatusCode.Redirect, responseBody: null) + .ShouldEqual(true, "A 302 is the Azure DevOps sign-in redirect and must reject credentials"); + } + + [TestCase] + public void BadRequest400WithAuthRequiredMessageRejectsCredentials() + { + // 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, responseBody: null) + .ShouldEqual(false, "A 404 must NOT reject credentials"); + HttpRequestor.ShouldRejectCredentials(HttpStatusCode.InternalServerError, responseBody: null) + .ShouldEqual(false, "A 500 must NOT reject credentials"); + HttpRequestor.ShouldRejectCredentials(HttpStatusCode.RequestTimeout, responseBody: null) + .ShouldEqual(false, "A 408 must NOT reject credentials"); + } + } +} diff --git a/GVFS/GVFS.UnitTests/Maintenance/PackfileMaintenanceStepTests.cs b/GVFS/GVFS.UnitTests/Maintenance/PackfileMaintenanceStepTests.cs index 811b55e15..7f7aab907 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; + } + } } }