feat(flags): sweep flag_evaluations in property removal requests by haacked · Pull Request #112066 · PostHog/posthog

haacked · 2026-10-05T18:11:11Z

Problem

A property removal request cannot finish for a team whose feature flag calls carry the named property. The job refuses while the flag_evaluations table holds matching rows, and only the table's TTL clears that, which takes up to about 120 days.
The rewrite from feat(data-deletion): split property removal into per-shard s3-staged ops #110902 sweeps only the two events tables, so it never cleans flag call rows.

Changes

Property removal now rewrites matching flag_evaluations rows with the same per-shard copy, delete, reingest and verify ops it runs on events.
A request naming $feature_flag completes. The cleaned row goes back with an empty flag_key, so per-flag usage counts stop counting it.
The copy also blanks the table's extracted columns (flag_key, response, session_id, request_id, $group_0 to $group_4) when their source property is erased. The job finds them by their column_materializer comments on the flag_evaluations read table.
flag_evaluations stores no person properties, so the job drops that half of a request there. A request naming only person properties skips the table.
A request with a HogQL predicate leaves flag_evaluations out of the sweep, because the predicate only compiles against events. The gate refuses that request while the table holds rows matching its other criteria.
DeletionTarget.accepts_property_rewrite decides which tables the fan-out sweeps, instead of a fixed list of events tables. The gate and the whole-request verify read the same flag.
Each swept table runs on the cluster that holds it, through the placements from feat(clickhouse): sweep events_json with patch parts on the events cluster #112514. sharded_events_json is swept on the events cluster, as on master.
Rewritten flag_evaluations rows keep their Kafka _timestamp, which the backfill reads as its consumer position. Events rows still get the version bump their ReplacingMergeTree needs.
The reingest syncs the replica first, so a retry does not insert a month twice into a table that never merges duplicates away.
A request whose window reaches past the 90-day TTL no longer fails on every retry. A reingested part that holds only expired rows can drop before the reingest or the shard verify counts it.
On a table with DeletionTarget.ttl_days set, both checks count only rows at least two days short of the TTL, on the table and in the staged files alike.
The whole-request verify counts every swept table through a data node, with the same predicate builder as the shard ops.
The admin "remaining" count covers every legacy-schema table the job rewrites, so the Verify button agrees with the job. It skips sharded_events_json, as master does, because its predicate is legacy-only.
The unsweepable-rows gate logs each table it skips without counting, with the reason.
Mechanical: the coverage doc, comments and docstrings.

flag_evaluations stays out of MATERIALIZATION_VALID_TABLES. The job finds the extracted columns through system.columns, and adding the table there would also enroll it in materialize() and the HogQL property planner.

Warning

Merge this only after the #112264 migration has run in US and EU. It stamps flag_evaluations.inserted_at at insert, so a row the consumer reads after the request starts falls outside it, as on events. With the old Kafka-time stamp, a late row could lose its staged copy, wedge verification, or keep the property after the request completes.

Note

A request covering all events now also rewrites the team's whole flag_evaluations range through S3, which lengthens the job. The size estimate an approver sees still counts only events.

How did you test this code?

Test rationale:

test_full_job_property_removal_rewrites_flag_evaluations (3 cases, one with a Kafka replay duplicate) fails if the sweep skips flag_evaluations or an extracted column keeps its old value. It also fails if a duplicate breaks the request, the job queries person_properties there, or a rewritten row loses its Kafka _timestamp. The events full-job tests cannot see this table.
test_verify_property_removal_narrows_person_properties_on_flag_evaluations (5 cases) replaces a fan-out-only version. Verify runs after the shards rewrote rows, so a query error there leaves an erasure half done. It also checks that the admin count matches the job's verify.
test_get_property_removal_shards_refuses_a_hogql_predicate_when_flag_evaluations_holds_matching_rows replaces the old refusal test, now scoped to the HogQL case.
test_an_events_schema_target_cannot_take_the_property_rewrite_without_person_properties is a pure unit test for the remaining DeletionTarget guard.
test_full_job_property_removal_survives_the_ttl_dropping_reingested_flag_evaluations fails if a restore check counts rows past the TTL. Tests build the table without a TTL, so it deletes the expired cleaned row between reingest and verify, where the drop would. It failed on the previous head with the mismatch ReviewHog reported.
test_native_property_removal_gate_checks_retained_copies now runs the gate on a copy of EVENTS_JSON without the flag, since the job sweeps that table after feat(clickhouse): sweep events_json with patch parts on the events cluster #112514.
Seen failing without the change: against the pre-feat(data-deletion): split property removal into per-shard s3-staged ops #110902 source, the HogQL refusal test, one verify case and all three full-job cases fail with the old refusal. On feat(data-deletion): split property removal into per-shard s3-staged ops #110902's head, all three full-job cases fail with the read-table lookup reverted to events.
Ran locally after merging master with feat(clickhouse): sweep events_json with patch parts on the events cluster #112514 and fix(flags): stamp flag_evaluations inserted_at with the insert time #112264: posthog/dags/tests/test_data_deletion_requests.py, test_deletes.py, test_person_overrides.py, posthog/models/test/test_deletion_targets.py, posthog/models/test/test_data_deletion_request.py and posthog/clickhouse/test/test_deletion_coverage.py.
Not checked: a multi-replica shard, prod-sized data, or the extracted-column behavior on production ClickHouse (measured on local 26.6.2).
test_delete_person_events_op_runs_lightweight_delete_per_shard failed once in a full local run and passed three times alone. This PR does not touch the person-deletion path.
test_data_deletion_requests.py stays out of .github/new-events-schema-targets.txt, as that file explains, so no label is needed.

Release status

No feature flag controls this change
This change is behind a feature flag and is not available to users
This change makes a previously flagged feature available to everyone

Automatic notifications

Publish to changelog?

Docs update

Internal only: docs/internal/clickhouse-deletion-coverage.md gains a "Property removal on flag_evaluations" section.

🤖 Agent context

Autonomy: Human-driven (agent-assisted)

Agent: Claude Code, Claude Opus 5.5 (claude-opus-5-5)

I first built this against the old single-op rewrite, which needed a staging sort key override, one row per uuid, and a skip for MATERIALIZED columns. feat(data-deletion): split property removal into per-shard s3-staged ops #110902 replaced that design, so I rebuilt the change on it, where its SELECT-based copy and uuid counting make all three unnecessary.
Review found that the tables the job swept and the tables it gated came from two lists that nothing kept in sync. That led to DeletionTarget.property_rewrite.
I considered restricting the delete to rows with a cleaned twin to close one late-arrival case and dropped it. The restriction needs a subquery on the replicated table, which each replica evaluates at a different time.
CodeRabbit CLI ran with --deep against the merge base. Its one finding, a stale line about sharded_events_json in the coverage doc, is fixed.
ReviewHog pushed four fix commits on the first head: the Kafka _timestamp, the replica sync before reingest, the admin count, and a docstring. I kept them and adapted the admin count to the flag.
The simplify pass found that the sharded_events_json sweep could never copy a row, which led to dropping it and returning to a plain flag. feat(clickhouse): sweep events_json with patch parts on the events cluster #112514 then made that sweep real on the events cluster, and the merge with master took its targets.
ReviewHog found the TTL failure. The fix follows the second option it offered: a per-table TTL on DeletionTarget.
Skills: /writing-tests, /writing-code-comments, /simplify, /reviewing-with-coderabbit, /stacking-prs, /writing-pr-descriptions, /plain-writing, /go.
Duplicate search: feat(data-deletion): split property removal into per-shard s3-staged ops #110902 overlapped and is now this PR's base. Nothing else open covers it.
Nothing here comes from non-public material.

https://claude.ai/code/session_015t1ujRuwiinSWVo39yGvcE

Property removal now runs its per-shard copy, delete, reingest and verify ops on sharded_flag_evaluations as well as the events tables, so a request whose flag calls carry a named property completes instead of being refused. - DeletionTarget.property_rewrite marks each table the job sweeps, and the fan-out builds its targets from it instead of a fixed list. - The job drops the person_properties half for a table that does not store it, and finds extracted columns on each table's own read table. - A request with a HogQL predicate skips flag_evaluations and refuses while the table holds matching rows. - The whole-request verify counts every swept table through a data node. - assert_no_unsweepable_rows logs each target it skips without a count, with the reason. Claude-Session: https://claude.ai/code/session_015t1ujRuwiinSWVo39yGvcE

trunk-io · 2026-10-05T18:11:15Z

😎 Merged successfully - details.

posthog · 2026-10-05T18:11:33Z

🦔 PostHog Review reviewed this pull request

Found 0 must fix, 3 should fix, 2 consider.

Published 5 findings (view the review).

Resolved comments: 4 fixed, 2 declined, 2 already settled, 2 left for you

greptile-apps · 2026-10-05T18:15:27Z

[Critical risk] Extends property deletion to flag evaluations table.

No new blocking issue was identified; the PR appears safe to merge on this review’s findings.

_{Reviews (2) · Last reviewed commit: "fix(flags): leave rows past the TTL out ..."}

Copilot

Copilot review overview

🟡 Changes recommended

Rewriting _timestamp can invalidate the Kafka consumer-lag safety check and permit duplicate backfill ingestion.

Review effort: Balanced
Findings: 1 · 1

Open (2)

Preserve Kafka timestamps when rewriting flag evaluations · New
Reference the existing property rewrite targets helper · New

What changed in this PR

Extends property-removal requests to rewrite matching flag_evaluations rows alongside events.

Changes:

Adds target-specific property-rewrite capabilities and verification.
Resets extracted flag-evaluation columns during rewrites.
Adds integration coverage and updates deletion documentation.

File	Description
`posthog/models/test/test_deletion_targets.py`	Tests rewrite capability validation.
`posthog/models/flag_evaluations/sql.py`	Updates extracted-column documentation.
`posthog/models/deletion_targets.py`	Registers rewrite capabilities and skip reasons.
`posthog/models/data_deletion_request.py`	Clarifies event-only admin counts.
`posthog/dags/tests/test_data_deletion_requests.py`	Tests flag-evaluation rewriting and verification.
`posthog/dags/data_deletion_requests.py`	Extends rewrite fan-out and verification.
`docs/internal/clickhouse-deletion-coverage.md`	Documents coverage, limitations, and recovery behavior.

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

hosthog · 2026-10-05T18:20:22Z

HostHog preview — `posthog-desktop-web`

The previews for this PR have been torn down and no longer serve.

coderabbitai · 2026-10-05T18:26:03Z

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

@coderabbitai resume to resume automatic reviews.
@coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

▶️ Resume reviews
🔍 Trigger review

📝 Walkthrough

Walkthrough

Property-removal requests now use declared rewrite coverage and target-specific criteria. The workflow gates unsupported rewrites and HogQL-excluded requests, then copies, deletes, and verifies rows on eligible targets. Verification counts remaining rows across targets. Tests and documentation cover flag-evaluation rewrites and their limitations.

Priority: ➖ Normal

Merge Risk: 🔵 Low · up to f4173

The change extends property removal to flag evaluations with target-aware rewrite and verification. The remaining issue is stale wording that could lead an operator to think an erasure is incomplete when it is complete. It is safe to merge once the comments and docs are cleaned up.

Security Architecture Review

Security architecture risk: 🟡 Moderate · up to f4173

A failed removal resumed after a later removal can restore personal data covered by the later completed request. Approval and team scoping limit exposure, but completed deletion guarantees need to remain valid through recovery.

Retained concerns

Medium · security · inferred: A failed rewrite can retain a snapshot containing properties outside its own removal criteria. If request A deletes originals and fails before restoration, the sensor can run an already-approved overlapping request B because A is no longer active. B can verify and complete while A's rows are absent. Resuming A then inserts its older snapshot, restoring B's targeted property; A verifies only its own criteria. The active-run exclusion does not fence this sequential failure/recovery case. The underlying protocol already affected events, but enabling it for flag evaluations introduces this recovery exposure for additional personal telemetry that the base would have refused to rewrite.

Security review details

Security Blast Radius

inferred — The recovery concern is bounded by the overlapping requests' team, event/time scope, and staged rows. It can restore unnamed personal properties in flag telemetry after a later request completes, but the inspected path does not establish cross-team access or privilege escalation. Original event timestamps retain the table's TTL boundary.

Security Findings and Attack Paths

inferred — The supported security concern is an interrupted-workflow privacy failure, not a demonstrated unauthenticated attack: fail A after deletion, complete approved overlapping B while A's rows are absent, then resume A's restoration. Request-specific verification does not enforce B's previously completed erasure against A's older snapshot.

Trust Boundaries and Controls

observed — Request criteria do not select arbitrary storage tables. Targets resolve through the code-owned registry, execution loads an approved request under a row lock, and generated predicates retain parameterized team and time bounds. Unsupported target capabilities are checked before mutation and again before completion.

Resilience and Maintainability Implications

observed — Persisted progress, staged-count checks, synchronous deletion, UUID-scoped retry cleanup, and post-verification cleanup protect recovery within one request. The sensor's one-active-run rule provides normal execution serialization, but does not reserve recovery ownership for failed requests.

Hardening Proposals

proposed — Preserve deletion ordering across failed recovery states: fence overlapping requests while an unrestored snapshot exists, or apply subsequent completed deletion policies when restoring that snapshot. Treat recovery ownership as lasting beyond the lifetime of an active run.

🚥 Pre-merge checks | ✅ 1

✅ Passed checks (1 passed)

Check name	Status	Explanation
Description check	✅ Passed	The description explains the problem, user-visible changes, testing, release status, documentation, and agent context. It includes detailed regression coverage and limitations. The Release status sect…

✨ Finishing Touches

📝 Generate docstrings

Commit to this branch
Create a new PR

🧪 Generate unit tests (beta)

Commit to this branch
Create a new PR

Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

_{Comment @coderabbitai help to get the list of available commands.}

coderabbitai

Actionable comments posted: 1

🧹 Nitpick comments (1)

posthog/dags/tests/test_data_deletion_requests.py (1)

2687-2724: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win

Add unaffected flag-evaluation controls.

Every inserted row has both candidate properties and falls within the request window. The test can pass if the rewrite ignores the property filter, and it has no time-boundary control. _flag_evaluation_rows reads only PROP_TEAM_ID, so it cannot detect changes to another team’s rows. Add rows without the requested property and outside the request window, plus a row for another team that you read separately. Assert that all remain unchanged.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository: PostHog/posthog/.coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: 0b62db7d-0bdd-493e-b9db-c12ba65a5c18

📥 Commits

Reviewing files that changed from the base of the PR and between b4cc77c and 1385e70.

📒 Files selected for processing (7)

docs/internal/clickhouse-deletion-coverage.md
posthog/dags/data_deletion_requests.py
posthog/dags/tests/test_data_deletion_requests.py
posthog/models/data_deletion_request.py
posthog/models/deletion_targets.py
posthog/models/flag_evaluations/sql.py
posthog/models/test/test_deletion_targets.py

Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review.

posthog · 2026-10-05T18:36:17Z

PostHog Review alpha 🦔 If you find any issues helpful - please reply "valid", "invalid", etc., for evaluation purposes 🙏

posthog

PostHog Review

Found 3 should fix, 2 consider.

The admin Verify button marks a property removal completed when count_remaining_property_events returns zero, but that count read only events. With flag_evaluations now swept, a request whose events chain finished and whose flag_evaluations chain failed could be marked completed while flag rows still held the property. The count now loops over every default deletion target the job rewrites, the same targets the job sweeps: - Extracted columns are found on each target's own read table. - A target without person_properties drops that half, and is skipped when the request names only person properties. - A target that cannot take the HogQL fragment is counted without it, which matches a superset and can only hold a request open. The flag_evaluations verify test now also asserts that the admin count agrees with the job's verify on each case. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Generated-By: PostHog Desktop Task-Id: 1d01e0a4-f637-41c7-b519-c09a2dd1e3a2

github-actions · 2026-10-05T18:47:14Z

🤖 CI report

⚠️

Trunk lane — backend Python lane

This PR is assigned to the backend Python lane. It runs backend Python tests and may merge in parallel with PRs in other lanes.

⚠️

Duplication (Python) — 1 new duplicated block (worst 99 tokens)

New Python code duplication introduced by this branch. Fails at 70+ tokens in app code, or 150+ tokens when both copies live in test files. Advisory while the gate proves itself: extract a shared helper instead of copying.

First copy	Second copy	Lines	Tokens
`posthog/models/data_deletion_request.py:824`	`posthog/models/data_deletion_request.py:967`	19	99

✅ Duplication (TypeScript) — clean

New TypeScript code duplication introduced by this branch. Fails at 70+ tokens in app code, or 150+ tokens when both copies live in test files. Advisory while the gate proves itself: extract a shared helper instead of copying.

🚨 Comment density — 10% of added code lines are comments (50 of 490)

This section warns when comments are more than 3% of the code lines a PR adds, and alerts above 6%. Before agent-assisted PRs, the typical share was about 2%. Only full-line comments count. Docstrings, generated files, snapshots, migrations, and workflow files are left out.

Comments that restate the code, record how the change came about, or narrate the next line add noise for the next reader. Keep the comments that explain a reason the code cannot show, and remove the rest. See .agents/skills/writing-code-comments/SKILL.md for the house rules.

Files with the most added comment lines:

File	Comment lines	Added lines
`posthog/models/deletion_targets.py`	17	42
`posthog/dags/tests/test_data_deletion_requests.py`	16	185
`posthog/dags/data_deletion_requests.py`	10	189
`posthog/models/flag_evaluations/sql.py`	7	7

This check does not block merging. It updates on every push and clears when the share drops.

⚠️

Playwright — 2 flaky

🎭 Playwright report · View test results →

⚠️ 2 flaky tests:

Logout in another tab results in logout in the current tab too (chromium)
Inline editing insight description via compact card popover (chromium)

These issues are not necessarily caused by your changes.
Annoyed by this section? Help fix flakies and failures and it will go green!

⚠️

Backend snapshots — 1 updated (1 modified, 0 added, 0 deleted)

Query snapshots: Backend query snapshots updated

Changes: 1 snapshots (1 modified, 0 added, 0 deleted)

What this means:

Query snapshots have been automatically updated to match current output
These changes reflect modifications to database queries or schema

Next steps:

Review the query changes to ensure they're intentional
If unexpected, investigate what caused the query to change

Review snapshot changes →

The property-removal copy replaced _timestamp with the marker plus one second on every table. That bump is the ReplacingMergeTree version on the events tables. sharded_flag_evaluations is a plain MergeTree, so the bump has no effect on its merges. There _timestamp is the Kafka message time, and flag_evaluations_backfill reads max(_timestamp) per partition as the consumer position. A rewritten row could make a stalled partition look current and let the backfill pass its lag guard. The copy now reads the table engine and bumps _timestamp only on a ReplacingMergeTree. flag_evaluations keeps the original value, and inserted_at still carries the marker. The flag_evaluations full-job test pins a Kafka time on its rows and asserts that it survives the rewrite. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Generated-By: PostHog Desktop Task-Id: 1d01e0a4-f637-41c7-b519-c09a2dd1e3a2

The copy, delete and shard verify ops each run SYSTEM SYNC REPLICA LIGHTWEIGHT before they read. The reingest did not. A re-execution can land on another replica of the shard than the failed attempt. If that replica has not fetched the month the earlier attempt inserted, the leftover count misses those rows and the insert adds them again with deduplication off. On events the ReplacingMergeTree merges the extra copies away. sharded_flag_evaluations is a plain MergeTree, so they stay and inflate per-flag usage counts. The reingest now syncs the replica once before its month loop, as its sibling ops do. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Generated-By: PostHog Desktop Task-Id: 1d01e0a4-f637-41c7-b519-c09a2dd1e3a2

…ring The _refuse_property_removal_unsweepable docstring pointed at _property_rewrite_scopes, which does not exist. The helper that leaves HogQL-excluded targets out of the sweep is _property_rewrite_targets. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Generated-By: PostHog Desktop Task-Id: 1d01e0a4-f637-41c7-b519-c09a2dd1e3a2

trunk-io · 2026-10-05T19:10:44Z

Failed Test	Failure Summary	Logs
`Dashboard compact cards and inline editing › Inline editing insight description via compact card popover`	The test exceeded the maximum allowed time without completing.	Logs ↗︎
`Auth › Logout in another tab results in logout in the current tab too`	The page reload was aborted, possibly because the frame was detached.	Logs ↗︎

_{View Full Report ↗︎ ⋅ Docs}

coderabbitai

Caution

Some comments are outside the diff and can’t be posted inline due to GitHub limitations.

⚠️ Outside diff range comments (1)

🟠 Major · Gate final verification on Kafka progress through the marker. · data_deletion_requests.py:1710-1759

posthog/dags/data_deletion_requests.py:1710-1759
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift

Gate final verification on Kafka progress through the marker.

FLAG_EVALUATIONS now enters property-removal targeting, but its Kafka materialized view uses message time for inserted_at. If a pre-marker message reaches ClickHouse after verify_property_removal runs, the count misses it. The job then cleans staging and marks the immediate request COMPLETED, leaving the property in the new row. Wait for the consumer to process through the marker before running the final check.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository: PostHog/posthog/.coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: e7092da6-50bf-4997-b724-d4334ecef2ba

📥 Commits

Reviewing files that changed from the base of the PR and between 1385e70 and 8a1397a.

📒 Files selected for processing (3)

posthog/dags/data_deletion_requests.py
posthog/dags/tests/test_data_deletion_requests.py
posthog/models/data_deletion_request.py

Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 4 remain after this review.

- accepts_property_rewrite is a plain flag again, and the fan-out sweeps only tables that set it. sharded_events_json keeps it off, so the gate alone handles that table instead of shard ops that copy nothing once the gate passes. - The whole-request verify builds its predicate with _shard_predicate and counts with surviving_rows_sql, one table per future. - _scoped_to returns the narrowed request, and the empty-request check sits in the two places that skip a table. - The coverage doc describes how property removal treats sharded_events_json. - The admin remaining count skips tables without accepts_property_rewrite. Claude-Session: https://claude.ai/code/session_015t1ujRuwiinSWVo39yGvcE

coderabbitai

Actionable comments posted: 2

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository: PostHog/posthog/.coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: bdf25bef-f7b5-44fb-9c74-f5a295178ed5

📥 Commits

Reviewing files that changed from the base of the PR and between 8a1397a and 97ae026.

📒 Files selected for processing (5)

docs/internal/clickhouse-deletion-coverage.md
posthog/dags/data_deletion_requests.py
posthog/models/data_deletion_request.py
posthog/models/deletion_targets.py
posthog/models/test/test_deletion_targets.py

Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review.

…hecks sharded_flag_evaluations drops a part once its newest row passes the 90-day TTL. A request window can reach expired rows that a live part still holds, and the copy stages them. A reingested part that holds only those rows can drop before the reingest or the shard verify counts it, so both checks reported missing uuids and every retry failed the same way. DeletionTarget.ttl_days now marks a table with a TTL. On such a table both checks compare only rows at least two days short of their TTL, counted with one cutoff on the table and on the staged files. A part that holds one of those rows cannot drop. Tables without a TTL keep the counts the copy recorded. Claude-Session: https://claude.ai/code/session_015t1ujRuwiinSWVo39yGvcE

parameterai · 2026-10-05T23:21:39Z

Risk: No findings

The only change since the last review is one automated commit (tests-posthog[bot], "test(backend): update query snapshots") that regenerates the .ambr snapshot file for dashboard tests — renumbering recorded queries and adding a new snapshot of the dashboard-tiles serializer query. No production code changed. The snapshot contains only column-name lists from recorded SQL (names like api_token and password are column references, not values), so there is no security impact.

_{Sentinel reviewed 1815f93 · Review settings}

…tions-property-removal

stamphog

Not approved — escalated to a human reviewer.

Re-add the stamphog label to request another review once you have addressed this.

This extends irreversible data-erasure logic to a new table, which is risky territory. There's an unresolved thread describing a real failure: rows past the 90-day TTL can make verification fail on every retry. The author's familiarity is only moderate and no human or owning-team reviewer has signed off.

Author wrote 48% of the modified lines and has 52 merged PRs in these paths (familiarity MODERATE).
👍 on the PR from greptile-apps[bot].
Unresolved inline thread (@PostHog on posthog/dags/data_deletion_requests.py): rows past the flag_evaluations 90-day TTL can be dropped by a TTL merge after reingest, so the uuid count no longer matches and every retry fails verification. The thread says a human must choose between leaving expired rows out of the sweep or adding per-table TTL handling on DeletionTarget.
The PR's own warning describes late-arrival races, caused by inserted_at being stamped with the Kafka message time. In some of these cases a row is deleted without a staged copy, or keeps the property after the request completes. The fix is deferred to a follow-up change to the materialized view.
CodeRabbit raised an unresolved concern about gating the final verify on Kafka consumer progress. Copilot raised one about preserving the Kafka _timestamp, which the PR says it addressed.

Gate mechanics and policy version

Gate		Result
prerequisites	✓	all clear
deny-list	✓	no deny categories matched
size	✓	511L, 4F substantive, 853L/7F incl. docs/generated/snapshots — within ceiling
tier	✓	T1-agent / T1d-complex (853L, 7F, two-areas, feat)
stamphog 2.3.1		`.stamphog/policy.yml` @ `d74301c` · reviewed head `d74301c`

…tions-property-removal # Conflicts: # docs/internal/clickhouse-deletion-coverage.md # posthog/dags/data_deletion_requests.py # posthog/dags/tests/test_data_deletion_requests.py # posthog/models/deletion_targets.py

coderabbitai

Note

Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.

🟡 Other comments (1)

posthog/models/deletion_targets.py-104-115 (1)

104-115: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Remove the stale pre-cutover blind-spot text.

The comment says rows stored before the producer stopped sending person_properties "hold real values wherever the column survives." The column was dropped on both prod clusters, and posthog/models/flag_evaluations/sql.py no longer declares it. So no stored value is left in reach. The same claim appears at Lines 206-208. It also appears in docs/internal/clickhouse-deletion-coverage.md Line 159. An operator who reads this text can conclude that an erasure is incomplete when it is complete. Remove the blind-spot sentences, or state that the column is absent.

This follows from the author's clarification on the earlier review thread.

Source: Learnings

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository: PostHog/posthog/.coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: 72988ec1-f028-4818-8395-9708f8e64354

📥 Commits

Reviewing files that changed from the base of the PR and between d74301c and f417331.

📒 Files selected for processing (6)

docs/internal/clickhouse-deletion-coverage.md
posthog/dags/data_deletion_requests.py
posthog/dags/tests/test_data_deletion_requests.py
posthog/models/data_deletion_request.py
posthog/models/deletion_targets.py
posthog/models/flag_evaluations/sql.py

Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review.

stamphog

Not approved — escalated to a human reviewer.

Re-add the stamphog label to request another review once you have addressed this.

This extends the destructive copy, delete and reingest path for customer data deletion to a new production table. The only assurance is bot reviews with some findings still open, the author's familiarity is only moderate, and merging safely depends on a migration having already run in US and EU. A teammate who knows this area needs to review it. Suggested reviewers: Paweł Szczur, Rory Shanks.

Author wrote 47% of the modified lines and has 54 merged PRs in these paths (familiarity MODERATE).
coderabbitai[bot] reviewed the current head.
Risky territory: the property-removal job now deletes and reinserts rows in flag_evaluations in production, and a mistake there means data loss or leaves the property in place. The only review on the current head is a CodeRabbit COMMENTED review, which has a docs nit still open.
The PR description says to merge only after the #112264 migration has run in US and EU. Otherwise late rows can lose their staged copy, wedge verification, or keep the property after the request completes. A human needs to confirm that ordering.
The author lists as untested a multi-replica shard, prod-sized data and the extracted-column behavior on production ClickHouse. Those are the conditions where a delete, reingest and verify cycle is most likely to go wrong.

Gate mechanics and policy version

Gate		Result
prerequisites	✓	all clear
deny-list	✓	no deny categories matched
size	✓	514L, 4F substantive, 849L/7F incl. docs/generated/snapshots — within ceiling
tier	✓	T1-agent / T1d-complex (849L, 7F, two-areas, feat)
stamphog 2.3.1		`.stamphog/policy.yml` @ `f417331` · reviewed head `f417331`

deployment-status-posthog · 2026-10-07T18:42:09Z

Deploy status

Environment	Status	Deployed At	Workflow
dev	✅ Deployed	2026-10-07 18:42 UTC	Run
prod-us	✅ Deployed	2026-10-07 18:56 UTC	Run
prod-eu	✅ Deployed	2026-10-07 18:58 UTC	Run

haacked self-assigned this Oct 5, 2026

haacked added the reviewhog label Oct 5, 2026

haacked requested a balanced review from Copilot October 5, 2026 18:11

Copilot started reviewing on behalf of haacked October 5, 2026 18:11

View session

greptile-apps Bot reviewed Oct 5, 2026

View reviewed changes

Comment thread posthog/models/deletion_targets.py Resolved

Comment thread posthog/models/data_deletion_request.py Resolved

Copilot AI reviewed Oct 5, 2026

View reviewed changes

Comment thread posthog/models/deletion_targets.py Resolved

Comment thread posthog/dags/data_deletion_requests.py Resolved

coderabbitai Bot reviewed Oct 5, 2026

View reviewed changes

Comment thread posthog/models/deletion_targets.py Resolved

posthog Bot reviewed Oct 5, 2026

View reviewed changes

posthog Bot removed the reviewhog label Oct 5, 2026

posthog Bot added 3 commits October 5, 2026 18:58

coderabbitai Bot reviewed Oct 5, 2026

View reviewed changes

haacked added the reviewhog label Oct 5, 2026

coderabbitai Bot reviewed Oct 5, 2026

View reviewed changes

Comment thread docs/internal/clickhouse-deletion-coverage.md Resolved

haacked marked this pull request as ready for review October 5, 2026 23:21

haacked added the stamphog Request AI approval (no full review) label Oct 5, 2026

github-actions Bot added feature/feature-flags Feature Tag: Feature flags team/feature-flags labels Oct 5, 2026

github-project-automation Bot added this to Feature Flags Oct 5, 2026

Merge remote-tracking branch 'origin/master' into haacked/flag-evalua…

d74301c

…tions-property-removal

stamphog Bot reviewed Oct 5, 2026

View reviewed changes

stamphog Bot removed the stamphog Request AI approval (no full review) label Oct 5, 2026

parameterai Bot reviewed Oct 5, 2026

View reviewed changes

Comment thread posthog/models/deletion_targets.py Resolved

haacked mentioned this pull request Oct 6, 2026

fix(flags): stamp flag_evaluations inserted_at with the insert time #112264

Merged

coderabbitai Bot reviewed Oct 6, 2026

View reviewed changes

haacked added the stamphog Request AI approval (no full review) label Oct 6, 2026

stamphog Bot reviewed Oct 6, 2026

View reviewed changes

stamphog Bot removed the stamphog Request AI approval (no full review) label Oct 6, 2026

test(backend): update query snapshots

1815f93

haacked requested review from orian and rorylshanks October 7, 2026 00:25

bciaraldi approved these changes Oct 7, 2026

View reviewed changes

trunk-io Bot merged commit 5c90b9b into master Oct 7, 2026

trunk-io Bot deleted the haacked/flag-evaluations-property-removal branch October 7, 2026 18:16

haacked mentioned this pull request Oct 20, 2026

Sprint - October 19 to October 30 #116781

Open

Conversation

haacked commented Oct 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Changes

How did you test this code?

Release status

Automatic notifications

Docs update

🤖 Agent context

Uh oh!

trunk-io Bot commented Oct 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

posthog Bot commented Oct 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦔 PostHog Review reviewed this pull request

Uh oh!

greptile-apps Bot commented Oct 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

hosthog Bot commented Oct 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

HostHog preview — posthog-desktop-web

Uh oh!

coderabbitai Bot commented Oct 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews paused

Walkthrough

Security Architecture Review

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

posthog Bot commented Oct 5, 2026

Uh oh!

posthog Bot left a comment

Choose a reason for hiding this comment

PostHog Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Oct 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🤖 CI report

Query snapshots: Backend query snapshots updated

Uh oh!

trunk-io Bot commented Oct 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

parameterai Bot commented Oct 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stamphog Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

haacked commented Oct 5, 2026 •

edited

Loading

trunk-io Bot commented Oct 5, 2026 •

edited

Loading

posthog Bot commented Oct 5, 2026 •

edited

Loading

greptile-apps Bot commented Oct 5, 2026 •

edited

Loading

hosthog Bot commented Oct 5, 2026 •

edited

Loading

HostHog preview — `posthog-desktop-web`

coderabbitai Bot commented Oct 5, 2026 •

edited

Loading

github-actions Bot commented Oct 5, 2026 •

edited

Loading

trunk-io Bot commented Oct 5, 2026 •

edited

Loading

parameterai Bot commented Oct 5, 2026 •

edited

Loading

deployment-status-posthog Bot commented Oct 7, 2026 •

edited

Loading