Skip to content

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
microsoft:masterfrom
tyrielv:tyrielv/auth-lock-contention
Draft

Auth concurrency: release HTTP pool slot before credential reject; thread cancellation to the credential path (stacked on #2046)#2082
tyrielv wants to merge 2 commits into
microsoft:masterfrom
tyrielv:tyrielv/auth-lock-contention

Conversation

@tyrielv

@tyrielv tyrielv commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Stacked on #2046 — do not merge before it.
This PR branches off tyrielv/prefetch-auth-timeout (the head of #2046), not master.
Until #2046 merges, the diff shown here also includes that PR's single commit. Review
only the top commit ("Stop holding the HTTP pool slot across credential work; thread
cancellation to the credential path"). #2046 is still a draft and will be rebased, so
this branch will be rebased onto its new tip before it is ready.

What this changes

A six-lens review of #2046 found three pre-existing HIGH issues that the 120s 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 — gitAuthLock held across the credential fetch) is a riskier monitor-lock redesign
and will follow as its own stacked PR.

F07 — thread CancellationToken to 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 RetryWrapper observed the cancellation.

  • 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 — release the connection-pool slot before the reject leg (off by default)

On a 401, RejectCredentials can block for a long time on a slow or hung credential
helper while SendRequest still holds a process-wide connection-pool slot. Under
parallel 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.

  • The error body is already buffered into errorMessage, so SendRequest now frees the
    slot before the reject leg runs.
  • Gated behind the new off-by-default 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 throws (for example on cancellation).

Tests

Six new tests pin the new invariants:

  • Cancellation interrupts a blocked credential fetch and a blocked reject-reload.
  • The caller's token reaches the git invocation.
  • The pool slot is released before the reject leg when the flag is on, and held when it
    is off.
  • The slot is released exactly once when a reject is canceled (no double-release).

HttpRequestorTests drives the real SendRequest through an injected HttpMessageHandler
and a MockGitProcess that can block until signaled or canceled. Each assertion was
mutation-tested: reverting the matching fix makes exactly the intended test fail.

Full unit suite: 908 tests, 0 failed. StyleCop clean.

Notes for review

  • InvokeGitImpl throws OperationCanceledException on cancellation rather than returning
    a failure Result. This is deliberate: returning a failure would make RetryWrapper
    retry, which is wrong for a canceled operation. RetryWrapper does not treat
    OperationCanceledException as retryable, so it aborts.
  • F06 is behind a flag; F07 is always on because it only extends cancellation reach and
    does not change behavior when no cancelable token is passed.

tyrielv and others added 2 commits August 10, 2026 16:09
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 runtime bound uses the generous BackgroundCredentialTimeoutMs (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 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.

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.

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.

Fixes AB#63011829

Assisted-by: Claude Opus 4.8
Signed-off-by: Tyrie Vella <tyrielv@gmail.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.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>
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