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

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 3, 2016, 19:54 UTC
Message-ID: <xmqqlh0d8w6v.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20160803193018.ydhmxntikhyowmjz@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 9 quoted lines

> I agree it would be a good property to have. I think it's hard to do
> atomically, though. Certainly we can wait to tell the other side "your
> push has been recorded" until after the hook is run. But we would
> already have updated the refs locally at that point (and we must -- that
> is part of the interface to the post-receive hooks, that the refs are
> already in place). So would we roll-back the ref update then? Even that
> suffers from power failures, etc.
>
> So I'm not sure if making it truly atomic is all the feasible.

As long as the requirement is that post- hook must see the updated ref in place, I do not think it is feasible to give "the hook always runs once" guarantee, without cooperation by other parts of the flow (think: pulling the power at an arbitrary point in the process).

A receiving repository can implement it all in the userland, I would think, though:

 * A pre-receive hook records the intention to update a ref (from
   what old commit to what new commit), and does not return until
   that record is securely in a database;

 * A post-receive hook checks the entry in the database above (it
   _must_ find one), and atomically does its thing and marks the
   entry "done";

 * A separate sweeper scans the database for entries not yet marked
   as "done", sees if the ref has been already updated, and
   atomically does its thing and marks the entry "done" (the same
   can be done as part of a post-receive for previously pushed
   commit that pre-receive recorded but did not manage to run
   post-receive before the power was pulled or the user did \C-c).

As you originally described, the non-atomicity is not new; as long as we have necessary hooks in place on the git-core side for repositories that want a stronger guarantee, I do not think there is any more thing we need to do on this topic. If we can narrow the window in a non-intrusive way, that would be a good thing to do, though.

> However,
> we could certainly make things more robust than they are now.

And this change may to the "narrowing the window in a non-intrusive way" (I wonder if we also need to lift the post-update hook the same way, though).

But that would still not guarantee "the hook always runs once". What we have is "the hook runs at most once".

Thanks.

Show 29 quoted lines

> The simplest thing may be to just bump the post-receive hook before the
> status report. That opens up the question of whether clients are
> actually waiting already for the post-receive to finish. Looking at the
> code in send-pack, it looks like the network is hooked up to the
> sideband demuxer thread, which will read until EOF on the network. So we
> are waiting either way for the post-receive to run. It doesn't really
> matter if it happens before or after the report to the client.
>
> So I _think_ something like this would work:
>
> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index 15c323a..91d01f0 100644
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -1767,9 +1767,9 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)
>  		execute_commands(commands, unpack_status, &si);
>  		if (pack_lockfile)
>  			unlink_or_warn(pack_lockfile);
> +		run_receive_hook(commands, "post-receive", 1);
>  		if (report_status)
>  			report(commands, unpack_status);
> -		run_receive_hook(commands, "post-receive", 1);
>  		run_update_post_hook(commands);
>  		if (auto_gc) {
>  			const char *argv_gc_auto[] = {
>
> but maybe there are other gotchas.
>
> -Peff