Conversation
…nals (#150) This pull request significantly expands signal handler registration to cover all catchable termination signals across Unix/Linux/macOS and Windows platforms, improves error handling and defensive programming in signal handlers, and enhances test coverage for signal handler edge cases. **Signal handler expansion:** * Expanded `setup_signal_handler_logging()` to register handlers for all catchable termination signals instead
… code organization Extracted `_signal_handler` and `_register_signal` as module-level functions from the nested scope within `setup_signal_handler_logging()`, enabling direct testing without complex mocking. Updated `_signal_handler` to use `types.FrameType | None` type hint instead of `object` for the frame parameter. Simplified test cases by removing unnecessary `importlib.reload()` calls and mock extraction logic, directly importing
Contributor
There was a problem hiding this comment.
Pull request overview
This pull request enhances the signal handler logging infrastructure in deephaven_mcp._logging by expanding coverage to all catchable termination signals across Unix-like and Windows platforms. The changes introduce robust defensive error handling, improve code maintainability through refactoring, and add comprehensive test coverage for various edge cases and failure scenarios.
Changes:
- Expanded signal handler registration to cover all catchable termination signals (SIGTERM, SIGINT, SIGABRT, SIGHUP, SIGQUIT, SIGUSR1, SIGUSR2, SIGALRM, SIGPIPE, SIGBREAK)
- Refactored signal handler logic into separate
_signal_handlerand_register_signalfunctions with defensive error handling - Added comprehensive tests for defensive logging, platform-specific signals, error handling, and edge cases
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| src/deephaven_mcp/_logging.py | Refactored signal handler registration into modular functions, expanded signal coverage, added defensive error handling with stderr fallback, and improved documentation |
| tests/test__logging.py | Added 8 new test functions covering defensive logging failures, platform-specific signals, multiple error scenarios, and updated existing tests to verify multiple signal registration |
…ad of direct function imports Changed test imports from `from deephaven_mcp._logging import _signal_handler` to `import deephaven_mcp._logging as logging_mod` and updated function calls to use `logging_mod._signal_handler()`. This improves test isolation and follows the pattern established in the earlier signal handler refactoring.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
This pull request significantly expands and improves the signal handler logging infrastructure in
deephaven_mcp._logging. It now registers handlers for all catchable termination signals across Unix-like and Windows platforms, provides robust logging for signal-related shutdowns, and includes defensive error handling to ensure visibility even if logging fails. Comprehensive tests have been added to verify the correctness and resilience of the new signal handler logic.Signal handler infrastructure improvements:
src/deephaven_mcp/_logging.py: Expanded signal handler registration to cover all catchable termination signals (including SIGTERM, SIGINT, SIGABRT, SIGHUP, SIGQUIT, SIGUSR1, SIGUSR2, SIGALRM, SIGPIPE, SIGBREAK), and clarified documentation to distinguish between catchable and non-catchable signals. [1] [2] [3]src/deephaven_mcp/_logging.py: Refactored signal handler registration logic into_signal_handlerand_register_signalfunctions, improving maintainability and error handling. Signal registration failures are now logged as debug messages, and the handler is defensive against logging failures, falling back to stderr as a last resort.Testing and robustness:
tests/test__logging.py: Added and expanded tests to verify multiple signals are registered, defensive logging behavior, platform-specific signal handling, and graceful error handling for signal registration failures (OSError, RuntimeError, missing signals). [1] [2] [3]These changes make the logging module much more robust and informative for debugging shutdowns and signal-related process events, especially in production and containerized environments.