From 5aa56cf71623947e0e9d653c65c9c0ec9bf8f3cc Mon Sep 17 00:00:00 2001 From: Hayden <154503486+groupthinking@users.noreply.github.com> Date: Mon, 3 Aug 2026 18:27:44 -0500 Subject: [PATCH 1/4] perf: offload video-detail cache read, drop the stat probe GET /api/v2/videos/{video_id} performed its whole cache lookup inline on the event loop: Path.exists(), then open(), then a full json.load() of the stored analysis. Only the two syscalls are bounded; the parse scales with the payload the processor wrote, so a large analysis stalled every other in-flight request on the worker. Add a module-level _read_video_analysis_sync() helper and await it through asyncio.to_thread(), mirroring _collect_processed_videos_sync() from #1288. _get_cache_path() stays on the loop: it is pure string arithmetic. The helper opens directly and treats FileNotFoundError as the miss instead of probing with exists() first. That is one syscall rather than two, and it closes the window in which the entry could be removed between the check and the open - a race that previously surfaced as a 500 rather than the correct 404. Every other OSError still propagates, so a directory or an unreadable entry keeps surfacing as a 500 instead of being reported as a missing video. Deliberately does not reuse RealVideoProcessor._read_cache_file. That helper applies a 24-hour TTL and returns None for anything older; this endpoint has never had a TTL, so reusing it would silently turn every analysis over a day old into a 404. TestVideoDetailIgnoresProcessorCacheTtl pins that. real_video_processor.py is left untouched (claimed by open PR #1237). Verification: 97 passed (88 pre-existing + 9 new). Prove-fail: reverting only the to_thread delegation, keeping the helper defined, fails exactly the two off-loop tests (ticks=0, assert 0 >= 5) and passes the other 95. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../backend/real_api_endpoints.py | 45 +++- tests/unit/test_real_api_endpoints.py | 229 ++++++++++++++++++ 2 files changed, 270 insertions(+), 4 deletions(-) diff --git a/src/youtube_extension/backend/real_api_endpoints.py b/src/youtube_extension/backend/real_api_endpoints.py index 4e8136798..28bdf7167 100644 --- a/src/youtube_extension/backend/real_api_endpoints.py +++ b/src/youtube_extension/backend/real_api_endpoints.py @@ -85,6 +85,38 @@ def _collect_processed_videos_sync(cache_dir: Path) -> list[dict[str, Any]]: return processed_videos +def _read_video_analysis_sync(cache_path: Path) -> Optional[dict[str, Any]]: + """Read and parse a single cached video analysis. + + This performs blocking filesystem work (``open()`` and a full + ``json.load()`` of the analysis payload) and is therefore intended to be + executed in a worker thread via :func:`asyncio.to_thread` rather than + directly on the event loop. + + Opening directly and treating :class:`FileNotFoundError` as the miss + replaces a separate ``Path.exists()`` probe. That is one syscall instead of + two, and it closes the window in which the entry could be removed between + the check and the open. Every other ``OSError`` still propagates, so a + directory or an unreadable entry keeps surfacing as a 500 rather than being + silently reported as a missing video. + + Deliberately does *not* apply the processor's cache TTL. This endpoint has + always served a cached analysis regardless of age, whereas + ``RealVideoProcessor._read_cache_file`` treats anything older than + ``_CACHE_TTL_SECONDS`` as a miss; reusing it here would turn every analysis + over 24 hours old into a 404. + + Returns the parsed payload, or ``None`` when no cache entry exists. + """ + try: + handle = open(cache_path, encoding="utf-8") + except FileNotFoundError: + return None + + with handle as f: + return json.load(f) + + # Pydantic models for API requests/responses class VideoProcessingRequest(BaseModel): video_url: str = Field(..., description="YouTube video URL or ID") @@ -268,15 +300,20 @@ async def get_video_analysis(video_id: str): processor = get_real_video_processor() cache_path = processor._get_cache_path(video_id) - if not cache_path.exists(): + # Reading an entry opens and JSON-parses a file whose size is set + # by the stored analysis payload, so the parse cost scales with the + # video rather than being bounded. Run it in a worker thread so a + # large analysis cannot stall the event loop for every other + # in-flight request. Building the path stays here: it is pure + # string arithmetic and touches no filesystem. + video_data = await asyncio.to_thread(_read_video_analysis_sync, cache_path) + + if video_data is None: raise HTTPException( status_code=404, detail=f"Video analysis not found: {video_id}" ) - with open(cache_path, encoding='utf-8') as f: - video_data = json.load(f) - return video_data except HTTPException: diff --git a/tests/unit/test_real_api_endpoints.py b/tests/unit/test_real_api_endpoints.py index 09b6b50af..aa95ad703 100644 --- a/tests/unit/test_real_api_endpoints.py +++ b/tests/unit/test_real_api_endpoints.py @@ -23,6 +23,7 @@ import asyncio import contextlib import json +import os import sys import threading import time @@ -49,6 +50,7 @@ VideoProcessingRequest, VideoValidationRequest, _collect_processed_videos_sync, + _read_video_analysis_sync, init_real_api_services, setup_real_api_endpoints, ) @@ -1059,3 +1061,230 @@ def test_non_matching_files_are_ignored(self, tmp_cache): (tmp_cache / "other.json").write_text("{}", encoding="utf-8") assert _collect_processed_videos_sync(tmp_cache) == [] + + +# =========================================================================== +# GET /api/v2/videos/{video_id} - blocking I/O offload (performance regression) +# =========================================================================== + + +class _SlowReadPath: + """Path-like proxy whose resolution blocks, standing in for a large read. + + ``open()`` resolves a non-``str`` argument through ``__fspath__``, so the + sleep lands inside the blocking read itself rather than around it. If the + read is dispatched to a worker thread the loop stays free for that whole + window; if it is not, the loop is pinned for exactly this long. + """ + + def __init__(self, real_path: Path, duration: float) -> None: + self._real = real_path + self._duration = duration + + def __fspath__(self) -> str: + time.sleep(self._duration) + return str(self._real) + + def __str__(self) -> str: + return str(self._real) + + +class TestVideoDetailOffloadsBlockingIO: + """The single-entry cache read must not run on the event loop thread.""" + + def test_cache_entry_read_runs_off_the_event_loop_thread( + self, api_app, mock_processor, mock_youtube, mock_cost_monitor, tmp_cache + ): + tmp_cache.mkdir(parents=True, exist_ok=True) + cache_file = _write_cache_file(tmp_cache, "auJzb1D-fag") + + read_thread_ids: list[int] = [] + mock_processor._get_cache_path.return_value = _ThreadRecordingPath( + cache_file, read_thread_ids + ) + + # get_real_video_processor() is invoked by the handler *on the event + # loop thread*, immediately before the read is dispatched. Recording it + # here gives us the loop's thread id without assuming the test itself + # runs on that loop. + loop_thread_ids: list[int] = [] + + def _record_loop_thread(): + loop_thread_ids.append(threading.get_ident()) + return mock_processor + + with ( + patch( + "youtube_extension.backend.real_api_endpoints.get_real_video_processor", + side_effect=_record_loop_thread, + ), + patch( + "youtube_extension.backend.real_api_endpoints.get_youtube_service", + return_value=mock_youtube, + ), + patch( + "youtube_extension.backend.real_api_endpoints.cost_monitor", + mock_cost_monitor, + ), + ): + with TestClient(api_app, raise_server_exceptions=False) as c: + response = c.get("/api/v2/videos/auJzb1D-fag") + + assert response.status_code == 200 + assert response.json()["video_id"] == "auJzb1D-fag" + + assert loop_thread_ids, "handler never resolved the processor" + assert read_thread_ids, "cache entry was never read" + + loop_thread_id = loop_thread_ids[0] + assert all(tid != loop_thread_id for tid in read_thread_ids), ( + "blocking cache entry read ran on the event loop thread " + f"({loop_thread_id}); observed {read_thread_ids}" + ) + + async def test_event_loop_stays_responsive_during_cache_read( + self, api_app, mock_processor, mock_youtube, mock_cost_monitor, tmp_cache + ): + """A slow read must not starve other tasks on the loop.""" + tmp_cache.mkdir(parents=True, exist_ok=True) + cache_file = _write_cache_file(tmp_cache, "auJzb1D-fag") + + read_duration = 0.30 + mock_processor._get_cache_path.return_value = _SlowReadPath( + cache_file, read_duration + ) + + heartbeats = 0 + + async def _heartbeat(): + nonlocal heartbeats + while True: + await asyncio.sleep(0.01) + heartbeats += 1 + + with ( + patch( + "youtube_extension.backend.real_api_endpoints.get_real_video_processor", + return_value=mock_processor, + ), + patch( + "youtube_extension.backend.real_api_endpoints.get_youtube_service", + return_value=mock_youtube, + ), + patch( + "youtube_extension.backend.real_api_endpoints.cost_monitor", + mock_cost_monitor, + ), + ): + transport = httpx.ASGITransport(app=api_app) + async with httpx.AsyncClient( + transport=transport, base_url="http://testserver" + ) as ac: + ticker = asyncio.create_task(_heartbeat()) + try: + response = await ac.get("/api/v2/videos/auJzb1D-fag") + finally: + ticker.cancel() + with contextlib.suppress(asyncio.CancelledError): + await ticker + + assert response.status_code == 200 + # A responsive loop ticks ~30x during a 0.30s read. Assert a very + # conservative fraction of that to stay robust on loaded CI runners, + # while still failing outright when the loop is fully blocked. + assert ( + heartbeats >= 5 + ), f"event loop was starved during the cache read (ticks={heartbeats})" + + def test_offloaded_read_returns_same_payload( + self, client, mock_processor, tmp_cache + ): + """Offloading must not change the response contract.""" + tmp_cache.mkdir(parents=True, exist_ok=True) + cache_file = _write_cache_file(tmp_cache, "auJzb1D-fag") + mock_processor._get_cache_path.return_value = cache_file + + response = client.get("/api/v2/videos/auJzb1D-fag") + + assert response.status_code == 200 + assert response.json() == _read_video_analysis_sync(cache_file) + + +class TestReadVideoAnalysisSync: + """Direct coverage of the extracted blocking helper.""" + + def test_missing_file_returns_none(self, tmp_path): + assert _read_video_analysis_sync(tmp_path / "absent_processed.json") is None + + def test_existing_file_returns_parsed_payload(self, tmp_cache): + tmp_cache.mkdir(parents=True, exist_ok=True) + cache_file = _write_cache_file(tmp_cache, "auJzb1D-fag") + + result = _read_video_analysis_sync(cache_file) + + assert result is not None + assert result["video_id"] == "auJzb1D-fag" + assert result == json.loads(cache_file.read_text(encoding="utf-8")) + + def test_corrupt_entry_raises_rather_than_reporting_a_miss(self, tmp_cache): + """A damaged entry must surface as a 500, never as a 404. + + Only ``FileNotFoundError`` means "no such analysis"; anything else is a + real fault and has to keep propagating. + """ + tmp_cache.mkdir(parents=True, exist_ok=True) + cache_file = tmp_cache / "bad_processed.json" + cache_file.write_text("{invalid json", encoding="utf-8") + + with pytest.raises(json.JSONDecodeError): + _read_video_analysis_sync(cache_file) + + def test_directory_path_raises_rather_than_reporting_a_miss(self, tmp_cache): + """Opening a directory is an ``OSError`` but not a missing entry.""" + tmp_cache.mkdir(parents=True, exist_ok=True) + directory = tmp_cache / "a_directory_processed.json" + directory.mkdir() + + with pytest.raises(OSError) as excinfo: + _read_video_analysis_sync(directory) + + assert not isinstance(excinfo.value, FileNotFoundError) + + +class TestVideoDetailIgnoresProcessorCacheTtl: + """This endpoint has never expired cached analyses, and still must not. + + ``RealVideoProcessor._read_cache_file`` treats any entry older than + ``_CACHE_TTL_SECONDS`` (24 hours) as a miss. Reusing it here to avoid a + second reader would silently turn every analysis over a day old into a 404. + These tests pin the existing contract so that "simplification" cannot land + unnoticed. + """ + + @staticmethod + def _age_file(path: Path, seconds: float) -> None: + stale = path.stat().st_mtime - seconds + os.utime(path, (stale, stale)) + + def test_helper_returns_entry_older_than_processor_ttl(self, tmp_cache): + tmp_cache.mkdir(parents=True, exist_ok=True) + cache_file = _write_cache_file(tmp_cache, "auJzb1D-fag") + self._age_file(cache_file, 48 * 60 * 60) + + result = _read_video_analysis_sync(cache_file) + + assert result is not None + assert result["video_id"] == "auJzb1D-fag" + + def test_endpoint_serves_entry_older_than_processor_ttl( + self, client, mock_processor, tmp_cache + ): + tmp_cache.mkdir(parents=True, exist_ok=True) + cache_file = _write_cache_file(tmp_cache, "auJzb1D-fag") + self._age_file(cache_file, 48 * 60 * 60) + mock_processor._get_cache_path.return_value = cache_file + + response = client.get("/api/v2/videos/auJzb1D-fag") + + assert response.status_code == 200 + assert response.json()["video_id"] == "auJzb1D-fag" From 7eefa9aae550ccdd76a85cbdf1763963ffa5d00a Mon Sep 17 00:00:00 2001 From: Hayden <154503486+groupthinking@users.noreply.github.com> Date: Mon, 3 Aug 2026 19:46:18 -0500 Subject: [PATCH 2/4] fix: keep JSON null and null-byte ids off the 404/500 path Review of the parent commit surfaced two behaviour regressions introduced by replacing the ``exists()`` + ``open()`` pair with a single ``open()``. 1. A cache entry whose content is the JSON literal ``null`` parses to ``None``, which the handler could not distinguish from "no entry". ``main`` served it as a 200; the parent commit turned it into a 404. Fixed with a module-level ``_CACHE_MISS`` sentinel and an identity check, so every falsy payload (``null``, ``{}``, ``[]``, ``""``, ``0``, ``false``) keeps its 200. 2. ``Path.exists()`` swallows ``ValueError`` as well as ``OSError``, so a ``video_id`` carrying an embedded null byte (``GET /api/v2/videos/%00``) used to report the entry as absent and return 404. A bare ``open()`` let the ``ValueError`` escape and turned that into a 500. Fixed by treating ``ValueError`` as a miss alongside ``FileNotFoundError``. Every other ``OSError`` still propagates, so the directory case and the corrupt-JSON case keep their 500s. Verified with a four-case version-swap parity probe (null-content entry, control object, absent entry, %00) showing byte-identical status codes and response bodies between ``main`` and this branch. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../backend/real_api_endpoints.py | 33 +++-- tests/unit/test_real_api_endpoints.py | 134 +++++++++++++++++- 2 files changed, 154 insertions(+), 13 deletions(-) diff --git a/src/youtube_extension/backend/real_api_endpoints.py b/src/youtube_extension/backend/real_api_endpoints.py index 28bdf7167..141c9881b 100644 --- a/src/youtube_extension/backend/real_api_endpoints.py +++ b/src/youtube_extension/backend/real_api_endpoints.py @@ -13,7 +13,7 @@ import os from datetime import datetime, timezone from pathlib import Path -from typing import Any, Optional +from typing import Any, Final, Optional from fastapi import BackgroundTasks, FastAPI, HTTPException from pydantic import BaseModel, Field @@ -28,6 +28,11 @@ # Configure logging logger = logging.getLogger(__name__) +# Distinguishes "no cache entry" from an entry that parses to ``None`` -- which is +# what a file holding the JSON literal ``null`` yields. A plain ``None`` return +# conflates the two and turns a stored ``null`` analysis into a 404. +_CACHE_MISS: Final = object() + def _collect_processed_videos_sync(cache_dir: Path) -> list[dict[str, Any]]: """Scan the processing cache directory and parse every cached result. @@ -85,7 +90,7 @@ def _collect_processed_videos_sync(cache_dir: Path) -> list[dict[str, Any]]: return processed_videos -def _read_video_analysis_sync(cache_path: Path) -> Optional[dict[str, Any]]: +def _read_video_analysis_sync(cache_path: Path) -> Any: """Read and parse a single cached video analysis. This performs blocking filesystem work (``open()`` and a full @@ -100,18 +105,25 @@ def _read_video_analysis_sync(cache_path: Path) -> Optional[dict[str, Any]]: directory or an unreadable entry keeps surfacing as a 500 rather than being silently reported as a missing video. + :class:`ValueError` is also treated as a miss. ``Path.exists()`` swallows it + and reports the entry as absent, so a ``video_id`` carrying an embedded null + byte used to yield a 404; letting the bare ``open()`` raise would turn that + malformed-identifier case into a 500. + Deliberately does *not* apply the processor's cache TTL. This endpoint has always served a cached analysis regardless of age, whereas - ``RealVideoProcessor._read_cache_file`` treats anything older than - ``_CACHE_TTL_SECONDS`` as a miss; reusing it here would turn every analysis - over 24 hours old into a 404. + ``RealVideoProcessor._read_cache_file`` in ``services/real_video_processor.py`` + treats anything older than ``_CACHE_TTL_SECONDS`` (24 hours) as a miss; + reusing it here would turn every analysis over a day old into a 404. - Returns the parsed payload, or ``None`` when no cache entry exists. + Returns the parsed JSON payload, which may legitimately be ``None`` when the + entry holds the literal ``null``. Absence is reported as the distinct + :data:`_CACHE_MISS` sentinel so the two cannot be confused. """ try: handle = open(cache_path, encoding="utf-8") - except FileNotFoundError: - return None + except (FileNotFoundError, ValueError): + return _CACHE_MISS with handle as f: return json.load(f) @@ -308,7 +320,10 @@ async def get_video_analysis(video_id: str): # string arithmetic and touches no filesystem. video_data = await asyncio.to_thread(_read_video_analysis_sync, cache_path) - if video_data is None: + # Identity check against the sentinel, not a truthiness or ``is None`` + # test: an entry holding the JSON literal ``null`` parses to ``None`` + # and has always been served as a 200, so it must not 404 here. + if video_data is _CACHE_MISS: raise HTTPException( status_code=404, detail=f"Video analysis not found: {video_id}" diff --git a/tests/unit/test_real_api_endpoints.py b/tests/unit/test_real_api_endpoints.py index aa95ad703..cbeac4678 100644 --- a/tests/unit/test_real_api_endpoints.py +++ b/tests/unit/test_real_api_endpoints.py @@ -45,6 +45,7 @@ # initialised at import time via init_real_api_services()) # --------------------------------------------------------------------------- from youtube_extension.backend.real_api_endpoints import ( # noqa: E402 + _CACHE_MISS, BatchProcessingRequest, VideoAnalysisResponse, VideoProcessingRequest, @@ -1213,8 +1214,10 @@ def test_offloaded_read_returns_same_payload( class TestReadVideoAnalysisSync: """Direct coverage of the extracted blocking helper.""" - def test_missing_file_returns_none(self, tmp_path): - assert _read_video_analysis_sync(tmp_path / "absent_processed.json") is None + def test_missing_file_returns_the_miss_sentinel(self, tmp_path): + assert ( + _read_video_analysis_sync(tmp_path / "absent_processed.json") is _CACHE_MISS + ) def test_existing_file_returns_parsed_payload(self, tmp_cache): tmp_cache.mkdir(parents=True, exist_ok=True) @@ -1226,11 +1229,50 @@ def test_existing_file_returns_parsed_payload(self, tmp_cache): assert result["video_id"] == "auJzb1D-fag" assert result == json.loads(cache_file.read_text(encoding="utf-8")) + def test_null_content_is_a_payload_not_a_miss(self, tmp_cache): + """A stored JSON ``null`` parses to ``None`` but is *not* a cache miss. + + Returning a bare ``None`` for absence would conflate the two and turn + such an entry into a 404. The sentinel keeps them distinguishable. + """ + tmp_cache.mkdir(parents=True, exist_ok=True) + cache_file = tmp_cache / "nullish_processed.json" + cache_file.write_text("null", encoding="utf-8") + + result = _read_video_analysis_sync(cache_file) + + assert result is None + assert result is not _CACHE_MISS + + def test_falsy_payloads_are_not_misses(self, tmp_cache): + """Neither is any other falsy JSON value the helper can legally parse.""" + tmp_cache.mkdir(parents=True, exist_ok=True) + + for index, raw in enumerate(("{}", "[]", '""', "0", "false")): + cache_file = tmp_cache / f"falsy{index}_processed.json" + cache_file.write_text(raw, encoding="utf-8") + + assert _read_video_analysis_sync(cache_file) is not _CACHE_MISS, raw + + def test_embedded_null_byte_in_path_is_a_miss(self, tmp_cache): + """A path the OS cannot even name is a miss, not a fault. + + ``Path.exists()`` swallows the ``ValueError`` that an embedded null + byte provokes and reports the entry as absent, so the pre-change + handler answered 404. A bare ``open()`` lets that ``ValueError`` + escape, which would turn the same request into a 500. + """ + tmp_cache.mkdir(parents=True, exist_ok=True) + poisoned = Path(f"{tmp_cache}/\x00_processed.json") + + assert _read_video_analysis_sync(poisoned) is _CACHE_MISS + def test_corrupt_entry_raises_rather_than_reporting_a_miss(self, tmp_cache): """A damaged entry must surface as a 500, never as a 404. - Only ``FileNotFoundError`` means "no such analysis"; anything else is a - real fault and has to keep propagating. + Only "the path names no readable entry" -- ``FileNotFoundError`` or a + path the OS rejects outright -- means "no such analysis"; anything + else is a real fault and has to keep propagating. """ tmp_cache.mkdir(parents=True, exist_ok=True) cache_file = tmp_cache / "bad_processed.json" @@ -1288,3 +1330,87 @@ def test_endpoint_serves_entry_older_than_processor_ttl( assert response.status_code == 200 assert response.json()["video_id"] == "auJzb1D-fag" + + +# =========================================================================== +# Cache-miss sentinel: absence vs. a stored ``null`` +# =========================================================================== + + +class TestVideoDetailDistinguishesNullFromMissing: + """A stored JSON ``null`` is a payload; only absence is a 404. + + The helper runs in a worker thread and hands its result back to the + handler, so the value it uses to signal "no entry" must be one that + ``json.load`` can never itself produce. ``None`` fails that test, and + using it regressed a ``null`` entry from 200 to 404. + """ + + def test_null_content_entry_is_served_as_200( + self, client, mock_processor, tmp_cache + ): + tmp_cache.mkdir(parents=True, exist_ok=True) + cache_file = tmp_cache / "auJzb1D-fag_processed.json" + cache_file.write_text("null", encoding="utf-8") + mock_processor._get_cache_path.return_value = cache_file + + response = client.get("/api/v2/videos/auJzb1D-fag") + + assert response.status_code == 200 + assert response.json() is None + + def test_absent_entry_is_still_a_404(self, client, mock_processor, tmp_cache): + """The control: the sentinel must not swallow genuine misses.""" + tmp_cache.mkdir(parents=True, exist_ok=True) + mock_processor._get_cache_path.return_value = ( + tmp_cache / "auJzb1D-fag_processed.json" + ) + + response = client.get("/api/v2/videos/auJzb1D-fag") + + assert response.status_code == 404 + + +# ============================================================================ +# Malformed identifiers stay on the 404 path +# ============================================================================ + + +class TestVideoDetailRejectsMalformedIdentifiers: + """A ``video_id`` the filesystem cannot name is a 404, not a 500. + + Dropping the ``Path.exists()`` probe removed an implicit guard: that call + catches ``ValueError`` as well as ``OSError``, so an identifier carrying an + embedded null byte was reported as absent. A bare ``open()`` raises + instead, which escalated the same request from 404 to 500. + """ + + def test_null_byte_identifier_is_a_404_not_a_500( + self, client, mock_processor, tmp_cache + ): + tmp_cache.mkdir(parents=True, exist_ok=True) + # Real path-building semantics, mirroring ``_get_cache_path``: a fixed + # return value would never carry the null byte into the open() call. + mock_processor._get_cache_path.side_effect = lambda vid: Path( + f"{tmp_cache}/{vid}_processed.json" + ) + + response = client.get("/api/v2/videos/%00") + + assert response.status_code == 404 + assert "not found" in response.json()["detail"].lower() + + def test_wellformed_identifier_still_reaches_the_payload( + self, client, mock_processor, tmp_cache + ): + """The control: the widened miss rule must not swallow real reads.""" + tmp_cache.mkdir(parents=True, exist_ok=True) + _write_cache_file(tmp_cache, "auJzb1D-fag") + mock_processor._get_cache_path.side_effect = lambda vid: Path( + f"{tmp_cache}/{vid}_processed.json" + ) + + response = client.get("/api/v2/videos/auJzb1D-fag") + + assert response.status_code == 200 + assert response.json()["video_id"] == "auJzb1D-fag" From beaf7a06850eced55e8fcd0806cf18a698653060 Mon Sep 17 00:00:00 2001 From: Hayden <154503486+groupthinking@users.noreply.github.com> Date: Mon, 3 Aug 2026 20:33:22 -0500 Subject: [PATCH 3/4] docs: narrow the offload claim to filesystem latency json.load holds the GIL, so the to_thread hop relocates the parse stall rather than removing it. Narrow the documented guarantee accordingly and add a characterisation test so the weaker claim stays honest. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../backend/real_api_endpoints.py | 22 ++++++ tests/unit/test_real_api_endpoints.py | 67 ++++++++++++++++++- 2 files changed, 88 insertions(+), 1 deletion(-) diff --git a/src/youtube_extension/backend/real_api_endpoints.py b/src/youtube_extension/backend/real_api_endpoints.py index 141c9881b..98aa44f36 100644 --- a/src/youtube_extension/backend/real_api_endpoints.py +++ b/src/youtube_extension/backend/real_api_endpoints.py @@ -98,6 +98,28 @@ def _read_video_analysis_sync(cache_path: Path) -> Any: executed in a worker thread via :func:`asyncio.to_thread` rather than directly on the event loop. + What that offload does and does not buy is worth stating precisely, because + the two halves of this function behave differently under the GIL: + + * ``open()``/``read()`` release the GIL, so moving them off the loop removes + the caller's exposure to filesystem latency entirely. This is the part + that is unbounded -- a cold page cache, a networked mount or a contended + disk can stall for hundreds of milliseconds. + * ``json.load()`` is CPU-bound C code that *holds* the GIL for its whole + duration. Running it in a worker thread does not stop it blocking the + loop; it only relocates it. Measured stall tracks payload size at roughly + 3 ms/MB (~0.4 ms for a typical one-hour transcript, ~3 ms for long-form). + + So this converts an unbounded, environment-dependent stall into a bounded, + payload-proportional one. It does not make the read non-blocking. On a warm + page cache with a small payload the executor hop is measurably *worse* than + reading inline (~0.5 ms of dispatch overhead against a ~0.4 ms parse); the + change earns its keep when the filesystem is slow, which is precisely the + case that cannot be predicted from inside the handler. + + Removing the residual parse stall needs a different fix (streaming/incremental + parse, a size cap, or a process pool) and is tracked separately. + Opening directly and treating :class:`FileNotFoundError` as the miss replaces a separate ``Path.exists()`` probe. That is one syscall instead of two, and it closes the window in which the entry could be removed between diff --git a/tests/unit/test_real_api_endpoints.py b/tests/unit/test_real_api_endpoints.py index cbeac4678..efc2918b8 100644 --- a/tests/unit/test_real_api_endpoints.py +++ b/tests/unit/test_real_api_endpoints.py @@ -1146,7 +1146,16 @@ def _record_loop_thread(): async def test_event_loop_stays_responsive_during_cache_read( self, api_app, mock_processor, mock_youtube, mock_cost_monitor, tmp_cache ): - """A slow read must not starve other tasks on the loop.""" + """A slow read must not starve other tasks on the loop. + + Scope note: ``_SlowReadPath`` sleeps in ``__fspath__``, which models + *filesystem* latency -- and ``time.sleep`` releases the GIL, exactly as + a real blocking syscall does. So this test covers the I/O half of the + read only. It is not vacuous: reverting the ``asyncio.to_thread`` hop + drives ``heartbeats`` to 0. But it deliberately says nothing about the + ``json.load()`` half, which holds the GIL and still stalls the loop -- + see ``test_parse_still_stalls_the_loop_in_proportion_to_payload``. + """ tmp_cache.mkdir(parents=True, exist_ok=True) cache_file = _write_cache_file(tmp_cache, "auJzb1D-fag") @@ -1197,6 +1206,62 @@ async def _heartbeat(): heartbeats >= 5 ), f"event loop was starved during the cache read (ticks={heartbeats})" + async def test_parse_still_stalls_the_loop_in_proportion_to_payload( + self, tmp_path + ): + """Document the residual: ``json.load`` blocks the loop even off-thread. + + ``open()``/``read()`` release the GIL, so the ``asyncio.to_thread`` hop + genuinely removes filesystem latency from the loop. ``json.load()`` does + not -- it is CPU-bound C code that holds the GIL for its full duration, + so relocating it to a worker thread does not stop it blocking. + + This is a *characterisation* test. It exists so the weaker guarantee + stays honest: if someone later claims this endpoint's read is fully + non-blocking, this test is the counter-example. Asserted as a ratio + between a small and a large payload so it does not depend on the + absolute speed of the runner. + """ + small = tmp_path / "small.json" + large = tmp_path / "large.json" + small.write_text(json.dumps({"transcript": [{"t": i} for i in range(200)]})) + large.write_text( + json.dumps({"transcript": [{"t": i} for i in range(400_000)]}) + ) + + async def _max_loop_gap(path): + gaps: list[float] = [] + stop = asyncio.Event() + + async def _ticker(): + last = time.perf_counter() + while not stop.is_set(): + await asyncio.sleep(0) + now = time.perf_counter() + gaps.append(now - last) + last = now + + task = asyncio.create_task(_ticker()) + await asyncio.sleep(0.02) + gaps.clear() + await asyncio.to_thread(_read_video_analysis_sync, path) + stop.set() + await task + return max(gaps) + + small_gap = await _max_loop_gap(small) + large_gap = await _max_loop_gap(large) + + # The large payload is ~2000x the small one. Even allowing for executor + # dispatch overhead dominating the small case, the parse must show up as + # a materially longer stall -- that is the point being documented. + assert large_gap > small_gap * 5, ( + "expected json.load to stall the loop in proportion to payload size " + f"(small={small_gap * 1000:.2f}ms, large={large_gap * 1000:.2f}ms); " + "if this now passes trivially, the parse may have been made " + "incremental -- update the endpoint's documented guarantee" + ) + def test_offloaded_read_returns_same_payload( self, client, mock_processor, tmp_cache ): From 8601178ad2df1e282688dbb5e2d0b530ccad02ff Mon Sep 17 00:00:00 2001 From: Hayden <154503486+groupthinking@users.noreply.github.com> Date: Mon, 3 Aug 2026 20:35:10 -0500 Subject: [PATCH 4/4] docs: link residual parse stall to issue #1306 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- src/youtube_extension/backend/real_api_endpoints.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/youtube_extension/backend/real_api_endpoints.py b/src/youtube_extension/backend/real_api_endpoints.py index 98aa44f36..be7ae7868 100644 --- a/src/youtube_extension/backend/real_api_endpoints.py +++ b/src/youtube_extension/backend/real_api_endpoints.py @@ -118,7 +118,7 @@ def _read_video_analysis_sync(cache_path: Path) -> Any: case that cannot be predicted from inside the handler. Removing the residual parse stall needs a different fix (streaming/incremental - parse, a size cap, or a process pool) and is tracked separately. + parse, a size cap, or a process pool) and is tracked in issue #1306. Opening directly and treating :class:`FileNotFoundError` as the miss replaces a separate ``Path.exists()`` probe. That is one syscall instead of