Skip to content

A lock created or released during an in-flight lock-list refresh is missing from listings for a full ListTtl: the refresh publishes its stale walk after the invalidation ran #70

Description

@matt-edmondson

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.

  1. Client A's GET locks becomes the refresh leader and starts walking pages. With thousands of locks this takes seconds.
  2. 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.
  3. A's walk finishes. Its pages were read before B's lock existed, but it publishes them stamped with the current time.
  4. 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.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't working

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions