Skip to content

[Fix #1247] Adding sync/async implementations#1248

Open
fjtirado wants to merge 1 commit intoserverlessworkflow:mainfrom
fjtirado:#Fix_1247
Open

[Fix #1247] Adding sync/async implementations#1248
fjtirado wants to merge 1 commit intoserverlessworkflow:mainfrom
fjtirado:#Fix_1247

Conversation

@fjtirado
Copy link
Collaborator

Fix #1247

Copilot AI review requested due to automatic review settings March 20, 2026 16:12
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Introduces a PersistenceExecutor abstraction to support synchronous and asynchronous persistence execution paths, and wires these into persistence instance writing to address #1247.

Changes:

  • Added PersistenceExecutor interface plus SyncPersistenceExecutor and async implementations.
  • Introduced TransactedPersistenceInstanceWriter to route persistence operations through a PersistenceExecutor.
  • Updated default persistence handlers/writer to use the new executor model (sync by default, async when configured).

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
impl/persistence/api/src/main/java/io/serverlessworkflow/impl/persistence/TransactedPersistenceInstanceWriter.java Adds a writer base that executes persistence actions via a PersistenceExecutor.
impl/persistence/api/src/main/java/io/serverlessworkflow/impl/persistence/SyncPersistenceExecutor.java Adds a synchronous executor implementation.
impl/persistence/api/src/main/java/io/serverlessworkflow/impl/persistence/PersistenceExecutor.java Defines executor contract with lifecycle hooks for start/delete.
impl/persistence/api/src/main/java/io/serverlessworkflow/impl/persistence/DefaultPersistenceInstanceWriter.java Switches default writer to the new transacted/executor model.
impl/persistence/api/src/main/java/io/serverlessworkflow/impl/persistence/DefaultPersistenceInstanceHandlers.java Wires builder configuration to create sync/async executors.
impl/persistence/api/src/main/java/io/serverlessworkflow/impl/persistence/AsyncPersistenceInstanceWriter.java Refactors prior async writer logic into an async executor implementation.
impl/persistence/api/src/main/java/io/serverlessworkflow/impl/persistence/AsyncPersistenceExecutor.java Adds a concrete async executor that supplies an ExecutorService.
impl/persistence/api/src/main/java/io/serverlessworkflow/impl/persistence/AbstractPersistenceInstanceWriter.java Splits “start” and “complete” hooks from generic transaction execution.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Signed-off-by: fjtirado <ftirados@redhat.com>
Copilot AI review requested due to automatic review settings March 20, 2026 16:24
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (2)

impl/persistence/api/src/main/java/io/serverlessworkflow/impl/persistence/AbstractAsyncPersistenceExecutor.java:61

  • deleteInstance can skip the provided delete runnable entirely when there is no pending future for the instance (it returns a completed future without running runnable). This means completed/failed/aborted may not remove the process instance for workflows that don't enqueue any async writes after startInstance (which is currently synchronous), leaving stale state. Also, canceling the in-flight future can interrupt/cancel queued persistence writes and causes the returned future to complete exceptionally (CancellationException), which defeats the goal of ensuring persistence completes before instance deletion. Consider always executing the runnable, and when a prior future exists, chain the runnable to run after that future settles (without canceling it), returning a future that represents the delete runnable's completion.
    impl/persistence/api/src/main/java/io/serverlessworkflow/impl/persistence/AbstractAsyncPersistenceExecutor.java:75
  • close() waits on each future via future.get(), but it doesn't handle CancellationException. With the current deleteInstance implementation canceling futures, close() can throw at runtime and skip draining/clearing remaining entries. Either avoid canceling the futures in deleteInstance, or handle CancellationException here similarly to ExecutionException to keep shutdown robust.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Blocking persistence writes

3 participants