edge case fixes for time skipping propagation and fast-forward completion - #11373
Merged
Merged
Conversation
feiyang3cat
force-pushed
the
vts-fix-propagation
branch
2 times, most recently
from
August 3, 2026 22:42
29a9c10 to
8e9ae5e
Compare
feiyang3cat
changed the title
feiyang3cat
changed the title
feiyang3cat
marked this pull request as ready for review
August 3, 2026 23:39
feiyang3cat
force-pushed
the
vts-fix-propagation
branch
from
August 3, 2026 23:39
8e9ae5e to
873b1e5
Compare
feiyang3cat
commented
Aug 3, 2026
| @@ -267,16 +276,16 @@ func (t *timeSkippingTransition) GateByFastForward(ff *persistencespb.FastForwar | |||
| return | |||
| } | |||
| ffTargetTime := ff.GetTargetTime().AsTime() | |||
| // If a real candidate is scheduled strictly before the fast-forward target, we skip to | |||
| // that and the fast-forward budget is not yet exhausted — leave time skipping enabled. | |||
| if !ffTargetTime.After(t.CurrentTime) { | |||
Contributor
Author
There was a problem hiding this comment.
for reviewers:
about the edgy case bug fix that ff completes during the transaction and Now() is after ff target time
simvlad
approved these changes
Aug 5, 2026
|
|
||
| // actions based on ff states: 1) disable time skipping 2) add ff timer task | ||
| if hasReached { | ||
| tsc.Enabled = false |
Contributor
There was a problem hiding this comment.
So FastForwardInfo is still set after time skipping is disabled. Do we always consult Enabled before returning FastForwardInfo (e.g. describe workflow execution call and others)
Contributor
Author
There was a problem hiding this comment.
- read API like describe workflow execution returns the time skipping state (enabled, fast-forward info, etc) regardless of time skipping is enabled or not.
So FastForwardInfo is still set after time skipping is disabled.right now the design is "we propagate both fast-forward config and fast-forward info thru the chain" so we don't differentiate the propagation on states. I am taking this path also kind of feeling this design is simpler than propagating different contents with different state?
| ms.executionInfo.TimeSkippingInfo.Config = config | ||
| // restart the skip session before (re)applying fast-forward so the fast-forward's max-skip |
Contributor
There was a problem hiding this comment.
nit: I don't think applyFastForward actually looks at SessionSkipCount at all
Contributor
Author
There was a problem hiding this comment.
you are right. this comment misleads.
| } | ||
| ms.executionInfo.TimeSkippingInfo = tsi |
yiminc
approved these changes
Aug 19, 2026
feiyang3cat
force-pushed
the
vts-fix-propagation
branch
from
August 19, 2026 21:20
d85d2e3 to
f19ad90
Compare
This was referenced Aug 19, 2026
davidporter-id-au
pushed a commit
to davidporter-id-au/temporal
that referenced
this pull request
Aug 24, 2026
…tion (temporalio#11373) ## What changed and why? 1. Edge case: If fast-forward completes during a workflow transaction but when time skipping checks at close transaction this fast-forward time is before ms.Now(), time skipping should still get disabled. Now we don't look at time points that are in the past and it is a bug. 2. Time-skipping propagation: Always propagate the time-skipping configuration and fast-forward state to the next run and other executions, regardless of whether time skipping is currently active. This ensures that read APIs (for example, Describe and PollFastForward) continue to return meaningful information instead of nil. - Otherwise, in an edge case where fast-forward completes in the first run and the user polls after the second run has become the current run of the workflow execution, the poll API would return a NotFound error instead of a completed poll result. - Similarly, the Describe API would return a nil configuration instead of the original configuration that should have been propagated. 3. Trivial changes: - simiplify parameter of `propagateTimeSkippingToNextRun` - unify UT names of timeskipping_test.go
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 and why?
Edge case: If fast-forward completes during a workflow transaction but when time skipping checks at close transaction this fast-forward time is before ms.Now(), time skipping should still get disabled. Now we don't look at time points that are in the past and it is a bug.
Time-skipping propagation: Always propagate the time-skipping configuration and fast-forward state to the next run and other executions, regardless of whether time skipping is currently active. This ensures that read APIs (for example, Describe and PollFastForward) continue to return meaningful information instead of nil.
Trivial changes:
propagateTimeSkippingToNextRun