# Re: [PATCH 11/11] builtin/maintenance: fix locking race when handling "gc" task 5 messages from 2025-05-30 to 2025-05-30. Participants: Ben Knoble, Patrick Steinhardt. Thread: https://gitlist.dev/t/63550 ## Ben Knoble, 2025-05-30 12:55 Subject: Re: [PATCH 11/11] builtin/maintenance: fix locking race when handling "gc" task Message-ID: <819EFBF4-C435-4213-91C7-BF15B49B1B09@gmail.com> URL: https://gitlist.dev/e/819EFBF4-C435-4213-91C7-BF15B49B1B09%40gmail.com ```  > Le 27 mai 2025 à 11:29, Patrick Steinhardt a écrit : > > The "gc" task has a similar locking race as the one that we have fixed > for the "pack-refs" and "reflog-expire" tasks in preceding commits. Fix > this by splitting up the logic of the "gc" task: > > - Before detaching we execute `gc_before_repack()`, which contains the > logic that git-gc(1) itself would execute before detaching. > > - After detaching we spawn git-gc(1), but with a new hidden flag that > suppresses calling `gc_before_repack()`. > > Like this we have roughly the same logic as git-gc(1) itself and know to > repack refs and reflogs before detaching, thus fixing the race. > > Note that `gc_before_repack()` is renamed to `gc_before_detach()` to > better reflect what this function does. > > Signed-off-by: Patrick Steinhardt > --- > builtin/gc.c | 39 ++++++++++++++++++++++++++------------- > t/t7900-maintenance.sh | 12 ++++++------ > 2 files changed, 32 insertions(+), 19 deletions(-) > > diff --git a/builtin/gc.c b/builtin/gc.c > index 174357b9c25..2cf61efcee9 100644 > --- a/builtin/gc.c > +++ b/builtin/gc.c > @@ -816,7 +816,7 @@ static int report_last_gc_error(void) > return ret; > } > > -static int gc_before_repack(struct maintenance_run_opts *opts, > +static int gc_before_detach(struct maintenance_run_opts *opts, > struct gc_config *cfg) > { > if (cfg->pack_refs && maintenance_task_pack_refs(opts, cfg)) > @@ -837,6 +837,7 @@ int cmd_gc(int argc, > pid_t pid; > int daemonized = 0; > int keep_largest_pack = -1; > + int skip_maintenance_before_detach = 0; > timestamp_t dummy; > struct maintenance_run_opts opts = MAINTENANCE_RUN_OPTS_INIT; > struct gc_config cfg = GC_CONFIG_INIT; > @@ -869,6 +870,8 @@ int cmd_gc(int argc, > N_("repack all other packs except the largest pack")), > OPT_STRING(0, "expire-to", &cfg.repack_expire_to, N_("dir"), > N_("pack prefix to store a pack containing pruned objects")), > + OPT_HIDDEN_BOOL(0, "skip-maintenance-before-detach", &skip_maintenance_before_detach, > + N_("skip maintenance steps typically done before detaching")), > OPT_END() > }; > > @@ -952,14 +955,16 @@ int cmd_gc(int argc, > goto out; > } > > - if (lock_repo_for_gc(force, &pid)) { > - ret = 0; > - goto out; > - } > + if (!skip_maintenance_before_detach) { > + if (lock_repo_for_gc(force, &pid)) { > + ret = 0; > + goto out; > + } > > - if (gc_before_repack(&opts, &cfg) < 0) > - exit(127); > - delete_tempfile(&pidfile); > + if (gc_before_detach(&opts, &cfg) < 0) > + exit(127); > + delete_tempfile(&pidfile); > + } > > /* > * failure to daemonize is ok, we'll continue > @@ -988,8 +993,8 @@ int cmd_gc(int argc, > free(path); > } > > - if (opts.detach <= 0) > - gc_before_repack(&opts, &cfg); > + if (opts.detach <= 0 && !skip_maintenance_before_detach) > + gc_before_detach(&opts, &cfg); > > if (!repository_format_precious_objects) { > struct child_process repack_cmd = CHILD_PROCESS_INIT; > @@ -1225,8 +1230,14 @@ static int maintenance_task_prefetch(struct maintenance_run_opts *opts, > return 0; > } > > -static int maintenance_task_gc(struct maintenance_run_opts *opts, > - struct gc_config *cfg UNUSED) > +static int maintenance_task_gc_before_detach(struct maintenance_run_opts *opts, > + struct gc_config *cfg) > +{ > + return gc_before_detach(opts, cfg); > +} > + > +static int maintenance_task_gc_after_detach(struct maintenance_run_opts *opts, > + struct gc_config *cfg UNUSED) > { > struct child_process child = CHILD_PROCESS_INIT; > > @@ -1240,6 +1251,7 @@ static int maintenance_task_gc(struct maintenance_run_opts *opts, > else > strvec_push(&child.args, "--no-quiet"); > strvec_push(&child.args, "--no-detach"); > + strvec_push(&child.args, "--skip-maintenance-before-detach"); I suspect this would be more obvious to me if I had the manual available right now, but if we are not detaching (« --no-detach ») why do we need to skip something before detaching (that presumably won’t happen)? > > return run_command(&child); > } > @@ -1561,7 +1573,8 @@ static const struct maintenance_task tasks[] = { > }, > [TASK_GC] = { > .name = "gc", > - .after_detach = maintenance_task_gc, > + .before_detach = maintenance_task_gc_before_detach, > + .after_detach = maintenance_task_gc_after_detach, > .auto_condition = need_to_gc, > }, > [TASK_COMMIT_GRAPH] = { > diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh > index 1ada5246606..e09a36ab021 100755 > --- a/t/t7900-maintenance.sh > +++ b/t/t7900-maintenance.sh > @@ -49,9 +49,9 @@ test_expect_success 'run [--auto|--quiet]' ' > git maintenance run --auto 2>/dev/null && > GIT_TRACE2_EVENT="$(pwd)/run-no-quiet.txt" \ > git maintenance run --no-quiet 2>/dev/null && > - test_subcommand git gc --quiet --no-detach - test_subcommand ! git gc --auto --quiet --no-detach - test_subcommand git gc --no-quiet --no-detach + test_subcommand git gc --quiet --no-detach --skip-maintenance-before-detach + test_subcommand ! git gc --auto --quiet --no-detach --skip-maintenance-before-detach + test_subcommand git gc --no-quiet --no-detach --skip-maintenance-before-detach ' > > test_expect_success 'maintenance.auto config option' ' > @@ -154,9 +154,9 @@ test_expect_success 'run --task=' ' > git maintenance run --task=commit-graph 2>/dev/null && > GIT_TRACE2_EVENT="$(pwd)/run-both.txt" \ > git maintenance run --task=commit-graph --task=gc 2>/dev/null && > - test_subcommand ! git gc --quiet --no-detach - test_subcommand git gc --quiet --no-detach - test_subcommand git gc --quiet --no-detach + test_subcommand ! git gc --quiet --no-detach --skip-maintenance-before-detach + test_subcommand git gc --quiet --no-detach --skip-maintenance-before-detach + test_subcommand git gc --quiet --no-detach --skip-maintenance-before-detach test_subcommand git commit-graph write --split --reachable --no-progress test_subcommand ! git commit-graph write --split --reachable --no-progress test_subcommand git commit-graph write --split --reachable --no-progress > -- > 2.49.0.1266.g31b7d2e469.dirty ``` ## Patrick Steinhardt, 2025-05-30 14:05 Subject: Re: [PATCH 11/11] builtin/maintenance: fix locking race when handling "gc" task Message-ID: URL: https://gitlist.dev/e/aDm7HcdCbam6wgbA%40pks.im In-Reply-To: <819EFBF4-C435-4213-91C7-BF15B49B1B09@gmail.com> ``` On Fri, May 30, 2025 at 08:55:49AM -0400, Ben Knoble wrote: > > @@ -1240,6 +1251,7 @@ static int maintenance_task_gc(struct maintenance_run_opts *opts, > > else > > strvec_push(&child.args, "--no-quiet"); > > strvec_push(&child.args, "--no-detach"); > > + strvec_push(&child.args, "--skip-maintenance-before-detach"); > > I suspect this would be more obvious to me if I had the manual > available right now, but if we are not detaching (« --no-detach ») why > do we need to skip something before detaching (that presumably won’t > happen)? We have two levels here: git-maintenance(1) and git-gc(1), where the former executes the latter when the "gc" task is configured. What is important to realize is that in this setup it is not git-gc(1) which detaches -- it is git-maintenance(1). So git-maintenance(1) runs in the background, but any tasks it invokes itself must run synchronously in the foreground. The flow thus looks like this: 1. git-maintenance(1) starts. 2. We perform the pre-detach tasks from git-gc(1) in the same process. 3. We detach and thus the main process exits. 4. We execute git-gc(1) in the already-detached process. 5. We wait for git-gc(1) to exit. 6. The detached git-maintenance(1) exits. So because (4) is running in the already-detached process we ask git-gc(1) to not detach again. And because we already ran the pre-detach tasks we also ask it to not run those again. Patrick ``` ## Ben Knoble, 2025-05-30 15:10 Subject: Re: [PATCH 11/11] builtin/maintenance: fix locking race when handling "gc" task Message-ID: <0D09F80A-B6FA-40E7-AB2B-A6F78F2D58E7@gmail.com> URL: https://gitlist.dev/e/0D09F80A-B6FA-40E7-AB2B-A6F78F2D58E7%40gmail.com In-Reply-To: ``` > Le 30 mai 2025 à 10:05, Patrick Steinhardt a écrit : > > On Fri, May 30, 2025 at 08:55:49AM -0400, Ben Knoble wrote: >>> @@ -1240,6 +1251,7 @@ static int maintenance_task_gc(struct maintenance_run_opts *opts, >>> else >>> strvec_push(&child.args, "--no-quiet"); >>> strvec_push(&child.args, "--no-detach"); >>> + strvec_push(&child.args, "--skip-maintenance-before-detach"); >> >> I suspect this would be more obvious to me if I had the manual >> available right now, but if we are not detaching (« --no-detach ») why >> do we need to skip something before detaching (that presumably won’t >> happen)? > > We have two levels here: git-maintenance(1) and git-gc(1), where the > former executes the latter when the "gc" task is configured. What is > important to realize is that in this setup it is not git-gc(1) which > detaches -- it is git-maintenance(1). So git-maintenance(1) runs in the > background, but any tasks it invokes itself must run synchronously in > the foreground. > > The flow thus looks like this: > > 1. git-maintenance(1) starts. > 2. We perform the pre-detach tasks from git-gc(1) in the same process. > 3. We detach and thus the main process exits. > 4. We execute git-gc(1) in the already-detached process. > 5. We wait for git-gc(1) to exit. > 6. The detached git-maintenance(1) exits. > > So because (4) is running in the already-detached process we ask > git-gc(1) to not detach again. And because we already ran the pre-detach > tasks we also ask it to not run those again. > > Patrick Aha, thanks! I thought I understood the sequence, but I was wrong about some details. I was wondering if not detaching should just imply skipping work before a (non-existent) detach—if there’s no detach, should we do any pre-detach work at all? But presumably that does the wrong thing for (non-detaching) invocations that come from outside git-maintenance, doesn’t it? Hm. Maybe the flip-around for me is that « pre-detach work » here actually refers to « foreground work », which we obviously want to do even if we aren’t detaching, and which maintenance (which has already done this) needs to skip. ``` ## Patrick Steinhardt, 2025-05-30 15:24 Subject: Re: [PATCH 11/11] builtin/maintenance: fix locking race when handling "gc" task Message-ID: URL: https://gitlist.dev/e/aDnNuGvargGZtubY%40pks.im In-Reply-To: <0D09F80A-B6FA-40E7-AB2B-A6F78F2D58E7@gmail.com> ``` On Fri, May 30, 2025 at 11:10:27AM -0400, Ben Knoble wrote: > > > Le 30 mai 2025 à 10:05, Patrick Steinhardt a écrit : > > > > On Fri, May 30, 2025 at 08:55:49AM -0400, Ben Knoble wrote: > >>> @@ -1240,6 +1251,7 @@ static int maintenance_task_gc(struct maintenance_run_opts *opts, > >>> else > >>> strvec_push(&child.args, "--no-quiet"); > >>> strvec_push(&child.args, "--no-detach"); > >>> + strvec_push(&child.args, "--skip-maintenance-before-detach"); > >> > >> I suspect this would be more obvious to me if I had the manual > >> available right now, but if we are not detaching (« --no-detach ») why > >> do we need to skip something before detaching (that presumably won’t > >> happen)? > > > > We have two levels here: git-maintenance(1) and git-gc(1), where the > > former executes the latter when the "gc" task is configured. What is > > important to realize is that in this setup it is not git-gc(1) which > > detaches -- it is git-maintenance(1). So git-maintenance(1) runs in the > > background, but any tasks it invokes itself must run synchronously in > > the foreground. > > > > The flow thus looks like this: > > > > 1. git-maintenance(1) starts. > > 2. We perform the pre-detach tasks from git-gc(1) in the same process. > > 3. We detach and thus the main process exits. > > 4. We execute git-gc(1) in the already-detached process. > > 5. We wait for git-gc(1) to exit. > > 6. The detached git-maintenance(1) exits. > > > > So because (4) is running in the already-detached process we ask > > git-gc(1) to not detach again. And because we already ran the pre-detach > > tasks we also ask it to not run those again. > > > > Patrick > > Aha, thanks! I thought I understood the sequence, but I was wrong > about some details. > > I was wondering if not detaching should just imply skipping work > before a (non-existent) detach—if there’s no detach, should we do any > pre-detach work at all? But presumably that does the wrong thing for > (non-detaching) invocations that come from outside git-maintenance, > doesn’t it? Hm. Yeah, we always want to do these tasks no matter whether we detach or not. > Maybe the flip-around for me is that « pre-detach work » here actually > refers to « foreground work », which we obviously want to do even if > we aren’t detaching, and which maintenance (which has already done > this) needs to skip. Hm. That's actually a better way to put it, agreed. Too bad I already sent out the new version a couple minutes ago :) I'll have a look on Monday and rephrase this part. Patrick ``` ## Ben Knoble, 2025-05-30 20:46 Subject: Re: [PATCH 11/11] builtin/maintenance: fix locking race when handling "gc" task Message-ID: <06B496C9-BC0E-479E-A5ED-BC00004E2368@gmail.com> URL: https://gitlist.dev/e/06B496C9-BC0E-479E-A5ED-BC00004E2368%40gmail.com In-Reply-To: ``` > Le 30 mai 2025 à 11:31, Patrick Steinhardt a écrit : > > On Fri, May 30, 2025 at 11:10:27AM -0400, Ben Knoble wrote: >> >>>> Le 30 mai 2025 à 10:05, Patrick Steinhardt a écrit : >>> >>> On Fri, May 30, 2025 at 08:55:49AM -0400, Ben Knoble wrote: >>>>> @@ -1240,6 +1251,7 @@ static int maintenance_task_gc(struct maintenance_run_opts *opts, >>>>> else >>>>> strvec_push(&child.args, "--no-quiet"); >>>>> strvec_push(&child.args, "--no-detach"); >>>>> + strvec_push(&child.args, "--skip-maintenance-before-detach"); >>>> >>>> I suspect this would be more obvious to me if I had the manual >>>> available right now, but if we are not detaching (« --no-detach ») why >>>> do we need to skip something before detaching (that presumably won’t >>>> happen)? >>> >>> We have two levels here: git-maintenance(1) and git-gc(1), where the >>> former executes the latter when the "gc" task is configured. What is >>> important to realize is that in this setup it is not git-gc(1) which >>> detaches -- it is git-maintenance(1). So git-maintenance(1) runs in the >>> background, but any tasks it invokes itself must run synchronously in >>> the foreground. >>> >>> The flow thus looks like this: >>> >>> 1. git-maintenance(1) starts. >>> 2. We perform the pre-detach tasks from git-gc(1) in the same process. >>> 3. We detach and thus the main process exits. >>> 4. We execute git-gc(1) in the already-detached process. >>> 5. We wait for git-gc(1) to exit. >>> 6. The detached git-maintenance(1) exits. >>> >>> So because (4) is running in the already-detached process we ask >>> git-gc(1) to not detach again. And because we already ran the pre-detach >>> tasks we also ask it to not run those again. >>> >>> Patrick >> >> Aha, thanks! I thought I understood the sequence, but I was wrong >> about some details. >> >> I was wondering if not detaching should just imply skipping work >> before a (non-existent) detach—if there’s no detach, should we do any >> pre-detach work at all? But presumably that does the wrong thing for >> (non-detaching) invocations that come from outside git-maintenance, >> doesn’t it? Hm. > > Yeah, we always want to do these tasks no matter whether we detach or > not. > >> Maybe the flip-around for me is that « pre-detach work » here actually >> refers to « foreground work », which we obviously want to do even if >> we aren’t detaching, and which maintenance (which has already done >> this) needs to skip. > > Hm. That's actually a better way to put it, agreed. Too bad I already > sent out the new version a couple minutes ago :) I'll have a look on > Monday and rephrase this part. Aha, no worries :) thanks for taking the time. ```