From 0c89bfba124789b47663a5ef92061cf4dfa2b1f7 Mon Sep 17 00:00:00 2001 From: Ricardo Boni Date: Mon, 31 Aug 2026 19:20:24 -0400 Subject: [PATCH 1/3] fix(installer): sweep the standalone processes the old selector could not see A Ctrl+C in pip mode on Windows left 2 of 23 processes running -- both Streamlit, holding port 8501. Two separate reasons, and fixing either alone changes nothing. The sweep could not see them. On Windows it matched the image name `testgen.exe`, but every child is `python`. On POSIX it matched `testgen.*run-app`, but `run_ui` spawns the UI as `python -m streamlit run .../testgen/ui/app.py`, which carries no `run-app` -- and `start_new_session` puts it outside the group `killpg` targets. Match on the uv tool environment path instead: it appears in argv[0] of every child and in postgres' own command line, so it identifies the install without depending on what any process is called. Separators are normalised, since postgres writes its command line with forward slashes and the python children with back. The Windows clause is generated from the same patterns so the platforms cannot drift. The sweep was also never reached. `stop_app_tree` returned early on `proc.poll() is not None`, and on Windows the parent is usually gone by then: a console Ctrl+C reaches every process in the console's group, and the app is spawned without CREATE_NEW_PROCESS_GROUP (`start_new_session` is POSIX-only), so it dies with us. The UI does not -- `run_ui` gives it a group of its own. Run the force-kill path on Windows regardless of whether the parent is alive; a dead parent there says nothing about its children. POSIX keeps the early return, since there the parent forwards the signal before exiting. Windows enumerates and kills each match by PID rather than `taskkill /T`, which walks the tree as it stands -- killing a parent re-parents its children mid-walk and they escape. A failed sweep now logs: PowerShell can be absent or blocked by policy, and silence there left no trace of why the next start found the port taken. Verified on both platforms with real process trees. Linux, via a container: 16 orphans after a SIGKILLed installer, 0 after. Windows, on a real console: 2 survivors before, 0 after. Test fixtures use the command lines captured from both. Co-Authored-By: Claude Opus 5 (1M context) --- dk-installer.py | 105 +++++++++++++++++++------ tests/test_orphan_sweep.py | 155 +++++++++++++++++++++++++++++++++++++ tests/test_tg_start.py | 36 +++++++++ 3 files changed, 271 insertions(+), 25 deletions(-) create mode 100644 tests/test_orphan_sweep.py diff --git a/dk-installer.py b/dk-installer.py index 8429c9b..87ffe32 100755 --- a/dk-installer.py +++ b/dk-installer.py @@ -2643,17 +2643,21 @@ def stop_app_tree(proc: subprocess.Popen, timeout: int = 10) -> bool: running job may need to reach a checkpoint first. A second Ctrl+C during that wait is taken as "stop now" and skips straight to the force-kill. - Returns ``True`` when the tree exited within ``timeout`` (or was already - gone), ``False`` when it had to be force-killed. See ``supports_graceful_stop`` - for why Windows always reports ``False`` when there was a live process. + Returns ``True`` when the tree exited within ``timeout`` (or was already gone on a + platform where the parent tears its own tree down), ``False`` otherwise. Windows always + reports ``False``: it force-kills, and it cannot tell a stopped tree from a dead parent + with live children. See ``supports_graceful_stop``. """ - if proc.poll() is not None: - return True - if not supports_graceful_stop(): + # Not conditioned on proc still running: a console Ctrl+C kills the parent along with + # us, while the UI -- which run_ui spawns into its own group -- never sees it. A dead + # parent here says nothing about its children. force_kill_app_tree(proc, timeout=timeout) return False + if proc.poll() is not None: + return True + with contextlib.suppress(Exception): os.killpg(os.getpgid(proc.pid), signal.SIGTERM) try: @@ -2668,6 +2672,46 @@ def stop_app_tree(proc: subprocess.Popen, timeout: int = 10) -> bool: return True +# Command-line patterns identifying every process a standalone install spawns. +# +# Both are needed. ``testgen.*run-app`` misses the UI, which ``run_ui`` spawns as +# ``python -m streamlit run .../testgen/ui/app.py`` -- no ``run-app`` in it, and in a session +# of its own so killpg cannot reach it either. The tool-environment path catches that, and +# postgres. Matching on an image name cannot work at all: every child is ``python``. +STANDALONE_PROC_PATTERNS = ( + r"testgen.*run-app", + r"tools[/\\]dataops-testgen", +) + +# Separators are normalised first: postgres writes its command line with forward slashes, +# the python children with backslashes. Substituted rather than str.format-ed -- PowerShell +# is brace-heavy and every brace would need escaping. +_WINDOWS_ORPHAN_SWEEP = r""" +$ErrorActionPreference = 'SilentlyContinue' +$spare = @(PIDS_TO_SPARE) + $PID +Get-CimInstance Win32_Process | Where-Object { + $cmd = ($_.CommandLine -replace '\\', '/') + $exe = ($_.ExecutablePath -replace '\\', '/') + $spare -notcontains $_.ProcessId -and (MATCH_CLAUSE) +} | ForEach-Object { Stop-Process -Id $_.ProcessId -Force } +""" + + +def _windows_sweep_script(spare_pid: int) -> str: + """The sweep, with its match clause built from ``STANDALONE_PROC_PATTERNS``. + + Generated rather than spelled out in the script so the two platforms cannot drift: + Windows is the platform this sweep exists for, and a pattern edited in only one place + would silently reintroduce the leak. Separators are already normalised to ``/`` by the + script, so the ``[/\\]`` class collapses to a plain slash. + """ + clause = " -or ".join( + f"($cmd -match '{pattern}') -or ($exe -match '{pattern}')" + for pattern in (p.replace(r"[/\\]", "/") for p in STANDALONE_PROC_PATTERNS) + ) + return _WINDOWS_ORPHAN_SWEEP.replace("MATCH_CLAUSE", clause).replace("PIDS_TO_SPARE", str(spare_pid)) + + def stop_standalone_orphans() -> None: """Best-effort kill of orphan ``testgen`` + embedded ``postgres`` processes left over from a previous dirty exit. @@ -2677,11 +2721,12 @@ def stop_standalone_orphans() -> None: only logs when something is actually killed. Postgres is targeted by PID via ``/postmaster.pid`` so a user's - other Postgres installs aren't touched. ``testgen.exe`` is targeted by - image name on Windows — the installer itself is ``dk-installer.exe``, - so there's no risk of self-kill. Killing ``testgen.exe`` before - ``uv tool uninstall`` also matters on Windows: a running .exe holds an - exclusive file lock, so ``uv`` would otherwise fail to delete the binary. + other Postgres installs aren't touched. Everything else is matched on its + command line (see ``STANDALONE_PROC_PATTERNS``); the installer's own argv + matches neither pattern, so there's no risk of self-kill. Reaching the UI + matters on Windows especially: it holds the tool environment's ``python.exe`` + open, so ``uv tool uninstall`` cannot delete it and ``tg delete`` leaves the + install behind. """ # Outer guard so a transient filesystem/permission glitch in this best-effort # cleanup can never crash the install or delete flow. @@ -2708,27 +2753,37 @@ def stop_standalone_orphans() -> None: os.kill(postgres_pid, signal.SIGKILL) if is_windows: - # Image-name match — covers any leftover `testgen run-app` parents. - # `/T` propagates to their children (UI/scheduler/server subprocesses). + # Kill each match by PID, not ``taskkill /T``: that walks the tree as it stands, + # so killing a parent re-parents its children mid-walk and they escape. with contextlib.suppress(Exception): - subprocess.run( - ["taskkill", "/F", "/T", "/IM", "testgen.exe"], + result = subprocess.run( + [ + "powershell", + "-NoProfile", + "-NonInteractive", + "-Command", + _windows_sweep_script(os.getpid()), + ], stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL, creationflags=getattr(subprocess, "CREATE_NO_WINDOW", 0), check=False, ) + if result.returncode != 0: + # PowerShell can be absent or blocked by policy. Without this the sweep + # does nothing and leaves no trace of why the next start found the port taken. + LOG.warning("Orphan sweep exited %s; leftover processes may remain", result.returncode) else: - # `pkill -f` matches against the full command line. The installer's own - # argv is `python dk-installer.py …` — doesn't contain `run-app`, so - # no self-kill risk. - with contextlib.suppress(Exception): - subprocess.run( - ["pkill", "-9", "-f", r"testgen.*run-app"], - stdout=subprocess.DEVNULL, - stderr=subprocess.DEVNULL, - check=False, - ) + # ``pkill -f`` matches against the full command line. The installer's own + # argv is ``python dk-installer.py ...`` -- matches neither pattern. + for pattern in STANDALONE_PROC_PATTERNS: + with contextlib.suppress(Exception): + subprocess.run( + ["pkill", "-9", "-f", pattern], + stdout=subprocess.DEVNULL, + stderr=subprocess.DEVNULL, + check=False, + ) except Exception: LOG.exception("Unexpected error during orphan cleanup; continuing") diff --git a/tests/test_orphan_sweep.py b/tests/test_orphan_sweep.py new file mode 100644 index 0000000..43b6c7a --- /dev/null +++ b/tests/test_orphan_sweep.py @@ -0,0 +1,155 @@ +"""Selector coverage for ``stop_standalone_orphans``. + +The command lines below are captured from real standalone installs, only the user +directory renamed. The image-name match this replaced could see the ``testgen.exe`` +shim and nothing else: every child is ``python``. +""" + +import re +from unittest.mock import patch + +import pytest + +from tests.installer import STANDALONE_PROC_PATTERNS, stop_standalone_orphans + +TOOLS = r"D:\Users\dev\AppData\Roaming\uv\tools\dataops-testgen" + +# Every process the install spawned, by role. +SPAWNED = { + "shim": r"D:\Users\dev\.local\bin\testgen.exe run-app", + "shim-trampoline": rf'"{TOOLS}\Scripts\python.exe" "D:\Users\dev\.local\bin\testgen.exe" run-app', + "ui": rf"{TOOLS}\Scripts\python.exe -m testgen run-app ui", + "scheduler": rf"{TOOLS}\Scripts\python.exe -m testgen run-app scheduler", + "server": rf"{TOOLS}\Scripts\python.exe -m testgen run-app server", + # The one that survived: no "run-app" anywhere, and spawned into its own session. + "streamlit": ( + rf"{TOOLS}\Scripts\python.exe -m streamlit run " + rf"{TOOLS}\Lib\site-packages\testgen\ui/app.py --server.port=8501" + ), + # postgres writes forward slashes, and hangs off a detached cmd.exe whose parent has + # already exited -- so no tree walk reaches it. + "postgres-wrapper": ( + r'"C:\Windows\system32\cmd.exe" /C ""D:/Users/dev/AppData/Roaming/uv/tools/' + r'dataops-testgen/Lib/site-packages/pixeltable_pgserver/pginstall18/bin/postgres.exe" ' + r'-D "D:/Users/dev/.testgen/pgdata" -h "127.0.0.1" -p 57028"' + ), + "postgres": ( + r'"D:/Users/dev/AppData/Roaming/uv/tools/dataops-testgen/Lib/site-packages/' + r'pixeltable_pgserver/pginstall18/bin/postgres.exe" -D "D:/Users/dev/.testgen/pgdata"' + ), + "postgres-forkchild": ( + r'"D:/Users/dev/AppData/Roaming/uv/tools/dataops-testgen/Lib/site-packages/' + r'pixeltable_pgserver/pginstall18/bin/postgres.exe" --forkchild="bgworker" 5720' + ), +} + +# The same tree on Linux. Kept alongside the Windows set because the POSIX sweep matches +# the same patterns via `pkill -f`. +POSIX_TOOLS = "/home/tester/.local/share/uv/tools/dataops-testgen" +SPAWNED_POSIX = { + "shim": f"{POSIX_TOOLS}/bin/python /home/tester/.local/bin/testgen run-app", + "ui": f"{POSIX_TOOLS}/bin/python -m testgen run-app ui", + "scheduler": f"{POSIX_TOOLS}/bin/python -m testgen run-app scheduler", + "server": f"{POSIX_TOOLS}/bin/python -m testgen run-app server", + "streamlit": ( + f"{POSIX_TOOLS}/bin/python -m streamlit run " + f"{POSIX_TOOLS}/lib/python3.13/site-packages/testgen/ui/app.py --server.port=8501" + ), + "postgres": ( + f"{POSIX_TOOLS}/lib/python3.13/site-packages/pixeltable_pgserver/pginstall18/bin/postgres " + f"-D /home/tester/.testgen/pgdata -h -k /home/tester/.testgen/pgdata" + ), +} + +# Must survive the sweep: killing either of these is killing ourselves. +SPARED = { + "installer-exe": r'"D:\Users\dev\Downloads\dk-installer.exe" tg start', + "installer-py": "python3 dk-installer.py tg install --pip", +} + + +def matches(command_line): + return any(re.search(p, command_line) for p in STANDALONE_PROC_PATTERNS) + + +@pytest.mark.unit +@pytest.mark.parametrize("role", sorted(SPAWNED)) +def test_sweep_matches_every_spawned_process(role): + assert matches(SPAWNED[role]), f"{role} would be left running" + + +@pytest.mark.unit +@pytest.mark.parametrize("role", sorted(SPARED)) +def test_sweep_spares_the_installer(role): + assert not matches(SPARED[role]) + + +@pytest.mark.unit +@pytest.mark.parametrize("role", sorted(SPAWNED_POSIX)) +def test_sweep_matches_every_spawned_process_on_posix(role): + assert matches(SPAWNED_POSIX[role]), f"{role} would be left running" + + +@pytest.mark.unit +def test_streamlit_needs_the_tool_env_pattern(): + """The regression that motivated this: the UI carries no 'run-app', so the original + pattern missed it -- and it is the process holding port 8501.""" + for cmdline in (SPAWNED["streamlit"], SPAWNED_POSIX["streamlit"]): + assert not re.search(r"testgen.*run-app", cmdline) + assert matches(cmdline) + + +@pytest.mark.unit +def test_posix_sweep_runs_every_pattern(tmp_path): + with ( + patch("tests.installer.platform.system", return_value="Linux"), + patch("tests.installer.pathlib.Path.home", return_value=tmp_path), + patch("tests.installer.subprocess.run") as run_mock, + ): + stop_standalone_orphans() + + patterns = [c.args[0][-1] for c in run_mock.call_args_list if c.args[0][0] == "pkill"] + assert patterns == list(STANDALONE_PROC_PATTERNS) + + +@pytest.mark.unit +def test_windows_sweep_spares_the_installer_pid(tmp_path): + with ( + patch("tests.installer.platform.system", return_value="Windows"), + patch("tests.installer.pathlib.Path.home", return_value=tmp_path), + patch("tests.installer.os.getpid", return_value=4242), + patch("tests.installer.subprocess.run") as run_mock, + ): + stop_standalone_orphans() + + script = next(c.args[0][-1] for c in run_mock.call_args_list if c.args[0][0] == "powershell") + assert "@(4242)" in script + assert "PIDS_TO_SPARE" not in script # substitution actually happened + # Kills by PID rather than walking a tree that re-parents mid-teardown. + assert "Stop-Process -Id" in script + assert "/T" not in script + + +@pytest.mark.unit +def test_windows_sweep_no_longer_matches_by_image_name(tmp_path): + """`taskkill /IM testgen.exe` could not see the python children at all.""" + with ( + patch("tests.installer.platform.system", return_value="Windows"), + patch("tests.installer.pathlib.Path.home", return_value=tmp_path), + patch("tests.installer.subprocess.run") as run_mock, + ): + stop_standalone_orphans() + + assert not any("/IM" in str(c.args[0]) for c in run_mock.call_args_list) + + +@pytest.mark.unit +def test_sweep_never_raises(tmp_path): + """Best-effort cleanup: it runs on the install and delete paths, and must not be able + to crash either.""" + with ( + patch("tests.installer.platform.system", return_value="Linux"), + patch("tests.installer.pathlib.Path.home", return_value=tmp_path), + patch("tests.installer.subprocess.run", side_effect=OSError("boom")), + ): + stop_standalone_orphans() # must not raise diff --git a/tests/test_tg_start.py b/tests/test_tg_start.py index a798764..90795d7 100644 --- a/tests/test_tg_start.py +++ b/tests/test_tg_start.py @@ -455,3 +455,39 @@ def _assert_any_msg_contains(text: str): mock.assert_any_msg_contains = _assert_any_msg_contains yield mock + + +@pytest.mark.unit +def test_windows_sweeps_even_when_the_parent_is_already_gone(): + """A console Ctrl+C reaches everything in the console's process group, so the parent is + usually dead before we are called -- while the UI, spawned into its own group, is not. + Returning early on a dead parent is what left Streamlit holding port 8501.""" + proc = MagicMock() + proc.poll.return_value = 0 # parent already exited + proc.pid = 4242 + + with ( + patch("tests.installer.platform.system", return_value="Windows"), + patch("tests.installer.subprocess.run"), + patch("tests.installer.stop_standalone_orphans") as sweep_mock, + ): + assert stop_app_tree(proc, timeout=90) is False + + sweep_mock.assert_called_once() + + +@pytest.mark.unit +def test_posix_still_trusts_a_dead_parent(): + """On POSIX the parent forwards the signal to its children before exiting, so a dead one + really does mean a stopped tree -- no need to reach for pkill on every clean stop.""" + proc = MagicMock() + proc.poll.return_value = 0 + proc.pid = 4242 + + with ( + patch("tests.installer.platform.system", return_value="Linux"), + patch("tests.installer.stop_standalone_orphans") as sweep_mock, + ): + assert stop_app_tree(proc, timeout=90) is True + + sweep_mock.assert_not_called() From dd3a60a0eeb7d3f7109411d6296983d853de9012 Mon Sep 17 00:00:00 2001 From: Ricardo Boni Date: Mon, 31 Aug 2026 19:20:40 -0400 Subject: [PATCH 2/3] fix(installer): stop tg delete reporting a success it did not verify On Windows `tg delete` printed "TestGen uninstalled." while leaving the uv tool environment and the `testgen` entry point on disk. A leaked UI process held `/Scripts/python.exe` open, `uv tool uninstall` could not remove it, and the resulting error was caught and softened to "it may already be uninstalled" -- which is exactly backwards. It had not been uninstalled at all. Check the disk instead of trusting the exit code, and name what survived: TestGen was only partly uninstalled. These were left behind: ~/.local/share/uv/tools/dataops-testgen ~/.local/bin/testgen This usually means a TestGen process was still running and held a file open. Close any running TestGen, then run this command again. That retry has to be able to do something, so an incomplete uninstall now keeps the install marker -- dropping it makes the next `tg delete` report "nothing to delete" -- and keeps the installer-local uv, which the retry needs to uninstall with. The leftover check runs before the rest of the cleanup, so it tolerates any failure rather than only `CommandFailed`: uv can fail to spawn outright, and aborting there left more behind than doing nothing would. The pre-existing uninstall call is widened for the same reason. uv's bin directory is shared, commonly ~/.local/bin, so a `testgen` shim there is not necessarily ours. It is only reported alongside a surviving tool environment; on its own it would call a clean uninstall a failure and then strand the user behind the marker we now keep. Co-Authored-By: Claude Opus 5 (1M context) --- dk-installer.py | 72 ++++++++++++++++++++----- tests/test_tg_pip_delete.py | 103 ++++++++++++++++++++++++++++++++++++ 2 files changed, 161 insertions(+), 14 deletions(-) diff --git a/dk-installer.py b/dk-installer.py index 87ffe32..51aa604 100755 --- a/dk-installer.py +++ b/dk-installer.py @@ -3384,8 +3384,11 @@ def execute(self, args): if self._resolved_mode == INSTALL_MODE_DOCKER: self._delete_docker(args) - else: - self._delete_pip(args) + elif not self._delete_pip(args): + # Something survived, so the install is still here and the marker has to stay: + # dropping it makes the retry we just recommended report "nothing to delete". + LOG.info("Keeping the install marker -- uninstall was incomplete") + return InstallMarker(self.data_folder, args.prod, args.compose_file_name).unlink() def _delete_docker(self, args): @@ -3407,15 +3410,17 @@ def _delete_pip(self, args): stop_standalone_orphans() uv_path = resolve_uv_path(self.data_folder) + leftovers: list[str] = [] if uv_path: try: self.run_cmd(uv_path, "tool", "uninstall", TESTGEN_PIP_PACKAGE) - except CommandFailed: + except Exception: + # Not just CommandFailed: uv may fail to spawn at all. Everything below still + # needs to happen -- stopping here leaves more behind than doing nothing. LOG.exception("Failed to uninstall testgen via uv") - CONSOLE.msg( - "Note: 'uv tool uninstall testgen' reported an error " - "(it may already be uninstalled); see session logs." - ) + # Verify rather than trust the exit code: a process still holding a file in the + # tool environment makes the uninstall a silent no-op. + leftovers = surviving_tool_paths(self, uv_path) else: LOG.info("uv not found; skipping uv tool uninstall") CONSOLE.msg("uv not found; skipping 'uv tool uninstall testgen'.") @@ -3428,17 +3433,56 @@ def _delete_pip(self, args): # may have other Streamlit projects on this machine. The config dir # is tiny and harmless if left behind. - # Remove the installer-local uv binary if we downloaded one. A - # pre-existing uv on PATH is left alone. - local_uv = self.data_folder / UV_BIN_SUBDIR / ("uv.exe" if platform.system() == "Windows" else "uv") - if remove_path(local_uv, label="installer-local uv"): - with contextlib.suppress(OSError): - local_uv.parent.rmdir() + # Remove the installer-local uv binary if we downloaded one. A pre-existing uv on + # PATH is left alone. Kept when something survived: the retry we are about to + # recommend needs a uv to run `tool uninstall` with. + if not leftovers: + local_uv = self.data_folder / UV_BIN_SUBDIR / ("uv.exe" if platform.system() == "Windows" else "uv") + if remove_path(local_uv, label="installer-local uv"): + with contextlib.suppress(OSError): + local_uv.parent.rmdir() remove_path(self.data_folder / CREDENTIALS_FILE.format(args.prod)) CONSOLE.space() - CONSOLE.msg("TestGen uninstalled.") + if leftovers: + CONSOLE.msg("TestGen was only partly uninstalled. These were left behind:") + for path in leftovers: + CONSOLE.msg(f" {simplify_path(pathlib.Path(path))}") + CONSOLE.space() + CONSOLE.msg("This usually means a TestGen process was still running and held a file open.") + CONSOLE.msg("Close any running TestGen, then run this command again.") + else: + CONSOLE.msg("TestGen uninstalled.") CONSOLE.space() + return not leftovers + + +def surviving_tool_paths(action, uv_path: str) -> list: + """Parts of the uv tool install still on disk after an uninstall attempt. + + An empty list means the uninstall really did remove everything. Best-effort throughout: + this only decides what to *report*, and must never be the reason a delete stops early -- + everything after it in ``_delete_pip`` still needs to run. + """ + + def read(*args_): + try: + return (action.run_cmd(uv_path, *args_, capture_text=True) or "").strip() + except Exception: + LOG.info("Could not read 'uv %s'; skipping that leftover check", " ".join(args_)) + return "" + + survivors = [] + if (tool_dir := read("tool", "dir")) and (env := pathlib.Path(tool_dir) / TESTGEN_PIP_PACKAGE).exists(): + LOG.info("Uninstall left [%s] behind", env) + survivors.append(str(env)) + # Only alongside the environment: uv's bin directory is shared (commonly + # ~/.local/bin), so a shim there on its own may well be someone else's. + bin_name = "testgen.exe" if platform.system() == "Windows" else "testgen" + if (bin_dir := read("tool", "dir", "--bin")) and (shim := pathlib.Path(bin_dir) / bin_name).exists(): + LOG.info("Uninstall left [%s] behind", shim) + survivors.append(str(shim)) + return survivors class TestgenRunDemoAction(DemoContainerAction, ComposeActionMixin): diff --git a/tests/test_tg_pip_delete.py b/tests/test_tg_pip_delete.py index 9a1fd43..f44e85d 100644 --- a/tests/test_tg_pip_delete.py +++ b/tests/test_tg_pip_delete.py @@ -186,3 +186,106 @@ def test_delete_routes_to_docker_legacy(delete_action, args_mock, tmp_data_folde docker_branch.assert_called_once_with(args_mock) assert delete_action.analytics.additional_properties["install_mode"] == INSTALL_MODE_DOCKER + + +@pytest.mark.integration +def test_pip_delete_reports_what_survived(pip_delete_action, start_cmd_mock, stdout_mock, tmp_path, console_msg_mock): + """A process we failed to stop can hold a file in the tool environment open, making + `uv tool uninstall` a no-op. Reporting success then sends the user to a reinstall + that cannot work either -- so check the disk rather than trusting the exit code.""" + tool_dir, bin_dir = tmp_path / "tools", tmp_path / "bin" + tool_dir.mkdir() + bin_dir.mkdir() + (tool_dir / "dataops-testgen").mkdir() # survived the uninstall + (bin_dir / "testgen").write_text("#!/bin/sh\n") # so did the entry point + # uv tool uninstall, then `uv tool dir`, then `uv tool dir --bin` + stdout_mock.side_effect = [[], [str(tool_dir)], [str(bin_dir)]] + + with patch("tests.installer.shutil.which", return_value="/usr/local/bin/uv"): + pip_delete_action.execute() + + console_msg_mock.assert_any_msg_contains("only partly uninstalled") + console_msg_mock.assert_any_msg_contains("still running and held a file open") + printed = " ".join(str(c) for c in console_msg_mock.call_args_list) + assert "dataops-testgen" in printed + assert "TestGen uninstalled." not in printed + + +@pytest.mark.integration +def test_pip_delete_reports_success_when_nothing_survives( + pip_delete_action, start_cmd_mock, stdout_mock, tmp_path, console_msg_mock +): + tool_dir, bin_dir = tmp_path / "tools", tmp_path / "bin" + tool_dir.mkdir() + bin_dir.mkdir() + # uv tool uninstall, then `uv tool dir`, then `uv tool dir --bin` + stdout_mock.side_effect = [[], [str(tool_dir)], [str(bin_dir)]] + + with patch("tests.installer.shutil.which", return_value="/usr/local/bin/uv"): + pip_delete_action.execute() + + console_msg_mock.assert_any_msg_contains("TestGen uninstalled.") + printed = " ".join(str(c) for c in console_msg_mock.call_args_list) + assert "only partly uninstalled" not in printed + + +@pytest.mark.integration +def test_pip_delete_keeps_the_marker_when_incomplete( + pip_delete_action, start_cmd_mock, stdout_mock, tmp_data_folder, tmp_path, console_msg_mock +): + """The retry we recommend has to be able to do something. Dropping the marker makes the + next `tg delete` report "nothing to delete", and removing the installer-local uv leaves + it with nothing to uninstall with.""" + tool_dir, bin_dir = tmp_path / "tools", tmp_path / "bin" + tool_dir.mkdir() + bin_dir.mkdir() + (tool_dir / "dataops-testgen").mkdir() + local_uv = Path(tmp_data_folder) / "bin" / "uv" + local_uv.parent.mkdir(parents=True, exist_ok=True) + local_uv.write_text("#!/bin/sh\n") + stdout_mock.side_effect = [[], [str(tool_dir)], [str(bin_dir)]] + + with patch("tests.installer.shutil.which", return_value="/usr/local/bin/uv"): + pip_delete_action.execute() + + console_msg_mock.assert_any_msg_contains("only partly uninstalled") + assert (Path(tmp_data_folder) / INSTALL_MARKER_FILE.format("tg")).exists(), "marker was dropped" + assert local_uv.exists(), "installer-local uv was removed, so the retry has no uv" + + +@pytest.mark.integration +def test_pip_delete_ignores_a_foreign_shim( + pip_delete_action, start_cmd_mock, stdout_mock, tmp_data_folder, tmp_path, console_msg_mock +): + """uv's bin directory is shared (commonly ~/.local/bin). A `testgen` there when the tool + environment is gone is somebody else's -- reporting it would call a clean uninstall a + failure, and would then strand the user via the kept marker.""" + tool_dir, bin_dir = tmp_path / "tools", tmp_path / "bin" + tool_dir.mkdir() + bin_dir.mkdir() + (bin_dir / "testgen").write_text("#!/bin/sh\n") # not ours + stdout_mock.side_effect = [[], [str(tool_dir)], [str(bin_dir)]] + + with patch("tests.installer.shutil.which", return_value="/usr/local/bin/uv"): + pip_delete_action.execute() + + console_msg_mock.assert_any_msg_contains("TestGen uninstalled.") + assert not (Path(tmp_data_folder) / INSTALL_MARKER_FILE.format("tg")).exists() + + +@pytest.mark.integration +def test_pip_delete_survives_a_broken_uv(pip_delete_action, start_cmd_mock, stdout_mock, tmp_path, tmp_data_folder): + """The leftover check runs before the rest of the cleanup, so a uv that cannot be spawned + must not abort the delete and leave more behind than before.""" + fake_tg_home = tmp_path / ".testgen" + fake_tg_home.mkdir() + (fake_tg_home / "config.env").write_text("x=1\n") + start_cmd_mock.side_effect = PermissionError("blocked by security software") + + with ( + patch("tests.installer.shutil.which", return_value="/usr/local/bin/uv"), + patch.dict("tests.installer.os.environ", {"TG_TESTGEN_HOME": str(fake_tg_home)}), + ): + pip_delete_action.execute() + + assert not fake_tg_home.exists(), "delete stopped early and left the data directory" From 68f4c5dc37041e81694b44f78e640aea1535d0c5 Mon Sep 17 00:00:00 2001 From: Ricardo Boni Date: Mon, 31 Aug 2026 19:20:57 -0400 Subject: [PATCH 3/3] feat(installer): fail early when the embedded database cannot run A pip install on Windows failed inside `standalone-setup` with: initdb: error: program "postgres" is needed by initdb but was not found in the same directory as ".../initdb.exe" postgres.exe was present the whole time. initdb runs `postgres -V`, that launch failed, and initdb reported the failed launch as a missing file -- naming the one thing that was not wrong. (The cause was a pgserver build shipped without a library it links against, but the class is wider than that one bug.) Run `postgres -V` ourselves first. If the bundled binary will not run, the install cannot succeed, so stop and point at `--docker`. The exit code goes to the session log, where support can use it; the console gets the way forward instead, since neither an NT status code nor a library name helps the person installing. Under `upgrade` it says to delete first, because `tg install` refuses while an install marker exists and would otherwise walk the user into a refusal. Checking before `standalone-setup` matters for a second reason: a partial setup leaves `config.env` behind, and every retry then stops on an interactive "Overwrite?" prompt the installer has no terminal to answer. One failed install otherwise wedges every attempt after it. The check runs after the tool environment is built -- it has to, since the binary ships inside it -- so it saves seconds, not minutes. Its value is a legible failure and a clean retry, not speed. Guidance is emitted from `on_action_fail` rather than `execute`, which runs inside a partial console line. Skips rather than fails when the binary cannot be located: not finding it says nothing about whether it works. Co-Authored-By: Claude Opus 5 (1M context) --- dk-installer.py | 91 +++++++++++++++- tests/test_tg_pip_upgrade.py | 7 +- tests/test_verify_embedded_postgres.py | 143 +++++++++++++++++++++++++ 3 files changed, 237 insertions(+), 4 deletions(-) create mode 100644 tests/test_verify_embedded_postgres.py diff --git a/dk-installer.py b/dk-installer.py index 51aa604..f04768f 100755 --- a/dk-installer.py +++ b/dk-installer.py @@ -2907,6 +2907,87 @@ def on_action_success(self, action, args): CONSOLE.msg(f"Updated to v{new_version}.") +def find_embedded_postgres(action) -> typing.Optional[pathlib.Path]: + """Path to the ``postgres`` binary pixeltable-pgserver bundles, or None if not found. + + Only the two layouts uv creates are searched, rather than walking the whole tool + environment: site-packages is large and this runs on the install path. + """ + uv_path = action.ctx.get("uv_path") or resolve_uv_path(action.data_folder) + if uv_path is None: + return None + try: + tool_dir = action.run_cmd(uv_path, "tool", "dir", capture_text=True) + except CommandFailed: + return None + if not tool_dir: + return None + + env_dir = pathlib.Path(tool_dir.strip()) / TESTGEN_PIP_PACKAGE + for pattern in ( + "Lib/site-packages/pixeltable_pgserver/pginstall*/bin/postgres*", + "lib/python*/site-packages/pixeltable_pgserver/pginstall*/bin/postgres*", + ): + for candidate in sorted(env_dir.glob(pattern)): + if candidate.is_file(): + return candidate + return None + + +class TestgenVerifyEmbeddedPostgresStep(Step): + """Smoke-test the bundled postgres before anything tries to use it. + + A bundled build that cannot run on this machine surfaces deep inside ``standalone-setup``, + where ``initdb`` reports a failed *launch* of ``postgres`` as "program not found in the + same directory" -- pointing at the one thing that is not wrong. Observed with a + pixeltable-pgserver build shipped without a library it links against, but the cause does + not matter here: if the binary will not run, the install cannot succeed. + + Checking before ``standalone-setup`` also keeps a failure from wedging later attempts. + A partial setup leaves ``config.env`` behind, and every retry then stops on an + interactive "Overwrite?" prompt that the installer has no terminal to answer. + """ + + label = "Verifying the embedded database" + + def __init__(self): + self.unusable = False + super().__init__() + + def execute(self, action, args): + postgres_path = find_embedded_postgres(action) + if postgres_path is None: + # Not finding it says nothing about whether it works -- let the real step decide. + LOG.info("Embedded postgres binary not found; skipping the check") + raise SkipStep + + LOG.info("Verifying embedded postgres at [%s]", postgres_path) + try: + action.run_cmd(str(postgres_path), "-V") + except CommandFailed as e: + # The exit code is the diagnosis; keep it for the session zip, not the console. + LOG.warning("Embedded postgres [%s] failed to run: exit %s", postgres_path, e.ret_code) + self.unusable = True + raise AbortAction + + def on_action_fail(self, action, args): + # Not from execute: steps run inside a partial console line, so anything printed + # there lands mid-line with "FAILED" appended. + if not self.unusable: + return + CONSOLE.msg("The embedded PostgreSQL database cannot run on this machine.") + if action.args_cmd == "upgrade": + # `tg install` refuses while an install marker exists, so Docker is only + # reachable from an existing pip install by removing it first. + CONSOLE.msg(f"To move to Docker, {command_hint(args.prod, 'delete', 'Uninstall TestGen')} first,") + CONSOLE.msg(f"then {command_hint(args.prod, 'install --docker', 'Install TestGen')}.") + else: + CONSOLE.msg( + f"To install TestGen with Docker instead, " + f"{command_hint(args.prod, 'install --docker', 'Install TestGen')}." + ) + + class TestgenStandaloneSetupStep(Step): label = "Initializing TestGen" @@ -2984,7 +3065,13 @@ class TestgenInstallAction(ComposeActionMixin, AnalyticsMultiStepAction): prompts). """ - pip_steps = [UvBootstrapStep, UvToolInstallStep, TestgenStandaloneSetupStep, TestgenQuickStartStep] + pip_steps = [ + UvBootstrapStep, + UvToolInstallStep, + TestgenVerifyEmbeddedPostgresStep, + TestgenStandaloneSetupStep, + TestgenQuickStartStep, + ] docker_steps = [ ComposeVerifyExistingInstallStep, DockerNetworkStep, @@ -3188,7 +3275,7 @@ def execute(self, action, args): class TestgenUpgradeAction(ComposeActionMixin, AnalyticsMultiStepAction): """Upgrade an existing TestGen install. Mode is read from the install marker.""" - pip_steps = [UvBootstrapStep, UvToolUpgradeStep, TestgenStandaloneUpgradeStep] + pip_steps = [UvBootstrapStep, UvToolUpgradeStep, TestgenVerifyEmbeddedPostgresStep, TestgenStandaloneUpgradeStep] docker_steps = [ UpdateComposeFileStep, ComposeStopStep, diff --git a/tests/test_tg_pip_upgrade.py b/tests/test_tg_pip_upgrade.py index aa25dd7..3ab7b7a 100644 --- a/tests/test_tg_pip_upgrade.py +++ b/tests/test_tg_pip_upgrade.py @@ -37,12 +37,13 @@ def pip_upgrade_action(action_cls, args_mock, tmp_data_folder, start_cmd_mock): @pytest.mark.integration def test_tg_pip_upgrade_happy_path(pip_upgrade_action, start_cmd_mock, stdout_mock, tmp_data_folder, console_msg_mock): # Step pipeline: pre_execute uv tool list → execute uv --version - # (UvBootstrapStep records uv version) → uv tool upgrade → - # testgen upgrade-system-version → on_action_success uv tool list. + # (UvBootstrapStep records uv version) → uv tool upgrade → embedded-postgres + # check → testgen upgrade-system-version → on_action_success uv tool list. stdout_mock.side_effect = [ ["dataops-testgen v5.10.0", "- testgen"], ["uv 0.11.7"], [], + [], # `uv tool dir` for the embedded-postgres check (no binary -> skipped) [], ["dataops-testgen v5.10.0", "- testgen"], ] @@ -84,6 +85,7 @@ def test_tg_pip_upgrade_reports_version_change(pip_upgrade_action, start_cmd_moc ["dataops-testgen v5.10.0", "- testgen"], ["uv 0.11.7"], [], + [], # `uv tool dir` for the embedded-postgres check (no binary -> skipped) [], ["dataops-testgen v5.10.1", "- testgen"], ] @@ -105,6 +107,7 @@ def test_tg_pip_upgrade_marker_preserves_created_on(pip_upgrade_action, stdout_m ["dataops-testgen v5.10.0", "- testgen"], ["uv 0.11.7"], [], + [], # `uv tool dir` for the embedded-postgres check (no binary -> skipped) [], ["dataops-testgen v5.10.0", "- testgen"], ] diff --git a/tests/test_verify_embedded_postgres.py b/tests/test_verify_embedded_postgres.py new file mode 100644 index 0000000..ec1efb7 --- /dev/null +++ b/tests/test_verify_embedded_postgres.py @@ -0,0 +1,143 @@ +"""Coverage for the embedded-postgres smoke test. + +When the bundled postgres cannot load a library it links against, initdb reports it as +"program postgres is needed by initdb but was not found in the same directory" -- naming +the one thing that is not wrong. +""" + +from unittest.mock import MagicMock, patch + +import pytest + +from tests.installer import ( + AbortAction, + CommandFailed, + SkipStep, + TestgenInstallAction, + TestgenVerifyEmbeddedPostgresStep, + find_embedded_postgres, + TestgenStandaloneSetupStep, +) + +WINDOWS_LAYOUT = "Lib/site-packages/pixeltable_pgserver/pginstall18/bin/postgres.exe" +POSIX_LAYOUT = "lib/python3.13/site-packages/pixeltable_pgserver/pginstall18/bin/postgres" + + +@pytest.fixture +def action(tmp_path): + act = MagicMock() + act.ctx = {"uv_path": "/usr/local/bin/uv"} + act.data_folder = tmp_path + act.run_cmd.return_value = str(tmp_path) + return act + + +def place_binary(tmp_path, layout): + target = tmp_path / "dataops-testgen" / layout + target.parent.mkdir(parents=True) + target.write_text("#!/bin/sh\n") + return target + + +@pytest.mark.unit +@pytest.mark.parametrize("layout", (WINDOWS_LAYOUT, POSIX_LAYOUT)) +def test_finds_the_bundled_binary_in_either_layout(action, tmp_path, layout): + expected = place_binary(tmp_path, layout) + assert find_embedded_postgres(action) == expected + + +@pytest.mark.unit +def test_returns_none_when_not_installed(action): + assert find_embedded_postgres(action) is None + + +@pytest.mark.unit +def test_returns_none_when_uv_cannot_be_resolved(action): + action.ctx = {} + with patch("tests.installer.resolve_uv_path", return_value=None): + assert find_embedded_postgres(action) is None + + +@pytest.mark.unit +def test_step_skips_when_binary_is_absent(action, args_mock): + """Not finding it says nothing about whether it works -- the real step must still run.""" + with pytest.raises(SkipStep): + TestgenVerifyEmbeddedPostgresStep().execute(action, args_mock) + + +@pytest.mark.unit +def test_step_passes_when_postgres_reports_its_version(action, tmp_path, args_mock): + place_binary(tmp_path, POSIX_LAYOUT) + TestgenVerifyEmbeddedPostgresStep().execute(action, args_mock) # must not raise + + +@pytest.mark.unit +@pytest.mark.parametrize( + "layout, ret_code", + ( + # Windows reports STATUS_DLL_NOT_FOUND as an unsigned exit status; any other + # non-zero exit means the same thing to the person installing. + (WINDOWS_LAYOUT, 0xC0000135), + (POSIX_LAYOUT, 1), + ), +) +def test_step_recommends_docker_whatever_the_cause(action, tmp_path, args_mock, console_msg_mock, layout, ret_code): + """The exit code is the diagnosis and belongs in the log. What the user needs is the + way forward, so the console says the same thing however the binary failed.""" + place_binary(tmp_path, layout) + action.run_cmd.side_effect = [str(tmp_path), CommandFailed(1, "postgres -V", ret_code)] + action.args_cmd = "install" + step = TestgenVerifyEmbeddedPostgresStep() + + with pytest.raises(AbortAction): + step.execute(action, args_mock) + step.on_action_fail(action, args_mock) + + console_msg_mock.assert_any_msg_contains("cannot run on this machine") + console_msg_mock.assert_any_msg_contains("--docker") + # No exit codes or library names in front of the user. + printed = " ".join(str(c) for c in console_msg_mock.call_args_list) + assert "0xC0000135" not in printed + assert "libwinpthread" not in printed + + +@pytest.mark.unit +def test_upgrade_path_says_to_delete_first(action, tmp_path, args_mock, console_msg_mock): + """`tg install` refuses while an install marker exists, so telling an upgrading user to + just run it sends them into a refusal.""" + place_binary(tmp_path, POSIX_LAYOUT) + action.run_cmd.side_effect = [str(tmp_path), CommandFailed(1, "postgres -V", 1)] + action.args_cmd = "upgrade" + step = TestgenVerifyEmbeddedPostgresStep() + + with pytest.raises(AbortAction): + step.execute(action, args_mock) + step.on_action_fail(action, args_mock) + + console_msg_mock.assert_any_msg_contains("delete") + console_msg_mock.assert_any_msg_contains("--docker") + + +@pytest.mark.unit +def test_step_stays_quiet_when_it_did_not_fail(action, args_mock, console_msg_mock): + """on_action_fail runs for every step when any step fails -- this one must not chime in + about a database it never found fault with.""" + TestgenVerifyEmbeddedPostgresStep().on_action_fail(action, args_mock) + assert not console_msg_mock.call_args_list + + +@pytest.mark.unit +def test_check_runs_before_anything_uses_the_database(): + """The whole point is to fail before standalone-setup spends time on a broken install.""" + steps = TestgenInstallAction.pip_steps + assert steps.index(TestgenVerifyEmbeddedPostgresStep) < steps.index(TestgenStandaloneSetupStep) + + +@pytest.mark.unit +def test_upgrade_checks_too(): + """`uv tool upgrade` replaces the package, so it can pull a broken wheel just as an + install can -- and the failure would surface the same misleading way.""" + from tests.installer import TestgenStandaloneUpgradeStep, TestgenUpgradeAction + + steps = TestgenUpgradeAction.pip_steps + assert steps.index(TestgenVerifyEmbeddedPostgresStep) < steps.index(TestgenStandaloneUpgradeStep)