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

Fixes the engine findings in #3620607, one commit per finding, each with a test seen to fail against unmodified code. Rebased onto 1.x after #3620606 merged.

State that could be lost or resurrected

The attempt-count reset was an unguarded full-entity save. It is the one contended write in WorkflowExecutor that was still a load-then-save. It runs after advance() has committed its transaction and released the token's lock in its finally, and it saved the whole entity, state included, from an object loaded before the advance. A concurrent actor that won the advance and consumed the token had its row overwritten with the loser's stale ACTIVE state: a token back from the dead. StateTransitions' own docblock states the law that broke. The count now goes there as a single-column write, beside setInstanceStatus(), which is the shape for writing one column without touching state.

A resume reported success for a resume that did nothing. The engine already answers this: signal() returns the result of its guarded PARKED to ACTIVE claim, and says in its docblock that callers running completion side effects must gate them on it. The answer was thrown away twice on the way out, by a void signature on the engine's own interface and by a signaler that returned the resolved completion instead, so the operation resumer reported an unconditional TRUE. That breaks the documented idempotence: a caller is told its completion happened when a concurrent one won the token, and the side effects gated on the answer, an interaction's onResumed() among them, fire twice for one completion.

A migration remapped node ids but not the arc a token arrived on. A token carries the flow id of the version it was created under, and a synchronizing join tallies the arcs that arrived by comparing those against the target version's own flow ids. A target whose flow ids differ left every waiting token holding an arc that counts toward nothing: the join waits for an arc that can never arrive, the run never finishes, and nothing is written to say why. An arc is identified across versions by the two nodes it connects, read through the same node map the tokens are moved by, so that is what the plan resolves; where the target has no such arc the migration is refused, naming the token and the flow.

Reach and configuration

A domain nothing matched lent its tenant. The resolver's own docblock said a request with no negotiated domain defers, and that branch was unreachable: the negotiator does not report what was negotiated, it negotiates on the first read, and negotiation ends in an unconditional fallback to the site's default domain record. So on the CLI, on cron, or on any unregistered host there was still an active domain, and the resolver answered with whatever tenant that record is bound to — the tenancy boundary failing open. Only a match of exactly "none" is refused, because a domain a caller entered deliberately, which is how a notification is rendered on a tenant's own domain, carries no match type at all and is not a fallback. The existing test was itself passing through that fallback, and now pins the context the way a served request does.

A migration accepted another workflow's version snapshot. The plan validates node ids and nothing else, and two workflows can name a node alike, so a foreign snapshot passed that validation and pinned the runs to a definition that was never theirs. The Drush command is the one surface where the version id is the operator's to name; it validated existence only.

A deleted workflow kept its pinned version in manual mode. The shared invalidate() is gated on auto mode, which is right for an edit and wrong for a deletion: there is no workflow left to pin, and the pointer is keyed by machine name, so a workflow created again under that name inherited the dead one's version.

A partial retention override silently disabled purging. Both override forms say an empty age falls back to the tenant and then to the site default. Precedence was read per scope, so the mere existence of a rule row excluded that scope's instances from the wider branches for every state: a workflow given a completed age stopped having its canceled and failed runs purged at all.

Surfaces

A stale outcome on the task signal route was a 500. A signal link is rendered with the outcome baked into its path, and a workflow can be edited between the render and the click. Completion documents that it throws for an outcome the node does not configure, and nine entry points catch it; this one did not. Its own pull twin, the operation controller, wraps the identical call and says why in a comment.

A composite condition's Add and Remove buttons were shared between flows. Every other hand-named button in the modeler scopes its name; these two did not, so two flows both using a composite rendered two buttons named condition_add_root with the same value. Core resolves the clicked button by name and value, so the click landed on whichever the form built first: a sub-condition added to one flow appeared on the other.

Still to come on this branch

The moderation transition acting on the default revision, then the medium and low findings listed in the issue.

The issue's audit-trail finding is held back deliberately: audit_trail 1.x landed #3620300, which defers a chain write made inside a caller's transaction rather than chaining it under one, so what "strict mode aborts the transition" can mean has changed and the finding needs re-verifying against the new behaviour first.

Edited by Frank Mably

Merge request reports

Loading