Mirror reaper can delete a mirror mid-fetch, failing in-flight and follower requests

[matt-edmondson]

What's wrong

MirrorMaintenanceService.Sweep() (GitBranchStateCache/Mirrors/MirrorMaintenanceService.cs) decides whether a mirror is idle purely from on-disk markers read via IMirrorStore.LastUsedAt/RefsFetchedAt (falling back to directory creation time), and once a mirror looks idle beyond MirrorIdleMaxAge it calls mirrors.Delete(directory) — a recursive filesystem delete (MirrorStore.Delete → Directory.Delete(directory, recursive: true)).

Those markers are only rewritten after a git operation finishes: MirrorFetcher.FetchAsync/CloneAsync call mirrors.MarkFetched(directory) only once git fetch/git clone has succeeded, and BranchStateHandler calls mirrors.MarkUsed(repository.Directory) only after fetcher.EnsureCurrentAsync(...) has already returned. MirrorMaintenanceService has no reference to ISingleFlight and there is no lock, refcount, or other coordination that stops it from reaping a directory that another part of the process is actively reading from or writing to.

Concrete failure scenario

1. A mirror hasn't been queried in just under MirrorIdleMaxAge (default 30 days), so its .last-used / .refs-fetched-at markers are correspondingly old.
2. A client request arrives. MirrorFetcher.EnsureCurrentAsync finds the refs older than RefsTtl (default 30s), becomes the SingleFlight leader for that repository, and starts FetchAsync (git fetch --prune --quiet origin) against the existing mirror.git directory. On a large repository this can legitimately run for close to FetchTimeout (default 2 minutes).
3. MirrorMaintenanceService.ExecuteAsync ticks (default hourly, MaintenanceInterval) while that fetch is still running. Sweep() reads LastTouched(directory) for this same directory — it still sees the stale, pre-fetch markers, because MarkFetched/MarkUsed are only written once the fetch completes — computes it as idle beyond MirrorIdleMaxAge, and calls Reap(directory), i.e. Directory.Delete(directory, recursive: true), on the very directory the concurrent git fetch process has as its working directory.
4. The in-flight fetch fails (or, depending on filesystem/OS timing, partially succeeds against a half-deleted tree). Any follower requests waiting on the same SingleFlight ticket (MirrorFetcher.FollowAsync) then observe !mirrors.Exists(directory) and are told "No mirror exists for this repository and the request that was creating one did not finish" — even though a fully working mirror existed moments before the sweep ran.

The same class of race, more narrowly, can also hit a request that skipped the fetch path entirely (refs already fresh, EnsureCurrentAsync returns Current immediately with no SingleFlight involvement) but has not yet reached its own mirrors.MarkUsed call: if the sweep reaps the directory in that gap, the subsequent refs.ListAsync/diff git subprocesses run against a directory that disappears underneath them.

Why it matters

This is exactly the class of bug this cache is most exposed to: it hits precisely the mirrors the reaper exists to protect — ones sitting right at the idle-age boundary that are being queried again — and turns a routine maintenance sweep into an intermittent 502/503 (or a mirror that has to be re-cloned from scratch) for a real, legitimate request.

Suggested fix / acceptance criteria

• Give MirrorMaintenanceService a way to know a directory is currently being fetched/read before reaping it, e.g. have it consult the same ISingleFlight instance MirrorFetcher uses and skip any directory whose flight key currently has an in-flight entry.
• And/or have MirrorFetcher write a "touched" marker (or call MarkUsed) at the start of a fetch/clone, not only on completion, so Sweep's LastTouched sees an in-progress operation as recent rather than stale.
• Add a test that starts a slow/blocked fetch against a mirror whose markers are already older than MirrorIdleMaxAge, runs MirrorMaintenanceService.Sweep() concurrently, and asserts the mirror survives and the fetch completes successfully.

[matt-edmondson]

Triage

Category: Bug (concurrency / data race)
Priority: High
Area: GitBranchStateCache/Mirrors/MirrorMaintenanceService.cs + MirrorFetcher

A maintenance sweep can recursively delete a mirror directory while a git fetch is running against it, turning routine maintenance into intermittent 5xx responses and a from-scratch re-clone. The race targets exactly the mirrors the reaper is meant to spare — ones at the idle-age boundary that are being queried again — because the freshness markers are only written after the git operation completes. Not Critical because it needs the idle-age boundary to coincide with the hourly sweep, but it is a genuine race with no coordination at all in the current code, and the failure is user-visible.

Related: #24 in this repo is the other half of mirror-identity correctness (casing). Independent fixes, but both live in the mirror resolve/lifecycle path and could reasonably be reviewed together.

Not covered by an open PR.

Suggested next step: the cheaper of the two proposed fixes is having MirrorFetcher mark the directory as touched at the start of a fetch/clone rather than only on success — it needs no new dependency on ISingleFlight and closes the wide window. Consulting ISingleFlight from MirrorMaintenanceService additionally closes the narrow post-EnsureCurrentAsync, pre-MarkUsed gap.

---

Generated by Claude Code