diff --git a/api/cloudbuild.yaml b/api/cloudbuild.yaml index 8e999376010..d4cb8168816 100644 --- a/api/cloudbuild.yaml +++ b/api/cloudbuild.yaml @@ -5,7 +5,22 @@ substitutions: _MEMORY: 1Gi _CPU: "1" _MIN_INSTANCES: "1" - _MAX_INSTANCES: "3" + # max=1, deliberately. Every scale-out was a cold start that a live request + # paid for: measured 2026-08-29..09-10, Cloud Run started 464 extra instances + # (16-54 a day) while the warm one sat at a concurrency of ~2, because a page + # load fans out 4-6 API calls in the same instant — and it pinned one call of + # each burst to the new instance for the full 9-13 s start. That was 8-37 + # browser requests a day with referer anyplot.ai, median 11.3 s, on the four + # calls the start page makes. With a single instance a burst queues on the + # warm one for milliseconds instead. The instance can carry it: request p50 + # 32 ms, CPU p95 6 %, more than 9 concurrent requests in ~2 minutes a day, + # no 429 in the window. Raising --concurrency on its own would not have + # helped (the starts never came from the concurrency target); it is raised + # below so the cap has headroom. If 429s ever appear, max=2 is the next + # step, not a bigger limit. Min stays 1: the start classes that remain + # (deploys, Google's roughly daily instance replacement) are exactly what a + # warm instance is for. + _MAX_INSTANCES: "1" # Cloudflare Access (Zero Trust) for /debug/* — the team domain is stable; # the AUD must be filled in once the Self-hosted Application is created in # the Cloudflare Zero Trust dashboard. With AUD unset (or with no email @@ -86,7 +101,13 @@ steps: # next deploy, which is the trade already accepted for the secrets. - "--update-env-vars=^|^ENVIRONMENT=production|GOOGLE_CLOUD_PROJECT=$PROJECT_ID|GCS_BUCKET=anyplot-images|CF_ACCESS_TEAM_DOMAIN=${_CF_ACCESS_TEAM_DOMAIN}|CF_ACCESS_AUD=${_CF_ACCESS_AUD}|ADMIN_ALLOWED_EMAILS=${_ADMIN_ALLOWED_EMAILS}" - "--cpu-throttling" - - "--concurrency=15" + # 40, not 15: with _MAX_INSTANCES=1 this limit is the only capacity the + # service has, and 15 was a default nobody had measured against. On this + # async stack everything cached answers in milliseconds; the one CPU-heavy + # path, OG image compositing, runs in a worker thread behind its own + # two-slot semaphore (api/routers/og_images.py), so 40 in-flight requests + # cannot become 40 concurrent PIL renders. + - "--concurrency=40" - "--timeout=600" # Deploy WITHOUT routing traffic: the smoke step probes this revision on # its tag URL first, and only the promote step below shifts traffic — so a diff --git a/api/main.py b/api/main.py index 451eb0da4c1..23c4ab22e66 100644 --- a/api/main.py +++ b/api/main.py @@ -9,15 +9,16 @@ load_dotenv() +import asyncio # noqa: E402 import logging # noqa: E402 -from contextlib import asynccontextmanager # noqa: E402 +from contextlib import asynccontextmanager, suppress # noqa: E402 from fastapi import FastAPI, HTTPException, Request, Response # noqa: E402 from fastapi.middleware.cors import CORSMiddleware # noqa: E402 from starlette.middleware.gzip import GZipMiddleware # noqa: E402 from api.analytics import classify_asset, track_asset_fetch, track_bot_fetch # noqa: E402 -from api.cache import cache_key, set_cache # noqa: E402 +from api.cache import cache_key, get_or_set_cache # noqa: E402 from api.exceptions import ( # noqa: E402 AnyplotException, anyplot_exception_handler, @@ -70,6 +71,13 @@ mcp_http_app = mcp_server.http_app(path="/", stateless_http=True) +# Strong reference to the running prewarm task. asyncio keeps only a weak +# reference to a task, so without this the prewarm could be garbage-collected +# mid-flight; it also gives the shutdown path something to cancel before the +# DB engine goes away. +_prewarm_task: asyncio.Task[None] | None = None + + async def _prewarm_cache() -> None: """Populate the in-memory cache for the endpoints the frontend hits on page load: the four AppDataProvider metadata calls (/stats, /libraries, @@ -81,8 +89,17 @@ async def _prewarm_cache() -> None: The cache lives per Cloud Run instance, so every new instance that comes up from autoscale or a cold start would otherwise force its first user to wait on the full DB roundtrip — which is exactly the user-reported - "manchmal echt lange" on the NumbersStrip and the /specs page. Prewarming - runs once per process startup so the first request hits a warm cache. + "manchmal echt lange" on the NumbersStrip and the /specs page. + + It runs as a BACKGROUND task, scheduled by `_start_prewarm` from the + lifespan, not awaited there. uvicorn runs the lifespan to completion + before it binds the socket, so an awaited prewarm sat inside every cold + start: a stable 2.0–3.6 s of an ~11 s start (197 starts measured on + 2026-09-10), paid by Cloud Run's startup probe and by whichever request + was already pinned to the starting instance. Each key goes through + `get_or_set_cache`, so a request that arrives first computes the key + itself under the per-key lock and the prewarm finds it cached; the two + never run the same query twice or reset each other's refresh clock. Failures here are non-fatal: log and continue. A failed prewarm just means the first user request takes the cold-cache path it would have @@ -100,13 +117,29 @@ async def _prewarm_cache() -> None: ) for key, factory in refreshers: try: - result = await factory() - set_cache(cache_key(key), result) + await get_or_set_cache(cache_key(key), factory) logger.info("Cache prewarm: %s OK", key) except Exception: logger.warning("Cache prewarm failed for %s — falling back to lazy load", key, exc_info=True) +def _start_prewarm() -> asyncio.Task[None]: + """Schedule `_prewarm_cache` as a task and keep the strong reference.""" + global _prewarm_task + _prewarm_task = asyncio.create_task(_prewarm_cache(), name="cache-prewarm") + return _prewarm_task + + +async def _stop_prewarm() -> None: + """Cancel a prewarm that is still running, so it cannot touch a closed DB engine.""" + global _prewarm_task + task, _prewarm_task = _prewarm_task, None + if task is not None and not task.done(): + task.cancel() + with suppress(asyncio.CancelledError): + await task + + @asynccontextmanager async def lifespan(app: FastAPI): """Manage application lifecycle.""" @@ -125,7 +158,10 @@ async def lifespan(app: FastAPI): try: await init_db() logger.info("Database connection initialized") - await _prewarm_cache() + # Scheduled, not awaited: the port opens while the prewarm runs. + # `init_db()` stays awaited above — a request must never arrive + # before the engine exists. See `_prewarm_cache` for the numbers. + _start_prewarm() except Exception as e: logger.error(f"Failed to initialize database: {e}") @@ -134,8 +170,10 @@ async def lifespan(app: FastAPI): logger.info("MCP server initialized") yield - # Cleanup database connection + # Cleanup: stop a prewarm that is still in flight BEFORE the engine it + # would query is disposed. logger.info("Shutting down anyplot API...") + await _stop_prewarm() await close_db() diff --git a/api/routers/og_images.py b/api/routers/og_images.py index 431188db5aa..4f6b1648408 100644 --- a/api/routers/og_images.py +++ b/api/routers/og_images.py @@ -2,8 +2,10 @@ import asyncio import logging +from collections.abc import Callable from io import BytesIO from pathlib import Path +from typing import Any, TypeVar import httpx from fastapi import APIRouter, Depends, HTTPException, Request @@ -71,6 +73,25 @@ def _image_to_bytes(img: Image.Image) -> bytes: return buf.getvalue() +_R = TypeVar("_R") + +# PIL compositing is CPU-bound and takes 1–3 s per card. Called inline from an +# `async def`, it held the event loop for that long and stalled every other +# request on the instance — and since 2026-09-10 the service runs as ONE +# instance (api/cloudbuild.yaml), so that stall would be the whole service. +# The renders therefore go to a worker thread. The semaphore caps how many run +# at once: a collage holds six decoded PNGs, and a few concurrent collages are +# what produced the 647 MiB peak of 2026-08-26 against the 1 GiB limit. Two +# slots keep that bounded whatever the request concurrency limit says. +_RENDER_SLOTS = asyncio.Semaphore(2) + + +async def _render(fn: Callable[..., _R], /, *args: Any, **kwargs: Any) -> _R: + """Run a synchronous PIL render off the event loop, at most two at a time.""" + async with _RENDER_SLOTS: + return await asyncio.to_thread(fn, *args, **kwargs) + + router = APIRouter(prefix="/og", tags=["og-images"]) @@ -85,7 +106,9 @@ async def get_home_og_image(request: Request) -> Response: track_og_image(request, page="home", filters=filters) return Response( - content=_get_static_og_image(), media_type="image/png", headers={"Cache-Control": "public, max-age=86400"} + content=await _render(_get_static_og_image), + media_type="image/png", + headers={"Cache-Control": "public, max-age=86400"}, ) @@ -95,7 +118,9 @@ async def get_plots_og_image(request: Request) -> Response: track_og_image(request, page="plots") return Response( - content=_get_static_og_image(), media_type="image/png", headers={"Cache-Control": "public, max-age=86400"} + content=await _render(_get_static_og_image), + media_type="image/png", + headers={"Cache-Control": "public, max-age=86400"}, ) @@ -167,8 +192,8 @@ async def get_branded_impl_image( # Fetch the original plot image image_bytes = await _fetch_image(impl.preview_url) - # Create branded image - branded_bytes = create_branded_og_image(image_bytes, spec_id=spec_id, library=library) + # Create branded image (worker thread, see _render) + branded_bytes = await _render(create_branded_og_image, image_bytes, spec_id=spec_id, library=library) # Cache the result set_cache(key, branded_bytes) @@ -225,8 +250,8 @@ async def get_spec_collage_image( # off the trailing token, and the spec_id is now in the section title. labels = [impl.library_id for impl in selected_impls] - # Create collage - collage_bytes = create_og_collage(images, labels=labels, spec_id=spec_id) + # Create collage (worker thread, see _render) + collage_bytes = await _render(create_og_collage, images, labels=labels, spec_id=spec_id) # Cache the result set_cache(key, collage_bytes) diff --git a/changelog.d/api-single-instance.md b/changelog.d/api-single-instance.md new file mode 100644 index 00000000000..f3b6ef6ea43 --- /dev/null +++ b/changelog.d/api-single-instance.md @@ -0,0 +1,43 @@ +### Changed + +- **`anyplot-api` runs as a single instance: min 1, max 1, concurrency 40.** Every + scale-out was a cold start that a live request paid for. Between 2026-08-29 and + 09-10 Cloud Run started 464 extra instances (16–54 a day) while the warm one sat + at a concurrency of ~2, because a page load fans out 4–6 API calls in the same + instant, and it pinned one call of each burst to the new instance for its full + 9–13 s start — 8 to 37 browser requests a day with referer anyplot.ai, median + 11.3 s, on `/libraries`, `/languages`, `/stats` and `/specs`. With one instance a + burst queues on the warm one for milliseconds. The limit has headroom: request + p50 is 32 ms, CPU p95 6 %, more than 9 concurrent requests occur in about two + minutes a day, and the window saw no 429. `api/cloudbuild.yaml` pins it; if 429s + ever appear, max 2 is the next step. (#11828) +- **The startup cache prewarm no longer blocks the port.** uvicorn runs the + lifespan to completion before it binds the socket, so the six awaited prewarm + queries added a stable ~2.5 s to every cold start (2.0–3.6 s of an ~11 s start, + measured over 197 starts). The prewarm is now a background task that runs each + key through `get_or_set_cache`, so it shares the per-key lock with a request that + arrives first instead of duplicating its query, and it is cancelled on shutdown + before the DB engine closes. (#11828) +- **OG image compositing runs in a worker thread, at most two at once.** The + collage and branded-image endpoints called PIL inline in the event loop, which + stalled every other request on the instance for the 1–3 s a render takes, and a + handful of concurrent collages produced the 647 MiB memory peak of 2026-08-26 + against the 1 GiB limit. With a single instance both would hit the whole + service; the renders now go through `asyncio.to_thread` behind a two-slot + semaphore. (#11828) + +### Fixed + +- **An empty MonoLisa cache file no longer disables the brand font.** A failed + download leaves a 0-byte file under `/tmp/anyplot-fonts/` (the client opens the + target before it fetches), which `_get_monolisa_font_path` accepted as the + cached font; PIL then failed to open it on every render and the OG cards + silently fell back to DejaVu for the life of the instance (and the swash test + failed locally instead of skipping). Empty files now count as missing, a failed + download deletes its leftover, and the failure is remembered in-process for ten + minutes so a GCS outage costs one attempt per cooldown rather than one per + render. (#11828) +- **`docs/reference/performance.md` describes the live services again.** The + infrastructure table still listed the frontend at min-instances=1 with 256Mi + (it has scaled to zero on 512Mi since 2026-08-29) and Cloud SQL as `db-g1-small` + (it is `db-custom-1-3840` on a 3-year commitment). (#11828) diff --git a/core/images.py b/core/images.py index a9f8dff5b1c..c18db5d7f94 100644 --- a/core/images.py +++ b/core/images.py @@ -17,6 +17,8 @@ """ import logging +import threading +import time from io import BytesIO from pathlib import Path @@ -385,6 +387,33 @@ def create_responsive_variants( # ============================================================================= +def _is_cached_font(path: Path) -> bool: + """A usable cache entry is a file with bytes in it — see `_get_monolisa_font_path`.""" + return path.is_file() and path.stat().st_size > 0 + + +# Seconds before a failed font download is attempted again, per cache file. +# A failed attempt (no credentials, no network) costs a full client timeout, +# and until 2026-09-10 the 0-byte file it left behind was, by accident, what +# stopped every later render from retrying. The failure is remembered here +# instead, in-process, so the cost is one attempt per cooldown — not one per +# render, and not "never again" for the life of the instance. +_FONT_DOWNLOAD_RETRY_AFTER = 600.0 +_font_download_failed_at: dict[str, float] = {} + +# One lock around check → download → publish. Renders run in worker threads +# (api/routers/og_images.py, two at a time), and two of them can miss the cache +# in the same instant; without the lock both download to the same path and a +# failing one could delete what its peer just wrote. Re-entrant because the +# italic branch falls back to the upright font through the same function. +_font_cache_lock = threading.RLock() + + +def _download_recently_failed(cache_filename: str) -> bool: + failed_at = _font_download_failed_at.get(cache_filename) + return failed_at is not None and time.monotonic() - failed_at < _FONT_DOWNLOAD_RETRY_AFTER + + def _get_monolisa_font_path(local_only: bool = False, italic: bool = False) -> Path | None: """Get path to MonoLisa font, downloading from GCS if needed. @@ -400,32 +429,50 @@ def _get_monolisa_font_path(local_only: bool = False, italic: bool = False) -> P gcs_blob = MONOLISA_ITALIC_FONT_PATH if italic else MONOLISA_FONT_PATH cached_font = FONT_CACHE_DIR / cache_filename - # Return cached font if exists - if cached_font.exists(): - return cached_font - - if local_only: - # If italic was requested but isn't cached, fall through to upright cache. - if italic: - upright_cached = FONT_CACHE_DIR / "MonoLisaVariableNormal.ttf" - if upright_cached.exists(): - return upright_cached - return None - - # Try to download from GCS - try: - from google.cloud import storage - - FONT_CACHE_DIR.mkdir(parents=True, exist_ok=True) - - client = storage.Client() - bucket = client.bucket(GCS_STATIC_BUCKET) - blob = bucket.blob(gcs_blob) - blob.download_to_filename(str(cached_font)) - logger.info(f"Downloaded MonoLisa font to {cached_font}") - return cached_font - except Exception as e: - logger.warning(f"Could not load MonoLisa font from GCS ({gcs_blob}): {e}") + with _font_cache_lock: + # Return the cached font if it exists AND has content. An interrupted + # download leaves a 0-byte file behind (two of them, dated 2026-08-17, + # sat in a local cache until 2026-09-10); `exists()` accepted it, PIL + # failed to open it on every render, and the card silently fell back to + # DejaVu. An empty file is treated as missing so the download below + # replaces it. + if _is_cached_font(cached_font): + return cached_font + + if local_only: + # If italic was requested but isn't cached, fall through to upright cache. + if italic: + upright_cached = FONT_CACHE_DIR / "MonoLisaVariableNormal.ttf" + if _is_cached_font(upright_cached): + return upright_cached + return None + + # Try to download from GCS — once per cooldown, see _FONT_DOWNLOAD_RETRY_AFTER. + # The bytes land in a `.part` file first and are published with one + # rename: a reader sees either no font or the whole font, never the + # partial file `download_to_filename` opens before it fetches. + if not _download_recently_failed(cache_filename): + partial = cached_font.with_name(cached_font.name + ".part") + try: + from google.cloud import storage + + FONT_CACHE_DIR.mkdir(parents=True, exist_ok=True) + + client = storage.Client() + bucket = client.bucket(GCS_STATIC_BUCKET) + blob = bucket.blob(gcs_blob) + blob.download_to_filename(str(partial)) + if not _is_cached_font(partial): + raise OSError(f"download of {gcs_blob} produced an empty file") + partial.replace(cached_font) + logger.info(f"Downloaded MonoLisa font to {cached_font}") + return cached_font + except Exception as e: + logger.warning(f"Could not load MonoLisa font from GCS ({gcs_blob}): {e}") + # Only the partial file is ever removed — the published one is + # somebody's successful download and stays. + partial.unlink(missing_ok=True) + _font_download_failed_at[cache_filename] = time.monotonic() # Italic missing → fall back to upright so we still get *some* MonoLisa. if italic: return _get_monolisa_font_path(local_only=local_only, italic=False) diff --git a/docs/reference/performance.md b/docs/reference/performance.md index 39e965a8793..f4d9c6e0374 100644 --- a/docs/reference/performance.md +++ b/docs/reference/performance.md @@ -6,9 +6,9 @@ Backend API response time measurements for anyplot-backend (Cloud Run, europe-we | Component | Config | Notes | |-----------|--------|-------| -| Cloud Run (backend) | 1 vCPU, 1Gi RAM, min-instances=1 | gen2, startup-cpu-boost=true | -| Cloud Run (frontend) | 1 vCPU, 256Mi RAM, min-instances=1 | nginx serving SPA | -| Cloud SQL | `db-g1-small`, PostgreSQL 18, PD-SSD 10GB | 0.5 shared vCPU, 1.7GB RAM | +| Cloud Run (backend) | 1 vCPU, 1Gi RAM, min-instances=1, max-instances=1, concurrency 40 | gen2, startup-cpu-boost=true. Single instance since 2026-09-10: every scale-out was a 9-13 s cold start that a live request paid for (see `api/cloudbuild.yaml`) | +| Cloud Run (frontend) | 1 vCPU, 512Mi RAM, min-instances=0, max-instances=3, concurrency 15 | nginx serving SPA; scales to zero since 2026-08-29, crawler traffic keeps the instance alive | +| Cloud SQL | `db-custom-1-3840`, PostgreSQL 18, PD-SSD 10GB | 1 vCPU, 3.75GB RAM; 3-year commitment until 2029-04-01 | | Cache | In-memory TTLCache, 86400s TTL (24h), max 1000 entries | Per-instance, stampede-protected, stale-while-revalidate | ## Baseline: before `--no-cpu-throttling` (March 24, 2026) diff --git a/tests/unit/api/test_main.py b/tests/unit/api/test_main.py index b9dd978d236..bc92d98d833 100644 --- a/tests/unit/api/test_main.py +++ b/tests/unit/api/test_main.py @@ -7,11 +7,14 @@ - Hello endpoint (/hello/{name}) """ +import asyncio +from contextlib import asynccontextmanager from unittest.mock import AsyncMock, MagicMock, patch import pytest from fastapi.testclient import TestClient +from api.cache import clear_cache from api.main import app, fastapi_app from api.version import APP_VERSION from core.constants import LIBRARIES_METADATA @@ -376,13 +379,16 @@ class TestPrewarmCache: the first request to a fresh instance pays the full DB roundtrip and the user-visible NumbersStrip + /specs page sit on placeholders. The hook populates the four metadata caches plus the two heavy user-facing - payloads (unfiltered gallery `filter:all` and /specs/map) before the - app starts serving. + payloads (unfiltered gallery `filter:all` and /specs/map). It runs as a + background task so the port opens while it works — uvicorn binds only + after the lifespan returns, and an awaited prewarm sat inside every + cold start (2.0–3.6 s of ~11 s, measured 2026-09-10). """ async def test_prewarm_populates_all_caches(self) -> None: from api.main import _prewarm_cache + clear_cache() # get_or_set_cache skips the factory on a hit; start from empty with ( patch("api.main._refresh_stats", new=AsyncMock(return_value="STATS")) as m_stats, patch("api.main._refresh_libraries", new=AsyncMock(return_value="LIBS")) as m_libs, @@ -390,7 +396,7 @@ async def test_prewarm_populates_all_caches(self) -> None: patch("api.main._refresh_specs_list", new=AsyncMock(return_value="SPECS")) as m_specs, patch("api.main._refresh_specs_map", new=AsyncMock(return_value="MAP")) as m_map, patch("api.main._refresh_filter_all", new=AsyncMock(return_value="FILTER")) as m_filter, - patch("api.main.set_cache") as m_set, + patch("api.cache.set_cache") as m_set, ): await _prewarm_cache() @@ -409,6 +415,7 @@ async def test_prewarm_swallows_one_failure_and_continues(self) -> None: endpoints just fall back to lazy load on the first user request.""" from api.main import _prewarm_cache + clear_cache() with ( patch("api.main._refresh_stats", new=AsyncMock(side_effect=RuntimeError("db down"))), patch("api.main._refresh_libraries", new=AsyncMock(return_value="LIBS")) as m_libs, @@ -416,7 +423,7 @@ async def test_prewarm_swallows_one_failure_and_continues(self) -> None: patch("api.main._refresh_specs_list", new=AsyncMock(return_value="SPECS")) as m_specs, patch("api.main._refresh_specs_map", new=AsyncMock(return_value="MAP")) as m_map, patch("api.main._refresh_filter_all", new=AsyncMock(return_value="FILTER")) as m_filter, - patch("api.main.set_cache") as m_set, + patch("api.cache.set_cache") as m_set, ): await _prewarm_cache() # must not raise @@ -428,6 +435,67 @@ async def test_prewarm_swallows_one_failure_and_continues(self) -> None: cached_keys = {call.args[0] for call in m_set.call_args_list} assert cached_keys == {"libraries", "languages", "specs_list", "specs_map", "filter:all"} + async def test_prewarm_shares_the_per_key_lock_with_requests(self) -> None: + """A key a request already computed is not computed again by the + prewarm — it goes through get_or_set_cache, not a bare set_cache, so + the two never duplicate the DB work or reset each other's refresh + clock.""" + from api.cache import set_cache + from api.main import _prewarm_cache + + clear_cache() + set_cache("stats", "ALREADY-THERE") + with ( + patch("api.main._refresh_stats", new=AsyncMock(return_value="STATS")) as m_stats, + patch("api.main._refresh_libraries", new=AsyncMock(return_value="LIBS")), + patch("api.main._refresh_languages", new=AsyncMock(return_value="LANGS")), + patch("api.main._refresh_specs_list", new=AsyncMock(return_value="SPECS")), + patch("api.main._refresh_specs_map", new=AsyncMock(return_value="MAP")), + patch("api.main._refresh_filter_all", new=AsyncMock(return_value="FILTER")), + ): + await _prewarm_cache() + + m_stats.assert_not_awaited() + clear_cache() + + async def test_lifespan_does_not_wait_for_the_prewarm(self) -> None: + """Startup finishes while the prewarm is still running (that is the + whole point: the port opens without it), and shutdown cancels what is + left before the DB engine is closed.""" + import api.main as main + + started = asyncio.Event() + release = asyncio.Event() + + async def slow_prewarm() -> None: + started.set() + await release.wait() + + @asynccontextmanager + async def noop_lifespan(_app): + yield + + mcp_stub = MagicMock() + mcp_stub.lifespan = noop_lifespan + close_db = AsyncMock() + + with ( + patch("api.main.is_db_configured", return_value=True), + patch("api.main.init_db", new=AsyncMock()), + patch("api.main.close_db", new=close_db), + patch("api.main.mcp_http_app", mcp_stub), + patch("api.main._prewarm_cache", new=slow_prewarm), + ): + async with main.lifespan(fastapi_app): + # Startup returned; the prewarm is running, not finished. + await asyncio.wait_for(started.wait(), timeout=1) + assert main._prewarm_task is not None + assert not main._prewarm_task.done() + + # Shutdown cancelled the prewarm and only then closed the engine. + assert main._prewarm_task is None + close_db.assert_awaited_once() + async def test_prewarm_filter_all_key_matches_endpoint_cache_key(self) -> None: """The prewarm must write the exact key the /plots/filter endpoint reads for the unfiltered request — a drifted key would silently diff --git a/tests/unit/core/test_images.py b/tests/unit/core/test_images.py index 1d8555f42f8..0cbf5963598 100644 --- a/tests/unit/core/test_images.py +++ b/tests/unit/core/test_images.py @@ -829,6 +829,53 @@ def test_create_og_collage_dark_theme(self, sample_plot_image: Path) -> None: class TestGetMonolisaFontPath: """Tests for _get_monolisa_font_path GCS font download.""" + @pytest.fixture(autouse=True) + def _forget_download_failures(self): + """The download cooldown is process-wide state; no test inherits another's failure.""" + import core.images + + core.images._font_download_failed_at.clear() + yield + core.images._font_download_failed_at.clear() + + def test_empty_cached_file_counts_as_missing(self, tmp_path: Path) -> None: + """A 0-byte leftover of a failed download is not the font.""" + from unittest.mock import patch + + import core.images + + (tmp_path / "MonoLisaVariableNormal.ttf").touch() + + with patch("core.images.FONT_CACHE_DIR", tmp_path): + assert core.images._get_monolisa_font_path(local_only=True) is None + + def test_failed_download_leaves_no_file_and_is_not_retried_at_once(self, tmp_path: Path) -> None: + """A failed download must not leave a 0-byte file behind (PIL would fail + on it for the life of the instance) and must not be retried on the very + next render — one attempt per cooldown.""" + from unittest.mock import MagicMock, patch + + import core.images + + target = tmp_path / "MonoLisaVariableNormal.ttf" + + def leave_empty_file(filename: str) -> None: + Path(filename).touch() # what download_to_filename does before it fails + raise RuntimeError("403 Forbidden") + + mock_client = MagicMock() + mock_client.bucket.return_value.blob.return_value.download_to_filename.side_effect = leave_empty_file + + with ( + patch("core.images.FONT_CACHE_DIR", tmp_path), + patch("google.cloud.storage.Client", return_value=mock_client) as client_cls, + ): + assert core.images._get_monolisa_font_path() is None + assert not target.exists() + assert core.images._get_monolisa_font_path() is None + + assert client_cls.call_count == 1 + def test_returns_cached_font_without_gcs_call(self, tmp_path: Path) -> None: """Should return cached font path without calling GCS when file exists.""" from unittest.mock import patch @@ -851,13 +898,14 @@ def test_downloads_from_gcs_on_cache_miss(self, tmp_path: Path) -> None: import core.images mock_blob = MagicMock() + # The client writes into a `.part` file that is then renamed into place. + mock_blob.download_to_filename.side_effect = lambda filename: Path(filename).write_bytes(b"font bytes") mock_bucket = MagicMock() mock_bucket.blob.return_value = mock_blob mock_client = MagicMock() mock_client.bucket.return_value = mock_bucket with ( - patch.object(type(tmp_path / "MonoLisaVariableNormal.ttf"), "exists", return_value=False), patch("core.images.FONT_CACHE_DIR", tmp_path), patch("google.cloud.storage.Client", return_value=mock_client), ): @@ -868,6 +916,66 @@ def test_downloads_from_gcs_on_cache_miss(self, tmp_path: Path) -> None: mock_bucket.blob.assert_called_once_with("fonts/MonoLisaVariableNormal.ttf") mock_blob.download_to_filename.assert_called_once() assert result == tmp_path / "MonoLisaVariableNormal.ttf" + assert result.read_bytes() == b"font bytes" + assert not (tmp_path / "MonoLisaVariableNormal.ttf.part").exists() + + def test_concurrent_misses_download_once_and_never_lose_the_font(self, tmp_path: Path) -> None: + """Renders run in worker threads, two at a time. Two threads that miss the + cache in the same instant must end up with ONE download and a usable + font — the second waits on the lock and then finds the cache — and a + failing peer must never delete a font a successful one published.""" + import threading + import time + from concurrent.futures import ThreadPoolExecutor + from unittest.mock import MagicMock, patch + + import core.images + + started = threading.Event() + + def slow_download(filename: str) -> None: + started.set() + time.sleep(0.05) # long enough for the second thread to queue on the lock + Path(filename).write_bytes(b"font bytes") + + mock_blob = MagicMock() + mock_blob.download_to_filename.side_effect = slow_download + mock_client = MagicMock() + mock_client.bucket.return_value.blob.return_value = mock_blob + + with ( + patch("core.images.FONT_CACHE_DIR", tmp_path), + patch("google.cloud.storage.Client", return_value=mock_client), + ThreadPoolExecutor(max_workers=2) as pool, + ): + first = pool.submit(core.images._get_monolisa_font_path) + started.wait(timeout=1) + second = pool.submit(core.images._get_monolisa_font_path) + results = {first.result(timeout=5), second.result(timeout=5)} + + assert results == {tmp_path / "MonoLisaVariableNormal.ttf"} + assert mock_blob.download_to_filename.call_count == 1 + assert (tmp_path / "MonoLisaVariableNormal.ttf").read_bytes() == b"font bytes" + + # A later failure (cooldown expired, GCS down) must not touch the + # published file: only its own `.part` is removed. + core.images._font_download_failed_at.clear() + (tmp_path / "MonoLisaVariableNormal.ttf").unlink() + (tmp_path / "MonoLisaVariableItalic.ttf").write_bytes(b"italic bytes") + + def fail_after_opening(filename: str) -> None: + Path(filename).touch() + raise RuntimeError("403 Forbidden") + + mock_blob.download_to_filename.side_effect = fail_after_opening + with ( + patch("core.images.FONT_CACHE_DIR", tmp_path), + patch("google.cloud.storage.Client", return_value=mock_client), + ): + assert core.images._get_monolisa_font_path() is None + + assert (tmp_path / "MonoLisaVariableItalic.ttf").read_bytes() == b"italic bytes" + assert not (tmp_path / "MonoLisaVariableNormal.ttf.part").exists() def test_returns_none_on_gcs_exception(self, tmp_path: Path) -> None: """Should return None gracefully when GCS download fails.""" @@ -1005,9 +1113,11 @@ def test_home_og_image_renders_swashes(self) -> None: from PIL import ImageChops - from core.images import FONT_CACHE_DIR, create_home_og_image + from core.images import FONT_CACHE_DIR, _is_cached_font, create_home_og_image - if not (FONT_CACHE_DIR / "MonoLisaVariableItalic.ttf").exists(): + # `_is_cached_font`, not `exists()`: a 0-byte leftover of an interrupted + # download would pass the skip and then fail the pixel comparison. + if not _is_cached_font(FONT_CACHE_DIR / "MonoLisaVariableItalic.ttf"): pytest.skip("MonoLisa italic not cached locally — swash rendering cannot be verified") with_features = create_home_og_image(theme="light")