Return the same error from Updater after closing

Updater's error handling is somewhat complicated as the actual
update-ref command may error at any point between the calls. Some
of the error conditions, like doing an invalid reference update, may
only yield an error on the next method call as the update-ref process
executes the commands asynchronously. This has then led to somewhat
complicated error handling code which can currently return different
errors from different methods. For example, context being canceled
may not return a context canceled error but may actually lead to
returning an invalid state transition error.

This commit tries to clean the error handling up by centralizing it
in the Close method. The rules are as follows:

1. If stderr contains an error we recognize, return the spcific error.
2. If the command failed but we don't recognize the error, return the
   error from the command and add the stderr as metadata so the reason
   will be clear in logs.
3. If the command didn't error but our logic did, return our error.

Once Close has been called, the error returned by the call will be
returned from all methods. This will ensure for example that context
cancellation errors are returned from every method call, not just the
ones that check for it.

3 files changed, 191 insertions, 77 deletions

diff --git a/internal/git/updateref/updateref.go b/internal/git/updateref/updateref.go
index aab4fddc4..dbeb1bc49 100644
--- a/internal/git/updateref/updateref.go
+++ b/internal/git/updateref/updateref.go
@@ -13,6 +13,9 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v16/internal/structerr"
 )
 
+// errClosed is returned when accessing an updater that has already been closed.
+var errClosed = errors.New("closed")
+
 // AlreadyLockedError indicates a reference cannot be locked because another
 // process has already locked it.
 type AlreadyLockedError struct {
@@ -176,8 +179,6 @@ const (
 	// accepts reference changes until the current transaction is committed and
 	// a new one started.
 	statePrepared state = "prepared"
-	// stateClosed means the updater has been closed and is no longer usable.
-	stateClosed state = "closed"
 )
 
 // invalidStateTransitionError is returned when the updater is used incorrectly.
@@ -209,6 +210,7 @@ func (err invalidStateTransitionError) Error() string {
 type Updater struct {
 	repo       git.RepositoryExecutor
 	cmd        *command.Command
+	closeErr   error
 	stdout     *bufio.Reader
 	stderr     *bytes.Buffer
 	objectHash git.ObjectHash
@@ -294,9 +296,12 @@ func New(ctx context.Context, repo git.RepositoryExecutor, opts ...UpdaterOpt) (
 
 // expectState returns an error and closes the updater if it is not in the expected state.
 func (u *Updater) expectState(expected state) error {
+	if u.closeErr != nil {
+		return u.closeErr
+	}
+
 	if err := u.checkState(expected); err != nil {
-		_ = u.Close()
-		return err
+		return u.closeWithError(err)
 	}
 
 	return nil
@@ -387,17 +392,44 @@ func (u *Updater) Commit() error {
 // Close closes the updater and aborts a possible open transaction. No changes will be written
 // to disk, all lockfiles will be cleaned up and the process will exit.
 func (u *Updater) Close() error {
-	u.state = stateClosed
+	return u.closeWithError(nil)
+}
+
+// closeWithError closes the updater with the given error. The passed in error is only used
+// if the updater closes successfully. This is used to close the Updater with errors raised
+// by our logic when the command itself hasn't errored. All subsequent method calls return
+// the error returned from first closeWithError call.
+func (u *Updater) closeWithError(closeErr error) error {
+	if u.closeErr != nil {
+		return u.closeErr
+	}
 
 	if err := u.cmd.Wait(); err != nil {
-		return fmt.Errorf("closing updater: %w", err)
+		err = structerr.New("%w", err).WithMetadataItems(
+			structerr.MetadataItem{Key: "stderr", Value: u.stderr.String()},
+			structerr.MetadataItem{Key: "close_error", Value: closeErr},
+		)
+		if parsedErr := u.parseStderr(); parsedErr != nil {
+			// If stderr contained a specific error, return it instead.
+			err = parsedErr
+		}
+
+		u.closeErr = err
+		return err
 	}
+
+	if closeErr != nil {
+		u.closeErr = closeErr
+		return closeErr
+	}
+
+	u.closeErr = errClosed
 	return nil
 }
 
 func (u *Updater) write(format string, args ...interface{}) error {
 	if _, err := fmt.Fprintf(u.cmd, format, args...); err != nil {
-		return u.handleIOError(err)
+		return u.closeWithError(err)
 	}
 
 	return nil
@@ -425,37 +457,17 @@ func (u *Updater) setState(state string) error {
 	// raised.
 	line, err := u.stdout.ReadString('\n')
 	if err != nil {
-		return u.handleIOError(fmt.Errorf("state update to %q failed: %w", state, err))
+		return u.closeWithError(fmt.Errorf("state update to %q failed: %w", state, err))
 	}
 
 	if line != fmt.Sprintf("%s: ok\n", state) {
-		_ = u.Close()
-		return fmt.Errorf("state update to %q not successful: expected ok, got %q", state, line)
+		return u.closeWithError(fmt.Errorf("state update to %q not successful: expected ok, got %q", state, line))
 	}
 
 	return nil
 }
 
-// handleIOError handles errors after reading from or writing to git-update-ref(1) has failed.
-// It makes sure to properly tear down the process so that the stderr gets synchronized and handles
-// well-known errors. If the error message is not a well-known error then this function returns the
-// fallback error provided by the caller.
-func (u *Updater) handleIOError(fallbackErr error) error {
-	// We need to explicitly cancel the command here and wait for it to terminate such that we
-	// can retrieve the command's stderr in a race-free manner.
-	//
-	// Furthermore, if I/O has failed because we cancelled the process then we don't want to
-	// return a converted error, but instead want to return the actual context cancellation
-	// error.
-	if err := u.Close(); err != nil {
-		switch {
-		case errors.Is(err, context.Canceled):
-			return err
-		case errors.Is(err, context.DeadlineExceeded):
-			return err
-		}
-	}
-
+func (u *Updater) parseStderr() error {
 	stderr := u.stderr.Bytes()
 
 	matches := refLockedRegex.FindSubmatch(stderr)
@@ -509,5 +521,5 @@ func (u *Updater) handleIOError(fallbackErr error) error {
 		}
 	}
 
-	return structerr.New("%w", fallbackErr).WithMetadata("stderr", string(stderr))
+	return nil
 }
diff --git a/internal/git/updateref/updateref_test.go b/internal/git/updateref/updateref_test.go
index 0a409acc2..9e15bd524 100644
--- a/internal/git/updateref/updateref_test.go
+++ b/internal/git/updateref/updateref_test.go
@@ -3,8 +3,9 @@ package updateref
 import (
 	"context"
 	"encoding/hex"
+	"errors"
 	"fmt"
-	"io"
+	"io/fs"
 	"os"
 	"path/filepath"
 	"testing"
@@ -143,7 +144,9 @@ func TestUpdater_properErrorOnWriteFailure(t *testing.T) {
 	} {
 		t.Run(tc.desc, func(t *testing.T) {
 			cfg, _, repoPath, updater := setupUpdater(t, ctx)
-			defer func() { require.ErrorContains(t, updater.Close(), "closing updater: exit status 128") }()
+
+			expectedErr := InvalidReferenceFormatError{ReferenceName: referenceName}
+			defer func() { require.Equal(t, expectedErr, updater.Close()) }()
 
 			commitID := gittest.WriteCommit(t, cfg, repoPath)
 
@@ -158,7 +161,7 @@ func TestUpdater_properErrorOnWriteFailure(t *testing.T) {
 					continue
 				}
 
-				require.Equal(t, InvalidReferenceFormatError{ReferenceName: referenceName}, err)
+				require.Equal(t, expectedErr, err)
 				break
 			}
 		})
@@ -234,7 +237,13 @@ func TestUpdater_fileDirectoryConflict(t *testing.T) {
 				t.Run(method.desc, func(t *testing.T) {
 					t.Run("different transaction", func(t *testing.T) {
 						cfg, _, repoPath, updater := setupUpdater(t, ctx)
-						defer func() { require.ErrorContains(t, updater.Close(), "closing updater: exit status 128") }()
+
+						expectedErr := FileDirectoryConflictError{
+							ExistingReferenceName:    tc.firstReference.String(),
+							ConflictingReferenceName: tc.secondReference.String(),
+						}
+
+						defer func() { require.Equal(t, expectedErr, updater.Close()) }()
 
 						commitID := gittest.WriteCommit(t, cfg, repoPath)
 
@@ -245,15 +254,17 @@ func TestUpdater_fileDirectoryConflict(t *testing.T) {
 						require.NoError(t, updater.Start())
 						require.NoError(t, updater.Create(tc.secondReference, commitID))
 
-						require.Equal(t, FileDirectoryConflictError{
-							ExistingReferenceName:    tc.firstReference.String(),
-							ConflictingReferenceName: tc.secondReference.String(),
-						}, method.finish(updater))
+						require.Equal(t, expectedErr, method.finish(updater))
 					})
 
 					t.Run("same transaction", func(t *testing.T) {
 						cfg, _, repoPath, updater := setupUpdater(t, ctx)
-						defer func() { require.ErrorContains(t, updater.Close(), "closing updater: exit status 128") }()
+
+						expectedErr := InTransactionConflictError{
+							FirstReferenceName:  tc.firstReference.String(),
+							SecondReferenceName: tc.secondReference.String(),
+						}
+						defer func() { require.Equal(t, expectedErr, updater.Close()) }()
 
 						commitID := gittest.WriteCommit(t, cfg, repoPath)
 
@@ -261,10 +272,7 @@ func TestUpdater_fileDirectoryConflict(t *testing.T) {
 						require.NoError(t, updater.Create(tc.firstReference, commitID))
 						require.NoError(t, updater.Create(tc.secondReference, commitID))
 
-						require.Equal(t, InTransactionConflictError{
-							FirstReferenceName:  tc.firstReference.String(),
-							SecondReferenceName: tc.secondReference.String(),
-						}, method.finish(updater))
+						require.Equal(t, expectedErr, method.finish(updater))
 					})
 				})
 			}
@@ -277,23 +285,104 @@ func TestUpdater_invalidStateTransitions(t *testing.T) {
 
 	ctx := testhelper.Context(t)
 
-	cfg, repo, repoPath, updater := setupUpdater(t, ctx)
-	defer testhelper.MustClose(t, updater)
+	for _, tc := range []struct {
+		desc        string
+		expectedErr error
+		perform     func(*testing.T, *Updater) error
+	}{
+		{
+			desc: "update before starting",
+			perform: func(t *testing.T, u *Updater) error {
+				return u.Update("", "", "")
+			},
+			expectedErr: invalidStateTransitionError{expected: stateStarted, actual: stateIdle},
+		},
+		{
+			desc: "create before starting",
+			perform: func(t *testing.T, u *Updater) error {
+				return u.Create("", "")
+			},
+			expectedErr: invalidStateTransitionError{expected: stateStarted, actual: stateIdle},
+		},
+		{
+			desc: "delete before starting",
+			perform: func(t *testing.T, u *Updater) error {
+				return u.Delete("")
+			},
+			expectedErr: invalidStateTransitionError{expected: stateStarted, actual: stateIdle},
+		},
+		{
+			desc: "prepare before starting",
+			perform: func(t *testing.T, u *Updater) error {
+				return u.Prepare()
+			},
+			expectedErr: invalidStateTransitionError{expected: stateStarted, actual: stateIdle},
+		},
+		{
+			desc: "preparing before starting",
+			perform: func(t *testing.T, u *Updater) error {
+				return u.Prepare()
+			},
+			expectedErr: invalidStateTransitionError{expected: stateStarted, actual: stateIdle},
+		},
+		{
+			desc: "preparing when prepared",
+			perform: func(t *testing.T, u *Updater) error {
+				require.NoError(t, u.Start())
+				require.NoError(t, u.Prepare())
+				return u.Prepare()
+			},
+			expectedErr: invalidStateTransitionError{expected: stateStarted, actual: statePrepared},
+		},
+		{
+			desc: "starting when started",
+			perform: func(t *testing.T, u *Updater) error {
+				require.NoError(t, u.Start())
+				return u.Start()
+			},
+			expectedErr: invalidStateTransitionError{expected: stateIdle, actual: stateStarted},
+		},
+		{
+			desc: "starting when prepared",
+			perform: func(t *testing.T, u *Updater) error {
+				require.NoError(t, u.Start())
+				require.NoError(t, u.Prepare())
+				return u.Start()
+			},
+			expectedErr: invalidStateTransitionError{expected: stateIdle, actual: statePrepared},
+		},
+	} {
+		t.Run(tc.desc, func(t *testing.T) {
+			_, repo, _, updater := setupUpdater(t, ctx)
+			defer func() { require.Equal(t, tc.expectedErr, updater.Close()) }()
 
-	commitID := gittest.WriteCommit(t, cfg, repoPath)
+			require.Equal(t, tc.expectedErr, tc.perform(t, updater))
 
-	require.Equal(t, invalidStateTransitionError{expected: stateStarted, actual: stateIdle}, updater.Update("refs/heads/main", commitID, ""))
-	require.Equal(t, invalidStateTransitionError{expected: stateStarted, actual: stateClosed}, updater.Create("refs/heads/main", commitID))
-	require.Equal(t, invalidStateTransitionError{expected: stateStarted, actual: stateClosed}, updater.Delete("refs/heads/main"))
-	require.Equal(t, invalidStateTransitionError{expected: stateStarted, actual: stateClosed}, updater.Prepare())
-	require.Equal(t, invalidStateTransitionError{expected: stateStarted, actual: stateClosed}, updater.Commit())
-	require.Equal(t, invalidStateTransitionError{expected: stateIdle, actual: stateClosed}, updater.Start())
-	require.NoError(t, updater.Close())
+			// Verify no references were created.
+			refs, err := repo.GetReferences(ctx)
+			require.NoError(t, err)
+			require.Empty(t, refs)
+		})
+	}
+}
 
-	// Verify no references were created.
-	refs, err := repo.GetReferences(ctx)
-	require.NoError(t, err)
-	require.Empty(t, refs)
+func TestUpdater_allMethodsReturnSameError(t *testing.T) {
+	t.Parallel()
+
+	ctx := testhelper.Context(t)
+
+	_, _, _, updater := setupUpdater(t, ctx)
+
+	expectedErr := invalidStateTransitionError{expected: stateStarted, actual: stateIdle}
+	defer func() { require.Equal(t, expectedErr, updater.Close()) }()
+
+	require.Equal(t, expectedErr, updater.Update("", "", ""))
+	require.Equal(t, expectedErr, updater.Create("", ""))
+	require.Equal(t, expectedErr, updater.Delete(""))
+	require.Equal(t, expectedErr, updater.Prepare())
+	require.Equal(t, expectedErr, updater.Commit())
+	require.Equal(t, expectedErr, updater.Start())
+	require.Equal(t, expectedErr, updater.Close())
 }
 
 func TestUpdater_update(t *testing.T) {
@@ -305,7 +394,9 @@ func TestUpdater_update(t *testing.T) {
 
 	// The updater cancel should fail at the end of the test as the final operation is an error,
 	// which results in closing the updater.
-	defer func() { require.ErrorContains(t, updater.Close(), "closing updater: exit status 128") }()
+
+	var expectedErr error
+	defer func() { require.Equal(t, expectedErr, updater.Close()) }()
 
 	oldCommitID := gittest.WriteCommit(t, cfg, repoPath, gittest.WithBranch("main"))
 	newCommitID := gittest.WriteCommit(t, cfg, repoPath, gittest.WithParents(oldCommitID))
@@ -328,12 +419,13 @@ func TestUpdater_update(t *testing.T) {
 	require.NoError(t, updater.Start())
 	require.NoError(t, updater.Update("refs/heads/main", newCommitID, otherCommitID))
 
-	require.Equal(t, MismatchingStateError{
+	expectedErr = MismatchingStateError{
 		ReferenceName:    "refs/heads/main",
 		ExpectedObjectID: otherCommitID.String(),
 		ActualObjectID:   oldCommitID.String(),
-	}, updater.Commit())
-	require.Equal(t, invalidStateTransitionError{expected: stateIdle, actual: stateClosed}, updater.Start())
+	}
+	require.Equal(t, expectedErr, updater.Commit())
+	require.Equal(t, expectedErr, updater.Start())
 
 	require.Equal(t, gittest.ResolveRevision(t, cfg, repoPath, "refs/heads/main"), oldCommitID)
 }
@@ -363,7 +455,9 @@ func TestUpdater_prepareLocksTransaction(t *testing.T) {
 	ctx := testhelper.Context(t)
 
 	cfg, _, repoPath, updater := setupUpdater(t, ctx)
-	defer testhelper.MustClose(t, updater)
+
+	expectedErr := invalidStateTransitionError{expected: stateStarted, actual: statePrepared}
+	defer func() { require.Equal(t, expectedErr, updater.Close()) }()
 
 	commitID := gittest.WriteCommit(t, cfg, repoPath)
 
@@ -372,7 +466,7 @@ func TestUpdater_prepareLocksTransaction(t *testing.T) {
 	require.NoError(t, updater.Prepare())
 
 	require.Equal(t, invalidStateTransitionError{expected: stateStarted, actual: statePrepared}, updater.Update("refs/heads/feature", commitID, ""))
-	require.Equal(t, invalidStateTransitionError{expected: stateStarted, actual: stateClosed}, updater.Commit())
+	require.Equal(t, invalidStateTransitionError{expected: stateStarted, actual: statePrepared}, updater.Commit())
 }
 
 func TestUpdater_invalidReferenceName(t *testing.T) {
@@ -388,11 +482,11 @@ func TestUpdater_invalidReferenceName(t *testing.T) {
 	repo := localrepo.NewTestRepo(t, cfg, repoProto, git.WithSkipHooks())
 	commitID := gittest.WriteCommit(t, cfg, repoPath)
 
+	const referenceName = `refs/heads\master`
 	updater, err := New(ctx, repo)
 	require.NoError(t, err)
-	defer func() { require.ErrorContains(t, updater.Close(), "closing updater: exit status 128") }()
+	defer func() { require.Equal(t, InvalidReferenceFormatError{ReferenceName: referenceName}, updater.Close()) }()
 
-	const referenceName = `refs/heads\master`
 	require.NoError(t, updater.Start())
 	require.NoError(t, updater.Update(referenceName, commitID, ""))
 	require.Equal(t, InvalidReferenceFormatError{ReferenceName: referenceName}, updater.Prepare())
@@ -523,7 +617,7 @@ func TestUpdater_cancel(t *testing.T) {
 	ctx := testhelper.Context(t)
 
 	cfg, repo, repoPath, firstUpdater := setupUpdater(t, ctx)
-	defer testhelper.MustClose(t, firstUpdater)
+	defer func() { require.Equal(t, errClosed, firstUpdater.Close()) }()
 
 	gittest.WriteCommit(t, cfg, repoPath, gittest.WithBranch("main"))
 
@@ -536,13 +630,15 @@ func TestUpdater_cancel(t *testing.T) {
 	// because the reference is locked already.
 	failingUpdater, err := New(ctx, repo)
 	require.NoError(t, err)
-	defer func() { require.ErrorContains(t, failingUpdater.Close(), "closing updater: exit status 128") }()
+
+	expectedErr := AlreadyLockedError{
+		ReferenceName: "refs/heads/main",
+	}
+	defer func() { require.Equal(t, expectedErr, failingUpdater.Close()) }()
 
 	require.NoError(t, failingUpdater.Start())
 	require.NoError(t, failingUpdater.Delete(git.ReferenceName("refs/heads/main")))
-	require.Equal(t, AlreadyLockedError{
-		ReferenceName: "refs/heads/main",
-	}, failingUpdater.Commit())
+	require.Equal(t, expectedErr, failingUpdater.Commit())
 
 	// We now cancel the initial updater. Afterwards, it should be possible again to update the
 	// ref because locks should have been released.
@@ -589,16 +685,23 @@ func TestUpdater_capturesStderr(t *testing.T) {
 	ctx := testhelper.Context(t)
 
 	_, _, _, updater := setupUpdater(t, ctx)
-	defer func() { require.ErrorContains(t, updater.Close(), "closing updater: exit status 128") }()
+	var expectedErr error
+	defer func() { require.Equal(t, expectedErr, updater.Close()) }()
 
 	require.NoError(t, updater.Start())
 	// fail the process by writing bad input
 	_, err := updater.cmd.Write([]byte("garbage input"))
 	require.NoError(t, err)
 
-	require.Equal(t, structerr.New("%w", fmt.Errorf("state update to %q failed: %w", "commit", io.EOF)).WithMetadata(
-		"stderr", "fatal: unknown command: garbage inputcommit\n",
-	), updater.Commit())
+	expectedErr = structerr.New("%w", updater.cmd.Wait()).WithMetadataItems(
+		structerr.MetadataItem{Key: "stderr", Value: "fatal: unknown command: garbage input\n"},
+		structerr.MetadataItem{Key: "close_error", Value: &fs.PathError{
+			Op:   "write",
+			Path: "|1",
+			Err:  errors.New("file already closed"),
+		}},
+	)
+	require.Equal(t, expectedErr, updater.Commit())
 }
 
 func BenchmarkUpdater(b *testing.B) {
diff --git a/internal/gitaly/service/operations/user_create_branch_test.go b/internal/gitaly/service/operations/user_create_branch_test.go
index 6e58e4a48..c62e4389d 100644
--- a/internal/gitaly/service/operations/user_create_branch_test.go
+++ b/internal/gitaly/service/operations/user_create_branch_test.go
@@ -1,7 +1,6 @@
 package operations
 
 import (
-	"io"
 	"testing"
 
 	"github.com/stretchr/testify/require"
@@ -399,7 +398,7 @@ func TestUserCreateBranch_failure(t *testing.T) {
 			startPoint: "master",
 			user:       gittest.TestUser,
 			err: testhelper.WithInterceptedMetadata(
-				structerr.NewFailedPrecondition("reference update: state update to %q failed: %w", "prepare", io.EOF),
+				structerr.NewFailedPrecondition("reference update: exit status 128"),
 				"stderr",
 				"fatal: prepare: cannot lock ref 'refs/heads/master': reference already exists\n",
 			),