fix task token invalidation after resetting an activity#11097
Conversation
df1919e to
b2be8eb
Compare
| if token.Attempt != ByIDTokenAttempt && token.Attempt != a.LastAttempt.Get(ctx).GetCount() { | ||
| return serviceerror.NewNotFound("activity task not found") | ||
| } | ||
| if token.GetActivityAttemptStamp() != 0 && token.GetActivityAttemptStamp() != a.LastAttempt.Get(ctx).GetStartedStamp()+1 { |
There was a problem hiding this comment.
During rollout upgrade, we should handle the old activity state that doesn't yet have StartedStamp. We could assume it's 0 for protobuf default, and handle it like:
| if token.GetActivityAttemptStamp() != 0 && token.GetActivityAttemptStamp() != a.LastAttempt.Get(ctx).GetStartedStamp()+1 { | |
| startedStamp := a.LastAttempt.Get(ctx).GetStartedStamp() | |
| // A zero started stamp indicates state persisted before this field was introduced. | |
| if token.GetActivityAttemptStamp() != 0 && startedStamp != 0 && token.GetActivityAttemptStamp() != startedStamp+1 { |
There was a problem hiding this comment.
We shoud test out this case an update happens and doesn't bump StartStamp and only Stamp
Specifically
- Start + poll an activity so the worker holds a token
- UpdateActivityExecutionOptions on the running activity
- Assert the Stamp changed ( and not StartStamp)
- With the original token: RecordActivityTaskHeartbeat and RespondActivityTaskCompleted → both succeed
dandavison
left a comment
There was a problem hiding this comment.
Nice catch! I can't think of a better solution.
| componentRef := task.event.GetData().GetComponentRef() | ||
| activityAttemptStamp := int32(0) | ||
| if len(componentRef) > 0 { | ||
| activityAttemptStamp = task.event.Data.GetStamp() + 1 |
There was a problem hiding this comment.
It would be nice if we can avoid doing the +1 comparison: the logic is split over History and Matching, and recapitulated in tests. Can you construct a test that fails if we do not do it?
There was a problem hiding this comment.
also it feels a little weird that setting activityAttemptStamp is conditional on a different field.. what happens if we just do this all the time? non-chasm activity tokens get slightly larger?
| @@ -1804,6 +1804,9 @@ func (a *Activity) validateActivityTaskToken( | |||
| if token.Attempt != ByIDTokenAttempt && token.Attempt != a.LastAttempt.Get(ctx).GetCount() { | |||
| return serviceerror.NewNotFound("activity task not found") | |||
| } | |||
There was a problem hiding this comment.
After the transitional phase of rollout, would we drop the naive attempt comparisons? Maybe leave a comment explaining that the attempt stamp comparison is sufficient now.
There was a problem hiding this comment.
We have to wait until all activities created without this stamp are finished which would require a full table on every cell
| s.LessOrEqual(expectedRange, s.taskManager.getQueueDataByKey(tlID).rangeID) | ||
| } | ||
|
|
||
| func (s *matchingEngineSuite) TestCreatePollActivityTaskQueueResponse_StandaloneAttemptStamp() { |
There was a problem hiding this comment.
@dandavison this is the test that validates the +1 is needed, it will fail if that is removed.
There was a problem hiding this comment.
Looks like this test verifies that the +1 happens, but doesn't show that it's needed.
| // The attempt stamp captured when the current worker started this attempt. | ||
| // Unlike stamp, this does not change on options updates while the worker owns the token. | ||
| int32 started_stamp = 14; |
There was a problem hiding this comment.
put this field right under stamp so readers will see them both next to each other and it's easier to compare+contrast the semantics.
also, what does "worker owns the token" mean?
| componentRef := task.event.GetData().GetComponentRef() | ||
| activityAttemptStamp := int32(0) | ||
| if len(componentRef) > 0 { | ||
| activityAttemptStamp = task.event.Data.GetStamp() + 1 |
There was a problem hiding this comment.
also it feels a little weird that setting activityAttemptStamp is conditional on a different field.. what happens if we just do this all the time? non-chasm activity tokens get slightly larger?
| if len(componentRef) > 0 { | ||
| activityAttemptStamp = task.event.Data.GetStamp() + 1 | ||
| } | ||
| activityAttemptStamp := task.event.Data.GetStamp() + 1 |
Implements Pause, Unpause, Reset, and UpdateActivityExecutionOptions RPCs for standalone CHASM activities. Adds matching frontend and history service handlers in chasm/lib/activity that dispatch to either the workflow-activity path or the standalone-activity path based on the presence of a workflow ID. Also adds PAUSE_REQUESTED and RESET_REQUESTED state machine states for standalone activities, the original_options snapshot for restore-on-reset semantics, the activity-options merge helpers (common/activityoptions), and the supporting proto schema changes (start_request_id, sdk_name, sdk_version, etc.). Squashed PRs: #9693 RPC boilerplate and code generation #9850 Implement UpdateActivityExecutionOptions #9851 Implement Pause/UnpauseActivityExecution for SAA #9852 Implement ResetActivityExecution for SAA #10001 Pause returns FailedPrecondition when already paused #10025 Validator function fixes #10265 Replace PauseState flag with PAUSE_REQUESTED state #10364 Add RESET_REQUESTED state to standalone activity #10745 Add support for updating start_delay via UpdateActivityOptions #10913 Preserve SAA retry backoff across unpause re-dispatch #10914 Fix SAA unpause for keep-paused reset #10917 Reject stale legacy SAA schedule-to-close timers #10920 Preserve NextRetryDelay across unrelated activity option updates #10921 Validate merged SAA retry policy updates #10923 Fix SAA reset attempt dispatch time reporting #11010 Use default retry policy when updating with nil #11020 Fix: de-duplicate pause even if activity is unpaused #11013 Fix: reject restore_original when a standalone activity has no original options snapshot #11032 Reject unsupported and empty update_mask paths in UpdateActivityExecutionOptions #11030 Fix: describe only provides heartbeat/failure when opted-in #11016 Preserve pending activity next retry delay #11068 Add SAA operator APIs to DC redirection whitelist #10954 Remove reset_heartbeats field in ResetActivityExecutionRequest #11087 Fix standalone activity heartbeat flag precedence #11117 Validation to reject negative activity jitter #11120 Fix bogus StartToCloseLatency for non-running standalone activities #11097 Fix task token invalidation after resetting an activity #11067 SAA bug fix: update start delay while paused #11123 SAA: cleanup
What changed?
Added a monotonic activity attempt stamp (
activity_attempt_stamp) to task tokens for standalone activities. CHASM activity task-token validation checks that non-legacy stamped tokens match the current attempt stamp, so a token from before an attempt reset cannot heartbeat or complete the reset attempt after the visible attempt counter is rewound.Workflow activity behavior is unchanged: workflow and by-ID synthesized activity tokens pass
0for the new field, preserving existing validation semantics.Why?
Standalone activity reset can rewind the visible attempt count back to
1. Without a monotonic generation in the task token, a stale worker holding an older attempt-1 token can later match the reset attempt-1 state and incorrectly heartbeat or complete obsolete work.The new stamp fences those stale tokens while keeping existing/pre-upgrade tokens usable. A missing stamp decodes as
0and skips the new check for backward compatibility.How did you test it?
Potential risks
This change is intended to be wire-compatible: older tokens decode with
activity_attempt_stamp = 0, and older servers should ignore the unknown field. During rolling upgrades, stale-token protection only applies when a stamped token is validated by upgraded CHASM activity code. Existing in-flight tokens remain accepted by design.