Repository navigation
Conversation
`@WorkflowMethod(compensateOnCancel = true)` routes a cancel into the existing saga chain. With `INFLIGHT_CANCELLATION_CHECKPOINTS` on, cancelWorkflow keeps an execution task instead of removing it, so the run settles through the execution handler's cancel guard; that guard schedules the compensation task when completed compensable checkpoints exist and publishes `CANCEL_COMPENSATION_SCHEDULED`, naming an interrupted action that is left uncompensated. The compensation handler proceeds on CANCELLED for opted-in workflows. Compensating actions are exempt from the cancel boundary check and from interrupt registration. Workflows that do not opt in are unchanged. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…cel paths One helper reloads the checkpoints and schedules the compensation task for both entries; the opt-in resolver joins WorkflowInspector beside the signal-persistence one; the counter and the public surface on WorkflowCancelledException are dropped; comments and docs reworded. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…ted action cancelWorkflow schedules the kept execution task without a rerun request, so an in-flight run settles once and never starts a second compensation; the request is sent after onCancelled. The cancel settle publishes CANCEL_COMPENSATION on every opted-in cancel, naming an interrupted action whether or not anything is compensable. Docs state the crash bound for unwritten checkpoints and that the two callbacks have no guaranteed order. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…t a failed cancel be retried The compensation task honors an active lease, so a later run of the cancelled workflow (a timer firing, a signal) cannot restart reversals that are already running; the cancel settle also skips the request when a compensation task exists. Cancelling an already cancelled opted-in workflow only re-requests the settle run, so a cancel that died after writing CANCELLED can be retried. Docs widen the uncompensated-checkpoint bound to a cancel landing on the run's final write. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…cel behavior Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
rgamba
left a comment
There was a problem hiding this comment.
Reviewed the cancel → settle → compensation paths. One confirmed bug (a timer that fires after compensation completes makes the execution task retry forever), plus a few smaller correctness gaps inline.
| executionContext: ExecutionContext, | ||
| result: WorkflowExecutor.ExecutionResult, | ||
| ) { | ||
| val scheduled = scheduleCompensationIfNeeded(workflowInstance, executionContext) |
There was a problem hiding this comment.
Confirmed bug: a wait timer that fires after cancel-compensation completes puts the execution task into an endless retry loop.
Cancelling a WAITING workflow leaves its timer ACTIVE. Once compensation finishes, the timer fires and TimerTaskHandler schedules an execution. WorkflowExecutor sees a terminal status and returns convertToExecutionResult (newStatus COMPENSATION_COMPLETED, plus the stored result). persistExecutionResult then passes that result along because the status is terminal, and the WorkflowUpdateRequest init rejects it with result must only be present for terminal statuses (COMPLETED, ERROR, CANCELLED). handleWorkflowExecutionError treats this as TRANSIENT_ERROR and reschedules, and the same thing happens on every retry.
Reproduced on SQLite by adding this to TestCompensateOnCancel:
@Test
fun timerFiringAfterCompensationCompleted() {
val workflow = workflowFactory<CompensateOnCancelWorkflow>(workflowId)
workflow.oneStepThenShortWait() // 2 s wait
helper.waitForWorkflowToReachStatus(WorkflowInstance.Status.WAITING)
skipperEngine.cancelWorkflow(workflowId, "cancelled while waiting")
helper.waitForWorkflowToReachStatus(WorkflowInstance.Status.COMPENSATION_COMPLETED)
Thread.sleep(5_000)
assertThat(scheduler.getTask<Any>(workflowId).isEmpty).isTrue() // fails
}After 5 s the task is still PENDING with retryCount=8, and every run throws the IllegalArgumentException above. Before this PR the instance stayed CANCELLED, so a late timer run was harmless. testCancelWhileWaitingCompensatesTheCompletedAction misses this because its 30 s timer never fires within the test.
Suggested fix: cancel the instance's ACTIVE timers (and remove their timer tasks) when the cancel compensation is requested here. Alternatively, have persistExecutionResult skip the write when the run saw a COMPENSATION_* status, which would cover the ERROR path too.
| if (WorkflowInspector.isCompensateOnCancel(workflowInstance.workflowClass, workflowInstance.workflowMethod) && | ||
| !compensationPending(workflowInstance.workflowId) | ||
| ) { | ||
| scheduleCancelCompensation(workflowInstance, context, result) |
There was a problem hiding this comment.
If scheduleCancelCompensation throws (the checkpoint reload or persistentScheduler.schedule hits a DB error), the exception escapes persistExecutionResult and handleWorkflowExecutionError turns it into TRANSIENT_ERROR. notifyCaller then calls onRetryableError on a workflow that already received onCancelled, and a blocked caller's future fails with TransientError instead of CancelledWorkflow. The retry settles it eventually, but callbacks and the caller see a spurious error. Could this catch the scheduling failure, return the CANCELLED result with a retry delay, and leave the callbacks alone?
| * Whether a compensation task for the workflow already exists, in any state. A later run of | ||
| * a cancelled workflow (a timer firing, a signal) must not request a second one. | ||
| */ | ||
| private fun compensationPending(workflowId: String): Boolean { |
There was a problem hiding this comment.
When nothing was compensable, no compensation task is ever created, so this always returns false for that instance. Every later run of the CANCELLED instance (timer expiry, a signal, a re-cancel) goes through the guard again: one more full getActionCheckpoints read and one more CANCEL_COMPENSATION event ("compensation scheduled: false") for the same cancel. It may be worth recording that the cancel was settled, or emitting the event only from the first settle.
| private val log = LoggerFactory.getLogger(SkipperEngine::class.java) | ||
|
|
||
| /** Id of the compensation task of [workflowId]; one per workflow. */ | ||
| fun compensationTaskId(workflowId: String): String = "$workflowId-compensation" |
There was a problem hiding this comment.
forceDeleteWorkflow removes only the execution task. With this PR, a CANCELLED opted-in instance can have a pending <id>-compensation task. Force-deleting it skips the cancel (CANCELLED counts as terminal), deletes the row and leaves the compensation task behind. CompensationFlowTaskHandler.retrieveWorkflowInstance then throws Workflow not found, and the generic catch reschedules it at unexpectedErrorRetryDelay, forever. Since this helper now exists, forceDeleteWorkflow could also remove compensationTaskId(workflowId). That would cover this case and the ERROR one.
| val compensate = | ||
| featureGate.isEnabled(FeatureGate.Keys.INFLIGHT_CANCELLATION_CHECKPOINTS) && | ||
| WorkflowInspector.isCompensateOnCancel(instance.workflowClass, instance.workflowMethod) | ||
| if (compensate && instance.status == WorkflowInstance.Status.CANCELLED) { |
There was a problem hiding this comment.
This retry only re-requests the settle run. It drops the new reason and never re-fires onCancelled or WORKFLOW_CANCELLED, so a cancel that died after the CANCELLED write but before the callback can't be fully retried, though the docs say it can. It also changes the API contract: WorkflowsService.cancelWorkflows now reports already-cancelled opted-in IDs as successfully cancelled (and schedules a run for each), where it used to throw. The admin page hides Cancel for CANCELLED rows (index.html:216), so operators can't reach this retry path from the UI anyway.
| if (status == WorkflowInstance.Status.ERROR || status.isCompensationInProgress()) { | ||
| return StatusValidationResult.PROCEED | ||
| } | ||
| if (status == WorkflowInstance.Status.CANCELLED && WorkflowInspector.isCompensateOnCancel( |
There was a problem hiding this comment.
The opt-in decision is gated on INFLIGHT_CANCELLATION_CHECKPOINTS in two places, cancelWorkflow (keep the task) and the settle guard in persistExecutionResult, while this handler doesn't check the gate at all. If the gate is turned off between a cancel and its settle run, for example during a rollback, the settle run skips the guard, persists CANCELLED, completes the task and never compensates. Nothing records that compensation was skipped, and re-cancelling with the gate off throws. Could the settle run use the decision made at cancel time (a kept task means compensate), or could the docs call out the rollback hazard?
…ensation request a cancel, clean up on force delete A timer or signal that starts a run of an instance in a COMPENSATION_* status now finishes the task without executing or writing (the write was rejected and retried forever). If requesting the cancel's compensation fails, the settle is retried later and the caller still sees the cancel. A cancel settle with nothing scheduled and nothing interrupted publishes no event. forceDeleteWorkflow also removes a pending compensation task. Docs state that a re-cancel drops the new reason and does not re-fire onCancelled, and that the gate must stay on until settle. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The pending-compensation lookup now sits inside the same catch as the compensation request, and the failure log names the interrupted action. Comments reworded. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
… on cancel Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Opt-in compensation on cancel. A workflow method declared
@WorkflowMethod(compensateOnCancel = true)has its completed compensable actions rolled back when the instance is cancelled, through the same saga chain that runs on ERROR, and the instance ends asCOMPENSATION_COMPLETED(orCOMPENSATION_ERROR) instead of stayingCANCELLED. RequiresINFLIGHT_CANCELLATION_CHECKPOINTS. Workflows that do not opt in keep today's cancel behavior.Mechanism. ERROR is a terminal status that enters the chain by scheduling a compensation task that
CompensationFlowTaskHandleraccepts; CANCELLED now does the same for opted-in workflows, with no new persisted status. Compensation is requested where the execution settles, inWorkflowExecutionTaskHandler's existing cancel guard, because only there are all completed actions' checkpoints stored. To make every cancel settle there,cancelWorkflowkeeps an execution task for an opted-in workflow instead of removing it, requested afteronCancelledwith the lease honored and no rerun: a run in flight keeps its lease and settles once on its own; an idle workflow gets a run that stops at the cancelled check and settles the same way. (Whether a run is in flight cannot be read from the scheduler, since an in-memory execution's persisted task is PENDING, so nothing branches on it.) The guard reloads checkpoints through the helper the ERROR path uses, schedules the compensation task when a completed compensable action exists and none is pending, and publishesCANCEL_COMPENSATION, whose description says whether a task was scheduled and names an action that an in-flight interrupt aborted (no checkpoint, not compensated).scheduleCompensationTaskhonors an active lease, so a later run of the cancelled workflow (a timer firing, a signal) cannot restart reversals already running. The compensation handler proceeds onCANCELLEDonly for opted-in workflows. Compensating actions are exempt from the cancel boundary check and from interrupt registration, since they run while the row says CANCELLED. Cancelling an already cancelled opted-in instance only re-requests the settle run, so a cancel that failed after writing CANCELLED can be retried. The cancel reason stays the instance result through compensation.CANCELLED, completed actions' side effects keptCOMPENSATION_COMPLETEDCANCELLED, task removedAlso changed. Because
scheduleCompensationTasknow honors an active lease for every compensating workflow,WorkflowsService.reExecuteWorkflowson aCOMPENSATION_IN_PROGRESSinstance whose task is pending or leased is a no-op until that task fails; before, it reset the task and could run the same reversals on a second worker.cancelWorkflowsnow reports an already cancelled opted-in instance as cancelled (it re-requests the settle run) where it used to throw. Two fixes reach the ERROR path too: a run started by a late timer or signal for an instance in aCOMPENSATION_*status now finishes its task instead of retrying a rejected status write forever (checked in the task handler before anything executes, so neither the executor nor the checkpoint and timer reads run for it), andforceDeleteWorkflowremoves a pending compensation task.Limits, stated in the docs. Actions whose
EVENTUAL_CHECKPOINTcheckpoints were not yet written when the cancelled run could not finish (worker died, or the cancel landed on the run's final write) are not compensated, since a cancelled instance is not replayed; useIMMEDIATE_CHECKPOINTwhere that matters.onCancelledandonCompensationCompletedfire in no guaranteed order, andonCompensationCompletedis delivered at least once, as on ERROR. A cancel that dies between the CANCELLED write and the settle request leaves an instance waiting only for a signal (no timer, no task) uncompensated until it is cancelled again; a re-cancel re-requests compensation but drops the new reason and does not re-fireonCancelled. The gate must stay on from the cancel until the instance reaches its final status; one caught in between stays CANCELLED without compensation. If requesting compensation fails at settle (the lookup or the request), the settle is retried and the caller still sees the cancel. A pre-existing gap stays open for a follow-up: a compensating instance whose execution timeout passes is still timed out, on the ERROR path as well.Tests.
WorkflowExecutionTaskHandlerTest(settle requests compensation and names the interrupted action; still names it when nothing is compensable; nothing without opt-in; no second request while one is pending),CompensationFlowTaskHandlerTest(CANCELLED proceeds only with opt-in),SkipperEngineTest(task kept for an opted-in cancel with the gate on, removed otherwise; compensation request honors the lease; re-cancel re-requests the run, is retryable after a scheduler failure, and still throws past CANCELLED or without opt-in),ActionExecutorTest(a compensating action runs while the workflow is CANCELLED and is not registered for interrupts),WorkflowInspectorTest, and SQLite integ tests (cancel mid-flight compensates two completed actions in reverse and not the interrupted one; a slow compensation and both callbacks happen once; a wait timer firing during compensation does not repeat it; a timer firing after compensation completed leaves no task; cancel while waiting compensates; a failed compensation is re-entered via re-execution and finishes; the existing non-opted-in cancel test still skips compensation).