← 输入框|github.com|刷新
Skip to content

[Tests] Stop php -S servers together with their forked workers - #658

Merged
chr-hertel merged 1 commit into
modelcontextprotocol:mainfrom
ruudk:fix/stop-php-server-workers
Nov 3, 2026
Merged

chr-hertel merged 1 commit into
modelcontextprotocol:mainfrom
ruudk:fix/stop-php-server-workers

Conversation

@ruudk

@ruudk ruudk commented Nov 3, 2026 •

Copy link
Copy Markdown
Contributor

Stopping a php -S process only ended the master, so its PHP_CLI_SERVER_WORKERS were reparented and kept the port and the inherited stdout/stderr open, which made make integration-tests | tail hang after the suite finished. Every test that starts a php -S server now stops it through ServerProcess::stop(), which ends the whole process tree.

@chr-hertel chr-hertel left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @ruudk - good one, could you please streamline the other cases as well?

  • tests/Integration/HttpNegotiationTest.php:48
  • tests/Integration/StatelessClientTest.php:67
  • tests/Integration/StatelessLifecycleTest.php:58
  • tests/Interop/Server/Http/HttpInspectorSnapshotTestCase.php:73

Stopping the php -S process only ended the master. With PHP_CLI_SERVER_WORKERS the workers were reparented, kept their port and the inherited stdout/stderr, so `make integration-tests | tail` never saw EOF. Stop every php -S server the tests start through ServerProcess::stop(), which ends the whole process tree.
@ruudk
ruudk force-pushed the fix/stop-php-server-workers branch from 6117756 to f3b98c1 Compare November 3, 2026 13:39
@ruudk ruudk changed the title [Tests] Stop the forked php -S workers of the dual-era example tests [Tests] Stop php -S servers together with their forked workers Nov 3, 2026
@ruudk

ruudk commented Nov 3, 2026

Copy link
Copy Markdown
Contributor Author

Done!

@ruudk
ruudk requested a review from chr-hertel November 3, 2026 14:05

@chr-hertel chr-hertel left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @ruudk!

@chr-hertel
chr-hertel merged commit b77af64 into modelcontextprotocol:main Nov 3, 2026
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants