Draft: Fix zombie process on process destruction

composer-require-check.json

@@ -20,7 +20,11 @@
         "escapeArgument",
         "IS_WINDOWS",
         "posix_kill",
+        "posix_get_last_error",
+        "posix_strerror",
         "pcntl_waitpid",
+        "pcntl_get_last_error",
+        "PCNTL_EINTR",
         "WNOHANG"
     ],
     "php-core-extensions": [

src/Internal/Posix/PosixHandle.php

@@ -37,7 +37,7 @@ public function __construct(
         $stdin = \WeakReference::create($stdin);
         $this->extraDataPipeCallbackId = EventLoop::unreference(EventLoop::onReadable(
             $extraDataPipe,
-            static function (string $callbackId, $stream) use (&$status, $deferred, $stdin, $shellPid): void {
+            static function (string $callbackId, $stream) use (&$status, $deferred, $stdin, $proc, $shellPid): void {
                 EventLoop::disable($callbackId);
 
                 $status = ProcessStatus::Ended;
@@ -56,7 +56,7 @@ static function (string $callbackId, $stream) use (&$status, $deferred, $stdin,
                     \fclose($stream);
                 }
 
-                self::asyncWaitPid($shellPid);
+                self::asyncWaitPid($proc, $shellPid);
             },
         ));
     }
@@ -75,18 +75,28 @@ public function unreference(): void
         }
     }
 
-    private static function asyncWaitPid(int $pid): void
+    /** @param resource $proc */
+    private static function asyncWaitPid($proc, int $pid): void
     {
-        if (self::hasChildExited($pid)) {
+        if (self::hasChildExited($proc, $pid)) {
             return;
         }
 
-        EventLoop::unreference(EventLoop::defer(static fn () => self::asyncWaitPid($pid)));
+        EventLoop::unreference(EventLoop::defer(static fn () => self::asyncWaitPid($proc, $pid)));
     }
 
-    private static function hasChildExited(int $pid): bool
+    /** @param resource $proc */
+    private static function hasChildExited($proc, int $pid): bool
     {
-        return !\extension_loaded('pcntl') || \pcntl_waitpid($pid, $status, \WNOHANG) !== 0;
+        if (!\function_exists('pcntl_waitpid')) {
+            return !\proc_get_status($proc)['running'];
+        }
+
+        do {
+            $result = \pcntl_waitpid($pid, $status, \WNOHANG);
+        } while ($result === -1 && \pcntl_get_last_error() === \PCNTL_EINTR);
+
+        return $result !== 0;
     }
 
     public function __destruct()
@@ -96,18 +106,33 @@ public function __destruct()
             $this->extraDataPipeCallbackId = null;
         }
 
-        if ($this->joinDeferred->isComplete()) {
+        if ($this->status === ProcessStatus::Ended) {
+            $this->reapShell();
+            return;
+        }
+
+        self::asyncWaitPid($this->proc, $this->shellPid);
+    }
+
+    public function reapShell(): void
+    {
+        if (\function_exists('pcntl_waitpid')) {
+            do {
+                $result = \pcntl_waitpid($this->shellPid, $status);
+            } while ($result === -1 && \pcntl_get_last_error() === \PCNTL_EINTR);
+
             return;
         }
 
-        self::asyncWaitPid($this->shellPid);
+        while (\proc_get_status($this->proc)['running']) {
+            \usleep(1_000);
+        }
     }
 
     #[\Override]
     public function wait(): void
     {
-        if (\extension_loaded('pcntl')) {
-            \pcntl_waitpid($this->pid, $status);
-        }
+        // Do not block the shutdown handler before ProcHolder destruction terminates the process.
+        self::hasChildExited($this->proc, $this->shellPid);
     }
 }

src/Internal/Posix/PosixRunner.php

@@ -22,6 +22,8 @@
  */
 final class PosixRunner implements ProcessRunner
 {
+    private const ESRCH = 3;
+
     use ForbidCloning;
     use ForbidSerialization;
 
@@ -173,13 +175,32 @@ public function kill(ProcessHandle $handle): void
         $handle->reference();
 
         $this->signal($handle, 9);
+        $handle->reapShell();
     }
 
     #[\Override]
     public function signal(ProcessHandle $handle, int $signal): void
     {
         /** @noinspection PhpComposerExtensionStubsInspection */
-        \posix_kill($handle->pid, $signal);
+        if (\posix_kill($handle->pid, $signal)) {
+            return;
+        }
+
+        $error = \posix_get_last_error();
+        if ($error === self::ESRCH) {
+            return;
+        }
+
+        throw new ProcessException(
+            \sprintf(
+                "Failed to send signal %d to process %d: Errno: %d; %s",
+                $signal,
+                $handle->pid,
+                $error,
+                \posix_strerror($error),
+            ),
+            $error,
+        );
     }
 
     #[\Override]

src/Internal/ProcessHandle.php

@@ -14,9 +14,8 @@ abstract class ProcessHandle
 
     /**
      * @var resource
-     * @psalm-suppress UnusedProperty
      */
-    private $proc;
+    protected $proc;
 
     /** @var DeferredFuture<int> */
     public readonly DeferredFuture $joinDeferred;

test/ProcessTest.php

@@ -6,6 +6,7 @@
 use Amp\Future;
 use Amp\PHPUnit\AsyncTestCase;
 use Amp\Process\Process;
+use Amp\Process\ProcessException;
 use Amp\TimeoutCancellation;
 use const Amp\Process\IS_WINDOWS;
 use function Amp\async;
@@ -143,6 +144,123 @@ public function testKillImmediately(): void
         self::assertSame(IS_WINDOWS ? 1 : 137, $process->join());
     }
 
+    /**
+     * @requires extension pcntl
+     * @runInSeparateProcess
+     * @preserveGlobalState disabled
+     */
+    public function testKillReapsShellWithPcntlWhileProcessIsRetained(): void
+    {
+        $process = Process::start(self::CMD_PROCESS_SLOW);
+        $process->kill();
+
+        // Keep Process reachable after kill; this is the state that previously left the shell unreaped.
+        $status = 0;
+        $remainingChildPid = \pcntl_waitpid(-1, $status);
+        $error = \pcntl_get_last_error();
+        $exitCode = $process->join();
+
+        self::assertSame(-1, $remainingChildPid);
+        self::assertSame(\PCNTL_ECHILD, $error);
+        self::assertSame(137, $exitCode);
+    }
+
+    public function testKillReapsShellWithoutPcntlWhileProcessIsRetained(): void
+    {
+        if (\DIRECTORY_SEPARATOR === "\\") {
+            self::markTestSkipped("Signals are not supported on Windows");
+        }
+
+        $code = \sprintf(
+            'require %s;'
+            . '$process = Amp\\Process\\Process::start("sleep 30");'
+            . '$handle = (new ReflectionProperty($process, "handle"))->getValue($process);'
+            . '$shellPid = (new ReflectionProperty($handle, "shellPid"))->getValue($handle);'
+            . '$process->kill();'
+            . 'exit(posix_kill($shellPid, 0) ? 1 : 0);',
+            \var_export(\dirname(__DIR__) . '/vendor/autoload.php', true),
+        );
+        $process = Process::start([
+            \PHP_BINARY,
+            '-d',
+            'disable_functions=pcntl_waitpid,pcntl_get_last_error',
+            '-r',
+            $code,
+        ]);
+
+        $exitCode = $process->join(new TimeoutCancellation(2));
+        $error = buffer($process->getStderr());
+
+        self::assertSame(0, $exitCode, $error);
+    }
+
+    /**
+     * @requires extension pcntl
+     * @runInSeparateProcess
+     * @preserveGlobalState disabled
+     */
+    public function testProcessDestructionReapsShell(): void
+    {
+        $process = Process::start(self::CMD_PROCESS_SLOW);
+        unset($process);
+
+        $status = 0;
+        self::assertSame(-1, \pcntl_waitpid(-1, $status, \WNOHANG));
+        self::assertSame(\PCNTL_ECHILD, \pcntl_get_last_error());
+    }
+
+    /**
+     * @requires extension pcntl
+     * @runInSeparateProcess
+     * @preserveGlobalState disabled
+     */
+    public function testCompletedProcessDestructionReapsShell(): void
+    {
+        $process = Process::start('exit 0');
+        self::assertSame(0, $process->join());
+        unset($process);
+
+        $status = 0;
+        self::assertSame(-1, \pcntl_waitpid(-1, $status, \WNOHANG));
+        self::assertSame(\PCNTL_ECHILD, \pcntl_get_last_error());
+    }
+
+    /**
+     * @requires extension pcntl
+     */
+    public function testShutdownDoesNotWaitForRunningProcess(): void
+    {
+        $code = \sprintf(
+            'require %s; $GLOBALS["process"] = Amp\\Process\\Process::start("sleep 30");',
+            \var_export(\dirname(__DIR__) . '/vendor/autoload.php', true),
+        );
+        $process = Process::start([\PHP_BINARY, '-r', $code]);
+
+        self::assertSame(0, $process->join(new TimeoutCancellation(2)));
+    }
+
+    /**
+     * @requires extension pcntl
+     */
+    public function testRunningHandleDestructionDoesNotWaitForProcess(): void
+    {
+        // Emulate handle destruction in a long-running PHP worker without depending on garbage collection order.
+        $code = \sprintf(
+            'require %s;'
+            . '$process = Amp\\Process\\Process::start("cat >/dev/null");'
+            . '$handle = (new ReflectionProperty($process, "handle"))->getValue($process);'
+            . '$handle->__destruct();',
+            \var_export(\dirname(__DIR__) . '/vendor/autoload.php', true),
+        );
+        $process = Process::start([\PHP_BINARY, '-r', $code]);
+
+        try {
+            self::assertSame(0, $process->join(new TimeoutCancellation(2)));
+        } finally {
+            $process->kill();
+        }
+    }
+
     public function testKillThenReadStdout(): void
     {
         $this->setTimeout(1);
@@ -259,6 +377,42 @@ public function testSignal(): void
         self::assertSame(42, $process->join());
     }
 
+    /**
+     * @requires extension posix
+     */
+    public function testSignalThrowsIfDeliveryFails(): void
+    {
+        $process = Process::start(self::CMD_PROCESS_SLOW);
+
+        try {
+            $this->expectException(ProcessException::class);
+            $this->expectExceptionMessage('Failed to send signal 9999');
+            $process->signal(9999);
+        } finally {
+            $process->kill();
+            $process->join();
+        }
+    }
+
+    /**
+     * @requires extension posix
+     */
+    public function testSignalIgnoresProcessThatAlreadyExited(): void
+    {
+        $process = Process::start('exit 0');
+
+        // Keep the event loop paused so Process status is not updated before the OS process exits.
+        $isRunning = true;
+        for ($attempt = 0; $attempt < 1000 && $isRunning; ++$attempt) {
+            $isRunning = \posix_kill($process->getPid(), 0);
+            \usleep(1000);
+        }
+
+        self::assertFalse($isRunning);
+        $process->signal(0);
+        self::assertSame(0, $process->join());
+    }
+
     public function testCancellation(): void
     {
         $this->expectException(CancelledException::class);