Activity reset #1730

THardy98 · 2025-06-19T11:50:23Z

What was changed

Added support to reset an activity. Cancellations due to reset are received through the heartbeat mechanism of the activity.

Why?

Part of activity operator work.

Closes [typescript] SDK support for activity reset #1693
How was this tested:
Integration test
Any docs updates needed?
Likely

THardy98 · 2025-06-19T11:52:41Z

Blocked on the merge of:
#1729

and server release that populates activity_reset in heartbeat response

cretz · 2025-08-19T19:57:50Z

packages/test/src/helpers-integration.ts

    await handle.client.workflowService.unpauseActivity(req);
+  } else {
+    const resetReq = { ...req, resetHeartbeat: true };
+    await handle.client.workflowService.resetActivity(resetReq);


I am looking into doing this in other langs too, can you confirm the current dev server supports this and/or do we need to upgrade to an RC one for tests?

As in supports the resetActivity call? Yes should be fine. Latest CLI version is 1.4.1, which has temporal server version 1.28.0. ResetActivity was included in 1.27.1 with some improvements in 1.28.0

Edit: will need to set frontend.activityAPIsEnabled to true to enable this API

mjameswh · 2025-08-26T20:05:10Z

packages/client/src/async-completion-client.ts

      }
    } catch (err) {
      this.handleError(err);
    }
    if (cancelRequested) {
      throw new ActivityCancelledError('cancelled');
    }
+    if (reset) {


Should we be worried about this cascade of if statements? Can multiple of these conditions be true at the same time? If so, are we 100% sure about the order?

I believe it's possible for multiple of these conditions can be true at the same time - user issues an activity cancel and a reset command between heartbeats (not sure if server has some logic that guards against this).

In any case, the ordering of these statements follows that of core:
https://github.com/temporalio/sdk-core/blob/d34c1d6d393462a816baf2469c256a21ffbaf196/core/src/worker/activities/activity_heartbeat_manager.rs#L148-L159

Well, though more than one may be true, the way this is written where each branch throws, that's kind of irrelevant. I would make them elses for that reason I think.

mjameswh · 2025-08-26T20:08:20Z

packages/test/src/helpers-integration.ts

-    //  • the activity vanished (already completed)
-    return activityInfo ? !activityInfo.paused : true;
+    if (state === 'pause') {
+      return activityInfo ? activityInfo.paused ?? false : true;


Not new code, but pretty hard on reader.

mjameswh · 2025-08-26T20:09:57Z

packages/worker/src/activity.ts

-            // If cancel due to activity pause, emit an application failure for the pause.
-            if (this.context.cancellationDetails?.paused) {
+            // If cancel due to activity pause or reset, emit an application failure.
+            if (this.context.cancellationDetails?.reset) {


Are we confident on if clause ordering? reset vs pause?

Between reset and pause yes. Added a conditional for explicit cancellations, ordered before the reset branch (to keep the cancellation -> reset -> pause ordering, though this is just for logging)

mjameswh · 2025-08-26T20:18:31Z

packages/worker/src/activity.ts

+                failed: {
+                  failure: await encodeErrorToFailure(
+                    this.dataConverter,
+                    new ApplicationFailure('Activity reset', 'ActivityReset')


Retryable or not? Prefer making this explicit, for the benefit of readers.

Is there any official semantic on this error? Does the server expect specific type?

Should we avoid attaching stack trace on that specific error?

I'm a bit concerned here that ApplicationFailure are meant for user usage, so exposing activity reset as that error type might be "misinterpreted" as something they need to investigate, so they would look at the stack trace and try to figure it out. "Activity Reset" is a pretty generic name, an error message that users could have written themselves...

I've make non-retryable explicitly false.

The server does not expect a specific error type (this error returned is agnostic to server)

I don't really see a reason why

I'm less concerned about using ApplicationFailure for this, partly because it's a user-triggered error, and because it's what we chose to do for Python and TS for activity pause (though other SDKs diverge - Java introduced ActivityPausedException as a subtype of ActivityCompletionException, Go cancels the context with a ErrActivityPaused cause).

The alternative is we don't wrap and we emit a CancelledFailure with message RESET, see:
https://github.com/temporalio/sdk-typescript/blob/9089bc682b4334287e52cd0bdb4547836f63e335/packages/worker/src/activity.ts#L77-84
and
https://github.com/temporalio/sdk-typescript/blob/9089bc682b4334287e52cd0bdb4547836f63e335/packages/worker/src/worker.ts#L1072-1092
(the else conditional)

Whatever choice we make, we should also apply to the pause case, and potentially to Python as well.

@Sushisource wdyt

IMO what Java (wrap with more specific types) is probably the most sensible. But I agree we'd need to apply it to Python and pause as well.

You should get @dandavison 's take and coordinate Python change with him

@tconley1428 is working on activity reset right now

Then him, hehe 😅

After some discussion - we'll leave this as is.

mjameswh

Had a very quick look at it, pointing out anything that popped to my mind without any real analysis, so be careful.

Leaving more complete review to @Sushisource.

Sushisource

This makes sense to me, approving, but I have couple minor things to address, and possibly an extra test to add.

Sushisource · 2025-09-02T17:13:28Z

packages/test/src/helpers-integration.ts

-    //  • the activity vanished (already completed)
-    return activityInfo ? !activityInfo.paused : true;
+    if (state === 'pause') {
+      return activityInfo ? activityInfo.paused ?? false : true;


Sushisource · 2025-09-02T17:16:23Z

packages/client/src/async-completion-client.ts

      }
    } catch (err) {
      this.handleError(err);
    }
    if (cancelRequested) {
      throw new ActivityCancelledError('cancelled');
    }
+    if (reset) {


Well, though more than one may be true, the way this is written where each branch throws, that's kind of irrelevant. I would make them elses for that reason I think.

Sushisource · 2025-09-02T17:17:28Z

packages/test/src/test-integration-workflows.ts

+    t.deepEqual(result, {
+      cancelRequested: false,
+      notFound: false,
+      paused: false,


Do we have any tests where multiple things are true at once? EX: Paused and reset?

THardy98 requested a review from a team as a code owner June 19, 2025 11:50

THardy98 mentioned this pull request Jun 19, 2025

Activity reset #1720

Closed

THardy98 marked this pull request as draft June 19, 2025 11:51

THardy98 changed the title ~~Activity reset~~ [BLOCKED] Activity reset Jun 19, 2025

THardy98 force-pushed the activity_pause_unpause branch 2 times, most recently from de50973 to 3ee7227 Compare August 13, 2025 15:14

Base automatically changed from activity_pause_unpause to main August 18, 2025 19:26

THardy98 changed the title ~~[BLOCKED] Activity reset~~ Activity reset Aug 18, 2025

THardy98 force-pushed the activity_reset branch from e0d0403 to ab76e93 Compare August 18, 2025 21:03

THardy98 marked this pull request as ready for review August 18, 2025 21:50

cretz reviewed Aug 19, 2025

View reviewed changes

mjameswh reviewed Aug 26, 2025

View reviewed changes

THardy98 requested a review from Sushisource August 29, 2025 18:09

Sushisource approved these changes Sep 2, 2025

View reviewed changes

THardy98 added 4 commits September 2, 2025 15:42

activity reset

156fd42

use current core commit

6d3d364

fix test

600a776

add conditional branch for explicit cancellation log

1aeb11a

THardy98 force-pushed the activity_reset branch 4 times, most recently from f028958 to 16ef9e5 Compare September 2, 2025 19:58

address pr feedback, add pause + reset test

051ea89

THardy98 force-pushed the activity_reset branch from 16ef9e5 to 051ea89 Compare September 2, 2025 19:58

make pause + reset test less flaky

cf9ecfc

THardy98 merged commit fcda0b7 into main Sep 3, 2025
42 of 43 checks passed

THardy98 deleted the activity_reset branch September 3, 2025 00:20

Activity reset #1730

Activity reset #1730

Uh oh!

Conversation

THardy98 commented Jun 19, 2025

What was changed

Why?

Uh oh!

THardy98 commented Jun 19, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

THardy98 Aug 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

THardy98 Aug 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sushisource Aug 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjameswh left a comment

Choose a reason for hiding this comment

Uh oh!

Sushisource left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

THardy98 Aug 20, 2025 •

edited

Loading

THardy98 Aug 28, 2025 •

edited

Loading

Sushisource Aug 28, 2025 •

edited

Loading