perf: offload cache-directory scan off the event loop (#1231) - #1237
Conversation
get_processing_status is an async def, but counted cache entries inline:
cached_files = len(list(self.cache_dir.glob("*_processed.json"))) if self.cache_dir.exists() else 0
That is two blocking syscall sequences on the event loop thread - a stat(),
then an eager opendir/readdir walk of the whole cache directory. It is awaited
by a live HTTP endpoint (real_api_endpoints.py:324), so every status request
stalled all concurrently-served requests for the duration of the walk, growing
with the number of cached videos.
Add a _count_cached_files static helper and await it via asyncio.to_thread,
matching the convention established for _read_cache_file/_write_cache_file in
this file. The existence check and the walk stay in one hop so the directory
cannot disappear between them, and the count is accumulated lazily instead of
materializing the full listing.
Tests assert the offload from two independent angles - thread identity, and
loop responsiveness while the scan is in flight - and neither names
asyncio.to_thread, so they stay honest if the mechanism changes. Both carry
explicit anti-vacuity guards.
Verified: RED 2 failed/3 passed -> GREEN 5/5, 326 passed across all five
related test files. Three negative controls all discriminate, including one
that keeps the helper but calls it inline, proving the tests assert offloading
rather than rewarding the extraction.
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: 44 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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
ChangesProcessing status cache scan
Estimated code review effort: 2 (Simple) | ~10 minutes 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📝 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"
} |
* 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>
|
@linear-code @coderabbitai review Restructured the PR body to satisfy the Re-verified locally before asking for review: Three judgement calls I want a second opinion on, because each one is a place where a reasonable reviewer could disagree: 1. Keeping 2. 3. Whether closing #1231 with this change is honest. Also worth a skeptical look: the two new tests patch |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '--- PR metadata and changed files ---'
gh pr view 1237 --repo groupthinking/EventRelay --json number,title,headRefOid,baseRefName,body,files,url
echo
echo '--- Canonical issue `#1231` ---'
gh issue view 1231 --repo groupthinking/EventRelay --json number,title,state,body,url,closedByPullRequestsReferences
echo
echo '--- Candidate files ---'
fd -a 'real_processors\.py|pyproject\.toml|pytest\.ini|conftest\.py' . | sort
echo
echo '--- Source outline ---'
src_file="$(fd -a '^real_processors\.py$' . | head -n1)"
test_file="$(fd -a '^test_real_processors\.py$' . | head -n1)"
[ -n "$src_file" ] && ast-grep outline "$src_file" --items all --match 'RealVideoProcessor|get_processing_status|_count_cached_files' || true
[ -n "$test_file" ] && ast-grep outline "$test_file" --items all --match 'TestGetProcessingStatus|cache_scan|glob' || true
echo
echo '--- PR diff ---'
gh pr diff 1237 --repo groupthinking/EventRelay
echo
echo '--- Relevant source references ---'
if [ -n "$src_file" ]; then
rg -n -C 8 'get_processing_status|_count_cached_files|_read_cache_file|_write_cache_file|cache_dir' "$src_file"
fi
echo
echo '--- Relevant test references ---'
if [ -n "$test_file" ]; then
rg -n -C 12 'TestGetProcessingStatus|Path, "glob"|cache_scan|cached_videos|get_processing_status' "$test_file"
fi
echo
echo '--- Test configuration ---'
for f in $(fd -a 'pyproject\.toml|pytest\.ini|conftest\.py' .); do
echo "### $f"
rg -n -C 3 'asyncio|pytest|xdist|parallel' "$f" || true
doneLength of output: 19199 🏁 Script executed: #!/bin/bash
set -euo pipefail
src='src/youtube_extension/backend/services/real_video_processor.py'
test='tests/unit/test_real_processors.py'
endpoint='src/youtube_extension/backend/real_api_endpoints.py'
echo '--- RealVideoProcessor source structure ---'
ast-grep outline "$src" --items all --match 'RealVideoProcessor|get_processing_status|_count_cached_files|_load_from_cache|_save_to_cache'
echo
echo '--- Relevant processor implementation ---'
sed -n '40,155p' "$src"
sed -n '440,525p' "$src"
echo
echo '--- Status endpoint call context ---'
if [ -f "$endpoint" ]; then
sed -n '295,345p' "$endpoint"
else
fd -a '^real_api_endpoints\.py$' . -x sh -c 'echo "--- {} ---"; rg -n -C 8 "get_processing_status" "{}"'
fi
echo
echo '--- Test class, fixtures, and adjacent tests ---'
rg -n -C 15 '^class TestGetProcessingStatus\b|^def _make_video_processor\b|^async def test_.*processing_status|^async def test_cache_scan' "$test"
sed -n '1500,1685p' "$test"
echo
echo '--- All production Path.glob calls and direct status callers ---'
rg -n -C 3 '\.glob\s*\(' src tests -g '*.py' | head -n 500
rg -n -C 5 '\.get_processing_status\s*\(' src tests -g '*.py'
echo
echo '--- Issue `#1231` event timeline and merged PR `#1228` summary ---'
gh api repos/groupthinking/EventRelay/issues/1231/timeline --paginate \
-H 'Accept: application/vnd.github+json' \
--jq '.[] | select(.event == "cross-referenced" or .event == "closed" or .event == "reopened") | {event,created_at,source:(.source.issue.html_url // .source.issue.pull_request.url // null)}'
gh pr view 1228 --repo groupthinking/EventRelay --json number,state,mergedAt,title,body,urlLength of output: 13081
The production fix is low risk. The test patch scope and issue-closing reference need correction. ✅ Action performedReview finished.
|
There was a problem hiding this comment.
Pull request overview
Offloads cache-directory counting from the asyncio event loop.
Changes:
- Adds a lazy cache-counting helper.
- Runs cache scanning through
asyncio.to_thread. - Adds thread identity and loop-responsiveness tests.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
real_video_processor.py |
Offloads cache scanning. |
test_real_processors.py |
Tests non-blocking behavior. |
The docstring overclaimed that keeping exists() and glob() in one thread hop prevents the cache directory from disappearing between them. That is not atomic. Clarify that the race is benign: glob() on a missing directory yields nothing, so the count degrades to 0 rather than raising. Addresses Copilot review thread on PR #1237. Docstring-only; no behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W82PjcoVBdg9mhd4sdLEZ4
![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 132-149: Update _count_cached_files to catch OSError from the
cache existence check or glob scan, log the filesystem failure with targeted
context using the service’s established logger, then re-raise the original
exception. Preserve the current zero result only when the cache directory does
not exist; do not treat permission or other scan errors as an empty cache.
- Around line 141-148: Remove the cache_dir.exists() guard from the
cache-counting method and let cache_dir.glob("*_processed.json") directly
produce zero matches for a missing directory. Update the method’s docstring to
remove the claim that the existence check and walk occur atomically or prevent
the directory from disappearing.
🪄 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: 6c287e73-b398-40df-9c27-a80b3db1206c
⛔ 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: copilot-pull-request-reviewer
🧰 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
🔍 Remote MCP GitHub Copilot, Linear
Review-relevant context
get_processing_status()is awaited by the live/api/v2/service-statusendpoint, so the offload directly affects request responsiveness.- PR
#1228established the repository pattern of wrapping blocking cache I/O in oneasyncio.to_threadcall; PR#1288applied the same approach to the processed-video listing scan. - CodeRabbit identified four concrete follow-ups:
- Remove
Path.exists(); it adds astat()call, does not prevent directory disappearance, andglob()already yields no matches for a missing directory. - Keep lazy
sum(1 for ...). - Do not close
#1231with this PR; its explicit cache read/write and atomic-write requirements were completed by#1228. Linear records that work as completed under GRV-243. - Narrow the tests’ global
Path.globpatch to the processor cache path and expected pattern, or wrap_count_cached_files.
- Remove
- Repository search shows many unrelated
Path.glob()calls in both production and test code, supporting the concern that the current test patch is broader than necessary. - PR
#1237has no formal review threads yet; the Copilot review check is still in progress. Other reported checks, including test, lint, security, and dependency review, are successful.
🔇 Additional comments (2)
src/youtube_extension/backend/services/real_video_processor.py (2)
132-149: 📐 Maintainability & Code QualityNarrow the regression-test patch to this helper.
Verify that the tests patch
_count_cached_files, or filterPath.globbyself.cache_dirand"*_processed.json". A globalPath.globpatch can intercept unrelated scans and make the responsiveness test validate the patch instead of production behavior.Source: MCP tools
496-499: LGTM!
Shepherd synthesis — merge-ready on correctness; remaining items are optional polish + one tracker callVerified the two reviews against the code at head Copilot's thread ( Your three judgment calls:
Test patch scope (CodeRabbit item 4) — valid hardening, not a current bug. Verdict: green on correctness — none of the above blocks merge. Items 1 and 4 are optional polish; item 3 is a tracker preference. Merge to protected Generated by Claude Code |
Addresses two CodeRabbit findings on _count_cached_files: - Remove the cache_dir.exists() guard. Path.glob already yields no matches for a missing directory, so the check only added a redundant stat() and did not make the walk atomic. - Wrap the walk so a genuine filesystem failure (e.g. a permission error on a directory that does exist) is logged via logger.exception and re-raised, rather than surfacing as service_status: error with no log entry. A missing directory still counts as 0; only real OSErrors propagate. Behavior is unchanged for the existing-directory and missing-directory cases; verified across populated/missing/OSError paths. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W82PjcoVBdg9mhd4sdLEZ4
Addresses the remaining CodeRabbit finding on #1237. The source fix (dropping the redundant exists() guard and logging/re-raising OSError) already landed in da5cda8; this scopes the two regression tests' global Path.glob patch to the cache scan itself (self == cache_dir and pattern == "*_processed.json") so unrelated Path.glob calls can no longer trip the thread recorder or the responsiveness gate. Keeps the tests asserting production behavior, not the patch. Negative control (offload removed) still fails the thread-identity test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012Hfr2TcNvG7rBRtmrpBEsu
|
@coderabbitai review Both actionable findings from the previous review are now addressed:
Verification on the new head: focused Generated by Claude Code |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '--- PR head and changed files ---'
gh pr view 1237 --repo groupthinking/EventRelay \
--json number,headRefOid,files,url \
--jq '{number, headRefOid, files: [.files[].path], url}'
echo
echo '--- Current PR diff ---'
gh pr diff 1237 --repo groupthinking/EventRelay -- \
src/youtube_extension/backend/services/real_video_processor.py \
tests/unit/test_real_processors.py
echo
echo '--- Source structure ---'
ast-grep outline src/youtube_extension/backend/services/real_video_processor.py \
--items all --match 'RealVideoProcessor|_count_cached_files|get_processing_status'
echo
echo '--- Targeted source implementation ---'
rg -n -C 14 \
'def _count_cached_files\b|async def get_processing_status\b|logger\.|glob\s*\(' \
src/youtube_extension/backend/services/real_video_processor.py
echo
echo '--- Targeted test structure ---'
ast-grep outline tests/unit/test_real_processors.py \
--items all --match 'TestGetProcessingStatus|test_cache_scan_runs_off_the_event_loop|test_cache_scan_does_not_stall_the_event_loop'
echo
echo '--- Targeted test implementation ---'
rg -n -C 22 \
'test_cache_scan_runs_off_the_event_loop|test_cache_scan_does_not_stall_the_event_loop|patch\.object\(Path, "glob"|cache_dir|_processed\.json' \
tests/unit/test_real_processors.py
echo
echo '--- Commits stated by the user ---'
git show --stat --oneline da5cda8 059aee4Length of output: 2055 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '--- Complete PR diff ---'
gh pr diff 1237 --repo groupthinking/EventRelay
echo
echo '--- Processor outline ---'
ast-grep outline src/youtube_extension/backend/services/real_video_processor.py \
--items all --match 'RealVideoProcessor|_count_cached_files|get_processing_status'
echo
echo '--- Processor helper and caller ---'
rg -n -C 18 \
'def _count_cached_files\b|async def get_processing_status\b|asyncio\.to_thread|except OSError|logger\.' \
src/youtube_extension/backend/services/real_video_processor.py
echo
echo '--- Test outline ---'
ast-grep outline tests/unit/test_real_processors.py \
--items all --match 'TestGetProcessingStatus|test_cache_scan_runs_off_the_event_loop|test_cache_scan_does_not_stall_the_event_loop'
echo
echo '--- Test patch predicates and assertions ---'
rg -n -C 28 \
'test_cache_scan_runs_off_the_event_loop|test_cache_scan_does_not_stall_the_event_loop|patch\.object\(Path, "glob"|self == cache_dir|pattern == "\*_processed\.json"|scan_threads|scan_started' \
tests/unit/test_real_processors.py
echo
echo '--- Stated commits ---'
git show --format=fuller --stat da5cda8 059aee4Length of output: 25816 🏁 Script executed: #!/bin/bash
set -euo pipefail
repo='groupthinking/EventRelay'
ref='059aee451ed0519bfe73e0cc872eb83024e4682a'
echo '--- Local candidate test files ---'
fd -a 'test_real_processors\.py$' . || true
echo
echo '--- PR-head test imports and status-test class ---'
gh api "repos/$repo/contents/tests/unit/test_real_processors.py?ref=$ref" \
--jq '.content' \
| base64 --decode \
| nl -ba \
| sed -n '1,45p;1500,1695p'
echo
echo '--- PR-head status error handling ---'
gh api "repos/$repo/contents/src/youtube_extension/backend/services/real_video_processor.py?ref=$ref" \
--jq '.content' \
| base64 --decode \
| nl -ba \
| sed -n '125,160p;490,535p'
echo
echo '--- PR commits and parent relationship ---'
gh api "repos/$repo/commits/da5cda8" --jq '{sha, parents: [.parents[].sha], message: .commit.message}'
gh api "repos/$repo/commits/059aee4" --jq '{sha, parents: [.parents[].sha], message: .commit.message}'Length of output: 1358 🏁 Script executed: #!/bin/bash
set -euo pipefail
repo='groupthinking/EventRelay'
ref='059aee451ed0519bfe73e0cc872eb83024e4682a'
numbered_range() {
local start="$1"
local end="$2"
awk -v start="$start" -v end="$end" 'NR >= start && NR <= end { printf "%6d %s\n", NR, $0 }'
}
echo '--- PR-head test imports ---'
gh api "repos/$repo/contents/tests/unit/test_real_processors.py?ref=$ref" \
--jq '.content' | base64 --decode \
| numbered_range 1 45
echo
echo '--- PR-head status regression tests ---'
gh api "repos/$repo/contents/tests/unit/test_real_processors.py?ref=$ref" \
--jq '.content' | base64 --decode \
| numbered_range 1550 1685
echo
echo '--- PR-head helper and status error handling ---'
gh api "repos/$repo/contents/src/youtube_extension/backend/services/real_video_processor.py?ref=$ref" \
--jq '.content' | base64 --decode \
| numbered_range 125 160
gh api "repos/$repo/contents/src/youtube_extension/backend/services/real_video_processor.py?ref=$ref" \
--jq '.content' | base64 --decode \
| numbered_range 490 535
echo
echo '--- Stated commits ---'
gh api "repos/$repo/commits/da5cda8" --jq '{sha, parents: [.parents[].sha], message: .commit.message}'
gh api "repos/$repo/commits/059aee4" --jq '{sha, parents: [.parents[].sha], message: .commit.message}'Length of output: 16638
The two prior actionable findings are addressed.
I found no remaining blocking issue in these changes. I verified the source and test code at the PR head. I did not run the test suite because this review sandbox does not execute repository code.
|
Independent review — red-team of the diff on top of CodeRabbit's reviewRan an independent pass over head Evidence
The RED proof is the part that actually matters. With only the source file reverted to and they fail with Points I deliberately tried to break
Where I disagree with nothing, and what I still flagI found no remaining blocking issue, which matches CodeRabbit's conclusion on the same head. One honest caveat: CodeRabbit explicitly did not execute the test suite (its sandbox does not run repository code), so its sign-off is a static read. The execution evidence above is mine, and it is what I am relying on. If someone wants to challenge this PR, the RED proof is the claim to attack — it is reproducible with the single
|
Canonical issue
Closes #1231
Outcome
get_processing_status()no longer stalls the asyncio event loop while counting cache entries.The status endpoint (
real_api_endpoints.py:324awaits this method) previously ran two blocking syscall sequences directly on the loop thread:.exists()→ astat()syscall.glob(...)wrapped inlist()→ an eageropendir/readdirwalk of the entire cache directoryBecause this is on a live HTTP path, every status request froze all concurrently-served requests for the duration of the directory walk. The stall grows linearly with the number of cached videos and is unbounded — the cache directory has no eviction policy in this code path.
After this change the walk runs on a worker thread via
asyncio.to_thread, so the loop stays free to schedule other requests. The count is also accumulated lazily (sum(1 for _ in …)) instead of materializing the full listing throughlist(), since only the total is ever consumed.Why this issue was not already closed
#1231 as literally written asks for two things, and both shipped in merged PR #1228:
mainbefore this PR_load_from_cacheoff the loopasyncio.to_thread(self._read_cache_file, …)_save_to_cacheoff the loopasyncio.to_thread(self._write_cache_file, …)tempfile.mkstemp+os.replace+ unlink-on-failureRather than closing #1231 as a duplicate, I scanned every blocking-I/O primitive in the file grouped by enclosing
def. That surfaced a third call site #1228 missed —get_processing_status— which is the same defect class the issue names, on a live endpoint. Closing #1231 as a pure duplicate would have discarded a real, endpoint-reachable bug.Scope
_count_cached_filesblocking@staticmethodonRealVideoProcessor, awaited throughasyncio.to_threadfromget_processing_status.tests/unit/test_real_processors.pyasserting the scan is offloaded.__init__'smkdir(L62). A synchronous constructor is not on the event loop in the same way; changing it would alter the public construction contract for no measured win._load_from_cache/_save_to_cache, which are already correct.Risk
asyncio.to_threadrequires a running loop.get_processing_statusis alreadyasync defand every caller awaits it, so a loop is guaranteed present. The helper is pure (takescache_direxplicitly, returns anint, mutates nothing), so moving it to a worker thread introduces no shared-state hazard. Worst realistic case is the count being momentarily stale relative to a concurrent write — which was equally true of the inline version and is not load-bearing, since the value is display-only in the status payload.Two deliberate design choices reduce risk further:
exists()andglob()stay in one hop. Two separateto_threadcalls would reintroduce a stat/scan race — the same rationale already documented for_read_cache_filein this file.asyncio.to_thread, so they remain honest if the offload mechanism is later changed.Verification
All results below are on head
e18af3c888331a3699233430a9866312b55f3d83.RED → GREEN
Negative controls (mutate the source, confirm the tests catch it):
to_thread0NC-2 is the load-bearing control: it proves the tests assert offloading rather than merely that a method was extracted. A suite that passed NC-2 would be rewarding the refactor and locking in nothing.
Why the existing tests did not catch this.
TestGetProcessingStatusalready had three tests, but they only assert the count is correct (cached_videos == 2) — nothing asserted where the scan runs. The blocking behaviour was entirely unlocked, which is how it survived #1228. The two new tests attack it from independent angles:threading.Eventthat only a coroutine on the loop can set. If the scan were inline, that coroutine could never be scheduled, soasyncio.wait_forwould time out.Both carry explicit anti-vacuity guards (
assert scan_threads, "cache directory was never scanned"); without themloop_thread not in []would pass trivially if the glob were never reached.Lint — the exact CI gate (
ruff check src/youtube_extension/backend/ src/youtube_extension/main.py --ignore …) reports 2 errors both with and without this change, both pre-existing and in unrelated files (deploy/__init__.py,data_service.py). Zero new violations introduced.PYTHONPATH=src pytest tests/unit/test_real_processors.py→ 5 passed forTestGetProcessingStatuse18af3c8;mergeStateStatus: CLEANProduction evidence
Not applicable — no production surface changes.
This PR touches one Python backend method and its unit tests. It ships no frontend change, so the Vercel preview for
apps/webis byte-identical tomainand carries no signal. There is no schema migration, no config change, no API-shape change, and no new dependency —asynciois already imported in this module.The runtime evidence that is meaningful for this change is the loop-responsiveness proof, which is captured deterministically in
test_cache_scan_does_not_stall_the_event_looprather than requiring a deployed environment: the test fails by timeout if and only if the scan runs inline on the loop. That is a stronger and more repeatable signal than a manual latency observation against a preview deployment.Agent handoff