Skip to content

fix(workflows): retry ses-throttled emails instead of dropping them - #112295

Merged
trunk-io[bot] merged 2 commits into
PostHog:masterfrom
Silthus:fix/workflows-ses-throttle-keep-email-params
Oct 8, 2026
Merged

trunk-io[bot] merged 2 commits into
PostHog:masterfrom
Silthus:fix/workflows-ses-throttle-keep-email-params

Conversation

@Silthus

@Silthus Silthus commented Oct 6, 2026 •

Copy link
Copy Markdown
Contributor

Problem

  • When SES throttles a workflow email, the email is never sent. The run continues as if the step had finished.
  • This affects every team that sends from its own domain, not only sandbox sends.
  • The SES throttle reschedule (email.service.ts#L624-L631) does not re-attach the email queue parameters. createInvocationResult clears them.
  • On the retry, the executor finds no email queue parameters (hog-executor-async.service.ts#L127). It resumes the Hog VM instead (#L196), so the send is skipped.
  • The per-workflow pacing and team cap reschedules already re-attach the parameters for this reason.

Warning

🟡 Merge risk: medium (merge-risk rubric)

  • The change is on the real email send path. Throttled sends that were silently dropped now go out on retry.
  • A retried send charges the team cap and the per-workflow pacing again (see below). That can delay other emails. It cannot drop or duplicate an email.
  • The diff is one line in the throttle branch, with one focused assertion. It reverts cleanly.

Changes

Kind Files Lines
Production 1 +1 −0
Tests 1 +1 −0
Total 2 +2 −0
  • An SES-throttled email now goes out on the retry. The run waits for it, as it already does for pacing and cap delays.
  • The throttle branch sets result.invocation.queueParameters = params, the same as the other two reschedule paths.
  • The priority class is unchanged. The throttled send keeps its incoming queuePriority.

Not fixed here: the retry charges the cap twice

  • Yes, on master a throttled send keeps its team cap claim. The retry then claims again.
  • claimTeamSendingBudget runs before the SES call (#L556). In enforce mode it takes the tokens atomically (#L762).
  • RateLimiterService has no refund or release method, and the throttle branch returns no tokens.
  • The per-workflow pacing token behaves the same way.
  • Before this PR, the double charge never happened, because the retry never re-entered the send path. This PR makes it happen for each throttled send.
  • fix(workflows): guard sandbox sends on empty settings and SES throttle #112071 refunds the cap for sandbox sends only. A general refund belongs in a follow-up.

How did you test this code?

Test rationale: the existing SES throttle it.each in email.service.test.ts now asserts that the reschedule carries the email queue parameters. It failed on master with Received: undefined for both throttle errors, and it passes with the fix. The pacing reschedule test already makes the same assertion. The throttle path had no such check.

Run locally, not committed: an e2e probe in workflows-e2e.serial.test.ts (the email queue block). It makes the first maildev send throw SESThrottleError, then counts the run's metrics.

Send attempts email_sent Job status
master 1 0 completed
This PR 2 1 completed
  • On master the run completes, but the email is lost.
  • Not checked: a real SES throttle against AWS.

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 (claude-opus-5-5)

🤖 Generated with Claude Code

The SES throttle reschedule did not re-attach the email queue parameters,
so the retry resumed the Hog VM and the email was never sent. Re-attach
them, the same way as the per-workflow and team cap reschedules.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@trunk-io

trunk-io Bot commented Oct 6, 2026 •

Copy link
Copy Markdown

😎 Merged successfully - details.

@coderabbitai

coderabbitai Bot commented Oct 6, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: PostHog/posthog/.coderabbit.yaml
  • Review profile: QUIET
  • Plan: Enterprise
  • Run ID: 7e1daecb-d8ea-45bd-ac71-fe17e48b1735
📥 Commits

Reviewing files that changed from the base of the PR and between 1ebb06a and 9668491.

📒 Files selected for processing (2)
  • nodejs/src/cdp/services/messaging/email.service.test.ts
  • nodejs/src/cdp/services/messaging/email.service.ts
💤 Files with no reviewable changes (2)
  • nodejs/src/cdp/services/messaging/email.service.test.ts
  • nodejs/src/cdp/services/messaging/email.service.ts

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


📝 Walkthrough

Walkthrough

When SES returns a recognized throttle error, executeSendEmail restores the email queue parameters before scheduling a retry. The test now checks that the rescheduled invocation retains the original parameters.

Priority: ➖ Normal

Merge Risk: ⚪ Minimal · up to 96684

Throttled emails retain the information needed for a retry. No merge-blocking issue was identified.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 1ebb0

The fix preserves the existing retry mechanism and retains tenant, sender, suppression, and cancellation controls. One unresolved privacy question is whether a delayed marketing retry respects an unsubscribe made after the original send was authorized.

Retained concerns

  • Medium · security · inferred: Functional SES-throttle retries may deliver marketing email after an intervening opt-out. The retained payload reaches direct email dispatch, whose recipient gate checks suppression, while marketing opt-outs update a separate preference store checked upstream. Confirming that the enclosing workflow resume repeats that preference check is necessary to establish consent continuity. The control-placement question predates this PR for pacing/cap retries; the potential added exposure is the newly enabled SES-throttle retry path, not additional tenant authority.
Security review details

Security Blast Radius

  • inferred — The incremental exposure concerns previously authorized, SES-throttled email invocations and their recipients. Existing integration ownership and team-scoped recipient lookups provide no evidence of new cross-tenant authority or credential access.

Security Findings and Attack Paths

  • inferred — The privacy scenario requiring confirmation is: a marketing action passes its initial preference check, SES throttles the send, the recipient opts out while it waits, and the retained email payload later dispatches without refreshed consent. The separate stores and direct inner dispatch support this possibility, but the complete enclosing action-resume path remains a coverage gap.

Trust Boundaries and Controls

  • observed — Marketing consent and deliverability suppression are distinct controls. Preference enforcement checks category and all-marketing opt-outs upstream, with an explicit transactional exemption. Send-time suppression uses a separate team-scoped cached lookup that fails open on lookup errors; that behavior predates this change.

Resilience and Maintainability Implications

  • observed — Queue rescheduling updates the job under its existing lock identity and wakes promptly when cancellation is requested. Deliberate reschedules reset the consecutive-stall counter, so that counter alone does not bound repeated throttle retries. Production expiry or dead-letter policy and provider-side exactly-once behavior were not established.

Hardening Proposals

  • proposed — If workflow resume does not refresh marketing consent, enforce category-aware recipient preferences at the final dispatch boundary for deferred sends, preserving the intentional transactional exemption and tenant scope.
🚥 Pre-merge checks | ✅ 1
✅ Passed checks (1 passed)
Check name Status Explanation
Description check ✅ Passed The description explains the problem, user impact, code change, test rationale and results, known retry-cost limitation, release status, and agent context. It is substantially complete and stands on i…
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

@Silthus
Silthus marked this pull request as ready for review October 6, 2026 04:29
@Silthus
Silthus requested a review from a team as a code owner October 6, 2026 04:29
@parameterai

parameterai Bot commented Oct 6, 2026 •

Copy link
Copy Markdown

Risk: No findings

The change since the last review removes two explanatory comments only; the production fix — re-attaching invocation.queueParameters on the SES-throttle reschedule path in email.service.ts — is unchanged and matches the existing pacing and team-cap reschedule branches. No behavioral delta and no new security surface.

Sentinel reviewed 9668491 · Review settings

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
This was referenced Oct 6, 2026
@trunk-io
trunk-io Bot merged commit ecc6b4f into PostHog:master Oct 8, 2026
@deployment-status-posthog

deployment-status-posthog Bot commented Oct 8, 2026 •

Copy link
Copy Markdown

Deploy status

Environment Status Deployed At Workflow
dev ✅ Deployed 2026-10-08 07:59 UTC Run
prod-us ✅ Deployed 2026-10-08 08:15 UTC Run
prod-eu ✅ Deployed 2026-10-08 08:17 UTC Run

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.

2 participants