One network error on a single locks/batch item turns the whole fan-out into a 500, discarding locks upstream already granted

[matt-edmondson]

What's wrong

LockFanOut.SendWithRetriesAsync (GitLfsCache/Locks/LockFanOut.cs, around line 186) calls upstreamClient.SendAsync(...) and response.Content.ReadAsStringAsync(...) with no handling for transport failures. The upstream HttpClient is registered with Timeout = InfiniteTimeSpan and UpstreamClient is a thin pass-through, so a connection reset, refused connection, DNS failure, or a body cut off mid-read surfaces as an HttpRequestException or IOException straight out of the per-item delegate.

That delegate runs inside Parallel.ForAsync (line 66). When one iteration throws, Parallel.ForAsync cancels the token it passed to every other iteration, stops scheduling new ones, and rethrows. ExecuteAsync never reaches the result assembly or the snapshots.Invalidate(...) call (line 89), and nothing in LockRouteHandler.FanOutAsync or GitLfsCacheHandler catches it, so the client gets an unhandled-exception 500 with no body.

This contradicts the locks spec, which makes partial success the contract for this endpoint (locks-subsystem-design.md L113):``

Response is always 200 when the request itself was well formed and the caller was admitted, because partial success is the normal outcome and a transport-level failure would discard the successful half

Failure scenario

1. An editor sends POST .../locks/batch for 500 paths. MaxFanOutConcurrency is 8.
2. 200 items succeed upstream, so those locks are now held by the caller.
3. Item 201 gets a TCP reset from the forge, which is routine under load. SendAsync throws HttpRequestException.
4. Parallel.ForAsync cancels the 7 items still in flight. Some of those POSTs may already have been applied upstream even though their responses are discarded. It then rethrows.
5. The client receives a 500. It cannot tell which of the 200+ locks it now holds, which is exactly the reconciliation the result array exists for.
6. The snapshot is not invalidated. Other editors keep seeing those 200+ files as unlocked for up to ListTtl, even though the fan-out that locked them has already finished.

The same applies to an exception from RefreshForResolutionAsync (unlock by path), because LockListRefresher also does not catch transport exceptions.

Suggested fix

• In SendWithRetriesAsync, catch HttpRequestException and IOException, and OperationCanceledException when the request's own token was not cancelled. Turn each one into a per-item Failure(...), for example status 502 with a message saying upstream could not be reached, rather than letting it escape the delegate. Decide separately whether a transport failure should count against MaxFanOutRetries.
• Do the same in the unlock-by-path resolution branch, so a failed refresh gives a per-item failure instead of a thrown exception.
• Make sure invalidation still runs whenever any item succeeded, even if the request is then cancelled. A try/finally around the Parallel.ForAsync call would do it.

Acceptance criteria

• A test with a stub upstream that throws HttpRequestException for one path out of several returns 200 with ok: true results for the others and ok: false for the failed one.
• After such a request, the snapshot for the repository is invalidated whenever at least one item succeeded.
• A client disconnect (the request's own token cancelled) still stops the fan-out, but it does not leave the snapshot un-invalidated after items have succeeded.

[matt-edmondson]

Triage

• Category: Bug (breaks the locks spec's contract that a well-formed request always gets a 200 with per-item results)
• Priority: High. When one item hits a transport error, the client gets a bare 500 while locks upstream already granted stay held, and the client can't tell which ones it now owns. The snapshot is also left un-invalidated, so other editors see those files as unlocked for up to ListTtl. For binary LFS assets, where locks exist to prevent merges nobody can resolve, that is the failure the subsystem is meant to prevent. Connection resets under load are routine, so this will happen in practice.
• Area / suggested assignment: Locks subsystem: GitLfsCache/Locks/LockFanOut.cs (SendWithRetriesAsync, the Parallel.ForAsync loop), LockListRefresher, and LockRouteHandler.FanOutAsync.
• Duplicates: none found.
• In progress: no. The only open PRs in GitLfsCache are dependabot bumps (Bump Testably.Abstractions.FileSystem.Interface from 10.3.0 to 10.4.0 #53, Bump Testably.Abstractions.Testing from 7.0.2 to 7.1.0 #54).

Suggested next step: make the fix in two parts. First, turn transport exceptions into per-item Failure(502, …) inside the delegate, so Parallel.ForAsync never sees a throw. Second, wrap the loop in try/finally so invalidation runs whenever any item succeeded. The first part alone restores the 200 contract. The second covers client disconnects too. Settle whether transport failures count against MaxFanOutRetries before writing the test, because it changes what the stub upstream has to simulate.

---

Generated by Claude Code