diff options
author | Patrick Steinhardt <psteinhardt@gitlab.com> | 2023-09-14 18:19:00 +0300 |
---|---|---|
committer | Patrick Steinhardt <psteinhardt@gitlab.com> | 2023-09-14 18:19:00 +0300 |
commit | 1f43cd77966ec64b232d4a5fa26a1e3baec94601 (patch) | |
tree | 05ed4bf837de8986b0ebc3d82f6d0981fd776968 | |
parent | df878b9f587abf53769767632dfc0af758d0efa3 (diff) | |
parent | 899b60ac6345e425512ad863a4924abd743bf9c7 (diff) |
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>
-rw-r--r-- | internal/gitaly/service/ref/find_local_branches_test.go | 39 | ||||
-rw-r--r-- | internal/gitaly/service/ref/util.go | 35 |
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 { |