Re: Help needed on 2.54.0-rc0 t5301.13 looping.

[Jeff King <peff@peff.net>]

On Tue, Apr 07, 2026 at 07:37:49PM -0400, rsbecker@nexbridge.com wrote:

> Weird fail on t5401.13. Any opinions or advise on this?
> 
> expecting success of 5401.13 'pre-receive hook that forgets to read its
> input':
>         test_hook --clobber -C victim.git pre-receive <<-\EOF &&
>         exit 0
>         EOF
>         rm -f victim.git/hooks/update victim.git/hooks/post-update &&
> 
>         test_seq -f "create refs/heads/branch_%d main" 100 999 |
>         git update-ref --stdin &&
>         git push ./victim.git "+refs/heads/*:refs/heads/*"

OK, so this test is trying to feed a bunch of data to a pre-receive hook
that doesn't read anything, and we want to make sure we aren't killed by
SIGPIPE.

When the test was added originally in ec7dbd145b (receive-pack: allow
hooks to ignore its standard input stream, 2014-09-12), we just stuck a:

  sigchain_push(SIGPIPE, SIG_IGN);

at the top of the hook function. But now that function has been
rewritten to use the hook API, and that sigchain_push() is gone.

What does the new hook API do? It works with run-command's
run_processes_parallel() function. Which similarly ignores SIGPIPE,
courtesy of ec0becacc9 (run-command: add stdin callback for
parallelization, 2026-01-28):

          /*
           * Child tasks might receive input via stdin, terminating early (or not), so
           * ignore the default SIGPIPE which gets handled by each feed_pipe_fn which
           * actually writes the data to children stdin fds.
           */
          sigchain_push(SIGPIPE, SIG_IGN);

OK, so far so good. But that's not quite the end of the story. In
pp_init(), we then sigchain_push() another handler, but this time it's a
real function:

  static void handle_children_on_signal(int signo)
  {
          kill_children_signal(pp_for_signal, signo);
          sigchain_pop(signo);
          raise(signo);
  }

So now when we get a SIGPIPE, we end up there. It tries to propagate the
signal to any child processes we spawned. Which is a bit funny, since it
was the child process closing that caused us to get the signal in the
first place, but of course our handler doesn't know that.

And then afterwards, it pops itself off the handler stack and re-raises.
But that will hit the SIG_IGN we pushed earlier and do nothing. So that
part isn't interesting.

What is interesting is that we end up calling "kill(<pid>, SIGPIPE)" on
the child via kill_children_signal(). At least on my Linux system that
works fine, since we haven't reaped the process via wait() yet. But of
course nothing interesting happens to the child, which has already
exited. Killing it with SIGPIPE does not seem to affect its exit code.

However, I could believe that on some other system it might behave
differently. Possibly even racily. For example, if the child process had
closed its pipe (giving us SIGPIPE in the parent) but not yet fully
exited, could our kill() cause it to change its exit code? And then it
would look like the hook reported failure (because it died by signal).
And I'd expect the output you saw.

I think the root of the issue is that we should not be trying to
propagate SIGPIPE to the child in this case at all. Our handler is
pushed there only because it's part of sigchain_push_common(), which is
sensible: in general if we are dying to SIGPIPE we want to do our
cleanup. It's just funny in this case with the ordering of our SIG_IGN,
because now that SIG_IGN isn't on top of the stack anymore.

I.e., I think we want to reorder like this:

diff --git a/run-command.c b/run-command.c
index 32c290ee6a..8a95f7ff1e 100644
--- a/run-command.c
+++ b/run-command.c
@@ -1895,14 +1895,19 @@ void run_processes_parallel(const struct run_process_parallel_opts *opts)
 					   "max:%"PRIuMAX,
 					   (uintmax_t)opts->processes);
 
+	pp_init(&pp, opts, &pp_sig);
+
 	/*
 	 * Child tasks might receive input via stdin, terminating early (or not), so
 	 * ignore the default SIGPIPE which gets handled by each feed_pipe_fn which
 	 * actually writes the data to children stdin fds.
+	 *
+	 * This _must_ come after pp_init(), because it installs its own
+	 * SIGPIPE handler (to cleanup children), and we want to supersede
+	 * that.
 	 */
 	sigchain_push(SIGPIPE, SIG_IGN);
 
-	pp_init(&pp, opts, &pp_sig);
 	while (1) {
 		for (i = 0;
 		    i < spawn_cap && !pp.shutdown &&

Does that make your problem go away?

I suspect we could construct a related case that does fail on Linux
without the patch above. Imagine we actually have two hooks running in
parallel. The first one is fast and does not read its input, and the
second one is slow. We'll get SIGPIPE writing to the first one, and then
kill _both_ children. But that's wrong! There is no reason to kill the
second hook, as our intent was to ignore SIGPIPE.

-Peff