From fb56b82a383469dd707f92b8ee35e0fdc0d6fb07 Mon Sep 17 00:00:00 2001 From: oobabooga Date: Thu, 11 Jun 2026 23:23:14 -0300 Subject: [PATCH] Studio: fix llama.cpp update banner offering a downgrade / sticking on mix releases (#6219) --- studio/backend/routes/llama.py | 4 +- .../backend/tests/test_llama_cpp_freshness.py | 134 +++++++++++++++++- studio/backend/tests/test_llama_cpp_update.py | 52 ++++++- studio/backend/utils/llama_cpp_freshness.py | 87 ++++++++++-- studio/backend/utils/llama_cpp_update.py | 33 ++++- 5 files changed, 290 insertions(+), 20 deletions(-) diff --git a/studio/backend/routes/llama.py b/studio/backend/routes/llama.py index 47a5bc1dd3..3aae6f4209 100644 --- a/studio/backend/routes/llama.py +++ b/studio/backend/routes/llama.py @@ -41,7 +41,9 @@ class LlamaUpdateStatusResponse(BaseModel): False, description = "True when the install came from an Unsloth prebuilt (has a marker).", ) - update_available: bool = Field(False, description = "True when installed_tag != latest_tag.") + update_available: bool = Field( + False, description = "True when the latest release is genuinely newer than the install." + ) stale: bool = Field( False, description = "Update available AND install older than the staleness threshold." ) diff --git a/studio/backend/tests/test_llama_cpp_freshness.py b/studio/backend/tests/test_llama_cpp_freshness.py index cb17e0d5e7..f8e4619ded 100644 --- a/studio/backend/tests/test_llama_cpp_freshness.py +++ b/studio/backend/tests/test_llama_cpp_freshness.py @@ -21,12 +21,25 @@ _BACKEND_DIR = str(Path(__file__).resolve().parent.parent) if _BACKEND_DIR not in sys.path: sys.path.insert(0, _BACKEND_DIR) + +class _NoopLogger: + """structlog-style logger: every method swallows positional + kwargs. + + A stdlib logging.Logger rejects structlog's keyword fields (e.g. + ``logger.warning(msg, error=...)``), which leaked into the update module's + error path and failed only when this file's stub loaded first. + """ + + def __getattr__(self, _name): + return lambda *a, **k: None + + _loggers_stub = _types.ModuleType("loggers") -_loggers_stub.get_logger = lambda name: __import__("logging").getLogger(name) +_loggers_stub.get_logger = lambda *a, **k: _NoopLogger() sys.modules.setdefault("loggers", _loggers_stub) _structlog_stub = _types.ModuleType("structlog") -_structlog_stub.get_logger = lambda *a, **k: __import__("logging").getLogger("stub") +_structlog_stub.get_logger = lambda *a, **k: _NoopLogger() sys.modules.setdefault("structlog", _structlog_stub) import pytest @@ -51,6 +64,11 @@ def _write_marker(install_dir: Path, **overrides) -> Path: .replace("+00:00", "Z"), } payload.update(overrides) + # The installer always writes `tag` and `release_tag` from the same release + # (a normalized base vs the full release tag), so keep the pair consistent + # when a test overrides only `tag`. + if "tag" in overrides and "release_tag" not in overrides: + payload["release_tag"] = overrides["tag"] install_dir.mkdir(parents = True, exist_ok = True) (install_dir / "UNSLOTH_PREBUILT_INFO.json").write_text(json.dumps(payload)) return install_dir / "UNSLOTH_PREBUILT_INFO.json" @@ -303,3 +321,115 @@ def test_format_stale_warning_singular_day(): msg = fr.format_stale_warning({"installed_tag": "b9190", "latest_tag": "b9300", "age_days": 1}) assert "1 day" in msg assert "1 days" not in msg + + +# parse_base_build / is_behind. + + +def test_parse_base_build(): + assert fr.parse_base_build("b9596") == 9596 + assert fr.parse_base_build(" b9596 ") == 9596 + assert fr.parse_base_build("b9596-mix-e6f2453") == 9596 # mix suffix doesn't defeat it + assert fr.parse_base_build("9596") is None + assert fr.parse_base_build("master-abc") is None + assert fr.parse_base_build("") is None + assert fr.parse_base_build(None) is None + + +@pytest.mark.parametrize( + "installed, latest, expected", + [ + ( + "b9596-mix-e6f2453", + "b9596-mix-e6f2453", + False, + ), # already on the mix latest -> not behind + ("b9596", "b9594", False), # latest is an older build -> downgrade guard + ("b9596", "b9594-mix-xxx", False), # older mix latest -> still guarded + ("b9500", "b9596-mix-e6f2453", True), # newer base -> behind + ("b9596-mix-aaa", "b9596-mix-bbb", True), # new mix at same base -> behind + ("b9596", "b9596-mix-bbb", True), # clean -> mix at same base -> behind + ("b9596-mix-aaa", "b9596", False), # bare base never supersedes a mix install + ("b9596", "b9596", False), # identical -> not behind + (" b9596 ", "b9596", False), # whitespace-only diff -> not behind + ("master-abc", "master-def", True), # non-bNNNN both -> plain inequality + ("master-abc", "master-abc", False), + (None, "b9596", False), + ("b9596", None, False), + ], +) +def test_is_behind(installed, latest, expected): + assert fr.is_behind(installed, latest) is expected + + +def test_check_prebuilt_freshness_not_behind_on_mix_latest(monkeypatch, tmp_path): + # Installed the mix latest: marker base tag b9596, full release_tag with sha, + # GitHub latest is that same full tag. Must not report behind (sticky bug). + install_dir = tmp_path / "llama.cpp" + _write_marker(install_dir, tag = "b9596", release_tag = "b9596-mix-e6f2453") + bin_path = _fake_binary(install_dir, layout = "root") + monkeypatch.setattr( + fr, "_fetch_latest_release_tag", lambda repo, timeout = 5.0: "b9596-mix-e6f2453" + ) + info = fr.check_prebuilt_freshness(str(bin_path)) + assert info["behind"] is False + assert info["stale"] is False + + +def test_check_prebuilt_freshness_downgrade_guard(monkeypatch, tmp_path): + # A lagging latest (older build than installed) must never read as behind/stale. + install_dir = tmp_path / "llama.cpp" + _write_marker( + install_dir, + tag = "b9585", + installed_at_utc = (datetime.now(tz = timezone.utc) - timedelta(days = 30)) + .isoformat() + .replace("+00:00", "Z"), + ) + bin_path = _fake_binary(install_dir, layout = "root") + monkeypatch.setattr(fr, "_fetch_latest_release_tag", lambda repo, timeout = 5.0: "b9518") + info = fr.check_prebuilt_freshness(str(bin_path)) + assert info["behind"] is False + assert info["stale"] is False + + +def test_fetch_latest_release_tag_uses_publish_time(monkeypatch): + # Resolves newest by published_at (like the installer), skips drafts/prereleases, + # and does NOT just take GitHub's first/`/releases/latest` item. + import urllib.request + + class _Resp: + def __init__(self, payload): + self._p = json.dumps(payload).encode() + + def read(self): + return self._p + + def __enter__(self): + return self + + def __exit__(self, *a): + return False + + payload = [ + { + "tag_name": "b9518", + "draft": False, + "prerelease": False, + "published_at": "2026-06-04T21:11:19Z", + }, + { + "tag_name": "b9596-mix-e6f2453", + "draft": False, + "prerelease": False, + "published_at": "2026-06-11T22:50:41Z", + }, + { + "tag_name": "b9999-draft", + "draft": True, + "prerelease": False, + "published_at": "2026-06-12T00:00:00Z", + }, + ] + monkeypatch.setattr(urllib.request, "urlopen", lambda req, timeout = 5.0: _Resp(payload)) + assert fr._fetch_latest_release_tag("unslothai/llama.cpp") == "b9596-mix-e6f2453" diff --git a/studio/backend/tests/test_llama_cpp_update.py b/studio/backend/tests/test_llama_cpp_update.py index d21d653b76..3b8f511a7c 100644 --- a/studio/backend/tests/test_llama_cpp_update.py +++ b/studio/backend/tests/test_llama_cpp_update.py @@ -82,18 +82,21 @@ def _write_install( tag: str, repo: str = "unslothai/llama.cpp", asset: str | None = None, + release_tag: str | None = None, ) -> str: """Create a fake prebuilt install tree and return the llama-server path. ``asset`` is the bundle filename recorded in the marker; omit it to model an - older marker that predates asset-based ROCm forwarding (backward compat).""" + older marker that predates asset-based ROCm forwarding (backward compat). + ``release_tag`` is the full release tag (e.g. a ``b9596-mix-`` mix + build); defaults to ``tag`` for a plain prebuilt.""" bin_dir = dir_ / "build" / "bin" bin_dir.mkdir(parents = True, exist_ok = True) binary = bin_dir / "llama-server" binary.write_text("#!/bin/sh\necho stub\n") marker = { "tag": tag, - "release_tag": tag, + "release_tag": release_tag or tag, "published_repo": repo, "installed_at_utc": "2020-01-01T00:00:00Z", "bundle_profile": "cuda13-newer", @@ -106,10 +109,13 @@ def _write_install( @pytest.fixture(autouse = True) -def _clean_state(monkeypatch): +def _clean_state(monkeypatch, tmp_path): freshness.reset_caches() upd._reset_job_for_tests() upd._resolve_memo.clear() + # Isolate the freshness disk cache so the suite never writes the real + # ~/.unsloth cache (the default when storage_roots can't be imported). + monkeypatch.setattr(freshness, "_cache_dir", lambda: tmp_path / ".freshness_cache") # Deterministic markerless paths: no host-pinned binary, no custom dir. monkeypatch.delenv("LLAMA_SERVER_PATH", raising = False) monkeypatch.delenv("UNSLOTH_LLAMA_CPP_PATH", raising = False) @@ -395,6 +401,7 @@ def test_start_update_installer_failure_reports_error(monkeypatch, tmp_path): binary = _write_install(install_dir, "b9493") monkeypatch.setattr(upd, "_find_binary", lambda: binary) monkeypatch.setattr(upd, "_installer_script", lambda: tmp_path / "install_llama_prebuilt.py") + monkeypatch.setattr(freshness, "_fetch_latest_release_tag", lambda repo, timeout = 5.0: "b9518") _patch_installer_popen(monkeypatch, returncode = 2, lines = ["boom: network error\n"]) @@ -617,6 +624,7 @@ def test_update_clears_maintenance_flag_on_installer_failure(monkeypatch, tmp_pa binary = _write_install(install_dir, "b9493") monkeypatch.setattr(upd, "_find_binary", lambda: binary) monkeypatch.setattr(upd, "_installer_script", lambda: tmp_path / "install_llama_prebuilt.py") + monkeypatch.setattr(freshness, "_fetch_latest_release_tag", lambda repo, timeout = 5.0: "b9518") backend = _FakeBackend() _inject_backend(monkeypatch, backend) @@ -795,3 +803,41 @@ def test_start_update_source_build_refuses_when_newer(monkeypatch, tmp_path): res = upd.start_update() assert res["started"] is False assert res["reason"] == "up_to_date" + + +# --- mix-tag detection + apply guard (the reported banner bug) --- + + +def test_status_not_offered_on_mix_latest(monkeypatch, tmp_path): + # Installed the mix latest; GitHub latest is that same full tag -> no banner. + binary = _write_install(tmp_path / "llama.cpp", "b9596", release_tag = "b9596-mix-e6f2453") + monkeypatch.setattr(upd, "_find_binary", lambda: binary) + monkeypatch.setattr( + freshness, "_fetch_latest_release_tag", lambda repo, timeout = 5.0: "b9596-mix-e6f2453" + ) + st = upd.get_update_status() + assert st["update_available"] is False + assert st["installed_tag"] == "b9596" + assert st["latest_tag"] == "b9596-mix-e6f2453" + + +def test_status_not_offered_when_latest_lags(monkeypatch, tmp_path): + # A lagging latest (older build than installed) must never be offered. + binary = _write_install(tmp_path / "llama.cpp", "b9585") + monkeypatch.setattr(upd, "_find_binary", lambda: binary) + monkeypatch.setattr(freshness, "_fetch_latest_release_tag", lambda repo, timeout = 5.0: "b9518") + st = upd.get_update_status() + assert st["update_available"] is False + + +def test_start_update_marked_refuses_when_not_behind(monkeypatch, tmp_path): + # A direct POST / stale banner must not reinstall when already on the latest. + binary = _write_install(tmp_path / "llama.cpp", "b9596", release_tag = "b9596-mix-e6f2453") + monkeypatch.setattr(upd, "_find_binary", lambda: binary) + monkeypatch.setattr(upd, "_installer_script", lambda: tmp_path / "install_llama_prebuilt.py") + monkeypatch.setattr( + freshness, "_fetch_latest_release_tag", lambda repo, timeout = 5.0: "b9596-mix-e6f2453" + ) + res = upd.start_update() + assert res["started"] is False + assert res["reason"] == "up_to_date" diff --git a/studio/backend/utils/llama_cpp_freshness.py b/studio/backend/utils/llama_cpp_freshness.py index 3e5066ca2d..f5fd745334 100644 --- a/studio/backend/utils/llama_cpp_freshness.py +++ b/studio/backend/utils/llama_cpp_freshness.py @@ -13,6 +13,7 @@ from __future__ import annotations import json import os +import re import time from datetime import datetime, timezone from pathlib import Path @@ -104,11 +105,18 @@ def _save_disk_cache(repo: str, latest_tag: Optional[str]) -> None: def _fetch_latest_release_tag(repo: str, timeout: float = 5.0) -> Optional[str]: - """GitHub API call. None on any failure (offline, rate-limited, etc).""" + """Newest published release tag for `repo`, by publish time. + + Resolves "latest" the way install_llama_prebuilt.py does (newest + non-draft/non-prerelease by ``published_at``), NOT via GitHub's + ``/releases/latest`` pointer. That pointer sorts by commit date and can lag + behind the build the installer actually installs, so detection and apply + disagreed -- the cause of the downgrade/sticky banner. None on any failure + (offline, rate-limited, etc).""" import urllib.error import urllib.request - url = f"https://api.github.com/repos/{repo}/releases/latest" + url = f"https://api.github.com/repos/{repo}/releases?per_page=30" headers = { "Accept": "application/vnd.github+json", "User-Agent": "unsloth-studio-freshness-check", @@ -128,8 +136,21 @@ def _fetch_latest_release_tag(repo: str, timeout: float = 5.0) -> Optional[str]: ) as exc: logger.debug("freshness fetch failed", repo = repo, error = str(exc)) return None - tag = data.get("tag_name") - return tag if isinstance(tag, str) and tag else None + if not isinstance(data, list): + return None + published = [ + r + for r in data + if isinstance(r, dict) + and not r.get("draft") + and not r.get("prerelease") + and isinstance(r.get("tag_name"), str) + and r.get("tag_name") + ] + if not published: + return None + newest = max(published, key = lambda r: r.get("published_at") or "") + return newest["tag_name"] def latest_published_release(repo: str, *, force_refresh: bool = False) -> Optional[str]: @@ -172,19 +193,58 @@ def _parse_installed_at(value: object) -> Optional[datetime]: return dt +def parse_base_build(tag: object) -> Optional[int]: + """Numeric base build from a release tag. Handles both a plain ``bNNNN`` and + a mix-build tag like ``b9596-mix-`` (anchored at the start, so the mix + suffix doesn't defeat it). None for anything not starting with ``bNNNN``.""" + if not isinstance(tag, str): + return None + m = re.match(r"b(\d+)", tag.strip()) + return int(m.group(1)) if m else None + + +def is_behind(installed: Optional[str], latest: Optional[str]) -> bool: + """Whether `installed` is genuinely behind `latest`, comparing the FULL + release identity (so a mix build can legitimately be the latest) with a + base-build guard so a lagging GitHub /releases/latest can never read as an + update or a downgrade. + + - identical tags -> not behind (clears the sticky banner post-update) + - higher base build on `latest` -> behind; lower -> NOT behind (downgrade guard) + - same base build: a different/new mix -> behind, but a bare ``bNNNN`` never + supersedes a mix build (extra PRs) at that base -> not behind + - non-bNNNN tags -> behind (plain inequality, since they already differ) + """ + if not installed or not latest: + return False + installed, latest = installed.strip(), latest.strip() + if installed == latest: + return False + ib, lb = parse_base_build(installed), parse_base_build(latest) + if ib is None or lb is None: + return True + if lb != ib: + return lb > ib + # Same base build, different tags: offer a mix (latest carries a suffix), but + # never offer a bare base over a mix install at the same base. + return latest != f"b{lb}" + + def check_prebuilt_freshness( binary_path: Optional[str], *, threshold_days: int = STALENESS_THRESHOLD_DAYS, now: Optional[datetime] = None, ) -> dict: - """Returns {has_marker, stale, installed_tag, latest_tag, + """Returns {has_marker, stale, behind, installed_tag, latest_tag, installed_at_utc, age_days, published_repo, threshold_days}. - stale = True iff installed != latest AND age >= threshold. - Fails open on missing data (stale stays False).""" + behind = installed genuinely older than latest (see is_behind). + stale = behind AND age >= threshold. + Fails open on missing data (behind/stale stay False).""" out: dict = { "has_marker": False, "stale": False, + "behind": False, "installed_tag": None, "latest_tag": None, "installed_at_utc": None, @@ -196,16 +256,25 @@ def check_prebuilt_freshness( if not marker: return out out["has_marker"] = True + # Display prefers the normalized base ("tag"); comparison below prefers the + # full "release_tag" -- deliberately opposite fallbacks. out["installed_tag"] = marker.get("tag") or marker.get("release_tag") out["installed_at_utc"] = marker.get("installed_at_utc") out["published_repo"] = marker.get("published_repo") + # The marker records both a normalized base tag ("tag", e.g. b9596) and the + # full release tag ("release_tag", e.g. b9596-mix-). Compare against the + # FULL identity, since GitHub /releases/latest returns the full tag_name -- + # comparing the normalized base against the full latest is what produced the + # permanent "downgrade" banner on every mix release. + installed_full = marker.get("release_tag") or marker.get("tag") repo = out["published_repo"] - if not repo or not out["installed_tag"]: + if not repo or not installed_full: return out latest = latest_published_release(repo) out["latest_tag"] = latest - if not latest or latest == out["installed_tag"]: + out["behind"] = is_behind(installed_full, latest) + if not out["behind"]: return out installed_at = _parse_installed_at(out["installed_at_utc"]) diff --git a/studio/backend/utils/llama_cpp_update.py b/studio/backend/utils/llama_cpp_update.py index 55bf4b3d84..654ade6cd4 100644 --- a/studio/backend/utils/llama_cpp_update.py +++ b/studio/backend/utils/llama_cpp_update.py @@ -286,9 +286,10 @@ def get_update_status(*, force_refresh: bool = False) -> dict: freshness = check_prebuilt_freshness(binary) installed = freshness.get("installed_tag") latest = freshness.get("latest_tag") - update_available = bool( - freshness.get("has_marker") and installed and latest and installed != latest - ) + # `behind` compares the full release identity with a base-build guard, so a + # lagging /releases/latest or a mix-tagged latest can't show a false update + # (see llama_cpp_freshness.is_behind). + update_available = bool(freshness.get("has_marker") and freshness.get("behind")) with _job_lock: job = dict(_job) @@ -405,9 +406,14 @@ def _run_update(install_dir: Path, repo: str, asset: Optional[str], script: Path tail = "".join(tail_lines).strip()[-1500:] raise RuntimeError(f"installer exited {returncode}: {tail or 'no output'}") - # New UNSLOTH_PREBUILT_INFO.json is on disk; drop caches so the next - # status read reflects the freshly installed tag. + # New UNSLOTH_PREBUILT_INFO.json is on disk; drop in-memory caches and + # re-prime the 24h disk freshness cache with the true newest, so the + # banner can't linger on a stale same-base value after the swap. reset_caches() + try: + latest_published_release(repo, force_refresh = True) + except Exception as exc: # pragma: no cover - network defensive + logger.debug("llama update: post-install freshness refresh failed", error = str(exc)) new_marker = read_install_marker(_find_binary()) new_tag = (new_marker or {}).get("tag") or (new_marker or {}).get("release_tag") @@ -456,7 +462,24 @@ def start_update() -> dict: "job": get_update_status()["job"], } + # A job already in flight wins over any freshness re-check below (and skips + # its network call). The final lock block re-checks to close the TOCTOU. + with _job_lock: + if _job["state"] == _JOB_RUNNING: + return {"started": False, "reason": "already_running", "job": dict(_job)} + if marker: + # Mirror the detection guard: a direct POST or a stale banner must not + # start an install when the latest is not actually newer (force a fresh + # check so a stale 24h cache can't wrongly block a real update either). + status = get_update_status(force_refresh = True) + if not status.get("update_available"): + return { + "started": False, + "reason": "up_to_date", + "message": "The installed llama.cpp build is already at the latest prebuilt.", + "job": status["job"], + } install_dir = _install_dir_for(binary) repo = marker.get("published_repo") or DEFAULT_PUBLISHED_REPO from_tag = marker.get("tag") or marker.get("release_tag")