Skip to content

perf: /api/v1/videos/{video_id} blocks the event loop on an uncached recursive walk and a full file read #1389

Description

@groupthinking

Problem

GET /api/v1/videos/{video_id} is declared async, but the handler calls a fully synchronous, blocking filesystem routine directly on the event loop.

src/youtube_extension/backend/api/v1/router.py:981

async def get_video_detail_v1(
    video_id: str, data_service: DataService = Depends(get_data_service)
):
    """Get detailed info for specific video"""
    try:
        video_detail = data_service.get_video_detail(video_id)   # <-- blocking, not awaited

DataService.get_video_detail() (src/youtube_extension/backend/services/data_service.py:262-337) performs, per request:

Line Operation Cost
273 enhanced_analysis_dir.exists() 1 stat
281-283 rglob(f"{video_id}_*_enhanced.md") full recursive walk of the enhanced-analysis tree
292 md_files.sort(key=lambda x: x.stat().st_mtime) 1 stat per match
297 parent_dir.glob(f"{video_id}_*_metadata.json") directory scan
300 metadata_file.exists() 1 stat
302-303 open() + json.load() blocking read + parse
310-311 open() + f.read() blocking read of the entire markdown body into memory

Every one of those runs on the event loop thread. While a single request is resolving, the process cannot accept a connection, service a health check, or make progress on any other in-flight request.

Why this one matters

  • The walk is unbounded and uncached. rglob at line 281 is its own fresh traversal. It does not go through _get_all_files_cached() (data_service.py:136), so there is no TTL cache in front of it and no upper bound on how many directory entries are visited.
  • rglob cost is independent of how good the match is. The pattern is anchored on video_id, so the result set is tiny — usually one file — but rglob still has to walk the whole tree to establish that. Cost scales with total corpus size, not with the size of the answer.
  • It then reads an entire markdown document into memory (line 310-311) with a blocking read(). These are full enhanced-analysis transcripts, not stubs.
  • This is the detail endpoint. It is the natural target of a click-through from the list view, so it sits directly on an interactive path.

Why the existing tests do not catch it

Every current test either drives the endpoint through TestClient with a MagicMock service, or calls the synchronous DataService method directly:

  • tests/unit/test_v1_router_extended.py:187 sets svc.get_video_detail.return_value; :525 test_get_video_detail_found, :531 test_get_video_detail_not_found, :2265 test_get_video_detail_service_error.
  • tests/unit/test_data_service.py:204-240 and :376-397 exercise the real method synchronously.

A MagicMock returns instantly, so no test can observe whether the call blocked. TestClient runs the app on its own thread, so even a thread-identity assertion written against it would be meaningless. Nothing in the suite asserts which thread the filesystem work happens on, which is exactly why this regressed silently.

Proposed fix

Dispatch the existing synchronous call to a worker thread from the async layer:

video_detail = await asyncio.to_thread(data_service.get_video_detail, video_id)

asyncio is already imported at router.py:10. The change is confined to the endpoint; DataService is not modified.

Deliberately out of scope — each deserves its own issue and its own review:

  • Routing get_video_detail() through _get_all_files_cached() (changes staleness semantics).
  • Streaming the markdown body instead of reading it whole (changes the response contract).
  • Removing the duplicate unreachable except block at data_service.py:339-341.

Acceptance criteria

  • The filesystem work runs on a worker thread, not the event loop thread.
  • A regression test proves the event loop stays responsive while the read is in flight, and fails if the fix is reverted.
  • Exactly one asyncio.to_thread hop, asserted.
  • The 200, 404 and 500 response contracts are unchanged.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions