Agenda: Can't cancel a job once it's already locked

[vkarpov15]

The below script will still print "Hello" long after the job has successfully been cancelled. For now I'm going to just check whether the job has been cancelled in the job itself, but is there any better way to work around this? Based on my reading of the code, it looks like once a job is locked and in the in-memory queue, there's no way to cancel it.

'use strict';

const Agenda = require('agenda');
const {MongoClient} = require('mongodb');
const co = require('co');

co(function * () {
  const db = yield MongoClient.connect('mongodb://localhost:27017/test');
  const agenda = (new Agenda).mongo(db, 'Task');

  agenda.define('hello', () => console.log('hello'));

  yield cb => agenda.on('ready', () => { agenda.start(); cb(); });

  yield cb => agenda.schedule(new Date(Date.now() + 1000), 'hello', {}, cb);
  yield cb => agenda.cancel({ name: 'hello' }, cb);
  console.log('cancelled');
});

[simison]

Excellent question!

This even seems like a potential bug.

[simison]

Meanwhile as a work-around it's possible to do this:

This functionality can also be achieved by first retrieving all the jobs from the database using agenda.jobs(), looping through the resulting array and calling job.remove() on each. It is however preferable to use agenda.cancel() for this use case, as this ensures the operation is atomic.

— via docs

[niftylettuce]

This is definitely a bug - in the logic for Agenda.prototype.cancel it only runs a database query to deleteMany - it does not remove them from in memory e.g. we need to take into account these functions using existing memory - so we'd have to revert these:

self._lockedJobs.push(job);
definitions[job.attrs.name].locked++;

[niftylettuce]

So basically we'd just have to remove it from the variable jobQueue @simison ?

[simison]

@niftylettuce not sure, I assume those in-memory steps are there to ensure queue keeps functioning when multiple Agenda workers are working on the same queue/db.

@agenda/maintainers any thoughts here?

[lushc]

Copying over my previous comment...

We could check if the job is still in the database in runOrRetry() at the same point it checks if the lock has expired. Since the job keeps a reference to its ObjectId it would be a fast lookup:

var exists = this._collection.find({_id: job.attrs._id}, {_id: 1}).limit(1).count() > 0;

Or, we could refactor the cancel method to first find jobs matching the query, create jobs out of the returned values and then splice them out of the _lockedJobs array. However, we'd most likely need to manage a flag to "pause" the job queue while a cancellation is in progress, otherwise, I think there's a potential race condition where a job to be cancelled could be executed or requeued.

While the former solution results in more database queries as the check happens every time a job is due to run, it'll be less error prone and shouldn't have much of a performance impact (but needs testing!)

@simison messing with the in-memory queue shouldn't affect other running instances since they're isolated from each other and don't step on each other's toes thanks to findOneAndUpdate's locking behaviour. As long as we put the job back into a state where it can be picked up again by those queries it should be OK 👍

[curry684]

Any updates on this? I'm researching tooling for a big project about to be started and Agenda seems like a superior match for my case, but cancellation would have to be "nearly always reliable".

[simonjanin]

Updates on this welcome. Reliable cancellation is a big factor in my decision whether to use Agenda or not.

[jofinkuriakose]

Any updates on this?

[simison]

@jofinkuriakose Nope. Help PRing and reviewing most welcome!

[mladibejn]

Any updates on this yet?

[BernaLang]

This is definitely a bug - in the logic for Agenda.prototype.cancel it only runs a database query to deleteMany - it does not remove them from in memory e.g. we need to take into account these functions using existing memory - so we'd have to revert these:

self._lockedJobs.push(job);
definitions[job.attrs.name].locked++;

I've tried doing what @niftylettuce mentioned on the cancel method but the job seems to run anyway, maybe it saves on a different memory variable, since it's my first time picking up this project i don't really know.

So i went ahead and tried @lushc option and that seems to work fine, i did some tests and on average the query increases about 4ms to the runOrRetry method, i'll be adding a PR in a bit.

[BernaLang]

I can't consistently pass one of the tests ("should run higher priority jobs first").
I think the query to the database is sometimes slowing down the higher priority tests and making the lower priority ones run first.

I have the fork with the changes on my profile if you wanna help, either way I'll try to look into it again later when i have the time.