Skip to content

refactor(cdp): extract TransactionalEmail.accept from the send handler - #133888

Closed
mayteio wants to merge 4 commits into
posthog/cyclotron-idempotency-keysfrom
posthog/cdp-intake-transactional-email-accept
Closed

refactor(cdp): extract TransactionalEmail.accept from the send handler#133888
mayteio wants to merge 4 commits into
posthog/cyclotron-idempotency-keysfrom
posthog/cdp-intake-transactional-email-accept

Conversation

@mayteio

@mayteio mayteio commented Nov 11, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Problem

Reviewers of the coming transactional email intake work (ticket, spec IN2 and IN5) would read new behavior mixed into a 250-line route handler.

Merge #132891 first. This PR stacks on it, then rebases onto master.

Warning

🟡 Merge risk: medium · two-way door (merge-risk rubric)

  • Every transactional send runs the moved code; a slip could block retries or send twice.
  • Limited by the unchanged route suite and a matching shadow diff.
  • Revert: git revert the merge commit.

Changes

Pure move: review with move detection on (git diff --color-moved=dimmed-zebra --color-moved-ws=allow-indentation-change, or GitHub's moved-lines view).

  • Mechanical move; callers see no change.
  • TransactionalEmail.accept holds everything after the JWT check. The route maps its typed outcome to the response.

How did you test this code?

Test rationale: none needed. The route suite covers every outcome and passes unchanged.

Evidence
  • Route suite on HEAD: 290 of 290 passed. Typecheck and lint passed.
  • Shadow diff: the route suite ran on the base and on HEAD with a hook that records every HTTP response. Both runs gave 386 responses, 268 of them from transactional email tests. After masking values that differ run to run (durations, random hashes, ids, signed-URL fields), the two sets were identical.
  • One difference outside responses: the validation-failure warning now logs team_id as a number, not the raw path string.
  • CI report, comment density: 17 of the 20 counted comment lines moved with their code unchanged. The new ones say why CdpApi builds TransactionalEmail per request, and why the attachment deadline starts at the request.
  • Not checked: a live run against a deployed stack. The change moves code and alters no runtime behavior.

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

Skills: /writing-code-comments, /writing-pr-descriptions

Question Answer Who Where
Where do lookups run? Inside accept; validation precedes them today 🤖 Agent ticket
When is it built? Per request; tests swap collaborators on CdpApi 🤖 Agent ticket

Involvement: design pass (gpt-6.1-sol, clean); shadow diff captured.

More context for AI agents
  • Session: Claude Code in T3 Code, stack thread for the ticket above.
  • Follow-ups: PR 2 moves the lifecycle row, the job insert and its rollback for both routes into nodejs/src/cdp/intake/.
  • Read closely: transactionalEmailResponse in cdp-api.ts, which maps each outcome back to the response the base sent.

🤖 Generated with Claude Code

mayteio and others added 4 commits November 11, 2026 17:07
@mayteio
feat(cdp): store idempotency keys in the cyclotron database
515be7f 
Adds a cyclotron_idempotency_keys table and a manager method that writes the key and its job in one transaction, reporting created, replayed or mismatch per job. The janitor sweeps keys after they expire. Nothing calls the method yet; the transactional send API switches over in a follow-up.

Refs #132733

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Generated-By: PostHog Desktop
Task-Id: 289e441f-3aec-4099-9f50-734327e0e97f
@mayteio
fix(cdp): close idempotency key races found in review
4a6f087 
- Lowercase and sort ids before claiming, so Postgres's lowercase RETURNING matches and every call locks key rows in the same order.
- Do not reclaim an expired key while its job still exists, and keep such keys in the sweep, so a repeat cannot hit the job's primary key.
- Sweep with FOR UPDATE SKIP LOCKED and recheck expiry in the outer DELETE, so the janitor neither waits on a writer nor deletes a key a writer just renewed.
- Require unique ids per call, which removes the in-batch duplicate mapping.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Generated-By: PostHog Desktop
Task-Id: 289e441f-3aec-4099-9f50-734327e0e97f
@mayteio
chore(cdp): trim idempotency key comments to the reasons
0c488db 
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Generated-By: PostHog Desktop
Task-Id: 289e441f-3aec-4099-9f50-734327e0e97f
@mayteio @claude
refactor(cdp): extract TransactionalEmail.accept from the send handler
5827b5f 
The send route keeps the JWT check and the mapping from the outcome to
HTTP. TransactionalEmail.accept holds validation, the lookups, the run,
the idempotency reservation, the refusals, the file checks and the queue
insert. No behavior changes: the route tests pass unchanged.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@mayteio mayteio self-assigned this Nov 11, 2026
@github-actions

github-actions Bot commented Nov 11, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

🤖 CI report

✅ Trunk lane — non-backend lane

This PR is assigned to the non-backend lane. It does not run backend Python tests and may merge in parallel with PRs in other lanes.

✅ Duplication (Python) — clean

New Python code duplication introduced by this branch. Fails at 70+ tokens in app code, or 150+ tokens when both copies live in test files. Advisory while the gate proves itself: extract a shared helper instead of copying.

✅ Duplication (TypeScript) — clean

New TypeScript code duplication introduced by this branch. Fails at 70+ tokens in app code, or 150+ tokens when both copies live in test files. Advisory while the gate proves itself: extract a shared helper instead of copying.

⚠️ Comment density — 3% of added code lines are comments (20 of 572)

This section warns when comments are more than 3% of the code lines a PR adds, and alerts above 6%. Before agent-assisted PRs, the typical share was about 2%. Only full-line comments count. Docstrings, generated files, snapshots, migrations, and workflow files are left out.

Comments that restate the code, record how the change came about, or narrate the next line add noise for the next reader. Keep the comments that explain a reason the code cannot show, and remove the rest. See .agents/skills/writing-code-comments/SKILL.md for the house rules.

Files with the most added comment lines:

File Comment lines Added lines
nodejs/src/cdp/services/hogflows/transactional-email-accept.ts 18 512
nodejs/src/cdp/cdp-api.ts 2 60

This check does not block merging. It updates on every push and clears when the share drops.

posthog[bot]
posthog Bot reviewed Nov 11, 2026
View reviewed changes

@posthog posthog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PostHog Review · Standard · reviewed 5827b5f: no findings.

@mayteio
mayteio added this pull request to stack #133955 November 11, 2026 19:51
@mayteio mayteio mentioned this pull request Nov 11, 2026
Merged
@mayteio
mayteio force-pushed the posthog/cyclotron-idempotency-keys branch 2 times, most recently from 0f0be76 to 3ef1d1e Compare November 12, 2026 10:15
@mayteio

mayteio commented Nov 12, 2026

Copy link
Copy Markdown
Contributor Author

Closing: master's #128871 already extracts the send route's acceptance into TransactionalSends.accept, which is what this PR did. The rest of the stack moves onto that module.

@mayteio mayteio closed this Nov 12, 2026
@mayteio
mayteio removed this pull request from stack #133955 November 12, 2026 11:12
@mayteio mayteio mentioned this pull request Nov 12, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@posthog posthog[bot] posthog[bot] left review comments

Assignees

@mayteio mayteio

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@mayteio