From 23ca9717fe35bf12dbb84e2232f455b155b32810 Mon Sep 17 00:00:00 2001 From: Tyrie Vella Date: Wed, 8 Jul 2026 10:49:44 -0700 Subject: [PATCH 1/2] Bound runtime credential fetch so prefetch can't hang on auth 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 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- GVFS/GVFS.Common/Git/GitAuthentication.cs | 34 +++++---- GVFS/GVFS.Common/Git/GitProcess.cs | 25 +++++-- GVFS/GVFS.Common/Git/ICredentialStore.cs | 4 +- GVFS/GVFS.Common/Http/HttpRequestor.cs | 16 ++++- .../Git/GitAuthenticationTests.cs | 72 +++++++++++++++++++ .../GVFS.UnitTests/Mock/Git/MockGitProcess.cs | 24 +++++-- 6 files changed, 149 insertions(+), 26 deletions(-) diff --git a/GVFS/GVFS.Common/Git/GitAuthentication.cs b/GVFS/GVFS.Common/Git/GitAuthentication.cs index 96f47673d1..ef00bf719c 100644 --- a/GVFS/GVFS.Common/Git/GitAuthentication.cs +++ b/GVFS/GVFS.Common/Git/GitAuthentication.cs @@ -16,6 +16,13 @@ public class GitAuthentication public const int DefaultCredentialTimeoutMs = 30_000; public const int BackgroundCredentialTimeoutMs = 120_000; + /// + /// Minimum time to wait for an in-flight credential fetch before giving up on + /// serialization. The effective wait is never shorter than the fetch timeout + /// itself, so a slow but legitimate prompt cannot cause a second prompt. + /// + private const int DefaultCredentialGateWaitMs = 60_000; + private readonly Lock gitAuthLock = new Lock(); private readonly SemaphoreSlim credentialGate = new SemaphoreSlim(1, 1); private readonly ICredentialStore credentialStore; @@ -52,7 +59,7 @@ public bool IsBackingOff private GitSsl GitSsl { get; } - public void ApproveCredentials(ITracer tracer, string credentialString) + public void ApproveCredentials(ITracer tracer, string credentialString, int credentialTimeoutMs = DefaultCredentialTimeoutMs) { lock (this.gitAuthLock) { @@ -70,7 +77,7 @@ public void ApproveCredentials(ITracer tracer, string credentialString) string password; if (TryParseCredentialString(this.cachedCredentialString, out username, out password)) { - if (!this.credentialStore.TryStoreCredential(tracer, this.repoUrl, username, password, out string error)) + if (!this.credentialStore.TryStoreCredential(tracer, this.repoUrl, username, password, out string error, credentialTimeoutMs)) { // Storing credentials is best effort attempt - log failure, but do not fail tracer.RelatedWarning("Failed to store credential string: {0}", error); @@ -91,7 +98,7 @@ public void ApproveCredentials(ITracer tracer, string credentialString) } } - public void RejectCredentials(ITracer tracer, string credentialString) + public void RejectCredentials(ITracer tracer, string credentialString, int credentialTimeoutMs = DefaultCredentialTimeoutMs) { lock (this.gitAuthLock) { @@ -102,7 +109,7 @@ public void RejectCredentials(ITracer tracer, string credentialString) // We can't assume that the credential store's cached credential is the same as the one we have. // Reload the credential from the store to ensure we're rejecting the correct one. int attemptsBeforeCheckingExistingCredential = this.numberOfAttempts; - if (this.TryCallGitCredential(tracer, out string getCredentialError)) + if (this.TryCallGitCredential(tracer, out string getCredentialError, credentialTimeoutMs)) { if (this.cachedCredentialString != cachedCredentialAtStartOfReject) { @@ -123,7 +130,7 @@ public void RejectCredentials(ITracer tracer, string credentialString) string password; if (TryParseCredentialString(this.cachedCredentialString, out username, out password)) { - if (!this.credentialStore.TryDeleteCredential(tracer, this.repoUrl, username, password, out string error)) + if (!this.credentialStore.TryDeleteCredential(tracer, this.repoUrl, username, password, out string error, credentialTimeoutMs)) { // Deleting credentials is best effort attempt - log failure, but do not fail tracer.RelatedWarning("Failed to delete credential string: {0}", error); @@ -138,7 +145,7 @@ public void RejectCredentials(ITracer tracer, string credentialString) ["RepoUrl"] = this.repoUrl, }); tracer.RelatedWarning(metadata, "Failed to parse credential string for rejection. Rejecting any credential for this repo URL."); - this.credentialStore.TryDeleteCredential(tracer, this.repoUrl, username: null, password: null, error: out string error); + this.credentialStore.TryDeleteCredential(tracer, this.repoUrl, username: null, password: null, error: out string error, timeoutMs: credentialTimeoutMs); } this.cachedCredentialString = null; @@ -153,7 +160,7 @@ public void RejectCredentials(ITracer tracer, string credentialString) } } - public bool TryGetCredentials(ITracer tracer, out string credentialString, out string errorMessage) + public bool TryGetCredentials(ITracer tracer, out string credentialString, out string errorMessage, int credentialTimeoutMs = DefaultCredentialTimeoutMs) { if (!this.isInitialized) { @@ -173,7 +180,7 @@ public bool TryGetCredentials(ITracer tracer, out string credentialString, out s return false; } - if (!this.TryCallGitCredential(tracer, out errorMessage)) + if (!this.TryCallGitCredential(tracer, out errorMessage, credentialTimeoutMs)) { return false; } @@ -390,10 +397,13 @@ private bool TryCallGitCredential(ITracer tracer, out string errorMessage, int t // Serialize credential fetches so only one git-credential-fill // process runs at a time. Without this, a background auth task // and a foreground object download could both spawn GCM prompts. - // Wait up to 60s for an in-flight fetch; if the gate is still - // held (e.g., background GCM prompt), fall through and let this - // caller spawn its own credential fetch. - bool acquired = this.credentialGate.Wait(60_000); + // Wait at least as long as the fetch itself may take; otherwise the + // gate would expire while the in-flight fetch is still legitimately + // waiting on the user, and we would fall through and spawn a second + // competing GCM prompt in exactly the slow-prompt case this bound + // exists to tolerate. + int gateTimeoutMs = timeoutMs < 0 ? DefaultCredentialGateWaitMs : Math.Max(DefaultCredentialGateWaitMs, timeoutMs); + bool acquired = this.credentialGate.Wait(gateTimeoutMs); try { string gitUsername; diff --git a/GVFS/GVFS.Common/Git/GitProcess.cs b/GVFS/GVFS.Common/Git/GitProcess.cs index 03cc27b417..158cde9dcb 100644 --- a/GVFS/GVFS.Common/Git/GitProcess.cs +++ b/GVFS/GVFS.Common/Git/GitProcess.cs @@ -37,6 +37,12 @@ public class GitProcess : ICredentialStore /// private const int MaxCapturedStdOutChars = 128 * 1024 * 1024; // ~256 MB of UTF-16 + /// + /// How long to wait for a killed process tree to actually exit before we give up + /// and read whatever the async stdout/stderr readers have captured so far. + /// + private const int ProcessKillTimeoutMs = 5_000; + private static readonly Encoding UTF8NoBOM = new UTF8Encoding(false); private static bool failedToSetEncoding = false; private static string expireTimeDateString; @@ -192,7 +198,7 @@ public bool TryKillRunningProcess(out string processName, out int exitCode, out } } - public virtual bool TryDeleteCredential(ITracer tracer, string repoUrl, string username, string password, out string errorMessage) + public virtual bool TryDeleteCredential(ITracer tracer, string repoUrl, string username, string password, out string errorMessage, int timeoutMs = -1) { StringBuilder sb = new StringBuilder(); sb.AppendFormat("url={0}\n", repoUrl); @@ -214,7 +220,8 @@ public virtual bool TryDeleteCredential(ITracer tracer, string repoUrl, string u GenerateCredentialVerbCommand("reject"), stdin => stdin.Write(stdinConfig), null, - usePreCommandHook: false); + usePreCommandHook: false, + timeoutMs: timeoutMs); if (result.ExitCodeIsFailure) { @@ -228,7 +235,7 @@ public virtual bool TryDeleteCredential(ITracer tracer, string repoUrl, string u return true; } - public virtual bool TryStoreCredential(ITracer tracer, string repoUrl, string username, string password, out string errorMessage) + public virtual bool TryStoreCredential(ITracer tracer, string repoUrl, string username, string password, out string errorMessage, int timeoutMs = -1) { StringBuilder sb = new StringBuilder(); sb.AppendFormat("url={0}\n", repoUrl); @@ -242,7 +249,8 @@ public virtual bool TryStoreCredential(ITracer tracer, string repoUrl, string us GenerateCredentialVerbCommand("approve"), stdin => stdin.Write(stdinConfig), null, - usePreCommandHook: false); + usePreCommandHook: false, + timeoutMs: timeoutMs); if (result.ExitCodeIsFailure) { @@ -1051,7 +1059,14 @@ protected virtual Result InvokeGitImpl( if (!this.executingProcess.WaitForExit(timeoutMs)) { - this.executingProcess.Kill(); + // Kill the entire process tree. Killing only git.exe would leave + // helper children (e.g. an interactive credential manager prompt) + // running, holding the credential store and showing orphaned UI. + this.executingProcess.Kill(entireProcessTree: true); + + // Give the tree a bounded chance to actually exit so the async + // stdout/stderr readers flush before we read their buffers. + this.executingProcess.WaitForExit(ProcessKillTimeoutMs); return new Result(output.ToString(), "Operation timed out: " + errors.ToString(), Result.GenericFailureCode, output.Truncated, errors.Truncated); } diff --git a/GVFS/GVFS.Common/Git/ICredentialStore.cs b/GVFS/GVFS.Common/Git/ICredentialStore.cs index 9ab38e13d9..2ab8c137cb 100644 --- a/GVFS/GVFS.Common/Git/ICredentialStore.cs +++ b/GVFS/GVFS.Common/Git/ICredentialStore.cs @@ -6,8 +6,8 @@ public interface ICredentialStore { bool TryGetCredential(ITracer tracer, string url, out string username, out string password, out string error, int timeoutMs = -1); - bool TryStoreCredential(ITracer tracer, string url, string username, string password, out string error); + bool TryStoreCredential(ITracer tracer, string url, string username, string password, out string error, int timeoutMs = -1); - bool TryDeleteCredential(ITracer tracer, string url, string username, string password, out string error); + bool TryDeleteCredential(ITracer tracer, string url, string username, string password, out string error, int timeoutMs = -1); } } diff --git a/GVFS/GVFS.Common/Http/HttpRequestor.cs b/GVFS/GVFS.Common/Http/HttpRequestor.cs index 0f9767dde1..9dbc86dd4e 100644 --- a/GVFS/GVFS.Common/Http/HttpRequestor.cs +++ b/GVFS/GVFS.Common/Http/HttpRequestor.cs @@ -84,6 +84,16 @@ protected HttpRequestor(ITracer tracer, RetryConfig retryConfig, Enlistment enli protected ITracer Tracer { get; } + // Runtime credential fetches (object/pack downloads, incl. background + // maintenance prefetch) are bounded so a missed/ignored credential prompt + // can't hang forever. We use the generous BackgroundCredentialTimeoutMs + // (120s) rather than the 30s default: this same requestor is shared by + // interactive on-demand hydration and by 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 that a noticed prompt is not cut off spuriously. + protected virtual int CredentialTimeoutMs => GitAuthentication.BackgroundCredentialTimeoutMs; + public static long GetNewRequestId() { return Interlocked.Increment(ref requestCount); @@ -109,7 +119,7 @@ protected GitEndPointResponseData SendRequest( string authString = null; string errorMessage; if (!this.authentication.IsAnonymous && - !this.authentication.TryGetCredentials(this.Tracer, out authString, out errorMessage)) + !this.authentication.TryGetCredentials(this.Tracer, out authString, out errorMessage, this.CredentialTimeoutMs)) { return new GitEndPointResponseData( HttpStatusCode.Unauthorized, @@ -207,7 +217,7 @@ protected GitEndPointResponseData SendRequest( if (!this.authentication.IsAnonymous) { - this.authentication.ApproveCredentials(this.Tracer, authString); + this.authentication.ApproveCredentials(this.Tracer, authString, this.CredentialTimeoutMs); } Stream responseStream = response.Content.ReadAsStreamAsync().GetAwaiter().GetResult(); @@ -234,7 +244,7 @@ protected GitEndPointResponseData SendRequest( } else if (response.StatusCode == HttpStatusCode.Unauthorized || response.StatusCode == HttpStatusCode.BadRequest || response.StatusCode == HttpStatusCode.Redirect) { - this.authentication.RejectCredentials(this.Tracer, authString); + this.authentication.RejectCredentials(this.Tracer, authString, this.CredentialTimeoutMs); if (!this.authentication.IsBackingOff) { errorMessage = string.Format("Server returned error code {0} ({1}). Your PAT may be expired and we are asking for a new one. Original error message from server: {2}", statusInt, response.StatusCode, errorMessage); diff --git a/GVFS/GVFS.UnitTests/Git/GitAuthenticationTests.cs b/GVFS/GVFS.UnitTests/Git/GitAuthenticationTests.cs index c083ac5862..efae03c590 100644 --- a/GVFS/GVFS.UnitTests/Git/GitAuthenticationTests.cs +++ b/GVFS/GVFS.UnitTests/Git/GitAuthenticationTests.cs @@ -273,6 +273,78 @@ public void RejectionShouldNotBeSentIfUnderlyingTokenHasChanged() gitProcess.CredentialRejections.ShouldBeEmpty(); } + [TestCase] + public void TryGetCredentialsTimesOutWhenCredentialManagerDoesNotRespond() + { + MockTracer tracer = new MockTracer(); + MockGitProcess gitProcess = this.GetGitProcess(); + + GitAuthentication dut = new GitAuthentication(gitProcess, "mock://repoUrl"); + dut.TryInitializeAndRequireAuth(tracer, out _); + + string authString; + string err; + dut.TryGetCredentials(tracer, out authString, out err).ShouldEqual(true, "Initial credential fetch should succeed: " + err); + + // Override the fill command to simulate a credential manager timeout + gitProcess.SetExpectedCommandResult( + $"{AzureDevOpsUseHttpPathString} credential fill", + () => new GitProcess.Result(string.Empty, "Operation timed out: git credential fill", GitProcess.Result.GenericFailureCode), + matchPrefix: true); + + // Reject clears the cache so the next TryGetCredentials must refetch + dut.RejectCredentials(tracer, authString); + + // The re-fetch should time out + dut.TryGetCredentials(tracer, out authString, out err, credentialTimeoutMs: 1000).ShouldEqual(false, "Expected timeout to cause failure"); + err.ShouldContain("did not respond"); + + // Assert the bound was actually plumbed all the way down to the git invocation. + // Without this the test would still pass with the timeout plumbing reverted, because + // GitProcess maps any "Operation timed out" stderr to a "did not respond" message + // (with timeoutMs = -1 that renders as "within 0 seconds", which also matches above). + gitProcess.LastInvokedTimeoutMs.ShouldEqual(1000, "Expected the credential timeout to reach InvokeGitImpl"); + err.ShouldContain("within 1 seconds"); + } + + [TestCase] + public void RejectCredentialsBoundsTheCredentialReload() + { + MockTracer tracer = new MockTracer(); + MockGitProcess gitProcess = this.GetGitProcess(); + + GitAuthentication dut = new GitAuthentication(gitProcess, "mock://repoUrl"); + dut.TryInitializeAndRequireAuth(tracer, out _); + + string authString; + string err; + dut.TryGetCredentials(tracer, out authString, out err).ShouldEqual(true, "Initial credential fetch should succeed: " + err); + + // The 401-retry leg reloads the credential and then erases it. Both legs spawn a git + // process, and both must honor the caller's bound rather than waiting forever. + gitProcess.InvokedTimeoutMs.Clear(); + dut.RejectCredentials(tracer, authString, credentialTimeoutMs: 1000); + + gitProcess.InvokedTimeoutMs.Count.ShouldEqual(2, "Expected RejectCredentials to reload and then erase the credential"); + gitProcess.InvokedTimeoutMs.ShouldNotContain(timeout => timeout < 0); + gitProcess.LastInvokedTimeoutMs.ShouldEqual(1000, "Expected the credential erase to be bounded too"); + } + + [TestCase] + public void TryGetCredentialsSucceedsWithExplicitTimeout() + { + MockTracer tracer = new MockTracer(); + MockGitProcess gitProcess = this.GetGitProcess(); + + GitAuthentication dut = new GitAuthentication(gitProcess, "mock://repoUrl"); + dut.TryInitializeAndRequireAuth(tracer, out _); + + string cred; + string err; + dut.TryGetCredentials(tracer, out cred, out err, credentialTimeoutMs: 30000).ShouldEqual(true, "Expected success with explicit timeout: " + err); + cred.ShouldNotBeNull(); + } + private MockGitProcess GetGitProcess() { MockGitProcess gitProcess = new MockGitProcess(); diff --git a/GVFS/GVFS.UnitTests/Mock/Git/MockGitProcess.cs b/GVFS/GVFS.UnitTests/Mock/Git/MockGitProcess.cs index c9095cc2c8..69cad652fc 100644 --- a/GVFS/GVFS.UnitTests/Mock/Git/MockGitProcess.cs +++ b/GVFS/GVFS.UnitTests/Mock/Git/MockGitProcess.cs @@ -18,12 +18,26 @@ public MockGitProcess() : base(new MockGVFSEnlistment()) { this.CommandsRun = new List(); + this.InvokedTimeoutMs = new List(); + this.LastInvokedTimeoutMs = null; this.StoredCredentials = new Dictionary(StringComparer.OrdinalIgnoreCase); this.CredentialApprovals = new Dictionary>(); this.CredentialRejections = new Dictionary>(); } public List CommandsRun { get; } + + /// + /// The timeout passed to every InvokeGitImpl call, in order. Lets tests assert that a + /// caller actually plumbed a finite timeout rather than defaulting to -1 (infinite). + /// + public List InvokedTimeoutMs { get; } + + /// + /// The timeout passed to the most recent InvokeGitImpl call, or null if none has run. + /// + public int? LastInvokedTimeoutMs { get; private set; } + public bool ShouldFail { get; set; } public Dictionary StoredCredentials { get; } public Dictionary> CredentialApprovals { get; } @@ -35,7 +49,7 @@ public void SetExpectedCommandResult(string command, Func result, bool m this.expectedCommandInfos.Add(commandInfo); } - public override bool TryStoreCredential(ITracer tracer, string repoUrl, string username, string password, out string error) + public override bool TryStoreCredential(ITracer tracer, string repoUrl, string username, string password, out string error, int timeoutMs = -1) { Credential credential = new Credential(username, password); @@ -52,10 +66,10 @@ public override bool TryStoreCredential(ITracer tracer, string repoUrl, string u // Store the credential this.StoredCredentials[repoUrl] = credential; - return base.TryStoreCredential(tracer, repoUrl, username, password, out error); + return base.TryStoreCredential(tracer, repoUrl, username, password, out error, timeoutMs); } - public override bool TryDeleteCredential(ITracer tracer, string repoUrl, string username, string password, out string error) + public override bool TryDeleteCredential(ITracer tracer, string repoUrl, string username, string password, out string error, int timeoutMs = -1) { Credential credential = new Credential(username, password); @@ -72,7 +86,7 @@ public override bool TryDeleteCredential(ITracer tracer, string repoUrl, string // Erase the credential this.StoredCredentials.Remove(repoUrl); - return base.TryDeleteCredential(tracer, repoUrl, username, password, out error); + return base.TryDeleteCredential(tracer, repoUrl, username, password, out error, timeoutMs); } protected override Result InvokeGitImpl( @@ -87,6 +101,8 @@ protected override Result InvokeGitImpl( bool usePrecommandHook = true) { this.CommandsRun.Add(command); + this.LastInvokedTimeoutMs = timeoutMs; + this.InvokedTimeoutMs.Add(timeoutMs); if (this.ShouldFail) { From c2b26d1a6f2e4813ad4d38e74bf8305351df66e0 Mon Sep 17 00:00:00 2001 From: Tyrie Vella Date: Tue, 11 Aug 2026 09:09:06 -0700 Subject: [PATCH 2/2] Stop holding the HTTP pool slot across credential work; thread cancellation 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> --- GVFS/GVFS.Common/GVFSConstants.cs | 6 + GVFS/GVFS.Common/Git/GitAuthentication.cs | 31 ++- GVFS/GVFS.Common/Git/GitProcess.cs | 80 ++++++- GVFS/GVFS.Common/Git/ICredentialStore.cs | 7 +- GVFS/GVFS.Common/Http/HttpRequestor.cs | 142 +++++++++-- .../Git/GitAuthenticationTests.cs | 119 ++++++++++ .../GVFS.UnitTests/Http/HttpRequestorTests.cs | 223 ++++++++++++++++++ .../Mock/Common/MockGVFSEnlistment.cs | 9 + .../GVFS.UnitTests/Mock/Git/MockGitProcess.cs | 53 ++++- 9 files changed, 620 insertions(+), 50 deletions(-) create mode 100644 GVFS/GVFS.UnitTests/Http/HttpRequestorTests.cs diff --git a/GVFS/GVFS.Common/GVFSConstants.cs b/GVFS/GVFS.Common/GVFSConstants.cs index 8f135786aa..0460b03a4d 100644 --- a/GVFS/GVFS.Common/GVFSConstants.cs +++ b/GVFS/GVFS.Common/GVFSConstants.cs @@ -51,6 +51,12 @@ public static class GitConfig public const string PrefetchUseIdx = GVFSPrefix + "prefetch-use-idx"; public const bool PrefetchUseIdxDefault = false; + + /* Off-by-default flag (stabilization ship). When enabled, HttpRequestor releases + * its process-wide connection-pool slot before running the credential-reject leg on + * a 401, so a slow credential helper cannot starve healthy parallel requests. */ + public const string ReleaseConnectionBeforeCredentialReject = GVFSPrefix + "release-connection-before-credential-reject"; + public const bool ReleaseConnectionBeforeCredentialRejectDefault = false; } public static class LocalGVFSConfig diff --git a/GVFS/GVFS.Common/Git/GitAuthentication.cs b/GVFS/GVFS.Common/Git/GitAuthentication.cs index ef00bf719c..4a59881fea 100644 --- a/GVFS/GVFS.Common/Git/GitAuthentication.cs +++ b/GVFS/GVFS.Common/Git/GitAuthentication.cs @@ -59,7 +59,16 @@ public bool IsBackingOff private GitSsl GitSsl { get; } - public void ApproveCredentials(ITracer tracer, string credentialString, int credentialTimeoutMs = DefaultCredentialTimeoutMs) + /// + /// Test-only hook to force the anonymous state. Production code determines this + /// by probing the server in . + /// + internal void SetIsAnonymousForTesting(bool isAnonymous) + { + this.IsAnonymous = isAnonymous; + } + + public void ApproveCredentials(ITracer tracer, string credentialString, int credentialTimeoutMs = DefaultCredentialTimeoutMs, CancellationToken cancellationToken = default) { lock (this.gitAuthLock) { @@ -77,7 +86,7 @@ public void ApproveCredentials(ITracer tracer, string credentialString, int cred string password; if (TryParseCredentialString(this.cachedCredentialString, out username, out password)) { - if (!this.credentialStore.TryStoreCredential(tracer, this.repoUrl, username, password, out string error, credentialTimeoutMs)) + if (!this.credentialStore.TryStoreCredential(tracer, this.repoUrl, username, password, out string error, credentialTimeoutMs, cancellationToken)) { // Storing credentials is best effort attempt - log failure, but do not fail tracer.RelatedWarning("Failed to store credential string: {0}", error); @@ -98,7 +107,7 @@ public void ApproveCredentials(ITracer tracer, string credentialString, int cred } } - public void RejectCredentials(ITracer tracer, string credentialString, int credentialTimeoutMs = DefaultCredentialTimeoutMs) + public void RejectCredentials(ITracer tracer, string credentialString, int credentialTimeoutMs = DefaultCredentialTimeoutMs, CancellationToken cancellationToken = default) { lock (this.gitAuthLock) { @@ -109,7 +118,7 @@ public void RejectCredentials(ITracer tracer, string credentialString, int crede // We can't assume that the credential store's cached credential is the same as the one we have. // Reload the credential from the store to ensure we're rejecting the correct one. int attemptsBeforeCheckingExistingCredential = this.numberOfAttempts; - if (this.TryCallGitCredential(tracer, out string getCredentialError, credentialTimeoutMs)) + if (this.TryCallGitCredential(tracer, out string getCredentialError, credentialTimeoutMs, cancellationToken)) { if (this.cachedCredentialString != cachedCredentialAtStartOfReject) { @@ -130,7 +139,7 @@ public void RejectCredentials(ITracer tracer, string credentialString, int crede string password; if (TryParseCredentialString(this.cachedCredentialString, out username, out password)) { - if (!this.credentialStore.TryDeleteCredential(tracer, this.repoUrl, username, password, out string error, credentialTimeoutMs)) + if (!this.credentialStore.TryDeleteCredential(tracer, this.repoUrl, username, password, out string error, credentialTimeoutMs, cancellationToken)) { // Deleting credentials is best effort attempt - log failure, but do not fail tracer.RelatedWarning("Failed to delete credential string: {0}", error); @@ -145,7 +154,7 @@ public void RejectCredentials(ITracer tracer, string credentialString, int crede ["RepoUrl"] = this.repoUrl, }); tracer.RelatedWarning(metadata, "Failed to parse credential string for rejection. Rejecting any credential for this repo URL."); - this.credentialStore.TryDeleteCredential(tracer, this.repoUrl, username: null, password: null, error: out string error, timeoutMs: credentialTimeoutMs); + this.credentialStore.TryDeleteCredential(tracer, this.repoUrl, username: null, password: null, error: out string error, timeoutMs: credentialTimeoutMs, cancellationToken: cancellationToken); } this.cachedCredentialString = null; @@ -160,7 +169,7 @@ public void RejectCredentials(ITracer tracer, string credentialString, int crede } } - public bool TryGetCredentials(ITracer tracer, out string credentialString, out string errorMessage, int credentialTimeoutMs = DefaultCredentialTimeoutMs) + public bool TryGetCredentials(ITracer tracer, out string credentialString, out string errorMessage, int credentialTimeoutMs = DefaultCredentialTimeoutMs, CancellationToken cancellationToken = default) { if (!this.isInitialized) { @@ -180,7 +189,7 @@ public bool TryGetCredentials(ITracer tracer, out string credentialString, out s return false; } - if (!this.TryCallGitCredential(tracer, out errorMessage, credentialTimeoutMs)) + if (!this.TryCallGitCredential(tracer, out errorMessage, credentialTimeoutMs, cancellationToken)) { return false; } @@ -392,7 +401,7 @@ private void UpdateBackoff() this.numberOfAttempts++; } - private bool TryCallGitCredential(ITracer tracer, out string errorMessage, int timeoutMs = -1) + private bool TryCallGitCredential(ITracer tracer, out string errorMessage, int timeoutMs = -1, CancellationToken cancellationToken = default) { // Serialize credential fetches so only one git-credential-fill // process runs at a time. Without this, a background auth task @@ -403,12 +412,12 @@ private bool TryCallGitCredential(ITracer tracer, out string errorMessage, int t // competing GCM prompt in exactly the slow-prompt case this bound // exists to tolerate. int gateTimeoutMs = timeoutMs < 0 ? DefaultCredentialGateWaitMs : Math.Max(DefaultCredentialGateWaitMs, timeoutMs); - bool acquired = this.credentialGate.Wait(gateTimeoutMs); + bool acquired = this.credentialGate.Wait(gateTimeoutMs, cancellationToken); try { string gitUsername; string gitPassword; - if (!this.credentialStore.TryGetCredential(tracer, this.repoUrl, out gitUsername, out gitPassword, out errorMessage, timeoutMs)) + if (!this.credentialStore.TryGetCredential(tracer, this.repoUrl, out gitUsername, out gitPassword, out errorMessage, timeoutMs, cancellationToken)) { this.UpdateBackoff(); return false; diff --git a/GVFS/GVFS.Common/Git/GitProcess.cs b/GVFS/GVFS.Common/Git/GitProcess.cs index 158cde9dcb..c9efa6cb9c 100644 --- a/GVFS/GVFS.Common/Git/GitProcess.cs +++ b/GVFS/GVFS.Common/Git/GitProcess.cs @@ -198,7 +198,7 @@ public bool TryKillRunningProcess(out string processName, out int exitCode, out } } - public virtual bool TryDeleteCredential(ITracer tracer, string repoUrl, string username, string password, out string errorMessage, int timeoutMs = -1) + public virtual bool TryDeleteCredential(ITracer tracer, string repoUrl, string username, string password, out string errorMessage, int timeoutMs = -1, CancellationToken cancellationToken = default) { StringBuilder sb = new StringBuilder(); sb.AppendFormat("url={0}\n", repoUrl); @@ -221,7 +221,8 @@ public virtual bool TryDeleteCredential(ITracer tracer, string repoUrl, string u stdin => stdin.Write(stdinConfig), null, usePreCommandHook: false, - timeoutMs: timeoutMs); + timeoutMs: timeoutMs, + cancellationToken: cancellationToken); if (result.ExitCodeIsFailure) { @@ -235,7 +236,7 @@ public virtual bool TryDeleteCredential(ITracer tracer, string repoUrl, string u return true; } - public virtual bool TryStoreCredential(ITracer tracer, string repoUrl, string username, string password, out string errorMessage, int timeoutMs = -1) + public virtual bool TryStoreCredential(ITracer tracer, string repoUrl, string username, string password, out string errorMessage, int timeoutMs = -1, CancellationToken cancellationToken = default) { StringBuilder sb = new StringBuilder(); sb.AppendFormat("url={0}\n", repoUrl); @@ -250,7 +251,8 @@ public virtual bool TryStoreCredential(ITracer tracer, string repoUrl, string us stdin => stdin.Write(stdinConfig), null, usePreCommandHook: false, - timeoutMs: timeoutMs); + timeoutMs: timeoutMs, + cancellationToken: cancellationToken); if (result.ExitCodeIsFailure) { @@ -328,7 +330,8 @@ public virtual bool TryGetCredential( out string username, out string password, out string errorMessage, - int timeoutMs = -1) + int timeoutMs = -1, + CancellationToken cancellationToken = default) { username = null; password = null; @@ -343,7 +346,8 @@ public virtual bool TryGetCredential( stdin => stdin.Write($"url={repoUrl}\n\n"), parseStdOutLine: null, usePreCommandHook: false, - timeoutMs: timeoutMs); + timeoutMs: timeoutMs, + cancellationToken: cancellationToken); if (gitCredentialOutput.ExitCodeIsFailure) { @@ -983,7 +987,8 @@ protected virtual Result InvokeGitImpl( Action parseStdOutLine, int timeoutMs, string gitObjectsDirectory = null, - bool usePreCommandHook = true) + bool usePreCommandHook = true, + CancellationToken cancellationToken = default) { if (failedToSetEncoding && writeStdIn != null) { @@ -1057,7 +1062,12 @@ protected virtual Result InvokeGitImpl( this.executingProcess.BeginOutputReadLine(); this.executingProcess.BeginErrorReadLine(); - if (!this.executingProcess.WaitForExit(timeoutMs)) + bool cancellationRequested = false; + bool exited = cancellationToken.CanBeCanceled + ? this.WaitForExitWithCancellation(timeoutMs, cancellationToken, out cancellationRequested) + : this.executingProcess.WaitForExit(timeoutMs); + + if (!exited) { // Kill the entire process tree. Killing only git.exe would leave // helper children (e.g. an interactive credential manager prompt) @@ -1068,6 +1078,14 @@ protected virtual Result InvokeGitImpl( // stdout/stderr readers flush before we read their buffers. this.executingProcess.WaitForExit(ProcessKillTimeoutMs); + if (cancellationRequested) + { + // The caller (e.g. mount shutdown or a cancelled request) asked us + // to stop. Surface cancellation rather than a timeout so callers such + // as RetryWrapper abort promptly instead of retrying the operation. + throw new OperationCanceledException(cancellationToken); + } + return new Result(output.ToString(), "Operation timed out: " + errors.ToString(), Result.GenericFailureCode, output.Truncated, errors.Truncated); } } @@ -1085,6 +1103,46 @@ protected virtual Result InvokeGitImpl( } } + /// + /// Waits for the currently executing git process to exit, giving up when the + /// timeout elapses or the caller cancels. Polls at a short interval so cancellation + /// (e.g. mount shutdown) is observed promptly even though + /// has no cancellation-aware overload. + /// + /// True if the process exited on its own; false if it must be killed. + private bool WaitForExitWithCancellation(int timeoutMs, CancellationToken cancellationToken, out bool cancellationRequested) + { + const int PollIntervalMs = 100; + cancellationRequested = false; + + Stopwatch stopwatch = Stopwatch.StartNew(); + while (true) + { + int waitMs = PollIntervalMs; + if (timeoutMs >= 0) + { + long remainingMs = timeoutMs - stopwatch.ElapsedMilliseconds; + if (remainingMs <= 0) + { + return false; + } + + waitMs = (int)Math.Min(PollIntervalMs, remainingMs); + } + + if (this.executingProcess.WaitForExit(waitMs)) + { + return true; + } + + if (cancellationToken.IsCancellationRequested) + { + cancellationRequested = true; + return false; + } + } + } + private static string GenerateCredentialVerbCommand(string verb) { return $"-c {GitConfigSetting.CredentialUseHttpPath}=true credential {verb}"; @@ -1170,7 +1228,8 @@ private Result InvokeGitAgainstDotGitFolder( Action parseStdOutLine, bool usePreCommandHook = true, string gitObjectsDirectory = null, - int timeoutMs = -1) + int timeoutMs = -1, + CancellationToken cancellationToken = default) { // This git command should not need/use the working directory of the repo. // Run git.exe in Environment.SystemDirectory to ensure the git.exe process @@ -1184,7 +1243,8 @@ private Result InvokeGitAgainstDotGitFolder( parseStdOutLine: parseStdOutLine, timeoutMs: timeoutMs, gitObjectsDirectory: gitObjectsDirectory, - usePreCommandHook: usePreCommandHook); + usePreCommandHook: usePreCommandHook, + cancellationToken: cancellationToken); } public class Result diff --git a/GVFS/GVFS.Common/Git/ICredentialStore.cs b/GVFS/GVFS.Common/Git/ICredentialStore.cs index 2ab8c137cb..684a874c95 100644 --- a/GVFS/GVFS.Common/Git/ICredentialStore.cs +++ b/GVFS/GVFS.Common/Git/ICredentialStore.cs @@ -1,13 +1,14 @@ using GVFS.Common.Tracing; +using System.Threading; namespace GVFS.Common.Git { public interface ICredentialStore { - bool TryGetCredential(ITracer tracer, string url, out string username, out string password, out string error, int timeoutMs = -1); + bool TryGetCredential(ITracer tracer, string url, out string username, out string password, out string error, int timeoutMs = -1, CancellationToken cancellationToken = default); - bool TryStoreCredential(ITracer tracer, string url, string username, string password, out string error, int timeoutMs = -1); + bool TryStoreCredential(ITracer tracer, string url, string username, string password, out string error, int timeoutMs = -1, CancellationToken cancellationToken = default); - bool TryDeleteCredential(ITracer tracer, string url, string username, string password, out string error, int timeoutMs = -1); + bool TryDeleteCredential(ITracer tracer, string url, string username, string password, out string error, int timeoutMs = -1, CancellationToken cancellationToken = default); } } diff --git a/GVFS/GVFS.Common/Http/HttpRequestor.cs b/GVFS/GVFS.Common/Http/HttpRequestor.cs index 9dbc86dd4e..5e5e145378 100644 --- a/GVFS/GVFS.Common/Http/HttpRequestor.cs +++ b/GVFS/GVFS.Common/Http/HttpRequestor.cs @@ -22,6 +22,7 @@ public abstract class HttpRequestor : IDisposable private static long requestCount = 0; private static SemaphoreSlim availableConnections; private static int connectionLimitConfigured = 0; + private static bool releaseConnectionBeforeCredentialReject = GVFSConstants.GitConfig.ReleaseConnectionBeforeCredentialRejectDefault; private readonly ProductInfoHeaderValue userAgentHeader; @@ -38,6 +39,17 @@ static HttpRequestor() } protected HttpRequestor(ITracer tracer, RetryConfig retryConfig, Enlistment enlistment) + : this(tracer, retryConfig, enlistment, handlerOverride: null) + { + } + + /// + /// Test-only constructor that injects a custom so + /// can be exercised without real network I/O. Production code + /// uses the parameterless-handler overload, which builds a configured + /// . + /// + internal HttpRequestor(ITracer tracer, RetryConfig retryConfig, Enlistment enlistment, HttpMessageHandler handlerOverride) { this.RetryConfig = retryConfig; @@ -50,6 +62,7 @@ protected HttpRequestor(ITracer tracer, RetryConfig retryConfig, Enlistment enli if (Interlocked.CompareExchange(ref connectionLimitConfigured, 1, 0) == 0) { TryApplyConnectionLimitFromConfig(tracer, enlistment); + TryApplyReleaseConnectionBeforeRejectFromConfig(tracer, enlistment); } // WARNING: Do NOT set Credentials or ServerCredentials on this handler. @@ -61,14 +74,23 @@ protected HttpRequestor(ITracer tracer, RetryConfig retryConfig, Enlistment enli // GVFS cache servers and Azure DevOps accept PAT/OAuth tokens via the // "Authorization: Basic " header that SendRequest already attaches. // Transport-level credentials are redundant and purely wasteful. - SocketsHttpHandler handler = new SocketsHttpHandler() + HttpMessageHandler handler; + if (handlerOverride != null) { - MaxConnectionsPerServer = Environment.ProcessorCount, - PooledConnectionLifetime = Timeout.InfiniteTimeSpan, - PooledConnectionIdleTimeout = TimeSpan.FromMinutes(5), - }; + handler = handlerOverride; + } + else + { + SocketsHttpHandler socketsHandler = new SocketsHttpHandler() + { + MaxConnectionsPerServer = Environment.ProcessorCount, + PooledConnectionLifetime = Timeout.InfiniteTimeSpan, + PooledConnectionIdleTimeout = TimeSpan.FromMinutes(5), + }; - this.authentication.ConfigureSocketsHandlerSslIfNeeded(this.Tracer, handler, enlistment.CreateGitProcess()); + this.authentication.ConfigureSocketsHandlerSslIfNeeded(this.Tracer, socketsHandler, enlistment.CreateGitProcess()); + handler = socketsHandler; + } this.client = new HttpClient(handler) { @@ -84,6 +106,20 @@ protected HttpRequestor(ITracer tracer, RetryConfig retryConfig, Enlistment enli protected ITracer Tracer { get; } + /// + /// Number of currently-available connection-pool permits. Test-only observability hook + /// for asserting that releases its slot at the right time. + /// + internal static int AvailableConnectionCount => availableConnections.CurrentCount; + + /// + /// When true, releases the connection-pool slot before running + /// the (potentially slow) credential-reject leg on a 401. Off by default; enabled via + /// . Overridable + /// in tests. + /// + protected virtual bool ShouldReleaseConnectionBeforeCredentialReject => releaseConnectionBeforeCredentialReject; + // Runtime credential fetches (object/pack downloads, incl. background // maintenance prefetch) are bounded so a missed/ignored credential prompt // can't hang forever. We use the generous BackgroundCredentialTimeoutMs @@ -119,7 +155,7 @@ protected GitEndPointResponseData SendRequest( string authString = null; string errorMessage; if (!this.authentication.IsAnonymous && - !this.authentication.TryGetCredentials(this.Tracer, out authString, out errorMessage, this.CredentialTimeoutMs)) + !this.authentication.TryGetCredentials(this.Tracer, out authString, out errorMessage, this.CredentialTimeoutMs, cancellationToken)) { return new GitEndPointResponseData( HttpStatusCode.Unauthorized, @@ -186,6 +222,11 @@ protected GitEndPointResponseData SendRequest( GitEndPointResponseData gitEndPointResponseData = null; HttpResponseMessage response = null; + // Tracks whether we already released the connection-pool slot inside the try body + // (F06 early-release before the credential-reject leg). The finally block must not + // release a second time, which would corrupt the semaphore's permit count. + bool connectionReleasedEarly = false; + try { requestStopwatch.Restart(); @@ -217,7 +258,7 @@ protected GitEndPointResponseData SendRequest( if (!this.authentication.IsAnonymous) { - this.authentication.ApproveCredentials(this.Tracer, authString, this.CredentialTimeoutMs); + this.authentication.ApproveCredentials(this.Tracer, authString, this.CredentialTimeoutMs, cancellationToken); } Stream responseStream = response.Content.ReadAsStreamAsync().GetAwaiter().GetResult(); @@ -232,39 +273,54 @@ protected GitEndPointResponseData SendRequest( else { errorMessage = response.Content.ReadAsStringAsync().GetAwaiter().GetResult(); - int statusInt = (int)response.StatusCode; + HttpStatusCode statusCode = response.StatusCode; + int statusInt = (int)statusCode; - bool shouldRetry = ShouldRetry(response.StatusCode); + bool shouldRetry = ShouldRetry(statusCode); - if (response.StatusCode == HttpStatusCode.Unauthorized && + if (statusCode == HttpStatusCode.Unauthorized && this.authentication.IsAnonymous) { shouldRetry = false; errorMessage = "Anonymous request was rejected with a 401"; } - else if (response.StatusCode == HttpStatusCode.Unauthorized || response.StatusCode == HttpStatusCode.BadRequest || response.StatusCode == HttpStatusCode.Redirect) + else if (statusCode == HttpStatusCode.Unauthorized || statusCode == HttpStatusCode.BadRequest || statusCode == HttpStatusCode.Redirect) { - this.authentication.RejectCredentials(this.Tracer, authString, this.CredentialTimeoutMs); + if (this.ShouldReleaseConnectionBeforeCredentialReject) + { + // F06: the error body is already buffered into errorMessage, and the + // reject leg can block for a long time on a slow or hung credential + // helper. Free the process-wide connection slot before that wait so + // healthy parallel requests are not starved by credential contention. + // The finally block honors connectionReleasedEarly so a reject that + // throws (e.g. on cancellation) does not double-release the permit. + response.Dispose(); + response = null; + availableConnections.Release(); + connectionReleasedEarly = true; + } + + this.authentication.RejectCredentials(this.Tracer, authString, this.CredentialTimeoutMs, cancellationToken); if (!this.authentication.IsBackingOff) { - errorMessage = string.Format("Server returned error code {0} ({1}). Your PAT may be expired and we are asking for a new one. Original error message from server: {2}", statusInt, response.StatusCode, errorMessage); + errorMessage = string.Format("Server returned error code {0} ({1}). Your PAT may be expired and we are asking for a new one. Original error message from server: {2}", statusInt, statusCode, errorMessage); } else { - errorMessage = string.Format("Server returned error code {0} ({1}) after successfully renewing your PAT. You may not have access to this repo. Original error message from server: {2}", statusInt, response.StatusCode, errorMessage); + errorMessage = string.Format("Server returned error code {0} ({1}) after successfully renewing your PAT. You may not have access to this repo. Original error message from server: {2}", statusInt, statusCode, errorMessage); } } else { - errorMessage = string.Format("Server returned error code {0} ({1}). Original error message from server: {2}", statusInt, response.StatusCode, errorMessage); + errorMessage = string.Format("Server returned error code {0} ({1}). Original error message from server: {2}", statusInt, statusCode, errorMessage); } gitEndPointResponseData = new GitEndPointResponseData( - response.StatusCode, - new GitObjectsHttpException(response.StatusCode, errorMessage), + statusCode, + new GitObjectsHttpException(statusCode, errorMessage), shouldRetry, - message: response, - onResponseDisposed: () => availableConnections.Release()); + message: connectionReleasedEarly ? null : response, + onResponseDisposed: connectionReleasedEarly ? (Action)null : () => availableConnections.Release()); } } catch (TaskCanceledException) @@ -314,7 +370,12 @@ protected GitEndPointResponseData SendRequest( response.Dispose(); } - availableConnections.Release(); + // Don't release a second time if the connection slot was already freed + // early (F06) before a reject leg that then threw (e.g. on cancellation). + if (!connectionReleasedEarly) + { + availableConnections.Release(); + } } } @@ -440,5 +501,44 @@ private static void TryApplyConnectionLimitFromConfig(ITracer tracer, Enlistment tracer.RelatedWarning(metadata, "HttpRequestor: Failed to read gvfs.max-http-connections config, using default"); } } + + private static void TryApplyReleaseConnectionBeforeRejectFromConfig(ITracer tracer, Enlistment enlistment) + { + try + { + GitProcess.ConfigResult result = enlistment.CreateGitProcess().GetFromConfig(GVFSConstants.GitConfig.ReleaseConnectionBeforeCredentialReject); + if (!result.TryParseAsString(out string value, out string error)) + { + EventMetadata metadata = new EventMetadata(); + metadata.Add("error", error); + tracer.RelatedWarning(metadata, "HttpRequestor: Failed to read gvfs.release-connection-before-credential-reject config, using default"); + return; + } + + if (!string.IsNullOrWhiteSpace(value) && IsGitConfigTrue(value)) + { + releaseConnectionBeforeCredentialReject = true; + + EventMetadata metadata = new EventMetadata(); + metadata.Add("value", value); + tracer.RelatedEvent(EventLevel.Informational, "HttpRequestor_ReleaseConnectionBeforeCredentialRejectEnabled", metadata); + } + } + catch (Exception e) + { + EventMetadata metadata = new EventMetadata(); + metadata.Add("Exception", e.ToString()); + tracer.RelatedWarning(metadata, "HttpRequestor: Failed to read gvfs.release-connection-before-credential-reject config, using default"); + } + } + + private static bool IsGitConfigTrue(string value) + { + // Mirror git's boolean truthiness for config values. + return value.Equals("true", StringComparison.OrdinalIgnoreCase) + || value.Equals("1", StringComparison.Ordinal) + || value.Equals("yes", StringComparison.OrdinalIgnoreCase) + || value.Equals("on", StringComparison.OrdinalIgnoreCase); + } } } diff --git a/GVFS/GVFS.UnitTests/Git/GitAuthenticationTests.cs b/GVFS/GVFS.UnitTests/Git/GitAuthenticationTests.cs index efae03c590..d4823ddc55 100644 --- a/GVFS/GVFS.UnitTests/Git/GitAuthenticationTests.cs +++ b/GVFS/GVFS.UnitTests/Git/GitAuthenticationTests.cs @@ -1,5 +1,6 @@ using System; using System.Linq; +using System.Threading; using GVFS.Common.Git; using GVFS.Tests; using GVFS.Tests.Should; @@ -345,6 +346,124 @@ public void TryGetCredentialsSucceedsWithExplicitTimeout() cred.ShouldNotBeNull(); } + [TestCase] + public void RejectCredentialsPlumbsCancellationTokenToGitProcess() + { + MockTracer tracer = new MockTracer(); + MockGitProcess gitProcess = this.GetGitProcess(); + + GitAuthentication dut = new GitAuthentication(gitProcess, "mock://repoUrl"); + dut.TryInitializeAndRequireAuth(tracer, out _); + + string authString; + dut.TryGetCredentials(tracer, out authString, out _).ShouldBeTrue(); + + using (CancellationTokenSource cts = new CancellationTokenSource()) + { + // The reject leg reloads and then erases the credential; both spawn a git process. + // Assert the caller's token reached the git invocation. Without the plumbing the + // recorded token would be the default (non-cancelable) CancellationToken. + dut.RejectCredentials(tracer, authString, GitAuthentication.DefaultCredentialTimeoutMs, cts.Token); + + gitProcess.LastInvokedCancellationToken.CanBeCanceled.ShouldEqual(true, "Expected the caller's cancellation token to reach the git invocation"); + gitProcess.LastInvokedCancellationToken.ShouldEqual(cts.Token, "Expected the exact caller token to reach the git invocation"); + } + } + + [TestCase] + public void TryGetCredentialsCancellationInterruptsBlockedFetch() + { + MockTracer tracer = new MockTracer(); + MockGitProcess gitProcess = this.GetGitProcess(); + + GitAuthentication dut = new GitAuthentication(gitProcess, "mock://repoUrl"); + dut.TryInitializeAndRequireAuth(tracer, out _); + + string authString; + dut.TryGetCredentials(tracer, out authString, out _).ShouldBeTrue(); + + // Clear the cache so the next TryGetCredentials must re-fetch through git. + dut.RejectCredentials(tracer, authString); + + using (ManualResetEventSlim reached = new ManualResetEventSlim(false)) + using (ManualResetEventSlim block = new ManualResetEventSlim(false)) + using (CancellationTokenSource cts = new CancellationTokenSource()) + { + gitProcess.InvokeReachedBlock = reached; + gitProcess.BlockInvokeUntilSignaled = block; + + Exception caught = null; + Thread worker = new Thread(() => + { + try + { + dut.TryGetCredentials(tracer, out _, out _, GitAuthentication.BackgroundCredentialTimeoutMs, cts.Token); + } + catch (Exception e) + { + caught = e; + } + }); + worker.IsBackground = true; + worker.Start(); + + reached.Wait(TimeSpan.FromSeconds(5)).ShouldEqual(true, "The git credential invocation should have started"); + + // Cancellation must interrupt the in-flight fetch instead of waiting the full + // 120s bound. Without the token reaching InvokeGitImpl the worker blocks forever + // and this Join times out. + cts.Cancel(); + worker.Join(TimeSpan.FromSeconds(5)).ShouldEqual(true, "Cancellation should have unblocked the credential fetch promptly"); + + caught.ShouldNotBeNull("Expected the canceled fetch to throw"); + (caught is OperationCanceledException).ShouldEqual(true, "Expected an OperationCanceledException, got: " + caught); + } + } + + [TestCase] + public void RejectCredentialsCancellationInterruptsBlockedReload() + { + MockTracer tracer = new MockTracer(); + MockGitProcess gitProcess = this.GetGitProcess(); + + GitAuthentication dut = new GitAuthentication(gitProcess, "mock://repoUrl"); + dut.TryInitializeAndRequireAuth(tracer, out _); + + string authString; + dut.TryGetCredentials(tracer, out authString, out _).ShouldBeTrue(); + + using (ManualResetEventSlim reached = new ManualResetEventSlim(false)) + using (ManualResetEventSlim block = new ManualResetEventSlim(false)) + using (CancellationTokenSource cts = new CancellationTokenSource()) + { + gitProcess.InvokeReachedBlock = reached; + gitProcess.BlockInvokeUntilSignaled = block; + + Exception caught = null; + Thread worker = new Thread(() => + { + try + { + dut.RejectCredentials(tracer, authString, GitAuthentication.BackgroundCredentialTimeoutMs, cts.Token); + } + catch (Exception e) + { + caught = e; + } + }); + worker.IsBackground = true; + worker.Start(); + + reached.Wait(TimeSpan.FromSeconds(5)).ShouldEqual(true, "The reject leg should have started a git invocation"); + + cts.Cancel(); + worker.Join(TimeSpan.FromSeconds(5)).ShouldEqual(true, "Cancellation should have unblocked the reject leg promptly"); + + caught.ShouldNotBeNull("Expected the canceled reject to throw"); + (caught is OperationCanceledException).ShouldEqual(true, "Expected an OperationCanceledException, got: " + caught); + } + } + private MockGitProcess GetGitProcess() { MockGitProcess gitProcess = new MockGitProcess(); diff --git a/GVFS/GVFS.UnitTests/Http/HttpRequestorTests.cs b/GVFS/GVFS.UnitTests/Http/HttpRequestorTests.cs new file mode 100644 index 0000000000..f638ccfaec --- /dev/null +++ b/GVFS/GVFS.UnitTests/Http/HttpRequestorTests.cs @@ -0,0 +1,223 @@ +using System; +using System.Net; +using System.Net.Http; +using System.Threading; +using System.Threading.Tasks; +using GVFS.Common; +using GVFS.Common.Git; +using GVFS.Common.Http; +using GVFS.Common.Tracing; +using GVFS.Tests.Should; +using GVFS.UnitTests.Mock.Common; +using GVFS.UnitTests.Mock.Git; +using NUnit.Framework; + +namespace GVFS.UnitTests.Http +{ + [TestFixture] + public class HttpRequestorTests + { + private const string RepoUrl = "mock://repoUrl"; + private const string AzureDevOpsUseHttpPathString = "-c credential.\"https://dev.azure.com\".useHttpPath=true"; + + [TestCase] + public void SendRequestReleasesConnectionBeforeCredentialRejectWhenEnabled() + { + this.RunConnectionReleaseTest(releaseEarly: true, expectReleasedDuringReject: true); + } + + [TestCase] + public void SendRequestHoldsConnectionDuringCredentialRejectWhenDisabled() + { + this.RunConnectionReleaseTest(releaseEarly: false, expectReleasedDuringReject: false); + } + + [TestCase] + public void SendRequestDoesNotDoubleReleaseConnectionWhenRejectCanceled() + { + MockTracer tracer = new MockTracer(); + MockGitProcess gitProcess = CreateGitProcess(); + GitAuthentication authentication = CreateInitializedAuthentication(tracer, gitProcess); + MockGVFSEnlistment enlistment = new MockGVFSEnlistment(gitProcess, authentication); + + using (ManualResetEventSlim reached = new ManualResetEventSlim(false)) + using (ManualResetEventSlim block = new ManualResetEventSlim(false)) + using (CancellationTokenSource cts = new CancellationTokenSource()) + using (StubHttpMessageHandler handler = new StubHttpMessageHandler(HttpStatusCode.Unauthorized, "unauthorized")) + using (TestingHttpRequestor requestor = new TestingHttpRequestor(tracer, new RetryConfig(), enlistment, handler, releaseEarly: true)) + { + int before = HttpRequestor.AvailableConnectionCount; + gitProcess.InvokeReachedBlock = reached; + gitProcess.BlockInvokeUntilSignaled = block; + + Exception caught = null; + Thread worker = new Thread(() => + { + try + { + using (requestor.Send(cts.Token)) + { + } + } + catch (Exception e) + { + caught = e; + } + }); + worker.IsBackground = true; + worker.Start(); + + reached.Wait(TimeSpan.FromSeconds(5)).ShouldEqual(true, "The reject leg should have started a git invocation"); + HttpRequestor.AvailableConnectionCount.ShouldEqual(before, "The connection slot should have been released before the reject leg ran"); + + cts.Cancel(); + worker.Join(TimeSpan.FromSeconds(5)).ShouldEqual(true, "Cancellation should have unblocked the reject leg promptly"); + + caught.ShouldNotBeNull("Expected the canceled request to throw"); + (caught is OperationCanceledException).ShouldEqual(true, "Expected an OperationCanceledException, got: " + caught); + + // The key invariant: the early release plus the canceled reject must not + // double-release the process-wide connection permit. + HttpRequestor.AvailableConnectionCount.ShouldEqual(before, "The connection slot must be released exactly once, not double-released"); + } + } + + private static MockGitProcess CreateGitProcess() + { + MockGitProcess gitProcess = new MockGitProcess(); + gitProcess.SetExpectedCommandResult("config gvfs.FunctionalTests.UserName", () => new GitProcess.Result(string.Empty, string.Empty, GitProcess.Result.GenericFailureCode)); + gitProcess.SetExpectedCommandResult("config gvfs.FunctionalTests.Password", () => new GitProcess.Result(string.Empty, string.Empty, GitProcess.Result.GenericFailureCode)); + gitProcess.SetExpectedCommandResult("config --get-urlmatch http mock://repoUrl", () => new GitProcess.Result(string.Empty, string.Empty, GitProcess.Result.SuccessCode)); + + // HttpRequestor reads these once (on the first instance constructed in the process) + // during its connection-limit / flag initialization. Register them so the read does + // not fault the mock, regardless of test ordering. + gitProcess.SetExpectedCommandResult("config gvfs.max-http-connections", () => new GitProcess.Result(string.Empty, string.Empty, GitProcess.Result.SuccessCode)); + gitProcess.SetExpectedCommandResult("config gvfs.release-connection-before-credential-reject", () => new GitProcess.Result(string.Empty, string.Empty, GitProcess.Result.SuccessCode)); + + int rejections = 0; + gitProcess.SetExpectedCommandResult( + $"{AzureDevOpsUseHttpPathString} credential fill", + () => new GitProcess.Result("username=username\r\npassword=password" + rejections + "\r\n", string.Empty, GitProcess.Result.SuccessCode)); + + gitProcess.SetExpectedCommandResult( + $"{AzureDevOpsUseHttpPathString} credential approve", + () => new GitProcess.Result(string.Empty, string.Empty, GitProcess.Result.SuccessCode)); + + gitProcess.SetExpectedCommandResult( + $"{AzureDevOpsUseHttpPathString} credential reject", + () => + { + rejections++; + return new GitProcess.Result(string.Empty, string.Empty, GitProcess.Result.SuccessCode); + }); + + return gitProcess; + } + + private static GitAuthentication CreateInitializedAuthentication(MockTracer tracer, MockGitProcess gitProcess) + { + GitAuthentication authentication = new GitAuthentication(gitProcess, RepoUrl); + authentication.TryInitializeAndRequireAuth(tracer, out _); + + // Populate the credential cache so SendRequest attaches auth and reaches the reject leg. + authentication.TryGetCredentials(tracer, out _, out _).ShouldBeTrue(); + + // Force the non-anonymous path; production determines this by probing the server. + authentication.SetIsAnonymousForTesting(false); + + return authentication; + } + + private void RunConnectionReleaseTest(bool releaseEarly, bool expectReleasedDuringReject) + { + MockTracer tracer = new MockTracer(); + MockGitProcess gitProcess = CreateGitProcess(); + GitAuthentication authentication = CreateInitializedAuthentication(tracer, gitProcess); + MockGVFSEnlistment enlistment = new MockGVFSEnlistment(gitProcess, authentication); + + using (ManualResetEventSlim reached = new ManualResetEventSlim(false)) + using (ManualResetEventSlim block = new ManualResetEventSlim(false)) + using (StubHttpMessageHandler handler = new StubHttpMessageHandler(HttpStatusCode.Unauthorized, "unauthorized")) + using (TestingHttpRequestor requestor = new TestingHttpRequestor(tracer, new RetryConfig(), enlistment, handler, releaseEarly)) + { + int before = HttpRequestor.AvailableConnectionCount; + gitProcess.InvokeReachedBlock = reached; + gitProcess.BlockInvokeUntilSignaled = block; + + GitEndPointResponseData response = null; + Thread worker = new Thread(() => response = requestor.Send(CancellationToken.None)); + worker.IsBackground = true; + worker.Start(); + + reached.Wait(TimeSpan.FromSeconds(5)).ShouldEqual(true, "The reject leg should have started a git invocation"); + + int duringReject = HttpRequestor.AvailableConnectionCount; + if (expectReleasedDuringReject) + { + duringReject.ShouldEqual(before, "The connection slot should be released before the reject leg runs"); + } + else + { + duringReject.ShouldEqual(before - 1, "The connection slot should still be held during the reject leg"); + } + + block.Set(); + worker.Join(TimeSpan.FromSeconds(5)).ShouldEqual(true, "The request should complete after the reject leg unblocks"); + + response.ShouldNotBeNull("Expected a response"); + response.HasErrors.ShouldEqual(true, "Expected a 401 error response"); + response.Dispose(); + + HttpRequestor.AvailableConnectionCount.ShouldEqual(before, "The connection slot should be fully released after completion"); + } + } + + private sealed class TestingHttpRequestor : HttpRequestor + { + private readonly bool releaseEarly; + + public TestingHttpRequestor(ITracer tracer, RetryConfig retryConfig, Enlistment enlistment, HttpMessageHandler handler, bool releaseEarly) + : base(tracer, retryConfig, enlistment, handler) + { + this.releaseEarly = releaseEarly; + } + + protected override int CredentialTimeoutMs => GitAuthentication.BackgroundCredentialTimeoutMs; + + protected override bool ShouldReleaseConnectionBeforeCredentialReject => this.releaseEarly; + + public GitEndPointResponseData Send(CancellationToken cancellationToken) + { + return this.SendRequest( + GetNewRequestId(), + new Uri("https://mock.gvfs/gvfs/objects"), + HttpMethod.Get, + requestContent: null, + cancellationToken: cancellationToken); + } + } + + private sealed class StubHttpMessageHandler : HttpMessageHandler + { + private readonly HttpStatusCode statusCode; + private readonly string body; + + public StubHttpMessageHandler(HttpStatusCode statusCode, string body) + { + this.statusCode = statusCode; + this.body = body; + } + + protected override Task SendAsync(HttpRequestMessage request, CancellationToken cancellationToken) + { + HttpResponseMessage response = new HttpResponseMessage(this.statusCode) + { + Content = new StringContent(this.body), + }; + + return Task.FromResult(response); + } + } + } +} diff --git a/GVFS/GVFS.UnitTests/Mock/Common/MockGVFSEnlistment.cs b/GVFS/GVFS.UnitTests/Mock/Common/MockGVFSEnlistment.cs index 2292149b65..77b59deb3b 100644 --- a/GVFS/GVFS.UnitTests/Mock/Common/MockGVFSEnlistment.cs +++ b/GVFS/GVFS.UnitTests/Mock/Common/MockGVFSEnlistment.cs @@ -29,6 +29,15 @@ public MockGVFSEnlistment(MockGitProcess gitProcess) this.gitProcess = gitProcess; } + public MockGVFSEnlistment(MockGitProcess gitProcess, GitAuthentication authentication) + : base(Path.Combine("mock:", "path"), "mock://repoUrl", Path.Combine("mock:", "git"), authentication) + { + this.gitProcess = gitProcess; + this.GitObjectsRoot = Path.Combine("mock:", "path", ".git", "objects"); + this.LocalObjectsRoot = this.GitObjectsRoot; + this.GitPackRoot = Path.Combine("mock:", "path", ".git", "objects", "pack"); + } + public override string GitObjectsRoot { get; protected set; } public override string LocalObjectsRoot { get; protected set; } diff --git a/GVFS/GVFS.UnitTests/Mock/Git/MockGitProcess.cs b/GVFS/GVFS.UnitTests/Mock/Git/MockGitProcess.cs index 69cad652fc..c0d2cb0552 100644 --- a/GVFS/GVFS.UnitTests/Mock/Git/MockGitProcess.cs +++ b/GVFS/GVFS.UnitTests/Mock/Git/MockGitProcess.cs @@ -7,6 +7,7 @@ using System.IO; using System.Linq; using System.Text; +using System.Threading; namespace GVFS.UnitTests.Mock.Git { @@ -20,6 +21,7 @@ public MockGitProcess() this.CommandsRun = new List(); this.InvokedTimeoutMs = new List(); this.LastInvokedTimeoutMs = null; + this.LastInvokedCancellationToken = CancellationToken.None; this.StoredCredentials = new Dictionary(StringComparer.OrdinalIgnoreCase); this.CredentialApprovals = new Dictionary>(); this.CredentialRejections = new Dictionary>(); @@ -38,6 +40,26 @@ public MockGitProcess() /// public int? LastInvokedTimeoutMs { get; private set; } + /// + /// The cancellation token passed to the most recent InvokeGitImpl call. Lets tests assert + /// that a caller plumbed a real (cancelable) token down to the git invocation. + /// + public CancellationToken LastInvokedCancellationToken { get; private set; } + + /// + /// When set, InvokeGitImpl blocks until this event is signaled or the caller's token is + /// canceled. Lets tests simulate a slow/hung git credential process and prove that + /// cancellation interrupts it and that shared resources are not held meanwhile. + /// + public ManualResetEventSlim BlockInvokeUntilSignaled { get; set; } + + /// + /// Signaled by InvokeGitImpl right before it starts blocking on + /// . Lets a test wait until the git invocation is + /// actually in-flight before it inspects shared state or cancels. + /// + public ManualResetEventSlim InvokeReachedBlock { get; set; } + public bool ShouldFail { get; set; } public Dictionary StoredCredentials { get; } public Dictionary> CredentialApprovals { get; } @@ -49,7 +71,7 @@ public void SetExpectedCommandResult(string command, Func result, bool m this.expectedCommandInfos.Add(commandInfo); } - public override bool TryStoreCredential(ITracer tracer, string repoUrl, string username, string password, out string error, int timeoutMs = -1) + public override bool TryStoreCredential(ITracer tracer, string repoUrl, string username, string password, out string error, int timeoutMs = -1, CancellationToken cancellationToken = default) { Credential credential = new Credential(username, password); @@ -66,10 +88,10 @@ public override bool TryStoreCredential(ITracer tracer, string repoUrl, string u // Store the credential this.StoredCredentials[repoUrl] = credential; - return base.TryStoreCredential(tracer, repoUrl, username, password, out error, timeoutMs); + return base.TryStoreCredential(tracer, repoUrl, username, password, out error, timeoutMs, cancellationToken); } - public override bool TryDeleteCredential(ITracer tracer, string repoUrl, string username, string password, out string error, int timeoutMs = -1) + public override bool TryDeleteCredential(ITracer tracer, string repoUrl, string username, string password, out string error, int timeoutMs = -1, CancellationToken cancellationToken = default) { Credential credential = new Credential(username, password); @@ -86,7 +108,7 @@ public override bool TryDeleteCredential(ITracer tracer, string repoUrl, string // Erase the credential this.StoredCredentials.Remove(repoUrl); - return base.TryDeleteCredential(tracer, repoUrl, username, password, out error, timeoutMs); + return base.TryDeleteCredential(tracer, repoUrl, username, password, out error, timeoutMs, cancellationToken); } protected override Result InvokeGitImpl( @@ -98,11 +120,32 @@ protected override Result InvokeGitImpl( Action parseStdOutLine, int timeoutMs, string gitObjectsDirectory = null, - bool usePrecommandHook = true) + bool usePrecommandHook = true, + CancellationToken cancellationToken = default) { this.CommandsRun.Add(command); this.LastInvokedTimeoutMs = timeoutMs; this.InvokedTimeoutMs.Add(timeoutMs); + this.LastInvokedCancellationToken = cancellationToken; + + // Simulate a slow/hung git process that only completes when the test signals it or the + // caller cancels. This lets tests assert that cancellation actually interrupts an + // in-flight credential invocation instead of blocking for the full timeout. + ManualResetEventSlim blockUntilSignaled = this.BlockInvokeUntilSignaled; + if (blockUntilSignaled != null) + { + this.InvokeReachedBlock?.Set(); + try + { + blockUntilSignaled.Wait(cancellationToken); + } + catch (OperationCanceledException) + { + // Mirror the real GitProcess.InvokeGitImpl contract: a canceled invocation + // surfaces cancellation rather than returning a timeout Result. + throw new OperationCanceledException(cancellationToken); + } + } if (this.ShouldFail) {