Refactor executor to iterative loop, add condition evaluation, improve validation#36
Merged
Refactor executor to iterative loop, add condition evaluation, improve validation#36
Conversation
… 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).
…ction 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).
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR makes significant improvements to the workflow execution engine, focusing on robustness, correctness, and scalability:
Executor Refactoring: Converts the recursive
processWorkflow()method to an iterative loop with a safety bound to prevent infinite loops and stack overflows on long workflows.Conditional Step Execution: Implements proper condition evaluation in
executeStep(). Steps whose conditions don't match are now skipped (marked completed without running the action) so downstream transitions continue to flow.Enhanced Condition Evaluator: Extends
ConditionEvaluatorto support:"key"or"!key"to check if a dotted path is truthy/falsytrue,false,null, integers, floats, and quoted stringsValidation on Instance Restore:
WorkflowInstance::fromArray()now validates that:completed_stepsandfailed_stepsreference steps defined in the workflowWorkflowStatevalueid,state) are present and well-formedInvalidWorkflowStateExceptionBuilder Improvements:
when()conditions that accumulateException Handling: Changed from catching
Exceptionto\Throwablethroughout for better error handling, and simplified exception wrapping logic inexecuteStep().Security & Reliability:
random_bytes()instead ofmt_rand()WorkflowExceptionType of Change
Testing
BuilderExecutionTest.php: End-to-end tests for builder-defined workflows with multi-step execution and conditional branchingConditionEvaluatorTest.php: Comprehensive tests for condition evaluation (comparisons, truthy keys, negation, edge cases)ExecutorIterationTest.php: Tests that 200-step workflows execute without stack overflowWorkflowInstanceRestoreTest.php: Validation tests for instance restoration from arraysWorkflowBuilderAutoIdTest.php: Tests for auto-generated step ID collision avoidanceChecklist
Additional Notes
The executor refactoring is a significant architectural change that improves both correctness and performance. The iterative approach with a bounded iteration count (
maxIterations = totalSteps * 2 + 1) prevents pathological workflows from looping forever while still allowing legitimate conditional branching. The +1 guard handles zero-step workflows.Condition evaluation now supports both comparison expressions (
user.plan === 'premium') and truthy key checks (review.approved,!review.rejected), making workflow definitions more expressive and readable.Instance validation on restore prevents silent corruption when loading workflows from storage, catching issues early with clear error messages.
https://claude.ai/code/session_01FfatPksXmoqBpfmELb8fza