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 (2 pre-existing unrelated errors on main, in files this PR does not touch)
• npm test — 14,613 passing (was 14,609 on unmodified main; net +4 new passing tests), same 7 pre-existing failures on unmodified main (missing optional packages / unrelated test drift, confirmed via git stash comparison) — not introduced by this PR
• Live-container IT suite (test/it/postgres/serenity.test.js) — could not run in this sandbox: the whole app fails to load at bootstrap due to a pre-existing missing optional package (@adobe/spacecat-shared-ticket-client), reproducible on unmodified main; additionally ECR login failed here (no AWS credentials configured in this environment). Needs a run in an environment with a full npm ci and AWS/ECR access before merge.
• Dry-run the rightsizing sweep against a dev org with a real IMS token before trusting it at scale (no live Semrush credentials available to me)

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 👎.}