fix(locking): a parked run keeps its locks, and the lock nodes exist in the editor

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

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()
 

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
 		);

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,19 +258,75 @@ 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
 
 		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.
 	 *

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.
 	 *

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<int, object> $transitions The enabled transitions.
+	 * @param array<int, string> $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<int, string> $produced The places the commit is about to mark.
+	 * @param string|null $settling A stream whose place stops counting.
+	 *
+	 * @return array<string, true> 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