From 4a7cf6ee78041572b24a24ffda8ac3745e1bda43 Mon Sep 17 00:00:00 2001 From: Bharath Vissapragada Date: Sat, 29 Aug 2026 22:55:37 -0700 Subject: [PATCH] fix(transaction): re-apply table-created updates on commit retry PendingUpdate::Commit's table-created path (Table::NewFastAppend, NewDeleteFiles, NewOverwrite, ...) applied the update to the temporary transaction's metadata builder but never registered it in pending_updates_. When the commit lost the CAS race and the retry runner re-entered CommitOnce, the builder was rebuilt from the refreshed metadata and the re-apply loop iterated an empty list, so the retry posted an UpdateTable request with no changes, guarded only by assert-table-uuid. The catalog accepted it and the commit reported success while the update was silently dropped - an acked append or delete that vanished whenever it raced another writer. Register the update with the transaction before applying it, so a retry re-applies it onto the refreshed base like transaction-created updates. The transaction does not outlive the call, so the non-owning handle is safe. With the update registered, the transaction finalizes it, so the explicit post-commit Finalize calls in the table-created branch are dropped - keeping them would repeat cleanup and file-deletion callbacks. Transaction::Commit's empty-changes early return now finalizes registered updates so that path stays covered. --- src/iceberg/test/transaction_test.cc | 44 ++++++++++++++++++++++++++++ src/iceberg/transaction.cc | 3 ++ src/iceberg/update/pending_update.cc | 11 +++++-- 3 files changed, 55 insertions(+), 3 deletions(-) diff --git a/src/iceberg/test/transaction_test.cc b/src/iceberg/test/transaction_test.cc index 3a13b7bc5..dbc0db0c9 100644 --- a/src/iceberg/test/transaction_test.cc +++ b/src/iceberg/test/transaction_test.cc @@ -151,6 +151,50 @@ TEST_F(TransactionRetryTest, CommitRetrySucceedsAfterConflict) { EXPECT_EQ(update_call_count, 2); } +TEST_F(TransactionRetryTest, TableCreatedUpdateIsReappliedOnRetry) { + // A concurrent commit moves the metadata location, so the retry's Refresh() swaps + // metadata and CommitOnce rebuilds the builder by re-applying registered updates; + // with an unchanged location the rebuild never runs and this regression cannot fire. + ON_CALL(*mock_catalog_, LoadTable(::testing::_)) + .WillByDefault([this](const TableIdentifier&) -> Result> { + // A distinct metadata object at a new location: Refresh() must actually swap + // metadata, or CommitOnce keeps the stale builder and never re-applies updates. + return Table::Make(table_->name(), + std::make_shared(*table_->metadata()), + std::string(table_->metadata_file_location()) + "-refreshed", + table_->io(), mock_catalog_); + }); + + int update_call_count = 0; + size_t second_attempt_update_count = 0; + ON_CALL(*mock_catalog_, UpdateTable(::testing::_, ::testing::_, ::testing::_)) + .WillByDefault([this, &update_call_count, &second_attempt_update_count]( + const TableIdentifier&, + const std::vector>&, + const std::vector>& updates) + -> Result> { + ++update_call_count; + if (update_call_count == 1) { + return CommitFailed("conflict on first attempt"); + } + second_attempt_update_count = updates.size(); + return Table::Make(mock_table_->name(), mock_table_->metadata(), + std::string(mock_table_->metadata_file_location()), + mock_table_->io(), mock_catalog_); + }); + + // Created from the table, not from a transaction: the temporary transaction inside + // PendingUpdate::Commit must re-apply this update when the first attempt conflicts, + // instead of committing an empty change set and reporting success. + ICEBERG_UNWRAP_OR_FAIL(auto update, mock_table_->NewUpdateProperties()); + update->Set("retry.test", "value"); + EXPECT_THAT(update->Commit(), IsOk()); + + EXPECT_EQ(update_call_count, 2); + EXPECT_GT(second_attempt_update_count, 0) + << "retry posted an empty change set: the pending update was dropped"; +} + TEST_F(TransactionRetryTest, CommitRetryExhausted) { int update_call_count = 0; ON_CALL(*mock_catalog_, UpdateTable(::testing::_, ::testing::_, ::testing::_)) diff --git a/src/iceberg/transaction.cc b/src/iceberg/transaction.cc index 7abce27cb..7331b96bd 100644 --- a/src/iceberg/transaction.cc +++ b/src/iceberg/transaction.cc @@ -358,6 +358,9 @@ Result> Transaction::Commit() { const auto& updates = ctx_->metadata_builder->changes(); if (updates.empty()) { + for (const auto& update : pending_updates_) { + std::ignore = update->Finalize(ctx_->table->metadata().get()); + } committed_ = true; return ctx_->table; } diff --git a/src/iceberg/update/pending_update.cc b/src/iceberg/update/pending_update.cc index 4b3000652..4cb6632e1 100644 --- a/src/iceberg/update/pending_update.cc +++ b/src/iceberg/update/pending_update.cc @@ -35,19 +35,24 @@ Status PendingUpdate::Commit() { if (!ctx_->transaction) { // Table-created path: no transaction exists yet, create a temporary one. ICEBERG_ASSIGN_OR_RAISE(auto txn, Transaction::Make(ctx_)); + // Register with the transaction so a commit retry re-applies this update onto the + // refreshed metadata; without it a retry rebuilds the metadata with no changes and + // reports success while silently dropping the update. The transaction does not + // outlive this call, so the non-owning handle is safe. + ICEBERG_RETURN_UNEXPECTED( + txn->AddUpdate(std::shared_ptr(this, [](PendingUpdate*) {}))); auto apply_status = txn->Apply(*this); if (!apply_status.has_value()) { std::ignore = Finalize(std::unexpected(apply_status.error())); return apply_status; } + // The transaction finalizes its registered updates, this one included; finalizing + // here as well would repeat cleanup and file-deletion callbacks. auto commit_result = txn->Commit(); if (!commit_result.has_value()) { - std::ignore = Finalize(std::unexpected(commit_result.error())); return std::unexpected(commit_result.error()); } - - std::ignore = Finalize(commit_result.value()->metadata().get()); return {}; } auto txn = ctx_->transaction->lock();