Preserve a paused deployment schedule on redeploy#22396
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8ef8a3d586
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| """ | ||
| if slug: | ||
| return f"slug:{slug}" | ||
| definition = schedule.model_dump_json() if schedule is not None else "null" |
There was a problem hiding this comment.
Avoid matching on generated interval anchors
Matching slug-less schedules on the full schedule.model_dump_json() makes preservation depend on fields that Prefect regenerates during deployment construction. For a common redeploy like flow.deploy(..., interval=3600) or Interval(3600) without an explicit anchor_date, the client creates a fresh anchor timestamp each time, so the serialized definition no longer matches the stored schedule and _resolve_schedule_active falls back to True. In that case a schedule paused in the UI is still silently reactivated on redeploy even though active was omitted.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@apoorva-01 this review comment will need to be addressed before we can merge these changes
There was a problem hiding this comment.
anchor_date (and the injected rrule DTSTART) get regenerated every build, so the full-dump key never matched on redeploy and the paused schedule flipped back on.
Fixed in 75dbbb2, matching on the recurrence definition without those fields, plus an interval regression test.
Merging this PR will not alter performance
Comparing Footnotes
|
Re-deploying without an explicit active silently re-activated a paused slug-less schedule: the client always sent active=True and the server defaulted it to True on update. Leave active unset and inherit it.
8ef8a3d to
18b4d90
Compare
The previous match keyed on the full serialized schedule, which includes fields Prefect regenerates on each deploy: an interval schedule's anchor_date and the DTSTART injected into rrule strings. A redeploy of an unchanged `Interval(3600)` therefore failed to match and reactivated a paused schedule. Key on the recurrence definition with those regenerated fields excluded instead.
closes #19302
Pause a deployment schedule, re-
.deploy()without passingactive, and it silently turns back on (only for slug-less schedules).Two things combine, so the fix is on both sides. The client always sent
active=Truewhen the caller didn't set it, so theexclude_unset=Truepayload still carried it. And on update the server rebuilds slug-less schedules from scratch and defaultedactivetoTrue. (OSS resets too; the slug workaround in the issue looks Cloud-specific.)What changed
activeis left unset when the caller doesn't pass it (the deployment-schedule builders and theSchedule/Cron/Interval/RRulefactories), so an update no longer forcesTrue. On update the server keeps the existing schedule'sactivewhen none is given, matched by slug or, for slug-less ones, by serialized definition. New/changed schedules still default to active; an explicitactive=still wins. Added tests for the paused-on-redeploy case (slug + slug-less, client + server); existing suites pass.Design note — your call
This flips the default of
Schedule.active(and the factoryactiveparams) fromTruetoNone. Create behavior is unchanged (unset still resolves toTrueon create);Nonejust means "leave active alone on update", but it's a public default change. The bigger alternative is fully idempotent reconciliation, larger and overlapping with the slug-rename work in #19298.