Copilot commented on code in PR #582:
URL: https://github.com/apache/airavata/pull/582#discussion_r2572637710
##########
airavata-api/src/main/java/org/apache/airavata/helix/impl/task/completing/CompletingTask.java:
##########
@@ -39,6 +41,23 @@ public TaskResult onRun(TaskHelper helper, TaskContext
taskContext) {
logger.info("Process " + getProcessId() + " successfully completed");
saveAndPublishProcessStatus(ProcessState.COMPLETED);
cleanup();
+
+ try {
+ if (getExperimentModel().getCleanUpStrategy() ==
ExperimentCleanupStrategy.ALWAYS) {
+ AgentAdaptor adaptor = helper
+ .getAdaptorSupport()
+ .fetchAdaptor(
+ getTaskContext().getGatewayId(),
+ getTaskContext().getComputeResourceId(),
+ getTaskContext().getJobSubmissionProtocol(),
+
getTaskContext().getComputeResourceCredentialToken(),
+
getTaskContext().getComputeResourceLoginUserName());
+ logger.info("Cleaning up the working directory {}",
taskContext.getWorkingDir());
+ adaptor.deleteDirectory(getTaskContext().getWorkingDir());
+ }
+ } catch (Exception e) {
+ logger.error("Failed clean up experiment " + getExperimentId(), e);
Review Comment:
The error message states "Failed clean up experiment" but should say "Failed
to clean up experiment" for proper grammar.
```suggestion
logger.error("Failed to clean up experiment " +
getExperimentId(), e);
```
##########
airavata-api/src/main/java/org/apache/airavata/helix/agent/ssh/SshAgentAdaptor.java:
##########
@@ -182,6 +182,40 @@ public void createDirectory(String path, boolean
recursive) throws AgentExceptio
}
}
+ @Override
+ public void deleteDirectory(String path) throws AgentException {
+ String command = "rm -rf " + path;
+ ChannelExec channelExec = null;
+ try {
+ channelExec = (ChannelExec) session.openChannel("exec");
+ StandardOutReader stdOutReader = new StandardOutReader();
+
+ channelExec.setCommand(command);
+ InputStream out = channelExec.getInputStream();
+ InputStream err = channelExec.getErrStream();
+ channelExec.connect();
+
+ stdOutReader.readStdOutFromStream(out);
+ stdOutReader.readStdErrFromStream(err);
+
+ if (stdOutReader.getStdError() != null &&
stdOutReader.getStdError().contains("mkdir:")) {
Review Comment:
The error check is looking for "mkdir:" in the standard error output, but
this is a delete operation using `rm -rf`. This appears to be copy-pasted from
the `createDirectory` method above. The error check should either be removed or
updated to check for appropriate `rm` error patterns.
```suggestion
if (stdOutReader.getStdError() != null &&
stdOutReader.getStdError().contains("rm:")) {
```
##########
airavata-api/src/main/java/org/apache/airavata/agents/api/StorageResourceAdaptor.java:
##########
@@ -41,6 +41,8 @@ public void downloadFile(String remoteFile, OutputStream
localOutStream, FileMet
public void createDirectory(String path, boolean recursive) throws
AgentException;
+ public void deleteDirectory(String path) throws AgentException;
Review Comment:
This method overrides [AgentAdaptor.deleteDirectory](1); it is advisable to
add an Override annotation.
##########
airavata-api/src/main/java/org/apache/airavata/helix/adaptor/SSHJAgentAdaptor.java:
##########
@@ -277,6 +277,30 @@ public void createDirectory(String path, boolean
recursive) throws AgentExceptio
}
}
+ @Override
+ public void deleteDirectory(String path) throws AgentException {
+ SFTPClientWrapper sftpClient = null;
+ try {
+ sftpClient = sshjClient.newSFTPClientWrapper();
+ sftpClient.rmdir(path);
Review Comment:
The `SSHJAgentAdaptor.deleteDirectory` uses `sftpClient.rmdir(path)` which
only removes empty directories. This will fail if the working directory
contains files or subdirectories. The implementation should recursively delete
the directory contents, similar to how `SshAgentAdaptor` uses `rm -rf`.
Consider using a recursive deletion approach or executing a remote command like
`rm -rf`.
##########
airavata-api/src/main/java/org/apache/airavata/helix/agent/ssh/SshAgentAdaptor.java:
##########
@@ -182,6 +182,40 @@ public void createDirectory(String path, boolean
recursive) throws AgentExceptio
}
}
+ @Override
+ public void deleteDirectory(String path) throws AgentException {
+ String command = "rm -rf " + path;
Review Comment:
Missing space between "rm -rf" and the path parameter. While the shell will
execute this correctly, it's a style inconsistency. Should be `"rm -rf " +
path` for proper spacing.
```suggestion
String command = "rm -rf " + path;
```
##########
.devcontainer/database_scripts/init/07-cleanup-strategy-migration.sql:
##########
@@ -0,0 +1,4 @@
+USE experiment_catalog;
+
+-- Add cleanAfterStaged flag to APPLICATION_INTERFACE
Review Comment:
The comment describes adding a `cleanAfterStaged` flag, but the actual
column being added is `CLEANUP_STRATEGY`. The comment should be updated to
accurately reflect what's being added.
```suggestion
-- Add CLEANUP_STRATEGY column to EXPERIMENT table
```
##########
airavata-api/src/main/java/org/apache/airavata/helix/impl/task/TaskContext.java:
##########
@@ -176,6 +178,10 @@ public void setProcessModel(ProcessModel processModel) {
this.processModel = processModel;
}
+ public void setExperimentModel(ExperimentModel experimentModel) {
+ this.experimentModel = experimentModel;
+ }
+
Review Comment:
The `TaskContext` class has a setter for `experimentModel` but no
corresponding getter method. This prevents other classes from accessing the
experiment model through the task context. A `getExperimentModel()` method
should be added to complete the API.
```suggestion
public ExperimentModel getExperimentModel() {
return experimentModel;
}
```
##########
airavata-api/src/main/java/org/apache/airavata/helix/agent/ssh/SshAgentAdaptor.java:
##########
@@ -182,6 +182,40 @@ public void createDirectory(String path, boolean
recursive) throws AgentExceptio
}
}
+ @Override
+ public void deleteDirectory(String path) throws AgentException {
+ String command = "rm -rf " + path;
Review Comment:
The `rm -rf` command is executed without proper path sanitization or
validation. If the path contains special characters or is maliciously crafted,
it could lead to unintended deletions. Consider validating the path or using
proper shell escaping before executing the command.
##########
airavata-api/src/main/java/org/apache/airavata/helix/impl/task/completing/CompletingTask.java:
##########
@@ -39,6 +41,23 @@ public TaskResult onRun(TaskHelper helper, TaskContext
taskContext) {
logger.info("Process " + getProcessId() + " successfully completed");
saveAndPublishProcessStatus(ProcessState.COMPLETED);
cleanup();
+
+ try {
+ if (getExperimentModel().getCleanUpStrategy() ==
ExperimentCleanupStrategy.ALWAYS) {
+ AgentAdaptor adaptor = helper
+ .getAdaptorSupport()
+ .fetchAdaptor(
+ getTaskContext().getGatewayId(),
+ getTaskContext().getComputeResourceId(),
+ getTaskContext().getJobSubmissionProtocol(),
+
getTaskContext().getComputeResourceCredentialToken(),
+
getTaskContext().getComputeResourceLoginUserName());
+ logger.info("Cleaning up the working directory {}",
taskContext.getWorkingDir());
+ adaptor.deleteDirectory(getTaskContext().getWorkingDir());
+ }
Review Comment:
The enum defines `ONLY_COMPLETED` and `ONLY_FAILED` strategies, but only
`ALWAYS` is implemented in the `CompletingTask`. The `ONLY_COMPLETED` strategy
is not checked/handled here (though it would apply to this task), and there's
no implementation for `ONLY_FAILED` which should likely be added to the failure
handling path (e.g., in `AiravataTask.onFail()`). This incomplete
implementation means the feature won't work as documented.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
