Make native-php E2E tests pass on Windows#3343
Conversation
📊 Performance Test ResultsComparing 17938f9 vs trunk app-size
site-editor
site-startup
Results are median values from multiple test runs. Legend: 🟢 Improvement (faster) | 🔴 Regression (slower) | ⚪ No change (<50ms diff) |
| payload: {}, | ||
| }; | ||
| case 'kill-daemon': | ||
| await this.beginShutdownByKillingChildren( 'kill-daemon' ); |
There was a problem hiding this comment.
Splitting the shutdown method into beginShutdownByKillingChildren and finalizeShutdownByClosingSocketServersAndExiting isn't necessary for the core purpose of this PR: getting E2E tests to pass on Windows. It does help with correctness, though, because it makes the site stop --all command complete after the child processes are closed instead of before.
There was a problem hiding this comment.
We should eventually revert this change, but I'm personally open to landing this PR as is and reverting this once WordPress/php-toolkit#265 has landed and we have a new php-toolkit release.
| private async finalizeShutdownByClosingSocketServersAndExiting(): Promise< void > { | ||
| await this.controlServer.close(); | ||
| await this.eventsServer.close(); | ||
| process.exit( 0 ); |
There was a problem hiding this comment.
I have no indication that the process manager daemon didn't die after executing site stop --all, but some defensiveness here doesn't hurt.
|
The ~250MB increase in installer size must be some kind of fluke… Let's see what CI reports now that I've merged trunk again. EDIT: Confirmed that this was the case. |
fredrikekelund
changed the title
native-php E2E tests pass on Windowsnative-php E2E tests pass on Windows
| // maps to TerminateProcess of a single PID, so neither cleanup nor tree-walk runs. | ||
| // Closing the IPC channel triggers the wrapper's 'disconnect' handler instead, which | ||
| // kills the PHP child and exits cleanly. Force escalation falls back to taskkill /T. | ||
| if ( managedProcess.child.connected ) { |
There was a problem hiding this comment.
Should we add a disconnect handler to playground child as well?
There was a problem hiding this comment.
We always call child.kill() anyway, so there's no need for Playground child processes to handle the disconnect event (the only logical way for them to handle it would be to exit, as I see it). With native PHP, this approach is necessary for the child process to "forward" the kill signal to the PHP process.
3 participants
Related issues
How AI was used in this PR
Codex was used to help diagnose race condition issues in
collectDirectoryMetadataand to fix an issue inblueprints.phargiven error logs from a local Windows VM. I also used it to add thenative-phpE2E testing config for Buildkite. Claude was used to debug the persistently failing E2E tests and came up with the OPcache directory fix.Proposed Changes
php-server-child.tson Windows, too.blueprints.pharwith a bugfix needed to makeblueprints.test.tspass. This can be removed when CloseProcessOutputStreaminRuntime::eval_php_code_in_subprocessWordPress/php-toolkit#265 has landed and there's a new php-toolkit release. I advise landing this PR first and reverting this change later.collectDirectoryMetadatamore robust against race conditions.Testing Instructions
E2E tests should pass on both platforms.
Pre-merge Checklist