OAuth: handle organization member events without a sync per event

Bulk membership changes send one organization event per member, and
each event triggered a full sync of the installation, costing at least
one API request per repository. Large installations exceed GitHub's
per-installation rate limit this way.

Removed members are now revoked directly from the database using the
event payload, without querying the API. Syncs triggered by member
events are debounced per installation, so a bulk change triggers one
sync instead of one per member.

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

Author: claude

readthedocs/oauth/tasks.py

@@ -5,10 +5,12 @@
 
 import structlog
 from django.contrib.auth.models import User
+from django.core.cache import cache
 from django.db.models.functions import ExtractIsoWeekDay
 from django.urls import reverse
 from django.utils import timezone
 
+from readthedocs.allauth.providers.githubapp.provider import GitHubAppProvider
 from readthedocs.api.v2.views.integrations import ExternalVersionData
 from readthedocs.builds.constants import EXTERNAL
 from readthedocs.builds.utils import memcache_lock
@@ -25,6 +27,7 @@
 from readthedocs.oauth.constants import GITHUB_APP
 from readthedocs.oauth.models import GitHubAppInstallation
 from readthedocs.oauth.models import RemoteRepository
+from readthedocs.oauth.models import RemoteRepositoryRelation
 from readthedocs.oauth.notifications import MESSAGE_OAUTH_SYNCING_REMOTE_REPOSITORIES
 from readthedocs.oauth.notifications import MESSAGE_OAUTH_UNSUPPORTED_GIT_PROVIDER
 from readthedocs.oauth.notifications import MESSAGE_OAUTH_WEBHOOK_INTEGRATION_MISMATCH
@@ -757,6 +760,19 @@ def _handle_organization_event(self):
         # 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
+                # 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.
+            if self._installation_sync_recently_triggered(installation):
+                log.info("Installation was synced recently, skipping sync.")
+                return
             installation.service.sync()
             return
 
@@ -781,6 +797,39 @@ def _handle_organization_event(self):
         # - member_invited: We don't do anything with invited members.
         return
 
+    def _remove_member_repository_relations(self, installation):
+        """
+        Remove all repository relations from the member the event was triggered for.
+
+        A member removed from the organization lost access to all its repositories,
+        so we can revoke their access without querying the GH API.
+        """
+        member_id = self.data.get("membership", {}).get("user", {}).get("id")
+        if not member_id:
+            log.info("Organization event doesn't include the affected member.")
+            return
+        count, _ = RemoteRepositoryRelation.objects.filter(
+            account__provider=GitHubAppProvider.id,
+            account__uid=str(member_id),
+            remote_repository__github_app_installation=installation,
+        ).delete()
+        log.info(
+            "Removed repository relations from member removed from the organization.",
+            member_id=member_id,
+            count=count,
+        )
+
+    def _installation_sync_recently_triggered(self, installation) -> bool:
+        """
+        Check if a sync was triggered recently for the installation, and mark it as synced.
+
+        ``cache.add`` is atomic, so concurrent events trigger only one sync.
+        The key isn't removed if the sync fails, otherwise a burst of events would
+        keep triggering expensive syncs while we are rate limited.
+        """
+        cache_key = f"githubapp-installation-sync:{installation.installation_id}"
+        return not cache.add(cache_key, True, timeout=60 * 5)
+
     def _handle_member_event(self):
         """
         Handle the member event.

readthedocs/oauth/tests/test_githubapp_webhook.py

@@ -7,6 +7,7 @@
 from allauth.socialaccount.models import SocialAccount
 from django.conf import settings
 from django.contrib.auth.models import User
+from django.core.cache import cache
 from django.test import TestCase
 from django.urls import reverse
 from django_dynamic_fixture import get
@@ -32,6 +33,7 @@
     GitHubAppInstallation,
     RemoteOrganization,
     RemoteRepository,
+    RemoteRepositoryRelation,
 )
 from readthedocs.oauth.services import GitHubAppService
 from readthedocs.projects.models import Project
@@ -81,6 +83,8 @@ def setUp(self):
         self.project.remote_repository = self.remote_repository
         self.project.save()
         self.url = reverse("github_app_webhook")
+        # Syncs triggered by organization events are debounced with the cache.
+        cache.clear()
 
     def post_webhook(self, event, payload):
         headers = {
@@ -915,19 +919,66 @@ def test_organization_member_added(self, sync):
         assert r.status_code == 200
         sync.assert_called_once()
 
+    @mock.patch.object(GitHubAppService, "sync")
+    def test_organization_member_events_are_debounced(self, sync):
+        payload = {
+            "installation": {
+                "id": self.installation.installation_id,
+                "target_id": self.installation.target_id,
+                "target_type": self.installation.target_type,
+            },
+            "action": "member_added",
+        }
+        r = self.post_webhook("organization", payload)
+        assert r.status_code == 200
+        r = self.post_webhook("organization", payload)
+        assert r.status_code == 200
+        # Bulk membership changes send one event per member,
+        # only the first one triggers a sync.
+        sync.assert_called_once()
+
     @mock.patch.object(GitHubAppService, "sync")
     def test_organization_member_removed(self, sync):
+        member = get(User)
+        member_account = get(
+            SocialAccount,
+            user=member,
+            provider=GitHubAppProvider.id,
+            uid="9999",
+        )
+        get(
+            RemoteRepositoryRelation,
+            remote_repository=self.remote_repository,
+            user=member,
+            account=member_account,
+        )
+        relation = get(
+            RemoteRepositoryRelation,
+            remote_repository=self.remote_repository,
+            user=self.user,
+            account=self.socialaccount,
+        )
         payload = {
             "installation": {
                 "id": self.installation.installation_id,
                 "target_id": self.installation.target_id,
                 "target_type": self.installation.target_type,
             },
             "action": "member_removed",
+            "membership": {
+                "user": {
+                    "login": "member",
+                    "id": 9999,
+                },
+            },
         }
         r = self.post_webhook("organization", payload)
         assert r.status_code == 200
         sync.assert_called_once()
+        # 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()
+        assert list(self.user.remote_repository_relations.all()) == [relation]
 
     @mock.patch.object(GitHubAppService, "sync")
     def test_organization_renamed(self, sync):
@@ -1082,6 +1133,8 @@ def setUp(self):
         self.project.remote_repository = self.remote_repository
         self.project.save()
         self.url = reverse("github_app_webhook")
+        # Syncs triggered by organization events are debounced with the cache.
+        cache.clear()
 
     def post_webhook(self, event, payload):
         headers = {