From: Stephen Morton Date: Tue, 02 Aug 2016 16:01:57 GMT Subject: Re: Client exit whilst running pre-receive hook : commit accepted but no post-receive hook ran Message-ID: In-Reply-To: <20160725222201.GC13589@sigill.intra.peff.net> On 2016-07-25 6:22 PM, Jeff King wrote: > On Mon, Jul 25, 2016 at 12:34:04PM +0200, Jan Smets wrote: > >> I have always assumed the post-receive hook to be executed whenever a commit >> is "accepted" by the (gitolite) server. That does not seem to be true any >> more. > Generally, yeah, I would expect that to be the case, too. > >> Since 9658846 is appears that, when a client bails out, the pre-receive hook >> continues to run and the commit is written to the repository, but no >> post-receive hook is executed. No signal of any kind is received in the >> hook, not even a sig pipe when the post- hook is writing to stdout whilst >> the client has disconnected. > I see. The problem is that cmd_receive_pack() does this: > > execute_commands(commands, unpack_status, &si); > if (pack_lockfile) > unlink_or_warn(pack_lockfile); > if (report_status) > report(commands, unpack_status); > run_receive_hook(commands, "post-receive", 1); > run_update_post_hook(commands); > > It reports the status to the client, and _then_ runs the post-receive > hook. But if that reporting fails (either because of an error, or if we > just get SIGPIPE because the client hung up), then we don't actually run > the hooks. > > Leaving 9658846 out of it entirely, it is always going to be racy > whether we notice that the client hung up during the pre-receive step. > E.g.: > > - your pre-receive might not write any output, so the muxer has > nothing to write to the other side, and we never notice that the > connection closed until we write the status out in report() > > - if NO_PTHREADS is set, the sideband muxer runs in a sub-process, not > a sub-thread. And thus we don't know of a hangup except by checking > the result of finish_async(), which we never do. > > - the client could hang up just _after_ we've written the pre-receive > output, but before report() is called, so there's nothing to notice > until we're in report() > > So I think 9658846 just made that race a bit longer, because it means > that a write error in the sideband muxer during the pre-receive hook > means we return an error via finish_async() rather than unceremoniously > calling exit() from a sub-thread. So we have a longer period during > which we might actually finish off execute_commands() but not make it > out of report(). > > And the real solution is to make cmd_receive_pack() more robust, and try > harder to run the hooks even when the client hangs up or we have some > other reporting error (because getting data back to the user is only one > use of post-receive hooks; they are also used to queue jobs or do > maintenance). > > But that's a bit tricky, as it requires report() to ignore SIGPIPE, and > to stop using write_or_die() or any other functions that can exit (some > of which happen at a lower level). Plus if a client does hangup, we > don't want our hook to die with SIGPIPE either, so we'd want to proxy > the data into /dev/null. > > -Peff Sounds tricky. I do think it's important, almost a 'data integrity' issue, that IF a commit is received, THEN the post-receive hook must be run. Too much mission-critical stuff is based on post-receive hooks. The alternatives, as I see them --either document that the post-receive hook cannot be fully trusted and that all such uses must change to asynchronous polling, or otherwise just say that nobody should hit Ctrl-C during a push (not even reflexively when their lizard-brain says "Woops, no!") and hope that network issues don't cause the same thing-- are simply not realistic. Stephen