Merge branch 'double_cron_fix' into develop

test/api/v3/integration/user/GET-user_anonymized.test.js

@@ -13,7 +13,7 @@ describe('GET /user/anonymized', () => {
 
   before(async () => {
     user = await generateUser();
-    await user.update({ newMessages: ['some', 'new', 'messages'], profile: 'profile', 'purchased.plan': 'purchased plan',
+    await user.update({ newMessages: ['some', 'new', 'messages'], 'profile.name': 'profile', 'purchased.plan': 'purchased plan',
                         contributor: 'contributor', invitations: 'invitations', 'items.special.nyeReceived': 'some', 'items.special.valentineReceived': 'some',
                         webhooks: 'some', 'achievements.challenges': 'some',
                         'inbox.messages': [{ text: 'some text' }],

test/api/v3/unit/libs/cron.test.js

@@ -1,6 +1,7 @@
 /* eslint-disable global-require */
 import moment from 'moment';
-import { cron } from '../../../../../website/server/libs/api-v3/cron';
+import Bluebird from 'bluebird';
+import { recoverCron, cron } from '../../../../../website/server/libs/api-v3/cron';
 import { model as User } from '../../../../../website/server/models/user';
 import * as Tasks from '../../../../../website/server/models/task';
 import { clone } from 'lodash';
@@ -34,15 +35,6 @@ describe('cron', () => {
     };
   });
 
-  it('updates user.auth.timestamps.loggedin and lastCron', () => {
-    let now = new Date();
-
-    cron({user, tasksByType, daysMissed, analytics, now});
-
-    expect(user.auth.timestamps.loggedin).to.equal(now);
-    expect(user.lastCron).to.equal(now);
-  });
-
   it('updates user.preferences.timezoneOffsetAtLastCron', () => {
     let timezoneOffsetFromUserPrefs = 1;
 
@@ -571,3 +563,68 @@ describe('cron', () => {
     });
   });
 });
+
+describe('recoverCron', () => {
+  let locals, status, execStub;
+
+  beforeEach(() => {
+    execStub = sandbox.stub();
+    sandbox.stub(User, 'findOne').returns({ exec: execStub });
+
+    status = { times: 0 };
+    locals = {
+      user: new User({
+        auth: {
+          local: {
+            username: 'username',
+            lowerCaseUsername: 'username',
+            email: 'email@email.email',
+            salt: 'salt',
+            hashed_password: 'hashed_password', // eslint-disable-line camelcase
+          },
+        },
+      }),
+    };
+  });
+
+  afterEach(() => {
+    sandbox.restore();
+  });
+
+  it('throws an error if user cannot be found', async (done) => {
+    execStub.returns(Bluebird.resolve(null));
+
+    try {
+      await recoverCron(status, locals);
+    } catch (err) {
+      expect(err.message).to.eql(`User ${locals.user._id} not found while recovering.`);
+
+      done();
+    }
+  });
+
+  it('increases status.times count and reruns up to 3 times', async (done) => {
+    execStub.returns(Bluebird.resolve({_cronSignature: 'RUNNING_CRON'}));
+    execStub.onCall(3).returns(Bluebird.resolve({_cronSignature: 'NOT_RUNNING'}));
+
+    await recoverCron(status, locals);
+
+    expect(status.times).to.eql(3);
+    expect(locals.user).to.eql({_cronSignature: 'NOT_RUNNING'});
+
+    done();
+  });
+
+  it('throws an error if recoverCron runs 4 times', async (done) => {
+    execStub.returns(Bluebird.resolve({_cronSignature: 'RUNNING_CRON'}));
+
+    try {
+      await recoverCron(status, locals);
+    } catch (err) {
+      expect(status.times).to.eql(4);
+      expect(err.message).to.eql(`Impossible to recover from cron for user ${locals.user._id}.`);
+
+      done();
+    }
+  });
+});

test/api/v3/unit/middlewares/cronMiddleware.js

@@ -1,26 +1,26 @@
 import {
   generateRes,
   generateReq,
-  generateNext,
   generateTodo,
   generateDaily,
 } from '../../../../helpers/api-unit.helper';
+import { cloneDeep } from 'lodash';
 import cronMiddleware from '../../../../../website/server/middlewares/api-v3/cron';
 import moment from 'moment';
 import { model as User } from '../../../../../website/server/models/user';
 import { model as Group } from '../../../../../website/server/models/group';
 import * as Tasks from '../../../../../website/server/models/task';
 import analyticsService from '../../../../../website/server/libs/api-v3/analyticsService';
+import * as cronLib from '../../../../../website/server/libs/api-v3/cron';
 import { v4 as generateUUID } from 'uuid';
 
 describe('cron middleware', () => {
-  let res, req, next;
+  let res, req;
   let user;
 
-  beforeEach(() => {
+  beforeEach((done) => {
     res = generateRes();
     req = generateReq();
-    next = generateNext();
     user = new User({
       auth: {
         local: {
@@ -33,24 +33,31 @@ describe('cron middleware', () => {
       },
     });
 
-    user._statsComputed = {
+    user.save()
+    .then(savedUser => {
+      savedUser._statsComputed = {
         mp: 10,
         maxMP: 100,
       };
 
-    res.locals.user = user;
+      res.locals.user = savedUser;
       res.analytics = analyticsService;
+      done();
+    })
+    .catch(done);
   });
 
-  it('calls next when user is not attached', () => {
+  afterEach(() => {
+    sandbox.restore();
+  });
+
+  it('calls next when user is not attached', (done) => {
     res.locals.user = null;
-    cronMiddleware(req, res, next);
-    expect(next).to.be.calledOnce;
+    cronMiddleware(req, res, (err) => done(err));
   });
 
-  it('calls next when days have not been missed', () => {
-    cronMiddleware(req, res, next);
-    expect(next).to.be.calledOnce;
+  it('calls next when days have not been missed', (done) => {
+    cronMiddleware(req, res, (err) => done(err));
   });
 
   it('should clear todos older than 30 days for free users', async (done) => {
@@ -59,35 +66,37 @@ describe('cron middleware', () => {
     task.dateCompleted = moment(new Date()).subtract({days: 31});
     task.completed = true;
     await task.save();
+    await user.save();
 
-    cronMiddleware(req, res, () => {
-      Tasks.Task.findOne({_id: task}, function (err, taskFound) {
-        expect(err).to.not.exist;
+    cronMiddleware(req, res, (err) => {
+      Tasks.Task.findOne({_id: task}, function (secondErr, taskFound) {
+        expect(secondErr).to.not.exist;
         expect(taskFound).to.not.exist;
-        done();
+        done(err);
       });
     });
   });
 
-  it('should not clear todos older than 30 days for subscribed users', (done) => {
+  it('should not clear todos older than 30 days for subscribed users', async (done) => {
     user.purchased.plan.customerId = 'subscribedId';
     user.purchased.plan.dateUpdated = moment('012013', 'MMYYYY');
     user.lastCron = moment(new Date()).subtract({days: 2});
     let task = generateTodo(user);
     task.dateCompleted = moment(new Date()).subtract({days: 31});
     task.completed = true;
-    task.save();
+    await task.save();
+    await user.save();
 
-    cronMiddleware(req, res, () => {
-      Tasks.Task.findOne({_id: task}, function (err, taskFound) {
-        expect(err).to.not.exist;
+    cronMiddleware(req, res, (err) => {
+      Tasks.Task.findOne({_id: task}, function (secondErr, taskFound) {
+        expect(secondErr).to.not.exist;
         expect(taskFound).to.exist;
-        done();
+        done(err);
       });
     });
   });
 
-  it('should clear todos older than 90 days for subscribed users', (done) => {
+  it('should clear todos older than 90 days for subscribed users', async (done) => {
     user.purchased.plan.customerId = 'subscribedId';
     user.purchased.plan.dateUpdated = moment('012013', 'MMYYYY');
     user.lastCron = moment(new Date()).subtract({days: 2});
@@ -95,46 +104,60 @@ describe('cron middleware', () => {
     let task = generateTodo(user);
     task.dateCompleted = moment(new Date()).subtract({days: 91});
     task.completed = true;
-    task.save();
+    await task.save();
+    await user.save();
 
-    cronMiddleware(req, res, () => {
-      Tasks.Task.findOne({_id: task}, function (err, taskFound) {
-        expect(err).to.not.exist;
+    cronMiddleware(req, res, (err) => {
+      Tasks.Task.findOne({_id: task}, function (secondErr, taskFound) {
+        expect(secondErr).to.not.exist;
         expect(taskFound).to.not.exist;
-        done();
+        done(err);
       });
     });
   });
 
-  it('should call next is user was not modified after cron', (done) => {
+  it('should call next if user was not modified after cron', async (done) => {
     let hpBefore = user.stats.hp;
     user.lastCron = moment(new Date()).subtract({days: 2});
+    await user.save();
 
-    user.save().then(function () {
-      cronMiddleware(req, res, function () {
+    cronMiddleware(req, res, (err) => {
       expect(hpBefore).to.equal(user.stats.hp);
-        done();
-      });
+      done(err);
     });
   });
 
-  it('does damage for missing dailies', (done) => {
+  it('updates user.auth.timestamps.loggedin and lastCron', async (done) => {
+    user.lastCron = moment(new Date()).subtract({days: 2});
+    let now = new Date();
+    await user.save();
+
+    cronMiddleware(req, res, (err) => {
+      expect(moment(now).isSame(user.lastCron, 'day'));
+      expect(moment(now).isSame(user.auth.timestamps.loggedin, 'day'));
+      done(err);
+    });
+  });
+
+  it('does damage for missing dailies', async (done) => {
     let hpBefore = user.stats.hp;
     user.lastCron = moment(new Date()).subtract({days: 2});
     let daily = generateDaily(user);
     daily.startDate = moment(new Date()).subtract({days: 2});
-    daily.save();
+    await daily.save();
+    await user.save();
 
-    cronMiddleware(req, res, () => {
+    cronMiddleware(req, res, (err) => {
       expect(user.stats.hp).to.be.lessThan(hpBefore);
-      done();
+      done(err);
     });
   });
 
-  it('updates tasks', (done) => {
+  it('updates tasks', async (done) => {
     user.lastCron = moment(new Date()).subtract({days: 2});
     let todo = generateTodo(user);
     let todoValueBefore = todo.value;
+    await user.save();
 
     cronMiddleware(req, res, () => {
       Tasks.Task.findOne({_id: todo._id}, function (err, todoFound) {
@@ -150,7 +173,7 @@ describe('cron middleware', () => {
     user.lastCron = moment(new Date()).subtract({days: 2});
     let daily = generateDaily(user);
     daily.startDate = moment(new Date()).subtract({days: 2});
-    daily.save();
+    await daily.save();
 
     let questKey = 'dilatory';
     user.party.quest.key = questKey;
@@ -174,4 +197,28 @@ describe('cron middleware', () => {
       done();
     });
   });
+
+  it('recovers from failed cron and does not error when user is already cronning', async (done) => {
+    user.lastCron = moment(new Date()).subtract({days: 2});
+    await user.save();
+
+    let updatedUser = cloneDeep(user);
+    updatedUser.nMatched = 0;
+
+    sandbox.spy(cronLib, 'recoverCron');
+
+    sandbox.stub(User, 'update')
+      .withArgs({ _id: user._id, _cronSignature: 'NOT_RUNNING' })
+      .returns({
+        exec () {
+          return Promise.resolve(updatedUser);
+        },
+      });
+
+    cronMiddleware(req, res, () => {
+      expect(cronLib.recoverCron).to.be.calledOnce;
+
+      done();
+    });
+  });
 });

website/server/controllers/api-v3/groups.js

@@ -117,9 +117,6 @@ api.getGroups = {
 api.getGroup = {
   method: 'GET',
   url: '/groups/:groupId',
-  // Disable cron when getting groups to avoid race conditions when the site is loaded
-  // and requests for party and user data are concurrent
-  runCron: false,
   middlewares: [authWithHeaders()],
   async handler (req, res) {
     let user = res.locals.user;

website/server/libs/api-v3/cron.js

@@ -1,4 +1,6 @@
 import moment from 'moment';
+import Bluebird from 'bluebird';
+import { model as User } from '../../models/user';
 import common from '../../../../common/';
 import { preenUserHistory } from '../../libs/api-v3/preening';
 import _ from 'lodash';
@@ -81,12 +83,33 @@ function performSleepTasks (user, tasksByType, now) {
   });
 }
 
+export async function recoverCron (status, locals) {
+  let {user} = locals;
+
+  await Bluebird.delay(300);
+
+  let reloadedUser = await User.findOne({_id: user._id}).exec();
+
+  if (!reloadedUser) {
+    throw new Error(`User ${user._id} not found while recovering.`);
+  } else if (reloadedUser._cronSignature !== 'NOT_RUNNING') {
+    status.times++;
+
+    if (status.times < 4) {
+      await recoverCron(status, locals);
+    } else {
+      throw new Error(`Impossible to recover from cron for user ${user._id}.`);
+    }
+  } else {
+    locals.user = reloadedUser;
+    return null;
+  }
+}
+
 // Perform various beginning-of-day reset actions.
 export function cron (options = {}) {
   let {user, tasksByType, analytics, now = new Date(), daysMissed, timezoneOffsetFromUserPrefs} = options;
 
-  user.auth.timestamps.loggedin = now;
-  user.lastCron = now;
   user.preferences.timezoneOffsetAtLastCron = timezoneOffsetFromUserPrefs;
   // User is only allowed a certain number of drops a day. This resets the count.
   if (user.items.lastDrop.count > 0) user.items.lastDrop.count = 0;

website/server/middlewares/api-v3/cron.js

@@ -5,24 +5,24 @@ import * as Tasks from '../../models/task';
 import Bluebird from 'bluebird';
 import { model as Group } from '../../models/group';
 import { model as User } from '../../models/user';
-import { cron } from '../../libs/api-v3/cron';
+import { recoverCron, cron } from '../../libs/api-v3/cron';
+import { v4 as uuid } from 'uuid';
 
 const daysSince = common.daysSince;
 
-module.exports = function cronMiddleware (req, res, next) {
+async function cronAsync (req, res) {
   let user = res.locals.user;
-
-  if (!user) return next(); // User might not be available when authentication is not mandatory
+  if (!user) return null; // User might not be available when authentication is not mandatory
 
   let analytics = res.analytics;
-
   let now = new Date();
 
+  try {
     // If the user's timezone has changed (due to travel or daylight savings),
     // cron can be triggered twice in one day, so we check for that and use
     // both timezones to work out if cron should run.
     // CDS = Custom Day Start time.
-  let timezoneOffsetFromUserPrefs = user.preferences.timezoneOffset || 0;
+    let timezoneOffsetFromUserPrefs = user.preferences.timezoneOffset;
     let timezoneOffsetAtLastCron = _.isFinite(user.preferences.timezoneOffsetAtLastCron) ? user.preferences.timezoneOffsetAtLastCron : timezoneOffsetFromUserPrefs;
     let timezoneOffsetFromBrowser = Number(req.header('x-user-timezoneoffset'));
     timezoneOffsetFromBrowser = _.isFinite(timezoneOffsetFromBrowser) ? timezoneOffsetFromBrowser : timezoneOffsetFromUserPrefs;
@@ -96,17 +96,37 @@ module.exports = function cronMiddleware (req, res, next) {
       }
     }
 
-  if (daysMissed <= 0) return next();
+    if (daysMissed <= 0) {
+      if (user.isModified()) await user.save();
+      return null;
+    }
 
-  // Fetch active tasks (no completed todos)
-  Tasks.Task.find({
+    let _cronSignature = uuid();
+
+    // To avoid double cron we first set _cronSignature to now and then check that it's not changed while processing
+    let userUpdateResult = await User.update({
+      _id: user._id,
+      _cronSignature: 'NOT_RUNNING', // Check that in the meantime another cron has not started
+    }, {
+      $set: {
+        _cronSignature,
+      },
+    }).exec();
+
+    // If the cron signature is already set, cron is running in another request
+    // throw an error and recover later,
+    if (userUpdateResult.nMatched === 0 || userUpdateResult.nModified === 0) {
+      throw new Error('CRON_ALREADY_RUNNING');
+    }
+
+    let tasks = await Tasks.Task.find({
       userId: user._id,
       $or: [ // Exclude completed todos
         {type: 'todo', completed: false},
         {type: {$in: ['habit', 'daily', 'reward']}},
       ],
-  }).exec()
-  .then(tasks => {
+    }).exec();
+
     let tasksByType = {habits: [], dailys: [], todos: [], rewards: []};
     tasks.forEach(task => tasksByType[`${task.type}s`].push(task));
 
@@ -125,11 +145,7 @@ module.exports = function cronMiddleware (req, res, next) {
       'challenge.id': {$exists: false},
     }).exec();
 
-    let ranCron = user.isModified();
-    let quest = common.content.quests[user.party.quest.key];
-
-    if (ranCron) res.locals.wasModified = true; // TODO remove after v2 is retired
-    if (!ranCron) return next();
+    res.locals.wasModified = true; // TODO remove after v2 is retired
 
     // Group.tavernBoss(user, progress);
 
@@ -138,23 +154,65 @@ module.exports = function cronMiddleware (req, res, next) {
     tasks.forEach(task => {
       if (task.isModified()) toSave.push(task.save());
     });
+    await Bluebird.all(toSave);
 
-    return Bluebird.all(toSave)
-    .then(saved => {
-      user = res.locals.user = saved[0];
-      if (!quest) return;
+    let quest = common.content.quests[user.party.quest.key];
+
+    if (quest) {
       // If user is on a quest, roll for boss & player, or handle collections
       let questType = quest.boss ? 'boss' : 'collect';
-      // TODO this saves user, runs db updates, loads user. Is there a better way to handle this?
-      return Group[`${questType}Quest`](user, progress)
-      .then(() => User.findById(user._id).exec()) // fetch the updated user...
-      .then(updatedUser => {
-        res.locals.user = updatedUser;
+      await Group[`${questType}Quest`](user, progress);
+    }
 
+    // Set _cronSignature, lastCron and auth.timestamps.loggedin to signal end of cron
+    await User.update({
+      _id: user._id,
+    }, {
+      $set: {
+        _cronSignature: 'NOT_RUNNING',
+        lastCron: now,
+        'auth.timestamps.loggedin': now,
+      },
+    }).exec();
+
+    // Reload user
+    res.locals.user = await User.findOne({_id: user._id}).exec();
     return null;
-      });
+  } catch (err) {
+    // If cron was aborted for a race condition try to recover from it
+    if (err.message === 'CRON_ALREADY_RUNNING') {
+      // Recovering after abort, wait 300ms and reload user
+      // do it for max 4 times then reset _cronSignature so that it doesn't prevent cron from running
+      // at the next request
+      let recoveryStatus = {
+        times: 0,
+      };
+
+      recoverCron(recoveryStatus, res.locals);
+    } else {
+      // For any other error make sure to reset _cronSignature so that it doesn't prevent cron from running
+      // at the next request
+      try {
+        await User.update({
+          _id: user._id,
+        }, {
+          _cronSignature: 'NOT_RUNNING',
+        }).exec();
+
+        throw err; // re-throw original error
+      } catch (secondErr) {
+        throw secondErr;
+      }
+    }
+  }
+}
+
+module.exports = function cronMiddleware (req, res, next) {
+  cronAsync(req, res)
+    .then(() => {
+      next();
     })
-    .then(() => next())
-    .catch(next);
+    .catch(err => {
+      next(err);
     });
 };

website/server/models/group.js

@@ -25,6 +25,9 @@ const Schema = mongoose.Schema;
 export const INVITES_LIMIT = 100;
 export const TAVERN_ID = shared.TAVERN_ID;
 
+const CRON_SAFE_MODE = nconf.get('CRON_SAFE_MODE') === 'true';
+const CRON_SEMI_SAFE_MODE = nconf.get('CRON_SEMI_SAFE_MODE') === 'true';
+
 // NOTE once Firebase is enabled any change to groups' members in MongoDB will have to be run through the API
 // changes made directly to the db will cause Firebase to get out of sync
 export let schema = new Schema({
@@ -281,7 +284,7 @@ schema.methods.sendChat = function sendChat (message, user) {
   this.chat.splice(200);
 
   // Kick off chat notifications in the background.
-  let lastSeenUpdate = {$set: {}, $inc: {_v: 1}};
+  let lastSeenUpdate = {$set: {}};
   lastSeenUpdate.$set[`newMessages.${this._id}`] = {name: this.name, value: true};
 
   // do not send notifications for guilds with more than 5000 users and for the tavern
@@ -431,7 +434,6 @@ schema.methods.finishQuest = function finishQuest (quest) {
   updates.$inc[`achievements.quests.${questK}`] = 1;
   updates.$inc['stats.gp'] = Number(quest.drop.gp);
   updates.$inc['stats.exp'] = Number(quest.drop.exp);
-  updates.$inc._v = 1;
 
   if (this._id === TAVERN_ID) {
     updates.$set['party.quest.completed'] = questK; // Just show the notif
@@ -498,11 +500,11 @@ schema.statics.collectQuest = async function collectQuest (user, progress) {
   // Still needs completing
   if (_.find(shared.content.quests[group.quest.key].collect, (v, k) => {
     return group.quest.progress.collect[k] < v.count;
-  })) return group.save();
+  })) return await group.save();
 
   await group.finishQuest(quest);
   group.sendChat('`All items found! Party has received their rewards.`');
-  return group.save();
+  return await group.save();
 };
 
 schema.statics.bossQuest = async function bossQuest (user, progress) {
@@ -517,7 +519,7 @@ schema.statics.bossQuest = async function bossQuest (user, progress) {
   group.quest.progress.hp -= progress.up;
   // TODO Create a party preferred language option so emits like this can be localized. Suggestion: Always display the English version too. Or, if English is not displayed to the players, at least include it in a new field in the chat object that's visible in the database - essential for admins when troubleshooting quests!
   let playerAttack = `${user.profile.name} attacks ${quest.boss.name('en')} for ${progress.up.toFixed(1)} damage.`;
-  let bossAttack = nconf.get('CRON_SAFE_MODE') === 'true' || nconf.get('CRON_SEMI_SAFE_MODE') === 'true' ? `${quest.boss.name('en')} does not attack, because it respects the fact that there are some bugs\` \`post-maintenance and it doesn't want to hurt anyone unfairly. It will continue its rampage soon!` : `${quest.boss.name('en')} attacks party for ${Math.abs(down).toFixed(1)} damage.`;
+  let bossAttack = CRON_SAFE_MODE || CRON_SEMI_SAFE_MODE ? `${quest.boss.name('en')} does not attack, because it respects the fact that there are some bugs\` \`post-maintenance and it doesn't want to hurt anyone unfairly. It will continue its rampage soon!` : `${quest.boss.name('en')} attacks party for ${Math.abs(down).toFixed(1)} damage.`;
   // TODO Consider putting the safe mode boss attack message in an ENV var
   group.sendChat(`\`${playerAttack}\` \`${bossAttack}\``);
 
@@ -538,7 +540,7 @@ schema.statics.bossQuest = async function bossQuest (user, progress) {
   await User.update({
     _id: {$in: _.keys(group.quest.members)},
   }, {
-    $inc: {'stats.hp': down, _v: 1},
+    $inc: {'stats.hp': down},
   }, {multi: true}).exec();
   // Apply changes the currently cronning user locally so we don't have to reload it to get the updated state
   // TODO how to mark not modified? https://github.com/Automattic/mongoose/pull/1167
@@ -552,10 +554,9 @@ schema.statics.bossQuest = async function bossQuest (user, progress) {
 
     // Participants: Grant rewards & achievements, finish quest
     await group.finishQuest(shared.content.quests[group.quest.key]);
-    return group.save();
   }
 
-  return group.save();
+  return await group.save();
 };
 
 // to set a boss: `db.groups.update({_id:TAVERN_ID},{$set:{quest:{key:'dilatory',active:true,progress:{hp:1000,rage:1500}}}})`

website/server/models/user.js

@@ -345,6 +345,7 @@ export let schema = new Schema({
   },
 
   lastCron: {type: Date, default: Date.now},
+  _cronSignature: {type: String, default: 'NOT_RUNNING'}, // Private property used to avoid double cron
 
   // {GROUP_ID: Boolean}, represents whether they have unseen chat messages
   newMessages: {type: Schema.Types.Mixed, default: () => {
@@ -528,7 +529,7 @@ export let schema = new Schema({
 schema.plugin(baseModel, {
   // noSet is not used as updating uses a whitelist and creating only accepts specific params (password, email, username, ...)
   noSet: [],
-  private: ['auth.local.hashed_password', 'auth.local.salt'],
+  private: ['auth.local.hashed_password', 'auth.local.salt', '_cronSignature'],
   toJSONTransform: function userToJSON (plainObj, originalDoc) {
     // plainObj.filters = {}; // TODO Not saved, remove?
     plainObj._tmp = originalDoc._tmp; // be sure to send down drop notifs