fix(launcher): resolve close on process 'exit' rather than stdio 'close'#41211
fix(launcher): resolve close on process 'exit' rather than stdio 'close'#41211danymarques wants to merge 1 commit into
Conversation
Resolve launchProcess cleanup on the child process 'exit' event (run once, with 'close' as a\nfallback) instead of waiting for 'close'. The 'close' event waits for stdio EOF, which a grandchild that inherits the browser's stdio pipe\n— such as msedge's EdgeUpdater — can hold open for ~20s after the browser has exited, delaying\nbrowser.close(). Add a launchProcess regression test covering this.
| // example msedge spawning EdgeUpdater — can keep the pipe open long after the | ||
| // browser process exits, which would otherwise delay close() until that grandchild | ||
| // exits. 'exit' always precedes 'close', so run the handler at most once. | ||
| const onProcessGone = (exitCode: number | null, signal: NodeJS.Signals | null) => { |
There was a problem hiding this comment.
there are almost always regressions whenever this code is modified as it is used by everything
typically we use launch flags to disable stuff like this, so perhaps we're not doing that correctly or something changed recently in Edge?
Test results for "MCP"1 failed 7265 passed, 1119 skipped Merge workflow run. |
Test results for "tests 1"6 failed 4 flaky39500 passed, 774 skipped Merge workflow run. |
2 participants
Resolve launchProcess cleanup on the child process 'exit' event (run once, with 'close' as a fallback) instead of waiting for 'close'.
The 'close' event waits for stdio EOF, which a grandchild that inherits the browser's stdio pipe — such as msedge's EdgeUpdater — can hold open for ~20s after the browser has exited, delaying browser.close(). Add a launchProcess regression test covering this.
#41210