From 47a4690d1199338b6826038f2d14ee2fa1823511 Mon Sep 17 00:00:00 2001 From: Chris Burr Date: Sat, 24 Oct 2026 19:57:43 +0200 Subject: [PATCH 1/2] [io] Forget the cached blocks when reading them fails TFileCacheRead::ReadBufferExtNormal sorts the list of blocks to read, which marks it as sorted, before reading them all into fBuffer with TFile::ReadBuffers. When that read fails, e.g. on an XRootD socket timeout, only the request that triggered it reports an error: the following requests for blocks of the same list find them in the list and are served from fBuffer, which still holds the blocks of the previous list, or nothing at all. The baskets of the rest of the cluster are thus unzipped and streamed from unrelated data. Reading LHCb DSTs over XRootD, a single timeout is followed by "Inconsistency found in header (nin=0, nbuf=0)" errors and allocations of several GB from the bogus content. Reset the list of blocks when reading them fails, so that subsequent requests miss the cache and are read from the file. Assisted-by: claude-code:claude-opus-5-5 --- io/io/src/TFileCacheRead.cxx | 2 + tree/tree/test/CMakeLists.txt | 1 + tree/tree/test/TTreeCacheReadError.cxx | 123 +++++++++++++++++++++++++ 3 files changed, 126 insertions(+) create mode 100644 tree/tree/test/TTreeCacheReadError.cxx diff --git a/io/io/src/TFileCacheRead.cxx b/io/io/src/TFileCacheRead.cxx index ec859aa0d72e6..65deceaeabf65 100644 --- a/io/io/src/TFileCacheRead.cxx +++ b/io/io/src/TFileCacheRead.cxx @@ -460,6 +460,8 @@ Int_t TFileCacheRead::ReadBufferExtNormal(char *buf, Long64_t pos, Int_t len, In if (!fAsyncReading) { // Then we use the vectored read to read everything now if (fFile->ReadBuffers(fBuffer,fPos,fLen,fNb)) { + // fBuffer does not hold the blocks: forget them, so that they are read from the file instead + TFileCacheRead::Prefetch(0, 0); return -1; } fIsTransferred = kTRUE; diff --git a/tree/tree/test/CMakeLists.txt b/tree/tree/test/CMakeLists.txt index 4e903b06daafc..94ece5abc821c 100644 --- a/tree/tree/test/CMakeLists.txt +++ b/tree/tree/test/CMakeLists.txt @@ -38,6 +38,7 @@ ROOT_ADD_GTEST(entrylist_enter entrylist_enter.cxx LIBRARIES RIO Tree) ROOT_ADD_GTEST(entrylist_enterrange entrylist_enterrange.cxx LIBRARIES RIO Tree) ROOT_ADD_GTEST(friendinfo friendinfo.cxx LIBRARIES RIO Tree) ROOT_ADD_GTEST(testTTreeCompression TTreeCompression.cxx LIBRARIES RIO Tree MathCore) +ROOT_ADD_GTEST(testTTreeCacheReadError TTreeCacheReadError.cxx LIBRARIES RIO Tree) ROOT_ADD_GTEST(evolution evolution.cxx LIBRARIES RIO Tree) ROOT_GENERATE_DICTIONARY(EvolutionStruct ${CMAKE_CURRENT_SOURCE_DIR}/EvolutionStruct.hxx MODULE evolution diff --git a/tree/tree/test/TTreeCacheReadError.cxx b/tree/tree/test/TTreeCacheReadError.cxx new file mode 100644 index 0000000000000..7c6ff5729eafa --- /dev/null +++ b/tree/tree/test/TTreeCacheReadError.cxx @@ -0,0 +1,123 @@ +#include "ROOT/TestSupport.hxx" + +#include "TFile.h" +#include "TSystem.h" +#include "TTree.h" +#include "TTreeCacheUnzip.h" + +#include "gtest/gtest.h" + +#include + +namespace { + +/// A file whose `failAt`-th vectored read fails without filling the buffer, as a TNetXNGFile does on a socket timeout. +class FailingReadBuffersFile : public TFile { + int fCalls = 0; + int fFailAt; + +public: + FailingReadBuffersFile(const char *name, int failAt) : TFile(name), fFailAt(failAt) {} + + Bool_t ReadBuffers(char *buf, Long64_t *pos, Int_t *len, Int_t nbuf) override + { + if (++fCalls == fFailAt) + return kTRUE; + return TFile::ReadBuffers(buf, pos, len, nbuf); + } + + int GetCalls() const { return fCalls; } +}; + +class TTreeCacheReadError : public ::testing::Test { +protected: + static constexpr const char *kFileName = "ttreecache_readerror.root"; + static constexpr int kEntries = 20000; + // The size of a cache holding the baskets of a single cluster. + static constexpr int kCacheSize = 20000; + + static void SetUpTestSuite() + { + TFile f(kFileName, "RECREATE"); + TTree t("t", "t"); + int a; + std::vector v; + t.Branch("a", &a, 4000); + t.Branch("v", &v, 4000); + t.SetAutoFlush(2000); + for (int i = 0; i < kEntries; ++i) { + a = i; + v.assign(i % 7, i); + t.Fill(); + } + t.Write(); + } + + static void TearDownTestSuite() { gSystem->Unlink(kFileName); } + + /// Read all entries of `t`, checking their content, and return the number of entries that could not be read. + static int ReadAll(TTree &t) + { + int a = -1; + std::vector *v = nullptr; + t.SetBranchAddress("a", &a); + t.SetBranchAddress("v", &v); + int nErrors = 0; + for (int i = 0; i < kEntries; ++i) { + if (t.GetEntry(i) <= 0) { + ++nErrors; + continue; + } + EXPECT_EQ(a, i); + EXPECT_NE(v, nullptr); + if (!v) + continue; + EXPECT_EQ(v->size(), static_cast(i % 7)); + for (int x : *v) + EXPECT_EQ(x, i); + } + return nErrors; + } +}; + +/// Use a TTreeCacheUnzip for the tree while in scope. +class UnzipCacheRAII { +public: + UnzipCacheRAII(TTree &t, Long64_t cacheSize) + { + TTreeCacheUnzip::SetParallelUnzip(TTreeCacheUnzip::kEnable); + new TTreeCacheUnzip(&t, cacheSize); + } + ~UnzipCacheRAII() { TTreeCacheUnzip::SetParallelUnzip(TTreeCacheUnzip::kDisable); } +}; + +} // namespace + +// A failed read of the baskets of a cluster must not leave the cache serving the other baskets of that cluster from a +// buffer that was never filled. +TEST_F(TTreeCacheReadError, FailedReadBuffers) +{ + // The first vectored read is that of the cache in learning phase, the third one that of the second cluster. + FailingReadBuffersFile f(kFileName, /*failAt=*/3); + auto t = f.Get("t"); + ASSERT_NE(t, nullptr); + t->SetCacheSize(kCacheSize); + + ROOT::TestSupport::CheckDiagsRAII diags; + diags.requiredDiag(kError, "TBranch::GetBasket", "badread=1", /*matchFullMessage=*/false); + // Only the entry whose read failed is lost; the other baskets of its cluster are read from the file. + EXPECT_EQ(ReadAll(*t), 1); + EXPECT_GT(f.GetCalls(), 3); +} + +// The same for TTreeCacheUnzip, where the read that fails is retried by TBasket. +TEST_F(TTreeCacheReadError, FailedReadBuffersUnzip) +{ + FailingReadBuffersFile f(kFileName, /*failAt=*/3); + auto t = f.Get("t"); + ASSERT_NE(t, nullptr); + UnzipCacheRAII unzip(*t, kCacheSize); + + EXPECT_EQ(ReadAll(*t), 0); + EXPECT_GT(f.GetCalls(), 3); +} From b954eace34ab97c0650a4d986ef10db91acdca44 Mon Sep 17 00:00:00 2001 From: Chris Burr Date: Sat, 24 Oct 2026 20:29:06 +0200 Subject: [PATCH 2/2] [tree] Do not unzip stale data after a read error TTreeCacheUnzip::GetUnzipBuffer reads the requested basket from the file only when TTreeCache::ReadBufferExt reports a miss. When that call itself fails to read the blocks of the cache, it returns -1, and the basket is unzipped from whatever fCompBuffer holds: the previous basket, or uninitialised memory. This happens when learning prefill has filled the cache before the first basket is requested. Treat a failure as a miss, so that the basket is read from the file. Assisted-by: claude-code:claude-opus-5-5 --- tree/tree/src/TTreeCacheUnzip.cxx | 3 ++- tree/tree/test/TTreeCacheReadError.cxx | 14 ++++++++++++++ 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/tree/tree/src/TTreeCacheUnzip.cxx b/tree/tree/src/TTreeCacheUnzip.cxx index d3783c58d86dd..0d621ea49fe42 100644 --- a/tree/tree/src/TTreeCacheUnzip.cxx +++ b/tree/tree/src/TTreeCacheUnzip.cxx @@ -787,7 +787,8 @@ Int_t TTreeCacheUnzip::GetUnzipBuffer(char **buf, Long64_t pos, Int_t len, bool } res = 0; - if (!ReadBufferExt(fCompBuffer, pos, len, loc)) { + // Read the buffer from the file unless it was found in the cache, including when reading the cache failed + if (ReadBufferExt(fCompBuffer, pos, len, loc) != 1) { // Cache is invalidated and we need to wait for all unzipping tasks to be finished before fill new baskets in cache. #ifdef R__USE_IMT if(ROOT::IsImplicitMTEnabled() && fUnzipTaskGroup) { diff --git a/tree/tree/test/TTreeCacheReadError.cxx b/tree/tree/test/TTreeCacheReadError.cxx index 7c6ff5729eafa..fc872ec23b84a 100644 --- a/tree/tree/test/TTreeCacheReadError.cxx +++ b/tree/tree/test/TTreeCacheReadError.cxx @@ -121,3 +121,17 @@ TEST_F(TTreeCacheReadError, FailedReadBuffersUnzip) EXPECT_EQ(ReadAll(*t), 0); EXPECT_GT(f.GetCalls(), 3); } + +// When prefilling the cache during learning, the first basket is requested from TTreeCacheUnzip before its blocks are +// read; a failure to read them must not unzip whatever its buffer for compressed data holds. +TEST_F(TTreeCacheReadError, FailedReadBuffersUnzipLearnPrefill) +{ + FailingReadBuffersFile f(kFileName, /*failAt=*/1); + auto t = f.Get("t"); + ASSERT_NE(t, nullptr); + UnzipCacheRAII unzip(*t, kCacheSize); + t->GetReadCache(&f)->SetLearnPrefill(TTreeCache::kAllBranches); + + EXPECT_EQ(ReadAll(*t), 0); + EXPECT_GT(f.GetCalls(), 1); +}