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

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

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

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

* test(locking): say why each terminal leg rebuilds the harness

Author: rubenvdlinde

img/lock.svg

@@ -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

img/unlock.svg

@@ -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/Controller/ObjectsController.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/Db/ObjectEntity.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/FlowEngine.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/FlowNodeRegistry.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