Repository navigation
feat(runtime): per-handler shutdown timeouts (OMN-882) - #613
Merged
Merged
Conversation
📝 WalkthroughWalkthroughAdds per-handler shutdown timeouts: handlers (DB, HTTP) enforce close timeouts; a new config field exposes handler_shutdown_timeout_seconds; ProtocolLifecycleExecutor and RuntimeHostProcess apply per-handler timeouts; tests validate timeout, clamping, and priority/parallel shutdown behavior. Changes
Sequence DiagramsequenceDiagram
participant Client as Client
participant PLE as ProtocolLifecycleExecutor
participant Handler as Handler (DB/HTTP)
participant Asyncio as asyncio.wait_for
participant Logger as Logger
Client->>PLE: request shutdown(handler_type)
PLE->>Logger: log shutdown start and effective timeout
PLE->>Asyncio: wait_for(handler.shutdown(), timeout_seconds)
alt completes within timeout
Handler->>Handler: close pool/client
Asyncio-->>PLE: success
PLE-->>Client: return success ModelLifecycleResult
else timeout fires
Asyncio->>Logger: timeout warning
PLE->>Handler: force-terminate (e.g., terminate/close)
Handler->>Handler: force cleanup
Asyncio-->>PLE: TimeoutError
PLE-->>Client: return failed ModelLifecycleResult (timeout)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/omnibase_infra/handlers/handler_db.py`:
- Around line 130-133: The pool close path can leak connections if a
CancelledError escapes before pool termination; update the shutdown routine in
handler.shutdown() (the block that closes self._pool and calls terminate()) to
add an except asyncio.CancelledError clause that calls self._pool.terminate()
(or the handler's terminate() method) to forcibly close connections, then
re-raises the CancelledError so cancellation propagates; ensure the existing
finally that clears self._pool still runs after termination so no live pool
reference is left.
In `@src/omnibase_infra/handlers/handler_http.py`:
- Around line 303-322: The coroutine can be cancelled while awaiting
self._client.aclose(), which can skip the current post-close lines and leave
self._initialized True; move the finalization into a finally block so
self._initialized is set to False unconditionally. Concretely, wrap the await
asyncio.wait_for(self._client.aclose(), timeout=_CLIENT_CLOSE_TIMEOUT_SECONDS)
in try/except as now but ensure both self._client = None and self._initialized =
False are executed in the finally path (referencing self._client,
self._initialized, asyncio.wait_for and _CLIENT_CLOSE_TIMEOUT_SECONDS) so
cancellation or timeout cannot leave stale initialized state.
In `@src/omnibase_infra/runtime/service_runtime_host_process.py`:
- Around line 825-833: The handler shutdown timeout path currently accepts only
int/float and skips string parsing and bounds clamping; update the code that
sets self._handler_shutdown_timeout_seconds to mirror the validation used for
health_check_timeout_seconds and drain_timeout_seconds by accepting numeric
strings (parse to float), falling back to DEFAULT_HANDLER_SHUTDOWN_TIMEOUT, and
clamping the parsed value between MIN_HANDLER_SHUTDOWN_TIMEOUT and
MAX_HANDLER_SHUTDOWN_TIMEOUT (import these constants from
omnibase_infra.runtime.protocol_lifecycle_executor alongside
DEFAULT_HANDLER_SHUTDOWN_TIMEOUT) so the field and any code referencing
handler_shutdown_timeout_seconds are always within valid bounds.
In `@tests/unit/runtime/test_protocol_lifecycle_executor.py`:
- Around line 884-899: The test
test_shutdown_by_priority_with_timeout_slow_handler creates two handlers but
both have default priority 0, so update the mocks to assign different shutdown
priorities so the executor exercises cross-priority shutdown ordering: modify
the MockHandlerWithSlowShutdown instance (slow_handler) to return a higher
priority (e.g., implement or set shutdown_priority() -> 10) and set the
MockHandler instance (normal_handler) to a lower priority (e.g.,
shutdown_priority() -> 0 or -10), then run the shutdown through
ProtocolLifecycleExecutor(handler_shutdown_timeout_seconds=0.5) to ensure the
slow high-priority handler does not block the lower-priority group.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 375d4344-fe2b-48e5-bce2-6a4d5f965172
📒 Files selected for processing (6)
src/omnibase_infra/handlers/handler_db.pysrc/omnibase_infra/handlers/handler_http.pysrc/omnibase_infra/runtime/models/model_shutdown_config.pysrc/omnibase_infra/runtime/protocol_lifecycle_executor.pysrc/omnibase_infra/runtime/service_runtime_host_process.pytests/unit/runtime/test_protocol_lifecycle_executor.py
Comment on lines
+130
to
+133
| # Per-handler pool close timeout (OMN-882) | ||
| # asyncpg pool.close() can take 30s timeout x pool_size connections worst case. | ||
| # This timeout prevents unbounded shutdown waits. | ||
| _POOL_CLOSE_TIMEOUT_SECONDS: float = 5.0 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Reading handler_db.py around shutdown method ==="
head -n 450 src/omnibase_infra/handlers/handler_db.py | tail -n 50
echo
echo "=== Searching for wait_for and CancelledError handling in handler_db.py ==="
rg -n 'wait_for|CancelledError|except.*Error' src/omnibase_infra/handlers/handler_db.py | head -30
echo
echo "=== Looking for handler shutdown timeout orchestration ==="
fd -type f -name '*.py' -path '*/runtime/*' | xargs rg -l 'handler.*shutdown|shutdown.*timeout' 2>/dev/null | head -5Repository: OmniNode-ai/omnibase_infra
Length of output: 4161
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Inspecting protocol_lifecycle_executor.py for handler shutdown ==="
fd -type f -name 'protocol_lifecycle_executor.py' -o -name '*lifecycle*executor*'Repository: OmniNode-ai/omnibase_infra
Length of output: 328
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Full view of handler_db.py shutdown method ==="
wc -l src/omnibase_infra/handlers/handler_db.py
echo
rg -n -A 20 'async def shutdown' src/omnibase_infra/handlers/handler_db.pyRepository: OmniNode-ai/omnibase_infra
Length of output: 1209
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Search for calls to handler shutdown() ==="
rg -n 'shutdown\(\)' src/omnibase_infra --type py | grep -i handler | head -20
echo
echo "=== Search for lifecycle/orchestrator files ==="
fd -name '*orchestr*' -o -name '*lifecycle*' -o -name '*executor*' src/omnibase_infra | head -10
echo
echo "=== Search for task timeout management ==="
rg -n 'wait_for.*shutdown|gather.*shutdown|timeout.*handler' src/omnibase_infra --type py | head -20Repository: OmniNode-ai/omnibase_infra
Length of output: 2699
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Find all Python files in runtime directory ==="
find src/omnibase_infra/runtime -name '*.py' 2>/dev/null
echo
echo "=== Search for handler shutdown orchestration patterns ==="
rg -n 'handler.*shutdown|shutdown.*handler' src/omnibase_infra --type py -A 2 | head -40Repository: OmniNode-ai/omnibase_infra
Length of output: 14894
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Read protocol_lifecycle_executor.py around line 307 ==="
sed -n '290,330p' src/omnibase_infra/runtime/protocol_lifecycle_executor.py
echo
echo "=== Search for effective_timeout calculation ==="
rg -n 'effective_timeout' src/omnibase_infra/runtime/protocol_lifecycle_executor.py -B 5 -A 3Repository: OmniNode-ai/omnibase_infra
Length of output: 4778
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check if asyncio.CancelledError is caught anywhere in shutdown path ==="
rg -n 'CancelledError' src/omnibase_infra/handlers/handler_db.py
echo
echo "=== Verify asyncio.wait_for behavior with nested contexts ==="
python3 << 'PYTHON_EOF'
import asyncio
import inspect
# Check wait_for signature and CancelledError behavior
print("asyncio.wait_for behavior with nested timeouts:")
print("- Outer wait_for timeout < inner timeout raises asyncio.TimeoutError (wrapped as CancelledError)")
print("- Caller of wait_for must handle TimeoutError explicitly")
print("- CancelledError is a BaseException, NOT caught by 'except TimeoutError'")
print()
print("Code path if outer timeout fires before inner timeout:")
print("1. Outer wait_for() times out -> raises asyncio.TimeoutError")
print("2. Inner except TimeoutError does NOT catch this")
print("3. Finally block executes, clearing self._pool without terminate()")
print("4. Leaked connections remain active")
PYTHON_EOFRepository: OmniNode-ai/omnibase_infra
Length of output: 145
Handle cancellation path in pool close; without CancelledError handling, connections leak on cancellation.
The outer handler.shutdown() call at protocol_lifecycle_executor.py:307 wraps the handler in asyncio.wait_for(handler.shutdown(), timeout=effective_timeout). If this outer timeout or task cancellation fires before the inner pool close completes, a CancelledError is raised. Since CancelledError doesn't inherit from TimeoutError, the exception bypasses the handler at line 424, but the finally block at line 435 still executes and clears self._pool without calling terminate(). This leaves live database connections active.
Add except asyncio.CancelledError handling to ensure terminate() is called:
Suggested fix
if self._pool is not None:
try:
await asyncio.wait_for(
self._pool.close(), timeout=_POOL_CLOSE_TIMEOUT_SECONDS
)
except TimeoutError:
logger.warning(
"Database pool close timed out, terminating connections",
extra={
"timeout_seconds": _POOL_CLOSE_TIMEOUT_SECONDS,
"pool_size": self._pool_size,
},
)
# Terminate remaining connections forcefully
self._pool.terminate()
+ except asyncio.CancelledError:
+ logger.warning(
+ "Database pool close cancelled, terminating connections",
+ extra={"pool_size": self._pool_size},
+ )
+ self._pool.terminate()
+ raise
finally:
self._pool = None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/omnibase_infra/handlers/handler_db.py` around lines 130 - 133, The pool
close path can leak connections if a CancelledError escapes before pool
termination; update the shutdown routine in handler.shutdown() (the block that
closes self._pool and calls terminate()) to add an except asyncio.CancelledError
clause that calls self._pool.terminate() (or the handler's terminate() method)
to forcibly close connections, then re-raises the CancelledError so cancellation
propagates; ensure the existing finally that clears self._pool still runs after
termination so no live pool reference is left.
Comment on lines
+825
to
+833
| # Per-handler shutdown timeout (OMN-882) | ||
| # Prevents a single slow handler from blocking entire shutdown sequence | ||
| _handler_shutdown_raw = config.get("handler_shutdown_timeout_seconds") | ||
| handler_shutdown_value: float = DEFAULT_HANDLER_SHUTDOWN_TIMEOUT | ||
| if isinstance(_handler_shutdown_raw, int | float): | ||
| handler_shutdown_value = float(_handler_shutdown_raw) | ||
|
|
||
| self._handler_shutdown_timeout_seconds: float = handler_shutdown_value | ||
|
|
There was a problem hiding this comment.
Validate and clamp handler_shutdown_timeout_seconds the same way as other timeout configs.
Unlike health_check_timeout_seconds and drain_timeout_seconds, this path currently skips string parsing and bounds clamping, so invalid numeric values pass through silently.
Suggested fix
- _handler_shutdown_raw = config.get("handler_shutdown_timeout_seconds")
- handler_shutdown_value: float = DEFAULT_HANDLER_SHUTDOWN_TIMEOUT
- if isinstance(_handler_shutdown_raw, int | float):
- handler_shutdown_value = float(_handler_shutdown_raw)
+ _handler_shutdown_raw = config.get("handler_shutdown_timeout_seconds")
+ handler_shutdown_value: float = DEFAULT_HANDLER_SHUTDOWN_TIMEOUT
+ if isinstance(_handler_shutdown_raw, int | float):
+ handler_shutdown_value = float(_handler_shutdown_raw)
+ elif isinstance(_handler_shutdown_raw, str):
+ try:
+ handler_shutdown_value = float(_handler_shutdown_raw)
+ except ValueError:
+ logger.warning(
+ "Invalid handler_shutdown_timeout_seconds string value, using default",
+ extra={
+ "invalid_value": _handler_shutdown_raw,
+ "default_value": DEFAULT_HANDLER_SHUTDOWN_TIMEOUT,
+ },
+ )
+ handler_shutdown_value = DEFAULT_HANDLER_SHUTDOWN_TIMEOUT
+
+ if (
+ handler_shutdown_value < MIN_HANDLER_SHUTDOWN_TIMEOUT
+ or handler_shutdown_value > MAX_HANDLER_SHUTDOWN_TIMEOUT
+ ):
+ logger.warning(
+ "handler_shutdown_timeout_seconds out of valid range, clamping",
+ extra={
+ "original_value": handler_shutdown_value,
+ "min_value": MIN_HANDLER_SHUTDOWN_TIMEOUT,
+ "max_value": MAX_HANDLER_SHUTDOWN_TIMEOUT,
+ "clamped_value": max(
+ MIN_HANDLER_SHUTDOWN_TIMEOUT,
+ min(handler_shutdown_value, MAX_HANDLER_SHUTDOWN_TIMEOUT),
+ ),
+ },
+ )
+ handler_shutdown_value = max(
+ MIN_HANDLER_SHUTDOWN_TIMEOUT,
+ min(handler_shutdown_value, MAX_HANDLER_SHUTDOWN_TIMEOUT),
+ )# Also import bounds alongside DEFAULT_HANDLER_SHUTDOWN_TIMEOUT
from omnibase_infra.runtime.protocol_lifecycle_executor import (
DEFAULT_HANDLER_SHUTDOWN_TIMEOUT,
MAX_HANDLER_SHUTDOWN_TIMEOUT,
MIN_HANDLER_SHUTDOWN_TIMEOUT,
ProtocolLifecycleExecutor,
)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/omnibase_infra/runtime/service_runtime_host_process.py` around lines 825
- 833, The handler shutdown timeout path currently accepts only int/float and
skips string parsing and bounds clamping; update the code that sets
self._handler_shutdown_timeout_seconds to mirror the validation used for
health_check_timeout_seconds and drain_timeout_seconds by accepting numeric
strings (parse to float), falling back to DEFAULT_HANDLER_SHUTDOWN_TIMEOUT, and
clamping the parsed value between MIN_HANDLER_SHUTDOWN_TIMEOUT and
MAX_HANDLER_SHUTDOWN_TIMEOUT (import these constants from
omnibase_infra.runtime.protocol_lifecycle_executor alongside
DEFAULT_HANDLER_SHUTDOWN_TIMEOUT) so the field and any code referencing
handler_shutdown_timeout_seconds are always within valid bounds.
Comment on lines
+884
to
+899
| async def test_shutdown_by_priority_with_timeout_slow_handler(self) -> None: | ||
| """Test that slow handler in priority group doesn't block other groups.""" | ||
| executor = ProtocolLifecycleExecutor(handler_shutdown_timeout_seconds=0.5) | ||
|
|
||
| # Slow handler at high priority | ||
| slow_handler = MockHandlerWithSlowShutdown( | ||
| handler_type="slow", delay_seconds=10.0 | ||
| ) | ||
|
|
||
| # Normal handler at low priority | ||
| normal_handler = MockHandler(handler_type="normal") | ||
|
|
||
| handlers: dict[str, ProtocolHandler] = { | ||
| "slow": slow_handler, # type: ignore[dict-item] | ||
| "normal": normal_handler, # type: ignore[dict-item] | ||
| } |
There was a problem hiding this comment.
This test does not currently create different priority groups.
The docstring says “slow handler at high priority” and “normal handler at low priority”, but both handlers default to priority 0 (no shutdown_priority()), so this does not validate cross-priority progression.
Suggested fix
- # Slow handler at high priority
- slow_handler = MockHandlerWithSlowShutdown(
+ # Slow handler at high priority
+ slow_handler = MockHandlerWithSlowShutdown(
handler_type="slow", delay_seconds=10.0
)
+ slow_handler.shutdown_priority = lambda: 100 # type: ignore[attr-defined]
- # Normal handler at low priority
- normal_handler = MockHandler(handler_type="normal")
+ # Normal handler at low priority
+ normal_handler = MockHandlerWithPriority(
+ handler_type="normal",
+ priority=0,
+ )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/runtime/test_protocol_lifecycle_executor.py` around lines 884 -
899, The test test_shutdown_by_priority_with_timeout_slow_handler creates two
handlers but both have default priority 0, so update the mocks to assign
different shutdown priorities so the executor exercises cross-priority shutdown
ordering: modify the MockHandlerWithSlowShutdown instance (slow_handler) to
return a higher priority (e.g., implement or set shutdown_priority() -> 10) and
set the MockHandler instance (normal_handler) to a lower priority (e.g.,
shutdown_priority() -> 0 or -10), then run the shutdown through
ProtocolLifecycleExecutor(handler_shutdown_timeout_seconds=0.5) to ensure the
slow high-priority handler does not block the lower-priority group.
jonahgabriel
enabled auto-merge
March 4, 2026 23:52
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/omnibase_infra/runtime/service_runtime_host_process.py (1)
839-846:⚠️ Potential issue | 🟡 MinorNormalize/clamp
handler_shutdown_timeout_secondslike other timeout configs.Line 843-Line 846 currently skips numeric-string parsing and bounds clamping, so values like
"15"are ignored and out-of-range numbers are not normalized at this layer.Suggested patch
from omnibase_infra.runtime.protocol_lifecycle_executor import ( DEFAULT_HANDLER_SHUTDOWN_TIMEOUT, + MAX_HANDLER_SHUTDOWN_TIMEOUT, + MIN_HANDLER_SHUTDOWN_TIMEOUT, ProtocolLifecycleExecutor, ) _handler_shutdown_raw = config.get("handler_shutdown_timeout_seconds") handler_shutdown_value: float = DEFAULT_HANDLER_SHUTDOWN_TIMEOUT if isinstance(_handler_shutdown_raw, int | float): handler_shutdown_value = float(_handler_shutdown_raw) + elif isinstance(_handler_shutdown_raw, str): + try: + handler_shutdown_value = float(_handler_shutdown_raw) + except ValueError: + logger.warning( + "Invalid handler_shutdown_timeout_seconds string value, using default", + extra={ + "invalid_value": _handler_shutdown_raw, + "default_value": DEFAULT_HANDLER_SHUTDOWN_TIMEOUT, + }, + ) + handler_shutdown_value = DEFAULT_HANDLER_SHUTDOWN_TIMEOUT + + if ( + handler_shutdown_value < MIN_HANDLER_SHUTDOWN_TIMEOUT + or handler_shutdown_value > MAX_HANDLER_SHUTDOWN_TIMEOUT + ): + logger.warning( + "handler_shutdown_timeout_seconds out of valid range, clamping", + extra={ + "original_value": handler_shutdown_value, + "min_value": MIN_HANDLER_SHUTDOWN_TIMEOUT, + "max_value": MAX_HANDLER_SHUTDOWN_TIMEOUT, + "clamped_value": max( + MIN_HANDLER_SHUTDOWN_TIMEOUT, + min(handler_shutdown_value, MAX_HANDLER_SHUTDOWN_TIMEOUT), + ), + }, + ) + handler_shutdown_value = max( + MIN_HANDLER_SHUTDOWN_TIMEOUT, + min(handler_shutdown_value, MAX_HANDLER_SHUTDOWN_TIMEOUT), + ) self._handler_shutdown_timeout_seconds: float = handler_shutdown_value🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/omnibase_infra/runtime/service_runtime_host_process.py` around lines 839 - 846, The handler shutdown timeout parsing in __init__ uses isinstance(_handler_shutdown_raw, int | float) and therefore skips numeric strings and does not clamp out-of-range values; update the logic that sets handler_shutdown_value (currently using DEFAULT_HANDLER_SHUTDOWN_TIMEOUT and config.get("handler_shutdown_timeout_seconds")) to also accept numeric strings by attempting to parse str values to float, and then normalize/clamp the resulting float to the same min/max bounds used for other timeouts (reuse the existing timeout min/max constants or helper clamp function if available) before assigning self._handler_shutdown_timeout_seconds.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/omnibase_infra/runtime/service_runtime_host_process.py`:
- Around line 839-846: The handler shutdown timeout parsing in __init__ uses
isinstance(_handler_shutdown_raw, int | float) and therefore skips numeric
strings and does not clamp out-of-range values; update the logic that sets
handler_shutdown_value (currently using DEFAULT_HANDLER_SHUTDOWN_TIMEOUT and
config.get("handler_shutdown_timeout_seconds")) to also accept numeric strings
by attempting to parse str values to float, and then normalize/clamp the
resulting float to the same min/max bounds used for other timeouts (reuse the
existing timeout min/max constants or helper clamp function if available) before
assigning self._handler_shutdown_timeout_seconds.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f4ecff13-593d-42e4-bda9-374b27184612
📒 Files selected for processing (3)
src/omnibase_infra/runtime/models/model_shutdown_config.pysrc/omnibase_infra/runtime/protocol_lifecycle_executor.pysrc/omnibase_infra/runtime/service_runtime_host_process.py
…eful shutdown (OMN-882) Database pool cleanup (asyncpg.Pool.close()) can take >40s per connection, leading to unpredictable shutdown times up to 150s worst case. This change adds configurable per-handler shutdown timeouts so no single handler can block the entire shutdown sequence. Changes: - Add handler_shutdown_timeout_seconds (default 10s) to ModelShutdownConfig - Wrap ProtocolLifecycleExecutor.shutdown_handler() with asyncio.wait_for() - Add per-call timeout override parameter to shutdown_handler() - Wrap HandlerDb pool.close() with 5s timeout, fall back to terminate() - Wrap HandlerHttpRest client.aclose() with 5s timeout - Wire handler_shutdown_timeout_seconds from config through RuntimeHostProcess - Add 9 new unit tests covering timeout behavior
Remove boilerplate docstring openers flagged by check_ai_slop.py in model_shutdown_config.py and protocol_lifecycle_executor.py. Replace raw topic literal strings in service_runtime_host_process.py with SUFFIX_CONTRACT_REGISTERED/DEREGISTERED constants from topics module.
jonahgabriel
force-pushed
the
jonah/omn-882-per-handler-shutdown-timeouts
branch
from
March 5, 2026 02:13
07e3db5 to
e4ce73b
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/omnibase_infra/handlers/handler_db.py (1)
420-435:⚠️ Potential issue | 🟠 MajorHandle cancellation during pool close to avoid orphaned DB connections.
At Line 421,
wait_for()can fail via cancellation (not only timeout). With onlyexcept TimeoutError, cancellation can skip forced termination and still hit Line 435 (self._pool = None), dropping the reference without ensuring connections are torn down.Suggested fix
if self._pool is not None: try: await asyncio.wait_for( self._pool.close(), timeout=_POOL_CLOSE_TIMEOUT_SECONDS ) except TimeoutError: logger.warning( "Database pool close timed out, terminating connections", extra={ "timeout_seconds": _POOL_CLOSE_TIMEOUT_SECONDS, "pool_size": self._pool_size, }, ) # Terminate remaining connections forcefully self._pool.terminate() + except asyncio.CancelledError: + logger.warning( + "Database pool close cancelled, terminating connections", + extra={"pool_size": self._pool_size}, + ) + self._pool.terminate() + raise finally: self._pool = None#!/bin/bash set -euo pipefail echo "=== Verify shutdown exception handling in HandlerDb ===" rg -n -C4 'await asyncio\.wait_for\(|except TimeoutError|except asyncio\.CancelledError|terminate\(\)|self\._pool = None' src/omnibase_infra/handlers/handler_db.py echo echo "=== Verify asyncio exception hierarchy relevant to this handler ===" python - <<'PY' import asyncio print("CancelledError MRO:", asyncio.CancelledError.__mro__) print("Is CancelledError a TimeoutError?", issubclass(asyncio.CancelledError, TimeoutError)) PY🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/omnibase_infra/handlers/handler_db.py` around lines 420 - 435, The current shutdown block in HandlerDb only catches TimeoutError, so asyncio.CancelledError can bypass the termination logic and let self._pool be set to None while connections remain open; update the try/except/finally around await asyncio.wait_for(self._pool.close(), ...) to also catch asyncio.CancelledError (and any other relevant exceptions) and in those handlers ensure you call self._pool.terminate() before clearing the reference, keeping the termination call and self._pool = None behavior consistent (use symbols: await asyncio.wait_for(self._pool.close()), except asyncio.CancelledError, self._pool.terminate(), and self._pool = None).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/omnibase_infra/handlers/handler_db.py`:
- Around line 420-435: The current shutdown block in HandlerDb only catches
TimeoutError, so asyncio.CancelledError can bypass the termination logic and let
self._pool be set to None while connections remain open; update the
try/except/finally around await asyncio.wait_for(self._pool.close(), ...) to
also catch asyncio.CancelledError (and any other relevant exceptions) and in
those handlers ensure you call self._pool.terminate() before clearing the
reference, keeping the termination call and self._pool = None behavior
consistent (use symbols: await asyncio.wait_for(self._pool.close()), except
asyncio.CancelledError, self._pool.terminate(), and self._pool = None).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9171b5ef-03bd-4c93-825c-045b35e6bb39
📒 Files selected for processing (2)
src/omnibase_infra/handlers/handler_db.pysrc/omnibase_infra/handlers/handler_http.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/omnibase_infra/handlers/handler_http.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
asyncpg.Pool.close()) was taking >40s in worst case (30s timeout x 5 connections = 150s). Now bounded by configurable timeout (default 10s at executor level, 5s at handler level)ProtocolLifecycleExecutor.shutdown_handler()withasyncio.wait_for()and adds timeout override parameterHandlerDb.shutdown()pool close with 5s timeout, falling back topool.terminate()on timeoutHandlerHttpRest.shutdown()client close with 5s timeouthandler_shutdown_timeout_secondsfield toModelShutdownConfig(default 10s, range 1-300s)RuntimeHostProcesstoProtocolLifecycleExecutorFiles Changed
src/omnibase_infra/runtime/models/model_shutdown_config.py- Newhandler_shutdown_timeout_secondsfieldsrc/omnibase_infra/runtime/protocol_lifecycle_executor.py-asyncio.wait_for()wrapping, new init param, timeout constantssrc/omnibase_infra/handlers/handler_db.py- Pool close timeout with terminate fallbacksrc/omnibase_infra/handlers/handler_http.py- Client close timeoutsrc/omnibase_infra/runtime/service_runtime_host_process.py- Config wiringtests/unit/runtime/test_protocol_lifecycle_executor.py- 9 new tests (33 total, all passing)Test plan
Closes OMN-882
Summary by CodeRabbit
New Features
Tests