* Stop every running Unsloth server, and refuse to start a second on a taken port * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Check the fallback range, guard PID reuse, and keep writing studio.pid * Signal each server once when its PID is recorded in more than one file * Confirm a recorded PID is a Studio server before signalling it * Pin PID records to process start time and check every listener on a port * Keep every recorded start time per PID and accept in-process Studio servers * Match the blocking listener address and stop trusting unverifiable PID records * Never delete a PID record that cannot be verified * Detect our own server from our own records instead of a psutil listener scan * Match a pre-upgrade studio.pid to the blocked port before falling back * Never signal PID 0 or 1, and verify a per-port record before trusting it * Stop unverifiable records instead of skipping them, and record every bind address * Drop the command-line guess, fix Windows liveness, and free the PID record last * 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. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Studio: let a caller that follows the port keep the fallback, and never take studio.pid from a live server Two problems with keying the own-server abort on api_only. `unsloth studio run` is not the bare-banner path: it stores `app = run_server(...)` and reads `app.state.server_port` back, then uses it for the health wait, the model load and the printed base URL. Gating on api_only aborted it, so starting a second model while the first was up stopped working, where before it landed on the next port and printed the right URL. Replace the proxy with an explicit abort_if_own_studio, defaulting to the old api_only behaviour so the exec'd `run.py` path is unchanged, and have `studio run` opt out. The api_only exemption also reopened the orphan from the other side. _write_pid_file overwrote studio.pid unconditionally, and a pre-upgrade server is recorded there and nowhere else, so an exempt launch falling back past one erased its only record. Take the file over only when it is free, already ours, or held by a dead PID. Also resync _pid_is_studio_backend with the CLI copy: an untimed record next to a timed one carried no information but cancelled the start-time check, which is what let a reused PID be treated as ours. Tests: 51 backend, 26 CLI, 9 under tests/studio. Real Studio servers still abort the bare same-port relaunch, still fall back past a foreign listener, and one `unsloth studio stop` still stops every server in all five scenarios. * Studio: hand over the legacy PID pointer, and fail stop on unreadable records Two follow-ups from review of the previous commit. Only one backend owns studio.pid at a time. When that server exited it deleted the file, so an older CLI, which reads nothing else, could no longer stop a sibling that was still serving. _remove_pid_file now hands the pointer to a live sibling instead of dropping it. _pid_file_entries skipped records it could not read, for instance one written by a server started under sudo. When that was the only record, stop printed "No running Unsloth server found" and exited 0 while the server kept serving. Unreadable records are now reported and make stop exit 1, so a partial stop is never mistaken for a complete one. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Daniel Han <unslothai@gmail.com>
131 lines
5 KiB
Python
131 lines
5 KiB
Python
# SPDX-License-Identifier: AGPL-3.0-only
|
|
# Copyright 2026-present the Unsloth AI Inc. team. All rights reserved.
|
|
|
|
"""Regression tests for `unsloth studio stop` on Windows (PR #5940).
|
|
|
|
`stop` once used `os.kill(pid, 0)`, which raises WinError 87 on Windows before
|
|
reaching taskkill; the fix adds cross-platform `_pid_alive` (tasklist on Windows,
|
|
signal-0 elsewhere). AST + mock-only; no real processes, no Unsloth deps imported.
|
|
"""
|
|
|
|
import ast
|
|
import os
|
|
import subprocess
|
|
import sys
|
|
import types
|
|
from pathlib import Path
|
|
|
|
import pytest
|
|
|
|
_STUDIO_CMD_PY = Path(__file__).resolve().parents[2] / "unsloth_cli" / "commands" / "studio.py"
|
|
_SOURCE = _STUDIO_CMD_PY.read_text(encoding = "utf-8")
|
|
|
|
|
|
def _func_source(name: str) -> str:
|
|
"""Return the source of a top-level function `name` in studio.py."""
|
|
tree = ast.parse(_SOURCE)
|
|
for node in ast.walk(tree):
|
|
if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)) and node.name == name:
|
|
return ast.get_source_segment(_SOURCE, node)
|
|
raise AssertionError(f"function {name!r} not found in studio.py")
|
|
|
|
|
|
def _load_pid_alive(platform: str, fake_run = None):
|
|
"""Exec just `_pid_alive` with injectable sys/subprocess to drive the win32
|
|
branch on any host without importing unsloth_cli."""
|
|
src = _func_source("_pid_alive")
|
|
fake_sys = types.SimpleNamespace(platform = platform)
|
|
fake_sub = types.SimpleNamespace(run = fake_run) if fake_run is not None else subprocess
|
|
ns = {"os": os, "sys": fake_sys, "subprocess": fake_sub}
|
|
exec(src, ns)
|
|
return ns["_pid_alive"]
|
|
|
|
|
|
# ── AST: stop() must not use the broken bare liveness probe ──────────────────
|
|
|
|
|
|
# `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):
|
|
continue
|
|
f = call.func
|
|
is_os_kill = (
|
|
isinstance(f, ast.Attribute)
|
|
and f.attr == "kill"
|
|
and isinstance(f.value, ast.Name)
|
|
and f.value.id == "os"
|
|
)
|
|
if is_os_kill and len(call.args) == 2:
|
|
sig = call.args[1]
|
|
if isinstance(sig, ast.Constant) and sig.value == 0:
|
|
raise AssertionError(
|
|
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
|
|
assert "tasklist" in helper
|
|
|
|
|
|
# ── Behavioral: the win32 tasklist branch ────────────────────────────────────
|
|
|
|
|
|
def _fake_tasklist(returns_pid: int | None, *, raises: bool = False):
|
|
def _run(
|
|
cmd,
|
|
capture_output = False,
|
|
text = False,
|
|
timeout = None,
|
|
):
|
|
assert cmd[0] == "tasklist"
|
|
assert "/FI" in cmd # filtered by PID
|
|
if raises:
|
|
raise OSError("boom")
|
|
if returns_pid is None:
|
|
stdout = "INFO: No tasks are running which match the specified criteria.\n"
|
|
else:
|
|
stdout = f'"python.exe","{returns_pid}","Console","1","12,345 K"\n'
|
|
return types.SimpleNamespace(stdout = stdout, returncode = 0)
|
|
|
|
return _run
|
|
|
|
|
|
def test_pid_alive_windows_true_when_tasklist_lists_pid():
|
|
pid_alive = _load_pid_alive("win32", fake_run = _fake_tasklist(4242))
|
|
assert pid_alive(4242) is True
|
|
|
|
|
|
def test_pid_alive_windows_false_when_tasklist_empty():
|
|
pid_alive = _load_pid_alive("win32", fake_run = _fake_tasklist(None))
|
|
assert pid_alive(4242) is False
|
|
|
|
|
|
def test_pid_alive_windows_assumes_alive_when_tasklist_errors():
|
|
# Can't determine -> assume alive; taskkill is the source of truth.
|
|
pid_alive = _load_pid_alive("win32", fake_run = _fake_tasklist(None, raises = True))
|
|
assert pid_alive(4242) is True
|
|
|
|
|
|
# ── Behavioral: the POSIX signal-0 branch (skip on Windows runners) ───────────
|
|
|
|
|
|
@pytest.mark.skipif(sys.platform == "win32", reason = "POSIX os.kill(pid,0) branch")
|
|
def test_pid_alive_posix_true_for_self_false_for_dead():
|
|
pid_alive = _load_pid_alive("linux")
|
|
assert pid_alive(os.getpid()) is True
|
|
assert pid_alive(2_000_000_000) is False
|