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