Skip to content

Commit 4568aa5

Browse files
durable-workflow-opsdurable-workflow-ops
committed
Reject ambiguous workflow command retry and timeout scopes
Reject ambiguous retry timeout command scopes
1 parent a70ceea commit 4568aa5

3 files changed

Lines changed: 234 additions & 0 deletions

File tree

‎README.md‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -476,6 +476,18 @@ curl "$SERVER/api/workflows/order-42/runs/abc123/history" \
476476
| `record_version_marker` | No | Record a version marker |
477477
| `upsert_search_attributes` | No | Update search attributes |
478478

479+
Retry and timeout fields are scoped to the command layer they control.
480+
`schedule_activity` accepts activity `retry_policy`,
481+
`start_to_close_timeout`, `schedule_to_start_timeout`,
482+
`schedule_to_close_timeout`, and `heartbeat_timeout`; heartbeat, start, and
483+
schedule-to-start budgets cannot exceed schedule-to-close or start-to-close
484+
where those outer budgets are present. `start_child_workflow` accepts child
485+
workflow `retry_policy`, `execution_timeout_seconds`, and
486+
`run_timeout_seconds`; the run timeout cannot exceed the execution timeout.
487+
`non_retryable` is only a failure outcome flag on `fail_workflow` and
488+
`fail_update`. HTTP transport retry policy is configured by clients outside the
489+
workflow-task command payload.
490+
479491
## API Overview
480492

481493
### System

‎app/Http/Controllers/Api/WorkerController.php‎

Lines changed: 119 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313
use Illuminate\Http\JsonResponse;
1414
use Illuminate\Http\Request;
1515
use Illuminate\Support\Str;
16+
use Illuminate\Validation\ValidationException;
1617
use Workflow\V2\Contracts\WorkflowTaskBridge;
1718
use Workflow\V2\Exceptions\StructuralLimitExceededException;
1819
use Workflow\V2\Models\WorkflowTask;
@@ -487,6 +488,8 @@ public function completeWorkflowTask(Request $request, string $taskId): JsonResp
487488
'commands.*.timeout_seconds' => ['nullable', 'integer', 'min:0'],
488489
]);
489490

491+
$this->validateWorkflowTaskCommandScopes($validated['commands']);
492+
490493
if ($response = $this->guardWorkflowTaskOwnership(
491494
$request,
492495
$namespace,
@@ -529,6 +532,122 @@ public function completeWorkflowTask(Request $request, string $taskId): JsonResp
529532
], $this->workflowOutcomeStatus($outcome['reason']));
530533
}
531534

535+
/**
536+
* @param list<array<string, mixed>> $commands
537+
*
538+
* @throws ValidationException
539+
*/
540+
private function validateWorkflowTaskCommandScopes(array $commands): void
541+
{
542+
$errors = [];
543+
544+
foreach ($commands as $index => $command) {
545+
$type = $command['type'] ?? null;
546+
547+
if (! is_string($type)) {
548+
continue;
549+
}
550+
551+
if ($this->hasCommandValue($command, 'retry_policy')
552+
&& ! in_array($type, ['schedule_activity', 'start_child_workflow'], true)
553+
) {
554+
$errors["commands.{$index}.retry_policy"][] =
555+
'retry_policy is only supported for schedule_activity and start_child_workflow commands.';
556+
}
557+
558+
foreach (['start_to_close_timeout', 'schedule_to_start_timeout', 'schedule_to_close_timeout', 'heartbeat_timeout'] as $field) {
559+
if ($this->hasCommandValue($command, $field) && $type !== 'schedule_activity') {
560+
$errors["commands.{$index}.{$field}"][] =
561+
"{$field} is only supported for schedule_activity commands.";
562+
}
563+
}
564+
565+
foreach (['execution_timeout_seconds', 'run_timeout_seconds'] as $field) {
566+
if ($this->hasCommandValue($command, $field) && $type !== 'start_child_workflow') {
567+
$errors["commands.{$index}.{$field}"][] =
568+
"{$field} is only supported for start_child_workflow commands.";
569+
}
570+
}
571+
572+
if ($this->hasCommandValue($command, 'non_retryable')
573+
&& ! in_array($type, ['fail_workflow', 'fail_update'], true)
574+
) {
575+
$errors["commands.{$index}.non_retryable"][] =
576+
'non_retryable is only supported for fail_workflow and fail_update commands.';
577+
}
578+
579+
if ($type === 'schedule_activity') {
580+
$this->validateActivityTimeoutEnvelope($command, $index, $errors);
581+
}
582+
583+
if ($type === 'start_child_workflow') {
584+
$this->validateChildWorkflowTimeoutEnvelope($command, $index, $errors);
585+
}
586+
}
587+
588+
if ($errors !== []) {
589+
throw ValidationException::withMessages($errors);
590+
}
591+
}
592+
593+
/**
594+
* @param array<string, mixed> $command
595+
*/
596+
private function hasCommandValue(array $command, string $field): bool
597+
{
598+
return array_key_exists($field, $command) && $command[$field] !== null;
599+
}
600+
601+
/**
602+
* @param array<string, mixed> $command
603+
* @param array<string, list<string>> $errors
604+
*/
605+
private function validateActivityTimeoutEnvelope(array $command, int $index, array &$errors): void
606+
{
607+
$startToClose = $this->optionalCommandInt($command, 'start_to_close_timeout');
608+
$scheduleToStart = $this->optionalCommandInt($command, 'schedule_to_start_timeout');
609+
$scheduleToClose = $this->optionalCommandInt($command, 'schedule_to_close_timeout');
610+
$heartbeat = $this->optionalCommandInt($command, 'heartbeat_timeout');
611+
612+
if ($heartbeat !== null && $startToClose !== null && $heartbeat > $startToClose) {
613+
$errors["commands.{$index}.heartbeat_timeout"][] =
614+
'heartbeat_timeout cannot exceed start_to_close_timeout.';
615+
}
616+
617+
if ($startToClose !== null && $scheduleToClose !== null && $startToClose > $scheduleToClose) {
618+
$errors["commands.{$index}.start_to_close_timeout"][] =
619+
'start_to_close_timeout cannot exceed schedule_to_close_timeout.';
620+
}
621+
622+
if ($scheduleToStart !== null && $scheduleToClose !== null && $scheduleToStart > $scheduleToClose) {
623+
$errors["commands.{$index}.schedule_to_start_timeout"][] =
624+
'schedule_to_start_timeout cannot exceed schedule_to_close_timeout.';
625+
}
626+
}
627+
628+
/**
629+
* @param array<string, mixed> $command
630+
* @param array<string, list<string>> $errors
631+
*/
632+
private function validateChildWorkflowTimeoutEnvelope(array $command, int $index, array &$errors): void
633+
{
634+
$executionTimeout = $this->optionalCommandInt($command, 'execution_timeout_seconds');
635+
$runTimeout = $this->optionalCommandInt($command, 'run_timeout_seconds');
636+
637+
if ($executionTimeout !== null && $runTimeout !== null && $runTimeout > $executionTimeout) {
638+
$errors["commands.{$index}.run_timeout_seconds"][] =
639+
'run_timeout_seconds cannot exceed execution_timeout_seconds.';
640+
}
641+
}
642+
643+
/**
644+
* @param array<string, mixed> $command
645+
*/
646+
private function optionalCommandInt(array $command, string $field): ?int
647+
{
648+
return is_int($command[$field] ?? null) ? $command[$field] : null;
649+
}
650+
532651
/**
533652
* Heartbeat a claimed workflow task to extend its lease.
534653
*/

‎tests/Feature/WorkerProtocolContractTest.php‎

Lines changed: 103 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -104,6 +104,109 @@ public function test_workflow_task_command_validation_errors_use_worker_protocol
104104
);
105105
}
106106

107+
/**
108+
* @param array<string, mixed> $command
109+
*/
110+
#[DataProvider('invalidWorkflowTaskCommandScopeProvider')]
111+
public function test_workflow_task_command_rejects_retry_and_timeout_fields_outside_their_scope(
112+
array $command,
113+
string $errorField,
114+
string $expectedMessage,
115+
): void {
116+
$response = $this->withHeaders($this->workerHeaders() + [
117+
ControlPlaneProtocol::HEADER => ControlPlaneProtocol::VERSION,
118+
])->postJson('/api/worker/workflow-tasks/missing-task/complete', [
119+
'lease_owner' => 'command-scope-worker',
120+
'workflow_task_attempt' => 1,
121+
'commands' => [$command],
122+
]);
123+
124+
$response->assertStatus(422)
125+
->assertHeader(WorkerProtocol::HEADER, WorkerProtocol::VERSION)
126+
->assertHeaderMissing(ControlPlaneProtocol::HEADER)
127+
->assertJsonPath('protocol_version', WorkerProtocol::VERSION)
128+
->assertJsonPath('reason', 'validation_failed')
129+
->assertJsonMissingPath('control_plane');
130+
131+
$this->assertSame(
132+
$expectedMessage,
133+
$response->json('validation_errors')[$errorField][0] ?? null,
134+
);
135+
}
136+
137+
/**
138+
* @return array<string, array{command: array<string, mixed>, errorField: string, expectedMessage: string}>
139+
*/
140+
public static function invalidWorkflowTaskCommandScopeProvider(): array
141+
{
142+
return [
143+
'retry policy on completion' => [
144+
'command' => [
145+
'type' => 'complete_workflow',
146+
'retry_policy' => ['max_attempts' => 2],
147+
],
148+
'errorField' => 'commands.0.retry_policy',
149+
'expectedMessage' => 'retry_policy is only supported for schedule_activity and start_child_workflow commands.',
150+
],
151+
'activity timeout on child command' => [
152+
'command' => [
153+
'type' => 'start_child_workflow',
154+
'workflow_type' => 'tests.external-child-workflow',
155+
'start_to_close_timeout' => 30,
156+
],
157+
'errorField' => 'commands.0.start_to_close_timeout',
158+
'expectedMessage' => 'start_to_close_timeout is only supported for schedule_activity commands.',
159+
],
160+
'child timeout on activity command' => [
161+
'command' => [
162+
'type' => 'schedule_activity',
163+
'activity_type' => 'tests.external-activity',
164+
'run_timeout_seconds' => 30,
165+
],
166+
'errorField' => 'commands.0.run_timeout_seconds',
167+
'expectedMessage' => 'run_timeout_seconds is only supported for start_child_workflow commands.',
168+
],
169+
'non retryable on completion' => [
170+
'command' => [
171+
'type' => 'complete_workflow',
172+
'non_retryable' => true,
173+
],
174+
'errorField' => 'commands.0.non_retryable',
175+
'expectedMessage' => 'non_retryable is only supported for fail_workflow and fail_update commands.',
176+
],
177+
'heartbeat exceeds start to close' => [
178+
'command' => [
179+
'type' => 'schedule_activity',
180+
'activity_type' => 'tests.external-activity',
181+
'start_to_close_timeout' => 10,
182+
'heartbeat_timeout' => 30,
183+
],
184+
'errorField' => 'commands.0.heartbeat_timeout',
185+
'expectedMessage' => 'heartbeat_timeout cannot exceed start_to_close_timeout.',
186+
],
187+
'schedule to start exceeds schedule to close' => [
188+
'command' => [
189+
'type' => 'schedule_activity',
190+
'activity_type' => 'tests.external-activity',
191+
'schedule_to_start_timeout' => 60,
192+
'schedule_to_close_timeout' => 30,
193+
],
194+
'errorField' => 'commands.0.schedule_to_start_timeout',
195+
'expectedMessage' => 'schedule_to_start_timeout cannot exceed schedule_to_close_timeout.',
196+
],
197+
'child run exceeds execution' => [
198+
'command' => [
199+
'type' => 'start_child_workflow',
200+
'workflow_type' => 'tests.external-child-workflow',
201+
'execution_timeout_seconds' => 60,
202+
'run_timeout_seconds' => 120,
203+
],
204+
'errorField' => 'commands.0.run_timeout_seconds',
205+
'expectedMessage' => 'run_timeout_seconds cannot exceed execution_timeout_seconds.',
206+
],
207+
];
208+
}
209+
107210
/**
108211
* @return array<string, array{path: string, body: array<string, mixed>, errorFields: list<string>}>
109212
*/

0 commit comments

Comments
 (0)