Engine correctness: state that can be lost, resurrected or stranded

Problem

Correctness defects in the engine and its collaborators, each with a concrete wrong outcome rather than a style concern: work that is silently undone, a token that comes back from the dead, a run that can never finish, and a recovery path that guarantees the failure it exists to prevent.

17 findings: 12 high, 3 med, 2 low.

High severity

The attempts reset is an unguarded full-entity save, and can resurrect a token another actor moved on

Where: src/WorkflowExecutor.php:458-460

This is the one contended write in the class that is still a load-then-save. It runs in advanceQueued() after advance() has committed its transaction and released the token lock in its finally, and it saves the whole $token entity, state included, from an object loaded before the advance. StateTransitions' own class docblock states the law it breaks ("Never trust a re-read to observe a concurrent commit; guard the write instead"), and getTransition() documents its extra-columns argument as taking exactly this column ("e.g. attempts on a token"), which is how the sibling failure path at :505 writes it.

Strict mode does not guarantee "an entry per fact": on the non-transactional cancel path the fact is already committed

Where: modules/orchestra_audit_trail/src/EventSubscriber/OrchestraAuditTrailSubscriber.php:173-175; the path is modules/orchestra_vbo/src/Plugin/Action/CancelToken.php:31 to WorkflowExecutor::cancel() (src/WorkflowExecutor.php:857-873, no startTransaction()) to the token save at :905 to recordToken('canceled') at :918.

The shipped comment and the settings form promise that strict mode "aborts the transition" so no fact stands without its row. That holds only where the emitter runs inside a transaction. cancel() opens none, so the CANCELED token save is committed before the record is dispatched. The same is true of checkCompletion() (:1462-:1500), which flips the instance to COMPLETED and only then records it.

moderation_transition acts on the default revision, so a pending revision (the reviewer's own edits) is ignored and stale content is published

Where: modules/orchestra_content_moderation/src/Plugin/TaskType/ModerationTransition.php:101 and :114

AttachmentManager::loadTarget() resolves the target with load($id), which returns the default revision, never the latest. For a moderated entity whose default revision is published, a non-default-revision state saved by the preceding entity_form step becomes a pending revision while the default revision still holds pre-review content. Setting the moderation state on that stale default revision and saving creates a new default revision carrying the old field values, and the reviewer's pending revision stays non-default forever. Core's own equivalent operates on the latest revision.

The task signal route is the only completion entry point in the codebase that does not catch the documented InvalidArgumentException, so a stale outcome is a 500

Where: modules/orchestra_interaction_task/src/Controller/InteractionTaskController.php:158

WorkItemManager::complete() documents that it throws when the outcome is not one the node configures, and reaches Completion::fromConfig(), which throws at src/Completion.php:85. Nothing on this path catches it. Every other resume or complete entry point does: InboxController, UserTaskForm, UserOperationForm, InstanceListForm, OrchestraUiController, CommentForm, CompleteTask, the webform handler, and decisively this controller's own pull twin InteractionOperationController::signal():105-113, which wraps the identical call and comments "treat as no longer available rather than a 500". Nine catch sites, one hole, and the hole is in the identity twin of the file that explains why the catch exists.

OperationResumer::resume() returns unconditional TRUE, breaking the documented idempotence contract

Where: modules/orchestra_interaction_operation/src/Gateway/OperationResumer.php:55-68

InteractionResumerInterface::resume() documents that resuming is idempotent and returns FALSE when the step was not still actionable, so a caller can skip its side effects. The engine can honour that: WorkflowExecutor::signal() returns the result of its guarded PARKED to ACTIVE claim and says "Callers that run completion side effects must gate them on this, so a lost race does not fire them twice". That bool is discarded twice on this path: ProcessControlInterface::signal() is declared void, and OutcomeSignaler::signal() never consults it. So this method reports success for a resume that did nothing, and the PARKED pre-check at line 56 is read-then-write, not a claim. Both siblings do it correctly.

DomainTenantResolver never defers: on the CLI, cron, or any unregistered host it resolves the default domain record's tenant

Where: modules/orchestra_domain/src/DomainTenantResolver.php:61-68, claim at :16-19

DomainNegotiator::getActiveDomain() does not return the domain that was negotiated; when nothing has negotiated yet it runs negotiation, and DomainResolver::resolveDomain() ends with an unconditional fallback to the default domain record. So it is effectively never NULL on a site with any domain record: the not-a-domain branch is unreachable, the documented "defers by returning NULL" never happens, and the resolver silently answers with whatever tenant the default domain record is bound to. The multi-tenancy boundary fails open instead of closed.

Composite-condition Add and Remove buttons share one name across flows, so a click lands on the wrong flow

Where: modules/orchestra_cm/src/ConditionTreeBuilder.php:298 and :309

Every other hand-named button in this module scopes its name (apply_node_id, remove_flow_id, remove_feature_node_key); these two do not. Two flows that both use a composite condition therefore render two buttons with the same name and the same value. Core resolves the AJAX triggering element by name plus value, so the sub-condition is added to, or removed from, the last flow rather than the one clicked. Both existing tests use a single-flow workflow.

orchestra:migrate-versions --to accepts a version snapshot belonging to a different workflow

Where: src/Drush/Commands/OrchestraVersionCommands.php:52-63 and the manager it trusts, src/WorkflowVersionManager.php:438-484

Nothing on this path compares the target snapshot's own workflow with the one named on the command line, although WorkflowVersionInterface::getWorkflowId() exists and getVersions() already filters snapshots by workflow. The UI path is safe because it derives the target from the instance's own workflow; the Drush command is the only surface taking an operator-supplied version id, and it validates existence only. Running instances can be repinned onto a foreign, possibly another tenant's, definition while their definition field still names the old workflow.

Deleting a workflow never clears its pinned-version pointer in manual mode, so a reused machine name inherits the dead workflow's version

Where: src/Hook/OrchestraWorkflowVersionHooks.php:43-46, calling WorkflowVersionManager::invalidate() (src/WorkflowVersionManager.php:232-239)

The class docblock states the rule unconditionally, "Editing or deleting a workflow must drop that so the next start re-resolves", but the shared invalidate() is gated on auto mode. That gate is correct for an edit, since manual mode must survive edits and its own comment says so, and wrong for a delete: there is no workflow left to keep pinning. The pointer is written nowhere else on this path, and ensureCurrentVersion() returns a non-NULL pointer without checking the snapshot still belongs to the workflow.

A migration remaps node_id but never a waiting token's source_flow, so a synchronizing join can never fire again

Where: src/WorkflowVersionManager.php:507-527, :597-631, :390-410; consumed at src/JoinCoordinator.php:78-96 and src/JoinState.php:77-84

A token's source_flow is a flow id of the version it was created under, and a synchronizing join decides by a strict membership test of the target definition's flow ids against the tokens' stored source_flow. The migration validates and remaps node ids only: there is no flow map, no flow validation in planMigration(), and the preview reports unmapped nodes only. So a migration to a version whose flow ids differ leaves every WAITING token holding an arc id the target version does not have, and the instance hangs with no incident while the preview reports it ready.

A partial retention override silently disables purging for the states it leaves blank, contradicting the form's stated fallback

Where: src/RetentionManager.php:203-231, against src/Form/WorkflowRetentionForm.php:59 and src/Form/TenantRetentionForm.php:59

Precedence is applied per scope, not per state. The mere existence of a rule row puts the workflow (or tenant) into the excluded set, which removes its instances from the tenant branch and from the site-default branch for every state; the workflow branch then no-ops for any state whose age is blank. The class docblock at :178-181 states the opposite, "a per-workflow rule wins, then a per-tenant rule, then the site default", and so does the form.

The dead-letter handler itself throws for an unpinned instance whose workflow is gone: the queue retries for ever and no incident is raised

Where: src/IncidentStore.php:85, reached from src/WorkflowExecutor.php:497; root accessor src/DefinitionResolver.php:83-85 and :167-175

getInstanceDefinition() throws when the instance pins nothing (or its snapshot is gone) and the live workflow is also gone; its docblock only says it "falls back to the live workflow config" and documents no throws. getRetryPolicy() is the very first thing handleAdvanceFailure() does, outside any try, so the failure handler throws out of advanceQueued(): the attempt is never counted, no incident is ever raised, the instance is never failed, and cron re-runs the item for ever. The non-throwing sibling getEffectiveDefinition() exists for exactly this and is not used here.

The rest

One line each: what it is, and where. The failing scenario and the fix for each are carried by its own commit.

• med An unknown timeout action falls back to advancing the run. src/TimeoutSweeper.php:653-668
• med A stale or crafted flow argument on the trace's Signal action silently advances the token with no outcome and reports success. modules/orchestra_ui/src/Controller/OrchestraUiController.php:463-497
• med PendingActionsFinder::collect() calls the throwing definition accessor uncaught, so one unresolvable instance 500s the whole "My actions" page. src/PendingActionsFinder.php:202
• low QuorumJoin counts votes from arcs it does not count as arrived. src/Plugin/Join/QuorumJoin.php:45-56
• low A subprocess node whose child workflow is gone becomes a pass-through, silently. src/Plugin/TaskType/SubprocessTask.php:86-91 and :110-113

Remaining tasks

• Fix the findings above, one commit per finding where they are independent.
• Add or correct a test for every behaviour change.
• Run every linter and the affected test classes before pushing.

This issue summary was drafted with the assistance of an AI agent (Claude). The analysis and the wording were reviewed by me before posting, and accountability for the content is mine.