[Console] Fix signal handlers not being cleared after command termination

yoeunes · nicolas-grekas · commit 32d9216a8570 · 2025-11-13T09:19:35.000+01:00
diff --git a/Application.php b/Application.php
@@ -1012,12 +1012,16 @@ protected function doRunCommand(Command $command, InputInterface $input, OutputI
             }
         }
 
+        $registeredSignals = false;
         $commandSignals = $command instanceof SignalableCommandInterface ? $command->getSubscribedSignals() : [];
         if ($commandSignals || $this->dispatcher && $this->signalsToDispatchEvent) {
             if (!$this->signalRegistry) {
                 throw new RuntimeException('Unable to subscribe to signal events. Make sure that the "pcntl" extension is installed and that "pcntl_*" functions are not disabled by your php.ini\'s "disable_functions" directive.');
             }
 
+            $registeredSignals = true;
+            $this->getSignalRegistry()->pushCurrentHandlers();
+
             if ($this->dispatcher) {
                 // We register application signals, so that we can dispatch the event
                 foreach ($this->signalsToDispatchEvent as $signal) {
@@ -1067,7 +1071,13 @@ protected function doRunCommand(Command $command, InputInterface $input, OutputI
         }
 
         if (null === $this->dispatcher) {
-            return $command->run($input, $output);
+            try {
+                return $command->run($input, $output);
+            } finally {
+                if ($registeredSignals) {
+                    $this->getSignalRegistry()->popPreviousHandlers();
+                }
+            }
         }
 
         // bind before the console.command event, so the listeners have access to input options/arguments
@@ -1097,6 +1107,10 @@ protected function doRunCommand(Command $command, InputInterface $input, OutputI
             if (0 === $exitCode = $event->getExitCode()) {
                 $e = null;
             }
+        } finally {
+            if ($registeredSignals) {
+                $this->getSignalRegistry()->popPreviousHandlers();
+            }
         }
 
         $event = new ConsoleTerminateEvent($command, $input, $output, $exitCode);
diff --git a/SignalRegistry/SignalRegistry.php b/SignalRegistry/SignalRegistry.php
@@ -13,8 +13,21 @@
 
 final class SignalRegistry
 {
+    /**
+     * @var array<int, array<callable>>
+     */
     private array $signalHandlers = [];
 
+    /**
+     * @var array<array<int, array<callable>>>
+     */
+    private array $stack = [];
+
+    /**
+     * @var array<int, callable|int|string>
+     */
+    private array $originalHandlers = [];
+
     public function __construct()
     {
         if (\function_exists('pcntl_async_signals')) {
@@ -24,17 +37,21 @@ public function __construct()
 
     public function register(int $signal, callable $signalHandler): void
     {
-        if (!isset($this->signalHandlers[$signal])) {
-            $previousCallback = pcntl_signal_get_handler($signal);
+        $previous = pcntl_signal_get_handler($signal);
+
+        if (!isset($this->originalHandlers[$signal])) {
+            $this->originalHandlers[$signal] = $previous;
+        }
 
-            if (\is_callable($previousCallback)) {
-                $this->signalHandlers[$signal][] = $previousCallback;
+        if (!isset($this->signalHandlers[$signal])) {
+            if (\is_callable($previous) && [$this, 'handle'] !== $previous) {
+                $this->signalHandlers[$signal][] = $previous;
             }
         }
 
         $this->signalHandlers[$signal][] = $signalHandler;
 
-        pcntl_signal($signal, $this->handle(...));
+        pcntl_signal($signal, [$this, 'handle']);
     }
 
     public static function isSupported(): bool
@@ -54,4 +71,38 @@ public function handle(int $signal): void
             $signalHandler($signal, $hasNext);
         }
     }
+
+    /**
+     * Pushes the current active handlers onto the stack and clears the active list.
+     *
+     * This prepares the registry for a new set of handlers within a specific scope.
+     *
+     * @internal
+     */
+    public function pushCurrentHandlers(): void
+    {
+        $this->stack[] = $this->signalHandlers;
+        $this->signalHandlers = [];
+    }
+
+    /**
+     * Restores the previous handlers from the stack, making them active.
+     *
+     * This also restores the original OS-level signal handler if no
+     * more handlers are registered for a signal that was just popped.
+     *
+     * @internal
+     */
+    public function popPreviousHandlers(): void
+    {
+        $popped = $this->signalHandlers;
+        $this->signalHandlers = array_pop($this->stack) ?? [];
+
+        // Restore OS handler if no more Symfony handlers for this signal
+        foreach ($popped as $signal => $handlers) {
+            if (!($this->signalHandlers[$signal] ?? false) && isset($this->originalHandlers[$signal])) {
+                pcntl_signal($signal, $this->originalHandlers[$signal]);
+            }
+        }
+    }
 }
diff --git a/Tests/ApplicationTest.php b/Tests/ApplicationTest.php
@@ -824,7 +824,7 @@ public function testSetCatchErrors(bool $catchExceptions)
 
         try {
             $tester->run(['command' => 'boom']);
-            $this->fail('The exception is not catched.');
+            $this->fail('The exception is not caught.');
         } catch (\Throwable $e) {
             $this->assertInstanceOf(\Error::class, $e);
             $this->assertSame('This is an error.', $e->getMessage());
@@ -2259,6 +2259,181 @@ private function runRestoresSttyTest(array $params, int $expectedExitCode, bool
         }
     }
 
+    /**
+     * @requires extension pcntl
+     */
+    public function testSignalHandlersAreCleanedUpAfterCommandRuns()
+    {
+        $application = new Application();
+        $application->setAutoExit(false);
+        $application->setCatchExceptions(false);
+        $application->add(new SignableCommand(false));
+
+        $signalRegistry = $application->getSignalRegistry();
+        $tester = new ApplicationTester($application);
+
+        $this->assertCount(0, $this->getHandlersForSignal($signalRegistry, \SIGUSR1), 'Registry should be empty initially.');
+
+        $tester->run(['command' => 'signal']);
+        $this->assertCount(0, $this->getHandlersForSignal($signalRegistry, \SIGUSR1), 'Registry should be empty after first run.');
+
+        $tester->run(['command' => 'signal']);
+        $this->assertCount(0, $this->getHandlersForSignal($signalRegistry, \SIGUSR1), 'Registry should still be empty after second run.');
+    }
+
+    /**
+     * @requires extension pcntl
+     */
+    public function testSignalHandlersCleanupOnException()
+    {
+        $command = new class('signal:exception') extends Command implements SignalableCommandInterface {
+            public function getSubscribedSignals(): array
+            {
+                return [\SIGUSR1];
+            }
+
+            public function handleSignal(int $signal, int|false $previousExitCode = 0): int|false
+            {
+                return false;
+            }
+
+            protected function execute(InputInterface $input, OutputInterface $output): int
+            {
+                throw new \RuntimeException('Test exception');
+            }
+        };
+
+        $application = new Application();
+        $application->setAutoExit(false);
+        $application->setCatchExceptions(true);
+        $application->add($command);
+
+        $signalRegistry = $application->getSignalRegistry();
+        $tester = new ApplicationTester($application);
+
+        $this->assertCount(0, $this->getHandlersForSignal($signalRegistry, \SIGUSR1), 'Pre-condition: Registry must be empty.');
+
+        $tester->run(['command' => 'signal:exception']);
+        $this->assertCount(0, $this->getHandlersForSignal($signalRegistry, \SIGUSR1), 'Signal handlers must be cleaned up even on exception.');
+    }
+
+    /**
+     * @requires extension pcntl
+     */
+    public function testNestedCommandsIsolateSignalHandlers()
+    {
+        $application = new Application();
+        $application->setAutoExit(false);
+        $application->setCatchExceptions(false);
+
+        $signalRegistry = $application->getSignalRegistry();
+        $self = $this;
+
+        $innerCommand = new class('signal:inner') extends Command implements SignalableCommandInterface {
+            public $signalRegistry;
+            public $self;
+
+            public function getSubscribedSignals(): array
+            {
+                return [\SIGUSR1];
+            }
+
+            public function handleSignal(int $signal, int|false $previousExitCode = 0): int|false
+            {
+                return false;
+            }
+
+            protected function execute(InputInterface $input, OutputInterface $output): int
+            {
+                $handlers = $this->self->getHandlersForSignal($this->signalRegistry, \SIGUSR1);
+                $this->self->assertCount(1, $handlers, 'Inner command should only see its own handler.');
+                $output->write('Inner execute.');
+
+                return 0;
+            }
+        };
+
+        $outerCommand = new class('signal:outer') extends Command implements SignalableCommandInterface {
+            public $signalRegistry;
+            public $self;
+
+            public function getSubscribedSignals(): array
+            {
+                return [\SIGUSR1];
+            }
+
+            public function handleSignal(int $signal, int|false $previousExitCode = 0): int|false
+            {
+                return false;
+            }
+
+            protected function execute(InputInterface $input, OutputInterface $output): int
+            {
+                $handlersBefore = $this->self->getHandlersForSignal($this->signalRegistry, \SIGUSR1);
+                $this->self->assertCount(1, $handlersBefore, 'Outer command must have its handler registered.');
+
+                $output->write('Outer pre-run.');
+
+                $this->getApplication()->find('signal:inner')->run(new ArrayInput([]), $output);
+
+                $output->write('Outer post-run.');
+
+                $handlersAfter = $this->self->getHandlersForSignal($this->signalRegistry, \SIGUSR1);
+                $this->self->assertCount(1, $handlersAfter, 'Outer command\'s handler must be restored.');
+                $this->self->assertSame($handlersBefore, $handlersAfter, 'Handler stack must be identical after pop.');
+
+                return 0;
+            }
+        };
+
+        $innerCommand->self = $self;
+        $innerCommand->signalRegistry = $signalRegistry;
+        $outerCommand->self = $self;
+        $outerCommand->signalRegistry = $signalRegistry;
+
+        $application->add($innerCommand);
+        $application->add($outerCommand);
+
+        $tester = new ApplicationTester($application);
+
+        $this->assertCount(0, $this->getHandlersForSignal($signalRegistry, \SIGUSR1), 'Pre-condition: Registry must be empty.');
+        $tester->run(['command' => 'signal:outer']);
+        $this->assertStringContainsString('Outer pre-run.Inner execute.Outer post-run.', $tester->getDisplay());
+
+        $this->assertCount(0, $this->getHandlersForSignal($signalRegistry, \SIGUSR1), 'Registry must be empty after all commands are finished.');
+    }
+
+    /**
+     * @requires extension pcntl
+     */
+    public function testOriginalHandlerRestoredAfterPop()
+    {
+        $this->assertSame(\SIG_DFL, pcntl_signal_get_handler(\SIGUSR1), 'Pre-condition: Original handler for SIGUSR1 must be SIG_DFL.');
+
+        $application = new Application();
+        $application->setAutoExit(false);
+        $application->setCatchExceptions(false);
+        $application->add(new SignableCommand(false));
+
+        $tester = new ApplicationTester($application);
+        $tester->run(['command' => 'signal']);
+
+        $this->assertSame(\SIG_DFL, pcntl_signal_get_handler(\SIGUSR1), 'OS-level handler for SIGUSR1 must be restored to SIG_DFL.');
+
+        $tester->run(['command' => 'signal']);
+        $this->assertSame(\SIG_DFL, pcntl_signal_get_handler(\SIGUSR1), 'OS-level handler must remain SIG_DFL after a second run.');
+    }
+
+    /**
+     * Reads the private "signalHandlers" property of the SignalRegistry for assertions.
+     */
+    public function getHandlersForSignal(SignalRegistry $registry, int $signal): array
+    {
+        $handlers = (\Closure::bind(fn () => $this->signalHandlers, $registry, SignalRegistry::class))();
+
+        return $handlers[$signal] ?? [];
+    }
+
     private function createSignalableApplication(Command $command, ?EventDispatcherInterface $dispatcher): Application
     {
         $application = new Application();
diff --git a/Tests/SignalRegistry/SignalRegistryTest.php b/Tests/SignalRegistry/SignalRegistryTest.php
@@ -129,4 +129,67 @@ public function testTwoCallbacksForASignalPreviousCallbackFromAnotherRegistry()
         $this->assertTrue($isHandled1);
         $this->assertTrue($isHandled2);
     }
+
+    public function testPushPopIsolatesHandlers()
+    {
+        $registry = new SignalRegistry();
+
+        $signal = \SIGUSR1;
+
+        $handler1 = static function () {};
+        $handler2 = static function () {};
+
+        $registry->pushCurrentHandlers();
+        $registry->register($signal, $handler1);
+
+        $this->assertCount(1, $this->getHandlersForSignal($registry, $signal));
+
+        $registry->pushCurrentHandlers();
+        $registry->register($signal, $handler2);
+
+        $this->assertCount(1, $this->getHandlersForSignal($registry, $signal));
+        $this->assertSame([$handler2], $this->getHandlersForSignal($registry, $signal));
+
+        $registry->popPreviousHandlers();
+
+        $this->assertCount(1, $this->getHandlersForSignal($registry, $signal));
+        $this->assertSame([$handler1], $this->getHandlersForSignal($registry, $signal));
+
+        $registry->popPreviousHandlers();
+
+        $this->assertCount(0, $this->getHandlersForSignal($registry, $signal));
+    }
+
+    public function testRestoreOriginalOnEmptyAfterPop()
+    {
+        if (!\extension_loaded('pcntl')) {
+            $this->markTestSkipped('PCNTL extension required');
+        }
+
+        $registry = new SignalRegistry();
+
+        $signal = \SIGUSR2;
+
+        $original = pcntl_signal_get_handler($signal);
+
+        $handler = static function () {};
+
+        $registry->pushCurrentHandlers();
+        $registry->register($signal, $handler);
+
+        $this->assertNotEquals($original, pcntl_signal_get_handler($signal));
+
+        $registry->popPreviousHandlers();
+
+        $this->assertEquals($original, pcntl_signal_get_handler($signal));
+    }
+
+    private function getHandlersForSignal(SignalRegistry $registry, int $signal): array
+    {
+        $ref = new \ReflectionClass($registry);
+        $prop = $ref->getProperty('signalHandlers');
+        $handlers = $prop->getValue($registry);
+
+        return $handlers[$signal] ?? [];
+    }
 }

Original file line number	Diff line number	Diff line change
`@@ -1012,12 +1012,16 @@ protected function doRunCommand(Command $command, InputInterface $input, OutputI`
`1012`	`1012`	`}`
`1013`	`1013`	`}`
`1014`	`1014`
	`1015`	`+ $registeredSignals = false;`
`1015`	`1016`	`$commandSignals = $command instanceof SignalableCommandInterface ? $command->getSubscribedSignals() : [];`
`1016`	`1017`	`if ($commandSignals \|\| $this->dispatcher && $this->signalsToDispatchEvent) {`
`1017`	`1018`	`if (!$this->signalRegistry) {`
`1018`	`1019`	`throw new RuntimeException('Unable to subscribe to signal events. Make sure that the "pcntl" extension is installed and that "pcntl_*" functions are not disabled by your php.ini\'s "disable_functions" directive.');`
`1019`	`1020`	`}`
`1020`	`1021`
	`1022`	`+ $registeredSignals = true;`
	`1023`	`+ $this->getSignalRegistry()->pushCurrentHandlers();`
	`1024`	`+`
`1021`	`1025`	`if ($this->dispatcher) {`
`1022`	`1026`	`// We register application signals, so that we can dispatch the event`
`1023`	`1027`	`foreach ($this->signalsToDispatchEvent as $signal) {`
`@@ -1067,7 +1071,13 @@ protected function doRunCommand(Command $command, InputInterface $input, OutputI`
`1067`	`1071`	`}`
`1068`	`1072`
`1069`	`1073`	`if (null === $this->dispatcher) {`
`1070`		`- return $command->run($input, $output);`
	`1074`	`+ try {`
	`1075`	`+ return $command->run($input, $output);`
	`1076`	`+ } finally {`
	`1077`	`+ if ($registeredSignals) {`
	`1078`	`+ $this->getSignalRegistry()->popPreviousHandlers();`
	`1079`	`+ }`
	`1080`	`+ }`
`1071`	`1081`	`}`
`1072`	`1082`
`1073`	`1083`	`// bind before the console.command event, so the listeners have access to input options/arguments`
`@@ -1097,6 +1107,10 @@ protected function doRunCommand(Command $command, InputInterface $input, OutputI`
`1097`	`1107`	`if (0 === $exitCode = $event->getExitCode()) {`
`1098`	`1108`	`$e = null;`
`1099`	`1109`	`}`
	`1110`	`+ } finally {`
	`1111`	`+ if ($registeredSignals) {`
	`1112`	`+ $this->getSignalRegistry()->popPreviousHandlers();`
	`1113`	`+ }`
`1100`	`1114`	`}`
`1101`	`1115`
`1102`	`1116`	`$event = new ConsoleTerminateEvent($command, $input, $output, $exitCode);`