Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(backend): deadlocks #3320

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open

fix(backend): deadlocks #3320

wants to merge 14 commits into from

Conversation

BlairCurrey
Copy link
Contributor

@BlairCurrey BlairCurrey commented Feb 24, 2025

Changes proposed in this pull request

  • fixes deadlocks by either using transaction after opening one, or moving calls before/after the commit
    • changes quote creation to not create then patch. which required some small changes to the expiryDate (base it off Date.now() before creating instead of quote.createdAt)
  • adds create local outgoing payment k6 script
  • adds caching to fees

Context

fixes #3319 #3226

Checklist

  • Related issues linked using fixes #number
  • Tests added/updated
  • Make sure that all checks pass
  • Bruno collection updated (if necessary)
  • Documentation issue created with user-docs label (if necessary)
  • OpenAPI specs updated (if necessary)

- fixes deadlocks by ensuring we dont open new connections before resolving open transactions
- forced knex connections to 1 to help find these cases
@github-actions github-actions bot added type: tests Testing related pkg: backend Changes in the backend package. type: source Changes business logic labels Feb 24, 2025
Copy link

netlify bot commented Feb 24, 2025

Deploy Preview for brilliant-pasca-3e80ec canceled.

Name Link
🔨 Latest commit cdac832
🔍 Latest deploy log https://app.netlify.com/sites/brilliant-pasca-3e80ec/deploys/67c09d3166efec000863af4b

- payment returned from createOutgoingPayment previously
had walletAddress of undefined
(coming from withGraphFetched(quote)).
When assigning manually it included wallet address.
Updated get methods to include wallet address on quote as well.
Comment on lines +180 to +193
// TODO: should getQuote happen inside trx? wasnt in main (was inside but not using trx).
// If so, in the getQuote method, need to not only pass into IlpQuoteDetails but also connector.
// Probably should have IlpQuoteDetails usingt he trx but not sure about the rest (just
// including the IlpQuoteDetails insert would prly require refactor to to that here)
const quote = await deps.paymentMethodHandlerService.getQuote(
paymentMethod,
{
quoteId,
walletAddress,
receiver,
receiveAmount: options.receiveAmount,
debitAmount: options.debitAmount
}
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Something still needs to be done here one way or another.

  1. return ilp quote details from get quote and save in transaction with quote insert. This assumes the ilp connector db calls do not need to use the transaction (they are not using it on main). This should be more performant as it greatly reduces the duration of the transaction. However it does leak the ilp payment method concern into quotes.

  2. Do this in the transaction. Pass the transaction into the connector and use for all the db calls.

Originally we called this inside the transaction. The transaction was passed in but only used to save the IlpQuoteDetails. It was not used by the connector. This getQuote is a heavy operation (for ilp payments) because of all the db/network calls so it would be nice not to include it in the transaction provided its safe.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another idea for 1. is a new saveAdditionalDetails(trx: Knex.Transction, quote: Quote) method on the paymentMethodHandlerService that inserts into IlpQuoteDetails for the ilp paymnet method, and does nothing for local payments. Keeps the ilp quote details encapsulated in the ilp payment method handler.

quote: UnfinalizedQuote,
receiver: Receiver
): Date {
const quoteExpiry = new Date(Date.now() + deps.config.quoteLifespan)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

creates an expiry of now, instead of createdAt (since we are setting ahead of creation). This was to allow us to simply create the quote and not create then update it.

@BlairCurrey BlairCurrey marked this pull request as ready for review February 27, 2025 15:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: backend Changes in the backend package. type: source Changes business logic type: tests Testing related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Database Deadlocks
1 participant