# [PATCH v2 1/2] builtin/maintenance: fix locking with "--detach" 5 messages from 2026-05-12 to 2026-05-13. Participants: Patrick Steinhardt, Taylor Blau. Thread: https://gitlist.dev/t/65621 ## Patrick Steinhardt, 2026-05-12 08:30 Subject: [PATCH v2 0/2] builtin/maintenance: fix locking and respect "gc.auto" Message-ID: <20260512-pks-maintenance-fix-lock-with-detach-v2-0-dc6f2d284b6d@pks.im> In-Reply-To: <20260511-pks-maintenance-fix-lock-with-detach-v1-0-ccd7d62c9a40@pks.im> ``` 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]: --- 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 ``` ## Patrick Steinhardt, 2026-05-12 08:30 Subject: [PATCH v2 1/2] builtin/maintenance: fix locking with "--detach" Message-ID: <20260512-pks-maintenance-fix-lock-with-detach-v2-1-dc6f2d284b6d@pks.im> In-Reply-To: <20260512-pks-maintenance-fix-lock-with-detach-v2-0-dc6f2d284b6d@pks.im> ``` 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 Helped-by: Jeff King Helped-by: Taylor Blau Helped-by: Derrick Stolee Signed-off-by: Patrick Steinhardt --- 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&- && + 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 ``` ## Patrick Steinhardt, 2026-05-12 08:30 Subject: [PATCH v2 2/2] run-command: honor "gc.auto" for auto-maintenance Message-ID: <20260512-pks-maintenance-fix-lock-with-detach-v2-2-dc6f2d284b6d@pks.im> In-Reply-To: <20260512-pks-maintenance-fix-lock-with-detach-v2-0-dc6f2d284b6d@pks.im> ``` 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 Signed-off-by: Patrick Steinhardt --- 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 In-Reply-To: <20260512-pks-maintenance-fix-lock-with-detach-v2-1-dc6f2d284b6d@pks.im> ``` 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/ ``` ## Patrick Steinhardt, 2026-05-13 06:23 Subject: Re: [PATCH v2 1/2] builtin/maintenance: fix locking with "--detach" Message-ID: In-Reply-To: ``` 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 ```