Sync the session directory after committing the renames

Syncing the files alone does not make the new names survive a crash.

src/session/download_storer.cc

@@ -17,6 +17,13 @@
 #include "core/download.h"
 #include "utils/directory.h"
 
+// O_DIRECTORY makes the open refuse anything that is not a directory. A
+// platform that does not define it still gets the read-only directory
+// handle fsync needs, so fall back to no extra flag.
+#ifndef O_DIRECTORY
+#define O_DIRECTORY 0
+#endif
+
 namespace session {
 
 DownloadStorer::DownloadStorer(core::Download* download)
@@ -166,6 +173,26 @@ save_stream(const std::string& path, bool use_fsyncdisk, const std::stringstream
     throw torrent::storage_error("failed to close file descriptor : " + path);
 }
 
+void
+sync_directory_of(const std::string& path) {
+  auto separator = path.rfind('/');
+  auto directory = separator == std::string::npos ? std::string(".") :
+                   separator == 0                 ? std::string("/") : path.substr(0, separator);
+
+  int fd = ::open(directory.c_str(), O_RDONLY | O_DIRECTORY);
+
+  if (fd < 0)
+    throw torrent::storage_error("failed to open session directory for sync : " + directory);
+
+  if (::fsync(fd) == -1) {
+    ::close(fd);
+    throw torrent::storage_error("failed to sync session directory : " + directory);
+  }
+
+  if (::close(fd) == -1)
+    throw torrent::storage_error("failed to close session directory : " + directory);
+}
+
 } // namespace anonymous
 
 void
@@ -193,6 +220,11 @@ DownloadStorer::save_and_move_streams(const std::string& path, bool use_fsyncdis
 
   if (::rename((rtorrent_path + ".new").c_str(), rtorrent_path.c_str()) == -1)
     throw torrent::storage_error("failed to rename rtorrent resume file : " + rtorrent_path);
+
+  // Syncing the files themselves does not persist the renames; the directory
+  // holding them has to be synced for the new names to survive a crash.
+  if (use_fsyncdisk)
+    sync_directory_of(path);
 }
 
 utils::Directory

src/session/session_manager.cc

@@ -75,7 +75,6 @@ SessionManager::save_resume_download(core::Download* download) {
   {
     std::unique_lock<std::mutex> lock(m_pending_builds_mutex);
 
-    // TODO: This is under the wrong lock.
     if (!m_active)
       throw torrent::internal_error("SessionManager::save_resume_download() called while not active.");
 

src/session/session_manager.h

@@ -1,6 +1,7 @@
 #ifndef RTORRENT_SESSION_SESSION_MANAGER_H
 #define RTORRENT_SESSION_SESSION_MANAGER_H
 
+#include <atomic>
 #include <condition_variable>
 #include <deque>
 #include <future>
@@ -99,9 +100,13 @@ private:
   bool                         m_use_fsyncdisk{true};
   bool                         m_use_lock{true};
 
-  align_cacheline std::mutex   m_mutex;
+  align_cacheline
 
-  bool                         m_active{};
+  std::atomic<bool>            m_active{};
+
+  align_cacheline
+
+  std::mutex                   m_mutex;
 
   std::deque<SaveRequest>      m_save_requests;
   std::atomic<size_t>          m_save_request_counter{};

test/Makefile.am

@@ -69,6 +69,8 @@ rtorrent_Test_Src_SOURCES = $(rtorrent_Test_Common) \
 	src/test_command_string.h \
 	src/test_command_throttle.cc \
 	src/test_command_throttle.h \
+	src/test_session_commit.cc \
+	src/test_session_commit.h \
 	src/test_session_storer.cc \
 	src/test_session_storer.h \
 	src/test_setup.cc \

test/src/test_session_commit.cc

@@ -0,0 +1,94 @@
+#include "config.h"
+
+#include "test/src/test_session_commit.h"
+
+#include <cstdlib>
+#include <dirent.h>
+#include <fstream>
+#include <sstream>
+#include <sys/stat.h>
+#include <unistd.h>
+
+#include "session/download_storer.h"
+
+CPPUNIT_TEST_SUITE_REGISTRATION(TestSessionCommit);
+
+namespace {
+
+const char* entry_name = "0123456789ABCDEF0123456789ABCDEF01234567.torrent";
+
+std::string
+read_file(const std::string& path) {
+  std::ifstream     file(path.c_str());
+  std::stringstream buffer;
+
+  buffer << file.rdbuf();
+  return buffer.str();
+}
+
+void
+remove_directory(const std::string& path) {
+  DIR* d = ::opendir(path.c_str());
+
+  if (d == NULL)
+    return;
+
+  struct dirent* entry;
+
+  while ((entry = ::readdir(d)) != NULL) {
+    if (entry->d_name[0] == '.' && (entry->d_name[1] == '\0' || (entry->d_name[1] == '.' && entry->d_name[2] == '\0')))
+      continue;
+
+    ::unlink((path + "/" + entry->d_name).c_str());
+  }
+
+  ::closedir(d);
+  ::rmdir(path.c_str());
+}
+
+} // namespace
+
+void
+TestSessionCommit::setUp() {
+  test_fixture::setUp();
+
+  char temp_dir[] = "/tmp/rtorrent_test_commit_XXXXXX";
+
+  CPPUNIT_ASSERT(mkdtemp(temp_dir) != nullptr);
+  m_session_dir = temp_dir;
+}
+
+void
+TestSessionCommit::tearDown() {
+  remove_directory(m_session_dir);
+
+  test_fixture::tearDown();
+}
+
+void
+TestSessionCommit::commit_and_verify(bool use_fsyncdisk) {
+  auto path = m_session_dir + "/" + entry_name;
+
+  std::stringstream torrent_stream("torrent-data");
+  std::stringstream rtorrent_stream("rtorrent-data");
+  std::stringstream libtorrent_stream("libtorrent-data");
+
+  session::DownloadStorer::save_and_move_streams(path, use_fsyncdisk, &torrent_stream, &rtorrent_stream, &libtorrent_stream);
+
+  CPPUNIT_ASSERT_EQUAL(std::string("torrent-data"), read_file(path));
+  CPPUNIT_ASSERT_EQUAL(std::string("rtorrent-data"), read_file(path + ".rtorrent"));
+  CPPUNIT_ASSERT_EQUAL(std::string("libtorrent-data"), read_file(path + ".libtorrent_resume"));
+
+  struct stat st;
+  CPPUNIT_ASSERT(::stat((path + ".new").c_str(), &st) == -1);
+}
+
+void
+TestSessionCommit::test_commit_publishes_all_three_files() {
+  commit_and_verify(false);
+}
+
+void
+TestSessionCommit::test_commit_with_fsync_publishes_all_three_files() {
+  commit_and_verify(true);
+}

test/src/test_session_commit.h

@@ -0,0 +1,24 @@
+#include "test/helpers/test_fixture.h"
+
+#include <string>
+
+class TestSessionCommit : public test_fixture {
+  CPPUNIT_TEST_SUITE(TestSessionCommit);
+
+  CPPUNIT_TEST(test_commit_publishes_all_three_files);
+  CPPUNIT_TEST(test_commit_with_fsync_publishes_all_three_files);
+
+  CPPUNIT_TEST_SUITE_END();
+
+public:
+  void setUp();
+  void tearDown();
+
+  void test_commit_publishes_all_three_files();
+  void test_commit_with_fsync_publishes_all_three_files();
+
+private:
+  void commit_and_verify(bool use_fsyncdisk);
+
+  std::string m_session_dir;
+};