feat(templates): serve stale gallery cache immediately, revalidate in background (BE-3427)

[mattmillerai]

ELI-5

comfy templates ls/show/fetch reads its catalog from a small JSON file cached at ~/.cache/comfy-cli/gallery/index.json. PR #559 (BE-3393) gave that cache a 24h expiry, but past the expiry it re-fetches synchronously — so on an offline/firewalled machine every templates command hangs for the full 15s fetch timeout before falling back to the stale cache, once per invocation past the TTL.

This makes it stale-while-revalidate: a stale-but-present cache is served immediately, and the refresh happens in a detached background process that updates the cache for the next invocation. The current call never blocks on the network — online or offline.

Stacked on #559 (BE-3393)

This is a stacked PR: base = matt/be-3393-gallery-cache-ttl (the TTL + synchronous stale-fallback foundation, still open). It builds directly on that branch and changes its stale-path behavior from synchronous to SWR — explicitly the follow-up BE-3427 that #559 scoped out. GitHub will retarget this to main when #559 merges; review sees only the net SWR diff.

What changed

• _load_gallery — a TTL-expired but present cache is now returned right away, and _spawn_background_refresh() is fired to revalidate it. No inline fetch on this path.
• _spawn_background_refresh — launches a fully detached comfy templates _refresh-cache (start_new_session=True, stdio → /dev/null, spawn failure swallowed), mirroring the existing comfy run async job-watcher idiom (command/run/watcher.py). It outlives the parent without ever blocking it.
• _refresh-cache (hidden command) — fetch + atomic persist only; no output, no telemetry, never a non-zero exit. It validates the body before persisting and swallows every error, so a bad/garbage/offline response can never clobber the last-known-good cache.
• Staleness signal — a non-fatal renderer.warn on stderr ("serving the cached copy and refreshing in the background"), consistent with the foundation's existing fallback warning. The JSON envelope shape is unchanged (no per-payload stale field added).

Design decision (the ticket asked for one)

Detached subprocess, not a thread. CLI processes are short-lived, which rules out both thread options:

• A daemon thread is killed at interpreter exit before the ~1s online fetch+write completes → the cache would almost never refresh: a regression from the foundation's reliable auto-refresh.
• A non-daemon thread keeps the process alive until the fetch returns → on an offline machine it lingers up to the full 15s timeout after emitting output, every invocation. That just relocates the exact hang we're removing (and is just as bad for the agentic JSON callers this CLI targets, which wait on process exit).

A fully detached subprocess is the only option that returns the parent instantly in both the online and offline cases and reliably persists the refreshed cache for the next call. It's also the pattern already used in this repo for comfy run's async watcher. The "next invocation" refresh the ticket mentions is exactly what the detached child delivers — it writes the cache that the next ls/show/fetch reads.

Behavior preserved (Chesterton's fence)

• --refresh stays synchronous and still surfaces fetch errors (fatal gallery_load_failed) — an explicit user request is not deferred.
• No cache at all still fetches synchronously and is fatal on failure (nothing to serve).
• An unreadable/corrupt stale cache falls through to the synchronous path rather than serving garbage.
• The "don't clobber the last-good cache with a bad body / non-200 / offline" guarantees from feat(templates): add 24h TTL with stale-cache fallback to gallery cache (BE-3393) #559 are preserved — they now live in the _refresh-cache background entrypoint (which uses the same atomic _persist_cache + validate-before-write).

Tests

tests/comfy_cli/command/test_templates.py (updates the #559 TTL tests that asserted synchronous refetch, since the stale path is now SWR):

• expired cache → serves stale immediately, spawns background refresh, no inline fetch (_fetch_gallery monkeypatched to fail loudly), cache untouched by the foreground.
• future-mtime clock-skew → treated as stale: served + revalidated in background.
• _refresh-cache entrypoint → persists a fresh index; keeps the stale cache on fetch failure / garbage-200 / non-200; never exits non-zero.
• _spawn_background_refresh → asserts the detached spawn shape (argv ends templates _refresh-cache, start_new_session=True, stdio → DEVNULL); swallows OSError spawn failure.
• --refresh synchronous-path tests (fatal on failure, read-only-dir resilience) retained.
• Verified live end-to-end: hidden command routes via python -m comfy_cli; a stale cache serves in ~1s (no 15s block) with the staleness warning on stderr.

Full suite green locally (2587 passed, 37 skipped); ruff check + ruff format --check clean on the touched files.

Negative-claim falsification

Not applicable — this diff adds a capability (opportunistic background refresh) and makes stale-serving more available; it introduces no "unsupported"/deny/dead-end path. The staleness warning is a positive affordance, not a capability denial.

Judgment calls / known minors

• Rapid-fire invocations within the ~1s refresh window can each spawn a background refresher. They all os.replace atomically (last-writer-wins, no corruption); once the first completes, the cache is fresh again and subsequent calls don't spawn. Not worth a lockfile for this path.
• The ttl_auto_refresh synchronous fallback block below the SWR branch is now only reachable for the corrupt-stale-cache edge; left intact to minimize churn on the foundation's structure.
• comfy-cli uses (BE-####) PR/commit refs (per feat(templates): add 24h TTL with stale-cache fallback to gallery cache (BE-3393) #559 and repo history), so this follows that convention rather than the no-Linear-ref guardrail that applies to the ComfyUI/ComfyUI_frontend public repos.

[coderabbitai]

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in: 30 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 64d1c638-1f04-4670-b876-c08385f0acbe

📥 Commits

Reviewing files that changed from the base of the PR and between 978c1c5 and b7829f0.

📒 Files selected for processing (2)

• comfy_cli/command/templates.py
• tests/comfy_cli/command/test_templates.py

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch matt/be-3427-templates-stale-while-revalidate

✨ Simplify code

• Create PR with simplified code
• Commit simplified code in branch matt/be-3427-templates-stale-while-revalidate

---

_{Comment @coderabbitai help to get the list of available commands.}

[github-actions]

🔍 Cursor Review — Consolidated panel

Triggered by @mattmillerai.

Found 8 finding(s).

Severity	Count
🟠 High	2
🟡 Medium	3
🟢 Low	2
⚪ Nit	1

Panel: 6/8 reviewers contributed findings.

Reviewers that did not contribute: kimi-k2.5:adversarial (empty), kimi-k2.5:edge-case (empty)

[bigcat88]

Needs a rebase — and it's worth doing promptly, because I verified this PR closes two holes that are live on main right now.

Why it conflicts: its base (#559, matt/be-3393-gallery-cache-ttl) just landed as a squash merge, so the stacked commits no longer match main's history. GitHub retargeted this to main automatically and it flipped to CONFLICTING. Mechanical: rebase onto main and drop the commits that came in with #559.

What it fixes, confirmed empirically while reviewing #559:

1. Non-UTF-8 200 body escapes as an uncaught traceback. feat(templates): add 24h TTL with stale-cache fallback to gallery cache (BE-3393) #559's _GALLERY_LOAD_ERRORS catches json.JSONDecodeError, but a non-UTF-8 body raises UnicodeDecodeError — a ValueError, not a JSONDecodeError:

   json.loads(b'caf\xe9 error page, not json')
   -> UnicodeDecodeError   ValueError=True   JSONDecodeError=False
   

   With _fetch_gallery returning latin-1 bytes against a stale cache, both main and feat(templates): add 24h TTL with stale-cache fallback to gallery cache (BE-3393) #559 die with UNCAUGHT UnicodeDecodeError: 'utf-8' codec can't decode byte 0xe9 …. Broadening the tuple to ValueError here is what actually makes feat(templates): add 24h TTL with stale-cache fallback to gallery cache (BE-3393) #559's "never an uncaught traceback" comment true.

2. Valid-JSON-wrong-shape gets cached and served. json.loads(b'{"error":"rate limited"}') parses fine as a dict, so today a captive-portal or rate-limit JSON body passes validation, overwrites the cache, and reaches _flatten_templates. _looks_like_gallery is the missing gate.

So this isn't just a stale-while-revalidate nicety — it completes the error handling #559 started. The _REFRESH_DEBOUNCE_SECONDS reasoning is sound too: without it, an offline host never advances the cache mtime and would spawn a detached refresher on every ls/show/fetch.

Ping me once it's rebased and I'll review and merge it.

[mattmillerai]

Rebased onto main — the conflict was exactly as diagnosed: #559 landed as a squash, so this branch's stacked commits no longer matched main's history. Merged main in, dropped the now-duplicate #559 commits, kept this PR's SWR + cursor-review-hardening commits on top. Full test suite passes (3659 passed, 37 skipped) and lint/format are clean on the touched files. Ready for another look @bigcat88.

[skishore23]

Approving the design — the spawn hardening here is the best in this stack. But the rebase is semantic, not mechanical, and one conflict is a correctness trap. Please read the first section before resolving.

#559 is merged and this PR's base is already main, so the stacking note is moot.

⚠️ The _GALLERY_LOAD_ERRORS conflict — both sides carry a guard the other lacks

HEAD:        (urllib.error.URLError, OSError, RuntimeError, ValueError)
origin/main: (urllib.error.URLError, OSError, RuntimeError, json.JSONDecodeError, ResponseTooLarge)

I checked the hierarchy:

JSONDecodeError    -> ValueError : True     # your ValueError subsumes main's entry
UnicodeDecodeError -> ValueError : True     # main's narrower tuple MISSES this
ResponseTooLarge   -> ValueError : False    # derives from Exception — NOT covered by yours

So neither naive resolution is safe:

• Keep HEAD → an over-cap body raises ResponseTooLarge, which nothing catches → uncaught traceback instead of the stale-cache fallback.
• Keep main → a non-UTF-8 200 raises bare UnicodeDecodeError, which json.JSONDecodeError doesn't catch → same traceback. Your comment calling this out is a real catch that main doesn't have.

Correct merge:

_GALLERY_LOAD_ERRORS = (urllib.error.URLError, OSError, RuntimeError, ValueError, ResponseTooLarge)

I applied exactly that plus your comment and debounce constant — ruff check clean. Keep your comment text (it explains the ValueError breadth) and extend it to say ResponseTooLarge is listed separately because it isn't a ValueError, so nobody "simplifies" it away later.

The 3 test conflicts are semantic too

Both sides restructured the same tests, so a union doesn't work — I tried it and got duplicate defs of test_spawn_background_refresh_debounces_rapid_calls and test_oversize_gallery_during_ttl_refresh_falls_back_to_stale (3 failures, my artifact, not yours). These need hand-reconciliation: keep main's ResponseTooLarge/oversize coverage and your debounce + SWR tests. Please re-run the file after.

34 passed as-authored, so the PR is sound in isolation; I just can't certify the merged result until those are reconciled.

The spawn hardening is excellent

Worth calling out explicitly, because this is the part most likely to be "simplified" by a future reader:

• -P on 3.11+ plus cwd=_refresh_cwd() — a genuine belt-and-braces defense against the -m comfy_cli cwd-import vector. Anchoring the child in the cache dir rather than inheriting the user's cwd is the right call, and the docstring explains why a planted comfy_cli.py would otherwise execute. Same vector #641 addresses for nvidia-smi; nice to see a consistent posture.
• Telemetry opt-out in child_env — spotting that the child's entry callback would persist an anonymous user_id via a non-atomic config.ini rewrite, racing the foreground process, is a subtle one. Good catch.
• _note_refresh_launched is not called on spawn failure — so a transient failure retries next invocation instead of being debounced away for 60s. Easy to get backwards.
• Windows native DETACHED_PROCESS | CREATE_NEW_PROCESS_GROUP rather than assuming start_new_session works there — it's silently ignored on Windows, which would have left the child tied to the console.
• List-form argv, no shell, close_fds=True, stdio to DEVNULL.

The debounce also closes the unbounded-PID-fan-out hole that plain SWR would have opened on exactly the offline host this feature targets — which reads like it came out of review, and was the right thing to add.

Design choice

The detached-subprocess-over-thread reasoning is correct and worth keeping in the commit message. A daemon thread dies at interpreter exit before the write lands; a non-daemon thread relocates the 15s hang to after output, which is just as bad for agentic callers that wait on process exit. Subprocess is the only option that satisfies both.