Retry transient directory errors, and a shared retry policy

[bc1cindy]

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.

[bc1cindy]

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?

[xstoicunicornx]

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.

[bc1cindy]

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?

[xstoicunicornx]

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

[bc1cindy]

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.