Skip to content

Add support for updating start_delay via UpdateActivityOptions#10745

Merged
fretz12 merged 9 commits into
feature/activity-operator-cmdsfrom
fredtzeng/saa-start-delay-update
Jul 3, 2026
Merged

Add support for updating start_delay via UpdateActivityOptions#10745
fretz12 merged 9 commits into
feature/activity-operator-cmdsfrom
fredtzeng/saa-start-delay-update

Conversation

@fretz12

@fretz12 fretz12 commented Jun 16, 2026

Copy link
Copy Markdown
Contributor

What changed?

start_delay is now updatable via UpdateActivityOptions for standalone activities. The new value is anchored to the original schedule_time, so users can shorten it (including to 0, which dispatches immediately if the target is already past), extend it, or restore the original via RestoreOriginal. Updates once the activity has left its delay window (first dispatch passed, or no longer SCHEDULED) are rejected with InvalidArgument. The merge, validation, and namespace dynamic-config check from activity creation are reused, so create and update stay consistent.

This also adds the shared infrastructure for the downstream PRs:

  • A persisted first_attempt_started_time on ActivityState, set once on the first SCHEDULED → STARTED and never updated on retries/resets. It is the canonical "has the first dispatch happened" discriminator.
  • A respectStartDelay helper that lifts a candidate dispatch time to scheduleTime + start_delay until first pickup, and no-ops afterward. It drives ActivityDispatchTask re-issuance so an unrelated update in the delay window no longer dispatches early, and gates the RestoreOriginal restore so it never shows a stale value post-dispatch.
  • Timer re-issuance split into reissueRunningAttemptTimers (StartToClose/Heartbeat) and reissueScheduledDispatch (Dispatch/ScheduleToStart).

Also changes a pre-existing behavior where the update path anchored ScheduleToClose to scheduleTime + timeout instead of firstDispatchTime + timeout; it now uses the canonical scheduleToCloseDeadline(), matching TransitionScheduled and hasEnoughTimeForRetry.

Why?

Run-now (cancel-delay / kickstart) is table-stakes in the job queues SAA is replacing (BullMQ, Sidekiq, Cloud Tasks, Asynq) and a GA recommendation. Exposing start_delay through UpdateActivityOptions covers both run-now (set 0) and rescheduling with one primitive, using the same anchored-to-schedule-time semantics as creation. start_delay is a first-attempt parameter, so first_attempt_started_time keeps every re-scheduling path from re-applying or restoring it after dispatch, where it would only mislead. The ScheduleToClose change prevents an unrelated update during the delay window from shrinking the deadline (sometimes into the past) and timing the activity out before it dispatches.

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)

@fretz12 fretz12 marked this pull request as ready for review June 16, 2026 22:31
@fretz12 fretz12 requested review from a team as code owners June 16, 2026 22:31
@fretz12 fretz12 marked this pull request as draft June 17, 2026 15:57
@fretz12 fretz12 force-pushed the fredtzeng/saa-start-delay-update branch from e6ff106 to a73e9e7 Compare June 17, 2026 16:44
@fretz12 fretz12 force-pushed the fredtzeng/saa-start-delay-update branch from a73e9e7 to 2bfa39e Compare June 17, 2026 17:07
@fretz12 fretz12 force-pushed the fredtzeng/saa-start-delay-update branch 2 times, most recently from 8b0c9ab to 1dd4571 Compare June 18, 2026 21:23
Comment thread chasm/lib/activity/activity.go Outdated
// respectStartDelay lifts a candidate dispatch time up to scheduleTime + start_delay when the
// activity has not yet been picked up by a worker, so pre-dispatch re-scheduling (unpause, Reset+
// RestoreOriginalOptions, options update) honors start_delay. No-op once dispatched.
func (a *Activity) respectStartDelay(scheduleTime time.Time) time.Time {

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.

important helper reused in downstream PRs

Comment thread chasm/lib/activity/activity.go Outdated
// reissueScheduledDispatch re-emits the ActivityDispatchTask and ScheduleToStart timeout task for
// a SCHEDULED activity. Retries fire at the retry time; first attempts dispatch now, lifted to
// honor any pending start_delay.
func (a *Activity) reissueScheduledDispatch(ctx chasm.MutableContext, attempt *activitypb.ActivityAttemptState) {

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.

Refactoring due to lint complaining code complexity

// paths after stamp bump so the old tasks are invalidated and replaced with the (possibly
// updated) timeouts. No-op unless the activity is in a status where a worker holds the task token
// (STARTED / CANCEL_REQUESTED / PAUSE_REQUESTED / RESET_REQUESTED).
func (a *Activity) reissueRunningAttemptTimers(ctx chasm.MutableContext, attempt *activitypb.ActivityAttemptState) {

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.

Refactoring due to lint complaining code complexity

@fretz12 fretz12 marked this pull request as ready for review June 18, 2026 22:55
if a.GetStatus() != activitypb.ACTIVITY_EXECUTION_STATUS_SCHEDULED ||
!a.firstDispatchTime().After(ctx.Now(a)) {
return nil, serviceerror.NewInvalidArgument(
"cannot update start_delay: activity is no longer in its delay window")

@dandavison dandavison Jun 22, 2026

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.

I think this should be FailedPrecondition. That would match what we do e.g. when pausing an already-paused. Changed in #10884

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.

ok. Letting your change take effect.

@dandavison dandavison left a comment

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.

Looks good!

@dandavison dandavison mentioned this pull request Jun 29, 2026

@dandavison dandavison left a comment

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.

Nice, LGTM!

@dandavison dandavison force-pushed the fredtzeng/saa-start-delay-update branch from 897a35d to 9e5d417 Compare July 1, 2026 11:16
@fretz12 fretz12 force-pushed the feature/activity-operator-cmds branch from 0f70f60 to d4e66c6 Compare July 1, 2026 21:16
fretz12 and others added 4 commits July 2, 2026 16:09
…tivities (#10752)

`Reset` with `RestoreOriginalOptions` now restores `start_delay` for
standalone activities, gated on `FirstAttemptStartedTime == nil` (the
first attempt has not been picked up yet). When restored, the next
dispatch is clamped to the original wall-clock target (`scheduleTime +
original_start_delay`) via `respectStartDelay`, so undoing a "zeroed
out" delay lands at the moment originally requested instead of firing
immediately. Once the activity has dispatched (`STARTED`, retry-backoff
`SCHEDULED`, or post-dispatch `PAUSED`), `start_delay` is left
untouched, since it only governs the first dispatch and restoring it
afterward would just surface a stale value and shift the close deadline.

Also simplifies `TransitionResetRequested` to take no event payload,
since it is a pure status flip and all state mutation happens in
`handleReset` before `Apply`.

`RestoreOriginalOptions` is the documented "back to original" path, so
without restoring `start_delay` the undo was only half-honored: timeouts
and retry policy snapped back but the dispatch target stayed at the
updated value. Gating on `FirstAttemptStartedTime` (rather than status)
keeps the contract consistent with the update path: `start_delay` is a
first-attempt parameter, so it is only meaningful to restore before the
first dispatch.

- [X] built
- [X] run locally and tested manually
- [X] covered by existing tests
- [ ] added new unit test(s)
- [X] added new functional test(s)

---------

Co-authored-by: Dan Davison <dan.davison@temporal.io>
## What changed?
- This PR should cause no change in SAA behavior at all
- Documented standard terminology for the times in the attempt lifecycle
- Renamed functions and variables to more accurately and consistently
describe mechanics
- Cleaned up repeat calculation of current time on scheduling 
- Added a test of updating Task Queue

## Why?
- Code clarity
- Subsequent PRs on top of this one fix bugs; these bug fixes are easier
to understand expressed in the consistent terminology

## How did you test it?
- [x] covered by existing tests

<!-- CURSOR_SUMMARY -->
---

> [!NOTE]
> <sup>[Cursor Bugbot](https://cursor.com/bugbot) is generating a
summary for commit 8faf8ac. Configure
[here](https://www.cursor.com/dashboard/bugbot).</sup>
<!-- /CURSOR_SUMMARY -->
# SAA operator API: Reset / UpdateOptions fixes

## What changed?
- Reset and UpdateOptions were not recreating the ScheduleToClose timer
when an option change moved the deadline.
- A plain Reset (no RestoreOriginalOptions) was not honoring the
remaining start_delay.
- Reset(RestoreOriginalOptions) was mutating options immediately, which
would impact a running attempt. Changed to defer all mutation to the
next attempt.
- Reset while STARTED was not honoring the reset if retries were
exhausted, or the failure was non-retryable.
- Return FailedPrecondition instead of InvalidArgument when
UpdateOptions is rejected due to start_delay.
- Change Reset to rejected with FailedPrecondition when a cancel or
another reset is already pending.

## Why?
Correctness

## How did you test it?
- [x] added new functional test(s)

## Potential risks
Could result in incorrect SAA task scheduling / timeouts.
dandavison and others added 3 commits July 2, 2026 16:10
…delay (#10877)

## What changed?
Previously Unpause on attempt > 1 re-dispatched immediately, discarding
the remaining retry delay. Changed to honor it the same way that it
honors start delay. Reset discards the remaining start delay.

## Why?
Correctness: agreed behavior was not implemented

## How did you test it?
- [x] added new functional test(s)

## Potential risks
Could cause incorrect SAA task scheduling
…ryably (#10909)

## What changed?
Honor Reset request even if attempt fails non-retryably

## Why?
Agreed product spec.

## How did you test it?
- [x] added new functional test(s)

## Potential risks
Could break SAA reset behavior if wrong
@fretz12 fretz12 force-pushed the fredtzeng/saa-start-delay-update branch from 637cf36 to ed2faed Compare July 3, 2026 00:01
@fretz12 fretz12 merged commit 5092270 into feature/activity-operator-cmds Jul 3, 2026
44 checks passed
@fretz12 fretz12 deleted the fredtzeng/saa-start-delay-update branch July 3, 2026 01:55
fretz12 added a commit that referenced this pull request Jul 10, 2026
`start_delay` is now updatable via `UpdateActivityOptions` for
standalone activities. The new value is anchored to the original
`schedule_time`, so users can shorten it (including to 0, which
dispatches immediately if the target is already past), extend it, or
restore the original via `RestoreOriginal`. Updates once the activity
has left its delay window (first dispatch passed, or no longer
`SCHEDULED`) are rejected with `InvalidArgument`. The merge, validation,
and namespace dynamic-config check from activity creation are reused, so
create and update stay consistent.

This also adds the shared infrastructure for the downstream PRs:
- A persisted `first_attempt_started_time` on `ActivityState`, set once
on the first `SCHEDULED → STARTED` and never updated on retries/resets.
It is the canonical "has the first dispatch happened" discriminator.
- A `respectStartDelay` helper that lifts a candidate dispatch time to
`scheduleTime + start_delay` until first pickup, and no-ops afterward.
It drives `ActivityDispatchTask` re-issuance so an unrelated update in
the delay window no longer dispatches early, and gates the
`RestoreOriginal` restore so it never shows a stale value post-dispatch.
- Timer re-issuance split into `reissueRunningAttemptTimers`
(StartToClose/Heartbeat) and `reissueScheduledDispatch`
(Dispatch/ScheduleToStart).

Also changes a pre-existing behavior where the update path anchored
`ScheduleToClose` to `scheduleTime + timeout` instead of
`firstDispatchTime + timeout`; it now uses the canonical
`scheduleToCloseDeadline()`, matching `TransitionScheduled` and
`hasEnoughTimeForRetry`.

Run-now (cancel-delay / kickstart) is table-stakes in the job queues SAA
is replacing (BullMQ, Sidekiq, Cloud Tasks, Asynq) and a GA
recommendation. Exposing `start_delay` through `UpdateActivityOptions`
covers both run-now (set 0) and rescheduling with one primitive, using
the same anchored-to-schedule-time semantics as creation. `start_delay`
is a first-attempt parameter, so `first_attempt_started_time` keeps
every re-scheduling path from re-applying or restoring it after
dispatch, where it would only mislead. The `ScheduleToClose` change
prevents an unrelated update during the delay window from shrinking the
deadline (sometimes into the past) and timing the activity out before it
dispatches.

- [X] built
- [X] run locally and tested manually
- [X] covered by existing tests
- [ ] added new unit test(s)
- [X] added new functional test(s)

---------

Co-authored-by: Dan Davison <dan.davison@temporal.io>
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.

2 participants