mirror of
https://github.com/unslothai/unsloth.git
synced 2026-08-16 04:13:54 +00:00
* Studio: make research browser harnesses enforce their claims * Have the browser harnesses own their dev server for PR #8736 The new CI step backgrounds `npm run dev`, so $! is the npm wrapper and the EXIT trap leaves the vite node child alive holding the port and the step's stdout pipe. Reproduced: after the trap fires the port still answers 200. The ANSI smoke already solved this, so its start_vite / stop_process / drain_process_output move into _playwright_robust.py and all three harnesses use them. The CI step is two plain python3 calls now, and each file runs standalone the way its own docstring says. Readiness checks content, not status. Vite's SPA fallback answers 200 with index.html for anything missing, so a status check passes on a deleted smoke page. Confirmed locally: /smoke-DOESNOTEXIST.html returns 200. The report phase keeps a real, hit-tested click. A synthetic element.click() lands even with `body { pointer-events: none }` stranded, which is the freeze under test, so clicks_registered had stopped covering it. Measured both ways against a stranded layer: synthetic +1, real +0 and not actionable. The stall probe stays alongside it as main_thread_stall_ms, since nothing here reads an input timestamp, and its budget goes to 1000ms: 500 left only 1.2x against 342-416ms measured on a loaded host, and 1000 still fails ten times the report size (1518ms). Also: chat wall time and rAF count come from one page evaluation so they bracket the same interval; smoke-ansi-main.tsx joins the typechecked entries; the ANSI default port moves off the contended 8000; the job timeout goes to 20 minutes now that two browser smokes sit inside it; and the contract test pins the new verdicts plus the self-hosting rule. Verified: all three harnesses exit 0 standalone with no leftover vite and every port closed; typecheck 0; 2389 frontend tests; contract tests pass and fail when the new guards are removed. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Harden the shared dev-server lifecycle for PR #8736 Simulated the failure modes of the self-hosting change in an isolated venv. Five were real; all five are fixed here, each with a test that fails when the fix is reverted. 1. A busy port was measured instead of refused. Under --strictPort our vite exits, and the readiness poll then talks to whoever else holds the port. Worst case is a squatter serving a page that happens to contain the entry string, which content matching alone accepts. start_vite now refuses an occupied port up front. 2. A dead server cost the full 120s timeout, three times per CI run. Readiness now takes the process and gives up the moment it exits, surfacing vite's own last lines. Measured: 120s to 0.0s. 3. SIGTERM orphaned the server. `finally` covers exceptions and SIGINT but not SIGTERM, which is what a CI cancel sends, so the exact leak this work is about survived a cancelled job. SIGTERM and SIGHUP now tear down registered servers, chaining to any previous handler, and atexit covers the rest. 4. Teardown could raise over the failure that called it. stop_process runs from a `finally`, and a child outliving SIGKILL turned a clean harness failure into a TimeoutExpired traceback with the real error lost. 5. An exported-but-empty SMOKE_BASE_URL counted as external, so no server started and the harness drove "" as its base URL. Empty now means unset. tests/studio/test_playwright_server_lifecycle.py drives both platform branches by injecting os.name, so the Windows path (CREATE_NEW_PROCESS_GROUP, taskkill /T then /T /F) is checked on every run rather than on someone's machine. It needs no browser and no npm, so CI runs it before the Chromium install. Verified: all three harnesses pass self-hosting and in the pre-existing SMOKE_BASE_URL mode, where an external server is correctly left running; ANSI smoke passes on chromium, firefox and webkit; no leftover processes and every port closed; 22 simulations and 23 in-repo tests pass; all four mutants of the fixes above are caught. * Run the lifecycle tests after the playwright install They import the harnesses, which import playwright, so the browserless step I put before the install could never have worked on a clean runner. Caught by staging CI, reproduced locally in a pytest-only venv (same 5 failures), and confirmed fixed in that same venv once playwright is present: 23 passed. pytest moves into the existing install line rather than getting its own pip call. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Tighten the comments added for PR #8736 Comment-only pass: collapses the duplicated SMOKE_BASE_URL notes, the teardown and readiness asides, and the click rationale in the contract test. Intent unchanged, 12 fewer lines. Pre-existing comments from #8633 are left alone. AST-verified comments only (comment_tools.py check: 4/4 OK), 56 tests pass. * Skip the harness-import tests when playwright is absent Cross-platform staging CI on macos-14 failed 5 of 23: the two tests that reload a harness module pull in playwright.sync_api, and that runner does not ship it. Several workflows run tests/studio without installing playwright, so the file broke collection there rather than only in Frontend CI. pytest.importorskip is the convention already used in this directory (test_cached_model_path_selection.py, test_pdf_qa_recipe_contract.py). The other 18 touch stdlib-only helpers and are unaffected. Verified in a pytest-only venv: 18 passed, 5 skipped, where it was 5 failed. With playwright present all 17 still run rather than skipping. * Honour SMOKE_BASE_URL in the ANSI smoke It started its own server unconditionally, so pointing it at an external one still spawned a second vite, and with the new occupied-port check it now raises before the external page is ever tested. Reproduced against a server on the default 5203: RuntimeError: 127.0.0.1:5203 is already serving. The other two harnesses already derive OWNS_SERVER from SMOKE_BASE_URL; this brings the third into line. Self-hosting is unchanged, since OWNS_SERVER is true there. Verified both ways: external mode exits 0 and leaves that server running; self-hosting still starts and stops its own. * Let the POSIX teardown tests run on Windows Cross-platform staging on windows-latest failed 3 of 23: AttributeError: <module 'os' (frozen)> has no attribute 'killpg' os.killpg is POSIX-only, so monkeypatch.setattr had no attribute to replace and raised during setup. The tests that exist to prove the Windows branch is covered were themselves the ones Windows could not run. raising = False lets them install the stub on either platform; the assertions are unchanged, and os.name is already injected so the POSIX branch is what they exercise. Verified with os.killpg deleted from the interpreter to mimic the runner: 17 passed, same as on POSIX. * Make the Windows paths actually work in the harness and its tests Two Windows-only breaks, both confirmed against the Python docs before fixing. signal.SIGKILL is Unix-only, so the tests that force os.name to posix to exercise the POSIX teardown could not name it on a Windows interpreter, and raised after the fake process timed out. raising=False on os.killpg was not enough. A posix_branch fixture now stubs both, and the assertion compares against the same portable constant. start_vite ran a bare npm. On Windows npm is the batch file npm.cmd, and CreateProcess cannot execute a .cmd with shell=False, so the self-hosting default this PR documents would have failed with FileNotFoundError before vite started. shutil.which honours PATHEXT and resolves npm.cmd there, and returns /usr/bin/npm here. Verified with os.killpg and signal.SIGKILL both deleted from the interpreter: 17 passed, same as POSIX. The ANSI smoke still runs end to end on Linux through shutil.which. --------- Co-authored-by: Daniel Han <danielhanchen@gmail.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
196 lines
7.3 KiB
Python
196 lines
7.3 KiB
Python
# SPDX-License-Identifier: AGPL-3.0-only
|
|
# Copyright 2026-present the Unsloth AI Inc. team. All rights reserved. See /studio/LICENSE.AGPL-3.0
|
|
|
|
"""The dev-server lifecycle the browser smokes share.
|
|
|
|
Linux CI exercises the POSIX path only, and the Windows path is the one nobody runs until it
|
|
is broken on someone's machine. These drive both by injecting `os.name`, so the branch that
|
|
picks CREATE_NEW_PROCESS_GROUP and taskkill is checked on every run.
|
|
|
|
Everything here is monkeypatched: no npm, no browser, no sockets bound.
|
|
"""
|
|
|
|
from __future__ import annotations
|
|
|
|
import signal
|
|
import subprocess
|
|
import sys
|
|
from pathlib import Path
|
|
|
|
import pytest
|
|
|
|
sys.path.insert(0, str(Path(__file__).resolve().parent))
|
|
import _playwright_robust as robust # noqa: E402
|
|
|
|
# Unix-only, so a Windows interpreter cannot name it even to drive the POSIX branch.
|
|
SIGKILL = getattr(signal, "SIGKILL", 9)
|
|
|
|
HARNESSES = (
|
|
"playwright_chat_autoscroll",
|
|
"playwright_research_freeze",
|
|
"playwright_strip_ansi_smoke",
|
|
)
|
|
|
|
|
|
class _FakeProc:
|
|
"""A child that never dies, so both escalation steps are reachable."""
|
|
|
|
def __init__(self) -> None:
|
|
self.pid = 4242
|
|
self.stdout = None
|
|
self.returncode = None
|
|
|
|
def poll(self):
|
|
return None
|
|
|
|
def wait(self, timeout = None):
|
|
raise subprocess.TimeoutExpired("vite", timeout or 0)
|
|
|
|
|
|
@pytest.fixture
|
|
def no_signals(monkeypatch):
|
|
monkeypatch.setattr(robust, "_arm_teardown_signals", lambda: None)
|
|
monkeypatch.setattr(robust, "_LIVE_SERVERS", [])
|
|
|
|
|
|
@pytest.fixture
|
|
def posix_branch(monkeypatch, no_signals):
|
|
"""Drive the POSIX teardown from any host: os.killpg and signal.SIGKILL are Unix-only."""
|
|
monkeypatch.setattr(robust.os, "name", "posix")
|
|
monkeypatch.setattr(robust.signal, "SIGKILL", SIGKILL, raising = False)
|
|
|
|
|
|
@pytest.mark.parametrize("osname", ["posix", "nt"])
|
|
def test_start_vite_picks_the_platform_process_group(monkeypatch, no_signals, osname) -> None:
|
|
captured: dict = {}
|
|
monkeypatch.setattr(robust.os, "name", osname)
|
|
monkeypatch.setattr(robust, "_port_is_taken", lambda port, host: False)
|
|
monkeypatch.setattr(
|
|
robust.threading, "Thread", lambda **kw: type("T", (), {"start": lambda self: None})()
|
|
)
|
|
monkeypatch.setattr(
|
|
robust.subprocess, "Popen", lambda cmd, **kw: captured.update(cmd = cmd, kw = kw) or _FakeProc()
|
|
)
|
|
if osname == "nt":
|
|
monkeypatch.setattr(robust.subprocess, "CREATE_NEW_PROCESS_GROUP", 0x200, raising = False)
|
|
|
|
robust.start_vite(5199)
|
|
|
|
assert "--strictPort" in captured["cmd"], "a drifting port must fail, not pick another"
|
|
if osname == "nt":
|
|
assert captured["kw"]["creationflags"] == 0x200
|
|
assert "start_new_session" not in captured["kw"]
|
|
else:
|
|
# Without its own session, killing the npm wrapper orphans the node child.
|
|
assert captured["kw"]["start_new_session"] is True
|
|
assert "creationflags" not in captured["kw"]
|
|
|
|
|
|
def test_posix_teardown_signals_the_group_and_escalates(monkeypatch, posix_branch) -> None:
|
|
sent = []
|
|
monkeypatch.setattr(
|
|
robust.os, "killpg", lambda pid, sig: sent.append((pid, sig)), raising = False
|
|
)
|
|
robust.stop_process(_FakeProc())
|
|
assert sent == [(4242, signal.SIGTERM), (4242, SIGKILL)]
|
|
|
|
|
|
def test_windows_teardown_kills_the_tree_and_escalates(monkeypatch, no_signals) -> None:
|
|
calls = []
|
|
monkeypatch.setattr(robust.os, "name", "nt")
|
|
monkeypatch.setattr(robust.subprocess, "run", lambda cmd, **kw: calls.append(cmd))
|
|
robust.stop_process(_FakeProc())
|
|
assert calls == [
|
|
["taskkill", "/PID", "4242", "/T"],
|
|
["taskkill", "/PID", "4242", "/T", "/F"],
|
|
]
|
|
|
|
|
|
def test_teardown_never_raises_over_the_failure_that_called_it(monkeypatch, posix_branch) -> None:
|
|
"""stop_process runs from a `finally`. A child that outlives SIGKILL must not replace the
|
|
harness's real error with a TimeoutExpired."""
|
|
monkeypatch.setattr(robust.os, "killpg", lambda pid, sig: None, raising = False)
|
|
robust.stop_process(_FakeProc())
|
|
|
|
|
|
def test_teardown_tolerates_a_process_that_already_vanished(monkeypatch, posix_branch) -> None:
|
|
def gone(pid, sig):
|
|
raise ProcessLookupError
|
|
|
|
monkeypatch.setattr(robust.os, "killpg", gone, raising = False)
|
|
robust.stop_process(_FakeProc())
|
|
|
|
|
|
def test_an_occupied_port_is_refused_rather_than_measured(monkeypatch, no_signals) -> None:
|
|
"""--strictPort makes our vite exit, and the readiness poll would then be reading whatever
|
|
else holds the port. Refuse up front instead."""
|
|
monkeypatch.setattr(robust, "_port_is_taken", lambda port, host: True)
|
|
with pytest.raises(RuntimeError, match = "already serving"):
|
|
robust.start_vite(5199)
|
|
|
|
|
|
def test_readiness_gives_up_as_soon_as_our_server_dies(monkeypatch, no_signals) -> None:
|
|
"""Otherwise a dead server costs the full timeout, three times over, per CI run."""
|
|
|
|
class Dead:
|
|
returncode = 1
|
|
vite_tail = ["Port 5199 is already in use"]
|
|
|
|
def poll(self):
|
|
return 1
|
|
|
|
with pytest.raises(RuntimeError, match = "vite exited with code 1") as caught:
|
|
robust.wait_for_smoke_page(
|
|
"http://127.0.0.1:5199/x.html", "x.tsx", proc = Dead(), timeout_s = 30.0
|
|
)
|
|
assert "already in use" in str(caught.value), "vite's own reason should be surfaced"
|
|
|
|
|
|
@pytest.mark.parametrize("harness", HARNESSES)
|
|
def test_ports_do_not_collide_and_are_overridable(harness) -> None:
|
|
import re
|
|
src = (Path(__file__).resolve().parent / f"{harness}.py").read_text()
|
|
assert re.search(r'SMOKE_PORT",\s*"\d+"', src), f"{harness} has no SMOKE_PORT default"
|
|
|
|
|
|
def test_every_harness_picks_a_different_default_port() -> None:
|
|
import re
|
|
|
|
ports = {}
|
|
for harness in HARNESSES:
|
|
src = (Path(__file__).resolve().parent / f"{harness}.py").read_text()
|
|
ports[harness] = re.search(r'SMOKE_PORT",\s*"(\d+)"', src).group(1)
|
|
assert len(set(ports.values())) == len(HARNESSES), f"default ports collide: {ports}"
|
|
|
|
|
|
@pytest.mark.parametrize("harness", HARNESSES)
|
|
def test_an_empty_smoke_base_url_means_unset(harness, monkeypatch) -> None:
|
|
"""Exported-but-empty is common in shell wrappers. `in os.environ` would call it external
|
|
and then drive "" as the base URL."""
|
|
pytest.importorskip("playwright") # importing a harness pulls in playwright.sync_api
|
|
import importlib
|
|
|
|
monkeypatch.setenv("SMOKE_BASE_URL", "")
|
|
module = importlib.reload(importlib.import_module(harness))
|
|
try:
|
|
assert module.BASE.startswith("http://"), f"empty SMOKE_BASE_URL gave BASE={module.BASE!r}"
|
|
finally:
|
|
monkeypatch.delenv("SMOKE_BASE_URL", raising = False)
|
|
importlib.reload(module)
|
|
|
|
|
|
@pytest.mark.parametrize("harness", ("playwright_chat_autoscroll", "playwright_research_freeze"))
|
|
def test_an_external_smoke_base_url_is_still_honoured(harness, monkeypatch) -> None:
|
|
"""The documented pre-existing invocation. A harness that started its own server anyway
|
|
would fail on the busy-port check."""
|
|
pytest.importorskip("playwright")
|
|
import importlib
|
|
|
|
monkeypatch.setenv("SMOKE_BASE_URL", "http://127.0.0.1:9999")
|
|
module = importlib.reload(importlib.import_module(harness))
|
|
try:
|
|
assert module.BASE == "http://127.0.0.1:9999"
|
|
assert module.OWNS_SERVER is False
|
|
finally:
|
|
monkeypatch.delenv("SMOKE_BASE_URL", raising = False)
|
|
importlib.reload(module)
|