Repository navigation
fix(persons): reject a split that leaves the person with no distinct ids - #116701
Conversation
A split that moves every distinct id off a person, or that names a main distinct id the person does not own, leaves a live person no event can reach and no cleanup job picks up. The API now returns 400 for both requests, and the split task re-checks before it moves anything. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
😎 Merged successfully - details. |
🤖 CI report
|
| File | Comment lines | Added lines |
|---|---|---|
posthog/api/person.py |
1 | 5 |
posthog/models/person/person.py |
1 | 25 |
This check does not block merging. It updates on every push and clears when the share drops.
|
Risk: Low · 2 low This increment drops the database-timeout retry loop from Sentinel reviewed |
![parameterai[bot]](https://avatars.githubusercontent.com/in/2665233?s=60&v=4){kind=small}
Two splits of the same person could each see an id the other was about to move and together leave the person with no live distinct id. The SplitPerson RPC now claims the source person's lifecycle mark before any row lock, as ingestion merges and the tombstone RPC do, and then refuses with FailedPrecondition a split that would leave no live distinct id. A held mark or row fails the call within a 2s lock timeout. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
eli-r-ph
changed the title
The tombstone RPC locks persons in id order, and the split locked its target persons in no particular order, so a split and a tombstone call naming two of the same persons could deadlock. The comment on the lock timeout now says it bounds each wait, not the whole call. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The merge queue keeps Python and Rust changes in separate lanes, so the SplitPerson lifecycle mark change moves to #116730. This branch keeps the API and task checks only. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
eli-r-ph
changed the title
A split that waits behind a merge of the same person can hit a database lock or statement timeout. The server rolls the whole batch back, so the task now sends it again, up to three times, instead of failing after the user already saw the split succeed. Other errors are not retried. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
![stamphog[bot]](https://avatars.githubusercontent.com/in/3000332?s=60&v=4){kind=small}
There was a problem hiding this comment.
Not approved — this change needs a human reviewer.
Re-add the stamphog label to request another review once you have addressed this.
The change enters API-contract and person-data territory, with owning-team authorship but no current-head reviews. The bot's retained-ID concurrency concern remains unaddressed; request review from @PostHog/team-ingestion and @PostHog/team-product-analytics after addressing it or obtaining explicit acceptance.
- Author wrote 0% of the modified lines and has 16 merged PRs in these paths (familiarity MODERATE).
- @parameterai[bot]'s concurrency concern remains in posthog/models/person/person.py: the retained-ID checks occur separately from the split RPC, so concurrent splits can each pass the check and move complementary IDs. rust/personhog-replica/src/storage/postgres/person.rs locks only requested ID rows and does not enforce a retained ID atomically. Address this concern with server-side transactional enforcement or obtain explicit non-author acceptance before re-requesting; suggested reviewers are Nick Best and Zach Brown.
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 39L, 2F substantive, 85L/4F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1b-small (85L, 4F, single-area, fix) |
| stamphog 2.5.1 | .stamphog/policy.yml @ 2f22581 · reviewed head 2f22581 |
|
/trunk merge |
2 participants
Problem
A split can leave a person with no distinct IDs: it stays live, no event reaches it, and no cleanup removes it.
This happens when a request moves every ID, or names a
main_distinct_idthe person does not own.Changes
Note
A request that moves the person's creating ID along with every other ID now gets a 400.
How did you test this code?
Test rationale: no test covered a split that empties a person.
Release status
Docs update
None.
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Agent: Claude Code, Claude Opus 5.5. Skills:
/writing-tests,/writing-code-comments,/writing-pr-descriptions,/improving-drf-endpoints.🤖 Generated with Claude Code