From bdd0cf3835f324058640c1040e4dd5472d3dc131 Mon Sep 17 00:00:00 2001 From: Nilay Yadav Date: Wed, 29 Jul 2026 04:32:51 +0530 Subject: [PATCH] Never signal PID 0 or 1, and verify a per-port record before trusting it --- studio/backend/run.py | 14 ++++++++---- studio/backend/tests/test_studio_pid_files.py | 22 +++++++++++++++++++ unsloth_cli/commands/studio.py | 8 ++++++- unsloth_cli/tests/test_studio_stop.py | 21 ++++++++++++++++++ 4 files changed, 60 insertions(+), 5 deletions(-) diff --git a/studio/backend/run.py b/studio/backend/run.py index 8c347da0cc..62a6cbac6b 100644 --- a/studio/backend/run.py +++ b/studio/backend/run.py @@ -817,6 +817,10 @@ 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()) + # 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: @@ -824,7 +828,7 @@ def _read_pid_record(path: Path) -> "tuple[int, float | None, str | None] | None except ValueError: created = None address = lines[2].strip() if len(lines) > 2 and lines[2].strip() else None - return int(lines[0].strip()), created, address + return pid, created, address def _pid_is_studio_backend(pid: int, created_times: "Sequence[float | None]" = ()) -> "bool | None": @@ -891,9 +895,11 @@ def _legacy_studio_on_port(port: int) -> "int | None": 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 unported records get here. - if any(r and r[0] == pid for r in _per_port_records()): - return None + # 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]]) is not False: + return None blocker = _get_pid_on_port(port) if blocker is not None and blocker[0] != pid: return None diff --git a/studio/backend/tests/test_studio_pid_files.py b/studio/backend/tests/test_studio_pid_files.py index 1d4d1e86cf..134c8d1f7b 100644 --- a/studio/backend/tests/test_studio_pid_files.py +++ b/studio/backend/tests/test_studio_pid_files.py @@ -117,6 +117,15 @@ def test_read_pid_record_tolerates_a_bare_pid(tmp_path): 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") @@ -261,6 +270,19 @@ def test_a_dead_legacy_record_falls_back(tmp_path, monkeypatch): 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. diff --git a/unsloth_cli/commands/studio.py b/unsloth_cli/commands/studio.py index 8d916707b4..4b3447a177 100644 --- a/unsloth_cli/commands/studio.py +++ b/unsloth_cli/commands/studio.py @@ -2432,13 +2432,17 @@ def _read_pid_record(path: Path) -> "tuple[int, float | None] | None": return None if not lines or not lines[0].strip().isdigit(): return None + pid = int(lines[0].strip()) + # 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 int(lines[0].strip()), created + return pid, created def _pid_file_entries() -> "list[tuple[int, list[float | None], list[Path]]]": @@ -2507,6 +2511,8 @@ 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 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. diff --git a/unsloth_cli/tests/test_studio_stop.py b/unsloth_cli/tests/test_studio_stop.py index e7d011566d..9255081b56 100644 --- a/unsloth_cli/tests/test_studio_stop.py +++ b/unsloth_cli/tests/test_studio_stop.py @@ -381,6 +381,27 @@ def test_stop_continues_after_one_server_fails_to_stop(monkeypatch, tmp_path): 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")