fix: Resolve #1522 - fix(ts): clean up lifecycle signal listeners#1523
fix: Resolve #1522 - fix(ts): clean up lifecycle signal listeners#1523washim0988-art wants to merge 2 commits into
Conversation
Signed-off-by: washim0988-art <islowashin@gmail.com>
📝 WalkthroughWalkthroughReplaces the prior lifecycle implementation with a smaller ChangesServer lifecycle cleanup
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@fix_1522.py`:
- Around line 11-30: Update start() to capture the current SIGINT and SIGTERM
handlers before installing this class’s handlers, and update stop() to restore
those saved handlers instead of unconditionally using signal.SIG_DFL. After
unregistering/restoring and stopping health_polling_loop, clear the handler
references and saved signal-handler state so repeated stop() calls are
idempotent.
- Around line 68-76: Update test_lifecycle to mock atexit.register/unregister
and signal.signal before starting ServerLifecycle, then assert the expected
registration, unregistration, and signal-reset calls. Remove the private
atexit._exithandlers assertion and avoid inspecting or mutating real process
signal state.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| def start(self): | ||
| self.exit_handler = self.exit_handler_func | ||
| self.sigint_handler = self.sigint_handler_func | ||
| self.sigterm_handler = self.sigterm_handler_func | ||
|
|
||
| atexit.register(self.exit_handler) | ||
| signal.signal(signal.SIGINT, self.sigint_handler) | ||
| signal.signal(signal.SIGTERM, self.sigterm_handler) | ||
|
|
||
| def stop(self): | ||
| if self.exit_handler: | ||
| atexit.unregister(self.exit_handler) | ||
| if self.sigint_handler: | ||
| signal.signal(signal.SIGINT, signal.SIG_DFL) | ||
| if self.sigterm_handler: | ||
| signal.signal(signal.SIGTERM, signal.SIG_DFL) | ||
|
|
||
| if self.health_polling_loop: | ||
| self.health_polling_loop.stop() | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve original signal handlers and clear state for idempotency.
Currently, start() overwrites any existing signal handlers without saving them, and stop() unconditionally restores them to signal.SIG_DFL. This can break integration with other libraries or host applications that register their own signal handlers.
Furthermore, stop() does not clear the handler references (self.exit_handler, etc.), which means subsequent calls to stop() will unnecessarily re-execute the unregistration logic, leaving an idempotency gap.
🛠️ Proposed fix to save/restore signals and clear state
def start(self):
self.exit_handler = self.exit_handler_func
self.sigint_handler = self.sigint_handler_func
self.sigterm_handler = self.sigterm_handler_func
atexit.register(self.exit_handler)
- signal.signal(signal.SIGINT, self.sigint_handler)
- signal.signal(signal.SIGTERM, self.sigterm_handler)
+ self._orig_sigint = signal.signal(signal.SIGINT, self.sigint_handler)
+ self._orig_sigterm = signal.signal(signal.SIGTERM, self.sigterm_handler)
def stop(self):
if self.exit_handler:
atexit.unregister(self.exit_handler)
+ self.exit_handler = None
if self.sigint_handler:
- signal.signal(signal.SIGINT, signal.SIG_DFL)
+ signal.signal(signal.SIGINT, getattr(self, '_orig_sigint', signal.SIG_DFL))
+ self.sigint_handler = None
if self.sigterm_handler:
- signal.signal(signal.SIGTERM, signal.SIG_DFL)
+ signal.signal(signal.SIGTERM, getattr(self, '_orig_sigterm', signal.SIG_DFL))
+ self.sigterm_handler = None
if self.health_polling_loop:
self.health_polling_loop.stop()
+ self.health_polling_loop = None📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def start(self): | |
| self.exit_handler = self.exit_handler_func | |
| self.sigint_handler = self.sigint_handler_func | |
| self.sigterm_handler = self.sigterm_handler_func | |
| atexit.register(self.exit_handler) | |
| signal.signal(signal.SIGINT, self.sigint_handler) | |
| signal.signal(signal.SIGTERM, self.sigterm_handler) | |
| def stop(self): | |
| if self.exit_handler: | |
| atexit.unregister(self.exit_handler) | |
| if self.sigint_handler: | |
| signal.signal(signal.SIGINT, signal.SIG_DFL) | |
| if self.sigterm_handler: | |
| signal.signal(signal.SIGTERM, signal.SIG_DFL) | |
| if self.health_polling_loop: | |
| self.health_polling_loop.stop() | |
| def start(self): | |
| self.exit_handler = self.exit_handler_func | |
| self.sigint_handler = self.sigint_handler_func | |
| self.sigterm_handler = self.sigterm_handler_func | |
| atexit.register(self.exit_handler) | |
| self._orig_sigint = signal.signal(signal.SIGINT, self.sigint_handler) | |
| self._orig_sigterm = signal.signal(signal.SIGTERM, self.sigterm_handler) | |
| def stop(self): | |
| if self.exit_handler: | |
| atexit.unregister(self.exit_handler) | |
| self.exit_handler = None | |
| if self.sigint_handler: | |
| signal.signal(signal.SIGINT, getattr(self, '_orig_sigint', signal.SIG_DFL)) | |
| self.sigint_handler = None | |
| if self.sigterm_handler: | |
| signal.signal(signal.SIGTERM, getattr(self, '_orig_sigterm', signal.SIG_DFL)) | |
| self.sigterm_handler = None | |
| if self.health_polling_loop: | |
| self.health_polling_loop.stop() | |
| self.health_polling_loop = None |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@fix_1522.py` around lines 11 - 30, Update start() to capture the current
SIGINT and SIGTERM handlers before installing this class’s handlers, and update
stop() to restore those saved handlers instead of unconditionally using
signal.SIG_DFL. After unregistering/restoring and stopping health_polling_loop,
clear the handler references and saved signal-handler state so repeated stop()
calls are idempotent.
Refactor ServerLifecycle to accept handlers as parameters and preserve original signal handlers. Update test to verify lifecycle behavior with mocked handlers.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
fix_1522.py (1)
30-44: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winClear all handler references to ensure full idempotency.
While
self.sigint_handlerandself.sigterm_handlerare properly set toNoneafter restoration,self.exit_handlerandself.health_polling_loopare not. Ifstop()is invoked multiple times,self.health_polling_loop.stop()will be called repeatedly, which may raise errors depending on the loop's implementation.Setting these to
Noneensures repeated calls are perfectly safe and prevents holding onto references longer than needed.🛠️ Proposed fix to clear remaining state
def stop(self): if self._registered_exit and self.exit_handler: atexit.unregister(self.exit_handler) self._registered_exit = False + self.exit_handler = None if self.sigint_handler and self._original_sigint is not None: signal.signal(signal.SIGINT, self._original_sigint) self.sigint_handler = None + self._original_sigint = None if self.sigterm_handler and self._original_sigterm is not None: signal.signal(signal.SIGTERM, self._original_sigterm) self.sigterm_handler = None + self._original_sigterm = None if self.health_polling_loop: self.health_polling_loop.stop() + self.health_polling_loop = None🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@fix_1522.py` around lines 30 - 44, Update stop() to clear all released handler references: set self.exit_handler to None after unregistering it and set self.health_polling_loop to None after stopping it. Preserve the existing guards and SIGINT/SIGTERM cleanup so repeated stop() calls remain safe.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@fix_1522.py`:
- Around line 2-3: Add the missing atexit module import alongside the existing
imports in fix_1522.py so the atexit.register and atexit.unregister calls used
by start() and stop() resolve directly at runtime.
---
Duplicate comments:
In `@fix_1522.py`:
- Around line 30-44: Update stop() to clear all released handler references: set
self.exit_handler to None after unregistering it and set
self.health_polling_loop to None after stopping it. Preserve the existing guards
and SIGINT/SIGTERM cleanup so repeated stop() calls remain safe.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| import signal | ||
| from unittest.mock import patch, MagicMock |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Add missing atexit import.
The atexit module is used in start() and stop(), but it is not imported. This will raise a NameError in production. (The test likely passed because patch('atexit.register') imports the module globally under the hood, masking the issue here).
🐛 Proposed fix
import signal
+import atexit
from unittest.mock import patch, MagicMock📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import signal | |
| from unittest.mock import patch, MagicMock | |
| import signal | |
| import atexit | |
| from unittest.mock import patch, MagicMock |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@fix_1522.py` around lines 2 - 3, Add the missing atexit module import
alongside the existing imports in fix_1522.py so the atexit.register and
atexit.unregister calls used by start() and stop() resolve directly at runtime.
1 participant
Resolves #1522
Built autonomously by Cloud Agent.
Bounty payout address: Gq46qirFLJY3qptAWkAmAeDfGVAE4MYYGTcRmpKjsyR
Summary by CodeRabbit
New Features
Tests