feat: [Scheduler] add worker.schedulerV1VersionOverride - #11856
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Claude finished @chaptersix's task in 3m 9s —— View job Reviewed 6 files, 4 findings. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
662eac6 to
49984ac
Compare
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0d72047da1
ℹ️ 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 override > int(LatestSchedulerWorkflowVersion) { | ||
| s.logger.Warn("worker.schedulerV1VersionOverride above the latest supported version; ignored", | ||
| "override", override, "latestSupportedVersion", LatestSchedulerWorkflowVersion) |
There was a problem hiding this comment.
When a namespace sets an override above the latest supported version, determineVersion runs during every tweakables MutableSideEffect evaluation for every schedule, so this warning is emitted again at every wakeup. A single invalid namespace-level value can therefore flood logs and add avoidable load across all active schedules until corrected; track the last-seen override and warn only when it changes, as the ceiling handling immediately above already does.
Useful? React with 👍 / 👎.
| if override >= int(v) && override <= int(LatestSchedulerWorkflowVersion) { | ||
| v = SchedulerWorkflowVersion(override) |
There was a problem hiding this comment.
When an operator sets this override to 13 or 14, enables EnableCHASMSchedulerMigration, leaves CHASMSchedulerMigrationRolloutPercent at its default 0, and signals one schedule for migration, the v13 rollback logic clears PendingMigration before the activity runs because the workflow-side migration flag includes the rollout decision. This is already demonstrated by the skipped TestOperatorMigrateSignalSurvivesRolloutPercentZero; previously the shipped default remained v12, but accepting the override now makes the broken v13 path reachable in production and silently drops operator-requested migrations.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b5808cfe6a
ℹ️ 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".
b5808cf to
d91a34e
Compare
> **Stack 1/2** (base `sch-v1-wf`): **#11831** -> #11856. ## What changes relative to `main` `main` records the static `CurrentTweakablePolicies.Version` in the scheduler workflow `tweakables` `MutableSideEffect`; it is currently v12. There is no namespace dynamic config for choosing a V1 scheduler workflow version. A running workflow can advance only when a later binary changes that static default. The base branch adds `worker.schedulerV1VersionCeiling`, but selects the capped version only on the first `tweakables` evaluation. Raising or removing a v11 ceiling therefore leaves an in-flight workflow at v11 until continue-as-new. This PR re-reads the ceiling on every `tweakables` evaluation. The pure transition is: ```text next version = max(recorded version, min(binary default, current ceiling)) ``` A negative ceiling is unset. The version remains monotonic, while the ceiling is captured for the current evaluation. ## Result With a binary default of v12: ```text first evaluation: ceiling=11 -> records version=11, ceiling=11 next wakeup: ceiling=-1 -> records version=12, ceiling=-1 ``` The scheduler advances at that next wakeup; it does not wait for continue-as-new. Conversely, lowering a ceiling after a run has already recorded a higher version does not downgrade that run. The new, lower ceiling is recorded and applies when a fresh run starts. ## Replay compatibility `MutableSideEffect` records each selected version/ceiling result, so replay consumes history rather than live dynamic config. Older markers that predate the ceiling field remain readable; a later live evaluation records the current ceiling. The recorded version remains the monotonic floor throughout. ## Scope A ceiling can restrict the binary default but cannot promote beyond it. The child PR #11856 adds the explicit override needed to select a supported version such as v14 before a later release changes the static default. ## Coverage The pure transition table covers unset and zero ceilings, capping, retaining an already-recorded version under a newly lower ceiling, and advancement when the ceiling is raised or removed. Workflow coverage verifies that lifting the ceiling advances and permits migration within the same run.
d91a34e to
66f25ee
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 66f25ee652
ℹ️ 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 shouldWarnForVersionCeiling(s.tweakables, resolveVersionBeforeCeiling(defaultVersion, override), ceiling) { | ||
| s.logger.Warn("worker.schedulerV1VersionCeiling above the version this binary records; no effect", | ||
| "ceiling", ceiling, "recordedVersion", defaultVersion) |
There was a problem hiding this comment.
The current revision accounts for the live override, but after a run has recorded v14, removing the override and setting the ceiling to 13 still enters this branch because it compares 13 with the default v12. The warning consequently claims the ceiling is above the recorded version and reports recordedVersion=12, even though the run actually retains v14 because determineVersionTransition cannot downgrade it. This misleads operators attempting to restore rollback compatibility; compare against s.tweakables.Version and emit the retained-version diagnostic instead.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0895ea3f8c
ℹ️ 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".
| // TestEveryVersionIsMapped verifies that each fixture records the version it represents. | ||
| var versionFixtures = map[scheduler.SchedulerWorkflowVersion]string{ | ||
| scheduler.MigrationHandoffFixes: "replay_migration_v1_to_v2.json.gz", | ||
| scheduler.RefreshCompletionDesiredTime: "replay_version_override_v14.json.gz", |
There was a problem hiding this comment.
The newly added fixture only proves that a v14 marker exists: its sole LocalActivity is StartWorkflow, followed by force-continue-as-new, with no WatchWorkflow completion. Replay therefore never reaches the only v14 behavior at workflow.go:1075-1078 (RefreshCompletionDesiredTime), so a nondeterministic change in that branch would still pass both replay tests. This is fresh evidence beyond the earlier missing-fixture finding; capture a history where refresh discovers a completion while a start is buffered.
Useful? React with 👍 / 👎.
## Summary We held back several fixes in the V1 schedule workflow and bundled them into a single atomic commit with the intention of avoiding having to perform multiple increments of the workflow versioning. The changes are broadly: Migration v1->v2 fixes + CGS version changes. ## Risk This touches V1 schedules workflow, and a mistake risks nondeterminitism. This is relatively high impact and has quite a bit of subtlety. This is therefore a commit we need to merge with care ## Testing and validation 1. Changes have been manually tested on individual PRs already 2. Changes have been manually run through CGS's validation suite (cc @liam-lowe) Our intention is to a) make the 163 release cut and b) continue to manually testing a few operational scenarios manually while this is being merged in. If we see problems we can hotfix this. Manually doing some scenario testing will take time so we intend to do this in parallel. Some of the scenarios we will test manually are: 1. Deploy and rollback to an earlier version (probably with LLM assist, but running locally) 2. Manually lifting the the version of a schedule via dynamic config (manually) ensuring there's no nondeterminitism risk 3. V2->v1 Rollback works as expected Some of the tests we've already run and manually validated(cc @liam-lowe) 1. Version floor works as expected 2. That this change fixes v1->v2 migration 3. That this change avoids the problematic history entries which broke a customer using the coinbase Ruby SDK ## LLM Summary This PR stacks a series of changes to the V1 (legacy) scheduler workflow: two migration-correctness fixes, and dynamic-config levers to safely roll the V1 workflow version forward/backward across a multi-cluster deployment without requiring continue-as-new. Summary of each PR in the stack, in order: ### #11462 — V1→V2 migration-eligibility fix and migrated-start ID (David Porter) Combines two previously separate fixes under one shared version bump (`v13`), avoiding two separate version-bump deploys for the same version number: - `RefreshBeforeMigrationCheck`: fixes a bug that was preventing V1→V2 migration from ever succeeding under default configuration. - `PreserveMigratedStartIDs`: preserves the request IDs workflows were originally started with, in case of a rollback. - Adds a guard against late migrations caused by a transient error bouncing a migration attempt and the schedule re-attempting it after waking again. - Follows the existing two-phase-rollout pattern: this PR only teaches the scheduler to *understand* v13 (gated behind `hasMinVersion(13)`); `CurrentTweakablePolicies.Version` itself stays at v12 pending a follow-up activation deploy. ### #11588 — Fix schedule action delay after refresh (Alex Stanfield) - `processWatcherResult` only recorded `DesiredTime` on the long-poll path; when a refresh instead discovered the prior action had completed, `DesiredTime` stayed unset, inflating the reported `ScheduleActionDelay` for back-to-back buffered actions. - Backdates `DesiredTime` on the refresh path too, gated behind a new version, `RefreshCompletionDesiredTime` (v14). - Handles two follow-on correctness gaps surfaced in review: `ALLOW_ALL` starts (never blocked by a running workflow, so shouldn't be backdated to an unrelated close time) and multiple tracked executions in one refresh pass (must only move the recorded close time forward, never backward). - Extracts the decision into a pure, unit-testable `shouldBackdateDesiredTime` function, and a shared `IgnoresRunningWorkflow` helper so `ProcessBuffer` and the new backdate logic can't drift apart on what "waits for a running workflow" means. ### #11827 — Sort BufferedStarts by due time on CHASM-to-V1 rollback (Alex Stanfield) - `CHASMToLegacyStartScheduleArgs` (the CHASM→V1 rollback conversion) appended trigger-derived `BufferedStarts` after the regular pending ones unconditionally, without sorting by due time. - V1's buffer-processing code assumes `BufferedStarts[0]` is always the earliest-due pending start — an invariant not guaranteed across a rollback, since manual triggers are built by iterating a Go map (randomized order) and appended regardless of their own due time. - Fix: sort the combined list by `ActualTime` after appending, mirroring the sort already applied to `RecentActions` a few lines above. ### #11831 — Re-evaluate V1 version ceiling per iteration (Alex Stanfield) - The base PR (#10817, by liam-lowe) added `worker.schedulerV1VersionCeiling` but only applied it on the first `tweakables` evaluation, so raising/removing a ceiling left an in-flight workflow stuck at the capped version until continue-as-new. - Re-reads the ceiling on every evaluation: `next version = max(recorded version, min(binary default, current ceiling))`. The recorded version stays monotonic — a newly-lowered ceiling never downgrades a version already recorded in the current run, but a raised/removed ceiling lets the workflow advance at its next wakeup instead of waiting for continue-as-new. - Recorded via the existing `MutableSideEffect`, so replay consumes history rather than re-evaluating live dynamic config. ### #11856 — Add worker.schedulerV1VersionOverride (Alex Stanfield) - #11831 makes the ceiling dynamic, but a ceiling can only restrict a binary default, never promote past it. Adds the namespace-level override needed to activate a newer version already supported by the binary. - `requested version = valid override, otherwise binary default`; `next version = max(recorded version, min(requested version, current ceiling))`. Default `-1` retains the binary default; values below the binary default or above `LatestSchedulerWorkflowVersion` are ignored. - The frontend uses the same override for initial schedule memo/list-info construction, so version-dependent metadata agrees with the first worker task. Base of the stack: #10817 (liam-lowe) introduced the original (static-per-run) `worker.schedulerV1VersionCeiling` dynamic config that #11831/#11856 build on. ## Why? To support safe, gradual rollout/rollback of V1 scheduler workflow version bumps (migration fixes, action-delay-after-refresh fix) in a cross-version multi-cluster topology, without requiring continue-as-new to pick up config changes. ## How did you test it? - [x] built - [x] covered by existing tests - [x] added new unit test(s) --------- Co-authored-by: liam-lowe <56076876+liam-lowe@users.noreply.github.com> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> Co-authored-by: alex.stanfield <13949480+chaptersix@users.noreply.github.com> Co-authored-by: Stephan Behnke <stephanos@users.noreply.github.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: michaely520 <michaely520@users.noreply.github.com> Co-authored-by: Feiyang Xie <feiyang3cat@outlook.com> Co-authored-by: Kannan <rkannan82@users.noreply.github.com> Co-authored-by: Fred Tzeng <41805201+fretz12@users.noreply.github.com> Co-authored-by: Lakshay <54310363+Lakshaymiddha@users.noreply.github.com> Co-authored-by: samm <sam.mathis@temporal.io> Co-authored-by: Quinn Klassen <klassenq@gmail.com> Co-authored-by: Will Duan <xinw.duan@gmail.com> Co-authored-by: Qian Chen <qyc5937@gmail.com> Co-authored-by: Prathyush PV <prathyush.pv@temporal.io> Co-authored-by: Sean Kane <sean.kane@temporal.io> Co-authored-by: mavemuri <74267563+mavemuri@users.noreply.github.com> Co-authored-by: Rodrigo Zhou <rodrigo.zhou@temporal.io> Co-authored-by: Brian VanLoo <brian.vanloo@gmail.com> Co-authored-by: akbala <akbala@gmail.com> Co-authored-by: Dan Davison <dan.davison@temporal.io> Co-authored-by: Chris Smith <chrsmith@users.noreply.github.com>
2 participants
Purpose
#11831 makes a ceiling dynamic, but a ceiling can only restrict a default. If the binary default is v12,
min(v12, v14)is still v12. This PR adds the namespace-level promotion control required to activate a newer behavior already supported by the binary.What changed
Adds
worker.schedulerV1VersionOverride. Its default-1retains the binary default. A value from the current default throughLatestSchedulerWorkflowVersionis valid; values below the default or above the versions supported by this binary are ignored.The override is re-read during every scheduler
tweakablesMutableSideEffectevaluation. It is selected before the ceiling from #11831, and the version already recorded by the current run remains a floor:With the static default at v12 and this binary supporting v14:
If a v14 override is already configured while a v12 ceiling is active, lifting the ceiling lets the workflow record v14 at its next wakeup. Lowering/removing the override, or lowering the ceiling, never downgrades a version already recorded in that run; the lower value applies to a fresh run after continue-as-new.
The frontend uses the same override for initial schedule memo/list-info construction, so version-dependent metadata agrees with the first worker task.
Determinism and coverage
The selected result is recorded by the existing
tweakablesMutableSideEffect; replay uses that marker rather than live dynamic config. A single dynamic-config struct supplies the workflow dependencies, and named version-selection fields make the frontend handoff explicit. The pure transition table covers promotion to the latest supported version, ceiling precedence, and invalid overrides. Workflow coverage verifies that an override advances within the same run when a prior ceiling is lifted.