EOS tests: follow-up to #5 (appends during reads, find limits as nobody, TSan)

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

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

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,

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:

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}

tests/integration/test_eos.cc

@@ -20,6 +20,7 @@
 #include <chrono>
 #include <cstdlib>
 #include <map>
+#include <set>
 #include <utility>
 #include <vector>
 
@@ -178,8 +179,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());
@@ -869,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<std::string> 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<std::string> 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") {
@@ -924,21 +946,32 @@ 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) {
     if (!std::exchange(appended, true)) append(dir.url + "/g", "more");
   };
   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");
@@ -1046,8 +1079,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,

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