From 2084cdf23772b9219ba4f3aa26d10dc7a274fd89 Mon Sep 17 00:00:00 2001
From: anand76 <anand76@devvm6332.prn0.facebook.com>
Date: Fri, 12 May 2023 22:39:39 -0700
Subject: [PATCH] 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: https://github.com/facebook/rocksdb/pull/11423

Test Plan: Add a unit test

Reviewed By: akankshamahajan15

Differential Revision: D45540454

Pulled By: anand1976

fbshipit-source-id: 47facdc30d8cc5667036312d04b21d3fc253c92e
---
 HISTORY.md                |  1 +
 db/db_impl/db_impl.cc     |  9 +++++++++
 db/db_options_test.cc     | 34 ++++++++++++++++++++++++++++++++++
 options/options_parser.cc |  2 ++
 4 files changed, 46 insertions(+)

diff --git a/HISTORY.md b/HISTORY.md
index 408a4ce818b..6fa5dc855cc 100644
--- a/HISTORY.md
+++ b/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).
diff --git a/db/db_impl/db_impl.cc b/db/db_impl/db_impl.cc
index bf4f9232a08..07aee535db5 100644
--- a/db/db_impl/db_impl.cc
+++ b/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();
diff --git a/db/db_options_test.cc b/db/db_options_test.cc
index 777e2fe2216..729afdf3db9 100644
--- a/db/db_options_test.cc
+++ b/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) {
diff --git a/options/options_parser.cc b/options/options_parser.cc
index d423f76ff97..a8c855d6e22 100644
--- a/options/options_parser.cc
+++ b/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);