From 25e33273a934a97b76da4e061dd2de576859635e Mon Sep 17 00:00:00 2001
From: sdong <siying.d@fb.com>
Date: Mon, 4 May 2020 14:15:55 -0700
Subject: [PATCH] Avoid Swallowing Some File Consistency Checking Bugs (#6793)

Summary:
We are swallowing some file consistency checking failures. This is not expected. We are fixing two cases: DB reopen and manifest dump.
More places are not fixed and need follow-up.

Error from CheckConsistencyForDeletes() is also swallowed, which is not fixed in this PR.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/6793

Test Plan: Add a unit test to cover the reopen case.

Reviewed By: riversand963

Differential Revision: D21366525

fbshipit-source-id: eb438a322237814e8d5125f916a3c6de97f39ded
---
 HISTORY.md            |  2 +-
 db/db_test2.cc        | 18 ++++++++++++++++++
 db/version_builder.cc |  5 ++++-
 db/version_set.cc     |  8 ++++++--
 4 files changed, 29 insertions(+), 4 deletions(-)

diff --git a/HISTORY.md b/HISTORY.md
index fbec3ae87..819f55388 100644
--- a/HISTORY.md
+++ b/HISTORY.md
@@ -2,7 +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.
+* Fix consistency checking error swallowing in some cases when options.force_consistency_checks = true.
 
 ## 6.9.3 (04/28/2020)
 ### Bug Fixes
diff --git a/db/db_test2.cc b/db/db_test2.cc
index cf2c19d72..4cf6d7245 100644
--- a/db/db_test2.cc
+++ b/db/db_test2.cc
@@ -4306,6 +4306,24 @@ TEST_F(DBTest2, SameSmallestInSameLevel) {
   ASSERT_EQ("2,3,4,5,6,7,8", Get("key"));
 }
 
+TEST_F(DBTest2, FileConsistencyCheckInOpen) {
+  Put("foo", "bar");
+  Flush();
+
+  SyncPoint::GetInstance()->SetCallBack(
+      "VersionBuilder::CheckConsistencyBeforeReturn", [&](void* arg) {
+        Status* ret_s = static_cast<Status*>(arg);
+        *ret_s = Status::Corruption("fcc");
+      });
+  SyncPoint::GetInstance()->EnableProcessing();
+
+  Options options = CurrentOptions();
+  options.force_consistency_checks = true;
+  ASSERT_NOK(TryReopen(options));
+
+  SyncPoint::GetInstance()->DisableProcessing();
+}
+
 TEST_F(DBTest2, BlockBasedTablePrefixIndexSeekForPrev) {
   // create a DB with block prefix index
   BlockBasedTableOptions table_options;
diff --git a/db/version_builder.cc b/db/version_builder.cc
index c6ec40b44..628604962 100644
--- a/db/version_builder.cc
+++ b/db/version_builder.cc
@@ -312,7 +312,10 @@ class VersionBuilder::Rep {
       }
     }
 
-    return Status::OK();
+    Status ret_s;
+    TEST_SYNC_POINT_CALLBACK("VersionBuilder::CheckConsistencyBeforeReturn",
+                             &ret_s);
+    return ret_s;
   }
 
   Status CheckConsistencyForDeletes(VersionEdit* /*edit*/, uint64_t number,
diff --git a/db/version_set.cc b/db/version_set.cc
index 6eda94bbe..763d5eba7 100644
--- a/db/version_set.cc
+++ b/db/version_set.cc
@@ -4581,7 +4581,11 @@ Status VersionSet::Recover(
       Version* v = new Version(cfd, this, file_options_,
                                *cfd->GetLatestMutableCFOptions(),
                                current_version_number_++);
-      builder->SaveTo(v->storage_info());
+      s = builder->SaveTo(v->storage_info());
+      if (!s.ok()) {
+        delete v;
+        return s;
+      }
 
       // Install recovered version
       v->PrepareApply(*cfd->GetLatestMutableCFOptions(),
@@ -5132,7 +5136,7 @@ Status VersionSet::DumpManifest(Options& options, std::string& dscname,
       Version* v = new Version(cfd, this, file_options_,
                                *cfd->GetLatestMutableCFOptions(),
                                current_version_number_++);
-      builder->SaveTo(v->storage_info());
+      s = builder->SaveTo(v->storage_info());
       v->PrepareApply(*cfd->GetLatestMutableCFOptions(), false);
 
       printf("--------------- Column family \"%s\"  (ID %" PRIu32