Repository navigation
feat(flags): sweep flag_evaluations in data deletions - #82056
Conversation
|
😎 Merged successfully - details. |
🤖 CI report
|
| File | Patch | Uncovered changed lines |
|---|---|---|
posthog/models/data_deletion_request.py |
7.7% | 201–202, 215–216, 773, 775, 784–788, 790 |
posthog/models/event/deletion.py |
60.0% | 28, 33 |
posthog/models/deletion_targets.py |
69.9% | 81, 85, 94–96, 148–149, 163–166, 173, 178, 200, 224–226, 231–235 |
🤖 Agents: add a test covering the lines above, or note why under "How did you test this code?". Machine-readable gap list: the patch-coverage artifact on this run (gh run download 31731630034 -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 |
batch_exports |
████████░░░░░░░░░░░░ 39.2% |
8,860 / 22,581 |
warehouse_sources_queue |
█████████░░░░░░░░░░░ 45.9% |
90 / 196 |
demo |
███████████░░░░░░░░░ 56.3% |
1,497 / 2,661 |
data_tools |
█████████████░░░░░░░ 66.2% |
90 / 136 |
tasks |
██████████████░░░░░░ 70.1% |
37,701 / 53,772 |
ai_gateway |
███████████████░░░░░ 75.0% |
9 / 12 |
signals |
█████████████████░░░ 82.9% |
27,748 / 33,465 |
apm |
█████████████████░░░ 83.9% |
1,222 / 1,457 |
cdp |
█████████████████░░░ 84.2% |
3,914 / 4,649 |
data_modeling |
█████████████████░░░ 86.3% |
8,909 / 10,326 |
actions |
█████████████████░░░ 86.6% |
717 / 828 |
wizard |
█████████████████░░░ 86.7% |
1,090 / 1,257 |
notebooks |
█████████████████░░░ 87.1% |
8,512 / 9,769 |
cohorts |
██████████████████░░ 87.6% |
6,527 / 7,451 |
product_tours |
██████████████████░░ 87.9% |
1,303 / 1,482 |
data_warehouse |
██████████████████░░ 88.7% |
12,307 / 13,880 |
managed_warehouse |
██████████████████░░ 88.8% |
6,642 / 7,478 |
business_knowledge |
██████████████████░░ 89.0% |
4,384 / 4,928 |
exports |
██████████████████░░ 89.1% |
7,529 / 8,449 |
engineering_analytics |
██████████████████░░ 89.4% |
6,839 / 7,649 |
dashboards |
██████████████████░░ 89.5% |
6,216 / 6,949 |
visual_review |
██████████████████░░ 89.5% |
5,870 / 6,558 |
alerts |
██████████████████░░ 90.4% |
4,554 / 5,039 |
mcp_analytics |
██████████████████░░ 90.4% |
3,968 / 4,388 |
conversations |
██████████████████░░ 90.5% |
18,399 / 20,324 |
links |
██████████████████░░ 90.6% |
183 / 202 |
streamlit_apps |
██████████████████░░ 90.7% |
2,630 / 2,901 |
error_tracking |
██████████████████░░ 91.2% |
11,233 / 12,323 |
stamphog |
██████████████████░░ 91.3% |
4,505 / 4,936 |
canvas |
██████████████████░░ 92.2% |
2,678 / 2,904 |
slack_app |
██████████████████░░ 92.5% |
11,333 / 12,254 |
managed_migrations |
███████████████████░ 92.6% |
1,556 / 1,681 |
early_access_features |
███████████████████░ 92.9% |
1,347 / 1,450 |
notifications |
███████████████████░ 92.9% |
1,060 / 1,141 |
ai_observability |
███████████████████░ 92.9% |
17,244 / 18,560 |
web_analytics |
███████████████████░ 92.9% |
16,337 / 17,578 |
mcp_store |
███████████████████░ 93.0% |
7,162 / 7,704 |
surveys |
███████████████████░ 93.2% |
5,919 / 6,349 |
posthog_ai |
███████████████████░ 93.2% |
1,326 / 1,422 |
reminders |
███████████████████░ 93.4% |
468 / 501 |
marketing_analytics |
███████████████████░ 93.5% |
16,404 / 17,550 |
approvals |
███████████████████░ 93.5% |
3,491 / 3,734 |
product_analytics |
███████████████████░ 93.6% |
7,124 / 7,611 |
legal_documents |
███████████████████░ 93.8% |
1,628 / 1,736 |
review_hog |
███████████████████░ 94.1% |
9,319 / 9,906 |
endpoints |
███████████████████░ 94.3% |
8,814 / 9,349 |
workflows |
███████████████████░ 94.4% |
8,046 / 8,526 |
tracing |
███████████████████░ 94.4% |
2,678 / 2,837 |
skills |
███████████████████░ 94.7% |
3,869 / 4,085 |
data_quality |
███████████████████░ 95.3% |
1,457 / 1,529 |
logs |
███████████████████░ 95.3% |
12,288 / 12,891 |
replay_vision |
███████████████████░ 95.5% |
20,466 / 21,434 |
messaging |
███████████████████░ 95.6% |
3,423 / 3,580 |
experiments |
███████████████████░ 95.8% |
28,862 / 30,123 |
annotations |
███████████████████░ 96.2% |
732 / 761 |
revenue_analytics |
███████████████████░ 96.3% |
1,887 / 1,960 |
customer_analytics |
███████████████████░ 96.5% |
12,779 / 13,248 |
user_interviews |
███████████████████░ 96.5% |
2,638 / 2,734 |
feature_flags |
███████████████████░ 96.5% |
18,794 / 19,469 |
access_control |
███████████████████░ 96.7% |
873 / 903 |
growth |
███████████████████░ 96.7% |
6,494 / 6,717 |
warehouse_sources |
███████████████████░ 97.5% |
391,561 / 401,605 |
data_catalog |
████████████████████ 97.8% |
2,747 / 2,808 |
analytics_platform |
████████████████████ 98.1% |
2,254 / 2,298 |
metrics |
████████████████████ 98.2% |
2,491 / 2,536 |
pulse |
████████████████████ 98.4% |
2,017 / 2,049 |
live_debugger |
████████████████████ 99.2% |
613 / 618 |
field_notes |
████████████████████ 99.4% |
158 / 159 |
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.
ℹ️ Docs preview — preview build triggered
Docs from this PR will be published at posthog.com.
| Project | Preview | Updated (UTC) |
|---|---|---|
| posthog.com | Open preview | Aug 13, 2026, 6:37 PM |
The preview should be ready in about 10 minutes. Open the preview at /handbook/engineering/.
🦔 ReviewHog reviewed this pull requestFound 0 must fix, 2 should fix, 0 consider. Published 2 findings (view the review). |
|
|
ReviewHog Alpha 🦔 If you find any issues helpful - please reply "valid", "invalid", etc., for evaluation purposes 🙏 |
There was a problem hiding this comment.
ReviewHog Report
Business logic
Issues: 1 issue
Files (4)
posthog/dags/data_deletion_requests.pyposthog/dags/deletes.pyposthog/models/data_deletion_request.pyposthog/clickhouse/cluster.py
What were the main changes
- Event/property/person removal dags now sweep every registered deletion target (including flag_evaluations) instead of only the events tables, using may_hold_any_of to skip tables that can't hold the named events
- New _refuse_unsweepable/_event_removal_targets logic: property removal and HogQL-predicate event removal now fail loudly (naming table + row count) instead of completing while matching flag_evaluations rows survive
- deletes.py (person/team/queued-uuid deletion) now iterates personal_data_tables(cluster) rather than events_data_tables(cluster)
- data_deletion_request.py adds portable_event_removal_where (schema-agnostic subset of the predicate) and extends count_remaining_matching_events to count across all registered read targets
- Fixes a SQL injection in cluster.py's find_existing_mutations: binds rendered mutation commands as parameters instead of interpolating them into a
$__sql$ heredoc, which a value containing the delimiter could break out of
Documentation
Issues: 1 issue
Files (1)
docs/internal/clickhouse-deletion-coverage.md
What were the main changes
- New internal doc cataloguing which sweeps reach which ClickHouse tables, TTL-only tables, and the reasoning behind the two known coverage gaps (property removal and HogQL-predicate event removal not reaching flag_evaluations)
- Documents the person_id parity contract the flag_evaluations producer must uphold and instructions for registering future tables
d312576 to
6735c0c
Compare
Prompt To Fix All With AI### Issue 1
posthog/dags/tests/conftest.py:28
**Complete new test annotations**
The new test helpers and test functions omit required annotations: this helper leaves `client` untyped and uses an unparameterized `tuple`, while related additions also omit return annotations. This weakens static type coverage and violates the repository's strict typing requirement.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "feat(flags): sweep flag_evaluations in d..." | Re-trigger Greptile |
There was a problem hiding this comment.
Pull request overview
Adds flag_evaluations to PostHog’s ClickHouse data-deletion coverage by introducing a central registry of “personal data” tables and updating deletion/verification codepaths to sweep and/or explicitly refuse requests that would otherwise strand undeletable rows.
Changes:
- Introduces
posthog/models/deletion_targets.pyas the single registry of ClickHouse tables that must be swept for person/team/queued-uuid deletions (includingflag_evaluations) and adds a ClickHouse-backed coverage test to prevent future gaps. - Updates event-removal + deferred-queue + verification logic to use the target registry, including explicit refusal paths for HogQL-predicate removals and property-removal requests when matching rows would survive on unsweepable tables.
- Fixes a ClickHouse mutation-deduplication SQL injection vector in
find_existing_mutationsand adds regression coverage.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| posthog/test/base.py | Ensures flag-evaluation tables are dropped/recreated during ClickHouse test DB resets. |
| posthog/models/flag_evaluations/sql.py | Adds the source event constant and drop helpers to support resets and deletion targeting. |
| posthog/models/event/deletion.py | Refactors events-table discovery to use the shared deletion-target registry helpers. |
| posthog/models/deletion_targets.py | New registry defining sweep targets/capabilities and utilities for presence checks + refusal counting. |
| posthog/models/data_deletion_request.py | Uses the registry for cross-table “remaining rows” verification; adds a portable predicate helper. |
| posthog/dags/tests/test_deletes.py | Extends deletes-job tests to assert flag_evaluations rows are swept/drained appropriately. |
| posthog/dags/tests/test_data_deletion_requests.py | Adds coverage for deferred queueing + property-removal refusal behavior involving flag_evaluations. |
| posthog/dags/tests/conftest.py | Adds a shared helper to insert flag_evaluations rows for DAG tests. |
| posthog/dags/deletes.py | Expands deletes-job drain to apply queued UUID deletes to all personal-data tables (via registry). |
| posthog/dags/data_deletion_requests.py | Updates event/property deletion flows to select applicable targets and refuse when rows would strand. |
| posthog/clickhouse/test/test_deletion_coverage.py | New “coverage backstop” test ensuring MergeTree tables with person_properties are swept or explicitly TTL-only. |
| posthog/clickhouse/test/test_cluster.py | Adds regression coverage for delimiter-shaped parameter values in mutation dedup logic. |
| posthog/clickhouse/cluster.py | Fixes SQL injection vector in find_existing_mutations by rendering commands safely and binding as params. |
| docs/internal/clickhouse-deletion-coverage.md | Documents deletion sweep coverage, TTL-only decisions, and known gaps/refusal rationale. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
6735c0c to
a30b555
Compare
flag_evaluations stores the same person payload as events — the full properties JSON, person_properties, person_id and distinct_id — but no deletion scanner reached it, leaving its TTL as the only erasure control. The table is empty everywhere and has no producer, so this is a gate before one lands rather than a live fix. Which tables a deletion must sweep is a property of the data they store, not of the events table, so posthog/models/deletion_targets.py now owns that list and posthog/models/event/deletion.py becomes a shim over it. Per-target capability fields carry what each table's schema can take: which HogQL schema a predicate compiles against, whether its property columns can be rewritten in place, which events it can hold. Person deletion, team deletion, the queued-uuid drain and event removal all sweep the new table, keyed on person_id like every other events-shaped table. That relies on the producer populating person_id as the events pipeline does — the resolved uuid, else a deterministic per-distinct_id uuid — a contract recorded in docs/internal/clickhouse-deletion-coverage.md. Property removal and HogQL-predicate event removal cannot reach the table: its nine typed columns are true ClickHouse MATERIALIZED, so ClickHouse rejects both an UPDATE of them and of properties itself, and no HogQL table definition exists for it. Both refuse loudly when the table holds matching rows rather than completing while data survives. The refusal check is bounded by the request's deletion marker, so a row ingested after the marker — one the sweep itself would never touch — can't refuse the request forever. find_existing_mutations, which every ClickHouse mutation in the repo goes through for dedup, interpolated the command template into a $__sql$ heredoc and let the driver substitute parameters inside it. The driver escapes quotes and backslashes but not `$`, so a parameter value containing the delimiter closed the heredoc early and the remainder parsed as SQL, silently, since the surrounding array kept its length. Mutation parameters were staff-authored until this branch's person sweep put a person's distinct_id there, so that is reachable input now. Each command is rendered in Python and bound as an ordinary parameter instead, and a new test pins a delimiter-shaped parameter value against exactly this construction. A coverage test fails on any storage table that declares person_properties and is neither registered nor listed as TTL-only, so the next events-shaped table forces the same decision. The tables that rely on TTL alone erase later than their TTL suggests: ttl_only_drop_parts means a part drops only once its newest row expires, so the real wait is ~8 days for sharded_events_recent (daily partitions) and ~120 days for flag_evaluations (monthly), documented in docs/internal/clickhouse-deletion-coverage.md alongside the sweep table. Fixes #81002 Generated-By: PostHog Code Task-Id: b6e91f38-dd91-4c39-bfce-322d583c221e
a30b555 to
cf6d474
Compare
orian
left a comment
There was a problem hiding this comment.
Hi Phil, i've read all the code. I think we're safe to go.
|
|
||
| @pytest.mark.django_db | ||
| def test_get_property_removal_shards_refuses_when_flag_evaluations_holds_matching_rows(cluster: ClickhouseCluster): | ||
| # Property removal cannot rewrite flag_evaluations: its typed columns are MATERIALIZED, so |
There was a problem hiding this comment.
Probably not a perfect place for this comment: you shall replace the MATERIALIZED with DEFAULT for those columns.
We don't use MATERIALIZED in events exactly because of it, DEFAULT achieves the same result.
In depth reason: MATERIALIZED is computed when data is saved. If you use DEFAULT instead, the value could be computed on the node that ingest the data, moving the calculation out of the node processing queries (of course, it makes the data to be send over the network so it may / may not make sense).
There was a problem hiding this comment.
Ok! The table's empty, so recreating it is free right now. I'll do the DEFAULT switch as a follow-up PR before the producer ships rather than fold it in here.
|
|
||
| # The same delete must land on every physical events table (legacy + native-JSON), or the | ||
| # tables diverge while both exist. | ||
| # The predicate below reads only team_id, person_id, timestamp and uuid — columns every |
There was a problem hiding this comment.
the previous column explained the reason behind it better
Generated-By: PostHog Desktop Task-Id: b6e91f38-dd91-4c39-bfce-322d583c221e
…valuations-deletion-coverage
|
/trunk merge |
Problem
Nothing deletes a person's flag-evaluation history from ClickHouse.
The
flag_evaluationstable stores the same person data asevents(properties, person_properties, person_id, distinct_id), but no data-deletion scanner reaches it.A 90-day TTL is the only thing that removes that data today, and it can lag past 90 days.
A person-deletion or property-removal request completes while
flag_evaluationsstill holds the person's data.The table is empty everywhere and has no producer yet, so this closes the gap before one lands rather than fixing a live leak.
Closes #81002
Changes
posthog/models/deletion_targets.pyis a new registry listing every table a deletion sweep must reach.flag_evaluationsflag_evaluations.MATERIALIZEDcolumns, so anUPDATEto them, or to the sourcepropertiescolumn, is rejected.person_propertiesand isn't registered here.sharded_events_recent, which stores person data on a TTL and had never been registered or exempted.person_idonly, matching every other events-shaped table.flag_evaluationsproducer already resolvesperson_idthe way the events pipeline does, so nodistinct_idfallback is needed here.docs/internal/clickhouse-deletion-coverage.md.find_existing_mutations.$__sql$delimiter closed a heredoc early, letting the remainder parse as SQL.How did you test this code?
test_storage_tables_carrying_person_properties_are_swept_or_exemptfails if a future table with person data isn't registered or exempted.test_deferred_event_removal_queues_flag_evaluations_and_blocks_promotionfails if a deferred request completes while a queuedflag_evaluationsrow survives.test_get_property_removal_shards_refuses_when_flag_evaluations_holds_matching_rowsfails if the property-removal refusal is silently dropped.test_full_job_person_deletes,test_full_job_team_deletes,test_full_job_adhoc_event_deletes, andtest_delete_person_events_op_runs_lightweight_delete_per_shard.flag_evaluationsrows are actually swept, not just that the code runs.test_find_existing_mutations_handles_delimiter_shaped_parameter_valuefails if the SQL injection fix is reverted.I also ran the person-removal flow by hand against a local dev stack, end to end.
flag_evaluationsrows throughwritable_flag_evaluations, one for a targetperson_idand one bystander.person_removalrequest and randata_deletion_request_person_removalin-process.person_idvalues were fresh random UUIDs, so the same sweep matched zero real rows insharded_events.sharded_flag_evaluationsand deleted the request row afterward.Ran locally, all passing: the deletion-request suite, the deletes-job suite, the new coverage test, and the mutation-dedup suite.
Also ran mypy across the whole repo and ruff, both clean.
Not run: the CI matrix itself, which runs on this PR.
Automatic notifications
This is an internal gate with no user-visible behavior, so it doesn't need a changelog entry.
Docs update
This PR adds
docs/internal/clickhouse-deletion-coverage.md.It lists every table each sweep reaches, the tables that rely on TTL alone, and the reasoning behind the two known gaps above.
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
/writing-tests,/simplify,/review-code --fix,/resolve-conflicts,/squash,/writing-pr-descriptions.distinct_idfallback for rows without a resolvedperson_id.flag_evaluationsproducer already resolvesperson_idthe way events does./review-code --fixpass found and fixed the SQL injection and a separate silent-erasure bug.distinct_idfallback also removed the one path that made the injection reachable through this branch's own code.find_existing_mutationsstands on its own regardless.