[PATCH 0/2] builtin/maintenance: fix locking and respect "gc.auto"

[PATCH 0/2] builtin/maintenance: fix locking and respect "gc.auto"

[Patrick Steinhardt]

Hi,

this patch series addresses the issues reported in [1]. The series is
built on top of Git 2.54.0.

Thanks!

Patrick

[1]: <CAKcFC3arsYExb5dCMQspo4V9UFDadFaj8Q4PUsMWZJw_eYrMzA@mail.gmail.com>

---
Patrick Steinhardt (2):
      builtin/maintenance: fix locking with "--detach"
      run-command: honor "gc.auto" for auto-maintenance

 builtin/gc.c           | 26 ++++++++++++--
 lockfile.c             |  9 +++++
 lockfile.h             | 10 ++++++
 run-command.c          |  6 ++--
 setup.c                | 31 +++++++++++-----
 setup.h                |  1 +
 t/t7900-maintenance.sh | 95 ++++++++++++++++++++++++++++++++++++++++++++------
 7 files changed, 154 insertions(+), 24 deletions(-)


---
base-commit: 13ef77ce6e222bef3ab145642e6ef1486075211c
change-id: 20260511-pks-maintenance-fix-lock-with-detach-a608e9b6adeb

[PATCH 1/2] builtin/maintenance: fix locking with "--detach"

[Patrick Steinhardt]

When running git-maintenance(1), we create a lockfile that is supposed
to keep other maintenance processes from running at the same time. This
lockfile is broken though in case the "--detach" flag is passed: the
lockfile is created by the parent process and will be cleaned up either
manually or on exit. But when detaching, the parent will exit before all
of the background maintenance tasks have been ran, and consequently the
lock only covers a smaller part of the whole maintenance process.

Fix this bug by introducing two new functions:

  - `daemonize_without_exit()` is the same as `daemonize()`, but doesn't
    call exit(3p) for the parent process.

  - `lock_file_reassign_owner()` reassigns the owner of its owned
    tempfiles so that they don't get unlinked anymore when the previous
    owner exits.

Together this allows us to reassign ownership of the lockfile after we
have daemonized so that the lockfile is now owned by the child process.

Reported-by: Jean-Christophe Manciot <actionmystique@gmail.com>
Helped-by: Jeff King <peff@peff.net>
Helped-by: Taylor Blau <me@ttaylorr.com>
Helped-by: Derrick Stolee <stolee@gmail.com>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/gc.c           | 26 ++++++++++++++++++++--
 lockfile.c             |  9 ++++++++
 lockfile.h             | 10 +++++++++
 setup.c                | 31 +++++++++++++++++++--------
 setup.h                |  1 +
 t/t7900-maintenance.sh | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 124 insertions(+), 11 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index 3a71e314c9..09cb92ac97 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -1810,10 +1810,32 @@ static int maintenance_run_tasks(struct maintenance_run_opts *opts,
 				   TASK_PHASE_FOREGROUND))
 			result = 1;
 
-	/* Failure to daemonize is ok, we'll continue in foreground. */
 	if (opts->detach > 0) {
+		pid_t child_pid;
+
 		trace2_region_enter("maintenance", "detach", the_repository);
-		daemonize();
+
+		child_pid = daemonize_without_exit();
+		if (!child_pid) {
+			/*
+			 * We're in the child process, so we take ownership of
+			 * the lockfile.
+			 */
+			lock_file_reassign_owner(&lk, getpid());
+		} else if (child_pid > 0) {
+			/*
+			 * We're in the parent process, so we assign ownership
+			 * of the lockfile to the child and then exit immediately.
+			 */
+			lock_file_reassign_owner(&lk, child_pid);
+			exit(0);
+		} else {
+			/*
+			 * Failure to daemonize is ok, we'll continue in
+			 * foreground.
+			 */
+		}
+
 		trace2_region_leave("maintenance", "detach", the_repository);
 	}
 
diff --git a/lockfile.c b/lockfile.c
index 7add2f136a..96aab3c885 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -356,3 +356,12 @@ int rollback_lock_file(struct lock_file *lk)
 	delete_tempfile(&lk->pid_tempfile);
 	return delete_tempfile(&lk->tempfile);
 }
+
+void lock_file_reassign_owner(struct lock_file *lk, pid_t owner)
+{
+	if (!is_lock_file_locked(lk))
+		BUG("cannot reassign ownership of unlocked lockfile");
+	lk->tempfile->owner = owner;
+	if (lk->pid_tempfile)
+		lk->pid_tempfile->owner = owner;
+}
diff --git a/lockfile.h b/lockfile.h
index e7233f28de..0b10b624fa 100644
--- a/lockfile.h
+++ b/lockfile.h
@@ -341,4 +341,14 @@ static inline int commit_lock_file_to(struct lock_file *lk, const char *path)
  */
 int rollback_lock_file(struct lock_file *lk);
 
+/*
+ * Reassign ownership of the lockfile to a different process.
+ *
+ * This is intended for use after `fork(2)`-ing. The parent transfers ownership
+ * to the daemonized child so that its atexit handler does not unlink the lock
+ * that should outlive it, and the child claims the inherited tempfiles so that
+ * they are cleaned up when the daemon exits.
+ */
+void lock_file_reassign_owner(struct lock_file *lk, pid_t owner);
+
 #endif /* LOCKFILE_H */
diff --git a/setup.c b/setup.c
index 7ec4427368..34deb6e985 100644
--- a/setup.c
+++ b/setup.c
@@ -2156,20 +2156,18 @@ void sanitize_stdfds(void)
 		close(fd);
 }
 
-int daemonize(void)
+pid_t daemonize_without_exit(void)
 {
 #ifdef NO_POSIX_GOODIES
 	errno = ENOSYS;
 	return -1;
 #else
-	switch (fork()) {
-		case 0:
-			break;
-		case -1:
-			die_errno(_("fork failed"));
-		default:
-			exit(0);
-	}
+	pid_t pid = fork();
+	if (pid < 0)
+		return -1;
+	if (pid > 0)
+		return pid;
+
 	if (setsid() == -1)
 		die_errno(_("setsid failed"));
 	close(0);
@@ -2180,6 +2178,21 @@ int daemonize(void)
 #endif
 }
 
+int daemonize(void)
+{
+#ifdef NO_POSIX_GOODIES
+	errno = ENOSYS;
+	return -1;
+#else
+	pid_t pid = daemonize_without_exit();
+	if (pid < 0)
+		die_errno(_("fork failed"));
+	if (pid > 0)
+		exit(0);
+	return 0;
+#endif
+}
+
 struct template_dir_cb_data {
 	char *path;
 	int initialized;
diff --git a/setup.h b/setup.h
index 80bc6e5f07..396af8d808 100644
--- a/setup.h
+++ b/setup.h
@@ -150,6 +150,7 @@ int path_inside_repo(const char *prefix, const char *path);
 
 void sanitize_stdfds(void);
 int daemonize(void);
+pid_t daemonize_without_exit(void);
 
 /*
  * GIT_REPO_VERSION is the version we write by default. The
diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
index 4700beacc1..df0bbc1669 100755
--- a/t/t7900-maintenance.sh
+++ b/t/t7900-maintenance.sh
@@ -1438,6 +1438,64 @@ test_expect_success '--no-detach causes maintenance to not run in background' '
 	)
 '
 
+test_expect_success PIPE '--detach holds maintenance lock until daemonized child exits' '
+	test_when_finished "rm -rf repo" &&
+	git init repo &&
+	(
+		cd repo &&
+
+		git config maintenance.auto false &&
+		git config core.lockfilepid true &&
+
+		git remote add origin /does/not/exist &&
+		git config set remote.origin.uploadpack "cat fifo-uploadpack" &&
+
+		mkfifo fifo-uploadpack fifo-maint &&
+
+		# Open the maintenance FIFO, as otherwise spawning
+		# git-maintenance(1) would block. Note that we need to open it
+		# as read-write, as otherwise we would block here already.
+		exec 9<>fifo-maint &&
+
+		{ git maintenance run --task=prefetch --detach 7>&9 & } &&
+		parent="$!" &&
+
+		# Reap the parent process so that the exec call below will not
+		# get SIGCHLD.
+		wait "$parent" &&
+
+		# Open the git-upload-pack(1) FIFO for writing, which will
+		# block until the upload-pack script opens it for reading. Once
+		# exec returns, we know that the daemonized child is alive and
+		# pinned.
+		exec 8>fifo-uploadpack &&
+
+		test_path_is_file .git/objects/maintenance.lock &&
+		test_path_is_file .git/objects/"maintenance~pid.lock" &&
+
+		# Verify that the maintenance.lock still exists, and
+		# that it was created by the parent process, not the
+		# child.
+		echo "pid $parent" >expect &&
+		test_cmp expect .git/objects/"maintenance~pid.lock" &&
+
+		# Reopen the maintenance FIFO as read-only so that
+		# git-maintenance(1) is the only writer. This will cause it to
+		# close the FIFO once the process exits.
+		exec 9<&- &&
+		exec 9<fifo-maint &&
+
+		# Close the FIFO used by git-upload-pack(1) to unblock it and
+		# then wait until the maintenance FIFO is closed by
+		# git-maintenance(1), indicating that it has exited.
+		exec 8>&- &&
+		cat <&9 &&
+
+		test_path_is_missing .git/objects/maintenance.lock &&
+		test_path_is_missing .git/objects/"maintenance~pid.lock"
+	)
+'
+
 test_expect_success '--detach causes maintenance to run in background' '
 	test_when_finished "rm -rf repo" &&
 	git init repo &&

-- 
2.54.0.545.g6539524ca2.dirty

[PATCH 2/2] run-command: honor "gc.auto" for auto-maintenance

[Patrick Steinhardt]

The "gc.auto" configuration has traditionally been used to turn off
running git-gc(1) as part of our auto-maintenance. We have eventually
switched over to git-maintenance(1) in a95ce12430 (maintenance: replace
run_auto_gc(), 2020-09-17), and with 1942d48380 (maintenance: optionally
skip --auto process, 2020-08-28) we have introduced "maintenance.auto"
to control whether or not to run auto-maintenance.

At that point though we still shelled out to git-gc(1) internally. So
if "gc.auto=0" was set we would still _execute_ git-maintenance(1), but
the command would have exited fast because git-gc(1) itself knew to
honor the config key.

This has recently changed though, as we have adapted the default
maintenance strategy to not use git-gc(1) anymore. The consequence is
that "gc.auto=0" doesn't have an effect anymore, which is a somewhat
surprising change in behaviour for our users.

Adapt `run_auto_maintenance()` so that it knows to also read "gc.auto",
similar to how it also reads both "maintenance.autoDetach" and
"gc.autoDetach".

Reported-by: Jean-Christophe Manciot <actionmystique@gmail.com>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 run-command.c          |  6 ++++--
 t/t7900-maintenance.sh | 37 ++++++++++++++++++++++++++-----------
 2 files changed, 30 insertions(+), 13 deletions(-)

diff --git a/run-command.c b/run-command.c
index c146a56532..1e7b789010 100644
--- a/run-command.c
+++ b/run-command.c
@@ -1946,8 +1946,10 @@ int prepare_auto_maintenance(struct repository *r, int quiet,
 {
 	int enabled, auto_detach;
 
-	if (!repo_config_get_bool(r, "maintenance.auto", &enabled) &&
-	    !enabled)
+	if (repo_config_get_bool(r, "maintenance.auto", &enabled) &&
+	    repo_config_get_bool(r, "gc.auto", &enabled))
+		enabled = 1;
+	if (!enabled)
 		return 0;
 
 	/*
diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
index df0bbc1669..1f70462678 100755
--- a/t/t7900-maintenance.sh
+++ b/t/t7900-maintenance.sh
@@ -60,17 +60,32 @@ test_expect_success 'run [--auto|--quiet] with gc strategy' '
 	test_subcommand git gc --no-quiet --no-detach --skip-foreground-tasks <run-no-quiet.txt
 '
 
-test_expect_success 'maintenance.auto config option' '
-	GIT_TRACE2_EVENT="$(pwd)/default" git commit --quiet --allow-empty -m 1 &&
-	test_subcommand git maintenance run --auto --quiet --detach <default &&
-	GIT_TRACE2_EVENT="$(pwd)/true" \
-		git -c maintenance.auto=true \
-		commit --quiet --allow-empty -m 2 &&
-	test_subcommand git maintenance run --auto --quiet --detach <true &&
-	GIT_TRACE2_EVENT="$(pwd)/false" \
-		git -c maintenance.auto=false \
-		commit --quiet --allow-empty -m 3 &&
-	test_subcommand ! git maintenance run --auto --quiet --detach <false
+for cfg in maintenance.auto gc.auto
+do
+	test_expect_success "$cfg config option" '
+		GIT_TRACE2_EVENT="$(pwd)/default" git commit --quiet --allow-empty -m 1 &&
+		test_subcommand git maintenance run --auto --quiet --detach <default &&
+		GIT_TRACE2_EVENT="$(pwd)/true" \
+			git -c $cfg=true commit --quiet --allow-empty -m 2 &&
+		test_subcommand git maintenance run --auto --quiet --detach <true &&
+		GIT_TRACE2_EVENT="$(pwd)/false" \
+			git -c $cfg=false commit --quiet --allow-empty -m 3 &&
+		test_subcommand ! git maintenance run --auto --quiet --detach <false
+	'
+done
+
+test_expect_success "maintenance.auto overrides gc.auto" '
+	test_when_finished "rm -f trace" &&
+
+	test_config maintenance.auto false &&
+	test_config gc.auto true &&
+	GIT_TRACE2_EVENT="$(pwd)/trace" git commit --quiet --allow-empty -m 1 &&
+	test_subcommand ! git maintenance run --auto --quiet --detach <trace &&
+
+	test_config maintenance.auto true &&
+	test_config gc.auto false &&
+	GIT_TRACE2_EVENT="$(pwd)/trace" git commit --quiet --allow-empty -m 1 &&
+	test_subcommand git maintenance run --auto --quiet --detach <trace
 '
 
 for cfg in maintenance.autoDetach gc.autoDetach

-- 
2.54.0.545.g6539524ca2.dirty

Re: [PATCH 2/2] run-command: honor "gc.auto" for auto-maintenance

[Jeff King]

On Mon, May 11, 2026 at 02:29:56PM +0200, Patrick Steinhardt wrote:

> @@ -1946,8 +1946,10 @@ int prepare_auto_maintenance(struct repository *r, int quiet,
>  {
>  	int enabled, auto_detach;
>  
> -	if (!repo_config_get_bool(r, "maintenance.auto", &enabled) &&
> -	    !enabled)
> +	if (repo_config_get_bool(r, "maintenance.auto", &enabled) &&
> +	    repo_config_get_bool(r, "gc.auto", &enabled))
> +		enabled = 1;
> +	if (!enabled)
>  		return 0;

gc.auto isn't a bool; it's the count of loose objects after which to run
maintenance. So "0" works in both contexts, but will we complain if
gc.auto is set to 100? I think maybe not, because we fall back to
git_parse_int(), but it feels kind of fragile.

The gc code uses repo_config_get_int() here.

-Peff

Re: [PATCH 1/2] builtin/maintenance: fix locking with "--detach"

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> diff --git a/builtin/gc.c b/builtin/gc.c
> index 3a71e314c9..09cb92ac97 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -1810,10 +1810,32 @@ static int maintenance_run_tasks(struct maintenance_run_opts *opts,
>  				   TASK_PHASE_FOREGROUND))
>  			result = 1;
>  
> -	/* Failure to daemonize is ok, we'll continue in foreground. */
>  	if (opts->detach > 0) {
> +		pid_t child_pid;
> +
>  		trace2_region_enter("maintenance", "detach", the_repository);
> -		daemonize();
> +
> +		child_pid = daemonize_without_exit();
> +		if (!child_pid) {
> +			/*
> +			 * We're in the child process, so we take ownership of
> +			 * the lockfile.
> +			 */
> +			lock_file_reassign_owner(&lk, getpid());
> +		} else if (child_pid > 0) {
> +			/*
> +			 * We're in the parent process, so we assign ownership
> +			 * of the lockfile to the child and then exit immediately.
> +			 */
> +			lock_file_reassign_owner(&lk, child_pid);
> +			exit(0);

The point of reassigning the owner to somebody else is so that we
won't clean them when we exit as the tempfile.c::remove_tempfile()
function checks the "owner" is "me" and refrains from unlinking
those that do not belong to us, so there is nothing wrong in this
code, but this somehow felt awkward.  In a sense, child_pid here
does not have to be what fork() returned but anything that is not
our own pid.  Perhaps "we assign ... to the child" -> "we relinquish
... to prevent us removing upon exiting" would convey the intention
better?  I dunno.

> -int daemonize(void)
> +pid_t daemonize_without_exit(void)
>  {
>  #ifdef NO_POSIX_GOODIES
>  	errno = ENOSYS;
>  	return -1;
>  #else
> -	switch (fork()) {
> -		case 0:
> -			break;
> -		case -1:
> -			die_errno(_("fork failed"));
> -		default:
> -			exit(0);
> -	}
> +	pid_t pid = fork();
> +	if (pid < 0)
> +		return -1;
> +	if (pid > 0)
> +		return pid;
> +
>  	if (setsid() == -1)
>  		die_errno(_("setsid failed"));
>  	close(0);
> @@ -2180,6 +2178,21 @@ int daemonize(void)
>  #endif
>  }
>  
> +int daemonize(void)
> +{
> +#ifdef NO_POSIX_GOODIES
> +	errno = ENOSYS;
> +	return -1;
> +#else
> +	pid_t pid = daemonize_without_exit();
> +	if (pid < 0)
> +		die_errno(_("fork failed"));
> +	if (pid > 0)
> +		exit(0);
> +	return 0;
> +#endif
> +}

I was hoping that we can do without the #ifdef in this caller as
daemonize_without_exit() already has exactly the same condtional
compilation.  If the NO_POSIX_GOODIES side can just return silently
wit ENOSYS, shouldn't the callers be also fine if we return failure
instead of calling die_errno(_("fork failed")), I have to wonder.

But because (1) as long as we have to call die_errno() here, we must
keep the conditional compilation in daemonize() as well as
daemonize_without_exit(), and (2) changing what the callers get when
fork failed here is totally outside of this topic, I would say that
the code around here is good as-is.

Re: [PATCH 2/2] run-command: honor "gc.auto" for auto-maintenance

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Mon, May 11, 2026 at 02:29:56PM +0200, Patrick Steinhardt wrote:
>
>> @@ -1946,8 +1946,10 @@ int prepare_auto_maintenance(struct repository *r, int quiet,
>>  {
>>  	int enabled, auto_detach;
>>  
>> -	if (!repo_config_get_bool(r, "maintenance.auto", &enabled) &&
>> -	    !enabled)
>> +	if (repo_config_get_bool(r, "maintenance.auto", &enabled) &&
>> +	    repo_config_get_bool(r, "gc.auto", &enabled))
>> +		enabled = 1;
>> +	if (!enabled)
>>  		return 0;
>
> gc.auto isn't a bool; it's the count of loose objects after which to run
> maintenance. So "0" works in both contexts, but will we complain if
> gc.auto is set to 100? I think maybe not, because we fall back to
> git_parse_int(), but it feels kind of fragile.
>
> The gc code uses repo_config_get_int() here.
>
> -Peff

Very good point.  I was about to send the same message ;-)

Re: [PATCH 2/2] run-command: honor "gc.auto" for auto-maintenance

[Patrick Steinhardt]

On Tue, May 12, 2026 at 10:21:43AM +0900, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
> 
> > On Mon, May 11, 2026 at 02:29:56PM +0200, Patrick Steinhardt wrote:
> >
> >> @@ -1946,8 +1946,10 @@ int prepare_auto_maintenance(struct repository *r, int quiet,
> >>  {
> >>  	int enabled, auto_detach;
> >>  
> >> -	if (!repo_config_get_bool(r, "maintenance.auto", &enabled) &&
> >> -	    !enabled)
> >> +	if (repo_config_get_bool(r, "maintenance.auto", &enabled) &&
> >> +	    repo_config_get_bool(r, "gc.auto", &enabled))
> >> +		enabled = 1;
> >> +	if (!enabled)
> >>  		return 0;
> >
> > gc.auto isn't a bool; it's the count of loose objects after which to run
> > maintenance. So "0" works in both contexts, but will we complain if
> > gc.auto is set to 100? I think maybe not, because we fall back to
> > git_parse_int(), but it feels kind of fragile.
> >
> > The gc code uses repo_config_get_int() here.
> >
> > -Peff
> 
> Very good point.  I was about to send the same message ;-)

Ugh, true indeed. Will fix, thanks!

Patrick

Re: [PATCH 1/2] builtin/maintenance: fix locking with "--detach"

[Patrick Steinhardt]

On Tue, May 12, 2026 at 10:19:22AM +0900, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > diff --git a/builtin/gc.c b/builtin/gc.c
> > index 3a71e314c9..09cb92ac97 100644
> > --- a/builtin/gc.c
> > +++ b/builtin/gc.c
> > @@ -1810,10 +1810,32 @@ static int maintenance_run_tasks(struct maintenance_run_opts *opts,
> >  				   TASK_PHASE_FOREGROUND))
> >  			result = 1;
> >  
> > -	/* Failure to daemonize is ok, we'll continue in foreground. */
> >  	if (opts->detach > 0) {
> > +		pid_t child_pid;
> > +
> >  		trace2_region_enter("maintenance", "detach", the_repository);
> > -		daemonize();
> > +
> > +		child_pid = daemonize_without_exit();
> > +		if (!child_pid) {
> > +			/*
> > +			 * We're in the child process, so we take ownership of
> > +			 * the lockfile.
> > +			 */
> > +			lock_file_reassign_owner(&lk, getpid());
> > +		} else if (child_pid > 0) {
> > +			/*
> > +			 * We're in the parent process, so we assign ownership
> > +			 * of the lockfile to the child and then exit immediately.
> > +			 */
> > +			lock_file_reassign_owner(&lk, child_pid);
> > +			exit(0);
> 
> The point of reassigning the owner to somebody else is so that we
> won't clean them when we exit as the tempfile.c::remove_tempfile()
> function checks the "owner" is "me" and refrains from unlinking
> those that do not belong to us, so there is nothing wrong in this
> code, but this somehow felt awkward.  In a sense, child_pid here
> does not have to be what fork() returned but anything that is not
> our own pid.  Perhaps "we assign ... to the child" -> "we relinquish
> ... to prevent us removing upon exiting" would convey the intention
> better?  I dunno.

Fair. This is what I got now:

	/*
	 * We're in the parent process, so we drop ownership of
	 * the lockfile to prevent us from removing it upon
	 * exit.
	 */

> > -int daemonize(void)
> > +pid_t daemonize_without_exit(void)
> >  {
> >  #ifdef NO_POSIX_GOODIES
> >  	errno = ENOSYS;
> >  	return -1;
> >  #else
> > -	switch (fork()) {
> > -		case 0:
> > -			break;
> > -		case -1:
> > -			die_errno(_("fork failed"));
> > -		default:
> > -			exit(0);
> > -	}
> > +	pid_t pid = fork();
> > +	if (pid < 0)
> > +		return -1;
> > +	if (pid > 0)
> > +		return pid;
> > +
> >  	if (setsid() == -1)
> >  		die_errno(_("setsid failed"));
> >  	close(0);
> > @@ -2180,6 +2178,21 @@ int daemonize(void)
> >  #endif
> >  }
> >  
> > +int daemonize(void)
> > +{
> > +#ifdef NO_POSIX_GOODIES
> > +	errno = ENOSYS;
> > +	return -1;
> > +#else
> > +	pid_t pid = daemonize_without_exit();
> > +	if (pid < 0)
> > +		die_errno(_("fork failed"));
> > +	if (pid > 0)
> > +		exit(0);
> > +	return 0;
> > +#endif
> > +}
> 
> I was hoping that we can do without the #ifdef in this caller as
> daemonize_without_exit() already has exactly the same condtional
> compilation.  If the NO_POSIX_GOODIES side can just return silently
> wit ENOSYS, shouldn't the callers be also fine if we return failure
> instead of calling die_errno(_("fork failed")), I have to wonder.
> 
> But because (1) as long as we have to call die_errno() here, we must
> keep the conditional compilation in daemonize() as well as
> daemonize_without_exit(), and (2) changing what the callers get when
> fork failed here is totally outside of this topic, I would say that
> the code around here is good as-is.

Yeah, I was also pondering whether I can drop the additional ifdef. But
I eventually decided to aim for the most minimal fix that has the least
potential for additional regressions. So I aimed to keep `daemonize()`
semantically the same as before, and I aimed to only fix the issue with
the lockfile we know about.

I agree though that this is something we should probably clean up in a
subsequent series. We don't have that many callers of `daemonize()`
after all, so it shouldn't be that involved, either.

Thanks!

Patrick

[PATCH v2 0/2] builtin/maintenance: fix locking and respect "gc.auto"

[Patrick Steinhardt]

Hi,

this patch series addresses the issues reported in [1]. The series is
built on top of Git 2.54.0.

Changes in v2:
  - Clarify comment when dropping ownership of the lock in the parent
    process.
  - Properly treat "gc.auto" as an integer, not a boolean.
  - Link to v1: https://patch.msgid.link/20260511-pks-maintenance-fix-lock-with-detach-v1-0-ccd7d62c9a40@pks.im

Thanks!

Patrick

[1]: <CAKcFC3arsYExb5dCMQspo4V9UFDadFaj8Q4PUsMWZJw_eYrMzA@mail.gmail.com>

---
Patrick Steinhardt (2):
      builtin/maintenance: fix locking with "--detach"
      run-command: honor "gc.auto" for auto-maintenance

 builtin/gc.c           | 27 ++++++++++++++--
 lockfile.c             |  9 ++++++
 lockfile.h             | 10 ++++++
 run-command.c          | 10 ++++--
 setup.c                | 31 +++++++++++++------
 setup.h                |  1 +
 t/t7900-maintenance.sh | 83 ++++++++++++++++++++++++++++++++++++++++++++++++++
 7 files changed, 157 insertions(+), 14 deletions(-)

Range-diff versus v1:

1:  d0609c03b4 ! 1:  8dda16ec8d builtin/maintenance: fix locking with "--detach"
    @@ builtin/gc.c: static int maintenance_run_tasks(struct maintenance_run_opts *opts
     +			lock_file_reassign_owner(&lk, getpid());
     +		} else if (child_pid > 0) {
     +			/*
    -+			 * We're in the parent process, so we assign ownership
    -+			 * of the lockfile to the child and then exit immediately.
    ++			 * We're in the parent process, so we drop ownership of
    ++			 * the lockfile to prevent us from removing it upon
    ++			 * exit.
     +			 */
     +			lock_file_reassign_owner(&lk, child_pid);
     +			exit(0);
2:  959ce46f7d < -:  ---------- run-command: honor "gc.auto" for auto-maintenance
-:  ---------- > 2:  d5507b5dd2 run-command: honor "gc.auto" for auto-maintenance

---
base-commit: 13ef77ce6e222bef3ab145642e6ef1486075211c
change-id: 20260511-pks-maintenance-fix-lock-with-detach-a608e9b6adeb

[PATCH v2 1/2] builtin/maintenance: fix locking with "--detach"

[Patrick Steinhardt]

When running git-maintenance(1), we create a lockfile that is supposed
to keep other maintenance processes from running at the same time. This
lockfile is broken though in case the "--detach" flag is passed: the
lockfile is created by the parent process and will be cleaned up either
manually or on exit. But when detaching, the parent will exit before all
of the background maintenance tasks have been ran, and consequently the
lock only covers a smaller part of the whole maintenance process.

Fix this bug by introducing two new functions:

  - `daemonize_without_exit()` is the same as `daemonize()`, but doesn't
    call exit(3p) for the parent process.

  - `lock_file_reassign_owner()` reassigns the owner of its owned
    tempfiles so that they don't get unlinked anymore when the previous
    owner exits.

Together this allows us to reassign ownership of the lockfile after we
have daemonized so that the lockfile is now owned by the child process.

Reported-by: Jean-Christophe Manciot <actionmystique@gmail.com>
Helped-by: Jeff King <peff@peff.net>
Helped-by: Taylor Blau <me@ttaylorr.com>
Helped-by: Derrick Stolee <stolee@gmail.com>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/gc.c           | 27 +++++++++++++++++++++--
 lockfile.c             |  9 ++++++++
 lockfile.h             | 10 +++++++++
 setup.c                | 31 +++++++++++++++++++--------
 setup.h                |  1 +
 t/t7900-maintenance.sh | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 125 insertions(+), 11 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index 3a71e314c9..d866c19b92 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -1810,10 +1810,33 @@ static int maintenance_run_tasks(struct maintenance_run_opts *opts,
 				   TASK_PHASE_FOREGROUND))
 			result = 1;
 
-	/* Failure to daemonize is ok, we'll continue in foreground. */
 	if (opts->detach > 0) {
+		pid_t child_pid;
+
 		trace2_region_enter("maintenance", "detach", the_repository);
-		daemonize();
+
+		child_pid = daemonize_without_exit();
+		if (!child_pid) {
+			/*
+			 * We're in the child process, so we take ownership of
+			 * the lockfile.
+			 */
+			lock_file_reassign_owner(&lk, getpid());
+		} else if (child_pid > 0) {
+			/*
+			 * We're in the parent process, so we drop ownership of
+			 * the lockfile to prevent us from removing it upon
+			 * exit.
+			 */
+			lock_file_reassign_owner(&lk, child_pid);
+			exit(0);
+		} else {
+			/*
+			 * Failure to daemonize is ok, we'll continue in
+			 * foreground.
+			 */
+		}
+
 		trace2_region_leave("maintenance", "detach", the_repository);
 	}
 
diff --git a/lockfile.c b/lockfile.c
index 7add2f136a..96aab3c885 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -356,3 +356,12 @@ int rollback_lock_file(struct lock_file *lk)
 	delete_tempfile(&lk->pid_tempfile);
 	return delete_tempfile(&lk->tempfile);
 }
+
+void lock_file_reassign_owner(struct lock_file *lk, pid_t owner)
+{
+	if (!is_lock_file_locked(lk))
+		BUG("cannot reassign ownership of unlocked lockfile");
+	lk->tempfile->owner = owner;
+	if (lk->pid_tempfile)
+		lk->pid_tempfile->owner = owner;
+}
diff --git a/lockfile.h b/lockfile.h
index e7233f28de..0b10b624fa 100644
--- a/lockfile.h
+++ b/lockfile.h
@@ -341,4 +341,14 @@ static inline int commit_lock_file_to(struct lock_file *lk, const char *path)
  */
 int rollback_lock_file(struct lock_file *lk);
 
+/*
+ * Reassign ownership of the lockfile to a different process.
+ *
+ * This is intended for use after `fork(2)`-ing. The parent transfers ownership
+ * to the daemonized child so that its atexit handler does not unlink the lock
+ * that should outlive it, and the child claims the inherited tempfiles so that
+ * they are cleaned up when the daemon exits.
+ */
+void lock_file_reassign_owner(struct lock_file *lk, pid_t owner);
+
 #endif /* LOCKFILE_H */
diff --git a/setup.c b/setup.c
index 7ec4427368..34deb6e985 100644
--- a/setup.c
+++ b/setup.c
@@ -2156,20 +2156,18 @@ void sanitize_stdfds(void)
 		close(fd);
 }
 
-int daemonize(void)
+pid_t daemonize_without_exit(void)
 {
 #ifdef NO_POSIX_GOODIES
 	errno = ENOSYS;
 	return -1;
 #else
-	switch (fork()) {
-		case 0:
-			break;
-		case -1:
-			die_errno(_("fork failed"));
-		default:
-			exit(0);
-	}
+	pid_t pid = fork();
+	if (pid < 0)
+		return -1;
+	if (pid > 0)
+		return pid;
+
 	if (setsid() == -1)
 		die_errno(_("setsid failed"));
 	close(0);
@@ -2180,6 +2178,21 @@ int daemonize(void)
 #endif
 }
 
+int daemonize(void)
+{
+#ifdef NO_POSIX_GOODIES
+	errno = ENOSYS;
+	return -1;
+#else
+	pid_t pid = daemonize_without_exit();
+	if (pid < 0)
+		die_errno(_("fork failed"));
+	if (pid > 0)
+		exit(0);
+	return 0;
+#endif
+}
+
 struct template_dir_cb_data {
 	char *path;
 	int initialized;
diff --git a/setup.h b/setup.h
index 80bc6e5f07..396af8d808 100644
--- a/setup.h
+++ b/setup.h
@@ -150,6 +150,7 @@ int path_inside_repo(const char *prefix, const char *path);
 
 void sanitize_stdfds(void);
 int daemonize(void);
+pid_t daemonize_without_exit(void);
 
 /*
  * GIT_REPO_VERSION is the version we write by default. The
diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
index 4700beacc1..df0bbc1669 100755
--- a/t/t7900-maintenance.sh
+++ b/t/t7900-maintenance.sh
@@ -1438,6 +1438,64 @@ test_expect_success '--no-detach causes maintenance to not run in background' '
 	)
 '
 
+test_expect_success PIPE '--detach holds maintenance lock until daemonized child exits' '
+	test_when_finished "rm -rf repo" &&
+	git init repo &&
+	(
+		cd repo &&
+
+		git config maintenance.auto false &&
+		git config core.lockfilepid true &&
+
+		git remote add origin /does/not/exist &&
+		git config set remote.origin.uploadpack "cat fifo-uploadpack" &&
+
+		mkfifo fifo-uploadpack fifo-maint &&
+
+		# Open the maintenance FIFO, as otherwise spawning
+		# git-maintenance(1) would block. Note that we need to open it
+		# as read-write, as otherwise we would block here already.
+		exec 9<>fifo-maint &&
+
+		{ git maintenance run --task=prefetch --detach 7>&9 & } &&
+		parent="$!" &&
+
+		# Reap the parent process so that the exec call below will not
+		# get SIGCHLD.
+		wait "$parent" &&
+
+		# Open the git-upload-pack(1) FIFO for writing, which will
+		# block until the upload-pack script opens it for reading. Once
+		# exec returns, we know that the daemonized child is alive and
+		# pinned.
+		exec 8>fifo-uploadpack &&
+
+		test_path_is_file .git/objects/maintenance.lock &&
+		test_path_is_file .git/objects/"maintenance~pid.lock" &&
+
+		# Verify that the maintenance.lock still exists, and
+		# that it was created by the parent process, not the
+		# child.
+		echo "pid $parent" >expect &&
+		test_cmp expect .git/objects/"maintenance~pid.lock" &&
+
+		# Reopen the maintenance FIFO as read-only so that
+		# git-maintenance(1) is the only writer. This will cause it to
+		# close the FIFO once the process exits.
+		exec 9<&- &&
+		exec 9<fifo-maint &&
+
+		# Close the FIFO used by git-upload-pack(1) to unblock it and
+		# then wait until the maintenance FIFO is closed by
+		# git-maintenance(1), indicating that it has exited.
+		exec 8>&- &&
+		cat <&9 &&
+
+		test_path_is_missing .git/objects/maintenance.lock &&
+		test_path_is_missing .git/objects/"maintenance~pid.lock"
+	)
+'
+
 test_expect_success '--detach causes maintenance to run in background' '
 	test_when_finished "rm -rf repo" &&
 	git init repo &&

-- 
2.54.0.545.g6539524ca2.dirty

[PATCH v2 2/2] run-command: honor "gc.auto" for auto-maintenance

[Patrick Steinhardt]

The "gc.auto" configuration has traditionally been used to turn off
running git-gc(1) as part of our auto-maintenance. We have eventually
switched over to git-maintenance(1) in a95ce12430 (maintenance: replace
run_auto_gc(), 2020-09-17), and with 1942d48380 (maintenance: optionally
skip --auto process, 2020-08-28) we have introduced "maintenance.auto"
to control whether or not to run auto-maintenance.

At that point though we still shelled out to git-gc(1) internally. So
if "gc.auto=0" was set we would still _execute_ git-maintenance(1), but
the command would have exited fast because git-gc(1) itself knew to
honor the config key.

This has recently changed though, as we have adapted the default
maintenance strategy to not use git-gc(1) anymore. The consequence is
that "gc.auto=0" doesn't have an effect anymore, which is a somewhat
surprising change in behaviour for our users.

Adapt `run_auto_maintenance()` so that it knows to also read "gc.auto",
similar to how it also reads both "maintenance.autoDetach" and
"gc.autoDetach".

Reported-by: Jean-Christophe Manciot <actionmystique@gmail.com>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 run-command.c          | 10 +++++++---
 t/t7900-maintenance.sh | 25 +++++++++++++++++++++++++
 2 files changed, 32 insertions(+), 3 deletions(-)

diff --git a/run-command.c b/run-command.c
index c146a56532..28202a81d8 100644
--- a/run-command.c
+++ b/run-command.c
@@ -1944,10 +1944,14 @@ void run_processes_parallel(const struct run_process_parallel_opts *opts)
 int prepare_auto_maintenance(struct repository *r, int quiet,
 			     struct child_process *maint)
 {
-	int enabled, auto_detach;
+	int enabled = 1, auto_detach;
 
-	if (!repo_config_get_bool(r, "maintenance.auto", &enabled) &&
-	    !enabled)
+	if (repo_config_get_bool(r, "maintenance.auto", &enabled)) {
+		int gc_threshold;
+		if (!repo_config_get_int(r, "gc.auto", &gc_threshold))
+			enabled = gc_threshold > 0;
+	}
+	if (!enabled)
 		return 0;
 
 	/*
diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
index df0bbc1669..97c8c701bb 100755
--- a/t/t7900-maintenance.sh
+++ b/t/t7900-maintenance.sh
@@ -73,6 +73,31 @@ test_expect_success 'maintenance.auto config option' '
 	test_subcommand ! git maintenance run --auto --quiet --detach <false
 '
 
+test_expect_success 'gc.auto config option' '
+	GIT_TRACE2_EVENT="$(pwd)/default" git commit --quiet --allow-empty -m 1 &&
+	test_subcommand git maintenance run --auto --quiet --detach <default &&
+	GIT_TRACE2_EVENT="$(pwd)/true" \
+		git -c gc.auto=1 commit --quiet --allow-empty -m 2 &&
+	test_subcommand git maintenance run --auto --quiet --detach <true &&
+	GIT_TRACE2_EVENT="$(pwd)/false" \
+		git -c gc.auto=0 commit --quiet --allow-empty -m 3 &&
+	test_subcommand ! git maintenance run --auto --quiet --detach <false
+'
+
+test_expect_success 'maintenance.auto overrides gc.auto' '
+	test_when_finished "rm -f trace" &&
+
+	test_config maintenance.auto false &&
+	test_config gc.auto 1 &&
+	GIT_TRACE2_EVENT="$(pwd)/trace" git commit --quiet --allow-empty -m 1 &&
+	test_subcommand ! git maintenance run --auto --quiet --detach <trace &&
+
+	test_config maintenance.auto true &&
+	test_config gc.auto 0 &&
+	GIT_TRACE2_EVENT="$(pwd)/trace" git commit --quiet --allow-empty -m 1 &&
+	test_subcommand git maintenance run --auto --quiet --detach <trace
+'
+
 for cfg in maintenance.autoDetach gc.autoDetach
 do
 	test_expect_success "$cfg=true config option" '

-- 
2.54.0.545.g6539524ca2.dirty

Re: [PATCH v2 1/2] builtin/maintenance: fix locking with "--detach"

[Taylor Blau]

On Tue, May 12, 2026 at 10:30:30AM +0200, Patrick Steinhardt wrote:
> diff --git a/builtin/gc.c b/builtin/gc.c
> index 3a71e314c9..d866c19b92 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -1810,10 +1810,33 @@ static int maintenance_run_tasks(struct maintenance_run_opts *opts,
>  				   TASK_PHASE_FOREGROUND))
>  			result = 1;
>
> -	/* Failure to daemonize is ok, we'll continue in foreground. */
>  	if (opts->detach > 0) {
> +		pid_t child_pid;
> +
>  		trace2_region_enter("maintenance", "detach", the_repository);
> -		daemonize();
> +
> +		child_pid = daemonize_without_exit();
> +		if (!child_pid) {
> +			/*
> +			 * We're in the child process, so we take ownership of
> +			 * the lockfile.
> +			 */
> +			lock_file_reassign_owner(&lk, getpid());
> +		} else if (child_pid > 0) {
> +			/*
> +			 * We're in the parent process, so we drop ownership of
> +			 * the lockfile to prevent us from removing it upon
> +			 * exit.
> +			 */
> +			lock_file_reassign_owner(&lk, child_pid);
> +			exit(0);
> +		} else {
> +			/*
> +			 * Failure to daemonize is ok, we'll continue in
> +			 * foreground.
> +			 */
> +		}
> +

This works almost identically as the original implementation I shared[1]
original thread discussing this issue, but it takes a slightly different
approach.

Instead of doing this automatically as a part of daemonize(), this patch
only adjusts this singular call-site, and forces us to introduce a new
function daemonize_without_exit() in order to facilitate it.

I personally prefer the other approach, for many of the reasons that
Peff wrote about in [2, 3]. There are two questions in my mind that I
would be curious of your thoughts on:

 * Should we transfer ownership of all tempfiles, or a subset of them?

 * Should we perform that transfer automatically via daemonize(), or
   manually via its callers?

Judging from the other parts of the original thread, I think the answer
to the first question is an unambiguous "yes". The second question I
think there is less of a consensus on, but I'd like to advocate for
doing this automatically via daemonize().

To summarize some of my thoughts after reading [2], I think that because
daemonize() is about letting a process continue on in the background
rather than fork()-ing children and then continuing on ourselves, *not*
automatically handing those off feels like an accident waiting to
happen.

I can't think of a situation where someone would want to daemonize() but
not have the new process assume ownership of its locks and tempfiles. I
think an alternative approach here would be to document the behavior I'm
proposing above clearly in daemonize(). Should a caller arise in the
future that *doesn't* want to this behavior, then we could introduce a
function similar to your daemonize_without_exit() (maybe in this case it
would be daemonize_without_transfer(), since not exiting in a function
that is supposed to daemonize feels awkward).

I guess what I'm saying is that I feel that future daemonize() callers
are much more likely to want this behavior than not, and putting this in
daemonize() feels like the safer option between the two.

FWIW, I feel fairly strongly about ^ this, but I'm of course happy to
discuss more if you feel differently.

(If you do end up taking that approach, that makes the C changes
identical to the ones I shared in [1], so you are free to forge my
Co-authored-by and Signed-off-by lines if you want to take that approach
instead.)

> diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
> index 4700beacc1..df0bbc1669 100755
> --- a/t/t7900-maintenance.sh
> +++ b/t/t7900-maintenance.sh
> @@ -1438,6 +1438,64 @@ test_expect_success '--no-detach causes maintenance to not run in background' '
>  	)
>  '
>
> +test_expect_success PIPE '--detach holds maintenance lock until daemonized child exits' '
> +	test_when_finished "rm -rf repo" &&
> +	git init repo &&
> +	(
> +		cd repo &&
> +
> +		git config maintenance.auto false &&
> +		git config core.lockfilepid true &&
> +
> +		git remote add origin /does/not/exist &&
> +		git config set remote.origin.uploadpack "cat fifo-uploadpack" &&
> +
> +		mkfifo fifo-uploadpack fifo-maint &&
> +
> +		# Open the maintenance FIFO, as otherwise spawning
> +		# git-maintenance(1) would block. Note that we need to open it
> +		# as read-write, as otherwise we would block here already.
> +		exec 9<>fifo-maint &&

Very nice. The fifo-maint idea is much more robust than what I was able
to come up with in my original sketch, and it is a nice compliment to
fifo-uploadpack.

The rest of the test is nearly identical to mine and looks good to me.

Thanks,
Taylor

[1]: https://lore.kernel.org/git/af+snTGFeoUUyfPU@nand.local/
[2]: https://lore.kernel.org/git/20260511200112.GA22912@coredump.intra.peff.net/
[3]: https://lore.kernel.org/git/20260511202258.GD22912@coredump.intra.peff.net/

Re: [PATCH v2 1/2] builtin/maintenance: fix locking with "--detach"

[Patrick Steinhardt]

On Tue, May 12, 2026 at 05:14:22PM -0400, Taylor Blau wrote:
> On Tue, May 12, 2026 at 10:30:30AM +0200, Patrick Steinhardt wrote:
> > diff --git a/builtin/gc.c b/builtin/gc.c
> > index 3a71e314c9..d866c19b92 100644
> > --- a/builtin/gc.c
> > +++ b/builtin/gc.c
> > @@ -1810,10 +1810,33 @@ static int maintenance_run_tasks(struct maintenance_run_opts *opts,
> >  				   TASK_PHASE_FOREGROUND))
> >  			result = 1;
> >
> > -	/* Failure to daemonize is ok, we'll continue in foreground. */
> >  	if (opts->detach > 0) {
> > +		pid_t child_pid;
> > +
> >  		trace2_region_enter("maintenance", "detach", the_repository);
> > -		daemonize();
> > +
> > +		child_pid = daemonize_without_exit();
> > +		if (!child_pid) {
> > +			/*
> > +			 * We're in the child process, so we take ownership of
> > +			 * the lockfile.
> > +			 */
> > +			lock_file_reassign_owner(&lk, getpid());
> > +		} else if (child_pid > 0) {
> > +			/*
> > +			 * We're in the parent process, so we drop ownership of
> > +			 * the lockfile to prevent us from removing it upon
> > +			 * exit.
> > +			 */
> > +			lock_file_reassign_owner(&lk, child_pid);
> > +			exit(0);
> > +		} else {
> > +			/*
> > +			 * Failure to daemonize is ok, we'll continue in
> > +			 * foreground.
> > +			 */
> > +		}
> > +
> 
> This works almost identically as the original implementation I shared[1]
> original thread discussing this issue, but it takes a slightly different
> approach.
> 
> Instead of doing this automatically as a part of daemonize(), this patch
> only adjusts this singular call-site, and forces us to introduce a new
> function daemonize_without_exit() in order to facilitate it.
> 
> I personally prefer the other approach, for many of the reasons that
> Peff wrote about in [2, 3]. There are two questions in my mind that I
> would be curious of your thoughts on:
> 
>  * Should we transfer ownership of all tempfiles, or a subset of them?
> 
>  * Should we perform that transfer automatically via daemonize(), or
>    manually via its callers?
> 
> Judging from the other parts of the original thread, I think the answer
> to the first question is an unambiguous "yes". The second question I
> think there is less of a consensus on, but I'd like to advocate for
> doing this automatically via daemonize().
> 
> To summarize some of my thoughts after reading [2], I think that because
> daemonize() is about letting a process continue on in the background
> rather than fork()-ing children and then continuing on ourselves, *not*
> automatically handing those off feels like an accident waiting to
> happen.
> 
> I can't think of a situation where someone would want to daemonize() but
> not have the new process assume ownership of its locks and tempfiles. I
> think an alternative approach here would be to document the behavior I'm
> proposing above clearly in daemonize(). Should a caller arise in the
> future that *doesn't* want to this behavior, then we could introduce a
> function similar to your daemonize_without_exit() (maybe in this case it
> would be daemonize_without_transfer(), since not exiting in a function
> that is supposed to daemonize feels awkward).
> 
> I guess what I'm saying is that I feel that future daemonize() callers
> are much more likely to want this behavior than not, and putting this in
> daemonize() feels like the safer option between the two.
> 
> FWIW, I feel fairly strongly about ^ this, but I'm of course happy to
> discuss more if you feel differently.
> 
> (If you do end up taking that approach, that makes the C changes
> identical to the ones I shared in [1], so you are free to forge my
> Co-authored-by and Signed-off-by lines if you want to take that approach
> instead.)

Hmm. I was initially aiming for a more minimal fix, and that's why I
decided to make only this one callsite reassign tempfiles. But there's
only two callsites right now, and thinking a bit more about the problem
makes me agree with your take on it.

Will adapt, thanks for pushing back!

Patrick

[PATCH v3 0/2] builtin/maintenance: fix locking and respect "gc.auto"

[Patrick Steinhardt]

Hi,

this patch series addresses the issues reported in [1]. The series is
built on top of Git 2.54.0.

Changes in v3:
  - Use Taylor's approach to reassign all tempfiles when daemonizing.
  - Link to v2: https://patch.msgid.link/20260512-pks-maintenance-fix-lock-with-detach-v2-0-dc6f2d284b6d@pks.im

Changes in v2:
  - Clarify comment when dropping ownership of the lock in the parent
    process.
  - Properly treat "gc.auto" as an integer, not a boolean.
  - Link to v1: https://patch.msgid.link/20260511-pks-maintenance-fix-lock-with-detach-v1-0-ccd7d62c9a40@pks.im

Thanks!

Patrick

[1]: <CAKcFC3arsYExb5dCMQspo4V9UFDadFaj8Q4PUsMWZJw_eYrMzA@mail.gmail.com>

---
Patrick Steinhardt (2):
      builtin/maintenance: fix locking with "--detach"
      run-command: honor "gc.auto" for auto-maintenance

 run-command.c          | 10 ++++--
 setup.c                | 16 +++++++++-
 setup.h                | 15 +++++++++
 t/t7900-maintenance.sh | 83 ++++++++++++++++++++++++++++++++++++++++++++++++++
 tempfile.c             | 12 ++++++++
 tempfile.h             | 11 +++++++
 6 files changed, 143 insertions(+), 4 deletions(-)

Range-diff versus v2:

1:  5eb608ad43 < -:  ---------- builtin/maintenance: fix locking with "--detach"
-:  ---------- > 1:  3fc8872f36 builtin/maintenance: fix locking with "--detach"
2:  15e2ae8104 = 2:  665a6c9a50 run-command: honor "gc.auto" for auto-maintenance

---
base-commit: 13ef77ce6e222bef3ab145642e6ef1486075211c
change-id: 20260511-pks-maintenance-fix-lock-with-detach-a608e9b6adeb

[PATCH v3 1/2] builtin/maintenance: fix locking with "--detach"

[Patrick Steinhardt]

When running git-maintenance(1), we create a lockfile that is supposed
to keep other maintenance processes from running at the same time. This
lockfile is broken though in case the "--detach" flag is passed: the
lockfile is created by the parent process and will be cleaned up either
manually or on exit. But when detaching, the parent will exit before all
of the background maintenance tasks have been run, and consequently the
lock only covers a smaller part of the whole maintenance process.

Fix this bug by reassigning all tempfiles from the parent process to the
child process when daemonizing so that it becomes the responsibility of
the child to clean them up.

Note that this is a broader fix, as we now always reassign tempfiles
when daemonizing. This is a natural consequence of the semantics of
`daemonize()` though, as it essentially promises to continue running the
current process in the background. It is thus sensible to have that
function perform the whole dance of assigning resources to the child
process, including tempfiles.

There's only a single other caller in "daemon.c", but that process
doesn't create any tempfiles before the call to `daemonize()` and is
thus not impacted by this change.

Reported-by: Jean-Christophe Manciot <actionmystique@gmail.com>
Helped-by: Jeff King <peff@peff.net>
Helped-by: Derrick Stolee <stolee@gmail.com>
Co-authored-by: Taylor Blau <me@ttaylorr.com>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 setup.c                | 16 +++++++++++++-
 setup.h                | 15 +++++++++++++
 t/t7900-maintenance.sh | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++
 tempfile.c             | 12 +++++++++++
 tempfile.h             | 11 ++++++++++
 5 files changed, 111 insertions(+), 1 deletion(-)

diff --git a/setup.c b/setup.c
index 7ec4427368..14445a71a4 100644
--- a/setup.c
+++ b/setup.c
@@ -2162,12 +2162,26 @@ int daemonize(void)
 	errno = ENOSYS;
 	return -1;
 #else
-	switch (fork()) {
+	pid_t parent_pid = getpid();
+	pid_t child_pid = fork();
+
+	switch (child_pid) {
 		case 0:
+			/*
+			 * We're in the child process, so we take ownership of
+			 * all tempfiles.
+			 */
+			reassign_tempfile_ownership(parent_pid, getpid());
 			break;
 		case -1:
 			die_errno(_("fork failed"));
 		default:
+			/*
+			 * We're in the parent process, so we drop ownership of
+			 * all tempfiles to prevent us from removing them upon
+			 * exit.
+			 */
+			reassign_tempfile_ownership(parent_pid, child_pid);
 			exit(0);
 	}
 	if (setsid() == -1)
diff --git a/setup.h b/setup.h
index 80bc6e5f07..b5bc5f280c 100644
--- a/setup.h
+++ b/setup.h
@@ -149,6 +149,21 @@ void verify_non_filename(const char *prefix, const char *name);
 int path_inside_repo(const char *prefix, const char *path);
 
 void sanitize_stdfds(void);
+
+/*
+ * Daemonize the current process by forking and then exiting the parent
+ * process. Returns 0 when successful, in which case the parent process will
+ * have exited and it's the child process that continues to run the code.
+ * Otherwise, a negative error code is returned and the parent process will
+ * continue execution.
+ *
+ * Note that this function will also perform the following changes:
+ *
+ *   - Standard file descriptors in the child process are closed.
+ *   - The child process is made a session leader via setsid(3p).
+ *   - All tempfiles owned by the parent process are reassigned to the
+ *     daemonized child process.
+ */
 int daemonize(void);
 
 /*
diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
index 4700beacc1..df0bbc1669 100755
--- a/t/t7900-maintenance.sh
+++ b/t/t7900-maintenance.sh
@@ -1438,6 +1438,64 @@ test_expect_success '--no-detach causes maintenance to not run in background' '
 	)
 '
 
+test_expect_success PIPE '--detach holds maintenance lock until daemonized child exits' '
+	test_when_finished "rm -rf repo" &&
+	git init repo &&
+	(
+		cd repo &&
+
+		git config maintenance.auto false &&
+		git config core.lockfilepid true &&
+
+		git remote add origin /does/not/exist &&
+		git config set remote.origin.uploadpack "cat fifo-uploadpack" &&
+
+		mkfifo fifo-uploadpack fifo-maint &&
+
+		# Open the maintenance FIFO, as otherwise spawning
+		# git-maintenance(1) would block. Note that we need to open it
+		# as read-write, as otherwise we would block here already.
+		exec 9<>fifo-maint &&
+
+		{ git maintenance run --task=prefetch --detach 7>&9 & } &&
+		parent="$!" &&
+
+		# Reap the parent process so that the exec call below will not
+		# get SIGCHLD.
+		wait "$parent" &&
+
+		# Open the git-upload-pack(1) FIFO for writing, which will
+		# block until the upload-pack script opens it for reading. Once
+		# exec returns, we know that the daemonized child is alive and
+		# pinned.
+		exec 8>fifo-uploadpack &&
+
+		test_path_is_file .git/objects/maintenance.lock &&
+		test_path_is_file .git/objects/"maintenance~pid.lock" &&
+
+		# Verify that the maintenance.lock still exists, and
+		# that it was created by the parent process, not the
+		# child.
+		echo "pid $parent" >expect &&
+		test_cmp expect .git/objects/"maintenance~pid.lock" &&
+
+		# Reopen the maintenance FIFO as read-only so that
+		# git-maintenance(1) is the only writer. This will cause it to
+		# close the FIFO once the process exits.
+		exec 9<&- &&
+		exec 9<fifo-maint &&
+
+		# Close the FIFO used by git-upload-pack(1) to unblock it and
+		# then wait until the maintenance FIFO is closed by
+		# git-maintenance(1), indicating that it has exited.
+		exec 8>&- &&
+		cat <&9 &&
+
+		test_path_is_missing .git/objects/maintenance.lock &&
+		test_path_is_missing .git/objects/"maintenance~pid.lock"
+	)
+'
+
 test_expect_success '--detach causes maintenance to run in background' '
 	test_when_finished "rm -rf repo" &&
 	git init repo &&
diff --git a/tempfile.c b/tempfile.c
index 82dfa3d82f..f0fdf58279 100644
--- a/tempfile.c
+++ b/tempfile.c
@@ -373,3 +373,15 @@ int delete_tempfile(struct tempfile **tempfile_p)
 
 	return err ? -1 : 0;
 }
+
+void reassign_tempfile_ownership(pid_t from, pid_t to)
+{
+	volatile struct volatile_list_head *pos;
+
+	list_for_each(pos, &tempfile_list) {
+		struct tempfile *p = list_entry(pos, struct tempfile, list);
+
+		if (is_tempfile_active(p) && p->owner == from)
+			p->owner = to;
+	}
+}
diff --git a/tempfile.h b/tempfile.h
index 2d2ae5b657..2227a095fd 100644
--- a/tempfile.h
+++ b/tempfile.h
@@ -282,4 +282,15 @@ int delete_tempfile(struct tempfile **tempfile_p);
  */
 int rename_tempfile(struct tempfile **tempfile_p, const char *path);
 
+/*
+ * Reassign ownership of all active tempfiles whose `owner` field matches
+ * `from` to `to`.
+ *
+ * This is intended for use by `daemonize()`; after `fork(2)`-ing, the parent
+ * transfers ownership to the daemonized child so that its atexit handler does
+ * not unlink tempfiles that should outlive it, and the child claims the
+ * inherited tempfiles so that they are cleaned up when the daemon exits.
+ */
+void reassign_tempfile_ownership(pid_t from, pid_t to);
+
 #endif /* TEMPFILE_H */

-- 
2.54.0.709.gd731d7959a.dirty

[PATCH v3 2/2] run-command: honor "gc.auto" for auto-maintenance

[Patrick Steinhardt]

The "gc.auto" configuration has traditionally been used to turn off
running git-gc(1) as part of our auto-maintenance. We have eventually
switched over to git-maintenance(1) in a95ce12430 (maintenance: replace
run_auto_gc(), 2020-09-17), and with 1942d48380 (maintenance: optionally
skip --auto process, 2020-08-28) we have introduced "maintenance.auto"
to control whether or not to run auto-maintenance.

At that point though we still shelled out to git-gc(1) internally. So
if "gc.auto=0" was set we would still _execute_ git-maintenance(1), but
the command would have exited fast because git-gc(1) itself knew to
honor the config key.

This has recently changed though, as we have adapted the default
maintenance strategy to not use git-gc(1) anymore. The consequence is
that "gc.auto=0" doesn't have an effect anymore, which is a somewhat
surprising change in behaviour for our users.

Adapt `run_auto_maintenance()` so that it knows to also read "gc.auto",
similar to how it also reads both "maintenance.autoDetach" and
"gc.autoDetach".

Reported-by: Jean-Christophe Manciot <actionmystique@gmail.com>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 run-command.c          | 10 +++++++---
 t/t7900-maintenance.sh | 25 +++++++++++++++++++++++++
 2 files changed, 32 insertions(+), 3 deletions(-)

diff --git a/run-command.c b/run-command.c
index c146a56532..28202a81d8 100644
--- a/run-command.c
+++ b/run-command.c
@@ -1944,10 +1944,14 @@ void run_processes_parallel(const struct run_process_parallel_opts *opts)
 int prepare_auto_maintenance(struct repository *r, int quiet,
 			     struct child_process *maint)
 {
-	int enabled, auto_detach;
+	int enabled = 1, auto_detach;
 
-	if (!repo_config_get_bool(r, "maintenance.auto", &enabled) &&
-	    !enabled)
+	if (repo_config_get_bool(r, "maintenance.auto", &enabled)) {
+		int gc_threshold;
+		if (!repo_config_get_int(r, "gc.auto", &gc_threshold))
+			enabled = gc_threshold > 0;
+	}
+	if (!enabled)
 		return 0;
 
 	/*
diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
index df0bbc1669..97c8c701bb 100755
--- a/t/t7900-maintenance.sh
+++ b/t/t7900-maintenance.sh
@@ -73,6 +73,31 @@ test_expect_success 'maintenance.auto config option' '
 	test_subcommand ! git maintenance run --auto --quiet --detach <false
 '
 
+test_expect_success 'gc.auto config option' '
+	GIT_TRACE2_EVENT="$(pwd)/default" git commit --quiet --allow-empty -m 1 &&
+	test_subcommand git maintenance run --auto --quiet --detach <default &&
+	GIT_TRACE2_EVENT="$(pwd)/true" \
+		git -c gc.auto=1 commit --quiet --allow-empty -m 2 &&
+	test_subcommand git maintenance run --auto --quiet --detach <true &&
+	GIT_TRACE2_EVENT="$(pwd)/false" \
+		git -c gc.auto=0 commit --quiet --allow-empty -m 3 &&
+	test_subcommand ! git maintenance run --auto --quiet --detach <false
+'
+
+test_expect_success 'maintenance.auto overrides gc.auto' '
+	test_when_finished "rm -f trace" &&
+
+	test_config maintenance.auto false &&
+	test_config gc.auto 1 &&
+	GIT_TRACE2_EVENT="$(pwd)/trace" git commit --quiet --allow-empty -m 1 &&
+	test_subcommand ! git maintenance run --auto --quiet --detach <trace &&
+
+	test_config maintenance.auto true &&
+	test_config gc.auto 0 &&
+	GIT_TRACE2_EVENT="$(pwd)/trace" git commit --quiet --allow-empty -m 1 &&
+	test_subcommand git maintenance run --auto --quiet --detach <trace
+'
+
 for cfg in maintenance.autoDetach gc.autoDetach
 do
 	test_expect_success "$cfg=true config option" '

-- 
2.54.0.709.gd731d7959a.dirty

Re: [PATCH v3 1/2] builtin/maintenance: fix locking with "--detach"

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> Note that this is a broader fix, as we now always reassign tempfiles
> when daemonizing. This is a natural consequence of the semantics of
> `daemonize()` though, as it essentially promises to continue running the
> current process in the background.

Exactly.  I do agree that it is the right wy to look at it.  The
process that daemonise creates and leaves in the background is
logically the process that continues to execute the service the
process the user started, and unless the original process explicitly
says "we are done serving this thing" and cleans up tempfile or
lockfile it needed to serve that thing, it is natural to make the
surviving process to take over the responsibility.