Repository navigation
Conversation
7ff4f0f to
e3307e2
Compare
e3307e2 to
6e86f86
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| operationTargets, allTargetsFuture := collectFutureNexusOperationTargets( | ||
| now, | ||
| op.GetScheduledTime().AsTime(), | ||
| op.GetNextAttemptScheduleTime().AsTime(), |
There was a problem hiding this comment.
med — HSM NextAttemptScheduleTime is in wall-clock time, but it is compared with virtual ms.Now() and used as a virtual skip target.
The HSM executor sets EventAttemptFailed.Time from env.Now() (hsm/nexusoperations/executors.go:570). stateMachineEnvironment.Now() returns shardContext.GetTimeSource().Now(), which is wall-clock time. So NextAttemptScheduleTime = wall + delay. ms.Now() is the time-skipping wrapped source, so it equals wall + S, where S is the accumulated skip. ScheduledTime comes from the event time, so it is virtual, and these targets mix both frames. The CHASM path does not have this problem because it uses ctx.Now(o).
After any earlier skip (S > 0):
- If
delay <= S,collectFutureNexusOperationTargetssees the retry as already due and blocks skipping. User timers are then not skipped while the operation backs off. - If
delay > S, the transition skips onlydelay - S, andTIME_SKIPPING_TRANSITIONEDrecords the wrong target and duration.
This happens on the second retry of the PR's own scenario. The first retry skip sets S = delay₁, and attempt 2 then computes wall + delay₂. The functional test makes only one retry, starting from S = 0, so it does not catch this. A related problem already exists: executeStateMachineTimers treats the same wall-clock deadline as virtual and subtracts S, so the HSM backoff timer fires S early. This PR now uses that value as a skip target.
Suggestion: Convert the HSM retry deadline to the virtual frame before using it, for example AdjustNowWithTimeSkipping/+ ms.accumulatedSkippedDuration() as of when it was computed. A better fix is to make the HSM Nexus executor stamp NextAttemptScheduleTime from the mutable state's virtual clock, matching CHASM. Also add a test with two retries, or one that starts with a non-zero accumulated skip.
| return workflowSkipEligibility{} | ||
| } | ||
| if nexusEvaluation.blocksSkipping { | ||
| noSkippingReason = "has pending nexus operations" |
There was a problem hiding this comment.
small — CHASM Nexus Operations that are SCHEDULED or STARTED now block time skipping, which is new behavior the PR description doesn't mention.
Before this PR, isWorkflowSkippable checked only nexusoperations.MachineCollection(ms.HSM()).Size(). A workflow whose Nexus Operations lived in the CHASM Workflow.Operations map could therefore skip time while an Operation was scheduled or running. Now evaluateCHASMNexusOperationsForTimeSkipping blocks skipping for any CHASM Operation that is not BACKING_OFF. The description says SCHEDULED/STARTED Operations "still block skipping", which is true only for HSM. This is probably the right behavior and matches HSM, but it is a new restriction for CHASM-backed workflows. For example, a long-running async CHASM Operation now stops user timers from being skipped.
Suggestion: State in the PR description that CHASM Nexus Operations now gate time skipping the same way HSM Operations do. Add a CHASM STARTED case to TestEvaluateWorkflowSkipEligibility, since only SCHEDULED is covered now.
There was a problem hiding this comment.
nit — Only tests call findNextSkipTarget now.
The close path calls findNextSkipTargetWithNexus(eligibility.nexusTimerTargets) directly. That leaves findNextSkipTarget as a production wrapper used only by tests, and it repeats the Nexus evaluation and its error logging.
Suggestion: Remove findNextSkipTarget. Have the tests call ms.findNextSkipTargetWithNexus(ms.evaluateWorkflowSkipEligibility().nexusTimerTargets), or rename findNextSkipTargetWithNexus to findNextSkipTarget.
What changed?
MutableState.AddTaskscallers.Why?
A pending Nexus Operation currently blocks workflow time skipping even when it cannot make progress until a future retry deadline. Treating retry backoff as idle time allows the workflow to advance to the next relevant deadline without skipping past the Operation's timeout boundaries.
Each retry skip intentionally consumes one
SessionSkipCount.MaxSessionSkipCountremains the runaway guard: after the limit is reached, time skipping is disabled and subsequent retries observe their normal wall-clock backoff.How did you test it?
Potential risks
BACKING_OFFand all retry and timeout targets are strictly in the future;SCHEDULEDandSTARTEDOperations still block skipping.