feat(serenity): rollout hardening — rightsizing sweep, observability, cross-container lock ADR (LLMO-6191)

[byteclimber]

Summary

Operational hardening for the dynamic (JIT) Semrush AI resource allocator shipped dormant in #2764 (SERENITY_DYNAMIC_ALLOCATION, default OFF). Implements LLMO-6191, right-sized per the ticket's own scope guidance — see below for what's real code vs. deferred to a design doc.

Note on the ticket's "safety-valve timeout" line item: verified LOCK_TIMEOUT_MS already exists in src/support/serenity/resource-lock.js (shipped in #2764) — struck from scope, no work needed here.

1. Rightsizing sweep — real code

scripts/serenity-rightsizing-sweep.mjs: one-time backfill lowering already-carved sub-workspaces to actual usage via the existing releaseAiSurplus reclaim primitive. Dry-run mode, --org-ids/--brand-ids allow-lists, rate limiting, a reason-aware consecutive-error abort threshold (expected pool/limit/busy outcomes don't trip it — only genuinely unclassified errors do), and a resumable checkpoint file (a fleet-wide sweep can take hours; IMS tokens expire mid-run).

Requires an operator-supplied SEMRUSH_IMS_TOKEN — there is no service-account path to Semrush in this repo (every call forwards a live human's IMS bearer token verbatim). This needs a dry-run against a dev org before being trusted at scale — flagging per the task's own caveat, since I have no live Semrush/IMS credentials to validate it myself.

releaseAiSurplus gained a dryRun option (single source of truth for the preview math — the sweep reuses it instead of reimplementing the round-up/floor/to-zero logic) and now surfaces a typed errorCode on its swallowed-error outcome, so a batch caller can tell an expected pool/limit/busy failure apart from an unexpected one.

2. Observability and SLIs — real code

src/support/serenity/allocation-metrics.js wraps the existing CloudWatch EMF emitter (metrics-emf.js, already used elsewhere in this repo — no new metrics pipeline invented) with allocator-specific metrics: hot-path ratio, top-up latency, advisory pool-free ratio, typed-rejection counters, not-ready retry rate, release-outcome counters, and a 405-classifier match-ratio metric wired into isMeteredQuota (which has no production call site yet in this codebase — documented candidly as a known gap, not fabricated coverage).

3. Cross-container serialization — deferred to a design doc

docs/decisions/007-cross-container-resource-lock.md. Per the ticket's explicit guidance, this PR does not unilaterally provision new infrastructure (no DynamoDB table, no Redis). This repo has zero existing DynamoDB usage and no distributed-lock primitive was found in spacecat-shared. The ADR lays out 5 options (DynamoDB conditional write, an upstream Semrush CAS transfer, accept + observe, SQS FIFO MessageGroupId, single-flight coalescing) and recommends accept + observe for the initial ON rollout, contingent on this PR's item-2 telemetry actually landing and being watched.

One real code fix shipped here as part of closing the same-container half of the gap: releaseAiSurplus's one production call site (markets-subworkspace.js, the model-update seam) is now wrapped in withResourceLock alongside ensureAiHeadroom — a same-container ensure/release race that review surfaced as a gap in the original PR (withResourceLock's doc only covered ensure).

4. Alerting + runbook — real doc

docs/runbooks/serenity-zombie-workspace-recovery.md (matches the existing runbook format), covering diagnosis/recovery of a sub-workspace stuck in a partially-applied-transfer state, plus the pager-worthy vs. dashboard-only metric split. No alerting-as-code file exists in this repo to wire an actual alarm into, so paging config itself remains a manual step (documented, not invented) — per the ticket's scope guidance not to build a new alerting pipeline.

Review process

Plan reviewed by senior-staff-engineer (architecture/lock options) and senior-sre (sweep safety, metric design) before implementation. Findings folded in: reason-aware abort threshold, dryRun on the primitive (not reimplemented in the script), corrected failure-mode framing in the ADR (the absolute-set race is bounded, not unbounded — see the ADR's analysis), added the SQS-FIFO and coalescing options the staff-engineer review flagged as missing, and wrapped releaseAiSurplus in the lock.

Test plan

• npm run type-check — clean
• npm run lint — clean on all touched files
• npm test — full suite passing, no regressions
• Live-gateway dry-run + real run of serenity-rightsizing-sweep.mjs against a real throwaway sandbox sub-workspace seeded via a local Postgres+PostgREST IT stack (db+data-service, real migrations) — see PR comment below for the full writeup. Dry-run made zero mutating calls and computed the correct release target; the real run resized the workspace to exactly the floor and the surplus was confirmed to land back on the master pool by the exact predicted amount. Also fixed a real createDataAccess wiring bug the script had (camelCase config keys), pushed in 0959efde4.
• Live dry-run against real prod org/brand data specifically — blocked on the dev POSTGREST_API_KEY (dx_mysticat/dev/api-service) failing PostgREST's JWT validation ("Unsupported token type") — a separate credential issue, tracked but not resolved here.

LLMO-6191

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[MysticatBot]

Hey @byteclimber,

Verdict: Request changes - three observability-correctness issues that would produce misleading metrics from day one.
Complexity: HIGH - large diff; architectural decision record + API surface changes.
Changes: Adds rollout hardening for the dynamic Semrush AI allocator - a rightsizing sweep script, CloudWatch EMF observability, a cross-container lock ADR, and a zombie-workspace runbook (11 files).
Note: Recommend a human read before merge - this change amends an architectural invariant documented in docs/decisions/007-cross-container-resource-lock.md. The bot review is a complement to, not a replacement for, a human read here.

Must fix before merge

1. [Important] isMeteredQuota emits metric on early-return path for every non-405 error, making the classifier ratio meaningless - src/support/serenity/errors.js:90 (details inline)
2. [Important] Metric emission as side-effect of error-factory construction couples telemetry to object creation - src/support/serenity/resource-manager.js:169 (details inline)
3. [Important] TopUpLatencyMs has incomparable semantics between transferAndSettle (full retry loop) and transferOnce (single attempt) with no distinguishing dimension - src/support/serenity/resource-manager.js:208 (details inline)

Non-blocking (3): minor issues and suggestions

• suggestion: Checkpoint write is not atomic - a crash mid-write loses the entire progress set. Use write-to-tmp + renameSync for crash-safe checkpointing - scripts/serenity-rightsizing-sweep.mjs:169
• suggestion: --limit, --rate-limit-ms, --max-consecutive-errors silently produce NaN on non-numeric input (e.g. --limit foo), causing a silent no-op. Add a Number.isNaN(...) guard with an error message - scripts/serenity-rightsizing-sweep.mjs:119
• nit: dryRun path in releaseAiSurplus does not emit recordReleaseOutcome('dry-run') - the sweep's dry-run mode is invisible to CloudWatch, so an operator cannot confirm the metric pipeline is functional end-to-end - src/support/serenity/resource-manager.js:453

[MysticatBot]

issue (blocking): recordMeteredQuotaClassifier(false) fires on the early-return path for every error that is not a SerenityTransportError with status 405. When a caller is eventually wired up in a generic catch path, this will emit Matched=false for every TypeError, network timeout, auth failure, etc. - drowning the actual 405-classifier signal.

The metric's stated purpose is "of all 405s, how many are disguised quota rejections." That ratio requires emitting only when status === 405.

Fix: Remove the metric call from the early-return path. Emit only inside the 405 branch, so the metric fires only for actual 405 errors.

[MysticatBot]

issue (blocking): orgPoolExhausted(), brandAiLimit(), and workspaceBusy() emit recordRejection(...) at construction time. These are plain factory functions returning Error objects. If any code path ever constructs one without throwing (for comparison, logging, or testing), a phantom rejection metric fires.

This breaks the implicit purity contract: the peer predicates in errors.js (isPoolExhausted, isWorkspaceNotReady) are side-effect-free, so developers will reasonably expect the factories to be pure too.

Fix: Move recordRejection(...) to the throw/catch boundary where the rejection actually surfaces to the caller - e.g. inline at the throw site in transferAndSettle/transferOnce, or in the catch-path of ensureAiHeadroom where the typed e.code is already available.

[MysticatBot]

issue (blocking): transferAndSettle records Date.now() - startedAt in the finally block, which includes all retry sleeps (up to NOT_READY_RETRIES * intervalMs * attempt - potentially tens of seconds of idle waiting). transferOnce (line 260) records only a single transport call. Both emit the same TopUpLatencyMs metric with no distinguishing dimension.

This creates a bimodal distribution: p99 spikes could be either a slow gateway or a workspace in a retry loop, making percentile-based alerting unreliable.

Fix: Add a Path dimension (settle vs fail-fast) to recordTopUpLatency(ms, path), so operators can alert on each path independently. Low cost to add now, avoids a redeploy later when the alarm author discovers the bimodal shape.

[github-actions]

This PR will trigger a minor release when merged.

[MysticatBot]

Hey @byteclimber,

Verdict: Approve - all prior findings addressed correctly; no new blocking issues.
Complexity: HIGH - large diff; architectural decision record + API surface changes.
Changes: Adds rollout hardening for the dynamic Semrush AI allocator - a rightsizing sweep script, CloudWatch EMF observability, a cross-container lock ADR, and a zombie-workspace runbook (12 files).
Note: Recommend a human read before merge - this change introduces an architectural decision record (docs/decisions/007-cross-container-resource-lock.md). The bot review is a complement to, not a replacement for, a human read here.

Non-blocking (5): minor issues and suggestions

• suggestion: PoolFreeRatio only fires on the topped-up path (the master-resource read only happens there), creating a draw-time-only sample. Consider noting this in the metric catalog so operators know it measures "pool state at draw-time" not "continuous pool health" before relying on it as the Option C escalation trigger - src/support/serenity/allocation-metrics.js:64
• nit: recordMeteredQuotaClassifier(matched) passes a boolean as the Matched dimension value while every other dimension in the module uses strings. CloudWatch stringifies booleans anyway, but explicit 'true'/'false' would be consistent - src/support/serenity/allocation-metrics.js:119
• suggestion: ADR Option D (SQS FIFO) does not mention the 300 TPS per MessageGroupId limit, which would become the bottleneck if a single child workspace ever sees high concurrency - docs/decisions/007-cross-container-resource-lock.md:100
• nit: The .tmp checkpoint path (${checkpointFile}.tmp) is predictable. In a shared /tmp directory a co-tenant could race the rename via symlink. Using O_CREAT | O_EXCL on the temp path would harden against this - scripts/serenity-rightsizing-sweep.mjs:521
• suggestion: createSerenityTransport receives the full process.env object. Consider passing only the required subset ({ SEMRUSH_IMS_TOKEN }) to minimize the reachable secret surface - scripts/serenity-rightsizing-sweep.mjs:495

Previously flagged, now resolved

• isMeteredQuota metric emission scoped to actual 405s only (no longer fires on every non-405 error)
• recordRejection moved from error factories to throw sites (factories are now side-effect-free)
• TopUpLatencyMs now carries a Path dimension (settle vs fail-fast) distinguishing the two semantics
• Checkpoint write uses atomic write-to-tmp + renameSync
• Numeric CLI options validated with parseNumericOption (exits on NaN instead of silent no-op)
• dryRun path now emits recordReleaseOutcome('dry-run') for dashboard visibility

---

_{Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 1m 50s | Cost: $4.74 | Commit: 64855dfa6a3c4ccc22c1d1cba2981618850812c6
If this code review was useful, please react with 👍. Otherwise, react with 👎.}

[byteclimber]

Live e2e verification (local Postgres/PostgREST + adobe-hackathon.semrush.com sandbox)

Ran scripts/serenity-rightsizing-sweep.mjs end-to-end against real infrastructure — not just a syntax/startup check:

Setup: brought up the local IT stack (docker compose -f test/it/postgres/docker-compose.yml up db data-service, real migrations via dbmate), seeded one throwaway org + brand row with semrush_sub_workspace_id pointing at a real sandbox sub-workspace carved with a legacy-style flat over-allocation ({projects:5, prompts:800}, 0 actual usage) — i.e. exactly the pre-JIT brand shape this sweep targets.

• --dry-run: correctly enumerated the seeded org/brand, correctly computed would release → {projects:1, prompts:100} (the floor), and made zero mutating calls — confirmed by re-reading the workspace afterward: still {5, 800}, untouched.
• Real run: resized the workspace to exactly {projects:1, prompts:100} (the floor). Confirmed the surplus genuinely credited back to the master pool, not just a status-code success — master's totals moved by the exact predicted amount across the carve/release sequence (100→95 on carve, 95→99 on release-back for prompts; consistent for projects too).

Along the way, fixed a real bug the previous script had (createDataAccess({POSTGREST_URL: ...}) → needs camelCase postgrestUrl/postgrestSchema/postgrestApiKey), pushed in 0959efde4.

Dev-Vault-backed live dry-run against real prod data is still blocked separately (POSTGREST_API_KEY from dx_mysticat/dev/api-service fails PostgREST JWT validation with "Unsupported token type") — tracked as a follow-up, not a blocker for this PR's own correctness.

[rainer-friederich]

@byteclimber I think we documented in the spec that we will not do the right-size sweep if I remember correctly.

I also don't understand why the test plan can't be executed.

[byteclimber]

Two different things got conflated with the same name — worth separating out explicitly:

1. The rejected item (serenity-docs#22 §4): "Children carved under the flat model keep their pre-carve allocation, and no backfill job corrects them. A rightsizing sweep was evaluated and rejected as not worth a background job." That's about automating this as a recurring/scheduled job — rejected.

2. What this PR actually built (scripts/serenity-rightsizing-sweep.mjs): a manually-invoked, operator-run one-shot script — never scheduled, never triggered automatically, requires a human to supply SEMRUSH_IMS_TOKEN and run it by hand. That's not the rejected item — it's the thing the same design doc section explicitly calls for immediately after the rejection: "If the pool becomes tight, the cheapest fix is a one-shot operator script issuing a lowering transfer to a non-zero floor... Sizing that need is what the stranded-units audit in §7 is for." This PR is that one-shot operator script.

(There's a separate, correctly-rejected item in the code itself — a comment in workspace-lifecycle.js on ensureSubworkspace, dated 2026-07-08 from you — that's about NOT auto-migrating flat-carved children at re-activation time, which is a different question again and stayed correctly out of scope here.)

On the test plan: I went back and actually ran it live this week rather than leaving it as an open checkbox —

• Got AWS/ECR access (klam login → spacecat-dev profile), pulled the mysticat-data-service IT image, brought up the local Postgres+PostgREST stack (db + data-service from test/it/postgres/docker-compose.yml, migrations via dbmate).
• Seeded one throwaway org/brand pointing at a real sandbox sub-workspace carved with a legacy-style flat over-allocation ({projects:5, prompts:800}, 0 actual usage — the exact shape this sweep targets).
• --dry-run: correctly enumerated it, computed would release → {projects:1, prompts:100}, made zero mutating calls (verified by re-reading the workspace unchanged afterward).
• Real run: resized the workspace to exactly that floor, and confirmed the surplus genuinely credited back to the master pool by the exact predicted amount across the carve/release sequence — not just a status-code success.
• Also found and fixed a real bug the script had: createDataAccess({POSTGREST_URL: ...}) needed camelCase postgrestUrl/postgrestSchema/postgrestApiKey — pushed in 0959efde4.

Full writeup in the earlier comment on this PR. The one thing still genuinely open is a live dry-run against real prod org/brand data specifically — that's blocked on the dev POSTGREST_API_KEY from dx_mysticat/dev/api-service failing PostgREST's JWT validation ("Unsupported token type"), which is a separate credential issue, not something in this PR's code.

[rainer-friederich]

@byteclimber I just think we don't need that. All existing workspaces already have a good amount of projects and prompts.

[byteclimber]

On "we don't need that" — I don't think this is something I should resolve unilaterally by re-explaining scope again, so a direct question instead: the design doc's own §7 says the stranded-units audit that would tell us whether the pool actually needs rightsizing "has not been run" — so right now neither of us has data, just priors. Your prior is "existing workspaces already have a good amount of headroom, this isn't needed." Mine (from building it) was "cheap insurance for whenever it turns out to be needed."

The script itself has zero operational cost either way — it's never scheduled, never auto-invoked, only runs if an operator manually supplies live Semrush credentials and calls it by hand. So the actual question isn't "does the code work," it's "do we want this capability to exist at all right now." Two honest options, your call:

1. Merge it anyway as a ready-but-unused tool — costs nothing until someone needs it, and if your prior is right, it just never gets run.
2. Drop it from this PR entirely and re-scope LLMO-6191 without it — if you're confident enough in "we don't need that" that you'd rather not carry the maintenance surface at all.

I'll do whichever you pick; not going to keep re-litigating this without your explicit answer.

Separately — one more §8 item from the design doc that I noticed got silently dropped rather than addressed either way: "the org-pool check is advisory, and that is deliberate... What is missing is the customer-facing story: two brands topping up at once against a nearly-empty pool means one gets a 409 orgPoolExhausted on an ordinary write. Confirm that is the intended experience before the flip." That's a product sign-off, not code, and nothing in this PR (or the thread) confirms it either way. Can you confirm whether "one brand gets a 409 while another is mid-top-up" is the intended customer experience, or does that need a different UX treatment before the flag ever goes ON?

[solaris007]

🎉 This PR is included in version 1.661.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀