From 4834b9617879f16e29a46b8efe31bc4421b3cd70 Mon Sep 17 00:00:00 2001 From: Ruben van der Linde Date: Sat, 5 Sep 2026 16:57:08 +0200 Subject: [PATCH 1/3] fix(locking): a parked run keeps its locks, and the lock nodes exist in the editor MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three defects in the run-scoped locking that #3444 shipped, all three found by walking a live instance and none by the suite. A RUN LOST EVERY LOCK THE MOMENT IT PARKED. The dispatch predicate really is `isTerminal()` on the persisted row, and the row really did say `completed` — transiently, in the middle of a pass that went on to store `suspended`. `FlowEngine::fireOnStream()` computes `enabledAfter` by asking `FlowStreamWalk::workRemains()` for the transitions enabled on the marking it has just advanced, but that method answers from the walk's in-memory stream picture, which `commitFiring()` only re-reads AFTER the commit. So it compared the NEW enabled transitions against the OLD places and answered "no work remains" at every ordinary mid-flow firing. `applyDerivedStatus()` then took its "nothing enabled, nothing parked, nothing terminal" arm and wrote `completed`, `FlowRunMapper::update()` announced terminality, and the lock listener released the run's locks — every firing, of every run, not just ones that lock. The park path had the mirror bug: `workRemains()` was evaluated before `park()` marked the stream parked, so a parking run was derived `queued` with no wake time until `finalize()` corrected it. `workRemains()` now takes what the caller's commit is about to change: `produced`, the places the firing takes, and `settling`, a stream whose place stops counting because it is parking. Over-counting is the safe direction here — it yields `queued`, which the next pass corrects — and under-counting is what produced a false terminal. A RUN-KIND LOCK REFUSED THE RUN THAT HELD IT. `SaveObject` asked the guard "which user", never "which run", so a flow that locked a case was turned away by its own lock at its next write. The run identity now reaches the guard through the ambient `FlowRunContext`, for the same reason attribution does: the write is routinely several calls deep inside code that has never heard of flows. Absent, it reads as a person, which is the fail-closed answer. The D-6 sweep is re-done over the GUARD's callers, not just `lockObject()`'s: `SaveObject`, `RevertHandler` and `ObjectsController::update` now pass the caller's run; the three post-save auto-unlock tests in the controller deliberately do not, and say so — they decide a RELEASE, and a run's lock must outlive every write the run makes. `LockObjectNode` and `UnlockObjectNode` already passed theirs. The same predicate had the opposite hole: a user lock did not refuse a run executing as its holder, so a run passing over a person's locked object took the extend branch, rewrote the payload as its own run lock, and destroyed the person's lock when it ended. A run and the person it runs as are different holders in both directions. BOTH NODES WERE INVISIBLE IN THE EDITOR. `core/img/actions/lock.svg` and `unlock.svg` do not exist in NC 33 or 34, `imagePath()` throws for an image the server does not ship, and `palette()` caught that with everything else — so the catalogue held 25 nodes rather than 27 and neither node could be added to a flow at all. The icons are now app-owned, and the silent skip is loud: an icon is resolved on its own, an unresolvable one is an ERROR naming the node, and the node is served with the app icon instead of being deleted from the catalogue. A node that survives with the wrong picture beats a node that does not exist. Tests, each proven red first, driving the real engine rather than a fake — the existing coverage mocked `FlowRunMapper::update()` and restated `workRemains()` in a fake, so both agreed with the bug: - a suspended run announces nothing and keeps its locks, and each of `FlowRun::TERMINAL` releases them (iterated from the constant) - the holding run writes to its own locked object; another run and a person are refused - a person's lock survives a run passing over the object, payload byte-identical - every registered node's icon resolves, as a sweep over all 27 rather than a check of these two, plus the palette behaviour that stops the next one vanishing --- img/lock.svg | 1 + img/unlock.svg | 1 + lib/Controller/ObjectsController.php | 57 ++- lib/Db/ObjectEntity.php | 15 + lib/Service/Flow/FlowEngine.php | 21 +- lib/Service/Flow/FlowNodeRegistry.php | 78 +++- lib/Service/Flow/FlowRunContext.php | 34 ++ lib/Service/Flow/FlowStreamWalk.php | 57 ++- lib/Service/Flow/Nodes/LockObjectNode.php | 6 +- lib/Service/Flow/Nodes/UnlockObjectNode.php | 4 +- lib/Service/Object/RevertHandler.php | 39 +- lib/Service/Object/SaveObject.php | 14 +- .../specs/run-scoped-object-locking/spec.md | 69 +++ tests/Fixtures/nextcloud-core-images.txt | 259 ++++++++++++ tests/Unit/Db/ObjectEntityRunLockTest.php | 46 ++ .../Service/Flow/FlowNodePaletteIconsTest.php | 336 +++++++++++++++ .../Flow/RunLockReleaseTerminalityTest.php | 395 ++++++++++++++++++ tests/Unit/Service/Object/SaveObjectTest.php | 124 +++++- 18 files changed, 1533 insertions(+), 23 deletions(-) create mode 100644 img/lock.svg create mode 100644 img/unlock.svg create mode 100644 tests/Fixtures/nextcloud-core-images.txt create mode 100644 tests/Unit/Service/Flow/FlowNodePaletteIconsTest.php create mode 100644 tests/Unit/Service/Flow/RunLockReleaseTerminalityTest.php diff --git a/img/lock.svg b/img/lock.svg new file mode 100644 index 0000000000..5c0eb3f668 --- /dev/null +++ b/img/lock.svg @@ -0,0 +1 @@ + \ No newline at end of file diff --git a/img/unlock.svg b/img/unlock.svg new file mode 100644 index 0000000000..4e0291dad5 --- /dev/null +++ b/img/unlock.svg @@ -0,0 +1 @@ + \ No newline at end of file diff --git a/lib/Controller/ObjectsController.php b/lib/Controller/ObjectsController.php index 96063676d4..add2a3eaf2 100644 --- a/lib/Controller/ObjectsController.php +++ b/lib/Controller/ObjectsController.php @@ -2856,7 +2856,7 @@ public function update( // `lock()` actually writes; the service-layer guard read a key // that never existed. Both now delegate to the same predicate, so // there is one comparison rather than two spellings of it. - if ($existingObject->isLockedBySomeoneElse(userId: $this->container->get('userId')) === true) { + if ($existingObject->isLockedBySomeoneElse(userId: $this->container->get('userId'), runUuid: $this->callerRunUuid()) === true) { // Return a "locked" error naming the holder. return new JSONResponse( data: [ @@ -2923,6 +2923,14 @@ public function update( // lock must survive somebody else's write: without this test // an administrator's write would silently strip a run's lock // as a side effect of a guard it had just passed. + // + // NO `runUuid` HERE, DELIBERATELY, and it is the one guard in + // this file that omits it. This decides a RELEASE, not a + // refusal: asking it as the run would make a run's own write + // drop the run's own lock the moment it saved — the lock is + // meant to outlive every write the run makes. Asked as a + // person, a run-held lock reads as somebody else's and is + // left alone, which is what this test is for. if ($objectEntity->isLocked() === true && $objectEntity->isLockedBySomeoneElse(userId: $this->container->get('userId')) === false ) { @@ -3152,6 +3160,14 @@ public function patch( // lock must survive somebody else's write: without this test // an administrator's write would silently strip a run's lock // as a side effect of a guard it had just passed. + // + // NO `runUuid` HERE, DELIBERATELY, and it is the one guard in + // this file that omits it. This decides a RELEASE, not a + // refusal: asking it as the run would make a run's own write + // drop the run's own lock the moment it saved — the lock is + // meant to outlive every write the run makes. Asked as a + // person, a run-held lock reads as somebody else's and is + // left alone, which is what this test is for. if ($objectEntity->isLocked() === true && $objectEntity->isLockedBySomeoneElse(userId: $this->container->get('userId')) === false ) { @@ -3334,6 +3350,14 @@ public function postPatch( // lock must survive somebody else's write: without this test // an administrator's write would silently strip a run's lock // as a side effect of a guard it had just passed. + // + // NO `runUuid` HERE, DELIBERATELY, and it is the one guard in + // this file that omits it. This decides a RELEASE, not a + // refusal: asking it as the run would make a run's own write + // drop the run's own lock the moment it saved — the lock is + // meant to outlive every write the run makes. Asked as a + // person, a run-held lock reads as somebody else's and is + // left alone, which is what this test is for. if ($objectEntity->isLocked() === true && $objectEntity->isLockedBySomeoneElse(userId: $this->container->get('userId')) === false ) { @@ -4844,4 +4868,35 @@ private function folderAccessDeniedResponse(FolderAccessDeniedException $excepti statusCode: FolderAccessDeniedException::HTTP_STATUS ); }//end folderAccessDeniedResponse() + + /** + * The flow run this write is being made for, or null when a person is + * writing. + * + * A run-scoped lock refuses every caller but the holding run, so a guard + * that cannot name the caller's run refuses the run that took the lock. + * Resolved from the container rather than injected because the ambient + * stack is a shared service and this is the only thing here that needs it. + * + * A container that cannot serve it answers "a person", which is the + * FAIL-CLOSED direction: a run lock refuses a caller with no run uuid, so + * the worst outcome is a refusal, never a lock walked through. + * + * @return string|null The executing run's uuid, or null. + * + * @spec openspec/changes/run-scoped-object-locking/specs/run-scoped-object-locking/spec.md#requirement-ownership-is-decided-by-one-predicate + */ + private function callerRunUuid(): ?string { + try { + $flowContext = $this->container->get(\OCA\OpenRegister\Service\Flow\FlowRunContext::class); + } catch (\Throwable $unavailable) { + return null; + } + + if ($flowContext instanceof \OCA\OpenRegister\Service\Flow\FlowRunContext === false) { + return null; + } + + return $flowContext->currentRunUuid(); + }//end callerRunUuid() }//end class diff --git a/lib/Db/ObjectEntity.php b/lib/Db/ObjectEntity.php index 88a058e1e5..5a631429d4 100644 --- a/lib/Db/ObjectEntity.php +++ b/lib/Db/ObjectEntity.php @@ -1163,6 +1163,12 @@ private function getFormattedDate(?DateTime $date): ?string { * - Run lock with no `runUuid`: held against everybody. A malformed lock * fails CLOSED, because the alternative is silently converting a * writer's bug into an open door. + * - User lock, run caller: held against the run. A run is not the person + * it runs as, in EITHER direction. Reading them as one holder let a run + * walk into a person's lock, take the extend branch and rewrite the + * payload as its own — and then release it at the end of the run. The + * person's lock was destroyed by a flow merely passing over the object, + * with no error and no audit of a displacement. * * @param string|null $userId The caller's user id, or null when anonymous. * @param string|null $runUuid The caller's flow-run uuid, when the caller is a run. @@ -1189,6 +1195,15 @@ public function isLockedBySomeoneElse(?string $userId, ?string $runUuid = null): return ($runUuid === null || trim($runUuid) !== trim($holder)); } + // A USER lock and a caller acting for a run: different holders, so the + // lock is held against it. The kinds are compared BEFORE the user id + // because they are what distinguishes the holders — a run under + // `alice` matching a lock alice took is exactly the confusion that let + // a flow take over a person's lock. + if ($runUuid !== null && trim($runUuid) !== '') { + return true; + } + return (($lock['user'] ?? null) !== $userId); }//end isLockedBySomeoneElse() diff --git a/lib/Service/Flow/FlowEngine.php b/lib/Service/Flow/FlowEngine.php index 1cfa3cb07e..3838482c68 100644 --- a/lib/Service/Flow/FlowEngine.php +++ b/lib/Service/Flow/FlowEngine.php @@ -938,7 +938,15 @@ private function walkStreams( resumeAt: $suspension->getResumeAt(), reason: $suspension->getMessage(), claimed: $claimed, - enabled: $streams->workRemains(transitions: $workflow->getEnabledTransitions(subject: $subject)) + // `settling`: this stream is the one parking, and the walk + // does not know that yet — its token still enables the very + // transition it is waiting ON. Counted, the park derives + // `queued` with no wake time, which a parallel worker picks + // up immediately. + enabled: $streams->workRemains( + transitions: $workflow->getEnabledTransitions(subject: $subject), + settling: $streamId + ) ); continue; } catch (Throwable $e) { @@ -1123,7 +1131,16 @@ private function fireOnStream( placeItems: $placeItems, claimed: $claimed, logEntry: $entry, - enabledAfter: $streams->workRemains(transitions: $workflow->getEnabledTransitions(subject: $subject)), + // The places this firing TAKES, handed over so the answer describes + // the marking AFTER the commit rather than the one the walk still + // holds: the stream picture is only re-read inside commitFiring(). + // Without them every ordinary mid-flow firing reported "no work + // remains", the commit derived `completed` for a run that was still + // walking, and every terminal listener fired on it. + enabledAfter: $streams->workRemains( + transitions: $workflow->getEnabledTransitions(subject: $subject), + produced: $takenTos + ), streamStatus: FlowRun::STATUS_RUNNING, streamError: $streamError ); diff --git a/lib/Service/Flow/FlowNodeRegistry.php b/lib/Service/Flow/FlowNodeRegistry.php index bc4889cfce..036df05b22 100644 --- a/lib/Service/Flow/FlowNodeRegistry.php +++ b/lib/Service/Flow/FlowNodeRegistry.php @@ -32,6 +32,7 @@ namespace OCA\OpenRegister\Service\Flow; use OCP\EventDispatcher\IEventDispatcher; +use OCP\IURLGenerator; use OCP\WorkflowEngine\IManager; use Psr\Log\LoggerInterface; use UnexpectedValueException; @@ -108,10 +109,17 @@ public static function renamedTypes(): array { * * @param IEventDispatcher $dispatcher Dispatches the contribution event. * @param LoggerInterface $logger The logger. + * @param IURLGenerator|null $urls Resolves the fallback icon for a node + * whose own icon does not resolve. Optional + * so the registry stays constructible + * without a container; absent, such a node + * is served with no icon rather than being + * dropped. */ public function __construct( private readonly IEventDispatcher $dispatcher, private readonly LoggerInterface $logger, + private readonly ?IURLGenerator $urls = null, ) { }//end __construct() @@ -204,7 +212,7 @@ public function palette(int $scope = IManager::SCOPE_ADMIN): array { 'id' => $id, 'displayName' => $node->getDisplayName(), 'description' => $node->getDescription(), - 'icon' => $node->getIcon(), + 'icon' => $this->iconFor(node: $node, type: $id), // ALWAYS present, and always one of trigger/step/end. An // editor that had to infer this fell back to matching the // id against a naming convention, which mis-labels every @@ -250,12 +258,21 @@ public function palette(int $scope = IManager::SCOPE_ADMIN): array { $palette[] = $entry; } catch (\Throwable $e) { - // One node whose metadata throws (a missing icon, a broken - // translation) must not blank the whole palette — the author - // would lose every node, not just the bad one. - $this->logger->warning( - message: '[FlowNodeRegistry] Skipping a node whose palette metadata failed: ' . $e->getMessage(), - context: ['file' => __FILE__, 'line' => __LINE__, 'type' => $id] + // One node whose metadata throws must not blank the whole + // palette — the author would lose every node, not just the bad + // one. A DROPPED NODE IS NOT A COSMETIC PROBLEM, though: it + // cannot be added to a flow at all, from anywhere, and the + // author is given no reason. So this is an error naming the + // type, not a warning, and the commonest cause — an icon the + // server does not ship — no longer reaches here at all + // ({@see self::iconFor()}). + $this->logger->error( + message: sprintf( + '[FlowNodeRegistry] DROPPED the node "%s" from the palette: its metadata threw (%s). It cannot be added to a flow until this is fixed.', + $id, + $e->getMessage() + ), + context: ['file' => __FILE__, 'line' => __LINE__, 'type' => $id, 'exception' => $e] ); }//end try }//end foreach @@ -263,6 +280,53 @@ public function palette(int $scope = IManager::SCOPE_ADMIN): array { return $palette; }//end palette() + /** + * A node's icon, or the app's own when the node's does not resolve. + * + * 🔴 A MISSING ICON USED TO DELETE THE NODE. `IURLGenerator::imagePath()` + * throws for an image the server does not ship, `palette()` caught that + * along with everything else, and the node simply was not in the + * catalogue: `openregister.lock-object` and `openregister.unlock-object` + * shipped pointing at `actions/lock.svg` and `actions/unlock.svg`, which + * NEITHER NC 33 NOR NC 34 has, so neither node could be added to a flow + * from the editor at all. Nothing failed; they were absent. Core's icon + * set is not a stable API and the next node to name a retired icon would + * have vanished the same way. + * + * So an icon is now resolved on its own, an unresolvable one is an ERROR + * naming the type and the icon, and the node is served with the app's icon + * instead of being dropped. A node the author can see and place with the + * wrong picture is strictly better than a node that does not exist. + * + * @param IFlowNode $node The node. + * @param string $type Its type id, for the message. + * + * @return string|null The icon path, the app's icon, or null when neither resolves. + * + * @spec openspec/changes/or-flow-nodes/specs/flow-nodes/spec.md + */ + private function iconFor(IFlowNode $node, string $type): ?string { + try { + return $node->getIcon(); + } catch (\Throwable $missing) { + $this->logger->error( + message: sprintf( + '[FlowNodeRegistry] The node "%s" names an icon this server does not have (%s); ' + . 'it is served with the app icon instead. Point it at an image that exists.', + $type, + $missing->getMessage() + ), + context: ['file' => __FILE__, 'line' => __LINE__, 'type' => $type, 'exception' => $missing] + ); + } + + try { + return $this->urls?->imagePath('openregister', 'app-dark.svg'); + } catch (\Throwable $noFallback) { + return null; + } + }//end iconFor() + /** * The links one run-log entry earns, from the node that wrote it. * diff --git a/lib/Service/Flow/FlowRunContext.php b/lib/Service/Flow/FlowRunContext.php index cc34fa3540..39d3a6d4b3 100644 --- a/lib/Service/Flow/FlowRunContext.php +++ b/lib/Service/Flow/FlowRunContext.php @@ -160,6 +160,40 @@ public function current(): ?array { return $this->frames[(count($this->frames) - 1)]; }//end current() + /** + * The uuid of the run currently executing, or null outside any run. + * + * WHY A WRITE GUARD ASKS THIS. A run-scoped lock refuses every caller but + * the holding run — the run's own `runAs` user included, which is the + * point. A guard that cannot name the caller's run therefore refuses the + * run that took the lock: the flow locks a case and is then refused by its + * own lock at the next write. This is the seam that answers "which run is + * writing", and it is ambient for the same reason attribution is: the + * write may be several calls deep inside a leaf app that has never heard + * of flows. + * + * Null outside a run, and null for a hop that is not attributable. Both + * read as "a person is writing", which is the FAIL-CLOSED direction: a run + * lock refuses a caller with no run uuid. + * + * @return string|null The executing run's uuid, or null. + * + * @spec openspec/changes/run-scoped-object-locking/specs/run-scoped-object-locking/spec.md#requirement-ownership-is-decided-by-one-predicate + */ + public function currentRunUuid(): ?string { + $frame = $this->current(); + if ($frame === null) { + return null; + } + + $run = trim((string)$frame['run']); + if ($run === '') { + return null; + } + + return $run; + }//end currentRunUuid() + /** * How deep the stack is. Test and diagnostic use only. * diff --git a/lib/Service/Flow/FlowStreamWalk.php b/lib/Service/Flow/FlowStreamWalk.php index d0c92f6e3b..5c74c72f98 100644 --- a/lib/Service/Flow/FlowStreamWalk.php +++ b/lib/Service/Flow/FlowStreamWalk.php @@ -253,23 +253,33 @@ private function isAdvanceable(string $id): bool { * Petri net, but that transition is a wait, not work — counting it would * make every parked run read as `queued` and spin the worker. * + * 🔴 THE ANSWER MUST DESCRIBE THE STATE AFTER THE THING BEING COMMITTED, + * not the state the walk still holds in memory. It is called as an + * ARGUMENT to `commitFiring()` and to `park()`, and both of those are what + * move the stream — so at the moment this runs, `$this->streams` still has + * the firing stream on the place the firing just consumed and `$this->parked` + * does not yet know about the stream that is parking. Answering from that + * stale picture said "no work remains" at EVERY ordinary mid-flow firing, + * `FlowRunCommit::applyDerivedStatus()` then derived `completed` for a run + * that was still working, `FlowRunMapper::update()` announced it, and + * `FlowRunLockReleaseListener` released the run's object locks — the + * defect this parameter pair exists to close. `$produced` and `$settling` + * are how the caller says what its commit is about to change. + * * @param array $transitions The enabled transitions. + * @param array $produced The places the commit is about to put tokens on. + * @param string|null $settling A stream whose current place is about to stop counting: it is parking, or ending. * * @return bool True when an unparked stream has an enabled transition. * * @spec openspec/changes/flow-parallel-streams/specs/flow-parallel-streams/spec.md#requirement-a-runs-status-must-stay-derivable-from-its-streams-with-no-new-value */ - public function workRemains(array $transitions): bool { - $unparked = []; - foreach ($this->streams as $id => $stream) { - if (isset($this->parked[$id]) === false && $stream['place'] !== null) { - $unparked[$stream['place']] = true; - } - } + public function workRemains(array $transitions, array $produced = [], ?string $settling = null): bool { + $live = $this->livePlacesAfter(produced: $produced, settling: $settling); foreach ($transitions as $transition) { foreach ($transition->getFroms() as $from) { - if (isset($unparked[(string)$from]) === true) { + if (isset($live[(string)$from]) === true) { return true; } } @@ -278,6 +288,37 @@ public function workRemains(array $transitions): bool { return false; }//end workRemains() + /** + * The places a token will sit on once the caller's commit lands, excluding + * the parked streams' — see {@see self::workRemains()} for why the answer + * cannot be read off the walk's own picture. + * + * @param array $produced The places the commit is about to mark. + * @param string|null $settling A stream whose place stops counting. + * + * @return array The places, as a set. + * + * @spec openspec/changes/flow-parallel-streams/specs/flow-parallel-streams/spec.md#requirement-a-runs-status-must-stay-derivable-from-its-streams-with-no-new-value + */ + private function livePlacesAfter(array $produced, ?string $settling): array { + $live = []; + foreach ($this->streams as $id => $stream) { + if ($id === $settling || isset($this->parked[$id]) === true) { + continue; + } + + if ($stream['place'] !== null) { + $live[$stream['place']] = true; + } + } + + foreach ($produced as $place) { + $live[(string)$place] = true; + } + + return $live; + }//end livePlacesAfter() + /** * The ordinal path of a stream, for a log entry that is not a firing (a * suspension, a stop, a terminal failure) and so is written by the step diff --git a/lib/Service/Flow/Nodes/LockObjectNode.php b/lib/Service/Flow/Nodes/LockObjectNode.php index 81463f3018..1bb17172a8 100644 --- a/lib/Service/Flow/Nodes/LockObjectNode.php +++ b/lib/Service/Flow/Nodes/LockObjectNode.php @@ -195,7 +195,11 @@ public function getDescription(): string { * @return string The icon path. */ public function getIcon(): string { - return $this->urls->imagePath('core', 'actions/lock.svg'); + // The APP's icon, not core's. `core/img/actions/lock.svg` does not + // exist in NC 33 or 34, `imagePath()` throws for an image the server + // does not ship, and this node was therefore absent from the editor's + // palette entirely. An icon the app ships cannot go missing under it. + return $this->urls->imagePath('openregister', 'lock.svg'); }//end getIcon() /** diff --git a/lib/Service/Flow/Nodes/UnlockObjectNode.php b/lib/Service/Flow/Nodes/UnlockObjectNode.php index cf58af6b61..a01c87902b 100644 --- a/lib/Service/Flow/Nodes/UnlockObjectNode.php +++ b/lib/Service/Flow/Nodes/UnlockObjectNode.php @@ -125,7 +125,9 @@ public function getDescription(): string { * @return string The icon path. */ public function getIcon(): string { - return $this->urls->imagePath('core', 'actions/unlock.svg'); + // The APP's icon: see LockObjectNode::getIcon(). `actions/unlock.svg` + // is not part of NC 33's or 34's core icon set either. + return $this->urls->imagePath('openregister', 'unlock.svg'); }//end getIcon() /** diff --git a/lib/Service/Object/RevertHandler.php b/lib/Service/Object/RevertHandler.php index 5c0eb84e98..57680d8063 100644 --- a/lib/Service/Object/RevertHandler.php +++ b/lib/Service/Object/RevertHandler.php @@ -37,6 +37,12 @@ /** * Class RevertHandler * Service for handling object reversion + * + * @SuppressWarnings(PHPMD.CouplingBetweenObjects) One collaborator over the + * threshold, and it is the ambient flow-run stack: a revert is a write, and a + * write guard that cannot name the caller's run refuses the run holding the + * lock. Splitting the class to avoid naming one more type would trade a real + * guard for a metric. */ class RevertHandler { @@ -154,7 +160,7 @@ public function revert( // Check if the object is locked. Ownership is decided by the one // production predicate, so a run-held lock refuses the run's own // runAs user here exactly as it does at every other guard. - if ($object->isLockedBySomeoneElse(userId: $this->container->get('userId')) === true) { + if ($object->isLockedBySomeoneElse(userId: $this->container->get('userId'), runUuid: $this->callerRunUuid()) === true) { throw new LockedException( message: sprintf('Object is locked by %s', (string)$object->describeLockHolder()) ); @@ -179,4 +185,35 @@ public function revert( return $savedObject; }//end revert() + + /** + * The flow run this write is being made for, or null when a person is + * writing. + * + * A run-scoped lock refuses every caller but the holding run, so a guard + * that cannot name the caller's run refuses the run that took the lock. + * Resolved from the container rather than injected because the ambient + * stack is a shared service and this is the only thing here that needs it. + * + * A container that cannot serve it answers "a person", which is the + * FAIL-CLOSED direction: a run lock refuses a caller with no run uuid, so + * the worst outcome is a refusal, never a lock walked through. + * + * @return string|null The executing run's uuid, or null. + * + * @spec openspec/changes/run-scoped-object-locking/specs/run-scoped-object-locking/spec.md#requirement-ownership-is-decided-by-one-predicate + */ + private function callerRunUuid(): ?string { + try { + $flowContext = $this->container->get(\OCA\OpenRegister\Service\Flow\FlowRunContext::class); + } catch (\Throwable $unavailable) { + return null; + } + + if ($flowContext instanceof \OCA\OpenRegister\Service\Flow\FlowRunContext === false) { + return null; + } + + return $flowContext->currentRunUuid(); + }//end callerRunUuid() }//end class diff --git a/lib/Service/Object/SaveObject.php b/lib/Service/Object/SaveObject.php index 6cb96e59c8..4569408703 100644 --- a/lib/Service/Object/SaveObject.php +++ b/lib/Service/Object/SaveObject.php @@ -307,6 +307,7 @@ class SaveObject { * @param IEventDispatcher|null $eventDispatcher Event dispatcher (reference events) * @param \OCA\OpenRegister\Service\ObjectSource\ObjectSourceRegistry|null $objectSourceRegistry Writable object-source provider registry * @param FieldEncryptionHandler|null $fieldEncryptionHandler Field-level encryption handler + * @param \OCA\OpenRegister\Service\Flow\FlowRunContext|null $runContext The ambient flow-run stack, so the lock guard can tell which run is writing * * @SuppressWarnings(PHPMD.ExcessiveParameterList) Nextcloud DI requires constructor injection * @@ -341,6 +342,7 @@ public function __construct( private readonly ?IEventDispatcher $eventDispatcher = null, private readonly ?\OCA\OpenRegister\Service\ObjectSource\ObjectSourceRegistry $objectSourceRegistry = null, private readonly ?FieldEncryptionHandler $fieldEncryptionHandler = null, + private readonly ?\OCA\OpenRegister\Service\Flow\FlowRunContext $runContext = null, ) { $this->twig = new Environment($arrayLoader); }//end __construct() @@ -3197,7 +3199,17 @@ private function findAndValidateExistingObject( $currentUserId = $currentUser->getUID(); } - if ($existingObject->isLockedBySomeoneElse(userId: $currentUserId) === true) { + // WHICH RUN IS WRITING, not just which user. A run-scoped lock + // refuses every caller but the holding run, so a guard that omits + // the run identity refuses THE RUN THAT TOOK THE LOCK: a flow + // locks a case at one step and is turned away by its own lock at + // the next. Ambient rather than a parameter because the write is + // routinely several calls deep in code that has never heard of + // flows; absent, it reads as a person, which is the fail-closed + // answer a run lock already gives. + $callerRun = $this->runContext?->currentRunUuid(); + + if ($existingObject->isLockedBySomeoneElse(userId: $currentUserId, runUuid: $callerRun) === true) { $holder = (string)$existingObject->describeLockHolder(); $unlockAdvice = 'Please unlock the object before attempting to update it.'; throw new Exception("Cannot update object: Object is locked by {$holder}. " . $unlockAdvice); diff --git a/openspec/changes/run-scoped-object-locking/specs/run-scoped-object-locking/spec.md b/openspec/changes/run-scoped-object-locking/specs/run-scoped-object-locking/spec.md index 0c767e14ee..1b7b7d9440 100644 --- a/openspec/changes/run-scoped-object-locking/specs/run-scoped-object-locking/spec.md +++ b/openspec/changes/run-scoped-object-locking/specs/run-scoped-object-locking/spec.md @@ -49,6 +49,14 @@ differs from the recorded one, **including a caller presenting the run's own `runAs` user id and no run uuid**. An expired lock SHALL be held against nobody. +A run and the person it executes as are DIFFERENT HOLDERS, in both +directions: a user lock SHALL be held against a caller presenting a run uuid, +**including a run executing as the person who took it**. Without that, a run +walking over a person's locked object did not merely pass the guard — it took +the extend branch, rewrote the payload as its own run lock, and released it +when the run ended, destroying a lock a person was relying on with no error +and no audited displacement. + #### Scenario: Two runs under one user conflict - **GIVEN** an object locked by run A executing as `alice` - **WHEN** run B, also executing as `alice`, evaluates the lock @@ -65,6 +73,13 @@ nobody. - **WHEN** run A locks it again - **THEN** the lock MUST be extended rather than refused +#### Scenario: A person's lock survives a run passing over the object +- **GIVEN** an object locked by `alice` as a person +- **WHEN** a run executing as `alice` locks the same object +- **THEN** the run MUST be refused, the payload MUST be unchanged, and the + lock MUST still be a user lock when the run ends +- @e2e exclude engine-internal, covered by ObjectEntityRunLockTest and the rig walk + ### Requirement: A lock refuses a write and names its holder While a live lock is held against the caller, the system SHALL refuse to @@ -74,6 +89,14 @@ When a run holds the lock the message SHALL name the run. The refusal SHALL apply on the service write path, not only at the HTTP controller: a lock that only the controller enforces is not a lock. +Every refusal guard SHALL identify the caller by BOTH its user id and, when a +flow run is executing, that run's uuid. The run identity is ambient — a write +made for a run is routinely several calls deep inside code that has never +heard of flows — and a guard that omits it refuses the run that took the +lock, which makes the lock step unusable by the flow that used it. A guard +that decides a RELEASE rather than a refusal SHALL NOT ask as the run: a run's +lock is meant to outlive every write the run makes. + #### Scenario: A person is refused while a run holds the lock - **GIVEN** an object locked by a flow run - **WHEN** a person updates it over the API @@ -84,6 +107,13 @@ controller: a lock that only the controller enforces is not a lock. - **WHEN** that same person updates it - **THEN** the write MUST succeed +#### Scenario: The holding run writes to the object it locked +- **GIVEN** an object locked by run A +- **WHEN** a step of run A updates it +- **THEN** the write MUST succeed, while the same write made for run B or by a + person MUST be refused +- @e2e exclude engine-internal, covered by SaveObjectTest and the rig walk + #### Scenario: A successful write releases only the writer's own lock - **GIVEN** an object carrying a lock the writer does not hold - **WHEN** a write to that object completes @@ -165,6 +195,13 @@ The engine SHALL release every lock a run holds on **any** terminal outcome: `completed`, `stopped`, `failed` and `dead_letter`. The release SHALL NOT depend on a node running, so a run that crashed or failed still releases. +A run that has NOT ended SHALL keep every lock it holds. The status a run's +commit path derives mid-pass is part of that guarantee: a run whose stored +status is `suspended`, `queued` or `running` MUST NOT have been announced as +terminal on the way there, because the announcement is what releases the +locks — and a case that cannot stay locked while its flow works has no lock +at all. + The release SHALL be idempotent, since terminality can be observed more than once, and SHALL NOT propagate a failure into the run's own terminal write. @@ -174,6 +211,13 @@ NOT require reading every object table. The existing lock expiry SHALL remain in force as the final backstop. +#### Scenario: A parked run keeps its locks +- **GIVEN** a run that has taken a lock and reached a step that waits +- **WHEN** the run is stored as `suspended` +- **THEN** no terminal event MUST have been announced for it, and the lock MUST + still be held by that run +- @e2e exclude engine-internal, covered by RunLockReleaseTerminalityTest and the rig walk + #### Scenario: A completed run releases its locks - **GIVEN** a run holding a lock - **WHEN** the run completes @@ -209,6 +253,31 @@ The existing lock expiry SHALL remain in force as the final backstop. - **WHEN** any caller evaluates it - **THEN** the lock MUST be held against nobody +### Requirement: A node the engine registers is offered in the palette + +Every node type the engine can execute SHALL appear in the node catalogue the +editor reads. A node SHALL NOT be dropped from the catalogue because its icon +does not resolve: `IURLGenerator::imagePath()` throws for an image the server +does not ship, Nextcloud's core icon set is not a stable API, and a node that +is missing from the catalogue cannot be added to a flow at all — a total +failure produced by a cosmetic cause. A node whose icon cannot be resolved +SHALL be served with the app's own icon, and the failure SHALL be logged as an +error naming the node. + +#### Scenario: The lock and unlock nodes can be added to a flow +- **GIVEN** an instance running any supported Nextcloud +- **WHEN** the editor reads the node catalogue +- **THEN** `openregister.lock-object` and `openregister.unlock-object` MUST both + be present, each with an icon that resolves +- @e2e exclude covered by the palette sweep in FlowNodePaletteIconsTest and the rig walk + +#### Scenario: A node with an unresolvable icon is reported, not deleted +- **GIVEN** a registered node whose `getIcon()` names an image the server does not ship +- **WHEN** the catalogue is built +- **THEN** the node MUST still be listed, with the app icon, and an error naming + the node MUST be logged +- @e2e exclude covered by FlowNodePaletteIconsTest + ## MODIFIED Requirements ### Requirement: A lock records what it was taken for diff --git a/tests/Fixtures/nextcloud-core-images.txt b/tests/Fixtures/nextcloud-core-images.txt new file mode 100644 index 0000000000..db13b2fb21 --- /dev/null +++ b/tests/Fixtures/nextcloud-core-images.txt @@ -0,0 +1,259 @@ +# Every file Nextcloud 34.0.3 ships under `core/img/`, one relative path per +# line, `#` comments ignored. +# +# WHY THIS EXISTS. `IURLGenerator::imagePath('core', )` THROWS for an +# image the server does not ship, and FlowNodeRegistry::palette() used to drop +# any node whose metadata threw — so a node naming a core icon that does not +# exist could not be added to a flow at all, silently. Two shipped that way +# (`openregister.lock-object` and `openregister.unlock-object`, both naming +# `actions/lock.svg`, which no supported Nextcloud has). Core's icon set is not +# a stable API and nothing else in this repository knows what is in it. +# +# THIS FILE IS THE FALLBACK, NOT THE AUTHORITY. The sweep in +# tests/Unit/Service/Flow/FlowNodePaletteIconsTest.php resolves against the +# REAL server tree whenever the tests can see one — which they always can in +# CI, where the app is checked out into `server/apps/openregister` — and uses +# this list only when they cannot, so a standalone clone still gets a verdict +# instead of a skip. +# +# Regenerate against a running instance: +# docker exec sh -c 'cd /var/www/html/core/img && find . -type f | sed "s|^\./||" | sort' +actions/add-folder-description.svg +actions/add.svg +actions/address.png +actions/address.svg +actions/alert-outline.svg +actions/arrow-left.svg +actions/arrow-right.svg +actions/audio-off.svg +actions/audio.svg +actions/bluesky.svg +actions/caret-white.svg +actions/caret.svg +actions/change.svg +actions/checkbox-mark-dark.svg +actions/checkbox-mark-white.svg +actions/checkbox-mark.svg +actions/checkbox-mixed-dark.svg +actions/checkbox-mixed-white.svg +actions/checkbox-mixed.svg +actions/checkmark-white.svg +actions/checkmark.png +actions/checkmark.svg +actions/clippy.svg +actions/close.svg +actions/comment.png +actions/comment.svg +actions/confirm-fade.svg +actions/confirm-white.svg +actions/confirm.svg +actions/delete.png +actions/delete.svg +actions/details.svg +actions/disabled-user.svg +actions/disabled-users.svg +actions/download.png +actions/download.svg +actions/edit.svg +actions/error-white.svg +actions/error.svg +actions/external.svg +actions/filter.svg +actions/fullscreen.svg +actions/group.svg +actions/history.png +actions/history.svg +actions/info-white.svg +actions/info.png +actions/info.svg +actions/logout.svg +actions/mail.svg +actions/mastodon.svg +actions/menu-sidebar.svg +actions/menu.svg +actions/more-white.svg +actions/more.png +actions/more.svg +actions/password-white.svg +actions/password.png +actions/password.svg +actions/pause.svg +actions/phone.svg +actions/play-add.svg +actions/play-next.svg +actions/play-previous.svg +actions/play.svg +actions/profile.svg +actions/projects.svg +actions/public-white.svg +actions/public.svg +actions/quota.svg +actions/recent.svg +actions/rename.svg +actions/screen-off.svg +actions/screen.svg +actions/search.svg +actions/settings-dark.svg +actions/settings.svg +actions/share.png +actions/share.svg +actions/shared.svg +actions/sound-off.svg +actions/sound.svg +actions/star-dark.svg +actions/star-rounded.svg +actions/star.png +actions/star.svg +actions/starred.png +actions/starred.svg +actions/tag.png +actions/tag.svg +actions/template-add.svg +actions/timezone.svg +actions/toggle-background.svg +actions/toggle-filelist.svg +actions/toggle-pictures.svg +actions/toggle.svg +actions/triangle-e.svg +actions/triangle-n.svg +actions/triangle-s.svg +actions/twitter.svg +actions/unshare.svg +actions/upload.svg +actions/user-admin.svg +actions/user.svg +actions/verified.svg +actions/verify.svg +actions/verifying.svg +actions/video-off.svg +actions/video-switch.svg +actions/video.svg +actions/view-close.svg +actions/view-download.svg +actions/view-next.svg +actions/view-pause.svg +actions/view-play.svg +actions/view-previous.svg +apps/circles.svg +apps/notes.svg +apps/richdocuments.svg +apps/richdocumentscode.svg +apps/spreed.svg +appstore.svg +bluesky-light.svg +bluesky.svg +breadcrumb.svg +caldav/attendees.png +caldav/attendees.svg +caldav/description.png +caldav/description.svg +caldav/link.png +caldav/link.svg +caldav/location.png +caldav/location.svg +caldav/organizer.png +caldav/organizer.svg +caldav/time.png +caldav/time.svg +caldav/title.png +caldav/title.svg +categories/auth.svg +categories/bundles.svg +categories/customization.svg +categories/dashboard.svg +categories/dashboard.svg.license +categories/files.svg +categories/games.svg +categories/integration.svg +categories/monitoring.svg +categories/multimedia.svg +categories/office.svg +categories/organization.svg +categories/social.svg +categories/workflow.svg +clients/desktop.svg +clients/phone.svg +clients/tablet.svg +desktopapp.svg +f-droid.svg +facebook-light.svg +facebook.svg +favicon-fb.png +favicon-mask.svg +favicon-touch.png +favicon-touch.svg +favicon.ico +favicon.png +favicon.svg +filetypes/application-pdf.svg +filetypes/application.svg +filetypes/audio.svg +filetypes/file.svg +filetypes/folder-drag-accept.svg +filetypes/folder-encrypted.svg +filetypes/folder-external.svg +filetypes/folder-public.svg +filetypes/folder-shared.svg +filetypes/folder-starred.svg +filetypes/folder.svg +filetypes/font.svg +filetypes/image.svg +filetypes/link.svg +filetypes/location.svg +filetypes/mindmap.svg +filetypes/package-x-generic.svg +filetypes/text-calendar.svg +filetypes/text-code.svg +filetypes/text-vcard.svg +filetypes/text.svg +filetypes/video.svg +filetypes/whiteboard.svg +filetypes/x-office-document.svg +filetypes/x-office-drawing.svg +filetypes/x-office-form-template.svg +filetypes/x-office-form.svg +filetypes/x-office-presentation.svg +filetypes/x-office-spreadsheet.svg +googleplay.png +loading-dark.gif +loading-small-dark.gif +loading-small.gif +loading.gif +logo/logo-enterprise.png +logo/logo-enterprise.svg +logo/logo-icon-175px.png +logo/logo-mail.png +logo/logo.png +logo/logo.svg +mail.svg +manifest.json +manifest.json.license +mastodon-light.svg +mastodon.svg +places/calendar-dark.png +places/calendar.png +places/calendar.svg +places/contacts-dark.png +places/contacts.svg +places/default-app-icon.svg +places/files.svg +places/home.svg +places/link.svg +places/music.svg +places/picture.svg +rating/s0.svg +rating/s1.svg +rating/s10.svg +rating/s2.svg +rating/s3.svg +rating/s4.svg +rating/s5.svg +rating/s6.svg +rating/s7.svg +rating/s8.svg +rating/s9.svg +rss.svg +twitter.svg +x-dark.svg +x-light.svg +x.svg diff --git a/tests/Unit/Db/ObjectEntityRunLockTest.php b/tests/Unit/Db/ObjectEntityRunLockTest.php index 643b7e57e8..b4518e8b88 100644 --- a/tests/Unit/Db/ObjectEntityRunLockTest.php +++ b/tests/Unit/Db/ObjectEntityRunLockTest.php @@ -142,6 +142,52 @@ public function testARunLockRefusesItsOwnRunAsUser(): void { ); }//end testARunLockRefusesItsOwnRunAsUser() + /** + * A PERSON'S LOCK SURVIVES A RUN PASSING OVER THE OBJECT. + * + * A run and the person it runs as are different holders, and reading them + * as one holder was not merely a refusal that did not happen: `lock()` + * took the EXTEND branch and rewrote the payload as a run lock. The + * person's lock was then released when the run ended — destroyed by a flow + * that merely passed over the object, with no error and no audited + * displacement. + * + * @return void + */ + public function testAPersonsLockSurvivesARunPassingOverTheObject(): void { + $this->entity->lock($this->session('alice'), 'reviewing it myself', 3600, null); + $before = $this->entity->getLocked(); + + try { + $this->entity->lock($this->session('alice'), 'step-one', 3600, self::RUN_A); + $this->fail('a run took over a person\'s lock'); + } catch (Exception $refused) { + $this->assertStringContainsString('alice', $refused->getMessage()); + } + + $this->assertSame($before, $this->entity->getLocked(), 'the run rewrote the person\'s lock'); + $this->assertNull($this->entity->getLockedByRun(), 'the lock became a run lock'); + }//end testAPersonsLockSurvivesARunPassingOverTheObject() + + /** + * The same thing at the predicate: a user lock is held AGAINST a run, + * including a run executing as the holder. + * + * @return void + */ + public function testAUserLockIsHeldAgainstARunRunningAsItsHolder(): void { + $this->entity->lock($this->session('alice'), 'reviewing it myself', 3600, null); + + $this->assertTrue( + $this->entity->isLockedBySomeoneElse(userId: 'alice', runUuid: self::RUN_A), + 'a run inherited the lock of the person it runs as' + ); + $this->assertFalse( + $this->entity->isLockedBySomeoneElse(userId: 'alice', runUuid: null), + 'alice was refused her own lock' + ); + }//end testAUserLockIsHeldAgainstARunRunningAsItsHolder() + // --------------------------------------------------------------- // The record shape, and the records that already exist. // --------------------------------------------------------------- diff --git a/tests/Unit/Service/Flow/FlowNodePaletteIconsTest.php b/tests/Unit/Service/Flow/FlowNodePaletteIconsTest.php new file mode 100644 index 0000000000..d766d617bc --- /dev/null +++ b/tests/Unit/Service/Flow/FlowNodePaletteIconsTest.php @@ -0,0 +1,336 @@ + + * SPDX-License-Identifier: EUPL-1.2 + * + * @category Test + * @package OCA\OpenRegister\Tests\Unit\Service\Flow + * + * @author Conduction Development Team + * @copyright 2026 Conduction B.V. + * @license EUPL-1.2 https://joinup.ec.europa.eu/collection/eupl/eupl-text-eupl-12 + * + * @link https://OpenRegister.app + * + * @spec openspec/changes/or-flow-nodes/specs/flow-nodes/spec.md + */ + +declare(strict_types=1); + +namespace OCA\OpenRegister\Tests\Unit\Service\Flow; + +use OCA\OpenRegister\Service\Flow\FlowNodeRegistry; +use OCA\OpenRegister\Service\Flow\IFlowNode; +use OCA\OpenRegister\Service\Flow\RegisterFlowNodesEvent; +use OCP\EventDispatcher\IEventDispatcher; +use OCP\IURLGenerator; +use OCP\WorkflowEngine\IManager; +use PHPUnit\Framework\TestCase; +use Psr\Log\LoggerInterface; +use RuntimeException; + +/** + * A node whose metadata answers whatever the test needs. + */ +class PaletteProbeNode implements IFlowNode { + + /** + * Constructor. + * + * @param string $icon The icon to return, or '' to throw as imagePath() does. + */ + public function __construct(private readonly string $icon) { + } + + /** + * The type id. + * + * @return string The id. + */ + public function getId(): string { + return 'test.probe'; + } + + /** + * Accepts any configuration. + * + * @param array $config The config. + * + * @return void + */ + public function validateConfig(array $config): void { + } + + /** + * The display name. + * + * @return string The name. + */ + public function getDisplayName(): string { + return 'Probe'; + } + + /** + * The description. + * + * @return string The description. + */ + public function getDescription(): string { + return 'A node that exists only in this test.'; + } + + /** + * The icon — or the throw a missing image produces. + * + * @return string The icon path. + */ + public function getIcon(): string { + if ($this->icon === '') { + // The shape IURLGenerator::imagePath() throws with. + throw new RuntimeException('image not found: image:actions/lock.svg webroot:/ serverroot:/var/www/html'); + } + + return $this->icon; + } + + /** + * Available everywhere. + * + * @param int $scope The scope. + * + * @return bool True. + */ + public function isAvailableForScope(int $scope): bool { + return true; + } + + /** + * Execute. + * + * @param array $items The items. + * @param array $config The config. + * @param array $context The context. + * + * @return array The items. + */ + public function execute(array $items, array $config, array $context): array { + return $items; + } +}//end class + +/** + * The palette's icons. + * + * @covers \OCA\OpenRegister\Service\Flow\FlowNodeRegistry + */ +class FlowNodePaletteIconsTest extends TestCase { + + /** + * The app root. + * + * @var string + */ + private const APP_ROOT = __DIR__ . '/../../../..'; + + /** + * Every `imagePath(app, path)` a flow node's getIcon() names. + * + * The nodes are read from source rather than instantiated because building + * all 27 needs the server container; what is asserted — the image a node + * asks the palette for — is a literal in every one of them. + * + * @return array Class file => [app, path]. + */ + private function declaredIcons(): array { + $icons = []; + $found = []; + $walker = new \RecursiveIteratorIterator(new \RecursiveDirectoryIterator(self::APP_ROOT . '/lib')); + foreach ($walker as $file) { + if ($file->isFile() === false || $file->getExtension() !== 'php') { + continue; + } + + $source = (string)file_get_contents($file->getPathname()); + if (str_contains($source, 'implements IFlowNode') === false) { + continue; + } + + $found[] = $file->getFilename(); + $matched = preg_match( + '/function getIcon\(\)[^{]*\{.*?imagePath\(\s*\'([^\']+)\'\s*,\s*\'([^\']+)\'\s*\)/s', + $source, + $parts + ); + + $this->assertSame( + 1, + $matched, + $file->getFilename() . ' implements IFlowNode but its getIcon() does not name an image this sweep can read; ' + . 'a node this sweep cannot read is a node whose icon nobody checks.' + ); + + $icons[$file->getFilename()] = [$parts[1], $parts[2]]; + } + + $this->assertGreaterThan(20, count($found), 'the node sweep found almost nothing, so it swept nothing'); + + return $icons; + }//end declaredIcons() + + /** + * The Nextcloud source root, when the tests can see one. + * + * Present in CI, where the app is checked out into `server/apps/openregister`; + * absent in a standalone clone, which is why the fixture below exists. + * + * @return string|null The root, or null. + */ + private function nextcloudRoot(): ?string { + $explicit = getenv('OPENREGISTER_TEST_NC_ROOT'); + if (is_string($explicit) === true && $explicit !== '' && is_dir($explicit . '/core/img') === true) { + return rtrim($explicit, '/'); + } + + $dir = realpath(self::APP_ROOT); + for ($depth = 0; $depth < 8 && is_string($dir) === true; $depth++) { + if (is_dir($dir . '/core/img') === true && is_dir($dir . '/apps') === true) { + return $dir; + } + + $parent = dirname($dir); + if ($parent === $dir) { + break; + } + + $dir = $parent; + } + + return null; + }//end nextcloudRoot() + + /** + * The recorded core inventory, for a checkout with no server beside it. + * + * @return array Path => true. + */ + private function recordedCoreImages(): array { + $path = self::APP_ROOT . '/tests/Fixtures/nextcloud-core-images.txt'; + $this->assertFileExists($path, 'the offline core-image inventory is missing, so the sweep has nothing to check against'); + + $images = []; + foreach (file($path, (FILE_IGNORE_NEW_LINES | FILE_SKIP_EMPTY_LINES)) as $line) { + $line = trim($line); + if ($line === '' || str_starts_with($line, '#') === true) { + continue; + } + + $images[$line] = true; + } + + $this->assertGreaterThan(100, count($images), 'the recorded inventory is too small to be a real one'); + + return $images; + }//end recordedCoreImages() + + /** + * EVERY node's icon is an image that exists. + * + * Resolved against the real server tree when the tests can see one — they + * always can in CI — and against the recorded inventory otherwise, so a + * standalone clone gets a verdict rather than a skip. + * + * @return void + */ + public function testEveryFlowNodeNamesAnImageThatExists(): void { + $root = $this->nextcloudRoot(); + $recorded = ($root === null) ? $this->recordedCoreImages() : []; + + $checked = 0; + foreach ($this->declaredIcons() as $node => [$app, $image]) { + if ($app === 'openregister') { + $this->assertFileExists( + self::APP_ROOT . '/img/' . $image, + $node . ' names img/' . $image . ", which this app does not ship: the node would be served with the app's icon and its own would never render." + ); + $checked++; + continue; + } + + if ($root !== null) { + $this->assertFileExists( + $root . '/' . $app . '/img/' . $image, + $node . ' names ' . $app . '/img/' . $image . ', which this Nextcloud does not ship: imagePath() throws and the node loses its icon.' + ); + $checked++; + continue; + } + + $this->assertArrayHasKey( + $image, + $recorded, + $node . ' names ' . $app . '/img/' . $image . ', which Nextcloud does not ship (checked against ' + . 'tests/Fixtures/nextcloud-core-images.txt; run with OPENREGISTER_TEST_NC_ROOT set to check a live tree).' + ); + $checked++; + } + + $this->assertGreaterThan(20, $checked, 'the sweep verified almost nothing'); + }//end testEveryFlowNodeNamesAnImageThatExists() + + /** + * A node whose icon cannot be resolved stays in the palette, and the + * failure is an ERROR naming the node. + * + * This is the half that stops the defect recurring: the inventory above + * only knows about the images nodes name TODAY. + * + * @return void + */ + public function testANodeWithAnUnresolvableIconIsReportedNotDropped(): void { + $logged = []; + $logger = $this->createMock(LoggerInterface::class); + $logger->method('error')->willReturnCallback(static function (string $message) use (&$logged): void { + $logged[] = $message; + }); + + $urls = $this->createMock(IURLGenerator::class); + $urls->method('imagePath')->willReturn('/apps/openregister/img/app-dark.svg'); + + $dispatcher = $this->createMock(IEventDispatcher::class); + $dispatcher->method('dispatchTyped')->willReturnCallback(static function (object $event): void { + if ($event instanceof RegisterFlowNodesEvent === true) { + $event->registerNode(new PaletteProbeNode(icon: '')); + } + }); + + $palette = (new FlowNodeRegistry($dispatcher, $logger, $urls))->palette(scope: IManager::SCOPE_ADMIN); + + $entry = null; + foreach ($palette as $candidate) { + if ($candidate['id'] === 'test.probe') { + $entry = $candidate; + } + } + + $this->assertNotNull($entry, 'a node with an unresolvable icon vanished from the palette; it cannot be added to a flow at all'); + $this->assertSame('Probe', $entry['displayName']); + $this->assertSame('/apps/openregister/img/app-dark.svg', $entry['icon'], 'the node should fall back to the app icon'); + $this->assertNotSame([], $logged, 'the icon failure was not reported anywhere'); + $this->assertStringContainsString('test.probe', $logged[0], 'the report does not name the node'); + }//end testANodeWithAnUnresolvableIconIsReportedNotDropped() +}//end class diff --git a/tests/Unit/Service/Flow/RunLockReleaseTerminalityTest.php b/tests/Unit/Service/Flow/RunLockReleaseTerminalityTest.php new file mode 100644 index 0000000000..7aa80b37d0 --- /dev/null +++ b/tests/Unit/Service/Flow/RunLockReleaseTerminalityTest.php @@ -0,0 +1,395 @@ + + * SPDX-License-Identifier: EUPL-1.2 + * + * @category Test + * @package OCA\OpenRegister\Tests\Unit\Service\Flow + * + * @author Conduction Development Team + * @copyright 2026 Conduction B.V. + * @license EUPL-1.2 https://joinup.ec.europa.eu/collection/eupl/eupl-text-eupl-12 + * + * @link https://OpenRegister.app + * + * @spec openspec/changes/run-scoped-object-locking/specs/run-scoped-object-locking/spec.md#requirement-every-lock-a-run-holds-is-released-when-the-run-ends + */ + +declare(strict_types=1); + +namespace OCA\OpenRegister\Tests\Unit\Service\Flow; + +use OCA\OpenRegister\Db\FlowClaim; +use OCA\OpenRegister\Db\FlowClaimMapper; +use OCA\OpenRegister\Db\FlowRun; +use OCA\OpenRegister\Db\FlowRunMapper; +use OCA\OpenRegister\Db\FlowRunStep; +use OCA\OpenRegister\Db\FlowRunStepMapper; +use OCA\OpenRegister\Db\FlowStream; +use OCA\OpenRegister\Db\FlowStreamMapper; +use OCA\OpenRegister\Event\FlowRunTerminalEvent; +use OCA\OpenRegister\Listener\FlowRunLockReleaseListener; +use OCA\OpenRegister\Service\Flow\FlowDefinitionBuilder; +use OCA\OpenRegister\Service\Flow\FlowEngine; +use OCA\OpenRegister\Service\Flow\FlowItems; +use OCA\OpenRegister\Service\Flow\FlowPlaceClaims; +use OCA\OpenRegister\Service\Flow\FlowRunCommit; +use OCA\OpenRegister\Service\Flow\FlowRunMarkingStore; +use OCA\OpenRegister\Service\Flow\FlowStepDispatcher; +use OCA\OpenRegister\Service\Flow\FlowStop; +use OCA\OpenRegister\Service\Flow\FlowStreamWalk; +use OCA\OpenRegister\Service\Flow\FlowSuspension; +use OCA\OpenRegister\Service\Object\RunLockRegistry; +use OCA\OpenRegister\Tests\Unit\Db\FluentQueryBuilderTrait; +use OCP\EventDispatcher\Event; +use OCP\EventDispatcher\IEventDispatcher; +use OCP\IDBConnection; +use PHPUnit\Framework\TestCase; +use Psr\Log\NullLogger; +use RuntimeException; +use stdClass; + +/** + * A mapper that reads its locked row from memory and writes through the real + * `update()` — the dispatch predicate under test. + */ +class InMemoryFlowRunMapper extends FlowRunMapper { + + /** + * Constructor. + * + * @param IDBConnection $db The (mocked) connection. + * @param IEventDispatcher $dispatcher The spy dispatcher. + * @param FlowRun $row The one run row. + */ + public function __construct( + IDBConnection $db, + IEventDispatcher $dispatcher, + private readonly FlowRun $row, + ) { + parent::__construct(db: $db, dispatcher: $dispatcher); + } + + /** + * The locked read, from memory. + * + * @param string $uuid The run uuid. + * + * @return FlowRun The row. + */ + public function lockByUuid(string $uuid): FlowRun { + return $this->row; + } +}//end class + +/** + * A dispatcher that suspends, stops or fails on a named step. + */ +class LockLifecycleDispatcher implements FlowStepDispatcher { + + /** + * Constructor. + * + * @param string|null $suspendOn Node id that parks the run. + * @param string|null $stopOn Node id that raises a FlowStop. + * @param string|null $failOn Node id that throws. + */ + public function __construct( + private readonly ?string $suspendOn = null, + private readonly ?string $stopOn = null, + private readonly ?string $failOn = null, + ) { + } + + /** + * Dispatch one step. + * + * @param array $step The step. + * @param array $items The items. + * @param array $context The context. + * + * @return array The items. + */ + public function dispatch(array $step, array $items, array $context): array { + $id = (string)($step['id'] ?? ''); + if ($id === $this->suspendOn) { + throw new FlowSuspension(resumeAt: new \DateTime('+5 minutes'), reason: 'waiting on a person'); + } + + if ($id === $this->stopOn) { + throw new FlowStop(reason: 'the author asked it to stop'); + } + + if ($id === $this->failOn) { + throw new RuntimeException('boom'); + } + + return $items; + } +}//end class + +/** + * Release layer 1, end to end. + * + * @covers \OCA\OpenRegister\Service\Flow\FlowRunCommit + * @covers \OCA\OpenRegister\Service\Flow\FlowStreamWalk + * @covers \OCA\OpenRegister\Listener\FlowRunLockReleaseListener + */ +class RunLockReleaseTerminalityTest extends TestCase { + use FluentQueryBuilderTrait; + + private const RUN = 'run-lock-1'; + + /** + * The run row. + */ + private FlowRun $row; + + /** + * The stream rows by id. + * + * @var array + */ + private array $streams = []; + + /** + * Every run uuid whose locks were released, in order. + * + * @var array + */ + private array $released = []; + + /** + * Every terminal status announced, in order. + * + * @var array + */ + private array $announced = []; + + /** + * The walk over the real commit path. + */ + private FlowStreamWalk $walk; + + /** + * The engine. + */ + private FlowEngine $engine; + + protected function setUp(): void { + parent::setUp(); + + $this->row = new FlowRun(); + $this->row->setId(1); + $this->row->setUuid(self::RUN); + $this->row->setFlowId('flow-1'); + $this->row->setStatus(FlowRun::STATUS_RUNNING); + $this->row->setFirings(0); + + $db = $this->connectionWith(); + + // The listener, wired to the dispatcher exactly as Application.php + // wires it, over a registry that records what it was asked to release. + $registry = $this->createMock(RunLockRegistry::class); + $registry->method('releaseRunLocks')->willReturnCallback(function (string $runUuid): int { + $this->released[] = $runUuid; + return 1; + }); + $listener = new FlowRunLockReleaseListener(locks: $registry, logger: new NullLogger()); + + $dispatcher = $this->createMock(IEventDispatcher::class); + $dispatcher->method('dispatchTyped')->willReturnCallback(function (Event $event) use ($listener): void { + if ($event instanceof FlowRunTerminalEvent === true) { + $this->announced[] = $event->getStatus(); + } + + $listener->handle($event); + }); + + $runs = new InMemoryFlowRunMapper(db: $db, dispatcher: $dispatcher, row: $this->row); + + $streams = $this->createMock(FlowStreamMapper::class); + $streams->method('findByRun')->willReturnCallback(function (): array { + $list = array_values($this->streams); + usort($list, static fn (FlowStream $a, FlowStream $b): int => strcmp((string)$a->getOrdinalPath(), (string)$b->getOrdinalPath())); + return $list; + }); + $streams->method('findByRunAndStream')->willReturnCallback(fn (string $runUuid, string $streamId): ?FlowStream => ($this->streams[$streamId] ?? null)); + $streams->method('insert')->willReturnCallback(function (FlowStream $stream): FlowStream { + $this->streams[(string)$stream->getStreamId()] = $stream; + return $stream; + }); + $streams->method('update')->willReturnCallback(function (FlowStream $stream): FlowStream { + $this->streams[(string)$stream->getStreamId()] = $stream; + return $stream; + }); + $streams->method('allocateNextSequence')->willReturnCallback(function (string $runUuid, string $streamId): int { + $stream = ($this->streams[$streamId] ?? null); + if ($stream === null) { + return 0; + } + + $next = (int)$stream->getNextSequence(); + $stream->setNextSequence(($next + 1)); + return $next; + }); + + $claimRows = []; + $claims = $this->createMock(FlowClaimMapper::class); + $claims->method('findByRun')->willReturnCallback(static fn (): array => array_values($claimRows)); + $claims->method('release')->willReturn(0); + $claims->method('releaseByOwner')->willReturn(0); + + $steps = $this->createMock(FlowRunStepMapper::class); + $steps->method('highestSequence')->willReturn(0); + $steps->method('insert')->willReturnCallback(static fn (FlowRunStep $step): FlowRunStep => $step); + + $commit = new FlowRunCommit( + db: $db, + runs: $runs, + streams: $streams, + claims: $claims, + steps: $steps, + logger: new NullLogger() + ); + + // Claims are not the property under test: every acquire succeeds. + $places = $this->createMock(FlowPlaceClaims::class); + $places->method('acquire')->willReturnCallback(static function (string $runUuid, string $streamId, string $transition, array $places): ?array { + sort($places, SORT_STRING); + return $places; + }); + + $this->walk = new FlowStreamWalk( + run: $this->row, + claims: $places, + commit: $commit, + streamMapper: $streams, + owner: 'pass-1' + ); + + $this->engine = new FlowEngine(new FlowDefinitionBuilder(), new NullLogger()); + }//end setUp() + + /** + * A three-node line: two ordinary steps and a third that the dispatcher + * decides the fate of. + * + * @return array The flow. + */ + private function flow(): array { + return [ + 'id' => 'lock-then-wait', + 'nodes' => [ + ['id' => 'start', 'type' => 'passthrough'], + ['id' => 'lock', 'type' => 'openregister.lock-object'], + ['id' => 'decide', 'type' => 'passthrough'], + ], + 'edges' => [ + ['id' => 'e1', 'from' => 'start', 'to' => 'lock'], + ['id' => 'e2', 'from' => 'lock', 'to' => 'decide'], + ], + ]; + }//end flow() + + /** + * Walk the flow. + * + * @param FlowStepDispatcher $dispatcher The dispatcher. + * + * @return array The result envelope. + */ + private function walkFlow(FlowStepDispatcher $dispatcher): array { + return $this->engine->run( + flow: $this->flow(), + store: new FlowRunMarkingStore(run: $this->row), + subject: new stdClass(), + dispatcher: $dispatcher, + context: [], + items: [FlowItems::item(json: ['n' => 1])], + startAt: null, + streams: $this->walk + ); + }//end walkFlow() + + /** + * A run that parks keeps every lock it holds. + * + * The run reaches `decide`, which waits for a person. Nothing about that + * is terminal, so no terminal event may be announced and the registry + * must not be asked to release anything — the case stays locked while its + * flow works, which is the entire point of a run-scoped lock. + * + * @return void + */ + public function testAParkedRunKeepsItsLocks(): void { + $result = $this->walkFlow(new LockLifecycleDispatcher(suspendOn: 'decide')); + + $this->assertSame(FlowRun::STATUS_SUSPENDED, $result['status']); + $this->assertSame(FlowRun::STATUS_SUSPENDED, (string)$this->row->getStatus()); + $this->assertFalse($this->row->isTerminal(), 'the run is not terminal'); + $this->assertSame([], $this->announced, 'a working run announced terminality: ' . implode(',', $this->announced)); + $this->assertSame([], $this->released, 'a parked run lost its locks'); + }//end testAParkedRunKeepsItsLocks() + + /** + * Every terminal outcome releases the run's locks — the four the + * production constant names, not a list restated here. + * + * @return void + */ + public function testEveryTerminalOutcomeReleasesTheLocks(): void { + $drivers = [ + FlowRun::STATUS_COMPLETED => static fn (): FlowStepDispatcher => new LockLifecycleDispatcher(), + FlowRun::STATUS_STOPPED => static fn (): FlowStepDispatcher => new LockLifecycleDispatcher(stopOn: 'decide'), + FlowRun::STATUS_FAILED => static fn (): FlowStepDispatcher => new LockLifecycleDispatcher(failOn: 'decide'), + FlowRun::STATUS_DEAD_LETTER => static fn (): FlowStepDispatcher => new LockLifecycleDispatcher(failOn: 'decide'), + ]; + + $this->assertSame( + [], + array_values(array_diff(FlowRun::TERMINAL, array_keys($drivers))), + 'a terminal status shipped with no driver here, so its release goes untested' + ); + $this->assertCount(count(FlowRun::TERMINAL), $drivers, 'a driver names a status that is not terminal'); + + foreach (FlowRun::TERMINAL as $status) { + $this->setUp(); + $flow = $this->flow(); + if ($status === FlowRun::STATUS_DEAD_LETTER) { + $flow['nodes'][2]['onError'] = FlowEngine::ON_ERROR_DEAD_LETTER; + } + + $result = $this->engine->run( + flow: $flow, + store: new FlowRunMarkingStore(run: $this->row), + subject: new stdClass(), + dispatcher: $drivers[$status](), + context: [], + items: [FlowItems::item(json: ['n' => 1])], + startAt: null, + streams: $this->walk + ); + + $this->assertSame($status, $result['status'], 'the walk did not reach ' . $status); + $this->assertTrue($this->row->isTerminal(), $status . ' left a non-terminal row'); + $this->assertContains($status, $this->announced, $status . ' was never announced'); + $this->assertContains(self::RUN, $this->released, $status . ' did not release the run\'s locks'); + } + }//end testEveryTerminalOutcomeReleasesTheLocks() +}//end class diff --git a/tests/Unit/Service/Object/SaveObjectTest.php b/tests/Unit/Service/Object/SaveObjectTest.php index 7e025e3496..7e2891be23 100644 --- a/tests/Unit/Service/Object/SaveObjectTest.php +++ b/tests/Unit/Service/Object/SaveObjectTest.php @@ -49,6 +49,13 @@ */ class SaveObjectTest extends TestCase { /** @var SaveObject */ + /** + * The ambient flow-run stack the guard reads. + * + * @var \OCA\OpenRegister\Service\Flow\FlowRunContext + */ + private \OCA\OpenRegister\Service\Flow\FlowRunContext $runContext; + private SaveObject $handler; /** @var MagicMapper&MockObject */ @@ -113,6 +120,7 @@ protected function setUp(): void { $arrayLoader = new ArrayLoader(); + $this->runContext = new \OCA\OpenRegister\Service\Flow\FlowRunContext(); $this->handler = new SaveObject( $this->objectEntityMapper, $this->unifiedObjectMapper, @@ -135,7 +143,15 @@ protected function setUp(): void { $this->logger, $this->createMock(\OCA\OpenRegister\Service\TmloService::class), $this->createMock(\OCA\OpenRegister\Service\File\FolderManagementHandler::class), - $arrayLoader + $arrayLoader, + null, + null, + null, + null, + null, + // The REAL ambient stack, so the lock guard is asked the same + // question production asks it: which run is writing. + $this->runContext ); } @@ -1868,6 +1884,112 @@ public function testFindAndValidateExistingObjectRefusesAPersonWhileARunHoldsThe ]); } + /** + * THE HOLDING RUN MAY WRITE TO THE OBJECT IT LOCKED. + * + * The guard used to ask only "which user", so a run that had just taken a + * lock was refused by its own lock at its next write — the lock step made + * the flow that used it unable to proceed. The run identity is ambient, + * exactly as attribution is, because the write is routinely several calls + * deep inside code that has never heard of flows. + * + * @return void + */ + public function testTheHoldingRunMayWriteToItsOwnLockedObject(): void { + $runUuid = 'run-aaaaaaaa-0000-0000-0000-000000000001'; + $entity = $this->lockedEntity('alice', $runUuid); + + $this->objectEntityMapper->method('find')->willReturn($entity); + + $user = $this->createMock(IUser::class); + $user->method('getUID')->willReturn('alice'); + $this->userSession->method('getUser')->willReturn($user); + + $this->runContext->push($runUuid, 'lock-object', 1); + try { + $result = $this->invokePrivateMethod('findAndValidateExistingObject', [ + 'test-uuid', null, null, false, false, + ]); + } finally { + $this->runContext->pop(); + } + + $this->assertSame($entity, $result, 'a run was refused by the lock it holds'); + } + + /** + * A DIFFERENT run is still refused, and the refusal names the holder. + * + * The control for the test above: admitting the holding run must not have + * been done by admitting every run. + * + * @return void + */ + public function testADifferentRunIsStillRefused(): void { + $holder = 'run-aaaaaaaa-0000-0000-0000-000000000001'; + $entity = $this->lockedEntity('alice', $holder); + + $this->objectEntityMapper->method('find')->willReturn($entity); + + $user = $this->createMock(IUser::class); + $user->method('getUID')->willReturn('alice'); + $this->userSession->method('getUser')->willReturn($user); + + $this->runContext->push('run-bbbbbbbb-0000-0000-0000-000000000002', 'lock-object', 1); + + // The refusal is captured rather than caught around a fail(): PHPUnit's + // fail() throws, and a catch broad enough for the guard's \Exception + // swallows it, so the test would report the wrong thing when it broke. + $refusal = null; + try { + $this->invokePrivateMethod('findAndValidateExistingObject', [ + 'test-uuid', null, null, false, false, + ]); + } catch (Exception $refused) { + $refusal = $refused; + } finally { + $this->runContext->pop(); + } + + $this->assertNotNull($refusal, 'a second run wrote through another run\'s lock'); + $this->assertStringContainsString($holder, $refusal->getMessage()); + } + + /** + * A PERSON'S LOCK SURVIVES A RUN PASSING OVER THE OBJECT. + * + * A run writing as the person who holds the lock is not the holder: the + * two are different kinds of holder and reading them as one is what let a + * flow take over, and then release, a lock a person was relying on. + * + * @return void + */ + public function testARunIsRefusedByAPersonsLockEvenWhenItRunsAsThatPerson(): void { + $entity = $this->lockedEntity('alice'); + + $this->objectEntityMapper->method('find')->willReturn($entity); + + $user = $this->createMock(IUser::class); + $user->method('getUID')->willReturn('alice'); + $this->userSession->method('getUser')->willReturn($user); + + $this->runContext->push('run-aaaaaaaa-0000-0000-0000-000000000001', 'object-write', 1); + + $refusal = null; + try { + $this->invokePrivateMethod('findAndValidateExistingObject', [ + 'test-uuid', null, null, false, false, + ]); + } catch (Exception $refused) { + $refusal = $refused; + } finally { + $this->runContext->pop(); + } + + $this->assertNotNull($refusal, 'a run wrote through a person\'s lock and would go on to take it over'); + $this->assertStringContainsString('alice', $refusal->getMessage()); + } + /** * An expired lock refuses nobody: the TTL backstop, at the write guard. */ From 1d73f67e30be465be8e16a8896a9ef6b31af20db Mon Sep 17 00:00:00 2001 From: Ruben van der Linde Date: Sat, 5 Sep 2026 17:06:47 +0200 Subject: [PATCH 2/3] fix(locking): trace the two icon methods to the palette requirement gate-16 counts a changed method with no `@spec` as an untraceable change, and these two are exactly the methods the palette requirement is about. --- lib/Service/Flow/Nodes/LockObjectNode.php | 2 ++ lib/Service/Flow/Nodes/UnlockObjectNode.php | 2 ++ 2 files changed, 4 insertions(+) diff --git a/lib/Service/Flow/Nodes/LockObjectNode.php b/lib/Service/Flow/Nodes/LockObjectNode.php index 1bb17172a8..af0a7f8f4a 100644 --- a/lib/Service/Flow/Nodes/LockObjectNode.php +++ b/lib/Service/Flow/Nodes/LockObjectNode.php @@ -193,6 +193,8 @@ public function getDescription(): string { * The palette icon. * * @return string The icon path. + * + * @spec openspec/changes/run-scoped-object-locking/specs/run-scoped-object-locking/spec.md#requirement-a-node-the-engine-registers-is-offered-in-the-palette */ public function getIcon(): string { // The APP's icon, not core's. `core/img/actions/lock.svg` does not diff --git a/lib/Service/Flow/Nodes/UnlockObjectNode.php b/lib/Service/Flow/Nodes/UnlockObjectNode.php index a01c87902b..075b531309 100644 --- a/lib/Service/Flow/Nodes/UnlockObjectNode.php +++ b/lib/Service/Flow/Nodes/UnlockObjectNode.php @@ -123,6 +123,8 @@ public function getDescription(): string { * The palette icon. * * @return string The icon path. + * + * @spec openspec/changes/run-scoped-object-locking/specs/run-scoped-object-locking/spec.md#requirement-a-node-the-engine-registers-is-offered-in-the-palette */ public function getIcon(): string { // The APP's icon: see LockObjectNode::getIcon(). `actions/unlock.svg` From 3c0a9a8332f7d75aee75496c4f7f2b0b38a8aeab Mon Sep 17 00:00:00 2001 From: Ruben van der Linde Date: Sat, 5 Sep 2026 17:16:23 +0200 Subject: [PATCH 3/3] test(locking): say why each terminal leg rebuilds the harness --- tests/Unit/Service/Flow/RunLockReleaseTerminalityTest.php | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/Unit/Service/Flow/RunLockReleaseTerminalityTest.php b/tests/Unit/Service/Flow/RunLockReleaseTerminalityTest.php index 7aa80b37d0..e14bb38f32 100644 --- a/tests/Unit/Service/Flow/RunLockReleaseTerminalityTest.php +++ b/tests/Unit/Service/Flow/RunLockReleaseTerminalityTest.php @@ -369,6 +369,9 @@ public function testEveryTerminalOutcomeReleasesTheLocks(): void { $this->assertCount(count(FlowRun::TERMINAL), $drivers, 'a driver names a status that is not terminal'); foreach (FlowRun::TERMINAL as $status) { + // A fresh run, walk and commit per outcome. Reusing one would let + // an earlier leg's release satisfy a later leg's assertion, which + // is the failure this whole file exists to rule out. $this->setUp(); $flow = $this->flow(); if ($status === FlowRun::STATUS_DEAD_LETTER) {