-
Notifications
You must be signed in to change notification settings - Fork 261
[DRAFT/PROPOSAL] continue-as-new-suggested reason and signal #2082
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: master
Are you sure you want to change the base?
Conversation
e1db952
to
4bc0d82
Compare
case SignalTypeSuggestContinueAsNew: | ||
weh.workflowInfo.continueAsNewSuggested = true | ||
var reason enumspb.ContinueAsNewSuggestedReason | ||
err := weh.dataConverter.FromPayload(attributes.GetInput().GetPayloads()[0], &reason) |
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.
Bug: Signal Handling Accesses Unchecked Payload
In handleWorkflowExecutionSignaled
, processing SignalTypeSuggestContinueAsNew
directly accesses attributes.GetInput().GetPayloads()[0]
. This is unsafe as it doesn't validate if the input or payloads slice exists or has elements, potentially causing a runtime panic if the signal lacks expected payload data.
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.
Proposal idea makes sense (though unsure about the details), though it's a bit strange to propose on the Go SDK, not everyone reads the Go SDK (we have more popular SDKs). May be worth proposing the problem and possible solutions internally for general SDK discussion not specific to this one SDK/PR.
QueryTypeWorkflowMetadata string = "__temporal_workflow_metadata" | ||
|
||
// SignalTypeSuggestContinueAsNew is the signal name to set ContinueAsNewSuggested=true in the workflow env. | ||
SignalTypeSuggestContinueAsNew = "__suggest_continue_as_new" |
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 this a proposal to change every SDK to accept a special built-in signal name to suggest continue as new? I would recommend keeping the one canonical place to get suggest-continue-as-new, in the task event. And if we need an RPC call to have the server apply this manually instead of how it does today, we can add it I think. Is the whole reason to use a signal because 1) we're concerned we can't send an empty task, and 2) we're unwilling to make a continue-as-new-suggested event?
If we want to take an approach like this, I think we can make continue as new suggested something that can be updated via UpdateWorkflowExecutionOptions
(or separate RPC) and use WorkflowPropertiesModifiedExternallyEventAttributes
or WorkflowExecutionOptionsUpdatedEventAttributes
to relay the update to the SDK.
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.
Hey Chad, my intention was not for this to be ready for review as an actual ready-to-merge PR (it doesn't even have tests or a server implementation for the API change yet). I'm working on a blueprint for the PINNED_UNTIL_CONTINUE_AS_NEW
behavior / "Trampolining" feature and wanted to use this PR diff as a way to communicate my initial ideas for how it could be implemented in the SDKs.
I don't intend to skip cross-SDK design for this at all. In fact, this PR is still in the design phase. Sketching it out in Go felt like the most straightforward way to get a sense of whether the implementation was technically viable, since I'm not fluent enough in any of our other SDKs to sketch it there.
I thought marking it as DRAFT/PROPOSAL while I worked on some other things would sufficiently communicate that it wasn't ready for review, but next time I'll be more explicit in the PR description as well. Keep an eye out for an incoming https://github.com/temporalio/features/issues and request for comment on my blueprint!
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 reason to use a signal is to provide an API through which inactive workflows can be woken up so that they have the opportunity to continue-as-new onto a new worker version even if they are waiting on a timer or other condition.
Ideally this suggestion signal / empty task / API could be exposed to users so that they can apply this suggestion to workflows when the user's specific validation requirements are met for the new version if they choose.
Signal seems like a straightforward way to do that, but WorkflowExecutionOptionsUpdatedEventAttributes
+ scheduling an empty task could totally do the same thing, and I think is a great option that I hadn't thought of!
What was changed
Add a system-defined signal to set
workflowInfo.continueAsNewSuggested
to true. Add a reason enum so workflows can see why continue-as-new was suggested whether it came from a signal or from a workflow-task-started event as usual.Why?
So that workflows who want to upgrade their worker version via continue-as-new can find out about a new version change in their worker deployment.
Checklist
Closes
How was this tested:
Note
Expose reason for suggested continue-as-new and support a system signal to set it at runtime.
ContinueAsNewSuggestedReason
enum with reasons (history size, update registry, worker deployment change) and surface viaworkflow.GetWorkflowInfo().GetContinueAsNewSuggestedReason()
.WorkflowInfo
withcontinueAsNewSuggestedReason
and accessor; keepcontinueAsNewSuggested
.continueAsNewSuggested
and reason from server attributes.__suggest_continue_as_new
that setscontinueAsNewSuggested=true
and decodes reason payload to updateWorkflowInfo
.enumspb.ContinueAsNewSuggestedReason
to SDK enum.ContinueAsNewSuggestedReason
inworkflow/deterministic_wrappers.go
.go.temporal.io/api
tov1.55.1-0.20251017163919-d527807d0d4e
.Written by Cursor Bugbot for commit 4bc0d82. This will update automatically on new commits. Configure here.