Handled two processes booting a site with no members-migrations row

ref https://linear.app/ghost/issue/HKG-1983/move-members-migrations-out-of-the-background-jobs-system

The row is written after the backfills run, so two processes booting a site
that has no row yet both reach the insert, and the unique index on jobs.name
rejects the second one. The old job claimed the row before running, so the
loser used to wait instead of failing. The runner now treats a failed insert
as done when the row exists afterwards, and still rethrows when it does not.

Both processes running the backfills in that window is accepted: every site
that has booted since Ghost 5.6 already has the row, and the backfills do
nothing without Stripe, which a site on its first boot never has.

Author: vershwal

ghost/core/core/server/services/members/service.js

@@ -126,6 +126,9 @@ const membersMigrationJobName = 'members-migrations';
  * row is written after the run, so a boot that dies mid-way retries next time.
  * The row is written even when Stripe is not configured, matching the job-based
  * version: a site that connects Stripe later never runs these 2021-era backfills.
+ * Two processes booting a site with no row will both run the backfills, where the
+ * job-based version claimed the row first. Accepted: every site that has booted
+ * since Ghost 5.6 has the row, and the backfills do nothing without Stripe.
  *
  * @TODO: Delete the backfills, this runner and the `jobs` rows in the next major
  *
@@ -157,8 +160,18 @@ async function runStripeMigrations(stripeService) {
   const attrs = { status, started_at: new Date(startedAt), finished_at: new Date() };
   if (existingJob) {
     await models.Job.edit(attrs, { id: existingJob.id });
-  } else {
+    return;
+  }
+
+  try {
     await models.Job.add({ name: membersMigrationJobName, ...attrs });
+  } catch (err) {
+    // Two processes booting a site with no row yet both get here, and the unique
+    // index on jobs.name rejects the second insert. The row exists, so move on.
+    if (!(await models.Job.findOne({ name: membersMigrationJobName }))) {
+      throw err;
+    }
+    logging.warn(`Stripe ${membersMigrationJobName} row was already written by another process`);
   }
 }
 

ghost/core/test/e2e-server/services/members-migrations.test.js

@@ -2,6 +2,7 @@ const assert = require('node:assert/strict');
 const sinon = require('sinon');
 const logging = require('@tryghost/logging');
 const { agentProvider } = require('../../utils/e2e-framework');
+const db = require('../../../core/server/data/db');
 const models = require('../../../core/server/models');
 const jobsService = require('../../../core/server/services/jobs');
 const membersService = require('../../../core/server/services/members');
@@ -32,12 +33,11 @@ describe('Members migrations on boot', function () {
     let jobId;
 
     beforeEach(async function () {
+      const attrs = { status: 'finished', started_at: STALE, finished_at: STALE };
       const job = await models.Job.findOne({ name: JOB_NAME });
-      jobId = job.id;
-      await models.Job.edit(
-        { status: 'finished', started_at: STALE, finished_at: STALE },
-        { id: jobId },
-      );
+      jobId = job
+        ? (await models.Job.edit(attrs, { id: job.id })).id
+        : (await models.Job.add({ name: JOB_NAME, ...attrs })).id;
     });
 
     it('skips the migrations and writes nothing when the row already exists', async function () {
@@ -91,5 +91,41 @@ describe('Members migrations on boot', function () {
       assert.equal(after.get('status'), 'failed');
       assert.ok(after.get('started_at') > STALE, 'expected the failed run to rewrite the row');
     });
+
+    it('keeps booting when another process writes the row first', async function () {
+      await db.knex('jobs').where({ name: JOB_NAME }).del();
+      sinon.stub(stripeService.migrations, 'execute').resolves();
+      const add = sinon.stub(models.Job, 'add').callsFake(async function (...args) {
+        // The other process inserts the row first, then our own insert hits the
+        // unique index on jobs.name. The runner never inspects the error itself:
+        // any failed insert with a row present takes the warn path.
+        await add.wrappedMethod.apply(this, args);
+        return add.wrappedMethod.apply(this, args);
+      });
+      const loggingWarn = sinon.stub(logging, 'warn');
+
+      await membersService.init();
+
+      sinon.assert.calledOnce(add);
+      sinon.assert.calledWithMatch(loggingWarn, /row was already written by another process/);
+
+      const rows = await db.knex('jobs').where({ name: JOB_NAME });
+      assert.equal(rows.length, 1);
+      assert.equal(rows[0].status, 'finished');
+    });
+
+    it('fails boot when the insert fails and no other process wrote the row', async function () {
+      await db.knex('jobs').where({ name: JOB_NAME }).del();
+      const execute = sinon.stub(stripeService.migrations, 'execute').resolves();
+      sinon.stub(models.Job, 'add').rejects(new Error('insert exploded'));
+      const loggingWarn = sinon.stub(logging, 'warn');
+
+      await assert.rejects(membersService.init(), { message: 'insert exploded' });
+
+      sinon.assert.calledOnce(execute);
+      sinon.assert.notCalled(loggingWarn);
+      const rows = await db.knex('jobs').where({ name: JOB_NAME });
+      assert.equal(rows.length, 0);
+    });
   });
 });