[Silthus]

Note

Merged master in e4095eb6a8a to resolve conflicts with the email domain wizard stack (#120402 and later). The conflict resolutions are the commit range to review: 25af2dbab43..e4095eb6a8a.

• status() keeps master's read-only shape. Only verify(), the background sweep and the advisor path save verification.
• verify() sends email domain verified through master's _report_email_domain_verified on the first verification. The background sweep sends no event.
• The SES provider keeps master's status read config and adds this PR's paced background client.
• Review fix 287950a29f1: email domain verified fires once per domain across MAIL FROM subdomains, as on master. The status check matches a MAIL FROM domain that differs in case or by a trailing dot.

Problem

Workflows users can finish DNS setup and still see their sender as unverified until they open its verification UI. After this PR, a new sender shows as verified within about a minute of SES accepting its DNS.

Impact

Pays into Grow Workflows through Workflows activation rate. A sender that verifies without the user on the page removes a return visit before the first send, and the upcoming "send once verified" change builds on this sweep so a waiting workflow can go live by itself. Expected move on its own: small, up.

Warning

🟡 Merge risk: medium · one-way door for saved verification flags and sender defaults

• The sweep updates verification across projects. A Redis lease and hard task limit prevent overlapping checks.
• The sweep shares SES read throttles with the verify button. Batched, paced reads keep half of that rate free.
• Matching senders are row-locked while they are marked verified, so a concurrent edit cannot make an unverified sender the default. SES setup writes stay on the explicit verification path.
• Sender config gains a verification_requested_at timestamp, visible in the integrations API. Creating a sender, a pending verify click and a resolved advisor finding set it.
• Reverting stops the checks; senders already marked verified retain that saved state, and the timestamp stays in config unused.

Changes

Kind	Files	Lines
Production	9	+458 −83
Tests	5	+392 −6
Total	14	+850 −89

• Background checks update the verified flag, so Channels and MCP reads pick it up without opening the setup UI.
• Senders get checked every minute for an hour after verification is requested, then every 5 minutes up to 7 days. Creating a sender, a pending verify click and a resolved advisor finding request verification. Senders without a request time fall back to created_at. Each tier has its own lock and cursor.
• A resolved SES VDM advisor finding for a domain, such as "DKIM record was not found", arrives through the existing SES events webhook. It requests verification for the pending senders on that domain, so the next 1-minute sweep checks them whatever their age. A failed check is retried like any other pending sender.
• Background checks read SES and DNS. They require successful domain verification, DKIM, matching MAIL FROM, DMARC, and the project's SES association.
• One SES status read covers up to 100 senders. Each kind of read, including tenant reads, runs at most once every two seconds, half the SES limit, so the verify button keeps headroom. Advisor events only write a timestamp, so a burst of events adds no parallel SES readers. User requests are not paced.
• The DNS and tenant checks run only for senders that SES already reports as ready.
• The sweeps skip senders whose last verification request is older than 7 days. SES fails a domain after 72 hours without DNS, and only the verify button restarts it.
• Background SES calls have connect and read timeouts and two attempts in total. A step starts only when its worst case, including retry backoff and pacing, fits the task's budget, so the task ends before its hard limit.
• MAIL FROM matching ignores case and a trailing dot.
• The verify button marks only senders on the same domain and MAIL FROM subdomain as verified. SES holds one MAIL FROM per domain, so other subdomains stay pending.
• Verification updates matching senders and reloads sending workers. It writes only the verified flag, preserving other configuration fields.
• Verified senders become the project's default when none exists, preserving #114682. The matching senders are locked while the flag and default change, so an edited sender cannot become the default.
• Provider failures leave the sender pending and allow the sweep to continue. Verified senders leave subsequent sweeps.
• Each sweep resumes a bounded pass over pending senders. New arrivals join the next pass, allowing older pending senders to retry.
• Mechanical changes register the task and update SES fixtures with their MAIL FROM domain.

Before:

flowchart LR
    UI[Open verification UI] --> Check[Check SES and DNS]
    Check --> Saved[Save verified flag]
    Saved --> Reads[UI and MCP reads]
    classDef phYellow fill:#f9bd2b,stroke:#f9bd2b,color:#000;
    classDef phRed fill:#f54e00,stroke:#f54e00,color:#fff;
    classDef phGray fill:#e5e7eb,stroke:#c7ccd1,color:#000;
    class UI,Reads phYellow;
    class Check phRed;
    class Saved phGray;

Loading

After:

flowchart LR
    Beat[Every minute, under 1 hour old] --> Guard[Acquire tier lease]
    Beat5[Every 5 minutes, 1 hour to 7 days old] --> Guard
    Guard --> Pending[Resume pass over the tier]
    Advisor[SES advisor finding resolved] --> Queue[Mark pending senders requested]
    Queue --> Guard
    Pending --> Batch[Read SES status for 100 senders]
    Batch --> Check[Check DNS and tenant for ready senders]
    Check --> Saved[Update matching verified flags]
    Saved --> Reads[UI and MCP reads]
    Saved --> Workers[Reload sending workers]
    classDef phYellow fill:#f9bd2b,stroke:#f9bd2b,color:#000;
    classDef phBlue fill:#1d4aff,stroke:#1d4aff,color:#fff;
    classDef phRed fill:#f54e00,stroke:#f54e00,color:#fff;
    classDef phGray fill:#e5e7eb,stroke:#c7ccd1,color:#000;
    class Beat,Beat5,Advisor,Reads phYellow;
    class Guard,Pending,Queue,Batch,Workers phBlue;
    class Check phRed;
    class Saved phGray;

Loading

Warning

The advisor trigger needs AWS-side setup before it fires. The SES EventBridge rule that feeds the SES events webhook must also match Advisor Recommendation Status Resolved, and VDM must be on for the account. Until then, the two sweeps carry the load.

Note

Known gaps, not fixed here:

• Saving a sender's whole config during a sweep can overwrite a background "verified". The next sweep restores it within 5 minutes.
• Each tier paces its own SES reads. Both tiers reading at once could use the whole SES read rate for a moment.

How did you test this code?

Test seams: the background Celery task, integration reads used by API/MCP, and the project API's default sender. Tests fake AWS, DNS, and elapsed time.

Test rationale: The closest existing test, test_email_verify_updates_integration, exercises explicit verification and mocks the facade. It cannot catch a missing background refresh. The task/API cases cover verification, incomplete checks, concurrent edits, failure recovery, overlapping sweeps, and retry fairness. Extended cases preserve default selection after merging master and reject an edited sender when its sibling verifies.

The review round added cases for batched status reads, stale senders, MAIL FROM case, a budget run out by slow SES calls, and the verify button skipping senders on another MAIL FROM subdomain. Each case fails when its fix is reverted.

The tier round added age-boundary cases for both sweeps, a per-domain check that ignores age and domain case, and a webhook case where an identity named team-5.example.com reaches the sender check, not the tenant sync. Each fails when its rule is reverted.

Master refresh on 25af2dba: the touched-area suites (sender sweep, SES provider, SES integration, SES events webhook, TestEmailIntegration, scheduler, task registration, team workflows config) passed locally, 112 tests. test_ses_provider.py runs on its own, away from the known logging-setup clash below. One drift fix: the sweep tests no longer clear the per-process Team.workflows_config cache, because #118777 removed it. hogli product:lint workflows and hogli ci:preflight --strict passed. Repository-wide mypy was not rerun after the refresh.

Local red/green proof and verification commands (before the master refresh)

Before the implementation, SES and DNS reported success but the integration API returned false:

assert response.json()["config"]["verified"] is True
E assert False is True

The read-only and concurrent-edit cases also failed before their fixes. The task now updates the API status without calling SES setup methods.

The stronger overlap test fails when a temporary test plugin bypasses the lock. The budget test fails without a fixed pass boundary:

Overlap without lock: assert 2 == 1
Retry during new arrivals before fix: assert [False, True] == [True, True]
After fixes: 56 passed
Review round, each fix reverted on its own:
  no 7-day window       -> stale_sender fails
  exact MAIL FROM match -> mail_from_differs_in_case fails
  one read per sender   -> both interrupted-sweep cases fail
  no per-step budget    -> budget_exhausted fails
  all-subdomain marking -> test_email_verify_updates_all_other_integrations_with_same_domain fails
Review round suites: 458 + 40 passed
Tier round, each rule reverted on its own:
  1-hour tier covers 7 days    -> settled_sender_not_in_minute_sweep fails
  exact domain match on event  -> domain_check_verifies_pending_senders_on_that_domain_of_any_age fails
  no advisor dispatch          -> resolved_advisor_finding_checks_the_senders_on_that_identity fails
Tier round suites: 478 + 40 passed
QA swarm round 3 fixes, each reverted on its own:
  domain removed before its check -> advisor_domain_stays_queued_until_its_check_finishes fails
  unpaced tenant reads            -> tenant_reads_for_ready_senders_are_spaced_apart fails
Retry probe against a local HTTP server returning Throttling:
  old adaptive max_attempts=2: 3 attempts
  new standard total_max_attempts=2: 2 attempts, under 1s backoff
Round 3 suites: 503 + 40 passed. No automated test covers the row lock, because it needs two real concurrent transactions.
QA swarm round 4 fixes, each reverted on its own:
  verify click keeps old request time -> test_email_verify_returns_ses_result fails
  tiers ignore the request time       -> 4 tier and advisor cases fail
  user requests paced                 -> test_verify_email_domain_success fails
Final suites on ed71c30b: 505 + 40 passed, and 550 passed across the email model, domain connect and full workflows backend tests.
Explicit verification, scheduler, registration and project config: 25 passed
Backend compilation: passed
hogli build: no generation pipelines triggered by the 11 PR files
Repository-wide mypy: no issues found in 23384 source files

.codex/with-flox hogli test products/workflows/backend/test/test_email_sender_verification.py products/workflows/backend/test/test_ses_provider.py products/workflows/backend/test/test_email_integration_ses.py
.codex/with-flox hogli test posthog/api/test/test_integration.py::TestEmailIntegration posthog/tasks/test/test_scheduled.py posthog/tasks/test/test_task_registration.py products/workflows/backend/test/test_team_workflows_config.py
.codex/with-flox python -m compileall -q posthog/models/integration/email.py products/workflows/backend
.codex/with-flox hogli build
.codex/with-flox hogli product:lint workflows
.codex/with-flox uv run mypy --cache-fine-grained .
.codex/with-flox hogli ci:preflight --strict

No frontend code or layout changes. Not checked: real SES, public DNS propagation, production mail delivery, or a real VDM advisor event. The advisor event shape follows the SES EventBridge docs. Three TestGetIdentityIspMetrics cases fail when they share a process with posthog/tasks/test/test_task_registration.py, which re-runs Django's logging setup and disables existing loggers. This happens on master too and gets its own fix PR. These tests do not read event properties.

Release status

• No feature flag controls this change

Docs update

None. No existing document under docs/ covers sender verification.

🤖 Agent context

Autonomy: Human-driven (agent-assisted)

Question	Answer	Who	Where
Refresh method onto current master	One squashed commit on master instead of replaying the old merge commit and its 8 commits	🤖 Agent	ticket
Per-process team config gap	Dropped from known gaps: #118777 loads team configs per team instance	🤖 Agent	ticket
Scope of the refresh	Same goal and size; the task-module import list conflicted, and the sweep tests dropped a cache clear that #118777 removed	🤖 Agent	ticket

Involvement: no grilling rounds or prototypes in this refresh; earlier rounds ran two QA swarms and three single reviews; proof is the local test output above.

Agent: T3 Code / Codex, gpt-6.1-sol. Review-round fixes: T3 Code / Claude Code, claude-opus-5-5.

Skills: tdd, writing-tests, hogli, running-ci-preflight, qa-swarm, pr, writing-pr-descriptions, merge-risk, writing-voice, unslop, principle-prove-it-works.

Two QA rounds fixed overlapping sweeps, ordered failure mocks, an insensitive overlap test, and retry starvation during continuous arrivals. All findings are fixed or rejected.

The deferred-import comment request was rejected under the user's explicit no-code-comments instruction. GPT-6.1 Sol reviewed correctness and risk; GPT-6 Astra covered fresh eyes and design after Opus returned 429. CodeRabbit CLI skipped because it is signed out.

The master refresh used a merge commit. A read-only Claude Opus 5.5 review found that a changed sender could become the default when a sibling verified. The fix requires the checked sender to match the verification update; a failing task/API case proves the guard. The same guard excludes a sender deleted during the provider check.

A QA swarm (GPT-6.1 Sol: correctness, risk, design; Claude Opus 5.5: fresh eyes) on the tier round found a default-sender race, a retry count of three, unpaced tenant reads, parallel advisor readers, and dropped unfinished domain checks. All are fixed. The narrow fix round then found that the Redis advisor queue added in round 3 could starve, drop events, and drop failed checks. The queue was replaced by a per-sender verification_requested_at timestamp that the tier sweeps already process. That also covers re-added and re-verified senders. Hardening the queue with event generations was the alternative.

A critical Claude Opus 5.5 review after the merge flagged unbatched SES reads, an unbounded sweep, unbounded SES calls, and case-sensitive MAIL FROM matching. All four are fixed. The read-only check moved into the workflows service, which removed the duplicate SES path on the integration model and the one-function facade module. Three lower-risk findings are listed as known gaps. soft_time_limit was rejected: Celery raises it as an Exception, which the per-sender guard would swallow. Client timeouts and the per-step budget bound the run instead.

SES sends no EventBridge event when a domain identity, DKIM or MAIL FROM becomes verified. A resolved VDM advisor finding is the closest signal, so it only triggers a fresh check. The sweeps stay as the source of truth.

GitHub CI requires maintainer approval; CI and patch coverage remain unchecked. GitHub denied the requested assignee, so the PR remains unassigned.

A Claude Opus 5.5 (claude-opus-5-5) refresh moved the PR onto current master as one commit. Master had added a default reply-to on senders (#115898), turned off SES feedback forwarding (#113335), and registered new workflows task modules. All three merged without semantic conflicts. The per-team-instance config load from #118777 needed one test setup change. Skills: modernize-pr, pr-description.

The duplicate search found #110516 (on-demand domain status reads) and #113059 (verification access scope). Both closed without merging, and no merged PR adds a background sweep. Fixtures contain invented domains, addresses and tokens.

[trunk-io]

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

[coderabbitai]

Walkthrough

The change adds SES verification refresh for pending email senders. Verification results update only integrations that match the relevant team, domain, provider, and mail-from subdomain. A Celery task runs the refresh every five minutes. Tests cover verification conditions and recovery after a provider failure.

Priority: Low

Merge Risk: Low · up to 21e71

An interrupted verification may leave one sender pending longer than necessary. The sweep is bounded and prevents overlapping runs, so this is a limited risk that can be fixed or explicitly accepted before merging.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 21e71

Automatic sender activation remains gated by domain verification and project association checks, with tighter protection against concurrent configuration edits. Saved activations survive rollback. Production scheduling and recovery guarantees still need confirmation, but no introduced security bypass was established.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

• observed — The sweep's aggregate reach spans all pending SES email integrations in the queried database. Each successful result can promote multiple matching sender records, but its write scope is restricted to the source team and verified domain/provider/subdomain configuration. The resulting flag is an email-sending eligibility control consumed by existing workers.

Trust Boundaries and Controls

• observed — Stored sender configuration supplies the domain and MAIL FROM inputs to privileged SES lookup and DNS resolution. The expected tenant derives from the integration's team ID. Database predicates recheck relevant configuration before promotion, and the field-level update preserves unrelated fields. Tests cover a concurrent MAIL FROM edit that prevents stale promotion.
• observed — The unchanged worker send path rejects wrong-team integration references, requires email kind and verified state, and restricts custom From addresses to the integration's domain. Automatic promotion does not remove these downstream ownership and sender-domain controls.

Resilience and Maintainability Implications

• inferred — The intended scheduled path combines a global nonblocking lease, a work budget, and a shorter task hard limit to contain overlap and provider delays. The lease is neither renewed nor used to fence checkpoint writes, so that ownership guarantee depends on terminating the invocation before lease expiry. No alternate production caller was established.

Hardening Proposals

• proposed — For stronger recovery guarantees, checkpoint after a completed or caught verification attempt and provide retryable reconciliation of persisted verification state with worker caches. An operational rollback procedure should distinguish stopping checks from deliberately revoking saved activations. These are recovery improvements, not verified security findings.

Pre-merge checks: 1

Check name	Status	Explanation
Description check		The pull request description is complete and follows the required template. It explains the problem, user impact, implementation changes, architecture, testing rationale and evidence, release status, …

Full details: Description check

Explanation

The pull request description is complete and follows the required template. It explains the problem, user impact, implementation changes, architecture, testing rationale and evidence, release status, documentation status, agent context, known gaps, and merge risks.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

• Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

🧹 Nitpick comments (1)

products/workflows/backend/services/email_sender_verification.py (1)

11-29: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy lift

Bound the sweep cost: it has no time limit, no overlap guard, and no cap on abandoned senders.

The query selects every SES sender with verified=False. Senders that never complete DNS setup stay in this set permanently. Each unique (team, domain, subdomain) key costs several SES calls and a DMARC lookup with a 5-second lifetime. It also builds a new SESProvider(), which creates five boto3 clients. The sweep is sequential and the task expires after 300 seconds. As the pending set grows, one run can take longer than the 5-minute schedule. Runs then overlap on the LONG_RUNNING queue and repeat the same SES calls.

Suggested fixes:

• Add a soft_time_limit and a single-flight lock (for example a Redis lock) to the task.
• Skip or back off senders that have been pending for a long time (for example older than N days).
• Reuse one SESProvider per sweep instead of one per sender.
  [medium_effort_placeholder]

---

ℹ️ Review info

⚙️ Run configuration

• Configuration used: Repository: PostHog/posthog/.coderabbit.yaml
• Review profile: QUIET
• Plan: Enterprise
• Run ID: baef3df4-319e-45c2-a144-aac79d1b6bf8

📥 Commits

Reviewing files that changed from the base of the PR and between 5d74efc and cc5e0a2.

📒 Files selected for processing (11)

• posthog/models/integration/email.py
• posthog/tasks/scheduled.py
• products/workflows/backend/facade/email.py
• products/workflows/backend/facade/tasks.py
• products/workflows/backend/providers/ses.py
• products/workflows/backend/services/email_sender_verification.py
• products/workflows/backend/tasks/__init__.py
• products/workflows/backend/tasks/email_sender_verification.py
• products/workflows/backend/test/test_email_integration_ses.py
• products/workflows/backend/test/test_email_sender_verification.py
• products/workflows/backend/test/test_ses_provider.py

Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 3 remain after this review.

[Silthus]

Note

🤖 GPT-6.1 Sol responding on behalf of Michael

@coderabbitai Fixed in ea4acf1 and 21e71d6. The sweep now uses a token-owned Redis lease, a four-minute work budget, a 270-second hard task limit, and saved progress over a bounded sender pass. Older pending senders remain eligible for retries even while new senders arrive; the task/API tests cover overlap and retry fairness.

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail
git show --stat --oneline ea4acf1b
git show --stat --oneline 21e71d6d
ast-grep outline products/workflows/backend/services/email_sender_verification.py --items all
ast-grep outline products/workflows/backend/tasks/email_sender_verification.py --items all
ast-grep outline products/workflows/backend/test/test_email_sender_verification.py --items all
sed -n '1,300p' products/workflows/backend/services/email_sender_verification.py
sed -n '1,220p' products/workflows/backend/tasks/email_sender_verification.py
rg -n -C 5 'lease|overlap|fair|retry|progress|budget|time_limit|lock|cursor|bounded' products/workflows/backend/test/test_email_sender_verification.py

Length of output: 7475

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail
sed -n '29,270p' products/workflows/backend/test/test_email_sender_verification.py

Length of output: 9094

---

@Silthus The inspected code addresses overlap and retry fairness. The tests cover reentrant sweeps, budget exhaustion, and retrying an older sender after a new sender arrives. I did not run the tests.

You are interacting with an AI system.

[coderabbitai]

Note

Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.

🟡 Other comments (1)

products/workflows/backend/services/email_sender_verification.py (1)

43-43: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Advance CURSOR_KEY after the refresh, not before it.

Line 43 writes integration.id to CURSOR_KEY before refresh_verification() runs. The task has time_limit=270. If Celery hard-kills the task during an SES or DNS call, the cursor already points at the unchecked integration. The next run then filters id__gt=cursor and skips that integration until the next full pass. Write the cursor after the refresh. The budget return at Line 42 then keeps resuming at the unprocessed row, and a killed refresh is retried.

Proposed fix

             if monotonic() - started_at >= EmailSenderVerification.WORK_BUDGET_SECONDS:
                 return
-            redis.set(EmailSenderVerification.CURSOR_KEY, integration.id)
             domain = integration.config.get("domain")
             mail_from_subdomain = integration.config.get("mail_from_subdomain", "feedback")
             if not domain:
+                redis.set(EmailSenderVerification.CURSOR_KEY, integration.id)
                 continue
             key = (integration.team_id, domain, mail_from_subdomain)
             if key in checked:
+                redis.set(EmailSenderVerification.CURSOR_KEY, integration.id)
                 continue
             checked.add(key)
             try:
                 EmailIntegration(integration).refresh_verification()
             except Exception:
                 logger.exception(...)
+            redis.set(EmailSenderVerification.CURSOR_KEY, integration.id)

---

ℹ️ Review info

⚙️ Run configuration

• Configuration used: Repository: PostHog/posthog/.coderabbit.yaml
• Review profile: QUIET
• Plan: Enterprise
• Run ID: 37d23168-5101-4170-9d8a-11d62d2ddd0f

📥 Commits

Reviewing files that changed from the base of the PR and between cc5e0a2 and 21e71d6.

📒 Files selected for processing (3)

• products/workflows/backend/services/email_sender_verification.py
• products/workflows/backend/tasks/email_sender_verification.py
• products/workflows/backend/test/test_email_sender_verification.py

Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 3 remain after this review.

[parameterai]

Risk: No findings

The delta since the last review splits a team-scoped sender query into a reusable _domain_senders() helper, broadens the "domain already verified" analytics-event gate to all MAIL FROM subdomains on a domain, and makes the SES MAIL FROM status match DNS-equivalent (case/trailing-dot) domains. All verified-flag writes still go through the exact-match, row-locked, team-scoped path, and the relaxed comparison only affects status reporting, so no new security risk was found.

_{Sentinel reviewed 287950a · Review settings}

[scheduled-actions-posthog]

This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, please remove the stale label – otherwise this will be closed in another week. If you want to permanently keep it open, use the waiting label.

[Silthus]

Moved to #127146, which runs on a PostHog branch so CI can run.