Do not fall back to anonymous when the /gvfs/config probe is indeterminate - #2104
Tyrie Vella (tyrielv) wants to merge 2 commits into
Conversation
…inate Mount initializes authentication by querying /gvfs/config without credentials. A success means the server allows anonymous access. A 401 means credentials are required. Any other outcome - a timeout, a 5xx, or a socket error - says nothing about authentication, but the code left IsAnonymous at its default of true and marked initialization complete. That state is unrecoverable for the life of the mount process. HttpRequestor omits the Authorization header while IsAnonymous is true, and the short-circuit in SendRequest means TryGetCredentials is never called, so no credential is ever fetched. The Azure DevOps cache server answers such a request with 400 and the body "A valid Basic Authorization header is required." A 400 is not retryable, RejectCredentials is a no-op because no credential was ever cached, and initialization is already latched, so the probe never runs again. Mount proceeds past this failure when a cache server is configured, so the repo stays mounted but cannot download objects or blob sizes. Directory enumeration then fails with SizesUnavailableException and git reports tracked files as deleted. Every enumeration retries and fails the same way, which floods the mount log. Only a remount clears the state. Default IsAnonymous to false and set it explicitly on the indeterminate branch. Anonymous access is now an affirmative determination that requires a successful unauthenticated probe. If the server does allow anonymous access, it ignores the Authorization header that GVFS sends after an indeterminate probe. Extract IGVFSConfigRequestor from ConfigHttpRequestor and add an internal factory seam so tests can drive each probe outcome without a server. Five tests cover the three branches; the three that pin the fix fail if either half of it is reverted. Assisted-by: Claude Opus 5 Signed-off-by: Tyrie Vella <tyrielv@gmail.com>
Review found that the previous commit broke the probe it depends on. The probe that determines whether the server allows anonymous access was unauthenticated only as a side effect of IsAnonymous defaulting to true. Defaulting it to false removed that mechanism without replacing it, so HttpRequestor.SendRequest saw IsAnonymous == false, called TryGetCredentials, and waited on initializationComplete - the event that only the same call stack can set. Each wait cost the full 120 second initialization timeout and returned a retryable 401, so a mount stalled for roughly 14 minutes across the default seven attempts and then wrongly concluded that credentials were required. This happened on every mount. The mock-based tests never reached SendRequest, so they could not catch it. Make the probe explicitly unauthenticated instead of implicitly so. SendRequest takes a forceAnonymous parameter, ConfigHttpRequestor.TryQueryGVFSConfig passes it through, and the initial probe sets it. A request that carries no credentials now also skips ApproveCredentials and keeps the existing anonymous handling of a 401, which is the definitive answer that the server requires authentication. SendRequest also resolves the decision once into a local instead of reading IsAnonymous four times. Another thread can change the property mid-request, and the credential gate, the Authorization header, and the response handling must agree on one value. Trace the indeterminate branch with Keywords.Telemetry and structured metadata. The plain RelatedWarning overload traces with Keywords.None, which the telemetry listener filters out, so the affected population could not be measured. Replace the ConfigRequestorFactory Func with a plain ConfigRequestorOverride property, matching the settable-knob style this class already uses for InitializationWaitTimeoutMs, and make IGVFSConfigRequestor internal since no production code needs the polymorphism. Add InitialConfigProbeIsSentWithoutCredentials, which mirrors the SendRequest credential gate and fails fast if the probe ever waits on its own initialization. Convert the indeterminate-status test to real TestCase rows so each status reports independently. Mutation testing, per half: removing forceAnonymous fails the new test; reverting the default fails AuthIsNotAnonymousBeforeInitialization; deleting the explicit IsAnonymous assignment on the indeterminate branch fails nothing, so that line is kept only as defensive symmetry and is commented as such. Full unit suite: 948 tests, 0 failed. Assisted-by: Claude Opus 5 Signed-off-by: Tyrie Vella <tyrielv@gmail.com>
61dd891 to
7ef10af
Compare
Nathan Baird (ShiningMassXAcc)
left a comment
Nathan Baird (ShiningMassXAcc)
left a comment
There was a problem hiding this comment.
Reviewed the full diff plus the surrounding auth/HTTP paths. The core change is sound: IsAnonymous now defaults false, all four HttpRequestor consumers snapshot into a single sendAnonymous, and the initial probe forces anonymous so it cannot wait on the initialization it is performing. Good unit coverage of the three probe outcomes, and CI is green across the full functional + upgrade matrix on x64 and arm64.
Seven findings, ranked.
1. High — GVFS/GVFS.Common/Git/GitAuthentication.cs:48 (IsAnonymous default)
Flipping the default from true to false changes behavior for any requestor that runs before TryInitializeAndQueryGVFSConfig.
GVFSVerb.QueryGVFSConfig (GVFS/GVFS/CommandLine/GVFSVerb.cs:343) builds a ConfigHttpRequestor and calls TryQueryGVFSConfig without forceAnonymous, and it is not guaranteed that auth was initialized first:
EnsureLocalCacheIsHealthyreaches it wheneverserverGVFSConfig == nullandGitObjectsRootis missing.PrefetchVerb(PrefetchVerb.cs:355) can hit that whenResolvedCacheServeris set butServerGVFSConfigis null, so theTryAuthenticateAndQueryGVFSConfigblock is skipped entirely.DehydrateVerb.cs:110(the maintenance-job path) callsInitializeLocalCacheAndObjectsPathswith noTryAuthenticateahead of it.
Previously that request went out anonymously. Now SendRequest takes the credential path, and TryGetCredentials blocks on initializationComplete for InitializationWaitTimeoutMs (= BackgroundCredentialTimeoutMs, 120_000) before failing with "Timed out waiting for authentication to initialize".
Could you confirm those paths always initialize first? If not, either pass forceAnonymous: true there, or make TryGetCredentials fail fast when initialization was never started (as opposed to in flight). A silent two-minute stall is a worse failure mode than the one being fixed here.
2. Medium — GVFS.UnitTests/Git/GitAuthenticationTests.cs, InitialConfigProbeIsSentWithoutCredentials
This test re-implements the thing it is testing:
// Mirror HttpRequestor.SendRequest's credential gate exactly.
bool sendAnonymous = forceAnonymous || dut.IsAnonymous;That is a hand copy of the gate in HttpRequestor.SendRequest, so the test keeps passing if that gate ever changes. The HttpRequestor.cs edits are the riskiest part of this PR and currently have no direct coverage. Could we get one test that drives the real SendRequest and asserts the Authorization header is absent under forceAnonymous and present otherwise?
3. Medium — GitAuthentication.cs, indeterminate branch (~307-326)
Roughly twenty lines of comment for three lines of code, and most of it narrates the investigation rather than the code. The Azure DevOps cache-server 400 chain, "a 400 is not retryable", "initialization is already latched - so re-initialization throws" is already in the PR description, which is where it belongs.
Suggest trimming to the two or three lines a future reader actually needs — the probe outcome is unknown, so assume auth is required; a server that allows anonymous access will ignore the header — and leaving the incident analysis in the PR and linked work item.
4. Medium — duplicated rationale across four sites
The "forceAnonymous is required, not incidental / this probe DETERMINES whether the server allows anonymous access / self-deadlock" explanation appears nearly verbatim in GitAuthentication.cs, the HttpRequestor.SendRequest param doc, IGVFSConfigRequestor.cs, and the test's XML doc. Keep one canonical version (the interface doc reads like the right home) and <see cref=""/> it from the other three. Four copies will drift.
5. Low — GitAuthentication.cs, this.IsAnonymous = false; in the indeterminate branch
The comment concedes this is a no-op: "the field already defaults to false and no earlier path in this method can set it true without returning". Either drop the assignment or keep it and drop the four-line justification — as written the justification costs more than the statement it defends.
6. Low — no assertion on the new telemetry keyword
The PR specifically calls out that the plain RelatedWarning(string, params object[]) overload traces with Keywords.None and gets filtered by TelemetryDaemonEventListener. Worth a MockTracer assertion that the indeterminate warning carries Keywords.Telemetry, otherwise the next refactor silently loses the signal this PR is adding.
7. Low — GitAuthentication.cs:48 <remarks>
"...so a default of true makes every request unauthenticated when the probe does not run or does not complete" describes the old default. Reword to describe current behavior: while this is true, SendRequest omits the Authorization header and never calls TryGetCredentials.
No stray .github AI artifacts in the diff, and no coverage gate exists in this repo (GitHub Actions only), so item 2 is the closest thing to a differential-coverage gap.
2 participants
Problem and Context
The mount process probes
/gvfs/configwithout credentials to determine therepository authentication mode. A successful response enables anonymous access,
and a 401 enables credential retrieval.
If that probe fails for another reason, such as a timeout, a 5xx response, or a
socket failure, the process can mount with a cache server but leave
IsAnonymousset totrue. Later requests omit theAuthorizationheader andnever retrieve a credential. The cache server rejects those requests, so GVFS
cannot hydrate files or retrieve blob sizes. Directory enumeration then fails
and Git can show projected files as deleted until the user remounts.
The initial probe must still omit credentials after this change. It now does so
explicitly instead of relying on the default authentication state.
Changes
requests after an indeterminate probe so GVFS can retrieve credentials.
forceAnonymouspath for the initial/gvfs/configprobe.This prevents the probe from waiting for the initialization that it performs.
credential gate, authorization header, and response handling consistent.
status when one exists.
authenticated, and indeterminate probe results, including the forced
anonymous initial probe.