Repository navigation
fix(embed): launch the base pythonw for uv venv trampolines on Windows (#4466) - #4478
Merged
Merged
Conversation
#4466) A uv venv's Scripts/pythonw.exe is not an interpreter: it is a small trampoline that CreateProcess's the base interpreter recorded in pyvenv.cfg, and that relaunch lands on the CUI python.exe. Our DETACHED_PROCESS flags only ever applied to the trampoline, not to the process the trampoline went on to spawn, so the console window they were meant to prevent appeared anyway on every daemon start. Resolve the trampoline ourselves and launch the base pythonw.exe, with the venv's site-packages on PYTHONPATH so hindsight_api stays importable from outside the venv. Two guards keep this narrow: - It fires only when pyvenv.cfg carries uv's own "uv =" marker. A stdlib venv's pythonw.exe is the GUI venvwlauncher, which already redirects to the base *pythonw*, so bypassing it would drop the daemon out of its venv for no gain. - If the env is not writable or site-packages is missing, the original stub is returned. A daemon that will not start is worse than a console flash. _windows_gui_interpreter keeps returning str | None; env is threaded in as an optional out-param, so no existing caller or test changes. Not fixed here: control_center/lifecycle.py spawns with a bare sys.executable and never consults _windows_gui_interpreter, so the control center has its own console-flash path. Pre-existing and independent of this issue.
- Annotate `candidates` as list[Path]; it had no type hint. - Drop the 2-tuple return from the `_windows_launcher_venv` test helper — the same rule the production change had to satisfy. It returns the Scripts dir, and `_base_pythonw()` names the base interpreter. - Cover the site-packages-missing fallback. Bypassing the trampoline is only safe when we can put the venv's imports back on PYTHONPATH; that guard decides whether a failed resolution degrades to a console flash or to a daemon that never starts, and nothing tested it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #4466.
Root cause
The issue reports that
_start_daemon_locked()bypasses_windows_gui_interpreter(). It doesn't — it reaches it through_find_api_command(), and the resolution works. The problem is what it resolves to.A uv venv's
Scripts\pythonw.exeis not an interpreter. It is a ~45KB trampoline thatCreateProcess's the base interpreter recorded inpyvenv.cfg, and that relaunch lands on the CUIpython.exe. OurDETACHED_PROCESSflags only ever applied to the trampoline, never to the process the trampoline went on to spawn — which is exactly the grandchild in the reported process tree:So we correctly pick a GUI-subsystem launcher and it hands the work to a console one.
I also ruled out the other candidate:
hindsight_api/daemon.pyre-execs viasys.executable, which would be the CUIpython.exe, but it returns early onwin32before the re-exec. Not involved.Fix
Resolve the trampoline ourselves and launch the base
pythonw.exe, with the venv'ssite-packagesonPYTHONPATHsohindsight_apistill imports from outside the venv.Two guards keep it narrow:
pyvenv.cfgcarries uv's ownuv =marker. A stdlib venv'spythonw.exeis the GUIvenvwlauncher, which already redirects to the base pythonw — bypassing that would drop the daemon out of its venv for no gain.site-packagesis missing, the original stub is returned. A daemon that won't start is worse than a console window._windows_gui_interpreterkeeps returningstr | None;envis threaded in as an optional out-param, so no existing caller or test changed.Relationship to #3103
PR #3103 was closed, not merged — contrary to the issue's note that it shipped in v0.10.0. Its code is not on main (
_resolve_real_pythonw/_inject_venv_pathsexist nowhere). The 0.10.0 changelog line "The embedded daemon no longer flashes console windows on Windows" refers to commit3f14672ea, which only touched TypeScript (hindsight-all-npm,coding-agents/src/core/daemon.ts) and is filed under 0.9.2.This PR differs from #3103 in two ways that matter:
uvmarker._windows_gui_interpreter's return type, forcing every caller to unpack. The project bans multi-item tuple returns outright, "not even for internal/private functions". This uses a frozenUvTrampolinedataclass.I also skipped #3103's
.pth-file parsing: it existed to makepywintypesimportable, and there is no pywin32 anywhere in this repo. Plainsite-packagesonPYTHONPATHis enough for our dependencies.Testing
lint.shandtyclean.Caveat worth a reviewer's attention: I could not verify this on Windows. The tests are fakes built from
tmp_path, not real trampolines, and the evidence that uv's pythonw stub relaunches CUIpython.exeis the process tree in the issue rather than something I reproduced. That assumption is what the whole fix rests on. Worth a manualwindows-smoke.ymldispatch before merge;test-embed-windowswill also run here.Not fixed here
control_center/lifecycle.py:167spawns with a baresys.executableand never consults_windows_gui_interpreter, so the control center has its own console-flash path. Pre-existing and independent of this issue — happy to fold it in if you'd rather fix both at once.