-
Notifications
You must be signed in to change notification settings - Fork 3.3k
fix(delete-teams): stop confirming project deletion before data is gone #91339
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2200,32 +2200,46 @@ def send_conversation_restore_email(email: str, team_id: int, restore_url: str) | |
| logger.info(f"Sent conversation restore email to {email} for team {team.id}") | ||
|
|
||
|
|
||
| # Upper bound for erasing analytics event data after a project or organization is deleted. The | ||
| # metadata delete is immediate, but event data is removed by a weekly batch job, so a deletion | ||
| # waits at most one cycle. | ||
| DATA_ERASURE_WINDOW_DAYS = 7 | ||
|
|
||
|
|
||
| @shared_task(**EMAIL_TASK_KWARGS) | ||
| @skip_team_scope_audit | ||
| def send_project_deleted_email( | ||
| user_id: int, | ||
| project_name: str, | ||
| ) -> None: | ||
| """Send email notification when project deletion is complete.""" | ||
| """Send email notification when project deletion starts. | ||
|
|
||
| The email goes out once the project metadata is removed. Analytics event data is then erased | ||
| by a scheduled batch job, so the email states the window rather than claiming completion. | ||
| """ | ||
| user = User.objects.filter(id=user_id).first() | ||
| if not user: | ||
| logger.warning(f"User {user_id} not found for project deletion email") | ||
| return | ||
|
|
||
| data_erased_by = (timezone.now() + datetime.timedelta(days=DATA_ERASURE_WINDOW_DAYS)).strftime("%B %d, %Y") | ||
| message = EmailMessage( | ||
| use_http=True, | ||
| campaign_key=f"project_deleted_{user_id}_{timezone.now().timestamp()}", | ||
| subject=f"Your project '{project_name}' has been deleted", | ||
| subject=f"Your project '{project_name}' is being deleted", | ||
| template_name="project_deleted", | ||
| template_context={ | ||
| "project_name": project_name, | ||
| # team_name aliases project_name for the remote Customer.io template; keep it until | ||
| # template 54 is confirmed to no longer read team_name. | ||
| "team_name": project_name, | ||
| "data_erased_by": data_erased_by, | ||
| "site_url": settings.SITE_URL, | ||
|
Comment on lines
2226
to
2237
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Customer.io ignores the changed subject and HTMLWhy we think it's a valid issue
Issue descriptionBoth messages set Suggested fixUpdate Customer.io templates 54 and 55 before rollout. Change each remote subject and body. Make both templates read Prompt to fix with AI (copy-paste)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Confirmed - on Cloud this is a real gap. When I can't fix this from the repository: the copy that Cloud sends lives in Customer.io, and there is no code here that pushes local template content to that service. This needs a human with Customer.io access to update transactional templates 54 (project_deleted) and 55 (organization_deleted) so their subject and body match the new wording, and to reference the One related note for whoever verifies: the two new tests assert on
posthog[bot] marked this conversation as resolved.
|
||
| }, | ||
| ) | ||
| message.add_user_recipient(user) | ||
| message.send() | ||
| logger.info(f"Sent project deletion confirmation email to user {user_id} for project {project_name}") | ||
| logger.info(f"Sent project deletion email to user {user_id} for project {project_name}") | ||
|
|
||
|
|
||
| @shared_task(**EMAIL_TASK_KWARGS) | ||
|
|
@@ -2235,26 +2249,32 @@ def send_organization_deleted_email( | |
| organization_name: str, | ||
| project_names: list[str], | ||
| ) -> None: | ||
| """Send email notification when organization deletion is complete.""" | ||
| """Send email notification when organization deletion starts. | ||
|
|
||
| The email goes out once the organization metadata is removed. Analytics event data is then | ||
| erased by a scheduled batch job, so the email states the window rather than claiming completion. | ||
| """ | ||
| user = User.objects.filter(id=user_id).first() | ||
| if not user: | ||
| logger.warning(f"User {user_id} not found for organization deletion email") | ||
| return | ||
|
|
||
| data_erased_by = (timezone.now() + datetime.timedelta(days=DATA_ERASURE_WINDOW_DAYS)).strftime("%B %d, %Y") | ||
| message = EmailMessage( | ||
| use_http=True, | ||
| campaign_key=f"organization_deleted_{user_id}_{timezone.now().timestamp()}", | ||
| subject=f"Your organization '{organization_name}' has been deleted", | ||
| subject=f"Your organization '{organization_name}' is being deleted", | ||
| template_name="organization_deleted", | ||
| template_context={ | ||
| "organization_name": organization_name, | ||
| "project_names": project_names, | ||
| "data_erased_by": data_erased_by, | ||
| "site_url": settings.SITE_URL, | ||
| }, | ||
| ) | ||
| message.add_user_recipient(user) | ||
| message.send() | ||
| logger.info(f"Sent organization deletion confirmation email to user {user_id} for organization {organization_name}") | ||
| logger.info(f"Sent organization deletion email to user {user_id} for organization {organization_name}") | ||
|
|
||
|
|
||
| @shared_task(ignore_result=True) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,8 +1,8 @@ | ||
| {% extends "email/base.html" %} {% load posthog_assets %} {% block section %} | ||
| <p> | ||
| As requested, we've deleted your organization <strong>{{ organization_name }}</strong>{% if project_names %} and its project{{ project_names|length|pluralize }}: <strong>{{ project_names|join:", " }}</strong>{% endif %}. | ||
| As requested, we've removed your organization <strong>{{ organization_name }}</strong>{% if project_names %} and its project{{ project_names|length|pluralize }}: <strong>{{ project_names|join:", " }}</strong>{% endif %}, and started deleting the data. | ||
| </p> | ||
| <p> | ||
| This can't be undone, but hey, if you ever want to start fresh, we'll be here. | ||
| The analytics data is permanently erased by <strong>{{ data_erased_by }}</strong>. This can't be undone, but hey, if you ever want to start fresh, we'll be here. | ||
| </p> | ||
| {% endblock %}{% load posthog_filters %} {% block heading %}Organization deleted{% endblock %} | ||
| {% endblock %}{% load posthog_filters %} {% block heading %}Deleting your organization{% endblock %} |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,8 +1,8 @@ | ||
| {% extends "email/base.html" %} {% load posthog_assets %} {% block section %} | ||
| <p> | ||
| As requested, we've deleted <strong>{{ project_name }}</strong> and all its data. | ||
| As requested, we've removed <strong>{{ project_name }}</strong> and started deleting its data. | ||
| </p> | ||
| <p> | ||
| This can't be undone, but hey, if you ever want to start fresh, we'll be here. | ||
| Its analytics data is permanently erased by <strong>{{ data_erased_by }}</strong>. This can't be undone, but hey, if you ever want to start fresh, we'll be here. | ||
|
Comment on lines
+3
to
+6
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The copy promises erasure for data outside the event deletion processWhy we think it's a valid issue
Issue descriptionThe new deadline applies to “analytics data,” but the seven-day process covers only event tables. The workflow does not wait for session-recording deletion. Deleted recording metadata also has a ten-day grace period. Suggested fixChange both templates to say “analytics event data.” If the broader claim is required, wait for every deletion workflow and verified store. Prompt to fix with AI (copy-paste)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Confirmed against the current code — this is a real over-claim, and it affects both the project and organization emails. The weekly batch job that erases data after deletion covers the event tables plus person, distinct-id, group, and cohort-membership data, but it deliberately leaves session recordings and logs to their own TTL (see the note above I'm escalating rather than rewording it myself, because the right copy is a product/legal call, not a mechanical swap:
What a human needs to decide: whether to (a) scope the dated promise to only the data the weekly job actually erases (events + person/group/cohort) and explicitly say session recordings and logs follow their own retention, or (b) drop the specific date. Whichever is chosen should be applied to both |
||
| </p> | ||
| {% endblock %}{% load posthog_filters %} {% block heading %}Project deleted{% endblock %} | ||
| {% endblock %}{% load posthog_filters %} {% block heading %}Deleting your project{% endblock %} | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seven days is not an erasure upper bound
Why we think it's a valid issue
DeletionType.Teamrow afterenqueue_clickhouse_deletion_activitywrites it (posthog/temporal/delete_teams/activities.py:124-135), and on what cadence.deletes_job(posthog/dags/deletes.py:1000-1031), which reads pendingQ(deletion_type=DeletionType.Team)rows atposthog/dags/deletes.py:509-511. It has no cron of its own. It is triggered only byrun_deletes_after_squash(posthog/dags/deletes.py:1033-1045), arun_status_sensorthat fires onsquash_person_overridesSUCCESS.squash_scheduleusesSQUASH_PERSON_OVERRIDES_SCHEDULE(posthog/dags/person_overrides.py:380-388), default"0 22 * * 6"— Saturday 22:00 UTC (posthog/settings/dagster.py:20-21). So the wait to the next cycle alone reaches nearly 7 full days, before the drain does any work.posthog/dags/person_overrides.py:386-387andposthog/dags/deletes.py:1039-1040both state "mutation waits can span hours" and raiseretry_max_attemptsto 20 for it. A deletion arriving just after a Saturday drain begins therefore completes at about send-time + 7 days + hours, past the emailed date. That is the ordinary outcome for that arrival window, not a rare edge.dagster.DagsterRunStatus.SUCCESSonly, so a failed squash means no deletes run and the drain slips another full week. The elevated retry budget exists because these runs do hit transient failures.sharded_events_recentis never swept — it sits inTTL_ONLY_TABLES(posthog/models/deletion_targets.py:183). The internal docdocs/internal/clickhouse-deletion-coverage.md:97states the case directly: the table partitions by day withttl_only_drop_parts = 1, so "the real worst case is about 8 days plus TTL-merge lag, not a flat 7". Its clock runs frominserted_at, so events ingested just before deletion outlive a 7-day promise regardless of when the drain runs.deletes_jobskipscohortpeopleentirely; the entry is commented out atposthog/dags/deletes.py:1017-1018with "the mutations run here overload the cluster pretty badly". So some team data this path is expected to clear is not cleared at all.posthog/tasks/email.py:2203-2206) asserts "a deletion waits at most one cycle", which omits the cycle-boundary wait, the multi-hour job runtime, the success-gated trigger, and the TTL-only table.Issue description
The deletion pipeline does not guarantee this seven-day deadline.
sharded_events_recenthas a documented worst case of about eight days plus TTL merge lag. The weekly deletion job can also start late, run for hours, or retry. Both emails can therefore promise permanent erasure while event rows still exist.Suggested fix
Remove the fixed erasure date until the system has an enforced deadline. Otherwise, use a conservative SLA that covers TTL behavior, job runtime, and retries.
Prompt to fix with AI (copy-paste)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confirmed — the finding is correct against the current code, and I'm escalating rather than editing it unattended because the fix is a product/compliance decision, not a mechanical change. What I verified: (a)
sharded_events_recentis reclaimed by a 7-day TTL keyed oninserted_atwithttl_only_drop_parts=1, and the internal deletion-coverage doc puts the real worst case at ~8 days plus TTL-merge lag — and because that clock runs from ingestion, events ingested just before a deletion can outlive a flat 7 days measured from send time; (b) the drain (deletes_job) runs only weekly, gated on the Saturday 22:00 UTC squash succeeding, so a deletion arriving right after a drain waits ~7 days for the next cycle before any work, the run itself 'can span hours', and a failed squash slips it another full week; (c)cohortpeopledeletion is disabled outright. So the emailed date is not an enforced upper bound and can be exceeded under normal operation — a real problem for a customer filing it as a compliance record. A human needs to choose the direction, because none of the options are provable or mechanical here: (1) drop the specific date and reword to a non-guarantee (but that undoes this PR's purpose), (2) commit to a conservative SLA that genuinely covers TTL behavior + job runtime + retries + the cohortpeople gap (but no finite number is truly guaranteed given unbounded merge lag and the success-gated weekly trigger), or (3) enforce a real deadline in the deletion pipeline. I left the code unchanged pending that decision.