fix(triggers): make event processing idempotent under redelivery and retries (EN-1221) - #184
fix(triggers): make event processing idempotent under redelivery and retries (EN-1221)#184flemzord wants to merge 1 commit into
Conversation
…retries The event bus is at-least-once. Previously only SAVED_PAYMENT/SAVED_ACCOUNT got a deterministic, dedup-ing Temporal workflow id; every other event type ran with a server-generated id, so a redelivery (or a partial-failure NACK after some triggers had already started) re-executed triggers and replayed side-effecting stages such as money movements. - listener: derive a deterministic workflow id for ALL events (taskIDPrefix-triggerID-<objectID|msg.UUID>) with REJECT_DUPLICATE, so a redelivery is rejected as a duplicate instead of starting a second run. (H1) - occurrence id: ExecuteTrigger built the occurrence with uuid.NewString() in workflow code, yielding a different id on every Temporal replay. Use the (deterministic) workflow execution id instead. (M4) - insert activities: InsertNewInstance, InsertNewStage and InsertTriggerOccurrence now use ON CONFLICT DO NOTHING. With deterministic primary keys, a retry after a lost ack (row committed, result lost) would otherwise fail forever on the duplicate key and wedge the workflow. (M2) Adds a redelivery regression test for a non-payment event.
|
Warning Review limit reached
More reviews will be available in 32 minutes and 28 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
flemzord
changed the title
| instance := NewInstance(activity.GetInfo(ctx).WorkflowExecution.ID, workflowID) | ||
| // Idempotent: the primary key is the (deterministic) workflow execution id, | ||
| // so a Temporal retry after a lost ack must not fail on the duplicate key. | ||
| // The returned instance is rebuilt deterministically, so it is correct even |
There was a problem hiding this comment.
On the conflict path this does not actually return the row that was inserted by the first activity attempt; it returns a freshly rebuilt Instance. NewInstance uses time.Now(), and NewStage below does the same for StartedAt, so after a lost activity ack + retry the workflow can continue with newer timestamps and later UpdateInstance/UpdateStage can overwrite the original created_at/started_at. To make the idempotent path correct, please return the existing row on conflict (for example with a follow-up SELECT when RowsAffected == 0, or a RETURNING-based upsert) instead of returning the newly constructed struct.
|
Superseded by #199, which consolidates this change with the related reliability and safety fixes on top of the current main branch. |
1 participant
Problem (H1 + M2 + M4)
The event bus is at-least-once.
SAVED_PAYMENT/SAVED_ACCOUNTgot a deterministic, dedup-ing Temporal workflow id (listener.go:30-51,155-159, commented "Quick hack"). For every other event typeExecuteWorkflowran with a server-generated id. A redelivery — or a partial-failure NACK after some triggers in the loop had already started — re-executed triggers and replayed side-effectingsendstages (real ledger/wallet/PSP movements).ExecuteTriggerbuilt the occurrence withuuid.NewString()in workflow code (workflow_trigger.go:71), producing a different occurrence id on every Temporal replay → publishedSUCCEEDED/FAILED_TRIGGERevents could reference an id absent from the table.InsertNewInstance,InsertNewStage,InsertTriggerOccurrencedid plainINSERTs with deterministic PKs and unlimited retries. A retry after a lost ack (row committed, result lost) fails forever on the duplicate key → wedged workflow.Fix
taskIDPrefix-triggerID-<objectID│msg.UUID>withREJECT_DUPLICATE.msg.UUIDis preserved across redeliveries, so a redelivery is rejected as a duplicate.ON CONFLICT DO NOTHING(deterministic PKs make this safe; the returned struct is rebuilt deterministically).Tests
New
TestHandleMessage/redelivery of a non-payment event is skipped(samemsg.UUIDdelivered twice ⇒ a single occurrence). Existing SAVED_PAYMENT dedup and multi-trigger tests still pass.Severity: HIGH (H1) + MEDIUM (M2, M4) — grouped by root cause (idempotence).