From adc16da4f19c7ea3f99277a18a5d9ea6f170ff11 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 15 Apr 2026 03:45:09 +0000 Subject: [PATCH 1/3] Fix critical bugs in WorkflowBuilder, condition evaluation, and error handling The fluent WorkflowBuilder API never generated transitions between steps, so the Executor's transition-driven navigation stopped after the first step and silently marked multi-step workflows as completed. Builder-based step conditions were also stored on the Step but never consulted during execution, and ConditionEvaluator compared every right-hand side as a string, breaking strict comparisons against booleans, integers, nulls, and other typed values. Error handling caught Exception rather than Throwable, leaving instances in an inconsistent state when actions threw Errors, and classified exceptions by substring-matching their messages. - WorkflowBuilder::addStep now appends a transition from the previous step so sequential builder workflows actually chain. - when() conditions accumulate into Step::$conditions and are evaluated in Executor::executeStep; steps whose conditions don't match are marked complete (skipped) so downstream transitions still flow. - ConditionEvaluator parses RHS literals by type (bool/null/int/float/ quoted string) and supports the truthy-key form ("review.approved", "!review.approved") used by QuickWorkflowBuilder. - Executor now catches Throwable in execute(), executeStep(), the retry loop, and the timeout wrapper; WorkflowFailedEvent and StepExecutionException::fromException accept Throwable. - Removed the brittle str_contains exception classifier; rely on the typed exceptions thrown by executeAction(). Adds tests covering multi-step builder execution, conditional builder branches, and the typed ConditionEvaluator behavior. Test count goes from 93 to 107 (259 assertions). --- src/Core/Executor.php | 40 ++++-- src/Core/WorkflowBuilder.php | 15 ++- src/Core/WorkflowEngine.php | 3 +- src/Events/WorkflowFailedEvent.php | 2 +- src/Exceptions/StepExecutionException.php | 6 +- src/Support/ConditionEvaluator.php | 108 +++++++++++++--- tests/Integration/EventDispatchTest.php | 9 +- tests/Integration/WorkflowIntegrationTest.php | 14 +- tests/Unit/BuilderExecutionTest.php | 121 ++++++++++++++++++ tests/Unit/ConditionEvaluatorTest.php | 80 ++++++++++++ tests/Unit/ExecutorRetryTest.php | 4 +- tests/Unit/WorkflowEngineTest.php | 7 +- 12 files changed, 356 insertions(+), 53 deletions(-) create mode 100644 tests/Unit/BuilderExecutionTest.php create mode 100644 tests/Unit/ConditionEvaluatorTest.php diff --git a/src/Core/Executor.php b/src/Core/Executor.php index 89f4477..38677b6 100644 --- a/src/Core/Executor.php +++ b/src/Core/Executor.php @@ -2,7 +2,6 @@ namespace SolutionForest\WorkflowEngine\Core; -use Exception; use SolutionForest\WorkflowEngine\Contracts\EventDispatcher; use SolutionForest\WorkflowEngine\Contracts\Logger; use SolutionForest\WorkflowEngine\Contracts\WorkflowAction; @@ -121,7 +120,7 @@ public function execute(WorkflowInstance $instance): void { try { $this->processWorkflow($instance); - } catch (Exception $e) { + } catch (\Throwable $e) { $this->logger->error('Workflow execution failed', [ 'workflow_id' => $instance->getId(), 'workflow_name' => $instance->getDefinition()->getName(), @@ -134,7 +133,7 @@ public function execute(WorkflowInstance $instance): void $this->stateManager->setError($instance, $e->getMessage()); $this->eventDispatcher->dispatch(new WorkflowFailedEvent($instance, $e)); - // Re-throw the original exception to maintain the error context + // Re-throw the original throwable to maintain the error context throw $e; } } @@ -206,6 +205,23 @@ private function processWorkflow(WorkflowInstance $instance): void */ private function executeStep(WorkflowInstance $instance, Step $step): void { + // Evaluate step conditions. Steps whose conditions don't match the current + // workflow data are skipped (marked completed without running the action) + // so that downstream transitions continue to flow. + if (! $step->canExecute($instance->getData())) { + $this->logger->info('Skipping workflow step; conditions not met', [ + 'workflow_id' => $instance->getId(), + 'step_id' => $step->getId(), + 'conditions' => $step->getConditions(), + ]); + + $instance->setCurrentStepId($step->getId()); + $this->stateManager->markStepCompleted($instance, $step->getId()); + $this->processWorkflow($instance); + + return; + } + $this->logger->info('Executing workflow step', [ 'workflow_id' => $instance->getId(), 'workflow_name' => $instance->getDefinition()->getName(), @@ -234,7 +250,7 @@ private function executeStep(WorkflowInstance $instance, Step $step): void // Continue execution recursively $this->processWorkflow($instance); - } catch (Exception $e) { + } catch (\Throwable $e) { $context = new WorkflowContext( workflowId: $instance->getId(), stepId: $step->getId(), @@ -243,13 +259,11 @@ private function executeStep(WorkflowInstance $instance, Step $step): void instance: $instance ); - // Create detailed step execution exception - $stepException = match (true) { - $e instanceof ActionNotFoundException => $e, - str_contains($e->getMessage(), 'does not exist') => ActionNotFoundException::classNotFound($step->getActionClass(), $step, $context), - str_contains($e->getMessage(), 'must implement') => ActionNotFoundException::invalidInterface($step->getActionClass(), $step, $context), - default => StepExecutionException::fromException($e, $step, $context) - }; + // Wrap non-typed throwables in a StepExecutionException while preserving + // ActionNotFoundException (and other domain exceptions) as-is. + $stepException = $e instanceof ActionNotFoundException + ? $e + : StepExecutionException::fromException($e, $step, $context); $this->logger->error('Workflow step execution failed', [ 'workflow_id' => $instance->getId(), @@ -296,7 +310,7 @@ private function executeActionWithRetry(WorkflowInstance $instance, Step $step): $this->executeAction($instance, $step); return; // Success — exit retry loop - } catch (\Exception $e) { + } catch (\Throwable $e) { $lastException = $e; if ($attempt === $maxAttempts) { @@ -366,7 +380,7 @@ private function executeWithTimeout(callable $callback, int $timeoutSeconds): mi pcntl_alarm(0); return $result; - } catch (\Exception $e) { + } catch (\Throwable $e) { pcntl_alarm(0); throw $e; diff --git a/src/Core/WorkflowBuilder.php b/src/Core/WorkflowBuilder.php index 80902c3..0d0377e 100644 --- a/src/Core/WorkflowBuilder.php +++ b/src/Core/WorkflowBuilder.php @@ -200,6 +200,12 @@ public function addStep( } } + // Auto-chain sequential steps by appending a transition from the previous step. + if (! empty($this->steps)) { + $previousId = $this->steps[array_key_last($this->steps)]['id']; + $this->transitions[] = ['from' => $previousId, 'to' => $id]; + } + $this->steps[] = [ 'id' => $id, 'action' => is_string($action) ? $action : $action::class, @@ -288,9 +294,12 @@ public function when(string $condition, callable $callback): self $callback($this); $newStepsCount = count($this->steps); - // Mark new steps as conditional + // Mark new steps as conditional. Nested when() calls accumulate conditions + // so all must be true for the step to execute. for ($i = $originalStepsCount; $i < $newStepsCount; $i++) { - $this->steps[$i]['condition'] = $condition; + $existing = $this->steps[$i]['conditions'] ?? []; + $existing[] = $condition; + $this->steps[$i]['conditions'] = $existing; } return $this; @@ -486,7 +495,7 @@ public function build(): WorkflowDefinition config: $stepData['config'], timeout: $timeoutString, retryAttempts: $stepData['retry_attempts'], - conditions: isset($stepData['condition']) ? [$stepData['condition']] : [] + conditions: $stepData['conditions'] ?? [] ); } diff --git a/src/Core/WorkflowEngine.php b/src/Core/WorkflowEngine.php index d992caf..9852da2 100644 --- a/src/Core/WorkflowEngine.php +++ b/src/Core/WorkflowEngine.php @@ -6,6 +6,7 @@ use SolutionForest\WorkflowEngine\Contracts\StorageAdapter; use SolutionForest\WorkflowEngine\Events\WorkflowCancelledEvent; use SolutionForest\WorkflowEngine\Events\WorkflowStartedEvent; +use SolutionForest\WorkflowEngine\Exceptions\InvalidWorkflowDefinitionException; use SolutionForest\WorkflowEngine\Exceptions\InvalidWorkflowStateException; use SolutionForest\WorkflowEngine\Exceptions\WorkflowInstanceNotFoundException; @@ -94,7 +95,7 @@ public function __construct( * @param array $context Initial context data for the workflow * @return string The workflow instance ID * - * @throws \SolutionForest\WorkflowEngine\Exceptions\InvalidWorkflowDefinitionException If the workflow definition is invalid + * @throws InvalidWorkflowDefinitionException If the workflow definition is invalid * @throws \RuntimeException If the workflow cannot be started due to system issues * * @example Starting a simple workflow diff --git a/src/Events/WorkflowFailedEvent.php b/src/Events/WorkflowFailedEvent.php index 1cdd44a..ec82c07 100644 --- a/src/Events/WorkflowFailedEvent.php +++ b/src/Events/WorkflowFailedEvent.php @@ -8,7 +8,7 @@ { public function __construct( public WorkflowInstance $instance, - public \Exception $exception, + public \Throwable $exception, ) {} public function getWorkflowId(): string diff --git a/src/Exceptions/StepExecutionException.php b/src/Exceptions/StepExecutionException.php index 55ef961..1ca185d 100644 --- a/src/Exceptions/StepExecutionException.php +++ b/src/Exceptions/StepExecutionException.php @@ -198,14 +198,14 @@ public static function timeout( } /** - * Create a StepExecutionException from any other exception. + * Create a StepExecutionException from any other throwable. * - * @param \Exception $exception The original exception + * @param \Throwable $exception The original throwable * @param Step $step The step that failed * @param WorkflowContext $context The execution context */ public static function fromException( - \Exception $exception, + \Throwable $exception, Step $step, WorkflowContext $context ): static { diff --git a/src/Support/ConditionEvaluator.php b/src/Support/ConditionEvaluator.php index 72a7739..18ee24b 100644 --- a/src/Support/ConditionEvaluator.php +++ b/src/Support/ConditionEvaluator.php @@ -9,7 +9,12 @@ final class ConditionEvaluator /** * Evaluate a condition expression against workflow data. * - * @param string $condition Condition expression (e.g., "user.plan === premium") + * Supports two forms: + * - Comparison: "key operator value" where operator is one of ===, !==, >=, <=, ==, !=, >, < + * and value is a boolean, null, integer, float, or quoted string literal. + * - Truthy key: "key" or "!key" to check whether the dotted key is truthy/falsy. + * + * @param string $condition Condition expression (e.g., "user.plan === 'premium'") * @param array $data Workflow data to evaluate against * @return bool True if condition evaluates to true * @@ -17,29 +22,96 @@ final class ConditionEvaluator */ public static function evaluate(string $condition, array $data): bool { - if (! preg_match('/(\w+(?:\.\w+)*)\s*(===|!==|>=|<=|==|!=|>|<)\s*(.+)/', $condition, $matches)) { + $trimmed = trim($condition); + + if ($trimmed === '') { throw InvalidWorkflowDefinitionException::invalidCondition( $condition, - 'Condition must be in format: "key operator value" (e.g., "user.plan === premium")' + 'Condition cannot be empty.' ); } - $key = $matches[1]; - $operator = $matches[2]; - $value = trim($matches[3], '"\''); + // Comparison form: "key operator value". + if (preg_match('/^(\w+(?:\.\w+)*)\s*(===|!==|>=|<=|==|!=|>|<)\s*(.+)$/', $trimmed, $matches)) { + $key = $matches[1]; + $operator = $matches[2]; + $rawValue = trim($matches[3]); + + $value = self::parseLiteral($rawValue, $condition); + $dataValue = Arr::get($data, $key); + + return match ($operator) { + '===' => $dataValue === $value, + '!==' => $dataValue !== $value, + '>=' => $dataValue >= $value, + '<=' => $dataValue <= $value, + '==' => $dataValue == $value, + '!=' => $dataValue != $value, + '>' => $dataValue > $value, + '<' => $dataValue < $value, + default => false, + }; + } + + // Truthy key form: "key" or "!key". + if (preg_match('/^(!?)(\w+(?:\.\w+)*)$/', $trimmed, $matches)) { + $negate = $matches[1] === '!'; + $dataValue = Arr::get($data, $matches[2]); + + return $negate ? ! $dataValue : (bool) $dataValue; + } + + throw InvalidWorkflowDefinitionException::invalidCondition( + $condition, + 'Condition must be a truthy key (e.g. "user.active") or "key operator value" (e.g. "user.plan === \'premium\'").' + ); + } - $dataValue = Arr::get($data, $key); + /** + * Parse a literal value from its string form into a typed PHP value. + * + * Supports: true/false, null, integers, floats, and quoted strings. + * Unquoted identifiers are returned as strings for backwards compatibility. + */ + private static function parseLiteral(string $raw, string $condition): mixed + { + if ($raw === '') { + throw InvalidWorkflowDefinitionException::invalidCondition( + $condition, + 'Right-hand side of the comparison is empty.' + ); + } + + $lower = strtolower($raw); + if ($lower === 'true') { + return true; + } + if ($lower === 'false') { + return false; + } + if ($lower === 'null') { + return null; + } + + // Quoted strings. + $len = strlen($raw); + if ($len >= 2) { + $first = $raw[0]; + $last = $raw[$len - 1]; + if (($first === '"' && $last === '"') || ($first === "'" && $last === "'")) { + return substr($raw, 1, -1); + } + } + + // Numeric literals. + if (preg_match('/^-?\d+$/', $raw)) { + return (int) $raw; + } + if (preg_match('/^-?\d+\.\d+$/', $raw)) { + return (float) $raw; + } - return match ($operator) { - '===' => $dataValue === $value, - '!==' => $dataValue !== $value, - '>=' => $dataValue >= $value, - '<=' => $dataValue <= $value, - '==' => $dataValue == $value, - '!=' => $dataValue != $value, - '>' => $dataValue > $value, - '<' => $dataValue < $value, - default => false, - }; + // Fallback: treat as an unquoted string (backwards compatible). + return $raw; } } diff --git a/tests/Integration/EventDispatchTest.php b/tests/Integration/EventDispatchTest.php index 45e822d..02390f5 100644 --- a/tests/Integration/EventDispatchTest.php +++ b/tests/Integration/EventDispatchTest.php @@ -1,6 +1,9 @@ parse($definition); $id = 'event-test-4'; - $instance = new \SolutionForest\WorkflowEngine\Core\WorkflowInstance( + $instance = new WorkflowInstance( id: $id, definition: $workflowDef, - state: \SolutionForest\WorkflowEngine\Core\WorkflowState::RUNNING, + state: WorkflowState::RUNNING, ); $storage->save($instance); diff --git a/tests/Integration/WorkflowIntegrationTest.php b/tests/Integration/WorkflowIntegrationTest.php index 40317cc..f5ae80d 100644 --- a/tests/Integration/WorkflowIntegrationTest.php +++ b/tests/Integration/WorkflowIntegrationTest.php @@ -1,5 +1,7 @@ parse($definition); $workflowId = 'cancellable-workflow'; - $instance = new \SolutionForest\WorkflowEngine\Core\WorkflowInstance( + $instance = new WorkflowInstance( id: $workflowId, definition: $workflowDef, - state: \SolutionForest\WorkflowEngine\Core\WorkflowState::RUNNING, + state: WorkflowState::RUNNING, ); $this->storage->save($instance); @@ -141,13 +143,13 @@ $workflow1Id = $this->engine->start('list-test-1', $definition1); // For the second one, create it in RUNNING state so we can cancel it - $parser = new \SolutionForest\WorkflowEngine\Core\DefinitionParser; + $parser = new DefinitionParser; $workflowDef = $parser->parse($definition2); $workflow2Id = 'list-test-2'; - $instance = new \SolutionForest\WorkflowEngine\Core\WorkflowInstance( + $instance = new WorkflowInstance( id: $workflow2Id, definition: $workflowDef, - state: \SolutionForest\WorkflowEngine\Core\WorkflowState::RUNNING, + state: WorkflowState::RUNNING, ); $this->storage->save($instance); diff --git a/tests/Unit/BuilderExecutionTest.php b/tests/Unit/BuilderExecutionTest.php new file mode 100644 index 0000000..3f1585c --- /dev/null +++ b/tests/Unit/BuilderExecutionTest.php @@ -0,0 +1,121 @@ + */ + public static array $executed = []; + + public static function reset(): void + { + self::$executed = []; + } + + protected function doExecute(WorkflowContext $context): ActionResult + { + self::$executed[] = $this->getConfig('label', $context->getStepId()); + + return ActionResult::success(); + } + + public function getName(): string + { + return 'Recording Action'; + } + + public function getDescription(): string + { + return 'Test action that records its execution order.'; + } +} + +describe('WorkflowBuilder → Executor end-to-end', function () { + beforeEach(function () { + RecordingAction::reset(); + $this->storage = new InMemoryStorage; + $this->engine = new WorkflowEngine($this->storage); + }); + + test('a builder-defined multi-step workflow executes every step in order', function () { + $definition = WorkflowBuilder::create('multi-step') + ->addStep('first', RecordingAction::class, ['label' => 'first']) + ->addStep('second', RecordingAction::class, ['label' => 'second']) + ->addStep('third', RecordingAction::class, ['label' => 'third']) + ->build(); + + $id = $this->engine->start('multi-step-1', $definition->toArray(), []); + $instance = $this->engine->getInstance($id); + + expect($instance->getState())->toBe(WorkflowState::COMPLETED); + expect(RecordingAction::$executed)->toBe(['first', 'second', 'third']); + expect($instance->getCompletedSteps())->toBe(['first', 'second', 'third']); + }); + + test('then() chains execute every step', function () { + $definition = WorkflowBuilder::create('then-chain') + ->startWith(RecordingAction::class, ['label' => 'a']) + ->then(RecordingAction::class, ['label' => 'b']) + ->then(RecordingAction::class, ['label' => 'c']) + ->build(); + + $id = $this->engine->start('then-chain-1', $definition->toArray(), []); + $instance = $this->engine->getInstance($id); + + expect($instance->getState())->toBe(WorkflowState::COMPLETED); + expect(RecordingAction::$executed)->toBe(['a', 'b', 'c']); + }); + + test('when() conditions skip steps whose condition is false and run steps whose condition is true', function () { + $definition = WorkflowBuilder::create('conditional') + ->addStep('start', RecordingAction::class, ['label' => 'start']) + ->when("user.plan === 'premium'", function ($builder) { + $builder->addStep('premium_only', RecordingAction::class, ['label' => 'premium_only']); + }) + ->when("user.plan === 'free'", function ($builder) { + $builder->addStep('free_only', RecordingAction::class, ['label' => 'free_only']); + }) + ->addStep('finish', RecordingAction::class, ['label' => 'finish']) + ->build(); + + $id = $this->engine->start('conditional-1', $definition->toArray(), [ + 'user' => ['plan' => 'premium'], + ]); + $instance = $this->engine->getInstance($id); + + expect($instance->getState())->toBe(WorkflowState::COMPLETED); + expect(RecordingAction::$executed)->toBe(['start', 'premium_only', 'finish']); + // Skipped steps are still marked completed so transitions flow. + expect($instance->getCompletedSteps())->toContain('free_only'); + }); + + test('truthy when() keys work against boolean data', function () { + $definition = WorkflowBuilder::create('truthy-when') + ->addStep('start', RecordingAction::class, ['label' => 'start']) + ->when('review.approved', function ($builder) { + $builder->addStep('approve', RecordingAction::class, ['label' => 'approve']); + }) + ->when('!review.approved', function ($builder) { + $builder->addStep('reject', RecordingAction::class, ['label' => 'reject']); + }) + ->build(); + + $id = $this->engine->start('truthy-when-1', $definition->toArray(), [ + 'review' => ['approved' => true], + ]); + $instance = $this->engine->getInstance($id); + + expect($instance->getState())->toBe(WorkflowState::COMPLETED); + expect(RecordingAction::$executed)->toBe(['start', 'approve']); + }); +}); diff --git a/tests/Unit/ConditionEvaluatorTest.php b/tests/Unit/ConditionEvaluatorTest.php new file mode 100644 index 0000000..d7197b7 --- /dev/null +++ b/tests/Unit/ConditionEvaluatorTest.php @@ -0,0 +1,80 @@ + ['age' => 25]]; + + expect(ConditionEvaluator::evaluate('user.age === 25', $data))->toBeTrue(); + expect(ConditionEvaluator::evaluate('user.age !== 25', $data))->toBeFalse(); + expect(ConditionEvaluator::evaluate('user.age === 26', $data))->toBeFalse(); + }); + + test('strict equality works against boolean data', function () { + $data = ['user' => ['premium' => true, 'verified' => false]]; + + expect(ConditionEvaluator::evaluate('user.premium === true', $data))->toBeTrue(); + expect(ConditionEvaluator::evaluate('user.verified === false', $data))->toBeTrue(); + expect(ConditionEvaluator::evaluate('user.premium === false', $data))->toBeFalse(); + }); + + test('strict equality works against null data', function () { + $data = ['token' => null]; + + expect(ConditionEvaluator::evaluate('token === null', $data))->toBeTrue(); + expect(ConditionEvaluator::evaluate('token !== null', $data))->toBeFalse(); + }); + + test('strict equality works against quoted strings', function () { + $data = ['user' => ['plan' => 'premium']]; + + expect(ConditionEvaluator::evaluate("user.plan === 'premium'", $data))->toBeTrue(); + expect(ConditionEvaluator::evaluate('user.plan === "premium"', $data))->toBeTrue(); + expect(ConditionEvaluator::evaluate("user.plan === 'free'", $data))->toBeFalse(); + }); + + test('numeric comparison operators work with float data', function () { + $data = ['order' => ['total' => 1500.50]]; + + expect(ConditionEvaluator::evaluate('order.total > 1000', $data))->toBeTrue(); + expect(ConditionEvaluator::evaluate('order.total > 1500.50', $data))->toBeFalse(); + expect(ConditionEvaluator::evaluate('order.total >= 1500.50', $data))->toBeTrue(); + expect(ConditionEvaluator::evaluate('order.total < 2000', $data))->toBeTrue(); + expect(ConditionEvaluator::evaluate('order.total <= 1500.50', $data))->toBeTrue(); + }); + + test('truthy key form evaluates a dotted path to bool', function () { + $data = ['review' => ['approved' => true, 'rejected' => false, 'notes' => '']]; + + expect(ConditionEvaluator::evaluate('review.approved', $data))->toBeTrue(); + expect(ConditionEvaluator::evaluate('review.rejected', $data))->toBeFalse(); + expect(ConditionEvaluator::evaluate('review.notes', $data))->toBeFalse(); + expect(ConditionEvaluator::evaluate('review.missing', $data))->toBeFalse(); + }); + + test('negated truthy key form inverts the result', function () { + $data = ['review' => ['approved' => false]]; + + expect(ConditionEvaluator::evaluate('!review.approved', $data))->toBeTrue(); + expect(ConditionEvaluator::evaluate('!review.missing', $data))->toBeTrue(); + }); + + test('empty condition throws', function () { + expect(fn () => ConditionEvaluator::evaluate(' ', [])) + ->toThrow(InvalidWorkflowDefinitionException::class); + }); + + test('malformed condition throws', function () { + expect(fn () => ConditionEvaluator::evaluate('1 + 2', [])) + ->toThrow(InvalidWorkflowDefinitionException::class); + }); + + test('unquoted identifiers on RHS fall back to string comparison for backwards compatibility', function () { + $data = ['status' => 'pending']; + + // RHS without quotes is treated as a string literal. + expect(ConditionEvaluator::evaluate('status === pending', $data))->toBeTrue(); + }); +}); diff --git a/tests/Unit/ExecutorRetryTest.php b/tests/Unit/ExecutorRetryTest.php index b37daf0..770ed97 100644 --- a/tests/Unit/ExecutorRetryTest.php +++ b/tests/Unit/ExecutorRetryTest.php @@ -25,7 +25,7 @@ protected function doExecute(WorkflowContext $context): ActionResult $failCount = $this->getConfig('fail_count', 0); if (self::$callCount <= $failCount) { - throw new \RuntimeException('Intentional failure #'.self::$callCount); + throw new RuntimeException('Intentional failure #'.self::$callCount); } return ActionResult::success(['attempts' => self::$callCount]); @@ -47,7 +47,7 @@ class AlwaysFailAction extends BaseAction { protected function doExecute(WorkflowContext $context): ActionResult { - throw new \RuntimeException('Always fails'); + throw new RuntimeException('Always fails'); } public function getName(): string diff --git a/tests/Unit/WorkflowEngineTest.php b/tests/Unit/WorkflowEngineTest.php index 8a4642d..3719a79 100644 --- a/tests/Unit/WorkflowEngineTest.php +++ b/tests/Unit/WorkflowEngineTest.php @@ -1,5 +1,6 @@ parse($definition); $workflowId = 'test-workflow'; $instance = new WorkflowInstance( @@ -117,7 +118,7 @@ ]; // Create a workflow in RUNNING state (so it can be cancelled) - $parser = new \SolutionForest\WorkflowEngine\Core\DefinitionParser; + $parser = new DefinitionParser; $workflowDef = $parser->parse($definition); $workflowId = 'test-workflow'; $instance = new WorkflowInstance( @@ -208,7 +209,7 @@ $completedId = $this->engine->start('completed-workflow', $definition); // Create a workflow in RUNNING state, then cancel it - $parser = new \SolutionForest\WorkflowEngine\Core\DefinitionParser; + $parser = new DefinitionParser; $workflowDef = $parser->parse($definition); $cancelledId = 'cancelled-workflow'; $instance = new WorkflowInstance( From 3ac8e5b16a1b17234fb369d9a778374f9a75e153 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 15 Apr 2026 04:19:32 +0000 Subject: [PATCH 2/3] Harden Executor, WorkflowInstance restoration, builder IDs, and HttpAction MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 2 of the codebase review fixes builds on the previous commit by replacing the remaining footguns flagged in the audit. - Executor: replace the recursive processWorkflow/executeStep call chain with a bounded iterative loop so long workflows (200+ steps) no longer consume stack frames. Adds a safety iteration cap driven by the total step count to catch cyclic transition graphs. - Executor: cap the retry backoff at 2s (MAX_BACKOFF_MICROSECONDS) and extract calculateBackoff() so a misconfigured step cannot block a worker for minutes on exponential sleep. - Executor: harden the pcntl timeout wrapper — enable async signals via pcntl_async_signals, restore the previous handler and async-mode on exit, and clarify in the warning log that timeout enforcement is CLI/queue-only. - WorkflowInstance::fromArray now validates the restored payload: missing/invalid id, unknown state values, non-list completed_steps, completed/current step references to steps that don't exist in the definition — all raise InvalidWorkflowStateException instead of silently producing a broken instance. - WorkflowBuilder: auto-generated step IDs (then/startWith/email/delay/ http/condition) now come from a monotonic counter that skips past any IDs already claimed by explicit addStep() calls, so mixing "step_1" with then() can no longer throw duplicateStepId. - BaseAction and HttpAction: catch Throwable rather than Exception so Errors from action code flow through the normal failure path. - HttpAction: add CURLOPT_CONNECTTIMEOUT, explicit CURLOPT_SSL_VERIFYPEER/ VERIFYHOST, a bounded CURLOPT_MAXREDIRS, and an http/https protocol allowlist (using CURLOPT_PROTOCOLS_STR when available) so redirects cannot hand the request off to file://, gopher://, etc. - Uuid: swap mt_rand() for random_bytes(), matching the docblock's "cryptographically random" claim. - SimpleWorkflow: drop uniqid() in favor of Uuid::v4() for unique workflow instance IDs. - ConditionAction: remove unused Attributes\Condition import. Adds 9 new tests covering Executor iteration (200-step workflow), fromArray validation, and builder auto-ID collisions. Test count goes from 107 to 116 (277 assertions). --- src/Actions/BaseAction.php | 8 +- src/Actions/ConditionAction.php | 1 - src/Actions/HttpAction.php | 28 +++- src/Core/Executor.php | 147 ++++++++++++++------- src/Core/WorkflowBuilder.php | 40 ++++-- src/Core/WorkflowInstance.php | 86 +++++++++++- src/Support/SimpleWorkflow.php | 4 +- src/Support/Uuid.php | 30 ++--- tests/Unit/ExecutorIterationTest.php | 67 ++++++++++ tests/Unit/WorkflowBuilderAutoIdTest.php | 37 ++++++ tests/Unit/WorkflowInstanceRestoreTest.php | 96 ++++++++++++++ 11 files changed, 452 insertions(+), 92 deletions(-) create mode 100644 tests/Unit/ExecutorIterationTest.php create mode 100644 tests/Unit/WorkflowBuilderAutoIdTest.php create mode 100644 tests/Unit/WorkflowInstanceRestoreTest.php diff --git a/src/Actions/BaseAction.php b/src/Actions/BaseAction.php index ce32197..3057c9e 100644 --- a/src/Actions/BaseAction.php +++ b/src/Actions/BaseAction.php @@ -186,10 +186,10 @@ public function execute(WorkflowContext $context): ActionResult ]); throw $e; - } catch (\Exception $e) { - // Log and return failure result for general exceptions - // The Executor will convert this to a StepExecutionException with Step context - $this->logger->error('Action failed with unexpected exception', [ + } catch (\Throwable $e) { + // Log and return failure result for general throwables (exceptions and errors). + // The Executor will convert this to a StepExecutionException with Step context. + $this->logger->error('Action failed with unexpected throwable', [ 'action' => static::class, 'workflow_id' => $context->getWorkflowId(), 'step_id' => $context->getStepId(), diff --git a/src/Actions/ConditionAction.php b/src/Actions/ConditionAction.php index 3ec8d2d..23f3062 100644 --- a/src/Actions/ConditionAction.php +++ b/src/Actions/ConditionAction.php @@ -2,7 +2,6 @@ namespace SolutionForest\WorkflowEngine\Actions; -use SolutionForest\WorkflowEngine\Attributes\Condition; use SolutionForest\WorkflowEngine\Attributes\WorkflowStep; use SolutionForest\WorkflowEngine\Core\ActionResult; use SolutionForest\WorkflowEngine\Core\WorkflowContext; diff --git a/src/Actions/HttpAction.php b/src/Actions/HttpAction.php index 1cfd889..3471ea8 100644 --- a/src/Actions/HttpAction.php +++ b/src/Actions/HttpAction.php @@ -37,7 +37,10 @@ protected function doExecute(WorkflowContext $context): ActionResult $method = strtoupper($this->getConfig('method', 'GET')); $data = $this->getConfig('data', []); $headers = $this->getConfig('headers', []); - $timeout = $this->getConfig('timeout', 30); + $timeout = (int) $this->getConfig('timeout', 30); + $connectTimeout = (int) $this->getConfig('connect_timeout', min(10, $timeout)); + $verifyTls = (bool) $this->getConfig('verify_tls', true); + $maxRedirects = (int) $this->getConfig('max_redirects', 3); if (! $url) { return ActionResult::failure('URL is required for HTTP action'); @@ -59,7 +62,22 @@ protected function doExecute(WorkflowContext $context): ActionResult curl_setopt($ch, CURLOPT_URL, $url); curl_setopt($ch, CURLOPT_RETURNTRANSFER, true); curl_setopt($ch, CURLOPT_TIMEOUT, $timeout); - curl_setopt($ch, CURLOPT_FOLLOWLOCATION, true); + curl_setopt($ch, CURLOPT_CONNECTTIMEOUT, $connectTimeout); + curl_setopt($ch, CURLOPT_FOLLOWLOCATION, $maxRedirects > 0); + curl_setopt($ch, CURLOPT_MAXREDIRS, $maxRedirects); + curl_setopt($ch, CURLOPT_SSL_VERIFYPEER, $verifyTls); + curl_setopt($ch, CURLOPT_SSL_VERIFYHOST, $verifyTls ? 2 : 0); + // Restrict redirects to HTTP/HTTPS so a Location header cannot + // hand the request off to file://, gopher://, etc. The constants + // were renamed in cURL 7.85 — prefer the new one when present. + if (defined('CURLOPT_PROTOCOLS_STR')) { + curl_setopt($ch, CURLOPT_PROTOCOLS_STR, 'http,https'); + curl_setopt($ch, CURLOPT_REDIR_PROTOCOLS_STR, 'http,https'); + } elseif (defined('CURLPROTO_HTTP') && defined('CURLPROTO_HTTPS')) { + $allowed = CURLPROTO_HTTP | CURLPROTO_HTTPS; + curl_setopt($ch, CURLOPT_PROTOCOLS, $allowed); + curl_setopt($ch, CURLOPT_REDIR_PROTOCOLS, $allowed); + } // Set HTTP method and body if ($method !== 'GET') { @@ -99,9 +117,9 @@ protected function doExecute(WorkflowContext $context): ActionResult $error = curl_error($ch); curl_close($ch); - if ($error) { + if ($responseBody === false || $error) { return ActionResult::failure( - "HTTP request failed: {$error}", + 'HTTP request failed: '.($error ?: 'unknown cURL error'), [ 'error' => $error, 'url' => $url, @@ -137,7 +155,7 @@ protected function doExecute(WorkflowContext $context): ActionResult ] ); - } catch (\Exception $e) { + } catch (\Throwable $e) { return ActionResult::failure( "HTTP request exception: {$e->getMessage()}", [ diff --git a/src/Core/Executor.php b/src/Core/Executor.php index 38677b6..743b45a 100644 --- a/src/Core/Executor.php +++ b/src/Core/Executor.php @@ -139,10 +139,12 @@ public function execute(WorkflowInstance $instance): void } /** - * Process workflow execution by managing state transitions and step execution. + * Process workflow execution by iterating over runnable steps until none remain. * - * This private method handles the core workflow processing logic, including - * state management, step scheduling, and completion detection. + * Each iteration asks the instance for its next runnable steps, executes them, + * and loops again. The loop is bounded by the total number of steps in the + * definition (x2 to allow for conditional skips) to defend against + * pathological definitions that would otherwise loop forever. * * @param WorkflowInstance $instance The workflow instance to process * @@ -157,36 +159,60 @@ private function processWorkflow(WorkflowInstance $instance): void $this->stateManager->save($instance); } - // Get next steps to execute - $nextSteps = $instance->getNextSteps(); + $totalSteps = count($instance->getDefinition()->getSteps()); + // Upper bound on iterations: each step can be visited at most once as a + // runnable step and once as a skip. The +1 guards the degenerate zero-step + // workflow from tripping the safety check immediately. + $maxIterations = max(1, $totalSteps * 2 + 1); + $iterations = 0; + + while (true) { + if (++$iterations > $maxIterations) { + throw new \RuntimeException( + "Workflow '{$instance->getId()}' exceeded maximum execution iterations ({$maxIterations}); ". + 'this usually indicates a cycle in the transition graph.' + ); + } - if (empty($nextSteps)) { - // Workflow completed successfully - $instance->setState(WorkflowState::COMPLETED); - $this->stateManager->save($instance); - $this->eventDispatcher->dispatch(new WorkflowCompletedEvent($instance)); + $nextSteps = $instance->getNextSteps(); - $this->logger->info('Workflow completed successfully', [ - 'workflow_id' => $instance->getId(), - 'workflow_name' => $instance->getDefinition()->getName(), - 'completed_steps' => count($instance->getCompletedSteps()), - 'execution_time' => abs($instance->getUpdatedAt()->getTimestamp() - $instance->getCreatedAt()->getTimestamp()).'s', - ]); + if (empty($nextSteps)) { + // Workflow completed successfully + $instance->setState(WorkflowState::COMPLETED); + $this->stateManager->save($instance); + $this->eventDispatcher->dispatch(new WorkflowCompletedEvent($instance)); - return; - } + $this->logger->info('Workflow completed successfully', [ + 'workflow_id' => $instance->getId(), + 'workflow_name' => $instance->getDefinition()->getName(), + 'completed_steps' => count($instance->getCompletedSteps()), + 'execution_time' => abs($instance->getUpdatedAt()->getTimestamp() - $instance->getCreatedAt()->getTimestamp()).'s', + ]); - // Execute each next step - foreach ($nextSteps as $step) { - if ($instance->isStepCompleted($step->getId())) { - continue; // Skip already completed steps + return; } - if (! $instance->canExecuteStep($step->getId())) { - continue; // Skip steps that can't be executed yet + $progressed = false; + + foreach ($nextSteps as $step) { + if ($instance->isStepCompleted($step->getId())) { + continue; // Skip already completed steps + } + + if (! $instance->canExecuteStep($step->getId())) { + continue; // Skip steps that can't be executed yet + } + + $this->executeStep($instance, $step); + $progressed = true; } - $this->executeStep($instance, $step); + // If no steps made progress this iteration, the workflow is stuck + // (e.g. all next steps were blocked on unmet prerequisites). Exit + // the loop and let the next resume() reattempt. + if (! $progressed) { + return; + } } } @@ -217,7 +243,6 @@ private function executeStep(WorkflowInstance $instance, Step $step): void $instance->setCurrentStepId($step->getId()); $this->stateManager->markStepCompleted($instance, $step->getId()); - $this->processWorkflow($instance); return; } @@ -246,10 +271,6 @@ private function executeStep(WorkflowInstance $instance, Step $step): void 'workflow_id' => $instance->getId(), 'step_id' => $step->getId(), ]); - - // Continue execution recursively - $this->processWorkflow($instance); - } catch (\Throwable $e) { $context = new WorkflowContext( workflowId: $instance->getId(), @@ -292,6 +313,13 @@ private function executeStep(WorkflowInstance $instance, Step $step): void * @throws ActionNotFoundException If the action class doesn't exist * @throws StepExecutionException If all retry attempts are exhausted */ + /** + * Maximum backoff sleep between retries, in microseconds. Caps the + * exponential growth so a misconfigured step cannot block a worker for + * minutes. + */ + private const MAX_BACKOFF_MICROSECONDS = 2_000_000; // 2 seconds + private function executeActionWithRetry(WorkflowInstance $instance, Step $step): void { $maxAttempts = $step->getRetryAttempts() + 1; // +1 for initial attempt @@ -303,16 +331,12 @@ private function executeActionWithRetry(WorkflowInstance $instance, Step $step): return; } - $lastException = null; - for ($attempt = 1; $attempt <= $maxAttempts; $attempt++) { try { $this->executeAction($instance, $step); return; // Success — exit retry loop } catch (\Throwable $e) { - $lastException = $e; - if ($attempt === $maxAttempts) { $this->logger->error('Step failed after all retry attempts', [ 'workflow_id' => $instance->getId(), @@ -325,11 +349,14 @@ private function executeActionWithRetry(WorkflowInstance $instance, Step $step): throw $e; // Final attempt failed — propagate } + $backoffMicroseconds = $this->calculateBackoff($attempt); + $this->logger->warning('Step failed, retrying', [ 'workflow_id' => $instance->getId(), 'step_id' => $step->getId(), 'attempt' => $attempt, 'max_attempts' => $maxAttempts, + 'backoff_ms' => (int) ($backoffMicroseconds / 1000), 'error' => $e->getMessage(), ]); @@ -341,51 +368,73 @@ private function executeActionWithRetry(WorkflowInstance $instance, Step $step): $e )); - // Exponential backoff: 100ms, 200ms, 400ms... (keep short for a library) - $backoffMicroseconds = (int) (100000 * pow(2, $attempt - 1)); usleep($backoffMicroseconds); } } } + /** + * Calculate exponential backoff delay between retry attempts. + * + * Doubles each attempt starting at 100ms, capped at MAX_BACKOFF_MICROSECONDS + * to prevent runaway worker blocking. + * + * @param int $attempt 1-based attempt number + * @return int Delay in microseconds + */ + private function calculateBackoff(int $attempt): int + { + $base = 100_000; // 100ms + $delay = (int) ($base * (2 ** ($attempt - 1))); + + return min($delay, self::MAX_BACKOFF_MICROSECONDS); + } + /** * Execute a callback with a timeout constraint. * - * Uses pcntl_alarm when available, otherwise logs a warning and executes without timeout. + * Uses pcntl_alarm when the pcntl extension is loaded. pcntl is generally + * only available under the CLI SAPI, so for web/FPM contexts this method + * logs a warning and runs the callback unbounded — long-running workflow + * steps should be dispatched via a queue worker instead. * * @param callable $callback The callback to execute * @param int $timeoutSeconds Maximum execution time in seconds * @return mixed The callback's return value * - * @throws StepExecutionException If the timeout is exceeded + * @throws \RuntimeException If the timeout is exceeded while running under pcntl */ private function executeWithTimeout(callable $callback, int $timeoutSeconds): mixed { - if (! function_exists('pcntl_alarm') || ! function_exists('pcntl_signal')) { + if (! function_exists('pcntl_alarm') || ! function_exists('pcntl_signal') || ! function_exists('pcntl_async_signals')) { $this->logger->warning('pcntl extension not available, timeout not enforced', [ 'timeout_seconds' => $timeoutSeconds, + 'hint' => 'Execute workflows via CLI or queue workers to enforce step timeouts.', ]); return $callback(); } - pcntl_signal(SIGALRM, function () use ($timeoutSeconds) { + // Ensure the signal handler runs at the VM tick rather than waiting for + // an explicit pcntl_signal_dispatch() call. Without this, SIGALRM can + // be delivered unpredictably or not at all. + $previousAsync = pcntl_async_signals(true); + $previousHandler = pcntl_signal_get_handler(SIGALRM); + + pcntl_signal(SIGALRM, function () use ($timeoutSeconds): never { throw new \RuntimeException("Step execution timed out after {$timeoutSeconds} seconds"); }); pcntl_alarm($timeoutSeconds); try { - $result = $callback(); - pcntl_alarm(0); - - return $result; - } catch (\Throwable $e) { - pcntl_alarm(0); - - throw $e; + return $callback(); } finally { - pcntl_signal(SIGALRM, SIG_DFL); + // Always clear the alarm and restore the previous signal handler, + // even if the callback threw or the alarm fired. + pcntl_alarm(0); + pcntl_signal(SIGALRM, $previousHandler ?: SIG_DFL); + pcntl_async_signals($previousAsync); } } diff --git a/src/Core/WorkflowBuilder.php b/src/Core/WorkflowBuilder.php index 0d0377e..00c4eb6 100644 --- a/src/Core/WorkflowBuilder.php +++ b/src/Core/WorkflowBuilder.php @@ -66,6 +66,14 @@ final class WorkflowBuilder /** @var array Additional workflow metadata */ private array $metadata = []; + /** + * Monotonic counter used when auto-generating step IDs for sugar methods + * like then(), startWith(), email(), delay(), http(), and condition(). + * Using a dedicated counter (instead of count($this->steps)) means explicit + * step IDs mixed with auto-generated ones can never collide. + */ + private int $autoStepCounter = 0; + /** * Private constructor to enforce factory pattern usage. * @@ -237,9 +245,7 @@ public function startWith( string|int|null $timeout = null, int $retryAttempts = 0 ): self { - $stepId = 'step_'.(count($this->steps) + 1); - - return $this->addStep($stepId, $action, $config, $timeout, $retryAttempts); + return $this->addStep($this->generateStepId('step'), $action, $config, $timeout, $retryAttempts); } /** @@ -262,9 +268,7 @@ public function then( string|int|null $timeout = null, int $retryAttempts = 0 ): self { - $stepId = 'step_'.(count($this->steps) + 1); - - return $this->addStep($stepId, $action, $config, $timeout, $retryAttempts); + return $this->addStep($this->generateStepId('step'), $action, $config, $timeout, $retryAttempts); } /** @@ -331,7 +335,7 @@ public function email( array $data = [] ): self { return $this->addStep( - 'email_'.count($this->steps), + $this->generateStepId('email'), 'SolutionForest\\WorkflowEngine\\Actions\\EmailAction', [ 'template' => $template, @@ -370,7 +374,7 @@ public function delay(?int $seconds = null, ?int $minutes = null, ?int $hours = } return $this->addStep( - 'delay_'.count($this->steps), + $this->generateStepId('delay'), 'SolutionForest\\WorkflowEngine\\Actions\\DelayAction', ['seconds' => $totalSeconds] ); @@ -402,7 +406,7 @@ public function http( array $headers = [] ): self { return $this->addStep( - 'http_'.count($this->steps), + $this->generateStepId('http'), 'SolutionForest\\WorkflowEngine\\Actions\\HttpAction', [ 'url' => $url, @@ -427,12 +431,28 @@ public function http( public function condition(string $condition): self { return $this->addStep( - 'condition_'.count($this->steps), + $this->generateStepId('condition'), 'SolutionForest\\WorkflowEngine\\Actions\\ConditionAction', ['condition' => $condition] ); } + /** + * Generate a unique auto step ID using the internal counter. Skips over + * any IDs that have already been claimed by explicit addStep() calls so + * the sugar methods cannot collide with user-defined IDs. + */ + private function generateStepId(string $prefix): string + { + $existing = array_column($this->steps, 'id'); + + do { + $candidate = $prefix.'_'.(++$this->autoStepCounter); + } while (in_array($candidate, $existing, true)); + + return $candidate; + } + /** * Add custom metadata to the workflow definition. * diff --git a/src/Core/WorkflowInstance.php b/src/Core/WorkflowInstance.php index 31ab20c..811e003 100644 --- a/src/Core/WorkflowInstance.php +++ b/src/Core/WorkflowInstance.php @@ -402,24 +402,102 @@ public function toArray(): array /** * Create a workflow instance from an array representation. * + * Validates that the stored payload is well-formed: every `completed_steps` + * and `failed_steps` entry must reference a step that exists in the + * definition, and the restored state must be a valid WorkflowState value. + * Corrupt or foreign payloads throw InvalidWorkflowStateException so that + * callers don't silently resume broken instances. + * * @param array $data Array data to restore from * @param WorkflowDefinition $definition The workflow definition * @return static Restored workflow instance + * + * @throws InvalidWorkflowStateException If the payload is structurally invalid */ public static function fromArray(array $data, WorkflowDefinition $definition): static { + if (! isset($data['id']) || ! is_string($data['id']) || $data['id'] === '') { + throw new InvalidWorkflowStateException( + 'Cannot restore workflow instance: missing or invalid "id" field.', + WorkflowState::PENDING, + WorkflowState::PENDING, + (string) ($data['id'] ?? '') + ); + } + + if (! isset($data['state']) || ! is_string($data['state'])) { + throw new InvalidWorkflowStateException( + "Cannot restore workflow instance '{$data['id']}': missing or invalid \"state\" field.", + WorkflowState::PENDING, + WorkflowState::PENDING, + $data['id'] + ); + } + + $state = WorkflowState::tryFrom($data['state']); + if ($state === null) { + throw new InvalidWorkflowStateException( + "Cannot restore workflow instance '{$data['id']}': unknown state '{$data['state']}'.", + WorkflowState::PENDING, + WorkflowState::PENDING, + $data['id'] + ); + } + + $completedSteps = $data['completed_steps'] ?? []; + $failedSteps = $data['failed_steps'] ?? []; + + if (! is_array($completedSteps) || ! array_is_list($completedSteps)) { + throw new InvalidWorkflowStateException( + "Cannot restore workflow instance '{$data['id']}': \"completed_steps\" must be a list.", + $state, + $state, + $data['id'] + ); + } + + foreach ($completedSteps as $stepId) { + if (! is_string($stepId) || ! $definition->hasStep($stepId)) { + throw new InvalidWorkflowStateException( + "Cannot restore workflow instance '{$data['id']}': completed step '".(is_string($stepId) ? $stepId : gettype($stepId))."' is not defined in the workflow.", + $state, + $state, + $data['id'] + ); + } + } + + if (! is_array($failedSteps)) { + throw new InvalidWorkflowStateException( + "Cannot restore workflow instance '{$data['id']}': \"failed_steps\" must be an array.", + $state, + $state, + $data['id'] + ); + } + + $currentStepId = $data['current_step_id'] ?? null; + if ($currentStepId !== null && (! is_string($currentStepId) || ! $definition->hasStep($currentStepId))) { + throw new InvalidWorkflowStateException( + "Cannot restore workflow instance '{$data['id']}': current step '".(is_string($currentStepId) ? $currentStepId : gettype($currentStepId))."' is not defined in the workflow.", + $state, + $state, + $data['id'] + ); + } + $instance = new self( id: $data['id'], definition: $definition, - state: WorkflowState::from($data['state']), + state: $state, data: $data['data'] ?? [], createdAt: new \DateTime($data['created_at']), updatedAt: new \DateTime($data['updated_at']) ); - $instance->currentStepId = $data['current_step_id'] ?? null; - $instance->completedSteps = $data['completed_steps'] ?? []; - $instance->failedSteps = $data['failed_steps'] ?? []; + $instance->currentStepId = $currentStepId; + $instance->completedSteps = $completedSteps; + $instance->failedSteps = $failedSteps; $instance->errorMessage = $data['error_message'] ?? null; return $instance; diff --git a/src/Support/SimpleWorkflow.php b/src/Support/SimpleWorkflow.php index c4c27d4..4c56303 100644 --- a/src/Support/SimpleWorkflow.php +++ b/src/Support/SimpleWorkflow.php @@ -84,7 +84,7 @@ public function sequential(string $name, array $actions, array $context = []): s $workflow = $builder->build(); - return $this->engine->start($name.'_'.uniqid(), $workflow->toArray(), $context); + return $this->engine->start($name.'_'.Uuid::v4(), $workflow->toArray(), $context); } /** @@ -163,7 +163,7 @@ public function executeBuilder(WorkflowBuilder $builder, array $context = []): s $workflow = $builder->build(); return $this->engine->start( - $workflow->getName().'_'.uniqid(), + $workflow->getName().'_'.Uuid::v4(), $workflow->toArray(), $context ); diff --git a/src/Support/Uuid.php b/src/Support/Uuid.php index d4105ff..1cb93c1 100644 --- a/src/Support/Uuid.php +++ b/src/Support/Uuid.php @@ -12,7 +12,7 @@ * * ## Features * - **RFC 4122 Compliant**: Generates valid UUID v4 strings - * - **Cryptographically Random**: Uses mt_rand() for pseudo-random generation + * - **Cryptographically Random**: Uses random_bytes() for secure random generation * - **No Dependencies**: Pure PHP implementation without external libraries * - **Thread Safe**: Safe for concurrent usage in multi-threaded environments * @@ -81,25 +81,21 @@ class Uuid */ public static function v4(): string { - return sprintf( - '%04x%04x-%04x-%04x-%04x-%04x%04x%04x', - // 32 bits for "time_low" - mt_rand(0, 0xFFFF), mt_rand(0, 0xFFFF), - - // 16 bits for "time_mid" - mt_rand(0, 0xFFFF), + $bytes = random_bytes(16); - // 16 bits for "time_hi_and_version", - // four most significant bits holds version number 4 - mt_rand(0, 0x0FFF) | 0x4000, + // Set version (4) and variant (DCE 1.1 / RFC 4122) bits. + $bytes[6] = chr((ord($bytes[6]) & 0x0F) | 0x40); + $bytes[8] = chr((ord($bytes[8]) & 0x3F) | 0x80); - // 16 bits, 8 bits for "clk_seq_hi_res", - // 8 bits for "clk_seq_low", - // two most significant bits holds zero and one for variant DCE1.1 - mt_rand(0, 0x3FFF) | 0x8000, + $hex = bin2hex($bytes); - // 48 bits for "node" - mt_rand(0, 0xFFFF), mt_rand(0, 0xFFFF), mt_rand(0, 0xFFFF) + return sprintf( + '%s-%s-%s-%s-%s', + substr($hex, 0, 8), + substr($hex, 8, 4), + substr($hex, 12, 4), + substr($hex, 16, 4), + substr($hex, 20, 12) ); } } diff --git a/tests/Unit/ExecutorIterationTest.php b/tests/Unit/ExecutorIterationTest.php new file mode 100644 index 0000000..0bf9119 --- /dev/null +++ b/tests/Unit/ExecutorIterationTest.php @@ -0,0 +1,67 @@ + */ + public static array $log = []; + + public static function reset(): void + { + self::$log = []; + } + + protected function doExecute(WorkflowContext $context): ActionResult + { + self::$log[] = $context->getStepId(); + + return ActionResult::success(); + } + + public function getName(): string + { + return 'Sequence Action'; + } + + public function getDescription(): string + { + return 'Test action that logs the order of execution.'; + } +} + +describe('Executor iteration', function () { + beforeEach(function () { + SequenceAction::reset(); + }); + + test('iteratively executes a long chain of steps without stack overflow', function () { + // 200 steps would blow the default xdebug stack if the executor were + // still recursive. With iteration, it should run cleanly. + $builder = WorkflowBuilder::create('long-chain'); + for ($i = 0; $i < 200; $i++) { + $builder->addStep("s{$i}", SequenceAction::class); + } + + $definition = $builder->build(); + $engine = new WorkflowEngine(new InMemoryStorage); + + $id = $engine->start('long-chain-1', $definition->toArray(), []); + $instance = $engine->getInstance($id); + + expect($instance->getState())->toBe(WorkflowState::COMPLETED); + expect(count(SequenceAction::$log))->toBe(200); + expect(SequenceAction::$log[0])->toBe('s0'); + expect(SequenceAction::$log[199])->toBe('s199'); + }); +}); diff --git a/tests/Unit/WorkflowBuilderAutoIdTest.php b/tests/Unit/WorkflowBuilderAutoIdTest.php new file mode 100644 index 0000000..21f5281 --- /dev/null +++ b/tests/Unit/WorkflowBuilderAutoIdTest.php @@ -0,0 +1,37 @@ +addStep('step_1', LogAction::class, ['message' => 'first']) + ->then(LogAction::class, ['message' => 'second']) + ->then(LogAction::class, ['message' => 'third']) + ->build(); + + $ids = array_keys($definition->getSteps()); + + expect($ids)->toHaveCount(3); + expect($ids)->toContain('step_1'); + // The auto-generated IDs must not reuse "step_1"; the helper advances + // the counter past any colliding explicit IDs. + expect(array_filter($ids, fn ($id) => $id === 'step_1'))->toHaveCount(1); + }); + + test('email/delay/http/condition sugar methods use monotonic counters', function () { + $definition = WorkflowBuilder::create('sugar-ids') + ->email('welcome', 'user@example.com', 'Hi') + ->email('followup', 'user@example.com', 'Hi again') + ->delay(seconds: 30) + ->delay(seconds: 60) + ->build(); + + $ids = array_keys($definition->getSteps()); + + expect($ids)->toHaveCount(4); + expect(count(array_unique($ids)))->toBe(4); + }); +}); diff --git a/tests/Unit/WorkflowInstanceRestoreTest.php b/tests/Unit/WorkflowInstanceRestoreTest.php new file mode 100644 index 0000000..6ba9f88 --- /dev/null +++ b/tests/Unit/WorkflowInstanceRestoreTest.php @@ -0,0 +1,96 @@ +definition = $parser->parse([ + 'name' => 'restore-test', + 'steps' => [ + ['id' => 'a', 'action' => 'log'], + ['id' => 'b', 'action' => 'log'], + ], + 'transitions' => [ + ['from' => 'a', 'to' => 'b'], + ], + ]); +}); + +describe('WorkflowInstance::fromArray validation', function () { + test('restores a valid payload round-trip', function () { + $restored = WorkflowInstance::fromArray([ + 'id' => 'inst-1', + 'state' => 'running', + 'data' => ['foo' => 'bar'], + 'current_step_id' => 'b', + 'completed_steps' => ['a'], + 'failed_steps' => [], + 'error_message' => null, + 'created_at' => '2026-01-01T00:00:00+00:00', + 'updated_at' => '2026-01-02T00:00:00+00:00', + ], $this->definition); + + expect($restored->getId())->toBe('inst-1'); + expect($restored->getState())->toBe(WorkflowState::RUNNING); + expect($restored->getCurrentStepId())->toBe('b'); + expect($restored->getCompletedSteps())->toBe(['a']); + }); + + test('rejects payloads with unknown state values', function () { + expect(fn () => WorkflowInstance::fromArray([ + 'id' => 'inst-bad-state', + 'state' => 'not-a-real-state', + 'completed_steps' => [], + 'failed_steps' => [], + 'created_at' => '2026-01-01T00:00:00+00:00', + 'updated_at' => '2026-01-01T00:00:00+00:00', + ], $this->definition))->toThrow(InvalidWorkflowStateException::class); + }); + + test('rejects payloads that reference unknown completed steps', function () { + expect(fn () => WorkflowInstance::fromArray([ + 'id' => 'inst-bad-step', + 'state' => 'running', + 'completed_steps' => ['a', 'ghost_step'], + 'failed_steps' => [], + 'created_at' => '2026-01-01T00:00:00+00:00', + 'updated_at' => '2026-01-01T00:00:00+00:00', + ], $this->definition))->toThrow(InvalidWorkflowStateException::class); + }); + + test('rejects payloads with a current step that is not defined', function () { + expect(fn () => WorkflowInstance::fromArray([ + 'id' => 'inst-bad-current', + 'state' => 'running', + 'current_step_id' => 'nonexistent', + 'completed_steps' => [], + 'failed_steps' => [], + 'created_at' => '2026-01-01T00:00:00+00:00', + 'updated_at' => '2026-01-01T00:00:00+00:00', + ], $this->definition))->toThrow(InvalidWorkflowStateException::class); + }); + + test('rejects payloads missing required id field', function () { + expect(fn () => WorkflowInstance::fromArray([ + 'state' => 'running', + 'completed_steps' => [], + 'failed_steps' => [], + 'created_at' => '2026-01-01T00:00:00+00:00', + 'updated_at' => '2026-01-01T00:00:00+00:00', + ], $this->definition))->toThrow(InvalidWorkflowStateException::class); + }); + + test('rejects payloads whose completed_steps is not a list', function () { + expect(fn () => WorkflowInstance::fromArray([ + 'id' => 'inst-non-list', + 'state' => 'running', + 'completed_steps' => ['a' => true], + 'failed_steps' => [], + 'created_at' => '2026-01-01T00:00:00+00:00', + 'updated_at' => '2026-01-01T00:00:00+00:00', + ], $this->definition))->toThrow(InvalidWorkflowStateException::class); + }); +}); From f2341119197c393fa0c611b340c7312d0c16b6cb Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 15 Apr 2026 04:22:26 +0000 Subject: [PATCH 3/3] Remove dead factory methods and fix InMemoryStorage key preservation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit WorkflowException::fromContext() and fromInstance() call `new static(...)` with the base constructor signature, but every concrete subclass (StepExecutionException, ActionNotFoundException, InvalidWorkflowState Exception, ...) has a different constructor. These factories were PHPStan-ignored and are never called in the codebase — they would throw ArgumentCountError if anyone tried to use them. Remove them. InMemoryStorage::findInstances with filter criteria returned the raw array_filter result, which preserves the original string keys. Wrap it with array_values so callers receive a sequential list matching the no-criteria path. --- src/Exceptions/WorkflowException.php | 50 ---------------------------- tests/Support/InMemoryStorage.php | 4 ++- 2 files changed, 3 insertions(+), 51 deletions(-) diff --git a/src/Exceptions/WorkflowException.php b/src/Exceptions/WorkflowException.php index 59fafc7..8201099 100644 --- a/src/Exceptions/WorkflowException.php +++ b/src/Exceptions/WorkflowException.php @@ -3,8 +3,6 @@ namespace SolutionForest\WorkflowEngine\Exceptions; use Exception; -use SolutionForest\WorkflowEngine\Core\WorkflowContext; -use SolutionForest\WorkflowEngine\Core\WorkflowInstance; use Throwable; /** @@ -102,52 +100,4 @@ public function getSuggestions(): array * @return string User-friendly error description */ abstract public function getUserMessage(): string; - - /** - * Create an exception from a workflow context. - * - * @param string $message The error message - * @param WorkflowContext $context The workflow context - * @param Throwable|null $previous Previous exception - * @return static The created exception instance - */ - public static function fromContext( - string $message, - WorkflowContext $context, - ?Throwable $previous = null - ): static { - // @phpstan-ignore-next-line new.static - return new static($message, [ - 'workflow_id' => $context->getWorkflowId(), - 'step_id' => $context->getStepId(), - 'context_data' => $context->getData(), - 'config' => $context->getConfig(), - 'executed_at' => $context->executedAt->format('Y-m-d H:i:s'), - ], 0, $previous); - } - - /** - * Create an exception from a workflow instance. - * - * @param string $message The error message - * @param WorkflowInstance $instance The workflow instance - * @param Throwable|null $previous Previous exception - * @return static The created exception instance - */ - public static function fromInstance( - string $message, - WorkflowInstance $instance, - ?Throwable $previous = null - ): static { - // @phpstan-ignore-next-line new.static - return new static($message, [ - 'instance_id' => $instance->getId(), - 'workflow_name' => $instance->getDefinition()->getName(), - 'current_state' => $instance->getState()->value, - 'current_step' => $instance->getCurrentStepId(), - 'instance_data' => $instance->getData(), - 'created_at' => $instance->getCreatedAt()->format('Y-m-d H:i:s'), - 'updated_at' => $instance->getUpdatedAt()->format('Y-m-d H:i:s'), - ], 0, $previous); - } } diff --git a/tests/Support/InMemoryStorage.php b/tests/Support/InMemoryStorage.php index b9fc690..df2fb68 100644 --- a/tests/Support/InMemoryStorage.php +++ b/tests/Support/InMemoryStorage.php @@ -29,7 +29,7 @@ public function findInstances(array $criteria = []): array return array_values($this->instances); } - return array_filter($this->instances, function ($instance) use ($criteria) { + $filtered = array_filter($this->instances, function ($instance) use ($criteria) { foreach ($criteria as $key => $value) { // Simple implementation for basic filtering if ($key === 'state' && $instance->getState()->value !== $value) { @@ -39,6 +39,8 @@ public function findInstances(array $criteria = []): array return true; }); + + return array_values($filtered); } public function delete(string $id): void