'git push --atomic' doesn't report errors from our handlers

Gitlab-Shell is exiting with code 1 if the command errored. This may happen for example if one of Gitaly's RPC handlers return an error, for example for the receive-pack command. If the command failed, one would expect to have an error printed out.

With non-atomic pushes, Git prints out the error and exits with non-zero code as expected when gitlab-shell exits with non-zero code:

❯ git push origin main
Enumerating objects: 1, done.
Counting objects: 100% (1/1), done.
Writing objects: 100% (1/1), 181 bytes | 181.00 KiB/s, done.
Total 1 (delta 0), reused 0 (delta 0), pack-reused 0
To ssh://localhost:2222/root/test-project-1.git
   6b0a2ab..82e4baa  main -> main
error: failed to push some refs to 'ssh://localhost:2222/root/test-project-1.git'

However, the error doesn't get printed out when the push is done with --atomic, and git exits with code 0 indicating success. The below call has actually errored out even though the output doesn't show it in anyway:

❯ git push origin main --atomic
Enumerating objects: 1, done.
Counting objects: 100% (1/1), done.
Writing objects: 100% (1/1), 181 bytes | 181.00 KiB/s, done.
Total 1 (delta 0), reused 0 (delta 0), pack-reused 0
To ssh://localhost:2222/root/test-project-1.git
   6b0a2ab..82e4baa  main -> main

This appears to be a bug in git as it is explicitly ignoring the exit code of the command with --atomic. The commit that introduced this issue implies this is intentional, and the error should be checked elsewhere but the result is that the exit code is ignored and no error is displayed to the user.

This is confusing and the user would have no idea the operation has actually failed. If the status code of the error is not Internal, we'd still print out our own error message in gitlab-shell.

❯ git push origin main --atomic
Pushing to ssh://localhost:2222/root/test-project-1.git
Enumerating objects: 1, done.
Counting objects: 100% (1/1), done.
Writing objects: 100% (1/1), 181 bytes | 181.00 KiB/s, done.
Total 1 (delta 0), reused 0 (delta 0), pack-reused 0
remote: 
remote: ========================================================================
remote: 
remote: ERROR: handler error

remote: 
remote: ========================================================================
remote: 
To ssh://localhost:2222/root/test-project-1.git
   6b0a2ab..82e4baa  main -> main

The push with --atomic would still only display our custom error, not the final error line by Git and would exit with code 0 indicating success.

Here is a mailing list message with some more context about why we are ignoring errors coming from finish_connect() https://lore.kernel.org/git/xmqqlfmtvqkh.fsf@gitster.c.googlers.com/

/cc @jcaigitlab might be something we want to fix as the users are told an operation succeeded even if it didn't.

added bugfunctional group::gitalygit sectioncore platform typebug labels

added devopssystems label

changed the description

@samihiltunen does this have implications on the transaction manager or WAL work?

@jcaigitlab yes, and no. As things currently are, we'd report that changes have been persisted and error afterwards. As --atomic doesn't show the error, user would have no idea changes were not persisted as things currently are.

I believe that problem can also be solved by using proc-receive hook. proc-receive allows us to report correct status per reference so we wouldn't first say things have been persisted and error afterwards. We'd error before giving a status for any of the references.

The work on proc-receive has been scheduled for next release so the problem in the context of transactions might be addressed through there. The behavior of the push with --atomic is still wrong to report a success when there has been a failure though.

changed the description

mentioned in issue gitaly#5642 (closed)

mentioned in issue gitaly#5991 (closed)

added Category:Gitaly label

@samihiltunen Interesting, this is the first time I see this, and if true this would be something we should fix soonish indeed. I did give this a quick test and couldn't reproduce it though:

 /tmp $ git init repo
Initialized empty Git repository in /tmp/repo/.git/

 /tmp $ cd repo/

 /tmp/repo:HEAD $ git commit --allow-empty -mx
[main (root-commit) a4e1321] x

 /tmp/repo:main $ cd ..
 /tmp $ git clone repo/ --bare repo.git
Cloning into bare repository 'repo.git'...
done.

 /tmp $ cd repo

 /tmp/repo:main $ git remote add origin file:///tmp/repo.git/

 /tmp/repo:main $ git commit -amx --allow-empty
[main 72f21c2] x

 /tmp/repo:main $ git push --set-upstream origin main
Enumerating objects: 1, done.
Counting objects: 100% (1/1), done.
Writing objects: 100% (1/1), 179 bytes | 179.00 KiB/s, done.
Total 1 (delta 0), reused 0 (delta 0), pack-reused 0 (from 0)
To file:///tmp/repo.git/
   a4e1321..72f21c2  main -> main
branch 'main' set up to track 'origin/main'.

 /tmp/repo:main $ git commit --amend --allow-empty
[main be86511] xy
 Date: Tue Nov 12 08:39:32 2024 +0100

 /tmp/repo:main $ git push
To file:///tmp/repo.git/
 ! [rejected]        main -> main (non-fast-forward)
error: failed to push some refs to 'file:///tmp/repo.git/'
hint: Updates were rejected because the tip of your current branch is behind
hint: its remote counterpart. If you want to integrate the remote changes,
hint: use 'git pull' before pushing again.
hint: See the 'Note about fast-forwards' in 'git push --help' for details.

 /tmp/repo:main $ git push --atomic
error: atomic push failed for ref refs/heads/main. status: 2
fatal: the remote end hung up unexpectedly
To file:///tmp/repo.git/
 ! [rejected]        main -> main (non-fast-forward)
error: failed to push some refs to 'file:///tmp/repo.git/'
hint: Updates were rejected because the tip of your current branch is behind
hint: its remote counterpart. If you want to integrate the remote changes,
hint: use 'git pull' before pushing again.
hint: See the 'Note about fast-forwards' in 'git push --help' for details.

As you can see, both with --atomic and without it the push succeeds. Am I missing anything?

@pks-gitlab The test in the issue description was running with transactions. The reference updates performed in the snapshot by receive-pack succeeded but committing the transaction fails. That error was raised after receive-pack has already acknowledged the reference changes. The handler would return the error, and that would be relayed as a bad exit code which is then ignored.

This issue could be reproduced by for example unconditionally returning an error at the end of *ReceivePack handlers, or by wrapping the receive-pack process and returning a non-zero exit code.

@samihiltunen Huh, not sure I completely understand the issue then. I assume with transactions you mean the Gitaly-side transactions, not the Git-side ones, right? If so it sounds less like a Git-side issue but more like an issue with how Gitaly wields Git: once the Git-side transaction has been committedit is expected that Git will report success to the client.

But I feel like I'm still misunderstanding what the Git-side issue is. Is it possible to come up with a minimum reproducer that demonstrates the issue using Git, only?

I assume with transactions you mean the Gitaly-side transactions, not the Git-side ones, right?

@pks-gitlab Yes, talking about Gitaly's transactions in this context.

If so it sounds less like a Git-side issue but more like an issue with how Gitaly wields Git: once the Git-side transaction has been committedit is expected that Git will report success to the client.

Gitaly is nowadays using proc-receive hook with transactions. Reference updates are acknowledged only after the transaction is committed.

This issue pre-dates the proc-receive hook integration, and before that receive-pack acknowledged the reference updates after performing them in the snapshot but before the transaction committed.

But I feel like I'm still misunderstanding what the Git-side issue is.

The fact that the exit code of receive-pack is ignored with git push --atomic. The exit code is not ignored with git push --no-atomic.

Is it possible to come up with a minimum reproducer that demonstrates the issue using Git, only?

Wrapping the receive-pack process on the remote with a script that returns non-zero exit code is likely enough to reproduce without Gitaly.

The fact that the exit code of receive-pack is ignored with git push --atomic. The exit code is not ignored with git push --no-atomic.

@samihiltunen Okay, makes sense, I can reproduce the issue now. Thanks!

assigned to @pks-gitlab

changed milestone to %Git 2.48

mentioned in merge request !250 (closed)

changed milestone to %Git 2.49

added workflowin review label

I'm reviving this patch series after the responsible party upstream doesn't respond anymore.

Let me also connect it to Support Gitaly's write-ahead logging on the Git... (&16089), given that it seems to be relevant in the context of the WAL.

added gitlab-org#16089 as parent epic

Merged to next via 7d93e5ad (Merge branch 'ps/send-pack-unhide-error-in-atomic-push' into next, 2025-02-10). Closing.

closed

added workflowcomplete label and removed workflowin review label