[PATCH] config: retry acquiring config.lock for 100ms

[PATCH] config: retry acquiring config.lock for 100ms

[Joerg Thalheim]

From: Jörg Thalheim <joerg@thalheim.io>

When multiple processes write to a config file concurrently, they
contend on its ".lock" file, which is acquired via open(O_EXCL) with
no retry. The losers fail immediately with "could not lock config
file". Two processes writing unrelated keys (say, "branch.a.remote"
and "branch.b.remote") have no semantic conflict, yet one of them
fails for a purely mechanical reason.

This bites in practice when running `git worktree add -b` concurrently
against the same repository. Each invocation makes several writes to
".git/config" to set up branch tracking, and tooling that creates
worktrees in parallel sees intermittent failures. Worse, `git worktree
add` does not propagate the failed config write to its exit code: the
worktree is created and the command exits 0, but tracking
configuration is silently dropped.

The lock is held only for the duration of rewriting a small file, so
retrying for 100 ms papers over any realistic contention while still
failing fast if a stale lock has been left behind by a crashed
process. This mirrors what we already do for individual reference
locks (4ff0f01cb7 (refs: retry acquiring reference locks for 100ms,
2017-08-21)).

Signed-off-by: Jörg Thalheim <joerg@thalheim.io>
---
 config.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/config.c b/config.c
index 156f2a24fa..f7aff8725d 100644
--- a/config.c
+++ b/config.c
@@ -2903,6 +2903,14 @@ char *git_config_prepare_comment_string(const char *comment)
 	return prepared;
 }
 
+/*
+ * How long to retry acquiring config.lock when another process holds it.
+ * The lock is held only for the duration of rewriting a small file, so
+ * 100 ms covers any realistic contention while still failing fast if
+ * a stale lock has been left behind by a crashed process.
+ */
+#define CONFIG_LOCK_TIMEOUT_MS 100
+
 static void validate_comment_string(const char *comment)
 {
 	size_t leading_blanks;
@@ -2986,7 +2994,8 @@ int repo_config_set_multivar_in_file_gently(struct repository *r,
 	 * The lock serves a purpose in addition to locking: the new
 	 * contents of .git/config will be written into it.
 	 */
-	fd = hold_lock_file_for_update(&lock, config_filename, 0);
+	fd = hold_lock_file_for_update_timeout(&lock, config_filename, 0,
+					       CONFIG_LOCK_TIMEOUT_MS);
 	if (fd < 0) {
 		error_errno(_("could not lock config file %s"), config_filename);
 		ret = CONFIG_NO_LOCK;
@@ -3331,7 +3340,8 @@ static int repo_config_copy_or_rename_section_in_file(
 	if (!config_filename)
 		config_filename = filename_buf = repo_git_path(r, "config");
 
-	out_fd = hold_lock_file_for_update(&lock, config_filename, 0);
+	out_fd = hold_lock_file_for_update_timeout(&lock, config_filename, 0,
+						   CONFIG_LOCK_TIMEOUT_MS);
 	if (out_fd < 0) {
 		ret = error(_("could not lock config file %s"), config_filename);
 		goto out;
-- 
2.53.0

Re: [PATCH] config: retry acquiring config.lock for 100ms

[Junio C Hamano]

Joerg Thalheim <joerg@thalheim.io> writes:

> From: Jörg Thalheim <joerg@thalheim.io>
>
> When multiple processes write to a config file concurrently, they
> contend on its ".lock" file, which is acquired via open(O_EXCL) with
> no retry. The losers fail immediately with "could not lock config
> file". Two processes writing unrelated keys (say, "branch.a.remote"
> and "branch.b.remote") have no semantic conflict, yet one of them
> fails for a purely mechanical reason.
>
> This bites in practice when running `git worktree add -b` concurrently
> against the same repository. Each invocation makes several writes to
> ".git/config" to set up branch tracking, and tooling that creates
> worktrees in parallel sees intermittent failures. Worse, `git worktree
> add` does not propagate the failed config write to its exit code: the
> worktree is created and the command exits 0, but tracking
> configuration is silently dropped.
>
> The lock is held only for the duration of rewriting a small file, so
> retrying for 100 ms papers over any realistic contention while still
> failing fast if a stale lock has been left behind by a crashed
> process. This mirrors what we already do for individual reference
> locks (4ff0f01cb7 (refs: retry acquiring reference locks for 100ms,
> 2017-08-21)).
>
> Signed-off-by: Jörg Thalheim <joerg@thalheim.io>
> ---
>  config.c | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)

OK.  This is consistent with the default used for a ref update with
files backend.  Various subsystems use their own value randomly
chosen out of thin air, it seems.  credential-store uses 1000ms, gc
uses 150ms to interact with launchctl, refs have their own per
backend, etc.

> diff --git a/config.c b/config.c
> index 156f2a24fa..f7aff8725d 100644
> --- a/config.c
> +++ b/config.c
> @@ -2903,6 +2903,14 @@ char *git_config_prepare_comment_string(const char *comment)
>  	return prepared;
>  }
>  
> +/*
> + * How long to retry acquiring config.lock when another process holds it.
> + * The lock is held only for the duration of rewriting a small file, so
> + * 100 ms covers any realistic contention while still failing fast if
> + * a stale lock has been left behind by a crashed process.
> + */
> +#define CONFIG_LOCK_TIMEOUT_MS 100
> +

Making this configurable would make a cute chicken-and-egg problem?

;-)  No, no need for that---just kidding.

Will queue.  Thanks.

> @@ -2986,7 +2994,8 @@ int repo_config_set_multivar_in_file_gently(struct repository *r,
>  	 * The lock serves a purpose in addition to locking: the new
>  	 * contents of .git/config will be written into it.
>  	 */
> -	fd = hold_lock_file_for_update(&lock, config_filename, 0);
> +	fd = hold_lock_file_for_update_timeout(&lock, config_filename, 0,
> +					       CONFIG_LOCK_TIMEOUT_MS);
>  	if (fd < 0) {
>  		error_errno(_("could not lock config file %s"), config_filename);
>  		ret = CONFIG_NO_LOCK;
> @@ -3331,7 +3340,8 @@ static int repo_config_copy_or_rename_section_in_file(
>  	if (!config_filename)
>  		config_filename = filename_buf = repo_git_path(r, "config");
>  
> -	out_fd = hold_lock_file_for_update(&lock, config_filename, 0);
> +	out_fd = hold_lock_file_for_update_timeout(&lock, config_filename, 0,
> +						   CONFIG_LOCK_TIMEOUT_MS);
>  	if (out_fd < 0) {
>  		ret = error(_("could not lock config file %s"), config_filename);
>  		goto out;

Re: [PATCH] config: retry acquiring config.lock for 100ms

[Patrick Steinhardt]

On Fri, Apr 03, 2026 at 12:01:35PM +0200, Joerg Thalheim wrote:
> From: Jörg Thalheim <joerg@thalheim.io>
> 
> When multiple processes write to a config file concurrently, they
> contend on its ".lock" file, which is acquired via open(O_EXCL) with
> no retry. The losers fail immediately with "could not lock config
> file". Two processes writing unrelated keys (say, "branch.a.remote"
> and "branch.b.remote") have no semantic conflict, yet one of them
> fails for a purely mechanical reason.
> 
> This bites in practice when running `git worktree add -b` concurrently
> against the same repository. Each invocation makes several writes to
> ".git/config" to set up branch tracking, and tooling that creates
> worktrees in parallel sees intermittent failures. Worse, `git worktree
> add` does not propagate the failed config write to its exit code: the
> worktree is created and the command exits 0, but tracking
> configuration is silently dropped.

This very much sounds like a bug that is worth fixing independently.

> The lock is held only for the duration of rewriting a small file, so
> retrying for 100 ms papers over any realistic contention while still
> failing fast if a stale lock has been left behind by a crashed
> process. This mirrors what we already do for individual reference
> locks (4ff0f01cb7 (refs: retry acquiring reference locks for 100ms,
> 2017-08-21)).

Famous last words :) Experience tells me that any timeout value that
isn't excessive will eventually be hit in some production system. Which
raises the question whether we want to make the timeout configurable,
similar to "core.filesRefLockTimeout" and "core.packedRefsTimeout".

> diff --git a/config.c b/config.c
> index 156f2a24fa..f7aff8725d 100644
> --- a/config.c
> +++ b/config.c
> @@ -2903,6 +2903,14 @@ char *git_config_prepare_comment_string(const char *comment)
>  	return prepared;
>  }
>  
> +/*
> + * How long to retry acquiring config.lock when another process holds it.
> + * The lock is held only for the duration of rewriting a small file, so
> + * 100 ms covers any realistic contention while still failing fast if
> + * a stale lock has been left behind by a crashed process.
> + */
> +#define CONFIG_LOCK_TIMEOUT_MS 100
> +
>  static void validate_comment_string(const char *comment)
>  {
>  	size_t leading_blanks;
> @@ -2986,7 +2994,8 @@ int repo_config_set_multivar_in_file_gently(struct repository *r,
>  	 * The lock serves a purpose in addition to locking: the new
>  	 * contents of .git/config will be written into it.
>  	 */
> -	fd = hold_lock_file_for_update(&lock, config_filename, 0);
> +	fd = hold_lock_file_for_update_timeout(&lock, config_filename, 0,
> +					       CONFIG_LOCK_TIMEOUT_MS);
>  	if (fd < 0) {
>  		error_errno(_("could not lock config file %s"), config_filename);
>  		ret = CONFIG_NO_LOCK;
> @@ -3331,7 +3340,8 @@ static int repo_config_copy_or_rename_section_in_file(
>  	if (!config_filename)
>  		config_filename = filename_buf = repo_git_path(r, "config");
>  
> -	out_fd = hold_lock_file_for_update(&lock, config_filename, 0);
> +	out_fd = hold_lock_file_for_update_timeout(&lock, config_filename, 0,
> +						   CONFIG_LOCK_TIMEOUT_MS);
>  	if (out_fd < 0) {
>  		ret = error(_("could not lock config file %s"), config_filename);
>  		goto out;

Okay, so we now handle this more gracefully with concurrent writers. But
even if we handle that more gracefully, we still don't really have any
guarantee that the outcome will be reasonable, or even close.

The main difference to the locking timeouts we have for refs is that we
have an easy way to check for conflicts there: we simpliy verify that
the refs we want to update haven't moved from their expected value. We
have no such thing for our configuration though, so two processes that
race with one another will happily overwrite each other's changes.

Honestly though, I'm not really sure what to make with this. We could
of course also add some validation that the configuration we want to set
hasn't been modified meanwhile. But that would now lead to a situation
where we have to update every single caller in our tree to make use of
the new mechanism, which would be a bunch of work.

And adding the timeout doesn't really change the status quo, either. We
already have the case that we'll happily overwrite changes made by
concurrent processes. The only thing that changes is that we make it
more likely for concurrent changes to succeed.

So I'm a bit cautious, but I think overall I'm fine with this?

Thanks!

Patrick

Re: [PATCH] config: retry acquiring config.lock for 100ms

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

>> This bites in practice when running `git worktree add -b` concurrently
>> against the same repository. Each invocation makes several writes to
>> ".git/config" to set up branch tracking, and tooling that creates
>> worktrees in parallel sees intermittent failures. Worse, `git worktree
>> add` does not propagate the failed config write to its exit code: the
>> worktree is created and the command exits 0, but tracking
>> configuration is silently dropped.
>
> This very much sounds like a bug that is worth fixing independently.
>
>> The lock is held only for the duration of rewriting a small file, so
>> retrying for 100 ms papers over any realistic contention while still
>> failing fast if a stale lock has been left behind by a crashed
>> process. This mirrors what we already do for individual reference
>> locks (4ff0f01cb7 (refs: retry acquiring reference locks for 100ms,
>> 2017-08-21)).
>
> Famous last words :) Experience tells me that any timeout value that
> isn't excessive will eventually be hit in some production system. Which
> raises the question whether we want to make the timeout configurable,
> similar to "core.filesRefLockTimeout" and "core.packedRefsTimeout".
> ...
> Honestly though, I'm not really sure what to make with this. We could
> of course also add some validation that the configuration we want to set
> hasn't been modified meanwhile. But that would now lead to a situation
> where we have to update every single caller in our tree to make use of
> the new mechanism, which would be a bunch of work.
>
> And adding the timeout doesn't really change the status quo, either. We
> already have the case that we'll happily overwrite changes made by
> concurrent processes. The only thing that changes is that we make it
> more likely for concurrent changes to succeed.

We haven't heard any response to these points raised in the message
I am responding to.  Should I still keep the patch in my tree,
hoping that a responses may come some day?  I am tempted to discard
the topic as it has been quite a while since we last looked at it.

Thanks.

Re: [PATCH] config: retry acquiring config.lock for 100ms

[Patrick Steinhardt]

On Mon, May 11, 2026 at 11:32:33AM +0900, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> >> This bites in practice when running `git worktree add -b` concurrently
> >> against the same repository. Each invocation makes several writes to
> >> ".git/config" to set up branch tracking, and tooling that creates
> >> worktrees in parallel sees intermittent failures. Worse, `git worktree
> >> add` does not propagate the failed config write to its exit code: the
> >> worktree is created and the command exits 0, but tracking
> >> configuration is silently dropped.
> >
> > This very much sounds like a bug that is worth fixing independently.
> >
> >> The lock is held only for the duration of rewriting a small file, so
> >> retrying for 100 ms papers over any realistic contention while still
> >> failing fast if a stale lock has been left behind by a crashed
> >> process. This mirrors what we already do for individual reference
> >> locks (4ff0f01cb7 (refs: retry acquiring reference locks for 100ms,
> >> 2017-08-21)).
> >
> > Famous last words :) Experience tells me that any timeout value that
> > isn't excessive will eventually be hit in some production system. Which
> > raises the question whether we want to make the timeout configurable,
> > similar to "core.filesRefLockTimeout" and "core.packedRefsTimeout".
> > ...
> > Honestly though, I'm not really sure what to make with this. We could
> > of course also add some validation that the configuration we want to set
> > hasn't been modified meanwhile. But that would now lead to a situation
> > where we have to update every single caller in our tree to make use of
> > the new mechanism, which would be a bunch of work.
> >
> > And adding the timeout doesn't really change the status quo, either. We
> > already have the case that we'll happily overwrite changes made by
> > concurrent processes. The only thing that changes is that we make it
> > more likely for concurrent changes to succeed.
> 
> We haven't heard any response to these points raised in the message
> I am responding to.  Should I still keep the patch in my tree,
> hoping that a responses may come some day?  I am tempted to discard
> the topic as it has been quite a while since we last looked at it.

Same here, let's discard it for now as it can be easily added back in
once a new version was posted or the feedback has been addressed.

Patrick

Re: [PATCH] config: retry acquiring config.lock for 100ms

[Jörg Thalheim]

I am not really sure what you want me to do here.
I don't see how git can have this value configurable, given it's about reading the configuration itself.
Is the user supposed via command line?

Meanwhile the project I was facing this issue, added the required file lock in its own code,
which has since then worked perfectly to fix my use case: https://github.com/raine/workmux/issues/116



May 11, 2026 at 4:32 AM, "Junio C Hamano" <gitster@pobox.com mailto:gitster@pobox.com?to=%22Junio%20C%20Hamano%22%20%3Cgitster%40pobox.com%3E > wrote:


> 
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > 
> > > 
> > > This bites in practice when running `git worktree add -b` concurrently
> > >  against the same repository. Each invocation makes several writes to
> > >  ".git/config" to set up branch tracking, and tooling that creates
> > >  worktrees in parallel sees intermittent failures. Worse, `git worktree
> > >  add` does not propagate the failed config write to its exit code: the
> > >  worktree is created and the command exits 0, but tracking
> > >  configuration is silently dropped.
> > > 
> >  This very much sounds like a bug that is worth fixing independently.
> > 
> > > 
> > > The lock is held only for the duration of rewriting a small file, so
> > >  retrying for 100 ms papers over any realistic contention while still
> > >  failing fast if a stale lock has been left behind by a crashed
> > >  process. This mirrors what we already do for individual reference
> > >  locks (4ff0f01cb7 (refs: retry acquiring reference locks for 100ms,
> > >  2017-08-21)).
> > > 
> >  Famous last words :) Experience tells me that any timeout value that
> >  isn't excessive will eventually be hit in some production system. Which
> >  raises the question whether we want to make the timeout configurable,
> >  similar to "core.filesRefLockTimeout" and "core.packedRefsTimeout".
> >  ...
> >  Honestly though, I'm not really sure what to make with this. We could
> >  of course also add some validation that the configuration we want to set
> >  hasn't been modified meanwhile. But that would now lead to a situation
> >  where we have to update every single caller in our tree to make use of
> >  the new mechanism, which would be a bunch of work.
> > 
> >  And adding the timeout doesn't really change the status quo, either. We
> >  already have the case that we'll happily overwrite changes made by
> >  concurrent processes. The only thing that changes is that we make it
> >  more likely for concurrent changes to succeed.
> > 
> We haven't heard any response to these points raised in the message
> I am responding to. Should I still keep the patch in my tree,
> hoping that a responses may come some day? I am tempted to discard
> the topic as it has been quite a while since we last looked at it.
> 
> Thanks.
>

Re: [PATCH] config: retry acquiring config.lock for 100ms

[Patrick Steinhardt]

On Mon, May 11, 2026 at 09:06:00AM +0000, Jörg Thalheim wrote:
> May 11, 2026 at 4:32 AM, "Junio C Hamano" <gitster@pobox.com mailto:gitster@pobox.com?to=%22Junio%20C%20Hamano%22%20%3Cgitster%40pobox.com%3E > wrote:
> > Patrick Steinhardt <ps@pks.im> writes:
> > > > This bites in practice when running `git worktree add -b` concurrently
> > > >  against the same repository. Each invocation makes several writes to
> > > >  ".git/config" to set up branch tracking, and tooling that creates
> > > >  worktrees in parallel sees intermittent failures. Worse, `git worktree
> > > >  add` does not propagate the failed config write to its exit code: the
> > > >  worktree is created and the command exits 0, but tracking
> > > >  configuration is silently dropped.
> > > > 
> > >  This very much sounds like a bug that is worth fixing independently.
> > > 
> > > > 
> > > > The lock is held only for the duration of rewriting a small file, so
> > > >  retrying for 100 ms papers over any realistic contention while still
> > > >  failing fast if a stale lock has been left behind by a crashed
> > > >  process. This mirrors what we already do for individual reference
> > > >  locks (4ff0f01cb7 (refs: retry acquiring reference locks for 100ms,
> > > >  2017-08-21)).
> > > > 
> > >  Famous last words :) Experience tells me that any timeout value that
> > >  isn't excessive will eventually be hit in some production system. Which
> > >  raises the question whether we want to make the timeout configurable,
> > >  similar to "core.filesRefLockTimeout" and "core.packedRefsTimeout".
> > >  ...
> > >  Honestly though, I'm not really sure what to make with this. We could
> > >  of course also add some validation that the configuration we want to set
> > >  hasn't been modified meanwhile. But that would now lead to a situation
> > >  where we have to update every single caller in our tree to make use of
> > >  the new mechanism, which would be a bunch of work.
> > > 
> > >  And adding the timeout doesn't really change the status quo, either. We
> > >  already have the case that we'll happily overwrite changes made by
> > >  concurrent processes. The only thing that changes is that we make it
> > >  more likely for concurrent changes to succeed.
> > > 
> > We haven't heard any response to these points raised in the message
> > I am responding to. Should I still keep the patch in my tree,
> > hoping that a responses may come some day? I am tempted to discard
> > the topic as it has been quite a while since we last looked at it.
> 
> I am not really sure what you want me to do here.

In general, the idea here is to engage in a discussion that can
ultimately lead to one of two outcomes:

  - The discussion surfaces an area the author hasn't thought about, so
    the patch is adapted accordingly.

  - The discussion shows that the author already did think about the
    issue, but hasn't documented the assumptions. In this case, it
    should be the commit message that gets adapted.

> I don't see how git can have this value configurable, given it's about
> reading the configuration itself. Is the user supposed via command
> line?

This is a fair point indeed. But if it's not possible to change via the
configuration itself, then the next-best thing might be to introduce an
environment variable that allows configuring it.

The other aspect that wasn't discussed in the commit message is how
concurrent writes are handled, both when they are non-conflicting
(updating different keys) and when they are conflicting (updating the
same key). After spending some more time in the code I think it's
ultimately nothing we have to worry about too much, as we only start
reading the configuration after we've locked it.

So in the semantically non-conflicting case there isn't really much of a
race, because things already work as expected. But in the semantically
conflicting case it's a bit different, as the latter writer will
overwrite the result of the former one. In theory it would be possible
to detect such conflicts by:

  - Reading the configuration file.

  - Taking the lock.

  - Rereading the configuration to check for conflicts.

But even that is racy as the first writer might have succeeded before we
read the configuration the first time. So I'm not sure whether we can do
anything about that in the first place, as the race basically exists in
the outer loop controlled by the caller.

So there probably isn't much we can do about that, and unless I missed
something I think your timeout is sensible. But ideally, such nuances
would be discussed as part of the commit message so that reviewers and
future readers are made aware of them.

Thanks!

Patrick