Breaking change to current_retry_interval - #11203
Conversation
9e65264 to
525929e
Compare
There was a problem hiding this comment.
Don't forget to update the API docs: https://github.com/temporalio/api/blob/master/temporal/api/workflow/v1/message.proto#L310-L314
There was a problem hiding this comment.
| } else { | ||
| // in this case activity is at least scheduled | ||
| // retry has been dispatched to Matching | ||
| p.NextAttemptScheduleTime = nil |
There was a problem hiding this comment.
nit: technically this else clause is not needed since p is freshly created (line 100). But if you want to keep it here for clarity and behavior doc, that's fine
There was a problem hiding this comment.
Thanks, I think I'll leave it seeing as the code is going away, and there are other things in it that could be cleaned up. CHASM is the real clean up.
0cfa38f to
1a47931
Compare
Covers the SCHEDULED branch where the retry has already been dispatched to Matching (now >= scheduledTime): both NextAttemptScheduleTime and CurrentRetryInterval must be nil. Run: go test ./service/history/workflow/ -run 'TestActivitySuite/TestGetPendingActivityInfoRetryDispatchedToMatching' -count=1
Return nil after dispatch to Matching.
Consider an activity that's in retry backoff before attempt 2. This retry interval is 5s and the
next one, if there is another, will be 10s.
current_retry_interval:
SCHEDULED (dispatched to matching) STARTED (attempt completed/failed)
before 5s 10s nil
after 5s nil nil
next_attempt_schedule_time for comparison:
SCHEDULED (dispatched to matching at t) STARTED (attempt completed/failed)
t nil nil
A paused activity is held on the server and not dispatched to Matching, but it reaches the same code path as a dispatched retry, so current_retry_interval is now nil for it too. Run: go test ./tests/xdc/ -run 'TestActivityApiStateReplicationSuite/TestPauseActivityFailover'
Adds TestParityCurrentRetryInterval, driving both a workflow activity (the oracle) and a standalone activity through the same retry-backoff states and asserting the same public retry-scheduling info (current_retry_interval, next_attempt_schedule_time), via the halfway-house saaDriver/wfaDriver harness: - BackingOff: before the retry is dispatched to Matching, both fields are populated. - RetryDispatched: once dispatched (now >= scheduledTime), both are nil. - PausedAfterDispatch: pausing a dispatched retry preserves the nil. All three pass on both surfaces, so the current_retry_interval contract the WFA change locks holds for SAA too. Parameterizes the shared singleActivityWorkflow with RetryInterval. Run: go test -tags test_dep -run 'TestStandaloneActivityTestSuite/TestParityCurrentRetryInterval' -count=1 -v -timeout 480s ./tests/
… dispatch Adds PausedBeforeDispatch: an activity paused while still backing off should report neither current_retry_interval nor next_attempt_schedule_time, since no dispatch occurs while paused. This FAILS today: WFA keeps reporting the backing-off values until it flips the state to PAUSED, whereas SAA already nils them. The failing assertion records the intended end state. Run: go test -tags test_dep -run 'TestStandaloneActivityTestSuite/TestParityCurrentRetryInterval' -count=1 -v -timeout 480s ./tests/
GetPendingActivityInfo populated next_attempt_schedule_time / current_retry_interval whenever a SCHEDULED activity was still before its scheduled retry time, even when paused. A paused activity will not dispatch, so it must report neither. Gate the backing-off branch on !ai.Paused. Fixes TestParityCurrentRetryInterval/PausedBeforeDispatch. Run: go test ./service/history/workflow/ -run TestActivitySuite -count=1 go test -tags test_dep -run 'TestStandaloneActivityTestSuite/TestParityCurrentRetryInterval' -count=1 -v -timeout 480s ./tests/
1a47931 to
dd3c00c
Compare
TestParityCurrentRetryInterval now covers everything the SAA-only TestNextAttemptScheduleTimeAndCurrentRetryInterval did, checking WFA (the oracle) and SAA together: FirstAttemptRunning, NextRetryDelayOverride, RetryAttemptRunning, and FinalAttemptRunning (max 2). StartDelayPending stays SAA-only (WFA has no per-activity start delay). Deletes the now-redundant SAA-only test. Run: go test -tags test_dep -run 'TestStandaloneActivityTestSuite/TestParityCurrentRetryInterval' -count=1 -v -timeout 580s ./tests/
|
I've updated this PR so that |
…vers The drivers waited out the retry backoff with time.Sleep. Instead: where the next step is a poll, issue the long-poll directly (PollActivityTaskQueue blocks until the retry dispatches); where we observe the dispatched-but-unpolled state, awaitObserve until the projection shows the dispatch (next-schedule cleared). Tracks the real transition rather than a fixed wait; the subtest wall-clock drops ~4x. Run: go test -tags test_dep -run 'TestStandaloneActivityTestSuite/TestParityCurrentRetryInterval' -count=1 -v -timeout 580s ./tests/ Observe running state with a single describe, not a client-side poll loop PollActivityTaskQueue returns only after Matching records the attempt's start on the History shard, and DescribeActivityExecution reads that shard strongly-consistently, so the STARTED state is already visible when the poll returns. The observe-running cases now do a single describe instead of awaitObserve. Verified deterministic over repeated runs. Run: go test -tags test_dep -run 'TestStandaloneActivityTestSuite/TestParityCurrentRetryInterval' -count=1 -v -timeout 580s ./tests/ Use single describe after synchronous RPCs; reserve polling for dispatch-delay elapses Apply the driver waiting taxonomy (see dandavison/log#272): a synchronous RPC (RespondActivityTaskFailed, PauseActivityExecution) commits its transition on the History shard before returning, and Describe reads that shard strongly-consistently, so the backing-off / next-retry-delay / paused-before-dispatch cases observe with a single describe instead of a client-poll loop. awaitObserve is now reserved for the dispatch-delay elapse (backoff), whose read-time projection flip bumps no version for a long-poll to wake on. Verified over repeated runs. Run: go test -tags test_dep -run 'TestStandaloneActivityTestSuite/TestParityCurrentRetryInterval' -count=1 -v -timeout 580s ./tests/
The dispatch-delay projection poll used a hand-rolled loop with time.Sleep (forbidden by forbidigo). Switch awaitObserve to await.Require (the blessed polling helper): observe/describeActivity now take require.TestingT so the poll's describe runs against the callback's *await.T, making transient RPC errors retryable rather than a hard failure. Plain require.Eventually doesn't fit — it runs the condition in a goroutine (so observe's require calls would Goexit it) and returns only a bool, not the projection. Run: go test -tags test_dep -run 'TestStandaloneActivityTestSuite/TestParityCurrentRetryInterval' -count=1 -v -timeout 580s ./tests/
What changed?
Return nil after dispatch to Matching. Previously we were returning the predicted next retry interval
Consider an activity that's in retry backoff before attempt 2. This retry interval is 5s and the next one, if there is another, will be 10s.
current_retry_interval:next_attempt_schedule_timefor comparison:Why?
I think that the current behavior is a bug.
How did you test it?
Potential risks
Breaking change: someone could be relying on the old behavior
Note
Medium Risk
Intentional breaking change to public Describe metadata; clients that relied on post-dispatch predicted retry intervals will see nil instead. Execution/retry scheduling is unchanged—only read-side projection.
Overview
Breaking change to how pending activities expose
current_retry_intervalandnext_attempt_schedule_timeon Describe (workflow and standalone).Those fields are now set only while a retry is backing off before dispatch to Matching (future
scheduled_time, not paused). Once the retry task is dispatched—or while the activity is paused—both fields are nil. The server no longer fillscurrent_retry_intervalwith a predicted next backoff viaExponentialBackoffAlgorithmafter dispatch or while a retry is queued in Matching.Tests were expanded: unit coverage in
GetPendingActivityInfo, a consolidated workflow vs standalone parity suite (TestParityCurrentRetryInterval), removal of the duplicate standalone-only test, and XDC pause/reset expectations updated to expect nil interval when paused.Reviewed by Cursor Bugbot for commit c1ac44a. Bugbot is set up for automated code reviews on this repo. Configure here.