fix: follow-ups to the payout dispatch queue (post-merge audit of #883) - #893
Conversation
touch_order_payout_claim closed the double-payout window the dispatch queue opened, but not the spurious-failure one: a task queued past the reconcile grace window still lost its claim to re-arm, burning the buyer's retry budget — and eventually re-prompting for a fresh invoice — for a payout that was never sent. Reachable on stock settings whenever more than 8 payouts ride out their 75s bound at once, which is precisely the stuck-payout scenario this code exists to survive. Keep the claim younger than grace instead: while waiting for a permit, re-validate and re-stamp it every PAYOUT_QUEUE_HEARTBEAT (derived as 2/3 of the reconciler's MIN_GRACE_SECS, now promoted to a module constant so the relation is structural). The acquire future is pinned outside the select! loop so the task keeps its FIFO position in the semaphore queue across heartbeats. A lost claim or a DB error drops the dispatch — the same safe directions the post-permit touch already uses — and that final touch stays as the gate, since the last heartbeat can be a full cadence old. Also narrows the semaphore/permit docs: the bound covers concurrent payment streams (not LND connections, a deliberate fail-fast-before- claim trade) and the permit spans the send and RPC-error reconcile (the watcher is a sibling task).
The guard's catch-all silently absorbed the two cases worth knowing about: a lookup timeout and a lookup transport error both fell through to proceed with no trace, so an LND whose track_payment_v2 consistently exceeds the 2s bound would leave the guard permanently disabled and nothing in the logs would say so — the abort path logged, the degraded path did not. Give both arms an info! line; the remaining catch-all now covers only Failed/Unknown/no-record, the normal go-ahead.
Removing PAYMENT_STATUS_RECV_TIMEOUT left classify_send_verdict
admitting states its only caller could no longer produce: the drain
yields exactly None or Some((Terminal, _)), so the Indeterminate
stream-failure arm and its unwrap_or were dead code, and the
(succeeded, Option<failure>) pair could still encode the impossible
(true, Some(Terminal)) pairing the function had to pick a winner for.
Replace the pair with StreamOutcome { Succeeded, Failed(String),
Ended } — precisely what the drain observes — and classify over that.
The verdict logic is unchanged; the six tests port 1:1 and their
matrix is now total over the real input domain instead of total over
the reachable subset
The dispatch path had no coverage above the DB layer: the do_payment tests stop at pre-claim failures, so the timeout-keeps-the-marker invariant — the central safety argument of the background dispatch — lived only in a comment. Apply the same move classify_send_verdict got on the bond side: extract the post-send claim decision into a pure classify_dispatch(send_outcome, lookup) returning StreamEnded / KeepMarker / ReArm, with the LND status lookup as a parameter. Nine tests cover the full matrix: a timed-out send always keeps the claim; an RPC-level failure keeps it when LND reports the payment in flight or settled (or the lookup itself fails), and re-arms retry when LND reports failed/unknown/no record or the hash was unusable. The caller keeps the side effects (scoped fail_order_payout + retry bookkeeping); the warn causes now travel inside the verdict, a minor log reshuffle with no behavior change.
The docs presented payout_claimed_at as an identity token, but it has 1-second resolution (a same-second collision is harmless: the CAS also pins the hash, and LND rejects duplicate sends) and is vulnerable to backward clock steps (bounded in practice by the queue heartbeat). Name both caveats and record the clean fix — a monotonic per-order sequence — as deliberate future work.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
Included review availability: Your plan includes up to 2 reviews per rolling hour; 1 remains after this review. WalkthroughThe payout flow now maintains claims while tasks wait, revalidates claims before sending, classifies payment stream and RPC outcomes, centralizes reconciliation timing, documents timestamp limits, and logs degraded duplicate-payment lookups. ChangesPayout processing
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to The payout dispatch follow-up changes address the documented queue, outcome-classification, testing, documentation, and logging updates; no actionable merge-blocking risk remains after normal checks and review. Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant PayoutQueue
participant ClaimStorage
participant PaymentStream
participant LND
participant RetryState
PayoutQueue->>ClaimStorage: refresh and revalidate claim
ClaimStorage-->>PayoutQueue: valid or stale claim
PayoutQueue->>PaymentStream: send payment
PaymentStream-->>PayoutQueue: stream result or send error
PayoutQueue->>LND: look up status after RPC error
LND-->>PayoutQueue: payment state
PayoutQueue->>RetryState: retain marker or re-arm retry
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
grunch
left a comment
grunch
left a comment
There was a problem hiding this comment.
Review — approve.
Verified locally on the head commit (8482383): cargo fmt --check, cargo clippy --bin mostrod and cargo test --bin mostrod all clean — 1219 passed, including the nine new dispatch_* tests and the six ported classify_* tests.
Finding 1 — the heartbeat is correct, and the arithmetic is structural. PAYOUT_QUEUE_HEARTBEAT = MIN_GRACE_SECS * 2 / 3 = 20s against grace_secs = max(payment_retries_interval, MIN_GRACE_SECS) ≥ 30s, so a queued claim is always re-stamped before find_inflight_payouts' cutoff can see it; promoting MIN_GRACE_SECS to a module constant makes the relation hold by construction rather than by comment. Pinning the acquire future outside the select! is the right call: tokio's semaphore is FIFO only while the Acquire future is alive, and re-issuing acquire() per iteration would indeed have rotated the waiter to the back of the queue every cadence.
I traced the token chain end to end: claim(T0) → heartbeat CAS(T0→T1) → … → post-permit touch(Tn-1→Tn), with the final rebind landing before the watcher closure is constructed, so the watcher's clear_order_payout/fail_order_payout and the outer ReArm path all carry Tn — no path is left on a stale token. Both heartbeat failure directions are the safe one (Ok(None): a newer claim owns the order, drop; Err: keep the marker, the reconciler resolves). Race against the reconciler checked both ways: if the touch commits first, the reconciler's token-scoped release CAS no longer matches; if the release commits first, the touch sees a NULL hash and returns None → drop. No double-dispatch path.
Findings 2–3 — faithful equivalences, now under test. StreamOutcome collapses the old tuple exactly: (true, _) → Succeeded, (false, Some((Terminal, msg))) → Failed(msg), (false, None) → Ended, and the (succeeded, Some(_)) pairing the old type admitted is now unrepresentable. classify_dispatch preserves the old inline Ok(Err) arm (keep on InFlight/Succeeded/lookup-error; re-arm on Failed/Unknown/no-record/unusable-hash), and the timeout-keeps-the-claim invariant moved from a comment into dispatch_timeout_keeps_the_marker.
Findings 4–5 and the approval notes — accurate. The permit doc now matches reality (the watcher is a spawned sibling and finishes outside the RAII scope). The semaphore doc's "bounds payment streams, NOT connections" is true — LndConnector::new() runs before the claim, so queued tasks do hold idle connections, and the doc says so plainly. The duplicate guard's two degraded arms log and fall through to the same behavior as before; the match stays exhaustive.
Two non-blocking nits, no re-review needed:
db.rs, "Backward clock steps" bullet: a backward step after stamping only movesnow - gracebackwards, which makes a claim less reconcilable, not more. The grace-bypass case is a claim stamped while the clock sits behind (born old, immediately reconcilable once the clock corrects forward) — or a plain forward step. The exposure named is the right one; the sentence compresses the mechanism into the wrong direction.PAYOUT_QUEUE_HEARTBEAT's* 2 / 3degenerates to0sforMIN_GRACE_SECS = 1(integer division →sleep(Duration::ZERO)spin against the DB). Guarded today by the constant being 30 and the doc note next to it; a.max(1)would make the floor structural if the constant is ever expected to move.
All five audit findings and both approval notes are addressed faithfully, and the risky invariant — a queued-but-never-sent payout never ages past grace — is now enforced by construction and pinned by tests. LGTM.
2 participants
Addresses the post-merge audit on #883 (all five findings) plus the two
non-blocking notes from the approval review.
classify_send_verdictadmits states its only caller can no longer produceFinding 1 — while waiting for a send permit, the task now heartbeats its
claim:
touch_order_payout_claimeveryPAYOUT_QUEUE_HEARTBEAT(derived as2/3 of
MIN_GRACE_SECS, promoted to a module constant so the relation isstructural), so a queued claim never ages past grace and reconciliation can
no longer re-arm a queued-but-never-sent payout. The acquire future is pinned
outside the
select!loop so the task keeps its FIFO position in thesemaphore queue across heartbeats — the sketch in the review re-issued
acquire()per iteration, which would send the waiter to the back of thequeue every cadence. The post-permit touch stays as the final gate.
Findings 2–3 — the drain now returns
StreamOutcome { Succeeded, Failed(String), Ended }(the impossible pairings are unrepresentable; thesix tests port 1:1), and the buyer-side dispatch decision got the same
extraction the bond side already had: a pure
classify_dispatch(send_outcome, lookup)returningStreamEnded / KeepMarker / ReArm, with nine tests putting thetimeout-keeps-the-claim invariant under test instead of under a comment.
Findings 4–5 and the approval notes — docs narrowed to what the code
does (the semaphore bounds payment streams, not connections; the permit
spans the send and RPC-error reconcile; the token's 1s resolution and
clock-step caveats are named, with a monotonic per-order sequence recorded
as future work), and the duplicate guard's timeout/transport fall-throughs
now log instead of degrading silently.
Summary by CodeRabbit
Bug Fixes
Documentation
Reliability