perf: offload video-detail cache read, drop the stat probe - #1304
Conversation
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>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe video-analysis endpoint now reads and parses one cache file in a worker thread. A sentinel distinguishes missing files from cached JSON ChangesVideo analysis cache access
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 6 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (6 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"
} |
|
@linear Third in the blocking-I/O series, after #1288 (list endpoint) and #1298/#1301 (L1 cache bookkeeping). This one moves the single-video cache read off the loop. It is the smallest of the three, but it carries one judgement call I would rather you shoot down now than after it ships. Three judgement calls, weakest first.
One thing I did not change: Verification: 97 passed (88 pre-existing + 9 new). Prove-fail: reverting only the Good to merge, or do you want the duplicated helper pulled back out in favour of parameterising @coderabbitai review |
|
Good to merge. I’d keep the duplicated helper and not parameterize
I only have one style-level suggestion: if you want to make the duplication even harder to “clean up” later, add one short sentence in the docstring of _read_video_analysis_sync saying it intentionally diverges from the TTL semantics in real_video_processor.py. Otherwise, I wouldn’t block this. |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== PR metadata and diff ==='
gh pr view 1304 --repo groupthinking/EventRelay --json number,title,state,baseRefName,headRefName,files --jq '{number,title,state,baseRefName,headRefName,files:[.files[].path]}'
gh pr diff 1304 --repo groupthinking/EventRelay -- src/youtube_extension/backend/real_api_endpoints.py tests/unit/test_real_api_endpoints.py
echo '=== Source outline ==='
ast-grep outline src/youtube_extension/backend/real_api_endpoints.py --items all --view expanded
echo '=== Processor outline ==='
processor_file="$(fd -t f '^real_video_processor\.py$' src | head -n 1)"
test_file="$(fd -t f '^test_real_api_endpoints\.py$' tests | head -n 1)"
printf 'processor=%s\ntest=%s\n' "$processor_file" "$test_file"
ast-grep outline "$processor_file" --items all --match 'RealVideoProcessor|_read_cache_file' --view expanded
echo '=== Relevant source symbols ==='
rg -n -A45 -B12 'def _read_video_analysis_sync|async def get_video_analysis|def get_video_analysis' src/youtube_extension/backend/real_api_endpoints.py
echo '=== Existing processor helper ==='
rg -n -A45 -B12 'def _read_cache_file' "$processor_file"
echo '=== New test symbols ==='
rg -n -A50 -B10 'class _SlowReadPath|class TestVideoDetailOffloadsBlockingIO|class TestReadVideoAnalysisSync|class TestVideoDetailIgnoresProcessorCacheTtl' "$test_file"Length of output: 1993 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== PR diff ==='
gh api \
-H 'Accept: application/vnd.github.v3.diff' \
/repos/groupthinking/EventRelay/pulls/1304
processor_file="$(fd -t f '^real_video_processor\.py$' src | head -n 1)"
test_file="$(fd -t f '^test_real_api_endpoints\.py$' tests | head -n 1)"
printf '\n=== Located files ===\nprocessor=%s\ntest=%s\n' "$processor_file" "$test_file"
echo '=== Endpoint symbols ==='
ast-grep outline src/youtube_extension/backend/real_api_endpoints.py --items all --view expanded
rg -n -A55 -B15 'def _read_video_analysis_sync|async def get_video_analysis|def get_video_analysis' \
src/youtube_extension/backend/real_api_endpoints.py
echo '=== Existing processor helper ==='
ast-grep outline "$processor_file" --items all --match 'RealVideoProcessor|_read_cache_file' --view expanded
rg -n -A45 -B12 'def _read_cache_file' "$processor_file"
echo '=== Added test symbols ==='
rg -n -A65 -B12 'class _SlowReadPath|class TestVideoDetailOffloadsBlockingIO|class TestReadVideoAnalysisSync|class TestVideoDetailIgnoresProcessorCacheTtl' \
"$test_file"Length of output: 14464 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Endpoint imports and handler context ==='
sed -n '1,125p' src/youtube_extension/backend/real_api_endpoints.py
sed -n '245,340p' src/youtube_extension/backend/real_api_endpoints.py
processor_file="$(fd -t f '^real_video_processor\.py$' src | head -n 1)"
printf '\n=== Processor cache path and cache file contracts: %s ===\n' "$processor_file"
rg -n -A42 -B12 'def _get_cache_path|def _read_cache_file|json\.(dump|load)|open\(.*cache' \
"$processor_file"
echo '=== Cache path call sites and endpoint payload assumptions ==='
rg -n -C4 '_get_cache_path\s*\(' src
rg -n -C4 '_read_video_analysis_sync|video_data is None|return video_data' \
src/youtube_extension/backend/real_api_endpoints.pyLength of output: 19854
Keep the duplicated Keep Keep the heartbeat result. It demonstrates event-loop availability without presenting synthetic timing as production latency data. The thread-identity test and prove-fail result make the offload requirement non-vacuous. This is ready to merge from the reviewed implementation. ✏️ Learnings added
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Pull request overview
Offloads single-video cache reads from the event loop and removes the check-then-open race.
Changes:
- Adds a synchronous cache reader for worker-thread execution.
- Adds regression tests for offloading, errors, and TTL behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/youtube_extension/backend/real_api_endpoints.py |
Offloads cache reading and parsing. |
tests/unit/test_real_api_endpoints.py |
Adds cache-read behavior and performance tests. |
Independent review — head
|
![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/real_api_endpoints.py`:
- Around line 111-117: Update processor._get_cache_path, used by
get_video_analysis, to reject traversal-capable video_id values or resolve the
generated cache path and verify it remains under self.cache_dir before opening
it. Preserve valid cache lookups, and add coverage for encoded forward-slash and
backslash separators used in traversal attempts.
- Around line 88-117: Update _read_video_analysis_sync and get_video_analysis to
distinguish a missing cache file from a successfully parsed JSON null value,
using a distinct found/payload result or equivalent sentinel. Have
get_video_analysis raise 404 only when the cache entry is absent, while
preserving a 200 response with null for valid null content, and add a regression
test covering a cache file containing null.
🪄 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: 687f5424-c92d-4f73-979a-46501b59d484
⛔ 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. (3)
- GitHub Check: Generate and Upload Coverage
- GitHub Check: test
- GitHub Check: trivy
🧰 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] 111-111: 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)
🔍 Remote MCP GitHub Copilot
Additional review context
- PR
#1304implements issue#1303’s requested off-loop read and directopen()/FileNotFoundErrorhandling. The existing processor helper has a 24-hour TTL, so keeping a separate helper avoids changing this endpoint’s age-independent behavior. - Blocking correctness issue found by Copilot: valid JSON
nullis now treated as a cache miss because bothFileNotFoundErrorandjson.load()returningNoneproduceNone; the endpoint therefore returns 404 instead of the previous 200null. Use a unique miss sentinel or(found, payload), and add a regression test. - The PR claims 97 focused tests pass, but the retrieved combined status is still pending; test, coverage, and Trivy checks were still in progress at retrieval time.
🔇 Additional comments (1)
src/youtube_extension/backend/real_api_endpoints.py (1)
303-309: LGTM!
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>
|
Both findings actioned in 1.
|
| version | entry whose content is null |
absent entry (control) |
|---|---|---|
main (pre-branch) |
200, body null |
404 |
5aa56cf71 (what you reviewed) |
404 ✗ | 404 |
7eefa9aae (fixed) |
200, body null ✅ |
404 ✅ |
Fix. A module-level sentinel rather than a tri-state or an exception:
# Distinguishes "no cache entry" from an entry that parses to ``None`` -- which is
# what a file holding the JSON literal ``null`` yields. A plain ``None`` return
# conflates the two and turns a stored ``null`` analysis into a 404.
_CACHE_MISS: Final = object()The helper returns _CACHE_MISS on a miss and its return type widened from Optional[dict] to Any; the handler tests if video_data is _CACHE_MISS — identity, not truthiness and not is None, because None is now a legitimate payload.
Test coverage (restating in prose since tests/** is outside your review scope):
test_missing_file_returns_the_miss_sentinel— assertsis _CACHE_MISS, notis Nonetest_null_content_is_a_payload_not_a_miss— helper leveltest_falsy_payloads_are_not_misses— loops{},[],"",0,false; all five returned 200 onmainand a truthiness check would have broken every one of themtest_null_content_entry_is_served_as_200— end-to-end throughTestClienttest_absent_entry_is_still_a_404— the control
Prove-fail. Reverting the behaviour only (keeping _CACHE_MISS defined so imports still resolve, otherwise the tests would fail on ImportError and prove nothing):
3 failed, 101 passed
:1218 assert None is <object object> test_missing_file_returns_the_miss_sentinel
:1268 assert None is <object object> test_embedded_null_byte_in_path_is_a_miss
:1359 assert 404 == 200 test_null_content_entry_is_served_as_200
test_absent_entry_is_still_a_404 and test_falsy_payloads_are_not_misses correctly pass on both sides — the first exercises genuine absence, which is 404 either way, and the second exercises falsy-but-present values, where is not _CACHE_MISS holds regardless. They are fairness controls guarding against a future if not video_data, not coverage of this fix.
2. Path traversal — declined, but with evidence rather than assertion
I built a probe that plants secret_processed.json as a sibling of the cache directory and wires _get_cache_path to the real production lambda from real_video_processor.py L74–76 (cache_dir / f"{video_id}_processed.json") — a fixed MagicMock.return_value would short-circuit the vulnerability and prove nothing.
The decisive detail is printing the response body, which discriminates a router 404 from a handler 404:
PROBE '../secret' -> 404 leaked=False body={"detail":"Not Found"} <- ROUTER
PROBE '..%2Fsecret' -> 404 leaked=False body={"detail":"Not Found"} <- ROUTER
PROBE '..%252Fsecret' -> 404 leaked=False body={"detail":"Not Found"} <- ROUTER
PROBE '%2e%2e%2fsecret' -> 404 leaked=False body={"detail":"Not Found"} <- ROUTER
PROBE '..' -> 404 leaked=False body={"detail":"Not Found"} <- ROUTER
PROBE 'auJzb1D-fag' -> 404 leaked=False body={"detail":"Video analysis not found: auJzb1D-fag"} <- HANDLER
The last line is the control: a well-formed identifier does reach the handler and produces the handler's own 404 message. Every traversal payload produces Starlette's router 404 instead, because the default {video_id} path convertor is str, whose regex excludes /, and %2F is normalised before routing. The payloads never reach _read_video_analysis_sync at all.
I am not claiming the code is correct — only that this PR neither introduces nor widens the exposure. The pre-change code passed the identical unvalidated value to cache_path.exists() and then to open(); the diff swaps two syscalls for one and does not touch identifier handling. The right place to harden this is _get_cache_path itself, which lives in services/real_video_processor.py — currently owned by open PR #1237, so I am deliberately not touching it here. Happy to file a defence-in-depth issue against that path if you want it tracked.
3. What your traversal flag actually uncovered — a second regression
While running that probe I added %00 as an extra payload and it came back 500, not 404. That was not something I was looking for.
Mechanism. Path.exists() catches ValueError in addition to OSError:
Path('/tmp/a\x00b').exists() -> False
open(Path('/tmp/a\x00b')) -> ValueError: embedded null byte
So main's if not cache_path.exists(): 404 quietly absorbed malformed identifiers. My single-hop open() had no such guard and let the ValueError escape.
| version | GET /api/v2/videos/%00 |
|---|---|
main |
404 {"detail":"Video analysis not found: \u0000"} |
5aa56cf71 |
500 {"detail":"Internal server error"} ✗ |
7eefa9aae |
404 {"detail":"Video analysis not found: \u0000"} ✅ |
Why the fix is ValueError-only and not a broad except OSError. main's real rule decomposes as errors raised by stat() → 404, errors raised by open() on a stat-able path → 500. Widening to except (OSError, ValueError) would flip the directory case from 500 to 404, matching neither main nor the intent, and would break the already-shipped test_directory_path_raises_rather_than_reporting_a_miss.
| case | main |
bare except FileNotFoundError |
client-reachable? |
|---|---|---|---|
| missing file | 404 | 404 ✓ | yes |
| directory | 500 | 500 ✓ | no |
| corrupt JSON | 500 | 500 ✓ | yes |
| embedded null byte | 404 | 500 ✗ | yes — %00 |
NotADirectoryError |
404 | 500 ✗ | no — needs / in the id |
PermissionError on parent dir |
404 | 500 ✗ | no — misconfiguration |
PermissionError on the file |
500 | 500 ✓ | no |
The two PermissionError sub-cases diverge in opposite directions on main and are indistinguishable without a second syscall — which would defeat the single-hop, no-TOCTOU property that is the entire point of this PR. So except (FileNotFoundError, ValueError) fixes the only client-reachable divergence and I am disclosing the remaining unreachable ones rather than papering over them. The docstring now states the rule explicitly.
Prove-fail (narrowing back to except FileNotFoundError):
2 failed, 102 passed
ValueError: embedded null byte
src/youtube_extension/backend/real_api_endpoints.py:124: in _read_video_analysis_sync
"GET /api/v2/videos/%00 HTTP/1.1" 500 Internal Server Error
tests/unit/test_real_api_endpoints.py:1400: AssertionError
Two new tests cover it — test_embedded_null_byte_in_path_is_a_miss at the helper level and test_null_byte_identifier_is_a_404_not_a_500 end-to-end, the latter wiring _get_cache_path with a side_effect lambda rather than a return_value so the null byte actually reaches the syscall. test_wellformed_identifier_still_reaches_the_payload is its fairness control and passes on both sides.
Combined parity check
One probe, four cases, run against both versions of the module by swapping file content in place. Status codes and bodies compared:
| case | main |
7eefa9aae |
|---|---|---|
entry containing null |
200 null |
200 null ✅ |
| control object | 200 {"video_id":"control"} |
200 identical ✅ |
| absent entry | 404 Video analysis not found: nosuchvideo |
404 identical ✅ |
%00 |
404 Video analysis not found: \u0000 |
404 identical ✅ |
Full suite 104 passed. All three prove-fails re-run after the new tests landed (adding a test changed one of the earlier counts — the sentinel prove-fail went 2 → 3 failures because the new helper-level null-byte test also asserts against the sentinel). Black clean on both changed files; ruff check parity against main is identical on both files.
Thanks for the traversal flag — it was the reason I found the %00 case, which was a genuine 200→500 regression I would otherwise have shipped.
@coderabbitai review
|
@linear Thanks — merged your docstring note and shipped two corrections in Your nit was already satisfied. The TTL divergence is named explicitly in Correction to my own PR. CodeRabbit caught that the helper returned Second one, which nobody flagged. While disproving CodeRabbit's path-traversal finding I added I chose Traversal itself I declined, with evidence: printing the response body discriminates a router 404 from a handler 404, and all four encodings return Starlette's Parity, four cases, both versions, comparing bodies not just status codes:
Verification: 104 passed. Three prove-fails, all re-run after the new tests landed — the sentinel one went from 2 to 3 failures because a new helper-level test also asserts against the sentinel, which is the kind of thing that quietly rots if you don't re-run them. Offload prove-fail unchanged at The carry-forward lesson I'm taking into the rest of this series: when you delete a guard call, enumerate every exception it was implicitly swallowing, not just its happy path. Anything you want reopened before I merge? |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Current PR head and checks ==='
gh pr view 1304 --repo groupthinking/EventRelay \
--json headRefOid,statusCheckRollup \
--jq '{headRefOid, checks:[.statusCheckRollup[] | {name, status, conclusion}]}'
echo '=== Focused PR diff ==='
gh api -H 'Accept: application/vnd.github.v3.diff' \
/repos/groupthinking/EventRelay/pulls/1304 \
| sed -n '/real_api_endpoints.py/,$p' \
| sed -n '1,300p'
echo '=== Endpoint outline ==='
ast-grep outline src/youtube_extension/backend/real_api_endpoints.py --items all --view expanded
echo '=== Endpoint implementation ==='
rg -n -A55 -B15 '_CACHE_MISS|def _read_video_analysis_sync|async def get_video_analysis|video_data is' \
src/youtube_extension/backend/real_api_endpoints.py
test_file="$(fd -t f '^test_real_api_endpoints\.py$' tests | head -n 1)"
printf '\n=== Focused tests: %s ===\n' "$test_file"
rg -n -A35 -B10 \
'test_missing_file_returns_the_miss_sentinel|test_null_content_is_a_payload_not_a_miss|test_falsy_payloads_are_not_misses|test_null_content_entry_is_served_as_200|test_embedded_null_byte_in_path_is_a_miss|test_null_byte_identifier_is_a_404_not_a_500|test_directory_path_raises' \
"$test_file"Length of output: 29994 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Locate candidate test files ==='
fd -t f -i 'test_real_api_endpoints.py' . || true
fd -t f -i 'real_api_endpoints.py' . || true
echo '=== PR test diff: sentinel, falsy payload, and NUL cases ==='
gh api -H 'Accept: application/vnd.github.v3.diff' \
/repos/groupthinking/EventRelay/pulls/1304 \
| rg -n -A55 -B12 \
'test_missing_file_returns_the_miss_sentinel|test_null_content_is_a_payload_not_a_miss|test_falsy_payloads_are_not_misses|test_null_content_entry_is_served_as_200|test_embedded_null_byte_in_path_is_a_miss|test_null_byte_identifier_is_a_404_not_a_500|test_directory_path_raises' \
|| true
echo '=== Imports and uses of the changed helper ==='
rg -n -C3 '_read_video_analysis_sync|_CACHE_MISS|from typing import .*Final' \
src/youtube_extension/backend/real_api_endpoints.py
echo '=== Current file changes in the PR ==='
gh pr view 1304 --repo groupthinking/EventRelay --json files \
--jq '.files[] | {path, additions, deletions}'Length of output: 13354
Catching The added helper-level and endpoint-level tests cover both regressions and their controls. The traversal evidence supports keeping identifier hardening outside this PR. The source and focused test diff are ready to merge. Several CI checks were still in progress during this review, so merge should remain conditional on those checks succeeding. 🐇 ✅ ✏️ Learnings added
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! ✅ Action performedReview finished.
|
|
I wouldn’t reopen the code before merge. One non-blocking thing is worth fixing in the writeup: the current PR description still reflects the earlier state, so it now understates the final semantics and test count. In particular, src/youtube_extension/backend/real_api_endpoints.py now treats |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/youtube_extension/backend/real_api_endpoints.py:321
asyncio.to_threadmoves the call off the loop thread, but it does not guarantee the stated responsiveness for the unboundedjson.load(): CPython's_jsondecoder holds the GIL while scanning, so a sufficiently large transcript/analysis can still prevent the event-loop thread from running. The new responsiveness test only sleeps in__fspath__;time.sleep()releases the GIL, so it proves slow path resolution/I/O is isolated but does not exercise the parse cost identified in #1303. Parse in a separate process (or with a verified GIL-releasing strategy) and test an actual large payload, or narrow this change's guarantee to filesystem latency and track the remaining parse stall separately.
video_data = await asyncio.to_thread(_read_video_analysis_sync, cache_path)
Review (head
|
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>
🔍 PR Validation |
|
You were right, and I checked rather than argued. Numbers below. The claim was wrong. I said this moves the read off the event loop.
Three ticks over the whole parse — the stall is essentially the entire wall time. It scales linearly: 1.58 MB → 4.61 ms, 8.02 MB → 23.07 ms, 24.42 MB → 68.02 ms. About 3 ms/MB. Before/after, corrected (payload = real cache shape, full transcript):
The warm/small row is a regression I would have missed if you had not pushed: the executor hop (~0.5 ms) costs more than the 0.4 ms parse it defers. It is now stated in the PR body rather than omitted. This change wins on slow or unpredictable filesystems and loses on a warm cache with small payloads. On the test. Half-conceded. You are right that Taking your option (c). In
Residual parse stall needs its own fix — streaming parse, size cap, or process pool. Filing that separately rather than widening this PR. 105 tests pass. Ruff parity with |
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
Publish gate — green, awaiting human merge approval. All gates now pass on head Not auto-merging: gh pr merge 1304 --squash --delete-branch # closes #1303Generated by Claude Code |
|
@linear Thanks — actioned, and you were right on both counts. Your description-sync request (00:48). Done. The writeup now matches the final diff:
One thing you should know that I got wrong. While editing the body I ran a patch script whose section-boundary match hit a backticked mention of a heading in my own prose instead of the real heading, and it deleted On the earlier substantive point, restated so the record is in one place: you were right that All checks green, |
|
I’m merge-ready. Nothing left I’d reopen before merge. The description now matches the code, the earlier |
…e of #1216) (#1333) * fix(security): sandbox local media paths in cloud AI providers (#1209) All three cloud AI providers dispatch `analyze_image(image_url, ...)` on the string's prefix: `s3://` and `http(s)://` are treated as remote sources, and anything else fell through to an unguarded `open()`. A caller-supplied absolute path, `../` traversal, or symlink could therefore read any file readable by the service account. The same unguarded sink existed in all three providers, not just the one named in the issue: - aws_rekognition.py `_prepare_image_input` - azure_vision.py `_prepare_image_input` - google_cloud.py inline `open()` in `analyze_image` Introduce `cloud_ai/media_paths.py` as the single policy for local reads: - Local reads are opt-in via `CLOUD_AI_MEDIA_ROOT`. Unset (the default) disables them entirely, restricting providers to `s3://`/`https://`. This is fail-closed, and answers the issue's open question. - When a root is configured, both root and candidate are fully resolved (`Path.resolve()` follows symlinks) and the candidate must be contained by the root -- covering symlink escapes, not just lexical `..` segments. - Non-regular files (FIFO, device, directory) are rejected, so a FIFO placed inside the root cannot pin a `to_thread` worker forever. - Rejection raises the new typed `UnsafeMediaPathError(CloudAIError)` instead of silently returning empty bytes. Each provider re-raises `CloudAIError` subclasses unchanged so the type survives to the caller. - Providers read from the resolved path, not the caller string, narrowing the check-to-open race. - Error messages echo only the caller-supplied value; the resolved path is logged server-side for forensics rather than returned. Adds tests/unit/test_cloud_ai_media_paths.py (44 tests) covering absolute paths, `../` traversal, symlink escape, non-regular files, the disabled default, and per-provider propagation. Existing local-file tests now set `CLOUD_AI_MEDIA_ROOT`. Full cloud AI suite: 505 passed. Closes #1209 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * fix(security): reject non-directory CLOUD_AI_MEDIA_ROOT; close review gaps (#1216) Addresses the three unresolved Copilot review threads on #1216: 1. Fail-closed on a misconfigured root. get_media_root() left resolve() non-strict, so CLOUD_AI_MEDIA_ROOT=/etc/passwd (a regular file) was accepted as the root; that file then passed its own is_relative_to() containment check and was returned as a permitted read. Require the resolved root to be an existing directory, raising ConfigurationError otherwise. This also surfaces a nonexistent-directory typo loudly instead of silently rejecting every candidate. 2. Cover the Google permitted-file branch. AWS/Azure verified successful reads but the Google class only had rejection cases, while the PR's coverage table claimed the check for all three providers. Add an end-to-end analyze_image test asserting the resolved file's bytes are assigned to vision.Image().content. 3. Correct the module docstring. Remote-scheme handling is provider- specific: only AWS Rekognition recognises s3:// (Azure and Google treat it as a local path, rejected while local reads are disabled), and all three accept plain http:// as well as https://. Focused suite: 47 passed (44 + 3 new). Full cloud AI provider suites: 368 passed. ruff/mypy clean; black formatted. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TFFcgtEzNyhrxJnHgimcd2 * test: set CLOUD_AI_MEDIA_ROOT in read-offload tests added on main The event-loop offload tests from #1304/#1323 pass raw local paths to _prepare_image_input/analyze_image; with local reads now fail-closed behind CLOUD_AI_MEDIA_ROOT, they must opt in via tmp_path, matching the other pre-existing local-file tests. Generated with [Linear](https://linear.app/myxstack/issue/GRV-296/land-pr-1216-fixsecurity-sandbox-local-media-paths#agent-session-3138b916) Co-authored-by: linear-code[bot] <222613912+linear-code[bot]@users.noreply.github.com> * fix(security): correct s3:// provider guidance in media-path guard The disabled-reads UnsafeMediaPathError message and .env.example both suggested s3:// as a recovery scheme for all three cloud AI providers, but only AWS Rekognition recognizes s3://. Azure Vision and Google Vision route s3:// through the disabled local-path branch, so following that guidance just raises UnsafeMediaPathError again. Reword both to scope s3:// to AWS Rekognition and point Azure/Google callers at https:// (valid for every provider). No logic change; the guard behavior is unchanged. Addresses the two Copilot review threads on this PR. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W8yJv2udCCnwN4u586e9PL --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: linear-code[bot] <222613912+linear-code[bot]@users.noreply.github.com>
Canonical issue
Closes #1303.
Outcome
The video-detail endpoint (
GET /api/v2/videos/{video_id}) previously read andparsed the cached analysis synchronously on the event loop, behind a separate
Path.exists()stat probe. This PR moves theopen()/read()onto a workerthread via
asyncio.to_threadand collapses theexists()+open()pair intoa single
open(), treating the miss asFileNotFoundError.The read path also gained a correctness fix: a distinct
_CACHE_MISSsentinelnow separates "no cache entry" from a cache file holding the JSON literal
null. The former 404s; the latter is served as200 nullexactly as before.Risk
before/after numbers rather than quietly patched.
page cache with a small payload the max event-loop stall rises from ~0.40 ms
to ~0.94 ms, because the executor hop (~0.5 ms) costs more than the parse it
defers. The change is a net win only when the filesystem is cold, slow, or
networked — the case it was written for.
json.load()holds the GIL, so the parse half stillstalls the loop and scales with payload size. A separate fix (streaming parse,
size cap, or process pool) is tracked outside this PR.
queues behind other
to_threadwork. Corrupt JSON still surfaces as a 500(unchanged —
json.load()is outside the miss-exceptscope).video_idpath-traversal(CWE-22) — surface unchanged by this PR, tracked with perf: offload cache-directory scan off the event loop (#1231) #1237 which owns
_get_cache_path; unbounded payload size; no cache sweeper.perf/video-detail-read; the endpointreturns to the synchronous
exists()+open()+json.load()path withidentical status codes.
Verification
origin/main(ruff codes identical; Blackclean on added lines);
lint-pythonCI job green on head.to_threadhop drives theoffload heartbeat to
ticks=0, so they discriminate the change):nullpayload →200 null(parity with pre-PR behaviour)video_id→ValueError→ treated as miss →404(matching the oldPath.exists()swallow, not a 500)500(still propagates, not silently downgraded to a miss)404tests/unit/test_real_api_endpoints.py— 105 passed, whichincludes the four focused cases above plus a new characterisation test
(
test_parse_still_stalls_the_loop_in_proportion_to_payload) asserting theresidual parse stall scales with payload size. That test exists to stop the
narrowed claim from silently drifting back to the overstated one: if the parse
is ever made incremental, it fails and tells the maintainer to update the
documented guarantee.
0.40 → 0.94 ms (regression, disclosed); cold +50 ms 123.2 → 3.08 ms (−97%);
networked +200 ms 424.4 → 3.07 ms (−99%).
open()+json.load()ticked the loop179× (the read yields), bare
json.loads()only 3× (the parse does not).nullvs miss ambiguity was fixed in7eefa9a(_CACHE_MISSsentinel + identity check); the CWE-22 finding wastriaged as pre-existing and out of scope, tracked with perf: offload cache-directory scan off the event loop (#1231) #1237.
Production evidence
No production telemetry is attached; this endpoint's stall is a function of the
stored analysis size, which is not instrumented. What is offered instead is the
local measurement in
## Verificationabove, run against the real cacheshape (
real_video_processor.py:294stores the full transcript, so entry sizetracks video duration: ~0.15 MB for an hour, ~1.6 MB long-form). Production
evidence is not applicable until the endpoint's payload size is instrumented,
which is out of scope for this perf refactor.
Agent handoff
origin/mainreal_video_processor.py, perf: offload cache-directory scan off the event loop (#1231) #1237)video_id; unbounded payload size; no cache sweeper)