Enable workflow restoration for WAITING status#1246
Enable workflow restoration for WAITING status#1246mcruzdev wants to merge 1 commit intoserverlessworkflow:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Enables persisted workflows that are in WAITING (e.g., blocked on listen()/wait) to restore correctly after a restart by persisting WAITING status changes and ensuring restored instances re-enter WAITING rather than being treated like SUSPENDED.
Changes:
- Persist workflow
WAITINGtransitions via a newPersistenceInstanceWriter.statusChanged(...)hook. - Restore persisted
WAITINGworkflows by setting instance status toWAITINGonstart(). - Add a default
statusChanged(...)implementation in the abstract writer to write the persisted status.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| impl/persistence/api/src/main/java/io/serverlessworkflow/impl/persistence/WorkflowPersistenceListener.java | Persists WAITING status transitions on lifecycle status-change events. |
| impl/persistence/api/src/main/java/io/serverlessworkflow/impl/persistence/WorkflowPersistenceInstance.java | On restore/start, sets restored instances to WAITING when persisted status is WAITING. |
| impl/persistence/api/src/main/java/io/serverlessworkflow/impl/persistence/PersistenceInstanceWriter.java | Extends persistence writer API with statusChanged(...). |
| impl/persistence/api/src/main/java/io/serverlessworkflow/impl/persistence/AbstractPersistenceInstanceWriter.java | Implements statusChanged(...) by writing status in a transaction. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
...ence/api/src/main/java/io/serverlessworkflow/impl/persistence/PersistenceInstanceWriter.java
Outdated
Show resolved
Hide resolved
| if (ev.status() == WorkflowStatus.WAITING) { | ||
| // Only persist WAITING for now | ||
| persistenceWriter.statusChanged(ev.workflowContext(), ev.status()); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Persisting WAITING on entry without also clearing/updating the persisted status when the workflow leaves WAITING can leave a stale WAITING status in persistence (e.g., when the workflow transitions WAITING -> RUNNING after an event arrives). This can make restored instances and operational queries report incorrect state. Consider also handling status changes away from WAITING (e.g., if previousStatus == WAITING and new status != WAITING then clear the persisted status, or persist all status changes).
| if (ev.status() == WorkflowStatus.WAITING) { | |
| // Only persist WAITING for now | |
| persistenceWriter.statusChanged(ev.workflowContext(), ev.status()); | |
| } | |
| } | |
| } | |
| // Persist all status changes so a previously persisted WAITING status | |
| // is overwritten when the workflow leaves WAITING. | |
| persistenceWriter.statusChanged(ev.workflowContext(), ev.status()); | |
| } | |
| } |
| public CompletableFuture<WorkflowModel> start() { | ||
| return startExecution( | ||
| () -> { | ||
| if (info.status() == WorkflowStatus.SUSPENDED) { | ||
| internalSuspend(); | ||
| } else if (info.status() == WorkflowStatus.WAITING) { | ||
| status(WorkflowStatus.WAITING); | ||
| } | ||
| }); |
There was a problem hiding this comment.
This PR’s behavior change is meant to ensure restored WAITING workflows re-register event listeners and can resume after restart, but there doesn’t appear to be a test that simulates: persist a workflow while WAITING -> restart application/load from persistence -> publish the awaited CloudEvent -> assert the workflow resumes/completes. Adding an integration test using the existing InMemoryEvents broker + persistence handlers would guard against regressions.
Workflows in WAITING status now properly restore after restart by re-registering event listeners. Modified WorkflowPersistenceInstance to skip internalSuspend() for WAITING status, allowing listen tasks to re-execute and register handlers. Signed-off-by: Matheus Cruz <matheuscruz.dev@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @Override | ||
| public CompletableFuture<Void> statusChanged( | ||
| WorkflowContextData workflowContext, WorkflowStatus status) { | ||
| return doTransaction(t -> t.writeStatus(workflowContext, status), workflowContext); |
There was a problem hiding this comment.
The new statusChanged implementation is a bit harder to read than the surrounding methods due to being on a single long line. Consider wrapping the doTransaction(...) call in the same style as suspended(...)/taskCompleted(...) to keep formatting consistent and improve readability.
| return doTransaction(t -> t.writeStatus(workflowContext, status), workflowContext); | |
| return doTransaction( | |
| t -> t.writeStatus(workflowContext, status), workflowContext); |
|
I do not see the need for this one. |
|
What needs to be debugged is why, when after restore, ListerExecutor is called, the re-registration does not occur, but that wont be fixed with this change |
This pull request introduces support for handling workflow status changes, specifically persisting the
WAITINGstatus, in the workflow persistence layer. It does so by extending the persistence writer and listener interfaces and implementations to handle status change events. The most important changes are grouped below:Persistence API Enhancements:
statusChangedmethod to thePersistenceInstanceWriterinterface, allowing the persistence layer to handle workflow status updates.statusChangedmethod inAbstractPersistenceInstanceWriter, which writes the new status using a transaction.Workflow Status Handling:
WorkflowPersistenceListenerto listen forWorkflowStatusEventand persist theWAITINGstatus via the newstatusChangedmethod. [1] [2]WorkflowPersistenceInstanceto explicitly persist theWAITINGstatus when starting a workflow, ensuring the status is accurately reflected.