Merge branch 'pks-updateref-fix-context-cancellation-races' into 'master'

updateref: Fix handling of context cancellation errors

See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5345

Merged-by: Quang-Minh Nguyen <qmnguyen@gitlab.com>
Approved-by: Quang-Minh Nguyen <qmnguyen@gitlab.com>
Approved-by: karthik nayak <knayak@gitlab.com>
Reviewed-by: Quang-Minh Nguyen <qmnguyen@gitlab.com>
Reviewed-by: karthik nayak <knayak@gitlab.com>
Co-authored-by: Patrick Steinhardt <psteinhardt@gitlab.com>

2 files changed, 45 insertions, 19 deletions

diff --git a/internal/git/updateref/updateref.go b/internal/git/updateref/updateref.go
index 7a078f978..a161d8435 100644
--- a/internal/git/updateref/updateref.go
+++ b/internal/git/updateref/updateref.go
@@ -4,11 +4,13 @@ import (
 	"bufio"
 	"bytes"
 	"context"
+	"errors"
 	"fmt"
 	"regexp"
 
 	"gitlab.com/gitlab-org/gitaly/v15/internal/command"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git"
+	"gitlab.com/gitlab-org/gitaly/v15/internal/structerr"
 )
 
 // ErrAlreadyLocked indicates a reference cannot be locked because another
@@ -317,13 +319,7 @@ func (u *Updater) Close() error {
 
 func (u *Updater) write(format string, args ...interface{}) error {
 	if _, err := fmt.Fprintf(u.cmd, format, args...); err != nil {
-		// 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.
-		_ = u.Close()
-		// The update-ref process may have already exited due to an error. In such cases,
-		// the write errors are not meaningful. If we find one of the typed errors in
-		// stderr, we'll return it instead.
-		return parseStderrError(u.stderr.Bytes(), fmt.Errorf("%w: %q", err, u.stderr))
+		return u.handleIOError(err)
 	}
 
 	return nil
@@ -350,11 +346,7 @@ func (u *Updater) setState(state string) error {
 	// raised.
 	line, err := u.stdout.ReadString('\n')
 	if err != nil {
-		// 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.
-		_ = u.Close()
-		return parseStderrError(u.stderr.Bytes(), fmt.Errorf("state update to %q failed: %w, stderr: %q", state, err, u.stderr))
+		return u.handleIOError(fmt.Errorf("state update to %q failed: %w", state, err))
 	}
 
 	if line != fmt.Sprintf("%s: ok\n", state) {
@@ -365,9 +357,28 @@ func (u *Updater) setState(state string) error {
 	return nil
 }
 
-// parseStderrError returns any typed error that might be present in stderr. If
-// the error message does not match any typed error, defaultErr is returned instead.
-func parseStderrError(stderr []byte, defaultErr error) error {
+// 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
+		}
+	}
+
+	stderr := u.stderr.Bytes()
+
 	matches := refLockedRegex.FindSubmatch(stderr)
 	if len(matches) > 1 {
 		return &ErrAlreadyLocked{Ref: string(matches[1])}
@@ -410,5 +421,5 @@ func parseStderrError(stderr []byte, defaultErr error) error {
 		}
 	}
 
-	return defaultErr
+	return structerr.New("%w", fallbackErr).WithMetadata("stderr", string(stderr))
 }
diff --git a/internal/git/updateref/updateref_test.go b/internal/git/updateref/updateref_test.go
index addafa2da..ce04344a8 100644
--- a/internal/git/updateref/updateref_test.go
+++ b/internal/git/updateref/updateref_test.go
@@ -3,7 +3,9 @@ package updateref
 import (
 	"context"
 	"encoding/hex"
+	"errors"
 	"fmt"
+	"io"
 	"os"
 	"path/filepath"
 	"testing"
@@ -14,6 +16,7 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git/gittest"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git/localrepo"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/config"
+	"gitlab.com/gitlab-org/gitaly/v15/internal/structerr"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper/testcfg"
 )
@@ -325,7 +328,10 @@ func TestUpdater_update(t *testing.T) {
 	// when the old commit ID doesn't match.
 	require.NoError(t, updater.Start())
 	require.NoError(t, updater.Update("refs/heads/main", newCommitID, otherCommitID))
-	require.ErrorContains(t, updater.Commit(), fmt.Sprintf("fatal: commit: cannot lock ref 'refs/heads/main': is at %s but expected %s", oldCommitID, otherCommitID))
+
+	require.Equal(t, structerr.New("%w", fmt.Errorf("state update to %q failed: %w", "commit", io.EOF)).WithMetadata(
+		"stderr", fmt.Sprintf("fatal: commit: cannot lock ref 'refs/heads/main': is at %s but expected %s\n", oldCommitID, otherCommitID),
+	), updater.Commit())
 	require.Equal(t, invalidStateTransitionError{expected: stateIdle, actual: stateClosed}, updater.Start())
 
 	require.Equal(t, gittest.ResolveRevision(t, cfg, repoPath, "refs/heads/main"), oldCommitID)
@@ -515,7 +521,14 @@ func TestUpdater_cancel(t *testing.T) {
 
 	require.NoError(t, failingUpdater.Start())
 	require.NoError(t, failingUpdater.Delete(git.ReferenceName("refs/heads/main")))
-	require.ErrorContains(t, failingUpdater.Commit(), "fatal: commit: cannot lock ref 'refs/heads/main'")
+
+	err = failingUpdater.Commit()
+	require.EqualError(t, err, fmt.Sprintf("state update to %q failed: %v", "commit", io.EOF))
+	var structErr structerr.Error
+	require.True(t, errors.As(err, &structErr))
+	// The error message returned by git-update-ref(1) is simply too long to fully verify it, so
+	// we just check that it matches a specific substring.
+	require.Contains(t, structErr.Metadata()["stderr"], "fatal: commit: cannot lock ref 'refs/heads/main'")
 
 	// We now cancel the initial updater. Afterwards, it should be possible again to update the
 	// ref because locks should have been released.
@@ -569,7 +582,9 @@ func TestUpdater_capturesStderr(t *testing.T) {
 	_, err := updater.cmd.Write([]byte("garbage input"))
 	require.NoError(t, err)
 
-	require.EqualError(t, updater.Commit(), `state update to "commit" failed: EOF, stderr: "fatal: unknown command: garbage inputcommit\n"`)
+	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())
 }
 
 func BenchmarkUpdater(b *testing.B) {