activity-parity: allow SAA to be manually completed by ID

[spkane31]

What changed?

RespondActivityTaskCompletedById can now force-complete an SAA from the Scheduled state.

Why?

Workflow activities allow force completing an activity before any worker starts it.

How did you test it?

• built
• run locally and tested manually
• covered by existing tests
• added new unit test(s)
• added new functional test(s)
  • Added a new file tests/activity_parity_test.go to hold all tests related to parity of workflow activities and standalone activities.

Potential risks

NA

[dandavison]

The PR looks great, my only question is I don't think there's any test coverage for this?

[dandavison]

TestStandaloneActivityTestSuite and TestActivityParityTestSuite seem to pass without it.

[spkane31]

Added an assertion at the end of the activity_parity_test.go file

[fretz12]

Since we're allowing completion from scheduled, we should not emit metrics.ActivityStartToCloseLatency if it's from scheduled. I belive WFA guards against it too. Probably need to pass that status into this helper.

[dandavison]

Not blocking: this function's bothering me. A function named "validateTaskToken" shouldn't need to know anything about "force-completing from scheduled state". It should just validate a task token. The problem is that it's mixing state transition logic with validation logic ("if the activity's not in the right state then I don't care whether your token is valid -- I'm just going to say it's invalid"). Not urgent - we can fix it whenever we are clear what a better arrangement is.

[dandavison]

We're forgetting PAUSED. Looks like WFA allows it. Let's add full parity tests for Complete/Fail/Cancel in multiple relevant states (at least SCHEDULED and PAUSED)

[dandavison]

This should use hasAttemptInProgress() -- it makes the semantics clear and it fixes the pause disparity.

How about applying this change. It uses hasAttemptInProgress and avoids the ad-hoc status check in the metrics emission function:

diff --git a/chasm/lib/activity/activity.go b/chasm/lib/activity/activity.go
index f9037a3481..beec3f2807 100644
--- a/chasm/lib/activity/activity.go
+++ b/chasm/lib/activity/activity.go
@@ -1994,11 +1994,11 @@ func (a *Activity) emitOnAttemptFailedMetrics(ctx chasm.Context, handler metrics
 	metrics.ActivityTaskFail.With(handler).Record(1)
 }
 
-func (a *Activity) emitOnCompletedMetrics(ctx chasm.Context, handler metrics.Handler) {
+func (a *Activity) emitOnCompletedMetrics(ctx chasm.Context, handler metrics.Handler, attemptWasStarted bool) {
 	attempt := a.LastAttempt.Get(ctx)
 	startedTime := attempt.GetStartedTime().AsTime()
 
-	if a.GetStatus() != activitypb.ACTIVITY_EXECUTION_STATUS_SCHEDULED {
+	if attemptWasStarted {
 		startToCloseLatency := time.Since(startedTime)
 		metrics.ActivityStartToCloseLatency.With(handler).Record(startToCloseLatency)
 	}
diff --git a/chasm/lib/activity/statemachine.go b/chasm/lib/activity/statemachine.go
index af64f86f3c..c8007d3787 100644
--- a/chasm/lib/activity/statemachine.go
+++ b/chasm/lib/activity/statemachine.go
@@ -199,8 +199,11 @@ func(a *Activity, ctx chasm.MutableContext, event completeEvent) error {
 		return a.StoreOrSelf(ctx).RecordCompleted(ctx, func(ctx chasm.MutableContext) error {
 			req := event.req.GetCompleteRequest()
 
+			attemptWasStarted := a.hasAttemptInProgress()
 			attempt := a.LastAttempt.Get(ctx)
-			if a.GetStatus() == activitypb.ACTIVITY_EXECUTION_STATUS_SCHEDULED {
+			if !attemptWasStarted {
+				// RespondActivityTaskCompletedById can complete an activity when no attempt is in
+				// progress.
 				attempt.StartedTime = timestamppb.New(ctx.Now(a))
 				if a.FirstAttemptStartedTime == nil {
 					a.FirstAttemptStartedTime = attempt.StartedTime
@@ -215,7 +218,7 @@ func(a *Activity, ctx chasm.MutableContext, event completeEvent) error {
 				},
 			}
 
-			a.emitOnCompletedMetrics(ctx, event.metricsHandler)
+			a.emitOnCompletedMetrics(ctx, event.metricsHandler, attemptWasStarted)
 
 			return nil
 		})

[dandavison]

It would be ideal if we could avoid having these kind of special-case state conditions scattered through the code. They're a source of bugs: e.g. here what about PAUSED.... I made a suggestion below (please see the diff below)