perf: scan processed-video cache off the event loop - #1288
Conversation
GET /api/v2/videos/list is declared async but its whole body was blocking filesystem work: a stat, a directory glob, and one open()+json.load() per cached video, with no bound on entry count. The handler never awaited, so the loop was stalled for the full scan and no other request could be served. Extract the scan into a module-level _collect_processed_videos_sync() helper and dispatch it with asyncio.to_thread(), matching the pattern used in #1194, #1196, #1228, #1233, #1240, #1245 and #1251. The scan logic is moved verbatim, so the response payload, newest-first ordering, per-entry corrupt-file skip and empty-list fallbacks are unchanged. Measured on a 2,000-entry cache, peak event-loop stall drops from ~193 ms to ~2 ms. Scan wall time is unchanged: this is a latency and fairness fix, not a throughput one. Closes #1287 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
@linear-code @coderabbitai review |
📝 WalkthroughSummary by CodeRabbit
WalkthroughChangesProcessed-video listing
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant get_processed_videos_list
participant asyncio_to_thread
participant collect_processed_videos_sync
participant ProcessedVideoCache
get_processed_videos_list->>asyncio_to_thread: offload cache scan
asyncio_to_thread->>collect_processed_videos_sync: run synchronous helper
collect_processed_videos_sync->>ProcessedVideoCache: read processed-video JSON files
ProcessedVideoCache-->>collect_processed_videos_sync: cached video data
collect_processed_videos_sync-->>get_processed_videos_list: sorted video summaries
Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5 | ❌ 2❌ Failed checks (2 inconclusive)
✅ Passed checks (5 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 |
|
✅ Action performedReview finished.
|
Agent Completion Truth Gate: NOT_APPLICABLEEvidence agrees. Machine-readable verdict{
"details": {},
"reasons": [],
"verdict": "not_applicable"
} |
There was a problem hiding this comment.
Pull request overview
Moves processed-video cache scanning off FastAPI’s event loop while preserving endpoint behavior.
Changes:
- Extracts synchronous cache scanning into a module-level helper.
- Runs the helper through
asyncio.to_thread(). - Adds regression and behavior-parity tests.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/youtube_extension/backend/real_api_endpoints.py |
Offloads blocking cache I/O to a worker thread. |
tests/unit/test_real_api_endpoints.py |
Tests thread offloading, responsiveness, and existing behavior. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/real_api_endpoints.py`:
- Around line 50-74: Run Black on the modified helper in real_api_endpoints.py
to wrap lines exceeding 88 characters and normalize string quoting, without
changing the helper’s behavior.
🪄 Autofix (Beta)
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: 48cc60f6-b58c-453e-887f-080106ec4315
⛔ Files ignored due to path filters (1)
tests/unit/test_real_api_endpoints.pyis excluded by!tests/**
📒 Files selected for processing (1)
src/youtube_extension/backend/real_api_endpoints.py
📜 Review details
⏰ Context from checks skipped due to timeout. (5)
- GitHub Check: test
- GitHub Check: build
- GitHub Check: Security Scan - python
- GitHub Check: Generate and Upload Coverage
- GitHub Check: trivy
⚠️ CI failures not shown inline (4)
GitHub Actions: PR Checks / agent-completion_truth-gate: perf: scan processed-video cache off the event loop
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 / 0_agent-completion_truth-gate.txt: perf: scan processed-video cache off the event loop
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 / agent-completion_truth-gate: perf: scan processed-video cache off the event loop
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-1288: agent-completion/truth-gate/pr-1288
Conclusion: failure
gate-owner:51574894844 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/real_api_endpoints.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/real_api_endpoints.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/real_api_endpoints.py
**/*.{py,js,ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Maintain >80% code coverage for new features
Files:
src/youtube_extension/backend/real_api_endpoints.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/real_api_endpoints.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/real_api_endpoints.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/real_api_endpoints.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/real_api_endpoints.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/real_api_endpoints.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/real_api_endpoints.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/real_api_endpoints.py
🪛 ast-grep (0.45.0)
src/youtube_extension/backend/real_api_endpoints.py
[warning] 49-49: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(cache_file, encoding='utf-8')
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
🔍 Remote MCP GitHub Copilot
Review-relevant context
- PR
#1288changes only the cache-list endpoint and its tests. The endpoint now delegates the full scan to_collect_processed_videos_syncviaawait asyncio.to_thread(...); payload construction and sorting were moved unchanged. - Issue
#1287requires off-loopexists,glob, and JSON reads while preserving 12-key entries, newest-first ordering, corrupt-file skipping, and empty/error fallbacks. - The repository already uses this helper-plus-
asyncio.to_threadpattern, including the merged PR#1228for video cache I/O. - Current
real_video_processor.pywrites cache files through a temporary file followed byos.replace, so normal in-process cache writes are atomic. This makes the PR description’s “torn read becomes marginally more likely” warning mainly relevant to external/non-atomic writers. - The new tests verify worker-thread execution, loop responsiveness, payload parity, missing/empty directories, corrupt entries, ordering, and filename filtering.
httpxis already a project dependency. - CI is not yet fully green:
test,build, coverage, security scans, and automated reviews remain in progress. The agent-completion truth gate has failed withinvalid_payload; lint, guards, dependency review, gitleaks, and several other checks succeeded.
🔇 Additional comments (2)
src/youtube_extension/backend/real_api_endpoints.py (2)
10-15: LGTM!
241-247: 📐 Maintainability & Code QualityBlock PR
#1288until the merge gates are satisfied.PR
#1288already has thecopilot-rabbitlabel and AI-generated unit tests committed, but GitHub Copilot only submitted aCOMMENTEDreview. Require an explicit CopilotAPPROVEDreview before this change can proceed.
Normalize string quotes to double and wrap the dict-append and sort call in _collect_processed_videos_sync to satisfy the 88-char limit, addressing the CodeRabbit review on #1288. Behaviour-preserving: diff is confined to the new helper and the reformat is Black's own AST-equivalent output (verified with --target-version py311). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013rG7vUAn6z9tXoEuA3dAqz
|
@coderabbitai Thanks — actioned in
I scoped the reformat to the new helper rather than the whole file, because:
Verification after the fix:
@coderabbitai review |
|
@linear-code this one is still awaiting your read — flagging it since you've Summary: Measured on a 2000-entry warm cache, max event-loop stall drops All checks are green including |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Commit summary and diff ==='
git show --stat --oneline --decorate --find-renames bfb60d1d7
git show --format=fuller --find-renames --no-ext-diff bfb60d1d7 -- \
src/youtube_extension/backend/real_api_endpoints.py \
tests/unit/test_real_api_endpoints.py
echo '=== Changed-file structure ==='
ast-grep outline src/youtube_extension/backend/real_api_endpoints.py --items all --type function --match 'collect_processed_videos_sync|list' || true
ast-grep outline tests/unit/test_real_api_endpoints.py --items all --type function --match 'processed|video|thread|loop' || true
echo '=== Relevant implementation locations ==='
rg -n -C 12 \
'def _collect_processed_videos_sync\b|asyncio\.to_thread\s*\(|/api/v2/videos/list|processed_videos' \
src/youtube_extension/backend/real_api_endpoints.py
echo '=== Relevant test locations ==='
rg -n -C 8 \
'_collect_processed_videos_sync|asyncio\.to_thread|worker.thread|threading|/api/v2/videos/list|processed_videos' \
tests/unit/test_real_api_endpoints.pyLength of output: 10795 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Candidate endpoint test files ==='
fd -t f -i 'real_api_endpoints' . || true
fd -t f -e py . | rg -n '(^|/)(test|tests?)(_|/)|real_api|endpoint'
echo '=== Cache-list implementation introduced by this PR ==='
git diff --no-ext-diff --unified=80 bfb60d1d7^1..bfb60d1d7^1 -- \
src/youtube_extension/backend/real_api_endpoints.py
echo '=== Test definitions and assertions ==='
rg -n -C 10 \
'get_processed_videos_list|/api/v2/videos/list|_collect_processed_videos_sync|asyncio\.to_thread|threading\.get_ident|worker.?thread|processed_at' \
-g '*.py' .
echo '=== Python test configuration and changed test files across the PR ==='
fd -t f -a 'pyproject.toml' -o -a 'pytest.ini' -o -a 'setup.cfg' -o -a 'tox.ini' .
git diff --name-status bfb60d1d7^1..bfb60d1d7
git diff --name-status "$(git merge-base origin/main HEAD)"..HEADLength of output: 184890 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Repository state ==='
git status --short
git remote -v
git branch -a --contains HEAD
git log --oneline --decorate -20
echo '=== Files changed from the merge base with main ==='
if git show-ref --verify --quiet refs/remotes/origin/main; then
base="$(git merge-base refs/remotes/origin/main HEAD)"
echo "merge-base: $base"
git diff --name-status "$base"..HEAD
git log --oneline "$base"..HEAD
else
echo 'refs/remotes/origin/main is unavailable'
fi
echo '=== Tracked Python test files ==='
git ls-files '*.py' | rg -n '(^|/)(test[^/]*|[^/]*_test)\.py$' || true
echo '=== Test-related commits and occurrences ==='
git log --all --oneline --decorate -- \
'tests/**' '*test*.py' 'src/youtube_extension/backend/real_api_endpoints.py' | head -80
git grep -n -I -E \
'get_processed_videos_list|/api/v2/videos/list|_collect_processed_videos_sync' \
HEAD -- '*.py' || true
echo '=== GitHub PR file list ==='
gh pr view 1288 --repo groupthinking/EventRelay --json baseRefName,headRefName,commits,files \
--jq '{baseRefName, headRefName, commits: [.commits[] | {oid: .oid, messageHeadline: .messageHeadline}], files: [.files[] | {path: .path, additions: .additions, deletions: .deletions}]}'Length of output: 18573 🏁 Script executed: #!/bin/bash
set -euo pipefail
base="$(git merge-base refs/remotes/origin/main HEAD)"
echo '=== Complete PR diff ==='
git diff --no-ext-diff --unified=16 "$base"..HEAD -- \
src/youtube_extension/backend/real_api_endpoints.py \
tests/unit/test_real_api_endpoints.py
echo '=== New and modified test definitions ==='
git diff --no-ext-diff --unified=6 "$base"..HEAD -- tests/unit/test_real_api_endpoints.py \
| rg -n -C 8 '^\+def test_|^\+async def test_|^\+class Test|thread|asyncio|processed|cache|corrupt|sort|empty|missing' \
|| true
echo '=== Endpoint implementation before and after the feature commit ==='
git show 4945cf1^:src/youtube_extension/backend/real_api_endpoints.py | sed -n '1,90p;230,270p'
echo '--- after ---'
git show HEAD:src/youtube_extension/backend/real_api_endpoints.py | sed -n '1,95p;235,265p'
echo '=== GitHub PR state and changed files ==='
gh pr view 1288 --repo groupthinking/EventRelay \
--json state,isDraft,mergeable,reviewDecision,statusCheckRollup,files \
--jq '{state, isDraft, mergeable, reviewDecision, files: [.files[] | {path, additions, deletions}], checks: [.statusCheckRollup[] | {name, conclusion, status}]}'Length of output: 41153
The endpoint still moves the full cache scan to The added tests cover off-loop execution, event-loop responsiveness, response parity, and cache-scan edge cases. I found no blocking issue in this update. CI still has 🐇 ✅ If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! ✅ Action performedReview finished.
|
|
Remediation complete — Merge is intentionally left for a human: base gh pr merge 1288 --repo groupthinking/EventRelay --squash --delete-branchGenerated by Claude Code |
|
@linear Please review this one. Context: this is an event-loop offload in the same family as #1228, #1233, #1240, #1245, #1251, #1262 (all merged). Three things I would specifically like challenged, because they are the parts where I made a judgement call rather than a mechanical transformation:
One thing I will pre-emptively flag, since you caught the equivalent on #1245: Verification summary: 88 passed in |
|
I don’t see a blocker here.
On the The only nuance I’d call out is that the thread-recorder test proves |
The thread-recording cache directory previously asserted only that exists()/glob() ran off-loop, and relied on the helper extraction to imply the per-entry open()/json.load() moved with them. glob() now yields path-like proxies whose __fspath__ records the calling thread. Because open() resolves a non-str argument through __fspath__, this captures the thread at the exact moment each blocking read starts, so the read is proven off-loop rather than inferred. Verified by reverting only the handler call site to the inline form: the new assertion fails independently with "blocking cache entry read ran on the event loop thread". Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
@linear Thanks — both points actioned in 1. Headline reframed. 2. Read-vs-scan nuance closed. You were right that the recorder proved I re-ran the prove-fail for the new assertion specifically. Because the scan assertion fires first and masks it, I re-ran against the same inline source with the scan assertion temporarily neutralised, and it fails on its own: I left Still 88 passed. Ruff is unchanged against Anything further, or is this good to merge? |
|
This looks good to merge. The extra |
* perf: offload video-detail cache read, drop the stat probe
GET /api/v2/videos/{video_id} performed its whole cache lookup inline on
the event loop: Path.exists(), then open(), then a full json.load() of the
stored analysis. Only the two syscalls are bounded; the parse scales with
the payload the processor wrote, so a large analysis stalled every other
in-flight request on the worker.
Add a module-level _read_video_analysis_sync() helper and await it through
asyncio.to_thread(), mirroring _collect_processed_videos_sync() from #1288.
_get_cache_path() stays on the loop: it is pure string arithmetic.
The helper opens directly and treats FileNotFoundError as the miss instead
of probing with exists() first. That is one syscall rather than two, and it
closes the window in which the entry could be removed between the check and
the open - a race that previously surfaced as a 500 rather than the correct
404. Every other OSError still propagates, so a directory or an unreadable
entry keeps surfacing as a 500 instead of being reported as a missing video.
Deliberately does not reuse RealVideoProcessor._read_cache_file. That helper
applies a 24-hour TTL and returns None for anything older; this endpoint has
never had a TTL, so reusing it would silently turn every analysis over a day
old into a 404. TestVideoDetailIgnoresProcessorCacheTtl pins that.
real_video_processor.py is left untouched (claimed by open PR #1237).
Verification: 97 passed (88 pre-existing + 9 new). Prove-fail: reverting only
the to_thread delegation, keeping the helper defined, fails exactly the two
off-loop tests (ticks=0, assert 0 >= 5) and passes the other 95.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
* fix: keep JSON null and null-byte ids off the 404/500 path
Review of the parent commit surfaced two behaviour regressions introduced
by replacing the ``exists()`` + ``open()`` pair with a single ``open()``.
1. A cache entry whose content is the JSON literal ``null`` parses to
``None``, which the handler could not distinguish from "no entry".
``main`` served it as a 200; the parent commit turned it into a 404.
Fixed with a module-level ``_CACHE_MISS`` sentinel and an identity
check, so every falsy payload (``null``, ``{}``, ``[]``, ``""``, ``0``,
``false``) keeps its 200.
2. ``Path.exists()`` swallows ``ValueError`` as well as ``OSError``, so a
``video_id`` carrying an embedded null byte (``GET /api/v2/videos/%00``)
used to report the entry as absent and return 404. A bare ``open()``
let the ``ValueError`` escape and turned that into a 500. Fixed by
treating ``ValueError`` as a miss alongside ``FileNotFoundError``.
Every other ``OSError`` still propagates, so the directory case and
the corrupt-JSON case keep their 500s.
Verified with a four-case version-swap parity probe (null-content entry,
control object, absent entry, %00) showing byte-identical status codes
and response bodies between ``main`` and this branch.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
* docs: narrow the offload claim to filesystem latency
json.load holds the GIL, so the to_thread hop relocates the parse stall
rather than removing it. Narrow the documented guarantee accordingly and
add a characterisation test so the weaker claim stays honest.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
* docs: link residual parse stall to issue #1306
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
---------
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Canonical issue
Closes #1287.
Outcome
/api/v2/videos/listcache scan — a directorystat, aglob, and oneopen()+json.load()per cached video — off the event loop into a worker thread viaasyncio.to_thread().processed_at, same per-entrytry/exceptthat skips a corrupt file and logsError loading cached video …, same[]for a missing cache directory, same[]from the outer handler on unexpected error.GET /api/v2/videos/{video_id}(get_video_analysis), which has the same blockingopen()+json.load()defect, orclear_processing_cache, which calls blockingshutil.rmtree()+mkdir(). Both are scoped out in #1287 and left for separate PRs.real_video_processor.py, whose cache handling is claimed by open PR #1237.Scope
Two files.
src/youtube_extension/backend/real_api_endpoints.py_collect_processed_videos_sync(cache_dir: Path) -> list[dict[str, Any]]. The body is the previous handler body moved unchanged — theexists()early return, theglob("*_processed.json")loop, the per-fileopen()/json.load()insidetry/except, the 12-key dict build, and the descending sort.get_processed_videos_list()reduces to resolvingprocessor.cache_dirandreturn await asyncio.to_thread(_collect_processed_videos_sync, processor.cache_dir). The outerexceptthat returns[]is untouched.asyncioandpathlib.Path(the latter for the helper's annotation).tests/unit/test_real_api_endpoints.py— one helper class and eight tests appended. No existing test was modified.The helper is module-level rather than nested inside
setup_real_api_endpoints()so it is directly unit-testable, matching the*_syncconvention already used inapi_cost_monitor.py,protocol_bridge.py,deployment_manager.pyandcloud_tasks_queue.py.Risk
This is a latency change, not a throughput change. The scan does exactly the same
filesystem work and takes the same wall-clock time; it simply no longer does it on the
loop thread. The benefit is entirely to other coroutines, which previously could not
run at all for the duration of the scan.
Thread-safety. The helper takes
cache_diras a parameter and touches no sharedmutable state. It reads the filesystem, builds a fresh list, and returns it. The only
cross-thread interaction is
logger.warning()on a corrupt entry, andloggingisthread-safe by design.
processor.cache_diris still resolved on the loop thread beforedispatch, so the
get_real_video_processor()call ordering is unchanged.Thread-pool pressure.
asyncio.to_threaduses the loop's default executor, sharedwith the other
to_threadcall sites in this codebase. This adds one occupant perin-flight
/videos/listrequest. That is a real cost, but it is bounded by concurrentrequest count and is strictly better than the status quo, where the same work occupied
the only loop thread. No new executor is introduced.
A torn read becomes marginally more likely. Previously the scan was atomic with
respect to other coroutines in this process; now a write from another task can interleave
with it. In practice this changes nothing: the scan already had no atomicity guarantee
against the separate worker processes that write these files, and the existing per-entry
try/exceptalready treats a half-written file as skippable. No new failure mode.Verification
Non-vacuity
Headline: max event-loop stall drops from ~190 ms to ~2 ms while scan wall time stays
flat. The worst-case blocking of the loop is eliminated; the work itself is not made
faster, and this PR does not claim it is.
Benchmarked with a 2,000-entry cache of realistic
*_processed.jsonpayloads (transcriptplus AI-analysis bodies), page cache pre-warmed so both variants are comparable, and a
1 ms heartbeat task measuring how long the loop goes unscheduled. Only the dispatch
strategy varies:
main)to_thread(this PR)Across three runs the max stall was 192.6 / 176.3 / 214.7 ms before and 2.0 / 1.6 / 2.0 ms
after. Scan wall time was unchanged in every run.
The other rows are deliberately not the headline, and deserve honest reading:
the 0.2 s of blocking work off the loop thread rather than making it cheaper — so the
max-stall drop is attributable to the offload and not to doing less work.
This is a tail-latency defect, so only the max is expected to move.
an indirect proxy — a count of how often an unrelated task got scheduled — so they
corroborate the max-stall figure rather than establishing it.
Prove-fail
The eight new tests were run against the pre-change source. To isolate the property under
test rather than produce a collection error, only the call site was reverted to its
inline form while leaving the helper defined, so imports still resolve. Two tests fail:
with:
The directory scan and the per-entry read are two separate blocking operations, so the
thread recorder asserts both independently. Because the scan assertion fires first and
would otherwise mask the read assertion, the read assertion was re-run in isolation
against the same inline source with the scan assertion temporarily neutralised, and fails
on its own:
Six of the eight pass both before and after, and are documented as such rather than
presented as proof of the change:
test_offloaded_scan_returns_same_payload— a parity check. It is designed to passin both states; that is the point, since the payload must not change.
TestCollectProcessedVideosSynctests — these unit-test the extracted helperdirectly. They pass before the change only because the prove-fail run deliberately
leaves the helper defined. They exist to pin the behaviour that was moved, not to
demonstrate the offload.
Tests added
test_cache_scan_runs_off_the_event_loop_threadexists()/glob()call and every per-entryopen()records a thread id different from the loop thread'stest_event_loop_stays_responsive_during_cache_scantest_offloaded_scan_returns_same_payloadtest_missing_directory_returns_empty_listcache_dir→[], no exceptiontest_empty_directory_returns_empty_listcache_dir→[]test_corrupt_entry_is_skipped_without_failing_the_scantest_results_are_sorted_by_timestamp_descendingtest_non_matching_files_are_ignored*_processed.jsonare not readThe loop thread id is captured by patching
get_real_video_processor, which the handlercalls on the loop thread immediately before dispatch. This avoids assuming the test body
itself runs on that loop —
TestClientdrives the loop on a separate thread.The per-entry read is proven rather than inferred: the stub
glob()yields path-likeproxies whose
__fspath__records the calling thread.open()resolves a non-strargument through
__fspath__, so the thread is captured at the exact moment each blockingread begins. Without this, the test would only show that
exists()/glob()movedoff-loop and would rely on the helper extraction to imply the
open()/json.load()movedwith them.
Commands
Baseline on
mainis 80 passed; the 8 new tests bring it to 88. All 80 pre-existing testspass unmodified, including the seven in
TestGetProcessedVideosListEndpointthat pin theempty-directory, missing-directory, corrupt-entry, ordering and exact-field-set behaviour
of this endpoint.
Ruff was run on both changed files and diffed against their
origin/maincounterparts.The diagnostic sets are identical (6 pre-existing
B904, none in changed lines, noneadded).
Production evidence
Not applicable. This endpoint is a backend FastAPI route that is not exercised by the
Vercel preview deployment, and the change is behaviour-preserving — the same directory is
scanned, the same files are parsed, and the same payload is returned; only the thread it
runs on changes. Correctness is covered by the eight focused tests above plus the seven
untouched behavioural tests for this endpoint, and the performance claim by the benchmark
in "Non-vacuity".
Agent handoff
production-approval change.