mirror of
https://github.com/temporalio/temporal.git
synced 2026-08-30 18:41:49 -07:00
0e6194c52c
## What changed? - Keep attaching completion callbacks to every CHASM scheduler workflow start, including actions whose resolved overlap policy is `ALLOW_ALL`, so start requests remain safe across rolling upgrades. - Exclude scheduler-wide last-completion result/failure input from new `ALLOW_ALL` actions. - After `StartWorkflowExecution` succeeds, remove the new `ALLOW_ALL` `BufferedStart` in the same transaction. Separately, copy its start data to `ScheduleInfo.RecentActions` as a start-only `RUNNING` history record. - Resolve an unspecified pending-start policy while building the V1-to-V2 migration request, so every V2 dispatch and retry uses the policy persisted on the buffered start. - For a pre-existing callback, use the policy stamped on that start rather than resolving against the schedule's current policy; an unstamped V1-migrated running workflow remains tracked, as it was in V1. - Preserve compatibility for previously persisted `ALLOW_ALL` callbacks without updating last-completion state or `PauseOnFailure`. - Carry start-only history through CHASM-to-V1 migration. - Merge start-only and completion-tracked history by actual start time, retaining the newest ten actions across both sources. - Re-run generation after recording a start-only action, so a finite or manual-only schedule rearms its idle timer from that start. ## Behavior This makes CHASM match modern V1: - A new `ALLOW_ALL` workflow starts normally, but is absent from `DescribeSchedule.Info.RunningWorkflows` and does not consume active-buffer capacity. - It appears in `DescribeSchedule.Info.RecentActions` (and List's recent actions) with its start time, workflow execution, and `RUNNING` status. - Its completion callback remains attached for rolling-upgrade compatibility. Because the start has already moved out of active buffered state, the callback is ignored and success or failure cannot change shared completion input or `PauseOnFailure`; its recent status therefore remains `RUNNING`. - For a final `ALLOW_ALL` action, an idle task armed before the start is invalidated by the newer start time; generation immediately arms its replacement, so the schedule still closes after `IdleTime`. - If an older handler retained an `ALLOW_ALL` start, its callback remains a compatibility path: the terminal action record is retained, but its completion cannot update shared completion state or pause the schedule once handled by this version. - `DescribeSchedule.Info.RecentActions` is ordered by actual start time and bounded to the newest ten actions across start-only and completion-tracked history. Newer retained actions evict the oldest; completion does not remove a start-only action. - Non-`ALLOW_ALL` actions remain active and appear in `RunningWorkflows` until their completion is handled. - A V1 pending start with an unspecified override snapshots the schedule's effective policy when its V2 migration request is built. - A V1-migrated `RunningWorkflows` entry has an unspecified policy but is nevertheless tracked: its completion updates sequential state and may pause the schedule, regardless of the schedule's current `ALLOW_ALL` default. ## Why? `ALLOW_ALL` actions are independent executions. Tracking their completions made scheduler-wide last result/failure and `PauseOnFailure` depend on callback arrival order, and retaining them as active could affect overlap and capacity behavior. Callbacks remain attached because `StartWorkflowExecution` deduplicates by request ID without reconciling callback differences. Keeping the request callback-compatible prevents a mixed-version retry from retaining a start that waits for a callback the workflow never received. ## How did you test it? - [x] added unit coverage - [x] added shared V1/CHASM functional coverage Commands: - `go test -tags test_dep ./chasm/lib/scheduler/... -count=1` - `go test -tags test_dep ./tests -run '^TestSchedule(CHASM|V1)/TestAllowAllDescribeContract$' -count=1` - `make fmt-imports` - `git diff --check` - `env GOCACHE=/tmp/sch-038-gocache go vet -tags disable_grpc_modules,,test_dep -vettool=.bin/errortype -style-check=false ./chasm/lib/scheduler/...` The functional test uses workflow signals to control completion. It asserts counters, buffer size, recent-action status/timestamps, active workflows, and failure/pause isolation across both backends. A CHASM functional idle-close case verifies that a final `ALLOW_ALL` action still closes after `IdleTime`. The migration regression and callback-reason metric test drive real component transactions through the CHASM test engine. `make lint-code` currently exits before analysis with `no go files to analyze` from its `--new-from-rev` filter, despite the Go diff; package `go vet` is clean. ## Additional observability Ignored callbacks are tagged as either `unrecognized_request_id` or `already_completed`. A newly started `ALLOW_ALL` callback is expected to be unrecognized after its buffered start moves to start-only history. An already-completed callback is a valid redelivery (for example, after a workflow reset). ~~Both preserve scheduler state while emitting a warning and counter increment.~~ Missing request IDs are now metric-only because they include expected `ALLOW_ALL` callbacks; known `already_completed` redeliveries still emit the warning, event, and counter. ## Potential risks Keeping callbacks attached avoids permanently orphaning buffered starts when old and new binaries race on the same request ID. During a rolling upgrade, however, old and new requests still differ in whether scheduler-wide last-completion result/failure input is included for `ALLOW_ALL`; the request that wins deduplication determines whether that workflow receives the legacy input. An older callback handler can also temporarily apply the legacy completion and pause semantics. These mixed-version differences end once the rollout completes, and the callback ensures an older handler cannot wait indefinitely. New `ALLOW_ALL` terminal status is intentionally not reflected in schedule Describe/List results, as in modern V1. ~~Its expected late callback is recorded as `unrecognized_request_id`, which adds callback delivery plus warning/metric volume compared with omitting callbacks.~~ Its expected late callback remains recorded as `unrecognized_request_id`, but only as a metric; warning and event logging are suppressed until these callbacks can be safely omitted. Migration resolves an unspecified pending-start policy at the V1-to-V2 boundary. If the schedule policy changes while that start remains pending and the schedule then rolls back to V1, the explicit migrated policy is preserved instead of inheriting the newer schedule policy. This is a narrow semantic difference that keeps V2 dispatch and retry behavior stable. --------- Co-authored-by: David Porter <david.porter@temporal.io> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
98 lines
3.0 KiB
Go
98 lines
3.0 KiB
Go
package scheduler
|
|
|
|
import (
|
|
"context"
|
|
"time"
|
|
|
|
schedulespb "go.temporal.io/server/api/schedule/v1"
|
|
"go.temporal.io/server/chasm"
|
|
"go.temporal.io/server/chasm/lib/scheduler/gen/schedulerpb/v1"
|
|
"go.temporal.io/server/common/log"
|
|
legacyscheduler "go.temporal.io/server/service/worker/scheduler"
|
|
)
|
|
|
|
// Export unexported methods for testing.
|
|
|
|
// ExecutionStatus search-attribute values, exported for tests.
|
|
var (
|
|
ExecutionStatusRunning = executionStatusRunning
|
|
ExecutionStatusCompleted = executionStatusCompleted
|
|
)
|
|
|
|
func NewTestHandler(logger log.Logger) *handler {
|
|
return newHandler(logger, legacyscheduler.NewSpecBuilder(func() int { return 0 }, func() int { return 0 }))
|
|
}
|
|
|
|
func (h *handler) TestCreateFromMigrationState(ctx context.Context, req *schedulerpb.CreateFromMigrationStateRequest) (*schedulerpb.CreateFromMigrationStateResponse, error) {
|
|
return h.CreateFromMigrationState(ctx, req)
|
|
}
|
|
|
|
func (h *handler) TestMigrateToWorkflow(ctx context.Context, req *schedulerpb.MigrateToWorkflowRequest) (*schedulerpb.MigrateToWorkflowResponse, error) {
|
|
return h.MigrateToWorkflow(ctx, req)
|
|
}
|
|
|
|
func (s *Scheduler) RecordCompletedAction(
|
|
ctx chasm.MutableContext,
|
|
completed *schedulespb.CompletedResult,
|
|
requestID string,
|
|
) time.Time {
|
|
invoker := s.Invoker.Get(ctx)
|
|
return invoker.recordCompletedAction(ctx, completed, requestID)
|
|
}
|
|
|
|
func (i *Invoker) RunningWorkflowID(requestID string) string {
|
|
return i.runningWorkflowID(requestID)
|
|
}
|
|
|
|
func ContextWithTweakables(ctx chasm.Context, tweakables Tweakables) chasm.Context {
|
|
config := Config{
|
|
Tweakables: func(string) Tweakables { return tweakables },
|
|
}
|
|
return chasm.ContextWithValue(ctx, tweakablesCtxKey, config.Tweakables)
|
|
}
|
|
|
|
// RecentActionCount exposes the completed-retention limit for tests.
|
|
const RecentActionCount = recentActionCount
|
|
|
|
// ApplyCompletedRetention exposes applyCompletedRetention for tests.
|
|
func (i *Invoker) ApplyCompletedRetention() {
|
|
i.applyCompletedRetention()
|
|
}
|
|
|
|
// RecordExecuteResult exposes recordExecuteResult so tests can pin the
|
|
// per-RequestId idempotency guard against concurrent ExecuteTasks.
|
|
func (i *Invoker) RecordExecuteResult(
|
|
ctx chasm.MutableContext,
|
|
completed []*schedulespb.BufferedStart,
|
|
retryable []*schedulespb.BufferedStart,
|
|
) (newlyStarted, droppedDuplicates int, startOnlyActions []*schedulespb.BufferedStart) {
|
|
return i.recordExecuteResult(ctx, &executeResult{
|
|
CompletedStarts: completed,
|
|
RetryableStarts: retryable,
|
|
})
|
|
}
|
|
|
|
func (s *Scheduler) RecordStartOnlyActions(
|
|
ctx chasm.MutableContext,
|
|
starts []*schedulespb.BufferedStart,
|
|
) {
|
|
s.recordStartOnlyActions(ctx, starts)
|
|
}
|
|
|
|
func (b *BackfillerTaskHandler) ProcessBackfill(
|
|
scheduler *Scheduler,
|
|
backfiller *Backfiller,
|
|
limit int,
|
|
) (backfillProgressResult, error) {
|
|
return b.processBackfill(nil, scheduler, backfiller, limit)
|
|
}
|
|
|
|
func (b *BackfillerTaskHandler) AllowedBufferedStarts(
|
|
ctx chasm.Context,
|
|
scheduler *Scheduler,
|
|
invoker *Invoker,
|
|
tweakables Tweakables,
|
|
) (int, error) {
|
|
return b.allowedBufferedStarts(ctx, scheduler, invoker, tweakables)
|
|
}
|