Merge master into vnext (20260827) - #2098
Open
tyrielv wants to merge 11 commits into
Open
Conversation
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 <tyrielv@gmail.com>
GVFS erased a valid credential when an object-download response was HTTP 400, which triggered a storm of Git Credential Manager popups. A 400 is a request or formatting problem, not an authentication failure. An expired or invalid credential always returns 401 (Unauthorized) or 302 (the Azure DevOps sign-in redirect), never 400. Treating 400 as an auth failure erased good credentials and produced the misleading "Your PAT may be expired" message. Remove BadRequest (400) from the credential-rejection branch in SendRequest. Only 401 and 302 now reject credentials; 400 flows through the generic, non-auth error path (unchanged retry / circuit-breaker behavior, and 400 remains non-retryable). Extract the decision into ShouldRejectCredentials so it is unit tested: 400 does not reject credentials, while 401 and 302 still do. Assisted-by: Claude Opus 4.8 Signed-off-by: Tyrie Vella <tyrielv@gmail.com>
…"auth required" body An earlier change dropped HTTP 400 from the credential-rejection branch entirely, on the premise that a 400 is never an authentication failure. That premise is incomplete. The Azure DevOps GVFS cache server returns a 400 (not a 401) in one genuine authentication case: when the request carried no parseable Basic Authorization header. Its response body is "A valid Basic Authorization header is required." microsoft/git's git-gvfs-helper maps that same cache-server 400 to a 401 for this reason, and its own TODO says to confirm the response body - which is what this change does. A present-but-expired or invalid credential still returns 401, and a malformed request (for example a corrupt object SHA in the loose-object URL) returns a 400 that has nothing to do with credentials. So the decision is now body-aware: - 401 and 302 always reject credentials. - 400 rejects credentials only when the body matches the cache server's auth-required message (case-insensitive substring). - Every other 400 (and 404/5xx/timeouts) does not reject credentials. This stops the credential-manager popup storm caused by rejecting a valid credential on a non-auth 400, while preserving credential refresh for the one 400 that really does mean "authentication required", keeping the behavior consistent with git-gvfs-helper. Tests updated for the new body-aware signature and cases. Assisted-by: Claude Opus 4.8 Signed-off-by: Tyrie Vella <tyrielv@gmail.com>
…-actions Pin GitHub Actions to full-length commit SHAs
…-credentials-on-400 HttpRequestor: do not reject credentials on HTTP 400
…ption-recovery Auto-recover corrupt packfiles in packfile maintenance
The upgrade test job selected the last-known-good installer with `(Get-ChildItem gvfs-lkg\SetupGVFS*.exe).FullName`. Releases now publish both an x64 and an arm64 installer, so the glob matches two files and `.FullName` returns an array. `Start-Process -FilePath` then fails with "Cannot convert 'System.Object[]' to the type 'System.String'". Select the x64 installer explicitly. The x64 installer has no architecture suffix; the arm64 one is named `SetupGVFS.<version>-arm64.exe`. These tests run on an x64 runner and download the x64 "new" installer, so the x64 LKG installer is the correct match. Apply the same guard to the "new" installer selection and throw a clear error if no x64 installer is present. Assisted-by: Claude Opus 4.8 Signed-off-by: Tyrie Vella <tyrielv@gmail.com>
Address self-review feedback on the multi-installer fix. Replace the `-arm64` denylist plus `Select-Object -First 1` with a shared `Select-X64Installer` helper that positively matches the x64 asset by its suffix-less name (`SetupGVFS.<version>.exe`) and requires exactly one match. The denylist would still pass a future non-x64 asset (for example a `-x86` or `-arm` installer) and `-First 1` would then pick an arbitrary file. The positive allowlist matches the documented x64 naming contract and fails loudly when the directory holds an unexpected number of installers. The helper also removes the duplicated filter across the LKG and new installer selection, and its error message reports the directory and the files found. Note in a comment that arm64 upgrade is not exercised here because the runner is x64. Assisted-by: Claude Opus 4.8 Signed-off-by: Tyrie Vella <tyrielv@gmail.com>
…g-arm64 Fix upgrade tests when the LKG release has multiple installers
…ter-to-vnext-20260827
tyrielv
enabled auto-merge
August 27, 2026 17:30
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Routine forward-merge of
masterintovnextto keepvnextcurrent so master's fixes flow down and the two lines do not diverge. Merge commit only (no squash/rebase).master was 10 commits ahead of vnext. Commits brought down:
The merge was clean with no conflicts.