From c4dc0caf8b94a684349a5b9bde3e6983eda2510c Mon Sep 17 00:00:00 2001 From: Jakob Blomer Date: Tue, 2 Jun 2026 16:43:26 +0200 Subject: [PATCH 01/12] [tree] add overwrite unit test --- tree/tree/test/CMakeLists.txt | 2 +- tree/tree/test/TTreeOverwrite.cxx | 40 +++++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+), 1 deletion(-) create mode 100644 tree/tree/test/TTreeOverwrite.cxx diff --git a/tree/tree/test/CMakeLists.txt b/tree/tree/test/CMakeLists.txt index 4e903b06daafc..2bff62cc1e114 100644 --- a/tree/tree/test/CMakeLists.txt +++ b/tree/tree/test/CMakeLists.txt @@ -30,6 +30,7 @@ if(imt) endif() ROOT_ADD_GTEST(testTChainSaveAsCxx TChainSaveAsCxx.cxx LIBRARIES RIO Tree) ROOT_ADD_GTEST(testTChainRegressions TChainRegressions.cxx LIBRARIES RIO Tree) +ROOT_ADD_GTEST(testTTreeOverwrite TTreeOverwrite.cxx LIBRARIES RIO Tree) ROOT_ADD_GTEST(testTTreeTruncatedDatatypes TTreeTruncatedDatatypes.cxx LIBRARIES RIO Tree) ROOT_ADD_GTEST(testTTreeRegressions TTreeRegressions.cxx LIBRARIES RIO Tree Hist) ROOT_ADD_GTEST(entrylist_addsublist entrylist_addsublist.cxx LIBRARIES RIO Tree) @@ -44,4 +45,3 @@ ROOT_GENERATE_DICTIONARY(EvolutionStruct ${CMAKE_CURRENT_SOURCE_DIR}/EvolutionSt LINKDEF EvolutionStructLinkDef.h OPTIONS -inlineInputHeader DEPENDENCIES RIO) - diff --git a/tree/tree/test/TTreeOverwrite.cxx b/tree/tree/test/TTreeOverwrite.cxx new file mode 100644 index 0000000000000..e44d8cef90a3b --- /dev/null +++ b/tree/tree/test/TTreeOverwrite.cxx @@ -0,0 +1,40 @@ +#include "gtest/gtest.h" + +#include + +#include +#include + +TEST(TTree, Overwrite) +{ + ROOT::TestSupport::FileRaii fileGuard("test_ttree_overwrite.root"); + + auto f = TFile::Open(fileGuard.GetPath().c_str(), "RECREATE"); + auto t = new TTree("t", ""); + f->Write(); + f->Close(); + delete f; + + f = TFile::Open(fileGuard.GetPath().c_str(), "UPDATE"); + EXPECT_FALSE(f->TestBit(TFile::kRecovered)); + t = f->Get("t"); + t->Delete("all"); + f->Write(); + f->Close(); + delete f; + + f = TFile::Open(fileGuard.GetPath().c_str(), "UPDATE"); + // Empty files are always "recovered" because they don't contain keys; however, TFile::Recover() doesn't + // set the kRecovered bit because there are no keys to recover. + EXPECT_FALSE(f->TestBit(TFile::kRecovered)); + t = new TTree("t", ""); + f->Write(); + f->Close(); + delete f; + + f = TFile::Open(fileGuard.GetPath().c_str(), "UPDATE"); + EXPECT_FALSE(f->TestBit(TFile::kRecovered)); + EXPECT_NE(nullptr, f->Get("t")); + f->Close(); + delete f; +} From 19fd3884e6243f547eb1630becfa0056e73ad16c Mon Sep 17 00:00:00 2001 From: Jakob Blomer Date: Tue, 2 Jun 2026 22:42:22 +0200 Subject: [PATCH 02/12] [io] add basic unit test for free segments --- io/io/test/TFileTests.cxx | 47 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 47 insertions(+) diff --git a/io/io/test/TFileTests.cxx b/io/io/test/TFileTests.cxx index 5ef2ed684feee..eedf0d65d8dff 100644 --- a/io/io/test/TFileTests.cxx +++ b/io/io/test/TFileTests.cxx @@ -332,3 +332,50 @@ TEST(TFile, UUID) TMemFile f("uuidtest.root", "RECREATE"); EXPECT_EQ('4', f.GetUUID().AsString()[14]); } + +TEST(TFile, DeleteKey) +{ + ROOT::TestSupport::FileRaii fileGuard("tfile_test_delete_keys.root"); + + auto fnCountGaps = [](const std::string &fileName) { + auto f = std::unique_ptr(TFile::Open(fileName.c_str())); + std::uint64_t nGaps = 0; + for (const auto &k : f->WalkTKeys()) { + if (k.fType == ROOT::Detail::TKeyMapNode::kGap) + nGaps++; + } + return nGaps; + }; + + auto f = std::unique_ptr(TFile::Open(fileGuard.GetPath().c_str(), "RECREATE")); + f->SetCompressionSettings(0); + f->Write(); + f->Close(); + + // The empty file should have no gaps. Note that gaps are created temporarily when certain keys are overwritten. + EXPECT_EQ(0, fnCountGaps(fileGuard.GetPath())); + + f = std::unique_ptr(TFile::Open(fileGuard.GetPath().c_str(), "UPDATE")); + std::vector v; + f->WriteObject(&v, "va0"); + f->WriteObject(&v, "va1"); + f->WriteObject(&v, "va2"); + f->Write(); + f->Close(); + // 2 gaps: new (larger) keys list and free list are written + EXPECT_EQ(2, fnCountGaps(fileGuard.GetPath())); + + f = std::unique_ptr(TFile::Open(fileGuard.GetPath().c_str(), "UPDATE")); + f->Delete("va1;*"); // should create small gap that cannot be merged, trapped between v0 and v2 + f->Write(); + f->Close(); + + EXPECT_EQ(3, fnCountGaps(fileGuard.GetPath())); + + f = std::unique_ptr(TFile::Open(fileGuard.GetPath().c_str(), "UPDATE")); + f->Delete("va2;*"); // gaps at the tail should merge + f->Write(); + f->Close(); + + EXPECT_EQ(2, fnCountGaps(fileGuard.GetPath())); +} From a198575badfe3ef13f8cd07befe361362e93cf23 Mon Sep 17 00:00:00 2001 From: Jakob Blomer Date: Tue, 2 Jun 2026 23:14:25 +0200 Subject: [PATCH 03/12] [io] minor improvements to TFile::MakeFree() --- io/io/inc/TFile.h | 3 +++ io/io/src/TFile.cxx | 46 +++++++++++++++++++++++++++++---------------- 2 files changed, 33 insertions(+), 16 deletions(-) diff --git a/io/io/inc/TFile.h b/io/io/inc/TFile.h index ee87f2bb96bb4..2cd0e29e02a99 100644 --- a/io/io/inc/TFile.h +++ b/io/io/inc/TFile.h @@ -276,6 +276,9 @@ class TFile : public TDirectoryFile { }; enum ERelativeTo { kBeg = 0, kCur = 1, kEnd = 2 }; enum { kStartBigFile = 2000000000 }; + enum { + kMaxGapSize = 2000000000 + }; // Maximum absolute value of the free segment on-disk size marker /// File type enum EFileType { // clang++ #include +#include #ifdef R__FBSD #include @@ -1504,27 +1505,40 @@ Bool_t TFile::IsOpen() const void TFile::MakeFree(Long64_t first, Long64_t last) { - TFree *f1 = (TFree*)fFree->First(); - if (!f1) return; - TFree *newfree = f1->AddFree(fFree,first,last); - if(!newfree) return; + assert(0 < first && first < last && last < fEND); + + TFree *f1 = static_cast(fFree->First()); + assert(f1); // There must always be at least the virtual free segment at the end of the file + + TFree *newfree = f1->AddFree(fFree, first, last); + assert(newfree); // AddFree() always succeeds + Long64_t nfirst = newfree->GetFirst(); - Long64_t nlast = newfree->GetLast(); - Long64_t nbytesl= nlast-nfirst+1; - if (nbytesl > 2000000000) nbytesl = 2000000000; - Int_t nbytes = -Int_t (nbytesl); + Long64_t nlast = newfree->GetLast(); + assert(nfirst > 0 && nfirst <= first && nlast >= last); + Long64_t nbytesl = nlast - nfirst + 1; + assert(nbytesl >= static_cast(sizeof(Int_t))); + + if (last == fEND - 1) + fEND = nfirst; + + if (nbytesl > TFile::kMaxGapSize) + nbytesl = TFile::kMaxGapSize; + + Int_t nbytes = -Int_t(nbytesl); char buffer[sizeof(Int_t)]; char *pbuffer = buffer; tobuf(pbuffer, nbytes); - if (last == fEND-1) fEND = nfirst; + Seek(nfirst); - // We could not update the meta data for this block on the file. - // This is not fatal as this only means that we won't get it 'right' - // if we ever need to Recover the file before the block is actually - // (attempted to be reused. - // coverity[unchecked_value] - WriteBuffer(buffer, sizeof(buffer)); - if (fMustFlush) Flush(); + if (WriteBuffer(buffer, sizeof(buffer)) != 0) { + // Not fatal, this only means that we won't get it 'right' + // if we ever need to Recover the file before the block is actually + // attempted to be reused. + Warning("TFile::MakeFree()", "failed to write free segment header"); + } + if (fMustFlush) + Flush(); } //////////////////////////////////////////////////////////////////////////////// From 897fed9c2924524d4e3552982d2cb972e2f9ec6d Mon Sep 17 00:00:00 2001 From: Jakob Blomer Date: Thu, 4 Jun 2026 09:56:04 +0200 Subject: [PATCH 04/12] [io] Preserve file segment links with large gaps The free segment header for gaps >2GB was previously truncated, leaving the chain of segments broken. Now we will link together several smaller gaps on disk to keep the chain intact. The free list, however, will still contain one large gap. --- io/io/src/TFile.cxx | 53 ++++++++++++++++++++++----------- io/io/test/TFileTests.cxx | 62 ++++++++++++++++++++++++++++++++++++++- 2 files changed, 97 insertions(+), 18 deletions(-) diff --git a/io/io/src/TFile.cxx b/io/io/src/TFile.cxx index 35bdb05060dd7..6be6d83fb1dd8 100644 --- a/io/io/src/TFile.cxx +++ b/io/io/src/TFile.cxx @@ -174,6 +174,7 @@ The structure of a directory is shown in TDirectoryFile::TDirectoryFile #include #include #include +#include #ifdef R__FBSD #include @@ -1513,30 +1514,48 @@ void TFile::MakeFree(Long64_t first, Long64_t last) TFree *newfree = f1->AddFree(fFree, first, last); assert(newfree); // AddFree() always succeeds - Long64_t nfirst = newfree->GetFirst(); - Long64_t nlast = newfree->GetLast(); + const Long64_t nfirst = newfree->GetFirst(); + const Long64_t nlast = newfree->GetLast(); assert(nfirst > 0 && nfirst <= first && nlast >= last); - Long64_t nbytesl = nlast - nfirst + 1; + Long64_t nbytesl = std::min(nlast, fEND) - nfirst + 1; assert(nbytesl >= static_cast(sizeof(Int_t))); - if (last == fEND - 1) - fEND = nfirst; + auto fnWriteGapHeader = [this](ULong64_t offset, ULong64_t gapSize) { + assert((gapSize <= TFile::kMaxGapSize) && (fEND > 0) && + ((offset + sizeof(Int_t)) <= static_cast(fEND))); + + auto nbytes = -static_cast(gapSize); + char buffer[sizeof(Int_t)]; + char *pbuffer = buffer; + tobuf(pbuffer, nbytes); + + Seek(offset); + if (WriteBuffer(buffer, sizeof(buffer)) != 0) { + // Not fatal, this only means that we won't get it 'right' + // if we ever need to Recover the file before the block is actually + // attempted to be reused. + Warning("TFile::MakeFree()", "failed to write free segment header"); + } + }; - if (nbytesl > TFile::kMaxGapSize) - nbytesl = TFile::kMaxGapSize; + Long64_t offset = nfirst; + while (nbytesl > TFile::kMaxGapSize) { + // For gaps larger than 2GB, link several consecutive gaps together. This has to be done because the size + // marker on disk is 32 bits. The free list, however, will still have one large gap because the free list + // uses 64 bit [first..last] pairs to represent gaps. - Int_t nbytes = -Int_t(nbytesl); - char buffer[sizeof(Int_t)]; - char *pbuffer = buffer; - tobuf(pbuffer, nbytes); + // Make sure that the second gap is large enough to write its size on disk + Long64_t gapSize = TFile::kMaxGapSize - sizeof(Int_t); + fnWriteGapHeader(offset, gapSize); - Seek(nfirst); - if (WriteBuffer(buffer, sizeof(buffer)) != 0) { - // Not fatal, this only means that we won't get it 'right' - // if we ever need to Recover the file before the block is actually - // attempted to be reused. - Warning("TFile::MakeFree()", "failed to write free segment header"); + nbytesl -= gapSize; + offset += gapSize; } + fnWriteGapHeader(offset, nbytesl); + + if (last == fEND - 1) + fEND = nfirst; + if (fMustFlush) Flush(); } diff --git a/io/io/test/TFileTests.cxx b/io/io/test/TFileTests.cxx index eedf0d65d8dff..00f4ccf060f70 100644 --- a/io/io/test/TFileTests.cxx +++ b/io/io/test/TFileTests.cxx @@ -2,6 +2,7 @@ #include #include #include +#include #include "gtest/gtest.h" @@ -337,10 +338,15 @@ TEST(TFile, DeleteKey) { ROOT::TestSupport::FileRaii fileGuard("tfile_test_delete_keys.root"); - auto fnCountGaps = [](const std::string &fileName) { + auto fnCountGaps = [](const std::string &fileName) -> std::uint64_t { auto f = std::unique_ptr(TFile::Open(fileName.c_str())); std::uint64_t nGaps = 0; for (const auto &k : f->WalkTKeys()) { + if (k.fLen == TFile::kMaxGapSize) { + // this used to indicate a truncated free segment (corrupt segment list). Gaps could still exactly + // be 2GB in size but not for the files in this unit test. + throw std::runtime_error("truncated free segment"); + } if (k.fType == ROOT::Detail::TKeyMapNode::kGap) nGaps++; } @@ -378,4 +384,58 @@ TEST(TFile, DeleteKey) f->Close(); EXPECT_EQ(2, fnCountGaps(fileGuard.GetPath())); + + // The following tests run out of memory on 32bit platforms + if (sizeof(std::size_t) == 4) { + printf("Skipping test partially on 32bit platform.\n"); + return; + } + + v.resize(1000 * 1000, 'x'); // next few objects are 1MB in size + f = std::unique_ptr(TFile::Open(fileGuard.GetPath().c_str(), "RECREATE")); + f->SetCompressionSettings(0); + f->WriteObject(&v, "vb0"); + f->WriteObject(&v, "vb1"); + f->WriteObject(&v, "vb2"); + f->WriteObject(&v, "vb3"); + v.resize(1000 * 1000 * 1000 - 100, 'x'); // almost 1GB + f->WriteObject(&v, "vc0"); + f->WriteObject(&v, "vc1"); + f->WriteObject(&v, "vc2"); + f->Write(); + EXPECT_GT(f->GetEND(), TFile::kStartBigFile); + f->Close(); + + // New file, no gaps + EXPECT_EQ(0, fnCountGaps(fileGuard.GetPath())); + + f = std::unique_ptr(TFile::Open(fileGuard.GetPath().c_str(), "UPDATE")); + f->Delete("vb1;*"); // Make a medium sized gap into which smaller objects fit, e.g. the free list + f->Delete("vb3;*"); // | + f->Delete("vc0;*"); // | + f->Delete("vc1;*"); // |---> Single merged gap in free list, multi-hop free segment on disk + f->Write(); + f->Close(); + + // Free list in gap created by vb1, one gap at the end because we have a smaller keys list. Two consecutive + // gaps for removed vb3, vc0, vc1. + EXPECT_EQ(4, fnCountGaps(fileGuard.GetPath())); + f = std::unique_ptr(TFile::Open(fileGuard.GetPath().c_str(), "UPDATE")); + EXPECT_FALSE(f->TestBit(TFile::kRecovered)); + // Only 3 real gaps plus one virtual gap at the end of the file + EXPECT_EQ(4, f->GetNfree()); + // Force the next open to recover the file + f->GetListOfKeys()->Clear(); + f->Write(); + f->Close(); + + { + // File recovery should work + ROOT::TestSupport::CheckDiagsRAII diagsRaii; + diagsRaii.requiredDiag(kInfo, "TFile::Recover", "recovered key vector", false); + f = std::unique_ptr(TFile::Open(fileGuard.GetPath().c_str(), "UPDATE")); + EXPECT_TRUE(f->TestBit(TFile::kRecovered)); + f->Write(); + f->Close(); + } } From 4c58a12529cc311fff265a75e8b9d4ba137cd9a1 Mon Sep 17 00:00:00 2001 From: Jakob Blomer Date: Thu, 4 Jun 2026 14:01:47 +0200 Subject: [PATCH 05/12] [io] fix recovery of free segments In TFile::Recover(), incorrect free segments were added during recovery. The file offset counter should be incremented _after_ adding the free segment, as it is done now. Also, remove an unnecessary `Seek()`, which will take place at the next loop iteration anyway. --- io/io/src/TFile.cxx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/io/io/src/TFile.cxx b/io/io/src/TFile.cxx index 6be6d83fb1dd8..e6f7a0ebf327b 100644 --- a/io/io/src/TFile.cxx +++ b/io/io/src/TFile.cxx @@ -2181,9 +2181,9 @@ Int_t TFile::Recover() break; } if (nbytes < 0) { + if (fWritable) + new TFree(fFree, idcur, idcur - nbytes - 1); idcur -= nbytes; - if (fWritable) new TFree(fFree,idcur,idcur-nbytes-1); - Seek(idcur); continue; } Version_t versionkey; From da9bcf256f52cb9a279040bc155e9b14313a28c0 Mon Sep 17 00:00:00 2001 From: Jakob Blomer Date: Thu, 4 Jun 2026 14:11:27 +0200 Subject: [PATCH 06/12] [io] check min/max gap size in recovery --- io/io/src/TFile.cxx | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/io/io/src/TFile.cxx b/io/io/src/TFile.cxx index e6f7a0ebf327b..ff9ce36c70db8 100644 --- a/io/io/src/TFile.cxx +++ b/io/io/src/TFile.cxx @@ -2181,6 +2181,10 @@ Int_t TFile::Recover() break; } if (nbytes < 0) { + if ((-nbytes < static_cast(sizeof(Int_t))) || (-nbytes > static_cast(TFile::kMaxGapSize))) { + Error("Recover", "Address = %lld\tNbytes = %d\t=====E R R O R=======", idcur, nbytes); + break; + } if (fWritable) new TFree(fFree, idcur, idcur - nbytes - 1); idcur -= nbytes; From d6678be1ca351b50dddd285933779d9be45cc97b Mon Sep 17 00:00:00 2001 From: Jakob Blomer Date: Thu, 4 Jun 2026 14:13:26 +0200 Subject: [PATCH 07/12] [io] always recover free list from linked segments Do not use an existing free list during file recovery. It will result in stale and/or duplicated entries in the recovered free list. Instead, only use the information picked up from the segment headers. --- io/io/src/TFile.cxx | 28 +++++++++++++++++++--------- io/io/src/TFree.cxx | 6 ++++-- 2 files changed, 23 insertions(+), 11 deletions(-) diff --git a/io/io/src/TFile.cxx b/io/io/src/TFile.cxx index ff9ce36c70db8..a852ea98bd8c2 100644 --- a/io/io/src/TFile.cxx +++ b/io/io/src/TFile.cxx @@ -2158,7 +2158,14 @@ Int_t TFile::Recover() fEND = Long64_t(size); - if (fWritable && !fFree) fFree = new TList; + if (fWritable) { + if (fFree) { + // Remove an existing free list because we will recover it from the chain of segments + fFree->Delete(); + delete fFree; + } + fFree = new TList(); + } Int_t nrecov = 0; nwheader = 1024; @@ -2233,15 +2240,18 @@ Int_t TFile::Recover() idcur += nbytes; } if (fWritable) { - Long64_t max_file_size = Long64_t(kStartBigFile); - if (max_file_size < fEND) max_file_size = fEND+1000000000; - TFree *last = (TFree*)fFree->Last(); - if (last) { - last->AddFree(fFree,fEND,max_file_size); - } else { - new TFree(fFree,fEND,max_file_size); + if (fFree->Last() && static_cast(fFree->Last())->GetLast() == idcur - 1) { + // If the last recovered segment is a free segment, remove it and replace it by a newly created artificial one + fEND = static_cast(fFree->Last())->GetFirst(); + delete fFree->Last(); + fFree->Remove(fFree->LastLink()); } - if (nrecov) Write(); + Long64_t max_file_size = Long64_t(kStartBigFile); + if (max_file_size < fEND) + max_file_size = fEND + 1000000000; + new TFree(fFree, fEND, max_file_size); + if (nrecov) + Write(); } return nrecov; } diff --git a/io/io/src/TFree.cxx b/io/io/src/TFree.cxx index 5a06ac2497ada..d70ebb8025810 100644 --- a/io/io/src/TFree.cxx +++ b/io/io/src/TFree.cxx @@ -92,7 +92,10 @@ TFree *TFree::AddFree(TList *lfree, Long64_t first, Long64_t last) } idcur = (TFree*)lfree->After(idcur); } - return 0; + + // never here + assert(false); + return nullptr; } //////////////////////////////////////////////////////////////////////////////// @@ -186,4 +189,3 @@ Int_t TFree::Sizeof() const if (fLast > TFile::kStartBigFile) return 18; else return 10; } - From 17f269803b9371b0517ef81567dab86faccfd740 Mon Sep 17 00:00:00 2001 From: Jakob Blomer Date: Thu, 4 Jun 2026 14:53:38 +0200 Subject: [PATCH 08/12] [io] merge consecutive gaps during recovery --- io/io/src/TFile.cxx | 12 +++++++++--- io/io/test/TFileTests.cxx | 23 ++++++++++++++++++----- 2 files changed, 27 insertions(+), 8 deletions(-) diff --git a/io/io/src/TFile.cxx b/io/io/src/TFile.cxx index a852ea98bd8c2..2ae71cc6951a5 100644 --- a/io/io/src/TFile.cxx +++ b/io/io/src/TFile.cxx @@ -1542,7 +1542,7 @@ void TFile::MakeFree(Long64_t first, Long64_t last) while (nbytesl > TFile::kMaxGapSize) { // For gaps larger than 2GB, link several consecutive gaps together. This has to be done because the size // marker on disk is 32 bits. The free list, however, will still have one large gap because the free list - // uses 64 bit [first..last] pairs to represent gaps. + // uses 64 bit [first..last] pairs to represent gaps. File recovery will merge consecutive gaps. // Make sure that the second gap is large enough to write its size on disk Long64_t gapSize = TFile::kMaxGapSize - sizeof(Int_t); @@ -2192,8 +2192,14 @@ Int_t TFile::Recover() Error("Recover", "Address = %lld\tNbytes = %d\t=====E R R O R=======", idcur, nbytes); break; } - if (fWritable) - new TFree(fFree, idcur, idcur - nbytes - 1); + if (fWritable) { + const Long64_t last = idcur - nbytes - 1; + if (fFree->Last() && static_cast(fFree->Last())->GetLast() + 1 == idcur) { + static_cast(fFree->Last())->SetLast(last); + } else { + new TFree(fFree, idcur, last); + } + } idcur -= nbytes; continue; } diff --git a/io/io/test/TFileTests.cxx b/io/io/test/TFileTests.cxx index 00f4ccf060f70..55ff8f85d5583 100644 --- a/io/io/test/TFileTests.cxx +++ b/io/io/test/TFileTests.cxx @@ -8,15 +8,16 @@ #include -#include "TFile.h" -#include "TMemFile.h" #include "TDirectory.h" +#include "TEnv.h" +#include "TFile.h" +#include "TFree.h" #include "TKey.h" +#include "TMemFile.h" #include "TNamed.h" #include "TPluginManager.h" -#include "TROOT.h" // gROOT +#include "TROOT.h" #include "TSystem.h" -#include "TEnv.h" // gEnv TEST(TFile, WriteObjectTObject) { @@ -430,12 +431,24 @@ TEST(TFile, DeleteKey) f->Close(); { - // File recovery should work ROOT::TestSupport::CheckDiagsRAII diagsRaii; diagsRaii.requiredDiag(kInfo, "TFile::Recover", "recovered key vector", false); f = std::unique_ptr(TFile::Open(fileGuard.GetPath().c_str(), "UPDATE")); EXPECT_TRUE(f->TestBit(TFile::kRecovered)); + // We got one more free gap due to the replacement of the empty keys list. Otherwise, we still want to see + // that the large gap was merged from the smaller segments. + EXPECT_EQ(5, f->GetNfree()); + bool foundLargeGap = false; + for (const auto gap : ROOT::Detail::TRangeStaticCast(f->GetListOfFree())) { + if (gap->GetLast() - gap->GetFirst() >= TFile::kMaxGapSize) { + foundLargeGap = true; + break; + } + } + EXPECT_TRUE(foundLargeGap); f->Write(); f->Close(); } + // Same as before the recovery + EXPECT_EQ(4, fnCountGaps(fileGuard.GetPath())); } From d98324371c04c5df01ca551f57ec1b3286be4c53 Mon Sep 17 00:00:00 2001 From: Jakob Blomer Date: Thu, 4 Jun 2026 15:41:35 +0200 Subject: [PATCH 09/12] [io] remove useless statement in TFree::GetBestFree() --- io/io/src/TFree.cxx | 1 - 1 file changed, 1 deletion(-) diff --git a/io/io/src/TFree.cxx b/io/io/src/TFree.cxx index d70ebb8025810..fcdc3d06d9650 100644 --- a/io/io/src/TFree.cxx +++ b/io/io/src/TFree.cxx @@ -129,7 +129,6 @@ void TFree::FillBuffer(char *&buffer) TFree *TFree::GetBestFree(TList *lfree, Int_t nbytes) { TFree *idcur = this; - if (idcur == 0) return 0; TFree *idcur1 = 0; do { Long64_t nleft = Long64_t(idcur->fLast - idcur->fFirst +1); From 86a8bfd405050d4c852be6c04fdbbee0beb78a82 Mon Sep 17 00:00:00 2001 From: Jakob Blomer Date: Thu, 4 Jun 2026 15:41:54 +0200 Subject: [PATCH 10/12] [io] add test for adding object close to 1GiB --- io/io/test/TFileTests.cxx | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/io/io/test/TFileTests.cxx b/io/io/test/TFileTests.cxx index 55ff8f85d5583..f1cdce9300ff2 100644 --- a/io/io/test/TFileTests.cxx +++ b/io/io/test/TFileTests.cxx @@ -452,3 +452,36 @@ TEST(TFile, DeleteKey) // Same as before the recovery EXPECT_EQ(4, fnCountGaps(fileGuard.GetPath())); } + +TEST(TFile, KeySizeLimit) +{ + // The following tests run out of memory on 32bit platforms + if (sizeof(std::size_t) == 4) { + GTEST_SKIP() << "Skipping test on 32bit platform."; + } + + ROOT::TestSupport::FileRaii fileGuard("tfile_test_key_size_limit.root"); + + auto f = std::unique_ptr(TFile::Open(fileGuard.GetPath().c_str(), "RECREATE")); + f->SetCompressionSettings(0); + + // Check that we can add keys >1GB (but smaller than 1GiB, obviously) in small and large files. + // This does work even though the last, virtual free segment is 1GB (and not 1GiB). The reason it works is + // that when the last free segment is not large enough, the code path that supports upgrading from a small file + // to a large file is activated and extends the last free segment as needed. + + std::vector v; + v.resize(1000 * 1000 * 1000 + 100, 'x'); // more than 1GB but less the 1GiB + f->WriteObject(&v, "v0"); + EXPECT_LT(f->GetEND(), TFile::kStartBigFile); + f->WriteObject(&v, "v1"); + EXPECT_GT(f->GetEND(), TFile::kStartBigFile); + f->Write(); + f->Close(); + + f = std::unique_ptr(TFile::Open(fileGuard.GetPath().c_str(), "UPDATE")); + EXPECT_GT(f->GetEND(), TFile::kStartBigFile); + f->WriteObject(&v, "v2"); + f->Write(); + f->Close(); +} From 09891248f53a662e9f1d3ed97613babf60df269a Mon Sep 17 00:00:00 2001 From: Jakob Blomer Date: Thu, 4 Jun 2026 15:59:38 +0200 Subject: [PATCH 11/12] [io] add reproducer of #19245 as test --- io/io/test/TFileTests.cxx | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/io/io/test/TFileTests.cxx b/io/io/test/TFileTests.cxx index f1cdce9300ff2..948ccaa3865d8 100644 --- a/io/io/test/TFileTests.cxx +++ b/io/io/test/TFileTests.cxx @@ -451,6 +451,33 @@ TEST(TFile, DeleteKey) } // Same as before the recovery EXPECT_EQ(4, fnCountGaps(fileGuard.GetPath())); + + // Write in large gap (between 1GB and 2GB), reproducer of issue https://github.com/root-project/root/issues/19245 + f = std::unique_ptr(TFile::Open(fileGuard.GetPath().c_str(), "RECREATE")); + f->SetCompressionSettings(0); + + v.resize(1000 * 1000 * 1000 - 100, 'x'); // almost 1GB + f->WriteObject(&v, "big1"); + f->WriteObject(&v, "big2"); + v.resize(1024 * 1024, 'x'); + f->WriteObject(&v, "small1"); + f->Write(); + f->Close(); + + f = std::unique_ptr(TFile::Open(fileGuard.GetPath().c_str(), "UPDATE")); + // Creates a combined gap close to 2GB + f->Delete("big1;*"); + f->Delete("big2;*"); + f->Write(); + f->Close(); + + f = std::unique_ptr(TFile::Open(fileGuard.GetPath().c_str(), "UPDATE")); + f->WriteObject(&v, "small2"); + f->Write(); + f->Close(); + + f = std::unique_ptr(TFile::Open(fileGuard.GetPath().c_str(), "UPDATE")); + EXPECT_FALSE(f->TestBit(TFile::kRecovered)); } TEST(TFile, KeySizeLimit) From 1d071c704396a262d600e181fbadb2d65bac062e Mon Sep 17 00:00:00 2001 From: Jakob Blomer Date: Wed, 10 Jun 2026 13:06:06 +0200 Subject: [PATCH 12/12] [io] fix creating key in large gap If a new key is created in a free segment such that the remaining size is larger than 2G, we have to first read the original free segment header before we update the new, smaller header. Along the way, we change TKey::fLeft from Int_t to Long64_t. --- io/io/inc/TKey.h | 10 ++- io/io/src/TKey.cxx | 104 +++++++++++++++++++++-- io/io/test/TFileTests.cxx | 155 ++++++++++++++++++++++++++++++++++ tree/ntuple/src/RMiniFile.cxx | 13 +-- 4 files changed, 269 insertions(+), 13 deletions(-) diff --git a/io/io/inc/TKey.h b/io/io/inc/TKey.h index 75f27524ead31..cd4f4ea5b2de0 100644 --- a/io/io/inc/TKey.h +++ b/io/io/inc/TKey.h @@ -21,6 +21,8 @@ class TBuffer; #include "TBuffer.h" #endif +#include + class TBrowser; class TDirectory; class TFile; @@ -38,6 +40,11 @@ class TKey : public TNamed { Int_t UnzipBuffer(char *targetBuffer, const char *compressedBuffer) const; protected: + // After a key that has been placed in a gap larger than the key itself, one or very rarely two marker bytes + // follow to restore the linked list of segments. The two char buffers are meant to be written to disk, i.e. + // they should contain big-endian encoded negative integer values for the size of a gap, or zero if unused. + using GapHeaderBuf_t = std::array, 2>; + Int_t fVersion; ///< Key version identifier Int_t fNbytes; ///< Number of bytes for the whole key on file (key header and data) Int_t fObjlen; ///< Length of uncompressed object in bytes @@ -47,7 +54,7 @@ class TKey : public TNamed { Long64_t fSeekKey; ///< Location of object on file Long64_t fSeekPdir; ///< Location of parent directory on file TString fClassName; ///< Object Class name - Int_t fLeft; ///< Number of bytes left in current segment + Long64_t fLeft; ///< Number of bytes left in current segment char *fBuffer; ///< Object buffer TBuffer *fBufferRef; ///< Pointer to the TBuffer object UShort_t fPidOffset; ///GetLast() - fSeekKey - nsize + 1); + fLeft = bestfree->GetLast() - fSeekKey - nsize + 1; } //*-*----------------- Case where new object fills exactly a deleted gap fNbytes = nsize; @@ -554,11 +554,11 @@ void TKey::Create(Int_t nbytes, TFile* externFile) //*-*----------------- Case where new object is placed in a deleted gap larger than itself if (fLeft > 0) { // found a bigger segment if (!fBuffer) { + // We reserve space for the new free segment size marker but we don't fill the buffer yet. + // We have to check (on writing, i.e. when we do I/O) if we are writing into a large gap (>2GB), + // in which case we need to read the first link size first. fBuffer = new char[nsize+sizeof(Int_t)]; } - char *buffer = fBuffer+nsize; - Int_t nbytesleft = -fLeft; // set header of remaining record - tobuf(buffer, nbytesleft); bestfree->SetFirst(fSeekKey+nsize); } @@ -1480,6 +1480,74 @@ void TKey::Streamer(TBuffer &b) } } +/// Fills zero, one, or two (rare) free segment header buffers into the provided `buffers` parameter. +/// If the key fits exactly in a provided gap or is at the end of the file, no buffer is filled. Otherwise, +/// the buffer for the integer immediately after the key data is filled with the size of the remaining, shortened gap. +/// In rare cases, it can be necessary to fill the second buffer with another free segment link. +/// Returns the number of buffers filled. +UShort_t TKey::FillGapHeaderBuffers(TFile &f, GapHeaderBuf_t &buffers) const +{ + if (fLeft <= 0) + return 0; + + // We must fill at least one free segment header + + if (fLeft <= TFile::kMaxGapSize) { + char *pbuf = buffers[0].data(); + tobuf(pbuf, -static_cast(fLeft)); + return 1; + } + + // Large gap, we need to read the free segment headers that are going to be overwritten by the key to find the + // first free segment header beyond the key. In fact, we must find a free segment header far enough beyond the + // key so that we can inject another, new free segment header between the key end and the existing one (which means + // key end + sizeof(int)) + const auto newGapOffset = fSeekKey + fNbytes; + auto existingGapOffset = fSeekKey; + auto prevGapOffset = 0; + do { + char readBuf[sizeof(Int_t)]; + Int_t header = 0; + + f.Seek(existingGapOffset); + if (f.ReadBuffer(readBuf, sizeof(Int_t))) { + Error("FillGapHeaderBuffers", "cannot read free segment link size at %lld", existingGapOffset); + return 0; + } + + char *pbuf = readBuf; + frombuf(pbuf, &header); + const Int_t gapSize = -header; + if (gapSize < static_cast(sizeof(Int_t)) || gapSize > TFile::kMaxGapSize || + gapSize > (newGapOffset - existingGapOffset + fLeft)) { + Error("FillGapHeaderBuffers", "invalid free segment header size %d at %lld", gapSize, existingGapOffset); + return 0; + } + + prevGapOffset = existingGapOffset; + existingGapOffset += gapSize; + } while (existingGapOffset <= newGapOffset + static_cast(sizeof(Int_t))); + + if ((prevGapOffset < newGapOffset) || (existingGapOffset - newGapOffset) <= TFile::kMaxGapSize) { + // Normal case: writing the new free segment header won't overwrite an existing one beyond the key. + // Rare case: the new free segment header will overwrite (part of) and existing one beyond the key. + // We can then lengthen the gap beteween [prevGapOffset, newGapOffset] except if this has already + // the maximum length. + char *pbuf = buffers[0].data(); + tobuf(pbuf, -static_cast(existingGapOffset - newGapOffset)); + return 1; + } + + // Very rare case: we need two free segment headers after the key + char *pbuf = buffers[0].data(); + tobuf(pbuf, -static_cast(sizeof(Int_t))); + const auto newGapSize = existingGapOffset - newGapOffset - sizeof(Int_t); + assert(newGapSize <= TFile::kMaxGapSize); + pbuf = buffers[1].data(); + tobuf(pbuf, -static_cast(newGapSize)); + return 2; +} + //////////////////////////////////////////////////////////////////////////////// /// Write the encoded object supported by this key. /// The function returns the number of bytes committed to the file. @@ -1498,9 +1566,21 @@ Int_t TKey::WriteFile(Int_t cycle, TFile* f) buffer = fBuffer; } - if (fLeft > 0) nsize += sizeof(Int_t); + GapHeaderBuf_t gapHeaderBuffers; + auto nGaps = FillGapHeaderBuffers(*f, gapHeaderBuffers); + assert(nGaps <= 2); + if (nGaps > 0) { + std::memcpy(fBuffer + fNbytes, gapHeaderBuffers[0].data(), sizeof(Int_t)); + nsize += sizeof(Int_t); + } + f->Seek(fSeekKey); Bool_t result = f->WriteBuffer(buffer,nsize); + + if (nGaps == 2) { + f->WriteBuffer(gapHeaderBuffers[1].data(), sizeof(Int_t)); + nsize += sizeof(Int_t); + } //f->Flush(); Flushing takes too much time. // Let user flush the file when they want. if (gDebug) { @@ -1525,9 +1605,21 @@ Int_t TKey::WriteFileKeepBuffer(TFile *f) Int_t nsize = fNbytes; char *buffer = fBuffer; - if (fLeft > 0) nsize += sizeof(Int_t); + GapHeaderBuf_t gapHeaderBuffers; + auto nGaps = FillGapHeaderBuffers(*f, gapHeaderBuffers); + assert(nGaps <= 2); + if (nGaps > 0) { + std::memcpy(fBuffer + fNbytes, gapHeaderBuffers[0].data(), sizeof(Int_t)); + nsize += sizeof(Int_t); + } + f->Seek(fSeekKey); Bool_t result = f->WriteBuffer(buffer,nsize); + + if (nGaps == 2) { + f->WriteBuffer(gapHeaderBuffers[1].data(), sizeof(Int_t)); + nsize += sizeof(Int_t); + } //f->Flush(); Flushing takes too much time. // Let user flush the file when they want. if (gDebug) { diff --git a/io/io/test/TFileTests.cxx b/io/io/test/TFileTests.cxx index 948ccaa3865d8..2760d20d95d3c 100644 --- a/io/io/test/TFileTests.cxx +++ b/io/io/test/TFileTests.cxx @@ -480,6 +480,161 @@ TEST(TFile, DeleteKey) EXPECT_FALSE(f->TestBit(TFile::kRecovered)); } +TEST(TFile, WriteInLargeGap) +{ + // The following tests run out of memory on 32bit platforms + if (sizeof(std::size_t) == 4) { + GTEST_SKIP() << "Skipping test on 32bit platform."; + } + + ROOT::TestSupport::FileRaii fileGuard("tfile_test_large_gap_at_end.root"); + auto f = std::unique_ptr(TFile::Open(fileGuard.GetPath().c_str(), "RECREATE")); + f->SetCompressionSettings(0); + std::vector v; + v.resize(1000 * 1000 * 1000 - 100, 'x'); // almost 1GB + f->WriteObject(&v, "v0"); + f->WriteObject(&v, "v1"); + f->WriteObject(&v, "v2"); + f->Write(); + f->Close(); + + f = std::unique_ptr(TFile::Open(fileGuard.GetPath().c_str(), "UPDATE")); + EXPECT_GT(f->GetEND(), TFile::kStartBigFile); + f->Delete("v0;*"); + f->Delete("v1;*"); + f->Delete("v2;*"); + v.clear(); + f->WriteObject(&v, "small"); //< This should not crash, i.e. the new key should be placed in the large gap + + int nConsecutiveGapsAfterSmall = -1; + for (const auto &k : f->WalkTKeys()) { + if (k.fType == ROOT::Detail::TKeyMapNode::kGap && nConsecutiveGapsAfterSmall >= 0) { + nConsecutiveGapsAfterSmall++; + continue; + } + if (k.fKeyName == "small") { + nConsecutiveGapsAfterSmall = 0; + continue; + } + if (nConsecutiveGapsAfterSmall >= 0) + break; + } + EXPECT_EQ(2, nConsecutiveGapsAfterSmall); + + TNamed x("x", ""); + f->WriteObject(&x, "x"); //< Update the streamer info so that it doesn't block shrinking the file + f->Write(); + f->Close(); + + f = std::unique_ptr(TFile::Open(fileGuard.GetPath().c_str())); + EXPECT_LT(f->GetEND(), TFile::kStartBigFile); +} + +TEST(TFile, WriteInLargeGapCornerCase) +{ + // The following tests run out of memory on 32bit platforms + if (sizeof(std::size_t) == 4) { + GTEST_SKIP() << "Skipping test on 32bit platform."; + } + + // Constructs a case that requires writing 2 free segment headers after a key + + ROOT::TestSupport::FileRaii fileGuard("tfile_test_write_in_large_gap_corner_case.root"); + auto f = std::unique_ptr(TFile::Open(fileGuard.GetPath().c_str(), "RECREATE")); + f->SetCompressionSettings(0); + std::vector v; + v.resize(1000 * 1000 * 1000 - 100, 'x'); // almost 1GB + f->WriteObject(&v, "v0"); + f->WriteObject(&v, "v1"); + f->WriteObject(&v, "v2"); + f->Write(); + f->Delete("v0;*"); + f->Delete("v1;*"); + f->Delete("v2;*"); + f->Write(); + + // We should have two large consecutive gaps + Long64_t seekGap = 0; + Long64_t gapSize = 0; // the combined gap size + for (const auto &k : f->WalkTKeys()) { + if (seekGap > 0) { + // second gap + EXPECT_EQ(ROOT::Detail::TKeyMapNode::kGap, k.fType); + gapSize += k.fLen; + break; + } + if (k.fLen > 1000000) { + EXPECT_EQ(ROOT::Detail::TKeyMapNode::kGap, k.fType); + seekGap = k.fAddr; + gapSize = k.fLen; + } + } + ASSERT_GT(seekGap, 0); + ASSERT_GT(gapSize, TFile::kMaxGapSize); + + // Create new key in the large gap; not huge but large enough to only fit in the large gap + TKey testKey("TEST", "TITLE", TObject::Class(), 1024 * 1024, f.get()); + EXPECT_EQ(seekGap, testKey.GetSeekKey()); + + // Manipulate the linked list of free segments in the large gap: + // - 2 empty free gaps + // - 1 max sized gap 1 byte after the key end + // - final segment pointing to the original end + std::array buf; + char *pbuf = buf.data(); + + Long64_t offset = seekGap; + f->Seek(offset); + tobuf(pbuf, -static_cast(sizeof(Int_t))); + EXPECT_FALSE(f->WriteBuffer(buf.data(), sizeof(Int_t))); + + offset += sizeof(Int_t); + pbuf = buf.data(); + tobuf(pbuf, -static_cast(seekGap + testKey.GetNbytes() + 1 - offset)); + EXPECT_FALSE(f->WriteBuffer(buf.data(), sizeof(Int_t))); + + offset += testKey.GetNbytes() + 1 - sizeof(Int_t); + f->Seek(offset); + pbuf = buf.data(); + tobuf(pbuf, -static_cast(TFile::kMaxGapSize)); + EXPECT_FALSE(f->WriteBuffer(buf.data(), sizeof(Int_t))); + + offset += TFile::kMaxGapSize; + f->Seek(offset); + EXPECT_GT(seekGap + gapSize, offset); + EXPECT_LT(seekGap + gapSize - offset, TFile::kMaxGapSize); + pbuf = buf.data(); + tobuf(pbuf, -static_cast(seekGap + gapSize - offset)); + EXPECT_FALSE(f->WriteBuffer(buf.data(), sizeof(Int_t))); + + testKey.WriteFile(1, f.get()); + + int step = 0; + for (const auto &k : f->WalkTKeys()) { + if (step == 3) { + EXPECT_EQ(ROOT::Detail::TKeyMapNode::kGap, k.fType); + EXPECT_EQ(k.fAddr + k.fLen, seekGap + gapSize); + step = 4; + } else if (step == 2) { + EXPECT_EQ(ROOT::Detail::TKeyMapNode::kGap, k.fType); + EXPECT_LT(k.fLen, TFile::kMaxGapSize); + step = 3; + } else if (step == 1) { + EXPECT_EQ(ROOT::Detail::TKeyMapNode::kGap, k.fType); + EXPECT_EQ(sizeof(Int_t), k.fLen); + step = 2; + } else if (k.fAddr == static_cast(seekGap)) { + EXPECT_EQ(ROOT::Detail::TKeyMapNode::kKey, k.fType); + EXPECT_EQ(k.fLen, testKey.GetNbytes()); + step = 1; + } + } + EXPECT_EQ(4, step); + + f->Write(); + f->Close(); +} + TEST(TFile, KeySizeLimit) { // The following tests run out of memory on 32bit platforms diff --git a/tree/ntuple/src/RMiniFile.cxx b/tree/ntuple/src/RMiniFile.cxx index 35f28cb666b71..9d07c7112ad22 100644 --- a/tree/ntuple/src/RMiniFile.cxx +++ b/tree/ntuple/src/RMiniFile.cxx @@ -618,6 +618,8 @@ struct RTFileControlBlock { /// on some platforms. class RKeyBlob : public TKey { public: + using TKey::GapHeaderBuf_t; + RKeyBlob() = default; explicit RKeyBlob(TFile *file) : TKey(file) @@ -634,7 +636,7 @@ class RKeyBlob : public TKey { *seekKey = fSeekKey; } - bool WasAllocatedInAFreeSlot() const { return fLeft > 0; } + using TKey::FillGapHeaderBuffers; ClassDefInlineOverride(RKeyBlob, 0) }; @@ -1173,11 +1175,10 @@ std::uint64_t ROOT::Internal::RNTupleFileWriter::ReserveBlobKey(T &caller, TFile caller.Write(localKeyBuffer, kBlobKeyLen, offsetKey); } - if (keyBlob.WasAllocatedInAFreeSlot()) { - // If the key was allocated in a free slot, the last 4 bytes of its buffer contain the new size - // of the remaining free slot and we need to write it to disk before the key gets destroyed at the end of the - // function. - caller.Write(keyBlob.GetBuffer() + nbytes, sizeof(Int_t), offsetKey + kBlobKeyLen + nbytes); + RKeyBlob::GapHeaderBuf_t buffers; + auto nGaps = keyBlob.FillGapHeaderBuffers(file, buffers); + for (unsigned int i = 0; i < nGaps; ++i) { + caller.Write(buffers[i].data(), sizeof(Int_t), offsetKey + kBlobKeyLen + nbytes + i * sizeof(Int_t)); } auto offsetData = offsetKey + kBlobKeyLen;