From 02eb5351e1d2424873a1548c7b1eceefff7fd394 Mon Sep 17 00:00:00 2001 From: Daniel Han Date: Wed, 29 Jul 2026 06:35:14 +0000 Subject: [PATCH] Studio: harden the per-port PID records against the cases that lose a server Follow-up on the per-port PID files. Each item below is a case where the new code either lost a server the old code could still stop, or stopped something that was not ours. All were reproduced against real Studio servers. studio/backend/run.py - Write the per-port record and the legacy studio.pid independently. They shared one try, so a studio root that could not take a new directory entry left the server recorded nowhere at all and unstoppable from the CLI; the old code still recorded it in studio.pid, which is an overwrite of an existing path and can still succeed. _remove_pid_file now also checks studio.pid when the per-port write failed. - Write the record through a temp file and os.replace. `stop` reads these concurrently and treats a truncated read as a corrupt record. - A failed Windows tasklist probe now means "alive", matching the CLI. Treating it as dead pruned a live server's record and let the next launch fall back past it, which is the orphan this work exists to fix. - Guard the unlink in _own_studio_on_port. Pruning is a courtesy and must not abort startup. - Extract _resolve_port so the requested-port abort is reachable from a test. Deleting that abort previously left the whole suite green. - Keep the plain fallback for api-only callers. The desktop app hardcodes 8888 and documents its reliance on the 8888-8908 range, and it reports a non-zero backend exit to the user as "Server stopped unexpectedly". It reads the bound port back from TAURI_PORT, as `studio run` does from app.state.server_port, so a fallback there is harmless and both servers are still recorded and stoppable. The interactive path prints the requested port, so it still aborts. - isdigit() is not enough to gate int(): a superscript two passes it and the ValueError escaped into every caller of _read_pid_record. unsloth_cli/commands/studio.py - An untimed record no longer cancels a timed one for the same PID. Every current server writes both a timed per-port record and an untimed studio.pid, so the start-time check was inert exactly where it mattered, and after a crash plus a PID reuse `stop` sent SIGTERM to whatever unrelated process had inherited the PID. - Distinguish an unreadable record from an invalid one. A root-owned record, or one caught mid-write, still belongs to a live server, and deleting it stranded that server. - Route every PID-file removal through _unlink_quietly. One undeletable record raised PermissionError and left the remaining live servers running. - Same isdigit()/int() guard as the backend. Tests - The requested-port abort, the recorded bind address, and the api-only fallback are now covered; all three previously survived deletion. - tests/studio/test_studio_pid_file_contract.py pins run.py's filename scheme to the CLI's glob and keeps studio.pid parseable by an older CLI. It lives under tests/studio because unsloth_cli/tests is not run by any workflow. - test_cli_studio_stop_windows.py now checks _signal_stop as well as stop. The kill moved into _signal_stop, so the os.kill(pid, 0) guard passed vacuously. --- studio/backend/run.py | 93 ++++++++++++---- studio/backend/tests/test_studio_pid_files.py | 100 +++++++++++++++++- tests/studio/test_cli_studio_stop_windows.py | 16 ++- tests/studio/test_studio_pid_file_contract.py | 66 ++++++++++++ unsloth_cli/commands/studio.py | 68 +++++++++--- unsloth_cli/tests/test_studio_stop.py | 95 +++++++++++++++-- 6 files changed, 385 insertions(+), 53 deletions(-) create mode 100644 tests/studio/test_studio_pid_file_contract.py diff --git a/studio/backend/run.py b/studio/backend/run.py index c4824ab417..da930b018c 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, Sequence, Tuple +from typing import NoReturn, Optional, Sequence, Tuple def _fix_torch_cuda_ld_path(): @@ -798,8 +798,8 @@ def _pid_alive(pid: int) -> bool: except ImportError: pass if sys.platform == "win32": - # os.kill(pid, 0) raises OSError for every pid on Windows, so a stale record - # would look alive forever and block this port. Unconfirmed means prune. + # 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( @@ -809,7 +809,11 @@ def _pid_alive(pid: int) -> bool: timeout = 10, ).stdout except Exception: - return False + # 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) @@ -836,7 +840,11 @@ def _read_pid_record(path: Path) -> "tuple[int, float | None, str | None] | None return None if not lines or not lines[0].strip().isdigit(): return None - pid = int(lines[0].strip()) + 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 @@ -883,7 +891,11 @@ def _own_studio_on_port(port: int, host: str) -> "int | None": continue pid, created, address = record if not _pid_alive(pid): - path.unlink(missing_ok = True) + # 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 @@ -925,6 +937,23 @@ def _per_port_records() -> "list[tuple[int, float | None, str | None] | None]": 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 " @@ -968,24 +997,44 @@ def _write_pid_file(port: int, host: str = ""): path = _pid_file_for_port(port) try: 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}" - path.write_text(body, encoding = "utf-8") - # An older CLI's `stop` only reads this one, and expects a bare PID. + # 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: _PID_FILE.write_text(str(os.getpid()), encoding = "utf-8") except OSError: - return - _OWN_PID_FILE = path + pass def _remove_pid_file(): - """Remove the PID files that belong to this process.""" - if _OWN_PID_FILE is None: - return - for path in (_OWN_PID_FILE, _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. + """ + for path in ([_OWN_PID_FILE] if _OWN_PID_FILE is not None else []) + [_PID_FILE]: record = _read_pid_record(path) if path.is_file() else None if record is not None and record[0] == os.getpid(): try: @@ -1736,14 +1785,14 @@ 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) - # Falling back past our own server is what creates the orphan. - own = _own_studio_on_port(port, host) - if own is not None: - _abort_already_running(own, port) - port = _find_free_port(host, port + 1, avoid_own_studio = True) + 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) + if port != original_port: + blocker = _get_pid_on_port(original_port) if not silent: print("") print("=" * 50) diff --git a/studio/backend/tests/test_studio_pid_files.py b/studio/backend/tests/test_studio_pid_files.py index 4b35dae6d0..0b18cec680 100644 --- a/studio/backend/tests/test_studio_pid_files.py +++ b/studio/backend/tests/test_studio_pid_files.py @@ -122,7 +122,10 @@ def test_windows_liveness_does_not_call_every_pid_alive(monkeypatch): assert run._pid_alive(9999) is False -def test_windows_liveness_prunes_when_tasklist_fails(monkeypatch): +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): @@ -133,7 +136,7 @@ def test_windows_liveness_prunes_when_tasklist_fails(monkeypatch): monkeypatch.setattr(sys, "platform", "win32") monkeypatch.setattr(subprocess, "run", _boom) - assert run._pid_alive(8550) is False + assert run._pid_alive(8550) is True def test_read_pid_record_parses_pid_time_and_address(tmp_path): @@ -410,3 +413,96 @@ def test_fallback_still_skips_foreign_processes(tmp_path, monkeypatch): 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 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..5901fbf259 --- /dev/null +++ b/tests/studio/test_studio_pid_file_contract.py @@ -0,0 +1,66 @@ +# 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}, + "_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 fa120422fc..34d6c420b0 100644 --- a/unsloth_cli/commands/studio.py +++ b/unsloth_cli/commands/studio.py @@ -2426,15 +2426,16 @@ def _pid_alive(pid: int) -> bool: return True -def _read_pid_record(path: Path) -> "tuple[int, float | None] | None": - """Parse ``pid`` / optional ``create_time`` from a PID file.""" - try: - lines = path.read_text(encoding = "utf-8").splitlines() - except (OSError, UnicodeDecodeError): - return None +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 - pid = int(lines[0].strip()) + 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 @@ -2447,6 +2448,27 @@ def _read_pid_record(path: Path) -> "tuple[int, float | None] | None": return pid, created +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 _pid_file_entries() -> "list[tuple[int, list[float | None], list[Path]]]": """(pid, create_times, files) per recorded server, including the legacy studio.pid. @@ -2465,10 +2487,18 @@ def _pid_file_entries() -> "list[tuple[int, list[float | None], list[Path]]]": if path in seen or not path.is_file(): continue seen.add(path) - record = _read_pid_record(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) + continue + record = _parse_pid_record(text) if record is None: typer.echo(f"Ignoring invalid PID file {path.name}") - path.unlink(missing_ok = True) + _unlink_quietly(path) continue pid, created = record created_times, files = by_pid.setdefault(pid, ([], [])) @@ -2481,13 +2511,19 @@ def _pid_is_studio_server(pid: int, created_times: "Sequence[float | None]" = () """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 (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. + 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 or len(known) < len(created_times): + if not known: return True try: import psutil @@ -2531,7 +2567,7 @@ def stop(): 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: - path.unlink(missing_ok = True) + _unlink_quietly(path) continue error = _signal_stop(pid) if error is not None: @@ -2554,7 +2590,7 @@ def stop(): pid, paths = entry if not _pid_alive(pid): for path in paths: - path.unlink(missing_ok = True) + _unlink_quietly(path) pending.remove(entry) stopped = len(signalled) - len(pending) diff --git a/unsloth_cli/tests/test_studio_stop.py b/unsloth_cli/tests/test_studio_stop.py index a50c0f5b5b..752a7f2c2e 100644 --- a/unsloth_cli/tests/test_studio_stop.py +++ b/unsloth_cli/tests/test_studio_stop.py @@ -195,7 +195,6 @@ def test_an_untimed_record_is_trusted(monkeypatch): assert studio_mod._pid_is_studio_server(8550) is True assert studio_mod._pid_is_studio_server(8550, [None]) is True - assert studio_mod._pid_is_studio_server(8550, [111.5, None]) is True def test_an_unverifiable_record_is_still_stopped(monkeypatch): @@ -224,9 +223,12 @@ def test_stop_signals_a_timestamped_record_without_psutil(monkeypatch, tmp_path) assert not (tmp_path / "studio-8901-8550.pid").exists() -def test_a_legacy_record_survives_a_stale_timestamp_for_the_same_pid(monkeypatch): - # Stale per-port file + live legacy studio.pid sharing a reused PID: judging - # only by the stale timestamp would drop the live server. +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: @@ -236,13 +238,37 @@ def test_a_legacy_record_survives_a_stale_timestamp_for_the_same_pid(monkeypatch def create_time(self): return 999.0 - def cmdline(self): - return ["/venv/bin/unsloth", "studio", "-p", "8901"] - monkeypatch.setitem(sys.modules, "psutil", SimpleNamespace(Process = _FakeProcess)) - assert studio_mod._pid_is_studio_server(8550, [111.5, None]) is True + 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): @@ -401,3 +427,56 @@ def test_stop_discards_a_corrupt_pid_file(monkeypatch, tmp_path): 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_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()