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

[Jeff King <peff@peff.net>]

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