[PHP] Fix "Controller is already closed" crash in CLI server#3441
[PHP] Fix "Controller is already closed" crash in CLI server#3441
Conversation
When an HTTP client disconnects while a PHP request is still in progress, the stream pipeline cancels the ReadableStream. This cancellation propagates to the controller, putting it in a closed state. When PHP then finishes executing, the finally block in #executeWithErrorHandling tries to call controller.close() on the already-cancelled controller, which throws "Invalid state: Controller is already closed" and crashes the Node host process. The fix wraps all stream controller operations (close, error, enqueue) in try-catch so that a cancelled stream never crashes the host. This applies to three locations: the main request handler's finally block, the WASM crash error path, and the subprocess data callbacks. The portToStream fallback in api.ts gets the same treatment for environments that don't support transferable streams.
There was a problem hiding this comment.
Pull request overview
Fixes a Node CLI server crash caused by attempting to operate on ReadableStreamDefaultControllers after the consumer has cancelled the stream (e.g., client disconnect), resulting in "Invalid state: Controller is already closed".
Changes:
- Add
safeStreamError()/safeStreamClose()helpers and use them in PHP execution error/finally paths. - Guard child-process stdout/stderr stream enqueues and errors against cancellation.
- Wrap
portToStream()controller operations in a try/catch and add a regression test reproducing the cancellation-before-exit scenario.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/php-wasm/universal/src/lib/php.ts | Adds safe controller close/error helpers and applies them to crash/finally and child process stream handling. |
| packages/php-wasm/universal/src/lib/api.ts | Wraps ReadableStream controller operations in portToStream() to avoid exceptions after cancellation. |
| packages/php-wasm/node/src/test/php-crash.spec.ts | Adds a regression test for cancellation before PHP exits. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| spy.mockImplementation((c_func) => { | ||
| if (c_func === 'wasm_sapi_handle_request') { | ||
| // Simulate PHP writing some output | ||
| php[__private__dont__use].onStdout?.( | ||
| new Uint8Array([72, 101, 108, 108, 111]) | ||
| ); | ||
| // Return a promise we control so we can cancel | ||
| // the stream before PHP "exits". | ||
| return new Promise((resolve) => { | ||
| resolveExecution = resolve; | ||
| }); | ||
| } | ||
| }); |
There was a problem hiding this comment.
This mock returns undefined for any ccall other than wasm_sapi_handle_request. That can make the test brittle (or accidentally change behavior) if runStream() relies on other ccall results during setup/teardown. Prefer delegating to the original ccall for all other c_func values (store the original before spying), and only override the one function needed for the scenario.
When a mu-plugin writes output (triggering closeHeadersStream via onStdout) and then a WASM crash occurs, the error handler tried to call headers.controller.error() on the already-closed headers controller. The safeStreamError helper now handles this gracefully. This test verifies the crash surfaces through the stream as a catchable error rather than crashing the Node host.
When enqueue() fails on a child process stream, detach the data listener instead of silently dropping chunks in a loop. In portToStream, use safeStreamClose/safeStreamError for the close and error message types so we don't lose error info in a blanket catch. Document why we swallow errors in each case: the consumer already has the terminal state, and re-throwing would crash the Node process for no benefit. Also fix TS7030 in test mocks by adding explicit return values.
Summary
This PR fixes two related Node-side crash paths in PHP-WASM that could bring down the Playground CLI server instead of failing a single request cleanly.
The first crash happened when PHP had already produced output, which closes the headers stream, and then execution failed afterwards. In that case, Node could still try to
error()aReadableStreamcontroller that had already been closed, which threw:The validated repro for that path is:
closeHeadersStream()closes the headers controllerA simple
echo ...; exit;by itself is not the exact repro.The second crash happened when a stream consumer cancelled the response stream before PHP finished, for example because the HTTP pipeline completed or the client disconnected. In that case, the runtime could still try to
close()an already-cancelled controller and hit the sameController is already closedfailure.The third crash path was in the
proc_open()bridge used by the Node PHP runtime. If a spawned process timed out or exited while Node still had buffered stdout/stderr chunks to forward, the generatedphp.jsbridge could try to write into a PIPEFS stream that PHP had already closed. That crashed the host with an error like:Example:
What changed
off()to the sharedEventEmitterinterface so the child-process typing matches the APIs used by the runtime cleanup logic.asyncifyandjspi, PHP 7.4 through 8.5) from the updated build source.Test plan
CI