feat(data-deletion): link the last dagster run from deletion requests - #70638
Conversation
Migration SQL ChangesHey 👋, we've detected some migrations on this PR. Here's the SQL output for each migration, make sure they make sense:
|
🔍 Migration Risk AnalysisWe've analyzed your migrations for potential risks. Summary: 1 Safe | 0 Needs Review | 0 Blocked ✅ SafeBrief or no lock, backwards compatible 📚 How to Deploy These Changes SafelyAddField: This operation acquires a brief lock but doesn't rewrite the table. Deployment uses lock timeouts with automatic retries, so lock contention will cause retries rather than connection pile-up. Last updated: 2026-07-14 09:58 UTC (e105cc8) |
Store the Dagster run id on every APPROVED -> IN_PROGRESS transition and render it in the Django admin as a link to the run, so debugging an in-flight or failed deletion request no longer means hunting for the run in the Dagster UI. The url is built from the deployment rather than stored: DAGSTER_DOMAIN when set, otherwise derived from CLOUD_DEPLOYMENT (web pods don't carry DAGSTER_DOMAIN).
c63d86d to
2e23194
Compare
|
Reviews (1): Last reviewed commit: "feat(data-deletion): link the last dagst..." | Re-trigger Greptile |
🤖 CI report✅ Backend coverage — all changed backend lines covered🧪 Backend test coveragePatch coverage — changed backend lines (products + core): All changed backend lines are covered ✅ Per-product line coverage (touched products)
Report-only. Patch coverage = changed backend lines covered vs |
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
…etion-dagster-run-link
The run id is written by the same save() that marks a request IN_PROGRESS, so an over-long id would fail the deletion job over a field that only exists to help debugging. varchar(255) removes that edge entirely. Also covers dagster_run_url and the admin display method, including the lowercase CLOUD_DEPLOYMENT case.
|
Reviews (2): Last reviewed commit: "fix(data-deletion): widen run id column ..." | Re-trigger Greptile |
Problem
When a
DataDeletionRequestis running (or has failed), there is no way to get from the request to the Dagster run that executed it.The model already tracks
attempt_count,first_executed_atandlast_executed_at, so you know an attempt happened, but not which run did it.Debugging then means hunting through the Dagster UI for the right run.
Changes
Adds
last_dagster_run_idtoDataDeletionRequest, set on every APPROVED → IN_PROGRESS transition (inside the existingselect_for_updateblock in_record_execution_attempt, so it lands atomically with the status change and the attempt counter).All three load ops pass it: event removal, property removal, person removal.
Each retry overwrites it, matching the "last" semantics of
last_executed_at.The admin renders it in the audit trail as a link to the run.
The URL is built at render time rather than stored, because a stored URL bakes in the environment:
DAGSTER_DOMAINwhen set (the existing pattern from the Slack failure alerts)CLOUD_DEPLOYMENT, since the admin runs on web pods which don't carryDAGSTER_DOMAIN:US→prod-us,EU→prod-eu,DEV→dev, producinghttps://posthog.dagster.cloud/<deployment>/runs/<run_id>Migration 1254 is a nullable
ADD COLUMNonposthog_datadeletionrequest, which is not a hot table and has no non-Django writers.How did you test this code?
Automated, run by me (Claude):
posthog/dags/tests/test_data_deletion_requests.py— 57 passed. I extended the three existingload_*transition tests with one assertion each (request.last_dagster_run_id == context.run_id). That is one per load op, and each guards its own call site: the regression is a load op flipping a request to IN_PROGRESS without recording which run did it. No new test functions, since the existing ones already set up exactly this state.posthog/admin/test_data_deletion_request_admin.pyandposthog/models/test/test_data_deletion_request.py— 120 passed.manage.py sqlmigrateshows a plainALTER TABLE ... ADD COLUMN ... varchar(64) NULL, andmakemigrations --checkreports no drift.I did not click through the admin page in a browser, so the rendered link is unverified visually. Locally it would render as a bare run id anyway, since neither
DAGSTER_DOMAINnorCLOUD_DEPLOYMENTis set in dev.Automatic notifications
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Written by Claude (Claude Code, Opus 4.8) working from a plan I reviewed and approved.
Skills invoked:
/django-migrationsand/writing-tests.The main decision was whether to store the full run URL or just the run id.
We went with the id: a stored URL hardcodes the deployment into the row, and the run id is the durable fact.
That pushed the question of where the admin gets the deployment from, since
DAGSTER_DOMAINis only present on Dagster pods, not on the web pods serving the admin.CLOUD_DEPLOYMENTis always set there, so the helper prefersDAGSTER_DOMAINwhen available and otherwise mapsCLOUD_DEPLOYMENTonto the Dagster Cloud deployment slug, falling back to plain text when neither is set.