githttp: remove a stale ref lock automatically when no git can own it

#485 lands only detection for stale ref locks (#464): hydration logs each lock older than 1 minute that blocks a failed ref update, and deploy-production.md gives a manual runbook (stop the replica, check for Git processes, remove the lock). A replica hit by #464 stays unable to serve that repository until an operator acts.

This issue tracks an automatic, safe removal. Three Codex review rounds on #485 showed that a /proc scan for "no process works in the repository" is hard to make complete:

Hydration's own read-only for-each-ref children run in the repository while a batch flushes: the deletion pass and the update pass (syncRefsPassUpdates) both have one. The check must exclude each of them, or it refuses cleanup on large syncs (fails safe, but the node stays wedged).
A git process can reach the cache through a linked worktree or GIT_COMMON_DIR, so its cwd, argv and GIT_DIR do not name the cache repository.
Processes the scan cannot read (another user's process, hidepid, another PID namespace that shares the cache volume) must count as possible owners.

Possible approaches: hold the repository's hydrate lock in every git that can take a cache-repo ref lock, so hydration can prove no owner exists without scanning /proc; or check open file descriptors for the lock file. The closed approach and its tests are in the history of #485 (commits f32e9ac to 76d8cd1).

Trigger: a report of a replica stuck on ref lock … blocks ref updates, or a decision to make cache repos self-healing.

Refs #464

Generated with xum • Model: anthropic:claude-opus-5-5 • Thinking: high

#485 ships the final-stop version (merge pending): hydration logs one line per failed batch, hydrate <repo>: <n> ref lock(s) older than 1m0s block ref updates, and the runbook rebuilds the repository's cache (stop the replica, delete <cache dir>/<repo ID>.git, start it). Two more review findings belong here for any automatic removal: report per-lock detail only in bounded form (a killed batch can leave 10,000 locks), and never put a ref-derived path into an operator command (ref names can contain $(...)).

Generated with xum • Model: anthropic:claude-opus-5-5 • Thinking: high

Follow-ups from the clean-context merge advisory on #485 (none blocked the merge). Do them with this issue or as a docs fix:

Runbook step 1 says to take the path from "the text after hydrate". Say "the text between hydrate and the colon".
Runbook step 2 refers to "steps 2 and 3 above". Those are in the commit-graph runbook: name that section.
Before rm -rf, tell the operator to confirm that the path is <cache dir>/<repo ID>.git. A mistyped path is the main data-loss risk.
refLockPaths walks each ref's subtree on a failed batch. This runs only on the failure path, but a large ref tree makes it slow.
The log says the locks "block ref updates", but the batch can fail for another reason. Consider "may block".

Generated with xum • Model: anthropic:claude-opus-5-5 • Thinking: high

Design: automatic, safe removal of stale ref locks

Status: proposal. No code until the desk decides. Facts below are from main a09d6563 and from throwaway experiments (git 2.55, Linux).

Recommendation

Option E now, with Option A ready on a trigger. Keep #485's detection plus the manual runbook. Build Option A (below) when the trigger fires: one report of ref lock(s) … block ref updates, or a desk decision that cache repos must heal themselves.

Reasons:

Since fix(serve): stop git children when the shutdown drain times out #472, a stop sends git SIGTERM first, and git removes its own locks on SIGTERM. A stale lock now needs a SIGKILL inside a ref transaction that lasts milliseconds: the 1-second kill after SIGTERM, the OOM killer, or a host crash.
Option A is the only design I found whose proof does not rest on path or /proc heuristics. It still has residual holes (see the Options table), it touches every git call site, and it costs about 650 lines in two PRs.
The runbook already heals the replica in one stop and start.

Option A in one paragraph: a kernel lock that every serve-started git holds for its lifetime. Serve opens <repo>/codestorage.gitlease.lock, takes a shared flock on it and passes it to the git child as fd 3. The parent closes its own copy right after Start. The lock then lives exactly as long as the git process tree: git, its hooks and their children all hold the inherited descriptor. Cleanup takes the lock exclusively with LOCK_NB. If that succeeds, no git process that serve started for this repository is alive, so none can own a ref lock. Cleanup then moves each old lock to a quarantine directory and deletes it.

1. How the code proves that no git process can own a lock

The proof does not scan /proc. It does not look at paths, working directories, argv or environment. That is why the gaps from the earlier rounds disappear:

Case	Why Option A covers it
Hydration's own `for-each-ref` (deletion pass and update pass)	Cleanup runs after both sync passes return and their children are reaped, under the hydrate lock. The failed batch returns its error. Hydration then cleans up and runs the whole sync once more. No hydration child is alive at the check.
Linked worktrees, `GIT_COMMON_DIR`, relative `--git-dir`	Not relevant: the lease is a descriptor, not a path. A serve-started git holds it wherever its cwd or git dir points.
Other users' processes, unreadable `/proc` entries, `hidepid`, other PID namespaces	Not relevant: the kernel arbitrates `flock` across users, PID namespaces and containers on the same host. The cache directory is mode 0700 and owned by the serve user, so only that user or root can start git in it.
An orphaned git of a crashed serve	It still holds its inherited descriptor, so the exclusive try fails. This works without `Pdeathsig`, which product code does not set.
A process that starts after the check	Every serve-started git takes the shared lock before `Start`. While cleanup holds the exclusive lock, a new git waits. It then starts with the lock already gone.

Experiment E2 shows the mechanism. A shell took flock -s on fd 3 and ran git push to a local bare repo. The push started receive-pack, which started a pre-receive hook that slept. The shell then closed its copy. While the hook was alive, flock -n -x was refused. After a SIGKILL of the push's process group, it succeeded. Git (2.55) did not close the inherited descriptor before it ran the hook. Test row 1 pins this for every git version CI runs.

Coverage, the one thing the proof needs. Every git that serve starts in a cache repository must hold the lease. Today these call sites do that work:

Through gitlimit (one choke point): the githttp service and hydration calls (server.go, hydrate.go, commitgraph.go), gitread, and the REST writers in api. REST notes writes take ref locks in the cache repository itself (update-ref refs/notes/wref-tmp-*, git notes --ref).
Outside gitlimit:
- jobs/repack.go:87 runs pack-objects in the cache repository (read-only for refs).
- lfsscan/scan.go:84-107 runs rev-list and two cat-file processes in the cache repository (read-only).
- hook.go:2482, 2651, 2693, 2711, 2766 run read-only git. They run inside receive-pack's hook, so they inherit the lease.
- sync/worker*.go runs in its own temp directories, not in the cache.

gitlimit.Start(cmd) gets only the command, and deriving the repository from cmd.Dir would bring back the path logic. So the API must change: callers pass the repository (gitlimit.StartIn(repoPath, cmd) and the matching run helpers) at all 13 call sites, and repack and lfsscan move behind it. A debug assertion in gitlimit fails a test when a child's Dir is inside the cache root without a lease. A static test fails on a new exec.Command("git" in a cache-touching package that bypasses gitlimit, apart from an allow-list (hook.go, whose git inherits the lease). A missed call site is the one way this proof breaks. It is not fail-safe: if that site takes ref locks, cleanup can delete its live lock. That is why the audit is part of PR 1, not a later fix.

gc.auto=0 is pinned in cache repositories (hydrate.go:183), so no automatic gc or pack-refs runs.

Mixed binaries. A cache can outlive a binary: hydrate.go:3741 keeps the hydrate lock name so that old and new binaries serialize on one cache. A git started by an old binary holds no lease, and an orphan of a crashed old serve can still hold a ref lock after the upgrade. The 1-minute age gate does not cover a transaction parked longer than that. Mitigation: the first lease-aware serve that opens a cache writes a lease-epoch file, and cleanup removes only locks created after that epoch. Older locks stay with the manual runbook. The hole that remains is an old-binary orphan that survived the upgrade and creates a new lock after the epoch. This needs a SIGKILL of the old serve while a transaction is parked, and the orphan must outlive the restart.

Descriptor hygiene. A hook that reassigns fd 3 (exec 3>… in a shell hook) drops the lease for its own subtree. Serve installs its own hooks, but the reference-transaction hook is install-if-absent, so an operator-provided shell hook can do this. Test row 13 pins it, and the docs must say: do not touch fd 3 in hooks. No daemonizing descendant exists today. I found no such code, but I did not audit deployments.

Progress. Cleanup never blocks, but it can starve: any live serve git in that repository (a long clone, a REST read) refuses the exclusive try. On a wedged repository most requests fail at hydration before they start git, so holders are rare. Cleanup still needs a counter (openstorage_stale_ref_lock_cleanup_total{outcome} with removed, busy and kept) and a starvation test (row 14).

2. Why there is no race between the check and the delete

Cleanup holds the exclusive lease for the whole check, move and delete. It runs under the hydrate lock, so no second cleanup or hydration of that repository runs at the same time.
No serve-started git can start during cleanup (see the table). The only processes that can still touch a lock are processes serve did not start, which are out of contract, and an old-binary orphan (see Mixed binaries).
For each candidate lock, cleanup:
1. reads it with Lstat and requires a regular file older than 1 minute;
2. renames it into <repo>/codestorage-stale-locks/ (same filesystem, so atomic);
3. compares the moved file's device and inode with the Lstat result;
4. deletes it on a match. On a mismatch, it puts the file back with a no-replace rename, keeps it, and logs once.
A rename followed by an identity check is exact. Lstat followed by unlink by name can delete a different file.
The quarantine directory must be outside refs/. Experiment E3: a file refs/heads/x.lock.stale shows up in git for-each-ref as the ref refs/heads/x.lock.stale. A file in <repo>/codestorage-stale-locks/ does not.
Cleanup logs one line per run: the repository and the number of locks removed. Round 5 on fix(githttp): report stale ref locks and document their manual removal #485 showed that one line per lock is not bounded.

Cleanup never blocks: if the exclusive try fails, it keeps every lock and logs once. The serve goroutine never holds the shared lease while it waits for another lock, because only children hold it after Start. So there is no deadlock.

3. Simpler approaches

Rebuild the cache in place on this error. This needs the same proof: you cannot delete a directory that a live writer uses. It costs a full re-download of every pack. Rejected.
Build a fresh cache directory and swap it in. This never touches the stale lock, but:
- Live processes keep working in the old inode, while serve code that resolves RepoPath sees the new one. A push in flight would mix the two directories.
- The hydrate and pack locks live inside the repository directory, so waiters would hold locks on the dead copy.
- It re-downloads every pack and doubles disk use until the old copy is deleted, and that deletion needs the same proof.
Larger and riskier than Option A. Rejected.
git update-ref --no-deref with a fresh lock. This is not possible. Experiment E1: with refs/heads/x.lock present, both update-ref --no-deref and -c core.filesRefLockTimeout=300 fail with Unable to create '…/refs/heads/x.lock': File exists. The files backend always creates <ref>.lock exclusively. --no-deref only changes symref handling. Writing loose ref files around git's lock means ignoring a lock that a live writer can hold, which is exactly the unsafe case. Rejected.
Reftable backend. A killed writer leaves tables.list.lock, the same problem, and every cache would need a migration. Rejected.
Keep fix(githttp): report stale ref locks and document their manual removal #485 as is (Option E). Detection plus the manual runbook. Since fix(serve): stop git children when the shutdown drain times out #472, a stop sends git SIGTERM first, and git removes its own locks on SIGTERM. A stale lock now needs a SIGKILL inside a transaction that lasts milliseconds: the 1-second kill after SIGTERM, the OOM killer, or a host crash. See the recommendation for the trigger.

4. Test matrix

Each end-to-end row runs on PostgreSQL, local and S3-WAL.

#	Case	Expected
1	Lease unit test: child tree holds the shared lock after the parent closes its copy	exclusive try refused while alive, acquired after a group SIGKILL
2	Coverage: the debug assertion over the full `internal/githttp` and `internal/api` suites; the static `exec.Command("git"` audit	no git in a cache repository without a lease
3	Stale lock, no holder (the #485 reproduction with the lock aged)	lock quarantined and deleted, the retry serves the ref, one log line
4	Live holders: parked receive-pack in `prepared`, parked hook, parked REST notes write, a long upload-pack	lock kept, one log line
5	Orphan: serve SIGKILLed while receive-pack is parked, then a new serve on the same cache	lock kept while the orphan lives, removed after it exits
6	Writer starts during cleanup (test seam holds the exclusive lease)	the writer waits, then runs with no lock
7	Lock replaced between `Lstat` and rename (out-of-contract writer simulated)	file put back, lock kept, logged
8	Overflow syncs: more than one batch of deletions and of updates (small batch-size seam)	cleanup still runs: the round-2 and round-3 P1 cases
9	Young lock (under 1 minute)	kept
10	Quarantine directory	not visible to `for-each-ref`
11	Mutations: drop the lease from one call site; skip the identity check	rows 2, 4 and 7 fail
12	Builds	darwin and linux, amd64 and arm64 (`flock` exists on both, no `/proc` dependency)
13	A hook script that runs `exec 3>/dev/null` and sleeps	documents the hole: lock removable while that hook lives
14	A shared holder that never exits during cleanup	outcome `busy`, lock kept, counter incremented
15	A lock older than the lease epoch	kept, manual runbook

The rows that test a timing-dependent proof step (3 to 8 and 14) run 20× per backend. The others run once per lane.

5. Options

Option	What	Size	Residual risk
A	Lease plus cleanup with quarantine	about 650 lines, 2 PRs (lease API at 13 call sites plus the audit; then cleanup, tests and docs)	git started outside serve (an operator by hand, another tool) holds no lease. An old-binary orphan that survives an upgrade (see Mixed binaries). A hook that reassigns fd 3. A future call site that bypasses `gitlimit` is caught by the audit and the assertion in tests, not in production. Network filesystems break `flock`, and the docs already forbid them for the cache. Mitigations: the 1-minute age gate, the lease epoch, the identity check, and the docs rules.
E (recommended now)	Keep detection plus the runbook (#485)	0	A replica hit by #464 cannot serve that repository until an operator acts. Trigger to build A: one report, or a desk decision.
Swap rebuild	Fresh directory plus swap	over 600 lines	mixed-directory pushes, lock identity, pack re-download

I recommend E now and A on the trigger. If the desk wants automatic removal now, A is the design to build: its proof rests on one kernel guarantee and one audited choke point, not on a growing list of /proc heuristics.

Advisory

One clean-context advisor reviewed the first draft against the code at a09d6563. Verdict: revise (do not approve A as drafted; E is defensible until a report arrives). Its points and what I changed:

Mixed binaries break the lease proof (hydrate.go:3741). I added the lease epoch and named the remaining hole.
The coverage list missed lfsscan/scan.go and listed hook.go without lines. Both are now listed, and both are read-only.
gitlimit.Start cannot find the repository. The API change is now explicit, and the size estimate went from about 550 to about 650 lines.
fd 3 can be reassigned by a hook. This is now a residual risk, test row 13 and a docs rule.
Cleanup can starve. Added the counter and test row 14.
Daemonized descendants: none found, not audited in deployments. Stated.
Matrix cost: loops now run only the timing-dependent rows.

The advisor did not rerun experiments E1 to E3 or check the size estimate.

Generated with xum • Model: anthropic:claude-opus-5-5 • Thinking: high

Desk decision: Option E. Keep #485's detection and the manual runbook. Do not build the automatic removal now.

Reasons:

Since fix(serve): stop git children when the shutdown drain times out #472, git gets SIGTERM first and removes its own locks. A stale lock now needs a SIGKILL inside a ref transaction that lasts milliseconds.
The runbook fixes it with one replica stop and start, and no ref-derived path reaches a shell.
Option A is about 650 lines, touches every git call site, and still leaves holes: git that serve did not start, orphans from an older binary, and hooks that reassign fd 3.

Trigger to build Option A (the design in the comment above is the plan of record):

one real report of the log line ref lock(s) ... block ref updates, or
a product decision that cache repos must repair themselves without an operator.

This issue stays open with that trigger. Owner: the maintainer desk.

Generated with xum • Model: anthropic:claude-opus-5-5 • Thinking: medium

fix(githttp): give a directory without HEAD its lease epoch, and lock…

docs(skills): record the lane-schedule model after the epoch moved to…

docs(deploy): name the Git starts that fail on the lease file and two…

docs(githttp): name the modification time and size in the identity ch…

docs(skills): record the lane-schedule model after the hydration clea…

fix(githttp): treat any entry named HEAD as a HEAD when deciding to w…

docs(skills): record the lane-schedule model after the epoch test use…

docs(deploy): say to delete the epoch files before a rollback to a re…

Navigation Menu

Repository navigation

githttp: remove a stale ref lock automatically when no git can own it #490

Design: automatic, safe removal of stale ref locks

Recommendation

1. How the code proves that no git process can own a lock

2. Why there is no race between the check and the delete

3. Simpler approaches

4. Test matrix

5. Options

Advisory

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

githttp: remove a stale ref lock automatically when no git can own it #490

Description

Activity

ThomasK33 commented on Oct 14, 2026

ThomasK33 commented on Oct 14, 2026

ThomasK33 commented on Oct 14, 2026

Design: automatic, safe removal of stale ref locks

Recommendation

1. How the code proves that no git process can own a lock

2. Why there is no race between the check and the delete

3. Simpler approaches

4. Test matrix

5. Options

Advisory

ThomasK33 commented on Oct 14, 2026

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions