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..af0a7f8f4a 100644
--- a/lib/Service/Flow/Nodes/LockObjectNode.php
+++ b/lib/Service/Flow/Nodes/LockObjectNode.php
@@ -193,9 +193,15 @@ 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 {
- 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..075b531309 100644
--- a/lib/Service/Flow/Nodes/UnlockObjectNode.php
+++ b/lib/Service/Flow/Nodes/UnlockObjectNode.php
@@ -123,9 +123,13 @@ 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 {
- 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..e14bb38f32
--- /dev/null
+++ b/tests/Unit/Service/Flow/RunLockReleaseTerminalityTest.php
@@ -0,0 +1,398 @@
+
+ * 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) {
+ // 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) {
+ $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.
*/