From dfcb99196ea21f8f08bf1f8ad4b0bd35763edf74 Mon Sep 17 00:00:00 2001 From: Nilay Yadav Date: Wed, 29 Jul 2026 03:48:02 +0530 Subject: [PATCH] Never delete a PID record that cannot be verified --- unsloth_cli/commands/studio.py | 38 ++++++++++++++++++--------- unsloth_cli/tests/test_studio_stop.py | 25 ++++++++++++++---- 2 files changed, 45 insertions(+), 18 deletions(-) diff --git a/unsloth_cli/commands/studio.py b/unsloth_cli/commands/studio.py index 98acb938b5..8d916707b4 100644 --- a/unsloth_cli/commands/studio.py +++ b/unsloth_cli/commands/studio.py @@ -2471,14 +2471,14 @@ def _pid_file_entries() -> "list[tuple[int, list[float | None], list[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: - """Guard against PID reuse: a stale record must not get an unrelated process killed. +def _pid_is_studio_server(pid: int, created_times: "Sequence[float | None]" = ()) -> "bool | None": + """Guard against PID reuse. True = ours, False = not ours, None = can't tell. - A start time only exists if psutil was available when the server started, so - without psutil now such a record cannot be trusted -- but untimed (legacy) - records must keep working, or `stop` stops nothing on a psutil-less install. - The cmdline check covers those: the in-venv path runs run_server() in-process, - so its argv is `unsloth studio ...` with no run.py. + psutil is not a base CLI dependency but the managed backend has it, so the CLI + can meet timestamped records it cannot verify. Those are None, never False: + deleting one silently orphans a live server, which is the bug this fixes. + The cmdline check covers untimed legacy records -- the in-venv path runs + run_server() in-process, so its argv is `unsloth studio ...` with no run.py. """ known = [c for c in created_times if c is not None] untimed = not created_times or len(known) < len(created_times) @@ -2486,12 +2486,12 @@ def _pid_is_studio_server(pid: int, created_times: "Sequence[float | None]" = () import psutil proc = psutil.Process(pid) except Exception: - return untimed + return True if untimed else None if known: try: actual = proc.create_time() except Exception: - return untimed + return True if untimed else None if any(abs(actual - c) < 1.0 for c in known): return True if not untimed: @@ -2499,7 +2499,7 @@ def _pid_is_studio_server(pid: int, created_times: "Sequence[float | None]" = () try: cmdline = " ".join(proc.cmdline()).lower() except Exception: - return untimed + return True return "studio" in cmdline and ("run.py" in cmdline or "unsloth" in cmdline) @@ -2531,9 +2531,19 @@ def stop(): typer.echo("No running Unsloth server found (no PID file).") raise typer.Exit(0) - signalled, failed = [], [] + signalled, failed, unverified = [], [], [] for pid, created_times, paths in entries: - if not _pid_alive(pid) or not _pid_is_studio_server(pid, created_times): + identity = _pid_is_studio_server(pid, created_times) if _pid_alive(pid) else False + if identity is None: + # Keep the record: deleting it is what strands a live server. + unverified.append(pid) + typer.echo( + f"Cannot confirm PID {pid} is an Unsloth server (install psutil to check); " + "leaving it alone. Stop it manually if it is one.", + err = True, + ) + continue + if not identity: for path in paths: path.unlink(missing_ok = True) continue @@ -2546,6 +2556,8 @@ def stop(): signalled.append((pid, paths)) if not signalled and not failed: + if unverified: + raise typer.Exit(1) typer.echo("No running Unsloth server found (cleaned up stale PID files).") raise typer.Exit(0) @@ -2566,7 +2578,7 @@ def stop(): 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 failed: + if failed or unverified: raise typer.Exit(1) diff --git a/unsloth_cli/tests/test_studio_stop.py b/unsloth_cli/tests/test_studio_stop.py index 546544fdea..e7d011566d 100644 --- a/unsloth_cli/tests/test_studio_stop.py +++ b/unsloth_cli/tests/test_studio_stop.py @@ -184,17 +184,32 @@ def test_pid_identity_check_accepts_an_in_process_studio(monkeypatch): assert studio_mod._pid_is_studio_server(8550) is True -def test_a_timestamped_record_is_not_trusted_without_psutil(monkeypatch): - # psutil is not a base CLI dependency. A start time only exists if psutil was - # present when the server started, so without it now the record is unverifiable - # and must not get a reused PID killed. +def test_a_timestamped_record_is_unverifiable_without_psutil(monkeypatch): + # psutil is not a base CLI dependency but the managed backend has it, so the + # CLI meets records it cannot check. Unknown, not "not ours". studio_mod = _studio() monkeypatch.setitem(sys.modules, "psutil", None) - assert studio_mod._pid_is_studio_server(8550, [111.5]) is False + assert studio_mod._pid_is_studio_server(8550, [111.5]) is None assert studio_mod._pid_is_studio_server(8550, [None]) is True +def test_stop_keeps_an_unverifiable_record_instead_of_deleting_it(monkeypatch, tmp_path): + # Deleting it strands a live server with no record -- the bug this PR fixes. + 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) + + combined = (result.output or "") + (getattr(result, "stderr", "") or "") + assert result.exit_code == 1, combined + assert killed == [] + assert (tmp_path / "studio-8901-8550.pid").exists() + assert "cannot confirm pid 8550" in combined.lower() + + 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.