From: Taylor Blau <me@ttaylorr.com>
Date: Wed, 03 Dec 2025 23:19:46 GMT
Subject: Re: [PATCH] lockfile: add PID file for debugging stale locks
Message-ID: <aTDFks3RW57Ytwvq@nand.local>
In-Reply-To: <20251203211610.GA64204@coredump.intra.peff.net>

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.

> 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)

Changing the naming scheme as above would cause us to hold
"foo.pid.lock" in addition to "foo.lock". That would allow process B
here to write branch "foo.lock.pid" (as is the case today). But if the
scenario were instead "process B wants to write branch foo.pid.lock", it
would fail immediately since the ".lock" suffix is reserved.

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

Yeah, I don't think that we should change the refname constraints here,
especially in a world where reftable deployments are more common. In
that world I think we should err on the side of removing constraints,
not adding them ;-).

> 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).

Yeah, I think that something similar to the "which files do we fsync()
and how?" configuration we have today would be a nice complement here.

(As an aside, I wonder if that interface, too, could be slightly
improved. Right now we have a comma-separated list of values in the
"core.fsync" configuration for listing different "components", and then
a global core.fsyncMethod to either issue a fsync(), or a pagecache
writeback, or writeout-only flushes in batches. It might be nice to have
something like:

  [fsync "loose-object"]
    method = fsync
  [fsync "packfile"]
    method = writeout

, so the analog here would be something like:

  [lockfile "refs"]
    pidfile = false
  [lockfile "index"]
    pidfile = true

or similar. That could also be represented as core.lockfile=index,
omitting "refs" to avoid tracking it. It may be that people don't really
care to ever use different fsync methods for different fsync-able
components, so perhaps the analogy doesn't hold up perfectly.)

Thanks,
Taylor