Increment does not appear to be atomic

[theolundqvist]

New Issue Checklist

• I am not disclosing a vulnerability.
• I am not just asking a question.
• I have searched through existing issues.
• I can reproduce the issue with the latest version of Parse Server.

Issue Description

obj.increment does not appear to be atomic.

Steps to reproduce

I have the following code in the beforeSave hook of "childClass" and child has a pointer "parent" to "parentClass".

await obj.get("parent").increment("number_children").save(null, {
    useMasterKey: true,
});

Actual Outcome

Creating two child objects simultaneously results in "number_children" increasing from 0 to 1.
This does not happen with:

await obj1.save();
await obj2.save();

but only with

await Promise.all(obj1.save(), obj2.save());

Expected Outcome

I expected the count to increase from 0 to 2.

Environment

Server

• Parse Server version: 6.2.2
• Operating system: MacOS Ventura
• Local or remote host (AWS, Azure, Google Cloud, Heroku, Digital Ocean, etc): local

Database

• System (MongoDB or Postgres): MongoDB
• Database version: 6.0.10
• Local or remote host (MongoDB Atlas, mLab, AWS, Azure, Google Cloud, etc): MongoDB Atlas

Client

• SDK (iOS, Android, JavaScript, PHP, Unity, etc): js
• SDK version: 4.2.0

Logs

[parse-github-assistant]

Thanks for opening this issue!

• 🚀 You can help us to fix this issue faster by opening a pull request with a failing test. See our Contribution Guide for how to make a pull request, or read our New Contributor's Guide if this is your first time contributing.

[mtrezza]

Could you add a test case with that example that you already posted?

[theolundqvist]

I will have to examinate if I am doing anything special on my side. The following test works:

  fit('regression test for #8772 (increment should be atomic))', async () => {
    Parse.Object.disableSingleInstance();

    Parse.Cloud.beforeSave('Parent', (req) => {});
    Parse.Cloud.beforeSave('Child', async (req) => {
      await req.object.get("parent").increment("num_child").save(null, {useMasterKey:true})
    });

    let parent = await new Parse.Object('Parent').save(null, {useMasterKey:true});
    
    const child = () => new Parse.Object('Child').set("parent",parent).save();

    // add synchronously
    await child()
    await child()
    parent = await parent.fetch();
    expect(parent.get('num_child')).toBe(2);

    // add asynchronously
    await Promise.all(Array.from({length: 40}, () => child()))
    parent = await parent.fetch();

    expect(parent.get('num_child')).toBe(42);

[mtrezza]

I's be surprised if no atomic test already exists. If there really is none we can add your test in any case. Could you open a PR?

[theolundqvist]

Actually I can't find any. I'll open a PR

[RahulLanjewar93]

Yea even for me the updates are not atomic

[theolundqvist]

Yea even for me the updates are not atomic

Could you elaborate on this? The test passes. Can you provide an example?

[RahulLanjewar93]

Yea even for me the updates are not atomic

Could you elaborate on this? The test passes. Can you provide an example?

Yea i tried as well the test you added is passing.
I am using multiple instances of parse server and there are cases where I am updating the object from all the servers at sa,e time using increment op.
But it seems the increment isn't atomic when using multiple instances.

[theolundqvist]

@RahulLanjewar93 Why could it be working in parse-server tests but not in our own code?

Did you run the test on your own infrastructure?

The increment operation should be sent directly to mongo so therefore it should always be atomic independent of how many instances you are using? I am only using one instance

[RahulLanjewar93]

Yea, increment ops in mongo are atomic.
I had tested it before in my own infrastructure where i was using multiple instances. Currently I am occupied with something else, i'll have a test again and update on the same.

[mtrezza]

But it seems the increment isn't atomic when using multiple instances.

Although I haven't looked, I doubt that there is anything instance specific in the request that is sent to the DB. Maybe it's a DB issue? Or maybe it's an infrastructure issue where requests are lost, time out, or similar?

Or maybe it is a more complex Parse JS SDK issue where multiple increments / decrements, and object re-fetches in between cause issues with caching? You could add a more complex test to the #8789 where you randomly increment / decrement / re-fetch an object in a loop and check whether the end result is still correct.

[mtrezza]

@RahulLanjewar93 Do you have any update on the issue? You mentioned you observed the same issue, but the test in #8789 passed, so I wonder if this issue here can be closed?

[dblythy]

I was unable to confirm this, here is the passing test case:

it('increment should be atomic', async () => {
    const obj = new TestObject();
    obj.set('count', 0);
    await obj.save();
  
    const headers = {
      'X-Parse-Application-Id': Parse.applicationId,
      'X-Parse-Master-Key': Parse.masterKey,
      'Content-Type': 'application/json',
    };
  
    const promises = [];
    for (let i = 0; i < 100; i++) {
      promises.push(
        fetch(`http://localhost:8378/1/classes/TestObject/${obj.id}`, {
          method: 'PUT',
          headers,
          body: JSON.stringify({
            count: { __op: 'Increment', amount: 1 },
          }),
        })
      );
    }
  
    await Promise.all(promises);
  
    const object = await fetch(`http://localhost:8378/1/classes/TestObject/${obj.id}`, {
      method: 'GET',
      headers,
    });
    const json = await object.json();

    expect(json.count).toBe(100);
  });

[mtrezza]

Can we rule out that multiple Parse Server instances have anything to do with it? It sounds unlikely to me, because it would have to be a bug in MongoDB since the atomicity on multiple requests needs to be guaranteed there.

However, it would be good to verify atomicity on the client side with a more complex test. I have actually observed something similar a while back, but didn't investigate further; the scenario was:

• multiple server instances in-/decrementing simultaneously (may be irrelevant, see above)
• large numbers (~10k) being de/incremented thousands of times
• multiple in-/decrements for multiple objs before saving the objs:

  for (const obj of objs) {
    obj.increment(field, z ? -1 : 1);
  }
  await Parse.Object.saveAll(objs, { useMasterKey: true });
  

Especially the last point makes me think that it could be a Parse JS SDK caching issue. Maybe the issue is the atomicity of these operations within the SDK, not on the DB side. Or there is a spill-over between objects for this operation. A test could look like this:

1. Create an array of 1k random numbers in columns like:

initialFieldValue (random)	increments (ramdon)	decrements (random)	finalFieldValue (calculated)
123	4	8	119 (= 123 + 4 - 8)

2. For each obj, in random order, execute the in- and decrements in random order
3. Save all objs with Parse.Object.saveAll.

[dblythy]

For completeness, I tested this on a seperate server/client to make sure things weren't persisting in memory. Here's the cloud code:

Parse.Cloud.beforeSave('Parent', (req) => {});

Parse.Cloud.beforeSave('Child', async (req) => {
  const parent = req.object.get("parent");

  await new Promise((resolve) => setTimeout(resolve, Math.random() * 100));

  await parent.fetch({ useMasterKey: true });
  parent.increment("num_child");

  await new Promise((resolve) => setTimeout(resolve, Math.random() * 100));
  await parent.save(null, { useMasterKey: true });
});

The additional timeouts increase the likelihood of race conditions.

To test, I added a multi-cluster script:

const cluster = require('cluster');
const os = require('os');
const Parse = require('parse/node');

// Initialize Parse
Parse.initialize('myAppId');
Parse.serverURL = "http://localhost:1337/parse";

if (cluster.isMaster) {
  // Master process
  const numCPUs = os.cpus().length;
  console.log(`Master process is running on PID: ${process.pid}`);
  console.log(`Forking ${numCPUs} workers...`);

  // Create the parent object
  (async () => {
    const parent = new Parse.Object('Parent');
    await parent.save(); // Save the parent object
    const parentId = parent.id;
    console.log(`Parent created with ID: ${parentId}`);

    // Share the parentId with workers via environment variable
    for (let i = 0; i < numCPUs; i++) {
      cluster.fork({ PARENT_ID: parentId });
    }

    // Listen for worker exit events
    cluster.on('exit', (worker, code, signal) => {
      console.log(`Worker ${worker.process.pid} exited with code ${code} (${signal})`);
    });

    setTimeout(async () => {
      // Once all workers are done, fetch the parent data
      await parent.fetch();
        console.log(`Parent has ${parent.get('num_child')} children.`);
    }, 10000);
  })().catch((error) => {
    console.error('Error in master process:', error);
  });
} else {
  // Worker process
  console.log(`Worker process started on PID: ${process.pid}`);

  (async () => {
    const parentId = process.env.PARENT_ID;

    if (!parentId) {
      throw new Error('Parent ID is not available in worker environment.');
    }

    // Retrieve the shared parent object
    const parent = new Parse.Object('Parent');
    parent.id = parentId;

    // Each worker creates 40 child objects
    const promises = Array.from({ length: 40 }, async () => {
      const child = new Parse.Object('Child');
      child.set('parent', parent);
      return child.save();
    });

    await Promise.all(promises);
  })().catch((error) => {
    console.error(`Error in worker ${process.pid}:`, error);
  });
}

On my 8 core device, the result is Parent has 320 children (which is expected if increment is working correctly)

If we update the cloud code to parent.set("num_child", (parent.get("num_child") || 0) + 1);, this should demonstrate a non-atomic operation: Parent has 26 children.

@mtrezza attempted your idea but could not verify either:

const Parse = require('parse/node');

// Initialize Parse
Parse.initialize('myAppId');
Parse.serverURL = "http://localhost:1337/parse";

(async () => {
  try {
    console.log('Starting the test...');

    // Step 1: Create the initial objects
    const numObjects = 200;
    const objects = [];

    for (let i = 0; i < numObjects; i++) {
      const initialFieldValue = Math.floor(Math.random() * 1000); // Random initial value
      const increments = Math.floor(Math.random() * 20); // Random increments
      const decrements = Math.floor(Math.random() * 20); // Random decrements

      const obj = new Parse.Object('TestObject');
      obj.set('initialFieldValue', initialFieldValue);
      obj.set('increments', increments);
      obj.set('decrements', decrements);
      obj.set('finalFieldValue', initialFieldValue + increments - decrements); // Pre-calculated
      obj.set('currentValue', initialFieldValue); // Will be modified during operations

      objects.push(obj);
    }

    await Parse.Object.saveAll(objects);
    console.log(`${numObjects} objects created and saved.`);

    // Step 2: Perform random operations on the objects
    const updateOperations = [];
    for (const obj of objects) {
      const increments = obj.get('increments');
      const decrements = obj.get('decrements');
      const operations = [];

      // Add increment operations
      for (let i = 0; i < increments; i++) {
        operations.push(async () => {
          obj.increment('currentValue', 1);
          await obj.save();
        });
      }

      // Add decrement operations
      for (let i = 0; i < decrements; i++) {
        operations.push(async () => {
          obj.increment('currentValue', -1);
          await obj.save();
        });
      }

      // Randomize the order of operations
      operations.sort(() => Math.random() - 0.5);
      updateOperations.push(...operations);
    }

    // Execute all operations in random order
    console.log('Executing random operations...');
    await Promise.all(updateOperations.map((op) => op()));

    // Step 3: Validate the final values
    console.log('Validating final values...');
    const query = new Parse.Query('TestObject');
    const updatedObjects = await query.find();

    updatedObjects.forEach((obj) => {
      const finalFieldValue = obj.get('finalFieldValue');
      const currentValue = obj.get('currentValue');

      if (currentValue !== finalFieldValue) {
        console.error(
          `Validation failed for object ${obj.id}: Expected ${finalFieldValue}, but got ${currentValue}`
        );
      }
    });

    console.log('Validation complete. Test finished.');
  } catch (error) {
    console.error('Error during test:', error);
  }
})();

[mtrezza]

You tried all combinations I can think of. Maybe we really can close this issue for now, since we cannot reproduce it. It's just strange that @RahulLanjewar93 confirmed the issue that @theolundqvist observed, that's very suspicious. If we close the issue, then we can at least add the tests from this discussion to the CI.

[RahulLanjewar93]

Give me a day or two, will look into this again.
Was occupied with other stuff, so missed this thread.
Let me try to reproduce it
Sorry for the delay. :)

[mtrezza]

That would be fantastic, @RahulLanjewar93 !

[RahulLanjewar93]

Even I am not able to reproduce it. :(
But I am sure it was there in our codebase and which is why we had to switch from Parse increment to Mongo increment instead.

I think its best to close this issue for now.
We can reopen if we find more details @mtrezza

[mtrezza]

Just to clarify, you switched from increment via Parse.Object.increment to a MongoDB increment operation calling the MongoDB adapter in Parse Server directly, and the issue went away immediately without any other code changes?

[RahulLanjewar93]

Just to clarify, you switched from increment via Parse.Object.increment to a MongoDB increment operation calling the MongoDB adapter in Parse Server directly, and the issue went away immediately without any other code changes?

Yeah you're right, didn't need any other code change.

I was not using the internal mongoDB adapter. We had our own mongodb instance as well, we used that

[mtrezza]

Got it; too bad we have to close this as unresolved as it is for now. If you find you anything, please let us know and we can re-open.