Engine correctness, and the tests that did not pin it

Problem/Motivation

State the engine can lose, resurrect or strand; guards that cannot fire; an operator action that reports success and does nothing. Plus the test-suite gaps that let each of them through, because an untested fix is one the next audit rediscovers.

Found by a complete file-by-file audit of the whole module at commit bc205f4f: 1130 tracked files, 1126 read in full, 4 binary assets recorded as skipped. Every linter was already clean at that commit, so none of this is something a tool reports: phpcs Drupal over 1008 files and DrupalPractice over 814 both at zero, PHPStan level 5 with no errors, cspell against the CI configuration over 1061 files with no issues, eslint with no errors, stylelint with no problems, composer validate, and the per-project translation check reporting every string translated in the catalog of the project that ships it.

Findings

High

1. Fire timer now silently does nothing on a token waiting at a timeout join, and reports success (src/TimeoutSweeper.php line 322, correctness). fireOne()'s WAITING branch never consults its $due_only parameter and hard-codes a deadline-at-or-before-now condition on the claim, so the operator fire-now path (which passes $due_only = FALSE precisely to fire ahead of the deadline) claims 0 rows and returns. An operator on the process instance page clicks Fire timer now on a token waiting at a timeout join (the link is offered for exactly that state) and is told 'Fired the timer on token N', but the join is not fired and never will be until the deadline passes on its own. Fix: Thread $due_only into the WAITING claim: apply the deadline condition only when $due_only is TRUE, and isNotNull('deadline') otherwise, keeping the surplus-sibling cleared-deadline guard.
2. AssignmentMatcher::matches() fatals on a step whose audience is Everyone (src/AssignmentMatcher.php line 490, correctness). matches() guards the empty-array answer but not the NULL answer getCandidates() returns for a step open to everyone, passing NULL into array_intersect(), a TypeError on PHP 8. Any user opening or access-checked for a user_operation or interaction_operation node whose assignment row is the shipped 'everyone' audience gets a white screen: the route access check dies before any output. Fix: Return TRUE on a NULL candidate set before the intersect, matching isAccountCandidate() and Everyone's documented semantics.

Medium

1. spawn() fans out without opening a fork cohort, so a downstream early-firing join cannot tear its branches down (src/WorkflowExecutor.php line 1085, correctness). proceed() stamps a new fork-cohort root on every branch of a fan-out, but spawn() creates its successors with the 4-argument createToken() call, so multi-branch spawn siblings inherit the parked token's fork id instead of forming their own cohort. A timer escalation that spawns two or more branches converging on a threshold or timeout join leaves the losing branch WAITING forever after the early fire; the instance never completes and cron's recovery finds nothing to arm, so an operator sees a stuck run with no incident and no log entry. Fix: Give spawn() the same fan-out rule as proceed(), and exclude the still-parked forking token from tearDownCohort()'s straggler set.
2. A composite flow condition's children are not recorded as workflow dependencies (src/WorkflowPluginProviders.php line 131, correctness). getModules() asks the flow-condition manager only about the flow's own condition plugin and never descends into a composite's conditions tree, so a child condition provided by another module contributes no dependency. A site whose workflow gates a flow with all/any over a condition from another module can uninstall that module without the uninstall validator objecting; CompositeConditionBase::children() then skips the unknown child and the All combiner returns TRUE on an empty set, so the gate silently flips to always-true and running instances take a branch they should not. Fix: Walk the composite tree when collecting flow-condition providers, recursing through each nested condition's plugin.
3. FatalExceptionInterface is never consulted, so a fatal wrapping a retryable cause is retried (src/Exception/FatalExceptionInterface.php line 12, correctness). The interface documents immediate dead-lettering without retry, but no code reads FatalExceptionInterface, and WorkflowExecutor::isRetryable() walks the whole cause chain, so a FatalException wrapping a retryable cause is retried. A task author who rethrows an unrecoverable failure as a FatalException carrying the original as its previous exception sees the engine burn the node's whole retry policy instead of raising the incident at once that the interface and two documentation pages promise. Fix: Return FALSE from isRetryable() as soon as the chain walk meets a FatalExceptionInterface, checking the thrown exception first.
4. An impossible calendar date is accepted and silently rolled over into a different deadline (src/Duration.php line 114, correctness). toTimestamp() rejects a value only on a non-zero error count or a missing year, but PHP reports an out-of-range day as a warning, so 2026-02-30 passes the guard and DateTimeImmutable rolls it forward. A modeler author who types 2026-02-30 into an absolute deadline gets no validation error and the run's timeout fires on 2 March instead, two days off with nothing saying so. Fix: Also return NULL on a non-zero warning count; a legitimate ISO-8601 datetime produces no warning and the numeric-timestamp path is handled earlier.
5. IncidentInterface::resolve() is an @api method that bypasses the guarded state transition and nothing calls it (src/Entity/Incident.php line 188, correctness). Incident::resolve() flips state from open to resolved with a plain set(), side-stepping StateTransitions::claimState(), the module's documented single primitive for that contended flip; no code or test calls it. An integrator following the @api interface calls resolve() then save(), wins no race, and two concurrent operators both believe they resolved the incident, so the dead-lettered branch is retried, skipped or canceled twice. Fix: Delete resolve() from Incident and IncidentInterface so IncidentStore::claimIncident() is the only way to resolve an incident.
6. A quorum join with no collect variable always decides 'not reachable' (src/Plugin/Join/QuorumJoin.php line 55, correctness). QuorumJoin reads its votes exclusively from the arrived values, which JoinCoordinator populates only when the join's collect setting is non-empty, yet collect is optional and QuorumJoin neither requires nor validates it. An author who configures a quorum join and leaves Collect variable empty gets a workflow that saves cleanly and then fires the 'approval can no longer be reached' branch on every run, routing even a unanimous approval down the rejected path. Fix: Set #required with a #required_error on the collect field, the pattern TimeoutJoin already uses for its own timeout field.
7. A Count condition with an empty variable name is unconditionally true for three of its six operators (src/Plugin/FlowCondition/Count.php line 95, correctness). Count::evaluate() has no guard for an unnamed variable and no validateConfigurationForm(), so the resolver returns NULL, the match count is 0, and the at-most, fewer-than and not-equal operators all hold against a threshold of 1 - the same defect its sibling Comparison was already fixed for. An author who leaves List variable empty and picks 'at most', 'fewer than' or 'not' saves without a word, and every token then takes that outgoing flow. Fix: Give Count the same two guards as Comparison: an early return FALSE for an empty name, and a validateConfigurationForm() that refuses it.
8. The Roles audience's authenticated/anonymous notify warning can never be shown (src/Plugin/Audience/Roles.php line 101, correctness). Roles::buildConfigurationForm() overrides the notify element's description behind an isset() guard, but the notify checkbox is grafted onto the row after the plugin's own form is built, so the guard can never fire. An author assigning a step to the authenticated or anonymous pseudo-role and ticking Notify this audience only sees the generic description; nobody is ever emailed and nothing says why. Fix: Override buildNotifyToggleElement() in Roles and set the description there, so the wording travels with the element the plugin owns.
9. The reassign timeout action never validates the escalation audience's own settings (src/Plugin/TimeoutAction/Reassign.php line 225, correctness). Reassign builds and submits the chosen assignment plugin's subform but its validateConfigurationForm() only checks the selected id is an AssignmentInterface; it never calls the inner plugin's own validate, unlike every other inner-plugin host in the module. An author escalating a timed-out task to a mistyped or deleted account saves with no error; at timeout the candidate set is empty, onTimeout() returns at the nobody-at-the-other-end guard, the task is never escalated and nothing is logged or raised. Fix: Call the selected plugin's validateConfigurationForm() on its subform, mirroring submitConfigurationForm().
10. The variables form flattens a declared number or boolean to a string, contradicting its own docblock (src/Form/WorkflowVariablesForm.php line 169, correctness). row() protects only non-scalar declared values behind #disabled, so an integer, float or boolean initial value is rendered as a text field and written back as a string on save, while the class docblock promises a number or a boolean keeps whatever the imported configuration carries. An administrator who opens the Variables tab of a workflow imported with a numeric or boolean value and presses Save turns the declared 3 into the string '3' and FALSE into an empty string (the field even renders blank), so the run is seeded with a JSON string where the workflow declared a number or a boolean. Fix: Preserve any non-string stored value and render a bool or number read-only like a list, or correct the docblock.
11. The advance drainer's whole failure branch, the one that protects the user's request, has no test (tests/src/Kernel/AdvanceQueueDrainerTest.php line 66, tests). drain() documents four failure behaviours - a delayed requeue, a release-and-stop on RequeueException or SuspendQueueException, and any other Throwable released, logged and swallowed so it never surfaces in the user's request - and the only test class for the drainer exercises none of them. An operator who signals a queued task from the interaction, operation or task controller, each of which drains immediately before redirecting, gets a white screen instead of their redirect, or loses the queued item, if that catch block is ever narrowed - and the suite stays green. Fix: Drive a workflow whose node throws through drain(), asserting the call returns, the item is still queued and the released-for-retry line was logged; a second case with a retry backoff asserts the item is delayed rather than lost.
12. The docblock-reference sweep silently skips 17 percent of the references it exists to check (tests/src/Unit/DocblockReferenceTest.php line 147, tests). getDeclaredTypes() finds a file's type name with an unanchored preg_match, so the first bare occurrence of class, interface, trait or enum anywhere in the file - including the class docblock's prose - is registered instead of the declaration; 75 files register a bogus name, and every comment reference to those classes is skipped without a word. The map is also keyed by short name, so eight colliding classes overwrite one another. A maintainer renaming a method on WorkflowExecutor, ProcessInstanceInterface, WorkItemManagerInterface, AssignmentInterface or TokenInterface leaves the prose pointing at a method that no longer exists and this test still reports green - the exact failure it was written to catch. 11 of the 65 references it meets are skipped although the class is ours. Fix: Anchor the declaration match to the start of a line, add a floor assertion on the type count against the number of files scanned, and key the map by namespaced name.
13. The one tenant route whose access is entity-based is the one RouteAccessTest does not drive (tests/src/Functional/RouteAccessTest.php line 49, tests). The class exists precisely because a mistyped permission or a missing requirement would pass every object-level test, but it drives only the four permission-gated routes and never the tenant delete form, the single route that deliberately uses entity access so the default tenant's delete form is refused before it opens. An administrator following the Delete operation on the default tenant would get the confirm form and then an exception on submit instead of a 403, and nothing in the suite would notice. Fix: Add the default tenant's delete path expecting 403 and another tenant's expecting 200, which pins both the route requirement and the access handler's operation name through the router.

Low

1. AssignmentMatcher::getCandidates() documents a return type it never has (src/AssignmentMatcher.php line 144, documentation). The @return omits the NULL the signature declares and the body returns, on a method of an @api service. A contrib author reading the docblock writes a foreach over the result and gets a fatal the first time a step uses the everyone audience; it is also why phpstan level 5 cannot see the crash above, since the narrowed phpdoc tells it the value is never NULL. Fix: Declare the nullable return with the sentence AssignmentInterface::getCandidates() already uses.
2. getAvailableInTenant() does not restore the label order loadMultiple() can break (src/StatusRepository.php line 48, correctness). The method copies loadMultiple()'s result verbatim instead of walking the sorted ids, and core returns statically cached entities before freshly loaded ones, so the promised label order is lost whenever some of the tenant's statuses are already in the static cache. An author opening the node editor's Status select, or a viewer opening the Views Status exposed filter, sees the options in an arbitrary order instead of the alphabetical order the interface promises. Fix: Walk the sorted ids and pick each entity out of the loaded set, as StatusHistory::getHistory() already does for the same trap.
3. getStuckInstanceQuery()'s docblock claims order-plus-range makes the batch advance; its own caller adds a cursor because it does not (src/InstanceRecovery.php line 392, documentation). The shared base query's docblock states that ordered by id and range-limited, the batch advances deterministically instead of re-loading the same head-of-table rows, which is exactly what an ordered range from id 0 does not do for a shape that keeps matching. Whoever adds a third recovery sweep on this base takes the docblock at its word and ships a sweep that re-reads the same wedged head rows forever, the failure recoverStalledInstances() had to add a state cursor to avoid. Fix: Reword the bullet to say what the ordering actually buys, and state that a sweep whose matches can persist across runs must add its own cursor.
4. The queued execution constant is documented as the default; the shipped default is synchronous (src/WorkflowEngineInterface.php line 35, documentation). The constant's docblock reads 'tokens advance on cron via the queue (the default)', but the shipped settings set the execution mode to synchronous and the resolver falls back to synchronous when the setting is empty. A developer reading the engine's own public contract to decide whether a run needs cron concludes the opposite of what the site does, and the two neighbouring docblocks in the same code path say the reverse. Fix: Move '(the default)' onto the synchronous constant.
5. The synchronizing joins repeat one firing rule three times (src/Plugin/Join/WaitAllJoin.php line 36, duplication). WaitAllJoin::arrive() and MatchingJoin::arrive() are identical bodies and TimeoutJoin::arrive() is the same body with one extra short-circuit; the arc-covering rule is written out three times, and WaitAllJoin's docblock already concedes the two are the same behaviour under two conventional names. A change to the shared rule has to be made in three places. Fix: Put the rule in MergingJoinBase::arrive() and let TimeoutJoin override with the deadline short-circuit before calling the parent.
6. The Drush migration target is cast to int without validation, unlike the sibling command's limit (src/Drush/Commands/OrchestraVersionCommands.php line 53, correctness). The destructive migration target is cast with no check, while the sibling retention command validates its limit with ctype_digit() and a comment explaining why a bare cast is wrong. An operator who mistypes the target has every running instance of the workflow migrated onto the wrong version and is told it succeeded. Fix: Apply the same ctype_digit() guard and return a failure exit code naming the bad value.
7. EngineOutcomeTest re-declares a helper the trait it uses already provides, dropping the trait's guard (tests/src/Kernel/EngineOutcomeTest.php line 317, duplication). The class uses DrainEngineQueueTrait and then defines its own processNextQueueItem(), which shadows the trait's method and silently returns when the queue is empty where the trait asserts the item exists. A maintainer strengthening the shared helper gets no effect here, and a regression where start() enqueues nothing turns a test into a null-method error instead of the trait's named assertion failure. Fix: Delete the private method; the trait's is already in scope and is what every other class calls.
8. Two index tests each claim to be the exhaustive list, and neither is (tests/src/Kernel/RuntimeEntityIndexTest.php line 44, documentation). RuntimeEntityIndexTest says its list is deliberately the whole of what those schemas declare, and EngineIndexTest points maintainers at it for the same reason, yet the token table's tenant-and-state index is asserted only in EngineIndexTest while three assertions are duplicated between the two classes. A maintainer who removes the index the account-wide pending-actions scan needs, and checks the class both docblocks name as canonical, sees nothing missing there. Fix: Move the three assertions into RuntimeEntityIndexTest so one class really is the whole set, and reduce or delete EngineIndexTest.
9. Assertions that restate the query that produced the value (tests/src/Kernel/JoinCancellationTest.php line 63, tests). The value asserted was fetched by a query that already filtered on it, so the assertion cannot fail; the only real check is the assertNotEmpty inside the helper. The same shape appears in TokenCancellationTest. A reader believes the parked state of the sibling branch is pinned at these call sites when nothing there pins it. Fix: Drop the restatement, or assert the state on a token loaded without the state filter.
10. A comparison of two auto-increment ids that cannot fail (tests/src/Kernel/CorrelationLookupTest.php line 84, tests). assertNotSame() compares the ids of two separately saved entities, which are distinct by construction; it is the only use of the second instance, whose correlation key is never looked up. The test reads as if it proved two keyed runs resolve apart, and nothing in it would notice a lookup that never resolved the second key at all. Fix: Assert that the correlation lookup on the second key resolves to the second instance, which is what the line was standing in for.
11. Two base-module admin forms have no test of any kind (tests/src/Functional, tests). WorkflowExecutionForm and TenantReadAccessForm are the only two base-module forms with neither a kernel nor a functional test; every sibling has one. An author switching a workflow to Synchronous writes the execution mode through a path nothing exercises, so a drift between the radio keys and what the resolver and the config schema accept would ship silently and the workflow would keep advancing on cron with the form showing Synchronous. Fix: Extend WorkflowVariablesUiTest with a save of the execution form, and RetentionUiTest with the tenant read-access save.
12. The Variables form's AJAX callback is never exercised, only its no-JS fallback (tests/src/Functional/WorkflowVariablesUiTest.php line 65, tests). submitForm() in a BrowserTestBase posts the button and re-renders the whole page, so the form's AJAX callback and its wrapper pairing are never run; the base module ships no FunctionalJavascript test at all. On a JS-enabled site a broken wrapper id or a callback returning the wrong subtree leaves an author clicking Add another variable with no new row appearing, while this test keeps passing because it takes the non-AJAX path. Fix: Add a FunctionalJavascript test that presses the add button, waits on the new field appearing (never a sleep), and asserts the already-typed row survived the rebuild.

Proposed resolution

One merge request for this issue, one commit per finding, each commit naming the finding it closes. Every defect gets a test that fails against the unfixed code, so the ground is closed and the next audit pass has to look somewhere new.

Remaining tasks

• Fix each finding above, with its test.
• Run every linter the pipeline runs, and the impacted test classes, before pushing.
• Play the next-major lane by hand, since its composer job is manual and a skipped lane reads as green.

User interface changes

None.

API changes

Pre-1.0, so a signature is changed where the better shape needs it rather than preserved; each is named in its finding.

AI-Generated: Yes (Claude Code was used to run this audit and to draft this issue summary. I reviewed the findings against the source myself before posting; the code and tests will follow on the merge request.)