Repository navigation
fix(mixins): add event-based shutdown signal to health monitor loop - #90
Conversation
The health monitor loop had a 30-second asyncio.sleep() that did not respond immediately to shutdown signals. When tests ended, tasks would remain pending during sleep, causing warnings and errors during pytest-xdist worker shutdown. Changes: - Add _shutdown_event: asyncio.Event for coordinated shutdown - Replace asyncio.sleep() with event-based wait_for() timeout - Signal shutdown event in stop_service_mode() and aclose() - Use safe attribute access with hasattr()/getattr() for Pydantic This eliminates Task was destroyed but it is pending warnings and I/O operation on closed file errors in test teardown.
WalkthroughAdds an internal Changes
Sequence Diagram(s)sequenceDiagram
participant SM as Service Manager
participant SV as MixinNodeService
participant HM as Health Monitor
participant SE as _shutdown_event
SM->>+SV: start_service_mode()
SV->>SE: create (lazy) %%#[note: lazy creation]
par Service loop
loop poll
SV->>SE: wait(timeout=0.1s)
alt SE set
SE-->>SV: wake -> exit/cleanup
else timeout
SV->>SV: iterate work
end
end
and Health monitor
loop periodic check
HM->>SE: wait(timeout=30s)
alt SE set
SE-->>HM: wake -> exit/cleanup
else timeout
HM->>HM: perform health check
end
end
end
SM->>SV: stop_service_mode() / aclose()
SV->>SE: set() %% immediate wakeups
SE-->>SV: wake pending waits
SE-->>HM: wake pending waits
SV->>SM: cleanup complete
HM->>SM: cleanup complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🧰 Additional context used📓 Path-based instructions (3)**/*.py📄 CodeRabbit inference engine (CLAUDE.md)
Files:
src/omnibase_core/**/*.py📄 CodeRabbit inference engine (CLAUDE.md)
Files:
tests/**/*.py📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧬 Code graph analysis (1)tests/unit/mixins/test_mixin_node_service_shutdown.py (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
🔇 Additional comments (16)
Comment |
Code Review: Event-Based Shutdown Signal for Health Monitor LoopOverall Assessment: ✅ APPROVE WITH MINOR SUGGESTIONSThis is a well-designed solution to a real problem. The implementation properly addresses the "Task was destroyed but it is pending!" warnings during test teardown by introducing interruptible sleep patterns. ✅ Strengths1. Correct Problem DiagnosisThe root cause analysis is spot-on: 30-second asyncio.sleep() calls were blocking immediate shutdown response, causing task destruction warnings in pytest-xdist workers. 2. Clean Implementation
3. Follows ONEX Architecture Patterns
4. Good Code DocumentationComments clearly explain the "why" behind the implementation (lines 129-131, 172-174, 214-215, 432-434). 🔍 Potential Issues & Suggestions1. Thread Safety Consideration
|
| Category | Status | Notes |
|---|---|---|
| Type Safety | ✅ Pass | Proper type annotations (asyncio.Event or None) |
| Pydantic Compliance | ✅ Pass | Correct use of object.setattr() |
| Error Handling | ✅ Pass | Maintains existing exception patterns |
| Documentation | ✅ Pass | Clear comments explaining behavior |
| ONEX Patterns | ✅ Pass | Follows mixin conventions |
| Thread Safety | See Issue #1 above | |
| Test Coverage | See Issue #3 above |
📝 Recommendations
Must Address (Before Merge):
- Clarify thread safety assumptions - Add documentation about event loop requirements
- Add explicit shutdown timing test - Verify immediate wakeup behavior
Nice to Have (Future Work):
- Eliminate race condition - Tighten null checks in shutdown paths
- Document wait timeout differences - Explain 0.1s vs 30s rationale
🎯 Final Verdict
LGTM with minor documentation improvements needed.
This PR solves a real pain point (test teardown warnings) with a clean, maintainable solution. The event-based coordination pattern is appropriate and follows asyncio best practices.
Once the thread safety assumptions are documented and a timing test is added, this is ready to merge.
Estimated Risk: Low
Architectural Impact: Minimal (internal mixin change only)
User Impact: Positive (cleaner test runs, faster CI)
Great work @jonahgabriel! 🚀
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/omnibase_core/mixins/mixin_node_service.py (2)
172-176: Consider simplifying the defensive check.Since
_shutdown_eventis always initialized in__init__, thehasattrcheck is redundant. A simplerif self._shutdown_event is not None:would suffice.However, the current defensive approach is acceptable for mixin classes where
__init__may not always be called in complex MRO scenarios.- if hasattr(self, "_shutdown_event") and self._shutdown_event is not None: + if self._shutdown_event is not None: self._shutdown_event.set()
398-409: Minor inconsistency in attribute access pattern.This segment uses
getattr(self, "_shutdown_event", None)whilestop_service_mode()andaclose()usehasattr(...) and self._shutdown_event. Consider using a consistent pattern across the file.The interruptible wait logic is correct and well-implemented for responsive shutdown.
For consistency, consider aligning with the simpler pattern:
- shutdown_event = getattr(self, "_shutdown_event", None) - if shutdown_event is not None: + if self._shutdown_event is not None: try: - await asyncio.wait_for(shutdown_event.wait(), timeout=0.1) + await asyncio.wait_for(self._shutdown_event.wait(), timeout=0.1)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/omnibase_core/mixins/mixin_node_service.py(7 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Always use Poetry for Python package management and task execution - never use pip or python directly
Always use ModelOnexError with EnumCoreErrorCode for structured error handling, never raise generic Exception
Use @standard_error_handling decorator on async functions to eliminate boilerplate try/catch logic
Use duck typing with protocols instead of isinstance checks - protocols define contracts through interfaces, not runtime type checking
Always use 100% strict mypy type checking - all functions must have type annotations and no untyped definitions are allowed
Run code through black formatter, isort for import sorting, and ruff linter before committing
Use async/await pattern with asyncio for asynchronous operations - never use threading for I/O operations
Use ModelEventEnvelope for inter-service event-driven communication and process event payloads through envelope pattern
Files:
src/omnibase_core/mixins/mixin_node_service.py
src/omnibase_core/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use container.get_service('ProtocolName') for dependency resolution by protocol name, never by concrete class name
Files:
src/omnibase_core/mixins/mixin_node_service.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Code Quality
- GitHub Check: Smoke Tests
- GitHub Check: claude-review
🔇 Additional comments (5)
src/omnibase_core/mixins/mixin_node_service.py (5)
85-85: LGTM!Type annotation for
_shutdown_eventfollows the established pattern and uses the appropriateasyncio.Event | Noneunion type.
108-109: LGTM!Proper lazy initialization of
_shutdown_eventusingobject.__setattr__for Pydantic compatibility. Creating the event lazily instart_service_mode()is correct sinceasyncio.Eventrequires an active event loop.
129-132: LGTM!The
asyncio.Eventis correctly created duringstart_service_mode()when an active event loop is guaranteed. The direct assignment is consistent with how other internal state attributes are updated in this method.
214-216: LGTM!Consistent with
stop_service_mode(). Setting the event early ensures sleeping tasks wake immediately before cleanup begins. The same optional simplification mentioned above applies here.
432-447: LGTM!This is the core fix for eliminating the "Task was destroyed but it is pending!" warnings. The 30-second timeout preserves the original health check interval while enabling immediate response to shutdown signals.
Same optional consistency refinement applies as noted for
_service_event_loop().
- Add comprehensive tests for _shutdown_event coordination - Add thread safety documentation to MixinNodeService docstring - Simplify attribute access: remove redundant hasattr() checks since _shutdown_event is initialized in model_post_init and __init__
PR Review: Event-Based Shutdown Signal for Health Monitor Loop✅ Overall Assessment: LGTM with Minor SuggestionsThis is an excellent fix that addresses a real problem with clean shutdown timing in the test suite. The implementation is well-thought-out, properly tested, and aligns with ONEX architecture principles. 🎯 Strengths1. Problem Identification
2. Implementation Quality
3. Test Coverage
4. Documentation
🔍 Code Quality FindingsThread Safety Documentation ⭐Lines 73-78: The new thread safety warning is appropriate and aligns with docs/guides/THREADING.md. Suggestion: Consider adding a reference to the THREADING.md guide in the docstring for discoverability. Error Handling PatternLines 434-446 & 399-409: The try/except TimeoutError pattern is correct:
Test QualityExcellent timing validation that double-checks both overall elapsed time AND internal task wait time. 🚨 RecommendationsShould Address (Before Merge)
Nice to Have (Follow-up)
🔐 Security & ArchitectureNo Security Concerns Identified
ONEX Architecture Compliance ⭐
Impact: This fix improves shutdown behavior across all service-mode nodes ✅ 📊 Test Coverage & PerformanceTest Impact
Performance Impact
🎉 ConclusionThis is a high-quality fix that:
Recommendation: Approve and merge after addressing the two "Should Address" items. Great work! 🚀 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tests/unit/mixins/test_mixin_node_service_shutdown.py (2)
45-52: Rename unusedinput_stateparameter to avoid lint noise
MinimalServiceNode.runnever usesinput_state, which triggers ARG002. In tests this is harmless, but renaming to_input_statekeeps intent clear and satisfies linters without behavior change.
70-103: Loosen strict wall‑clock assertions to reduce flakiness riskThe tests assert tight real‑time bounds (e.g.,
< 1.0or< 0.2seconds) based ontime.time(). Under loaded CI, scheduler delays can occasionally exceed these thresholds even when logic is correct.Consider:
- Using a slightly higher upper bound (e.g., 2–3 seconds), or
- Using a relative check like
elapsed < timeout / 2orpytest.approxwith a tolerance.This keeps the tests robust while still validating “immediate” wakeup semantics.
Also applies to: 110-117, 164-171, 219-222, 244-246, 284-296
src/omnibase_core/mixins/mixin_node_service.py (1)
692-706: Consider also signaling_shutdown_eventfrom signal handler for symmetryThe SIGTERM/SIGINT handler currently sets
_shutdown_requestedbut does not set_shutdown_event. That means:
_service_event_loopwill exit within its 0.1s timeout, but_health_monitor_loopmay still wait up to 30s for its timeout ifstop_service_modeoracloseare never called.If you want the same “immediate wakeup” behavior on OS signals as on explicit shutdown, consider:
def signal_handler(signum: int, _frame: Any) -> None: self._log_info(...) self._shutdown_requested = True if self._shutdown_event is not None: self._shutdown_event.set()This stays consistent with the new event‑based shutdown mechanism and further reduces the chance of pending‑task warnings on signal‑driven teardown.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/omnibase_core/mixins/mixin_node_service.py(8 hunks)tests/unit/mixins/test_mixin_node_service_shutdown.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Always use Poetry for Python package management and task execution - never use pip or python directly
Always use ModelOnexError with EnumCoreErrorCode for structured error handling, never raise generic Exception
Use @standard_error_handling decorator on async functions to eliminate boilerplate try/catch logic
Use duck typing with protocols instead of isinstance checks - protocols define contracts through interfaces, not runtime type checking
Always use 100% strict mypy type checking - all functions must have type annotations and no untyped definitions are allowed
Run code through black formatter, isort for import sorting, and ruff linter before committing
Use async/await pattern with asyncio for asynchronous operations - never use threading for I/O operations
Use ModelEventEnvelope for inter-service event-driven communication and process event payloads through envelope pattern
Files:
src/omnibase_core/mixins/mixin_node_service.pytests/unit/mixins/test_mixin_node_service_shutdown.py
src/omnibase_core/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use container.get_service('ProtocolName') for dependency resolution by protocol name, never by concrete class name
Files:
src/omnibase_core/mixins/mixin_node_service.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Maintain minimum 60% code coverage with pytest tests - unit tests go in tests/unit/, integration tests in tests/integration/
Run pytest tests with parallelization disabled (-n 0 -xvs) for debugging and use standard pytest run for CI
Use @pytest.mark decorators (unit, integration, slow, smoke, performance) to categorize tests appropriately
Files:
tests/unit/mixins/test_mixin_node_service_shutdown.py
🧬 Code graph analysis (1)
tests/unit/mixins/test_mixin_node_service_shutdown.py (1)
src/omnibase_core/mixins/mixin_node_service.py (3)
get_service_health(322-352)aclose(212-229)stop_service_mode(162-210)
🪛 Ruff (0.14.5)
tests/unit/mixins/test_mixin_node_service_shutdown.py
50-50: Unused method argument: input_state
(ARG002)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Code Quality
- GitHub Check: Smoke Tests
- GitHub Check: claude-review
🔇 Additional comments (3)
src/omnibase_core/mixins/mixin_node_service.py (2)
73-77: _shutdown_event wiring and documentation look consistentThe new
_shutdown_eventattribute, its lazy initialization instart_service_mode, and the added thread‑safety note are coherent with the asyncio‑only service model. This gives a clear coordination point for shutdown without changing public APIs.Also applies to: 91-116, 135-139
178-183: CatchingTimeoutErrorinasyncio.wait_foris correct; explicitasyncio.TimeoutErroroffers no functional advantage and contradicts modern Python best practicesIn Python 3.11+,
asyncio.TimeoutErroris an alias to the built-inTimeoutError, making them functionally identical. The current code at lines 399–415 and 437–451 correctly catches bareTimeoutErrorafterasyncio.wait_for(...)calls. Modern Python guidance favors catching the built-inTimeoutErrorover the asyncio alias, so the existing approach is already aligned with best practices. No refactoring is needed.(Note: Lines 178–183 and 220–223 do not contain timeout handling; they only signal the shutdown event and are not applicable to this suggestion.)
tests/unit/mixins/test_mixin_node_service_shutdown.py (1)
78-84: The suggestion to useasyncio.TimeoutErroris incorrect for modern Python.In Python 3.11+,
asyncio.TimeoutErroris a deprecated alias of the built-inTimeoutError. Theasyncio.wait_for()function now raises the built-inTimeoutErrordirectly. The current code usingTimeoutErroris already the correct and recommended approach. Usingasyncio.TimeoutErrorwould be unnecessary and contrary to modern Python best practices, which moved away from the asyncio-specific exception in favor of the standard library exception.
- Fix 6 RuntimeWarning "coroutine was never awaited" issues: - Change _update_specialized_metrics mock from AsyncMock to Mock (sync method) - Fix mocked asyncio.create_task to capture and close coroutines - Fix mocked asyncio.wait_for to close coroutines passed to it - Replace coroutine-based __await__ with generator-based approach - Fix asyncio "Task destroyed but pending" logging error: - Patch BaseEventLoop.call_exception_handler to suppress shutdown noise - Add session-scoped atexit handler for final task cleanup - Suppress logging errors from closed file handles during Python shutdown Test results: 407 passed, 1 skipped, 0 warnings
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/unit/models/nodes/node_services/conftest.py (1)
102-113: Consider catchingasyncio.CancelledErrorseparately.While the broad exception handling is appropriate for shutdown,
asyncio.CancelledErrorshould typically be re-raised in async contexts. However, since this is patching synchronous exception handling during interpreter shutdown, the current approach is acceptable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
tests/unit/models/nodes/node_services/conftest.py(3 hunks)tests/unit/models/nodes/node_services/test_model_service_compute_integration.py(1 hunks)tests/unit/models/nodes/node_services/test_model_service_effect_health.py(1 hunks)tests/unit/models/nodes/node_services/test_model_service_effect_lifecycle.py(2 hunks)tests/unit/models/nodes/node_services/test_model_service_orchestrator_health.py(9 hunks)tests/unit/models/nodes/node_services/test_model_service_orchestrator_lifecycle.py(5 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Always use Poetry for Python package management and task execution - never use pip or python directly
Always use ModelOnexError with EnumCoreErrorCode for structured error handling, never raise generic Exception
Use @standard_error_handling decorator on async functions to eliminate boilerplate try/catch logic
Use duck typing with protocols instead of isinstance checks - protocols define contracts through interfaces, not runtime type checking
Always use 100% strict mypy type checking - all functions must have type annotations and no untyped definitions are allowed
Run code through black formatter, isort for import sorting, and ruff linter before committing
Use async/await pattern with asyncio for asynchronous operations - never use threading for I/O operations
Use ModelEventEnvelope for inter-service event-driven communication and process event payloads through envelope pattern
Files:
tests/unit/models/nodes/node_services/test_model_service_compute_integration.pytests/unit/models/nodes/node_services/test_model_service_effect_health.pytests/unit/models/nodes/node_services/test_model_service_effect_lifecycle.pytests/unit/models/nodes/node_services/conftest.pytests/unit/models/nodes/node_services/test_model_service_orchestrator_lifecycle.pytests/unit/models/nodes/node_services/test_model_service_orchestrator_health.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Maintain minimum 60% code coverage with pytest tests - unit tests go in tests/unit/, integration tests in tests/integration/
Run pytest tests with parallelization disabled (-n 0 -xvs) for debugging and use standard pytest run for CI
Use @pytest.mark decorators (unit, integration, slow, smoke, performance) to categorize tests appropriately
Files:
tests/unit/models/nodes/node_services/test_model_service_compute_integration.pytests/unit/models/nodes/node_services/test_model_service_effect_health.pytests/unit/models/nodes/node_services/test_model_service_effect_lifecycle.pytests/unit/models/nodes/node_services/conftest.pytests/unit/models/nodes/node_services/test_model_service_orchestrator_lifecycle.pytests/unit/models/nodes/node_services/test_model_service_orchestrator_health.py
🧬 Code graph analysis (4)
tests/unit/models/nodes/node_services/test_model_service_compute_integration.py (1)
src/omnibase_core/infrastructure/node_core_base.py (1)
_update_specialized_metrics(761-825)
tests/unit/models/nodes/node_services/test_model_service_effect_lifecycle.py (3)
tests/unit/models/nodes/node_services/test_model_service_orchestrator_lifecycle.py (1)
mock_create_task_impl(287-293)tests/unit/models/nodes/node_services/test_model_service_effect_health.py (1)
service_effect(56-123)src/omnibase_core/mixins/mixin_node_service.py (1)
start_service_mode(117-160)
tests/unit/models/nodes/node_services/conftest.py (1)
src/omnibase_core/mixins/mixin_node_service.py (1)
stop_service_mode(162-210)
tests/unit/models/nodes/node_services/test_model_service_orchestrator_lifecycle.py (2)
tests/unit/models/nodes/node_services/conftest.py (3)
mock_container(365-386)service_cleanup(267-361)register(287-290)src/omnibase_core/mixins/mixin_node_service.py (1)
start_service_mode(117-160)
🪛 Ruff (0.14.5)
tests/unit/models/nodes/node_services/conftest.py
71-72: try-except-pass detected, consider logging the exception
(S110)
71-71: Do not catch blind exception: Exception
(BLE001)
110-113: try-except-pass detected, consider logging the exception
(S110)
110-110: Do not catch blind exception: Exception
(BLE001)
162-163: try-except-pass detected, consider logging the exception
(S110)
162-162: Do not catch blind exception: Exception
(BLE001)
164-165: try-except-pass detected, consider logging the exception
(S110)
164-164: Do not catch blind exception: Exception
(BLE001)
166-167: try-except-pass detected, consider logging the exception
(S110)
166-166: Do not catch blind exception: Exception
(BLE001)
189-190: try-except-pass detected, consider logging the exception
(S110)
189-189: Do not catch blind exception: Exception
(BLE001)
191-192: try-except-pass detected, consider logging the exception
(S110)
191-191: Do not catch blind exception: Exception
(BLE001)
193-194: try-except-pass detected, consider logging the exception
(S110)
193-193: Do not catch blind exception: Exception
(BLE001)
328-329: try-except-pass detected, consider logging the exception
(S110)
328-328: Do not catch blind exception: Exception
(BLE001)
tests/unit/models/nodes/node_services/test_model_service_orchestrator_health.py
421-421: Unused function argument: timeout
(ARG001)
428-429: Remove exception handler; error is immediately re-raised
(TRY203)
543-543: Unused function argument: timeout
(ARG001)
586-586: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Code Quality
- GitHub Check: Smoke Tests
- GitHub Check: claude-review
🔇 Additional comments (19)
tests/unit/models/nodes/node_services/test_model_service_compute_integration.py (1)
106-110: Correctly distinguish sync vs async metrics helpers in test fixtureUsing
Mock()for_update_specialized_metrics(sync) andAsyncMock()for_update_processing_metrics(async) matches the production signatures and will avoid both “can’t await” errors and unawaited coroutine warnings. This is an appropriate and targeted fixture adjustment.tests/unit/models/nodes/node_services/test_model_service_effect_lifecycle.py (2)
97-97: LGTM: Proper initialization of_shutdown_eventfor test fixture.Aligns with the new event-based shutdown mechanism in MixinNodeService where
_shutdown_eventisNonebeforestart_service_mode()is called.
248-269: LGTM: Proper coroutine capture and cleanup pattern.The pattern correctly:
- Captures coroutines passed to
asyncio.create_task- Returns a mock task with proper
done()andcancel()methods- Closes captured coroutines after the test to prevent RuntimeWarning
This matches the established pattern used in other test files.
tests/unit/models/nodes/node_services/test_model_service_effect_health.py (1)
76-78: LGTM: Clear documentation of_shutdown_eventinitialization.The comment explains why
_shutdown_eventis initialized toNone- it mirrors the pre-start state of the service, allowingstop_service_mode()to work correctly when called on a service that was never started.tests/unit/models/nodes/node_services/test_model_service_orchestrator_lifecycle.py (5)
56-60: LGTM: Fixture properly integrates with cleanup infrastructure.The updated signature adds
service_cleanupdependency and the docstring explains the automatic cleanup purpose.
99-100: LGTM: Consistent_shutdown_eventinitialization.Initialization to
Nonematches the pre-start state expected by the mixin, with a helpful comment explaining it can be eitherNoneor an actual Event.
116-118: LGTM: Service registered for automatic cleanup.This ensures the fixture properly cleans up even if tests fail, preventing lingering async tasks.
284-305: LGTM: Coroutine capture pattern correctly applied.Matches the pattern used in other test files for preventing RuntimeWarning about unclosed coroutines.
491-496: LGTM: Clever generator-based awaitability pattern.Using
iter([])for__await__makes the mock task awaitable without creating an actual coroutine that would trigger unawaited warnings. This is a clean solution for mocking asyncio tasks.tests/unit/models/nodes/node_services/conftest.py (4)
5-16: LGTM: Clear documentation of session-level cleanup purpose.The docstring explains the root cause of the "Task was destroyed but it is pending!" error and how the atexit handler addresses it by running before Python's logging shutdown.
66-72: Bare exception handling is acceptable in this shutdown context.Static analysis flags
except Exception: passpatterns, but these are intentional for shutdown cleanup where:
- Logging may already be shut down (closed file handles)
- We want best-effort cleanup without propagating errors
- The alternative (crashing during cleanup) is worse
The pattern is appropriate here.
126-194: LGTM: Robust session-level asyncio cleanup.The fixture correctly:
- Registers an atexit handler for final cleanup
- Installs the silent exception handler to suppress shutdown noise
- Cancels and awaits all pending tasks
- Handles both session teardown and interpreter shutdown scenarios
299-342: LGTM: Proper shutdown event signaling in cleanup.The cleanup correctly:
- Signals
_shutdown_event.set()first to wake sleeping tasks- Sets
_shutdown_requestedflag- Waits briefly (0.3s) for health task to exit cleanly
- Falls back to cancellation if the task doesn't exit in time
- Handles both
TimeoutErrorandCancelledErrorappropriatelyThis aligns with the new event-based shutdown mechanism in the mixin.
tests/unit/models/nodes/node_services/test_model_service_orchestrator_health.py (6)
87-88: LGTM: Event initialized for fixture representing active service.Unlike other fixtures that use
None(pre-start state), this fixture represents a service that's actively being tested for health monitoring, so initializing_shutdown_eventas an actualasyncio.Event()is appropriate.
163-167: LGTM: Fixture cleanup signals shutdown event.Changing from
returntoyieldenables cleanup after the test, and setting the shutdown event ensures any pending tasks are woken up for clean termination.
417-434: LGTM: Correct mock for event-based wait pattern.The
mock_wait_forfunction correctly simulates the behavior ofasyncio.wait_for(shutdown_event.wait(), timeout=30):
- Returns immediately if shutdown event is set
- Uses short timeout (0.01s) for fast test iteration
- Re-raises TimeoutError as expected
The unused
timeoutparameter (flagged by static analysis) is intentional - we're replacing it with a fixed short timeout for testing.
500-517: LGTM: Proper verification of 30-second timeout.The test correctly captures and verifies that
asyncio.wait_foris called with a 30-second timeout, matching the implementation's interruptible wait pattern.
543-546: LGTM: Proper coroutine cleanup before raising CancelledError.Closing the coroutine with
coro.close()before raisingCancelledErrorprevents the "coroutine was never awaited" warning. This is the correct pattern for mocking cancellation.
619-627: LGTM: Test correctly verifies immediate exit when shutdown is pre-requested.When
_shutdown_requestedis alreadyTrue, the health monitor loop should exit immediately without entering the wait. The assertion thatwait_forwas not called validates this behavior.
Code Review - PR #90: Event-Based Shutdown SignalOverall AssessmentThis is a well-designed and thoroughly tested fix that addresses a real problem with graceful shutdown in asyncio services. The implementation is clean, follows ONEX conventions, and includes comprehensive test coverage. Strengths1. Problem Understanding
2. Implementation QualityArchitecture Alignment:
Code Quality:
3. Test CoverageComprehensive Test Suite:
Issues & RecommendationsCRITICAL: Patching asyncio.base_eventsLocation: tests/unit/models/nodes/node_services/conftest.py:75-123 This monkey-patches the global asyncio base class at module import time. Risky because:
Recommendation: Use pytest filterwarnings instead of global patching. WARNING: Exception SwallowingLocation: conftest.py:110-113 Bare except Exception: pass could hide real bugs, resource leaks, and state corruption. Recommendation: Be more specific about which exceptions to ignore. MINOR: Test Timeout Values1.0s assertion timeout might be tight on heavily loaded CI runners (20 parallel splits). Recommendation: Use CI-aware timeouts (2.0s on CI, 1.0s locally) to prevent flakes. Performance ConsiderationsPositive Impact:
CI Impact:
Recommendations SummaryMust Fix (Before Merge):
Should Fix:
Final VerdictRecommendation: APPROVE with requested changes Excellent software engineering demonstrated here. The only concern is the global asyncio patching in test infrastructure. Estimated Risk: Low (after addressing asyncio patching) Great work! Looking forward to seeing this merged! |
- Add _shutdown_event.set() to signal handler for symmetry with explicit shutdown - Rename unused input_state parameter to _input_state (fixes ARG002 linter warning) - Loosen timing assertions from 1.0s to 2.0s threshold for CI stability
Code Review - PR #90: Event-Based Shutdown SignalSummaryThis PR implements a well-designed solution to eliminate asyncio shutdown warnings by using event-based signaling for immediate task cancellation. The implementation is clean, well-tested, and follows omnibase_core architectural patterns. ✅ Strengths1. Excellent Problem AnalysisThe PR correctly identifies the root cause: a 30-second 2. Clean Implementation# Lazy initialization - event created only when service starts
self._shutdown_event = asyncio.Event() # Line 138
# Event-based interruptible wait with timeout
await asyncio.wait_for(self._shutdown_event.wait(), timeout=30) # Lines 443The use of 3. Comprehensive Test Coverage
4. Proper Shutdown SymmetryThe shutdown event is set consistently across all shutdown paths:
This ensures immediate wakeup regardless of how shutdown is triggered. 5. Thread Safety DocumentationLines 73-77 clearly document that 6. Fixture ImprovementsThe
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
src/omnibase_core/mixins/mixin_node_service.py (2)
404-414: Consider clarifying the fallback path.The logic is correct, but the
elsebranch (lines 413-414) that falls back toasyncio.sleep(0.1)seems overly defensive. The_shutdown_eventis always created instart_service_mode()before this loop runs, so this fallback "shouldn't happen" as the inline comment suggests.Consider either:
- Removing the else branch entirely and relying on the None check to raise an error if the event is missing (fail-fast)
- Adding a more explicit comment explaining hypothetical scenarios where the event might be None
The current implementation is safe and defensive, so this is a minor maintainability suggestion.
437-451: LGTM! This is the key improvement for shutdown timing.Replacing the fixed 30-second sleep with an interruptible wait is the primary value of this PR, enabling immediate response to shutdown signals instead of waiting up to 30 seconds. The comment clearly explains the benefit.
Minor note: The same optional refactoring suggestion from the service event loop applies here regarding the fallback
elsebranch (lines 449-451), but this doesn't impact correctness.tests/unit/mixins/test_mixin_node_service_shutdown.py (3)
62-104: Test is correct but timing threshold is quite loose.The test correctly validates immediate wakeup behavior. However, the 2.0-second threshold (lines 100, 103-104) is quite generous. While the PR objectives explain this accounts for "CI scheduler delays under heavy load," this loose threshold might hide actual performance regressions.
Consider adding a comment referencing the CI stability rationale for the 2.0s threshold, or tracking this as a known limitation in case tighter bounds become feasible in the future.
106-119: Upper bound is too loose for timeout verification.The upper bound check of 2.0 seconds (line 119) is excessively loose for validating a 0.5-second timeout. This could mask scenarios where the timeout mechanism isn't working correctly. An elapsed time of 1.5+ seconds would still pass but indicates something is wrong.
Consider tightening to
assert 0.4 < elapsed < 0.8unless there's specific evidence of CI instability requiring the 2.0s bound.
191-251: LGTM! Comprehensive test of aclose() behavior.The test correctly validates that
aclose()triggers immediate wakeup. Cleanup handling is thorough with proper task cancellation on both success (lines 238-244) and failure paths (lines 231-235).Minor note: The cleanup code is duplicated between the failure path and after the completion check. This is acceptable for test clarity, though it could be extracted to a helper if more tests follow this pattern.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/omnibase_core/mixins/mixin_node_service.py(9 hunks)tests/unit/mixins/test_mixin_node_service_shutdown.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Always use Poetry for Python package management and task execution - never use pip or python directly
Always use ModelOnexError with EnumCoreErrorCode for structured error handling, never raise generic Exception
Use @standard_error_handling decorator on async functions to eliminate boilerplate try/catch logic
Use duck typing with protocols instead of isinstance checks - protocols define contracts through interfaces, not runtime type checking
Always use 100% strict mypy type checking - all functions must have type annotations and no untyped definitions are allowed
Run code through black formatter, isort for import sorting, and ruff linter before committing
Use async/await pattern with asyncio for asynchronous operations - never use threading for I/O operations
Use ModelEventEnvelope for inter-service event-driven communication and process event payloads through envelope pattern
Files:
src/omnibase_core/mixins/mixin_node_service.pytests/unit/mixins/test_mixin_node_service_shutdown.py
src/omnibase_core/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use container.get_service('ProtocolName') for dependency resolution by protocol name, never by concrete class name
Files:
src/omnibase_core/mixins/mixin_node_service.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Maintain minimum 60% code coverage with pytest tests - unit tests go in tests/unit/, integration tests in tests/integration/
Run pytest tests with parallelization disabled (-n 0 -xvs) for debugging and use standard pytest run for CI
Use @pytest.mark decorators (unit, integration, slow, smoke, performance) to categorize tests appropriately
Files:
tests/unit/mixins/test_mixin_node_service_shutdown.py
🧬 Code graph analysis (1)
tests/unit/mixins/test_mixin_node_service_shutdown.py (1)
src/omnibase_core/mixins/mixin_node_service.py (3)
get_service_health(322-352)aclose(212-229)stop_service_mode(162-210)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Smoke Tests
- GitHub Check: Code Quality
- GitHub Check: claude-review
🔇 Additional comments (9)
src/omnibase_core/mixins/mixin_node_service.py (5)
91-91: LGTM! Clean lazy initialization pattern.The type annotation correctly reflects the optional nature, and initialization to None with a clear comment about lazy creation in
start_service_mode()is well-documented.Also applies to: 114-115
135-138: LGTM! Proper lazy initialization of shutdown event.The event is created at the right time (when the service starts and event loop is available), and the comment clearly explains the immediate-wakeup benefit for sleeping tasks.
178-182: LGTM! Correct shutdown signaling with defensive check.The None check before calling
set()is appropriate defensive programming, and the comment clearly explains the benefit of avoiding the 30-second sleep interval.
220-222: LGTM! Consistent shutdown signaling in aclose().The pattern matches
stop_service_mode(), ensuring immediate wakeup before cleaning up the health task. This is the correct sequence for async cleanup.
701-704: LGTM! Correct signal handler integration.Setting the shutdown event in the signal handler ensures consistent immediate-wakeup behavior whether shutdown is triggered by signals (SIGTERM/SIGINT) or explicit calls to
stop_service_mode()/aclose(). The defensive None check is appropriate here.tests/unit/mixins/test_mixin_node_service_shutdown.py (4)
38-56: LGTM! Clean test helper implementation.The
MinimalServiceNodeprovides just enough implementation to test shutdown behavior without unnecessary complexity. The use ofobject.__setattr__for_node_idis consistent with the pattern inMixinNodeService.
121-189: LGTM! Good integration test with proper cleanup.This test validates the actual health monitor loop pattern used in
MixinNodeService. The manual state initialization (lines 131-134) is necessary for isolated testing, and the cleanup logic (lines 177-183) properly handles task cancellation.
253-301: LGTM! Clean test of service event loop behavior.The test correctly validates that the service event loop responds immediately to shutdown signals despite the short 0.1s iteration timeout. Tracking loop iterations (line 297) adds useful validation that the loop actually executed.
304-357: LGTM! Comprehensive integration test coverage.These integration tests cover important scenarios:
- Event is properly set during shutdown (lines 307-329)
- Lazy initialization pattern is verified (lines 331-337)
- Idempotency of multiple signals (lines 339-357)
The tests are clear, focused, and validate key properties of the shutdown event mechanism.
- Add global asyncio patching warning to conftest.py docstring - Fix signal handler thread safety with loop.call_soon_threadsafe() - Implement environment-aware timing thresholds (0.6s local, 2.0s CI) - Add type narrowing for shutdown event in test closures
Code Review: Event-Based Shutdown Signal for Health Monitor LoopOverall Assessment: ✅ EXCELLENT - This is a high-quality fix that addresses a real problem with a well-designed solution. The implementation follows ONEX best practices and includes comprehensive test coverage. 🎯 Problem & SolutionProblem: Health monitor loop had a 30-second
Solution: Event-based shutdown signaling using ✅ Strengths1. Excellent Architecture
2. Comprehensive Test Coverage (393 lines!)
3. Excellent Code Quality
4. Documentation & Safety
🔍 Code Quality DetailsImplementation Pattern (Perfect ✨)# ✅ Interruptible wait pattern - idiomatic asyncio
if self._shutdown_event is not None:
try:
await asyncio.wait_for(self._shutdown_event.wait(), timeout=30)
break # Shutdown signaled
except TimeoutError:
pass # Normal timeout - continueThis is exactly how asyncio shutdown coordination should be done. Signal Handler Safety (Excellent 👍)# ✅ Thread-safe signal handler (lines 704-710)
loop = asyncio.get_running_loop()
loop.call_soon_threadsafe(self._shutdown_event.set)Correctly uses
|
| Standard | Status | Evidence |
|---|---|---|
| Poetry Usage | ✅ Pass | Pre-commit checks use poetry |
| Type Hints | ✅ Pass | _shutdown_event: asyncio.Event | None |
| Error Handling | ✅ Pass | Proper exception handling in signal handlers |
| Thread Safety | ✅ Pass | Documented in docstring + correct implementation |
| Test Coverage | ✅ Pass | 393 lines of new tests |
| Documentation | ✅ Pass | Clear docstrings and comments |
🎯 Recommendations
Required Before Merge
NONE - This PR is ready to merge as-is.
Optional Enhancements (Future)
- Consider extracting
get_timing_threshold()to a shared test utility (DRY principle) - Add metrics for shutdown timing in production (observability)
- Consider backport to other projects using
MixinNodeService
📝 Final Verdict
Status: ✅ APPROVED - Ready to Merge
Rationale:
- Solves a real, annoying problem (test suite warnings)
- Implementation is clean, idiomatic, and well-tested
- No breaking changes to public API
- Follows all ONEX architectural patterns
- Comprehensive test coverage (8 test methods covering edge cases)
- Excellent documentation and comments
Nice work on this fix! 🎉 The event-based approach is the correct pattern for asyncio shutdown coordination, and the implementation is thorough and professional.
Review completed by: Claude Code (Sonnet 4.5)
Review date: 2025-11-26
Files reviewed: 9 files, +846/-67 lines
Test coverage: 393 new test lines
…ar snapshot [OMN-2655] Add three new Pydantic event models as the Core-layer view for workflow automation event types defined in OMN-2655 Part 2 (omnibase_core scope): - ModelGitHubPRStatusEvent — GitHub PR triage event (topic: onex.evt.github.pr-status.v1) - ModelGitHookEvent — Git hook invocation event (topic: onex.evt.git.hook.v1) - ModelLinearSnapshotEvent — Linear workspace snapshot event (topic: onex.evt.linear.snapshot.v1) All models extend ModelRuntimeEventBase with frozen=True, extra="forbid". All SPI contract fields (ContractGitHubPRStatusEvent, ContractGitHookEvent, ContractLinearSnapshotEvent from omnibase_spi PR #90) are present as a superset, satisfying the SPI ⊆ Core alignment invariant. Topic constants added to constants_event_types.py. All three models and their topic constants exported from models/events/__init__.py.
…ar snapshot [OMN-2655] (#531) * feat(events): add Core event models for GitHub PR, Git hook, and Linear snapshot [OMN-2655] Add three new Pydantic event models as the Core-layer view for workflow automation event types defined in OMN-2655 Part 2 (omnibase_core scope): - ModelGitHubPRStatusEvent — GitHub PR triage event (topic: onex.evt.github.pr-status.v1) - ModelGitHookEvent — Git hook invocation event (topic: onex.evt.git.hook.v1) - ModelLinearSnapshotEvent — Linear workspace snapshot event (topic: onex.evt.linear.snapshot.v1) All models extend ModelRuntimeEventBase with frozen=True, extra="forbid". All SPI contract fields (ContractGitHubPRStatusEvent, ContractGitHookEvent, ContractLinearSnapshotEvent from omnibase_spi PR #90) are present as a superset, satisfying the SPI ⊆ Core alignment invariant. Topic constants added to constants_event_types.py. All three models and their topic constants exported from models/events/__init__.py. * chore: reflow pre-commit hook description and bump lockfile to 0.19.0 Reformats the check-local-paths hook description to a single paragraph (no mid-sentence line-break) and regenerates uv.lock to reflect the 0.19.0 package version. * fix(events): convert TriageState Literal to StrEnum per governance check
1 participant
Summary
asyncio.Eventbased shutdown signaling toMixinNodeService._health_monitor_loop()for immediate response to shutdown signalsasyncio.sleep()with interruptiblewait_for()that responds to shutdown eventsProblem
The health monitor loop had a 30-second
asyncio.sleep(30)that didn't respond immediately to shutdown signals. When tests ended, the task would remain pending during the sleep, causing:Solution
Added an
asyncio.Eventbased shutdown signaling mechanism:_shutdown_event: asyncio.Event | None- created when service startsawait asyncio.sleep(30)toawait asyncio.wait_for(shutdown_event.wait(), timeout=30)- allows immediate wakeupstop_service_mode()andaclose()now set the shutdown event to wake up sleeping tasks immediatelyhasattr()andgetattr()to handle Pydantic models safelyKey Changes
_shutdown_eventtype annotation_shutdown_event = Nonein__init__start_service_mode()stop_service_mode()aclose()_service_event_loop()_health_monitor_loop()Test plan
Summary by CodeRabbit
Refactor
Tests
Chores
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.