From 7648b2e2e67ede7f4ebcb1e7b50136b6d10666e2 Mon Sep 17 00:00:00 2001 From: Tyrie Vella Date: Tue, 6 Oct 2026 09:44:58 -0700 Subject: [PATCH 1/3] Keep authentication adaptive after indeterminate config probes Mount probes /gvfs/config without credentials to learn whether the server requires authentication. A timeout, server error, or socket failure does not answer that question. Treating this result as anonymous caused later cache requests to omit credentials forever. Treating it as authenticated can block requests before they reach a server that accepts anonymous access. Keep authentication in one of three states: unknown, anonymous, or authenticated. After an indeterminate probe, send the next runtime request without credentials. Record anonymous access after a successful response. Promote the state to authenticated after a 401, redirect, or the cache server's authentication-required 400 response, then retry with credentials. A known-anonymous state can also promote if server behavior changes. Keep the initial config probe independent from the state that it determines. Make mount mark initialization before it queues background authentication, so callbacks wait for the initial result. Always signal initialization completion, including after exceptions. Capture probe failures in telemetry and user-visible errors. Document the dual-success credential contract. Add request-level tests for unknown, anonymous, and authenticated transitions, plus initialization failure paths. Assisted-by: Claude Opus 5 Assisted-by: GPT-5.6 Sol Signed-off-by: Tyrie Vella --- GVFS/GVFS.Common/Git/GitAuthentication.cs | 172 ++++++- GVFS/GVFS.Common/Http/ConfigHttpRequestor.cs | 13 +- GVFS/GVFS.Common/Http/HttpRequestor.cs | 105 +++-- GVFS/GVFS.Common/Http/IGVFSConfigRequestor.cs | 33 ++ GVFS/GVFS.Common/InternalsVisibleTo.cs | 1 + GVFS/GVFS.Mount/InProcessMount.cs | 4 + .../Git/GitAuthenticationTests.cs | 445 ++++++++++++++++++ .../Mock/Common/MockGVFSEnlistment.cs | 7 +- GVFS/GVFS.UnitTests/Mock/Common/MockTracer.cs | 22 +- .../Mock/Http/MockGVFSConfigRequestor.cs | 101 ++++ .../FileSystemCallbacksTests.cs | 6 +- 11 files changed, 854 insertions(+), 55 deletions(-) create mode 100644 GVFS/GVFS.Common/Http/IGVFSConfigRequestor.cs create mode 100644 GVFS/GVFS.UnitTests/Mock/Http/MockGVFSConfigRequestor.cs diff --git a/GVFS/GVFS.Common/Git/GitAuthentication.cs b/GVFS/GVFS.Common/Git/GitAuthentication.cs index c35a11a823..b11720b3f2 100644 --- a/GVFS/GVFS.Common/Git/GitAuthentication.cs +++ b/GVFS/GVFS.Common/Git/GitAuthentication.cs @@ -12,6 +12,13 @@ namespace GVFS.Common.Git { public class GitAuthentication { + private enum AuthenticationState + { + Unknown = 0, + Anonymous, + Authenticated, + } + private const double MaxBackoffSeconds = 30; public const int DefaultCredentialTimeoutMs = 30_000; public const int BackgroundCredentialTimeoutMs = 120_000; @@ -36,6 +43,8 @@ public class GitAuthentication private bool isCachedCredentialStringApproved = false; private bool isInitialized; + private int authenticationState; + private int initializationStarted; public GitAuthentication(GitProcess git, string repoUrl) { @@ -56,7 +65,36 @@ public bool IsBackingOff } } - public bool IsAnonymous { get; private set; } = true; + /// + /// True only when the server is known to allow anonymous access. + /// + /// + /// This is false while the state is unknown. While true, + /// omits the Authorization + /// header and never calls . + /// + public bool IsAnonymous + { + get => this.GetAuthenticationState() == AuthenticationState.Anonymous; + } + + internal bool ShouldSendAnonymous => + !this.IsInitializationStarted || + (this.initializationComplete.IsSet && this.GetAuthenticationState() != AuthenticationState.Authenticated); + + /// + /// Indicates whether authentication initialization has started. + /// + internal bool IsInitializationStarted => Volatile.Read(ref this.initializationStarted) != 0; + + /// + /// Marks authentication initialization as scheduled so that requests wait + /// for its result instead of being sent anonymously. + /// + internal void MarkInitializationStarted() + { + Interlocked.Exchange(ref this.initializationStarted, 1); + } /// /// How long a caller of will wait for @@ -66,6 +104,14 @@ public bool IsBackingOff /// internal int InitializationWaitTimeoutMs { get; set; } = BackgroundCredentialTimeoutMs; + /// + /// Test seam for the /gvfs/config probe. When null, production code uses + /// . Unit tests substitute a fake so the + /// probe outcome - anonymous success, 401, or an indeterminate network + /// failure - can be driven deterministically. + /// + internal IGVFSConfigRequestor ConfigRequestorOverride { get; set; } + private GitSsl GitSsl { get; } public void ApproveCredentials(ITracer tracer, string credentialString) @@ -169,6 +215,19 @@ public void RejectCredentials(ITracer tracer, string credentialString) } } + /// + /// Gets the credential to send with a request. + /// + /// + /// The Basic credential, or null when this returns true because the server + /// allows anonymous access or its authentication requirement remains unknown. + /// Callers must check for null before using it. + /// + /// The failure reason when this returns false. + /// + /// True when the request can proceed: with the credential in + /// , or anonymously when it is null. + /// public bool TryGetCredentials(ITracer tracer, out string credentialString, out string errorMessage) { if (!this.isInitialized) @@ -186,6 +245,13 @@ public bool TryGetCredentials(ITracer tracer, out string credentialString, out s } } + if (this.GetAuthenticationState() != AuthenticationState.Authenticated) + { + credentialString = null; + errorMessage = null; + return true; + } + credentialString = this.cachedCredentialString; if (credentialString == null) { @@ -256,19 +322,51 @@ public bool TryInitializeAndQueryGVFSConfig( throw new InvalidOperationException("Already initialized"); } + this.MarkInitializationStarted(); + + try + { + return this.TryInitializeAndQueryGVFSConfigCore( + tracer, + enlistment, + retryConfig, + out serverGVFSConfig, + out errorMessage, + out isAuthFailure, + credentialTimeoutMs); + } + finally + { + if (!this.isInitialized) + { + this.MarkInitialized(); + } + } + } + + private bool TryInitializeAndQueryGVFSConfigCore( + ITracer tracer, + Enlistment enlistment, + RetryConfig retryConfig, + out ServerGVFSConfig serverGVFSConfig, + out string errorMessage, + out bool isAuthFailure, + int credentialTimeoutMs) + { serverGVFSConfig = null; errorMessage = null; isAuthFailure = false; - using (ConfigHttpRequestor configRequestor = new ConfigHttpRequestor(tracer, enlistment, retryConfig)) + IGVFSConfigRequestor configRequestor = this.ConfigRequestorOverride ?? new ConfigHttpRequestor(tracer, enlistment, retryConfig); + using (configRequestor) { HttpStatusCode? httpStatus; + string probeError; - // First attempt without credentials. If anonymous access works, - // we get the config in a single request. - if (configRequestor.TryQueryGVFSConfig(false, out serverGVFSConfig, out httpStatus, out _)) + // The probe must not consult the authentication state it determines. + if (configRequestor.TryQueryGVFSConfig(false, out serverGVFSConfig, out httpStatus, out probeError, forceAnonymous: true)) { - this.IsAnonymous = true; + this.MarkAnonymousAccessSucceeded(); this.MarkInitialized(); tracer.RelatedInfo("{0}: Anonymous access succeeded, config obtained in one request", nameof(this.TryInitializeAndQueryGVFSConfig)); return true; @@ -276,14 +374,31 @@ public bool TryInitializeAndQueryGVFSConfig( if (httpStatus != HttpStatusCode.Unauthorized) { + // The probe did not determine whether the server allows anonymous + // access. Keep the state unknown so later requests can try + // anonymously and promote to authenticated if the server rejects them. this.MarkInitialized(); - errorMessage = "Unable to query /gvfs/config"; - tracer.RelatedWarning("{0}: Config query failed with status {1}", nameof(this.TryInitializeAndQueryGVFSConfig), httpStatus?.ToString() ?? "None"); + errorMessage = "Unable to query /gvfs/config" + + (string.IsNullOrEmpty(probeError) ? string.Empty : ": " + probeError) + + ". The authentication requirement remains unknown."; + + // The probe is unauthenticated, so the error text carries no credential. + EventMetadata indeterminateMetadata = new EventMetadata(new Dictionary + { + ["Area"] = nameof(GitAuthentication), + ["HttpStatus"] = httpStatus?.ToString() ?? "None", + ["ProbeError"] = probeError ?? string.Empty, + ["AuthenticationState"] = AuthenticationState.Unknown.ToString(), + }); + tracer.RelatedWarning( + indeterminateMetadata, + $"{nameof(this.TryInitializeAndQueryGVFSConfig)}: Config query failed with status {httpStatus?.ToString() ?? "None"}; authentication requirement remains unknown", + Keywords.Telemetry); return false; } // Server requires authentication — fetch credentials - this.IsAnonymous = false; + this.MarkAuthenticationRequired(); if (!this.TryCallGitCredential(tracer, out errorMessage, credentialTimeoutMs)) { @@ -327,13 +442,44 @@ internal bool TryInitializeAndRequireAuth(ITracer tracer, out string errorMessag throw new InvalidOperationException("Already initialized"); } - if (this.TryCallGitCredential(tracer, out errorMessage)) + this.MarkInitializationStarted(); + this.MarkAuthenticationRequired(); + + try { - this.MarkInitialized(); - return true; + if (this.TryCallGitCredential(tracer, out errorMessage)) + { + this.MarkInitialized(); + return true; + } + + return false; } + finally + { + if (!this.isInitialized) + { + this.MarkInitialized(); + } + } + } - return false; + internal void MarkAnonymousAccessSucceeded() + { + Interlocked.CompareExchange( + ref this.authenticationState, + (int)AuthenticationState.Anonymous, + (int)AuthenticationState.Unknown); + } + + internal void MarkAuthenticationRequired() + { + Interlocked.Exchange(ref this.authenticationState, (int)AuthenticationState.Authenticated); + } + + private AuthenticationState GetAuthenticationState() + { + return (AuthenticationState)Volatile.Read(ref this.authenticationState); } public void ConfigureHttpClientHandlerSslIfNeeded(ITracer tracer, HttpClientHandler httpClientHandler, GitProcess gitProcess) diff --git a/GVFS/GVFS.Common/Http/ConfigHttpRequestor.cs b/GVFS/GVFS.Common/Http/ConfigHttpRequestor.cs index 95b531bb36..0c1b2ae77a 100644 --- a/GVFS/GVFS.Common/Http/ConfigHttpRequestor.cs +++ b/GVFS/GVFS.Common/Http/ConfigHttpRequestor.cs @@ -7,7 +7,7 @@ namespace GVFS.Common.Http { - public class ConfigHttpRequestor : HttpRequestor + public class ConfigHttpRequestor : HttpRequestor, IGVFSConfigRequestor { private readonly string repoUrl; @@ -17,7 +17,7 @@ public ConfigHttpRequestor(ITracer tracer, Enlistment enlistment, RetryConfig re this.repoUrl = enlistment.RepoUrl; } - public bool TryQueryGVFSConfig(bool logErrors, out ServerGVFSConfig serverGVFSConfig, out HttpStatusCode? httpStatus, out string errorMessage) + public bool TryQueryGVFSConfig(bool logErrors, out ServerGVFSConfig serverGVFSConfig, out HttpStatusCode? httpStatus, out string errorMessage, bool forceAnonymous = false) { serverGVFSConfig = null; httpStatus = null; @@ -56,7 +56,8 @@ public bool TryQueryGVFSConfig(bool logErrors, out ServerGVFSConfig serverGVFSCo gvfsConfigEndpoint, HttpMethod.Get, requestContent: null, - cancellationToken: CancellationToken.None)) + cancellationToken: CancellationToken.None, + forceAnonymous: forceAnonymous)) { if (response.HasErrors) { @@ -89,6 +90,12 @@ public bool TryQueryGVFSConfig(bool logErrors, out ServerGVFSConfig serverGVFSCo httpStatus = httpException.StatusCode; errorMessage = httpException.Message; } + else + { + // No HTTP response (TLS, DNS, proxy, or socket failure): there is no + // status to report, so keep the exception text as the only diagnostic. + errorMessage = output.Error.Message; + } if (logErrors) { diff --git a/GVFS/GVFS.Common/Http/HttpRequestor.cs b/GVFS/GVFS.Common/Http/HttpRequestor.cs index ee1558abf2..34e626b128 100644 --- a/GVFS/GVFS.Common/Http/HttpRequestor.cs +++ b/GVFS/GVFS.Common/Http/HttpRequestor.cs @@ -38,6 +38,20 @@ static HttpRequestor() } protected HttpRequestor(ITracer tracer, RetryConfig retryConfig, Enlistment enlistment) + : this(tracer, retryConfig, enlistment, handlerOverride: null) + { + } + + /// + /// Test-only constructor that injects the so + /// can run without network I/O. Production code uses + /// the overload that builds a configured . + /// + internal HttpRequestor( + ITracer tracer, + RetryConfig retryConfig, + Enlistment enlistment, + HttpMessageHandler handlerOverride) { this.RetryConfig = retryConfig; @@ -52,23 +66,28 @@ protected HttpRequestor(ITracer tracer, RetryConfig retryConfig, Enlistment enli TryApplyConnectionLimitFromConfig(tracer, enlistment); } - // WARNING: Do NOT set Credentials or ServerCredentials on this handler. - // - // Setting Credentials = CredentialCache.DefaultCredentials causes the handler - // to perform an NTLM/Negotiate challenge-response on every new connection. - // On SocketsHttpHandler this adds ~400ms per request vs ~14ms without. - // - // 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 = handlerOverride; + if (handler == null) { - MaxConnectionsPerServer = Environment.ProcessorCount, - PooledConnectionLifetime = Timeout.InfiniteTimeSpan, - PooledConnectionIdleTimeout = TimeSpan.FromMinutes(5), - }; + // WARNING: Do NOT set Credentials or ServerCredentials on this handler. + // + // Setting Credentials = CredentialCache.DefaultCredentials causes the handler + // to perform an NTLM/Negotiate challenge-response on every new connection. + // On SocketsHttpHandler this adds ~400ms per request vs ~14ms without. + // + // 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 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) { @@ -98,25 +117,37 @@ public void Dispose() } } + /// Sends without credentials regardless of authentication state. protected GitEndPointResponseData SendRequest( long requestId, Uri requestUri, HttpMethod httpMethod, string requestContent, CancellationToken cancellationToken, - MediaTypeWithQualityHeaderValue acceptType = null) + MediaTypeWithQualityHeaderValue acceptType = null, + bool forceAnonymous = false) { + // Before initialization starts, preserve anonymous requests. During + // initialization, callers wait for the auth decision to complete. + // Resolve the decision once so all request handling uses one value. + bool sendAnonymous = forceAnonymous || this.authentication.ShouldSendAnonymous; + string authString = null; string errorMessage; - if (!this.authentication.IsAnonymous && - !this.authentication.TryGetCredentials(this.Tracer, out authString, out errorMessage)) + if (!sendAnonymous) { - return new GitEndPointResponseData( - HttpStatusCode.Unauthorized, - new GitObjectsHttpException(HttpStatusCode.Unauthorized, errorMessage), - shouldRetry: true, - message: null, - onResponseDisposed: null); + if (!this.authentication.TryGetCredentials(this.Tracer, out authString, out errorMessage)) + { + return new GitEndPointResponseData( + HttpStatusCode.Unauthorized, + new GitObjectsHttpException(HttpStatusCode.Unauthorized, errorMessage), + shouldRetry: true, + message: null, + onResponseDisposed: null); + } + + // Initialization may finish anonymously while this request waits. + sendAnonymous = authString == null; } HttpRequestMessage request = new HttpRequestMessage(httpMethod, requestUri); @@ -127,7 +158,7 @@ protected GitEndPointResponseData SendRequest( request.Headers.UserAgent.Add(this.userAgentHeader); - if (!this.authentication.IsAnonymous) + if (!sendAnonymous) { request.Headers.Authorization = new AuthenticationHeaderValue("Basic", authString); } @@ -205,10 +236,14 @@ protected GitEndPointResponseData SendRequest( string contentType = GetSingleHeaderOrEmpty(response.Content.Headers, "Content-Type"); responseMetadata.Add("ContentType", contentType); - if (!this.authentication.IsAnonymous) + if (!sendAnonymous) { this.authentication.ApproveCredentials(this.Tracer, authString); } + else if (!forceAnonymous) + { + this.authentication.MarkAnonymousAccessSucceeded(); + } Stream responseStream = response.Content.ReadAsStreamAsync().GetAwaiter().GetResult(); @@ -226,11 +261,21 @@ protected GitEndPointResponseData SendRequest( bool shouldRetry = ShouldRetry(response.StatusCode); - if (response.StatusCode == HttpStatusCode.Unauthorized && - this.authentication.IsAnonymous) + if (sendAnonymous && + ShouldRejectCredentials(response.StatusCode, errorMessage)) { - shouldRetry = false; - errorMessage = "Anonymous request was rejected with a 401"; + // A forced anonymous request is the initialization probe. Its + // caller owns the state transition and authenticated retry. + shouldRetry = !forceAnonymous; + if (!forceAnonymous) + { + this.authentication.MarkAuthenticationRequired(); + } + + errorMessage = string.Format( + "Anonymous request was rejected with error code {0} ({1})", + statusInt, + response.StatusCode); } else if (ShouldRejectCredentials(response.StatusCode, errorMessage)) { diff --git a/GVFS/GVFS.Common/Http/IGVFSConfigRequestor.cs b/GVFS/GVFS.Common/Http/IGVFSConfigRequestor.cs new file mode 100644 index 0000000000..85126896ba --- /dev/null +++ b/GVFS/GVFS.Common/Http/IGVFSConfigRequestor.cs @@ -0,0 +1,33 @@ +using System; +using System.Net; + +namespace GVFS.Common.Http +{ + /// + /// Queries the server's /gvfs/config endpoint. Extracted from + /// so that + /// can be + /// driven with a deterministic probe outcome in unit tests. Production code + /// always uses . + /// + internal interface IGVFSConfigRequestor : IDisposable + { + /// + /// Queries /gvfs/config. + /// + /// Whether to trace failures as errors. + /// The parsed config when this returns true. + /// + /// The HTTP status the server responded with, or null when the request + /// never produced an HTTP response (for example a DNS or socket failure). + /// + /// The failure description when this returns false. + /// + /// Sends the request without credentials, regardless of authentication + /// state. The initial probe must set this because it determines whether + /// the server allows anonymous access. + /// + /// True when the config was retrieved. + bool TryQueryGVFSConfig(bool logErrors, out ServerGVFSConfig serverGVFSConfig, out HttpStatusCode? httpStatus, out string errorMessage, bool forceAnonymous = false); + } +} diff --git a/GVFS/GVFS.Common/InternalsVisibleTo.cs b/GVFS/GVFS.Common/InternalsVisibleTo.cs index 0ba48d81be..f25a75048b 100644 --- a/GVFS/GVFS.Common/InternalsVisibleTo.cs +++ b/GVFS/GVFS.Common/InternalsVisibleTo.cs @@ -1,3 +1,4 @@ using System.Runtime.CompilerServices; [assembly: InternalsVisibleTo("GVFS.UnitTests")] +[assembly: InternalsVisibleTo("GVFS.Mount")] diff --git a/GVFS/GVFS.Mount/InProcessMount.cs b/GVFS/GVFS.Mount/InProcessMount.cs index 579c65207e..0224a167e4 100644 --- a/GVFS/GVFS.Mount/InProcessMount.cs +++ b/GVFS/GVFS.Mount/InProcessMount.cs @@ -160,6 +160,10 @@ private void MountWithLockAcquired(EventLevel verbosity, Keywords keywords) }, Keywords.Telemetry); + // Deliberately unconditional: callbacks must wait for the auth result in + // every mount, not only when background auth is enabled. + this.enlistment.Authentication.MarkInitializationStarted(); + // With background auth enabled, 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. A diff --git a/GVFS/GVFS.UnitTests/Git/GitAuthenticationTests.cs b/GVFS/GVFS.UnitTests/Git/GitAuthenticationTests.cs index 6ebc4cb9de..90b0624b19 100644 --- a/GVFS/GVFS.UnitTests/Git/GitAuthenticationTests.cs +++ b/GVFS/GVFS.UnitTests/Git/GitAuthenticationTests.cs @@ -1,15 +1,21 @@ using System; +using System.Collections.Generic; using System.Diagnostics; using System.Linq; +using System.Net; +using System.Net.Http; using System.Text; 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; using GVFS.Tests.Should; using GVFS.UnitTests.Mock.Common; using GVFS.UnitTests.Mock.Git; +using GVFS.UnitTests.Mock.Http; using NUnit.Framework; namespace GVFS.UnitTests.Git @@ -639,6 +645,397 @@ public void TryGetCertificatePasswordLogsConsoleVisibilityWhenCredentialFillFail } } + [TestCase] + public void InitialConfigProbeIsSentWithoutCredentials() + { + MockTracer tracer = new MockTracer(); + MockGitProcess gitProcess = this.GetGitProcess(); + MockGVFSConfigRequestor requestor = MockGVFSConfigRequestor.AnonymousSucceeds(); + + GitAuthentication dut = new GitAuthentication(gitProcess, "mock://repoUrl"); + dut.ConfigRequestorOverride = requestor; + + dut.TryInitializeAndQueryGVFSConfig(tracer, null, new RetryConfig(), out _, out _, out _) + .ShouldEqual(true, "The anonymous probe should have succeeded"); + + requestor.LastQueryForcedAnonymous.ShouldEqual(true, "The initial config probe must be forced anonymous so it cannot wait on its own initialization"); + } + + [TestCase] + public void SendRequestOmitsAuthorizationBeforeInitializationAndWhenForcedAnonymous() + { + MockTracer tracer = new MockTracer(); + MockGitProcess gitProcess = this.GetGitProcess(); + GitAuthentication dut = new GitAuthentication(gitProcess, "mock://repoUrl"); + MockGVFSEnlistment enlistment = new MockGVFSEnlistment(dut); + TestHttpMessageHandler handler = new TestHttpMessageHandler(); + + using (TestHttpRequestor requestor = new TestHttpRequestor(tracer, new RetryConfig(), enlistment, handler)) + { + using (GitEndPointResponseData response = requestor.SendRequest( + new Uri("https://mock.invalid/gvfs/config"), + forceAnonymous: false)) + { + response.HasErrors.ShouldEqual(false, "The uninitialized request should succeed anonymously"); + } + + using (GitEndPointResponseData response = requestor.SendRequest( + new Uri("https://mock.invalid/gvfs/config"), + forceAnonymous: true)) + { + response.HasErrors.ShouldEqual(false, "The forced-anonymous request should succeed"); + } + } + + handler.AuthorizationHeaders.Count.ShouldEqual(2, "The handler should receive both requests"); + handler.AuthorizationHeaders[0].ShouldEqual(null, "A request before initialization must remain anonymous"); + handler.AuthorizationHeaders[1].ShouldEqual(null, "A forced-anonymous request must omit Authorization"); + } + + [TestCase] + public void SendRequestSendsAuthorizationAfterAuthentication() + { + MockTracer tracer = new MockTracer(); + MockGitProcess gitProcess = this.GetGitProcess(); + GitAuthentication dut = new GitAuthentication(gitProcess, "mock://repoUrl"); + MockGVFSEnlistment enlistment = new MockGVFSEnlistment(dut); + TestHttpMessageHandler handler = new TestHttpMessageHandler(); + + dut.TryInitializeAndRequireAuth(tracer, out _) + .ShouldEqual(true, "The mock credential should initialize successfully"); + dut.TryGetCredentials(tracer, out string authString, out _) + .ShouldEqual(true, "The initialized credential should be available"); + + using (TestHttpRequestor requestor = new TestHttpRequestor(tracer, new RetryConfig(), enlistment, handler)) + { + using (GitEndPointResponseData response = requestor.SendRequest( + new Uri("https://mock.invalid/gvfs/config"), + forceAnonymous: false)) + { + response.HasErrors.ShouldEqual(false, "The authenticated request should succeed"); + } + + using (GitEndPointResponseData response = requestor.SendRequest( + new Uri("https://mock.invalid/gvfs/config"), + forceAnonymous: true)) + { + response.HasErrors.ShouldEqual(false, "The forced-anonymous request should succeed"); + } + } + + handler.AuthorizationHeaders.Count.ShouldEqual(2, "The handler should receive both requests"); + handler.AuthorizationHeaders[0].ShouldEqual( + "Basic " + authString, + "An authenticated request should send the cached credential"); + handler.AuthorizationHeaders[1].ShouldEqual(null, "forceAnonymous must suppress Authorization after initialization too"); + } + + [TestCase] + public void SendRequestWaitsForAnonymousInitializationWithoutFetchingCredentials() + { + MockTracer tracer = new MockTracer(); + MockGitProcess gitProcess = this.GetGitProcess(); + GitAuthentication dut = new GitAuthentication(gitProcess, "mock://repoUrl"); + MockGVFSEnlistment enlistment = new MockGVFSEnlistment(dut); + MockGVFSConfigRequestor configRequestor = MockGVFSConfigRequestor.AnonymousSucceeds(); + TestHttpMessageHandler handler = new TestHttpMessageHandler(); + using ManualResetEventSlim probeStarted = new ManualResetEventSlim(false); + using ManualResetEventSlim continueProbe = new ManualResetEventSlim(false); + using ManualResetEventSlim requestStarted = new ManualResetEventSlim(false); + configRequestor.BeforeQueryResult = _ => + { + probeStarted.Set(); + continueProbe.Wait(); + }; + dut.ConfigRequestorOverride = configRequestor; + dut.MarkInitializationStarted(); + + Task initialize = Task.Run(() => + dut.TryInitializeAndQueryGVFSConfig(tracer, enlistment, new RetryConfig(), out _, out _, out _)); + using TestHttpRequestor requestor = new TestHttpRequestor(tracer, new RetryConfig(), enlistment, handler); + Task send = null; + try + { + probeStarted.Wait(TimeSpan.FromSeconds(5)).ShouldBeTrue("Authentication probe should start"); + send = Task.Run(() => + { + requestStarted.Set(); + using GitEndPointResponseData response = requestor.SendRequest( + new Uri("https://mock.invalid/gvfs/config"), + forceAnonymous: false); + return !response.HasErrors; + }); + + requestStarted.Wait(TimeSpan.FromSeconds(5)).ShouldBeTrue("HTTP request should start"); + Thread.Sleep(100); + send.IsCompleted.ShouldEqual(false, "The HTTP request should wait for auth initialization"); + } + finally + { + continueProbe.Set(); + } + + Task.WaitAll(send == null ? new[] { initialize } : new Task[] { initialize, send }); + initialize.Result.ShouldEqual(true, "Anonymous config initialization should succeed"); + if (send != null) + { + send.Result.ShouldEqual(true, "The waiting request should continue after anonymous initialization"); + } + + handler.AuthorizationHeaders.Count.ShouldEqual(1, "The handler should receive one request"); + handler.AuthorizationHeaders[0].ShouldEqual(null, "The waiting request must not attach Authorization"); + gitProcess.CommandsRun.Any(command => command.Contains("credential fill")) + .ShouldEqual(false, "Anonymous initialization must not trigger credential lookup"); + } + + [TestCase] + public void SendRequestOmitsAuthorizationWhenForcedAnonymousAfterInitialization() + { + MockTracer tracer = new MockTracer(); + MockGitProcess gitProcess = this.GetGitProcess(); + GitAuthentication dut = new GitAuthentication(gitProcess, "mock://repoUrl"); + MockGVFSEnlistment enlistment = new MockGVFSEnlistment(dut); + TestHttpMessageHandler handler = new TestHttpMessageHandler(); + + dut.TryInitializeAndRequireAuth(tracer, out _).ShouldEqual(true, "Authentication should initialize"); + + using (TestHttpRequestor requestor = new TestHttpRequestor(tracer, new RetryConfig(), enlistment, handler)) + using (GitEndPointResponseData response = requestor.SendRequest( + new Uri("https://mock.invalid/gvfs/config"), + forceAnonymous: true)) + { + response.HasErrors.ShouldEqual(false, "The forced-anonymous request should succeed"); + } + + handler.AuthorizationHeaders.Count.ShouldEqual(1, "The handler should receive one request"); + handler.AuthorizationHeaders[0].ShouldEqual(null, "forceAnonymous must suppress Authorization after initialization"); + } + + [TestCase] + public void InitializationExceptionReleasesCredentialWaiters() + { + MockTracer tracer = new MockTracer(); + MockGitProcess gitProcess = this.GetGitProcess(); + GitAuthentication dut = new GitAuthentication(gitProcess, "mock://repoUrl") + { + InitializationWaitTimeoutMs = 100, + ConfigRequestorOverride = MockGVFSConfigRequestor.Indeterminate(HttpStatusCode.RequestTimeout), + }; + MockGVFSConfigRequestor requestor = (MockGVFSConfigRequestor)dut.ConfigRequestorOverride; + requestor.BeforeQueryResult = _ => throw new InvalidOperationException("probe failed"); + + Assert.Throws(() => + dut.TryInitializeAndQueryGVFSConfig(tracer, null, new RetryConfig(), out _, out _, out _)); + + dut.TryGetCredentials(tracer, out string authString, out string error) + .ShouldEqual(true, "Waiters should be released after the probe throws: " + error); + authString.ShouldEqual(null, "An indeterminate probe should leave later requests anonymous until rejected"); + } + + [TestCase] + public void FailedTestAuthInitializationReleasesCredentialWaiters() + { + MockTracer tracer = new MockTracer(); + MockGitProcess gitProcess = this.GetGitProcess(); + GitAuthentication dut = new GitAuthentication(gitProcess, "mock://repoUrl") + { + InitializationWaitTimeoutMs = 100, + }; + gitProcess.ShouldFail = true; + + dut.TryInitializeAndRequireAuth(tracer, out _) + .ShouldEqual(false, "The forced credential failure should be reported"); + + dut.TryGetCredentials(tracer, out _, out string error) + .ShouldEqual(false, "A failed credential fetch should remain a failure"); + error.Contains("Timed out waiting for authentication to initialize") + .ShouldEqual(false, "Credential failure must not be reported as an initialization timeout"); + } + + [TestCase] + public void AuthIsNotAnonymousBeforeInitialization() + { + MockGitProcess gitProcess = this.GetGitProcess(); + + GitAuthentication dut = new GitAuthentication(gitProcess, "mock://repoUrl"); + + dut.IsAnonymous.ShouldEqual(false, "Auth must not report anonymous before a probe has proven the server allows it"); + } + + [TestCase] + public void AnonymousConfigProbeSuccessLeavesAuthAnonymous() + { + MockTracer tracer = new MockTracer(); + MockGitProcess gitProcess = this.GetGitProcess(); + MockGVFSConfigRequestor requestor = MockGVFSConfigRequestor.AnonymousSucceeds(); + + GitAuthentication dut = new GitAuthentication(gitProcess, "mock://repoUrl"); + dut.ConfigRequestorOverride = requestor; + + dut.TryInitializeAndQueryGVFSConfig(tracer, null, new RetryConfig(), out _, out _, out bool isAuthFailure) + .ShouldEqual(true, "An anonymous server should initialize successfully"); + + dut.IsAnonymous.ShouldEqual(true, "A successful unauthenticated probe means the server allows anonymous access"); + isAuthFailure.ShouldEqual(false, "An anonymous success is not an auth failure"); + requestor.QueryCount.ShouldEqual(1, "An anonymous server needs only the single unauthenticated query"); + } + + [TestCase] + public void TryGetCredentialsDoesNotFetchCredentialsForAnonymousServer() + { + MockTracer tracer = new MockTracer(); + MockGitProcess gitProcess = this.GetGitProcess(); + GitAuthentication dut = new GitAuthentication(gitProcess, "mock://repoUrl"); + dut.ConfigRequestorOverride = MockGVFSConfigRequestor.AnonymousSucceeds(); + + dut.TryInitializeAndQueryGVFSConfig(tracer, null, new RetryConfig(), out _, out _, out _) + .ShouldEqual(true, "The anonymous config probe should succeed"); + dut.TryGetCredentials(tracer, out string authString, out string error) + .ShouldEqual(true, "An anonymous server does not require credentials: " + error); + + authString.ShouldEqual(null, "No credential is needed for an anonymous server"); + gitProcess.CommandsRun.Any(command => command.Contains("credential fill")) + .ShouldEqual(false, "Do not prompt for credentials after an anonymous probe"); + } + + [TestCase] + public void UnauthorizedConfigProbeRequiresAuthentication() + { + MockTracer tracer = new MockTracer(); + MockGitProcess gitProcess = this.GetGitProcess(); + + GitAuthentication dut = new GitAuthentication(gitProcess, "mock://repoUrl"); + MockGVFSConfigRequestor requestor = MockGVFSConfigRequestor.RequiresAuthenticationThenSucceeds(); + dut.ConfigRequestorOverride = requestor; + + dut.TryInitializeAndQueryGVFSConfig(tracer, null, new RetryConfig(), out ServerGVFSConfig config, out _, out bool isAuthFailure) + .ShouldEqual(true, "The authenticated retry should retrieve config"); + + dut.IsAnonymous.ShouldEqual(false, "A 401 from the unauthenticated probe means credentials are required"); + config.ShouldNotBeNull("The authenticated probe should return config"); + isAuthFailure.ShouldEqual(false, "A successful authenticated retry is not an auth failure"); + requestor.QueryCount.ShouldEqual(2, "The requestor should probe once anonymously and once with credentials"); + dut.TryGetCredentials(tracer, out string authString, out _).ShouldEqual(true, "Credentials should be available after a 401 probe"); + authString.ShouldNotBeNull("A credential should have been fetched"); + } + + [TestCase] + public void UnauthorizedConfigProbeWithRejectedCredentialsIsAuthFailure() + { + MockTracer tracer = new MockTracer(); + GitAuthentication dut = new GitAuthentication(this.GetGitProcess(), "mock://repoUrl"); + MockGVFSConfigRequestor requestor = MockGVFSConfigRequestor.RequiresAuthentication(); + dut.ConfigRequestorOverride = requestor; + + dut.TryInitializeAndQueryGVFSConfig(tracer, null, new RetryConfig(), out _, out _, out bool isAuthFailure) + .ShouldEqual(false, "Rejected credentials must fail initialization"); + + isAuthFailure.ShouldEqual(true, "A 401 after authenticating is an authentication failure"); + dut.IsAnonymous.ShouldEqual(false, "A 401 probe must never leave auth anonymous"); + } + + [TestCase(HttpStatusCode.RequestTimeout)] + [TestCase(HttpStatusCode.InternalServerError)] + [TestCase(HttpStatusCode.ServiceUnavailable)] + [TestCase(HttpStatusCode.BadRequest)] + [TestCase(null)] + public void IndeterminateConfigProbeDoesNotLeaveAuthAnonymous(HttpStatusCode? status) + { + MockTracer tracer = new MockTracer(); + MockGitProcess gitProcess = this.GetGitProcess(); + + GitAuthentication dut = new GitAuthentication(gitProcess, "mock://repoUrl"); + dut.ConfigRequestorOverride = MockGVFSConfigRequestor.Indeterminate(status); + + dut.TryInitializeAndQueryGVFSConfig(tracer, null, new RetryConfig(), out _, out _, out bool isAuthFailure) + .ShouldEqual(false, $"A config query that failed with {status?.ToString() ?? "no response"} should report failure"); + + dut.IsAnonymous.ShouldEqual( + false, + $"An indeterminate config probe ({status?.ToString() ?? "no response"}) must not leave auth in anonymous mode"); + + isAuthFailure.ShouldEqual(false, "An indeterminate failure is not an authentication failure"); + tracer.RelatedWarningEvents.Count.ShouldEqual(1, "The probe should emit one warning"); + tracer.RelatedWarningKeywords.Count.ShouldEqual(1, "Each warning should record its keyword"); + tracer.RelatedWarningKeywords[0].ShouldEqual(Keywords.Telemetry, "Indeterminate authentication telemetry must use the telemetry keyword"); + tracer.RelatedWarningEvents[0].ShouldContain("HttpStatus", status?.ToString() ?? "None"); + tracer.RelatedWarningEvents[0].ShouldContain("AuthenticationState", "Unknown"); + tracer.RelatedWarningEvents[0].ShouldContain("ProbeError", "Mock config query failure"); + } + + [TestCase] + public void IndeterminateConfigProbeAllowsLaterAnonymousSuccess() + { + MockTracer tracer = new MockTracer(); + MockGitProcess gitProcess = this.GetGitProcess(); + GitAuthentication dut = new GitAuthentication(gitProcess, "mock://repoUrl"); + MockGVFSEnlistment enlistment = new MockGVFSEnlistment(dut); + TestHttpMessageHandler handler = new TestHttpMessageHandler(); + dut.ConfigRequestorOverride = MockGVFSConfigRequestor.Indeterminate(HttpStatusCode.RequestTimeout); + + dut.TryInitializeAndQueryGVFSConfig(tracer, null, new RetryConfig(), out _, out _, out _); + + using (TestHttpRequestor requestor = new TestHttpRequestor(tracer, new RetryConfig(), enlistment, handler)) + using (GitEndPointResponseData response = requestor.SendRequest( + new Uri("https://mock.invalid/gvfs/objects"), + forceAnonymous: false)) + { + response.HasErrors.ShouldEqual(false, "A later anonymous success should complete the request"); + } + + handler.AuthorizationHeaders.Single().ShouldEqual(null, "Unknown authentication must try anonymously first"); + dut.IsAnonymous.ShouldEqual(true, "A successful anonymous request should resolve the state"); + gitProcess.CommandsRun.Any(command => command.Contains("credential fill")) + .ShouldEqual(false, "Anonymous success must not prompt for credentials"); + } + + [TestCase(false, HttpStatusCode.Unauthorized, "Unauthorized")] + [TestCase(false, HttpStatusCode.BadRequest, HttpRequestor.CacheServerAuthRequiredBadRequestMessage)] + [TestCase(true, HttpStatusCode.Unauthorized, "Unauthorized")] + [TestCase(true, HttpStatusCode.BadRequest, HttpRequestor.CacheServerAuthRequiredBadRequestMessage)] + public void AnonymousAuthFailurePromotesLaterRequestToAuthenticated( + bool startKnownAnonymous, + HttpStatusCode statusCode, + string responseBody) + { + MockTracer tracer = new MockTracer(); + MockGitProcess gitProcess = this.GetGitProcess(); + GitAuthentication dut = new GitAuthentication(gitProcess, "mock://repoUrl"); + MockGVFSEnlistment enlistment = new MockGVFSEnlistment(dut); + TestHttpMessageHandler handler = new TestHttpMessageHandler(); + dut.ConfigRequestorOverride = startKnownAnonymous + ? MockGVFSConfigRequestor.AnonymousSucceeds() + : MockGVFSConfigRequestor.Indeterminate(HttpStatusCode.RequestTimeout); + + dut.TryInitializeAndQueryGVFSConfig(tracer, null, new RetryConfig(), out _, out _, out _); + handler.EnqueueResponse(statusCode, responseBody); + handler.EnqueueResponse(HttpStatusCode.OK, "{}"); + + using (TestHttpRequestor requestor = new TestHttpRequestor(tracer, new RetryConfig(), enlistment, handler)) + { + using (GitEndPointResponseData response = requestor.SendRequest( + new Uri("https://mock.invalid/gvfs/objects"), + forceAnonymous: false)) + { + response.HasErrors.ShouldEqual(true, "The rejected anonymous attempt should report its error"); + response.ShouldRetry.ShouldEqual(true, "The rejected anonymous attempt should retry with credentials"); + } + + using (GitEndPointResponseData response = requestor.SendRequest( + new Uri("https://mock.invalid/gvfs/objects"), + forceAnonymous: false)) + { + response.HasErrors.ShouldEqual(false, "The authenticated retry should succeed"); + } + } + + handler.AuthorizationHeaders.Count.ShouldEqual(2, "Both attempts should reach the server"); + handler.AuthorizationHeaders[0].ShouldEqual(null, "The first attempt should be anonymous"); + handler.AuthorizationHeaders[1].StartsWith("Basic ", StringComparison.Ordinal) + .ShouldEqual(true, "The retry should carry the credential"); + dut.IsAnonymous.ShouldEqual(false, "The auth-required response should promote the state"); + } + private MockGitProcess GetGitProcess() { MockGitProcess gitProcess = new MockGitProcess(); @@ -677,5 +1074,53 @@ private MockGitProcess GetGitProcess() }); return gitProcess; } + + private sealed class TestHttpRequestor : HttpRequestor + { + public TestHttpRequestor( + MockTracer tracer, + RetryConfig retryConfig, + MockGVFSEnlistment enlistment, + HttpMessageHandler httpMessageHandler) + : base(tracer, retryConfig, enlistment, httpMessageHandler) + { + } + + public GitEndPointResponseData SendRequest(Uri requestUri, bool forceAnonymous) + { + return this.SendRequest( + HttpRequestor.GetNewRequestId(), + requestUri, + HttpMethod.Get, + requestContent: null, + CancellationToken.None, + forceAnonymous: forceAnonymous); + } + } + + private sealed class TestHttpMessageHandler : HttpMessageHandler + { + private readonly Queue> responses = new Queue>(); + + public List AuthorizationHeaders { get; } = new List(); + + public void EnqueueResponse(HttpStatusCode statusCode, string content) + { + this.responses.Enqueue(new KeyValuePair(statusCode, content)); + } + + protected override Task SendAsync(HttpRequestMessage request, CancellationToken cancellationToken) + { + this.AuthorizationHeaders.Add(request.Headers.Authorization?.ToString()); + KeyValuePair response = this.responses.Count == 0 + ? new KeyValuePair(HttpStatusCode.OK, "{}") + : this.responses.Dequeue(); + + return Task.FromResult(new HttpResponseMessage(response.Key) + { + Content = new StringContent(response.Value), + }); + } + } } } diff --git a/GVFS/GVFS.UnitTests/Mock/Common/MockGVFSEnlistment.cs b/GVFS/GVFS.UnitTests/Mock/Common/MockGVFSEnlistment.cs index 2292149b65..6fb128930d 100644 --- a/GVFS/GVFS.UnitTests/Mock/Common/MockGVFSEnlistment.cs +++ b/GVFS/GVFS.UnitTests/Mock/Common/MockGVFSEnlistment.cs @@ -10,7 +10,12 @@ public class MockGVFSEnlistment : GVFSEnlistment private MockGitProcess gitProcess; public MockGVFSEnlistment() - : base(Path.Combine("mock:", "path"), "mock://repoUrl", Path.Combine("mock:", "git"), authentication: null) + : this(authentication: null) + { + } + + public MockGVFSEnlistment(GitAuthentication authentication) + : base(Path.Combine("mock:", "path"), "mock://repoUrl", Path.Combine("mock:", "git"), authentication) { this.GitObjectsRoot = Path.Combine("mock:", "path", ".git", "objects"); this.LocalObjectsRoot = this.GitObjectsRoot; diff --git a/GVFS/GVFS.UnitTests/Mock/Common/MockTracer.cs b/GVFS/GVFS.UnitTests/Mock/Common/MockTracer.cs index 1ad0c9690d..caa2faa15d 100644 --- a/GVFS/GVFS.UnitTests/Mock/Common/MockTracer.cs +++ b/GVFS/GVFS.UnitTests/Mock/Common/MockTracer.cs @@ -19,6 +19,7 @@ public MockTracer() this.waitEvent = new AutoResetEvent(false); this.RelatedInfoEvents = new List(); this.RelatedWarningEvents = new List(); + this.RelatedWarningKeywords = new List(); this.RelatedErrorEvents = new List(); this.RelatedEventNames = new List(); this.RelatedEventKeywords = new List(); @@ -33,6 +34,7 @@ public MockTracer() public List RelatedInfoEvents { get; } public List RelatedWarningEvents { get; } + public List RelatedWarningKeywords { get; } public List RelatedErrorEvents { get; } // Names of events reported via RelatedEvent (which, unlike RelatedInfo/Warning/Error, @@ -106,21 +108,29 @@ public void RelatedInfo(string format, params object[] args) } public void RelatedWarning(EventMetadata metadata, string message) + { + this.AddRelatedWarning(metadata, message, Keywords.None); + } + + public void RelatedWarning(EventMetadata metadata, string message, Keywords keyword) + { + this.AddRelatedWarning(metadata, message, keyword); + } + + private void AddRelatedWarning(EventMetadata metadata, string message, Keywords keyword) { if (metadata != null) { metadata[TracingConstants.MessageKey.WarningMessage] = message; this.RelatedWarningEvents.Add(GVFSJsonOptions.Serialize(metadata)); + this.RelatedWarningKeywords.Add(keyword); } else if (message != null) { - this.RelatedWarning(message); + this.RelatedWarningEvents.Add(message); + this.RelatedWarningKeywords.Add(keyword); } - } - public void RelatedWarning(EventMetadata metadata, string message, Keywords keyword) - { - this.RelatedWarning(metadata, message); if (metadata != null && (keyword & Keywords.Telemetry) != 0) { this.TelemetryWarnings.Add(metadata); @@ -129,11 +139,13 @@ public void RelatedWarning(EventMetadata metadata, string message, Keywords keyw public void RelatedWarning(string message) { + this.RelatedWarningKeywords.Add(Keywords.None); this.RelatedWarningEvents.Add(message); } public void RelatedWarning(string format, params object[] args) { + this.RelatedWarningKeywords.Add(Keywords.None); this.RelatedWarningEvents.Add(string.Format(format, args)); } diff --git a/GVFS/GVFS.UnitTests/Mock/Http/MockGVFSConfigRequestor.cs b/GVFS/GVFS.UnitTests/Mock/Http/MockGVFSConfigRequestor.cs new file mode 100644 index 0000000000..b104a77ad2 --- /dev/null +++ b/GVFS/GVFS.UnitTests/Mock/Http/MockGVFSConfigRequestor.cs @@ -0,0 +1,101 @@ +using GVFS.Common; +using GVFS.Common.Http; +using System; +using System.Net; + +namespace GVFS.UnitTests.Mock.Http +{ + /// + /// Drives + /// with a fixed /gvfs/config probe outcome so the anonymous / authenticated / + /// indeterminate branches can be tested without a server. + /// + internal class MockGVFSConfigRequestor : IGVFSConfigRequestor + { + private readonly bool succeedAnonymously; + private readonly bool succeedAfterAuthentication; + private readonly HttpStatusCode? statusCode; + + private MockGVFSConfigRequestor(bool succeedAnonymously, HttpStatusCode? statusCode, bool succeedAfterAuthentication = false) + { + this.succeedAnonymously = succeedAnonymously; + this.statusCode = statusCode; + this.succeedAfterAuthentication = succeedAfterAuthentication; + } + + /// + /// Number of times the probe was issued. + /// + public int QueryCount { get; private set; } + + /// + /// The value of on the most recent probe. + /// + public bool LastQueryForcedAnonymous { get; private set; } + + public Action BeforeQueryResult { get; set; } + + /// + /// The server allows anonymous access: the unauthenticated probe returns the config. + /// + public static MockGVFSConfigRequestor AnonymousSucceeds() + { + return new MockGVFSConfigRequestor(succeedAnonymously: true, statusCode: HttpStatusCode.OK); + } + + /// + /// The server requires authentication: the unauthenticated probe returns 401. + /// + public static MockGVFSConfigRequestor RequiresAuthentication() + { + return new MockGVFSConfigRequestor(succeedAnonymously: false, statusCode: HttpStatusCode.Unauthorized); + } + + /// + /// The initial unauthenticated query is rejected, then the authenticated retry succeeds. + /// + public static MockGVFSConfigRequestor RequiresAuthenticationThenSucceeds() + { + return new MockGVFSConfigRequestor( + succeedAnonymously: false, + statusCode: HttpStatusCode.Unauthorized, + succeedAfterAuthentication: true); + } + + /// + /// The probe failed for a reason that says nothing about authentication, so + /// whether the server allows anonymous access is unknown. Pass null for + /// to model a failure with no HTTP response at all. + /// + public static MockGVFSConfigRequestor Indeterminate(HttpStatusCode? statusCode) + { + return new MockGVFSConfigRequestor(succeedAnonymously: false, statusCode: statusCode); + } + + public bool TryQueryGVFSConfig(bool logErrors, out ServerGVFSConfig serverGVFSConfig, out HttpStatusCode? httpStatus, out string errorMessage, bool forceAnonymous = false) + { + this.QueryCount++; + this.LastQueryForcedAnonymous = forceAnonymous; + this.BeforeQueryResult?.Invoke(forceAnonymous); + + bool querySucceeded = this.succeedAnonymously || + (this.succeedAfterAuthentication && this.QueryCount > 1 && !forceAnonymous); + httpStatus = querySucceeded ? HttpStatusCode.OK : this.statusCode; + + if (querySucceeded) + { + serverGVFSConfig = new ServerGVFSConfig(); + errorMessage = null; + return true; + } + + serverGVFSConfig = null; + errorMessage = "Mock config query failure"; + return false; + } + + public void Dispose() + { + } + } +} diff --git a/GVFS/GVFS.UnitTests/Virtualization/FileSystemCallbacksTests.cs b/GVFS/GVFS.UnitTests/Virtualization/FileSystemCallbacksTests.cs index 856b0c6f1b..14cbe1b2b8 100644 --- a/GVFS/GVFS.UnitTests/Virtualization/FileSystemCallbacksTests.cs +++ b/GVFS/GVFS.UnitTests/Virtualization/FileSystemCallbacksTests.cs @@ -123,7 +123,7 @@ public void GetMetadataForHeartBeatDoesNotChangeEventLevelWhenNoPlaceholderHaveB metadata.ShouldContain("ModifiedPathsCount", 1); metadata.ShouldContain("FilePlaceholderCount", 0); metadata.ShouldContain(nameof(RepoMetadata.Instance.EnlistmentId), RepoMetadata.Instance.EnlistmentId); - metadata.ShouldContain("IsSendingAnonymousRequests", true); + metadata.ShouldContain("IsSendingAnonymousRequests", false); } mockPlaceholderDb.VerifyAll(); @@ -174,7 +174,7 @@ public void GetMetadataForHeartBeatDoesSetsEventLevelToInformationalWhenPlacehol metadata.ShouldContain("FolderPlaceholderCount", 0); metadata.ShouldContain(nameof(RepoMetadata.Instance.EnlistmentId), RepoMetadata.Instance.EnlistmentId); metadata.ContainsKey("PhysicalDiskInfo").ShouldBeTrue(); - metadata.ShouldContain("IsSendingAnonymousRequests", true); + metadata.ShouldContain("IsSendingAnonymousRequests", false); // Create more placeholders fileSystemCallbacks.OnPlaceholderFileCreated("test.txt", "2222233333444445555566666777778888899999", "GVFS.UnitTests.exe2"); @@ -208,7 +208,7 @@ public void GetMetadataForHeartBeatDoesSetsEventLevelToInformationalWhenPlacehol metadata.ShouldContain("FolderPlaceholderCount", 1); metadata.ShouldContain(nameof(RepoMetadata.Instance.EnlistmentId), RepoMetadata.Instance.EnlistmentId); metadata.ContainsKey("PhysicalDiskInfo").ShouldBeTrue(); - metadata.ShouldContain("IsSendingAnonymousRequests", true); + metadata.ShouldContain("IsSendingAnonymousRequests", false); } mockPlaceholderDb.VerifyAll(); From a33ca20ce1c93d6f95078a8b1aeca571e2caf46f Mon Sep 17 00:00:00 2001 From: Tyrie Vella Date: Mon, 2 Nov 2026 16:32:49 -0800 Subject: [PATCH 2/3] Fix authentication promotion and retry accounting Make an authentication challenge promote requests that run before explicit initialization. Report the actual anonymous request mode in heartbeat telemetry and trace runtime authentication transitions. Preserve authentication challenge response details. Keep expected anonymous challenges out of global circuit-breaker accounting while retaining their credentialed retry. Add tests for pre-initialization promotion, transition tracing, response details, heartbeat telemetry, and circuit-breaker behavior. Assisted-by: GPT-5.6 Sol Signed-off-by: Tyrie Vella --- GVFS/GVFS.Common/Git/GitAuthentication.cs | 18 +++++++++ GVFS/GVFS.Common/Http/ConfigHttpRequestor.cs | 6 ++- .../Http/GitEndPointResponseData.cs | 11 ++++- .../Http/GitObjectsHttpRequestor.cs | 14 +++++-- GVFS/GVFS.Common/Http/HttpRequestor.cs | 24 +++++++++-- .../Git/GitAuthenticationTests.cs | 40 +++++++++++++++++++ .../Http/GitObjectsHttpRequestorTests.cs | 38 ++++++++++++++++-- .../FileSystemCallbacksTests.cs | 6 +-- .../FileSystemCallbacks.cs | 2 +- 9 files changed, 142 insertions(+), 17 deletions(-) diff --git a/GVFS/GVFS.Common/Git/GitAuthentication.cs b/GVFS/GVFS.Common/Git/GitAuthentication.cs index b11720b3f2..5f9db0c724 100644 --- a/GVFS/GVFS.Common/Git/GitAuthentication.cs +++ b/GVFS/GVFS.Common/Git/GitAuthentication.cs @@ -78,6 +78,8 @@ public bool IsAnonymous get => this.GetAuthenticationState() == AuthenticationState.Anonymous; } + public bool IsSendingAnonymousRequests => this.ShouldSendAnonymous; + internal bool ShouldSendAnonymous => !this.IsInitializationStarted || (this.initializationComplete.IsSet && this.GetAuthenticationState() != AuthenticationState.Authenticated); @@ -477,6 +479,22 @@ internal void MarkAuthenticationRequired() Interlocked.Exchange(ref this.authenticationState, (int)AuthenticationState.Authenticated); } + internal bool PromoteToAuthenticationRequired(out string previousAuthenticationState) + { + AuthenticationState previousState = (AuthenticationState)Interlocked.Exchange( + ref this.authenticationState, + (int)AuthenticationState.Authenticated); + + previousAuthenticationState = previousState.ToString(); + + if (Interlocked.CompareExchange(ref this.initializationStarted, 1, 0) == 0) + { + this.MarkInitialized(); + } + + return previousState != AuthenticationState.Authenticated; + } + private AuthenticationState GetAuthenticationState() { return (AuthenticationState)Volatile.Read(ref this.authenticationState); diff --git a/GVFS/GVFS.Common/Http/ConfigHttpRequestor.cs b/GVFS/GVFS.Common/Http/ConfigHttpRequestor.cs index 0c1b2ae77a..a341e778d2 100644 --- a/GVFS/GVFS.Common/Http/ConfigHttpRequestor.cs +++ b/GVFS/GVFS.Common/Http/ConfigHttpRequestor.cs @@ -61,7 +61,11 @@ public bool TryQueryGVFSConfig(bool logErrors, out ServerGVFSConfig serverGVFSCo { if (response.HasErrors) { - return new RetryWrapper.CallbackResult(response.Error, response.ShouldRetry); + return new RetryWrapper.CallbackResult( + response.Error, + response.ShouldRetry, + result: null, + shouldRecordFailure: response.ShouldRecordFailure); } try diff --git a/GVFS/GVFS.Common/Http/GitEndPointResponseData.cs b/GVFS/GVFS.Common/Http/GitEndPointResponseData.cs index 9eb6164219..0be18adc01 100644 --- a/GVFS/GVFS.Common/Http/GitEndPointResponseData.cs +++ b/GVFS/GVFS.Common/Http/GitEndPointResponseData.cs @@ -17,11 +17,18 @@ public class GitEndPointResponseData : IDisposable /// /// Constructor used when GitEndPointResponseData contains an error response /// - public GitEndPointResponseData(HttpStatusCode statusCode, Exception error, bool shouldRetry, HttpResponseMessage message, Action onResponseDisposed) + public GitEndPointResponseData( + HttpStatusCode statusCode, + Exception error, + bool shouldRetry, + HttpResponseMessage message, + Action onResponseDisposed, + bool shouldRecordFailure = true) { this.StatusCode = statusCode; this.Error = error; this.ShouldRetry = shouldRetry; + this.ShouldRecordFailure = shouldRetry && shouldRecordFailure; this.message = message; this.onResponseDisposed = onResponseDisposed; } @@ -40,6 +47,8 @@ public GitEndPointResponseData(HttpStatusCode statusCode, string contentType, St public bool ShouldRetry { get; } + public bool ShouldRecordFailure { get; } + public HttpStatusCode StatusCode { get; } public Stream Stream { get; private set; } diff --git a/GVFS/GVFS.Common/Http/GitObjectsHttpRequestor.cs b/GVFS/GVFS.Common/Http/GitObjectsHttpRequestor.cs index 278351b8ee..80d64030ec 100644 --- a/GVFS/GVFS.Common/Http/GitObjectsHttpRequestor.cs +++ b/GVFS/GVFS.Common/Http/GitObjectsHttpRequestor.cs @@ -121,7 +121,11 @@ public virtual List QueryForFileSizes(IEnumerable objectI if (response.HasErrors) { - return new RetryWrapper>.CallbackResult(response.Error, response.ShouldRetry); + return new RetryWrapper>.CallbackResult( + response.Error, + response.ShouldRetry, + result: null, + shouldRecordFailure: response.ShouldRecordFailure); } string objectSizesString = response.RetryableReadToEnd(); @@ -176,7 +180,11 @@ public virtual GitRefs QueryInfoRefs(string branch) { if (response.HasErrors) { - return new RetryWrapper.CallbackResult(response.Error, response.ShouldRetry); + return new RetryWrapper.CallbackResult( + response.Error, + response.ShouldRetry, + result: null, + shouldRecordFailure: response.ShouldRecordFailure); } List infoRefsResponse = response.RetryableReadAllLines(); @@ -438,7 +446,7 @@ public virtual RetryWrapper.InvocationResult TrySendProtoco response.Error, shouldFallBack || response.ShouldRetry, new GitObjectTaskResult(response.StatusCode, requestEndPoint), - shouldRecordFailure: response.ShouldRetry && !shouldFallBack); + shouldRecordFailure: response.ShouldRecordFailure && !shouldFallBack); } RetryWrapper.CallbackResult result; diff --git a/GVFS/GVFS.Common/Http/HttpRequestor.cs b/GVFS/GVFS.Common/Http/HttpRequestor.cs index 34e626b128..4ea8e41d57 100644 --- a/GVFS/GVFS.Common/Http/HttpRequestor.cs +++ b/GVFS/GVFS.Common/Http/HttpRequestor.cs @@ -260,6 +260,7 @@ protected GitEndPointResponseData SendRequest( int statusInt = (int)response.StatusCode; bool shouldRetry = ShouldRetry(response.StatusCode); + bool shouldRecordFailure = shouldRetry; if (sendAnonymous && ShouldRejectCredentials(response.StatusCode, errorMessage)) @@ -269,13 +270,27 @@ protected GitEndPointResponseData SendRequest( shouldRetry = !forceAnonymous; if (!forceAnonymous) { - this.authentication.MarkAuthenticationRequired(); + shouldRecordFailure = false; + if (this.authentication.PromoteToAuthenticationRequired(out string previousAuthenticationState)) + { + EventMetadata authenticationMetadata = new EventMetadata + { + ["PreviousAuthenticationState"] = previousAuthenticationState, + ["AuthenticationState"] = "Authenticated", + ["HttpStatus"] = response.StatusCode, + }; + + this.Tracer.RelatedInfo( + authenticationMetadata, + "Anonymous request was rejected; authentication is now required"); + } } errorMessage = string.Format( - "Anonymous request was rejected with error code {0} ({1})", + "Anonymous request was rejected with error code {0} ({1}). Original error message from server: {2}", statusInt, - response.StatusCode); + response.StatusCode, + errorMessage); } else if (ShouldRejectCredentials(response.StatusCode, errorMessage)) { @@ -299,7 +314,8 @@ protected GitEndPointResponseData SendRequest( new GitObjectsHttpException(response.StatusCode, errorMessage), shouldRetry, message: response, - onResponseDisposed: () => availableConnections.Release()); + onResponseDisposed: () => availableConnections.Release(), + shouldRecordFailure: shouldRecordFailure); } } catch (TaskCanceledException) diff --git a/GVFS/GVFS.UnitTests/Git/GitAuthenticationTests.cs b/GVFS/GVFS.UnitTests/Git/GitAuthenticationTests.cs index 90b0624b19..4995577719 100644 --- a/GVFS/GVFS.UnitTests/Git/GitAuthenticationTests.cs +++ b/GVFS/GVFS.UnitTests/Git/GitAuthenticationTests.cs @@ -1019,6 +1019,8 @@ public void AnonymousAuthFailurePromotesLaterRequestToAuthenticated( { response.HasErrors.ShouldEqual(true, "The rejected anonymous attempt should report its error"); response.ShouldRetry.ShouldEqual(true, "The rejected anonymous attempt should retry with credentials"); + response.ShouldRecordFailure.ShouldEqual(false, "An expected authentication challenge must not count against the circuit breaker"); + response.Error.Message.ShouldContain(responseBody); } using (GitEndPointResponseData response = requestor.SendRequest( @@ -1034,6 +1036,44 @@ public void AnonymousAuthFailurePromotesLaterRequestToAuthenticated( handler.AuthorizationHeaders[1].StartsWith("Basic ", StringComparison.Ordinal) .ShouldEqual(true, "The retry should carry the credential"); dut.IsAnonymous.ShouldEqual(false, "The auth-required response should promote the state"); + tracer.RelatedInfoEvents.Any(info => info.Contains("PreviousAuthenticationState")) + .ShouldEqual(true, "Runtime authentication promotion should be traced"); + } + + [TestCase] + public void AnonymousAuthFailurePromotesWithoutPriorInitialization() + { + MockTracer tracer = new MockTracer(); + MockGitProcess gitProcess = this.GetGitProcess(); + GitAuthentication dut = new GitAuthentication(gitProcess, "mock://repoUrl"); + MockGVFSEnlistment enlistment = new MockGVFSEnlistment(dut); + TestHttpMessageHandler handler = new TestHttpMessageHandler(); + handler.EnqueueResponse(HttpStatusCode.Unauthorized, "Authentication required"); + handler.EnqueueResponse(HttpStatusCode.OK, "{}"); + + using (TestHttpRequestor requestor = new TestHttpRequestor(tracer, new RetryConfig(), enlistment, handler)) + { + using (GitEndPointResponseData response = requestor.SendRequest( + new Uri("https://mock.invalid/gvfs/objects"), + forceAnonymous: false)) + { + response.HasErrors.ShouldEqual(true, "The anonymous request should report the authentication challenge"); + response.ShouldRetry.ShouldEqual(true, "The challenge should request an authenticated retry"); + response.ShouldRecordFailure.ShouldEqual(false, "The expected challenge must not count against the circuit breaker"); + } + + using (GitEndPointResponseData response = requestor.SendRequest( + new Uri("https://mock.invalid/gvfs/objects"), + forceAnonymous: false)) + { + response.HasErrors.ShouldEqual(false, "The retry should fetch and send a credential"); + } + } + + handler.AuthorizationHeaders.Count.ShouldEqual(2, "Both attempts should reach the server"); + handler.AuthorizationHeaders[0].ShouldEqual(null, "The first request should be anonymous"); + handler.AuthorizationHeaders[1].StartsWith("Basic ", StringComparison.Ordinal) + .ShouldEqual(true, "Promotion must affect the next request without prior initialization"); } private MockGitProcess GetGitProcess() diff --git a/GVFS/GVFS.UnitTests/Http/GitObjectsHttpRequestorTests.cs b/GVFS/GVFS.UnitTests/Http/GitObjectsHttpRequestorTests.cs index ee89a68ae2..1e7236202e 100644 --- a/GVFS/GVFS.UnitTests/Http/GitObjectsHttpRequestorTests.cs +++ b/GVFS/GVFS.UnitTests/Http/GitObjectsHttpRequestorTests.cs @@ -103,6 +103,31 @@ public void PrefetchRequestFallsBackToGlobalCacheServer() requestor.RequestUris[1].AbsoluteUri.ShouldEqual(GlobalCacheServerUrl + "/gvfs/prefetch?lastPackTimestamp=0"); } + [TestCase] + public void RetryableAuthenticationChallengeDoesNotIncrementCircuitBreaker() + { + TestGitObjectsHttpRequestor requestor = this.CreateRequestor(maxRetries: 1); + requestor.EnqueueResponse( + HttpStatusCode.Unauthorized, + shouldRetry: true, + shouldRecordFailure: false); + requestor.EnqueueResponse(HttpStatusCode.OK); + + RetryWrapper.InvocationResult result = + requestor.TrySendProtocolRequest( + requestId: 1, + onSuccess: SuccessfulRequest, + onFailure: null, + method: HttpMethod.Get, + endPointGenerator: () => new Uri(EndpointCacheServerUrl + "/gvfs/objects"), + requestBodyGenerator: () => null, + cancellationToken: CancellationToken.None); + + result.Succeeded.ShouldEqual(true); + requestor.RequestUris.Count.ShouldEqual(2); + RetryCircuitBreaker.ConsecutiveFailures.ShouldEqual(0); + } + [TestCase] public void TransportExceptionFallsBackToGlobalCacheServer() { @@ -405,9 +430,13 @@ private TestGitObjectsHttpRequestor( public MockTracer TestTracer { get; } public List RequestUris { get; } - public void EnqueueResponse(HttpStatusCode statusCode, string body = "", bool shouldRetry = false) + public void EnqueueResponse( + HttpStatusCode statusCode, + string body = "", + bool shouldRetry = false, + bool shouldRecordFailure = true) { - this.responses.Enqueue(Tuple.Create(statusCode, body, shouldRetry)); + this.responses.Enqueue(Tuple.Create(statusCode, body, shouldRetry, shouldRecordFailure)); } public void EnqueueException(Exception exception) @@ -445,7 +474,7 @@ protected override GitEndPointResponseData SendProtocolRequest( onResponseDisposed: null); } - Tuple response = (Tuple)nextResponse; + Tuple response = (Tuple)nextResponse; if (response.Item1 == HttpStatusCode.OK) { @@ -462,7 +491,8 @@ protected override GitEndPointResponseData SendProtocolRequest( new GitObjectsHttpException(response.Item1, "Test failure"), shouldRetry: response.Item3, message: null, - onResponseDisposed: null); + onResponseDisposed: null, + shouldRecordFailure: response.Item4); } } diff --git a/GVFS/GVFS.UnitTests/Virtualization/FileSystemCallbacksTests.cs b/GVFS/GVFS.UnitTests/Virtualization/FileSystemCallbacksTests.cs index 14cbe1b2b8..856b0c6f1b 100644 --- a/GVFS/GVFS.UnitTests/Virtualization/FileSystemCallbacksTests.cs +++ b/GVFS/GVFS.UnitTests/Virtualization/FileSystemCallbacksTests.cs @@ -123,7 +123,7 @@ public void GetMetadataForHeartBeatDoesNotChangeEventLevelWhenNoPlaceholderHaveB metadata.ShouldContain("ModifiedPathsCount", 1); metadata.ShouldContain("FilePlaceholderCount", 0); metadata.ShouldContain(nameof(RepoMetadata.Instance.EnlistmentId), RepoMetadata.Instance.EnlistmentId); - metadata.ShouldContain("IsSendingAnonymousRequests", false); + metadata.ShouldContain("IsSendingAnonymousRequests", true); } mockPlaceholderDb.VerifyAll(); @@ -174,7 +174,7 @@ public void GetMetadataForHeartBeatDoesSetsEventLevelToInformationalWhenPlacehol metadata.ShouldContain("FolderPlaceholderCount", 0); metadata.ShouldContain(nameof(RepoMetadata.Instance.EnlistmentId), RepoMetadata.Instance.EnlistmentId); metadata.ContainsKey("PhysicalDiskInfo").ShouldBeTrue(); - metadata.ShouldContain("IsSendingAnonymousRequests", false); + metadata.ShouldContain("IsSendingAnonymousRequests", true); // Create more placeholders fileSystemCallbacks.OnPlaceholderFileCreated("test.txt", "2222233333444445555566666777778888899999", "GVFS.UnitTests.exe2"); @@ -208,7 +208,7 @@ public void GetMetadataForHeartBeatDoesSetsEventLevelToInformationalWhenPlacehol metadata.ShouldContain("FolderPlaceholderCount", 1); metadata.ShouldContain(nameof(RepoMetadata.Instance.EnlistmentId), RepoMetadata.Instance.EnlistmentId); metadata.ContainsKey("PhysicalDiskInfo").ShouldBeTrue(); - metadata.ShouldContain("IsSendingAnonymousRequests", false); + metadata.ShouldContain("IsSendingAnonymousRequests", true); } mockPlaceholderDb.VerifyAll(); diff --git a/GVFS/GVFS.Virtualization/FileSystemCallbacks.cs b/GVFS/GVFS.Virtualization/FileSystemCallbacks.cs index 2f07ecccd2..81945fa58e 100644 --- a/GVFS/GVFS.Virtualization/FileSystemCallbacks.cs +++ b/GVFS/GVFS.Virtualization/FileSystemCallbacks.cs @@ -314,7 +314,7 @@ public EventMetadata GetAndResetHeartBeatMetadata(out bool logToFile) // header to outgoing requests" (see HttpRequestor.cs), so a mount stuck // reporting true for long stretches against a server that requires auth // is the signature of the anonymous-auth-latch bug this field detects. - metadata.Add("IsSendingAnonymousRequests", this.context.Enlistment.Authentication.IsAnonymous); + metadata.Add("IsSendingAnonymousRequests", this.context.Enlistment.Authentication.IsSendingAnonymousRequests); return metadata; } From 8511f62830e0a0adf17f992334059e225d9cb802 Mon Sep 17 00:00:00 2001 From: Tyrie Vella Date: Tue, 3 Nov 2026 09:18:10 -0800 Subject: [PATCH 3/3] Preserve retry accounting through blob hydration Carry the retry failure-accounting decision through RetryWrapper invocation results and object-download outcomes. Blob hydration can then retry an expected anonymous authentication challenge without incrementing the global circuit breaker in its outer retry loop. Add tests for RetryWrapper propagation and the blob-hydration boundary. Assisted-by: GPT-5.6 Sol Signed-off-by: Tyrie Vella --- GVFS/GVFS.Common/Git/GVFSGitObjects.cs | 29 ++++++++++--- GVFS/GVFS.Common/RetryWrapper.cs | 13 +++++- .../Common/RetryWrapperTests.cs | 17 ++++++++ .../GVFS.UnitTests/Git/GVFSGitObjectsTests.cs | 43 ++++++++++++++++++- 4 files changed, 95 insertions(+), 7 deletions(-) diff --git a/GVFS/GVFS.Common/Git/GVFSGitObjects.cs b/GVFS/GVFS.Common/Git/GVFSGitObjects.cs index 06a1af9908..38065f28f0 100644 --- a/GVFS/GVFS.Common/Git/GVFSGitObjects.cs +++ b/GVFS/GVFS.Common/Git/GVFSGitObjects.cs @@ -67,10 +67,14 @@ public enum BlobHydrationFailureCategory /// internal class DownloadAttemptResult { - public DownloadAttemptResult(DownloadAndSaveObjectResult result, HttpStatusCode? httpStatusCode) + public DownloadAttemptResult( + DownloadAndSaveObjectResult result, + HttpStatusCode? httpStatusCode, + bool shouldRecordFailure = true) { this.Result = result; this.HttpStatusCode = httpStatusCode; + this.ShouldRecordFailure = shouldRecordFailure; } public DownloadAndSaveObjectResult Result { get; } @@ -78,6 +82,8 @@ public DownloadAttemptResult(DownloadAndSaveObjectResult result, HttpStatusCode? // The HTTP status of the last download attempt, or null when no HTTP response was // received (for example an exhausted retry that ended in an exception). public HttpStatusCode? HttpStatusCode { get; } + + public bool ShouldRecordFailure { get; } } protected GVFSContext Context { get; private set; } @@ -217,7 +223,11 @@ public virtual bool TryCopyBlobContentStream( downloadSucceededButCopyFailed = true; } - return new RetryWrapper.CallbackResult(error: null, shouldRetry: true); + return new RetryWrapper.CallbackResult( + error: null, + shouldRetry: true, + result: false, + shouldRecordFailure: lastDownloadResult?.ShouldRecordFailure ?? true); } }); @@ -367,17 +377,26 @@ private DownloadAttemptResult DoDownloadAndSaveObject( { if (output.Succeeded && output.Result.Success) { - return new DownloadAttemptResult(DownloadAndSaveObjectResult.Success, httpStatusCode); + return new DownloadAttemptResult( + DownloadAndSaveObjectResult.Success, + httpStatusCode, + output.ShouldRecordFailure); } if (output.Result.HttpStatusCodeResult == HttpStatusCode.NotFound) { this.objectNegativeCache.AddOrUpdate(objectId, DateTime.Now, (unused1, unused2) => DateTime.Now); - return new DownloadAttemptResult(DownloadAndSaveObjectResult.ObjectNotOnServer, httpStatusCode); + return new DownloadAttemptResult( + DownloadAndSaveObjectResult.ObjectNotOnServer, + httpStatusCode, + output.ShouldRecordFailure); } } - return new DownloadAttemptResult(DownloadAndSaveObjectResult.Error, httpStatusCode); + return new DownloadAttemptResult( + DownloadAndSaveObjectResult.Error, + httpStatusCode, + output.ShouldRecordFailure); } } } \ No newline at end of file diff --git a/GVFS/GVFS.Common/RetryWrapper.cs b/GVFS/GVFS.Common/RetryWrapper.cs index 4d56ccc1fa..ae2e4bd9ba 100644 --- a/GVFS/GVFS.Common/RetryWrapper.cs +++ b/GVFS/GVFS.Common/RetryWrapper.cs @@ -95,7 +95,11 @@ public InvocationResult Invoke(Func toInvoke) if (!this.ShouldRetry(tryCount, null, result)) { - return new InvocationResult(tryCount, result.Error, result.Result); + return new InvocationResult( + tryCount, + result.Error, + result.Result, + result.ShouldRecordFailure); } } else @@ -201,15 +205,22 @@ public InvocationResult(int tryCount, Exception error) } public InvocationResult(int tryCount, Exception error, T result) + : this(tryCount, error, result, shouldRecordFailure: true) + { + } + + public InvocationResult(int tryCount, Exception error, T result, bool shouldRecordFailure) : this(tryCount, error) { this.Result = result; + this.ShouldRecordFailure = shouldRecordFailure; } public T Result { get; } public int Attempts { get; } public bool Succeeded { get; } public Exception Error { get; } + public bool ShouldRecordFailure { get; } = true; } public class CallbackResult diff --git a/GVFS/GVFS.UnitTests/Common/RetryWrapperTests.cs b/GVFS/GVFS.UnitTests/Common/RetryWrapperTests.cs index 528252fa38..d023b7f336 100644 --- a/GVFS/GVFS.UnitTests/Common/RetryWrapperTests.cs +++ b/GVFS/GVFS.UnitTests/Common/RetryWrapperTests.cs @@ -317,6 +317,23 @@ public void CircuitBreakerIgnoresNonRetryableErrors() RetryCircuitBreaker.ConsecutiveFailures.ShouldEqual(0); } + [TestCase] + public void InvocationResultPreservesFailureAccountingDecision() + { + RetryWrapper wrapper = new RetryWrapper(1, CancellationToken.None); + + RetryWrapper.InvocationResult result = wrapper.Invoke( + tryCount => new RetryWrapper.CallbackResult( + new Exception("Expected retry"), + shouldRetry: true, + result: false, + shouldRecordFailure: false)); + + result.Succeeded.ShouldEqual(false); + result.ShouldRecordFailure.ShouldEqual(false); + RetryCircuitBreaker.ConsecutiveFailures.ShouldEqual(0); + } + [TestCase] [Category(CategoryConstants.ExceptionExpected)] public void CircuitBreakerClosesAfterCooldown() diff --git a/GVFS/GVFS.UnitTests/Git/GVFSGitObjectsTests.cs b/GVFS/GVFS.UnitTests/Git/GVFSGitObjectsTests.cs index 60826ca694..0d522bb5c8 100644 --- a/GVFS/GVFS.UnitTests/Git/GVFSGitObjectsTests.cs +++ b/GVFS/GVFS.UnitTests/Git/GVFSGitObjectsTests.cs @@ -83,6 +83,32 @@ public void CatchesFileNotFoundAfterFileDeleted() } } + [TestCase] + public void ExpectedAuthenticationChallengeDoesNotIncrementHydrationCircuitBreaker() + { + MockHttpGitObjects httpObjects = new MockHttpGitObjects + { + StatusCodeToReturn = HttpStatusCode.Unauthorized, + ShouldRecordFailureToReturn = false, + }; + GVFSGitObjects dut = this.CreateTestableGVFSGitObjects( + httpObjects, + new MissingGitRepo(new MockTracer()), + out _); + + dut.TryCopyBlobContentStream( + ValidTestObjectFileSha1, + CancellationToken.None, + GVFSGitObjects.RequestSource.FileStreamCallback, + (stream, length) => Assert.Fail("The challenged download should not provide content"), + out GVFSGitObjects.BlobHydrationFailureCategory _) + .ShouldEqual(false); + + RetryCircuitBreaker.ConsecutiveFailures.ShouldEqual( + 0, + "The outer hydration retry must preserve the expected-challenge classification"); + } + [TestCase] [Category(CategoryConstants.ExceptionExpected)] public void TerminalBlobHydrationFailureIsTaggedWithCategory() @@ -1050,6 +1076,7 @@ private MockHttpGitObjects(MockGVFSEnlistment enlistment) public Stream InputStream { get; set; } public string MediaType { get; set; } public HttpStatusCode? StatusCodeToReturn { get; set; } + public bool ShouldRecordFailureToReturn { get; set; } = true; // When true, TryDownloadObjects returns a failing result built from GitObjectTaskResult(bool), // i.e. Result is non-null but carries no HTTP status (HttpStatusCodeResult == 0). This @@ -1100,7 +1127,8 @@ public override RetryWrapper.InvocationResult TryDownloadOb return new RetryWrapper.InvocationResult( 0, error: null, - result: new GitObjectTaskResult(this.StatusCodeToReturn.Value)); + result: new GitObjectTaskResult(this.StatusCodeToReturn.Value), + shouldRecordFailure: this.ShouldRecordFailureToReturn); } if (this.FailWithoutStatus) @@ -1164,6 +1192,19 @@ public override bool TryCopyBlobContentStream(string blobSha, Action writeAction) + { + return false; + } + } + private class CoalescingTestHttpGitObjects : GitObjectsHttpRequestor { private readonly byte[] objectContents;