[Fix #1749] Publish onWorkflowCancelled before cancelling pending futures

[edeandrea]

Many thanks for submitting your Pull Request ❤️!

What this PR does / why we need it:

Fixes #1749.

This bug shows up when cancel() / cancelFuture() is called while an instance is WAITING on a listen task: metadata can be cleared before onWorkflowCancelled runs. There are also two related races: late listen callbacks can still interfere with cancellation, and terminal transitions can still publish onWorkflowCompleted / onWorkflowFailed after cancellation.

This PR fixes those cancellation races by:

• Keeping metadata alive through onWorkflowCancelled, then clearing it after that callback completes.
• Preventing late callbacks from moving a cancelled instance back out of CANCELLED.
• Failing terminal transitions on a cancelled instance with CancellationException, so cancelled workflows do not publish completion/failure events.

Special notes for reviewers:

• This is the smaller API-preserving fix: onWorkflowCancelled remains the terminal callback; there is no new beforeCancelled event.
• The regression coverage exercises both cancellation APIs (cancel() and cancelFuture()), both workflow shapes (listen and wait), and the terminal-transition races.

Additional information (if needed):

mvn -pl impl -amd install passes locally. SchedulerTest.testAfter (200 ms await) is flaky on main independently of this change: it failed 1 of 5 runs on unmodified main locally.

[Copilot]

🟡 Changes recommended

Incoming listen events during asynchronous publication can still overwrite cancellation status or clear metadata before cancellation listeners run.

2 open findings

• Late listen callbacks revive instances after cancellation · New
• Listen completion cleans up metadata before cancellation publication · New

What changed in this PR

Addresses #1749 by publishing workflow cancellation events before cancelling registered futures, preserving metadata for cancellation listeners.

Changes:

• Separates cancellation state changes from future cancellation.
• Adds metadata lifecycle tests for both cancellation APIs with listen and wait workflows.

File	Description
impl/​test/​src/​test/​java/​io/​serverlessworkflow/​impl/​test/​CancelMetadataTest.java	Tests metadata availability during cancellation callbacks.
impl/​core/​src/​main/​java/​io/​serverlessworkflow/​impl/​WorkflowMutableInstance.java	Defers future cancellation until lifecycle publication finishes.

🧠 Review effort: Balanced

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟡 Changes recommended

Rejected terminal transitions still allow conflicting lifecycle events after cancellation.

1 open finding

• Rejected terminal transitions still publish completion or failure events · New

2 resolved since last review

• Late listen callbacks revive instances after cancellation
• Listen completion cleans up metadata before cancellation publication

🧠 Review effort: Balanced

[Copilot]

🟢 Approval recommended

The changes address the reported cancellation races with targeted regression coverage and no unresolved blocking findings.

0 open findings

3 resolved since last review

• Rejected terminal transitions still publish completion or failure events
• Late listen callbacks revive instances after cancellation
• Listen completion cleans up metadata before cancellation publication

🧠 Review effort: Balanced

[fjtirado]

@edeandrea
I think cancel event should be triggered after the workflow is actually cancelled.
Whats the use case for a metadata associated to the process intance once the process instance is gone?

[fjtirado]

I think onWorkflowCancelled should state as it is, after workflow is cancelled.
It seems there is a valid user case for "beforeCancelled", so, a new event should be, to be triggered before the workflow is cancelled.

[edeandrea]

The intent isn’t to keep metadata around after the instance is logically gone; it’s to keep it available until onWorkflowCancelled finishes.

Listeners often use addMetadataIfAbsent / findMetadata for per-instance bookkeeping that must be cleaned up or finalized in the terminal event itself, e.g. ending an OpenTelemetry span, flushing buffered state, or closing an AutoCloseable created on onWorkflowStarted.

In this bug, cleanUp() runs before onWorkflowCancelled, so that final callback can’t see its own state anymore. The fix keeps metadata alive only through the lifecycle callback, then clears it right after.

[fjtirado]

Ok, so rather than chanigng the order of hte listeners, why not postponing the metadata clearance till the workflow is actually cancelled?
That will the two possible valid solutions on my opinion:

1. Do not clear the metadata till the listeners has completed
2. Add a new listener "beforeCancelled"

I prefer the first one and I think is easily achievable with the previous strcuture.

[edeandrea]

@fjtirado I agree onWorkflowCancelled should remain the terminal event after cancellation. The bug here is just that listener-owned metadata is cleared before that terminal callback runs, so listeners cannot finish their own cleanup. This PR keeps metadata alive through onWorkflowCancelled and clears it immediately after. If we ever want a distinct pre-cancel hook, that should be a separate API change.

[edeandrea]

@fjtirado Yes — that is the first option I’m aiming for. The goal is not to reorder listeners, but to keep metadata alive until onWorkflowCancelled has finished, and only then clear it. I agree a separate beforeCancelled hook would be a different API change; for this PR I prefer the smaller fix that preserves the current listener model.

[edeandrea]

This comment summarizes the thread and addresses the review points raised so far.

I think onWorkflowCancelled should stay as it is, after the workflow is cancelled.
It seems there is a valid user case for "beforeCancelled", so, a new event should be triggered before the workflow is cancelled.

Agreed that onWorkflowCancelled should remain the terminal callback. This PR does not add a new beforeCancelled event. It keeps listener-owned metadata alive only until onWorkflowCancelled completes, then clears it immediately after.

Ok, so rather than changing the order of the listeners, why not postponing the metadata clearance till the workflow is actually cancelled?

That is the approach this PR takes. The bug was that cleanUp() ran before onWorkflowCancelled, so listeners lost access to their own per-instance state too early.

Deferring cancellation leaves listen subscriptions active while an asynchronous lifecycle listener is pending.

Fixed by preventing late listen callbacks from reviving a cancelled instance or moving it back out of CANCELLED.

Delaying cancel(true) does not prevent the listen future from completing normally during cancellation publication.

Fixed by deferring cleanup until cancellation publication finishes, so metadata is still available when onWorkflowCancelled runs.

Returning a normally completed false future rejects the status change but does not stop terminal-event publication.

Fixed by making terminal transitions on a cancelled instance fail with CancellationException instead of publishing onWorkflowCompleted / onWorkflowFailed.

So the current behavior is: keep the existing listener model, preserve metadata through the terminal cancellation callback, and close the cancellation races around late listen callbacks and terminal transitions.

[fjtirado]

Ok, the PR description makes me thing you were changing the order of the listeners.
Ill review the changes tomorrow, but I feel too much is changed to just achieve that goal.

[edeandrea]

Ok, the PR description makes me thing you were changing the order of the listeners. Ill review the changes tomorrow, but I feel too much is changed to just achieve that goal.

Fair point — the PR description is misleading if it sounds like this is only reordering listeners. The actual fix keeps metadata alive through onWorkflowCancelled, prevents late callbacks from reviving a cancelled instance, and makes terminal transitions on a cancelled instance fail instead of publishing completion/failure. I’ll update the description to reflect that broader cancellation behavior change.

[fjtirado]

I think we are addressing several issues with the same PR (which is not necessarily bad)

1. Workflow information getting clear before the cancel listener has completed (that I had originally doubt about being really an issue, but OTEL demostrate it is)
2. Workflow completed listener getting invoked for a cancelled instance
3. An event arriving while the cancel callback is executed will change the status from cancel to another status.

Im not sure 2) and 3) were really there before the change to 1). (Im pretty sure workflow failed was not invoked once canclled, but not so sure about workflow completed) so, what Im going to do is take your unit test, ran it with previous version (which should fail, a least for some of them) and try to make all test work starting from current state. Im doing that because the changes are not trivial, they affect core behaviour and, to be honest, the one in status looks too specific and the one for cleanup too verbose (a handle and a compoese where a compose should be enough)

[fjtirado]

Btw, thanks a lot for detecting the issue and proposing solution. Ill probably send a PR over your PR tomorrow (too late for me now)

[edeandrea]

I think we are addressing several issues with the same PR (which is not necessarily bad)

I agree and think you are right. The other issues are actually issues Copilot pointed out in its reviews (see #1751 (review) & #1751 (review))

1. Workflow information getting clear before the cancel listener has completed (that I had originally doubt about being really an issue, but OTEL demostrate it is)
2. Workflow completed listener getting invoked for a cancelled instance
3. An event arriving while the cancel callback is executed will change the status from cancel to another status.

Im not sure 2) and 3) were really there before the change to 1). (Im pretty sure workflow failed was not invoked once canclled, but not so sure about workflow completed) so, what Im going to do is take your unit test, ran it with previous version (which should fail, a least for some of them) and try to make all test work starting from current state. Im doing that because the changes are not trivial, they affect core behaviour and, to be honest, the one in status looks too specific and the one for cleanup too verbose (a handle and a compoese where a compose should be enough)

The problem I really care about (yes, I'm selfish :) ) is that if I call cancel() on a WAITING instance, it clears the instance metadata before onWorkflowCancelled was published. I need the metadata to survive inside onWorkflowCancelled.

Thats what I documented in #1749 and also what I worked around in quarkiverse/quarkus-flow#1059

The other option could be to just publish from the pipeline rather than publishing before cancelling the futures: have handleException publish onWorkflowCancelled when it sees a CancellationException, so it runs before whenComplete(this::cleanUp) on every path, and stop publishing it from cancel() / cancelFuture(). That also covers the RUNNING path consistently, but cancelFuture() would then need to complete only once the pipeline has published.

[fjtirado]

@edeandrea
Thats good, because I just confirmed that the two other issue were a side effect of the proposed solution.
So, tomorrow, Ill add commit with my proposal to fix the original issue and your selfishness and mine will be fully satisfied ;)

[Copilot]

🟡 Changes recommended

Unresolved listener-tracking races, exposed cleanup futures, binary compatibility changes, and ineffective race tests prevent approval.

4 open findings

• Preserve cancelCheck(TaskContext) JVM signature · New
• Protect out-of-order listener registration and cleanup from races · New
• Do not expose mutable publication future to callers · New
• Cancellation forces listen completion before late callback execution · New

4 resolved since last review

• Late cancellation incorrectly allows successful completion
• Finalization failure is misreported as cancellation
• Cancellation cleanup races with callback metadata use
• Completion callback suppressed after prior COMPLETED transition

🧠 Review effort: Balanced

[Copilot]

🔵 Needs a closer look

Cancellation ordering and race-test effectiveness need human confirmation before approval.

3 open findings

• Protect out-of-order listener registration and cleanup from races
• Preserve cancelCheck(TaskContext) JVM signature
• Cancellation forces listen completion before late callback execution

1 resolved since last review

• Do not expose mutable publication future to callers

🧠 Review effort: Balanced

[Copilot]

🟡 Changes recommended

Unsynchronized listener tracking can lose pending publications and allow premature metadata cleanup.

2 open findings

• Unsynchronized listener list races with execution cleanup · New
• Cancellation forces listen completion before late callback execution

2 resolved since last review

• Protect out-of-order listener registration and cleanup from races
• Preserve cancelCheck(TaskContext) JVM signature

🧠 Review effort: Balanced

[Copilot]

🟡 Changes recommended

cancel() can now throw after accepting cancellation when a listener throws synchronously.

2 open findings

• Preserve cancel success despite synchronous listener failures · New
• Cancellation forces listen completion before late callback execution

1 resolved since last review

• Unsynchronized listener list races with execution cleanup

🧠 Review effort: Balanced

[Copilot]

🟡 Changes recommended

Tracked listener failures can replace workflow outcomes, and completed publication futures accumulate until workflow termination.

2 open findings

• Preserve workflow result when publication cleanup fails · New
• Remove completed publications to prevent listener history growth · New

2 resolved since last review

• Preserve cancel success despite synchronous listener failures
• Cancellation forces listen completion before late callback execution

🧠 Review effort: Balanced

[Copilot]

🔵 Needs a closer look

Timing-sensitive coordination between cancellation, lifecycle callbacks, and cleanup requires final human validation.

0 open findings

2 resolved since last review

• Preserve workflow result when publication cleanup fails
• Remove completed publications to prevent listener history growth

🧠 Review effort: Balanced

[fjtirado]

@edeandrea
At the end I basically followed the same approach than you with a subttle difference. To avoid side effects because cancel delay, the workflow does not wait for the listener to complete, it cancels the workflow right away. It still waits in the clean up for the listener to complete, achieving the desired effect.
There was an additional issue with reviving that, after some trials, I left basically as you devised (a safeguard in status method)

[edeandrea]

Thanks @fjtirado for the work!