Skip to content

improvements on time-skipping task regeneration - #11404

Merged
feiyang3cat merged 3 commits into
temporalio:mainfrom
feiyang3cat:fix-timeskipping-task-refreshment
Aug 19, 2026
Merged

improvements on time-skipping task regeneration#11404
feiyang3cat merged 3 commits into
temporalio:mainfrom
feiyang3cat:fix-timeskipping-task-refreshment

Conversation

@feiyang3cat

@feiyang3cat feiyang3cat commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

What changed?

  1. perf improvement: no time-skipping task regen on full refresh
  2. new functional test: claim Nexus HSM timers are part of in-fight nexus operation, and won’t be skipped, add functional test
  3. new functional test: add a functional test to verify time skipping won't change retention time
  4. trivial bug fix: time-skipping: task regen didn’t read EnableWorkflowExecutionTimeoutTimer

Why?

  • correctness related No. 4 though it is an edgy case
  • perf related No.1
  • test coverage No.2,3

How did you test it?

  • built
  • run locally and tested manually
  • covered by existing tests
  • added new unit test(s)
  • added new functional test(s)

@feiyang3cat
feiyang3cat force-pushed the fix-timeskipping-task-refreshment branch 2 times, most recently from 5714420 to fe9e764 Compare August 3, 2026 19:12
@feiyang3cat feiyang3cat changed the title improvements on timeskipping task refreshment improvements on time-skipping task regeneration Aug 3, 2026
@feiyang3cat
feiyang3cat force-pushed the fix-timeskipping-task-refreshment branch 3 times, most recently from cf93b8b to 9a1ecea Compare August 3, 2026 21:32
@feiyang3cat
feiyang3cat marked this pull request as ready for review August 3, 2026 21:32
@feiyang3cat
feiyang3cat requested a review from a team August 3, 2026 21:32
@feiyang3cat
feiyang3cat requested review from a team as code owners August 3, 2026 21:32
@feiyang3cat
feiyang3cat marked this pull request as draft August 3, 2026 21:32
@feiyang3cat
feiyang3cat force-pushed the fix-timeskipping-task-refreshment branch 3 times, most recently from aee04c1 to 27fee51 Compare August 3, 2026 22:17
@feiyang3cat
feiyang3cat marked this pull request as ready for review August 3, 2026 22:17
@feiyang3cat
feiyang3cat force-pushed the fix-timeskipping-task-refreshment branch from 27fee51 to c7855bf Compare August 3, 2026 22:22
…skipping regen.

time-skipping: task regen didn't read EnableWorkflowExecutionTimeoutTimer
@feiyang3cat
feiyang3cat force-pushed the fix-timeskipping-task-refreshment branch from c7855bf to 4c6faef Compare August 17, 2026 17:47
// task the start path never generated.
executionTimeoutTimer := r.mutableState.GetExecutionInfo().WorkflowExecutionExpirationTime
if !timeNotSet(executionTimeoutTimer) {
if r.config.EnableWorkflowExecutionTimeoutTimer() && !timeNotSet(executionTimeoutTimer) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it ok that there are more conditions here (do we need anything else): https://github.com/temporalio/temporal/blob/main/service/history/workflow/task_generator.go#L161-L166

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@claude try to answer this question

@feiyang3cat feiyang3cat Aug 19, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the only missing ones are

  • !isFirstRun
  • executionInfo.WorkflowExecutionTimerTaskStatus == TimerTaskStatusNone

I thought they are not related to correctness just for perf refinement, but now I think we can simplify the check here to just checking WorkflowExecutionTimerTaskStatus == TimerTaskStatusCreated.
And I think we change the design here for execution timeout slightly from "checking the original logic" to "checking if there is an original timer for regeneration"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, looks good

@feiyang3cat
feiyang3cat force-pushed the fix-timeskipping-task-refreshment branch from 54522c0 to 77c74a0 Compare August 19, 2026 18:03
@feiyang3cat
feiyang3cat enabled auto-merge (squash) August 19, 2026 18:22
@feiyang3cat
feiyang3cat merged commit d0924bd into temporalio:main Aug 19, 2026
54 checks passed
davidporter-id-au pushed a commit to davidporter-id-au/temporal that referenced this pull request Aug 24, 2026
## What changed?

1. perf improvement: no time-skipping task regen on full refresh
2. new functional test: claim Nexus HSM timers are part of in-fight
nexus operation, and won’t be skipped, add functional test
3. new functional test: add a functional test to verify time skipping
won't change retention time
4. trivial bug fix: time-skipping: task regen didn’t read
EnableWorkflowExecutionTimeoutTimer

## Why?
- correctness related No. 4 though it is an edgy case
- perf related No.1 
- test coverage No.2,3

## How did you test it?
- [x] built
- [ ] run locally and tested manually
- [x] covered by existing tests
- [x] added new unit test(s)
- [x] added new functional test(s)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants