Skip to content

Fix schedule update removing cron/spec when not provided (fixes #927) #928

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
billrich2001 wants to merge 1 commit into
base: main
Choose a base branch
from
+178 −22
Open

Fix schedule update removing cron/spec when not provided (fixes #927) #928

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
100 changes: 78 additions & 22 deletions internal/temporalcli/commands.schedule.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -221,31 +221,82 @@ func toIntervalSpec(str string) (client.ScheduleIntervalSpec, error) {
}

func (c *ScheduleConfigurationOptions) toScheduleSpec(spec *client.ScheduleSpec) error {
spec.CronExpressions = c.Cron
// Skip not supported
spec.Jitter = c.Jitter.Duration()
spec.TimeZoneName = c.TimeZone
spec.StartAt = c.StartTime.Time()
spec.EndAt = c.EndTime.Time()
return c.toScheduleSpecForUpdate(spec, nil)
}

var err error
for _, calPbStr := range c.Calendar {
var calPb schedpb.CalendarSpec
if err = protojson.Unmarshal([]byte(calPbStr), &calPb); err != nil {
return fmt.Errorf("failed to parse json calendar spec: %w", err)
// toScheduleSpecForUpdate applies CLI options to spec, preserving existing spec
// fields when the user did not pass the corresponding flags (fully incremental update).
// If existing is nil, behaves like toScheduleSpec (no preservation).
func (c *ScheduleConfigurationOptions) toScheduleSpecForUpdate(spec *client.ScheduleSpec, existing *client.ScheduleSpec) error {
Copy link
Member

@cretz cretz Feb 2, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

I will let CLI reviewers review this, but I wanted to clarify...

The way this CLI works is not a bug, it is very much by intention. It's a backwards incompatible alteration to change this CLI command from being full update to be partial update. For example, users have been able to remove jitter by simply not providing the --jitter flag, but now they can't. Not only is that an incompatible alteration, there are likely a lot of users using it in this declarative form (i.e. they make update calls that are effectively no-ops because they have the same CLI args as what's already on server).

There is an inherent desire for declarative schedules so a user can partial update what they know to be the spec in some separate file. In the meantime, to remain compatible, we need to split off the concept of "partial update" and "full update" (and therefore if the former is needed as a new feature, it should be opted into either via flag or separate command).

Copy link
Contributor Author

@billrich2001 billrich2001 Feb 2, 2026

Choose a reason for hiding this comment

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

Makes sense, this might make more sense as a separate patch command or something where it is intended to keep everything except for updated fields.

Will wait for feedback from CLI team

Copy link
Contributor

@chaptersix chaptersix Feb 2, 2026

Choose a reason for hiding this comment

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

I agree with Chad on not making a backwards incompatible change. I'm not sure off the top of my head how we'd want to address this.

Copy link
Contributor

@chaptersix chaptersix Feb 2, 2026

Choose a reason for hiding this comment

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

I don't think the server api supports updating the cron only right now:
https://github.com/temporalio/api/blob/e966760430f98290e83459571f40c91126ece7d8/temporal/api/schedule/v1/message.proto#L300

Copy link
Contributor

@dnr dnr Feb 2, 2026

Choose a reason for hiding this comment

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

You can make whatever "patch" update you want with UpdateSchedule and the conflict token guard. This is built into the SDK as well

Copy link
Contributor

@dnr dnr Feb 2, 2026

Choose a reason for hiding this comment

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

(but keep in mind that cron strings are canonicalized to structured calendar specs)

// Jitter, TimeZoneName, StartAt, EndAt: preserve from existing when flag not set
if existing != nil && c.FlagSet != nil {
if !c.FlagSet.Changed("jitter") {
spec.Jitter = existing.Jitter
} else {
spec.Jitter = c.Jitter.Duration()
}
cron, err := toCronString(&calPb)
if err != nil {
return err
if !c.FlagSet.Changed("time-zone") {
spec.TimeZoneName = existing.TimeZoneName
} else {
spec.TimeZoneName = c.TimeZone
}
if !c.FlagSet.Changed("start-time") {
spec.StartAt = existing.StartAt
} else {
spec.StartAt = c.StartTime.Time()
}
if !c.FlagSet.Changed("end-time") {
spec.EndAt = existing.EndAt
} else {
spec.EndAt = c.EndTime.Time()
}
} else {
spec.Jitter = c.Jitter.Duration()
spec.TimeZoneName = c.TimeZone
spec.StartAt = c.StartTime.Time()
spec.EndAt = c.EndTime.Time()
}

// Cron/calendar: preserve from existing when not provided by CLI
if len(c.Cron) == 0 && len(c.Calendar) == 0 {
if existing != nil {
spec.CronExpressions = append([]string(nil), existing.CronExpressions...)
spec.Calendars = append([]client.ScheduleCalendarSpec(nil), existing.Calendars...)
} else {
spec.CronExpressions = nil
spec.Calendars = nil
}
} else {
spec.CronExpressions = append([]string(nil), c.Cron...)
var err error
for _, calPbStr := range c.Calendar {
var calPb schedpb.CalendarSpec
if err = protojson.Unmarshal([]byte(calPbStr), &calPb); err != nil {
return fmt.Errorf("failed to parse json calendar spec: %w", err)
}
cron, err := toCronString(&calPb)
if err != nil {
return err
}
spec.CronExpressions = append(spec.CronExpressions, cron)
}
spec.CronExpressions = append(spec.CronExpressions, cron)
}
for _, intStr := range c.Interval {
int, err := toIntervalSpec(intStr)
if err != nil {
return err

// Intervals: preserve from existing when not provided by CLI
if len(c.Interval) == 0 {
if existing != nil {
spec.Intervals = append([]client.ScheduleIntervalSpec(nil), existing.Intervals...)
} else {
spec.Intervals = nil
}
} else {
for _, intStr := range c.Interval {
intv, err := toIntervalSpec(intStr)
if err != nil {
return err
}
spec.Intervals = append(spec.Intervals, intv)
}
spec.Intervals = append(spec.Intervals, int)
}

return nil
Expand Down Expand Up @@ -540,13 +591,18 @@ func (c *TemporalScheduleUpdateCommand) run(cctx *CommandContext, args []string)
newSchedule.State.RemainingActions = c.RemainingActions
}

if err = c.toScheduleSpec(newSchedule.Spec); err != nil {
sch := cl.ScheduleClient().GetHandle(cctx, c.ScheduleId)
description, err := sch.Describe(cctx)
if err != nil {
return err
}

if err = c.toScheduleSpecForUpdate(newSchedule.Spec, description.Schedule.Spec); err != nil {
return err
} else if newSchedule.Action, err = toScheduleAction(&c.SharedWorkflowStartOptions, &c.PayloadInputOptions); err != nil {
return err
}

sch := cl.ScheduleClient().GetHandle(cctx, c.ScheduleId)
return sch.Update(cctx, client.ScheduleUpdateOptions{
DoUpdate: func(u client.ScheduleUpdateInput) (*client.ScheduleUpdate, error) {
// replace whole schedule
Expand Down
100 changes: 100 additions & 0 deletions internal/temporalcli/commands.schedule_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"io"
"math/rand"
"regexp"
"strings"
"time"

"github.com/stretchr/testify/assert"
Expand Down Expand Up @@ -510,6 +511,105 @@ func (s *SharedServerSuite) TestSchedule_Update() {
}, 10*time.Second, 100*time.Millisecond)
}

func (s *SharedServerSuite) TestSchedule_Update_PreservesCronWhenNotProvided() {
schedId, schedWfId, res := s.createSchedule("--cron", "0 9 * * *")
s.NoError(res.Err)

// Update with only non-spec flags (no --cron). Existing cron should be preserved.
res = s.Execute(
"schedule", "update",
"--address", s.Address(),
"-s", schedId,
"--task-queue", "SomeOtherTq",
"--type", "SomeOtherWf",
"--workflow-id", schedWfId,
"--overlap-policy", "AllowAll",
)
s.NoError(res.Err)

s.Eventually(func() bool {
res = s.Execute(
"schedule", "describe",
"--address", s.Address(),
"-s", schedId,
"-o", "json",
)
s.NoError(res.Err)
var j struct {
Schedule struct {
Spec struct {
CronExpressions []string `json:"cronExpressions"`
} `json:"spec"`
Action struct {
StartWorkflow struct {
WorkflowType struct {
Name string `json:"name"`
} `json:"workflowType"`
TaskQueue struct {
Name string `json:"name"`
} `json:"taskQueue"`
} `json:"startWorkflow"`
} `json:"action"`
} `json:"schedule"`
Info struct {
FutureActionTimes []string `json:"futureActionTimes"`
} `json:"info"`
}
s.NoError(json.Unmarshal(res.Stdout.Bytes(), &j))
// Cron was preserved: either cronExpressions contains our cron, or futureActionTimes
// are at 09:00 UTC (server may return structuredCalendar instead of cronExpressions).
cronPreserved := (len(j.Schedule.Spec.CronExpressions) == 1 && j.Schedule.Spec.CronExpressions[0] == "0 9 * * *") ||
(len(j.Info.FutureActionTimes) > 0 && (strings.Contains(j.Info.FutureActionTimes[0], "T09:00") || strings.Contains(j.Info.FutureActionTimes[0], " 09:00")))
return cronPreserved &&
j.Schedule.Action.StartWorkflow.WorkflowType.Name == "SomeOtherWf" &&
j.Schedule.Action.StartWorkflow.TaskQueue.Name == "SomeOtherTq"
}, 10*time.Second, 100*time.Millisecond)
}

func (s *SharedServerSuite) TestSchedule_Update_PreservesJitterWhenNotProvided() {
schedId, schedWfId, res := s.createSchedule("--interval", "10d", "--jitter", "1m")
s.NoError(res.Err)

// Update with only non-spec flags (no --jitter). Existing jitter should be preserved.
res = s.Execute(
"schedule", "update",
"--address", s.Address(),
"-s", schedId,
"--task-queue", "SomeOtherTq",
"--type", "SomeOtherWf",
"--workflow-id", schedWfId,
)
s.NoError(res.Err)

s.Eventually(func() bool {
res = s.Execute(
"schedule", "describe",
"--address", s.Address(),
"-s", schedId,
"-o", "json",
)
s.NoError(res.Err)
var j struct {
Schedule struct {
Spec struct {
Jitter string `json:"jitter"`
} `json:"spec"`
Action struct {
StartWorkflow struct {
WorkflowType struct {
Name string `json:"name"`
} `json:"workflowType"`
} `json:"startWorkflow"`
} `json:"action"`
} `json:"schedule"`
}
s.NoError(json.Unmarshal(res.Stdout.Bytes(), &j))
// Jitter 1m is stored as 60s by the server
return (j.Schedule.Spec.Jitter == "60s" || j.Schedule.Spec.Jitter == "1m") &&
j.Schedule.Action.StartWorkflow.WorkflowType.Name == "SomeOtherWf"
}, 10*time.Second, 100*time.Millisecond)
}

func (s *SharedServerSuite) TestSchedule_Memo_Update() {
schedId, schedWfId, res := s.createSchedule("--memo", "bar=1")
s.NoError(res.Err)
Expand Down
Loading