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.
This commit is contained in:
Daniel Han
parent
5d006896f8
commit
02eb5351e1
6 changed files with 385 additions and 53 deletions
|
|
@ -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)
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
66
tests/studio/test_studio_pid_file_contract.py
Normal file
66
tests/studio/test_studio_pid_file_contract.py
Normal file
|
|
@ -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()
|
||||
|
|
@ -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)
|
||||
|
|
|
|||
|
|
@ -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()
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue