Report what an action actually did, and stop Hold erasing the retention ages

Five findings from the full file-by-file audit of 1.x at 500e8cbc, all of one class: the code knows what happened and does not pass it on, so somebody is told something untrue or a decision is dropped in silence.

One commit per finding, each with a test proven red without the fix and green with it.

Ticking Hold erased the retention ages it overrides

RetentionOverrideFormBase had the three age fields behind '#states' => ['disabled' => …]. A browser does not submit a disabled input, and core does not fall back to #default_value for a missing one — handleInputElement() writes an explicit NULL into the input and marks it present — so submitForm() filtered all three away and stored the hold alone.

An author freezing a workflow for an audit therefore lost every configured age, and once the hold was lifted three empty fields mean keep this state forever: purging silently off, indistinguishable from having asked for it.

disabled → invisible, which is still submitted. It was the only disabled state in the module; the other seven already use visible/invisible, and SubprocessTask had the correct precedent.

The test is FunctionalJavascript, because #states is client-side: Mink submits every rendered field whatever a state says, so no Functional test could see this.

Three of four operation doorways reported success when the signal did nothing

OutcomeSignaler::signal() returns whether it actually claimed the parked token, and its docblock says a doorway has to pass that on. OperationResumer did. OperationController::signal() and UserOperationForm::submitForm() said "Done.", and OrchestraUiController::signal() said it had set the variable — all three discarding the answer. That last one discarded ProcessControlInterface::signal()'s own boolean at two more sites.

The state each doorway checks first is a read; the claim is what decides. Between them another actor can take the token: the inbox finishing the same work item, a stand-in, a timeout resume action, a second tab, a double click. The operator was told their outcome was recorded when it was not.

The pattern was already written twice in orchestra_ui — OrchestraUiController::resolveIncident() and IncidentResumeForm::submitForm() both check and report — so this brings the three signal doorways along, with the same wording. OrchestraUiController grows a small saySignaled() so its three call sites report once rather than three times.

Each doorway gets a kernel test that reproduces the race by its own mechanism: the row is moved on with a raw write, which is what a concurrent claim is, so the doorway's pre-check passes on a stale read exactly as it does in production.

A cache invalidation that fired when nothing changed

StateTransitions::clearTokenAttempts() invalidated unconditionally, while RawUpdateTrait's own docblock asks every caller to invalidate only on a change, because a write that changed nothing leaves every render built from it still true. Every other caller in the tree already checked the affected-row count.

The count is now named in the WHERE clause as well as the SET, which is what makes that number mean the same thing on every driver: MySQL reports rows changed, so a no-op already answers zero, while SQLite reports rows matched and would answer one. The test caught exactly that — it failed on SQLite against the first version of the fix — so the guard is a property of the query now, not of the driver.

An incident that conflated two causes and gave the advice for one

The incident for a human step whose audience is empty said the node "names an audience that matched nobody" and told the operator to fix the audience. getCandidates() answers with an empty list for two configurations, though: every audience resolved to nobody, or the node names none at all. assignments is not a required key, so a definition arriving by config import can carry the second, which the authoring form refuses — and "fix the audience" sends that author to correct something they never configured.

Same conflation [#3621013] closed for the stalled-join report, at another site.

A refused duration did not say which field it belongs to

RetentionPreviewTrait::validateDurationField() is the shared refusal for every duration these forms take, and it attaches its message with FormState::setError(), which shows what it is given and nothing more. Both callers refuse several fields with that one sentence — four on the site settings form, three on a retention override — so an author was told a duration could not be measured with, and left to work out which of four fields it was.

On an override it is worse than a guess, and the Hold fix above is what makes it so: the three ages are hidden while the scope is held and still validated, deliberately, because they are stored alongside the hold and a typo would otherwise outlive the hold that hid it. So the field an error belongs to need not be on the page to be looked at. The message now names it, read from its own #title.

SettingsFormDurationTest asserted the errors but not what they said. It now reads each field's own #title off the built form and asserts the refusal names it, so a retitled field carries the assertion with it. That test also stopped building a stand-in for each form: it carried only #parents, because that was all validation read, and a hand-built stub answers for a form until the form asks for one more key — then it answers with nothing while the assertion still passes. It builds the real forms and adds only the #parents FormBuilder would have added.

One structural change, which is not a finding

Listed apart from the five because it is not one of them. In WorkflowExecutor::advance() the committed-state re-read under the advance lock ran before the block that releases the lock, and the lock was still freed on both paths the code takes: the early return released it explicitly, the working path in the finally.

The one exit that freed nothing was a throw from that select, which takes a database error — a deadlock, a lock-wait timeout, a dropped connection — rather than anything a run does. Then the lock stood for its full 300 second expiry, and nothing could advance that token, or at a synchronizing join that whole (instance, node). spawn() already had the identical shape written correctly, with its re-read inside its try, which is the evidence this was an oversight rather than a decision.

So the re-read moves inside the try, and the explicit release goes with it, because the finally now covers that return too. Keeping both would not have broken anything — release() deletes the semaphore row for this name and this process's lock id, so a second call deletes nothing — but it would be two paths doing one job, and only one of them also covers a throw.

The test arms the lock decorator to take the lock and then rename the token table away, so the re-read throws — the one window a test cannot otherwise reach. It asserts that release() was reached rather than that the lock is free, because a kernel test's lock is core's NullLockBackend, whose lockMayBeAvailable() answers TRUE whatever has been acquired; asserting the consequence would have passed against the unfixed code and proved nothing.