Re: Client exit whilst running pre-receive hook : commit accepted but no post-receive hook ran

> 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