Add nodeActivity - #1016
Open
ayolab wants to merge 4 commits into
Open
Conversation
Signed-off-by: Ayoub LABIDI <ayoub.labidi@protonmail.com>
📝 WalkthroughWalkthroughThe PR replaces blocked-node state with local and shared activity statuses persisted in the database. Activity guards now coordinate tree writes, builds, unbuilds, computations, notifications, REST endpoints, rebuild workflows, and asynchronous cleanup. ChangesNode activity contracts and persistence
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant StudyController
participant NodeActivityGuardService
participant StudyService
participant ConsumerService
StudyController->>NodeActivityGuardService: acquire activity for write or computation
NodeActivityGuardService->>StudyService: execute guarded operation
StudyService-->>ConsumerService: computation completion or failure
ConsumerService->>NodeActivityGuardService: release local activity
NodeActivityGuardService-->>StudyController: return operation result
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches⚔️ Resolve merge conflicts
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Checkov (3.3.8)src/main/resources/db/changelog/db.changelog-master.yamlTraceback (most recent call last): 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: 1
🧹 Nitpick comments (2)
src/main/java/org/gridsuite/study/server/service/StudyService.java (1)
1770-1782: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
getBuildInfos/setModificationReportsrun afterBUILDINGis set but outside thetry.If
getBuildInfosorsetModificationReportsthrows,clearNodeActivityis not invoked. DB state reverts via the caller's transaction, but theBUILDINGactivity-status notification emitted bysetNodeActivityis not rolled back, so clients can be left seeing a stuckBUILDINGstate. Consider extending thetryto cover these steps.♻️ Suggested scope widening
setNodeActivity(studyUuid, rootNetworkUuid, nodeUuid, NodeActivityStatus.BUILDING); - BuildInfos buildInfos = networkModificationTreeService.getBuildInfos(nodeUuid, rootNetworkUuid); - - // Store all reports (inherited + new) for this node - networkModificationTreeService.setModificationReports(nodeUuid, rootNetworkUuid, buildInfos.getAllReportsAsMap()); try { + BuildInfos buildInfos = networkModificationTreeService.getBuildInfos(nodeUuid, rootNetworkUuid); + // Store all reports (inherited + new) for this node + networkModificationTreeService.setModificationReports(nodeUuid, rootNetworkUuid, buildInfos.getAllReportsAsMap()); networkModificationService.buildNode(nodeUuid, rootNetworkUuid, buildInfos, workflowInfos); } catch (Exception e) { clearNodeActivity(studyUuid, rootNetworkUuid, nodeUuid); throw 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/StudyService.java` around lines 1770 - 1782, The node activity can get stuck in BUILDING because getBuildInfos and setModificationReports run after setNodeActivity but outside the guarded failure path. Move the build-info retrieval and modification-report update into the same try/catch used around networkModificationService.buildNode in StudyService so any exception triggers clearNodeActivity. Keep the existing cleanup logic in the catch and make sure the BUILDING status is only emitted once the whole build preparation and execution flow is safely covered.src/main/java/org/gridsuite/study/server/dto/InvalidateNodeTreeParameters.java (1)
21-25: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDeclare these shared presets as
final.These are public shared constant instances but are only
static, so they can be accidentally reassigned by any caller, silently corrupting invalidation behavior everywhere. Mark themfinal.♻️ Proposed change
- public static InvalidateNodeTreeParameters ALL = new InvalidateNodeTreeParameters(InvalidationMode.ALL, ComputationsInvalidationMode.ALL); + public static final InvalidateNodeTreeParameters ALL = new InvalidateNodeTreeParameters(InvalidationMode.ALL, ComputationsInvalidationMode.ALL); `@SuppressWarnings`("checkstyle:AbbreviationAsWordInName") - public static InvalidateNodeTreeParameters ONLY_CHILDREN = new InvalidateNodeTreeParameters(InvalidationMode.ONLY_CHILDREN, ComputationsInvalidationMode.ALL); + public static final InvalidateNodeTreeParameters ONLY_CHILDREN = new InvalidateNodeTreeParameters(InvalidationMode.ONLY_CHILDREN, ComputationsInvalidationMode.ALL); `@SuppressWarnings`("checkstyle:AbbreviationAsWordInName") - public static InvalidateNodeTreeParameters ONLY_CHILDREN_BUILD_STATUS = new InvalidateNodeTreeParameters(InvalidationMode.ONLY_CHILDREN_BUILD_STATUS, ComputationsInvalidationMode.ALL); + public static final InvalidateNodeTreeParameters ONLY_CHILDREN_BUILD_STATUS = new InvalidateNodeTreeParameters(InvalidationMode.ONLY_CHILDREN_BUILD_STATUS, ComputationsInvalidationMode.ALL);🤖 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/dto/InvalidateNodeTreeParameters.java` around lines 21 - 25, The shared preset instances in InvalidateNodeTreeParameters are mutable static fields, so they can be reassigned by callers; make ALL, ONLY_CHILDREN, and ONLY_CHILDREN_BUILD_STATUS final to treat them as true constants. Update the field declarations in InvalidateNodeTreeParameters so these public presets cannot be reassigned, keeping the existing initialization with InvalidationMode and ComputationsInvalidationMode unchanged.
🤖 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 `@src/main/java/org/gridsuite/study/server/service/StudyService.java`:
- Around line 3244-3246: The modification notification flow in StudyService is
unbalanced because setNodeActivity(...) can throw after
emitStartModificationEquipmentNotification() but before the try/finally block
starts, leaving the UI stuck in MODIFICATIONS_UPDATING_IN_PROGRESS. Reorder the
logic in the same method so the UPDATING state is set before emitting the start
notification, then keep the existing try/finally cleanup with
emitEndModificationEquipmentNotification() and clearNodeActivity() to guarantee
the start/end pair always matches.
---
Nitpick comments:
In
`@src/main/java/org/gridsuite/study/server/dto/InvalidateNodeTreeParameters.java`:
- Around line 21-25: The shared preset instances in InvalidateNodeTreeParameters
are mutable static fields, so they can be reassigned by callers; make ALL,
ONLY_CHILDREN, and ONLY_CHILDREN_BUILD_STATUS final to treat them as true
constants. Update the field declarations in InvalidateNodeTreeParameters so
these public presets cannot be reassigned, keeping the existing initialization
with InvalidationMode and ComputationsInvalidationMode unchanged.
In `@src/main/java/org/gridsuite/study/server/service/StudyService.java`:
- Around line 1770-1782: The node activity can get stuck in BUILDING because
getBuildInfos and setModificationReports run after setNodeActivity but outside
the guarded failure path. Move the build-info retrieval and modification-report
update into the same try/catch used around networkModificationService.buildNode
in StudyService so any exception triggers clearNodeActivity. Keep the existing
cleanup logic in the catch and make sure the BUILDING status is only emitted
once the whole build preparation and execution flow is safely covered.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e65c8fcc-dcc3-45b9-b0e4-0dbf2f83a195
📒 Files selected for processing (18)
src/main/java/org/gridsuite/study/server/controller/StudyController.javasrc/main/java/org/gridsuite/study/server/dto/InvalidateNodeTreeParameters.javasrc/main/java/org/gridsuite/study/server/dto/RootNetworkNodeInfo.javasrc/main/java/org/gridsuite/study/server/networkmodificationtree/dto/BuildStatus.javasrc/main/java/org/gridsuite/study/server/networkmodificationtree/dto/NetworkModificationNode.javasrc/main/java/org/gridsuite/study/server/networkmodificationtree/dto/NodeActivityStatus.javasrc/main/java/org/gridsuite/study/server/networkmodificationtree/dto/NodeBuildStatus.javasrc/main/java/org/gridsuite/study/server/networkmodificationtree/entities/RootNetworkNodeInfoEntity.javasrc/main/java/org/gridsuite/study/server/notification/NotificationService.javasrc/main/java/org/gridsuite/study/server/repository/rootnetwork/RootNetworkNodeInfoRepository.javasrc/main/java/org/gridsuite/study/server/service/ConsumerService.javasrc/main/java/org/gridsuite/study/server/service/NetworkModificationTreeService.javasrc/main/java/org/gridsuite/study/server/service/RebuildNodeService.javasrc/main/java/org/gridsuite/study/server/service/RootNetworkNodeInfoService.javasrc/main/java/org/gridsuite/study/server/service/StudyService.javasrc/main/java/org/gridsuite/study/server/service/SupervisionService.javasrc/main/resources/db/changelog/changesets/changelog_20260703T120000Z.xmlsrc/main/resources/db/changelog/db.changelog-master.yaml
💤 Files with no reviewable changes (2)
- src/main/java/org/gridsuite/study/server/networkmodificationtree/dto/NodeBuildStatus.java
- src/main/java/org/gridsuite/study/server/networkmodificationtree/dto/BuildStatus.java
Signed-off-by: Ayoub LABIDI <ayoub.labidi@protonmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/java/org/gridsuite/study/server/controller/StudyController.java (1)
1706-1715: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winTOCTOU between the build-status check and the guarded unbuild.
getNodeBuildStatus(...).isBuilt()is read outside the activity guard, and the guard is only acquired if that read wastrue. A concurrent build/unbuild between the check and the guard acquisition can make this decision stale — either skipping an unbuild that should now happen, or acquiring the guard for a node that's already been unbuilt.🐛 Proposed fix: move the check inside the guarded action
- if (networkModificationTreeService.getNodeBuildStatus(nodeUuid, rootNetworkUuid).isBuilt()) { - nodeActivityGuardService.runGuarded(studyUuid, List.of(rootNetworkUuid), List.of(nodeUuid), NodeActivityCheckScope.SELF, NodeActivityStatus.UPDATING, - () -> studyService.unbuildStudyNode(studyUuid, nodeUuid, rootNetworkUuid, userId)); - } + nodeActivityGuardService.runGuarded(studyUuid, List.of(rootNetworkUuid), List.of(nodeUuid), NodeActivityCheckScope.SELF, NodeActivityStatus.UPDATING, + () -> { + if (networkModificationTreeService.getNodeBuildStatus(nodeUuid, rootNetworkUuid).isBuilt()) { + studyService.unbuildStudyNode(studyUuid, nodeUuid, rootNetworkUuid, userId); + } + return null; + });🤖 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/controller/StudyController.java` around lines 1706 - 1715, Move the getNodeBuildStatus(nodeUuid, rootNetworkUuid).isBuilt() check into the action passed to nodeActivityGuardService.runGuarded in unbuildNode, and invoke unbuildStudyNode only when the status is built. Always acquire the guard before evaluating the status, preserving the existing response behavior.src/main/java/org/gridsuite/study/server/service/RebuildNodeService.java (1)
186-203: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winKeep
UPDATINGheld through the rebuild trigger TherunGuardedscope ends before thebuildNodeloop starts, so another request can slip in afterUPDATINGis cleared but before the rebuild call acquires its ownBUILDINGactivity. Move the loop inside the guarded action or wrap it in a second guard.🤖 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/RebuildNodeService.java` around lines 186 - 203, Keep the node rebuild trigger within the activity guard in the method containing runGuarded: move the rootNetworkUuidsByNodeBuilt buildNode loop into the action executed by nodeActivityGuardService.runGuarded, or apply a second guard covering that loop. Ensure NodeActivityStatus.UPDATING remains held until all studyService.buildNode calls have been initiated.
🤖 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
`@src/main/java/org/gridsuite/study/server/service/NodeActivityGuardService.java`:
- Around line 40-56: Update acquireActivity to catch the broader unchecked
failures that can arise from setNodeActivity, while preserving the original
exception for propagation. During rollback over acquired root networks, handle
clearNodeActivity failures without replacing the original error, such as by
suppressing them on the caught exception.
---
Outside diff comments:
In `@src/main/java/org/gridsuite/study/server/controller/StudyController.java`:
- Around line 1706-1715: Move the getNodeBuildStatus(nodeUuid,
rootNetworkUuid).isBuilt() check into the action passed to
nodeActivityGuardService.runGuarded in unbuildNode, and invoke unbuildStudyNode
only when the status is built. Always acquire the guard before evaluating the
status, preserving the existing response behavior.
In `@src/main/java/org/gridsuite/study/server/service/RebuildNodeService.java`:
- Around line 186-203: Keep the node rebuild trigger within the activity guard
in the method containing runGuarded: move the rootNetworkUuidsByNodeBuilt
buildNode loop into the action executed by nodeActivityGuardService.runGuarded,
or apply a second guard covering that loop. Ensure NodeActivityStatus.UPDATING
remains held until all studyService.buildNode calls have been initiated.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 14f15bed-eda9-4c90-b28b-5a30a79693a4
📒 Files selected for processing (7)
src/main/java/org/gridsuite/study/server/controller/StudyController.javasrc/main/java/org/gridsuite/study/server/networkmodificationtree/dto/NodeActivityCheckScope.javasrc/main/java/org/gridsuite/study/server/service/ConsumerService.javasrc/main/java/org/gridsuite/study/server/service/NetworkModificationTreeService.javasrc/main/java/org/gridsuite/study/server/service/NodeActivityGuardService.javasrc/main/java/org/gridsuite/study/server/service/RebuildNodeService.javasrc/main/java/org/gridsuite/study/server/service/StudyService.java
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/java/org/gridsuite/study/server/service/NetworkModificationTreeService.java
- src/main/java/org/gridsuite/study/server/service/StudyService.java
Signed-off-by: Ayoub LABIDI <ayoub.labidi@protonmail.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/org/gridsuite/study/server/service/ConsumerService.java (1)
495-506: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winClear calculation activity in a
finallyblock.
handleClearNodeActivity(...)runs only after the result UUID update succeeds. If that database update or earlier result processing throws, the local activity remains set and can permanently block subsequent operations. Wrap the result handling intry/finallyand release the activity for the parsed receiver.🤖 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 495 - 506, Update the result-processing flow around the computation/result UUID updates so handleClearNodeActivity is invoked from a finally block, ensuring cleanup even when result processing or database updates throw. Use the parsed receiver object and resolve the study UUID needed for cleanup within the protected flow, while preserving the existing LOAD_FLOW and other computation update behavior.
🤖 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.
Outside diff comments:
In `@src/main/java/org/gridsuite/study/server/service/ConsumerService.java`:
- Around line 495-506: Update the result-processing flow around the
computation/result UUID updates so handleClearNodeActivity is invoked from a
finally block, ensuring cleanup even when result processing or database updates
throw. Use the parsed receiver object and resolve the study UUID needed for
cleanup within the protected flow, while preserving the existing LOAD_FLOW and
other computation update behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 22e65c59-8540-478a-a614-1f16f6921361
📒 Files selected for processing (22)
src/main/java/org/gridsuite/study/server/controller/StudyController.javasrc/main/java/org/gridsuite/study/server/dto/RootNetworkNodeInfo.javasrc/main/java/org/gridsuite/study/server/error/StudyBusinessErrorCode.javasrc/main/java/org/gridsuite/study/server/error/StudyExceptionHandler.javasrc/main/java/org/gridsuite/study/server/networkmodificationtree/dto/LocalActivityStatus.javasrc/main/java/org/gridsuite/study/server/networkmodificationtree/dto/NetworkModificationNode.javasrc/main/java/org/gridsuite/study/server/networkmodificationtree/dto/NodeCheckScope.javasrc/main/java/org/gridsuite/study/server/networkmodificationtree/dto/RootNetworkActivity.javasrc/main/java/org/gridsuite/study/server/networkmodificationtree/dto/SharedActivityStatus.javasrc/main/java/org/gridsuite/study/server/networkmodificationtree/entities/NetworkModificationNodeInfoEntity.javasrc/main/java/org/gridsuite/study/server/networkmodificationtree/entities/RootNetworkNodeInfoEntity.javasrc/main/java/org/gridsuite/study/server/notification/NotificationService.javasrc/main/java/org/gridsuite/study/server/repository/networkmodificationtree/NetworkModificationNodeInfoRepository.javasrc/main/java/org/gridsuite/study/server/repository/rootnetwork/RootNetworkNodeInfoRepository.javasrc/main/java/org/gridsuite/study/server/service/ConsumerService.javasrc/main/java/org/gridsuite/study/server/service/NetworkModificationTreeService.javasrc/main/java/org/gridsuite/study/server/service/NodeActivityGuardService.javasrc/main/java/org/gridsuite/study/server/service/RebuildNodeService.javasrc/main/java/org/gridsuite/study/server/service/RootNetworkNodeInfoService.javasrc/main/java/org/gridsuite/study/server/service/StudyService.javasrc/main/resources/db/changelog/changesets/changelog_20260703T125842Z.xmlsrc/main/resources/db/changelog/db.changelog-master.yaml
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.
No description provided.