patch, 11 partsRe: [PATCH 11/11] builtin/maintenance: fix locking race when handling "gc" task

> Le 27 mai 2025 à 11:29, Patrick Steinhardt <ps@pks.im> 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 <ps@pks.im>
> ---
> 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");

> 
>   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 <run-no-auto.txt &&
> -    test_subcommand ! git gc --auto --quiet --no-detach <run-auto.txt &&
> -    test_subcommand git gc --no-quiet --no-detach <run-no-quiet.txt
> +    test_subcommand git gc --quiet --no-detach --skip-maintenance-before-detach <run-no-auto.txt &&
> +    test_subcommand ! git gc --auto --quiet --no-detach --skip-maintenance-before-detach <run-auto.txt &&
> +    test_subcommand git gc --no-quiet --no-detach --skip-maintenance-before-detach <run-no-quiet.txt
> '
> 
> test_expect_success 'maintenance.auto config option' '
> @@ -154,9 +154,9 @@ test_expect_success 'run --task=<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 <run-commit-graph.txt &&
> -    test_subcommand git gc --quiet --no-detach <run-gc.txt &&
> -    test_subcommand git gc --quiet --no-detach <run-both.txt &&
> +    test_subcommand ! git gc --quiet --no-detach --skip-maintenance-before-detach <run-commit-graph.txt &&
> +    test_subcommand git gc --quiet --no-detach --skip-maintenance-before-detach <run-gc.txt &&
> +    test_subcommand git gc --quiet --no-detach --skip-maintenance-before-detach <run-both.txt &&
>   test_subcommand git commit-graph write --split --reachable --no-progress <run-commit-graph.txt &&
>   test_subcommand ! git commit-graph write --split --reachable --no-progress <run-gc.txt &&
>   test_subcommand git commit-graph write --split --reachable --no-progress <run-both.txt
> 
> --
> 2.49.0.1266.g31b7d2e469.dirty

> > @@ -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)?

> Le 30 mai 2025 à 10:05, Patrick Steinhardt <ps@pks.im> 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

> 
> > Le 30 mai 2025 à 10:05, Patrick Steinhardt <ps@pks.im> 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.

> Le 30 mai 2025 à 11:31, Patrick Steinhardt <ps@pks.im> a écrit :
> 
> On Fri, May 30, 2025 at 11:10:27AM -0400, Ben Knoble wrote:
>> 
>>>> Le 30 mai 2025 à 10:05, Patrick Steinhardt <ps@pks.im> 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.