Replace PauseState flag with PAUSE_REQUESTED state machine state#10265
Open
dandavison wants to merge 3 commits into
Open
Replace PauseState flag with PAUSE_REQUESTED state machine state#10265dandavison wants to merge 3 commits into
dandavison wants to merge 3 commits into
Conversation
5 tasks
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Reviewed by Cursor Bugbot for commit 3b0b77a. Configure here.
| a.emitOnUnpausedMetrics(event.metricsHandler) | ||
| return nil | ||
| }, | ||
| ) |
There was a problem hiding this comment.
TransitionUnpausedToStarted doesn't clear PauseState field
Low Severity
TransitionUnpausedToStarted (PAUSE_REQUESTED → STARTED) only emits metrics but never sets a.PauseState = nil. The sibling TransitionUnpaused (PAUSED → SCHEDULED) clears PauseState via the unpause() helper. After unpausing a PAUSE_REQUESTED activity, stale pause metadata (identity, reason, time, request ID) remains in the persisted ActivityState proto despite the activity no longer being paused.
Reviewed by Cursor Bugbot for commit 3b0b77a. Configure here.
dandavison
force-pushed
the
feature/activity-operator-cmds-pause-requested
branch
from
3b0b77a to
e28990c
Compare
Eliminate the dual representation of "paused" (status PAUSED vs. PauseState flag on STARTED/SCHEDULED) by introducing PAUSE_REQUESTED as a real internal status. Status becomes the single source of truth for whether the activity is paused; PauseState remains as audit data (identity, reason, time, request_id) but no longer drives behavior. Key changes: - Add ACTIVITY_EXECUTION_STATUS_PAUSE_REQUESTED to the proto enum. - New TransitionPauseRequested (STARTED -> PAUSE_REQUESTED), no stamp bump. - New TransitionUnpauseRequested (PAUSE_REQUESTED -> STARTED). - New TransitionRescheduledPaused (PAUSE_REQUESTED -> PAUSED) for the retry- while-paused path; eliminates the zone-2 (SCHEDULED + flag) intermediate. - Add PAUSE_REQUESTED to source sets of Completed, Failed, Terminated, CancelRequested, TimedOut. - validateActivityTaskToken accepts PAUSE_REQUESTED so the worker's token stays valid while paused-while-running. - Drop the PauseState == nil check from the dispatch and schedule-to-start task validators; their pause guard is now redundant with the status check. - Heartbeat response: ActivityPaused = (status == PAUSE_REQUESTED). - buildActivityExecutionInfo: drop the synthetic PAUSE_REQUESTED runState derivation; the status maps directly via internalStatusToRunState. - handleReset: PAUSE_REQUESTED is treated like STARTED (defer via flags). Drops the SCHEDULED+PauseState special case. - Remove the post-terminal PauseState = nil cleanups; PauseState is now audit data and may persist into terminal states. Tests not updated.
dandavison
force-pushed
the
feature/activity-operator-cmds-pause-requested
branch
from
e28990c to
3d8f7cf
Compare
Adds three sub-tests to TestPauseActivityExecution that exercise the PAUSE_REQUESTED + worker-does-not-yield interaction. Each was missing before; verified by reverting the corresponding fix and observing the specific test fail: - StartToCloseTimeoutWhilePauseRequested: Pause a STARTED activity; the worker stops responding; the StartToCloseTimeoutTask must still fire (validator must accept PAUSE_REQUESTED) and the retry must land in PAUSED. - HeartbeatTimeoutWhilePauseRequested: Same pattern for HeartbeatTimeoutTask. - UpdateOptionsPreservesTimeoutsWhilePauseRequested: Pause a STARTED activity, call UpdateActivityExecutionOptions with a shorter StartToCloseTimeout; the handler must re-emit a fresh timeout task for PAUSE_REQUESTED activities, not only STARTED/CANCEL_REQUESTED. Otherwise the stamp bump silently strips timeout enforcement from the running worker.
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
1 participant
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.
This is a proposed change to #10106.
See https://github.com/temporalio/temporal/pull/10106/changes#r3238854955
Note
Medium Risk
Changes core activity state machine and task validation around pausing, retries, and timeouts; subtle regressions could leave activities stuck or incorrectly timed out/canceled. Covered by new standalone activity tests, but behavior changes touch multiple transitions and token validation paths.
Overview
Refactors activity pausing to use a new internal status,
ACTIVITY_EXECUTION_STATUS_PAUSE_REQUESTED, instead of relying onPauseStateas a flag while the activity isSTARTED.Updates transitions and validators so
PAUSE_REQUESTEDkeeps worker tokens/attempt-scoped timers valid (heartbeat/start-to-close, completion/failure/cancel paths), and so retries/timeouts while pause-requested land the activity inPAUSEDrather than re-dispatching. API surfaces are adjusted accordingly (PendingActivityStatemapping,DescribeActivityExecutionrun state, andRecordActivityTaskHeartbeat.ActivityPaused).Adds standalone activity tests covering start-to-close timeout, heartbeat timeout, and options updates while
PAUSE_REQUESTED, and updates cancel/unpause expectations under the new state model.Reviewed by Cursor Bugbot for commit 055b7ed. Bugbot is set up for automated code reviews on this repo. Configure here.