Skip to content

feat: cooperative in-flight cancellation checkpoint (opt-in) - #7

Merged
sndre merged 9 commits into
mainfrom
feat/layer1-inflight-cancellation
Sep 16, 2026
Merged

feat: cooperative in-flight cancellation checkpoint (opt-in)#7
sndre merged 9 commits into
mainfrom
feat/layer1-inflight-cancellation

Conversation

@sndre

@sndre sndre commented Sep 2, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Opt-in cooperative in-flight cancellation. With FeatureGate.Keys.INFLIGHT_CANCELLATION_CHECKPOINTS enabled, ActionExecutor re-reads the workflow status before each unchecked action and stops with WorkflowCancelledException once it is CANCELLED; WorkflowExecutor settles that as CANCELLED, not ERROR, so no compensation runs. Cooperative: the action already running completes.

Also:

  • FeatureGate.Keys.enabledByDefault, honoured by InMemoryFeatureGate with overrides; the new key and the DISABLE_SIGNAL_PERSISTENCE kill switch are declared off, everything else stays on, so the default SkipperConfig behaves as before.
  • An execution that ends after cancelWorkflow no longer writes its stale version (which lost the optimistic lock, became TRANSIENT_ERROR and replayed a cancelled workflow): checkpoints are kept, nothing is written or rescheduled, and the caller gets the recorded CancelledWorkflow. Gated by the same flag; opted-in apps pay one store read per execution end, others keep today's lock-failure-then-retry path.
  • ActionExecutor loses its 8-arg constructor; subclasses pass the gate. README now declares com.airbnb.skipper.internal.* outside the supported API.

Tests: ActionExecutorTest, WorkflowExecutorTest, WorkflowExecutionTaskHandlerTest, and the SQLite integ test.

@sndre
sndre marked this pull request as ready for review September 2, 2026 20:39
@sndre

sndre commented Sep 2, 2026

Copy link
Copy Markdown
Contributor Author

@joherpec, all your comments from treehouse PR are addressed!

@rgamba, please also take a look! 🙏

@sndre
sndre force-pushed the feat/layer1-inflight-cancellation branch 2 times, most recently from 58e72da to 4702dbd Compare September 9, 2026 03:08
@sndre
sndre force-pushed the feat/layer1-inflight-cancellation branch 2 times, most recently from 5e7abee to 38c9a81 Compare September 14, 2026 18:37
@sndre
sndre force-pushed the feat/layer1-inflight-cancellation branch from 38c9a81 to 0009370 Compare September 15, 2026 20:26
rgamba
rgamba approved these changes Sep 15, 2026
View reviewed changes

@rgamba rgamba left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

Comment thread src/main/java/com/airbnb/skipper/internal/scheduler/WorkflowExecutionTaskHandler.kt Outdated
// so a status write here would only lose the optimistic lock and reschedule a dead
// workflow. Keep the checkpoints, skip the write, and hand the caller the recorded
// cancellation. Not keyed on the result type: the execution may end with any outcome.
val stored: Option<WorkflowInstance>? = workflowStore.getWorkflow(workflowInstance.workflowId)

@rgamba rgamba Sep 15, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a nice to have but it adds one extra read to all the executions and is only useful in the very rare edge cases where the workflow is cancelled mid flight. If we don't do this, the workflow write attempt below will fail with an optimistic locking error, which will trigger a retry, which will then identify that the workflow is already cancelled and stop the execution

@sndre sndre Sep 15, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed on the cost. Gated it behind INFLIGHT_CANCELLATION_CHECKPOINTS in dff74b9: apps that have not opted in keep the lock-failure-then-retry path with no extra read; opted-in apps keep the clean settle, which also preserves this execution's not-yet-persisted checkpoints (the rolled-back write drops them) — that matters for compensate-on-cancel later.

@sndre

sndre commented Sep 15, 2026

Copy link
Copy Markdown
Contributor Author

@rgamba addressed your comment in dff74b9: the cancelled-settle check is now behind INFLIGHT_CANCELLATION_CHECKPOINTS, so apps that have not opted in keep today's lock-failure-then-retry path with no extra read. Rebased on current main, all CI jobs green — ready for another look.

sndre and others added 9 commits September 15, 2026 16:43
@sndre
feat: cooperative in-flight cancellation checkpoint (opt-in)
fcb8e69 
Today a cancel is only observed at scheduling boundaries: a workflow cancelled while one of
its actions is running keeps starting further actions. Add a cooperative check that re-reads
the workflow's current status before starting each not-yet-checkpointed action and, if it is
CANCELLED, stops with a non-retryable WorkflowCancelledException instead of running the next
action. WorkflowExecutor settles that as CANCELLED rather than ERROR: a cancellation is not a
failure, so it does not trigger the compensation chain.

Opt-in and off by default via FeatureGate.INFLIGHT_CANCELLATION_CHECKPOINTS; apps that do not
opt in keep today's behavior byte-for-byte. Backward compatible: the new nullable featureGate
constructor parameter is paired with a non-@Inject 8-arg secondary constructor so existing
callers and subclasses compile unchanged (a second @Inject constructor would break Guice).

Tests: ActionExecutor unit tests for gate-enabled / no-gate / gate-off; WorkflowExecutor
mapping to CANCELLED; an end-to-end SQLite test asserting a mid-flight cancel aborts the run,
settles durably CANCELLED, and runs no compensation.
@sndre
chore: reword a stale comment on the excluded cross-backend prefix test
d3979be 
The comment pointed at a build target that does not exist in this repository. Describe the
exclusion on its own terms: the test needs a multi-backend setup this Gradle build does not
provide.
@sndre
fix: address review on in-flight cancellation checkpoints
77aba54 
- Settle an in-flight-cancelled execution without a stale-version status
  write: keep the completed actions' checkpoints, skip the status write and
  timers, and complete the caller's future with WorkflowCancelledException
  instead of surfacing an optimistic-lock TRANSIENT_ERROR and a doomed
  re-execution.
- FeatureGate.Keys carries enabledByDefault and InMemoryFeatureGate honours
  it (with optional overrides), so INFLIGHT_CANCELLATION_CHECKPOINTS is
  genuinely off under the default SkipperConfig; existing keys unchanged.
- Remove the secondary 8-arg ActionExecutor constructor; test call sites
  pass the gate explicitly.
- Trim the WorkflowCancelledException KDoc and drop implementation-tracking
  labels from comments.
- The integ test asserts the strict end-to-end outcome: precise exception,
  next action never starts, no compensation.
@sndre
docs: declare the supported API boundary
a9bba58 
Everything under com.airbnb.skipper.internal is engine machinery and may
change in any release; the supported surface is the top-level package.
@sndre
fix: settle any execution outcome as cancelled once the store says CA…
e39c1b3 
…NCELLED

The first cut keyed on a CANCELLED execution result, which misses the case CI
hit: cancelWorkflow removes the running task, the next lease-renewal attempt
fails on it and interrupts the worker, and the execution ends TRANSIENT_ERROR.
That completed the caller's future with a TransientError, and the proxy's
polling then surfaced the stored CancelledWorkflow -- while the retry replayed
an already-cancelled workflow.

The settle check now runs for every execution outcome (one store read per
execution end): if the workflow is CANCELLED, keep the completed actions'
checkpoints, skip the status write and timers, and complete the caller's
future with the cancellation cancelWorkflow recorded (CancelledWorkflow), so a
synchronous caller sees the same exception any other observer does.
WorkflowCancelledException stays the signal workflow code sees.

Adds a handler unit test for the path and asserts CancelledWorkflow in the
integ test.
@sndre
docs: trim comments and README to the load-bearing facts
2d407ed
@sndre
test: stub feature gates before the scheduler threads start
4e8e248 
The integ suite stubbed the FeatureGate mock after schedulerManager.start(),
while the fetch loop was already invoking it. Mockito attaches a stub to the
mock's last invocation from any thread, so under load the blanket
isEnabled(any()) stub could land on the wrong invocation and leave gates
false. That is what made the in-flight cancellation test flaky: the check
saw the gate off and let the next action run.

All gate stubbing now happens before start(); suites override specific keys
in customizeDeps. Drops two in-test stubs that were no-ops under the blanket.
@sndre
style: apply spotless formatting
e9efa60
@sndre @claude
fix: gate the cancelled-settle check behind the in-flight cancellatio…
fee2472 
…n flag

Apps that have not opted in keep today's behavior exactly: the stale write
loses the optimistic lock and the retry stops at the start-of-execution gate.
Only opted-in apps pay the one extra store read per execution end.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@sndre
sndre force-pushed the feat/layer1-inflight-cancellation branch from dff74b9 to fee2472 Compare September 15, 2026 23:43
@sndre
sndre merged commit 41a810c into main Sep 16, 2026
10 checks passed
@sndre
sndre deleted the feat/layer1-inflight-cancellation branch September 16, 2026 00:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rgamba rgamba rgamba approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@sndre @rgamba