What's wrong
- Publish doesn't check for invalidation.
LockListService.RefreshAsync publishes whatever the walk returned, unconditionally: snapshots.Publish(key, result.Snapshot!) at GitLfsCache/Locks/LockListService.cs:155. The fan-out's resolution refresh does the same (Locks/LockFanOut.cs:273).
- Invalidation leaves no trace.
LockSnapshotStore.Invalidate (Locks/LockSnapshotStore.cs:36-48) removes only the snapshots that exist when it runs. Publish (:26-32) overwrites without looking, so a walk that was already running when the invalidation happened still gets published.
- The stale result looks fresh. The snapshot is timestamped when the walk finishes (
Locks/LockListRefresher.cs:127, new LockSnapshot(collected, timeProvider.GetUtcNow())), so it is treated as fresh for the whole ListTtl.
Failure scenario
The Unreal plugin lists locks every 30 s from every editor, so on a busy repository a multi-page walk is often in progress.
- Client A's
GET locks becomes the refresh leader and starts walking pages. With thousands of locks this takes seconds.
- Client B locks a file. The
POST locks is relayed, upstream answers 201, and LockRouteHandler.InvalidateIfChanged (Endpoints/LockRouteHandler.cs:230-235) calls Invalidate. There's nothing to remove, because the previous snapshot has already expired or been dropped.
- A's walk finishes. Its pages were read before B's lock existed, but it publishes them stamped with the current time.
- For the next
ListTtl (15 s by default), every admitted client, B included, is served a listing without B's lock. An unlock fails the same way in reverse: the file still shows as locked.
A locks/batch fan-out that invalidates while a listing walk is running hits the same gap (LockFanOut.cs:89).
This breaks the guarantee the invalidation exists for ("the change this client just made is visible to the next listing", LockRouteHandler.cs:164-166). #46/#60 made invalidation cover every ref, but didn't close this race. No test exercises an invalidation during a refresh.
Suggested fix
- Keep a per-repository generation counter in
LockSnapshotStore, and have Invalidate increment it.
- Capture the generation before the walk starts, and publish only if it hasn't changed. For example,
Publish(key, snapshot, expectedGeneration) returns false otherwise, and the caller serves the result to its own requester without caching it.
- Optionally, also stamp the snapshot with the time the walk started.
Acceptance criteria
- A test with a refresher that blocks mid-walk: call
Invalidate for that repository while it's blocked, then release the refresher. The next ResolveAsync must refresh again rather than serve the pre-invalidation walk.
- The same guard applies to
LockFanOut.RefreshForResolutionAsync.
What's wrong
LockListService.RefreshAsyncpublishes whatever the walk returned, unconditionally:snapshots.Publish(key, result.Snapshot!)atGitLfsCache/Locks/LockListService.cs:155. The fan-out's resolution refresh does the same (Locks/LockFanOut.cs:273).LockSnapshotStore.Invalidate(Locks/LockSnapshotStore.cs:36-48) removes only the snapshots that exist when it runs.Publish(:26-32) overwrites without looking, so a walk that was already running when the invalidation happened still gets published.Locks/LockListRefresher.cs:127,new LockSnapshot(collected, timeProvider.GetUtcNow())), so it is treated as fresh for the wholeListTtl.Failure scenario
The Unreal plugin lists locks every 30 s from every editor, so on a busy repository a multi-page walk is often in progress.
GET locksbecomes the refresh leader and starts walking pages. With thousands of locks this takes seconds.POST locksis relayed, upstream answers 201, andLockRouteHandler.InvalidateIfChanged(Endpoints/LockRouteHandler.cs:230-235) callsInvalidate. There's nothing to remove, because the previous snapshot has already expired or been dropped.ListTtl(15 s by default), every admitted client, B included, is served a listing without B's lock. An unlock fails the same way in reverse: the file still shows as locked.A
locks/batchfan-out that invalidates while a listing walk is running hits the same gap (LockFanOut.cs:89).This breaks the guarantee the invalidation exists for ("the change this client just made is visible to the next listing",
LockRouteHandler.cs:164-166). #46/#60 made invalidation cover every ref, but didn't close this race. No test exercises an invalidation during a refresh.Suggested fix
LockSnapshotStore, and haveInvalidateincrement it.Publish(key, snapshot, expectedGeneration)returnsfalseotherwise, and the caller serves the result to its own requester without caching it.Acceptance criteria
Invalidatefor that repository while it's blocked, then release the refresher. The nextResolveAsyncmust refresh again rather than serve the pre-invalidation walk.LockFanOut.RefreshForResolutionAsync.