Re: unexpected auto-maintenance, was Re: git hogs the CPU, RAM and storage despite its config

[Patrick Steinhardt <ps@pks.im>]

On Sun, May 10, 2026 at 04:00:08PM -0400, Taylor Blau wrote:
> On Sun, May 10, 2026 at 12:08:14PM -0400, Derrick Stolee wrote:
> > > That's not too terrible to write, and I would feel OK about putting it
> > > in a 2.54.1 release soon-ish provided that others think it is reasonable.
> >
> > I'd rather revert the geometric maintenance default for a point
> > release and let something more complicated like this percolate
> > until the next release window.
> >
> > > Simply reverting 452b12c2e0 (builtin/maintenance: use "geometric"
> > > strategy by default, 2026-02-24) feels somewhat unsatisfying, since it
> > > is merely making the bug less likely rather than eliminating it
> > > entirely.
> >
> > It does limit the behavior to those who previously chose to opt-in to
> > this behavior, and likely that's a low number or else we'd have more
> > bug reports.
> 
> I agree with what you're saying. In an ideal world we would be able to
> apply a fix on top that would prevent this bug from occurring, but if
> that's not trivial to do then we shouldn't rush it in the 2.54.1 window.
> 
> I think that not having this bug whatsoever (as opposed to simply
> reverting 452b12c2e0) would be preferable, but as you note we haven't
> seen any bug reports in a pre-452b12c2e0 world, so perhaps the risk is
> low enough that we could merely revert 452b12c2e0.

I think having an actual fix for the locking logic in git-maintenance(1)
would be preferable though, as the issue isn't merely contained to Git
2.54. With that release we are of course much more likely to hit the
issue as the "geometric" strategy is the default now. But even before
this release it was possible to configure that strategy, and if the user
had opted in to it they might've hit the same bug.

> > For me to be convinced that this forward fix is the right direction,
> > I'd need to see a test that proves the detached process will clean up
> > the locks on a normal process end and an early exit.
> 
> Yeah, I agree. Testing this is a little tricky, but I played around
> with it for a while and came up with the following:
> 
> --- 8< ---
> diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
> index 4700beacc18..77bfa537385 100755
> --- a/t/t7900-maintenance.sh
> +++ b/t/t7900-maintenance.sh
> @@ -1460,6 +1460,56 @@ test_expect_success '--detach causes maintenance to 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 &&
> +
> +		mkfifo fifo &&
> +
> +		git config maintenance.auto false &&
> +		git config core.lockfilepid true &&
> +
> +		git remote add origin /does/not/exist &&
> +		git config set remote.origin.uploadpack \
> +			"echo \$PPID >child && cat \"$(pwd)/fifo\"" &&
> +
> +		# Start a detached prefetch maintenance task. Note that
> +		# we are backgrounding git-maintenance here in order to
> +		# determine its PID to validate that the lockfile was
> +		# created by the parent.
> +		{ git maintenance run --task=prefetch --detach & } &&
> +		parent="$!" &&
> +
> +		# Open fifo for writing, which will block until the
> +		# upload-pack helper opens it for reading. Once exec
> +		# returns, we know that the daemonized child is alive
> +		# and pinned.
> +		exec 8>fifo &&
> +
> +		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 &&
> +
> +		# Close the write end of the FIFO, causing our upload-pack
> +		# helper to quit. Wait until the grandparent (from the
> +		# perspective of our upload-pack helper, the daemonized
> +		# git-maintenance child)
> +		exec 8>&- &&
> +		gpid="$(ps -o ppid= -p $(cat child) | tr -d " ")" &&
> +		test -n $gpid && wait $gpid &&
> +
> +		test_path_is_missing .git/objects/maintenance.lock &&
> +		test_path_is_missing .git/objects/maintenance~pid.lock
> +	)
> +'
> +
>  test_expect_success 'repacking loose objects is quiet' '
>  	test_when_finished "rm -rf repo" &&
>  	git init repo &&
> --- >8 ---
> 
> I'm not sure how portable it is, though. I think that 'ps -o ppid=' is
> OK, since 'start_git_in_background()' uses it, but I'm not sure if $PPID
> is available on Windows.

Yeah, this test seems to work on my system, too. But it's another
Windows machine indeed. I'll start a CI pipeline to check whether it
works on Windows, as well.

In any case, I also agree with Stolee that it may have unintended
consequences to unconditionally reassign tempfiles to the child process
when daemonizing. It is okay for all current callers, but I'm not sure
whether future callers would expect this behaviour.

An alternative would be to explicitly reassign the lockfile's ownership
in git-maintenance(1). Something like the below patch.

Thanks!

Patrick

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..d651e11d4b 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 error_errno(_("fork failed"));
+	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(NULL);
+	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..10f8b377f2 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);
+int daemonize_without_exit(void);
 
 /*
  * GIT_REPO_VERSION is the version we write by default. The