From ecefc1014dede9020376f6eeb140779e206850a2 Mon Sep 17 00:00:00 2001 From: yangjie01 Date: Thu, 1 Oct 2026 18:54:31 +0800 Subject: [PATCH 1/2] fix(catalog): validate namespace existence in InMemoryCatalog create and register CreateTable wrote the table metadata file before checking the namespace, leaking an orphaned metadata file on object-store FileIO and returning a misleading kIOError on local FileIO. RegisterTable's if (!NamespaceExists(...)) tested the Result's has_value() rather than the contained bool, so the missing-namespace branch was dead code and the error surfaced as kUnknownError. Both paths now unwrap NamespaceExists and return kNoSuchNamespace before any metadata I/O: CreateTable before the write, RegisterTable before the read, so a register into a missing namespace fails fast without a wasted (and possibly failing) metadata read. --- .../catalog/memory/in_memory_catalog.cc | 20 +++++++-- src/iceberg/test/in_memory_catalog_test.cc | 43 +++++++++++++++++++ 2 files changed, 60 insertions(+), 3 deletions(-) diff --git a/src/iceberg/catalog/memory/in_memory_catalog.cc b/src/iceberg/catalog/memory/in_memory_catalog.cc index d0797e728..112d5aba5 100644 --- a/src/iceberg/catalog/memory/in_memory_catalog.cc +++ b/src/iceberg/catalog/memory/in_memory_catalog.cc @@ -422,6 +422,11 @@ Result> InMemoryCatalog::CreateTable( const std::string& location, const std::unordered_map& properties) { std::unique_lock lock(mutex_); + ICEBERG_ASSIGN_OR_RAISE(auto namespace_exists, + root_namespace_->NamespaceExists(identifier.ns)); + if (!namespace_exists) { + return NoSuchNamespace("Table namespace does not exist: {}", identifier.ns); + } if (root_namespace_->TableExists(identifier).value_or(false)) { return AlreadyExists("Table already exists: {}", identifier); } @@ -605,13 +610,22 @@ Result> InMemoryCatalog::RegisterTable( return InvalidArgument("file_io is not set for catalog {}", catalog_name_); } + // Validate the namespace up front so a register into a missing namespace fails + // fast, before any metadata read. The read then runs outside the lock, so the + // catalog mutex is not held during (possibly remote) metadata I/O. + { + std::unique_lock lock(mutex_); + ICEBERG_ASSIGN_OR_RAISE(auto namespace_exists, + root_namespace_->NamespaceExists(identifier.ns)); + if (!namespace_exists) { + return NoSuchNamespace("Table namespace does not exist: {}", identifier.ns); + } + } + ICEBERG_ASSIGN_OR_RAISE(auto metadata, TableMetadataUtil::Read(*file_io_, metadata_file_location)); std::unique_lock lock(mutex_); - if (!root_namespace_->NamespaceExists(identifier.ns)) { - return NoSuchNamespace("Table namespace does not exist: {}", identifier.ns); - } if (!root_namespace_->RegisterTable(identifier, metadata_file_location)) { return UnknownError("The registry failed."); } diff --git a/src/iceberg/test/in_memory_catalog_test.cc b/src/iceberg/test/in_memory_catalog_test.cc index b2c88f571..9e7261a52 100644 --- a/src/iceberg/test/in_memory_catalog_test.cc +++ b/src/iceberg/test/in_memory_catalog_test.cc @@ -158,6 +158,49 @@ TEST_F(InMemoryCatalogTest, RegisterTable) { ASSERT_EQ(table.value()->location(), "s3://bucket/test/location"); } +TEST_F(InMemoryCatalogTest, CreateTableNonexistentNamespace) { + TableIdentifier table_ident{.ns = Namespace{.levels = {"missing"}}, .name = "t1"}; + auto schema = std::make_shared( + std::vector{SchemaField::MakeRequired(1, "id", int64())}, + /*schema_id=*/1); + auto spec = PartitionSpec::Unpartitioned(); + auto sort_order = SortOrder::Unsorted(); + + // Use an explicit location whose metadata directory already exists (the local + // FileIO does not create parent dirs). A write-before-validate bug would land a + // detectable orphan there. GenerateTestTableLocation is unique per test and + // auto-cleaned via created_temp_paths_. + auto table_location = GenerateTestTableLocation(table_ident.name); + + auto table = + catalog_->CreateTable(table_ident, schema, spec, sort_order, table_location, {}); + EXPECT_THAT(table, IsError(ErrorKind::kNoSuchNamespace)); + + // The namespace check must run before any metadata file is written, so a + // failed create must leave no orphaned metadata file behind. + std::error_code ec; + size_t metadata_files = 0; + for (auto it = std::filesystem::recursive_directory_iterator(table_location, ec); + it != std::filesystem::recursive_directory_iterator(); it.increment(ec)) { + if (it->path().extension() == ".json") { + ++metadata_files; + } + } + EXPECT_EQ(metadata_files, 0); +} + +TEST_F(InMemoryCatalogTest, RegisterTableNonexistentNamespace) { + TableIdentifier table_ident{.ns = Namespace{.levels = {"missing"}}, .name = "t1"}; + + // The namespace is validated before the metadata is read, so registering into a + // missing namespace reports NoSuchNamespace rather than a read error, even when the + // metadata location is unreadable. (NamespaceExists returns a Result carrying + // false, not an error, so the Result must be unwrapped before it is tested.) + auto table = + catalog_->RegisterTable(table_ident, "/nonexistent/iceberg/v1.metadata.json"); + EXPECT_THAT(table, IsError(ErrorKind::kNoSuchNamespace)); +} + TEST_F(InMemoryCatalogTest, RefreshTable) { TableIdentifier table_ident{.ns = {}, .name = "t1"}; auto schema = std::make_shared( From 9b4f2a6b0af1847aeb580737a5bfb760c76b0419 Mon Sep 17 00:00:00 2001 From: yangjie01 Date: Fri, 2 Oct 2026 01:57:59 +0800 Subject: [PATCH 2/2] fix(catalog): keep RegisterTable namespace check and registration atomic Read the metadata outside the lock, then validate the namespace and register under a single lock, rather than splitting validation and registration into two critical sections. This keeps the check and insert atomic, so a concurrent namespace drop cannot slip between them, and still never holds the catalog mutex during metadata I/O. --- src/iceberg/catalog/memory/in_memory_catalog.cc | 17 +++++------------ src/iceberg/test/in_memory_catalog_test.cc | 16 ++++++++++------ 2 files changed, 15 insertions(+), 18 deletions(-) diff --git a/src/iceberg/catalog/memory/in_memory_catalog.cc b/src/iceberg/catalog/memory/in_memory_catalog.cc index 112d5aba5..86d832388 100644 --- a/src/iceberg/catalog/memory/in_memory_catalog.cc +++ b/src/iceberg/catalog/memory/in_memory_catalog.cc @@ -610,22 +610,15 @@ Result> InMemoryCatalog::RegisterTable( return InvalidArgument("file_io is not set for catalog {}", catalog_name_); } - // Validate the namespace up front so a register into a missing namespace fails - // fast, before any metadata read. The read then runs outside the lock, so the - // catalog mutex is not held during (possibly remote) metadata I/O. - { - std::unique_lock lock(mutex_); - ICEBERG_ASSIGN_OR_RAISE(auto namespace_exists, - root_namespace_->NamespaceExists(identifier.ns)); - if (!namespace_exists) { - return NoSuchNamespace("Table namespace does not exist: {}", identifier.ns); - } - } - ICEBERG_ASSIGN_OR_RAISE(auto metadata, TableMetadataUtil::Read(*file_io_, metadata_file_location)); std::unique_lock lock(mutex_); + ICEBERG_ASSIGN_OR_RAISE(auto namespace_exists, + root_namespace_->NamespaceExists(identifier.ns)); + if (!namespace_exists) { + return NoSuchNamespace("Table namespace does not exist: {}", identifier.ns); + } if (!root_namespace_->RegisterTable(identifier, metadata_file_location)) { return UnknownError("The registry failed."); } diff --git a/src/iceberg/test/in_memory_catalog_test.cc b/src/iceberg/test/in_memory_catalog_test.cc index 9e7261a52..db90aa14a 100644 --- a/src/iceberg/test/in_memory_catalog_test.cc +++ b/src/iceberg/test/in_memory_catalog_test.cc @@ -192,12 +192,16 @@ TEST_F(InMemoryCatalogTest, CreateTableNonexistentNamespace) { TEST_F(InMemoryCatalogTest, RegisterTableNonexistentNamespace) { TableIdentifier table_ident{.ns = Namespace{.levels = {"missing"}}, .name = "t1"}; - // The namespace is validated before the metadata is read, so registering into a - // missing namespace reports NoSuchNamespace rather than a read error, even when the - // metadata location is unreadable. (NamespaceExists returns a Result carrying - // false, not an error, so the Result must be unwrapped before it is tested.) - auto table = - catalog_->RegisterTable(table_ident, "/nonexistent/iceberg/v1.metadata.json"); + ICEBERG_UNWRAP_OR_FAIL(auto metadata, + ReadTableMetadataFromResource("TableMetadataV2Valid.json")); + auto table_location = GenerateTestTableLocation(table_ident.name); + auto metadata_location = std::format("{}v1.metadata.json", table_location); + ASSERT_THAT(TableMetadataUtil::Write(*file_io_, metadata_location, *metadata), IsOk()); + + // Registering into a nonexistent namespace reports NoSuchNamespace. The dead + // if (!NamespaceExists(...)) check tested the Result's has_value() rather + // than the contained bool, so this previously fell through to kUnknownError. + auto table = catalog_->RegisterTable(table_ident, metadata_location); EXPECT_THAT(table, IsError(ErrorKind::kNoSuchNamespace)); }