gitserver: Better clean up after state changes

cmd/frontend/graphqlbackend/repositories_test.go

@@ -741,8 +741,19 @@ func TestRepositories_Integration(t *testing.T) {
 		if err := gitserverRepos.SetRepoSize(ctx, rsc.repo.Name, rsc.size, "shard-1"); err != nil {
 			t.Fatal(err)
 		}
-		if err := gitserverRepos.SetCloneStatus(ctx, rsc.repo.Name, rsc.cloneStatus, "shard-1"); err != nil {
-			t.Fatal(err)
+		switch rsc.cloneStatus {
+		case types.CloneStatusCloned:
+			if err := gitserverRepos.SetCloned(ctx, rsc.repo.Name, "shard-1"); err != nil {
+				t.Fatal(err)
+			}
+		case types.CloneStatusCloning:
+			if err := gitserverRepos.SetCloning(ctx, rsc.repo.Name, "shard-1"); err != nil {
+				t.Fatal(err)
+			}
+		case types.CloneStatusNotCloned:
+			if err := gitserverRepos.SetNotCloned(ctx, rsc.repo.Name); err != nil {
+				t.Fatal(err)
+			}
 		}
 
 		if rsc.indexed {

cmd/gitserver/internal/cleanup.go

@@ -37,7 +37,6 @@ import (
 	"github.com/sourcegraph/sourcegraph/internal/goroutine"
 	"github.com/sourcegraph/sourcegraph/internal/hostname"
 	"github.com/sourcegraph/sourcegraph/internal/lazyregexp"
-	"github.com/sourcegraph/sourcegraph/internal/types"
 	"github.com/sourcegraph/sourcegraph/internal/wrexec"
 	"github.com/sourcegraph/sourcegraph/lib/errors"
 )
@@ -78,7 +77,7 @@ func NewJanitor(ctx context.Context, cfg JanitorConfig, db database.DB, fs gitse
 
 					toFree := howManyBytesToFree(logger, usage, cfg.DesiredPercentFree)
 
-					if err := freeUpSpace(ctx, logger, db, fs, cfg.ShardID, usage, cfg.DesiredPercentFree, toFree); err != nil {
+					if err := freeUpSpace(ctx, logger, db, fs, usage, cfg.DesiredPercentFree, toFree); err != nil {
 						logger.Error("error freeing up space", log.Error(err))
 					}
 
@@ -366,7 +365,7 @@ func cleanupRepos(
 		reposRemoved.WithLabelValues(reason).Inc()
 
 		// Set as not_cloned in the database.
-		if err := db.GitserverRepos().SetCloneStatus(ctx, repoName, types.CloneStatusNotCloned, shardID); err != nil {
+		if err := db.GitserverRepos().SetNotCloned(ctx, repoName); err != nil {
 			return true, errors.Wrap(err, "failed to update clone status")
 		}
 
@@ -486,7 +485,7 @@ func cleanupRepos(
 			return true, errors.Wrap(err, "failed to remove repo")
 		}
 		// Set as not_cloned in the database.
-		if err := db.GitserverRepos().SetCloneStatus(ctx, repoName, types.CloneStatusNotCloned, shardID); err != nil {
+		if err := db.GitserverRepos().SetNotCloned(ctx, repoName); err != nil {
 			return true, errors.Wrap(err, "failed to update clone status")
 		}
 
@@ -716,7 +715,7 @@ func howManyBytesToFree(logger log.Logger, usage diskusage.DiskUsage, desiredPer
 
 // freeUpSpace removes git directories under the fs, in order from least
 // recently to most recently used, until it has freed howManyBytesToFree.
-func freeUpSpace(ctx context.Context, logger log.Logger, db database.DB, fs gitserverfs.FS, shardID string, usage diskusage.DiskUsage, desiredPercentFree int, howManyBytesToFree int64) error {
+func freeUpSpace(ctx context.Context, logger log.Logger, db database.DB, fs gitserverfs.FS, usage diskusage.DiskUsage, desiredPercentFree int, howManyBytesToFree int64) error {
 	if howManyBytesToFree <= 0 {
 		return nil
 	}
@@ -773,7 +772,7 @@ func freeUpSpace(ctx context.Context, logger log.Logger, db database.DB, fs gits
 			continue
 		}
 		// Set as not_cloned in the database.
-		if err := db.GitserverRepos().SetCloneStatus(ctx, repoName, types.CloneStatusNotCloned, shardID); err != nil {
+		if err := db.GitserverRepos().SetNotCloned(ctx, repoName); err != nil {
 			logger.Warn("failed to update clone status", log.Error(err))
 		}
 		spaceFreed += delta

cmd/gitserver/internal/cleanup_test.go

@@ -811,12 +811,12 @@ func TestFreeUpSpace(t *testing.T) {
 	fs := gitserverfs.New(observation.TestContextTB(t), root)
 	require.NoError(t, fs.Initialize())
 	t.Run("no error if no space requested and no repos", func(t *testing.T) {
-		if err := freeUpSpace(context.Background(), logger, newMockedGitserverDB(), fs, "test-gitserver", &fakeDiskUsage{}, 10, 0); err != nil {
+		if err := freeUpSpace(context.Background(), logger, newMockedGitserverDB(), fs, &fakeDiskUsage{}, 10, 0); err != nil {
 			t.Fatal(err)
 		}
 	})
 	t.Run("error if space requested and no repos", func(t *testing.T) {
-		if err := freeUpSpace(context.Background(), logger, newMockedGitserverDB(), fs, "test-gitserver", &fakeDiskUsage{}, 10, 1); err == nil {
+		if err := freeUpSpace(context.Background(), logger, newMockedGitserverDB(), fs, &fakeDiskUsage{}, 10, 1); err == nil {
 			t.Fatal("want error")
 		}
 	})
@@ -843,7 +843,7 @@ func TestFreeUpSpace(t *testing.T) {
 		gr := dbmocks.NewMockGitserverRepoStore()
 		db.GitserverReposFunc.SetDefaultReturn(gr)
 		// Run.
-		if err := freeUpSpace(context.Background(), logger, db, fs, "test-gitserver", &fakeDiskUsage{}, 10, 1000); err != nil {
+		if err := freeUpSpace(context.Background(), logger, db, fs, &fakeDiskUsage{}, 10, 1000); err != nil {
 			t.Fatal(err)
 		}
 
@@ -860,10 +860,9 @@ func TestFreeUpSpace(t *testing.T) {
 			t.Errorf("repo dir size is %d, want no more than %d", rds, wantSize)
 		}
 
-		if len(gr.SetCloneStatusFunc.History()) == 0 {
-			t.Fatal("expected gitserverRepos.SetCloneStatus to be called, but wasn't")
+		if len(gr.SetNotClonedFunc.History()) == 0 {
+			t.Fatal("expected gitserverRepos.SetNotCloned to be called, but wasn't")
 		}
-		require.Equal(t, gr.SetCloneStatusFunc.History()[0].Arg2, types.CloneStatusNotCloned)
 	})
 }
 

cmd/gitserver/internal/integration_tests/clone_test.go

@@ -26,7 +26,6 @@ import (
 	"github.com/sourcegraph/sourcegraph/internal/database/dbmocks"
 	"github.com/sourcegraph/sourcegraph/internal/observation"
 	"github.com/sourcegraph/sourcegraph/internal/ratelimit"
-	"github.com/sourcegraph/sourcegraph/internal/types"
 	"github.com/sourcegraph/sourcegraph/internal/vcs"
 	"github.com/sourcegraph/sourcegraph/internal/wrexec"
 )
@@ -104,9 +103,10 @@ func TestClone(t *testing.T) {
 	mockassert.CalledOnce(t, lock.ReleaseFunc)
 
 	// Check it was set to cloning first, then cloned.
-	mockassert.CalledN(t, gsStore.SetCloneStatusFunc, 2)
-	mockassert.CalledWith(t, gsStore.SetCloneStatusFunc, mockassert.Values(mockassert.Skip, repo, types.CloneStatusCloning, "test-shard"))
-	mockassert.CalledWith(t, gsStore.SetCloneStatusFunc, mockassert.Values(mockassert.Skip, repo, types.CloneStatusCloned, "test-shard"))
+	mockassert.CalledN(t, gsStore.SetCloningFunc, 1)
+	mockassert.CalledN(t, gsStore.SetClonedFunc, 1)
+	mockassert.CalledWith(t, gsStore.SetCloningFunc, mockassert.Values(mockassert.Skip, repo, "test-shard"))
+	mockassert.CalledWith(t, gsStore.SetClonedFunc, mockassert.Values(mockassert.Skip, repo, "test-shard"))
 
 	// Last output should have been stored for the repo.
 	mockrequire.CalledOnce(t, gsStore.SetLastOutputFunc)
@@ -212,7 +212,9 @@ func TestClone_Fail(t *testing.T) {
 	require.Contains(t, gsStore.SetLastErrorFunc.History()[0].Arg2, "exit status 128")
 
 	// And no other DB activity has happened.
-	mockassert.NotCalled(t, gsStore.SetCloneStatusFunc)
+	mockassert.NotCalled(t, gsStore.SetClonedFunc)
+	mockassert.NotCalled(t, gsStore.SetCloningFunc)
+	mockassert.NotCalled(t, gsStore.SetNotClonedFunc)
 	mockassert.NotCalled(t, gsStore.SetLastOutputFunc)
 
 	// ===================
@@ -246,9 +248,10 @@ func TestClone_Fail(t *testing.T) {
 	mockassert.CalledN(t, lock.ReleaseFunc, 2)
 
 	// Check it was set to cloning first, then uncloned again (since clone failed).
-	mockassert.CalledN(t, gsStore.SetCloneStatusFunc, 2)
-	mockassert.CalledWith(t, gsStore.SetCloneStatusFunc, mockassert.Values(mockassert.Skip, repo, types.CloneStatusCloning, "test-shard"))
-	mockassert.CalledWith(t, gsStore.SetCloneStatusFunc, mockassert.Values(mockassert.Skip, repo, types.CloneStatusNotCloned, "test-shard"))
+	mockassert.CalledN(t, gsStore.SetCloningFunc, 1)
+	mockassert.CalledN(t, gsStore.SetNotClonedFunc, 1)
+	mockassert.CalledWith(t, gsStore.SetCloningFunc, mockassert.Values(mockassert.Skip, repo, "test-shard"))
+	mockassert.CalledWith(t, gsStore.SetNotClonedFunc, mockassert.Values(mockassert.Skip, repo))
 
 	// Last output should have been stored for the repo.
 	mockrequire.CalledOnce(t, gsStore.SetLastOutputFunc)

cmd/gitserver/internal/integration_tests/utils_test.go

@@ -29,7 +29,6 @@ import (
 	"github.com/sourcegraph/sourcegraph/internal/api"
 	"github.com/sourcegraph/sourcegraph/internal/database/dbmocks"
 	"github.com/sourcegraph/sourcegraph/internal/extsvc"
-	"github.com/sourcegraph/sourcegraph/internal/gitserver"
 	proto "github.com/sourcegraph/sourcegraph/internal/gitserver/v1"
 	internalgrpc "github.com/sourcegraph/sourcegraph/internal/grpc"
 	"github.com/sourcegraph/sourcegraph/internal/grpc/defaults"
@@ -44,9 +43,8 @@ var root string
 // This is a default gitserver test client currently used for RequestRepoUpdate
 // gitserver calls during invocation of MakeGitRepository function
 var (
-	testGitserverClient gitserver.Client
-	GitserverAddresses  []string
-	testServer          *server.Server
+	GitserverAddresses []string
+	testServer         *server.Server
 )
 
 func InitGitserver() {
@@ -135,8 +133,6 @@ func InitGitserver() {
 	}()
 
 	serverAddress := l.Addr().String()
-	source := gitserver.NewTestClientSource(&t, []string{serverAddress})
-	testGitserverClient = gitserver.NewTestClient(&t).WithClientSource(source)
 	GitserverAddresses = []string{serverAddress}
 	testServer = s
 }

cmd/gitserver/internal/repo_info.go

@@ -7,7 +7,6 @@ import (
 	"github.com/sourcegraph/sourcegraph/internal/api"
 	"github.com/sourcegraph/sourcegraph/internal/database"
 	"github.com/sourcegraph/sourcegraph/internal/gitserver/protocol"
-	"github.com/sourcegraph/sourcegraph/internal/types"
 	"github.com/sourcegraph/sourcegraph/lib/errors"
 )
 
@@ -35,7 +34,6 @@ func repoCloneProgress(fs gitserverfs.FS, locker RepositoryLocker, repo api.Repo
 func deleteRepo(
 	ctx context.Context,
 	db database.DB,
-	shardID string,
 	fs gitserverfs.FS,
 	repo api.RepoName,
 ) error {
@@ -44,7 +42,7 @@ func deleteRepo(
 		return errors.Wrap(err, "removing repo directory")
 	}
 
-	err = db.GitserverRepos().SetCloneStatus(ctx, repo, types.CloneStatusNotCloned, shardID)
+	err = db.GitserverRepos().SetNotCloned(ctx, repo)
 	if err != nil {
 		return errors.Wrap(err, "setting clone status after delete")
 	}

cmd/gitserver/internal/repo_info_test.go

@@ -92,7 +92,7 @@ func testDeleteRepo(t *testing.T, deletedInDB bool) {
 	}
 
 	// Now we can delete it
-	require.NoError(t, deleteRepo(ctx, db, "", s.fs, dbRepo.Name))
+	require.NoError(t, deleteRepo(ctx, db, s.fs, dbRepo.Name))
 
 	size, err = s.fs.DirSize(string(s.fs.RepoDir(repoName)))
 	require.NoError(t, err)

cmd/gitserver/internal/repositoryservice.go

@@ -72,7 +72,7 @@ func (s *repositoryServiceServer) DeleteRepository(ctx context.Context, req *pro
 		return nil, status.New(codes.NotFound, "repository not found").Err()
 	}
 
-	if err := deleteRepo(ctx, s.db, s.hostname, s.fs, repoName); err != nil {
+	if err := deleteRepo(ctx, s.db, s.fs, repoName); err != nil {
 		s.logger.Error("failed to delete repository", log.String("repo", string(repoName)), log.Error(err))
 		return &proto.DeleteRepositoryResponse{}, status.Errorf(codes.Internal, "failed to delete repository %s: %s", repoName, err)
 	}
@@ -121,6 +121,8 @@ func (s *repositoryServiceServer) FetchRepository(req *proto.FetchRepositoryRequ
 
 	lastFetched, lastChanged, err := s.svc.FetchRepository(ss.Context(), repoName)
 	if err != nil {
+		s.svc.LogIfCorrupt(context.Background(), repoName, err)
+
 		return status.New(codes.Internal, errors.Wrap(err, "failed to fetch repository").Error()).Err()
 	}
 

cmd/gitserver/internal/server.go

@@ -35,7 +35,6 @@ import (
 	"github.com/sourcegraph/sourcegraph/internal/gitserver/protocol"
 	"github.com/sourcegraph/sourcegraph/internal/limiter"
 	"github.com/sourcegraph/sourcegraph/internal/ratelimit"
-	"github.com/sourcegraph/sourcegraph/internal/types"
 	"github.com/sourcegraph/sourcegraph/internal/vcs"
 	"github.com/sourcegraph/sourcegraph/internal/wrexec"
 	"github.com/sourcegraph/sourcegraph/lib/errors"
@@ -468,21 +467,21 @@ func (s *Server) cloneRepo(ctx context.Context, repo api.RepoName, lock Reposito
 	defer os.RemoveAll(tmpDir)
 	tmpPath := filepath.Join(tmpDir, ".git")
 
-	if err := s.db.GitserverRepos().SetCloneStatus(ctx, repo, types.CloneStatusCloning, s.hostname); err != nil {
+	// Mark as cloning in the DB.
+	if err := s.db.GitserverRepos().SetCloning(ctx, repo, s.hostname); err != nil {
 		s.logger.Error("Setting clone status in DB", log.Error(err))
 	}
 	defer func() {
-		cloned, err := s.fs.RepoCloned(repo)
-		if err != nil {
-			s.logger.Error("failed to check if repo is cloned", log.Error(err))
-		} else if err := s.db.GitserverRepos().SetCloneStatus(
-			// Use a background context to ensure we still update the DB even if we time out
-			context.Background(),
-			repo,
-			cloneStatus(cloned, false),
-			s.hostname,
-		); err != nil {
-			s.logger.Error("Setting clone status in DB", log.Error(err))
+		// If the clone was a success, mark the repo as cloned in the DB.
+		if err == nil {
+			if err := s.db.GitserverRepos().SetCloned(
+				// Use a background context to ensure we still update the DB even if we time out
+				context.Background(),
+				repo,
+				s.hostname,
+			); err != nil {
+				s.logger.Error("Setting clone status in DB", log.Error(err))
+			}
 		}
 	}()
 

cmd/gitserver/internal/server_test.go

@@ -12,6 +12,8 @@ import (
 	"strings"
 	"testing"
 
+	"github.com/keegancsmith/sqlf"
+
 	"github.com/sourcegraph/sourcegraph/internal/actor"
 
 	"github.com/google/go-cmp/cmp"
@@ -887,9 +889,9 @@ func TestSyncRepoState(t *testing.T) {
 
 	t.Run("sync deleted repo", func(t *testing.T) {
 		// Fake setting an incorrect status
-		if err := db.GitserverRepos().SetCloneStatus(ctx, dbRepo.Name, types.CloneStatusUnknown, hostname); err != nil {
-			t.Fatal(err)
-		}
+		q := sqlf.Sprintf("UPDATE gitserver_repos SET clone_status = %s WHERE repo_id = (SELECT id FROM repo where name = %s)", "unknown", dbRepo.Name)
+		_, err := db.ExecContext(ctx, q.Query(sqlf.PostgresBindVar), q.Args()...)
+		require.NoError(t, err)
 
 		// We should continue to sync deleted repos
 		if err := db.Repos().Delete(ctx, dbRepo.ID); err != nil {