Skip to content

--parallel option fails if pcntl_waitpid returns non-managed process ID #1112

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
4 tasks done
NanoSector opened this issue Jun 4, 2025 · 6 comments · May be fixed by #1126
Open
4 tasks done

--parallel option fails if pcntl_waitpid returns non-managed process ID #1112

NanoSector opened this issue Jun 4, 2025 · 6 comments · May be fixed by #1126

Comments

@NanoSector
Copy link

NanoSector commented Jun 4, 2025

Describe the bug

The returned PID of pcntl_waitpid is not checked against the list of known PIDs in src/Runner.php. In our setup, this call always returns a PID with a value 1 lower than the lowest expected PID in the $childProcs list which causes phpcs and phpcbf to fail.

E.g. the $childProcs list contains [5125, 5126], but the function first returns 5124 and this causes the runner to crash with the following exception:

PHP Fatal error:  Uncaught PHP_CodeSniffer\Exceptions\RuntimeException: Undefined array key 5124 in .../vendor/squizlabs/php_codesniffer/src/Runner.php on line 789 in .../vendor/squizlabs/php_codesniffer/src/Runner.php:624
Stack trace:
#0 .../vendor/squizlabs/php_codesniffer/src/Runner.php(789): PHP_CodeSniffer\Runner->handleErrors()
#1 .../vendor/squizlabs/php_codesniffer/src/Runner.php(560): PHP_CodeSniffer\Runner->processChildProcs()
#2 .../vendor/squizlabs/php_codesniffer/src/Runner.php(216): PHP_CodeSniffer\Runner->run()
#3 .../vendor/squizlabs/php_codesniffer/bin/phpcbf(14): PHP_CodeSniffer\Runner->runPHPCBF()
#4 .../vendor/bin/phpcbf(119): include('...')
#5 {main}
  thrown in .../vendor/squizlabs/php_codesniffer/src/Runner.php on line 624

Code sample

N/A, this bug happens in every project regardless of code snippets.

To reproduce

Steps to reproduce the behavior:

  1. Create two test PHP files and point phpcbf to them with the following command: bash -c 'bash --init-file <(echo "cd /home/vagrant/projects/my-project") -i -t -c "vendor/bin/phpcbf --parallel=8 -p test.php test2.php"'
  2. Notice the exception

Expected behavior

The parallel option works & phpcs only takes care of managed processes, even when invoked with constructs like these.

Versions (please complete the following information)

Operating System Ubuntu 22.04
PHP version 8.2
PHP_CodeSniffer version 3.13.0
Standard PSR12
Install type Composer (local)

Additional context

pcntl_waitpid already returns the lower value before the first call in pcntl_fork in Runner.php so this is very likely an external factor.

Patching Runner.php with the following code is sufficient to fix the issue, but I am not sure if this is the wanted solution:

        while (count($childProcs) > 0) {
            $pid = pcntl_waitpid(0, $status);
            if ($pid <= 0) {
                continue;
            }

change to

        while (count($childProcs) > 0) {
            $pid = pcntl_waitpid(0, $status);
            if ($pid <= 0 || !\array_key_exists($pid, $childProcs)) {
                continue;
            }

Please confirm

  • I have searched the issue list and am not opening a duplicate issue.
  • I have read the Contribution Guidelines and this is not a support question.
  • I confirm that this bug is a bug in PHP_CodeSniffer and not in one of the external standards.
  • I have verified the issue still exists in the master branch of PHP_CodeSniffer.
@rodrigoprimo
Copy link
Contributor

Thanks for reporting this issue, @NanoSector!

I was unable to reproduce it on an Ubuntu 24.04 installation running PHP 8.3. I tested using the following commands:

phpcs test.php test2.php --parallel=2
phpcbf test.php test2.php --parallel=2

Both PHPCS and PHPCBF worked as expected. I was also able to confirm using Xdebug that pcntl_waitpid() returns the correct PIDs. Is there anything unique to your environment that might help explain why pcntl_waitpid() is not returning the expected values? Are you able to test this on a different machine/environment to see if you can reproduce it?

As a side note, the example you provided (phpcs test.php test.php --parallel=2 - note that the command is passing the same file twice) uncovered what I believe to be another issue. I reported it here: #1113.

@NanoSector
Copy link
Author

Thanks @rodrigoprimo, I'm not really sure what is causing this either, I hadn't looked into it that far. My assumption is that a PHP extension is starting a thread (maybe XDebug?) for some reason as there's no code preceding the phpcbf/phpcs execution.

The Ubuntu 22.04 machine in question is a slightly modified Laravel Homestead VM, but I understand that's a bit much to ask to setup to try to reproduce this issue.

I'll try to reproduce it on my personal MacBook later tonight, if I can't reproduce it there I'll make a dump of loaded php modules and PHP's ini values and compare the two, in that case I'll get back to you tomorrow.
But I highly doubt the underlying issue stems from phpcs, I think it's just exhibiting symptoms, the fix seemed small enough to report here.

Funny how this uncovered another issue 😅 I was fairly positive that I changed the file names appropriately.

@jrfnl
Copy link
Member

jrfnl commented Jun 4, 2025

@NanoSector I look forward to seeing the more detailed environment info tomorrow. I kind of doubt it will be related to Xdebug, as Xdebug is used a lot and I've not seen this issue reported before, but definitely curious to see what is causing it.

@NanoSector
Copy link
Author

NanoSector commented Jun 5, 2025

I dug a little deeper, turns out it is not a PHP environment issue but a side effect of a helper command I use to talk to the Vagrant VM from the host, written in Fish.

It eventually executes the following command:

ssh -o LogLevel=QUIET -t vagrant@192.168.56.56 -p 22 "bash --init-file <(echo \"cd /home/vagrant/projects/my-project\") -t -c \"vendor/bin/phpcbf --parallel=8\""

Running the following in the VM itself does not exhibit this behaviour:

bash --init-file <(echo "cd /home/vagrant/projects/my-project") -i -t -c "vendor/bin/phpcbf --parallel=8"

And moving the cd call into the -c argument also does not exhibit this behaviour:

ssh -o LogLevel=QUIET -t vagrant@192.168.56.56 -p 22 "bash -t -c \"cd /home/vagrant/projects/my-project && vendor/bin/phpcbf --parallel=8\""

What I think is happening is that Bash is somehow putting the file redirection into the same process group as phpcbf and this trips up the Runner. The in-VM implementation does file redirection before invoking bash with phpcbf.

This bug can be triggered from Bash without SSH by the following command:

bash -c 'bash --init-file <(echo "cd /home/vagrant/projects/my-project") -i -t -c "vendor/bin/phpcbf --parallel=8 -p"'

I do feel like phpcs/phpcbf should at least check that it's monitoring its own processes / ignore foreign processes, regardless of this unusual use case.

@jrfnl
Copy link
Member

jrfnl commented Jun 5, 2025

@NanoSector Interesting case for sure. Would there be any way this can be triggered without your custom Vagrant VM set up though ?

@NanoSector
Copy link
Author

@jrfnl Yes, you should be able to trigger it locally using the last command I provided (replace the path with something pointing to a project with phpcs in it). This works on my MacBook without any VMs involved.

NanoSector added a commit to NanoSector/PHP_CodeSniffer that referenced this issue Jun 6, 2025
…rocess

The --parallel option spawns processes with pcntl_fork and expects the
result of pcntl_waitpid to be one of its spawned processes. It looks up
the temporary output file of a process in the given $childProcs array
without checking if the returned process ID is actually managed, which
under some circumstances caused by external factors may result in an
undefined array index error.

Fixes PHPCSStandards#1112
NanoSector added a commit to NanoSector/PHP_CodeSniffer that referenced this issue Jun 6, 2025
…rocess

The --parallel option spawns processes with pcntl_fork and expects the
result of pcntl_waitpid to be one of its spawned processes. It looks up
the temporary output file of a process in the given $childProcs array
without checking if the returned process ID is actually managed, which
under some circumstances caused by external factors may result in an
undefined array index error.

Fixes PHPCSStandards#1112
NanoSector added a commit to NanoSector/PHP_CodeSniffer that referenced this issue Jun 9, 2025
…rocess

The --parallel option spawns processes with pcntl_fork and expects the
result of pcntl_waitpid to be one of its spawned processes. It looks up
the temporary output file of a process in the given $childProcs array
without checking if the returned process ID is actually managed, which
under some circumstances caused by external factors may result in an
undefined array index error.

Fixes PHPCSStandards#1112
NanoSector added a commit to NanoSector/PHP_CodeSniffer that referenced this issue Jun 9, 2025
This introduces the bashunit test suite as requested by @jrfnl. I opted to include the binary in the repository to make local testing easier & to lock the dependency on the project side instead of relying on external scripts.

This tests both a simple happy and a simple unhappy flow, as well as a test case for PHPCSStandards#1112.
NanoSector added a commit to NanoSector/PHP_CodeSniffer that referenced this issue Jun 9, 2025
This introduces the bashunit test suite as requested by @jrfnl. I opted to include the binary in the repository to make local testing easier & to lock the dependency on the project side instead of relying on external scripts.

This tests both a simple happy and a simple unhappy flow, as well as a test case for PHPCSStandards#1112.
NanoSector added a commit to NanoSector/PHP_CodeSniffer that referenced this issue Jun 9, 2025
This introduces the bashunit test suite as requested by @jrfnl. I opted to include the binary in the repository to make local testing easier & to lock the dependency on the project side instead of relying on external scripts.

This tests both a simple happy and a simple unhappy flow, as well as a test case for PHPCSStandards#1112.
NanoSector added a commit to NanoSector/PHP_CodeSniffer that referenced this issue Jun 9, 2025
This introduces the bashunit test suite as requested by @jrfnl. I opted to include the binary in the repository to make local testing easier & to lock the dependency on the project side instead of relying on external scripts.

This tests both a simple happy and a simple unhappy flow, as well as a test case for PHPCSStandards#1112.
NanoSector added a commit to NanoSector/PHP_CodeSniffer that referenced this issue Jun 9, 2025
This introduces the bashunit test suite as requested by @jrfnl. I opted to include the binary in the repository to make local testing easier & to lock the dependency on the project side instead of relying on external scripts.

This tests both a simple happy and a simple unhappy flow, as well as a test case for PHPCSStandards#1112.
NanoSector added a commit to NanoSector/PHP_CodeSniffer that referenced this issue Jun 9, 2025
This introduces the bashunit test suite as requested by @jrfnl. I opted to include the binary in the repository to make local testing easier & to lock the dependency on the project side instead of relying on external scripts.

This tests both a simple happy and a simple unhappy flow, as well as a test case for PHPCSStandards#1112.
NanoSector added a commit to NanoSector/PHP_CodeSniffer that referenced this issue Jun 9, 2025
This introduces the bashunit test suite as requested by @jrfnl. I opted to include the binary in the repository to make local testing easier & to lock the dependency on the project side instead of relying on external scripts.

This tests both a simple happy and a simple unhappy flow, as well as a test case for PHPCSStandards#1112.
NanoSector added a commit to NanoSector/PHP_CodeSniffer that referenced this issue Jun 9, 2025
This introduces the bashunit test suite as requested by @jrfnl. I opted to include the binary in the repository to make local testing easier & to lock the dependency on the project side instead of relying on external scripts.

This tests both a simple happy and a simple unhappy flow, as well as a test case for PHPCSStandards#1112.
NanoSector added a commit to NanoSector/PHP_CodeSniffer that referenced this issue Jun 9, 2025
This introduces the bashunit test suite as requested by @jrfnl. I opted to include the binary in the repository to make local testing easier & to lock the dependency on the project side instead of relying on external scripts.

This tests both a simple happy and a simple unhappy flow, as well as a test case for PHPCSStandards#1112.
NanoSector added a commit to NanoSector/PHP_CodeSniffer that referenced this issue Jun 9, 2025
This introduces the bashunit test suite as requested by @jrfnl. I opted to include the binary in the repository to make local testing easier & to lock the dependency on the project side instead of relying on external scripts.

This tests both a simple happy and a simple unhappy flow, as well as a test case for PHPCSStandards#1112.
NanoSector added a commit to NanoSector/PHP_CodeSniffer that referenced this issue Jun 9, 2025
This introduces the bashunit test suite as requested by @jrfnl. I opted to include the binary in the repository to make local testing easier & to lock the dependency on the project side instead of relying on external scripts.

This tests both a simple happy and a simple unhappy flow, as well as a test case for PHPCSStandards#1112.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants