From 24df678bed64e5ff1b5b6675d8b73a493bbcd99f Mon Sep 17 00:00:00 2001 From: ekzyis Date: Tue, 21 Jan 2025 13:12:55 +0100 Subject: [PATCH] Use optimistic concurrency lock for retries --- api/paidAction/README.md | 9 ++-- api/paidAction/index.js | 3 +- api/resolvers/paidAction.js | 44 ++++++++++++------- api/resolvers/wallet.js | 24 ++++------ api/typeDefs/paidAction.js | 2 +- components/use-invoice.js | 4 +- fragments/paidAction.js | 4 +- lib/constants.js | 3 -- .../migration.sql | 6 ++- prisma/schema.prisma | 2 +- wallets/index.js | 10 ++++- wallets/server.js | 6 +-- 12 files changed, 64 insertions(+), 53 deletions(-) diff --git a/api/paidAction/README.md b/api/paidAction/README.md index a32588076..08be33720 100644 --- a/api/paidAction/README.md +++ b/api/paidAction/README.md @@ -15,8 +15,9 @@ stateDiagram-v2 PENDING --> FAILED PAID --> [*] CANCELING --> FAILED - FAILED --> RETRYING + FAILED --> RETRY_PENDING FAILED --> [*] + RETRY_PENDING --> RETRYING RETRYING --> [*] [*] --> PENDING_HELD PENDING_HELD --> HELD @@ -59,8 +60,9 @@ stateDiagram-v2 PENDING --> FAILED PAID --> [*] CANCELING --> FAILED - FAILED --> RETRYING + FAILED --> RETRY_PENDING FAILED --> [*] + RETRY_PENDING --> RETRYING RETRYING --> [*] ``` @@ -121,8 +123,9 @@ This works by requesting an invoice from the recipient's wallet and reusing the stateDiagram-v2 PAID --> [*] CANCELING --> FAILED - FAILED --> RETRYING + FAILED --> RETRY_PENDING FAILED --> [*] + RETRY_PENDING --> RETRYING RETRYING --> [*] [*] --> PENDING_HELD PENDING_HELD --> FORWARDING diff --git a/api/paidAction/index.js b/api/paidAction/index.js index 8feabc306..5e4a34566 100644 --- a/api/paidAction/index.js +++ b/api/paidAction/index.js @@ -363,11 +363,10 @@ export async function retryPaidAction (actionType, args, incomingContext) { return await models.$transaction(async tx => { const context = { ...retryContext, tx, invoiceArgs } - // update the old invoice to RETRYING, so that it's not confused with FAILED await tx.invoice.update({ where: { id: failedInvoice.id, - actionState: 'FAILED' + actionState: 'RETRY_PENDING' }, data: { actionState: 'RETRYING' diff --git a/api/resolvers/paidAction.js b/api/resolvers/paidAction.js index 7a5184181..8ac379ef3 100644 --- a/api/resolvers/paidAction.js +++ b/api/resolvers/paidAction.js @@ -50,35 +50,45 @@ export default { } }, Mutation: { - retryPaidAction: async (parent, { invoiceId }, { models, me, lnd }) => { + retryPaidAction: async (parent, { invoiceId, newAttempt }, { models, me, lnd }) => { if (!me) { throw new Error('You must be logged in') } - const invoice = await models.invoice.findUnique({ where: { id: invoiceId, userId: me.id } }) + // optimistic concurrency control: + // make sure only one client at a time can retry by immediately transitioning to an intermediate state + const [invoice] = await models.$queryRaw` + UPDATE "Invoice" + SET "actionState" = 'RETRY_PENDING' + WHERE id in ( + SELECT id FROM "Invoice" + WHERE id = ${invoiceId} AND "userId" = ${me.id} AND "actionState" = 'FAILED' + FOR UPDATE + ) + RETURNING *` if (!invoice) { throw new Error('Invoice not found') } - if (invoice.actionState !== 'FAILED') { - if (invoice.actionState === 'PAID') { - throw new Error('Invoice is already paid') - } - throw new Error(`Invoice is not in failed state: ${invoice.actionState}`) - } - - // a locked invoice means we want to retry a payment from the beginning - // with all sender and receiver wallets so we need to increment the retry count - const paymentAttempt = invoice.lockedAt ? invoice.paymentAttempt + 1 : invoice.paymentAttempt + // do we want to retry a payment from the beginning with all sender and receiver wallets? + const paymentAttempt = newAttempt ? invoice.paymentAttempt + 1 : invoice.paymentAttempt if (paymentAttempt > WALLET_MAX_RETRIES) { throw new Error('Payment has been retried too many times') } - const result = await retryPaidAction(invoice.actionType, { invoice }, { paymentAttempt, models, me, lnd }) - - return { - ...result, - type: paidActionType(invoice.actionType) + try { + const result = await retryPaidAction(invoice.actionType, { invoice }, { paymentAttempt, models, me, lnd }) + return { + ...result, + type: paidActionType(invoice.actionType) + } + } catch (err) { + // revert state transition to allow new retry for this invoice + await models.invoice.update({ + where: { id: invoiceId, actionState: 'RETRY_PENDING' }, + data: { actionState: 'FAILED' } + }) + throw err } } }, diff --git a/api/resolvers/wallet.js b/api/resolvers/wallet.js index 7271a406c..8e6f3f0e7 100644 --- a/api/resolvers/wallet.js +++ b/api/resolvers/wallet.js @@ -12,7 +12,6 @@ import { WALLET_CREATE_INVOICE_TIMEOUT_MS, WALLET_RETRY_AFTER_MS, WALLET_RETRY_BEFORE_MS, - WALLET_VISIBILITY_TIMEOUT_MS, WALLET_MAX_RETRIES } from '@/lib/constants' import { amountSchema, validateSchema, withdrawlSchema, lnAddrSchema } from '@/lib/validate' @@ -467,21 +466,14 @@ const resolvers = { } // make sure each invoice is only returned once via visibility timeouts and SKIP LOCKED return await models.$queryRaw` - UPDATE "Invoice" - SET "lockedAt" = now() - WHERE id IN ( - SELECT id FROM "Invoice" - WHERE "userId" = ${me.id} - AND "actionState" = 'FAILED' - AND "userCancel" = false - AND "cancelledAt" < now() - ${`${WALLET_RETRY_AFTER_MS} milliseconds`}::interval - AND "cancelledAt" > now() - ${`${WALLET_RETRY_BEFORE_MS} milliseconds`}::interval - AND ("lockedAt" IS NULL OR "lockedAt" < now() - ${`${WALLET_VISIBILITY_TIMEOUT_MS} milliseconds`}::interval) - AND "paymentAttempt" < ${WALLET_MAX_RETRIES} - ORDER BY id DESC - FOR UPDATE SKIP LOCKED - ) - RETURNING *` + SELECT * FROM "Invoice" + WHERE "userId" = ${me.id} + AND "actionState" = 'FAILED' + AND "userCancel" = false + AND "cancelledAt" < now() - ${`${WALLET_RETRY_AFTER_MS} milliseconds`}::interval + AND "cancelledAt" > now() - ${`${WALLET_RETRY_BEFORE_MS} milliseconds`}::interval + AND "paymentAttempt" < ${WALLET_MAX_RETRIES} + ORDER BY id DESC` } }, Wallet: { diff --git a/api/typeDefs/paidAction.js b/api/typeDefs/paidAction.js index 45c66c397..38a592090 100644 --- a/api/typeDefs/paidAction.js +++ b/api/typeDefs/paidAction.js @@ -7,7 +7,7 @@ extend type Query { } extend type Mutation { - retryPaidAction(invoiceId: Int!): PaidAction! + retryPaidAction(invoiceId: Int!, newAttempt: Boolean): PaidAction! } enum PaymentMethod { diff --git a/components/use-invoice.js b/components/use-invoice.js index 534426358..f59cd1db3 100644 --- a/components/use-invoice.js +++ b/components/use-invoice.js @@ -42,9 +42,9 @@ export default function useInvoice () { return data.cancelInvoice }, [cancelInvoice]) - const retry = useCallback(async ({ id, hash, hmac }, { update } = {}) => { + const retry = useCallback(async ({ id, hash, hmac, newAttempt = false }, { update } = {}) => { console.log('retrying invoice:', hash) - const { data, error } = await retryPaidAction({ variables: { invoiceId: Number(id) }, update }) + const { data, error } = await retryPaidAction({ variables: { invoiceId: Number(id), newAttempt }, update }) if (error) throw error const newInvoice = data.retryPaidAction.invoice diff --git a/fragments/paidAction.js b/fragments/paidAction.js index 0aa0a71a6..8596ce52c 100644 --- a/fragments/paidAction.js +++ b/fragments/paidAction.js @@ -89,8 +89,8 @@ export const RETRY_PAID_ACTION = gql` ${PAID_ACTION} ${ITEM_PAID_ACTION_FIELDS} ${ITEM_ACT_PAID_ACTION_FIELDS} - mutation retryPaidAction($invoiceId: Int!) { - retryPaidAction(invoiceId: $invoiceId) { + mutation retryPaidAction($invoiceId: Int!, $newAttempt: Boolean) { + retryPaidAction(invoiceId: $invoiceId, newAttempt: $newAttempt) { __typename ...PaidActionFields ... on ItemPaidAction { diff --git a/lib/constants.js b/lib/constants.js index 154fdb122..fe46b71a6 100644 --- a/lib/constants.js +++ b/lib/constants.js @@ -200,8 +200,5 @@ export const WALLET_CREATE_INVOICE_TIMEOUT_MS = 45_000 // be retried by the client due to sender or receiver fallbacks are not returned to the client. export const WALLET_RETRY_AFTER_MS = 60_000 // 1 minute export const WALLET_RETRY_BEFORE_MS = 86_400_000 // 24 hours -// timeout after which we give up on waiting for the retry of a previously returned invoice -// and thus we allow returning an invoice to a client again -export const WALLET_VISIBILITY_TIMEOUT_MS = 60_000 // 1 minute // we want to attempt a payment three times so we retry two times export const WALLET_MAX_RETRIES = 2 diff --git a/prisma/migrations/20250107084543_automated_retries/migration.sql b/prisma/migrations/20250107084543_automated_retries/migration.sql index 17770e9e3..2cbaf7f16 100644 --- a/prisma/migrations/20250107084543_automated_retries/migration.sql +++ b/prisma/migrations/20250107084543_automated_retries/migration.sql @@ -1,4 +1,6 @@ -- AlterTable ALTER TABLE "Invoice" ADD COLUMN "paymentAttempt" INTEGER NOT NULL DEFAULT 0; -ALTER TABLE "Invoice" ADD COLUMN "lockedAt" TIMESTAMP(3); -CREATE INDEX "Invoice_cancelledAt_idx" ON "Invoice"("cancelledAt"); \ No newline at end of file +CREATE INDEX "Invoice_cancelledAt_idx" ON "Invoice"("cancelledAt"); + +-- AlterEnum +ALTER TYPE "InvoiceActionState" ADD VALUE 'RETRY_PENDING'; \ No newline at end of file diff --git a/prisma/schema.prisma b/prisma/schema.prisma index f1a189c1e..d13c67909 100644 --- a/prisma/schema.prisma +++ b/prisma/schema.prisma @@ -890,6 +890,7 @@ enum InvoiceActionState { FORWARDING FORWARDED FAILED_FORWARD + RETRY_PENDING RETRYING CANCELING } @@ -924,7 +925,6 @@ model Invoice { cancelled Boolean @default(false) cancelledAt DateTime? userCancel Boolean? - lockedAt DateTime? paymentAttempt Int @default(0) msatsRequested BigInt msatsReceived BigInt? diff --git a/wallets/index.js b/wallets/index.js index 83c6c13a0..4bbc3359f 100644 --- a/wallets/index.js +++ b/wallets/index.js @@ -238,7 +238,15 @@ function RetryHandler ({ children }) { useEffect(() => { (async () => { for (const invoice of failedInvoices) { - const newInvoice = await invoiceHelper.retry(invoice) + let newInvoice + try { + // retries can fail since only one client at a time is allowed to retry + // but multiple clients could attempt a retry at the same time + newInvoice = await invoiceHelper.retry({ ...invoice, newAttempt: true }) + } catch (err) { + console.error('retry failed:', err.message) + continue + } waitForWalletPayment(newInvoice).catch(console.error) } })() diff --git a/wallets/server.js b/wallets/server.js index 58c00c11b..3e71fc5f0 100644 --- a/wallets/server.js +++ b/wallets/server.js @@ -114,8 +114,8 @@ export async function createWrappedInvoice (userId, export async function getInvoiceableWallets (userId, { paymentAttempt, predecessorId, models }) { // filter out all wallets that have already been tried by recursively following the retry chain of predecessor invoices. - // the current predecessor invoice is in state 'FAILED' and not in state 'RETRYING' because we are currently retrying it - // so it has not been updated yet. + // the current predecessor invoice is in state 'RETRY_PENDING' and not in state 'RETRYING' + // because we are currently retrying it so it has not been updated yet. // if predecessorId is not provided, the subquery will be empty and thus no wallets are filtered out. const wallets = await models.$queryRaw` SELECT @@ -135,7 +135,7 @@ export async function getInvoiceableWallets (userId, { paymentAttempt, predecess -- this failed invoice will be used to start the recursion SELECT "Invoice"."id", "Invoice"."predecessorId" FROM "Invoice" - WHERE "Invoice"."id" = ${predecessorId} AND "Invoice"."actionState" = 'FAILED' + WHERE "Invoice"."id" = ${predecessorId} AND "Invoice"."actionState" = 'RETRY_PENDING' UNION ALL