chadtmiller.com/knotless

Switch repository

Loading…

What changes

A mirror's sync now sets its refs, commit-graph and HEAD in a task of its own, which holds the mirror's lock until git is done. A caller that stops waiting no longer releases the lock while git update-ref is still running.

Why

The e2e run of a merge queue batch failed on "merging a pull" with a 502 page: git update-ref: fatal: cannot lock ref 'refs/heads/bytes': Unable to create '…/mirrors/…/3mzdx5ek7vl5a.git/refs/heads/bytes.lock': File exists. Only Mirror::sync writes a mirror's refs, and it does so under its lock file. But the lock lives in the sync's future. When a request is dropped mid-sync (the browser navigates on, as Playwright does between steps, or a reader on prod leaves), the future is dropped and the lock with it. The git process it started is not killed and keeps running. The next sync then takes the lock, and its update-ref finds git's own .lock file and fails. Killing git when the future is dropped would leave that .lock behind for good, so the work is moved somewhere a drop can't reach. The pull that failed (What each notification says is found for its page alone) changes only the notifications query.

How it was checked

Every sync now goes through the moved code, which the library and serve tests cover. CI runs them, along with e2e. I didn't add a test that drops a sync in the middle, because it would only test tokio's promise that a spawned task outlives a dropped handle.

Shortcomings

The fetch before the lock can still be dropped in the middle. It only indexes packs under names of their own, and a later sync fetches what is missing.

Changeset: none

🤖 Generated with Claude Code

This moves the ref update, commit-graph write and HEAD update in Mirror into a spawned settle task that holds the lock file until git is done. That fixes the cause you describe: a dropped request no longer releases the lock while update-ref is still running. I found nothing that must change.

• Extra change. The pull also moves the repack out of the ref lock. sync still tidies inline, but knotless-serve now calls refresh and then aside, which runs tidy in a separate task under a non-blocking .tidy lock. The title doesn't mention this, but the body of storage.md and the new tests cover it. It looks safe: repack -a -d -k only removes packs that existed when it started, and fetches write packs under their own names.
• Checks. At review time check, docs, e2e and test were still queued or running. Confirm they pass before merging, especially e2e, which showed the original failure.
• Not checked. I couldn't confirm that settled.await? compiles against the crate's error type, because that needs From<JoinError>. The test check will show it.
• Doc nit. In docs/design/storage.md, the line "git's own lock files on the refs still there. A new mirror is created under a temporary name and" is longer than the rest of the paragraph. Re-wrap it if the docs check cares.

Since my last review the branch has only gained a "Merge main" commit. The settle task that holds the lock until git is done is as I reviewed it, and I have nothing new to ask for. My earlier review asked for no change, only that the checks pass and that the long line in docs/design/storage.md be re-wrapped. I did not re-check the line, so re-wrap it if the docs check complains.

At review time the check, test, e2e and docs runs on this commit were still queued or running, and the pull is already in the merge queue. Merge only if they pass. e2e matters most, since it showed the original failure.