Skip to content

Persister no longer treats transient errors as errors in MaybeTerminalSuccessTransition#1721

Closed
benalleng wants to merge 1 commit into
payjoin:masterfrom
benalleng:distinguish-transient-error
Closed

Persister no longer treats transient errors as errors in MaybeTerminalSuccessTransition#1721
benalleng wants to merge 1 commit into
payjoin:masterfrom
benalleng:distinguish-transient-error

Conversation

@benalleng

Copy link
Copy Markdown
Collaborator

Closes #1709

Previously a transient error was treated as an error in the persister alongside fatal errors but unlike fatal errors transient errors were not giving the state at which it was reached.

This commit returns the current state alongside the error while still retaining a NoOp in the persister with the expectation that the caller will want to try again.

Coded with assistance from deepseek v4

Pull Request Checklist

Please confirm the following before requesting review:

@benalleng benalleng force-pushed the distinguish-transient-error branch from bb02b0d to e62be8b Compare July 9, 2026 15:43
@coveralls

coveralls commented Jul 9, 2026

Copy link
Copy Markdown
Collaborator

Coverage Report for CI Build 29046681515

Coverage decreased (-0.07%) to 85.932%

Details

  • Coverage decreased (-0.07%) from the base build.
  • Patch coverage: 38 uncovered changes across 3 files (51 of 89 lines covered, 57.3%).
  • 2 coverage regressions across 2 files.

Uncovered Changes

File Changed Covered %
payjoin-cli/src/app/v2/mod.rs 32 0 0.0%
payjoin/src/core/persist.rs 38 34 89.47%
payjoin/src/core/receive/v2/mod.rs 19 17 89.47%

Coverage Regressions

2 previously-covered lines in 2 files lost coverage.

File Lines Losing Coverage Coverage
payjoin-cli/src/app/v2/mod.rs 1 51.59%
payjoin/src/core/persist.rs 1 95.9%

Coverage Stats

Coverage Status
Relevant Lines: 15695
Covered Lines: 13487
Line Coverage: 85.93%
Coverage Strength: 347.63 hits per line

💛 - Coveralls

@benalleng benalleng force-pushed the distinguish-transient-error branch 4 times, most recently from 7090905 to 7189410 Compare July 9, 2026 16:09
@benalleng benalleng marked this pull request as ready for review July 9, 2026 16:09
@benalleng benalleng requested a review from spacebear21 July 9, 2026 16:09
@benalleng benalleng force-pushed the distinguish-transient-error branch from 7189410 to 592747e Compare July 9, 2026 16:23
@benalleng benalleng requested a review from xstoicunicornx July 9, 2026 16:25
Previously a transient error was treated as an error in the persister
alongside fatal errors but unlike fatal errors transient errors were
not giving the state at which it was reached.

This commit returns the current state alongside the error while still
retaining a NoOp in the persister with the expectation that the caller
will want to try again.
@benalleng benalleng force-pushed the distinguish-transient-error branch from 592747e to 1c003c9 Compare July 9, 2026 20:05
@xstoicunicornx

Copy link
Copy Markdown
Collaborator

I think it is creating a bit of an anti pattern - namely that for this transition the transient error is considered a success while for all other transitions the transient error is still an error. Was there a reason you feel like for this transition it should be a success rather than an error?

@benalleng benalleng changed the title Make persister no longer treat transient errors as an error Persister no longer treats transient errors as errors in MaybeTerminalSuccessTransition Jul 9, 2026
@benalleng

Copy link
Copy Markdown
Collaborator Author

This serves as a blueprint for the transition states. If we feel like we like this shape I can implement the rest the same way

@spacebear21

Copy link
Copy Markdown
Collaborator

I had Fable write an alternative, more generic approach for the same concept here #1724. The first commit is the relevant one, introducing current state on all transient errors.

@spacebear21

Copy link
Copy Markdown
Collaborator

Superseded by #1724

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Ambiguous state when persisting MaybeTerminalSuccessTransition

4 participants