[Silthus]

Problem

A workflow run that waits on a retry while it sits on the email queue loses its place: when it leaves, it resumes on the hogflow queue at the email priority instead of its own.

• The email worker stores the run's origin queue and priority in queueMetadata when it takes the email.
• Three email reschedule paths dropped that metadata: SES throttles, workflow pacing, and team sending caps.
• A second email sent inline right after the first dropped it too.
• A push notification retried on the email queue replaced it with only its retry counter.
• Push retries also set the row's priority to the retry count, which ranks them among the email priority classes (transactional 0, bulk 1).
• Without the origin, the run falls back to hogflow at the email class priority, so a bulk send can jump ahead of other work.

Note

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

• 23 production lines change which queue and priority a run uses after a retry on the email queue. Sends, retry delays and retry budgets do not change.
• The round-trip test pins every line, and a revert restores the old fallback. Metadata left on queued rows is read by code already on master.

Changes

• Runs that wait on an email or push retry on the email queue now resume on their origin queue at their original priority.
• EmailService copies queueMetadata onto the result on all three reschedule paths.
• HogFlowExecutorService keeps queueMetadata between actions while the run is on the email queue.
• HogExecutorAsyncService passes it to an inline email send, so a throttled second email keeps it too.
• A push retry on the email queue gets the origin fields added back after the send, and waits at its own message category's email priority. That is the rule an inline email send already follows.
• Each push action keeps its own retry budget. The obvious fix copies the run's metadata onto the push before the send. Because the executor now keeps that metadata between actions, it would carry one push's retry count into the next push.
• Mechanical: getEmailQueuePriority names the class-to-priority lookup that the inline email send and the push retry share.
• PushNotificationService does not change. Push retries off the email queue keep the retry-count priority.
• The sending-tiers doc states the retry behavior.

The push fix first opened as #115192, which is closed and folded in here. It needs the executor line above: on master alone, the run loses its origin between the email and the push action.

How did you test this code?

Test rationale: each production line has an existing test that now fails without it. No new standalone tests.

• email.service.test.ts: the shared invocation carries origin metadata. The SES throttle, workflow pacing and team cap reschedule tests assert it survives. Before the fix, 8 cases failed with Received: undefined.
• hogflow-executor.service.test.ts: the existing round trip now sends an email, two transactional pushes that FCM rejects once each, a throttled email, then a fetch. It uses the real PushNotificationService, with only the FCM fetch stubbed.
• It asserts each push retry keeps originQueue: hogflow, originPriority: 2, tries: 1 at priority 0, and that the run returns to hogflow at priority 2.
• Without the push fix, the run returned at priority 1. With the obvious pre-send copy in its place, the second push started at tries: 2.
• Ran locally on 37152c1: email.service, email-priority, hogflow-executor.service, hog-executor.service, push-notification.service and cdp-cyclotron-worker-email.consumer tests, all green.
• CI has not run on this head: the workflow runs wait for maintainer approval because the branch lives in a fork.
• Not checked: a real SES account, a real FCM or APNs provider, or a deployed worker.

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

• Silthus/posthog#310
• #112820
• #112316
• #112295
• Red on parent
• Consecutive-email red
• Green on final head
• Full local gate and commands
• included hogli runner
• QA ledger and review reports

Release status

• No feature flag controls this change

Docs update

docs/internal/workflows-email-sending-tiers.md states that a retried email or push restores the run's origin queue and priority, and that a push retry waits at its own category's priority.

🤖 Agent context

Autonomy: Human-driven (agent-assisted)

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

• A QA swarm ran two review rounds across GPT 6.1 Sol and Claude Opus 5.5. It found one test-isolation issue and two nits, all fixed in 37152c1, and nothing in the fixes.
• Skills: tdd, writing-tests, stacking-prs, pr, writing-pr-descriptions, writing-voice, merge-risk, qa-swarm.
• The branch was rebuilt from master after chore(workflows): send test email through a local ses fake #112316 lost its container setup. The tests moved from the container consumer test to the sesV2Client spy and the executor round trip.
• A Codex GPT 6.1 Sol review of the rebuild found the push retry bug. The fix opened as fix(workflows): keep email origin when a push retries on email queue #115192, then moved here because it cannot work without this PR's executor line.
• Merge risk stays low: the change moves only resume priority and queue metadata.
• Duplicate check: no other open PR carries this fix.

🤖 Generated with Claude Code

[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

📝 Walkthrough

📝 Walkthrough

Walkthrough

Email reschedules caused by workflow pacing, team caps, and SES throttling now retain queue metadata. The hogflow executor carries that metadata into results for email-queue invocations. The change adds local SES Compose and test support, plus service, consumer, and end-to-end coverage for email delivery and retry behavior.

---

---

---

---

Priority: ➖ Normal

Severity of issue fixed: Low

Merge Risk: 🔵 Low · up to a63fc

Email retry behavior appears mergeable, but the consecutive-email test should distinguish the two deliveries so a skipped second email cannot pass unnoticed.

Security Architecture Review

Security architecture risk: 🔵 Low · up to a63fc

The inspected changes preserve scheduling context without granting additional email-sending permissions or weakening sending limits. Risk is low but not zero: dispatch ordering changes have irreversible effects once emails are sent, and interrupted-delivery behavior is not fully established.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

• inferred — The production effect is bounded to email-owned invocations and their subsequent workflow routing, potentially across teams using that shared execution path. The inspected changes do not grant new queue-injection capability, alter tenant identity, or create a remotely published SES service.

Trust Boundaries and Controls

• observed — The inspected API caller checks function/team ownership, queued invocations carry identity independently of routing metadata, and the email service checks integration ownership before sending. The metadata preservation does not bypass these boundaries; authorization for every upstream queue producer remains outside the inspected coverage.

Resilience and Maintainability Implications

• inferred — Metadata preservation supports recovery through delayed retries without weakening the inspected pacing or cap branches. It does not establish atomicity between provider acceptance and queue persistence, or introduce an exactly-once delivery guarantee. Crash/replay idempotency remains an unestablished property rather than an observed PR regression.

---

---

---

Pre-merge checks | 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Description check		The description covers the queue-metadata behavior, release status, documentation, agent context, and existing regression tests. It does not describe substantial changes shown in the diff: the local S…	Update Problem and Changes to explain the local SES test infrastructure and all user-visible or mechanical changes. Update How did you test this code? with rationale and evidence for each new or changed test group, including the LocalSes, R…

Full details: Description check

Explanation

The description covers the queue-metadata behavior, release status, documentation, agent context, and existing regression tests. It does not describe substantial changes shown in the diff: the local SES Compose service, CI service startup and path-filter changes, LocalSes and TestRedisV2 helpers, the new EmailService SES integration suite, expanded consumer and E2E tests, or the fixture update. It also states that no new standalone tests were added, which conflicts with the added test files.

Resolution

Update Problem and Changes to explain the local SES test infrastructure and all user-visible or mechanical changes. Update How did you test this code? with rationale and evidence for each new or changed test group, including the LocalSes, Redis, EmailService SES, consumer, and E2E coverage. Reconcile the stated scope, risk, and line-count claims with the complete diff.

---

• Fix all pre-merge checks with AI

✨ 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)

nodejs/src/cdp/consumers/cdp-cyclotron-worker-email.consumer.test.ts (1)

126-142: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value

Use a one-shot subscription for the reload notification.

The shared test hub keeps PubSub.on listeners until suite teardown. Each paused case therefore leaves a listener attached after its promise resolves. Add PubSub.once and use it here.

Suggested fix

--- a/nodejs/src/common/utils/pubsub.ts
+++ b/nodejs/src/common/utils/pubsub.ts
@@
         void this.promises.schedule(this.redisSubscriber.subscribe(channel))
         this.eventEmitter.on(channel, (message) => listener(message ? parseJSON(message) : {}))
     }
+
+    public once<T extends Record<string, any>>(channel: string, listener: (message: T) => void): void {
+        if (!this.redisSubscriber) {
+            throw new Error('PubSub must be started before subscribing to channels!')
+        }
+
+        void this.promises.schedule(this.redisSubscriber.subscribe(channel))
+        this.eventEmitter.once(channel, (message) => listener(message ? parseJSON(message) : {}))
+    }
 }
--- a/nodejs/src/cdp/consumers/cdp-cyclotron-worker-email.consumer.test.ts
+++ b/nodejs/src/cdp/consumers/cdp-cyclotron-worker-email.consumer.test.ts
@@
-                hub.pubSub.on('reload-hog-flows', () => resolve())
+                hub.pubSub.once('reload-hog-flows', () => resolve())

---

ℹ️ Review info

⚙️ Run configuration

• Configuration used: Repository: PostHog/posthog/.coderabbit.yaml
• Review profile: QUIET
• Plan: Enterprise
• Run ID: 5da4a5ad-6bc4-455b-9664-95e7221f2b88

📥 Commits

Reviewing files that changed from the base of the PR and between db1a0c4 and b354d34.

📒 Files selected for processing (14)

• .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/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; 9 remain after this review.

[Silthus]

Note

🤖 GPT-6.1 Sol responding on behalf of Michael

@coderabbitai The retained reload listener is confirmed in the unchanged #112831 fixture at parent 86f601c. Suite teardown clears the listeners; adding a shared PubSub.once API is outside this email queue metadata fix. The cleanup is recorded as a proposed follow-up in the QA ledger.

[coderabbitai]

🧹 Nitpick comments (1)

nodejs/src/cdp/consumers/cdp-cyclotron-worker-email.consumer.test.ts (1)

291-291: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Give the two email actions distinct content.

The intermediate and final SES assertions check identical content. Sending first-email twice while skipping email can still satisfy them. Give first-email a distinct subject, assert that the intermediate SES output contains only that subject, and assert that completed delivery contains both subjects.

Suggested test changes

             if (consecutive) {
-                const emailAction = flow.actions.find((action) => action.id === 'email')!
-                flow.actions.push({ ...emailAction, id: 'first-email' })
+                const emailAction = flow.actions.find(
+                    (action) => action.id === 'email' && action.type === 'function_email'
+                )!
+                flow.actions.push({
+                    ...emailAction,
+                    id: 'first-email',
+                    config: {
+                        ...emailAction.config,
+                        inputs: {
+                            ...emailAction.config.inputs,
+                            email: {
+                                ...emailAction.config.inputs.email!,
+                                value: {
+                                    ...emailAction.config.inputs.email!.value,
+                                    subject: 'First email',
+                                },
+                            },
+                        },
+                    },
+                })
@@
-                            subject: 'S2 round trip',
+                            subject: 'First email',
@@
-            expect(await ses.getEmails()).toEqual(
-                Array.from({ length: sentCount }, () =>
-                    expect.objectContaining({
-                        subject: 'S2 round trip',
-                        body: { text: 'Retry delivery', html: '<p>Retry delivery</p>' },
-                    })
-                )
-            )
+            expect(await ses.getEmails()).toEqual(
+                consecutive
+                    ? [
+                          expect.objectContaining({
+                              subject: 'First email',
+                              body: { text: 'Retry delivery', html: '<p>Retry delivery</p>' },
+                          }),
+                          expect.objectContaining({
+                              subject: 'S2 round trip',
+                              body: { text: 'Retry delivery', html: '<p>Retry delivery</p>' },
+                          }),
+                      ]
+                    : Array.from({ length: sentCount }, () =>
+                          expect.objectContaining({
+                              subject: 'S2 round trip',
+                              body: { text: 'Retry delivery', html: '<p>Retry delivery</p>' },
+                          })
+                      )
+            )

---

ℹ️ Review info

⚙️ Run configuration

• Configuration used: Repository: PostHog/posthog/.coderabbit.yaml
• Review profile: QUIET
• Plan: Enterprise
• Run ID: 9248613d-a983-49d2-aac4-af16c54917b1

📥 Commits

Reviewing files that changed from the base of the PR and between b354d34 and a63fc2a.

📒 Files selected for processing (3)

• docs/internal/workflows-email-sending-tiers.md
• nodejs/src/cdp/consumers/cdp-cyclotron-worker-email.consumer.test.ts
• nodejs/src/cdp/services/hog-executor-async.service.ts

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

[coderabbitai]

Tip

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

@Silthus, thanks for the scope clarification. The listener cleanup can remain follow-up work rather than a required change for #112850. Would you like a GitHub issue for the follow-up recorded in the QA ledger?

---

✏️ Learnings added

Learnt from: Silthus
URL: https://github.com/PostHog/posthog/pull/112850

Timestamp: 2026-10-06T19:28:43.742Z
Learning: For PostHog PR #112850, the retained reload listener is inherited from the unchanged #112831 test fixture at parent commit 86f601c. Suite teardown clears the listeners. A shared PubSub.once API is outside the email queue metadata fix; listener cleanup is proposed follow-up work in the QA ledger at https://gist.github.com/Silthus/4f4c825c4e76007477236cb6e0eb71bd#file-wf310-review-ledger-md.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

You are interacting with an AI system.

[parameterai]

Risk: No findings

This PR preserves a workflow run's origin queue and priority (queueMetadata with originQueue/originPriority) across retries that happen while the run sits on the email queue: SES throttle, workflow pacing, team sending caps, inline second emails, and push-notification retries. I traced every tenant-reachable input into the new code; the only tenant-controlled value reaching a priority decision is message_category_type in the flow action config, which maps to the email queue's pre-existing fast/bulk priority classes (email-priority.ts:36-42) — a self-declared, already-shipped opt-in design, not a new capability. originQueue/originPriority are only ever written from internal invocation fields (hog-executor-async.service.ts:308-312), and the restore path (routeToQueue, line 340-357) clears the metadata when leaving the email queue, so no stale or forged metadata can reorder a run on the hogflow queue. The tries retry counter remains bounded by maxRetries on every path. I found no security vulnerability introduced by this change.

_{Sentinel reviewed 37152c1 · Review settings}

[pr-assigner-resolver-posthog]

👀 Auto-assigned reviewers

These soft owners were skipped because they only have minor changes here. Nothing blocks merge, so self-assign if you'd like a look:

• @PostHog/team-ingestion (owners.yaml)

Soft owners come from each directory's owners.yaml and each product's product.yaml (resolved nearest-file-wins). For a skipped owner, the locator is the file that decided it. Generated files and lockfiles are ignored when deciding ownership.

[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 #127102 so CI can run on a PostHog branch.