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

The full file-by-file audit of 1.x at 500e8cbc found five places where the engine or a surface knows what happened and does not pass it on, so somebody is told something untrue or a decision is dropped in silence. They are filed together because they are one class of defect.

Ticking Hold erases the retention ages it is meant to override

RetentionOverrideFormBase puts the three per-state age fields behind a #states disabled condition on the hold checkbox. A browser does not submit a disabled input, and core does not fall back to the default when one is missing: FormBuilder::handleInputElement() writes an explicit NULL into the input and marks it as present, so the element value becomes NULL rather than its default. submitForm() then filters the three trimmed values to an empty array and stores the hold alone.

So an author who holds a workflow or a tenant for an audit or a litigation freeze loses every age they had configured. When the hold is lifted all three fields are empty, and empty means keep this state forever, so purging never resumes for that scope. That is indistinguishable from having asked for it, which is the exact failure validateDurationField() exists to prevent. The class docblock asserts the opposite, that submitForm() stores the ages alongside the hold so a typo would outlive the hold that hid it.

It is the only disabled state in the module. The other seven all use visible or invisible, and an invisible element is still submitted; SubprocessTask already has the correct precedent on its relaunch delay. Mink ignores #states, which is why no Functional test could see this, so the test is FunctionalJavascript.

A refused duration does not say which field it belongs to

RetentionPreviewTrait::validateDurationField() is the shared refusal for every duration these forms accept, 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 is told a duration cannot 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 fix above is what makes it so. The three ages are hidden while the scope is held, and they are 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: type an unusable age, tick Hold, save, and the form refuses while showing nothing that can be corrected. The message now names the field, read from its own #title.

Three of four operation doorways report 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, because an unconditional success tells a caller its completion happened when it did not. Four doorways call it. OperationResumer in orchestra_interaction_operation returns it. The other three discard it and report success regardless: OperationController::signal() and UserOperationForm::submitForm() both say Done, and OrchestraUiController::signal() says it signaled the token and set the variable. That last one also discards the engine signal boolean at two more sites.

An operator picks an outcome from My actions, or submits the operation form. Between the route parked-state check, or the form own re-check, and the signal, another actor claims the token: the inbox completing the same work item, a stand-in, a timeout resume action, a second browser tab, a double click. The engine correctly does nothing. The operator is told Done and the run has not moved, so their decision is lost with nothing to prompt them to look again.

The pattern is already written twice in the same module. OrchestraUiController::resolveIncident(), thirty lines below signal(), checks the boolean and warns that nothing changed and the run has moved since the page was built. IncidentResumeForm::submitForm() does the same for a resume. The three signal doorways were not brought along.

A cache invalidation that fires when nothing changed

StateTransitions::clearTokenAttempts() calls invalidateCachesAfterRawUpdate() unconditionally, while RawUpdateTrait says in its own docblock to call it only when the update reported a change, because a guard that refused has changed nothing and invalidating then throws away renders that are still true. Every other caller in the tree checks the affected-row count first. MySQL reports zero changed rows for a no-op write of attempts zero, so a clean advance of a token already at zero discards that token tags and the orchestra_token_list tag for nothing.

An incident that conflates two causes and gives the advice for one

The incident raised when a human step audience is empty says the node names an audience that matched nobody, and tells the operator to fix the audience and retry. AssignmentMatcher::getCandidates() answers with an empty array for two different states though: audiences that all resolved to nobody, and a node that declares no assignments at all. The schema marks assignments as not required, so a config import can produce the second, and Assignments::validateEditor() refuses it only in the authoring form. Fix the audience is wrong there, because there is no audience to fix and one has to be added. This is the conflation #3621013: Resolve a step's audience before parking, and raise an incident when it finds nobody fixed for the stalled-join report, at another site.

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 runs before the try and finally that releases the lock, and the lock is still freed on both paths the code takes: the early return releases it explicitly, the working path in the finally. The one exit that frees nothing is 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, and then the lock stands for its full 300 second expiry, with nothing able to advance that token or, at a synchronizing join, that whole instance and node pair. spawn() in the same class already has the identical shape written correctly, with its re-read inside its try, which is what says 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 covers that return too.

Tests

Every one of the five gets a test that pins the defect rather than the helper: a FunctionalJavascript test that ticks Hold and asserts the ages survive the save; a kernel test per doorway that consumes the token first and asserts the operator is warned instead of told Done; a kernel test that reads the cache-tag checksum across a no-op attempts write; a kernel test on a node with no assignments at all asserting the incident names that cause; and, for the refusal, each field own #title read off the built form and asserted to appear in the message, so a retitled field carries the assertion with it rather than pinning a copy of an old label.

That last test also stopped building a stand-in for each form. It carried only #parents, because that was all validation read; validation now reads #title too, and a hand-built stub answers for a form until the form asks for one more key, and then answers with nothing while the assertion still passes. It builds the real forms and adds only the #parents FormBuilder would have added.

The structural change is covered as well, by a kernel test that makes the state read throw under the lock and asserts release() was reached. It asserts that rather than that the next worker can take the lock, because a kernel test lock is core NullLockBackend, whose lockMayBeAvailable() answers TRUE whatever has been acquired, so asserting the consequence would pass against the unfixed code and prove nothing.

AI-Generated: Yes (Claude Code performed the audit that found these, drafted this summary, and wrote the fixes and their tests on the merge request. I reviewed the findings against the source, and ran the tests, before posting.)