Skip to content

feat(personhog): tombstone by default in DeletePersons #110276

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
nickbest-ph wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

feat(personhog): tombstone by default in DeletePersons #110276

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion nodejs/src/common/generated/personhog/personhog/types/v1/person_pb.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1873,7 +1873,7 @@ export const FoldPersonDocumentResponseSchema: GenMessage<FoldPersonDocumentResp
*/
export enum DeletePersonsMode {
/**
* Same as HARD.
* Same as TOMBSTONE.
*
* @generated from enum value: DELETE_PERSONS_MODE_UNSPECIFIED = 0;
*/
Expand Down
2 changes: 1 addition & 1 deletion posthog/personhog_client/fake_client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -624,7 +624,7 @@ def delete_persons(
self, request: person_pb2.DeletePersonsRequest, timeout: float | None = None
) -> person_pb2.DeletePersonsResponse:
self.calls.append(_Call("delete_persons", request))
tombstone = request.mode == person_pb2.DELETE_PERSONS_MODE_TOMBSTONE
tombstone = request.mode != person_pb2.DELETE_PERSONS_MODE_HARD

@greptile-apps greptile-apps Bot Oct 1, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Fake default lacks test coverage The fake now tombstones when the mode is omitted, but its deletion tests specify either HARD or TOMBSTONE. The old hard-delete default could return without failing the Python suite, making tests that omit the mode behave differently. Parameterize the tombstone test to cover both an omitted mode and explicit TOMBSTONE.

Prompt To Fix With AI 
This is a comment left during a code review.
Path: posthog/personhog_client/fake_client.py
Line: 627

Comment:
**Fake default lacks test coverage** The fake now tombstones when the mode is omitted, but its deletion tests specify either `HARD` or `TOMBSTONE`. The old hard-delete default could return without failing the Python suite, making tests that omit the mode behave differently. Parameterize the tombstone test to cover both an omitted mode and explicit `TOMBSTONE`.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

response = person_pb2.DeletePersonsResponse(tombstoned=tombstone)
for uuid in request.person_uuids:
person = self._persons_by_uuid.get((request.team_id, uuid))
Expand Down
6 changes: 5 additions & 1 deletion posthog/personhog_client/test_fake_client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -613,7 +613,11 @@ def test_wrong_team_touches_nothing(self):

def test_delete_persons_still_removes_tombstoned_and_live_alike(self):
resp = self.client.delete_persons(
person_pb2.DeletePersonsRequest(team_id=self.TEAM_ID, person_uuids=["tombstoned", "live", "blocked"])
person_pb2.DeletePersonsRequest(
team_id=self.TEAM_ID,
person_uuids=["tombstoned", "live", "blocked"],
mode=person_pb2.DELETE_PERSONS_MODE_HARD,
)
)

assert resp.deleted_count == 3
Expand Down
8 changes: 7 additions & 1 deletion posthog/test/persons.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -431,7 +431,13 @@ def delete_person(person: Person) -> None:
person.team_id, did.distinct_id, str(person.uuid), version=(did.version or 0) + 100, is_deleted=True
)

fake.delete_persons(person_pb2.DeletePersonsRequest(team_id=person.team_id, person_uuids=[str(person.uuid)]))
fake.delete_persons(
person_pb2.DeletePersonsRequest(
team_id=person.team_id,
person_uuids=[str(person.uuid)],
mode=person_pb2.DELETE_PERSONS_MODE_HARD,
)
)


def update_person(person: Person) -> None:
Expand Down
2 changes: 1 addition & 1 deletion proto/personhog/types/v1/person.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,7 @@ message UpdatePersonPropertiesResponse {

// How DeletePersons removes the rows.
enum DeletePersonsMode {
// Same as HARD.
// Same as TOMBSTONE.
DELETE_PERSONS_MODE_UNSPECIFIED = 0;
// Remove the rows, tombstoned ones included. For callers that publish no
// ClickHouse tombstones and want nothing left behind, such as a purge.
Expand Down
2 changes: 1 addition & 1 deletion rust/personhog-replica/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@

### Person deletes

`DeletePersons` hard-deletes person and distinct-id rows unless the request asks for `DELETE_PERSONS_MODE_TOMBSTONE`.
`DeletePersons` tombstones person and distinct-id rows unless the request asks for `DELETE_PERSONS_MODE_HARD`.
A tombstone keeps the rows with `is_deleted = true`, the version bumped by one, and person properties scrubbed, and the response reports the versions written so the caller can publish ClickHouse tombstones at exactly those versions.
The row's version counter survives, so a later create on the same key revives it above its own ClickHouse tombstone instead of restarting at version 0.
The tombstone cleanup drain (`DeleteTombstonedPersons`) removes the rows later, once their ClickHouse history is gone.
Expand Down
2 changes: 1 addition & 1 deletion rust/personhog-replica/src/service/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -422,7 +422,7 @@ impl PersonHogReplica for PersonHogReplicaService {
.map_err(|e| Status::invalid_argument(format!("Invalid UUID: {e}")))?;

let mode = match ProtoDeletePersonsMode::try_from(req.mode) {
Ok(ProtoDeletePersonsMode::Unspecified) => DeletePersonsMode::Hard,
Ok(ProtoDeletePersonsMode::Unspecified) => DeletePersonsMode::Tombstone,
Ok(ProtoDeletePersonsMode::Hard) => DeletePersonsMode::Hard,
Ok(ProtoDeletePersonsMode::Tombstone) => DeletePersonsMode::Tombstone,
Err(_) => {
Expand Down
7 changes: 5 additions & 2 deletions rust/personhog-replica/tests/service_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1427,8 +1427,11 @@ async fn test_delete_tombstoned_persons_reports_each_outcome(
ctx.cleanup().await.ok();
}

#[rstest]
#[case::unspecified(DeletePersonsMode::Unspecified as i32)]
#[case::tombstone(DeletePersonsMode::Tombstone as i32)]
#[tokio::test]
async fn test_delete_persons_tombstone_mode_reports_versions() {
async fn test_delete_persons_tombstone_mode_reports_versions(#[case] mode: i32) {
let ctx = ServiceTestContext::new().await;
let person = ctx.insert_person("svc_tomb_mode", None).await.unwrap();

Expand All @@ -1437,7 +1440,7 @@ async fn test_delete_persons_tombstone_mode_reports_versions() {
.delete_persons(Request::new(DeletePersonsRequest {
team_id: ctx.team_id,
person_uuids: vec![person.uuid.to_string()],
mode: DeletePersonsMode::Tombstone as i32,
mode,
}))
.await
.expect("RPC failed")
Expand Down
Loading