Merge pull request #30 from screwdriver-cd/race

fix: add delete key to prevent race condition
screwdriver-cd · Jun 12, 2018 · b1a6e9e · b1a6e9e
2 parents ecf9b1a + 627c441
commit b1a6e9e
Show file tree

Hide file tree

Showing 2 changed files with 29 additions and 11 deletions.
diff --git a/index.js b/index.js
@@ -10,6 +10,7 @@ const winston = require('winston');
 const cron = require('./lib/cron');
 const Breaker = fuses.breaker;
 const FuseBox = fuses.box;
+const EXPIRE_TIME = 1800; // 30 mins
 
 class ExecutorQueue extends Executor {
     /**
@@ -309,6 +310,14 @@ class ExecutorQueue extends Executor {
             jobId,
             blockedBy: blockedBy.toString()
         }]);
+        const deleteKey = `deleted_${jobId}_${buildId}`;
+
+        // This is to prevent the case where a build is aborted while still in buildQueue
+        // The job might be picked up by the worker, so it's not deleted from buildQueue here
+        // Immediately after, the job gets put back to the queue, so it's always inside buildQueue
+        // This key will be cleaned up automatically or when it's picked up by the worker
+        await this.redisBreaker.runCommand('set', deleteKey, '');
+        await this.redisBreaker.runCommand('expire', deleteKey, EXPIRE_TIME);
 
         if (numDeleted !== 0) {
             // Build hadn't been started, "start" event was removed from queue

diff --git a/test/index.test.js b/test/index.test.js
@@ -11,20 +11,22 @@ const testConnection = require('./data/testConnection.json');
 const testConfig = require('./data/fullConfig.json');
 const testPipeline = require('./data/testPipeline.json');
 const testJob = require('./data/testJob.json');
+const { buildId, jobId, blockedBy } = testConfig;
 const partialTestConfig = {
-    buildId: testConfig.buildId,
-    jobId: testConfig.jobId,
-    blockedBy: testConfig.blockedBy
+    buildId,
+    jobId,
+    blockedBy
 };
 const partialTestConfigToString = Object.assign({}, partialTestConfig, {
-    blockedBy: testConfig.blockedBy.toString() });
+    blockedBy: blockedBy.toString() });
 const tokenGen = sinon.stub().returns('123456abc');
 const testDelayedConfig = {
     pipeline: testPipeline,
     job: testJob,
     apiUri: 'http://localhost',
     tokenGen
 };
+
 const EventEmitter = require('events').EventEmitter;
 
 sinon.assert.expose(chai.assert, { prefix: '' });
@@ -86,7 +88,9 @@ describe('index test', () => {
         };
         redisMock = {
             hdel: sinon.stub().yieldsAsync(),
-            hset: sinon.stub().yieldsAsync()
+            hset: sinon.stub().yieldsAsync(),
+            set: sinon.stub().yieldsAsync(),
+            expire: sinon.stub().yieldsAsync()
         };
         redisConstructorMock = sinon.stub().returns(redisMock);
         cronMock = {
@@ -302,14 +306,14 @@ describe('index test', () => {
 
         it('enqueues a build and caches the config', () => executor.start(testConfig).then(() => {
             assert.calledOnce(queueMock.connect);
-            assert.calledWith(redisMock.hset, 'buildConfigs', testConfig.buildId,
+            assert.calledWith(redisMock.hset, 'buildConfigs', buildId,
                 JSON.stringify(testConfig));
             assert.calledWith(queueMock.enqueue, 'builds', 'start', [partialTestConfigToString]);
         }));
 
         it('enqueues a build and caches the config', () => executor.start(testConfig).then(() => {
             assert.calledOnce(queueMock.connect);
-            assert.calledWith(redisMock.hset, 'buildConfigs', testConfig.buildId,
+            assert.calledWith(redisMock.hset, 'buildConfigs', buildId,
                 JSON.stringify(testConfig));
             assert.calledWith(queueMock.enqueue, 'builds', 'start', [partialTestConfigToString]);
         }));
@@ -336,13 +340,18 @@ describe('index test', () => {
             });
         });
 
-        it('removes a start event from the queue and the cached buildconfig',
-            () => executor.stop(partialTestConfig).then(() => {
+        it('removes a start event from the queue and the cached buildconfig', () => {
+            const deleteKey = `deleted_${jobId}_${buildId}`;
+
+            return executor.stop(partialTestConfig).then(() => {
                 assert.calledOnce(queueMock.connect);
                 assert.calledWith(queueMock.del, 'builds', 'start', [partialTestConfigToString]);
-                assert.calledWith(redisMock.hdel, 'buildConfigs', 8609);
+                assert.calledWith(redisMock.hdel, 'buildConfigs', buildId);
+                assert.calledWith(redisMock.set, deleteKey, '');
+                assert.calledWith(redisMock.expire, deleteKey, 1800);
                 assert.notCalled(queueMock.enqueue);
-            }));
+            });
+        });
 
         it('adds a stop event to the queue if no start events were removed', () => {
             queueMock.del.yieldsAsync(null, 0);