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

[Taylor Blau <me@ttaylorr.com>]

Hi Paulo,

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.

(For the purposes of this review, I'll ignore the naming conventions
that are discussed elsewhere in the thread, which I think can be
resolved separately of any technical concerns.)

> diff --git a/Documentation/git.adoc b/Documentation/git.adoc
> index ce099e78b8..6fdd509d34 100644
> --- a/Documentation/git.adoc
> +++ b/Documentation/git.adoc
> @@ -1010,6 +1010,16 @@ be in the future).
>  	the background which do not want to cause lock contention with
>  	other operations on the repository.  Defaults to `1`.
>
> +`GIT_LOCK_PID_INFO`::
> +	If this Boolean environment variable is set to `1`, Git will create
> +	a `.lock.pid` file alongside each lock file containing the PID of the
> +	process that created the lock. This information is displayed in error
> +	messages when a lock conflict occurs, making it easier to identify
> +	stale locks or debug locking issues. The PID files are automatically
> +	cleaned up via signal and atexit handlers; however, if a process is
> +	terminated abnormally (e.g., SIGKILL), the file may remain as a stale
> +	indicator. Disabled by default.

Regardless of whether or not we expose this functionality behind an
environment variable or configuration, I think it would be nice to be
able to turn PID tracking on and off for different components (e.g., for
scenarios where you care about who is holding open, say, $GIT_DIR/index,
but not who is creating a lock during ref creation).

If we determined this through an environment variable, I think it would
be reasonable to adopt the convention from the "core.fsync"
configuration and use a comma-separated list. Alternatively, we could
adopt that same convention for a configuration variable, say,
"core.lockfile".

But I think that having this exposed as a per-component setting via
configuration is preferable than a global switch, since callers don't
have to remember to set this variable in their environment to get the
desired effect. Callers that want to opt-in or out of this feature on a
one-off basis can still override the configuration via the top-level
"-c" flag.

>  `GIT_REDIRECT_STDIN`::
>  `GIT_REDIRECT_STDOUT`::
>  `GIT_REDIRECT_STDERR`::
> diff --git a/lockfile.c b/lockfile.c
> index 1d5ed01682..4a694b9c3d 100644
> --- a/lockfile.c
> +++ b/lockfile.c
> @@ -6,6 +6,9 @@
>  #include "abspath.h"
>  #include "gettext.h"
>  #include "lockfile.h"
> +#include "parse.h"
> +#include "strbuf.h"
> +#include "wrapper.h"
>
>  /*
>   * path = absolute or relative path name
> @@ -71,6 +74,62 @@ static void resolve_symlink(struct strbuf *path)
>  	strbuf_reset(&link);
>  }
>
> +/*
> + * Lock PID file functions - write PID to a .lock.pid file alongside
> + * the lock file for debugging stale locks. The PID file is registered
> + * as a tempfile so it gets cleaned up by signal/atexit handlers.
> + */
> +
> +static int lock_pid_info_enabled(void)
> +{

With the above suggestion, I think this function would get a little more
complicated to instead take a component argument. That's not a huge deal
in and of itself, but callers that *create* a lockfile will have to
somehow pass in the component name when acquiring the lock.

> +	return git_env_bool(GIT_LOCK_PID_INFO_ENVIRONMENT, 0);
> +}
> +
> +static struct tempfile *create_lock_pid_file(const char *lock_path, int mode)
> +{
> +	struct strbuf pid_path = STRBUF_INIT;
> +	struct strbuf content = STRBUF_INIT;
> +	struct tempfile *pid_tempfile = NULL;
> +	int fd;
> +
> +	if (!lock_pid_info_enabled())
> +		return NULL;
> +
> +	strbuf_addf(&pid_path, "%s%s", lock_path, LOCK_PID_SUFFIX);
> +	fd = open(pid_path.buf, O_WRONLY | O_CREAT | O_TRUNC, mode);

I'm not sure whether or not we should pass O_EXCL here, but I think it
depends on the naming convention we pick.

> +	if (fd >= 0) {

This is a style preference, but I'd suggest handling the happy path
outside of this conditional if possible by inverting it. Perhaps
something like:

    if (fd < 0)
        goto out;

    strbuf_addf(&content, ...);
    if (write_in_full(fd, content.buf, content.len) < 0)
        warning_errno(...);

    close(fd);
    pid_tempfile = register_tempfile(pid_path.buf);

    out:
        strbuf_release(...);
        return pid_tempfile;

> +static int read_lock_pid(const char *lock_path, uintmax_t *pid_out)
> +{
> +	struct strbuf pid_path = STRBUF_INIT;
> +	struct strbuf content = STRBUF_INIT;
> +	int ret = -1;
> +
> +	strbuf_addf(&pid_path, "%s%s", lock_path, LOCK_PID_SUFFIX);
> +	if (strbuf_read_file(&content, pid_path.buf, LOCK_PID_MAXLEN) > 0) {
> +		char *endptr;
> +		*pid_out = strtoumax(content.buf, &endptr, 10);
> +		if (*pid_out > 0 && (*endptr == '\n' || *endptr == '\0'))
> +			ret = 0;

Same note here. I'd suggest setting "ret = 0" during initialization, and
inverting this conditional to:

    if (*pid_out <= 0 || (*endptr != '\n' && *endptr != '\0')) {
        warning(...);
        ret = -1;
        goto out;
    }
> +		else
> +			warning(_("malformed lock pid file '%s'"), pid_path.buf);
> +	}
> +	strbuf_release(&pid_path);
> +	strbuf_release(&content);
> +	return ret;
> +}
> +
>  /* Make sure errno contains a meaningful value on error */
>  static int lock_file(struct lock_file *lk, const char *path, int flags,
>  		     int mode)
> @@ -80,9 +139,12 @@ static int lock_file(struct lock_file *lk, const char *path, int flags,
>  	strbuf_addstr(&filename, path);
>  	if (!(flags & LOCK_NO_DEREF))
>  		resolve_symlink(&filename);
> -
>  	strbuf_addstr(&filename, LOCK_SUFFIX);
> +

These look like stray whitespace changes that were left behind from
development.

> +		if (pid_status == 1)
> +			strbuf_addf(buf, _("Lock is held by process %"PRIuMAX". "
> +			    "Wait for it to finish, or remove the lock file to continue"),
> +			    pid);
> +		else if (pid_status == -1)
> +			strbuf_addf(buf, _("Lock was held by process %"PRIuMAX", "
> +			    "which is no longer running. Remove the stale lock file to continue"),
> +			    pid);
> +		else
> +			strbuf_addstr(buf, _("Another git process seems to be running in this repository. "
> +			    "Wait for it to finish, or remove the lock file to continue"));

On one hand I prefer the new "Another git process" message for when we
don't have a PID lockfile. But on the other hand, I think the "If it
still fails, a git process may have crashed..." message is useful for
users who may not be immediately aware of the consequences of simply
removing the lockfile to continue.

I do think the original message is somewhat verbose, so maybe the change
here in the non-PID case is worth doing. What are your thoughts?

> +
> +		strbuf_release(&lock_path);
>  	} else
>  		strbuf_addf(buf, _("Unable to create '%s.lock': %s"),
>  			    absolute_path(path), strerror(err));
> @@ -207,6 +292,8 @@ int commit_lock_file(struct lock_file *lk)
>  {
>  	char *result_path = get_locked_file_path(lk);
>
> +	delete_tempfile(&lk->pid_tempfile);
> +

Do we want to wait to delete the PID file until after we know that we
successfully committed the lockfile?

> +/* Maximum length for PID file content */
> +#define LOCK_PID_MAXLEN 32

Makes sense ;-). This should be plenty of space, since on my system
maximum PID value is 2^22:

    $ cat /proc/sys/kernel/pid_max
    4194304

> +test_expect_success 'PID info file cleaned up on successful operation when enabled' '
> +	git init repo4 &&
> +	(
> +		cd repo4 &&
> +		echo content >file &&
> +		env GIT_LOCK_PID_INFO=1 git add file &&
> +		# After successful add, no lock or PID files should exist
> +		! test -f .git/index.lock &&
> +		! test -f .git/index.lock.pid

These should be:

    test_path_is_missing .git/index.lock &&
    test_path_is_missing .git/index.lock.pid

instead of bare "! test -f"'s.

Thanks,
Taylor