Conversation
Rework the TransactionAction trait per the stateful transaction RFC (milestone 1): each action declares an associated retry-persistent State type created via new_state(), commit() borrows the action immutably and its state mutably per attempt, and a consuming cleanup() plus CommitStatus define terminal-cleanup semantics (not yet invoked; wired in a later milestone). Transactions now store one entry per action, pairing immutable intent with exclusively-owned execution state behind an object-safe ErasedActionEntry adapter. Cloning a transaction creates a new execution of the action plan: actions are duplicated and paired with fresh state, so retry state is never shared between executions. All existing actions migrate with State = () and no-op cleanup.
CTTY
marked this pull request as draft
September 22, 2026 21:52
4 tasks
CTTY
marked this pull request as ready for review
September 22, 2026 22:19
JanKaul
reviewed
Sep 23, 2026
JanKaul
left a comment
JanKaul
left a comment
Collaborator
There was a problem hiding this comment.
I left a minor comment. Generally, looks good to me.
| /// The pairing is preserved by construction: the entry is created with fresh | ||
| /// state and owns both exclusively, so terminal cleanup can consume them together. | ||
| pub(crate) struct ActionEntry<A: TransactionAction> { | ||
| action: Box<A>, |
Collaborator
There was a problem hiding this comment.
Is the inner Box<A> here necessary? Since the entry is already stored as Box<dyn ErasedActionEntry>, this looks like a second nested allocation — and as far as I can tell it only exists to satisfy the self: Box<Self> receiver on TransactionAction::cleanup.
Given that TransactionAction is never used as a trait object (erasure happens at the ErasedActionEntry layer), could cleanup take self by value instead of Box<Self>? That would let ActionEntry hold action: A directly and drop the extra allocation:
async fn cleanup(self, state: Self::State, table: &Table, status: CommitStatus);
Or was the Box<Self> receiver a deliberate choice for something later in the RFC?
This branch has not been deployed
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
2 participants
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.
Which issue does this PR close?
Part of epic #3266 (milestone 1 of RFC-0003, introduced in #2620).
What changes are included in this PR?
TransactionActiontrait:Clone + Send + Sync + 'staticsupertraits, associatedStatetype,new_state(), per-attemptcommit(&self, &mut State, &Table), and a consumingcleanup(Box<Self>, State, &Table, CommitStatus)(defined but not yet invoked; wiring lands with milestone 3).CommitStatus(Committed/Failed/Unknown) carrying the RFC §6 terminal-cleanup semantics.ActionEntry<A>pairs action intent with its state by construction, erased behind the object-safeErasedActionEntryadapter;TransactionstoresVec<TransactionActionEntry>.CloneforTransactionviafresh_clone(): clones duplicate intent and create fresh state, never sharing retry state between executions.State = ()and explicit no-op cleanup; no public API changes other than the actions now implementingClone.Are these changes tested?
Covered by the existing transaction test suite (updated for the new trait shape), plus a new test asserting that cloning a transaction produces independent entries. Full
iceberglib suite passes.AI Disclosure
Used Claude for the development