fix(triggers): make trigger occurrence insert idempotent (EN-1221)

[Quentin-David-24]

Jira: EN-1221 (component: Flows, epic EN-1217)

Problem

Seen in production: ExecuteTrigger stuck with the InsertTriggerOccurrence activity failing on a loop with

ERROR: duplicate key value violates unique constraint "triggers_occurrences_pkey" (SQLSTATE 23505)

InsertTriggerOccurrence (internal/triggers/activities.go) does a plain INSERT, and its activity options set only StartToCloseTimeout: 10s with no retry policy, so the SDK default applies: unlimited attempts, no schedule-to-close cap.

The occurrence id is part of the recorded activity input, so it is frozen in the workflow history. Once the row is committed but the result never reaches the server — the 10s timeout fires while the commit is in flight (slow DB, lock contention, connection-pool wait), or the worker is killed/rolled out between commit and ack — every subsequent retry replays the same id, can only ever fail on the primary key, and retries forever.

Knock-on effects:

• SendEventForTriggerTermination runs after the insert, so no SUCCEEDED_TRIGGER/FAILED_TRIGGER event is ever published for those runs.
• For SAVED_PAYMENT/SAVED_ACCOUNT the workflow id is deterministic with REJECT_DUPLICATE (listener.go), so a wedged run holds that id and silently swallows redeliveries for the same object.

This is the same failure mode InsertNewStage already guards against — it carries the comment "when a workflow times out this might already be present on retry" and an ON CONFLICT DO NOTHING (internal/workflow/activities.go). Triggers never got it.

Fix

• ON CONFLICT (id) DO NOTHING on the occurrence insert, so a retry after a lost ack is a no-op instead of a permanent failure.
• The occurrence id is now the workflow execution id rather than a uuid.NewString() minted in workflow code. That makes it replay-stable, which is what makes DO NOTHING correct rather than silently lossy — a random uuid in workflow code also meant the id published in the termination event could reference a row that does not exist.

The occurrence id is not part of the public API surface (the v1/v2 TriggerOccurrence schemas expose only date, triggerID, workflowInstanceID, workflowInstance, event, error), so the id format change is not API-visible. It does appear as id in the SUCCEEDED_TRIGGER/FAILED_TRIGGER event payloads, which are unconstrained strings.

Tests

New TestInsertTriggerOccurrenceIsIdempotent: calls the activity twice with the same occurrence and asserts no error plus a single row. Verified it reproduces the production error without the fix:

Error: Received unexpected error:
       ERROR: duplicate key value violates unique constraint "triggers_occurrences_pkey" (SQLSTATE 23505)

go test ./internal/... is green.

Scope

Deliberately narrow — only the wedging. EN-1221 also covers the broader at-least-once redelivery problem (deterministic workflow id for all event types, so a redelivery cannot replay side-effecting send stages); that part stays in #199, along with the equivalent ON CONFLICT for InsertNewInstance. This is the subset that unblocks stuck production workflows and is small enough to review on its own. Supersedes the occurrence-idempotency part of the closed #184.

Backport to a release branch on request — no released version (latest tag v2.6.3) has this fix.

[NumaryBot]

✅ Approve — automated review

The insert is now idempotent for replayed activity inputs, and the occurrence ID is stable across workflow replays. No blocking correctness issues were found.

No findings.

[shipfox-ai]

This PR makes the trigger-occurrence insert idempotent: the occurrence id is now the (deterministic, replay-stable) Temporal workflow execution id instead of a uuid minted in workflow code, and the insert gained ON CONFLICT (id) DO NOTHING so a Temporal activity retry after a lost ack is a no-op instead of failing forever on triggers_occurrences_pkey and wedging ExecuteTrigger. Both fix requirements and the new test match the PR body exactly, the signature change is complete (no stale NewTriggerOccurrence call sites), the public v1 TriggerOccurrence schema exposes no id, and the id-uniqueness assumptions hold under the current code paths. I verified the diff, the schema (triggers_occurrences PK is (id), internal/storage/migrations.go), and the id-provenance paths (internal/triggers/workflow_trigger.go, internal/triggers/listener.go). Recommendation: approve with comments — one minor, forward-looking note below; nothing blocks merge.

Standards

No confirmed material finding. The repo documents no coding standards applicable to internal/ code, and both candidate smell reports (adjacent string parameters on NewTriggerOccurrence, repetition of the conflict-ignore insert shape from InsertNewStage) are style judgement calls with no correctness, security, compatibility, or risky-test impact, and are mitigated by the diff's own comments and narrow scope. They are not retained.

Spec

1. Minor — DO NOTHING silently drops any id conflict, not just lost-ack replays (internal/triggers/activities.go:89, id sourced at internal/triggers/workflow_trigger.go:73-76). The occurrence id is WorkflowExecution.ID (the workflow id, not RunID — note the contrast with InsertNewStage, which keys on RunID). Today uniqueness holds: RunTrigger starts ExecuteTrigger children without an explicit WorkflowID, so Temporal generates a unique one per run, and the deterministic path in listener.go uses WORKFLOW_ID_REUSE_POLICY_REJECT_DUPLICATE, preventing a second run under the same id. But nothing enforces this: if a workflow-level retry policy, continue-as-new, or workflow-id reuse were ever introduced for ExecuteTrigger, a fresh logical occurrence would reuse the id, be silently dropped by the insert, and SendEventForTriggerTermination would still publish a SUCCEEDED_TRIGGER/FAILED_TRIGGER event referencing a row that does not exist. Worth an author comment or a follow-up guard (e.g. assert RowsAffected or key on RunID if that invariant ever changes). Not a defect in the current code; low likelihood.

Everything else in the PR body checks out: the ON CONFLICT clause, the execution-id occurrence id, the new TestInsertTriggerOccurrenceIsIdempotent (which correctly relies on the per-test isolated database from setupTestDB), and the deliberate carve-out of the deterministic-workflow-id/redelivery work to #199 (listener.go is untouched, as stated). No missing or partial requirements, no scope creep.

Reviewed independently by GLM (glm-5.3-flash) and DeepSeek (deepseek-v4-pro-0813) via Shipfox; verified and synthesized by GLM.

[laouji]

Will the workflow ID be reused on subsequent executions (see the docs here?

I'm not super familiar with the context of this workflow but the name suggests we might want a new row in the DB every time the workflow runs, so I'm a bit worried we might skip inserting rows when Temporal starts recycling workflow IDs.

Can we generate some truly unique identifiers based on maybe workflow.ID & workflow.RunID like elsewhere?