From c29ba8438bbf0068ab00b892419caab6d5b54f08 Mon Sep 17 00:00:00 2001 From: Oliver Schulz Date: Fri, 30 Oct 2026 18:08:36 +0100 Subject: [PATCH 1/4] Tests: suppress a TSan report between XrdCl's own threads GitHub's ubuntu-24.04 thread job fails now and then on a report that XrdCl's poller thread frees a response buffer that a job thread copied before in XRootDMsgHandler::ParseResponse. Both stacks are inside the uninstrumented library, without eosmirror's code in them. Assisted-By: generative AI --- tests/tsan.supp | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/tests/tsan.supp b/tests/tsan.supp index 52c6ee0..8f242dd 100644 --- a/tests/tsan.supp +++ b/tests/tsan.supp @@ -7,3 +7,8 @@ race:XrdSysTimer::TimeZone # uninstrumented library: TSan takes the handler's destruction by the last # owner, an XrdCl thread, for a race with the File's earlier calls. race:XrdCl::FileStateHandler +# Now and then, with Ubuntu 24.04's XRootD 5.6, TSan reports XrdCl's poller +# thread freeing a response buffer that a job thread copied before in +# XRootDMsgHandler::ParseResponse: a race, if any, between XrdCl's own +# threads inside the uninstrumented library. +race:XrdCl::XRootDMsgHandler::ParseResponse From 4c44aaa7db20dc3142f997cf258287d5ba75967d Mon Sep 17 00:00:00 2001 From: Oliver Schulz Date: Fri, 30 Oct 2026 18:08:36 +0100 Subject: [PATCH 2/4] EOS tests: the find-limits test skips for a caller that is nobody Such a caller owns the directory that the test makes unreadable for nobody, so the test failed where it should skip. The comment and the docs said imprecisely which callers become nobody when they ask for it: every caller but a sudoer does, and a sudoer stays one. Assisted-By: generative AI --- README.md | 4 ++-- docs/architecture.md | 4 ++-- tests/integration/test_eos.cc | 10 ++++++---- 3 files changed, 10 insertions(+), 8 deletions(-) diff --git a/README.md b/README.md index 94aaa80..0a8a761 100644 --- a/README.md +++ b/README.md @@ -300,8 +300,8 @@ a user whom EOS does not take for root, they leave out owners, and the tests that need root on EOS (changing a file's layout) say that they are skipped; with `EOSMIRROR_EOS_AS_ROOT=1`, as `scripts/integration-test.sh` sets it, they fail instead. The test of listings with find limits needs -the limits for nobody that `instance.sh` sets, and a caller that EOS lets -become nobody. +the limits for nobody that `instance.sh` sets, and a caller that is neither +nobody nor a sudoer. Two longer tests (about a minute each) run only when asked for, as in CI's on-demand job: `ctest --test-dir build -C long -L long`. One kills diff --git a/docs/architecture.md b/docs/architecture.md index 87bd747..6fa7986 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -463,8 +463,8 @@ of it, not an independent model of EOS. (read back with stripes dropped, since a normal read cannot tell), the EOS-specific failure paths, the self-test, a sudoer's identity. The EOS tests run as root on EOS, and check that they do; on another instance as - another user, they leave out owners and skip, with a message, what needs - root there. + another user, they leave out owners and skip, with a message, what that + user cannot test there. - Long (`ctest -C long -L long`): the randomized differential test (random trees, random changes, injected faults; the journal must record what was left behind before a clean run repairs it, and the target must then equal diff --git a/tests/integration/test_eos.cc b/tests/integration/test_eos.cc index d3053e3..3ed8b06 100644 --- a/tests/integration/test_eos.cc +++ b/tests/integration/test_eos.cc @@ -178,8 +178,9 @@ bool as_root(EosEndpoint& ep) { } // Whether asking for nobody's identity (eos.ruid, eos.rgid) makes this -// caller nobody, and no sudoer, as find limits need. EOS grants it to a -// sudoer, and to another caller that holds nobody's ids (common/Mapping.cc). +// caller nobody, and no sudoer, as find limits need. EOS makes every caller +// but a sudoer nobody then: one that does not hold the ids it asks for gets +// nobody's. A sudoer gets them, but stays a sudoer (common/Mapping.cc). bool becomes_nobody(const std::string& url) { auto ep = EosEndpoint::create(url + "?eos.ruid=65534&eos.rgid=65534"); REQUIRE(ep.ok()); @@ -1046,8 +1047,9 @@ TEST_CASE("eos endpoint: listings for an identity with find limits") { if (!base_url()) return; RemoteDir dir("limits"); EosEndpoint& ep = *dir.endpoint; - if (!becomes_nobody(dir.url)) { - MESSAGE("this caller cannot become nobody: listings with find limits not tested"); + // A caller that is nobody would own the directory that nobody may not read. + if (eos_id(ep).uid == 65534 || !becomes_nobody(dir.url)) { + MESSAGE("the caller is nobody, or a sudoer as nobody: listings with find limits not tested"); return; } // The instance limits nobody to 20 files and 10 directories per find, From 01e53db04e71fa2d5b731b25811c4643770e436e Mon Sep 17 00:00:00 2001 From: Oliver Schulz Date: Fri, 30 Oct 2026 18:08:36 +0100 Subject: [PATCH 3/4] EOS reads: an append that the replica read does not show On an instance with a file's replicas on several storage nodes, the replica read may not show an append made during the read (3 of 3 runs on such an instance). The copy is then the listed version, and the next run copies the file again. docs/design.md records this. The test accepts that outcome only for a file whose replicas fileinfo puts on more than one host, so that the single-node test instance still catches a post-read check that misses changes. Assisted-By: generative AI --- docs/design.md | 32 +++++++++++++------------- src/eosmirror/eos_endpoint.cc | 7 +++--- tests/integration/test_eos.cc | 42 ++++++++++++++++++++++++++++++----- 3 files changed, 58 insertions(+), 23 deletions(-) diff --git a/docs/design.md b/docs/design.md index 8bbe042..791b90e 100644 --- a/docs/design.md +++ b/docs/design.md @@ -669,21 +669,23 @@ endpoint uses the MGM's commands, sent the way the `eos` client sends them read the storage node is asked for the replica's stat again (a request on the open file, which XrdCl sends to the storage node): another size or mtime means that the file changed while it was read, and the copy is - retried. That stat is of the replica opened: a file replaced during the - read is not seen there, its copy gets the old, listed mtime, and the next - run copies it again. A file with another size than listed has changed - since the listing: it is stated through the MGM before and after reading, - so that it is still copied in this run, with its current metadata. Those - stats must find the file opened under its name, known by its id from the - redirect (`mgm.id`, as for uploads): a file replaced once more since the - open has another id, which fails the copy as changed (it is retried). A - file rewritten with the same size after the listing shows up as a checksum - that differs from the listed one. A stat through the MGM then tells a - change (the copy fails as changed, after its retries, and the next run - copies it) from corrupt data (the MGM still has the listed size and - mtime). Where the listing has no checksum (a directory without - `sys.forced.checksum`), such a file is copied with the listed, older - mtime, and the next run copies it again. + retried. That stat is of the replica opened. It does not show a file + replaced during the read, and of a file with several replicas, it may not + show an append made during the read (seen with replicas on different + storage nodes). The copy is then of the listed version, with the listed + mtime, and the next run copies the file again. A file with another size + than listed has changed since the listing: it is stated through the MGM + before and after reading, so that it is still copied in this run, with its + current metadata. Those stats must find the file opened under its name, + known by its id from the redirect (`mgm.id`, as for uploads): a file + replaced once more since the open has another id, which fails the copy as + changed (it is retried). A file rewritten with the same size after the + listing shows up as a checksum that differs from the listed one. A stat + through the MGM then tells a change (the copy fails as changed, after its + retries, and the next run copies it) from corrupt data (the MGM still has + the listed size and mtime). Where the listing has no checksum (a directory + without `sys.forced.checksum`), such a file is copied with the listed, + older mtime, and the next run copies it again. - EOS's own hidden entries (atomic temporaries `.sys.a#.`, version directories `.sys.v#.`) are never treated as entries of the tree. An atomic temporary gets the `eos.mtime` of its open when EOS creates it, diff --git a/src/eosmirror/eos_endpoint.cc b/src/eosmirror/eos_endpoint.cc index f93850d..234e0c3 100644 --- a/src/eosmirror/eos_endpoint.cc +++ b/src/eosmirror/eos_endpoint.cc @@ -234,9 +234,10 @@ std::string format_timespec(const Timespec& t) { // replica has the listed size. The storage node's stat of the replica, which // comes with the open, has the file's size (the logical size of erasure-coded // files) but the replica's own mtime, in seconds (fst/XrdFstOfsFile.cc stat): -// a change during the copy shows in its stat after the read. A file whose -// size differs from the listing has changed since. It is stated through the -// MGM before and after reading, and each stat must find the opened file under +// a change to that replica during the copy shows in its stat after the read +// (see docs/design.md for the changes it does not show). A file whose size +// differs from the listing has changed since. It is stated through the MGM +// before and after reading, and each stat must find the opened file under // its name (a file replaced since has another id). class EosReader : public XrdReader { public: diff --git a/tests/integration/test_eos.cc b/tests/integration/test_eos.cc index 3ed8b06..f4dc705 100644 --- a/tests/integration/test_eos.cc +++ b/tests/integration/test_eos.cc @@ -20,6 +20,7 @@ #include #include #include +#include #include #include @@ -870,6 +871,26 @@ void append(const std::string& url, std::string_view data) { REQUIRE(file.Close().IsOK()); } +// The storage nodes that hold a file's replicas, from the hosts in the JSON +// of fileinfo. +std::set replica_hosts(EosEndpoint& ep, const RelPath& path) { + auto info = ep.proc("mgm.cmd=fileinfo&mgm.format=json&mgm.path=" + ep.absolute(path)); + REQUIRE(info.ok()); + const std::string& json = info.value().out; + INFO("fileinfo: ", json); + std::set hosts; + for (size_t at = json.find("\"host\""); at != std::string::npos; + at = json.find("\"host\"", at + 1)) { + size_t begin = json.find('"', json.find(':', at)); + REQUIRE(begin != std::string::npos); + size_t end = json.find('"', ++begin); + REQUIRE(end != std::string::npos); + hosts.insert(json.substr(begin, end - begin)); + } + REQUIRE_FALSE(hosts.empty()); + return hosts; +} + } // namespace TEST_CASE("EOS to FS: files that changed since the listing or while they are read") { @@ -925,10 +946,13 @@ TEST_CASE("EOS to FS: files that changed since the listing or while they are rea CHECK(tmp.read_file("dst/f") == "replaced with a longer content"); // Appended to while it is read: changed, and copied as it is now when the - // copy is retried with the same listed entry. + // copy is retried with the same listed entry. Of a file with replicas on + // several storage nodes, the one read may not show the append + // (docs/design.md): the copy is then of the listed version. std::string content = pattern(300000); put(ep, "g", content); old = listed(ep, "g"); + size_t nodes = replica_hosts(ep, "g").size(); CopyOptions appending = options; bool appended = false; appending.on_chunk = [&](uint64_t) { @@ -936,10 +960,18 @@ TEST_CASE("EOS to FS: files that changed since the listing or while they are rea }; auto grown = copy_file(ep, dst, "g", old, appending, pool, cancel); REQUIRE(appended); - REQUIRE_FALSE(grown.ok()); - INFO("error: ", grown.error().describe()); - CHECK(grown.error().kind == ErrorKind::Changed); - CHECK_FALSE(dst.stat("g").ok()); + if (grown.ok() && nodes > 1) { + MESSAGE("the replica read does not show the append: copied as listed"); + CHECK(tmp.read_file("dst/g") == content); + REQUIRE(dst.flush().ok()); + CHECK(must(dst.stat("g")).mtime == old.mtime); + } else { + INFO("storage nodes: ", nodes); + REQUIRE_FALSE(grown.ok()); + INFO("error: ", grown.error().describe()); + CHECK(grown.error().kind == ErrorKind::Changed); + CHECK_FALSE(dst.stat("g").ok()); + } auto retried = copy_file(ep, dst, "g", old, options, pool, cancel); REQUIRE(retried.ok()); CHECK(tmp.read_file("dst/g") == content + "more"); From 416f10044a789820a5e270c85c5e19622b51bb50 Mon Sep 17 00:00:00 2001 From: Oliver Schulz Date: Fri, 30 Oct 2026 18:26:05 +0100 Subject: [PATCH 4/4] Tests: the CMake comment lists the new TSan suppression The comment on the sanitizer settings names what tsan.supp suppresses, and missed the report in XrdCl's response parsing added before. Assisted-By: generative AI --- tests/CMakeLists.txt | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 57e6783..606116f 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -74,9 +74,9 @@ if(EOSMIRROR_WITH_XROOTD) # that may be where the compiler's sanitizer runtimes are found (a # compiler outside the system's, as in an LCG view). Leaks in XRootD's and # OpenSSL's global state are not ours (lsan.supp), nor are the races TSan - # sees in glibc's time zone setup and in XrdCl's file state handler - # (tsan.supp). The unit tests load the - # client library too. UBSan stops at its first finding, with a stack trace. + # sees in glibc's time zone setup, XrdCl's file state handler and XrdCl's + # response parsing (tsan.supp). The unit tests load the client library + # too. UBSan stops at its first finding, with a stack trace. if(EOSMIRROR_SANITIZER AND EOSMIRROR_XROOTD_LIBDIR) set(sanitized_tests unit unit_unprivileged integration integration_sudoer cli_smoke differential crash_consistency) set_tests_properties(${sanitized_tests}