perf: offload video result cache disk I/O to worker threads - #1228
Conversation
RealVideoProcessor._load_from_cache and _save_to_cache ran blocking filesystem and JSON work inline inside async def, stalling the event loop for every concurrently-served request on the hot path of process_video(). Move both to asyncio.to_thread via two static helpers. _read_cache_file collapses the previous exists/stat/open/json.load sequence into a single off-loop hop, which also closes the TOCTOU window where a cache entry could be evicted between the existence check and the read. Observable behaviour is unchanged, including the 24h TTL boundary and the broad except that degrades a corrupt cache entry to a miss. Add TestCacheDiskIOOffEventLoop, which asserts on thread identity rather than wall-clock timing so it stays deterministic under CI load. The two off-loop tests fail against the pre-change implementation. Closes #1227 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reached
Next review available in: 46 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository YAML (base), Repository UI (inherited), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
Walkthrough
ChangesResult-cache I/O
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
Suggested labels: 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: NOT_APPLICABLEEvidence agrees. Machine-readable verdict{
"details": {},
"reasons": [],
"verdict": "not_applicable"
} |
|
Please triage and track this one. Context for the issue record:
This is item 1 of 10 in a sequenced performance campaign against event-loop-blocking calls in this service. Remaining items target |
|
@coderabbitai review Adversarial challenges — please try to break these specifically rather than summarising the diff:
|
|
I need your go-ahead before updating #1227.
Should I apply it? |
There was a problem hiding this comment.
Pull request overview
Offloads RealVideoProcessor cache filesystem and JSON operations from the event loop.
Changes:
- Adds threaded cache read/write helpers and a shared TTL constant.
- Adds regression tests for thread execution, TTL behavior, and payload preservation.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/youtube_extension/backend/services/real_video_processor.py |
Moves cache operations to worker threads. |
tests/unit/test_real_processors.py |
Tests off-loop cache behavior and semantics. |
Suppressed comments (1)
src/youtube_extension/backend/services/real_video_processor.py:103
- Moving this direct write into the thread pool removes the event loop's previous serialization. Concurrent requests—or duplicate URLs in
batch_process_videos—can now open the same final path withwon separate workers and interleavejson.dumpwrites, leaving a corrupt cache entry. Write each payload to a unique temporary file in the cache directory and atomicallyos.replaceit (and cover simultaneous saves) so readers only observe complete JSON.
with open(cache_path, 'w', encoding='utf-8') as f:
json.dump(payload, f, indent=2, ensure_ascii=False, default=str)
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- changed files ---'
git diff --name-only HEAD^ HEAD
printf '%s\n' '--- implementation outline ---'
ast-grep outline src/youtube_extension/backend/services/real_video_processor.py --items all || true
printf '%s\n' '--- test outline ---'
ast-grep outline tests/unit/test_real_processors.py --items all || true
printf '%s\n' '--- cache symbols and test class ---'
rg -n -C 12 '(_CACHE_TTL_SECONDS|_load_from_cache|_save_to_cache|_read_cache_file|_write_cache_file|TestCacheDiskIOOffEventLoop)' \
src/youtube_extension/backend/services/real_video_processor.py \
tests/unit/test_real_processors.py
printf '%s\n' '--- PR diff ---'
git diff --find-renames HEAD^ HEAD -- \
src/youtube_extension/backend/services/real_video_processor.py \
tests/unit/test_real_processors.py
printf '%s\n' '--- pytest/xdist configuration references ---'
rg -n -C 3 '(xdist|pytest|addopts|testpaths)' \
pyproject.toml pytest.ini setup.cfg tox.ini conftest.py 2>/dev/null || trueLength of output: 19884 🏁 Script executed: #!/bin/bash
set -u
test_file="$(fd -t f '^test_real_processors\.py$' . | head -n 1)"
printf '%s\n' "--- discovered test file: ${test_file:-not found} ---"
printf '%s\n' '--- current implementation: Lines 70-150 ---'
sed -n '70,150p' src/youtube_extension/backend/services/real_video_processor.py
printf '%s\n' '--- parent implementation: Lines 65-145 ---'
git show HEAD^:src/youtube_extension/backend/services/real_video_processor.py 2>/dev/null | sed -n '65,145p' || true
printf '%s\n' '--- focused source diff ---'
git diff --find-renames HEAD^ HEAD -- src/youtube_extension/backend/services/real_video_processor.py || true
if [ -n "${test_file:-}" ]; then
printf '%s\n' '--- cache regression tests ---'
rg -n -C 20 '(TestCacheDiskIOOffEventLoop|patch\.dict|threading\.get_ident|json\.load|json\.dump)' "$test_file" || true
printf '%s\n' '--- focused test diff ---'
git diff --find-renames HEAD^ HEAD -- "$test_file" || true
fi
printf '%s\n' '--- pytest and xdist configuration ---'
fd -t f -a '^(pyproject\.toml|pytest\.ini|setup\.cfg|tox\.ini|conftest\.py)$' . \
-x sh -c 'echo "--- $1 ---"; rg -n -C 3 "(xdist|pytest|addopts|testpaths)" "$1" || true' sh {} \;Length of output: 14049 🏁 Script executed: #!/bin/bash
set -u
printf '%s\n' '--- tracked paths matching the test name ---'
git ls-tree -r --name-only HEAD | rg '(^|/)test_real_processors\.py$' || true
printf '%s\n' '--- tracked paths containing the new test class ---'
git grep -n -I 'TestCacheDiskIOOffEventLoop' HEAD -- ':!docs/**' || true
test_path="$(git ls-tree -r --name-only HEAD | rg '(^|/)test_real_processors\.py$' | head -n 1)"
if [ -n "${test_path:-}" ]; then
printf '%s\n' "--- HEAD test content: $test_path ---"
git show "HEAD:$test_path" | nl -ba | sed -n '/class TestCacheDiskIOOffEventLoop/,/^class /p'
printf '%s\n' "--- parent-to-HEAD test diff: $test_path ---"
git diff --find-renames HEAD^ HEAD -- "$test_path" || true
fi
printf '%s\n' '--- all xdist configuration or invocation references ---'
rg -n -i -C 3 '(pytest-xdist|pytest.*-[nN]\b|-[nN]\s*(auto|[0-9]+)|xdist)' \
README.md pyproject.toml .github tests scripts 2>/dev/null || trueLength of output: 10221
✅ Action performedReview finished.
|
|
@linear Yes — please apply it. Two corrections to fold into the record before you write it, so the tracking entry does not overstate the fix:
Everything else in your tracking context matches the diff. Current status: governance, |
|
Done — I updated #1227 with the corrected tracking context and current validation status. |
![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/services/real_video_processor.py`:
- Around line 89-91: Update the cache-age validation in the cache lookup flow
around cache_age to reject non-finite values before applying the TTL comparison.
Use math.isfinite(cache_age), and return None for NaN or infinite ages while
preserving the existing expiration behavior for finite ages.
- Around line 80-82: Update _save_to_cache to serialize and flush JSON into a
temporary file located in cache_path.parent, then atomically publish it with
os.replace only after the write completes; ensure temporary files are cleaned up
on failure. Keep the full cache write sequence in one worker-thread hop, and
revise the nearby comment to avoid claiming that exists/stat/open make reads
atomic while preserving the off-event-loop guidance.
🪄 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: e40ff265-4c76-4365-8deb-e47258da3fc0
⛔ Files ignored due to path filters (1)
tests/unit/test_real_processors.pyis excluded by!tests/**
📒 Files selected for processing (1)
src/youtube_extension/backend/services/real_video_processor.py
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: Generate and Upload Coverage
⚠️ CI failures not shown inline (1)
GitHub Check: PR Governance: Canonical delivery contract blocked
Conclusion: failure
## Risk is missing or still contains only template placeholders; ## Production evidence is missing or still contains only template placeholders
🧰 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/services/real_video_processor.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/services/real_video_processor.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/services/real_video_processor.py
**/*.{py,js,ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Maintain >80% code coverage for new features
Files:
src/youtube_extension/backend/services/real_video_processor.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/services/real_video_processor.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/services/real_video_processor.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/services/real_video_processor.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/services/real_video_processor.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/services/real_video_processor.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/services/real_video_processor.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/services/real_video_processor.py
🪛 ast-grep (0.45.0)
src/youtube_extension/backend/services/real_video_processor.py
[warning] 92-92: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(cache_path, encoding='utf-8')
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
[warning] 101-101: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(cache_path, 'w', 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, Linear
Relevant review context
-
Primary risk: concurrent cache writes. The PR moves
open(..., "w")/json.dump()into worker threads but explicitly adds no locking.batch_process_videos()can run duplicate URLs concurrently, andprocess_video()ultimately calls_save_to_cache()for the same cache path. This can expose partial or invalid JSON to readers. The Copilot review flagged this, and related issue GRV-230 requires readers never observe partially written files. Consider an atomic temp-file +os.replace()write. -
TOCTOU wording is too strong. One
to_threadcall removes await points betweenexists()/stat()/open(), but those filesystem operations remain non-atomic. The existing broad exception handling converts read failures into cache misses, but the race is narrowed rather than closed. -
TTL edge case. The old and new predicates agree for finite ordered ages, including negative ages and the exact 24-hour boundary, but differ for
NaN: the old code treated it as stale, while the new code would accept it. -
Validation status. The PR reports 183 focused tests passing. At retrieval, lint/security checks were successful, while the main test and coverage jobs were still in progress.
🔇 Additional comments (2)
src/youtube_extension/backend/services/real_video_processor.py (2)
38-40: LGTM!Also applies to: 105-121
111-112: 🔒 Security & PrivacyNo cache-path traversal issue here.
extract_video_id()returns only captured 11-character IDs that exclude/and.., so the cache path is already restricted by the normal extraction path.
|
Adjudicating CodeRabbit's 1. NaN TTL divergence — if not math.isfinite(cache_age) or cache_age >= _CACHE_TTL_SECONDS:
return NoneEdge-case only (a real mtime won't be 2. Concurrent-write corruption — # write to a temp file in cache_path.parent, flush/close, then:
os.replace(tmp_path, cache_path) # atomic on POSIX + Windows; clean up tmp on failureNon-actionable / cleared: path-traversal flag is a false positive ( Disposition: I'm not pushing these from this out-of-band session — a commit here would trip Generated by Claude Code |
Review follow-ups on the cache off-loop change. Atomic publish: _write_cache_file opened the destination with 'w', which truncates it before json.dump completes, so a concurrent _load_from_cache could observe an empty or half-written entry. Serialize into a sibling temp file and os.replace it into position instead, so readers only ever see a complete entry. The temp file shares the cache directory to keep the rename on one filesystem, and is unlinked if serialization fails. TTL parity: the staleness guard had been rewritten as `cache_age >= TTL -> miss`. NaN compares False against both < and >=, so a non-finite mtime that the original `cache_age < TTL` guard treated as a miss would instead have been served as a hit. Restored the positive form so behaviour matches the original for every value, non-finite included. Because os.replace swaps the inode, staleness is now read via os.fstat on the already-open descriptor rather than a separate stat on the path, so the timestamp and the parsed bytes always describe the same inode. Adds three regression tests; all three fail against the previous implementation rather than erroring. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
@coderabbitai Both findings adopted in 1. Atomic publish (Major) — adopted as described. Confirmed. 2. Non-finite cache age (Minor) — adopted, and it was worse than "a non-finite file timestamp". You were right about the mechanism, and tracing it back made clear this was a behaviour change I introduced in the first commit rather than a latent edge case. The original guard was: if cache_age < 86400: # NaN -> False -> missand I had rewritten it as: if cache_age >= _CACHE_TTL_SECONDS: return None # NaN -> False -> hit
I went with restoring the positive form rather than adding a Interaction between the two — I also changed the reader, which neither comment asked for. These two findings are coupled, and adopting #1 alone would have introduced a new defect. An earlier thread on this PR proposed reading the mtime via
Net effect: no partial reads, and the timestamp and parsed bytes always describe the same inode. Verification Three regression tests added, all of which fail (not error) against the previous implementation:
The atomicity test asserts from inside a patched
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- current commit ---'
git rev-parse --short HEAD
printf '%s\n' '--- implementation outline ---'
ast-grep outline src/youtube_extension/backend/services/real_video_processor.py \
--items all --match 'RealVideoProcessor|_read_cache_file|_write_cache_file|_load_from_cache|_save_to_cache' || true
printf '%s\n' '--- cache implementation ---'
rg -n -C 18 '(_CACHE_TTL_SECONDS|def _read_cache_file|def _write_cache_file|os\.fstat|os\.replace|mkstemp|cache_age)' \
src/youtube_extension/backend/services/real_video_processor.py
printf '%s\n' '--- changes since f4b1ad591 parent ---'
git diff --find-renames f4b1ad591^ f4b1ad591 -- \
src/youtube_extension/backend/services/real_video_processor.py \
tests/unit/test_real_processors.py || true
printf '%s\n' '--- regression tests ---'
rg -n -C 22 '(test_load_from_cache_rejects_non_finite_age|test_save_to_cache_publishes_atomically|test_save_to_cache_leaves_no_temp_file_on_failure)' \
tests/unit/test_real_processors.pyLength of output: 16521
Both findings are addressed.
The final standalone test-path lookup failed because that exact path was absent in the checkout. The commit diff contained the full test changes, so this did not block verification. ✏️ Learnings added
You are interacting with an AI system. |
|
@coderabbitai review Both findings from your review are addressed in
If you consider these resolved, please update the review state so the PR is unblocked. |
|
Independently verified
No new issues introduced (corrupt-entry-degrades-to-miss and negative/future-dated-age behavior are both preserved). From my side the two findings I raised are fully resolved. One note on merge state: the PR currently shows Generated by Claude Code |
|
✅ Action performedReview finished.
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- HEAD ---'
git rev-parse HEAD
git status --short
printf '%s\n' '--- cache helper outline ---'
ast-grep outline src/youtube_extension/backend/services/real_video_processor.py \
--items all \
--match 'RealVideoProcessor|_read_cache_file|_write_cache_file|_load_from_cache|_save_to_cache'
printf '%s\n' '--- cache helper implementation ---'
rg -n -C 20 \
'(^import (os|tempfile)|^from .* import .*tempfile|_CACHE_TTL_SECONDS|def _read_cache_file|def _write_cache_file|os\.fstat|os\.replace|tempfile\.mkstemp|cache_age <|cache_age >=|unlink)' \
src/youtube_extension/backend/services/real_video_processor.py
printf '%s\n' '--- focused implementation diff ---'
git diff --find-renames f4b1ad591^ f4b1ad591 -- \
src/youtube_extension/backend/services/real_video_processor.py
printf '%s\n' '--- focused regression tests ---'
rg -n -C 22 \
'(test_load_from_cache_rejects_non_finite_age|test_save_to_cache_publishes_atomically|test_save_to_cache_leaves_no_temp_file_on_failure|mkstemp|os\.replace|fstat)' \
tests/unit/test_real_processors.py
printf '%s\n' '--- focused test diff ---'
git diff --find-renames f4b1ad591^ f4b1ad591 -- \
tests/unit/test_real_processors.pyLength of output: 11937 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- cache-load error handling ---'
sed -n '132,190p' src/youtube_extension/backend/services/real_video_processor.py
printf '%s\n' '--- candidate test files ---'
fd -t f -i 'test_real_processors.py' . || true
printf '%s\n' '--- tracked regression-test definitions ---'
git grep -n -I -E \
'def test_(load_from_cache_rejects_non_finite_age|save_to_cache_publishes_atomically|save_to_cache_leaves_no_temp_file_on_failure)' \
HEAD -- ':!docs/**' || true
test_path="$(
git grep -l -I -E \
'def test_(load_from_cache_rejects_non_finite_age|save_to_cache_publishes_atomically|save_to_cache_leaves_no_temp_file_on_failure)' \
HEAD -- ':!docs/**' | head -n 1 || true
)"
if [ -n "$test_path" ]; then
printf '%s\n' "--- regression test source: $test_path ---"
git show "HEAD:$test_path" | nl -ba | sed -n \
'/def test_load_from_cache_rejects_non_finite_age/,/def test_save_to_cache_leaves_no_temp_file_on_failure/p'
fi
printf '%s\n' '--- test changes in f4b1ad591 ---'
git diff --find-renames f4b1ad591^ f4b1ad591 -- \
':!src/youtube_extension/backend/services/real_video_processor.py'Length of output: 4180 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- regression test source ---'
git show HEAD:tests/unit/test_real_processors.py | sed -n '1200,1345p'
printf '%s\n' '--- regression test diff ---'
git diff --find-renames f4b1ad591^ f4b1ad591 -- tests/unit/test_real_processors.py
printf '%s\n' '--- cache helper diff, including surrounding call sites ---'
git diff --find-renames f4b1ad591^ f4b1ad591 -- \
src/youtube_extension/backend/services/real_video_processor.pyLength of output: 15045
Both findings are resolved.
I found no remaining blocking defect in this scope. [resolve] You are interacting with an AI system. |
|
Publish gate — All review findings are addressed in head
CI is green on This run does not auto-merge to a protected branch. Staged command for a human to merge once approved: Generated by Claude Code |
* perf: scan processed-video cache off the event loop 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> * style: Black-format _collect_processed_videos_sync helper 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 * test: prove per-file cache read is off the event loop 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> --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Co-authored-by: Claude <noreply@anthropic.com>
Head:
f4b1ad59106df561abd7dce576374adc69ba104cCanonical issue
Closes #1227
Outcome
_load_from_cache/_save_to_cachedisk + JSON work off the event loop viaasyncio.to_threadexists→stat→open→json.loadinto a single off-loop hop, narrowing the TOCTOU windowexceptthat degrades a corrupt entry to a cache missEnhancedVideoProcessoror any other processorScope
Two files:
src/youtube_extension/backend/services/real_video_processor.py— extract_read_cache_file/_write_cache_filestatic helpers;await asyncio.to_thread(...)from both async methods; hoist the magic86400to_CACHE_TTL_SECONDS.tests/unit/test_real_processors.py— addTestCacheDiskIOOffEventLoop(4 tests). No pre-existing test was modified.Design notes
Why
asyncio.to_threadand notaiofiles. This matches the pattern already merged in this repo for the same class of defect (#1205,aws_rekognition.py), and adds no dependency. The work is a short CPU+syscall burst, not a long stream, so a worker-thread hop is the right granularity.Why one helper instead of offloading each call. Offloading the stat and the parse as separate
to_threadhops would pay multiple context switches and leave a TOCTOU window open between them._read_cache_filedoes the whole read-and-validate in one hop and returnsNonefor "treat as miss", so the async method stays a thin coordinator. Itopens first and takes the age fromos.fstaton that descriptor, so the timestamp and the parsed bytes are guaranteed to describe the same inode.TTL boundary is unchanged — after a correction. The original guard was
if cache_age < 86400, so an entry aged exactly 86400s was already a miss.test_load_from_cache_treats_exact_ttl_as_stalepins that and passes against both the old and new code.An earlier revision of this PR expressed the guard as its negation,
if cache_age >= _CACHE_TTL_SECONDS: return None, and this description claimed that was equivalent. That claim was wrong, and CodeRabbit caught it.NaNcomparesFalseagainst both<and>=, so a non-finitemtimethat the original guard treated as a miss would have been served as a hit. Fixed inf4b1ad591by restoring the positive form (if cache_age < _CACHE_TTL_SECONDS: return payload, cache_age), which is equivalent to the original by construction for every input rather than by case analysis.test_load_from_cache_rejects_non_finite_agepins it and fails against the earlier revision.Writes publish atomically. Also from review: serializing straight into the destination with
open(path, 'w')truncates it up front, so a concurrent reader could observe an empty or half-written entry._write_cache_filenow writes to a sibling temp file in the same directory andos.replaces it into position, unlinking the temp file if serialization raises. Becauseos.replaceswaps the inode on every write, the reader'sos.fstat-on-open (above) is load-bearing rather than cosmetic: a path-basedstatcould otherwise resolve to a different inode than the subsequent read.Why thread identity, not timing. The tests wrap the module's
jsonbinding in a proxy that recordsthreading.get_ident()duringload/dump, then assert the event-loop thread id is absent from the recorded set. This is a direct observation of the property under test and is immune to CI scheduling noise. The proxy is installed withpatch.dicton the function's__globals__rather than on a re-imported module object, because sibling test modules rebindyoutube_extension.*entries insys.modules, which makes module-object patching unreliable in a full-suite run.Risk
Low. Cache semantics, the TTL boundary, the on-disk JSON format, and the corrupt-entry-degrades-to-miss path are all unchanged (see the Outcome table's "Does not" column and the TTL design note). No new dependency is introduced; the pattern mirrors the already-merged #1205 fix. No public API, signature, or return contract of
_load_from_cache/_save_to_cachechanges. Rollback is a single-commit revert with no data-migration or format implications.Two behavioral deltas are deliberate, and both narrow existing failure modes rather than widening them:
asyncio.to_thread, removing a blocking-I/O stall from the loop. Collapsing the read-and-validate into a single off-loop hop also narrows, rather than widens, the pre-existing TOCTOU window.os.replace), so readers can no longer observe a truncated entry mid-write. Previously this window was reachable on every concurrent write.Honest caveat on how that second item was reached: the first revision of this PR was described as strictly behavior-preserving, and it was not — negating the TTL predicate flipped non-finite ages from miss to hit (detailed under Design notes). That was found in review, not by the tests I shipped, and the gap is now closed by a regression test that fails against the earlier revision. The current head is behavior-preserving on the TTL guard for all inputs including non-finite ones.
Production evidence
This is a Python-only backend change and is not exercised by the Vercel Next.js preview, which builds the
apps/webroot; the exact-head Vercel deployment reports READY and commit-verified, which proves web compatibility, not Python runtime behavior. Runtime behavior is evidenced by the test suite on the exact head:Pre-change fail-test — with only the source file reverted and the new tests kept, every new test fails (none error), confirming they exercise the real defects rather than passing vacuously:
The last three are the review follow-ups: against the pre-fix source they fail with, respectively, a cache hit for a non-finite age, an empty destination observed mid-
dump, and a partial file left behind after a serialization error.ruff checkandruff format --checkoutput for both touched files is byte-identical tomain(7 pre-existing findings before and after; no new findings introduced).Verification
Agent handoff
ruffat parity withmainf4b1ad59106df561abd7dce576374adc69ba104c