fix(github-backend): handle race condition in editorial workflow (#2658)

packages/netlify-cms-backend-github/src/implementation.js

@@ -2,6 +2,7 @@ import React from 'react';
 import trimStart from 'lodash/trimStart';
 import semaphore from 'semaphore';
 import { stripIndent } from 'common-tags';
+import { asyncLock } from 'netlify-cms-lib-util';
 import AuthenticationPage from './AuthenticationPage';
 import API from './API';
 import GraphQLAPI from './GraphQLAPI';
@@ -67,6 +68,21 @@ export default class GitHub {
     this.token = '';
     this.squash_merges = config.getIn(['backend', 'squash_merges']);
     this.use_graphql = config.getIn(['backend', 'use_graphql']);
+    this.lock = asyncLock();
+  }
+
+  async runWithLock(func, message) {
+    try {
+      const acquired = await this.lock.acquire();
+      if (!acquired) {
+        console.warn(message);
+      }
+
+      const result = await func();
+      return result;
+    } finally {
+      this.lock.release();
+    }
   }
 
   authComponent() {
@@ -281,7 +297,11 @@ export default class GitHub {
   }
 
   persistEntry(entry, mediaFiles = [], options = {}) {
-    return this.api.persistFiles(entry, mediaFiles, options);
+    // persistEntry is a transactional operation
+    return this.runWithLock(
+      () => this.api.persistFiles(entry, mediaFiles, options),
+      'Failed to acquire persist entry lock',
+    );
   }
 
   async persistMedia(mediaFile, options = {}) {
@@ -394,13 +414,22 @@ export default class GitHub {
   }
 
   updateUnpublishedEntryStatus(collection, slug, newStatus) {
-    return this.api.updateUnpublishedEntryStatus(collection, slug, newStatus);
+    // updateUnpublishedEntryStatus is a transactional operation
+    return this.runWithLock(
+      () => this.api.updateUnpublishedEntryStatus(collection, slug, newStatus),
+      'Failed to acquire update entry status lock',
+    );
   }
 
   deleteUnpublishedEntry(collection, slug) {
     return this.api.deleteUnpublishedEntry(collection, slug);
   }
+
   publishUnpublishedEntry(collection, slug) {
-    return this.api.publishUnpublishedEntry(collection, slug);
+    // publishUnpublishedEntry is a transactional operation
+    return this.runWithLock(
+      () => this.api.publishUnpublishedEntry(collection, slug),
+      'Failed to acquire publish entry lock',
+    );
   }
 }

packages/netlify-cms-lib-util/src/__tests__/asyncLock.spec.js

@@ -0,0 +1,85 @@
+import { asyncLock } from '../asyncLock';
+
+jest.useFakeTimers();
+jest.spyOn(console, 'warn').mockImplementation(() => {});
+
+describe('asyncLock', () => {
+  it('should be able to acquire a new lock', async () => {
+    const lock = asyncLock();
+
+    const acquired = await lock.acquire();
+
+    expect(acquired).toBe(true);
+  });
+
+  it('should not be able to acquire an acquired lock', async () => {
+    const lock = asyncLock();
+    await lock.acquire();
+
+    const promise = lock.acquire();
+
+    // advance by default lock timeout
+    jest.advanceTimersByTime(15000);
+
+    const acquired = await promise;
+
+    expect(acquired).toBe(false);
+  });
+
+  it('should be able to acquire an acquired lock that was released', async () => {
+    const lock = asyncLock();
+    await lock.acquire();
+
+    const promise = lock.acquire();
+
+    // release the lock in the "future"
+    setTimeout(() => lock.release(), 100);
+
+    // advance to the time where the lock will be released
+    jest.advanceTimersByTime(100);
+
+    const acquired = await promise;
+
+    expect(acquired).toBe(true);
+  });
+
+  it('should accept a timeout for acquire', async () => {
+    const lock = asyncLock();
+    await lock.acquire();
+
+    const promise = lock.acquire(50);
+
+    /// advance by lock timeout
+    jest.advanceTimersByTime(50);
+
+    const acquired = await promise;
+
+    expect(acquired).toBe(false);
+  });
+
+  it('should be able to re-acquire a lock after a timeout', async () => {
+    const lock = asyncLock();
+    await lock.acquire();
+
+    const promise = lock.acquire();
+
+    // advance by default lock timeout
+    jest.advanceTimersByTime(15000);
+
+    let acquired = await promise;
+
+    expect(acquired).toBe(false);
+
+    acquired = await lock.acquire();
+    expect(acquired).toBe(true);
+  });
+
+  it('should suppress "leave called too many times" error', async () => {
+    const lock = asyncLock();
+
+    await expect(() => lock.release()).not.toThrow();
+
+    expect(console.warn).toHaveBeenCalledTimes(1);
+    expect(console.warn).toHaveBeenCalledWith('leave called too many times.');
+  });
+});

packages/netlify-cms-lib-util/src/asyncLock.js

@@ -0,0 +1,41 @@
+import semaphore from 'semaphore';
+
+export const asyncLock = () => {
+  let lock = semaphore(1);
+
+  const acquire = (timeout = 15000) => {
+    const promise = new Promise(resolve => {
+      // this makes sure a caller doesn't gets stuck forever awaiting on the lock
+      const timeoutId = setTimeout(() => {
+        // we reset the lock in that case to allow future consumers to use it without being blocked
+        lock = semaphore(1);
+        resolve(false);
+      }, timeout);
+
+      lock.take(() => {
+        clearTimeout(timeoutId);
+        resolve(true);
+      });
+    });
+
+    return promise;
+  };
+
+  const release = () => {
+    try {
+      // suppress too many calls to leave error
+      lock.leave();
+    } catch (e) {
+      // calling 'leave' too many times might not be good behavior
+      // but there is no reason to completely fail on it
+      if (e.message !== 'leave called too many times.') {
+        throw e;
+      } else {
+        console.warn('leave called too many times.');
+        lock = semaphore(1);
+      }
+    }
+  };
+
+  return { acquire, release };
+};

packages/netlify-cms-lib-util/src/index.js

@@ -26,6 +26,7 @@ import {
 } from './backendUtil';
 import loadScript from './loadScript';
 import getBlobSHA from './getBlobSHA';
+import { asyncLock } from './asyncLock';
 
 export const NetlifyCmsLibUtil = {
   APIError,
@@ -77,4 +78,5 @@ export {
   responseParser,
   loadScript,
   getBlobSHA,
+  asyncLock,
 };