From 33087a2881544b4b0f3d3998d6da01b2a3d076e5 Mon Sep 17 00:00:00 2001
From: Andrew Kryczka <andrewkr@fb.com>
Date: Tue, 5 May 2020 14:52:18 -0700
Subject: [PATCH] fix swallowed error for file deletion consistency check
 (#6809)

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

Reviewed By: pdillinger

Differential Revision: D21411971

Pulled By: ajkr

fbshipit-source-id: 900b6b0370b76e9a3e5e03f968e2ac1bbaab73b8
---
 HISTORY.md                 |  1 +
 db/version_builder.cc      |  5 ++++-
 db/version_builder_test.cc | 30 ++++++++++++++++++++++++++++++
 3 files changed, 35 insertions(+), 1 deletion(-)

diff --git a/HISTORY.md b/HISTORY.md
index 628faabbf..fbec3ae87 100644
--- a/HISTORY.md
+++ b/HISTORY.md
@@ -2,6 +2,7 @@
 ## 6.9.4 (04/30/2020)
 ### Bug Fixes
 * Fix a bug caused by overwrite the status with io status in block based table builder when writing data blocks. If status stores the error message (e.g., failure of verify block compression), the bug will make the io status overwrite the status.
+* Fix `force_consistency_checks=true` use cases to not swallow errors returned by `CheckConsistencyForDeletes()`, which can expose internal bugs like multiple compactions involving a single file.
 
 ## 6.9.3 (04/28/2020)
 ### Bug Fixes
diff --git a/db/version_builder.cc b/db/version_builder.cc
index 13b34b3e8..c6ec40b44 100644
--- a/db/version_builder.cc
+++ b/db/version_builder.cc
@@ -441,7 +441,10 @@ class VersionBuilder::Rep {
       const auto number = del_file.second;
       if (level < num_levels_) {
         levels_[level].deleted_files.insert(number);
-        CheckConsistencyForDeletes(edit, number, level);
+        s = CheckConsistencyForDeletes(edit, number, level);
+        if (!s.ok()) {
+          return s;
+        }
 
         auto exising = levels_[level].added_files.find(number);
         if (exising != levels_[level].added_files.end()) {
diff --git a/db/version_builder_test.cc b/db/version_builder_test.cc
index 9523278f7..08f01a738 100644
--- a/db/version_builder_test.cc
+++ b/db/version_builder_test.cc
@@ -792,6 +792,36 @@ TEST_F(VersionBuilderTest, CheckConsistencyForBlobFilesAllGarbage) {
   UnrefFilesInVersion(&new_vstorage);
 }
 
+TEST_F(VersionBuilderTest, CheckConsistencyForFileDeletedTwice) {
+  Add(0, 1U, "150", "200", 100U);
+  UpdateVersionStorageInfo();
+
+  VersionEdit version_edit;
+  version_edit.DeleteFile(0, 1U);
+
+  EnvOptions env_options;
+  constexpr TableCache* table_cache = nullptr;
+  constexpr VersionSet* version_set = nullptr;
+
+  VersionBuilder version_builder(env_options, &ioptions_, table_cache,
+                                 &vstorage_, version_set);
+  VersionStorageInfo new_vstorage(&icmp_, ucmp_, options_.num_levels,
+                                  kCompactionStyleLevel, nullptr,
+                                  true /* force_consistency_checks */);
+  ASSERT_OK(version_builder.Apply(&version_edit));
+  ASSERT_OK(version_builder.SaveTo(&new_vstorage));
+
+  VersionBuilder version_builder2(env_options, &ioptions_, table_cache,
+                                 &new_vstorage, version_set);
+  VersionStorageInfo new_vstorage2(&icmp_, ucmp_, options_.num_levels,
+                                  kCompactionStyleLevel, nullptr,
+                                  true /* force_consistency_checks */);
+  ASSERT_NOK(version_builder2.Apply(&version_edit));
+
+  UnrefFilesInVersion(&new_vstorage);
+  UnrefFilesInVersion(&new_vstorage2);
+}
+
 TEST_F(VersionBuilderTest, EstimatedActiveKeys) {
   const uint32_t kTotalSamples = 20;
   const uint32_t kNumLevels = 5;