[Tyrie Vella (tyrielv)]

Problem and Context

The mount process probes /gvfs/config without credentials to determine whether the repository requires authentication.

A timeout, server error, or socket failure does not answer that question. The prior behavior kept anonymous mode after this failure. Later cache requests omitted the Authorization header and never retrieved credentials. The cache server rejected every request, so GVFS could not hydrate files or retrieve blob sizes until remount.

The opposite fallback is also incomplete. If GVFS assumes authentication is required, credential retrieval can fail before a request reaches a server that accepts anonymous access.

Changes

• Track authentication as unknown, anonymous, or authenticated. An indeterminate config probe keeps the state unknown.
• Send unknown-state runtime requests without credentials. Record anonymous state after success. Promote to authenticated state after a 401, redirect, or the cache server's authentication-required 400 response, then retry with credentials.
• Allow a known-anonymous state to promote if the server later requires authentication.
• Force the initial /gvfs/config probe to remain anonymous, independent of the state that it determines.
• Mark mount authentication initialization before background work starts. Signal completion on every exit path, including exceptions.
• Preserve probe failure details in errors and telemetry, including failures without an HTTP response.
• Restrict the initialization marker to internal callers and document the null-credential success contract.
• Add tests for unknown, anonymous, and authenticated transitions, probe diagnostics, failed credential acquisition, forced-anonymous requests, and initialization waiters.

Known gaps and deliberate decisions

• Credential helper timeout — Runtime credential operations keep their existing timeout behavior in this change. Draft PR Bound credential operations and prevent authentication resource contention #2082 bounds credential fill, approve, reject, and credential-store operations on the same path.
• Authenticated-to-anonymous transition — A successful authenticated response does not prove that authentication was required. GVFS does not issue a separate anonymous probe to demote the state. Azure DevOps is the only GVFS protocol provider and accepts an extra Authorization header on endpoints that permit anonymous access.
• Overlap with Bound credential operations and prevent authentication resource contention #2082 — Both changes edit authentication and HTTP requestor code. Both use the same internal handler-injection constructor, so the second change to merge must rebase its implementation and tests.

[Nathan Baird (ShiningMassXAcc)]

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:

• EnsureLocalCacheIsHealthy reaches it whenever serverGVFSConfig == null and GitObjectsRoot is missing.
• PrefetchVerb (PrefetchVerb.cs:355) can hit that when ResolvedCacheServer is set but ServerGVFSConfig is null, so the TryAuthenticateAndQueryGVFSConfig block is skipped entirely.
• DehydrateVerb.cs:110 (the maintenance-job path) calls InitializeLocalCacheAndObjectsPaths with no TryAuthenticate ahead 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.

[Tyrie Vella (tyrielv)]

Note

This reply was machine-drafted and approved by the PR author before posting.

Thanks, Nathan. I traced the pre-initialization callers and kept their anonymous behavior; mount now marks auth as scheduled before queuing the background initializer. I added tests that exercise real SendRequest headers (before initialization, after authentication, and forced anonymous), a request that waits through anonymous initialization, a successful credentialed config retry, and waiter release after initialization fails. The handler seam now matches #2082's signature. The credential-helper timeout is deferred to #2082 and noted in the description. The full unit suite passes.

[Nathan Baird (ShiningMassXAcc)]

Review pass over this PR. 8 comment(s).

These findings concern code the diff does not touch directly, so GitHub cannot anchor them inline:

GVFS/GVFS.Common/Git/GitAuthentication.cs (around line 242) - HIGH

This change moves an unbounded credential call onto a path that previously could not reach it.

Before: an indeterminate /gvfs/config probe left IsAnonymous true, so a virtualization callback never invoked the credential helper. After: the indeterminate branch (349-368) leaves IsAnonymous false and calls MarkInitialized(), so the next callback reaches TryCallGitCredential(tracer, out errorMessage) — the two-argument overload, which defaults timeoutMs to -1, i.e. Timeout.Infinite.

That call happens while holding gitAuthLock. If the credential helper blocks — GCM waiting on an interactive prompt that has nowhere to appear, a hung broker — the callback never returns, and every thread serializing on gitAuthLock follows it down. An indeterminate probe is exactly the condition under which a flaky network is likely, which is also when the helper is most likely to block.

I would not defer this to #2082: DefaultCredentialTimeoutMs is already declared at line 16 of this file and TryInitializeAndQueryGVFSConfigCore already threads a bounded timeout at line 373. Passing a bounded timeout here looks like a small change relative to the exposure.

GVFS/GVFS.Common/Git/GitAuthentication.cs (around line 368) - MEDIUM

This is a deliberate back-compat break — a genuinely anonymous-capable server whose config probe is indeterminate goes from "works anonymously" to "fails" — and there is no way to opt out of it short of downgrading.

AGENTS.md asks for new behavior gates to be gvfs.-prefixed git config keys declared in GVFSConstants.cs under GitConfig and read via GetConfigBoolOrDefault. Something like gvfs.require-auth-on-indeterminate-config, defaulting to the new (safe) behavior, would give a stuck user a documented escape rather than a rollback. gvfs.background-cache-auth is the pattern.

Separately, the error text ("Unable to query /gvfs/config") describes the probe but not the consequence — worth saying that GVFS is now assuming authentication is required, so the user knows what changed.

GVFS/GVFS/CommandLine/PrefetchVerb.cs (around line 324) - MEDIUM

The new invariant here — sendAnonymous = forceAnonymous || IsAnonymous || !IsInitializationStarted — depends on every requestor path having called MarkInitializationStarted(), but the only product call site is InProcessMount.cs:162.

InitializeServerConnection skips TryAuthenticateAndQueryGVFSConfig entirely when ResolvedCacheServer != null, which is the clone follow-on path (CloneVerb.cs:324), and GVFSVerb.Execute<TVerb> only forwards WorkingDirectoryRoot, so the sub-verb constructs a fresh GitAuthentication with initializationStarted == 0. On that path every object download is permanently anonymous, and a 401 reaches HttpRequestor.cs:262 with shouldRetry = false.

This is not a regression from your change, but it is the one remaining silently-anonymous path against a cache server, and nothing covers it. Worth either closing here or filing it alongside, so the invariant this PR introduces is actually true everywhere.

[Nathan Baird (ShiningMassXAcc)]

This branch now decides whether the user gets authenticated access, but it throws away the only evidence of why. Line 341 passes logErrors: false and discards the message with out _, so there is no StandardErrorHandler output and no final RelatedError. The indeterminateMetadata that does ship carries only Area and HttpStatus — and for the failures that most often land here (TLS, DNS, proxy, socket), httpStatus is null, so telemetry records HttpStatus = "None".

The result is that the fail-closed path is the least diagnosable one, which inverts what you would want when this starts firing in the field. Capturing out string probeError into indeterminateMetadata as ProbeError would fix it — the probe is unauthenticated, so there is no credential material in that string.

[Nathan Baird (ShiningMassXAcc)]

TryGetCredentials can now return true with credentialString == null, meaning "proceed anonymously" rather than "here are your credentials". The only in-repo caller handles it correctly (HttpRequestor.cs:152), but the TryXxx shape reads as a promise that the out parameter is usable on success, and this is a public method.

A short XML doc comment on the declaration stating the dual-success contract would stop the next caller from dereferencing it.

[Nathan Baird (ShiningMassXAcc)]

The MarkInitializationStarted() call is unconditional, which is correct, but it sits directly under a comment block describing the useBackgroundAuth case. A reader tidying this up could reasonably conclude it belongs inside if (useBackgroundAuth) — which would silently restore the fail-open for the foreground path. Moving it above the comment, or annotating it as deliberately unconditional, removes the trap.

[Nathan Baird (ShiningMassXAcc)]

RequiresAuthentication() has no callers, so the outcome it would set up — a 401 on the probe followed by credential acquisition also failing — is not covered. Either wire up that test or drop the method.

[Nathan Baird (ShiningMassXAcc)]

MarkInitializationStarted() is public while IsInitializationStarted is internal, so this is a one-way switch that external callers can flip but cannot observe. Making it internal and adding GVFS.Mount to InternalsVisibleTo.cs would match the visibility of the state it controls.

[Tyrie Vella (tyrielv)]

Note

This reply was machine-drafted and human-approved.

Thanks for the detailed pass. I pushed 18f328c8 with these changes:

• The indeterminate probe now records ProbeError, including TLS, DNS, proxy, and socket failures that have no HTTP status. The user-visible error also carries that detail.
• TryGetCredentials now documents that success with a null credential means that the request can proceed anonymously.
• MarkInitializationStarted() is internal, GVFS.Mount has internals access, and the unconditional mount call now sits above the background-auth comment.
• RequiresAuthentication() now covers a 401 probe followed by credential acquisition failure.

I also changed the fallback design. An indeterminate probe now leaves authentication in an unknown state. Later requests try anonymously first. A 401, redirect, or the cache server's authentication-required 400 promotes the state to authenticated and makes the retry use credentials. A known-anonymous state can also promote later. This removes the compatibility break without a config flag.

I left credential timeout work to #2082 after verifying that it directly bounds this same TryGetCredentials path, plus approve, reject, and credential-store operations.

I also traced the clone follow-on path. CloneVerb authenticates the existing GVFSEnlistment, then calls Execute<PrefetchVerb>(enlistment, ...). That overload calls verb.Execute(enlistment.Authentication), so Prefetch receives the initialized GitAuthentication object rather than constructing a new one.

The full unit suite passes with 1,081 tests and 11 expected native-hook ignores. Release builds of GVFS and GVFS.Mount also pass.

[Nathan Baird (ShiningMassXAcc)]

Re-review of the force-pushed 18f328c8. The tri-state AuthenticationState is a better design than the fail-closed version it replaces, and all five of my inline asks are genuinely addressed. The back-compat concern I raised is now moot — Unknown stays anonymous-first, so there's no break to gate.

Six observations below. The first one I believe makes the PR's central mechanism inert, so it's worth resolving before this lands. CI is green across every leg.

[Nathan Baird (ShiningMassXAcc)]

High — I think this short-circuit makes MarkAuthenticationRequired() inert on the path the PR is written for.

ShouldSendAnonymous returns true on !IsInitializationStarted before consulting GetAuthenticationState(). So on any requestor whose authentication was never initialized, promoting to Authenticated has no observable effect — the next request is still sent anonymously.

Combined with HttpRequestor now setting shouldRetry = true on that branch, the retry loop resends a byte-identical anonymous request until MaxAttempts is exhausted. The previous code failed fast. That turns one clear failure into seven identical ones.

This is reachable in production: DehydrateVerb.cs:110 (--maintenance-job) → EnsureLocalCacheIsHealthy → GVFSVerb.QueryGVFSConfig:343 runs with authentication never initialized. No existing test covers promote-without-initialization, which is why it's green.

Could MarkAuthenticationRequired() also mark initialization started and complete, so the promotion actually takes effect? A test driving promotion with no prior initialization would lock it in.

[Nathan Baird (ShiningMassXAcc)]

Medium — These three assertions are being flipped true → false to accommodate the new tri-state, but I think the field underneath them is now wrong rather than the assertions.

The heartbeat still emits Authentication.IsAnonymous under a comment stating it means "no Authorization header is being sent". With the tri-state, Unknown yields false while requests genuinely are anonymous. So the one piece of telemetry that would detect the High finding above is blind to it, by construction.

Would it be better to make the heartbeat field reflect ShouldSendAnonymous (what is actually on the wire) and leave these assertions at true?

[Nathan Baird (ShiningMassXAcc)]

Medium — The runtime promotion — the behaviour this PR exists to add — emits no trace. Support sees the indeterminate-probe warning begin and then has no signal that it ever resolved, which makes the feature effectively invisible in a log-only investigation.

A single RelatedInfo on the transition (with the old and new state) would make this diagnosable.

[Nathan Baird (ShiningMassXAcc)]

Medium — This two-argument TryCallGitCredential path resolves to timeoutMs = -1 while holding gitAuthLock, and credentialGate.Wait(60_000) falls through on timeout rather than aborting — so the overall wait is unbounded.

That was survivable before, but this PR is the first change to route a virtualization callback thread into it, on an indeterminate-probe mount. A hung credential helper now blocks a callback rather than a verb.

I know this is deferred to #2082, but that PR is still a draft. DefaultCredentialTimeoutMs already exists in this file — passing it here would be a small, self-contained guard in the meantime.

[Nathan Baird (ShiningMassXAcc)]

Low — MarkAnonymousAccessSucceeded() fires on any non-forced anonymous 200, including before initialization. One permissive endpoint early in a session therefore latches the state to Anonymous process-wide, for endpoints that were never probed. Scoping the latch to the probe, or requiring initialization, would be tighter.

[Nathan Baird (ShiningMassXAcc)]

Low — This message drops the response body, unlike both sibling branches that include it. For the cache-server auth-required 400 in particular, the body is the only detail that identifies what actually happened.