gitserver: make vcs syncer implementation request a fresh clone URL i…

…n between each remote git operation (#61312)

wip: gitserver: make vcs syncer implementation request a fresh clone URL in between each remote git operation

commit-id:4575598b
(cherry picked from commit cbbb9c3)

 Conflicts:
	cmd/gitserver/internal/integration_tests/clone_test.go
	cmd/gitserver/internal/integration_tests/resolverevisions_test.go
	cmd/gitserver/internal/integration_tests/test_utils.go
	cmd/gitserver/internal/server.go
	cmd/gitserver/internal/server_test.go
	cmd/gitserver/internal/vcssyncer/git.go

CHANGELOG.md

@@ -26,7 +26,7 @@ All notable changes to Sourcegraph are documented in this file.
 
 ### Fixed
 
--
+- Fixed a bug in gitserver where it was possible to use expired Github App authorization tokens when syncing a large repository. Now, gitserver will use the latest tokens for each step of the syncing process (and refresh them if necessary). [#61179](https://github.com/sourcegraph/sourcegraph/pull/61179/)
 
 ### Removed
 

cmd/gitserver/internal/executil/executil.go

@@ -347,6 +347,11 @@ func RunCommandWriteOutput(ctx context.Context, cmd wrexec.Cmder, writer io.Writ
 	}
 
 	err = cmd.Wait()
+	var exitErr *exec.ExitError
+	if errors.As(err, &exitErr) {
+		// Redact the stderr output if we have it.
+		exitErr.Stderr = []byte(redactor(string(exitErr.Stderr)))
+	}
 
 	if ps := cmd.Unwrap().ProcessState; ps != nil && ps.Sys() != nil {
 		if ws, ok := ps.Sys().(syscall.WaitStatus); ok {

cmd/gitserver/internal/integration_tests/BUILD.bazel

@@ -25,7 +25,9 @@ go_library(
         "//internal/observation",
         "//internal/ratelimit",
         "//internal/types",
+        "//internal/vcs",
         "//internal/wrexec",
+        "//lib/errors",
         "@com_github_sourcegraph_log//:log",
         "@com_github_sourcegraph_log//logtest",
         "@org_golang_x_sync//semaphore",

cmd/gitserver/internal/integration_tests/archivereader_test.go

@@ -6,6 +6,7 @@ import (
 	"context"
 	"encoding/base64"
 	"fmt"
+	"github.com/sourcegraph/sourcegraph/internal/vcs"
 	"io"
 	"net/http/httptest"
 	"net/url"
@@ -96,21 +97,42 @@ func TestClient_ArchiveReader(t *testing.T) {
 	runArchiveReaderTestfunc := func(t *testing.T, mkClient func(t *testing.T, addrs []string) gitserver.Client, name api.RepoName, test test) {
 		t.Run(string(name), func(t *testing.T) {
 			// Setup: Prepare the test Gitserver server + register the gRPC server
+
+			getRemoteURLFunc := func(_ context.Context, name api.RepoName) (string, error) {
+				if test.remote != "" {
+					return test.remote, nil
+				}
+				return "", errors.Errorf("no remote for %s", test.name)
+			}
+
 			s := &server.Server{
 				Logger:   logtest.Scoped(t),
 				ReposDir: filepath.Join(root, "repos"),
 				DB:       newMockDB(),
 				GetBackendFunc: func(dir common.GitDir, repoName api.RepoName) git.GitBackend {
 					return gitcli.NewBackend(logtest.Scoped(t), wrexec.NewNoOpRecordingCommandFactory(), dir, repoName)
 				},
-				GetRemoteURLFunc: func(_ context.Context, name api.RepoName) (string, error) {
-					if test.remote != "" {
-						return test.remote, nil
-					}
-					return "", errors.Errorf("no remote for %s", test.name)
-				},
+				GetRemoteURLFunc: getRemoteURLFunc,
 				GetVCSSyncer: func(ctx context.Context, name api.RepoName) (vcssyncer.VCSSyncer, error) {
-					return vcssyncer.NewGitRepoSyncer(logtest.Scoped(t), wrexec.NewNoOpRecordingCommandFactory()), nil
+					getRemoteURLSource := func(ctx context.Context, name api.RepoName) (vcssyncer.RemoteURLSource, error) {
+						source := vcssyncer.RemoteURLSourceFunc(func(ctx context.Context) (*vcs.URL, error) {
+							raw, err := getRemoteURLFunc(ctx, name)
+							if err != nil {
+								return nil, errors.Wrapf(err, "failed to get remote URL for %s", name)
+							}
+
+							u, err := vcs.ParseURL(raw)
+							if err != nil {
+								return nil, errors.Wrapf(err, "failed to parse remote URL for repository: %s, raw: %s", name, raw)
+							}
+
+							return u, nil
+						})
+
+						return source, nil
+					}
+
+					return vcssyncer.NewGitRepoSyncer(logtest.Scoped(t), wrexec.NewNoOpRecordingCommandFactory(), getRemoteURLSource), nil
 				},
 				RecordingCommandFactory: wrexec.NewNoOpRecordingCommandFactory(),
 				Locker:                  server.NewRepositoryLocker(),

cmd/gitserver/internal/integration_tests/clone_test.go

@@ -3,6 +3,7 @@ package inttests
 import (
 	"container/list"
 	"context"
+	"github.com/sourcegraph/sourcegraph/lib/errors"
 	"net/http/httptest"
 	"net/url"
 	"os"
@@ -51,19 +52,37 @@ func TestClone(t *testing.T) {
 	lock := NewMockRepositoryLock()
 	locker.TryAcquireFunc.SetDefaultReturn(lock, true)
 
+	getRemoteURLFunc := func(_ context.Context, name api.RepoName) (string, error) { //nolint:unparam
+		require.Equal(t, repo, name)
+		return remote, nil
+	}
+
 	s := server.Server{
 		Logger:   logger,
 		ReposDir: reposDir,
 		GetBackendFunc: func(dir common.GitDir, repoName api.RepoName) git.GitBackend {
 			return gitcli.NewBackend(logtest.Scoped(t), wrexec.NewNoOpRecordingCommandFactory(), dir, repoName)
 		},
-		GetRemoteURLFunc: func(_ context.Context, name api.RepoName) (string, error) {
-			require.Equal(t, repo, name)
-			return remote, nil
-		},
+		GetRemoteURLFunc: getRemoteURLFunc,
 		GetVCSSyncer: func(ctx context.Context, name api.RepoName) (vcssyncer.VCSSyncer, error) {
+			getRemoteURLSource := func(ctx context.Context, name api.RepoName) (vcssyncer.RemoteURLSource, error) {
+				require.Equal(t, repo, name)
+				return vcssyncer.RemoteURLSourceFunc(func(ctx context.Context) (*vcs.URL, error) {
+					raw, err := getRemoteURLFunc(ctx, name)
+					if err != nil {
+						return nil, errors.Wrapf(err, "failed to get remote URL for %s", name)
+					}
+
+					u, err := vcs.ParseURL(raw)
+					if err != nil {
+						return nil, errors.Wrapf(err, "failed to parse remote URL %q", raw)
+					}
+					return u, nil
+				}), nil
+			}
+
 			require.Equal(t, repo, name)
-			return vcssyncer.NewGitRepoSyncer(logtest.Scoped(t), wrexec.NewNoOpRecordingCommandFactory()), nil
+			return vcssyncer.NewGitRepoSyncer(logtest.Scoped(t), wrexec.NewNoOpRecordingCommandFactory(), getRemoteURLSource), nil
 		},
 		DB:                      db,
 		Perforce:                perforce.NewService(ctx, observation.TestContextTB(t), logger, db, list.New()),
@@ -93,10 +112,10 @@ func TestClone(t *testing.T) {
 
 	// Should have acquired a lock.
 	mockassert.CalledOnce(t, locker.TryAcquireFunc)
-	// Should have reported status. 21 lines is the output git currently produces.
+	// Should have reported status. 24 lines is the output git currently produces.
 	// This number might need to be adjusted over time, but before doing so please
 	// check that the calls actually use the args you would expect them to use.
-	mockassert.CalledN(t, lock.SetStatusFunc, 21)
+	mockassert.CalledN(t, lock.SetStatusFunc, 24)
 	// Should have released the lock.
 	mockassert.CalledOnce(t, lock.ReleaseFunc)
 
@@ -147,19 +166,36 @@ func TestClone_Fail(t *testing.T) {
 	lock := NewMockRepositoryLock()
 	locker.TryAcquireFunc.SetDefaultReturn(lock, true)
 
+	getRemoteURLFunc := func(_ context.Context, name api.RepoName) (string, error) { //nolint:unparam
+		require.Equal(t, repo, name)
+		return remote, nil
+	}
+
 	s := server.Server{
 		Logger:   logger,
 		ReposDir: reposDir,
 		GetBackendFunc: func(dir common.GitDir, repoName api.RepoName) git.GitBackend {
 			return gitcli.NewBackend(logtest.Scoped(t), wrexec.NewNoOpRecordingCommandFactory(), dir, repoName)
 		},
-		GetRemoteURLFunc: func(_ context.Context, name api.RepoName) (string, error) {
-			require.Equal(t, repo, name)
-			return remote, nil
-		},
+		GetRemoteURLFunc: getRemoteURLFunc,
 		GetVCSSyncer: func(ctx context.Context, name api.RepoName) (vcssyncer.VCSSyncer, error) {
 			require.Equal(t, repo, name)
-			return vcssyncer.NewGitRepoSyncer(logtest.Scoped(t), wrexec.NewNoOpRecordingCommandFactory()), nil
+			getRemoteURLSource := func(ctx context.Context, name api.RepoName) (vcssyncer.RemoteURLSource, error) {
+				require.Equal(t, repo, name)
+				return vcssyncer.RemoteURLSourceFunc(func(ctx context.Context) (*vcs.URL, error) {
+					raw, err := getRemoteURLFunc(ctx, name)
+					if err != nil {
+						return nil, errors.Wrapf(err, "failed to get remote URL for %s", name)
+					}
+
+					u, err := vcs.ParseURL(raw)
+					if err != nil {
+						return nil, errors.Wrapf(err, "failed to parse remote URL %q", raw)
+					}
+					return u, nil
+				}), nil
+			}
+			return vcssyncer.NewGitRepoSyncer(logtest.Scoped(t), wrexec.NewNoOpRecordingCommandFactory(), getRemoteURLSource), nil
 		},
 		DB:                      db,
 		Perforce:                perforce.NewService(ctx, observation.TestContextTB(t), logger, db, list.New()),
@@ -210,7 +246,7 @@ func TestClone_Fail(t *testing.T) {
 
 	// Now, fake that the IsCloneable check passes, then Clone will be called
 	// and is expected to fail.
-	vcssyncer.TestGitRepoExists = func(ctx context.Context, remoteURL *vcs.URL) error {
+	vcssyncer.TestGitRepoExists = func(ctx context.Context, name api.RepoName) error {
 		return nil
 	}
 	t.Cleanup(func() {

cmd/gitserver/internal/integration_tests/resolverevisions_test.go

@@ -3,6 +3,8 @@ package inttests
 import (
 	"container/list"
 	"context"
+	"github.com/sourcegraph/sourcegraph/internal/vcs"
+	"github.com/sourcegraph/sourcegraph/lib/errors"
 	"net/http/httptest"
 	"net/url"
 	"path/filepath"
@@ -69,17 +71,33 @@ func TestClient_ResolveRevisions(t *testing.T) {
 	db := newMockDB()
 	ctx := context.Background()
 
+	getRemoteURLFunc := func(_ context.Context, name api.RepoName) (string, error) { //nolint:unparam
+		return remote, nil
+	}
+
 	s := server.Server{
 		Logger:   logger,
 		ReposDir: filepath.Join(root, "repos"),
 		GetBackendFunc: func(dir common.GitDir, repoName api.RepoName) git.GitBackend {
 			return gitcli.NewBackend(logtest.Scoped(t), wrexec.NewNoOpRecordingCommandFactory(), dir, repoName)
 		},
-		GetRemoteURLFunc: func(_ context.Context, name api.RepoName) (string, error) {
-			return remote, nil
-		},
+		GetRemoteURLFunc: getRemoteURLFunc,
 		GetVCSSyncer: func(ctx context.Context, name api.RepoName) (vcssyncer.VCSSyncer, error) {
-			return vcssyncer.NewGitRepoSyncer(logtest.Scoped(t), wrexec.NewNoOpRecordingCommandFactory()), nil
+			getRemoteURLSource := func(ctx context.Context, name api.RepoName) (vcssyncer.RemoteURLSource, error) {
+				return vcssyncer.RemoteURLSourceFunc(func(ctx context.Context) (*vcs.URL, error) {
+					raw, err := getRemoteURLFunc(ctx, name)
+					if err != nil {
+						return nil, errors.Wrapf(err, "failed to get remote URL for %s", name)
+					}
+
+					u, err := vcs.ParseURL(raw)
+					if err != nil {
+						return nil, errors.Wrapf(err, "failed to parse remote URL %q", raw)
+					}
+					return u, nil
+				}), nil
+			}
+			return vcssyncer.NewGitRepoSyncer(logtest.Scoped(t), wrexec.NewNoOpRecordingCommandFactory(), getRemoteURLSource), nil
 		},
 		DB:                      db,
 		Perforce:                perforce.NewService(ctx, observation.TestContextTB(t), logger, db, list.New()),

cmd/gitserver/internal/integration_tests/test_utils.go

@@ -2,6 +2,8 @@ package inttests
 
 import (
 	"context"
+	"github.com/sourcegraph/sourcegraph/internal/vcs"
+	"github.com/sourcegraph/sourcegraph/lib/errors"
 	"net"
 	"net/http"
 	"os"
@@ -76,18 +78,36 @@ func InitGitserver() {
 	})
 	db.ReposFunc.SetDefaultReturn(r)
 
+	getRemoteURLFunc := func(_ context.Context, name api.RepoName) (string, error) { //nolint:unparam // context is unused but required by the interface, error is not used in this test
+		return filepath.Join(root, "remotes", string(name)), nil
+	}
+
 	s := server.Server{
 		Logger:         sglog.Scoped("server"),
 		ObservationCtx: &observation.TestContext,
 		ReposDir:       filepath.Join(root, "repos"),
 		GetBackendFunc: func(dir common.GitDir, repoName api.RepoName) git.GitBackend {
 			return gitcli.NewBackend(logtest.Scoped(&t), wrexec.NewNoOpRecordingCommandFactory(), dir, repoName)
 		},
-		GetRemoteURLFunc: func(ctx context.Context, name api.RepoName) (string, error) {
-			return filepath.Join(root, "remotes", string(name)), nil
-		},
+		GetRemoteURLFunc: getRemoteURLFunc,
 		GetVCSSyncer: func(ctx context.Context, name api.RepoName) (vcssyncer.VCSSyncer, error) {
-			return vcssyncer.NewGitRepoSyncer(logger, wrexec.NewNoOpRecordingCommandFactory()), nil
+			getRemoteURLSource := func(ctx context.Context, name api.RepoName) (vcssyncer.RemoteURLSource, error) {
+				return vcssyncer.RemoteURLSourceFunc(func(ctx context.Context) (*vcs.URL, error) {
+					raw, err := getRemoteURLFunc(ctx, name)
+					if err != nil {
+						return nil, errors.Wrapf(err, "failed to get remote URL for %s", name)
+					}
+
+					u, err := vcs.ParseURL(raw)
+					if err != nil {
+						return nil, errors.Wrapf(err, "failed to parse remote URL %q", raw)
+					}
+
+					return u, nil
+				}), nil
+			}
+
+			return vcssyncer.NewGitRepoSyncer(logger, wrexec.NewNoOpRecordingCommandFactory(), getRemoteURLSource), nil
 		},
 		GlobalBatchLogSemaphore: semaphore.NewWeighted(32),
 		DB:                      db,