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

Transaction retries corrupt subdocument arrays #14340

Closed
2 tasks done
juona opened this issue Feb 7, 2024 · 4 comments · Fixed by #14346
Closed
2 tasks done

Transaction retries corrupt subdocument arrays #14340

juona opened this issue Feb 7, 2024 · 4 comments · Fixed by #14346
Labels
confirmed-bug We've confirmed this is a bug in Mongoose and will fix it.
Milestone

Comments

@juona
Copy link

juona commented Feb 7, 2024

Prerequisites

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

Mongoose version

8.1.1

Node.js version

18.18.2

MongoDB server version

5

Typescript version (if applicable)

No response

Description

Hello, me again!

  1. A document has a subdocument array.
  2. The array is modified (namely, push, pull or splice is used).
  3. The document is saved inside a transaction.
  4. The transaction is retried at least one (I mean the whole transaction as opposed to just the commit).

Result: the whole array is saved incorrectly and can even become corrupted, in turn ruining the whole document.

I noticed that in more recent versions document arrays are actually proxy objects, and my guess is that this might be causing problems, especially when using .pull()/.splice().

Steps to Reproduce

import mongoose from "mongoose";
import { MongoServerError, MongoErrorLabel } from "mongodb";

console.log("connecting...");

// Connecting to a *ReplicaSet* specifically to make sure that transactions work correctly.
// Nevertheless, whether or not an actual ReplicaSet is used has no effect on the issue.
await mongoose.connect(
  "mongodb://mongo-node1:27017,mongo-node2:27018,mongo-node3:27019/xxx?replicaSet=eof"
);

console.log("connected!");

// Define some schemas...
const subItemSchema = new mongoose.Schema(
  {
    name: { type: String, required: true },
  },
  { _id: false }
);

const itemSchema = new mongoose.Schema(
  {
    name: { type: String, required: true },
    subItems: { type: [subItemSchema], required: true },
  },
  { _id: false }
);

const schema = new mongoose.Schema({
  items: { type: [itemSchema], required: true },
});

// ...and a model
const Model = mongoose.model("MyModel", schema);

// Clear the collection...
await Model.deleteMany({});

// ...and create one document
await Model.create({
  items: [
    { name: "test1", subItems: [{ name: "x1" }] },
    { name: "test2", subItems: [{ name: "x2" }] },
  ],
});

const doc = await Model.findOne();

// Array modification - choose one...
// Works okay, but I guess that's because it's an idempotent operation
doc.items.addToSet({ name: "test3", subItems: [{ name: "x3" }] });
// Adds unexpected extra entries
doc.items.push({ name: "test3", subItems: [{ name: "x3" }] });
// Corrupts the document. It seems that moving this inside the `.transaction()` callback solves the problem
doc.items.pull(doc.items[0]);
// Corrupts the document
doc.items.splice(0, 1);

let attempt = 0;

await mongoose.connection.transaction(async (session) => {
  console.log(`Attempt: ${attempt}`);

  await doc.save({ session });

  // This is the important bit. Uncomment this to trigger a transaction retry.
  // if (attempt === 0) {
  //   attempt += 1;
  //   throw new MongoServerError({
  //     message: "Test transient transaction failures & retries",
  //     errorLabels: [MongoErrorLabel.TransientTransactionError],
  //   });
  // }
});

const updatedDoc = await Model.findOne();

// See the structure here. Depending on the array operation, the structure may be different,
// but it's always wrong.
console.log(updatedDoc.items);

// Bye bye!
await mongoose.disconnect();

Expected Behavior

If .push() is used

The total number of documents added to the array grows exponentially with every retry - it is 2^n, where n is the number of retries (forgive me, can't go without a lol here :)).

Expectation: only one document is added.

If .pull() or .splice() is used

The document that is saved in the database looks like this:

{
  "_id": {
    "$oid": "65c40f6c0975fbad509f0aa8"
  },
  "items": {
    "0": {
      "name": "test2",
      "subItems": [
        {
          "name": "x2"
        }
      ]
    }
  },
  "__v": 0
}

As you can see, the array got converted into an object, and that's why I'm throwing suspicious looks at the Proxy object used for document array.

@IslandRhythms IslandRhythms added the has repro script There is a repro script, the Mongoose devs need to confirm that it reproduces the issue label Feb 9, 2024
@vkarpov15 vkarpov15 added confirmed-bug We've confirmed this is a bug in Mongoose and will fix it. and removed has repro script There is a repro script, the Mongoose devs need to confirm that it reproduces the issue labels Feb 9, 2024
@vkarpov15 vkarpov15 added this to the 8.1.3 milestone Feb 9, 2024
@vkarpov15
Copy link
Collaborator

I did some digging and found that the lib/plugins/trackTransactions.js file's mergeAtomics() is responsible for the conversion of array to object. However, even with that fix, the array contents are wrong, so I'll keep debugging and see what's causing this.

@juona
Copy link
Author

juona commented Feb 9, 2024

Any chance to add this to v7? : ]

@vkarpov15
Copy link
Collaborator

@juona yep we will backport

vkarpov15 added a commit that referenced this issue Feb 14, 2024
Avoid corrupting $set-ed arrays when transaction error occurs
@jmikrut
Copy link

jmikrut commented Feb 15, 2024

Hey @vkarpov15 — we are seeing some strange behavior that I think is related to this issue.

Only in Jest, and on Mongoose v6.12.3, our subdocument arrays are being transformed into objects like the OP posted.

We're seeing this when using the findOneAndUpdate method. Probably happening elsewhere too, but that's where we're seeing it now. We do use transactions, but only when replica sets are available, and our testing does not implement any type of transactions at all.

Does this ring any bells for you?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug We've confirmed this is a bug in Mongoose and will fix it.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants