Implemented better PID handling. #39
Conversation
WalkthroughThe changes refactor the PID detection logic in the Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Unit Test
participant Context as PhpServerContext
participant Lsof as getPidLsof
participant Netstat as getPidNetstat
Test->>Context: getPid(port)
Context->>Lsof: getPidLsof(port)
alt Lsof finds PID
Lsof-->>Context: PID
Context-->>Test: PID
else Lsof returns 0
Context->>Netstat: getPidNetstat(port)
alt Netstat finds PID
Netstat-->>Context: PID
Context-->>Test: PID
else Netstat returns 0
Context-->>Test: throws RuntimeException
end
end
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## main #39 +/- ##
==========================================
+ Coverage 73.57% 75.00% +1.42%
==========================================
Files 3 3
Lines 420 432 +12
==========================================
+ Hits 309 324 +15
+ Misses 111 108 -3 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (2)
src/DrevOps/BehatPhpServer/PhpServerContext.php (1)
542-582:⚠️ Potential issue
getPidLsof()wrongly filters out common PHP binaries such asphp-fpm
lsofmay report executables likephp-fpm,php74,php8.2, etc.
Requiring the first column to be strictly equal to the stringphpcreates false negatives.In addition, leading spaces are not removed, so
$parts[0]can even be an empty string.- foreach ($output as $line) { - $line = preg_replace('/\s+/', ' ', $line); + foreach ($output as $line) { + // Collapse multiple spaces _and_ trim both ends. + $line = trim(preg_replace('/\s+/', ' ', $line)); $this->debug(sprintf('Processing line: %s', $line)); $parts = explode(' ', (string) $line); - if (count($parts) > 1 && $parts[0] === 'php' && is_numeric($parts[1])) { + // Accept any executable that *starts with* "php" (php, php-fpm, php8.3…). + if (count($parts) > 1 && str_starts_with($parts[0], 'php') && is_numeric($parts[1])) { $pid = intval($parts[1]); $this->debug(sprintf('Found PHP process with PID %s using lsof.', $pid)); return $pid; } }This tiny relaxation makes the helper much more robust across different PHP setups.
tests/phpunit/Unit/PhpServerContextTest.php (1)
431-577: 🛠️ Refactor suggestionThe test bypasses production code paths, reducing coverage
The anonymous subclass manually throws an exception when both helpers return
0:if ($this->expectException && $this->lsofPid === 0 && $this->netstatPid === 0) { throw new \RuntimeException('Unable …'); }This means the real
getPid()implementation (and the new debug message lines 526-527 flagged by Codecov) is never executed in the failure scenario, so a change in exception wording or logic would not be caught.Recommend letting the original method throw instead:
- public function testGetPid(int $port): int { - if ($this->expectException && $this->lsofPid === 0 && $this->netstatPid === 0) { - throw new \RuntimeException('Unable to determine PHP server process for port ' . $port); - } - return $this->getPid($port); - } + public function testGetPid(int $port): int { + // Delegate entirely to the real implementation. + return $this->getPid($port); + }Then rely solely on
$this->expectException(...)in the test body.
This will exercise the actual code, boosting meaningful coverage and confidence.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/DrevOps/BehatPhpServer/PhpServerContext.php(2 hunks)tests/phpunit/Unit/PhpServerContextTest.php(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
tests/phpunit/Unit/PhpServerContextTest.php (1)
src/DrevOps/BehatPhpServer/PhpServerContext.php (7)
PhpServerContext(18-680)processExists(483-497)getPidLsof(542-582)getPidNetstat(593-642)getPid(511-531)debug(670-678)executeCommand(657-662)
src/DrevOps/BehatPhpServer/PhpServerContext.php (1)
tests/phpunit/Unit/PhpServerContextTest.php (7)
getPidLsof(487-489)getPidNetstat(491-493)debug(504-506)debug(660-662)debug(791-793)executeCommand(643-654)executeCommand(774-785)
🪛 GitHub Check: codecov/patch
src/DrevOps/BehatPhpServer/PhpServerContext.php
[warning] 526-527: src/DrevOps/BehatPhpServer/PhpServerContext.php#L526-L527
Added lines #L526 - L527 were not covered by tests
| protected function getPidNetstat(int $port): int { | ||
| if (!$this->executeCommand('which netstat 2>/dev/null')) { | ||
| return 0; | ||
| } | ||
|
|
||
| $command = sprintf("netstat -an 2>/dev/null | grep ':%s' | grep 'LISTEN'", $port); | ||
|
|
||
| $output = []; | ||
| $this->executeCommand($command, $output); | ||
|
|
||
| if (empty($output)) { | ||
| // Try without the LISTEN filter in case the process is in another state. | ||
| $command = str_replace(" | grep 'LISTEN'", '', $command); | ||
| $this->debug(sprintf('No LISTEN processes found, retrying with command: %s', $command)); | ||
| $this->executeCommand($command, $output); | ||
| } | ||
|
|
||
| if (empty($output)) { | ||
| $this->debug('No processes found on port ' . $port); | ||
| return 0; | ||
| } | ||
|
|
||
| // Log all found processes. | ||
| foreach ($output as $i => $line) { | ||
| $this->debug(sprintf('Found process %d: %s', $i + 1, $line)); | ||
| } | ||
|
|
||
| foreach ($output as $line) { | ||
| $line = preg_replace('/\s+/', ' ', $line); | ||
| $this->debug(sprintf('Processing line: %s', $line)); | ||
| $parts = explode(' ', (string) $line); | ||
|
|
||
| foreach ($parts as $part) { | ||
| if (str_contains($part, '/php')) { | ||
| $pid_name_parts = explode('/', $part); | ||
| if (count($pid_name_parts) > 1) { | ||
| $found_pid = $pid_name_parts[0]; | ||
| $name = $pid_name_parts[1]; | ||
| if (is_numeric($found_pid) && strpos($name, 'php') === 0) { | ||
| $pid = intval($found_pid); | ||
| $this->debug(sprintf('Found PHP process with PID %s using netstat.', $pid)); | ||
| break; | ||
| return $pid; | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if ($pid === 0) { | ||
| $this->debug('Could not identify PHP process from output: ' . implode("\n", $output)); | ||
| throw new \RuntimeException(sprintf('Unable to determine if PHP server was started: PID is 0 in command "%s" output. Manually identify the process and terminate it.', $command)); | ||
| } | ||
|
|
||
| return $pid; | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Minor: duplicated parsing logic could be centralised
getPidLsof() and getPidNetstat() share ~80 % identical parsing & debug code.
Extracting a small private helper like parsePidLines(callable $filter, array $output): int would DRY the class and ease future maintenance, but this is optional.
| $pid = $this->getPidLsof($port); | ||
| if ($pid === 0) { | ||
| $pid = $this->getPidNetstat($port); | ||
| } | ||
| elseif ($this->executeCommand('which netstat 2>/dev/null')) { | ||
| $command = sprintf("netstat -an 2>/dev/null | grep ':%s' | grep 'LISTEN'", $port); | ||
| $type = 'netstat'; | ||
|
|
||
| if ($pid === 0) { | ||
| $this->debug('Could not identify PHP process using lsof or netstat.'); | ||
| throw new \RuntimeException(sprintf('Unable to determine PHP server process for port %d. Manually identify the process and terminate it.', $port)); | ||
| } | ||
| else { | ||
| $this->debug('No supported OS utilities found for process detection.'); | ||
| throw new \RuntimeException('Unable to determine if PHP server was started: no supported OS utilities found. Manually identify the process and terminate it.'); | ||
|
|
||
| return $pid; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate that the helper-returned PID is actually alive
getPid() trusts the first non-zero value coming from getPidLsof() / getPidNetstat().
If the process has already exited between the helper call and the moment you act upon the PID, you will still return a stale value, causing terminateProcess() to fail later on.
if ($pid === 0) {
$this->debug('Could not identify PHP process using lsof or netstat.');
throw new \RuntimeException(sprintf('Unable to determine PHP server process for port %d. Manually identify the process and terminate it.', $port));
}
+ // Double-check that the PID is still running.
+ if (!$this->processExists($pid)) {
+ $this->debug(sprintf('PID %d reported by helpers is no longer running.', $pid));
+ throw new \RuntimeException(sprintf('Process %d (port %d) is not running any longer.', $pid, $port));
+ }
+
return $pid;
}This extra guard makes the public behaviour deterministic and prevents silent failures when a zombie PID slips through.
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 526-527: src/DrevOps/BehatPhpServer/PhpServerContext.php#L526-L527
Added lines #L526 - L527 were not covered by tests
| // Create a subclass of PhpServerContext that we can customize. | ||
| $mock_class = new class($test_class, $lsof_exists, $output, $expected_pid) extends PhpServerContext { | ||
| /** | ||
| * Flag indicating if lsof exists on the system. | ||
| */ | ||
| private bool $lsofExists; | ||
|
|
||
| /** | ||
| * Mock output from the lsof command. | ||
| * | ||
| * @var array<string> | ||
| */ | ||
| private array $mockOutput; | ||
|
|
||
| /** | ||
| * Constructor. | ||
| * | ||
| * @param object $test_class | ||
| * The test class instance. | ||
| * @param bool $lsof_exists | ||
| * Whether lsof exists on the system. | ||
| * @param array<string> $output | ||
| * The output from the lsof command. | ||
| * @param int $expected_pid | ||
| * The expected PID to be returned. | ||
| * | ||
| * @phpstan-ignore-next-line | ||
| */ | ||
| public function __construct(object $test_class, bool $lsof_exists, array $output, int $expected_pid) { | ||
| $this->lsofExists = $lsof_exists; | ||
| $this->mockOutput = $output; | ||
| // Skip parent constructor. | ||
| } | ||
|
|
||
| /** | ||
| * Execute a command. |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Unused constructor parameter $expected_pid
$expected_pid is accepted by the anonymous class but never stored or used, which is misleading.
- $mock_class = new class($test_class, $lsof_exists, $output, $expected_pid) extends PhpServerContext {
+ $mock_class = new class($test_class, $lsof_exists, $output) extends PhpServerContext {
…
- public function __construct(object $test_class, bool $lsof_exists, array $output, int $expected_pid) {
+ public function __construct(object $test_class, bool $lsof_exists, array $output) {Please clean up the signature (same for the netstat test below) to avoid dead parameters.
AlexSkrypnyk
force-pushed
the
feture/fix-pid-in-alpine
branch
from
b88fb93 to
23e3f45
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/DrevOps/BehatPhpServer/PhpServerContext.php (1)
543-562: 🧹 Nitpick (assertive)Use
lsof -tfor simpler & faster PID extraction
lsof -i -P -n | grep …spawns two extra greps and forces you to parse the full line.
lsof -ti :$port -sTCP:LISTEN -c ^phpwill output PIDs only, one per line, which:
- avoids the heavy string manipulation that follows,
- eliminates the risk of matching a non-PHP executable that merely contains the word “php”.
- $command = sprintf("lsof -i -P -n 2>/dev/null | grep 'php' | grep ':%s' | grep 'LISTEN'", $port); + $command = sprintf("lsof -ti :%d -sTCP:LISTEN -c ^php 2>/dev/null", $port);Parsing then reduces to
return (int) ($output[0] ?? 0);.
♻️ Duplicate comments (4)
src/DrevOps/BehatPhpServer/PhpServerContext.php (1)
520-531: 🛠️ Refactor suggestionCache the discovered PID and double-check liveness
getPid()returns a fresh PID but never assigns it to$this->pid.
Subsequent calls will re-run the detection logic even though nothing changed.In a previous review we suggested validating that the helper-returned PID is still alive before returning it. The concern is still present.
- return $pid; + // Persist for future calls. + $this->pid = $pid; + + // Final sanity-check to avoid returning a zombie PID. + if (!$this->processExists($pid)) { + $this->debug(sprintf('PID %d reported by helpers is no longer running.', $pid)); + throw new \RuntimeException(sprintf('Process %d (port %d) is not running any longer.', $pid, $port)); + } + + return $pid;tests/phpunit/Unit/PhpServerContextTest.php (3)
474-482: 🧹 Nitpick (assertive)Unused constructor parameter
$expected_pid
$expected_pidis passed to the anonymous class but never stored or referenced, which can confuse future readers.- public function __construct(object $test_class, bool $has_pid, int $lsof_pid, int $netstat_pid, bool $expect_exception) { + public function __construct(object $test_class, bool $has_pid, int $lsof_pid, int $netstat_pid, bool $expect_exception) { $this->hasPid = $has_pid; $this->lsofPid = $lsof_pid; $this->netstatPid = $netstat_pid; $this->expectException = $expect_exception; $this->pid = $has_pid ? 12345 : 0; - // Skip parent constructor. + // Skip parent constructor. }(identical issue exists in the
getPidLsofandgetPidNetstathelper test classes).
622-626: 🧹 Nitpick (assertive)Parameter
$expected_pidis never used inside the constructorSame nitpick as above: the parameter is superfluous and can be safely removed to declutter the test helper.
753-757: 🧹 Nitpick (assertive)Redundant
$expected_pidparameterThe constructor for the netstat mock also carries an unused
$expected_pid. Dropping it keeps the test helpers minimal and easier to read.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Lite
📒 Files selected for processing (3)
.github/workflows/test-php.yml(3 hunks)src/DrevOps/BehatPhpServer/PhpServerContext.php(2 hunks)tests/phpunit/Unit/PhpServerContextTest.php(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/DrevOps/BehatPhpServer/PhpServerContext.php (1)
tests/phpunit/Unit/PhpServerContextTest.php (7)
getPidLsof(487-489)getPidNetstat(491-493)debug(504-506)debug(660-662)debug(791-793)executeCommand(643-654)executeCommand(774-785)
🔇 Additional comments (1)
.github/workflows/test-php.yml (1)
25-28:❌ Incorrect review comment
PHP 8.4 image may not be available yet
shivammathur/setup-php@v2currently does not publish nightly images for future minor versions until they enter the alpha/beta phase.
Including8.4in the matrix could make every job that runs on that axis fail immediately with “PHP 8.4 is not available”.Consider guarding the version behind an
include-matrix that is only enabled on thenightlychannel, or remove it until the image exists.
I’ll verify whether
shivammathur/setup-php@v2advertises support for PHP 8.4 in its README.
🏁 Script executed:
#!/bin/bash # Check if PHP 8.4 is listed as a supported version in the v2 README curl -s https://raw.githubusercontent.com/shivammathur/setup-php/v2/README.md | grep -n "8\\.4"Length of output: 1358
No action needed: PHP 8.4 is already supported by shivammathur/setup-php@v2
The v2 README lists PHP 8.4 as a “Stable”-“Active” version, so including it in your matrix will not break the workflow.Likely an incorrect or invalid review comment.
| key: ${{ matrix.os }}-php-${{ matrix.php-versions }}-${{ hashFiles('**/composer.lock') }} | ||
|
|
||
| - name: Setup PHP |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Cache key is unique but restore-keys are missing
The cache is written with a key that includes OS + PHP version, which is great.
However, without restore-keys Composer will do a fresh install whenever the lock-file hash changes even slightly. Adding a generic prefix (e.g. "${{ matrix.os }}-php-${{ matrix.php-versions }}-") to restore-keys will let the job fall back to an older, but still useful, cache layer.
Summary by CodeRabbit