Follow-up to #3608215. That issue extracted the shared StatusTransitions primitive and routed the engine's status flips through it, but the further physical split of the recovery and incident logic into their own service classes (IncidentManager, InstanceRecovery, JoinCoordinator) is blocked by a circular dependency: those collaborators call back into the engine's core execution methods (proceed(), checkCompletion(), advance(), armJoinTimeouts()), and the engine would in turn depend on them, which the service container cannot construct.

The clean way

Extract a small core execution service holding the token-advance loop, that both the WorkflowEngine facade and the new collaborators depend on. That breaks the cycle: the collaborators depend on the core service, not on the full engine, and WorkflowEngine becomes a thin orchestrator delegating to the core service plus the collaborators.

Proposed steps

  • Extract the lower-risk self-contained clusters first, independently of the cycle: the timeout and duration scheduling math (parkDeadline, untilDeadline, resolveDuration, the anchor helpers) into a scheduler, and the variable store (getVariables, setVariable, variablesFor) into its own service. These are pure mechanical moves with no callback into the advance loop.
  • Extract a core execution service with the advance loop (advance, advanceNode, proceed, successors, checkCompletion, join and split resolution).
  • Move the recovery sweeps (reconcileStuckInstances, recoverStalledInstances, reconcileTerminalTokens, the stuck-instance finder) into an InstanceRecovery service that depends on the core service.
  • Move the incident actions (raise, retry, resume, skip, cancel, fail, dead-letter) into an IncidentManager that depends on the core service.
  • Move the join coordination (synchronize, armJoinTimeouts, join firing) into a JoinCoordinator.
  • WorkflowEngine keeps only the public API, delegating to the core service and the collaborators.

Constraints

Behavior-preserving throughout: no runtime behavior change, and the full test suite stays green at each extraction. The core execution model (the token and lock discipline, the exactly-once conditional claims, the shared StatusTransitions primitive) is relocated, not changed. This is higher risk than #3608215, so it lands as its own change once the behavioral fixes there are in.

Motivation: WorkflowEngine is a single class of roughly 3,600 lines carrying lifecycle, joins, forks, timers, incidents, dead-letter, recovery, retention, variables, subprocess and migration coordination; this is the path to it becoming a facade over cohesive collaborators.

Issue fork orchestra-3608236

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

mably created an issue. See original summary.

mably’s picture

mably commented

Design note for the extraction, mapped from the current WorkflowEngine (about 55 methods), so the work can proceed from a clear cut.

The blocker is a circular dependency: the recovery and incident collaborators call back into the engine core (proceed(), checkCompletion(), advance(), armJoinTimeouts()), and the engine would in turn depend on them, which the service container cannot construct. The fix is a core execution service that both the facade and the collaborators depend on, so the collaborators depend on that narrow service rather than the whole engine.

The cut: three new classes plus a thinned facade

1. WorkflowExecutor (core execution service) holds the advance loop and the machinery it needs:

  • advance loop: advance, advanceQueued, advanceNode, resolveNode, proceed, successors, liveFlows
  • completion: checkCompletion, claimTerminalStatus, tokensAt
  • join and split resolution: synchronize, applyJoinMerge, tearDownCohort, cohortClosed, armJoinTimeouts
  • token machinery: createToken, loadToken, enqueue, spawn, spawnTimers
  • locking: advanceLockName, acquireJoinLock
  • the timeout math it uses when parking (parkDeadline, resolveDuration, untilDeadline, toTimestamp, applyOffset, nodeTimeoutAnchor, clearNodeTimeoutAnchor, cancelTimers) and the variable reads it uses to evaluate conditions (getVariables, variablesFor, lineageIds, descendantsOf, commonAncestorId)

2. IncidentManager holds the incident lifecycle: handleAdvanceFailure, retryPolicyFor, unrecoverableFailurePolicy, raiseIncident, retryIncident, resumeIncident, claimIncident, resolveOpenIncidents, requeueIncidentToken, skipIncident, cancelIncident, failFromIncident, openIncidentToken, openIncidentCount. It depends on WorkflowExecutor to requeue and re-advance a token.

3. InstanceRecovery holds the recovery sweeps: reconcileStuckInstances, recoverStalledInstances, reconcileTerminalTokens, stuckInstanceQuery, sweepInstances, tokenInstanceColumn, isStalled, recoverStalledInstance. It depends on WorkflowExecutor plus IncidentManager.

4. WorkflowEngine becomes a thin facade: the public API (start, signal, resumeParked, resumeWithResult, cancel, recordResult, spawn, getVariables, setVariable, cancelInstance, failInstance, findInstancesByCorrelationKey, batch, drainSynchronous, and the subprocess resume and relaunch helpers) delegating to WorkflowExecutor and the collaborators, keeping its current method signatures so no caller changes.

Callback surface: the collaborators need only a small slice of the executor (enqueue, advance and advanceQueued, proceed, checkCompletion, armJoinTimeouts, createToken and loadToken, plus the shared status transitions). That slice is the interface the executor exposes; nothing in the executor depends back on the collaborators, which is what breaks the cycle.

Open point: the subprocess helpers (resumeFromCompletedChild, resumeParent, armSubprocessRetry, relaunchDueSubprocess, cancelChildInstances) also touch the advance loop; their placement (executor vs facade) is decided during the move by following the call graph.

Execution order, each step keeping the full engine kernel suite green before the next:

  1. Introduce WorkflowExecutor and move the advance loop and its machinery into it; WorkflowEngine delegates. No behavior change.
  2. Move IncidentManager out onto the executor.
  3. Move InstanceRecovery out onto the executor plus IncidentManager.
  4. Confirm WorkflowEngine is a thin orchestrator and run the whole suite.

mably opened merge request !278

mably’s picture

mably commented
Status: Active » Needs review

Opened MR !278. It extracts three collaborators behind an unchanged public contract: WorkflowExecutor (the core execution service: the advance loop with its retry and dead-letter handling, signal and resume, cancel and fail teardown, join and split resolution, token machinery, locking, timeout math, variable reads and writes, subprocess coordination and the inline drain), IncidentManager (the operator actions resolving open incidents) and InstanceRecovery (the three cron recovery sweeps). WorkflowEngine is now a 290-line facade implementing WorkflowEngineInterface by delegation; every public method signature is unchanged, so no caller changes.

One deliberate deviation from the method cut in the plan comment above, needed to actually break the cycle: the loop-side failure handling (handleAdvanceFailure, retryPolicyFor, raiseIncident, openIncidentCount, resolveOpenIncidents) and the per-instance stall recovery (isStalled, recoverStalledInstance, armJoinTimeouts) stay in the executor, because the advance loop itself calls them: advanceQueued dead-letters, checkCompletion gates on open incidents, and cancel recovers stalls. The collaborators hold only what sits outside the loop, and both depend on the executor, never the reverse.

Extracted in three reviewable commits (executor, then IncidentManager, then InstanceRecovery), each kept green against the full kernel suite. The whole local matrix passes: root and submodule kernel suites on MySQL, the functional suites in Docker, the functional-javascript suites, phpcs, phpstan parity and cspell.

  • mably committed f5ad7878 on 1.x 
    task: #3608236 Extract a core execution service so the recovery and...
mably’s picture

mably commented
Status: Needs review » Fixed

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.