OAuth: sync collaborators of repositories with projects only on org events

Listing collaborators costs at least one API request per repository,
which is the dominant cost of a full installation sync, and makes large
installations exceed GitHub's per-installation rate limit.

Syncs triggered by organization events now list collaborators only for
repositories linked to a project, which is what SSO access and
maintainer listings need fresh. Collaborators of other repositories
only feed the list of repositories available to import, and keep being
refreshed when each user signs in or manually re-syncs.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LVjkPjH3EDEQmtywJvDVau

Author: claude

readthedocs/oauth/services/githubapp.py

@@ -173,7 +173,7 @@ def sync_user_access(cls, user):
         if has_error:
             raise SyncServiceError()
 
-    def sync(self):
+    def sync(self, *, sync_all_collaborators=True):
         """
         Sync all repositories and organizations that are accessible to the installation.
 
@@ -182,6 +182,13 @@ def sync(self):
 
         If a remote organization doesn't have any repositories after removing the repositories,
         we remove the organization from the database.
+
+        :param sync_all_collaborators: If ``False``, collaborators are synced only for
+         repositories that are linked to a project. Listing collaborators costs at least
+         one API request per repository, which makes a full sync of a large installation
+         exceed the GitHub API rate limit. Collaborators of repositories without a project
+         are only used to build the list of repositories available to import, which is
+         refreshed when each user signs in or manually re-syncs their repositories.
         """
         try:
             app_installation = self.get_app_installation()
@@ -207,9 +214,22 @@ def sync(self):
             self.installation.delete()
             raise SyncServiceError()
 
+        repos_with_projects = set()
+        if not sync_all_collaborators:
+            repos_with_projects = set(
+                self.installation.repositories.filter(projects__isnull=False).values_list(
+                    "remote_id", flat=True
+                )
+            )
+
         remote_repositories = []
         for gh_repo in app_installation.get_repos():
-            remote_repo = self._create_or_update_repository_from_gh(gh_repo)
+            remote_repo = self._create_or_update_repository_from_gh(
+                gh_repo,
+                resync_collaborators=(
+                    sync_all_collaborators or str(gh_repo.id) in repos_with_projects
+                ),
+            )
             if remote_repo:
                 remote_repositories.append(remote_repo)
 
@@ -265,12 +285,13 @@ def update_or_create_repositories(self, repository_ids: list[int]):
             self.installation.delete_repositories(repositories_to_delete)
 
     def _create_or_update_repository_from_gh(
-        self, gh_repo: GHRepository
+        self, gh_repo: GHRepository, *, resync_collaborators: bool = True
     ) -> RemoteRepository | None:
         """
         Create or update a remote repository from a GitHub repository object.
 
-        We also sync the collaborators of the repository with the database,
+        We also sync the collaborators of the repository with the database
+        (unless ``resync_collaborators`` is ``False``),
         and create or update the organization of the repository.
         """
         target_id = self.installation.target_id
@@ -323,7 +344,8 @@ def _create_or_update_repository_from_gh(
             remote_repo.organization = self.update_or_create_organization(gh_repo.owner.login)
 
         remote_repo.save()
-        self._resync_collaborators(gh_repo, remote_repo)
+        if resync_collaborators:
+            self._resync_collaborators(gh_repo, remote_repo)
         return remote_repo
 
     # NOTE: normally, this should cache only one organization at a time, but just in case...

readthedocs/oauth/tasks.py

@@ -756,32 +756,35 @@ def _handle_organization_event(self):
         if created:
             return
 
-        # We need to do a full sync of the repositories if members were added or removed,
+        # We need to re-sync the repositories if members were added or removed,
         # this is since we don't know to which repositories the members have access.
         # GH doesn't send a member event for this.
         if action in ("member_added", "member_removed"):
             if action == "member_removed":
                 # We know which member was removed, so we can revoke their access
-                # without querying the GH API. The sync below covers members whose
-                # access changed in other ways, but it may be skipped completely
+                # without querying the GH API. The sync below only covers
+                # repositories linked to a project, and may be skipped completely
                 # if the installation was synced recently.
                 self._remove_member_repository_relations(installation)
 
             # Bulk membership changes send one event per member, and each sync costs
             # at least one API request per repository in the installation, so we
             # debounce syncs to avoid exceeding the GitHub API rate limit.
+            # Collaborators of repositories without a project are refreshed when
+            # each user signs in or manually re-syncs their repositories.
             if self._installation_sync_recently_triggered(installation):
                 log.info("Installation was synced recently, skipping sync.")
                 return
-            installation.service.sync()
+            installation.service.sync(sync_all_collaborators=False)
             return
 
         # NOTE: installation_target should handle this instead?
         # But I wasn't able to trigger neither of those events when renaming an organization.
         # Maybe a bug?
         # If the organization is renamed, we need to sync the repositories, so they use the new name.
+        # Renaming doesn't change permissions, so we don't need to sync all collaborators.
         if action == "renamed":
-            installation.service.sync()
+            installation.service.sync(sync_all_collaborators=False)
             return
 
         if action == "deleted":

readthedocs/oauth/tests/test_githubapp_webhook.py

@@ -917,7 +917,7 @@ def test_organization_member_added(self, sync):
         }
         r = self.post_webhook("organization", payload)
         assert r.status_code == 200
-        sync.assert_called_once()
+        sync.assert_called_once_with(sync_all_collaborators=False)
 
     @mock.patch.object(GitHubAppService, "sync")
     def test_organization_member_events_are_debounced(self, sync):
@@ -935,7 +935,7 @@ def test_organization_member_events_are_debounced(self, sync):
         assert r.status_code == 200
         # Bulk membership changes send one event per member,
         # only the first one triggers a sync.
-        sync.assert_called_once()
+        sync.assert_called_once_with(sync_all_collaborators=False)
 
     @mock.patch.object(GitHubAppService, "sync")
     def test_organization_member_removed(self, sync):
@@ -974,7 +974,7 @@ def test_organization_member_removed(self, sync):
         }
         r = self.post_webhook("organization", payload)
         assert r.status_code == 200
-        sync.assert_called_once()
+        sync.assert_called_once_with(sync_all_collaborators=False)
         # The relations from the removed member are deleted without querying the API,
         # relations from other users are kept.
         assert not member.remote_repository_relations.exists()
@@ -992,7 +992,7 @@ def test_organization_renamed(self, sync):
         }
         r = self.post_webhook("organization", payload)
         assert r.status_code == 200
-        sync.assert_called_once()
+        sync.assert_called_once_with(sync_all_collaborators=False)
 
     def test_organization_deleted(self):
         organization_id = 1234

readthedocs/rtd_tests/tests/test_oauth.py

@@ -674,6 +674,61 @@ def test_sync(self, request):
         assert relation.account == self.account
         assert relation.admin
 
+    @requests_mock.Mocker(kw="request")
+    def test_sync_collaborators_from_repositories_with_projects_only(self, request):
+        # ``self.remote_repository`` is linked to a project, ``repo2`` is not.
+        repo2 = get(
+            RemoteRepository,
+            remote_id="7777",
+            name="repo2",
+            full_name="user/repo2",
+            vcs_provider=GITHUB_APP,
+            github_app_installation=self.installation,
+        )
+        get(
+            RemoteRepositoryRelation,
+            remote_repository=repo2,
+            user=self.user,
+            account=self.account,
+            admin=True,
+        )
+        request.get(
+            f"{self.api_url}/app/installations/1111",
+            json=self._get_installation_json(id=1111),
+        )
+        request.post(
+            f"{self.api_url}/app/installations/1111/access_tokens",
+            json=self._get_access_token_json(),
+        )
+        request.get(
+            f"{self.api_url}/installation/repositories",
+            json={
+                "repositories": [
+                    self._get_repository_json(
+                        full_name="user/repo", id=int(self.remote_repository.remote_id)
+                    ),
+                    self._get_repository_json(
+                        full_name="user/repo2", id=7777, description="New description"
+                    ),
+                ]
+            },
+        )
+        # Only the repository linked to a project has its collaborators listed,
+        # requesting the collaborators of user/repo2 would fail the test (not mocked).
+        request.get(
+            f"{self.api_url}/repos/user/repo/collaborators",
+            json=[self._get_collaborator_json()],
+        )
+
+        service = self.installation.service
+        service.sync(sync_all_collaborators=False)
+
+        # The metadata of the repository without projects is still updated,
+        # and its existing relations are kept.
+        repo2.refresh_from_db()
+        assert repo2.description == "New description"
+        assert repo2.remote_repository_relations.count() == 1
+
     @requests_mock.Mocker(kw="request")
     def test_sync_delete_remote_repositories(self, request):
         assert self.installation.repositories.count() == 1