Skip to content

Bound runtime credential fetch so prefetch can't hang on auth - #2046

Draft
tyrielv wants to merge 1 commit into
microsoft:masterfrom
tyrielv:tyrielv/prefetch-auth-timeout
Draft

Bound runtime credential fetch so prefetch can't hang on auth#2046
tyrielv wants to merge 1 commit into
microsoft:masterfrom
tyrielv:tyrielv/prefetch-auth-timeout

Conversation

@tyrielv

@tyrielv tyrielv commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Problem

A user missed a Git Credential Manager auth popup (it was behind another window). Instead of timing out, GVFS waited indefinitely. The blocked operation was the mount's background maintenance PrefetchStep (not user-initiated), which holds the shared prefetch-commits-trees.lock on the gvfs object cache. Because that lock was never released, a subsequent user-initiated gvfs prefetch blocked behind it forever.

Root cause

Runtime credential fetches flow through:

HttpRequestor.SendRequest
  -> GitAuthentication.TryGetCredentials(tracer, out cred, out err)
  -> GitAuthentication.TryCallGitCredential(tracer, out err)   // timeoutMs defaulted to -1
  -> GitProcess.TryGetCredential(..., timeoutMs: -1)
  -> InvokeGitImpl(... timeoutMs: -1) -> Process.WaitForExit(-1)  // infinite

The mount startup auth path was already bounded (credentialTimeoutMs on TryInitializeAndQueryGVFSConfig), but the runtime path — used by every object/pack download, including the background maintenance prefetch — was never given a finite timeout. The timeout plumbing already exists end-to-end in GitProcess/InvokeGitImpl; the runtime path simply never passed a finite value.

The mount's maintenance prefetch and on-demand hydration share one GitObjectsHttpRequestor / one GitAuthentication, so bounding the shared runtime path fixes the maintenance-prefetch hang.

Fix

1. Bound every runtime credential invocation

  • TryGetCredentials takes credentialTimeoutMs and plumbs it to TryCallGitCredential.
  • RejectCredentials — which reloads the credential on the 401-retry leg — takes and plumbs the same timeout. This leg is the actual stale-token hang path.
  • ApproveCredentials and the ICredentialStore store/delete operations are bounded too. git credential approve and git credential reject previously ran with timeoutMs = -1 while holding gitAuthLock, so a stalled helper could still pin the prefetch lock forever even after the fill leg was bounded.
  • HttpRequestor exposes a protected virtual int CredentialTimeoutMs and passes it to all three.

2. The gate must outlast the fetch it serializes

TryCallGitCredential waited a fixed 60s on credentialGate, and on expiry fell through and spawned a second credential fetch. With a 120s fetch bound that guaranteed a second, competing GCM prompt in exactly the slow-prompt case the longer bound exists to tolerate. The gate now waits at least as long as the fetch.

3. A timeout no longer burns the retry budget

SendRequest previously returned shouldRetry: true for every credential failure, so one timeout could consume the whole RetryWrapper budget (up to MaxAttempts x 120s), re-prompting the user each time. TryGetCredentials now reports whether the failure was a timeout, and SendRequest sets shouldRetry accordingly. Genuine auth failures still retry exactly as before.

4. Kill the process tree, not just git.exe

On timeout InvokeGitImpl called Process.Kill(), which left the credential-helper child alive — holding the credential store and showing orphaned prompt UI. It now kills the whole tree, then waits (bounded) so the async stdout/stderr readers flush before their buffers are read.

5. Configurable, with an escape hatch

The bound lives on RetryConfig (default 120s), overridable via gvfs.credential-timeout-seconds; 0 or less restores the historical unbounded wait.

RetryConfig is already loaded once per process from git config and already passed into the HttpRequestor constructor alongside MaxRetries/Timeout, so this adds no config I/O at requestor construction — no git config spawn on the mount startup path, and no new unbounded git invocation (GetFromConfig itself runs unbounded, which is the very class of call this PR removes).

6. Measurable in the field

The timeout emits a distinct CredentialFetchTimedOut event with structured timeoutMs/RepoUrl fields rather than only warning text — so we can measure how often the bound fires, and correlate a timeout with a later successful fetch (a prompt that got cut off) versus none (a hang that was prevented).

Why 120s and not 30s: the mount's requestor is shared by the background maintenance prefetch, interactive on-demand hydration, and the user-initiated gvfs prefetch/clone verbs. A 30s bound risks converting today's hang into a spurious auth failure when a human is legitimately slow to answer a GCM cold-start / MFA / smartcard prompt. 120s still bounds the indefinite hang — the reported case was a prompt never answered — while giving a noticed prompt ample time.

On timeout, TryGetCredentials returns false and engages backoff. The download gives up, TryDownloadPrefetchPacks returns false, and PrefetchStep releases prefetch-commits-trees.lock — unblocking the user-initiated prefetch — instead of hanging forever.

Compatibility

ICredentialStore.TryGetCredential gains a required out bool timedOut parameter (needed to tell a timeout apart from a genuine auth failure). GitProcess is the only implementer and GitAuthentication the only caller, both updated here. The remaining new parameters are optional with defaults, and CredentialTimeoutMs is virtual, so all other callers compile unchanged.

Tests

  • TryGetCredentialsTimesOutWhenCredentialManagerDoesNotRespond — asserts the timeout reaches InvokeGitImpl and is rendered in the message, not just that some failure occurred.
  • RejectCredentialsBoundsTheCredentialReload — the 401-reject leg bounds both the credential reload and the erase (no invocation left at -1).
  • TryGetCredentialsReportsTimedOutOnlyForTimeouts — a generic auth failure is not reported as a timeout, so real failures still retry.
  • RetryConfig coverage for the default, an explicit value, the non-positive unbounded escape hatch, and a guard that hand-built RetryConfigs still get a bounded default.
  • MockGitProcess now records the timeout passed to each git invocation, which is what makes the above assertions possible.

Mutation-verified: the two central tests were confirmed to fail when the fix is reverted — removing the credentialTimeoutMs plumbing, and removing the timedOut signal.

Full suite: 930 tests, 919 passed, 0 failed (11 pre-existing ignored); builds with 0 warnings / 0 errors.

The runtime credential path (HttpRequestor.SendRequest ->
GitAuthentication.TryGetCredentials/RejectCredentials ->
TryCallGitCredential) called git-credential with timeoutMs = -1, so
Process.WaitForExit(-1) waited forever. When a GCM auth popup was missed
(e.g. behind another window), the mount's background maintenance
PrefetchStep blocked indefinitely while holding the shared
prefetch-commits-trees.lock, which in turn blocked a user-initiated
`gvfs prefetch`.

The mount startup auth path was already bounded via credentialTimeoutMs;
this extends the same bound to every runtime credential invocation:

- TryGetCredentials takes credentialTimeoutMs (default
  DefaultCredentialTimeoutMs) and plumbs it to TryCallGitCredential.
- RejectCredentials, which reloads the credential on the 401-retry leg,
  takes and plumbs the same timeout (this leg is the actual stale-token
  hang path and was otherwise still unbounded).
- ApproveCredentials, RejectCredentials and the ICredentialStore
  store/delete operations are bounded too. `git credential approve` and
  `git credential reject` previously ran with timeoutMs = -1 while
  holding gitAuthLock, so a stalled helper could still pin the prefetch
  lock forever even after the fill leg was bounded.
- HttpRequestor exposes a protected virtual CredentialTimeoutMs and
  passes it to TryGetCredentials, RejectCredentials and
  ApproveCredentials.

The bound is generous (120s) rather than the 30s default: the mount's
requestor is shared by the background maintenance prefetch, interactive
on-demand hydration, and the user-initiated prefetch/clone verbs, where a
human may legitimately take longer than 30s to answer a GCM cold-start /
MFA / smartcard prompt. 120s still bounds the hang while being long
enough not to cut off a prompt the user is actively answering.

The value lives on RetryConfig, which is already loaded once from git
config and already passed into the HttpRequestor constructor alongside
MaxRetries and Timeout. It is overridable via
gvfs.credential-timeout-seconds; 0 or less restores the old unbounded
wait as a field escape hatch. Reading it here rather than inside the
requestor keeps requestor construction free of config I/O: a per-instance
read would spawn `git config` on the mount startup path, and
GetFromConfig itself runs unbounded, which is exactly the class of
unbounded git invocation this change exists to remove.

The credential serialization gate now waits at least as long as the fetch
it is serializing. It previously waited a fixed 60s, and on expiry fell
through and spawned a second credential fetch. With a 120s fetch bound
that guaranteed a second, competing GCM prompt in exactly the slow-prompt
case the longer bound exists to tolerate.

A timed-out fetch no longer asks the caller to retry. SendRequest
previously returned shouldRetry: true for every credential failure, so a
timeout burned the whole RetryWrapper budget (up to MaxAttempts x 120s),
re-prompting the user each time. TryGetCredentials now reports whether
the failure was a timeout, and SendRequest sets shouldRetry accordingly;
genuine auth failures still retry as before.

On timeout the git process tree is killed, not just git.exe. Killing only
git.exe left the credential helper child alive, holding the credential
store and showing orphaned prompt UI. The kill is now followed by a
bounded wait so the async stdout/stderr readers flush before their
buffers are read.

The timeout is reported as a distinct CredentialFetchTimedOut telemetry
event with structured timeoutMs and RepoUrl fields, rather than only as
warning message text. This is what makes the 120s choice measurable in
the field: how often the bound fires, and whether a timeout is followed
by a successful fetch (a prompt that was cut off) or not (a hang that was
prevented).

On timeout the fetch fails, backoff engages, the download gives up, and
the lock is released instead of hanging forever.

Tests: MockGitProcess now records the timeout passed to each git
invocation, so tests can assert the bound is actually plumbed rather than
just that a failure message appears. The timeout test asserts the
observed timeout and the rendered "within 1 seconds" message; reverting
the plumbing makes it fail (verified by mutation). Adds a test that the
401-reject leg bounds both the credential reload and the erase, a test
that only genuine timeouts are reported as timeouts so a real auth
failure still retries (also mutation-verified), and RetryConfig coverage
for the default, configured, and unbounded-escape-hatch values.

Assisted-by: Claude Opus 4.8
Signed-off-by: Tyrie Vella <tyrielv@gmail.com>
@tyrielv
tyrielv force-pushed the tyrielv/prefetch-auth-timeout branch from 0e8da9e to 7895b45 Compare August 11, 2026 20:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant