Auth concurrency: release HTTP pool slot before credential reject; thread cancellation to the credential path (stacked on #2046) - #2082
Draft
tyrielv wants to merge 2 commits into
Conversation
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>
…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 <223556219+Copilot@users.noreply.github.com>
tyrielv
force-pushed
the
tyrielv/auth-lock-contention
branch
from
August 18, 2026 20:51
c2b26d1 to
840201a
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.
What this changes
A six-lens review of #2046 found three pre-existing HIGH issues that the credential bound
makes worse. They share one theme: do not hold shared locks and shared resources while
doing credential work. This PR does the two lower-risk ones (F06, F07). The third
(F04 —
gitAuthLockheld across the credential fetch) is a riskier monitor-lock redesignand will follow as its own stacked PR.
F07 — thread
CancellationTokento the credential path (always on)Mount shutdown and user cancellation could not interrupt credential work, so a canceled
request could stay blocked for minutes before
RetryWrapperobserved the cancellation.SendRequestnow passes its token toTryGetCredentials,ApproveCredentials, andRejectCredentials, then on throughICredentialStoreandGitProcesstoInvokeGitImpl.credentialGate.Waitnow observes the token.InvokeGitImplwaits for the git child with a cancellation-aware poll loop, becauseProcess.WaitForExithas no token overload. On cancellation it kills the process treeand throws
OperationCanceledException, soRetryWrapperaborts promptly instead ofretrying. Callers that pass no token keep the previous behavior.
out bool timedOutsignal; the two features coexiston the same call chain.
F06 — release the connection-pool slot before the reject leg (off by default)
On a 401,
RejectCredentialscan block for a long time on a slow or hung credentialhelper while
SendRequeststill holds a process-wide connection-pool slot. Underparallel 401s every slot can be occupied by a thread waiting on credentials, so healthy
interactive hydration fails on the 30s pool-wait even though the network is fine.
errorMessage, soSendRequestnow frees theslot before the reject leg runs.
gvfs.release-connection-before-credential-reject,per the repo convention for risky runtime changes during stabilization ships.
finallyblock does not release a second time if a reject that ran after the earlyrelease then throws (for example on cancellation).
Tests
Six new tests pin the new invariants:
is off.
HttpRequestorTestsdrives the realSendRequestthrough an injectedHttpMessageHandlerand a
MockGitProcessthat can block until signaled or canceled. Each assertion wasmutation-tested: reverting the matching fix makes exactly the intended test fail.
Full unit suite: 939 tests, 0 failed. StyleCop clean.
Notes for review
InvokeGitImplthrowsOperationCanceledExceptionon cancellation rather than returninga failure
Result. This is deliberate: returning a failure would makeRetryWrapperretry, which is wrong for a canceled operation.
RetryWrapperdoes not treatOperationCanceledExceptionas retryable, so it aborts.does not change behavior when no cancelable token is passed.