From a88ffc680eed43a331e312d1d8a20e0fffcabda1 Mon Sep 17 00:00:00 2001 From: gomitrah Date: Thu, 3 Sep 2026 11:02:04 +0530 Subject: [PATCH 01/12] task: add Select for racing multiple tasks (WhenAny) Signed-off-by: gomitrah --- task/orchestrator.go | 75 ++++++++++++++ task/task.go | 31 ++++-- tests/select_test.go | 226 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 322 insertions(+), 10 deletions(-) create mode 100644 tests/select_test.go diff --git a/task/orchestrator.go b/task/orchestrator.go index f0c8a390..d6429f4b 100644 --- a/task/orchestrator.go +++ b/task/orchestrator.go @@ -2,6 +2,7 @@ package task import ( "container/list" + "errors" "fmt" "math" "reflect" @@ -720,6 +721,80 @@ func (ctx *WorkflowContext) WaitForSingleEvent(eventName string, timeout time.Du return task } +// Select blocks until the first of the given [tasks] completes and returns its index. Once Select +// returns, callers should call Await on the task at the returned index to obtain its result or +// error; the remaining tasks are left pending and may still be selected or awaited later (for +// example, in a loop that repeatedly selects over the tasks that have not yet completed). +// +// A task that was already completed before Select was called is treated as an immediate winner. If +// more than one of the given tasks is already completed at the time of the call, the one with the +// lowest index wins. +// +// Select requires at least one task and returns an error if no tasks are given. Every task passed to +// Select must have been obtained from this same WorkflowContext (e.g. via CallActivity, CreateTimer, +// or WaitForSingleEvent); tasks whose completion cannot be observed without calling Await -- such as +// a task returned by CallActivity/CallChildWorkflow configured with a retry policy -- are not +// supported and cause Select to return ErrTaskNotSelectable. +// +// Like Await, Select may panic with ErrTaskBlocked as the panic value when none of the tasks have +// completed and there is no further history to process. This is normal control flow for workflow +// functions, which must never recover from such panics. +func (ctx *WorkflowContext) Select(tasks ...Task) (int, error) { + if len(tasks) == 0 { + return -1, errors.New("Select requires at least one task") + } + + completable := make([]*completableTask, len(tasks)) + for i, t := range tasks { + ct, ok := underlyingCompletableTask(t) + if !ok { + return -1, fmt.Errorf("%w: task at index %d", ErrTaskNotSelectable, i) + } + completable[i] = ct + } + + winner := -1 + for i, ct := range completable { + ct.onCompleted(func() { + if winner == -1 { + winner = i + } + }) + if winner != -1 { + // A task that was already completed at registration time invokes its + // callback synchronously above; no need to look at the rest just to + // find the lowest-index winner, since ties can only happen among + // already-completed tasks and those are visited in index order. + break + } + } + + for winner == -1 { + ok, err := ctx.processNextEvent() + if err != nil { + return -1, err + } + if !ok { + break + } + } + + if winner != -1 { + return winner, nil + } + + panic(ErrTaskBlocked) +} + +// underlyingCompletableTask unwraps [t] to the concrete *completableTask backing it, if any. Retry +// wrapped tasks (taskWrapper) are deliberately not unwrapped: their real completion, including any +// retry attempts, only happens as a side effect of calling Await on them, so their delegate's +// completion state does not reflect whether the wrapped task, as a whole, is done. +func underlyingCompletableTask(t Task) (*completableTask, bool) { + ct, ok := t.(*completableTask) + return ct, ok +} + func (ctx *WorkflowContext) ContinueAsNew(newInput any, options ...ContinueAsNewOption) { ctx.continuedAsNew = true ctx.continuedAsNewInput = newInput diff --git a/task/task.go b/task/task.go index 78bdd927..a8793978 100644 --- a/task/task.go +++ b/task/task.go @@ -17,6 +17,12 @@ var ErrTaskBlocked = errors.New("the current task is blocked") // when configured timeouts expire. var ErrTaskCanceled = errors.New("the task was canceled") // CONSIDER: More specific info about the task +// ErrTaskNotSelectable is returned by [WorkflowContext.Select] when one of the given tasks doesn't +// support the completion-callback hook Select relies on to detect a winner without calling Await +// (for example, a task returned by a retried CallActivity/CallChildWorkflow, whose completion state +// is only ever discovered as a side effect of calling Await). +var ErrTaskNotSelectable = errors.New("task does not support Select") + // Task is an interface for asynchronous durable tasks. A task is conceptually similar to a future. type Task interface { Await(v any) error @@ -24,13 +30,13 @@ type Task interface { } type completableTask struct { - workflowCtx *WorkflowContext - isCompleted bool - isCanceled bool - rawResult []byte - failureDetails *protos.TaskFailureDetails - completedCallback func() - taskExecutionId string + workflowCtx *WorkflowContext + isCompleted bool + isCanceled bool + rawResult []byte + failureDetails *protos.TaskFailureDetails + completedCallbacks []func() + taskExecutionId string // kind is the resolution correlator family this task belongs to when it // is registered in pendingTasks (task, timer or child). A resolution // event only completes a pending entry of its own kind; anything else is @@ -88,6 +94,9 @@ func (t *completableTask) TaskExecutionId() string { return t.taskExecutionId } +// onCompleted registers [callback] to run when the task completes. Multiple callbacks may be +// registered on the same task (e.g. by both WaitForSingleEvent's internal timer plumbing and +// Select), and all of them run, in registration order, when the task completes. func (t *completableTask) onCompleted(callback func()) { // A task can already be completed at registration time when a buffered // early resolution was delivered as the task was scheduled; fire the @@ -96,7 +105,7 @@ func (t *completableTask) onCompleted(callback func()) { callback() return } - t.completedCallback = callback + t.completedCallbacks = append(t.completedCallbacks, callback) } func (t *completableTask) complete(rawResult []byte) { @@ -116,8 +125,10 @@ func (t *completableTask) cancel() { func (t *completableTask) completeInternal() { t.isCompleted = true - if t.completedCallback != nil { - t.completedCallback() + callbacks := t.completedCallbacks + t.completedCallbacks = nil + for _, callback := range callbacks { + callback() } } diff --git a/tests/select_test.go b/tests/select_test.go new file mode 100644 index 00000000..c6c1c16d --- /dev/null +++ b/tests/select_test.go @@ -0,0 +1,226 @@ +package tests + +import ( + "context" + "errors" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/dapr/durabletask-go/api" + "github.com/dapr/durabletask-go/api/protos" + "github.com/dapr/durabletask-go/task" +) + +// Test_Select_ExternalEventRace exercises the scenario from +// https://github.com/dapr/dapr/issues/10447: waiting on whichever of several +// named external events arrives first, without polling. +func Test_Select_ExternalEventRace(t *testing.T) { + r := task.NewTaskRegistry() + r.AddWorkflowN("SelectRaceWorkflow", func(ctx *task.WorkflowContext) (any, error) { + approve := ctx.WaitForSingleEvent("Approve", -1) + reject := ctx.WaitForSingleEvent("Reject", -1) + abort := ctx.WaitForSingleEvent("Abort", -1) + + winner, err := ctx.Select(approve, reject, abort) + if err != nil { + return nil, err + } + + switch winner { + case 0: + var v string + if err := approve.Await(&v); err != nil { + return nil, err + } + return "Approve:" + v, nil + case 1: + var v string + if err := reject.Await(&v); err != nil { + return nil, err + } + return "Reject:" + v, nil + default: + var v string + if err := abort.Await(&v); err != nil { + return nil, err + } + return "Abort:" + v, nil + } + }) + + ctx := context.Background() + client, worker := initTaskHubWorker(ctx, r) + defer worker.Shutdown(ctx) + + id, err := client.ScheduleNewWorkflow(ctx, "SelectRaceWorkflow") + require.NoError(t, err) + + _, err = client.WaitForWorkflowStart(ctx, id) + require.NoError(t, err) + + // Only raise the event that should win the race; the workflow must not + // need the other two to ever be raised. + require.NoError(t, client.RaiseEvent(ctx, id, "Reject", api.WithEventPayload("nope"))) + + timeoutCtx, cancel := context.WithTimeout(ctx, 5*time.Second) + defer cancel() + metadata, err := client.WaitForWorkflowCompletion(timeoutCtx, id) + require.NoError(t, err) + require.Equal(t, protos.OrchestrationStatus_ORCHESTRATION_STATUS_COMPLETED, metadata.RuntimeStatus) + assert.Equal(t, `"Reject:nope"`, metadata.Output.Value) +} + +// Test_Select_LoopOverRemaining models a workflow that must observe every one +// of several events, in whatever order they arrive, by repeatedly Selecting +// over the tasks that have not yet completed. +func Test_Select_LoopOverRemaining(t *testing.T) { + r := task.NewTaskRegistry() + r.AddWorkflowN("SelectLoopWorkflow", func(ctx *task.WorkflowContext) (any, error) { + pending := []task.Task{ + ctx.WaitForSingleEvent("First", -1), + ctx.WaitForSingleEvent("Second", -1), + ctx.WaitForSingleEvent("Third", -1), + } + var order []string + for len(pending) > 0 { + winner, err := ctx.Select(pending...) + if err != nil { + return nil, err + } + var v string + if err := pending[winner].Await(&v); err != nil { + return nil, err + } + order = append(order, v) + pending = append(pending[:winner], pending[winner+1:]...) + } + return order, nil + }) + + ctx := context.Background() + client, worker := initTaskHubWorker(ctx, r) + defer worker.Shutdown(ctx) + + id, err := client.ScheduleNewWorkflow(ctx, "SelectLoopWorkflow") + require.NoError(t, err) + _, err = client.WaitForWorkflowStart(ctx, id) + require.NoError(t, err) + + // Raise out of declaration order to prove Select isn't just returning + // index 0 every time. + require.NoError(t, client.RaiseEvent(ctx, id, "Third", api.WithEventPayload("c"))) + require.NoError(t, client.RaiseEvent(ctx, id, "First", api.WithEventPayload("a"))) + require.NoError(t, client.RaiseEvent(ctx, id, "Second", api.WithEventPayload("b"))) + + timeoutCtx, cancel := context.WithTimeout(ctx, 5*time.Second) + defer cancel() + metadata, err := client.WaitForWorkflowCompletion(timeoutCtx, id) + require.NoError(t, err) + require.Equal(t, protos.OrchestrationStatus_ORCHESTRATION_STATUS_COMPLETED, metadata.RuntimeStatus) + assert.Equal(t, `["c","a","b"]`, metadata.Output.Value) +} + +// Test_Select_AlreadyCompletedWins verifies that a task which is already +// complete by the time Select is called (e.g. a timer created earlier in the +// same execution that has since fired) is picked immediately, without +// needing to process any further history. +func Test_Select_AlreadyCompletedWins(t *testing.T) { + r := task.NewTaskRegistry() + r.AddWorkflowN("SelectAlreadyCompletedWorkflow", func(ctx *task.WorkflowContext) (any, error) { + immediate := ctx.CreateTimer(0) + if err := immediate.Await(nil); err != nil { + return nil, err + } + + neverFires := ctx.WaitForSingleEvent("NeverSent", -1) + + winner, err := ctx.Select(neverFires, immediate) + if err != nil { + return nil, err + } + return winner, nil + }) + + ctx := context.Background() + client, worker := initTaskHubWorker(ctx, r) + defer worker.Shutdown(ctx) + + id, err := client.ScheduleNewWorkflow(ctx, "SelectAlreadyCompletedWorkflow") + require.NoError(t, err) + + timeoutCtx, cancel := context.WithTimeout(ctx, 5*time.Second) + defer cancel() + metadata, err := client.WaitForWorkflowCompletion(timeoutCtx, id) + require.NoError(t, err) + require.Equal(t, protos.OrchestrationStatus_ORCHESTRATION_STATUS_COMPLETED, metadata.RuntimeStatus) + assert.Equal(t, `1`, metadata.Output.Value) +} + +// Test_Select_NoTasks verifies that calling Select with no tasks reports an +// error instead of panicking or blocking forever. +func Test_Select_NoTasks(t *testing.T) { + r := task.NewTaskRegistry() + r.AddWorkflowN("SelectNoTasksWorkflow", func(ctx *task.WorkflowContext) (any, error) { + _, err := ctx.Select() + if err == nil { + return nil, nil + } + return err.Error(), nil + }) + + ctx := context.Background() + client, worker := initTaskHubWorker(ctx, r) + defer worker.Shutdown(ctx) + + id, err := client.ScheduleNewWorkflow(ctx, "SelectNoTasksWorkflow") + require.NoError(t, err) + + timeoutCtx, cancel := context.WithTimeout(ctx, 5*time.Second) + defer cancel() + metadata, err := client.WaitForWorkflowCompletion(timeoutCtx, id) + require.NoError(t, err) + require.Equal(t, protos.OrchestrationStatus_ORCHESTRATION_STATUS_COMPLETED, metadata.RuntimeStatus) + assert.Equal(t, `"Select requires at least one task"`, metadata.Output.Value) +} + +// Test_Select_RetryWrappedTaskRejected verifies that a task returned by +// CallActivity with a retry policy -- whose completion, including any +// retries, is only ever driven by calling Await -- is rejected by Select +// rather than silently never winning or racing incorrectly. +func Test_Select_RetryWrappedTaskRejected(t *testing.T) { + r := task.NewTaskRegistry() + r.AddWorkflowN("SelectRetryWrappedWorkflow", func(ctx *task.WorkflowContext) (any, error) { + retried := ctx.CallActivity("FailActivity", task.WithActivityRetryPolicy(&task.RetryPolicy{ + MaxAttempts: 2, + InitialRetryInterval: 10 * time.Millisecond, + })) + plain := ctx.CreateTimer(1 * time.Hour) + + _, err := ctx.Select(retried, plain) + if err == nil { + return "no error", nil + } + return err.Error(), nil + }) + r.AddActivityN("FailActivity", func(ctx task.ActivityContext) (any, error) { + return nil, errors.New("activity should not have been invoked") + }) + + ctx := context.Background() + client, worker := initTaskHubWorker(ctx, r) + defer worker.Shutdown(ctx) + + id, err := client.ScheduleNewWorkflow(ctx, "SelectRetryWrappedWorkflow") + require.NoError(t, err) + + timeoutCtx, cancel := context.WithTimeout(ctx, 5*time.Second) + defer cancel() + metadata, err := client.WaitForWorkflowCompletion(timeoutCtx, id) + require.NoError(t, err) + require.Equal(t, protos.OrchestrationStatus_ORCHESTRATION_STATUS_COMPLETED, metadata.RuntimeStatus) + assert.Contains(t, metadata.Output.Value, "task does not support Select") +} + From 5388ae91128d73666ebc1b9ddb368f0f248bd766 Mon Sep 17 00:00:00 2001 From: gomitrah Date: Mon, 7 Sep 2026 11:39:43 +0530 Subject: [PATCH 02/12] workflow: expose Select on the public WorkflowContext Consumers using the public workflow package couldn't call Select because workflow.WorkflowContext wraps its underlying *task.WorkflowContext privately. Add a matching wrapper that converts []workflow.Task to []task.Task and delegates to it, following the same pattern as the other WorkflowContext methods (CallActivity, CreateTimer, etc). Also adds tests/workflow, a new end-to-end suite that drives the public workflow package over a real gRPC connection (mirroring tests/grpc), with a test demonstrating Select racing external events. Signed-off-by: gomitrah --- tests/workflow/select_test.go | 137 ++++++++++++++++++++++++++++++++++ workflow/workflow.go | 22 ++++++ 2 files changed, 159 insertions(+) create mode 100644 tests/workflow/select_test.go diff --git a/tests/workflow/select_test.go b/tests/workflow/select_test.go new file mode 100644 index 00000000..f38e3d46 --- /dev/null +++ b/tests/workflow/select_test.go @@ -0,0 +1,137 @@ +// Package tests_workflow exercises the public github.com/dapr/durabletask-go/workflow +// package end to end over a real gRPC connection, the same way an external consumer of +// that package would use it. +package tests_workflow + +import ( + "context" + "log" + "net" + "os" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "google.golang.org/grpc" + "google.golang.org/grpc/credentials/insecure" + + "github.com/dapr/durabletask-go/backend" + "github.com/dapr/durabletask-go/backend/sqlite" + "github.com/dapr/durabletask-go/workflow" +) + +var ( + ctx = context.Background() + workflowClient *workflow.Client +) + +// TestMain sets up a gRPC server and a workflow.Client connected to it, mirroring +// tests/grpc/grpc_test.go but driving everything through the public workflow package +// instead of the lower-level task/client packages. +func TestMain(m *testing.M) { + be := sqlite.NewSqliteBackend(sqlite.NewSqliteOptions(""), backend.DefaultLogger()) + logger := backend.DefaultLogger() + grpcServer := grpc.NewServer() + grpcExecutor, registerFn := backend.NewGrpcExecutor(be, logger) + registerFn(grpcServer) + workflowWorker := backend.NewWorkflowWorker(backend.WorkflowWorkerOptions{ + Backend: be, + Executor: grpcExecutor, + Logger: logger, + AppID: "testapp", + }) + activityWorker := backend.NewActivityTaskWorker(be, grpcExecutor, logger) + taskHubWorker := backend.NewTaskHubWorker(be, workflowWorker, activityWorker, logger) + if err := taskHubWorker.Start(ctx); err != nil { + log.Fatalf("failed to start worker: %v", err) + } + + lis, err := net.Listen("tcp", ":0") + if err != nil { + log.Fatalf("failed to listen: %v", err) + } + + go func() { + if err := grpcServer.Serve(lis); err != nil { + log.Fatalf("failed to serve: %v", err) + } + }() + + time.Sleep(1 * time.Second) + + conn, err := grpc.Dial(lis.Addr().String(), grpc.WithTransportCredentials(insecure.NewCredentials())) + if err != nil { + log.Fatalf("failed to connect to gRPC server: %v", err) + } + defer conn.Close() + workflowClient = workflow.NewClient(conn) + + exitCode := m.Run() + + timeoutCtx, cancel := context.WithTimeout(ctx, 5*time.Second) + defer cancel() + if err := grpcExecutor.Shutdown(timeoutCtx); err != nil { + log.Fatalf("failed to shutdown grpc executor: %v", err) + } + + timeoutCtx, cancel = context.WithTimeout(ctx, 5*time.Second) + defer cancel() + if err := taskHubWorker.Shutdown(timeoutCtx); err != nil { + log.Fatalf("failed to shutdown worker: %v", err) + } + grpcServer.Stop() + os.Exit(exitCode) +} + +func startWorker(t *testing.T, r *workflow.Registry) context.CancelFunc { + cancelCtx, cancel := context.WithCancel(ctx) + require.NoError(t, workflowClient.StartWorker(cancelCtx, r)) + return cancel +} + +// Test_Workflow_Select_ExternalEventRace demonstrates the scenario from +// https://github.com/dapr/dapr/issues/10447 using only the public workflow package: +// a workflow that must respond to whichever of several named external events arrives +// first, using workflow.WorkflowContext.Select instead of polling. +func Test_Workflow_Select_ExternalEventRace(t *testing.T) { + r := workflow.NewRegistry() + require.NoError(t, r.AddWorkflowN("SelectRaceWorkflow", func(ctx *workflow.WorkflowContext) (any, error) { + approve := ctx.WaitForExternalEvent("Approve", -1) + reject := ctx.WaitForExternalEvent("Reject", -1) + abort := ctx.WaitForExternalEvent("Abort", -1) + + winner, err := ctx.Select(approve, reject, abort) + if err != nil { + return nil, err + } + + tasks := []workflow.Task{approve, reject, abort} + names := []string{"Approve", "Reject", "Abort"} + + var v string + if err := tasks[winner].Await(&v); err != nil { + return nil, err + } + return names[winner] + ":" + v, nil + })) + + cancel := startWorker(t, r) + defer cancel() + + id, err := workflowClient.ScheduleWorkflow(ctx, "SelectRaceWorkflow") + require.NoError(t, err) + + _, err = workflowClient.WaitForWorkflowStart(ctx, id) + require.NoError(t, err) + + // Only raise the event that should win the race; the workflow must not need + // the other two to ever be raised. + require.NoError(t, workflowClient.RaiseEvent(ctx, id, "Reject", workflow.WithEventPayload("nope"))) + + timeoutCtx, cancelTimeout := context.WithTimeout(ctx, 30*time.Second) + defer cancelTimeout() + metadata, err := workflowClient.WaitForWorkflowCompletion(timeoutCtx, id) + require.NoError(t, err) + assert.Equal(t, `"Reject:nope"`, metadata.Output.Value) +} diff --git a/workflow/workflow.go b/workflow/workflow.go index 54fb3509..8ca3a18b 100644 --- a/workflow/workflow.go +++ b/workflow/workflow.go @@ -96,6 +96,28 @@ func (w *WorkflowContext) WaitForExternalEvent(eventName string, timeout time.Du return w.oc.WaitForSingleEvent(eventName, timeout) } +// Select blocks until the first of the given [tasks] completes and returns its index. Once Select +// returns, callers should call Await on the task at the returned index to obtain its result or +// error; the remaining tasks are left pending and may still be selected or awaited later (for +// example, in a loop that repeatedly selects over the tasks that have not yet completed). +// +// A task that was already completed before Select was called is treated as an immediate winner. If +// more than one of the given tasks is already completed at the time of the call, the one with the +// lowest index wins. +// +// Select requires at least one task and returns an error if no tasks are given. Every task passed to +// Select must have been obtained from this same WorkflowContext (e.g. via CallActivity, CreateTimer, +// or WaitForExternalEvent); tasks whose completion cannot be observed without calling Await -- such +// as a task returned by CallActivity/CallChildWorkflow configured with a retry policy -- are not +// supported and cause Select to return [task.ErrTaskNotSelectable]. +func (w *WorkflowContext) Select(tasks ...Task) (int, error) { + otasks := make([]task.Task, len(tasks)) + for i, t := range tasks { + otasks[i] = t + } + return w.oc.Select(otasks...) +} + func (w *WorkflowContext) ContinueAsNew(newInput any, options ...ContinueAsNewOption) { oopts := make([]task.ContinueAsNewOption, len(options)) for i, o := range options { From 1674d16ef3215a4b8b006d709b3fbc466767c899 Mon Sep 17 00:00:00 2001 From: gomitrah Date: Tue, 8 Sep 2026 11:00:12 +0530 Subject: [PATCH 03/12] task: fix Select callback leak on losing tasks, per review onCompleted registered a completion callback on every candidate task passed to Select but only ever cleared it when the task itself completed, so a task that lost the same Select call repeatedly (e.g. selected in a loop against tasks that mostly haven't completed yet) accumulated one dead callback per call. onCompleted now returns an unregister function, backed by a map keyed by a monotonic id rather than a plain slice so a specific registration can be removed without disturbing others regardless of registration/ unregistration order; completion still runs callbacks in registration order by sorting on that id. Select unregisters every callback it registered, winner included, in a defer so cleanup happens on every return path (including the ErrTaskBlocked panic path). Also: reword the ErrTaskNotSelectable wrap to "task at index %d: %w" per review, and add a RuntimeStatus assertion to tests/workflow/select_test.go before checking Output.Value. Signed-off-by: gomitrah --- task/orchestrator.go | 16 ++++++-- task/orchestrator_buffered_test.go | 41 ++++++++++++++++++++ task/task.go | 61 ++++++++++++++++++++++-------- tests/select_test.go | 1 - tests/workflow/select_test.go | 2 + 5 files changed, 102 insertions(+), 19 deletions(-) diff --git a/task/orchestrator.go b/task/orchestrator.go index d6429f4b..a8729e78 100644 --- a/task/orchestrator.go +++ b/task/orchestrator.go @@ -748,18 +748,19 @@ func (ctx *WorkflowContext) Select(tasks ...Task) (int, error) { for i, t := range tasks { ct, ok := underlyingCompletableTask(t) if !ok { - return -1, fmt.Errorf("%w: task at index %d", ErrTaskNotSelectable, i) + return -1, fmt.Errorf("task at index %d: %w", i, ErrTaskNotSelectable) } completable[i] = ct } winner := -1 + unregister := make([]func(), 0, len(completable)) for i, ct := range completable { - ct.onCompleted(func() { + unregister = append(unregister, ct.onCompleted(func() { if winner == -1 { winner = i } - }) + })) if winner != -1 { // A task that was already completed at registration time invokes its // callback synchronously above; no need to look at the rest just to @@ -769,6 +770,15 @@ func (ctx *WorkflowContext) Select(tasks ...Task) (int, error) { } } + // Whichever task wins (or if none do because there's no more history to process), every + // registration made above must be undone so a losing task doesn't carry a dead callback + // into a future Select or WaitForSingleEvent call on it. + defer func() { + for _, u := range unregister { + u() + } + }() + for winner == -1 { ok, err := ctx.processNextEvent() if err != nil { diff --git a/task/orchestrator_buffered_test.go b/task/orchestrator_buffered_test.go index 4aae7b3d..d00d68be 100644 --- a/task/orchestrator_buffered_test.go +++ b/task/orchestrator_buffered_test.go @@ -700,6 +700,47 @@ func Test_CompletableTask_OnCompletedAfterCompletion(t *testing.T) { assert.True(t, fired, "onCompleted on an already completed task must fire immediately") } +// Test_CompletableTask_OnCompletedUnregister verifies that calling the unregister function +// returned by onCompleted removes the callback before the task completes, and that unregistering +// is safe to call again afterwards (a no-op) and safe even after the task has since completed. +func Test_CompletableTask_OnCompletedUnregister(t *testing.T) { + task := newTask(newTestContext(t)) + + fired := false + unregister := task.onCompleted(func() { fired = true }) + require.Len(t, task.completedCallbacks, 1) + + unregister() + task.complete([]byte("x")) + assert.False(t, fired, "unregistered callback must not fire on completion") + + // Calling it again, and calling it after completion, must not panic. + unregister() +} + +// Test_Select_DoesNotLeakCallbacksOnLosingTasks is a regression test for a callback leak: Select +// used to register a completion callback on every candidate task but only ever remove it when the +// task itself completed, so a task that lost the same Select call repeatedly (e.g. selected in a +// loop against a set of tasks that mostly haven't completed yet) accumulated one dead callback per +// call. Select must unregister its callback from every losing task before returning. +func Test_Select_DoesNotLeakCallbacksOnLosingTasks(t *testing.T) { + ctx := newTestContext(t) + + done := newTask(ctx) + done.complete([]byte(`null`)) + + pending := newTask(ctx) + + const iterations = 5 + for i := 0; i < iterations; i++ { + winner, err := ctx.Select(pending, done) + require.NoError(t, err) + require.Equal(t, 1, winner) + require.Empty(t, pending.completedCallbacks, + "Select must not leave a dead callback behind on a task that didn't win") + } +} + // Benchmark_ReplaySequentialActivities measures a full replay of a workflow // with 50 sequential completed activities, the shape dominated by the // per-event and per-schedule bookkeeping this file's feature adds to. diff --git a/task/task.go b/task/task.go index a8793978..939b9b2b 100644 --- a/task/task.go +++ b/task/task.go @@ -3,6 +3,7 @@ package task import ( "errors" "fmt" + "sort" "github.com/dapr/durabletask-go/api/protos" "github.com/dapr/durabletask-go/backend/runtimestate/dedup" @@ -30,12 +31,16 @@ type Task interface { } type completableTask struct { - workflowCtx *WorkflowContext - isCompleted bool - isCanceled bool - rawResult []byte - failureDetails *protos.TaskFailureDetails - completedCallbacks []func() + workflowCtx *WorkflowContext + isCompleted bool + isCanceled bool + rawResult []byte + failureDetails *protos.TaskFailureDetails + // completedCallbacks holds the callbacks registered via onCompleted, keyed by an id from + // nextCallbackID so a specific registration can be removed (see the unregister function + // onCompleted returns) without disturbing any other registration. + completedCallbacks map[int64]func() + nextCallbackID int64 taskExecutionId string // kind is the resolution correlator family this task belongs to when it // is registered in pendingTasks (task, timer or child). A resolution @@ -94,18 +99,34 @@ func (t *completableTask) TaskExecutionId() string { return t.taskExecutionId } -// onCompleted registers [callback] to run when the task completes. Multiple callbacks may be -// registered on the same task (e.g. by both WaitForSingleEvent's internal timer plumbing and -// Select), and all of them run, in registration order, when the task completes. -func (t *completableTask) onCompleted(callback func()) { +// onCompleted registers [callback] to run when the task completes, and returns a function that +// unregisters it. Multiple callbacks may be registered on the same task (e.g. by both +// WaitForSingleEvent's internal timer plumbing and Select), and all of them run, in registration +// order, when the task completes. +// +// Callers that register a callback but then lose interest before the task completes (for example, +// Select registering on every candidate task but only caring about the one that actually won) must +// call the returned unregister function, or the callback is retained on the task indefinitely. +// Unregistering after the task has already completed, or more than once, is a no-op, and is safe to +// do regardless of what other callbacks have since been registered or unregistered on the task. +func (t *completableTask) onCompleted(callback func()) (unregister func()) { // A task can already be completed at registration time when a buffered // early resolution was delivered as the task was scheduled; fire the - // callback immediately so completion side effects are not lost. + // callback immediately so completion side effects are not lost. There is + // nothing to unregister in this case since the callback was never stored. if t.isCompleted { callback() - return + return func() {} + } + id := t.nextCallbackID + t.nextCallbackID++ + if t.completedCallbacks == nil { + t.completedCallbacks = make(map[int64]func()) + } + t.completedCallbacks[id] = callback + return func() { + delete(t.completedCallbacks, id) } - t.completedCallbacks = append(t.completedCallbacks, callback) } func (t *completableTask) complete(rawResult []byte) { @@ -127,8 +148,18 @@ func (t *completableTask) completeInternal() { t.isCompleted = true callbacks := t.completedCallbacks t.completedCallbacks = nil - for _, callback := range callbacks { - callback() + if len(callbacks) == 0 { + return + } + // Callback ids are assigned in registration order (see onCompleted), so sorting by id runs + // them in the same deterministic order regardless of map iteration order. + ids := make([]int64, 0, len(callbacks)) + for id := range callbacks { + ids = append(ids, id) + } + sort.Slice(ids, func(i, j int) bool { return ids[i] < ids[j] }) + for _, id := range ids { + callbacks[id]() } } diff --git a/tests/select_test.go b/tests/select_test.go index c6c1c16d..3c28d170 100644 --- a/tests/select_test.go +++ b/tests/select_test.go @@ -223,4 +223,3 @@ func Test_Select_RetryWrappedTaskRejected(t *testing.T) { require.Equal(t, protos.OrchestrationStatus_ORCHESTRATION_STATUS_COMPLETED, metadata.RuntimeStatus) assert.Contains(t, metadata.Output.Value, "task does not support Select") } - diff --git a/tests/workflow/select_test.go b/tests/workflow/select_test.go index f38e3d46..d61df3f9 100644 --- a/tests/workflow/select_test.go +++ b/tests/workflow/select_test.go @@ -16,6 +16,7 @@ import ( "google.golang.org/grpc" "google.golang.org/grpc/credentials/insecure" + "github.com/dapr/durabletask-go/api/protos" "github.com/dapr/durabletask-go/backend" "github.com/dapr/durabletask-go/backend/sqlite" "github.com/dapr/durabletask-go/workflow" @@ -133,5 +134,6 @@ func Test_Workflow_Select_ExternalEventRace(t *testing.T) { defer cancelTimeout() metadata, err := workflowClient.WaitForWorkflowCompletion(timeoutCtx, id) require.NoError(t, err) + require.Equal(t, protos.OrchestrationStatus_ORCHESTRATION_STATUS_COMPLETED, metadata.RuntimeStatus) assert.Equal(t, `"Reject:nope"`, metadata.Output.Value) } From 764c5824ab8a0174c200e6ff600b8161be4f9db2 Mon Sep 17 00:00:00 2001 From: gomitrah Date: Fri, 11 Sep 2026 13:48:32 +0530 Subject: [PATCH 04/12] task: make retry-configured tasks selectable, validate Select's inputs Per review: rejecting a retry-configured CallActivity/CallChildWorkflow task in Select happened too late to be safe. Retries went through internalScheduleTaskWithRetries, which scheduled the first attempt eagerly and only ran retry logic inside Await via a taskWrapper. Select rejected that wrapper with ErrTaskNotSelectable, but by then the ScheduleTask/CreateChildWorkflow action was already committed -- so the activity ran anyway, orphaned, with no task left to reach its result. internalScheduleTaskWithRetries now returns a plain *completableTask and drives retries (backoff timer, next attempt) reactively from onCompleted callbacks instead of from Await, so it's selectable like any other task. taskWrapper is now unused and removed. While wiring this up, onTaskFailed's field-write order broke replay determinism for retried tasks: it set task.taskExecutionId after fail() had already run completion callbacks, so a callback reading TaskExecutionId() (needed to thread the original, history-recorded execution ID into the next retry attempt and its backoff timer's origin) saw a stale value. Caught by an existing test, Test_ActivityRetry_TimerOriginMatchesTaskExecutionId; fixed by setting the field before calling fail(). Also, per review: Select now rejects a nil task and a task obtained from a different WorkflowContext (previously such a task passed the type assertion silently, could never complete, and blocked the workflow indefinitely with no diagnostic). Signed-off-by: gomitrah --- task/orchestrator.go | 123 ++++++++++++++++++++--------- task/orchestrator_buffered_test.go | 26 ++++++ task/task.go | 44 +++++------ tests/select_test.go | 44 +++++++---- workflow/workflow.go | 5 +- 5 files changed, 164 insertions(+), 78 deletions(-) diff --git a/task/orchestrator.go b/task/orchestrator.go index a8729e78..2b5e1d84 100644 --- a/task/orchestrator.go +++ b/task/orchestrator.go @@ -407,7 +407,7 @@ func (ctx *WorkflowContext) CallActivity(activity interface{}, opts ...CallActiv activityName := helpers.GetTaskFunctionName(activity) if options.retryPolicy != nil { - return ctx.internalScheduleTaskWithRetries(activityName+"-retry", ctx.CurrentTimeUtc, func(taskExecutionId string, _ bool) Task { + return ctx.internalScheduleTaskWithRetries(activityName+"-retry", ctx.CurrentTimeUtc, func(taskExecutionId string, _ bool) *completableTask { return ctx.internalScheduleActivity(activityName, taskExecutionId, options) }, *options.retryPolicy, 0, uuid.NewString(), func(a *protos.CreateTimerAction, execID string) { a.Origin = &protos.CreateTimerAction_ActivityRetry{ @@ -421,7 +421,7 @@ func (ctx *WorkflowContext) CallActivity(activity interface{}, opts ...CallActiv return ctx.internalScheduleActivity(activityName, uuid.NewString(), options) } -func (ctx *WorkflowContext) internalScheduleActivity(activityName, taskExecutionId string, options *callActivityOptions) Task { +func (ctx *WorkflowContext) internalScheduleActivity(activityName, taskExecutionId string, options *callActivityOptions) *completableTask { scheduleTaskAction := &protos.WorkflowAction{ Id: ctx.getNextSequenceNumber(), WorkflowActionType: &protos.WorkflowAction_ScheduleTask{ @@ -475,7 +475,7 @@ func (ctx *WorkflowContext) CallChildWorkflow(workflow interface{}, opts ...Chil if firstInstanceID == "" { firstInstanceID = helpers.GenerateChildWorkflowInstanceID(string(ctx.ID), ctx.sequenceNumber) } - return ctx.internalScheduleTaskWithRetries(workflowName+"-retry", ctx.CurrentTimeUtc, func(_ string, isRetry bool) Task { + return ctx.internalScheduleTaskWithRetries(workflowName+"-retry", ctx.CurrentTimeUtc, func(_ string, isRetry bool) *completableTask { // On retry attempts (2nd onward) carry the first attempt's instance // ID so the runtime can record it on the resulting // ChildWorkflowInstanceCreatedEvent and consumers can correlate the @@ -500,7 +500,7 @@ func (ctx *WorkflowContext) CallChildWorkflow(workflow interface{}, opts ...Chil // when non-nil, is the instance ID of the first attempt in this child's retry // chain; it is set only on retry attempts (2nd onward) and is persisted by the // runtime onto the ChildWorkflowInstanceCreatedEvent for retry-chain correlation. -func (ctx *WorkflowContext) internalCallChildWorkflow(workflowName string, options *callChildWorkflowOptions, retryParentInstanceID *string) Task { +func (ctx *WorkflowContext) internalCallChildWorkflow(workflowName string, options *callChildWorkflowOptions, retryParentInstanceID *string) *completableTask { createChildWorkflow := &protos.CreateChildWorkflowAction{ Name: workflowName, Input: options.rawInput, @@ -537,40 +537,80 @@ func (ctx *WorkflowContext) internalCallChildWorkflow(workflowName string, optio return task } -func (ctx *WorkflowContext) internalScheduleTaskWithRetries(name string, initialAttempt time.Time, schedule func(taskExecutionId string, isRetry bool) Task, policy RetryPolicy, retryCount int, taskExecutionId string, setTimerOrigin func(*protos.CreateTimerAction, string)) Task { - return &taskWrapper{ - delegate: schedule(taskExecutionId, retryCount > 0), - onAwaitResult: func(v any, taskExecutionId string, err error) error { +// internalScheduleTaskWithRetries schedules [schedule]'s first attempt immediately and returns a +// plain *completableTask representing the outcome of the whole retry chain: it completes +// successfully as soon as any attempt succeeds, or with the final attempt's failure once retries +// are exhausted. Unlike the rest of an attempt's own completion, which is driven by history events, +// the decision to retry (creating a backoff timer, then scheduling the next attempt) is driven +// reactively from each attempt's and timer's onCompleted callback rather than by calling Await, so +// the returned task's completion never depends on the caller calling Await -- in particular, so it +// can be safely passed to Select like any other task (see ErrTaskNotSelectable's history: an +// earlier version of this function returned a wrapper task whose retry logic only ran inside +// Await, which made it impossible for Select to safely observe without either driving retries +// itself or silently leaving an already-scheduled attempt orphaned). +func (ctx *WorkflowContext) internalScheduleTaskWithRetries(name string, initialAttempt time.Time, schedule func(taskExecutionId string, isRetry bool) *completableTask, policy RetryPolicy, retryCount int, taskExecutionId string, setTimerOrigin func(*protos.CreateTimerAction, string)) Task { + outer := newTask(ctx) + + // giveUp completes outer with whichever terminal state [t] (an attempt or a backoff timer) + // ended in: canceled if [t] was canceled, failed with the same details otherwise. + giveUp := func(t *completableTask) { + if t.isCanceled { + outer.cancel() + return + } + outer.fail(t.failureDetails) + } + + var attempt func(retryCount int, taskExecutionId string) + attempt = func(retryCount int, taskExecutionId string) { + delegate := schedule(taskExecutionId, retryCount > 0) + delegate.onCompleted(func() { + // Once the delegate resolves, its TaskExecutionId is the one recorded on the + // TaskScheduled/TaskCompleted/TaskFailed history events for this attempt, which is + // what must be threaded into the retry timer and the next attempt so replay stays + // deterministic; the taskExecutionId this attempt was scheduled with (a value that, + // for every attempt after the first, was itself only known once its predecessor's + // delegate resolved) is no longer authoritative once history says otherwise. + taskExecutionId := delegate.TaskExecutionId() + + err := delegate.completionError() if err == nil { - return nil + outer.complete(delegate.rawResult) + return } if retryCount+1 >= policy.MaxAttempts { // next try will exceed the max attempts, dont continue - return err + giveUp(delegate) + return } nextDelay := computeNextDelay(ctx.CurrentTimeUtc, policy, retryCount, initialAttempt, err) if nextDelay == 0 { - return err - } - task, action := ctx.createTimerInternal(&name, nextDelay) - setTimerOrigin(action, taskExecutionId) - timerErr := task.Await(nil) - if timerErr != nil { - // TODO use errors.Join when updating golang - return fmt.Errorf("%v %w", timerErr, err) - } - - t := ctx.internalScheduleTaskWithRetries(name, initialAttempt, schedule, policy, retryCount+1, taskExecutionId, setTimerOrigin) - err = t.Await(v) - if err == nil { - return nil + giveUp(delegate) + return } - return err - }, + timer, action := ctx.createTimerInternal(&name, nextDelay) + setTimerOrigin(action, taskExecutionId) + timer.onCompleted(func() { + if timer.completionError() != nil { + // Defensive: nothing in this codebase currently cancels or fails a + // pending timer task -- onTimerFired is the only resolver for one, and + // it always calls complete(nil) -- so this is unreachable today. It + // guards against silently retrying forever (attempt would otherwise run + // again below) if that ever changes, giving up with the timer's own + // terminal state instead. + giveUp(timer) + return + } + attempt(retryCount+1, taskExecutionId) + }) + }) } + attempt(retryCount, taskExecutionId) + + return outer } func computeNextDelay(currentTimeUtc time.Time, policy RetryPolicy, attempt int, firstAttempt time.Time, err error) time.Duration { @@ -730,11 +770,12 @@ func (ctx *WorkflowContext) WaitForSingleEvent(eventName string, timeout time.Du // more than one of the given tasks is already completed at the time of the call, the one with the // lowest index wins. // -// Select requires at least one task and returns an error if no tasks are given. Every task passed to -// Select must have been obtained from this same WorkflowContext (e.g. via CallActivity, CreateTimer, -// or WaitForSingleEvent); tasks whose completion cannot be observed without calling Await -- such as -// a task returned by CallActivity/CallChildWorkflow configured with a retry policy -- are not -// supported and cause Select to return ErrTaskNotSelectable. +// Select requires at least one task and returns an error if no tasks are given, if any task is nil, +// or if any task was not obtained from this same WorkflowContext (e.g. via CallActivity, CreateTimer, +// or WaitForSingleEvent, with or without a retry policy) -- a task from a different WorkflowContext +// can never complete from this context's point of view, which would otherwise block the workflow +// indefinitely with no diagnostic. A Task implementation from outside this package is also rejected, +// with ErrTaskNotSelectable. // // Like Await, Select may panic with ErrTaskBlocked as the panic value when none of the tasks have // completed and there is no further history to process. This is normal control flow for workflow @@ -746,10 +787,16 @@ func (ctx *WorkflowContext) Select(tasks ...Task) (int, error) { completable := make([]*completableTask, len(tasks)) for i, t := range tasks { + if t == nil { + return -1, fmt.Errorf("task at index %d is nil", i) + } ct, ok := underlyingCompletableTask(t) if !ok { return -1, fmt.Errorf("task at index %d: %w", i, ErrTaskNotSelectable) } + if ct.workflowCtx != ctx { + return -1, fmt.Errorf("task at index %d belongs to a different WorkflowContext", i) + } completable[i] = ct } @@ -796,10 +843,10 @@ func (ctx *WorkflowContext) Select(tasks ...Task) (int, error) { panic(ErrTaskBlocked) } -// underlyingCompletableTask unwraps [t] to the concrete *completableTask backing it, if any. Retry -// wrapped tasks (taskWrapper) are deliberately not unwrapped: their real completion, including any -// retry attempts, only happens as a side effect of calling Await on them, so their delegate's -// completion state does not reflect whether the wrapped task, as a whole, is done. +// underlyingCompletableTask reports whether [t] is backed by this package's own *completableTask, +// the only Task implementation whose completion Select can observe without calling Await. Every +// Task this package hands out (including retry-configured ones, see internalScheduleTaskWithRetries) +// is one; this only fails for a Task implementation from outside the package. func underlyingCompletableTask(t Task) (*completableTask, bool) { ct, ok := t.(*completableTask) return ct, ok @@ -995,9 +1042,13 @@ func (ctx *WorkflowContext) onTaskFailed(tf *protos.TaskFailedEvent) error { delete(ctx.pendingTasks, taskID) ctx.resolvedResolutions[key] = struct{}{} + // taskExecutionId must be set before fail() so that a completion callback registered via + // onCompleted (e.g. a retry chain reading TaskExecutionId to schedule its next attempt) sees + // it already populated: fail() runs registered callbacks synchronously as part of completing + // the task, before returning here. + task.taskExecutionId = tf.TaskExecutionId // completing a task will resume the corresponding Await() call task.fail(tf.FailureDetails) - task.taskExecutionId = tf.TaskExecutionId return nil } diff --git a/task/orchestrator_buffered_test.go b/task/orchestrator_buffered_test.go index d00d68be..53a7aab8 100644 --- a/task/orchestrator_buffered_test.go +++ b/task/orchestrator_buffered_test.go @@ -741,6 +741,32 @@ func Test_Select_DoesNotLeakCallbacksOnLosingTasks(t *testing.T) { } } +// Test_Select_RejectsNilTask is a regression test: Select(a, nil) used to report the nil task as +// ErrTaskNotSelectable (the same error as an unrelated, unsupported Task implementation), which +// misidentified the actual problem. +func Test_Select_RejectsNilTask(t *testing.T) { + ctx := newTestContext(t) + other := newTask(ctx) + + _, err := ctx.Select(other, nil) + require.EqualError(t, err, "task at index 1 is nil") +} + +// Test_Select_RejectsTaskFromDifferentContext is a regression test: a *completableTask obtained +// from a different WorkflowContext passes the underlyingCompletableTask type assertion, so without +// this check Select would register a callback that can never fire (the task belongs to a workflow +// execution ctx never processes events for), blocking the workflow indefinitely with no diagnostic. +func Test_Select_RejectsTaskFromDifferentContext(t *testing.T) { + ctx := newTestContext(t) + other := newTestContext(t) + + own := newTask(ctx) + foreign := newTask(other) + + _, err := ctx.Select(own, foreign) + require.EqualError(t, err, "task at index 1 belongs to a different WorkflowContext") +} + // Benchmark_ReplaySequentialActivities measures a full replay of a workflow // with 50 sequential completed activities, the shape dominated by the // per-event and per-schedule bookkeeping this file's feature adds to. diff --git a/task/task.go b/task/task.go index 939b9b2b..8906de82 100644 --- a/task/task.go +++ b/task/task.go @@ -18,10 +18,11 @@ var ErrTaskBlocked = errors.New("the current task is blocked") // when configured timeouts expire. var ErrTaskCanceled = errors.New("the task was canceled") // CONSIDER: More specific info about the task -// ErrTaskNotSelectable is returned by [WorkflowContext.Select] when one of the given tasks doesn't -// support the completion-callback hook Select relies on to detect a winner without calling Await -// (for example, a task returned by a retried CallActivity/CallChildWorkflow, whose completion state -// is only ever discovered as a side effect of calling Await). +// ErrTaskNotSelectable is returned by [WorkflowContext.Select] when one of the given tasks isn't +// backed by this package's own task implementation, and so doesn't support the completion-callback +// hook Select relies on to detect a winner without calling Await. Every Task returned by a +// WorkflowContext method is selectable; this only happens for a Task implementation from outside +// this package. var ErrTaskNotSelectable = errors.New("task does not support Select") // Task is an interface for asynchronous durable tasks. A task is conceptually similar to a future. @@ -68,10 +69,8 @@ func newTask(ctx *WorkflowContext) *completableTask { func (t *completableTask) Await(v any) error { for { if t.isCompleted { - if t.failureDetails != nil { - return fmt.Errorf("task failed with an error: %v", t.failureDetails.ErrorMessage) - } else if t.isCanceled { - return ErrTaskCanceled + if err := t.completionError(); err != nil { + return err } if v != nil && len(t.rawResult) > 0 { if err := unmarshalData(t.rawResult, v); err != nil { @@ -99,6 +98,19 @@ func (t *completableTask) TaskExecutionId() string { return t.taskExecutionId } +// completionError returns the error a completed task represents -- nil on success, the formatted +// failure on a failed task, or ErrTaskCanceled on a canceled one -- without touching the task's raw +// result. It must only be called once t.isCompleted is true. +func (t *completableTask) completionError() error { + if t.failureDetails != nil { + return fmt.Errorf("task failed with an error: %v", t.failureDetails.ErrorMessage) + } + if t.isCanceled { + return ErrTaskCanceled + } + return nil +} + // onCompleted registers [callback] to run when the task completes, and returns a function that // unregisters it. Multiple callbacks may be registered on the same task (e.g. by both // WaitForSingleEvent's internal timer plumbing and Select), and all of them run, in registration @@ -162,19 +174,3 @@ func (t *completableTask) completeInternal() { callbacks[id]() } } - -type taskWrapper struct { - delegate Task - onAwaitResult func(any, string, error) error -} - -var _ Task = &taskWrapper{} - -func (t *taskWrapper) Await(v any) error { - err := t.delegate.Await(v) - return t.onAwaitResult(v, t.delegate.TaskExecutionId(), err) -} - -func (t *taskWrapper) TaskExecutionId() string { - return t.delegate.TaskExecutionId() -} diff --git a/tests/select_test.go b/tests/select_test.go index 3c28d170..c859808e 100644 --- a/tests/select_test.go +++ b/tests/select_test.go @@ -3,6 +3,8 @@ package tests import ( "context" "errors" + "fmt" + "sync/atomic" "testing" "time" @@ -186,27 +188,38 @@ func Test_Select_NoTasks(t *testing.T) { assert.Equal(t, `"Select requires at least one task"`, metadata.Output.Value) } -// Test_Select_RetryWrappedTaskRejected verifies that a task returned by -// CallActivity with a retry policy -- whose completion, including any -// retries, is only ever driven by calling Await -- is rejected by Select -// rather than silently never winning or racing incorrectly. -func Test_Select_RetryWrappedTaskRejected(t *testing.T) { +// Test_Select_RetryWrappedTaskWins verifies that a task returned by CallActivity with a retry +// policy is a real, selectable task: its retries are driven independently of Select or Await, so +// it can win a Select once its retries succeed, racing normally against a plain task. +func Test_Select_RetryWrappedTaskWins(t *testing.T) { r := task.NewTaskRegistry() r.AddWorkflowN("SelectRetryWrappedWorkflow", func(ctx *task.WorkflowContext) (any, error) { - retried := ctx.CallActivity("FailActivity", task.WithActivityRetryPolicy(&task.RetryPolicy{ - MaxAttempts: 2, + retried := ctx.CallActivity("FlakyActivity", task.WithActivityRetryPolicy(&task.RetryPolicy{ + MaxAttempts: 3, InitialRetryInterval: 10 * time.Millisecond, })) - plain := ctx.CreateTimer(1 * time.Hour) + neverFires := ctx.CreateTimer(1 * time.Hour) - _, err := ctx.Select(retried, plain) - if err == nil { - return "no error", nil + winner, err := ctx.Select(retried, neverFires) + if err != nil { + return nil, err } - return err.Error(), nil + if winner != 0 { + return nil, fmt.Errorf("expected the retried activity (index 0) to win, got index %d", winner) + } + + var v string + if err := retried.Await(&v); err != nil { + return nil, err + } + return v, nil }) - r.AddActivityN("FailActivity", func(ctx task.ActivityContext) (any, error) { - return nil, errors.New("activity should not have been invoked") + var attempts int32 + r.AddActivityN("FlakyActivity", func(ctx task.ActivityContext) (any, error) { + if atomic.AddInt32(&attempts, 1) < 2 { + return nil, errors.New("not yet") + } + return "eventually succeeded", nil }) ctx := context.Background() @@ -221,5 +234,6 @@ func Test_Select_RetryWrappedTaskRejected(t *testing.T) { metadata, err := client.WaitForWorkflowCompletion(timeoutCtx, id) require.NoError(t, err) require.Equal(t, protos.OrchestrationStatus_ORCHESTRATION_STATUS_COMPLETED, metadata.RuntimeStatus) - assert.Contains(t, metadata.Output.Value, "task does not support Select") + assert.Equal(t, `"eventually succeeded"`, metadata.Output.Value) + assert.Equal(t, int32(2), atomic.LoadInt32(&attempts)) } diff --git a/workflow/workflow.go b/workflow/workflow.go index 8ca3a18b..c8d22dd6 100644 --- a/workflow/workflow.go +++ b/workflow/workflow.go @@ -107,9 +107,8 @@ func (w *WorkflowContext) WaitForExternalEvent(eventName string, timeout time.Du // // Select requires at least one task and returns an error if no tasks are given. Every task passed to // Select must have been obtained from this same WorkflowContext (e.g. via CallActivity, CreateTimer, -// or WaitForExternalEvent); tasks whose completion cannot be observed without calling Await -- such -// as a task returned by CallActivity/CallChildWorkflow configured with a retry policy -- are not -// supported and cause Select to return [task.ErrTaskNotSelectable]. +// or WaitForExternalEvent, with or without a retry policy); a Task implementation from outside this +// package causes Select to return [task.ErrTaskNotSelectable]. func (w *WorkflowContext) Select(tasks ...Task) (int, error) { otasks := make([]task.Task, len(tasks)) for i, t := range tasks { From ff7b45b68ae43568a64761a450e94f88d4b9fe64 Mon Sep 17 00:00:00 2001 From: gomitrah Date: Fri, 11 Sep 2026 13:55:50 +0530 Subject: [PATCH 05/12] tests: add fan-out retry replay test, per review discussion JoshVanL's review noted that driving retries from onCompleted moves the backoff timer's sequence number from Await time (whenever the workflow function happens to call it) to TaskFailed-processing time, and asked for a replay-compatibility guard for fan-out shapes. Add Test_ActivityRetries_FanOutAwaitedOutOfOrder: three retry- configured activities scheduled together, two of which fail once and retry, awaited in a different order than they were scheduled. This exercises exactly that shape across the multi-turn replay it produces and passes, confirming sequence numbers stay consistent regardless of await order. Signed-off-by: gomitrah --- tests/orchestrations_test.go | 60 +++++++++++++++++++++++++++++++++++- 1 file changed, 59 insertions(+), 1 deletion(-) diff --git a/tests/orchestrations_test.go b/tests/orchestrations_test.go index 6be2af67..07b8010a 100644 --- a/tests/orchestrations_test.go +++ b/tests/orchestrations_test.go @@ -360,6 +360,65 @@ func Test_ActivityRetries(t *testing.T) { ) } +// Test_ActivityRetries_FanOutAwaitedOutOfOrder fans out several retry-configured activities +// together and awaits them in a different order than they were scheduled, replaying across +// multiple turns as each one fails, backs off, and retries. Retries schedule their backoff timer +// and next attempt reactively, as each attempt's TaskFailed event is processed, rather than lazily +// at whatever later point the workflow calls Await on that particular task; this exercises that a +// fan-out of several such tasks still assigns action sequence numbers consistently across replay +// regardless of the order the workflow function later awaits them in. +func Test_ActivityRetries_FanOutAwaitedOutOfOrder(t *testing.T) { + r := task.NewTaskRegistry() + r.AddWorkflowN("ActivityRetriesFanOut", func(ctx *task.WorkflowContext) (any, error) { + retryPolicy := &task.RetryPolicy{ + MaxAttempts: 2, + InitialRetryInterval: 10 * time.Millisecond, + } + // Scheduled in order A, B, C; B never fails, A and C each fail once then succeed. + a := ctx.CallActivity("FlakyActivity", task.WithActivityInput("A"), task.WithActivityRetryPolicy(retryPolicy)) + b := ctx.CallActivity("FlakyActivity", task.WithActivityInput("B"), task.WithActivityRetryPolicy(retryPolicy)) + c := ctx.CallActivity("FlakyActivity", task.WithActivityInput("C"), task.WithActivityRetryPolicy(retryPolicy)) + + // Awaited out of declaration order: C, then A, then B. + var results []string + for _, tk := range []task.Task{c, a, b} { + var v string + if err := tk.Await(&v); err != nil { + return nil, err + } + results = append(results, v) + } + return results, nil + }) + var failedOnce sync.Map + r.AddActivityN("FlakyActivity", func(ctx task.ActivityContext) (any, error) { + var name string + if err := ctx.GetInput(&name); err != nil { + return nil, err + } + if name != "B" { + if _, alreadyFailed := failedOnce.LoadOrStore(name, true); !alreadyFailed { + return nil, fmt.Errorf("%s: not yet", name) + } + } + return name + "-done", nil + }) + + ctx := context.Background() + client, worker := initTaskHubWorker(ctx, r) + defer worker.Shutdown(ctx) + + id, err := client.ScheduleNewWorkflow(ctx, "ActivityRetriesFanOut") + require.NoError(t, err) + + timeoutCtx, cancel := context.WithTimeout(ctx, 10*time.Second) + defer cancel() + metadata, err := client.WaitForWorkflowCompletion(timeoutCtx, id) + require.NoError(t, err) + require.Equal(t, protos.OrchestrationStatus_ORCHESTRATION_STATUS_COMPLETED, metadata.RuntimeStatus) + assert.Equal(t, `["C-done","A-done","B-done"]`, metadata.Output.Value) +} + func Test_ActivityFanOut(t *testing.T) { // Registration r := task.NewTaskRegistry() @@ -2079,7 +2138,6 @@ func Test_StartedAt_AfterExecution(t *testing.T) { "StartedAt %v should be >= CreatedAt %v", startedAt, metadata.CreatedAt.AsTime()) } - func Test_StartedAt_WithScheduleTime(t *testing.T) { r := task.NewTaskRegistry() r.AddWorkflowN("StartedAtAfterExec", func(ctx *task.WorkflowContext) (any, error) { From 3a54702db9f9342f45fd2f1ee07e7be4a59c1b68 Mon Sep 17 00:00:00 2001 From: gomitrah Date: Fri, 11 Sep 2026 16:02:36 +0530 Subject: [PATCH 06/12] task,workflow,tests: address remaining Select review polish points Per review, five more of JoshVanL's points on the Select PR: - Inline underlyingCompletableTask at its one call site in Select; it was exactly a type assertion with a comment duplicating ErrTaskNotSelectable's own doc. - Extract WorkflowContext.awaitUntil(done func() bool) error, shared by completableTask.Await and WorkflowContext.Select, so the process-events-until-ready loop (and its ErrTaskBlocked panic) has one implementation instead of two that have to change together. - Re-export ErrTaskCanceled and ErrTaskNotSelectable from the workflow package (workflow/task.go) so a consumer using only that package can errors.Is against them without importing task directly; update Select's workflow-level doc to reference the re-export and add the missing ErrTaskBlocked panic warning that was already on the task-level doc. - Add the two missing Select test scenarios called out in review: a plain CallActivity racing a CreateTimer, and a WaitForSingleEvent timeout winning a race with ErrTaskCanceled surfacing through Await. Also add tests proving the two new workflow-level error re-exports actually work with errors.Is. - Fold tests/workflow/select_test.go's duplicated TestMain into tests/grpc/grpc_test.go's existing one (a second workflow.Client next to the existing grpcClient, sharing the same connection and server) and move both of its tests there, removing the second harness, its deprecated grpc.Dial, its dead defers (TestMain ends in os.Exit), and its extra ~1s startup sleep. The moved tests keep a 30s completion timeout, matching this file's own pre-existing convention (used elsewhere in grpc_test.go) rather than the 5s used in tests/select_test.go, which is a different package against a different (in-process, not gRPC) backend. The old Test_Select_RetryWrappedTaskRejected -- flagged for asserting a misleading, unverified error message -- no longer exists; it was already replaced by Test_Select_RetryWrappedTaskWins, which does assert a real invocation counter, when the underlying rejection behavior it tested was removed in an earlier commit. Points D (an alternative polling-based Select implementation) and E (the performance cost of the current callback-based one, which D would also resolve) are deliberately not addressed here. Signed-off-by: gomitrah --- task/orchestrator.go | 51 +++++++------ task/task.go | 33 +++----- tests/grpc/grpc_test.go | 129 ++++++++++++++++++++++++++++++- tests/select_test.go | 82 ++++++++++++++++++++ tests/workflow/select_test.go | 139 ---------------------------------- workflow/task.go | 8 ++ workflow/workflow.go | 16 +++- 7 files changed, 265 insertions(+), 193 deletions(-) delete mode 100644 tests/workflow/select_test.go diff --git a/task/orchestrator.go b/task/orchestrator.go index 2b5e1d84..529a359b 100644 --- a/task/orchestrator.go +++ b/task/orchestrator.go @@ -278,6 +278,26 @@ func (ctx *WorkflowContext) processNextEvent() (bool, error) { return true, nil } +// awaitUntil processes history events one at a time until [done] reports true, returning nil once +// it does. It returns any error processNextEvent reports along the way. If history runs out before +// [done] is satisfied, awaitUntil panics with ErrTaskBlocked, the same control-flow signal Await +// uses: this is normal for workflow functions, which must never recover from it. +func (ctx *WorkflowContext) awaitUntil(done func() bool) error { + for !done() { + ok, err := ctx.processNextEvent() + if err != nil { + return err + } + if !ok { + // TODO: Need a rule about using "defer" in workflows because planned panics will invoke them unexpectedly + // TODO: @joshvanl: remove panic- panic is something that should + // _never_ be called in normal operation. + panic(ErrTaskBlocked) + } + } + return nil +} + func (ctx *WorkflowContext) getNextHistoryEvent() (*protos.HistoryEvent, bool) { var historyList []*protos.HistoryEvent index := ctx.historyIndex @@ -790,7 +810,10 @@ func (ctx *WorkflowContext) Select(tasks ...Task) (int, error) { if t == nil { return -1, fmt.Errorf("task at index %d is nil", i) } - ct, ok := underlyingCompletableTask(t) + // Every Task this package hands out, including retry-configured ones (see + // internalScheduleTaskWithRetries), is backed by *completableTask; this only fails for a + // Task implementation from outside the package. + ct, ok := t.(*completableTask) if !ok { return -1, fmt.Errorf("task at index %d: %w", i, ErrTaskNotSelectable) } @@ -826,30 +849,10 @@ func (ctx *WorkflowContext) Select(tasks ...Task) (int, error) { } }() - for winner == -1 { - ok, err := ctx.processNextEvent() - if err != nil { - return -1, err - } - if !ok { - break - } + if err := ctx.awaitUntil(func() bool { return winner != -1 }); err != nil { + return -1, err } - - if winner != -1 { - return winner, nil - } - - panic(ErrTaskBlocked) -} - -// underlyingCompletableTask reports whether [t] is backed by this package's own *completableTask, -// the only Task implementation whose completion Select can observe without calling Await. Every -// Task this package hands out (including retry-configured ones, see internalScheduleTaskWithRetries) -// is one; this only fails for a Task implementation from outside the package. -func underlyingCompletableTask(t Task) (*completableTask, bool) { - ct, ok := t.(*completableTask) - return ct, ok + return winner, nil } func (ctx *WorkflowContext) ContinueAsNew(newInput any, options ...ContinueAsNewOption) { diff --git a/task/task.go b/task/task.go index 8906de82..db933e5a 100644 --- a/task/task.go +++ b/task/task.go @@ -67,31 +67,18 @@ func newTask(ctx *WorkflowContext) *completableTask { // of any kind. However, workflow functions must never attempt to recover from such panics to ensure that // the workflow execution can proceed normally. func (t *completableTask) Await(v any) error { - for { - if t.isCompleted { - if err := t.completionError(); err != nil { - return err - } - if v != nil && len(t.rawResult) > 0 { - if err := unmarshalData(t.rawResult, v); err != nil { - return fmt.Errorf("failed to decode task result: %w", err) - } - } - return nil - } - - ok, err := t.workflowCtx.processNextEvent() - if err != nil { - return err - } - if !ok { - break + if err := t.workflowCtx.awaitUntil(func() bool { return t.isCompleted }); err != nil { + return err + } + if err := t.completionError(); err != nil { + return err + } + if v != nil && len(t.rawResult) > 0 { + if err := unmarshalData(t.rawResult, v); err != nil { + return fmt.Errorf("failed to decode task result: %w", err) } } - // TODO: Need a rule about using "defer" in workflows because planned panics will invoke them unexpectedly - // TODO: @joshvanl: remove panic- panic is something that should - // _never_ be called in normal operation. - panic(ErrTaskBlocked) + return nil } func (t *completableTask) TaskExecutionId() string { diff --git a/tests/grpc/grpc_test.go b/tests/grpc/grpc_test.go index 9e3e23bd..f2dda607 100644 --- a/tests/grpc/grpc_test.go +++ b/tests/grpc/grpc_test.go @@ -26,13 +26,15 @@ import ( "github.com/dapr/durabletask-go/client" "github.com/dapr/durabletask-go/task" "github.com/dapr/durabletask-go/tests/utils" + "github.com/dapr/durabletask-go/workflow" "go.opentelemetry.io/otel" ) var ( - grpcClient *client.TaskHubGrpcClient - ctx = context.Background() - tracer = otel.Tracer("grpc-test") + grpcClient *client.TaskHubGrpcClient + workflowClient *workflow.Client + ctx = context.Background() + tracer = otel.Tracer("grpc-test") ) // TestMain is the entry point for the test suite. We use this to set up a gRPC server and client instance @@ -77,6 +79,7 @@ func TestMain(m *testing.M) { } defer conn.Close() grpcClient = client.NewTaskHubGrpcClient(conn, logger) + workflowClient = workflow.NewClientWithLogger(conn, logger) // Run the test exitCode exitCode := m.Run() @@ -103,6 +106,12 @@ func startGrpcListener(t *testing.T, r *task.TaskRegistry) context.CancelFunc { return cancel } +func startWorkflowListener(t *testing.T, r *workflow.Registry) context.CancelFunc { + cancelCtx, cancel := context.WithCancel(ctx) + require.NoError(t, workflowClient.StartWorker(cancelCtx, r)) + return cancel +} + func Test_Grpc_WaitForInstanceStart_Timeout(t *testing.T) { r := task.NewTaskRegistry() r.AddWorkflowN("WaitForInstanceStartThrowsException", func(ctx *task.WorkflowContext) (any, error) { @@ -662,3 +671,117 @@ func Test_Grpc_PatchedWorkflow(t *testing.T) { assert.Equal(t, []bool{false}, patches1Found) assert.Equal(t, []bool{true}, patches2Found) } + +// Test_Workflow_Select_ExternalEventRace demonstrates the scenario from +// https://github.com/dapr/dapr/issues/10447 using only the public workflow package: a workflow +// that must respond to whichever of several named external events arrives first, using +// workflow.WorkflowContext.Select instead of polling. +func Test_Workflow_Select_ExternalEventRace(t *testing.T) { + r := workflow.NewRegistry() + require.NoError(t, r.AddWorkflowN("SelectRaceWorkflow", func(ctx *workflow.WorkflowContext) (any, error) { + approve := ctx.WaitForExternalEvent("Approve", -1) + reject := ctx.WaitForExternalEvent("Reject", -1) + abort := ctx.WaitForExternalEvent("Abort", -1) + + winner, err := ctx.Select(approve, reject, abort) + if err != nil { + return nil, err + } + + tasks := []workflow.Task{approve, reject, abort} + names := []string{"Approve", "Reject", "Abort"} + + var v string + if err := tasks[winner].Await(&v); err != nil { + return nil, err + } + return names[winner] + ":" + v, nil + })) + + cancel := startWorkflowListener(t, r) + defer cancel() + + id, err := workflowClient.ScheduleWorkflow(ctx, "SelectRaceWorkflow") + require.NoError(t, err) + + _, err = workflowClient.WaitForWorkflowStart(ctx, id) + require.NoError(t, err) + + // Only raise the event that should win the race; the workflow must not need the other two to + // ever be raised. + require.NoError(t, workflowClient.RaiseEvent(ctx, id, "Reject", workflow.WithEventPayload("nope"))) + + timeoutCtx, cancelTimeout := context.WithTimeout(ctx, 30*time.Second) + defer cancelTimeout() + metadata, err := workflowClient.WaitForWorkflowCompletion(timeoutCtx, id) + require.NoError(t, err) + require.Equal(t, protos.OrchestrationStatus_ORCHESTRATION_STATUS_COMPLETED, metadata.RuntimeStatus) + assert.Equal(t, `"Reject:nope"`, metadata.Output.Value) +} + +// Test_Workflow_ErrTaskCanceledIsCheckable verifies that a consumer using only the public workflow +// package can check a timed-out WaitForExternalEvent's error against workflow.ErrTaskCanceled with +// errors.Is, since task.ErrTaskCanceled itself isn't reachable without importing the lower-level +// task package. +func Test_Workflow_ErrTaskCanceledIsCheckable(t *testing.T) { + r := workflow.NewRegistry() + require.NoError(t, r.AddWorkflowN("EventTimeoutWorkflow", func(ctx *workflow.WorkflowContext) (any, error) { + err := ctx.WaitForExternalEvent("NeverRaised", 50*time.Millisecond).Await(nil) + if !errors.Is(err, workflow.ErrTaskCanceled) { + return "wrong error: " + fmt.Sprint(err), nil + } + return "ok", nil + })) + + cancel := startWorkflowListener(t, r) + defer cancel() + + id, err := workflowClient.ScheduleWorkflow(ctx, "EventTimeoutWorkflow") + require.NoError(t, err) + + timeoutCtx, cancelTimeout := context.WithTimeout(ctx, 30*time.Second) + defer cancelTimeout() + metadata, err := workflowClient.WaitForWorkflowCompletion(timeoutCtx, id) + require.NoError(t, err) + require.Equal(t, protos.OrchestrationStatus_ORCHESTRATION_STATUS_COMPLETED, metadata.RuntimeStatus) + assert.Equal(t, `"ok"`, metadata.Output.Value) +} + +// foreignWorkflowTask is a workflow.Task implementation from outside the task/workflow packages, +// used to exercise the ErrTaskNotSelectable path: only *completableTask (unexported, from package +// task) is selectable. +type foreignWorkflowTask struct{} + +func (foreignWorkflowTask) Await(v any) error { return nil } +func (foreignWorkflowTask) TaskExecutionId() string { return "" } + +// Test_Workflow_Select_ErrTaskNotSelectableIsCheckable verifies that a consumer using only the +// public workflow package (not importing task directly) can check a Select error against +// workflow.ErrTaskNotSelectable with errors.Is, since task.ErrTaskNotSelectable itself isn't +// reachable without importing the lower-level task package. +func Test_Workflow_Select_ErrTaskNotSelectableIsCheckable(t *testing.T) { + r := workflow.NewRegistry() + require.NoError(t, r.AddWorkflowN("SelectNotSelectableWorkflow", func(ctx *workflow.WorkflowContext) (any, error) { + _, err := ctx.Select(foreignWorkflowTask{}) + if err == nil { + return "no error", nil + } + if !errors.Is(err, workflow.ErrTaskNotSelectable) { + return "wrong error: " + err.Error(), nil + } + return "ok", nil + })) + + cancel := startWorkflowListener(t, r) + defer cancel() + + id, err := workflowClient.ScheduleWorkflow(ctx, "SelectNotSelectableWorkflow") + require.NoError(t, err) + + timeoutCtx, cancelTimeout := context.WithTimeout(ctx, 30*time.Second) + defer cancelTimeout() + metadata, err := workflowClient.WaitForWorkflowCompletion(timeoutCtx, id) + require.NoError(t, err) + require.Equal(t, protos.OrchestrationStatus_ORCHESTRATION_STATUS_COMPLETED, metadata.RuntimeStatus) + assert.Equal(t, `"ok"`, metadata.Output.Value) +} diff --git a/tests/select_test.go b/tests/select_test.go index c859808e..6015a024 100644 --- a/tests/select_test.go +++ b/tests/select_test.go @@ -237,3 +237,85 @@ func Test_Select_RetryWrappedTaskWins(t *testing.T) { assert.Equal(t, `"eventually succeeded"`, metadata.Output.Value) assert.Equal(t, int32(2), atomic.LoadInt32(&attempts)) } + +// Test_Select_ActivityRacesTimer covers the most common WhenAny shape: a plain (non-retry) +// CallActivity racing a CreateTimer. The activity's 1s sleep is deliberately much longer than the +// timer's 50ms delay so the timer wins with a wide margin, comfortably covering the extra latency +// the sqlite backend's polling (with its own exponential backoff) adds on top of the timer's raw +// delay before the workflow actually picks the TimerFired event up. +func Test_Select_ActivityRacesTimer(t *testing.T) { + r := task.NewTaskRegistry() + r.AddWorkflowN("SelectActivityRacesTimerWorkflow", func(ctx *task.WorkflowContext) (any, error) { + slowActivity := ctx.CallActivity("SlowActivity") + timer := ctx.CreateTimer(50 * time.Millisecond) + + winner, err := ctx.Select(slowActivity, timer) + if err != nil { + return nil, err + } + if winner != 1 { + return nil, fmt.Errorf("expected the timer (index 1) to win, got index %d", winner) + } + if err := timer.Await(nil); err != nil { + return nil, err + } + return "timer won", nil + }) + r.AddActivityN("SlowActivity", func(ctx task.ActivityContext) (any, error) { + time.Sleep(1 * time.Second) + return "too slow", nil + }) + + ctx := context.Background() + client, worker := initTaskHubWorker(ctx, r) + defer worker.Shutdown(ctx) + + id, err := client.ScheduleNewWorkflow(ctx, "SelectActivityRacesTimerWorkflow") + require.NoError(t, err) + + timeoutCtx, cancel := context.WithTimeout(ctx, 5*time.Second) + defer cancel() + metadata, err := client.WaitForWorkflowCompletion(timeoutCtx, id) + require.NoError(t, err) + require.Equal(t, protos.OrchestrationStatus_ORCHESTRATION_STATUS_COMPLETED, metadata.RuntimeStatus) + assert.Equal(t, `"timer won"`, metadata.Output.Value) +} + +// Test_Select_WaitForSingleEventTimeoutWins verifies that when a WaitForSingleEvent task times out +// before its event is ever raised, Select returns that task as the winner and Await on it surfaces +// ErrTaskCanceled, exactly as it would outside of Select. +func Test_Select_WaitForSingleEventTimeoutWins(t *testing.T) { + r := task.NewTaskRegistry() + r.AddWorkflowN("SelectEventTimeoutWorkflow", func(ctx *task.WorkflowContext) (any, error) { + neverRaised := ctx.WaitForSingleEvent("NeverRaised", 50*time.Millisecond) + neverFires := ctx.CreateTimer(1 * time.Hour) + + winner, err := ctx.Select(neverRaised, neverFires) + if err != nil { + return nil, err + } + if winner != 0 { + return nil, fmt.Errorf("expected the timed-out event wait (index 0) to win, got index %d", winner) + } + + err = neverRaised.Await(nil) + if !errors.Is(err, task.ErrTaskCanceled) { + return nil, fmt.Errorf("expected ErrTaskCanceled, got %v", err) + } + return "timed out as expected", nil + }) + + ctx := context.Background() + client, worker := initTaskHubWorker(ctx, r) + defer worker.Shutdown(ctx) + + id, err := client.ScheduleNewWorkflow(ctx, "SelectEventTimeoutWorkflow") + require.NoError(t, err) + + timeoutCtx, cancel := context.WithTimeout(ctx, 5*time.Second) + defer cancel() + metadata, err := client.WaitForWorkflowCompletion(timeoutCtx, id) + require.NoError(t, err) + require.Equal(t, protos.OrchestrationStatus_ORCHESTRATION_STATUS_COMPLETED, metadata.RuntimeStatus) + assert.Equal(t, `"timed out as expected"`, metadata.Output.Value) +} diff --git a/tests/workflow/select_test.go b/tests/workflow/select_test.go deleted file mode 100644 index d61df3f9..00000000 --- a/tests/workflow/select_test.go +++ /dev/null @@ -1,139 +0,0 @@ -// Package tests_workflow exercises the public github.com/dapr/durabletask-go/workflow -// package end to end over a real gRPC connection, the same way an external consumer of -// that package would use it. -package tests_workflow - -import ( - "context" - "log" - "net" - "os" - "testing" - "time" - - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" - "google.golang.org/grpc" - "google.golang.org/grpc/credentials/insecure" - - "github.com/dapr/durabletask-go/api/protos" - "github.com/dapr/durabletask-go/backend" - "github.com/dapr/durabletask-go/backend/sqlite" - "github.com/dapr/durabletask-go/workflow" -) - -var ( - ctx = context.Background() - workflowClient *workflow.Client -) - -// TestMain sets up a gRPC server and a workflow.Client connected to it, mirroring -// tests/grpc/grpc_test.go but driving everything through the public workflow package -// instead of the lower-level task/client packages. -func TestMain(m *testing.M) { - be := sqlite.NewSqliteBackend(sqlite.NewSqliteOptions(""), backend.DefaultLogger()) - logger := backend.DefaultLogger() - grpcServer := grpc.NewServer() - grpcExecutor, registerFn := backend.NewGrpcExecutor(be, logger) - registerFn(grpcServer) - workflowWorker := backend.NewWorkflowWorker(backend.WorkflowWorkerOptions{ - Backend: be, - Executor: grpcExecutor, - Logger: logger, - AppID: "testapp", - }) - activityWorker := backend.NewActivityTaskWorker(be, grpcExecutor, logger) - taskHubWorker := backend.NewTaskHubWorker(be, workflowWorker, activityWorker, logger) - if err := taskHubWorker.Start(ctx); err != nil { - log.Fatalf("failed to start worker: %v", err) - } - - lis, err := net.Listen("tcp", ":0") - if err != nil { - log.Fatalf("failed to listen: %v", err) - } - - go func() { - if err := grpcServer.Serve(lis); err != nil { - log.Fatalf("failed to serve: %v", err) - } - }() - - time.Sleep(1 * time.Second) - - conn, err := grpc.Dial(lis.Addr().String(), grpc.WithTransportCredentials(insecure.NewCredentials())) - if err != nil { - log.Fatalf("failed to connect to gRPC server: %v", err) - } - defer conn.Close() - workflowClient = workflow.NewClient(conn) - - exitCode := m.Run() - - timeoutCtx, cancel := context.WithTimeout(ctx, 5*time.Second) - defer cancel() - if err := grpcExecutor.Shutdown(timeoutCtx); err != nil { - log.Fatalf("failed to shutdown grpc executor: %v", err) - } - - timeoutCtx, cancel = context.WithTimeout(ctx, 5*time.Second) - defer cancel() - if err := taskHubWorker.Shutdown(timeoutCtx); err != nil { - log.Fatalf("failed to shutdown worker: %v", err) - } - grpcServer.Stop() - os.Exit(exitCode) -} - -func startWorker(t *testing.T, r *workflow.Registry) context.CancelFunc { - cancelCtx, cancel := context.WithCancel(ctx) - require.NoError(t, workflowClient.StartWorker(cancelCtx, r)) - return cancel -} - -// Test_Workflow_Select_ExternalEventRace demonstrates the scenario from -// https://github.com/dapr/dapr/issues/10447 using only the public workflow package: -// a workflow that must respond to whichever of several named external events arrives -// first, using workflow.WorkflowContext.Select instead of polling. -func Test_Workflow_Select_ExternalEventRace(t *testing.T) { - r := workflow.NewRegistry() - require.NoError(t, r.AddWorkflowN("SelectRaceWorkflow", func(ctx *workflow.WorkflowContext) (any, error) { - approve := ctx.WaitForExternalEvent("Approve", -1) - reject := ctx.WaitForExternalEvent("Reject", -1) - abort := ctx.WaitForExternalEvent("Abort", -1) - - winner, err := ctx.Select(approve, reject, abort) - if err != nil { - return nil, err - } - - tasks := []workflow.Task{approve, reject, abort} - names := []string{"Approve", "Reject", "Abort"} - - var v string - if err := tasks[winner].Await(&v); err != nil { - return nil, err - } - return names[winner] + ":" + v, nil - })) - - cancel := startWorker(t, r) - defer cancel() - - id, err := workflowClient.ScheduleWorkflow(ctx, "SelectRaceWorkflow") - require.NoError(t, err) - - _, err = workflowClient.WaitForWorkflowStart(ctx, id) - require.NoError(t, err) - - // Only raise the event that should win the race; the workflow must not need - // the other two to ever be raised. - require.NoError(t, workflowClient.RaiseEvent(ctx, id, "Reject", workflow.WithEventPayload("nope"))) - - timeoutCtx, cancelTimeout := context.WithTimeout(ctx, 30*time.Second) - defer cancelTimeout() - metadata, err := workflowClient.WaitForWorkflowCompletion(timeoutCtx, id) - require.NoError(t, err) - require.Equal(t, protos.OrchestrationStatus_ORCHESTRATION_STATUS_COMPLETED, metadata.RuntimeStatus) - assert.Equal(t, `"Reject:nope"`, metadata.Output.Value) -} diff --git a/workflow/task.go b/workflow/task.go index ec7c0f27..5e045720 100644 --- a/workflow/task.go +++ b/workflow/task.go @@ -5,3 +5,11 @@ import "github.com/dapr/durabletask-go/task" // Task is an interface for asynchronous durable tasks. A task is conceptually // similar to a future. type Task task.Task + +// ErrTaskCanceled is returned by [Task.Await] when the task was canceled, for example because a +// timeout configured via [WorkflowContext.WaitForExternalEvent] expired. +var ErrTaskCanceled = task.ErrTaskCanceled + +// ErrTaskNotSelectable is returned by [WorkflowContext.Select] when one of the given tasks isn't +// selectable -- see Select's doc comment for the specific cases. +var ErrTaskNotSelectable = task.ErrTaskNotSelectable diff --git a/workflow/workflow.go b/workflow/workflow.go index c8d22dd6..f6e4c4ed 100644 --- a/workflow/workflow.go +++ b/workflow/workflow.go @@ -105,10 +105,18 @@ func (w *WorkflowContext) WaitForExternalEvent(eventName string, timeout time.Du // more than one of the given tasks is already completed at the time of the call, the one with the // lowest index wins. // -// Select requires at least one task and returns an error if no tasks are given. Every task passed to -// Select must have been obtained from this same WorkflowContext (e.g. via CallActivity, CreateTimer, -// or WaitForExternalEvent, with or without a retry policy); a Task implementation from outside this -// package causes Select to return [task.ErrTaskNotSelectable]. +// Select requires at least one task and returns an error if no tasks are given, if any task is nil, +// or if any task was not obtained from this same WorkflowContext (e.g. via CallActivity, CreateTimer, +// or WaitForExternalEvent, with or without a retry policy) -- a task from a different WorkflowContext +// can never complete from this context's point of view, which would otherwise block the workflow +// indefinitely with no diagnostic. A Task implementation from outside this package is also rejected, +// with [ErrTaskNotSelectable]. +// +// Like Await, Select may panic with [task.ErrTaskBlocked] as the panic value when none of the tasks +// have completed and there is no further history to process. This is normal control flow for +// workflow functions, which must never recover from such panics: doing so prevents the workflow +// runtime's own recovery from observing the signal, and the workflow will never emit its pending +// actions. func (w *WorkflowContext) Select(tasks ...Task) (int, error) { otasks := make([]task.Task, len(tasks)) for i, t := range tasks { From a7102d030f159fdc6de9105dac72427575c1455d Mon Sep 17 00:00:00 2001 From: gomitrah Date: Wed, 23 Sep 2026 12:33:36 +0530 Subject: [PATCH 07/12] task: replace Select's callback machinery with direct polling Per review (points D and E): Select's callback/map/unregister machinery, added earlier to fix a callback-leak bug, was unnecessary overhead -- tasks only ever complete inside single-threaded processNextEvent, so Select can just poll isCompleted directly instead of registering anything. Select now uses awaitUntil (already extracted in an earlier commit) with a done closure that scans candidates in index order and returns the first completed one, replacing the whole callback-registration/ unregister-defer block. completableTask.onCompleted/completeInternal revert to a single completedCallback field, since Select no longer uses onCompleted at all and its two remaining callers -- WaitForSingleEvent's internal timer, and the retry driver added earlier on this branch -- each register at most one callback on a single-use task object, never needing more than one registration or an unregister. Tie-break rule: when several Select candidates complete within one processNextEvent call (only reachable via onExecutionResumed replaying a batch of events buffered during a suspension), the lowest-index candidate now wins, matching the already-documented rule for tasks completed before Select was even called, rather than having two different rules depending on timing. Added Test_Select_TieBreak_LowestIndexWinsAcrossSuspendedBatch to lock this in -- verified by hand that a naive reverse-order scan makes this test fail, confirming it actually discriminates the rule rather than passing regardless. Removed Test_CompletableTask_OnCompletedUnregister and Test_Select_DoesNotLeakCallbacksOnLosingTasks (both tested the now-removed unregister mechanism / a leak that can no longer happen). Added Test_Select_DoesNotRegisterOnLosingTasks, asserting Select never touches a losing candidate's callback field at all, and Benchmark_SelectFanOut, a 50-task Select-loop benchmark matching the shape used in review: measures 488 allocs/op, matching the reviewer's own cited number for this approach. completableTask is back to 80 bytes (was 88 with the map fields), matching the pre-regression baseline. Signed-off-by: gomitrah --- task/orchestrator.go | 45 ++++----- task/orchestrator_buffered_test.go | 150 +++++++++++++++++++++-------- task/task.go | 66 ++++--------- workflow/workflow.go | 6 +- 4 files changed, 147 insertions(+), 120 deletions(-) diff --git a/task/orchestrator.go b/task/orchestrator.go index 529a359b..b3829ee8 100644 --- a/task/orchestrator.go +++ b/task/orchestrator.go @@ -786,9 +786,11 @@ func (ctx *WorkflowContext) WaitForSingleEvent(eventName string, timeout time.Du // error; the remaining tasks are left pending and may still be selected or awaited later (for // example, in a loop that repeatedly selects over the tasks that have not yet completed). // -// A task that was already completed before Select was called is treated as an immediate winner. If -// more than one of the given tasks is already completed at the time of the call, the one with the -// lowest index wins. +// Select polls the given tasks' completion state after each history event is processed, so if more +// than one of them is found completed at the same time -- whether because they were already +// completed before Select was called, or because a single history event completed several of them +// at once (possible when a batch of events buffered during a suspended execution is replayed) -- +// the one with the lowest index wins; this is the only tie-break rule Select ever applies. // // Select requires at least one task and returns an error if no tasks are given, if any task is nil, // or if any task was not obtained from this same WorkflowContext (e.g. via CallActivity, CreateTimer, @@ -823,33 +825,24 @@ func (ctx *WorkflowContext) Select(tasks ...Task) (int, error) { completable[i] = ct } + // Tasks complete only inside the single-threaded processing of a history event (see + // processNextEvent), so no task can transition to completed between one poll of completable and + // the next: polling here needs no completion callbacks, unlike WaitForSingleEvent's internal + // timer plumbing, which reacts to a specific other task's completion from outside this call + // entirely. A single history event can still complete more than one candidate at once -- e.g. + // onExecutionResumed replays a whole batch of events buffered during a suspension within one + // processNextEvent call -- so this always scans in index order and takes the first completed + // task it finds, rather than assuming at most one newly-completed candidate per poll. winner := -1 - unregister := make([]func(), 0, len(completable)) - for i, ct := range completable { - unregister = append(unregister, ct.onCompleted(func() { - if winner == -1 { + if err := ctx.awaitUntil(func() bool { + for i, ct := range completable { + if ct.isCompleted { winner = i + return true } - })) - if winner != -1 { - // A task that was already completed at registration time invokes its - // callback synchronously above; no need to look at the rest just to - // find the lowest-index winner, since ties can only happen among - // already-completed tasks and those are visited in index order. - break - } - } - - // Whichever task wins (or if none do because there's no more history to process), every - // registration made above must be undone so a losing task doesn't carry a dead callback - // into a future Select or WaitForSingleEvent call on it. - defer func() { - for _, u := range unregister { - u() } - }() - - if err := ctx.awaitUntil(func() bool { return winner != -1 }); err != nil { + return false + }); err != nil { return -1, err } return winner, nil diff --git a/task/orchestrator_buffered_test.go b/task/orchestrator_buffered_test.go index 53a7aab8..8669f8c5 100644 --- a/task/orchestrator_buffered_test.go +++ b/task/orchestrator_buffered_test.go @@ -700,30 +700,38 @@ func Test_CompletableTask_OnCompletedAfterCompletion(t *testing.T) { assert.True(t, fired, "onCompleted on an already completed task must fire immediately") } -// Test_CompletableTask_OnCompletedUnregister verifies that calling the unregister function -// returned by onCompleted removes the callback before the task completes, and that unregistering -// is safe to call again afterwards (a no-op) and safe even after the task has since completed. -func Test_CompletableTask_OnCompletedUnregister(t *testing.T) { - task := newTask(newTestContext(t)) +// Test_Select_RejectsNilTask is a regression test: Select(a, nil) used to report the nil task as +// ErrTaskNotSelectable (the same error as an unrelated, unsupported Task implementation), which +// misidentified the actual problem. +func Test_Select_RejectsNilTask(t *testing.T) { + ctx := newTestContext(t) + other := newTask(ctx) - fired := false - unregister := task.onCompleted(func() { fired = true }) - require.Len(t, task.completedCallbacks, 1) + _, err := ctx.Select(other, nil) + require.EqualError(t, err, "task at index 1 is nil") +} - unregister() - task.complete([]byte("x")) - assert.False(t, fired, "unregistered callback must not fire on completion") +// Test_Select_RejectsTaskFromDifferentContext is a regression test: a *completableTask obtained +// from a different WorkflowContext passes the type assertion in Select's validation loop, so +// without this check Select would poll a task that can never complete from this context's point of +// view (its workflow execution belongs to a different ctx, whose history this ctx never processes), +// blocking the workflow indefinitely with no diagnostic. +func Test_Select_RejectsTaskFromDifferentContext(t *testing.T) { + ctx := newTestContext(t) + other := newTestContext(t) - // Calling it again, and calling it after completion, must not panic. - unregister() + own := newTask(ctx) + foreign := newTask(other) + + _, err := ctx.Select(own, foreign) + require.EqualError(t, err, "task at index 1 belongs to a different WorkflowContext") } -// Test_Select_DoesNotLeakCallbacksOnLosingTasks is a regression test for a callback leak: Select -// used to register a completion callback on every candidate task but only ever remove it when the -// task itself completed, so a task that lost the same Select call repeatedly (e.g. selected in a -// loop against a set of tasks that mostly haven't completed yet) accumulated one dead callback per -// call. Select must unregister its callback from every losing task before returning. -func Test_Select_DoesNotLeakCallbacksOnLosingTasks(t *testing.T) { +// Test_Select_DoesNotRegisterOnLosingTasks confirms Select's polling implementation never touches +// a losing candidate's completedCallback: repeatedly Selecting a still-pending task against an +// already-completed one must leave the pending task's callback field untouched (nil), since Select +// only reads isCompleted -- it never calls onCompleted. +func Test_Select_DoesNotRegisterOnLosingTasks(t *testing.T) { ctx := newTestContext(t) done := newTask(ctx) @@ -736,35 +744,45 @@ func Test_Select_DoesNotLeakCallbacksOnLosingTasks(t *testing.T) { winner, err := ctx.Select(pending, done) require.NoError(t, err) require.Equal(t, 1, winner) - require.Empty(t, pending.completedCallbacks, - "Select must not leave a dead callback behind on a task that didn't win") + require.Nil(t, pending.completedCallback, + "Select must never register a callback on a task it only polls") } } -// Test_Select_RejectsNilTask is a regression test: Select(a, nil) used to report the nil task as -// ErrTaskNotSelectable (the same error as an unrelated, unsupported Task implementation), which -// misidentified the actual problem. -func Test_Select_RejectsNilTask(t *testing.T) { - ctx := newTestContext(t) - other := newTask(ctx) - - _, err := ctx.Select(other, nil) - require.EqualError(t, err, "task at index 1 is nil") -} +// Test_Select_TieBreak_LowestIndexWinsAcrossSuspendedBatch is a regression test for Select's +// tie-break rule in the one scenario where it's actually observable: several candidates completing +// within a single processNextEvent call. onExecutionResumed replays a whole batch of events +// buffered during a suspension in one such call, so if the event for the higher-index candidate +// (EventB, index 1) appears in that batch before the event for the lower-index one (EventA, index +// 0), Select must still return index 0 -- history order must not win over argument order. +func Test_Select_TieBreak_LowestIndexWinsAcrossSuspendedBatch(t *testing.T) { + r := NewTaskRegistry() + require.NoError(t, r.AddWorkflowN("wf", func(ctx *WorkflowContext) (any, error) { + taskA := ctx.WaitForSingleEvent("EventA", -1) + taskB := ctx.WaitForSingleEvent("EventB", -1) -// Test_Select_RejectsTaskFromDifferentContext is a regression test: a *completableTask obtained -// from a different WorkflowContext passes the underlyingCompletableTask type assertion, so without -// this check Select would register a callback that can never fire (the task belongs to a workflow -// execution ctx never processes events for), blocking the workflow indefinitely with no diagnostic. -func Test_Select_RejectsTaskFromDifferentContext(t *testing.T) { - ctx := newTestContext(t) - other := newTestContext(t) + winner, err := ctx.Select(taskA, taskB) + if err != nil { + return nil, err + } + return winner, nil + })) - own := newTask(ctx) - foreign := newTask(other) + actions, cl := runBuffered(t, r, nil, []*protos.HistoryEvent{ + evExecutionStarted("wf"), + evSuspended(), + // EventB (Select index 1) is raised first in the suspended batch; EventA (Select index 0) + // second. If Select's tie-break preferred history order, it would return 1 here. + evEventRaised("EventB"), + evEventRaised("EventA"), + evResumed(), + }) - _, err := ctx.Select(own, foreign) - require.EqualError(t, err, "task at index 1 belongs to a different WorkflowContext") + co := completeAction(t, actions) + require.NotNil(t, co) + assert.Equal(t, protos.OrchestrationStatus_ORCHESTRATION_STATUS_COMPLETED, co.WorkflowStatus) + assert.Equal(t, `0`, co.GetResult().GetValue(), "the lowest-index candidate (EventA) must win regardless of which event appeared first in the replayed batch") + assert.Empty(t, cl.warns) } // Benchmark_ReplaySequentialActivities measures a full replay of a workflow @@ -802,6 +820,54 @@ func Benchmark_ReplaySequentialActivities(b *testing.B) { } } +// Benchmark_SelectFanOut measures a full replay of a workflow that fans out 50 activities and then +// repeatedly Selects over whichever ones haven't completed yet until all 50 have -- the shape +// dominated by Select's own per-call cost, as opposed to Benchmark_ReplaySequentialActivities, +// which is dominated by Await/CallActivity's per-schedule bookkeeping. +func Benchmark_SelectFanOut(b *testing.B) { + const n = 50 + r := NewTaskRegistry() + if err := r.AddWorkflowN("wf", func(ctx *WorkflowContext) (any, error) { + pending := make([]Task, n) + for i := range pending { + pending[i] = ctx.CallActivity("act") + } + for len(pending) > 0 { + winner, err := ctx.Select(pending...) + if err != nil { + return nil, err + } + if err := pending[winner].Await(nil); err != nil { + return nil, err + } + pending = append(pending[:winner], pending[winner+1:]...) + } + return "done", nil + }); err != nil { + b.Fatal(err) + } + if err := r.AddActivityN("act", func(ActivityContext) (any, error) { return nil, nil }); err != nil { + b.Fatal(err) + } + + events := make([]*protos.HistoryEvent, 0, 2*n+1) + events = append(events, evExecutionStarted("wf")) + for i := range int32(n) { + events = append(events, evTaskScheduled(i, "act")) + } + for i := range int32(n) { + events = append(events, evTaskCompleted(i, `null`)) + } + + b.ReportAllocs() + for b.Loop() { + ctx := NewWorkflowContext(r, "bench", events, nil) + if actions := ctx.start(); len(actions) != 1 { + b.Fatalf("expected 1 action, got %d", len(actions)) + } + } +} + func Test_BufferedResolution_KindGuardOnOccupiedID(t *testing.T) { // A TaskCompleted whose id is occupied by a pending TIMER (here the // synthetic external event timer at id 0) must buffer rather than diff --git a/task/task.go b/task/task.go index db933e5a..c405328f 100644 --- a/task/task.go +++ b/task/task.go @@ -3,7 +3,6 @@ package task import ( "errors" "fmt" - "sort" "github.com/dapr/durabletask-go/api/protos" "github.com/dapr/durabletask-go/backend/runtimestate/dedup" @@ -32,17 +31,13 @@ type Task interface { } type completableTask struct { - workflowCtx *WorkflowContext - isCompleted bool - isCanceled bool - rawResult []byte - failureDetails *protos.TaskFailureDetails - // completedCallbacks holds the callbacks registered via onCompleted, keyed by an id from - // nextCallbackID so a specific registration can be removed (see the unregister function - // onCompleted returns) without disturbing any other registration. - completedCallbacks map[int64]func() - nextCallbackID int64 - taskExecutionId string + workflowCtx *WorkflowContext + isCompleted bool + isCanceled bool + rawResult []byte + failureDetails *protos.TaskFailureDetails + completedCallback func() + taskExecutionId string // kind is the resolution correlator family this task belongs to when it // is registered in pendingTasks (task, timer or child). A resolution // event only completes a pending entry of its own kind; anything else is @@ -98,34 +93,19 @@ func (t *completableTask) completionError() error { return nil } -// onCompleted registers [callback] to run when the task completes, and returns a function that -// unregisters it. Multiple callbacks may be registered on the same task (e.g. by both -// WaitForSingleEvent's internal timer plumbing and Select), and all of them run, in registration -// order, when the task completes. -// -// Callers that register a callback but then lose interest before the task completes (for example, -// Select registering on every candidate task but only caring about the one that actually won) must -// call the returned unregister function, or the callback is retained on the task indefinitely. -// Unregistering after the task has already completed, or more than once, is a no-op, and is safe to -// do regardless of what other callbacks have since been registered or unregistered on the task. -func (t *completableTask) onCompleted(callback func()) (unregister func()) { +// onCompleted registers [callback] to run when the task completes. Only one callback may be +// registered on a task at a time; each of this package's current callers (WaitForSingleEvent's +// internal timer plumbing, and the retry driver in internalScheduleTaskWithRetries) registers +// exactly one, on a task scoped to that single registration and never reused for another. +func (t *completableTask) onCompleted(callback func()) { // A task can already be completed at registration time when a buffered // early resolution was delivered as the task was scheduled; fire the - // callback immediately so completion side effects are not lost. There is - // nothing to unregister in this case since the callback was never stored. + // callback immediately so completion side effects are not lost. if t.isCompleted { callback() - return func() {} - } - id := t.nextCallbackID - t.nextCallbackID++ - if t.completedCallbacks == nil { - t.completedCallbacks = make(map[int64]func()) - } - t.completedCallbacks[id] = callback - return func() { - delete(t.completedCallbacks, id) + return } + t.completedCallback = callback } func (t *completableTask) complete(rawResult []byte) { @@ -145,19 +125,7 @@ func (t *completableTask) cancel() { func (t *completableTask) completeInternal() { t.isCompleted = true - callbacks := t.completedCallbacks - t.completedCallbacks = nil - if len(callbacks) == 0 { - return - } - // Callback ids are assigned in registration order (see onCompleted), so sorting by id runs - // them in the same deterministic order regardless of map iteration order. - ids := make([]int64, 0, len(callbacks)) - for id := range callbacks { - ids = append(ids, id) - } - sort.Slice(ids, func(i, j int) bool { return ids[i] < ids[j] }) - for _, id := range ids { - callbacks[id]() + if t.completedCallback != nil { + t.completedCallback() } } diff --git a/workflow/workflow.go b/workflow/workflow.go index f6e4c4ed..821272d2 100644 --- a/workflow/workflow.go +++ b/workflow/workflow.go @@ -101,9 +101,9 @@ func (w *WorkflowContext) WaitForExternalEvent(eventName string, timeout time.Du // error; the remaining tasks are left pending and may still be selected or awaited later (for // example, in a loop that repeatedly selects over the tasks that have not yet completed). // -// A task that was already completed before Select was called is treated as an immediate winner. If -// more than one of the given tasks is already completed at the time of the call, the one with the -// lowest index wins. +// If more than one of the given tasks is found completed at the same time -- whether because they +// were already completed before Select was called, or a single event completed several of them at +// once -- the one with the lowest index wins; this is the only tie-break rule Select ever applies. // // Select requires at least one task and returns an error if no tasks are given, if any task is nil, // or if any task was not obtained from this same WorkflowContext (e.g. via CallActivity, CreateTimer, From 7a0c25d63beda53e87e3f7063a32fdad01d41d0f Mon Sep 17 00:00:00 2001 From: gomitrah Date: Thu, 1 Oct 2026 12:32:53 +0530 Subject: [PATCH 08/12] task: make retry advancement lazy, fix replay and background-retry bugs Per review: the retry-driver rewrite from an earlier commit on this branch, which made retry-configured tasks pollable by Select, had two correctness regressions versus main: - Replay compatibility: the backoff timer's sequence number was allocated inside delegate.onCompleted, i.e. while the TaskFailed event itself was being processed, instead of where main allocated it -- inside Await, when workflow code actually observes the failure. Any workflow scheduling something between those two points got a different action ordering than before, breaking replay for histories recorded pre-upgrade. - Background retries: because retries were driven reactively from onCompleted, a failed attempt retried even if nothing ever awaited or selected it, including after the workflow function had already returned, emitting actions for a task nobody was watching. internalScheduleTaskWithRetries is now driven lazily: the outer task gets an advance func(), invoked by a new pollCompleted() that Await and Select's poll both call instead of reading isCompleted directly. advance only runs when something actually polls the task, and it's where the backoff timer and next attempt get scheduled -- so a workflow observes a retry at the same point it always has (Await, or Select's poll, immediately after each event), and an unobserved attempt never retries at all. Added Test_RetryPolicy_UnobservedAttemptNeverRetries and Test_RetryPolicy_TimerSequenceIdMatchesAwaitObservationPoint, reproducing both bugs directly; verified both fail against the prior commit and pass against this one. Also per review: WaitForSingleEvent/Select have a real hazard where a losing WaitForSingleEvent task, if discarded rather than carried into the next Select call, stays queued and steals the next same-named event meant for a new task -- the exact approval-gate loop from dapr/dapr#10447. Documented the required carry-forward pattern on both methods (and their workflow-package mirrors) rather than adding new API surface. Added Test_Select_ApprovalGateLoop_CarriesLosingWaitForward, which wins iteration 1 with "Approve" (so "Reject" is a genuine loser to carry forward) before iteration 2 is won by a fresh "Reject"; verified it hangs to timeout if the carry-forward in its own workflow body is broken. Fixed Test_Select_ActivityRacesTimer's flakiness (wall-clock sleep racing a timer under load) by blocking the activity on a channel instead, closed before worker.Shutdown so its drain doesn't wait on the now-unblocked activity forever. Reworded a few doc comments per review (ErrTaskNotSelectable, the "Task implementation from outside this package" phrasing, a stale onTaskFailed comment referencing the now-removed onCompleted-based retry path) and trimmed internalScheduleTaskWithRetries's doc. Signed-off-by: gomitrah --- task/orchestrator.go | 137 ++++++++++++++--------------- task/orchestrator_buffered_test.go | 93 ++++++++++++++++++-- task/task.go | 31 +++++-- tests/select_test.go | 82 ++++++++++++++++- workflow/workflow.go | 19 +++- 5 files changed, 272 insertions(+), 90 deletions(-) diff --git a/task/orchestrator.go b/task/orchestrator.go index b3829ee8..f7c8ea0a 100644 --- a/task/orchestrator.go +++ b/task/orchestrator.go @@ -558,77 +558,64 @@ func (ctx *WorkflowContext) internalCallChildWorkflow(workflowName string, optio } // internalScheduleTaskWithRetries schedules [schedule]'s first attempt immediately and returns a -// plain *completableTask representing the outcome of the whole retry chain: it completes -// successfully as soon as any attempt succeeds, or with the final attempt's failure once retries -// are exhausted. Unlike the rest of an attempt's own completion, which is driven by history events, -// the decision to retry (creating a backoff timer, then scheduling the next attempt) is driven -// reactively from each attempt's and timer's onCompleted callback rather than by calling Await, so -// the returned task's completion never depends on the caller calling Await -- in particular, so it -// can be safely passed to Select like any other task (see ErrTaskNotSelectable's history: an -// earlier version of this function returned a wrapper task whose retry logic only ran inside -// Await, which made it impossible for Select to safely observe without either driving retries -// itself or silently leaving an already-scheduled attempt orphaned). +// plain *completableTask for the whole retry chain. The chain advances -- deciding whether a failed +// attempt retries, arming its backoff timer, scheduling the next attempt -- only when the returned +// task is polled (by Await or Select), never as a side effect of history alone: an attempt nobody +// observes stays failed and retries no further. A decode error from Await(v) on a successful +// attempt is returned as-is and never triggers a retry. func (ctx *WorkflowContext) internalScheduleTaskWithRetries(name string, initialAttempt time.Time, schedule func(taskExecutionId string, isRetry bool) *completableTask, policy RetryPolicy, retryCount int, taskExecutionId string, setTimerOrigin func(*protos.CreateTimerAction, string)) Task { outer := newTask(ctx) + current := schedule(taskExecutionId, retryCount > 0) + var timer *completableTask - // giveUp completes outer with whichever terminal state [t] (an attempt or a backoff timer) - // ended in: canceled if [t] was canceled, failed with the same details otherwise. - giveUp := func(t *completableTask) { - if t.isCanceled { - outer.cancel() - return - } - outer.fail(t.failureDetails) - } - - var attempt func(retryCount int, taskExecutionId string) - attempt = func(retryCount int, taskExecutionId string) { - delegate := schedule(taskExecutionId, retryCount > 0) - delegate.onCompleted(func() { - // Once the delegate resolves, its TaskExecutionId is the one recorded on the - // TaskScheduled/TaskCompleted/TaskFailed history events for this attempt, which is - // what must be threaded into the retry timer and the next attempt so replay stays - // deterministic; the taskExecutionId this attempt was scheduled with (a value that, - // for every attempt after the first, was itself only known once its predecessor's - // delegate resolved) is no longer authoritative once history says otherwise. - taskExecutionId := delegate.TaskExecutionId() - - err := delegate.completionError() + outer.advance = func() { + for { + if timer != nil { + if !timer.isCompleted { + return + } + // Observing the fired timer is itself what schedules the next attempt, and its + // action's sequence number is assigned here -- at the same point Await/Select's + // poll notices the timer fired -- matching where the previous, Await-driven retry + // design allocated it, so replay stays compatible with histories recorded before + // retries became pollable. + current = schedule(taskExecutionId, true) + timer = nil + retryCount++ + continue + } + + if !current.isCompleted { + return + } + + // TaskExecutionId reflects what's recorded on this attempt's TaskScheduled/ + // TaskCompleted/TaskFailed history events, which is what must be threaded into the + // next attempt and its backoff timer's origin so replay stays deterministic. + taskExecutionId = current.TaskExecutionId() + + err := current.completionError() if err == nil { - outer.complete(delegate.rawResult) + outer.complete(current.rawResult) return } if retryCount+1 >= policy.MaxAttempts { - // next try will exceed the max attempts, dont continue - giveUp(delegate) + outer.fail(current.failureDetails) return } - nextDelay := computeNextDelay(ctx.CurrentTimeUtc, policy, retryCount, initialAttempt, err) if nextDelay == 0 { - giveUp(delegate) + outer.fail(current.failureDetails) return } - timer, action := ctx.createTimerInternal(&name, nextDelay) + var action *protos.CreateTimerAction + timer, action = ctx.createTimerInternal(&name, nextDelay) setTimerOrigin(action, taskExecutionId) - timer.onCompleted(func() { - if timer.completionError() != nil { - // Defensive: nothing in this codebase currently cancels or fails a - // pending timer task -- onTimerFired is the only resolver for one, and - // it always calls complete(nil) -- so this is unreachable today. It - // guards against silently retrying forever (attempt would otherwise run - // again below) if that ever changes, giving up with the timer's own - // terminal state instead. - giveUp(timer) - return - } - attempt(retryCount+1, taskExecutionId) - }) - }) + return + } } - attempt(retryCount, taskExecutionId) return outer } @@ -727,6 +714,16 @@ func (ctx *WorkflowContext) createExternalEventTimerInternal(eventName string, f // Workflows can wait for the same event name multiple times, so waiting for multiple events with the same name // is allowed. Each event received by an workflow will complete just one task returned by this method. // +// A task returned by this method that is passed to [WorkflowContext.Select] but loses -- another +// candidate completes first -- is not retired: it remains queued for its event name. If the +// workflow then calls WaitForSingleEvent again for that same name (for example, re-selecting in a +// loop after handling the winner) the two tasks queue in call order, and the next matching event +// completes whichever of them is oldest, not necessarily the one just created. A loop over Select +// that discards losing tasks between iterations can therefore end up permanently waiting on a task +// no later iteration still holds a reference to. To race the same event name across iterations +// safely, carry every losing task forward into the next Select call instead of creating a new one +// for a name still pending. +// // Note that event names are case-insensitive. func (ctx *WorkflowContext) WaitForSingleEvent(eventName string, timeout time.Duration) Task { task := newTask(ctx) @@ -784,7 +781,10 @@ func (ctx *WorkflowContext) WaitForSingleEvent(eventName string, timeout time.Du // Select blocks until the first of the given [tasks] completes and returns its index. Once Select // returns, callers should call Await on the task at the returned index to obtain its result or // error; the remaining tasks are left pending and may still be selected or awaited later (for -// example, in a loop that repeatedly selects over the tasks that have not yet completed). +// example, in a loop that repeatedly selects over the tasks that have not yet completed) -- doing so +// is required, not optional, for a losing [WorkflowContext.WaitForSingleEvent] task: see its doc +// comment for why discarding one instead of carrying it forward can hang a later Select on the same +// event name. // // Select polls the given tasks' completion state after each history event is processed, so if more // than one of them is found completed at the same time -- whether because they were already @@ -796,8 +796,8 @@ func (ctx *WorkflowContext) WaitForSingleEvent(eventName string, timeout time.Du // or if any task was not obtained from this same WorkflowContext (e.g. via CallActivity, CreateTimer, // or WaitForSingleEvent, with or without a retry policy) -- a task from a different WorkflowContext // can never complete from this context's point of view, which would otherwise block the workflow -// indefinitely with no diagnostic. A Task implementation from outside this package is also rejected, -// with ErrTaskNotSelectable. +// indefinitely with no diagnostic. A Task not created by a WorkflowContext method is rejected with +// ErrTaskNotSelectable. // // Like Await, Select may panic with ErrTaskBlocked as the panic value when none of the tasks have // completed and there is no further history to process. This is normal control flow for workflow @@ -825,18 +825,16 @@ func (ctx *WorkflowContext) Select(tasks ...Task) (int, error) { completable[i] = ct } - // Tasks complete only inside the single-threaded processing of a history event (see - // processNextEvent), so no task can transition to completed between one poll of completable and - // the next: polling here needs no completion callbacks, unlike WaitForSingleEvent's internal - // timer plumbing, which reacts to a specific other task's completion from outside this call - // entirely. A single history event can still complete more than one candidate at once -- e.g. - // onExecutionResumed replays a whole batch of events buffered during a suspension within one - // processNextEvent call -- so this always scans in index order and takes the first completed - // task it finds, rather than assuming at most one newly-completed candidate per poll. + // A single history event can complete more than one candidate at once -- e.g. onExecutionResumed + // replays a whole batch of events buffered during a suspension within one processNextEvent call + // -- so this always scans every candidate in index order and takes the first completed task it + // finds, rather than stopping at the first newly-completed one. pollCompleted (rather than + // reading isCompleted) is what lets a retry-configured candidate (see + // internalScheduleTaskWithRetries) advance its own retry chain by being polled here. winner := -1 if err := ctx.awaitUntil(func() bool { for i, ct := range completable { - if ct.isCompleted { + if ct.pollCompleted() { winner = i return true } @@ -1038,10 +1036,9 @@ func (ctx *WorkflowContext) onTaskFailed(tf *protos.TaskFailedEvent) error { delete(ctx.pendingTasks, taskID) ctx.resolvedResolutions[key] = struct{}{} - // taskExecutionId must be set before fail() so that a completion callback registered via - // onCompleted (e.g. a retry chain reading TaskExecutionId to schedule its next attempt) sees - // it already populated: fail() runs registered callbacks synchronously as part of completing - // the task, before returning here. + // Set before fail() so TaskExecutionId() already reflects this event's value for anything that + // observes the task's completion afterward (e.g. a retry chain's advance, polled later by + // Await/Select). task.taskExecutionId = tf.TaskExecutionId // completing a task will resume the corresponding Await() call task.fail(tf.FailureDetails) diff --git a/task/orchestrator_buffered_test.go b/task/orchestrator_buffered_test.go index 8669f8c5..658e4d0a 100644 --- a/task/orchestrator_buffered_test.go +++ b/task/orchestrator_buffered_test.go @@ -532,10 +532,10 @@ func Test_BufferedResolution_EarlyFailureWithRetryPolicy(t *testing.T) { evEventRaised("go"), }) - // The buffered failure resolves attempt one at scheduling time and the - // retry wrapper immediately arms the backoff timer: the workflow blocks - // on the retry timer instead of completing, the failed attempt's - // ScheduleTask is suppressed, and the retry timer action is emitted. + // The buffered failure resolves attempt one as soon as it's scheduled, so by the time Await + // polls it, the retry chain is already in its failed-with-retries-remaining state and arms the + // backoff timer right there: the workflow blocks on the retry timer instead of completing, the + // failed attempt's ScheduleTask is suppressed, and the retry timer action is emitted. assert.Nil(t, completeAction(t, actions)) assert.Zero(t, countActions(actions, isScheduleTask)) assert.Equal(t, 1, countActions(actions, func(a *protos.WorkflowAction) bool { @@ -544,6 +544,85 @@ func Test_BufferedResolution_EarlyFailureWithRetryPolicy(t *testing.T) { assert.Empty(t, cl.warns) } +// Test_RetryPolicy_UnobservedAttemptNeverRetries is a regression test: a retry-configured task that +// nothing ever awaits or selects must not retry in the background. The retry chain only advances +// when its outer task is polled (see internalScheduleTaskWithRetries's advance hook), so a failed +// attempt nobody observes stays failed and emits no further actions, even though the workflow +// itself goes on to complete via a different, awaited task. +func Test_RetryPolicy_UnobservedAttemptNeverRetries(t *testing.T) { + r := NewTaskRegistry() + require.NoError(t, r.AddWorkflowN("wf", func(ctx *WorkflowContext) (any, error) { + _ = ctx.CallActivity("A", WithActivityRetryPolicy(&RetryPolicy{ + MaxAttempts: 3, + InitialRetryInterval: time.Second, + BackoffCoefficient: 2, + })) + b := ctx.CallActivity("B") + return nil, b.Await(nil) + })) + require.NoError(t, r.AddActivityN("A", func(ActivityContext) (any, error) { return nil, nil })) + require.NoError(t, r.AddActivityN("B", func(ActivityContext) (any, error) { return nil, nil })) + + actions, cl := runBuffered(t, r, nil, []*protos.HistoryEvent{ + evExecutionStarted("wf"), + evTaskFailed(0, "exec-a"), + evTaskCompleted(1, `null`), + }) + + co := completeAction(t, actions) + require.NotNil(t, co, "the workflow must complete once its only awaited task (B) resolves") + assert.Equal(t, protos.OrchestrationStatus_ORCHESTRATION_STATUS_COMPLETED, co.WorkflowStatus) + assert.Zero(t, countActions(actions, func(a *protos.WorkflowAction) bool { return a.GetCreateTimer() != nil }), + "A's failure was never observed, so no retry backoff timer may be armed for it") + assert.Empty(t, cl.warns) +} + +// Test_RetryPolicy_TimerSequenceIdMatchesAwaitObservationPoint is a regression test for replay +// compatibility: the retry backoff timer's action id (sequence number) must match where the +// workflow function actually observes the failed attempt by calling Await, not where the attempt +// happens to fail in history. A workflow that schedules C between observing A's failure and +// awaiting A again must see C take the sequence id that would otherwise go to A's retry timer, and +// the retry timer take the next id after that -- exactly as if the retry decision were made inline +// in Await itself, which is what histories recorded before retries became pollable assume. +func Test_RetryPolicy_TimerSequenceIdMatchesAwaitObservationPoint(t *testing.T) { + r := NewTaskRegistry() + require.NoError(t, r.AddWorkflowN("wf", func(ctx *WorkflowContext) (any, error) { + a := ctx.CallActivity("A", WithActivityRetryPolicy(&RetryPolicy{ + MaxAttempts: 3, + InitialRetryInterval: time.Second, + BackoffCoefficient: 2, + })) // id 0 + b := ctx.CallActivity("B") // id 1 + if err := b.Await(nil); err != nil { + return nil, err + } + c := ctx.CallActivity("C") // id 2: scheduled before A's failure is ever observed + if err := a.Await(nil); err != nil && !errors.Is(err, ErrTaskBlocked) { + // A's retry timer is armed here, once Await actually polls A -- this call blocks + // (processNextEvent runs out of history) rather than returning an error. + } + return nil, c.Await(nil) + })) + require.NoError(t, r.AddActivityN("A", func(ActivityContext) (any, error) { return nil, nil })) + require.NoError(t, r.AddActivityN("B", func(ActivityContext) (any, error) { return nil, nil })) + require.NoError(t, r.AddActivityN("C", func(ActivityContext) (any, error) { return nil, nil })) + + actions, cl := runBuffered(t, r, nil, []*protos.HistoryEvent{ + evExecutionStarted("wf"), + evTaskFailed(0, "exec-a"), + evTaskCompleted(1, `null`), + }) + + require.Equal(t, 3, countActions(actions, isScheduleTask), "A's first attempt, B, and C are each scheduled on this turn") + require.True(t, countActions(actions, func(a *protos.WorkflowAction) bool { + return a.GetScheduleTask().GetName() == "C" && a.Id == 2 + }) == 1, "C must take sequence id 2, the id it would get if A's retry were still decided inside Await rather than eagerly on TaskFailed") + require.True(t, countActions(actions, func(a *protos.WorkflowAction) bool { + return a.GetCreateTimer() != nil && a.Id == 3 + }) == 1, "A's retry backoff timer must take the next id after C, since Await only observes A's failure after C is scheduled") + assert.Empty(t, cl.warns) +} + func Test_BufferedResolution_FanOutTwoEarlyCompletions(t *testing.T) { r := NewTaskRegistry() require.NoError(t, r.AddWorkflowN("wf", func(ctx *WorkflowContext) (any, error) { @@ -700,9 +779,9 @@ func Test_CompletableTask_OnCompletedAfterCompletion(t *testing.T) { assert.True(t, fired, "onCompleted on an already completed task must fire immediately") } -// Test_Select_RejectsNilTask is a regression test: Select(a, nil) used to report the nil task as -// ErrTaskNotSelectable (the same error as an unrelated, unsupported Task implementation), which -// misidentified the actual problem. +// Test_Select_RejectsNilTask verifies that Select reports a nil task explicitly, rather than +// misattributing it to ErrTaskNotSelectable, the error for an unrelated, unsupported Task +// implementation. func Test_Select_RejectsNilTask(t *testing.T) { ctx := newTestContext(t) other := newTask(ctx) diff --git a/task/task.go b/task/task.go index c405328f..67b5c780 100644 --- a/task/task.go +++ b/task/task.go @@ -17,11 +17,8 @@ var ErrTaskBlocked = errors.New("the current task is blocked") // when configured timeouts expire. var ErrTaskCanceled = errors.New("the task was canceled") // CONSIDER: More specific info about the task -// ErrTaskNotSelectable is returned by [WorkflowContext.Select] when one of the given tasks isn't -// backed by this package's own task implementation, and so doesn't support the completion-callback -// hook Select relies on to detect a winner without calling Await. Every Task returned by a -// WorkflowContext method is selectable; this only happens for a Task implementation from outside -// this package. +// ErrTaskNotSelectable is returned by [WorkflowContext.Select] when one of the given tasks was not +// created by a WorkflowContext method. var ErrTaskNotSelectable = errors.New("task does not support Select") // Task is an interface for asynchronous durable tasks. A task is conceptually similar to a future. @@ -44,6 +41,22 @@ type completableTask struct { // buffered. Zero (KindNone) for tasks never held in pendingTasks, such // as external event wait tasks. kind dedup.Kind + // advance, when set, runs on every poll (see pollCompleted) before isCompleted is read. Used by + // a retry chain's outer task (see internalScheduleTaskWithRetries) to drive itself forward -- + // start the next attempt once a backoff timer fires, fail or complete once an attempt's outcome + // is known -- only when something is actually polling for completion, not as a side effect of + // history alone. + advance func() +} + +// pollCompleted runs t's advance hook, if any, then reports whether t is completed. Await and +// Select's poll both call this instead of reading isCompleted directly, so that observing a task is +// what drives a retry chain forward. +func (t *completableTask) pollCompleted() bool { + if t.advance != nil { + t.advance() + } + return t.isCompleted } func newTask(ctx *WorkflowContext) *completableTask { @@ -62,7 +75,7 @@ func newTask(ctx *WorkflowContext) *completableTask { // of any kind. However, workflow functions must never attempt to recover from such panics to ensure that // the workflow execution can proceed normally. func (t *completableTask) Await(v any) error { - if err := t.workflowCtx.awaitUntil(func() bool { return t.isCompleted }); err != nil { + if err := t.workflowCtx.awaitUntil(t.pollCompleted); err != nil { return err } if err := t.completionError(); err != nil { @@ -94,9 +107,9 @@ func (t *completableTask) completionError() error { } // onCompleted registers [callback] to run when the task completes. Only one callback may be -// registered on a task at a time; each of this package's current callers (WaitForSingleEvent's -// internal timer plumbing, and the retry driver in internalScheduleTaskWithRetries) registers -// exactly one, on a task scoped to that single registration and never reused for another. +// registered on a task at a time; this package's only current caller, WaitForSingleEvent's internal +// timer plumbing, registers exactly one, on a task scoped to that single registration and never +// reused for another. func (t *completableTask) onCompleted(callback func()) { // A task can already be completed at registration time when a buffered // early resolution was delivered as the task was scheduled; fire the diff --git a/tests/select_test.go b/tests/select_test.go index 6015a024..2777f18c 100644 --- a/tests/select_test.go +++ b/tests/select_test.go @@ -261,14 +261,21 @@ func Test_Select_ActivityRacesTimer(t *testing.T) { } return "timer won", nil }) + // Block until the test is done instead of racing a wall-clock sleep against the timer: with a + // fixed sleep, the sqlite backend's poll backoff could delay TimerFired past the activity's + // own completion under load, making the activity win and the test fail. + release := make(chan struct{}) r.AddActivityN("SlowActivity", func(ctx task.ActivityContext) (any, error) { - time.Sleep(1 * time.Second) + <-release return "too slow", nil }) ctx := context.Background() client, worker := initTaskHubWorker(ctx, r) defer worker.Shutdown(ctx) + // Registered after worker.Shutdown's defer, so it runs first (LIFO) and unblocks the activity + // before Shutdown's StopAndDrain waits on it. + defer close(release) id, err := client.ScheduleNewWorkflow(ctx, "SelectActivityRacesTimerWorkflow") require.NoError(t, err) @@ -319,3 +326,76 @@ func Test_Select_WaitForSingleEventTimeoutWins(t *testing.T) { require.Equal(t, protos.OrchestrationStatus_ORCHESTRATION_STATUS_COMPLETED, metadata.RuntimeStatus) assert.Equal(t, `"timed out as expected"`, metadata.Output.Value) } + +// Test_Select_ApprovalGateLoop_CarriesLosingWaitForward demonstrates the approval-gate pattern from +// https://github.com/dapr/dapr/issues/10447 across multiple iterations of the same event names: on +// each iteration the workflow races WaitForSingleEvent("Approve", ...) against +// WaitForSingleEvent("Reject", ...) via Select, and must carry the losing task into the next +// iteration's Select call rather than discarding it and creating a fresh WaitForSingleEvent for the +// same name -- see WaitForSingleEvent's doc comment for why discarding it would hang. +// +// Iteration 1 is won by "Approve", leaving "Reject" as a genuine loser that must be carried +// forward; iteration 2 is then won by a fresh "Reject". This specifically exercises the hazard: if +// the carried-forward "Reject" from iteration 1 were discarded and a new one created instead, the +// stale task would still be the oldest queued waiter for that name and would steal iteration 2's +// "Reject" event out from under the new task, hanging the workflow. Racing two iterations both won +// by the same name (e.g. "Reject" then "Reject") would not catch this: nothing would have lost yet +// for that name to go stale. +func Test_Select_ApprovalGateLoop_CarriesLosingWaitForward(t *testing.T) { + r := task.NewTaskRegistry() + r.AddWorkflowN("ApprovalGateLoopWorkflow", func(ctx *task.WorkflowContext) (any, error) { + approve := ctx.WaitForSingleEvent("Approve", -1) + reject := ctx.WaitForSingleEvent("Reject", -1) + + var decisions []string + for i := 0; i < 2; i++ { + winner, err := ctx.Select(approve, reject) + if err != nil { + return nil, err + } + + var v string + switch winner { + case 0: + if err := approve.Await(&v); err != nil { + return nil, err + } + decisions = append(decisions, "Approve:"+v) + // Carry the losing "Reject" wait forward; a fresh WaitForSingleEvent("Reject", ...) + // here would leave this one permanently queued and stealing the next "Reject" event. + approve = ctx.WaitForSingleEvent("Approve", -1) + case 1: + if err := reject.Await(&v); err != nil { + return nil, err + } + decisions = append(decisions, "Reject:"+v) + reject = ctx.WaitForSingleEvent("Reject", -1) + } + } + return decisions, nil + }) + + ctx := context.Background() + client, worker := initTaskHubWorker(ctx, r) + defer worker.Shutdown(ctx) + + id, err := client.ScheduleNewWorkflow(ctx, "ApprovalGateLoopWorkflow") + require.NoError(t, err) + _, err = client.WaitForWorkflowStart(ctx, id) + require.NoError(t, err) + + // Iteration 1 is won by "Approve", so the original "Reject" task loses and must be carried + // forward. Iteration 2 is then resolved by a second "Reject" event: if the carry-forward were + // wrong (losing task discarded, a fresh one created instead), the stale iteration-1 "Reject" + // task would still be the oldest queued waiter for that name and would steal this event, + // leaving iteration 2's Select -- and the test -- hanging until the timeout below. + require.NoError(t, client.RaiseEvent(ctx, id, "Approve", api.WithEventPayload("first"))) + require.NoError(t, client.RaiseEvent(ctx, id, "Reject", api.WithEventPayload("second"))) + + timeoutCtx, cancel := context.WithTimeout(ctx, 5*time.Second) + defer cancel() + metadata, err := client.WaitForWorkflowCompletion(timeoutCtx, id) + require.NoError(t, err) + require.Equal(t, protos.OrchestrationStatus_ORCHESTRATION_STATUS_COMPLETED, metadata.RuntimeStatus) + assert.Equal(t, `["Approve:first","Reject:second"]`, metadata.Output.Value) +} diff --git a/workflow/workflow.go b/workflow/workflow.go index 821272d2..4c8d4a57 100644 --- a/workflow/workflow.go +++ b/workflow/workflow.go @@ -91,6 +91,16 @@ func (w *WorkflowContext) CreateTimer(delay time.Duration, opts ...CreateTimerOp // multiple events with the same name is allowed. Each event received by an // workflow will complete just one task returned by this method. // +// A task returned by this method that is passed to [WorkflowContext.Select] but loses -- another +// candidate completes first -- is not retired: it remains queued for its event name. If the +// workflow then calls WaitForExternalEvent again for that same name (for example, re-selecting in a +// loop after handling the winner) the two tasks queue in call order, and the next matching event +// completes whichever of them is oldest, not necessarily the one just created. A loop over Select +// that discards losing tasks between iterations can therefore end up permanently waiting on a task +// no later iteration still holds a reference to. To race the same event name across iterations +// safely, carry every losing task forward into the next Select call instead of creating a new one +// for a name still pending. +// // Note that event names are case-insensitive. func (w *WorkflowContext) WaitForExternalEvent(eventName string, timeout time.Duration) Task { return w.oc.WaitForSingleEvent(eventName, timeout) @@ -99,7 +109,10 @@ func (w *WorkflowContext) WaitForExternalEvent(eventName string, timeout time.Du // Select blocks until the first of the given [tasks] completes and returns its index. Once Select // returns, callers should call Await on the task at the returned index to obtain its result or // error; the remaining tasks are left pending and may still be selected or awaited later (for -// example, in a loop that repeatedly selects over the tasks that have not yet completed). +// example, in a loop that repeatedly selects over the tasks that have not yet completed) -- doing so +// is required, not optional, for a losing [WorkflowContext.WaitForExternalEvent] task: see its doc +// comment for why discarding one instead of carrying it forward can hang a later Select on the same +// event name. // // If more than one of the given tasks is found completed at the same time -- whether because they // were already completed before Select was called, or a single event completed several of them at @@ -109,8 +122,8 @@ func (w *WorkflowContext) WaitForExternalEvent(eventName string, timeout time.Du // or if any task was not obtained from this same WorkflowContext (e.g. via CallActivity, CreateTimer, // or WaitForExternalEvent, with or without a retry policy) -- a task from a different WorkflowContext // can never complete from this context's point of view, which would otherwise block the workflow -// indefinitely with no diagnostic. A Task implementation from outside this package is also rejected, -// with [ErrTaskNotSelectable]. +// indefinitely with no diagnostic. A Task not created by a WorkflowContext method is rejected with +// [ErrTaskNotSelectable]. // // Like Await, Select may panic with [task.ErrTaskBlocked] as the panic value when none of the tasks // have completed and there is no further history to process. This is normal control flow for From b2dd73d320e7a18bc603767d6344576e589d76d7 Mon Sep 17 00:00:00 2001 From: gomitrah Date: Tue, 6 Oct 2026 12:17:10 +0530 Subject: [PATCH 09/12] task: stop retry advance re-running after completion, poll every Select candidate Per review, two correctness bugs in the lazy retry-advancement design: - advance kept running its failure-handling branch on every poll even after the outer task had already completed, so a second Await (or a Select that still included the task) called policy.Handle again and could arm a new backoff timer for a task the caller had already seen fail. Fixed with a guard at the top of the closure: if outer is already completed, return immediately. - Select's poll loop short-circuited at the first completed candidate, so a losing retry-configured candidate later in argument order never got its own advance run during that call. Its backoff timer was left for whenever something later happened to poll it, making the timer's action sequence number depend on which candidate won rather than only on argument order and history -- swapping two Select arguments could silently change action ids and break replay. Fixed by polling every candidate on every check instead of stopping at the first completion, keeping the lowest-index tie-break via a winner < 0 guard instead of an early return. Added Test_RetryPolicy_SecondAwaitDoesNotReRunChain and Test_Select_PollsEveryCandidateRegardlessOfWinner, both verified to fail against the prior behavior and pass against the fix. Also added the one-sentence doc clarification on Select that two retry candidates failing within the same poll still get their backoff timers allocated in argument order, not simultaneously. Signed-off-by: gomitrah --- task/orchestrator.go | 26 ++++--- task/orchestrator_buffered_test.go | 113 +++++++++++++++++++++++++++++ 2 files changed, 129 insertions(+), 10 deletions(-) diff --git a/task/orchestrator.go b/task/orchestrator.go index 30f598e5..f91c6c72 100644 --- a/task/orchestrator.go +++ b/task/orchestrator.go @@ -566,6 +566,9 @@ func (ctx *WorkflowContext) internalScheduleTaskWithRetries(name string, initial var timer *completableTask outer.advance = func() { + if outer.isCompleted { + return + } for { if timer != nil { if !timer.isCompleted { @@ -787,7 +790,9 @@ func (ctx *WorkflowContext) WaitForSingleEvent(eventName string, timeout time.Du // than one of them is found completed at the same time -- whether because they were already // completed before Select was called, or because a single history event completed several of them // at once (possible when a batch of events buffered during a suspended execution is replayed) -- -// the one with the lowest index wins; this is the only tie-break rule Select ever applies. +// the one with the lowest index wins; this is the only tie-break rule Select ever applies. When two +// or more of the given tasks are retry-configured and both become newly failed within the same +// poll, their backoff timers are still allocated in argument order, not simultaneously. // // Select requires at least one task and returns an error if no tasks are given, if any task is nil, // or if any task was not obtained from this same WorkflowContext (e.g. via CallActivity, CreateTimer, @@ -822,21 +827,22 @@ func (ctx *WorkflowContext) Select(tasks ...Task) (int, error) { completable[i] = ct } - // A single history event can complete more than one candidate at once -- e.g. onExecutionResumed - // replays a whole batch of events buffered during a suspension within one processNextEvent call - // -- so this always scans every candidate in index order and takes the first completed task it - // finds, rather than stopping at the first newly-completed one. pollCompleted (rather than - // reading isCompleted) is what lets a retry-configured candidate (see - // internalScheduleTaskWithRetries) advance its own retry chain by being polled here. + // Every candidate is polled on every check, not just until the first completed one is found: + // skipping a later candidate's pollCompleted call would skip its advance too (see + // internalScheduleTaskWithRetries), leaving a retry candidate's backoff timer allocated on + // whatever later poll happens to reach it instead of here -- making its action sequence number + // depend on which candidate happened to win, rather than only on argument order and history, the + // same way plain tasks already behave. A single history event can also complete more than one + // candidate at once (e.g. onExecutionResumed replaying a batch buffered during a suspension), so + // ties still go to the lowest index. winner := -1 if err := ctx.awaitUntil(func() bool { for i, ct := range completable { - if ct.pollCompleted() { + if ct.pollCompleted() && winner < 0 { winner = i - return true } } - return false + return winner >= 0 }); err != nil { return -1, err } diff --git a/task/orchestrator_buffered_test.go b/task/orchestrator_buffered_test.go index e9469caa..55728549 100644 --- a/task/orchestrator_buffered_test.go +++ b/task/orchestrator_buffered_test.go @@ -632,6 +632,119 @@ func Test_RetryPolicy_TimerSequenceIdMatchesAwaitObservationPoint(t *testing.T) assert.Empty(t, cl.warns) } +// Test_RetryPolicy_SecondAwaitDoesNotReRunChain is a regression test: once a retry-configured +// task's outer task has failed, polling it again (a second Await, or a Select that still includes +// it) must be a no-op -- it must not consult policy.Handle again, and it must not arm another +// backoff timer for a task the caller has already observed fail. MaxAttempts is left high enough +// that retries are nominally still available; policy.Handle itself declines the retry, which is the +// only way to reach the failure path without exhausting MaxAttempts, so a bug that re-runs this +// branch on every subsequent poll is caught by handleCalls incrementing again, not masked by +// retryCount already being at its limit. +func Test_RetryPolicy_SecondAwaitDoesNotReRunChain(t *testing.T) { + r := NewTaskRegistry() + var handleCalls int + require.NoError(t, r.AddWorkflowN("wf", func(ctx *WorkflowContext) (any, error) { + policy := &RetryPolicy{ + MaxAttempts: 3, + InitialRetryInterval: time.Second, + BackoffCoefficient: 2, + Handle: func(err error) bool { + handleCalls++ + return false + }, + } + flaky := ctx.CallActivity("Flaky", WithActivityRetryPolicy(policy)) // id 0 + err1 := flaky.Await(nil) + // The chain already failed (Handle declined the only attempt, resolved from buffered + // history below): this second Await must be a pure no-op that returns the same error. + err2 := flaky.Await(nil) + if err1 == nil || err2 == nil || err1.Error() != err2.Error() { + return nil, fmt.Errorf("expected both Awaits to return the same error, got %v and %v", err1, err2) + } + return nil, nil + })) + require.NoError(t, r.AddActivityN("Flaky", func(ActivityContext) (any, error) { return nil, nil })) + + actions, cl := runBuffered(t, r, nil, []*protos.HistoryEvent{ + evExecutionStarted("wf"), + evTaskFailed(0, "exec-1"), + }) + + co := completeAction(t, actions) + require.NotNil(t, co, "the workflow must complete once the retry chain fails and is observed twice") + assert.Equal(t, protos.OrchestrationStatus_ORCHESTRATION_STATUS_COMPLETED, co.WorkflowStatus) + assert.Zero(t, countActions(actions, func(a *protos.WorkflowAction) bool { return a.GetCreateTimer() != nil }), + "Handle declined the only attempt; no backoff timer may ever be armed, including on the second Await") + assert.Equal(t, 1, handleCalls, "policy.Handle must run exactly once, for the one real failure, not again on the second Await") + assert.Empty(t, cl.warns) +} + +// Test_Select_PollsEveryCandidateRegardlessOfWinner is a regression test: Select must poll every +// candidate on every check, not stop at the first one found completed. A candidate Select never +// polls because an earlier one already won never gets its own advance run (see +// internalScheduleTaskWithRetries), so a retry candidate's backoff timer is left for whenever +// something later happens to poll it again -- here, scheduling D right after Select -- instead of +// being allocated during the Select call itself. That makes the timer's action sequence number land +// after D's rather than before it, breaking replay for any history recorded before this fix. +// +// The shape here is load-bearing and deliberate: c.Await(nil) is what lets A's failure and B's +// completion land from already-present history before Select is ever called, and D is what gives +// the fix something to prove its ordering against. A bare comparison of Select(a, b) vs Select(b, a) +// with no intervening task would NOT catch this bug -- awaitUntil's very first poll runs before any +// history event is processed, so on that first call neither candidate is resolved yet regardless of +// argument order, and the short-circuit never gets a chance to matter. +func Test_Select_PollsEveryCandidateRegardlessOfWinner(t *testing.T) { + r := NewTaskRegistry() + require.NoError(t, r.AddWorkflowN("wf", func(ctx *WorkflowContext) (any, error) { + a := ctx.CallActivity("A", WithActivityRetryPolicy(&RetryPolicy{ // id 0 + MaxAttempts: 3, + InitialRetryInterval: time.Second, + BackoffCoefficient: 2, + })) + b := ctx.CallActivity("B") // id 1 + c := ctx.CallActivity("C") // id 2 + // A fails and B completes while waiting here; neither a nor b (the retry chain's outer + // task and the plain task) has been polled yet. + if err := c.Await(nil); err != nil { + return nil, err + } + + // B wins at once. If Select stopped polling once it found B's winning completion, A would + // never be polled this call, so its backoff timer would not be allocated here. + winner, err := ctx.Select(b, a) + if err != nil { + return nil, err + } + if winner != 0 { + return nil, fmt.Errorf("expected B (index 0) to win Select(b, a), got index %d", winner) + } + + if err := ctx.CallActivity("D").Await(nil); err != nil { // must get sequence id 4, after A's timer + return nil, err + } + return nil, a.Await(nil) + })) + require.NoError(t, r.AddActivityN("A", func(ActivityContext) (any, error) { return nil, nil })) + require.NoError(t, r.AddActivityN("B", func(ActivityContext) (any, error) { return nil, nil })) + require.NoError(t, r.AddActivityN("C", func(ActivityContext) (any, error) { return nil, nil })) + require.NoError(t, r.AddActivityN("D", func(ActivityContext) (any, error) { return nil, nil })) + + actions, cl := runBuffered(t, r, nil, []*protos.HistoryEvent{ + evExecutionStarted("wf"), + evTaskFailed(0, "exec-a"), + evTaskCompleted(1, `null`), + evTaskCompleted(2, `null`), + }) + + require.Equal(t, 1, countActions(actions, func(a *protos.WorkflowAction) bool { + return a.GetCreateTimer() != nil && a.Id == 3 + }), "A's retry timer must be allocated during the Select call that observed its failure (id 3), before D") + require.Equal(t, 1, countActions(actions, func(act *protos.WorkflowAction) bool { + return act.GetScheduleTask().GetName() == "D" && act.Id == 4 + }), "D must come after A's retry timer, not before it") + assert.Empty(t, cl.warns) +} + func Test_BufferedResolution_FanOutTwoEarlyCompletions(t *testing.T) { r := NewTaskRegistry() require.NoError(t, r.AddWorkflowN("wf", func(ctx *WorkflowContext) (any, error) { From 23133a33fb5bffb08cd5a573dfacff46afa4f513 Mon Sep 17 00:00:00 2001 From: gomitrah Date: Wed, 7 Oct 2026 11:31:57 +0530 Subject: [PATCH 10/12] tests,workflow: fix stale comments and doc duplication from review Signed-off-by: gomitrah --- tests/orchestrations_test.go | 11 +++----- tests/select_test.go | 11 ++++---- workflow/workflow.go | 53 ++++-------------------------------- 3 files changed, 15 insertions(+), 60 deletions(-) diff --git a/tests/orchestrations_test.go b/tests/orchestrations_test.go index ff7d5a4a..1d1c7f87 100644 --- a/tests/orchestrations_test.go +++ b/tests/orchestrations_test.go @@ -360,13 +360,10 @@ func Test_ActivityRetries(t *testing.T) { ) } -// Test_ActivityRetries_FanOutAwaitedOutOfOrder fans out several retry-configured activities -// together and awaits them in a different order than they were scheduled, replaying across -// multiple turns as each one fails, backs off, and retries. Retries schedule their backoff timer -// and next attempt reactively, as each attempt's TaskFailed event is processed, rather than lazily -// at whatever later point the workflow calls Await on that particular task; this exercises that a -// fan-out of several such tasks still assigns action sequence numbers consistently across replay -// regardless of the order the workflow function later awaits them in. +// Test_ActivityRetries_FanOutAwaitedOutOfOrder fans out several retry-configured activities and +// awaits them in a different order than they were scheduled, replaying across multiple turns as +// each one fails, backs off, and retries, and checks that action sequence numbers stay +// consistent across replay. func Test_ActivityRetries_FanOutAwaitedOutOfOrder(t *testing.T) { r := task.NewTaskRegistry() r.AddWorkflowN("ActivityRetriesFanOut", func(ctx *task.WorkflowContext) (any, error) { diff --git a/tests/select_test.go b/tests/select_test.go index 2777f18c..7b47637e 100644 --- a/tests/select_test.go +++ b/tests/select_test.go @@ -189,8 +189,8 @@ func Test_Select_NoTasks(t *testing.T) { } // Test_Select_RetryWrappedTaskWins verifies that a task returned by CallActivity with a retry -// policy is a real, selectable task: its retries are driven independently of Select or Await, so -// it can win a Select once its retries succeed, racing normally against a plain task. +// policy is a real, selectable task: Select's own polling drives its retries forward, so it can +// win a Select once its retries succeed, racing normally against a plain task. func Test_Select_RetryWrappedTaskWins(t *testing.T) { r := task.NewTaskRegistry() r.AddWorkflowN("SelectRetryWrappedWorkflow", func(ctx *task.WorkflowContext) (any, error) { @@ -239,10 +239,9 @@ func Test_Select_RetryWrappedTaskWins(t *testing.T) { } // Test_Select_ActivityRacesTimer covers the most common WhenAny shape: a plain (non-retry) -// CallActivity racing a CreateTimer. The activity's 1s sleep is deliberately much longer than the -// timer's 50ms delay so the timer wins with a wide margin, comfortably covering the extra latency -// the sqlite backend's polling (with its own exponential backoff) adds on top of the timer's raw -// delay before the workflow actually picks the TimerFired event up. +// CallActivity racing a CreateTimer. The activity blocks on a channel the test only closes after +// the timer has had a chance to win, so the timer wins deterministically rather than by a wall-clock +// margin that could be eaten by the sqlite backend's own polling latency. func Test_Select_ActivityRacesTimer(t *testing.T) { r := task.NewTaskRegistry() r.AddWorkflowN("SelectActivityRacesTimerWorkflow", func(ctx *task.WorkflowContext) (any, error) { diff --git a/workflow/workflow.go b/workflow/workflow.go index 4c8d4a57..c442ae79 100644 --- a/workflow/workflow.go +++ b/workflow/workflow.go @@ -77,59 +77,18 @@ func (w *WorkflowContext) CreateTimer(delay time.Duration, opts ...CreateTimerOp // WaitForExternalEvent creates a task that is completed only after an event // named [eventName] is received by this workflow or when the specified timeout -// expires. -// -// The [timeout] parameter can be used to define a timeout for receiving the -// event. If the timeout expires before the named event is received, the task -// will be completed and will return a timeout error value [ErrTaskCanceled] -// when awaited. Otherwise, the awaited task will return the deserialized -// payload of the received event. A Duration value of zero returns a canceled -// task if the event isn't already available in the history. Use a negative -// Duration to wait indefinitely for the event to be received. -// -// Workflows can wait for the same event name multiple times, so waiting for -// multiple events with the same name is allowed. Each event received by an -// workflow will complete just one task returned by this method. -// -// A task returned by this method that is passed to [WorkflowContext.Select] but loses -- another -// candidate completes first -- is not retired: it remains queued for its event name. If the -// workflow then calls WaitForExternalEvent again for that same name (for example, re-selecting in a -// loop after handling the winner) the two tasks queue in call order, and the next matching event -// completes whichever of them is oldest, not necessarily the one just created. A loop over Select -// that discards losing tasks between iterations can therefore end up permanently waiting on a task -// no later iteration still holds a reference to. To race the same event name across iterations -// safely, carry every losing task forward into the next Select call instead of creating a new one -// for a name still pending. -// -// Note that event names are case-insensitive. +// expires. See [task.WorkflowContext.WaitForSingleEvent] for the full behavior, +// including how a losing [WorkflowContext.Select] candidate must be carried +// forward rather than discarded. Note that event names are case-insensitive. func (w *WorkflowContext) WaitForExternalEvent(eventName string, timeout time.Duration) Task { return w.oc.WaitForSingleEvent(eventName, timeout) } // Select blocks until the first of the given [tasks] completes and returns its index. Once Select // returns, callers should call Await on the task at the returned index to obtain its result or -// error; the remaining tasks are left pending and may still be selected or awaited later (for -// example, in a loop that repeatedly selects over the tasks that have not yet completed) -- doing so -// is required, not optional, for a losing [WorkflowContext.WaitForExternalEvent] task: see its doc -// comment for why discarding one instead of carrying it forward can hang a later Select on the same -// event name. -// -// If more than one of the given tasks is found completed at the same time -- whether because they -// were already completed before Select was called, or a single event completed several of them at -// once -- the one with the lowest index wins; this is the only tie-break rule Select ever applies. -// -// Select requires at least one task and returns an error if no tasks are given, if any task is nil, -// or if any task was not obtained from this same WorkflowContext (e.g. via CallActivity, CreateTimer, -// or WaitForExternalEvent, with or without a retry policy) -- a task from a different WorkflowContext -// can never complete from this context's point of view, which would otherwise block the workflow -// indefinitely with no diagnostic. A Task not created by a WorkflowContext method is rejected with -// [ErrTaskNotSelectable]. -// -// Like Await, Select may panic with [task.ErrTaskBlocked] as the panic value when none of the tasks -// have completed and there is no further history to process. This is normal control flow for -// workflow functions, which must never recover from such panics: doing so prevents the workflow -// runtime's own recovery from observing the signal, and the workflow will never emit its pending -// actions. +// error; the remaining tasks are left pending and may still be selected or awaited later. See +// [task.WorkflowContext.Select] for the full behavior, including its tie-break rule, validation +// errors, and [task.ErrTaskBlocked] panic semantics. func (w *WorkflowContext) Select(tasks ...Task) (int, error) { otasks := make([]task.Task, len(tasks)) for i, t := range tasks { From 97ae0bb7292f5519cfc911999c4c42abe4c8b8f7 Mon Sep 17 00:00:00 2001 From: joshvanl Date: Wed, 7 Oct 2026 14:19:06 +0100 Subject: [PATCH 11/12] task,workflow: fix TaskExecutionId and cancel handling in retry chain TaskExecutionId() on a retry-configured task returned "". The outer task now takes the id of the failed attempt, as the previous design did. A canceled attempt completed the chain as a nil-error success. It now returns ErrTaskCanceled. Nothing cancels activity or child tasks today. internalScheduleTaskWithRetries loses its retryCount and taskExecutionId parameters. Both callers passed 0 and a fresh uuid. The retry chain doc states that a second Await or Select on a finished chain is a no-op, and that this and the decode-error change break replay of histories from the previous design. workflow.WaitForExternalEvent's doc gets its timeout semantics back. A dead ErrTaskBlocked check is removed from a test. Signed-off-by: joshvanl --- task/orchestrator.go | 30 +++- task/orchestrator_buffered_test.go | 158 ++++++++++++++++++- task/retry_replay_compat_test.go | 239 +++++++++++++++++++++++++++++ workflow/workflow.go | 22 ++- 4 files changed, 435 insertions(+), 14 deletions(-) create mode 100644 task/retry_replay_compat_test.go diff --git a/task/orchestrator.go b/task/orchestrator.go index bd974891..0dcddcb6 100644 --- a/task/orchestrator.go +++ b/task/orchestrator.go @@ -398,7 +398,7 @@ func (ctx *WorkflowContext) CallActivity(activity interface{}, opts ...CallActiv if options.retryPolicy != nil { return ctx.internalScheduleTaskWithRetries(activityName+"-retry", ctx.CurrentTimeUtc, func(taskExecutionId string, _ bool) *completableTask { return ctx.internalScheduleActivity(activityName, taskExecutionId, options) - }, *options.retryPolicy, 0, uuid.NewString(), func(a *protos.CreateTimerAction, execID string) { + }, *options.retryPolicy, func(a *protos.CreateTimerAction, execID string) { a.Origin = &protos.CreateTimerAction_ActivityRetry{ ActivityRetry: &protos.TimerOriginActivityRetry{ TaskExecutionId: execID, @@ -471,7 +471,7 @@ func (ctx *WorkflowContext) CallChildWorkflow(workflow interface{}, opts ...Chil return ctx.internalCallChildWorkflow(workflowName, options, &firstInstanceID) } return ctx.internalCallChildWorkflow(workflowName, options, nil) - }, *options.retryPolicy, 0, uuid.NewString(), func(a *protos.CreateTimerAction, _ string) { + }, *options.retryPolicy, func(a *protos.CreateTimerAction, _ string) { a.Origin = &protos.CreateTimerAction_ChildWorkflowRetry{ ChildWorkflowRetry: &protos.TimerOriginChildWorkflowRetry{ InstanceId: firstInstanceID, @@ -527,12 +527,27 @@ func (ctx *WorkflowContext) internalCallChildWorkflow(workflowName string, optio // attempt retries, arming its backoff timer, scheduling the next attempt -- only when the returned // task is polled (by Await or Select), never as a side effect of history alone: an attempt nobody // observes stays failed and retries no further. A decode error from Await(v) on a successful -// attempt is returned as-is and never triggers a retry. -func (ctx *WorkflowContext) internalScheduleTaskWithRetries(name string, initialAttempt time.Time, schedule func(taskExecutionId string, isRetry bool) *completableTask, policy RetryPolicy, retryCount int, taskExecutionId string, setTimerOrigin func(*protos.CreateTimerAction, string)) Task { +// attempt is returned as-is and never triggers a retry, and polling a chain that has already +// completed or failed (a second Await, or a Select that still includes it) is a no-op that never +// starts another chain. Both differ from the Await-driven design this replaced, and histories it +// recorded in either state do not replay. +func (ctx *WorkflowContext) internalScheduleTaskWithRetries(name string, initialAttempt time.Time, schedule func(taskExecutionId string, isRetry bool) *completableTask, policy RetryPolicy, setTimerOrigin func(*protos.CreateTimerAction, string)) Task { outer := newTask(ctx) - current := schedule(taskExecutionId, retryCount > 0) + taskExecutionId := uuid.NewString() + current := schedule(taskExecutionId, false) + retryCount := 0 var timer *completableTask + // giveUp completes outer with the current attempt's terminal state. A canceled attempt has no + // failureDetails, so it must propagate as a cancellation rather than as a nil-error success. + giveUp := func() { + if current.isCanceled { + outer.cancel() + return + } + outer.fail(current.failureDetails) + } + outer.advance = func() { if outer.isCompleted { return @@ -567,14 +582,15 @@ func (ctx *WorkflowContext) internalScheduleTaskWithRetries(name string, initial outer.complete(current.rawResult) return } + outer.taskExecutionId = taskExecutionId if retryCount+1 >= policy.MaxAttempts { - outer.fail(current.failureDetails) + giveUp() return } nextDelay := computeNextDelay(ctx.CurrentTimeUtc, policy, retryCount, initialAttempt, err) if nextDelay == 0 { - outer.fail(current.failureDetails) + giveUp() return } diff --git a/task/orchestrator_buffered_test.go b/task/orchestrator_buffered_test.go index 06ccae9f..2ec92a3e 100644 --- a/task/orchestrator_buffered_test.go +++ b/task/orchestrator_buffered_test.go @@ -119,6 +119,33 @@ func evSuspended() *protos.HistoryEvent { } } +func evTimerFired(id int32) *protos.HistoryEvent { + return &protos.HistoryEvent{ + EventId: -1, + Timestamp: timestamppb.Now(), + EventType: &protos.HistoryEvent_TimerFired{ + TimerFired: &protos.TimerFiredEvent{TimerId: id, FireAt: timestamppb.Now()}, + }, + } +} + +// evRetryTimerCreated is the TimerCreated event the backend records for a retry backoff timer +// action, as emitted by both the Await-driven retry design and the pollable one. +func evRetryTimerCreated(id int32, execID string) *protos.HistoryEvent { + return &protos.HistoryEvent{ + EventId: id, + Timestamp: timestamppb.Now(), + EventType: &protos.HistoryEvent_TimerCreated{ + TimerCreated: &protos.TimerCreatedEvent{ + FireAt: timestamppb.Now(), + Origin: &protos.TimerCreatedEvent_ActivityRetry{ + ActivityRetry: &protos.TimerOriginActivityRetry{TaskExecutionId: execID}, + }, + }, + }, + } +} + func evResumed() *protos.HistoryEvent { return &protos.HistoryEvent{ EventId: -1, @@ -207,10 +234,9 @@ func Test_RetryPolicy_TimerSequenceIdMatchesAwaitObservationPoint(t *testing.T) return nil, err } c := ctx.CallActivity("C") // id 2: scheduled before A's failure is ever observed - if err := a.Await(nil); err != nil && !errors.Is(err, ErrTaskBlocked) { - // A's retry timer is armed here, once Await actually polls A -- this call blocks - // (processNextEvent runs out of history) rather than returning an error. - } + // A's retry timer is armed here, once Await actually polls A. The call then blocks by + // panicking with ErrTaskBlocked (history has run out), so nothing after it runs. + _ = a.Await(nil) return nil, c.Await(nil) })) require.NoError(t, r.AddActivityN("A", func(ActivityContext) (any, error) { return nil, nil })) @@ -280,6 +306,130 @@ func Test_RetryPolicy_SecondAwaitDoesNotReRunChain(t *testing.T) { assert.Empty(t, cl.warns) } +// Test_RetryPolicy_TaskExecutionIdReflectsFailedAttempt pins the TaskExecutionId contract of a +// retry-configured task: it is the id recorded on the most recently observed failed attempt, and +// empty while no attempt has failed. The Await-driven design delegated to the first attempt, which +// only ever learned its id from a TaskFailed event, so this is what callers saw before retries +// became pollable. +func Test_RetryPolicy_TaskExecutionIdReflectsFailedAttempt(t *testing.T) { + policy := func(maxAttempts int) *RetryPolicy { + return &RetryPolicy{MaxAttempts: maxAttempts, InitialRetryInterval: time.Second, BackoffCoefficient: 2} + } + awaitThenReport := func(maxAttempts int) func(ctx *WorkflowContext) (any, error) { + return func(ctx *WorkflowContext) (any, error) { + a := ctx.CallActivity("A", WithActivityRetryPolicy(policy(maxAttempts))) + _ = a.Await(nil) + return a.TaskExecutionId(), nil + } + } + + for name, tc := range map[string]struct { + wf func(ctx *WorkflowContext) (any, error) + history []*protos.HistoryEvent + want string + }{ + "retried then succeeded": { + wf: awaitThenReport(3), + history: []*protos.HistoryEvent{ + evTaskScheduled(0, "A"), evTaskFailed(0, "exec-1"), evRetryTimerCreated(1, "exec-1"), + evTimerFired(1), evTaskScheduled(2, "A"), evTaskCompleted(2, `null`), + }, + want: `"exec-1"`, + }, + "retries exhausted": { + wf: awaitThenReport(2), + history: []*protos.HistoryEvent{ + evTaskScheduled(0, "A"), evTaskFailed(0, "exec-1"), evRetryTimerCreated(1, "exec-1"), + evTimerFired(1), evTaskScheduled(2, "A"), evTaskFailed(2, "exec-1"), + }, + want: `"exec-1"`, + }, + "first attempt succeeded": { + wf: awaitThenReport(3), + history: []*protos.HistoryEvent{evTaskScheduled(0, "A"), evTaskCompleted(0, `null`)}, + want: `""`, + }, + "observed through Select": { + wf: func(ctx *WorkflowContext) (any, error) { + a := ctx.CallActivity("A", WithActivityRetryPolicy(policy(3))) + b := ctx.CallActivity("B") + if i, err := ctx.Select(a, b); err != nil || i != 0 { + return nil, fmt.Errorf("expected A (index 0) to win, got %d, %v", i, err) + } + return a.TaskExecutionId(), nil + }, + history: []*protos.HistoryEvent{ + evTaskScheduled(0, "A"), evTaskScheduled(1, "B"), evTaskFailed(0, "exec-1"), + evRetryTimerCreated(2, "exec-1"), evTimerFired(2), evTaskScheduled(3, "A"), evTaskCompleted(3, `null`), + }, + want: `"exec-1"`, + }, + } { + t.Run(name, func(t *testing.T) { + r := NewTaskRegistry() + require.NoError(t, r.AddWorkflowN("wf", tc.wf)) + require.NoError(t, r.AddActivityN("A", func(ActivityContext) (any, error) { return nil, nil })) + require.NoError(t, r.AddActivityN("B", func(ActivityContext) (any, error) { return nil, nil })) + + actions, cl := runBuffered(t, r, nil, append([]*protos.HistoryEvent{evExecutionStarted("wf")}, tc.history...)) + + co := completeAction(t, actions) + require.NotNil(t, co) + require.Equal(t, protos.OrchestrationStatus_ORCHESTRATION_STATUS_COMPLETED, co.WorkflowStatus, co.GetFailureDetails().GetErrorMessage()) + assert.Equal(t, tc.want, co.GetResult().GetValue()) + assert.Empty(t, cl.warns) + }) + } +} + +// Test_RetryPolicy_CanceledAttemptPropagatesCancel guards a latent path: nothing cancels an +// activity or child task today, but a canceled attempt has nil failureDetails, so if the chain +// gave up with outer.fail(nil) the caller would see a nil-error success. The attempt is canceled +// directly on the pending task here, since no history event can do it. +func Test_RetryPolicy_CanceledAttemptPropagatesCancel(t *testing.T) { + t.Run("give up surfaces ErrTaskCanceled", func(t *testing.T) { + r := NewTaskRegistry() + require.NoError(t, r.AddWorkflowN("wf", func(ctx *WorkflowContext) (any, error) { + a := ctx.CallActivity("A", WithActivityRetryPolicy(&RetryPolicy{MaxAttempts: 1, InitialRetryInterval: time.Second})) + ctx.pendingTasks[0].cancel() + err := a.Await(nil) + return errors.Is(err, ErrTaskCanceled), nil + })) + require.NoError(t, r.AddActivityN("A", func(ActivityContext) (any, error) { return nil, nil })) + + actions, cl := runBuffered(t, r, nil, []*protos.HistoryEvent{evExecutionStarted("wf")}) + + co := completeAction(t, actions) + require.NotNil(t, co) + assert.Equal(t, protos.OrchestrationStatus_ORCHESTRATION_STATUS_COMPLETED, co.WorkflowStatus) + assert.Equal(t, `true`, co.GetResult().GetValue(), "Await must return ErrTaskCanceled, not nil") + assert.Empty(t, cl.warns) + }) + + t.Run("retries remaining consults Handle with ErrTaskCanceled", func(t *testing.T) { + r := NewTaskRegistry() + var handled error + require.NoError(t, r.AddWorkflowN("wf", func(ctx *WorkflowContext) (any, error) { + a := ctx.CallActivity("A", WithActivityRetryPolicy(&RetryPolicy{ + MaxAttempts: 3, + InitialRetryInterval: time.Second, + BackoffCoefficient: 2, + Handle: func(err error) bool { handled = err; return true }, + })) + ctx.pendingTasks[0].cancel() + return nil, a.Await(nil) + })) + require.NoError(t, r.AddActivityN("A", func(ActivityContext) (any, error) { return nil, nil })) + + actions, cl := runBuffered(t, r, nil, []*protos.HistoryEvent{evExecutionStarted("wf")}) + + assert.Nil(t, completeAction(t, actions), "the workflow must block on the backoff timer") + assert.ErrorIs(t, handled, ErrTaskCanceled) + assert.Equal(t, 1, countActions(actions, func(a *protos.WorkflowAction) bool { return a.GetCreateTimer() != nil })) + assert.Empty(t, cl.warns) + }) +} + // Test_Select_PollsEveryCandidateRegardlessOfWinner is a regression test: Select must poll every // candidate on every check, not stop at the first one found completed. A candidate Select never // polls because an earlier one already won never gets its own advance run (see diff --git a/task/retry_replay_compat_test.go b/task/retry_replay_compat_test.go new file mode 100644 index 00000000..8c389eb2 --- /dev/null +++ b/task/retry_replay_compat_test.go @@ -0,0 +1,239 @@ +/* +Copyright 2026 The Dapr Authors +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + http://www.apache.org/licenses/LICENSE-2.0 +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package task + +import ( + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "google.golang.org/protobuf/types/known/timestamppb" + "google.golang.org/protobuf/types/known/wrapperspb" + + "github.com/dapr/durabletask-go/api/protos" +) + +// Test_RetryPolicy_ReplayCompatWithAwaitDrivenHistories replays histories recorded by the +// Await-driven retry design (retries decided inside taskWrapper.Await) against the pollable retry +// chain. Each history was captured from that design by running the same workflow function and +// feeding one completion per turn; the action sequence numbers embedded in it must line up with +// what this implementation emits, or replay fails with a nondeterminism error instead of reaching +// the expected CompleteWorkflow action. This is what lets a workflow mid-retry at upgrade time +// finish under the new code. +// +// The helpers below are self-contained so this file can be dropped into a checkout of either +// design and run unchanged. +func Test_RetryPolicy_ReplayCompatWithAwaitDrivenHistories(t *testing.T) { + policy := func(maxAttempts int) *RetryPolicy { + return &RetryPolicy{MaxAttempts: maxAttempts, InitialRetryInterval: time.Second, BackoffCoefficient: 2} + } + awaitAll := func(tasks ...Task) error { + for _, tk := range tasks { + if err := tk.Await(nil); err != nil { + return err + } + } + return nil + } + + for name, tc := range map[string]struct { + wf func(ctx *WorkflowContext) (any, error) + history []*protos.HistoryEvent + completeID int32 + status protos.OrchestrationStatus + }{ + "single activity retried twice": { + wf: func(ctx *WorkflowContext) (any, error) { + return nil, ctx.CallActivity("A", WithActivityRetryPolicy(policy(5))).Await(nil) + }, + history: []*protos.HistoryEvent{ + cEvTaskScheduled(0, "A"), cEvTaskFailed(0), cEvRetryTimerCreated(1), cEvTimerFired(1), + cEvTaskScheduled(2, "A"), cEvTaskFailed(2), cEvRetryTimerCreated(3), cEvTimerFired(3), + cEvTaskScheduled(4, "A"), cEvTaskCompleted(4), + }, + completeID: 5, + status: protos.OrchestrationStatus_ORCHESTRATION_STATUS_COMPLETED, + }, + "retries exhausted": { + wf: func(ctx *WorkflowContext) (any, error) { + return nil, ctx.CallActivity("A", WithActivityRetryPolicy(policy(3))).Await(nil) + }, + history: []*protos.HistoryEvent{ + cEvTaskScheduled(0, "A"), cEvTaskFailed(0), cEvRetryTimerCreated(1), cEvTimerFired(1), + cEvTaskScheduled(2, "A"), cEvTaskFailed(2), cEvRetryTimerCreated(3), cEvTimerFired(3), + cEvTaskScheduled(4, "A"), cEvTaskFailed(4), + }, + completeID: 5, + status: protos.OrchestrationStatus_ORCHESTRATION_STATUS_FAILED, + }, + "fan-out awaited out of declaration order": { + // A fails once, B twice, C once; awaited as C, A, B. Every failure lands before + // anything is awaited, so each retry timer's id is decided by Await order alone. + wf: func(ctx *WorkflowContext) (any, error) { + a := ctx.CallActivity("A", WithActivityRetryPolicy(policy(5))) + b := ctx.CallActivity("B", WithActivityRetryPolicy(policy(5))) + c := ctx.CallActivity("C", WithActivityRetryPolicy(policy(5))) + return nil, awaitAll(c, a, b) + }, + history: []*protos.HistoryEvent{ + cEvTaskScheduled(0, "A"), cEvTaskScheduled(1, "B"), cEvTaskScheduled(2, "C"), + cEvTaskFailed(0), cEvTaskFailed(1), cEvTaskFailed(2), + cEvRetryTimerCreated(3), cEvTimerFired(3), cEvTaskScheduled(4, "C"), cEvTaskCompleted(4), + cEvRetryTimerCreated(5), cEvTimerFired(5), cEvTaskScheduled(6, "A"), cEvTaskCompleted(6), + cEvRetryTimerCreated(7), cEvTimerFired(7), cEvTaskScheduled(8, "B"), cEvTaskFailed(8), + cEvRetryTimerCreated(9), cEvTimerFired(9), cEvTaskScheduled(10, "B"), cEvTaskCompleted(10), + }, + completeID: 11, + status: protos.OrchestrationStatus_ORCHESTRATION_STATUS_COMPLETED, + }, + "failure lands before a plain task is awaited": { + // A fails while the workflow is awaiting B; C is scheduled before A's failure is + // observed, so C takes id 2 and A's first retry timer id 3. + wf: func(ctx *WorkflowContext) (any, error) { + a := ctx.CallActivity("A", WithActivityRetryPolicy(policy(5))) + b := ctx.CallActivity("B") + if err := b.Await(nil); err != nil { + return nil, err + } + c := ctx.CallActivity("C") + return nil, awaitAll(a, c) + }, + history: []*protos.HistoryEvent{ + cEvTaskScheduled(0, "A"), cEvTaskScheduled(1, "B"), cEvTaskFailed(0), cEvTaskCompleted(1), + cEvTaskScheduled(2, "C"), cEvRetryTimerCreated(3), cEvTaskCompleted(2), cEvTimerFired(3), + cEvTaskScheduled(4, "A"), cEvTaskFailed(4), cEvRetryTimerCreated(5), cEvTimerFired(5), + cEvTaskScheduled(6, "A"), cEvTaskCompleted(6), + }, + completeID: 7, + status: protos.OrchestrationStatus_ORCHESTRATION_STATUS_COMPLETED, + }, + "retry interleaved with a plain timer": { + wf: func(ctx *WorkflowContext) (any, error) { + a := ctx.CallActivity("A", WithActivityRetryPolicy(policy(5))) + if err := ctx.CreateTimer(time.Second).Await(nil); err != nil { + return nil, err + } + b := ctx.CallActivity("B") + return nil, awaitAll(a, b) + }, + history: []*protos.HistoryEvent{ + cEvTaskScheduled(0, "A"), cEvPlainTimerCreated(1), cEvTaskFailed(0), cEvTimerFired(1), + cEvTaskScheduled(2, "B"), cEvRetryTimerCreated(3), cEvTaskCompleted(2), cEvTimerFired(3), + cEvTaskScheduled(4, "A"), cEvTaskFailed(4), cEvRetryTimerCreated(5), cEvTimerFired(5), + cEvTaskScheduled(6, "A"), cEvTaskCompleted(6), + }, + completeID: 7, + status: protos.OrchestrationStatus_ORCHESTRATION_STATUS_COMPLETED, + }, + "child workflow retried twice": { + wf: func(ctx *WorkflowContext) (any, error) { + return nil, ctx.CallChildWorkflow("Child", WithChildWorkflowRetryPolicy(policy(4))).Await(nil) + }, + history: []*protos.HistoryEvent{ + cEvChildCreated(0, "Child"), cEvChildFailed(0), cEvChildRetryTimerCreated(1), cEvTimerFired(1), + cEvChildCreated(2, "Child"), cEvChildFailed(2), cEvChildRetryTimerCreated(3), cEvTimerFired(3), + cEvChildCreated(4, "Child"), cEvChildCompleted(4), + }, + completeID: 5, + status: protos.OrchestrationStatus_ORCHESTRATION_STATUS_COMPLETED, + }, + } { + t.Run(name, func(t *testing.T) { + r := NewTaskRegistry() + require.NoError(t, r.AddWorkflowN("wf", tc.wf)) + for _, a := range []string{"A", "B", "C"} { + require.NoError(t, r.AddActivityN(a, func(ActivityContext) (any, error) { return nil, nil })) + } + require.NoError(t, r.AddWorkflowN("Child", func(*WorkflowContext) (any, error) { return nil, nil })) + + ctx := NewWorkflowContext(r, "compat", append([]*protos.HistoryEvent{cEvExecutionStarted("wf")}, tc.history...), nil) + actions := ctx.start() + + require.Len(t, actions, 1, "a fully recorded history must leave only the completion to emit, got %v", actions) + co := actions[0].GetCompleteWorkflow() + require.NotNil(t, co, "expected CompleteWorkflow, got %v", actions[0]) + assert.Equal(t, tc.completeID, actions[0].Id) + assert.Equal(t, tc.status, co.WorkflowStatus, co.GetFailureDetails().GetErrorMessage()) + if tc.status == protos.OrchestrationStatus_ORCHESTRATION_STATUS_FAILED { + assert.NotContains(t, co.GetFailureDetails().GetErrorMessage(), "a previous execution called") + } + }) + } +} + +func cEvExecutionStarted(name string) *protos.HistoryEvent { + return &protos.HistoryEvent{EventId: -1, Timestamp: timestamppb.Now(), EventType: &protos.HistoryEvent_ExecutionStarted{ + ExecutionStarted: &protos.ExecutionStartedEvent{Name: name, WorkflowInstance: &protos.WorkflowInstance{InstanceId: "compat"}}, + }} +} + +func cEvTaskScheduled(id int32, name string) *protos.HistoryEvent { + return &protos.HistoryEvent{EventId: id, Timestamp: timestamppb.Now(), EventType: &protos.HistoryEvent_TaskScheduled{ + TaskScheduled: &protos.TaskScheduledEvent{Name: name}, + }} +} + +func cEvTaskCompleted(id int32) *protos.HistoryEvent { + return &protos.HistoryEvent{EventId: -1, Timestamp: timestamppb.Now(), EventType: &protos.HistoryEvent_TaskCompleted{ + TaskCompleted: &protos.TaskCompletedEvent{TaskScheduledId: id, Result: wrapperspb.String(`null`)}, + }} +} + +func cEvTaskFailed(id int32) *protos.HistoryEvent { + return &protos.HistoryEvent{EventId: -1, Timestamp: timestamppb.Now(), EventType: &protos.HistoryEvent_TaskFailed{ + TaskFailed: &protos.TaskFailedEvent{TaskScheduledId: id, TaskExecutionId: "exec", FailureDetails: &protos.TaskFailureDetails{ErrorType: "TestError", ErrorMessage: "injected"}}, + }} +} + +func cEvTimerFired(id int32) *protos.HistoryEvent { + return &protos.HistoryEvent{EventId: -1, Timestamp: timestamppb.Now(), EventType: &protos.HistoryEvent_TimerFired{ + TimerFired: &protos.TimerFiredEvent{TimerId: id, FireAt: timestamppb.Now()}, + }} +} + +func cEvTimerCreated(id int32, tc *protos.TimerCreatedEvent) *protos.HistoryEvent { + tc.FireAt = timestamppb.Now() + return &protos.HistoryEvent{EventId: id, Timestamp: timestamppb.Now(), EventType: &protos.HistoryEvent_TimerCreated{TimerCreated: tc}} +} + +func cEvPlainTimerCreated(id int32) *protos.HistoryEvent { + return cEvTimerCreated(id, &protos.TimerCreatedEvent{Origin: &protos.TimerCreatedEvent_CreateTimer{CreateTimer: &protos.TimerOriginCreateTimer{}}}) +} + +func cEvRetryTimerCreated(id int32) *protos.HistoryEvent { + return cEvTimerCreated(id, &protos.TimerCreatedEvent{Origin: &protos.TimerCreatedEvent_ActivityRetry{ActivityRetry: &protos.TimerOriginActivityRetry{TaskExecutionId: "exec"}}}) +} + +func cEvChildRetryTimerCreated(id int32) *protos.HistoryEvent { + return cEvTimerCreated(id, &protos.TimerCreatedEvent{Origin: &protos.TimerCreatedEvent_ChildWorkflowRetry{ChildWorkflowRetry: &protos.TimerOriginChildWorkflowRetry{}}}) +} + +func cEvChildCreated(id int32, name string) *protos.HistoryEvent { + return &protos.HistoryEvent{EventId: id, Timestamp: timestamppb.Now(), EventType: &protos.HistoryEvent_ChildWorkflowInstanceCreated{ + ChildWorkflowInstanceCreated: &protos.ChildWorkflowInstanceCreatedEvent{Name: name}, + }} +} + +func cEvChildCompleted(id int32) *protos.HistoryEvent { + return &protos.HistoryEvent{EventId: -1, Timestamp: timestamppb.Now(), EventType: &protos.HistoryEvent_ChildWorkflowInstanceCompleted{ + ChildWorkflowInstanceCompleted: &protos.ChildWorkflowInstanceCompletedEvent{TaskScheduledId: id, Result: wrapperspb.String(`null`)}, + }} +} + +func cEvChildFailed(id int32) *protos.HistoryEvent { + return &protos.HistoryEvent{EventId: -1, Timestamp: timestamppb.Now(), EventType: &protos.HistoryEvent_ChildWorkflowInstanceFailed{ + ChildWorkflowInstanceFailed: &protos.ChildWorkflowInstanceFailedEvent{TaskScheduledId: id, FailureDetails: &protos.TaskFailureDetails{ErrorType: "TestError", ErrorMessage: "injected"}}, + }} +} diff --git a/workflow/workflow.go b/workflow/workflow.go index c442ae79..7a25d92d 100644 --- a/workflow/workflow.go +++ b/workflow/workflow.go @@ -77,9 +77,25 @@ func (w *WorkflowContext) CreateTimer(delay time.Duration, opts ...CreateTimerOp // WaitForExternalEvent creates a task that is completed only after an event // named [eventName] is received by this workflow or when the specified timeout -// expires. See [task.WorkflowContext.WaitForSingleEvent] for the full behavior, -// including how a losing [WorkflowContext.Select] candidate must be carried -// forward rather than discarded. Note that event names are case-insensitive. +// expires. +// +// The [timeout] parameter can be used to define a timeout for receiving the +// event. If the timeout expires before the named event is received, the task +// will be completed and will return a timeout error value [ErrTaskCanceled] +// when awaited. Otherwise, the awaited task will return the deserialized +// payload of the received event. A Duration value of zero returns a canceled +// task if the event isn't already available in the history. Use a negative +// Duration to wait indefinitely for the event to be received. +// +// Workflows can wait for the same event name multiple times, so waiting for +// multiple events with the same name is allowed. Each event received by an +// workflow will complete just one task returned by this method. +// +// A task returned by this method that loses a [WorkflowContext.Select] stays +// queued for its event name and must be carried forward into the next Select +// rather than discarded; see [task.WorkflowContext.WaitForSingleEvent] for why. +// +// Note that event names are case-insensitive. func (w *WorkflowContext) WaitForExternalEvent(eventName string, timeout time.Duration) Task { return w.oc.WaitForSingleEvent(eventName, timeout) } From 08a76c3adb4a5967087d86a6375b903c3f64eccd Mon Sep 17 00:00:00 2001 From: gomitrah Date: Thu, 8 Oct 2026 16:41:58 +0530 Subject: [PATCH 12/12] Merge branch 'pr131-review-fixes' of JoshVanL/durabletask-go into feature/10447-workflow-select JoshVanL found and fixed three bugs in the retry chain: TaskExecutionId() always returning empty on a retry-configured task, a canceled attempt surfacing as a silent nil-error success instead of ErrTaskCanceled, and two always-constant parameters on internalScheduleTaskWithRetries. Also closes a gap in the cancellation fix found during review: a canceled attempt with retries remaining was clearing the chain's carried execution id instead of preserving it for the next attempt. Signed-off-by: gomitrah --- task/orchestrator.go | 9 +++++++-- task/orchestrator_buffered_test.go | 8 ++++++++ 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/task/orchestrator.go b/task/orchestrator.go index 0dcddcb6..9757223e 100644 --- a/task/orchestrator.go +++ b/task/orchestrator.go @@ -574,8 +574,13 @@ func (ctx *WorkflowContext) internalScheduleTaskWithRetries(name string, initial // TaskExecutionId reflects what's recorded on this attempt's TaskScheduled/ // TaskCompleted/TaskFailed history events, which is what must be threaded into the - // next attempt and its backoff timer's origin so replay stays deterministic. - taskExecutionId = current.TaskExecutionId() + // next attempt and its backoff timer's origin so replay stays deterministic. A + // canceled attempt has no execution id of its own (nothing cancels activity/child + // tasks today, so this is currently unreachable, but cancellation must not erase the + // chain's carried id for whichever future attempt comes next). + if id := current.TaskExecutionId(); id != "" { + taskExecutionId = id + } err := current.completionError() if err == nil { diff --git a/task/orchestrator_buffered_test.go b/task/orchestrator_buffered_test.go index 2ec92a3e..f0d7d9c4 100644 --- a/task/orchestrator_buffered_test.go +++ b/task/orchestrator_buffered_test.go @@ -425,7 +425,15 @@ func Test_RetryPolicy_CanceledAttemptPropagatesCancel(t *testing.T) { assert.Nil(t, completeAction(t, actions), "the workflow must block on the backoff timer") assert.ErrorIs(t, handled, ErrTaskCanceled) + var timerExecID string + for _, act := range actions { + if ct := act.GetCreateTimer(); ct != nil { + timerExecID = ct.GetActivityRetry().GetTaskExecutionId() + } + } assert.Equal(t, 1, countActions(actions, func(a *protos.WorkflowAction) bool { return a.GetCreateTimer() != nil })) + assert.NotEmpty(t, timerExecID, + "a canceled attempt has no execution id of its own; the retry timer must still carry the chain's real id, not overwrite it with an empty one") assert.Empty(t, cl.warns) }) }