remove reset_heartbeats field in ResetActivityExecutionRequest#10954
Merged
spkane31 merged 7 commits intoJul 15, 2026
Conversation
There was a problem hiding this comment.
Pull request overview
This PR simplifies the activity reset API/behavior by making activity resets always clear heartbeat details, removing the now-unnecessary reset_heartbeats state, and updating state machine behavior and tests accordingly.
Changes:
- Always clear stored heartbeat details on activity reset (including deferred reset application paths).
- Remove
reset_heartbeatsfrom the CHASMActivityStateproto/state and delete associated state machine logic. - Update functional/unit tests to reflect the new “reset always clears heartbeat” semantics.
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
tests/activity_standalone_test.go |
Updates standalone reset tests to remove the reset-heartbeat toggle and assert heartbeat clearing as the default behavior. |
chasm/lib/activity/statemachine.go |
Removes ResetHeartbeats state handling and makes deferred reset transitions always clear heartbeat state. |
chasm/lib/activity/statemachine_test.go |
Replaces the old reset-flag tests with focused unit tests that verify heartbeat state is cleared on reset/deferred reset. |
chasm/lib/activity/proto/v1/activity_state.proto |
Removes the reset_heartbeats field from persisted activity state. |
chasm/lib/activity/handler.go |
Forces reset-heartbeat behavior on the workflow-path reset request (consistent with “always clear”). |
chasm/lib/activity/gen/activitypb/v1/activity_state.pb.go |
Regenerated/updated protobuf output reflecting the removed field. |
chasm/lib/activity/activity.go |
Makes reset paths unconditionally clear heartbeat state and removes now-unused request-driven branching. |
Files not reviewed (1)
- chasm/lib/activity/gen/activitypb/v1/activity_state.pb.go: Generated file
| @@ -128,11 +128,6 @@ message ActivityState { | |||
| // metadata only. | |||
| ActivityPauseState last_pause_state = 16; | |||
Contributor
There was a problem hiding this comment.
I think this is the right thing to do. EDIT @spkane31 points out that it isn't because these proto fields have never been deployed
fretz12
approved these changes
Jul 9, 2026
| }, | ||
| Activity: &workflowservice.ResetActivityRequest_Id{Id: frontendReq.GetActivityId()}, | ||
| ResetHeartbeat: frontendReq.GetResetHeartbeat(), | ||
| ResetHeartbeat: true, |
Contributor
There was a problem hiding this comment.
Do you think we should remove this from the proto as well since it's always true now?
Contributor
Author
There was a problem hiding this comment.
This is for the existing ResetActivity endpoint that was already existing for resetting activities, we should set this to true because that is the new default behavior.
fretz12
force-pushed
the
feature/activity-operator-cmds
branch
from
July 10, 2026 17:31
6d922e5 to
9ccc41b
Compare
spkane31
force-pushed
the
spk/remove-keep-heartbeats
branch
from
July 15, 2026 15:18
ada8536 to
bed22f2
Compare
spkane31
force-pushed
the
spk/remove-keep-heartbeats
branch
from
July 15, 2026 15:27
bed22f2 to
fb11277
Compare
dandavison
reviewed
Jul 15, 2026
| a.ResetHeartbeats = false | ||
| a.clearHeartbeat(ctx) | ||
| } | ||
| a.clearHeartbeat(ctx) |
Contributor
There was a problem hiding this comment.
How about we name this clearHeartbeatDetails (this PR or another). My problem with clearHeartbeat is that IMO "heartbeat" on its own refers to the liveness check (the heartbeating) as opposed to the checkpoint data (what Temporal calls "heartbeat details" ). And here it's the checkpoint data we're clearing. (However, we shouldn't introduce the word "checkpoint" seeing as it has baggage and Temporal doesn't use it elsewhere.)
dandavison
reviewed
Jul 15, 2026
| }, 5*time.Second, 200*time.Millisecond) | ||
|
|
||
| // Reset with heartbeat reset | ||
| // Reset clears recorded heartbeat state. |
Contributor
There was a problem hiding this comment.
this terminology "heartbeat state" seems good to me but the test names could be improved to ResetClearsHeartbeat{Details,State}
dandavison
approved these changes
Jul 15, 2026
dandavison
pushed a commit
that referenced
this pull request
Jul 18, 2026
Implements Pause, Unpause, Reset, and UpdateActivityExecutionOptions RPCs for standalone CHASM activities. Adds matching frontend and history service handlers in chasm/lib/activity that dispatch to either the workflow-activity path or the standalone-activity path based on the presence of a workflow ID. Also adds PAUSE_REQUESTED and RESET_REQUESTED state machine states for standalone activities, the original_options snapshot for restore-on-reset semantics, the activity-options merge helpers (common/activityoptions), and the supporting proto schema changes (start_request_id, sdk_name, sdk_version, etc.). Squashed PRs: #9693 RPC boilerplate and code generation #9850 Implement UpdateActivityExecutionOptions #9851 Implement Pause/UnpauseActivityExecution for SAA #9852 Implement ResetActivityExecution for SAA #10001 Pause returns FailedPrecondition when already paused #10025 Validator function fixes #10265 Replace PauseState flag with PAUSE_REQUESTED state #10364 Add RESET_REQUESTED state to standalone activity #10745 Add support for updating start_delay via UpdateActivityOptions #10913 Preserve SAA retry backoff across unpause re-dispatch #10914 Fix SAA unpause for keep-paused reset #10917 Reject stale legacy SAA schedule-to-close timers #10920 Preserve NextRetryDelay across unrelated activity option updates #10921 Validate merged SAA retry policy updates #10923 Fix SAA reset attempt dispatch time reporting #11010 Use default retry policy when updating with nil #11020 Fix: de-duplicate pause even if activity is unpaused #11013 Fix: reject restore_original when a standalone activity has no original options snapshot #11032 Reject unsupported and empty update_mask paths in UpdateActivityExecutionOptions #11030 Fix: describe only provides heartbeat/failure when opted-in #11016 Preserve pending activity next retry delay #11068 Add SAA operator APIs to DC redirection whitelist #10954 Remove reset_heartbeats field in ResetActivityExecutionRequest #11087 Fix standalone activity heartbeat flag precedence #11117 Validation to reject negative activity jitter #11120 Fix bogus StartToCloseLatency for non-running standalone activities #11097 Fix task token invalidation after resetting an activity #11067 SAA bug fix: update start delay while paused #11123 SAA: cleanup
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changed?
Resetting an activity execution now always clears heartbeat details.
temporalio/api#820 Removes the
reset_heartbeatfield from the Request message.Why?
This change simplifies the API surface and we (the engineering/product team) do not see a good use case for resetting an activity and keeping heartbeat data. If this is needed or there is a big customer ask for it we can add this back in the future.
How did you test it?
Potential risks
Minimal, PR is into a feature branch