diff --git a/comfy_cli/command/templates.py b/comfy_cli/command/templates.py index e750712c..da800d2d 100644 --- a/comfy_cli/command/templates.py +++ b/comfy_cli/command/templates.py @@ -19,6 +19,8 @@ import json import os +import subprocess +import sys import tempfile import time import urllib.error @@ -46,10 +48,22 @@ # 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) +# 200-with-garbage body raises a ``ValueError`` — ``JSONDecodeError`` for +# malformed JSON, but also a bare ``UnicodeDecodeError`` (a ``ValueError`` +# subclass, *not* a ``JSONDecodeError``) for a non-UTF-8 body, plus the shape +# ``ValueError`` we raise below when a valid-JSON 200 isn't the expected array. +# Catching ``ValueError`` covers all three. All of these must route through the +# same stale-cache fallback / command-level error, never an uncaught traceback. +_GALLERY_LOAD_ERRORS = (urllib.error.URLError, OSError, RuntimeError, ValueError) + +# How long a single background revalidation "counts" before another may be +# launched. Stale-while-revalidate serves the cache on every call past the TTL; +# without a debounce, an offline host (where the refresh fetch never succeeds and +# so never advances the cache mtime) would spawn a fresh detached refresher on +# *every* ``templates ls/show/fetch`` — unbounded PID fan-out / a local DoS in +# exactly the offline scenario this feature targets. This caps the steady-state +# launch rate to one per window while still revalidating promptly once back online. +_REFRESH_DEBOUNCE_SECONDS = 60.0 # --------------------------------------------------------------------------- @@ -63,6 +77,19 @@ def _cache_path() -> Path: return Path(base) / "comfy-cli" / "gallery" / "index.json" +def _looks_like_gallery(parsed: Any) -> bool: + """A gallery index is a JSON *array* of category objects. + + ``json.loads`` only proves the body is valid JSON, not that it is the shape + the rest of this module assumes. A 200 with a valid-but-wrong-shape payload — + a captive-portal/rate-limit ``{"error": …}``, a bare ``null`` or ``1`` — must + never be cached or served: ``_flatten_templates`` iterates the value expecting + dicts, so a dict yields silently-empty results and ``None``/an int raises + ``TypeError``. Gate on this before persisting and before serving. + """ + return isinstance(parsed, list) + + def _fetch_gallery(url: str = GALLERY_URL, timeout: float = 15.0) -> bytes: req = urllib.request.Request(url, headers={"User-Agent": "comfy-cli"}) with urllib.request.urlopen(req, timeout=timeout) as resp: @@ -75,45 +102,103 @@ def _load_gallery( explicit_path: str | None, *, refresh: bool = False, + background_ok: bool = True, ) -> list[dict[str, Any]]: """Resolve the gallery index. Precedence: explicit --gallery > cache > fetch. 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. + A cache older than ``GALLERY_TTL_SECONDS`` is served immediately and + revalidated in the background (stale-while-revalidate, BE-3427): a stale + cache is returned right away and a detached subprocess re-fetches it for + the *next* invocation, so an offline/firewalled machine never blocks on the + 15s fetch timeout on every call. An explicit ``--refresh`` (or a genuinely + absent/corrupt cache) still fetches synchronously and surfaces errors. + + ``background_ok=False`` opts a caller out of the stale-while-revalidate fast + path: an exact-name lookup (``show``/``fetch``) resolves a specific template, + and serving stale would report a freshly-added template as *not found* until + a later call — so those callers fetch synchronously on a stale cache (still + falling back to the stale copy if the fetch fails offline). """ if explicit_path: - return json.loads(Path(explicit_path).read_bytes()) + parsed = json.loads(Path(explicit_path).read_bytes()) + if not _looks_like_gallery(parsed): + raise ValueError("gallery index must be a JSON array of categories") + return parsed cache = _cache_path() have_cache = cache.exists() if not refresh and have_cache and not _cache_is_stale(cache): - return json.loads(cache.read_bytes()) + cached = json.loads(cache.read_bytes()) + if _looks_like_gallery(cached): + return cached + # A fresh-but-wrong-shape cache (poisoned by an older build, or a + # tampered file) is unsafe to serve; fall through to a synchronous fetch. + + # Stale-while-revalidate: a TTL-expired *but present* cache is served + # immediately, and a detached background process re-fetches it for the next + # invocation. This is what keeps an offline/firewalled machine from hanging + # on the full fetch timeout once per invocation past the TTL — the fetch + # never blocks the current call. `--refresh` is an explicit user request and + # `background_ok=False` (exact-name lookups) both deliberately stay + # synchronous (fetch + surface errors) below. + if not refresh and have_cache and background_ok: + try: + stale = json.loads(cache.read_bytes()) + except (OSError, ValueError): + # Cache is unreadable/corrupt (bad bytes, non-UTF-8, malformed + # JSON) — nothing safe to serve, so fall through to a synchronous + # fetch instead of the SWR fast path. + stale = None + if stale is not None and not _looks_like_gallery(stale): + # Valid JSON but not the expected array shape — treat as corrupt. + stale = None + if stale is not None: + spawned = _spawn_background_refresh() + if spawned: + get_renderer().warn( + f"gallery index is stale (last updated {_cache_age_str(cache)} ago); " + "serving the cached copy and refreshing in the background", + hint="run `comfy templates refresh` to update it now", + ) + else: + # The spawn failed outright (no fork, exec denied); don't claim a + # refresh is happening when none is. + get_renderer().warn( + f"gallery index is stale (last updated {_cache_age_str(cache)} ago); " + "serving the cached copy (couldn't start a background refresh)", + hint="run `comfy templates refresh` to update it now", + ) + return stale - # 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. + # No cache at all, an explicit `--refresh`, an exact-name lookup on a stale + # cache (``background_ok=False``), or an unreadable/wrong-shape stale cache: + # fetch synchronously. On a TTL auto-refresh with a cache present a fetch + # failure still falls back to the stale cache; `--refresh` / no-cache surface + # the 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. + # (rate-limit HTML, captive portal, truncated response) or a valid-JSON + # but wrong-shape body (``{"error": …}``, ``null``) must never clobber + # the last-known-good cache with garbage. parsed = json.loads(data) + if not _looks_like_gallery(parsed): + raise ValueError("gallery fetch returned an unexpected shape (not a JSON array)") 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. + # also fails (or is wrong-shape), surface the original fetch error. try: stale = json.loads(cache.read_bytes()) - except (OSError, json.JSONDecodeError): + except (OSError, ValueError): + raise e + if not _looks_like_gallery(stale): raise e get_renderer().warn( f"gallery refresh failed ({e}); using cached index (last updated {_cache_age_str(cache)} ago)", @@ -182,6 +267,120 @@ def _cache_age_str(cache: Path) -> str: return f"{age / 60:.0f}m" +def _refresh_marker_path() -> Path: + """Debounce marker next to the cache (``index.refresh``). Its mtime is the + time of the last background-refresh launch.""" + return _cache_path().with_suffix(".refresh") + + +def _refresh_due(marker: Path) -> bool: + """True when no background refresh has been launched within the debounce + window — i.e. a new one may fire. Best-effort rate limiter (mtime-based, no + hard lock): concurrent callers in the same instant may both spawn, but the + steady-state launch rate is capped at one per ``_REFRESH_DEBOUNCE_SECONDS``, + which is what bounds the offline fan-out (see ``_REFRESH_DEBOUNCE_SECONDS``). + """ + try: + age = time.time() - marker.stat().st_mtime + except OSError: + return True # no marker yet (or unreadable) → a refresh is due + # A future-dated marker (clock skew / restored file) must not pin the + # debounce open indefinitely; treat anything outside the window as due. + return not (0 <= age < _REFRESH_DEBOUNCE_SECONDS) + + +def _note_refresh_launched(marker: Path) -> None: + """Record 'a refresh was just launched' by (re)touching the debounce marker. + Best-effort: a read-only cache dir simply means no debounce this run (the + common case is already bounded), never a command failure.""" + try: + marker.parent.mkdir(parents=True, exist_ok=True) + marker.touch() + except OSError: + pass + + +def _refresh_cwd() -> str | None: + """A trusted working directory for the detached refresher. + + Running ``sys.executable -m comfy_cli`` prepends the child's cwd to + ``sys.path``, so inheriting the parent's cwd would let a ``comfy_cli.py`` (or + ``comfy_cli/`` package) planted in whatever directory the user happened to run + ``comfy templates`` from be imported and executed by the detached child. + Anchor the child in our own cache dir instead (created by comfy-cli, under the + user's home) — writing there already requires home-dir access. ``-P`` on + 3.11+ disables the prepend outright as defense-in-depth. + """ + parent = _cache_path().parent + return str(parent) if parent.is_dir() else None + + +def _spawn_background_refresh() -> bool: + """Kick off a detached subprocess that re-fetches the gallery index. + + Serve-stale-while-revalidate (BE-3427): the caller has already returned the + stale cache, so this revalidation must never block or delay process exit — + a firewalled machine would otherwise hang on the 15s fetch timeout on every + invocation past the TTL. We spawn a fully detached ``comfy templates + _refresh-cache`` (stdio → /dev/null; new session on POSIX, native detach + flags on Windows), broadly like the ``comfy run`` async job watcher does: the + child re-fetches and atomically rewrites the cache for the *next* invocation, + and its success or failure never touches the current command. + + Returns ``True`` when a background refresh is now running — freshly spawned, + or already in flight from a launch within the debounce window; ``False`` only + when the spawn was attempted and failed and none is in flight (so the caller + can avoid telling the user a refresh started when it didn't). + """ + marker = _refresh_marker_path() + if not _refresh_due(marker): + # A refresh was launched moments ago and is (at worst) still in flight; + # don't pile another detached process on top of it. Report True: a + # refresh is genuinely happening. + return True + + argv = [sys.executable] + if sys.version_info >= (3, 11): + # -P stops Python prepending the process cwd to sys.path (3.11+), + # neutralizing the `-m comfy_cli` cwd-import vector across the board. + argv.append("-P") + argv += ["-m", "comfy_cli", "templates", "_refresh-cache"] + + # The detached child runs the full `comfy` entry callback, which on a + # first-run / non-TTY host would persist an anonymous user_id via a + # *non-atomic* config.ini rewrite — racing the foreground process and risking + # a corrupt config. `_refresh-cache` is contractually 'no telemetry, + # best-effort', so opt the child out of consent entirely. + child_env = {**os.environ, "COMFY_NO_TELEMETRY": "1", "DO_NOT_TRACK": "1"} + + kwargs: dict[str, Any] = dict( + stdout=subprocess.DEVNULL, + stderr=subprocess.DEVNULL, + stdin=subprocess.DEVNULL, + close_fds=True, + cwd=_refresh_cwd(), + env=child_env, + ) + if sys.platform == "win32": + # start_new_session maps to setsid and is silently ignored on Windows; + # use the native flags so the child is truly detached from the parent's + # console/process group and survives console-close / Ctrl-C. + kwargs["creationflags"] = subprocess.CREATE_NEW_PROCESS_GROUP | subprocess.DETACHED_PROCESS + else: + kwargs["start_new_session"] = True + + try: + subprocess.Popen(argv, **kwargs) + except OSError: + # Couldn't spawn the refresher (no fork available, exec denied, …). We + # already served the stale cache, so degrade silently rather than + # failing the foreground command. Don't record a launch — a transient + # failure should be retried on the next call, not debounced away. + return False + _note_refresh_launched(marker) + return True + + def _flatten_templates(categories: list[dict[str, Any]]) -> list[dict[str, Any]]: """Walk the nested (category → templates) shape and flatten to a list. @@ -453,7 +652,10 @@ def show_cmd( ): renderer = get_renderer() try: - cats = _load_gallery(gallery_path, refresh=refresh) + # Exact-name lookup: opt out of stale-while-revalidate so a template + # added upstream since the cache went stale resolves on *this* call + # rather than being reported not-found until a later background refresh. + cats = _load_gallery(gallery_path, refresh=refresh, background_ok=False) except _GALLERY_LOAD_ERRORS as e: renderer.error(code="gallery_load_failed", message=str(e)) raise typer.Exit(code=1) from e @@ -506,6 +708,29 @@ def refresh_cmd(): renderer.emit(payload, command="templates refresh") +@app.command("_refresh-cache", hidden=True) +def _refresh_cache_cmd(): + """Hidden: the detached background gallery refresh (see + ``_spawn_background_refresh``). + + Fetch + atomically persist only — no output, no telemetry, never a non-zero + exit. It is spawned by ``templates ls/show/fetch`` when they serve a stale + cache, so any failure (offline, rate-limit, garbage 200) must be swallowed: + the foreground command already succeeded on the stale copy, and a bad body + must not clobber the last-known-good cache. + """ + try: + data = _fetch_gallery() + # Validate before persisting — never cache garbage. Reject both malformed + # JSON (and non-UTF-8 bodies, via ValueError) and a valid-JSON-but-wrong- + # shape body (``{"error": …}``, ``null``) that would poison the cache. + if not _looks_like_gallery(json.loads(data)): + return + except _GALLERY_LOAD_ERRORS: + return + _persist_cache(_cache_path(), data) + + # Where the per-template workflow JSONs live on GitHub. The gallery index lists # each template by ``name``; the corresponding workflow is at # ``Comfy-Org/workflow_templates/templates/.json``. @@ -550,9 +775,11 @@ def fetch_cmd( # Resolve against the gallery index first so we surface "no such template" # with the same close_matches affordance the rest of the CLI uses, instead - # of letting the user hit a raw GitHub 404. + # of letting the user hit a raw GitHub 404. Exact-name lookup, so opt out of + # stale-while-revalidate (background_ok=False): a template added upstream + # since the cache went stale must resolve now, not on a later call. try: - cats = _load_gallery(gallery_path, refresh=refresh) + cats = _load_gallery(gallery_path, refresh=refresh, background_ok=False) 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 86c2f5a6..2f651619 100644 --- a/tests/comfy_cli/command/test_templates.py +++ b/tests/comfy_cli/command/test_templates.py @@ -9,6 +9,7 @@ import json import os +import sys import time from pathlib import Path @@ -24,6 +25,10 @@ set_renderer, ) +# The genuine spawn helper, captured before the autouse stub replaces it, so the +# spawn-seam tests can exercise the real (Popen-calling) implementation. +_REAL_SPAWN_BACKGROUND_REFRESH = templates_cmd._spawn_background_refresh + FIXTURE = [ { "moduleName": "default", @@ -94,6 +99,26 @@ def reset_singleton(): reset_renderer_for_testing() +@pytest.fixture(autouse=True) +def no_real_background_refresh(monkeypatch): + """Never spawn a real detached refresher during tests (stale-while-revalidate). + + ``_spawn_background_refresh`` would otherwise fork ``python -m comfy_cli`` + on every stale-cache serve. Replace it with a counter so tests can assert it + fired without paying a subprocess/network round-trip. + """ + calls = {"count": 0} + + def _record(): + calls["count"] += 1 + # The real helper returns True when a refresh is running; mirror that so + # callers exercise the "refreshing in the background" path. + return True + + monkeypatch.setattr(templates_cmd, "_spawn_background_refresh", _record) + return calls + + def _force_json_renderer(): """Pin the renderer to JSON so tests can read envelopes off stdout.""" r = Renderer.resolve( @@ -316,67 +341,47 @@ def test_fresh_cache_within_ttl_serves_cache_without_fetching(cache_file, monkey assert env["data"]["total_in_gallery"] == 3 -def test_expired_cache_refetches_and_rewrites(cache_file, monkeypatch): +def test_expired_cache_serves_stale_immediately_and_spawns_background_refresh( + cache_file, monkeypatch, no_real_background_refresh +): # 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) + # Stale-while-revalidate (BE-3427): the foreground command must NOT block on + # a network fetch when a stale cache is present — it serves the stale copy + # immediately and revalidates in a detached background process. + 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 - 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 + # Served the stale 3-row fixture right away (no inline fetch — _fetch_boom + # would have raised if it were called). + assert env["data"]["total_in_gallery"] == 3 + # A background refresh was kicked off for the next invocation. + assert no_real_background_refresh["count"] == 1 + # The foreground command leaves the cache untouched (the bg process rewrites + # it), so a failed/slow revalidation can never clobber the last-good copy. + assert json.loads(cache_file.read_bytes()) == FIXTURE -def test_expired_cache_fetch_failure_falls_back_to_stale(cache_file, monkeypatch, capsys): +def test_refresh_cache_entrypoint_keeps_cache_on_fetch_failure(cache_file, monkeypatch): + # The detached background refresher (`templates _refresh-cache`) is spawned + # after the foreground served stale. If its fetch fails (offline), it must + # exit cleanly and leave the last-known-good cache untouched. 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. + result = runner.invoke(templates_cmd.app, ["_refresh-cache"]) 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 @@ -421,24 +426,18 @@ def _boom(*args, **kwargs): assert env["error"]["code"] == "gallery_load_failed" -def test_expired_cache_garbage_200_keeps_stale_and_does_not_clobber(cache_file, monkeypatch): +def test_refresh_cache_entrypoint_ignores_garbage_200(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) - + # overwrite the last-known-good cache — the background refresher validates + # before persisting and leaves the stale cache intact on garbage. 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. + result = runner.invoke(templates_cmd.app, ["_refresh-cache"]) 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 @@ -461,23 +460,19 @@ def _garbage(*args, **kwargs): 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 test_refresh_cache_entrypoint_swallows_non_200(cache_file, monkeypatch): + # `_fetch_gallery` raises RuntimeError on a non-200 status; the background + # refresher must swallow it (never escape as an uncaught traceback) and keep + # the stale cache intact. 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"]) + result = runner.invoke(templates_cmd.app, ["_refresh-cache"]) assert result.exit_code == 0, result.output - env = _envelope(result.output) - assert env["data"]["total_in_gallery"] == 3 + assert json.loads(cache_file.read_bytes()) == FIXTURE def test_non_200_status_under_refresh_is_fatal_not_uncaught(cache_file, monkeypatch): @@ -497,32 +492,29 @@ def _non_200(*args, **kwargs): assert env["error"]["code"] == "gallery_load_failed" -def test_future_mtime_clock_skew_is_treated_as_stale(cache_file, monkeypatch): +def test_future_mtime_clock_skew_serves_stale_and_revalidates(cache_file, monkeypatch, no_real_background_refresh): # 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. + # read as stale so the cache can't be pinned "fresh" forever. Under SWR that + # means: served immediately (no inline fetch) and revalidated in the + # background, rather than the future-dated cache being trusted. _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) + 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 - # A refresh was attempted rather than the future-dated cache being trusted. - assert fetched["count"] == 1 + env = _envelope(result.output) + assert env["data"]["total_in_gallery"] == 3 + # A background refresh was kicked off rather than the future-dated cache + # being trusted forever. + assert no_real_background_refresh["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) - +def test_refresh_cache_entrypoint_still_persists_when_atomic_rename_used(cache_file, monkeypatch): + # The background refresher persists a freshly fetched index via the atomic + # `_persist_cache` path, rewriting the cache for the next invocation. refreshed = [ { "moduleName": "default", @@ -533,21 +525,251 @@ def test_readonly_cache_dir_still_serves_fetched_data(cache_file, monkeypatch): } ] - def _fake_fetch(*args, **kwargs): - return json.dumps(refreshed).encode() + monkeypatch.setattr(templates_cmd, "_fetch_gallery", lambda *a, **k: json.dumps(refreshed).encode()) + + runner = CliRunner() + result = runner.invoke(templates_cmd.app, ["_refresh-cache"]) + assert result.exit_code == 0, result.output + # Cache was rewritten with the refreshed payload for the next `ls`/`show`. + assert json.loads(cache_file.read_bytes()) == refreshed + + +def test_readonly_cache_dir_still_serves_fetched_data_on_refresh(cache_file, monkeypatch): + # `--refresh` fetches synchronously; if persisting the freshly fetched index + # fails (read-only dir / disk full), the command must still succeed on the + # in-hand data rather than error out. + refreshed = [ + { + "moduleName": "default", + "category": "GENERATION TYPE", + "title": "Image", + "type": "image", + "templates": [{"name": "brand_new_template", "title": "Brand New", "tags": [], "models": [], "logos": []}], + } + ] def _boom_mkstemp(*args, **kwargs): raise OSError("read-only file system") - monkeypatch.setattr(templates_cmd, "_fetch_gallery", _fake_fetch) + monkeypatch.setattr(templates_cmd, "_fetch_gallery", lambda *a, **k: json.dumps(refreshed).encode()) # 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"]) + result = runner.invoke(templates_cmd.app, ["ls", "--refresh"]) 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"] + + +# --------------------------------------------------------------------------- +# Stale-while-revalidate spawn seam (BE-3427): the foreground serves stale and +# fires a *detached* background refresher; here we assert the spawn shape. +# --------------------------------------------------------------------------- + + +@pytest.fixture +def isolated_cache(tmp_path: Path, monkeypatch) -> Path: + """Point ``_cache_path`` at a clean tmp dir so the debounce marker and safe + cwd never touch (or read a stale marker from) the real user cache.""" + path = tmp_path / "gallery" / "index.json" + path.parent.mkdir(parents=True, exist_ok=True) + monkeypatch.setattr(templates_cmd, "_cache_path", lambda: path) + return path + + +def test_spawn_background_refresh_is_fully_detached(monkeypatch, isolated_cache): + # `_spawn_background_refresh` must launch a detached `comfy templates + # _refresh-cache` — new session (POSIX) / native detach flags (Windows), stdio + # → /dev/null — so it can outlive the parent without ever blocking it (offline: + # the parent must not wait on the 15s fetch timeout). + # Restore the real helper (the autouse fixture stubs it out for other tests). + monkeypatch.setattr(templates_cmd, "_spawn_background_refresh", _REAL_SPAWN_BACKGROUND_REFRESH) + captured = {} + + def _fake_popen(argv, **kwargs): + captured["argv"] = argv + captured["kwargs"] = kwargs + return object() + + monkeypatch.setattr(templates_cmd.subprocess, "Popen", _fake_popen) + assert templates_cmd._spawn_background_refresh() is True + + assert captured["argv"][-2:] == ["templates", "_refresh-cache"] + assert captured["argv"][0] == sys.executable + kwargs = captured["kwargs"] + if sys.platform == "win32": + flags = kwargs["creationflags"] + assert flags & templates_cmd.subprocess.CREATE_NEW_PROCESS_GROUP + assert flags & templates_cmd.subprocess.DETACHED_PROCESS + else: + assert kwargs["start_new_session"] is True + assert kwargs["stdout"] is templates_cmd.subprocess.DEVNULL + assert kwargs["stderr"] is templates_cmd.subprocess.DEVNULL + assert kwargs["stdin"] is templates_cmd.subprocess.DEVNULL + # The child is anchored in our own cache dir (not the parent's cwd) and opted + # out of telemetry so it can't race-write config.ini or import a planted + # comfy_cli.py from an untrusted directory. + assert kwargs["cwd"] == str(isolated_cache.parent) + assert kwargs["env"]["COMFY_NO_TELEMETRY"] == "1" + assert kwargs["env"]["DO_NOT_TRACK"] == "1" + + +def test_spawn_background_refresh_swallows_spawn_failure(monkeypatch, isolated_cache): + # If the OS can't spawn the refresher (no fork, exec denied), the foreground + # command has already served stale — the failure must be swallowed and + # reported as False (no refresh running), not raised. + monkeypatch.setattr(templates_cmd, "_spawn_background_refresh", _REAL_SPAWN_BACKGROUND_REFRESH) + + def _boom_popen(*args, **kwargs): + raise OSError("cannot fork") + + monkeypatch.setattr(templates_cmd.subprocess, "Popen", _boom_popen) + # Must not raise, and reports failure so the caller doesn't claim a refresh started. + assert templates_cmd._spawn_background_refresh() is False + + +# --------------------------------------------------------------------------- +# Wrong-shape / non-UTF-8 payload hardening: a valid-JSON-but-wrong-shape or +# non-UTF-8 200 must never poison the cache or crash the command. +# --------------------------------------------------------------------------- + + +@pytest.mark.parametrize("body", [b'{"error": "rate limited"}', b"null", b"1"]) +def test_refresh_cache_entrypoint_ignores_wrong_shape_200(cache_file, monkeypatch, body): + # A 200 whose body is valid JSON but not the expected array (captive-portal + # error object, bare null/number) must NOT overwrite the last-known-good cache. + monkeypatch.setattr(templates_cmd, "_fetch_gallery", lambda *a, **k: body) + + runner = CliRunner() + result = runner.invoke(templates_cmd.app, ["_refresh-cache"]) + assert result.exit_code == 0, result.output + assert json.loads(cache_file.read_bytes()) == FIXTURE + + +def test_explicit_refresh_wrong_shape_200_is_fatal(cache_file, monkeypatch): + # Under an explicit `--refresh`, a valid-JSON-but-wrong-shape body surfaces as + # gallery_load_failed rather than silently poisoning or serving stale. + monkeypatch.setattr(templates_cmd, "_fetch_gallery", lambda *a, **k: b'{"error": "nope"}') + _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" + assert json.loads(cache_file.read_bytes()) == FIXTURE # good cache intact + + +def test_ls_wrong_shape_stale_cache_falls_through_to_fetch(cache_file, monkeypatch, no_real_background_refresh): + # A stale cache whose *content* is valid JSON but the wrong shape can't be + # served — SWR must fall through to a synchronous fetch instead of handing a + # non-list to _flatten_templates (which would raise / silently drop rows). + cache_file.write_text(json.dumps({"error": "poisoned"})) + _set_mtime(cache_file, templates_cmd.GALLERY_TTL_SECONDS + 3600) + monkeypatch.setattr(templates_cmd, "_fetch_gallery", lambda *a, **k: json.dumps(FIXTURE).encode()) + _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 + # It fetched synchronously rather than serving the poisoned cache in the background. + assert no_real_background_refresh["count"] == 0 + + +def test_explicit_refresh_non_utf8_body_is_fatal_not_uncaught(cache_file, monkeypatch): + # A non-UTF-8 200 body makes json.loads raise UnicodeDecodeError — a + # ValueError subclass that is NOT a JSONDecodeError. It must still route + # through _GALLERY_LOAD_ERRORS as gallery_load_failed, never an uncaught crash. + monkeypatch.setattr(templates_cmd, "_fetch_gallery", lambda *a, **k: b"\xff\xfe\x00garbage") + _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" + + +# --------------------------------------------------------------------------- +# Exact-name lookups (show/fetch) opt out of stale-while-revalidate so a +# freshly-added template resolves on the same call (BE-3427 review). +# --------------------------------------------------------------------------- + + +def test_show_stale_cache_fetches_synchronously_for_exact_name(cache_file, monkeypatch, no_real_background_refresh): + # The stale cache lacks `brand_new_template`; `show` must fetch synchronously + # (not serve stale + background-refresh) so a template added upstream after + # the TTL expired resolves immediately instead of reporting not-found. + refreshed = [ + { + "moduleName": "default", + "category": "GENERATION TYPE", + "title": "Image", + "type": "image", + "templates": [{"name": "brand_new_template", "title": "Brand New", "tags": [], "models": [], "logos": []}], + } + ] + _set_mtime(cache_file, templates_cmd.GALLERY_TTL_SECONDS + 3600) + monkeypatch.setattr(templates_cmd, "_fetch_gallery", lambda *a, **k: json.dumps(refreshed).encode()) + _force_json_renderer() + + runner = CliRunner() + result = runner.invoke(templates_cmd.app, ["show", "brand_new_template"]) + assert result.exit_code == 0, result.output + env = _envelope(result.output) + assert env["data"]["template"]["name"] == "brand_new_template" + # Synchronous fetch — no detached background refresher was spawned. + assert no_real_background_refresh["count"] == 0 + + +def test_show_stale_cache_falls_back_to_stale_when_offline(cache_file, monkeypatch, no_real_background_refresh): + # background_ok=False still preserves the offline safety net: when the + # synchronous fetch fails, `show` falls back to the stale cache rather than + # erroring, so a known template still resolves offline. + 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, ["show", "image_flux2"]) + assert result.exit_code == 0, result.output + env = _envelope(result.output) + assert env["data"]["template"]["name"] == "image_flux2" + assert no_real_background_refresh["count"] == 0 + + +def test_spawn_background_refresh_debounces_rapid_calls(monkeypatch, isolated_cache): + # Stale-while-revalidate serves the cache on *every* call past the TTL. Without + # a debounce, an offline host would spawn a fresh detached refresher each time + # — unbounded PID fan-out / a local DoS. The second call within the window must + # NOT spawn again (but still reports True: a refresh is in flight). + monkeypatch.setattr(templates_cmd, "_spawn_background_refresh", _REAL_SPAWN_BACKGROUND_REFRESH) + spawns = {"count": 0} + + def _counting_popen(argv, **kwargs): + spawns["count"] += 1 + return object() + + monkeypatch.setattr(templates_cmd.subprocess, "Popen", _counting_popen) + + assert templates_cmd._spawn_background_refresh() is True + assert templates_cmd._spawn_background_refresh() is True + assert spawns["count"] == 1 # second call debounced, no extra process + + # Once the marker ages past the debounce window, a fresh launch is due again. + marker = templates_cmd._refresh_marker_path() + _set_mtime(marker, templates_cmd._REFRESH_DEBOUNCE_SECONDS + 5) + assert templates_cmd._spawn_background_refresh() is True + assert spawns["count"] == 2