Repo Sync: If all errors are warnings, delete repos with bad permissi…

…ons (#40690)

* Prevent stopping sync if a querying for repos in Bitbucket project key returns a 'fatal' error

internal/repos/syncer.go

@@ -636,7 +636,6 @@ func (s *Syncer) SyncExternalService(
 
 			continue
 		}
-
 		for _, r := range diff.Repos() {
 			seen[r.ID] = struct{}{}
 		}
@@ -661,22 +660,46 @@ func (s *Syncer) SyncExternalService(
 	}
 
 	// We don't delete any repos of site-level external services if there were any
-	// errors during a sync.
+	// non-warning errors during a sync.
 	//
 	// Only user or organization external services will delete
 	// repos in a sync run with fatal errors.
 	//
 	// Site-level external services can own lots of repos and are managed by site admins.
 	// It's preferable to have them fix any invalidated token manually rather than deleting the repos automatically.
 	deleted := 0
-	if errs == nil || (!svc.IsSiteOwned() && fatal(errs)) {
+
+	// If all of our errors are warnings and either Forbidden or Unauthorized,
+	// we want to proceed with the deletion. This is to be able to properly sync
+	// repos (by removing ones if code-host permissions have changed).
+	abortDeletion := false
+	if errs != nil {
+		var ref errors.MultiError
+		if errors.As(errs, &ref) {
+			for _, e := range ref.Errors() {
+				if errors.IsWarning(e) {
+					baseError := errors.Unwrap(e)
+					if !errcode.IsForbidden(baseError) && !errcode.IsUnauthorized(baseError) {
+						abortDeletion = true
+						break
+					}
+					continue
+				}
+				abortDeletion = true
+				break
+			}
+		}
+	}
+
+	if !abortDeletion || (!svc.IsSiteOwned() && fatal(errs)) {
 		// Remove associations and any repos that are no longer associated with any
 		// external service.
 		//
 		// We don't want to delete all repos that weren't seen if we had a lot of
 		// spurious errors since that could cause lots of repos to be deleted, only to be
-		// added the next sync. We delete only if we had no errors or we had one of the
-		// fatal errors.
+		// added the next sync. We delete only if we had no errors,
+		// or all of our errors are warnings and either Forbidden or Unauthorized,
+		// or we had one of the fatal errors and the service is not site owned.
 		var deletedErr error
 		deleted, deletedErr = s.delete(ctx, svc, seen)
 		if deletedErr != nil {

internal/repos/syncer_test.go

@@ -268,6 +268,31 @@ func testSyncerSync(s repos.Store) func(*testing.T) {
 					svcs: []*types.ExternalService{tc.svc},
 					err:  "bad credentials",
 				},
+				testCase{
+					// If the source is unauthorized with a warning rather than an error,
+					// the sync will continue. If the warning error is unauthorized, the
+					// corresponding repos will be deleted as it's seen as permissions changes.
+					name: string(tc.repo.Name) + "/unauthorized-with-warning",
+					sourcer: repos.NewFakeSourcer(nil,
+						repos.NewFakeSource(tc.svc.Clone(), errors.NewWarningError(&repos.ErrUnauthorized{})),
+					),
+					store: s,
+					stored: types.Repos{tc.repo.With(
+						typestest.Opt.RepoSources(tc.svc.URN()),
+					)},
+					now: clock.Now,
+					diff: repos.Diff{
+						Deleted: types.Repos{
+							tc.repo.With(func(r *types.Repo) {
+								r.Sources = map[string]*types.SourceInfo{}
+								r.DeletedAt = clock.Time(0)
+								r.UpdatedAt = clock.Time(0)
+							}),
+						},
+					},
+					svcs: []*types.ExternalService{tc.svc},
+					err:  "bad credentials",
+				},
 				testCase{
 					// If the source is forbidden we should treat this as if zero repos were returned
 					// as it indicates that the source no longer has access to its repos
@@ -286,6 +311,31 @@ func testSyncerSync(s repos.Store) func(*testing.T) {
 					svcs: []*types.ExternalService{tc.svc},
 					err:  "forbidden",
 				},
+				testCase{
+					// If the source is forbidden with a warning rather than an error,
+					// the sync will continue. If the warning error is forbidden, the
+					// corresponding repos will be deleted as it's seen as permissions changes.
+					name: string(tc.repo.Name) + "/forbidden-with-warning",
+					sourcer: repos.NewFakeSourcer(nil,
+						repos.NewFakeSource(tc.svc.Clone(), errors.NewWarningError(&repos.ErrForbidden{})),
+					),
+					store: s,
+					stored: types.Repos{tc.repo.With(
+						typestest.Opt.RepoSources(tc.svc.URN()),
+					)},
+					now: clock.Now,
+					diff: repos.Diff{
+						Deleted: types.Repos{
+							tc.repo.With(func(r *types.Repo) {
+								r.Sources = map[string]*types.SourceInfo{}
+								r.DeletedAt = clock.Time(0)
+								r.UpdatedAt = clock.Time(0)
+							}),
+						},
+					},
+					svcs: []*types.ExternalService{tc.svc},
+					err:  "forbidden",
+				},
 				testCase{
 					// If the source account has been suspended we should treat this as if zero repos were returned as it indicates
 					// that the source no longer has access to its repos
@@ -304,6 +354,22 @@ func testSyncerSync(s repos.Store) func(*testing.T) {
 					svcs: []*types.ExternalService{tc.svc},
 					err:  "account suspended",
 				},
+				testCase{
+					// If the source is account suspended with a warning rather than an error,
+					// the sync will terminate. This is the only warning error that the sync will abort
+					name: string(tc.repo.Name) + "/accountsuspended-with-warning",
+					sourcer: repos.NewFakeSourcer(nil,
+						repos.NewFakeSource(tc.svc.Clone(), errors.NewWarningError(&repos.ErrAccountSuspended{})),
+					),
+					store: s,
+					stored: types.Repos{tc.repo.With(
+						typestest.Opt.RepoSources(tc.svc.URN()),
+					)},
+					now:  clock.Now,
+					diff: diff,
+					svcs: []*types.ExternalService{tc.svc},
+					err:  "account suspended",
+				},
 				testCase{
 					// Test that spurious errors don't cause deletions.
 					name: string(tc.repo.Name) + "/spurious-error",
@@ -321,6 +387,24 @@ func testSyncerSync(s repos.Store) func(*testing.T) {
 					svcs: []*types.ExternalService{tc.svc},
 					err:  io.EOF.Error(),
 				},
+				testCase{
+					// If the source is a spurious error with a warning rather than an error,
+					// the sync will continue. However, no repos will be deleted.
+					name: string(tc.repo.Name) + "/spurious-error-with-warning",
+					sourcer: repos.NewFakeSourcer(nil,
+						repos.NewFakeSource(tc.svc.Clone(), errors.NewWarningError(io.EOF)),
+					),
+					store: s,
+					stored: types.Repos{tc.repo.With(
+						typestest.Opt.RepoSources(tc.svc.URN()),
+					)},
+					now: clock.Now,
+					diff: repos.Diff{Unmodified: types.Repos{tc.repo.With(
+						typestest.Opt.RepoSources(tc.svc.URN()),
+					)}},
+					svcs: []*types.ExternalService{tc.svc},
+					err:  io.EOF.Error(),
+				},
 				testCase{
 					// It's expected that there could be multiple stored sources but only one will ever be returned
 					// by the code host as it can't know about others.

lib/errors/warning.go

@@ -69,6 +69,11 @@ func (w *warning) IsWarning() bool {
 	return true
 }
 
+// Unwrap returns the underlying error of the warning.
+func (w *warning) Unwrap() error {
+	return w.error
+}
+
 // As will return true if the target is of type warning.
 //
 // However, this method is not invoked when `errors.As` is invoked. See note in the docstring of the