ref(hc): OrganizationMember now denormalizes user.is_active (#49999)

In order to greatly simplify OrganizationMember.user removal, we need to
replicate user's is_active property into each region to which the user
belongs so that OrganizationMember can behave independently of cross
silo queries.

On `OrganizationMemberMapping` creation, we also trigger `User` update
outboxes to force syncing down the user's is_active status (generally
will still be True, but this just catches race conditions). Furthermore,
`User` updates now process by syncing this field to each region the user
belongs to.

migrations_lockfile.txt

@@ -6,5 +6,5 @@ To resolve this, rebase against latest master and regenerate your migration. Thi
 will then be regenerated, and you should be able to merge without conflicts.
 
 nodestore: 0002_nodestore_no_dictfield
-sentry: 0469_backfill_orgmembermapping
+sentry: 0470_denormalize_user_is_active
 social_auth: 0001_initial

src/sentry/migrations/0470_denormalize_user_is_active.py

@@ -0,0 +1,46 @@
+# Generated by Django 2.2.28 on 2023-05-30 17:29
+
+from django.db import migrations, models
+
+from sentry.new_migrations.migrations import CheckedMigration
+
+
+class Migration(CheckedMigration):
+    # This flag is used to mark that a migration shouldn't be automatically run in production. For
+    # the most part, this should only be used for operations where it's safe to run the migration
+    # after your code has deployed. So this should not be used for most operations that alter the
+    # schema of a table.
+    # Here are some things that make sense to mark as dangerous:
+    # - Large data migrations. Typically we want these to be run manually by ops so that they can
+    #   be monitored and not block the deploy for a long period of time while they run.
+    # - Adding indexes to large tables. Since this can take a long time, we'd generally prefer to
+    #   have ops run this and not block the deploy. Note that while adding an index is a schema
+    #   change, it's completely safe to run the operation after the code has deployed.
+    is_dangerous = False
+
+    dependencies = [
+        ("sentry", "0469_backfill_orgmembermapping"),
+    ]
+
+    operations = [
+        migrations.SeparateDatabaseAndState(
+            database_operations=[
+                migrations.RunSQL(
+                    """
+                    ALTER TABLE "sentry_organizationmember" ADD COLUMN "user_is_active" BOOLEAN NOT NULL DEFAULT TRUE;
+                    """,
+                    reverse_sql="""
+                    ALTER TABLE "sentry_organizationmember" DROP COLUMN "user_is_active";
+                    """,
+                    hints={"tables": ["sentry_organizationmember"]},
+                ),
+            ],
+            state_operations=[
+                migrations.AddField(
+                    model_name="organizationmember",
+                    name="user_is_active",
+                    field=models.BooleanField(default=True),
+                ),
+            ],
+        )
+    ]

src/sentry/models/organizationmember.py

@@ -223,6 +223,11 @@ class OrganizationMember(Model):
     # Deprecated -- no longer used
     type = BoundedPositiveIntegerField(default=50, blank=True)
 
+    user_is_active = models.BooleanField(
+        null=False,
+        default=True,
+    )
+
     class Meta:
         app_label = "sentry"
         db_table = "sentry_organizationmember"

src/sentry/models/organizationmembermapping.py

@@ -1,12 +1,13 @@
 from __future__ import annotations
 
 from django.conf import settings
-from django.db import models
+from django.db import models, transaction
 from django.utils import timezone
 
 from sentry.db.models import BoundedBigIntegerField, FlexibleForeignKey, Model, sane_repr
 from sentry.db.models.base import control_silo_only_model
 from sentry.db.models.fields.hybrid_cloud_foreign_key import HybridCloudForeignKey
+from sentry.db.postgres.roles import in_test_psql_role_override
 from sentry.models.organizationmember import InviteStatus
 from sentry.roles import organization_roles
 
@@ -54,4 +55,11 @@ class OrganizationMemberMapping(Model):
             ("organization_id", "organizationmember_id"),
         )
 
+    def save(self, *args, **kwds):
+        with transaction.atomic(), in_test_psql_role_override("postgres"):
+            if self.user and self.id is None:
+                for outbox in self.user.outboxes_for_update():
+                    outbox.save()
+            super().save(*args, **kwds)
+
     __repr__ = sane_repr("organization_id", "organizationmember_id", "user_id", "role")

src/sentry/models/user.py

@@ -185,14 +185,24 @@ class User(BaseModel, AbstractBaseUser):
             avatar = self.avatar.first()
             if avatar:
                 avatar.delete()
-            for outbox in User.outboxes_for_update(self.id):
+            for outbox in self.outboxes_for_update():
                 outbox.save()
             return super().delete()
 
+    def update(self, *args, **kwds):
+        with transaction.atomic(), in_test_psql_role_override("postgres"):
+            for outbox in self.outboxes_for_update():
+                outbox.save()
+            return super().update(*args, **kwds)
+
     def save(self, *args, **kwargs):
-        if not self.username:
-            self.username = self.email
-        return super().save(*args, **kwargs)
+        with transaction.atomic(), in_test_psql_role_override("postgres"):
+            if not self.username:
+                self.username = self.email
+            result = super().save(*args, **kwargs)
+            for outbox in self.outboxes_for_update():
+                outbox.save()
+            return result
 
     def has_perm(self, perm_name):
         warnings.warn("User.has_perm is deprecated", DeprecationWarning)
@@ -278,8 +288,11 @@ class User(BaseModel, AbstractBaseUser):
         for email in email_list:
             self.send_confirm_email_singular(email, is_new_user)
 
+    def outboxes_for_update(self) -> List[ControlOutbox]:
+        return User.outboxes_for_user_update(self.id)
+
     @staticmethod
-    def outboxes_for_update(identifier: int) -> List[ControlOutbox]:
+    def outboxes_for_user_update(identifier: int) -> List[ControlOutbox]:
         return [
             ControlOutbox(
                 shard_scope=OutboxScope.USER_SCOPE,

src/sentry/receivers/outbox/control.py

@@ -22,6 +22,7 @@ from sentry.models import (
     process_control_outbox,
 )
 from sentry.receivers.outbox import maybe_process_tombstone
+from sentry.services.hybrid_cloud.organization import RpcRegionUser, organization_service
 from sentry.silo.base import SiloMode
 
 logger = logging.getLogger(__name__)
@@ -30,10 +31,12 @@ logger = logging.getLogger(__name__)
 
 
 @receiver(process_control_outbox, sender=OutboxCategory.USER_UPDATE)
-def process_user_updates(object_identifier: int, **kwds: Any):
+def process_user_updates(object_identifier: int, region_name: str, **kwds: Any):
     if (user := maybe_process_tombstone(User, object_identifier)) is None:
         return
-    user  # Currently we do not sync any other user changes, but if we did, you can use this variable.
+    organization_service.update_region_user(
+        user=RpcRegionUser(id=user.id, is_active=user.is_active), region_name=region_name
+    )
 
 
 @receiver(process_control_outbox, sender=OutboxCategory.INTEGRATION_UPDATE)

src/sentry/services/hybrid_cloud/organization/impl.py

@@ -27,6 +27,7 @@ from sentry.services.hybrid_cloud.organization import (
     RpcOrganizationMember,
     RpcOrganizationMemberFlags,
     RpcOrganizationSummary,
+    RpcRegionUser,
     RpcUserInviteContext,
     RpcUserOrganizationContext,
 )
@@ -448,3 +449,10 @@ class DatabaseBackedOrganizationService(OrganizationService):
                 .bitand(~OrganizationMember.flags["idp:provisioned"])
                 .bitand(~OrganizationMember.flags["idp:role-restricted"])
             )
+
+    def update_region_user(self, *, user: RpcRegionUser, region_name: str) -> None:
+        # Normally, calling update on a QS for organization member fails because we need to ensure that updates to
+        # OrganizationMember objects produces outboxes.  In this case, it is safe to do the update directly because
+        # the attribute we are changing never needs to produce an outbox.
+        with in_test_psql_role_override("postgres"):
+            OrganizationMember.objects.filter(user_id=user.id).update(user_is_active=user.is_active)

src/sentry/services/hybrid_cloud/organization/model.py

@@ -209,3 +209,13 @@ class RpcUserInviteContext(RpcUserOrganizationContext):
     """
 
     invite_organization_member_id: int = 0
+
+
+class RpcRegionUser(RpcModel):
+    """
+    Represents user information that may be propagated to each region that a user belongs to, often to make
+    more performant queries on organization member information.
+    """
+
+    id: int = -1
+    is_active: bool = True

src/sentry/services/hybrid_cloud/organization/service.py

@@ -10,6 +10,7 @@ from sentry.services.hybrid_cloud.organization import (
     RpcOrganizationMember,
     RpcOrganizationMemberFlags,
     RpcOrganizationSummary,
+    RpcRegionUser,
     RpcUserInviteContext,
     RpcUserOrganizationContext,
 )
@@ -17,6 +18,7 @@ from sentry.services.hybrid_cloud.region import (
     ByOrganizationId,
     ByOrganizationIdAttribute,
     ByOrganizationSlug,
+    ByRegionName,
     UnimplementedRegionResolution,
 )
 from sentry.services.hybrid_cloud.rpc import RpcService, regional_rpc_method
@@ -230,6 +232,11 @@ class OrganizationService(RpcService):
     def remove_user(self, *, organization_id: int, user_id: int) -> RpcOrganizationMember:
         pass
 
+    @regional_rpc_method(resolve=ByRegionName())
+    @abstractmethod
+    def update_region_user(self, *, user: RpcRegionUser, region_name: str) -> None:
+        pass
+
     @regional_rpc_method(resolve=ByOrganizationId())
     @abstractmethod
     def reset_idp_flags(self, *, organization_id: int) -> None:

src/sentry/services/hybrid_cloud/region.py

@@ -45,6 +45,17 @@ class ByOrganizationObject(RegionResolution):
         return self._resolve_from_mapping(mapping)
 
 
+@dataclass(frozen=True)
+class ByRegionName(RegionResolution):
+    """Resolve from an `str` parameter representing a region's name"""
+
+    parameter_name: str = "region_name"
+
+    def resolve(self, arguments: ArgumentDict) -> Region:
+        region_name = arguments[self.parameter_name]
+        return get_region_by_name(region_name)
+
+
 @dataclass(frozen=True)
 class ByOrganizationId(RegionResolution):
     """Resolve from an `int` parameter representing an organization ID."""