Skip to content

perf: offload the video-detail lookup off the event loop - #1392

Merged
groupthinking merged 2 commits into
mainfrom
perf/video-detail-offload-1389
Aug 5, 2026
Merged

perf: offload the video-detail lookup off the event loop#1392
groupthinking merged 2 commits into
mainfrom
perf/video-detail-offload-1389

Conversation

@groupthinking

Copy link
Copy Markdown
Owner

Canonical issue

Closes #1389

GET /api/v1/videos/{video_id} calls data_service.get_video_detail(video_id) directly on the event loop. That helper is not a cheap lookup — it is a recursive filesystem walk plus two file reads:

Step data_service.py Cost
rglob("*") over the enhanced-analysis tree L281-283 grows with corpus size; uncached
.stat() on every match L292 one syscall per file
glob for the metadata sibling L297 directory scan
.exists() probe L300 syscall
json.load of the metadata file L302-303 blocking read + parse
full markdown body read L310-312 blocking read, unbounded size

None of this is behind the _get_all_files_cached() TTL cache (_file_cache_ttl = 60, L48) that other call sites use. Every request re-walks the tree. While that runs, the event loop is stalled for all clients in the process, not just the caller.

Outcome

The lookup is dispatched to a worker thread with asyncio.to_thread, so the event loop stays free to service other requests while the walk and the reads proceed.

The judgement call worth reviewing — one shared budget, not one gate per endpoint.

asyncio.to_thread hands work to the loop's default ThreadPoolExecutor, sized min(32, (os.cpu_count() or 1) + 4) — as few as 5 workers. The learning-log endpoint (merged in #1388) already gates its walk at 4.

The obvious move was to give this endpoint its own gate of 4. That is locally correct and globally wrong: two independent gates each reasoning about a shared resource could between them occupy all 5 workers and starve the executor — exactly the failure the gate was introduced to prevent. Anything else needing a thread would queue behind them.

So this PR generalises the existing learning-log gate into a single filesystem-walk budget that both endpoints draw from:

  • _LEARNING_LOG_MAX_CONCURRENCY_FS_WALK_MAX_CONCURRENCY (still 4)
  • _get_learning_log_gate()_get_fs_walk_gate()
  • the gate block moves above both call sites

With one shared budget of 4 against a floor of 5 workers, at least one worker is always free.

The trade-off, stated plainly: the two endpoints now head-of-line block each other. Four in-flight learning-log walks will make a video-detail request wait. I consider that acceptable because both endpoints do the same class of work (recursive walk + read) over the same tree, so a shared budget matches the shared cost. If a future change needs per-endpoint fairness, the correct fix is a shared total budget with per-endpoint sub-limits — not two independent gates, which would reintroduce the starvation. That reasoning is now enforced by a test, not left to a comment.

The rename is behaviour-neutral: the whole test file was 133 passed before and 133 passed after the rename, with no other change applied.

Risk

  • Risk level: low
  • Blast radius: one endpoint's dispatch mechanism, plus the rename of a private module-level gate used by one other endpoint in the same file. No public surface, no schema, no serialisation, no config change. data_service.get_video_detail is untouched.
  • Failure mode: if the shared budget is too tight under real traffic, video-detail latency rises under concurrent learning-log load (queueing, not failure). Responses stay correct; the 404 and 500 contracts are unchanged and separately tested.
  • Why the gate is per-loop and not module-level: an asyncio.Semaphore created at import time binds to whichever loop first contends on it, and then raises at runtime from any other loop. This is subtle — Semaphore.acquire returns on the fast path before _get_loop() is ever reached:
    if not self.locked():
        self._value -= 1
        return True          # returns before _get_loop() is called
    fut = self._get_loop().create_future()   # only the waiting path binds
    So a module-level semaphore is a latent landmine, not an obvious bug: it passes every low-concurrency test and fails only under the burst it exists to absorb. The gate is therefore keyed per running loop in a weakref.WeakKeyDictionary, guarded by a plain threading.Lock (not an asyncio.Lock, which would itself be loop-bound).
  • Rollback: revert the commit. The change is self-contained in one router file plus its test file.

Verification

Negative-control ladder. Every rung was applied to the merged source with a guarded replacement (assert s.count(old) == 1), run, then restored from a byte-exact backup and confirmed with diff -q.

# Mutation Result Reading
NC-1 git checkout origin/main -- router.py (revert the whole fix) 5 failed, 3 passed The 3 survivors are the contract tests — argument forwarding, 404, 500 — which the offload genuinely does not change. A total wipe would have suggested the tests were coupled to the diff rather than to behaviour.
NC-2 Drop async with _get_fs_walk_gate():, keep the offload 2 failed, 6 passed Fails exactly the cap test and the shared-budget test. The six offload tests correctly survive, because the offload is still intact.
NC-3 Give video-detail its own correctly-written per-loop gate of 4 1 failed, 7 passed The important one. A complete, plausible, locally-correct implementation. It fails only test_budget_is_shared_with_the_learning_log_walk, with combined peak of 8 across both walk endpoints with a shared cap of 4 — i.e. assert 8 == 4, precisely 2 × limit.

NC-3 is the control that matters: without that one test, the globally-wrong implementation passes everything else, including its own per-endpoint cap test. Two earlier attempts at NC-3 were discarded rather than reported — the first failed its assert s.count(old) == 1 guard (wrong indentation) and never mutated the file; the second raised NameError at collection. Neither is a valid control, since a collection error demonstrates nothing about test discrimination.

Tests added — TestVideoDetailOffloading, 8 tests:

Test What would break without it
test_lookup_runs_on_a_worker_thread thread identity, with an anti-vacuity assertion so it cannot pass trivially
test_video_id_is_forwarded_to_the_service the offload silently dropping the path parameter
test_event_loop_stays_responsive_while_lookup_is_in_flight timing-relative: counts only ticks recorded before the walk finishes
test_lookup_uses_exactly_one_to_thread_hop a double dispatch
test_missing_video_still_returns_404 the except HTTPException: raise re-raise being swallowed into a 500
test_error_contract_is_unchanged the 500 path
test_concurrent_lookups_are_capped_by_the_gate asserts peak == limit, not <=, so an absent or oversized cap fails
test_budget_is_shared_with_the_learning_log_walk the design decision above — two gates would give peak == 2 * limit

On the responsiveness test: the inherited pattern in this file asserts a raw tick count while the mock blocks, which passes even against a fully inline implementation. This test instead records time.monotonic() per tick and compares against the walk's finish time, so it fails if the work is not actually offloaded. (The pre-existing weakness in TestListVideosOffloading is tracked separately in #1390 and deliberately not touched here.)

Runs:

Scope Result
TestVideoDetailOffloading 8 passed
tests/unit/test_v1_router_extended.py (whole file) 141 passed — 133 before + 8 new, matching the count predicted before the run
Rename in isolation, no other change 133 passed before, 133 after

Lint parity — absolute counts are meaningless here, so this is a stash contrast over the same two paths:

ruff check findings ruff format --diff lines
baseline (stashed) 26 189
this branch 26 189

Identical rule-and-file sets (25 router, 1 test) on both sides. This branch introduces zero new findings and zero format drift; the 26 are pre-existing and deliberately not "fixed" here.

  • Focused tests
  • Required CI
  • Review threads resolved

Production evidence

Not applicable — no production surface changes.

This PR changes only where an existing synchronous call runs: on a worker thread instead of on the event loop. The route, its path parameter, the response body, and both error contracts are byte-identical, and data_service.get_video_detail itself is untouched. There is no migration, no feature flag, no configuration change, and nothing to observe in a production dashboard beyond reduced event-loop stalling under concurrent load.

Two deployment facts make the per-loop gate the right unit here. The Dockerfile runs Uvicorn without --workers, so there is one event loop per process and a per-loop semaphore is effectively a per-process limit. And no custom default-executor is configured anywhere in the repo, so the min(32, cpu_count + 4) sizing above is the real bound in production, not a theoretical one. The default rate limit is 60/minute per client and explicitly permits bursts, with no endpoint-specific override — so a burst of concurrent walks is a reachable state, not a hypothetical.

`GET /api/v1/videos/{video_id}` called `data_service.get_video_detail`
directly on the event loop. That helper runs an uncached recursive
`rglob` over the enhanced-analysis tree, stats every match, then opens
and reads a metadata file and the full markdown body. All of it is
blocking I/O whose cost grows with the corpus, so a single request
stalled every other request sharing the loop.

Dispatch the lookup to a worker thread with `asyncio.to_thread`.

The thread pool is a shared, and small, resource: `asyncio.to_thread`
uses the loop's default `ThreadPoolExecutor`, sized
`min(32, (os.cpu_count() or 1) + 4)`, so as few as 5 workers. The
learning-log endpoint already gates its walk at 4. Giving this endpoint
its own gate of 4 would be locally correct but globally wrong, because
the two gates could between them occupy all 5 workers and starve the
executor. So the existing gate is generalised into one shared
filesystem-walk budget that both endpoints draw from, which always
leaves at least one worker free. The trade-off is head-of-line blocking
between the two endpoints, which is acceptable because they do the same
class of work over the same tree.

Adds `TestVideoDetailOffloading` (8 tests) covering thread identity,
argument forwarding, timing-relative loop responsiveness, hop count,
the 404 and 500 contracts, the concurrency cap, and the shared budget.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@vercel

vercel Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
v0-uvai Ready Ready Preview, v0 Aug 5, 2026 3:20am

@coderabbitai

coderabbitai Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Important

Review skipped

Auto reviews are limited based on label configuration.

🏷️ Required labels (at least one) (1)
  • [‘architecture-gap’, ‘bug’, ‘ci-cd’, ‘ci/cd’, ‘copilot-rabbit’, ‘documentation’, ‘duplicate’, ‘enhancement’, ‘frontend’, ‘github_actions’, ‘good first issue’, ‘help wanted’, ‘high-priority’, ‘invalid’, ‘javascript’, ‘ml-model’, ‘needs-triage’, ‘pipeline-critical’, ‘placeholder-code’, ‘priority:high’, ‘python’, ‘python:uv’, ‘question’, ‘styling’, ‘tests’, ‘v0’]

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository YAML (base), Repository UI (inherited), Organization UI (inherited)

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: cae220f5-0af4-4834-9b17-5cd60f3d09f9

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
📝 Walkthrough

Summary by CodeRabbit

  • Performance Improvements
    • Improved responsiveness when retrieving video details and learning logs.
    • Limited concurrent filesystem operations to prevent resource contention during simultaneous requests.

Walkthrough

The router adds a shared per-event-loop semaphore that limits filesystem walks to four concurrent operations. Video detail retrieval moves to a worker thread, and learning-log retrieval uses the shared semaphore.

Changes

Filesystem concurrency control

Layer / File(s) Summary
Shared filesystem-walk gate
src/youtube_extension/backend/api/v1/router.py
A weakly held, per-event-loop semaphore limits filesystem-walk operations to four concurrent tasks.
Endpoint filesystem integration
src/youtube_extension/backend/api/v1/router.py
Video detail retrieval uses asyncio.to_thread under the shared gate. Learning-log retrieval uses the same gate.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant VideoDetailEndpoint
  participant FilesystemGate
  participant WorkerThread
  participant DataService

  Client->>VideoDetailEndpoint: request video details
  VideoDetailEndpoint->>FilesystemGate: acquire semaphore
  FilesystemGate->>WorkerThread: run filesystem work
  WorkerThread->>DataService: get_video_detail(video_id)
  DataService-->>WorkerThread: return video detail
  WorkerThread-->>VideoDetailEndpoint: return result
  VideoDetailEndpoint-->>Client: response
Loading

Possibly related issues

Possibly related PRs

Suggested labels: copilot-rabbit

Suggested reviewers: claude, copilot

Poem

Four walks pass through the gate,
While worker threads keep loops awake.
Video details safely stream,
Learning logs share the same scheme.
The event loop moves, not waits.

🚥 Pre-merge checks | ✅ 3 | ❌ 4

❌ Failed checks (4 inconclusive)

Check name Status Explanation Resolution
Linked Issues check ❓ Inconclusive The router summary matches #1389, but required regression tests cannot be verified because tests/unit/test_v1_router_extended.py is excluded by the !tests/** filter. Review the excluded test file and confirm the worker-thread, responsiveness, single-hop, error-contract, and shared-budget acceptance tests pass.
Out of Scope Changes check ❓ Inconclusive The reviewed router change is within #1389, but related test-file changes cannot be checked because tests/unit/test_v1_router_extended.py is excluded by the !tests/** filter. Review the excluded test file to confirm it contains only tests for video-detail offloading, error preservation, concurrency limits, and shared budgeting.
Enforce Copilot Verification ❓ Inconclusive Evidence collection has not started; no approval decision is recorded yet. Check the pull request review records for an explicit GitHub Copilot approval.
Require Ai Unit Tests ❓ Inconclusive I am checking the committed tests and pull-request metadata before deciding whether the required label and AI-generated tests are present. Need repository and pull-request metadata evidence.
✅ Passed checks (3 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Title check ✅ Passed The title clearly identifies the primary change: moving the video-detail lookup off the event loop.
Description check ✅ Passed The description thoroughly covers the issue, outcome, risks, verification, production impact, and rollback for the implemented change.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch perf/video-detail-offload-1389
✨ Simplify code
  • Create PR with simplified code
  • Commit simplified code in branch perf/video-detail-offload-1389

Warning

Review ran into problems

🔥 Problems

These MCP integrations need to be re-authenticated in the Integrations settings: Sentry


Linked repositories: Public OSS repositories can only analyze public repositories installed in this organization. No linked repositories were analyzed; skipped groupthinking/uvai-skills.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions github-actions Bot added the python label Aug 5, 2026
@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

Snapshot Warnings

⚠️: No snapshots were found for the head SHA 8e3beae.
Ensure that dependencies are being submitted on PR branches and consider enabling retry-on-snapshot-warnings. See the documentation for more information and troubleshooting advice.

Scanned Files

None

@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown

Agent Completion Truth Gate: NOT_APPLICABLE

Evidence agrees.

Machine-readable verdict
{
  "details": {},
  "reasons": [],
  "verdict": "not_applicable"
}

Workflow evidence

@groupthinking

Copy link
Copy Markdown
Owner Author

@linear-code @coderabbitai review

Context, and the specific judgement call I would most like a second opinion on.

A note on the canonical issue. #1389 was auto-closed as duplicate shortly after opening, and I have reopened it. The overlap is with #1304, "offload video-detail cache read, drop the stat probe" — but that PR changed get_cached_video_v1, which serves /cache/{video_id}. This PR changes get_video_detail_v1, which serves /videos/{video_id} and calls a different helper. The defect is still live on main: sed-ing the function body out of origin/main and grepping for to_thread returns only video_detail = data_service.get_video_detail(video_id) — a bare synchronous call. If you still read that as a duplicate, please say so and I will close this rather than argue.

The design decision. asyncio.to_thread uses the loop's default ThreadPoolExecutor, sized min(32, (os.cpu_count() or 1) + 4) — a floor of 5 workers. The learning-log endpoint already gates its walk at 4 (added in #1388).

The obvious implementation was to give this endpoint its own gate of 4. I deliberately did not do that. Two independent gates of 4 each reason locally about a shared resource, and could between them occupy all 5 workers — starving the executor for everything else in the process, which is the exact failure the original gate was introduced to prevent. Instead the existing gate is generalised into one shared filesystem-walk budget both endpoints draw from, so at least one worker is always free.

The cost is real and I want it on the record: the two endpoints now head-of-line block each other. Four in-flight learning-log walks will make a video-detail request wait. I judged that acceptable because both do the same class of work (recursive walk plus file reads) over the same tree, so one budget matches one underlying cost. If per-endpoint fairness is ever needed, I believe the right answer is a shared total budget with per-endpoint sub-limits — not two independent gates. Please push back if you disagree, because this is the kind of trade-off that is much cheaper to change now than after it ships.

Why I trust the test that enforces it. I ran a three-rung negative-control ladder, each mutation applied with a guarded replacement and then restored byte-exact:

# Mutation Result
NC-1 revert the router to origin/main 5 failed, 3 passed
NC-2 drop the async with gate, keep the offload 2 failed, 6 passed
NC-3 give video-detail its own correct per-loop gate 1 failed, 7 passed

NC-3 is the one that earns its keep. It is a complete, plausible, locally-correct implementation — it even passes its own per-endpoint concurrency-cap test. It fails only test_budget_is_shared_with_the_learning_log_walk, with combined peak of 8 across both walk endpoints with a shared cap of 4: assert 8 == 4, exactly 2 × limit. Without that single test, the globally-wrong version ships green. Note also that NC-1 is deliberately partial — if reverting the fix had failed all 8, I would have suspected the tests were coupled to the diff rather than to behaviour.

Two earlier NC-3 attempts are not in that table because they were not valid controls: the first tripped its assert s.count(old) == 1 guard and never mutated the file (it then reported "8 passed", which looks like a result and is not), and the second raised NameError at collection.

Two smaller things worth your attention:

  1. The gate is keyed per running loop, not module-level. asyncio.Semaphore.acquire returns on the fast path before _get_loop() is reached, so a module-level semaphore binds to whichever loop first contends on it. It therefore passes every low-concurrency test and fails only under the burst it exists to absorb. The weakref.WeakKeyDictionary is guarded by a plain threading.Lock, not an asyncio.Lock, since the latter would itself be loop-bound.

  2. The responsiveness test is timing-relative, not a raw tick count. The inherited pattern in this file asserts ticks >= 3 while a mock blocks, which passes even against a fully inline implementation. This one records time.monotonic() per tick and counts only ticks landing before the walk finished. I found that same weakness in the already-merged TestListVideosOffloading and filed test: TestListVideosOffloading responsiveness test passes against an inline implementation #1390 rather than widening this PR's diff.

Deliberately not fixed here: the dead duplicate except Exception block at data_service.py:338-341, the 26 pre-existing ruff findings on these two files (branch and baseline are identical at 26/26 findings and 189/189 format-diff lines), and #1390.

@coderabbitai

coderabbitai Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor
🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo '--- changed files ---'
git diff --name-status origin/main...HEAD

echo '--- diff ---'
git diff --unified=80 origin/main...HEAD

echo '--- candidate source files ---'
fd -t f 'router\.py|test_v1_router_extended\.py|data_service\.py' . | sort

echo '--- outlines ---'
for f in $(fd -t f 'router\.py|test_v1_router_extended\.py' .); do
  echo "### $f"
  ast-grep outline "$f" --items all --type function,class --view expanded || true
done

Length of output: 43003


🏁 Script executed:

#!/bin/bash
set -euo pipefail

router='src/youtube_extension/backend/api/v1/router.py'
service='src/youtube_extension/backend/services/data_service.py'
tests='tests/unit/test_v1_router_extended.py'

echo '--- router imports and shared gate ---'
sed -n '1,135p' "$router"
sed -n '970,1080p' "$router"

echo '--- synchronous helper implementations ---'
rg -n -A80 -B10 '^\s*def (get_video_detail|get_learning_log)\s*\(' "$service"

echo '--- all router asyncio.to_thread call sites ---'
rg -n -A4 -B4 'asyncio\.to_thread\s*\(' "$router"

echo '--- gate tests and module state references ---'
rg -n -A20 -B10 '_fs_walk_gates|_get_fs_walk_gate|_FS_WALK_MAX_CONCURRENCY' "$tests" "$router"

echo '--- default executor configuration ---'
rg -n -i -A3 -B3 'set_default_executor|ThreadPoolExecutor|default.executor' \
  -g '*.py' -g 'Dockerfile' -g 'docker-compose*.yml' .

Length of output: 29025


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo '--- Docker configuration candidates ---'
fd -t f -i '^(dockerfile|docker-compose.*\.(yml|yaml))$' . -x sh -c '
  echo "### $1"
  sed -n "1,240p" "$1"
' sh {}

echo '--- executor configuration candidates ---'
rg -n -i -A3 -B3 'set_default_executor|ThreadPoolExecutor|default[_ -]?executor' \
  -g '*.py' -g 'Dockerfile' -g 'docker-compose*.yml' -g 'docker-compose*.yaml' . || true

echo '--- Uvicorn launch configuration ---'
rg -n -i -A3 -B3 '\buvicorn\b|--workers\b' \
  -g 'Dockerfile' -g '*.py' -g '*.sh' -g '*.yml' -g '*.yaml' . || true

echo '--- current diff summary ---'
git diff --check origin/main...HEAD
git diff --stat origin/main...HEAD

Length of output: 42180


@groupthinking No blocking findings.

Issue #1389 is not a duplicate of #1304. This change affects GET /videos/{video_id} through get_video_detail_v1 and DataService.get_video_detail. #1304 affected the separate cache route.

The shared _get_fs_walk_gate() is the correct design. Both endpoints submit uncached filesystem walks to the same default executor. Separate limits of four could consume all five default workers. One per-loop limit of four preserves one worker for unrelated asyncio.to_thread work.

The head-of-line blocking is an acceptable trade-off here. Both endpoints use the same expensive resource class. If fairness becomes necessary, retain this shared total limit and add per-endpoint limits beneath it.

The Docker launch uses one Uvicorn worker. The repository does not configure a custom default executor. The executor sizing assumption is valid for this deployment.

One non-blocking correction: after contention, asyncio.Semaphore retains its bound loop. Since _fs_walk_gates strongly retains the semaphore value, WeakKeyDictionary does not necessarily allow that loop to be collected. This does not affect the configured single, long-lived production loop. The comment should not claim guaranteed collection if this code later supports repeated loop creation.

git diff --check reports no whitespace errors. I did not execute the test suite because the review environment does not permit repository code execution.

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@groupthinking
groupthinking marked this pull request as ready for review August 5, 2026 03:06
Copilot AI balanced review requested due to automatic review settings August 5, 2026 03:06

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Offloads video-detail filesystem work from the event loop and shares concurrency limits with learning-log processing.

Changes:

  • Adds worker-thread dispatch for video-detail lookup.
  • Generalizes the filesystem-walk concurrency gate.
  • Adds offloading, contract, and concurrency tests.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
src/youtube_extension/backend/api/v1/router.py Adds shared gated thread offloading.
tests/unit/test_v1_router_extended.py Tests offloading and shared concurrency.

Comment on lines +1002 to +1015
# Maps a running event loop -> the ``asyncio.Semaphore`` bound to that loop.
_fs_walk_gates: weakref.WeakKeyDictionary = weakref.WeakKeyDictionary()
_fs_walk_gates_lock = threading.Lock()


def _get_fs_walk_gate() -> asyncio.Semaphore:
"""Return the filesystem-walk concurrency gate bound to the running loop."""
loop = asyncio.get_running_loop()
with _fs_walk_gates_lock:
gate = _fs_walk_gates.get(loop)
if gate is None:
gate = asyncio.Semaphore(_FS_WALK_MAX_CONCURRENCY)
_fs_walk_gates[loop] = gate
return gate
Comment on lines +2921 to +2927
video_id="dQw4w9WgXcQ", data_service=svc
)

result = asyncio.run(_run())

assert result == self.PAYLOAD
svc.get_video_detail.assert_called_once_with("dQw4w9WgXcQ")
coderabbitai[bot]
coderabbitai Bot previously approved these changes Aug 5, 2026

@groupthinking groupthinking left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed on request. Independently verified the diff at 8af194b — the change is correct and the design reasoning holds up.

Verified locally

  • Read the full get_video_detail_v1 body: the offload sits inside the try, the except HTTPException: raise re-raise is intact, and the if not video_detail → 404 check runs after the async with releases the gate. Both error contracts are preserved.
  • Ran tests/unit/test_v1_router_extended.py141 passed (133 pre-existing + 8 new), matching the PR's claim. The 8 TestVideoDetailOffloading tests each carry a real anti-vacuity assertion, and test_budget_is_shared_with_the_learning_log_walk is the one that actually pins the design decision (a per-endpoint gate would give peak == 2 * limit).
  • Confirmed the per-loop-WeakKeyDictionary semaphore rationale: Semaphore.acquire returns on the fast path before _get_loop(), so a module-level semaphore is a latent cross-loop landmine. The per-loop keying is the right fix.

Endorse the shared-budget call. Gating exactly the two uncached rglob endpoints (get_video_detail, get_learning_log) against one budget of 4 — rather than one gate each — is the correct reading of the shared ThreadPoolExecutor as the real resource. I verified in data_service.py that these two are the uncached (rglob, no _get_all_files_cached) walks, so the boundary you drew is principled, not arbitrary.

One non-blocking observation (does not block merge): the stated invariant "at least one worker is always free" is slightly weaker than absolute, because list_videos_v1_collect_videos_page also dispatches a walk via asyncio.to_thread outside this gate. It's normally behind the 60s TTL cache, so on a cache hit it does no walk — which is exactly why leaving it ungated is defensible. But _get_all_files_cached() holds no lock (per its own docstring), so a burst that lands on an expired/cold cache can have several list_videos requests each rebuild the cache concurrently on the executor, none of them drawing on _FS_WALK_MAX_CONCURRENCY. Under that specific window the shared budget isn't the only claimant on the ~5 workers. Not a regression this PR introduces, and arguably out of scope (cache-miss walks are ≤ ~once/60s in the common case) — flagging it only because if the goal is a hard executor bound, folding the cache-refresh path into the same budget (or serialising the refresh) would close the last gap. Fine to track separately.

No changes requested. This is merge-ready from a review standpoint; leaving the actual merge to you.


Generated by Claude Code

Addresses CodeRabbit's non-blocking finding on #1392: the rationale comment
claimed each loop's semaphore "is collected along with the loop it belongs
to". That is only true for a gate that is never contended.

WeakKeyDictionary holds its keys weakly but its *values* strongly, and
asyncio's _LoopBoundMixin._get_loop() stores the loop on the semaphore --
reached only on the waiting path. So once a gate is contended the chain
module global -> data dict -> semaphore -> loop keeps the weak key
reachable and the removal callback never fires.

Measured directly (WeakKeyDictionary[loop] -> Semaphore, entries retained
after the loop is closed and gc.collect() runs):

  uncontended: 0 retained   (sem._loop is None -> True)
  contended:   1 retained   (sem._loop is None -> False)

So the very fast-path asymmetry the comment correctly identifies as the
latent landmine is also what defeats the weak collection it then claims.

Two changes:

- Rewrite the comment to state this accurately rather than overclaim.
- Add _discard_closed_fs_walk_gates() and call it from _get_fs_walk_gate()
  on the gate-creation path only, so the retention is actually bounded and
  not merely documented. Creation is rare (once per loop), so this adds no
  per-request cost. Deletions are applied after the comprehension completes
  because a WeakKeyDictionary must not change size during iteration.

Rejected alternative: storing the gate as an attribute on the loop object,
which would form a self-contained collectable cycle. uvloop.Loop is a C
extension type and setattr would raise AttributeError in production.

Adds test_closed_loops_are_discarded_from_the_gate_registry, which asserts
both halves: that a contended gate really does pin its closed loop
(anti-vacuity -- without it the test would pass against a registry that
never needed pruning), and that a later gate build reclaims it.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@groupthinking

Copy link
Copy Markdown
Owner Author

@coderabbitai — your WeakKeyDictionary finding was correct, and it was correct for a more interesting reason than "the comment overclaims". I've verified it empirically rather than by reading, and pushed a fix in 8e3beae9e.

The claim, and why it was wrong

The comment said each loop's semaphore "is collected along with the loop it belongs to". I measured it directly — a WeakKeyDictionary[loop] -> Semaphore, then close the loop, drop every strong reference, gc.collect(), and count surviving entries:

scenario entries retained after loop death + gc sem._loop is None
gate never contended 0 True
gate contended once 1 False

The mechanism is the same fast-path asymmetry the comment already describes two paragraphs earlier:

  • WeakKeyDictionary.__setitem__ does self.data[ref(key, self._remove)] = value — the key is weak, the value is strong.
  • asyncio.mixins._LoopBoundMixin._get_loop() does self._loop = loop, a strong reference, and is reached only on the waiting path.

So after the first genuine contention the chain module global -> data dict -> semaphore -> loop makes the weak key permanently reachable, the removal callback never fires, and the entry outlives its loop. The comment correctly identified that asymmetry as the latent landmine, then wrote a sentence that the same asymmetry defeats. Half right is the worst kind of comment, because it reads as though it has been thought through.

What changed

  1. The comment now states this accurately — weak keying bounds growth, it is not a guarantee of collection; uncontended gates still fall out by themselves; contended ones need help.
  2. The retention is now actually bounded rather than merely documented. _discard_closed_fs_walk_gates() drops entries whose loop is closed, called from _get_fs_walk_gate() on the creation path only — once per event loop, so no per-request cost. Deletions are materialised into a list first, because a WeakKeyDictionary must not change size while being iterated.

I could have left this as a comment-only fix, since production is one Uvicorn worker and one long-lived loop — a single entry either way. I didn't, because "it doesn't matter in production" is exactly the reasoning that produced the bug you flagged in the first place.

Rejected alternative

Storing the gate as an attribute on the loop object would form a self-contained collectable cycle and need no registry at all. Rejected: uvloop.Loop is a C extension type without __dict__, so setattr raises AttributeError in production while passing every test on the stdlib selector loop. That is the same class of defect as the original module-level-semaphore trap — invisible locally, fatal in the deployment.

Test

test_closed_loops_are_discarded_from_the_gate_registry asserts both halves, because only asserting the second half would pass against a registry that never needed pruning:

  • assert gate._loop is first_loop — proves the contended path actually ran, so the test is about a bound semaphore.
  • assert first_ref() is not Noneanti-vacuity: proves the closed loop really is still pinned before the prune is exercised.
  • assert first_ref() is None after a second loop builds its gate — proves the prune reclaims it.

Negative control, with the _discard_closed_fs_walk_gates() call removed and nothing else changed:

E  AssertionError: a closed loop survived a later gate build;
   _discard_closed_fs_walk_gates did not reclaim it
E  assert <_UnixSelectorEventLoop running=False closed=True debug=False> is None
1 failed, 141 deselected

It fails at the third assertion and not the second — i.e. the retention it is asserting is real, and the test isolates the prune rather than the registry design.

Whole file: 142 passed (was 141). Lint parity against the previous branch commit on the same two paths: 26 findings vs 26, identical set; ruff format --diff 524 lines vs 524.

Thanks for the catch, and for actually running the shell checks on the shared-gate question rather than pattern-matching it — the executor-sizing verification in particular was the part I most wanted a second opinion on.

@coderabbitai

coderabbitai Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

router='src/youtube_extension/backend/api/v1/router.py'
tests='tests/unit/test_v1_router_extended.py'

echo '--- current commit and fix commit ---'
git log --oneline -8
git show --stat --oneline 8e3beae9e

echo '--- router shared gate declarations and implementations ---'
ast-grep outline "$router" --items all --type function --match 'fs_walk|gate' --view expanded || true
rg -n -A70 -B15 \
  '(_FS_WALK_MAX_CONCURRENCY|_fs_walk_gates|_discard_closed_fs_walk_gates|_get_fs_walk_gate)' \
  "$router"

echo '--- regression test ---'
rg -n -A110 -B15 \
  'test_closed_loops_are_discarded_from_the_gate_registry' \
  "$tests"

echo '--- fix commit patch ---'
git show --format=fuller --find-renames --find-copies --unified=60 8e3beae9e -- "$router" "$tests"

echo '--- whitespace validation ---'
git diff --check origin/main...HEAD

Length of output: 11107


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo '--- files changed by the fix commit ---'
git show --format='' --name-only 8e3beae9e

echo '--- locate the regression test ---'
fd -t f -a '.*v1.*router.*extended.*\.py$|.*router.*\.py$' . \
  | while IFS= read -r f; do
      if rg -n -q 'test_closed_loops_are_discarded_from_the_gate_registry' "$f"; then
        echo "### $f"
        rg -n -A110 -B20 \
          'test_closed_loops_are_discarded_from_the_gate_registry' "$f"
      fi
    done

echo '--- precise fix patch ---'
git show --format=fuller --unified=80 8e3beae9e -- \
  src/youtube_extension/backend/api/v1/router.py

git show --format=fuller --unified=100 8e3beae9e -- \
  "$(git show --format='' --name-only 8e3beae9e | rg 'test.*\.py$')"

echo '--- whitespace validation ---'
git diff --check origin/main...HEAD

Length of output: 22018


@groupthinking Confirmed. Commit 8e3beae9e fixes the retention defect.

_discard_closed_fs_walk_gates() runs while _fs_walk_gates_lock is held. It first materializes closed-loop keys, then deletes them. This avoids mutation during WeakKeyDictionary iteration.

The cleanup runs only when _get_fs_walk_gate() creates a gate. It does not add work to normal requests that reuse an existing gate.

The regression test is strong. It forces the semaphore waiting path, verifies that the closed first loop remains pinned before pruning, then verifies that creation on a second loop removes the first entry. The test would fail if the cleanup call were removed.

git diff --check origin/main...HEAD passed with no output.

The comment now describes the actual ownership chain and collection behavior. No further finding.

🐇 ✅


✏️ Learnings added
Learnt from: groupthinking
URL: https://github.com/groupthinking/EventRelay/pull/1392

Timestamp: 2026-08-05T03:20:18.878Z
Learning: In `src/youtube_extension/backend/api/v1/router.py`, `_fs_walk_gates` maps event loops to `asyncio.Semaphore` instances. A contended semaphore binds itself to its event loop and therefore strongly retains the weak dictionary key through the dictionary value. `_discard_closed_fs_walk_gates()` removes closed-loop entries during new-gate creation under `_fs_walk_gates_lock`. The regression test is `TestVideoDetailOffloading.test_closed_loops_are_discarded_from_the_gate_registry` in `tests/unit/test_v1_router_extended.py`.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.


If you found this review helpful, would you consider giving us a shout-out on X?

Thank you for using CodeRabbit!

You are interacting with an AI system.

@groupthinking

Copy link
Copy Markdown
Owner Author

Independent review - red-team of the diff on top of CodeRabbit's review

I re-reviewed this change adversarially after CodeRabbit signed off, on the exact SHA that is pushed (8e3beae9e09a09e0224156adcf32accfe4756468), in a detached worktree rather than my working tree, so nothing local could mask a defect. Everything below is measured output, not assertion.

1. The one non-blocking finding, and why CodeRabbit was right

CodeRabbit's only correction was that the original rationale comment claimed the gate entry would be "collected along with the loop it belongs to". I did not take that on trust in either direction - I wrote a standalone reproduction:

Scenario Entries retained after loop close + gc.collect() sem._loop is None
Semaphore never contended 0 True
Semaphore contended at least once 1 False

The mechanism, confirmed against CPython source:

  • WeakKeyDictionary.__setitem__ does self.data[ref(key, self._remove)] = value. The key is weak; the value is strong.
  • asyncio.mixins._LoopBoundMixin._get_loop() executes self._loop = loop, and it is reached only on the waiting path - Semaphore.acquire returns early via if not self.locked(): self._value -= 1; return True before ever touching _get_loop().
  • So after a single contention event the chain module global -> data dict -> semaphore -> loop makes the weak key permanently reachable and it is never collected.

The bitter irony is that the same lazy-binding asymmetry the comment correctly identified as the reason the gate must be loop-scoped is also precisely what defeats weak collection. The comment was self-inconsistent, and CodeRabbit caught it. This is now the second time in this PR series that a CodeRabbit documentation-accuracy finding was correct on verification (the first was the CPython to_thread doc wording on #1388), so I now treat those findings as presumptively right.

Fix applied in 8e3beae9e: the comment was rewritten to describe the real ownership chain, and _discard_closed_fs_walk_gates() was added. It runs under _fs_walk_gates_lock, materialises the closed-loop key list before deleting (a WeakKeyDictionary must not change size during iteration), and is called only inside the if gate is None: creation branch - so steady-state requests that reuse an existing gate pay nothing.

2. Rejected alternative, and why

The obvious "simpler" fix is to store the gate as an attribute on the loop object itself, which sidesteps the registry entirely. Rejected: production runs uvloop, whose Loop is a C extension type with no __dict__, so setattr would raise AttributeError at runtime - while passing every single test on the stdlib selector loop used in CI. That is the worst possible failure shape: green tests, broken production.

3. Negative-control ladder - four rungs, each failing a strictly smaller set

A regression test that cannot fail is decoration. I mutated the source four ways and confirmed the suite catches each one, with a shrinking blast radius:

# Mutation Result What it proves
NC-1 Restore router.py wholesale from origin/main 5 failed / 3 passed The behaviour tests genuinely bind to the change; the 3 survivors are exactly the contract tests, as intended
NC-2 Delete only the async with from the video-detail path 2 failed / 6 passed Isolates the concurrency cap test and the cross-endpoint shared-budget test
NC-3 Give video-detail its own correctly loop-scoped, correctly locked gate 1 failed / 7 passed - assert 8 == 4 The strongest rung: a locally correct implementation that is globally wrong. Only the shared-budget test dies
NC-4 Remove the _discard_closed_fs_walk_gates() call 1 failed (prune test only) The new registry-pruning test is load-bearing and nothing else depends on it

NC-3 is the one I care about most. It is not a strawman - it is the design a reasonable reviewer would have asked for, and the suite rejects it for the right reason with the right number.

Two of these rungs initially failed to apply cleanly (NC-2 silently matched nothing and printed a deceptive "8 passed"; NC-3's first attempt raised NameError at collection). Both were caught because every mutation is guarded with assert s.count(old) == N and prints an APPLIED-OK token before the run. A control that passes everything is a red flag, not a result. A control that errors at collection is not a result either.

4. Fresh RED -> GREEN proof in a detached worktree

Run against the pushed SHA, not my working tree:

Step Command Result
GREEN, as pushed full test file 142 passed
RED git checkout origin/main -- router.py (tests kept) 10 failed / 132 passed
Restore git checkout HEAD -- router.py git status --short empty
GREEN again full test file 142 passed

The whole-file RED (10) is broader than NC-1's 5 because it additionally catches the four TestLearningLogOffloading tests that reference the renamed shared gate, plus the new prune test. Both numbers are correct for their scope.

5. Neutrality and hygiene

Check Result
Ruff findings, branch vs. committed base, same path set 26 vs 26, diff empty
ruff format --diff lines, both sides 524 vs 524
Staged-diff secret scan clean
PR head SHA vs. local HEAD match
Blocking CI failures zero
Full suite 4 failed, 8288 passed, 18 skipped, 5 xpassed in 738.21s

I predicted the full-suite pass count as 8288 (the 8287 from the previous run plus the one new prune test) before starting it, and that is exactly what it returned. Predicting the number first is the only way to notice a silent collection change.

Lint parity was measured with leading whitespace stripped before sorting, because ruff right-aligns its --> marker by line-number width and a digit-count change otherwise manufactures a phantom diff.

6. What I looked for and did not find

  • A deadlock or lock-ordering hazard. _fs_walk_gates_lock is a plain threading.Lock held only across dictionary reads/writes; no await and no second lock is acquired inside it.
  • Per-request cost from the prune. The call site is inside if gate is None:. A steady-state loop creates its gate once.
  • A new unbounded fan-out. The whole point of the shared gate is that there is exactly one budget of 4 across both offloaded endpoints, so the default ThreadPoolExecutor - as small as 5 workers on a 1-vCPU box, min(32, (os.cpu_count() or 1) + 4) - always retains at least one free worker.
  • Behavioural drift in the response. The offload forwards arguments verbatim and returns the same object; there is an explicit argument-forwarding test.
  • Vacuous assertions in the new tests. The prune test asserts both that the contended path actually ran (gate._loop is first_loop) and that the retention was real before pruning (first_ref() is not None). Under NC-4 both of those still passed and only the final assertion failed - exactly the diagnostic signature I wanted.

7. Accepted cost, stated plainly

One shared budget means head-of-line blocking between endpoints: four concurrent learning-log walks will make a video-detail request wait. That is deliberate. Two independent budgets of 4 could occupy every worker in the pool and starve unrelated to_thread callers. I would rather have bounded cross-endpoint latency than an unbounded starvation mode. CodeRabbit reviewed this trade-off explicitly and called the shared gate "the correct design".

8. Deliberately not fixed here

TestListVideosOffloading (merged earlier in this series) has a weaker responsiveness assertion - it counts ticks without anchoring them to the walk's completion time, so it would not detect an inline implementation. That is tracked separately as #1390 and is intentionally out of scope: bundling an unrelated test hardening into a perf PR would make this diff harder to review and harder to revert.

Four pre-existing failures remain on main (three in tests/test_code_generator.py that require a live Gemini key, one SDK header test that is sensitive to an untracked local .env). They are unrelated to this change and reproduce on a clean checkout of main.

Verdict

The change is bounded, tested against four independent mutations, neutral on lint, verified RED -> GREEN on the pushed SHA in an isolated worktree, and CodeRabbit's single correction has been fixed and re-reviewed to "no further finding". Merging.

@groupthinking
groupthinking merged commit a3ff4da into main Aug 5, 2026
34 checks passed
@groupthinking
groupthinking deleted the perf/video-detail-offload-1389 branch August 5, 2026 03:32
@linear-code

linear-code Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

GRV-325

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

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

2 participants