Sch v1 wf

[davidporter-id-au]

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 (add workflow version clamp to the V1 scheduler's recorded version #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)

• feat: [Scheduler] re-evaluate V1 version ceiling per iteration #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?

• built
• covered by existing tests
• added new unit test(s)

[github-actions]

Claude finished @davidporter-id-au's task in 6m 11s —— View job

---

Reviewed 23 files, 9 findings.
• branch sch-v1-wf

[github-actions]

high — The v13 rollback reset also discards operator-initiated migrations, making the admin MigrateSchedule RPC a no-op under default dynamic config.

PendingMigration has two producers, and this reset can't tell them apart:

• the auto-eligibility block immediately below (s.tweakables.EnableCHASMMigration && ...)
• handleMigrateSignal (workflow.go:1172), which is what admin_handler.go:2283 signals for AdminService.MigrateSchedule

s.tweakables.EnableCHASMMigration is EnableCHASMSchedulerMigration(ns) && RolloutAccepts(key, CHASMSchedulerMigrationRolloutPercent(ns)) (fx.go:117-120). CHASMSchedulerMigrationRolloutPercent defaults to 0, so it is false for essentially every schedule in a default-configured namespace.

Sequence once MigrationHandoffFixes becomes the recorded version:

1. Operator calls AdminService.MigrateSchedule → signal → sleep() receives it → PendingMigration = true.
2. updateTweakables() runs; EnableCHASMMigration is false (rollout percent 0).
3. Next loop iteration reaches this block first and clears PendingMigration.
4. executeMigration() never runs. No error, no metric, no log — the RPC returns success and nothing happens.

The PR already knows about this: workflow_test.go:3584 TestOperatorMigrateSignalSurvivesRolloutPercentZero asserts the correct behavior and is t.Skip'd as a "known gap". A skipped test is not a mitigation for an RPC that silently stops working — and this is exactly the "activate v13" deploy this PR is staging for.

Note the activity-side guard added in activities.go:398 reads only EnableCHASMSchedulerMigration (not the rollout percent), so the two rollback guards disagree on what "rolled back" means.

Suggestion: Make the reset condition match the intent ("the operator turned migration off"), not the per-schedule rollout sampling. Thread a second reader that consults only dynamicconfig.EnableCHASMSchedulerMigration — the same predicate activities.migrationEnabled uses — and gate the reset on that:

		// if there's been a rollback, turn off the pending migration flag
		if s.hasMinVersion(MigrationHandoffFixes) && s.State.PendingMigration &&
			!s.tweakables.MigrationFeatureEnabled {
			s.State.PendingMigration = false
		}

Whichever shape you pick, the skipped test at workflow_test.go:3584 should be unskipped as the acceptance criterion.

[davidporter-id-au]

This comment misunderstands the problematic mechanism of late/retriggered migrations.

[github-actions]

med — The migrationEnabled != nil guard makes an unwired dependency fail open, which is the wrong polarity for a rollback guard.

activities is only constructed in two places: fx.go:156 (always sets migrationEnabled) and activities_test.go:33 (always sets it). So the nil branch is unreachable in production and exists only to let one test set the field to nil.

The PR's own TestMigrateScheduleToChasm_MigrationEnabledUnwired (activities_test.go) asserts that behavior and its comment says it is "the wrong polarity for a rollback guard". A test that pins a known-wrong default entrenches it: if a future refactor adds a third construction site and forgets the field, this activity silently resumes creating V2 schedules after a rollback, which is the exact failure the guard was added to prevent — and no test fails.

Suggestion: Drop the nil check so a missing dependency panics loudly instead of migrating, and delete TestMigrateScheduleToChasm_MigrationEnabledUnwired.

Suggested change

	if a.migrationEnabled != nil && !a.migrationEnabled() {
	if !a.migrationEnabled() {

[chaptersix]

seems like is should probably be if a.migrationEnabled == nil || !a.migrationEnabled()

not a big deal though. it should always not be nil

[github-actions]

med — Two new fields are persisted into the replay-critical TweakablePolicies solely to deduplicate a log warning.

VersionCeiling and VersionCeilingSet have exactly one reader in the whole tree — shouldWarnForVersionCeiling (workflow.go:1923). Neither participates in determineVersionTransition, in hasMinVersion, or in any behavior. Their only effect is to suppress a repeat logger.Warn.

The cost is not free:

• updateTweakables sets VersionCeilingSet = true unconditionally, so on the first wakeup after this deploy every running V1 scheduler workflow in the fleet fails the eq comparison against its recorded tweakables and writes a fresh MutableSideEffect marker — a fleet-wide history write for no behavior change.
• Thereafter, any ceiling edit writes another marker per schedule even when the effective version is unchanged (the common case: a ceiling above the binary default, which is precisely what this warning is about).

The asymmetry also undercuts the rationale: the sibling worker.schedulerV1VersionOverride warning at workflow.go:1916 has no dedup at all and fires on every iteration for as long as the value is misconfigured — TestDetermineVersionDiagnostics (version_ceiling_test.go:82) explicitly asserts it is logged twice for two evaluations. So a misconfigured override already produces the unbounded log stream this machinery exists to avoid for the ceiling.

Suggestion: Drop VersionCeiling and VersionCeilingSet, return only the version from determineVersionTransition, and handle both misconfiguration warnings the same way — either log unconditionally (matching the override) or dedupe both via a non-persisted scheduler field, which does not need to survive replay since MutableSideEffect callbacks don't run then.

[davidporter-id-au]

already discussed. I don't like having random bools around, but this was added due to the ambiguity between zero value for version and unset. It seemed the lesser of the two evils

[github-actions]

med — 225 lines of integration test are added that never execute, including two helpers nothing references and comments citing a file that isn't in the repo.

This is the only test in the +225 hunk, and it is t.Skip'd unconditionally. The five helpers below it (requireNoChasmSentinel, createV1Schedule, awaitRunningAction, awaitV1SchedulerCompleted, requireNoOptionsUpdatedEvent) are reachable only from it, so none of them run either. On top of that:

• awaitAnyAction (line 2465) and requireV2ScheduleExists (line 2525) are not referenced from anywhere in tests/ — dead on arrival.
• The comments at lines 2506 and 2516 point readers at repros/scheduler-migration-bug-evidence.md; there is no repros/ directory in the repo, so the pointer is a dead end for anyone triaging the skip later.

None of this exercises the V1 workflow changes this PR is about, and it will silently rot: the skip message describes a bug in the admin migration path, not something this PR fixes or regresses.

Suggestion: Drop the skipped test, its five exclusive helpers, and the two unreferenced ones from this PR, and track the admin-path EVENT_TYPE_WORKFLOW_EXECUTION_OPTIONS_UPDATED bug as an issue instead. If it must stay, at minimum delete awaitAnyAction and requireV2ScheduleExists and replace the repros/... references with the issue link.

[davidporter-id-au]

This was mostly documentation for a repro of an issue identified with the Coinbase SDK, it's here as a form of documentation

[github-actions]

small — The workflow-ID derivation now needs three interacting conditionals (one of them a double negative) to express two cases.

The "" sentinel plus the (!s.hasMinVersion(MigrationHandoffFixes) || start.WorkflowId == "") re-test makes the reader reconstruct the invariant that the suffix must be suppressed exactly when the migrated ID was used — which is the one thing that must not be got wrong here, since a doubled suffix breaks dedup against V2's run (TestMigratedBufferedStartSkipsTimestampSuffixForNonAllowAll). Nesting the fallback makes it structural instead of inferred.

Suggestion:

Suggested change

	workflowID := ""
	if s.hasMinVersion(MigrationHandoffFixes) {
	workflowID = start.WorkflowId
	}
	if workflowID == "" {
	workflowID = newWorkflow.WorkflowId
	}
	if (!s.hasMinVersion(MigrationHandoffFixes) || start.WorkflowId == "") &&
	(start.OverlapPolicy == enumspb.SCHEDULE_OVERLAP_POLICY_ALLOW_ALL || s.tweakables.AlwaysAppendTimestamp) {
	// must match AppendedTimestampForValidation
	workflowID += "-" + nominalTimeSec.Format(time.RFC3339)
	}
	workflowID := start.WorkflowId
	if !s.hasMinVersion(MigrationHandoffFixes) || workflowID == "" {
	workflowID = newWorkflow.WorkflowId
	if start.OverlapPolicy == enumspb.SCHEDULE_OVERLAP_POLICY_ALLOW_ALL || s.tweakables.AlwaysAppendTimestamp {
	// must match AppendedTimestampForValidation
	workflowID += "-" + nominalTimeSec.Format(time.RFC3339)
	}
	}

[github-actions]

small — The recordedVersion log tag carries the binary default, not the recorded version, and it disagrees with the value the warning was decided on.

defaultVersion is the incoming CurrentTweakablePolicies.Version. The version actually recorded for this run is s.tweakables.Version. Worse, the predicate compares the ceiling against resolveVersionBeforeCeiling(defaultVersion, override) while the log reports the un-overridden defaultVersion — so with an active override the message claims "ceiling above the version this binary records" against a number that is not the version it decided on. Anyone reading this warning to work out whether their ceiling is doing anything gets a misleading number.

Suggestion: Log the value the decision used, under an accurate name.

Suggested change

	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)
	requestedVersion := resolveVersionBeforeCeiling(defaultVersion, override)
	if shouldWarnForVersionCeiling(s.tweakables, requestedVersion, ceiling) {
	s.logger.Warn("worker.schedulerV1VersionCeiling above the version this binary records; no effect",
	"ceiling", ceiling, "requestedVersion", requestedVersion, "recordedVersion", s.tweakables.Version)

[github-actions]

small — TestDetermineVersionTransition recomputes the expected value with the same algorithm as the code under test, so it cannot detect a wrong specification.

The wantVersion block is a line-for-line restatement of resolveVersion + max(..., recordedVersion). It will pass for any implementation that agrees with itself, including one whose precedence is wrong (e.g. clamp-before-override instead of override-before-clamp, or a ceiling that can lower the recorded version). Those precedence choices are the entire point of the function and the whole reason this change carries nondeterminism risk.

The four-deep loop also runs ~1,400 iterations to assert capturedCeiling == ceiling, which is a pass-through.

Suggestion: Replace the mirrored oracle with a table of hand-written cases naming the property each pins — e.g. override promotes past the default; override below the default is ignored; ceiling caps an override; a lowered ceiling does not downgrade recordedVersion; ceiling above Latest is a no-op. A dozen explicit rows document the contract and would fail if the precedence were inverted.

[davidporter-id-au]

I think this is true and worth a followup, I wasn't going to block on it

[github-actions]

small — The same 11-line rationale is repeated verbatim on appendSortedTriggerStarts's doc comment, and both copies are written as an argument against an alternative rather than a description of the code.

Lines 193-202 and 239-245 say the same thing twice, ~20 comment lines for an 8-line function, and both are framed as "why not the other design" ("so simulating if CHASM kept running means...", "not repositioned into it by time"). Per the repo's comment guidance, a comment should describe the code, not relitigate the decision, and should not stack subordinate clauses.

Two load-bearing facts are in there and are worth keeping once: ProcessBuffer takes the first matching entry in list order without comparing ActualTime, and a pending trigger Backfiller is only ever appended by Invoker.EnqueueBufferedStarts when its task runs. Everything else is commentary. The three tests added in migration_test.go already carry the scenario-level explanation.

(Separately: the PR description says this fix "sort[s] the combined list by ActualTime after appending" — the opposite of what the code and tests do. Worth correcting before merge so the commit message doesn't contradict the change.)

Suggestion: Keep one short call-site comment and reduce the function doc to a single line.

Suggested change

	// load-bearing -- it's fed into the same ProcessBuffer V1 uses, where BUFFER_ONE and
	// "nothing running" both take whichever entry comes first in iteration order, never
	// comparing ActualTime. A pending trigger Backfiller hasn't been enqueued yet --
	// BackfillerTaskHandler.processTrigger builds its single BufferedStart and only appends it
	// via Invoker.EnqueueBufferedStarts once its task actually executes, regardless of the
	// trigger's own ActualTime -- so simulating "if CHASM kept running" means every still-pending
	// trigger belongs after whatever's already buffered, not repositioned into it by time.
	// triggerStarts are sorted only among themselves (built from a randomized map iteration, so
	// they have no defined relative order of their own) purely for a deterministic tie-break,
	// then appended after bufferedStarts unchanged.
	bufferedStarts = appendSortedTriggerStarts(bufferedStarts, triggerStarts)
	// V1's ProcessBuffer takes the first entry in list order without comparing ActualTime, so
	// bufferedStarts' enqueue order must be preserved. A pending trigger Backfiller has not been
	// enqueued yet -- Invoker.EnqueueBufferedStarts appends it only when its task runs -- so it
	// belongs after everything already buffered.
	bufferedStarts = appendSortedTriggerStarts(bufferedStarts, triggerStarts)

[github-actions]

nit — The recentFromInfo variable and the second if add a branch without changing behavior; the block was split only to make room for a comment.

The original single if len(info.GetRecentActions()) > 0 block did exactly this. Now the same predicate is tested twice with convertBackfillersCHASMToLegacy moved in between, purely so the sort/truncate can be introduced by a comment. That leaves a reader checking whether the two halves can ever disagree.

Suggestion: Restore the single block and keep ongoingBackfills, triggerStarts := ... after it; the sort comment reads the same inside the block.

[github-actions]

small — versionReleases entries are only checked for file existence, so the version→snapshot mapping this test exists to guarantee is unverified for v0–v12.

versionFixtures gets the real check (fixtureTweakablesVersions must contain the version), but versionReleases gets only os.Stat. So mapped[v] = true claims coverage for v0 through v12 on the strength of a filename.

That matters here specifically: this PR renames replay_v1.23-pre.json.gz → replay_v1.23.0.json.gz and maps it to DontTrackOverlapping, and adds four new snapshots each mapped to two or three versions. Nothing checks that any of those files actually record the version claimed — a snapshot captured from the wrong build, or a mislabelled rename, passes silently while TestEveryVersionIsMapped reports full coverage.

Suggestion: Apply fixtureTweakablesVersions to the release snapshots too, for the versions that can record one. Version only exists from BatchAndCacheTimeQueries on, and v4–v6 / v7–v8 / v9–v10 / v11–v12 share a file, so the check is "the snapshot records some version in this release's set":

	for _, vr := range versionReleases {
		mapped[vr.version] = true
		path := filepath.Join("testdata", "replay_"+vr.server+".json.gz")
		_, err := os.Stat(path)
		require.NoErrorf(t, err, "missing snapshot replay_%s.json.gz", vr.server)
		if vr.version < scheduler.BatchAndCacheTimeQueries {
			continue // predates the Version field
		}
		require.Containsf(t, releaseVersions(vr.server), maxVersionForRelease(vr.server),
			"snapshot replay_%s.json.gz does not record the highest version of its release", vr.server)
	}

Even just asserting the highest version per release would have to be right for the rename and the four new files.

[chaptersix]

not a big deal / not worth blocking on

[chaptersix]

A lot of nits that I don't think are all that important.

[davidporter-id-au]

Manual testing done, looks good