{"thread":{"id":"56841","subject":"[RFC PATCH] receive-pack: run post-receive before reporting status","startedAt":"2021-11-04T13:44:53Z","lastAt":"2021-12-29T14:22:00Z","messageCount":11,"participants":["Robin Jarry","Ævar Arnfjörð Bjarmason","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"440452","messageId":"20211104133546.1967308-1-robin.jarry@6wind.com","threadId":"56841","inReplyTo":null,"subject":"[RFC PATCH] receive-pack: run post-receive before reporting status","fromName":"Robin Jarry","fromEmail":"robin.jarry@6wind.com","sentAt":"2021-11-04T13:35:47Z","receivedAt":"2021-11-04T13:44:53Z","isPatch":true,"sender":{"key":"robin.jarry@6wind.com","avatar":null},"body":"When a remote client exits while the pre-receive hook is running,\nreceive-pack is not killed by SIGPIPE because the signal is ignored.\nThis is a side effect of commit ec7dbd145bd8 (\"receive-pack: allow hooks\nto ignore its standard input stream\").\n\nThe pre-receive hook is not interrupted and does not receive any error\nsince its stdout is a pipe which is read in an async thread and output\nback to the client socket in a side band channel. When writing the data\nin the socket, the async thread gets a SIGPIPE which also seems ignored.\nThis may be a race between the main and the async threads. I do not know\nthe code well enough to be sure.\n\nAfter the pre-receive has exited the SIGPIPE default handler is restored\nand if the hook did not report any error, objects are migrated from\ntemporary to permanent storage.\n\nBefore running the post-receive hook, status info is reported back to\nthe client. Since the client has died, receive-pack is killed by SIGPIPE\nand post-receive is never executed.\n\nThe post-receive hook is often used to send email notifications (see\ncontrib/hooks/post-receive-email), update bug trackers, start automatic\nbuilds, etc. Not executing it after an interrupted yet \"successful\" push\ncan lead to inconsistencies.\n\nExecute the post-receive hook before reporting status to the client to\navoid this issue. This is not an ideal solution but I don't know if\nallowing hooks to be killed when a client exits is a good idea. Maybe\nfor pre-receive but definitely not for post-receive.\n\nSigned-off-by: Robin Jarry \nSigned-off-by: Nicolas Dichtel \n---\n builtin/receive-pack.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 49b846d96052..df8bedf71319 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -2564,14 +2564,14 @@ 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\trun_receive_hook(commands, \"post-receive\", 1,\n+\t\t\t\t &push_options);\n \t\tif (pack_lockfile)\n \t\t\tunlink_or_warn(pack_lockfile);\n \t\tif (report_status_v2)\n \t\t\treport_v2(commands, unpack_status);\n \t\telse if (report_status)\n \t\t\treport(commands, unpack_status);\n-\t\trun_receive_hook(commands, \"post-receive\", 1,\n-\t\t\t\t &push_options);\n \t\trun_update_post_hook(commands);\n \t\tstring_list_clear(&push_options, 0);\n \t\tif (auto_gc) {\n-- \n2.30.2\n\n"},{"id":"440569","messageId":"211106.86y261g88n.gmgdl@evledraar.gmail.com","threadId":"56841","inReplyTo":"20211104133546.1967308-1-robin.jarry@6wind.com","subject":"Re: [RFC PATCH] receive-pack: run post-receive before reporting status","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-11-06T05:03:22Z","receivedAt":"2021-11-06T05:31:58Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Nov 04 2021, Robin Jarry wrote:\n\n> When a remote client exits while the pre-receive hook is running,\n> receive-pack is not killed by SIGPIPE because the signal is ignored.\n> This is a side effect of commit ec7dbd145bd8 (\"receive-pack: allow hooks\n> to ignore its standard input stream\").\n\nFWIW we include the date when mentioning commits. E.g. ec7dbd145bd\n(receive-pack: allow hooks to ignore its standard input stream,\n2014-09-12).\n\n> The pre-receive hook is not interrupted and does not receive any error\n> since its stdout is a pipe which is read in an async thread and output\n> back to the client socket in a side band channel. When writing the data\n> in the socket, the async thread gets a SIGPIPE which also seems ignored.\n> This may be a race between the main and the async threads. I do not know\n> the code well enough to be sure.\n>\n> After the pre-receive has exited the SIGPIPE default handler is restored\n> and if the hook did not report any error, objects are migrated from\n> temporary to permanent storage.\n>\n> Before running the post-receive hook, status info is reported back to\n> the client. Since the client has died, receive-pack is killed by SIGPIPE\n> and post-receive is never executed.\n>\n> The post-receive hook is often used to send email notifications (see\n> contrib/hooks/post-receive-email), update bug trackers, start automatic\n> builds, etc. Not executing it after an interrupted yet \"successful\" push\n> can lead to inconsistencies.\n>\n> Execute the post-receive hook before reporting status to the client to\n> avoid this issue. This is not an ideal solution but I don't know if\n> allowing hooks to be killed when a client exits is a good idea. Maybe\n> for pre-receive but definitely not for post-receive.\n>\n> Signed-off-by: Robin Jarry \n> Signed-off-by: Nicolas Dichtel \n> ---\n> builtin/receive-pack.c | 4 ++--\n> 1 file changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\n> index 49b846d96052..df8bedf71319 100644\n> --- a/builtin/receive-pack.c\n> +++ b/builtin/receive-pack.c\n> @@ -2564,14 +2564,14 @@ 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\trun_receive_hook(commands, \"post-receive\", 1,\n> +\t\t\t\t &push_options);\n> \t\tif (pack_lockfile)\n> \t\t\tunlink_or_warn(pack_lockfile);\n> \t\tif (report_status_v2)\n> \t\t\treport_v2(commands, unpack_status);\n> \t\telse if (report_status)\n> \t\t\treport(commands, unpack_status);\n> -\t\trun_receive_hook(commands, \"post-receive\", 1,\n> -\t\t\t\t &push_options);\n> \t\trun_update_post_hook(commands);\n> \t\tstring_list_clear(&push_options, 0);\n> \t\tif (auto_gc) {\n\nI think the discussion at [1] is current to everything you're seeing\nhere.\n\ntl;dr: Even with this change you're not guaranteed to run your hook.\n\nPersonally I've implemented something (independent of) what Junio\nsuggested downthread[2] of that. I.e. to simply insert a DB record on\npre-receive/post-receive, and have all \"real\" work done async by a job\nthat's following that.\n\nI used MySQL as a dumb queue, but this can also be done with a text\nfile. I'd end up with 3x records:\n\n A. pre-receive: what the client wants to push\n B. pre-receive (at the very end): what we accepted the client pushing (after running all checks)\n C. post-receive: logging the same rev range, hopefully\n\nAs you've found you won't always get a \"C\", so such a following job\ncurrently needs to repair such records after the fact, i.e. be ready to\ninspect the repo and see if the push actually happened. You also won't\nget \"C\" if you OK'd a push, but had a race for the *.lock file, or other\nsimilar push contention.\n\nI think one objection some might have to this is that we'd like to not\nwait for the post-receive hook, which I falsely recalled would be\nimpacted by this, but as Jeff King points out at [3] we'd do the same\neither way, so this change won't impact that either way.\n\nBut I think one thing that will be negatively impacted is touched upon\nby your:\n\n \"Since the client has died[...]Not executing it after an interrupted\n yet \"successful\" push can lead to inconsistencies\".\n\nYou don't know if it died, you just got killed by SIGPIPE. That can\nhappen for any number of reasons, the client might have gotten its data,\nyou just can't reach it anymore.\n\nI think you're right that it's inconsistent, but wrong about this\n\"fixing\" the inconsistency. I.e. the inconsistency is just being moved\nfrom the server-side to the client-side.\n\nI'd think that in this case we'd very much want to give the client the\nbenefit of the doubt, because the server can more easily work around\nissues with its hook workflow.\n\nBut a client inherently can't work around not getting an \"OK & flush\"\nwhile waiting for the hook to execute, and in the meantime the cat\nunplugged the WiFi, so we won't be getting the \"OK\" at all.\n\nI.e. if put a \"sleep 30\" in a post-receive hook, push, and in the middle\nof that sleep have the client disconnect from the network the push will\nhave gone through.\n\nBut aren't we changing what gets shown to the client from being a\nsuccessful push to a non-successful one, since they never got the\nreport() (and therefore flush) they were expecting? *Goes and tests*\n\nYes, e.g. with this:\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 49b846d9605..fc273e7dc4d 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -2567,9 +2567,9 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)\n if (pack_lockfile)\n unlink_or_warn(pack_lockfile);\n if (report_status_v2)\n- report_v2(commands, unpack_status);\n+ exit(0);\n else if (report_status)\n- report(commands, unpack_status);\n+ exit(0);\n run_receive_hook(commands, \"post-receive\", 1,\n &push_options);\n run_update_post_hook(commands);\n\nI've made an attempt to emulate that, and running that we'll get various\ntest suite failures with e.g.:\n \n + git push dest HEAD\n Enumerating objects: 4, done.\n Counting objects: 100% (4/4), done.\n Delta compression using up to 8 threads\n Compressing objects: 100% (3/3), done.\n Writing objects: 100% (4/4), 1.25 KiB | 1.25 MiB/s, done.\n Total 4 (delta 0), reused 0 (delta 0), pack-reused 0\n send-pack: unexpected disconnect while reading sideband packet\n fatal: the remote end hung up unexpectedly\n error: last command exited with $?=128\n\nWhich is a race we'll definitely see now, but would increase in\nfrequency if we wait longer in sending the OK.\n\nBut as noted in [1] there's a way forward to having our cake & eating it\ntoo. I.e. when we into that on the server-side we can try a little\nharder not to die, and run post-receive anyway, and in either case it\nwould be nice if we'd run it after disconnecting from the client, so it\ndoesn't have to wait for it.\n\n1. https://lore.kernel.org/git/5795EB1C.1080102@nokia.com/\n2. https://lore.kernel.org/git/xmqqlh0d8w6v.fsf@gitster.mtv.corp.google.com/\n3. https://lore.kernel.org/git/20160803193018.ydhmxntikhyowmjz@sigill.intra.peff.net/\n"},{"id":"440616","messageId":"CFJ0O7HEP2TY.NSQD8094SYCF@diabtop","threadId":"56841","inReplyTo":"211106.86y261g88n.gmgdl@evledraar.gmail.com","subject":"Re: [RFC PATCH] receive-pack: run post-receive before reporting status","fromName":"Robin Jarry","fromEmail":"robin.jarry@6wind.com","sentAt":"2021-11-06T21:32:38Z","receivedAt":"2021-11-06T21:32:49Z","isPatch":true,"sender":{"key":"robin.jarry@6wind.com","avatar":null},"body":"Hi Ævar,\n\nÆvar Arnfjörð Bjarmason, Nov 06, 2021 at 06:03:\n> FWIW we include the date when mentioning commits. E.g. ec7dbd145bd\n> (receive-pack: allow hooks to ignore its standard input stream,\n> 2014-09-12).\n\nOK, noted.\n\n> I think the discussion at [1] is current to everything you're seeing\n> here.\n\nThanks I had missed this thread.\n\n> tl;dr: Even with this change you're not guaranteed to run your hook.\n[snip]\n> But as noted in [1] there's a way forward to having our cake & eating it\n> too. I.e. when we into that on the server-side we can try a little\n> harder not to die, and run post-receive anyway, and in either case it\n> would be nice if we'd run it after disconnecting from the client, so it\n> doesn't have to wait for it.\n\nOK, I will try to propose a v2 masking SIGPIPE when reporting status to\nthe client to avoid dying before running post-receive.\n\nCheers,\n\n-- \nRobin\n"},{"id":"440629","messageId":"20211106220358.144886-1-robin@jarry.cc","threadId":"56841","inReplyTo":"20211104133546.1967308-1-robin.jarry@6wind.com","subject":"[PATCH v2] receive-pack: ignore SIGPIPE while reporting status to client","fromName":"Robin Jarry","fromEmail":"robin@jarry.cc","sentAt":"2021-11-06T22:03:59Z","receivedAt":"2021-11-06T22:05:31Z","isPatch":true,"sender":{"key":"robin@jarry.cc","avatar":"https://avatars.githubusercontent.com/u/472286?v=4"},"body":"When a remote client exits while the pre-receive hook is running,\nreceive-pack is not killed by SIGPIPE because the signal is ignored.\nThis is a side effect of commit ec7dbd145bd8 (receive-pack: allow hooks\nto ignore its standard input stream, 2014-09-12).\n\nThe pre-receive hook itself is not interrupted and does not receive any\nerror since its stdout is a pipe which is read in an async thread and\noutput back to the client socket in a side band channel.\n\nAfter the pre-receive has exited the SIGPIPE default handler is restored\nand if the hook did not report any error, objects are migrated from\ntemporary to permanent storage.\n\nBefore running the post-receive hook, status info is reported back to\nthe client. Since the client has died, receive-pack is killed by SIGPIPE\nand post-receive is never executed.\n\nThe post-receive hook is often used to send email notifications (see\ncontrib/hooks/post-receive-email), update bug trackers, start automatic\nbuilds, etc. Not executing it after an interrupted yet \"successful\" push\ncan lead to inconsistencies.\n\nIgnore SIGPIPE before reporting status to the client to increase the\nchances of post-receive running if pre-receive was successful. This does\nnot guarantee 100% consistency but it should resist early disconnection\nby the client.\n\nSigned-off-by: Robin Jarry \n---\nIdeally, pre-receive should not be allowed to succeed if the client has\ndisconnected before objects have been migrated from temporary to\npermanent storage. But that is another topic, and I think it would\ncomplement this patch.\n\n builtin/receive-pack.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 49b846d96052..5fe7992028d4 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -2564,12 +2564,14 @@ 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\tsigchain_push(SIGPIPE, SIG_IGN);\n \t\tif (pack_lockfile)\n \t\t\tunlink_or_warn(pack_lockfile);\n \t\tif (report_status_v2)\n \t\t\treport_v2(commands, unpack_status);\n \t\telse if (report_status)\n \t\t\treport(commands, unpack_status);\n+\t\tsigchain_pop(SIGPIPE);\n \t\trun_receive_hook(commands, \"post-receive\", 1,\n \t\t\t\t &push_options);\n \t\trun_update_post_hook(commands);\n-- \n2.30.2\n\n"},{"id":"440766","messageId":"xmqqzgqd11dp.fsf@gitster.g","threadId":"56841","inReplyTo":"20211106220358.144886-1-robin@jarry.cc","subject":"Re: [PATCH v2] receive-pack: ignore SIGPIPE while reporting status to client","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-09T21:10:26Z","receivedAt":"2021-11-09T21:10:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Robin Jarry writes:\n\n> When a remote client exits while the pre-receive hook is running,\n> receive-pack is not killed by SIGPIPE because the signal is ignored.\n> This is a side effect of commit ec7dbd145bd8 (receive-pack: allow hooks\n> to ignore its standard input stream, 2014-09-12).\n>\n> The pre-receive hook itself is not interrupted and does not receive any\n> error since its stdout is a pipe which is read in an async thread and\n> output back to the client socket in a side band channel.\n>\n> After the pre-receive has exited the SIGPIPE default handler is restored\n> and if the hook did not report any error, objects are migrated from\n> temporary to permanent storage.\n\nAll of the above talks about the pre-receive hook, but it is unclear\nhow that is relevant to this change. The first paragraph says\n\"... is not killed\", and if that was a bad thing (in other words, it\nshould be killed but is not, and that is a bug worth fixing), and if\nthis patch changes the behaviour, then that paragraph is worth\nsaying. Similarly for the other two.\n\n> Before running the post-receive hook, status info is reported back to\n> the client. Since the client has died, receive-pack is killed by SIGPIPE\n> and post-receive is never executed.\n\nIn other words, regardless of what happens (or does not happen) to\nthe pre-receive hook, which may not even exist, if \"git push\" dies\nbefore the post-receive hook runs, this paragraph applies, no? \n\nWhat I am getting at is that this can (and should) be the first\nparagraph of the description without losing clarity.\n\n> Ignore SIGPIPE before reporting status to the client to increase the\n> chances of post-receive running if pre-receive was successful. This does\n> not guarantee 100% consistency but it should resist early disconnection\n> by the client.\n\nOK.\n\n> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\n> index 49b846d96052..5fe7992028d4 100644\n> --- a/builtin/receive-pack.c\n> +++ b/builtin/receive-pack.c\n> @@ -2564,12 +2564,14 @@ 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\tsigchain_push(SIGPIPE, SIG_IGN);\n> \t\tif (pack_lockfile)\n> \t\t\tunlink_or_warn(pack_lockfile);\n\nShouldn't we start ignoring SIGPIPE here, not before we try to\nunlink the lockfile?\n\n> \t\tif (report_status_v2)\n> \t\t\treport_v2(commands, unpack_status);\n> \t\telse if (report_status)\n> \t\t\treport(commands, unpack_status);\n> +\t\tsigchain_pop(SIGPIPE);\n\nIn other words, push/pop pair should surround the part that reports\nthe status, as the proposed commit log message said.\n\n> \t\trun_receive_hook(commands, \"post-receive\", 1,\n> \t\t\t\t &push_options);\n> \t\trun_update_post_hook(commands);\n\nOther than these, looks good to me.\n\nThanks.\n"},{"id":"440769","messageId":"CFLKOIVJ8EX0.2PWQ6PCXZ340A@diabtop","threadId":"56841","inReplyTo":"xmqqzgqd11dp.fsf@gitster.g","subject":"Re: [PATCH v2] receive-pack: ignore SIGPIPE while reporting status to client","fromName":"Robin Jarry","fromEmail":"robin@jarry.cc","sentAt":"2021-11-09T21:38:45Z","receivedAt":"2021-11-09T21:38:54Z","isPatch":true,"sender":{"key":"robin@jarry.cc","avatar":"https://avatars.githubusercontent.com/u/472286?v=4"},"body":"Hi Junio,\n\nJunio C Hamano, Nov 09, 2021 at 22:10:\n> All of the above talks about the pre-receive hook, but it is unclear\n> how that is relevant to this change. The first paragraph says\n> \"... is not killed\", and if that was a bad thing (in other words, it\n> should be killed but is not, and that is a bug worth fixing), and if\n> this patch changes the behaviour, then that paragraph is worth\n> saying. Similarly for the other two.\n>\n> > Before running the post-receive hook, status info is reported back to\n> > the client. Since the client has died, receive-pack is killed by SIGPIPE\n> > and post-receive is never executed.\n>\n> In other words, regardless of what happens (or does not happen) to\n> the pre-receive hook, which may not even exist, if \"git push\" dies\n> before the post-receive hook runs, this paragraph applies, no? \n>\n> What I am getting at is that this can (and should) be the first\n> paragraph of the description without losing clarity.\n\nYou're right. I wanted to give context to better explain why\nreceive-pack is not killed while running the pre-receive hook but\nafterwards. This should go into another commit which fixes that issue.\n\nI will reword accordingly.\n\n> > \t\texecute_commands(commands, unpack_status, &si,\n> > \t\t\t\t &push_options);\n> > +\t\tsigchain_push(SIGPIPE, SIG_IGN);\n> > \t\tif (pack_lockfile)\n> > \t\t\tunlink_or_warn(pack_lockfile);\n>\n> Shouldn't we start ignoring SIGPIPE here, not before we try to\n> unlink the lockfile?\n\nI initially wanted to avoid getting SIGPIPE'd while printing a warning\nif the lockfile cannot be unlinked. Maybe this means the repository\nintegrity is compromised and we are well beyond ensuring post-receive is\nexecuted or not. I do not know git internals well enough to be sure.\n\nWhat do you think?\n\n-- \nRobin\n"},{"id":"440775","messageId":"xmqq5yt10w5r.fsf@gitster.g","threadId":"56841","inReplyTo":"CFLKOIVJ8EX0.2PWQ6PCXZ340A@diabtop","subject":"Re: [PATCH v2] receive-pack: ignore SIGPIPE while reporting status to client","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-09T23:03:12Z","receivedAt":"2021-11-09T23:03:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Robin Jarry\" writes:\n\n>> > +\t\tsigchain_push(SIGPIPE, SIG_IGN);\n>> > \t\tif (pack_lockfile)\n>> > \t\t\tunlink_or_warn(pack_lockfile);\n>>\n>> Shouldn't we start ignoring SIGPIPE here, not before we try to\n>> unlink the lockfile?\n>\n> I initially wanted to avoid getting SIGPIPE'd while printing a warning\n> if the lockfile cannot be unlinked. Maybe this means the repository\n> integrity is compromised and we are well beyond ensuring post-receive is\n> executed or not. I do not know git internals well enough to be sure.\n>\n> What do you think?\n\nI think that push/pop pair should surround the part that reports the\nstatus, as the proposed commit log message said.\n\nThanks.\n\n"},{"id":"440820","messageId":"20211110092942.1648429-1-robin@jarry.cc","threadId":"56841","inReplyTo":"20211106220358.144886-1-robin@jarry.cc","subject":"[PATCH v3] receive-pack: ignore SIGPIPE while reporting status to client","fromName":"Robin Jarry","fromEmail":"robin@jarry.cc","sentAt":"2021-11-10T09:29:42Z","receivedAt":"2021-11-10T09:30:20Z","isPatch":true,"sender":{"key":"robin@jarry.cc","avatar":"https://avatars.githubusercontent.com/u/472286?v=4"},"body":"Before running the post-receive hook, status info is reported back to\nthe client. If a remote client exits before or during the status report,\nreceive-pack is killed by SIGPIPE and post-receive is never executed.\n\nThe post-receive hook is often used to send email notifications (see\ncontrib/hooks/post-receive-email), update bug trackers, start automatic\nbuilds, etc. Not executing it after an interrupted yet \"successful\" push\ncan lead to inconsistencies.\n\nIgnore SIGPIPE before reporting status to the client to increase the\nchances of post-receive running if pre-receive was successful. This does\nnot guarantee 100% consistency but it should resist early disconnection\nby the client.\n\nSigned-off-by: Robin Jarry \n---\nChanges since v2:\n\n* Updated commit log with more pertinent info.\n* Only ignore SIGPIPE while reporting status, *after* removing the lock\n file.\n\n builtin/receive-pack.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 49b846d96052..2f4a38adfe2c 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -2566,10 +2566,12 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)\n \t\t\t\t &push_options);\n \t\tif (pack_lockfile)\n \t\t\tunlink_or_warn(pack_lockfile);\n+\t\tsigchain_push(SIGPIPE, SIG_IGN);\n \t\tif (report_status_v2)\n \t\t\treport_v2(commands, unpack_status);\n \t\telse if (report_status)\n \t\t\treport(commands, unpack_status);\n+\t\tsigchain_pop(SIGPIPE);\n \t\trun_receive_hook(commands, \"post-receive\", 1,\n \t\t\t\t &push_options);\n \t\trun_update_post_hook(commands);\n-- \n2.34.0.rc2.2.gbcf7eca935e4\n\n"},{"id":"440823","messageId":"20211110103525.171066-1-robin.jarry@6wind.com","threadId":"56841","inReplyTo":"xmqqzgqd11dp.fsf@gitster.g","subject":"[RFC PATCH] receive-pack: interrupt pre-receive when client disconnects","fromName":"Robin Jarry","fromEmail":"robin.jarry@6wind.com","sentAt":"2021-11-10T10:35:25Z","receivedAt":"2021-11-10T10:35:47Z","isPatch":true,"sender":{"key":"robin.jarry@6wind.com","avatar":null},"body":"When hitting ctrl-c on the client while a remote pre-receive hook is\nrunning, receive-pack is not killed by SIGPIPE because the signal is\nignored. This is a side effect of commit ec7dbd145bd8 (\"receive-pack:\nallow hooks to ignore its standard input stream\").\n\nThe pre-receive hook itself is not interrupted and does not receive any\nerror since its stdout is a pipe which is read in an async thread and\noutput back to the client socket in a side band channel.\n\nAfter the pre-receive has exited the SIGPIPE default handler is restored\nand if the hook did not report any error, objects are migrated from\ntemporary to permanent storage.\n\nThis can be confusing for most people and may even be considered a bug.\nWhen receive-pack cannot forward pre-receive output to the client, do\nnot ignore the error and kill the hook process so that the push does not\ncomplete.\n\nSigned-off-by: Robin Jarry \n---\nNote that if pre-receive does not produce any output, any disconnection\nof the client will not cause the hook to be killed. This is not ideal\nbut as far as I can see, there is no way to check if the client is alive\nwithout writing in the side band channel. Maybe by sending keepalive\npackets before cleaning up. I am not comfortable enough with git\ninternals to be sure.\n\n builtin/receive-pack.c | 55 ++++++++++++++++++++++++++++++++++++------\n sideband.c | 31 +++++++++++++++++++++---\n sideband.h | 4 +++\n 3 files changed, 79 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 2f4a38adfe2c..5668b8273486 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -469,6 +469,7 @@ static int copy_to_sideband(int in, int out, void *arg)\n {\n \tchar data[128];\n \tint keepalive_active = 0;\n+\tstruct child_process *proc = arg;\n \n \tif (keepalive_in_sec <= 0)\n \t\tuse_keepalive = KEEPALIVE_NEVER;\n@@ -494,7 +495,11 @@ static int copy_to_sideband(int in, int out, void *arg)\n \t\t\t} else if (ret == 0) {\n \t\t\t\t/* no data; send a keepalive packet */\n \t\t\t\tstatic const char buf[] = \"0005\\1\";\n-\t\t\t\twrite_or_die(1, buf, sizeof(buf) - 1);\n+\t\t\t\tif (proc && proc->pid > 0) {\n+\t\t\t\t\tif (write_in_full(1, buf, sizeof(buf) - 1) < 0)\n+\t\t\t\t\t\tgoto error;\n+\t\t\t\t} else\n+\t\t\t\t\twrite_or_die(1, buf, sizeof(buf) - 1);\n \t\t\t\tcontinue;\n \t\t\t} /* else there is actual data to read */\n \t\t}\n@@ -512,8 +517,21 @@ static int copy_to_sideband(int in, int out, void *arg)\n \t\t\t\t * with it.\n \t\t\t\t */\n \t\t\t\tkeepalive_active = 1;\n-\t\t\t\tsend_sideband(1, 2, data, p - data, use_sideband);\n-\t\t\t\tsend_sideband(1, 2, p + 1, sz - (p - data + 1), use_sideband);\n+\t\t\t\tif (proc && proc->pid > 0) {\n+\t\t\t\t\tif (send_sideband2(1, 2, data, p - data,\n+\t\t\t\t\t\t\t use_sideband) < 0)\n+\t\t\t\t\t\tgoto error;\n+\t\t\t\t\tif (send_sideband2(1, 2, p + 1,\n+\t\t\t\t\t\t\t sz - (p - data + 1),\n+\t\t\t\t\t\t\t use_sideband) < 0)\n+\t\t\t\t\t\tgoto error;\n+\t\t\t\t} else {\n+\t\t\t\t\tsend_sideband(1, 2, data, p - data,\n+\t\t\t\t\t\t use_sideband);\n+\t\t\t\t\tsend_sideband(1, 2, p + 1,\n+\t\t\t\t\t\t sz - (p - data + 1),\n+\t\t\t\t\t\t use_sideband);\n+\t\t\t\t}\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t}\n@@ -522,10 +540,24 @@ static int copy_to_sideband(int in, int out, void *arg)\n \t\t * Either we're not looking for a NUL signal, or we didn't see\n \t\t * it yet; just pass along the data.\n \t\t */\n-\t\tsend_sideband(1, 2, data, sz, use_sideband);\n+\t\tif (proc && proc->pid > 0) {\n+\t\t\tif (send_sideband2(1, 2, data, sz, use_sideband) < 0)\n+\t\t\t\tgoto error;\n+\t\t} else\n+\t\t\tsend_sideband(1, 2, data, sz, use_sideband);\n \t}\n \tclose(in);\n \treturn 0;\n+error:\n+\tclose(in);\n+\tif (proc && proc->pid > 0) {\n+\t\t/*\n+\t\t * SIGPIPE would be more relevant but we want to make sure that\n+\t\t * the hook does not ignore the signal.\n+\t\t */\n+\t\tkill(proc->pid, SIGKILL);\n+\t}\n+\treturn -1;\n }\n \n static void hmac_hash(unsigned char *out,\n@@ -807,7 +839,8 @@ struct receive_hook_feed_state {\n };\n \n typedef int (*feed_fn)(void *, const char **, size_t *);\n-static int run_and_feed_hook(const char *hook_name, feed_fn feed,\n+static int run_and_feed_hook(const char *hook_name,\n+\t\t\t int isolate_sigpipe, feed_fn feed,\n \t\t\t struct receive_hook_feed_state *feed_state)\n {\n \tstruct child_process proc = CHILD_PROCESS_INIT;\n@@ -843,6 +876,10 @@ static int run_and_feed_hook(const char *hook_name, feed_fn feed,\n \tif (use_sideband) {\n \t\tmemset(&muxer, 0, sizeof(muxer));\n \t\tmuxer.proc = copy_to_sideband;\n+\t\tif (isolate_sigpipe)\n+\t\t\tmuxer.data = NULL;\n+\t\telse\n+\t\t\tmuxer.data = &proc;\n \t\tmuxer.in = -1;\n \t\tcode = start_async(&muxer);\n \t\tif (code)\n@@ -923,6 +960,7 @@ static int feed_receive_hook(void *state_, const char **bufp, size_t *sizep)\n static int run_receive_hook(struct command *commands,\n \t\t\t const char *hook_name,\n \t\t\t int skip_broken,\n+\t\t\t int isolate_sigpipe,\n \t\t\t const struct string_list *push_options)\n {\n \tstruct receive_hook_feed_state state;\n@@ -936,7 +974,8 @@ static int run_receive_hook(struct command *commands,\n \t\treturn 0;\n \tstate.cmd = commands;\n \tstate.push_options = push_options;\n-\tstatus = run_and_feed_hook(hook_name, feed_receive_hook, &state);\n+\tstatus = run_and_feed_hook(hook_name, isolate_sigpipe,\n+\t\t\t\t feed_receive_hook, &state);\n \tstrbuf_release(&state.buf);\n \treturn status;\n }\n@@ -1970,7 +2009,7 @@ static void execute_commands(struct command *commands,\n \t\t}\n \t}\n \n-\tif (run_receive_hook(commands, \"pre-receive\", 0, push_options)) {\n+\tif (run_receive_hook(commands, \"pre-receive\", 0, 0, push_options)) {\n \t\tfor (cmd = commands; cmd; cmd = cmd->next) {\n \t\t\tif (!cmd->error_string)\n \t\t\t\tcmd->error_string = \"pre-receive hook declined\";\n@@ -2572,7 +2611,7 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)\n \t\telse if (report_status)\n \t\t\treport(commands, unpack_status);\n \t\tsigchain_pop(SIGPIPE);\n-\t\trun_receive_hook(commands, \"post-receive\", 1,\n+\t\trun_receive_hook(commands, \"post-receive\", 1, 1,\n \t\t\t\t &push_options);\n \t\trun_update_post_hook(commands);\n \t\tstring_list_clear(&push_options, 0);\ndiff --git a/sideband.c b/sideband.c\nindex 85bddfdcd4f5..27f8d653eb24 100644\n--- a/sideband.c\n+++ b/sideband.c\n@@ -247,11 +247,25 @@ int demultiplex_sideband(const char *me, int status,\n \treturn 1;\n }\n \n+static int send_sideband_priv(int fd, int band, const char *data, ssize_t sz,\n+\t\t\t int packet_max, int ignore_errors);\n+\n /*\n * fd is connected to the remote side; send the sideband data\n * over multiplexed packet stream.\n */\n void send_sideband(int fd, int band, const char *data, ssize_t sz, int packet_max)\n+{\n+\t(void)send_sideband_priv(fd, band, data, sz, packet_max, 1);\n+}\n+\n+int send_sideband2(int fd, int band, const char *data, ssize_t sz, int packet_max)\n+{\n+\treturn send_sideband_priv(fd, band, data, sz, packet_max, 0);\n+}\n+\n+static int send_sideband_priv(int fd, int band, const char *data, ssize_t sz,\n+\t\t\t int packet_max, int ignore_errors)\n {\n \tconst char *p = data;\n \n@@ -265,13 +279,24 @@ void send_sideband(int fd, int band, const char *data, ssize_t sz, int packet_ma\n \t\tif (0 <= band) {\n \t\t\txsnprintf(hdr, sizeof(hdr), \"%04x\", n + 5);\n \t\t\thdr[4] = band;\n-\t\t\twrite_or_die(fd, hdr, 5);\n+\t\t\tif (ignore_errors)\n+\t\t\t\twrite_or_die(fd, hdr, 5);\n+\t\t\telse if (write_in_full(fd, hdr, 5) < 0)\n+\t\t\t\treturn -1;\n \t\t} else {\n \t\t\txsnprintf(hdr, sizeof(hdr), \"%04x\", n + 4);\n-\t\t\twrite_or_die(fd, hdr, 4);\n+\t\t\tif (ignore_errors)\n+\t\t\t\twrite_or_die(fd, hdr, 4);\n+\t\t\telse if (write_in_full(fd, hdr, 4) < 0)\n+\t\t\t\treturn -1;\n \t\t}\n-\t\twrite_or_die(fd, p, n);\n+\t\tif (ignore_errors)\n+\t\t\twrite_or_die(fd, p, n);\n+\t\telse if (write_in_full(fd, p, n) < 0)\n+\t\t\treturn -1;\n \t\tp += n;\n \t\tsz -= n;\n \t}\n+\n+\treturn 0;\n }\ndiff --git a/sideband.h b/sideband.h\nindex 5a25331be55d..cb92777418e1 100644\n--- a/sideband.h\n+++ b/sideband.h\n@@ -29,5 +29,9 @@ int demultiplex_sideband(const char *me, int status,\n \t\t\t enum sideband_type *sideband_type);\n \n void send_sideband(int fd, int band, const char *data, ssize_t sz, int packet_max);\n+/*\n+ * Do not die on write errors, return -1 instead.\n+ */\n+int send_sideband2(int fd, int band, const char *data, ssize_t sz, int packet_max);\n \n #endif\n-- \n2.30.2\n\n"},{"id":"441587","messageId":"CFSSYGFOUE5S.37W5QLMSUSV5T@diabtop","threadId":"56841","inReplyTo":"20211110092942.1648429-1-robin@jarry.cc","subject":"Re: [PATCH v3] receive-pack: ignore SIGPIPE while reporting status to client","fromName":"Robin Jarry","fromEmail":"robin@jarry.cc","sentAt":"2021-11-18T09:36:32Z","receivedAt":"2021-11-18T09:36:42Z","isPatch":true,"sender":{"key":"robin@jarry.cc","avatar":"https://avatars.githubusercontent.com/u/472286?v=4"},"body":"Hi,\n\ndid you get a chance to look at v3? Were there additional remarks?\n\nThanks.\n"},{"id":"445178","messageId":"CGRUPBBYR6GK.3FR53MJK5K0NT@diabtop","threadId":"56841","inReplyTo":"20211110103525.171066-1-robin.jarry@6wind.com","subject":"Re: [RFC PATCH] receive-pack: interrupt pre-receive when client disconnects","fromName":"Robin Jarry","fromEmail":"robin.jarry@6wind.com","sentAt":"2021-12-29T14:21:57Z","receivedAt":"2021-12-29T14:22:00Z","isPatch":true,"sender":{"key":"robin.jarry@6wind.com","avatar":null},"body":"Hi,\n\ndo you have any remarks on this patch? Is it going in the right\ndirection? Should I send a non-RFC?\n\nHappy holidays all.\n\nThanks!\n"}]}