Nexus workflow operation cancellation for CHASM#9915
Merged
bergundy merged 14 commits intotemporalio:mainfrom Apr 13, 2026
Merged
Nexus workflow operation cancellation for CHASM#9915bergundy merged 14 commits intotemporalio:mainfrom
bergundy merged 14 commits intotemporalio:mainfrom
Conversation
- Add ParentPtr mock support for testing (NewMockParentPtr) - Add Operation ParentPtr to Cancellation component for parent access - Improve cancellation state machine: set task Destination via event, move Attempt increment to scheduled transitions, track Failure on terminal states - Add cancelArgs, loadArgs, saveResult to Cancellation component - Add OnNexusOperationCancellationCompleted/Failed to OperationStore - Add comprehensive cancellation state machine tests
- Rename invocationResult to startResult, move to start_result.go - Move cancellationResult types to cancellation_result.go - Remove extracted types from task_handler_helpers.go
- Add OnNexusOperationCancellationCompleted/Failed to Workflow component to record history events when cancellation succeeds or fails - Simplify CancelRequestCompleted/Failed event handlers to use Get() instead of TryGet() (cancellation must be present) - Replace fmt.Sprintf error construction with serviceerror formatting functions (NewInternalf, NewNotFoundf)
- Extract nexusTaskHandlerBase with shared methods (lookupEndpoint, buildCallbackURL, setupCallContext, recordCallOutcome, newInvocation) - Add Cancel() to invocation interface with HTTP, system, and timeout implementations - Refactor operationInvocationTaskHandler to embed nexusTaskHandlerBase - Implement cancellationInvocationTaskHandler with full Validate/Execute - Implement cancellationBackoffTaskHandler with Validate/Execute - Use commonTaskHandlerOptions for backoff and timeout handlers
Extract invocation interface and implementations into invocation.go, base handler and options into task_handler_base.go, and move generateCallbackToken to operation_tasks.go.
bergundy
changed the title
stephanos
reviewed
Apr 12, 2026
Contributor
stephanos
left a comment
stephanos
left a comment
There was a problem hiding this comment.
I didn't get through the tests yet; I'll do that async after merge.
| if err != nil { | ||
| return fmt.Errorf("failed to construct invocation: %w", err) | ||
| } | ||
| startTime := args.currentTime |
Contributor
There was a problem hiding this comment.
operationInvocationTaskHandler is using
startTime := time.Now() // nolint:forbidigo // Time can be used for timing metrics.
here - should this, too?
| } | ||
| // Cancellation must be present to deliver a cancel request. | ||
| cancellation := field.Get(ctx).Cancellation.Get(ctx) | ||
| return nexusoperation.TransitionCancellationFailed.Apply(cancellation, ctx, nexusoperation.EventCancellationFailed{}) |
Contributor
There was a problem hiding this comment.
This seems to be missing the Failure field assignment.
| RetryPolicy: input.retryPolicy(), | ||
| }) | ||
| default: | ||
| return nil, serviceerror.NewInternalf("cannot save invocation result of type %T", r) |
Contributor
There was a problem hiding this comment.
Suggested change
| return nil, serviceerror.NewInternalf("cannot save invocation result of type %T", r) | |
| return nil, serviceerror.NewInternalf("cannot save cancellation result of type %T", r) |
| if errors.Is(callErr, errOpProcessorFailed) { | ||
| return "operation-processor-failed" | ||
| } | ||
| var opTimeoutBelowMinErr *operationTimeoutBelowMinError |
Contributor
There was a problem hiding this comment.
We could use `errors.AsType in this method instead.
| } | ||
| if args.scheduleToCloseTimeout > 0 { | ||
| callTimeout = min(callTimeout, args.scheduleToCloseTimeout-args.currentTime.Sub(args.scheduledTime)) | ||
| timeoutType = enumspb.TIMEOUT_TYPE_SCHEDULE_TO_CLOSE |
Contributor
There was a problem hiding this comment.
callTimeout and timeoutType are always overwritten when both timeouts are set?
| e.WorkerMayIgnore = true // For compatibility with older SDKs. | ||
| }) | ||
| return err | ||
|
|
| e.WorkerMayIgnore = true // For compatibility with older SDKs. | ||
| }) | ||
| return err | ||
|
|
bergundy
force-pushed
the
nexus-cancelation-task-handlers
branch
from
0843c2c to
06c95df
Compare
stephanos
approved these changes
Apr 13, 2026
simvlad
approved these changes
Apr 13, 2026
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
3 participants
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.
What changed?
Full implementation of the cancellation path for workflow Nexus operations on CHASM.
Including:
I also ended up doing quite a bit of refactoring to reuse as much of the code as possible across the cancellation and invocation executors.
Also added missing test coverage for the invocation executors.
I tried to structure this into separate commits that make reviews easy but ended up with a lot of fixes in later commits that touched code in earlier commits. Best to review in one piece.