Skip to content

fix(users): serialize email change and pin verification email recipient#73010

Open
Piccirello wants to merge 4 commits into
masterfrom
tom/email-change-verification-race
Open

fix(users): serialize email change and pin verification email recipient#73010
Piccirello wants to merge 4 commits into
masterfrom
tom/email-change-verification-race

Conversation

@Piccirello

Copy link
Copy Markdown
Member

Problem

The email-change flow sets pending_email, mints a verification token, and sends the email as three unlocked steps. Two concurrent changes on the same account can interleave so a token minted for one address has its verification email sent to another, because the recipient is re-read from pending_email at send time instead of the address the token was minted for.

Changes

  • Serialize the change under a row lock (select_for_update) and mint the token inside the lock, so the token binds to one consistent pending_email.
  • Send after the transaction commits, with the recipient pinned to the captured address via a new target_email arg on send_email_verification.
  • Split create_token_and_send_email_verification into mint + send_verification_email so the send stays synchronous and keeps its existing error handling.

No change to the normal single-request flow.

How did you test this code?

Added test_email_verification.py (SimpleTestCase, no DB): the recipient is the pinned target_email even when pending_email has drifted, and falls back to pending_email when no target is passed. Confirmed it goes red on the old behavior. The test_user.py change-then-verify tests need ClickHouse, which my local harness couldn't reach this session, so they run in CI. I traced them: the existing ones are unaffected (the send still routes through the same module path) and I updated one patch target for the split. ruff and mypy pass on the changed files.

Automatic notifications

  • Publish to changelog?
  • Alert Sales and Marketing teams?

🤖 Agent context

Autonomy: Human-driven (agent-assisted)

I (Claude) wrote this with Tom. Invoked /writing-tests. Considered pinning only the recipient, and rebinding the token to the in-memory instance; rejected both, since the recipient pin alone leaves a second interleaving open and rebinding touches a token primitive shared by 7 flows. Serializing the critical section closes the class by construction. Used a synchronous post-commit send instead of transaction.on_commit to keep the friendly send-failure error and avoid on_commit not firing under TestCase.

@Piccirello Piccirello added the stamphog Request AI approval (no full review) label Jul 22, 2026
@Piccirello Piccirello self-assigned this Jul 22, 2026
@trunk-io

trunk-io Bot commented Jul 22, 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

@assign-reviewers-posthog
assign-reviewers-posthog Bot requested a review from a team July 22, 2026 21:09
@greptile-apps

greptile-apps Bot commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

Reviews (1): Last reviewed commit: "fix(users): serialize email change and p..." | Re-trigger Greptile

@stamphog

stamphog Bot commented Jul 22, 2026

Copy link
Copy Markdown

Note

🤖 stamphog reviewed 0c293d1b855a5bb747e101bb4a50bd5c021b4e21 — verdict: ESCALATE

This is a genuine fix to a security-sensitive flow (email-change token binding/recipient), and my own read of the diff shows the fix (row lock + pinned target_email) correctly closes the described race and is covered by a new test. But it's authored by someone off the owning team with only moderate (not strong) familiarity, and there is no formal top-level review (APPROVED/COMMENTED) on the current head — only bot discussion comments and 👍 reactions, which are corroborating but not independent assurance for auth-sensitive changes.

  • Author wrote 0% of the modified lines and has 78 merged PRs in these paths (familiarity MODERATE).
  • 👍 on the PR from greptile-apps[bot], hex-security-app[bot].
  • Touches the email-verification/account-email-change flow (auth-sensitive surface) with zero formal top-level reviews on the current head.
  • Author (Piccirello) is not on either owning team (@PostHog/team-product-analytics, @PostHog/team-platform-features) and has only MODERATE familiarity, not STRONG — so ownership/familiarity doesn't substitute for review here.
  • Existing signals (ReviewHog 'nothing to raise' comment, Greptile comment, 👍 from greptile-apps[bot] and hex-security-app[bot]) are positive but are reactions/comments, not a submitted APPROVED or COMMENTED review, so they don't meet the bar for independent assurance in risky territory.
Gate mechanics and policy version
Gate Result
prerequisites all clear
deny-list no deny categories matched
size 36L, 3F substantive, 15006L/18F incl. docs/generated/snapshots — within ceiling
tier T1-agent / T1d-complex (15006L, 18F, two-areas, fix)
stamphog 2.0.0b3 .stamphog/policy.yml @ 5f13a28 · reviewed head 0c293d1

Updated in place — this replaces 2 earlier stamphog review(s) on this PR.

@stamphog stamphog Bot removed the stamphog Request AI approval (no full review) label Jul 22, 2026
@github-actions

github-actions Bot commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

🤖 CI report

Playwright — all passed

All tests passed.

View test results →

⚠️ Backend snapshots — 11 updated (11 modified, 0 added, 0 deleted)

Query snapshots: Backend query snapshots updated

Changes: 11 snapshots (11 modified, 0 added, 0 deleted)

What this means:

  • Query snapshots have been automatically updated to match current output
  • These changes reflect modifications to database queries or schema

Next steps:

  • Review the query changes to ensure they're intentional
  • If unexpected, investigate what caused the query to change

Review snapshot changes →

⚠️ Backend coverage — 96.0% of changed backend lines covered — 1 uncovered

🧪 Backend test coverage

Patch coverage — changed backend lines (products + core): ███████████████████░ 96.0% (25 / 26)

File Patch Uncovered changed lines
posthog/tasks/email.py 0.0% 488

🤖 Agents: add a test covering the lines above, or note why under "How did you test this code?". Machine-readable gap list: the patch-coverage artifact on this run (gh run download 29966380715 -n patch-coverage), or the coverage-data block at the end of this comment.

Per-product line coverage (touched products)
Product Coverage Lines
platform_features ██░░░░░░░░░░░░░░░░░░ 12.1% 7 / 58
batch_exports ████████░░░░░░░░░░░░ 39.6% 8,414 / 21,250
demo ███████████░░░░░░░░░ 56.2% 1,497 / 2,663
warehouse_sources_queue ████████████░░░░░░░░ 59.2% 148 / 250
tasks ██████████████░░░░░░ 69.1% 29,366 / 42,488
data_tools ██████████████░░░░░░ 70.0% 63 / 90
ai_gateway ███████████████░░░░░ 75.0% 9 / 12
signals ████████████████░░░░ 79.1% 20,388 / 25,788
cdp ████████████████░░░░ 80.7% 3,117 / 3,863
data_modeling █████████████████░░░ 82.8% 5,529 / 6,675
wizard █████████████████░░░ 84.8% 935 / 1,102
notebooks █████████████████░░░ 85.3% 7,266 / 8,522
agent_platform █████████████████░░░ 86.4% 3,807 / 4,405
actions █████████████████░░░ 86.6% 717 / 828
cohorts ██████████████████░░ 87.8% 4,488 / 5,114
product_tours ██████████████████░░ 87.9% 1,303 / 1,482
exports ██████████████████░░ 88.4% 6,949 / 7,861
data_warehouse ██████████████████░░ 88.8% 11,867 / 13,358
business_knowledge ██████████████████░░ 89.0% 4,391 / 4,936
engineering_analytics ██████████████████░░ 89.2% 5,641 / 6,323
conversations ██████████████████░░ 89.3% 16,820 / 18,834
dashboards ██████████████████░░ 89.4% 5,983 / 6,693
visual_review ██████████████████░░ 89.4% 5,818 / 6,505
error_tracking ██████████████████░░ 89.7% 10,174 / 11,342
alerts ██████████████████░░ 89.9% 4,054 / 4,508
early_access_features ██████████████████░░ 90.1% 1,031 / 1,144
mcp_analytics ██████████████████░░ 90.1% 2,763 / 3,065
streamlit_apps ██████████████████░░ 90.4% 2,501 / 2,767
links ██████████████████░░ 90.6% 183 / 202
slack_app ██████████████████░░ 90.7% 9,025 / 9,948
marketing_analytics ██████████████████░░ 91.0% 11,792 / 12,964
stamphog ██████████████████░░ 91.1% 4,056 / 4,450
product_analytics ██████████████████░░ 91.3% 5,811 / 6,362
mcp_store ██████████████████░░ 91.9% 4,257 / 4,634
notifications ███████████████████░ 92.6% 1,011 / 1,092
ai_observability ███████████████████░ 92.7% 14,944 / 16,117
surveys ███████████████████░ 93.0% 5,730 / 6,163
web_analytics ███████████████████░ 93.0% 14,147 / 15,206
managed_migrations ███████████████████░ 93.1% 1,424 / 1,530
posthog_ai ███████████████████░ 93.2% 1,326 / 1,422
approvals ███████████████████░ 93.3% 3,437 / 3,682
reminders ███████████████████░ 93.4% 468 / 501
workflows ███████████████████░ 93.5% 6,337 / 6,775
legal_documents ███████████████████░ 94.1% 1,568 / 1,667
endpoints ███████████████████░ 94.1% 8,640 / 9,177
messaging ███████████████████░ 94.3% 2,682 / 2,845
revenue_analytics ███████████████████░ 94.5% 3,598 / 3,809
tracing ███████████████████░ 94.5% 2,670 / 2,826
skills ███████████████████░ 94.5% 2,893 / 3,061
review_hog ███████████████████░ 94.6% 6,808 / 7,193
logs ███████████████████░ 95.4% 9,935 / 10,416
experiments ███████████████████░ 95.7% 24,882 / 25,996
growth ███████████████████░ 96.1% 3,245 / 3,376
replay_vision ███████████████████░ 96.2% 14,973 / 15,572
annotations ███████████████████░ 96.2% 732 / 761
feature_flags ███████████████████░ 96.5% 17,110 / 17,734
user_interviews ███████████████████░ 96.5% 2,638 / 2,734
access_control ███████████████████░ 96.8% 851 / 879
warehouse_sources ███████████████████░ 97.1% 327,769 / 337,448
customer_analytics ███████████████████░ 97.3% 8,380 / 8,610
data_catalog ███████████████████░ 97.4% 2,367 / 2,429
analytics_platform ████████████████████ 98.0% 2,153 / 2,197
metrics ████████████████████ 98.2% 2,491 / 2,536
pulse ████████████████████ 98.4% 2,017 / 2,049
live_debugger ████████████████████ 99.2% 613 / 618
field_notes ████████████████████ 99.4% 158 / 159

Report-only. Patch coverage = changed backend lines covered vs origin/master. Sorted lowest first.
Known gaps: lines covered only by Temporal tests show as uncovered; core line numbers may drift if master changed the same file.

@Piccirello Piccirello added the reviewhog ($$$) Reviews pull requests before humans do label Jul 22, 2026
@posthog

posthog Bot commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

🦔 ReviewHog reviewed this pull request

Nothing worth raising this time, so here's a calming gif instead:

Someone relaxing in a sunny garden

@Piccirello Piccirello added the stamphog Request AI approval (no full review) label Jul 22, 2026
@stamphog stamphog Bot removed the stamphog Request AI approval (no full review) label Jul 22, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

reviewhog ($$$) Reviews pull requests before humans do

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant