Skip to content

C# implementation of Transactions for Procedures#3809

Merged
rekhoff merged 36 commits intomasterfrom
rekhoff/csharp-procedure-transactions
Dec 18, 2025
Merged

C# implementation of Transactions for Procedures#3809
rekhoff merged 36 commits intomasterfrom
rekhoff/csharp-procedure-transactions

Conversation

@rekhoff
Copy link
Contributor

@rekhoff rekhoff commented Dec 2, 2025

Description of Changes

Implements the C# equivalent of #3638

This implement uses inheritance, where abstract base classes (like ProcedureContextBase in ProcedureContext.cs) store the core of the implementation, and then generated wrappers (like ProcedureContext in the generated FFI.cs file) inherit from them.

For error handling, we work like Rust's implementation of Result<T,E> but we require where E : Exception because of how exceptions work in C#. Transaction-level failures come back as a TxOutcome and user errors should follow the Result<T,E> pattern. In this implementation, we have UnwrapOrThrow() throws exceptions directly because of C#'s error handling pattern.

Unlike the Rust implementation's direct Result propagation, we are using an AbortGuard pattern (in ProcedureContext.cs) for exception handling, which uses IDisposable for automatic cleanup.

Most changes should have fairly similar Rust-equivalents beyond that.
For module authors, the changes here allow for the transation logic to work like:

ctx.TryWithTx<ResultType, Exception>(tx => {
    // transaction logic
    return Result<ResultType, Exception>.Ok(result);
});

This change includes a number of tests added to the sdks/csharp/examples~/regression-tests/'s server and client to validate the behavior of the changes. server changes provide further usage examples for module authors.

API and ABI breaking changes

Should not be a breaking change

Expected complexity level and risk

2

Testing

  • Created Regression Tests that show transitions in procedures working in various ways, all of which pass.

@rekhoff rekhoff self-assigned this Dec 2, 2025
@rekhoff rekhoff marked this pull request as ready for review December 14, 2025 05:49
Copy link
Contributor

@gefjon gefjon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to see a more in-depth PR description which goes into detail on your design and implementation choices. Especially in big PRs like this, there's a burden on the author to both keep the changes as contained and laser-focused as possible, and also to proactively explain to reviewers what's going on in the diff. Anywhere I've left a comment asking for reasoning, that's the kind of thing that should be in your PR description proactively.

@bfops bfops added the release-any To be landed in any release window label Dec 15, 2025
Copy link
Contributor

@JasonAtClockwork JasonAtClockwork left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks solid now, I've run the regression tests and walked through the changes. One small nit would be to add Log.Debug() for all successful tests but I think we want to rework the test runner soon anyhow.

@rekhoff rekhoff requested a review from gefjon December 17, 2025 21:40
Copy link
Contributor

@gefjon gefjon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great, thanks!

@rekhoff rekhoff added this pull request to the merge queue Dec 18, 2025
Merged via the queue into master with commit 39f0128 Dec 18, 2025
26 of 30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release-any To be landed in any release window

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants