{"thread":{"id":"59369","subject":"[PATCH] receive-pack: fix stale packfile locks when dying","startedAt":"2023-03-09T13:09:38Z","lastAt":"2023-03-10T13:20:44Z","messageCount":11,"participants":["Patrick Steinhardt","Jeff King","Taylor Blau","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"473281","messageId":"e16bd81bf9e251aa6959fbe10a3fbc215a4a1c12.1678367338.git.ps@pks.im","threadId":"59369","inReplyTo":null,"subject":"[PATCH] receive-pack: fix stale packfile locks when dying","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-03-09T13:09:23Z","receivedAt":"2023-03-09T13:09:38Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When accepting a packfile in git-receive-pack(1), we feed that packfile\ninto git-index-pack(1) to generate the packfile index. As the packfile\nwould often only contain unreachable objects until the references have\nbeen updated, concurrently running garbage collection might be tempted\nto delete the packfile right away and thus cause corruption. To fix\nthis, we ask git-index-pack(1) to create a `.keep` file before moving\nthe packfile into place, which is getting deleted again once all of the\nreference updates have been processed.\n\nNow in production systems we have observed that those `.keep` files are\nsometimes not getting deleted as expected, where the result is that\nrepositories tend to grow packfiles that are never deleted over time.\nThis seems to be caused by a race when git-receive-pack(1) is killed\nafter we have migrated the kept packfile from the quarantine directory\ninto the main object database. While this race window is typically small\nit can be extended for example by installing a `proc-receive` hook.\n\nFix this race by installing an atexit(3P) handler that unlinks the keep\nfile.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/receive-pack.c | 7 +++++++\n 1 file changed, 7 insertions(+)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex cd5c7a28ef..0a6030d775 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -2186,6 +2186,12 @@ static const char *parse_pack_header(struct pack_header *hdr)\n \n static const char *pack_lockfile;\n \n+static void unlink_pack_lockfile(void)\n+{\n+\tif (pack_lockfile)\n+\t\tunlink(pack_lockfile);\n+}\n+\n static void push_header_arg(struct strvec *args, struct pack_header *hdr)\n {\n \tstrvec_pushf(args, \"--pack_header=%\"PRIu32\",%\"PRIu32,\n@@ -2281,6 +2287,7 @@ static const char *unpack(int err_fd, struct shallow_info *si)\n \t\tif (status)\n \t\t\treturn \"index-pack fork failed\";\n \t\tpack_lockfile = index_pack_lockfile(child.out, NULL);\n+\t\tatexit(unlink_pack_lockfile);\n \t\tclose(child.out);\n \t\tstatus = finish_command(&child);\n \t\tif (status)\n-- \n2.40.0.rc0\n\n"},{"id":"473289","messageId":"ZAoCUPsHbstSJ0+B@coredump.intra.peff.net","threadId":"59369","inReplyTo":"e16bd81bf9e251aa6959fbe10a3fbc215a4a1c12.1678367338.git.ps@pks.im","subject":"Re: [PATCH] receive-pack: fix stale packfile locks when dying","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-03-09T15:59:12Z","receivedAt":"2023-03-09T15:59:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 09, 2023 at 02:09:23PM +0100, Patrick Steinhardt wrote:\n\n> Now in production systems we have observed that those `.keep` files are\n> sometimes not getting deleted as expected, where the result is that\n> repositories tend to grow packfiles that are never deleted over time.\n> This seems to be caused by a race when git-receive-pack(1) is killed\n> after we have migrated the kept packfile from the quarantine directory\n> into the main object database. While this race window is typically small\n> it can be extended for example by installing a `proc-receive` hook.\n\nThat makes sense, and I think this is a good direction.\n\n> Fix this race by installing an atexit(3P) handler that unlinks the keep\n> file.\n\nThis will work if we call die(), but I think you'd be better off using\nthe tempfile subsystem:\n\n - this patch doesn't handle signal death, and I don't see any reason\n you wouldn't want to handle it there (in fact, from your\n description, it sounds like signal death is the culprit you suspect)\n\n - this will double-unlink in most cases; once when we intend to after\n calling execute_commands(), and then it will try again (and\n presumably fail) at exit. Probably not a huge deal, but kind of\n ugly. You could set it to NULL after unlinking, but...\n\n - as the variable is not marked as volatile, a signal that causes an\n exit could cause the handler to see an inconsistent state if you\n modify it after setting up the handler. The tempfile code gets this\n right and is pretty battle-tested.\n\nI think you'd just want something like this (totally untested):\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex cd5c7a28eff..22bbce573e9 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -2184,7 +2184,7 @@ static const char *parse_pack_header(struct pack_header *hdr)\n \t}\n }\n \n-static const char *pack_lockfile;\n+static struct tempfile *pack_lockfile;\n \n static void push_header_arg(struct strvec *args, struct pack_header *hdr)\n {\n@@ -2198,6 +2198,7 @@ static const char *unpack(int err_fd, struct shallow_info *si)\n \tconst char *hdr_err;\n \tint status;\n \tstruct child_process child = CHILD_PROCESS_INIT;\n+\tchar *lockfile;\n \tint fsck_objects = (receive_fsck_objects >= 0\n \t\t\t ? receive_fsck_objects\n \t\t\t : transfer_fsck_objects >= 0\n@@ -2280,7 +2281,9 @@ static const char *unpack(int err_fd, struct shallow_info *si)\n \t\tstatus = start_command(&child);\n \t\tif (status)\n \t\t\treturn \"index-pack fork failed\";\n-\t\tpack_lockfile = index_pack_lockfile(child.out, NULL);\n+\t\tlockfile = index_pack_lockfile(child.out, NULL);\n+\t\tpack_lockfile = register_tempfile(lockfile);\n+\t\tfree(lockfile);\n \t\tclose(child.out);\n \t\tstatus = finish_command(&child);\n \t\tif (status)\n@@ -2568,8 +2571,7 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)\n \t\tuse_keepalive = KEEPALIVE_ALWAYS;\n \t\texecute_commands(commands, unpack_status, &si,\n \t\t\t\t &push_options);\n-\t\tif (pack_lockfile)\n-\t\t\tunlink_or_warn(pack_lockfile);\n+\t\tdelete_tempfile(&pack_lockfile);\n \t\tsigchain_push(SIGPIPE, SIG_IGN);\n \t\tif (report_status_v2)\n \t\t\treport_v2(commands, unpack_status);\n\nThe unconditional call to delete_tempfile() should be OK. If we don't\nhave a file (because we did unpack-objects instead), then it's a noop.\n\nI think one could also make an argument that index_pack_lockfile()\nshould return a tempfile struct itself, but I didn't look too closely at\nthe other caller on the fetch side (but it should be conceptually the\nsame).\n\n-Peff\n"},{"id":"473306","messageId":"ZAokxsXGwgdNEmDc@nand.local","threadId":"59369","inReplyTo":"e16bd81bf9e251aa6959fbe10a3fbc215a4a1c12.1678367338.git.ps@pks.im","subject":"Re: [PATCH] receive-pack: fix stale packfile locks when dying","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-03-09T18:26:14Z","receivedAt":"2023-03-09T18:26:20Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Mar 09, 2023 at 02:09:23PM +0100, Patrick Steinhardt wrote:\n> Fix this race by installing an atexit(3P) handler that unlinks the keep\n> file.\n\nThis reminded me of a discussion that I thought you and I had a few\nmonths ago on the list about whether or not it was safe to call unlink()\nin an async signal handler.\n\nTL;DR, it is, and the link back to that discussion is here:\n\n https://lore.kernel.org/git/YdjBkZsnYd+zYne1@nand.local/\n\n> Signed-off-by: Patrick Steinhardt \n> ---\n> builtin/receive-pack.c | 7 +++++++\n> 1 file changed, 7 insertions(+)\n>\n> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\n> index cd5c7a28ef..0a6030d775 100644\n> --- a/builtin/receive-pack.c\n> +++ b/builtin/receive-pack.c\n> @@ -2186,6 +2186,12 @@ static const char *parse_pack_header(struct pack_header *hdr)\n>\n> static const char *pack_lockfile;\n>\n> +static void unlink_pack_lockfile(void)\n> +{\n> +\tif (pack_lockfile)\n> +\t\tunlink(pack_lockfile);\n> +}\n> +\n\n...and I think that this would all work, but I agree that using the\ntempfile API here (as Peff suggests below) would probably be more\nergonomic.\n\nThanks,\nTaylor\n"},{"id":"473309","messageId":"xmqqsfedahhl.fsf@gitster.g","threadId":"59369","inReplyTo":"e16bd81bf9e251aa6959fbe10a3fbc215a4a1c12.1678367338.git.ps@pks.im","subject":"Re: [PATCH] receive-pack: fix stale packfile locks when dying","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-09T18:48:22Z","receivedAt":"2023-03-09T18:48:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\n> index cd5c7a28ef..0a6030d775 100644\n> --- a/builtin/receive-pack.c\n> +++ b/builtin/receive-pack.c\n> @@ -2186,6 +2186,12 @@ static const char *parse_pack_header(struct pack_header *hdr)\n> \n> static const char *pack_lockfile;\n> \n> +static void unlink_pack_lockfile(void)\n> +{\n> +\tif (pack_lockfile)\n> +\t\tunlink(pack_lockfile);\n> +}\n> +\n> static void push_header_arg(struct strvec *args, struct pack_header *hdr)\n> {\n> \tstrvec_pushf(args, \"--pack_header=%\"PRIu32\",%\"PRIu32,\n> @@ -2281,6 +2287,7 @@ static const char *unpack(int err_fd, struct shallow_info *si)\n> \t\tif (status)\n> \t\t\treturn \"index-pack fork failed\";\n> \t\tpack_lockfile = index_pack_lockfile(child.out, NULL);\n> +\t\tatexit(unlink_pack_lockfile);\n\nHmph, why isn't this a straight application of tempfile API?\n"},{"id":"473310","messageId":"xmqqo7p1ahgi.fsf@gitster.g","threadId":"59369","inReplyTo":"xmqqsfedahhl.fsf@gitster.g","subject":"Re: [PATCH] receive-pack: fix stale packfile locks when dying","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-09T18:49:01Z","receivedAt":"2023-03-09T18:49:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n>> @@ -2281,6 +2287,7 @@ static const char *unpack(int err_fd, struct shallow_info *si)\n>> \t\tif (status)\n>> \t\t\treturn \"index-pack fork failed\";\n>> \t\tpack_lockfile = index_pack_lockfile(child.out, NULL);\n>> +\t\tatexit(unlink_pack_lockfile);\n>\n> Hmph, why isn't this a straight application of tempfile API?\n\nAh, sorry, that has already been raised in the thread. I should\nhave checked first.\n\nSorry for the noise.\n"},{"id":"473338","messageId":"ZArNJHMmPd8QCLwQ@ncase","threadId":"59369","inReplyTo":"ZAoCUPsHbstSJ0+B@coredump.intra.peff.net","subject":"Re: [PATCH] receive-pack: fix stale packfile locks when dying","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-03-10T06:24:36Z","receivedAt":"2023-03-10T06:26:57Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Mar 09, 2023 at 10:59:12AM -0500, Jeff King wrote:\n> On Thu, Mar 09, 2023 at 02:09:23PM +0100, Patrick Steinhardt wrote:\n> \n> > Now in production systems we have observed that those `.keep` files are\n> > sometimes not getting deleted as expected, where the result is that\n> > repositories tend to grow packfiles that are never deleted over time.\n> > This seems to be caused by a race when git-receive-pack(1) is killed\n> > after we have migrated the kept packfile from the quarantine directory\n> > into the main object database. While this race window is typically small\n> > it can be extended for example by installing a `proc-receive` hook.\n> \n> That makes sense, and I think this is a good direction.\n> \n> > Fix this race by installing an atexit(3P) handler that unlinks the keep\n> > file.\n> \n> This will work if we call die(), but I think you'd be better off using\n> the tempfile subsystem:\n> \n> - this patch doesn't handle signal death, and I don't see any reason\n> you wouldn't want to handle it there (in fact, from your\n> description, it sounds like signal death is the culprit you suspect)\n> \n> - this will double-unlink in most cases; once when we intend to after\n> calling execute_commands(), and then it will try again (and\n> presumably fail) at exit. Probably not a huge deal, but kind of\n> ugly. You could set it to NULL after unlinking, but...\n> \n> - as the variable is not marked as volatile, a signal that causes an\n> exit could cause the handler to see an inconsistent state if you\n> modify it after setting up the handler. The tempfile code gets this\n> right and is pretty battle-tested.\n\nAh, I didn't know that you can easily register an already-existing file\nas tempfile. That is indeed much nicer, thanks!\n\n> I think one could also make an argument that index_pack_lockfile()\n> should return a tempfile struct itself, but I didn't look too closely at\n> the other caller on the fetch side (but it should be conceptually the\n> same).\n\nI had a look at it, but git-fetch-pack(1) works quite differently in\nthat regard as it also supports the case where the packfile lock should\nstay locked after it exits via the `--keep` switch. So the logic is more\nintricate here.\n\nFurthermore, git-fetch-pack(1) only does the locking, but never unlocks\nthe packfiles. That is instead handled by git-fetch(1). So converting\nthe interface to use tempfiles directly wouldn't work as we are crossing\nprocess boundaries here.\n\nAnd last but not least, git-fetch(1) already knows to unlock packs both\nvia an atexit handler and via a signal handler. So there is nothing to\nbe done here.\n\nPatrick\n"},{"id":"473339","messageId":"e1ee1d8026a361bc58d16bc741e2b347ada7a53e.1678431076.git.ps@pks.im","threadId":"59369","inReplyTo":"e16bd81bf9e251aa6959fbe10a3fbc215a4a1c12.1678367338.git.ps@pks.im","subject":"[PATCH v2] receive-pack: fix stale packfile locks when dying","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-03-10T06:52:10Z","receivedAt":"2023-03-10T06:52:33Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When accepting a packfile in git-receive-pack(1), we feed that packfile\ninto git-index-pack(1) to generate the packfile index. As the packfile\nwould often only contain unreachable objects until the references have\nbeen updated, concurrently running garbage collection might be tempted\nto delete the packfile right away and thus cause corruption. To fix\nthis, we ask git-index-pack(1) to create a `.keep` file before moving\nthe packfile into place, which is getting deleted again once all of the\nreference updates have been processed.\n\nNow in production systems we have observed that those `.keep` files are\nsometimes not getting deleted as expected, where the result is that\nrepositories tend to grow packfiles that are never deleted over time.\nThis seems to be caused by a race when git-receive-pack(1) is killed\nafter we have migrated the kept packfile from the quarantine directory\ninto the main object database. While this race window is typically small\nit can be extended for example by installing a `proc-receive` hook.\n\nFix this race by registering the lockfile as a tempfile so that it will\nautomatically be removed at exit or when receiving a signal.\n\nHelped-by: Jeff King \nSigned-off-by: Patrick Steinhardt \n---\n builtin/receive-pack.c | 14 ++++++++++----\n 1 file changed, 10 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex cd5c7a28ef..b5369d7429 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -2184,7 +2184,7 @@ static const char *parse_pack_header(struct pack_header *hdr)\n \t}\n }\n \n-static const char *pack_lockfile;\n+static struct tempfile *pack_lockfile;\n \n static void push_header_arg(struct strvec *args, struct pack_header *hdr)\n {\n@@ -2251,6 +2251,7 @@ static const char *unpack(int err_fd, struct shallow_info *si)\n \t\t\treturn \"unpack-objects abnormal exit\";\n \t} else {\n \t\tchar hostname[HOST_NAME_MAX + 1];\n+\t\tchar *lockfile;\n \n \t\tstrvec_pushl(&child.args, \"index-pack\", \"--stdin\", NULL);\n \t\tpush_header_arg(&child.args, &hdr);\n@@ -2280,8 +2281,14 @@ static const char *unpack(int err_fd, struct shallow_info *si)\n \t\tstatus = start_command(&child);\n \t\tif (status)\n \t\t\treturn \"index-pack fork failed\";\n-\t\tpack_lockfile = index_pack_lockfile(child.out, NULL);\n+\n+\t\tlockfile = index_pack_lockfile(child.out, NULL);\n+\t\tif (lockfile) {\n+\t\t\tpack_lockfile = register_tempfile(lockfile);\n+\t\t\tfree(lockfile);\n+\t\t}\n \t\tclose(child.out);\n+\n \t\tstatus = finish_command(&child);\n \t\tif (status)\n \t\t\treturn \"index-pack abnormal exit\";\n@@ -2568,8 +2575,7 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)\n \t\tuse_keepalive = KEEPALIVE_ALWAYS;\n \t\texecute_commands(commands, unpack_status, &si,\n \t\t\t\t &push_options);\n-\t\tif (pack_lockfile)\n-\t\t\tunlink_or_warn(pack_lockfile);\n+\t\tdelete_tempfile(&pack_lockfile);\n \t\tsigchain_push(SIGPIPE, SIG_IGN);\n \t\tif (report_status_v2)\n \t\t\treport_v2(commands, unpack_status);\n-- \n2.40.0.rc0\n\n"},{"id":"473340","messageId":"ZArsRLHksmJ9Tgoy@coredump.intra.peff.net","threadId":"59369","inReplyTo":"ZArNJHMmPd8QCLwQ@ncase","subject":"Re: [PATCH] receive-pack: fix stale packfile locks when dying","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-03-10T08:37:24Z","receivedAt":"2023-03-10T08:40:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 10, 2023 at 07:24:36AM +0100, Patrick Steinhardt wrote:\n\n> > I think one could also make an argument that index_pack_lockfile()\n> > should return a tempfile struct itself, but I didn't look too closely at\n> > the other caller on the fetch side (but it should be conceptually the\n> > same).\n> \n> I had a look at it, but git-fetch-pack(1) works quite differently in\n> that regard as it also supports the case where the packfile lock should\n> stay locked after it exits via the `--keep` switch. So the logic is more\n> intricate here.\n> \n> Furthermore, git-fetch-pack(1) only does the locking, but never unlocks\n> the packfiles. That is instead handled by git-fetch(1). So converting\n> the interface to use tempfiles directly wouldn't work as we are crossing\n> process boundaries here.\n\nI think the calls into fetch-pack.c that handle the pack lockfiles can\nhappen in-process from git-fetch itself. But I also think there are\nprobably cases where they don't (v0 git-over-http should use a separate\n\"fetch-pack --stateless-rpc\", I believe).\n\nSo yeah, it's probably too complicated to worry about lumping in here,\nespecially since you noted that it handles cleanup correctly already.\n\nThanks for looking into it.\n\n-Peff\n"},{"id":"473341","messageId":"ZArvkVPhF4Th424Z@coredump.intra.peff.net","threadId":"59369","inReplyTo":"e1ee1d8026a361bc58d16bc741e2b347ada7a53e.1678431076.git.ps@pks.im","subject":"Re: [PATCH v2] receive-pack: fix stale packfile locks when dying","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-03-10T08:51:29Z","receivedAt":"2023-03-10T08:58:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 10, 2023 at 07:52:10AM +0100, Patrick Steinhardt wrote:\n\n> Fix this race by registering the lockfile as a tempfile so that it will\n> automatically be removed at exit or when receiving a signal.\n\nUnsurprisingly, this version looks good to me. :)\n\nLooks like you adjusted for index_pack_lockfile() returning NULL, which\nmakes sense.\n\n> builtin/receive-pack.c | 14 ++++++++++----\n> 1 file changed, 10 insertions(+), 4 deletions(-)\n\nThere's no test here, and I think we _could_ make a reliable one with\nsomething like:\n\n 1. Have a proc-receive hook that signals via fifo that it's running,\n then pauses indefinitely.\n\n 2. Start a push in the background, then wait on the fifo signal.\n\n 3. Kill receive-pack.\n\nBut it's awfully complicated for little gain. I'm fine with not worrying\nabout it (and I did something similar manually to make to sure it works\nas we expect).\n\n-Peff\n"},{"id":"473352","messageId":"ZAsZMAhgag1iBSK5@ncase","threadId":"59369","inReplyTo":"ZArvkVPhF4Th424Z@coredump.intra.peff.net","subject":"Re: [PATCH v2] receive-pack: fix stale packfile locks when dying","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-03-10T11:49:04Z","receivedAt":"2023-03-10T11:49:18Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Mar 10, 2023 at 03:51:29AM -0500, Jeff King wrote:\n> On Fri, Mar 10, 2023 at 07:52:10AM +0100, Patrick Steinhardt wrote:\n[snip]\n> > builtin/receive-pack.c | 14 ++++++++++----\n> > 1 file changed, 10 insertions(+), 4 deletions(-)\n> \n> There's no test here, and I think we _could_ make a reliable one with\n> something like:\n> \n> 1. Have a proc-receive hook that signals via fifo that it's running,\n> then pauses indefinitely.\n> \n> 2. Start a push in the background, then wait on the fifo signal.\n> \n> 3. Kill receive-pack.\n> \n> But it's awfully complicated for little gain. I'm fine with not worrying\n> about it (and I did something similar manually to make to sure it works\n> as we expect).\n\nI actually tried doing exactly what you're proposing here yesterday. It\ncaused hangs, lingering processes, you had to play games to not trigger\nthe &&-chain linter due to the backgrounding via &, and ultimately it\nsomehow just didn't work.\n\nSo at some point I came to the same conclusion as you did, that it's\njust too complicated for little gain.\n\nThanks.\n\nPatrick\n"},{"id":"473355","messageId":"ZAsupPsPPiRnEs4h@coredump.intra.peff.net","threadId":"59369","inReplyTo":"ZAsZMAhgag1iBSK5@ncase","subject":"Re: [PATCH v2] receive-pack: fix stale packfile locks when dying","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-03-10T13:20:36Z","receivedAt":"2023-03-10T13:20:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 10, 2023 at 12:49:04PM +0100, Patrick Steinhardt wrote:\n\n> > There's no test here, and I think we _could_ make a reliable one with\n> > something like:\n> > \n> > 1. Have a proc-receive hook that signals via fifo that it's running,\n> > then pauses indefinitely.\n> > \n> > 2. Start a push in the background, then wait on the fifo signal.\n> > \n> > 3. Kill receive-pack.\n> > \n> > But it's awfully complicated for little gain. I'm fine with not worrying\n> > about it (and I did something similar manually to make to sure it works\n> > as we expect).\n> \n> I actually tried doing exactly what you're proposing here yesterday. It\n> caused hangs, lingering processes, you had to play games to not trigger\n> the &&-chain linter due to the backgrounding via &, and ultimately it\n> somehow just didn't work.\n> \n> So at some point I came to the same conclusion as you did, that it's\n> just too complicated for little gain.\n\nSo I don't think we should apply this, because it's horrific. But\nbecause I like a challenge, here is a test that should be race-free,\ndoesn't hang, and doesn't leave lingering processes (the blocking hook\nwaits for its parent to die).\n\ndiff --git a/t/t5400-send-pack.sh b/t/t5400-send-pack.sh\nindex 3f81f16e133..ca6950ef433 100755\n--- a/t/t5400-send-pack.sh\n+++ b/t/t5400-send-pack.sh\n@@ -300,4 +300,37 @@ test_expect_success 'receive-pack de-dupes .have lines' '\n \ttest_cmp expect refs\n '\n \n+test_expect_success PIPE 'receive-pack cleans up .keep after signal' '\n+\tgit init --bare keep.git &&\n+\twrite_script keep.git/hooks/proc-receive <<-\\EOF &&\n+\t# tell the other side we are in the hook\n+\techo running proc-receive >&9\n+\t# and then wait until our parent receive-pack exits\n+\twait \"$PPID\"\n+\tEOF\n+\tgit -C keep.git config receive.procReceiveRefs refs/ &&\n+\n+\tmkfifo rpstatus &&\n+\t(\n+\t\t# this will fail because we kill receive-pack while it hangs,\n+\t\t# but we will not see its exit code until we \"wait\" below.\n+\t\tgit push \\\n+\t\t\t--receive-pack=\"echo \\$\\$ >&9; exec git-receive-pack\" \\\n+\t\t\t--all keep.git 9>rpstatus &\n+\t) &&\n+\n+\texec 9actual &&\n+\ttest_must_be_empty actual\n+'\n+\n test_done\n\nIt fails as expected without your patch (the .keep file is left in\n\"actual\"), and passes with it.\n\n-Peff\n"}]}