lister: Fix race between readdir and stat

Sometimes file::list_directory() returns entries without type set. In
thase case lister calls file_type() on the entry name to get it. In case
the call returns disengated type, the code assumes that some error
occurred and resolves into exception.

That's not correct. The file_type() method returns disengated type only
if the file being inspected is missing (i.e. on ENOENT errno). But this
can validly happen if a file is removed bettween readdir and stat. In
that case it's not "some error happened", but a enry should be just
skipped. In "some error happened", then file_type() would resolve into
exceptional future on its own.

Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>

Closes scylladb/scylladb#26595

test/boost/lister_test.cc

@@ -23,6 +23,7 @@
 
 #include "utils/assert.hh"
 #include "utils/lister.hh"
+#include "utils/error_injection.hh"
 
 class expected_exception : public std::exception {
 public:
@@ -186,3 +187,27 @@ SEASTAR_TEST_CASE(test_directory_lister_extra_get) {
 
     BOOST_REQUIRE_THROW(co_await dl.get(), seastar::broken_pipe_exception);
 }
+
+// regression test of #26314
+SEASTAR_TEST_CASE(test_directory_lister_refresh_type_race) {
+#ifdef SCYLLA_ENABLE_ERROR_INJECTION
+    utils::error_injection_parameters params;
+    params["unlink_file"] = "unlinkme";
+    utils::get_local_injector().enable("lister_refresh_type", false, std::move(params));
+    auto tmp = tmpdir();
+    co_await tests::touch_file(tmp.path() / "unlinkme");
+    co_await tests::touch_file(tmp.path() / "sentinel");
+    std::map<sstring, std::optional<directory_entry_type>> files;
+    co_await lister::scan_dir(tmp.path(), lister::dir_entry_types::full(), [&files] (fs::path dir, directory_entry de) {
+        BOOST_CHECK(!files.contains(de.name));
+        files[de.name] = de.type;
+        return make_ready_future<>();
+    });
+    BOOST_CHECK(!files.contains("unlinkme"));
+    BOOST_CHECK(files.contains("sentinel"));
+    BOOST_CHECK(files.at("sentinel").has_value());
+#else
+    fmt::print("Skipping test as it depends on error injection. Please run in mode where it's enabled (debug,dev).\n");
+    co_return;
+#endif
+}

utils/lister.cc

@@ -7,6 +7,7 @@
 #include "utils/assert.hh"
 #include "utils/lister.hh"
 #include "utils/checked-file-impl.hh"
+#include "utils/error_injection.hh"
 
 static seastar::logger llogger("lister");
 
@@ -20,7 +21,11 @@ lister::lister(file f, dir_entry_types type, walker_type walker, filter_type fil
         , _show_hidden(do_show_hidden) {}
 
 future<> lister::visit(directory_entry de) {
-    return guarantee_type(std::move(de)).then([this] (directory_entry de) {
+    return refresh_type(std::move(de)).then([this] (directory_entry de) {
+        // The entry was removed between readdir and stat, just skip it
+        if (!de.type.has_value()) {
+            return make_ready_future<>();
+        }
         // Hide all synthetic directories and hidden files if not requested to show them.
         if ((_expected_type && !_expected_type.contains(*(de.type))) || (!_show_hidden && de.name[0] == '.')) {
             return make_ready_future<>();
@@ -41,16 +46,19 @@ future<> lister::done() {
     });
 }
 
-future<directory_entry> lister::guarantee_type(directory_entry de) {
+future<directory_entry> lister::refresh_type(directory_entry de) {
+    if (utils::get_local_injector().enter("lister_refresh_type")) {
+        de.type.reset();
+        if (de.name == *utils::get_local_injector().inject_parameter<std::string_view>("lister_refresh_type", "unlink_file")) {
+            fs::remove((_dir / de.name).native());
+        }
+    }
+
     if (de.type) {
         return make_ready_future<directory_entry>(std::move(de));
     } else {
         auto f = file_type((_dir / de.name.c_str()).native(), follow_symlink::no);
-        return f.then([dir = _dir, de = std::move(de)] (std::optional<directory_entry_type> t) mutable {
-            // If some FS error occurs - return an exceptional future
-            if (!t) {
-                return make_exception_future<directory_entry>(std::runtime_error(fmt::format("Failed to get {} type.", (dir / de.name.c_str()).native())));
-            }
+        return f.then([de = std::move(de)] (std::optional<directory_entry_type> t) mutable {
             de.type = t;
             return make_ready_future<directory_entry>(std::move(de));
         });

utils/lister.hh

@@ -122,20 +122,22 @@ private:
     future<> visit(directory_entry de);
 
     /**
-     * Validates that the input parameter has its "type" optional field engaged.
+     * Try to get entry "type" if it's missing
      *
      * This helper method is called before further processing the @param de in order
-     * to ensure that its "type" field is engaged.
+     * to have its "type" field engaged.
      *
      * If it is engaged - returns the input value as is.
      * If "type"  isn't engaged - calls the file_type() for file represented by @param de and sets
-     * "type" field of @param de to the returned value and then returns @param de.
+     * "type" field of @param de to the returned value and then returns @param de. The "type" may
+     * still be disengaged after it, meaning that the corresponding file is now missing (because it
+     * was removed or renamed).
      *
      * @param de entry to check and return
      * @return a future that resolves with the @param de with the engaged de.type field or an
      * exceptional future with std::system_error exception if type of the file represented by @param de may not be retrieved.
      */
-    future<directory_entry> guarantee_type(directory_entry de);
+    future<directory_entry> refresh_type(directory_entry de);
 };
 
 class abstract_lister {