[Silthus]

Problem

Workflow developers cannot catch SES payload and HTTP error regressions when email tests replace the SDK’s send method.

Note

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

• The layer adds a test helper and tests. Production send behavior comes from the parent PR.
• The dev stack and CI do not change. Port 4566 stays dead locally, so an unconfigured send cannot reach real AWS.

Changes

Kind	Files	Lines
Production	1	+1 −0
Tests	3	+302 −0
Total	4	+303 −0

• Tests send email through the real SESv2 SDK to an in-process HTTP proxy on a random port.
• The proxy records each request, answers SendEmail with a MessageId, and keeps the delivered emails.
• The proxy injects SES errors at the HTTP boundary, so SDK serialization, retries and error mapping stay covered.
• A compose SES container is not necessary: the proxy already holds everything the tests assert on. An earlier revision of this PR used one, and review removed it.

Stacked on #112295; the GitHub diff includes its ancestors. Counts and reviews cover this layer.

Before:

flowchart LR
    Tests[EmailService tests] --> Mock[Mocked SDK send]
    classDef phYellow fill:#f9bd2b,stroke:#f9bd2b,color:#000;
    classDef phGray fill:#e5e7eb,stroke:#c7ccd1,color:#000;
    class Tests phYellow;
    class Mock phGray;

Loading

After:

flowchart LR
    Tests[EmailService tests] --> SDK[Real SESv2 SDK] --> Proxy[In-process SES proxy] --> Inbox[Request and delivery assertions]
    classDef phBlue fill:#1d4aff,stroke:#1d4aff,color:#fff;
    classDef phRed fill:#f54e00,stroke:#f54e00,color:#fff;
    classDef phYellow fill:#f9bd2b,stroke:#f9bd2b,color:#000;
    classDef phGray fill:#e5e7eb,stroke:#c7ccd1,color:#000;
    class Tests phYellow;
    class SDK,Proxy phBlue;
    class Inbox phGray;

Loading

How did you test this code?

Test rationale: S1 drives EmailService.executeSendEmail through the real SDK. Cases cover delivery, SDK retries and hard failures. With the throttle reschedule fix reverted, both retry cases fail.

Real AWS provisioning and mailbox delivery remain untested. Fork CI requires maintainer approval.

Evidence and context

• Real SDK connection refused
• Red rerun
• Green run
• Full local gate
• Evidence below predates the in-process proxy. The compose, platform and shared-inbox entries describe the removed container.
• Platform and service selection
• Profile and shared inbox
• Reviewed tree
• Final service selection
• Node.js CI
• Actions API evidence
• Silthus/posthog#297
• The final diagnostic correction passed red/green proof

Release status

• No feature flag controls this change

Docs update

None. The tests need no extra service, so the local development guide does not change.

🤖 Agent context

Autonomy: Human-driven (agent-assisted)

Agent: T3 Code / Codex, GPT-6.1 Sol

Skills: tdd, authoring-ci-workflows, writing-tests, writing-code-comments, writing-pr-descriptions, reviewing-with-coderabbit, running-ci-preflight, merge-risk, qa-swarm, leaving-pr-comments, writing-voice, unslop, principle-prove-it-works.

QA and read-only Opus review findings are fixed: inbox isolation, connection diagnostics, AWS profiles, compose compatibility and setup documentation.
CodeRabbit CLI was signed out and skipped.

A review follow-up by Claude Code (Claude Opus 5.5) replaced the compose container with the in-process proxy and removed the compose, CI and docs changes. Skills: writing-pr-descriptions.

[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

SES throttle retries now retain the original email queueParameters. Tests verify successful delivery, preserved payloads across retries, sent and failure metrics, and terminal behavior for LimitExceededException and SendingPausedException. A local SES server supports these tests by capturing requests, returning configured responses, and storing converted email messages.

---

---

Priority: ⬇️ Low

Merge Risk: ⚪ Minimal · up to 902b9

This change keeps the original email payload when a throttled send is retried, and the added tests cover delivery and error cases. No merge-blocking risk remains.

Security Architecture Review

Security architecture risk: 🔵 Low · up to a9a43

The retry retains existing sender-ownership, suppression and sending controls. The new test service uses loopback endpoints and invented credentials. No introduced security weakness was demonstrated, but delivery behavior after interrupted execution is not fully established.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

• inferred — The production effect concerns email invocations encountering SES throttling across projects using this service. A workflow-controlled sender reference remains constrained to the invocation's team, and retried sends still traverse suppression and pacing controls.

Trust Boundaries and Controls

• observed — The proxy forwards request headers to its configured upstream, but the concrete test consumer supplies invented AWS credentials and removes session-token and profile settings. Both proxy and published emulator endpoints are loopback-bound; inbox filtering is test attribution, not an authorization boundary for the shared fake.

Resilience and Maintainability Implications

• observed — Normal queue claims use atomic row locking and a fresh lock token; release operations are token-scoped. Stalled-job recovery can make an interrupted invocation available again, so these controls do not establish exactly-once external email delivery. The queue mechanisms are unchanged by this PR.
• observed — Scheduled SES retries are restricted to named throttle errors. SendingPausedException and generic transport failures do not enter this branch, limiting the newly effective replay path rather than making all send failures retryable.

---

---

Pre-merge checks | 1

✅ Passed checks (1 passed)

Check name	Status	Explanation
Description check		The description includes all required sections and clearly explains the problem, in-process SES proxy changes, testing rationale, release status, documentation impact, and agent context. The test rati…

---

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

nodejs/tests/helpers/ses.ts (2)

31-36: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value

A rejected upstream call can write headers twice.

If handleRequest throws after response.writeHead, the catch handler calls writeHead(502) again. The second call raises ERR_HTTP_HEADERS_SENT inside the promise rejection handler. That error goes unhandled and can fail the Jest run. At present the code writes nothing before the throw at Line 132, so this needs a future change to trigger. The cheap fix is to check response.headersSent first.

Proposed fix

             void this.handleRequest(request, response).catch((error: Error) => {
+                if (response.headersSent) {
+                    response.destroy(error)
+                    return
+                }
                 response.writeHead(502, { 'content-type': 'application/json' })

---

53-57: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

stop() can hang on keep-alive sockets.

server.close() calls back only after every open connection ends. The SDK's HTTP handler keeps sockets alive. The tests call sesV2Client.destroy() before stop(), which should release them. A test that fails before that call can leave afterEach waiting until it times out. Call closeAllConnections() before close().

Proposed fix

         await new Promise<void>((resolve, reject) => {
             this.server.close((error) => (error ? reject(error) : resolve()))
+            this.server.closeAllConnections()
         })

Source: Learnings

---

ℹ️ Review info

⚙️ Run configuration

• Configuration used: Repository: PostHog/posthog/.coderabbit.yaml
• Review profile: QUIET
• Plan: Enterprise
• Run ID: 894e936d-f9b8-4014-a2b5-a4784b9ff47c

📥 Commits

Reviewing files that changed from the base of the PR and between f2fe805 and a9a434b.

📒 Files selected for processing (8)

• .github/workflows/ci-nodejs.yml
• docker-compose.base.yml
• docker-compose.dev.yml
• docs/published/handbook/engineering/developing-locally.md
• 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/tests/helpers/ses.ts

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

[webjunkie]

Note

Agent review, actively prompted for and steered.

Requesting changes from the DevEx side. The ses compose service is not necessary. The LocalSes proxy already does the work that the tests assert on.

Answer SendEmail in the proxy and drop the container

LocalSes.handleRequest already parses and records every SendEmailCommandInput in requests. It also injects the SESv2 errors itself. The container supplies only two things:

• A MessageId. One randomUUID() in the proxy gives the same result.
• The /store inbox. It returns the same input that the proxy already recorded. Most getEmails() assertions sit next to an ses.requests assertion that checks the same send.

The SDK still sends real HTTP requests to the proxy. Serialization, retries and error mapping stay covered. A client-level mock such as aws-sdk-client-mock would lose that, so the proxy is the right level.

The change is small and the PR loses a lot:

• the ses service in docker-compose.base.yml and docker-compose.dev.yml, with its health check and profile
• the ci-nodejs.yml probe for unrebased branches, and the compose files added to node_full, which force a full Node.js run on every compose edit
• the developing-locally.md paragraph and the manual docker compose ... up ses step
• the shared-inbox isolation logic and the unrelatedClient test, which exist only because tests share one inbox

The layers above this PR (#112820 through #113056) use the fake only through LocalSes, in email.service.ses.test.ts, cdp-cyclotron-worker-email.consumer.test.ts and workflows-e2e.serial.test.ts. Their call sites can stay, so the change reaches them on the next restack. This came up while reading #112820.

Follow the precedent from the LocalStack removal

#81257 removed LocalStack from the dev stack in August, which also removed the last SES emulator. That PR kept dedicated containers only where a real backend makes the decision under test (dynamodb-local for conditional writes). For KMS it added an in-process stub, nodejs/tests/session-replay/recording-api/stub-kms-client.ts. SES here matches the KMS case.

#81257 also documents why SES_ENDPOINT in nodejs/src/cdp/config.ts and posthog/settings/ses.py stays at localhost:4566. The port is dead on purpose. The SES client passes no explicit credentials, so the dead port keeps a local send off real AWS. This PR binds the fake to that port, so whether the port is dead now depends on whether the optional profile runs. An in-process proxy on port 0 leaves the guard as it is.

[coderabbitai]

🧹 Nitpick comments (1)

nodejs/tests/helpers/ses.ts (1)

68-69: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Validate the parsed request body shape.

parseJSON(...) as SendEmailCommandInput is an unchecked cast. A request whose body is not a JSON object, such as null, is pushed into requests. toEmail then throws on property access. The error path returns 500, but the invalid input stays in requests. This is test-helper code, and the SDK always sends objects. The risk is low.

---

ℹ️ Review info

⚙️ Run configuration

• Configuration used: Repository: PostHog/posthog/.coderabbit.yaml
• Review profile: QUIET
• Plan: Advanced
• Run ID: dbb085db-bb45-47a0-887f-60fa4cd6fddf

📥 Commits

Reviewing files that changed from the base of the PR and between a9a434b and 902b9fd.

📒 Files selected for processing (2)

• nodejs/src/cdp/services/messaging/email.service.ses.test.ts
• nodejs/tests/helpers/ses.ts

💤 Files with no reviewable changes (1)

• nodejs/src/cdp/services/messaging/email.service.ses.test.ts

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

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

[scheduled-actions-posthog]

This PR was closed due to lack of activity. Feel free to reopen if it's still relevant.