Repository navigation
feat(cdp): cancel or move a scheduled send before a worker claims it - #128929
trunk-io[bot] merged 43 commits into
Conversation
New cdp-api routes cancel or reschedule a scheduled transactional email while its job waits unclaimed (available, transition_count 0). They verify a team-only JWT of a new audience on the transactional email secret. Cancel writes the canceled run row and status row; reschedule moves the job and its state's scheduledAt and writes scheduled again, bounded by the message's creation plus 30 days. An overwriteExisting upsert leaves a job canceled before any claim alone, so a rerun cannot bring it back. 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 — 2% of added code lines are comments (22 of 1096)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 66423db: 2 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.
…nceled result The manager's rescheduleUnclaimed takes only the new time and writes the state's scheduledAt from it, so no caller can let the two drift. The API cancel and the hogflow worker build the canceled run's result with one function, and the API cancel loads the workflow so its metrics carry the workflow version. The message id check uses the shared UUID validator. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ca miss A send scheduled moments ago may not be on the read replica yet, so the API cancel reads the workflow as the send route does. The cancel test also covers a lookup that fails: the cancel still answers 200 and writes its rows. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Semgrep's hardcoded-jwt-secret rule flags a literal secret in jwt.sign. The manage-token helper now takes the local dev secret as a defaulted parameter, as the send-token helper in the same file does. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The cancel and reschedule routes had no test for a forged signature, unlike the other scoped-token routes. The token table gains that row, which also gives the helper's secret option its caller. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Each producer method added to the interface had to be copied into thirteen mock literals, which the CI report flagged as duplication. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The canceled lifecycle row took the cancel time as its scheduled_at, so history filtered to the send's day still showed the run as running. It now takes the scheduled time of the send's running row. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…4-cancel-reschedule
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…4-cancel-reschedule
|
Risk: No findings This increment rewrites a canceled send's status rows when a cancel request repeats: the manager's already-canceled fallback now returns the job row when it was canceled before any claim (transition_count 0), and the service re-records the canceled run without double-counting metrics. The changes are correctly team-scoped and token-pinned; I found no new security issues. Sentinel reviewed |
…4-cancel-reschedule # Conflicts: # nodejs/src/cdp/services/hogflows/transactional-email.ts # nodejs/src/cdp/services/hogflows/transactional-sends.ts # nodejs/src/cdp/services/messaging/email-status.service.ts
The waiting-sends list reads the latest status row per message, so the canceled row and the rescheduled row name the recipient and the flow, and the rescheduled row carries the new due time. rescheduleUnclaimed returns the job it moved, as cancelUnclaimed does. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…4-cancel-reschedule # Conflicts: # nodejs/src/cdp/cdp-api.serial.test.ts # nodejs/src/cdp/consumers/cdp-cyclotron-worker-hogflow.consumer.ts # nodejs/src/cdp/services/hogflows/transactional-sends.ts
…4-cancel-reschedule
No worker sees a send canceled before its claim, so the worker's delete of its attachment rows never ran, and the rows stayed until the janitor sweep. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A status row's version has millisecond precision, so two reschedules whose lock handoff fell in one millisecond tied, and either row could win. Each reschedule now stores its status time in the job state and takes a time at least one millisecond after the previous one. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The send timed its scheduled status row by the API host's clock and stored nothing, so a first reschedule timed by a lagging database clock could write a lower version and leave the old time showing. The send now stores that time in the job, and every reschedule stays at least 1 ms after the stored time. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…reschedule Master removed email attachment storage (#131567), so a canceled scheduled send has no stored files to delete. The cancel no longer calls the attachments service, and its test goes with it. The hogflow consumer keeps master's removal of its attachment delete; the canceled-result builder lives in invocation-utils as this layer moved it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…reschedule # Conflicts: # nodejs/src/cdp/cdp-api.serial.test.ts
The cancel and reschedule tests now send through the Postgres job queue, act through the routes, and read the stored job, what an email worker can claim, and the rows written to the outputs. They no longer replace the job producer with canned answers or assert on its calls. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A reschedule can move a send up to the schedule bound, so the idempotency key now lives until a day after that bound instead of a day after the first scheduled time. A retry of the first request keeps replaying while the send waits. The lifecycle test now reschedules before the cancel and observes the real Kafka producer, so it proves the canceled row keeps the running row's scheduled time. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…reschedule # Conflicts: # nodejs/src/cdp/cdp-api.serial.test.ts # nodejs/src/cdp/cdp-api.ts # nodejs/src/cdp/services/hogflows/idempotency-keys.ts # nodejs/src/cdp/services/hogflows/transactional-sends.ts # nodejs/src/common/api/middleware/internal-api-auth.ts
…reschedule # Conflicts: # nodejs/src/cdp/services/hogflows/idempotency-keys.ts
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…reschedule # Conflicts: # nodejs/src/cdp/cdp-api.serial.test.ts
… move it to A send accepted to go out at once can still be rescheduled while no worker has claimed it, so its files are kept as long as a scheduled send's. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…reschedule # Conflicts: # nodejs/src/cdp/services/hogflows/transactional-email.ts
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A cancel that commits and then loses its process before the canceled status and run-history rows go out left the send reading scheduled and running. A repeat cancel now writes both rows again while the job row exists, without counting metrics a second time. A job a worker canceled after its claim keeps answering canceled and writes nothing. 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-adding the stamphog label gives the same result unless the changed files change.
stamphog can't approve this pull request on its own, because two gates refused it. The deny-list gate flagged it for matching the auth category, which fits the new JWT audience and the changes to jwt-utils.ts and internal-api-auth.ts. The tier gate classified it as T2-never (1429 lines across 17 files, a feature), a class that always needs a human reviewer. The size gate passed, so splitting the PR won't clear either refusal. Please ask a human reviewer, ideally someone who owns the auth and cyclotron code, to review it.
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✗ | matches: auth |
| size | ✓ | 500L, 13F substantive, 1429L/17F incl. docs/generated/snapshots — within ceiling |
| tier | ✗ | classified as T2-never: T2-never (1429L, 17F, single-area, feat) |
| stamphog 2.8.0 | .stamphog/policy.yml @ unknown · reviewed head 75f0ddb |
|
/trunk merge |
|
😎 Stack merged successfully - details. |
Deploy status
This table shows the |
Problem
Warning
🟡 Merge risk: medium · two-way door until Django exposes cancel and reschedule to customers
503 delivery_unavailableand frees the key; added accept latency at volume is not measured. No stored job carries files in either region (prod-shape).statusAt; the N3 cross-version run and the shadow diff show N3 reads those jobs and nothing else changes.Rollout
cdp_transactional_email_requests_total{outcome="delivery_unavailable"}; no alert pages on it.Merge first: #128881 (with #127745 under it), #128871, then #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
200 canceled, also on a repeat;409 already_sendingafter a claim; else404, also for a job id on a queue other than email.canceledstatus row and run row again, without metrics, so a retry after a crash between the commit and those rows still shows the email canceled.200 scheduled; a past time sends now; past 30 days from creation400;409once claimed or canceled.200and the409s hold while the job row exists; after the janitor deletes it,404.canceledstatus row and the run's terminal row, at the time of the run'srunningrow, so history shows the run canceled on the day it was listed.canceledrow and a reschedule's newscheduledrow name the recipient and the workflow, and the new row carries the newscheduled_at, so the latest row per message is enough for the waiting-sends list.scheduledrow takes its status time from the database right after the row lock, not from when the row is written, and at least one millisecond after the job's previousscheduledrow. The send and each reschedule keep that row's time in the job state, so a lagging database clock cannot put the first reschedule behind the send.overwriteExistingupsert leaves a job canceled before any claim alone, so a rerun cannot revive it while its row exists. After that, a manual rerun can (spec, out of scope).Note
The rerun premise holds (2026-11-04): a rerun upserts with
overwriteExisting, so the guard is needed. Details below.How did you test this code?
transition_countstands for a claim; team scoping; the upsert guard.Live run, cross-version and shadow diff at fd4106b, and the retried cancel at 75f0ddb
Real cdp-api, queue, email worker, maildev and S3 store. The base is N3 at e351094.
200 canceledboth times; acanceledstatus row; no mail.409 already_sending.404.401.400; exactly 30 days is accepted.statusAt, the longer file keep, the key lifetime, thecanceledrank and the upsert guard.scheduledand the runrunning; the retry answers200and writes bothcanceledrows.Not run: a rolling deploy on real infrastructure, and a rollback. A replay with the original body after a move or a cancel answers the original
scheduledreply, as any idempotent replay does.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
rescheduleUnclaimedlocks the row (FOR UPDATE, noSKIP LOCKED), decodes the JSON state in JS and writes it back, because a jsonb rewrite in SQL fails on a null character in email content. A cancel keepstransition_count 0, which marks the row for the upsert guard. A non-UUID message id answers 404 before any query. Reschedule takes only the time and writes the state'sscheduledAtfrom it. The API cancel and the workflow worker share one canceled-result builder, and the cancel loads the workflow (primary on a replica miss) so its metrics carry the version. A repeat cancel rewrites its rows only for a job canceled before any claim, so a worker's cancel after a claim writes nothing new (the author's choice for a PostHog Review finding, 2026-11-12). The per-item comments are dropped at the author's request; the CI report's comment density is 2%, and the remaining lines moved from master, such as the canceled-result builder's block.sendunless the request setsreplay_code_sends. feat(messaging): send an email with inline html from code #127123 sets it from the code-send flag, so with the flag on a code send reruns through the upsert, and atemplate_idsend reruns either way.team_id,message_idandactionclaims, maps the answers, and addscanceledandscheduledtoEmailDeliveryStatus(Django stack). The waiting-sends list reads the latestemail_status_eventsrow per message (Django stack). A moved send that later sends writes its terminal row at the new time, as a fetch retry does today. The history query filtersscheduled_atbefore it collapses rows, so the two days can disagree; a separate change can fix the query.cancelUnclaimed,rescheduleUnclaimedand the upsert guard inmanager.ts.🤖 Generated with Claude Code