Skip to content

Commit 841ebf4

Browse files
lam0819lam0819
authored
Merge pull request #36 from solutionforest/claude/review-codebase-fv2VL
Refactor executor to iterative loop, add condition evaluation, improve validation
2 parents bef55cc + f234111 commit 841ebf4

23 files changed

Lines changed: 809 additions & 194 deletions

‎src/Actions/BaseAction.php‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -186,10 +186,10 @@ public function execute(WorkflowContext $context): ActionResult
186186
]);
187187

188188
throw $e;
189-
} catch (\Exception $e) {
190-
// Log and return failure result for general exceptions
191-
// The Executor will convert this to a StepExecutionException with Step context
192-
$this->logger->error('Action failed with unexpected exception', [
189+
} catch (\Throwable $e) {
190+
// Log and return failure result for general throwables (exceptions and errors).
191+
// The Executor will convert this to a StepExecutionException with Step context.
192+
$this->logger->error('Action failed with unexpected throwable', [
193193
'action' => static::class,
194194
'workflow_id' => $context->getWorkflowId(),
195195
'step_id' => $context->getStepId(),

‎src/Actions/ConditionAction.php‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@
22

33
namespace SolutionForest\WorkflowEngine\Actions;
44

5-
use SolutionForest\WorkflowEngine\Attributes\Condition;
65
use SolutionForest\WorkflowEngine\Attributes\WorkflowStep;
76
use SolutionForest\WorkflowEngine\Core\ActionResult;
87
use SolutionForest\WorkflowEngine\Core\WorkflowContext;

‎src/Actions/HttpAction.php‎

Lines changed: 23 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,10 @@ protected function doExecute(WorkflowContext $context): ActionResult
3737
$method = strtoupper($this->getConfig('method', 'GET'));
3838
$data = $this->getConfig('data', []);
3939
$headers = $this->getConfig('headers', []);
40-
$timeout = $this->getConfig('timeout', 30);
40+
$timeout = (int) $this->getConfig('timeout', 30);
41+
$connectTimeout = (int) $this->getConfig('connect_timeout', min(10, $timeout));
42+
$verifyTls = (bool) $this->getConfig('verify_tls', true);
43+
$maxRedirects = (int) $this->getConfig('max_redirects', 3);
4144

4245
if (! $url) {
4346
return ActionResult::failure('URL is required for HTTP action');
@@ -59,7 +62,22 @@ protected function doExecute(WorkflowContext $context): ActionResult
5962
curl_setopt($ch, CURLOPT_URL, $url);
6063
curl_setopt($ch, CURLOPT_RETURNTRANSFER, true);
6164
curl_setopt($ch, CURLOPT_TIMEOUT, $timeout);
62-
curl_setopt($ch, CURLOPT_FOLLOWLOCATION, true);
65+
curl_setopt($ch, CURLOPT_CONNECTTIMEOUT, $connectTimeout);
66+
curl_setopt($ch, CURLOPT_FOLLOWLOCATION, $maxRedirects > 0);
67+
curl_setopt($ch, CURLOPT_MAXREDIRS, $maxRedirects);
68+
curl_setopt($ch, CURLOPT_SSL_VERIFYPEER, $verifyTls);
69+
curl_setopt($ch, CURLOPT_SSL_VERIFYHOST, $verifyTls ? 2 : 0);
70+
// Restrict redirects to HTTP/HTTPS so a Location header cannot
71+
// hand the request off to file://, gopher://, etc. The constants
72+
// were renamed in cURL 7.85 — prefer the new one when present.
73+
if (defined('CURLOPT_PROTOCOLS_STR')) {
74+
curl_setopt($ch, CURLOPT_PROTOCOLS_STR, 'http,https');
75+
curl_setopt($ch, CURLOPT_REDIR_PROTOCOLS_STR, 'http,https');
76+
} elseif (defined('CURLPROTO_HTTP') && defined('CURLPROTO_HTTPS')) {
77+
$allowed = CURLPROTO_HTTP | CURLPROTO_HTTPS;
78+
curl_setopt($ch, CURLOPT_PROTOCOLS, $allowed);
79+
curl_setopt($ch, CURLOPT_REDIR_PROTOCOLS, $allowed);
80+
}
6381

6482
// Set HTTP method and body
6583
if ($method !== 'GET') {
@@ -99,9 +117,9 @@ protected function doExecute(WorkflowContext $context): ActionResult
99117
$error = curl_error($ch);
100118
curl_close($ch);
101119

102-
if ($error) {
120+
if ($responseBody === false || $error) {
103121
return ActionResult::failure(
104-
"HTTP request failed: {$error}",
122+
'HTTP request failed: '.($error ?: 'unknown cURL error'),
105123
[
106124
'error' => $error,
107125
'url' => $url,
@@ -137,7 +155,7 @@ protected function doExecute(WorkflowContext $context): ActionResult
137155
]
138156
);
139157

140-
} catch (\Exception $e) {
158+
} catch (\Throwable $e) {
141159
return ActionResult::failure(
142160
"HTTP request exception: {$e->getMessage()}",
143161
[

‎src/Core/Executor.php‎

Lines changed: 123 additions & 60 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@
22

33
namespace SolutionForest\WorkflowEngine\Core;
44

5-
use Exception;
65
use SolutionForest\WorkflowEngine\Contracts\EventDispatcher;
76
use SolutionForest\WorkflowEngine\Contracts\Logger;
87
use SolutionForest\WorkflowEngine\Contracts\WorkflowAction;
@@ -121,7 +120,7 @@ public function execute(WorkflowInstance $instance): void
121120
{
122121
try {
123122
$this->processWorkflow($instance);
124-
} catch (Exception $e) {
123+
} catch (\Throwable $e) {
125124
$this->logger->error('Workflow execution failed', [
126125
'workflow_id' => $instance->getId(),
127126
'workflow_name' => $instance->getDefinition()->getName(),
@@ -134,16 +133,18 @@ public function execute(WorkflowInstance $instance): void
134133
$this->stateManager->setError($instance, $e->getMessage());
135134
$this->eventDispatcher->dispatch(new WorkflowFailedEvent($instance, $e));
136135

137-
// Re-throw the original exception to maintain the error context
136+
// Re-throw the original throwable to maintain the error context
138137
throw $e;
139138
}
140139
}
141140

142141
/**
143-
* Process workflow execution by managing state transitions and step execution.
142+
* Process workflow execution by iterating over runnable steps until none remain.
144143
*
145-
* This private method handles the core workflow processing logic, including
146-
* state management, step scheduling, and completion detection.
144+
* Each iteration asks the instance for its next runnable steps, executes them,
145+
* and loops again. The loop is bounded by the total number of steps in the
146+
* definition (x2 to allow for conditional skips) to defend against
147+
* pathological definitions that would otherwise loop forever.
147148
*
148149
* @param WorkflowInstance $instance The workflow instance to process
149150
*
@@ -158,36 +159,60 @@ private function processWorkflow(WorkflowInstance $instance): void
158159
$this->stateManager->save($instance);
159160
}
160161

161-
// Get next steps to execute
162-
$nextSteps = $instance->getNextSteps();
162+
$totalSteps = count($instance->getDefinition()->getSteps());
163+
// Upper bound on iterations: each step can be visited at most once as a
164+
// runnable step and once as a skip. The +1 guards the degenerate zero-step
165+
// workflow from tripping the safety check immediately.
166+
$maxIterations = max(1, $totalSteps * 2 + 1);
167+
$iterations = 0;
168+
169+
while (true) {
170+
if (++$iterations > $maxIterations) {
171+
throw new \RuntimeException(
172+
"Workflow '{$instance->getId()}' exceeded maximum execution iterations ({$maxIterations}); ".
173+
'this usually indicates a cycle in the transition graph.'
174+
);
175+
}
163176

164-
if (empty($nextSteps)) {
165-
// Workflow completed successfully
166-
$instance->setState(WorkflowState::COMPLETED);
167-
$this->stateManager->save($instance);
168-
$this->eventDispatcher->dispatch(new WorkflowCompletedEvent($instance));
177+
$nextSteps = $instance->getNextSteps();
169178

170-
$this->logger->info('Workflow completed successfully', [
171-
'workflow_id' => $instance->getId(),
172-
'workflow_name' => $instance->getDefinition()->getName(),
173-
'completed_steps' => count($instance->getCompletedSteps()),
174-
'execution_time' => abs($instance->getUpdatedAt()->getTimestamp() - $instance->getCreatedAt()->getTimestamp()).'s',
175-
]);
179+
if (empty($nextSteps)) {
180+
// Workflow completed successfully
181+
$instance->setState(WorkflowState::COMPLETED);
182+
$this->stateManager->save($instance);
183+
$this->eventDispatcher->dispatch(new WorkflowCompletedEvent($instance));
176184

177-
return;
178-
}
185+
$this->logger->info('Workflow completed successfully', [
186+
'workflow_id' => $instance->getId(),
187+
'workflow_name' => $instance->getDefinition()->getName(),
188+
'completed_steps' => count($instance->getCompletedSteps()),
189+
'execution_time' => abs($instance->getUpdatedAt()->getTimestamp() - $instance->getCreatedAt()->getTimestamp()).'s',
190+
]);
179191

180-
// Execute each next step
181-
foreach ($nextSteps as $step) {
182-
if ($instance->isStepCompleted($step->getId())) {
183-
continue; // Skip already completed steps
192+
return;
184193
}
185194

186-
if (! $instance->canExecuteStep($step->getId())) {
187-
continue; // Skip steps that can't be executed yet
195+
$progressed = false;
196+
197+
foreach ($nextSteps as $step) {
198+
if ($instance->isStepCompleted($step->getId())) {
199+
continue; // Skip already completed steps
200+
}
201+
202+
if (! $instance->canExecuteStep($step->getId())) {
203+
continue; // Skip steps that can't be executed yet
204+
}
205+
206+
$this->executeStep($instance, $step);
207+
$progressed = true;
188208
}
189209

190-
$this->executeStep($instance, $step);
210+
// If no steps made progress this iteration, the workflow is stuck
211+
// (e.g. all next steps were blocked on unmet prerequisites). Exit
212+
// the loop and let the next resume() reattempt.
213+
if (! $progressed) {
214+
return;
215+
}
191216
}
192217
}
193218

@@ -206,6 +231,22 @@ private function processWorkflow(WorkflowInstance $instance): void
206231
*/
207232
private function executeStep(WorkflowInstance $instance, Step $step): void
208233
{
234+
// Evaluate step conditions. Steps whose conditions don't match the current
235+
// workflow data are skipped (marked completed without running the action)
236+
// so that downstream transitions continue to flow.
237+
if (! $step->canExecute($instance->getData())) {
238+
$this->logger->info('Skipping workflow step; conditions not met', [
239+
'workflow_id' => $instance->getId(),
240+
'step_id' => $step->getId(),
241+
'conditions' => $step->getConditions(),
242+
]);
243+
244+
$instance->setCurrentStepId($step->getId());
245+
$this->stateManager->markStepCompleted($instance, $step->getId());
246+
247+
return;
248+
}
249+
209250
$this->logger->info('Executing workflow step', [
210251
'workflow_id' => $instance->getId(),
211252
'workflow_name' => $instance->getDefinition()->getName(),
@@ -230,11 +271,7 @@ private function executeStep(WorkflowInstance $instance, Step $step): void
230271
'workflow_id' => $instance->getId(),
231272
'step_id' => $step->getId(),
232273
]);
233-
234-
// Continue execution recursively
235-
$this->processWorkflow($instance);
236-
237-
} catch (Exception $e) {
274+
} catch (\Throwable $e) {
238275
$context = new WorkflowContext(
239276
workflowId: $instance->getId(),
240277
stepId: $step->getId(),
@@ -243,13 +280,11 @@ private function executeStep(WorkflowInstance $instance, Step $step): void
243280
instance: $instance
244281
);
245282

246-
// Create detailed step execution exception
247-
$stepException = match (true) {
248-
$e instanceof ActionNotFoundException => $e,
249-
str_contains($e->getMessage(), 'does not exist') => ActionNotFoundException::classNotFound($step->getActionClass(), $step, $context),
250-
str_contains($e->getMessage(), 'must implement') => ActionNotFoundException::invalidInterface($step->getActionClass(), $step, $context),
251-
default => StepExecutionException::fromException($e, $step, $context)
252-
};
283+
// Wrap non-typed throwables in a StepExecutionException while preserving
284+
// ActionNotFoundException (and other domain exceptions) as-is.
285+
$stepException = $e instanceof ActionNotFoundException
286+
? $e
287+
: StepExecutionException::fromException($e, $step, $context);
253288

254289
$this->logger->error('Workflow step execution failed', [
255290
'workflow_id' => $instance->getId(),
@@ -278,6 +313,13 @@ private function executeStep(WorkflowInstance $instance, Step $step): void
278313
* @throws ActionNotFoundException If the action class doesn't exist
279314
* @throws StepExecutionException If all retry attempts are exhausted
280315
*/
316+
/**
317+
* Maximum backoff sleep between retries, in microseconds. Caps the
318+
* exponential growth so a misconfigured step cannot block a worker for
319+
* minutes.
320+
*/
321+
private const MAX_BACKOFF_MICROSECONDS = 2_000_000; // 2 seconds
322+
281323
private function executeActionWithRetry(WorkflowInstance $instance, Step $step): void
282324
{
283325
$maxAttempts = $step->getRetryAttempts() + 1; // +1 for initial attempt
@@ -289,16 +331,12 @@ private function executeActionWithRetry(WorkflowInstance $instance, Step $step):
289331
return;
290332
}
291333

292-
$lastException = null;
293-
294334
for ($attempt = 1; $attempt <= $maxAttempts; $attempt++) {
295335
try {
296336
$this->executeAction($instance, $step);
297337

298338
return; // Success — exit retry loop
299-
} catch (\Exception $e) {
300-
$lastException = $e;
301-
339+
} catch (\Throwable $e) {
302340
if ($attempt === $maxAttempts) {
303341
$this->logger->error('Step failed after all retry attempts', [
304342
'workflow_id' => $instance->getId(),
@@ -311,11 +349,14 @@ private function executeActionWithRetry(WorkflowInstance $instance, Step $step):
311349
throw $e; // Final attempt failed — propagate
312350
}
313351

352+
$backoffMicroseconds = $this->calculateBackoff($attempt);
353+
314354
$this->logger->warning('Step failed, retrying', [
315355
'workflow_id' => $instance->getId(),
316356
'step_id' => $step->getId(),
317357
'attempt' => $attempt,
318358
'max_attempts' => $maxAttempts,
359+
'backoff_ms' => (int) ($backoffMicroseconds / 1000),
319360
'error' => $e->getMessage(),
320361
]);
321362

@@ -327,51 +368,73 @@ private function executeActionWithRetry(WorkflowInstance $instance, Step $step):
327368
$e
328369
));
329370

330-
// Exponential backoff: 100ms, 200ms, 400ms... (keep short for a library)
331-
$backoffMicroseconds = (int) (100000 * pow(2, $attempt - 1));
332371
usleep($backoffMicroseconds);
333372
}
334373
}
335374
}
336375

376+
/**
377+
* Calculate exponential backoff delay between retry attempts.
378+
*
379+
* Doubles each attempt starting at 100ms, capped at MAX_BACKOFF_MICROSECONDS
380+
* to prevent runaway worker blocking.
381+
*
382+
* @param int $attempt 1-based attempt number
383+
* @return int Delay in microseconds
384+
*/
385+
private function calculateBackoff(int $attempt): int
386+
{
387+
$base = 100_000; // 100ms
388+
$delay = (int) ($base * (2 ** ($attempt - 1)));
389+
390+
return min($delay, self::MAX_BACKOFF_MICROSECONDS);
391+
}
392+
337393
/**
338394
* Execute a callback with a timeout constraint.
339395
*
340-
* Uses pcntl_alarm when available, otherwise logs a warning and executes without timeout.
396+
* Uses pcntl_alarm when the pcntl extension is loaded. pcntl is generally
397+
* only available under the CLI SAPI, so for web/FPM contexts this method
398+
* logs a warning and runs the callback unbounded — long-running workflow
399+
* steps should be dispatched via a queue worker instead.
341400
*
342401
* @param callable $callback The callback to execute
343402
* @param int $timeoutSeconds Maximum execution time in seconds
344403
* @return mixed The callback's return value
345404
*
346-
* @throws StepExecutionException If the timeout is exceeded
405+
* @throws \RuntimeException If the timeout is exceeded while running under pcntl
347406
*/
348407
private function executeWithTimeout(callable $callback, int $timeoutSeconds): mixed
349408
{
350-
if (! function_exists('pcntl_alarm') || ! function_exists('pcntl_signal')) {
409+
if (! function_exists('pcntl_alarm') || ! function_exists('pcntl_signal') || ! function_exists('pcntl_async_signals')) {
351410
$this->logger->warning('pcntl extension not available, timeout not enforced', [
352411
'timeout_seconds' => $timeoutSeconds,
412+
'hint' => 'Execute workflows via CLI or queue workers to enforce step timeouts.',
353413
]);
354414

355415
return $callback();
356416
}
357417

358-
pcntl_signal(SIGALRM, function () use ($timeoutSeconds) {
418+
// Ensure the signal handler runs at the VM tick rather than waiting for
419+
// an explicit pcntl_signal_dispatch() call. Without this, SIGALRM can
420+
// be delivered unpredictably or not at all.
421+
$previousAsync = pcntl_async_signals(true);
422+
$previousHandler = pcntl_signal_get_handler(SIGALRM);
423+
424+
pcntl_signal(SIGALRM, function () use ($timeoutSeconds): never {
359425
throw new \RuntimeException("Step execution timed out after {$timeoutSeconds} seconds");
360426
});
361427

362428
pcntl_alarm($timeoutSeconds);
363429

364430
try {
365-
$result = $callback();
366-
pcntl_alarm(0);
367-
368-
return $result;
369-
} catch (\Exception $e) {
370-
pcntl_alarm(0);
371-
372-
throw $e;
431+
return $callback();
373432
} finally {
374-
pcntl_signal(SIGALRM, SIG_DFL);
433+
// Always clear the alarm and restore the previous signal handler,
434+
// even if the callback threw or the alarm fired.
435+
pcntl_alarm(0);
436+
pcntl_signal(SIGALRM, $previousHandler ?: SIG_DFL);
437+
pcntl_async_signals($previousAsync);
375438
}
376439
}
377440

0 commit comments

Comments
 (0)