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

[Patrick Steinhardt <ps@pks.im>]

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