From e3961fe4cac8749435dce4dc2acbab26ad41b4dd Mon Sep 17 00:00:00 2001 From: Ilia Denisov Date: Tue, 28 Jul 2026 12:15:59 +0200 Subject: [PATCH] fix(payments): a console refund that its own notification beat is not a repeat MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Refunding from /_gm reported "Already refunded" for a refund that had just succeeded, which reads as "you clicked twice". The two recording paths race. YooKassa fires refund.succeeded the moment POST /v3/refunds returns, so the notification handler often writes the reversal before the console's own write lands. Both name the same refund id, so the ledger's idempotency index rejects the second — which is the mechanism working exactly as intended: on the contour the money moved once, one refund row was written, the balance is right and no abuse flag was raised. Only the message was wrong about why. The console now reports success on that path, and the notification's log line no longer claims the refund was "issued outside the console" when it may well have come from it. Covered by an integration test that lands the notification first and then refunds from the console, asserting the operator is told it succeeded and that exactly one refund row exists. --- .claude/CLAUDE.md | 5 +++ .../inttest/payments_yookassa_test.go | 43 +++++++++++++++++++ .../internal/server/handlers_admin_refund.go | 9 ++++ backend/internal/server/handlers_yookassa.go | 5 ++- docs/PAYMENTS.md | 5 +++ docs/PAYMENTS_ru.md | 5 +++ 6 files changed, 71 insertions(+), 1 deletion(-) diff --git a/.claude/CLAUDE.md b/.claude/CLAUDE.md index da090b2..7de110e 100644 --- a/.claude/CLAUDE.md +++ b/.claude/CLAUDE.md @@ -252,6 +252,11 @@ The native Android app is a Capacitor 8 wrapper of the `ui` SPA, scaffolded unde IP bans being prod-only. It bought nothing anyway: the order is resolved from the notification metadata **before** any provider call, so a forged id costs one indexed read, and guessing a live order id means guessing a uuid. + - **The console refund and the `refund.succeeded` notification race, benignly.** YooKassa fires the + event the moment `POST /v3/refunds` returns, so the notification often records the reversal before + the console's own write lands. Both name the same refund id, so the ledger's idempotency index + catches the second and nothing is revoked twice — but the console must not then report "already + refunded", which reads as "you clicked twice" for a refund that just succeeded. - **Never key a re-check interval off the order TTL.** The order lifetime answers "how long may a customer take to pay"; the re-check answers "how soon do we notice a lost callback". Tying them together made a failed notification cost the customer the full 30-minute TTL before the chips diff --git a/backend/internal/inttest/payments_yookassa_test.go b/backend/internal/inttest/payments_yookassa_test.go index 9eae809..0c098a0 100644 --- a/backend/internal/inttest/payments_yookassa_test.go +++ b/backend/internal/inttest/payments_yookassa_test.go @@ -821,3 +821,46 @@ func TestYooKassaPendingRefundRecordsNothing(t *testing.T) { t.Errorf("balance = %d, want 0", got) } } + +// TestYooKassaConsoleRefundAfterItsOwnNotification covers the race the two recording paths really +// run: the console asks the provider to refund, the provider fires refund.succeeded at once, and the +// notification records the reversal before the console gets to. Both name the same refund id, so the +// idempotency index catches the second write and nothing is revoked twice — but the operator must be +// told the refund succeeded, not that they clicked twice. +func TestYooKassaConsoleRefundAfterItsOwnNotification(t *testing.T) { + f, shop := newFakeYooKassa(t) + srv, pay := yookassaServer(t, shop) + acc := provisionAccount(t) + orderID, paymentID := fundedYooKassaOrder(t, f, srv, pay, acc) + + // The provider's notification lands first, for the refund the console is about to create. + refundID := "refund-" + paymentID + f.setRefund(refundID, yookassa.Refund{ + Status: yookassa.StatusSucceeded, PaymentID: paymentID, + Amount: yookassa.Amount{Value: "149.00", Currency: "RUB"}, + }) + if code := postYooKassaRefundNotify(t, srv, refundID, paymentID); code != http.StatusOK { + t.Fatalf("refund notify = %d, want 200", code) + } + if got := readBalance(t, acc, "direct"); got != 0 { + t.Fatalf("balance after the notification = %d, want 0", got) + } + + base := "http://admin.test/_gm/users/" + acc.String() + code, body := consoleDo(srv.Handler(), http.MethodPost, base+"/refund", "order_id="+orderID.String(), "http://admin.test") + if code != http.StatusOK { + t.Fatalf("console refund = %d, want 200", code) + } + if strings.Contains(body, "Already refunded") { + t.Errorf("the console reported a repeat click for a refund it had just made successfully: %q", body) + } + if !strings.Contains(body, "Refunded") { + t.Errorf("console message = %q, want it to report the refund succeeded", body) + } + if ledgerRows(t, acc, "refund") != 1 { + t.Errorf("refund ledger rows = %d, want 1", ledgerRows(t, acc, "refund")) + } + if abuse, loss := readRisk(t, acc); abuse || loss != 0 { + t.Errorf("risk = (abuse %v, loss %d), want none", abuse, loss) + } +} diff --git a/backend/internal/server/handlers_admin_refund.go b/backend/internal/server/handlers_admin_refund.go index fb43156..c674028 100644 --- a/backend/internal/server/handlers_admin_refund.go +++ b/backend/internal/server/handlers_admin_refund.go @@ -49,6 +49,15 @@ func (s *Server) consoleRefund(c *gin.Context) { return } if out.AlreadyRefunded { + if ref.Provider == providerYooKassa { + // The provider refunded the money on this very click, so this is a success, not a repeat. + // The reversal was already in the ledger because the provider's own refund notification + // reached us first — it names the same refund id, which is exactly why the idempotency + // index caught it and nothing was revoked twice. Saying "already refunded" here would read + // as "you clicked twice". + s.renderConsoleMessage(c, "Refunded", "the money is back with the customer; the reversal was already recorded, so nothing changed just now", back) + return + } s.renderConsoleMessage(c, "Already refunded", "this order was already refunded", back) return } diff --git a/backend/internal/server/handlers_yookassa.go b/backend/internal/server/handlers_yookassa.go index 07d27b4..969f5dd 100644 --- a/backend/internal/server/handlers_yookassa.go +++ b/backend/internal/server/handlers_yookassa.go @@ -282,7 +282,10 @@ func (s *Server) handleYooKassaRefundNotification(c *gin.Context, note yookassa. return } if !out.AlreadyRefunded { - s.log.Info("yookassa refund notify: reversed a refund issued outside the console", + // This is the first record of the refund. It is usually one an operator made in the merchant + // cabinet, but it can equally be a console refund whose notification outran the console's own + // write — both name the same refund id, so either way it is recorded exactly once. + s.log.Info("yookassa refund notify: reversed a refund", zap.String("order", ref.OrderID.String()), zap.String("refund", refund.ID), zap.Int("revoked", out.Revoked), zap.Int("loss", out.Loss)) } diff --git a/docs/PAYMENTS.md b/docs/PAYMENTS.md index 25e5067..2963093 100644 --- a/docs/PAYMENTS.md +++ b/docs/PAYMENTS.md @@ -333,6 +333,11 @@ nothing twice. The engine is **full-refund-only** by design (it revokes exactly and rejects any other amount), so a **partial** refund is recorded as nothing at all and logged loudly for an operator: there is no non-arbitrary way to decide how many chips a part-refund costs. +The two recording paths race, benignly: YooKassa fires `refund.succeeded` the moment the refund is +created, so the notification often writes the reversal before the console's own write lands. Both +name the same refund id, so the second is caught by the idempotency index and nothing is revoked +twice; the console reports the refund as successful rather than as a repeated click. + VK refunds are still handled by support and Telegram Stars refunds issued with `refundStarPayment`, both recorded by hand afterwards. All of them converge on one engine — the `Refund` method (`internal/payments`): it matches the paid order, appends a **refund** ledger diff --git a/docs/PAYMENTS_ru.md b/docs/PAYMENTS_ru.md index 628e222..e53f512 100644 --- a/docs/PAYMENTS_ru.md +++ b/docs/PAYMENTS_ru.md @@ -326,6 +326,11 @@ at-least-once + идемпотентный приём (дедуп по `telegram записывается вовсе и громко логируется для оператора: непроизвольного способа решить, скольких Фишек стоит часть возврата, нет. +Два пути записи при этом безобидно гоняются: ЮKassa шлёт `refund.succeeded` сразу после создания +возврата, поэтому уведомление часто записывает реверс раньше, чем это успевает сделать сама консоль. +Оба называют один и тот же refund-id, поэтому вторая запись отсекается индексом идемпотентности и +ничего не отзывается дважды; консоль при этом сообщает об успешном возврате, а не о повторном нажатии. + Возвраты VK по-прежнему через поддержку, TG Stars — вызовом `refundStarPayment`, оба фиксируются руками после факта. Все сходятся на одном движке — метод `Refund` (`internal/payments`): матчит оплаченный заказ, пишет **refund**-строку журнала (идемпотентно по