feat: activity override and cancellation #101

thantos · 2022-12-13T04:43:49Z

closes #97

~~Cancel~~, Complete, or Fail any activity from event handler, activity, api, or workflow using it's activity token.
Cancel, ~~Complete, or Fail~~ an activity reference the workflow
Activity can check to see if it is "closed" by using the heartbeat call, which will return {closed: true} when the activity or the workflow is considered resolved/finished/closed.
Fixed a bug that would cause only an error's name to be output.

cfraz89 · 2022-12-13T05:17:55Z

apps/tests/aws-runtime/test/test-service.ts

@@ -332,3 +336,130 @@ const sendFinishEvent = activity("sendFinish", async (executionId: string) => {
    proxy: true,
  });
 });
+
+const activityOverrideEvent = event<{


I think its a good time to add comments about what each of the elements in this service are, and what they're used for in the tests. Its starting to become a lot to piece together.

cfraz89 · 2022-12-13T05:28:59Z

I'm a bit skeptical that it's a good idea to be able to override an activity's result, beyond cancellation. Wouldn't this make activities a lot less 'does what it says on the tin'? With this I'm seeing them as akin to functions that could return anything based on what's intercepting them, and so are harder to reason about. What would the use cases for this be?

thantos · 2022-12-13T05:59:50Z

The behavior is the same mechanism as "async activities".

I'm certain we need the ability to cancel, which is essentially the same as failing.

Which led to the current state.

Developers can do whatever they want, but in most cases they will only be able to allow others to override when the activity token is distributed, which is up to the activity.

cfraz89 · 2022-12-13T09:25:38Z

The behavior is the same mechanism as "async activities".

I'm certain we need the ability to cancel, which is essentially the same as failing.

Which led to the current state.

Developers can do whatever they want, but in most cases they will only be able to allow others to override when the activity token is distributed, which is up to the activity.

Ok makes sense

sam-goodwin · 2022-12-13T12:44:36Z

apps/tests/aws-runtime/test/test-service.ts

+    let n = 0;
+    while (n < 10) {
+      await delay(1);
+      const { closed } = await heartbeat();


does heartbeat throw if it's been cancelled?

No, the activity chooses what to do with this information, it can keep running if the developer wants. (ex: fire and forget activities).

sam-goodwin · 2022-12-13T12:48:49Z

packages/@eventual/core/src/activity.ts

+   * If the activity is calling {@link heartbeat}, closed: true will be
+   * return to signal the workflow considers the activity finished.


Ah ok, so heartbeat doesn't throw but it does return information that the workflow can use to adjust its behavior? I worry a bit a bout the ergonomics of that - seems like i'll be continually checking the result of heartbeat. Would throwing an exception be better, like an interruption.

Are you talking about throwing an error when heartbeat is called or by throwing an error somewhere into their activity?

It is not possible to just throw an error into a running process without some hook. The pattern I see is to use heartbeat to inject this information. An activity that is sensitive to cancellation will use the heartbeat to understand when it can cancel.

Otherwise we could add a poller in the activity worker to pull the status at additional overhead, but we'd still need a hook into the activity code that actually throws.

It is a valid use case to not care about cancelation and just keep running, so we wouldn't want to just kill the process.

Temporal: returns cancelled as data on heartbeat.
Step Functions: HeartBeat API throws TaskTimedOut error when the task has been timed out.

I think the developer using heartbeat is fine, the operation is inexpensive (single dynamo update) and gives them control over the experience.

I mean the heartbeat call wouod throw instead of return closed: true. Effectively the same but doesn't require checking a boolean. I'm not sure what's better

sam-goodwin · 2022-12-13T12:49:07Z

packages/@eventual/core/src/activity.ts

+   */
+
+  fail: (


nit: run prettier, code docs are not tight

Looks like prettier doesn't catch this, it had been run on this document.

Looks like prettier accepts 0 or 1 return here, if I add 10 returns it updates to 1 return. And it is fine with 0 returns (and putting the function on the same line as the comment).

sam-goodwin · 2022-12-13T12:49:43Z

packages/@eventual/core/src/activity.ts

+  /**
+   * Causes the activity to resolve the provided value to the workflow.
+   *
+   * If the activity is calling {@link heartbeat}, closed: true will be
+   * return to signal the workflow considers the activity finished.
+   */
+  complete: (result: T) => Promise<void>;


Why do we need this, this seems like a step beyond cancellation. What is the requirement driving this feature?

It is the same as completeActivity, just from the caller workflow.

Added it for the symmetry. I don't have a strong use case for it, other than it will just work.

sam-goodwin · 2022-12-13T12:51:42Z

packages/@eventual/core/src/calls/activity-call.ts

+  call.complete = function (result) {
+    return createOverrideActivityCall(
+      { type: ActivityTargetType.OwnActivity, seq: this.seq! },
+      Result.resolved(result)
+    ) as unknown as Promise<void>;
+  };


We're going to have a conflict here, I just added this as the way to complete a token

export interface ActivityFunction< Arguments extends any[], Output extends any = any > { (...args: Arguments): Promise<Awaited<UnwrapAsync<Output>>>; complete(request: CompleteActivityRequest): Promise<void>; }

I don't think there should be a way to override the result of an activity, we don't have any requirements for that. Let's slow down please.

Let's move on to examples, i think we are now building way too much.

Wait, what is the difference between what you have and I have?

sam-goodwin · 2022-12-13T12:52:48Z

packages/@eventual/core/src/interpret.ts

+          } else if (isOverrideActivityCall(activity)) {
+            if (activity.target.type === ActivityTargetType.OwnActivity) {
+              const act = callTable[activity.target.seq];
+              if (act === undefined) {
+                throw new DeterminismError(
+                  `Call for seq ${activity.target.seq} was not emitted.`
+                );
+              }
+              if (!act.result) {
+                act.result = activity.outcome;
+              }
+            }


Why do we need this? This seems like a way over the top opinion without any requirement driving it.

Why have a round trip to cancel/fail an activity if we don't need it? We can send a event back to the workflow queue, but it will have the same effect with a delay.

sam-goodwin · 2022-12-13T12:54:10Z

The behavior is the same mechanism as "async activities".

I'm certain we need the ability to cancel, which is essentially the same as failing.

Which led to the current state.

Developers can do whatever they want, but in most cases they will only be able to allow others to override when the activity token is distributed, which is up to the activity.

I agree with Chris, I don't think we should be adding the override functionality. Just because we can doesn't mean we should. Let requirements drive features. Please remove it. We only need the ability to complete an activity by token, not arbitrarily set the value.

thantos · 2022-12-13T16:45:48Z

The behavior is the same mechanism as "async activities".
I'm certain we need the ability to cancel, which is essentially the same as failing.
Which led to the current state.
Developers can do whatever they want, but in most cases they will only be able to allow others to override when the activity token is distributed, which is up to the activity.

I agree with Chris, I don't think we should be adding the override functionality. Just because we can doesn't mean we should. Let requirements drive features. Please remove it. We only need the ability to complete an activity by token, not arbitrarily set the value.

There is no difference between complete activity and this functionality. Override cannot override the output of a completed/failed activity, but it can complete an uncompleted activity.

Is the word override confusing? I had called it "finish", but that was also confusing with "complete".

sam-goodwin · 2022-12-13T17:05:08Z

In my slack pr I implemented a complete api. Is it different than that?

thantos · 2022-12-13T17:16:47Z

In my slack pr I implemented a complete api. Is it different than that?

Ok, you had implemented completeActivity from an activity function. This is the same as calling my completeActivity intrinsic from anywhere except from within a workflow.

Issues with the current state in main:

Missing the matching failActivity API
Calling activity.complete can actually complete any activity and it is not possible to know what activity a token is for from the token itself. So I can see the value in that it is typesafe, just doesn't do anything at runtime.
activity.complete is not safe to be called from within a workflow. It makes a network call and is not durable.

In this PR, I updated activity.complete to be safe within a workflow (durable command).

thantos · 2022-12-13T17:17:03Z

Ok, made changes based on feedback:

removed the ability to complete or fail an activity by reference (const act = myAct(); act.complete(value);)
2. the ability to cancel an activity remains (const act = myAct(); act.cancel(reason);)
removed the ability to "cancel" an activity from outside of the workflow that started it. (cancelActivity(activityToken))
3. the ability to complete or fail remain (completeActivity(activityToken, value))
Updated the activity.complete to work within a workflow. (const myAct = activity(...); workflow("", (token) => myAct.complete(token, value)))

sam-goodwin · 2022-12-13T17:19:12Z

I still don't understand what the use case is for this outside of just reporting success/failure for an async activity. Why does a workflow need to be able to cancel an activity? I worry we are adding too much breadth prematurely.

thantos · 2022-12-13T17:22:57Z

Now supported

const myAct = activity("myAct", () => { ... });

// in an event handle or API
someEvent.on(async (token) => {
    // complete any activity
    await myAct.complete(token, value);
    // complete or fail any activity
    await completeActivity(token, value);
    await failActivity(token, error, message);
})

workflow("", async () => {
    // complete any activity
    await myAct.complete(token, value);
    // complete or fail any activity
    await completeActivity(token, value);
    await failActivity(token, error, message);

    const actInstance = myAct();
    // cancel/fail an activity from it's calling workflow.
    await actInstance.cancel();
    await actInstance // throws ActivityCancelled;
})

// some other lambda code
handler = (token) => {
    await workflowClient.completeActivity(token);
    await workflowClient.failActivity(token);
}

thantos added 5 commits December 12, 2022 20:23

support complete and fail activity

8d60e3b

change finish to override

0871669

override test

073d789

tests pass

bffc121

docs

b7ce508

thantos requested review from sam-goodwin and cfraz89 December 13, 2022 04:43

fixed tests for new errors

046ff4f

cfraz89 reviewed Dec 13, 2022

View reviewed changes

thantos added 4 commits December 13, 2022 00:41

remove return

d879d04

Merge branch 'main' into sussman/activity_override

555df09

support activity.complete from within a workflow

398c653

remove complete and fail activity from workflow.

d8a5175

sam-goodwin requested changes Dec 13, 2022

View reviewed changes

thantos mentioned this pull request Dec 20, 2022

Bug: missing service type handling in some intrinsics #124

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: activity override and cancellation #101

feat: activity override and cancellation #101

thantos commented Dec 13, 2022 •

edited

Loading

cfraz89 Dec 13, 2022

cfraz89 commented Dec 13, 2022

thantos commented Dec 13, 2022

cfraz89 commented Dec 13, 2022

sam-goodwin Dec 13, 2022

thantos Dec 13, 2022

sam-goodwin Dec 13, 2022

thantos Dec 13, 2022

sam-goodwin Dec 13, 2022

sam-goodwin Dec 13, 2022

thantos Dec 13, 2022

thantos Dec 13, 2022

sam-goodwin Dec 13, 2022

thantos Dec 13, 2022

sam-goodwin Dec 13, 2022

sam-goodwin Dec 13, 2022

thantos Dec 13, 2022

sam-goodwin Dec 13, 2022

thantos Dec 13, 2022

sam-goodwin commented Dec 13, 2022

thantos commented Dec 13, 2022

sam-goodwin commented Dec 13, 2022

thantos commented Dec 13, 2022

thantos commented Dec 13, 2022

sam-goodwin commented Dec 13, 2022

thantos commented Dec 13, 2022

		* If the activity is calling {@link heartbeat}, closed: true will be
		* return to signal the workflow considers the activity finished.

feat: activity override and cancellation #101

Are you sure you want to change the base?

feat: activity override and cancellation #101

Conversation

thantos commented Dec 13, 2022 • edited Loading

Choose a reason for hiding this comment

cfraz89 commented Dec 13, 2022

thantos commented Dec 13, 2022

cfraz89 commented Dec 13, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sam-goodwin commented Dec 13, 2022

thantos commented Dec 13, 2022

sam-goodwin commented Dec 13, 2022

thantos commented Dec 13, 2022

thantos commented Dec 13, 2022

sam-goodwin commented Dec 13, 2022

thantos commented Dec 13, 2022

thantos commented Dec 13, 2022 •

edited

Loading