diff --git a/src/Interceptor/WorkflowOutboundCalls/TimerInput.php b/src/Interceptor/WorkflowOutboundCalls/TimerInput.php index 5a72cb9e5..8b8ef3b0e 100644 --- a/src/Interceptor/WorkflowOutboundCalls/TimerInput.php +++ b/src/Interceptor/WorkflowOutboundCalls/TimerInput.php @@ -24,7 +24,6 @@ final class TimerInput */ public function __construct( public readonly \DateInterval $interval, - public readonly ?TimerOptions $timerOptions, ) {} diff --git a/src/Internal/Workflow/Process/Process.php b/src/Internal/Workflow/Process/Process.php index cce5c8515..ecbea469a 100644 --- a/src/Internal/Workflow/Process/Process.php +++ b/src/Internal/Workflow/Process/Process.php @@ -55,17 +55,16 @@ public function __construct( $workflowInstance->getQueryDispatcher() ->setQueryExecutor(function (QueryInput $input, callable $handler) use ($inboundPipeline): mixed { try { - return $inboundPipeline->with( + return $this->scopeContext->runReadOnly(fn(): mixed => $inboundPipeline->with( function (QueryInput $input) use ($handler): mixed { - $context = $this->scopeContext - ->withInput(new Input($this->scopeContext->getInfo(), $input->arguments)); - $context->setReadonly(true); - Workflow::setCurrentContext($context); + Workflow::setCurrentContext($this->scopeContext + ->withInput(new Input($this->scopeContext->getInfo(), $input->arguments))); + return $handler($input->arguments); }, /** @see WorkflowInboundCallsInterceptor::handleQuery() */ 'handleQuery', - )($input); + )($input)); } finally { Workflow::setCurrentContext(null); } @@ -76,7 +75,7 @@ function (QueryInput $input) use ($handler): mixed { ->setUpdateValidator(function (UpdateInput $input, callable $handler) use ($inboundPipeline): void { try { Workflow::setCurrentContext($this->scopeContext); - $inboundPipeline->with( + $this->scopeContext->runReadOnly(fn(): mixed => $inboundPipeline->with( function (UpdateInput $input) use ($handler): void { Workflow::setCurrentContext($this->scopeContext->withInput( new Input( @@ -89,7 +88,7 @@ function (UpdateInput $input) use ($handler): void { }, /** @see WorkflowInboundCallsInterceptor::validateUpdate() */ 'validateUpdate', - )($input); + )($input)); } finally { Workflow::setCurrentContext(null); } diff --git a/src/Internal/Workflow/ScopeContext.php b/src/Internal/Workflow/ScopeContext.php index b9a3ebab4..762c9d85c 100644 --- a/src/Internal/Workflow/ScopeContext.php +++ b/src/Internal/Workflow/ScopeContext.php @@ -14,6 +14,7 @@ use React\Promise\Deferred; use React\Promise\PromiseInterface; use Temporal\Exception\Failure\CanceledFailure; +use Temporal\Exception\IllegalStateException; use Temporal\Internal\Transport\CompletableResult; use Temporal\Internal\Workflow\Process\Scope; use Temporal\Worker\Transport\Command\RequestInterface; @@ -55,6 +56,8 @@ public static function fromWorkflowContext( $ctx->onRequest = $onRequest; $ctx->updateContext = $updateContext; $ctx->readonly = $context->readonly; + /** @psalm-suppress UnsupportedPropertyReferenceUsage */ + $ctx->inReadOnlyCallback = &$context->inReadOnlyCallback; $ctx->continueAsNew = $context->continueAsNew; $ctx->trace = &$context->trace; $ctx->currentDetails = &$context->currentDetails; @@ -62,13 +65,29 @@ public static function fromWorkflowContext( return $ctx; } + #[\Override] + public function assertWritable(): void + { + if ($this->inReadOnlyCallback) { + throw new IllegalStateException( + 'Workflow calls that send commands are not allowed inside read-only callbacks.', + ); + } + + $this->parent->assertWritable(); + } + public function async(callable $handler): CancellationScopeInterface { + $this->assertWritable(); + return $this->scope->startScope($handler, false); } public function asyncDetached(callable $handler): CancellationScopeInterface { + $this->assertWritable(); + return $this->scope->startScope($handler, true); } @@ -78,6 +97,8 @@ public function request( bool $cancellable = true, bool $waitResponse = true, ): PromiseInterface { + $this->assertWritable(); + $cancellable && $this->scope->isCancelled() && throw new CanceledFailure( 'Attempt to send request to cancelled scope', ); diff --git a/src/Internal/Workflow/WorkflowContext.php b/src/Internal/Workflow/WorkflowContext.php index f32ed58e9..0814ff90d 100644 --- a/src/Internal/Workflow/WorkflowContext.php +++ b/src/Internal/Workflow/WorkflowContext.php @@ -17,6 +17,7 @@ use React\Promise\Deferred; use React\Promise\Exception\LengthException; use React\Promise\PromiseInterface; +use Temporal\Exception\IllegalStateException; use Temporal\Activity\ActivityOptions; use Temporal\Activity\ActivityOptionsInterface; use Temporal\Activity\LocalActivityOptions; @@ -71,6 +72,7 @@ use Temporal\Promise; use Temporal\Worker\FeatureFlags; use Temporal\Worker\Transport\Command\RequestInterface; +use Temporal\Workflow; use Temporal\Workflow\ActivityStubInterface; use Temporal\Workflow\ChildWorkflowOptions; use Temporal\Workflow\ChildWorkflowStubInterface; @@ -102,6 +104,7 @@ class WorkflowContext implements WorkflowContextInterface, HeaderCarrier, Destro protected array $trace = []; protected bool $continueAsNew = false; protected bool $readonly = true; + protected bool $inReadOnlyCallback = false; protected ?string $currentDetails = null; /** @var Pipeline */ @@ -176,10 +179,50 @@ public function setReadonly(bool $value = true): static return $this; } + public function isReadonly(): bool + { + return $this->readonly || $this->inReadOnlyCallback; + } + + public function assertWritable(): void + { + if ($this->inReadOnlyCallback) { + throw new IllegalStateException( + 'Workflow calls that send commands are not allowed inside read-only callbacks.', + ); + } + + if ($this->readonly) { + throw new \RuntimeException('Workflow is not initialized.'); + } + } + + /** + * @template T + * @param callable(): T $callback + * @return T + */ + public function runReadOnly(callable $callback): mixed + { + if ($this->inReadOnlyCallback || !FeatureFlags::$readOnlyWorkflowCallbacks) { + return $callback(); + } + + $this->inReadOnlyCallback = true; + + try { + return $callback(); + } finally { + $this->inReadOnlyCallback = false; + } + } + public function withInput(Input $input): static { $clone = clone $this; $clone->awaits = &$this->awaits; + /** @psalm-suppress UnsupportedPropertyReferenceUsage */ + $clone->inReadOnlyCallback = &$this->inReadOnlyCallback; $clone->trace = &$this->trace; $clone->input = $input; return $clone; @@ -264,11 +307,13 @@ public function sideEffect(callable $context, ?SideEffectOptions $options = null try { if (!$this->isReplaying()) { - $value = $this->callsInterceptor->with( - $closure, - /** @see WorkflowOutboundCallsInterceptor::sideEffect() */ - 'sideEffect', - )(new SideEffectInput($closure, $options)); + $value = $this->runReadOnly( + fn(): mixed => $this->callsInterceptor->with( + $closure, + /** @see WorkflowOutboundCallsInterceptor::sideEffect() */ + 'sideEffect', + )(new SideEffectInput($closure, $options)), + ); } } catch (\Throwable $e) { return reject($e); @@ -491,7 +536,7 @@ public function request( bool $cancellable = true, bool $waitResponse = true, ): PromiseInterface { - $this->readonly and throw new \RuntimeException('Workflow is not initialized.'); + $this->assertWritable(); $this->recordTrace(); // Intercept workflow outbound calls @@ -672,7 +717,7 @@ public function resolveConditions(): void { foreach ($this->awaits as $awaitsGroupId => $awaitsGroup) { foreach ($awaitsGroup as $i => [$condition, $deferred]) { - if ($condition()) { + if ($this->runReadOnly($condition)) { unset($this->awaits[$awaitsGroupId][$i]); $deferred->resolve(null); $this->resolveConditionGroup($awaitsGroupId); @@ -776,7 +821,9 @@ protected function awaitRequest(callable|Mutex|PromiseInterface ...$conditions): $condition instanceof Mutex and $condition = static fn(): bool => !$condition->isLocked(); if ($condition instanceof \Closure) { - $callableResult = $condition($conditionGroupId); + $callableResult = $this->runReadOnly( + static fn(): mixed => $condition($conditionGroupId), + ); if ($callableResult === true) { $this->resolveConditionGroup($conditionGroupId); return resolve(true); @@ -846,6 +893,10 @@ protected function addCondition(string $conditionGroupId, callable $condition): */ protected function recordTrace(): void { - $this->readonly or $this->trace = \debug_backtrace(\DEBUG_BACKTRACE_IGNORE_ARGS); + if ($this->isReadonly()) { + return; + } + + $this->trace = \debug_backtrace(\DEBUG_BACKTRACE_IGNORE_ARGS); } } diff --git a/src/Worker/FeatureFlags.php b/src/Worker/FeatureFlags.php index f4e15eabd..14705fe36 100644 --- a/src/Worker/FeatureFlags.php +++ b/src/Worker/FeatureFlags.php @@ -89,4 +89,15 @@ final class FeatureFlags * @link https://github.com/temporalio/sdk-php/issues/399 */ public static bool $settleAwaitOnFirstSettledCondition = false; + + /** + * Reject a call that sends a command from a callback that only observes the workflow: a query + * handler, an update validator, an await condition or a {@see Workflow::sideEffect()} callback. + * Such a command does not belong to the workflow's own sequence and breaks replay. + * + * Set to FALSE to keep the previous behavior, where the command is created. + * + * @since SDK 2.18.0 + */ + public static bool $readOnlyWorkflowCallbacks = true; } diff --git a/testing/src/TestService.php b/testing/src/TestService.php index 5e1b58e31..fbfc7571f 100644 --- a/testing/src/TestService.php +++ b/testing/src/TestService.php @@ -20,7 +20,6 @@ final class TestService { private TestServiceClient $testServiceClient; - private int $lockDelta = 0; public function __construct(TestServiceClient $testServiceClient) @@ -28,6 +27,13 @@ public function __construct(TestServiceClient $testServiceClient) $this->testServiceClient = $testServiceClient; } + public static function create(string $host): self + { + return new self( + new TestServiceClient($host, ['credentials' => ChannelCredentials::createInsecure()]), + ); + } + /** * Net lock/unlock delta applied through this instance since it was created. * @@ -39,13 +45,6 @@ public function lockDelta(): int return $this->lockDelta; } - public static function create(string $host): self - { - return new self( - new TestServiceClient($host, ['credentials' => ChannelCredentials::createInsecure()]), - ); - } - /** * Increments Time Locking Counter by one. * diff --git a/testing/src/WorkflowTestCase.php b/testing/src/WorkflowTestCase.php index 35455c3c5..7a4b9cd67 100644 --- a/testing/src/WorkflowTestCase.php +++ b/testing/src/WorkflowTestCase.php @@ -50,6 +50,19 @@ protected function tearDown(): void parent::tearDown(); } + /** + * @return list + */ + protected function clientInterceptors(): array + { + return []; + } + + protected function interactions(WorkflowRunInterface $run): WorkflowInteractions + { + return WorkflowInteractions::of($this->workflowClient, $run); + } + private function assertTimeSkippingBalanced(): void { $delta = $this->testingService->lockDelta(); @@ -71,17 +84,4 @@ private function assertTimeSkippingBalanced(): void $delta, )); } - - /** - * @return list - */ - protected function clientInterceptors(): array - { - return []; - } - - protected function interactions(WorkflowRunInterface $run): WorkflowInteractions - { - return WorkflowInteractions::of($this->workflowClient, $run); - } } diff --git a/tests/Unit/Internal/Support/DateIntervalTestCase.php b/tests/Unit/Internal/Support/DateIntervalTestCase.php index dd203729e..9056b1808 100644 --- a/tests/Unit/Internal/Support/DateIntervalTestCase.php +++ b/tests/Unit/Internal/Support/DateIntervalTestCase.php @@ -201,7 +201,6 @@ public function testParseDetectsIso8601FormatCorrectly(string $interval, bool $s // Arrange $reflection = new \ReflectionClass(DateInterval::class); $method = $reflection->getMethod('isIso8601DurationFormat'); - $method->setAccessible(true); // Act $result = $method->invoke(null, $interval); diff --git a/tests/Unit/Internal/Workflow/ReadOnlyCallbackTestCase.php b/tests/Unit/Internal/Workflow/ReadOnlyCallbackTestCase.php new file mode 100644 index 000000000..536cc5924 --- /dev/null +++ b/tests/Unit/Internal/Workflow/ReadOnlyCallbackTestCase.php @@ -0,0 +1,385 @@ +start(new QueryingWorkflow()); + $queued = $this->factory->getQueue()->count(); + + $handler = $instance->getQueryDispatcher()->findQueryHandler('sendsACommand'); + self::assertNotNull($handler); + + $error = null; + + try { + $handler(new QueryInput('sendsACommand', EncodedValues::empty(), $process->getContext()->getInfo())); + } catch (\Throwable $e) { + $error = $e; + } finally { + Workflow::setCurrentContext(null); + } + + self::assertSame($queued, $this->factory->getQueue()->count(), 'A query handler created a command.'); + self::assertInstanceOf(IllegalStateException::class, $error); + self::assertSame('Workflow calls that send commands are not allowed inside read-only callbacks.', $error->getMessage()); + } + + public function testAQueryHandlerDoesNotOverwriteTheWorkflowTrace(): void + { + [$process, $instance] = $this->start(new QueryingWorkflow()); + $before = $process->getContext()->getStackTrace(); + + try { + $handler = $instance->getQueryDispatcher()->findQueryHandler('sendsACommand'); + $handler(new QueryInput('sendsACommand', EncodedValues::empty(), $process->getContext()->getInfo())); + } catch (\Throwable) { + } finally { + Workflow::setCurrentContext(null); + } + + self::assertSame($before, $process->getContext()->getStackTrace()); + } + + public function testAnUpdateValidatorCannotCreateACommand(): void + { + $workflow = new ValidatingWorkflow(); + [$process, $instance] = $this->start($workflow); + $queued = $this->factory->getQueue()->count(); + + $error = null; + + try { + $validator = $instance->getUpdateDispatcher()->findValidateUpdateHandler('sendsACommand'); + self::assertNotNull($validator); + $validator(new UpdateInput( + 'sendsACommand', + 'update-id', + $process->getContext()->getInfo(), + EncodedValues::empty(), + Header::empty(), + false, + )); + } catch (\Throwable $e) { + $error = $e; + } finally { + Workflow::setCurrentContext(null); + } + + self::assertSame($queued, $this->factory->getQueue()->count(), 'An update validator created a command.'); + self::assertInstanceOf(IllegalStateException::class, $error); + self::assertSame('Workflow calls that send commands are not allowed inside read-only callbacks.', $error->getMessage()); + } + + public function testTheGuardIsDisabledByItsFeatureFlag(): void + { + FeatureFlags::$readOnlyWorkflowCallbacks = false; + + try { + $workflow = new ConditionSendingWorkflow(); + $this->start($workflow); + + self::assertNull($workflow->error); + self::assertGreaterThan(0, $this->factory->getQueue()->count()); + } finally { + FeatureFlags::$readOnlyWorkflowCallbacks = true; + } + } + + public function testAnAwaitConditionCannotCreateACommand(): void + { + $workflow = new ConditionSendingWorkflow(); + $this->start($workflow); + + self::assertSame(0, $this->factory->getQueue()->count(), 'An await condition created a command.'); + self::assertInstanceOf(IllegalStateException::class, $workflow->error); + self::assertSame('Workflow calls that send commands are not allowed inside read-only callbacks.', $workflow->error->getMessage()); + } + + public function testASideEffectCallbackCannotCreateACommand(): void + { + $workflow = new SideEffectSendingWorkflow(); + $this->start($workflow); + + self::assertInstanceOf(IllegalStateException::class, $workflow->error); + self::assertSame('Workflow calls that send commands are not allowed inside read-only callbacks.', $workflow->error->getMessage()); + } + + public function testAnInboundInterceptorCannotCreateACommandAroundAQueryHandler(): void + { + $interceptor = new CommandSendingInboundInterceptor(); + [$process, $instance] = $this->start(new HarmlessHandlersWorkflow(), [$interceptor]); + + $handler = $instance->getQueryDispatcher()->findQueryHandler('harmless'); + self::assertNotNull($handler); + + $handler(new QueryInput('harmless', EncodedValues::empty(), $process->getContext()->getInfo())); + + self::assertInstanceOf(IllegalStateException::class, $interceptor->error); + self::assertSame('Workflow calls that send commands are not allowed inside read-only callbacks.', $interceptor->error->getMessage()); + } + + public function testAnInboundInterceptorCannotCreateACommandAroundAnUpdateValidator(): void + { + $interceptor = new CommandSendingInboundInterceptor(); + [$process, $instance] = $this->start(new HarmlessHandlersWorkflow(), [$interceptor]); + + $validator = $instance->getUpdateDispatcher()->findValidateUpdateHandler('harmless'); + self::assertNotNull($validator); + + $validator(new UpdateInput( + 'harmless', + 'update-id', + $process->getContext()->getInfo(), + EncodedValues::empty(), + Header::empty(), + false, + )); + + self::assertInstanceOf(IllegalStateException::class, $interceptor->error); + self::assertSame('Workflow calls that send commands are not allowed inside read-only callbacks.', $interceptor->error->getMessage()); + } + + protected function tearDown(): void + { + Workflow::setCurrentContext(null); + } + + /** + * @return array{Process, WorkflowInstance} + */ + private function start(object $workflow, array $interceptors = []): array + { + $this->factory = new WorkerFactoryMock(DataConverter::createDefault()); + $services = ServiceContainer::fromWorkerFactory( + $this->factory, + ExceptionInterceptor::createDefault(), + new SimplePipelineProvider($interceptors), + new StderrLogger(), + ); + + $reflection = new \ReflectionClass($workflow); + $prototype = new WorkflowPrototype( + $reflection->getShortName(), + $reflection->getMethod('handle'), + $reflection, + ); + + foreach ($reflection->getMethods() as $method) { + foreach ($method->getAttributes(Workflow\UpdateMethod::class) as $attribute) { + $name = $attribute->newInstance()->name ?? $method->getName(); + $validator = null; + + foreach ($reflection->getMethods() as $candidate) { + foreach ($candidate->getAttributes(UpdateValidatorMethod::class) as $validatorAttribute) { + $validatorAttribute->newInstance()->forUpdate === $method->getName() and $validator = $candidate; + } + } + + $prototype->addUpdateHandler(new UpdateDefinition( + $name, + '', + HandlerUnfinishedPolicy::WarnAndAbandon, + 'string', + $method, + $validator, + )); + } + + foreach ($method->getAttributes(QueryMethod::class) as $attribute) { + $prototype->addQueryHandler(new QueryDefinition( + $attribute->newInstance()->name ?? $method->getName(), + 'string', + $method, + '', + )); + } + } + + $instance = new WorkflowInstance($prototype, $workflow); + $context = new WorkflowContext( + $services, + $services->client, + $instance, + new Input(), + EncodedValues::empty(), + ); + $process = new Process($services, 'run-id', $instance); + $process->initAndStart($context, $instance, false); + $this->factory->tick(); + + return [$process, $instance]; + } +} + +#[WorkflowInterface] +final class QueryingWorkflow +{ + #[WorkflowMethod(name: 'QueryingWorkflow')] + public function handle(): \Generator + { + yield Workflow::await(static fn(): bool => false); + } + + #[QueryMethod(name: 'sendsACommand')] + public function sendsACommand(): string + { + Workflow::timer(5); + + return 'unreachable'; + } +} + +#[WorkflowInterface] +final class ValidatingWorkflow +{ + #[WorkflowMethod(name: 'ValidatingWorkflow')] + public function handle(): \Generator + { + yield Workflow::await(static fn(): bool => false); + } + + #[Workflow\UpdateMethod(name: 'sendsACommand')] + public function sendsACommand(): string + { + return 'done'; + } + + #[UpdateValidatorMethod(forUpdate: 'sendsACommand')] + public function validateSendsACommand(): void + { + Workflow::timer(5); + } +} + +#[WorkflowInterface] +final class ConditionSendingWorkflow +{ + public ?\Throwable $error = null; + + #[WorkflowMethod(name: 'ConditionSendingWorkflow')] + public function handle(): \Generator + { + yield Workflow::await(function (): bool { + try { + Workflow::timer(5); + } catch (\Throwable $e) { + $this->error ??= $e; + } + + return false; + }); + } +} + +#[WorkflowInterface] +final class SideEffectSendingWorkflow +{ + public ?\Throwable $error = null; + + #[WorkflowMethod(name: 'SideEffectSendingWorkflow')] + public function handle(): \Generator + { + yield Workflow::sideEffect(function (): int { + try { + Workflow::timer(5); + } catch (\Throwable $e) { + $this->error ??= $e; + } + + return 1; + }); + + yield Workflow::await(static fn(): bool => false); + } +} + +final class CommandSendingInboundInterceptor implements WorkflowInboundCallsInterceptor +{ + use WorkflowInboundCallsInterceptorTrait; + + public ?\Throwable $error = null; + + public function handleQuery(QueryInput $input, callable $next): mixed + { + $result = $next($input); + $this->sendACommand(); + + return $result; + } + + public function validateUpdate(UpdateInput $input, callable $next): void + { + $next($input); + $this->sendACommand(); + } + + private function sendACommand(): void + { + try { + Workflow::timer(5); + } catch (\Throwable $e) { + $this->error ??= $e; + } + } +} + +#[WorkflowInterface] +final class HarmlessHandlersWorkflow +{ + #[WorkflowMethod(name: 'HarmlessHandlersWorkflow')] + public function handle(): \Generator + { + yield Workflow::await(static fn(): bool => false); + } + + #[QueryMethod(name: 'harmless')] + public function harmlessQuery(): string + { + return 'ok'; + } + + #[Workflow\UpdateMethod(name: 'harmless')] + public function harmlessUpdate(): string + { + return 'done'; + } + + #[UpdateValidatorMethod(forUpdate: 'harmlessUpdate')] + public function validateHarmless(): void {} +}