Studio: let a caller that follows the port keep the fallback, and never take studio.pid from a live server
Two problems with keying the own-server abort on api_only. `unsloth studio run` is not the bare-banner path: it stores `app = run_server(...)` and reads `app.state.server_port` back, then uses it for the health wait, the model load and the printed base URL. Gating on api_only aborted it, so starting a second model while the first was up stopped working, where before it landed on the next port and printed the right URL. Replace the proxy with an explicit abort_if_own_studio, defaulting to the old api_only behaviour so the exec'd `run.py` path is unchanged, and have `studio run` opt out. The api_only exemption also reopened the orphan from the other side. _write_pid_file overwrote studio.pid unconditionally, and a pre-upgrade server is recorded there and nowhere else, so an exempt launch falling back past one erased its only record. Take the file over only when it is free, already ours, or held by a dead PID. Also resync _pid_is_studio_backend with the CLI copy: an untimed record next to a timed one carried no information but cancelled the start-time check, which is what let a reused PID be treated as ours. Tests: 51 backend, 26 CLI, 9 under tests/studio. Real Studio servers still abort the bare same-port relaunch, still fall back past a foreign listener, and one `unsloth studio stop` still stops every server in all five scenarios.
This commit is contained in:
parent
52d671f3a2
commit
c854b2dc80
4 changed files with 55 additions and 8 deletions
|
|
@ -867,7 +867,7 @@ def _pid_is_studio_backend(pid: int, created_times: "Sequence[float | None]" = (
|
|||
the command line rejected real servers.
|
||||
"""
|
||||
known = [c for c in created_times if c is not None]
|
||||
if not known or len(known) < len(created_times):
|
||||
if not known:
|
||||
return True
|
||||
actual = _process_create_time(pid)
|
||||
if actual is None:
|
||||
|
|
@ -1027,7 +1027,12 @@ def _write_pid_file(port: int, host: str = ""):
|
|||
# independently of the per-port record: if that one failed, this is the only
|
||||
# thing keeping the server stoppable at all.
|
||||
try:
|
||||
_PID_FILE.write_text(str(os.getpid()), encoding = "utf-8")
|
||||
# Never take it from a server that is still running. A pre-upgrade server
|
||||
# is recorded here and nowhere else, so overwriting its entry is exactly
|
||||
# what strands it -- the orphan this file exists to prevent.
|
||||
prior = _read_pid_record(_PID_FILE) if _PID_FILE.is_file() else None
|
||||
if prior is None or prior[0] == os.getpid() or not _pid_alive(prior[0]):
|
||||
_PID_FILE.write_text(str(os.getpid()), encoding = "utf-8")
|
||||
except OSError:
|
||||
pass
|
||||
|
||||
|
|
@ -1659,6 +1664,7 @@ def run_server(
|
|||
enable_tools: "Optional[bool]" = None,
|
||||
password: "Optional[str]" = None,
|
||||
emit_tauri_port: bool = True,
|
||||
abort_if_own_studio: "Optional[bool]" = None,
|
||||
):
|
||||
"""
|
||||
Start the FastAPI server.
|
||||
|
|
@ -1793,11 +1799,13 @@ def run_server(
|
|||
|
||||
# Auto-find a free port if the requested one is in use.
|
||||
original_port = port
|
||||
# api-only callers read the bound port back (TAURI_PORT for the desktop app,
|
||||
# app.state.server_port for `studio run`), so a fallback there is harmless and
|
||||
# is what the desktop app's 8888-8908 range expects. The interactive path
|
||||
# prints the requested port, so falling back is what strands a server.
|
||||
port = _resolve_port(host, port, avoid_own_studio = not api_only)
|
||||
# Refusing rather than falling back is for callers that cannot follow us to
|
||||
# the new port. `studio run` reads app.state.server_port back and the desktop
|
||||
# app reads TAURI_PORT, so both should keep the plain fallback; only the
|
||||
# bare launch, which has nothing but the banner, benefits from the refusal.
|
||||
if abort_if_own_studio is None:
|
||||
abort_if_own_studio = not api_only
|
||||
port = _resolve_port(host, port, avoid_own_studio = abort_if_own_studio)
|
||||
if port != original_port:
|
||||
blocker = _get_pid_on_port(original_port)
|
||||
if not silent:
|
||||
|
|
|
|||
|
|
@ -286,7 +286,17 @@ def test_an_untimed_legacy_record_is_trusted(monkeypatch):
|
|||
|
||||
assert run._pid_is_studio_backend(8550) is True
|
||||
assert run._pid_is_studio_backend(8550, [None]) is True
|
||||
assert run._pid_is_studio_backend(8550, [111.5, None]) is True
|
||||
|
||||
|
||||
def test_the_untimed_legacy_record_does_not_cancel_a_timed_one(monkeypatch):
|
||||
# Mirrors _pid_is_studio_server in the CLI. An untimed record carries no
|
||||
# information, so it must not overrule a start time that says "not ours" --
|
||||
# every current server writes one of each, which made the check inert.
|
||||
monkeypatch.setattr(run, "_pid_is_studio_backend", _REAL_IS_STUDIO_BACKEND)
|
||||
monkeypatch.setattr(run, "_process_create_time", lambda pid: 999.0)
|
||||
|
||||
assert run._pid_is_studio_backend(8550, [111.5, None]) is False
|
||||
assert run._pid_is_studio_backend(8550, [111.5, 999.0]) is True
|
||||
|
||||
|
||||
def test_a_legacy_server_on_the_port_is_recognised(tmp_path, monkeypatch):
|
||||
|
|
@ -510,3 +520,26 @@ def test_a_record_whose_pid_is_not_ascii_digits_is_discarded(tmp_path):
|
|||
(tmp_path / "r.pid").write_text("²", encoding = "utf-8")
|
||||
|
||||
assert run._read_pid_record(tmp_path / "r.pid") is None
|
||||
|
||||
|
||||
def test_the_legacy_file_is_not_taken_from_a_live_server(tmp_path):
|
||||
# A pre-upgrade server is recorded in studio.pid and nowhere else, so a
|
||||
# second launch overwriting it is exactly what strands it. That is the
|
||||
# orphan this file exists to prevent, reached from the other direction.
|
||||
(tmp_path / "studio.pid").write_text("8550", encoding = "utf-8")
|
||||
|
||||
run._write_pid_file(8902, "127.0.0.1")
|
||||
|
||||
assert (tmp_path / "studio.pid").read_text(encoding = "utf-8") == "8550"
|
||||
assert (tmp_path / f"studio-8902-{os.getpid()}.pid").exists()
|
||||
|
||||
|
||||
def test_the_legacy_file_is_taken_over_from_a_dead_server(tmp_path, monkeypatch):
|
||||
# A stale record must not keep the pointer forever, or an older CLI could
|
||||
# never stop anything again.
|
||||
monkeypatch.setattr(run, "_pid_alive", lambda pid: False)
|
||||
(tmp_path / "studio.pid").write_text("8550", encoding = "utf-8")
|
||||
|
||||
run._write_pid_file(8902, "127.0.0.1")
|
||||
|
||||
assert (tmp_path / "studio.pid").read_text(encoding = "utf-8") == str(os.getpid())
|
||||
|
|
|
|||
|
|
@ -62,6 +62,9 @@ def test_the_legacy_file_stays_a_bare_pid_an_older_cli_can_parse(tmp_path, monke
|
|||
"_pid_file_for_port": lambda port: _backend_pid_path(tmp_path, port),
|
||||
"_process_create_time": lambda pid: None,
|
||||
"_bind_addresses": lambda host, port: {host},
|
||||
# _write_pid_file consults these before taking over studio.pid.
|
||||
"_read_pid_record": lambda path: None,
|
||||
"_pid_alive": lambda pid: False,
|
||||
"_OWN_PID_FILE": None,
|
||||
}
|
||||
exec(_func_source(_RUN_SRC, "_write_pid_file"), ns)
|
||||
|
|
|
|||
|
|
@ -2237,6 +2237,9 @@ def run(
|
|||
# Headless serving prints its own URL/API-key banner; the Tauri-only
|
||||
# TAURI_PORT line would corrupt that machine-parseable output.
|
||||
emit_tauri_port = False,
|
||||
# We read the bound port back below, so a fallback past another Studio is
|
||||
# safe here and keeps side-by-side model runs working.
|
||||
abort_if_own_studio = False,
|
||||
)
|
||||
# Forward the frontend validated before the gate (in-venv path).
|
||||
if resolved_frontend is not None:
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue