fix(activity): reject updates and unpause during pending reset - #11360
fix(activity): reject updates and unpause during pending reset#11360spkane31 wants to merge 5 commits into
Conversation
|
Claude finished @spkane31's task in 4m 57s —— View job Review complete
The core change looks sound. The Findings are inline: an internal-status leak in the new rejection error (plus a stale doc comment and a coverage gap left by the deleted timer test), a single-read state assertion that the rest of the file wraps in Not done: I could not run |
![github-actions[bot]](https://cdn.github.com/in/15368?s=60&v=4){kind=small}
| case activitypb.ACTIVITY_EXECUTION_STATUS_CANCELED, | ||
| activitypb.ACTIVITY_EXECUTION_STATUS_COMPLETED, | ||
| activitypb.ACTIVITY_EXECUTION_STATUS_FAILED, | ||
| activitypb.ACTIVITY_EXECUTION_STATUS_RESET_REQUESTED, |
There was a problem hiding this comment.
Folding RESET_REQUESTED into the terminal-state case makes the resulting error leak an internal status. RESET_REQUESTED has no public counterpart — runState() deliberately reports it as STARTED or PAUSE_REQUESTED because "the public PendingActivityState enum does not have a RESET_REQUESTED variant" (activity.go:1667-1670). So a caller who just saw STARTED from DescribeActivityExecution gets Cannot update options for activity in state ACTIVITY_EXECUTION_STATUS_RESET_REQUESTED, naming a state they can't observe. It also reads as permanent, when this rejection is transient — the update succeeds again once the worker yields.
A separate check ahead of the terminal switch keeps the two rejection reasons distinct:
if a.Status == activitypb.ACTIVITY_EXECUTION_STATUS_RESET_REQUESTED {
return nil, serviceerror.NewFailedPrecondition("cannot update options while a reset is pending")
}
switch a.Status {
case activitypb.ACTIVITY_EXECUTION_STATUS_CANCELED,
...Two follow-ons from this line:
reissueRunningAttemptTimers's doc comment (activity.go:1463-1467) still listsRESET_REQUESTEDamong the statuses it re-emits timers for.UpdateActivityExecutionOptionsis its only caller, so that status is now unreachable there and the comment is stale.- Deleting
UpdateOptionsPreservesTimeoutsWhileResetRequestedremoves the only coverage of that re-emission path for a running attempt. Worth confirming an equivalent case still exists forSTARTED/PAUSE_REQUESTED, otherwise the stamp-bump-invalidates-timers regression it guarded is now untested.
Also worth stating in the PR risks: shortening StartToCloseTimeout was an operator escape hatch for a worker that stops heartbeating while a reset is pending. That's gone; recovery now waits on the pre-existing timeouts or a terminate.
| // The deferred reset should land the activity in PAUSED, honoring the keep-paused intent | ||
| // that survived the rejected unpause (rather than dispatching a new attempt). | ||
| require.Equal(t, enumspb.PENDING_ACTIVITY_STATE_PAUSED, | ||
| describeActivity(ctx, t, activityID, startResp.GetRunId()).GetInfo().GetRunState()) |
There was a problem hiding this comment.
Every sibling assertion of this same transition — deferred keep-paused reset applying when the worker yields — waits rather than reading once: line 12999 (waitForState(... PENDING_ACTIVITY_STATE_PAUSED)) and lines 13537-13546 (await.Require). A bare describeActivity here asserts on the first read after failAttemptRetryably returns, so if that state isn't visible to Describe the instant the respond RPC returns, the subtest fails outright instead of retrying. waitForState is already in scope and used at line 13585 in this same subtest.
| // The deferred reset should land the activity in PAUSED, honoring the keep-paused intent | |
| // that survived the rejected unpause (rather than dispatching a new attempt). | |
| require.Equal(t, enumspb.PENDING_ACTIVITY_STATE_PAUSED, | |
| describeActivity(ctx, t, activityID, startResp.GetRunId()).GetInfo().GetRunState()) | |
| // The deferred reset should land the activity in PAUSED, honoring the keep-paused intent | |
| // that survived the rejected unpause. | |
| waitForState(ctx, t, activityID, startResp.GetRunId(), enumspb.PENDING_ACTIVITY_STATE_PAUSED) |
| // reports back. An unpause request that arrives in that window is rejected outright: | ||
| // RESET_REQUESTED is a niche state permutation the server refuses rather than resolving | ||
| // ambiguously (e.g. by clearing the pending keep-paused intent). |
There was a problem hiding this comment.
This explains the decision process rather than the test: "niche state permutation", "refuses rather than resolving ambiguously", and the parenthetical counterfactual about what the server could have done. The repo review guidelines call out exactly these (no counterfactuals or authoring-time discussion, no metaphorical/alien phrasing, simple sentence structure). The behavior is what the test needs to state:
| // reports back. An unpause request that arrives in that window is rejected outright: | |
| // RESET_REQUESTED is a niche state permutation the server refuses rather than resolving | |
| // ambiguously (e.g. by clearing the pending keep-paused intent). | |
| // reports back. An unpause request that arrives in that window is rejected. |
2cd1664 to
1e0d14a
Compare
| req *activitypb.UpdateActivityExecutionOptionsRequest, | ||
| ) (*activitypb.UpdateActivityExecutionOptionsResponse, error) { | ||
| frontendReq := req.GetFrontendRequest() | ||
| if a.Status == activitypb.ACTIVITY_EXECUTION_STATUS_RESET_REQUESTED { |
There was a problem hiding this comment.
This might have been before my merge, but move this after the request ID check below
|
|
||
| if a.Status == activitypb.ACTIVITY_EXECUTION_STATUS_RESET_REQUESTED { | ||
| return nil, serviceerror.NewFailedPrecondition("cannot update options while a reset is pending") | ||
| } |
There was a problem hiding this comment.
Let's just put this in the switch statement?
| } | ||
| case activitypb.ACTIVITY_EXECUTION_STATUS_RESET_REQUESTED: | ||
| a.ResetKeepPaused = false | ||
| default: |
There was a problem hiding this comment.
👍 Maybe one day we can think about whether it's possible to change the state machine library to allow expressing conditional destination states. The state machine library in sdk-core does support it FWIW.
| // (STARTED / CANCEL_REQUESTED / PAUSE_REQUESTED). | ||
| func (a *Activity) reissueRunningAttemptTimers(ctx chasm.MutableContext, attempt *activitypb.ActivityAttemptState) { | ||
| if !a.hasAttemptInProgress() { | ||
| return |
There was a problem hiding this comment.
Classic example of agent comments trying to explain how the entire code base works in a comment. Let's just nuke it. Seems like it was correct to have RESET_REQUESTED in there. But rather than having to think about it, let's just get rid of the comment.
87e320d to
e0860f8
Compare
debf40f to
f0b8296
Compare
f0b8296 to
c005ce5
Compare
3 participants
What changed?
UpdateActivityExecutionOptionswhile a reset is pending.UnpauseActivityExecuiontduring a deferred keep-paused reset.Why?
SAA crew (me, @fretz12, @dandavison) agree that these niche state transitions are better off as a
FailedPreconditionand can be allowed in the future or more specific errors can be given.How did you test it?
Potential risks
This intentionally changes previously allowed option updates during
RESET_REQUESTEDto returnFailedPrecondition.