fix(engine): populate external ids for durable task children inside of the existing trigger tx#4344
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a transactional consistency bug in durable task child-run triggering by ensuring external IDs (and the associated task events) are populated within the same database transaction as durable RUN event ingestion, preventing “ghost” skipped-child IDs when later steps fail and roll back.
Changes:
- Wrap
PopulateExternalIdsForWorkflowin an explicit transaction and introduce a tx-aware internal helper. - Thread the existing ingestion transaction into child external-id generation so task events and ingestion outcomes commit/rollback together.
- Remove the pre-ingestion external-id population call from the dispatcher path, relying on ingestion to do it atomically.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| pkg/repository/ids.go | Introduces tx-scoped external-id population and passes the ingestion tx through child ID generation. |
| pkg/repository/durable_events.go | Populates external IDs inside IngestDurableTaskEvent’s existing transaction for RUN events. |
| internal/services/dispatcher/server_v1.go | Removes redundant pre-ingestion external-id population so ingestion is the single atomic path. |
Comment on lines
+1067
to
+1069
| if populateErr := r.populateExternalIdsForWorkflow(ctx, tx, tenantId, opts.TriggerRuns.TriggerOpts); populateErr != nil { | ||
| return nil, fmt.Errorf("failed to populate external ids for workflow: %w", populateErr) | ||
| } |
abelanger5
approved these changes
Jul 2, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fixes an issue where we'd populate external ids (which also had the side effect of creating task events) in one tx, commit, and then begin a second tx to spawn the children. this meant that if we populated, committed, and then the second tx failed, we wouldn't roll back, and we'd get into a stuck state where we'd just hang because there would be a ghost id in the list of runs to skip, which would result in:
Type of change
Checklist
Changes have been:
🤖 AI Disclosure