lockfile: add holder info file for debugging stale locks #2011

pcasaretto · 2025-11-27T13:20:16Z

CC: Taylor Blau [email protected]
cc: "D. Ben Knoble" [email protected]

hcmaATshopify

I would argue for using pid instead of holder everywhere.

Pros of pid

Precise: The only payload in the file is the PID. Using “pid” makes it 100% clear and avoids ambiguity.
Conventional: Many tools use .pid files or similar conventions, so the meaning is familiar.
Unambiguous Code: Variables like pid_file or read_lock_pid() are explicitly about PIDs, not other info.

Documentation/git.adoc

hcmaATshopify · 2025-12-01T16:38:35Z

lockfile.c

+		return NULL;
+
+	strbuf_addf(&holder_path, "%s%s", lock_path, LOCK_HOLDER_SUFFIX);
+	fd = open(holder_path.buf, O_WRONLY | O_CREAT | O_TRUNC, mode);


Please make sure mode is restrictive enough (match whatever the .lock has and is readable by the creator).

It does use the same mode parameter as the lock file:

// In lock_file(): lk->tempfile = create_tempfile_mode(filename.buf, mode); // lock file lk->pid_tempfile = create_lock_pid_file(filename.buf, mode); // PID file - same mode

lockfile.c

hcmaATshopify · 2025-12-01T16:46:36Z

lockfile.c

+
+		if (lock_holder_info_enabled() &&
+		    !read_lock_holder_pid(lock_path.buf, &holder_pid))
+			strbuf_addf(buf, _("Lock is held by process %"PRIuMAX".\n\n"),


Lock (acquired via file '%s') is being held by process .

Given this will print after the "Unable to create '/path/to/.git/index.lock': File exists." message, I think it would be redundant.

hcmaATshopify · 2025-12-01T16:48:27Z

lockfile.c

+			strbuf_addf(buf, _("Lock is held by process %"PRIuMAX".\n\n"),
+				    holder_pid);
+
+		strbuf_addstr(buf, _("Another git process seems to be running in this repository, e.g.\n"


If the feature is enabled, we can actually improve this now and be assertive (for instance, if the pid is no longer around, we can say this "lock file is stale and can be removed" or "lock file is valid and held by process "

turns out kill was already implement and cross platform. Added a liveness check!

hcmaATshopify · 2025-12-01T16:50:42Z

lockfile.c

 	return 0;
 }
+
+int rollback_lock_file(struct lock_file *lk)


I recommend renaming rollback_lock_file to abort_lock_file for clarity and consistency with Git’s conventions. abort_lock_file is likely the best fit for the Git codebase, as this matches the standard “commit/abort” terminology for transactional semantics.

This function is not new, I just needed to change it from being an inline function to add new functionality.

hcmaATshopify · 2025-12-01T17:00:23Z

lockfile.c

+	strbuf_addf(&holder_path, "%s%s", lock_path, LOCK_HOLDER_SUFFIX);
+	fd = open(holder_path.buf, O_WRONLY | O_CREAT | O_TRUNC, mode);
+	if (fd >= 0) {
+		strbuf_addf(&content, "%"PRIuMAX"\n", (uintmax_t)getpid());


maybe strbuf_addf(&content, "%ld\n", (long)getpid()); since pid_t is generally int or long; cast to long for portability

Are we sure this will work for non-Unix platforms that might be supported by git? In other words, do we need to ifdef this whole thing so we don't break non-Unix builds?

Looking around the codebase it seems (uintmax_t)getpid() + PRIuMAX pattern is the established Git convention for portable PID handling:
Examples:

daemon.c:1458 - writes PID to file:

write_file(pid_file, "%"PRIuMAX, (uintmax_t) getpid());

builtin/gc.c:789-790 - writes PID to lockfile:

strbuf_addf(&sb, "%"PRIuMAX" %s", (uintmax_t) getpid(), my_host);

In terms of cross platform compatibility, I think I got lucky because it was already a first class citizen.
Did a little research and it works across the board.

getpid() is available on all Git-supported platforms (provided by libc on Unix, emulated on Windows)

pid_t is defined everywhere (native on Unix, typedef int on Windows via mingw-posix.h)

lockfile.c

hcmaATshopify · 2025-12-02T14:04:03Z

Thanks for doing this @pcasaretto - LGTM!

When a lock file is held, it can be helpful to know which process owns it, especially when debugging stale locks left behind by crashed processes. Add an optional feature that creates a companion .lock.pid file alongside each lock file, containing the PID of the lock holder. 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. When a lock conflict occurs, the code checks if the PID from the .pid file is still running using kill(pid, 0). This allows providing context-aware error messages. With PID info enabled: Lock is held by process 12345. Wait for it to finish, or remove the lock file to continue. Or for a stale lock: Lock was held by process 12345, which is no longer running. Remove the stale lock file to continue. Without PID info (default): Another git process seems to be running in this repository. Wait for it to finish, or remove the lock file to continue. The feature is opt-in via GIT_LOCK_PID_INFO=1 environment variable. Signed-off-by: Paulo Casaretto <[email protected]>

pcasaretto · 2025-12-02T15:00:43Z

/preview

gitgitgadget · 2025-12-02T15:01:54Z

Preview email sent as [email protected]

pcasaretto · 2025-12-02T15:03:56Z

/submit

gitgitgadget · 2025-12-02T15:07:54Z

Submitted as [email protected]

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-2011/pcasaretto/pid-holder-file-v1

To fetch this version to local tag pr-2011/pcasaretto/pid-holder-file-v1:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-2011/pcasaretto/pid-holder-file-v1

gitgitgadget · 2025-12-02T22:46:34Z

On the Git mailing list, "D. Ben Knoble" wrote (reply to this):

On Tue, Dec 2, 2025 at 10:07 AM Paulo Casaretto via GitGitGadget
<[email protected]> wrote:
>
> From: Paulo Casaretto <[email protected]>
>
> When a lock file is held, it can be helpful to know which process owns
> it, especially when debugging stale locks left behind by crashed
> processes. Add an optional feature that creates a companion .lock.pid
> file alongside each lock file, containing the PID of the lock holder.
>
> 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.
>
> When a lock conflict occurs, the code checks if the PID from the .pid
> file is still running using kill(pid, 0). This allows providing
> context-aware error messages. With PID info enabled:
>
>   Lock is held by process 12345. Wait for it to finish, or remove
>   the lock file to continue.
>
> Or for a stale lock:
>
>   Lock was held by process 12345, which is no longer running.
>   Remove the stale lock file to continue.
>
> Without PID info (default):
>
>   Another git process seems to be running in this repository.
>   Wait for it to finish, or remove the lock file to continue.
>
> The feature is opt-in via GIT_LOCK_PID_INFO=1 environment variable.
>
> Signed-off-by: Paulo Casaretto <[email protected]>

Sounds interesting. I think by the time I wish I knew what else was
using the lockfile, it's too late for me to alter my environment.
Perhaps (in addition to allowing the environment opt-in) we could
opt-in via configuration? Or is this really only useful, say, on the
server side where the environment is carefully controlled? I don't
relish putting this variable into my environment to take advantage of
something that looks very useful.

Are there downsides that make it necessary to be opt-in? I also
imagine this could be a useful default; occasionally folks at work hit
something similar and ask "what's up with that?"

Only other thing is: just because a process X is running doesn't mean
it was the one holding the lock, right? Since PIDs can be reused.

-- 
D. Ben Knoble

gitgitgadget · 2025-12-02T22:46:36Z

User "D. Ben Knoble" <[email protected]> has been added to the cc: list.

This comment was marked as spam.

Sign in to view

hcmaATshopify reviewed Dec 1, 2025

View reviewed changes

pcasaretto force-pushed the pid-holder-file branch from 5452ae9 to 51dbf28 Compare December 2, 2025 13:03

hcmaATshopify reviewed Dec 2, 2025

View reviewed changes

lockfile.c Outdated Show resolved Hide resolved

lockfile.c Outdated Show resolved Hide resolved

pcasaretto force-pushed the pid-holder-file branch from 51dbf28 to 7068ec0 Compare December 2, 2025 14:13

pcasaretto marked this pull request as ready for review December 2, 2025 14:14

lockfile: add holder info file for debugging stale locks #2011

Are you sure you want to change the base?

lockfile: add holder info file for debugging stale locks #2011

Conversation

pcasaretto commented Nov 27, 2025 • edited by gitgitgadget bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as spam.

hcmaATshopify left a comment

Choose a reason for hiding this comment

Pros of pid

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

hcmaATshopify commented Dec 2, 2025

Uh oh!

pcasaretto commented Dec 2, 2025

Uh oh!

gitgitgadget bot commented Dec 2, 2025

Uh oh!

pcasaretto commented Dec 2, 2025

Uh oh!

gitgitgadget bot commented Dec 2, 2025

Uh oh!

gitgitgadget bot commented Dec 2, 2025

Uh oh!

gitgitgadget bot commented Dec 2, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

pcasaretto commented Nov 27, 2025 •

edited by gitgitgadget bot

Loading