diff --git a/CHANGELOG.md b/CHANGELOG.md index 40a661d78..ed5542825 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,13 @@ # Changelog +## 2.11.3 - 2026-10-23 + +- Refuse dependency injection that would silently discard recorded activity + input before queued or local activity code runs. Record a typed attempt + failure and preserve the authored retry policy, original input and identity. +- Record activity entry dependency-resolution failures through the normal + outcome path instead of leaving the attempt leased without an outcome. + ## 2.11.2 - 2026-10-23 - Deliver unpinned activity tasks to eligible versioned workers during rolling diff --git a/composer.json b/composer.json index b6d8a10de..8c8420ea1 100644 --- a/composer.json +++ b/composer.json @@ -87,7 +87,7 @@ "dev-main": "2.0.x-dev" }, "durable-workflow": { - "product-train": "2.11.2", + "product-train": "2.11.3", "laravel-embedded-upgrade-contract": "resources/laravel-embedded-upgrade-contract.json", "laravel-dependency-security-policy": "resources/laravel-dependency-security-policy.json" }, diff --git a/src/V2/Exceptions/ActivityEntryArgumentsMismatchException.php b/src/V2/Exceptions/ActivityEntryArgumentsMismatchException.php new file mode 100644 index 000000000..29b825f32 --- /dev/null +++ b/src/V2/Exceptions/ActivityEntryArgumentsMismatchException.php @@ -0,0 +1,19 @@ +activity_class, $execution->activity_type); $activity = RuntimeObjectFactory::activity($activityClass, $execution, $execution->run, $this->taskId); $entryMethod = EntryMethod::forActivity($activity); - $arguments = $activity->resolveMethodDependencies($execution->activityArguments(), $entryMethod); $activityArguments = $execution->activityArguments(); $result = null; $throwable = null; + try { + $arguments = ActivityEntryArguments::resolve($activity, $activityArguments, $entryMethod); + } catch (Throwable $error) { + $arguments = []; + $throwable = $error; + } if (WorkflowStub::faked()) { WorkflowStub::recordDispatched($execution->activity_class, $activityArguments); } - if (WorkflowStub::hasMock($execution->activity_class)) { + if ($throwable === null && WorkflowStub::hasMock($execution->activity_class)) { $mocked = WorkflowStub::mockedResult( $execution->activity_class, new ActivityFakeContext( @@ -147,7 +153,7 @@ private function executeMaintainedClaim(ActivityTaskClaim $claim, ?EmbeddedActiv $result = $mocked['result']; $throwable = $mocked['throwable']; - } else { + } elseif ($throwable === null) { try { $result = $activity->{$entryMethod->getName()}(...$arguments); } catch (Throwable $error) { diff --git a/src/V2/Support/ActivityEntryArguments.php b/src/V2/Support/ActivityEntryArguments.php new file mode 100644 index 000000000..d119fe18f --- /dev/null +++ b/src/V2/Support/ActivityEntryArguments.php @@ -0,0 +1,31 @@ +resolveMethodDependencies($recorded, $entry); + + // A new injected value must not push recorded input past the last + // parameter, where PHP would silently discard it. Existing variadic + // handlers can consume those values and keep their ordinary behavior. + if (! $entry->isVariadic() && count($resolved) > $entry->getNumberOfParameters() + && count($resolved) > count($recorded)) { + throw new ActivityEntryArgumentsMismatchException( + $activity->execution->activity_type ?? $activity::class, + $entry->getName(), + ); + } + + return $resolved; + } +} diff --git a/src/V2/Support/LocalActivityExecutor.php b/src/V2/Support/LocalActivityExecutor.php index 88146f9db..3f598b41f 100644 --- a/src/V2/Support/LocalActivityExecutor.php +++ b/src/V2/Support/LocalActivityExecutor.php @@ -389,16 +389,20 @@ private function invokeCallback(WorkflowRun $run, WorkflowTask $task, ActivityEx $activity = RuntimeObjectFactory::activity($activityClass, $execution, $run, $task->id); $entryMethod = EntryMethod::forActivity($activity); $activityArguments = $execution->activityArguments(); - $arguments = $activity->resolveMethodDependencies($activityArguments, $entryMethod); - $result = null; $throwable = null; + try { + $arguments = ActivityEntryArguments::resolve($activity, $activityArguments, $entryMethod); + } catch (Throwable $error) { + $arguments = []; + $throwable = $error; + } if (WorkflowStub::faked()) { WorkflowStub::recordDispatched($execution->activity_class, $activityArguments); } - if (WorkflowStub::hasMock($execution->activity_class)) { + if ($throwable === null && WorkflowStub::hasMock($execution->activity_class)) { $mocked = WorkflowStub::mockedResult( $execution->activity_class, new ActivityFakeContext( @@ -413,7 +417,7 @@ private function invokeCallback(WorkflowRun $run, WorkflowTask $task, ActivityEx $result = $mocked['result']; $throwable = $mocked['throwable']; - } else { + } elseif ($throwable === null) { try { $result = $activity->{$entryMethod->getName()}(...$arguments); } catch (Throwable $error) { diff --git a/tests/Feature/V2/V2ActivityEntryArgumentsTest.php b/tests/Feature/V2/V2ActivityEntryArgumentsTest.php new file mode 100644 index 000000000..8f937e043 --- /dev/null +++ b/tests/Feature/V2/V2ActivityEntryArgumentsTest.php @@ -0,0 +1,310 @@ +scheduled(ActivityEntryDto::class, [7]); + $this->app->call([new RunActivityTask($task->id), 'handle']); + + self::assertFalse(ActivityEntryDto::$entered, 'The incompatible activity body ran.'); + self::assertSame($arguments, $execution->fresh()->getRawOriginal('arguments')); + self::assertSame([7], $execution->fresh()->activityArguments()); + self::assertSame(ActivityStatus::Failed, $execution->fresh()->status); + self::assertSame(1, $execution->fresh()->attempt_count); + $event = $this->history($stub->runId(), 'ActivityFailed'); + self::assertSame(ActivityEntryArgumentsMismatchException::class, $event->payload['exception_class']); + self::assertStringContainsString('recorded_argument_would_be_discarded', $event->payload['message']); + + $this->runWorkflowTask(); + self::assertTrue($stub->refresh()->failed()); + self::assertNull($stub->output()); + self::assertSame(0, WorkflowTask::query()->sum('repair_count')); + } + + public function testAuthoredRetryUsesTheSameOriginalInputAfterCompatibleCodeIsRestored(): void + { + [$stub, $execution, $task, $arguments] = $this->scheduled(ActivityEntryRetryDto::class, [7]); + $originalRunId = $stub->runId(); + $originalExecutionId = $execution->id; + $this->app->call([new RunActivityTask($task->id), 'handle']); + + self::assertFalse(ActivityEntryDto::$entered); + self::assertFalse($stub->refresh()->run()->status->isTerminal()); + self::assertSame(ActivityStatus::Pending, $execution->fresh()->status); + self::assertSame(1, $execution->fresh()->attempt_count); + $retry = $this->history($originalRunId, 'ActivityRetryScheduled'); + self::assertSame(ActivityEntryArgumentsMismatchException::class, $retry->payload['exception_class']); + + // A new deployment can supply a compatible implementation through the + // normal Laravel binding without rewriting durable class/input fields. + $this->app->bind(ActivityEntryRetryDto::class, static fn (): Activity => new ActivityEntryScalar()); + $retryTask = WorkflowTask::query()->where('task_type', TaskType::Activity->value) + ->where('status', TaskStatus::Ready->value)->sole(); + $this->app->call([new RunActivityTask($retryTask->id), 'handle']); + $this->runWorkflowTask(); + + self::assertTrue($stub->refresh()->completed()); + self::assertSame(7, $stub->output()); + self::assertSame($originalRunId, $stub->runId()); + self::assertSame($originalExecutionId, $execution->fresh()->id); + self::assertSame($arguments, $execution->fresh()->getRawOriginal('arguments')); + self::assertSame(2, $execution->fresh()->attempt_count); + self::assertSame(1, WorkflowHistoryEvent::query()->where('event_type', 'ActivityCompleted')->count()); + self::assertSame(0, WorkflowHistoryEvent::query()->where('event_type', 'ActivityFailed')->count()); + self::assertSame(0, WorkflowTask::query()->sum('repair_count')); + } + + #[DataProvider('compatibleEntries')] + public function testLegitimateDependencyInjectionAndDefaultsKeepRecordedValues( + string $activity, + array $arguments, + mixed $expected, + ): void { + [$stub, $execution, $task, $storedArguments] = $this->scheduled($activity, $arguments); + $this->app->call([new RunActivityTask($task->id), 'handle']); + $this->runWorkflowTask(); + + self::assertTrue($stub->refresh()->completed()); + self::assertSame($expected, $stub->output()); + self::assertSame($storedArguments, $execution->fresh()->getRawOriginal('arguments')); + self::assertSame(1, $execution->fresh()->attempt_count); + self::assertSame(0, WorkflowTask::query()->sum('repair_count')); + } + + public static function compatibleEntries(): iterable + { + yield 'service before scalar and default' => [ActivityEntryInjected::class, [7], [7, 'USD']]; + yield 'service and variadic values' => [ActivityEntryVariadic::class, [7, 8], 15]; + yield 'scalar without injection' => [ActivityEntryScalar::class, [7], 7]; + } + + public function testLocalActivityCannotDiscardRecordedInputDuringDependencyInjection(): void + { + ActivityEntryWorkflow::$local = true; + ActivityEntryWorkflow::$activity = ActivityEntryDto::class; + ActivityEntryWorkflow::$arguments = [7]; + $stub = WorkflowStub::make(ActivityEntryWorkflow::class); + $stub->start(); + $this->runWorkflowTask(); + + self::assertFalse(ActivityEntryDto::$entered, 'The incompatible local activity body ran.'); + $execution = ActivityExecution::query()->sole(); + self::assertSame([7], $execution->activityArguments()); + self::assertSame(ActivityStatus::Failed, $execution->status); + self::assertSame(1, $execution->attempt_count); + $event = $this->history($stub->runId(), 'ActivityFailed'); + self::assertSame(ActivityEntryArgumentsMismatchException::class, $event->payload['exception_class']); + self::assertSame(0, WorkflowTask::query()->where('task_type', TaskType::Activity->value)->count()); + self::assertTrue($stub->refresh()->failed()); + } + + #[DataProvider('executionModes')] + public function testDependencyResolutionFailureHasAnActivityOutcome(bool $local): void + { + ActivityEntryWorkflow::$local = $local; + ActivityEntryWorkflow::$activity = ActivityEntryRequiredDto::class; + ActivityEntryWorkflow::$arguments = [7]; + $stub = WorkflowStub::make(ActivityEntryWorkflow::class); + $stub->start(); + $this->runWorkflowTask(); + if (! $local) { + $task = WorkflowTask::query()->where('task_type', TaskType::Activity->value)->sole(); + $this->app->call([new RunActivityTask($task->id), 'handle']); + $this->runWorkflowTask(); + } + + self::assertFalse(ActivityEntryDto::$entered); + $execution = ActivityExecution::query()->sole(); + self::assertSame(ActivityStatus::Failed, $execution->status); + self::assertSame([7], $execution->activityArguments()); + self::assertSame(1, $execution->attempt_count); + $event = $this->history($stub->runId(), 'ActivityFailed'); + self::assertSame( + \Illuminate\Contracts\Container\BindingResolutionException::class, + $event->payload['exception_class'] + ); + self::assertTrue($stub->refresh()->failed()); + self::assertSame(0, WorkflowTask::query()->where('status', TaskStatus::Leased->value)->count()); + } + + public static function executionModes(): iterable + { + yield 'queued' => [false]; + yield 'local' => [true]; + } + + protected function getPackageProviders($app): array + { + return [WorkflowServiceProvider::class]; + } + + protected function defineEnvironment($app): void + { + $driver = getenv('DB_CONNECTION') ?: 'sqlite'; + $app['config']->set('database.default', $driver); + if ($driver === 'sqlite') { + $app['config']->set('database.connections.sqlite.database', ':memory:'); + } + $app['config']->set([ + 'app.key' => 'base64:' . base64_encode(str_repeat('a', 32)), + 'cache.default' => 'array', + 'queue.default' => 'redis', + 'queue.connections.redis.driver' => 'redis', + 'workflows.v1.enabled' => false, + 'workflows.serializer' => 'avro', + 'workflows.v2.execution_continuation.enabled' => false, + ]); + } + + private function scheduled(string $activity, array $arguments): array + { + ActivityEntryWorkflow::$activity = $activity; + ActivityEntryWorkflow::$arguments = $arguments; + $stub = WorkflowStub::make(ActivityEntryWorkflow::class); + $stub->start(); + $this->runWorkflowTask(); + $execution = ActivityExecution::query()->sole(); + $task = WorkflowTask::query()->where('task_type', TaskType::Activity->value)->sole(); + + return [$stub, $execution, $task, $execution->getRawOriginal('arguments')]; + } + + private function runWorkflowTask(): void + { + $task = WorkflowTask::query()->where('task_type', TaskType::Workflow->value) + ->where('status', TaskStatus::Ready->value)->sole(); + $this->app->call([new RunWorkflowTask($task->id), 'handle']); + } + + private function history(string $runId, string $type): WorkflowHistoryEvent + { + return WorkflowHistoryEvent::query()->where('workflow_run_id', $runId)->where('event_type', $type)->sole(); + } +} + +class ActivityEntryWorkflow extends Workflow +{ + public static bool $local = false; + + public static string $activity; + + public static array $arguments; + + public function handle(): mixed + { + if (self::$local) { + return \Workflow\V2\localActivity(self::$activity, ...self::$arguments); + } + + return \Workflow\V2\activity(self::$activity, ...self::$arguments); + } +} + +class ActivityEntryData +{ + public function __construct( + public int $amount = 0 + ) { + } +} + +class ActivityEntryDto extends Activity +{ + public static bool $entered = false; + + public function handle(ActivityEntryData $data): int + { + self::$entered = true; + + return $data->amount; + } +} + +class ActivityEntryRequiredData +{ + public function __construct( + public int $amount + ) { + } +} + +class ActivityEntryRequiredDto extends Activity +{ + public function handle(ActivityEntryRequiredData $data): int + { + ActivityEntryDto::$entered = true; + + return $data->amount; + } +} + +class ActivityEntryRetryDto extends ActivityEntryDto +{ + public int $tries = 2; + + public function backoff(): int + { + return 0; + } +} + +class ActivityEntryScalar extends Activity +{ + public function handle(int $amount): int + { + return $amount; + } +} + +class ActivityEntryService +{ +} + +class ActivityEntryInjected extends Activity +{ + public function handle(ActivityEntryService $service, int $amount, string $currency = 'USD'): array + { + return [$amount, $currency]; + } +} + +class ActivityEntryVariadic extends Activity +{ + public function handle(ActivityEntryService $service, int ...$amounts): int + { + return array_sum($amounts); + } +} diff --git a/tests/Unit/V2/ActivityEntryArgumentsTest.php b/tests/Unit/V2/ActivityEntryArgumentsTest.php new file mode 100644 index 000000000..9927d09a8 --- /dev/null +++ b/tests/Unit/V2/ActivityEntryArgumentsTest.php @@ -0,0 +1,123 @@ +previousContainer = Container::getInstance(); + $container = new Container(); + $container->instance('config', new Repository()); + Container::setInstance($container); + } + + protected function tearDown(): void + { + Container::setInstance($this->previousContainer); + parent::tearDown(); + } + + public function testMismatchDiagnosticContainsIdentityButNoRecordedValues(): void + { + $activity = $this->activity(ArgumentDtoActivity::class); + try { + ActivityEntryArguments::resolve( + $activity, + ['private-payment-note'], + new ReflectionMethod($activity, 'handle') + ); + self::fail('Injected DTO displaced recorded input.'); + } catch (ActivityEntryArgumentsMismatchException $exception) { + self::assertStringContainsString('unit-activity-type', $exception->getMessage()); + self::assertStringContainsString('handle', $exception->getMessage()); + self::assertStringContainsString('recorded_argument_would_be_discarded', $exception->getMessage()); + self::assertStringNotContainsString('private-payment-note', $exception->getMessage()); + } + } + + public function testInjectionWithoutRecordedInputRemainsValid(): void + { + $activity = $this->activity(ArgumentDtoActivity::class); + $resolved = ActivityEntryArguments::resolve($activity, [], new ReflectionMethod($activity, 'handle')); + + self::assertCount(1, $resolved); + self::assertInstanceOf(ArgumentData::class, $resolved[0]); + self::assertSame(0, $activity->handle(...$resolved)); + } + + public function testOverlappingServiceInterfacesDoNotMisclassifyTheRecordedNull(): void + { + Container::getInstance()->bind(FirstArgumentService::class, CombinedArgumentService::class); + $activity = $this->activity(NullableServiceActivity::class); + $resolved = ActivityEntryArguments::resolve($activity, [null], new ReflectionMethod($activity, 'handle')); + + self::assertCount(2, $resolved); + self::assertInstanceOf(FirstArgumentService::class, $resolved[0]); + self::assertNull($resolved[1]); + self::assertNull($activity->handle(...$resolved)); + } + + private function activity(string $class): Activity + { + return RuntimeObjectFactory::activity( + $class, + new ActivityExecution([ + 'activity_type' => 'unit-activity-type', + ]), + new WorkflowRun(), + 'unit-task', + ); + } +} + +class ArgumentData +{ + public function __construct( + public int $amount = 0 + ) { + } +} + +class ArgumentDtoActivity extends Activity +{ + public function handle(ArgumentData $data): int + { + return $data->amount; + } +} + +interface FirstArgumentService +{ +} + +interface SecondArgumentService +{ +} + +class CombinedArgumentService implements FirstArgumentService, SecondArgumentService +{ +} + +class NullableServiceActivity extends Activity +{ + public function handle(FirstArgumentService $first, ?SecondArgumentService $second = null): ?SecondArgumentService + { + return $second; + } +}