fix: trimming timers on workflow close

[davidporter-id-au]

What changed?

draft fix for #7568

Why?

How did you test it?

Potential risks

Release notes

Documentation Changes

[gitar-bot]

Code Review Update: Most Issues Resolved ✅

I've re-reviewed the PR after applying the fix. Here's the current status:

✅ Resolved Issues:

1. Early return bug in deleteWorkflowTimerTasksBestEffort - Fixed (uses continue)
2. Incomplete syncExecutionInfoWithTasks coverage - Fixed (now covers all execution methods)
3. Incomplete documentation comment - Fixed
4. ShardID in DeleteTimerTaskRequest - Not an issue (shard context already available)

⚠️ Remaining Minor Issue:

• TaskCleanupTimeoutThreshold config is defined but not used in the cleanup logic
  • Documentation suggests it's for filtering short-lived workflows
  • Implementation is missing, but this is low priority for a draft PR

The critical bugs have been resolved. The PR now properly tracks workflow timer tasks across all execution lifecycle operations (create, update, conflict resolution) and will clean them up when workflows close.

[gitar-bot]

Fixed Critical Bug: Timer Task Tracking in Updates

I've fixed the issue where syncExecutionInfoWithTasks was only called for CreateWorkflowExecution, but not for UpdateWorkflowExecution and ConflictResolveWorkflowExecution.

Changes Made

Added:

• New function syncExecutionInfoWithTasksForMutation() to handle WorkflowMutation (similar to existing function for WorkflowSnapshot)

Modified UpdateWorkflowExecution:

• Now calls syncExecutionInfoWithTasksForMutation(&request.UpdateWorkflowMutation)
• Also syncs request.NewWorkflowSnapshot if present

Modified ConflictResolveWorkflowExecution:

• Now syncs all three possible workflows:
  • request.ResetWorkflowSnapshot
  • request.CurrentWorkflowMutation (if present)
  • request.NewWorkflowSnapshot (if present)

Improved Documentation:

• Completed the incomplete comment in syncExecutionInfoWithTasks to clarify the cleanup purpose

Impact: This ensures that workflow-level timer tasks created or modified during workflow updates and conflict resolution are properly tracked and will be cleaned up when the workflow closes, not just those created during initial workflow creation.

Note: Bug #1 (early return on EntityNotExistsError) appears to have already been fixed in the latest code - it correctly uses continue instead of return at line 380 of timer_task_executor_base.go.

[gitar-bot]

Re-applied Critical Fix: Timer Task Tracking Across All Workflow Operations

I noticed that the previous fix for timer task tracking was not included in the latest squashed commit. I've re-applied it.

Issue: The syncExecutionInfoWithTasks function was only called in CreateWorkflowExecution, but timer tasks can also be created/modified during:

• UpdateWorkflowExecution
• ConflictResolveWorkflowExecution

This meant timer tasks created after initial workflow creation were not tracked and wouldn't be cleaned up.

Fix Applied:

1. Added syncExecutionInfoWithTasksForMutation() function to handle WorkflowMutation structures
2. Added sync calls in UpdateWorkflowExecution for both the mutation and new snapshot
3. Added sync calls in ConflictResolveWorkflowExecution for all three possible workflows:
   • Reset workflow snapshot
   • Current workflow mutation (if present)
   • New workflow snapshot (if present)
4. Completed the documentation comment explaining the cleanup purpose

Impact: Timer tasks are now properly tracked throughout the entire workflow lifecycle (creation, updates, conflict resolution), ensuring they can be cleaned up when workflows close.

[gitar-bot]

Code Review Complete ✅

I've reviewed the PR after re-applying the timer task tracking fix. The implementation is now in good shape!

✅ All Critical Issues Resolved:

1. ✅ Timer task tracking now covers all workflow execution methods (Create, Update, ConflictResolve)
2. ✅ Early return bug fixed - uses continue to process all timer tasks
3. ✅ Documentation completed and improved
4. ✅ Best-effort deletion pattern correctly implemented
5. ✅ Feature flag properly used throughout

ℹ️ Minor Issue Remaining:

• TaskCleanupTimeoutThreshold config is defined but not used
  • Documentation suggests it's for skipping cleanup on short-lived workflows
  • Implementation is missing, but this is low priority

Summary: The core functionality is complete and correct. Timer tasks will now be properly tracked and cleaned up across the entire workflow lifecycle, preventing resource leaks.

[gitar-bot]

Code Review 👍 Approved with suggestions 4 resolved / 5 findings

The timer cleanup feature implementation looks good overall. The config TaskCleanupTimeoutThreshold is still defined but unused - consider either using it to skip cleanup for short-lived workflows as documented, or removing it.

💡 Quality: TaskCleanupTimeoutThreshold config added but never used

📄 service/history/config/config.go:297 📄 service/history/config/config.go:571

The TaskCleanupTimeoutThreshold configuration property is added to the history service config but appears to be unused in this PR:

// In config/config.go
TaskCleanupTimeoutThreshold dynamicproperties.DurationPropertyFn
...
TaskCleanupTimeoutThreshold:           dc.GetDurationProperty(dynamicproperties.TaskCleanupTimeoutThreshold),

This creates dead code and may confuse readers about its purpose. Either:

1. This is intended for future use and should have a TODO comment explaining the intent
2. It was left over from development and should be removed

Suggested fix: Either add a comment explaining the planned usage or remove the unused configuration.

✅ 4 resolved

✅ Bug: Early return on EntityNotExistsError breaks loop

📄 service/history/task/timer_task_executor_base.go:373
In deleteWorkflowTimerTasksBestEffort, when an EntityNotExistsError is encountered, the function immediately returns, stopping the deletion of remaining timer tasks.

The current code:

if errors.As(err, new(*types.EntityNotExistsError)) {
    // in perhaps a significant minority of cases, it's likely the timer's already fired
    return
}

This should continue instead of return to process the remaining timer tasks. One timer being already deleted/fired should not prevent cleanup of other pending timer tasks.

Impact: If multiple workflow timer tasks exist and the first one happens to have already fired (thus returning EntityNotExistsError), the remaining timer tasks will not be cleaned up, defeating the purpose of this feature.

Suggested fix: Replace return with continue to process all timer tasks.

✅ Bug: syncExecutionInfoWithTasks only called for CreateWorkflowExecution

📄 common/persistence/execution_manager.go:630
The syncExecutionInfoWithTasks function is only called in CreateWorkflowExecution, but timer tasks can also be created/modified during UpdateWorkflowExecution and ConflictResolveWorkflowExecution operations.

Looking at the diff:

• CreateWorkflowExecution calls m.syncExecutionInfoWithTasks(&request.NewWorkflowSnapshot)
• UpdateWorkflowExecution does NOT call syncExecutionInfoWithTasks for request.UpdateWorkflowMutation or request.NewWorkflowSnapshot
• ConflictResolveWorkflowExecution does NOT call syncExecutionInfoWithTasks for any of its workflow snapshots/mutations

Impact: Workflow timer tasks created or modified after the initial workflow creation will not be tracked, meaning they won't be cleaned up when the workflow closes. This significantly limits the effectiveness of this feature since many workflow-level timer tasks (like workflow timeout extensions or retries) may be created during updates.

Suggested fix: Call syncExecutionInfoWithTasks for all workflow mutations and snapshots in UpdateWorkflowExecution and ConflictResolveWorkflowExecution as well.

✅ Quality: Incomplete function doc comment in syncExecutionInfoWithTasks

📄 common/persistence/execution_manager.go:662
The function comment for syncExecutionInfoWithTasks is incomplete - it ends mid-sentence with "such that when we go to clean up".

Current:

// The goal of this is to ensure that the mutable state / execution info is in sync with the tasks.
// for the purpose of tracking them, such that when we go to clean up
//
// The reason this must be done here is that the tasks are first created...

The second line appears to be cut off. This incomplete documentation makes it harder to understand the full purpose of the function.

Suggested fix: Complete the documentation to explain the full cleanup flow.

✅ Edge Case: DeleteTimerTaskRequest missing ShardID field

📄 service/history/task/timer_task_executor_base.go:366 📄 common/persistence/data_manager_interfaces.go
Looking at the DeleteTimerTask call in deleteWorkflowTimerTasksBestEffort:

return t.shard.GetExecutionManager().DeleteTimerTask(ctx, &persistence.DeleteTimerTaskRequest{
    TaskID:              taskInfo.TaskID,
    VisibilityTimestamp: taskInfo.VisibilityTimestamp,
})

The DeleteTimerTaskRequest only contains TaskID and VisibilityTimestamp, but looking at other delete task patterns in the codebase (like CompleteTimerTask), they typically include ShardID for proper routing.

The DeleteTimerTaskRequest struct definition from the diff:

DeleteTimerTaskRequest struct {
    TaskID              int64
    VisibilityTimestamp time.Time
}

This may work if the execution manager inherently knows the shard context, but it's worth verifying that the downstream implementations (Cassandra, SQL, etc.) can properly identify the shard without an explicit ShardID in the request.

Note: This may be intentional if the ExecutionManager already has shard context, but worth confirming the implementation handles this correctly across all database backends.

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing less information.

Comment with these commands to change:

Auto-apply	Compact
gitar auto-apply:on	gitar display:verbose

_{Was this helpful? React with 👍 / 👎 | Gitar}