Recreate only the activity timers whose deadline actually moved by xwduan · Pull Request #11565 · temporalio/temporal

xwduan · 2026-08-13T22:14:48Z

Problem

State-based replication blanket-cleared an activity's TimerTaskStatus whenever a replicated update changed the attempt, version or stamp:

if current == nil || ms.ShouldResetActivityTimerTaskMask(current, incoming) {
    incoming.TimerTaskStatus = TimerTaskStatusNone
}

Every cleared bit lets the next task refresh regenerate a timer task. CreateNextActivityTimer only ever creates the earliest timer in the sequence and bails if that one already exists — so the cost is at most one task per refresh, but which timer it is depends on the activity's state.

Between retry attempts, that earliest timer is schedule-to-close. Clearing the started state removes start-to-close and heartbeat from the sequence, and when the user doesn't set ScheduleToStartTimeout it is normalized to the schedule-to-close duration — anchored on ScheduledTime, which advances every retry, so it is always later than schedule-to-close anchored on the fixed FirstScheduledTime.

So the passive side regenerates schedule-to-close once per replicated retry, and because that deadline never moves, every duplicate carries an identical VisibilityTimestamp. A concrete case: a 31-day activity with a flat 10s backoff reached attempt 242,037 — on the order of 242k tasks piled on a single instant, essentially all of which re-derive the sequence, find nothing expired, and return errNoTimerFired.

The duplicates are dropped at execution, so there is no correctness impact. It is a timer-queue hotspot.

Approach

Compare the four timer deadlines before and after the update, and keep a bit only when its deadline is unchanged:

return current.TimerTaskStatus &^
    getActivityTimerDeadlines(current).changedMask(getActivityTimerDeadlines(incoming))

Why deadlines are the right and sufficient signal. An ActivityTimeoutTask is a wake-up at a point in time: processSingleActivityTimeoutTask re-derives the whole sequence from current mutable state and fires whatever expired, explicitly without consulting the task's attempt or stamp ("Note: we don't need to check activity Stamps"). A pending task whose deadline did not move is still correct no matter what else changed; one whose deadline moved is useless no matter what stayed put.

Attempt and stamp are only ever proxies for "some deadline probably moved", so they are deliberately not consulted. A retry needs no special case: clearing the started state removes start-to-close and heartbeat outright, and schedule-to-start's anchor advances — while schedule-to-close survives on its untouched FirstScheduledTime.

This also fixes a case the active side still has: for mutable state predating FirstScheduledTime, schedule-to-close falls back to the ScheduledTime anchor, which a retry does move. Keying off the attempt preserves that bit and leaves a task pointing at the old, earlier instant; comparing deadlines clears it.

A cross-cluster version change still resets everything — that is about task provenance, not deadlines.

The four deadline getters are split into free functions with one-line method wrappers so the comparison reuses timerSequenceImpl's existing math rather than duplicating it.

Scope

Limited to state-based replication. The event-based SyncActivity path in ndc/activity_state_replicator.go keeps its existing blanket reset and is untouched — it passes a synthetic ActivityInfo carrying only version and attempt, which cannot support a deadline comparison, and it is the older path.

Tests

9 cases. The ones carrying the change:

Retry_KeepsOnlyScheduleToClose — a real retry transition (attempt bumped, started state cleared, schedule time advanced); start-to-close and heartbeat bits drop, schedule-to-close survives.
Retry_LegacyAnchor_ClearsScheduleToClose — same retry with FirstScheduledTime nil; the moved deadline is detected and the bit clears.
AttemptChangedWithoutDeadlineMove_KeepsMask — attempt alone decides nothing.
ClearsOnlyMovedDeadlines / TimerDisappears — shorten or remove HeartbeatTimeout; only that bit drops.
UnrelatedOptionChanged_KeepsMask — stamp bumped, no deadline affected, mask fully preserved (the old code wiped it).

go build ./service/... and go vet clean; ./service/history/workflow/... ./service/history/ndc/... pass across repeated runs.

Two pre-existing flakes (not from this change)

Both reproduce identically on unmodified main and neither touches ActivityInfo.TimerTaskStatus. Skipped in the runs above and left alone here:

TestTaskRefresherSuite/TestRefreshSubStateMachineTasks — 7/12 failures on unmodified main. HSM timer infos get deadlines differing by a single nanosecond, so grouping depends on map iteration order. Worth a separate fix; it currently fails more often than it passes.
TestMutableStateSuite/*/TestApplyWorkflowExecutionOptionsUpdatedEvent_TimeSkippingConfig — 1/30 on clean tree, 1/30 with this change. Asserts a renewed TargetTime differs from the initial one; fails when both land on the same clock reading.

🤖 Generated with Claude Code

CLAassistant · 2026-08-13T22:14:57Z

All committers have signed the CLA.

yux0 · 2026-08-17T21:27:37Z

@@ -9296,11 +9329,7 @@ func (ms *MutableStateImpl) applyUpdatesToSubStateMachines(
 	isSnapshot bool,
 ) error {
 	err := applyUpdatesToSubStateMachine(ms, ms.pendingActivityInfoIDs, ms.updateActivityInfos, updatedActivityInfos, isSnapshot, ms.DeleteActivity, func(current, incoming *persistencespb.ActivityInfo) {
-		if current == nil || ms.ShouldResetActivityTimerTaskMask(current, incoming) {


do we need to handle the UpdateActivityInfo? https://github.com/temporalio/temporal/blob/main/service/history/workflow/mutable_state_impl.go#L2168

That is the old event based replication, I feel we can live with it.

Still an issue for s2c migration right?

YES. Will issue another PR for event based stack.

yycptt · 2026-08-17T20:28:12Z

+//   - Stamp changed: the activity's options were modified, which can move any subset of
+//     the four deadlines. Compare them one by one and drop only the bits whose deadline
+//     actually moved.
+func (ms *MutableStateImpl) nextActivityTimerTaskMask(current, incoming *persistencespb.ActivityInfo) int32 {


nit: re. naming: this returns the new status, not a status mask?

yycptt · 2026-08-18T00:15:51Z

@@ -9296,11 +9329,7 @@ func (ms *MutableStateImpl) applyUpdatesToSubStateMachines(
 	isSnapshot bool,
 ) error {
 	err := applyUpdatesToSubStateMachine(ms, ms.pendingActivityInfoIDs, ms.updateActivityInfos, updatedActivityInfos, isSnapshot, ms.DeleteActivity, func(current, incoming *persistencespb.ActivityInfo) {
-		if current == nil || ms.ShouldResetActivityTimerTaskMask(current, incoming) {


Still an issue for s2c migration right?

State-based replication blanket-cleared an activity's TimerTaskStatus whenever a replicated update changed the attempt, version or stamp. Every cleared bit lets the next task refresh regenerate a timer task, and CreateNextActivityTimer recreates whichever timer is earliest in the sequence. For an activity between retry attempts that is the schedule-to-close timer: clearing the started state removes the start-to-close and heartbeat timers, and schedule-to-start is normalized to the schedule-to-close duration when the user does not set it, so it is anchored later. Schedule-to-close is therefore regenerated once per replicated retry, every duplicate carrying the identical FirstScheduledTime-derived deadline. A long-lived activity retrying on a short backoff can accumulate hundreds of thousands of tasks piled on one instant. They are dropped at execution, so there is no correctness impact, but it is a timer-queue hotspot. Decide the carried-over mask by comparing the four timer deadlines before and after the update, keeping a bit only when its deadline is unchanged. Deadlines are the right and sufficient signal: an ActivityTimeoutTask is a wake-up at a point in time, and processSingleActivityTimeoutTask re-derives the whole sequence from current mutable state and fires whatever expired, without consulting the task's attempt or stamp. A pending task whose deadline did not move is still correct no matter what else changed, and one whose deadline moved is useless no matter what stayed put. Attempt and stamp are only ever proxies for "some deadline probably moved", so they are deliberately not consulted. This also fixes a case the active side still has: for mutable state predating FirstScheduledTime, schedule-to-close falls back to the ScheduledTime anchor, which a retry does move. Keying off the attempt preserves that bit and leaves a task pointing at the old instant; comparing deadlines clears it. A cross-cluster version change still resets everything, since that is about task provenance rather than deadlines. The four deadline getters are split into free functions with method wrappers so the comparison reuses the existing math instead of duplicating it. Scoped to state-based replication. The event-based SyncActivity path in ndc/activity_state_replicator.go keeps its existing blanket reset. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

…) (#11613) ## What changed? Recreate only the activity timers whose deadline actually moved to avoid duplicated schedule to close timer task. ### Original PRs #11565 ## Why is this necessary as a patch? To reduce the risk of hot shard ## What testing or retesting is appropriate for this patch after merge? We will run bench failover test against test cluster. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>

Resolves conflicts with temporalio#11565, which split the activity timer getters into free functions plus thin method wrappers. Both forms now return (TimerSequenceID, bool), and getActivityTimerDeadlines iterates over the getters instead of over their results.

## The bug `applyUpdatesToSubStateMachine` shadowed the variable it meant to populate: ```go var existing V // outer: never assigned if existing, ok := pendingInfos[key]; ok { // := declares a NEW existing, scoped to the if ... ms.approximateSize -= existing.Size() + getSizeOfKey(key) // inner one — correct here } val := updated if sanitizeFn != nil { val = common.CloneProto(updated) sanitizeFn(existing, val) // outer one — always nil } ``` `:=` only requires *one* new variable on the left (`ok`), and the `if` init statement is its own scope, so `existing` is redeclared there rather than assigned. It compiles because both variables are used, and `go vet` does not check shadowing by default. `sanitizeFn` has therefore always received `current == nil`. ## What this activates Two of the five `sanitizeFn` implementations depend on `current`; the other three (timers, request cancels, signals) ignore it or are `nil`, so they are unaffected. **Activities.** `getActivityTimerTaskStatus` short-circuits to `TimerTaskStatusNone` when `current` is nil, so every applied activity update cleared the *entire* timer task mask rather than only the bits whose deadline moved. The deadline comparison added in #11565 has never taken effect — that PR is currently a no-op. **Child executions.** `if current != nil { incoming.Clock = current.Clock }` never ran. `Clock` is a local shard vector clock that `sanitizeChildExecutionInfo` strips before replicating, so the incoming copy always arrives nil — the guard exists precisely to restore the local value. Without it, every replicated update to an existing child execution erased it. ## Risk Both are restorations of intended behavior, not new behavior, and both previously failed in the safe direction: - An over-cleared activity mask regenerates a timer task that is already pending. Duplicates are dropped at execution (`processSingleActivityTimeoutTask` re-derives the sequence and fires what expired), so the symptom was extra timer-queue writes, not missed timeouts. - A nil child clock is tolerated by callers — see the comment in `recordchildworkflowcompleted/api.go`: *"it should be fine e.g. that ci.Clock is nil"*. The child execution change is a **no-op unless the local cluster recorded a clock by starting the child itself**, which for a passive cluster means after a failover. On a standby that never started the child, both sides are nil. For activities, preserved bits suppress recreation of a timer task, so it is worth being explicit about why that is safe on the passive side: an expired-but-unresolved standby timer returns `ErrTaskRetry` and stays in the queue rather than being consumed, so there is nothing to recreate. The one path that does drop a task, past `StandbyTaskMissingEventsDiscardDelay`, logs a warning and increments `task_errors_discarded`, so it cannot happen silently. ## Tests Two new tests at the `applyUpdatesToSubStateMachines` level, one per activated `sanitizeFn`. **Both fail with the shadowing restored** (verified: `expected: 13, actual: 0` for the mask; `local child execution clock was not carried over` for the clock). The gap they close: the existing `getActivityTimerTaskStatus` tests call the decision function directly with a non-nil `current`, so no unit test could observe the *caller* passing nil. Nine passing tests, zero coverage of the wiring. Two pre-existing tests needed updating, both informative: - `TestApplyMutation` / `TestApplySnapshot` failed with *"Unexpected call to IsVersionFromSameCluster"* — that mock was never needed because the nil short-circuit made the cluster check unreachable. The gap is itself evidence the path was dead. - `verifyActivityInfos` asserted `s.Equal(int32(TimerTaskStatusNone), actual.TimerTaskStatus)`, encoding the bug and blocking any correct fix. Replaced with the invariant that actually matters: ```go s.Zero(actual.TimerTaskStatus&^originStatus, "TimerTaskStatus gained a bit that was not set locally") ``` Applying an update may carry over or drop locally set bits, but must never introduce one that was not already set — claiming a timer task exists when none does is the direction that loses timers. That holds regardless of which bits a given case preserves, so it will not need rewriting as the mask logic evolves. ## Related, tracked separately While auditing whether the mask can be trusted, one path was found that moves a deadline without invalidating the mask: unpausing an activity. Paused activities are excluded from the timer sequence, so a timeout task firing during a long pause is dropped as an invalid task, yet `unpauseActivityInfo` leaves the bit set. That is pre-existing and active-side, independent of this PR, and is being tracked as its own change. It is worth noting here because this PR removes the passive side's accidental repair for that class of stale bit: today's blanket wipe clears any stale bit on the next replicated activity update. --- `go build ./service/...`, `go vet`, and golangci-lint (repo config, `--new-from-rev=origin/main`) clean; `./service/history/workflow/... ./service/history/ndc/...` pass across 3 consecutive runs. Two pre-existing flakes are skipped in those runs and unrelated to this change (both reproduce on unmodified `main`): `TestTaskRefresherSuite/TestRefreshSubStateMachineTasks` (nanosecond-differing HSM deadlines, map iteration order — fails 7/12 on main) and `TestMutableStateSuite/*/TestApplyWorkflowExecutionOptionsUpdatedEvent_TimeSkippingConfig` (wall-clock resolution, ~1/30). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5 <noreply@anthropic.com>

…1646) ## The bug `applyUpdatesToSubStateMachine` shadowed the variable it meant to populate: ```go var existing V // outer: never assigned if existing, ok := pendingInfos[key]; ok { // := declares a NEW existing, scoped to the if ... ms.approximateSize -= existing.Size() + getSizeOfKey(key) // inner one — correct here } val := updated if sanitizeFn != nil { val = common.CloneProto(updated) sanitizeFn(existing, val) // outer one — always nil } ``` `:=` only requires *one* new variable on the left (`ok`), and the `if` init statement is its own scope, so `existing` is redeclared there rather than assigned. It compiles because both variables are used, and `go vet` does not check shadowing by default. `sanitizeFn` has therefore always received `current == nil`. ## What this activates Two of the five `sanitizeFn` implementations depend on `current`; the other three (timers, request cancels, signals) ignore it or are `nil`, so they are unaffected. **Activities.** `getActivityTimerTaskStatus` short-circuits to `TimerTaskStatusNone` when `current` is nil, so every applied activity update cleared the *entire* timer task mask rather than only the bits whose deadline moved. The deadline comparison added in temporalio#11565 has never taken effect — that PR is currently a no-op. **Child executions.** `if current != nil { incoming.Clock = current.Clock }` never ran. `Clock` is a local shard vector clock that `sanitizeChildExecutionInfo` strips before replicating, so the incoming copy always arrives nil — the guard exists precisely to restore the local value. Without it, every replicated update to an existing child execution erased it. ## Risk Both are restorations of intended behavior, not new behavior, and both previously failed in the safe direction: - An over-cleared activity mask regenerates a timer task that is already pending. Duplicates are dropped at execution (`processSingleActivityTimeoutTask` re-derives the sequence and fires what expired), so the symptom was extra timer-queue writes, not missed timeouts. - A nil child clock is tolerated by callers — see the comment in `recordchildworkflowcompleted/api.go`: *"it should be fine e.g. that ci.Clock is nil"*. The child execution change is a **no-op unless the local cluster recorded a clock by starting the child itself**, which for a passive cluster means after a failover. On a standby that never started the child, both sides are nil. For activities, preserved bits suppress recreation of a timer task, so it is worth being explicit about why that is safe on the passive side: an expired-but-unresolved standby timer returns `ErrTaskRetry` and stays in the queue rather than being consumed, so there is nothing to recreate. The one path that does drop a task, past `StandbyTaskMissingEventsDiscardDelay`, logs a warning and increments `task_errors_discarded`, so it cannot happen silently. ## Tests Two new tests at the `applyUpdatesToSubStateMachines` level, one per activated `sanitizeFn`. **Both fail with the shadowing restored** (verified: `expected: 13, actual: 0` for the mask; `local child execution clock was not carried over` for the clock). The gap they close: the existing `getActivityTimerTaskStatus` tests call the decision function directly with a non-nil `current`, so no unit test could observe the *caller* passing nil. Nine passing tests, zero coverage of the wiring. Two pre-existing tests needed updating, both informative: - `TestApplyMutation` / `TestApplySnapshot` failed with *"Unexpected call to IsVersionFromSameCluster"* — that mock was never needed because the nil short-circuit made the cluster check unreachable. The gap is itself evidence the path was dead. - `verifyActivityInfos` asserted `s.Equal(int32(TimerTaskStatusNone), actual.TimerTaskStatus)`, encoding the bug and blocking any correct fix. Replaced with the invariant that actually matters: ```go s.Zero(actual.TimerTaskStatus&^originStatus, "TimerTaskStatus gained a bit that was not set locally") ``` Applying an update may carry over or drop locally set bits, but must never introduce one that was not already set — claiming a timer task exists when none does is the direction that loses timers. That holds regardless of which bits a given case preserves, so it will not need rewriting as the mask logic evolves. ## Related, tracked separately While auditing whether the mask can be trusted, one path was found that moves a deadline without invalidating the mask: unpausing an activity. Paused activities are excluded from the timer sequence, so a timeout task firing during a long pause is dropped as an invalid task, yet `unpauseActivityInfo` leaves the bit set. That is pre-existing and active-side, independent of this PR, and is being tracked as its own change. It is worth noting here because this PR removes the passive side's accidental repair for that class of stale bit: today's blanket wipe clears any stale bit on the next replicated activity update. --- `go build ./service/...`, `go vet`, and golangci-lint (repo config, `--new-from-rev=origin/main`) clean; `./service/history/workflow/... ./service/history/ndc/...` pass across 3 consecutive runs. Two pre-existing flakes are skipped in those runs and unrelated to this change (both reproduce on unmodified `main`): `TestTaskRefresherSuite/TestRefreshSubStateMachineTasks` (nanosecond-differing HSM deadlines, map iteration order — fails 7/12 on main) and `TestMutableStateSuite/*/TestApplyWorkflowExecutionOptionsUpdatedEvent_TimeSkippingConfig` (wall-clock resolution, ~1/30). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5 <noreply@anthropic.com>

xwduan requested a review from a team August 13, 2026 22:14

xwduan requested review from a team as code owners August 13, 2026 22:14

github-actions Bot mentioned this pull request Aug 13, 2026

🦞 OpenClaw 生态日报 2026-08-14 junlinzhao327-oss/big_model_radar#343

Open

xwduan force-pushed the will/fix-passive-activity-timer-mask branch from 1a22e1c to b1d9cd6 Compare August 17, 2026 19:06

xwduan changed the title ~~Preserve schedule-to-close timer bit on passive activity retry~~ Recreate only the activity timers whose deadline actually moved Aug 17, 2026

xwduan force-pushed the will/fix-passive-activity-timer-mask branch from 278708d to 64edef8 Compare August 17, 2026 19:19

yux0 reviewed Aug 17, 2026

View reviewed changes

xwduan force-pushed the will/fix-passive-activity-timer-mask branch from 64edef8 to 29c61de Compare August 17, 2026 21:42

yux0 approved these changes Aug 17, 2026

View reviewed changes

yiminc approved these changes Aug 18, 2026

View reviewed changes

yycptt approved these changes Aug 18, 2026

View reviewed changes

xwduan force-pushed the will/fix-passive-activity-timer-mask branch from 29c61de to 7ba044e Compare August 18, 2026 16:07

xwduan added 2 commits August 18, 2026 10:00

address comments

351e06c

Merge branch 'main' into will/fix-passive-activity-timer-mask

433d2e8

xwduan enabled auto-merge (squash) August 18, 2026 17:02

xwduan merged commit f667755 into main Aug 18, 2026
54 checks passed

xwduan deleted the will/fix-passive-activity-timer-mask branch August 18, 2026 17:20

xwduan mentioned this pull request Aug 18, 2026

Recreate only the activity timers whose deadline actually moved (#11565) #11613

Merged

xwduan mentioned this pull request Aug 19, 2026

Pass the local sub state machine to sanitizeFn on apply #11646

Merged

awln-temporal mentioned this pull request Sep 3, 2026

awln/backport thrift 0.24 release 1.32 #11915

Closed

Navigation Menu

Uh oh!

Recreate only the activity timers whose deadline actually moved - #11565

Recreate only the activity timers whose deadline actually moved#11565
xwduan merged 3 commits into
mainfrom
will/fix-passive-activity-timer-mask

xwduan commented Aug 13, 2026 •

edited

Loading

Uh oh!

CLAassistant commented Aug 13, 2026 •

edited

Loading

Uh oh!

yux0 Aug 17, 2026

Uh oh!

xwduan Aug 17, 2026

Uh oh!

yycptt Aug 18, 2026

Uh oh!

xwduan Aug 18, 2026

Uh oh!

yycptt Aug 17, 2026

Uh oh!

yycptt Aug 18, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Uh oh!

Conversation

xwduan commented Aug 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Approach

Scope

Tests

Two pre-existing flakes (not from this change)

Uh oh!

CLAassistant commented Aug 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yux0 Aug 17, 2026

Choose a reason for hiding this comment

Uh oh!

xwduan Aug 17, 2026

Choose a reason for hiding this comment

Uh oh!

yycptt Aug 18, 2026

Choose a reason for hiding this comment

Uh oh!

xwduan Aug 18, 2026

Choose a reason for hiding this comment

Uh oh!

yycptt Aug 17, 2026

Choose a reason for hiding this comment

Uh oh!

yycptt Aug 18, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

xwduan commented Aug 13, 2026 •

edited

Loading

CLAassistant commented Aug 13, 2026 •

edited

Loading