- 
                Notifications
    
You must be signed in to change notification settings  - Fork 1.2k
 
Validate activity task stamps before reuse #8536
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| 
               | 
          ||
| ai, ok := mutableState.GetActivityInfo(scheduledEventID) | ||
| if ok && ai.StartedEventId == common.EmptyEventID { | ||
| if ok && ai.StartedEventId == common.EmptyEventID && ai.GetStamp() == stamp { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Won't this fail for existing activity tasks that were generated without a stamp?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stamp was added to AI the same time they were added to the matching tasks. For tasks before that, the Stamp from AI would be 0 and same for the tasks. But I could add a defensive check to enforce only when both stamp are > 0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah okay, so this should be fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we decided to revert this because the additional check will cause the first task always be valid and that could cause head-of-queue blocking issue on standby side.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The way that this is implemented feels error prone to me. It's too easy to mutate the activity and forget to update the stamp. I skimmed the locations where the activity is updated in the code and I think we may have some edge cases that are not covered (need to double check). I would consider updating the stamp in MutableStateImpl.UpdateActivity for any field change that warrants updating it.
          
 The purpose of this validation is to avoid building up backlog with invalid tasks. There isn't much of negative consequence to mis-update the stamp for any update to activity, as long as the most common case of retry has the stamp updated that would serve the purpose. But if there is better way of doing this, we could improve later.  | 
    
          
 We should do better for when we port this to CHASM.  | 
    
0fe742b    to
    65cff5f      
    Compare
  
    
Summary
IsActivityTaskValidRequestand generated API stubsTesting
GOTOOLCHAIN=local go test -tags test_dep ./service/history/workflow -run TestRetryActivity(fails: go.mod requires go >= 1.25.0 and local toolchain is go1.24.3)https://chatgpt.com/codex/tasks/task_b_68faffae045c832c9d8220bcbe50bac7