Merge pull request #646: Control Abandoned Child Workflow Cancellation

Author: roxblnfk

src/Internal/Workflow/ChildWorkflowStub.php

@@ -21,10 +21,12 @@
 use Temporal\Internal\Transport\Request\ExecuteChildWorkflow;
 use Temporal\Internal\Transport\Request\GetChildWorkflowExecution;
 use Temporal\Internal\Transport\Request\SignalExternalWorkflow;
+use Temporal\Worker\FeatureFlags;
 use Temporal\Worker\Transport\Command\RequestInterface;
 use Temporal\Workflow;
 use Temporal\Workflow\ChildWorkflowOptions;
 use Temporal\Workflow\ChildWorkflowStubInterface;
+use Temporal\Workflow\ParentClosePolicy;
 use Temporal\Workflow\WorkflowExecution;
 
 final class ChildWorkflowStub implements ChildWorkflowStubInterface
@@ -70,7 +72,10 @@ public function start(... $args): PromiseInterface
             $this->header,
         );
 
-        $this->result = $this->request($this->request);
+        $cancellable = FeatureFlags::$cancelAbandonedChildWorkflows
+            || $this->options->parentClosePolicy !== ParentClosePolicy::Abandon->value;
+
+        $this->result = $this->request($this->request, cancellable: $cancellable);
 
         $started = $this->request(new GetChildWorkflowExecution($this->request))
             ->then(
@@ -118,12 +123,12 @@ function (WorkflowExecution $execution) use ($name, $args) {
         );
     }
 
-    protected function request(RequestInterface $request): PromiseInterface
+    protected function request(RequestInterface $request, bool $cancellable = true): PromiseInterface
     {
         /** @var Workflow\WorkflowContextInterface $context */
         $context = Workflow::getCurrentContext();
 
-        return $context->request($request);
+        return $context->request($request, cancellable: $cancellable);
     }
 
     private function getOptionArray(): array

src/Internal/Workflow/Process/Scope.php

@@ -355,9 +355,9 @@ protected function callSignalOrUpdateHandler(callable $handler, ValuesInterface
         }, $values)->catch($this->onException(...));
     }
 
-    protected function onRequest(RequestInterface $request, PromiseInterface $promise): void
+    protected function onRequest(RequestInterface $request, PromiseInterface $promise, bool $cancellable = true): void
     {
-        $this->onCancel[++$this->cancelID] = function (?\Throwable $reason = null) use ($request): void {
+        $this->onCancel[++$this->cancelID] = function (?\Throwable $reason = null) use ($request, $cancellable): void {
             $client = $this->context->getClient();
             if ($reason instanceof DestructMemorizedInstanceException) {
                 // memory flush
@@ -370,6 +370,11 @@ protected function onRequest(RequestInterface $request, PromiseInterface $promis
                 return;
             }
 
+            if (!$cancellable) {
+                // non-cancellable request
+                return;
+            }
+
             $client->request(new Cancel($request->getID()), $this->scopeContext);
         };
 

src/Internal/Workflow/ScopeContext.php

@@ -23,13 +23,17 @@
 
 class ScopeContext extends WorkflowContext implements ScopedContextInterface
 {
+    /** @var \Closure(RequestInterface, PromiseInterface, bool $cancellable): void */
+    private \Closure $onRequest;
+
     private WorkflowContext $parent;
     private Scope $scope;
-    private \Closure $onRequest;
     private ?UpdateContext $updateContext = null;
 
     /**
      * Creates scope specific context.
+     *
+     * @param \Closure(RequestInterface, PromiseInterface, bool $cancellable): void $onRequest
      */
     public static function fromWorkflowContext(
         WorkflowContext $context,
@@ -78,11 +82,11 @@ public function request(
         );
 
         if (!$waitResponse) {
-            return $this->parent->request($request, $cancellable, false);
+            return $this->parent->request($request, cancellable: $cancellable, waitResponse: false);
         }
 
-        $promise = $this->parent->request($request);
-        ($this->onRequest)($request, $promise);
+        $promise = $this->parent->request($request, cancellable: $cancellable);
+        ($this->onRequest)($request, $promise, $cancellable);
 
         return new CompletableResult(
             $this,

src/Worker/FeatureFlags.php

@@ -31,4 +31,21 @@ final class FeatureFlags
      * @since SDK 2.11.0
      */
     public static bool $warnOnWorkflowUnfinishedHandlers = true;
+
+    /**
+     * When a parent workflow is canceled, it will also cancel all its Child Workflows, including abandoned ones.
+     * This behavior is not correct and will be improved by default in the next major SDK version.
+     *
+     * To fix the behavior now, set this flag to TRUE. In this case, be aware of the following:
+     * - If you start an abandoned Child Workflow in the main Workflow scope, it may miss
+     *   the cancellation signal if you await only on the Child Workflow.
+     * - If you start an abandoned Child Workflow in an async scope {@see Workflow::async()},
+     *   that is later canceled, the Child Workflow will not be affected.
+     * - You still can cancel abandoned Child Workflows manually by calling {@see WorkflowStubInterface::cancel()}.
+     *
+     * @see Workflow\ParentClosePolicy::Abandon
+     *
+     * @since SDK 2.16.0
+     */
+    public static bool $cancelAbandonedChildWorkflows = true;
 }

src/Workflow/WorkflowContextInterface.php

@@ -126,6 +126,8 @@ public function registerUpdate(string $name, callable $handler, ?callable $valid
     /**
      * Exchanges data between worker and host process.
      *
+     * @param bool $waitResponse Determine if the Request requires a Response from RoadRunner.
+     *
      * @internal This is an internal method
      */
     public function request(

tests/Acceptance/App/RuntimeBuilder.php

@@ -71,6 +71,7 @@ public static function init(): void
         \ini_set('display_errors', 'stderr');
         // Feature flags
         FeatureFlags::$workflowDeferredHandlerStart = true;
+        FeatureFlags::$cancelAbandonedChildWorkflows = false;
     }
 
     /**

tests/Acceptance/Harness/ChildWorkflow/CancelAbandonTest.php

@@ -9,22 +9,102 @@
 use Temporal\Client\WorkflowStubInterface;
 use Temporal\Exception\Failure\CanceledFailure;
 use Temporal\Exception\Failure\ChildWorkflowFailure;
+use Temporal\Promise;
 use Temporal\Tests\Acceptance\App\Attribute\Stub;
 use Temporal\Tests\Acceptance\App\TestCase;
 use Temporal\Workflow;
+use Temporal\Workflow\CancellationScopeInterface;
 use Temporal\Workflow\WorkflowInterface;
 use Temporal\Workflow\WorkflowMethod;
 
 class CancelAbandonTest extends TestCase
 {
+    /**
+     * If an abandoned Child Workflow is started in the main Workflow scope,
+     * the Child Workflow should not be affected by the cancellation of the parent workflow.
+     * But need to consider that we can miss the Cancellation signal if awaiting only on the Child Workflow.
+     * In the {@see MainScopeWorkflow} we use Timer + Child Workflow to ensure we catch the Cancellation signal.
+     */
     #[Test]
-    public static function check(
-        #[Stub('Harness_ChildWorkflow_CancelAbandon')]
+    public static function childWorkflowInMainScope(
+        #[Stub('Harness_ChildWorkflow_CancelAbandon_MainScope', args: ['test 42'])]
         WorkflowStubInterface $stub,
         WorkflowClientInterface $client,
     ): void {
-        self::markTestSkipped('To be resolved with https://github.com/temporalio/sdk-php/issues/634');
+        self::runTestScenario($stub, $client, 'test 42');
+    }
+
+    /**
+     * If an abandoned Child Workflow is started in an async Scope {@see Workflow::async()} that is later cancelled,
+     * the Child Workflow should not be affected by the cancellation of the parent workflow.
+     * Int his case the Scope will throw the CanceledFailure.
+     * @see InnerScopeCancelWorkflow
+     */
+    #[Test]
+    public static function childWorkflowInInnerScopeCancel(
+        #[Stub('Harness_ChildWorkflow_CancelAbandon_InnerScopeCancel', args: ['baz'])]
+        WorkflowStubInterface $stub,
+        WorkflowClientInterface $client,
+    ): void {
+        self::runTestScenario($stub, $client, 'baz');
+    }
+
+    /**
+     * If an abandoned Child Workflow is started in an async scope {@see Workflow::async()} that
+     * is later cancelled manually by a Signal to the parent workflow {@see InnerScopeCancelWorkflow::close()},
+     * the Child Workflow should not be affected by the cancellation of the parent scope.
+     */
+    #[Test]
+    public static function childWorkflowInClosingInnerScope(
+        #[Stub('Harness_ChildWorkflow_CancelAbandon_InnerScopeCancel', args: ['foo bar'])]
+        WorkflowStubInterface $stub,
+        WorkflowClientInterface $client,
+    ): void {
+        # Get Child Workflow Stub
+        $child = self::getChildWorkflowStub($client, $stub);
+
+        # Cancel the async scope
+        /** @see InnerScopeCancelWorkflow::close() */
+        $stub->signal('close');
+        # Expect the CanceledFailure in the parent workflow
+        self::assertSame('cancelled', $stub->getResult(timeout: 5));
+
+        # Signal the child workflow to exit
+        $child->signal('exit');
+        # No canceled failure in the child workflow
+        self::assertSame('foo bar', $child->getResult());
+    }
+
+    /**
+     * Send cancel to the parent workflow and expect the child workflow to be abandoned
+     * and not cancelled.
+     */
+    private static function runTestScenario(
+        WorkflowStubInterface $stub,
+        WorkflowClientInterface $client,
+        string $result,
+    ): void {
+        # Get Child Workflow Stub
+        $child = self::getChildWorkflowStub($client, $stub);
+
+        # Cancel the parent workflow
+        $stub->cancel();
+        # Expect the CanceledFailure in the parent workflow
+        self::assertSame('cancelled', $stub->getResult(timeout: 5));
+
+        # Signal the child workflow to exit
+        $child->signal('exit');
+        # No canceled failure in the child workflow
+        self::assertSame($result, $child->getResult());
+    }
 
+    /**
+     * Get Child Workflow Stub
+     */
+    private static function getChildWorkflowStub(
+        WorkflowClientInterface $client,
+        WorkflowStubInterface $stub,
+    ): WorkflowStubInterface {
         # Find the child workflow execution ID
         $deadline = \microtime(true) + 10;
         child_id:
@@ -40,52 +120,99 @@ public static function check(
             goto child_id;
         }
 
-        self::assertNotNull($execution, 'Child workflow execution not found in history');
+        self::assertNotNull($execution, 'Child Workflow execution not found in the history.');
 
         # Get Child Workflow Stub
-        $child = $client->newUntypedRunningWorkflowStub(
+        return $client->newUntypedRunningWorkflowStub(
             $execution->getWorkflowId(),
             $execution->getRunId(),
             'Harness_ChildWorkflow_CancelAbandon_Child',
         );
-
-        # Cancel the parent workflow
-        $stub->cancel();
-        # Expect the CanceledFailure in the parent workflow
-        self::assertSame('cancelled', $stub->getResult());
-
-        # Signal the child workflow to exit
-        $child->signal('exit');
-        # No canceled failure in the child workflow
-        self::assertSame('test 42', $child->getResult());
     }
 }
 
 #[WorkflowInterface]
-class MainWorkflow
+class MainScopeWorkflow
 {
-    #[WorkflowMethod('Harness_ChildWorkflow_CancelAbandon')]
-    public function run()
+    #[WorkflowMethod('Harness_ChildWorkflow_CancelAbandon_MainScope')]
+    public function run(string $input)
     {
-        $child = Workflow::newUntypedChildWorkflowStub(
+        /** @see ChildWorkflow */
+        $stub = Workflow::newUntypedChildWorkflowStub(
             'Harness_ChildWorkflow_CancelAbandon_Child',
             Workflow\ChildWorkflowOptions::new()
+                ->withWorkflowRunTimeout('20 seconds')
                 ->withParentClosePolicy(Workflow\ParentClosePolicy::Abandon),
         );
 
-        yield $child->start('test 42');
+        yield $stub->start($input);
+
+        try {
+            yield Promise::race([$stub->getResult(), Workflow::timer(5)]);
+            return 'timer';
+        } catch (CanceledFailure) {
+            return 'cancelled';
+        } catch (ChildWorkflowFailure $failure) {
+            # Check CanceledFailure
+            return $failure->getPrevious()::class === CanceledFailure::class
+                ? 'cancelled'
+                : throw $failure;
+        } finally {
+            yield Workflow::asyncDetached(function () {
+                # We shouldn't complete the Workflow immediately:
+                # all the commands from the tick must be sent for testing purposes.
+                yield Workflow::timer(1);
+            });
+        }
+    }
+}
+
+#[WorkflowInterface]
+class InnerScopeCancelWorkflow
+{
+    private CancellationScopeInterface $scope;
+
+    #[WorkflowMethod('Harness_ChildWorkflow_CancelAbandon_InnerScopeCancel')]
+    public function run(string $input)
+    {
+        $this->scope = Workflow::async(static function () use ($input) {
+            /** @see ChildWorkflow */
+            $stub = Workflow::newUntypedChildWorkflowStub(
+                'Harness_ChildWorkflow_CancelAbandon_Child',
+                Workflow\ChildWorkflowOptions::new()
+                    ->withWorkflowRunTimeout('20 seconds')
+                    ->withParentClosePolicy(Workflow\ParentClosePolicy::Abandon),
+            );
+            yield $stub->start($input);
+
+            return yield $stub->getResult('string');
+        });
+
 
         try {
-            return yield $child->getResult();
+            yield Promise::race([Workflow::timer(5) ,$this->scope]);
+            return 'timer';
         } catch (CanceledFailure) {
             return 'cancelled';
         } catch (ChildWorkflowFailure $failure) {
             # Check CanceledFailure
             return $failure->getPrevious()::class === CanceledFailure::class
                 ? 'cancelled'
                 : throw $failure;
+        } finally {
+            yield Workflow::asyncDetached(function () {
+                # We shouldn't complete the Workflow immediately:
+                # all the commands from the tick must be sent for testing purposes.
+                yield Workflow::timer(1);
+            });
         }
     }
+
+    #[Workflow\SignalMethod('close')]
+    public function close(): void
+    {
+        $this->scope->cancel();
+    }
 }
 
 #[WorkflowInterface]