Merge branch 'fix/vmgui-x-display' (vmgui: X preflight, no silent failure)

docs/RUNBOOK-PhaseC.md

@@ -216,22 +216,24 @@ ci vm cleanup --max-age-hours 4 --lock-file /var/lib/ci/vm-start.lock
 Launches the **interactive** VMware Workstation GUI (not the headless vmrun path)
 so you can watch/poke a clone by hand.
 
+**Required once per desktop session** — authorise `ci-runner` on your X server
+(run as YOUR logged-in user, not ci-runner):
 ```bash
-# authorise the service user on your desktop X session first (run as YOUR user):
 xhost +SI:localuser:ci-runner
-
-# then open the VMX as ci-runner, forcing the display:
+```
+Then open the GUI as ci-runner, forcing the display:
+```bash
 ci vmgui --vmx /var/lib/ci/build-vms/Clone_dbg-1_*/Clone_dbg-1_*.vmx --display :0
 ci vmgui --vmx <path> --display :0 --power-on      # also power it on (-x)
 ci vmgui --vmx <path> --display :0 --fullscreen    # power on + fullscreen (-X)
-
-# bare GUI, no VM loaded (just open Workstation):
-ci vmgui --display :0
+ci vmgui --display :0                              # bare GUI, no VM loaded
 ```
-The GUI needs an X display; `sudo -u ci-runner` strips `$DISPLAY`, so pass
-`--display :0` (or export `DISPLAY`). Other flags: `--new-window` (`-n`),
-`--vmware-path` to override the binary. The command spawns the GUI detached and
-returns immediately (prints the child PID).
+**If nothing opens**: you skipped the `xhost` step. `vmgui` now pre-flights the
+display (`xdpyinfo`) and fails with the exact `xhost` command instead of dying
+silently; it also drops an unreadable inherited `XAUTHORITY` and logs the GUI's
+output to `/tmp/vmgui-*.log`. The GUI needs an X display — `sudo -u ci-runner`
+loses your cookie, so `xhost` + `--display :0` is the reliable combo. Other
+flags: `--new-window` (`-n`), `--vmware-path`. Spawns detached, prints PID + log.
 
 ---
 

src/ci_orchestrator/commands/vm.py

@@ -15,6 +15,7 @@ from __future__ import annotations
 import os
 import shutil
 import subprocess
+import tempfile
 import time
 from datetime import UTC, datetime, timedelta
 from pathlib import Path
@@ -488,22 +489,49 @@ def vm_new(
 # ────────────────────────────────────────────────────────────────────────── open
 
 
-def _launch_gui(argv: list[str], env: dict[str, str]) -> int:
-    """Spawn the VMware Workstation GUI detached. Returns the child PID.
+def _x_display_reachable(env: dict[str, str]) -> tuple[bool, str]:
+    """Probe whether the X display in ``env`` is usable.
+
+    Uses ``xdpyinfo`` when available (it exits non-zero and prints the reason
+    if the display cannot be opened). Returns ``(ok, detail)``; if the probe
+    tool is missing we optimistically return ``(True, "")`` so the launch is
+    still attempted.
+    """
+    probe = shutil.which("xdpyinfo")
+    if probe is None:
+        return True, ""
+    try:
+        res = subprocess.run(  # fixed argv, no shell
+            [probe],
+            env=env,
+            stdout=subprocess.DEVNULL,
+            stderr=subprocess.PIPE,
+            timeout=10,
+            check=False,
+        )
+    except (OSError, subprocess.TimeoutExpired) as exc:
+        return False, str(exc)
+    if res.returncode == 0:
+        return True, ""
+    return False, (res.stderr or b"").decode(errors="replace").strip()
+
+
+def _launch_gui(argv: list[str], env: dict[str, str], log_path: Path) -> subprocess.Popen[bytes]:
+    """Spawn the VMware Workstation GUI detached, logging stdio to ``log_path``.
 
     Isolated so tests can monkeypatch it without spawning a real process. The
-    GUI must outlive the CLI, so the child is started in its own session with
-    its stdio detached.
+    GUI must outlive the CLI, so the child runs in its own session; its output
+    goes to a log file (not /dev/null) so failures are diagnosable.
     """
-    proc = subprocess.Popen(  # argv is a fixed list, never a shell string
+    log_fh = log_path.open("wb")
+    return subprocess.Popen(  # argv is a fixed list, never a shell string
         argv,
         env=env,
         stdin=subprocess.DEVNULL,
-        stdout=subprocess.DEVNULL,
-        stderr=subprocess.DEVNULL,
+        stdout=log_fh,
+        stderr=subprocess.STDOUT,
         start_new_session=True,
     )
-    return proc.pid
 
 
 @click.command("vmgui")
@@ -575,14 +603,35 @@ def vmgui(
     env = dict(os.environ)
     if display:
         env["DISPLAY"] = display
-    if not env.get("DISPLAY"):
+    disp = env.get("DISPLAY", "")
+    if not disp:
+        raise click.ClickException(
+            "no X display set ($DISPLAY empty and --display omitted). Pass "
+            "--display :0 (and authorise this user on the desktop session: "
+            "run `xhost +SI:localuser:ci-runner` as the logged-in user)."
+        )
+
+    # An inherited XAUTHORITY pointing at another user's cookie (e.g. the
+    # desktop user's ~/.Xauthority) is unreadable by ci-runner and blocks the
+    # connection with "Authorization required". Drop it so the launch relies on
+    # host-based auth (xhost) instead.
+    xauth = env.get("XAUTHORITY")
+    if xauth and not os.access(xauth, os.R_OK):
+        env.pop("XAUTHORITY", None)
         click.echo(
-            "[vmgui] WARNING: no X display set ($DISPLAY empty and --display "
-            "omitted). The GUI will fail to start. Pass --display :0 and run "
-            "`xhost +SI:localuser:ci-runner` from a desktop session first.",
+            f"[vmgui] dropping unreadable XAUTHORITY ({xauth}); relying on xhost.",
             err=True,
         )
 
+    # Pre-flight: fail loudly with the cause instead of silently spawning a
+    # GUI that immediately dies on "cannot open display".
+    ok, detail = _x_display_reachable(env)
+    if not ok:
+        raise click.ClickException(
+            f"cannot open X display {disp!r}: {detail or 'unknown error'}. "
+            "From the desktop session run:  xhost +SI:localuser:ci-runner"
+        )
+
     argv = [gui]
     if new_window:
         argv.append("-n")
@@ -599,12 +648,32 @@ def vmgui(
             err=True,
         )
 
+    ts = datetime.now(tz=UTC).strftime("%Y%m%d-%H%M%S")
+    log_path = Path(tempfile.gettempdir()) / f"vmgui-{os.getpid()}-{ts}.log"
+
     click.echo(f"[vmgui] launching VMware GUI: {' '.join(argv)}")
     try:
-        pid = _launch_gui(argv, env)
+        proc = _launch_gui(argv, env, log_path)
     except (OSError, ValueError) as exc:
         raise click.ClickException(f"failed to launch VMware GUI: {exc}") from exc
-    click.echo(f"[vmgui] VMware GUI started (pid {pid}); display={env.get('DISPLAY', '')}")
+
+    # Brief grace: if the GUI dies right away, surface its log instead of
+    # claiming success.
+    time.sleep(1.5)
+    rc = proc.poll()
+    if rc is not None and rc != 0:
+        try:
+            tail = log_path.read_text(errors="replace").strip().splitlines()[-5:]
+        except OSError:
+            tail = []
+        detail = "\n  ".join(tail) if tail else "(no output captured)"
+        raise click.ClickException(
+            f"VMware GUI exited immediately (code {rc}). Log {log_path}:\n  {detail}"
+        )
+
+    click.echo(
+        f"[vmgui] VMware GUI started (pid {proc.pid}); display={disp}; log={log_path}"
+    )
 
 
 __all__ = ["cleanup_orphans", "vm", "vmgui"]

tests/python/test_commands_vm.py

@@ -502,26 +502,44 @@ def test_vm_cleanup_backend_unavailable(monkeypatch: pytest.MonkeyPatch, tmp_pat
 # ── vmgui ───────────────────────────────────────────────────────────────────
 
 
-def _capture_launch(monkeypatch: pytest.MonkeyPatch) -> dict[str, Any]:
-    """Patch _launch_gui to record argv/env instead of spawning a process."""
+class _FakeProc:
+    """Minimal subprocess.Popen stand-in for vmgui tests."""
+
+    def __init__(self, pid: int = 4242, rc: int | None = None) -> None:
+        self.pid = pid
+        self._rc = rc
+
+    def poll(self) -> int | None:
+        return self._rc
+
+
+def _capture_launch(
+    monkeypatch: pytest.MonkeyPatch, *, rc: int | None = None
+) -> dict[str, Any]:
+    """Patch _launch_gui + preflight + sleep so no real process is spawned."""
     captured: dict[str, Any] = {}
 
-    def _fake(argv: list[str], env: dict[str, str]) -> int:
+    def _fake(argv: list[str], env: dict[str, str], log_path: Path) -> _FakeProc:
         captured["argv"] = argv
         captured["env"] = env
-        return 4242
+        captured["log_path"] = log_path
+        if rc is not None and rc != 0:
+            log_path.write_text("cannot open display\n")
+        return _FakeProc(rc=rc)
 
     monkeypatch.setattr(vm_module, "_launch_gui", _fake)
+    monkeypatch.setattr(vm_module, "_x_display_reachable", lambda _env: (True, ""))
+    monkeypatch.setattr(vm_module.time, "sleep", lambda _s: None)
     return captured
 
 
-def test_vm_open_missing_vmx_errors(tmp_path: Path) -> None:
+def test_vmgui_missing_vmx_errors(tmp_path: Path) -> None:
     result = CliRunner().invoke(cli, ["vmgui", "--vmx", str(tmp_path / "nope.vmx")])
     assert result.exit_code != 0
     assert "VMX file not found" in result.output
 
 
-def test_vm_open_happy_path(monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> None:
+def test_vmgui_happy_path(monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> None:
     vmx = tmp_path / "clone.vmx"
     vmx.write_text("config")
     cap = _capture_launch(monkeypatch)
@@ -533,7 +551,7 @@ def test_vm_open_happy_path(monkeypatch: pytest.MonkeyPatch, tmp_path: Path) ->
     assert "pid 4242" in result.output
 
 
-def test_vm_open_power_on_adds_x(monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> None:
+def test_vmgui_power_on_adds_x(monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> None:
     vmx = tmp_path / "clone.vmx"
     vmx.write_text("config")
     cap = _capture_launch(monkeypatch)
@@ -543,7 +561,7 @@ def test_vm_open_power_on_adds_x(monkeypatch: pytest.MonkeyPatch, tmp_path: Path
     assert "-x" in cap["argv"]
 
 
-def test_vm_open_fullscreen_adds_capital_x(monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> None:
+def test_vmgui_fullscreen_adds_capital_x(monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> None:
     vmx = tmp_path / "clone.vmx"
     vmx.write_text("config")
     cap = _capture_launch(monkeypatch)
@@ -555,7 +573,7 @@ def test_vm_open_fullscreen_adds_capital_x(monkeypatch: pytest.MonkeyPatch, tmp_
     assert "-X" in cap["argv"] and "-n" in cap["argv"]
 
 
-def test_vm_open_display_option_overrides_env(monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> None:
+def test_vmgui_display_option_overrides_env(monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> None:
     vmx = tmp_path / "clone.vmx"
     vmx.write_text("config")
     cap = _capture_launch(monkeypatch)
@@ -566,42 +584,80 @@ def test_vm_open_display_option_overrides_env(monkeypatch: pytest.MonkeyPatch, t
     assert "display=:1" in result.output
 
 
-def test_vm_open_warns_without_display(monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> None:
+def test_vmgui_errors_without_display(monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> None:
     vmx = tmp_path / "clone.vmx"
     vmx.write_text("config")
     _capture_launch(monkeypatch)
     monkeypatch.delenv("DISPLAY", raising=False)
     result = CliRunner().invoke(cli, ["vmgui", "--vmx", str(vmx)])
-    assert result.exit_code == 0, result.output
+    assert result.exit_code != 0
     assert "no X display" in result.output
 
 
-def test_vm_open_launch_failure_is_reported(monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> None:
+def test_vmgui_preflight_failure_reports_xhost(monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> None:
+    vmx = tmp_path / "clone.vmx"
+    vmx.write_text("config")
+    _capture_launch(monkeypatch)
+    # Override the optimistic preflight with a failing probe.
+    monkeypatch.setattr(
+        vm_module, "_x_display_reachable", lambda _env: (False, "cannot open display :0")
+    )
+    monkeypatch.setenv("DISPLAY", ":0")
+    result = CliRunner().invoke(cli, ["vmgui", "--vmx", str(vmx)])
+    assert result.exit_code != 0
+    assert "xhost +SI:localuser:ci-runner" in result.output
+
+
+def test_vmgui_drops_unreadable_xauthority(monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> None:
+    vmx = tmp_path / "clone.vmx"
+    vmx.write_text("config")
+    cap = _capture_launch(monkeypatch)
+    monkeypatch.setenv("DISPLAY", ":0")
+    monkeypatch.setenv("XAUTHORITY", str(tmp_path / "missing.Xauthority"))
+    result = CliRunner().invoke(cli, ["vmgui", "--vmx", str(vmx)])
+    assert result.exit_code == 0, result.output
+    assert "XAUTHORITY" not in cap["env"]
+    assert "dropping unreadable XAUTHORITY" in result.output
+
+
+def test_vmgui_immediate_exit_surfaces_log(monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> None:
+    vmx = tmp_path / "clone.vmx"
+    vmx.write_text("config")
+    _capture_launch(monkeypatch, rc=1)
+    monkeypatch.setenv("DISPLAY", ":0")
+    result = CliRunner().invoke(cli, ["vmgui", "--vmx", str(vmx)])
+    assert result.exit_code != 0
+    assert "exited immediately" in result.output
+    assert "cannot open display" in result.output
+
+
+def test_vmgui_launch_oserror_is_reported(monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> None:
     vmx = tmp_path / "clone.vmx"
     vmx.write_text("config")
 
-    def _boom(argv: list[str], env: dict[str, str]) -> int:
+    def _boom(argv: list[str], env: dict[str, str], log_path: Path) -> Any:
         raise OSError("vmware not found")
 
     monkeypatch.setattr(vm_module, "_launch_gui", _boom)
+    monkeypatch.setattr(vm_module, "_x_display_reachable", lambda _env: (True, ""))
+    monkeypatch.setattr(vm_module.time, "sleep", lambda _s: None)
     monkeypatch.setenv("DISPLAY", ":0")
     result = CliRunner().invoke(cli, ["vmgui", "--vmx", str(vmx)])
     assert result.exit_code != 0
     assert "failed to launch VMware GUI" in result.output
 
 
-def test_vm_open_without_vmx_launches_bare_gui(monkeypatch: pytest.MonkeyPatch) -> None:
+def test_vmgui_without_vmx_launches_bare_gui(monkeypatch: pytest.MonkeyPatch) -> None:
     cap = _capture_launch(monkeypatch)
     monkeypatch.setenv("DISPLAY", ":0")
     result = CliRunner().invoke(cli, ["vmgui"])
     assert result.exit_code == 0, result.output
-    # Only the binary (no vmx path, no power-on flags).
     assert cap["argv"][1:] == [] or cap["argv"][1:] == ["-n"]
     assert "-x" not in cap["argv"] and "-X" not in cap["argv"]
     assert "pid 4242" in result.output
 
 
-def test_vm_open_power_on_ignored_without_vmx(monkeypatch: pytest.MonkeyPatch) -> None:
+def test_vmgui_power_on_ignored_without_vmx(monkeypatch: pytest.MonkeyPatch) -> None:
     cap = _capture_launch(monkeypatch)
     monkeypatch.setenv("DISPLAY", ":0")
     result = CliRunner().invoke(cli, ["vmgui", "--power-on"])