Merge branch 'pks-ref-find-local-branches-pagination-and-sigpipe' into 'master'

ref: Handle SIGPIPE when truncating output from git-for-each-ref(1)

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

Merged-by: Patrick Steinhardt <psteinhardt@gitlab.com>
Approved-by: Toon Claes <toon@gitlab.com>
Approved-by: karthik nayak <knayak@gitlab.com>

2 files changed, 69 insertions, 5 deletions

diff --git a/internal/gitaly/service/ref/find_local_branches_test.go b/internal/gitaly/service/ref/find_local_branches_test.go
index 3e3064901..e47b82214 100644
--- a/internal/gitaly/service/ref/find_local_branches_test.go
+++ b/internal/gitaly/service/ref/find_local_branches_test.go
@@ -9,6 +9,7 @@ import (
 	"github.com/stretchr/testify/require"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/git"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/git/gittest"
+	"gitlab.com/gitlab-org/gitaly/v16/internal/git/updateref"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/storage"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/structerr"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/testhelper"
@@ -233,6 +234,42 @@ func TestFindLocalBranches(t *testing.T) {
 			},
 		},
 		{
+			desc: "with pagination limit and many filtered branches",
+			setup: func(t *testing.T) setupData {
+				repo, repoPath := gittest.CreateRepository(t, ctx, cfg)
+
+				commitID, commit := writeCommit(t, ctx, cfg, repo, gittest.WithBranch("branch-a"), gittest.WithMessage("commit a"))
+
+				updater, err := updateref.New(ctx, gittest.NewRepositoryPathExecutor(t, cfg, repoPath))
+				require.NoError(t, err)
+
+				// Create a bunch of local branches. When we only ask for a very small subset, this will
+				// cause us to `Wait()` for git-for-each-ref(1) early while it is still enumerating the
+				// branches. We thus close its stdout early and then wait for it to terminate, which can
+				// lead to the process receiving the EPIPE signal if we didn't take proper care.
+				//
+				// So the more references we have, the more likely it is that we'll see the issue.
+				require.NoError(t, updater.Start())
+				for i := 0; i < 1000; i++ {
+					require.NoError(t, updater.Create(git.ReferenceName(fmt.Sprintf("refs/heads/branch-%d", i)), commitID))
+				}
+				require.NoError(t, updater.Commit())
+
+				return setupData{
+					request: &gitalypb.FindLocalBranchesRequest{
+						Repository: repo,
+						PaginationParams: &gitalypb.PaginationParameter{
+							Limit: 2,
+						},
+					},
+					expectedBranches: []*gitalypb.Branch{
+						{Name: []byte("refs/heads/branch-0"), TargetCommit: commit},
+						{Name: []byte("refs/heads/branch-1"), TargetCommit: commit},
+					},
+				}
+			},
+		},
+		{
 			desc: "invalid page token",
 			setup: func(t *testing.T) setupData {
 				repo, _ := gittest.CreateRepository(t, ctx, cfg)
@@ -246,7 +283,7 @@ func TestFindLocalBranches(t *testing.T) {
 							PageToken: "refs/heads/does-not-exist",
 						},
 					},
-					expectedErr: structerr.NewInternal("finding refs: could not find page token"),
+					expectedErr: structerr.NewInternal("finding refs: sending lines: could not find page token"),
 				}
 			},
 		},
diff --git a/internal/gitaly/service/ref/util.go b/internal/gitaly/service/ref/util.go
index 5fb74f82d..4f90c3b0d 100644
--- a/internal/gitaly/service/ref/util.go
+++ b/internal/gitaly/service/ref/util.go
@@ -3,12 +3,17 @@ package ref
 import (
 	"bytes"
 	"context"
+	"errors"
 	"fmt"
 	"math"
+	"os/exec"
+	"strings"
+	"syscall"
 
 	"gitlab.com/gitlab-org/gitaly/v16/internal/git"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/git/catfile"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/helper/lines"
+	"gitlab.com/gitlab-org/gitaly/v16/internal/structerr"
 	"gitlab.com/gitlab-org/gitaly/v16/proto/go/gitalypb"
 )
 
@@ -129,13 +134,14 @@ func (s *server) findRefs(ctx context.Context, writer lines.Sender, repo git.Rep
 		options = append(options, opts.cmdArgs...)
 	}
 
+	var stderr strings.Builder
 	cmd, err := repo.Exec(ctx, git.Command{
 		Name:  "for-each-ref",
 		Flags: options,
 		Args:  patterns,
-	}, git.WithSetupStdout())
+	}, git.WithSetupStdout(), git.WithStderr(&stderr))
 	if err != nil {
-		return err
+		return fmt.Errorf("spawning for-each-ref: %w", err)
 	}
 
 	if err := lines.Send(cmd, writer, lines.SenderOpts{
@@ -144,10 +150,31 @@ func (s *server) findRefs(ctx context.Context, writer lines.Sender, repo git.Rep
 		Limit:          opts.Limit,
 		PageTokenError: opts.PageTokenError,
 	}); err != nil {
-		return err
+		return fmt.Errorf("sending lines: %w", err)
 	}
 
-	return cmd.Wait()
+	if err := cmd.Wait(); err != nil {
+		var exitErr *exec.ExitError
+		if errors.As(err, &exitErr) {
+			// When we have a limit set up and have sent all references upstream then the call to `Wait()` may
+			// indeed cause us to tear down the still-running git-for-each-ref(1) process. Because we close stdout
+			// before sending a signal the end result may be that the process will die with EPIPE because it failed
+			// to write to stdout.
+			//
+			// This is an expected error though, and thus we ignore it here.
+			status, ok := exitErr.ProcessState.Sys().(syscall.WaitStatus)
+			if ok && status.Signaled() && status.Signal() == syscall.SIGPIPE {
+				return nil
+			}
+
+			return structerr.New("listing failed with exit code %d", status.ExitStatus()).
+				WithMetadata("stderr", stderr.String())
+		}
+
+		return fmt.Errorf("waiting for for-each-ref: %w", err)
+	}
+
+	return nil
 }
 
 type paginationOpts struct {