[rgamba]

Problem

A workflow that starts children does so on its own execution thread, so whatever a child start throws escapes the parent's workflow method. WorkflowExecutor treats any exception it doesn't recognize as a bug in the workflow code and wraps it in a NonRetryableError, so the parent ends in ERROR, which is terminal. Starting a child writes to the workflow store and the scheduler, though, and a timeout on one of those writes isn't a bug. In a large fan-out, one transient blip among thousands of starts fails the whole parent.

Approach

The caller marks the invocation as a child start at the builder, and the factory's proxy acts on that. The engine changes in one narrow place, covered under "Recovering a child that was written but never scheduled" below.

workflowFactory.builder(ChildWorkflow.class, childId)
    .callbackHandler(ChildCallbackHandler.class)
    .asChildOf(this)
    .build()
    .run(input);

• InvocationBuilder.asChildOf(parent) records the parent's id, the same as .parentWorkflowId(parent.id).
• When an invocation carries a parent id, the WorkflowFactory proxy wraps its SkipperEngine.startWorkflow call and sorts out what that call throws:
  1. Already classified by Skipper (SkipperError subclasses, internal errors): rethrown unchanged.
  2. Rejected as invalid (IllegalArgumentException, ValidationError): rethrown unchanged, so it still fails the parent. Retrying would only repeat it.
  3. Anything else (store or scheduler timeouts, dropped connections): becomes an InternalError, the error Skipper's SQLite and MySQL stores and schedulers already throw when a statement fails. The task handler retries the parent after taskUnexpectedErrorRetryDelay, and the parent starts the child again under the same id. Children it already started aren't duplicated.

Recovering a child that was written but never scheduled

If the store commits the child's instance and then the scheduler fails to insert its task, the retried start finds an existing instance. CREATE_EXISTING_WORKFLOW_IS_NOOP is on by default, so before this change the start returned that instance without scheduling it, and the child never ran. SkipperEngine.startWorkflow now schedules an existing RUNNING instance that has no task, even when starting an existing workflow is otherwise a no-op.

RUNNING with no task means a start that failed, or one that hasn't reached its schedule yet. The store writes new instances as RUNNING, and the task handler persists the next status before the scheduler removes the task. This applies to any repeated start, not only children. It costs one getTask read per repeated start of a RUNNING workflow; WAITING and terminal instances aren't looked up.

Because the first start may only be slow, the recovering start writes just the persistent task, due once a lease has passed (SchedulerExecutionQueue.scheduleAfterLease), and queues no in-memory copy. That is the fallback row an in-memory start writes anyway, so whichever start writes it first, the other honours its lease and the workflow runs once. A child that really was stranded runs after schedulerTaskLeaseDuration (8 minutes by default).

Key decisions and trade-offs

• The behavior is inferred from the parent id, not from a new flag. The factory already receives parentWorkflowId on every invocation, so this needs no change to IWorkflowFactory. A new parameter there would break Java implementors and fakes, because the Kotlin interface has no JVM default methods. The consequence is that existing callers of .parentWorkflowId(id) get the new behavior too. A parent id means "this is a child", so that seems right. A caller outside a workflow that sets a parent id would now see an InternalError wrapping the cause instead of the raw exception.
• Only the start is wrapped. Validation, building the input, and waiting for the child's result all happen outside the wrapper, so a bug there still fails the parent, and the child's own failures reach the parent as before. This is narrower than wrapping the whole call site.
• Reuse the existing retry path. Throwing InternalError instead of a RetryableError with its own delay means a child-start failure retries exactly like a store failure anywhere else in the parent's execution. It needs no new public constant, and the delay stays configurable through taskUnexpectedErrorRetryDelay. Like that path, retries stop at schedulerTaskMaxRetries (100), leaving the parent in TRANSIENT_ERROR for an admin retry. That includes programming errors from a store, which before ended in ERROR. Deferring to the ExceptionClassifier was considered and rejected: the default one only retries timeouts, so a store's own rate-limit error would still fail the parent.
• This replaces the engine-level approach in fix: retry the parent when a child workflow start hits a transient store or scheduler failure #44.

Testing

• Integration test (ChildWorkflowStartFailureTest): real SQLite store and scheduler, with a fault injected on the first store or scheduler call for the second of three children.
  • Scheduler timeout scheduling a child: the parent is retried and COMPLETES; all children COMPLETE with parentWorkflowId recorded.
  • The same with CREATE_EXISTING_WORKFLOW_IS_NOOP on (the default; TestRuntime mocks gates off): the stranded child is scheduled and COMPLETES.
  • OptimisticLockingError from the store: rethrown unchanged and retried on its own delay.
  • Store timeout creating a child: same.
  • Invalid child request (IllegalArgumentException): the parent ends in ERROR and isn't retried.
  • The same timeout without asChildOf: the parent ends in ERROR. This is the baseline showing asChildOf is what makes the difference.
• Unit tests (SkipperEngineTest): with the no-op gate on, an existing RUNNING instance with no task is scheduled after a lease. One with a task isn't. A WAITING one isn't looked up.
• Interleaving tests (QueueBasedSchedulerTest): two queues share one scheduler, standing in for two nodes, and an in-memory start and an after-lease recovery write the same task in both orders. Each time only one copy can run, and the persistent row isn't due while it does. Dropping the lease delay fails them.
• Formal verification (formal/tla):
  • The TLA+ model now has concurrent startWorkflow callers and a NoOverlappingRuns invariant.
  • start_race (two in-process starts, either of which may fail) passes.
  • start_race_unguarded (the stranded start scheduled like any other) makes TLC find the double run.
  • start_retry, the stranded-start finding, now passes.
  • Trace validation now records ChildWorkflowStartFailureTest. Locally, all 110 in-scope runs are accepted and all 540 mutants rejected.
  • With the engine guard reverted, the stranded child's recorded run is rejected.
• Unit tests (WorkflowFactoryTest, mocked engine):
  • An infrastructure failure becomes an InternalError with the original cause, and the request carries the parent id.
  • NonRetryableError, RetryableError, InternalError, IllegalArgumentException and ValidationError are rethrown as the same instance.
  • An invocation without a parent rethrows its failure unchanged.
• Mutation checks:
  • Disabling the parent-id check makes the retry integration tests and the retry unit test fail.
  • Disabling the stranded-instance check makes the no-op-gate integration test and its engine unit test fail.
• Local runs: ./gradlew :test passes 933 of 996 tests (3 skipped). The other 60 are the MySQL suites, which need an embedded MariaDB that doesn't run locally; none touch this change. spotlessCheck is clean.
• Docs: new "Starting child workflows" section in Invoking Workflows, plus a builder reference row.

[rgamba]

skippererror could also be retryable error? in which case we'd like to treat it as retryable?

[rgamba]

Yes, and it already is. RetryableError extends SkipperError, so case 1 rethrows it unchanged. From there WorkflowExecutor.handleExecutionError puts the parent in TRANSIENT_ERROR and reruns it on the error's own delay or strategy. Only errors Skipper doesn't recognize get rewrapped, so a NonRetryableError still fails the parent and a RetryableError still retries it.

Done: in b60c9d9 I made this explicit in the KDoc (case 1 now names RetryableError and OptimisticLockingError as examples). I also added retryableErrorWhileCreatingAChildRetriesTheParent, which injects an OptimisticLockingError from the store while starting a child and asserts the parent retries and completes. That test passes with or without asChildOf, since this passthrough is existing executor behavior; it pins the behavior so it doesn't change. The unit test testChildStartClassifiedOrInvalidFailureIsRethrownUnchanged checks that the same instance is rethrown.

• Ricardo's agent

[sndre]

Two things inline: the first can leave a child that never runs on a default setup; the second is about reusing the existing retry path.