Snapshot and reset document modified state for custom transaction wrappers

[T-parrish]

Prerequisites

• I have written a descriptive issue title
• I have searched existing issues to ensure the bug has not already been reported

Mongoose version

7.6.4

Node.js version

18.17

MongoDB server version

5.9.1

Typescript version (if applicable)

No response

Description

While running a transaction that uses model.save({session}) to create one document and update another, we catch a transaction error due to a write conflict. After aborting the transaction and ending the session, we attempt to rerun the entire transaction and noticed some unexpected behavior.

Upon inspecting the server logs for the first pass of the transaction, here is what we see:

1. ModelA.save({session}) results in an 'insert' command
2. ModelB.save({session}) results in an 'update' command
3. The call to database_session.commitTransaction() fails due to write conflict
4. We abort the transaction, end the session, and retry everything with a new session after a short wait period

From the server logs, the second pass of the transaction behaves differently:

1. ModelA.save({session}) results in an 'insert' command
2. ModelB.save({session}) results in a 'findOne' command
3. The transaction succeeds, but the updates to ModelB are not persisted.

Looking through the docs, it doesn't seem like Model.save({session}) should ever resolve to a findOne() operation, but that appears to be what's happening. Is there something that I'm missing with how Model.save() is expected to behave after retrying a full transaction? How can we guarantee that ModelB.save({session}) will always attempt to update the target document?

Steps to Reproduce

Code for the Transaction Wrapper:

function wait(ms) {
  console.log(`Waiting for ${ms} milliseconds...`);
  return new Promise((resolve) => setTimeout(resolve, ms));
}

class TransactionBuilder {
  constructor(conn = null, max_retries = 3) {
    this.conn = conn;
    this.operations = [];
    this.max_retries = max_retries;
    this.retry = 0;
  }

  static async create({ mongoose_connection }) {
    return new TransactionBuilder(mongoose_connection);
  }

  addOperation(callback) {
    this.operations.push(callback);
  }

  async run(
    transaction_options = {
      writeConcern: {
        w: WRITE_CONCERN_LEVEL.MAJORITY,
        j: true,
      },
      readConcern: READ_CONCERN_LEVEL.SNAPSHOT,
    }
  ) {
    const database_session = await this.conn.startSession();
    database_session.startTransaction(transaction_options);

    for (const operation of this.operations) {
      try {
        loggerUtils
          .getLogger()
          .info(`TransactionBuilder: About to run operation...${operation}`);
        const result = await operation(database_session);
        loggerUtils
          .getLogger()
          .info(`Result TransactionBuilder: ${JSON.stringify(result)}`);
      } catch (error) {
        loggerUtils
          .getLogger()
          .error(
            `Error executing operation in TransactionBuilder: ${JSON.stringify(
              error
            )}`
          );
        await database_session.abortTransaction();
        await database_session.endSession();
        throw error;
      }
    }
    try {
      await database_session.commitTransaction();
      console.log('Transaction committed successfully');
      await database_session.endSession();
    } catch (error) {
      loggerUtils
        .getLogger()
        .error(
          `Unable to commit transaction on attempt ${
            this.retry + 1
          }: ${JSON.stringify(error)}`
        );
      await database_session.endSession();

      if (this.retry < this.max_retries) {
        this.retry++;
        const delay = this.retry ** Math.random(0, 1).toFixed(4);
        await wait(delay * 1000);
        console.log('Retrying transaction...');
        try {
          await this.run();
        } catch (error) {
          console.log(`Failed retry: ${error}`);
        }
        console.log('Transaction complete');
      } else {
        loggerUtils
          .getLogger()
          .error(
            `Failed to commit transaction after ${
              this.retry + 1
            } attempts: ${JSON.stringify(error)}`
          );
        throw error;
      }
    }
  }
}

The two operations for the transaction that are retried:

const transaction_builder = await TransactionBuilder.create({
  mongoose_connection: mongoose.connection,
});

transaction_builder.addOperation(async (session) => {
  const audit_log = new AuditLog(new_company_audit_log);
  await audit_log.save({ session });
});

transaction_builder.addOperation(async (session) => {
  await company.save({ session });
});

await transaction_builder.run();

Expected Behavior

During each attempt at running the transaction, the calls to ModelB.save({session}) should trigger updates to the target document and persist after the transaction is complete.

[T-parrish]

A few more updates: after running some more tests it seems like this issue may be related to Document state not resetting between transaction retries.

If we modify the the second operation to force a field to be 'modified', everything works as expected:

transaction_builder.addOperation(async (session) => {
   console.log('updating in banking config');
   company.markModified('features.banking_config');
   await company.save({ session });
 });

However, my assumption is that this will only apply updates to that specific field; to ensure that all updates are applied properly, we would need to diff the og object with the updated object and mark each updated field as 'modified' for each subsequent transaction retry.

So, a couple questions:

1. Why does the document state not fully revert when a transaction aborts and 'rolls back'?
2. What's the best way to ensure that the document remembers what fields have been modified between transaction retries?
3. How can a call to document.save() resolve to a findOne() operation? Is this documented anywhere?

Thank you kindly,
Taylor

[vkarpov15]

I managed to create a more simplified repro. The issue is that Mongoose's document state doesn't revert when the transaction aborts, and save() becomes a findOne() if there's no changes to save.

const mongoose = require('mongoose');

mongoose.set('debug', true);

main().catch(error => {
  console.error(error);
  process.exit(-1);
});

async function main() {
  await mongoose.connect('mongodb://127.0.0.1:27017,127.0.0.1:27018/mongoose_test?replicaSet=rs');

  const Test = mongoose.model('Test', mongoose.Schema({ name: String }));
  await Test.deleteMany({});
  const { _id } = await Test.create({ name: 'foo' });

  const doc = await Test.findById(_id);
  doc.name = 'bar';
  for (let i = 0; i < 2; ++i) {
    const session = await mongoose.startSession();
    session.startTransaction();

    await doc.save({ session });
    if (i === 0) {
      await session.abortTransaction();
    } else {
      await session.commitTransaction();
    }
    await session.endSession();
  }

  console.log(await Test.findById(_id)); // Still prints name: 'foo'
}

The easiest workaround is to use Mongoose's transaction() helper, which tracks and resets document state if the transaction errors.

Another workaround is to avoid modifying documents that are external to the transaction. For example, the following works fine:

  for (let i = 0; i < 2; ++i) {
    const session = await mongoose.startSession();
    session.startTransaction();

    const doc = await Test.findById(_id);
    doc.name = 'bar';

    await doc.save({ session });
    if (i === 0) {
      await session.abortTransaction();
    } else {
      await session.commitTransaction();
    }
    await session.endSession();
  }

Do either of these approaches work for you?

[github-actions]

This issue is stale because it has been open 14 days with no activity. Remove stale label or comment or this will be closed in 5 days

[brandonalfred]

Bumping this for visibility!

[vkarpov15]

@brandonalfred do you have any thoughts on this comment?

[github-actions]

This issue is stale because it has been open 14 days with no activity. Remove stale label or comment or this will be closed in 5 days

[hardcodet]

@vkarpov15

you suggested this as an alternative:

  for (let i = 0; i < 2; ++i) {
    const session = await mongoose.startSession();
    session.startTransaction();

    const doc = await Test.findById(_id);
    doc.name = 'bar';

    await doc.save({ session });
    if (i === 0) {
      await session.abortTransaction();
    } else {
      await session.commitTransaction();
    }
    await session.endSession();
  }

I don't think that's a viable approach to this problem. You are basically just repeating the whole thing twice, so that works of course, since the first and second attempt are not related at all, and work on independent entities. But that's not recovering from a failed transaction, but performing a full retry (which will include business logic etc. that may/should take place elsewhere).

[vkarpov15]

That's one of 2 alternatives I suggested, the other being to use Mongoose's transaction() helper, which handles reverting document state on transaction failure. If fetching in the transaction doesn't work for you, then try using the transaction() helper.

I would love to have an api for this that lets you snapshot the state of a document and then later revert back to that state. Would be handy for cases where you want to handle transactions yourself instead of relying on transaction(). Would that help?

[hardcodet]

@vkarpov15 The transaction helper is not always feasible for us, since it clashes with our own patterns. We introduced a wrapper around a session that we can pass around, and allows unrelated services to partake in transactional work.

As outlined in #14460, a fully transparent handling would be greatly preferrable to us. Given that the workaround we did already was super simple (which is basically also just a dumb snapshot), we were hoping that providing that transparently on the framework level would be equally easy for you without sacrificing performance or making the API more complex.

I assume clearing the change tracking after a transaction commit rather than doing it right away is not an option (in order to properly handle multiple updates on a document during a transaction)? But even then, just a simple map of performed changes might already give you everything you need for a quick rollback (you'd again have to consider the possibility of multiple updates of the same document there).

But I assume that's what the transaction helper already does behind the scenes?

[vkarpov15]

Yeah the transaction helper does this behind the scenes. Unfortunately there's no good way for Mongoose to know when a transaction is committed vs aborted by just observing operations AFAIK, which is why Mongoose has its own transaction wrapper: so Mongoose can store document state when transaction starts and revert before transaction retry. Best we can do is provide an API for custom transaction wrappers to do this snapshotting and restarting themsevles.

[hardcodet]

Ah, I see where you are coming from! A hook would be already good enough - we'd just add that to our wrapper and be done with it :)

Food for thought for a transparent solution - here's how we create our wrapper:

I assume the problem is that the returned session object is a MongoDB object, which causes you to loose the possibility to intercept subsequent calls that object (e.g. commitTransaction). What if mongoose.startSession just returned a decorator session object there that you control?

[vkarpov15]

Creating a wrapper around MongoDB ClientSession would be a bit unwieldy because we would need to figure out a way for the MongoDB Node driver to handle the case when someone calls findOne({}, { session }) with a Mongoose session wrapper as opposed to a ClientSession. We could consider creating a MongooseClientSession that extends the MongoDB Node driver's ClientSession class, but I am less confident in that approach because we would need to make sure MongooseClientSession is always compatible with ClientSession.

Plus I think the snapshotting and resetting functionality could be useful in other cases.