fix: connector doesn't recover from failure after network issue #328
Conversation
mohammad-arif662
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #328 +/- ##
==========================================
+ Coverage 75.48% 75.97% +0.48%
==========================================
Files 38 38
Lines 4083 4083
Branches 487 488 +1
==========================================
+ Hits 3082 3102 +20
+ Misses 817 796 -21
- Partials 184 185 +1
🚀 New features to boost your workflow:
|
|
/gemini review |
![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 correctly addresses a potential stability issue by changing the PublishConnectorStatus method from async void to async Task. This prevents unhandled exceptions from crashing the application and allows for proper error propagation. My review includes a few suggestions to align the naming of this asynchronous method with the project's style guide by adding the Async suffix. Overall, this is a solid and important fix.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request correctly addresses a critical issue where a network failure during a status update could crash the connector. By changing PublishConnectorStatus from async void to async Task and awaiting the calls, unhandled exceptions are now properly propagated and managed, allowing the connector to recover gracefully. A comprehensive test case has been added to validate this fix. My review includes one high-severity suggestion to improve exception handling consistency in PublishConnectorStatusAsync by rethrowing the caught ResponseException, in line with the repository's style guide.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request correctly addresses a critical issue where an unhandled exception from an async void method could crash the connector during network failures. Changing PublishConnectorStatus to return async Task and awaiting it is the right approach, ensuring exceptions are properly propagated and handled. The addition of an integration test to simulate and verify this scenario is a great improvement. I've identified one issue in the test utility code that could affect the reliability of the new test.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request addresses a critical issue where a network failure during a simulation run could crash the connector due to an unhandled exception in an async void method. The change to use async Task and properly await the operation is the correct approach to fix this.
The addition of a new unit test to specifically cover this network failure scenario is excellent and greatly improves the robustness of the codebase.
I've found one critical issue in the implementation where an exception is caught but not re-thrown. This prevents the error from being handled correctly by the calling method, which contradicts the stated goal of the PR. I have left a specific comment with a suggestion to fix this.
|
why does this PR say "PROSPER"? this is a generic library |
mohammad-arif662
changed the title
mohammad-arif662
added
the
waiting-for-risk-review
polomani
added
risk-review-ongoing
3 participants
Problem
When a simulation was running and the system lost internet connectivity, the connector would terminate unexpectedly, leaving the run stuck in "running" status indefinitely. It happens because
PublishConnectorStatuswas declared asasync void, which cannot be awaited by the caller and causes unhandled exception typeHttpRequestExceptionto crash the process.Fix
Changed
PublishConnectorStatusfromasync voidtoasync Taskand added proper await calls. Now network exceptions propagate to the outer try/catch block which allows the connector to continue running and recover when connectivity is restored and marks the run as "failure".