[Process] Fix Inconsistent Exit Status in proc_get_status for PHP Versions Below 8.3

[Luc45]

Q	A
Branch?	5.4
Bug fix?	yes
New feature?	no
Deprecations?	no
Issues	N/A
License	MIT

This PR addresses a bug in Symfony's Process component affecting PHP versions prior to 8.3. In these versions, calling proc_get_status multiple times on the same process resource only returns the correct exit status on the first call, with subsequent calls returning -1 due to the process being discarded. This behavior can lead to race conditions and incorrect process status reporting in Symfony applications.

PHP 8.3 changelog notes/fix PR:

Executing proc_get_status() multiple times

Executing proc_get_status() multiple times will now always return the right value on POSIX systems.
Previously, only the first call of the function returned the right value. Executing proc_close()
after proc_get_status() will now also return the right exit code. Previously this would return -1.

Symfony Process is very good at avoiding this, but it's possible to trigger this behavior if you pass the Process as a reference to the output callback itself, and inside of it you invoke a method that eventually triggers proc_get_status, such as isRunning, isSuccessful, etc.

This PR tries to mimick PHP 8.3 behavior in older versions, caching the first reported valid exit status code once the process exits, and reusing the cached value if the new exit status code is now invalid.

I believe this bug has been pestering Symfony Process for a long time, as per this Stack Overflow question, but it was very hard to track it down, and only happens in certain conditions, and it also have a racing condition factor depending on the time between the last output of the process and it's termination.

Changes:

• Added caching mechanism for the exit status in the Process component.
• Ensured the cached status is used for subsequent proc_get_status calls if PHP version is below 8.3.

Proof of Concept:

To demonstrate the issue and the effectiveness of the fix, a proof of concept script is included below. This script uses Symfony's Process component to start a subprocess that outputs a message and exits with a specific status code. The script then attempts to retrieve the exit status of the process using getExitCode(). In PHP versions prior to 8.3, without the proposed fix, this script will often incorrectly report an exit code of -1 due to the race condition described earlier.

<?php

if ( getenv( 'OUTPUT' ) ) {
	#echo 'This should fail when "wait" gets large';
	sleep( 2 );
	echo 'This should fail even when "wait" is small';
	die( 123 );
}

use Symfony\Component\Process\Process;

require_once __DIR__ . '/vendor/autoload.php';

$wait = 0;

do {
	// Increasingly bigger waits.
	$wait += 100000;
	echo sprintf( 'Wait: %s', $wait ) . PHP_EOL;

	$p = new Process( [ 'php', __FILE__ ], __DIR__, [
		'OUTPUT' => 1,
	] );

	$p->start( function ( string $type, string $out ) use ( $p ) {
		echo $out . PHP_EOL;

		/**
		 * Calling most methods in Symfony Process that triggers
		 * updateStatus() can potentially trigger the -1 bug.
		 *
		 * @see Process::updateStatus()
		 */
		echo sprintf( 'Is Running: %s', $p->isRunning() ? 'Yes' : 'No' ) . PHP_EOL;
		echo sprintf( 'Exit Code: %s', $p->getExitCode() ) . PHP_EOL;
	} );

	while ( $p->isRunning() ) {
		usleep( $wait );
	}

	if ( ! $p->isSuccessful() ) {
		break;
	}
} while ( true );

$is_started = $p->isStarted();
$is_running = $p->isRunning();
$exit_code  = $p->getExitCode();

echo sprintf( 'Started: %s, Running: %s, Exit code: %s', $is_started, $is_running, $exit_code ) . PHP_EOL;

Impact:

• This change only affects Symfony applications running on PHP versions below 8.3.
• It improves the reliability of process status reporting in these environments.
• No breaking changes or backward compatibility issues are introduced.

Testing:

I haven't added tests to this PR because:

• This bug involves racing conditions, which is hard to replicate.
• It's past 1 AM local time right now, and I've been debugging this for the past 6 hours.
• The provided "Proof of Concept" can serve as an initial check to confirm the bug.

I'm really not the type of developer that does not write tests, but I beg my pardon for the reasons above.

Considerations

• Based on the proc_open implementation, this fix might not be needed on Windows.
• You can find additional context for this bug in this Stack Overflow answer.

[carsonbot]

Hey!

Thanks for your PR. You are targeting branch "5.4" but it seems your PR description refers to branch "5.4, 6.4, 7.0".
Could you update the PR description or change target branch? This helps core maintainers a lot.

Cheers!

Carsonbot

[nicolas-grekas]

Can you please add a test case to cover the behavior?

[nicolas-grekas]

Reading now your full description... I'd still be great to figure out a test case :)

[stof]

A first testcase could be one running your reproducer (even if it does not fail 100% of the time due to relying on a race condition, it would still be likely to fail sometimes if we reintroduce the issue in the future)

[Luc45]

I've added a test that reproduces the bug, it fails on 7.1 tag and passes on this branch.

[Luc45]

If I remove the fix, you can see the racing condition factor in the tests causing the tests to fail erratically on the longRunning process test.

[Luc45]

My tests uses sleep to mimick racing condition scenarios, which adds a total of 4 seconds to the Process test suite, which is not ideal. I've grouped them in time-sensitive, and I'm open to ideas. Maybe we could reduce the sleep from 2 to 1 second. [Done]

[Luc45]

I don't know why CliDumperTest::testCollapse is failing, as it doesn't seem to use Process at all.

[nicolas-grekas]

LGTM thanks.

[Luc45]

I didn't want to touch this but Fabbot code style tests asks to remove it. Bringing it up just for clarity.

[Luc45]

7.2 tests fail because of the short closure syntax, want me to keep it @nicolas-grekas?

[nicolas-grekas]

nope, indeed

[Luc45]

Tests in Windows is failing because it doesn't have sleep. I've tweaked it to use inline PHP with sleep instead, and I can confirm it works by prepending time to the command, adjusting the sleep in the spawning process and re-running the test.

• Sleep 5: php ./phpunit symfony src/Symfony/Component/Process 83,90s user 59,06s system 688% cpu 20,776 total
• Sleep 1: php ./phpunit symfony src/Symfony/Component/Process 80,13s user 58,70s system 1094% cpu 12,684 total

Since it runs a bunch of tests in parallel, we have to ignore 80s and 59s because that's CPU time, and look at "actual time" which is the last one: 20s and 12s respectively, which sounds about right. Re-running.

[Luc45]

The remaining Windows failures seems unrelated

[nicolas-grekas]

Thank you @Luc45.