make database deletion more deterministic

arangod/MMFiles/MMFilesEngine.cpp

@@ -313,7 +313,7 @@ void MMFilesEngine::getDatabases(arangodb::velocypack::Builder& result) {
     if (!idSlice.isString() ||
         id != static_cast<TRI_voc_tick_t>(basics::StringUtils::uint64(idSlice.copyString()))) {
       LOG(ERR) << "database directory '" << directory
-               << "' does not contain a valid parameters file";
+               << "' does not contain a valid parameters file. database id is not a string";
       THROW_ARANGO_EXCEPTION(TRI_ERROR_ARANGO_ILLEGAL_PARAMETER_FILE);
     }
     
@@ -1237,7 +1237,14 @@ TRI_vocbase_t* MMFilesEngine::openExistingDatabase(TRI_voc_tick_t id, std::strin
       
 /// @brief physically erases the database directory
 int MMFilesEngine::dropDatabaseDirectory(std::string const& path) {
-  return TRI_RemoveDirectory(path.c_str());
+  // first create a .tmp file in the directory that will help us recover when we crash
+  // before the directory deletion is completed
+  std::string const tmpfile(
+      arangodb::basics::FileUtils::buildFilename(path, ".tmp"));
+  // ignore errors from writing this file...
+  TRI_WriteFile(tmpfile.c_str(), "", 0);
+
+  return TRI_RemoveDirectoryDeterministic(path.c_str());
 }
 
 /// @brief iterate over a set of datafiles, identified by filenames

js/client/modules/@arangodb/testing.js

@@ -3250,6 +3250,7 @@ const recoveryTests = [
   'empty-datafiles',
   'flush-drop-database-and-fail',
   'drop-database-flush-and-fail',
+  'drop-database-only-tmp',
   'create-databases',
   'recreate-databases',
   'drop-databases',

js/server/tests/recovery/drop-database-only-tmp.js

@@ -0,0 +1,90 @@
+/* jshint globalstrict:false, strict:false, unused: false */
+/* global fail, assertEqual */
+// //////////////////////////////////////////////////////////////////////////////
+// / @brief tests for transactions
+// /
+// / @file
+// /
+// / DISCLAIMER
+// /
+// / Copyright 2010-2012 triagens GmbH, Cologne, Germany
+// /
+// / Licensed under the Apache License, Version 2.0 (the "License")
+// / you may not use this file except in compliance with the License.
+// / You may obtain a copy of the License at
+// /
+// /     http://www.apache.org/licenses/LICENSE-2.0
+// /
+// / Unless required by applicable law or agreed to in writing, software
+// / distributed under the License is distributed on an "AS IS" BASIS,
+// / WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// / See the License for the specific language governing permissions and
+// / limitations under the License.
+// /
+// / Copyright holder is triAGENS GmbH, Cologne, Germany
+// /
+// / @author Jan Steemann
+// / @author Copyright 2013, triAGENS GmbH, Cologne, Germany
+// //////////////////////////////////////////////////////////////////////////////
+
+var db = require('@arangodb').db;
+var internal = require('internal');
+var fs = require('fs');
+var jsunity = require('jsunity');
+
+function runSetup () {
+  'use strict';
+  internal.debugClearFailAt();
+
+  var path = fs.join(fs.join(db._path(), ".."), "database-999999999");
+  fs.makeDirectory(path);
+
+  var data = { deleted: true, name: "UnitTestsRecovery", id: "999999999" };
+  fs.writeFile(fs.join(path, "parameter.json"), JSON.stringify(data));
+  fs.writeFile(fs.join(path, ".tmp"), "");
+
+  internal.debugSegfault('crashing server');
+}
+
+// //////////////////////////////////////////////////////////////////////////////
+// / @brief test suite
+// //////////////////////////////////////////////////////////////////////////////
+
+function recoverySuite () {
+  'use strict';
+  jsunity.jsUnity.attachAssertions();
+
+  return {
+    setUp: function () {},
+    tearDown: function () {},
+
+    // //////////////////////////////////////////////////////////////////////////////
+    // / @brief test whether we the data are correct after restart
+    // //////////////////////////////////////////////////////////////////////////////
+
+    testDropDatabaseOnlyTmp: function () {
+      try {
+        db._useDatabase('UnitTestsRecovery');
+        fail();
+      } catch (err) {
+        assertEqual(internal.errors.ERROR_ARANGO_DATABASE_NOT_FOUND.code, err.errorNum);
+      }
+    }
+
+  };
+}
+
+// //////////////////////////////////////////////////////////////////////////////
+// / @brief executes the test suite
+// //////////////////////////////////////////////////////////////////////////////
+
+function main (argv) {
+  'use strict';
+  if (argv[1] === 'setup') {
+    runSetup();
+    return 0;
+  } else {
+    jsunity.run(recoverySuite);
+    return jsunity.done().status ? 0 : 1;
+  }
+}

lib/Basics/files.cpp

@@ -575,19 +575,14 @@ int TRI_RemoveDirectory(char const* filename) {
     LOG(TRACE) << "removing symbolic link '" << filename << "'";
     return TRI_UnlinkFile(filename);
   } else if (TRI_IsDirectory(filename)) {
-    int res;
-
     LOG(TRACE) << "removing directory '" << filename << "'";
 
-    res = TRI_ERROR_NO_ERROR;
+    int res = TRI_ERROR_NO_ERROR;
     std::vector<std::string> files = TRI_FilesDirectory(filename);
     for (auto const& dir : files) {
-      char* full;
+      char* full = TRI_Concatenate2File(filename, dir.c_str());
-      int subres;
 
-      full = TRI_Concatenate2File(filename, dir.c_str());
+      int subres = TRI_RemoveDirectory(full);
-
-      subres = TRI_RemoveDirectory(full);
       TRI_FreeString(TRI_CORE_MEM_ZONE, full);
 
       if (subres != TRI_ERROR_NO_ERROR) {
@@ -608,21 +603,60 @@ int TRI_RemoveDirectory(char const* filename) {
     LOG(TRACE) << "attempt to remove non-existing file/directory '" << filename
                << "'";
 
+    // TODO: why do we actually return "no error" here?
     return TRI_ERROR_NO_ERROR;
   }
 }
 
+////////////////////////////////////////////////////////////////////////////////
+/// @brief removes a directory recursively
+////////////////////////////////////////////////////////////////////////////////
+
+int TRI_RemoveDirectoryDeterministic(char const* filename) {
+  std::vector<std::string> files = TRI_FullTreeDirectory(filename);
+  // start removing files from long names to short names
+  std::sort(files.begin(), files.end(), [](std::string const& lhs, std::string const& rhs) -> bool {
+    if (lhs.size() == rhs.size()) {
+      // equal lengths. now compare the file/directory names
+      return lhs < rhs;
+    }
+    return lhs.size() > rhs.size();
+  });
+
+  // LOG(TRACE) << "removing files in directory '" << filename << "' in this order: " << files;
+
+  int res = TRI_ERROR_NO_ERROR;
+
+  for (auto const& it : files) {
+    if (it.empty()) {
+      // TRI_FullTreeDirectory returns "" as its first member
+      continue;
+    }
+
+    char* full = TRI_Concatenate2File(filename, it.c_str());
+    int subres = TRI_RemoveDirectory(full);
+    TRI_FreeString(TRI_CORE_MEM_ZONE, full);
+
+    if (subres != TRI_ERROR_NO_ERROR) {
+      res = subres;
+    }
+  }
+
+  int subres = TRI_RemoveDirectory(filename);
+  if (subres != TRI_ERROR_NO_ERROR) {
+    res = subres;
+  }
+  
+  return res;
+}
+
 ////////////////////////////////////////////////////////////////////////////////
 /// @brief extracts the dirname
 ////////////////////////////////////////////////////////////////////////////////
 
 char* TRI_Dirname(char const* path) {
-  size_t n;
+  size_t n = strlen(path);
-  size_t m;
+  size_t m = 0;
-  char const* p;
-
-  n = strlen(path);
-  m = 0;
 
   if (1 < n) {
     if (path[n - 1] == TRI_DIR_SEPARATOR_CHAR) {
@@ -640,6 +674,7 @@ char* TRI_Dirname(char const* path) {
     return TRI_DuplicateString("..");
   }
 
+  char const* p;
   for (p = path + (n - m - 1); path < p; --p) {
     if (*p == TRI_DIR_SEPARATOR_CHAR) {
       break;
@@ -782,28 +817,29 @@ std::vector<std::string> TRI_FilesDirectory(char const* path) {
 std::vector<std::string> TRI_FilesDirectory(char const* path) {
   std::vector<std::string> result;
 
-  DIR* d;
+  DIR* d = opendir(path);
-  struct dirent* de;
 
-  d = opendir(path);
+  if (d == nullptr) {
-
-  if (d == 0) {
     return result;
   }
 
-  de = readdir(d);
+  struct dirent* de = readdir(d);
 
-  while (de != 0) {
+  try {
+    while (de != nullptr) {
       if (strcmp(de->d_name, ".") != 0 && strcmp(de->d_name, "..") != 0) {
-      result.emplace_back(de->d_name);
+        // may throw
+        result.emplace_back(std::string(de->d_name));
       }
 
       de = readdir(d);
     }
-
     closedir(d);
-
     return result;
+  } catch (...) {
+    closedir(d);
+    throw;
+  }
 }
 
 #endif

lib/Basics/files.h

@@ -101,11 +101,18 @@ int TRI_CreateDirectory(char const* path, long& systemError,
 int TRI_RemoveEmptyDirectory(char const* filename);
 
 ////////////////////////////////////////////////////////////////////////////////
-/// @brief removes a directory recursively
+/// @brief removes a directory recursively, using file order provided by
+/// the file system
 ////////////////////////////////////////////////////////////////////////////////
 
 int TRI_RemoveDirectory(char const* filename);
 
+////////////////////////////////////////////////////////////////////////////////
+/// @brief removes a directory recursively, using a deterministic order of files
+////////////////////////////////////////////////////////////////////////////////
+
+int TRI_RemoveDirectoryDeterministic(char const* filename);
+
 ////////////////////////////////////////////////////////////////////////////////
 /// @brief extracts the dirname
 ////////////////////////////////////////////////////////////////////////////////