fix: enforce pending update lifecycle - #868
Open
zhjwpku wants to merge 1 commit into
Open
Conversation
Pending updates need deterministic ownership and finalization across both standalone commits and explicit transactions. Register updates through shared ownership so transactions can retain and finalize them safely, and scope the temporary transaction binding used by standalone commits so it never escapes through TransactionContext. Finalize no-op commits and failed applies so every update reaches a terminal state and staged files are cleaned even when the caller never commits the transaction. During Transaction::Commit retries, defer eager finalization while updates are being reapplied; otherwise a retryable validation error would finalize the transaction and destroy staged state before RetryRunner can retry. Restore the retry lifecycle marker with RAII when commit exits or throws. Convert snapshot update factories and test helpers to shared_ptr to satisfy the ownership contract. Tests cover detached temporary transactions, cleared standalone bindings, rejection of unshared standalone updates, no-op and apply-failure finalization, staged-file cleanup, standalone retry reapplication, and lifecycle restoration after commit exceptions.
zhjwpku
force-pushed
the
fix/pending-update-lifecycle
branch
from
August 30, 2026 14:53
3f61d7f to
9983e1a
Compare
There was a problem hiding this comment.
Pull request overview
This PR tightens the lifecycle semantics for PendingUpdate/Transaction so updates have deterministic ownership and always reach a terminal finalized state across both standalone commits and explicit transactions, including transaction commit retries.
Changes:
- Switch several update factories to return
std::shared_ptr<...>and makePendingUpdateenable_shared_from_thisso transactions can retain/finalize updates safely. - Ensure updates are finalized exactly once (including no-op commits and apply failures) and prevent reuse by introducing
finalized_plus a centralizedTransaction::FinalizeUpdates(...). - Add a commit-retry lifecycle marker (
committing_) guarded by RAII to avoid premature finalization during retry reapplication; expand tests to cover these paths.
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/iceberg/update/row_delta.h | Factory now returns shared_ptr for shared ownership. |
| src/iceberg/update/row_delta.cc | Factory implementation updated to shared_ptr. |
| src/iceberg/update/rewrite_files.h | Factory now returns shared_ptr; doc updated. |
| src/iceberg/update/rewrite_files.cc | Factory implementation updated to shared_ptr. |
| src/iceberg/update/replace_partitions.h | Factory now returns shared_ptr. |
| src/iceberg/update/replace_partitions.cc | Factory implementation updated to shared_ptr. |
| src/iceberg/update/merge_append.h | Factory now returns shared_ptr. |
| src/iceberg/update/merge_append.cc | Factory implementation updated to shared_ptr. |
| src/iceberg/update/fast_append.h | Factory now returns shared_ptr. |
| src/iceberg/update/fast_append.cc | Factory implementation updated to shared_ptr. |
| src/iceberg/update/delete_files.h | Factory now returns shared_ptr. |
| src/iceberg/update/delete_files.cc | Factory implementation updated to shared_ptr. |
| src/iceberg/update/pending_update.h | Require shared ownership for commit; enable shared_from_this. |
| src/iceberg/update/pending_update.cc | Scoped temporary transaction binding; eager finalization on apply failures outside commit retries. |
| src/iceberg/transaction.h | Add FinalizeUpdates, finalized_, and committing_ to support deterministic finalization and retry safety. |
| src/iceberg/transaction.cc | Centralize finalization; detach temporary transactions; RAII retry lifecycle marker. |
| src/iceberg/test/transaction_test.cc | New tests for standalone bindings, shared-ownership enforcement, no-op commit finalization, apply-failure finalization, retry reapplication, and exception lifecycle restoration. |
| src/iceberg/test/replace_partitions_test.cc | Update helper return type to shared_ptr. |
| src/iceberg/test/merging_snapshot_update_test.cc | Update test-only update factories/return types to shared_ptr. |
| src/iceberg/test/fast_append_test.cc | New test asserting staged-file cleanup on transaction apply failure. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
52
to
54
| if (!ctx_->transaction) { | ||
| // Table-created path: no transaction exists yet, create a temporary one. | ||
| ICEBERG_ASSIGN_OR_RAISE(auto txn, Transaction::Make(ctx_)); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pending updates need deterministic ownership and finalization across both standalone commits and explicit transactions. Register updates through shared ownership so transactions can retain and finalize them safely, and scope the temporary transaction binding used by standalone commits so it never escapes through TransactionContext.
Finalize no-op commits and failed applies so every update reaches a terminal state and staged files are cleaned even when the caller never commits the transaction. During Transaction::Commit retries, defer eager finalization while updates are being reapplied; otherwise a retryable validation error would finalize the transaction and destroy staged state before RetryRunner can retry. Restore the retry lifecycle marker with RAII when commit exits or throws.
Tests cover detached temporary transactions, cleared standalone bindings, rejection of unshared standalone updates, no-op and apply-failure finalization, staged-file cleanup, standalone retry reapplication, and lifecycle restoration after commit exceptions.