feat(serenity): async Semrush sub-workspace provisioning worker (LLMO-7352/LLMO-7418) - #3233
Open
IrisAlexandrescu wants to merge 9 commits into
Open
Conversation
|
This PR will trigger a minor release when merged. |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
IrisAlexandrescu
temporarily deployed
to
dev-branches
September 8, 2026 17:31 — with 
GitHub Actions
Inactive
GitHub Actions
Inactive
This was referenced Sep 9, 2026
IrisAlexandrescu
force-pushed
the
fix/LLMO-7352-workspace-terminal-failure
branch
from
September 9, 2026 15:45
f3d28a1 to
f40444a
Compare
IrisAlexandrescu
force-pushed
the
feat/LLMO-7418-provisioning-worker
branch
from
September 9, 2026 16:00
70fe12d to
ae4b0a9
Compare
IrisAlexandrescu
temporarily deployed
to
dev-branches
September 9, 2026 18:04 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
IrisAlexandrescu
temporarily deployed
to
dev-branches
September 9, 2026 18:54 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
IrisAlexandrescu
force-pushed
the
fix/LLMO-7352-workspace-terminal-failure
branch
from
September 10, 2026 07:33
f40444a to
db0574f
Compare
…-7352/LLMO-7418) Adds an SQS-triggered worker that provisions a brand's Semrush sub-workspace out of the request path: one invocation does at most one create-or-adopt call and one status poll, then self-requeues with bounded exponential backoff (DelaySeconds, capped at 5 hops) instead of looping or sleeping in-Lambda. Every write to the brand row is an attempt-id-scoped compare-and-set, so a stale/superseded attempt (a retry, or a late at-least-once SQS redelivery) can never clobber a newer one, and cleanup provenance (freshlyCreated) is threaded through the self-requeue job metadata rather than re-derived per invocation, so a superseded attempt's own real workspace is reliably cleaned up regardless of which hop discovers the supersession. This is the replacement for the synchronous create-then-poll flow that could silently, permanently bind a brand to a workspace that never finished creating. Introduced by: N/A
…anchor, clean up orphans on any promotion failure (LLMO-7418 external-review Finding 5) promoteProvisioningReady wrote status: 'active' unconditionally on ready promotion, with no site_id check — bypassing the exact anchor invariant upsertBrand already enforces at create time (a siteless brand is forced to 'pending', never 'active'). The async bare-create path never requires baseSiteId, so a caller that omits it (the common case per prod data) hit the live chk_active_brand_has_site_id CHECK the moment the worker tried to promote — a 23514 not handled by the existing UNIQUE-conflict branch, so it fell to the generic catch, which recorded the failure but never cleaned up the Semrush workspace this same call had just confirmed ready, leaking it permanently on every retry. - getBrandProvisioningState now also reads site_id; the worker reuses this same state read (no extra query) to pass hasSiteAnchor into promoteProvisioningReady, which omits `status` from its update entirely when false rather than attempting (and violating) the CHECK. - Hoisted the handler's `transport` construction above the try (it was declared twice, once inline in an early-return branch) so cleanup is reachable from every failure path that needs it. - The ready-promotion's own try/catch now cleans up a freshly-created candidate for ANY promotion failure, not just the already-handled UNIQUE conflict — precisely scoped to this catch (not the outer generic one), since the self-requeue path deliberately carries its candidate forward and must never have it cleaned up mid-chain. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…LLMO-7418 external-review Finding 3) promoteProvisioningReady's CAS predicate (attempt_id + semrush_provisioning_status = 'pending') never checks the brand's own lifecycle status column, so a legitimate concurrent /serenity/deactivate was invisible to it: the attempt's later ready-promotion would still match and silently flip status back to 'active', resurrecting a brand the caller had just deliberately deactivated. Rather than teach promoteProvisioningReady brand-lifecycle semantics it otherwise has no reason to know, deactivate now cancels whatever attempt is currently pending as part of its own (best-effort) write, after its primary decommission work has already succeeded. This reuses the EXISTING, already-tested supersede mechanism: provisionWorkspaceHandler's own currency re-check at the top of every hop already stands down cleanly the moment semrush_provisioning_status is no longer 'pending' — cancelProvisioningAttempt just needs to make that true. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… the write (LLMO-7418 external-review Finding 7, corrected)
The external review's original framing ("the column is written but never
read, remove it") was incomplete: persistProvisioningCandidate's CAS
`.is('semrush_provisioning_candidate_workspace_id', null)` check is a real
double-delivery mutex — two concurrent SQS deliveries of the same first hop
start with identical, empty job metadata, so only a DB-level compare-and-set
can tell them apart and let exactly one proceed. Dropping the column
entirely, as originally proposed, would have silently reintroduced that
race. Confirmed and corrected before touching anything.
The genuinely dead part is narrower: getBrandProvisioningState selected the
column back and surfaced it as `provisioningCandidateWorkspaceId`, which no
caller anywhere ever consulted (the worker's actual candidate resolution is
entirely metadata-based, threaded hop-to-hop through the self-requeue
payload). Removed that read-back only; the write, and its CAS guard, are
unchanged. Cleaned up the now-misleading test fixtures that set this field
on a mocked state as if it mattered.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…MO-7418 external-review Finding 4) Correction to the remediation plan's own earlier mislabeling: this was never an OpenAPI documentation issue — the review's actual finding is that no async enqueue site in controllers/serenity.js passed `title` in the provision-workspace-job metadata, while provisionWorkspaceHandler forwards it straight into createOrAdoptSubworkspaceCandidate with no validation. Only brands.js's two createBrandForOrg sites passed it correctly. Traced each of the 4 serenity.js call sites individually rather than fixing all of them uniformly: - createMarket's and activate's project-activation (batch-market) async branches are provably safe as-is: both are documented, code-referenced invariants (`auth.mode === 'subworkspace'` / an already-ACTIVE brand) that the brand already has a canonical workspace pointer, so the worker's existing-pointer fast path is always taken and `title` is never read. Adding it there would be unjustified defensive code for a case that cannot happen. - activate's pending->active branch operates on a brand that is GUARANTEED pointer-less (a pending brand never has one), so the worker ALWAYS takes the create-or-adopt path — this was a real, always-triggered bug that called Semrush with an untitled sub-workspace on every async activation of a pending brand. Fixed: passes `title: brand.getName()`. - activate's bare-reactivation branch mirrors its own synchronous twin, which defensively calls the general-purpose `ensureSubworkspace` (create-or-existing) rather than assuming a pointer always exists — the async path needed the same defensiveness. Fixed identically. Also adds the worker's own last line of defense (this repo, provision- workspace-job.js): fail loudly with a clear error if `metadata.title` is ever missing when about to create, rather than silently asking Semrush to create an unrecoverable, permanently-unadoptable untitled sub-workspace. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… up its candidate (LLMO-7418 external-review Blocker 2) The currency check conflated two distinct conditions under one branch: "a DIFFERENT attempt now owns the brand" (genuine supersession — our own candidate, if any, is a stale leftover safe to clean up) and "OUR OWN attempt already reached a terminal status" (ready or failed — NOT a supersession, just an at-least-once SQS redelivery of a message whose earlier delivery already finished this exact job). Proven data-loss path: once a hop's candidate is promoted to ready (semrush_provisioning_status flips to 'ready', attempt_id unchanged) but before this Lambda invocation's own job.save() marks the AsyncJob COMPLETED, a redelivery of the SAME message can re-enter this handler. Under the old check, `state.provisioningStatus !== 'pending'` alone was enough to trigger cleanup — and by then `candidate` (carried forward via job metadata from a self-requeue hop) is the brand's now-CANONICAL, LIVE workspace, which may already hold a real market project the chained job created. cleanupIfOwned would empty it, deleting live customer data on a concurrent/duplicate delivery. Fix: only clean up when the attempt id itself differs. When the attempt id matches but the status is no longer pending, stand down as a true no-op — never touch the candidate. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ifier regressions in tests (LLMO-7418 external-review Finding 16 + Medium) Finding 16: orphan cleanup was narrower than the code's own comments claimed. Two gaps closed: - The requeue-exhausted branch (MAX_PROVISION_REQUEUE_DEPTH reached) recorded the attempt failed but never cleaned up a freshly-created candidate — normally a no-op (a not-ready shell has no projects yet) but closes the rarer case where one exists despite the not-ready status, and nothing else will ever revisit this attempt once it's failed. - The generic outer catch still had no cleanup call at all. A blanket fix would have been unsafe: once a self-requeue's own createAndEnqueueJob call succeeds, a FUTURE hop already owns the candidate and will poll it — cleaning it up in the outer catch (e.g. because the subsequent freshness-optimization write throws) would corrupt the workspace the next invocation is about to use. Added a `requeueEnqueued` flag, set only once that enqueue call itself succeeds, to gate the outer catch's cleanup precisely. Also tracks whether promoteProvisioningReady's own inner catch already cleaned up before rethrowing, to avoid a harmless but noisy double cleanupIfOwned call. Medium (test stub divergence): provision-workspace-job.test.js hand-rolled stubs for isWorkspaceReady/isWorkspaceTerminalFailure that omitted the space-separated 'creation failed' variant (the one actually observed in production) and dropped all case/whitespace normalization — pure functions with no reason to be re-implemented in a test double, and a real regression in either the classifiers or this handler's own use of them could hide behind the stub indefinitely. Removed the override; tests now exercise the real implementations (esmock passes them through un-mocked). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
IrisAlexandrescu
force-pushed
the
feat/LLMO-7418-provisioning-worker
branch
from
September 10, 2026 07:42
fce9e83 to
09c741f
Compare
IrisAlexandrescu
temporarily deployed
to
dev-branches
September 10, 2026 07:50 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
…O-7418 external-review N9, N3, Finding 3)
- N9: a transient failure of the best-effort job-id refresh AFTER a successful
self-requeue no longer reaches the outer catch. Previously it marked the still-live
attempt `failed` and re-threw, invalidating the promise token the requeued hop needs
-- killing a healthy attempt and orphaning its workspace. The write is wrapped locally,
matching its own "optimization, not safety mechanism" contract.
- N3: createSerenityTransport is now constructed INSIDE the try. normalizeBaseUrl throws
a 503 on a missing/malformed SEMRUSH_PROJECTS_BASE_URL; building it before the try let
that escape failBestEffort and strand the brand at `pending` forever. The catch guards
its only transport use on `transport &&` since construction can now fail.
- Finding 3 (delete/deactivate resurrection): promoteProvisioningReady's CAS now gates on
`status IN ('pending','active')`, so a soft-deleted ('deleted') or offboarded ('ignored')
brand -- neither of which touches the provisioning columns -- can no longer be flipped
back to `active` with a fresh workspace by a late worker hop.
Tests: added coverage for all three (post-requeue freshness-write swallow, transport
construction failure recorded-not-stranded, CAS status predicate + deleted-brand lost race).
Suites: provision-workspace-job + brands-storage = 293 passing. eslint + type-check clean.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
IrisAlexandrescu
temporarily deployed
to
dev-branches
September 10, 2026 10:35 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
…418 external-review Finding 7) The docstring claimed the worker uses this read "to decide whether to resume an already-persisted candidate (skip create-or-adopt) or run it for the first time" -- contradicting the PROVISIONING_SELECT comment three lines above it, which explains that the candidate column is deliberately NOT selected and that candidate resolution is entirely metadata-based (threaded hop-to-hop through the self-requeue payload). Corrected to state what the read is actually for (attempt-id currency + still-pending, the existing-pointer fast path, and the ready-promotion's site anchor) and to record explicitly that it does NOT resolve the in-flight candidate. Comment-only; no behavior change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
IrisAlexandrescu
temporarily deployed
to
dev-branches
September 10, 2026 11:26 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds an async, SQS-triggered worker (
src/support/serenity/handlers/provision-workspace-job.js) that provisions a brand's Semrush sub-workspace out of the request path — the replacement for the synchronous create-then-poll flow that caused LLMO-7352: a brand could get silently, permanently bound to a workspace that never finished creating, breaking "Add Market" for that brand with no operator visibility until a customer complained.DelaySecondswith bounded exponential backoff (5s/10s/20s/40s/80s, capped at 5 hops) instead of looping or sleeping in-Lambda.getBrandProvisioningState,persistProvisioningCandidate,updateProvisioningJobId,promoteProvisioningReady,promoteProvisioningFailed— new inbrands-storage.js) is an attempt-id-scoped compare-and-set, so a stale/superseded attempt (a retry, or a late at-least-once SQS redelivery) can never clobber a newer winner's write.createOrAdoptSubworkspaceCandidateis extracted from the existingensureSubworkspaceinworkspace-lifecycle.js(byte-identical,ensureSubworkspaceitself is untouched) so the worker reuses the exact same, already-tested claim-filter/504-recovery logic without a second, drifting copy.freshlyCreated) is threaded through the self-requeue job metadata rather than re-derived per invocation, so a superseded attempt's own real, freshly-created workspace is reliably cleaned up regardless of which hop discovers the supersession — an earlier version of this PR lost that provenance across hops; caught in adversarial review (see below).semrush_provisioning_status: 'failed'(sanitized message, never echoing raw upstream error text) before re-throwing, so a brand can no longer be stranded atpendingforever with no job ever revisiting it.Scope note: this PR ships the worker with no producer wired to it yet — no HTTP call site starts an attempt or enqueues
serenity-provision-workspacetoday. That's PR-C (endpoint conversions to202+ guards on the still-synchronous call sites).Known, deliberately deferred gap: the synchronous
ensureSubworkspace(Add Market, project/market activation) has no awareness ofsemrush_provisioning_status, so a concurrent sync call while an async attempt is mid-flight can independently create a second workspace for the same brand — a pre-existing race this PR widens from a single in-request duration to up to ~2.5 minutes. Documented explicitly inensureSubworkspace's JSDoc; the fix (pending-status guards on those call sites) is PR-C.Review
This PR went through a 4-agent review (adversarial, architect, senior-engineer, observability) against the real diff before opening. The adversarial pass found 3 real blockers, all fixed and covered by new tests before this PR was opened:
persistProvisioningCandidatethat could leak a workspace under two concurrent (at-least-once) SQS deliveries of the same first hop — fixed with an.is(..., null)guard.freshlyCreatedcleanup provenance across self-requeue hops — fixed by threading it through job metadata instead of re-reading the DB.pendingforever — fixed with an outer catch that best-effort records failure before re-throwing.The observability pass independently flagged the same uncaught-exception gap from the logging side (the outer job-runner's catch only logged the job ID, not brand/attempt context) — also fixed.
Test plan
npx mocha --require test/setup-env.js "test/support/serenity/**/*.test.js" "test/support/brands-storage.test.js" "test/serenity-prompt-classification/index.test.js"— 1805 passing.npm test— 18,135 passing, 0 failing (the coverage-threshold gate fails on pre-existing, unrelated 0%-covered files not touched by this PR).npm run type-check— clean.npx eslinton all touched files — clean.