Conversation
tyrielv
force-pushed
the
tyrielv/auth-lock-contention
branch
from
August 18, 2026 20:51
c2b26d1 to
840201a
Compare
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 <[email protected]>
…lation to the credential path This is a stacked follow-up on the runtime credential-timeout PR. It addresses two deferred HIGH findings from the review swarm (F06, F07). Both are pre-existing issues that the 120s credential bound makes worse. F07 (always-on): thread CancellationToken to the credential path. - SendRequest now passes its token to TryGetCredentials, ApproveCredentials, and RejectCredentials, then on through ICredentialStore and GitProcess to InvokeGitImpl. - credentialGate.Wait now observes the token. - InvokeGitImpl waits for the git child with a cancellation-aware poll loop, because Process.WaitForExit has no token overload. On cancellation it kills the process tree and throws OperationCanceledException, so RetryWrapper aborts promptly instead of retrying. Callers that pass no token keep the previous behavior. F06 (off by default): release the connection-pool slot before the credential-reject leg. - On a 401 the error body is already buffered, so SendRequest can free its process-wide connection slot before the reject leg blocks on a slow or hung credential helper. This stops parallel healthy requests from starving on the pool. - Gated behind the new off-by-default config flag gvfs.release-connection-before-credential-reject, per the repo convention for risky runtime changes during stabilization ships. - The finally block does not release a second time if a reject that ran after the early release then threw (for example on cancellation). Tests - 6 new tests (GitAuthenticationTests, HttpRequestorTests) pin the new invariants: cancellation interrupts a blocked fetch and a blocked reject-reload; the token reaches the git invocation; the pool slot is released before the reject leg when enabled and held when disabled; and the slot is released exactly once when a reject is canceled. - Each assertion was mutation-tested: reverting the fix makes the matching test fail. - Full unit suite: 908 tests, 0 failed. Co-authored-by: Copilot <[email protected]>
tyrielv
force-pushed
the
tyrielv/auth-lock-contention
branch
from
September 22, 2026 17:09
840201a to
21df248
Compare
tyrielv
changed the title
Preserve cancellation through the pre-clone credential fallback route. Reject timeout values that overflow milliseconds. Stop endpoint fallback after terminal credential timeouts. Cancel initialization and credential-gate waits, and never start a concurrent helper after gate acquisition times out. Sanitize credential-timeout telemetry, publish feature configuration atomically, emit feature-cohort telemetry, and drain asynchronous process output after exit. Add regression coverage for gate contention, cancellation cleanup, endpoint fallback, negative timeout values, and timeout overflow. Signed-off-by: Tyrie Vella <[email protected]>
tyrielv
force-pushed
the
tyrielv/auth-lock-contention
branch
from
September 22, 2026 18:16
21df248 to
8b58e57
Compare
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
1 participant
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.
Problem
Runtime credential helpers could wait forever during object downloads and background prefetch.
A missed Git Credential Manager prompt could block maintenance while it held the shared prefetch lock.
Credential work also held shared HTTP capacity. Concurrent authentication failures could then block healthy hydration requests.
Cancellation stopped at the HTTP retry layer. It could not interrupt credential gates or credential-helper processes.
Changes
Bound runtime credential operations
gvfs.credential-timeout-secondsthroughRetryConfig.CredentialFetchTimedOuttelemetry with the timeout and repository URL.Propagate cancellation
CancellationTokenfrom HTTP requests throughGitAuthentication,ICredentialStore, andGitProcess.OperationCanceledExceptionsoRetryWrapperstops instead of retrying.Callers that pass no cancelable token keep the previous cancellation behavior.
Release HTTP capacity before credential rejection
The response body is buffered before credential rejection starts.
When enabled, the requestor disposes the response and releases its connection permit before credential work begins.
This behavior uses the off-by-default
gvfs.release-connection-before-credential-rejectflag.The cleanup path tracks the early release and prevents a second release after cancellation or another exception.
Preserve credential routing
Credential commands use the enlistment
.gitdirectory when it exists.They run outside the enlistment before clone creates that directory.
Timeout and cancellation values now propagate through both routes.
Tests
GVFS.UnitTests.csprojin Debug with zero warnings and zero errors.vnext.Supersedes #2046
This PR contains all useful work from #2046 and the authentication-concurrency follow-up.
It is based directly on current
vnext. The earlier PR can close without merging.