[Caideyipi]

Description

Local whole-file LOAD exposes the destination TsFile before installing its resource and mods. A DataNode process crash in that window can let restart recover the file without its deletions and make deleted values visible again.

Stage the data under .tsfile.load-pending, install the resource and mods, finish legacy mods conversion synchronously, and publish the TsFile with a same-directory atomic rename. Keep the source file group until publication, then honor deleteOriginFile. Clean unpublished groups on installation failure and restart, and bind the resource's cached mods handle to the installed file.

Regression coverage

• Extend DataRegionTest to check source retention/deletion with and without mods, failure when the resource sidecar is missing, immediate staging cleanup, and the installed mods handle and contents.
• Add IoTDBLoadTsFileInstallIT, using real SQL and a shared 1C1D Simple-consensus fixture. JDWP/JDI pauses the actual local whole-file installation before mods, after mods but before publication, and after publication but before the client response. Each boundary covers both on-success policies and both mods formats: 3 methods, 12 crash combinations. Recovery is checked before any explicit new LOAD, with exact values, nulls, deletions, source/target file groups, and actual DataRegion readiness. JDWP is opt-in and bound to loopback in the test environment.

Validation

Before the master merge

• Built the required IoTDB dependency reactor from clean source, then rebuilt the final DataNode and downstream node templates after the last edits.
• The new unit method and the complete new IT class passed against the final artifacts: 1 unit method and 3 IT methods, with zero failures, errors, or skips. All newly added tests were executed locally.
• 19 existing related LOAD utility/manager unit methods also passed earlier in this task.
• Spotless, the final staged diff whitespace check, and IT selection validation passed (830 classes, 3 shards).

Executed locally on Windows with JDK 17.0.15 and Maven 3.8.8, using the ClusterIT profile with the explicit 1C1D fixture. Verification used -Drat.skip=true -Denforcer.skip=true and TsFile version 2.5.1-261023-20261023.065606-1. TsFile dependencies used published binaries and were not rebuilt or fully source-traced. The full suite and zh-locale reactor were not run; remote CI results are not included. This validates process-crash recovery; machine power-loss durability was not tested.

After the master merge

Merged master at 207503295e3 in commit 23a8ec1776d. The resolution preserves both the JDWP configuration and generic JVM options, as well as both independently added unit tests, without changing their logic.

Spotless and diff whitespace checks passed for the merge. Compilation and runtime tests were not rerun after the merge; the test results above refer to the pre-merge revision. Remote CI results are not included.

[codecov]

Codecov Report

❌ Patch coverage is 80.00000% with 16 lines in your changes missing coverage. Please review.
✅ Project coverage is 48.53%. Comparing base (2075032) to head (0871f70).
⚠️ Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
.../iotdb/db/storageengine/dataregion/DataRegion.java	76.11%	16 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##             master   #19136   +/-   ##
=========================================
  Coverage     48.53%   48.53%           
  Complexity      635      635           
=========================================
  Files          5573     5573           
  Lines        412215   412256   +41     
  Branches      54338    54344    +6     
=========================================
+ Hits         200059   200108   +49     
+ Misses       212156   212148    -8     

☔ 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.

[Caideyipi]

Initial independent self-review (one round).

Reviewed 100718bdc589886bfda75205045952c51dfcf88e against e92ecf1d5d62de0c67a52e80ff6cb7e307ab6583: the complete original six-file diff and the relevant installation, recovery, source-cleanup, debugger, and test context.

Fixed in e185fb836b69aa6917d79dbd92362bf72445a2fc:

• P2: A committed LOAD could be reported as failed. Source deletion errors in DataRegion.java were propagated after atomic publication and registration. A caller retry could import the file again. Source cleanup now logs the failure and preserves the committed success result.
• P2: Retained source data must retain its deletion sidecars. The existing LoadSingleTsFileNode.clean continued deleting resource/mods after a source-data deletion failure. This adjacent cleanup path also needed a guard when committed installation failures became nonfatal; it now stops if source-data deletion fails.

Added a local whole-file LOAD regression that pauses at TsFileManager.add exit, after publication/registration and before source cleanup. It covers data/resource deletion failures and both mods formats, requires the original request to still await its response before injection, then verifies SQL success, the expected source group, every value/null, and applied deletions.

Validation:

• Spotless passed with the integration-test profile.
• Clean source build of the IT dependency closure passed; the source-policy unit method passed (1 method, 0 failures/errors/skips). After the adjacent cleanup guard, DataNode and downstream templates were rebuilt clean using the unchanged upstream artifacts from that same source build.
• Exact Failsafe selection passed: the new IT method, four internal combinations, 0 failures/errors/skips. Node logs confirm all four injected cleanup failures; the template DataNode JAR matches the final built JAR by SHA256.
• Git Bash IT selection/category checks passed for all three shards (830 classes); whitespace checks passed.
• The earlier crash tests had already passed locally. This round reran only affected checks. An initial IT assertion incorrectly expected mods to remain after SQL's successful-batch cleanup; that assertion was corrected, and source-data failure coverage was added.
• An initial resumed build encountered incompatible installed IoTDB artifacts and a missing Surefire empty-selection option; rebuilding the full required source closure with the correct filters resolved it.
• Not run/verified: full suite, full zh reactor, remote CI, or machine power-loss durability. RAT and Enforcer were skipped; external TsFile snapshot binaries were not rebuilt from source.

Deferred, unchanged as requested: DataRegion.java:4867 requests limiter tokens using the destination file length before creation, potentially charging zero bytes. This predates this PR.

Final local HEAD = confirmed remote branch = confirmed PR head: e185fb836b69aa6917d79dbd92362bf72445a2fc. The worktree is clean and the PR remains Draft. A fresh independent re-review is required because this round found behavioral issues; no second review round was performed.

[Caideyipi]

Independent re-review, one round.

Reviewed the complete seven-file diff at e185fb836b69aa6917d79dbd92362bf72445a2fc against e92ecf1d5d62de0c67a52e80ff6cb7e307ab6583, including installation/recovery, source cleanup, cached mods handles, JDI boundaries, readiness/isolation, and the file/value/null/deletion assertions.

• P2 fixed: the legacy mods cache rebinding counted the installed mods twice. Synchronous conversion already records its target handle in FileMetrics; constructing another metrics-recording handle doubled file count and size, leaving residual counts after removal. The fix reuses the conversion result while discarding a cached source handle. The source-policy unit regression now covers retain/delete with no, legacy, and modern mods, with exact count/size increments and installed-handle/deletion checks.
• Deferred, unchanged P2: the load limiter reads the destination length before its creation and can charge zero bytes. This predates this PR and remains outside this repair.

Validation passed locally on Windows with JDK 17.0.15 and Maven 3.8.8:

• A clean required-source reactor rebuilt all 37 required IoTDB modules and node templates. The selected unit method passed all six combinations, with zero failures/errors/skips. The reactor's fixed CLI environment-script execution also passed.
• Targeted Failsafe verification ran exactly two affected crash methods (after mods installation/before publication, and after publication/before acknowledgement), eight combinations, with zero failures/errors/skips. Surefire was explicitly suppressed. The node template DataNode JAR matched the newly built JAR by SHA256.
• Spotless and the final staged whitespace check passed. IT discovery/categories were unchanged; prior selection evidence and unaffected test passes were retained. All newly added tests have been executed locally.

RAT and Enforcer were skipped. Published TsFile binaries were reused and were not rebuilt or fully source-traced. The full suite and zh-locale reactor were not run; remote CI was not inspected or awaited. This validates process crashes; power-loss durability was not tested.

Final local HEAD, confirmed source-branch head, and PR head are all bf65545e4252945454f78c6febf008cc5e8bbe65. The worktree is clean. PR remains Draft as requested. This behavioral finding requires a fresh independent session; no second review round was performed here.

[Caideyipi]

Independent re-review, one round.

Reviewed the complete seven-file diff at bf65545e4252945454f78c6febf008cc5e8bbe65 against the specified base e92ecf1d5d62de0c67a52e80ff6cb7e307ab6583, including the installation/recovery commit boundary, both mods formats, source cleanup, cached handles and metrics, debugger reliability, readiness/isolation, and file/value/null/deletion assertions. The advanced master branch was not merged or rebased into this change.

• P2 fixed: mods were counted before atomic publication. A failed publication, or a legacy conversion that fails after creating its output, rolls back by deleting staging files without undoing FileMetrics. Repeated failed LOAD attempts therefore inflate mods count and size. Installation now converts legacy mods without recording metrics and registers exactly one target handle only after publication. Cached source handles are still replaced; the existing recovery conversion API keeps its default metric behavior. The additional helper change is limited to TsFileResource.
• The existing source-policy regression now also rejects publication with both mods formats and injects a legacy-conversion completion error. It verifies unchanged metrics after failure, source bytes/sidecars, rollback, and the subsequent successful installation, alongside the existing six source-policy/mods combinations and exact successful count/size assertions.
• Deferred, unchanged P2: the load limiter reads the destination length before creation and can charge zero bytes. This predates the PR and was left unchanged as requested.

Validation on Windows, JDK 17.0.15 / Maven 3.8.8:

• Clean required-source reactor: all 37 modules and node templates passed (exit 0, 4m37s). The expanded source-policy unit method passed with zero failures/errors/skips. The reactor's fixed CLI environment-script execution also passed, separately from the selected unit count.
• Three existing synchronous/asynchronous conversion unit tests passed with zero failures/errors/skips, checking the default API behavior.
• Exact Failsafe selection passed: two affected crash methods, eight policy/mods combinations, zero failures/errors/skips. Surefire was explicitly suppressed with a nonexistent class. The node template DataNode JAR matched the newly built artifact by SHA256.
• Spotless and whitespace checks passed. IT discovery, categories, and selection were unchanged, so prior Git Bash selection evidence and unaffected test passes were retained. All newly added tests have been executed locally and ultimately passed. No build or test failed in this round.

RAT and Enforcer were skipped. External TsFile and transitive snapshot binaries were not rebuilt or fully source-traced. The full suite and zh reactor were not run; remote CI was not verified or awaited. These tests cover process crashes, not machine power loss.

Final local HEAD = confirmed source-branch head = confirmed PR head: 079c9e0a61e4d701ec5ee3722dfa114f4ed5762f. Worktree clean. The PR remains Draft, as requested. This behavioral finding requires a fresh independent session; no second review round was performed here.

[Caideyipi]

Independent re-review, round 4 (one round).

Reviewed the complete eight-file diff at 079c9e0a61e4d701ec5ee3722dfa114f4ed5762f against the specified base e92ecf1d5d62de0c67a52e80ff6cb7e307ab6583 and relevant production/test context. The advanced master branch was not merged or rebased into this change.

No new actionable in-scope findings; no fixes were needed. Coverage included staging and recovery, resource and both mods formats, atomic publication and failure cleanup, source policies and cached handles, metric registration, the new TsFileResource conversion overload/future and existing recovery defaults, JDI reliability, all three crash windows, outstanding requests versus committed installations and explicit new submissions, exact file/value/null/deletion assertions, Region readiness and isolation. The latest failed-publication/conversion regression's injection, assertions and cleanup were also reviewed. The separately tracked #19141 issue and #19128 were excluded as requested.

Verification in this round was read-only: all eight source hashes matched the recorded final build manifest, and the fixed-base whitespace check passed. Retained final XML was inspected for actual method selection and zero failures/errors/skips, including the latest source-policy/default-conversion tests and affected crash ITs. IT reports confirm Failsafe -Dit.test selection and explicit Surefire suppression. All newly added tests had already been executed locally and ultimately passed. With unchanged code and no uncovered verification gap, builds and tests were not repeated; these are existing execution results, not new passes in this round.

Existing limits remain: the full suite and full zh reactor were not run; remote CI was not independently verified or awaited. Recorded builds skipped RAT/Enforcer and used external TsFile/transitive snapshot binaries that were not rebuilt or fully source-traced. The evidence covers process crashes, not machine power loss.

Final local HEAD = confirmed source-branch head = confirmed PR head: 079c9e0a61e4d701ec5ee3722dfa114f4ed5762f. Worktree clean. No commit or push was necessary. The PR remains Draft under the explicit request; no ready transition or second review round was performed.

[Caideyipi]

CI investigation for job 127671379005:

• The LOAD failure was caused by the cleanup test merged from master still expecting sidecar deletion after source-data deletion failed. This PR intentionally retains the complete source group in that case. Updated the assertion and added a separate case proving that a resource-sidecar deletion failure does not prevent cleaning the remaining mods after the data file is gone. Production code is unchanged by this update.
• The PipeProcessorSubtaskExecutorTest failure is independent of this PR. Its Mockito spy was configured after registration exposed it to background workers; the CI exception was thrown from the worker's isClosed() call rather than submitSelf(). The ordering fix is in a separate master-based candidate and is not included here.

Validation on Windows with JDK 17.0.15 and Maven 3.8.8: rebuilt the required 28-module IoTDB dependency reactor from clean source, then clean-compiled this PR's DataNode against those source-identical prerequisite artifacts. All 4 LoadTsFileNodeTest methods and the DataRegion source-group installation regression passed: 5 tests, zero failures, errors or skips. Spotless and diff whitespace checks passed. The independent Pipe candidate's 17 executor tests and master's 3 original LOAD tests also passed separately. Existing local Maven configuration skipped Enforcer; TsFile 2.5.1-261023-SNAPSHOT came from the dependency cache and was not rebuilt or source-traced. The full suite, IT suite and zh locale were not rerun for this test-only repair.

Investigated starting SHA: 23a8ec1. Final local and confirmed remote branch/PR SHA: 0871f70. This was focused CI diagnosis, not a complete independent PR review; the PR remains draft pending a fresh full-diff re-review.

[Caideyipi]

Incremental re-review (one round).

Reviewed 0871f7080b248a8f9b3f66998d63d6b51ad08508 relative to its direct parent 23a8ec1776d4550883794bd2014730834c164edc: the complete one-file LoadTsFileNodeTest increment and the cleanup, retry, sidecar-path, constructor and caller context needed to assess it. The conclusion covers this increment only.

No actionable in-scope findings; no fixes were needed.

• Source deletion failure: a non-empty directory deterministically makes the source deletion fail through all retries. The test requires the source path to remain and all three sidecars (.resource, .mods2, .mods) to remain regular files. These assertions detect accidental sidecar cleanup after the source deletion fails.
• Sidecar deletion failure: the source is a regular file, while the resource sidecar is a non-empty directory. The test requires the source to be deleted, the failed resource directory and its child to remain, and both mods files to be deleted. This detects cleanup stopping at the resource failure, including a shared try/catch or an early return. Unique temporary directories and finally-block cleanup keep the cases isolated.

Verification: the incremental git diff --check passed in this round. Existing results supplied for the reviewed SHA report passing Spotless, clean DataNode compilation and five focused tests, with zero failures/errors/skips. All added or modified tests had already been run locally. With no code changes or new verification gap, these checks were not rerun. The full suite and zh build were not run; remote CI was not inspected or awaited.

Final local HEAD = confirmed source-branch head = confirmed PR head: 0871f7080b248a8f9b3f66998d63d6b51ad08508. The worktree is clean; no commit or push was necessary. The PR remains Draft, as requested.

[sonarqubecloud]

Quality Gate failed

Failed conditions
C Reliability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE