imbajin commented on code in PR #2937:
URL: https://github.com/apache/hugegraph/pull/2937#discussion_r3432918294
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/task/DistributedTaskScheduler.java:
##########
@@ -316,14 +355,18 @@ protected <V> HugeTask<V> deleteFromDB(Id id) {
@Override
public <V> HugeTask<V> delete(Id id, boolean force) {
- if (!force) {
- // Change status to DELETING, perform the deletion operation
through automatic
- // scheduling.
+ HugeTask<?> task = this.taskWithoutResult(id);
+
+ if (!force && !task.completed()) {
+ // Check task status: can't delete running tasks without force
this.updateStatus(id, null, TaskStatus.DELETING);
Review Comment:
I think this is still the key correctness issue in the current HStore
distributed path.
The impact is not only that `delete(id, false)` races with cancellation, but
that the user-visible delete invariant can be broken:
1. `delete(false)` marks the task as `DELETING`.
2. The cron path sees `DELETING`, calls `cancel(true)`, then immediately
removes the task vertex from DB.
3. `HugeTask.cancel()` triggers `TaskCallable.cancelled()`.
4. `UserJob.cancelled()` / `SysJob.cancelled()` call `save()`.
5. `done()` may also persist the final task state.
So a task can be physically deleted and then written back as `CANCELLED`
during cancellation cleanup. From the API user perspective, the task was
deleted, but it may reappear later.
A minimal reliable fix would be to make delete a two-phase operation for
running tasks:
- `delete(false)` only records the delete intent (`DELETING`) and returns
the task.
- The runner that actually owns the task cancels it.
- Physical `deleteFromDB()` happens only after the runner has fully exited
and no further task callback can save the task again.
One compact way to implement this is to keep an in-memory `deletingTasks`
set in `DistributedTaskScheduler`:
```java
private final Set<Id> deletingTasks = ConcurrentHashMap.newKeySet();
```
Then:
- add the id to `deletingTasks` when `delete(false)` transitions the task to
`DELETING`;
- in the `DELETING` cron branch, cancel the local running task but do not
delete immediately;
- in `TaskRunner.finally`, after `task.run()` has returned and
`runningTasks.remove()` is done, delete the task if its id is in
`deletingTasks`.
That keeps the fix local to the distributed scheduler and avoids
reintroducing master-worker/server-info logic.
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/task/DistributedTaskScheduler.java:
##########
@@ -316,14 +355,18 @@ protected <V> HugeTask<V> deleteFromDB(Id id) {
@Override
public <V> HugeTask<V> delete(Id id, boolean force) {
- if (!force) {
- // Change status to DELETING, perform the deletion operation
through automatic
- // scheduling.
+ HugeTask<?> task = this.taskWithoutResult(id);
+
+ if (!force && !task.completed()) {
Review Comment:
This is still visible on the current head, and the API impact is clearer now:
`TaskAPI.delete()` treats a `null` return value as "There is no task with id
...", but in this branch `DistributedTaskScheduler.delete(id, false)` returns
`null` after successfully marking an existing incomplete task as `DELETING`.
So the request can both:
- mutate the existing task to `DELETING`, and
- report to the caller that the task does not exist.
That makes retry/automation behavior confusing because clients cannot tell
whether the delete request was accepted or whether the id was invalid.
The smallest fix is to return the task when the `DELETING` transition
succeeds. A slightly safer version should also avoid unconditional
`prestatus=null`, so we do not overwrite a concurrently completed task.
```suggestion
HugeTask<V> task = this.taskWithoutResult(id);
if (!force && !task.completed()) {
TaskStatus currentStatus = task.status();
if (this.updateStatus(id, currentStatus, TaskStatus.DELETING)) {
task.overwriteStatus(TaskStatus.DELETING);
} else {
task = this.taskWithoutResult(id);
}
return task;
}
// Delete from DB directly for completed/DELETING tasks or force=true
return this.deleteFromDB(id);
```
This keeps the existing async-delete model, but makes the API contract
honest: an existing task marked for deletion is returned as existing, not as
missing.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
