Implement Pause/Unpause/Reset/UpdateOptions for standalone activities

[spkane31]

What changed?

This branch implements the activity operator commands feature — a set of server-initiated control APIs (PauseActivityExecution, UnpauseActivityExecution, ResetActivityExecution, UpdateActivityExecutionOptions) for both workflow-embedded and standalone activities.

• Pause, Unpause, Reset and UpdateOptions for standalone activities plus idempotency via RequestId
• common/activityoptions package (common/activityoptions/merge.go): extracted mergeActivityOptions from the update-options handler into a shared package (now also used by CHASM activity component).
• Metric renames: ActivityPauseRequests → ActivityPause, ActivityResetRequests → ActivityReset, ActivityUnpauseRequests → ActivityUnpause, ActivityUpdateOptionsRequests → ActivityUpdateOptions.
• RPC boilerplate, proto generation, and matching/frontend wiring for the new APIs.

Why?

Activity operator APIs existed for workflow-embedded activities but were not wired up for standalone activities.

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)

Potential risks

Minimal, this is a new feature so it won't break users.

[davidporter-id-au]

Question: This is intended to be a monotonic fencing token for updating activity options generally, is that correct?

In this specific instance you're using it for updating scheduleToClose timeout options, but in general it refers to the entire set of activity options being updated in the DB?

If that's the case, would there be value in describing this fencing token with a name that's a bit more specific to what it's guarding against? I don't have a firm opinion here, but perhaps something indicating that it's relating to the activity options lifecyle perhaps?

If this is a convention within CHASM in which every update is guarded by a field called stamp ignore me though, happy to go with the convention if it's a preexisting thing.

[spkane31]

Updating to schedule_to_close_stamp to make it obvious this stamp is only for schedule_to_close changes.

[davidporter-id-au]

ah, I just noticed htis is more of a convention for chasm, feel free to ignore my comment

[davidporter-id-au]

I'd be wary using booleans in IDL generally, because it's very often the case that the two things you want become a third - they usually work better as enums/ints over time.

Perhaps these three ought to be an an update struct, whose presence indicates that there's a need to update the options? Like, for example:

UpdateActivityOptions updateActivityOptions = 16

... 

message UpdateActivityOptions {
  enum ActivityReset {
     NoChange = 0;
     ResetAttempts  = 1;
  }
  enum HeartbeatReset {
     NoChange = 0;
     ResetHeartbeats  = 1;
  }

  ActivityReset activity_reset = 1;
  HeartbeatReset reset_heartbeats = 2;
}

[spkane31]

I agree with this sentiment but these are new APIs that are expected to be drop in replacements for the existing methods that only apply to workflow activities, we are planning on removing those APIs in the near future. So we are unfortunately stuck with this behavior because it is exposed in the CLI.

Example: https://github.com/temporalio/api/blob/4087826c775cd67f8658e24389c942e1510b51de/temporal/api/workflowservice/v1/request_response.proto#L2075-L2106

[dandavison]

I'm not quite following here -- this activity proto is internal. (Also what was the reference to the CLI?)

[davidporter-id-au]

is this safe to change? Is it in use? I'm not familiar with this file, so might be off base here, but this seems like a breaking change?

[spkane31]

Here is the original comment but no because this is an entirely internal implementation it does not constitute a breaking change.

[dandavison]

I believe this code path is missing generation of a request ID in the case where the client doesn't set it.

	if req.GetRequestId() == "" {
		req.RequestId = uuid.NewString()

Can you check that this is done in all validation code paths where it's needed? In the current PR state I believe there are code paths where we end up comparing request IDs "" == "" and saying "request IDs match". So it would be great to have a test that fails currently, and passes when we add the missing server-side generation of request IDs.

[dandavison]

Could you move this function (validateAndNormalizeStartRequest) down to its original location to make the diff readable?

[dandavison]

This function used to have

	if req.GetRequestId() == "" {
		req.RequestId = uuid.NewString()

but it looks like it was removed in a merge.

[spkane31]

Added back

[dandavison]

Can you check that we're minting missing request IDs and doing the request ID length check in all places needed?

[spkane31]

Checked and added the request ID length check where missing.

[dandavison]

Is there a place where we validate that workflow_id is present (and activity_id absent) if workflow_activity_type is present?

[spkane31]

We removed the support for workflow_activity_type so we do not have that validation

[fretz12]

Approved, but wait for Dan's approval too

[fretz12]

Suggested change

	return &activitypb.PauseActivityExecutionResponse{}, serviceerror.NewFailedPrecondition("activity is already paused")
	return nil, serviceerror.NewFailedPrecondition("activity is already paused")

[fretz12]

We can remove the TODO

[dandavison]

I inserted a call to Unpause here, expecting the test to fail, but it didn't.

		_, err = env.FrontendClient().UnpauseActivityExecution(ctx, &workflowservice.UnpauseActivityExecutionRequest{
			Namespace:  env.Namespace().String(),
			ActivityId: activityID,
			RunId:      runID,
			Identity:   "test-identity",
			Reason:     "test-pause",
		})
		require.NoError(t, err)

[spkane31]

Inserting a pause won't change the assertion, the assertion is validating that no task is dispatched while paused. The timeout on the poll is 2s so it's not guaranteed that the unpause will result in the task dispatch in that time. The assertion you're looking at is in UnpauseWhileScheduled which asserts the task gets dispatched after unpausing the test by using a longer context than the 2s

[dandavison]

No, that's not it. 2s is enough time for the Unpause to trigger a dispatch. The problem is that the 2s timeout you're using is too short. What's happening is that the poll request in the test is rejected with Context timeout is too short for long poll API, so your test assertion is never made, because if err == nil is always false.

So I think we need to increase that to something like 3s. If you do that, then adding an Unpause as I did does cause the test to fail. At the moment the test is not verifying that Pause blocks dispatch (but I'm not aware of any problem with the implementation, just the test.)

[dandavison]

nit: I believe in Go it's conventional to give a name prefix must to functions that panic on error, so mustResetActivity (same for any other functions of this form)

[spkane31]

This function won't panic, it will fail the test which is expected if the reset call returns an error.

[dandavison]

Nit: This test still passes even if the reset is removed. Could make that clear in the name ("WhileCancelRequestedDoesNotFail") or in comments.

[dandavison]

No, that's not it. 2s is enough time for the Unpause to trigger a dispatch. The problem is that the 2s timeout you're using is too short. What's happening is that the poll request in the test is rejected with Context timeout is too short for long poll API, so your test assertion is never made, because if err == nil is always false.

So I think we need to increase that to something like 3s. If you do that, then adding an Unpause as I did does cause the test to fail. At the moment the test is not verifying that Pause blocks dispatch (but I'm not aware of any problem with the implementation, just the test.)

[dandavison]

This doesn't actually check it's idempotent, just that it succeeds. To prove idempotency we could change some aspect of the request when it's submitted the second time and show that that aspect is ignored the second time because the server recognized it as a repeat request.

I tried "reason" but I think pause reason is not exposed? Also due to a known bug in CHASM, there's a persistence write causing StateTransitionCount to increase when processing repeat request IDs, so we can't use that either.

If you don't see a way to test idempotency currently, could just leave a comment to that effect.

[spkane31]

Added a note

[dandavison]

Why is the field in the proto if we're not using it for this purpose? I would have thought that we want to give a long-running activity a chance to stop executing on reset.

[spkane31]

Fixed

[dandavison]

Before we make a decision here, I'd like us to lay out the pros and cons.

When a pause request is received in state Started, there is a change in server state. We could model this explicitly in our state machine, or we could model it with additional state outside the explicit state machine.

In the current implementation, we transition from Scheduled to Paused, but when we receive Pause in state Started we transition to (Started, flag_set). Then, when the attempt is over and we are able to really pause, we enter state (Scheduled, flag_set). But this is confusing -- what's the difference between this and Paused? In fact, I think if we are going to use the flag, it might be cleaner to get rid of Paused state entirely and model pause via the flag alone rather than as a mixture.

There are various places where a code author has to remember to check the pause state as well as the standard statuses. For example, in task validators, in handlePauseRequested, handleUnpauseRequested. There's logic for when to clear the pause state (terminal states, and conditionally in handleReset, etc.
So, what are the pros and cons of the 3 options?

1. Add PauseRequested to the state machine. A slightly larger transition matrix but now code just branches on status and we don't worry about code forgetting or not understanding that it has to consider PauseState. This is consistent with the fact that we have a CancelRequested state.
2. Use a flag and also have Paused. But this seems to mean that Paused and (Scheduled, flag_set) are confusingly similar.
3. Use a flag alone to model pause, no pause-related state machine states.

SAA is using an explicit state machine to model/validate state transitions so we should default to (1). Are we aware of a strong argument for having CancelRequested but not PauseRequested If not, then let's embrace our state machine and implement them consistently.

That leaves ResetRequested. Do you think there's a stronger argument for implementing that one as a flag?

[dandavison]

I've made a prototype commit to add to this branch that switches to PauseRequested in #10265. @spkane31 could you take a look? I'm thinking that it is an improvement.

[spkane31]

PauseRequested (and ResetRequested if we decided to add that) would mean that standalone activities have a different behavior than workflow activities, which is something I tried to explicitly avoid. I think it's reasonable to want to use state transitions to model pause requested but this means when we port workflow activities to standalone activities that we have to be careful about behavior changes while doing that transition.

[dandavison]

The only user-visible difference if we use PauseRequested instead of a flag is that we cannot be in both cancel_requested and pause_requested at the same time: the heartbeat response can contain one or the other, but not both (cancel requested has precedence). Would you agree that that's better and clearer behavior? Can you think of any other user-facing differences?

As a general comment, IMO we should focus on designing the best user-facing behavior for Standalone Activity that we can, and we should also set a solid foundation and allow the new CHASM implementations to be good implementations (e.g. avoiding the confusion of having both Paused and Scheduled-with-pause-flag-set). It's OK to change minor details of workflow activity Pause, since this is not GA, especially ones that aren't desirable (being paused and canceled at the same time). We'll want to finalize this before merging -- we don't want to change the schema of the persisted data after it is live.

I'm not sure about ResetRequested, WDYT? Does the implementation work out cleanly with it as an explicit state rather the flags?

[spkane31]

I am ok with using Pause and Reset Requested transitions instead of state flags (I can prototype reset requested), I originally had a PauseRequested but it was removed. I am also trying to think about this in the light of worker commands and the worker nexus channel where the server can directly send commands to a worker instead of waiting for the worker to poll. This is something we probably want to implement in the future for activities (SDKs are still in progress for workflow cancellation request). If we chose to implement this how would that affect the XXXRequested state and end up being something we remove later, either way it's probably fine.
https://github.com/temporalio/temporal/blob/main/docs/architecture/worker-commands.md

[dandavison]

If we're in CancelRequested then Unpause will never result in execution resuming: retryable failures go straight to FAILED. So I'm thinking it should be a FailedPrecondition, like the terminal states are above.