[PATCH v2 12/12] builtin/maintenance: fix locking race when handling "gc" task
- From
Patrick Steinhardt <ps@pks.im>
- Date
- May 30, 2025, 15:08 UTC
- Message-ID
- <20250530-b4-pks-maintenance-ref-lock-race-v2-12-d04e2f93e51f@pks.im>
- In-Reply-To
- <20250530-b4-pks-maintenance-ref-lock-race-v2-0-d04e2f93e51f@pks.im>
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 e910a99e033..dfbf20491e3 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) - die(NULL); - delete_tempfile(&pidfile); + if (gc_before_detach(&opts, &cfg) < 0) + die(NULL); + 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.50.0.rc0.604.gd4ff7b7c86.dirty