Repository navigation
Conversation
posthog-python Compliance ReportDate: 2026-10-26T16:33:39.421972+00:00 ✅ All Tests Passed!121/121 tests passed Capture_V1 Tests✅ 95/95 tests passed View Details
Capture_Ai Tests✅ 5/5 tests passed View Details
Feature_Flags Tests✅ 17/17 tests passed View Details
Feature_Flags_Local_Evaluation Tests✅ 4/4 tests passed View Details
|
|
@PostHog review |
|
@mayteio PostHog Review is reviewing this pull request. |
![posthog[bot]](https://avatars.githubusercontent.com/in/1203049?s=60&v=4){kind=small}
| ``` | ||
| """ | ||
| if self._emails is None: | ||
| self._emails = Emails(self.secret_key, host=self.host) |
There was a problem hiding this comment.
P1 · Require an explicit app host for custom ingestion hosts
When Client.host points to an ingestion reverse proxy, emails reuses that address and sends the project secret key in its Authorization header. This exposes privileged credentials to a proxy configured only for public capture traffic; AsyncClient.emails does the same. Derive known Cloud app hosts and require an explicit app host for other addresses.
There was a problem hiding this comment.
Fixed in 5a81665. Emails send only to app_host, or to the PostHog Cloud app host derived from host. Any other host raises before a request, so the secret key never reaches an ingestion proxy. The code is now missing_app_host (d6d07a4), as PostHog/posthog#120921 defines.
| @property | ||
| def emails(self) -> Emails: |
There was a problem hiding this comment.
P2 · Forward module-level email sends to the default client
With the SDK's module-level setup (import posthog), the advertised posthog.emails.send(...) raises AttributeError: importing the new submodule binds posthog.emails to a module without send. Adding Client.emails only supports explicitly constructed clients. Add a module-level forwarding function that uses the configured default client.
There was a problem hiding this comment.
Resolved in 275a64d by not advertising it: every example and the release note now use client.emails.send(). There is deliberately no module-level forwarder, because the default module client is usually set up without a secret_key, so posthog.emails stays the module that holds Emails, AsyncEmails and EmailError.
| if self._emails is None: | ||
| self._emails = AsyncEmails(self.secret_key, host=self.host) | ||
| self._emails.disabled = self.disabled | ||
| return self._emails |
There was a problem hiding this comment.
P2 · Close owned email transports during client shutdown
After client.emails.send(), leaving async with AsyncPosthog(...) never closes the separate email HTTP client: shutdown() closes only _http_client. Repeated client lifetimes retain email connections; the sync wrapper also omits cleanup. Wire both email transports into parent shutdown, following the lifecycle contract required by AGENTS.md:20.
| self._session: Optional[requests.Session] = None | ||
| self._session_lock = threading.Lock() |
There was a problem hiding this comment.
P2 · Reinitialize email sessions and locks after a fork
When a process forks after creating Emails, children inherit its HTTP pool and _session_lock. A lock held by another parent thread stays locked forever in the child, so _get_session() hangs before the request timeout applies. Client._reinit_after_fork() does not reset email state; replace the lock and session in a child fork hook.
There was a problem hiding this comment.
Fixed in 5a81665. Client._reinit_after_fork() drops _emails, so a forked child builds a new Emails with its own session and lock instead of using the parent ones.
| """ | ||
| if self._emails is None: | ||
| self._emails = Emails(self.secret_key, host=self.host) | ||
| self._emails.disabled = self.disabled |
There was a problem hiding this comment.
P2 · Check the parent client's disabled state when sending
When an application stores emails = client.emails and later sets client.disabled = True, emails.send(...) still sends email because this property only copies the disabled state when accessed. AsyncClient.emails has the same bypass, violating the disabled-client contract for retained helpers. Read the parent's current disabled state inside each send.
There was a problem hiding this comment.
Fixed in 5a81665. client.emails reads the parent disabled flag on every send through _parent_disabled, so a retained handle stops sending as soon as the client is disabled.
| backoff = min(_MAX_RETRY_DELAY, 2.0**attempt) * random.uniform(0.8, 1.2) | ||
| return max(backoff, error.retry_after or 0.0) |
There was a problem hiding this comment.
P3 · Apply the retry delay ceiling after jitter
With max_retries=6 or higher, a sustained network failure reaches attempt 5, where _retry_delay() caps the backoff at 30 seconds and then multiplies it by jitter. This produces waits up to 36 seconds, exceeding _MAX_RETRY_DELAY and extending blocking sends beyond the intended ceiling; cap the backoff after applying jitter.
There was a problem hiding this comment.
Fixed on this branch. _retry_delay computes min(_MAX_RETRY_DELAY, 2.0**attempt * random.uniform(0.8, 1.2)), so the 30 second ceiling applies after the jitter.
marandaneto
left a comment
marandaneto
left a comment
There was a problem hiding this comment.
Automated advisory code review.
|
|
||
| if self._emails is not None: | ||
| try: | ||
| await self._emails.aclose() |
There was a problem hiding this comment.
blocking: Drain active email sends before closing their transport — AsyncEmails.send() is not registered in the parent's _inflight_operations, so shutdown reaches this close while an email call is still running. If an already-started request then returns a retryable 503, the send loop retries using its cached, now-closed httpx client and raises an uncaught RuntimeError instead of completing its bounded retry or returning EmailError; shutdown has already reported completion. Wait for admitted email operations before disposing their transport, as existing immediate-capture operations do. Reproduction: reproduced — python -m pytest posthog/test/test_review_candidates.py::test_async_shutdown_drains_inflight_email_before_transport_close --timeout=30 -q fails on reviewed head with RuntimeError: Cannot send a request, as the client has been closed, while the same probe passes at d2827abe107b46c1e71eb72d86e93e2ca96bda0b.
There was a problem hiding this comment.
Fixed in d20805a. A send through client.emails on AsyncClient now registers as an in-flight operation, the same way capture_immediate does, so shutdown() waits for it (retries included) before it closes the email transport. test_async_shutdown_waits_for_a_send_in_flight_and_then_refuses_sends covers the 503-then-202 case during shutdown.
| """ | ||
| if self._emails is None: | ||
| self._emails = _client_emails( | ||
| Emails, self.secret_key, self.host, lambda: self.disabled |
There was a problem hiding this comment.
blocking: Honor terminal parent shutdown before admitting email sends — The namespace checks only self.disabled; permanently shutting down the parent does not set that flag. Consequently a retained client.emails handle, or first namespace access after shutdown, can create a fresh transport and successfully send email after client.shutdown() has completed. The async namespace has the same omission at posthog/async_client.py:274. These sends also escape subsequent cleanup because repeated parent shutdown returns early; parent-owned email admission must honor the terminal lifecycle state, independently of whether standalone email clients support reopening. Reproduction: reproduced — python -m pytest posthog/test/test_review_candidates.py -k 'shutdown_is_terminal' --timeout=30 -q fails both sync and async cases on reviewed head because each sends a POST after parent shutdown, and both pass with the minimal temporary admission control.
There was a problem hiding this comment.
Fixed in d20805a. Parent shutdown is now terminal for client.emails: a retained handle or a fresh one raises EmailError with code="client_closed" and makes no request, on both Client and AsyncClient. Standalone Emails / AsyncEmails can still reopen after close(). Sync shutdown does not wait for a send already in flight; that send still completes, because a closed requests session stays usable. Covered by test_a_shut_down_client_sends_no_email.
marandaneto
left a comment
marandaneto
left a comment
There was a problem hiding this comment.
Public API / agent-friendliness review against the SDK guidelines. The send signature is readable and Pythonic. The only blocking concern is the advertised module-level posthog.emails.send(...) entry point, which still raises AttributeError; this is already tracked in #1093 (comment), so I have not duplicated that inline comment. All four new inline comments are optional, non-blocking suggestions; addressing them here or in a follow-up is at the author's discretion. Validation at this head: 44 email tests passed, public API snapshot check passed, and a no-network API probe reproduced the missing module entry point.
| ) -> _EmailsT: | ||
| app_host = determine_app_host(client_host) | ||
| is_cloud = app_host in (US_APP_ENDPOINT, EU_APP_ENDPOINT) | ||
| emails = factory(secret_key, host=app_host if is_cloud else None) |
There was a problem hiding this comment.
suggestion: Make email configuration explicit (non-blocking, optional) — This factory always uses the email defaults, so Posthog(..., max_retries=0, timeout=0.1).emails still has two retries and a 15-second timeout. The parent's settings are documented for event uploads, so this need not inherit them, but consider a documented email-specific configuration path for both sync and async clients, rather than requiring callers to mutate the returned helper or switch to a standalone client. This would make retry ownership easier for humans and agents to discover, in line with the API-design guidelines. Reproduction: reproduced — the focused pytest probe test_parent_configuration_is_not_email_configuration passed, confirming parent values (0.1, 0) and email values (15, 2).
There was a problem hiding this comment.
Done in 275a64d. Posthog and AsyncPosthog take email_request_timeout_seconds and email_request_max_retries, named after the existing feature_flags_request_* options and documented in the client's Args. They're validated at construction, so a bad value fails early instead of on the first send.
| ```python | ||
| from posthog.emails import Emails | ||
|
|
||
| emails = Emails("phs_...") |
There was a problem hiding this comment.
suggestion: Add a complete setup-and-verification quickstart (non-blocking, optional) — The send example is clear, but the prerequisites are scattered across docstrings. Consider a public quickstart recommending when to use client.emails versus standalone Emails, explaining the project-token/secret-key distinction, hog_flow:write, region, where to obtain a transactional template UUID, posthog[async], cleanup, and handling EmailError/the queued result. Document timeout units and scope, retry defaults, and the 30-second Retry-After policy alongside configuration. Include a way to verify success and diagnose a common configuration error, as suggested by the agent-friendly SDK guidelines. Reproduction: not reproduced — reviewed the changed documentation and examples; a clean-project integration using only published docs was not run.
There was a problem hiding this comment.
Done in 275a64d (c2ee166 after the rebase onto main). The posthog.emails module docstring is now a quickstart: when to use client.emails vs Emails, project token vs secret key and hog_flow:write, where the template ID comes from, hosts and app_host, posthog[async], what a result means, timeout units, retry defaults and the 30 s Retry-After policy, the error codes a setup usually hits (missing_secret_key, missing_app_host, 401/403, not_found, client_closed), cleanup and testing.
The code was renamed from app_host_required to missing_app_host in d6d07a4, to match PostHog/posthog#120921.
| def emails_with(replies: list[Reply], **options: Any) -> tuple[Emails, FakeAdapter]: | ||
| emails = Emails(SECRET_KEY, **options) | ||
| adapter = FakeAdapter(replies) | ||
| emails._get_session().mount("https://", adapter) |
There was a problem hiding this comment.
suggestion: Provide a supported no-network testing path (non-blocking, optional) — These tests need the private _get_session() transport hook, and the async cases replace _client; customers would need the same internal knowledge to assert email payloads or simulate a failure. Consider a documented fake/mock interface or supported transport injection for this feature, potentially as a follow-up. disabled=True prevents delivery but only returns id=None, so it cannot verify the attempted operation. This is the distinction made by the SDK testing guidelines. Reproduction: not reproduced — assessed the public API and test setup by inspection; no separate customer-facing mock integration was exercised.
There was a problem hiding this comment.
Done in 275a64d. Emails(session=...) takes a requests.Session and AsyncEmails(http_client=...) an httpx.AsyncClient; a transport you pass in is never closed by the SDK. The tests now use these public options instead of _get_session(), and the quickstart's testing section documents them. The remaining private access is in the client-lifecycle tests, which check the client's own transport.
|
|
||
| Needs ``secret_key``, a project secret API key with the ``hog_flow:write`` | ||
| scope. Sends go to the PostHog Cloud app host of ``host``. For any other | ||
| host, or to send email without capturing events, create |
There was a problem hiding this comment.
suggestion: Consider an app-host override on the existing client (non-blocking, optional) — Refusing to send the secret key to an ambiguous ingestion proxy is the right safety default. The ergonomic tradeoff is that a custom-host customer must create a separate Emails/AsyncEmails instance just to select the app host. Consider the explicit app_host override described in the shared email contract, or record why the standalone-client alternative is preferred. Either is an API decision rather than a security defect in the current refusal behavior; documenting the choice would help agents choose the intended integration path. Reproduction: not reproduced — compared the client property and standalone constructor with the linked contract; no custom-host network request was made.
There was a problem hiding this comment.
Done in 275a64d (c2ee166 after the rebase onto main). Both clients take app_host. With it set, client.emails sends there; without it, a non-Cloud host still raises missing_app_host before any request, and the error message now points at app_host.
The code was renamed from app_host_required to missing_app_host in d6d07a4, to match PostHog/posthog#120921.
marandaneto
left a comment
marandaneto
left a comment
There was a problem hiding this comment.
approving to unblock but left a few suggestions
|
@mayteio sdk-wise looks good but i guess your team should look at this as well since we dont have much context about this feature and how its gonna be used by customers, so a product-review would make sense for new stuff |
|
Thanks, its a good shout @marandaneto. I wont merge this until I've figure out what this looks like for customers, and in any case its a good reference for internal use-cases in correctly handling retries/errors. |
|
Note Update: the split is not needed for now. Every send carries an idempotency key, so a caller that retries can't queue the email twice, and only a caller that fails over to another provider needs to tell the two failures apart. PostHog/posthog#122019 makes that distinction in its own client. Could
Both can stay retryable with the same idempotency key, so Two smaller things:
Context: PostHog's own transactional emails ( |
Add posthog.emails.send() on Client and AsyncClient, plus standalone Emails and AsyncEmails in posthog.emails for processes that only send email. Sends go to the app host with a project secret API key and an Idempotency-Key, and retry network errors, rate limits, an in-flight key and 503 with the same key. Errors raise EmailError with PostHog's code and a Resend-style name. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Generated-By: PostHog Desktop Task-Id: c62b30cd-eb2e-496c-a73d-2a6e7a52a62e
…ports client.emails only infers PostHog Cloud app hosts. A custom host raises app_host_required before any request, so an ingestion proxy never receives the project secret key. A kept emails reference now reads the client's disabled state on every send. Shutdown closes the email transport, a forked child drops the parent's email session, and the retry ceiling applies after jitter. Docs examples use client.emails. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Generated-By: PostHog Desktop Task-Id: c62b30cd-eb2e-496c-a73d-2a6e7a52a62e
client.emails now asks the parent client to admit each send. After shutdown, a kept or new client.emails raises EmailError with code client_closed and makes no request. On AsyncClient, an admitted send is an in-flight operation, so shutdown waits for it, retries included, before it closes the email transport. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Generated-By: PostHog Desktop Task-Id: c62b30cd-eb2e-496c-a73d-2a6e7a52a62e
Posthog and AsyncPosthog take app_host, email_request_timeout_seconds and email_request_max_retries, so client.emails works behind a custom host and a job runner can turn retries off. Emails takes a session and AsyncEmails an http_client, which gives tests a supported no-network path. The posthog.emails module docs now hold a setup quickstart: keys and scope, template ID, hosts, retries, errors to expect, cleanup and testing. The release note names client.emails.send(). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Generated-By: PostHog Desktop Task-Id: c62b30cd-eb2e-496c-a73d-2a6e7a52a62e
275a64d to
c2ee166
Compare
- A non-Cloud host without app_host now fails with missing_app_host, the code the cross-SDK contract in PostHog/posthog#120921 defines, matching posthog-node. - A network_error keeps the requests or httpx exception as __cause__, so a failed send is easier to debug. - Regenerate the public API snapshot after the rebase onto main. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Generated-By: PostHog Desktop Task-Id: e72a2c10-f145-4f7c-8754-a4fcc06a18be
| self.secret_key, | ||
| self.host, | ||
| lambda: self.disabled, | ||
| lambda: None if self._shutdown_requested else _no_op, |
There was a problem hiding this comment.
P2 · Wait for admitted synchronous email sends before shutdown
When another thread calls shutdown() during emails.send(), this callback registers no active operation, so shutdown closes the session and returns before delivery finishes. Process teardown then loses unfinished sends. Track admitted sends and wait before cleanup, following the lifecycle contract checked under AGENTS.md:21.
There was a problem hiding this comment.
Not changing this. A sync shutdown() does not wait for a send that another thread already started, which matches the decision in #1093 (comment). Closing the requests session does not break a request in flight: the checked-out connection stays open, and a retry opens a new pool, so the send completes. Once shutdown is requested, a new send fails with client_closed. A send can only be lost if the caller runs it on a daemon thread and lets the process exit, and the capture lanes behave the same way at exit.
| response = await client.post( | ||
| url, content=prepared.body, headers=headers | ||
| ) |
There was a problem hiding this comment.
P2 · Apply the configured timeout to injected async clients
When callers supply http_client, _get_client() skips constructing the transport, and this request never applies self.timeout. Consequently, AsyncEmails(timeout=3, http_client=client) uses the client's timeout instead; a client configured with timeout=None leaves sends unbounded. Pass timeout=self.timeout on each request.
There was a problem hiding this comment.
Fixed in 8c622e8. Each async send passes timeout=self.timeout, so an injected client with timeout=None no longer leaves sends unbounded. The test "async send retries with the same key" now injects a client with timeout=None and checks the request timeout is 3 seconds.
| response = await client.post( | ||
| url, content=prepared.body, headers=headers | ||
| ) |
There was a problem hiding this comment.
P2 · Override transport authentication for async email requests
When an injected httpx.AsyncClient has default authentication, HTTPX applies it after merging these headers and replaces the email's Bearer token. A shared client configured with Basic auth therefore sends those credentials to PostHog and fails despite a valid secret_key. Pass auth=None per request to preserve the explicit Authorization header.
There was a problem hiding this comment.
Fixed in 8c622e8. Each async send passes auth=None, so the client's own auth can't replace the Bearer header. The same test now injects a client with Basic auth and checks the Bearer header. The sync path already passes its own auth on every request, which overrides Session.auth.
| to: str, | ||
| template_id: Union[str, UUID], | ||
| template_data: Optional[Mapping[str, Any]] = None, | ||
| distinct_id: Optional[str] = None, | ||
| idempotency_key: Optional[str] = None, |
There was a problem hiding this comment.
P2 · Expose the per-send engagement capture control
A caller passing capture_engagement=False gets TypeError because neither send method accepts it. The shared contract in PostHog/posthog#120921 requires this option for template sends, allowing callers to disable open and click tracking per email. Expose it on both send methods and serialize it into the request body, defaulting to True.
There was a problem hiding this comment.
Not adding this yet. PostHog/posthog#120921 defines capture_engagement, but the template send endpoint this PR calls (/messaging_templates/{id}/send/) does not accept it, so capture_engagement=False would be silently ignored and opens and clicks would still be captured. It can go in when the server supports it, which is planned for the single /messaging/send/email/ endpoint.
An httpx.AsyncClient passed as http_client can carry its own auth and timeout. Its auth replaced the email's Bearer header, and its timeout replaced the send timeout, so a client built with timeout=None left sends unbounded. Each async send now passes auth=None and timeout=self.timeout. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Generated-By: PostHog Desktop Task-Id: e72a2c10-f145-4f7c-8754-a4fcc06a18be
The template send route answers 422 for recipient_suppressed, sending_paused and sending_suspended as well as idempotency_key_reused. Only idempotency_key_reused is now invalid_idempotent_request; the send checks are validation_error, as the cross-SDK contract names them and posthog-node does. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Generated-By: PostHog Desktop Task-Id: e72a2c10-f145-4f7c-8754-a4fcc06a18be
A disabled client returned SendEmailResult(id=None), so a caller that treats a returned result as sent dropped the email silently. It now sends nothing and raises EmailError with code client_disabled (name application_error), as the cross-SDK contract in PostHog/posthog#120921 defines and posthog-node does. SendEmailResult.id is always the queued message ID, so its type is str. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Generated-By: PostHog Desktop Task-Id: e72a2c10-f145-4f7c-8754-a4fcc06a18be
💡 Motivation and Context
Note
Safe retries depend on the send API's idempotency key, which landed in PostHog/posthog#120446, PostHog/posthog#120510 and PostHog/posthog#120392. Release this only once those are deployed to every region.
Changes
client.emails.send(to=, template_id=, template_data=, distinct_id=, idempotency_key=)onPosthogandAsyncPosthogqueues one email from a transactional template and returns its message ID. A result means queued, not delivered.EmailsandAsyncEmailsinposthog.emailssend without a capture client, for processes that only send email or use their own key and host.us.i.posthog.comandeu.i.posthog.commap tous.posthog.comandeu.posthog.com. The path uses@current, so no project ID is needed.client.emailsrefuses any host that is not PostHog Cloud withmissing_app_host, before any request, unless the client setsapp_host. A custom host is often an ingestion proxy, which must not receive the secret key.email_request_timeout_secondsandemail_request_max_retrieson the client configure sends, next to the existingfeature_flags_request_*options.Emails(session=...)andAsyncEmails(http_client=...)accept your own transport, so tests can assert a send without private hooks.posthog.emailsmodule docs hold the setup quickstart: keys and scope, the template ID, hosts, retries, the errors a setup usually hits, cleanup and testing.Idempotency-Key. The client makes a UUID when the caller gives none.quota_limitedis never retried. ARetry-Afterover 30 seconds raises instead of waiting.max_retries=0leaves retries to a job runner.EmailErrorwith PostHog'scodeand a Resend-stylename. A disabled client sends nothing and raisesclient_disabled.{data, error}, and v1 sends by template UUID to/messaging_templates/{id}/send/until the key-based route lands. Both are in the contract comment.determine_app_hostinrequest.py, anauthparameter on the localrequestsstub, the public API snapshot, and a changeset.Note
Divergence from PostHog/posthog#120921: the contract's
capture_engagementsend option is not here yet. The template send endpoint this SDK calls does not accept it, so the option would be silently ignored. It lands with the server support.💚 How did you test it?
posthog/test/test_emails.pycatches these regressions: a retriedquota_limited429 ortemplate_outdated409, an idempotency key that changes between retries, a secret key sent to an ingestion host, a disabled client that still sends, andDecimalordatetimevalues intemplate_datathat fail to encode.toand a 401 mapped to the rightname.master, a second send with the same key returned the first send's ID withreplayed=True, and MailDev received one email. The same key with another body raisedidempotency_key_reusedasinvalid_idempotent_request.📝 Checklist
If releasing new changes
sampo addto generate a changeset file🤖 Agent context
Autonomy: Human-driven (agent-assisted)
claude-opus-5-5) in a PostHog Desktop cloud task, directed by the assignee. Skills used:writing-pr-descriptions.Emailsis a standalone class thatclient.emailswraps, becauseClient.disabledis true without a project token. An email-only process would otherwise never send.EmailErrorclass carriescodeandname. Subclasses can come later without breaking callers.template_datagoes through the sameclean()as capture properties, soDecimal,UUIDand datetimes encode the same way.Created with PostHog Desktop
🤖 Generated with Claude Code