feat(serenity): async Semrush sub-workspace provisioning worker (LLMO-7352/LLMO-7418)

[IrisAlexandrescu]

Summary

Second in the stack (base: #3223). Adds the SQS worker that provisions a Semrush sub-workspace off the request path — but wires up no producer, so nothing enqueues to it yet. Inert on merge by construction.

One create-or-adopt and one status poll per invocation, then a self-requeue with bounded backoff (5 hops), rather than blocking in-Lambda. The canonical pointer is written only after Semrush confirms the workspace is created — that is the actual fix for LLMO-7352.

Changes

• New provision-workspace-job handler, registered in the serenity job runner.
• Provisioning state on brands (status / attempt id / job id / error / candidate). Every write is an attempt-id-scoped compare-and-set, so a stale or superseded hop can never clobber a newer winner.
• An accepted-but-unconfirmed workspace id is stored as diagnostic candidate state only, never as the canonical pointer.
• Orphan cleanup on every promotion-failure path, and a redelivery of an already-terminal hop must not clean up its candidate.
• deactivate cancels an in-flight provisioning attempt.

Notes for review

• Idempotent under at-least-once SQS delivery: the worker re-reads its attempt id at the start of every invocation and exits if it is no longer current.
• It does not sleep for the lifetime of a Lambda invocation — backoff is per-message delivery delay.
• Two behavioural contracts from main now apply to these jobs and are exercised here: the typed promise-token contract, and the runner's message-type-vs-stored-type guard.

Test plan

• npm run lint, npm run type-check, npm run docs:lint — clean
• 363 passing across the worker entry, brands-storage, async-job-runner and the new handler
• CI green (12 checks)
• Rebased onto current main

Related Issues

• LLMO-7352 / LLMO-7418
• Depends on mysticat-data-service chore(deps): update dependency @adobe/helix-universal-devserver to v1.1.129 #1037 then docs: scrape job paths fixed #1040 (provisioning columns; the order matters and nothing enforces it)

🤖 Generated with Claude Code

[github-actions]

This PR will trigger a minor release when merged.

[codecov]

Codecov Report

❌ Patch coverage is 99.89919% with 1 line in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
src/support/serenity/workspace-lifecycle.js	99.25%	1 Missing ⚠️

📢 Thoughts on this report? Let us know!

[luis6156]

Review: feat(serenity): async Semrush sub-workspace provisioning worker (LLMO-7352/LLMO-7418)

Summary

The worker design is solid — the CAS-everywhere discipline is rigorous, the ownership threading for cleanup is carefully handled, and the "inert on merge" framing is the right way to land a complex piece without breaking prod. Three issues worth addressing before the producer is wired up, and one open process risk.

---

🔴 Misleading "redelivery" log when a cancelled brand's worker stands down

cancelProvisioningAttempt writes semrush_provisioning_status = 'failed' on the brand row. When the in-flight worker's NEXT delivery reads that state, it hits this branch:

if (state.provisioningStatus !== 'pending') {
  log?.info?.('provision-workspace-job: this attempt already reached a terminal state (redelivery); standing down without cleanup', {
    brandId, attemptId, currentStatus: state.provisioningStatus,
  });
  return { provisioningStatus: 'superseded' };
}

The label "redelivery" is wrong here — the status is 'failed' because the brand was deactivated, not because an earlier delivery of this same message already finished the job. An operator investigating why a brand's provisioning appears stuck would read "redelivery" and conclude this is a harmless SQS duplicate, when the real cause is a deliberate deactivation-cancel. The two cases need different log paths, or at minimum the log should include currentStatus in its message rather than implying a structural reason:

log?.info?.('provision-workspace-job: this attempt already reached a terminal state; standing down without cleanup', {
  brandId, attemptId, currentStatus: state.provisioningStatus,
  // 'failed' here means the brand was deactivated/cancelled — not necessarily a prior delivery
});

---

🟡 promoteProvisioningFailed CAS return value ignored in the terminal-failure branch

if (isWorkspaceTerminalFailure(status)) {
  await promoteProvisioningFailed({
    brandId, attemptId, error: TERMINAL_FAILURE_MESSAGE, postgrestClient,
  });
  log?.error?.('provision-workspace-job: sub-workspace settled to a terminal failure status', {
    brandId, attemptId, semrushWorkspaceId: candidate.workspaceId, status,
  });
  return { provisioningStatus: 'failed' };
}

If promoteProvisioningFailed returns false (CAS rejected because a newer attempt superseded this one), we still log an ERROR and return 'failed'. The behavior is safe — no cleanup is needed for a terminally-failed shell, and the newer attempt owns the brand — but the log's error-level emission and 'failed' return are misleading: we genuinely observed a terminal upstream failure, but the write didn't land. Checking the return and distinguishing this from a supersession would tighten both the logs and the return contract:

const recorded = await promoteProvisioningFailed({ ... });
if (!recorded) {
  log?.info?.('provision-workspace-job: terminal status observed but attempt already superseded', { ... });
  return { provisioningStatus: 'superseded' };
}
log?.error?.('provision-workspace-job: sub-workspace settled to a terminal failure status', { ... });
return { provisioningStatus: 'failed' };

---

🟡 504 recovery condition in createOrAdoptSubworkspaceCandidate — verify it matches ensureSubworkspace

} catch (e) {
  if (!(e instanceof ErrorWithStatusCode) && e?.status === 504) {
    created = await adoptFromFamily(transport, parentWorkspaceId, title, log, claim);
  } else {
    throw e;
  }
}

The condition enters the adoptFromFamily recovery branch only for errors that are not ErrorWithStatusCode instances. If transport.createSubworkspace throws an ErrorWithStatusCode(504) on a gateway timeout, this branch is dead code — the error is rethrown and 504 recovery silently breaks. Since this logic is extracted from ensureSubworkspace, the condition should match exactly what ensureSubworkspace uses. Please confirm the two are identical, especially that the transport actually emits a non-ErrorWithStatusCode error for this status (otherwise any test that mocks the transport won't catch the divergence either).

---

🟡 "PR-C" guard on ensureSubworkspace race has no enforcement mechanism

The comment added to ensureSubworkspace's JSDoc is correct and helpful:

Tracked as a required guard on this function's synchronous callers before general rollout (PR-C) — this comment documents the gap, it is not the fix.

But nothing in the codebase prevents the producer PR from merging before PR-C. Once the producer lands, two concurrent callers — a sync ensureSubworkspace and an async worker hop — can race on the same brand during the 0–155s provisioning window and create a duplicate workspace. Given that "the async worker widens the race window from a single in-request duration to that much longer span" (your own note), this is worth a test (FIXME or a failing integration test gated on a flag) or a CI comment rule in the producer PR to make the ordering explicit rather than relying on process memory.

---

What's good ✅

• CAS design is rigorous end-to-end. Every write uses attempt_id + provisioningStatus = 'pending' scoped compare-and-set; promoteProvisioningReady additionally gates on status IN ('pending', 'active') to prevent resurrecting deleted/ignored brands. The persistProvisioningCandidate null-guard (IS NULL) closing the double-delivery duplicate-create race is particularly careful.
• Ownership threading is correct. freshlyCreated threaded through self-requeue metadata (never re-derived from DB) is the right approach — the DB row may belong to a newer attempt by the time a superseded hop reads it. The outer-catch's requeueEnqueued + candidateAlreadyCleanedUp flags close all the cleanup edge cases without double-deletion.
• failBestEffort in the outer catch prevents permanently-pending brands. The "strand at pending forever" failure mode it prevents was the core silent-failure that motivated the redesign.
• "Inert on merge" construction. Registering the handler without a producer is the right deployment strategy for this complexity.
• Token redaction from error logs. Deleting promiseToken/promisePair from the safe-metadata before logging is the right call; this fixes a real leakage path.
• isWorkspaceReady/isWorkspaceTerminalFailure passed through un-mocked in tests. The comment in the test file explains exactly why — the hand-rolled stubs would have hidden the space-separated 'creation failed' variant. Good discipline.
• Test coverage hits all the adversarial cases. CAS-loss, UNIQUE-conflict-409, post-requeue freshness-write failure (Finding N9), transport-construction failure (Finding N3), double-cleanup guard — all exercised.

[luis6156]

Review — feat(serenity): async Semrush sub-workspace provisioning worker

The worker design is sound. The CAS-everywhere discipline is rigorous — attempt-id-scoped guards on every write, the double-delivery mutex (.is('semrush_provisioning_candidate_workspace_id', null)) on persistProvisioningCandidate, and the terminal-state split in the final commit (separating 'ready' redeliveries from 'failed' stands-down, each needing opposite cleanup semantics) are all correct. The freshlyCreated threading through self-requeue metadata rather than re-deriving it from DB is the right call given the timing issues.

Three observations for the record:

1. Leaked workspace on MAX_PROVISION_REQUEUE_DEPTH exhaustion (now fixed, but worth documenting)

emptyWorkspaceBestEffort clears projects but does not delete the Semrush workspace shell itself — that's intentional, documented in workspace-lifecycle.js (failed shells can't be deleted and must remain as diagnostic records). But a not-ready workspace exhausting its 5 hops is a distinct case: the shell is likely stuck at "not ready" (not in a terminal failure state), so it may eventually settle on its own after the attempt is failed. The reconciliation pass (future work) will need to distinguish "a failed-attempt's still-pending shell" from "a terminal shell" — the distinction the Python reference already captures (creation_failed = safe to re-create, failed/error = not). No action needed now, just worth capturing as scope for the reaper phase.

2. failBestEffort swallow and stuck-pending brands

If promoteProvisioningFailed itself throws in the outer catch (e.g. a Postgres blip after a real Lambda error), the brand row stays at semrush_provisioning_status: 'pending' with no living job to revisit it. The runner marks the AsyncJob FAILED and won't redeliver; nothing automatically reconciles the brand back. This is an acknowledged gap that the reconciliation pass (referenced in the spec) will need to cover — I'm flagging it here so it's on the radar for the reaper phase, not because it's a blocker for this PR.

3. persistProvisioningCandidate's null guard on the adopted path

The .is(..., null) guard correctly handles the at-least-once first-hop race. Confirmed: on an ADOPTION, freshlyCreated === false, so the losing delivery's cleanupIfOwned correctly skips teardown of the adopted workspace (which belongs to another brand). ✓

The final commit's deactivate-cancels-attempt fix is the right approach — reusing the existing supersede mechanism rather than teaching promoteProvisioningReady brand-lifecycle semantics it doesn't otherwise need.

Approving.