khepri_machine: propagate process_command/process_query errors from transactions by rgfaber · Pull Request #400 · rabbitmq/khepri

rgfaber · 2026-06-23T03:52:59Z

Problem

The sync transaction paths in khepri_machine wrapped every non-exception result with {ok, Ret} via a catch-all clause. When process_command/3 (read-write) or process_query/3 (read-only) returned an infrastructure error such as {error, noproc} (store not running) or {error, timeout}, the catch-all turned it into {ok, {error, Reason}}.

A caller matching on {ok, Value} then received an error tuple as the transaction result, mistaking a dispatch failure for a successful transaction and crashing downstream.

Fix

Add an explicit {error, _} = Error -> Error clause before the catch-all in:

readwrite_transaction1/3
both readonly_transaction/4 clauses (function and path-pattern)

so infrastructure errors propagate verbatim.

This aligns sync transactions with the convention already used by khepri:handle_async_ret/2, which propagates {error, _} directly and raises only {exception, _, _, _}. tx_ret() already includes {error, Reason} via tx_abort(), so the public return shape is unchanged. tx_ret() is documented accordingly.

Tests

Regression tests for both the read-write and read-only paths, issuing a path-pattern transaction against a store that is never started so the command/query reaches process_command/3 / process_query/3 and returns {error, noproc}. Each test fails without the corresponding {error, _} clause (producing {ok, {error, noproc}}) and passes with it.

Note for reviewers

A transaction body that returns a bare {error, Reason} value (rather than aborting) now surfaces as {error, Reason} instead of {ok, {error, Reason}}. This is consistent with how handle_async_ret/2 already treats asynchronous transactions and with tx_abort(). To return an error-shaped value as a successful result, wrap it explicitly ({ok, {error, Reason}}); this is documented on tx_ret().

…actions When process_command/3 returns {error, Reason} (e.g. {error, noproc} during Ra initialisation), the catch-all clause 'Ret -> {ok, Ret}' was wrapping it as {ok, {error, Reason}}. Callers that pattern-match on {ok, Value} would then silently receive an error tuple as the transaction result, causing crashes or wrong behaviour downstream. Add an explicit {error, _} = Error -> Error clause before the catch-all so infrastructure errors are propagated directly rather than being mistaken for a successful transaction return value.

Add a regression test asserting that a read-write transaction against a store that is not running returns {error, noproc} directly, rather than the wrapped {ok, {error, noproc}} that the catch-all clause previously produced. A path-pattern transaction is used so the command reaches process_command/3 without stand-alone function extraction (which would require the khepri application to be started). The test fails without the {error, _} = Error -> Error clause and passes with it.

…transactions The read-write fix left the same defect in both readonly_transaction/4 clauses. process_query/3 can return an infrastructure error such as {error, noproc} (store not running) or {error, timeout}, but the catch-all 'Ret -> {ok, Ret}' wrapped it as {ok, {error, Reason}}. A caller matching on {ok, Value} would then mistake the error for a successful transaction result. Add an explicit {error, _} = Error -> Error clause to both read-only clauses so dispatch errors propagate verbatim, matching the read-write path and khepri:handle_async_ret/2 (which already returns {error, _} directly and raises only {exception, _, _, _}). tx_ret() already permits {error, Reason} via tx_abort(), so the public shape is unchanged. Document tx_ret() and add a read-only regression test mirroring the existing read-write one against a non-running store.

dumbbell · 2026-09-09T08:24:41Z

Hi!

Sorry for the delay to get back to you.

I still need to think about this. I agree it’s better to return a problem with the Ra server as {error, Reason}, but anything from a successful execution of the transaction should stil be returned as {ok, TxResult}, even if TxResult is an {error, Reason}.

Perhaps I need to revisit the transaction API a bit to make things clearer. Like:

khepri_machine:transaction() always returns whatever the transaction function returns.
If the transation function aborts, it throws an exception (as in erlang:throw()).
If there are any errors from Khepri or Ra, it raises an exception using erlang:error().

Perhaps it would be more intuitive this way: it would be like the transaction function was executed directly and the fact it is passed to khepri_machine:transaction() is transparent.

What do you think?

rgfaber · 2026-09-09T14:04:02Z

No problem on the delay, transactions are tricky business :).

You're right about the side effect, and I agree: a transaction body that returns a bare {error, Reason} did execute, so {ok, {error, Reason}} is the correct answer.

As written, the PR doesn't actually satisfy that. readwrite_transaction1 already distinguishes a thrown or aborted transaction from a plain returned value, that's the {exception, _, _, _} case, and it's handled correctly today.

I think the gap isn't too wide, though: a Ra-level infrastructure failure (do_process_sync_command returning {error, timeout} or {error, noproc} after retries exhaust) comes back as a bare {error, Reason}, with no wrapper that distinguishes it from a value the transaction function might (legitimately) return itself. Both land in the same generic Ret -> {ok, Ret} catch-all. So the distinction needs to be made there, not by adding a pattern in the wrapper.

On the redesign: from the consumer side (reckon-db is an event store on Khepri; we carry {ok, {error, E}} -> {error, E} at three call sites since a startup race handed us {ok, {error, noproc}} and crashed our writers), what we need is exactly the tri-state you describe: the function's value, an abort, or "this did not run/outcome unknown".
That last one is the only case that needs retry logic, and for a timeout the caller also needs to know the command may have been applied.

So: transparent return, throw for abort, erlang:error for Khepri/Ra problems seems the way to go. That's also not new for the abort side, abort/1 already throws today and readwrite_transaction1 already special-cases it.
This would just extend the same distinction to Ra/Khepri failures, which currently fall through the generic catch-all instead of getting one of their own.

Two requests if you go that way:

Give the infrastructure exception a structured reason we can pattern-match on (a record or {khepri, Reason} style term, over a bare atom), so a caller can match noproc vs timeout without catching everything.
Since the transparent return changes what every existing khepri:transaction caller matches on, it probably wants a new entry point, or a major bump.
An intermediate step that keeps {ok, Ret} / {error, Abort} and only turns infrastructure errors into exceptions would already close this bug without touching the success path.

Happy to rework this PR along whichever line you prefer, including a test for the "body returns an error-shaped value" case so it stays {ok, {error, _}}.

Thoughts?

dumbbell · 2026-09-09T15:36:44Z

I think the gap isn't too wide, though: a Ra-level infrastructure failure (do_process_sync_command returning {error, timeout} or {error, noproc} after retries exhaust) comes back as a bare {error, Reason}, with no wrapper that distinguishes it from a value the transaction function might (legitimately) return itself.

Making the distinction between theses two sources of {error, Reason} is going to require a bit of refactoring anyway. That’s what make me favor a rework of the transaction API to make it more intuitve :-)

Give the infrastructure exception a structured reason we can pattern-match on

Khepri already uses the ?khepri_error() and ?khepri_ex() to create a common Khepri-specific error tuple. I think these should be used here too.

Since the transparent return changes what every existing khepri:transaction caller matches on, it probably wants a new entry point, or a major bump.

I’m strict about versioning (following semver) and breaking changes. In this case, the version would be bumped to e.g. 0.20.0. I’m not bumping the major version yet because the API is still not stable (I have other breaking changes in mind I didn’t get to work on yet, for instance around conditions in path patterns). I would also document the breaking change in the release notes with a "Breaking change" red rectangle. Finally, the machine version would have to be bumped too because all members in the cluster would have to act the same way.

I  was not aware of other consumers of Khepri beside RabbitMQ and some libraries such as khepri_mnesia_migration. That said, that’s exactly why I try to be as strict as possible with release engineering. This is out of scope for this pull request, but I’m very interested if something can be improved. Have you been beaten by a bad versioning or bad release in the past? What would you improve?

rgfaber · 2026-09-09T17:04:14Z

Frankly, "beaten by bad versioning/releases": not at all, not by khepri/ra.
We've had our share of issues, but they were in our own stack, not here.
Even for a v0.x project we never ran into extreme or frequent bugs, and getting feedback or a PR merged has never really been a blocker.

I think the more honest framing is that khepri/ra was never really meant to be the load-bearing substrate for actual data in the first place (at least, as I understand it).
RabbitMQ uses it for metadata, as a Mnesia replacement, which is a genuinely lighter-weight job.

ReckonDB builds a real, purpose-built event store on top of it, which is closer to abusing it than using it as intended, I suppose.
That wasn't because khepri looked like the right tool for that job specifically, it's that there were very few BEAM-native, clustering-capable options at all when that work started.
So where we've hit friction, I'd chalk it up to us pushing khepri/ra somewhere past its original design intention, not to khepri/ra being unreliable.

dumbbell · 2026-09-09T17:11:24Z

Thank you for the feedback!

Indeed, the current design of the internal tree is fine for small pieces of data like RabbitMQ metadata. But we also limit out usage internally because we can’t store large data inside it. One of the next big task on Khepri will be to tackle this problem. I know what I want to try and already have most of the details on paper. Hopefully, I should start to work on this before the end of this year.

dumbbell · 2026-09-11T13:37:44Z

I pushed the rework-how-tx-returns-value branch as an alternative implementation to yours: it wraps the transaction function early to help distinguish it from an error with Ra. The transaction function return value is unwrapped in the end, so this changes nothing for the caller. This requires a machine version bump to handle backward compatibility. I stole your testcases in the process :-)

Does it work for your use case?

I would like to push this effort further and implement what we discussed for transactions but also stored procedures if they need it too.

dumbbell · 2026-09-14T17:58:46Z

I pushed an update to rework-how-tx-returns-value branch. This time, a transaction function is "transparent" like a stored procedure already is. Transparent in the sense that it behaves like if Khepri was not involved: the return value is returned as is, exceptions are thrown as is. Only errors from Ra are thrown as ?khepri_error() exception to help distinguish them.

One last thing I need to do is deprecate khepri:abort/1 because any exception will abort the transaction. There is no need for a specific handling.

What do you think?

rgfaber · 2026-09-14T20:29:44Z

Thanks for picking this up, and steal away, the tests are better off in your branch :)

Short answer: yes, this is the shape we were hoping for. Tagging the fun's return as {txfun_ret, _} inside the machine fixes the ambiguity where it starts, instead of guessing from the shape afterwards like my PR did. On a running store it works for us: the success path gets simpler, and a Ra timeout arrives as {khepri, tx_error, #{reason => {error, timeout}}}, which we can match on.

I built the branch (b53a564, OTP 28) and ran a few cases against it. One gap: the case that started this PR still comes back in the old shape.

%% Store never started:
khepri:transaction(never_started, [foo], [], rw).
%% => {error, noproc}   (returned, not raised)

%% Running store, the fun itself returns an error tuple:
khepri:transaction(StoreId, fun() -> {error, noproc} end, rw).
%% => {error, noproc}

As far as I can tell, handle_tx_ret/2 only raises when does_api_comply_with(simplified_tx_ret, StoreId) is true, and that reads the persistent term cached in init/1. With no store running there is no term, so it takes the legacy path. That is exactly our startup race: writers dispatching while the Ra server is still coming up. The copied tests moved to a running store with timeout => 0, so this path isn't covered any more (their comments still mention a never-started store).

The same check probably leaves a short window right after init/1, while the cached version is still 0, where an abort comes back as {error, Reason} instead of a throw. I haven't reproduced that one; it's from reading the code.

Would it make sense to tag the dispatch errors where they are produced, the same way you tag the fun's return value? Then the caller side wouldn't need to infer anything from the machine version.

Either way, here are tests for it, for test/simple_tx_misc.erl. On the branch as it is, the first two fail with got: value {error,noproc} and the third passes. With a throwaway local change that raises in that case, all 51 tests in the module pass, so the assertions themselves should hold up.

Tests for the stopped-store case

readwrite_transaction_on_stopped_store_raises_test() ->
    %% A store that was never started has no Ra server, so the command never
    %% reaches the state machine and the transaction function never runs. The
    %% resulting `{error, noproc}' must be raised as a `tx_error', like any
    %% other error from Ra, so it can't be mistaken for a value returned by
    %% the transaction function.
    %%
    %% A path-pattern transaction is used so the command reaches
    %% `process_command/3' without going through stand-alone function
    %% extraction, which would require the khepri application.
    ?assertError(
       ?khepri_error(tx_error, #{reason := {error, noproc}}),
       khepri:transaction(?FUNCTION_NAME, [foo], [], rw)).

readonly_transaction_on_stopped_store_raises_test() ->
    %% Same expectation as the read-write case, for a read-only transaction
    %% going through `process_query/3'.
    ?assertError(
       ?khepri_error(tx_error, #{reason := {error, noproc}}),
       khepri:transaction(?FUNCTION_NAME, [foo], [], ro)).

tx_fun_returning_error_tuple_test_() ->
    %% The counterpart of the tests above: when the transaction function
    %% itself returns an error-shaped value, it ran, and that value is
    %% returned as is.
    {setup,
     fun() -> test_ra_server_helpers:setup(?FUNCTION_NAME) end,
     fun(Priv) -> test_ra_server_helpers:cleanup(Priv) end,
     [?_assertEqual(
         {error, noproc},
         begin
             Fun = fun() -> {error, noproc} end,
             khepri:transaction(?FUNCTION_NAME, Fun, rw)
         end),
      ?_assertEqual(
         {error, noproc},
         begin
             Fun = fun() -> {error, noproc} end,
             khepri:transaction(?FUNCTION_NAME, Fun, ro)
         end)]}.

One small thing I noticed while reading: failed_to_locate_sproc/2 calls does_api_comply_with/2 from inside the apply, so it reads the node-local cache rather than the machine version from Meta, as execute_tx1/2 does. It only changes the exception class, but keeping machine-side code on Meta might be worth it.

On our side the migration is small: a handful of call sites, and our khepri_tx:abort/1 calls become plain throws that we catch explicitly.

Thanks again!

dumbbell · 2026-09-15T13:07:57Z

Thank you @rgfaber for the feedback! I modified handle_tx_ret() to handle the usual {error, _} that we can get from Ra specifically. I got rid of the behaviour check because on the "client" side, the API should not change based on what the state machine can do here. It should always return the transaction function value or raise an error.

Hopefullly, your example is correctly handled now.

I also modified the documentation and comments which I didn’t touch before being sure of what I want from the API.

…rocs [Why] Before this patch, the return value of `khepri:transaction()` was: * `{ok, TxRet}` or * an exception if the transaction function aborted or crashed Any error, including with the communication with the Ra process (such as a `noproc` error or a timeout), was wrapped in `{ok, Error}` too. A return value of `{ok, {error, Reason}}` is awkward at best. It also made its interpretation quite messy: is the error from Khepri or the transaction function itself? This problem doesn't exist with an executed stored procedure because it is already transparent: it acts as if it was executed by the caller directly, like if Khepri was never involved. Having the same behaviour with transaction fixes the interpretation of the return values and makes the global API more consistent. [How] The return value of the transaction function is always wrapped in a `{txfun_ret, TxRet}` tuple internally. This helps distinguish it from other return values along the chain. Before returning anything to the caller, the transaction function return value `TxRet` is unwrapped. Any errors coming from outside the transaction function are thrown as exception. For instance `error:{error, noproc}`. `khepri:abort(Reason)` still raises a `throw:Reason` exception. That said, this API is deprecated because any Erlang exceptions will do the job just fine. This is a breaking change in Khepri transaction API. Callers will have to be adapted because the return value is changed from `{ok, ActualRet}` to `ActualRet`. The machine version is bumped to 5 and a new `transparent_tx_fun` behaviour is introduced to help with backward compatibility. Regardless of the effective machine version, the caller gets this new API. References #400.

…procs [Why] Before this patch, the return value of `khepri:transaction()` was: * `{ok, TxRet}` or * an exception if the transaction function aborted or crashed Any error, including with the communication with the Ra process (such as a `noproc` error or a timeout), was wrapped in `{ok, Error}` too. A return value of `{ok, {error, Reason}}` is awkward at best. It also made its interpretation quite messy: is the error from Khepri or the transaction function itself? This problem doesn't exist with an executed stored procedure because it is already transparent: it acts as if it was executed by the caller directly, like if Khepri was never involved. Having the same behaviour with transaction fixes the interpretation of the return values and makes the global API more consistent. [How] The return value of the transaction function is always wrapped in a `{txfun_ret, TxRet}` tuple internally. This helps distinguish it from other return values along the chain. Before returning anything to the caller, the transaction function return value `TxRet` is unwrapped. Any errors coming from outside the transaction function are thrown as exception. For instance `error:{error, noproc}`. `khepri:abort(Reason)` still raises a `throw:Reason` exception. That said, this API is deprecated because any Erlang exceptions will do the job just fine. This is a breaking change in Khepri transaction API. Callers will have to be adapted because the return value is changed from `{ok, ActualRet}` to `ActualRet`. The machine version is bumped to 5 and a new `transparent_tx_fun` behaviour is introduced to help with backward compatibility. Regardless of the effective machine version, the caller gets this new API. References #400.

…procs [Why] Before this patch, the return value of `khepri:transaction()` was: * `{ok, TxRet}` or * an exception if the transaction function aborted or crashed Any error, including with the communication with the Ra process (such as a `noproc` error or a timeout), was wrapped in `{ok, Error}` too. A return value of `{ok, {error, Reason}}` is awkward at best. It also made its interpretation quite messy: is the error from Khepri or the transaction function itself? This problem doesn't exist with an executed stored procedure because it is already transparent: it acts as if it was executed by the caller directly, like if Khepri was never involved. Having the same behaviour with transaction fixes the interpretation of the return values and makes the global API more consistent. [How] The return value of the transaction function is always wrapped in a `{txfun_ret, TxRet}` tuple internally. This helps distinguish it from other return values along the chain. Before returning anything to the caller, the transaction function return value `TxRet` is unwrapped. Any errors coming from outside the transaction function are thrown as exception. For instance `error:{error, noproc}`. `khepri:abort(Reason)` still raises a `throw:Reason` exception. That said, this API is deprecated because any Erlang exceptions will do the job just fine. This is a breaking change in Khepri transaction API. Callers will have to be adapted because the return value is changed from `{ok, ActualRet}` to `ActualRet`. The machine version is bumped to 5 and a new `transparent_tx_funs` behaviour is introduced to help with backward compatibility. Regardless of the effective machine version, the caller gets this new API. References #400.

dumbbell · 2026-09-17T14:25:22Z

@rgfaber: I opened #436 with the branch we were talking about. I made a few more changes to make sure that the khepri:transaction/5 API behaves always the same, regardless of the version of the state machine. Could you please take another look?

rgfaber force-pushed the fix/readwrite-transaction-error-propagation branch from 58449e6 to 292d419 Compare June 24, 2026 05:03

rgfaber added 2 commits June 24, 2026 07:09

rgfaber changed the title ~~khepri_machine: propagate process_command errors from readwrite transactions~~ khepri_machine: propagate process_command/process_query errors from transactions Jun 26, 2026

rgfaber mentioned this pull request Jul 4, 2026

does_path_match crash when a path-condition trigger is evaluated against a just-deleted node (0.17.2–0.18.0) #405

Closed

dumbbell mentioned this pull request Sep 17, 2026

khepri_machine: Make transaction functions "transparent" like stored procs #436

Draft

Navigation Menu

khepri_machine: propagate process_command/process_query errors from transactions - #400

khepri_machine: propagate process_command/process_query errors from transactions#400
rgfaber wants to merge 3 commits into
rabbitmq:mainfrom
rgfaber:fix/readwrite-transaction-error-propagation

rgfaber commented Jun 23, 2026 •

edited

Loading

Uh oh!

dumbbell commented Sep 9, 2026

Uh oh!

rgfaber commented Sep 9, 2026

Uh oh!

dumbbell commented Sep 9, 2026

Uh oh!

rgfaber commented Sep 9, 2026

Uh oh!

dumbbell commented Sep 9, 2026

Uh oh!

dumbbell commented Sep 11, 2026

Uh oh!

dumbbell commented Sep 14, 2026

Uh oh!

rgfaber commented Sep 14, 2026

Uh oh!

dumbbell commented Sep 15, 2026

Uh oh!

dumbbell commented Sep 17, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

rgfaber commented Jun 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Fix

Tests

Note for reviewers

Uh oh!

dumbbell commented Sep 9, 2026

Uh oh!

rgfaber commented Sep 9, 2026

Uh oh!

dumbbell commented Sep 9, 2026

Uh oh!

rgfaber commented Sep 9, 2026

Uh oh!

dumbbell commented Sep 9, 2026

Uh oh!

dumbbell commented Sep 11, 2026

Uh oh!

dumbbell commented Sep 14, 2026

Uh oh!

rgfaber commented Sep 14, 2026

Uh oh!

dumbbell commented Sep 15, 2026

Uh oh!

dumbbell commented Sep 17, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

rgfaber commented Jun 23, 2026 •

edited

Loading