Skip to content

Retry transient directory errors, and a shared retry policy #1670

Open
Open
Retry transient directory errors, and a shared retry policy#1670
Milestone
payjoin-cli-1.1

Description

@bc1cindy
bc1cindy
opened on Jun 22, 2026

the retry vector of #1586 (attempt count) is left to each integrator, which act differently, so a relay can tell wallets apart by retry behavior.

already in place: retries are freshly re-encapsulated (each create_*_request builds a new ohttp::ClientRequest, never resending bytes); outer relay failures already fail over to another relay; and the lib splits the directory's inner response via is_fatal() into fatal (4xx) vs transient (5xx).

per @nothingmuch: a relay must not be able to tell a retry from a first attempt, whatever the cause, the traffic it sees is the same whether or not you're retrying.

gap:

  1. transient directory errors aren't retried (message requests): the lib signals a 5xx as a transient transition, but no caller acts on it, payjoin-cli maps it to Err and stops the poll loop. this is the case @nothingmuch flags as the one that matters: it should retry, freshly built (already holds); a byte-identical resend would leak the prior failure to the relay.
  2. no shared retry policy to converge on: attempt-count diverge per integrator

plan:

  1. retry the transient (5xx) directory outcome with a fresh request via the normal poll (not a failover), a directory fault is not a relay failure, so do not mark_failed or fail over (switching relays won't fix a same-directory error and would burn asmap should be used for relay selection #919 windowed relays); leave fatal (4xx) non-retried: Resume v2 polling on transient directory errors #1671
  2. add a sans-io retry_policy() pure-function governing attempt-count and which error classes retry (transient yes, fatal no), shared so integrators converge
  3. make "rebuild, never resend" contractual and expose retry_policy() over uniffi so the bindings converge.

Activity

  1. benalleng
    added this to the payjoin-cli-1.1 milestone on Jun 29, 2026

  2. bc1cindy commented on Jul 1, 2026

    @bc1cindy
    bc1cindy
    on Jul 1, 2026
    ContributorAuthor

    what I have in mind for the shared fn (step 2):

    // payjoin/src/core/retry.rs

/// Owned by the crate, not the integrator, so a relay can't fingerprint by give-up count.
pub const MAX_RETRIES: u32 = 3;

pub enum RetryClass { Transient, Fatal } // crate derives this from is_fatal(): 5xx / 4xx
pub enum RetryDecision { Retry, GiveUp }

/// Directory-scoped: never fails over or marks a relay failed.
/// No delay, retry rides the normal poll schedule. `attempts` is the caller's
/// in-memory counter (durable cross-session tracking stays in #1697).
pub fn retry_policy(class: RetryClass, attempts: u32) -> RetryDecision {
    match class {
        RetryClass::Fatal => RetryDecision::GiveUp,
        RetryClass::Transient if attempts < MAX_RETRIES => RetryDecision::Retry,
        RetryClass::Transient => RetryDecision::GiveUp,
    }
}

    stacked on #1658, the pure fn compiles standalone, but wiring it into the poll loop lands on top of #1671

    sounds good?

  3. xstoicunicornx commented on Jul 21, 2026

    @xstoicunicornx
    xstoicunicornx
    on Jul 21, 2026
    Collaborator

    I think gap item (1) has been addressed by #1724 and #1737 now. What do you think?

    As for gap item (2), I am not sure we need to enforce alignment on the number of retries an implementer wants to have. The decision an implementer makes here could be heavily influenced by what their infrastructure looks like or who their main user base is.

  4. bc1cindy commented on Jul 21, 2026

    @bc1cindy
    bc1cindy
    on Jul 21, 2026
    ContributorAuthor

    thanks @xstoicunicornx

    yes, #1724 / #1737 resolved gap (1) and provided the infrastructure. closing #1671 as superseded.

    on (2): agreed we shouldn't dictate attempt counts for their own sake, but divergent retry cadence is exactly the fingerprint this issue is about, a relay that can cluster wallets by retry timing defeats the point. with #1658, the poll paths converge for free: a transient outcome just waits for the next tick of the shared PollSchedule, so a retry is indistinguishable from a normal poll and needs no separate policy. gap (2) then shrinks to a common policy (retry_policy) for just the three one-shot flows (original PSBT post, proposal post, error reply)

    wdyt?

  5. xstoicunicornx commented on Jul 21, 2026

    @xstoicunicornx
    xstoicunicornx
    on Jul 21, 2026
    Collaborator

    Shouldn't the policy be that for transient errors the retry should use a different relay altogether?

  6. bc1cindy commented on Jul 21, 2026

    @bc1cindy
    bc1cindy
    on Jul 21, 2026
    ContributorAuthor

    Shouldn't the policy be that for transient errors the retry should use a different relay altogether?

    no, the 5xx is the directory's, inside the encapsulation. the relay worked fine and can't even see it, and the directory is fixed per session, so switching can't fix it (relay failures already fail over in post_via_relay). an error-conditioned switch hands the directory an active probe: inject a 5xx, watch the mailbox hop relays, enumerate the client's relay set (#919). random per-request selection already makes a retry indistinguishable from a first attempt, the invariant.

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions