Skip to content

fix: retry the parent when a child workflow start hits a transient store or scheduler failure - #44

Closed
rgamba wants to merge 1 commit into
mainfrom
fix/child-start-transient-retry
Closed

rgamba wants to merge 1 commit into
mainfrom
fix/child-start-transient-retry

Conversation

@rgamba

@rgamba rgamba commented Oct 23, 2026

Copy link
Copy Markdown
Member

Problem

When a workflow method starts a child (workflowFactory.builder(...).build(), then calling the child's @WorkflowMethod), the child's instance is created and its task is scheduled on the parent's execution thread. If the store or scheduler fails transiently there, for example a task insert timing out with java.util.concurrent.TimeoutException, the raw exception escapes the parent's workflow method. WorkflowExecutor.wrapUnexpectedError treats any unclassified exception as a bug in workflow code and turns it into a NonRetryableError. The parent then goes to ERROR, which is terminal, and reExecuteWorkflows won't re-run it. A parent that fans out to many children is very likely to hit one such blip.

Change

SkipperEngine.startWorkflow now runs each of its store and scheduler calls through a small helper, startWorkflowIo. The calls are: create the instance, read an existing instance on a duplicate id, schedule the workflow task, and schedule the timeout task. The helper reports infrastructure failures as InternalError. That is the type the built-in JDBC stores and schedulers already throw for SQL failures, and the task handler already retries it as a transient execution failure. The parent's task is kept and re-run after taskUnexpectedErrorRetryDelay. On retry, children that were already created are found by id and scheduled again, which also recovers a child whose instance committed but whose task insert failed.

These pass through unchanged, so real programming errors still fail the caller:

  • errors the store or scheduler already classified: any SkipperError, InternalError, and EntityAlreadyExists, which the duplicate-id path still handles
  • IllegalArgumentException: requests rejected as invalid, including the serde's "cannot serialize" errors
  • Errors such as linkage or class-loading failures, which the helper doesn't catch
  • validation failures, which are raised in the factory before startWorkflow runs

Trade-offs

  • Where the classification lives. It sits at the I/O boundary in the engine, not in the workflow factory proxy. There is no reliable way to tell whether a proxy call is running inside a workflow execution: parentWorkflowId is optional, and suspend and async workflows change threads. The engine is the one place that knows which calls are store and scheduler I/O.
  • Top-level callers. Callers that start workflows outside a workflow now get InternalError(cause = ...) instead of the raw exception from a custom store or scheduler. With the built-in stores they already got InternalError, so the behavior is now the same whichever store is plugged in. Java callers could not catch a sneaky-thrown checked TimeoutException by type anyway.
  • InternalError vs RetryableError. InternalError uses the operator-configured retry delay for unexpected errors and adds no new retry policy.

Not covered

  • If a child's instance commits and then its task insert fails, and the child was started with createExistingWorkflowIsNoop (or that feature gate is on), the retried start returns the existing instance without scheduling it. This gap existed before this change.
  • Sending signals to other workflows from inside a workflow has the same exposure. It is not changed here.

Testing

New ChildWorkflowStartFailureTest. A parent starts three children (detached, runAsync), and the store or scheduler is wrapped to fail the first call for the second child:

Case Without the fix With the fix
Scheduler throws TimeoutException inserting the child's task parent ERROR (test fails) parent retried, COMPLETED, all 3 children COMPLETED
Store throws TimeoutException creating the child parent ERROR (test fails) parent retried, COMPLETED, all 3 children COMPLETED
Store throws IllegalArgumentException creating the child parent ERROR parent ERROR, not retried

Before the fix, both timeout cases failed with a transient child-start failure must not fail the parent ==> expected: <COMPLETED> but was: <ERROR>.

./gradlew spotlessCheck passes. The root ./gradlew :check passes locally except the MySQL suites (MySqlSchedulerTest, MySqlWorkflowStoreTest, MySqlClusterMembershipManagerTest), which fail in setup because MariaDB4j can't install its embedded database on this machine; they are left to CI.

… store or scheduler failure

Starting a child from inside a workflow method creates the child's instance and schedules its task
on the parent's execution thread. A store or scheduler failure there (for example a task insert
timing out) escaped the parent's workflow method as an unclassified exception, which the executor
treats as a bug in workflow code, so the parent went to ERROR and could not be re-executed.

SkipperEngine.startWorkflow now reports infrastructure failures of its store and scheduler calls as
InternalError, which the task handler retries. Errors the store or scheduler already classified,
and requests rejected as invalid (IllegalArgumentException, which also covers input that cannot be
serialized), are rethrown unchanged and still fail the caller.
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.

1 participant