Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 56 additions & 0 deletions lib/Service/Flow/FlowOversightRegistry.php
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@

namespace OCA\OpenRegister\Service\Flow;

use OCP\EventDispatcher\IEventDispatcher;
use Psr\Log\LoggerInterface;
use Throwable;

Expand All @@ -45,17 +46,68 @@ class FlowOversightRegistry {
*/
private array $checks = [];

/**
* Whether contributions have been collected.
*
* @var boolean
*/
private bool $discovered = false;

/**
* Constructor.
*
* @param LoggerInterface $logger Records a check that misbehaves.
* @param IEventDispatcher|null $dispatcher Collects contributed checks by
* dispatching
* {@see RegisterFlowOversightEvent},
* the same way FlowNodeRegistry
* collects node types. Nullable so
* the registry stays constructible
* without a container — a test that
* registers its checks by hand needs
* no discovery — and defaulted LAST
* so existing positional
* constructions keep meaning what
* they meant.
*/
public function __construct(
private readonly LoggerInterface $logger,
private readonly ?IEventDispatcher $dispatcher = null,
) {

}//end __construct()

/**
* Collect contributed checks once, lazily.
*
* THE GAP THIS CLOSES: the listeners for {@see RegisterFlowOversightEvent}
* were registered on every boot, but nothing ever DISPATCHED the event —
* `FlowNodeRegistry` dispatches its own registration event before first
* use, and this registry had no equivalent. The result was an oversight
* gate that was consulted on every hop and could never hold a check:
* `firstRefusal()` iterated an empty list and consented, so the instance
* kill switch (and every app-contributed check) was decorative. Observed
* live 2026-09-01: `flow_kill_switch=1` plus a resume left a suspended run
* suspended instead of stopping it, because no veto ever fired.
*
* Set BEFORE dispatching, exactly as FlowNodeRegistry::load() does: a
* listener that resolves a service which itself consults this registry
* would otherwise re-enter and dispatch again.
*
* @return void
*
* @spec openspec/changes/flow-engine-unification/specs/flow-oversight/spec.md
*/
private function discover(): void {
if ($this->discovered === true || $this->dispatcher === null) {
return;
}

$this->discovered = true;
$this->dispatcher->dispatchTyped(new RegisterFlowOversightEvent(registry: $this));

}//end discover()

/**
* Register an oversight check.
*
Expand All @@ -82,6 +134,8 @@ public function register(IFlowOversightCheck $check): void {
* @spec openspec/changes/flow-engine-unification/specs/flow-oversight/spec.md
*/
public function all(): array {
$this->discover();

return $this->checks;
}//end all()

Expand All @@ -100,6 +154,8 @@ public function all(): array {
* @spec openspec/changes/flow-engine-unification/specs/flow-oversight/spec.md
*/
public function firstRefusal(array $context): ?array {
$this->discover();

foreach ($this->checks as $id => $check) {
try {
$reason = $check->veto(context: $context);
Expand Down
31 changes: 27 additions & 4 deletions lib/Service/Flow/Nodes/WaitNode.php
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,11 @@
*
* Because the marking does not advance, this node runs a SECOND time when the
* run resumes. That is what makes it correct rather than a one-shot: on the
* way back in it sees `context.resuming` and lets the items straight through.
* way back in it sees its OWN resume slot held and lets the items straight
* through. Its own slot, not `context.resuming` — the run-wide flag is true
* for every node of a resumed walk, so a SECOND wait node reached later in
* the same walk would read it as "my wait is over" and pass through without
* ever having waited. The slot marks exactly the node that suspended.
*
* SPDX-FileCopyrightText: 2026 Conduction B.V. <info@conduction.nl>
* SPDX-License-Identifier: EUPL-1.2
Expand All @@ -31,6 +35,7 @@
namespace OCA\OpenRegister\Service\Flow\Nodes;

use DateTime;
use OCA\OpenRegister\Service\Flow\FlowNodeResumeState;
use OCA\OpenRegister\Service\Flow\FlowSuspension;
use OCA\OpenRegister\Service\Flow\IFlowNode;
use OCA\OpenRegister\Service\Flow\IFlowNodeConfigKeys;
Expand Down Expand Up @@ -175,9 +180,19 @@ public function execute(array $items, array $config, array $context): array {
return $items;
}

if (($context['resuming'] ?? false) === true) {
// Woken by the worker: the wait is over by construction, because
// the run was only eligible once `resumeAt` had passed.
$resume = ($context[FlowNodeResumeState::CONTEXT_KEY] ?? null);
if ($resume instanceof FlowNodeResumeState === true) {
if ($resume->isResuming() === true) {
// THIS node's wait is over: it wrote its slot when it
// suspended, and the run was only eligible again once its
// `resumeAt` had passed. The dispatcher clears the slot on
// return, so a loop back into this node waits again.
return $items;
}
} elseif (($context['resuming'] ?? false) === true) {
// No slot machinery at all — a context built outside a real run
// (the flow tester, a node unit test). There is only one wait in
// such a walk, so the run-wide flag is unambiguous there.
return $items;
}

Expand All @@ -189,6 +204,14 @@ public function execute(array $items, array $config, array $context): array {
return $items;
}

// The slot is the addressee mark the re-entry above reads. Without it,
// a resumed walk cannot tell the wait that is OVER from a wait it has
// only just reached — `context.resuming` is true for both, and reading
// that flag made a second wait node pass through in zero seconds.
if ($resume instanceof FlowNodeResumeState === true) {
$resume->set(key: 'waitingUntil', value: $resumeAt->format('c'));
}

throw new FlowSuspension(
resumeAt: $resumeAt,
reason: sprintf('waiting until %s', $resumeAt->format('c'))
Expand Down
57 changes: 57 additions & 0 deletions lib/Service/Flow/RegistryStepDispatcher.php
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,12 @@ public function dispatch(array $step, array $items, array $context): array {
// and cannot reach another node's slot even by accident.
$scoped = $this->scopeResumeState(step: $step, context: $context);

// Scope the resume SIGNAL the same way. Without this, the payload that
// answered ONE node's question is readable by every wait node the
// resumed walk goes on to enter, and each of them completes on somebody
// else's answer instead of suspending with a question of its own.
$this->scopeSignal(context: $context, scoped: $scoped);

$startedAt = microtime(true);
$out = $node->execute(items: $items, config: $config, context: $context);
$tookMs = (int)round((microtime(true) - $startedAt) * 1000);
Expand Down Expand Up @@ -159,6 +165,57 @@ private function scopeResumeState(array $step, array &$context): ?FlowNodeResume
return $scoped;
}//end scopeResumeState()

/**
* Withhold the resume signal from every node except the one it answers.
*
* A signal wakes a RUN, but it answers one NODE: the one that suspended and
* whose resume slot is still held. The walk's context is shared, so without
* this gate every wait node the resumed walk re-enters AFTER the answered
* one reads the same payload as its own answer and completes instead of
* suspending — a flow with two approval steps auto-approves the second the
* moment the first is granted, and a decision step downstream of an answered
* task adopts the task's payload as its decision outcome. Observed live on
* dossiq case flows (runs f8996ccc and ca50c56c, 2026-09-01): a DECISION
* node's outcome held `{decision, node: "ask-indiener", taskId}` — an
* applicant task's completion — and a second decision node inherited the
* first decision's reference because it never suspended at all.
*
* The slot is the addressee test, not `$context['resuming']`: the run-wide
* flag is true for every node of a resumed walk, while a held slot marks
* exactly the node that suspended and has not yet been given its answer.
* The dispatcher clears the slot when a node returns, so a wait node
* re-entered later in the SAME walk (a loop) asks fresh rather than
* re-reading a consumed answer. With no slot machinery at all (a
* container-built dispatcher walking a tester context) the signal is
* withheld too: with no slots there is no addressee, and withholding makes
* the node suspend visibly where delivering would answer the wrong
* question silently.
*
* The strip is LOCAL to this node's context copy — `dispatch()` receives
* `$context` by value — so the walk keeps carrying the signal to the node
* whose slot it answers, wherever in the round-robin that node is visited.
*
* @param array $context The node context, modified in place.
* @param FlowNodeResumeState|null $scoped This node's resume slot, when it has one.
*
* @return void
*
* @spec openspec/specs/flow-engine/spec.md#requirement-a-run-suspended-on-an-external-signal-must-be-reachable
*/
private function scopeSignal(array &$context, ?FlowNodeResumeState $scoped): void {
if (array_key_exists(FlowRunService::SIGNAL_CONTEXT_KEY, $context) === false) {
return;
}

if ($scoped !== null && $scoped->isResuming() === true) {
// This node is the one that suspended: the answer is its to read.
return;
}

unset($context[FlowRunService::SIGNAL_CONTEXT_KEY]);

}//end scopeSignal()

/**
* Stop a step that took longer than its own `maxRuntimeSeconds`.
*
Expand Down
Loading
Loading