Addresses the open review findings on PR #5375 plus the Windows Studio UI CI regression that landed on main. ### Refresh-token rotation (auth/storage.py) * DELETE ... RETURNING is SQLite 3.35+. Older system SQLite (Ubuntu 20.04, RHEL 8, some Windows builds) raised OperationalError and turned /api/auth/refresh into a 500 for every user. consume_refresh_token now feature-detects RETURNING at first use, falls back to a transactional SELECT + DELETE, and uses delete_cursor.rowcount as the canonical "did I win the race" signal so two concurrent refreshes still produce exactly one winner. * Added test_refresh_token_consume.py covering single-use rotation, replay -> None, the desktop flag round-trip, and an 8-thread race that asserts exactly one winner on the fallback path. ### /api/auth/logout (routes/auth.py) * Logout was swallowing all exceptions from revoke_user_refresh_tokens and returning 204 even when refresh tokens were not actually invalidated. The endpoint now surfaces a 500 with a generic detail (and logs the exception class for the operator) so a caller cannot be told "you're logged out" while a stolen refresh token stays live. ### Sandbox AST policy (core/inference/tools.py) The PR-5375 visitor only matched calls on literal "requests.<method>" / "urllib.request.urlopen" FQ names. That left three bypasses: 1. Module aliases: `import requests as r; r.get("http://169.254.169.254/")`. 2. From-import + alias: `from requests import get as fetch; fetch(...)`. 3. Session-bound variables: `s = requests.Session(); s.get(...)`. 4. Variable URLs: `u = "http://..."; requests.get(u)`. The visitor now tracks imports (Import, ImportFrom) and assignments (Assign, including JoinedStr f-strings that fold to a constant), synthesises canonical FQ names for aliased calls and session methods, and resolves simple variable URLs through the assignment table before policy eval. Genuinely runtime-computed URLs (env vars, user input) are now flagged as "opaque_url_blocked" rather than allowed through silently. _NETWORK_FQ_PREFIXES gained the session-method synthetic prefixes (requests.Session., httpx.Client., httpx.AsyncClient., aiohttp.ClientSession.); _UPLOAD_HTTP_METHODS gained the matching Session.post/put/patch/delete/request entries. Added 12 tests across TestImportAliasResolution and TestSessionObjectMethods plus updated TestUntrustedHostBlock (test_dynamic_url_not_statically_blocked replaced with three sharper tests: variable URL resolved, f-string folded, and opaque-runtime URL flagged). ### Windows process-group kill (core/inference/tools.py) * _kill_process_tree was unconditionally calling os.getpgid / os.killpg, which raised AttributeError on Windows and skipped the kill entirely. The supervisor then leaked runaway tool processes and returned an execution error instead of a clean timeout. The helper now gates on hasattr(os, "getpgid") and hasattr(os, "killpg"), and on Windows falls back to proc.kill() + a best-effort taskkill /F /T. Added test_kill_process_tree_platform.py with a Linux/macOS pgid path test plus two simulated-Windows tests that monkeypatch the attributes off os. ### /api/health launcher contract (main.py) * Stripping every legacy identity field from the unauthenticated payload broke install.sh::_check_health, studio/src-tauri/src/preflight/backend.rs, and the run_studio_browser_test orchestrator, all of which match on service / studio_root_id / desktop_protocol_version without authenticating. The launcher contract (status, timestamp, service, studio_root_id, the four desktop_* capability bits) is now always exposed; the sensitive diagnostic fields (version, studio_version, device_type, chat_only, native_path_leases_supported, desktop_owner) remain gated on a valid bearer. * Added test_health_unauth_contract.py for the contract on both sides, and updated test_middleware.py::TestHealthAuthGate to match. ### CSP (main.py) * connect-src "self" was blocking the frontend's direct Hugging Face searches (use-hf-model-search, use-hf-dataset-search). connect-src now includes huggingface.co + *.huggingface.co + cdn-lfs.huggingface.co + cdn-lfs.hf.co + hf.co + *.hf.co; img-src adds huggingface.co + cdn-avatars.huggingface.co for the search avatar pickers. script-src stays at 'self' + per-response nonce; no 'unsafe-inline' anywhere. ### tool_call_id correlation (models/inference.py, routes/inference.py) * ChatMessage._validate_role_shape was synthesising a random tool_call_id when role="tool" arrived without one. The random id broke correlation with the preceding assistant tool_calls and OpenAI-compatible backends rejected the tool result. The validator now emits a recognisable TOOL_CALL_ID_SYNTH_PREFIX placeholder; _pair_orphan_tool_ids in the route walks the message list before passthrough and rewrites synth ids to the matching announced tool_call id (FIFO, skipping already-consumed ids). When no preceding tool_call is available the synth id stays so the upstream backend can produce an explicit error. * Added test_tool_id_pairing.py covering single rewrite, idempotency, FIFO pairing, no-announce fallthrough, and not double-consuming an explicit match. ### Training cancel cleanup (core/training/{training,worker}.py) * On cancel-no-save the worker emits "complete" with output_dir=None; force_terminate was snapshotting _output_dir (None at that point) and the new _cleanup_cancelled_checkpoints call was skipped, so periodic checkpoint-* dirs stayed on disk. The worker now emits "run_started" with the resolved output_dir immediately after path resolution; force_terminate prefers that value (_active_run_dir) when cleaning up so the cancel-no-save path actually removes the partial checkpoints. ### Windows Studio UI CI test robustness (tests/studio/playwright_extra_ui.py) * The /studio block was looking for "Configure", "Current run", "History" tabs without waiting for runtime hydration -- under the 1.5s timeout the loading placeholder was still rendered and the assertions failed in CI. The probe now waits up to 30s for either the studio tabs or the chat_only redirect, clicks Configure before checking the data-tour anchors, falls back to text-based selectors if Radix tabs do not yet expose role="tab", and adds a 3s grace for the lazy-mounted ParamsSection. chat_only is now read from /api/health with the bearer token (since the field is gated post-hardening); the test falls back to URL-shape detection if the field is absent. ## Cross-platform / cross-browser simulation Before pushing, the patches were exercised in an isolated `uv venv` under workspace/temp/sim_venv/: * 19 cross-platform sim tests pinning _kill_process_tree (Linux / macOS / Windows simulated by monkeypatching os.getpgid/killpg + sys.platform), the refresh-token RETURNING fallback under simulated old SQLite, and the AST policy across all three simulated platforms. * 24 multi-browser Playwright smokes (Chromium, Firefox, WebKit) against all 8 live Studios (ports 18801-18808), verifying /api/health response shape, CSP, X-Frame-Options, X-Content-Type-Options, Referrer-Policy, Permissions-Policy and the Server header on each engine. WebKit skips automatically when libgtk-4 / libgraphene / libavif are not installed system-wide. * 8 Studios were brought up in parallel (2 per GPU across CUDA_VISIBLE_DEVICES=4,5,6,7) and ran the live security probe; all 8 returned PASS=18 FAIL=0 SKIP=1 (skip is the auth-bearer-flow blocked by the in-test rate-limit hit). ## Test plan * Studio backend unit tests: pytest studio/backend/tests/ ignoring the GPU-dependent and KV-cache networked tests -> 816 passed, 10 skipped. * New tests: 12 sandbox AST cases + 8 refresh-token cases + 4 kill_process_tree cases + 8 tool-id pairing cases + 6 health-contract cases all pass. * Live HTTP probe across 8 Studios: 18/18 PASS on every Studio. * Multi-browser Playwright probe (Chromium + Firefox) across all 8 Studios: 16/16 PASS.
160 lines
5.5 KiB
Python
160 lines
5.5 KiB
Python
# SPDX-License-Identifier: AGPL-3.0-only
|
|
# Copyright 2026-present the Unsloth AI Inc. team. All rights reserved.
|
|
|
|
"""Cross-platform contract tests for _kill_process_tree.
|
|
|
|
The post-PR-5375 hardening pass added ``os.setsid`` to the sandbox
|
|
pre-exec; the cancel/timeout supervisor calls ``_kill_process_tree`` to
|
|
SIGKILL the resulting process group. ``os.getpgid``/``os.killpg`` are
|
|
Unix-only -- on Windows the helper must fall back to ``proc.kill()`` +
|
|
``taskkill /T``. We simulate Windows by stripping the platform-specific
|
|
attributes off ``os`` and verifying the helper still reaches the kill
|
|
path without raising.
|
|
"""
|
|
|
|
from __future__ import annotations
|
|
|
|
import os
|
|
import subprocess
|
|
import sys
|
|
import time
|
|
from pathlib import Path
|
|
from unittest import mock
|
|
|
|
import pytest
|
|
|
|
_BACKEND_ROOT = Path(__file__).resolve().parents[1]
|
|
if str(_BACKEND_ROOT) not in sys.path:
|
|
sys.path.insert(0, str(_BACKEND_ROOT))
|
|
|
|
from core.inference import tools as tools_mod
|
|
|
|
|
|
def _spawn_sleep(seconds: int = 60):
|
|
"""Spawn a lightweight sleeper. Prefers /bin/sleep (no Python startup
|
|
cost) and falls back to ``python -c sleep`` on platforms that don't
|
|
have /bin/sleep on PATH (mostly Windows)."""
|
|
if sys.platform != "win32":
|
|
from shutil import which
|
|
|
|
sleep_bin = which("sleep") or "/bin/sleep"
|
|
if Path(sleep_bin).exists():
|
|
return subprocess.Popen(
|
|
[sleep_bin, str(seconds)],
|
|
stdout=subprocess.DEVNULL,
|
|
stderr=subprocess.DEVNULL,
|
|
start_new_session=True,
|
|
)
|
|
# Fallback: minimal Python sleeper. Adds ~25-40 MB per test which
|
|
# is fine for single-test runs but is the reason we prefer
|
|
# /bin/sleep when available (the suite spawns one per test).
|
|
return subprocess.Popen(
|
|
[sys.executable, "-c", "import time; time.sleep(%d)" % seconds],
|
|
stdout=subprocess.DEVNULL,
|
|
stderr=subprocess.DEVNULL,
|
|
)
|
|
|
|
|
|
@pytest.fixture()
|
|
def short_proc():
|
|
"""A subprocess that sleeps long enough to be killable."""
|
|
proc = _spawn_sleep()
|
|
try:
|
|
yield proc
|
|
finally:
|
|
try:
|
|
proc.kill()
|
|
except Exception:
|
|
pass
|
|
|
|
|
|
class TestUnixPath:
|
|
"""On Linux/macOS the pgid path runs and reaps the child."""
|
|
|
|
@pytest.mark.skipif(
|
|
not (hasattr(os, "getpgid") and hasattr(os, "killpg")),
|
|
reason="No process-group APIs on this platform",
|
|
)
|
|
def test_kill_terminates_subprocess(self, short_proc):
|
|
assert short_proc.poll() is None
|
|
tools_mod._kill_process_tree(short_proc)
|
|
# Give the OS a moment to reap. _kill_process_tree does not block.
|
|
deadline = time.time() + 5.0
|
|
while short_proc.poll() is None and time.time() < deadline:
|
|
time.sleep(0.05)
|
|
assert short_proc.poll() is not None, "subprocess should have died"
|
|
|
|
@pytest.mark.skipif(
|
|
not (hasattr(os, "getpgid") and hasattr(os, "killpg")),
|
|
reason="No process-group APIs on this platform",
|
|
)
|
|
def test_no_raise_on_already_exited(self):
|
|
# poll() already returned non-None: helper must early-return.
|
|
class Dead:
|
|
pid = 0
|
|
|
|
def poll(self):
|
|
return 0
|
|
|
|
# Should not raise even though pid 0 has no pgid.
|
|
tools_mod._kill_process_tree(Dead())
|
|
|
|
|
|
class TestWindowsFallback:
|
|
"""Simulate a Windows runtime where os lacks getpgid/killpg."""
|
|
|
|
def test_kill_falls_back_to_proc_kill_when_pgid_missing(
|
|
self, short_proc, monkeypatch
|
|
):
|
|
# Strip the Unix-only attributes so the helper takes the
|
|
# Windows branch.
|
|
if hasattr(os, "getpgid"):
|
|
monkeypatch.delattr(os, "getpgid", raising=False)
|
|
if hasattr(os, "killpg"):
|
|
monkeypatch.delattr(os, "killpg", raising=False)
|
|
# Also lie about sys.platform so the taskkill fallback runs.
|
|
monkeypatch.setattr(tools_mod.sys, "platform", "win32")
|
|
# taskkill won't exist on Linux; capture its absence as a no-op
|
|
# via subprocess.run rather than failing the test.
|
|
import subprocess as _sp
|
|
|
|
with mock.patch.object(_sp, "run", return_value=None):
|
|
tools_mod._kill_process_tree(short_proc)
|
|
deadline = time.time() + 5.0
|
|
while short_proc.poll() is None and time.time() < deadline:
|
|
time.sleep(0.05)
|
|
assert short_proc.poll() is not None, "subprocess should have died"
|
|
|
|
def test_no_attribute_error_on_simulated_windows(self, monkeypatch):
|
|
"""Regression: AttributeError used to skip the kill entirely.
|
|
|
|
Before the fix, ``os.getpgid(...)`` raised ``AttributeError`` on
|
|
Windows; the helper's exception list only covered
|
|
ProcessLookupError + PermissionError, so ``AttributeError``
|
|
bubbled and the supervisor skipped the kill. The fix gates on
|
|
``hasattr(os, ...)`` first, so this test pins that contract.
|
|
"""
|
|
if hasattr(os, "getpgid"):
|
|
monkeypatch.delattr(os, "getpgid", raising=False)
|
|
if hasattr(os, "killpg"):
|
|
monkeypatch.delattr(os, "killpg", raising=False)
|
|
monkeypatch.setattr(tools_mod.sys, "platform", "win32")
|
|
|
|
class FakeProc:
|
|
pid = 9999
|
|
|
|
def __init__(self):
|
|
self.killed = False
|
|
|
|
def poll(self):
|
|
return None if not self.killed else 0
|
|
|
|
def kill(self):
|
|
self.killed = True
|
|
|
|
fp = FakeProc()
|
|
import subprocess as _sp
|
|
|
|
with mock.patch.object(_sp, "run", return_value=None):
|
|
tools_mod._kill_process_tree(fp) # must not raise
|
|
assert fp.killed
|