Skip to content

[io] Fix data loss in nested directories during batched TFileMerger - #23791

Closed
JainAyush-01 wants to merge 4 commits into
root-project:masterfrom
JainAyush-01:fix/23666-hadd-nested-dirs
Closed

JainAyush-01 wants to merge 4 commits into
root-project:masterfrom
JainAyush-01:fix/23666-hadd-nested-dirs

Conversation

@JainAyush-01

@JainAyush-01 JainAyush-01 commented Oct 22, 2026 •

Copy link
Copy Markdown

This Pull request:

Fixes silent data loss when merging ROOT files containing nested subdirectories (depth >= 2) with chunking / batching enabled (TFileMerger::SetMaxOpenedFiles or hadd -n).

Changes or fixes:

Root Cause:

When TFileMerger merges files incrementally in batches (type & kIncremental), TFileMerger::MergeOne cleans up in-memory directory objects at lines 830–834:

if (ownobj && (!(type & kIncremental) || dirobj->GetFile() != target)) {
   dirobj->ResetBit(kMustCleanup);
   delete dirobj;
}

For top-level subdirectories (depth 1), target is the TFile* itself, so dirobj->GetFile() == target evaluates to true and dirobj is preserved across batches until the final fOutputFile->Write() flushes all accumulated objects.

However, for nested subdirectories at depth $\ge 2$ (e.g., nested/dir):

  • target is the parent TDirectoryFile* (nested)
  • dirobj->GetFile() returns the root TFile* (fOutputFile)

Because a TDirectoryFile* does not equal a TFile*, dirobj->GetFile() != target always evaluated to true. This prematurely deleted the in-memory subdirectory dirobj before subsequent batches could write and flush into it, causing silent loss of objects merged in later batches.

Fix:

Change dirobj->GetFile() != target to dirobj->GetFile() != fOutputFile. This correctly checks whether dirobj belongs to the output file being accumulated across incremental batches, ensuring nested directories and their contents remain intact in memory until written.

Testing:

  • Added GoogleTest regression test TEST(TFileMerger, NestedDirMaxOpenedFiles) in io/io/test/TFileMergerTests.cxx.
  • Validated with multi-level directory depths (depth 2 and depth 3) and batch configurations (-n 2, -n 3).
  • Verified 100% pass on full gtest-io-io-TFileMerger test suite via ctest.

Checklist:

  • tested changes locally
  • updated the docs (if necessary)

This PR fixes #23666


AI-assisted coding disclosure: I used an AI assistant to help navigate the codebase, structure the unit test, and format this PR description. I have personally reviewed, debugged, and tested the C++ changes locally and take full responsibility for them.

JainAyush-01 and others added 2 commits October 23, 2026 16:31
When TFileMerger merges files with SetMaxOpenedFiles (e.g. hadd -n),
merging is performed incrementally across batches with kIncremental.
In TFileMerger::MergeOne, the cleanup condition previously checked:
    if (ownobj && (!(type & kIncremental) || dirobj->GetFile() != target))

For nested subdirectories at depth >= 2, `target` is the parent
TDirectoryFile, whereas `dirobj->GetFile()` returns the root TFile
(`fOutputFile`). Consequently, `dirobj->GetFile() != target` always
evaluated to true for nested directories, prematurely deleting the
in-memory directory before fOutputFile->Write() could flush it,
causing silent data loss of objects merged in subsequent batches.

Comparing `dirobj->GetFile() != fOutputFile` properly ensures that
directories belonging to the output file are not prematurely deleted
during incremental merges.

Fixes root-project#23666. Added regression test NestedDirMaxOpenedFiles in
TFileMergerTests.cxx.
* Allocate TFile and TTree on the heap (to avoid corner-case issues with
  DeleteChangesMemory)
* Clarify intent of the test: it's not to use `kIncremental` but to
  exercise the `-n` option in hadd, using SetMaxOpenedFiles in
  TFileMerger
* Also check correct TTree entry values
@vepadulano
vepadulano force-pushed the fix/23666-hadd-nested-dirs branch from 629d49d to d901d78 Compare October 23, 2026 16:35
@enirolf
enirolf force-pushed the fix/23666-hadd-nested-dirs branch from 3438441 to 65ae899 Compare October 23, 2026 16:44
@github-actions

github-actions Bot commented Oct 23, 2026 •

Copy link
Copy Markdown

Test Results

    24 files      24 suites   3d 23h 28m 7s ⏱️
 3 900 tests  3 900 ✅ 0 💤 0 ❌
83 506 runs  83 506 ✅ 0 💤 0 ❌

Results for commit 971a552.

♻️ This comment has been updated with latest results.

@JainAyush-01

Copy link
Copy Markdown
Author

Thanks @vepadulano and @enirolf for improving the test and adding the comprehensive roottest coverage!

I noticed in the CI run for commit 65ae899 that the Alma10 build failed in gtest-io-io-TFileMerger due to histogram auto-registration being off in that configuration. Please let me know if you would like me to adjust the test or if you prefer to handle it directly. Happy to help!

@enirolf

enirolf commented Oct 26, 2026 •

Copy link
Copy Markdown
Contributor

Hi @JainAyush-01, thank you for the proposed fix. I've changed the test to also run correctly without object auto-registration, let's see if the CI is happy now.

@enirolf enirolf assigned vepadulano and enirolf and unassigned pcanal Oct 26, 2026
@enirolf enirolf added bug priority:critical in:I/O experiment Affects an experiment / reported by its software & computimng experts AI-Assisted labels Oct 26, 2026
@enirolf enirolf added this to the 6.42.00 milestone Oct 26, 2026
@enirolf

enirolf commented Oct 28, 2026

Copy link
Copy Markdown
Contributor

Hi @JainAyush-01, thank you again for pointing us in the right direction for the fix.
However, on our side, we still had to invest quite some time into understanding how exactly this proposed fix solved the underlying issue, and take care into validating it. We concluded that this PR only partially addresses the problem, both in terms of the fix and the testing thereof. As such, we have decided to instead open #23878 and work from there, and close this PR.

@enirolf enirolf closed this Oct 28, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AI-Assisted bug experiment Affects an experiment / reported by its software & computimng experts in:I/O priority:critical

Projects

None yet

Development

Successfully merging this pull request may close these issues.

hadd -n omits object from last file for objects in nested TDirectories

4 participants