Engine correctness, and the tests that did not pin it

Fixes the 27 findings filed in #3621296. 24 fixed, 1 withdrawn, 2 delivered as clarity fixes rather than the coverage gaps they were filed as. One commit per finding, so the diff can be read finding by finding.

Engine correctness

Fire timer now on a token waiting at a timeout join did nothing and reported success (F001). fireOne()'s WAITING branch ignored its $due_only parameter and hard-coded deadline <= now, so the operator action — which passes $due_only = FALSE precisely to fire ahead of the deadline — claimed 0 rows and returned after telling the operator "Fired the timer on token N". The claim now takes the due-only bound only on the cron sweep. Threading the parameter alone is not enough: the join gives up on a passed deadline, so an early fire also brings the deadline forward to now in the same write, or arrive() simply tells the token to keep waiting.

A step open to Everyone took the whole request down (F002). matches() guarded the empty-array answer but not the NULL one getCandidates() returns for an audience that restricts nobody, passing NULL into array_intersect(). Any account access-checked for a user_operation or interaction_operation node with the shipped everyone audience got a white screen, because the route access check dies before any output. NULL is now answered TRUE, as isAccountCandidate() already does. The @return string[] that hid it from phpstan is corrected too (F086).

A spawn escalation that fanned out opened no fork cohort (F007). spawn() created its successors with the four-argument createToken(), so multi-branch siblings inherited the parked token's fork id. A timer escalation spawning two branches onto a threshold or timeout join left the loser WAITING for a fire that can never come again — no incident, no log, a stuck run. spawn() now applies proceed()'s rule. Two things the finding did not anticipate, both settled by test rather than by argument:

• No tearDownCohort() exclusion was needed for the still-parked forking token: the descendant walk starts below the root, and isCohortClosed() reads the enclosing cohort rather than the token's own flag.
• Rooting a cohort at a token that stays parked does need something proceed() never faces — the same token fans out again next window, and the closed flag lives on the root. Left alone, every later escalation was born into a closed cohort and self-cancelled silently. A fan-out now re-opens the cohort it is about to populate, which can resurrect nothing: a teardown cancels the stragglers, consumes the joined siblings and moves the winner out. The last assertion of the new test is what caught this.

A composite condition's children were not dependencies (F008). getModules() never descended into settings['conditions'], so a child condition from another module contributed nothing and that module could be uninstalled unopposed. That is worse than the plugin-not-found it looks like: children() skips what it cannot resolve and All::combine([]) answers TRUE, so the gate silently turns always-true and running instances take a branch nobody chose.

A fatal wrapping a retryable cause was retried (F006). No code read FatalExceptionInterface, although it and two documentation pages promise an immediate incident. The chain walk now stops on either marker, whichever it meets first from the outside.

An impossible date became a different deadline (F018). PHP reports a day out of range for its month as a warning, so 2026-02-30 passed toTimestamp()'s guard and rolled forward to 2 March. warning_count now refuses it. Probed across a date, a datetime, a space separator, Z and +02:00, fractional seconds and a real leap day: none warns.

A quorum join with no collect variable decided against, every run (F023) and a Count condition with no variable was unconditionally true for three of six operators (F024). Both refused at authoring time now, the second mirroring the two guards its sibling Comparison already had.

The Roles notify warning could never be shown (F025). The override sat behind isset($form['notify']), but the assignment editor grafts that checkbox on after the plugin's form is built. The wording moves to a buildNotifyToggleElement() override, so it travels with the element the plugin owns.

The reassign timeout never validated its escalation audience (F026). An escalation to a mistyped account saved clean and then did nothing at timeout: getCandidates() answered nobody and onTimeout() returned at its guard, unlogged. The validate now hands the audience its subform through InnerPluginSettingsTrait::validateInnerSettings(), which is how the timeout feature and every plugin-sequence row already express this.

The variables form flattened a declared number or boolean (F031). Only a non-scalar was held read-only, so a workflow imported with value: 3 or value: false came back as "3" and "" after a save — the boolean field even rendered blank. Any stored non-string is now read-only and shown as its JSON.

The Drush migration target was cast, not validated (F045). --to=2x cast to 2 and migrated every running instance onto a version nobody named, reporting success. The sibling retention command's ctype_digit() guard now applies.

Incident::resolve() is deleted (F032): an @api method that bypassed the guarded transition, called by nothing, offering an integrator a resolution that wins no race.

Documentation that said the opposite of the code

getStuckInstanceQuery() claimed ordering plus a range makes a batch advance, which is exactly what ORDER BY id LIMIT n from id 0 does not do — its own caller had to add a cursor for that reason (F091). WorkflowEngineInterface marked EXECUTION_QUEUED as the default while the shipped setting is synchronous (F092); SynchronousExecutionTest's own docblock opened with the same false sentence, and it gains the case that pins the claim — every other case there set the mode explicitly, so nothing checked what a site gets unasked.

Tests

The advance drainer's failure branch had no test at all (F113), although it is what stands between a failing node and the request it runs inside: three controllers drain immediately before redirecting. Two cases now distinguish the two outcomes. Removing the Throwable catch fails the first with the task's own exception escaping drain(); removing the DelayedRequeueException catch fails the second on "not claimable until the backoff has passed".

The docblock-reference sweep silently skipped the classes it exists to check (F132). An unanchored declaration match registered the first bare class/interface word anywhere in the file — including the class docblock's prose — so 75 of 808 files registered a word like "for" or "is", and every reference to those classes went unjudged. The map was also keyed by short name, so the eight names this module declares twice overwrote each other. Now anchored, keyed by namespaced name, resolved the way a reader would (import, then own namespace, then a name declared once; ambiguous is skipped), with a floor assertion so a parser that stops registering declarations cannot report everything as fine. Measured: 105 references met, 26 judged before and 48 now; 748 map keys became 807. Renaming the reference in IncidentManager's docblock now fails the sweep naming file and line; the same edit passes green against the previous version.

Also: the two index tests each claiming to be exhaustive are consolidated into one that really is (F115) — EngineIndexTest is deleted, and the token index's stale justification is corrected against TokenStorageSchema's own comment; EngineOutcomeTest stops shadowing the trait helper whose guard it dropped (F114); the tenant delete form, the one route deliberately using _entity_access, is driven through the router (F133); WorkflowExecutionForm and TenantReadAccessForm, the only two base-module forms with no test of any kind, get one each, asserted through the resolver as well as the stored value (F134); and the base module's first FunctionalJavascript test presses the Variables form's add button in a browser, waiting on the new field rather than sleeping, and asserts the row typed above it survived the rebuild (F135).

Three assertions that could not fail are replaced with ones that can (F116, F117): a state restated by the query that selected on it, a sibling branch fetched after the cancel it is meant to have survived, and two auto-increment ids compared for inequality. The last becomes what it stood in for — the second correlation key resolving to the second instance — and dropping the correlation_key condition from the lookup now fails it.

Withdrawn

F094 — getAvailableInTenant() does not restore the label order. Not reachable. EntityStorageBase::loadMultiple() re-orders its result back into the requested id order ($flipped_ids), so a load cannot return statically cached entities before freshly loaded ones as the finding assumed. Verified on both cores available here, 11.3.16 and 12.0.0-alpha1; only 10.6 lacks that block, and this module requires ^11.3 || ^12. A test written against the finding passed both fixed and unfixed, including with a deliberately partial ConfigFactory cache, which is worse than no test — so nothing changed.

F116 is delivered narrower than filed. The finding says nothing pins the sibling branch's parked state; the helper's own assertNotEmpty does pin it, with a worse message. So this is a clarity fix, not a coverage gap, and the commit body says so.

F114 and F078 have no new test. Both are deduplications with no behaviour change: a shadowed helper deleted, and one firing rule moved into MergingJoinBase so WaitAllJoin and MatchingJoin declare nothing but their names and TimeoutJoin keeps only the deadline short-circuit it adds. The ten join and discriminator classes are the pin.

Verification

• phpcs --standard=Drupal, 1013 files: exit 0.
• phpcs --standard=DrupalPractice, 817 files: exit 0.
• phpstan analyse (cache cleared first): exit 0, no errors.
• Every kernel and unit class touched was run red-to-green locally against SQLite, each finding's fix toggled out and back. Functional and FunctionalJavascript are left to CI.

A failing test-only changes job is the wanted result here: that lane runs the new tests without the fixes.

Audit rounds after the first push

Five hand audits over this branch's own diff, one dimension at a time, until a round found nothing new. Rounds 1 to 4 each found something; round 5 found nothing. Every defect below is mine, introduced by the fixes above, and each carries a test unless there is nothing behavioural to pin.

Round 1 — engine correctness. My own impossible-date fix refused too much: it rejected any value date_parse() warned about, and an ISO-8601 ordinal date (2026-182) and week date (2026-W27-3) both warn and resolve correctly, to 1 July 2026. So a deadline an author had every right to write stopped being accepted, in the change meant to stop a deadline moving silently. date_parse() reports the digits it scanned rather than the date it resolved, so those notations come back as month 1 day 182 and month 1 day 1; the refusal is now a calendar question, with month and day bounds telling a real calendar triple from one of those notations and checkdate() deciding. Probed across 24 formats with no disagreement. Restoring the warning guard fails the new case on "Day 182 of 2026 is 1 July".

Also in round 1, from the StatusHistory correction: the same false premise about loadMultiple() not preserving id order had spread to three more files. InteractionResolver and PinnedPayment keep their loops, which do real filtering, with the reason corrected; InstanceSubmissionLocator had nothing else in its loop, and testLocatorListsSubmissionsNewestFirst already pins the order the simplification leans on.

Round 2 — the authoring surfaces. Two findings.

My Roles notify warning was placed one level too low. What it says is a fact about naming the authenticated or anonymous role, and the getRecipients() that skips them lives on RoleAudienceBase, shared with the audience whose roles come from a process variable — which therefore had exactly the gap the finding described. The override moves to the base, so both audiences carry it and the paragraph is not copied to reach the second one.

And, from auditing the composite-dependency fix: a composite flow condition combined its children with no guard on there being any, and "every child is true" over an empty list is vacuously true. An all with no sub-conditions was an always-taken gate, and so was one whose every child named a plugin the site can no longer resolve, which is the consequence the dependency finding described. Recording the dependency stops the uninstall; this stops the gate lying if the config arrives another way. It costs nothing — the children were already resolved once and the guard reads the list that was built — and it takes nothing from an author who means always, since the condition select offers - None (always taken) - first.

Round 3 — the same defect class elsewhere. The narrowed @return this issue corrected was not the only one: six parameters take the same nullable candidate set, where NULL is how "open to everyone" is spelled, and documented it as string[]. All six guard the NULL correctly; what was wrong is what they promise a reader, and that promise is what made the one place that really did pass NULL into array_intersect() invisible to both a reader and to phpstan. Rather than leave the class to be found by hand again, DocblockNullabilityTest sweeps every shipped method for a @param that omits a null its signature declares — the one question no linter here asks, since phpcs checks an annotation is present and shaped like a type, and this project's phpstan runs at a level that takes it at its word. At zero across src/ and modules/, with a floor assertion on the method count so a regex that stops matching cannot report nothing and mean it.

Round 4 — duplication in my own additions. That new sweep brought its own directory walk, and GuardedRowUpdateTest already had one answering the same question in the same words. It moves to ShippedSourceFilesTrait and both use it. DocblockReferenceTest keeps its own collection deliberately: it scans a different set, with two extensions, the docs and tests included, and the shipped files at the module root, so sharing would need four parameters and read worse at both call sites.

Round 5 — clean. Verified rather than assumed: that only the timeout join can carry a deadline on a WAITING token, so the brought-forward deadline = $now can only reach a join that reads deadlinePassed; that the cohort re-open cannot resurrect a straggler except beyond MAX_LINEAGE, where the teardown already logs that it is incomplete; that every inner-plugin host now validates its subform; that no other array_intersect() takes an unguarded nullable; that the mechanisms my new comments describe exist as written; and that the shipped example workflow's composites both have children.

Considered and deliberately not changed, with reasons: Comparison and Count flatten a declared number to a string on save too, but every operator absorbs it through loose comparison, so there is no reachable consequence; the two Drush positive-integer guards stay written out, because an extracted helper would have to return both a value and a failure and reads worse at both call sites; and fireNow() still reports success when the advance throws, which is now premature rather than false, since the brought-forward deadline makes the next sweep fire the join — making it exact needs fireNow() to report, which is a signature change beyond this issue.

Verification after the rounds. phpcs --standard=Drupal 1015 files exit 0; --standard=DrupalPractice 819 files exit 0; phpstan no errors on a cleared cache; check-translations.php exit 0; cspell clean of every word this branch adds; 108 unit tests and every touched kernel class green, StatusHistoryTest on both SQLite and MySQL. Rounds 6 to 8. Round 6 was found by CI rather than by reading, and it was mine: the round-4 deduplication adopted GuardedRowUpdateTest's root-wide walk, which is safe only where the checkout holds nothing but the project. drupal.org's CI builds the site inside it, so the walk descended into vendor/ and web/ and read every dependency's docblocks as though this project shipped them, failing the phpunit job on Composer's own code. GuardedRowUpdateTest had been scanning vendor/ there all along; its assertion simply never tripped over anything. The walk is now bounded to src/ and modules/, reproduced both ways locally by putting a vendor/composer/fake/src and a web/core/lib/Fake inside the worktree with a narrowed nullable parameter in each: the old walk fails naming that file, the bounded one passes. 2454 documented methods against the floor of 2000.

Round 7 found one more of mine, in prose: the sweep's docblock said this project's phpstan "runs at a level that takes the annotation's word for it", implying a low one. It runs at level 5, and the level is not the reason — a docblock that narrows a native type is taken as the truth about the parameter at every level, which is exactly how the candidates read as an array and the array_intersect() stayed invisible.

Round 8 re-verified every factual claim the new comments make against the code they describe, verbatim: that loadMultiple() reorders into the requested ids, drops what it could not load and returns keyed by id; that getRecipients() sits above the toggle it explains and matches nobody for the pseudo-roles; that the condition select really offers - None (always taken) - first; that the guard in Reassign::onTimeout() is where the docblock says; and that DocblockReferenceTest scans a genuinely different set. Nothing new.

Pipeline green on the final commit, phpunit included.