Added guard for activateTheme being called in parallel causing sync bugs (#15053)

refs https://github.com/TryGhost/Team/issues/1666

- it seems like we may have a situation where `.activateTheme()` can be called simultaneously resulting in unexpected behaviour in the sync such as duplicate theme setting records
- adjusted behaviour to keep track of the currently running activation within the service and if `.activateTheme()` is called again whilst it's in progress it will wait for completion of the first sync before exiting early or continuing with a new activation

**Note:** There is a known edge-case if there are _more_ than 2 parallel `.activateTheme()` calls. We don't believe that will be an issue but calling it out in case we do still see duplicated custom setting records being created.

Co-authored-by: Kevin Ansfield <kevin@lookingsideways.co.uk>

ghost/custom-theme-settings-service/lib/service.js

@@ -20,6 +20,10 @@ module.exports = class CustomThemeSettingsService {
         this.activeThemeName = null;
 
         /** @private */
+        this._activatingPromise = null;
+        this._activatingName = null;
+        this._activatingSettings = null;
+
         this._repository = new BREAD({model});
         this._valueCache = cache;
         this._activeThemeSettings = {};
@@ -27,21 +31,53 @@ module.exports = class CustomThemeSettingsService {
 
     /**
      * The service only deals with one theme at a time,
-     * that theme is changed by calling this method with the output from gscan
+     * that theme is changed by calling this method with the output from gscan.
+     *
+     * To avoid syncing issues with activateTheme being called in quick succession,
+     * any previous/still-running activation promise is awaited before re-starting
+     * if necessary.
      *
      * @param {string} name - the name of the theme (Ghost has different names to themes with duplicate package.json names)
      * @param {Object} theme - checked theme output from gscan
      */
     async activateTheme(name, theme) {
-        this.activeThemeName = name;
+        const activate = async () => {
+            this.activeThemeName = name;
 
-        // add/remove/edit key/value records in the respository to match theme settings
-        const settings = await this._syncRepositoryWithTheme(name, theme);
+            // add/remove/edit key/value records in the respository to match theme settings
+            const settings = await this._syncRepositoryWithTheme(name, theme);
 
-        // populate the shared cache with all key/value pairs for this theme
-        this._populateValueCacheForTheme(theme, settings);
-        // populate the cache used for exposing full setting details for editing
-        this._populateInternalCacheForTheme(theme, settings);
+            // populate the shared cache with all key/value pairs for this theme
+            this._populateValueCacheForTheme(theme, settings);
+            // populate the cache used for exposing full setting details for editing
+            this._populateInternalCacheForTheme(theme, settings);
+        };
+
+        if (this._activatingPromise) {
+            // NOTE: must be calculated before awaiting promise as the promise finishing will clear the properties
+            const isSameName = name === this._activatingName;
+            const isSameSettings = JSON.stringify(theme.customSettings) === this._activatingSettings;
+
+            // wait for previous activation to finish
+            await this._activatingPromise;
+
+            // skip sync if we're re-activating exactly the same theme settings
+            if (isSameName && isSameSettings) {
+                return;
+            }
+        }
+
+        try {
+            this._activatingName = name;
+            this._activatingSettings = JSON.stringify(theme.customSettings);
+            this._activatingPromise = activate();
+
+            await this._activatingPromise;
+        } finally {
+            this._activatingPromise = null;
+            this._activatingName = null;
+            this._activatingSettings = null;
+        }
     }
 
     /**

ghost/custom-theme-settings-service/test/service.test.js

@@ -394,6 +394,91 @@ describe('Service', function () {
             should.exist(model.findAll.getCall(1).firstArg.filter);
             should.doesNotThrow(() => nql.parse(model.findAll.getCall(1).firstArg.filter));
         });
+
+        it('does not allow simultaneous calls for same theme', async function () {
+            service.activateTheme('test', {
+                name: 'test',
+                customSettings: {
+                    // no change
+                    one: {
+                        type: 'select',
+                        options: ['1', '2'],
+                        default: '2'
+                    },
+                    // no change
+                    two: {
+                        type: 'select',
+                        options: ['1', '2'],
+                        default: '1'
+                    },
+                    // new setting
+                    three: {
+                        type: 'select',
+                        options: ['uno', 'dos', 'tres'],
+                        default: 'tres'
+                    }
+                }
+            });
+
+            await service.activateTheme('test', {
+                name: 'test',
+                customSettings: {
+                    // no change
+                    one: {
+                        type: 'select',
+                        options: ['1', '2'],
+                        default: '2'
+                    },
+                    // no change
+                    two: {
+                        type: 'select',
+                        options: ['1', '2'],
+                        default: '1'
+                    },
+                    // new setting
+                    three: {
+                        type: 'select',
+                        options: ['uno', 'dos', 'tres'],
+                        default: 'tres'
+                    }
+                }
+            });
+
+            // model methods are only called enough times for one .activate call despite being called twice
+            model.findAll.callCount.should.equal(2);
+            model.add.callCount.should.equal(1);
+
+            // internal cache is correct
+            service.listSettings().should.deepEqual([{
+                id: 1,
+                key: 'one',
+                type: 'select',
+                options: ['1', '2'],
+                default: '2',
+                value: '1'
+            }, {
+                id: 2,
+                key: 'two',
+                type: 'select',
+                options: ['1', '2'],
+                default: '1',
+                value: '2'
+            }, {
+                id: 3,
+                key: 'three',
+                type: 'select',
+                options: ['uno', 'dos', 'tres'],
+                default: 'tres',
+                value: 'tres'
+            }]);
+
+            // external cache is correct
+            cache.getAll().should.deepEqual({
+                one: '1',
+                two: '2',
+                three: 'tres'
+            });
+        });
     });
 
     describe('listSettings()', function () {