Skip to content

fix(cdp): keep the idempotency key of a rescheduled send - #134438

Merged
trunk-io[bot] merged 14 commits into
masterfrom
fix/send-reschedule-idempotency-expiry
Nov 12, 2026
Merged

trunk-io[bot] merged 14 commits into
masterfrom
fix/send-reschedule-idempotency-expiry

Conversation

@Silthus

@Silthus Silthus commented Nov 12, 2026 •

Copy link
Copy Markdown
Contributor

Problem

  • An email sent now with an idempotency key, then moved days ahead, can go out twice: the key expires after 24 hours, so a later retry queues a second send.
  • Found in review of #128929. The reschedule route has no caller yet.

Note

🟢 Merge risk: low · two-way door

  • Keyed sends store their key's replay record in job state as an optional field. Older pods ignore it, so a revert leaves inert data.
  • Ordinary workflow snapshots keep that record beside the variables they already keep. Sensitive ones drop it, live since #134624.
  • A reschedule that cannot extend the key keeps the move and answers 503, so the caller can retry it.
  • Only reschedule reads it. No Python or frontend code calls the reschedule route or mints its token yet.
  • The record a reschedule writes has the same format and 31-day ceiling that master already writes for a send scheduled at accept.
  • Webhook ingestion shares the completion script; its pending hold is always shorter than 24 hours, so its TTL is unchanged.
  • Deploy order: #134624 is deployed in both regions, so no worker keeps this record in a sensitive snapshot, before or after a revert.
  • Switch-off: none: live on deploy, fix forward.

Changes

  • A reschedule writes the send's replay record until a day after the latest time it can be moved to, as for a send scheduled at accept.
  • That write also covers a key whose queue completion was late, lost, or never stored.
  • A late completion keeps the longer window instead of resetting it to 24 hours.
  • Unmoved sends keep 24 hours, so the documented "a retry after 24 hours sends again" still holds.
  • When the key store fails or is not configured, the reschedule keeps the move and answers 503, so the caller retries it.
  • Workflow snapshots of sensitive messages drop the replay record, which hashes the message variables.
  • A replay still returns the first response, scheduled time included.

How did you test this code?

Test rationale: the serial test ages real Valkey TTLs and retries over HTTP, with rows on both sides of the window. Unit rows fail a completion that waits, loses its answer, or dies with its process, with or without its hold expiring. Fault rows fail the store, or leave it unconfigured, during the reschedule.

  • Live run: on a dev box with real Postgres and Valkey, 218 transactional email tests and 52 idempotency key and snapshot tests pass. That includes HTTP sends, reschedules, and retries after aged TTLs.
  • Prod-shape, cross-version: not run. The job state field is optional, older readers ignore it, and reschedule behaves as before without it.

Decisions

Question Answer Who Where
How long a keyed send's key lasts Only a reschedule extends it 🤖 comment
Where a reschedule's replay record comes from The job state, written back by the reschedule 🤖 comment
What a replay returns after a reschedule The first response 🤖 comment
What a reschedule answers when it cannot extend the key 503, with the move kept 🤖 comment
A key a newer send owns Left as it is, the reschedule succeeds 🤖 comment
When the snapshot strip ships First, in #134624, deployed before this PR merges 🤖 comment
Prod-shape and cross-version proofs Not run, readers handle both shapes 🤖 comment

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-tests, /writing-pr-descriptions

Security review: not needed

More context for AI agents
  • Session: T3 Code side-fix thread from the work on fix(cdp): gate new scheduled, batch and webhook runs on workflow quota #127105.
  • Decisions: extend on reschedule instead of keeping every key 31 days, which breaks the 24-hour contract for unmoved sends. The reschedule rewrites the full queued record from job state, so no pending record can hold a key for weeks.
  • Read closely: MARK_QUEUED_SCRIPT and replayUntilDayAfter in idempotency-keys.ts, and ScheduledTransactionalSends.reschedule.
  • Worth: build, duplicate-send class.

🤖 Generated with Claude Code

Silthus and others added 2 commits November 12, 2026 11:10
A send accepted to go out now kept its idempotency key for 24 hours, but a
reschedule can move it up to 30 days from creation before a worker claims it.
A retry of the first request after a day then queued a second send while the
first one still waited. Every keyed send now keeps its key until a day after
the latest time a reschedule can move it to, as its files already are.

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

Keeping every keyed send's key for 31 days would break the documented
contract that a retry after 24 hours sends again. A send now records its
key's hash in the job state, and a reschedule extends that key, while it
still holds the send, until a day after the latest time the send can be
moved to. Sends that are never moved keep the 24-hour window.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@Silthus Silthus self-assigned this Nov 12, 2026
@trunk-io

trunk-io Bot commented Nov 12, 2026 •

Copy link
Copy Markdown

😎 Merged successfully - details.

@github-actions

github-actions Bot commented Nov 12, 2026 •

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.

@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 2ef1e68: 1 Should fix.

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 completion that waited for the key store, or whose answer was lost,
rewrote the key with its original 24-hour TTL after a reschedule had
lengthened it. The completion now keeps the longer TTL, and a reschedule
also lengthens a key whose completion is still pending.

Adds tests for the window's upper bound and for a reschedule while the
key store is down.

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

@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 f1b2f1d: 1 Should fix.

Lengthening a pending reservation kept it pending for weeks when the
process that should complete it died, so retries got in-flight instead
of a replay. Accept now stores the key's queued record in job state, and
a reschedule writes that record with the longer window, so the key
replays whether its completion landed, is late, or was lost.

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

@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 3d7f77e: no findings.

Silthus and others added 3 commits November 12, 2026 11:48
…schedule

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

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

Silthus commented Nov 12, 2026 •

Copy link
Copy Markdown
Contributor Author

Note

🤖 Claude Opus 5.5 responding on behalf of Michael

  • 🤖 How long a keyed send's key lasts Only a reschedule extends it, to a day after the latest time the send can be moved to. Unmoved sends keep 24 hours, so the documented contract holds.
  • 🤖 Where the replay record comes from on a reschedule The job state keeps the record from accept, and the reschedule writes it back, so a late, lost, or expired completion still replays.
  • 🤖 What a replay returns after a reschedule The first response, scheduled time included, as any idempotent replay does.
  • 🤖 What a reschedule answers when it cannot extend the key 503, with the move kept, when the key store fails or is not configured. A retry of the reschedule then extends the key.
  • 🤖 A key a newer send owns Left as it is, and the reschedule succeeds. Another owner exists only after this send's key expired and a new request reused it.
  • 🤖 When the snapshot strip ships First, in fix(cdp): strip the idempotency key record from sensitive message snapshots #134624, deployed before this PR merges, so an older worker or a revert never copies the record into a sensitive snapshot.
  • 🤖 Prod-shape and cross-version proofs Not run: the new state field is optional and both old and new readers handle its absence, and nothing calls the reschedule route yet.

@Silthus
Silthus marked this pull request as ready for review November 12, 2026 13:14
@stamphog
stamphog Bot requested a review from a team November 12, 2026 13:14
@parameterai

parameterai Bot commented Nov 12, 2026 •

Copy link
Copy Markdown

Risk: No findings

This delta adds a 'triggered' app metric and a non-awaited metrics flush to the hogflow scheduled-invocation handler, plus tests. All metric fields are server-derived and team-scoped, and the flush swallows its own produce errors, so the change introduces no new attacker-reachable surface.

Sentinel reviewed d91e216 · Review settings

@veria-ai

veria-ai Bot commented Nov 12, 2026 •

Copy link
Copy Markdown
Contributor

PR overview

All previously flagged issues have been addressed. No open security concerns remain on this pull request.

Fixed/addressed: 1 · PR risk: 0/10

Reviews

@posthog-security-review-bot posthog-security-review-bot Bot added the security-review Request a security review label Nov 12, 2026
@posthog-security-review-bot

posthog-security-review-bot Bot commented Nov 12, 2026 •

Copy link
Copy Markdown

✅ Security review complete — 3e431b7d1c1d

2 findings posted as a review, in 8 min.

Add the security-review label to run again on the latest commit.

@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 3e431b7: no findings.

stamphog[bot]

This comment was marked as outdated.

The record's request fingerprint is an unkeyed hash of the variables, so
a lifecycle snapshot that strips a sensitive message's variables must
strip it too.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@stamphog
stamphog Bot dismissed their stale review November 12, 2026 13:22

New commits were pushed — dismissing the stamphog approval from an earlier head. Re-add the trigger label to request a fresh review.

@posthog-security-review-bot posthog-security-review-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Agent-driven security review - findings inline.

@posthog-security-review-bot posthog-security-review-bot Bot removed the security-review Request a security review label Nov 12, 2026

@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 507331a: no findings.

A 200 told the caller the move was safe while the key kept its shorter
window, so a later retry of the send could queue a duplicate. The move
stays committed, and a retry of the reschedule writes the key.

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

@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 f233f1c: 2 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.

… send's key

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

@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 966842c: 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.

…t a key write

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

@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 845623b: 1 Must fix, 1 Should fix.

This is review round 5 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.

@Silthus Silthus added the security-review Request a security review label Nov 12, 2026
@posthog-security-review-bot

posthog-security-review-bot Bot commented Nov 12, 2026 •

Copy link
Copy Markdown

✅ Security review complete — eaa9e00d0465

1 finding posted as a review, in 7 min.

Add the security-review label to run again on the latest commit.

@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 eaa9e00: no findings.

@posthog-security-review-bot posthog-security-review-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Agent-driven security review - findings inline.

@posthog-security-review-bot posthog-security-review-bot Bot removed the security-review Request a security review label Nov 12, 2026
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@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 0ce5910: no findings.

@stamphog stamphog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Approved.

The risky changes have a clean current-head agent review, owning-team authorship, and STRONG familiarity. No concrete unresolved substantive concerns or policy refusal grounds are present in the supplied context and checked code.

  • Author wrote 81% of the modified lines and has 32 merged PRs in these paths (familiarity STRONG).
  • PostHog Review (Standard) reviewed the current head: no findings.
  • posthog[bot] reviewed the current head.
Gate mechanics and policy version
Gate Result
prerequisites ✓ all clear
deny-list ✓ no deny categories matched
size ✓ 97L, 3F substantive, 380L/5F incl. docs/generated/snapshots — within ceiling
tier ✓ T1-agent / T1d-complex (380L, 5F, single-area, fix)
stamphog 2.8.0 .stamphog/policy.yml @ 0ce5910 · reviewed head 0ce5910

@Silthus

Silthus commented Nov 12, 2026

Copy link
Copy Markdown
Contributor Author

/trunk merge

@trunk-io
trunk-io Bot merged commit b7db6b4 into master Nov 12, 2026
207 of 208 checks passed
@trunk-io
trunk-io Bot deleted the fix/send-reschedule-idempotency-expiry branch November 12, 2026 19:47
@deployment-status-posthog

deployment-status-posthog Bot commented Nov 12, 2026 •

Copy link
Copy Markdown

Deploy status

Environment Status Deployed At Workflow
dev ✅ Deployed 2026-11-12 20:34 UTC Run
prod-us ✅ Deployed 2026-11-12 20:58 UTC Run
prod-eu ✅ Deployed 2026-11-12 21:04 UTC Run

This table shows the posthog release only. Hogstation shows every service that this PR reaches, Kargo services included.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant