From 1a2696554254bc6817a880e113c17f92ba9e9c65 Mon Sep 17 00:00:00 2001 From: Matteo Pagliazzi Date: Mon, 23 May 2016 16:42:45 +0200 Subject: [PATCH] wip(): prevent cron from running twice --- website/server/middlewares/api-v3/cron.js | 116 +++++++++++++++++----- website/server/models/group.js | 7 +- website/server/models/user.js | 3 +- 3 files changed, 94 insertions(+), 32 deletions(-) diff --git a/website/server/middlewares/api-v3/cron.js b/website/server/middlewares/api-v3/cron.js index 780f400e8f..a512a7f397 100644 --- a/website/server/middlewares/api-v3/cron.js +++ b/website/server/middlewares/api-v3/cron.js @@ -6,6 +6,7 @@ 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 { v4 as uuid } from 'uuid'; const daysSince = common.daysSince; @@ -98,20 +99,43 @@ module.exports = function cronMiddleware (req, res, next) { if (daysMissed <= 0) return next(); - // Fetch active tasks (no completed todos) - Tasks.Task.find({ - userId: user._id, - $or: [ // Exclude completed todos - {type: 'todo', completed: false}, - {type: {$in: ['habit', 'daily', 'reward']}}, - ], + let quest; + let progress; + let tasks; + + // To avoid double cron we set _cronSignature on the user to a random string + // and check that it has remained the same before saving + user._cronSignature = uuid(); + + User.update({ + _id: user._id, + _cronSignature: 'not-running', + }, { + $set: { + _cronSignature: user._cronSignature, + }, }).exec() - .then(tasks => { + .then((updateResult) => { // Fetch active tasks (no completed todos) + // if the cron signature is set, throw an error and recover later + if (updateResult.nMatched === 0 || updateResult.nUpdated === 0) { + throw new Error('cron-already-running'); + } + + return Tasks.Task.find({ + userId: user._id, + $or: [ // Exclude completed todos + {type: 'todo', completed: false}, + {type: {$in: ['habit', 'daily', 'reward']}}, + ], + }).exec(); + }) + .then(tasksFetched => { + tasks = tasksFetched; let tasksByType = {habits: [], dailys: [], todos: [], rewards: []}; tasks.forEach(task => tasksByType[`${task.type}s`].push(task)); // Run cron - let progress = cron({user, tasksByType, now, daysMissed, analytics, timezoneOffsetFromUserPrefs}); + progress = cron({user, tasksByType, now, daysMissed, analytics, timezoneOffsetFromUserPrefs}); // Clear old completed todos - 30 days for free users, 90 for subscribers // Do not delete challenges completed todos TODO unless the task is broken? @@ -126,7 +150,7 @@ module.exports = function cronMiddleware (req, res, next) { }).exec(); let ranCron = user.isModified(); - let quest = common.content.quests[user.party.quest.key]; + quest = common.content.quests[user.party.quest.key]; if (ranCron) res.locals.wasModified = true; // TODO remove after v2 is retired if (!ranCron) return next(); @@ -134,27 +158,65 @@ module.exports = function cronMiddleware (req, res, next) { // Group.tavernBoss(user, progress); // Save user and tasks - let toSave = [user.save()]; + // Uses mongoose's internals to get update command + let mongooseDelta = user.$__delta(); + if (mongooseDelta instanceof Error) { + throw mongooseDelta; + } + + let mongooseWhere = user.$__where(mongooseDelta[0]); + if (mongooseWhere instanceof Error) { + throw mongooseWhere; + } + mongooseWhere._cronSignature = user._cronSignature; // Only update the user if cron signature matches + + return User.update(mongooseWhere, mongooseDelta[1]); + }) + .then(updateResult => { + // if the cron signature is set, throw an error and recover later + if (updateResult.nMatched === 0 || updateResult.nUpdated === 0) { + throw new Error('cron-already-running'); + } + + let toSave = []; tasks.forEach(task => { if (task.isModified()) toSave.push(task.save()); }); - return Bluebird.all(toSave) - .then(saved => { - user = res.locals.user = saved[0]; - if (!quest) return; - // 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; + return Bluebird.all(toSave); + }) + .then(() => { + if (!quest) return; + // 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.findByIdAndUpdate(user._id, { + $set: {_cronSignature: 'not-running'}, + }, { + new: true, // return the updated document + }).exec(); + }) // fetch the updated user... + .then(updatedUser => { + user = res.locals.user = updatedUser; - return null; - }); - }) - .then(() => next()) - .catch(next); + return null; + }) + .then(() => next()) + .catch((err) => { + if (err.message === 'cron-already-running') { + // recovering after abort, wait 200ms and reload user + setTimeout(() => { + User.findById(user._id, (reloadErr, reloadedUser) => { + if (reloadErr) return next(reloadErr); + user = res.locals.user = reloadedUser; + return next(); + }); + }, 200); + } else { + return next(err); + } }); }; diff --git a/website/server/models/group.js b/website/server/models/group.js index fc1fba287b..b53138c1e7 100644 --- a/website/server/models/group.js +++ b/website/server/models/group.js @@ -281,7 +281,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 +431,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 @@ -536,9 +535,9 @@ schema.statics.bossQuest = async function bossQuest (user, progress) { // Everyone takes damage await User.update({ - _id: {$in: _.keys(group.quest.members)}, + _id: {$in: _.keys(group.quest.members.toObject ? group.quest.members.toObject() : 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 diff --git a/website/server/models/user.js b/website/server/models/user.js index 2d0e561a66..8a457500de 100644 --- a/website/server/models/user.js +++ b/website/server/models/user.js @@ -345,6 +345,7 @@ export let schema = new Schema({ }, lastCron: {type: Date, default: Date.now}, + _cronSignature: String, // Private property 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