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

From: Patrick Steinhardt <ps@pks.im>
Date: May 30, 2025, 14:05 UTC
Message-ID: <aDm7HcdCbam6wgbA@pks.im>
In-Reply-To: <819EFBF4-C435-4213-91C7-BF15B49B1B09@gmail.com>

On Fri, May 30, 2025 at 08:55:49AM -0400, Ben Knoble wrote:

Show 10 quoted lines

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