diff --git a/CHANGELOG.md b/CHANGELOG.md index f1ab675163f..aa44e79d769 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -30,6 +30,7 @@ All notable changes to Sourcegraph are documented in this file. - Code Monitors now properly ignores monitors associated with soft-deleted users, which previously would have led to an error on the overview page. [#60405](https://github.com/sourcegraph/sourcegraph/pull/60405) - Fixed a bug where clicking "Exclude Repo" on Azure DevOps or Gerrit repositories would not work. [#60509](https://github.com/sourcegraph/sourcegraph/pull/60509) - Links in codeintel popovers respect the revision from the URL. [#60545](https://github.com/sourcegraph/sourcegraph/pull/60545) +- Fixed a bug where permission syncs could be scheduled for repositories or users even when a sync is already scheduled or in progress, leading to significant delays in the permissions sync system as a whole. [#61024](https://github.com/sourcegraph/sourcegraph/pull/61024) ### Removed diff --git a/cmd/worker/internal/authz/perms_syncer.go b/cmd/worker/internal/authz/perms_syncer.go index 28fd8258dad..5c0541aa621 100644 --- a/cmd/worker/internal/authz/perms_syncer.go +++ b/cmd/worker/internal/authz/perms_syncer.go @@ -175,7 +175,6 @@ func (s *permsSyncerImpl) syncRepoPerms(ctx context.Context, repoID api.RepoID, ServiceID: provider.ServiceID(), AccountIDs: accountIDs, }) - if err != nil { return result, providerStates, errors.Wrapf(err, "get user IDs by external accounts for repository %q (id: %d)", repo.Name, repo.ID) } diff --git a/internal/database/perms_store.go b/internal/database/perms_store.go index ad46d78f8e8..6cf32be0d1c 100644 --- a/internal/database/perms_store.go +++ b/internal/database/perms_store.go @@ -1607,13 +1607,17 @@ SELECT u.id as user_id, MAX(p.finished_at) as finished_at FROM users u LEFT JOIN permission_sync_jobs p ON u.id = p.user_id AND p.user_id IS NOT NULL WHERE u.deleted_at IS NULL AND (%s) +AND NOT EXISTS ( + SELECT 1 FROM permission_sync_jobs p2 + WHERE p2.user_id = u.id AND (p2.state = 'queued' OR p2.state = 'processing') +) GROUP BY u.id ORDER BY finished_at ASC NULLS FIRST, user_id ASC LIMIT %d; ` // UserIDsWithOldestPerms lists the users with the oldest synced perms, limited -// to limit. If age is non-zero, users that have synced within "age" since now +// to limit, for which there is no sync job scheduled at the moment. If age is non-zero, users that have synced within "age" since now // will be filtered out. func (s *permsStore) UserIDsWithOldestPerms(ctx context.Context, limit int, age time.Duration) (map[int32]time.Time, error) { q := sqlf.Sprintf(usersWithOldestPermsQuery, s.getCutoffClause(age), limit) @@ -1625,11 +1629,18 @@ SELECT r.id as repo_id, MAX(p.finished_at) as finished_at FROM repo r LEFT JOIN permission_sync_jobs p ON r.id = p.repository_id AND p.repository_id IS NOT NULL WHERE r.private AND r.deleted_at IS NULL AND (%s) +AND NOT EXISTS ( + SELECT 1 FROM permission_sync_jobs p2 + WHERE p2.repository_id = r.id AND (p2.state = 'queued' OR p2.state = 'processing') +) GROUP BY r.id ORDER BY finished_at ASC NULLS FIRST, repo_id ASC LIMIT %d; ` +// ReposIDsWithOldestPerms lists the repositories with the oldest synced perms, limited +// to limit, for which there is no sync job scheduled at the moment. If age is non-zero, repos that have synced within "age" since now +// will be filtered out. func (s *permsStore) ReposIDsWithOldestPerms(ctx context.Context, limit int, age time.Duration) (map[api.RepoID]time.Time, error) { q := sqlf.Sprintf(reposWithOldestPermsQuery, s.getCutoffClause(age), limit) diff --git a/internal/database/perms_store_test.go b/internal/database/perms_store_test.go index 13aa551439f..92a6cce0fcc 100644 --- a/internal/database/perms_store_test.go +++ b/internal/database/perms_store_test.go @@ -3388,6 +3388,8 @@ func TestPermsStore_UserIDsWithOldestPerms(t *testing.T) { sqlf.Sprintf(`INSERT INTO users(id, username) VALUES(1, 'alice')`), sqlf.Sprintf(`INSERT INTO users(id, username) VALUES(2, 'bob')`), sqlf.Sprintf(`INSERT INTO users(id, username, deleted_at) VALUES(3, 'cindy', NOW())`), + sqlf.Sprintf(`INSERT INTO users(id, username) VALUES(4, 'david')`), + sqlf.Sprintf(`INSERT INTO users(id, username) VALUES(5, 'erica')`), } for _, q := range qs { executeQuery(t, ctx, s, q) @@ -3397,15 +3399,20 @@ func TestPermsStore_UserIDsWithOldestPerms(t *testing.T) { user1UpdatedAt := clock().Add(-15 * time.Minute) user2UpdatedAt := clock().Add(-5 * time.Minute) user3UpdatedAt := clock().Add(-11 * time.Minute) - q := sqlf.Sprintf(`INSERT INTO permission_sync_jobs(user_id, finished_at, reason) VALUES(%d, %s, %s)`, 1, user1UpdatedAt, ReasonUserOutdatedPermissions) + q := sqlf.Sprintf(`INSERT INTO permission_sync_jobs(user_id, state, finished_at, reason) VALUES(%d, 'completed', %s, %s)`, 1, user1UpdatedAt, ReasonUserOutdatedPermissions) executeQuery(t, ctx, s, q) - q = sqlf.Sprintf(`INSERT INTO permission_sync_jobs(user_id, finished_at, reason) VALUES(%d, %s, %s)`, 2, user2UpdatedAt, ReasonUserOutdatedPermissions) + q = sqlf.Sprintf(`INSERT INTO permission_sync_jobs(user_id, state, finished_at, reason) VALUES(%d, 'completed', %s, %s)`, 2, user2UpdatedAt, ReasonUserOutdatedPermissions) executeQuery(t, ctx, s, q) - q = sqlf.Sprintf(`INSERT INTO permission_sync_jobs(user_id, finished_at, reason) VALUES(%d, %s, %s)`, 3, user3UpdatedAt, ReasonUserOutdatedPermissions) + q = sqlf.Sprintf(`INSERT INTO permission_sync_jobs(user_id, state, finished_at, reason) VALUES(%d, 'completed', %s, %s)`, 3, user3UpdatedAt, ReasonUserOutdatedPermissions) + executeQuery(t, ctx, s, q) + q = sqlf.Sprintf(`INSERT INTO permission_sync_jobs(user_id, state, reason) VALUES(%d, 'queued', %s)`, 4, ReasonUserOutdatedPermissions) + executeQuery(t, ctx, s, q) + q = sqlf.Sprintf(`INSERT INTO permission_sync_jobs(user_id, state, reason) VALUES(%d, 'processing', %s)`, 5, ReasonUserOutdatedPermissions) executeQuery(t, ctx, s, q) - t.Run("One result when limit is 1", func(t *testing.T) { + t.Run("One result when limit is 1. Queued user ignored", func(t *testing.T) { // Should only get user 1 back, because limit is 1 + // Users with queued and processing permissions syncs are ignored results, err := s.UserIDsWithOldestPerms(ctx, 1, 0) if err != nil { t.Fatal(err) @@ -3559,6 +3566,8 @@ func TestPermsStore_ReposIDsWithOldestPerms(t *testing.T) { sqlf.Sprintf(`INSERT INTO repo(id, name, private) VALUES(1, 'private_repo_1', TRUE)`), // id=1 sqlf.Sprintf(`INSERT INTO repo(id, name, private) VALUES(2, 'private_repo_2', TRUE)`), // id=2 sqlf.Sprintf(`INSERT INTO repo(id, name, private, deleted_at) VALUES(3, 'private_repo_3', TRUE, NOW())`), // id=3 + sqlf.Sprintf(`INSERT INTO repo(id, name, private) VALUES(4, 'private_repo_4', TRUE)`), // id=2 + sqlf.Sprintf(`INSERT INTO repo(id, name, private) VALUES(5, 'private_repo_5', TRUE)`), // id=2 } for _, q := range qs { executeQuery(t, ctx, s, q) @@ -3568,15 +3577,20 @@ func TestPermsStore_ReposIDsWithOldestPerms(t *testing.T) { repo1UpdatedAt := clock().Add(-15 * time.Minute) repo2UpdatedAt := clock().Add(-5 * time.Minute) repo3UpdatedAt := clock().Add(-10 * time.Minute) - q := sqlf.Sprintf(`INSERT INTO permission_sync_jobs(repository_id, finished_at, reason) VALUES(%d, %s, %s)`, 1, repo1UpdatedAt, ReasonRepoOutdatedPermissions) + q := sqlf.Sprintf(`INSERT INTO permission_sync_jobs(repository_id, state, finished_at, reason) VALUES(%d, 'completed', %s, %s)`, 1, repo1UpdatedAt, ReasonRepoOutdatedPermissions) executeQuery(t, ctx, s, q) - q = sqlf.Sprintf(`INSERT INTO permission_sync_jobs(repository_id, finished_at, reason) VALUES(%d, %s, %s)`, 2, repo2UpdatedAt, ReasonRepoOutdatedPermissions) + q = sqlf.Sprintf(`INSERT INTO permission_sync_jobs(repository_id, state, finished_at, reason) VALUES(%d, 'completed', %s, %s)`, 2, repo2UpdatedAt, ReasonRepoOutdatedPermissions) executeQuery(t, ctx, s, q) - q = sqlf.Sprintf(`INSERT INTO permission_sync_jobs(repository_id, finished_at, reason) VALUES(%d, %s, %s)`, 3, repo3UpdatedAt, ReasonRepoOutdatedPermissions) + q = sqlf.Sprintf(`INSERT INTO permission_sync_jobs(repository_id, state, finished_at, reason) VALUES(%d, 'completed', %s, %s)`, 3, repo3UpdatedAt, ReasonRepoOutdatedPermissions) + executeQuery(t, ctx, s, q) + q = sqlf.Sprintf(`INSERT INTO permission_sync_jobs(repository_id, state, reason) VALUES(%d, 'queued', %s)`, 4, ReasonRepoOutdatedPermissions) + executeQuery(t, ctx, s, q) + q = sqlf.Sprintf(`INSERT INTO permission_sync_jobs(repository_id, state, reason) VALUES(%d, 'processing', %s)`, 5, ReasonRepoOutdatedPermissions) executeQuery(t, ctx, s, q) t.Run("One result when limit is 1", func(t *testing.T) { // Should only get private_repo_1 back, because limit is 1 + // Repos with jobs queued or in progress are ignored results, err := s.ReposIDsWithOldestPerms(ctx, 1, 0) if err != nil { t.Fatal(err)