perf: shut down container services concurrently (#1322) - #1326
perf: shut down container services concurrently (#1322)#1326groupthinking wants to merge 1 commit into
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.
|
|
Important Review skippedAuto reviews are limited based on label configuration. 🏷️ Required labels (at least one) (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository YAML (base), Repository UI (inherited), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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: BLOCKEDReasons: Machine-readable verdict{
"details": {
"invalid_fields": [
"policy.agent_login",
"policy.run_id"
]
},
"reasons": [
"invalid_payload"
],
"verdict": "blocked"
} |
Status — functional checks green; blocked only on the advisory gate + human merge approvalFunctional verification (head
The one red check — { "reasons": ["invalid_payload"], "details": { "invalid_fields": ["policy.agent_login", "policy.run_id"] } }The gate treats this as agent work because the branch is prefixed Per the repo's own No code change is warranted on this PR. The diff is verified and functionally green. Not pushing a new commit, because that would only re-trigger the same advisory gate and reset the head with no benefit.
Generated by Claude Code |
|
Closing as a duplicate — this work is already merged into The identical container-shutdown fix for #1322 landed via #1323 (branch The red checks here are moot and not worth chasing on a duplicate:
No action needed — #1322 is resolved by #1323. Closing. Generated by Claude Code |
1 participant
Canonical issue
Closes #1322
Outcome
ServiceContainer.shutdownis the FastAPI shutdown handler, so it runs inside the SIGTERM grace window on every deploy and restart. This PR removes two defects in that path:forloop awaited each service'sclose()/cleanup()in turn, so total cost was the sum of every teardown round-trip. Services are independent with no declared teardown ordering, so they now tear down under a singleasyncio.gather(..., return_exceptions=True); cost becomes the slowest single teardown, and the previous "log and continue" behaviour is preserved per-service._register_skill_dependency_aliasesregisters four aliases whose factories delegate toget_service, which caches into_singletons, so one instance can be stored under several names. The old loop calledcleanup()once per name, double-closing sessions/pools. Targets are now deduplicated by object identity (id()).inspect.isawaitable(result), matching theapi_cost_workeridiom and removing a spurious "await NoneType" error previously logged for synccleanup().Scope
src/youtube_extension/backend/containers/service_container.py(extract_shutdown_service, concurrentshutdown),tests/unit/test_service_container.py(newTestShutdownConcurrency).Risk
Verification
PYTHONPATH=src pytest tests/unit/test_service_container.py→ 63 passed at head482d7fd. New cases cover concurrent in-flight teardown, alias dedup (torn down once), distinct-service teardown, synchronouscleanup()support, and one-failure-does-not-block-others.Production evidence
Not applicable — this is a backend process-shutdown path (SIGTERM grace window). No UI/preview surface; effect is observable only in deploy/restart teardown timing and logs.
Agent handoff
mainAgent provenance
This PR publishes a previously-committed, human-authored fix (commit
482d7fd, author Hayden, co-authored by Copilot App) that closes #1322 but had not been pushed or opened as a PR. No separate agent run produced the diff, so noagent-lock-manifestevidence is asserted here.Generated by Claude Code