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

From: Patrick Steinhardt <ps@pks.im>
Date: Dec 5, 2025, 11:03 UTC
Message-ID: <aTK77RFUvwpI-L13@pks.im>
In-Reply-To: <aTDFks3RW57Ytwvq@nand.local>

On Wed, Dec 03, 2025 at 06:19:46PM -0500, Taylor Blau wrote:

Show 38 quoted lines

> On Wed, Dec 03, 2025 at 04:16:10PM -0500, Jeff King wrote:
> > 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.
> 
> Good point. I don't have a strong opinion on whether or not we should
> use an append-only log of which PIDs grabbed which lockfiles when versus
> tracking them on a per-lock basis. But I wonder if this would be
> mitigated by either:
> 
>  - Keeping the ".lock" suffix as-is, so that holding a lockfile at path
>    "$GIT_DIR/index.lock" would create "$GIT_DIR/index-pid.lock" or
>    something similar.
> 
>  - Introducing a new reference name constraint that treats ".lock.pid"
>    as a reserved in a manner identical to how we currently treat
>    ".lock".
> 
> Between the two, I vastly prefer the former, but see below for more on
> why.

Yeah, I agree that this is not really a feasible change for the "files" reference backend. Besides all the issues mentioned in this thread, we also have to consider alternative implementations of Git and old versions. Those wouldn't know that the ".lock.pid" files are special, so they will misbehave if Git started to write them now.

We could of course work around that issue by introducing a new repository extension. But I doubt that this is something we want to pursue in this context. The provided benefit just isn't high enough.

The overall idea still has merit though. So if we still want to pursue it we can likely work around the issue by introducing a toggle that allows specific callers to opt out of creating the PID files.

That'd raise the question though when we are most likely to need the PID files for debugging stuff. From my own experience I only ever had issues with stale locks in the ref subsystem, so if we disable the mechanism in exactly that subsystem it may be way less useful.

If references are the main culprit one could also think about a slightly ugly alternative approach: loose refs handle it just fine if they contain additional lines. So in theory, we could write a second line for each lock file that contains the PID. We do have an fsck check that warns about this, but in theory this should just work. Whether we want to go there is a different question though.

Patrick