Re: do people find t5504.8 flaky?

On Wed, Nov 13, 2019 at 01:07:47AM +0100, SZEDER Gábor wrote:

I was able to reproduce with that, too (I wonder why I had so much trouble before? Maybe "-r 1,8" simply gives many more opportunities per second).

Running with "-x", I don't see any sign of a premature SIGPIPE death, which is good:

  + git push --porcelain dst master:refs/heads/test
  remote: fatal: object of unexpected type        
  error: remote unpack failed: unpack-objects abnormal exit
  error: failed to push some refs to 'dst'
  + exit_code=1

The "exit_code" assignment there is from test_must_fail. And looking at the code, I think we handle a remote hangup well, because the writer is actually a pack-objects sub-process. So we hit this code in send-pack.c:

          rc = finish_command(&po);
          if (rc) {
                  /*
                   * For a normal non-zero exit, we assume pack-objects wrote
                   * something useful to stderr. For death by signal, though,
                   * we should mention it to the user. The exception is SIGPIPE
                   * (141), because that's a normal occurrence if the remote end
                   * hangs up (and we'll report that by trying to read the unpack
                   * status).
                   */
                  if (rc > 128 && rc != 141)
                          error("pack-objects died of signal %d", rc - 128);
                  return -1;
          }

But that error return might explain things.

The difference in your traces seems to be that in the failing case we exit immediately after seeing the unpack error:

Note that "receive-pack" sent us the "ng refs/heads/test" line. And our sideband demuxer even saw it! But it never made it to push.

Whereas in the successful case:

We do see "push" getting it.

So for some reason we're deciding not to read the status from out sideband demuxer. And that is probably because of this code in send_pack():

                  if (pack_objects(out, remote_refs, extra_have, args) < 0) {
                          for (ref = remote_refs; ref; ref = ref->next)
                                  ref->status = REF_STATUS_NONE;
                          if (args->stateless_rpc)
                                  close(out);
                          if (git_connection_is_socket(conn))
                                  shutdown(fd[0], SHUT_WR);
  
                          /*
                           * Do not even bother with the return value; we know we
                           * are failing, and just want the error() side effects.
                           */
                          if (status_report)
                                  receive_unpack_status(&reader);
  
                          if (use_sideband) {
                                  close(demux.out);
                                  finish_async(&demux);
                          }
                          fd[1] = -1;
                          return -1;
                  }

If we saw a pack-objects error, then we do bother to receive the unpack status, but we don't stick around to get the individual ref status, and everything stays as REF_STATUS_NONE.

And looking in transport_print_push_status(), NONE is skipped when printing. We probably ought to be seeding it with a different status there, but there's not really one for "hey, _we_ failed". Let's hack it to use REMOTE_REJECT, put put in our own message, like so: