Expose transient vs fatal on persisted errors#1716
Conversation
PersistedError already knows whether the underlying ApiError was Transient, Fatal, or FatalWithState, but api_error() collapses all three, so callers cannot tell whether to retry from the same state or treat the session as closed. Add is_transient() and is_fatal() accessors. Carry the distinction through the FFI: ReceiverPersistedError's Receiver variant becomes Transient and Fatal variants, so bindings consumers no longer have to treat every persisted error as session-fatal (or string-match messages) to stay safe. Unclassified errors map to Fatal, the conservative default for a session-driving loop.
Coverage Report for CI Build 29009340477Coverage increased (+0.02%) to 85.889%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
|
Is https://github.com/payjoin/rust-payjoin/actions/runs/29009340442/job/86088745217?pr=1716 taking 2 hours normal behaviour ? |
Definitely not. Windows does it in 7 minutes. I think nix is to blame, since Windows does it without it. |
| pub fn is_fatal(&self) -> bool { | ||
| matches!( | ||
| self.0, | ||
| InternalPersistedError::Api(ApiError::Fatal(_) | ApiError::FatalWithState(_, _)) | ||
| ) | ||
| } |
There was a problem hiding this comment.
This seems redundant IMO, If is_transient() returns a bool why do we need an additional method to return the inverse?
There was a problem hiding this comment.
| error | is_transient() | is_fatal() | !is_transient() |
|---|---|---|---|
| Api Transient | true | false | false |
| Api Fatal / FatalWithState | false | true | true |
| Storage | false | false | true ← the trap |
@benalleng This is kinda how I'm thinking about it, but there's probably a way to simply... You've basically given a litmus test for the API shape in a way
It's been like that for about a week... Same locally after the initial download, I think in CI it downloads everytime |
|
Dug into it, the nix suspicion was right, with a twist. The job isn't slow at building or testing: the script finishes in under five minutes (cargo 4m09s, dotnet tests 8s). The 2+ hours are |
|
Superseded by #1724 |
6 participants
Addresses the question in #1709.
PersistedErroralready knows whether the underlyingApiErrorwasTransient,Fatal, orFatalWithState, butapi_error()collapses all three, so after persisting a transition the caller cannot tell whether to retry from the same state or treat the session as closed. This addsis_transient()andis_fatal()accessors (bothfalsefor storage errors, which have their own accessor).The same information is then carried across the FFI, where today it is erased a second time:
ReceiverPersistedError's singleReceivervariant becomesTransientandFatalvariants. uniffi error enums can't export methods, so the kind has to ride as variants — this is a breaking change for bindings that match onReceiver, which seemed acceptable at rc stage given the alternative is that downstream languages stay stuck even after the core API gains a signal. Unclassified errors map toFatal, the conservative default for a session-driving loop.Field report for motivation: the BTCPay Server payjoin plugin (C# over payjoin-ffi) currently treats every exception from a transition save as session-fatal and closes the session, because the only alternative is string-matching display messages. That necessarily kills sessions that failed transiently, e.g. a directory hiccup surfacing through
process_response.Two scope notes:
MaybeTerminalSuccessTransition#1709 (TransientWithState, or transient-not-an-error persister outcomes). If one of those lands, the FFI carry-through here still applies to whatever replaces it.SenderPersistedErrorhas the same collapse; happy to follow up on the send side if this direction is agreeable.Disclosure: co-authored by Claude Code
Pull Request Checklist
Please confirm the following before requesting review:
AI
in the body of this PR.