From: Jeff King Date: Wed, 03 Dec 2025 21:16:10 GMT Subject: Re: [PATCH] lockfile: add PID file for debugging stale locks Message-ID: <20251203211610.GA64204@coredump.intra.peff.net> In-Reply-To: On Tue, Dec 02, 2025 at 03:07:27PM +0000, Paulo Casaretto via GitGitGadget wrote: > The .lock.pid file is created when a lock is acquired (if enabled), and > automatically cleaned up when the lock is released (via commit or > rollback). The file is registered as a tempfile so it gets cleaned up > by signal and atexit handlers if the process terminates abnormally. 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