fix(users): serialize email change and pin verification email recipient#73010
fix(users): serialize email change and pin verification email recipient#73010Piccirello wants to merge 4 commits 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 |
|
Reviews (1): Last reviewed commit: "fix(users): serialize email change and p..." | Re-trigger Greptile |
|
Note 🤖 stamphog reviewed 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.
Gate mechanics and policy version
Updated in place — this replaces 2 earlier stamphog review(s) on this PR. |
🤖 CI report
|
| 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.

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 frompending_emailat send time instead of the address the token was minted for.Changes
select_for_update) and mint the token inside the lock, so the token binds to one consistentpending_email.target_emailarg onsend_email_verification.create_token_and_send_email_verificationinto mint +send_verification_emailso 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 pinnedtarget_emaileven whenpending_emailhas drifted, and falls back topending_emailwhen no target is passed. Confirmed it goes red on the old behavior. Thetest_user.pychange-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
🤖 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 oftransaction.on_committo keep the friendly send-failure error and avoid on_commit not firing underTestCase.