fix(ConsumerService): ensure study creation error notification is always emitted on case import failure#1027
fix(ConsumerService): ensure study creation error notification is always emitted on case import failure#1027freddidierRTE wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughThe study creation failure handler now catches exceptions from study deletion and logs a warning instead of propagating the deletion failure. ChangesStudy failure handling
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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.
🧹 Nitpick comments (1)
src/main/java/org/gridsuite/study/server/service/ConsumerService.java (1)
358-362: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPreserve the deletion exception in the warning log.
Logging only
e.getMessage()discards the exception type and stack trace, making intermittentObjectOptimisticLockingFailureExceptionfailures difficult to diagnose.Proposed fix
} catch (Exception e) { - LOGGER.warn("Could not delete study during case import failure handling: {}", e.getMessage()); + LOGGER.warn("Could not delete study '{}' during case import failure handling", studyUuid, e); }🤖 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 `@src/main/java/org/gridsuite/study/server/service/ConsumerService.java` around lines 358 - 362, Update the catch block around studyService.deleteStudyIfNotCreationInProgress in ConsumerService to pass the caught exception to LOGGER.warn, preserving its type and stack trace while retaining the existing contextual warning message.
🤖 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.
Nitpick comments:
In `@src/main/java/org/gridsuite/study/server/service/ConsumerService.java`:
- Around line 358-362: Update the catch block around
studyService.deleteStudyIfNotCreationInProgress in ConsumerService to pass the
caught exception to LOGGER.warn, preserving its type and stack trace while
retaining the existing contextual warning message.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e249ff96-c619-4408-a300-f3f9159bc1bc
📒 Files selected for processing (1)
src/main/java/org/gridsuite/study/server/service/ConsumerService.java
…ays emitted on case import failure Isolate deleteStudyIfNotCreationInProgress in its own try/catch in consumeCaseImportFailed so that emitStudyCreationError is always called even if the deletion fails due to a concurrent transaction that already cleaned up the StudyCreationRequestEntity. This was causing a flaky test (testCreateStudyWithErrorDuringCaseImport) when running the full test suite because the WireMock stub stubImportNetworkWithServerError simulates a situation that does not exist in production: it returns a 5xx HTTP response AND sends a DLX message simultaneously. In reality, a synchronous 5xx means the network conversion server never processed the request and therefore never publishes on case.import.start, so no DLX message would ever be generated. This artificial combination caused the catch block in createStudy and the DLX consumer to race against each other trying to delete the same StudyCreationRequestEntity. Depending on thread scheduling, the consumer's delete could throw an ObjectOptimisticLockingFailureException, which was silently caught at the outer level, preventing emitStudyCreationError from being called and leaving the test waiting for a message that never arrived. Signed-off-by: freddidierRTE <[email protected]>
|
| if (receiver.getCaseImportAction() == CaseImportAction.STUDY_CREATION) { | ||
| try { | ||
| studyService.deleteStudyIfNotCreationInProgress(studyUuid, userId); | ||
| } catch (Exception e) { |
There was a problem hiding this comment.
maybe catch ObjectOptimisticLockingFailureException instead and add comment that if there are races to delete the study we swaller the exception ?
| try { | ||
| studyService.deleteStudyIfNotCreationInProgress(studyUuid, userId); | ||
| } catch (Exception e) { | ||
| LOGGER.warn("Could not delete study during case import failure handling: {}", e.getMessage()); |
There was a problem hiding this comment.
maybe add the same handling or at least a comment on the other places in the code participating in this race ? If I understand correctly, StudyService::createStudy try catch ? To me even if it's not happening right now, we could some day be in a situation where the call to network-conversion-server to return 500 even though it published a message (maybe proxies, maybe the code has changed and it throws after publishing the message, maybe something else)
2 participants
Isolate deleteStudyIfNotCreationInProgress in its own try/catch in
consumeCaseImportFailed so that emitStudyCreationError is always called
even if the deletion fails due to a concurrent transaction that already
cleaned up the StudyCreationRequestEntity.
This was causing a flaky test (testCreateStudyWithErrorDuringCaseImport)
when running the full test suite because the WireMock stub stubImportNetworkWithServerError
simulates a situation that does not exist in production:
it returns a 5xx HTTP response AND sends a DLX message simultaneously.
In reality, a synchronous 5xx means the network conversion server
never processed the request and therefore never publishes on case.import.start,
so no DLX message would ever be generated.
This artificial combination caused the catch block in
createStudy and the DLX consumer to race against each other trying to
delete the same StudyCreationRequestEntity. Depending on thread
scheduling, the consumer's delete could throw an
ObjectOptimisticLockingFailureException, which was silently caught at the
outer level, preventing emitStudyCreationError from being called and
leaving the test waiting for a message that never arrived.
Solution was just to catch an exception but as sonar complains with nested try/catch and complexity the method has been cut in two