Trampolining: addition of fields to remove the infinite loops for pinned wfs #721
Trampolining: addition of fields to remove the infinite loops for pinned wfs #721
Conversation
Shivs11
changed the title
Shivs11
force-pushed
the
ss/trampolining-infinite-loops
branch
from
776bba0 to
c69139f
Compare
Shivs11
changed the title
| // latest_target_worker_deployment_version on its versioning info. Used to | ||
| // detect whether the routing target has changed since this run started, | ||
| // preventing infinite continue-as-new loops for pinned workflows that do | ||
| // not have initial CaN behavior set as AutoUpgrade. Nil for fresh starts. |
There was a problem hiding this comment.
Maybe just explicitly say: "Nil for workflows that are not initiated by a versioned workflow continuing-as-new."
| // task, it compares the polling worker's deployment version against the task | ||
| // queue's routing target. If they differ, matching sends the routing target |
There was a problem hiding this comment.
nit: "the task queue's routing target" is not quite right, because "Target Worker Deployment Version" is a function of workflow_id, task_queue_routing_config, not just task_queue_routing_config. So I think it's best to just always say "Workflow's Target Version." We define that term in our docs, so we can just use it as is without explaining further imo
| // This field exists so that when a workflow continues-as-new, the new run | ||
| // can know what the routing target was at the end of the previous run. | ||
| // Specifically, it is copied into the new run's WorkflowExecutionStartedEvent, | ||
| // enabling the new run to detect whether the routing target has actually | ||
| // changed since it started — which is how we prevent infinite | ||
| // continue-as-new loops for pinned workflows that do not have initial | ||
| // CaN behavior set as AutoUpgrade. |
There was a problem hiding this comment.
I would consider just omitting this paragraph, but also ok to leave it.
If you keep it, please change "routing target" to "workflow's Target Version"
| // changed since it started — which is how we prevent infinite | ||
| // continue-as-new loops for pinned workflows that do not have initial | ||
| // CaN behavior set as AutoUpgrade. | ||
| temporal.api.deployment.v1.WorkerDeploymentVersion latest_target_worker_deployment_version = 9; |
There was a problem hiding this comment.
tbh since this is already in the context of "versioning info" we can probably just call it latest_target_version.
Also, I'm realizing that since VersioningInfo is an external API thing, if we want to put:
TargetVersionOnContinueAsNew version and InheritedPinnedVersionOnContinueAsNew bool in here, that would have to be in this API PR as well.
…arted event attributes
| // Target Version has changed since this run started, preventing infinite | ||
| // continue-as-new loops for pinned workflows that do not have initial CaN | ||
| // behavior set as AutoUpgrade. Nil for workflows that are not initiated by | ||
| // a versioned workflow continuing-as-new. |
There was a problem hiding this comment.
This field doesn't need to be looked at by SDK, correct? A line to that effect is always helpful in docstrings.
…_version docstring Clarifies that this field is server-internal and should not be read by SDKs, per reviewer feedback. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
| // Wrapper for the last Target Deployment Version that the server notified the SDK about | ||
| // via the targetDeploymentVersionChanged flag on a workflow task started event. | ||
| // See last_notified_target_version on WorkflowExecutionStartedEventAttributes. | ||
| message LastNotifiedTargetVersion { |
There was a problem hiding this comment.
fwiw: I was tempted to calling this NullableTargetVerison since we are using the message in the server repo here
however, chose against it for readability - @Sushisource / @ShahabT curious to hear how you folks think about this
There was a problem hiding this comment.
I think nullable is an odd name. This makes more sense to me.
| // Only populated by server with version >= 1.29.0. | ||
| bool eager_execution_accepted = 38; | ||
|
|
||
| // During the previous run of this workflow, if it exists, the server may have notified |
There was a problem hiding this comment.
During the previous run of this workflow, if it exists, the server may have notified
maybe remove the if it exists
| // | ||
| // Used internally by the server during continue-as-new and retry. | ||
| // Should not be read or interpreted by SDKs. | ||
| LastNotifiedTargetVersion last_notified_target_version = 40; |
There was a problem hiding this comment.
Here's how I think: previous run sends a declined_upgrade_for_target_version to the next run.
how previous run calculates this value at the CaN time:
- if prevMS.last_notified_target_version != nil -> that's the value.
- else prevMS.declined_upgrade_for_target_version is the value
how previous failed run calculates this value at retry:
- just pass starteventAttr.declined_upgrade_for_target_version
4 participants
READ BEFORE MERGING: All PRs require approval by both Server AND SDK teams before merging! This is why the number of required approvals is "2" and not "1"--two reviewers from the same team is NOT sufficient. If your PR is not approved by someone in BOTH teams, it may be summarily reverted.
-WISOTT
Breaking changes