Skip to content

fix(personhog): serialize splits of one person on its lifecycle mark - #116730

Closed
eli-r-ph wants to merge 3 commits into
masterfrom
eli/personhog-split-keeps-a-distinct-id
Closed

eli-r-ph wants to merge 3 commits into
masterfrom
eli/personhog-split-keeps-a-distinct-id

Conversation

@eli-r-ph

@eli-r-ph eli-r-ph commented Oct 14, 2026 •

Copy link
Copy Markdown
Contributor

Problem

Two splits of the same person, run at once, can each see an ID the other is about to move. Together they leave the person with no live distinct ID.
#116701 checks single requests in Python. Only the database transaction can stop the concurrent case.

Changes

  • SplitPerson claims the source person's lifecycle mark before any row lock. Ingestion merges and the tombstone RPC use the same mark.
  • Under the mark, it refuses with FailedPrecondition a split that would leave no live distinct ID.
  • Each lock wait is capped at 2s, and target persons are locked in id order, matching the tombstone RPC.
  • Refusals are logged and counted like other storage errors.

Note

A split now briefly blocks ingestion merges into the same person. The manual detach_distinct_id job takes no mark, so it can still race a split.

How did you test this code?

Test rationale: no storage test covered a split that empties a person, or a split racing another writer.

  • New storage tests:
    • A split of every ID is refused.
    • A split that waits behind a concurrent move of the same person, then refuses.
    • A mark held past the lock timeout rolls the split back.
    • A committed mark from another operation refuses the split.
  • Removing the mark claim, the check or the lock timeout fails these tests.
  • Ran cargo test, clippy, fmt and sqlx prepare --check locally.

Release status

  • No feature flag controls this change

Docs update

None.

🤖 Agent context

Autonomy: Human-driven (agent-assisted)

Agent: Claude Code, Claude Opus 5.5. Skills: /reviewing-personhog-protocol, /writing-tests, /writing-code-comments, /writing-pr-descriptions.

🤖 Generated with Claude Code

eli-r-ph and others added 2 commits October 13, 2026 21:19
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>
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>
@eli-r-ph eli-r-ph self-assigned this Oct 14, 2026
@trunk-io

trunk-io Bot commented Oct 14, 2026

Copy link
Copy Markdown

Merging to master in this repository is managed by Trunk.

  • To merge this pull request, check the box to the left or comment /trunk merge below.

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

@github-actions

github-actions Bot commented Oct 14, 2026 •

Copy link
Copy Markdown
Contributor

🤖 CI report

✅ Trunk lane — non-backend lane

This PR is assigned to the non-backend lane. It does not run backend Python tests and may merge in parallel with PRs in other lanes.

⚠️ Comment density — 5% of added code lines are comments (13 of 241)

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 9 96
rust/personhog-replica/tests/storage_tests.rs 4 145

This check does not block merging. It updates on every push and clears when the share drops.

eli-r-ph added a commit that referenced this pull request Oct 14, 2026
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>
@trunk-io

trunk-io Bot commented Oct 14, 2026 •

Copy link
Copy Markdown

Static Badge   Static Badge   Static Badge

View Full Report ↗︎ ⋅ Docs

@eli-r-ph
eli-r-ph requested a review from a team October 14, 2026 04:38
@eli-r-ph
eli-r-ph marked this pull request as ready for review October 14, 2026 04:38
@parameterai

parameterai Bot commented Oct 14, 2026 •

Copy link
Copy Markdown

Risk: No findings

This increment rewords comments, routes the split's FailedPrecondition error through the shared converter (same gRPC status, now also counted in a metric), and adds a lock-timeout test. No security-relevant behavior changed since the last review, which found no issues.

Sentinel reviewed 0e485da · Review settings

A refused split now goes through the shared error conversion, so it is
logged and counted in personhog_replica_storage_errors_total like other
refusals. A new test holds the source's mark past the lock timeout and
checks the split rolls back with nothing moved. Comments are shorter.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@eli-r-ph eli-r-ph closed this Oct 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant