Skip to content

Commit

Permalink
Fix flaky TestWorkflowTaskRedirectInRetryFirstTask (#6269)
Browse files Browse the repository at this point in the history
## What changed?
<!-- Describe what has changed in this PR -->
Do not update assigned build ID after AddWorkflowTask if the wft is
already started.

## Why?
<!-- Tell your future self why have you made these changes -->
This addresses an edge case in which the first wft of the workflow is
started (but not completed) and there happens to be another wft in the
transfer queue for the same execution which gets backlogged in matching.
When matching backlogs that task, it returned the assigned build ID and
history updates the MS build ID overriding the update made by the task
that was started. This should not be allowed.

## How did you test it?
<!-- How have you verified this change? Tested locally? Added a unit
test? Checked in staging env? -->
Existing tests.

## Potential risks
<!-- Assuming the worst case, what can be broken when deploying this
change to production? -->
None.

## Documentation
<!-- Have you made sure this change doesn't falsify anything currently
stated in `docs/`? If significant
new behavior is added, have you described that in `docs/`? -->
None.

## Is hotfix candidate?
<!-- Is this PR a hotfix candidate or does it require a notification to
be sent to the broader community? (Yes/No) -->
No.
  • Loading branch information
ShahabT authored Aug 26, 2024
1 parent 7103b82 commit afe9c0a
Show file tree
Hide file tree
Showing 2 changed files with 3 additions and 4 deletions.
5 changes: 3 additions & 2 deletions service/history/worker_versioning_util.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import (
"go.temporal.io/api/serviceerror"
persistencespb "go.temporal.io/server/api/persistence/v1"
taskqueuespb "go.temporal.io/server/api/taskqueue/v1"
"go.temporal.io/server/common"
"go.temporal.io/server/common/log"
"go.temporal.io/server/common/log/tag"
"go.temporal.io/server/common/metrics"
Expand Down Expand Up @@ -196,8 +197,8 @@ func initializeWorkflowAssignedBuildId(
return err
}

if mutableState.HasCompletedAnyWorkflowTask() {
// workflow has already completed a wft. buildId is stale and useless.
if mutableState.HasCompletedAnyWorkflowTask() || workflowTask.StartedEventID != common.EmptyEventID {
// workflow task is running or already completed. buildId is potentially stale and useless.
// workflow's assigned build ID should be already updated via RecordWorkflowTaskStarted
return nil
}
Expand Down
2 changes: 0 additions & 2 deletions tests/versioning.go
Original file line number Diff line number Diff line change
Expand Up @@ -945,7 +945,6 @@ func (s *VersioningIntegSuite) firstWorkflowTaskAssignmentSyncMatch() {
s.waitForChan(ctx, failedTask)

// MS should have the correct build ID
s.waitForWorkflowBuildId(ctx, run.GetID(), run.GetRunID(), v1)
s.validateWorkflowBuildIds(ctx, run.GetID(), run.GetRunID(), v1, true, "", "", nil)

// v2 times out the task
Expand Down Expand Up @@ -1452,7 +1451,6 @@ func (s *VersioningIntegSuite) testWorkflowTaskRedirectInRetry(firstTask bool) {
expectedStampBuildId = v1
}
// MS should have the correct build ID
s.waitForWorkflowBuildId(ctx, run.GetID(), run.GetRunID(), v1)
s.validateWorkflowBuildIds(ctx, run.GetID(), run.GetRunID(), v1, true, expectedStampBuildId, "", nil)

// v11 times out the task
Expand Down

0 comments on commit afe9c0a

Please sign in to comment.