From 074134d98fb14e3ae1b81eb6dbab80dbf6dbdbd5 Mon Sep 17 00:00:00 2001 From: Anand Ananthabhotla Date: Tue, 20 Oct 2026 20:21:11 -0700 Subject: [PATCH] Stop writes on retryable background IO errors when auto resume is disabled (#15444) Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15444 **Why the change is required** `max_bgerror_resume_count <= 0` turns off automatic `DB::Resume()`, so the application is responsible for recovering from background errors. Before automatic recovery was introduced, a background error under `paranoid_checks` latched `bg_error_` and made the DB read-only. Since then, two classes of retryable IO error are either never latched or latched as soft errors that leave the DB accepting writes. When auto resume is disabled, no one recovers from them and the application gets no clear signal. **Previous behavior** (with `max_bgerror_resume_count <= 0`) - A retryable or file-scoped IO error during compaction was never set in `bg_error_`. Compaction rescheduled itself and retried indefinitely. A persistent failure was visible only through `EventListener::OnBackgroundError` and the LOG, while compaction debt grew until writes slowed or stopped on L0/pending-compaction triggers. - A retryable or file-scoped IO error while writing a flush SST or the MANIFEST after a successful WAL sync (`kFlushNoWAL` / `kManifestWriteNoWAL`, which covers almost every retryable flush failure, not only `disableWAL` writes) was a soft error with `soft_error_no_bg_work_` set. Background work stopped, but writes kept succeeding into the memtable. Nothing retried the flush, so writers eventually blocked indefinitely on the memtable limit instead of failing. **New behavior** With `max_bgerror_resume_count <= 0`, every retryable or file-scoped background IO error that reaches the retryable path in `ErrorHandler::SetBGErrorImpl` is latched as `kHardError`, whatever the reason (compaction, flush, MANIFEST, WAL). Writes fail immediately and reads continue to work. `DB::Resume()` clears the error, flushes and reschedules compaction as before. **Implementation** In `SetBGErrorImpl`, the compaction special case (notify the listener and return without setting `bg_error_`) now applies only when auto resume is enabled, and the severity selection maps everything to `kHardError` when auto resume is disabled. Because `kErrorRecoveryRetryFlush` is no longer selected in that mode, `soft_error_no_bg_work_` is not set. The option documentation in `include/rocksdb/options.h` and the comment above `SetBGError` are updated. **Safety, compatibility, and limitations** - This is an intentional behavior change, limited to DBs that set `max_bgerror_resume_count <= 0`. With the default (`INT_MAX`) or any positive value, nothing changes. - fbcode callers that set the option to 0, and their apparent expectations: - `unicorn/live_updates/dispatcher/MessageStore.cpp`: the comment says "we do not allow any [background errors] as they should be retried by the underlying FS". Stopping on retryable compaction errors fits that intent better than silent retries. - `reliable_volumes/client/index_manager/RocksDBIndexStorage.cpp`: a JustKnob (`disableRocksDbBackgroundErrorAutoResume`) disables auto resume alongside other corruption-detection options, which suggests it wants background errors surfaced rather than masked. - `zippydb/server/store` (`RocksDBStoreResumeWritesTest`): uses 0 with `disableWAL` and drives recovery through its own `ResumeWrites` task calling `DB::Resume()`, so it already expects writes to stop until resume. I read the test and expect it to pass, but did not run it. No production zippydb configuration setting 0 was found. - Out-of-space errors (`kNoSpace`/`kSpaceLimit`) are untouched: they skip this path and are still recovered through `SstFileManager`, which does not consult `max_bgerror_resume_count`. Errors ignored under `paranoid_checks=false` are also unchanged. - No API, ABI or on-disk format change. - Found during testing, pre-existing and not fixed here: `SetBGErrorImpl` releases the DB mutex in `NotifyOnBackgroundError` before calling `StartRecoverFromRetryableBGIOError`. If the last failed auto-resume attempt's flush error is reported in that window, the recovery thread can give up and clear `recovery_in_prog_` first, and the late report then starts a new recovery with a fresh retry budget, so `max_bgerror_resume_count` can be exceeded. This only applies with auto resume enabled. Worth a separate fix. Reviewed By: joshkang97 Differential Revision: D126453053 fbshipit-source-id: 3f4be1d12a8d44552313b022db5afe200bc1c6ce --- db/error_handler.cc | 17 +- db/error_handler_fs_test.cc | 463 +++++++++--------- include/rocksdb/options.h | 5 +- .../no_auto_resume_stops_writes.md | 1 + 4 files changed, 251 insertions(+), 235 deletions(-) create mode 100644 unreleased_history/behavior_changes/no_auto_resume_stops_writes.md diff --git a/db/error_handler.cc b/db/error_handler.cc index d962208f34..c32deabdf6 100644 --- a/db/error_handler.cc +++ b/db/error_handler.cc @@ -413,6 +413,8 @@ void ErrorHandler::HandleKnownErrors(const Status& bg_err, // to a soft error. Note that, it includes the case that IO error happens // in SST or manifest write during flush. Auto resume will be called. // c) all other errors are mapped to hard error. Auto resume will be called. +// If auto resume is disabled (max_bgerror_resume_count <= 0), all of the +// above are mapped to hard error and wait for a manual DB::Resume(). // 3) for other cases, HandleKnownErrors(const Status& bg_err, // BackgroundErrorReason reason) will be called to handle other error cases // such as delegating to SstFileManager to handle no space error. @@ -523,7 +525,8 @@ void ErrorHandler::SetBGErrorImpl(const Status& bg_status, {} /* int_histograms */); ROCKS_LOG_INFO(db_options_.info_log, "ErrorHandler: Set background retryable IO error\n"); - if (BackgroundErrorReason::kCompaction == reason) { + const bool auto_resume_enabled = db_options_.max_bgerror_resume_count > 0; + if (BackgroundErrorReason::kCompaction == reason && auto_resume_enabled) { // We map the retryable IO error during compaction to soft error. Since // compaction can reschedule by itself. We will not set the BG error in // this case @@ -543,8 +546,14 @@ void ErrorHandler::SetBGErrorImpl(const Status& bg_status, } Status::Severity severity; - if (BackgroundErrorReason::kFlushNoWAL == reason || - BackgroundErrorReason::kManifestWriteNoWAL == reason) { + if (!auto_resume_enabled) { + // Without auto resume, nothing retries or recovers from the error on the + // user's behalf, so stop writes until the user calls DB::Resume(). This + // includes compaction and no-WAL flush errors, which would otherwise be + // retried by compaction or tolerated as soft errors. + severity = Status::Severity::kHardError; + } else if (BackgroundErrorReason::kFlushNoWAL == reason || + BackgroundErrorReason::kManifestWriteNoWAL == reason) { // When the BG Retryable IO error reason is flush without WAL, // We map it to a soft error. At the same time, all the background work // should be stopped except the BG work from recovery. Therefore, we @@ -569,7 +578,7 @@ void ErrorHandler::SetBGErrorImpl(const Status& bg_status, return; } CheckAndSetRecoveryAndBGError(bg_err, context); - bool auto_recovery = db_options_.max_bgerror_resume_count > 0; + bool auto_recovery = auto_resume_enabled; EventHelpers::NotifyOnBackgroundError(db_options_.listeners, reason, &new_bg_io_err, db_mutex_, &auto_recovery); diff --git a/db/error_handler_fs_test.cc b/db/error_handler_fs_test.cc index 248f743db5..a4cc62fa60 100644 --- a/db/error_handler_fs_test.cc +++ b/db/error_handler_fs_test.cc @@ -59,6 +59,34 @@ class DBErrorHandlingFSTest : public DBTestBase { return ""; } + // Holds automatic recovery from starting until ActivateFSAndRecover(), so + // the first auto resume attempt does not run against the failed FS. + void HoldAutoRecovery() { + SyncPoint::GetInstance()->LoadDependency( + {{"DBErrorHandlingFSTest::ReleaseAutoRecovery", + "RecoverFromRetryableBGIOError:BeforeStart"}, + {"RecoverFromRetryableBGIOError:RecoverSuccess", + "DBErrorHandlingFSTest::AutoRecoveryDone"}}); + } + + // Re-activates the FS and recovers from the background error: with a manual + // Resume() if auto resume is disabled, otherwise by waiting for the auto + // recovery held by HoldAutoRecovery() to succeed. The recovery thread is + // waited on directly because OnErrorRecoveryEnd() fires before the thread + // finishes, and a later shutdown would notify again. + void ActivateFSAndRecover() { + SyncPoint::GetInstance()->ClearAllCallBacks(); + fault_fs_->SetFilesystemActive(true); + TEST_SYNC_POINT("DBErrorHandlingFSTest::ReleaseAutoRecovery"); + if (dbfull()->GetDBOptions().max_bgerror_resume_count > 0) { + TEST_SYNC_POINT("DBErrorHandlingFSTest::AutoRecoveryDone"); + } else { + ASSERT_OK(dbfull()->Resume()); + } + ASSERT_OK(dbfull()->TEST_GetBGError()); + SyncPoint::GetInstance()->DisableProcessing(); + } + std::shared_ptr fault_fs_; std::unique_ptr fault_env_; }; @@ -278,7 +306,7 @@ TEST_F(DBErrorHandlingFSTest, FlushWriteRetryableError) { [&](void*) { fault_fs_->SetFilesystemActive(false, error_msg); }); SyncPoint::GetInstance()->EnableProcessing(); s = Flush(); - ASSERT_EQ(s.severity(), ROCKSDB_NAMESPACE::Status::Severity::kSoftError); + ASSERT_EQ(s.severity(), ROCKSDB_NAMESPACE::Status::Severity::kHardError); SyncPoint::GetInstance()->DisableProcessing(); fault_fs_->SetFilesystemActive(true); s = dbfull()->Resume(); @@ -304,7 +332,7 @@ TEST_F(DBErrorHandlingFSTest, FlushWriteRetryableError) { [&](void*) { fault_fs_->SetFilesystemActive(false, error_msg); }); SyncPoint::GetInstance()->EnableProcessing(); s = Flush(); - ASSERT_EQ(s.severity(), ROCKSDB_NAMESPACE::Status::Severity::kSoftError); + ASSERT_EQ(s.severity(), ROCKSDB_NAMESPACE::Status::Severity::kHardError); SyncPoint::GetInstance()->DisableProcessing(); fault_fs_->SetFilesystemActive(true); s = dbfull()->Resume(); @@ -318,7 +346,7 @@ TEST_F(DBErrorHandlingFSTest, FlushWriteRetryableError) { [&](void*) { fault_fs_->SetFilesystemActive(false, error_msg); }); SyncPoint::GetInstance()->EnableProcessing(); s = Flush(); - ASSERT_EQ(s.severity(), ROCKSDB_NAMESPACE::Status::Severity::kSoftError); + ASSERT_EQ(s.severity(), ROCKSDB_NAMESPACE::Status::Severity::kHardError); SyncPoint::GetInstance()->DisableProcessing(); fault_fs_->SetFilesystemActive(true); s = dbfull()->Resume(); @@ -329,88 +357,60 @@ TEST_F(DBErrorHandlingFSTest, FlushWriteRetryableError) { Destroy(options); } +// Without auto resume, the error stops writes until a manual Resume(). With +// auto resume, it is a soft error cleared by automatic recovery. TEST_F(DBErrorHandlingFSTest, FlushWriteFileScopeError) { - std::shared_ptr listener = - std::make_shared(); - Options options = GetDefaultOptions(); - options.env = fault_env_.get(); - options.create_if_missing = true; - options.listeners.emplace_back(listener); - options.max_bgerror_resume_count = 0; - Status s; + for (int resume_count : {0, 2}) { + SCOPED_TRACE("max_bgerror_resume_count=" + std::to_string(resume_count)); + std::shared_ptr listener = + std::make_shared(); + Options options = GetDefaultOptions(); + options.env = fault_env_.get(); + options.create_if_missing = true; + options.listeners.emplace_back(listener); + options.max_bgerror_resume_count = resume_count; + const Status::Severity expected_severity = + resume_count > 0 ? Status::Severity::kSoftError + : Status::Severity::kHardError; - listener->EnableAutoRecovery(false); - DestroyAndReopen(options); + listener->EnableAutoRecovery(false); + DestroyAndReopen(options); - IOStatus error_msg = IOStatus::IOError("File Scope Data Loss Error"); - error_msg.SetDataLoss(true); - error_msg.SetScope( - ROCKSDB_NAMESPACE::IOStatus::IOErrorScope::kIOErrorScopeFile); - error_msg.SetRetryable(false); + IOStatus error_msg = IOStatus::IOError("File Scope Data Loss Error"); + error_msg.SetDataLoss(true); + error_msg.SetScope(IOStatus::IOErrorScope::kIOErrorScopeFile); + error_msg.SetRetryable(false); - ASSERT_OK(Put(Key(1), "val1")); - SyncPoint::GetInstance()->SetCallBack( - "BuildTable:BeforeFinishBuildTable", - [&](void*) { fault_fs_->SetFilesystemActive(false, error_msg); }); - SyncPoint::GetInstance()->EnableProcessing(); - s = Flush(); - ASSERT_EQ(s.severity(), ROCKSDB_NAMESPACE::Status::Severity::kSoftError); - SyncPoint::GetInstance()->DisableProcessing(); - fault_fs_->SetFilesystemActive(true); - s = dbfull()->Resume(); - ASSERT_OK(s); - Reopen(options); - ASSERT_EQ("val1", Get(Key(1))); + auto fail_flush_and_recover = [&](const char* sync_point, + const std::string& key) { + ASSERT_OK(Put(key, "val")); + HoldAutoRecovery(); + SyncPoint::GetInstance()->SetCallBack(sync_point, [&](void*) { + fault_fs_->SetFilesystemActive(false, error_msg); + }); + SyncPoint::GetInstance()->EnableProcessing(); + ASSERT_EQ(Flush().severity(), expected_severity); + ASSERT_NO_FATAL_FAILURE(ActivateFSAndRecover()); + Reopen(options); + ASSERT_EQ("val", Get(key)); + }; - ASSERT_OK(Put(Key(2), "val2")); - SyncPoint::GetInstance()->SetCallBack( - "BuildTable:BeforeSyncTable", - [&](void*) { fault_fs_->SetFilesystemActive(false, error_msg); }); - SyncPoint::GetInstance()->EnableProcessing(); - s = Flush(); - ASSERT_EQ(s.severity(), ROCKSDB_NAMESPACE::Status::Severity::kSoftError); - SyncPoint::GetInstance()->DisableProcessing(); - fault_fs_->SetFilesystemActive(true); - s = dbfull()->Resume(); - ASSERT_OK(s); - Reopen(options); - ASSERT_EQ("val2", Get(Key(2))); + ASSERT_NO_FATAL_FAILURE( + fail_flush_and_recover("BuildTable:BeforeFinishBuildTable", Key(1))); + ASSERT_NO_FATAL_FAILURE( + fail_flush_and_recover("BuildTable:BeforeSyncTable", Key(2))); + ASSERT_NO_FATAL_FAILURE( + fail_flush_and_recover("BuildTable:BeforeCloseTableFile", Key(3))); - ASSERT_OK(Put(Key(3), "val3")); - SyncPoint::GetInstance()->SetCallBack( - "BuildTable:BeforeCloseTableFile", - [&](void*) { fault_fs_->SetFilesystemActive(false, error_msg); }); - SyncPoint::GetInstance()->EnableProcessing(); - s = Flush(); - ASSERT_EQ(s.severity(), ROCKSDB_NAMESPACE::Status::Severity::kSoftError); - SyncPoint::GetInstance()->DisableProcessing(); - fault_fs_->SetFilesystemActive(true); - s = dbfull()->Resume(); - ASSERT_OK(s); - Reopen(options); - ASSERT_EQ("val3", Get(Key(3))); + // not file scope, but retyrable set + error_msg.SetDataLoss(false); + error_msg.SetScope(IOStatus::IOErrorScope::kIOErrorScopeFileSystem); + error_msg.SetRetryable(true); + ASSERT_NO_FATAL_FAILURE( + fail_flush_and_recover("BuildTable:BeforeCloseTableFile", Key(4))); - // not file scope, but retyrable set - error_msg.SetDataLoss(false); - error_msg.SetScope( - ROCKSDB_NAMESPACE::IOStatus::IOErrorScope::kIOErrorScopeFileSystem); - error_msg.SetRetryable(true); - - ASSERT_OK(Put(Key(3), "val3")); - SyncPoint::GetInstance()->SetCallBack( - "BuildTable:BeforeCloseTableFile", - [&](void*) { fault_fs_->SetFilesystemActive(false, error_msg); }); - SyncPoint::GetInstance()->EnableProcessing(); - s = Flush(); - ASSERT_EQ(s.severity(), ROCKSDB_NAMESPACE::Status::Severity::kSoftError); - SyncPoint::GetInstance()->DisableProcessing(); - fault_fs_->SetFilesystemActive(true); - s = dbfull()->Resume(); - ASSERT_OK(s); - Reopen(options); - ASSERT_EQ("val3", Get(Key(3))); - - Destroy(options); + Destroy(options); + } } TEST_F(DBErrorHandlingFSTest, FlushWALWriteRetryableError) { @@ -528,14 +528,15 @@ TEST_F(DBErrorHandlingFSTest, FlushWritNoWALRetryableError1) { [&](void*) { fault_fs_->SetFilesystemActive(false, error_msg); }); SyncPoint::GetInstance()->EnableProcessing(); s = Flush(); - ASSERT_OK(Put(Key(2), "val2", wo)); - ASSERT_EQ(s.severity(), ROCKSDB_NAMESPACE::Status::Severity::kSoftError); - ASSERT_EQ("val2", Get(Key(2))); + // Without auto resume, the error stops writes until a manual Resume() + ASSERT_EQ(s.severity(), ROCKSDB_NAMESPACE::Status::Severity::kHardError); + ASSERT_NOK(Put(Key(2), "val2", wo)); SyncPoint::GetInstance()->DisableProcessing(); fault_fs_->SetFilesystemActive(true); s = dbfull()->Resume(); ASSERT_OK(s); ASSERT_EQ("val1", Get(Key(1))); + ASSERT_OK(Put(Key(2), "val2", wo)); ASSERT_EQ("val2", Get(Key(2))); ASSERT_OK(Put(Key(3), "val3", wo)); ASSERT_EQ("val3", Get(Key(3))); @@ -584,14 +585,15 @@ TEST_F(DBErrorHandlingFSTest, FlushWriteNoWALRetryableError2) { [&](void*) { fault_fs_->SetFilesystemActive(false, error_msg); }); SyncPoint::GetInstance()->EnableProcessing(); s = Flush(); - ASSERT_OK(Put(Key(2), "val2", wo)); - ASSERT_EQ(s.severity(), ROCKSDB_NAMESPACE::Status::Severity::kSoftError); - ASSERT_EQ("val2", Get(Key(2))); + // Without auto resume, the error stops writes until a manual Resume() + ASSERT_EQ(s.severity(), ROCKSDB_NAMESPACE::Status::Severity::kHardError); + ASSERT_NOK(Put(Key(2), "val2", wo)); SyncPoint::GetInstance()->DisableProcessing(); fault_fs_->SetFilesystemActive(true); s = dbfull()->Resume(); ASSERT_OK(s); ASSERT_EQ("val1", Get(Key(1))); + ASSERT_OK(Put(Key(2), "val2", wo)); ASSERT_EQ("val2", Get(Key(2))); ASSERT_OK(Put(Key(3), "val3", wo)); ASSERT_EQ("val3", Get(Key(3))); @@ -628,14 +630,15 @@ TEST_F(DBErrorHandlingFSTest, FlushWriteNoWALRetryableError3) { [&](void*) { fault_fs_->SetFilesystemActive(false, error_msg); }); SyncPoint::GetInstance()->EnableProcessing(); s = Flush(); - ASSERT_OK(Put(Key(2), "val2", wo)); - ASSERT_EQ(s.severity(), ROCKSDB_NAMESPACE::Status::Severity::kSoftError); - ASSERT_EQ("val2", Get(Key(2))); + // Without auto resume, the error stops writes until a manual Resume() + ASSERT_EQ(s.severity(), ROCKSDB_NAMESPACE::Status::Severity::kHardError); + ASSERT_NOK(Put(Key(2), "val2", wo)); SyncPoint::GetInstance()->DisableProcessing(); fault_fs_->SetFilesystemActive(true); s = dbfull()->Resume(); ASSERT_OK(s); ASSERT_EQ("val1", Get(Key(1))); + ASSERT_OK(Put(Key(2), "val2", wo)); ASSERT_EQ("val2", Get(Key(2))); ASSERT_OK(Put(Key(3), "val3", wo)); ASSERT_EQ("val3", Get(Key(3))); @@ -716,7 +719,7 @@ TEST_F(DBErrorHandlingFSTest, ManifestWriteRetryableError) { [&](void*) { fault_fs_->SetFilesystemActive(false, error_msg); }); SyncPoint::GetInstance()->EnableProcessing(); s = Flush(); - ASSERT_EQ(s.severity(), ROCKSDB_NAMESPACE::Status::Severity::kSoftError); + ASSERT_EQ(s.severity(), ROCKSDB_NAMESPACE::Status::Severity::kHardError); ASSERT_FALSE(dbfull()->TEST_GetFilesToQuarantine().empty()); SyncPoint::GetInstance()->ClearAllCallBacks(); SyncPoint::GetInstance()->DisableProcessing(); @@ -734,52 +737,52 @@ TEST_F(DBErrorHandlingFSTest, ManifestWriteRetryableError) { Close(); } +// Without auto resume, the error stops writes until a manual Resume(). With +// auto resume, it is a soft error cleared by automatic recovery. TEST_F(DBErrorHandlingFSTest, ManifestWriteFileScopeError) { - std::shared_ptr listener = - std::make_shared(); - Options options = GetDefaultOptions(); - options.env = fault_env_.get(); - options.create_if_missing = true; - options.listeners.emplace_back(listener); - options.max_bgerror_resume_count = 0; - Status s; - std::string old_manifest; - std::string new_manifest; + for (int resume_count : {0, 2}) { + SCOPED_TRACE("max_bgerror_resume_count=" + std::to_string(resume_count)); + std::shared_ptr listener = + std::make_shared(); + Options options = GetDefaultOptions(); + options.env = fault_env_.get(); + options.create_if_missing = true; + options.listeners.emplace_back(listener); + options.max_bgerror_resume_count = resume_count; + const Status::Severity expected_severity = + resume_count > 0 ? Status::Severity::kSoftError + : Status::Severity::kHardError; - listener->EnableAutoRecovery(false); - DestroyAndReopen(options); - old_manifest = GetManifestNameFromLiveFiles(); + listener->EnableAutoRecovery(false); + DestroyAndReopen(options); + const std::string old_manifest = GetManifestNameFromLiveFiles(); - IOStatus error_msg = IOStatus::IOError("File Scope Data Loss Error"); - error_msg.SetDataLoss(true); - error_msg.SetScope( - ROCKSDB_NAMESPACE::IOStatus::IOErrorScope::kIOErrorScopeFile); - error_msg.SetRetryable(false); + IOStatus error_msg = IOStatus::IOError("File Scope Data Loss Error"); + error_msg.SetDataLoss(true); + error_msg.SetScope(IOStatus::IOErrorScope::kIOErrorScopeFile); + error_msg.SetRetryable(false); - ASSERT_OK(Put(Key(0), "val")); - ASSERT_OK(Flush()); - ASSERT_OK(Put(Key(1), "val")); - SyncPoint::GetInstance()->SetCallBack( - "VersionSet::LogAndApply:WriteManifest", - [&](void*) { fault_fs_->SetFilesystemActive(false, error_msg); }); - SyncPoint::GetInstance()->EnableProcessing(); - s = Flush(); - ASSERT_FALSE(dbfull()->TEST_GetFilesToQuarantine().empty()); - ASSERT_EQ(s.severity(), ROCKSDB_NAMESPACE::Status::Severity::kSoftError); - SyncPoint::GetInstance()->ClearAllCallBacks(); - SyncPoint::GetInstance()->DisableProcessing(); - fault_fs_->SetFilesystemActive(true); - s = dbfull()->Resume(); - ASSERT_OK(s); + ASSERT_OK(Put(Key(0), "val")); + ASSERT_OK(Flush()); + ASSERT_OK(Put(Key(1), "val")); + HoldAutoRecovery(); + SyncPoint::GetInstance()->SetCallBack( + "VersionSet::LogAndApply:WriteManifest", + [&](void*) { fault_fs_->SetFilesystemActive(false, error_msg); }); + SyncPoint::GetInstance()->EnableProcessing(); + Status s = Flush(); + ASSERT_FALSE(dbfull()->TEST_GetFilesToQuarantine().empty()); + ASSERT_EQ(s.severity(), expected_severity); + ASSERT_NO_FATAL_FAILURE(ActivateFSAndRecover()); - new_manifest = GetManifestNameFromLiveFiles(); - ASSERT_NE(new_manifest, old_manifest); - ASSERT_TRUE(dbfull()->TEST_GetFilesToQuarantine().empty()); + ASSERT_NE(GetManifestNameFromLiveFiles(), old_manifest); + ASSERT_TRUE(dbfull()->TEST_GetFilesToQuarantine().empty()); - Reopen(options); - ASSERT_EQ("val", Get(Key(0))); - ASSERT_EQ("val", Get(Key(1))); - Close(); + Reopen(options); + ASSERT_EQ("val", Get(Key(0))); + ASSERT_EQ("val", Get(Key(1))); + Close(); + } } TEST_F(DBErrorHandlingFSTest, ManifestWriteNoWALRetryableError) { @@ -811,7 +814,7 @@ TEST_F(DBErrorHandlingFSTest, ManifestWriteNoWALRetryableError) { [&](void*) { fault_fs_->SetFilesystemActive(false, error_msg); }); SyncPoint::GetInstance()->EnableProcessing(); s = Flush(); - ASSERT_EQ(s.severity(), ROCKSDB_NAMESPACE::Status::Severity::kSoftError); + ASSERT_EQ(s.severity(), ROCKSDB_NAMESPACE::Status::Severity::kHardError); ASSERT_FALSE(dbfull()->TEST_GetFilesToQuarantine().empty()); SyncPoint::GetInstance()->ClearAllCallBacks(); SyncPoint::GetInstance()->DisableProcessing(); @@ -1076,102 +1079,77 @@ TEST_F(DBErrorHandlingFSTest, CompactionWriteError) { Destroy(options); } -TEST_F(DBErrorHandlingFSTest, DISABLED_CompactionWriteRetryableError) { - std::shared_ptr listener = - std::make_shared(); - Options options = GetDefaultOptions(); - options.env = fault_env_.get(); - options.create_if_missing = true; - options.level0_file_num_compaction_trigger = 2; - options.listeners.emplace_back(listener); - options.max_bgerror_resume_count = 0; - Status s; - DestroyAndReopen(options); +// Without auto resume, a retryable or file scope IO error during compaction +// must stop writes until a manual Resume(), rather than being silently +// retried by rescheduling the compaction. +TEST_F(DBErrorHandlingFSTest, CompactionWriteErrorNoAutoResume) { + IOStatus retryable_error = IOStatus::IOError("Retryable IO Error"); + retryable_error.SetRetryable(true); + IOStatus file_scope_error = IOStatus::IOError("File Scope Data Loss Error"); + file_scope_error.SetDataLoss(true); + file_scope_error.SetScope(IOStatus::IOErrorScope::kIOErrorScopeFile); + file_scope_error.SetRetryable(false); - IOStatus error_msg = IOStatus::IOError("Retryable IO Error"); - error_msg.SetRetryable(true); + for (const IOStatus& error_msg : {retryable_error, file_scope_error}) { + SCOPED_TRACE(error_msg.ToString()); + Options options = GetDefaultOptions(); + options.env = fault_env_.get(); + options.create_if_missing = true; + options.level0_file_num_compaction_trigger = 2; + options.max_bgerror_resume_count = 0; + options.statistics = CreateDBStatistics(); + DestroyAndReopen(options); - ASSERT_OK(Put(Key(0), "va;")); - ASSERT_OK(Put(Key(2), "va;")); - s = Flush(); - ASSERT_OK(s); + ASSERT_OK(Put(Key(0), "val")); + ASSERT_OK(Put(Key(2), "val")); + ASSERT_OK(Flush()); - listener->OverrideBGError(Status(error_msg, Status::Severity::kHardError)); - listener->EnableAutoRecovery(false); - ROCKSDB_NAMESPACE::SyncPoint::GetInstance()->LoadDependency( - {{"DBImpl::FlushMemTable:FlushMemTableFinished", - "BackgroundCallCompaction:0"}}); - ROCKSDB_NAMESPACE::SyncPoint::GetInstance()->SetCallBack( - "CompactionJob::OpenCompactionOutputFile", - [&](void*) { fault_fs_->SetFilesystemActive(false, error_msg); }); - ROCKSDB_NAMESPACE::SyncPoint::GetInstance()->SetCallBack( - "DBImpl::BackgroundCompaction:Finish", - [&](void*) { CancelAllBackgroundWork(dbfull()); }); - ROCKSDB_NAMESPACE::SyncPoint::GetInstance()->EnableProcessing(); + SyncPoint::GetInstance()->LoadDependency( + {{"DBImpl::FlushMemTable:FlushMemTableFinished", + "BackgroundCallCompaction:0"}}); + // Fail only the first attempt, so a rescheduled compaction would succeed + std::atomic injected(false); + SyncPoint::GetInstance()->SetCallBack( + "DBImpl::BackgroundCompaction:Start", [&](void*) { + if (injected.load()) { + fault_fs_->SetFilesystemActive(true); + } + }); + SyncPoint::GetInstance()->SetCallBack( + "CompactionJob::OpenCompactionOutputFile", [&](void*) { + if (!injected.exchange(true)) { + fault_fs_->SetFilesystemActive(false, error_msg); + } + }); + SyncPoint::GetInstance()->EnableProcessing(); - ASSERT_OK(Put(Key(1), "val")); - s = Flush(); - ASSERT_OK(s); + ASSERT_OK(Put(Key(1), "val")); + ASSERT_OK(Flush()); - s = dbfull()->TEST_GetBGError(); - ASSERT_OK(s); - fault_fs_->SetFilesystemActive(true); - SyncPoint::GetInstance()->ClearAllCallBacks(); - SyncPoint::GetInstance()->DisableProcessing(); - s = dbfull()->Resume(); - ASSERT_OK(s); - Destroy(options); -} + Status s = dbfull()->TEST_WaitForCompact(); + ASSERT_EQ(s.severity(), Status::Severity::kHardError); + ASSERT_EQ(dbfull()->TEST_GetBGError().severity(), + Status::Severity::kHardError); + ASSERT_NOK(Put(Key(3), "val")); -TEST_F(DBErrorHandlingFSTest, DISABLED_CompactionWriteFileScopeError) { - std::shared_ptr listener = - std::make_shared(); - Options options = GetDefaultOptions(); - options.env = fault_env_.get(); - options.create_if_missing = true; - options.level0_file_num_compaction_trigger = 2; - options.listeners.emplace_back(listener); - options.max_bgerror_resume_count = 0; - Status s; - DestroyAndReopen(options); + SyncPoint::GetInstance()->DisableProcessing(); + SyncPoint::GetInstance()->ClearAllCallBacks(); + fault_fs_->SetFilesystemActive(true); + // Reads are still served while writes are stopped + ASSERT_EQ("val", Get(Key(1))); + ASSERT_OK(dbfull()->Resume()); + ASSERT_OK(Put(Key(3), "val")); + ASSERT_OK(dbfull()->TEST_WaitForCompact()); + ASSERT_EQ(0, NumTableFilesAtLevel(0)); + ASSERT_EQ(0, options.statistics->getAndResetTickerCount( + ERROR_HANDLER_AUTORESUME_COUNT)); - IOStatus error_msg = IOStatus::IOError("File Scope Data Loss Error"); - error_msg.SetDataLoss(true); - error_msg.SetScope( - ROCKSDB_NAMESPACE::IOStatus::IOErrorScope::kIOErrorScopeFile); - error_msg.SetRetryable(false); - - ASSERT_OK(Put(Key(0), "va;")); - ASSERT_OK(Put(Key(2), "va;")); - s = Flush(); - ASSERT_OK(s); - - listener->OverrideBGError(Status(error_msg, Status::Severity::kHardError)); - listener->EnableAutoRecovery(false); - ROCKSDB_NAMESPACE::SyncPoint::GetInstance()->LoadDependency( - {{"DBImpl::FlushMemTable:FlushMemTableFinished", - "BackgroundCallCompaction:0"}}); - ROCKSDB_NAMESPACE::SyncPoint::GetInstance()->SetCallBack( - "CompactionJob::OpenCompactionOutputFile", - [&](void*) { fault_fs_->SetFilesystemActive(false, error_msg); }); - ROCKSDB_NAMESPACE::SyncPoint::GetInstance()->SetCallBack( - "DBImpl::BackgroundCompaction:Finish", - [&](void*) { CancelAllBackgroundWork(dbfull()); }); - ROCKSDB_NAMESPACE::SyncPoint::GetInstance()->EnableProcessing(); - - ASSERT_OK(Put(Key(1), "val")); - s = Flush(); - ASSERT_OK(s); - - s = dbfull()->TEST_GetBGError(); - ASSERT_OK(s); - - fault_fs_->SetFilesystemActive(true); - SyncPoint::GetInstance()->ClearAllCallBacks(); - SyncPoint::GetInstance()->DisableProcessing(); - s = dbfull()->Resume(); - ASSERT_OK(s); - Destroy(options); + Reopen(options); + for (int i = 0; i < 4; ++i) { + ASSERT_EQ("val", Get(Key(i))); + } + Destroy(options); + } } TEST_F(DBErrorHandlingFSTest, CorruptionError) { @@ -2196,13 +2174,15 @@ TEST_F(DBErrorHandlingFSTest, ManualResumePreservesStrongerRecoveryContext) { options.env = fault_env_.get(); options.create_if_missing = true; options.listeners.emplace_back(listener); - options.max_bgerror_resume_count = 0; + // A soft no-WAL error is only possible with auto resume enabled. Use a + // single attempt that fails, so the error is left for a manual Resume(). + options.max_bgerror_resume_count = 1; + options.bgerror_resume_retry_interval = 1000; options.avoid_flush_during_shutdown = true; listener->EnableAutoRecovery(false); DestroyAndReopen(options); ASSERT_OK(Put(Key(0), "value")); - const uint64_t wal_before_recovery = dbfull()->TEST_GetCurrentLogNumber(); IOStatus soft_error = IOStatus::IOError("injected soft error"); dbfull()->TEST_SetBGError(soft_error, BackgroundErrorReason::kAsyncFileOpen); @@ -2211,11 +2191,36 @@ TEST_F(DBErrorHandlingFSTest, ManualResumePreservesStrongerRecoveryContext) { IOStatus retryable_error = IOStatus::IOError("injected retryable error"); retryable_error.SetRetryable(true); + // Let the flush thread finish reporting its error before auto recovery gives + // up. Otherwise the report can start a new recovery after the old one ends. + SyncPoint::GetInstance()->LoadDependency( + {{"DBImpl::BackgroundCallFlush:FlushFinish:0", + "RecoverFromRetryableBGIOError:BeforeWait0"}, + {"RecoverFromRetryableBGIOError:LoopOut", + "ManualResumePreservesStrongerRecoveryContext:AutoResumeDone"}}); + std::atomic injected(false); + SyncPoint::GetInstance()->SetCallBack( + "FlushJob::WriteLevel0Table:s", [&](void* arg) { + if (!injected.exchange(true)) { + *static_cast(arg) = retryable_error; + } + }); + SyncPoint::GetInstance()->EnableProcessing(); dbfull()->TEST_SetBGError(retryable_error, BackgroundErrorReason::kFlushNoWAL); + TEST_SYNC_POINT( + "ManualResumePreservesStrongerRecoveryContext:AutoResumeDone"); + SyncPoint::GetInstance()->DisableProcessing(); + SyncPoint::GetInstance()->ClearAllCallBacks(); + ASSERT_TRUE(injected.load()); + ASSERT_FALSE(dbfull()->TEST_IsRecoveryInProgress()); ASSERT_EQ(Status::Severity::kSoftError, dbfull()->TEST_GetBGError().severity()); + // Writes continue under the soft error. The manual resume must flush them, + // which switches the WAL, rather than clear the error directly. + ASSERT_OK(Put(Key(1), "value")); + const uint64_t wal_before_recovery = dbfull()->TEST_GetCurrentLogNumber(); ASSERT_OK(dbfull()->Resume()); const uint64_t wal_after_recovery = dbfull()->TEST_GetCurrentLogNumber(); ASSERT_GT(wal_after_recovery, wal_before_recovery) @@ -2223,11 +2228,10 @@ TEST_F(DBErrorHandlingFSTest, ManualResumePreservesStrongerRecoveryContext) { Reopen(options); ASSERT_EQ("value", Get(Key(0))); + ASSERT_EQ("value", Get(Key(1))); Destroy(options); } -// Auto resume fromt the flush retryable IO error. Activate the FS before the -// first resume. Resume is successful TEST_F(DBErrorHandlingFSTest, FlushWritRetryableErrorAutoRecover1) { // Activate the FS before the first resume std::shared_ptr listener = @@ -4533,7 +4537,8 @@ TEST_F(DBErrorHandlingFSTest, AtomicFlushErrorRecoveryTwoCFs) { options.create_if_missing = true; options.atomic_flush = true; options.listeners.emplace_back(listener); - options.max_bgerror_resume_count = 0; + // Auto resume keeps the no-WAL error soft, so recovery retries the flushes + options.max_bgerror_resume_count = 2; listener->EnableAutoRecovery(false); CreateAndReopenWithCF({"pikachu"}, options); @@ -4552,20 +4557,18 @@ TEST_F(DBErrorHandlingFSTest, AtomicFlushErrorRecoveryTwoCFs) { ASSERT_OK(Put(0, Key(1), "v0b", wo)); ASSERT_OK(Put(1, Key(0), "v1", wo)); + HoldAutoRecovery(); SyncPoint::GetInstance()->SetCallBack( "VersionSet::LogAndApply:WriteManifest", [&](void*) { fault_fs_->SetFilesystemActive(false, error_msg); }); SyncPoint::GetInstance()->EnableProcessing(); Status s = Flush({0, 1}); ASSERT_EQ(s.severity(), ROCKSDB_NAMESPACE::Status::Severity::kSoftError); - SyncPoint::GetInstance()->ClearAllCallBacks(); - SyncPoint::GetInstance()->DisableProcessing(); - fault_fs_->SetFilesystemActive(true); - // Resume routes through RetryFlushesForErrorRecovery(wait=true); the pairing - // invariant in that function fires under a debug build if the ids are - // misaligned with cfds. - ASSERT_OK(dbfull()->Resume()); + // Auto recovery routes through RetryFlushesForErrorRecovery(wait=true); the + // pairing invariant in that function fires under a debug build if the ids + // are misaligned with cfds. + ASSERT_NO_FATAL_FAILURE(ActivateFSAndRecover()); ASSERT_EQ("v0", Get(0, Key(0))); ASSERT_EQ("v0b", Get(0, Key(1))); diff --git a/include/rocksdb/options.h b/include/rocksdb/options.h index 34d9ca7bfa..ebfc4f7d8e 100644 --- a/include/rocksdb/options.h +++ b/include/rocksdb/options.h @@ -1749,7 +1749,10 @@ struct DBOptions { // Error happens, SetBGError is called to deal with the error. If the error // can be auto-recovered (e.g., retryable IO Error during Flush or WAL write), // then db resume is called in background to recover from the error. If this - // value is 0 or negative, DB::Resume() will not be called automatically. + // value is 0 or negative, DB::Resume() will not be called automatically, and + // any background retryable IO error (including one during compaction, which + // would otherwise be retried by rescheduling the compaction) stops writes + // until the user calls DB::Resume(). // // Default: INT_MAX int max_bgerror_resume_count = INT_MAX; diff --git a/unreleased_history/behavior_changes/no_auto_resume_stops_writes.md b/unreleased_history/behavior_changes/no_auto_resume_stops_writes.md new file mode 100644 index 0000000000..5a237bc97b --- /dev/null +++ b/unreleased_history/behavior_changes/no_auto_resume_stops_writes.md @@ -0,0 +1 @@ +With `max_bgerror_resume_count <= 0` (auto resume disabled), a retryable or file-scoped IO error during compaction or flush is now a hard background error that stops writes until `DB::Resume()` is called. Previously, compaction errors were silently retried, and flush errors with a healthy WAL were soft errors that let writes continue while flushes were stopped.