diff --git a/comfy_cli/command/templates.py b/comfy_cli/command/templates.py index 92b9a264..e750712c 100644 --- a/comfy_cli/command/templates.py +++ b/comfy_cli/command/templates.py @@ -19,6 +19,8 @@ import json import os +import tempfile +import time import urllib.error import urllib.parse import urllib.request @@ -34,6 +36,21 @@ GALLERY_URL = "https://raw.githubusercontent.com/Comfy-Org/workflow_templates/main/templates/index.json" +# How long a cached gallery index stays fresh before ``_load_gallery`` transparently +# re-fetches it. 24h (not the spec's 7 days): the gallery updates weekly-ish and the +# fetch is one small JSON file, so a tighter TTL keeps agents off a frozen catalog +# cheaply. A network-down machine still lists from the stale cache (fetch failure +# falls back), and ``comfy templates refresh`` remains the manual force-refresh. +GALLERY_TTL_SECONDS = 24 * 60 * 60 + +# Everything a gallery load can throw. ``_fetch_gallery`` raises ``RuntimeError`` +# on a non-200 status (which ``urlopen`` doesn't already turn into an +# ``HTTPError``), the fetch itself raises ``URLError``/``OSError``, and decoding a +# 200-with-garbage body raises ``JSONDecodeError`` — all of which must route +# through the same stale-cache fallback / command-level error, never an uncaught +# traceback. +_GALLERY_LOAD_ERRORS = (urllib.error.URLError, OSError, RuntimeError, json.JSONDecodeError) + # --------------------------------------------------------------------------- # Gallery loading + caching @@ -63,17 +80,106 @@ def _load_gallery( Returns the raw decoded JSON (a list of category dicts). The CLI does its own filtering on top. + + A cache older than ``GALLERY_TTL_SECONDS`` is transparently re-fetched so + ``templates ls/show`` never serves a frozen catalog forever. If that refresh + fails (offline / GitHub down), we fall back to the stale cache with a + non-fatal renderer warning rather than erroring out. """ if explicit_path: return json.loads(Path(explicit_path).read_bytes()) cache = _cache_path() - if refresh or not cache.exists(): + have_cache = cache.exists() + + if not refresh and have_cache and not _cache_is_stale(cache): + return json.loads(cache.read_bytes()) + + # A TTL-expired cache is refreshed transparently, so a fetch failure here + # must NOT break `templates ls` — fall back to the stale cache with a + # non-fatal warning. An explicit `--refresh` (or a genuinely absent cache), + # by contrast, surfaces the fetch error so the user learns it failed. + ttl_auto_refresh = have_cache and not refresh + try: data = _fetch_gallery() + # Validate BEFORE we touch the cache: a 200 with a non-JSON body + # (rate-limit HTML, captive portal, truncated response) must never + # clobber the last-known-good cache with garbage. + parsed = json.loads(data) + except _GALLERY_LOAD_ERRORS as e: + if ttl_auto_refresh: + # The stale cache is our fallback — but a concurrent `refresh` may + # have removed it or left it corrupt mid-write. If reading it back + # also fails, surface the original fetch error, not the read error. + try: + stale = json.loads(cache.read_bytes()) + except (OSError, json.JSONDecodeError): + raise e + get_renderer().warn( + f"gallery refresh failed ({e}); using cached index (last updated {_cache_age_str(cache)} ago)", + hint="run `comfy templates refresh` once back online to update it", + ) + return stale + raise + _persist_cache(cache, data) + return parsed + + +def _persist_cache(cache: Path, data: bytes) -> None: + """Persist a freshly fetched index to the cache, atomically and best-effort. + + * Atomic — write to a temp file in the same directory then ``os.replace`` + it into place, so a concurrent ``templates`` reader never observes a + half-written index (which would parse-fail as ``gallery_load_failed``). + * Best-effort — a read-only cache dir (e.g. a gallery baked into a + container image) or a full disk must not break the command once we + already hold valid data, so a write failure is swallowed rather than + propagated. + """ + try: cache.parent.mkdir(parents=True, exist_ok=True) - cache.write_bytes(data) - return json.loads(data) - return json.loads(cache.read_bytes()) + fd, tmp = tempfile.mkstemp(dir=str(cache.parent), prefix=".index-", suffix=".tmp") + try: + with os.fdopen(fd, "wb") as f: + f.write(data) + os.replace(tmp, cache) + except OSError: + try: + os.unlink(tmp) + except OSError: + pass + raise + except OSError: + # Couldn't persist (read-only dir, disk full, …). We still have valid + # data in hand, so proceed without caching rather than failing the run. + pass + + +def _cache_is_stale(cache: Path) -> bool: + """True when the cache file is older than ``GALLERY_TTL_SECONDS``.""" + try: + age = time.time() - cache.stat().st_mtime + except OSError: + # Can't stat it → treat as stale so we attempt a refresh. + return True + # A future mtime (clock skew, or a restored/tampered file) yields a + # negative age; treat it as stale so the cache can't be pinned "fresh" + # indefinitely until wall-clock time catches up. + return age < 0 or age > GALLERY_TTL_SECONDS + + +def _cache_age_str(cache: Path) -> str: + """Human-friendly age of the cache file for the stale-fallback warning.""" + try: + age = max(0.0, time.time() - cache.stat().st_mtime) + except OSError: + return "unknown time" + hours = age / 3600.0 + if hours >= 24: + return f"{hours / 24:.1f}d" + if hours >= 1: + return f"{hours:.1f}h" + return f"{age / 60:.0f}m" def _flatten_templates(categories: list[dict[str, Any]]) -> list[dict[str, Any]]: @@ -251,7 +357,7 @@ def ls_cmd( try: cats = _load_gallery(gallery_path, refresh=refresh) - except (urllib.error.URLError, OSError, json.JSONDecodeError) as e: + except _GALLERY_LOAD_ERRORS as e: renderer.error( code="gallery_load_failed", message=str(e), @@ -348,7 +454,7 @@ def show_cmd( renderer = get_renderer() try: cats = _load_gallery(gallery_path, refresh=refresh) - except (urllib.error.URLError, OSError, json.JSONDecodeError) as e: + except _GALLERY_LOAD_ERRORS as e: renderer.error(code="gallery_load_failed", message=str(e)) raise typer.Exit(code=1) from e @@ -447,7 +553,7 @@ def fetch_cmd( # of letting the user hit a raw GitHub 404. try: cats = _load_gallery(gallery_path, refresh=refresh) - except (urllib.error.URLError, OSError, json.JSONDecodeError) as e: + except _GALLERY_LOAD_ERRORS as e: renderer.error(code="gallery_load_failed", message=str(e)) raise typer.Exit(code=1) from e diff --git a/tests/comfy_cli/command/test_templates.py b/tests/comfy_cli/command/test_templates.py index faa5fe8d..86c2f5a6 100644 --- a/tests/comfy_cli/command/test_templates.py +++ b/tests/comfy_cli/command/test_templates.py @@ -8,6 +8,8 @@ from __future__ import annotations import json +import os +import time from pathlib import Path import pytest @@ -274,3 +276,278 @@ def test_fetch_non_json_upstream_surfaces_workflow_invalid(gallery_file, monkeyp assert result.exit_code != 0 env = _envelope(result.output) assert env["error"]["code"] == "template_workflow_invalid_json" + + +# --------------------------------------------------------------------------- +# Gallery cache TTL (BE-3393): fresh-within-TTL serves the cache, expired +# re-fetches, and a fetch failure on an expired cache falls back to stale. +# --------------------------------------------------------------------------- + + +@pytest.fixture +def cache_file(tmp_path: Path, monkeypatch) -> Path: + """Point ``_cache_path`` at a tmp file seeded with the fixture index.""" + path = tmp_path / "cache" / "index.json" + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(json.dumps(FIXTURE)) + monkeypatch.setattr(templates_cmd, "_cache_path", lambda: path) + return path + + +def _set_mtime(path: Path, seconds_ago: float) -> None: + """Backdate a file's mtime so the cache reads as ``seconds_ago`` old.""" + when = time.time() - seconds_ago + os.utime(path, (when, when)) + + +def _fetch_boom(*args, **kwargs): + raise AssertionError("_fetch_gallery must not be called") + + +def test_fresh_cache_within_ttl_serves_cache_without_fetching(cache_file, monkeypatch): + # mtime is "now" (fixture just written) → well within the 24h TTL. + monkeypatch.setattr(templates_cmd, "_fetch_gallery", _fetch_boom) + _force_json_renderer() + + runner = CliRunner() + result = runner.invoke(templates_cmd.app, ["ls"]) + assert result.exit_code == 0, result.output + env = _envelope(result.output) + assert env["data"]["total_in_gallery"] == 3 + + +def test_expired_cache_refetches_and_rewrites(cache_file, monkeypatch): + # Backdate the cache past the TTL so a refresh is due. + _set_mtime(cache_file, templates_cmd.GALLERY_TTL_SECONDS + 3600) + + # A single-category "refreshed" gallery, distinct from the 3-row fixture. + refreshed = [ + { + "moduleName": "default", + "category": "GENERATION TYPE", + "title": "Image", + "type": "image", + "templates": [ + { + "name": "brand_new_template", + "title": "Brand New", + "description": "Freshly fetched.", + "tags": [], + "models": [], + "logos": [], + } + ], + } + ] + fetched = {"count": 0} + + def _fake_fetch(*args, **kwargs): + fetched["count"] += 1 + return json.dumps(refreshed).encode() + + monkeypatch.setattr(templates_cmd, "_fetch_gallery", _fake_fetch) + _force_json_renderer() + + runner = CliRunner() + result = runner.invoke(templates_cmd.app, ["ls"]) + assert result.exit_code == 0, result.output + assert fetched["count"] == 1 + + env = _envelope(result.output) + names = [r["name"] for r in env["data"]["rows"]] + assert names == ["brand_new_template"] + # Cache was rewritten with the refreshed payload. + assert json.loads(cache_file.read_bytes()) == refreshed + + +def test_expired_cache_fetch_failure_falls_back_to_stale(cache_file, monkeypatch, capsys): + import urllib.error + + _set_mtime(cache_file, templates_cmd.GALLERY_TTL_SECONDS + 3600) + + def _boom(*args, **kwargs): + raise urllib.error.URLError("network down") + + monkeypatch.setattr(templates_cmd, "_fetch_gallery", _boom) + _force_json_renderer() + + runner = CliRunner() + result = runner.invoke(templates_cmd.app, ["ls"]) + # Offline machine still lists from the stale cache — exit 0, original rows. + assert result.exit_code == 0, result.output + env = _envelope(result.output) + assert env["data"]["total_in_gallery"] == 3 + # The stale cache is untouched (not overwritten by a failed fetch). + assert json.loads(cache_file.read_bytes()) == FIXTURE + + +def test_expired_cache_fetch_failure_with_no_cache_is_fatal(tmp_path, monkeypatch): + import urllib.error + + # No cache on disk at all → a fetch failure has nothing to fall back to. + missing = tmp_path / "cache" / "index.json" + monkeypatch.setattr(templates_cmd, "_cache_path", lambda: missing) + + def _boom(*args, **kwargs): + raise urllib.error.URLError("network down") + + monkeypatch.setattr(templates_cmd, "_fetch_gallery", _boom) + _force_json_renderer() + + runner = CliRunner() + result = runner.invoke(templates_cmd.app, ["ls"]) + assert result.exit_code != 0 + env = _envelope(result.output) + assert env["ok"] is False + assert env["error"]["code"] == "gallery_load_failed" + + +def test_explicit_refresh_fetch_failure_is_fatal_not_stale_fallback(cache_file, monkeypatch): + import urllib.error + + # Even with a warm cache, `--refresh` is an explicit request: a fetch + # failure surfaces the error rather than silently serving stale. + def _boom(*args, **kwargs): + raise urllib.error.URLError("network down") + + monkeypatch.setattr(templates_cmd, "_fetch_gallery", _boom) + _force_json_renderer() + + runner = CliRunner() + result = runner.invoke(templates_cmd.app, ["ls", "--refresh"]) + assert result.exit_code != 0 + env = _envelope(result.output) + assert env["ok"] is False + assert env["error"]["code"] == "gallery_load_failed" + + +def test_expired_cache_garbage_200_keeps_stale_and_does_not_clobber(cache_file, monkeypatch): + # A 200 with a non-JSON body (rate-limit HTML / captive portal) must NOT + # overwrite the last-known-good cache — we validate before persisting and + # fall back to the stale cache. + _set_mtime(cache_file, templates_cmd.GALLERY_TTL_SECONDS + 3600) + + def _garbage(*args, **kwargs): + return b"rate limited" + + monkeypatch.setattr(templates_cmd, "_fetch_gallery", _garbage) + _force_json_renderer() + + runner = CliRunner() + result = runner.invoke(templates_cmd.app, ["ls"]) + # Offline-equivalent: stale cache still serves, exit 0, original rows. + assert result.exit_code == 0, result.output + env = _envelope(result.output) + assert env["data"]["total_in_gallery"] == 3 + # The good cache was left untouched, not clobbered with the HTML garbage. + assert json.loads(cache_file.read_bytes()) == FIXTURE + + +def test_explicit_refresh_garbage_200_is_fatal(cache_file, monkeypatch): + # A 200-with-garbage body under an explicit `--refresh` surfaces the decode + # error as gallery_load_failed rather than silently serving stale. + def _garbage(*args, **kwargs): + return b"rate limited" + + monkeypatch.setattr(templates_cmd, "_fetch_gallery", _garbage) + _force_json_renderer() + + runner = CliRunner() + result = runner.invoke(templates_cmd.app, ["ls", "--refresh"]) + assert result.exit_code != 0 + env = _envelope(result.output) + assert env["error"]["code"] == "gallery_load_failed" + # Good cache left intact. + assert json.loads(cache_file.read_bytes()) == FIXTURE + + +def test_non_200_status_falls_back_to_stale_not_uncaught(cache_file, monkeypatch): + # `_fetch_gallery` raises RuntimeError on a non-200 status; during a TTL + # auto-refresh that must route through the stale-cache fallback, never + # escape as an uncaught traceback. + _set_mtime(cache_file, templates_cmd.GALLERY_TTL_SECONDS + 3600) + + def _non_200(*args, **kwargs): + raise RuntimeError("gallery fetch failed: HTTP 429") + + monkeypatch.setattr(templates_cmd, "_fetch_gallery", _non_200) + _force_json_renderer() + + runner = CliRunner() + result = runner.invoke(templates_cmd.app, ["ls"]) + assert result.exit_code == 0, result.output + env = _envelope(result.output) + assert env["data"]["total_in_gallery"] == 3 + + +def test_non_200_status_under_refresh_is_fatal_not_uncaught(cache_file, monkeypatch): + # Same RuntimeError under an explicit `--refresh` surfaces cleanly as + # gallery_load_failed instead of crashing. + def _non_200(*args, **kwargs): + raise RuntimeError("gallery fetch failed: HTTP 500") + + monkeypatch.setattr(templates_cmd, "_fetch_gallery", _non_200) + _force_json_renderer() + + runner = CliRunner() + result = runner.invoke(templates_cmd.app, ["ls", "--refresh"]) + assert result.exit_code != 0 + env = _envelope(result.output) + assert env["ok"] is False + assert env["error"]["code"] == "gallery_load_failed" + + +def test_future_mtime_clock_skew_is_treated_as_stale(cache_file, monkeypatch): + # A future mtime (clock skew / restored file) yields a negative age; it must + # read as stale so the cache can't be pinned "fresh" forever. + _set_mtime(cache_file, -2 * 3600) # mtime 2h in the future + + fetched = {"count": 0} + + def _fake_fetch(*args, **kwargs): + fetched["count"] += 1 + return json.dumps(FIXTURE).encode() + + monkeypatch.setattr(templates_cmd, "_fetch_gallery", _fake_fetch) + _force_json_renderer() + + runner = CliRunner() + result = runner.invoke(templates_cmd.app, ["ls"]) + assert result.exit_code == 0, result.output + # A refresh was attempted rather than the future-dated cache being trusted. + assert fetched["count"] == 1 + + +def test_readonly_cache_dir_still_serves_fetched_data(cache_file, monkeypatch): + # If persisting the freshly fetched index fails (read-only dir / disk full), + # the command must still succeed on the in-hand data, not error out. + _set_mtime(cache_file, templates_cmd.GALLERY_TTL_SECONDS + 3600) + + refreshed = [ + { + "moduleName": "default", + "category": "GENERATION TYPE", + "title": "Image", + "type": "image", + "templates": [{"name": "brand_new_template", "title": "Brand New", "tags": [], "models": [], "logos": []}], + } + ] + + def _fake_fetch(*args, **kwargs): + return json.dumps(refreshed).encode() + + def _boom_mkstemp(*args, **kwargs): + raise OSError("read-only file system") + + monkeypatch.setattr(templates_cmd, "_fetch_gallery", _fake_fetch) + # Make the real _persist_cache's write fail (read-only dir / disk full); + # it must swallow the error and let the command proceed on in-hand data. + monkeypatch.setattr(templates_cmd.tempfile, "mkstemp", _boom_mkstemp) + _force_json_renderer() + + runner = CliRunner() + result = runner.invoke(templates_cmd.app, ["ls"]) + assert result.exit_code == 0, result.output + env = _envelope(result.output) + names = [r["name"] for r in env["data"]["rows"]] + assert names == ["brand_new_template"]