Conversation
Add five personhog RPCs for the ClickHouse person cleanup jobs: - GetPersonVersionHeads and GetDistinctIdVersionHeads read the stored version of each row, tombstones included, from a replica. - EnsurePersonVersionFloors and EnsureDistinctIdVersionFloors raise each tombstone to a minimum version on the primary, inserting a tombstone where no row exists, so a later revival by ingestion lands above the floor. A missing distinct id owner gets a version 0 person tombstone. A live row is left unchanged and comes back as LIVE with its current version. - TombstoneDistinctIds tombstones orphaned distinct id rows (is_deleted, version + 1), live rows whose person row does not exist, and reports the version each row holds. A live row whose person row exists comes back as NOT_ORPHANED, unchanged. Each write takes at most 250 keys in one transaction, locks rows in id order with a 2s lock timeout, and fails whole with FAILED_PRECONDITION when it loses an insert race. The Python client, fake client and proto re-exports cover the new RPCs. The helpers in posthog/models/person/util.py batch by the replica cap, retry a lost race a bounded number of times, and tombstone_distinct_ids_and_publish publishes the person_distinct_id2 tombstone at exactly the version Postgres returned. Node stubs and the test SERVICE_DEFAULTS are regenerated to match.
|
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
|
| First copy | Second copy | Lines | Tokens |
|---|---|---|---|
nodejs/src/common/personhog/client.test.ts:97 |
nodejs/src/common/personhog/persons.test.ts:32 |
68 | 727 |
⚠️ Comment density — 4% of added code lines are comments (126 of 3456)
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 |
|---|---|---|
rust/personhog-replica/src/storage/postgres/person.rs |
36 | 728 |
rust/personhog-replica/tests/storage_tests.rs |
32 | 956 |
rust/personhog-replica/src/storage/traits/person.rs |
16 | 45 |
posthog/models/person/util.py |
11 | 249 |
rust/personhog-replica/src/service/mod.rs |
9 | 247 |
rust/personhog-replica/src/storage/types/person.rs |
8 | 70 |
posthog/personhog_client/fake_client.py |
5 | 169 |
rust/personhog-replica/src/service/tests/mod.rs |
4 | 203 |
This check does not block merging. It updates on every push and clears when the share drops.
|
[High risk] Adds new RPC methods to the person service API contract. The PR should not merge until person-floor requests reject duplicate logical UUIDs, which can produce incorrect reported versions. Reviews (1) · Last reviewed commit: "feat(personhog): add version head, versi..." |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
HostHog preview —
|
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 🧰 Additional context used📚 Code guidelines (10)No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (5)
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughPersonHog adds RPCs to retrieve person and distinct-ID version heads, ensure version floors, and tombstone orphaned distinct IDs. The Rust service validates requests and maps storage outcomes to protocol results. PostgreSQL storage implements the reads and writes with row locking and race handling. Python clients add routed helpers, a fake client, batching, retries for Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to The new reconciliation helpers respect the server’s batch limit. No actionable merge-blocking issue is established. Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to The repair operations have bounded transactions and explicit retry outcomes. However, accepted terminal version values can leave newly created tombstones without a valid revival version, and the restrictions on who may invoke these operations remain unconfirmed. Retained concerns
Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 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/models/person/util.py-1140-1146 (1)
1140-1146: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winBatch the write helpers by the replica's fixed 250-key cap, not by
PERSONHOG_BATCH_SIZE.
- The replica rejects write batches larger than
MAX_LOCKED_WRITE_BATCH_SIZE = 250withINVALID_ARGUMENT.PERSONHOG_BATCH_SIZEcomes fromsettings.PERSONHOG_BATCH_SIZE, which is configurable.- If an operator raises that setting above 250, every
ensure_*_version_floorsandtombstone_distinct_ids_in_postgrescall with more than 250 keys fails._retry_lost_racedoes not retryINVALID_ARGUMENT, so these calls fail without recovery.- The helpers' docstrings and the PR description say they batch by the replica cap.
Add a module constant capped at 250 and use it in the three write loops.
Proposed fix
VERSION_FLOOR_ATTEMPTS = 3 +# The replica's per-request cap for the locked version-floor and tombstone writes. +VERSION_WRITE_BATCH_SIZE = min(PERSONHOG_BATCH_SIZE, 250)- for i in range(0, len(floors), PERSONHOG_BATCH_SIZE): + for i in range(0, len(floors), VERSION_WRITE_BATCH_SIZE): request = EnsurePersonVersionFloorsRequest( team_id=team_id, floors=[ PersonVersionFloorProto(person_uuid=str(f.uuid), min_version=f.min_version) - for f in floors[i : i + PERSONHOG_BATCH_SIZE] + for f in floors[i : i + VERSION_WRITE_BATCH_SIZE] ],Apply the same change in
ensure_distinct_id_version_floorsandtombstone_distinct_ids_in_postgres.Also applies to: 1171-1179, 1206-1210
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository: PostHog/posthog/.coderabbit.yaml
- Review profile: QUIET
- Plan: Enterprise
- Run ID:
4ba11d2d-7ebc-47cb-ac8f-eb068597763e
⛔ Files ignored due to path filters (2)
nodejs/src/common/generated/personhog/personhog/service/v1/service_pb.tsis excluded by!**/generated/**nodejs/src/common/generated/personhog/personhog/types/v1/person_pb.tsis excluded by!**/generated/**
📒 Files selected for processing (39)
nodejs/src/common/personhog/client.test.tsnodejs/src/common/personhog/persons.test.tspackages/personhog-proto/personhog/service/v1/service_pb2.pypackages/personhog-proto/personhog/service/v1/service_pb2_grpc.pypackages/personhog-proto/personhog/types/v1/person_pb2.pypackages/personhog-proto/personhog/types/v1/person_pb2.pyiposthog/models/person/test/test_util_personhog.pyposthog/models/person/util.pyposthog/personhog_client/README.mdposthog/personhog_client/client.pyposthog/personhog_client/fake_client.pyposthog/personhog_client/proto/__init__.pyposthog/personhog_client/test_fake_client.pyproto/personhog/replica/v1/replica.protoproto/personhog/service/v1/service.protoproto/personhog/types/v1/person.protorust/personhog-replica/.sqlx/query-04c101b2f3e2a3facee9d23f4c490e9015479caf677106cb775e383df4547fb5.jsonrust/personhog-replica/.sqlx/query-2c35d517f08845a7529e9da5541bf265652dd7d332fee9769eac84d60e1b9535.jsonrust/personhog-replica/.sqlx/query-601660f4382614a149274fcfbc929bab32cd8a07aa7af8d4db579287fef40217.jsonrust/personhog-replica/.sqlx/query-75a975d984eea7dc6b1715b4e5c1670c9bd1b99fc096ab6752e4cd4474d00bec.jsonrust/personhog-replica/.sqlx/query-9a71b118fb8cef86d6fbf17da8a2056c7e3a9e22ab61f2ffc4e32ef6c9c53bdf.jsonrust/personhog-replica/.sqlx/query-9b70b04382c00916b39e98d05fa2b9d0d4e9f7ef6cdce20a439eb3dc6e246282.jsonrust/personhog-replica/.sqlx/query-a11441daea9b9c1d19d08ffcbb4abbe065aeab8968d44d5cea5df28f13377201.jsonrust/personhog-replica/.sqlx/query-aebd2a9bd2d9292a6778dfee92a71dc6bac93c7da61b434543b2ef7e3498a33b.jsonrust/personhog-replica/.sqlx/query-ba431aa18128ea6b1d9b51f3b634ce2e78928cafaeda736e55f2d6e36bd2ef89.jsonrust/personhog-replica/.sqlx/query-e2d052c562fe45f3eeba03324a03a93fc294ad8a1bb7ad4a649539e3e656759e.jsonrust/personhog-replica/.sqlx/query-e8c981d389a199ecd1184e4375f5ced60d7d292fdebdfef14f63b02882910f84.jsonrust/personhog-replica/src/service/mod.rsrust/personhog-replica/src/service/tests/mocks.rsrust/personhog-replica/src/service/tests/mod.rsrust/personhog-replica/src/storage/mod.rsrust/personhog-replica/src/storage/postgres/person.rsrust/personhog-replica/src/storage/traits/person.rsrust/personhog-replica/src/storage/types/person.rsrust/personhog-replica/tests/service_tests.rsrust/personhog-replica/tests/storage_tests.rsrust/personhog-router/src/proxy.rsrust/personhog-router/tests/common/mod.rsrust/property-defs-rs/tests/group_type_resolver.rs
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review.
EnsurePersonVersionFloors now refuses two spellings of one UUID, which passed the string check but locked the same row. EnsureDistinctIdVersionFloors now fails FAILED_PRECONDITION when a concurrent writer inserts a distinct id after the unlocked read, so the owner tombstone prepared for it rolls back instead of committing unused. Callers already retry that code. The fake client tests return proto messages instead of wide tuples.
This branch has not been deployed
1 participant
Problem
The ClickHouse person cleanup jobs cannot read or fix Postgres versions through personhog, so a revived person or distinct id can land below a ClickHouse tombstone and stay hidden.
Changes
No user-visible change. Nothing calls these RPCs yet.
GetPersonVersionHeadsandGetDistinctIdVersionHeadsread stored versions from a replica, tombstones included. Missing keys are left out.EnsurePersonVersionFloorsandEnsureDistinctIdVersionFloorsraise each tombstone belowmin_versiontomin_versionon the primary.LIVEwith its current version.TombstoneDistinctIdstombstones only orphaned distinct id rows: live rows whose person row does not exist. It setsis_deletedand adds 1 to the version.NOT_ORPHANED, unchanged. A person tombstone counts as an existing person row.FOR UPDATElock on the distinct id rows. Person ids are never reused, and the lock blocks repointing, so the check cannot go stale.FAILED_PRECONDITION.posthog/models/person/util.pybatch by the replica cap and retry a lost race up to 3 times.tombstone_distinct_ids_and_publishpublishes theperson_distinct_id2tombstone at exactly the version Postgres returned. It publishes nothing forABSENTorNOT_ORPHANED.KNOWN_METHODS, the Python client, the fake client, the proto re-exports, the Python and Node stubs, and the test mocks.Note
TombstoneDistinctIdsis not wired intopersondistinctids_without_person_cleanupyet. A follow-up PR does that.Deploy order
KNOWN_METHODS.Stale comments left alone
These comments predate this PR and are outside its scope:
rust/personhog-replica/src/storage/postgres/person.rs: "No ON CONFLICT … unique index, not a unique constraint", and "deletes lock PDI rows before person rows".nodejs/src/common/persons/repositories/postgres-person-repository.ts: the missing-uuid-index comment.How did you test this code?
All automated, run locally. No manual testing.
Test rationale:
SetPersonVersionFloor, which has no batch or orphan semantics.NOT_ORPHANEDwith the row unchanged, next to the orphan case.person_uuid.NOT_ORPHANED.Not run: the full backend suite. Only the personhog and person test paths ran.
👉 Stay up-to-date with PostHog coding conventions for a smoother review.
Release status
Automatic notifications
Docs update
None. The personhog client README lists the new RPCs.
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Agent: Cursor, Claude Opus 5.5
/adding-personhog-rpc,/reviewing-personhog-protocol,/writing-tests,/writing-dataclasses,/writing-code-comments,/writing-pr-descriptions.TombstoneDistinctIdsbecame orphan-only and gainedNOT_ORPHANED = 4. The ensure RPCs stopped raising live rows.ALREADY_TOMBSTONEDwhatever its owner, so a repeat call still republishes after a failed delivery.