3d99378791
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 |
||
|---|---|---|
| .. | ||
| clipping_iterator.h | ||
| clipping_iterator_test.cc | ||
| compaction.cc | ||
| compaction.h | ||
| compaction_iteration_stats.h | ||
| compaction_iterator.cc | ||
| compaction_iterator.h | ||
| compaction_iterator_test.cc | ||
| compaction_job.cc | ||
| compaction_job.h | ||
| compaction_job_stats_test.cc | ||
| compaction_job_test.cc | ||
| compaction_outputs.cc | ||
| compaction_outputs.h | ||
| compaction_picker.cc | ||
| compaction_picker.h | ||
| compaction_picker_fifo.cc | ||
| compaction_picker_fifo.h | ||
| compaction_picker_level.cc | ||
| compaction_picker_level.h | ||
| compaction_picker_test.cc | ||
| compaction_picker_universal.cc | ||
| compaction_picker_universal.h | ||
| compaction_service_job.cc | ||
| compaction_service_test.cc | ||
| compaction_state.cc | ||
| compaction_state.h | ||
| file_pri.h | ||
| sst_partitioner.cc | ||
| subcompaction_state.cc | ||
| subcompaction_state.h | ||
| tiered_compaction_test.cc | ||