mirror of
https://github.com/facebook/rocksdb.git
synced 2026-10-23 08:09:02 +03:00
Summary: Fix leaked table cache entries that cause `TEST_VerifyNoObsoleteFilesCached` assertion failure during `DB::Close()` in ASAN crash test builds (T258745630): ``` File 126519 is not live nor quarantined Assertion `cached_file_is_live_or_quar' failed. ``` When a compaction fails *after* `VerifyOutputFiles` succeeds (e.g., at `VerifyCompactionRecordCounts`), the overall `compact_->status` is set to error but each subcompaction's individual `status` remains OK. `SubcompactionState::Cleanup` only checked the individual subcompaction status, so it skipped calling `ReleaseObsolete` on the output files' table cache entries — leaking them. Normally, `Close()`'s backstop (`FindObsoleteFiles(force=true)` + `PurgeObsoleteFiles`) would catch this by finding the orphan file on disk, evicting the cache entry, and deleting the file. However, `FindObsoleteFiles` calls `FaultInjectionTestFS::GetChildren` which can fail under metadata read fault injection (`--open_metadata_read_fault_one_in=8`). The error is silently ignored (`s.PermitUncheckedError()` in db_impl_files.cc:199), so the orphan file is never found and the leaked cache entry is never evicted. The `Cleanup` bug is latent and predates recent changes. PR https://github.com/facebook/rocksdb/issues/14433 added `verify_output_flags` randomization to the crash test, and PR https://github.com/facebook/rocksdb/issues/14456 fixed false-positive corruptions that https://github.com/facebook/rocksdb/issues/14433 caused. Before https://github.com/facebook/rocksdb/issues/14456, `VerifyOutputFiles` would produce false corruption errors that *accidentally prevented the leak* by setting the subcompaction status to non-OK. ### How it triggers **Step 1 — VerifyOutputFiles adds cache entries for compaction output files:** ``` CompactionJob::Run() RunSubcompactions() // all subcompactions succeed // output file 12 written to disk // each sub_compact.status = OK SyncOutputDirectories() // status = OK VerifyOutputFiles() for each output_file: table_cache()->NewIterator(output_file.meta) FindTable() cache->Insert(file_number=12) // <<< ENTRY ADDED TO TABLE CACHE return iterator holding handle delete iter // releases handle, entry stays in LRU // status = OK ``` **Step 2 — A post-verification step fails, overall status set but NOT subcompaction status:** ``` CompactionJob::Run() VerifyCompactionRecordCounts() // returns Status::Corruption(...) FinalizeCompactionRun(status=Corruption) compact_->status = Corruption // <<< OVERALL status = error // each sub_compact.status is still OK! ``` **Step 3 — Install skips InstallCompactionResults (file 12 never enters a Version):** ``` CompactionJob::Install() status = compact_->status // Corruption if (status.ok()) // FALSE InstallCompactionResults() // <<< SKIPPED — file 12 not in any version ``` **Step 4 — CleanupCompaction skips ReleaseObsolete (THE BUG):** ``` CompactionJob::CleanupCompaction() for each sub_compact: sub_compact.Cleanup(table_cache) if (!status.ok()) // checks sub_compact.status = OK // <<< FALSE — individual status is OK! ReleaseObsolete(...) // <<< NEVER CALLED — cache entry leaked! ``` **Step 5 — Metadata read fault prevents backstop from finding the orphan:** `FindObsoleteFiles(force=true)` calls `GetChildren()` to scan the DB directory. `FaultInjectionTestFS::GetChildren` injects a metadata read error (`--open_metadata_read_fault_one_in=8`). The error is silently ignored (`s.PermitUncheckedError()`), so the directory listing is empty and the orphan file is never found. This happens both in the post-compaction cleanup (`BackgroundCallCompaction`) and in `Close()`'s backstop: ``` FindObsoleteFiles(force=true) fs->GetChildren(path, ...) // returns IOError (fault injected) s.PermitUncheckedError(); // error silently ignored // files vector is empty — orphan file 12 not found PurgeObsoleteFiles() // nothing to do — no Evict called ``` **Step 6 — Assertion fires during Close():** ``` CloseHelper() FindObsoleteFiles(force=true) // GetChildren fails again → orphan missed PurgeObsoleteFiles() // nothing to evict TEST_VerifyNoObsoleteFilesCached() for each cache entry: file_number = 12 live_and_quar_files.find(12) == end() // NOT in any version! >>> assert(cached_file_is_live_or_quar) FAILS <<< ``` **Crash test call stack:** ``` frame https://github.com/facebook/rocksdb/issues/9: __assert_fail_base("cached_file_is_live_or_quar", "db_impl_debug.cc", 389) frame https://github.com/facebook/rocksdb/issues/11: DBImpl::TEST_VerifyNoObsoleteFilesCached()::lambda // finds leaked entry frame https://github.com/facebook/rocksdb/issues/18: LRUCacheShard::ApplyToSomeEntries(...) // iterating cache shard frame https://github.com/facebook/rocksdb/issues/19: ShardedCache::ApplyToAllEntries(...) // iterating all shards frame https://github.com/facebook/rocksdb/issues/20: DBImpl::TEST_VerifyNoObsoleteFilesCached() // the verification frame https://github.com/facebook/rocksdb/issues/21: DBImpl::CloseHelper() // during Close frame https://github.com/facebook/rocksdb/issues/22: DBImpl::CloseImpl() frame https://github.com/facebook/rocksdb/issues/23: DBImpl::Close() frame https://github.com/facebook/rocksdb/issues/24: StressTest::Reopen() // crash test reopen frame https://github.com/facebook/rocksdb/issues/25: StressTest::OperateDb() // worker thread ``` ### Fix Pass the overall `compact_->status` to `SubcompactionState::Cleanup` and call `ReleaseObsolete` when *either* the subcompaction status or the overall status is non-OK: ```cpp // Before: if (!status.ok()) { ReleaseObsolete(...); } // After: if (!status.ok() || !overall_status.ok()) { ReleaseObsolete(...); } ``` ## Key changes - **`SubcompactionState::Cleanup`**: Now takes an `overall_status` parameter and calls `ReleaseObsolete` when *either* the subcompaction status or the overall compaction status is non-OK. - **`CompactionJob::CleanupCompaction`**: Passes `compact_->status` (the overall status) to each subcompaction's `Cleanup`. - **Sync point**: Added `CompactionJob::Run():AfterVerifyOutputFiles` for error injection in tests. - **Unit test**: `DBCompactionTest.LeakedTableCacheEntryOnCompactionFailure` uses `FaultInjectionTestFS` to reproduce the crash test scenario — injects error after `VerifyOutputFiles` and deactivates the filesystem so `GetChildren` fails in `FindObsoleteFiles`, preventing the backstop from evicting the leaked cache entry. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14469 Test Plan: - `DBCompactionTest.LeakedTableCacheEntryOnCompactionFailure`: - **Without fix (ASAN)**: assertion fires during `Close()` — `File 12 is not live nor quarantined` - **With fix (ASAN)**: passes — `ReleaseObsolete` properly cleans up the cache entry - **With fix (non-ASAN)**: passes ``` $ COMPILE_WITH_ASAN=1 make -j db_compaction_test $ ./db_compaction_test --gtest_filter="DBCompactionTest.LeakedTableCacheEntryOnCompactionFailure" [ PASSED ] 1 test. ``` Reviewed By: xingbowang Differential Revision: D97190944 Pulled By: joshkang97 fbshipit-source-id: fdfd481cc1e192803cfb7d64052ccb9162c21b94
121 lines
4.3 KiB
C++
121 lines
4.3 KiB
C++
// Copyright (c) Meta Platforms, Inc. and affiliates.
|
|
//
|
|
// This source code is licensed under both the GPLv2 (found in the
|
|
// COPYING file in the root directory) and Apache 2.0 License
|
|
// (found in the LICENSE.Apache file in the root directory).
|
|
//
|
|
// Copyright (c) 2011 The LevelDB Authors. All rights reserved.
|
|
// Use of this source code is governed by a BSD-style license that can be
|
|
// found in the LICENSE file. See the AUTHORS file for names of contributors.
|
|
|
|
#include "db/compaction/subcompaction_state.h"
|
|
|
|
#include "rocksdb/sst_partitioner.h"
|
|
|
|
namespace ROCKSDB_NAMESPACE {
|
|
void SubcompactionState::AggregateCompactionOutputStats(
|
|
InternalStats::CompactionStatsFull& internal_stats) const {
|
|
// Outputs should be closed. By extension, any files created just for
|
|
// range deletes have already been written also.
|
|
assert(compaction_outputs_.HasBuilder() == false);
|
|
assert(proximal_level_outputs_.HasBuilder() == false);
|
|
|
|
// FIXME: These stats currently include abandonned output files
|
|
// assert(compaction_outputs_.stats_.num_output_files ==
|
|
// compaction_outputs_.outputs_.size());
|
|
// assert(proximal_level_outputs_.stats_.num_output_files ==
|
|
// proximal_level_outputs_.outputs_.size());
|
|
|
|
internal_stats.output_level_stats.Add(compaction_outputs_.stats_);
|
|
if (proximal_level_outputs_.HasOutput()) {
|
|
internal_stats.has_proximal_level_output = true;
|
|
internal_stats.proximal_level_stats.Add(proximal_level_outputs_.stats_);
|
|
}
|
|
}
|
|
|
|
OutputIterator SubcompactionState::GetOutputs() const {
|
|
return OutputIterator(proximal_level_outputs_.outputs_,
|
|
compaction_outputs_.outputs_);
|
|
}
|
|
|
|
void SubcompactionState::Cleanup(Cache* cache, const Status& overall_status) {
|
|
proximal_level_outputs_.Cleanup();
|
|
compaction_outputs_.Cleanup();
|
|
|
|
if (!status.ok() || !overall_status.ok()) {
|
|
for (const auto& out : GetOutputs()) {
|
|
// If this file was inserted into the table cache then remove it here
|
|
// because this compaction was not committed. This is not strictly
|
|
// required because of a backstop TableCache::Evict() in
|
|
// PurgeObsoleteFiles() but is our opportunity to apply
|
|
// uncache_aggressiveness. TODO: instead, put these files into the
|
|
// VersionSet::obsolete_files_ pipeline so that they don't have to
|
|
// be picked up by scanning the DB directory.
|
|
TableCache::ReleaseObsolete(
|
|
cache, out.meta.fd.GetNumber(), nullptr /*handle*/,
|
|
compaction->mutable_cf_options().uncache_aggressiveness);
|
|
}
|
|
}
|
|
// TODO: sub_compact.io_status is not checked like status. Not sure if thats
|
|
// intentional. So ignoring the io_status as of now.
|
|
io_status.PermitUncheckedError();
|
|
}
|
|
|
|
Slice SubcompactionState::SmallestUserKey() const {
|
|
if (proximal_level_outputs_.HasOutput()) {
|
|
Slice a = compaction_outputs_.SmallestUserKey();
|
|
Slice b = proximal_level_outputs_.SmallestUserKey();
|
|
if (a.empty()) {
|
|
return b;
|
|
}
|
|
if (b.empty()) {
|
|
return a;
|
|
}
|
|
const Comparator* user_cmp =
|
|
compaction->column_family_data()->user_comparator();
|
|
if (user_cmp->Compare(a, b) > 0) {
|
|
return b;
|
|
} else {
|
|
return a;
|
|
}
|
|
} else {
|
|
return compaction_outputs_.SmallestUserKey();
|
|
}
|
|
}
|
|
|
|
Slice SubcompactionState::LargestUserKey() const {
|
|
if (proximal_level_outputs_.HasOutput()) {
|
|
Slice a = compaction_outputs_.LargestUserKey();
|
|
Slice b = proximal_level_outputs_.LargestUserKey();
|
|
if (a.empty()) {
|
|
return b;
|
|
}
|
|
if (b.empty()) {
|
|
return a;
|
|
}
|
|
const Comparator* user_cmp =
|
|
compaction->column_family_data()->user_comparator();
|
|
if (user_cmp->Compare(a, b) < 0) {
|
|
return b;
|
|
} else {
|
|
return a;
|
|
}
|
|
} else {
|
|
return compaction_outputs_.LargestUserKey();
|
|
}
|
|
}
|
|
|
|
Status SubcompactionState::AddToOutput(
|
|
const CompactionIterator& iter, bool use_proximal_output,
|
|
const CompactionFileOpenFunc& open_file_func,
|
|
const CompactionFileCloseFunc& close_file_func,
|
|
const ParsedInternalKey& prev_iter_output_internal_key) {
|
|
// update target output
|
|
current_outputs_ =
|
|
use_proximal_output ? &proximal_level_outputs_ : &compaction_outputs_;
|
|
return current_outputs_->AddToOutput(iter, open_file_func, close_file_func,
|
|
prev_iter_output_internal_key);
|
|
}
|
|
|
|
} // namespace ROCKSDB_NAMESPACE
|