perf: shut down container services concurrently - #1323
Conversation
ServiceContainer.shutdown is the FastAPI shutdown handler, so it runs inside the SIGTERM grace window on every deploy and restart. It had two defects. Teardown was serial: a for loop awaited each service in turn, making total cost the sum of every close() round-trip rather than the slowest one. The services are independent and no teardown ordering was declared or guaranteed, so they now tear down under a single asyncio.gather. Exceptions are collected via return_exceptions and reported per service, preserving the previous "log and continue" behaviour. Aliased singletons were cleaned up twice. _register_skill_dependency_ aliases registers four aliases whose factories delegate to get_service, which caches into _singletons, so one instance ends up stored under two names. The loop then called cleanup() on it once per name, double closing sessions and pools. Targets are now deduplicated by identity. Teardown hooks may also be synchronous, so the result is awaited only when inspect.isawaitable, matching the idiom in api_cost_worker. This removes a spurious "await NoneType" error previously logged for sync cleanup(). Closes #1322 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ 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)
📜 Recent review details⏰ Context from checks skipped due to timeout. (3)
🧰 Additional context used📓 Path-based instructions (10)**/*.{py,js,jsx,ts,tsx}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
**/*.py📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
⚙️ CodeRabbit configuration file
Files:
**/*.{py,js,ts,tsx}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
**/*.{py,ts,tsx}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
**/*📄 CodeRabbit inference engine (Custom checks)
Files:
**/*.{py,pyw}📄 CodeRabbit inference engine (AGENTS.md)
Files:
src/**/*.py📄 CodeRabbit inference engine (CLAUDE.md)
Files:
**/*.{py,ts,tsx,js,jsx}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
**/*.{py,pyi}📄 CodeRabbit inference engine (GEMINI.md)
Files:
**/*.{py,pyi,ts,tsx}📄 CodeRabbit inference engine (GEMINI.md)
Files:
🔍 Remote MCP GitHub Copilot, LinearAdditional review context
🔇 Additional comments (3)
📝 WalkthroughSummary by CodeRabbit
WalkthroughChangesService shutdown
Estimated code review effort: 3 (Moderate) | ~20 minutes 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 |
|
@linear-code @coderabbitai review Two defects in the FastAPI shutdown handler:
Specific review asks:
CI will show two pre-existing reds from |
Agent Completion Truth Gate: NOT_APPLICABLEEvidence agrees. Machine-readable verdict{
"details": {},
"reasons": [],
"verdict": "not_applicable"
} |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== PR diff ==='
git diff --find-renames origin/main...HEAD -- \
src/youtube_extension/backend/containers/service_container.py \
tests/unit/test_service_container.py
echo '=== ServiceContainer outline ==='
ast-grep outline src/youtube_extension/backend/containers/service_container.py --items all
echo '=== Relevant ServiceContainer sections ==='
rg -n -C 5 \
'def (shutdown|_shutdown_service|get_service|register_service|_register_skill_dependency_aliases)\b|_singletons|cleanup|\.close\b' \
src/youtube_extension/backend/containers/service_container.py
echo '=== Registered services and alias factories ==='
rg -n -C 3 \
'register_service\(|register_singleton\(|_register_skill_dependency_aliases|lambda: self\.get_service' \
src/youtube_extension/backend/containers/service_container.py
echo '=== Cleanup/close definitions and cross-service shutdown references ==='
rg -n -i -C 3 \
'async def (cleanup|close|shutdown)\b|def (cleanup|close|shutdown)\b|await .*\.?(cleanup|close|shutdown)\s*\(|\.(cleanup|close|shutdown)\s*\(' \
src/youtube_extension/backend --glob '*.py'
echo '=== Test outline and changed test area ==='
ast-grep outline tests/unit/test_service_container.py --items all
git diff --find-renames origin/main...HEAD --unified=80 -- tests/unit/test_service_container.pyLength of output: 66800 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Factory methods and imports ==='
sed -n '1,40p' src/youtube_extension/backend/containers/service_container.py
sed -n '200,375p' src/youtube_extension/backend/containers/service_container.py
echo '=== Factory return statements and constructor dependencies ==='
rg -n -A18 -B3 \
'def _create_(cache_service|health_monitoring_service|data_service|video_processor_factory|video_processing_service|hybrid_processor_service|websocket_connection_manager|websocket_service|notification_service|metrics_service|pubsub_service|agent_orchestrator|mcp_orchestrator)\b|return [A-Za-z_][A-Za-z0-9_]*\(' \
src/youtube_extension/backend/containers/service_container.py
echo '=== Locate the unit test file ==='
fd -a '^test_service_container\.py$' .
echo '=== Teardown definitions in likely registered service modules ==='
rg -n -i -C 12 \
'^\s*(async\s+)?def\s+(cleanup|close)\s*\(' \
src/youtube_extension/backend/services \
src/youtube_extension/backend \
--glob '*.py' \
--glob '!ai_code_generator.py'
echo '=== Direct container service access inside teardown hooks ==='
rg -n -i -U -P \
'(?s)^\s*(async\s+)?def\s+(cleanup|close)\s*\([^)]*\):.*?(?:get_service\(|service_container|_container|notification_service|metrics_service|data_service|websocket_service|pubsub_service)' \
src/youtube_extension/backend --glob '*.py' --glob '!ai_code_generator.py'Length of output: 62453 🏁 Script executed: #!/bin/bash
set -u
echo '=== Candidate files for registered service products ==='
fd -a -t f \
'^(cache_service|health_monitoring_service|data_service|video_processing_service|websocket_service|notification_service|metrics_service|pubsub_service)\.py$|^__init__\.py$' \
src/youtube_extension | sort
echo '=== WebSocket service lifecycle ==='
ast-grep outline src/youtube_extension/backend/services/websocket_service.py --items all
rg -n -C 12 \
'class (WebSocketConnectionManager|WebSocketService)\b|def __init__\b|async def (cleanup|close)\b|def (cleanup|close)\b' \
src/youtube_extension/backend/services/websocket_service.py || true
echo '=== Registered backend service teardown hooks ==='
for f in \
src/youtube_extension/backend/services/cache_service.py \
src/youtube_extension/backend/services/health_monitoring_service.py \
src/youtube_extension/backend/services/data_service.py \
src/youtube_extension/backend/services/notification_service.py \
src/youtube_extension/backend/services/metrics_service.py \
src/youtube_extension/backend/services/pubsub_service.py \
src/youtube_extension/backend/services/video_processing_service.py
do
if [ -f "$f" ]; then
echo "--- $f ---"
rg -n -C 10 '^\s*(async\s+)?def\s+(cleanup|close)\s*\(' "$f" || true
fi
done
echo '=== AI and orchestration candidate teardown hooks ==='
rg -n -i -C 10 \
'class (HybridProcessorService|AgentOrchestrator|MCPOrchestrator)\b|^\s*(async\s+)?def\s+(cleanup|close)\s*\(' \
src/youtube_extension/services \
--glob '*.py' || true
echo '=== Container references in all concrete teardown method regions ==='
rg -n -i -C 8 \
'^\s*(async\s+)?def\s+(cleanup|close)\s*\(|self\.(video_processing_service|connection_manager|cache_service|video_processor_factory)|get_service\(' \
src/youtube_extension/backend/services/websocket_service.py \
src/youtube_extension/backend/services/video_processing_service.py \
src/youtube_extension/services \
--glob '*.py' || trueLength of output: 61380 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Tracked test paths at HEAD ==='
git ls-tree -r --name-only HEAD | rg '(^|/)test_service_container\.py$' || true
echo '=== Tracked test paths at merge-base ==='
base="$(git merge-base origin/main HEAD)"
git ls-tree -r --name-only "$base" | rg '(^|/)test_service_container\.py$' || true
echo '=== Name-status for test paths ==='
git diff --name-status "$base"...HEAD -- | rg 'test_service_container\.py$' || true
echo '=== Worktree status for the expected path ==='
git status --short -- tests/unit/test_service_container.py
git check-ignore -v tests/unit/test_service_container.py || trueLength of output: 1040
The changed test path, ✅ Action performedReview finished.
|
There was a problem hiding this comment.
Pull request overview
Improves container shutdown performance and prevents duplicate teardown of aliased services.
Changes:
- Adds concurrent, identity-deduplicated service teardown.
- Supports synchronous and asynchronous teardown hooks.
- Adds shutdown concurrency and failure-isolation tests.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
service_container.py |
Implements deduplicated concurrent shutdown. |
test_service_container.py |
Tests concurrency, aliases, sync hooks, and failures. |
| result = closer() | ||
| if inspect.isawaitable(result): | ||
| await result |
|
|
||
| assert peak == 6, f"expected all 6 teardowns in flight together, saw {peak}" | ||
| # Serial teardown would take >= 6 * 50ms = 300ms. | ||
| assert elapsed < 0.20, f"teardown appears serial ({elapsed * 1000:.0f}ms)" |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
Independent verification (head CI red is the known pre-existing failure only. The Both
Not merging from here: Generated by Claude Code |
|
CI verified: 1 failed, 7947 passed. The one failure is New |
…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 #1322
Scope
src/youtube_extension/backend/containers/service_container.py—shutdownand a new_shutdown_servicehelper. Tests intests/unit/test_service_container.py.No service implementation is touched; no public API changes.
Outcome
ServiceContainer.shutdownis the FastAPI@app.on_event("shutdown")handler(
backend/main.py), so it runs inside the SIGTERM grace window on every deploy,scale-in and restart. It had two defects.
1. Teardown was serial. A
forloop awaited each service in turn, so total cost wasthe sum of every
close()round-trip instead of the slowest one. The services areindependent and no teardown ordering was ever declared or guaranteed. They now tear down
under a single
asyncio.gather.2. Aliased singletons were cleaned up twice.
_register_skill_dependency_aliasesregisters four aliases whose factories are
lambda: self.get_service("<target>").get_servicecaches intoself._singletons[name], so once both names resolve, the sameinstance sits under two keys — and
shutdowniteratedself._singletons.items(), callingcleanup()on that one object twice:gemini_servicehybrid_processor_servicedatabase_servicedata_serviceanalytics_servicemetrics_serviceemail_servicenotification_serviceTargets are now deduplicated by
id(service)before teardown.Teardown hooks may also be synchronous, so the result is awaited only when
inspect.isawaitable— the same idiom already used atapi_cost_worker.py:235.Production evidence
Stub container, 8 services at 40ms teardown each plus one alias pointing at an existing
instance, driven through the real
ServiceContainer.shutdown:cleanup()calls on the aliased instanceThe serial cost scales with service count: 13 services are registered today, so every
added service extends the grace-window spend. If the total ever exceeds the platform's
grace period the process is SIGKILLed and the remaining services never close at all,
leaking connections server-side.
Risk
Low, but three behavioural deltas are worth stating explicitly.
Teardown ordering is no longer insertion-ordered.
gatherdoes not preserve dictorder. There is no declared shutdown dependency graph in the container, and the previous
code already swallowed per-service exceptions and continued to the next service, so it
never guaranteed dependency-safe teardown either. This makes the existing absence of
ordering explicit rather than incidental.
Synchronous closers now succeed. Previously
await service.cleanup()ran a synccleanup()and then raisedTypeErroronawait None, which was caught and logged as aspurious shutdown error. Such services now complete cleanly and record no error.
A non-callable
cleanupattribute now falls through toclose(). The oldif hasattr(cleanup) / elif hasattr(close)chain meant a non-callablecleanupattributesuppressed
close()entirely. Strictly an improvement, but it is a behaviour change.Fan-out is deliberately unbounded here. Width is capped by the registration count
(13 services + 4 aliases, a compile-time constant), not by request volume or user input,
so no semaphore is warranted — and throttling shutdown would risk the exact grace-window
overrun this PR prevents.
return_exceptions=Truecan yieldCancelledError, so resultsare checked with
isinstance(result, BaseException)rather thanException.Verification
Five new tests in
TestShutdownConcurrency. Three fail against pre-change source:The concurrency test asserts observed peak in-flight teardowns == 6 rather than relying
solely on a wall-clock threshold, so it does not flake on a loaded host.
The other two cover dedupe-by-identity (distinct-but-equal services are each torn down) and
failure isolation (a raising service is reported in
shutdown_errorswithout blocking therest).
After the change, the whole file passes including all five pre-existing
TestShutdowntests— no regression:
ruff checkon both changed files reports only two pre-existingE731lambdaassignments at
test_service_container.py:117and:131(both present onorigin/main;this branch adds no lambdas).
Agent handoff
CI is expected to show two pre-existing failures unrelated to this change:
testandGenerate and Upload Coverage, both fromtests/unit/test_gh_aw_workflow_governance.py::test_ci_investigator_requires_dedicated_codex_credential(the workflow file was deleted in
07b8a2ec2but its test fromc2c23f4fbremains).That failure is already owned by PRs #1317 and #1320 — please do not fix it here.