Re: [PATCH] lockfile: add PID file for debugging stale locks

On Tue, Dec 02, 2025 at 03:07:27PM +0000, Paulo Casaretto via GitGitGadget wrote:

I'm sympathetic to the goal of this series, and the implementation looks cleanly done. But I wonder if there might be some system-level side effects that make these .pid files awkward.

Temporarily having an extra .git/index.lock.pid file is probably not a big deal. But for other namespaces, like refs, we're colliding with names that have other meanings. So if we want to update refs/heads/foo, for example, we'll create refs/heads/foo.lock now. And after your patch, also refs/heads/foo.lock.pid.

The ".lock" suffix is special, in that we disallow it in a refname and know to skip it when iterating over loose refs. But for the ".pid" variant, we run the risk of colliding with a real branch named "foo.lock.pid", both for reading and writing.

On the writing side, creating foo.lock.pid may accidentally overwrite an existing branch. This can be mitigated by using O_EXCL when creating the pid file.

But we can see the writes in the opposite order, which I think can also lead to data loss. Something like:

  - process A wants to write branch "foo", so it holds
    refs/heads/foo.lock and now also the matching foo.lock.pid

  - process B wants to write branch "foo.lock.pid", so it holds
    refs/heads/foo.lock.pid.lock (and the matching pid)

  - process B finishes its write and atomically renames
    foo.lock.pid.lock into foo.lock.pid. It's expected to overwrite
    the existing file there, so now process A's pid file is gone.

  - process A finishes its write and tries to clean up its pid file.  So
    it deletes foo.lock.pid, which contains the actual data from process
    B's write.

And now process B's write has been lost (and maybe even the entire existence of the foo.lock.pid branch, if it was not also present in the packed-refs file).

On the reading side, anybody iterating refs/heads/ may racily see the foo.lock.pid file and think it is supposed to be an actual ref. So they open it, find it contains garbage, and then flag it as an error.

I think both could be mitigated if we disallowed ".lock.pid" as a suffix in refnames, but that is a big user-facing change.

In the long run, alternate ref stores like reftable won't suffer from this issue. It's possible there are other spots where we use lockfiles alongside arbitrarily-named files, though I couldn't think of any.

So I dunno what that means for your patch. I notice that the user has to enable the feature manually. But it feels more like it should be selective based on which subsystem is using the lockfile (so refs would never want it, but other lockfiles/tempfiles might).

-Peff