fix(serenity): remove dormant Semrush AI allocator, §10.1/§10.2 (SITES-49206) - #2995
fix(serenity): remove dormant Semrush AI allocator, §10.1/§10.2 (SITES-49206)#2995aliciadriani wants to merge 10 commits into
Conversation
Semrush no longer enforces AI project/prompt limits for proxy-routed LLMO workspaces (confirmed 2026-07-28), so the just-in-time (JIT) allocation machinery only works around limits that no longer apply. This removes the dormant allocator and unpicks its wiring. Removed: - Modules: dynamic-allocation-active.js, resource-manager.js, resource-lock.js (+ their test files, and the dynamic-allocation-fronting test). - allocation-metrics.js: dropped the six allocator-only recorders; kept recordRejection + recordMeteredQuotaClassifier (load-bearing for the §10.6 quota-exhaustion handling that is deleted LAST). - Wiring: controllers/serenity.js, brand-provisioning.js, handlers/prompts-subworkspace.js, handlers/markets-subworkspace.js (headroom guards, ensure/retryOnQuota, releaseAiSurplus, modelChangeUnits). - rest-transport.js transferWorkspaceResources (allocator-only method). - errors.js: isPoolExhausted / isWorkspaceNotReady predicates and the ORG_POOL_EXHAUSTED / BRAND_AI_LIMIT / WORKSPACE_BUSY codes. - Flags SERENITY_DYNAMIC_ALLOCATION / SERENITY_BRAND_AI_CEILING_PROJECTS / SERENITY_BRAND_AI_CEILING_PROMPTS (in-code references only). - scripts/serenity-rightsizing-sweep.mjs — pure allocator tooling that hard-imports the deleted releaseAiSurplus; could not compile without it. §10.2 (flat pre-carve) was already absent — createSubworkspace posts no resources body — so only the stale header comment was updated. Kept (behaviour byte-for-byte, since the allocator flag was OFF in every deployed environment): §10.3 publish best-effort/quota handling, §10.6 quota-exhaustion handling, and the §10.7 metered-405 canary. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
…(§10.7) Addresses PR-review doc-drift: the dormant JIT allocator was removed in this PR but the docs still described it as live/dormant. - ADR-007 (cross-container resource lock): marked SUPERSEDED — the race and its mitigation no longer exist. - ADR-008 (no-subworkspace-carve): amendment note — the no-carve decision now unconditional; the "retained behind the flag" fallback is gone. - docs/serenity.md: dropped "when to turn the JIT allocator on" + the "Dynamic AI resource allocation — operations" section (they instructed operators to enable now-deleted code); kept the still-valid no-carve rationale and pointed the premise re-check at the metered-405 canary. - Deleted docs/runbooks/serenity-zombie-workspace-recovery.md — its condition (a partially-applied allocation transfer) is now unreachable. - .gitignore: dropped the stale !scripts/serenity-rightsizing-sweep.mjs negation (the script was deleted with §10.1). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
This PR will trigger no release when merged. |
| # ADR-007: Cross-container serialization for the dynamic-allocation absolute-set race | ||
|
|
||
| > **Applies only when `SERENITY_DYNAMIC_ALLOCATION` is on**, which it is not in any deployed | ||
| > **SUPERSEDED (2026-08, SITES-49206).** The dynamic (JIT) Semrush AI allocator this ADR concerns — |
There was a problem hiding this comment.
Fix (review, doc-drift §10.7): this PR deletes resource-lock.js/resource-manager.js, so the race this ADR documents no longer exists. Marked SUPERSEDED per §10.7 (kept as a decision record, not deleted).
| @@ -1,5 +1,11 @@ | |||
| # ADR-008: A brand sub-workspace carries no resource allocation | |||
|
|
|||
| > **Update (2026-08, SITES-49206).** This decision stands and is now **unconditional**: the dormant | |||
There was a problem hiding this comment.
Fix (review, doc-drift §10.7): the no-carve decision here is now unconditional — the dormant JIT allocator it referenced as a flag-gated fallback is deleted in this PR. Amendment note added; core decision retained.
| future re-tightening upstream surfaces as a failed canary rather than a silent customer-facing | ||
| error. Run it per environment when validating the premise. | ||
|
|
||
| > **Removed (SITES-49206):** the just-in-time (JIT) top-up allocator |
There was a problem hiding this comment.
Fix (review, doc-drift §10.7): removed the "when to turn the JIT allocator on" + "Dynamic AI resource allocation — operations" sections (they instructed operators to enable now-deleted code). Kept the valid no-carve rationale; premise re-check now points at the retained metered-405 canary.
|
Self-review pass (round 1) — applied fixes Code review found the removal itself clean: no dangling references to removed symbols anywhere in
Re-review: nothing further to address. One known nit deliberately left: |
aliciadriani
changed the title
There was a problem hiding this comment.
Full manual review
Summary
Removes the dormant JIT Semrush AI allocator (§10.1) and the flat pre-carve (§10.2): ~4.5k lines of source+tests deleted, plus wiring unpicked across the controller, both metered handlers, brand-provisioning, workspace-lifecycle, rest-transport, and errors. The allocator was OFF in every deployed environment and Semrush stopped enforcing AI limits for proxy-routed LLMO workspaces (confirmed 2026-07-28), so this is a pure dead-code removal. Overall quality: high — consistent, well-scoped, green.
Must Fix (blocks merge)
None.
Should Fix
None. (The doc-drift found in review — ADR-007/008, docs/serenity.md, the zombie runbook, .gitignore — was fixed in commit 582d42e with inline comments.)
Nits (optional)
quota-alerts.jsQuotaCasetypedef still listsorgPoolExhausted/brandAiLimit— cases the now-removed allocator was the only producer of. Harmless (kept §10.6 file, never enabled) and retires with §10.6; no action here.- The future §4 settle-poll removal planned to reuse
isWorkspaceNotReady; it's removed here, so that later work re-introduces a bounded not-ready retry. Not a bug — just a forward note.
What's Good
- Verified no dangling references:
isDynamicAllocationEnabled,resolveBrandAiCeiling,ensureAiHeadroom,releaseAiSurplus,withResourceLock,createHeadroomGuard,transferWorkspaceResources,isPoolExhausted,isWorkspaceNotReady,bodyText,countPublishedPrompts— zero remaining callers insrc/test. - Correct partial trim of
allocation-metrics.js:recordRejection+recordMeteredQuotaClassifierare still imported by the deliberately-deferred §10.6 code (errors.js,markets-subworkspace.js) — deleting the whole module would have broken it. Verified. - Metered handlers:
retryOnQuota/headroom.ensurewere no-op passthroughs with the flag off (always), so the direct calls are byte-for-byte prod behaviour. ci / build(incl. tests) green,type-checkbase+strict green, 16,362 tests pass.
Verdict: approve-worthy. Needs a non-author CODEOWNER approval to clear branch protection.
…0.1) The `Serenity API — dynamic allocation ON (metered JIT via the live UM mock)` block asserted the JIT allocator's behaviour — a live /resources top-up transfer and a brandAiLimit 409 — which this PR removed. With the allocator gone the writes correctly return 200/201, so the block failed `expected 200 to equal 409` in ci/it-postgres. Deleted the block and its now-unused SERENITY_MOCK_WORKSPACE_ID / SERENITY_ORG_PARENT_WS_ID imports. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
GitHub Actions
Inactive
# Conflicts: # src/controllers/serenity.js # src/support/serenity/resource-manager.js
rainer-friederich
left a comment
rainer-friederich
left a comment
There was a problem hiding this comment.
The removal itself is faithful to §10.1/§10.2 and behaviour-preserving. I checked the flag-OFF equivalence rather than taking it on trust: on main, createHeadroomGuard with enabled: false returns ensure: async () => ({ toppedUp: false }) and retryOnQuota: (fn) => fn(), and both publish seams already default to an identity wrapper (handlers/prompts.js:402, handlers/markets.js:1209), so every deleted wrapper was a literal passthrough. The positional parentWorkspaceId that ensureSubworkspace/assertNotParent need survives; only the allocator's copy in the options bags went.
Four things to change before this lands.
1. The rightsizing sweep is not a deviation — the description has it backwards
§10.7 prescribes exactly what this PR did:
scripts/serenity-rightsizing-sweep.mjs— [...] It goes with §10.1, which it cannot compile without — it hard-importsreleaseAiSurplusand the block constants.
"Delete last" in §10.7 applies to the metered-405 canary, not the sweep. Deviation 2 should come out of the description; deleting the sweep is compliance, not divergence.
Deviation 1 (trimming allocation-metrics.js instead of deleting it) is correct and the spec is what's inconsistent: recordRejection is called by toQuotaExceededError (src/support/serenity/errors.js:173) and recordMeteredQuotaClassifier by isMeteredQuota (errors.js:112), both §10.6 keeps, while §10.6 itself names recordMeteredQuotaClassifier in its own delete set. Keeping the two is the only resolution that compiles — worth saying that plainly rather than as a deviation needing forgiveness.
2. Dead code the removal creates but does not clear
Each of these had exactly one production caller, and this PR deletes it:
countPublishedPrompts(src/support/serenity/handlers/markets.js:1095) — called only by the net-model-delta sizing inhandleUpdateModelsSubworkspace. It now has no caller insrc/; only its own unit test (test/support/serenity/handlers/markets.test.js:1886-1926) keeps it alive.alertPoolFreeThreshold(src/support/serenity/quota-alerts.js:264) — called only fromresource-manager.js:445onmain. It is the allocator's org-pool early-warning, so it is §10.1 in substance even though §10.6 filesquota-alerts.jswholesale for later. Delete it here, or state in the follow-up list that §10.6 inherits a function that can no longer fire.- The two
wrapPublishseams are now caller-less identity wrappers, and their JSDoc still describes the machinery this PR removes:handlers/prompts.js:381-386("The subworkspace create-prompts caller passesheadroom.retryOnQuota(LLMO-6190 item 4)") andhandlers/markets.js:1184-1196(same). §10.3 revisits this seam anyway, so keeping the parameter is defensible — the doc text pointing at deleted code is not.
3. Test and IT-harness leftovers
transferWorkspaceResourcesis still stubbed and assertedto.not.have.been.calledagainst a transport method that no longer exists:test/support/serenity/workspace-lifecycle.test.js(17 sites),test/support/serenity/brand-provisioning.test.js(7),test/support/serenity/handlers/markets-subworkspace.test.js:93,1021. Those assertions can no longer fail. The latter two files are already touched here.setUmMockQuota(test/it/postgres/setup.js:165) has no consumer left once the flag-ON IT block goes, but is still exported and threaded intomockControls(test/it/postgres/serenity.test.js:16,21). Keeping the__quotacontrol route until the spacecat-shared §10.5 change lands is reasonable — say so at the seam instead of leaving it looking wired.- Comments that still justify their subject by the removed guard:
test/it/postgres/setup.js:158("Used by the dynamic-allocation flag-ON IT"),test/it/shared/seed-ids.js:69-71("the condition the dynamic-allocation JIT guard needs to engage"),test/it/postgres/seed-data/organizations.js:30,test/it/README.md:206, andtest/support/serenity/handlers/markets.test.js:1916("resource-manager'smodelChangeUnitssizing").
4. ADR-008 keeps two pointers that no longer resolve
docs/serenity.md and ADR-007 are handled well — the replacement paragraph naming the canary as the premise re-check is exactly what §10.7 asked for. ADR-008 is the gap: the new header marks what follows as historical, but two of those lines are live operator instructions, not history.
docs/decisions/008-no-subworkspace-resource-carve.md:70— "scripts/serenity-rightsizing-sweep.mjscan reclaim them if a pool ever does matter" points at a file this PR deletes. The consequence is now that stranded carves have no remedy; the line should say that.:76— points atdocs/serenity.md§ When to turn the JIT allocator on, a section this PR removes.:72-73and:85now describe the inverse of reality: the alternative recorded as "Delete the JIT allocator alongside the carve. Rejected" is the decision being taken here. An ADR whose rejected alternative becomes the adopted one normally gets a superseding record rather than a header note.
On the §10 precondition
§10 requires verifying once per environment that a metered write into a zero-allocation sub-workspace succeeds, and names scripts/serenity-metered-405-canary.mjs as the instrument. It is ordered as a hard gate before §10.6, so this is not a blocker for §10.1 — but §10.1 deletes the mitigation that the removed "when to turn the JIT allocator on" procedure pointed at, on a premise currently resting on the vendor statement plus the single GM-migration observation. The canary stays in the tree and needs one IMS token; running it on dev, stage and prod and recording the results in the follow-up list would turn the premise into per-environment evidence, and it is the same run §10.6 will need regardless. The soak requirement ("not the same week") is met.
Description
Drop the Verification block — lint, type-check and test counts are what the checks report. Everything else in the description is worth keeping.
One observation for the record, no action needed here: §10.1's "wiring to unpick" list omits handlers/markets-subworkspace.js, which carried the largest share of the wiring. Good catch finding it — the inventory under-scoped.
…e ADRs (SITES-49206) Follows Rainer's review on #2995: - delete dead countPublishedPrompts (markets.js) and alertPoolFreeThreshold (quota-alerts.js) — both had the removed allocator as their only caller — with their tests. - keep the two wrapPublish injection seams but rewrite their JSDoc to stop pointing at the removed headroom.retryOnQuota machinery (§10.3 seam). - drop the now-vacuous transferWorkspaceResources stubs/assertions from the workspace-lifecycle, brand-provisioning and markets-subworkspace tests (the transport method no longer exists). - document the deliberately-retained setUmMockQuota / __quota IT seam at test/it/postgres/setup.js and serenity.test.js (kept for spacecat-shared §10.5); reconcile the stale guard-justifying comments in seed-ids.js, organizations.js and test/it/README.md. - add ADR-009 as the superseding record for the allocator removal (ADR-008's rejected "delete the allocator" alternative is now adopted); reconcile the ADR-007/008 headers + inline pointers and docs/serenity.md (which had wrongly listed the trimmed allocation-metrics.js as deleted). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
aliciadriani
left a comment
aliciadriani
left a comment
There was a problem hiding this comment.
Inline notes marking each change made to address the review — one per fix, grouped by location.
| * @param {any} [log] | ||
| * @returns {Promise<number>} number of published prompts on the project. | ||
| */ | ||
| export async function countPublishedPrompts(transport, semrushWorkspaceId, projectId, log) { |
There was a problem hiding this comment.
Fix (dead code cleared): deleted countPublishedPrompts. Its only production caller was the net-model-delta sizing in handleUpdateModelsSubworkspace, which this PR removed — so the function had no caller left in src/ and only its own unit test kept it alive. Removed here with that test.
| * `headroom.retryOnQuota` (LLMO-6190 item 4) so a disguised metered-405 gets ONE bounded | ||
| * top-up+retry; flat-mode callers omit this param, so flat mode is untouched. | ||
| * the inner `publishProject` call. Retained as an injection seam for a future publish-retry | ||
| * wrapper (§10.3); no caller passes a non-identity wrapper today, so every publish is a plain call. |
There was a problem hiding this comment.
Fix (stale JSDoc): the wrapPublish parameter is kept as an injection seam for a future publish-retry wrapper (§10.3), but its JSDoc used to describe the removed headroom.retryOnQuota (LLMO-6190) machinery. Rewritten to describe the seam as it now is: identity default, no non-identity caller today.
| * item 4) so a disguised metered-405 gets ONE bounded top-up+retry per project BEFORE it is | ||
| * recorded as a failure; flat-mode callers omit this param, so flat mode is untouched. | ||
| * behavior). Retained as a per-project injection seam for a future publish-retry wrapper | ||
| * (§10.3); no caller passes a non-identity wrapper today, so every publish is a plain call. |
There was a problem hiding this comment.
Fix (stale JSDoc): same wrapPublish seam as in markets.js — parameter retained (§10.3), JSDoc rewritten to stop pointing at the removed headroom.retryOnQuota machinery.
| * @param {object} [log] | ||
| * @returns {Promise<void>} | ||
| */ | ||
| export async function alertPoolFreeThreshold({ |
There was a problem hiding this comment.
Fix (dead code cleared): deleted alertPoolFreeThreshold, the org-pool early-warning alert. Its only caller was resource-manager.js (the deleted allocator). It is §10.1 in substance even though §10.6 files quota-alerts.js wholesale for later, so removing it here avoids §10.6 inheriting a function that can never fire. Its tests were removed too.
| * Global kill-switch (`SERENITY_QUOTA_ALERTS_ENABLED`, default OFF) mirrors the existing | ||
| * `SERENITY_*` env-flag pattern (dynamic-allocation-active.js, rest-transport.js) — adding this | ||
| * capability is not the same as turning it on; ops configure the channel/token and flip it. | ||
| * `SERENITY_*` env-flag pattern (rest-transport.js) — adding this capability is not the same as |
There was a problem hiding this comment.
Fix (stale reference): the module header cited dynamic-allocation-active.js (deleted by this PR) as an example of the SERENITY_* env-flag pattern. Trimmed to reference only a file that still exists.
| - **Transport target (`env.js`):** `SEMRUSH_PROJECTS_BASE_URL` → PE mock, `SEMRUSH_USERS_BASE_URL` → UM mock (the User-Manager origin split landed in api-service#2656; it falls back to the projects host when unset, so production needs no new config). `NODE_TLS_REJECT_UNAUTHORIZED=0` trusts the self-signed certs (IT process only). **None of these need Vault / deployed-env config.** | ||
| - **Auth (`SERENITY_ALLOW_NON_IMS_AUTH=true`, test-only):** the serenity controller normally forwards only IMS-typed tokens. This flag lets the harness's non-IMS JWT through. It's sound only against the mocks (which ignore the forwarded bearer); in production the real Semrush gateway validates the token end to end, so this never weakens deployed auth. The flag is never set in any deployed environment. | ||
| - **Statefulness / isolation:** the mocks are stateful within a run. Auth-exempt control routes — `POST /<prefix>/__reset`, `POST /<prefix>/__seed`, `GET /<prefix>/__dump`, `POST /<prefix>/__quota` — manage that. `setup.js` exposes `resetSemrushMocks()` for tests that mutate mock state, plus `dumpPeMock()` / `dumpUmMock()` (store snapshots) and `setUmMockQuota()` (UM resource metering); the mutating tests reach them through the factory's `mockControls` argument. Readiness is polled via `waitForSemrushMocks()`. | ||
| - **Statefulness / isolation:** the mocks are stateful within a run. Auth-exempt control routes — `POST /<prefix>/__reset`, `POST /<prefix>/__seed`, `GET /<prefix>/__dump`, `POST /<prefix>/__quota` — manage that. `setup.js` exposes `resetSemrushMocks()` for tests that mutate mock state, plus `dumpPeMock()` / `dumpUmMock()` (store snapshots) and `setUmMockQuota()` (UM resource metering — retained and still threaded through `mockControls`, but consumed by no test today after the allocator removal in SITES-49206; kept for the spacecat-shared §10.5 metered-write change); the mutating tests reach the others through the factory's `mockControls` argument. Readiness is polled via `waitForSemrushMocks()`. |
There was a problem hiding this comment.
Fix (stale doc): the README presented setUmMockQuota() as reached by mutating tests. Noted it is retained-but-unconsumed after the allocator removal, kept for the spacecat-shared §10.5 change.
| > `resource-lock.js`) and its operational surface — the "when to turn it on" flip, the | ||
| > zombie-workspace runbook, and the rightsizing sweep — have been deleted. Semrush no longer enforces | ||
| > AI limits for proxy-routed LLMO workspaces, so there is no allocation to top up, exhaust, or | ||
| > reclaim. `allocation-metrics.js` is **trimmed, not deleted** — `recordRejection` and |
There was a problem hiding this comment.
Fix (incorrect doc): this block listed allocation-metrics.js among the deleted modules, but it was trimmed (kept recordRejection / recordMeteredQuotaClassifier for the §10.6 classifier). Corrected, and added the ADR-009 reference for the removal.
| @@ -0,0 +1,85 @@ | |||
| # ADR-009: Remove the dormant JIT Semrush AI allocator | |||
There was a problem hiding this comment.
Fix (superseding record): new ADR-009 — ADR-008's rejected alternative "Delete the JIT allocator alongside the carve" is the decision this PR actually takes, so it gets a superseding record rather than only a header note. It documents the deletion, why the earlier rejection resolved, and the metered-405-canary as the standing per-env check.
| @@ -1,5 +1,14 @@ | |||
| # ADR-008: A brand sub-workspace carries no resource allocation | |||
|
|
|||
| > **Update (2026-08, SITES-49206).** The **no-carve decision stands and is now unconditional.** Its | |||
There was a problem hiding this comment.
Fix (stale pointers / reversed decision): ADR-008's no-carve decision stands, but its secondary "retain the allocator dormant" decision is reversed. Upgraded the header to say so and point at ADR-009; fixed the two dead body pointers (the deleted rightsizing-sweep line → "no in-tree remedy"; the removed docs/serenity.md flip-procedure section) and marked the reversed Decision/Alternatives inline.
| # ADR-007: Cross-container serialization for the dynamic-allocation absolute-set race | ||
|
|
||
| > **Applies only when `SERENITY_DYNAMIC_ALLOCATION` is on**, which it is not in any deployed | ||
| > **SUPERSEDED (2026-08, SITES-49206; see [ADR-009](009-remove-dormant-jit-allocator.md)).** The |
There was a problem hiding this comment.
Fix (cross-reference): added a pointer to ADR-009 (the record of the allocator removal) in this already-superseded ADR's header, for navigability.
|
@rainer-friederich thanks for the thorough review — all of it is addressed in a1abe03 (inline notes on each change above). Summary: 2 — dead code the removal stranded
3 — test / IT-harness leftovers
4 — ADR-008 / superseding record
1 & description
Not done, by design: the §10 metered-405 canary run (dev/stage/prod, live IMS token) stays a human step on the follow-up checklist. Verification: lint clean, |
rainer-friederich
left a comment
rainer-friederich
left a comment
There was a problem hiding this comment.
Hey @aliciadriani,
Verdict: Request changes - every fix from the last round landed and verifies clean, but the same commit deletes a transport method that the retained premise-check script still calls.
Changes: removes the dormant JIT Semrush AI allocator with its wiring, tests, allocator-era script and docs, and adds ADR-009 as the superseding decision record (43 files).
Must fix before merge
- [Critical] The retained metered-405 canary calls
transferWorkspaceResources, which this PR removes from the transport -src/support/serenity/rest-transport.js:1057(details inline) - [Important] ADR-009 records the canary as a standing signal, but section 10.7 makes it a manual probe that is itself deleted alongside the classifier -
docs/decisions/009-remove-dormant-jit-allocator.md:54(details inline)
Non-blocking (8): stale references the sweep did not reach
Each of these was accurate on main and became stale because of this PR, so they are removal residue rather than pre-existing debt.
- nit:
resource-lock.js's in-process lock cited as an existing peer cache, andclearResourceLockscited as a sibling reset -src/support/serenity/quota-alerts.js:34and:68(the same header block whose line 25 this commit corrected) - nit: the
QuotaCaseunion still admitsorgPoolExhaustedandbrandAiLimit; at head every producer passesbrandCarveExhausted, and their only former producer wasresource-manager.js-src/support/serenity/quota-alerts.js:40 - nit: cleanup guidance tells the operator to re-run
ensureAiHeadroom-scripts/serenity-metered-405-canary.mjs:54 - nit: the positional parent is documented as "the units pool when JIT allocation is on"; there is no JIT allocation at head -
src/support/serenity/handlers/markets-subworkspace.js:480 - nit: "// ensure was skipped: no settle/transfer was performed for this call" no longer has an assertion behind it (the test itself is fine - it still asserts the pre-resolved workspace) -
test/support/serenity/handlers/markets-subworkspace.test.js:1019 - nit:
dumpUmMockis now consumer-less exactly likesetUmMockQuota, but did not get the retention note this commit added to its sibling -test/it/postgres/setup.js:203, threaded attest/it/postgres/serenity.test.js:25, no longer destructured attest/it/shared/tests/serenity.js:843 - nit: "the mutating tests reach the others through the factory's
mockControlsargument" - "the others" is now onlydumpPeMock-test/it/README.md:206 - suggestion: the retained seam gets two different forward owners - the code says section 10.3, ADR-009 says section 10.6 -
docs/decisions/009-remove-dormant-jit-allocator.md:44vssrc/support/serenity/handlers/markets.js:1132andhandlers/prompts.js:383
Previously flagged, now resolved
- Description no longer frames the sweep deletion or the
allocation-metrics.jstrim as deviations, and the Verification block is gone. countPublishedPromptsandalertPoolFreeThresholddeleted with their tests; neither has a caller left anywhere at head.- Both
wrapPublishJSDoc blocks rewritten; no non-identity caller exists at head, so the text is accurate. - All 26 vacuous
transferWorkspaceResourcesstubs and assertions removed, and no survivingit()lost its only meaningful assertion. setUmMockQuota/__quotaretention documented at the seam and in the IT README; the guard-justifying comments now describe the survivingparentWorkspaceIdrequirement, whichworkspace-lifecycle.js:410still enforces.- ADR-008's two dead pointers fixed and its reversed decision marked, ADR-009 added as the superseding record, ADR-007 cross-referenced, and
docs/serenity.mdcorrected to stop listing the trimmedallocation-metrics.jsas deleted.
| * | ||
| * NOTE (SITES-49206): the just-in-time allocator that read this before a metered op was removed | ||
| * once Semrush stopped enforcing AI limits for proxy-routed LLMO workspaces; this read has no | ||
| * in-repo caller today but is kept as part of the transport's workspace-resource surface. |
There was a problem hiding this comment.
issue (blocking): scripts/serenity-metered-405-canary.mjs:122 calls transport.transferWorkspaceResources(...), and this PR deletes that method just below this note. The canary dies with a TypeError at step 2 (drain prompts.total to used) - after the getWorkspaceResources read on line 109, and before the publish that produces the evidence.
I checked every other transport method the script uses - listLanguages, createProject, createProjectTags, createPromptsByIds, publishProject, getWorkspaceResources. All six survive at head. transferWorkspaceResources is the only missing one, so the script is one method away from working.
Why this is more than a broken dev script:
- Section 10.7 keeps the canary deliberately and makes it the instrument for section 10's precondition: "Run it per env, record the result, and remove it only once section 10.6's classifier is gone."
- The description says the canary "only imports the transport" and is therefore safe to keep. It does only import the transport - the transport is what lost the method.
- ADR-009, added in this commit, names the canary twice as the standing per-environment premise check and as the replacement for the removed flip procedure.
docs/decisions/008-no-subworkspace-resource-carve.md:86and:104anddocs/serenity.md:457say the same. Four documented claims currently describe a script that cannot run. - ADR-009's own Alternatives section rejects keeping
serenity-rightsizing-sweep.mjsbecause it "hard-importsreleaseAiSurplus... so it cannot compile once the allocator is gone". That is the same dependency argument, applied to the transport rather than toresource-manager.js, and it was not carried across to the canary. - Nothing in CI can catch this.
scripts/sits outside thetsconfig.jsoninclude list, the call is a dynamic property access so lint does not resolve it, and the script never executes in CI. The green checks are not evidence here.
There is also an asymmetry in this very note: getWorkspaceResources is kept as having "no in-repo caller today but kept as part of the transport's workspace-resource surface", while its pair is deleted. The canary's step 1 is getWorkspaceResources and its step 2 is transferWorkspaceResources - one consumer, both methods.
Smallest fix: keep transferWorkspaceResources under the same rationale as the method above it, until sections 10.6 and 10.7 retire the canary together - that is the same "delete last" sequencing section 10.7 already grants the canary. If it has to go now, rewrite the drain step against a surviving call and re-run the script end to end, then correct the "it only imports the transport" line in the description and record the dependency in ADR-009 so the later sections sequence it knowingly.
There was a problem hiding this comment.
Fixed (now at branch head, commit 594dbd13). Restored transferWorkspaceResources on the transport with its unit test — you're right that it isn't allocator-only: it and getWorkspaceResources are the canary's read/drain pair. Both methods now carry a NOTE naming the canary as their sole remaining consumer and marking them delete-last with §10.6/§10.7. I also corrected the description's "removed transferWorkspaceResources" and "canary only imports the transport" claims, and recorded the dependency in ADR-009's Decision so §10.6/§10.7 sequence it knowingly.
Verified: node --check on the canary + a live typeof transport.transferWorkspaceResources === 'function' probe, type-check (base + strict) green, and the restored transport unit test passing.
|
|
||
| 1. *"The evidence supports 'our parents are unmetered', not 'no tenant is ever metered'."* Semrush's | ||
| confirmation plus the soak now support the stronger claim. The standing check that it still holds | ||
| is the `scripts/serenity-metered-405-canary.mjs` probe (serenity-docs#72 §10), run per |
There was a problem hiding this comment.
issue (blocking): this line calls the canary "the standing check", and line 70 calls it "the replacement signal" for the removed flip procedure. Section 10.7 describes something narrower on both counts. It "needs a live IMS token and a real sub-workspace id, so a human runs it" - nothing schedules it, and this PR adds no scheduling. And it is marked "delete last": removed once section 10.6's classifier is gone.
So the only re-check mechanism this ADR records is itself queued for deletion inside the same cleanup. Once section 10.6 lands, this line, line 70, docs/decisions/008-no-subworkspace-resource-carve.md:86 and :104, and docs/serenity.md:457 all become dead pointers - the same defect class flagged in ADR-008 last round and repaired in this commit, reintroduced one step further out. Section 10.7 anticipates exactly this: "Whatever replaces this paragraph must say how the limits-unenforced premise gets re-checked, rather than pointing at a token that no longer exists."
This is the durable record for a 5,000-line deletion resting on a vendor behaviour change confirmed 2026-07-28, so a reader arriving cold after section 10.6 needs the successor named, not the interim probe.
Fix, prose only and on this branch: in Consequences, say that the canary is a manual per-environment probe which section 10.7 retires together with the section 10.6 classifier, and that the durable re-check beyond that point is an open question section 10.6 must settle before deleting both. Align the ADR-008 bullet at line 86 to the same wording so the two records do not diverge.
There was a problem hiding this comment.
Fixed. Reframed the canary as an interim, manual per-environment probe that §10.7 retires together with the §10.6 classifier, and called out the durable re-check as an open question §10.6 must settle before deleting both. Applied to ADR-009 (both the rejection-grounds item and the Consequences bullet), the matching ADR-008 bullet (:86) and its Alternatives note (:104), and docs/serenity.md:457. ADR-009's Consequences now states explicitly that leaving a canary pointer past §10.6 would reintroduce the same dead-pointer defect class ADR-008 was repaired for last round.
…frame canary as interim probe (SITES-49206) Rainer's re-review on #2995: - [Critical] Restore transferWorkspaceResources on the transport (+ its unit test). The retained metered-405 canary (scripts/serenity-metered-405-canary.mjs:122) calls it; the PR had removed it, so the canary would TypeError at step 2. Neither it nor getWorkspaceResources is allocator-only any more — both are the canary's read/drain pair, retired with it under §10.6/§10.7 delete-last. Updated both methods' notes to name the canary as their consumer. - [Important] Reframe the canary as an interim, MANUAL per-env probe that §10.7 retires together with the §10.6 classifier — the durable re-check beyond that is an open question §10.6 must settle. Fixed ADR-009, ADR-008 and docs/serenity.md, which had called it a "standing signal" / "replacement signal". - Nits: quota-alerts.js header dropped resource-lock.js / clearResourceLocks refs; narrowed QuotaCase to brandCarveExhausted; canary cleanup note no longer says re-run ensureAiHeadroom; markets-subworkspace.js parent doc no longer cites JIT allocation; markets-subworkspace.test.js comment no longer claims a removed assertion; dumpUmMock got the same retention note as setUmMockQuota (both threaded-but-unconsumed; only dumpPeMock has a live consumer); README "the others" corrected; reconciled the wrapPublish seam owner (§10.3 in both code and ADR-009). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
aliciadriani
left a comment
aliciadriani
left a comment
There was a problem hiding this comment.
Re-review round: inline notes on each new fix. The two blocking items are answered in-thread; these mark the code/doc changes.
| * NOTE (SITES-49206): the just-in-time allocator that used to call this — `transferOnce` / | ||
| * `transferAndSettle` in the removed `resource-manager.js` — is gone. The sole remaining caller | ||
| * is the retained metered-405 canary (`scripts/serenity-metered-405-canary.mjs`, step 2), which | ||
| * drains a throwaway sub-workspace to zero prompt headroom to provoke the disguised 405. Kept — |
There was a problem hiding this comment.
Re-review fix [Critical]: restored transferWorkspaceResources (with its unit test). The retained metered-405 canary drains a throwaway sub-workspace through this method, so it is not allocator-only — it and getWorkspaceResources (its NOTE updated too) are the canary's read/drain pair, retired together under §10.6/§10.7 delete-last.
| /** @typedef {'brandCarveExhausted'|'orgPoolExhausted'|'brandAiLimit'} QuotaCase */ | ||
| // `orgPoolExhausted` / `brandAiLimit` were produced only by the removed allocator (SITES-49206); | ||
| // every producer at head passes `brandCarveExhausted`. | ||
| /** @typedef {'brandCarveExhausted'} QuotaCase */ |
There was a problem hiding this comment.
Re-review nit: narrowed QuotaCase — orgPoolExhausted / brandAiLimit were produced only by the removed allocator; every producer at head passes brandCarveExhausted.
| * five times is one sales signal, not five pages." This is warm-Lambda-container-scoped state | ||
| * (no DynamoDB/persistent store), the same shape as this codebase's existing per-container caches | ||
| * (markets.js's language cache, resource-lock.js's in-process lock) — sufficient for collapsing a | ||
| * (markets.js's language cache) — sufficient for collapsing a |
There was a problem hiding this comment.
Re-review nit: header no longer cites resource-lock.js's in-process lock (deleted) as a peer cache; the clearResourceLocks sibling-reset mention on clearQuotaAlertDedup was dropped too.
| * @param {object} brand - brand record/stub being provisioned. | ||
| * @param {string} parentWorkspaceId - the org parent workspace: the `assertNotParent` guard | ||
| * input, and the units pool when JIT allocation is on. Nothing is carved from it. | ||
| * @param {string} parentWorkspaceId - the org parent workspace, used as the `assertNotParent` guard |
There was a problem hiding this comment.
Re-review nit: the positional parentWorkspaceId doc no longer says it's "the units pool when JIT allocation is on" — there is no JIT allocation at head; it's only the assertNotParent guard input now.
| * to run against a disposable dev/throwaway sub-workspace. Re-run `ensureAiHeadroom`/an ordinary | ||
| * API top-up (or just re-activate the brand) afterwards if the workspace needs to keep working, or | ||
| * decommission the throwaway workspace entirely. | ||
| * to run against a disposable dev/throwaway sub-workspace. To restore headroom afterwards, transfer |
There was a problem hiding this comment.
Re-review nit: cleanup guidance no longer tells the operator to re-run the removed ensureAiHeadroom; it now points at the inverse transferWorkspaceResources transfer (or re-activating the brand).
| - **Transport target (`env.js`):** `SEMRUSH_PROJECTS_BASE_URL` → PE mock, `SEMRUSH_USERS_BASE_URL` → UM mock (the User-Manager origin split landed in api-service#2656; it falls back to the projects host when unset, so production needs no new config). `NODE_TLS_REJECT_UNAUTHORIZED=0` trusts the self-signed certs (IT process only). **None of these need Vault / deployed-env config.** | ||
| - **Auth (`SERENITY_ALLOW_NON_IMS_AUTH=true`, test-only):** the serenity controller normally forwards only IMS-typed tokens. This flag lets the harness's non-IMS JWT through. It's sound only against the mocks (which ignore the forwarded bearer); in production the real Semrush gateway validates the token end to end, so this never weakens deployed auth. The flag is never set in any deployed environment. | ||
| - **Statefulness / isolation:** the mocks are stateful within a run. Auth-exempt control routes — `POST /<prefix>/__reset`, `POST /<prefix>/__seed`, `GET /<prefix>/__dump`, `POST /<prefix>/__quota` — manage that. `setup.js` exposes `resetSemrushMocks()` for tests that mutate mock state, plus `dumpPeMock()` / `dumpUmMock()` (store snapshots) and `setUmMockQuota()` (UM resource metering); the mutating tests reach them through the factory's `mockControls` argument. Readiness is polled via `waitForSemrushMocks()`. | ||
| - **Statefulness / isolation:** the mocks are stateful within a run. Auth-exempt control routes — `POST /<prefix>/__reset`, `POST /<prefix>/__seed`, `GET /<prefix>/__dump`, `POST /<prefix>/__quota` — manage that. `setup.js` exposes `resetSemrushMocks()` for tests that mutate mock state, plus `dumpPeMock()` (PE store snapshot — the one control a mutating test reaches today, through the factory's `mockControls` argument). `dumpUmMock()` (UM store snapshot) and `setUmMockQuota()` (UM resource metering) are also threaded through `mockControls` but are consumed by no test today after the allocator removal in SITES-49206 — retained for the spacecat-shared §10.5 metered-write change. Readiness is polled via `waitForSemrushMocks()`. |
There was a problem hiding this comment.
Re-review nit: "the others" was inaccurate — only dumpPeMock has a live consumer; dumpUmMock / setUmMockQuota are retained-but-unconsumed.
| re-meter sizing, both of which had the allocator as their only caller; | ||
| - the flag-ON integration path. | ||
|
|
||
| Three things at the metered-write boundary are **kept**, none belonging to the allocator any more: |
There was a problem hiding this comment.
Re-review fix: records that getWorkspaceResources / transferWorkspaceResources are kept for the canary (delete-last), and splits the seam owners — wrapPublish → §10.3, classifier → §10.6 (aligning ADR with the code JSDoc).
| procedure.** ADR-008 pointed operators at a "when to turn the JIT allocator on" runbook; that | ||
| section and the allocator it armed are both removed. *For now* the `serenity-metered-405-canary.mjs` | ||
| probe is the re-check — a **manual** per-environment run whose failure is the trigger to | ||
| re-introduce the allocator from history, not to re-enable a flag. It is **not** a standing signal: |
There was a problem hiding this comment.
Re-review fix [Important]: the canary is reframed as an interim manual probe that §10.7 retires with the §10.6 classifier; the durable re-check is called out as an open question §10.6 must settle.
| - **A tenant whose parent enforces limits would get no sizing at all.** _(SITES-49206: this was the | ||
| risk the retained allocator covered. The allocator, and the "when to turn the JIT allocator on" | ||
| flip procedure this line pointed at in `docs/serenity.md`, are both removed — see ADR-009. The | ||
| re-check, for now, is the `scripts/serenity-metered-405-canary.mjs` probe — a **manual** |
There was a problem hiding this comment.
Re-review fix [Important]: aligned ADR-008's bullet to the same "interim manual probe, retired with §10.6" wording as ADR-009.
| The parent-pool premise is re-checked, not assumed: `scripts/serenity-metered-405-canary.mjs` | ||
| drives the real transport against a throwaway sub-workspace and publishes into zero headroom, so a | ||
| future re-tightening upstream surfaces as a failed canary rather than a silent customer-facing | ||
| error. It is a **manual** per-environment run (live IMS token + a real sub-workspace id; nothing |
There was a problem hiding this comment.
Re-review fix [Important]: the canary paragraph now says it's a manual per-env probe retired with §10.6, not the durable re-check.
|
@rainer-friederich re-review addressed — branch head [Critical] canary's transport dependency — restored [Important] canary over-described — reframed as an interim, manual per-env probe that §10.7 retires with the §10.6 classifier, with the durable re-check named as an open question §10.6 must settle. Applied across ADR-009, ADR-008 (:86, :104) and Nits (all 8) — Verified: lint + |
rainer-friederich
left a comment
rainer-friederich
left a comment
There was a problem hiding this comment.
Hey @aliciadriani,
Verdict: Request changes - the restore landed and every nit from the last round is clean, but the note that justifies it names the wrong caller, and the type guard that pinned the restored method did not come back with it.
Changes: removes the dormant JIT Semrush AI allocator with its wiring, tests, allocator-era script and docs, and adds ADR-009 as the superseding decision record (43 files).
Must fix before merge
- [Important]
getWorkspaceResourcesis documented as canary-only, butelements.jscheckAccesscalls it in production - and ADR-009 plus the §10.6 checklist schedule it for deletion -src/support/serenity/rest-transport.js:1055(details inline) - [Important] The
@ts-expect-errorguard pinningtransferWorkspaceResourceswas deleted with the method and not restored with it -test/types/base/serenity-transport.types.js:55(details inline) - [Important] The records make the canary the premise re-check but describe its outcome backwards, and it has no failure signal -
docs/decisions/009-remove-dormant-jit-allocator.md:78(details inline)
Non-blocking (3): minor issues and suggestions
- nit:
caseType: 'orgPoolExhausted'is still the caseType-varying arm of the dedup-key test, after this commit narrowedQuotaCasetobrandCarveExhaustedalone -test/support/serenity/quota-alerts.test.js:116 - suggestion: the strict tier is the only one that reports an unknown member, so adding
getWorkspaceResourcesandtransferWorkspaceResourcescall lines totest/types/strict/serenity-transport-strict.types.jswould give the canary's call site the compile-time gate it has never had - a rename would otherwise stay green acrossrest-transport.jsand its unit test and break the canary again - nit: the delete-last coupling is documented one way only - the transport notes point at the canary, but the canary's own header says nothing about what its deletion strands -
scripts/serenity-metered-405-canary.mjs:16
Previously flagged, now resolved
transferWorkspaceResourcesrestored with a unit test whose payload shape matches both the canary's call site and the user-manager v2 transfer contract.- Canary reframed as an interim manual probe across ADR-009, ADR-008 and
docs/serenity.md, with the durable re-check named as an open question for §10.6. - All eight nits landed: the
resource-lock.js/clearResourceLocksreferences dropped,QuotaCasenarrowed, canary cleanup no longer citingensureAiHeadroom, the positional parent doc de-JIT'd, the assertion-less test comment reworded,dumpUmMockgiven the retention note, the IT README corrected, and thewrapPublishseam owner reconciled to §10.3 in both places. - The three test-file edits in this commit are comment-only; no assertion was weakened.
| * NOTE: use this on the master id for the pool — `/parent/resources` returns the workspace's | ||
| * OWN allocation, not the master pool (live-verified 2026-07-02). | ||
| * | ||
| * NOTE (SITES-49206): the just-in-time allocator that read this before a metered op was removed |
There was a problem hiding this comment.
issue (blocking): this NOTE says the canary is getWorkspaceResources's "one remaining caller". It is not. src/controllers/elements.js:703 calls it from checkAccess, the handler for GET /v2/orgs/:spaceCatId/brands/:brandId/serenity/brand-presence/access (LLMO-6747). That endpoint is the whole "Semrush workspace access needed" banner: it probes the resource-allowance endpoint with the caller's own IMS token and reads a 401/403 as hasAccess: false. It is live on main, untouched by this PR, and covered by test/controllers/elements.test.js.
The claim on the sibling below is correct - I checked transferWorkspaceResources at head and the canary really is its only caller outside the transport itself and the new unit test. So the two methods have two different lifetimes, and this commit binds them to one.
Why this is more than a comment being wrong: the same claim is now written into two places that outlive the comment.
docs/decisions/009-remove-dormant-jit-allocator.md:47says both methods "are therefore retired with the canary under the same delete-last sequencing".- The description's §10.6 checklist item says that step "also retires the canary itself and its two retained transport methods (
getWorkspaceResources/transferWorkspaceResources)".
That checklist is the instruction a future engineer executes. Following it deletes a method a production endpoint depends on, and the failure is quiet rather than loud: checkAccess documents every non-401/403 error as INDETERMINATE, which the UI treats as "assume access". A TypeError there maps to a 5xx, so the banner silently stops firing for users who genuinely lack workspace access, with nothing surfaced to them.
This is also last round's defect class, inverted. Then, a method was deleted because its surviving caller was invisible to CI. The fix for that records a caller inventory by hand, and the inventory under-counts - in the direction that looks safe.
Fix: name elements.js checkAccess here as the production caller and drop this method from the canary-coupled sequencing; only transferWorkspaceResources below is canary-scoped. Then split the pair in ADR-009:47 and in the description's §10.6 item so both record two methods with two lifetimes. While correcting that inventory: ADR-009's "Three things at the metered-write boundary are kept" at :44 is also missing the fourth retained surface - the IT __quota controls (setUmMockQuota / dumpUmMock, §10.5), which today exist only in the test files and not in the record a later section will read.
| // @ts-expect-error - string has no properties in common with { projects?, prompts? }. | ||
| await transport.transferWorkspaceResources(parentWorkspaceId, { ai: 'unlimited' }); | ||
|
|
||
| // Argument types. |
There was a problem hiding this comment.
issue (blocking): the assertion that pinned transferWorkspaceResources was removed here along with the method, and did not come back with it. On main this block reads:
// Body shape, derived from the generated contract: `ai` carries unit counts.
// @ts-expect-error - string has no properties in common with { projects?, prompts? }.
await transport.transferWorkspaceResources(parentWorkspaceId, { ai: 'unlimited' });Everything else about the method came back in this commit - the implementation, the JSDoc with both @param tags, the WorkspaceResources typedef at rest-transport.js:77, the unit test, the ADR entry. This one artefact did not, and its absence is silent: a missing @ts-expect-error fails nothing.
src/support/serenity/CLAUDE.md names this file as the enforcement, in rule 6: "A new transport method documents its own parameters ... test/types/base/serenity-transport.types.js pins both halves." docs/decisions/005-opt-in-type-checking.md says the same. This file's own header explains why both halves are load-bearing - dropping a transport method's @param tags makes its assertions legal again, so the assertion is what keeps the tags honest.
With it gone, a later change that strips those tags or widens payload back to {object} compiles clean. That matters more for this method than for its neighbours, because its only consumer is the canary, and this PR's own argument is that scripts/ sits outside tsconfig and never runs in CI - so nothing else is watching.
Fix: re-add the three lines after the createSubworkspace arity block. The signature and the typedef are unchanged from main, so the expected error should be unchanged; if it is not, tsc tells you immediately as TS2578.
| - **A tenant whose parent enforces limits would get no sizing, and there is no longer a flip | ||
| procedure.** ADR-008 pointed operators at a "when to turn the JIT allocator on" runbook; that | ||
| section and the allocator it armed are both removed. *For now* the `serenity-metered-405-canary.mjs` | ||
| probe is the re-check — a **manual** per-environment run whose failure is the trigger to |
There was a problem hiding this comment.
issue (blocking): "whose failure is the trigger to re-introduce the allocator" - the canary has no failure to observe, and its on-screen labels read the other way round.
The script was written for LLMO-6190 to capture the disguised-405 fixture, and it still speaks that language at head:
scripts/serenity-metered-405-canary.mjs:175- "Publishing with zero prompt headroom - expecting the disguised metered-quota 405...":177- "UNEXPECTED: publish succeeded. The workspace may not actually be at zero headroom...":179- prints the 405 under "this is what isMeteredQuota must match"
Under this ADR's premise those meanings invert. A 405 at zero headroom means Semrush is enforcing again: the premise is broken and the allocator has to come back from history. A publish that succeeds at zero headroom is the confirmation the ADR wants. The script calls the first "expected" and the second "UNEXPECTED". And main() returns normally in both branches - exit(1) fires only on a usage error or an unexpected top-level throw - so "a failed canary" names nothing the script actually emits.
The labels themselves predate this PR. What this PR creates is the reliance on them: before it, ADR-008 pointed operators at the "when to turn the JIT allocator on" procedure in docs/serenity.md, and this commit replaces that pointer with the canary in three records at once - here at :78, at docs/decisions/008-no-subworkspace-resource-carve.md:87, and at docs/serenity.md:459. The description's §10.7 item then asks a human to run it on dev, stage and prod and record each result before §10.6. That operator gets UNEXPECTED: publish succeeded in the case where everything is fine, and will reasonably read it as a botched run.
It matters because this ADR accepts a one-way door - "re-introduced from history rather than re-enabled by a dead flag" - and names exactly one safeguard for it. A safeguard whose healthy result is printed as an anomaly is a nominal one.
Fix, prose only and on this branch: drop "failure" as the trigger vocabulary here, at ADR-008:87 and at docs/serenity.md:459, and name the observable outcome instead - the disguised 405 is the signal to re-introduce the allocator, a publish that succeeds against zero headroom confirms the premise holds. Then add two lines to the script header at scripts/serenity-metered-405-canary.mjs:16 saying it now has a second reading under ADR-009, and that its "expected" / "UNEXPECTED" labels refer to fixture capture rather than to the premise.
…pe guard, correct canary outcome semantics (SITES-49206) Rainer's third re-review on #2995: - [Important] getWorkspaceResources is NOT canary-only: elements.js checkAccess (GET .../brand-presence/access, LLMO-6747) calls it in production. Decoupled it from the canary in the transport NOTE, ADR-009 (split the two methods into different lifetimes + added the IT __quota controls as a retained surface), and the PR description's §10.6 checklist. Only transferWorkspaceResources is canary-scoped/delete-last. - [Important] Restored the transferWorkspaceResources @ts-expect-error body-shape guard in test/types/base/serenity-transport.types.js (deleted with the method, not restored with it). Also pinned both methods in the strict-tier types file so a rename is caught as TS2339 rather than only breaking the canary at runtime. - [Important] Corrected the canary outcome vocabulary in ADR-009/ADR-008/serenity.md: the canary exits 0 either way; a publish that SUCCEEDS at zero headroom confirms the premise, a disguised 405 means Semrush is enforcing again. Documented the script's inverted expected/UNEXPECTED (fixture-capture) labels and the delete-last coupling in the canary header. - Nit: quota-alerts dedup-key test no longer varies caseType with the removed orgPoolExhausted (uses rollbackFailed, a caseType the module still keys on). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
2 participants
Premise
Semrush no longer enforces AI project/prompt limits for LLMO workspaces reached via the Adobe proxy (confirmed 2026-07-28). The just-in-time (JIT) allocation machinery this PR removes only ever worked around limits that no longer apply, and its global kill-switch (
SERENITY_DYNAMIC_ALLOCATION) was OFF in every deployed environment, so the removed code paths were dormant. Behaviour on the flag-OFF path is byte-for-byte identical before/after — a no-op headroom guard is simply replaced by the direct call it wrapped.Part of the Serenity Post-GA cleanup (story SITES-49196). Inventory: https://github.com/adobe/serenity-docs/pull/62
Jira: https://jira.corp.adobe.com/browse/SITES-49206
What was removed (§10.1 / §10.2)
Modules deleted (+ their tests):
src/support/serenity/dynamic-allocation-active.js(kill-switch, ceiling resolver,createHeadroomGuard,retryOnQuota)src/support/serenity/resource-manager.js(whole allocator:ensureAiHeadroom,releaseAiSurplus,readAiTotals,roundUpToBlock,modelChangeUnits,transferOnce/transferAndSettle,requireWorkspaceId, blocks/ceiling constants, the org-pool/brand-limit/workspace-busy error factories)src/support/serenity/resource-lock.js(withResourceLock,clearResourceLocks)scripts/serenity-rightsizing-sweep.mjs— deleted per §10.7: it hard-imports the now-deletedreleaseAiSurplusand block constants fromresource-manager.js, so it cannot compile or lint once the allocator is gone. It is pure allocator tooling with no life after the allocator. The metered-405 canary is kept (scripts/serenity-metered-405-canary.mjs); §10.7's "delete last" applies to it, not to the sweep. It readsgetWorkspaceResourcesand drains viatransferWorkspaceResources; only the latter is canary-scoped (the former has a liveelements.jscaller) — see therest-transport.jsnote below.resource-manager.test.js,resource-lock.test.js,dynamic-allocation-active.test.js,dynamic-allocation-fronting.test.jsWiring unpicked:
controllers/serenity.js— removed thedynamicAllocationEnabled/brandAiCeilinghelpers and thedynamicAllocation/parentWorkspaceId/ceilingoptions from all five subworkspace handler call sites.brand-provisioning.js— dropped the flag/ceiling resolution and options.handlers/prompts-subworkspace.js— removed the headroom guard,ensure, andretryOnQuotawrapping (direct calls restored).handlers/markets-subworkspace.js— not in the original file list, but it carried the largest share of the allocator wiring (headroom guards in create-market / update-models / generate-prompts,releaseAiSurpluson delete and net-model-removal,modelChangeUnits). Unpicked here because the modules can't be deleted while it imports them.rest-transport.js—transferWorkspaceResourcesandgetWorkspaceResourcesare both kept, but on different lifetimes.getWorkspaceResourceshas a live production caller —elements.jscheckAccess(GET.../brand-presence/access, LLMO-6747, the "workspace access needed" banner) — so it is not canary-scoped and stays regardless.transferWorkspaceResourcesis canary-scoped: the retained §10.7 metered-405 canary (which drains a throwaway sub-workspace through it) is its only caller, so it is retired with the canary under §10.6/§10.7 "delete last". A strict-tier type pin (test/types/strict/serenity-transport-strict.types.js) now guards both against a silent rename (the canary sits outsidetsconfig/CI). Recorded in ADR-009. (Earlier revisions of this PR removedtransferWorkspaceResourcesoutright, and then wrongly coupledgetWorkspaceResourcesto the canary — both corrected; thanks @rainer-friederich.)errors.js— removedisPoolExhausted/isWorkspaceNotReady(+ orphanedbodyText) and theORG_POOL_EXHAUSTED/BRAND_AI_LIMIT/WORKSPACE_BUSYcodes.isWorkspaceBusyErrordoes not exist in this repo (its elmo-side detector is out of scope).handlers/markets.jscountPublishedPrompts(allocator was its only caller) andquota-alerts.jsalertPoolFreeThreshold(called only fromresource-manager.js), plus their tests. The twowrapPublishpublish-retry seams are kept as identity-default injection points for §10.3, with their JSDoc updated to stop describing the removedheadroom.retryOnQuotamachinery.SERENITY_DYNAMIC_ALLOCATION/SERENITY_BRAND_AI_CEILING_PROJECTS/SERENITY_BRAND_AI_CEILING_PROMPTSremoved.§10.2 (flat pre-carve): already absent —
createSubworkspaceposts noresourcesbody. Only the stale module-header comment inworkspace-lifecycle.jswas updated.Spec inconsistency, resolved the only way that compiles
allocation-metrics.jsis trimmed, not deleted — and this is compliance, not a deviation. §10.6 keepstoQuotaExceededError/isMeteredQuota, which callrecordRejection(errors.js:173) andrecordMeteredQuotaClassifier(errors.js:112) respectively — yet §10.6's own delete set also namesrecordMeteredQuotaClassifier. Those two instructions cannot both be honoured: deleting the recorder (or the whole module) fails to compile against the §10.6-kept callers. So the six allocator-only recorders were removed and these two kept, with the test trimmed to match. Keeping them is the only resolution consistent with §10.6, and the §10.6 delete-LAST boundary is where they finally go.FOLLOW-UP checklist (deferred)
publishMode:'best-effort'rejection is a distinct upstream check that survives the limits change; only then removerepublishBestEffort's 405 swallow and the competitor/alias/brand-URL best-effort paths. Left fully in place here (the twowrapPublishseams remain wired at identity for this).setUmMockQuota/__quotaseam is deliberately retained (unconsumed today) for this change — seetest/it/postgres/setup.js.ERROR_CODES.QUOTA_EXCEEDED,toQuotaExceededError,isMeteredQuota,quota-alerts.js, the two keptallocation-metrics.jsrecorders, and the elmo half) — delete LAST, only after the §10.7 canary records the new upstream behaviour per env. This step also retires the canary itself and the one canary-scoped transport method (transferWorkspaceResources) — NOTgetWorkspaceResources, whichelements.jscheckAccesskeeps alive — and must settle the durable re-check of the limits-unenforced premise (the manual canary is only the interim one) — see ADR-009.scripts/serenity-metered-405-canary.mjson dev, stage and prod with a live IMS token and a real throwaway sub-workspace id (manual HUMAN step — nothing schedules it) and record each result here before §10.6. This turns the no-carve premise from "vendor statement + single GM-migration observation" into per-env evidence, and it is the same run §10.6 needs regardless. Soak ("not the same week") is already met.SERENITY_DYNAMIC_ALLOCATION/SERENITY_BRAND_AI_CEILING_*from Vault (dx_mysticat/{env}/api-service) — not touched here.docs/decisions/009-remove-dormant-jit-allocator.mdas the superseding record for the removal (ADR-008's rejected "delete the allocator" alternative is now the adopted decision); reconciled the ADR-007 / ADR-008 headers and inline pointers anddocs/serenity.md(including its incorrect listing of the trimmedallocation-metrics.jsas deleted).docs/runbooks/serenity-zombie-workspace-recovery.mdwas deleted with the allocator.