Repository navigation
feat(cdp): park a transactional send until its scheduled_at - #128881
trunk-io[bot] merged 35 commits into
Conversation
Both transactional email routes take an optional scheduled_at with an explicit offset. A future time up to 30 days after the message's creation parks the job on the email queue's fast lane and answers scheduled; a time that has passed sends now; a later time answers 400 invalid_scheduled_at and frees the idempotency key. The fingerprint and the stored key change only when scheduled_at is set, so keys of a send now keep their shape across versions. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
🤖 CI report✅ Trunk lane — non-backend laneThis 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) — cleanNew 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) — cleanNew 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 — 0% of added code lines are comments (1 of 381)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 Files with the most added comment lines:
This check does not block merging. It updates on every push and clears when the share drops. |
There was a problem hiding this comment.
PostHog Review · Standard · reviewed 42f9b66: 3 Should fix.
This is the only automatic review while this pull request is a draft. Comment @posthog review to review new changes sooner. Otherwise, a review of the new changes runs when you mark it ready for review.
See this review and its findings in PostHog Review. Comment @posthog review --deep for a Deep review. A Deep review takes longer and looks deeper than a Standard review.
A scheduled transactional send recorded its whole parked wait in cdp_email_queue_to_send_ms, so every one landed past the top bucket of the fast lane. The accepted send now keeps its scheduled time in the invocation state, the email step copies it, and the email worker starts the wait at the later of the job's creation and that time. Also share the reservation release between the time refusal and the other refusals, and build the replayed key result in one place. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…sends/n3-scheduled-at
…sends/n3-scheduled-at
…ks of transactional emails
|
Risk: No findings This increment only deletes five explanatory comments from the Sentinel reviewed |
…sends/n3-scheduled-at # Conflicts: # nodejs/src/cdp/cdp-api.serial.test.ts # nodejs/src/cdp/services/hogflows/transactional-sends.ts
…nds/n3-scheduled-at The queued and scheduled rows now carry the recipient and the workflow, written from TransactionalSends, where the send logic moved.
The scheduled row, and the row a replay of it writes, carry the recipient and scheduled_at, so the list of waiting sends can read email_status_events. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…sends/n3-scheduled-at # Conflicts: # nodejs/src/cdp/cdp-api.ts # nodejs/src/cdp/services/hogflows/transactional-sends.ts
The janitor sweeps attachment rows older than seven days, but a send can be scheduled up to 30 days ahead. Such a send then failed with attachments_missing when it came due. The sweep now skips rows whose job is still available or running, the guard that delete() already uses. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A retry of the first request wrote a new scheduled row with the first time. After a reschedule or a cancel, that newer row hid the real state from a reader of the latest row per message. A replay of a send now writes no row either. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…sends/n3-scheduled-at # Conflicts: # nodejs/src/cdp/services/hogflows/transactional-sends.ts
Rows of scheduled sends that still wait stay past the max age, so the sweep now walks the table one batch at a time behind a cursor instead of scanning every retained row on each run. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…t is due A key lived 24 hours after cdp-api accepted the send, so a retry of a send scheduled further out queued a second copy for the same time. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…sends/n3-scheduled-at Kept the scheduled status row (scheduled_at on it) over N2's queued-only row, and the attachments switch from master now runs before the scheduled_at cap. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
PostHog Review · Standard · reviewed d01f5a1: 1 Should fix.
This is review round 3 on this pull request. When findings keep coming back in the same code, the approach is usually the problem. Before the next fix, check whether a different design would remove the whole class of findings.
…scheduled-at Master removed the email attachments side table and its janitor sweep (#131567, #131643), so the cursor sweep this layer added and its tests go with it. A scheduled send keeps its scheduled time and fingerprint; it carries no files. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The scheduled send tests now write through the Postgres job queue and read the stored job and what an email worker can claim, instead of the arguments a mocked queue received. The key retention test reads replays through reserve and finds the stored key by a scope of its own, instead of the private key builder. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
PostHog Review · Standard · reviewed 427cfb0: 1 Should fix.
This is review round 4 on this pull request. When findings keep coming back in the same code, the approach is usually the problem. Before the next fix, check whether a different design would remove the whole class of findings.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…scheduled-at # Conflicts: # nodejs/src/cdp/cdp-api.serial.test.ts # nodejs/src/cdp/cdp-api.ts # nodejs/src/cdp/services/hogflows/idempotency-keys.test.ts # nodejs/src/cdp/services/hogflows/idempotency-keys.ts # nodejs/src/cdp/services/hogflows/transactional-sends.ts
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… the wait clock Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Not approved — this change needs a human reviewer.
Re-add the stamphog label to request another review once you have addressed this.
Owning-team authorship and strong familiarity provide assurance for the risky changes, but substantive reviewer concerns remain unresolved in the checkout. Address the listed comments or obtain independent acceptance from @PostHog/team-workflows; Harley Alexander and Daniel Marchuk are suggested reviewers.
- Author wrote 82% of the modified lines and has 21 merged PRs in these paths (familiarity STRONG).
- @PostHog on nodejs/src/cdp/services/hogflows/transactional-sends.ts: status retention remains limited to 30 days in products/messaging/backend/models/email_status_sql.py and transactional_email_status.py, so sends due near the scheduling limit become unreadable at delivery. Address the retention concern before re-requesting.
- @PostHog on nodejs/src/cdp/services/hogflows/transactional-sends.ts: the Django delivery-status enum still excludes scheduled, and get_email_status rejects unknown statuses. The author's proposed later stack does not fix the reviewed checkout; update the reader or obtain independent acceptance of this sequencing before re-requesting.
- @PostHog on nodejs/src/cdp/services/messaging/email-status.service.ts: the checked-out ClickHouse status schemas and event-view projection do not retain the emitted due time. Land and verify the prerequisite schema migration, or obtain independent acceptance of that dependency, before re-requesting.
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 181L, 10F substantive, 467L/15F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1d-complex (467L, 15F, single-area, feat) |
| stamphog 2.8.0 | .stamphog/policy.yml @ e20bb3f · reviewed head e20bb3f |
|
This pull request was merged into |
Deploy status
This table shows the |
Problem
scheduledAt. PostHog's transactional email send has no way to wait, so they need a timer of their own (Silthus/personal#303, spec Silthus/personal#302).Note
🔵 Merge risk: low · two-way door until Django exposes
scheduled_atto customersscheduled_at, in US and EU, go out as before: the live run and the N2 worker cross-version run cover them, with and without files.scheduled_atchanges, and none does until Django's flagged stack ships. No stored job carries the new key (prod-shape, both regions).Merge first: #127745, which adds the recipient to status rows, then #128871 and #128868.
Landing: #129915 merges alone and its migration runs in US and EU first. #127745 merges next. Then one
/trunk mergeon #128929 lands N1 to N4.Changes
scheduled_atwith an offset. No caller sends it until Django's stack ships behind a flag.fastemail lane and answers202 { status: "scheduled", scheduled_at }.scheduled_at: null.400 invalid_scheduled_at, reasontoo_far, and frees the idempotency key.scheduled_atis set, so the deployed version reads every key.scheduledagain. It writes no status row, so a later move or cancel stays the latest row.scheduledtakes status rank 0, besidequeued.scheduledrow inemail_status_eventsnames the recipient, the workflow and the due time (scheduled_at, from #129915). The waiting-sends list reads it.email_statusandGET emails/{id}need no change.How did you test this code?
email.service.test.tsfails if the wait starts at the insert.71598b4on this branch's worker and the base branch's worker:Live run and cross-version results at e351094
Real cdp-api, queue, email worker, maildev and S3 store. The base is N2 at a2dd902.
completed.scheduled_at: null.replayed: true; another time422 idempotency_key_reused; one job.400; the key is freed.Not run: a rolling deploy on real infrastructure. The base answers a
scheduled_atby sending at once; no caller sets it until Django's stack ships.Release status
Docs update
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Agent: Claude Code, Claude Opus 5.5
Skills:
/writing-pr-descriptions,/writing-tests,/writing-code-commentsMore context for AI agents
scheduled_at,nullfor a send now. Django reads onlyinvocation_idandreplayedtoday, so the field is additive. The outcome metric keepsqueuedfor a scheduled send, because alerts key on it. The per-item comments are dropped at the author's request; the CI report's comment density is 0%.state.scheduledAtwith the cyclotronscheduledcolumn. The Django stack must change three readers before any caller can schedule:EmailDeliveryStatusgainsscheduled, so the status read stops answering404while a send waits; the email log also picksscheduledrows, not onlyqueuedones; and the status read's age gate moves from 30 to 60 days, so a send scheduled 30 days ahead stays readable after it goes out.TransactionalSends.acceptand the stored key shape inidempotency-keys.ts.🤖 Generated with Claude Code