Re: [PATCH] receive-pack: fix stale packfile locks when dying

From: Patrick Steinhardt <ps@pks.im>
Date: Mar 10, 2023, 06:24 UTC
Message-ID: <ZArNJHMmPd8QCLwQ@ncase>
In-Reply-To: <ZAoCUPsHbstSJ0+B@coredump.intra.peff.net>

On Thu, Mar 09, 2023 at 10:59:12AM -0500, Jeff King wrote:

Show 31 quoted lines

> On Thu, Mar 09, 2023 at 02:09:23PM +0100, Patrick Steinhardt wrote:
> 
> > Now in production systems we have observed that those `.keep` files are
> > sometimes not getting deleted as expected, where the result is that
> > repositories tend to grow packfiles that are never deleted over time.
> > This seems to be caused by a race when git-receive-pack(1) is killed
> > after we have migrated the kept packfile from the quarantine directory
> > into the main object database. While this race window is typically small
> > it can be extended for example by installing a `proc-receive` hook.
> 
> That makes sense, and I think this is a good direction.
> 
> > Fix this race by installing an atexit(3P) handler that unlinks the keep
> > file.
> 
> This will work if we call die(), but I think you'd be better off using
> the tempfile subsystem:
> 
>   - this patch doesn't handle signal death, and I don't see any reason
>     you wouldn't want to handle it there (in fact, from your
>     description, it sounds like signal death is the culprit you suspect)
> 
>   - this will double-unlink in most cases; once when we intend to after
>     calling execute_commands(), and then it will try again (and
>     presumably fail) at exit. Probably not a huge deal, but kind of
>     ugly. You could set it to NULL after unlinking, but...
> 
>   - as the variable is not marked as volatile, a signal that causes an
>     exit could cause the handler to see an inconsistent state if you
>     modify it after setting up the handler. The tempfile code gets this
>     right and is pretty battle-tested.

Ah, I didn't know that you can easily register an already-existing file as tempfile. That is indeed much nicer, thanks!

> I think one could also make an argument that index_pack_lockfile()
> should return a tempfile struct itself, but I didn't look too closely at
> the other caller on the fetch side (but it should be conceptually the
> same).

I had a look at it, but git-fetch-pack(1) works quite differently in that regard as it also supports the case where the packfile lock should stay locked after it exits via the `--keep` switch. So the logic is more intricate here.

Furthermore, git-fetch-pack(1) only does the locking, but never unlocks the packfiles. That is instead handled by git-fetch(1). So converting the interface to use tempfiles directly wouldn't work as we are crossing process boundaries here.

And last but not least, git-fetch(1) already knows to unlock packs both via an atexit handler and via a signal handler. So there is nothing to be done here.

Patrick