fix(photon): hide Windows sidecar console window - #65083
Conversation
|
This is ready for review. I confirmed the issue on Windows Desktop with Photon/iMessage enabled: the long-lived Photon sidecar keeps a visible blank Windows Terminal/conhost window open for node.exe. The sidecar process itself is expected, but the visible console is not. This patch keeps POSIX behavior unchanged and adds Windows creation flags plus hidden STARTUPINFO for the sidecar spawn. Validation: py_compile on the Photon adapter, git diff --check, and the targeted sidecar startup tests passed with 2 passed / 4 deselected. |
|
Thanks for the pointer. I agree #55554 is the earlier PR for the same Photon sidecar window issue. The main differences here are: (1) this PR includes regression coverage for the Windows sidecar spawn kwargs, which the sweeper review on #55554 requested, and (2) it adds hidden STARTUPINFO in addition to process creation flags. On my Windows Desktop setup with Photon enabled, the sidecar node.exe was still getting a visible Windows Terminal/conhost window, so the extra STARTUPINFO coverage is intended to keep the sidecar background-only. Happy to close this in favor of #55554 if maintainers prefer that route, or this can serve as the tested variant/reference for the missing coverage. |
teknium1
left a comment
There was a problem hiding this comment.
Thanks for adding focused Windows regression coverage. The current-main premise is valid: Photon starts the patch Node process at plugins/platforms/photon/adapter.py:948-958 and the persistent sidecar at plugins/platforms/photon/adapter.py:969-976 without no-window flags.
Problems
- The new sidecar kwargs combine
DETACHED_PROCESSwithstdin=PIPE/stdout=PIPE(plugins/platforms/photon/adapter.py:1002in this PR). The repository's compatibility contract saysDETACHED_PROCESSsevers stdio and breaks stdout capture (hermes_cli/_subprocess_compat.py:194-197), conflicting with the supervisor and EOF shutdown design. - The patch-script Node invocation remains unprotected at
plugins/platforms/photon/adapter.py:948-958. Related PR #55554 applieswindows_hide_flags()to both calls.
Suggested changes
- Use
windows_hide_flags()/CREATE_NO_WINDOWfor both spawn sites, preserving the existing pipe lifecycle. - Adjust the test to cover both subprocess calls and assert the hide flag, not detach flags.
Automated hermes-sweeper review.
| stderr=subprocess.STDOUT, | ||
| env=env, | ||
| start_new_session=(sys.platform != "win32"), | ||
| **_sidecar_popen_kwargs(), |
There was a problem hiding this comment.
DETACHED_PROCESS conflicts with this sidecar's stdin=PIPE/stdout=PIPE lifecycle. hermes_cli/_subprocess_compat.py:194-197 documents that it severs stdio and breaks stdout capture; use the established windows_hide_flags()/CREATE_NO_WINDOW value instead.
|
Thanks for the fix — this was a real bug, confirmed on main. It was independently fixed by five contributors; we merged the earliest submission (#54565 by @lEWFkRAD, Jun 29) via PR #65441, which uses the repo's shared |
Summary
start_new_session=TrueWhy
When Photon messages are enabled from Hermes Desktop on Windows, the sidecar is expected to keep running in the background. Today it can also create a visible blank Windows Terminal/conhost window for
node.exe, which is distracting and looks like an app error.start_new_sessiondoes not detach or hide console windows on Windows, so the sidecar needs explicit Windows process flags and hiddenSTARTUPINFO.Validation
python -m py_compile plugins/platforms/photon/adapter.pygit diff --checkuv run --with pytest --with pytest-asyncio python -m pytest tests/plugins/platforms/photon/test_sidecar_lifecycle.py -k "start_sidecar"Note: the full
test_sidecar_lifecycle.pyfile currently has unrelated Windows failures in the orphan-reaping tests, which expect signal behavior that is not exercised by this patch. The two sidecar startup tests pass.