fix: auto-disable subscriptions when the creator left or lost access

posthog/temporal/alerts/activities.py

@@ -32,7 +32,7 @@
 from posthog.email import is_email_available
 from posthog.errors import CH_TRANSIENT_ERRORS, CHQueryErrorQueryWasCancelled
 from posthog.exceptions_capture import capture_exception
-from posthog.query_creator_access import creator_access_revoked, report_creator_access_revoked
+from posthog.query_creator_access import creator_access_revoked
 from posthog.schema_migrations.upgrade_manager import upgrade_insight
 from posthog.sync import database_sync_to_async
 from posthog.tasks.alerts.investigation_notifications import run_investigation_notification_safety_net
@@ -279,11 +279,23 @@
 
 _SLOT_LOST_ERROR_TYPE = "EvaluationSlotLost"
 
+CREATOR_LEFT_DISABLE_REASON = (
+    "The owner of this alert is no longer in the organization. Create a new alert to keep monitoring this insight."
+)
+CREATOR_LOST_ACCESS_DISABLE_REASON = (
+    "The owner of this alert no longer has access to some of the data in this insight. "
+    "Ask a project admin for access, or create a new alert."
+)
+
 
 class _EvaluationStopped(Exception):
     """Raised in the evaluation thread once its attempt's cancellation began, so it records nothing."""
 
 
+class _CreatorLeftOrganization(Exception):
+    pass
+
+
 async def _hold_evaluation_slot_before_running(alert_id: str, *, lease_seconds: float) -> float:
     """Hold the slot for evaluate_alert, waiting while there is no room or Redis is unreachable.
 
@@ -472,11 +484,11 @@
 
 
 @temporalio.activity.defn
 async def prepare_alert(inputs: PrepareAlertActivityInputs) -> PrepareAlertResult:
     """Load the alert, validate its config, and decide whether to evaluate."""
 
     @database_sync_to_async(thread_sensitive=False)
     def _prepare() -> PrepareAlertResult:
         try:
             alert = AlertConfiguration.objects.select_related("insight", "team", "team__organization", "threshold").get(
                 id=inputs.alert_id
@@ -630,7 +642,7 @@
 
 
 @temporalio.activity.defn
 async def evaluate_alert(inputs: EvaluateAlertActivityInputs) -> EvaluateAlertResult:
     """Run the insight ClickHouse query, apply the state machine, persist an AlertCheck row."""
     info = temporalio.activity.info()
     evaluation_id = f"{info.workflow_run_id}:{info.activity_id}"
@@ -647,7 +659,7 @@
         if stopping.is_set():
             raise _EvaluationStopped()
 
     def _evaluate(alert: AlertConfiguration) -> EvaluateAlertResult:
         _stop_if_cancelled()
         evaluated_alert = alert
         evaluated_fingerprint = _evaluation_fingerprint(alert)
@@ -668,9 +680,18 @@
         breaches: list[str] | None = None
         error: dict | None = None
         invalid_configuration: str | None = None
+        disable_error_code = "invalid_configuration"
         alert_evaluation_result = None
 
         try:
+            # The query runs as the owner, and a query that reads no restricted table would still succeed.
+            # An alert with no recorded owner is left alone, and the platform delivers platform-owned alerts.
+            if (
+                alert.created_by is not None
+                and not alert.platform_owned
+                and creator_access_revoked(alert.created_by, alert.team)
+            ):
+                raise _CreatorLeftOrganization()
             alert_evaluation_result = check_alert_for_insight(alert, evaluation_id=evaluation_id)
             breaches = alert_evaluation_result.breaches
             if is_llm_detector_config(alert.detector_config):
@@ -700,29 +721,13 @@
             # existing path instead of capturing it as an exception, which would pollute error
             # tracking with a config problem that recurs on every check until fixed.
             invalid_configuration = str(err)
-        except TableAccessDeniedError as err:
-            logger.exception("Alert failed to evaluate", alert_id=alert.id, exc_info=err)
-            # A revoked creator's access-denied error is a known limitation - report it as an event
-            # rather than capturing it, which would recur on every check until the alert is fixed.
-            if creator_access_revoked(alert.created_by, alert.team):
-                report_creator_access_revoked(
-                    user=alert.created_by,
-                    team=alert.team,
-                    source="alert",
-                    error=err,
-                    properties={"alert_id": str(alert.id), "insight_id": alert.insight_id},
-                )
-                error = {"message": str(err)}
-            else:
-                capture_exception(
-                    err,
-                    additional_properties={
-                        "alert_configuration_id": str(alert.id),
-                        "insight_id": alert.insight_id,
-                        "team_id": alert.team_id,
-                    },
-                )
-                error = {"message": str(err), "traceback": traceback.format_exc()}
+        except _CreatorLeftOrganization:
+            invalid_configuration = CREATOR_LEFT_DISABLE_REASON
+            disable_error_code = "creator_left_organization"
+        except TableAccessDeniedError:
+            # The owner is still a member, so the denial is their access, not a bug to capture.
+            invalid_configuration = CREATOR_LOST_ACCESS_DISABLE_REASON
+            disable_error_code = "creator_lost_access"
         except Exception as err:
             logger.exception("Alert failed to evaluate", alert_id=alert.id, exc_info=err)
             capture_exception(
@@ -755,7 +760,7 @@
             alert = current_alert
             if invalid_configuration is not None:
                 alert_check = disable_invalid_alert(
-                    alert, invalid_configuration, notify_subscribers=False, error_code="invalid_configuration"
+                    alert, invalid_configuration, notify_subscribers=False, error_code=disable_error_code
                 )
                 return EvaluateAlertResult(
                     alert_check_id=str(alert_check.id),

posthog/temporal/tests/test_alerts_activities.py

@@ -30,6 +30,8 @@
     TrendsQuery,
 )
 
+from posthog.hogql.errors import TableAccessDeniedError
+
 from posthog.clickhouse.query_tagging import get_query_tags
 from posthog.constants import AvailableFeature
 from posthog.errors import CHQueryErrorQueryWasCancelled
@@ -38,7 +40,7 @@
     ClickHouseClusterMemoryLimitExceeded,
     ClickHouseQueryMemoryLimitExceeded,
 )
-from posthog.models import Team, User
+from posthog.models import OrganizationMembership, Team, User
 from posthog.redis import get_client
 from posthog.slo.types import SloOperation, SloOutcome
 from posthog.tasks.alerts.utils import (
@@ -1263,6 +1265,41 @@ async def test_evaluate_auto_disables_and_skips_error_tracking_on_configuration_
         await sync_to_async(check.refresh_from_db)()
         assert check.targets_notified
 
+    @pytest.mark.parametrize(
+        "owner_change, expected_code",
+        [("left_organization", "creator_left_organization"), ("lost_table_access", "creator_lost_access")],
+    )
+    async def test_evaluate_auto_disables_when_the_owner_can_no_longer_run_the_query(
+        self, alert_with_user, aorganization, owner_change, expected_code
+    ) -> None:
+        owner = await sync_to_async(lambda: alert_with_user.subscribed_users.get())()
+        alert_with_user.created_by = owner
+        await sync_to_async(alert_with_user.save)(update_fields=["created_by"])
+        if owner_change == "left_organization":
+            await sync_to_async(
+                OrganizationMembership.objects.filter(organization_id=aorganization.id, user=owner).delete
+            )()
+
+        with (
+            patch(
+                "posthog.temporal.alerts.activities.check_alert_for_insight",
+                side_effect=TableAccessDeniedError("revenue"),
+            ) as mock_check,
+            patch("posthog.temporal.alerts.activities.capture_exception") as mock_capture,
+        ):
+            result = await ActivityEnvironment().run(
+                evaluate_alert, EvaluateAlertActivityInputs(alert_id=str(alert_with_user.id))
+            )
+
+        assert result.new_state == AlertState.ERRORED
+        mock_capture.assert_not_called()
+        # A departed owner's alert must stop even when its query would still run.
+        assert mock_check.called is (owner_change == "lost_table_access")
+        check = await sync_to_async(AlertCheck.objects.get)(pk=result.alert_check_id)
+        assert check.error is not None and check.error["code"] == expected_code
+        refreshed = await sync_to_async(AlertConfiguration.objects.get)(pk=alert_with_user.pk)
+        assert refreshed.enabled is False
+
     # Transient CH errors bubble up so Temporal's retry policy handles them.
     # Capacity errors (codes 202/439) surface as ClickHouseAtCapacity, so that's what we simulate.
     # A server-wide or per-user memory limit is the same kind of cluster pressure: recording it as

products/subscriptions/backend/logic/auto_disable.py

@@ -1,13 +1,19 @@
 import uuid
+from collections.abc import Iterable
 from typing import NamedTuple
 from urllib.parse import urlparse
 
 import structlog
 
+from posthog.hogql.errors import TableAccessDeniedError
+
 from posthog.email import EmailMessage
 from posthog.exceptions_capture import capture_exception
-from posthog.models import User
+from posthog.models import OrganizationMembership, User
+from posthog.query_creator_access import creator_access_revoked
 
+from products.access_control.backend.facade.contracts import UserNotOrganizationMemberError
+from products.access_control.backend.facade.user_access_control import UserAccessControlError
 from products.notifications.backend.facade.api import (
     NotificationData,
     NotificationType,
@@ -58,6 +64,22 @@ class DisableReason(NamedTuple):
         "Add files:write to a custom Slack app, or reconnect PostHog's Slack app, then try again."
     ),
 )
+CREATOR_LEFT_DISABLE_REASON = DisableReason(
+    key="creator_left_organization",
+    description="The owner of this subscription is no longer in the organization",
+    user_message=(
+        "Cannot re-enable {target_type} subscription: its owner is no longer in the organization. "
+        "Create a new subscription to keep receiving this report."
+    ),
+)
+CREATOR_LOST_ACCESS_DISABLE_REASON = DisableReason(
+    key="creator_lost_access",
+    description="The owner of this subscription no longer has access to some of its data",
+    user_message=(
+        "Cannot re-enable {target_type} subscription until its owner has access to all of its data again. "
+        "Ask a project admin for access, or create a new subscription."
+    ),
+)
 AI_PROMPT_INVALID_DISABLE_REASON = DisableReason(
     key="ai_prompt_invalid",
     description="AI subscription prompt or creator is invalid",
@@ -83,6 +105,28 @@ def get_subscription_disable_reason(target_type: str | None, integration_id: int
     return None
 
 
+def get_creator_disable_reason(subscription: Subscription) -> DisableReason | None:
+    """Disable reason when the owner can no longer run the subscription's queries. Deliveries run as the owner.
+
+    A subscription with no recorded owner is left alone, because nobody is known to have left.
+    """
+    if subscription.created_by is not None and creator_access_revoked(subscription.created_by, subscription.team):
+        return CREATOR_LEFT_DISABLE_REASON
+    return None
+
+
+# Export failures that mean the owner lost access, not that the export broke.
+_ACCESS_DENIED_EXCEPTION_TYPES = frozenset(
+    {TableAccessDeniedError.__name__, UserAccessControlError.__name__, UserNotOrganizationMemberError.__name__}
+)
+
+
+def get_asset_access_disable_reason(exception_types: Iterable[str | None]) -> DisableReason | None:
+    if any(exception_type in _ACCESS_DENIED_EXCEPTION_TYPES for exception_type in exception_types):
+        return CREATOR_LOST_ACCESS_DISABLE_REASON
+    return None
+
+
 def target_type_label(target_type: str | None) -> str:
     if not target_type:
         return ""
@@ -110,6 +154,24 @@ def _get_notification_creator(subscription: Subscription) -> User | None:
     return creator
 
 
+def _disabled_notification_emails(subscription: Subscription, reason: DisableReason) -> list[str]:
+    creator = _get_notification_creator(subscription)
+    if creator is not None:
+        return [creator.email] if creator.email else []
+    if reason != CREATOR_LEFT_DISABLE_REASON:
+        return []
+    # The owner is gone, so the admins are the only people who can replace the subscription.
+    return list(
+        OrganizationMembership.objects.filter(
+            organization_id=subscription.team.organization_id,
+            level__gte=OrganizationMembership.Level.ADMIN,
+            user__is_active=True,
+        )
+        .exclude(user__email="")
+        .values_list("user__email", flat=True)
+    )
+
+
 def disable_invalid_subscription(subscription: Subscription, reason: DisableReason) -> None:
     # Compare-and-swap so only one racing caller sends the disabled-notification
     # email (UUID4 campaign keys mean MessagingRecord can't dedup the duplicate).
@@ -149,10 +211,10 @@ def disable_invalid_subscription(subscription: Subscription, reason: DisableReas
             exc_info=True,
         )
 
-    creator = _get_notification_creator(subscription)
-    if creator and creator.email:
+    emails = _disabled_notification_emails(subscription, reason)
+    if emails:
         try:
-            send_notifications_for_disabled_subscription(subscription, reason, [creator.email])
+            send_notifications_for_disabled_subscription(subscription, reason, emails)
         except Exception as e:
             # Disabling is the durable side effect; email is best-effort. If the email
             # fails (SMTP outage, ImproperlyConfigured on self-hosted, Customer.io 5xx)

products/subscriptions/backend/temporal/activities.py

@@ -21,6 +21,8 @@
 from products.subscriptions.backend.logic.auto_disable import (
     UNSUPPORTED_TARGET_DISABLE_REASON,
     disable_invalid_subscription,
+    get_asset_access_disable_reason,
+    get_creator_disable_reason,
     get_subscription_disable_reason,
 )
 from products.subscriptions.backend.logic.email_subscriptions import send_email_subscription_report
@@ -224,7 +226,7 @@ def get_subscriptions() -> list[DueSubscription]:
 async def validate_subscription_for_delivery(subscription_id: int) -> DeliveryAbort | None:
     """Returns abort info when delivery should not proceed; None to continue."""
     subscription = await database_sync_to_async(
-        Subscription.objects.select_related("created_by", "integration").get,
+        Subscription.objects.select_related("created_by", "integration", "team").get,
         thread_sensitive=False,
     )(pk=subscription_id)
 
@@ -234,7 +236,9 @@ async def validate_subscription_for_delivery(subscription_id: int) -> DeliveryAb
         await LOGGER.ainfo("validate_subscription.already_disabled_skipping", subscription_id=subscription_id)
         return DeliveryAbort()
 
-    reason = get_subscription_disable_reason(subscription.target_type, subscription.integration_id)
+    reason = get_subscription_disable_reason(
+        subscription.target_type, subscription.integration_id
+    ) or await database_sync_to_async(get_creator_disable_reason, thread_sensitive=False)(subscription)
     if reason is None:
         return None
 
@@ -506,6 +510,11 @@ async def _deliver_insight_dashboard_subscription(
         _capture_delivery_failed_event(subscription, Exception(NO_ASSETS_REASON))
         return DeliverSubscriptionResult(recipient_results=recipient_results)
 
+    # Disable rather than deliver a report with some tiles quietly missing.
+    access_reason = get_asset_access_disable_reason(asset.exception_type for asset in assets)
+    if access_reason is not None:
+        return await auto_disable_and_return(subscription, access_reason, recipient_results)
+
     if subscription.target_type == Subscription.SubscriptionTarget.EMAIL:
 
         async def _send_email(email: str) -> None:

products/subscriptions/backend/tests/logic/test_auto_disable.py

@@ -6,10 +6,11 @@
 
 from parameterized import parameterized
 
-from posthog.models import User
+from posthog.models import OrganizationMembership, User
 
 from products.subscriptions.backend.logic.auto_disable import (
     AI_PROMPT_INVALID_DISABLE_REASON,
+    CREATOR_LEFT_DISABLE_REASON,
     SLACK_DISCONNECTED_DISABLE_REASON,
     disable_invalid_subscription,
     send_notifications_for_disabled_subscription,
@@ -155,11 +156,16 @@ def test_no_email_when_creator_unable_to_receive(self, _label, use_creator_with_
 
     @parameterized.expand(
         [
-            ("former_creator", False),
-            ("deactivated_creator", True),
+            ("former_creator", False, SLACK_DISCONNECTED_DISABLE_REASON),
+            ("deactivated_creator", True, SLACK_DISCONNECTED_DISABLE_REASON),
+            ("former_creator_left_org", False, CREATOR_LEFT_DISABLE_REASON),
+            ("deactivated_creator_left_org", True, CREATOR_LEFT_DISABLE_REASON),
         ]
     )
-    def test_does_not_notify_creator_without_access(self, _label, deactivate_member):
+    def test_does_not_notify_creator_without_access(self, _label, deactivate_member, reason):
+        admin = User.objects.create_and_join(
+            self.organization, "admin@example.com", None, level=OrganizationMembership.Level.ADMIN
+        )
         if deactivate_member:
             creator = self.user
             creator.is_active = False
@@ -176,10 +182,15 @@ def test_does_not_notify_creator_without_access(self, _label, deactivate_member)
                 "products.subscriptions.backend.logic.auto_disable.send_notifications_for_disabled_subscription"
             ) as send_mock,
         ):
-            disable_invalid_subscription(sub, SLACK_DISCONNECTED_DISABLE_REASON)
+            disable_invalid_subscription(sub, reason)
 
         create_notification_mock.assert_not_called()
-        send_mock.assert_not_called()
+        if reason == CREATOR_LEFT_DISABLE_REASON:
+            emails = send_mock.call_args.args[2]
+            assert admin.email in emails
+            assert creator.email not in emails
+        else:
+            send_mock.assert_not_called()
 
     def test_send_notifications_uses_unique_campaign_key_per_call(self):
         sub = self._make_subscription()