[Silthus]

Problem

A workflow email step can show as failed even though the recipient got the email.

• After SES accepts the email, the worker builds the saved copy behind "View email" and reads the team's engagement setting.
• If building the saved copy throws, the step records email_failed with that error.
• If reading the engagement setting throws, the whole send call throws, and the step fails.

Note

🔵 Merge risk: low · one-way door (merge-risk rubric)

• Only the outcome recorded after SES accepts an email changes. Sends, retries and failed sends keep their current behavior.
• Events captured as sent stay captured after a revert. Two tests pin both fault paths.

Changes

• An accepted email stays sent when bookkeeping fails. The run log shows a warning instead:
  • "Email sent, but its saved copy could not be created."
  • "Email sent, but its engagement event could not be recorded."
• Without a saved copy, the "Email sent" log line leaves out the "View email" link, so the link does not 404.
• A new counter, cdp_email_bookkeeping_failures_total, counts each fault by stage.
• A failed send still throws when the engagement setting read fails, as it does today.

Before:

flowchart LR
    A[Provider accepts email] --> B[Bookkeeping fails] --> C[Run fails or email_failed]
    classDef phRed fill:#f54e00,stroke:#f54e00,color:#fff;
    classDef phBlue fill:#1d4aff,stroke:#1d4aff,color:#fff;
    classDef phYellow fill:#f9bd2b,stroke:#f9bd2b,color:#000;
    class A phRed;
    class B phBlue;
    class C phYellow;

Loading

After:

flowchart LR
    A[Provider accepts email] --> B[Bookkeeping fails] --> C[Sent outcome, warning and counter]
    classDef phRed fill:#f54e00,stroke:#f54e00,color:#fff;
    classDef phBlue fill:#1d4aff,stroke:#1d4aff,color:#fff;
    classDef phYellow fill:#f9bd2b,stroke:#f9bd2b,color:#000;
    class A phRed;
    class B phBlue;
    class C phYellow;

Loading

This replaces the earlier version of this PR, which sat on top of #112859 and the stack down to the local SES container from #112316. It now targets master alone, and drops the local-development doc paragraph that described that container suite.

How did you test this code?

Test rationale: one parameterized case next to the existing engagement-capture tests. SES accepts the send through the sesV2Client spy, then either the saved copy or the engagement setting read fails.

• It asserts no error, email_sent without email_failed, success: true on the VM stack, the run-log warning, and exactly one SES call.
• Before the fix, both cases failed: one with Received: "Asset row fault", the other with a thrown error.
• Ran locally: email.service.test.ts, all green.
• Not checked: the counter value (a Prometheus side channel), a real SES account, or a deployed worker.

Earlier evidence (stacked version on top of #112859, container-based)

• Silthus/posthog#312
• #112850
• #112831
• #112820
• #112316
• #112295
• Red artifact
• Green artifact
• Local gate artifact
• Test environment wrapper
• QA panel
• read-only Opus review

Release status

• No feature flag controls this change

Docs update

None. The earlier version documented this in the local-development guide's container section, which no longer exists.

🤖 Agent context

Autonomy: Human-driven (agent-assisted)

Agent: T3 Code / Claude Code, Claude Opus 5.5 (rebuild on master). The earlier stacked version: T3 Code / Codex, GPT-6.1 Sol.

• Skills: tdd, writing-tests, pr, writing-pr-descriptions, writing-voice, merge-risk.
• The branch was rebuilt from master after chore(workflows): send test email through a local ses fake #112316 lost its container setup. The production change is unchanged. The tests moved from the container suite to the sesV2Client spy.
• The consumer-test cleanup from the earlier version belonged to the container setup and is gone.

[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]

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: 635e0a4e-7ae6-4a78-8289-2963993fc56e

---

📥 Commits

Reviewing files that changed from the base of the PR and between 2c1850a and 1d9e48c.

---

📒 Files selected for processing (15)

• .github/workflows/ci-nodejs.yml
• docker-compose.base.yml
• docker-compose.dev.yml
• docs/internal/workflows-email-sending-tiers.md
• docs/published/handbook/engineering/developing-locally.md
• nodejs/src/cdp/consumers/cdp-cyclotron-worker-email.consumer.test.ts
• nodejs/src/cdp/services/hog-executor-async.service.ts
• nodejs/src/cdp/services/hogflows/hogflow-executor.service.ts
• nodejs/src/cdp/services/messaging/email.service.ses.test.ts
• nodejs/src/cdp/services/messaging/email.service.test.ts
• nodejs/src/cdp/services/messaging/email.service.ts
• nodejs/src/cdp/workflows-e2e.serial.test.ts
• nodejs/tests/fixtures/hog-bytecode/25244285018dd2a251c9f119b744f227c721c1916af1224ecea41b262a68bdab.json
• nodejs/tests/helpers/redis-v2.ts
• nodejs/tests/helpers/ses.ts

---

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

---

---

📝 Walkthrough

Walkthrough

Email reschedules now retain queue metadata across workflow pacing, team-cap, and SES-throttle retries. Post-send failures while saving email assets or looking up engagement configuration are logged and counted without undoing an accepted send. The change adds optional local SES Compose support, test helpers, and service and workflow coverage for SES delivery and retry behavior.

---

Priority: ➖ Normal

Merge Risk | ⚪ Minimal · up to 1d9e4

Merge Risk: ⚪ Minimal · up to 1d9e4

The SES tests have a configured service endpoint, and no confirmed issue remains to resolve before merging.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 1d9e4

The change improves failure containment after email delivery without an identified expansion of tenant privileges or external access. Risk is low, but delivery remains irreversible and recovery from an interruption around provider acceptance is not fully demonstrated.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

• inferred — The changed production behavior affects queued workflow emails, their external recipients, and the originating tenant's saved copies, metrics and engagement events. Preserved routing metadata does not replace tenant identity or recipient parameters in the inspected flow.

Trust Boundaries and Controls

• inferred — No new bypass of sender, tenant or recipient controls was established in the traced production paths. The bookkeeping guards run after the provider call; they do not move delivery ahead of those checks. The added test listener has no application authentication, but its inspected exposure is loopback-bound and test-owned.

Resilience and Maintainability Implications

• inferred — The PR contains the selected auxiliary failures without retrying an already accepted send. It does not demonstrate exactly-once delivery across process interruption or an uncertain provider response. The inspected provider path has no durable acceptance reconciliation or idempotency key; that limitation is unchanged from the PR base, not a newly introduced concern.

Hardening Proposals

• proposed — As separate hardening, establish the provider's acceptance-after-timeout contract and evaluate durable delivery reconciliation or deduplication where supported. Recovery tests should distinguish explicit pre-send throttling from interruption after acceptance.

---

Pre-merge checks | 1

✅ Passed checks (1 passed)

Check name	Status	Explanation
Description check		The description is complete and follows the required structure. It explains the problem, user-visible changes, testing and limitations, release status, documentation status, agent context, and include…

---

✨ 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.}

[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]

Keeping this open. I'll pick it up when I start on Nov 2.

[Silthus]

Moved to #127174 so CI runs.