From 7e14ea5ac8b7bd2c177a384c299a3aa8395d9b41 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=81ukasz=20Paszkowski?= Date: Thu, 23 Apr 2026 12:59:05 +0200 Subject: [PATCH] sstables: only wipe TemporaryHashes for sstable formats that have it Commit 8d34127684 ("sstables: clean up TemporaryHashes file in wipe()") unconditionally calls filename(..., component_type::TemporaryHashes) inside filesystem_storage::wipe(). However, the TemporaryHashes component is only registered in the component map of the 'ms' sstable format. For older formats (ka, la, mc, md, me) the lookup goes through sstable_version_constants::get_component_map(version).at(...) and throws std::out_of_range. The exception is then swallowed by the outer catch(...) in wipe(), which just logs and ignores. As a side effect, the subsequent remove_file(new_toc_name) is never reached and the TemporaryTOC ('*-TOC.txt.tmp') file is left as an orphan on disk after every unlink() of a non-'ms' sstable. Guard the lookup with get_component_map(version).contains() so the cleanup is only attempted for formats that actually define the component. Add a regression test in test/boost/sstable_directory_test.cc that creates an 'me'-format sstable, unlinks it and asserts that the sstable directory is left empty. Without the fix the test fails with a leftover 'me-...-TOC.txt.tmp' file. Fixes: https://scylladb.atlassian.net/browse/SCYLLADB-1697 Closes scylladb/scylladb#29620 --- sstables/storage.cc | 11 ++++++++--- test/boost/sstable_directory_test.cc | 27 +++++++++++++++++++++++++++ 2 files changed, 35 insertions(+), 3 deletions(-) diff --git a/sstables/storage.cc b/sstables/storage.cc index ee95236f56..4f7f49c4bf 100644 --- a/sstables/storage.cc +++ b/sstables/storage.cc @@ -543,11 +543,16 @@ future<> filesystem_storage::wipe(const sstable& sst, sync_dir sync) noexcept { // during SSTable writing and removed before sealing. If the write // failed before sealing, the file may still be on disk and must be // cleaned up explicitly. + // The component is only defined for the `ms` sstable format; for + // older formats it is absent from the component map and looking up + // its filename would throw std::out_of_range. // Use file_exists() to avoid a C++ exception on the common path // where the file was already removed before sealing. - auto temp_hashes = filename(sst, dir_name.native(), sst._generation, component_type::TemporaryHashes); - if (co_await file_exists(temp_hashes)) { - co_await sst.sstable_write_io_check(remove_file, std::move(temp_hashes)); + if (sstable_version_constants::get_component_map(sst.get_version()).contains(component_type::TemporaryHashes)) { + auto temp_hashes = filename(sst, dir_name.native(), sst._generation, component_type::TemporaryHashes); + if (co_await file_exists(temp_hashes)) { + co_await sst.sstable_write_io_check(remove_file, std::move(temp_hashes)); + } } if (sync) { co_await sst.sstable_write_io_check(sync_directory, dir_name.native()); diff --git a/test/boost/sstable_directory_test.cc b/test/boost/sstable_directory_test.cc index b8e85c1d9c..98faeb68b1 100644 --- a/test/boost/sstable_directory_test.cc +++ b/test/boost/sstable_directory_test.cc @@ -246,6 +246,33 @@ SEASTAR_TEST_CASE(sstable_directory_test_table_extra_temporary_toc) { }); } +// Reproducer for SCYLLADB-1697 +SEASTAR_TEST_CASE(sstable_directory_test_unlink_sstable_leaves_no_orphans) { + return sstables::test_env::do_with_async([] (test_env& env) { + for (const auto version : {sstable_version_types::me, sstable_version_types::ms}) { + testlog.info("Testing sstable version: {}", version); + auto sst = make_sstable_for_this_shard([&env, version] { + return env.make_sstable(test_table_schema(), version); + }); + + // Sanity: the TOC was written, otherwise the assertion below would be vacuous. + BOOST_REQUIRE(file_exists(test(sst).filename(sstables::component_type::TOC).native()).get()); + + sst->unlink().get(); + + std::vector remaining; + lister::scan_dir(env.tempdir().path(), lister::dir_entry_types::of(), + [&remaining] (fs::path, directory_entry de) { + remaining.push_back(de.name); + return make_ready_future<>(); + }).get(); + + BOOST_REQUIRE_MESSAGE(remaining.empty(), + fmt::format("Expected empty sstable dir after unlink for version {}, found: {}", version, remaining)); + } + }); +} + // Test the absence of TOC. Behavior is controllable by a flag SEASTAR_TEST_CASE(sstable_directory_test_table_missing_toc) { return sstables::test_env::do_with_async([] (test_env& env) {