Auth concurrency: release HTTP pool slot before credential reject; thread cancellation to the credential path (stacked on #2046)

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

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;
 
+        /// <summary>
+        /// 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.
+        /// </summary>
+        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,16 @@ public bool IsBackingOff
 
         private GitSsl GitSsl { get; }
 
-        public void ApproveCredentials(ITracer tracer, string credentialString)
+        /// <summary>
+        /// Test-only hook to force the anonymous state. Production code determines this
+        /// by probing the server in <see cref="TryInitializeAndQueryGVFSConfig"/>.
+        /// </summary>
+        internal void SetIsAnonymousForTesting(bool isAnonymous)
+        {
+            this.IsAnonymous = isAnonymous;
+        }
+
+        public void ApproveCredentials(ITracer tracer, string credentialString, int credentialTimeoutMs = DefaultCredentialTimeoutMs, CancellationToken cancellationToken = default)
         {
             lock (this.gitAuthLock)
             {
@@ -70,7 +86,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, cancellationToken))
                             {
                                 // Storing credentials is best effort attempt - log failure, but do not fail
                                 tracer.RelatedWarning("Failed to store credential string: {0}", error);
@@ -91,7 +107,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, CancellationToken cancellationToken = default)
         {
             lock (this.gitAuthLock)
             {
@@ -102,7 +118,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, cancellationToken))
                     {
                         if (this.cachedCredentialString != cachedCredentialAtStartOfReject)
                         {
@@ -123,7 +139,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, cancellationToken))
                         {
                             // Deleting credentials is best effort attempt - log failure, but do not fail
                             tracer.RelatedWarning("Failed to delete credential string: {0}", error);
@@ -138,7 +154,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, cancellationToken: cancellationToken);
                     }
 
                     this.cachedCredentialString = null;
@@ -153,7 +169,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, CancellationToken cancellationToken = default)
         {
             if (!this.isInitialized)
             {
@@ -173,7 +189,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, cancellationToken))
                         {
                             return false;
                         }
@@ -385,20 +401,23 @@ 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
             // 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, 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;

GVFS/GVFS.Common/Git/GitProcess.cs

@@ -37,6 +37,12 @@ public class GitProcess : ICredentialStore
         /// </summary>
         private const int MaxCapturedStdOutChars = 128 * 1024 * 1024; // ~256 MB of UTF-16
 
+        /// <summary>
+        /// 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.
+        /// </summary>
+        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, CancellationToken cancellationToken = default)
         {
             StringBuilder sb = new StringBuilder();
             sb.AppendFormat("url={0}\n", repoUrl);
@@ -214,7 +220,9 @@ public virtual bool TryDeleteCredential(ITracer tracer, string repoUrl, string u
                 GenerateCredentialVerbCommand("reject"),
                 stdin => stdin.Write(stdinConfig),
                 null,
-                usePreCommandHook: false);
+                usePreCommandHook: false,
+                timeoutMs: timeoutMs,
+                cancellationToken: cancellationToken);
 
             if (result.ExitCodeIsFailure)
             {
@@ -228,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)
+        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);
@@ -242,7 +250,9 @@ public virtual bool TryStoreCredential(ITracer tracer, string repoUrl, string us
                 GenerateCredentialVerbCommand("approve"),
                 stdin => stdin.Write(stdinConfig),
                 null,
-                usePreCommandHook: false);
+                usePreCommandHook: false,
+                timeoutMs: timeoutMs,
+                cancellationToken: cancellationToken);
 
             if (result.ExitCodeIsFailure)
             {
@@ -320,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;
@@ -335,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)
                 {
@@ -975,7 +987,8 @@ protected virtual Result InvokeGitImpl(
             Action<string> parseStdOutLine,
             int timeoutMs,
             string gitObjectsDirectory = null,
-            bool usePreCommandHook = true)
+            bool usePreCommandHook = true,
+            CancellationToken cancellationToken = default)
         {
             if (failedToSetEncoding && writeStdIn != null)
             {
@@ -1049,9 +1062,29 @@ 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)
                         {
-                            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);
+
+                            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);
                         }
@@ -1070,6 +1103,46 @@ protected virtual Result InvokeGitImpl(
             }
         }
 
+        /// <summary>
+        /// 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 <see cref="Process.WaitForExit(int)"/>
+        /// has no cancellation-aware overload.
+        /// </summary>
+        /// <returns>True if the process exited on its own; false if it must be killed.</returns>
+        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}";
@@ -1155,7 +1228,8 @@ private Result InvokeGitAgainstDotGitFolder(
             Action<string> 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
@@ -1169,7 +1243,8 @@ private Result InvokeGitAgainstDotGitFolder(
                 parseStdOutLine: parseStdOutLine,
                 timeoutMs: timeoutMs,
                 gitObjectsDirectory: gitObjectsDirectory,
-                usePreCommandHook: usePreCommandHook);
+                usePreCommandHook: usePreCommandHook,
+                cancellationToken: cancellationToken);
         }
 
         public class Result

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);
+        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);
+        bool TryDeleteCredential(ITracer tracer, string url, string username, string password, out string error, int timeoutMs = -1, CancellationToken cancellationToken = default);
     }
 }