Draft
Conversation
Signed-off-by: benrejebmoh <mohamed.ben-rejeb@rte-france.com>
📝 WalkthroughWalkthroughThis pull request introduces a publisher-based pattern for step execution tracking in the monitor commons. Three new interfaces (ReportPublisher, StepStatusPublisher, and StepExecutionInterface) define contracts for publishing reports and step status updates, with default orchestration methods. The StepExecutionService is refactored to implement this interface and delegate to publishers instead of handling notifications directly. Changes
Sequence DiagramsequenceDiagram
participant Client
participant StepExecutionService
participant StepStatusPublisher
participant ReportPublisher
Client->>StepExecutionService: executeStep(context, step)
StepExecutionService->>StepStatusPublisher: updateStepStatus(executionId, RUNNING)
StepExecutionService->>Client: Execute stepExecution runnable
Client-->>StepExecutionService: Runnable completes
StepExecutionService->>ReportPublisher: sendReport(reportInfos)
StepExecutionService->>StepStatusPublisher: updateStepStatus(executionId, COMPLETED)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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
🧪 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
🧹 Nitpick comments (2)
monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/services/StepExecutionService.java (1)
68-69:context.getReportInfos()called twice — cache in a local variable.On Line 68,
Objects.requireNonNull(context.getReportInfos())validates and discards the reference; Line 69 calls it again. Cache the result so the null guard and the value use the same object reference.♻️ Proposed fix
public void executeStep(ProcessStepExecutionContext<C> context, ProcessStep<C> step) { setExecutionId(context.getProcessExecutionId()); + ReportInfos reportInfos = Objects.requireNonNull(context.getReportInfos()); executeStep( context.getStepExecutionId(), step.getType().getName(), context.getStepOrder(), context.getStartedAt(), - Objects.requireNonNull(context.getReportInfos()).reportUuid(), - context.getReportInfos(), + reportInfos.reportUuid(), + reportInfos, context.getResultInfos(), () -> step.execute(context) ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/services/StepExecutionService.java` around lines 68 - 69, The code calls context.getReportInfos() twice—once inside Objects.requireNonNull(...) and again on the next line—so cache the value in a local variable (e.g., ReportInfos reportInfos = Objects.requireNonNull(context.getReportInfos())) and then use reportInfos.reportUuid() and reportInfos for the subsequent arguments; update the block in StepExecutionService where Objects.requireNonNull(context.getReportInfos()) and context.getReportInfos() are used to reference the single cached variable.monitor-commons/src/main/java/org/gridsuite/monitor/commons/steps/StepExecutionInterface.java (1)
65-68:sendReportinside thetryblock couples report-delivery failure to step failure.If
stepExecution.run()succeeds butgetReportPublisher().sendReport(reportInfos)throws on Line 67, the catch on Line 69 fires and the step is markedFAILED— even though the computation completed successfully. Consider whether report-delivery failure should be treated as a step failure or handled separately (e.g., its own retry/dead-letter flow).♻️ Proposed separation of concerns
try { stepExecution.run(); - getReportPublisher().sendReport(reportInfos); updateStepStatus(stepExecutionId, stepTypeName, stepOrder, startedAt, reportUuid, resultInfos, StepStatus.COMPLETED); } catch (Exception e) { try { updateStepStatus(stepExecutionId, stepTypeName, stepOrder, startedAt, reportUuid, resultInfos, StepStatus.FAILED); } catch (Exception statusEx) { e.addSuppressed(statusEx); } throw e; } +// Report delivery is a separate concern; handle its failure independently. +getReportPublisher().sendReport(reportInfos);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monitor-commons/src/main/java/org/gridsuite/monitor/commons/steps/StepExecutionInterface.java` around lines 65 - 68, The call to getReportPublisher().sendReport(reportInfos) is inside the try that surrounds stepExecution.run(), so a report-delivery failure will incorrectly mark the step as FAILED; move or isolate sendReport so it cannot change step outcome: keep stepExecution.run() and updateStepStatus(..., StepStatus.COMPLETED) inside the primary try for the computation, then perform getReportPublisher().sendReport(reportInfos) in a separate try/catch (or a retry/dead-letter flow) that logs or records report-delivery errors without calling updateStepStatus with a failure status; reference stepExecution.run(), getReportPublisher().sendReport(reportInfos), updateStepStatus(...) and StepStatus.COMPLETED when making this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@monitor-commons/src/main/java/org/gridsuite/monitor/commons/steps/StepExecutionInterface.java`:
- Around line 65-73: When handling failures in the try/catch around
stepExecution.run(), ensure the original exception from stepExecution.run() is
not lost if updateStepStatus(...) throws: inside the catch(Exception e) block
catch and suppress any exception thrown by updateStepStatus(stepExecutionId,
stepTypeName, stepOrder, startedAt, reportUuid, resultInfos, StepStatus.FAILED)
(optionally logging it via the same logger/reporting mechanism) and then rethrow
the original exception `e`; i.e., wrap the call to updateStepStatus in its own
try/catch so updateStepStatus failures cannot overwrite the original exception
thrown by stepExecution.run().
In
`@monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/services/StepExecutionService.java`:
- Around line 27-31: StepExecutionService stores executionId as instance state
causing a race between setExecutionId(...) and the interface default methods
(skipStep/executeStep) that call getExecutionId(); fix by removing shared
mutable state: either (A) replace the executionId field with a ThreadLocal<UUID>
(e.g., executionIdHolder) and ensure you call executionIdHolder.remove() in a
finally block after each step, updating setExecutionId/getExecutionId to use the
ThreadLocal, or (B, preferred) change the StepExecutionInterface API to remove
getExecutionId()/setExecutionId() and add UUID executionId as the first
parameter to skipStep and executeStep and propagate that parameter through all
implementing classes (including StepExecutionService) so no singleton holds
per-thread state.
---
Nitpick comments:
In
`@monitor-commons/src/main/java/org/gridsuite/monitor/commons/steps/StepExecutionInterface.java`:
- Around line 65-68: The call to getReportPublisher().sendReport(reportInfos) is
inside the try that surrounds stepExecution.run(), so a report-delivery failure
will incorrectly mark the step as FAILED; move or isolate sendReport so it
cannot change step outcome: keep stepExecution.run() and updateStepStatus(...,
StepStatus.COMPLETED) inside the primary try for the computation, then perform
getReportPublisher().sendReport(reportInfos) in a separate try/catch (or a
retry/dead-letter flow) that logs or records report-delivery errors without
calling updateStepStatus with a failure status; reference stepExecution.run(),
getReportPublisher().sendReport(reportInfos), updateStepStatus(...) and
StepStatus.COMPLETED when making this change.
In
`@monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/services/StepExecutionService.java`:
- Around line 68-69: The code calls context.getReportInfos() twice—once inside
Objects.requireNonNull(...) and again on the next line—so cache the value in a
local variable (e.g., ReportInfos reportInfos =
Objects.requireNonNull(context.getReportInfos())) and then use
reportInfos.reportUuid() and reportInfos for the subsequent arguments; update
the block in StepExecutionService where
Objects.requireNonNull(context.getReportInfos()) and context.getReportInfos()
are used to reference the single cached variable.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
monitor-commons/src/main/java/org/gridsuite/monitor/commons/steps/ReportPublisher.javamonitor-commons/src/main/java/org/gridsuite/monitor/commons/steps/StepExecutionInterface.javamonitor-commons/src/main/java/org/gridsuite/monitor/commons/steps/StepStatusPublisher.javamonitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/services/StepExecutionService.java
Comment on lines
+65
to
+73
| try { | ||
| stepExecution.run(); | ||
| getReportPublisher().sendReport(reportInfos); | ||
| updateStepStatus(stepExecutionId, stepTypeName, stepOrder, startedAt, reportUuid, resultInfos, StepStatus.COMPLETED); | ||
| } catch (Exception e) { | ||
| updateStepStatus(stepExecutionId, stepTypeName, stepOrder, startedAt, reportUuid, resultInfos, StepStatus.FAILED); | ||
| throw e; | ||
| } | ||
| } |
There was a problem hiding this comment.
Original exception e can be silently lost if updateStepStatus in the catch block throws.
If stepExecution.run() fails and updateStepStatus(..., FAILED) on Line 70 also throws (e.g., the notification service is unreachable), the throw e on Line 71 is never reached. The caller sees only the secondary updateStepStatus exception, and the root cause is permanently lost.
🐛 Proposed fix — suppress secondary exception
} catch (Exception e) {
- updateStepStatus(stepExecutionId, stepTypeName, stepOrder, startedAt, reportUuid, resultInfos, StepStatus.FAILED);
- throw e;
+ try {
+ updateStepStatus(stepExecutionId, stepTypeName, stepOrder, startedAt, reportUuid, resultInfos, StepStatus.FAILED);
+ } catch (Exception statusEx) {
+ e.addSuppressed(statusEx);
+ }
+ throw e;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@monitor-commons/src/main/java/org/gridsuite/monitor/commons/steps/StepExecutionInterface.java`
around lines 65 - 73, When handling failures in the try/catch around
stepExecution.run(), ensure the original exception from stepExecution.run() is
not lost if updateStepStatus(...) throws: inside the catch(Exception e) block
catch and suppress any exception thrown by updateStepStatus(stepExecutionId,
stepTypeName, stepOrder, startedAt, reportUuid, resultInfos, StepStatus.FAILED)
(optionally logging it via the same logger/reporting mechanism) and then rethrow
the original exception `e`; i.e., wrap the call to updateStepStatus in its own
try/catch so updateStepStatus failures cannot overwrite the original exception
thrown by stepExecution.run().
Comment on lines
+27
to
+31
| public class StepExecutionService<C extends ProcessConfig> implements StepExecutionInterface<ReportInfos> { | ||
|
|
||
| private final NotificationService notificationService; | ||
| private final ReportService reportService; | ||
| @Getter | ||
| @Setter | ||
| private UUID executionId; |
There was a problem hiding this comment.
Race condition on executionId in a singleton bean.
StepExecutionService is a Spring @Service (singleton). The pattern in skipStep and executeStep is: setExecutionId(...) on Lines 52/62 → delegate to interface default method → default method calls getExecutionId(). Between the write and the read, a second concurrent request can overwrite executionId, so Thread A's default method retrieves Thread B's ID.
The simplest fix is a ThreadLocal; a cleaner fix removes executionId from instance state entirely and threads it through call parameters.
🔒 Option A — ThreadLocal (minimal change)
-@Getter
-@Setter
-private UUID executionId;
+private final ThreadLocal<UUID> executionIdHolder = new ThreadLocal<>();
+
+@Override
+public UUID getExecutionId() {
+ return executionIdHolder.get();
+}
+
+@Override
+public void setExecutionId(UUID executionId) {
+ executionIdHolder.set(executionId);
+}Make sure executionIdHolder.remove() is called after each step (e.g., in a finally block) to avoid stale values on pooled threads.
♻️ Option B — pass executionId explicitly through the interface (preferred, requires interface change)
Remove getExecutionId() / setExecutionId() from StepExecutionInterface and add executionId as the first parameter to skipStep and executeStep, eliminating the shared-state problem entirely.
📝 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.
Suggested change
| public class StepExecutionService<C extends ProcessConfig> implements StepExecutionInterface<ReportInfos> { | |
| private final NotificationService notificationService; | |
| private final ReportService reportService; | |
| @Getter | |
| @Setter | |
| private UUID executionId; | |
| public class StepExecutionService<C extends ProcessConfig> implements StepExecutionInterface<ReportInfos> { | |
| private final ThreadLocal<UUID> executionIdHolder = new ThreadLocal<>(); | |
| `@Override` | |
| public UUID getExecutionId() { | |
| return executionIdHolder.get(); | |
| } | |
| `@Override` | |
| public void setExecutionId(UUID executionId) { | |
| executionIdHolder.set(executionId); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/services/StepExecutionService.java`
around lines 27 - 31, StepExecutionService stores executionId as instance state
causing a race between setExecutionId(...) and the interface default methods
(skipStep/executeStep) that call getExecutionId(); fix by removing shared
mutable state: either (A) replace the executionId field with a ThreadLocal<UUID>
(e.g., executionIdHolder) and ensure you call executionIdHolder.remove() in a
finally block after each step, updating setExecutionId/getExecutionId to use the
ThreadLocal, or (B, preferred) change the StepExecutionInterface API to remove
getExecutionId()/setExecutionId() and add UUID executionId as the first
parameter to skipStep and executeStep and propagate that parameter through all
implementing classes (including StepExecutionService) so no singleton holds
per-thread state.
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
PR Summary
We need to use the step execution service logic in other services, so common behaviour is extracted into the lib monitor-commons