perf: offload /api/v1/videos filesystem scan off the event loop (#1379) - #1387
perf: offload /api/v1/videos filesystem scan off the event loop (#1379)#1387groupthinking wants to merge 2 commits into
Conversation
list_videos_v1 is an async endpoint but called count_videos() and get_videos_summary() directly, so both ran on the event loop thread. get_videos_summary "Pass 2" is not cached: for every item on the page it runs parent_dir.glob(), Path.exists(), open() and json.load(). At the default limit of 50 that is roughly 200 blocking syscalls plus up to 50 JSON parses per request, all of which stall every other coroutine on the loop for the duration. Group both reads into _collect_videos_page() and dispatch them with a single asyncio.to_thread hop. One hop rather than two keeps the count and the page consistent with each other and avoids opening a second cache-refresh window between the two reads. Adds TestListVideosOffloading, which asserts where the work runs rather than only what it returns: thread identity for both calls, event-loop responsiveness while a scan is in flight, and preservation of the offset >= total short circuit. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…p count Follow-up to 360589a addressing CodeRabbit's review of #1382. 1. The helper docstring overclaimed. One `asyncio.to_thread` hop is not an atomic snapshot: `DataService` backs both reads with a TTL cache that holds no lock and shares no snapshot object between them, so the entry can still expire — or be refreshed by another worker — between `count_videos()` and `get_videos_summary()`. Reworded to say the grouped hop *narrows* that window to a single thread hand-off rather than eliminating it. 2. The `offset >= total` predicate was duplicated: once inside `_collect_videos_page` and again in `list_videos_v1`. The helper now returns `(total, page, past_end)` and the endpoint branches on `past_end`, so the bounds check lives in exactly one place. Still one dispatch hop. 3. The three offloading tests did not pin the *number* of hops — they all pass for a two-hop implementation that awaits `to_thread` separately per call. Added `test_page_read_uses_exactly_one_to_thread_hop`, which wraps (rather than replaces) the real `asyncio.to_thread` so the work still runs on a worker thread, and asserts exactly one dispatch of `_collect_videos_page`. Negative control NC-4 confirms the new test is load-bearing and the gap was real: under a two-hop implementation the three original tests still pass and only the new test fails, reporting the two dispatched mocks by name. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe video listing endpoint now combines count and page retrieval in ChangesVideo listing pagination
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 3❌ Failed checks (3 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Warning Review ran into problems🔥 ProblemsThese 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 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: BLOCKEDReasons: Machine-readable verdict{
"details": {
"collection_errors": [
"incomplete_linked_issue_contract",
"linked_issue_not_agent_task",
"missing_intent_snapshot",
"missing_agent_run_id",
"missing_agent_login"
],
"invalid_fields": [
"policy.agent_login",
"policy.run_id"
]
},
"reasons": [
"invalid_payload"
],
"verdict": "blocked"
} |
|
@coderabbitai review Generated by Claude Code |
|
✅ Action performedReview finished.
|
|
Status from the automated PR runbook — one red check, and it needs a human decision. Green / in progress
Red: This is a process/provenance gate, not a defect in the diff. The head branch name (
The pre-dispatch snapshot requirement can't be retrofitted onto an already-open PR: it would need a fresh agent-task issue → snapshot → then a new PR. I'm deliberately not fabricating a manifest / Decision needed (human): this is a draft PR touching only backend perf, with no auto-merge label, so it stops at the publish gate regardless. Please either (a) confirm Generated by Claude Code |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/youtube_extension/backend/api/v1/router.py`:
- Around line 951-953: Bound concurrent calls to _collect_videos_page by
guarding the existing asyncio.to_thread invocation with an asyncio.Semaphore,
limiting filesystem scans while retaining exactly one worker-thread hop per
request. Define or reuse a suitable semaphore in the router’s shared scope and
acquire it only around the listing call.
- Around line 951-953: Update DataService._get_all_files_cached to synchronize
reads, refreshes, and assignments of self._file_cache and
self._file_cache_timestamp with a threading.Lock or RLock, including concurrent
calls from asyncio.to_thread via _collect_videos_page. Preserve the existing
cache expiration and returned video behavior while ensuring callers never
observe an inconsistent cache state.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Repository UI (inherited), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5bff5b32-89ea-4510-9be3-7abd1f1438ff
⛔ Files ignored due to path filters (1)
tests/unit/test_v1_router_extended.pyis excluded by!tests/**
📒 Files selected for processing (1)
src/youtube_extension/backend/api/v1/router.py
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: Generate and Upload Coverage
- GitHub Check: trivy
- GitHub Check: test
⚠️ CI failures not shown inline (4)
GitHub Actions: PR Checks / agent-completion_truth-gate: perf: offload /api/v1/videos filesystem scan off the event loop (#1379)
Conclusion: failure
##[group]Run exit 1
�[36;1mexit 1�[0m
shell: /usr/bin/bash -e {0}
##[endgroup]
##[error]Process completed with exit code 1.
GitHub Actions: PR Checks / agent-completion_truth-gate: perf: offload /api/v1/videos filesystem scan off the event loop (#1379)
Conclusion: failure
##[group]Run actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3
with:
script: const fs = require('fs');
const owner = context.repo.owner;
const repo = context.repo.repo;
const marker = '<!-- agent-completion-truth-gate:v1 -->';
const runUrlPrefix = context.serverUrl + '/' + owner + '/' +
repo + '/actions/runs/';
const runUrl = runUrlPrefix + context.runId;
const gateContext = 'agent-completion/truth-gate/pr-' +
process.env.PR_NUMBER;
function gateStatusDisposition(
status,
expectedPendingId,
currentRunUrl,
targetPrefix
) {
if (!/^\d+$/.test(String(expectedPendingId || '')) ||
!status || !/^\d+$/.test(String(status.id || ''))) {
return 'fail_closed';
}
const target = String(
(status && status.target_url) || ''
);
const expectedId = BigInt(String(expectedPendingId));
const statusId = BigInt(String(status.id));
function validRunTarget(targetUrl) {
const value = String(targetUrl || '');
if (!value.startsWith(targetPrefix)) {
return false;
}
const suffix = value.slice(targetPrefix.length);
return /^\d+$/.test(suffix);
}
function statusOwnerId(candidate) {
if (candidate.state === 'pending') {
return BigInt(String(candidate.id));
}
const owner = String(candidate.description || '').match(
/^gate-owner:(\d+)(?:\s|$)/
);
return owner ? BigInt(owner[1]) : null;
}
if (!validRunTarget(currentRunUrl) ||
!validRunTarget(target)) {
return 'fail_closed';
}
const ownerId = statusOwnerId(status);
if (ownerId === null) {
return 'fail_closed';
}
if (ownerId === expectedId && target === currentRunUrl) {
if (statusId === expectedId &&
status.state === 'pending') {
return 'current_pending';
}
if (['failure', 'error'].includes(status.state)) {
return 'already_failed';
}
if (status.state === 'success') {
return 'already_succeeded';
}
return 'fail_closed';
}
if (target === currentRunUrl) {...
GitHub Actions: PR Checks / 0_agent-completion_truth-gate.txt: perf: offload /api/v1/videos filesystem scan off the event loop (#1379)
Conclusion: failure
##[group]Run actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3
with:
script: const fs = require('fs');
const owner = context.repo.owner;
const repo = context.repo.repo;
const marker = '<!-- agent-completion-truth-gate:v1 -->';
const runUrlPrefix = context.serverUrl + '/' + owner + '/' +
repo + '/actions/runs/';
const runUrl = runUrlPrefix + context.runId;
const gateContext = 'agent-completion/truth-gate/pr-' +
process.env.PR_NUMBER;
function gateStatusDisposition(
status,
expectedPendingId,
currentRunUrl,
targetPrefix
) {
if (!/^\d+$/.test(String(expectedPendingId || '')) ||
!status || !/^\d+$/.test(String(status.id || ''))) {
return 'fail_closed';
}
const target = String(
(status && status.target_url) || ''
);
const expectedId = BigInt(String(expectedPendingId));
const statusId = BigInt(String(status.id));
function validRunTarget(targetUrl) {
const value = String(targetUrl || '');
if (!value.startsWith(targetPrefix)) {
return false;
}
const suffix = value.slice(targetPrefix.length);
return /^\d+$/.test(suffix);
}
function statusOwnerId(candidate) {
if (candidate.state === 'pending') {
return BigInt(String(candidate.id));
}
const owner = String(candidate.description || '').match(
/^gate-owner:(\d+)(?:\s|$)/
);
return owner ? BigInt(owner[1]) : null;
}
if (!validRunTarget(currentRunUrl) ||
!validRunTarget(target)) {
return 'fail_closed';
}
const ownerId = statusOwnerId(status);
if (ownerId === null) {
return 'fail_closed';
}
if (ownerId === expectedId && target === currentRunUrl) {
if (statusId === expectedId &&
status.state === 'pending') {
return 'current_pending';
}
if (['failure', 'error'].includes(status.state)) {
return 'already_failed';
}
if (status.state === 'success') {
return 'already_succeeded';
}
return 'fail_closed';
}
if (target === currentRunUrl) {...
Commit Status: agent-completion/truth-gate/pr-1387: agent-completion/truth-gate/pr-1387
Conclusion: failure
gate-owner:51662721515 invalid_payload
🧰 Additional context used
📓 Path-based instructions (10)
**/*.{py,js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{py,js,jsx,ts,tsx}: Use Python 3.9+ and Node 18+ for development
Never hardcode API keys, database URLs, or secrets in code
Make minimal, surgical changes and avoid deleting working code unless fixing security issues
Files:
src/youtube_extension/backend/api/v1/router.py
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.py: Always use type hints for Python functions
Use Black formatter with 88 character line length for Python code
Follow PEP 8 conventions for Python code
Use descriptive variable names and add docstrings to all public functions in Python
Group imports in Python: standard library, third-party, local
Use SQLAlchemy ORM and never write raw SQL queries
Use environment variables via os.getenv() or pydantic-settings to access configuration
Wrap database operations in try-except blocks and use context managers or FastAPI dependencies for connection cleanup
Implement comprehensive error handling with proper logging in all functions
Version APIs using /api/v1/ prefix for stability
Use JSON-RPC 2.0 protocol for all MCP communication
Follow the single-flow workflow: YouTube link → context extraction → agent dispatch → outputs
Use context managers for resource management in Python code
Implement comprehensive input validation and sanitize outputs for security
Use parameterized queries and SQLAlchemy ORM to prevent SQL injection
Define API request/response models using Pydantic for FastAPI endpoints
Store the single unified workflow as the only workflow; never introduce alternate flows or manual triggers
Use SQLAlchemy with connection pooling for database connections and manage sessions with context managers
Provide sensible defaults for non-sensitive configuration in Python settings
Maintain backward compatibility and do not break existing API endpoints
Include comprehensive logging for debugging in MCP implementations
Files:
src/youtube_extension/backend/api/v1/router.py
⚙️ CodeRabbit configuration file
Python backend code. Check for type hints, proper exception handling, async context manager usage, and potential blocking calls in async functions. Flag any bare except clauses or missing timeout parameters on network calls. CRITICAL: Flag any file that contains placeholder/stub implementations — especially in unified_ai_sdk. Any class or function that says "TODO: Replace with production implementation" or returns mock/fake data must be flagged as a blocking issue. Flag any code generation output that reaches users without AST validation or syntax checking.
Files:
src/youtube_extension/backend/api/v1/router.py
**/*.{py,js,ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Maintain >80% code coverage for new features
Files:
src/youtube_extension/backend/api/v1/router.py
**/*.{py,ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{py,ts,tsx}: Keep frontend and backend data models synchronized using matching Pydantic (backend) and TypeScript (frontend) interfaces
Use type-safe interfaces for backend-frontend data exchange
**/*.{py,ts,tsx}: Production code must use real behavior only: no mock delays, fake data, or simulated responses.
Maintain strict type safety: mypy strict mode for Python and TypeScript strict mode for the frontend.
Name events using the<domain>.<entity>.<action>format.
Files:
src/youtube_extension/backend/api/v1/router.py
**/*
📄 CodeRabbit inference engine (Custom checks)
**/*: Strictly verify that GitHub Copilot has explicitly reviewed and approved the pull request; human approvals alone must not satisfy this check.
Before allowing a merge, require thecopilot-rabbitlabel and AI-generated unit tests committed alongside the code changes; fail the check if either is missing.For Vercel-specific work, include
https://vercel.com/docs/llms-full.txtin the AI assistant context set.
Files:
src/youtube_extension/backend/api/v1/router.py
**/*.{py,pyw}
📄 CodeRabbit inference engine (AGENTS.md)
Write Python code to remain compatible with Linux and Windows where possible, including correct handling of
asyncioevent loops.
Files:
src/youtube_extension/backend/api/v1/router.py
src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.py: Format Python code with Black using an 88-character line length.
Sort Python imports with isort using the Black profile.
Run Ruff with E, W, F, I, B, C4, and UP rules; E501 is ignored.
Use mypy strict mode; untyped function definitions are disallowed.
Target Python 3.9 or newer.
Validate backend inputs with Pydantic and sanitize subprocess arguments.
Use Anthropic SDK featuresthinking={"type": "adaptive"}andoutput_config={"effort": "..."}withanthropic>=0.105.0; do not addTypeErrorfallbacks for these parameters.Use the service container dependency-injection pattern in
backend/containers/.
Files:
src/youtube_extension/backend/api/v1/router.py
**/*.{py,ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Do not commit secrets; store keys and credentials in gitignored
.envfiles.
Files:
src/youtube_extension/backend/api/v1/router.py
**/*.{py,pyi}
📄 CodeRabbit inference engine (GEMINI.md)
**/*.{py,pyi}: Use Black formatting with an 88-character line length for Python code.
Use Ruff with rules E, W, F, I, B, C4, and UP for Python linting.
Use strict mypy type checking; do not define untyped functions.
Target Python 3.9 or newer.
Validate Python inputs with Pydantic.
Sanitize subprocess arguments before execution.
Do not add mock delays or fake data to production Python code; production runs in REAL_MODE_ONLY.
Keep secrets out of Python source code; load keys and credentials from environment variables instead.
Use absolute imports that resolve withPYTHONPATH=srcin the Python backend.
Files:
src/youtube_extension/backend/api/v1/router.py
**/*.{py,pyi,ts,tsx}
📄 CodeRabbit inference engine (GEMINI.md)
**/*.{py,pyi,ts,tsx}: Preserve the single workflow: YouTube link → transcript → events → agents → outputs; do not introduce alternative flows or manual triggers that bypass it.
Use event names following<domain>.<entity>.<action>, such asyoutube.video.captured.
Make surgical, precise changes and do not delete working code without justification.
Files:
src/youtube_extension/backend/api/v1/router.py
🔍 Remote MCP Context7, GitHub Copilot
Review-relevant context
- PR
#1387targets issue#1379, is draft, and changes only the router plus regression tests. The helper performscount_videos()and the bounds check before the page read inside oneasyncio.to_threadcall. DataServicemaintains mutable_file_cacheand_file_cache_timestampstate with a 60-second TTL; no lock is present. Since the same service may now be used from worker threads, concurrent-request cache access is worth verifying.- Existing behavior is preserved in the implementation:
offset >= totalskipsget_videos_summary(), and the outer handler still converts failures to HTTP 500. The added tests cover thread identity, loop responsiveness, bounds short-circuiting, and exactly one hop. - Related merged PR
#1288documents thatasyncio.to_threaduses the shared default executor and relocates work without reducing scan wall time; concurrent listing volume may therefore create executor pressure. - Current checks are incomplete: test, build, coverage, and security jobs were still in progress; an agent-completion truth gate failed due to invalid payload. No review threads or submitted reviews exist yet.
- The project declares Python
>=3.10, compatible withasyncio.to_thread.
Context7 was rate-limited, and repository secret scanning was unavailable because GitHub Advanced Security is disabled.
🔇 Additional comments (1)
src/youtube_extension/backend/api/v1/router.py (1)
911-937: LGTM!Also applies to: 954-961
| total, paginated_videos, past_end = await asyncio.to_thread( | ||
| _collect_videos_page, data_service, limit, offset | ||
| ) |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔵 Trivial
Bound concurrent filesystem scans.
asyncio.to_thread uses the shared default executor. Concurrent requests can occupy all available workers while filesystem scans run. Unrelated thread-offloaded work can then queue and experience higher latency.
Add an asyncio.Semaphore around the listing call, or confirm capacity with load tests and executor metrics. Keep the required single worker-thread hop per request.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/youtube_extension/backend/api/v1/router.py` around lines 951 - 953, Bound
concurrent calls to _collect_videos_page by guarding the existing
asyncio.to_thread invocation with an asyncio.Semaphore, limiting filesystem
scans while retaining exactly one worker-thread hop per request. Define or reuse
a suitable semaphore in the router’s shared scope and acquire it only around the
listing call.
Source: MCP tools
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
service="src/youtube_extension/backend/services/data_service.py"
router="src/youtube_extension/backend/api/v1/router.py"
ast-grep outline "$service" --items all --type function
rg -n -C 20 \
'_file_cache|_file_cache_timestamp|_get_all_files_cached|threading\.(Lock|RLock)' \
"$service"
rg -n -C 8 'asyncio\.to_thread|_collect_videos_page|list_videos_v1' "$router"Repository: groupthinking/EventRelay
Length of output: 10153
🏁 Script executed:
#!/bin/bash
set -euo pipefail
service="src/youtube_extension/backend/services/data_service.py"
router="src/youtube_extension/backend/api/v1/router.py"
printf 'Cache-related symbols in data_service.py:\n'
rg -n -C 12 \
'self\._file_cache|_file_cache_timestamp|_get_all_files_cached|count_videos|get_videos_summary|lock|RLock|Lock' \
"$service"
printf '\nRelevant router endpoint:\n'
sed -n '911,980p' "$router" | cat -n
printf '\nThread-safety / async imports in data_service.py:\n'
rg -n 'import threading|from threading|import asyncio|asyncio\.' "$service" || true
printf '\nBehavioral probe of DataService cache under interleaved refresh-like reads:\n'
python3 - <<'PY'
from pathlib import Path
import sys
sys.path.insert(0, str(Path("src").resolve()))
try:
from youtube_extension.backend.services.data_service import DataService
except Exception as exc:
raise SystemExit(f"Could not import DataService: {exc}")
service = DataService(enhanced_analysis_dir="youtube_processed_videos/enhanced_analysis",
feedback_dir="youtube_processed_videos/feedback",
knowledge_dir="youtube_processed_videos/knowledge")
print("initial cached count:", service.count_videos())
print("_file_cache type:", type(service._file_cache).__name__)
print("_file_cache_timestamp type:", type(service._file_cache_timestamp).__name__)
print("has _file_cache_lock:", hasattr(service, "_file_cache_lock") or any("_file_cache_lock" in line for line in Path(service.__module__.replace(".", "/") + ".py").read_text(errors="ignore").splitlines() if "lock" in line))
PYRepository: groupthinking/EventRelay
Length of output: 7759
Make DataService cache updates thread-safe.
_get_all_files_cached() reads, refreshes, and updates self._file_cache / self._file_cache_timestamp without a lock. Requests dispatched through asyncio.to_thread() can read while another refresh replaces the shared list and timestamp, so protect the cache with threading.Lock / RLock or replace it with a snapshot-safe method.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/youtube_extension/backend/api/v1/router.py` around lines 951 - 953,
Update DataService._get_all_files_cached to synchronize reads, refreshes, and
assignments of self._file_cache and self._file_cache_timestamp with a
threading.Lock or RLock, including concurrent calls from asyncio.to_thread via
_collect_videos_page. Preserve the existing cache expiration and returned video
behavior while ensuring callers never observe an inconsistent cache state.
Source: MCP tools
|
Closing as a duplicate of already-merged #1382 — no action needed. This PR's head commit That also resolves the On CodeRabbit's two findings — both are about code that is already in
Nothing is lost by closing: the fix is merged, and the two suggestions are captured here as follow-up candidates for a Generated by Claude Code |
1 participant
Canonical issue
Closes #1379
Outcome
GET /api/v1/videosno longer blocks the FastAPI event loop while it scans the filesystem.count_videos()andget_videos_summary()are both blocking, uncached I/O; they are now grouped into a single synchronous helper (_collect_videos_page) dispatched with exactly oneasyncio.to_threadhop, so the loop stays free to serve other requests while a listing is in flight.Scope
_collect_videos_page(data_service, limit, offset)helper that returns(total, page, past_end)and owns theoffset >= totalshort-circuit so the caller does not re-derive the bounds check.list_videos_v1now awaits the helper via a singleasyncio.to_threadhop instead of two inline blocking calls.TestListVideosOffloading(4 cases) asserting where the work runs and that exactly one thread hop is used.Risk
DataServicereads were somehow not thread-safe, moving them to a worker thread could surface a latent race. They are already invoked concurrently under load, so this does not introduce new sharing.Verification
Tied to head
e9b738c:pytest tests/unit/test_v1_router_extended.py::TestListVideosOffloading→ 4 passedThe tests are written to be non-vacuous: each asserts the normal payload is produced and that the blocking calls execute on a non-loop thread, so a status-code-only implementation cannot pass them.
Production evidence
Not applicable — internal event-loop scheduling change with no user-visible API contract change. Payload shape (
videos,total,limit,offset,has_more) is unchanged and asserted by the tests.Agent handoff
🤖 Generated with Claude Code
Generated by Claude Code