perf: offload the learning-log filesystem walk off the event loop - #1388
Conversation
`get_learning_log_v1` awaited nothing: it called
`DataService.get_learning_log()` directly, so the whole walk ran inline on
the event loop.
That call is worse than the sibling endpoints already fixed. It issues its
own fresh `rglob("*_enhanced.md")` rather than going through
`_get_all_files_cached()`, so it is completely uncached, and it takes no
`limit`/`offset`, so its cost grows linearly with the total video count
forever. Per entry it also runs a `glob`, an `exists`, an `open` +
`json.load` and a `stat`.
The walk is now dispatched with a single `asyncio.to_thread` hop. The
blocking implementation in `data_service.py` is deliberately untouched;
caching and pagination for it are follow-up work.
Adds `TestLearningLogOffloading` (4 tests) covering thread identity, loop
responsiveness while the walk is in flight, the exact to_thread hop count,
and the unchanged 500 error contract.
Closes #1386
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are limited based on label configuration. 🏷️ Required labels (at least one) (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository YAML (base), Repository UI (inherited), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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. Comment |
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Snapshot WarningsEnsure 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 FilesNone |
Agent Completion Truth Gate: NOT_APPLICABLEEvidence agrees. Machine-readable verdict{
"details": {},
"reasons": [],
"verdict": "not_applicable"
} |
|
@linear-code @coderabbitai review Third in the family of event-loop offload fixes, after #1237 and #1382. One line of source; the rest is the test scaffolding and the evidence for it. Judgement calls I would specifically like challenged: 1. Fixing this in the async layer rather than in 2. A vacuous test I caught mid-review, and whether the same flaw is now merged. My first responsiveness test counted event-loop ticks while the mock blocked on The part I want a second opinion on: 3. One 4. Executor pressure. This is now the third endpoint dispatching to the default Negative controls run and reported in the body: revert (3 failed), double-dispatch (1 failed, only the hop-count test), and yield-then-inline (3 failed). That last one is the mistake this fix pattern actually attracts — an |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== PR diff ==='
git diff --unified=80 origin/main...HEAD -- \
src/youtube_extension/backend/api/v1/router.py \
tests/unit/test_v1_router_extended.py
echo '=== Router outline ==='
ast-grep outline src/youtube_extension/backend/api/v1/router.py --items all --type function --match 'get_learning_log_v1|list_videos' || true
echo '=== Data service outline ==='
ast-grep outline src/youtube_extension/backend/services/data_service.py --items all --type function --match 'get_learning_log|_get_all_files_cached' || true
echo '=== Relevant test classes and concurrency controls ==='
rg -n -C 6 \
'class TestLearningLogOffloading\b|class TestListVideosOffloading\b|to_thread\s*\(|ThreadPoolExecutor|Semaphore|get_learning_log_v1\s*\(' \
src/youtube_extension/backend/api/v1/router.py \
tests/unit/test_v1_router_extended.py \
src/youtube_extension/backend/services/data_service.pyLength of output: 22126 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Locate relevant files ==='
fd -t f -a 'test_v1_router_extended\.py|data_service\.py|pyproject\.toml|requirements.*\.txt|Dockerfile|docker-compose.*\.ya?ml' . \
| sed -n '1,160p'
test_file="$(fd -t f -a '^test_v1_router_extended\.py$' . | head -n 1)"
data_file="$(fd -t f -a '^data_service\.py$' src | head -n 1)"
printf '\n=== Test sections: %s ===\n' "$test_file"
sed -n '2380,2700p' "$test_file"
printf '\n=== DataService learning-log and cache sections: %s ===\n' "$data_file"
sed -n '1,185p' "$data_file"
printf '\n=== Executor and server concurrency configuration ===\n'
rg -n -i -C 4 \
'ThreadPoolExecutor|set_default_executor|run_in_executor|asyncio\.to_thread|Semaphore|limit-concurrency|workers\s*=|uvicorn|gunicorn|max_workers' \
--glob '*.py' --glob 'pyproject.toml' --glob 'requirements*.txt' --glob 'Dockerfile*' --glob 'docker-compose*.y*ml' \
.Length of output: 2773 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Tracked test paths in HEAD and origin/main ==='
git ls-tree -r --name-only HEAD | rg -i '(^|/)(test_.*v1.*router.*|.*router.*test.*)\.py$|test_v1_router_extended\.py$' || true
git ls-tree -r --name-only origin/main | rg -i '(^|/)(test_.*v1.*router.*|.*router.*test.*)\.py$|test_v1_router_extended\.py$' || true
echo '=== Working-tree test candidates ==='
fd -t f -a -i 'router.*\.py|.*router.*' . | rg '/(test|tests)/|test_' || true
echo '=== DataService learning-log and cache implementation ==='
sed -n '1,185p' src/youtube_extension/backend/services/data_service.py
echo '=== Python requirement and deployment/server configuration ==='
printf '\n--- pyproject.toml ---\n'
sed -n '1,240p' pyproject.toml
printf '\n--- root Dockerfile ---\n'
sed -n '1,220p' Dockerfile
printf '\n--- production Dockerfile ---\n'
sed -n '1,220p' infrastructure/docker/Dockerfile.production
echo '=== Executor and request-concurrency controls ==='
rg -n -i -C 3 \
'ThreadPoolExecutor|set_default_executor|run_in_executor|Semaphore|limit-concurrency|max_workers|uvicorn|gunicorn' \
--glob '*.py' --glob 'pyproject.toml' --glob 'Dockerfile*' --glob 'docker-compose*.y*ml' \
. || trueLength of output: 97358 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Learning-log and videos offloading tests from tracked HEAD ==='
git show HEAD:tests/unit/test_v1_router_extended.py | sed -n '2380,2700p'
echo '=== Existing rate-limit declarations for the learning-log route ==='
git show HEAD:src/youtube_extension/backend/api/v1/router.py | \
rg -n -C 5 'learning-log|@.*limit|Limiter|router\s*=' || true
echo '=== Application limiter setup and router inclusion ==='
rg -n -C 5 \
'Limiter\s*\(|limiter\s*=|state\.limiter|_rate_limit_exceeded_handler|include_router' \
src/youtube_extension/main.py \
src/youtube_extension/backend/main.py \
src/youtube_extension/backend/api/v1/router.py \
src/youtube_extension/backend/config/production_config.py || true
echo '=== Current route diff metadata ==='
git diff --check origin/main...HEAD
git status --shortLength of output: 18439
Blocking: bound concurrent learning-log walks before merge.
Each walk uses the shared default executor. The executor also serves other Add a process-wide, event-loop-safe semaphore around this endpoint’s My assessment of the requested decisions:
I found no whitespace errors in the PR diff. You are interacting with an AI system. |
There was a problem hiding this comment.
Pull request overview
Offloads the learning-log filesystem scan to a worker thread, preventing it from blocking FastAPI’s event loop.
Changes:
- Runs
get_learning_log()throughasyncio.to_thread. - Adds regression tests for thread offloading, loop responsiveness, hop count, and error handling.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/youtube_extension/backend/api/v1/router.py |
Offloads blocking learning-log work. |
tests/unit/test_v1_router_extended.py |
Adds focused concurrency and contract tests. |
groupthinking
left a comment
groupthinking
left a comment
There was a problem hiding this comment.
Review (comment — GitHub blocks self-approval on this account)
Reviewed the full diff at head 82dbcce. Focused, correct, and well-tested; no blocking findings — LGTM.
Change
get_learning_log_v1now dispatches the blocking walk viaawait asyncio.to_thread(data_service.get_learning_log)instead of calling it inline.get_learning_logissues its own uncachedrglobplus a metadata read/stat per entry, so this is unbounded blocking I/O that previously pinned the event loop for the whole process. Moving it to a worker thread is the right fix and matches the already-landed offloads in #1237 and #1382.asynciois already imported/used in this module (the_collect_videos_pagehop), so no new import; the source change is one line plus an explanatory comment.
Correctness
- Error contract unchanged: an exception in the worker propagates through
awaitinto the existingexcept Exception→ 500.test_error_contract_is_unchangedpins this. - Payload unchanged: same object from the same function, just returned off-thread.
- No new shared mutable state, so no data race introduced.
Tests — genuinely non-vacuous:
- Every test asserts the real payload before asserting thread identity / hop count, so a change that skipped the call couldn't pass trivially.
test_event_loop_stays_responsive_while_walk_is_in_flightcompares tick timestamps against actual walk-completion rather than merely counting ticks — the failure mode a naiveawaitfix would slip past.test_walk_uses_exactly_one_to_thread_hopcapturesreal_to_threadbefore patching, so no recursion, and pins the hop count.- The PR body documents the NC-1/2/3 negative-control ladder and a caught vacuous-test fix — good rigor.
Scope — correctly narrow. Routing through _get_all_files_cached, adding limit/offset, and caching parsed metadata are deferred as separate changes; each would alter behavior or the public contract, so keeping them out keeps this reviewable.
Required CI is green on this head. mergeable_state is unstable only from non-required checks (Trivy neutral, coverage in-flight). This is ready to merge on your sign-off — I don't auto-merge to protected main.
Generated by Claude Code
Offloading the learning-log walk to a worker thread stopped it pinning the event loop, but it left a second problem in place: the walk is uncached, so every concurrent request starts its own. The endpoint has no endpoint-specific rate limit, and the default per-client limit permits bursts, so a burst could occupy every worker in the shared default executor and starve unrelated asyncio.to_thread callers. Gate the dispatch on a semaphore capped at 4 in-flight walks. Requests over the cap wait on the event loop holding no worker thread, so the endpoint degrades by queueing instead of by monopolising the executor. asyncio.Semaphore pins itself to the first event loop that awaits it and raises RuntimeError if reused from another, so a single module-level instance would break any process that runs more than one loop. The gate is therefore created per running loop and held in a WeakKeyDictionary keyed by that loop, guarded by a threading.Lock because weakref callbacks can run arbitrary Python and the dictionary is not thread-safe. Adds three tests: the cap holds with more callers than the limit, the gate is rebuilt per event loop, and it is shared within one loop. Negative controls confirm each detects a distinct defect - removing the gate yields 7 concurrent walks against a cap of 4, a fresh gate per call fails both the cap and sharing tests, and a single module-level gate fails the per-loop rebuild test. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
@coderabbitai Thank you — the blocking finding was correct and I have implemented it in Blocking finding: bound concurrent learning-log walks — accepted and fixedYour diagnosis holds. Offloading converts a hard event-loop pin into a shared-resource monopolisation path: I took the semaphore rather than a dedicated bounded executor, for the reason you gave: no lifecycle or shutdown handling to get wrong. async with _get_learning_log_gate():
learning_log = await asyncio.to_thread(data_service.get_learning_log)Cap is 4. Requests over the cap wait on the event loop holding no worker thread, so the endpoint degrades by queueing instead of by monopolising the executor. Exactly one hop remains inside the gate, per your point 3. One subtlety worth flagging, because the obvious implementation is brokenA plain module-level The gate is therefore created per running loop and held weakly: _LEARNING_LOG_MAX_CONCURRENCY = 4
# Maps a running event loop -> the ``asyncio.Semaphore`` bound to that loop.
_learning_log_gates: weakref.WeakKeyDictionary = weakref.WeakKeyDictionary()
_learning_log_gates_lock = threading.Lock()
def _get_learning_log_gate() -> asyncio.Semaphore:
"""Return the learning-log concurrency gate bound to the running loop."""
loop = asyncio.get_running_loop()
with _learning_log_gates_lock:
gate = _learning_log_gates.get(loop)
if gate is None:
gate = asyncio.Semaphore(_LEARNING_LOG_MAX_CONCURRENCY)
_learning_log_gates[loop] = gate
return gate
Proof the cap is real, not decorativeThree tests added to
NC-4 is the direct answer to your finding: with the gate removed, all 7 callers ( The cap test also polls until the first wave saturates, then deliberately sleeps 0.25 s before sampling the peak, so unbounded overflow has time to reveal itself rather than being masked by a race. It carries anti-vacuity assertions on payload equality and Your other four points
Verification after the change
Re-review welcome, particularly on the cap value of 4 and on whether the lock should instead be an |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Focused diff after concurrency-gate commit ==='
git diff --unified=100 origin/main...HEAD -- \
src/youtube_extension/backend/api/v1/router.py \
tests/unit/test_v1_router_extended.py
echo '=== Router structure ==='
ast-grep outline src/youtube_extension/backend/api/v1/router.py --items all --type function --match 'get_learning_log|learning_log|gate' || true
echo '=== Router learning-log implementation and imports ==='
rg -n -C 12 \
'LEARNING_LOG|_get_learning_log_gate|get_learning_log_v1|WeakKeyDictionary|threading\.Lock|asyncio\.Semaphore|asyncio\.to_thread' \
src/youtube_extension/backend/api/v1/router.py
echo '=== Learning-log offloading tests ==='
rg -n -C 10 \
'class TestLearningLogOffloading\b|test_.*(concurr|gate|loop|worker|hop|error)|_LEARNING_LOG_MAX_CONCURRENCY|_learning_log_gates' \
tests/unit/test_v1_router_extended.py
echo '=== Event-loop and worker deployment configuration ==='
rg -n -i -C 4 \
'uvicorn|gunicorn|workers\s*=|--workers|reload|ThreadPoolExecutor|set_default_executor|asyncio\.run\(' \
--glob '*.py' --glob 'pyproject.toml' --glob 'Dockerfile*' --glob 'docker-compose*.y*ml' \
. || true
echo '=== Diff hygiene ==='
git diff --check origin/main...HEAD
git status --shortLength of output: 37300 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Locate router tests and deployment files ==='
fd -t f -a . | rg -i '(^|/)(test_v1_router_extended\.py|.*router.*test.*\.py|Dockerfile.*|.*compose.*\.ya?ml|pyproject\.toml)$' | sed -n '1,240p'
echo '=== Test source from HEAD ==='
git show HEAD:tests/unit/test_v1_router_extended.py 2>/dev/null \
| sed -n '2580,2820p' \
|| true
echo '=== All tracked server/deployment configuration references ==='
git grep -n -i -E \
'uvicorn|gunicorn|--workers|workers[[:space:]]*=|loop[[:space:]]*=|set_default_executor|ThreadPoolExecutor|run_in_executor' \
HEAD -- \
':*.py' ':pyproject.toml' ':Dockerfile*' ':*compose*.yml' ':*compose*.yaml' \
|| true
echo '=== Relevant router imports and Python support declaration ==='
git show HEAD:src/youtube_extension/backend/api/v1/router.py | sed -n '1,35p'
git show HEAD:pyproject.toml 2>/dev/null | rg -n -C 3 'requires-python|python' || true
echo '=== Current diff validation ==='
git diff --check origin/main...HEADLength of output: 30253
The cap of Keep One documentation correction is needed. The concurrency tests are otherwise well targeted. The cap test proves both the upper bound and that the gate does not serialize all requests. The per-loop and same-loop tests protect the intended gate lifecycle. The follow-up in 🐇✅ You are interacting with an AI system. |
…test
CodeRabbit's second-round review flagged that the comment above
`_LEARNING_LOG_MAX_CONCURRENCY` mis-stated when an `asyncio.Semaphore`
binds to an event loop. The claim was that it binds on first acquisition.
That is wrong, and verifying it changed how the fix should be described.
`Semaphore.acquire` in CPython 3.12:
if not self.locked():
self._value -= 1
return True # returns before _get_loop()
fut = self._get_loop().create_future() # only the waiting path binds
The uncontended path never touches the loop. A module-level singleton is
therefore not an obvious bug that any test would catch -- it is a latent
landmine. It works across any number of loops until the first acquisition
that genuinely has to wait; that one pins it, and every later use from a
different loop raises `RuntimeError: ... is bound to a different event loop`.
The failure cannot surface in low-concurrency tests. It waits for exactly
the burst this gate exists to absorb. That makes the per-loop
`WeakKeyDictionary` design more justified, not less.
Verified from CPython source via `inspect.getsource`, then reproduced:
uncontended acquires on two different loops leave `_loop` as `None` with no
error; five waiters against a cap of four pin it; a fresh loop then raises.
Changes:
- Rewrite the comment block to describe contention-triggered binding and
the latent-landmine framing, replacing the incorrect first-use claim.
- Correct the `test_gate_is_rebuilt_for_each_event_loop` docstring.
- Add `test_gate_survives_a_contended_loop_then_a_fresh_loop`, which turns
the proxy identity assertion into a proof that the production failure
mode is prevented: it saturates the gate with `limit + 2` callers so at
least one is a real waiter (the only path that pins the semaphore), then
drives the endpoint on a brand-new loop and fails with a descriptive
message if `RuntimeError` escapes.
Negative control NC-7 (replace the per-loop accessor with a module-level
singleton) fails the new test with the exact production `RuntimeError`,
confirming it discriminates rather than merely passing.
Focused class 8 passed; whole file 133 passed (was 132).
`ruff check` findings identical to baseline (26, differing only by a
+7 line shift); `ruff format --diff` neutral at 189 lines, same as baseline.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Independent review — red-team of the diff on top of CodeRabbit's reviewCodeRabbit ran twice on this branch. Round 1 raised one blocking finding (unbounded concurrent walks); round 2 confirmed the gate fixed it and raised one documentation correction. Both are addressed. Rather than restate that, this is the adversarial pass I ran against my own diff, including three checks CodeRabbit did not and could not perform — it does not execute the test suite. 1. The documentation correction was right, and I was wrong in a way that matteredCodeRabbit's round-2 finding was that my comment mis-stated when # asyncio/locks.py, Semaphore.acquire
if not self.locked():
self._value -= 1
return True # returns before _get_loop()
fut = self._get_loop().create_future() # only the waiting path bindsThen I reproduced it in three phases:
CodeRabbit was correct, and the real behaviour is worse than what I had written. A module-level singleton is not an obvious bug — it is a latent landmine. It passes every low-concurrency test and works indefinitely, then detonates on the first genuinely contended acquisition, which is precisely the burst this gate exists to absorb. This makes the per-loop 2. It also exposed a real weakness in my own testThe correction has a direct consequence I had to act on: if binding requires contention, then So I added 3. NC-7 — the control that proves the pointI re-ran the negative-control ladder with a seventh rung: replace The new test fails with the exact production error, raised from the production line — not a proxy assertion. That is the difference between testing the implementation and testing the failure mode. Restored from backup afterwards and confirmed byte-identical with 4. Verified on the pushed head, in an isolated treeEverything above ran in my working copy, which is not proof about what is actually on the branch. So I checked out the pushed SHA into a detached worktree and re-ran there:
The RED run is worth reading closely: 7 of the 8 tests fail and one passes. The one that passes is 5. Full suiteRun against the exact pushed SHA The The four are pre-existing on clean 6. Lint parity, measured against a stashed baselineAbsolute lint counts are meaningless; only the contrast is. Over the two files this PR touches:
The format delta was +4 before I collapsed an 7. What I looked for and did not find
8. Deliberately not fixed here
ConclusionOne blocking finding, fixed and proven by NC-4. One documentation correction, verified from source, reproduced empirically, and proven by NC-7 — which also drove a genuine strengthening of the test suite rather than just a comment edit. Seven negative controls, all discriminating. RED→GREEN on the pushed SHA in an isolated worktree. Full suite clean apart from four documented pre-existing failures. Lint exactly neutral.
|
Canonical issue
Closes #1386
GET /api/v1/learning-logruns an unbounded, completely uncached filesystem walk directly on the event loop. This PR moves that walk to a worker thread.Outcome
get_learning_log_v1was declaredasyncbut awaited nothing:DataService.get_learning_log()(data_service.py:54-134) is not cheap:self.enhanced_analysis_dir.rglob("*_enhanced.md")parent_dir.glob(f"{video_id}_*_metadata.json").exists()statper entryopen()+json.load().stat()statper entryBecause it all ran inline, the entire process — every other request on every other endpoint, plus the health check — stalled for the duration.
Why this one is worse than the two already fixed
This is the third endpoint in this family, and it is the worst of them:
_get_all_files_cached(), which has a real 60-second TTL (data_service.py:48,136-160).get_learning_log()bypasses that helper entirely and issues its ownrglobat L71. There is no cache to amortise anything./videosendpoint fixed in perf: offload /api/v1/videos page read off the event loop #1382 at least tooklimit/offsetand capped a page at 50.get_learning_log_v1takes no parameters at all — the walk, the per-entry reads and the sort all scale linearly with the total number of analysed videos, forever.So where the earlier fixes bounded a stall, this one removes a stall that grows without limit as the corpus grows.
The second half of the fix: bounding concurrency
Offloading alone would have traded one problem for another, and CodeRabbit's review caught it as a blocking finding before merge.
asyncio.to_threaddispatches to the loop's default executor — the same pool every otherto_threadandrun_in_executor(None, …)caller in the process uses. Sinceget_learning_log()is uncached, each concurrent request starts its own independent walk. The default per-client rate limit is60/minuteand permits bursts, and this endpoint has no limit of its own, so a burst could occupy every worker in the shared pool and delay unrelated executor-backed requests. That is a real change in blast radius: from "one endpoint pins the loop" to "one endpoint starves everyone else's threads".So the dispatch is gated:
Cap is 4 in-flight walks. Requests over the cap wait on the event loop holding no worker thread, so the endpoint degrades by queueing rather than by monopolising the executor. Exactly one
to_threadhop remains inside the gate.I chose a semaphore over a dedicated bounded executor because it has no lifecycle or shutdown handling to get wrong.
Why the gate is per-loop, and why that is not over-engineering
A plain module-level
asyncio.Semaphore(4)is broken here — but not in the way I first wrote, and the correction (raised by CodeRabbit) makes the case for this design stronger rather than weaker.I originally claimed the semaphore binds to the first event loop that awaits it. That is wrong.
Semaphore.acquirein CPython 3.12 reads:The uncontended path never touches the loop. Binding happens only when an acquisition actually has to wait. I verified this from source via
inspect.getsourceand then reproduced it: uncontended acquires under two differentasyncio.run()loops leave_loopasNonewith no error; five concurrent holders against a cap of four pin it to that loop; a fresh loop afterwards raisesRuntimeError: <Semaphore …> is bound to a different event loop.So a naive singleton is not an obvious bug that any test would surface — it is a latent landmine. It works across any number of loops, and keeps working, right up until the first genuinely contended acquisition. That one pins it, and every later use from a different loop fails. The failure therefore cannot appear in low-concurrency tests; it waits for exactly the burst this gate exists to absorb. Building the gate per running loop and holding it weakly removes the trap outright.
The
threading.Lockis load-bearing, not decorative:WeakKeyDictionaryis not thread-safe (weakref finalisers can run arbitrary Python between bytecodes) andTestClientdrives a loop on its own thread. I skipped double-checked locking — one uncontended acquire is ~50 ns against a filesystem walk. Entries drop automatically when their loop is collected, so nothing leaks in a long-lived process.Both failure modes are covered by tests with negative controls, below.
Why one
to_threadhop and not twoThe whole of
get_learning_log()is a single synchronous call. There is no sequencing to break apart — no count-then-page shape like/videoshad — so there is exactly one thing to dispatch. Splitting it would add a hop and buy nothing, andtest_walk_uses_exactly_one_to_thread_hoppins that so a future refactor cannot silently add a second context switch.Why the blocking implementation is left alone
The fix is confined to the async layer.
data_service.pyis not touched.Deliberately out of scope, each worth its own change:
get_learning_log()through_get_all_files_cached()so it stops issuing its ownrgloblimit/offsetso the endpoint stops being unbounded (an API contract change)Those are real improvements, but each changes behaviour or the public contract. This PR changes where the existing work runs and nothing else, which is what makes it reviewable and safe to land on its own.
Scope
src/youtube_extension/backend/api/v1/router.pydata_service.get_learning_log()→await asyncio.to_thread(data_service.get_learning_log), plus a 3-line comment explaining whytests/unit/test_v1_router_extended.pyTestLearningLogOffloading(4 tests)asynciowas already imported (L10). No new dependencies. No new imports in the source file.Not changed:
data_service.py, the route decorator, the response shape, the status codes, theexceptblock.Risk
ThreadPoolExecutorworker.get_learning_log()reads the filesystem and touches no shared mutable state, so there is no new data race. Executor starvation under a concurrent burst was the realistic failure and is now bounded by the semaphore at 4 in-flight walks; the minimum possible default-executor size ismin(32, (os.cpu_count() or 1) + 4)= 5, so the cap always leaves at least one worker for unrelated callers even on a 1-CPU runner. Beyond the cap, requests queue on the event loop holding no thread. The remaining failure mode is added latency for the 5th-and-beyond concurrent caller — strictly better than today, where the first caller stalls the whole loop.awaitinto the sameexcept Exceptionand still yields a 500.test_error_contract_is_unchangedasserts this directly, and the pre-existingtest_get_learning_log_errorstill passes untouched.Verification
All commands run at the pushed head of this branch.
Focused tests
125 before, 133 after — the 8 new tests, with no existing test modified or removed.
test_walk_runs_on_a_worker_threadtest_event_loop_stays_responsive_while_walk_is_in_flighttest_walk_uses_exactly_one_to_thread_hoptest_error_contract_is_unchangedtest_concurrent_walks_are_capped_by_the_gatetest_gate_is_rebuilt_for_each_event_loopRuntimeErrortest_gate_is_shared_within_one_event_looptest_gate_survives_a_contended_loop_then_a_fresh_loopNegative controls
A test that cannot fail is not evidence, so each was mutated and re-run.
router.pytoorigin/main(direct inline call)to_threadtest_walk_uses_exactly_one_to_thread_hopfails, so it is specific rather than incidentally coupledawait asyncio.sleep(0)then call inline — the naive "just add an await" fixasync with, keepingto_thread— i.e. exactly the state CodeRabbit flaggedassert 7 <= 4— alllimit + 3callers walked at once, which is the unbounded burst reportedSemaphoreper call instead of a cached onedict— the loop-leak bugis noton two identical object idsSemaphoresingletonRuntimeError: … is bound to a different event loopNC-4 is the direct proof of CodeRabbit's finding: with the gate removed the test reports the true concurrent count rather than merely failing. The cap test asserts
peak == limit, notpeak <= limit, so it fails in both directions — an absent bound and an over-restrictive one that quietly serialises the endpoint. It also polls until the first wave saturates and then sleeps 0.25 s before sampling the peak, so unbounded overflow has time to appear instead of being masked by a race.NC-3 matters: it is the mistake this class of fix actually attracts. An
awaitmakes the function look correctly async and satisfies any status-code test, while the loop is still pinned.NC-7 is the one that closes the loop on the correction above. Because binding requires contention,
test_gate_is_rebuilt_for_each_event_loopcan only detect a singleton through object identity — it would never see aRuntimeError, since nothing in a quiet test makes an acquisition wait.test_gate_survives_a_contended_loop_then_a_fresh_loopcloses that gap by manufacturing the contention first, and under NC-7 it fails with the real runtime error rather than a proxy assertion. That is the difference between testing the implementation and testing the failure.A vacuous test I caught and fixed before pushing
The first draft of
test_event_loop_stays_responsive_while_walk_is_in_flightcounted loop ticks while the mock blocked onthreading.Event().wait(timeout=2.0)and assertedticks >= 3.It passed under NC-1. That first NC run returned
2 failed, 2 passedrather than the expected3 failed.The reason: the blocking call has a timeout, so it eventually returns. In the inline case the loop is pinned for 2s, the wait times out, the task finishes — and then the three ticks run freely and the assertion passes. It was measuring nothing.
The rewritten test timestamps each tick with
time.monotonic()and records when the walk actually finished, then asserts at least 3 ticks completed strictly before that moment. Inline, every tick necessarily lands after the walk ends, giving zero qualifying ticks. That version fails NC-1 and NC-3 as it should.I am calling this out rather than quietly fixing it because the failure mode — an assertion that is true for the wrong reason — is exactly what the negative-control ladder exists to catch, and it is the reason I run one.
Anti-vacuity assertions
Every test asserts the returned payload equals the mock's return value before asserting anything about threads or hops, and the responsiveness test additionally asserts the blocking work ran to completion. Without these, a change that skipped the call entirely would satisfy the thread-identity and hop-count assertions trivially.
Why the existing tests did not catch this
tests/unit/test_v1_router_extended.py:191configuressvc.get_learning_log.return_value = [...]on aMagicMock. The mock returns instantly, sotest_get_learning_log(L544) passes identically whether the real call blocks for 10ms or 10s — it asserts a status code, and a status code is agnostic to which thread produced it.tests/unit/test_data_service.py:47-100does exercise the realget_learning_log(), but calls it synchronously, so it never observes the endpoint's threading behaviour either.Neither is wrong; they test different things. That gap is why this defect survived two rounds of fixes to its immediate neighbours.
Lint
3 errors total, at
deploy/__init__.py:3:1,data_service.py:340:16andtest_v1_router_extended.py:1966:17. I stashed my changes and re-ran againstorigin/main: byte-identical, same three locations. All pre-existing; none introduced here, and none in code this PR touches.ruff format --diffover the two files this PR touches reports 189 changed lines on this branch and 189 on the stashed baseline — exactly neutral, so this PR adds zero new formatting drift. Reaching parity took two reflows of my own new lines (an over-long annotated assignment, and anasyncio.create_task(…)inside a comprehension that ruff wanted collapsed). CI does not runruff format, andlint-pythoniscontinue-on-error: true, but I hold the branch to parity anyway so the signal stays usable.The
ruff checkfindings were also contrasted against the stashed baseline over the same two files: 26 on both sides, differing only by a uniform +7 line shift from the longer comment block. No new finding of any rule.python -m compileall -q src/— clean, which is the guard CI actually enforces.Full suite
The four failures are the pre-existing ones described below; every other test in the repository passes.
Pre-existing failures
Four tests fail on clean
mainand are unrelated to this change:tests/test_code_generator.py×3 — make livegemini-2.5-flashAPI callstests/test_sdk_python.py::TestEventRelayClient::test_client_no_api_key_header_absent—main.py:57-61callsload_dotenvat import time; a local untracked.envsuppliesEVENTRELAY_API_KEY, whichsdk/python/eventrelay_sdk/client.py:57falls back to, so the header is present. Toggling only that one env var on cleanmainflips the result, which is what identifies it. The hermetic fix (monkeypatch.delenv) belongs in its own PR rather than bundled into a perf change.Focused tests
Required CI
Review threads resolved
Production evidence
Not applicable — no production surface changes.
This PR alters neither the route, the request signature, the response schema, the status codes nor any persisted state.
GET /api/v1/learning-logreturns the identical list of identical objects; the only difference is which thread assembled it. There is no feature flag, no migration and no deploy-order constraint, so there is no production artefact to attach. The relevant evidence is the negative-control ladder above, which demonstrates the tests fail when the fix is absent, when it is doubled, and when it is faked with a bareawait.