Fix context canceled errors when triggering manual sync (#41265)

* Fix context cancelled error when triggering manual sync

cmd/repo-updater/repoupdater/server.go

@@ -193,16 +193,14 @@ func (s *Server) handleExternalServiceSync(w http.ResponseWriter, r *http.Reques
 	}
 	logger := s.Logger.With(log.Int64("ExternalServiceID", req.ExternalServiceID))
 
-	var sourcer repos.Sourcer
-	if sourcer = s.Sourcer; sourcer == nil {
-		sourcerLogger := logger.Scoped("repos.Sourcer", "repositories source")
-
-		db := database.NewDBWith(sourcerLogger.Scoped("db", "sourcer database"), s)
-		depsSvc := livedependencies.GetService(db)
-		cf := httpcli.NewExternalClientFactory(httpcli.NewLoggingMiddleware(sourcerLogger))
-
-		sourcer = repos.NewSourcer(sourcerLogger, db, cf, repos.WithDependenciesService(depsSvc))
-	}
+	// We use the generic sourcer that doesn't have observability attached to it here because the way externalServiceValidate is set up,
+	// using the regular sourcer will cause a large dump of errors to be logged when it exits ListRepos prematurely.
+	var genericSourcer repos.Sourcer
+	sourcerLogger := logger.Scoped("repos.Sourcer", "repositories source")
+	db := database.NewDBWith(sourcerLogger.Scoped("db", "sourcer database"), s)
+	depsSvc := livedependencies.GetService(db)
+	cf := httpcli.NewExternalClientFactory(httpcli.NewLoggingMiddleware(sourcerLogger))
+	genericSourcer = repos.NewSourcer(sourcerLogger, db, cf, repos.WithDependenciesService(depsSvc))
 
 	externalServiceID := req.ExternalServiceID
 
@@ -222,36 +220,20 @@ func (s *Server) handleExternalServiceSync(w http.ResponseWriter, r *http.Reques
 		return
 	}
 
-	src, err := sourcer(ctx, es[0])
-
+	genericSrc, err := genericSourcer(ctx, es[0])
 	if err != nil {
 		logger.Error("server.external-service-sync", log.Error(err))
 		return
 	}
 
-	err = externalServiceValidate(ctx, es[0], src)
-	if err == github.ErrIncompleteResults {
-		logger.Info("server.external-service-sync", log.Error(err))
-		syncResult := &protocol.ExternalServiceSyncResult{
-			Error: err.Error(),
-		}
-		s.respond(w, http.StatusOK, syncResult)
+	statusCode, resp := handleExternalServiceValidate(ctx, logger, es[0], genericSrc)
+	if statusCode > 0 {
+		s.respond(w, statusCode, resp)
 		return
-	} else if ctx.Err() != nil {
+	}
+	if statusCode == 0 {
 		// client is gone
 		return
-	} else if err != nil {
-		logger.Error("server.external-service-sync", log.Error(err))
-		if errcode.IsUnauthorized(err) {
-			s.respond(w, http.StatusUnauthorized, err)
-			return
-		}
-		if errcode.IsForbidden(err) {
-			s.respond(w, http.StatusForbidden, err)
-			return
-		}
-		s.respond(w, http.StatusInternalServerError, err)
-		return
 	}
 
 	if s.RateLimitSyncer != nil {
@@ -269,6 +251,32 @@ func (s *Server) handleExternalServiceSync(w http.ResponseWriter, r *http.Reques
 	s.respond(w, http.StatusOK, &protocol.ExternalServiceSyncResult{})
 }
 
+func handleExternalServiceValidate(ctx context.Context, logger log.Logger, es *types.ExternalService, src repos.Source) (int, any) {
+	err := externalServiceValidate(ctx, es, src)
+	if err == github.ErrIncompleteResults {
+		logger.Info("server.external-service-sync", log.Error(err))
+		syncResult := &protocol.ExternalServiceSyncResult{
+			Error: err.Error(),
+		}
+		return http.StatusOK, syncResult
+	}
+	if ctx.Err() != nil {
+		// client is gone
+		return 0, nil
+	}
+	if err != nil {
+		logger.Error("server.external-service-sync", log.Error(err))
+		if errcode.IsUnauthorized(err) {
+			return http.StatusUnauthorized, err
+		}
+		if errcode.IsForbidden(err) {
+			return http.StatusForbidden, err
+		}
+		return http.StatusInternalServerError, err
+	}
+	return -1, nil
+}
+
 func externalServiceValidate(ctx context.Context, es *types.ExternalService, src repos.Source) error {
 	if !es.DeletedAt.IsZero() {
 		// We don't need to check deleted services.

cmd/repo-updater/repoupdater/server_test.go

@@ -763,7 +763,7 @@ func TestServer_handleSchedulePermsSync(t *testing.T) {
 	}
 }
 
-func TestServer_handleExternalServiceSync(t *testing.T) {
+func TestServer_handleExternalServiceValidate(t *testing.T) {
 	tests := []struct {
 		name        string
 		err         error
@@ -793,17 +793,11 @@ func TestServer_handleExternalServiceSync(t *testing.T) {
 					return test.err
 				},
 			}
-			r := httptest.NewRequest("POST", "/sync-external-service", strings.NewReader(`{"ExternalServiceID": 1}`))
-			w := httptest.NewRecorder()
-			s := repos.NewMockStore()
-			es := database.NewMockExternalServiceStore()
-			s.ExternalServiceStoreFunc.SetDefaultReturn(es)
-			es.ListFunc.PushReturn([]*types.ExternalService{{ID: 1, Kind: extsvc.KindGitHub, Config: extsvc.NewEmptyConfig()}}, nil)
-
-			srv := &Server{Logger: logtest.Scoped(t), Store: s, Syncer: &repos.Syncer{Sourcer: repos.NewFakeSourcer(nil, src)}}
-			srv.handleExternalServiceSync(w, r)
-			if w.Code != test.wantErrCode {
-				t.Errorf("Code: want %v but got %v", test.wantErrCode, w.Code)
+
+			es := &types.ExternalService{ID: 1, Kind: extsvc.KindGitHub, Config: extsvc.NewEmptyConfig()}
+			statusCode, _ := handleExternalServiceValidate(context.Background(), logtest.Scoped(t), es, src)
+			if statusCode != test.wantErrCode {
+				t.Errorf("Code: want %v but got %v", test.wantErrCode, statusCode)
 			}
 		})
 	}