Skip to content

Unlocking many paths through locks/batch can run a full lock-list walk per path, outside single-flight and the upstream limiter #63

Description

@matt-edmondson

What's wrong

When a locks/batch unlock target has a path but no id, LockFanOut resolves the id per item (GitLfsCache/Locks/LockFanOut.cs:134-146). On a miss it calls RefreshForResolutionAsync (LockFanOut.cs:261-275). That method calls refresher.RefreshAsync directly, which walks every page of the upstream lock listing. The call goes through neither the LockListService single-flight nor the IUpstreamLimiter.

Two things make misses common:

  • Any successful fan-out or relayed lock change invalidates the snapshot (LockFanOut.cs:87-90).
  • LockSnapshotStore.Invalidate removes the snapshot entirely (LockSnapshotStore.cs:36-40).

Failure scenarios

  1. A client locks 500 files via locks/batch, which invalidates the snapshot, then unlocks the same 500 by path. Every item in the first wave, up to MaxFanOutConcurrency (default 8), finds no snapshot, and each one concurrently walks the full listing.
  2. An unlock-by-path request for 1000 paths, 900 of which hold no lock. Each of the 900 misses triggers another full cursor walk, because resolution still fails after the refresh. These walks bypass the throttle a GitHub 403/429 with Retry-After is meant to impose, so they can get the token rate-limited.

Suggested fix

  • Resolve ids once per request, before fanning out.
  • Collect every target that has only a path. If any fails to resolve against the current snapshot, do one refresh. Route it through the same single-flight key the list service uses (key.ToFlightKey()) and through the upstream limiter.
  • Resolve all paths from that one snapshot. Paths still unresolved fail with 404 without any further refresh.

Acceptance criteria

  • With no snapshot, a 50-path unlock-by-path batch against a stub upstream that counts GET locks makes exactly one listing walk.
  • A batch in which every path is unlocked makes at most one walk.
  • The refresh respects IUpstreamLimiter.

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

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions