A duration a form accepts and the runtime silently discards

Fixes the eight findings in #3620608, plus two fields of the same shape it does not name: a subprocess task's re-launch delay and a payment step's timeout. Every fix has a test seen to fail against unmodified code, bar the one case that belongs to core rather than to this module — an empty join timeout, refused by the field's own #required.

The one shape behind all eight

Duration::getDeadline() answers NULL both for a duration that is absent and for one that is malformed. Every consumer reads NULL as "no deadline", which is almost always a legitimate thing to ask for. So by the time a deadline is computed, an author's typo is indistinguishable from their silence, and it reads as never rather than as a mistake — silently, and with nothing written to say so.

That distinction can only be drawn where the empty case is still known to be deliberate: at the form. So almost every fix here is a validation gap closed at authoring time, and the engine keeps reading an unusable duration as "no deadline" by design. The one exception is deliberate and described below.

Duration::isValid() gives the question a name (it was the open-coded idiom getDeadline(0, $x) === NULL in four places) and records in its docblock what the idiom could not: empty is not valid here, so a caller for which empty is meaningful guards it first — which is exactly what makes the two cases distinguishable.

One behaviour change, on purpose: zero

Writing this up is what exposed it. getDeadline() carried a positive-seconds guard on its numeric branch and none on its interval branch, so 0 yielded NULL while PT0S yielded the anchor — the same request written two ways meant "no deadline ever" and "a deadline that has already passed". I first documented that as a distinction. It is not one; nothing chose it, and no caller reads the two spellings differently on purpose.

Where it actually bit is retention, which inherited the guard: an age of 0 kept instances for ever, while an age of 1 purged everything a second old. So an operator asking to retain nothing got the opposite of what they typed, and the way to get what they wanted was to type 1.

Zero now measures to the anchor in both spellings, and the collector skips only an age it cannot use. Keeping instances for ever is asked for with an empty value — which is what every one of these forms already told the operator.

The kernel test testZeroAgeKeepsNothing pins it, and test-only changes shows it failing against unmodified code.

Retention is where it bit, but it is not the only reader. A stored 0 also stops meaning never and starts meaning immediately for a timer's after, a join's timeout and a relative deadline's duration. It changes nothing for a retry backoff or a subprocess re-launch delay, whose consumers already guard > 0. One reader was relying on the old answer and is fixed here: a payment step's timeout, which its own validation refused as unparseable when it was written 0 and accepts now.

Every form that takes a duration accepts zero, and each of them has a test saying so.

What was unvalidated

  • A timeout join's timeout. The keystone: the modeler built a join's settings subform and submitted it, and never validated it — while the task type and the flow conditions were both validated the same way, so this was an asymmetry rather than a policy. A timeout join could be saved with a timeout nothing can measure with, and then waited for every branch with nothing armed to end the wait: a wait_all wearing the timeout plugin's name. Both an empty and a malformed timeout are refused now, and the refusal is reachable because validateForm() delegates to the routing plugins the way it already delegated to everything else.
  • A timer's after. The per-row scalar validation checked presence and nothing else. A malformed offset makes the alarm's deadline NULL, and the sweep never matches a token with no deadline — so the timer never rings, on the first rung or on a recurring ladder, since the re-arm computes the same NULL. The declarative field contract gains a duration flag, which is what that base already is: subclasses declare, the base enforces.
  • A retry's backoff, in both places. An unmeasurable delay means to the runtime exactly what an empty one means: retry at once. The subprocess task's own re-launch delay is the same defect one module over and is not named in the issue — it had no validation at all, and its coordinator falls back to "now". Fixed in the same pass.
  • The site timeout and the retention ages, at site, workflow and tenant scope. One validator on the trait both forms already share, so the message stays a single string. The per-scope forms validate whether or not the scope is held, because the ages are stored alongside the hold and would outlive it.

One correction to the issue text

There is no per-workflow or per-tenant timeout in this module; those scopes have retention only, so the third finding's heading named a field that does not exist. The summary has been corrected. The finding itself held: the site timeout and the retention ages at all three scopes were unvalidated.

And one misdirection removed

armJoinTimeouts() had three silent skips, and recovery then advised "make the join a timeout join" — advice the operator had already followed, because one of those skips is a timeout join whose timeout is unparseable. The real cause was never named. It says which skip it took now. The forms refuse this state at authoring time, so what still reaches it came from a config import or predates them.

The audit round

An audit of this branch found twelve things, all fixed here rather than deferred.

Two mattered. The explanatory message for an empty join timeout was dead code: core validates required fields before any form-level handler and records only the first error per element, so #required won and the explanation was dropped. Its test passed only because it called validateConfigurationForm() directly, which is a path the real form never takes. And the zero change regressed a payment step, described above.

The new stall warning fired per token on every cron sweep, which is what recoverStalledInstance()'s $warn argument exists to prevent. It is one line per instance now, on the path that warns, and it names an empty timeout too: that was the one skip this issue left silent while fixing its sibling. Arming and the report now read one list, so they cannot disagree about what a candidate is.

Four fixes had no test against a claim that each had one, and six user-facing changes had no documentation. Both are made good. The rest were smaller: a missed occurrence of the idiom isValid() was named for, a guard that did not mirror the loop it said it mirrored exactly, and three comments that claimed more than the code did.

Edited by Frank Mably

Merge request reports

Loading
Loading