Delete temp OPTIONS file on failure to write it (#11423)

Summary:
When the DB is opened, RocksDB creates a temp OPTIONS file, writes the current options to it, and renames it. In case of a failure, the temp file is left behind, and is not deleted by PurgeObsoleteFiles(). Fix this by explicitly deleting the temp file if writing to it or renaming it fails.

Pull Request resolved: #11423

Test Plan: Add a unit test

Reviewed By: akankshamahajan15

Differential Revision: D45540454

Pulled By: anand1976

fbshipit-source-id: 47facdc30d8cc5667036312d04b21d3fc253c92e

HISTORY.md

@@ -15,6 +15,7 @@
 
 ### Bug Fixes
 * Delete an empty WAL file on DB open if the log number is less than the min log number to keep
+* Delete temp OPTIONS file on DB open if there is a failure to write it out or rename it
 
 ### Performance Improvements
 * Improved the I/O efficiency of prefetching SST metadata by recording more information in the DB manifest. Opening files written with previous versions will still rely on heuristics for how much to prefetch (#11406).

db/db_impl/db_impl.cc

@@ -4913,6 +4913,15 @@ Status DBImpl::WriteOptionsFile(bool need_mutex_lock,
   if (s.ok()) {
     s = RenameTempFileToOptionsFile(file_name);
   }
+
+  if (!s.ok() && GetEnv()->FileExists(file_name).ok()) {
+    if (!GetEnv()->DeleteFile(file_name).ok()) {
+      ROCKS_LOG_WARN(immutable_db_options_.info_log,
+                     "Unable to delete temp options file %s",
+                     file_name.c_str());
+    }
+  }
+
   // restore lock
   if (!need_mutex_lock) {
     mutex_.Lock();

db/db_options_test.cc

@@ -22,6 +22,7 @@
 #include "test_util/sync_point.h"
 #include "test_util/testutil.h"
 #include "util/random.h"
+#include "utilities/fault_injection_fs.h"
 
 namespace ROCKSDB_NAMESPACE {
 
@@ -1287,6 +1288,39 @@ TEST_F(DBOptionsTest, FIFOTemperatureAgeThresholdValidation) {
                           "supported when num_levels = 1."));
 }
 
+TEST_F(DBOptionsTest, TempOptionsFailTest) {
+  std::shared_ptr<FaultInjectionTestFS> fs;
+  std::unique_ptr<Env> env;
+
+  fs.reset(new FaultInjectionTestFS(env_->GetFileSystem()));
+  env = NewCompositeEnv(fs);
+  Options options = CurrentOptions();
+  options.env = env.get();
+
+  SyncPoint::GetInstance()->SetCallBack(
+      "PersistRocksDBOptions:create",
+      [&](void* /*arg*/) { fs->SetFilesystemActive(false); });
+  SyncPoint::GetInstance()->SetCallBack(
+      "PersistRocksDBOptions:written",
+      [&](void* /*arg*/) { fs->SetFilesystemActive(true); });
+
+  SyncPoint::GetInstance()->EnableProcessing();
+  TryReopen(options);
+  SyncPoint::GetInstance()->DisableProcessing();
+
+  std::vector<std::string> filenames;
+  ASSERT_OK(env_->GetChildren(dbname_, &filenames));
+  uint64_t number;
+  FileType type;
+  bool found_temp_file = false;
+  for (size_t i = 0; i < filenames.size(); i++) {
+    if (ParseFileName(filenames[i], &number, &type) && type == kTempFile) {
+      found_temp_file = true;
+    }
+  }
+  ASSERT_FALSE(found_temp_file);
+}
+
 }  // namespace ROCKSDB_NAMESPACE
 
 int main(int argc, char** argv) {

options/options_parser.cc

@@ -75,6 +75,7 @@ Status PersistRocksDBOptions(const ConfigOptions& config_options_in,
   std::unique_ptr<WritableFileWriter> writable;
   writable.reset(new WritableFileWriter(std::move(wf), file_name, EnvOptions(),
                                         nullptr /* statistics */));
+  TEST_SYNC_POINT("PersistRocksDBOptions:create");
 
   std::string options_file_content;
 
@@ -135,6 +136,7 @@ Status PersistRocksDBOptions(const ConfigOptions& config_options_in,
   if (s.ok()) {
     s = writable->Close();
   }
+  TEST_SYNC_POINT("PersistRocksDBOptions:written");
   if (s.ok()) {
     return RocksDBOptionsParser::VerifyRocksDBOptionsFromFile(
         config_options, db_opt, cf_names, cf_opts, file_name, fs);