From 4158edec8a4552f9c4c487ea0e5360658a17a353 Mon Sep 17 00:00:00 2001 From: Andrew Park Date: Thu, 28 May 2026 13:43:36 -0700 Subject: [PATCH] =?UTF-8?q?fix(cli):=20jarvis=20daemon=20UX=20=E2=80=94=20?= =?UTF-8?q?port=20collision,=20status=20URL,=20doctor=20hints?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three small but compounding rough edges on a fresh install. 1. `jarvis doctor` flagged missing optional extras as "Not installed (openjarvis[server])" — pip-style marker the user has to translate. Now reads "Not installed — run: uv sync --extra server" so it's directly copy-pasteable. The `pip install torch` line stays as-is (it's already a real command). 2. `jarvis start` with no --port silently collided with whatever was on 8000 (Docker, in my case). The Popen succeeded, the pidfile got written, and the daemon then died on uvicorn's bind — leaving `status` reporting a running server that wasn't there. Now we probe the port via socket.connect_ex() before forking and bail with a clear error pointing at --port. (connect_ex, not bind — SO_REUSEADDR on macOS' dual-stack lets an IPv4 bind succeed even when something holds the IPv6 wildcard, so the bind check was too permissive.) 3. `jarvis status` always printed the config-default URL even when start was given --port. Pidfile now stores {pid, host, port} as JSON and status reads from it. Legacy plain-integer pidfiles are still honored (host/port fall back to config defaults for those) so any currently-running daemon doesn't get orphaned by the upgrade. --- src/openjarvis/cli/daemon_cmd.py | 91 +++++++++++++++++++++++++++----- src/openjarvis/cli/doctor_cmd.py | 9 +++- 2 files changed, 85 insertions(+), 15 deletions(-) diff --git a/src/openjarvis/cli/daemon_cmd.py b/src/openjarvis/cli/daemon_cmd.py index e1b25eaf..94f8cdea 100644 --- a/src/openjarvis/cli/daemon_cmd.py +++ b/src/openjarvis/cli/daemon_cmd.py @@ -2,8 +2,10 @@ from __future__ import annotations +import json import os import signal +import socket import subprocess import sys import time @@ -17,24 +19,65 @@ _PID_FILE = DEFAULT_CONFIG_DIR / "server.pid" _LOG_FILE = DEFAULT_CONFIG_DIR / "server.log" -def _read_pid() -> int | None: - """Read PID from pid file, return None if not found or stale.""" +def _read_pidfile() -> dict | None: + """Read daemon record. Returns dict with pid/host/port if alive, else None. + + Accepts both the legacy plain-integer format (older daemons that only + wrote the pid) and the structured JSON format. For legacy files we fall + back to config defaults for host/port so ``status`` doesn't break for + anyone with a running daemon when this ships. + """ if not _PID_FILE.exists(): return None + raw = _PID_FILE.read_text().strip() + try: + record = json.loads(raw) + pid = int(record["pid"]) + except (json.JSONDecodeError, KeyError, TypeError, ValueError): + try: + pid = int(raw) + record = {"pid": pid} + except ValueError: + _PID_FILE.unlink(missing_ok=True) + return None try: - pid = int(_PID_FILE.read_text().strip()) - # Check if process is still running os.kill(pid, 0) - return pid - except (ValueError, OSError): + except OSError: _PID_FILE.unlink(missing_ok=True) return None + return record -def _write_pid(pid: int) -> None: - """Write PID to pid file.""" +def _read_pid() -> int | None: + record = _read_pidfile() + return record["pid"] if record else None + + +def _write_pidfile(pid: int, host: str, port: int) -> None: + """Persist the daemon's pid + bound address so ``status`` reports truth.""" DEFAULT_CONFIG_DIR.mkdir(parents=True, exist_ok=True) - _PID_FILE.write_text(str(pid)) + _PID_FILE.write_text(json.dumps({"pid": pid, "host": host, "port": port})) + + +def _port_in_use(host: str, port: int) -> bool: + """Best-effort check: is anything already listening on ``host:port``? + + Done before forking the daemon so the user gets a clean error instead + of a pidfile pointing at a process that died on startup. We use + ``connect_ex`` instead of trial-binding because ``bind`` with + SO_REUSEADDR is too permissive on macOS' dual-stack — a Docker + container holding ``*:8000`` on IPv6 doesn't block an IPv4 bind, but + uvicorn's actual bind still fails later. Connecting catches it. + + Wildcard hosts (``0.0.0.0``, ``::``) probe via loopback since you + can't ``connect()`` to a wildcard. + """ + probe_host = "127.0.0.1" if host in ("0.0.0.0", "::", "*") else host + with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as s: + s.settimeout(0.5) + if s.connect_ex((probe_host, port)) == 0: + return True + return False @click.group() @@ -68,6 +111,15 @@ def start( bind_host = host or config.server.host bind_port = port or config.server.port + if _port_in_use(bind_host, bind_port): + console.print( + f"[red]Port {bind_port} is already in use on {bind_host}.[/red]\n" + f" Something else is bound there (often Docker, another jarvis,\n" + f" or a leftover dev server). Free it or pick a different port:\n" + f" jarvis start --port " + ) + sys.exit(1) + # Build command to run jarvis serve cmd = [sys.executable, "-m", "openjarvis.cli", "serve"] if host: @@ -90,7 +142,7 @@ def start( stderr=log_fh, start_new_session=True, ) - _write_pid(proc.pid) + _write_pidfile(proc.pid, bind_host, bind_port) console.print( f"[green]OpenJarvis server started[/green] (PID {proc.pid})\n" @@ -146,11 +198,13 @@ def restart(ctx: click.Context) -> None: def status() -> None: """Show status of the OpenJarvis server daemon.""" console = Console(stderr=True) - pid = _read_pid() - if pid is None: + record = _read_pidfile() + if record is None: console.print("[yellow]Server is not running.[/yellow]") return + pid = record["pid"] + # Get process info uptime_info = "" try: @@ -164,10 +218,19 @@ def status() -> None: except (ImportError, Exception): pass - config = load_config() + # Prefer the address the daemon was actually started on. Legacy pidfiles + # (pre-structured-record) won't have these — fall back to config defaults + # so we don't lie about the URL if we genuinely don't know it. + if "host" in record and "port" in record: + bind_host = record["host"] + bind_port = record["port"] + else: + config = load_config() + bind_host = config.server.host + bind_port = config.server.port console.print( f"[green]Server is running[/green] (PID {pid}){uptime_info}\n" - f" URL: http://{config.server.host}:{config.server.port}\n" + f" URL: http://{bind_host}:{bind_port}\n" f" Log: {_LOG_FILE}" ) diff --git a/src/openjarvis/cli/doctor_cmd.py b/src/openjarvis/cli/doctor_cmd.py index f9db7232..01a27db0 100644 --- a/src/openjarvis/cli/doctor_cmd.py +++ b/src/openjarvis/cli/doctor_cmd.py @@ -213,11 +213,18 @@ def _check_optional_deps() -> List[CheckResult]: __import__(pkg) results.append(CheckResult(f"Optional: {description}", "ok", "Installed")) except Exception: + # `openjarvis[extra]` is a pip-style marker the user shouldn't + # have to translate. Surface a copy-pasteable command instead. + if install_hint.startswith("openjarvis[") and install_hint.endswith("]"): + extra = install_hint[len("openjarvis[") : -1] + hint = f"run: uv sync --extra {extra}" + else: + hint = install_hint results.append( CheckResult( f"Optional: {description}", "warn", - f"Not installed ({install_hint})", + f"Not installed — {hint}", ) ) return results