Skip to content

Mount: don't block startup on auth when cache server is configured #2034

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
tyrielv wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Mount: don't block startup on auth when cache server is configured #2034

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 44 additions & 18 deletions GVFS/GVFS.Common/Git/GitAuthentication.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,11 @@ namespace GVFS.Common.Git
public class GitAuthentication
{
private const double MaxBackoffSeconds = 30;
public const int DefaultCredentialTimeoutMs = 30_000;
public const int BackgroundCredentialTimeoutMs = 120_000;

private readonly Lock gitAuthLock = new Lock();
private readonly SemaphoreSlim credentialGate = new SemaphoreSlim(1, 1);
private readonly ICredentialStore credentialStore;
private readonly string repoUrl;

Expand Down Expand Up @@ -219,7 +222,8 @@ public bool TryInitializeAndQueryGVFSConfig(
RetryConfig retryConfig,
out ServerGVFSConfig serverGVFSConfig,
out string errorMessage,
out bool isAuthFailure)
out bool isAuthFailure,
int credentialTimeoutMs = DefaultCredentialTimeoutMs)
{
if (this.isInitialized)
{
Expand All @@ -246,6 +250,7 @@ public bool TryInitializeAndQueryGVFSConfig(

if (httpStatus != HttpStatusCode.Unauthorized)
{
this.isInitialized = true;
errorMessage = "Unable to query /gvfs/config";
tracer.RelatedWarning("{0}: Config query failed with status {1}", nameof(this.TryInitializeAndQueryGVFSConfig), httpStatus?.ToString() ?? "None");
return false;
Expand All @@ -254,9 +259,13 @@ public bool TryInitializeAndQueryGVFSConfig(
// Server requires authentication — fetch credentials
this.IsAnonymous = false;
Comment thread
tyrielv marked this conversation as resolved.

if (!this.TryCallGitCredential(tracer, out errorMessage))
if (!this.TryCallGitCredential(tracer, out errorMessage, credentialTimeoutMs))
{
isAuthFailure = true;
// Mark initialized even on failure so TryGetCredentials can
// retry later (e.g., when mount proceeds with a cache server
// and object downloads need auth).
this.isInitialized = true;
tracer.RelatedWarning("{0}: Credential fetch failed: {1}", nameof(this.TryInitializeAndQueryGVFSConfig), errorMessage);
return false;
}
Expand Down Expand Up @@ -376,28 +385,45 @@ private void UpdateBackoff()
this.numberOfAttempts++;
}

private bool TryCallGitCredential(ITracer tracer, out string errorMessage)
private bool TryCallGitCredential(ITracer tracer, out string errorMessage, int timeoutMs = -1)
{
string gitUsername;
string gitPassword;
if (!this.credentialStore.TryGetCredential(tracer, this.repoUrl, out gitUsername, out gitPassword, out errorMessage))
// 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);
try
{
this.UpdateBackoff();
return false;
}
string gitUsername;
string gitPassword;
if (!this.credentialStore.TryGetCredential(tracer, this.repoUrl, out gitUsername, out gitPassword, out errorMessage, timeoutMs))
{
this.UpdateBackoff();
return false;
}

if (!string.IsNullOrEmpty(gitUsername) && !string.IsNullOrEmpty(gitPassword))
{
this.cachedCredentialString = Convert.ToBase64String(Encoding.ASCII.GetBytes(gitUsername + ":" + gitPassword));
this.isCachedCredentialStringApproved = false;
if (!string.IsNullOrEmpty(gitUsername) && !string.IsNullOrEmpty(gitPassword))
{
this.cachedCredentialString = Convert.ToBase64String(Encoding.ASCII.GetBytes(gitUsername + ":" + gitPassword));
this.isCachedCredentialStringApproved = false;
}
else
{
errorMessage = "Got back empty credentials from git";
return false;
}

return true;
}
else
finally
{
errorMessage = "Got back empty credentials from git";
return false;
if (acquired)
{
this.credentialGate.Release();
}
}

return true;
}
}
}
33 changes: 24 additions & 9 deletions GVFS/GVFS.Common/Git/GitProcess.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -297,7 +297,8 @@ public virtual bool TryGetCredential(
string repoUrl,
out string username,
out string password,
out string errorMessage)
out string errorMessage,
int timeoutMs = -1)
{
username = null;
password = null;
Expand All @@ -311,16 +312,29 @@ public virtual bool TryGetCredential(
GenerateCredentialVerbCommand("fill"),
stdin => stdin.Write($"url={repoUrl}\n\n"),
parseStdOutLine: null,
usePreCommandHook: false);
usePreCommandHook: false,
timeoutMs: timeoutMs);

if (gitCredentialOutput.ExitCodeIsFailure)
{
EventMetadata errorData = new EventMetadata();
tracer.RelatedWarning(
errorData,
"Git could not get credentials: " + gitCredentialOutput.Errors,
Keywords.Network | Keywords.Telemetry);
errorMessage = gitCredentialOutput.Errors;

if (gitCredentialOutput.Errors.StartsWith("Operation timed out"))
{
errorMessage = "Credential manager did not respond within " + (timeoutMs / 1000) + " seconds";
tracer.RelatedWarning(
errorData,
"Git credential fill timed out after " + timeoutMs + "ms",
Keywords.Network | Keywords.Telemetry);
}
else
{
errorMessage = gitCredentialOutput.Errors;
tracer.RelatedWarning(
errorData,
"Git could not get credentials: " + gitCredentialOutput.Errors,
Keywords.Network | Keywords.Telemetry);
}

return false;
}
Expand Down Expand Up @@ -1113,7 +1127,8 @@ private Result InvokeGitAgainstDotGitFolder(
Action<StreamWriter> writeStdIn,
Action<string> parseStdOutLine,
bool usePreCommandHook = true,
string gitObjectsDirectory = null)
string gitObjectsDirectory = null,
int timeoutMs = -1)
{
// 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
Expand All @@ -1125,7 +1140,7 @@ private Result InvokeGitAgainstDotGitFolder(
useReadObjectHook: false,
writeStdIn: writeStdIn,
parseStdOutLine: parseStdOutLine,
timeoutMs: -1,
timeoutMs: timeoutMs,
gitObjectsDirectory: gitObjectsDirectory,
usePreCommandHook: usePreCommandHook);
}
Expand Down
2 changes: 1 addition & 1 deletion GVFS/GVFS.Common/Git/ICredentialStore.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ namespace GVFS.Common.Git
{
public interface ICredentialStore
{
bool TryGetCredential(ITracer tracer, string url, out string username, out string password, out string error);
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);

Expand Down
2 changes: 2 additions & 0 deletions GVFS/GVFS.Common/ReturnCode.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,5 +12,7 @@ public enum ReturnCode
DehydrateFolderFailures = 7,
MountAlreadyRunning = 8,
AuthenticationError = 9,
CredentialTimeout = 10,
RemoteGvfsConfigError = 11,
}
}
47 changes: 39 additions & 8 deletions GVFS/GVFS.Mount/InProcessMount.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,11 @@ private void MountWithLockAcquired(EventLevel verbosity, Keywords keywords)
// and config query into at most 2 HTTP requests (1 for anonymous repos), reusing
// the same HttpClient/TCP connection.
Stopwatch parallelTimer = Stopwatch.StartNew();
bool hasCacheServer = this.cacheServer != null && !string.IsNullOrWhiteSpace(this.cacheServer.Url);

// When a cache server is configured locally, auth/config is best-effort:
// mount can proceed without it. We still attempt it so GCM can pop up a
// renewal prompt for stale tokens, but we don't block mount on the result.
var networkTask = Task.Run(() =>
{
Stopwatch sw = Stopwatch.StartNew();
Expand All @@ -139,22 +143,31 @@ private void MountWithLockAcquired(EventLevel verbosity, Keywords keywords)

if (!this.enlistment.Authentication.TryInitializeAndQueryGVFSConfig(
this.tracer, this.enlistment, this.retryConfig,
Comment thread
tyrielv marked this conversation as resolved.
out config, out authConfigError, out isAuthFailure))
out config, out authConfigError, out isAuthFailure,
credentialTimeoutMs: hasCacheServer ? GitAuthentication.BackgroundCredentialTimeoutMs : GitAuthentication.DefaultCredentialTimeoutMs))
{
if (this.cacheServer != null && !string.IsNullOrWhiteSpace(this.cacheServer.Url))
if (hasCacheServer)
{
this.tracer.RelatedWarning("Mount will proceed with fallback cache server: " + authConfigError);
config = null;
}
else
{
ReturnCode exitCode = ReturnCode.RemoteGvfsConfigError;
if (isAuthFailure)
{
exitCode = authConfigError != null && authConfigError.Contains("Credential manager did not respond")
? ReturnCode.CredentialTimeout
: ReturnCode.AuthenticationError;
}

this.FailMountAndExit(
isAuthFailure ? ReturnCode.AuthenticationError : ReturnCode.GenericError,
exitCode,
"Unable to query /gvfs/config" + Environment.NewLine + authConfigError);
}
}

this.ValidateGVFSVersion(config);
this.ValidateGVFSVersion(config, failOnError: !hasCacheServer);
this.tracer.RelatedInfo("ParallelMount: Auth + config completed in {0}ms", sw.ElapsedMilliseconds);
return config;
});
Expand Down Expand Up @@ -242,7 +255,22 @@ private void MountWithLockAcquired(EventLevel verbosity, Keywords keywords)

try
{
Task.WaitAll(networkTask, localTask);
if (hasCacheServer)
{
// With a cache server, don't block mount on the network task.
// Auth runs in the background to warm credentials / pop GCM,
// but mount proceeds immediately using the local cache server URL.
localTask.Wait();

// Observe background task exceptions so they don't go unhandled.
networkTask.ContinueWith(
t => this.tracer.RelatedWarning("Background auth task failed: " + t.Exception.Flatten().InnerExceptions[0].Message),
TaskContinuationOptions.OnlyOnFaulted);
}
else
{
Task.WaitAll(networkTask, localTask);
}
}
catch (AggregateException ae)
{
Expand All @@ -252,7 +280,7 @@ private void MountWithLockAcquired(EventLevel verbosity, Keywords keywords)
parallelTimer.Stop();
this.tracer.RelatedInfo("ParallelMount: All parallel tasks completed in {0}ms", parallelTimer.ElapsedMilliseconds);

ServerGVFSConfig serverGVFSConfig = networkTask.Result;
ServerGVFSConfig serverGVFSConfig = hasCacheServer ? null : networkTask.Result;
Comment thread
tyrielv marked this conversation as resolved.

this.mountProgressMessage = "Resolving cache server";
CacheServerResolver cacheServerResolver = new CacheServerResolver(this.tracer, this.enlistment);
Expand Down Expand Up @@ -1467,7 +1495,7 @@ private ServerGVFSConfig QueryAndValidateGVFSConfig()
return serverGVFSConfig;
}

private void ValidateGVFSVersion(ServerGVFSConfig config)
private void ValidateGVFSVersion(ServerGVFSConfig config, bool failOnError = true)
{
using (ITracer activity = this.tracer.StartActivity("ValidateGVFSVersion", EventLevel.Informational))
{
Expand Down Expand Up @@ -1519,7 +1547,10 @@ private void ValidateGVFSVersion(ServerGVFSConfig config)
}

activity.RelatedError("GVFS version {0} is not supported", currentVersion);
this.FailMountAndExit("ERROR: Your GVFS version is no longer supported. Install the latest and try again.");
if (failOnError)
{
this.FailMountAndExit("ERROR: Your GVFS version is no longer supported. Install the latest and try again.");
}
}
}

Expand Down
20 changes: 20 additions & 0 deletions GVFS/GVFS/CommandLine/MountVerb.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -280,11 +280,31 @@ private bool TryMount(ITracer tracer, GVFSEnlistment enlistment, string mountExe

tracer.RelatedInfo($"{nameof(this.TryMount)}: Waiting for repo to be mounted");

Process process = this.mountProcess;
Func<GVFSEnlistment.MountProcessSnapshot> snapshot = () =>
{
try
{
if (!process.HasExited)
{
return new GVFSEnlistment.MountProcessSnapshot(process.Id, hasExited: false, exitCode: 0);
}

return new GVFSEnlistment.MountProcessSnapshot(process.Id, hasExited: true, exitCode: process.ExitCode);
}
catch (InvalidOperationException)
{
// Process object disposed or not started — treat as exited.
return new GVFSEnlistment.MountProcessSnapshot(processId: 0, hasExited: true, exitCode: -1);
}
};

return GVFSEnlistment.WaitUntilMounted(
tracer,
enlistment.NamedPipeName,
enlistment.WorkingDirectoryRoot,
this.Unattended,
snapshot,
out errorMessage,
onProgress: progress => this.currentMountProgress = progress);
}
Expand Down
Loading