Problem
One systemic defect with five instances. Several settings take a free-text duration and no validation, while Duration::getDeadline() returns NULL for anything it cannot parse and every consumer reads NULL as "no deadline". So the value saves, the feature silently does nothing, and nothing is logged. The same codebase already validates the identical input in three other places.
8 findings: 5 high, 3 med.
High severity
A timer's after accepts any string; a malformed one makes the timer never fire
Where: src/PluginSequenceFeatureBase.php:276-281 (the only validation of a scalar row field) with src/Plugin/NodeFeature/Timers.php:88-98
The field's own description promises "seconds or an ISO-8601 duration", but the editor accepts anything non-blank. The engine consumes it with Duration::getDeadline(), which returns NULL for anything malformed (src/Duration.php:35-51), and WorkflowExecutor::spawnTimers() stamps that NULL straight onto the timer token (src/WorkflowExecutor.php:1104). TimeoutSweeper only loads tokens whose deadline is at or before now, and its own comment at lines 170-171 says a NULL deadline "is never matched". So the setting saves, the token is created, and the escalation silently never happens. The sibling duration field is validated: RelativeDeadline::validateConfigurationForm() (src/Plugin/DeadlineProvider/RelativeDeadline.php:198-208) refuses exactly this. after and backoff are the only duration fields in the module with no such check.
validateForm() never validates the join/split settings subforms it builds and submits, so a timeout join can be saved with an unparseable timeout and waits for ever
Where: modules/orchestra_cm/src/Form/OrchestraCmForm.php:1425-1491 against :1558-1582
Every other embedded plugin subform this form renders is validated through its PluginFormInterface: task-type settings, node-feature editors and flow conditions all get a validate pass. The routing subforms are built and submitted but never validated, so a routing plugin's rules cannot reach the author, and adding a validator to the plugin alone would be dead code. This is not only latent: TimeoutJoin's own field is a free-text duration (src/Plugin/Join/TimeoutJoin.php:59-64) whose value JoinCoordinator resolves with Duration::getDeadline() (src/JoinCoordinator.php:164-174), and NULL there means no deadline is armed at all.
No duration validation on the site timeout or on the retention ages at any scope; an unparseable value silently means "never"
Where: src/Form/TimeoutSettingsForm.php:75-80, 123-133, 141-153 and src/Form/RetentionOverrideFormBase.php:97-110, 124-140
The schema types these keys as bare string with no constraint, and TimeoutSettingsForm does not use a config target, so ConfigFormBase validates nothing either. At runtime Duration::getDeadline() returns NULL for anything that is neither digits nor a DateInterval spec, and every consumer treats NULL as "no rule": RetentionManager::collect() returns early (src/RetentionManager.php:263-266) and RelativeDeadline::getDeadline() returns NULL. So a mistyped retention age makes retention silently mean "never" while isEnabled(), the dry-run preview and the Drush command all keep reporting normally. The module already has the validation idiom for exactly this field type, so the settings forms are the outlier.
A timeout join whose timeout is empty or malformed waits for ever, silently
Where: src/Plugin/Join/TimeoutJoin.php:51-53 and :58-65
The one thing the timeout join exists for, giving up on a branch that never finishes, is switched off by an author error that nothing rejects, nothing defaults and nothing logs. arrive() then degrades to wait_all: the deadline-passed flag is never TRUE, so it holds until every active arc delivers. Worse, the stall diagnostic then misdirects: recoverStalledInstance() re-parses the same unparseable value, arms nothing, and advises the operator to "make the join a timeout join", which is what they already did.
A timer whose after is malformed leaves the task with no deadline at all
Where: src/Plugin/NodeFeature/Timers.php:88-98
A non-empty but unparseable after creates a PARKED alarm token with a NULL deadline, which the sweep never selects, so the timer never fires and nothing is logged. It is worse than a dead timer, because a node carrying timers is also denied the node-level timeout: DeadlineCalculator::parkDeadline() returns NULL as its first act when the node declares any timers (src/DeadlineCalculator.php:66-69), which the Timeout feature's own docblock states as a design law. So the task ends up with no timeout and no escalation at once.
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 The recurring-timer re-arm parses
afteragain and writes the result straight into its guarded update, so an unparseable value silently ends a recurring reminder mid-series.src/TimeoutSweeper.php:637-641 - med
Retry'sbackoffis unvalidated; a malformed value silently means "retry immediately".src/Plugin/NodeFeature/Retry.php:90-99 - med A malformed site-wide
default_timeoutsilently removes the safety net from every parked task.src/Plugin/DeadlineProvider/RelativeDeadline.php:121-131
Two things the fix settles beyond the eight
Neither is a new finding; both are what closing the eight ran into.
Zero was two different answers. Duration::getDeadline() carried a positive-seconds guard on its numeric branch and none on its interval branch, so 0 answered NULL while PT0S answered the anchor: one request written two ways meant "no deadline ever" and "a deadline that has already passed". Retention inherited it, where an age of 0 kept instances for ever while an age of 1 purged everything a second old. Zero now measures to the anchor in both spellings, and keeping a status for ever is asked for with an empty value, which is what the forms already said.
Two more fields of the same shape. A subprocess task's re-launch delay and a payment step's timeout, neither named above, were unvalidated in the same way; sweeping the family rather than the list found them. The payment timeout also read the old zero: its own validation refused 0 as unparseable and accepts it now, so the window it hands the payment provider is guarded against zero on both of its branches.
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.
Issue fork orchestra-3620608
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:
Comments
Comment #3
mably commentedComment #4
mably commentedComment #6
mably commented