patchreceive-pack: fix stale packfile locks when dying

When accepting a packfile in git-receive-pack(1), we feed that packfile into git-index-pack(1) to generate the packfile index. As the packfile would often only contain unreachable objects until the references have been updated, concurrently running garbage collection might be tempted to delete the packfile right away and thus cause corruption. To fix this, we ask git-index-pack(1) to create a `.keep` file before moving the packfile into place, which is getting deleted again once all of the reference updates have been processed.

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.

Fix this race by installing an atexit(3P) handler that unlinks the keep file.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/receive-pack.c | 7 +++++++
 1 file changed, 7 insertions(+)

On Thu, Mar 09, 2023 at 02:09:23PM +0100, Patrick Steinhardt wrote:

That makes sense, and I think this is a good direction.

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.

I think you'd just want something like this (totally untested):

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

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

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

On Fri, Mar 10, 2023 at 07:24:36AM +0100, Patrick Steinhardt wrote:

I think the calls into fetch-pack.c that handle the pack lockfiles can happen in-process from git-fetch itself. But I also think there are probably cases where they don't (v0 git-over-http should use a separate "fetch-pack --stateless-rpc", I believe).

So yeah, it's probably too complicated to worry about lumping in here, especially since you noted that it handles cleanup correctly already.

Thanks for looking into it.

-Peff

On Thu, Mar 09, 2023 at 02:09:23PM +0100, Patrick Steinhardt wrote:

This reminded me of a discussion that I thought you and I had a few months ago on the list about whether or not it was safe to call unlink() in an async signal handler.

TL;DR, it is, and the link back to that discussion is here:

  https://lore.kernel.org/git/YdjBkZsnYd+zYne1@nand.local/

...and I think that this would all work, but I agree that using the tempfile API here (as Peff suggests below) would probably be more ergonomic.

Thanks, Taylor

Patrick Steinhardt <ps@pks.im> writes:

Hmph, why isn't this a straight application of tempfile API?

Junio C Hamano <gitster@pobox.com> writes:

Ah, sorry, that has already been raised in the thread. I should have checked first.

Sorry for the noise.

When accepting a packfile in git-receive-pack(1), we feed that packfile into git-index-pack(1) to generate the packfile index. As the packfile would often only contain unreachable objects until the references have been updated, concurrently running garbage collection might be tempted to delete the packfile right away and thus cause corruption. To fix this, we ask git-index-pack(1) to create a `.keep` file before moving the packfile into place, which is getting deleted again once all of the reference updates have been processed.

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.

Fix this race by registering the lockfile as a tempfile so that it will automatically be removed at exit or when receiving a signal.

Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/receive-pack.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

On Fri, Mar 10, 2023 at 07:52:10AM +0100, Patrick Steinhardt wrote:

Unsurprisingly, this version looks good to me. :)

Looks like you adjusted for index_pack_lockfile() returning NULL, which makes sense.

There's no test here, and I think we _could_ make a reliable one with something like:

  1. Have a proc-receive hook that signals via fifo that it's running,
     then pauses indefinitely.

  2. Start a push in the background, then wait on the fifo signal.

  3. Kill receive-pack.

But it's awfully complicated for little gain. I'm fine with not worrying about it (and I did something similar manually to make to sure it works as we expect).

-Peff

On Fri, Mar 10, 2023 at 03:51:29AM -0500, Jeff King wrote:

[snip]

I actually tried doing exactly what you're proposing here yesterday. It caused hangs, lingering processes, you had to play games to not trigger the &&-chain linter due to the backgrounding via &, and ultimately it somehow just didn't work.

So at some point I came to the same conclusion as you did, that it's just too complicated for little gain.

Thanks.

Patrick

On Fri, Mar 10, 2023 at 12:49:04PM +0100, Patrick Steinhardt wrote:

So I don't think we should apply this, because it's horrific. But because I like a challenge, here is a test that should be race-free, doesn't hang, and doesn't leave lingering processes (the blocking hook waits for its parent to die).