fix: V1 schedule version bump + Migration fixes + CGS version guards - #11633
Closed
davidporter-id-au wants to merge 8 commits into
Closed
fix: V1 schedule version bump + Migration fixes + CGS version guards#11633davidporter-id-au wants to merge 8 commits into
davidporter-id-au wants to merge 8 commits into
Conversation
… ID preservation under one v13 Combines PR temporalio#11134 and PR temporalio#11427, which independently introduced a new SchedulerWorkflowVersion = 13 for two different fixes. Shipping them separately would require two separate version-bump deploys for the "same" version number. This merges both behavioral changes under one shared v13: - RefreshBeforeMigrationCheck (from temporalio#11134): the V1 scheduler's automatic CHASM-migration eligibility check read len(s.Info.RunningWorkflows) before the same run-loop iteration's processBuffer() reconciled it, so an actively-firing schedule never observed an idle window and deferred migration forever. Reconcile running-workflow status before the eligibility check so it sees the genuine post-completion window. - PreserveMigratedStartIDs (from temporalio#11427): V1 discarded the workflow/request IDs already stored on buffered starts migrated from CHASM, breaking idempotency identity across the migration handoff. Prefer those IDs when present, falling back to the existing derivation/UUID generation. Following temporalio#11134's two-phase-rollout rationale, this PR only teaches the scheduler to *understand* v13 for safe replay/rollback; both fixes are gated behind hasMinVersion(13) but CurrentTweakablePolicies.Version stays at TriggerImmediatelyTimestamp (12). A follow-up deploy bumps Version to 13 to activate both fixes at once -- a single activation instead of two. Brings in temporalio#11134's replay fixture (testdata/replay_migration_v1_to_v2.json.gz) and integration test file (tests/schedule_migration_v1_to_v2_callback_compat_test.go) verbatim, and temporalio#11427's workflow.go/workflow_test.go changes, retargeting the version gate and forcing the version in TestMigratedBufferedStartPreservesIdempotencyIDs since Version no longer defaults to 13 here. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
If EnableCHASMSchedulerMigration is enabled, bounces on a transient error, and is then rolled back, the sleeping V1 scheduler workflow had no way to notice: PendingMigration is a persisted latch that, once set, retries the migration unconditionally on every wake-up regardless of the current flag value. Depending on the schedule's own cadence, that retry (and thus the migration) could fire long after the flag was believed to be off. Fix this in the local activity itself rather than in workflow code: MigrateScheduleToChasm now does a live (uncached) check of EnableCHASMSchedulerMigration right before calling CreateFromMigrationState, and fails if it's off. The workflow's retry loop is unchanged -- it just logs the failure and keeps going, so a disabled migration spins harmlessly (and resumes the moment the flag comes back on) without ever blocking the schedule's own actions. Also arm TestScheduleMigrationV1ToV2_RolloutMigration by setting CurrentTweakablePolicies.Version to MigrationHandoffFixes for the duration of the test, since that fix is still dormant pending a follow-up activation deploy. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
davidporter-id-au
changed the title
Contributor
Author
|
ignore this, my thinking wasn't all that clear |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This combines multiple fixes into a unified v1 schedule version bump. It includes:
This is being rolled into a large, bundled change, after being tested individually, because we want to minimize the number of version changes for V1 schedules.
LLM summary
This PR combines two scheduler changes that need to agree on the recorded V1 scheduler workflow version:
worker.schedulerV1VersionCeiling, allowing a newer cluster to clamp the version recorded in scheduler history for compatibility with an older failover/rollback peer.The combined result teaches the scheduler all v13 behavior while leaving
CurrentTweakablePolicies.Versionat 12. Version 13 can therefore be deployed safely before it is activated, and the ceiling can continue to hold selected namespaces at an older version.Why migration must require version 13
The version ceiling needs to be a semantic ceiling, not just a different number in
TweakablePolicies. A scheduler clamped to version 12 must behave like the historical version-12 scheduler throughout that run.Before the merge resolution, CHASM migration configuration and signals could still influence a clamped version-12 execution. In particular, the workflow could register and consume the migration signal, persist migration-related tweakables, or execute pending migration work even though v1.29/version 12 did not understand those behaviors. That would let a supposedly compatible execution write newer decisions into history and defeat the purpose of the ceiling.
This PR therefore gates the complete migration boundary on
MigrationHandoffFixes(13):Version 13 is also the shared activation point for the two fixes in #11462. Keeping them behind one version avoids two workflow-version activation deploys and ensures rollback/idempotency behavior changes together with migration eligibility.
Testing and verification
go test -tags test_dep ./service/worker/scheduler -count=1TEMPORAL_SDK_FLAG_7=0 GENERATE_SCHEDULER_V129_COMPATIBILITY_HISTORY=1 go test -tags integration,test_dep ./tests -run '^TestGenerateSchedulerV129CompatibilityHistory$' -count=1make lint-codepassed with no issues.The
TEMPORAL_SDK_FLAG_7=0setting in the focused reverse check is necessary because the current Go SDK records its newer memo-encoding flag by default, while the Go SDK used by Temporal v1.29 rejects that unknown SDK flag before scheduler replay begins. The scheduler version ceiling controls scheduler workflow behavior; it does not make newer SDK-internal flags understandable to an older SDK.Potential risks
Scheduler workflow changes carry nondeterminism risk. The main protection here is that every migration-visible decision is behind the same recorded v13 boundary, and the default remains version 12 until a separate rollout activates v13. Operators must also configure the ceiling no higher than the oldest failover/rollback peer they intend to support.