feat(personhog): tombstone by default in DeletePersons - #110276
nickbest-ph wants to merge 1 commit into
Conversation
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
🤖 CI report
|
|
[Critical risk] Changes default behavior of person deletion from hard to tombstone. The PR appears safe to merge; the remaining issue is non-blocking test coverage for the Python fake’s new default. Reviews (1) · Last reviewed commit: "feat(personhog): tombstone by default in..." |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| ) -> 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 |
There was a problem hiding this comment.
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!
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 🧰 Additional context used📚 Code guidelines (12)📝 WalkthroughWalkthroughUnspecified person deletion now uses tombstoning in the Rust service and fake client. Hard deletion remains selected explicitly by Priority: ⬇️ Low Merge Risk: 🔵 Low · up to The default change is intentional, and inspected production callers explicitly select a mode. The remaining concerns are limited to the Python fake accepting unknown modes and lacking a test for its new default behavior; address or track these small consistency and coverage gaps. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to Known production deletion and purge callers retain their existing behavior, and the new default uses an established transactional tombstone path. The remaining risk is compatibility with older or external callers that may expect immediate physical deletion without specifying a mode. Retained concerns
Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (1)
posthog/personhog_client/fake_client.py-627-627 (1)
627-627: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winReject unknown delete modes in the fake.
For a valid request with an unrecognized mode such as
99, the Rust service returnsinvalid_argument. The fake treats that mode as tombstone and returns success. A test can therefore accept a request that production rejects.Suggested fix
self.calls.append(_Call("delete_persons", request)) + if request.mode not in ( + person_pb2.DELETE_PERSONS_MODE_UNSPECIFIED, + person_pb2.DELETE_PERSONS_MODE_HARD, + person_pb2.DELETE_PERSONS_MODE_TOMBSTONE, + ): + raise ValueError(f"Unknown DeletePersonsMode {request.mode}") tombstone = request.mode != person_pb2.DELETE_PERSONS_MODE_HARD
🧹 Nitpick comments (1)
posthog/personhog_client/test_fake_client.py (1)
616-620: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the omitted delete mode in the fake client.
This test passes
DELETE_PERSONS_MODE_HARD, and the other fake test passesDELETE_PERSONS_MODE_TOMBSTONE. No fake test omitsmode, so changing omitted-mode handling back to hard deletion can pass all fake-client tests.Add a separate case that omits
modeand asserts tombstoning.Suggested fix
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"], mode=person_pb2.DELETE_PERSONS_MODE_HARD, ) ) assert resp.deleted_count == 3 for uuid in ("tombstoned", "live", "blocked"): assert not self._present(uuid) + def test_delete_persons_unspecified_mode_tombstones(self): + resp = self.client.delete_persons( + person_pb2.DeletePersonsRequest(team_id=self.TEAM_ID, person_uuids=["live"]) + ) + + assert resp.tombstoned + assert resp.deleted_count == 1 + stored = self.client.get_person_by_uuid( + person_pb2.GetPersonByUuidRequest(team_id=self.TEAM_ID, uuid="live") + ).person + assert stored.is_deleted + def test_delete_persons_tombstone_mode_keeps_rows_and_reports_versions(self):
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: PostHog/posthog/.coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: 8fe2f680-7a36-4c35-ad4c-c2a76b213c45
⛔ Files ignored due to path filters (1)
nodejs/src/common/generated/personhog/personhog/types/v1/person_pb.tsis excluded by!**/generated/**
📒 Files selected for processing (7)
posthog/personhog_client/fake_client.pyposthog/personhog_client/test_fake_client.pyposthog/test/persons.pyproto/personhog/types/v1/person.protorust/personhog-replica/README.mdrust/personhog-replica/src/service/mod.rsrust/personhog-replica/tests/service_tests.rs
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 10 remain after this review.
This branch has not been deployed
1 participant
Problem
A caller of the replica's
DeletePersonsRPC that leaves the mode unset gets a hard delete. Every production caller now names a mode, and the hard delete is the order that leaves a reused distinct id hidden in ClickHouse, so it should take an explicit request rather than be the thing you get by omission.TOMBSTONE.HARDon purpose, because they publish no ClickHouse tombstones.Changes
Nothing user-visible changes. No production caller sends an unset mode.
DELETE_PERSONS_MODE_UNSPECIFIEDnow meansTOMBSTONE.HARDkeeps its behavior and stays explicit.HARDexplicitly.How did you test this code?
Test rationale: the replica service test for tombstone mode is parametrized over the unset and explicit modes, so a regression back to a hard default fails it. The fake-client test that expects hard removal now names the mode, which is the same contract the Temporal purge relies on.
Run locally: the replica unit and
service_testscases for DeletePersons against a persons test database, clippy, and the Python suites that go through the fake (personhog_client, models/person, the person API tests, project deletion).Release status
Automatic notifications
Docs update
None.
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Agent: Claude Code, Fable 5.1
--deep: no findings.