diff --git a/studio/backend/run.py b/studio/backend/run.py index 076a1f851b..2d9e714d90 100644 --- a/studio/backend/run.py +++ b/studio/backend/run.py @@ -10,7 +10,7 @@ import os import sys import time from pathlib import Path -from typing import Optional, Tuple +from typing import NoReturn, Optional, Sequence, Tuple def _fix_torch_cuda_ld_path(): @@ -689,6 +689,33 @@ def _get_pid_on_port(port: int) -> "tuple[int, str] | None": return None +def _bind_addresses(host: str, port: int) -> "set[str]": + """Every address *host* resolves to. `localhost` is both 127.0.0.1 and ::1, and + recording only the first lets a later launch on the other one miss us.""" + import socket + + try: + infos = socket.getaddrinfo(host, port, socket.AF_UNSPEC, socket.SOCK_STREAM) + except OSError: + return {host} + return {info[4][0] for info in infos} or {host} + + +def _addresses_collide(recorded: "str | None", host: str, port: int) -> bool: + """Would a server bound to *recorded* block a bind to *host*? + + *recorded* may list several addresses. Unknown or wildcard on either side + collides: refusing with a clear message beats silently starting a duplicate. + """ + wildcards = ("0.0.0.0", "::", "") + if not recorded or host in wildcards: + return True + listed = {a.strip() for a in recorded.split(",") if a.strip()} + if not listed or listed & set(wildcards): + return True + return bool(listed & _bind_addresses(host, port)) + + def _is_port_free(host: str, port: int) -> bool: """Check if a port is available for binding. @@ -733,18 +760,213 @@ def _find_free_port( host: str, start: int, max_attempts: int = 20, + avoid_own_studio: bool = False, ) -> int: - """Find a free port from `start`, trying up to max_attempts ports.""" + """Find a free port from `start`, trying up to max_attempts ports. + + ``avoid_own_studio`` aborts rather than skipping past one of our own servers + in the fallback range, which would start a duplicate on a later port. + """ for offset in range(max_attempts): candidate = start + offset if _is_port_free(host, candidate): return candidate + if avoid_own_studio: + own = _own_studio_on_port(candidate, host) + if own is not None: + _abort_already_running(own, candidate) raise RuntimeError(f"Could not find a free port in range {start}-{start + max_attempts - 1}") from utils.paths.storage_roots import studio_root as _studio_root +# Legacy single-instance file; still read so `stop` finds an older build's server. _PID_FILE = _studio_root() / "studio.pid" +PID_FILE_GLOB = "studio-*.pid" + + +def _pid_file_for_port(port: int) -> Path: + # PID in the name: 127.0.0.1 and ::1 can share a port, and one file per port + # would let the second bind overwrite the first. + return _studio_root() / f"studio-{port}-{os.getpid()}.pid" + + +def _pid_alive(pid: int) -> bool: + try: + import psutil + return psutil.pid_exists(pid) + except ImportError: + pass + if sys.platform == "win32": + # os.kill(pid, 0) raises OSError for every pid on Windows, so tasklist is + # the only usable probe here. + import subprocess + try: + out = subprocess.run( + ["tasklist", "/FI", f"PID eq {int(pid)}", "/NH", "/FO", "CSV"], + capture_output = True, + text = True, + timeout = 10, + ).stdout + except Exception: + # Unconfirmed means keep, matching the CLI's _pid_alive. Pruning a + # live server's record is what lets the next launch fall back past it + # and strand it, which is the bug this file exists to fix. A stale + # record instead costs one clear "already running" message. + return True + return f'"{int(pid)}"' in out + try: + os.kill(pid, 0) + except ProcessLookupError: + return False + except OSError: + return True + return True + + +def _process_create_time(pid: int) -> "float | None": + try: + import psutil + return psutil.Process(pid).create_time() + except Exception: + return None + + +def _read_pid_record(path: Path) -> "tuple[int, float | None, str | None] | None": + """Parse ``pid`` / optional ``create_time`` / optional bind address.""" + try: + lines = path.read_text(encoding = "utf-8").splitlines() + except (OSError, UnicodeDecodeError): + return None + if not lines or not lines[0].strip().isdigit(): + return None + try: + # isdigit() is not enough: a superscript two passes it but int() rejects it. + pid = int(lines[0].strip()) + except ValueError: + return None + # kill(0) signals our whole process group; kill(1) is init. Never either. + if pid < 2: + return None + created = None + if len(lines) > 1: + try: + created = float(lines[1].strip()) + except ValueError: + created = None + address = lines[2].strip() if len(lines) > 2 and lines[2].strip() else None + return pid, created, address + + +def _pid_is_studio_backend(pid: int, created_times: "Sequence[float | None]" = ()) -> bool: + """False only when a recorded start time proves this PID is a different process. + + Any recorded time matching is enough -- a stale record must not veto a live + server that reused the PID. Untimed records cannot be checked at all, so they + are trusted: a legacy `python run.py` has no telltale argv, and guessing from + the command line rejected real servers. + """ + known = [c for c in created_times if c is not None] + if not known: + return True + actual = _process_create_time(pid) + if actual is None: + return True + return any(abs(actual - c) < 1.0 for c in known) + + +def _own_studio_on_port(port: int, host: str) -> "int | None": + """PID of one of our own servers already bound to *port* for *host*. + + Reads our own records rather than enumerating listeners: psutil is optional, + and without it a listener scan finds nothing and we silently start a duplicate. + """ + try: + paths = list(_studio_root().glob(f"studio-{port}-*.pid")) + except OSError: + return None + for path in paths: + record = _read_pid_record(path) + if record is None: + continue + pid, created, address = record + if not _pid_alive(pid): + # Pruning is a courtesy; an undeletable record must not abort startup. + try: + path.unlink(missing_ok = True) + except OSError: + pass + continue + if not _addresses_collide(address, host, port): + continue + if _pid_is_studio_backend(pid, [created]): + return pid + return _legacy_studio_on_port(port) + + +def _legacy_studio_on_port(port: int) -> "int | None": + """A pre-upgrade server recorded only its PID, so match it to the listener. + + Falling back past one leaves it running while `_write_pid_file` overwrites the + only record of it. When the listener is unknowable, assume it is ours. + """ + record = _read_pid_record(_PID_FILE) + if record is None: + return None + pid, created, _address = record + if not _pid_alive(pid): + return None + # A current build writes a per-port file too, so its port is already known -- + # and this port's records were just checked. Only count a record that still + # matches the live process: a stale one may just share a reused PID. + for other in _per_port_records(): + if other and other[0] == pid and _pid_is_studio_backend(pid, [other[1]]): + return None + blocker = _get_pid_on_port(port) + if blocker is not None and blocker[0] != pid: + return None + if not _pid_is_studio_backend(pid, [created]): + return None + return pid + + +def _per_port_records() -> "list[tuple[int, float | None, str | None] | None]": + try: + return [_read_pid_record(p) for p in _studio_root().glob(PID_FILE_GLOB)] + except OSError: + return [] + + +def _resolve_port( + host: str, + port: int, + avoid_own_studio: bool = True, +) -> int: + """The requested port, or the next free one. + + With ``avoid_own_studio`` this aborts rather than falling back past one of our + own servers, on *port* itself or anywhere in the fallback range: skipping one + is what strands it. Callers that read the bound port back pass False and keep + the plain fallback. + """ + if _is_port_free(host, port): + return port + if avoid_own_studio: + own = _own_studio_on_port(port, host) + if own is not None: + _abort_already_running(own, port) + return _find_free_port(host, port + 1, avoid_own_studio = avoid_own_studio) + + +def _abort_already_running(pid: int, port: int) -> "NoReturn": + print( + f"Error: Unsloth Studio is already running on port {port} (PID {pid}). Run " + "`unsloth studio stop` first, or start this one on a different --port.", + file = sys.stderr, + flush = True, + ) + sys.exit(1) + # Direct backend launches bypass the CLI's env re-export; do it here for # real custom roots so unsloth-zoo's import-time LLAMA_CPP_DEFAULT_DIR @@ -770,25 +992,101 @@ if _STUDIO_ROOT_RESOLVED != _LEGACY_STUDIO_ROOT: os.environ.setdefault("UNSLOTH_IS_PRESENT", "1") -def _write_pid_file(): - """Write the current process PID to the studio PID file.""" +_OWN_PID_FILE: "Path | None" = None + + +def _write_pid_file(port: int, host: str = ""): + """Record this PID under its own port so `stop` can find every server.""" + global _OWN_PID_FILE + path = _pid_file_for_port(port) try: - _PID_FILE.parent.mkdir(parents = True, exist_ok = True) - _PID_FILE.write_text(str(os.getpid()), encoding = "utf-8") + path.parent.mkdir(parents = True, exist_ok = True) + except OSError: + pass + try: + # Start time pins the record to this process; the bind address tells a + # later launch whether this server would actually block it. + created = _process_create_time(os.getpid()) + address = ",".join(sorted(_bind_addresses(host, port))) if host else "" + body = f"{os.getpid()}\n{'' if created is None else repr(created)}\n{address}" + # Write-then-rename: `stop` reads these concurrently, and a reader that + # catches the truncate window sees a corrupt record and deletes it. + tmp = path.with_name(path.name + ".tmp") + try: + tmp.write_text(body, encoding = "utf-8") + os.replace(tmp, path) + finally: + # A failed replace would otherwise leave the scratch file behind. It + # does not end in .pid, so no glob picks it up either way. + tmp.unlink(missing_ok = True) + except OSError: + pass + else: + _OWN_PID_FILE = path + # An older CLI's `stop` only reads this one, and expects a bare PID. Written + # independently of the per-port record: if that one failed, this is the only + # thing keeping the server stoppable at all. + try: + # 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 -def _remove_pid_file(): - """Remove the PID file if it belongs to this process.""" +def _legacy_heir() -> "int | None": + """Another live server's PID, to hand the legacy studio.pid over to. + + Only one server owns studio.pid at a time, so its exit would otherwise drop + the single record an older CLI can read, stranding any sibling that is still + serving. + """ try: - if _PID_FILE.is_file(): - stored = _PID_FILE.read_text(encoding = "utf-8").strip() - if stored == str(os.getpid()): + paths = sorted(_studio_root().glob(PID_FILE_GLOB)) + except OSError: + return None + for path in paths: + if _OWN_PID_FILE is not None and path == _OWN_PID_FILE: + continue + record = _read_pid_record(path) + if record is None or record[0] == os.getpid(): + continue + if _pid_alive(record[0]) and _pid_is_studio_backend(record[0], [record[1]]): + return record[0] + return None + + +def _remove_pid_file(): + """Remove the PID files that belong to this process. + + _PID_FILE is checked even when the per-port record was never written, since + _write_pid_file writes the two independently. + """ + # Nothing here may raise: _graceful_shutdown calls this at the end, and an + # unreadable or undeletable record must not abandon the rest of the exit + # path. _read_pid_record already swallows OSError/UnicodeDecodeError. + if _OWN_PID_FILE is not None: + try: + record = _read_pid_record(_OWN_PID_FILE) if _OWN_PID_FILE.is_file() else None + if record is not None and record[0] == os.getpid(): + _OWN_PID_FILE.unlink(missing_ok = True) + except OSError: + pass + try: + record = _read_pid_record(_PID_FILE) if _PID_FILE.is_file() else None + if record is not None and record[0] == os.getpid(): + # Hand the pointer to a live sibling rather than deleting it. An + # older CLI reads only this file, so dropping it while another + # server is still up leaves that server unstoppable. + heir = _legacy_heir() + if heir is None: _PID_FILE.unlink(missing_ok = True) - # Runs first in _graceful_shutdown: a corrupt PID file raising here would - # abandon the children the rest of that function exists to kill. - except (OSError, UnicodeDecodeError): + else: + _PID_FILE.write_text(str(heir), encoding = "utf-8") + except OSError: pass @@ -798,7 +1096,6 @@ def _graceful_shutdown(server = None): Called from signal handlers to clean up children before exit. Critical on Windows where atexit handlers are unreliable after Ctrl+C. """ - _remove_pid_file() logger.info("Graceful shutdown initiated -- cleaning up subprocesses...") # 1. Shut down uvicorn (releases the listening socket). @@ -851,6 +1148,9 @@ def _graceful_shutdown(server = None): except Exception as e: logger.warning("Error in process-lifetime sweep: %s", e) + # Last: while cleanup runs the server is still alive, and dropping the record + # early leaves a retried `stop` or a new launch unable to find it. + _remove_pid_file() logger.info("All subprocesses cleaned up") @@ -1400,6 +1700,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. @@ -1533,10 +1834,16 @@ def run_server( ) # Auto-find a free port if the requested one is in use. - if not _is_port_free(host, port): - original_port = port - blocker = _get_pid_on_port(port) - port = _find_free_port(host, port + 1) + original_port = port + # 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: print("") print("=" * 50) @@ -1734,7 +2041,7 @@ def run_server( (time.perf_counter() - boot_started) * 1000, ) - _write_pid_file() + _write_pid_file(port, host) import atexit atexit.register(_remove_pid_file) diff --git a/studio/backend/tests/test_studio_pid_files.py b/studio/backend/tests/test_studio_pid_files.py new file mode 100644 index 0000000000..df2c8e87f8 --- /dev/null +++ b/studio/backend/tests/test_studio_pid_files.py @@ -0,0 +1,568 @@ +# SPDX-License-Identifier: AGPL-3.0-only +# Copyright 2026-present the Unsloth AI Inc. team. All rights reserved. See /studio/LICENSE.AGPL-3.0 + +"""Per-port PID files, so `unsloth studio stop` can find every server. + +Imports run.py directly, so run under the Unsloth venv. +""" + +from __future__ import annotations + +import os +import sys +from pathlib import Path +from types import SimpleNamespace + +import pytest + +_BACKEND = Path(__file__).resolve().parents[1] +if str(_BACKEND) not in sys.path: + sys.path.insert(0, str(_BACKEND)) + +import run # noqa: E402 + +# Captured before the autouse fixture stubs them, for the tests that exercise them. +_REAL_IS_STUDIO_BACKEND = run._pid_is_studio_backend +_REAL_PID_ALIVE = run._pid_alive + + +@pytest.fixture(autouse = True) +def isolated_root(tmp_path, monkeypatch): + monkeypatch.setattr(run, "_studio_root", lambda: tmp_path) + monkeypatch.setattr(run, "_PID_FILE", tmp_path / "studio.pid") + monkeypatch.setattr(run, "_OWN_PID_FILE", None) + monkeypatch.setattr(run, "_pid_alive", lambda pid: True) + monkeypatch.setattr(run, "_pid_is_studio_backend", lambda pid, created_times = (): True) + yield + + +def _files(tmp_path): + return sorted(p.name for p in tmp_path.glob("studio-*.pid")) + + +def _pid_of(path): + return path.read_text(encoding = "utf-8").splitlines()[0] + + +def test_write_pid_file_records_port_and_pid(tmp_path): + run._write_pid_file(8901) + + assert _files(tmp_path) == [f"studio-8901-{os.getpid()}.pid"] + assert _pid_of(tmp_path / f"studio-8901-{os.getpid()}.pid") == str(os.getpid()) + + +def test_write_pid_file_records_the_start_time(tmp_path): + # Pins the record to this process, so a reused PID isn't mistaken for it. + run._write_pid_file(8901) + + record = run._read_pid_record(tmp_path / f"studio-8901-{os.getpid()}.pid") + + assert record[0] == os.getpid() + assert record[1] == pytest.approx(run._process_create_time(os.getpid())) + + +def test_write_pid_file_keeps_the_legacy_file_a_bare_pid(tmp_path): + # An older CLI's `stop` reads studio.pid and expects only digits. + run._write_pid_file(8901) + + assert (tmp_path / "studio.pid").read_text(encoding = "utf-8") == str(os.getpid()) + + +def test_second_port_does_not_clobber_the_first(tmp_path): + (tmp_path / "studio-8901-8550.pid").write_text("8550", encoding = "utf-8") + + run._write_pid_file(8902) + + assert _pid_of(tmp_path / "studio-8901-8550.pid") == "8550" + assert (tmp_path / f"studio-8902-{os.getpid()}.pid").exists() + + +def test_same_port_on_two_binds_does_not_clobber(tmp_path): + # 127.0.0.1:8888 and ::1:8888 can both listen; one file per port would lose one. + (tmp_path / "studio-8888-8550.pid").write_text("8550", encoding = "utf-8") + + run._write_pid_file(8888) + + assert len(_files(tmp_path)) == 2 + + +def test_remove_pid_file_only_removes_our_own(tmp_path, monkeypatch): + run._write_pid_file(8901) + (tmp_path / "studio-8902-8600.pid").write_text("8600", encoding = "utf-8") + # Nothing to hand the legacy pointer to, so it goes away with us. + monkeypatch.setattr(run, "_pid_alive", lambda pid: pid == os.getpid()) + + run._remove_pid_file() + + assert _files(tmp_path) == ["studio-8902-8600.pid"] + assert not (tmp_path / "studio.pid").exists() + + +def test_the_legacy_pointer_moves_to_a_live_sibling(tmp_path): + # Only one server owns studio.pid. Deleting it on our way out would leave an + # older CLI, which reads nothing else, unable to stop the sibling still up. + run._write_pid_file(8901) + (tmp_path / "studio-8902-8600.pid").write_text("8600", encoding = "utf-8") + + run._remove_pid_file() + + assert (tmp_path / "studio.pid").read_text(encoding = "utf-8").strip() == "8600" + + +def test_the_legacy_pointer_is_not_handed_to_a_dead_sibling(tmp_path, monkeypatch): + run._write_pid_file(8901) + (tmp_path / "studio-8902-8600.pid").write_text("8600", encoding = "utf-8") + monkeypatch.setattr(run, "_pid_is_studio_backend", lambda pid, created_times = (): False) + + run._remove_pid_file() + + assert not (tmp_path / "studio.pid").exists() + + +def test_remove_pid_file_leaves_a_reused_entry_alone(tmp_path): + run._write_pid_file(8901) + own = tmp_path / f"studio-8901-{os.getpid()}.pid" + own.write_text("999999", encoding = "utf-8") + + run._remove_pid_file() + + assert own.read_text(encoding = "utf-8") == "999999" + + +def test_windows_liveness_does_not_call_every_pid_alive(monkeypatch): + # os.kill(pid, 0) raises OSError for every pid on Windows, so without the + # tasklist fallback a stale record would block its port forever. + import subprocess + + monkeypatch.setattr(run, "_pid_alive", _REAL_PID_ALIVE) + monkeypatch.setitem(sys.modules, "psutil", None) + monkeypatch.setattr(sys, "platform", "win32") + monkeypatch.setattr( + subprocess, "run", lambda *a, **k: SimpleNamespace(stdout = '"python.exe","8550",...') + ) + + assert run._pid_alive(8550) is True + assert run._pid_alive(9999) is False + + +def test_windows_liveness_keeps_the_record_when_tasklist_fails(monkeypatch): + # Unconfirmed must mean keep, matching the CLI's _pid_alive. Pruning a live + # server's record lets the next launch fall back past it and strand it, which + # is the bug this file exists to fix; a stale record costs one clear abort. + import subprocess + + def _boom(*a, **k): + raise OSError("tasklist missing") + + monkeypatch.setattr(run, "_pid_alive", _REAL_PID_ALIVE) + monkeypatch.setitem(sys.modules, "psutil", None) + monkeypatch.setattr(sys, "platform", "win32") + monkeypatch.setattr(subprocess, "run", _boom) + + assert run._pid_alive(8550) is True + + +def test_read_pid_record_parses_pid_time_and_address(tmp_path): + (tmp_path / "r.pid").write_text("8550\n111.5\n127.0.0.1", encoding = "utf-8") + + assert run._read_pid_record(tmp_path / "r.pid") == (8550, 111.5, "127.0.0.1") + + +def test_read_pid_record_tolerates_a_bare_pid(tmp_path): + (tmp_path / "r.pid").write_text("8550", encoding = "utf-8") + + assert run._read_pid_record(tmp_path / "r.pid") == (8550, None, None) + + +def test_read_pid_record_rejects_pid_zero_and_init(tmp_path): + # kill(0) signals our whole process group. + (tmp_path / "zero.pid").write_text("0", encoding = "utf-8") + (tmp_path / "init.pid").write_text("1", encoding = "utf-8") + + assert run._read_pid_record(tmp_path / "zero.pid") is None + assert run._read_pid_record(tmp_path / "init.pid") is None + + +def test_read_pid_record_rejects_a_corrupt_file(tmp_path): + (tmp_path / "r.pid").write_text("not-a-pid", encoding = "utf-8") + + assert run._read_pid_record(tmp_path / "r.pid") is None + + +def test_graceful_shutdown_drops_the_record_last(monkeypatch): + # Cleanup can take seconds while the server is still alive. Dropping the record + # first leaves a retried `stop` or a new launch unable to find it. + order = [] + monkeypatch.setattr(run, "_remove_pid_file", lambda: order.append("remove_record")) + + class _Server: + def __setattr__(self, name, value): + order.append("release_socket") + + run._graceful_shutdown(_Server()) + + assert order == ["release_socket", "remove_record"] + + +def test_own_studio_on_port_is_found_without_psutil(tmp_path, monkeypatch): + # psutil is optional; a listener scan finds nothing without it, so detection + # must come from our own records or we silently start a duplicate. + monkeypatch.setitem(sys.modules, "psutil", None) + (tmp_path / "studio-8901-8550.pid").write_text("8550\n\n127.0.0.1", encoding = "utf-8") + + assert run._own_studio_on_port(8901, "127.0.0.1") == 8550 + + +def test_no_record_for_the_port_means_no_own_studio(tmp_path): + # jupyter-lab on 8888 must keep the fallback, not abort the launch. + (tmp_path / "studio-8901-8550.pid").write_text("8550", encoding = "utf-8") + + assert run._own_studio_on_port(8888, "127.0.0.1") is None + + +def test_own_studio_on_port_prunes_a_dead_record(tmp_path, monkeypatch): + monkeypatch.setattr(run, "_pid_alive", lambda pid: False) + (tmp_path / "studio-8901-8550.pid").write_text("8550", encoding = "utf-8") + + assert run._own_studio_on_port(8901, "127.0.0.1") is None + assert not (tmp_path / "studio-8901-8550.pid").exists() + + +def test_a_reused_pid_is_not_treated_as_our_studio(tmp_path, monkeypatch): + # Stale record + the OS handing that PID to something else must not abort. + monkeypatch.setattr(run, "_pid_is_studio_backend", lambda pid, created_times = (): False) + (tmp_path / "studio-8901-8550.pid").write_text("8550", encoding = "utf-8") + + assert run._own_studio_on_port(8901, "127.0.0.1") is None + + +def test_an_unverifiable_record_still_blocks_a_duplicate(tmp_path, monkeypatch): + # Can't tell: refusing with a clear message beats a silent second instance. + monkeypatch.setattr(run, "_pid_is_studio_backend", lambda pid, created_times = (): True) + (tmp_path / "studio-8901-8550.pid").write_text("8550", encoding = "utf-8") + + assert run._own_studio_on_port(8901, "127.0.0.1") == 8550 + + +def test_start_time_mismatch_rejects_a_reused_pid(monkeypatch): + 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]) is False + assert run._pid_is_studio_backend(8550, [999.0]) is True + + +def test_a_stale_record_does_not_veto_a_live_server_sharing_the_pid(monkeypatch): + # Crash leaves studio-8888-1234.pid, the OS reuses 1234 for a new server on + # another port. Keeping only the first timestamp would reject the live one. + 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(1234, [111.5, 999.0]) is True + assert run._pid_is_studio_backend(1234, [111.5, 222.5]) is False + + +def test_a_stale_record_on_another_port_does_not_hide_a_live_server(tmp_path, monkeypatch): + # 1234 was reused: the stale 8888 record must not stop us seeing 9000. + monkeypatch.setattr(run, "_pid_is_studio_backend", _REAL_IS_STUDIO_BACKEND) + monkeypatch.setattr(run, "_process_create_time", lambda pid: 999.0) + (tmp_path / "studio-8888-1234.pid").write_text("1234\n111.5\n", encoding = "utf-8") + (tmp_path / "studio-9000-1234.pid").write_text("1234\n999.0\n", encoding = "utf-8") + + assert run._own_studio_on_port(8888, "127.0.0.1") is None + assert run._own_studio_on_port(9000, "127.0.0.1") == 1234 + + +def test_a_start_time_is_the_only_thing_that_disproves_a_record(monkeypatch): + 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, [999.0]) is True + assert run._pid_is_studio_backend(8550, [111.5]) is False + + +def test_a_bare_run_py_command_line_is_not_rejected(monkeypatch): + # `cd studio/backend && python run.py --port 8901` has no "studio" or "unsloth" + # in argv. Guessing from the command line called that "not ours". + monkeypatch.setattr(run, "_pid_is_studio_backend", _REAL_IS_STUDIO_BACKEND) + + class _FakeProcess: + def __init__(self, pid): + self.pid = pid + + def cmdline(self): + return ["python", "run.py", "--port", "8901"] + + def create_time(self): + return 111.5 + + monkeypatch.setitem(sys.modules, "psutil", SimpleNamespace(Process = _FakeProcess)) + + assert run._pid_is_studio_backend(8550) is True + + +def test_an_untimed_legacy_record_is_trusted(monkeypatch): + # `python run.py --port 8901` has no telltale argv, so guessing from the + # command line rejected real servers. Only a start time can disprove one. + 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) is True + assert run._pid_is_studio_backend(8550, [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): + # Pre-upgrade servers wrote only studio.pid. Falling back past one strands it + # and then overwrites its record. + monkeypatch.setattr(run, "_get_pid_on_port", lambda p: (8550, "python")) + (tmp_path / "studio.pid").write_text("8550", encoding = "utf-8") + + assert run._own_studio_on_port(8901, "127.0.0.1") == 8550 + + +def test_a_legacy_record_for_a_different_listener_falls_back(tmp_path, monkeypatch): + # jupyter holds the port; the legacy server is elsewhere. Keep falling back. + monkeypatch.setattr(run, "_get_pid_on_port", lambda p: (117, "jupyter-lab")) + (tmp_path / "studio.pid").write_text("8550", encoding = "utf-8") + + assert run._own_studio_on_port(8901, "127.0.0.1") is None + + +def test_an_unknowable_listener_treats_the_legacy_record_as_ours(tmp_path, monkeypatch): + # No psutil: _get_pid_on_port can't say. Refusing beats a silent duplicate. + monkeypatch.setattr(run, "_get_pid_on_port", lambda p: None) + (tmp_path / "studio.pid").write_text("8550", encoding = "utf-8") + + assert run._own_studio_on_port(8901, "127.0.0.1") == 8550 + + +def test_a_dead_legacy_record_falls_back(tmp_path, monkeypatch): + monkeypatch.setattr(run, "_pid_alive", lambda pid: False) + monkeypatch.setattr(run, "_get_pid_on_port", lambda p: None) + (tmp_path / "studio.pid").write_text("8550", encoding = "utf-8") + + assert run._own_studio_on_port(8901, "127.0.0.1") is None + + +def test_a_stale_per_port_record_does_not_mask_a_legacy_server(tmp_path, monkeypatch): + # Crashed current build left studio-8901-8550.pid; 8550 was then reused by a + # pre-upgrade server recorded only in studio.pid. The stale record must not + # count as "port already known" and send us falling back past the live one. + monkeypatch.setattr(run, "_pid_is_studio_backend", _REAL_IS_STUDIO_BACKEND) + monkeypatch.setattr(run, "_process_create_time", lambda pid: 999.0) + monkeypatch.setattr(run, "_get_pid_on_port", lambda p: (8550, "python")) + (tmp_path / "studio-8901-8550.pid").write_text("8550\n111.5\n127.0.0.1", encoding = "utf-8") + (tmp_path / "studio.pid").write_text("8550", encoding = "utf-8") + + assert run._own_studio_on_port(8901, "127.0.0.1") == 8550 + + +def test_a_current_server_elsewhere_does_not_block_a_foreign_port(tmp_path, monkeypatch): + # Current builds write studio.pid too. Without psutil the legacy check can't + # see the listener, so it must not claim our 8901 server holds jupyter's 8888. + monkeypatch.setattr(run, "_get_pid_on_port", lambda p: None) + (tmp_path / "studio-8901-5000.pid").write_text("5000\n\n127.0.0.1", encoding = "utf-8") + (tmp_path / "studio.pid").write_text("5000", encoding = "utf-8") + + assert run._own_studio_on_port(8888, "127.0.0.1") is None + + +def test_a_per_port_record_is_preferred_over_the_legacy_one(tmp_path, monkeypatch): + monkeypatch.setattr(run, "_get_pid_on_port", lambda p: (8550, "python")) + (tmp_path / "studio-8901-8600.pid").write_text("8600\n\n127.0.0.1", encoding = "utf-8") + (tmp_path / "studio.pid").write_text("8550", encoding = "utf-8") + + assert run._own_studio_on_port(8901, "127.0.0.1") == 8600 + + +def test_our_studio_on_another_bind_address_does_not_abort(tmp_path): + # Our server holds ::1:8889; binding 127.0.0.1:8889 is not a conflict with us, + # so fall through to the next port instead of refusing. + (tmp_path / "studio-8889-8550.pid").write_text("8550\n\n::1", encoding = "utf-8") + + assert run._own_studio_on_port(8889, "127.0.0.1") is None + assert run._own_studio_on_port(8889, "::1") == 8550 + + +def test_address_matching(tmp_path): + assert run._addresses_collide("0.0.0.0", "127.0.0.1", 8889) is True + assert run._addresses_collide("127.0.0.1", "0.0.0.0", 8889) is True + assert run._addresses_collide("127.0.0.1", "127.0.0.1", 8889) is True + assert run._addresses_collide("::1", "127.0.0.1", 8889) is False + # An unrecorded address is unknown, so assume a conflict. + assert run._addresses_collide(None, "127.0.0.1", 8889) is True + + +def test_a_hostname_resolves_the_same_way_the_bind_does(tmp_path): + # `localhost` and the address _is_port_free actually binds must agree, or a + # recorded server is missed and a duplicate starts. + recorded = ",".join(sorted(run._bind_addresses("localhost", 8889))) + + assert run._addresses_collide(recorded, "localhost", 8889) is True + + +def test_a_hostname_records_every_address_it_resolves_to(tmp_path): + # `localhost` binds 127.0.0.1 AND ::1. Recording only the first lets a later + # launch on the other literal miss us and start a duplicate. + addrs = run._bind_addresses("localhost", 8889) + recorded = ",".join(sorted(addrs)) + + for literal in addrs: + assert run._addresses_collide(recorded, literal, 8889) is True + + +def test_a_multi_address_record_matches_either_literal(tmp_path): + recorded = "127.0.0.1,::1" + + assert run._addresses_collide(recorded, "127.0.0.1", 8889) is True + assert run._addresses_collide(recorded, "::1", 8889) is True + assert run._addresses_collide("127.0.0.1", "::1", 8889) is False + + +def test_fallback_aborts_on_our_own_server_further_up_the_range(tmp_path, monkeypatch): + # jupyter holds 8888, our server holds 8889: skipping to 8890 is the duplicate. + (tmp_path / "studio-8889-8550.pid").write_text("8550\n\n127.0.0.1", encoding = "utf-8") + monkeypatch.setattr(run, "_is_port_free", lambda host, p: p >= 8890) + + with pytest.raises(SystemExit) as excinfo: + run._find_free_port("127.0.0.1", 8889, avoid_own_studio = True) + + assert excinfo.value.code == 1 + + +def test_fallback_still_skips_foreign_processes(tmp_path, monkeypatch): + # No record for 8889, so the blocker is not ours: keep falling back. + monkeypatch.setattr(run, "_is_port_free", lambda host, p: p >= 8890) + + assert run._find_free_port("127.0.0.1", 8889, avoid_own_studio = True) == 8890 + + +def test_the_requested_port_is_kept_when_it_is_free(monkeypatch): + monkeypatch.setattr(run, "_is_port_free", lambda host, p: True) + + assert run._resolve_port("127.0.0.1", 8888) == 8888 + + +def test_our_own_server_on_the_requested_port_aborts_rather_than_falling_back( + tmp_path, monkeypatch +): + # The reported bug: 8888 is ours, so falling back to 8889 is the duplicate + # that leaves 8888 serving with nothing recording it. + monkeypatch.setattr(run, "_is_port_free", lambda host, p: p != 8888) + (tmp_path / "studio-8888-8550.pid").write_text("8550\n\n127.0.0.1", encoding = "utf-8") + + with pytest.raises(SystemExit) as excinfo: + run._resolve_port("127.0.0.1", 8888) + + assert excinfo.value.code == 1 + + +def test_a_foreign_process_on_the_requested_port_still_falls_back(monkeypatch): + # jupyter-lab on 8888 must not stop Unsloth starting on 8889. + monkeypatch.setattr(run, "_is_port_free", lambda host, p: p != 8888) + + assert run._resolve_port("127.0.0.1", 8888) == 8889 + + +def test_a_caller_that_reads_the_port_back_keeps_the_plain_fallback(tmp_path, monkeypatch): + # api-only callers (the desktop app via TAURI_PORT, `studio run` via + # app.state.server_port) follow us to the new port, so aborting there only + # turns a working launch into a crash the desktop app reports as "stopped + # unexpectedly". Both servers are still recorded, so `stop` finds them. + monkeypatch.setattr(run, "_is_port_free", lambda host, p: p != 8888) + (tmp_path / "studio-8888-8550.pid").write_text("8550\n\n127.0.0.1", encoding = "utf-8") + + assert run._resolve_port("127.0.0.1", 8888, avoid_own_studio = False) == 8889 + + +def test_the_recorded_address_is_every_address_the_bind_resolves_to(tmp_path): + # The only test that runs the writer with a real host. Recording `host` + # verbatim, or dropping the line, passes every other test here and silently + # stops matching a launch that spells the same interface differently. + run._write_pid_file(8901, "localhost") + + record = run._read_pid_record(tmp_path / f"studio-8901-{os.getpid()}.pid") + + assert record[2] is not None, "no bind address recorded" + assert set(record[2].split(",")) == run._bind_addresses("localhost", 8901) + + +def test_a_server_started_on_a_hostname_is_found_again_by_ip(tmp_path): + run._write_pid_file(8901, "localhost") + + for literal in run._bind_addresses("localhost", 8901): + assert run._own_studio_on_port(8901, literal) == os.getpid() + + +def test_bind_addresses_keeps_every_family_a_hostname_resolves_to(monkeypatch): + # Independent oracle: the sibling test derives its expectation from this + # function's own output, so dropping a family would pass it. + import socket + monkeypatch.setattr( + socket, + "getaddrinfo", + lambda *a, **k: [ + (socket.AF_INET, socket.SOCK_STREAM, 6, "", ("127.0.0.1", 8889)), + (socket.AF_INET6, socket.SOCK_STREAM, 6, "", ("::1", 8889, 0, 0)), + ], + ) + + assert run._bind_addresses("localhost", 8889) == {"127.0.0.1", "::1"} + + +def test_the_legacy_file_is_written_even_when_the_per_port_record_fails(tmp_path, monkeypatch): + # A studio root that cannot take a new entry used to leave the server + # recorded nowhere at all, so the CLI could not stop it. studio.pid is an + # overwrite of an existing path, so it can still succeed and must be tried. + blocked = tmp_path / "not-a-directory" + blocked.write_text("", encoding = "utf-8") + monkeypatch.setattr( + run, "_pid_file_for_port", lambda port: blocked / f"studio-{port}-{os.getpid()}.pid" + ) + + run._write_pid_file(8901, "127.0.0.1") + + assert (tmp_path / "studio.pid").read_text(encoding = "utf-8") == str(os.getpid()) + assert run._OWN_PID_FILE is None + + +def test_a_record_whose_pid_is_not_ascii_digits_is_discarded(tmp_path): + # A superscript two passes isdigit() but int() rejects it, so that gate alone + # let a ValueError escape into every caller of _read_pid_record. + (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()) diff --git a/tests/studio/test_cli_studio_stop_windows.py b/tests/studio/test_cli_studio_stop_windows.py index 2267d7feda..cef7cc6db7 100644 --- a/tests/studio/test_cli_studio_stop_windows.py +++ b/tests/studio/test_cli_studio_stop_windows.py @@ -44,9 +44,12 @@ def _load_pid_alive(platform: str, fake_run = None): # ── AST: stop() must not use the broken bare liveness probe ────────────────── -def test_stop_does_not_use_bare_oskill_liveness_probe(): - """stop() must not call os.kill(pid, 0) -- it crashes on Windows.""" - stop_src = _func_source("stop") +# `stop` delegates signalling to `_signal_stop`, so guarding only `stop` would +# let os.kill(pid, 0) come back one function along and still pass. +@pytest.mark.parametrize("func", ["stop", "_signal_stop"]) +def test_stop_does_not_use_bare_oskill_liveness_probe(func): + """The signalling path must not call os.kill(pid, 0) -- WinError 87 on Windows.""" + stop_src = _func_source(func) tree = ast.parse(stop_src) for call in ast.walk(tree): if not isinstance(call, ast.Call): @@ -62,14 +65,17 @@ def test_stop_does_not_use_bare_oskill_liveness_probe(): sig = call.args[1] if isinstance(sig, ast.Constant) and sig.value == 0: raise AssertionError( - "stop() still uses os.kill(pid, 0); it raises WinError 87 on " - "Windows. Use the cross-platform _pid_alive() helper instead." + f"{func}() still uses os.kill(pid, 0); it raises WinError 87 " + "on Windows. Use the cross-platform _pid_alive() helper." ) def test_pid_alive_helper_is_defined_and_used_by_stop(): assert "def _pid_alive(" in _SOURCE, "_pid_alive helper missing" assert "_pid_alive(pid)" in _func_source("stop"), "stop() must use _pid_alive" + # The kill itself moved into _signal_stop; keep both ends of the path pinned. + assert "def _signal_stop(" in _SOURCE, "_signal_stop helper missing" + assert "taskkill" in _func_source("_signal_stop") # The helper must special-case Windows via tasklist (os.kill(pid,0) is invalid there). helper = _func_source("_pid_alive") assert 'sys.platform == "win32"' in helper diff --git a/tests/studio/test_studio_pid_file_contract.py b/tests/studio/test_studio_pid_file_contract.py new file mode 100644 index 0000000000..23ace706b5 --- /dev/null +++ b/tests/studio/test_studio_pid_file_contract.py @@ -0,0 +1,73 @@ +# SPDX-License-Identifier: AGPL-3.0-only +# Copyright 2026-present the Unsloth AI Inc. team. All rights reserved. + +"""run.py writes the Studio PID files; `unsloth studio stop` globs for them. + +Nothing else ties the writer's filename to the reader's glob, and each side's own +tests hardcode the names they expect, so a rename on either side alone leaves both +suites green while `stop` silently finds nothing. `unsloth_cli/tests/` also runs +in no workflow, so this lives here, where the repo CPU job discovers it. + +AST + exec of the writer, so no backend dependency stack is imported. +""" + +import ast +import os +import sys +from pathlib import Path + +_ROOT = Path(__file__).resolve().parents[2] +if str(_ROOT) not in sys.path: + sys.path.insert(0, str(_ROOT)) + +_RUN_SRC = (_ROOT / "studio" / "backend" / "run.py").read_text(encoding = "utf-8") + + +def _func_source(source: str, name: str) -> str: + for node in ast.walk(ast.parse(source)): + if isinstance(node, ast.FunctionDef) and node.name == name: + return ast.get_source_segment(source, node) + raise AssertionError(f"function {name!r} not found") + + +def _backend_pid_path(root: Path, port: int) -> Path: + """The path run.py's own _pid_file_for_port builds, without importing run.py.""" + ns = {"os": os, "Path": Path, "_studio_root": lambda: root} + exec(_func_source(_RUN_SRC, "_pid_file_for_port"), ns) + return ns["_pid_file_for_port"](port) + + +def test_stop_finds_a_pid_file_named_the_way_the_backend_writes_it(tmp_path, monkeypatch): + from unsloth_cli.commands import studio as cli + + path = _backend_pid_path(tmp_path, 8901) + # The same three-line body _write_pid_file emits (create_time is blank when + # psutil is unavailable, and the CLI must tolerate that). + path.write_text(f"{os.getpid()}\n\n127.0.0.1", encoding = "utf-8") + + monkeypatch.setattr(cli, "STUDIO_HOME", tmp_path) + monkeypatch.setattr(cli, "_PID_FILE", tmp_path / "studio.pid") + + assert [pid for pid, _times, _files in cli._pid_file_entries()] == [os.getpid()] + + +def test_the_legacy_file_stays_a_bare_pid_an_older_cli_can_parse(tmp_path, monkeypatch): + # An older `unsloth studio stop` reads studio.pid and requires str.isdigit(), + # so the compatibility file must never gain the extra metadata lines. + ns = { + "os": os, + "Path": Path, + "_studio_root": lambda: tmp_path, + "_PID_FILE": tmp_path / "studio.pid", + "_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) + ns["_write_pid_file"](8901, "127.0.0.1") + + assert (tmp_path / "studio.pid").read_text(encoding = "utf-8").strip().isdigit() diff --git a/unsloth_cli/commands/studio.py b/unsloth_cli/commands/studio.py index 68a5a6357d..560df4aea2 100644 --- a/unsloth_cli/commands/studio.py +++ b/unsloth_cli/commands/studio.py @@ -19,7 +19,7 @@ import urllib.error import urllib.request from datetime import datetime, timezone from pathlib import Path -from typing import List, Literal, Optional +from typing import List, Literal, Optional, Sequence import typer from unsloth_cli import _studio_deps @@ -2265,6 +2265,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: @@ -2424,6 +2427,7 @@ def run( # ── unsloth studio stop ─────────────────────────────────────────────── _PID_FILE = STUDIO_HOME / "studio.pid" +PID_FILE_GLOB = "studio-*.pid" def _pid_alive(pid: int) -> bool: @@ -2453,58 +2457,210 @@ def _pid_alive(pid: int) -> bool: return True -@studio_app.command() -def stop(): - """Stop a running Unsloth Studio server. +def _parse_pid_record(text: str) -> "tuple[int, float | None] | None": + """Parse ``pid`` / optional ``create_time`` from PID file contents.""" + lines = text.splitlines() + if not lines or not lines[0].strip().isdigit(): + return None + try: + # isdigit() is not enough: "²".isdigit() is True but int() rejects it. + pid = int(lines[0].strip()) + except ValueError: + return None + # kill(0) signals our whole process group; kill(1) is init. Never either. + if pid < 2: + return None + created = None + if len(lines) > 1: + try: + created = float(lines[1].strip()) + except ValueError: + created = None + return pid, created - Reads the PID from ~/.unsloth/studio/studio.pid and sends SIGTERM - (or TerminateProcess on Windows) to shut it down gracefully. + +def _read_pid_record(path: Path) -> "tuple[int, float | None] | None": + """Parse ``pid`` / optional ``create_time`` from a PID file.""" + try: + text = path.read_text(encoding = "utf-8") + except (OSError, UnicodeDecodeError): + return None + return _parse_pid_record(text) + + +def _unlink_quietly(path: Path) -> None: + """Drop a record without letting one bad file end the loop. + + An undeletable record must not stop us reaching the other servers -- that is + the orphan this command exists to prevent. """ + try: + path.unlink(missing_ok = True) + except OSError as e: + typer.echo(f"Could not remove PID file {path.name}: {e}", err = True) + + +def _report_unreadable(paths: "list[Path]") -> None: + """Say which servers we could not reach, since `stop` is about to exit 1.""" + names = ", ".join(sorted(p.name for p in paths)) + typer.echo( + f"Could not read {len(paths)} PID file(s): {names}. A server recorded " + f"there may still be running; re-run with permission to read " + f"{STUDIO_HOME} to stop it.", + err = True, + ) + + +def _pid_file_entries( + unreadable: "list[Path] | None" = None, +) -> "list[tuple[int, list[float | None], list[Path]]]": + """(pid, create_times, files) per recorded server, including the legacy studio.pid. + + Paths that could not be read are appended to `unreadable` when given, so the + caller can tell "nothing is running" apart from "something is running and we + could not see it". + + Grouped by PID: a server writes both its per-port file and studio.pid, and + signalling twice would hit the SIG_DFL the first SIGTERM installs, hard-killing + it mid-shutdown. Every recorded time is kept -- a stale file and a live server + can share a PID, and the stale one must not veto the live one. + """ + by_pid: "dict[int, tuple[list[float | None], list[Path]]]" = {} + try: + paths = sorted(STUDIO_HOME.glob(PID_FILE_GLOB)) + [_PID_FILE] + except OSError: + paths = [_PID_FILE] + seen = set() + for path in paths: + if path in seen or not path.is_file(): + continue + seen.add(path) + try: + text = path.read_text(encoding = "utf-8") + except (OSError, UnicodeDecodeError) as e: + # Unreadable is not the same as invalid. A root-owned record, or one + # caught mid-write, still belongs to a live server, and deleting it + # strands that server -- the bug this command exists to fix. + typer.echo(f"Cannot read PID file {path.name}: {e}", err = True) + if unreadable is not None: + unreadable.append(path) + continue + record = _parse_pid_record(text) + if record is None: + typer.echo(f"Ignoring invalid PID file {path.name}") + _unlink_quietly(path) + continue + pid, created = record + created_times, files = by_pid.setdefault(pid, ([], [])) + created_times.append(created) + files.append(path) + return [(pid, times, files) for pid, (times, files) in by_pid.items()] + + +def _pid_is_studio_server(pid: int, created_times: "Sequence[float | None]" = ()) -> bool: + """False only when a recorded start time proves this PID is a different process. + + Any recorded time matching is enough -- a stale record must not veto a live + server that reused the PID. Records with no time at all (a legacy studio.pid, + or a server started without psutil) cannot be checked, so they are trusted: + the old `stop` signalled with no checks at all, and skipping a live server is + the orphan bug this exists to fix. + + An untimed record sitting *alongside* a timed one carries no information, so + it must not cancel the timed one either. Every current server writes both a + timed per-port record and an untimed studio.pid, so letting the untimed half + win made this check inert exactly where it matters and let `stop` SIGTERM an + unrelated process that had inherited the PID. + """ + known = [c for c in created_times if c is not None] + if not known: + return True + try: + import psutil + actual = psutil.Process(pid).create_time() + except Exception: + return True + return any(abs(actual - c) < 1.0 for c in known) + + +def _signal_stop(pid: int) -> "str | None": + """SIGTERM (or taskkill) the pid. Returns an error string, or None on success.""" import signal as _signal - if not _PID_FILE.is_file(): - typer.echo("No running Unsloth server found (no PID file).") - raise typer.Exit(0) - - pid_text = _PID_FILE.read_text(encoding = "utf-8").strip() - if not pid_text.isdigit(): - typer.echo(f"Invalid PID file contents: {pid_text}") - _PID_FILE.unlink(missing_ok = True) - raise typer.Exit(1) - - pid = int(pid_text) - - # Check if still alive (os.kill(pid, 0) is invalid on Windows -- see _pid_alive). - if not _pid_alive(pid): - typer.echo(f"Unsloth server (PID {pid}) is not running. Cleaning up stale PID file.") - _PID_FILE.unlink(missing_ok = True) - raise typer.Exit(0) - - # Send SIGTERM (graceful shutdown) or TerminateProcess on Windows + if pid < 2: + return f"refusing to signal PID {pid}" try: if sys.platform == "win32": # /T also stops llama-server children, which otherwise keep GPU and port. subprocess.run(["taskkill", "/PID", str(pid), "/T", "/F"], check = True) else: os.kill(pid, _signal.SIGTERM) - typer.echo(f"Sent shutdown signal to Unsloth server (PID {pid}).") except ProcessLookupError: - typer.echo(f"Unsloth server (PID {pid}) already exited.") - _PID_FILE.unlink(missing_ok = True) - raise typer.Exit(0) + return None except Exception as e: - typer.echo(f"Failed to stop Unsloth server (PID {pid}): {e}", err = True) - raise typer.Exit(1) + return str(e) + return None - # Wait briefly for the process to exit and clean up. + +@studio_app.command() +def stop(): + """Stop every running Unsloth Studio server for this STUDIO_HOME. + + The port fallback can leave more than one running, so stop them all. + """ + unreadable: "list[Path]" = [] + entries = _pid_file_entries(unreadable) + if not entries: + if unreadable: + # Reporting success here would be a lie: the records we could not + # read are kept, and the servers behind them are still serving. + _report_unreadable(unreadable) + raise typer.Exit(1) + typer.echo("No running Unsloth server found (no PID file).") + raise typer.Exit(0) + + signalled, failed = [], [] + for pid, created_times, paths in entries: + if not _pid_alive(pid) or not _pid_is_studio_server(pid, created_times): + for path in paths: + _unlink_quietly(path) + continue + error = _signal_stop(pid) + if error is not None: + failed.append((pid, error)) + typer.echo(f"Failed to stop Unsloth server (PID {pid}): {error}", err = True) + continue + typer.echo(f"Sent shutdown signal to Unsloth server (PID {pid}).") + signalled.append((pid, paths)) + + if not signalled and not failed: + if unreadable: + _report_unreadable(unreadable) + raise typer.Exit(1) + typer.echo("No running Unsloth server found (cleaned up stale PID files).") + raise typer.Exit(0) + + pending = list(signalled) for _ in range(10): + if not pending: + break time.sleep(0.5) - if not _pid_alive(pid): - _PID_FILE.unlink(missing_ok = True) - typer.echo("Unsloth server stopped.") - raise typer.Exit(0) + for entry in list(pending): + pid, paths = entry + if not _pid_alive(pid): + for path in paths: + _unlink_quietly(path) + pending.remove(entry) - typer.echo("Unsloth server is shutting down (may take a few seconds).") + stopped = len(signalled) - len(pending) + if stopped: + typer.echo(f"Unsloth server{'s' if stopped > 1 else ''} stopped ({stopped}).") + for pid, _paths in pending: + typer.echo(f"Unsloth server (PID {pid}) is shutting down (may take a few seconds).") + if unreadable: + _report_unreadable(unreadable) + if failed or unreadable: + raise typer.Exit(1) # ── unsloth studio setup / update ───────────────────────────────────── diff --git a/unsloth_cli/tests/test_studio_stop.py b/unsloth_cli/tests/test_studio_stop.py new file mode 100644 index 0000000000..74e34d4fa1 --- /dev/null +++ b/unsloth_cli/tests/test_studio_stop.py @@ -0,0 +1,530 @@ +# SPDX-License-Identifier: AGPL-3.0-only +# Copyright 2026-present the Unsloth AI Inc. team. All rights reserved. See /studio/LICENSE.AGPL-3.0 + +"""`unsloth studio stop` must stop every server it started. + +With one PID file the second launch overwrote the first entry, so stop killed +the newer server, claimed success, and left the older one serving. +""" + +from __future__ import annotations + +import sys +from pathlib import Path +from types import SimpleNamespace + +import pytest +from typer.testing import CliRunner + + +_REPO_ROOT = Path(__file__).resolve().parents[2] +if str(_REPO_ROOT) not in sys.path: + sys.path.insert(0, str(_REPO_ROOT)) + + +def _studio(): + from unsloth_cli.commands import studio as _studio_mod + return _studio_mod + + +# Captured before _install stubs it, for the tests that exercise it. +_REAL_IS_STUDIO_SERVER = _studio()._pid_is_studio_server + + +def _install( + monkeypatch, + tmp_path, + *, + alive, + killed = None, +): + """Point the CLI at tmp_path and fake process liveness.""" + studio_mod = _studio() + monkeypatch.setattr(studio_mod, "STUDIO_HOME", tmp_path) + monkeypatch.setattr(studio_mod, "_PID_FILE", tmp_path / "studio.pid") + monkeypatch.setattr(studio_mod.time, "sleep", lambda _s: None) + + live = set(alive) + killed = killed if killed is not None else [] + + monkeypatch.setattr(studio_mod, "_pid_alive", lambda pid: pid in live) + monkeypatch.setattr(studio_mod, "_pid_is_studio_server", lambda pid, created_times = (): True) + + def fake_kill(pid, _sig): + killed.append(pid) + live.discard(pid) + + monkeypatch.setattr(studio_mod.os, "kill", fake_kill) + monkeypatch.setattr(sys, "platform", "linux") + return studio_mod, live, killed + + +def _write_pid(tmp_path, name, pid): + (tmp_path / name).write_text(str(pid), encoding = "utf-8") + + +def _run_stop(studio_mod): + import typer as _typer + + app = _typer.Typer() + app.add_typer(studio_mod.studio_app, name = "studio") + return CliRunner().invoke(app, ["studio", "stop"]) + + +def test_stop_kills_every_recorded_server(monkeypatch, tmp_path): + studio_mod, _live, killed = _install(monkeypatch, tmp_path, alive = {8550, 8600}) + _write_pid(tmp_path, "studio-8901-8550.pid", 8550) + _write_pid(tmp_path, "studio-8902-8600.pid", 8600) + + result = _run_stop(studio_mod) + + assert result.exit_code == 0, result.output + assert sorted(killed) == [8550, 8600] + assert not list(tmp_path.glob("studio-*.pid")) + + +def test_stop_does_not_leave_the_older_instance_running(monkeypatch, tmp_path): + # The reported symptom: stop claimed success while instance A kept serving. + studio_mod, live, _killed = _install(monkeypatch, tmp_path, alive = {8550, 8600}) + _write_pid(tmp_path, "studio-8901-8550.pid", 8550) + _write_pid(tmp_path, "studio-8902-8600.pid", 8600) + + result = _run_stop(studio_mod) + + assert result.exit_code == 0, result.output + assert live == set() + + +def test_stop_signals_each_server_once(monkeypatch, tmp_path): + # A server writes its per-port file AND studio.pid. It stays alive while it + # shuts down gracefully, so a second SIGTERM would hit the SIG_DFL the first + # one installs and hard-kill it mid-cleanup. + studio_mod = _studio() + monkeypatch.setattr(studio_mod, "STUDIO_HOME", tmp_path) + monkeypatch.setattr(studio_mod, "_PID_FILE", tmp_path / "studio.pid") + monkeypatch.setattr(studio_mod.time, "sleep", lambda _s: None) + monkeypatch.setattr(studio_mod, "_pid_alive", lambda pid: True) + monkeypatch.setattr(studio_mod, "_pid_is_studio_server", lambda pid, created_times = (): True) + killed = [] + monkeypatch.setattr(studio_mod.os, "kill", lambda pid, _sig: killed.append(pid)) + monkeypatch.setattr(sys, "platform", "linux") + _write_pid(tmp_path, "studio-8901-8550.pid", 8550) + _write_pid(tmp_path, "studio.pid", 8550) + + result = _run_stop(studio_mod) + + assert result.exit_code == 0, result.output + assert killed == [8550] + assert result.output.lower().count("sent shutdown signal") == 1 + + +def test_stop_removes_every_stale_file_for_one_pid(monkeypatch, tmp_path): + studio_mod, _live, killed = _install(monkeypatch, tmp_path, alive = set()) + _write_pid(tmp_path, "studio-8901-8550.pid", 8550) + _write_pid(tmp_path, "studio.pid", 8550) + + result = _run_stop(studio_mod) + + assert result.exit_code == 0, result.output + assert killed == [] + assert not list(tmp_path.glob("*.pid")) + + +def test_stop_does_not_signal_a_reused_pid(monkeypatch, tmp_path): + # Crash leaves a per-port file behind, the OS hands that PID to something + # else: stop must drop the record, not SIGTERM an unrelated process. + studio_mod, _live, killed = _install(monkeypatch, tmp_path, alive = {8550}) + monkeypatch.setattr(studio_mod, "_pid_is_studio_server", lambda pid, created_times = (): False) + _write_pid(tmp_path, "studio-8901-8550.pid", 8550) + + result = _run_stop(studio_mod) + + assert result.exit_code == 0, result.output + assert killed == [] + assert not (tmp_path / "studio-8901-8550.pid").exists() + + +def test_stop_signals_a_live_server_whose_pid_has_a_stale_record(monkeypatch, tmp_path): + # Crash leaves studio-8888-8550.pid, the OS reuses 8550 for a new server on + # another port. The stale timestamp must not veto the live one. + studio_mod, _live, killed = _install(monkeypatch, tmp_path, alive = {8550}) + monkeypatch.setattr(studio_mod, "_pid_is_studio_server", _REAL_IS_STUDIO_SERVER) + + class _FakeProcess: + def __init__(self, pid): + self.pid = pid + + def create_time(self): + return 999.0 + + monkeypatch.setitem(sys.modules, "psutil", SimpleNamespace(Process = _FakeProcess)) + (tmp_path / "studio-8888-8550.pid").write_text("8550\n111.5", encoding = "utf-8") + (tmp_path / "studio-9000-8550.pid").write_text("8550\n999.0", encoding = "utf-8") + + result = _run_stop(studio_mod) + + assert result.exit_code == 0, result.output + assert killed == [8550] + assert not list(tmp_path.glob("studio-*.pid")) + + +def test_a_bare_run_py_command_line_is_not_rejected(monkeypatch): + # `cd studio/backend && python run.py --port 8901` has no "studio" or "unsloth" + # in argv. Guessing from the command line deleted its record without stopping it. + studio_mod = _studio() + + class _FakeProcess: + def __init__(self, pid): + self.pid = pid + + def cmdline(self): + return ["python", "run.py", "--port", "8901"] + + def create_time(self): + return 111.5 + + monkeypatch.setitem(sys.modules, "psutil", SimpleNamespace(Process = _FakeProcess)) + + assert studio_mod._pid_is_studio_server(8550) is True + + +def test_an_untimed_record_is_trusted(monkeypatch): + # A legacy `python run.py --port 8901` has no telltale argv, and the in-venv + # path runs in-process. Guessing from the command line rejected real servers. + studio_mod = _studio() + + assert studio_mod._pid_is_studio_server(8550) is True + assert studio_mod._pid_is_studio_server(8550, [None]) is True + + +def test_an_unverifiable_record_is_still_stopped(monkeypatch): + # psutil is not a base CLI dependency, so the CLI meets timestamped records it + # cannot check. The old `stop` signalled with no checks at all -- skipping one + # would leave a live server running, the orphan bug this exists to fix. + studio_mod = _studio() + monkeypatch.setitem(sys.modules, "psutil", None) + + assert studio_mod._pid_is_studio_server(8550, [111.5]) is True + assert studio_mod._pid_is_studio_server(8550, [None]) is True + + +def test_stop_signals_a_timestamped_record_without_psutil(monkeypatch, tmp_path): + # Multiple servers on different ports: only the newest is also in studio.pid, + # so the earlier ones are timestamp-only and must still be stopped. + studio_mod, _live, killed = _install(monkeypatch, tmp_path, alive = {8550}) + monkeypatch.setattr(studio_mod, "_pid_is_studio_server", _REAL_IS_STUDIO_SERVER) + monkeypatch.setitem(sys.modules, "psutil", None) + (tmp_path / "studio-8901-8550.pid").write_text("8550\n111.5", encoding = "utf-8") + + result = _run_stop(studio_mod) + + assert result.exit_code == 0, result.output + assert killed == [8550] + assert not (tmp_path / "studio-8901-8550.pid").exists() + + +def test_the_untimed_legacy_record_does_not_cancel_a_timed_one(monkeypatch): + # Every current server writes BOTH a timed per-port record and an untimed + # studio.pid, so letting the untimed half win made this check inert exactly + # where it matters: after a crash and a PID reuse, `stop` SIGTERMed whatever + # unrelated process had inherited the PID. An untimed record carries no + # information, so it must not overrule a start time that says "not ours". + studio_mod = _studio() + + class _FakeProcess: + def __init__(self, pid): + self.pid = pid + + def create_time(self): + return 999.0 + + monkeypatch.setitem(sys.modules, "psutil", SimpleNamespace(Process = _FakeProcess)) + + assert studio_mod._pid_is_studio_server(8550, [111.5, None]) is False + assert studio_mod._pid_is_studio_server(8550, [111.5]) is False + # A matching time still wins over a stale sibling record. + assert studio_mod._pid_is_studio_server(8550, [111.5, 999.0]) is True + assert studio_mod._pid_is_studio_server(8550, [None, None]) is True + + +def test_stop_does_not_signal_a_reused_pid_recorded_in_both_files(monkeypatch, tmp_path): + # End to end for the case above: a crashed server left studio-8901-8550.pid + # and studio.pid, and 8550 now belongs to something else entirely. + studio_mod, _live, killed = _install(monkeypatch, tmp_path, alive = {8550}) + monkeypatch.setattr(studio_mod, "_pid_is_studio_server", _REAL_IS_STUDIO_SERVER) + + class _FakeProcess: + def __init__(self, pid): + self.pid = pid + + def create_time(self): + return 999.0 + + monkeypatch.setitem(sys.modules, "psutil", SimpleNamespace(Process = _FakeProcess)) + (tmp_path / "studio-8901-8550.pid").write_text("8550\n111.5\n127.0.0.1", encoding = "utf-8") + (tmp_path / "studio.pid").write_text("8550", encoding = "utf-8") + + result = _run_stop(studio_mod) + + assert result.exit_code == 0, result.output + assert killed == [] + assert not list(tmp_path.glob("*.pid")) + + +def test_pid_identity_check_trusts_the_record_without_psutil(monkeypatch): + # No psutil: fall back to trusting the record rather than never stopping. + studio_mod = _studio() + monkeypatch.setitem(sys.modules, "psutil", None) + + assert studio_mod._pid_is_studio_server(8550) is True + + +def test_pid_identity_check_uses_the_recorded_start_time(monkeypatch): + studio_mod = _studio() + + class _FakeProcess: + def __init__(self, pid): + self.pid = pid + + def create_time(self): + return 111.5 + + monkeypatch.setitem(sys.modules, "psutil", SimpleNamespace(Process = _FakeProcess)) + + assert studio_mod._pid_is_studio_server(8550, [111.5]) is True + assert studio_mod._pid_is_studio_server(8550, [999.0]) is False + + +def test_stop_drops_a_record_whose_start_time_no_longer_matches(monkeypatch, tmp_path): + # The PID was reused: same number, different process. + studio_mod, _live, killed = _install(monkeypatch, tmp_path, alive = {8550}) + monkeypatch.setattr(studio_mod, "_pid_is_studio_server", _REAL_IS_STUDIO_SERVER) + + class _FakeProcess: + def __init__(self, pid): + self.pid = pid + + def create_time(self): + return 999.0 + + monkeypatch.setitem(sys.modules, "psutil", SimpleNamespace(Process = _FakeProcess)) + (tmp_path / "studio-8901-8550.pid").write_text("8550\n111.5", encoding = "utf-8") + + result = _run_stop(studio_mod) + + assert result.exit_code == 0, result.output + assert killed == [] + assert not (tmp_path / "studio-8901-8550.pid").exists() + # Dropped for the start-time mismatch, not because the record looked corrupt. + assert "invalid pid file" not in result.output.lower() + + +def test_stop_reads_the_legacy_single_pid_file(monkeypatch, tmp_path): + studio_mod, _live, killed = _install(monkeypatch, tmp_path, alive = {4242}) + _write_pid(tmp_path, "studio.pid", 4242) + + result = _run_stop(studio_mod) + + assert result.exit_code == 0, result.output + assert killed == [4242] + assert not (tmp_path / "studio.pid").exists() + + +def test_stop_reports_nothing_running_without_pid_files(monkeypatch, tmp_path): + studio_mod, _live, _killed = _install(monkeypatch, tmp_path, alive = set()) + + result = _run_stop(studio_mod) + + assert result.exit_code == 0, result.output + assert "no running unsloth server" in result.output.lower() + + +def test_stop_cleans_stale_pid_files_without_claiming_a_stop(monkeypatch, tmp_path): + studio_mod, _live, killed = _install(monkeypatch, tmp_path, alive = set()) + _write_pid(tmp_path, "studio-8901-8550.pid", 8550) + + result = _run_stop(studio_mod) + + assert result.exit_code == 0, result.output + assert killed == [] + assert not (tmp_path / "studio-8901-8550.pid").exists() + assert "stopped" not in result.output.lower() + + +def test_stop_does_not_claim_a_stop_while_a_server_is_still_alive(monkeypatch, tmp_path): + # SIGTERM delivered but it never exits: don't claim a stop, keep the file. + studio_mod = _studio() + monkeypatch.setattr(studio_mod, "STUDIO_HOME", tmp_path) + monkeypatch.setattr(studio_mod, "_PID_FILE", tmp_path / "studio.pid") + monkeypatch.setattr(studio_mod.time, "sleep", lambda _s: None) + monkeypatch.setattr(studio_mod, "_pid_alive", lambda pid: True) + monkeypatch.setattr(studio_mod, "_pid_is_studio_server", lambda pid, created_times = (): True) + monkeypatch.setattr(studio_mod.os, "kill", lambda pid, sig: None) + monkeypatch.setattr(sys, "platform", "linux") + _write_pid(tmp_path, "studio-8901-8550.pid", 8550) + + result = _run_stop(studio_mod) + + assert result.exit_code == 0, result.output + assert "shutting down" in result.output.lower() + assert "stopped" not in result.output.lower() + assert (tmp_path / "studio-8901-8550.pid").exists() + + +def test_stop_continues_after_one_server_fails_to_stop(monkeypatch, tmp_path): + studio_mod = _studio() + monkeypatch.setattr(studio_mod, "STUDIO_HOME", tmp_path) + monkeypatch.setattr(studio_mod, "_PID_FILE", tmp_path / "studio.pid") + monkeypatch.setattr(studio_mod.time, "sleep", lambda _s: None) + live = {8550, 8600} + monkeypatch.setattr(studio_mod, "_pid_alive", lambda pid: pid in live) + monkeypatch.setattr(studio_mod, "_pid_is_studio_server", lambda pid, created_times = (): True) + + def fake_kill(pid, _sig): + if pid == 8550: + raise PermissionError("not permitted") + live.discard(pid) + + monkeypatch.setattr(studio_mod.os, "kill", fake_kill) + monkeypatch.setattr(sys, "platform", "linux") + _write_pid(tmp_path, "studio-8901-8550.pid", 8550) + _write_pid(tmp_path, "studio-8902-8600.pid", 8600) + + result = _run_stop(studio_mod) + + combined = (result.output or "") + (getattr(result, "stderr", "") or "") + assert result.exit_code == 1, combined + assert 8600 not in live + assert "8550" in combined + + +def test_stop_never_signals_pid_zero_or_init(monkeypatch, tmp_path): + # os.kill(0, SIGTERM) hits our whole process group -- the shell and its jobs. + studio_mod, _live, killed = _install(monkeypatch, tmp_path, alive = {0, 1}) + _write_pid(tmp_path, "studio-8901-0.pid", 0) + _write_pid(tmp_path, "studio-8902-1.pid", 1) + + result = _run_stop(studio_mod) + + assert result.exit_code == 0, result.output + assert killed == [] + assert not list(tmp_path.glob("*.pid")) + + +def test_signal_stop_refuses_pid_zero_or_init(monkeypatch, tmp_path): + studio_mod, _live, killed = _install(monkeypatch, tmp_path, alive = {0, 1}) + + assert studio_mod._signal_stop(0) is not None + assert studio_mod._signal_stop(1) is not None + assert killed == [] + + +def test_stop_discards_a_corrupt_pid_file(monkeypatch, tmp_path): + studio_mod, _live, _killed = _install(monkeypatch, tmp_path, alive = set()) + (tmp_path / "studio-8901-8550.pid").write_text("not-a-pid", encoding = "utf-8") + + result = _run_stop(studio_mod) + + assert result.exit_code == 0, result.output + assert not (tmp_path / "studio-8901-8550.pid").exists() + + +def test_stop_keeps_a_record_it_cannot_read(monkeypatch, tmp_path): + # A root-owned record, or one caught mid-write, still belongs to a live + # server. Deleting it is `stop` manufacturing the orphan it exists to fix. + studio_mod, _live, killed = _install(monkeypatch, tmp_path, alive = {8550}) + path = tmp_path / "studio-8901-8550.pid" + path.write_text("8550", encoding = "utf-8") + real_read_text = Path.read_text + + def deny(self, *args, **kwargs): + if self == path: + raise PermissionError(13, "Permission denied") + return real_read_text(self, *args, **kwargs) + + monkeypatch.setattr(Path, "read_text", deny) + + result = _run_stop(studio_mod) + + assert path.exists(), "an unreadable record must not be deleted" + assert "cannot read" in (result.output + (result.stderr or "")).lower() + + +def test_stop_does_not_claim_success_when_the_only_record_is_unreadable(monkeypatch, tmp_path): + # A server started under sudo leaves a record we cannot read. Printing "no + # running server" and exiting 0 tells the user the opposite of the truth. + studio_mod, _live, killed = _install(monkeypatch, tmp_path, alive = {8550}) + path = tmp_path / "studio-8901-8550.pid" + path.write_text("8550", encoding = "utf-8") + real_read_text = Path.read_text + + def deny(self, *args, **kwargs): + if self == path: + raise PermissionError(13, "Permission denied") + return real_read_text(self, *args, **kwargs) + + monkeypatch.setattr(Path, "read_text", deny) + + result = _run_stop(studio_mod) + + assert result.exit_code == 1, "an unreachable server is not a successful stop" + output = result.output + (result.stderr or "") + assert "no running unsloth server" not in output.lower() + assert killed == [] + + +def test_stop_reports_failure_when_one_record_is_unreadable_but_another_stops( + monkeypatch, tmp_path +): + # Stopping the servers we can see is still a partial result, and exiting 0 + # would hide the one we could not. + studio_mod, _live, killed = _install(monkeypatch, tmp_path, alive = {8550, 8600}) + _write_pid(tmp_path, "studio-8901-8550.pid", 8550) + hidden = tmp_path / "studio-8902-8600.pid" + hidden.write_text("8600", encoding = "utf-8") + real_read_text = Path.read_text + + def deny(self, *args, **kwargs): + if self == hidden: + raise PermissionError(13, "Permission denied") + return real_read_text(self, *args, **kwargs) + + monkeypatch.setattr(Path, "read_text", deny) + + result = _run_stop(studio_mod) + + assert killed == [8550], "the readable server must still be stopped" + assert result.exit_code == 1 + assert hidden.exists() + + +def test_stop_reaches_every_server_when_one_record_cannot_be_removed(monkeypatch, tmp_path): + # One undeletable stale record must not end the loop before the live servers. + studio_mod, _live, killed = _install(monkeypatch, tmp_path, alive = {8600}) + _write_pid(tmp_path, "studio-8901-8550.pid", 8550) # dead -> stop prunes it + _write_pid(tmp_path, "studio-8902-8600.pid", 8600) # live -> stop signals it + real_unlink = Path.unlink + + def deny(self, *args, **kwargs): + if self.name == "studio-8901-8550.pid": + raise PermissionError(13, "Permission denied") + return real_unlink(self, *args, **kwargs) + + monkeypatch.setattr(Path, "unlink", deny) + + result = _run_stop(studio_mod) + + assert killed == [8600], "the live server must still be signalled" + assert result.exit_code == 0, result.output + + +def test_a_record_whose_pid_is_not_ascii_digits_is_discarded(monkeypatch, tmp_path): + # A superscript two passes isdigit() but int() rejects it, so that gate alone + # let a ValueError escape _read_pid_record and abort the whole command. + studio_mod, _live, _killed = _install(monkeypatch, tmp_path, alive = set()) + (tmp_path / "studio-8901-1.pid").write_text("²", encoding = "utf-8") + + result = _run_stop(studio_mod) + + assert result.exit_code == 0, result.output + assert not (tmp_path / "studio-8901-1.pid").exists()