diff --git a/dk-installer.py b/dk-installer.py index 8429c9b..f04768f 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") @@ -2852,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" @@ -2929,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, @@ -3133,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, @@ -3329,8 +3471,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): @@ -3352,15 +3497,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'.") @@ -3373,17 +3520,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_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_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" 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_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() 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)