Fix: v1->V2 schedules migration guard & fix#11134
Open
davidporter-id-au wants to merge 2 commits into
Open
Conversation
…g schedules The V1 scheduler's automatic CHASM-migration eligibility check read len(s.Info.RunningWorkflows) before the same run-loop iteration's processBuffer() reconciled it. processTimeRange sets NeedRefresh when it buffers a new action, but the refresh that prunes completed executions only runs inside processBuffer -- after the check, which also starts the replacement action. For a continuously-firing schedule the check therefore always saw the previous action as still running, so len(RunningWorkflows)==0 was never observed and, under the default EnableCHASMSchedulerMigrationWithRunningWorkflows=false guard, migration was deferred forever. Reconcile running-workflow status before the eligibility check so the check sees the genuine post-completion window and migrates before the next action starts. Gated behind a new SchedulerWorkflowVersion (RefreshBeforeMigrationCheck = 13) because refreshWorkflows issues recorded local activities and reordering it would otherwise break replay of existing histories. Tests: - workflow_test.go: TestAutoMigrateReconcilesRunningWorkflowBeforeCheck unit guard (fails without the version bump). - testdata/replay_migration_v1_to_v2.json.gz: new replay fixture exercising the v13 migration branch; TestReplays fails if the early-refresh block is reordered/removed. Regenerate via tests/schedule_migration_replay_fixture_gen_test.go (GENERATE_REPLAY_FIXTURE=1, inert in CI otherwise). - tests/schedule_migration_v1_to_v2_callback_compat_test.go: integration coverage; _AutomaticMigrationForActiveSchedule now passes. _NoUnsupportedHistoryEventOnRunningWorkflow stays skipped (separate admin-path callback-attach bug). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
| // which older SDKs cannot decode. This is EXPECTED TO FAIL on main today; | ||
| // remove this skip once the retroactive-attach behavior is fixed so the test | ||
| // arms. | ||
| t.Skip("regression guard for the admin-path V1->V2 migration callback-attach bug: it " + |
Contributor
Author
There was a problem hiding this comment.
This is not fixed. Manually calling 'migrate' via the CLI might cause problems.
Addresses the PR review notes on the callback-compat tests and clears the golangci failures on the changed files. Test unification (schedule_migration_v1_to_v2_callback_compat_test.go): - Extract shared helpers (createV1Schedule, awaitRunningAction, awaitAnyAction, awaitV1SchedulerCompleted, requireNoOptionsUpdatedEvent, requireV2ScheduleExists) to kill the ~4x-duplicated boilerplate. - Rename the admin-path test to TestScheduleMigrationV1ToV2_AdminMigratePreservesRunningWorkflowHistory (it is the tdbg/CLI poke-migration flow) and tighten the skip reason: the admin MigrateSchedule path still injects the unsupported event and is genuinely unfixed. - Merge the two dynamic-config tests into TestScheduleMigrationV1ToV2_RolloutMigration, which asserts both that a busy schedule migrates (regression guard for the ordering fix in this PR) and that no fired workflow gains EVENT_TYPE_WORKFLOW_EXECUTION_OPTIONS_UPDATED. The old single-action + refresh-nudge scaffolding existed only to sidestep the ordering bug this PR fixes, so it is dropped along with the stale "fails on main" framing and the local-path reference. Lint (golangci diff-mode now clean on the changed files): - Put ctx first in helpers (revive context-as-argument). - require.Eventually -> await.RequireTrue/RequireTruef in the rollout test and the replay-fixture generator (forbidigo). - s.NoError -> s.Require().NoError in the new workflow_test guard (testifylint require-error). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
| }) | ||
| require.NoError(t, err) | ||
| for _, event := range history.GetHistory().GetEvents() { | ||
| require.NotEqual(t, enumspb.EVENT_TYPE_WORKFLOW_EXECUTION_OPTIONS_UPDATED, event.GetEventType(), |
Contributor
Author
There was a problem hiding this comment.
callout here: This is important: This is (at least) one of the events not supported by SDKs like the ruby/coinbase SDK. We should not see this in history
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
1 participant
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 somewhat large-ish PR contains the following:
Details
The V1 scheduler's automatic CHASM-migration eligibility check read len(s.Info.RunningWorkflows) before the same run-loop iteration's processBuffer() reconciled it. processTimeRange sets NeedRefresh when it buffers a new action, but the refresh that prunes completed executions only runs inside processBuffer -- after the check, which also starts the replacement action. For a continuously-firing schedule the check therefore always saw the previous action as still running, so len(RunningWorkflows)==0 was never observed and, under the default
EnableCHASMSchedulerMigrationWithRunningWorkflows=false guard, migration was deferred forever.
Reconcile running-workflow status before the eligibility check so the check sees the genuine post-completion window and migrates before the next action starts. Gated behind a new SchedulerWorkflowVersion (RefreshBeforeMigrationCheck = 13) because refreshWorkflows issues recorded local activities and reordering it would otherwise break replay of existing histories.
Tests:
How did you test it?
Potential risks
This change touches v1 Scheduler code on the main-line path. A mistake here could cause serious problems with nondeterminitism and therefore breaking Schedules. Considerable care needs to be taken in the change, that it must be versioned.