[imbajin]

Purpose of the PR

Hubble could leave a Loader open after task exit or failed preparation/dispatch. Closing it also exposed an unsafe checkpoint: parsed rows could be skipped by Resume even when they had never been inserted.

Main Changes

Close each Loader before publishing task completion, including unused Loaders after save/update, dispatch and rollback failures. Preserve the original load failure and requested PAUSED/STOPPED state when shutdown only rethrows that failure; independent cleanup failures remain visible with their recovery responsibility.

Hubble disables progress-file writes for its initial and reconnected Loaders. Pause/Resume and FAILED-task Resume therefore retain the previous replay behavior instead of treating parsed offsets as inserted rows. Existing checkpoints are preserved; use Stop followed by Retry when an older checkpoint may be unsafe. Existing standalone Loader constructors keep their checkpoint behavior.

Task/job/source deletion waits for native cleanup and the exact final callback before opening its short metadata transaction. Reservations prevent retry/resume from replacing that task during deletion. An internal caller that already owns a transaction must finish it and retry if callbacks remain pending; completed tasks can still be deleted in that transaction. Upload/source reservations also protect concurrent creation and cleanup.

request stop → native cleanup → final callback commits → short metadata deletion transaction

Database faults use operator recovery in this release; there is no automatic per-owner polling repair. While this Hubble process retains unresolved callback/cleanup responsibility, unsafe replacement/deletion is refused. After an abnormal process exit with no retained runtime, deleting leftover RUNNING metadata is allowed. This removes Hubble metadata only: Server graph mode or a leftover loading owner must be inspected and recovered separately before new imports. Existing Pause/Resume or Stop/Retry actions are documented for keeping and rerunning the task after recovery.

Concurrent imports require the complete matching Loader owner integration and Server ownership API. Current paired real-Server/UI acceptance and the dependent managed-executor shutdown packaged shutdown check remain open.

Verifying these changes

• Native CSV/parser/semaphore and HTTP insertion regressions reproduce missing rows after pause or single-insert failure on the predecessor, then verify every row after Resume with this change. These use a controlled HTTP endpoint, not a real-Server acceptance claim.
• Real Spring/H2/MyBatis callbacks cover deletion, rollback, same-ID restart, pending transactions, rejected dispatch and retained cleanup failures. Waiting for callbacks leaves metadata connections available.
• The standalone PR passes Java 17 clean reactor compilation, complete Client/Loader/Hubble unit suites, formatting and strict checks. Its class-origin check confirms that it does not inherit the dependent executor configuration.
• Three independent reviewers passed the final source and recovery-contract clarification.

Does this PR potentially affect the following parts?

• Dependencies
• Modify configurations
• The public API
• Other affects: import resource cleanup and deletion lifecycle
• Nope

Documentation Status

• Doc - TODO
• Doc - Done: Hubble and Loader READMEs.
• Doc - No Need

Paired PR: hugegraph#66

Conceptual overview

Conceptual diagram; not runtime test evidence. 任务退出先完成 Loader 原生清理与最终回调，再用短事务删除元数据；Hubble 不新增解析偏移检查点，遗留元数据删除不等于 Server 恢复。 配对 Server / UI 验收及打包停机检查仍待完成。

[imbajin]

Same-source cross-review: 7.5/10 at fc0c3fc. Commit and tree match the reviewed org PR. Two open points remain: dispatch rejection followed by failure-state transaction rollback can discard the recovery owner (hugegraph#66 (comment)); normal same-job multi-file completion can restore NONE while another importer remains active (hugegraph#66 (comment)). The first is a baseline-existing recovery gap; the second is the existing shared GraphMode ownership family newly exposed by completion cleanup. Server #3331 alone does not integrate this client. No new runtime reproduction or ASF CI pass is claimed.

[codecov]

Codecov Report

❌ Patch coverage is 71.93585% with 245 lines in your changes missing coverage. Please review.
✅ Project coverage is 47.18%. Comparing base (bcd5ce2) to head (ebe5bd5).
⚠️ Report is 4 commits behind head on master.

Files with missing lines	Patch %	Lines
...apache/hugegraph/service/load/LoadTaskService.java	69.20%	114 Missing and 40 partials ⚠️
...ugegraph/controller/load/FileUploadController.java	73.07%	21 Missing and 21 partials ⚠️
...ache/hugegraph/service/load/JobManagerService.java	72.09%	14 Missing and 10 partials ⚠️
...che/hugegraph/service/load/FileMappingService.java	56.00%	6 Missing and 5 partials ⚠️
...ava/org/apache/hugegraph/entity/load/LoadTask.java	86.79%	1 Missing and 6 partials ⚠️
...a/org/apache/hugegraph/loader/HugeGraphLoader.java	82.05%	4 Missing and 3 partials ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master     #814      +/-   ##
============================================
+ Coverage     47.06%   47.18%   +0.11%     
+ Complexity     4518     3591     -927     
============================================
  Files           602      377     -225     
  Lines         29452    23137    -6315     
  Branches       3456     3159     -297     
============================================
- Hits          13862    10917    -2945     
+ Misses        14308    11059    -3249     
+ Partials       1282     1161     -121     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[bitflicker64]

Blocking: yes. Summary: Closing the Loader on task exit, on a failed initial save or dispatch and on rollback works as described, and the hook removal and checkpoint handling in the Loader are sound, but the new shutdown resets the graph mode to NONE when the first task of a multi-file job finishes while the others are still importing. Two smaller notes cover the resume and retry paths and the status after a failed load. Evidence: full diff 8e6ed84..8757dc8 read with LoadTask, LoadTaskService, LoadTaskController, LoadTaskExecutor, AsyncConfig, HugeGraphLoader, LoadContext and TaskManager at this head, and GraphTransaction.prepareAdditions in apache/hugegraph; no tests were run for this review. candidate-image (loader) failed on this head in its Install locked same-source SDK step and the Hubble image, CodeQL and module validation jobs were still queued at review time, so CI did not yet confirm the new tests.

[bitflicker64]

Blocking: yes. Summary: The earlier Important comment still applies at 2b2d66c: merged on its own, this PR resets the graph mode to NONE when the first of several concurrent tasks on a graph exits, because the protection for a shared loading mode is in #815 and apache/hugegraph#3331, which are both still open. The two earlier Minor notes are settled (resume and retry now close their Loader on a failed update or a rejected dispatch, and a shutdown that only rethrows the load failure keeps PAUSED or STOPPED), and one new non-blocking Important note covers a mode reset failure that is lost when the load has also failed. Evidence: full diff 8e6ed84..2b2d66c (9 files, +624/-26) read with LoadTask, LoadTaskService, LoadTaskController, JobManagerService, LoadTaskExecutor, HugeGraphLoader, LoadContext and TaskManager at this head. On JDK 17, mvn -pl hugegraph-loader,hugegraph-hubble/hubble-be -am test limited to LoadContextTest, LoadTaskServiceTest and IngestTransactionIntegrationTest passes (28, 20 and 12 tests, Checkstyle and RAT skipped), and two throwaway probe tests against an HTTP stub in the same run reproduce the new note. CI: the loader-ci and hubble-ci jobs were still pending on this head at review time, and no completed check had failed.

[imbajin]

Integration follow-up — the current standalone merge assessment is limited by the paired ownership lifecycle, not the now-fixed reset-error masking.

Reviewed #814 commit: 5e0389b.

The new exception-preservation tests and implementation address the earlier masking finding. The remaining integration dependency runs in both directions: this PR closes each Loader, but its LoadContext.unsetLoadingMode() still uses the shared enum reset, so concurrent imports require #815 and Server apache/hugegraph#3331. Conversely, #815 at 73ad996 still needs this PR's Hubble task-exit cleanup to release owner handles, as noted in #815 (comment).

Please validate the combined changes while preserving both this PR's reset/close exception precedence and #815's owner release. A useful paired regression starts two Hubble imports, keeps LOADING after the first task exits, and verifies NONE plus closed Loader resources after the final task exits, including failure/pause paths. Independent green checks on the two separate branches do not establish that combined lifecycle.

This consolidates existing integration requirements and does not reopen the fixed masking finding. Evidence is current-head source and existing review discussion; no new full Hubble/PD/HStore runtime acceptance is claimed.

[bitflicker64]

Blocking: no. Score: 9/10. Deductions: shared-mode reset reporting NONE early (Minor, corrected below) and the combined-lifecycle validation from @imbajin review 5513838881 (Minor): 10 - 0.5 - 0.5 = 9.

Correction to my comment 4261511700: the sibling row rejection described there does not happen. In apache/hugegraph master, the LOADING-specific Server effects are skipping checkNonnullProperty for vertices (GraphTransaction.java:340) and, on the Raft backend, skipping follower cache invalidation (RaftContext.java:294). The Loader already rejects a vertex missing a non-null key before sending it (ElementBuilder.checkNonNullableKeys, same rule, on every property-bearing build path), and an early NONE only re-enables cache invalidation. So merged alone, the first task of a multi-file job sets the graph to NONE early without changing any row outcome. Correct mode ownership still needs #815 and apache/hugegraph#3331, and #815 in turn needs this PR's task-exit cleanup.

Summary: at 5e0389b, task exit, unscheduled failures, resume/retry and rollback close each Loader, and the precedence between mode reset, recorded load failure and close failures matches the PR body.

Evidence: LoadContextTest 34/34, LoadTaskServiceTest 20/20 and IngestTransactionIntegrationTest 12/12 pass offline on JDK 17. A VertexBuilder check with the non-null column absent fails with "must be set, but missed keys [city]". CI checks all pass or skip. Not run here: PD/HStore and the combined build at this head.

[bitflicker64]

Blocking: yes. Summary: The cleanup itself works: task exit, unscheduled save/dispatch failures, resume/retry and rollback all close their Loader, and the hook removal and exception precedence match the PR body. But closing the Loader on task exit now also writes a progress checkpoint, which Hubble never did before, and that checkpoint counts lines as done once they are submitted, not once they are inserted. So resume after a pause or a failure can skip rows that never reached the Server. The earlier note on the shared graph mode (thread on LoadTask.java line 208) still applies and is not repeated here. Evidence: full diff 8e6ed84..5e0389b (9 files, +917/-27) read with LoadTask, LoadTaskService, LoadTaskExecutor, FileUploadController, FileMappingController, HugeGraphLoader, LoadContext, TaskManager, SingleInsertTask, FileReader, InputItemProgress and LoadProgress at this head. git grep at 8e6ed84 finds no Loader shutdown() or close() call anywhere in hubble-be/src/main. FileLoadTest.testLoadIncrementalModeAndLoadFailure shows the skip behaviour for the CLI. No tests were run for this review. CI: every check on this head passes or is skipped.

[bitflicker64]

Blocking: no. Score: 8.5/10 (10 - 0.5 shared-mode reset from thread 4261511700, still Minor at this head - 0.5 combined 814+815+3331 validation from @imbajin review 5513838881 - 0.5 admission lock held across Server calls, new = 8.5). Summary: At 3dbdd8d, Hubble Loaders close without writing parsed offsets. Task, job and source deletion now wait for native cleanup and the final callback before the metadata transaction. One new Minor note covers the process-wide admission lock in start(), resume() and retry(). Fixed since the earlier reviews: resume/retry cleanup and kept PAUSED/STOPPED status (2b2d66c); lost mode-reset failure (5e0389b); rows skipped on resume (ffa6318). Hubble now builds its Loaders with saveProgress=false, LoadContext.close holds the only progress write, and LoadTaskCheckpointTest covers both the semaphore pause and the single-insert failure. Scope: the branch shares eight early commits with #815, but a524308 removed that content, and the net diff against master touches no #815 file. The growth to +5394 comes from ffa6318 (deletion lifecycle, job and upload reservations, job-bound upload tokens) and about 3.6k lines of tests. Evidence: offline on JDK 17, LoadContextTest 36/36 passes. LoadTaskServiceTest, LoadTaskCheckpointTest, FileUploadControllerTest, JobManagerServiceTest, LoaderScopeControllerTest, IngestTransactionIntegrationTest and JobAdmissionIntegrationTest pass 123/123, and the new classes are in the hubble-be UnitTestSuite. CI at this head is still queued. Not run: PD/HStore, the UI, and the combined 814+815+3331 build.

[bitflicker64]

Minor: start() now builds the task inside synchronized (this.admissionLock). buildLoadTask reads the schema over HTTP (vlService.get and elService.get) and creates the Loader's clients, which also checks the Server version. resume and retry do the same under this lock through task.reconnect(token), which builds a new HugeGraphLoader. There is one lock per service, shared by all graphs.

While one of these waits on a slow Server (the schema client's client.request_timeout defaults to 310 s), every other user of the lock waits too: other starts, resume and retry, delete (requestJobStopForDelete, checkCallbackWaitAllowed), pauseAllTasks at shutdown, and the callback of every finished import (complete() -> release()), which also keeps its executor thread. The base built the task without any lock.

Please build the Loader before taking the lock, keep only the admission check, the metadata write and the dispatch inside it, and close the unused Loader if admission is refused.

[imbajin]

Confirmed from the current call chain: schema/version HTTP and Loader construction run under the shared admission monitor, so a slow bootstrap can delay other admission, callback release, delete and shutdown work. I have recorded this as a separate lock-granularity follow-up so the current checkpoint/cleanup change stays focused.

The recorded default-30-second runtime result covers eight already-admitted imports and one queued import. It does not establish a bound for arbitrary slow Server bootstrap. This thread remains open for that follow-up.

[imbajin]

Current-head CI diagnosis (2026-10-15): #814, #815 and #818 fail on the same existing CDC fixture omission: /job/bin/get-params.sh is missing beside the launcher, so the recovery test stops before Flink submission. Independent log inspection and cross-checking found all other selected jobs successful.

Evidence: #814 https://github.com/apache/hugegraph-toolchain/actions/runs/38654275910/job/116162549052 ; #815 https://github.com/apache/hugegraph-toolchain/actions/runs/38654305112/job/116162777279 ; #818 https://github.com/apache/hugegraph-toolchain/actions/runs/38654335307/job/116162814503 .

The existing #822 correction copies this helper and its current packaged CDC job passes. However, #822 now has a separate locked Server SDK dependency-resolution failure, so it is not fully green. Resolve that CI issue, land the fixture correction, then incorporate it before validating this stack. Repeating these unchanged stack heads would retain the missing file. This shared fixture failure does not demonstrate a new Hubble cleanup/ownership regression, and the separate admission-monitor limitation remains open. No score change follows from this diagnosis alone.

Reviewed heads: #814 bf86938; #815 ac8387e; #818 e83ff23.