Do not permit UpdateOptions in CANCEL_REQUESTED or RESET_REQUESTED - #11394
Do not permit UpdateOptions in CANCEL_REQUESTED or RESET_REQUESTED#11394dandavison wants to merge 1 commit into
UpdateOptions in CANCEL_REQUESTED or RESET_REQUESTED#11394Conversation
Assert the refusal of UpdateOptions in CANCEL_REQUESTED and RESET_REQUESTED Both tests asserted that an update applies in those states. UpdateWhileCancelRequestedFails now requires FailedPrecondition, an unchanged heartbeat timeout, and an undisturbed cancellation. UpdateOptionsPreservesTimeoutsWhileResetRequested existed to pin that the handler re-emits attempt-scoped timeout tasks for a RESET_REQUESTED activity, which no longer happens at all. It becomes UpdateOptionsWhileResetRequestedFails: the refusal, the surviving StartToCloseTimeout, and the still-pending reset. go test -count=1 -tags test_dep -run TestStandaloneActivityTestSuite ./tests/
|
Claude finished @dandavison's task in 4m 7s —— View job Review complete
The core change is sound and narrowly scoped. Four inline comments posted (2 on the test, 2 on Things I checked and found fine:
Not done: |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| require.NotEqual(t, 15*time.Second, descResp.GetInfo().GetHeartbeatTimeout().AsDuration(), | ||
| "a refused update must not apply") |
There was a problem hiding this comment.
This assertion is vacuous. startAndValidateActivity → startActivityWithType never sets HeartbeatTimeout, so the field is nil and AsDuration() is 0. NotEqual(15s, 0) passes no matter what the handler did — it would also pass if the refused update had somehow written 7 * time.Second. Assert the actual expected value instead:
| require.NotEqual(t, 15*time.Second, descResp.GetInfo().GetHeartbeatTimeout().AsDuration(), | |
| "a refused update must not apply") | |
| require.Zero(t, descResp.GetInfo().GetHeartbeatTimeout().AsDuration(), | |
| "a refused update must not apply") |
| // The refused update leaves the original 1-minute StartToCloseTimeout and the pending reset in | ||
| // place, so the activity stays RESET_REQUESTED until the worker yields. |
There was a problem hiding this comment.
The comment says the activity stays RESET_REQUESTED, but line 14553 asserts PENDING_ACTIVITY_STATE_STARTED. Both are correct — RESET_REQUESTED has no PendingActivityState variant (see activity.go:1724-1727) — but a reader hits an apparent contradiction. Worth stating the mapping:
| // The refused update leaves the original 1-minute StartToCloseTimeout and the pending reset in | |
| // place, so the activity stays RESET_REQUESTED until the worker yields. | |
| // The refused update leaves the original 1-minute StartToCloseTimeout and the pending reset in | |
| // place. RESET_REQUESTED has no PendingActivityState variant, so the run state remains STARTED. |
| activitypb.ACTIVITY_EXECUTION_STATUS_CANCEL_REQUESTED, | ||
| activitypb.ACTIVITY_EXECUTION_STATUS_RESET_REQUESTED, |
There was a problem hiding this comment.
With these two added, the permitted set is now exactly SCHEDULED, STARTED, PAUSED, PAUSE_REQUESTED — 4 of 12 statuses. Since the point of the PR is to narrow the surface, consider inverting to an explicit allowlist so a newly added status is rejected by default rather than silently permitted by the default: arm:
| activitypb.ACTIVITY_EXECUTION_STATUS_CANCEL_REQUESTED, | |
| activitypb.ACTIVITY_EXECUTION_STATUS_RESET_REQUESTED, | |
| switch a.Status { | |
| case activitypb.ACTIVITY_EXECUTION_STATUS_SCHEDULED, | |
| activitypb.ACTIVITY_EXECUTION_STATUS_STARTED, | |
| activitypb.ACTIVITY_EXECUTION_STATUS_PAUSED, | |
| activitypb.ACTIVITY_EXECUTION_STATUS_PAUSE_REQUESTED: | |
| default: | |
| return nil, serviceerror.NewFailedPreconditionf("Cannot update options for activity in state %s", a.Status.String()) | |
| } |
(the suggestion replaces lines 697-708)
Also, is the PAUSE_REQUESTED / CANCEL_REQUESTED asymmetry intended? Both are "worker still holds the task token" states; the restore-original ambiguity argued in the PR description applies to RESET_REQUESTED but not to either of those. Not blocking — just worth a sentence in the code or the description.
| activitypb.ACTIVITY_EXECUTION_STATUS_COMPLETED, | ||
| activitypb.ACTIVITY_EXECUTION_STATUS_FAILED, | ||
| activitypb.ACTIVITY_EXECUTION_STATUS_CANCEL_REQUESTED, | ||
| activitypb.ACTIVITY_EXECUTION_STATUS_RESET_REQUESTED, |
There was a problem hiding this comment.
Follow-on: reissueRunningAttemptTimers (line 775) is called only from this function, and its doc comment at line 1511-1515 ends with:
No-op unless the activity is in a status where a worker holds the task token (STARTED / CANCEL_REQUESTED / PAUSE_REQUESTED / RESET_REQUESTED).
After this change two of those four statuses can never reach the call site, so the comment now describes states that are unreachable. Suggest trimming it to (STARTED / PAUSE_REQUESTED).
1 participant
What changed?
Do not permit
UpdateOptionsinCANCEL_REQUESTEDorRESET_REQUESTEDWhy?
UpdateOptionslands afterReset(restore_original_options)then should the update be silently overridden when honoring the reset on attempt end?How did you test it?
Note
Medium Risk
Changes public API semantics for activities with pending cancel/reset; clients that relied on mid-flight option updates will now get FailedPrecondition.
Overview
UpdateActivityExecutionOptionsis no longer allowed while an activity is inCANCEL_REQUESTEDorRESET_REQUESTED. Those statuses are now treated like other non-updatable states and returnFailedPreconditionwith the same message pattern as terminal statuses.This replaces the prior behavior where options could still be updated on a running attempt with a pending cancel, and where updates during
RESET_REQUESTEDcould bump the attempt stamp and re-issue timeout tasks. Standalone activity tests were flipped to expect refusal and to assert timeouts and run state stay unchanged.Reviewed by Cursor Bugbot for commit b4c607a. Bugbot is set up for automated code reviews on this repo. Configure here.