Fix stale no-reopen tracking in fault injection FS (#14822)

Summary:
PR https://github.com/facebook/rocksdb/issues/14585 added FileOpenContract enforcement to FaultInjectionTestFS. The no-reopen-for-write check recorded contracts by path but did not clear a stale contract after the file was deleted. A later SST create that reused the same path could be rejected as a forbidden reopen, causing DBWALTest.WALWithChecksumHandoff to fail with "NewWritableFile violates no-reopen-for-write contract". If the ASSERT exited the test early, the local Env stack object could also be destroyed before DB fixture teardown closed the DB, producing the follow-on TSAN heap-use-after-free.

When opening a file for write, drop stale no-reopen tracking if the target file no longer exists, while still rejecting writes through reopened handles. Also close the DB before the WAL test's local Env unwinds on assertion failure.

A separate TSAN report in DBBlobBasicIOErrorTest.GetEntityMergeWithBlobBaseIOError exposed unsynchronized access to FaultInjectionTestEnv::error_: SetFilesystemActive() updated the stored error under mutex_, while background purge work read it through GetError() without that mutex. Copy injected errors under the same mutex and read the no-space state in GetFreeSpace() while protected. Apply the same synchronization to FaultInjectionTestFS.

Bonus fix: Fix another TSAN data race in FaultInjectionTestEnv GetError not synchronized under mutex_

Pull Request resolved: https://github.com/facebook/rocksdb/pull/14822

Test Plan: CI

Reviewed By: mszeszko-meta

Differential Revision: D107689662

Pulled By: xingbowang

fbshipit-source-id: 6d4f8fdc8b898d3ddcad7816e385f3b7f20c4727

db/db_wal_test.cc

@@ -860,6 +860,8 @@ TEST_F(DBWALTest, WALWithChecksumHandoff) {
   std::shared_ptr<FaultInjectionTestFS> fault_fs(
       new FaultInjectionTestFS(FileSystem::Default()));
   std::unique_ptr<Env> fault_fs_env(NewCompositeEnv(fault_fs));
+  // Close the DB before the local Env is destroyed if an ASSERT exits early.
+  Defer close_db_on_exit([this]() { Close(); });
   do {
     Options options = CurrentOptions();
 

db/fault_injection_test.cc

@@ -694,6 +694,30 @@ TEST(FaultInjectionFSTest,
   ASSERT_TRUE(s.IsNotSupported());
   ASSERT_OK(reopened_writer->Close(IOOptions(), nullptr));
 }
+
+TEST(FaultInjectionFSTest, FileOpenContractAllowsCreateAfterDelete) {
+  std::shared_ptr<FaultInjectionTestFS> fault_fs =
+      std::make_shared<FaultInjectionTestFS>(FileSystem::Default());
+  const std::string fname =
+      test::PerThreadDBPath("file_open_contract_recreate_after_delete");
+
+  FileOptions writer_options;
+  writer_options.open_contract = FileOpenContract::kNoReopenForWrite;
+
+  std::unique_ptr<FSWritableFile> writer;
+  ASSERT_OK(fault_fs->NewWritableFile(fname, writer_options, &writer, nullptr));
+  ASSERT_OK(writer->Append("abc", IOOptions(), nullptr));
+  ASSERT_OK(writer->Close(IOOptions(), nullptr));
+
+  ASSERT_OK(FileSystem::Default()->DeleteFile(fname, IOOptions(), nullptr));
+
+  std::unique_ptr<FSWritableFile> recreated_writer;
+  ASSERT_OK(fault_fs->NewWritableFile(fname, writer_options, &recreated_writer,
+                                      nullptr));
+  ASSERT_OK(recreated_writer->Append("def", IOOptions(), nullptr));
+  ASSERT_OK(recreated_writer->Close(IOOptions(), nullptr));
+  ASSERT_OK(fault_fs->DeleteFile(fname, IOOptions(), nullptr));
+}
 }  // namespace ROCKSDB_NAMESPACE
 
 int main(int argc, char** argv) {

utilities/fault_injection_env.h

@@ -183,8 +183,13 @@ class FaultInjectionTestEnv : public EnvWrapper {
 // Undef to eliminate clash on Windows
 #undef GetFreeSpace
   Status GetFreeSpace(const std::string& path, uint64_t* disk_free) override {
-    if (!IsFilesystemActive() &&
-        error_.subcode() == IOStatus::SubCode::kNoSpace) {
+    bool inactive_no_space = false;
+    {
+      MutexLock l(&mutex_);
+      inactive_no_space = !filesystem_active_ &&
+                          error_.subcode() == IOStatus::SubCode::kNoSpace;
+    }
+    if (inactive_no_space) {
       *disk_free = 0;
       return Status::OK();
     } else {
@@ -242,7 +247,10 @@ class FaultInjectionTestEnv : public EnvWrapper {
     error.PermitUncheckedError();
   }
   void AssertNoOpenFile() { assert(open_managed_files_.empty()); }
-  Status GetError() { return error_; }
+  Status GetError() {
+    MutexLock l(&mutex_);
+    return error_;
+  }
 
  private:
   port::Mutex mutex_;

utilities/fault_injection_fs.cc

@@ -968,20 +968,44 @@ IOStatus FaultInjectionTestFS::ValidateReadOpen(const std::string& fname,
 
 IOStatus FaultInjectionTestFS::ValidateWriteOpen(const std::string& fname,
                                                  const char* op_name) {
-  return ValidateNoReopenForWrite(fname, op_name);
+  return ValidateNoReopenForWrite(fname, op_name,
+                                  true /* allow_missing_file */);
 }
 
 IOStatus FaultInjectionTestFS::ValidateReopenedWrite(const std::string& fname,
                                                      const char* op_name) {
-  return ValidateNoReopenForWrite(fname, op_name);
+  return ValidateNoReopenForWrite(fname, op_name,
+                                  false /* allow_missing_file */);
 }
 
 IOStatus FaultInjectionTestFS::ValidateNoReopenForWrite(
-    const std::string& fname, const char* op_name) {
+    const std::string& fname, const char* op_name, bool allow_missing_file) {
+  FileOpenContract open_contract = FileOpenContract::kDefault;
+  {
     MutexLock l(&mutex_);
     auto it = file_open_contracts_.find(fname);
-  if (it != file_open_contracts_.end() &&
-      HasFileOpenContract(it->second, FileOpenContract::kNoReopenForWrite)) {
+    if (it != file_open_contracts_.end()) {
+      open_contract = it->second;
+    }
+  }
+  if (HasFileOpenContract(open_contract, FileOpenContract::kNoReopenForWrite)) {
+    if (allow_missing_file) {
+      // Some crash-simulation paths can make the target file disappear without
+      // going through DeleteFile, so clear stale contracts lazily at open time.
+      IOStatus exists_s =
+          target()->FileExists(fname, IOOptions(), nullptr /* dbg */);
+      if (exists_s.IsNotFound()) {
+        MutexLock l(&mutex_);
+        auto it = file_open_contracts_.find(fname);
+        if (it != file_open_contracts_.end() && it->second == open_contract) {
+          file_open_contracts_.erase(it);
+        }
+        return IOStatus::OK();
+      }
+      if (!exists_s.ok()) {
+        return exists_s;
+      }
+    }
     return IOStatus::NotSupported(
         std::string(op_name) +
         " violates no-reopen-for-write contract: " + fname);

utilities/fault_injection_fs.h

@@ -595,8 +595,13 @@ class FaultInjectionTestFS : public FileSystemWrapper {
   IOStatus GetFreeSpace(const std::string& path, const IOOptions& options,
                         uint64_t* disk_free, IODebugContext* dbg) override {
     IOStatus io_s;
-    if (!IsFilesystemActive() &&
-        fs_error_.subcode() == IOStatus::SubCode::kNoSpace) {
+    bool inactive_no_space = false;
+    {
+      MutexLock l(&mutex_);
+      inactive_no_space = !filesystem_active_ &&
+                          fs_error_.subcode() == IOStatus::SubCode::kNoSpace;
+    }
+    if (inactive_no_space) {
       *disk_free = 0;
     } else {
       io_s = MaybeInjectThreadLocalError(FaultInjectionIOType::kMetadataRead,
@@ -728,7 +733,10 @@ class FaultInjectionTestFS : public FileSystemWrapper {
 
   void AssertNoOpenFile() { assert(open_managed_files_.empty()); }
 
-  IOStatus GetError() { return fs_error_; }
+  IOStatus GetError() {
+    MutexLock l(&mutex_);
+    return fs_error_;
+  }
 
   void SetFileSystemIOError(IOStatus io_error) {
     MutexLock l(&mutex_);
@@ -916,7 +924,8 @@ class FaultInjectionTestFS : public FileSystemWrapper {
       "failed to write to WAL";
 
   IOStatus ValidateNoReopenForWrite(const std::string& fname,
-                                    const char* op_name);
+                                    const char* op_name,
+                                    bool allow_missing_file);
 
   port::Mutex mutex_;
   std::map<std::string, FSFileState> db_file_state_;