Merge #38868

38868: libroach: avoid recycling encrypted WALs r=ajkr a=ajkr

RocksDB fails recovery if there are any non-empty WALs that have zero
readable entries. So we need to make sure
`EncryptedEnv::ReuseWritableFile()` cannot produce such files, even if
an inopportune crash happens. We do not know how to achieve this while
still changing the encryption key for the recycled WAL, so for now this
PR works around the problem in `EncryptedEnv::ReuseWritableFile()` by
faking recycling.

Release note: None

Co-authored-by: Andrew Kryczka <andrew.kryczka2@gmail.com>

c-deps/libroach/ccl/encrypted_env_test.cc

@@ -127,7 +127,7 @@ rocksdb::Status checkFileContents(rocksdb::Env* env, const std::string& filename
 
 };  // anonymous namespace
 
-TEST(EncryptedEnv, FileOps) {
+TEST(EncryptedEnv, BasicOps) {
   // Check various file operations against an encrypted env.
   // This exercises the env/file registry interaction.
   // We need to use a real underlying env as the MemEnv takes a **lot**
@@ -222,7 +222,113 @@ TEST(EncryptedEnv, FileOps) {
   EXPECT_OK(checkFileContents(encrypted_env.get(), file1, contents3));
   // Check with the plain env.
   EXPECT_OK(checkFileContents(env, file1, contents3));
-  // Try reading the still-encrypted file. We dropped the key on the floor.
-  EXPECT_ERR(checkFileContents(encrypted_env.get(), file2, contents),
-             "key_manager does not have a key with ID .*");
+  EXPECT_OK(checkFileContents(env, file2, contents3));
+}
+
+TEST(EncryptedEnv, ReuseWritableFileUnderlyingFailure) {
+  // Ensure all WALs are either empty or decryptable no matter when/whether
+  // `EncryptedEnv::ReuseWritableFile` fails. See comment above
+  // `EncryptedEnv::ReuseWritableFile`'s definition for details on why this
+  // guarantee is necessary for crash-safety.
+
+  // `ReuseWritableFileInjectionEnv` is used as the env underlying an
+  // `EncryptedEnv`. It injects faults in the operations used by
+  // `EncryptedEnv::ReuseWritableFile()`.
+  class ReuseWritableFileInjectionEnv : public rocksdb::EnvWrapper {
+   public:
+    enum class Mode {
+      kNone,
+      kFailCreation,
+      kFailDeletion,
+      kEnd,
+    };
+
+    explicit ReuseWritableFileInjectionEnv(Env* target) : EnvWrapper(target), mode_(Mode::kNone) {}
+
+    void set_mode(Mode mode) { mode_ = mode; }
+
+    rocksdb::Status NewWritableFile(const std::string& fname,
+                                    std::unique_ptr<rocksdb::WritableFile>* result,
+                                    const rocksdb::EnvOptions& options) override {
+      if (mode_ == Mode::kFailCreation) {
+        return rocksdb::Status::IOError("injected error");
+      }
+      return rocksdb::EnvWrapper::NewWritableFile(fname, result, options);
+    }
+
+    rocksdb::Status DeleteFile(const std::string& fname) override {
+      if (mode_ == Mode::kFailDeletion) {
+        return rocksdb::Status::IOError("injected error");
+      }
+      return rocksdb::EnvWrapper::DeleteFile(fname);
+    }
+
+   private:
+    Mode mode_;
+  };
+
+  for (int i = 0; i < static_cast<int>(ReuseWritableFileInjectionEnv::Mode::kEnd); ++i) {
+    auto mode = static_cast<ReuseWritableFileInjectionEnv::Mode>(i);
+    ReuseWritableFileInjectionEnv env(rocksdb::Env::Default());
+    auto key_manager = new MemKeyManager(MakeAES128Key(&env));
+    auto stream = new CTRCipherStreamCreator(key_manager, enginepb::Data);
+
+    auto tmpdir = std::unique_ptr<TempDirHandler>(new TempDirHandler());
+
+    auto file_registry = std::unique_ptr<FileRegistry>(
+        new FileRegistry(&env, tmpdir->Path(""), false /* read-only */));
+    EXPECT_OK(file_registry->Load());
+
+    std::unique_ptr<rocksdb::Env> encrypted_env(
+        rocksdb_utils::NewEncryptedEnv(&env, file_registry.get(), stream));
+
+    auto old_file = tmpdir->Path("foo1");
+    std::string contents("this is a file!");
+    ASSERT_OK(rocksdb::WriteStringToFile(encrypted_env.get(), contents, old_file, false));
+
+    auto new_file = tmpdir->Path("foo2");
+    env.set_mode(mode);
+    {
+      std::unique_ptr<rocksdb::WritableFile> res;
+      if (mode == ReuseWritableFileInjectionEnv::Mode::kNone) {
+        EXPECT_OK(
+            encrypted_env->ReuseWritableFile(new_file, old_file, &res, rocksdb::EnvOptions()));
+      } else {
+        EXPECT_TRUE(
+            encrypted_env->ReuseWritableFile(new_file, old_file, &res, rocksdb::EnvOptions())
+                .IsIOError());
+      }
+    }
+
+    switch (mode) {
+    case ReuseWritableFileInjectionEnv::Mode::kNone: {
+      // In success scenario, the new file should be empty and the old file should be deleted.
+      std::string new_file_contents;
+      EXPECT_OK(rocksdb::ReadFileToString(encrypted_env.get(), new_file, &new_file_contents));
+      EXPECT_STREQ("", new_file_contents.c_str());
+      EXPECT_TRUE(encrypted_env->FileExists(old_file).IsNotFound());
+      break;
+    }
+    case ReuseWritableFileInjectionEnv::Mode::kFailCreation:
+    case ReuseWritableFileInjectionEnv::Mode::kFailDeletion: {
+      // In failure scenarios, all existing files must be empty or readable.
+      for (const auto& file : {old_file, new_file}) {
+        auto exists_status = encrypted_env->FileExists(file);
+        if (exists_status.ok()) {
+          std::string file_contents;
+          rocksdb::ReadFileToString(encrypted_env.get(), file, &file_contents);
+          if (!file_contents.empty()) {
+            EXPECT_STREQ(contents.c_str(), file_contents.c_str());
+          }
+        } else {
+          EXPECT_TRUE(exists_status.IsNotFound());
+        }
+      }
+      break;
+    }
+    case ReuseWritableFileInjectionEnv::Mode::kEnd:
+      assert(false);
+      break;
+    }
+  }
 }

c-deps/libroach/rocksdbutils/env_encryption.cc

@@ -431,31 +431,32 @@ class EncryptedEnv : public rocksdb::EnvWrapper {
     return rocksdb::Status::OK();
   }
 
-  // Reuse an existing file by renaming it and opening it as writable.
+  // `ReuseWritableFile` is used for recycling WAL files. With `EncryptedEnv`, we
+  // currently do not know a crash-safe way to recycle WALs. The workaround is to
+  // pretend to recycle in this function by producing a new file and throwing away
+  // the old one. This method of "recycling" sacrifices performance for crash-safety.
+  //
+  // True recycling is not crash-safe because RocksDB recovery fails if it encounters
+  // a non-empty WAL with zero readable entries. With true recycling, we need to change
+  // the encryption key. Imagine a crash happened after we renamed the file and assigned
+  // a new key, but before we wrote any data to the recycled file. Then, that file would
+  // appear to recovery as containing all garbage data, causing it to fail.
+  //
+  // TODO(ajkr): Try to change how RocksDB handles WALs with zero readable entries:
+  // https://www.facebook.com/groups/rocksdb.dev/permalink/2405909486174218/
+  // Then we can have both performance and crash-safety.
   virtual rocksdb::Status ReuseWritableFile(const std::string& fname, const std::string& old_fname,
                                             std::unique_ptr<rocksdb::WritableFile>* result,
                                             const rocksdb::EnvOptions& options) override {
     result->reset();
     if (options.use_mmap_writes) {
       return rocksdb::Status::InvalidArgument();
     }
-    // Open file using underlying Env implementation
-    std::unique_ptr<rocksdb::WritableFile> underlying;
-    rocksdb::Status status =
-        rocksdb::EnvWrapper::ReuseWritableFile(fname, old_fname, &underlying, options);
-    if (!status.ok()) {
-      return status;
+    auto status = NewWritableFile(fname, result, options);
+    if (status.ok()) {
+      status = DeleteFile(old_fname);
     }
-
-    // Create cipher stream
-    std::unique_ptr<BlockAccessCipherStream> stream;
-    status = CreateCipherStream(fname, true /* new_file */, &stream);
-    if (!status.ok()) {
-      return status;
-    }
-    (*result) = std::unique_ptr<rocksdb::WritableFile>(
-        new EncryptedWritableFile(underlying.release(), stream.release()));
-    return rocksdb::Status::OK();
+    return status;
   }
 
   // Open `fname` for random read and write, if file dont exist the file