bug: stale queries get executed by subsequent lambda calls

[dan-kwiat]

Thanks for your work on this project @jeremydaly it's really useful :)

I just ran into a bug with throwing an error inside a transaction query, running on aws lambda. While the thrown error does prevent any previous queries from executing (as expected), it doesn't actually roll back. Worse, the previous queries do get executed by a subsequent call to the lambda function!

Minimal example:

import mysql from "serverless-mysql"

const db = mysql({
  config: {
    host: process.env.DB_HOST,
    database: process.env.DB_NAME,
    user: process.env.DB_USER,
    password: process.env.DB_PASS,
  },
})

export default async function handler() {
  try {
    await db
      .transaction()
      .query(() => ["INSERT INTO items (created_at) VALUES(?)", [new Date()]])
      .query(() => {
        throw new Error("Abort transaction")
      })
      .commit()

    console.log("Transaction succeeded")
  } catch (e) {
    console.error("Transaction failed")
  }

  await db.end()
}

Suppose this lambda is executed at time t1. As expected, the catch block gets triggered and nothing is inserted.

Now suppose the lambda is called again at a later time t2. Again, the catch block gets triggered and the new item is not inserted. However, as a result of the second lambda execution, the original item does get inserted with created_at = t1.

My short-term fix is to force a SQL error instead of throwing manually:

await db
  .transaction()
  .query(() => ["INSERT INTO users (created_at) VALUES(?)", [new Date()]])
  .query(() => ["SELECT hack_to_abort_transaction"])
  .commit()

Tested on MySQL 8.0.20.

[stephanelpaul]

you should be calling rollback in your try catch yourself actually.

const transaction = await db.transaction()

try {
 transaction.query(() => ["INSERT INTO items (created_at) VALUES(?)", [new Date()]])
 transaction.commit()
catch (e) {
   transaction.rollback()
}

 transaction.end()

[dan-kwiat]

you should be calling rollback in your try catch yourself actually.

I'm a bit late @StephaneP but your comment doesn't relate to the problem I described. I've just taken the time to test how the transaction rollback functionality works and this is what I've found:

1. transaction.rollback(e => {}) is just a callback handler, it doesn't trigger a ROLLBACK.
2. If there's a SQL error in any of your queries, calling .commit() will trigger a ROLLBACK, not a COMMIT. And an error will be thrown in your js.
3. No queries get submitted at all until .commit() is called. If your code throws an error before .commit() is called, there's nothing to rollback (note this doesn't include errors thrown inside a .query() call).
4. As described in my original post, a js error inside your .query() function is dangerous - it will not ROLLBACK or COMMIT. Dangerous because subsequent network requests which re-use the db connection will commit the old query from the previous request. This happens because START TRANSACTION from starting the new transaction will commit any previous queries implicitly.

To handle the last point, you need to add a manual rollback, like this:

async function executeTransaction(queries) {
  let transaction = db.transaction()
  try {
    queries.forEach((q) => {
      transaction.query(q)
    })
    await transaction.commit() // This submits all the queries, along with `ROLLBACK` or `COMMIT` at the end.
  } catch (error) {
    console.error(error)
    // If error was caused by javascript in one of our query funcs, we need to manually roll back
    // If error was due to bad SQL, rollback has already happened but no harm in doing it again
    await db.query("ROLLBACK;")
    throw error
  }
}

This way, any js errors inside .query() will abort & rollback the transaction safely. e.g.

const queries = [
  () => [
    "INSERT INTO test_transactions (created_at) VALUES(?)",
    [new Date()],
  ],
  (prevResponse) => {
    if (Math.random() > 0.5) {
      throw new Error("I think I'll abort this transaction")
    }
    return ["UPDATE test_transactions SET created_at = ?", [new Date()]]
  },
]
await executeTransaction(queries)
// Queries successfully commit 50% of the time, otherwise will rollback and throw error

Excuse the nonsense query examples. And remember to await db.end() at the end before responding to the network request.

[naorpeled]

Hey @dan-kwiat, good catch!

Will discuss this with Jeremy and if this is greenlit, I'll make sure to implement and release ASAP! (or if anyone else wants to implement the solution they can go for it, please talk to me before doing so :))