fix(serenity): guard fails closed only on genuine errors; async begin-sites reconcile stale attempts (LLMO-7418 external-review Findings 1, 9, 11)

Finding 1: guardAgainstConcurrentProvisioning's read (getBrandProvisioningState)
threw a bare Error on ANY PostgREST failure, including "column does not exist" —
meaning if this service ever deploys even briefly ahead of the mysticat-data-service
migrations that add the provisioning columns, every synchronous createMarket/activate
call 500s on an endpoint that worked a moment ago. Preserve the underlying SQLSTATE
on the thrown error and degrade to a no-op specifically for 42703 (undefined_column) —
no column means no attempt could possibly be in flight, so proceeding is correct, not
just convenient. Any other read error still fails closed, unchanged.

Finding 11 (adjacent, same function): a missing/unparseable updated_at yielded NaN,
which is never < the staleness threshold, so it fell through to "reconcile as stale" —
capable of tearing down a genuinely fresh, healthy attempt. Treat NaN as "assume
fresh" instead (409, never silently reconciled away).

Finding 9: beginProvisioningAttempt's own CAS has no staleness awareness, so a stuck
pending row (crashed worker, DLQ'd message) 409s forever once a brand's callers are
async-only — there is no scheduled sweep (architecturally impossible; Semrush only
accepts user-token auth). Call guardAgainstConcurrentProvisioning immediately before
beginProvisioningAttempt at the two async begin-sites in this branch (createMarket,
activate's project-activation batch) that operate on a pre-existing brand id — reusing
the guard's own tested reconcile-or-409 logic rather than inventing new logic. The
brand-create async sites (createBrandForOrg) are NOT touched: they mint a brand-new,
freshly-generated UUID in the same request, so no prior attempt could exist for it —
adding the guard there would be a pure no-op read on every call.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

Author: Iris Alexandrescu

src/controllers/serenity.js

@@ -98,7 +98,7 @@ import { computeWriteDeadline } from '../support/serenity/intent-classification.
 import AccessControlUtil from '../support/access-control-util.js';
 import { isServicePrincipal, resolveBrandUuid } from '../support/prompts-storage.js';
 import {
-  getBrandAliases, getBrandUrlSources, getBrandCompetitors, updateBrand, getBrandBaseSiteId,
+  getBrandAliases, getBrandBaseSiteId,
   cancelProvisioningAttempt,
   guardAgainstConcurrentProvisioning, beginProvisioningAttempt,
 } from '../support/brands-storage.js';
@@ -1048,6 +1048,19 @@ function SerenityController(context, log, env) {
           // brand's already-canonical workspace rather than provisioning a new one — every
           // brand reaching this branch already has one (`auth.mode === 'subworkspace'` IS that
           // invariant; see `authorize`).
+          //
+          // LLMO-7418 external-review Finding 9: beginProvisioningAttempt's own CAS has no
+          // staleness awareness — a stuck `pending` row (a crashed worker, a DLQ'd message) would
+          // 409 here forever, with nothing else to ever reconcile it once every caller has
+          // migrated to async (a scheduled sweep is architecturally impossible — Semrush only
+          // accepts user-token auth). Reuse the sync guard's own reconcile-or-409 logic first: a
+          // stale attempt is reconciled to `failed` here (so the CAS below then succeeds), a
+          // genuinely fresh one still 409s (via the guard's own throw, same shape as `!began`'s).
+          await guardAgainstConcurrentProvisioning(
+            auth.brandUuid,
+            ctx.dataAccess.services.postgrestClient,
+            log,
+          );
           const attemptId = randomUUID();
           const began = await beginProvisioningAttempt({
             brandId: auth.brandUuid,
@@ -1753,6 +1766,14 @@ function SerenityController(context, log, env) {
       // in-request settle-poll + project-create/publish sequence), slated for removal once every
       // known caller has migrated to `async: true`.
       if (validateAsync(body)) {
+        // LLMO-7418 external-review Finding 9: see the createMarket async branch above for the
+        // full rationale — reconcile a stale in-flight attempt before minting a new one, since
+        // beginProvisioningAttempt's own CAS has no staleness awareness.
+        await guardAgainstConcurrentProvisioning(
+          brandUuid,
+          ctx.dataAccess.services.postgrestClient,
+          log,
+        );
         const attemptId = randomUUID();
         const began = await beginProvisioningAttempt({
           brandId: brandUuid,

src/support/brands-storage.js

@@ -1927,7 +1927,13 @@ export async function getBrandProvisioningState(brandId, postgrestClient) {
     .maybeSingle();
 
   if (error) {
-    throw new Error(`Failed to read brand provisioning state: ${error.message}`);
+    // Preserve the underlying Postgres SQLSTATE (e.g. `42703` undefined_column) on the thrown
+    // error — LLMO-7418 external-review Finding 1: guardAgainstConcurrentProvisioning needs this
+    // to tell "the async-provisioning migrations haven't landed yet" apart from a genuine
+    // transient read failure, and a bare `new Error(message)` here erases it.
+    const err = new Error(`Failed to read brand provisioning state: ${error.message}`);
+    err.code = error.code;
+    throw err;
   }
   if (!data) {
     return null;
@@ -2316,16 +2322,43 @@ export const PROVISIONING_STALE_THRESHOLD_MS = 10 * 60 * 1000;
  * @param {object} postgrestClient
  * @param {object} [log]
  * @throws when a fresh attempt is genuinely in flight (`err.status = 409`,
- *   `err.code = 'semrush_provisioning_in_progress'`).
+ *   `err.code = 'semrush_provisioning_in_progress'`), or when the provisioning-state read itself
+ *   fails for a reason OTHER than the columns not existing yet (see below) — an unreadable state
+ *   must never be silently treated as an empty one.
  */
 export async function guardAgainstConcurrentProvisioning(brandId, postgrestClient, log) {
-  const state = await getBrandProvisioningState(brandId, postgrestClient);
+  // POSTGRES_UNDEFINED_COLUMN (LLMO-7418 external-review Finding 1): this guard defends against
+  // a race between a synchronous caller and a LIVE async provisioning attempt — a race that
+  // cannot exist until the async-provisioning columns (mysticat-data-service migrations
+  // 1037/1040) are actually deployed. If this API ships even briefly ahead of those migrations,
+  // EVERY synchronous createMarket/activate call would otherwise 500 on a column that doesn't
+  // exist yet, on the exact endpoints every current caller already depends on. Degrade to a
+  // no-op instead: no column means no attempt could possibly be in flight, so proceeding is
+  // correct, not just convenient. Any OTHER read failure (a genuine transient DB error) still
+  // fails closed below, unchanged.
+  const POSTGRES_UNDEFINED_COLUMN = '42703';
+  let state;
+  try {
+    state = await getBrandProvisioningState(brandId, postgrestClient);
+  } catch (error) {
+    if (error?.code === POSTGRES_UNDEFINED_COLUMN) {
+      log?.warn?.('brands-storage: async-provisioning columns not present yet; guard is a no-op', {
+        brandId, error: error.message,
+      });
+      return;
+    }
+    throw error;
+  }
   if (!state || state.provisioningStatus !== 'pending') {
     return;
   }
 
   const ageMs = Date.now() - new Date(state.updatedAt).getTime();
-  if (ageMs < PROVISIONING_STALE_THRESHOLD_MS) {
+  // A missing/unparseable updatedAt yields NaN, and NaN is never < the threshold — falling
+  // through to "stale, reconcile" would then kill a genuinely fresh, healthy attempt (LLMO-7418
+  // external-review Finding 11). Treat an unparseable age as "assume fresh" (the safer
+  // direction: at worst a later request is briefly 409'd, never a live attempt torn down).
+  if (Number.isNaN(ageMs) || ageMs < PROVISIONING_STALE_THRESHOLD_MS) {
     // ErrorWithStatusCode (not a plain Error+.status, unlike this file's other 409s): this guard
     // is called from BOTH serenity.js's activate (whose mapError only special-cases `instanceof
     // ErrorWithStatusCode`) and brands.js's createBrandForOrg (whose createErrorResponse accepts

test/controllers/serenity.test.js

@@ -1657,6 +1657,10 @@ describe('SerenityController', () => {
 
       expect(response.status).to.equal(202);
       expect(body).to.include({ jobId: 'job-abc', status: 'IN_PROGRESS' });
+      // LLMO-7418 external-review Finding 9: reconciles a stale in-flight attempt (reusing the
+      // sync guard's own logic) before minting a new one — beginProvisioningAttempt's own CAS
+      // has no staleness awareness on its own.
+      expect(guardAgainstConcurrentProvisioningStub).to.have.been.calledOnceWith(BRAND);
       expect(beginProvisioningAttemptStub).to.have.been.calledOnce;
       expect(beginProvisioningAttemptStub.firstCall.args[0]).to.include({
         brandId: BRAND, updatedBy: 'serenity-create-market',
@@ -2206,7 +2210,9 @@ describe('SerenityController', () => {
 
       expect(response.status).to.equal(202);
       expect(orchestrateActivateMarketsStub).to.not.have.been.called;
-      expect(guardAgainstConcurrentProvisioningStub).to.not.have.been.called;
+      // LLMO-7418 external-review Finding 9: the async begin-site now reconciles a stale
+      // in-flight attempt first, reusing the sync guard's own logic, before minting a new one.
+      expect(guardAgainstConcurrentProvisioningStub).to.have.been.calledOnceWith(BRAND);
       expect(beginProvisioningAttemptStub).to.have.been.calledOnce;
       expect(beginProvisioningAttemptStub.firstCall.args[0]).to.include({
         brandId: BRAND, updatedBy: 'serenity-activate',

test/support/brands-storage.test.js

@@ -4938,6 +4938,52 @@ describe('brands-storage', () => {
         }]);
       });
 
+      it('degrades to a no-op (does not throw) when the provisioning columns do not exist yet (LLMO-7418 external-review Finding 1)', async () => {
+        const postgrestClient = createTableMockClient({
+          brands: {
+            data: null,
+            error: { message: 'column brands.semrush_provisioning_status does not exist', code: '42703' },
+          },
+        });
+        const warn = sinon.stub();
+
+        const call = guardAgainstConcurrentProvisioning(BRAND_ID, postgrestClient, { warn });
+        await expect(call).to.not.be.rejected;
+        expect(warn).to.have.been.calledOnce;
+      });
+
+      it('still fails closed (rethrows) on any OTHER read error, e.g. a transient DB failure', async () => {
+        const postgrestClient = createTableMockClient({
+          brands: { data: null, error: { message: 'connection reset', code: 'ECONNRESET' } },
+        });
+
+        let caught;
+        try {
+          await guardAgainstConcurrentProvisioning(BRAND_ID, postgrestClient);
+        } catch (e) {
+          caught = e;
+        }
+
+        expect(caught).to.exist;
+        expect(caught.message).to.include('Failed to read brand provisioning state');
+      });
+
+      it('treats an unparseable updated_at (NaN age) as fresh rather than reconciling it away (LLMO-7418 external-review Finding 11)', async () => {
+        const postgrestClient = createTableMockClient({
+          brands: { data: stateWith({ updated_at: 'not-a-real-timestamp' }), error: null },
+        });
+
+        let caught;
+        try {
+          await guardAgainstConcurrentProvisioning(BRAND_ID, postgrestClient);
+        } catch (e) {
+          caught = e;
+        }
+
+        expect(caught).to.exist;
+        expect(caught.status).to.equal(409);
+      });
+
       it('does not throw when the stale-reconciliation CAS is itself rejected (already reconciled)', async () => {
         const staleAgeMs = PROVISIONING_STALE_THRESHOLD_MS + 1000;
         const staleUpdatedAt = new Date(Date.now() - staleAgeMs).toISOString();