Add nodeActivity#1016
Open
ayolab wants to merge 2 commits into
Open
Conversation
Signed-off-by: Ayoub LABIDI <ayoub.labidi@protonmail.com>
📝 WalkthroughWalkthroughThis PR replaces boolean blocked-node tracking with persisted ChangesNode activity state and persistence
Guarded service workflows
Entry points and asynchronous cleanup
Suggested reviewers: 🚥 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. 🔧 ast-grep (0.44.1)src/main/java/org/gridsuite/study/server/controller/StudyController.javaast-grep timed out on this file src/main/java/org/gridsuite/study/server/networkmodificationtree/dto/NodeActivityCheckScope.javaast-grep retry budget exhausted before isolating this batch src/main/java/org/gridsuite/study/server/service/ConsumerService.javaast-grep retry budget exhausted before isolating this batch
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
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
Comment on lines
+40
to
+56
| public List<UUID> acquireActivity(UUID studyUuid, List<UUID> rootNetworkUuids, List<UUID> nodeUuids, NodeActivityCheckScope checkScope, NodeActivityStatus activity) { | ||
| if (nodeUuids.isEmpty()) { | ||
| return List.of(); | ||
| } | ||
| List<UUID> checkSetUuids = resolveCheckSet(nodeUuids, checkScope); | ||
| List<UUID> acquired = new ArrayList<>(); | ||
| try { | ||
| for (UUID rootNetworkUuid : rootNetworkUuids) { | ||
| rootNetworkNodeInfoService.setNodeActivity(studyUuid, rootNetworkUuid, nodeUuids, checkSetUuids, activity); | ||
| acquired.add(rootNetworkUuid); | ||
| } | ||
| return acquired; | ||
| } catch (StudyException e) { | ||
| acquired.forEach(rootNetworkUuid -> rootNetworkNodeInfoService.clearNodeActivity(studyUuid, rootNetworkUuid, nodeUuids)); | ||
| throw e; | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect setNodeActivity implementation to verify exception types.
# First map the file structure
ast-grep outline src/main/java/org/gridsuite/study/server/service/RootNetworkNodeInfoService.java --match 'setNodeActivity' --view expanded
# Then read the method body
rg -n -A 30 'void setNodeActivity' src/main/java/org/gridsuite/study/server/service/RootNetworkNodeInfoService.javaRepository: gridsuite/study-server
Length of output: 2261
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the service class around the activity methods and the repository method they call.
sed -n '1,260p' src/main/java/org/gridsuite/study/server/service/RootNetworkNodeInfoService.java | nl -ba | sed -n '470,560p'
echo
echo "---- repository ----"
rg -n -A 20 -B 10 'setNodeActivity|clearNodeActivity' src/main/java/org/gridsuite/study/server/repository/RootNetworkNodeInfoRepository.java
echo
echo "---- any exception handling around acquireActivity callers ----"
rg -n -A 8 -B 8 'acquireActivity\(' src/main/javaRepository: gridsuite/study-server
Length of output: 200
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== RootNetworkNodeInfoService activity methods =="
sed -n '495,520p' src/main/java/org/gridsuite/study/server/service/RootNetworkNodeInfoService.java | cat -n
echo
echo "== RootNetworkNodeInfoRepository methods =="
rg -n -A 20 -B 8 'setNodeActivity|clearNodeActivity' src/main/java/org/gridsuite/study/server/repository/RootNetworkNodeInfoRepository.java
echo
echo "== acquireActivity callers =="
rg -n -A 8 -B 8 'acquireActivity\(' src/main/javaRepository: gridsuite/study-server
Length of output: 2178
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate repository file =="
fd -a 'RootNetworkNodeInfoRepository.java' src/main/java
echo
echo "== locate notification service =="
fd -a 'NotificationService.java' src/main/java
echo
echo "== inspect repository methods =="
repo_file="$(fd -a 'RootNetworkNodeInfoRepository.java' src/main/java | head -n 1)"
if [ -n "$repo_file" ]; then
rg -n -A 20 -B 8 'setNodeActivity|clearNodeActivity' "$repo_file"
fi
echo
echo "== inspect notification emission method =="
notif_file="$(fd -a 'NotificationService.java' src/main/java | head -n 1)"
if [ -n "$notif_file" ]; then
rg -n -A 20 -B 8 'emitNodeActivityStatusUpdated' "$notif_file"
fiRepository: gridsuite/study-server
Length of output: 4941
Broaden the rollback to cover repository and notification failures
setNodeActivity(...) can fail with unchecked Spring/Data or notification exceptions, not just StudyException, so a partial acquire can skip cleanup and leave some root networks non-IDLE. Also, a clearNodeActivity(...) failure during rollback can mask the original error. Catch the broader failure set and suppress rollback errors.
🤖 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/NodeActivityGuardService.java`
around lines 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.
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.