-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Add CHASM lifecycle paused state #10046
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1553,7 +1553,8 @@ func (n *Node) closeTransactionHandleRootLifecycleChange( | |
| var newState enumsspb.WorkflowExecutionState | ||
| var newStatus enumspb.WorkflowExecutionStatus | ||
| switch lifecycleState { | ||
| case LifecycleStateRunning: | ||
| case LifecycleStateRunning, LifecycleStatePaused: | ||
| // Paused is an OPEN state; the execution remains RUNNING from the persistence perspective. | ||
| newState = enumsspb.WORKFLOW_EXECUTION_STATE_RUNNING | ||
| newStatus = enumspb.WORKFLOW_EXECUTION_STATUS_RUNNING | ||
| case LifecycleStateCompleted: | ||
|
|
@@ -1856,6 +1857,16 @@ func (n *Node) validateTask( | |
| return false, err | ||
| } | ||
|
|
||
| // Paused components (and their non-detached sub-components) have all tasks invalidated. | ||
| // Component authors must re-emit tasks when transitioning back to running. | ||
| paused, err := n.isInPausedSubtree(validateContext) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hmm can we refactor the code a bit and check ancestor only once for both paused and access rule check? Both are based on ancestors' lifecycle state. Not for this PR: I think as a follow optimization for the close transaction code path, since we are already doing a prefix traversal of the tree, we know if a node is in the scope of a paused component or not and doesn't need to go back the ancestor nodes. I think this is doable by changing the andAllChildren iterator implementation to return more information.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. actually there's a problem for side effect tasks created before component is paused. When those tasks are executed, component may not be in paused state any more and can get executed. so we need to check if the corresponding logical task still exists for the component or not. |
||
| if err != nil { | ||
| return false, err | ||
| } | ||
| if paused { | ||
| return false, nil | ||
| } | ||
|
|
||
| defer log.CapturePanic(n.logger, &retErr) | ||
|
|
||
| return registableTask.validateFn( | ||
|
|
@@ -1867,6 +1878,60 @@ func (n *Node) validateTask( | |
| ) | ||
| } | ||
|
|
||
| // isInPausedSubtree returns true if this component node or any non-detached ancestor | ||
| // component is in the paused lifecycle state. Detached nodes do not inherit pause | ||
| // from their ancestors, matching the access-rule boundary semantics. | ||
| func (n *Node) isInPausedSubtree(ctx Context) (bool, error) { | ||
| if n.isDetached() { | ||
| // Detached nodes are isolated from ancestor pause state. | ||
| return false, nil | ||
| } | ||
|
|
||
| // Check non-detached ancestors first. | ||
| if n.parent != nil { | ||
| if paused, err := n.parent.isInPausedSubtreeHelper(ctx); err != nil || paused { | ||
| return paused, err | ||
| } | ||
| } | ||
|
|
||
| // Check this node itself. | ||
| if !n.isComponent() { | ||
| return false, nil | ||
| } | ||
| if err := n.prepareComponentValue(ctx); err != nil { | ||
| return false, err | ||
| } | ||
| comp, ok := n.value.(Component) //nolint:revive // unchecked-type-assertion | ||
| if !ok { | ||
| return false, nil | ||
| } | ||
| return comp.LifecycleState(ctx).IsPaused(), nil | ||
|
Comment on lines
+1897
to
+1908
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Isn't it cheaper to check if the components itself is paused first? |
||
| } | ||
|
|
||
| // isInPausedSubtreeHelper is the recursive ancestor-traversal helper for isInPausedSubtree. | ||
| // It checks whether this node (if a component) or any of its non-detached ancestors is paused, | ||
| // stopping the traversal at detached-component boundaries. | ||
| func (n *Node) isInPausedSubtreeHelper(ctx Context) (bool, error) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's the difference between this and the function above? From the comments, it seems they are the same. |
||
| // Traverse ancestors first, stopping at detached boundaries. | ||
| if !n.isDetached() && n.parent != nil { | ||
| if paused, err := n.parent.isInPausedSubtreeHelper(ctx); err != nil || paused { | ||
| return paused, err | ||
| } | ||
| } | ||
|
|
||
| if !n.isComponent() { | ||
| return false, nil | ||
| } | ||
| if err := n.prepareComponentValue(ctx); err != nil { | ||
| return false, err | ||
| } | ||
| comp, ok := n.value.(Component) //nolint:revive // unchecked-type-assertion | ||
| if !ok { | ||
| return false, nil | ||
| } | ||
| return comp.LifecycleState(ctx).IsPaused(), nil | ||
|
Comment on lines
+1922
to
+1932
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This looks the same as above. Refactor it. |
||
| } | ||
|
|
||
| func (n *Node) closeTransactionCleanupInvalidTasks( | ||
| validateContext Context, | ||
| ) (bool, error) { | ||
|
|
||
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.
Is it safe to simply uncomment this? This would change the values of the lifecycles below.