cron: set lastCron at the beginning and misc fixes

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

@@ -603,25 +603,25 @@ describe('recoverCron', () => {
     }
   });
 
-  it('increases status.times count and reruns up to 3 times', async (done) => {
+  it('increases status.times count and reruns up to 4 times', async (done) => {
     execStub.returns(Bluebird.resolve({_cronSignature: 'RUNNING_CRON'}));
-    execStub.onCall(3).returns(Bluebird.resolve({_cronSignature: 'NOT_RUNNING'}));
+    execStub.onCall(4).returns(Bluebird.resolve({_cronSignature: 'NOT_RUNNING'}));
 
     await recoverCron(status, locals);
 
-    expect(status.times).to.eql(3);
+    expect(status.times).to.eql(4);
     expect(locals.user).to.eql({_cronSignature: 'NOT_RUNNING'});
 
     done();
   });
 
-  it('throws an error if recoverCron runs 4 times', async (done) => {
+  it('throws an error if recoverCron runs 5 times', async (done) => {
     execStub.returns(Bluebird.resolve({_cronSignature: 'RUNNING_CRON'}));
 
     try {
       await recoverCron(status, locals);
     } catch (err) {
-      expect(status.times).to.eql(4);
+      expect(status.times).to.eql(5);
       expect(err.message).to.eql(`Impossible to recover from cron for user ${locals.user._id}.`);
 
       done();

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

@@ -12,6 +12,29 @@ const shouldDo = common.shouldDo;
 const scoreTask = common.ops.scoreTask;
 // const maxPMs = 200;
 
+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 < 5) {
+      await recoverCron(status, locals);
+    } else {
+      throw new Error(`Impossible to recover from cron for user ${user._id}.`);
+    }
+  } else {
+    locals.user = reloadedUser;
+    return null;
+  }
+}
+
 let CLEAR_BUFFS = {
   str: 0,
   int: 0,
@@ -83,29 +106,6 @@ 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;

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

@@ -103,13 +103,15 @@ async function cronAsync (req, res) {
 
     let _cronSignature = uuid();
 
-    // To avoid double cron we first set _cronSignature to now and then check that it's not changed while processing
+    // To avoid double cron we first set _cronSignature 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,
+        lastCron: now, // setting lastCron now so we don't risk re-running parts of cron if it fails
+        'auth.timestamps.loggedin': now,
       },
     }).exec();
 
@@ -170,8 +172,6 @@ async function cronAsync (req, res) {
     }, {
       $set: {
         _cronSignature: 'NOT_RUNNING',
-        lastCron: now,
-        'auth.timestamps.loggedin': now,
       },
     }).exec();
 
@@ -182,27 +182,23 @@ async function cronAsync (req, res) {
     // 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
+      // do it for max 5 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);
+      await 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;
-      }
+      throw err; // re-throw the original error
     }
   }
 }
@@ -212,7 +208,5 @@ module.exports = function cronMiddleware (req, res, next) {
     .then(() => {
       next();
     })
-    .catch(err => {
-      next(err);
-    });
+    .catch(next);
 };

website/server/models/group.js

@@ -279,6 +279,7 @@ export function chatDefaults (msg, user) {
 }
 
 const NO_CHAT_NOTIFICATIONS = [TAVERN_ID];
+
 schema.methods.sendChat = function sendChat (message, user) {
   this.chat.unshift(chatDefaults(message, user));
   this.chat.splice(200);
@@ -425,9 +426,9 @@ schema.statics.cleanGroupQuest = function cleanGroupQuest () {
   };
 };
 
-// Participants: Grant rewards & achievements, finish quest
-// Returns the promise from update().exec()
-schema.methods.finishQuest = function finishQuest (quest) {
+// Participants: Grant rewards & achievements, finish quest.
+// Changes the group object update members
+schema.methods.finishQuest = async function finishQuest (quest) {
   let questK = quest.key;
   let updates = {$inc: {}, $set: {}};
 
@@ -471,14 +472,14 @@ schema.methods.finishQuest = function finishQuest (quest) {
   let q = this._id === TAVERN_ID ? {} : {_id: {$in: _.keys(this.quest.members)}};
   this.quest = {};
   this.markModified('quest');
-  return User.update(q, updates, {multi: true}).exec();
+
+  return await User.update(q, updates, {multi: true}).exec();
 };
 
 function _isOnQuest (user, progress, group) {
   return group && progress && group.quest && group.quest.active && group.quest.members[user._id] === true;
 }
 
-// Returns a promise
 schema.statics.collectQuest = async function collectQuest (user, progress) {
   let group = await this.getGroup({user, groupId: 'party'});
   if (!_isOnQuest(user, progress, group)) return;
@@ -504,6 +505,7 @@ schema.statics.collectQuest = async function collectQuest (user, progress) {
 
   await group.finishQuest(quest);
   group.sendChat('`All items found! Party has received their rewards.`');
+
   return await group.save();
 };