feat(flags): sweep flag_evaluations in data deletions

docs/internal/clickhouse-deletion-coverage.md

@@ -0,0 +1,107 @@
+# ClickHouse deletion coverage
+
+Which ClickHouse tables a data deletion reaches, which rely on their TTL instead, and why.
+
+Deleting a person's data is not a property of the events table.
+It is a property of every table that stores rows attributable to a person.
+`posthog/models/deletion_targets.py` is the one list of those tables; this document is the reasoning behind it.
+
+## The sweeps
+
+| Sweep                    | Entry point                      | Predicate columns                          |
+| ------------------------ | -------------------------------- | ------------------------------------------ |
+| Person deletion (async)  | `deletes_job` → `delete_events`  | `team_id`, `person_id`, `timestamp`        |
+| Team deletion            | `deletes_job` → `delete_events`  | `team_id`                                  |
+| Queued uuid drain        | `deletes_job` → `delete_events`  | `team_id`, `uuid`                          |
+| Person removal request   | `delete_person_events_op`        | `team_id`, `person_id`, `timestamp`        |
+| Event removal request    | `execute_event_deletion`         | `team_id`, `timestamp`, `event`, + HogQL   |
+| Property removal request | `process_property_removal_shard` | `properties`, `person_properties`, + HogQL |
+
+The first four use only columns every target declares, so they apply unchanged to any registered table.
+The last two need more, which is what the capability fields on `DeletionTarget` express.
+
+Team deletion for tables that are replicated rather than sharded runs through a separate per-table loop (`delete_team_data_from`), which dispatches to a single host.
+A sharded table must not be registered there — it would sweep one shard.
+The team arm inside `delete_events` covers sharded tables.
+
+## Covered tables
+
+- `sharded_events` — all sweeps.
+- `sharded_events_json` — all sweeps. Optional: only present after the native-JSON migration.
+- `sharded_flag_evaluations` — person, team, queued-uuid and event removal. Not property removal (below). Optional.
+
+## Tables on TTL alone
+
+Listed in `TTL_ONLY_TABLES`.
+Each is a decision that erasure may lag by the retention window.
+
+- `sharded_events_recent` — a transient mirror of the last few days of events, 7-day TTL keyed on `inserted_at`. It partitions by day with `ttl_only_drop_parts = 1`, so a part drops only once its newest row expires: the real worst case is about 8 days plus TTL-merge lag, not a flat 7. Short enough to accept as the erasure bound, and a sweep would race the TTL for little benefit.
+
+Session recordings, the dead letter queue, and logs are likewise TTL-reclaimed.
+That decision predates this document; the older `posthog/models/async_deletion/delete_events.py` records it in a comment, but that module is legacy and is not the source of truth here.
+
+## Known gaps
+
+### Property removal does not reach `flag_evaluations`
+
+The events property-removal path rewrites rows in a staging table and resets each affected materialized column with `ALTER TABLE … UPDATE <col> = ''`.
+That works because `materialize()` creates columns as `DEFAULT <expr>`, which is assignable.
+
+`flag_evaluations` declares its nine typed columns as true ClickHouse `MATERIALIZED`. Measured against ClickHouse 26.6:
+
+- Assigning to one is rejected: `Cannot UPDATE materialized column 'session_id'`.
+- Updating `properties` is rejected too, because `flag_key` is materialized from it and sits in the sort key: `Updated column 'properties' affects MATERIALIZED column 'flag_key', which is a key column`.
+- `CREATE TABLE tmp AS sharded_flag_evaluations ENGINE = MergeTree()` inherits the sort key and the column kinds, so the staging table hits the same rejection.
+
+The planned fix is two follow-ups:
+
+1. Recreate the table with the typed columns as `DEFAULT <expr>`, the kind `materialize()` mints on events.
+   Recreating is only free while the table is empty, so this must land before the producer ships.
+2. Point the events rewrite machinery (column discovery, staging rewrite, shard walk) at the table; today all of it is scoped to `events`.
+
+One open question for the schema follow-up: `flag_key` sits in the sort key, and ClickHouse never accepts `UPDATE` on a key column, whatever its kind.
+Whether `UPDATE properties` is accepted once a `DEFAULT` key column depends on it needs measuring.
+If it is still rejected, the fallback is the heavier rewrite: `INSERT … SELECT` the cleaned rows, let the shard recompute the typed columns on write, then lightweight-delete the originals.
+
+Until the fix exists, `get_property_removal_shards` refuses to start when the table holds rows matching the request, so a request cannot complete while data it named survives.
+The check costs nothing while the table is empty.
+
+This is also why `flag_evaluations` is deliberately absent from `MATERIALIZATION_VALID_TABLES`.
+Adding it would let `materialize()` mint `DEFAULT`-kind columns on a table whose property-removal path cannot reset them.
+
+#### If a request arrives before the fix lands
+
+Today the refusal costs nothing, because the table is empty.
+Once it holds rows, a property removal with `delete_all_events` refuses whenever a single flag-evaluation row carries the named property, and the operator has no way through: `delete_all_events` and `events` are mutually exclusive on the model, so the request cannot be narrowed to exclude `$feature_flag_called`, and the admin Retry button replays the same failure.
+The only exits are waiting out the TTL or shipping the fix above. The table partitions by month with `ttl_only_drop_parts = 1`, so a part drops only once its newest row expires: the real wait is up to about 120 days, not the 90-day TTL. `posthog/models/flag_evaluations/sql.py` says the same thing next to the partition clause.
+
+Refusing beats silently under-deleting, so the gate is the right default.
+If the fix has not landed by the time real traffic hits, the cheaper stopgaps are letting a request exclude event names so an operator can scope around the table, or recording an explicit, audited acknowledgement on the request so an operator can accept the residue rather than being stuck.
+Doing nothing means the first affected GDPR request becomes an escalation.
+
+### Event removal with a HogQL predicate does not reach `flag_evaluations`
+
+`compile_hogql_predicate` resolves against the events HogQL table and emits events-specific physical columns. There is no HogQL table definition for `flag_evaluations`, so the fragment cannot run against it. Requests without a predicate are swept normally; requests with one are refused if the table holds matching rows.
+
+## Producer prerequisite: person_id parity
+
+Every person sweep is keyed on `person_id`, on `flag_evaluations` as on events.
+That is only correct because the producer populates `person_id` for every row, exactly as the events ingestion pipeline does: the resolved `Person.uuid`, or a deterministic per-`distinct_id` uuid when no person was resolved.
+The shadow-routing producer (`posthog-code/flag-evaluations-shadow-routing`) forks the enriched event downstream of person resolution, so it inherits that value for free.
+
+If a future producer emitted rows before person resolution, leaving `person_id` unset, a person deletion would strand them.
+The fix would belong to the producer, not the scanner.
+Keeping the fork downstream of person resolution is the contract, tracked on #81002.
+
+## Related, and deliberately unchanged
+
+`_fetch_stats` counts only the events tables. It feeds `AUTO_APPROVE_MAX_EVENTS`, a cost heuristic rather than a completeness claim, so a request auto-approved as small may move somewhat more rows than measured.
+
+`cleanup_old_events_by_partition` stays events-only. It enforces a multi-year retention floor for a named set of teams, and every other personal-data table already expires sooner under its own TTL.
+
+## Adding a table
+
+Register it in `PERSONAL_DATA_TARGETS`, with capability flags reflecting what its schema can actually take.
+If it is not going to be swept, add it to `TTL_ONLY_TABLES` with the window you are accepting.
+
+`posthog/clickhouse/test/test_deletion_coverage.py` fails on any storage table that declares `person_properties` and appears in neither list, so the decision has to be made rather than skipped.

posthog/clickhouse/cluster.py

@@ -856,7 +856,17 @@ def find_existing_mutations(self, client: Client, commands: Set[str] | None = No
         # ClickHouse normalizes bare references against the connection database when
         # storing the mutation, and a bare-vs-qualified mismatch defeats the join.
         alter_prefix = f"ALTER TABLE {settings.CLICKHOUSE_DATABASE}.{self.table} "
-        per_command_alters = ", ".join(f"$__sql${alter_prefix}{cmd}$__sql$" for cmd in command_list)
+        # Render each command's parameters here and bind the finished text as an ordinary parameter,
+        # rather than interpolating the template into a $__sql$ heredoc and letting the driver
+        # substitute values inside it. The driver escapes quotes and backslashes but not `$`, so a
+        # value containing the heredoc delimiter would close it early and the rest would parse as
+        # SQL. Mutation parameters carry third-party strings (a person's distinct_id), so that is
+        # reachable input, and the injection is silent because the surrounding array keeps its length.
+        rendered_commands = [
+            client.substitute_params(f"{alter_prefix}{cmd}", self.parameters, client.connection.context)
+            for cmd in command_list
+        ]
+        per_command_alters = ", ".join(f"%(__command_{i})s" for i in range(len(rendered_commands)))
         mutations = client.execute(
             f"""
             SELECT mutation_id
@@ -893,10 +903,12 @@ def find_existing_mutations(self, client: Client, commands: Set[str] | None = No
             SETTINGS join_use_nulls = 1
             """,
             {
-                f"__database": settings.CLICKHOUSE_DATABASE,
-                f"__table": self.table,
-                f"__alter_prefix": alter_prefix,
-                **self.parameters,
+                "__database": settings.CLICKHOUSE_DATABASE,
+                "__table": self.table,
+                "__alter_prefix": alter_prefix,
+                # self.parameters are already rendered into __command_*; passing them again would
+                # reintroduce the substitution this avoids.
+                **{f"__command_{i}": text for i, text in enumerate(rendered_commands)},
             },
         )
         assert len(mutations) == len(command_list)

posthog/clickhouse/test/test_cluster.py

@@ -339,6 +339,30 @@ def test_find_existing_mutations_handles_multiline_formatted_command(cluster: Cl
     assert shard_mutations == duplicate
 
 
+def test_find_existing_mutations_handles_delimiter_shaped_parameter_value(cluster: ClickhouseCluster) -> None:
+    """Regression test: `find_existing_mutations` used to interpolate 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 of the value parsed as SQL — silently, since the surrounding array kept
+    its length. A person's `distinct_id` is exactly such attacker-controlled input once it reaches a
+    mutation predicate. Parameters are now rendered via `substitute_params` and bound as an ordinary
+    parameter, so a delimiter-shaped value is just data; pre-fix, this raised a ClickHouse syntax
+    error instead of returning cleanly.
+    """
+    sentinel_uuid = uuid.uuid1()  # keeps the rendered command unique to this run
+
+    runner = AlterTableMutationRunner(
+        table=EVENTS_DATA_TABLE(),
+        commands={"UPDATE person_id = %(uuid)s WHERE distinct_id = %(distinct_id)s"},
+        parameters={"uuid": sentinel_uuid, "distinct_id": "$__sql$"},
+    )
+
+    existing = cluster.map_all_hosts(runner.find_existing_mutations).result()
+    assert all(not mutations for mutations in existing.values()), (
+        "expected no pre-existing mutation for this delimiter-shaped parameter"
+    )
+
+
 def test_alter_mutation_multiple_commands(cluster: ClickhouseCluster) -> None:
     table = EVENTS_DATA_TABLE()
     count = 100

posthog/clickhouse/test/test_deletion_coverage.py

@@ -0,0 +1,45 @@
+from posthog.test.base import BaseTest, ClickhouseTestMixin
+
+from django.conf import settings
+
+from posthog.clickhouse.client import sync_execute
+from posthog.models.deletion_targets import PERSONAL_DATA_TARGETS, TTL_ONLY_TABLES
+
+
+class TestDeletionCoverage(ClickhouseTestMixin, BaseTest):
+    def test_storage_tables_carrying_person_properties_are_swept_or_exempt(self):
+        # A table that stores person_properties stores personal data, so deleting a person has to
+        # reach it. flag_evaluations shipped without any scanner pointed at it precisely because
+        # nothing forced that decision to be made out loud (#81002).
+        #
+        # Keyed on person_properties rather than person_id: person_id is on cohortpeople,
+        # person_static_cohort, person_distinct_id2 and more, which makes for an exemption list too
+        # long to carry any signal. person_properties is the events-mirror signature.
+        #
+        # tmp_ is excluded because the property-removal job's staging tables are created AS
+        # sharded_events, so a leaked one would fail this with a misleading name.
+        rows = sync_execute(
+            """
+            SELECT t.name
+            FROM system.tables t
+            INNER JOIN system.columns c ON c.database = t.database AND c.table = t.name
+            WHERE t.database = %(database)s
+              AND c.name = 'person_properties'
+              AND t.engine LIKE '%%MergeTree%%'
+              AND t.name NOT LIKE 'tmp\\_%%'
+            ORDER BY t.name
+            """,
+            {"database": settings.CLICKHOUSE_DATABASE},
+        )
+        storage_tables = {row[0] for row in rows}
+        assert storage_tables, "expected at least the events tables to declare person_properties"
+
+        accounted_for = {target.data_table for target in PERSONAL_DATA_TARGETS} | TTL_ONLY_TABLES
+        unaccounted = sorted(storage_tables - accounted_for)
+
+        assert not unaccounted, (
+            f"{unaccounted} store person_properties but no deletion sweep reaches them. Register each "
+            "in PERSONAL_DATA_TARGETS (posthog/models/deletion_targets.py), or add it to "
+            "TTL_ONLY_TABLES with the retention window you are accepting as its erasure bound. "
+            "See docs/internal/clickhouse-deletion-coverage.md."
+        )