diff --git a/docs/openapi/brands-v2-api.yaml b/docs/openapi/brands-v2-api.yaml index 35c0664555..6b214963fb 100644 --- a/docs/openapi/brands-v2-api.yaml +++ b/docs/openapi/brands-v2-api.yaml @@ -81,13 +81,14 @@ v2-brands-for-org: $ref: './schemas.yaml#/V2Brand' '202': description: | - PR-C (LLMO-7352/LLMO-7418): the request set `async: true` alongside - `semrushMarket`. The brand row is persisted immediately (visible, + The request set `async: true` (PR-C, LLMO-7352/LLMO-7418, alongside + `semrushMarket`; or Phase 4, same epic, on a bare create with no + market). The brand row is persisted immediately (visible, non-active — "Setting up", no workspace pointer yet); the - sub-workspace ready-check and the initial market's project create/ - publish are enqueued to a background job chain instead of running - inline. Poll the returned `jobId` at `GET .../serenity/jobs/{jobId}` - for status + result. + sub-workspace ready-check — and, when `semrushMarket` was supplied, + the initial market's project create/publish — are enqueued to a + background job chain instead of running inline. Poll the returned + `jobId` at `GET .../serenity/jobs/{jobId}` for status + result. content: application/json: schema: @@ -117,7 +118,7 @@ v2-brands-for-org: organization (`code: brand_site_org_mismatch`). - The brand's primary URL is already the primary URL (base site) of another brand in this organization. - - (PR-C, `async: true` only) a provisioning attempt is already in flight + - (`async: true` only) a provisioning attempt is already in flight for this brand (`error: semrushProvisioningInProgress`). headers: X-Error: diff --git a/docs/openapi/schemas.yaml b/docs/openapi/schemas.yaml index d47e40fdab..487773d23d 100644 --- a/docs/openapi/schemas.yaml +++ b/docs/openapi/schemas.yaml @@ -8101,17 +8101,21 @@ V2BrandInput: type: boolean default: false description: > - PR-C (LLMO-7352/LLMO-7418). Only meaningful when `semrushMarket` is - supplied. When true, the brand row is persisted first (visible, - non-active — "Setting up"), then the sub-workspace ready-check and the - initial market's project create/publish are enqueued to a background - job chain instead of running inline, and the endpoint returns 202 with - a job id to poll via `GET .../serenity/jobs/{jobId}`. Omitted/false - processes synchronously as today (still 201) — this is opt-in only and - not yet the default. Unlike `SerenityCreatePromptsRequest`'s `async` + PR-C (LLMO-7352/LLMO-7418), extended by Phase 4 (LLMO-6405 "B"). Meaningful on + either of this endpoint's two Semrush-provisioning creates: with `semrushMarket` + supplied (sub-workspace ready-check + initial market's project create/publish, + enqueued as a job chain), or without it, for a sub-workspace-only active create + anchored by `baseSiteId` (sub-workspace create/adopt only, no project). In both + cases, true persists the brand row first (visible, non-active — "Setting up") + and returns 202 with a job id to poll via `GET .../serenity/jobs/{jobId}`. + Ignored (no error, brand created synchronously) when the organization is not + serenity-active (a flat/brandalf org) or the brand is created `pending` (a + pending brand always defers ALL Semrush provisioning regardless of this flag). + Omitted/false processes synchronously as today (still 201) — this is opt-in + only and not yet the default. Unlike `SerenityCreatePromptsRequest`'s `async` flag, this one is not intended to be a permanent dual-mode feature: the - synchronous path is the LLMO-7352 bug pattern itself, and is expected - to be retired once known callers have migrated to `async: true`. + synchronous path is the LLMO-7352 bug pattern itself, and is expected to be + retired once known callers have migrated to `async: true`. V2BrandUpdateInput: type: object @@ -12676,12 +12680,15 @@ SerenityActivateRequest: type: boolean default: false description: > - PR-C (LLMO-7352/LLMO-7418). Only meaningful on the already-active-brand, - markets-supplied path — a pending or bare-reactivation activation always - stays synchronous regardless of this flag. When true, the sub-workspace - ready-check and the per-market project create/publish + site-link + active - flip are enqueued to a background job chain instead of running inline, and - the endpoint returns 202 with a job id to poll via + PR-C (LLMO-7352/LLMO-7418), extended by Phase 4. Meaningful on all three + activation shapes this endpoint supports: the already-active-brand, + markets-supplied path (sub-workspace ready-check + per-market project + create/publish + site-link + active flip, enqueued as a job chain); the + pending→active activation (sub-workspace create/adopt only, enqueued as + `provision-workspace-job` -> `activate-brand-workspace-job`); and bare + reactivation of an already-active, pointer-less brand (same chain). In + every case, true enqueues the work to a background job chain instead of + running inline, and the endpoint returns 202 with a job id to poll via `GET .../serenity/jobs/{jobId}`. Omitted/false processes synchronously as today — this is opt-in only and not yet the default. @@ -12818,12 +12825,15 @@ SerenityCreateMarketRequest: type: boolean default: false description: > - PR-C (LLMO-7352/LLMO-7418). When true (sub-workspace-mode brands only), - the sub-workspace-ready check and the project create/publish are + PR-C (LLMO-7352/LLMO-7418). When true, and only for a sub-workspace-mode + brand, the sub-workspace-ready check and the project create/publish are enqueued to a background job chain instead of running inline, and the endpoint returns 202 with a job id to poll via - `GET .../serenity/jobs/{jobId}`. Omitted/false processes synchronously - as today — this is opt-in only and not yet the default. Unlike + `GET .../serenity/jobs/{jobId}`. Ignored (no error, market created + synchronously) for a flat-mode brand — flat mode has no sub-workspace + readiness to await, so there is nothing to defer. Omitted/false + processes synchronously as today — this is opt-in only and not yet the + default. Unlike `SerenityCreatePromptsRequest`'s `async` flag, this one is not intended to be a permanent dual-mode feature: the synchronous path is the LLMO-7352 bug pattern itself, and is expected to be retired once known diff --git a/docs/openapi/serenity-api.yaml b/docs/openapi/serenity-api.yaml index 4e32c47463..1f8eb3d57f 100644 --- a/docs/openapi/serenity-api.yaml +++ b/docs/openapi/serenity-api.yaml @@ -612,7 +612,14 @@ v2-serenity-markets: '404': description: Organization has no workspace, or serenity is not active for the organization. '409': - description: A market already exists for this (brandId, geoTargetId, languageCode) slice, or (async only) a provisioning attempt is already in flight for this brand. + description: >- + One of three conflicts, distinguished by the `error` token: (a) a market already exists + for this (brandId, geoTargetId, languageCode) slice; (b) `semrushProvisioningInProgress` + (`async: true` only) — a provisioning attempt is already in flight for this brand, so + retry shortly; or (c) `semrushProvisioningIncomplete` — the brand's Semrush sub-workspace + is pending or has failed to provision, so it has no workspace of its own to add a market + in. (b) converges on retry; (c) does not, and needs the brand's provisioning to complete + first. '502': description: Upstream returned a non-2xx response. content: @@ -1203,12 +1210,12 @@ v2-serenity-activate: intact; retry converges idempotently). An already-active brand re-supplying markets is never downgraded (a partial failure is reported as 207). - PR-C (LLMO-7352/LLMO-7418): when a primary URL/domain and markets are - present (the branch described above — NOT the sub-workspace-only pending - or bare-reactivation paths, which always stay synchronous), the caller may - set `async: true` to enqueue the whole batch to a background job chain - instead of running it inline; see the `202` response. Absent/false runs - synchronously as described above — this is opt-in only and not yet the + LLMO-7352/LLMO-7418: the caller may set `async: true` on ANY of the 3 + branches — project activation (PR-C), the sub-workspace-only + pending→active flip, and bare reactivation (both Phase 4) — to enqueue + that branch's work to a background job chain instead of running it + inline; see the `202` response. Absent/false runs synchronously as + described above on every branch — this is opt-in only and not yet the default. operationId: activateSerenityBrand security: @@ -1226,17 +1233,20 @@ v2-serenity-activate: schema: { $ref: './schemas.yaml#/SerenityActivateResponse' } '202': description: | - PR-C (LLMO-7352/LLMO-7418): the request set `async: true` on the - project-activation branch (primary URL + markets present). The whole - batch (sub-workspace ready-check, every market's project create/ - publish, the site link, and the active flip) is enqueued to a - background job chain instead of running inline. Poll the returned - `jobId` at `GET .../serenity/jobs/{jobId}` for status + result — the - polling endpoint transparently follows the job chain to its - EFFECTIVE terminal hop, so `status` only reports `COMPLETED` once - the actual batch has run, not merely once the sub-workspace became - ready; on COMPLETED, `result` is the same body a synchronous - `200`/`207` would have returned. + The request set `async: true`. On the project-activation branch + (primary URL + markets present — PR-C), the whole batch + (sub-workspace ready-check, every market's project create/publish, + the site link, and the active flip) is enqueued to a background job + chain. On the sub-workspace-only pending→active or bare-reactivation + branches (Phase 4), only the sub-workspace ready-check + the status + flip are enqueued — there is no project/market work on those + branches. Either way, poll the returned `jobId` at + `GET .../serenity/jobs/{jobId}` for status + result — the polling + endpoint transparently follows the job chain to its EFFECTIVE + terminal hop, so `status` only reports `COMPLETED` once the actual + batch (or status flip) has run, not merely once the sub-workspace + became ready; on COMPLETED, `result` is the same body the branch's + own synchronous `200`/`207`/`502` would have returned. content: application/json: schema: { $ref: './schemas.yaml#/SerenityPromptsJobAccepted' } @@ -1258,8 +1268,9 @@ v2-serenity-activate: outcomes — the operator must choose a different primary URL; (b) misconfiguration: the brand's sub-workspace equals the organization parent workspace (refused to avoid acting on the shared parent pool); or (c) - (PR-C, `async: true` only) a provisioning attempt is already in flight for - this brand (`error: semrush_provisioning_in_progress`) — retry shortly. + (`async: true` only, any branch) a provisioning attempt is already in + flight for this brand (`error: semrushProvisioningInProgress`) — + retry shortly. content: application/json: schema: diff --git a/src/controllers/brands.js b/src/controllers/brands.js index ef5d68acee..fc0687f4f7 100644 --- a/src/controllers/brands.js +++ b/src/controllers/brands.js @@ -67,6 +67,7 @@ import { readSerenityFlagScopes, withSerenityState, beginProvisioningAttempt, + updateProvisioningJobId, promoteProvisioningFailed, recordFreshBrandProvisioningStartFailure, } from '../support/brands-storage.js'; @@ -87,7 +88,9 @@ import { } from '../support/serenity/mapping-rows.js'; import { propagateSiteUrlToSemrush } from '../support/serenity/site-url-propagation.js'; import { createSerenityTransport } from '../support/serenity/rest-transport.js'; -import { isSemrushTransportError, unwrapTransportCause } from '../support/serenity/errors.js'; +import { + ERROR_CODES, isSemrushTransportError, unwrapTransportCause, +} from '../support/serenity/errors.js'; import { logUpstreamError } from '../support/serenity/upstream-log.js'; import { buildBrandMarketsResponse } from '../support/serenity/brand-markets.js'; import { syncBrandUrlsAcrossMarkets } from '../support/serenity/brand-urls.js'; @@ -97,6 +100,8 @@ import { isSerenityActiveForBrand, isSerenityActiveForOrg, isSerenityUiActiveForOrg, + isAsyncProvisioningKillSwitched, + isAsyncProvisioningEnabled, } from '../support/serenity/serenity-active.js'; import { buildReservedIdentities, @@ -1729,6 +1734,12 @@ function BrandsController(ctx, log, env) { // hoisted so the catch can mark that persisted-but-not-yet-provisioning row visibly failed if // starting the attempt itself throws (see the catch's compensation). let asyncMarketProvisioning = null; + // Phase 4 (LLMO-7352/LLMO-7418): the bare-create (no semrushMarket) sibling of + // asyncMarketProvisioning above — same "non-null after the row is persisted means start an + // attempt" contract, just with no chained job (a bare sub-workspace has no project to create + // afterward). Mutually exclusive with asyncMarketProvisioning (they're set in sibling + // if/else branches of the same hasSemrushMarket check). + let asyncBareProvisioning = null; try { if (!hasText(spaceCatId)) { @@ -1862,7 +1873,19 @@ function BrandsController(ctx, log, env) { // job chain, the same shared path Add Market uses. Unlike createPrompts's flag, this one // is NOT permanent: the synchronous branch is the LLMO-7352 bug pattern itself, slated // for removal once every known caller has migrated to `async: true`. - if (validateAsync(brandData)) { + if (validateAsync(brandData) && isAsyncProvisioningEnabled(context.env || env)) { + // LLMO-7418 external-review Finding 15: server-side kill switch — lets ops disable + // the async path for this organization without a deploy if it misbehaves in + // production. The caller falls back to the synchronous path on its own retry. + if (await isAsyncProvisioningKillSwitched(context, spaceCatId, log)) { + return createResponse( + { + error: 'asyncProvisioningDisabled', + message: 'Async provisioning is temporarily disabled for this organization; retry without async: true', + }, + 503, + ); + } // brandAliases/urls/competitors are NOT read here (unlike the sync branch below): the // brand row this section persists below (upsertBrand) writes them to storage, and the // async chain's orchestration reads them back from there — the same DB-backed source @@ -1928,6 +1951,39 @@ function BrandsController(ctx, log, env) { languageCode: provisioned.languageCode, }; } + } else if (validateAsync(brandData) && isAsyncProvisioningEnabled(context.env || env)) { + // B (LLMO-6405): sub-workspace-only active create — no market supplied, so + // no project is provisioned. Markets are added afterwards from the Markets + // tab. The brand is anchored by its primary site (baseSiteId, persisted by + // upsertBrand below) AND by its Semrush sub-workspace. + // + // Phase 4 (LLMO-7352/LLMO-7418): opt-in only (mirrors the hasSemrushMarket branch's own + // `async` flag above). Absent/false runs the EXACT bespoke synchronous + // provisionBrandSubworkspaceBare call this branch has always run. `async: true` persists + // the brand row FIRST (visible, active, no workspace pointer yet), then hands the bare + // sub-workspace provisioning off to provision-workspace-job — no chained job, since a + // bare create has no project to create once the workspace is ready. + // + // LLMO-7418 external-review Finding 15: server-side kill switch — see the + // hasSemrushMarket branch above for rationale. + if (await isAsyncProvisioningKillSwitched(context, spaceCatId, log)) { + return createResponse( + { + error: 'asyncProvisioningDisabled', + message: 'Async provisioning is temporarily disabled for this organization; retry without async: true', + }, + 503, + ); + } + // Resolved and validated HERE, before any write — same rationale as the + // hasSemrushMarket branch: a missing org workspace config must never leave a + // persisted, permanently-inert brand row behind. + const parentWorkspaceId = await resolveWorkspaceId(context, spaceCatId); + if (!parentWorkspaceId || !hasText(parentWorkspaceId)) { + return badRequest('Organization has no Semrush workspace configured'); + } + provisionedBrandId = randomUUID(); + asyncBareProvisioning = { parentWorkspaceId }; } else { // B (LLMO-6405): sub-workspace-only active create — no market supplied, so // no project is provisioned. Markets are added afterwards from the Markets @@ -2067,7 +2123,7 @@ function BrandsController(ctx, log, env) { if (!began) { return createResponse( { - error: 'semrushProvisioningInProgress', + error: ERROR_CODES.SEMRUSH_PROVISIONING_IN_PROGRESS, message: 'Unable to start Semrush provisioning for the new brand', }, 409, @@ -2100,8 +2156,20 @@ function BrandsController(ctx, log, env) { }, }, }); + // LLMO-7418 external-review Finding 17: records the first-hop job id, best-effort, + // so it isn't left permanently NULL until the worker's own self-requeue hop writes it. + await updateProvisioningJobId({ + brandId: asyncBrandId, + attemptId, + jobId: job.getId(), + postgrestClient, + }).catch((updateError) => { + log.error('brands: failed to record the first-hop job id (best-effort)', { + brandId: asyncBrandId, attemptId, jobId: job.getId(), error: updateError?.message, + }); + }); return createResponse( - { ...withSerenityState(created, serenityScopes), status: 'pending', jobId: job.getId() }, + { ...withSerenityState(created, serenityScopes), jobId: job.getId() }, 202, ); } catch (enqueueError) { @@ -2126,6 +2194,88 @@ function BrandsController(ctx, log, env) { } } + // Phase 4 (LLMO-7352/LLMO-7418): bare-create's async sibling of the block above — same + // shape, no chained job (no project to create once the workspace is ready). + if (asyncBareProvisioning) { + const asyncBrandId = /** @type {string} */ (provisionedBrandId); + const { parentWorkspaceId } = asyncBareProvisioning; + const attemptId = randomUUID(); + let began; + try { + began = await beginProvisioningAttempt({ + brandId: asyncBrandId, + attemptId, + postgrestClient, + updatedBy, + }); + } catch (beginError) { + log.error('brands: failed to begin the provisioning attempt after brand row was persisted', { + brandId: asyncBrandId, error: beginError?.message, + }); + await recordFreshBrandProvisioningStartFailure({ + brandId: asyncBrandId, + error: 'Failed to start Semrush provisioning', + postgrestClient, + }).catch((failError) => { + log.error('brands: failed to record the provisioning-start failure itself', { + brandId: asyncBrandId, error: failError?.message, + }); + }); + throw beginError; + } + if (!began) { + return createResponse( + { + error: ERROR_CODES.SEMRUSH_PROVISIONING_IN_PROGRESS, + message: 'Unable to start Semrush provisioning for the new brand', + }, + 409, + ); + } + try { + const job = await createAndEnqueueJob(context, { + jobType: PROVISION_WORKSPACE_JOB_TYPE, + metadata: { + brandId: asyncBrandId, + attemptId, + parentWorkspaceId, + title: brandData.name, + }, + }); + // LLMO-7418 external-review Finding 17: records the first-hop job id, best-effort, + // so it isn't left permanently NULL until the worker's own self-requeue hop writes it. + await updateProvisioningJobId({ + brandId: asyncBrandId, + attemptId, + jobId: job.getId(), + postgrestClient, + }).catch((updateError) => { + log.error('brands: failed to record the first-hop job id (best-effort)', { + brandId: asyncBrandId, attemptId, jobId: job.getId(), error: updateError?.message, + }); + }); + return createResponse( + { ...withSerenityState(created, serenityScopes), jobId: job.getId() }, + 202, + ); + } catch (enqueueError) { + log.error('brands: failed to start async Semrush provisioning after brand row was persisted', { + brandId: asyncBrandId, error: enqueueError?.message, + }); + await promoteProvisioningFailed({ + brandId: asyncBrandId, + attemptId, + error: 'Failed to start Semrush provisioning', + postgrestClient, + }).catch((failError) => { + log.error('brands: failed to record the provisioning-start failure itself', { + brandId: asyncBrandId, error: failError?.message, + }); + }); + throw enqueueError; + } + } + return createResponse(withSerenityState(created, serenityScopes), 201); } catch (error) { if (error.code === 'brand_status_demotion_not_allowed') { diff --git a/src/controllers/serenity.js b/src/controllers/serenity.js index d902fef896..8727205379 100644 --- a/src/controllers/serenity.js +++ b/src/controllers/serenity.js @@ -47,6 +47,7 @@ import { CLASSIFY_PROMPTS_JOB_TYPE } from '../support/serenity/handlers/classify import { PROVISION_WORKSPACE_JOB_TYPE } from '../support/serenity/handlers/provision-workspace-job.js'; import { CREATE_MARKET_JOB_TYPE } from '../support/serenity/handlers/create-market-job.js'; import { ACTIVATE_MARKETS_JOB_TYPE } from '../support/serenity/handlers/activate-markets-job.js'; +import { ACTIVATE_BRAND_WORKSPACE_JOB_TYPE } from '../support/serenity/handlers/activate-brand-workspace-job.js'; import { ORIGIN_VALUE } from '../support/serenity/prompt-tags.js'; import { BULK_TAGS_JOB_TYPE, @@ -91,7 +92,11 @@ import { handleTagImpactSubworkspace, } from '../support/serenity/handlers/tags.js'; import { ensureSubworkspace, decommissionBrandWorkspace } from '../support/serenity/workspace-lifecycle.js'; -import { isSerenityActiveForBrand } from '../support/serenity/serenity-active.js'; +import { + isSerenityActiveForBrand, + isAsyncProvisioningKillSwitched, + isAsyncProvisioningEnabled, +} from '../support/serenity/serenity-active.js'; import { marketForGeoTargetId } from '../support/serenity/locations.js'; import { brandNeedles, classifyBrandedTag } from '../support/serenity/branded-classifier.js'; import { computeWriteDeadline } from '../support/serenity/intent-classification.js'; @@ -1044,7 +1049,19 @@ function SerenityController(context, log, env) { // `createPrompts`'s flag, this one is NOT a permanent dual-mode feature: the // synchronous branch is the LLMO-7352 bug pattern itself, not a valid alternative, and // is slated for removal once every known caller has migrated to `async: true`. - if (validateAsync(requestBody)) { + if (validateAsync(requestBody) && isAsyncProvisioningEnabled(ctx.env || env)) { + // LLMO-7418 external-review Finding 15: server-side kill switch — lets ops disable + // the async path for this organization without a deploy if it misbehaves in + // production. The caller falls back to the synchronous path on its own retry. + if (await isAsyncProvisioningKillSwitched(ctx, ctx?.params?.spaceCatId, log)) { + return createResponse( + { + error: 'asyncProvisioningDisabled', + message: 'Async provisioning is temporarily disabled for this organization; retry without async: true', + }, + 503, + ); + } // The worker's existing-pointer fast path (provision-workspace-job.js) polls THIS // brand's already-canonical workspace rather than provisioning a new one — every // brand reaching this branch already has one (`auth.mode === 'subworkspace'` IS that @@ -1075,7 +1092,7 @@ function SerenityController(context, log, env) { + 'brand; please retry shortly.', 409, ); - err.code = 'semrush_provisioning_in_progress'; + err.code = ERROR_CODES.SEMRUSH_PROVISIONING_IN_PROGRESS; throw err; } // N2 note: no `title` is passed here deliberately. This branch runs only in @@ -1178,7 +1195,7 @@ function SerenityController(context, log, env) { + 'available until provisioning completes.', 409, ); - err.code = 'semrush_provisioning_incomplete'; + err.code = ERROR_CODES.SEMRUSH_PROVISIONING_INCOMPLETE; throw err; } // Flat handler self-derives brandDomain from siteId (it has Site access). @@ -1672,6 +1689,79 @@ function SerenityController(context, log, env) { if (!existingSiteId) { throw new ErrorWithStatusCode(`Brand has no onboarded primary site: ${brandUuid}`, 400); } + // Phase 4 (LLMO-7352/LLMO-7418): opt-in only (mirrors this endpoint's own + // project-activation branch below, and createMarket's/createBrandForOrg's `async` flag). + // Absent/false runs the EXACT synchronous pending->active flip this branch has always + // run. `async: true` hands the sub-workspace-ensure + status flip off to the + // `provision-workspace-job` -> + // `serenity-activate-brand-workspace` job chain instead. + if (validateAsync(body) && isAsyncProvisioningEnabled(ctx.env || env)) { + // LLMO-7418 external-review Finding 15: server-side kill switch — see createMarket's + // async branch for the full rationale. + if (await isAsyncProvisioningKillSwitched(ctx, ctx?.params?.spaceCatId, log)) { + return createResponse( + { + error: 'asyncProvisioningDisabled', + message: 'Async provisioning is temporarily disabled for this organization; retry without async: true', + }, + 503, + ); + } + // LLMO-7418 external-review Finding 9: see createMarket's async branch for the full + // rationale — reconcile a stale in-flight attempt (reusing the sync guard's own logic) + // before minting a new one, since beginProvisioningAttempt's own CAS has no staleness + // awareness on its own. + await guardAgainstConcurrentProvisioning( + brandUuid, + ctx.dataAccess.services.postgrestClient, + log, + ); + const attemptId = randomUUID(); + const began = await beginProvisioningAttempt({ + brandId: brandUuid, + attemptId, + postgrestClient: ctx.dataAccess.services.postgrestClient, + updatedBy: 'serenity-activate', + }); + if (!began) { + const err = new ErrorWithStatusCode( + 'A Semrush sub-workspace provisioning attempt is already in progress for this ' + + 'brand; please retry shortly.', + 409, + ); + err.code = ERROR_CODES.SEMRUSH_PROVISIONING_IN_PROGRESS; + throw err; + } + const job = await createAndEnqueueJob(ctx, { + jobType: PROVISION_WORKSPACE_JOB_TYPE, + metadata: { + brandId: brandUuid, + attemptId, + parentWorkspaceId: auth.parentWorkspaceId ?? '', + // LLMO-7418 external-review Finding 4: this branch's brand is GUARANTEED + // pointer-less (a pending brand never has a workspace pointer), so the worker + // ALWAYS takes the create-or-adopt path here, never the existing-pointer fast + // path — omitting `title` would call Semrush with an untitled sub-workspace on + // every single pending->active async activation. + title: brand.getName?.() ?? '', + chainedJobType: ACTIVATE_BRAND_WORKSPACE_JOB_TYPE, + chainedJobMetadata: { brandId: brandUuid, wasPending: true }, + }, + }); + // LLMO-7418 external-review Finding 17: records the first-hop job id, best-effort, + // so it isn't left permanently NULL until the worker's own self-requeue hop writes it. + await updateProvisioningJobId({ + brandId: brandUuid, + attemptId, + jobId: job.getId(), + postgrestClient: ctx.dataAccess.services.postgrestClient, + }).catch((updateError) => { + log.error('activate: failed to record the first-hop job id (best-effort)', { + brandId: brandUuid, attemptId, jobId: job.getId(), error: updateError?.message, + }); + }); + return accepted({ jobId: job.getId(), status: job.getStatus() }); + } // PR-C guard (LLMO-7352/LLMO-7418): this branch stays synchronous, but a market-creating // endpoint may have an async provisioning attempt in flight for this SAME brand — without // this check, ensureSubworkspace below could independently create a second workspace. @@ -1749,6 +1839,77 @@ function SerenityController(context, log, env) { if (generatePrompts) { throw new ErrorWithStatusCode('A primary URL is required to generate prompts', 400); } + // Phase 4 (LLMO-7352/LLMO-7418): opt-in only — see the wasPending branch above for the + // full rationale. `wasPending: false` in the chained metadata distinguishes this + // already-active no-op re-affirm from a real pending->active transition, so + // activate-brand-workspace-job.js's save-divergence handling matches this branch's own + // 207-not-502 contract. + if (validateAsync(body) && isAsyncProvisioningEnabled(ctx.env || env)) { + // LLMO-7418 external-review Finding 15: server-side kill switch — see createMarket's + // async branch for the full rationale. + if (await isAsyncProvisioningKillSwitched(ctx, ctx?.params?.spaceCatId, log)) { + return createResponse( + { + error: 'asyncProvisioningDisabled', + message: 'Async provisioning is temporarily disabled for this organization; retry without async: true', + }, + 503, + ); + } + // LLMO-7418 external-review Finding 9: see createMarket's async branch (and the + // wasPending branch above) for the full rationale. + await guardAgainstConcurrentProvisioning( + brandUuid, + ctx.dataAccess.services.postgrestClient, + log, + ); + const attemptId = randomUUID(); + const began = await beginProvisioningAttempt({ + brandId: brandUuid, + attemptId, + postgrestClient: ctx.dataAccess.services.postgrestClient, + updatedBy: 'serenity-activate', + }); + if (!began) { + const err = new ErrorWithStatusCode( + 'A Semrush sub-workspace provisioning attempt is already in progress for this ' + + 'brand; please retry shortly.', + 409, + ); + err.code = ERROR_CODES.SEMRUSH_PROVISIONING_IN_PROGRESS; + throw err; + } + const job = await createAndEnqueueJob(ctx, { + jobType: PROVISION_WORKSPACE_JOB_TYPE, + metadata: { + brandId: brandUuid, + attemptId, + parentWorkspaceId: auth.parentWorkspaceId ?? '', + // LLMO-7418 external-review Finding 4: see the wasPending branch above — an + // already-active brand isn't as reliably pointer-less as a pending one, but the + // SYNCHRONOUS twin of this exact branch still defensively calls the general-purpose + // `ensureSubworkspace` (create-or-existing), so the async path must be able to + // create with a real title too, not assume the existing-pointer fast path always + // applies. + title: brand.getName?.() ?? '', + chainedJobType: ACTIVATE_BRAND_WORKSPACE_JOB_TYPE, + chainedJobMetadata: { brandId: brandUuid, wasPending: false }, + }, + }); + // LLMO-7418 external-review Finding 17: records the first-hop job id, best-effort, + // so it isn't left permanently NULL until the worker's own self-requeue hop writes it. + await updateProvisioningJobId({ + brandId: brandUuid, + attemptId, + jobId: job.getId(), + postgrestClient: ctx.dataAccess.services.postgrestClient, + }).catch((updateError) => { + log.error('activate: failed to record the first-hop job id (best-effort)', { + brandId: brandUuid, attemptId, jobId: job.getId(), error: updateError?.message, + }); + }); + return accepted({ jobId: job.getId(), status: job.getStatus() }); + } // PR-C guard (LLMO-7352/LLMO-7418): see the wasPending branch above for rationale. await guardAgainstConcurrentProvisioning( brandUuid, @@ -1820,7 +1981,18 @@ function SerenityController(context, log, env) { // LLMO-7352 bug pattern itself (this is one of the 3 real conversion candidates — the // in-request settle-poll + project-create/publish sequence), slated for removal once every // known caller has migrated to `async: true`. - if (validateAsync(body)) { + if (validateAsync(body) && isAsyncProvisioningEnabled(ctx.env || env)) { + // LLMO-7418 external-review Finding 15: server-side kill switch — see createMarket's + // async branch for the full rationale. + if (await isAsyncProvisioningKillSwitched(ctx, ctx?.params?.spaceCatId, log)) { + return createResponse( + { + error: 'asyncProvisioningDisabled', + message: 'Async provisioning is temporarily disabled for this organization; retry without async: true', + }, + 503, + ); + } // 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. @@ -1842,7 +2014,7 @@ function SerenityController(context, log, env) { + 'brand; please retry shortly.', 409, ); - err.code = 'semrush_provisioning_in_progress'; + err.code = ERROR_CODES.SEMRUSH_PROVISIONING_IN_PROGRESS; throw err; } const job = await createAndEnqueueJob(ctx, { diff --git a/src/serenity-prompt-classification/index.js b/src/serenity-prompt-classification/index.js index 6354dc65e9..40b069b6b1 100644 --- a/src/serenity-prompt-classification/index.js +++ b/src/serenity-prompt-classification/index.js @@ -47,6 +47,10 @@ import { activateMarketsJobHandler, ACTIVATE_MARKETS_JOB_TYPE, } from '../support/serenity/handlers/activate-markets-job.js'; +import { + activateBrandWorkspaceJobHandler, + ACTIVATE_BRAND_WORKSPACE_JOB_TYPE, +} from '../support/serenity/handlers/activate-brand-workspace-job.js'; import { isRateLimited, isSemrushTransportError, @@ -119,7 +123,9 @@ export const vaultOpts = { * sub-workspace provisioning lives in `../support/serenity/handlers/provision-workspace-job.js`; * and PR-C's two chained market-creation phases (workspace ready -> create the market/run the * activate batch against it) live in `../support/serenity/handlers/create-market-job.js` and - * `../support/serenity/handlers/activate-markets-job.js`. All five are registered below. + * `../support/serenity/handlers/activate-markets-job.js`; Phase 4's brand-status-flip half of + * `activate`'s two remaining "skip"-mode branches lives in + * `../support/serenity/handlers/activate-brand-workspace-job.js`. All six are registered below. * * @type {Record Promise>} @@ -130,6 +136,7 @@ const HANDLERS = { [PROVISION_WORKSPACE_JOB_TYPE]: provisionWorkspaceHandler, [CREATE_MARKET_JOB_TYPE]: createMarketJobHandler, [ACTIVATE_MARKETS_JOB_TYPE]: activateMarketsJobHandler, + [ACTIVATE_BRAND_WORKSPACE_JOB_TYPE]: activateBrandWorkspaceJobHandler, }; const TRANSIENT_NETWORK_ERROR_CODES = new Set([ diff --git a/src/support/brands-storage.js b/src/support/brands-storage.js index b1cf2d861b..46a70ca971 100644 --- a/src/support/brands-storage.js +++ b/src/support/brands-storage.js @@ -20,6 +20,7 @@ import { sanitizeGuidanceText, } from './brand-guidance.js'; import { readFeatureFlagScopes, resolveFlagRowForBrand } from './feature-flags-storage.js'; +import { ERROR_CODES } from './serenity/errors.js'; import { SERENITY_FEATURE_FLAG_NAME, SERENITY_FEATURE_FLAG_PRODUCT, @@ -2338,9 +2339,9 @@ 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'`), 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. + * `err.code = ERROR_CODES.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) { // POSTGRES_UNDEFINED_COLUMN (LLMO-7418 external-review Finding 1): this guard defends against @@ -2383,7 +2384,7 @@ export async function guardAgainstConcurrentProvisioning(brandId, postgrestClien 'A Semrush sub-workspace provisioning attempt is already in progress for this brand; please retry shortly.', 409, ); - err.code = 'semrush_provisioning_in_progress'; + err.code = ERROR_CODES.SEMRUSH_PROVISIONING_IN_PROGRESS; throw err; } diff --git a/src/support/serenity/errors.js b/src/support/serenity/errors.js index ec366d646a..15f0e7028f 100644 --- a/src/support/serenity/errors.js +++ b/src/support/serenity/errors.js @@ -178,6 +178,20 @@ export const ERROR_CODES = Object.freeze({ // main_brand:true) could not be established. Surfaced via // MainBrandBenchmarkInvariantError, below. MAIN_BRAND_BENCHMARK_INVARIANT: 'mainBrandBenchmarkInvariant', + // Async sub-workspace provisioning (LLMO-7352/LLMO-7418). Both are 409s, and both are raised + // from BOTH serenity.js and brands.js, so they live here rather than as string literals at each + // throw site -- the two controllers previously spelled the in-progress one two different ways + // (`semrush_provisioning_in_progress` vs `semrushProvisioningInProgress`) and published both + // spellings in their respective OpenAPI documents, which no client could branch on reliably. + // + // They are NOT interchangeable, which is why there are two: + // IN_PROGRESS -- an attempt is live right now. RETRYABLE: wait and retry, it will converge. + // INCOMPLETE -- provisioning is pending or has FAILED, so the brand has no sub-workspace of + // its own to write into. NOT retryable on its own; the brand's provisioning + // has to complete (or be retried) first. + // A client that collapses them shows "please retry shortly" on a brand that will never converge. + SEMRUSH_PROVISIONING_IN_PROGRESS: 'semrushProvisioningInProgress', + SEMRUSH_PROVISIONING_INCOMPLETE: 'semrushProvisioningIncomplete', }); /** diff --git a/src/support/serenity/handlers/activate-brand-workspace-job.js b/src/support/serenity/handlers/activate-brand-workspace-job.js new file mode 100644 index 0000000000..33343adabb --- /dev/null +++ b/src/support/serenity/handlers/activate-brand-workspace-job.js @@ -0,0 +1,77 @@ +/* + * Copyright 2026 Adobe. All rights reserved. + * This file is licensed to you under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. You may obtain a copy + * of the License at http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software distributed under + * the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR REPRESENTATIONS + * OF ANY KIND, either express or implied. See the License for the specific language + * governing permissions and limitations under the License. + */ + +// @ts-check + +/** + * Job type dispatched to {@link activateBrandWorkspaceJobHandler} by the runner + * (`src/serenity-prompt-classification/index.js`). Phase 4 (LLMO-7352/LLMO-7418): the second + * phase of `activate`'s two "skip"-mode branches' async flow (pending→active, and bare + * reactivation of an already-active brand) — enqueued by `provision-workspace-job.js` as its + * `chainedJobType` once the sub-workspace it depends on is confirmed `ready`, never enqueued + * directly by an HTTP controller. + */ +export const ACTIVATE_BRAND_WORKSPACE_JOB_TYPE = 'serenity-activate-brand-workspace'; + +/** + * Reports the brand's current status now that its sub-workspace is confirmed ready — READ-ONLY, + * never writes. + * + * `promoteProvisioningReady` (the hop immediately before this chain is enqueued — + * `provision-workspace-job.js`) already durably flips the brand to `active` as part of the SAME + * atomic compare-and-set write that promotes `semrush_provisioning_status` to `ready`; the chain + * is only ever enqueued once that write has succeeded. So by the time this job runs, the brand + * is ALREADY `active` in the common case — this handler's own job is not to repeat that write + * (an earlier version of this file did, via `brand.setStatus('active'); brand.save()`, which was + * both redundant on success AND unguarded: a chained job can run minutes after its enqueue + * (self-requeue backoff, plain SQS delivery latency), and blindly re-writing `active` with no + * CAS/re-check would silently RESURRECT a brand a legitimate concurrent `/serenity/deactivate` + * had already moved back to `pending` in the meantime — exactly the "active brand bound to a + * workspace nobody meant it to have" failure class this whole epic exists to prevent, just + * reached through a different door. Fixed in review before merge.) + * + * @param {object} context - worker context (`dataAccess`, `log`). + * @param {object} job - the current `AsyncJob`. `job.getMetadata()` carries + * `{ brandId, wasPending? }` (the chain-enqueue's `chainedJobMetadata` — no `workspaceId` is + * needed here, unlike the market-creating chained jobs, since this job never talks to Semrush). + * @returns {Promise} `{ status: 200, body: {...} }` when the brand is (still) active — + * the common case; `{ status: 207, body: {...} }` reporting whatever the brand's CURRENT + * status actually is when it is no longer active (a legitimate concurrent deactivate raced + * ahead of this chain) — never a manufactured 502 "incomplete", since the sub-workspace itself + * provisioned successfully; only the brand's separate, later-changed active/pending state + * diverged from what this chain expected to find. + */ +export async function activateBrandWorkspaceJobHandler(context, job) { + const { dataAccess, log } = context; + const metadata = job.getMetadata() ?? {}; + const { brandId, wasPending = false } = metadata; + + const brand = await dataAccess.Brand.findById(brandId); + if (!brand) { + // A concurrent hard-delete raced ahead of this chain — report it the same way a + // no-longer-active brand is reported above, rather than crashing on brand.getStatus(). + log?.info?.('activate-brand-workspace-job: brand no longer exists by the time this chain ran; reporting deleted instead of throwing', { + brandId, wasPending, + }); + return { status: 207, body: { brandId, status: 'deleted', markets: [] } }; + } + const currentStatus = brand.getStatus?.(); + + if (currentStatus === 'active') { + return { status: 200, body: { brandId, status: 'active', markets: [] } }; + } + + log?.info?.('activate-brand-workspace-job: brand is no longer active by the time this chain ran; reporting its current state instead of overwriting it', { + brandId, wasPending, currentStatus, + }); + return { status: 207, body: { brandId, status: currentStatus, markets: [] } }; +} diff --git a/src/support/serenity/handlers/provision-workspace-job.js b/src/support/serenity/handlers/provision-workspace-job.js index b9970eccaa..684635720c 100644 --- a/src/support/serenity/handlers/provision-workspace-job.js +++ b/src/support/serenity/handlers/provision-workspace-job.js @@ -38,6 +38,23 @@ import { */ export const PROVISION_WORKSPACE_JOB_TYPE = 'serenity-provision-workspace'; +// LLMO-7418 external-review Finding 14: retry classification for a status-poll failure. +// `SerenityTransportError` always carries a numeric `.status` (the upstream HTTP status); +// a raw network-level failure (fetch itself throwing — DNS, connection reset, timeout) has +// none. Only these are treated as transient and routed through the existing bounded +// self-requeue ladder below; everything else (a permanent 4xx like an expired/invalid IMS +// token, or an unexpected non-transport error) keeps today's fail-fast behavior via the +// outer catch. +const RETRYABLE_TRANSPORT_STATUSES = new Set([429, 500, 502, 503, 504]); +function isRetryableWorkspaceStatusError(error) { + const { status } = error ?? {}; + if (typeof status !== 'number') { + // No upstream status at all — a network-level failure, not an application error. + return true; + } + return RETRYABLE_TRANSPORT_STATUSES.has(status); +} + /** * Hard cap on self-requeue depth. Live-verified settle time for a SUCCESSFUL create is * seconds, not minutes (LLMO-7352 incident data: ~10s); this ladder exists for the @@ -403,7 +420,25 @@ export async function provisionWorkspaceHandler(context, job, accessToken) { } } - const statusResult = await transport.getWorkspaceStatus(candidate.workspaceId); + let statusResult; + try { + statusResult = await transport.getWorkspaceStatus(candidate.workspaceId); + } catch (error) { + if (!isRetryableWorkspaceStatusError(error)) { + throw error; + } + // Transient upstream/network failure — leave `statusResult` undefined so `status` + // below is `undefined`, which is neither ready nor terminal-failure, and this hop + // falls straight into the existing "still settling" self-requeue branch below (same + // bounded backoff/depth cap already used for an actual `not ready` poll result). + log?.warn?.('provision-workspace-job: transient error polling workspace status; treating as not-ready and self-requeuing', { + brandId, + attemptId, + semrushWorkspaceId: candidate.workspaceId, + error: error?.message, + status: error?.status, + }); + } const status = statusResult?.status; if (isWorkspaceReady(status)) { diff --git a/src/support/serenity/serenity-active.js b/src/support/serenity/serenity-active.js index 39233e8b9c..58bdf00313 100644 --- a/src/support/serenity/serenity-active.js +++ b/src/support/serenity/serenity-active.js @@ -47,6 +47,53 @@ export const SERENITY_FEATURE_FLAG_NAME = 'serenity'; */ export const SERENITY_UI_FEATURE_FLAG_NAME = 'serenity_ui'; +/** + * Server-side kill switch for the opt-in `async: true` provisioning path (LLMO-7418 + * external-review Finding 15). Unlike `SERENITY_FEATURE_FLAG_NAME` above (an opt-IN + * rollout switch, off by default), this is an opt-OUT switch: the async path stays + * available by default, and an explicit `true` row disables it org-wide. Lets ops + * flip async provisioning off for one organization without a deploy, if it + * misbehaves in production — there was previously no way to do this short of a + * code change reverting every `async: true` call site at once. + */ +export const ASYNC_PROVISIONING_KILL_SWITCH_FLAG_NAME = 'serenity_async_provisioning_disabled'; + +/** + * GLOBAL async-provisioning master switch (LLMO-7352/LLMO-7418), DEFAULT OFF. + * + * This exists to separate "the code ships" from "the behaviour changes". Every Semrush + * brand-creating consumer now sends `async: true` (the dashboard's six flows), so without this + * gate the very act of merging flips every Serenity organization onto the async path at once, + * and the only way back is reverting a merge under pressure. With it, the stack merges inert, + * async is turned on deliberately, and it can be turned off again in seconds. + * + * Deliberately a single global env boolean rather than a per-organization opt-in row: there are + * ~140 Serenity organizations, so a per-org ENABLE flag would mean creating and maintaining a row + * for each — real operational burden for no extra safety. The per-org + * {@link ASYNC_PROVISIONING_KILL_SWITCH_FLAG_NAME} covers the opposite, cheap case (disable the + * one organization that misbehaves), because that one is exception-only. + * + * Same shape as this codebase's other global serenity toggles (`SERENITY_DEFER_PUBLISH`, + * `SERENITY_ALLOW_WORKSPACE_DELETE`, `SERENITY_ALLOW_NON_IMS_AUTH`). Wired to Vault at + * `dx_mysticat//api-service`, so flipping it is a config change, not a code deploy. + * + * When OFF, an `async: true` request is NOT an error — it falls through to the synchronous branch + * the endpoint has always run. That is what makes merging safe by default: every consumer keeps + * working exactly as today until this is switched on. + */ +export const ASYNC_PROVISIONING_ENABLED_ENV_FLAG = 'SERENITY_ASYNC_PROVISIONING_ENABLED'; + +/** + * Reads the global async-provisioning master switch. `true` ONLY for the exact string `'true'` + * (env values are strings); anything else — unset, `'false'`, a typo — is OFF. Fail-safe by + * design: the default is the synchronous path this endpoint has always run. + * @param {object} [env] - the request env (`context.env`). + * @returns {boolean} + */ +export function isAsyncProvisioningEnabled(env) { + return env?.[ASYNC_PROVISIONING_ENABLED_ENV_FLAG] === 'true'; +} + /** * Module-scoped TTL+size-bounded cache, mirroring the workspace-resolver cache * (warm Lambda containers reuse module state, so a Map here amortises the @@ -239,3 +286,34 @@ export async function isSerenityUiActiveForOrg(ctx, spaceCatId, log) { const scopes = await readCachedFlagScopes(ctx, spaceCatId, SERENITY_UI_FEATURE_FLAG_NAME, log); return scopes?.orgRow?.flag_value === true; } + +/** + * LLMO-7418 external-review Finding 15: server-side kill switch for the opt-in + * `async: true` provisioning path. Reads the org-wide + * `LLMO/serenity_async_provisioning_disabled` feature flag (cached, same + * machinery as every other flag in this file). + * + * Deliberately reuses the SAME "absent/unreadable resolves to `false`" + * fail-safe shape as every other predicate here — it happens to be the safe + * default in both directions: for a rollout flag, `false` means "stay off"; + * for this kill switch, `false` means "stay on" (async provisioning + * available). A transient PostgREST read failure must never silently disable + * async provisioning org-wide, so this is NOT inverted to fail closed. + * + * @param {object} ctx - Request context (uses + * `ctx.dataAccess.services.postgrestClient`). + * @param {string} spaceCatId - SpaceCat organization UUID. + * @param {object} [log] - Optional logger (used to surface a missing client / + * a read error without throwing on this hot path). + * @returns {Promise} `true` only when the flag is explicitly on + * (async provisioning is disabled for this organization). + */ +export async function isAsyncProvisioningKillSwitched(ctx, spaceCatId, log) { + const scopes = await readCachedFlagScopes( + ctx, + spaceCatId, + ASYNC_PROVISIONING_KILL_SWITCH_FLAG_NAME, + log, + ); + return scopes?.orgRow?.flag_value === true; +} diff --git a/test/controllers/brands.test.js b/test/controllers/brands.test.js index c7ac70bcd5..04668be930 100644 --- a/test/controllers/brands.test.js +++ b/test/controllers/brands.test.js @@ -157,6 +157,11 @@ describe('Brands Controller', () => { BRAND_IMS_CLIENT_ID: 'client123', BRAND_IMS_CLIENT_CODE: 'code123', BRAND_IMS_CLIENT_SECRET: 'secret123', + // LLMO-7418: the global async-provisioning master switch is DEFAULT OFF in production, so an + // `async: true` request falls through to the synchronous branch unless it is enabled. Default + // it ON here so the existing async-path tests exercise the async branch they were written + // for; the OFF behaviour has its own dedicated test that omits this flag. + SERENITY_ASYNC_PROVISIONING_ENABLED: 'true', }; const authContextAdmin = { @@ -5318,6 +5323,10 @@ describe('Brands Controller', () => { beginProvisioningAttempt = sinon.stub().resolves(true), createAndEnqueueJob = sinon.stub().resolves({ getId: () => 'job-abc' }), promoteProvisioningFailed = sinon.stub().resolves(true), + updateProvisioningJobId = sinon.stub().resolves(true), + // LLMO-7418 external-review Finding 15: kill switch off by default (async available); + // specific tests override it to resolve(true) to exercise the 503 gate. + isAsyncProvisioningKillSwitched = sinon.stub().resolves(false), } = {}) { const Mocked = await esmock('../../src/controllers/brands.js', { '../../src/support/serenity/brand-provisioning.js': { @@ -5327,7 +5336,10 @@ describe('Brands Controller', () => { provisionBrandSubworkspaceBare: provisionBrandSubworkspaceBare || sinon.stub().resolves({ semrushSubWorkspaceId: 'ws-bare' }), }, - '../../src/support/serenity/serenity-active.js': { isSerenityActiveForOrg }, + '../../src/support/serenity/serenity-active.js': { + isSerenityActiveForOrg, + isAsyncProvisioningKillSwitched, + }, '../../src/support/serenity/workspace-resolver.js': { resolveWorkspaceId }, '../../src/support/serenity/async-job-runner.js': { createAndEnqueueJob }, '../../src/support/serenity/handlers/provision-workspace-job.js': { @@ -5340,6 +5352,7 @@ describe('Brands Controller', () => { ...(upsertBrand ? { upsertBrand } : {}), beginProvisioningAttempt, promoteProvisioningFailed, + updateProvisioningJobId, }, }); return Mocked.default(context, loggerStub, mockEnv); @@ -5379,14 +5392,48 @@ describe('Brands Controller', () => { expect(enqueueStub).to.not.have.been.called; }); - it('mints a provisioning attempt and enqueues the provision->create-market job chain, answering 202', async () => { + it('runs the SYNCHRONOUS provisionBrandSubworkspace call when the global async switch is off, even with async: true (LLMO-7418 master switch)', async () => { + // The master switch is DEFAULT OFF in production. An `async: true` create must then fall + // through to the synchronous branch — NOT error — so this whole stack merges inert and + // async is turned on deliberately. `env: {}` on the context wins over mockEnv's `'true'`. + const provisionStub = sinon.stub().resolves({ semrushSubWorkspaceId: 'ws-1' }); const upsertStub = sinon.stub().resolves({ id: 'forced-id', name: 'New Brand' }); const beginStub = sinon.stub().resolves(true); const enqueueStub = sinon.stub().resolves({ getId: () => 'job-xyz' }); + const controller = await buildController({ + provisionBrandSubworkspace: provisionStub, + upsertBrand: upsertStub, + beginProvisioningAttempt: beginStub, + createAndEnqueueJob: enqueueStub, + }); + + const response = await controller.createBrandForOrg({ + ...context, + env: {}, // switch absent => off + params: { spaceCatId: ORGANIZATION_ID }, + data: { ...semrushData }, + dataAccess: mockDataAccess, + attributes: { authInfo: { getType: () => 'ims', profile: { email: 'user@test.com' } } }, + }); + + // Synchronous path: a 201 with the workspace already bound, not a 202 with a job id. + expect(response.status).to.equal(201); + expect(provisionStub.calledOnce).to.equal(true); + expect(upsertStub.firstCall.args[0].semrushSubWorkspaceId).to.equal('ws-1'); + expect(beginStub).to.not.have.been.called; + expect(enqueueStub).to.not.have.been.called; + }); + + it('mints a provisioning attempt and enqueues the provision->create-market job chain, answering 202', async () => { + const upsertStub = sinon.stub().resolves({ id: 'forced-id', name: 'New Brand', status: 'pending' }); + const beginStub = sinon.stub().resolves(true); + const enqueueStub = sinon.stub().resolves({ getId: () => 'job-xyz' }); + const updateProvisioningJobIdStub = sinon.stub().resolves(true); const controller = await buildController({ upsertBrand: upsertStub, beginProvisioningAttempt: beginStub, createAndEnqueueJob: enqueueStub, + updateProvisioningJobId: updateProvisioningJobIdStub, }); const response = await controller.createBrandForOrg({ @@ -5399,6 +5446,9 @@ describe('Brands Controller', () => { expect(response.status).to.equal(202); const body = await response.json(); + // LLMO-7418 external-review Medium finding: the 202 body's `status` must be the + // brand's REAL persisted status (same shape the sync 201 response returns), not a + // hardcoded literal that clobbers it via spread ordering. expect(body.status).to.equal('pending'); expect(body.jobId).to.equal('job-xyz'); @@ -5414,6 +5464,14 @@ describe('Brands Controller', () => { expect(enqueueStub.calledOnce).to.equal(true); expect(enqueueStub.calledAfter(beginStub)).to.equal(true); + // LLMO-7418 external-review Finding 17: the first hop's job id is recorded, not left + // permanently NULL — only the worker's own self-requeue path used to write this. + expect(updateProvisioningJobIdStub).to.have.been.calledOnceWith({ + brandId: upsertArgs.forceBrandId, + attemptId: beginStub.firstCall.args[0].attemptId, + jobId: 'job-xyz', + postgrestClient: sinon.match.any, + }); const [, enqueueArgs] = enqueueStub.firstCall.args; expect(enqueueArgs.jobType).to.equal('serenity-provision-workspace'); expect(enqueueArgs.metadata.brandId).to.equal(upsertArgs.forceBrandId); @@ -5504,6 +5562,28 @@ describe('Brands Controller', () => { expect(enqueueStub).to.not.have.been.called; }); + it('returns 503 without enqueuing when the async kill switch is on (LLMO-7418 external-review Finding 15)', async () => { + const beginStub = sinon.stub().resolves(true); + const enqueueStub = sinon.stub().resolves({ getId: () => 'job-xyz' }); + const controller = await buildController({ + beginProvisioningAttempt: beginStub, + createAndEnqueueJob: enqueueStub, + isAsyncProvisioningKillSwitched: sinon.stub().resolves(true), + }); + + const response = await controller.createBrandForOrg({ + ...context, + params: { spaceCatId: ORGANIZATION_ID }, + data: { ...semrushData }, + dataAccess: mockDataAccess, + attributes: { authInfo: { getType: () => 'ims', profile: { email: 'user@test.com' } } }, + }); + + expect(response.status).to.equal(503); + expect(beginStub).to.not.have.been.called; + expect(enqueueStub).to.not.have.been.called; + }); + it('returns 400 without persisting a row when the organization has no Semrush parent workspace configured', async () => { const upsertStub = sinon.stub().resolves({ id: 'forced-id', name: 'New Brand' }); const controller = await buildController({ @@ -5743,8 +5823,10 @@ describe('Brands Controller', () => { it('creates a bare sub-workspace (no project) when no market is supplied (serenity-active)', async () => { // LLMO-6405: market-scoped inputs moved to market creation, so a serenity-active - // create with no market provisions just the sub-workspace (no project), synchronously - // (unconverted, out of PR-C's scope) — the async job chain is never started. + // create with no market provisions just the sub-workspace (no project). Phase 4 + // (LLMO-7352/LLMO-7418) converted this branch too, but async is absent/false here, so + // this stays the EXACT synchronous provisionBrandSubworkspaceBare call — see the + // dedicated async tests below for the `async: true` job-chain path. const bareStub = sinon.stub().resolves({ semrushSubWorkspaceId: 'ws-bare' }); const upsertStub = sinon.stub().resolves({ id: 'forced-id', name: 'New Brand' }); const enqueueStub = sinon.stub().resolves({ getId: () => 'job-xyz' }); @@ -5771,6 +5853,222 @@ describe('Brands Controller', () => { expect(upsertArgs.brand.baseSiteId).to.equal('site-123'); }); + it('Phase 4: bare create mints a provisioning attempt and enqueues provision-workspace-job with NO chained job when async: true', async () => { + const bareStub = sinon.stub().resolves({ semrushSubWorkspaceId: 'ws-bare' }); + // A baseSiteId is supplied below, so upsertBrand's own hasAnchor invariant persists + // this brand as 'active' immediately — the async path only defers Semrush + // sub-workspace provisioning, not the brand's own lifecycle status. + const upsertStub = sinon.stub().resolves({ id: 'forced-id', name: 'New Brand', status: 'active' }); + const enqueueStub = sinon.stub().resolves({ getId: () => 'job-xyz' }); + const beginStub = sinon.stub().resolves(true); + const updateProvisioningJobIdStub = sinon.stub().resolves(true); + const controller = await buildController({ + provisionBrandSubworkspaceBare: bareStub, + upsertBrand: upsertStub, + createAndEnqueueJob: enqueueStub, + beginProvisioningAttempt: beginStub, + updateProvisioningJobId: updateProvisioningJobIdStub, + }); + + const response = await controller.createBrandForOrg({ + ...context, + params: { spaceCatId: ORGANIZATION_ID }, + data: { name: 'New Brand', baseSiteId: 'site-123', async: true }, + dataAccess: mockDataAccess, + attributes: { authInfo: { getType: () => 'ims', profile: { email: 'user@test.com' } } }, + }); + + expect(response.status).to.equal(202); + const body = await response.json(); + // LLMO-7418 external-review Medium finding: the 202 body's `status` must be the + // brand's REAL persisted status (same shape the sync 201 response returns), not a + // hardcoded literal that clobbers it via spread ordering. + expect(body.status).to.equal('active'); + expect(body.jobId).to.equal('job-xyz'); + // The bare-sync provisioner never runs on the async path. + expect(bareStub.called).to.equal(false); + expect(enqueueStub.calledOnce).to.equal(true); + const [, enqueueArgs] = enqueueStub.firstCall.args; + expect(enqueueArgs.jobType).to.equal('serenity-provision-workspace'); + expect(enqueueArgs.metadata.parentWorkspaceId).to.equal('parent-ws-1'); + expect(enqueueArgs.metadata.chainedJobType).to.equal(undefined); + expect(enqueueArgs.metadata.chainedJobMetadata).to.equal(undefined); + // The row is persisted with no workspace pointer yet. + const upsertArgs = upsertStub.firstCall.args[0]; + expect(upsertArgs.semrushSubWorkspaceId).to.equal(null); + // LLMO-7418 external-review Finding 17: the first hop's job id is recorded, not left + // permanently NULL — only the worker's own self-requeue path used to write this. + expect(updateProvisioningJobIdStub).to.have.been.calledOnceWith({ + brandId: upsertArgs.forceBrandId, + attemptId: beginStub.firstCall.args[0].attemptId, + jobId: 'job-xyz', + postgrestClient: sinon.match.any, + }); + }); + + it('Phase 4: bare create runs the SYNCHRONOUS provisionBrandSubworkspaceBare call when the global async switch is off, even with async: true (LLMO-7418 master switch)', async () => { + const bareStub = sinon.stub().resolves({ semrushSubWorkspaceId: 'ws-bare' }); + const upsertStub = sinon.stub().resolves({ id: 'forced-id', name: 'New Brand' }); + const beginStub = sinon.stub().resolves(true); + const enqueueStub = sinon.stub().resolves({ getId: () => 'job-xyz' }); + const controller = await buildController({ + provisionBrandSubworkspaceBare: bareStub, + upsertBrand: upsertStub, + beginProvisioningAttempt: beginStub, + createAndEnqueueJob: enqueueStub, + }); + + const response = await controller.createBrandForOrg({ + ...context, + env: {}, // switch absent => off + params: { spaceCatId: ORGANIZATION_ID }, + data: { name: 'New Brand', baseSiteId: 'site-123', async: true }, + dataAccess: mockDataAccess, + attributes: { authInfo: { getType: () => 'ims', profile: { email: 'user@test.com' } } }, + }); + + expect(response.status).to.equal(201); + expect(bareStub.called).to.equal(true); + expect(upsertStub.firstCall.args[0].semrushSubWorkspaceId).to.equal('ws-bare'); + expect(beginStub).to.not.have.been.called; + expect(enqueueStub).to.not.have.been.called; + }); + + it('Phase 4: bare create returns 409 without enqueuing when a provisioning attempt is already in flight', async () => { + const upsertStub = sinon.stub().resolves({ id: 'forced-id', name: 'New Brand' }); + const beginStub = sinon.stub().resolves(false); + const enqueueStub = sinon.stub().resolves({ getId: () => 'job-xyz' }); + const controller = await buildController({ + upsertBrand: upsertStub, + beginProvisioningAttempt: beginStub, + createAndEnqueueJob: enqueueStub, + }); + + const response = await controller.createBrandForOrg({ + ...context, + params: { spaceCatId: ORGANIZATION_ID }, + data: { name: 'New Brand', baseSiteId: 'site-123', async: true }, + dataAccess: mockDataAccess, + attributes: { authInfo: { getType: () => 'ims', profile: { email: 'user@test.com' } } }, + }); + + expect(response.status).to.equal(409); + expect(enqueueStub.called).to.equal(false); + }); + + it('Phase 4: bare create returns 503 without enqueuing when the async kill switch is on (LLMO-7418 external-review Finding 15)', async () => { + const beginStub = sinon.stub().resolves(true); + const enqueueStub = sinon.stub().resolves({ getId: () => 'job-xyz' }); + const controller = await buildController({ + beginProvisioningAttempt: beginStub, + createAndEnqueueJob: enqueueStub, + isAsyncProvisioningKillSwitched: sinon.stub().resolves(true), + }); + + const response = await controller.createBrandForOrg({ + ...context, + params: { spaceCatId: ORGANIZATION_ID }, + data: { name: 'New Brand', baseSiteId: 'site-123', async: true }, + dataAccess: mockDataAccess, + attributes: { authInfo: { getType: () => 'ims', profile: { email: 'user@test.com' } } }, + }); + + expect(response.status).to.equal(503); + expect(beginStub.called).to.equal(false); + expect(enqueueStub.called).to.equal(false); + }); + + it('Phase 4: bare create returns 400 without persisting a row when the organization has no Semrush parent workspace configured', async () => { + const upsertStub = sinon.stub().resolves({ id: 'forced-id', name: 'New Brand' }); + const controller = await buildController({ + upsertBrand: upsertStub, resolveWorkspaceId: sinon.stub().resolves(null), + }); + + const response = await controller.createBrandForOrg({ + ...context, + params: { spaceCatId: ORGANIZATION_ID }, + data: { name: 'New Brand', baseSiteId: 'site-123', async: true }, + dataAccess: mockDataAccess, + attributes: { authInfo: { getType: () => 'ims', profile: { email: 'user@test.com' } } }, + }); + + expect(response.status).to.equal(400); + expect(upsertStub.called).to.equal(false); + }); + + it('Phase 4: bare create records the provisioning-start failure and rethrows when the job enqueue itself fails', async () => { + const upsertStub = sinon.stub().resolves({ id: 'forced-id', name: 'New Brand' }); + const enqueueStub = sinon.stub().rejects(new Error('SQS unavailable')); + const promoteFailedStub = sinon.stub().resolves(true); + const controller = await buildController({ + upsertBrand: upsertStub, + createAndEnqueueJob: enqueueStub, + promoteProvisioningFailed: promoteFailedStub, + }); + + const response = await controller.createBrandForOrg({ + ...context, + params: { spaceCatId: ORGANIZATION_ID }, + data: { name: 'New Brand', baseSiteId: 'site-123', async: true }, + dataAccess: mockDataAccess, + attributes: { authInfo: { getType: () => 'ims', profile: { email: 'user@test.com' } } }, + }); + + // The row IS already persisted (visible, non-active) — marked failed rather than + // left silently inert — but the request itself surfaces the original error. + expect(response.status).to.equal(500); + expect(promoteFailedStub.calledOnce).to.equal(true); + expect(promoteFailedStub.firstCall.args[0].brandId) + .to.equal(upsertStub.firstCall.args[0].forceBrandId); + }); + + it('Phase 4: bare create records the provisioning-start failure via the fresh-brand path and rethrows when beginProvisioningAttempt itself fails', async () => { + const upsertStub = sinon.stub().resolves({ id: 'forced-id', name: 'New Brand' }); + const beginStub = sinon.stub().rejects(new Error('DB unavailable')); + const enqueueStub = sinon.stub().resolves({ getId: () => 'job-xyz' }); + const promoteFailedStub = sinon.stub().resolves(true); + const recordFreshFailureStub = sinon.stub().resolves(true); + const Mocked = await esmock('../../src/controllers/brands.js', { + '../../src/support/serenity/brand-provisioning.js': { + provisionBrandSubworkspaceBare: sinon.stub().resolves({ semrushSubWorkspaceId: 'ws-bare' }), + }, + '../../src/support/serenity/serenity-active.js': { isSerenityActiveForOrg: sinon.stub().resolves(true) }, + '../../src/support/serenity/workspace-resolver.js': { resolveWorkspaceId: sinon.stub().resolves('parent-ws-1') }, + '../../src/support/serenity/async-job-runner.js': { createAndEnqueueJob: enqueueStub }, + '../../src/support/serenity/handlers/provision-workspace-job.js': { + PROVISION_WORKSPACE_JOB_TYPE: 'serenity-provision-workspace', + }, + '../../src/support/serenity/handlers/create-market-job.js': { + CREATE_MARKET_JOB_TYPE: 'serenity-create-market', + }, + '../../src/support/brands-storage.js': { + upsertBrand: upsertStub, + beginProvisioningAttempt: beginStub, + promoteProvisioningFailed: promoteFailedStub, + recordFreshBrandProvisioningStartFailure: recordFreshFailureStub, + }, + }); + const controller = Mocked.default(context, loggerStub, mockEnv); + + const response = await controller.createBrandForOrg({ + ...context, + params: { spaceCatId: ORGANIZATION_ID }, + data: { name: 'New Brand', baseSiteId: 'site-123', async: true }, + dataAccess: mockDataAccess, + attributes: { authInfo: { getType: () => 'ims', profile: { email: 'user@test.com' } } }, + }); + + expect(response.status).to.equal(500); + const body = await response.json(); + expect(body.message).to.equal('DB unavailable'); + expect(recordFreshFailureStub.calledOnce).to.equal(true); + expect(recordFreshFailureStub.firstCall.args[0].brandId) + .to.equal(upsertStub.firstCall.args[0].forceBrandId); + // The pre-existing established-brand compensation must NOT fire for a fresh brand. + expect(promoteFailedStub.called).to.equal(false); + expect(enqueueStub.called).to.equal(false); + }); + it('surfaces a bare sub-workspace provisioning failure and does not write the brand', async () => { // The bare sub-workspace is provisioned BEFORE the brand row is written; if // that upstream call throws, the create surfaces the error and never persists @@ -10101,6 +10399,9 @@ describe('Brands Controller — defensive branch coverage', () => { BRAND_IMS_CLIENT_ID: 'client', BRAND_IMS_CLIENT_CODE: 'code', BRAND_IMS_CLIENT_SECRET: 'secret', + // LLMO-7418: global async-provisioning master switch (DEFAULT OFF in production). Enabled + // here so this block's `async: true` cases exercise the async branch they assert on. + SERENITY_ASYNC_PROVISIONING_ENABLED: 'true', }; function buildContext() { diff --git a/test/controllers/serenity.test.js b/test/controllers/serenity.test.js index cc3b921ada..a37cec5b23 100644 --- a/test/controllers/serenity.test.js +++ b/test/controllers/serenity.test.js @@ -115,7 +115,11 @@ function fakeContext({ data = undefined, brandId = BRAND, brand = makeBrandModel(), - env = {}, + // LLMO-7418: the global async-provisioning master switch is DEFAULT OFF in production, so an + // `async: true` request falls through to the synchronous branch unless it is enabled. Default it + // ON here so the existing async-path tests exercise the async branch they were written for; the + // OFF behaviour is covered by its own dedicated tests, which pass an env without this flag. + env = { SERENITY_ASYNC_PROVISIONING_ENABLED: 'true' }, promiseToken = undefined, headers = {}, } = {}) { @@ -199,6 +203,7 @@ describe('SerenityController', () => { let resolveWorkspaceIdStub; let resolveBrandWorkspaceStub; let isSerenityActiveStub; + let isAsyncProvisioningKillSwitchedStub; let createTransportStub; let resolveBrandUuidStub; let getBrandAliasesStub; @@ -238,6 +243,9 @@ describe('SerenityController', () => { // existing assertion that drives a brand-level route reaches its handler. // The "serenity inactive" describe overrides this to false. isSerenityActiveStub = sinon.stub().resolves(true); + // LLMO-7418 external-review Finding 15: kill switch off by default (async provisioning + // available); specific tests override it to resolve(true) to exercise the 503 gate. + isAsyncProvisioningKillSwitchedStub = sinon.stub().resolves(false); decommissionStub = sinon.stub().resolves(); ensureSubworkspaceStub = sinon.stub().resolves(SUBWS); clearBrandWorkspaceCacheStub = sinon.stub(); @@ -355,6 +363,7 @@ describe('SerenityController', () => { }, '../../src/support/serenity/serenity-active.js': { isSerenityActiveForBrand: isSerenityActiveStub, + isAsyncProvisioningKillSwitched: isAsyncProvisioningKillSwitchedStub, }, '../../src/support/access-control-util.js': MockAccessControlUtil, '../../src/support/prompts-storage.js': { @@ -421,6 +430,9 @@ describe('SerenityController', () => { '../../src/support/serenity/handlers/activate-markets-job.js': { ACTIVATE_MARKETS_JOB_TYPE: 'serenity-activate-markets', }, + '../../src/support/serenity/handlers/activate-brand-workspace-job.js': { + ACTIVATE_BRAND_WORKSPACE_JOB_TYPE: 'serenity-activate-brand-workspace', + }, })).default; }); @@ -1689,6 +1701,26 @@ describe('SerenityController', () => { expect(handlers.handleGetMarket).to.not.have.been.called; }); + it('createMarket runs the SYNCHRONOUS branch when the global async switch is off, even with async: true (LLMO-7418 master switch)', async () => { + // The master switch is DEFAULT OFF in production. An `async: true` request must then fall + // through to the synchronous branch — NOT error — so this whole stack can merge inert and + // async is turned on deliberately. This is what makes shipping safe by default. + const controller = SerenityController({ env: {} }, fakeLog(), {}); + const ctx = fakeContext({ + env: {}, // switch absent => off + data: { + market: 'us', languageCode: 'en', brandDomain: 'x.com', brandNames: ['X'], async: true, + }, + }); + + const response = await controller.createMarket(ctx); + + // Synchronous path: not a 202, and no provisioning attempt or job was created. + expect(response.status).to.not.equal(202); + expect(beginProvisioningAttemptStub).to.not.have.been.called; + expect(createAndEnqueueJobStub).to.not.have.been.called; + }); + it('createMarket (PR-C) mints a provisioning attempt and enqueues the provision->create-market job chain, answering 202', async () => { const controller = SerenityController({ env: {} }, fakeLog(), {}); const ctx = fakeContext({ @@ -1748,6 +1780,20 @@ describe('SerenityController', () => { expect(createAndEnqueueJobStub).to.not.have.been.called; }); + it('createMarket answers 503 without enqueuing anything when the async kill switch is on (LLMO-7418 external-review Finding 15)', async () => { + isAsyncProvisioningKillSwitchedStub.resolves(true); + const controller = SerenityController({ env: {} }, fakeLog(), {}); + const response = await controller.createMarket(fakeContext({ + data: { + market: 'us', languageCode: 'en', brandDomain: 'x.com', brandNames: ['X'], async: true, + }, + })); + expect(response.status).to.equal(503); + expect(guardAgainstConcurrentProvisioningStub).to.not.have.been.called; + expect(beginProvisioningAttemptStub).to.not.have.been.called; + expect(createAndEnqueueJobStub).to.not.have.been.called; + }); + it('createMarket runs the SAME synchronous orchestration it always has when async is absent (regression: default behavior unchanged)', async () => { orchestrateCreateMarketSubworkspaceStub.resolves({ status: 201, body: { brandId: BRAND, geoTargetId: 2840, languageCode: 'en' }, @@ -1782,7 +1828,7 @@ describe('SerenityController', () => { 'A Semrush sub-workspace provisioning attempt is already in progress for this brand; please retry shortly.', 409, ); - conflictErr.code = 'semrush_provisioning_in_progress'; + conflictErr.code = 'semrushProvisioningInProgress'; guardAgainstConcurrentProvisioningStub.rejects(conflictErr); const controller = SerenityController({ env: {} }, fakeLog(), {}); const response = await controller.createMarket(fakeContext({ @@ -2303,6 +2349,21 @@ describe('SerenityController', () => { expect(createAndEnqueueJobStub).to.not.have.been.called; }); + it('activate answers 503 without enqueuing when async: true and the async kill switch is on (LLMO-7418 external-review Finding 15)', async () => { + isAsyncProvisioningKillSwitchedStub.resolves(true); + const brand = makeBrandModel({ getStatus: () => 'active' }); + const controller = SerenityController({ env: {} }, fakeLog(), {}); + const response = await controller.activate(fakeContext({ + brand, + data: { + brandDomain: 'x.com', brandNames: ['X'], markets: [{ market: 'us', languageCode: 'en' }], async: true, + }, + })); + expect(response.status).to.equal(503); + expect(beginProvisioningAttemptStub).to.not.have.been.called; + expect(createAndEnqueueJobStub).to.not.have.been.called; + }); + it('activate 400s when async is present but not a boolean', async () => { const brand = makeBrandModel({ getStatus: () => 'active' }); const controller = SerenityController({ env: {} }, fakeLog(), {}); @@ -2335,6 +2396,29 @@ describe('SerenityController', () => { expect(createAndEnqueueJobStub).to.not.have.been.called; }); + it('activate runs the SYNCHRONOUS market batch when the global async switch is off, even with async: true (LLMO-7418 master switch)', async () => { + // Mirrors createMarket's master-switch test, for activate's project-activation branch. + // The switch is DEFAULT OFF in production, so `async: true` must fall through to the + // synchronous orchestration rather than error — that is what lets the stack merge inert. + orchestrateActivateMarketsStub.resolves({ + status: 200, body: { brandId: BRAND, status: 'active', markets: [] }, + }); + const brand = makeBrandModel({ getStatus: () => 'active' }); + const controller = SerenityController({ env: {} }, fakeLog(), {}); + const response = await controller.activate(fakeContext({ + brand, + env: {}, // switch absent => off + data: { + brandDomain: 'x.com', brandNames: ['X'], markets: [{ market: 'us', languageCode: 'en' }], async: true, + }, + })); + + expect(response.status).to.equal(200); + expect(orchestrateActivateMarketsStub).to.have.been.calledOnce; + expect(beginProvisioningAttemptStub).to.not.have.been.called; + expect(createAndEnqueueJobStub).to.not.have.been.called; + }); + it('activate 400s when generatePrompts is true but there is no primary URL (nothing to generate into)', async () => { const brand = makeBrandModel({ getStatus: () => 'active' }); const controller = SerenityController({ env: {} }, fakeLog(), {}); @@ -2427,7 +2511,7 @@ describe('SerenityController', () => { 'A Semrush sub-workspace provisioning attempt is already in progress for this brand; please retry shortly.', 409, ); - conflictErr.code = 'semrush_provisioning_in_progress'; + conflictErr.code = 'semrushProvisioningInProgress'; guardAgainstConcurrentProvisioningStub.rejects(conflictErr); getBrandBaseSiteIdStub.resolves('primary-site'); const brand = makeBrandModel({}); @@ -2435,7 +2519,7 @@ describe('SerenityController', () => { const response = await controller.activate(fakeContext({ brand, data: { brandNames: ['X'] } })); expect(response.status).to.equal(409); const { error } = await readBody(response); - expect(error).to.equal('semrush_provisioning_in_progress'); + expect(error).to.equal('semrushProvisioningInProgress'); expect(ensureSubworkspaceStub).to.not.have.been.called; expect(brand.setStatus).to.not.have.been.called; }); @@ -2445,7 +2529,7 @@ describe('SerenityController', () => { 'A Semrush sub-workspace provisioning attempt is already in progress for this brand; please retry shortly.', 409, ); - conflictErr.code = 'semrush_provisioning_in_progress'; + conflictErr.code = 'semrushProvisioningInProgress'; guardAgainstConcurrentProvisioningStub.rejects(conflictErr); const brand = makeBrandModel({ getStatus: () => 'active' }); const controller = SerenityController({ env: {} }, fakeLog(), {}); @@ -2454,6 +2538,202 @@ describe('SerenityController', () => { expect(ensureSubworkspaceStub).to.not.have.been.called; }); + it('Phase 4: pending→active activation mints a provisioning attempt and enqueues the provision->activate-brand-workspace job chain when async: true', async () => { + getBrandBaseSiteIdStub.resolves('primary-site'); + const brand = makeBrandModel({}); + const controller = SerenityController({ env: {} }, fakeLog(), {}); + const response = await controller.activate(fakeContext({ + brand, data: { brandNames: ['X'], async: true }, + })); + + expect(response.status).to.equal(202); + expect(ensureSubworkspaceStub).to.not.have.been.called; + // LLMO-7418 external-review Finding 9: reconciles a stale in-flight attempt first. + expect(guardAgainstConcurrentProvisioningStub).to.have.been.calledOnceWith(BRAND); + expect(brand.setStatus).to.not.have.been.called; + expect(beginProvisioningAttemptStub).to.have.been.calledOnce; + expect(beginProvisioningAttemptStub.firstCall.args[0]).to.include({ + brandId: BRAND, updatedBy: 'serenity-activate', + }); + expect(createAndEnqueueJobStub).to.have.been.calledOnce; + const [, enqueueArgs] = createAndEnqueueJobStub.firstCall.args; + expect(enqueueArgs.jobType).to.equal('serenity-provision-workspace'); + expect(enqueueArgs.metadata.chainedJobType).to.equal('serenity-activate-brand-workspace'); + expect(enqueueArgs.metadata.chainedJobMetadata) + .to.deep.equal({ brandId: BRAND, wasPending: true }); + // LLMO-7418 external-review Finding 4: a pending brand is GUARANTEED pointer-less, so the + // worker always takes the create path here — omitting title would create an untitled + // sub-workspace on every single async pending->active activation. + expect(enqueueArgs.metadata.title).to.equal('Test Brand'); + // LLMO-7418 external-review Finding 17: the first hop's job id is recorded, not left + // permanently NULL — only the worker's own self-requeue path used to write this. + expect(updateProvisioningJobIdStub).to.have.been.calledOnceWith({ + brandId: BRAND, + attemptId: beginProvisioningAttemptStub.firstCall.args[0].attemptId, + jobId: 'job-abc', + postgrestClient: sinon.match.any, + }); + }); + + it('Phase 4: pending→active activation answers 409 without enqueuing when async: true and a provisioning attempt is already in flight', async () => { + getBrandBaseSiteIdStub.resolves('primary-site'); + beginProvisioningAttemptStub.resolves(false); + const brand = makeBrandModel({}); + const controller = SerenityController({ env: {} }, fakeLog(), {}); + const response = await controller.activate(fakeContext({ + brand, data: { brandNames: ['X'], async: true }, + })); + expect(response.status).to.equal(409); + expect(createAndEnqueueJobStub).to.not.have.been.called; + }); + + it('Phase 4: pending→active activation answers 503 without enqueuing when the async kill switch is on (LLMO-7418 external-review Finding 15)', async () => { + getBrandBaseSiteIdStub.resolves('primary-site'); + isAsyncProvisioningKillSwitchedStub.resolves(true); + const brand = makeBrandModel({}); + const controller = SerenityController({ env: {} }, fakeLog(), {}); + const response = await controller.activate(fakeContext({ + brand, data: { brandNames: ['X'], async: true }, + })); + expect(response.status).to.equal(503); + expect(beginProvisioningAttemptStub).to.not.have.been.called; + expect(createAndEnqueueJobStub).to.not.have.been.called; + }); + + it('Phase 4: pending→active activation 400s when async is present but not a boolean', async () => { + getBrandBaseSiteIdStub.resolves('primary-site'); + const brand = makeBrandModel({}); + const controller = SerenityController({ env: {} }, fakeLog(), {}); + const response = await controller.activate(fakeContext({ + brand, data: { brandNames: ['X'], async: 'yes' }, + })); + expect(response.status).to.equal(400); + expect(createAndEnqueueJobStub).to.not.have.been.called; + expect(ensureSubworkspaceStub).to.not.have.been.called; + }); + + it('Phase 4: pending→active activation runs the same synchronous flip as an absent flag when async is explicitly false', async () => { + getBrandBaseSiteIdStub.resolves('primary-site'); + handlers.handleCreateMarketSubworkspace.resolves({ status: 201, body: {} }); + const brand = makeBrandModel({}); + const controller = SerenityController({ env: {} }, fakeLog(), {}); + const response = await controller.activate(fakeContext({ + brand, data: { brandNames: ['X'], async: false }, + })); + expect(response.status).to.equal(200); + expect(guardAgainstConcurrentProvisioningStub).to.have.been.calledOnce; + expect(ensureSubworkspaceStub).to.have.been.calledOnce; + expect(createAndEnqueueJobStub).to.not.have.been.called; + }); + + it('Phase 4: pending→active activation runs the SYNCHRONOUS flip when the global async switch is off, even with async: true (LLMO-7418 master switch)', async () => { + getBrandBaseSiteIdStub.resolves('primary-site'); + handlers.handleCreateMarketSubworkspace.resolves({ status: 201, body: {} }); + const brand = makeBrandModel({}); + const controller = SerenityController({ env: {} }, fakeLog(), {}); + const response = await controller.activate(fakeContext({ + brand, + env: {}, // switch absent => off + data: { brandNames: ['X'], async: true }, + })); + + expect(response.status).to.equal(200); + expect(ensureSubworkspaceStub).to.have.been.calledOnce; + expect(beginProvisioningAttemptStub).to.not.have.been.called; + expect(createAndEnqueueJobStub).to.not.have.been.called; + }); + + it('Phase 4: bare reactivation mints a provisioning attempt and enqueues the provision->activate-brand-workspace job chain when async: true', async () => { + const brand = makeBrandModel({ getStatus: () => 'active' }); + const controller = SerenityController({ env: {} }, fakeLog(), {}); + const response = await controller.activate(fakeContext({ + brand, data: { brandNames: ['X'], async: true }, + })); + + expect(response.status).to.equal(202); + expect(ensureSubworkspaceStub).to.not.have.been.called; + // LLMO-7418 external-review Finding 9: reconciles a stale in-flight attempt first. + expect(guardAgainstConcurrentProvisioningStub).to.have.been.calledOnceWith(BRAND); + expect(createAndEnqueueJobStub).to.have.been.calledOnce; + const [, enqueueArgs] = createAndEnqueueJobStub.firstCall.args; + expect(enqueueArgs.metadata.chainedJobType).to.equal('serenity-activate-brand-workspace'); + expect(enqueueArgs.metadata.chainedJobMetadata) + .to.deep.equal({ brandId: BRAND, wasPending: false }); + // LLMO-7418 external-review Finding 4: this branch's own synchronous twin defensively + // handles a pointer-less brand via ensureSubworkspace, so the async path needs a real + // title too rather than assuming a pointer always exists. + expect(enqueueArgs.metadata.title).to.equal('Test Brand'); + // LLMO-7418 external-review Finding 17: the first hop's job id is recorded, not left + // permanently NULL — only the worker's own self-requeue path used to write this. + expect(updateProvisioningJobIdStub).to.have.been.calledOnceWith({ + brandId: BRAND, + attemptId: beginProvisioningAttemptStub.firstCall.args[0].attemptId, + jobId: 'job-abc', + postgrestClient: sinon.match.any, + }); + }); + + it('Phase 4: bare reactivation answers 409 without enqueuing when async: true and a provisioning attempt is already in flight', async () => { + beginProvisioningAttemptStub.resolves(false); + const brand = makeBrandModel({ getStatus: () => 'active' }); + const controller = SerenityController({ env: {} }, fakeLog(), {}); + const response = await controller.activate(fakeContext({ + brand, data: { brandNames: ['X'], async: true }, + })); + expect(response.status).to.equal(409); + expect(createAndEnqueueJobStub).to.not.have.been.called; + }); + + it('Phase 4: bare reactivation answers 503 without enqueuing when the async kill switch is on (LLMO-7418 external-review Finding 15)', async () => { + isAsyncProvisioningKillSwitchedStub.resolves(true); + const brand = makeBrandModel({ getStatus: () => 'active' }); + const controller = SerenityController({ env: {} }, fakeLog(), {}); + const response = await controller.activate(fakeContext({ + brand, data: { brandNames: ['X'], async: true }, + })); + expect(response.status).to.equal(503); + expect(beginProvisioningAttemptStub).to.not.have.been.called; + expect(createAndEnqueueJobStub).to.not.have.been.called; + }); + + it('Phase 4: bare reactivation 400s when async is present but not a boolean', async () => { + const brand = makeBrandModel({ getStatus: () => 'active' }); + const controller = SerenityController({ env: {} }, fakeLog(), {}); + const response = await controller.activate(fakeContext({ + brand, data: { brandNames: ['X'], async: 'yes' }, + })); + expect(response.status).to.equal(400); + expect(createAndEnqueueJobStub).to.not.have.been.called; + expect(ensureSubworkspaceStub).to.not.have.been.called; + }); + + it('Phase 4: bare reactivation runs the same synchronous flip as an absent flag when async is explicitly false', async () => { + const brand = makeBrandModel({ getStatus: () => 'active' }); + const controller = SerenityController({ env: {} }, fakeLog(), {}); + const response = await controller.activate(fakeContext({ + brand, data: { brandNames: ['X'], async: false }, + })); + expect(response.status).to.equal(200); + expect(guardAgainstConcurrentProvisioningStub).to.have.been.calledOnce; + expect(ensureSubworkspaceStub).to.have.been.calledOnce; + expect(createAndEnqueueJobStub).to.not.have.been.called; + }); + + it('Phase 4: bare reactivation runs the SYNCHRONOUS flip when the global async switch is off, even with async: true (LLMO-7418 master switch)', async () => { + const brand = makeBrandModel({ getStatus: () => 'active' }); + const controller = SerenityController({ env: {} }, fakeLog(), {}); + const response = await controller.activate(fakeContext({ + brand, + env: {}, // switch absent => off + data: { brandNames: ['X'], async: true }, + })); + + expect(response.status).to.equal(200); + expect(ensureSubworkspaceStub).to.have.been.calledOnce; + expect(beginProvisioningAttemptStub).to.not.have.been.called; + expect(createAndEnqueueJobStub).to.not.have.been.called; + }); + it('activate 400s when the markets array exceeds the cap (validated before either branch dispatches)', async () => { const markets = Array.from({ length: 51 }, (_, i) => ({ market: 'us', languageCode: `l${i}` })); const controller = SerenityController({ env: {} }, fakeLog(), {}); diff --git a/test/support/brands-storage.test.js b/test/support/brands-storage.test.js index 8d4687959a..b2f685db44 100644 --- a/test/support/brands-storage.test.js +++ b/test/support/brands-storage.test.js @@ -4949,7 +4949,7 @@ describe('brands-storage', () => { expect(caught).to.exist; expect(caught.status).to.equal(409); - expect(caught.code).to.equal('semrush_provisioning_in_progress'); + expect(caught.code).to.equal('semrushProvisioningInProgress'); }); it('reconciles a STALE attempt to failed and returns without throwing', async () => { diff --git a/test/support/serenity/handlers/activate-brand-workspace-job.test.js b/test/support/serenity/handlers/activate-brand-workspace-job.test.js new file mode 100644 index 0000000000..a703124e50 --- /dev/null +++ b/test/support/serenity/handlers/activate-brand-workspace-job.test.js @@ -0,0 +1,130 @@ +/* + * Copyright 2026 Adobe. All rights reserved. + * This file is licensed to you under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. You may obtain a copy + * of the License at http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software distributed under + * the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR REPRESENTATIONS + * OF ANY KIND, either express or implied. See the License for the specific language + * governing permissions and limitations under the License. + */ + +import { expect } from 'chai'; +import sinon from 'sinon'; +import { activateBrandWorkspaceJobHandler, ACTIVATE_BRAND_WORKSPACE_JOB_TYPE } from '../../../../src/support/serenity/handlers/activate-brand-workspace-job.js'; + +const BRAND_ID = 'brand-1'; + +function fakeLog() { + return { + info: sinon.stub(), warn: sinon.stub(), error: sinon.stub(), debug: sinon.stub(), + }; +} + +function makeJob(metadata) { + return { getId: () => 'job-1', getMetadata: () => metadata }; +} + +function makeBrand(status) { + return { getStatus: () => status }; +} + +describe('handlers/activate-brand-workspace-job.js (Phase 4, LLMO-7352/LLMO-7418)', () => { + let context; + let findByIdStub; + + beforeEach(() => { + findByIdStub = sinon.stub(); + context = { + log: fakeLog(), + dataAccess: { Brand: { findById: findByIdStub } }, + }; + }); + + it('exports the expected job type constant', () => { + expect(ACTIVATE_BRAND_WORKSPACE_JOB_TYPE).to.equal('serenity-activate-brand-workspace'); + }); + + it('reports 200/active without writing anything when the brand is (still) active — the common case (wasPending: true)', async () => { + findByIdStub.resolves(makeBrand('active')); + const job = makeJob({ brandId: BRAND_ID, wasPending: true }); + + const result = await activateBrandWorkspaceJobHandler(context, job); + + expect(findByIdStub).to.have.been.calledOnceWith(BRAND_ID); + expect(result).to.deep.equal({ status: 200, body: { brandId: BRAND_ID, status: 'active', markets: [] } }); + }); + + it('reports 200/active without writing anything when the brand is (still) active (wasPending: false)', async () => { + findByIdStub.resolves(makeBrand('active')); + const job = makeJob({ brandId: BRAND_ID, wasPending: false }); + + const result = await activateBrandWorkspaceJobHandler(context, job); + + expect(result).to.deep.equal({ status: 200, body: { brandId: BRAND_ID, status: 'active', markets: [] } }); + }); + + it('never writes — this job has no save/setStatus to call, only a read', async () => { + const brand = makeBrand('active'); + findByIdStub.resolves(brand); + const job = makeJob({ brandId: BRAND_ID, wasPending: true }); + + await activateBrandWorkspaceJobHandler(context, job); + + // The brand fake exposes only getStatus() — no setStatus/save were even offered, and the + // handler must not assume they exist or call them. If this test needed a setStatus/save + // stub to pass, the handler would be writing again. + expect(brand).to.not.have.property('setStatus'); + expect(brand).to.not.have.property('save'); + }); + + it('reports the brand\'s ACTUAL current status (not a manufactured failure) when a concurrent deactivate raced ahead of this chain (wasPending: true)', async () => { + // promoteProvisioningReady already durably flipped the brand active before this chain was + // enqueued; finding it 'pending' here means a legitimate /serenity/deactivate ran in + // between (see the handler's own doc). Must report that truthfully, not re-write 'active'. + findByIdStub.resolves(makeBrand('pending')); + const job = makeJob({ brandId: BRAND_ID, wasPending: true }); + + const result = await activateBrandWorkspaceJobHandler(context, job); + + expect(result).to.deep.equal({ status: 207, body: { brandId: BRAND_ID, status: 'pending', markets: [] } }); + }); + + it('reports the brand\'s ACTUAL current status when a concurrent deactivate raced ahead of this chain (wasPending: false)', async () => { + findByIdStub.resolves(makeBrand('pending')); + const job = makeJob({ brandId: BRAND_ID, wasPending: false }); + + const result = await activateBrandWorkspaceJobHandler(context, job); + + expect(result).to.deep.equal({ status: 207, body: { brandId: BRAND_ID, status: 'pending', markets: [] } }); + }); + + it('logs when the brand is found not-active, for observability', async () => { + findByIdStub.resolves(makeBrand('pending')); + const job = makeJob({ brandId: BRAND_ID, wasPending: true }); + + await activateBrandWorkspaceJobHandler(context, job); + + expect(context.log.info).to.have.been.calledOnce; + }); + + it('reports 207/deleted instead of throwing when a concurrent hard-delete raced ahead of this chain (LLMO-7418 external-review Medium finding)', async () => { + findByIdStub.resolves(null); + const job = makeJob({ brandId: BRAND_ID, wasPending: true }); + + const result = await activateBrandWorkspaceJobHandler(context, job); + + expect(result).to.deep.equal({ status: 207, body: { brandId: BRAND_ID, status: 'deleted', markets: [] } }); + expect(context.log.info).to.have.been.calledOnce; + }); + + it('defaults wasPending to false when absent from metadata, without affecting the (read-only) outcome', async () => { + findByIdStub.resolves(makeBrand('active')); + const job = makeJob({ brandId: BRAND_ID }); + + const result = await activateBrandWorkspaceJobHandler(context, job); + + expect(result.status).to.equal(200); + }); +}); diff --git a/test/support/serenity/handlers/provision-workspace-job.test.js b/test/support/serenity/handlers/provision-workspace-job.test.js index 54015e57d0..20ec2052c8 100644 --- a/test/support/serenity/handlers/provision-workspace-job.test.js +++ b/test/support/serenity/handlers/provision-workspace-job.test.js @@ -763,6 +763,103 @@ describe('handlers/provision-workspace-job.js (LLMO-7352 / LLMO-7418)', () => { }); }); + describe('poll result: transient error retry classification (LLMO-7418 external-review Finding 14)', () => { + it('treats a 503 SerenityTransportError as transient and self-requeues instead of failing the attempt', async () => { + const err = new Error('Semrush GET .../status failed: 503'); + err.status = 503; + transport.getWorkspaceStatus.rejects(err); + const { provisionWorkspaceHandler } = await loadHandler(); + const job = makeJob(makeMetadata({ requeueDepth: 0 })); + + const result = await provisionWorkspaceHandler(context, job, 'token'); + + expect(createAndEnqueueJobStub).to.have.been.calledOnce; + expect(promoteProvisioningFailedStub).to.not.have.been.called; + expect(result).to.deep.equal({ requeuedJobId: 'job-followup' }); + }); + + it('retries 429/500/502/504 the same way', async () => { + for (const status of [429, 500, 502, 504]) { + const err = new Error(`upstream ${status}`); + err.status = status; + transport.getWorkspaceStatus.reset(); + transport.getWorkspaceStatus.rejects(err); + createAndEnqueueJobStub.resetHistory(); + promoteProvisioningFailedStub.resetHistory(); + // eslint-disable-next-line no-await-in-loop + const { provisionWorkspaceHandler } = await loadHandler(); + const job = makeJob(makeMetadata({ requeueDepth: 0 })); + + // eslint-disable-next-line no-await-in-loop + const result = await provisionWorkspaceHandler(context, job, 'token'); + + expect(createAndEnqueueJobStub, `status ${status}`).to.have.been.calledOnce; + expect(promoteProvisioningFailedStub, `status ${status}`).to.not.have.been.called; + expect(result, `status ${status}`).to.deep.equal({ requeuedJobId: 'job-followup' }); + } + }); + + it('treats a raw network failure (no .status) as transient and self-requeues', async () => { + transport.getWorkspaceStatus.rejects(new TypeError('fetch failed')); + const { provisionWorkspaceHandler } = await loadHandler(); + const job = makeJob(makeMetadata({ requeueDepth: 0 })); + + const result = await provisionWorkspaceHandler(context, job, 'token'); + + expect(createAndEnqueueJobStub).to.have.been.calledOnce; + expect(result).to.deep.equal({ requeuedJobId: 'job-followup' }); + }); + + it('still fails fast on a permanent 401 (expired/invalid IMS token), never self-requeuing it', async () => { + const err = new Error('Semrush GET .../status failed: 401'); + err.status = 401; + transport.getWorkspaceStatus.rejects(err); + const { provisionWorkspaceHandler } = await loadHandler(); + const job = makeJob(makeMetadata()); + + await expect(provisionWorkspaceHandler(context, job, 'token')).to.be.rejectedWith('401'); + + expect(createAndEnqueueJobStub).to.not.have.been.called; + expect(promoteProvisioningFailedStub).to.have.been.calledOnceWith({ + brandId: BRAND_ID, + attemptId: ATTEMPT_ID, + error: UNEXPECTED_ERROR_MESSAGE, + postgrestClient, + }); + }); + + it('still fails fast on a permanent 400, never self-requeuing it', async () => { + const err = new Error('Semrush GET .../status failed: 400'); + err.status = 400; + transport.getWorkspaceStatus.rejects(err); + const { provisionWorkspaceHandler } = await loadHandler(); + const job = makeJob(makeMetadata()); + + await expect(provisionWorkspaceHandler(context, job, 'token')).to.be.rejectedWith('400'); + + expect(createAndEnqueueJobStub).to.not.have.been.called; + }); + + it('still respects the requeue depth cap for a transient error at the cap (fails, does not loop forever)', async () => { + const err = new Error('upstream 503'); + err.status = 503; + transport.getWorkspaceStatus.rejects(err); + const { provisionWorkspaceHandler } = await loadHandler(); + const job = makeJob(makeMetadata({ requeueDepth: MAX_PROVISION_REQUEUE_DEPTH })); + + const result = await provisionWorkspaceHandler(context, job, 'token'); + + expect(createAndEnqueueJobStub).to.not.have.been.called; + expect(promoteProvisioningFailedStub).to.have.been.calledOnceWith({ + brandId: BRAND_ID, + attemptId: ATTEMPT_ID, + error: REQUEUE_EXHAUSTED_MESSAGE, + postgrestClient, + }); + expect(result).to.deep.equal({ provisioningStatus: 'failed' }); + }); + }); + describe('unexpected errors reaching the outer catch clean up an owned candidate (LLMO-7418 external-review Finding 16)', () => { it('cleans up a freshly-created candidate when an unexpected error is thrown BEFORE any self-requeue is enqueued', async () => { getBrandProvisioningStateStub.rejects(new Error('db read blip'));