mirror of
https://github.com/open-jarvis/OpenJarvis.git
synced 2026-07-27 21:05:34 +00:00
fix(cli): jarvis daemon UX — port collision, status URL, doctor hints
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.
This commit is contained in:
@@ -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 <other>"
|
||||
)
|
||||
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}"
|
||||
)
|
||||
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user