From b7decf813f253c5a9902f0be479a4602dee1d779 Mon Sep 17 00:00:00 2001 From: Alexander Pankratov Date: Fri, 28 Aug 2026 16:54:37 +0200 Subject: [PATCH 1/3] Handle failed POSIX signal delivery --- composer-require-check.json | 2 ++ src/Internal/Posix/PosixRunner.php | 22 +++++++++++++++++- test/ProcessTest.php | 37 ++++++++++++++++++++++++++++++ 3 files changed, 60 insertions(+), 1 deletion(-) diff --git a/composer-require-check.json b/composer-require-check.json index 6c1ee5d..4c532e4 100644 --- a/composer-require-check.json +++ b/composer-require-check.json @@ -20,6 +20,8 @@ "escapeArgument", "IS_WINDOWS", "posix_kill", + "posix_get_last_error", + "posix_strerror", "pcntl_waitpid", "WNOHANG" ], diff --git a/src/Internal/Posix/PosixRunner.php b/src/Internal/Posix/PosixRunner.php index 349b4a0..9774f1a 100644 --- a/src/Internal/Posix/PosixRunner.php +++ b/src/Internal/Posix/PosixRunner.php @@ -22,6 +22,8 @@ */ final class PosixRunner implements ProcessRunner { + private const ESRCH = 3; + use ForbidCloning; use ForbidSerialization; @@ -179,7 +181,25 @@ public function kill(ProcessHandle $handle): void 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] diff --git a/test/ProcessTest.php b/test/ProcessTest.php index 92072a5..63b7717 100644 --- a/test/ProcessTest.php +++ b/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; @@ -259,6 +260,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); From c76a60ffd8eb7ea7b161c0f0364b4e269a985514 Mon Sep 17 00:00:00 2001 From: Alexander Pankratov Date: Fri, 28 Aug 2026 16:55:03 +0200 Subject: [PATCH 2/3] Reap POSIX wrapper shell --- composer-require-check.json | 2 + src/Internal/Posix/PosixHandle.php | 27 +++++++-- src/Internal/Posix/PosixRunner.php | 1 + test/ProcessTest.php | 88 ++++++++++++++++++++++++++++++ 4 files changed, 113 insertions(+), 5 deletions(-) diff --git a/composer-require-check.json b/composer-require-check.json index 4c532e4..50e8694 100644 --- a/composer-require-check.json +++ b/composer-require-check.json @@ -23,6 +23,8 @@ "posix_get_last_error", "posix_strerror", "pcntl_waitpid", + "pcntl_get_last_error", + "PCNTL_EINTR", "WNOHANG" ], "php-core-extensions": [ diff --git a/src/Internal/Posix/PosixHandle.php b/src/Internal/Posix/PosixHandle.php index 9dc774e..1515aa3 100644 --- a/src/Internal/Posix/PosixHandle.php +++ b/src/Internal/Posix/PosixHandle.php @@ -86,7 +86,15 @@ private static function asyncWaitPid(int $pid): void private static function hasChildExited(int $pid): bool { - return !\extension_loaded('pcntl') || \pcntl_waitpid($pid, $status, \WNOHANG) !== 0; + if (!\extension_loaded('pcntl')) { + return true; + } + + do { + $result = \pcntl_waitpid($pid, $status, \WNOHANG); + } while ($result === -1 && \pcntl_get_last_error() === \PCNTL_EINTR); + + return $result !== 0; } public function __destruct() @@ -96,18 +104,27 @@ public function __destruct() $this->extraDataPipeCallbackId = null; } - if ($this->joinDeferred->isComplete()) { + if ($this->status === ProcessStatus::Ended) { + $this->reapShell(); return; } self::asyncWaitPid($this->shellPid); } - #[\Override] - public function wait(): void + public function reapShell(): void { if (\extension_loaded('pcntl')) { - \pcntl_waitpid($this->pid, $status); + do { + $result = \pcntl_waitpid($this->shellPid, $status); + } while ($result === -1 && \pcntl_get_last_error() === \PCNTL_EINTR); } } + + #[\Override] + public function wait(): void + { + // Do not block the shutdown handler before ProcHolder destruction terminates the process. + self::hasChildExited($this->shellPid); + } } diff --git a/src/Internal/Posix/PosixRunner.php b/src/Internal/Posix/PosixRunner.php index 9774f1a..46c0b84 100644 --- a/src/Internal/Posix/PosixRunner.php +++ b/src/Internal/Posix/PosixRunner.php @@ -175,6 +175,7 @@ public function kill(ProcessHandle $handle): void $handle->reference(); $this->signal($handle, 9); + $handle->reapShell(); } #[\Override] diff --git a/test/ProcessTest.php b/test/ProcessTest.php index 63b7717..64dd782 100644 --- a/test/ProcessTest.php +++ b/test/ProcessTest.php @@ -144,6 +144,94 @@ public function testKillImmediately(): void self::assertSame(IS_WINDOWS ? 1 : 137, $process->join()); } + /** + * @requires extension pcntl + * @runInSeparateProcess + * @preserveGlobalState disabled + */ + public function testKillReapsShellWhileProcessIsRetained(): 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); + } + + /** + * @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); From 5c32f9f81480f7693c57b4d18ff5aad6be52bd70 Mon Sep 17 00:00:00 2001 From: Alexander Pankratov Date: Fri, 28 Aug 2026 16:55:25 +0200 Subject: [PATCH 3/3] Support shell reaping without PCNTL --- src/Internal/Posix/PosixHandle.php | 30 ++++++++++++++++++----------- src/Internal/ProcessHandle.php | 3 +-- test/ProcessTest.php | 31 +++++++++++++++++++++++++++++- 3 files changed, 50 insertions(+), 14 deletions(-) diff --git a/src/Internal/Posix/PosixHandle.php b/src/Internal/Posix/PosixHandle.php index 1515aa3..60afc33 100644 --- a/src/Internal/Posix/PosixHandle.php +++ b/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,19 +75,21 @@ 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 { - if (!\extension_loaded('pcntl')) { - return true; + if (!\function_exists('pcntl_waitpid')) { + return !\proc_get_status($proc)['running']; } do { @@ -109,15 +111,21 @@ public function __destruct() return; } - self::asyncWaitPid($this->shellPid); + self::asyncWaitPid($this->proc, $this->shellPid); } public function reapShell(): void { - if (\extension_loaded('pcntl')) { + if (\function_exists('pcntl_waitpid')) { do { $result = \pcntl_waitpid($this->shellPid, $status); } while ($result === -1 && \pcntl_get_last_error() === \PCNTL_EINTR); + + return; + } + + while (\proc_get_status($this->proc)['running']) { + \usleep(1_000); } } @@ -125,6 +133,6 @@ public function reapShell(): void public function wait(): void { // Do not block the shutdown handler before ProcHolder destruction terminates the process. - self::hasChildExited($this->shellPid); + self::hasChildExited($this->proc, $this->shellPid); } } diff --git a/src/Internal/ProcessHandle.php b/src/Internal/ProcessHandle.php index a25cfb4..b93a133 100644 --- a/src/Internal/ProcessHandle.php +++ b/src/Internal/ProcessHandle.php @@ -14,9 +14,8 @@ abstract class ProcessHandle /** * @var resource - * @psalm-suppress UnusedProperty */ - private $proc; + protected $proc; /** @var DeferredFuture */ public readonly DeferredFuture $joinDeferred; diff --git a/test/ProcessTest.php b/test/ProcessTest.php index 64dd782..a8958ed 100644 --- a/test/ProcessTest.php +++ b/test/ProcessTest.php @@ -149,7 +149,7 @@ public function testKillImmediately(): void * @runInSeparateProcess * @preserveGlobalState disabled */ - public function testKillReapsShellWhileProcessIsRetained(): void + public function testKillReapsShellWithPcntlWhileProcessIsRetained(): void { $process = Process::start(self::CMD_PROCESS_SLOW); $process->kill(); @@ -165,6 +165,35 @@ public function testKillReapsShellWhileProcessIsRetained(): void 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