Fix DimseConnector retry failure by resetting assoc in abort_connection #283
Conversation
## Problem
When abort_connection() was called during error handling, it would abort
the DICOM association but fail to reset self.assoc to None. This caused
subsequent retry attempts to fail with AssertionError("A former connection
was not closed properly.") because open_connection() detected a stale
association reference.
## Root Cause
The abort_connection() method (line 204-207) was missing the critical
`self.assoc = None` cleanup that close_connection() correctly performed
(line 202). This inconsistency prevented the stamina retry mechanism from
working properly - retries would fail immediately at the connection level
instead of establishing a new connection.
## Solution
Added `self.assoc = None` to abort_connection() after calling abort(),
matching the behavior of close_connection(). This allows retry decorators
to detect that no connection exists and successfully open a new connection
on retry attempts.
## Testing
- Added comprehensive test suite in test_dimse_connector.py
- Tests verify retry behavior works correctly with the fix
- All existing tests continue to pass
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughA 1-line fix to Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used🧬 Code graph analysis (1)adit/core/tests/utils/test_dimse_connector.py (4)
🪛 Ruff (0.14.10)adit/core/tests/utils/test_dimse_connector.py43-43: 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). (2)
🔇 Additional comments (6)
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 |
Summary of ChangesHello @samuelvkwong, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves a significant issue preventing DICOM operation retries from functioning as intended. Previously, transient network failures would lead to an Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses a critical bug where DICOM connection retries failed due to an improperly cleared association reference in abort_connection. The fix is simple and correct, adding self.assoc = None to ensure the connection state is properly reset. The accompanying new test suite is comprehensive, verifying the fix, confirming existing behavior, and testing edge cases. My review found the code changes to be of high quality. I've only added a couple of minor suggestions in the new test file to improve mocking consistency.
| class TestDimseConnectorRetry: | ||
| """Test retry behavior and connection cleanup in DimseConnector.""" | ||
|
|
||
| @patch("adit.core.utils.retry_config.settings.ENABLE_STAMINA_RETRY", True) |
There was a problem hiding this comment.
For consistency with the use of the mocker fixture from pytest-mock throughout the test, consider using mocker.patch to modify the settings instead of the unittest.mock.patch decorator. This keeps the mocking style uniform within the test function.
For example, you could remove this decorator and add the following line at the beginning of the test body:
mocker.patch("adit.core.utils.retry_config.settings.ENABLE_STAMINA_RETRY", True)| # Operation should have succeeded on retry | ||
| assert call_count == 2 | ||
|
|
||
| @patch("adit.core.utils.retry_config.settings.ENABLE_STAMINA_RETRY", True) |
There was a problem hiding this comment.
For consistency with the use of the mocker fixture from pytest-mock throughout the test, consider using mocker.patch to modify the settings instead of the unittest.mock.patch decorator. This keeps the mocking style uniform within the test function.
For example, you could remove this decorator and add the following line at the beginning of the test body:
mocker.patch("adit.core.utils.retry_config.settings.ENABLE_STAMINA_RETRY", True)
Code Review - PR #283: Fix DimseConnector retry failureSummaryThis PR fixes a critical bug where DICOM connection retries would fail after ✅ Strengths
🔍 Code Quality AssessmentThe Fix (dimse_connector.py:208) - Rating: 5/5
Test Quality (test_dimse_connector.py) - Rating: 4.5/5
🐛 Potential IssuesNone identified. The fix is correct and safe. 🎯 Suggestions for Improvement1. Consider Refactoring Duplicate Code (Low Priority) 2. Test Mock Complexity (Minor) 3. Test Documentation Enhancement (Minor) 🔒 Security ConsiderationsNo concerns. This is defensive code that prevents stale state - actually improves reliability. ⚡ Performance ConsiderationsPositive impact. The fix enables proper retry behavior, reducing task failures and improving overall system resilience. No performance regression risk. 📊 Test CoverageExcellent. All critical paths covered:
Coverage delta: +160 lines (all test code), 0 uncovered lines in implementation. 📋 Alignment with Project StandardsChecking against CLAUDE.md guidelines:
🎓 Architecture FitThis fix correctly handles the network layer retry (stamina) as documented in ✅ Final VerdictAPPROVED - This is an excellent bug fix PR that:
The suggestions above are minor polish opportunities, not blockers. 🤖 Review completed using repository guidelines from CLAUDE.md |
Summary
Fixes a critical bug where DICOM connection retries would fail with
AssertionError: A former connection was not closed properly.This prevented the stamina retry mechanism from recovering from transient network errors.Problem
The
abort_connection()method inDimseConnectorwas not resettingself.assoc = Noneafter aborting a connection. When a DICOM operation failed and the retry mechanism attempted to reconnect,open_connection()would detect the stale association reference and raise an AssertionError, preventing any retries from succeeding.Error Stack Trace
Root Cause
Inconsistency between two connection cleanup methods:
close_connection()(line 202): Correctly setsself.assoc = Noneafter releasingabort_connection()(line 207): Missingself.assoc = Noneafter abortingThis caused the
@connect_to_serverdecorator's retry logic to fail becauseself.assoc and self.assoc.is_alive()check would find a non-None association that was no longer alive.Solution
Added
self.assoc = Nonetoabort_connection()after callingabort(), matching the behavior ofclose_connection(). This allows the retry decorator to correctly detect that no active connection exists and establish a new one.Code Change
Testing
test_dimse_connector.pywith 4 teststest_abort_connection_resets_assoc_allowing_retry: Verifies retry works after aborttest_close_connection_properly_resets_assoc: Confirms normal close path workstest_open_connection_fails_if_previous_not_closed: Validates the safety checktest_abort_connection_with_no_connection: Tests edge case handlingImpact
Related
Resolves the error reported in the production logs where DICOM C-GET operations were failing with connection cleanup assertions.
🤖 Generated with Claude Code
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.