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

  1. matt-edmondson commented on Sep 28, 2026

    @matt-edmondson
    ContributorAuthor

    Triage

    • Category: Bug
    • Priority: Medium. A routine lock-then-unlock-by-path of many files triggers up to MaxFanOutConcurrency concurrent full listing walks, plus one walk per miss. All of them bypass single-flight and the upstream limiter, so they can get the upstream token rate-limited for everyone using the proxy.
    • Area / suggested assignment: Locks, in GitLfsCache/Locks/LockFanOut.cs (lines 134-146 and 261-275)
    • Duplicates: none found among open issues in the org
    • In progress: no direct match. Open PR Invalidate every ref's lock snapshot when a relayed lock changes #60 changes snapshot invalidation after lock changes, which is one of the triggers here, so check the interaction.

    Notes: Resolve ids once per request, with at most one refresh routed through key.ToFlightKey() and IUpstreamLimiter. That also bounds the cost for #65's over-ceiling repositories, so fixing both together is sensible.


    Generated by Claude Code

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