From 37bb694ef5efd47c82524fc18c21efeb9f05ae4c Mon Sep 17 00:00:00 2001 From: olegbrok Date: Tue, 4 Aug 2026 06:42:37 -0700 Subject: [PATCH] fix(admin): fall back from broken venv pip --- src/pinky_daemon/api.py | 54 ++++++++++++++++-- tests/test_admin_update.py | 114 ++++++++++++++++++++++++++++++++++++- 2 files changed, 161 insertions(+), 7 deletions(-) diff --git a/src/pinky_daemon/api.py b/src/pinky_daemon/api.py index 65cc9df3..4ae0f110 100644 --- a/src/pinky_daemon/api.py +++ b/src/pinky_daemon/api.py @@ -11836,14 +11836,42 @@ def _run_update() -> dict: deps_drift = [] if changed or force_deps or deps_drift: - # Prefer project venv pip if present, else use the running daemon's - # interpreter (sys.executable). This works for both venv and - # system-python deployments — the prior `.venv/bin/pip`-only path - # silently skipped rebuilds on system-python hosts. + # Prefer a functional project venv pip, else use the running + # daemon's interpreter (sys.executable). A vestigial .venv can + # retain bin/pip even after pip disappears from its interpreter, + # so file existence alone is not enough to select it. venv_pip = Path(repo_dir) / ".venv" / "bin" / "pip" + venv_python = Path(repo_dir) / ".venv" / "bin" / "python" + pip_cmd: list[str] | None = None if venv_pip.exists(): - pip_cmd = [str(venv_pip), "install", "-e", ".[all]", "--quiet"] - else: + try: + sp.check_output( + [str(venv_python), "-m", "pip", "--version"], + cwd=repo_dir, stderr=sp.STDOUT, timeout=10, + ) + except Exception as probe_exc: + probe_output = getattr(probe_exc, "output", None) + if isinstance(probe_output, bytes): + probe_detail = probe_output.decode(errors="replace") + elif probe_output: + probe_detail = str(probe_output) + else: + probe_detail = f"{type(probe_exc).__name__}: {probe_exc}" + probe_detail = " ".join(probe_detail.split())[:500] + if not probe_detail: + probe_detail = f"{type(probe_exc).__name__}: {probe_exc}" + _log( + "admin: .venv exists but its pip is broken " + f"({probe_detail}) — treating it as vestigial and " + f"falling back to {sys.executable}" + ) + else: + pip_cmd = [ + str(venv_python), "-m", "pip", "install", + "-e", ".[all]", "--quiet", + ] + + if pip_cmd is None: # PEP 668: system pythons (Homebrew, Debian) mark themselves # externally-managed. --break-system-packages lets us install # into the same env the daemon imports from. @@ -11924,6 +11952,20 @@ def _run_update() -> dict: result = await asyncio.to_thread(_run_update) + if result.get("deps_error"): + try: + delivered = await scheduler._owner_notify_callback( + "admin", + "ADMIN UPDATE DEPENDENCY REBUILD FAILED: " + f"{result['deps_error']}. The code update may have landed " + "without required dependencies; inspect the daemon log and " + "rerun the dependency rebuild.", + ) + if not delivered: + _log("admin: dependency rebuild owner alert was not delivered") + except Exception as notify_exc: + _log(f"admin: dependency rebuild owner alert failed: {notify_exc}") + # Schedule graceful restart if anything changed if result.get("restarting"): import signal diff --git a/tests/test_admin_update.py b/tests/test_admin_update.py index ae2816c1..671518fd 100644 --- a/tests/test_admin_update.py +++ b/tests/test_admin_update.py @@ -4,15 +4,18 @@ import os import subprocess as sp +import sys import tempfile from pathlib import Path -from unittest.mock import patch +from unittest.mock import AsyncMock, patch import pytest from fastapi.testclient import TestClient from pinky_daemon.self_update import DeployDecision +_REAL_PATH_EXISTS = Path.exists + @pytest.fixture(autouse=True) def _stub_resolve_and_verify(): @@ -513,6 +516,12 @@ def wrapped(cmd, **kwargs): return wrapped + @staticmethod + def _with_venv_pip(path: Path) -> bool: + if str(path).endswith("/.venv/bin/pip"): + return True + return _REAL_PATH_EXISTS(path) + def test_force_deps_triggers_pip_install_even_on_clean_pull(self): """force_deps=True → pip install runs even when nothing changed in git.""" gm = _GitMock(dirty_files=[], before_hash="same1", after_hash="same1") @@ -600,6 +609,109 @@ def fail_on_pip(cmd, **kwargs): assert body.get("deps_error") assert "pip install failed" in body["deps_error"] + def test_broken_venv_pip_falls_back_and_logs_vestigial_warning(self): + gm = _GitMock(dirty_files=[], before_hash="same1", after_hash="same1") + calls: list[list[str]] = [] + logs: list[str] = [] + + def broken_venv_pip(cmd, **kwargs): + cmd_list = list(cmd) + calls.append(cmd_list) + if cmd_list[1:] == ["-m", "pip", "--version"]: + raise sp.CalledProcessError( + 1, cmd, output=b".venv/bin/python: No module named pip", + ) + if "install" in cmd_list and "pip" in cmd_list: + return b"" + return gm(cmd, **kwargs) + + with ( + patch("subprocess.check_output", side_effect=broken_venv_pip), + patch("pathlib.Path.exists", autospec=True, side_effect=self._with_venv_pip), + patch("pinky_daemon.api._log", side_effect=logs.append), + patch("shutil.which", return_value=None), + patch("os.kill"), + ): + client = _make_client() + r = client.post("/admin/update?branch=main&force_deps=true") + + assert r.status_code == 200 + assert r.json()["deps_rebuilt"] is True + assert any( + call[:4] == [sys.executable, "-m", "pip", "install"] + and "--break-system-packages" in call + for call in calls + ) + install_calls = [call for call in calls if "install" in call and "pip" in call] + assert len(install_calls) == 1 + assert "--break-system-packages" in install_calls[0] + warnings = [line for line in logs if "treating it as vestigial" in line] + assert len(warnings) == 1 + assert "No module named pip" in warnings[0] + assert f"falling back to {sys.executable}" in warnings[0] + + def test_functional_venv_pip_is_still_preferred(self): + gm = _GitMock(dirty_files=[], before_hash="same1", after_hash="same1") + calls: list[list[str]] = [] + project_venv_python: list[str] = [] + + def functional_venv_pip(cmd, **kwargs): + cmd_list = list(cmd) + calls.append(cmd_list) + if cmd_list[1:] == ["-m", "pip", "--version"]: + project_venv_python.append(cmd_list[0]) + return b"pip 26.0" + if cmd_list[0].endswith("/.venv/bin/python") and "install" in cmd_list: + return b"" + return gm(cmd, **kwargs) + + with ( + patch("subprocess.check_output", side_effect=functional_venv_pip), + patch("pathlib.Path.exists", autospec=True, side_effect=self._with_venv_pip), + patch("shutil.which", return_value=None), + patch("os.kill"), + ): + client = _make_client() + r = client.post("/admin/update?branch=main&force_deps=true") + + assert r.status_code == 200 + assert r.json()["deps_rebuilt"] is True + assert any(call[1:] == ["-m", "pip", "--version"] for call in calls) + assert any( + call[0] == project_venv_python[0] + and call[1:4] == ["-m", "pip", "install"] + and "--break-system-packages" not in call + for call in calls + ) + install_calls = [call for call in calls if "install" in call and "pip" in call] + assert len(install_calls) == 1 + assert "--break-system-packages" not in install_calls[0] + + def test_deps_error_alerts_owner_once(self): + gm = _GitMock(dirty_files=[], before_hash="same1", after_hash="same1") + + def fail_on_pip(cmd, **kwargs): + cmd_list = list(cmd) + if "install" in cmd_list and "pip" in cmd_list: + raise sp.CalledProcessError(1, cmd, output=b"ERROR: package not found") + return gm(cmd, **kwargs) + + with ( + patch("subprocess.check_output", side_effect=fail_on_pip), + patch("shutil.which", return_value=None), + patch("os.kill"), + ): + client = _make_client() + owner_notify = AsyncMock(return_value=True) + client.app.state.scheduler._owner_notify_callback = owner_notify + r = client.post("/admin/update?branch=main&force_deps=true") + + assert r.status_code == 200 + assert r.json()["deps_error"] + owner_notify.assert_awaited_once() + assert owner_notify.await_args.args[0] == "admin" + assert "ADMIN UPDATE DEPENDENCY REBUILD FAILED" in owner_notify.await_args.args[1] + class TestInstalledDepsDriftDetection: """Direct unit tests for `_check_installed_deps_drift`.