Skip to content

fix(wallet): report swaps stuck in processing from the reconciliation cycle - #161

Open
zaxovaiko wants to merge 1 commit into
fix/rg-limit-on-address-depositsfrom
fix/stuck-swap-findings
Open

fix(wallet): report swaps stuck in processing from the reconciliation cycle#161
zaxovaiko wants to merge 1 commit into
fix/rg-limit-on-address-depositsfrom
fix/stuck-swap-findings

Conversation

@zaxovaiko

Copy link
Copy Markdown
Member

Summary

The wallet reconciliation cycle files a stuck_swap finding for every swap_out leg still processing past wallet.reconciliation.stuckAfterMinutes, the same way it reports a stuck custody sweep. It never refunds or credits on its own.

Why

A swap_out leg debits the player before the desk is called. If the process dies before the fill lands, or the desk reports a fill with no amount, the leg stayed processing and nothing looked at it again: the player's balance was down and no one was told.

Alternatives considered

  • Refunding automatically after the cutoff. The desk may already have filled the swap, so an automatic refund can pay the player twice.
  • Polling the desk for the swap status. The swap port has no status call today; worth adding once a desk exposes one, and the finding stays as the fallback.

Risks

… cycle

A swap_out leg debits the player before the desk is called. If the process dies before the fill lands, or the desk reports a fill with no amount, the leg stayed processing and nothing looked at it again. Past stuckAfterMinutes the cycle now files a stuck_swap finding for a human. It never refunds or credits on its own, because the desk may already have filled the swap.

Signed-off-by: Volodymyr Zakhovaiko <zaxovaiko@gmail.com>

@jakubfilinger-b jakubfilinger-b left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The gap this closes is a genuine one — a swap_out that debits and then goes quiet was money out of a player's balance with nothing watching it — and the restraint is right: report, never refund, because the desk may already have filled. Modelling it on reconcileStuckSweeps rather than inventing a new shape is the right instinct, and the starvation guard (asc(alreadyReported)) is carried over correctly, so a permanent backlog cannot hide newer stuck legs.

My blocking concern is the cutoff, not the mechanism: reusing the withdrawal cutoff for a state that is legitimately long-lived on the swap path turns normal async swaps into findings. Detail on that thread, together with the second half of the problem — nothing ever closes a finding once the desk does settle.

Process note: this is based on fix/rg-limit-on-address-deposits, and I've asked for changes on #160, so this will need a rebase before it can be read as a standalone diff. Nothing here depends on what I asked for there — the two only share the enum migration ordering.

One thing I did not flag as a defect, for the record: counts.stuckSwaps increments for every row scanned rather than every finding actually filed, so on a permanent backlog the run summary reports the same non-zero number forever. That is exactly what reconcileStuckSweeps and reconcileStuckWithdrawals already do, and matching the siblings is worth more than being right alone. If it gets fixed, it should be fixed for all three at once.

}

await this.reconcileStuckWithdrawals(runId, cfg.stuckAfterMinutes, cfg.batchSize, counts);
await this.reconcileStuckSwaps(runId, cfg.stuckAfterMinutes, cfg.batchSize, counts);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This hands swaps cfg.stuckAfterMinutes — the withdrawal cutoff. The sibling below deliberately does not do that: reconcileStuckSweeps takes unknownAfterMinutes, its own knob, precisely because "stuck" means something different for a sweep than for a withdrawal. Swaps have the same claim to their own number, and a stronger one, because processing is a legitimate resting state here: SwapService.swap returns { status: 'processing' } whenever the desk fills asynchronously, and the leg then waits for reconcileSwapStatus to be called by the webhook.

So the cutoff is not measuring "stuck", it is measuring "slower than a withdrawal". An operator whose desk settles in, say, 45 minutes against a 30-minute withdrawal cutoff gets a stuck_swap finding for every single normal swap, and the findings queue becomes noise the day it ships. Since countOpenFindings feeds alertThreshold, that also fires the reconciliation alert on healthy traffic.

Please give swaps their own wallet.reconciliation.stuckSwapAfterMinutes (defaulting to the withdrawal value if you want no config churn for existing operators), so the number can be set from how long the desk actually takes.

// is money debited from the player with nothing credited back, and the desk may or may not
// have filled it - so neither a refund nor a credit is safe without someone looking.
// Covered by wallet_transaction_status_type_created_at_idx, same as stuck withdrawals.
private async reconcileStuckSwaps(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The other half of the cutoff problem: nothing ever closes one of these. A swap reported at the cutoff and then settled normally by the desk webhook ten minutes later leaves an open finding that describes a situation which no longer exists, and only a human clicking resolve clears it. With the shared withdrawal cutoff above, that is not an edge case — it is every slow-but-healthy swap.

A stuck sweep has the same property, but a sweep that resolves itself is rarer and the row is at least still true when written. Here the row goes stale by design.

Cheapest fix that keeps "never move money automatically" intact: at the top of the cycle, resolve open stuck_swap findings whose transactionId is no longer processing — writing a resolutionNote saying the desk settled it, not touching a balance or a transaction status. That is a report-side write only, which is what this table is for.

currency: tx.currency,
amount: tx.amount,
transactionId: tx.id,
externalId: tx.id,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

externalId is the desk's own reference everywhere else in this table — the sweep sibling writes sweep.externalId ?? sweep.id, preferring the vendor's id and falling back to the local one. Here it is unconditionally tx.id, while the desk reference the operator would actually quote at the vendor (tx.providerRefId) goes into the prose of detail instead.

I understand why: tx.id is always present, so it is the reliable dedup key, and providerRefId is null for a leg that died before execute returned. But that makes the column mean two different things depending on the row's kind, and an operator joining findings to desk records by externalId gets nothing back for swaps. Worth either following the sweep's providerRefId ?? id pattern (dedup still works — a leg that later gains a reference would file a second finding, which is arguably correct since it is now a different situation), or leaving a comment here saying the local id is deliberate and why.

// Human resolution only, like a stuck sweep. A swap_out held `processing` past the cutoff
// is money debited from the player with nothing credited back, and the desk may or may not
// have filled it - so neither a refund nor a credit is safe without someone looking.
// Covered by wallet_transaction_status_type_created_at_idx, same as stuck withdrawals.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small accuracy point on the comment: wallet_transaction_status_type_created_at_idx is (status, type, created_at), so it covers the WHERE — but the ORDER BY leads with the correlated EXISTS expression, which no index can serve, so every cycle sorts the full matching set before applying LIMIT. On a healthy system that set is tiny and it does not matter; on the backlog this feature is designed to accumulate, it grows and never shrinks, because nothing moves a reported leg out of processing.

The same is true of the sweep sibling, so I am not asking you to change the query — just to not claim coverage the plan does not have. If the resolve-on-settle suggestion above lands, the backlog stops growing and the point becomes moot.

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.

2 participants