{"thread":{"id":"42918","subject":"Client exit whilst running pre-receive hook : commit accepted but no post-receive hook ran","startedAt":"2016-07-25T10:45:15Z","lastAt":"2016-08-04T00:36:42Z","messageCount":6,"participants":["Jan Smets","Jeff King","Stephen Morton","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"292089","messageId":"5795EB1C.1080102@nokia.com","threadId":"42918","inReplyTo":null,"subject":"Client exit whilst running pre-receive hook : commit accepted but no post-receive hook ran","fromName":"Jan Smets","fromEmail":"jan.smets@nokia.com","sentAt":"2016-07-25T10:34:04Z","receivedAt":"2016-07-25T10:45:15Z","isPatch":false,"sender":{"key":"jan.smets@nokia.com","avatar":null},"body":"Hi\n\nI have always assumed the post-receive hook to be executed whenever a \ncommit is \"accepted\" by the (gitolite) server. That does not seem to be \ntrue any more.\n\nSince 9658846 is appears that, when a client bails out, the pre-receive \nhook continues to run and the commit is written to the repository, but \nno post-receive hook is executed. No signal of any kind is received in \nthe hook, not even a sig pipe when the post- hook is writing to stdout \nwhilst the client has disconnected.\n\n\ncommit 9658846ce3d379b9ff8010a2ed326fcafc10eb82\nAuthor: Jeff King \nDate: Wed Feb 24 02:40:16 2016 -0500\n\n write_or_die: handle EPIPE in async threads\n\ndiff --git a/write_or_die.c b/write_or_die.c\n...\n static void check_pipe(int err)\n {\n if (err == EPIPE) {\n+ if (in_async())\n+ async_exit(141);\n\n\n\nPlease keep me in CC as I am not subscribed to the list.\n\nThanks\nJan\n\n\n\nThe pre-receive hook from my quick testing => press Ctrl-C on the client \nwhen it is busy processing the 'sleep 5'\nIn my testing I was committing/pushing 32MB+ binary files that take some \ntime to process.\n\n#!/bin/bash\ntrap 'echo TRAP >> /tmp/gittest/log' 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 \n16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31\nIN=$(cat /dev/stdin)\n\necho -n $(date) >> /tmp/gittest/log\necho \" : PRE START\" >> /tmp/gittest/log\n\nfor i in $(seq 1 10); do\n echo This is the pre-receive hook $i; sleep 0.1\ndone\n\n# give time for client to ctrl-c out\nsleep 5\n\necho -n $(date) >> /tmp/gittest/log\necho \" : PRE END\" >> /tmp/gittest/log\n\n# This should result in a sigpipe? but it isn't.\necho \"Done !\"\necho \"Done !\"\n\n# no exit code -> accept commit\n\n\n\n"},{"id":"292159","messageId":"20160725222201.GC13589@sigill.intra.peff.net","threadId":"42918","inReplyTo":"5795EB1C.1080102@nokia.com","subject":"Re: Client exit whilst running pre-receive hook : commit accepted but no post-receive hook ran","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-07-25T22:22:02Z","receivedAt":"2016-07-25T22:22:15Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jul 25, 2016 at 12:34:04PM +0200, Jan Smets wrote:\n\n> I have always assumed the post-receive hook to be executed whenever a commit\n> is \"accepted\" by the (gitolite) server. That does not seem to be true any\n> more.\n\nGenerally, yeah, I would expect that to be the case, too.\n\n> Since 9658846 is appears that, when a client bails out, the pre-receive hook\n> continues to run and the commit is written to the repository, but no\n> post-receive hook is executed. No signal of any kind is received in the\n> hook, not even a sig pipe when the post- hook is writing to stdout whilst\n> the client has disconnected.\n\nI see. The problem is that cmd_receive_pack() does this:\n\n execute_commands(commands, unpack_status, &si);\n if (pack_lockfile)\n unlink_or_warn(pack_lockfile);\n if (report_status)\n report(commands, unpack_status);\n run_receive_hook(commands, \"post-receive\", 1);\n run_update_post_hook(commands);\n\nIt reports the status to the client, and _then_ runs the post-receive\nhook. But if that reporting fails (either because of an error, or if we\njust get SIGPIPE because the client hung up), then we don't actually run\nthe hooks.\n\nLeaving 9658846 out of it entirely, it is always going to be racy\nwhether we notice that the client hung up during the pre-receive step.\nE.g.:\n\n - your pre-receive might not write any output, so the muxer has\n nothing to write to the other side, and we never notice that the\n connection closed until we write the status out in report()\n\n - if NO_PTHREADS is set, the sideband muxer runs in a sub-process, not\n a sub-thread. And thus we don't know of a hangup except by checking\n the result of finish_async(), which we never do.\n\n - the client could hang up just _after_ we've written the pre-receive\n output, but before report() is called, so there's nothing to notice\n until we're in report()\n\nSo I think 9658846 just made that race a bit longer, because it means\nthat a write error in the sideband muxer during the pre-receive hook\nmeans we return an error via finish_async() rather than unceremoniously\ncalling exit() from a sub-thread. So we have a longer period during\nwhich we might actually finish off execute_commands() but not make it\nout of report().\n\nAnd the real solution is to make cmd_receive_pack() more robust, and try\nharder to run the hooks even when the client hangs up or we have some\nother reporting error (because getting data back to the user is only one\nuse of post-receive hooks; they are also used to queue jobs or do\nmaintenance).\n\nBut that's a bit tricky, as it requires report() to ignore SIGPIPE, and\nto stop using write_or_die() or any other functions that can exit (some\nof which happen at a lower level). Plus if a client does hangup, we\ndon't want our hook to die with SIGPIPE either, so we'd want to proxy\nthe data into /dev/null.\n\n-Peff\n"},{"id":"292842","messageId":"a3f64a09-3094-eee1-0050-9960f0674036@nokia.com","threadId":"42918","inReplyTo":"20160725222201.GC13589@sigill.intra.peff.net","subject":"Re: Client exit whilst running pre-receive hook : commit accepted but no post-receive hook ran","fromName":"Stephen Morton","fromEmail":"stephen.morton@nokia.com","sentAt":"2016-08-02T16:01:57Z","receivedAt":"2016-08-02T16:36:31Z","isPatch":false,"sender":{"key":"stephen.morton@nokia.com","avatar":"https://gravatar.com/avatar/710b2676975916f73774c03d77c48db73bfd4d6b29922de679d1f2612a6b0ba9?d=mp&s=160"},"body":"On 2016-07-25 6:22 PM, Jeff King wrote:\n> On Mon, Jul 25, 2016 at 12:34:04PM +0200, Jan Smets wrote:\n>\n>> I have always assumed the post-receive hook to be executed whenever a commit\n>> is \"accepted\" by the (gitolite) server. That does not seem to be true any\n>> more.\n> Generally, yeah, I would expect that to be the case, too.\n>\n>> Since 9658846 is appears that, when a client bails out, the pre-receive hook\n>> continues to run and the commit is written to the repository, but no\n>> post-receive hook is executed. No signal of any kind is received in the\n>> hook, not even a sig pipe when the post- hook is writing to stdout whilst\n>> the client has disconnected.\n> I see. The problem is that cmd_receive_pack() does this:\n>\n> execute_commands(commands, unpack_status, &si);\n> if (pack_lockfile)\n> unlink_or_warn(pack_lockfile);\n> if (report_status)\n> report(commands, unpack_status);\n> run_receive_hook(commands, \"post-receive\", 1);\n> run_update_post_hook(commands);\n>\n> It reports the status to the client, and _then_ runs the post-receive\n> hook. But if that reporting fails (either because of an error, or if we\n> just get SIGPIPE because the client hung up), then we don't actually run\n> the hooks.\n>\n> Leaving 9658846 out of it entirely, it is always going to be racy\n> whether we notice that the client hung up during the pre-receive step.\n> E.g.:\n>\n> - your pre-receive might not write any output, so the muxer has\n> nothing to write to the other side, and we never notice that the\n> connection closed until we write the status out in report()\n>\n> - if NO_PTHREADS is set, the sideband muxer runs in a sub-process, not\n> a sub-thread. And thus we don't know of a hangup except by checking\n> the result of finish_async(), which we never do.\n>\n> - the client could hang up just _after_ we've written the pre-receive\n> output, but before report() is called, so there's nothing to notice\n> until we're in report()\n>\n> So I think 9658846 just made that race a bit longer, because it means\n> that a write error in the sideband muxer during the pre-receive hook\n> means we return an error via finish_async() rather than unceremoniously\n> calling exit() from a sub-thread. So we have a longer period during\n> which we might actually finish off execute_commands() but not make it\n> out of report().\n>\n> And the real solution is to make cmd_receive_pack() more robust, and try\n> harder to run the hooks even when the client hangs up or we have some\n> other reporting error (because getting data back to the user is only one\n> use of post-receive hooks; they are also used to queue jobs or do\n> maintenance).\n>\n> But that's a bit tricky, as it requires report() to ignore SIGPIPE, and\n> to stop using write_or_die() or any other functions that can exit (some\n> of which happen at a lower level). Plus if a client does hangup, we\n> don't want our hook to die with SIGPIPE either, so we'd want to proxy\n> the data into /dev/null.\n>\n> -Peff\n\nSounds tricky. I do think it's important, almost a 'data integrity' \nissue, that IF a commit is received, THEN the post-receive hook must be \nrun. Too much mission-critical stuff is based on post-receive hooks.\n\nThe alternatives, as I see them --either document that the post-receive \nhook cannot be fully trusted and that all such uses must change to \nasynchronous polling, or otherwise just say that nobody should hit \nCtrl-C during a push (not even reflexively when their lizard-brain says \n\"Woops, no!\") and hope that network issues don't cause the same thing-- \nare simply not realistic.\n\nStephen\n\n\n\n"},{"id":"292969","messageId":"20160803193018.ydhmxntikhyowmjz@sigill.intra.peff.net","threadId":"42918","inReplyTo":"a3f64a09-3094-eee1-0050-9960f0674036@nokia.com","subject":"Re: Client exit whilst running pre-receive hook : commit accepted but no post-receive hook ran","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-08-03T19:30:18Z","receivedAt":"2016-08-03T19:30:37Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 02, 2016 at 12:01:57PM -0400, Stephen Morton wrote:\n\n> Sounds tricky. I do think it's important, almost a 'data integrity' issue,\n> that IF a commit is received, THEN the post-receive hook must be run. Too\n> much mission-critical stuff is based on post-receive hooks.\n\nI agree it would be a good property to have. I think it's hard to do\natomically, though. Certainly we can wait to tell the other side \"your\npush has been recorded\" until after the hook is run. But we would\nalready have updated the refs locally at that point (and we must -- that\nis part of the interface to the post-receive hooks, that the refs are\nalready in place). So would we roll-back the ref update then? Even that\nsuffers from power failures, etc.\n\nSo I'm not sure if making it truly atomic is all the feasible. However,\nwe could certainly make things more robust than they are now.\n\nThe simplest thing may be to just bump the post-receive hook before the\nstatus report. That opens up the question of whether clients are\nactually waiting already for the post-receive to finish. Looking at the\ncode in send-pack, it looks like the network is hooked up to the\nsideband demuxer thread, which will read until EOF on the network. So we\nare waiting either way for the post-receive to run. It doesn't really\nmatter if it happens before or after the report to the client.\n\nSo I _think_ something like this would work:\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 15c323a..91d01f0 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -1767,9 +1767,9 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)\n \t\texecute_commands(commands, unpack_status, &si);\n \t\tif (pack_lockfile)\n \t\t\tunlink_or_warn(pack_lockfile);\n+\t\trun_receive_hook(commands, \"post-receive\", 1);\n \t\tif (report_status)\n \t\t\treport(commands, unpack_status);\n-\t\trun_receive_hook(commands, \"post-receive\", 1);\n \t\trun_update_post_hook(commands);\n \t\tif (auto_gc) {\n \t\t\tconst char *argv_gc_auto[] = {\n\nbut maybe there are other gotchas.\n\n-Peff\n"},{"id":"292972","messageId":"xmqqlh0d8w6v.fsf@gitster.mtv.corp.google.com","threadId":"42918","inReplyTo":"20160803193018.ydhmxntikhyowmjz@sigill.intra.peff.net","subject":"Re: Client exit whilst running pre-receive hook : commit accepted but no post-receive hook ran","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-03T19:54:48Z","receivedAt":"2016-08-03T20:00:40Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I agree it would be a good property to have. I think it's hard to do\n> atomically, though. Certainly we can wait to tell the other side \"your\n> push has been recorded\" until after the hook is run. But we would\n> already have updated the refs locally at that point (and we must -- that\n> is part of the interface to the post-receive hooks, that the refs are\n> already in place). So would we roll-back the ref update then? Even that\n> suffers from power failures, etc.\n>\n> So I'm not sure if making it truly atomic is all the feasible.\n\nAs long as the requirement is that post- hook must see the updated\nref in place, I do not think it is feasible to give \"the hook always\nruns once\" guarantee, without cooperation by other parts of the flow\n(think: pulling the power at an arbitrary point in the process).\n\nA receiving repository can implement it all in the userland, I would\nthink, though:\n\n * A pre-receive hook records the intention to update a ref (from\n what old commit to what new commit), and does not return until\n that record is securely in a database;\n\n * A post-receive hook checks the entry in the database above (it\n _must_ find one), and atomically does its thing and marks the\n entry \"done\";\n\n * A separate sweeper scans the database for entries not yet marked\n as \"done\", sees if the ref has been already updated, and\n atomically does its thing and marks the entry \"done\" (the same\n can be done as part of a post-receive for previously pushed\n commit that pre-receive recorded but did not manage to run\n post-receive before the power was pulled or the user did \\C-c).\n\nAs you originally described, the non-atomicity is not new; as long\nas we have necessary hooks in place on the git-core side for\nrepositories that want a stronger guarantee, I do not think there is\nany more thing we need to do on this topic. If we can narrow the\nwindow in a non-intrusive way, that would be a good thing to do,\nthough.\n\n> However,\n> we could certainly make things more robust than they are now.\n\nAnd this change may to the \"narrowing the window in a non-intrusive\nway\" (I wonder if we also need to lift the post-update hook the same\nway, though).\n\nBut that would still not guarantee \"the hook always runs once\".\nWhat we have is \"the hook runs at most once\".\n\nThanks.\n\n> The simplest thing may be to just bump the post-receive hook before the\n> status report. That opens up the question of whether clients are\n> actually waiting already for the post-receive to finish. Looking at the\n> code in send-pack, it looks like the network is hooked up to the\n> sideband demuxer thread, which will read until EOF on the network. So we\n> are waiting either way for the post-receive to run. It doesn't really\n> matter if it happens before or after the report to the client.\n>\n> So I _think_ something like this would work:\n>\n> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\n> index 15c323a..91d01f0 100644\n> --- a/builtin/receive-pack.c\n> +++ b/builtin/receive-pack.c\n> @@ -1767,9 +1767,9 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)\n> \t\texecute_commands(commands, unpack_status, &si);\n> \t\tif (pack_lockfile)\n> \t\t\tunlink_or_warn(pack_lockfile);\n> +\t\trun_receive_hook(commands, \"post-receive\", 1);\n> \t\tif (report_status)\n> \t\t\treport(commands, unpack_status);\n> -\t\trun_receive_hook(commands, \"post-receive\", 1);\n> \t\trun_update_post_hook(commands);\n> \t\tif (auto_gc) {\n> \t\t\tconst char *argv_gc_auto[] = {\n>\n> but maybe there are other gotchas.\n>\n> -Peff\n"},{"id":"293054","messageId":"3f8fa2e0-9604-2bc9-338e-9977ea23135d@nokia.com","threadId":"42918","inReplyTo":"xmqqlh0d8w6v.fsf@gitster.mtv.corp.google.com","subject":"Re: Client exit whilst running pre-receive hook : commit accepted but no post-receive hook ran","fromName":"Stephen Morton","fromEmail":"stephen.morton@nokia.com","sentAt":"2016-08-04T00:35:01Z","receivedAt":"2016-08-04T00:36:42Z","isPatch":false,"sender":{"key":"stephen.morton@nokia.com","avatar":"https://gravatar.com/avatar/710b2676975916f73774c03d77c48db73bfd4d6b29922de679d1f2612a6b0ba9?d=mp&s=160"},"body":"On 2016-08-03 3:54 PM, Junio C Hamano wrote:\n> Jeff King writes:\n>\n>> I agree it would be a good property to have. I think it's hard to do\n>> atomically, though. Certainly we can wait to tell the other side \"your\n>> push has been recorded\" until after the hook is run. But we would\n>> already have updated the refs locally at that point (and we must -- that\n>> is part of the interface to the post-receive hooks, that the refs are\n>> already in place). So would we roll-back the ref update then? Even that\n>> suffers from power failures, etc.\n>>\n>> So I'm not sure if making it truly atomic is all the feasible.\n> As long as the requirement is that post- hook must see the updated\n> ref in place, I do not think it is feasible to give \"the hook always\n> runs once\" guarantee, without cooperation by other parts of the flow\n> (think: pulling the power at an arbitrary point in the process).\n>\n> A receiving repository can implement it all in the userland, I would\n> think, though:\n>\n> * A pre-receive hook records the intention to update a ref (from\n> what old commit to what new commit), and does not return until\n> that record is securely in a database;\n>\n> * A post-receive hook checks the entry in the database above (it\n> _must_ find one), and atomically does its thing and marks the\n> entry \"done\";\n>\n> * A separate sweeper scans the database for entries not yet marked\n> as \"done\", sees if the ref has been already updated, and\n> atomically does its thing and marks the entry \"done\" (the same\n> can be done as part of a post-receive for previously pushed\n> commit that pre-receive recorded but did not manage to run\n> post-receive before the power was pulled or the user did \\C-c).\n>\n> As you originally described, the non-atomicity is not new; as long\n> as we have necessary hooks in place on the git-core side for\n> repositories that want a stronger guarantee, I do not think there is\n> any more thing we need to do on this topic. If we can narrow the\n> window in a non-intrusive way, that would be a good thing to do,\n> though.\n> \n\nI certainly understand not being able to make it atomic when faced with \nsay \"pulling the power at an arbitrary point in the process\". That seems \nto me almost along the lines of disaster recovery contingency plans. But \ncould we not guarantee that if there is no problem on the receiving end, \nthat \"IF a commit is received and the ref updated, THEN the post-receive \nhook is guaranteed to run\".\n\nThe not-so-uncommon situation where I saw this was where a user had \nsecond-thoughts and hit Ctrl-C in the middle of a push. The push went \nthrough --the ref was updated-- but the post-receive hooks were not run.\n\nSteve\n\n\n\n"}]}