Skip to content
Draft
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions tester/tester.go
Original file line number Diff line number Diff line change
Expand Up @@ -376,7 +376,15 @@ func (wt *workflowTester[TResult]) Execute(ctx context.Context, args ...any) {

result, err := e.ExecuteTask(ctx, t)
if err != nil {
panic("Error while executing workflow" + err.Error())
// Set workflow error and mark as finished
wt.logger.Debug("ExecuteTask returned error", "error", err.Error())
if !tw.instance.SubWorkflow() {
wt.workflowFinished = true
wt.workflowErr = workflowerrors.FromError(err)
wt.logger.Debug("Set workflow error", "error", wt.workflowErr)
}
e.Close()
continue
}

e.Close()
Expand Down Expand Up @@ -724,7 +732,7 @@ func (wt *workflowTester[TResult]) scheduleActivity(wfi *core.WorkflowInstance,
}

wt.callbacks <- func() *history.WorkflowEvent {
var ne *history.Event
var ne *history.Event

if activityErr != nil {
aerr := workflowerrors.FromError(activityErr)
Expand Down
81 changes: 81 additions & 0 deletions tester/tester_pending_futures_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
package tester

import (
"context"
"testing"
"time"

wf "github.com/cschleiden/go-workflows/workflow"
"github.com/stretchr/testify/require"
)

// Test that timer futures are properly detected as pending when workflow completes without waiting
func TestPendingTimerFutures(t *testing.T) {
wft := NewWorkflowTester[any](workflowWithPendingTimer)

// This should panic due to pending timer future
require.Panics(t, func() {
wft.Execute(context.Background())
}, "Expected panic about pending timer futures")
}

func workflowWithPendingTimer(ctx wf.Context) error {
// Schedule a timer but don't wait for it
wf.ScheduleTimer(ctx, 10*time.Second)
return nil // BUG: Returns without waiting for timer
}

// This test demonstrates the CORRECT behavior: activities automatically block workflow completion
func TestActivitiesAutomaticallyBlockWorkflowCompletion(t *testing.T) {
wft := NewWorkflowTester[any](workflowWithScheduledActivity)
wft.Registry().RegisterActivity(testActivity)

// Activities automatically block workflow completion - this is the correct behavior
// The workflow will wait for the activity to complete before finishing
wft.Execute(context.Background())
require.True(t, wft.WorkflowFinished())

result, err := wft.WorkflowResult()
require.NoError(t, err)
require.Nil(t, result)
}

func workflowWithScheduledActivity(ctx wf.Context) error {
// Schedule activity but don't explicitly wait for it
// The workflow framework automatically waits for activities to complete
wf.ExecuteActivity[string](ctx, wf.DefaultActivityOptions, testActivity)
return nil // This returns after the activity completes (automatic blocking)
}

func workflowWithPendingActivity(ctx wf.Context) (string, error) {
// Schedule activity but don't explicitly wait for it
wf.ExecuteActivity[string](ctx, wf.DefaultActivityOptions, testActivity)

// Even though we don't call future.Get(), the workflow framework
// automatically waits for the activity to complete
return "should-not-be-returned", nil
}

func testActivity(ctx context.Context) (string, error) {
return "activity-result", nil
}

// Test that workflow properly waits for activities when explicitly using Get()
func TestWorkflowExplicitlyWaitsForActivity(t *testing.T) {
wft := NewWorkflowTester[string](workflowExplicitlyWaiting)
wft.Registry().RegisterActivity(testActivity)

wft.Execute(context.Background())
require.True(t, wft.WorkflowFinished())

result, err := wft.WorkflowResult()
require.NoError(t, err)
require.Equal(t, "activity-result", result)
}

func workflowExplicitlyWaiting(ctx wf.Context) (string, error) {
// Schedule activity and explicitly wait for it
future := wf.ExecuteActivity[string](ctx, wf.DefaultActivityOptions, testActivity)
result, err := future.Get(ctx)
return result, err
}
22 changes: 22 additions & 0 deletions workflow/executor/executor.go
Original file line number Diff line number Diff line change
Expand Up @@ -283,9 +283,31 @@ func (e *executor) executeNewEvents(newEvents []*history.Event) ([]*history.Even
e.workflowState.SetReplaying(false)

for i, event := range newEvents {
wasCompleted := e.workflow != nil && e.workflow.Completed()

if err := e.executeEvent(event); err != nil {
return newEvents[:i], err
}

// Check if workflow just completed during this event processing
if e.workflow != nil && !wasCompleted && e.workflow.Completed() {
if e.workflowState.HasPendingFutures() {
// This should not happen, provide debug information to the developer
var pending []string
pf := e.workflowState.PendingFutureNames()
for id, name := range pf {
pending = append(pending, fmt.Sprintf("%d-%s", id, name))
}
slices.Sort(pending)

if testing.Testing() {
panic(fmt.Sprintf("workflow completed, but there are still pending futures: %s", pending))
}

return newEvents[:i+1], tracing.WithSpanError(
e.workflowSpan, fmt.Errorf("workflow completed, but there are still pending futures: %s", pending))
}
}

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot we're already checking for .Completed in line 313. Why do we have to do this again here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're absolutely right. The duplicate check was redundant and unnecessary. I've removed the duplicate check and kept only the original check at line 313. The issue I was trying to solve requires a different approach - the timing of when pending futures are detected for activities in the tester vs real runtime.

}

if e.workflow.Completed() {
Expand Down