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

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

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2102 bytes --]

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(+)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index cd5c7a28ef..0a6030d775 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -2186,6 +2186,12 @@ static const char *parse_pack_header(struct pack_header *hdr)
 
 static const char *pack_lockfile;
 
+static void unlink_pack_lockfile(void)
+{
+	if (pack_lockfile)
+		unlink(pack_lockfile);
+}
+
 static void push_header_arg(struct strvec *args, struct pack_header *hdr)
 {
 	strvec_pushf(args, "--pack_header=%"PRIu32",%"PRIu32,
@@ -2281,6 +2287,7 @@ static const char *unpack(int err_fd, struct shallow_info *si)
 		if (status)
 			return "index-pack fork failed";
 		pack_lockfile = index_pack_lockfile(child.out, NULL);
+		atexit(unlink_pack_lockfile);
 		close(child.out);
 		status = finish_command(&child);
 		if (status)
-- 
2.40.0.rc0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Jeff King]

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.

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

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index cd5c7a28eff..22bbce573e9 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -2184,7 +2184,7 @@ static const char *parse_pack_header(struct pack_header *hdr)
 	}
 }
 
-static const char *pack_lockfile;
+static struct tempfile *pack_lockfile;
 
 static void push_header_arg(struct strvec *args, struct pack_header *hdr)
 {
@@ -2198,6 +2198,7 @@ static const char *unpack(int err_fd, struct shallow_info *si)
 	const char *hdr_err;
 	int status;
 	struct child_process child = CHILD_PROCESS_INIT;
+	char *lockfile;
 	int fsck_objects = (receive_fsck_objects >= 0
 			    ? receive_fsck_objects
 			    : transfer_fsck_objects >= 0
@@ -2280,7 +2281,9 @@ static const char *unpack(int err_fd, struct shallow_info *si)
 		status = start_command(&child);
 		if (status)
 			return "index-pack fork failed";
-		pack_lockfile = index_pack_lockfile(child.out, NULL);
+		lockfile = index_pack_lockfile(child.out, NULL);
+		pack_lockfile = register_tempfile(lockfile);
+		free(lockfile);
 		close(child.out);
 		status = finish_command(&child);
 		if (status)
@@ -2568,8 +2571,7 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)
 		use_keepalive = KEEPALIVE_ALWAYS;
 		execute_commands(commands, unpack_status, &si,
 				 &push_options);
-		if (pack_lockfile)
-			unlink_or_warn(pack_lockfile);
+		delete_tempfile(&pack_lockfile);
 		sigchain_push(SIGPIPE, SIG_IGN);
 		if (report_status_v2)
 			report_v2(commands, unpack_status);

The unconditional call to delete_tempfile() should be OK. If we don't
have a file (because we did unpack-objects instead), then it's a noop.

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).

-Peff

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

[Taylor Blau]

On Thu, Mar 09, 2023 at 02:09:23PM +0100, Patrick Steinhardt wrote:
> Fix this race by installing an atexit(3P) handler that unlinks the keep
> file.

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/

> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  builtin/receive-pack.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index cd5c7a28ef..0a6030d775 100644
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -2186,6 +2186,12 @@ static const char *parse_pack_header(struct pack_header *hdr)
>
>  static const char *pack_lockfile;
>
> +static void unlink_pack_lockfile(void)
> +{
> +	if (pack_lockfile)
> +		unlink(pack_lockfile);
> +}
> +

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

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

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index cd5c7a28ef..0a6030d775 100644
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -2186,6 +2186,12 @@ static const char *parse_pack_header(struct pack_header *hdr)
>  
>  static const char *pack_lockfile;
>  
> +static void unlink_pack_lockfile(void)
> +{
> +	if (pack_lockfile)
> +		unlink(pack_lockfile);
> +}
> +
>  static void push_header_arg(struct strvec *args, struct pack_header *hdr)
>  {
>  	strvec_pushf(args, "--pack_header=%"PRIu32",%"PRIu32,
> @@ -2281,6 +2287,7 @@ static const char *unpack(int err_fd, struct shallow_info *si)
>  		if (status)
>  			return "index-pack fork failed";
>  		pack_lockfile = index_pack_lockfile(child.out, NULL);
> +		atexit(unlink_pack_lockfile);

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

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

[Junio C Hamano]

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

>> @@ -2281,6 +2287,7 @@ static const char *unpack(int err_fd, struct shallow_info *si)
>>  		if (status)
>>  			return "index-pack fork failed";
>>  		pack_lockfile = index_pack_lockfile(child.out, NULL);
>> +		atexit(unlink_pack_lockfile);
>
> Hmph, why isn't this a straight application of tempfile API?

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

Sorry for the noise.

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

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2695 bytes --]

On Thu, Mar 09, 2023 at 10:59:12AM -0500, Jeff King wrote:
> 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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 3020 bytes --]

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(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index cd5c7a28ef..b5369d7429 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -2184,7 +2184,7 @@ static const char *parse_pack_header(struct pack_header *hdr)
 	}
 }
 
-static const char *pack_lockfile;
+static struct tempfile *pack_lockfile;
 
 static void push_header_arg(struct strvec *args, struct pack_header *hdr)
 {
@@ -2251,6 +2251,7 @@ static const char *unpack(int err_fd, struct shallow_info *si)
 			return "unpack-objects abnormal exit";
 	} else {
 		char hostname[HOST_NAME_MAX + 1];
+		char *lockfile;
 
 		strvec_pushl(&child.args, "index-pack", "--stdin", NULL);
 		push_header_arg(&child.args, &hdr);
@@ -2280,8 +2281,14 @@ static const char *unpack(int err_fd, struct shallow_info *si)
 		status = start_command(&child);
 		if (status)
 			return "index-pack fork failed";
-		pack_lockfile = index_pack_lockfile(child.out, NULL);
+
+		lockfile = index_pack_lockfile(child.out, NULL);
+		if (lockfile) {
+			pack_lockfile = register_tempfile(lockfile);
+			free(lockfile);
+		}
 		close(child.out);
+
 		status = finish_command(&child);
 		if (status)
 			return "index-pack abnormal exit";
@@ -2568,8 +2575,7 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)
 		use_keepalive = KEEPALIVE_ALWAYS;
 		execute_commands(commands, unpack_status, &si,
 				 &push_options);
-		if (pack_lockfile)
-			unlink_or_warn(pack_lockfile);
+		delete_tempfile(&pack_lockfile);
 		sigchain_push(SIGPIPE, SIG_IGN);
 		if (report_status_v2)
 			report_v2(commands, unpack_status);
-- 
2.40.0.rc0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Jeff King]

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

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

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

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

[Jeff King]

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

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

Unsurprisingly, this version looks good to me. :)

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

>  builtin/receive-pack.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)

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

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

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1108 bytes --]

On Fri, Mar 10, 2023 at 03:51:29AM -0500, Jeff King wrote:
> On Fri, Mar 10, 2023 at 07:52:10AM +0100, Patrick Steinhardt wrote:
[snip]
> >  builtin/receive-pack.c | 14 ++++++++++----
> >  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> 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).

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Jeff King]

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

> > 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).
> 
> 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.

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).

diff --git a/t/t5400-send-pack.sh b/t/t5400-send-pack.sh
index 3f81f16e133..ca6950ef433 100755
--- a/t/t5400-send-pack.sh
+++ b/t/t5400-send-pack.sh
@@ -300,4 +300,37 @@ test_expect_success 'receive-pack de-dupes .have lines' '
 	test_cmp expect refs
 '
 
+test_expect_success PIPE 'receive-pack cleans up .keep after signal' '
+	git init --bare keep.git &&
+	write_script keep.git/hooks/proc-receive <<-\EOF &&
+	# tell the other side we are in the hook
+	echo running proc-receive >&9
+	# and then wait until our parent receive-pack exits
+	wait "$PPID"
+	EOF
+	git -C keep.git config receive.procReceiveRefs refs/ &&
+
+	mkfifo rpstatus &&
+	(
+		# this will fail because we kill receive-pack while it hangs,
+		# but we will not see its exit code until we "wait" below.
+		git push \
+			--receive-pack="echo \$\$ >&9; exec git-receive-pack" \
+			--all keep.git 9>rpstatus &
+	) &&
+
+	exec 9<rpstatus &&
+	read <&9 rp_pid &&
+	read <&9 ready_signal &&
+	kill -15 "$rp_pid" &&
+	# in theory we get the signal code from receive-pack here,
+	# but we are racing with send-pack to call wait(), so we
+	# might also get "not found" from wait
+	{ wait "$rp_pid"; OUT=$?; } &&
+	{ test_match_signal 15 $OUT || test "$OUT" = 127; } &&
+
+	find keep.git/objects/pack -name "*.keep" >actual &&
+	test_must_be_empty actual
+'
+
 test_done

It fails as expected without your patch (the .keep file is left in
"actual"), and passes with it.

-Peff