Repository navigation
fix(deletions): stop crashing on person-property deletion requests - #97009
Conversation
|
😎 Merged successfully - details. |
🤖 CI report
|
| File | Comment lines | Added lines |
|---|---|---|
posthog/models/deletion_targets.py |
15 | 32 |
posthog/dags/tests/test_data_deletion_requests.py |
12 | 95 |
This check does not block merging. It updates on every push and clears when the share drops.
ℹ️ Docs preview — preview build triggered
Docs from this PR will be published at posthog.com.
| Project | Preview | Updated (UTC) |
|---|---|---|
| posthog.com | Open preview | Sep 8, 2026, 11:32 PM |
The preview should be ready in about 10 minutes. Open the preview at /handbook/engineering/.
a8e7b39 to
05adb2c
Compare
🦔 PostHog Review reviewed this pull requestFound 1 must fix, 0 should fix, 0 consider. Published 1 finding (view the review). Resolved comments: 1 declined, 2 left for you · couldn't handle 1 |
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
@orian its not clear to me who owns this, but GitHub mentioned you edited these files recently so I added you as a reviewer. I added @rorylshanks since this is related to some work he helped me with recently. 😄 |
There was a problem hiding this comment.
🔵 Needs a closer look
It changes safety gating in the data-deletion pipeline (ClickHouse) where subtle predicate mistakes can lead to under-deletion or incorrect refusals, so a human should do the final approval.
Pull request overview
Fixes a crash in the ClickHouse data-deletion property-removal gate when flag_evaluations is included and the (now-dropped) person_properties column is referenced, by making the gate build per-target predicates and selectively omitting person_properties checks for targets that don’t store them.
Changes:
- Add
DeletionTarget.stores_person_properties(and a__post_init__invariant) and set it toFalseforFLAG_EVALUATIONS. - Refactor the “unsweepable rows” gate to accept a
predicate_for(target)callable, enabling per-target predicate narrowing. - Add/adjust tests to validate the narrowed predicate behavior and update the internal coverage doc to document the new capability flag and its consequences.
File summaries
| File | Description |
|---|---|
| posthog/models/deletion_targets.py | Adds stores_person_properties capability + invariant; marks FLAG_EVALUATIONS as not storing person properties. |
| posthog/dags/data_deletion_requests.py | Refactors unsweepable gating to be per-target; narrows property-removal predicate for targets lacking person properties. |
| posthog/dags/tests/test_data_deletion_requests.py | Parameterizes coverage for the narrowed gate behavior on flag_evaluations. |
| docs/internal/clickhouse-deletion-coverage.md | Documents the new capability flag and the intentional “blind spot” tradeoff for flag_evaluations. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![veria-ai[bot]](https://avatars.githubusercontent.com/in/1723776?s=60&v=4){kind=small}
PR overviewAll previously flagged issues have been addressed. No open security concerns remain on this pull request. Security reviewNo open security issues remain on this pull request. Fixed/addressed: 1 · PR risk: 0/10 |
![stamphog[bot]](https://avatars.githubusercontent.com/in/3000332?s=60&v=4){kind=small}
There was a problem hiding this comment.
Approved.
Touches the personal-data deletion gate (privacy-sensitive), but the author has STRONG familiarity with exactly these files/lines (98% of touched lines, 97 merged PRs here in a year, last touch 2 days ago), the fix is narrowly scoped with new parametrized tests covering the changed behavior, and the residual blind spot is explicitly disclosed and TTL-bounded rather than hidden.
- Author wrote 98% of the modified lines and has 97 merged PRs in these paths (familiarity STRONG).
- 👍 on the PR from greptile-apps[bot].
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 105L, 2F substantive, 227L/4F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1c-medium (227L, 4F, two-areas, fix) |
| stamphog 2.0.0b4 | .stamphog/policy.yml @ 05adb2c · reviewed head 05adb2c |
|
PostHog Review alpha 🦔 If you find any issues helpful - please reply "valid", "invalid", etc., for evaluation purposes 🙏 |
![posthog[bot]](https://avatars.githubusercontent.com/in/1203049?s=60&v=4){kind=small}
The property-removal gate refused to run for a request naming a person property, because it built a person_properties presence check against flag_evaluations's copy of that column. The column carries no reachable person-property data (its producer stopped sending it, and prod ClickHouse has already dropped it), so the check raised an unhandled "unknown identifier" instead of the clean refusal the gate is meant to give. Add DeletionTarget.stores_person_properties, false on FLAG_EVALUATIONS, and generalize the unsweepable-rows check to build its predicate per target via a callable, dropping the person_properties half for a target that can't take it. Narrowing, not skipping: a request's event properties half still gates the target as before. A __post_init__ check on DeletionTarget now requires accepts_property_rewrite targets to also set stores_person_properties, turning that pairing into an import-time failure instead of an unenforced comment. A new test covers the _event_removal_placements HogQL-refusal branch for flag_evaluations, which had no coverage. Claude-Session: https://claude.ai/code/session_017RuDFm6NFyFAFxEDM79BT4
05adb2c to
c1a8c64
Compare
Problem
A property-removal deletion request that names a person property crashes the deletion job instead of completing or refusing cleanly, whenever
flag_evaluationsis in scope. The gate checks that table by querying itsperson_propertiescolumn for matching rows. That column holds no data the gate can trust: the producer stopped sending it, and production ClickHouse has already dropped the column in both regions. The gate only catches its own refusal error, so the raw ClickHouse "unknown identifier" error escapes as an unhandled job failure.Changes
flag_evaluationsnow completes, or refuses cleanly on the request's event properties, instead of crashing.DeletionTargetgets astores_person_propertiescapability flag, false onFLAG_EVALUATIONS. The gate drops theperson_propertieshalf of its predicate for a target lacking that capability, narrowing the check rather than skipping the target.DeletionTargetand the internal coverage doc explain the new flag and its dated cause.Warning
The flag is a static, one-time decision, not a live schema check. A
flag_evaluationsrow written before the producer stopped sendingperson_properties(2026-09-05) can still hold a real value wherever the column has not been dropped, and this gate no longer sees it. Exposure is bounded by the table's TTL, up to about 120 days.How did you test this code?
test_get_property_removal_shards_narrows_person_properties_on_flag_evaluationsreplaces the earlier single-case test with three parametrized cases: a person-property-only request, a mixed request that matches only on the person property, and a mixed request that matches only on the event property. Together they prove the gate narrows its check rather than skipping the target outright or suppressing the event-property half.No test reproduces the actual production crash. The local ClickHouse schema still declares
person_propertiesonflag_evaluations, so the unknown-identifier error this PR fixes cannot be reproduced here.Automatic notifications
Docs update
None beyond the internal coverage doc already listed in Changes.
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Claude Code implemented this from a detailed task specification, using
/writing-tests,/simplify,/comment-cleanup,/followup,/commit,/create-pr, and/writing-pr-descriptions, plus acode-reviewersubagent pass. Session: https://claude.ai/code/session_017RuDFm6NFyFAFxEDM79BT4The first implementation matched the task's own sketch: a two-branch split of unsweepable targets. A four-way parallel review (reuse, simplification, efficiency, altitude) converged independently on generalizing the existing
assert_no_unsweepable_rows/_refuse_unsweepablepair to take a per-target predicate callable, the same shapeassert_sweep_completealready uses. That refactor shipped instead of the two-bucket special case.The
code-reviewerpass found that an early comment claimingflag_evaluations"never carries real person-property data" was not backed by git history: the column held real values for about three and a half weeks before the producer change on 2026-09-05. The comments, the coverage doc, and the regression test were rewritten to state that dated window and insert a matching row, rather than assert an unqualified "never".0315); neither is quoted, and that migration is out of scope for this PR.Follow-ups filed, not done here to keep this PR scoped: logging the gate's silent skip on the audit path, adding
flag_evaluationscoverage to the post-sweep verification gate's tests, and considering a livesystem.columnsprobe instead of the static capability flag.