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

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

[Patrick Steinhardt]

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

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

[Ben Knoble]



> 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");

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 <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

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

[Patrick Steinhardt]

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

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

[Ben Knoble]

> 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.

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.

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

[Patrick Steinhardt]

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.

Patrick

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

[Ben Knoble]

> 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.

Aha, no worries :) thanks for taking the time.