feat(templates): add 24h TTL with stale-cache fallback to gallery cache (BE-3393) - #559
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 36 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the 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 configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @mattmillerai.
Found 7 finding(s).
| Severity | Count |
|---|---|
| 🟠 High | 2 |
| 🟡 Medium | 2 |
| 🟢 Low | 3 |
Panel: 6/8 reviewers contributed findings.
Reviewers that did not contribute: kimi-k2.5:adversarial (empty), kimi-k2.5:edge-case (empty)
…(BE-3393) Address cursor-review findings on the 24h-TTL gallery cache: - Validate the fetched body as JSON *before* writing the cache, so a 200 with a non-JSON body (rate-limit HTML, captive portal, truncated response) can no longer clobber the last-known-good cache with garbage. - Persist atomically via a temp file + os.replace so a concurrent `templates` reader never sees a half-written index. - Make the cache write best-effort: a read-only cache dir / full disk no longer breaks the command once valid data is in hand. - Route a non-200 status (RuntimeError from _fetch_gallery) through the same stale-cache fallback / gallery_load_failed path instead of letting it escape as an uncaught traceback (shared _GALLERY_LOAD_ERRORS tuple). - Guard the stale-fallback read: if the cache vanished/corrupted under us, surface the original fetch error rather than the read error. - Treat a future mtime (clock skew) as stale so the cache can't be pinned "fresh" indefinitely. Deferred: serve-stale-while-revalidate so offline machines don't block on the 15s fetch each call past the TTL (needs background-refresh design). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
🤖 The reviews loop filed Linear follow-up ticket(s) for review thread(s) deferred as out of scope for this PR:
|
bigcat88
left a comment
There was a problem hiding this comment.
Approving — I exercised the whole TTL matrix against the real filesystem and a real network failure (a dead proxy via https_proxy=http://127.0.0.1:1), with XDG_CACHE_HOME pointed at a scratch dir so the user cache was never touched.
| # | scenario | exit | result |
|---|---|---|---|
| 1 | no cache + offline | 1 | gallery_load_failed |
| 2 | populate (online) | 0 | cache written, 618 KB |
| 3 | fresh cache + offline | 0 | 580 templates, no network needed |
| 4 | stale (mtime −2d) + offline | 0 | stale served + stderr warning, cache untouched |
| 5 | templates refresh + offline |
1 | gallery_fetch_failed, cache not clobbered |
| 6 | stale + online | 0 | re-fetched, mtime advanced to now |
| 7 | stale + 200-with-garbage body | 0 | stale served, cache byte-identical (618495 → 618495) |
Rows 4 and 5 together are the Chesterton's-fence claim, and it holds exactly as written: the stale fallback fires only on the TTL-triggered auto-refresh, while an explicit refresh still fails loudly. Row 1 confirms an absent cache is still fatal.
Row 7 is the one I most wanted to see. Validating with json.loads before _persist_cache means a rate-limit HTML page or captive-portal response can't overwrite a good index with garbage — and because json.JSONDecodeError is in _GALLERY_LOAD_ERRORS, it routes through the same stale fallback instead of escaping as a traceback. I forced a 200 returning <html>429 Too Many Requests</html> against a stale cache: it warned, returned the 9 cached categories, and left the cache byte-for-byte identical. That ordering is load-bearing — worth keeping the comment.
The bug you're fixing is live on this machine. The real cache at ~/.cache/comfy-cli/gallery/index.json is dated Jun 27 — over a month frozen, exactly the dogfooding incident. With this change it self-heals on the next ls.
Nice details:
_persist_cachewriting to a temp file in the same directory thenos.replacemeans a concurrent reader never sees a half-written index — which would otherwise surface as a spuriousgallery_load_failed. Same-directory matters (cross-filesystemreplacewould fail), and you got that right.- Swallowing write failures is the right call: once you already hold valid data, a read-only cache dir (baked-into-a-container case) shouldn't break the command.
- Re-reading the stale cache inside the
exceptand re-raising the original fetch error if that read also fails is a subtle one most people miss — the user learns the network failed, not that a cache file was corrupt. - 24h over the spec's 7 days is well-argued: one small JSON file, weekly-ish upstream cadence, and the failure mode of a too-long TTL is silent staleness that nobody notices.
Full suite green on this branch merged with current main; ruff check + ruff format --diff clean under the CI-pinned 0.15.15.
Merge sequencing: #566 is stacked on this branch (matt/be-3393-gallery-cache-ttl), so this lands first and GitHub retargets #566 to main afterwards. I'll review #566 next.
One coverage gap — pre-existing, not a regression, but the comment overstates it
_GALLERY_LOAD_ERRORS's comment says a 200-with-garbage body "raises JSONDecodeError — all of which must route through the same stale-cache fallback … never an uncaught traceback." That's true for malformed JSON, but not for a non-UTF-8 body:
json.loads(b'caf\xe9 error page, not json')
# UnicodeDecodeError -> isinstance(e, ValueError) = True
# -> isinstance(e, json.JSONDecodeError) = FalseUnicodeDecodeError is a ValueError but not a JSONDecodeError, so the tuple misses it. Reproduced with _fetch_gallery returning latin-1 bytes against a stale cache:
this branch: -> UNCAUGHT UnicodeDecodeError: 'utf-8' codec can't decode byte 0xe9 …
main: -> UNCAUGHT UnicodeDecodeError: 'utf-8' codec can't decode byte 0xe9 …
Identical on both, so this is not a regression — main has the same hole and this PR simply doesn't close it. Not blocking.
(A BOM-prefixed body is a red herring, incidentally: b'\xff\xfe…' is a UTF-16 BOM, so json decodes it and you get a normal JSONDecodeError. Only invalid UTF-8 without a BOM escapes.)
This is a concrete reason to land #566 promptly behind this one. #566 broadens the tuple to ValueError — covering UnicodeDecodeError, JSONDecodeError, and its own shape-check — and adds _looks_like_gallery for the valid-JSON-wrong-shape case I confirmed slips through here (json.loads(b'{"error":"rate limited"}') parses fine as a dict, so today it would be cached and served). Between them the fallback is genuinely total; alone, this PR leaves two narrow paths open. Worth tightening the comment to say "malformed JSON" rather than implying full coverage.
ELI-5
comfy templates ls/showreads its catalog from a small JSON file cached at~/.cache/comfy-cli/gallery/index.json. Today, once that file exists it is served forever — the only way to update it iscomfy templates refresh, which nothing calls automatically. So agents browse a frozen template catalog (the 2026-07-16 dogfooding incident). This adds a 24h expiry: a cache older than a day is silently re-fetched on the nextls, and if the machine is offline the stale cache is still served (with a warning) so browsing never breaks.What changed
GALLERY_TTL_SECONDS = 24 * 60 * 60next toGALLERY_URL. 24h (not the spec's 7 days) — the gallery updates weekly-ish and the fetch is one tiny JSON file, so a tighter TTL is cheap insurance against a frozen catalog._load_gallerynow serves the cache only when it exists and is within the TTL. Past the TTL it re-fetches and rewrites the cache. If that TTL-triggered fetch fails (urllib.error.URLError/OSError), it falls back to the stale cache with a non-fatal renderer warning (stderr in JSON mode) — offline users keep a workingtemplates ls(exit 0)._cache_is_stale(mtime vs TTL) and_cache_age_str(human-friendly age for the warning).refresh_cmdis unchanged — it stays the manual force-refresh.All three
_load_gallerycallers (ls,show,fetch) get the TTL behavior transparently.Behavior preserved (Chesterton's fence)
The stale-fallback applies only to the TTL-triggered auto-refresh. An explicit
--refreshor a genuinely absent cache still surfaces a fetch error and exits non-zero, exactly as before — the user who asked to refresh (or has no cache at all) learns it failed rather than getting silently-stale data. A stale cache never triggered a fetch before this change, so the fetch-failure-on-stale path is brand-new, not a changed existing path.Tests
Added to
tests/comfy_cli/command/test_templates.py:_fetch_gallerymonkeypatched to fail loudly)gallery_load_failed)--refresh+ network failure → fatal, no silent stale fallbackFull suite green locally (2578 passed, 37 skipped);
ruff check+ruff formatclean on the touched files.Negative-claim falsification
N/A — this change adds a resilience capability (list offline from stale cache) rather than denying one. No deny/dead-end path, no "not supported" strings; the only new user-facing message is a non-fatal warning.