Repository navigation
Conversation
Subscriptions and alerts run their queries as their owner. When the owner leaves the organization, or loses access to a table the report reads, the run now disables the subscription or alert with a reason and notifies the people who can replace it, instead of failing on every run. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
🤖 CI report
|
| File | Comment lines | Added lines |
|---|---|---|
posthog/temporal/tests/test_alerts_activities.py |
5 | 65 |
products/subscriptions/backend/tests/temporal/test_subscriptions_workflows.py |
3 | 74 |
posthog/query_creator_access.py |
2 | 30 |
posthog/test/test_query_creator_access.py |
2 | 20 |
products/alerts/backend/evaluation/dispatcher.py |
2 | 13 |
products/subscriptions/backend/logic/auto_disable.py |
2 | 44 |
products/subscriptions/backend/temporal/activities.py |
2 | 29 |
products/subscriptions/backend/temporal/ai_subscription/delivery.py |
2 | 11 |
This check does not block merging. It updates on every push and clears when the share drops.
⚠️ Backend coverage — 95.0% of changed backend lines covered — 8 uncovered
🧪 Backend test coverage
Patch coverage — changed backend lines (products + core): ███████████████████░ 95.0% (152 / 160)
| File | Patch | Uncovered changed lines |
|---|---|---|
products/subscriptions/backend/temporal/insight_snapshot.py |
66.7% | 196, 198 |
products/alerts/backend/evaluation/dispatcher.py |
75.0% | 316, 318 |
products/subscriptions/backend/temporal/ai_subscription/delivery.py |
77.8% | 450–451 |
products/subscriptions/backend/temporal/activities.py |
91.7% | 518 |
products/subscriptions/backend/logic/auto_disable.py |
95.2% | 122 |
🤖 Agents: add a test only if an uncovered line exposes a realistic regression that existing tests miss. Otherwise explain why no new test is needed under "How did you test this code?". Gap list: the patch-coverage artifact on this run (gh run download 318565638870793 -n patch-coverage), or the coverage-data block at the end of this comment.
Per-product line coverage (touched products)
| Product | Coverage | Lines |
|---|---|---|
platform_features |
██░░░░░░░░░░░░░░░░░░ 12.1% |
7 / 58 |
demo |
███████████░░░░░░░░░ 56.4% |
1,542 / 2,736 |
data_tools |
████████████░░░░░░░░ 60.9% |
95 / 156 |
warehouse_sources_queue |
█████████████░░░░░░░ 65.2% |
2,161 / 3,314 |
authorization |
████████████████░░░░ 79.7% |
181 / 227 |
batch_exports |
████████████████░░░░ 81.8% |
23,046 / 28,185 |
engineering_analytics |
█████████████████░░░ 84.9% |
11,605 / 13,677 |
commands |
█████████████████░░░ 85.5% |
1,618 / 1,893 |
mcp_analytics |
█████████████████░░░ 86.3% |
5,795 / 6,714 |
ml_inference |
█████████████████░░░ 86.3% |
524 / 607 |
posthog_ai |
██████████████████░░ 87.9% |
4,297 / 4,887 |
apm |
██████████████████░░ 88.2% |
1,973 / 2,236 |
notebooks |
██████████████████░░ 88.5% |
15,330 / 17,321 |
mcp_registry |
██████████████████░░ 89.0% |
1,596 / 1,794 |
webmcp |
██████████████████░░ 89.3% |
292 / 327 |
dashboards |
██████████████████░░ 89.4% |
8,036 / 8,991 |
cohorts |
██████████████████░░ 89.6% |
8,556 / 9,547 |
product_tours |
██████████████████░░ 89.7% |
1,355 / 1,510 |
warehouse_suggestions |
██████████████████░░ 89.9% |
3,714 / 4,133 |
signals |
██████████████████░░ 90.3% |
92,910 / 102,857 |
today |
██████████████████░░ 90.6% |
3,546 / 3,912 |
visual_review |
██████████████████░░ 90.6% |
9,858 / 10,875 |
tasks |
██████████████████░░ 91.0% |
81,261 / 89,274 |
managed_warehouse |
██████████████████░░ 91.1% |
12,410 / 13,626 |
data_warehouse |
██████████████████░░ 91.2% |
15,435 / 16,931 |
canvas |
██████████████████░░ 91.2% |
8,189 / 8,980 |
autoresearch |
██████████████████░░ 91.4% |
11,616 / 12,705 |
cdp |
██████████████████░░ 91.4% |
10,296 / 11,261 |
data_modeling |
██████████████████░░ 91.7% |
11,149 / 12,163 |
metrics |
██████████████████░░ 91.7% |
10,627 / 11,590 |
wizard |
██████████████████░░ 91.7% |
6,872 / 7,492 |
slack_app |
██████████████████░░ 91.7% |
21,760 / 23,720 |
business_knowledge |
██████████████████░░ 91.8% |
10,296 / 11,221 |
streamlit_apps |
██████████████████░░ 91.9% |
3,139 / 3,415 |
error_tracking |
██████████████████░░ 92.2% |
19,730 / 21,401 |
conversations |
██████████████████░░ 92.3% |
30,747 / 33,324 |
review_hog |
██████████████████░░ 92.3% |
18,059 / 19,562 |
early_access_features |
███████████████████░ 92.6% |
1,339 / 1,446 |
stamphog |
███████████████████░ 92.6% |
8,891 / 9,597 |
alerts_platform |
███████████████████░ 92.8% |
8,302 / 8,947 |
notifications |
███████████████████░ 92.8% |
1,218 / 1,312 |
alerts |
███████████████████░ 92.9% |
10,667 / 11,482 |
aeo |
███████████████████░ 93.0% |
2,150 / 2,312 |
ai_training |
███████████████████░ 93.3% |
598 / 641 |
web_analytics |
███████████████████░ 93.3% |
26,193 / 28,069 |
cross_project_dashboards |
███████████████████░ 93.4% |
880 / 942 |
surveys |
███████████████████░ 93.4% |
6,941 / 7,429 |
exports |
███████████████████░ 93.6% |
4,192 / 4,480 |
ai_gateway |
███████████████████░ 93.6% |
787 / 841 |
approvals |
███████████████████░ 93.7% |
4,994 / 5,331 |
messaging |
███████████████████░ 93.7% |
6,712 / 7,164 |
workflows |
███████████████████░ 93.8% |
26,903 / 28,690 |
context_layer |
███████████████████░ 93.8% |
3,981 / 4,244 |
mcp_store |
███████████████████░ 93.9% |
9,188 / 9,784 |
marketing_analytics |
███████████████████░ 93.9% |
21,275 / 22,648 |
managed_migrations |
███████████████████░ 94.0% |
1,821 / 1,938 |
billing_alerts |
███████████████████░ 94.0% |
2,322 / 2,470 |
customer_analytics |
███████████████████░ 94.0% |
29,643 / 31,528 |
ai_observability |
███████████████████░ 94.1% |
31,204 / 33,169 |
replay_vision |
███████████████████░ 94.3% |
31,590 / 33,492 |
legal_documents |
███████████████████░ 94.4% |
2,313 / 2,451 |
logs |
███████████████████░ 94.5% |
19,334 / 20,467 |
actions |
███████████████████░ 94.7% |
1,004 / 1,060 |
experiments |
███████████████████░ 94.8% |
41,351 / 43,639 |
reminders |
███████████████████░ 94.8% |
761 / 803 |
endpoints |
███████████████████░ 94.9% |
9,369 / 9,875 |
growth |
███████████████████░ 94.9% |
11,450 / 12,063 |
annotations |
███████████████████░ 95.1% |
1,095 / 1,151 |
skills |
███████████████████░ 95.2% |
7,630 / 8,015 |
uptime_monitoring |
███████████████████░ 95.3% |
306 / 321 |
access_control |
███████████████████░ 95.5% |
8,281 / 8,671 |
subscriptions |
███████████████████░ 95.6% |
13,767 / 14,404 |
data_catalog |
███████████████████░ 95.7% |
4,713 / 4,927 |
product_analytics |
███████████████████░ 95.8% |
30,068 / 31,394 |
revenue_analytics |
███████████████████░ 95.9% |
1,877 / 1,958 |
feature_flags |
███████████████████░ 95.9% |
29,248 / 30,495 |
data_quality |
███████████████████░ 96.0% |
8,561 / 8,915 |
tracing |
███████████████████░ 96.4% |
4,418 / 4,582 |
warehouse_sources |
███████████████████░ 96.5% |
486,627 / 504,145 |
security |
███████████████████░ 96.7% |
1,549 / 1,602 |
account_audits |
███████████████████░ 96.7% |
770 / 796 |
pulse |
███████████████████░ 97.1% |
2,023 / 2,083 |
analytics_platform |
████████████████████ 98.0% |
2,855 / 2,912 |
groups |
████████████████████ 98.2% |
1,838 / 1,871 |
field_notes |
████████████████████ 99.4% |
172 / 173 |
Report-only. Patch coverage = changed backend lines covered vs origin/master. Sorted lowest first.
Known gaps: lines covered only by Temporal tests show as uncovered; core line numbers may drift if master changed the same file.
There was a problem hiding this comment.
PostHog Review · Standard · reviewed 746fe27: 1 Should fix.
This is the only automatic review while this pull request is a draft. Comment @posthog review to review new changes sooner. Otherwise, a review of the new changes runs when you mark it ready for review.
See this review and its findings in PostHog Review. Comment @posthog review --deep for a Deep review. A Deep review takes longer and looks deeper than a Standard review.
|
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
When the owner left the organization, the export and the snapshot no longer run the query and record the reason instead. The delivery goes out once with the reason on each failed tile, and then the subscription is disabled. Owners and admins no longer get a separate email for these two reasons. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The disabled-alert email, in-app notice and banner only recognise that code, so the new codes made subscribers expect another check. Access denials for platform-owned or ownerless alerts and subscriptions keep the old behaviour. The owner error lookup is now two name sets, the exporter comment is gone, and the table-access skill describes the new behaviour. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Both the insight stack and the alerts platform call check_alert_for_insight, and the platform keeps evaluating an alert after its shadow week ends. The evaluator now raises AlertOwnerAccessError, an AlertExtractionError, when the owner left or the query is denied, so each stack disables the alert through its existing broken-configuration path and no exemption list is needed. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Risk: No findings The increment since the last review touches only two test files: it moves the existing patch of the owner-access check to class-level setup in the alerts evaluation-delay test, and adds an autouse fixture patching the same check in the AI-subscription delivery test (whose team/user objects are mocks). No production code, authorization logic, or trust boundary changed, and the real owner checks remain in the dispatcher and delivery modules. Sentinel reviewed |
yasen-posthog
left a comment
There was a problem hiding this comment.
I think a better UX would be when a user has left the org and someone tries to reenable their sub/alert, they're presented with a modal to accept ownership of the resource and subscription. That would allow admins to take over if needed. Cuz currently if they reenable it, it fails on the next dispatch again and auto-disables.
Also another dashboard/tile issue:
- One denied tile disables the whole subscription. If someone adds a tile the owner can't access, a working dashboard subscription gets turned off. Maybe only disable when every tile is denied?
Also a couple of Qs:
- Is there a reason why we cover only dw table access and not project or dashboard AC changes? Would that be too broad of a change?
- Seems that a deleted owner counts as no owner in the conditionals so it's business as usual in this case, which I think should not be the case?
- AI subs seems to not be covered by this, do we want them included or are they out for a reason?
|
@yasen-posthog thanks for the feedback, will do another iteration on this, moved it back to draft for now.
Good point. I was planning to add an offboarding flow when an admin removes someone from the org (finishing it up here). This PR is more for the existing subscriptions from users who already left the org, so they stop working. Otherwise someone could, for example, keep receiving revenue updates to their personal email forever.
Makes sense. I designed this PR mostly for the case where the creator left the org and didn't consider the one-denied-insight case. It's much rarer, but in that case we can still deliver the dashboard with that one insight showing an error 👍
Not only warehouse tables, it's whatever the query raises:
For subscriptions
Will take a look. At first glance a deleted creator already auto-disables there ( |
…m's org The temporal team fixture lives in its own organization while the shared user fixture does not, so every creator looked like a member who left and the delivery tests stopped running their queries. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…owner left Subscriptions and alerts always get an owner on create, so a null one means the user was deleted and the row should stop like any other whose owner left. The check also covers a member who lost access to the project, not only one who left the organization. Exports keep their null-owner exemption because logs and activity-log exports have none on purpose. The alert test factories now write an owner, as production does. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…hen the send fails Lost access is judged on the whole delivery: a dashboard with one denied tile is still sent, with that tile showing why, and only a delivery whose every export was denied disables the subscription. The owner disable now runs after the send in a finally, so a permanent bounce no longer leaves the subscription enabled to repeat the failed exports every cycle. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…un them The AI report runs its queries as the creator and already stopped for a deleted one. A creator who left the organization or lost access to the project now ends the subscription the same way, before any model call. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…evoked-creator # Conflicts: # products/alerts/backend/evaluation/dispatcher.py
…ard or insight Delivery checked table and runner-resource access as the owner, but loaded the dashboard and insight directly, so an owner who lost view access to the object itself kept receiving it. Exports, the snapshot step and the alert evaluator now check object access as the owner before the query. The failure is a UserAccessControlError, which the lost-access path already handles: one private tile still delivers with that tile showing why, and an unviewable dashboard or alert insight disables the subscription or alert. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
assert_creator_can_run raises CreatorLeftOrganizationError when the owner's project access is revoked and UserAccessControlError when they cannot view one of the given objects, so each call site asks once instead of twice. The snapshot step's one-caller error helper is inlined. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
creator_access_revoked goes back to deleted, deactivated or not in the org. The project is the first object assert_creator_can_run checks, so a member who is still in the org but shut out of the project raises UserAccessControlError and the recipients read the lost-access reason, not the one about leaving. The AI path uses the same function. The send branches keep their original if/elif/else shape inside _send_to_target. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
mypy: the dashboard relation on the asset is optional. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
has_project_access is the facade's own answer, object rules never consult the project. The object loop drops its dead None branch, the AI path says which of the two reasons applies, and _send_to_target explains why it exists. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Re-enable / take ownership is going into a separate PR stacked on this one: #125382. It rejects re-enabling a subscription or alert whose owner can't run it anymore (instead of running once and auto-disabling again) and adds a take-ownership endpoint, so whoever still needs the report can become the owner and enable it. The modal will go there too. This PR stays just the auto-disable. |
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
There was a problem hiding this comment.
PostHog Review · Standard · reviewed 9008fb5: 1 Must fix, 1 Should fix.
This is review round 3 on this pull request. When findings keep coming back in the same code, the approach is usually the problem. Before the next fix, check whether a different design would remove the whole class of findings.
…ient send failures retryable has_project_access is a cached property, so calling it raised TypeError for every owner still in the organization and failed every owned export, snapshot and alert check. The delay tests build an alert with no owner and no database, so they patch the owner check out. The owner disable now runs after a successful send or a failure Temporal will not retry, so a transient SMTP, Slack or Teams failure leaves the row enabled for the retry to deliver the reason. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…tion Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
There was a problem hiding this comment.
PostHog Review · Standard · reviewed d365dab: 1 Should fix.
This is review round 4 on this pull request. When findings keep coming back in the same code, the approach is usually the problem. Before the next fix, check whether a different design would remove the whole class of findings.
…and alert tests Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
HostHog preview —
|
Problem
Right now subscriptions keep executing even when their creator has left the org. But if they reference warehouse tables, they raise
TableAccessDeniedError(which creates noise in error tracking). Alerts behave the same way.I originally planned to bypass warehouse access control so subscriptions keep working (#113573). But it turns out to be quite fragile to rely on a save/update-time check, and it's easy to forget that after a subscription is created, it just keeps executing.
I also considered using team default access. But
UserAccessControlis designed around user permissions. It would also mean a new access mode in HogQL (more complexity in the enforcement, more risk of accidental bugs). And more importantly, orgs with access control often set team default permissions low (no access or view) and then grant access explicitly to roles and members. That means a subscription would not reach a user even though the user has access.I researched it and found that other services (for example Power BI, Metabase, Looker and Sigma) mostly use the owner's permissions. Or the recipient's permissions (Tableau, but that's rarer and doesn't fit email/Slack subscriptions.
So this PR just auto-disables subscriptions and alerts if the owner is no longer in the org or lost access.
Changes
An owner can no longer run a query when they were deleted, left the organization, lost access to the project, or lost access to the dashboard or insight itself. A subscription or alert always gets an owner on create, so a null one means the user was deleted.
CreatorLeftOrganizationErrorbefore the query, because a query on no restricted table still succeeds. The subscription snapshot behind the AI change summary skips the query the same way.CreatorLeftOrganizationErroris a user error, so Temporal does not retry it and error tracking does not capture it.TableAccessDeniedErrornow disables the alert instead of going to error tracking on every check.check_alert_for_insight, which the insight stack and the alerts platform share, so platform-owned alerts are covered too. Each stack disables through its existing broken-configuration path.How did you test this code?
Test rationale: each case catches a regression no existing test covered.
test_deliver_subscription_sends_the_reason_then_disables_when_the_owner_cannot_run_it, new test with 4 cases: a subscription keeps delivering or emails its owner after the owner left; a dashboard with one denied tile gets disabled; a bouncing delivery stays enabled. No existing delivery test sends and then disables.test_export_does_not_run_when_its_owner_cannot, new test with 2 cases next to the exporter failure tests: an export runs its query for an owner who left, or for an owner who cannot view the dashboard.test_subscription_asset_error_message, 2 new cases: recipients see the raw "You don't have access to table" text.test_evaluate_auto_disables_when_the_owner_can_no_longer_run_the_query, new test with 3 cases (owner left, lost table access, cannot view the insight): no existing alert test reached the access-denied branch. It pins the sharedinvalid_configurationcode, which routes the check to the disabled-alert email and banner.test_an_alert_production_cannot_evaluate_as_configured_errors_and_is_disabled, 1 new case: the alerts platform does not disable on the owner error.test_member_without_project_access_is_revoked, new case: a member who lost project access still counts as able to run the query.test_generate_ai_report_prompt_rejected_aborts_and_auto_disables, 1 new case: an AI subscription whose creator left still calls the planner.Release status
Docs update
None. No existing doc describes when subscriptions or alerts get auto-disabled. The
debugging-table-access-deniedskill hint about background jobs is updated.🤖 Agent context