Skip to content

perf: /api/v1/learning-log blocks the event loop on an unbounded uncached filesystem walk #1386

Description

@groupthinking

Problem

GET /api/v1/learning-log is declared async but performs an unbounded,
uncached filesystem walk directly on the event loop.

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

async def get_learning_log_v1(data_service: DataService = Depends(get_data_service)):
    """Get learning log from enhanced analysis files"""
    try:
        learning_log = data_service.get_learning_log()   # <- blocking, not awaited
        return learning_log

DataService.get_learning_log() (services/data_service.py:54-134) then does,
synchronously:

  1. self.enhanced_analysis_dir.rglob("*_enhanced.md") — a fresh recursive walk
    of the whole tree
    (L71).
  2. For every file found:
    • parent_dir.glob(f"{video_id}_*_metadata.json") (L79)
    • metadata_file.exists() (L83)
    • open(...) + json.load(...) (L85-86)
    • md_file.stat() (L93)
  3. An in-memory sort of the full result set (L127).

Because FastAPI runs async def handlers on the event loop itself, every one of
those syscalls holds the loop. No other request on the worker — including
health checks and unrelated endpoints — can make progress while the walk runs.

Why this one is worse than the sibling defects

Two properties make this the most severe instance of this pattern in the router:

  • It is completely uncached. DataService already has
    _get_all_files_cached() (L136-160) with a 60s TTL, and the videos path uses
    it. get_learning_log() ignores it and issues its own rglob on every
    single request
    .
  • It is unbounded. There is no limit/offset. The cost scales linearly
    with the total number of processed videos forever, whereas the videos endpoint
    is at least capped at a page.

So the blocking cost is roughly 1 tree walk + 4 syscalls x N files + N JSON parses, with N growing without limit.

Why the existing tests do not catch it

tests/unit/test_v1_router_extended.py:191 installs
svc.get_learning_log.return_value = [...] on a MagicMock. The mock returns
instantly, so the handler never blocks under test and the suite stays green while
production stalls. tests/unit/test_data_service.py exercises the real method
but only synchronously — nothing asserts anything about the event loop.

This is the same blind spot that #1379 had, and it needs the same style of
regression test: one that asserts the work is dispatched to a worker thread, and
that the loop stays responsive while it is in flight.

Proposed fix

Offload the call with asyncio.to_thread, exactly as #1379 did for
/api/v1/videos:

learning_log = await asyncio.to_thread(data_service.get_learning_log)

asyncio is already imported in the router. The change is a single line plus
regression tests.

Deliberately out of scope (each is a separate, larger change):

  • Routing get_learning_log() through _get_all_files_cached().
  • Adding pagination to the endpoint.
  • Caching the parsed metadata.

Those are real follow-ups, but this issue is only about getting the blocking work
off the event loop, which is the part that degrades every other request.

Acceptance criteria

  • The scan runs on a worker thread, not the event loop.
  • The event loop stays responsive while the scan is in flight.
  • The 500-on-error contract of the endpoint is unchanged.
  • Regression tests fail if the offload is reverted, and fail if the helper is
    called inline rather than offloaded.

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