From cb9b54ce1715e44e0efcb1f3f0e89ddd44adb79e Mon Sep 17 00:00:00 2001 From: Gregor Jarisch Date: Tue, 21 Jul 2026 13:42:59 +0200 Subject: [PATCH 01/12] fix(admin): walk all descriptor pages and refuse to purge on an incomplete scan MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit readAllDescriptors advanced its cursor with `index += batch.size()`, but DescriptorStore.readDescriptors treats that argument as a PAGE index (skip = index * limit). The second iteration asked for skip=40000, always came back empty, and every store type was silently truncated at 200 rows. The dangerous half was buildReferencedUrisSet: the *referenced* set was truncated the same way, so on a deployment with >200 agents or >200 workflows, live in-use resources were classified as orphans — and purgeOrphans deletes with permanent=true (deleteAllPermanently, which removes the current document and all history). Separately, the reference scan failed open: read errors were swallowed (two at debug level) and the partial set returned as complete. Since that set is what protects a resource, each swallowed error made MORE things look orphaned. - readAllDescriptors: advance by page, bounded by MAX_PAGES; raise rather than truncate silently when the ceiling is hit - scanReferencedUris returns a ReferenceScan carrying a `complete` flag - purgeOrphans refuses with 409 when the scan is incomplete, naming the cause; scanOrphans still returns its best-effort read-only report - ResourceNotFoundException is not incompleteness — a descriptor whose resource is gone is a genuine orphan includeDeleted semantics are deliberately left alone here: with the page walk now complete, redefining that flag without also flipping its `true` default would turn the default DELETE from "purge <=200 already-soft- deleted rows" into "permanently wipe every unreferenced resource". --- docs/changelog.md | 28 +++ .../configs/admin/rest/RestOrphanAdmin.java | 100 ++++++-- .../admin/rest/RestOrphanAdminSafetyTest.java | 231 ++++++++++++++++++ 3 files changed, 343 insertions(+), 16 deletions(-) create mode 100644 src/test/java/ai/labs/eddi/configs/admin/rest/RestOrphanAdminSafetyTest.java diff --git a/docs/changelog.md b/docs/changelog.md index c400e51ad2..a7d80d2fa8 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -3,6 +3,34 @@ > **Purpose:** Living document tracking all changes, decisions, and reasoning during implementation. Updated as work progresses for easy reference and review. +--- + +## 🛡️ Orphan admin — fix page-walk truncation and refuse to purge on an incomplete scan (2026-07-21) + +**Repo:** EDDI (`claude/eddi-backend-manager-coverage-0598fe`) + +Two defects in `RestOrphanAdmin` that together could permanently delete live configuration. + +**1. The descriptor page walk never advanced past page 0.** `readAllDescriptors` advanced its cursor with `index += batch.size()`, but `DescriptorStore.readDescriptors` treats that argument as a *page* index (`skip = index * effectiveLimit`, `DescriptorStore.java:61`). The second iteration therefore asked for page 200 — `skip = 40 000` — which always came back empty, so every store type was silently truncated at 200 rows. Fixed to `pageIndex++`. + +The dangerous half of this was not the orphan list but `buildReferencedUrisSet`: the *referenced* set was truncated the same way, so on any deployment with more than 200 agents or 200 workflows, live in-use resources were classified as orphans — and `purgeOrphans` deletes with `permanent=true`, which is `deleteAllPermanently(id)` (current document *and* all history). + +A `MAX_PAGES` ceiling (100 pages / 20 000 rows per type) now bounds the walk, since the scan is a synchronous one-read-per-descriptor traversal on a blocking JAX-RS method. Hitting the ceiling raises `ResourceStoreException` rather than truncating silently — a truncated scan must not be used to decide what to delete. + +**2. The reference scan failed open.** Every read error while building the referenced-URI set was swallowed (two at `debug` level), and the partially-built set was returned as if complete. Because that set is what *protects* a resource, each swallowed error made **more** things look orphaned. One unreadable workflow could mark every rules/apicalls/output/llm/property/dictionary/parser resource in the database as an orphan. + +`scanReferencedUris()` now returns a `ReferenceScan` record carrying a `complete` flag, and `purgeOrphans` refuses with **409 Conflict** when the scan is incomplete, naming the cause. `scanOrphans` (read-only) still returns its best-effort report — only the irreversible path is gated. A `ResourceNotFoundException` is explicitly *not* treated as incompleteness: a descriptor whose resource is gone is a genuine orphan. + +### Design decisions + +- **Fail closed on the destructive path only.** Read-only callers tolerate a partial picture; a permanent delete may not. +- **Ceiling raises rather than truncates.** Silent truncation is what made the original bug invisible. +- **No change to `includeDeleted` semantics in this commit.** It is currently an equality filter (`Filters.eq("deleted", flag)`), so `scanOrphans` (defaults to `false`) and `purgeOrphans` (defaults to `true`) operate on *disjoint* sets. Fixing that is a real behaviour change to an irreversible endpoint and is deliberately left to its own commit — with the page-walk now complete, redefining the flag without also flipping the `true` default would turn the default `DELETE /administration/orphans` from "purge ≤200 already-soft-deleted rows" into "permanently wipe every unreferenced resource, unbounded". + +### Tests + +New `RestOrphanAdminSafetyTest` (7 tests): page index advances by 1 and a second page is actually requested; walk stops on the first partial page; an unreadable Agent aborts the purge with 409 and deletes nothing; a *missing* Agent resource does not abort; a complete scan purges normally; `scanOrphans` tolerates an incomplete reference set and never deletes; a workflow referenced by an Agent is never purged. Fixtures use 24-char hex ids because `RestUtilities.extractResourceId` returns a null id otherwise, which would make the assertions pass vacuously. All 27 pre-existing orphan tests unchanged and green. + --- ## 🔎 Auto-approve workflow — Copilot pagination review comment is a false positive (2026-07-20) diff --git a/src/main/java/ai/labs/eddi/configs/admin/rest/RestOrphanAdmin.java b/src/main/java/ai/labs/eddi/configs/admin/rest/RestOrphanAdmin.java index 04f7e721d0..80f4bac68b 100644 --- a/src/main/java/ai/labs/eddi/configs/admin/rest/RestOrphanAdmin.java +++ b/src/main/java/ai/labs/eddi/configs/admin/rest/RestOrphanAdmin.java @@ -19,6 +19,8 @@ import ai.labs.eddi.utils.RestUtilities; import jakarta.enterprise.context.ApplicationScoped; import jakarta.inject.Inject; +import jakarta.ws.rs.WebApplicationException; +import jakarta.ws.rs.core.Response; import org.jboss.logging.Logger; import java.net.URI; @@ -59,6 +61,15 @@ public class RestOrphanAdmin implements IRestOrphanAdmin { private static final int BATCH_SIZE = 200; + /** + * Hard ceiling on pages read per store type. The scan is a synchronous, + * one-read-per-descriptor traversal, so an unbounded walk would hold a worker + * thread for the whole collection. Hitting the ceiling is reported as an error + * rather than silently truncating — a truncated scan cannot be used to decide + * what to delete. + */ + private static final int MAX_PAGES = 100; + private final IAgentStore agentStore; private final IWorkflowStore workflowStore; private final IDocumentDescriptorStore documentDescriptorStore; @@ -81,7 +92,20 @@ public OrphanReport scanOrphans(Boolean includeDeleted) { @Override public OrphanReport purgeOrphans(Boolean includeDeleted) { - List orphans = findOrphans(includeDeleted); + // The reference set decides what is NOT an orphan. Every way of building it + // incompletely — a failed store read, a truncated page walk — makes MORE + // resources look unreferenced, and the delete below is permanent and + // irreversible. So an incomplete scan must refuse to purge rather than + // proceed on a partial picture. + ReferenceScan scan = scanReferencedUris(); + if (!scan.complete()) { + log.errorf("Refusing to purge orphans: the reference scan was incomplete (%s)", scan.failureReason()); + throw new WebApplicationException("Refusing to purge orphans: the reference scan was incomplete (" + scan.failureReason() + + "). Purging on a partial reference set could permanently delete resources that are still in use.", + Response.Status.CONFLICT); + } + + List orphans = collectOrphans(scan.referencedUris(), includeDeleted); int deletedCount = 0; for (OrphanInfo orphan : orphans) { @@ -98,12 +122,27 @@ public OrphanReport purgeOrphans(Boolean includeDeleted) { return new OrphanReport(orphans.size(), deletedCount, orphans); } + /** + * Outcome of building the referenced-URI set. + * + * @param referencedUris + * every URI referenced by at least one Agent or workflow + * @param complete + * false when any part of the traversal failed, meaning the set may + * be missing references and is therefore unsafe to delete against + * @param failureReason + * human-readable cause when {@code complete} is false + */ + private record ReferenceScan(Set referencedUris, boolean complete, String failureReason) { + } + private List findOrphans(boolean includeDeleted) { - // Step 1: Build the set of all referenced resource URIs - Set referencedUris = buildReferencedUrisSet(); + return collectOrphans(scanReferencedUris().referencedUris(), includeDeleted); + } + + private List collectOrphans(Set referencedUris, boolean includeDeleted) { log.infof("Orphan scan: found %d referenced resource URIs", referencedUris.size()); - // Step 2: Scan all store types and find unreferenced resources List orphans = new ArrayList<>(); for (String[] storeType : SCANNABLE_STORE_TYPES) { @@ -128,11 +167,18 @@ private List findOrphans(boolean includeDeleted) { } /** - * Build the complete set of all URIs that are referenced by at least one Agent - * or workflow. + * Build the set of all URIs that are referenced by at least one Agent or + * workflow, tracking whether the traversal completed. + * + *

+ * Completeness is reported rather than assumed: every failure here removes + * entries from the set, and a missing entry promotes a live resource to + * "orphan". Read-only callers may use a partial set; the purge may not. + *

*/ - private Set buildReferencedUrisSet() { + private ReferenceScan scanReferencedUris() { Set referencedUris = new HashSet<>(); + String failureReason = null; try { // Step 1a: Get all agents and collect their workflow URIs @@ -150,9 +196,11 @@ private Set buildReferencedUrisSet() { } } } catch (IResourceStore.ResourceNotFoundException e) { - // Agent descriptor exists but resource doesn't — skip + // Agent descriptor exists but resource doesn't — genuinely + // unreferenced, so this does not make the scan incomplete. } catch (Exception e) { - log.debugf("Error reading Agent %s: %s", agentDescriptor.getResource(), e.getMessage()); + log.warnf("Error reading Agent %s: %s", agentDescriptor.getResource(), e.getMessage()); + failureReason = "could not read Agent " + agentDescriptor.getResource() + ": " + e.getMessage(); } } @@ -167,17 +215,20 @@ private Set buildReferencedUrisSet() { WorkflowConfiguration workflowConfig = workflowStore.read(resourceId.getId(), resourceId.getVersion()); collectExtensionUris(workflowConfig, referencedUris); } catch (IResourceStore.ResourceNotFoundException e) { - // Workflow descriptor exists but resource doesn't — skip + // Workflow descriptor exists but resource doesn't — genuinely + // unreferenced, so this does not make the scan incomplete. } catch (Exception e) { - log.debugf("Error reading workflow %s: %s", workflowDescriptor.getResource(), e.getMessage()); + log.warnf("Error reading workflow %s: %s", workflowDescriptor.getResource(), e.getMessage()); + failureReason = "could not read workflow " + workflowDescriptor.getResource() + ": " + e.getMessage(); } } } catch (Exception e) { log.errorf("Error building referenced URIs set: %s", e.getMessage()); + failureReason = "could not enumerate Agent/workflow descriptors: " + e.getMessage(); } - return referencedUris; + return new ReferenceScan(referencedUris, failureReason == null, failureReason); } /** @@ -218,18 +269,35 @@ private void collectExtensionUris(WorkflowConfiguration workflowConfig, Set + * {@code index} is a PAGE index, not a row offset — + * {@link ai.labs.eddi.datastore.DescriptorStore#readDescriptors} computes + * {@code skip = index * limit}. Advancing it by {@code batch.size()} asked for + * page 200 (skip = 40 000) on the second iteration, which always came back + * empty, so every type was silently truncated at {@value #BATCH_SIZE} rows. + * That truncation also hit {@link #buildReferencedUrisSet()}, where a missing + * reference makes a live resource look unreferenced. + *

*/ private List readAllDescriptors(String type, boolean includeDeleted) throws IResourceStore.ResourceStoreException, IResourceStore.ResourceNotFoundException { List all = new ArrayList<>(); - int index = 0; + int pageIndex = 0; List batch; do { - batch = documentDescriptorStore.readDescriptors(type, "", index, BATCH_SIZE, includeDeleted); + batch = documentDescriptorStore.readDescriptors(type, "", pageIndex, BATCH_SIZE, includeDeleted); all.addAll(batch); - index += batch.size(); - } while (batch.size() == BATCH_SIZE); + pageIndex++; + } while (batch.size() == BATCH_SIZE && pageIndex < MAX_PAGES); + + if (batch.size() == BATCH_SIZE) { + log.warnf("Descriptor scan for type %s hit the %d-page ceiling (%d rows); results are incomplete", type, MAX_PAGES, + all.size()); + throw new IResourceStore.ResourceStoreException( + "Descriptor scan for type " + type + " exceeded " + (MAX_PAGES * BATCH_SIZE) + " rows"); + } return all; } diff --git a/src/test/java/ai/labs/eddi/configs/admin/rest/RestOrphanAdminSafetyTest.java b/src/test/java/ai/labs/eddi/configs/admin/rest/RestOrphanAdminSafetyTest.java new file mode 100644 index 0000000000..6320044e7c --- /dev/null +++ b/src/test/java/ai/labs/eddi/configs/admin/rest/RestOrphanAdminSafetyTest.java @@ -0,0 +1,231 @@ +/* + * Copyright EDDI contributors + * SPDX-License-Identifier: Apache-2.0 + */ +package ai.labs.eddi.configs.admin.rest; + +import ai.labs.eddi.configs.admin.model.OrphanReport; +import ai.labs.eddi.configs.agents.IAgentStore; +import ai.labs.eddi.configs.agents.model.AgentConfiguration; +import ai.labs.eddi.configs.descriptors.IDocumentDescriptorStore; +import ai.labs.eddi.configs.descriptors.model.DocumentDescriptor; +import ai.labs.eddi.configs.workflows.IWorkflowStore; +import ai.labs.eddi.configs.workflows.model.WorkflowConfiguration; +import ai.labs.eddi.datastore.IResourceStore; +import ai.labs.eddi.engine.runtime.client.configuration.IResourceClientLibrary; +import jakarta.ws.rs.WebApplicationException; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Nested; +import org.junit.jupiter.api.Test; +import org.mockito.Mock; + +import java.net.URI; +import java.util.ArrayList; +import java.util.List; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyBoolean; +import static org.mockito.ArgumentMatchers.anyInt; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; +import static org.mockito.MockitoAnnotations.openMocks; + +/** + * Safety behaviour of the orphan admin endpoint. + * + *

+ * Two properties are pinned here because getting either wrong permanently + * destroys configuration: + *

+ *
    + *
  1. the descriptor page walk must actually visit every page — a truncated + * walk leaves references out of the "referenced" set, which promotes live + * resources to "orphan"
    2. + *
  2. the purge must refuse to run on an incomplete reference scan
    3. + *
+ * + *

+ * Resource ids are 24-char hex because {@code RestUtilities.extractResourceId} + * returns a null id for anything shorter or non-hex, which would make these + * fixtures pass vacuously. + *

+ */ +@DisplayName("RestOrphanAdmin — scan completeness and purge safety") +class RestOrphanAdminSafetyTest { + + private static final int BATCH_SIZE = 200; + private static final String AGENT_ID = "aabbccddeeff112233445566"; + private static final URI AGENT_URI = URI.create("eddi://ai.labs.agent/agentstore/agents/" + AGENT_ID + "?version=1"); + + @Mock + private IAgentStore agentStore; + @Mock + private IWorkflowStore workflowStore; + @Mock + private IDocumentDescriptorStore documentDescriptorStore; + @Mock + private IResourceClientLibrary resourceClientLibrary; + + private RestOrphanAdmin restOrphanAdmin; + + @BeforeEach + void setUp() { + openMocks(this); + restOrphanAdmin = new RestOrphanAdmin(agentStore, workflowStore, documentDescriptorStore, resourceClientLibrary); + } + + private static DocumentDescriptor descriptor(URI resource, String name) { + DocumentDescriptor descriptor = new DocumentDescriptor(); + descriptor.setResource(resource); + descriptor.setName(name); + return descriptor; + } + + private static List fullPage(String type) { + List page = new ArrayList<>(BATCH_SIZE); + for (int i = 0; i < BATCH_SIZE; i++) { + page.add(descriptor(URI.create("eddi://" + type + "/store/items/" + String.format("%024x", i) + "?version=1"), "d" + i)); + } + return page; + } + + @Nested + @DisplayName("descriptor paging") + class Paging { + + @Test + @DisplayName("advances by PAGE index, so a second page is actually requested") + void walksEveryPage() throws Exception { + when(documentDescriptorStore.readDescriptors(anyString(), anyString(), anyInt(), anyInt(), anyBoolean())) + .thenReturn(List.of()); + when(documentDescriptorStore.readDescriptors(eq("ai.labs.rules"), anyString(), eq(0), eq(BATCH_SIZE), anyBoolean())) + .thenReturn(fullPage("ai.labs.rules")); + when(documentDescriptorStore.readDescriptors(eq("ai.labs.rules"), anyString(), eq(1), eq(BATCH_SIZE), anyBoolean())) + .thenReturn(List.of(descriptor(URI.create("eddi://ai.labs.rules/rulestore/rulesets/ffffffffffffffffffffffff?version=1"), + "page-two-item"))); + + OrphanReport report = restOrphanAdmin.scanOrphans(false); + + // Page 1 must have been requested with index == 1 (not 200, which is what + // advancing by batch size produced and which always returned empty). + verify(documentDescriptorStore).readDescriptors(eq("ai.labs.rules"), anyString(), eq(1), eq(BATCH_SIZE), anyBoolean()); + assertEquals(BATCH_SIZE + 1, report.getTotalOrphans(), "both pages should contribute orphans"); + assertTrue(report.getOrphans().stream().anyMatch(o -> "page-two-item".equals(o.getName())), + "the second page's descriptor must appear in the report"); + } + + @Test + @DisplayName("stops on the first partial page") + void stopsOnPartialPage() throws Exception { + when(documentDescriptorStore.readDescriptors(anyString(), anyString(), anyInt(), anyInt(), anyBoolean())) + .thenReturn(List.of()); + + restOrphanAdmin.scanOrphans(false); + + verify(documentDescriptorStore, never()).readDescriptors(anyString(), anyString(), eq(1), anyInt(), anyBoolean()); + } + } + + @Nested + @DisplayName("purge refuses an incomplete reference scan") + class PurgeSafety { + + @Test + @DisplayName("an unreadable Agent aborts the purge with 409 and deletes nothing") + void unreadableAgentAbortsPurge() throws Exception { + when(documentDescriptorStore.readDescriptors(anyString(), anyString(), anyInt(), anyInt(), anyBoolean())) + .thenReturn(List.of()); + when(documentDescriptorStore.readDescriptors(eq("ai.labs.agent"), anyString(), eq(0), anyInt(), anyBoolean())) + .thenReturn(List.of(descriptor(AGENT_URI, "broken-agent"))); + when(agentStore.read(eq(AGENT_ID), any())).thenThrow(new IResourceStore.ResourceStoreException("mongo down")); + + WebApplicationException thrown = assertThrows(WebApplicationException.class, () -> restOrphanAdmin.purgeOrphans(true)); + + assertEquals(409, thrown.getResponse().getStatus()); + verify(resourceClientLibrary, never()).deleteResource(any(), anyBoolean()); + } + + @Test + @DisplayName("a missing Agent resource is a real orphan, not a scan failure — purge proceeds") + void missingAgentDoesNotAbortPurge() throws Exception { + when(documentDescriptorStore.readDescriptors(anyString(), anyString(), anyInt(), anyInt(), anyBoolean())) + .thenReturn(List.of()); + when(documentDescriptorStore.readDescriptors(eq("ai.labs.agent"), anyString(), eq(0), anyInt(), anyBoolean())) + .thenReturn(List.of(descriptor(AGENT_URI, "deleted-agent"))); + when(agentStore.read(eq(AGENT_ID), any())).thenThrow(new IResourceStore.ResourceNotFoundException("gone")); + + OrphanReport report = restOrphanAdmin.purgeOrphans(true); + + assertEquals(0, report.getTotalOrphans()); + } + + @Test + @DisplayName("a complete scan purges normally") + void completeScanPurges() throws Exception { + URI orphan = URI.create("eddi://ai.labs.rules/rulestore/rulesets/aabbccddeeff112233445568?version=1"); + when(documentDescriptorStore.readDescriptors(anyString(), anyString(), anyInt(), anyInt(), anyBoolean())) + .thenReturn(List.of()); + when(documentDescriptorStore.readDescriptors(eq("ai.labs.rules"), anyString(), eq(0), anyInt(), anyBoolean())) + .thenReturn(List.of(descriptor(orphan, "unused-ruleset"))); + + OrphanReport report = restOrphanAdmin.purgeOrphans(true); + + assertEquals(1, report.getTotalOrphans()); + assertEquals(1, report.getDeletedCount()); + verify(resourceClientLibrary).deleteResource(orphan, true); + } + + @Test + @DisplayName("scanOrphans still returns a report when the reference scan is incomplete") + void scanToleratesIncompleteReferenceSet() throws Exception { + when(documentDescriptorStore.readDescriptors(anyString(), anyString(), anyInt(), anyInt(), anyBoolean())) + .thenReturn(List.of()); + when(documentDescriptorStore.readDescriptors(eq("ai.labs.agent"), anyString(), eq(0), anyInt(), anyBoolean())) + .thenReturn(List.of(descriptor(AGENT_URI, "broken-agent"))); + when(agentStore.read(eq(AGENT_ID), any())).thenThrow(new IResourceStore.ResourceStoreException("mongo down")); + + OrphanReport report = restOrphanAdmin.scanOrphans(false); + + assertEquals(0, report.getDeletedCount(), "a scan must never delete"); + verify(resourceClientLibrary, never()).deleteResource(any(), anyBoolean()); + } + } + + @Nested + @DisplayName("referenced resources are protected") + class ReferenceProtection { + + @Test + @DisplayName("a workflow referenced by an Agent is not reported as an orphan") + void referencedWorkflowIsNotOrphan() throws Exception { + URI workflowUri = URI.create("eddi://ai.labs.workflow/workflowstore/workflows/aabbccddeeff112233445569?version=1"); + + AgentConfiguration agentConfig = new AgentConfiguration(); + agentConfig.setWorkflows(List.of(workflowUri)); + + WorkflowConfiguration workflowConfig = new WorkflowConfiguration(); + workflowConfig.setWorkflowSteps(List.of()); + + when(documentDescriptorStore.readDescriptors(anyString(), anyString(), anyInt(), anyInt(), anyBoolean())) + .thenReturn(List.of()); + when(documentDescriptorStore.readDescriptors(eq("ai.labs.agent"), anyString(), eq(0), anyInt(), anyBoolean())) + .thenReturn(List.of(descriptor(AGENT_URI, "live-agent"))); + when(documentDescriptorStore.readDescriptors(eq("ai.labs.workflow"), anyString(), eq(0), anyInt(), anyBoolean())) + .thenReturn(List.of(descriptor(workflowUri, "live-workflow"))); + when(agentStore.read(eq(AGENT_ID), any())).thenReturn(agentConfig); + when(workflowStore.read(eq("aabbccddeeff112233445569"), any())).thenReturn(workflowConfig); + + OrphanReport report = restOrphanAdmin.purgeOrphans(true); + + assertEquals(0, report.getTotalOrphans(), "a referenced workflow must never be purged"); + verify(resourceClientLibrary, never()).deleteResource(any(), anyBoolean()); + } + } +} From 7641f60f3b51291587db6fff7eaf46ac0344bd78 Mon Sep 17 00:00:00 2001 From: Gregor Jarisch Date: Tue, 21 Jul 2026 13:52:17 +0200 Subject: [PATCH 02/12] fix(tenancy): align at-limit cost comparison and fix costMonth JSON shape MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two independent defects on the tenant cost-budget surface. 1. TenantQuotaService.checkCostBudget denies on `currentCost >= limit` and InMemoryTenantQuotaStore.tryAddCost matches it, but MongoTenantQuotaStore and PostgresTenantQuotaStore used `totalCost > limit`. At exactly the budget the pre-call gate denied while post-call accounting allowed. The in-memory store was deliberately moved to >= for this reason; the two production stores were never updated. 2. UsageSnapshot.costMonth serialized as the array [2026,7] instead of "2026-07": under quarkus.jackson.write-dates-as-timestamps=true Jackson's YearMonthSerializer takes its useTimestamp branch. Both stores already persist the value as an ISO string via YearMonth.toString()/parse and never route it through Jackson, so only the REST shape disagreed. Deliberately NOT done here: a mapper-wide configOverride(Instant.class). SerializationCustomizer's mapper is also the persistence mapper — JsonSerialization injects the same CDI ObjectMapper, which backs DocumentBuilder for every Mongo write and Postgres JSONB column. It would change on-disk formats; GroupConversation.lastModified is an Instant that GroupConversationStore sorts server-side on, so mixed numeric/string rows would sort wrongly and silently. Commit dc117cddc already reverted a broader version of that change for breaking findDueSchedules. The new exactlyAtLimit test was mutation-checked: reverting the Mongo comparison to `>` makes it fail, so it is not vacuous. --- docs/changelog.md | 23 +++++++ .../engine/tenancy/MongoTenantQuotaStore.java | 5 +- .../tenancy/PostgresTenantQuotaStore.java | 6 +- .../engine/tenancy/model/UsageSnapshot.java | 9 ++- .../tenancy/MongoTenantQuotaStoreTest.java | 13 ++++ .../UsageSnapshotSerializationTest.java | 67 +++++++++++++++++++ 6 files changed, 120 insertions(+), 3 deletions(-) create mode 100644 src/test/java/ai/labs/eddi/engine/tenancy/UsageSnapshotSerializationTest.java diff --git a/docs/changelog.md b/docs/changelog.md index a7d80d2fa8..cbf6690145 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -3,6 +3,29 @@ > **Purpose:** Living document tracking all changes, decisions, and reasoning during implementation. Updated as work progresses for easy reference and review. +--- + +## 💵 Tenancy — align the at-limit cost comparison and fix costMonth JSON shape (2026-07-21) + +**Repo:** EDDI (`claude/eddi-backend-manager-coverage-0598fe`) + +Two independent defects in the tenant cost-budget surface. + +**1. The two production quota stores disagreed with the gate at exactly the limit.** `TenantQuotaService.checkCostBudget` denies on `currentCost >= limit`, and `InMemoryTenantQuotaStore.tryAddCost` matches it — but `MongoTenantQuotaStore` and `PostgresTenantQuotaStore` used `totalCost > limit`. At exactly the budget the pre-call gate denied while post-call accounting allowed. `docs/changelog.md` records the in-memory store being deliberately moved to `>=` for this reason; the two DB stores were never updated. Both now use `>=`. + +Verified by mutation: reverting the Mongo comparison to `>` makes the new `exactlyAtLimit` test fail with `expected: but was: `, so the test is not vacuous. + +**2. `UsageSnapshot.costMonth` serialized as a JSON array.** Under `quarkus.jackson.write-dates-as-timestamps=true` (`application.properties:174`) Jackson's `YearMonthSerializer` takes its `useTimestamp` branch and emits `[2026,7]` instead of `"2026-07"`. Both stores already persist the value as an ISO string (`YearMonth.toString()` / `YearMonth.parse`, never through Jackson), so the REST representation was the only place that disagreed. Annotated the record component with `@JsonFormat(shape = JsonFormat.Shape.STRING)`. + +### Design decisions + +- **Annotation, not a mapper-wide override.** A `configOverride(Instant.class)` on `SerializationCustomizer` would have fixed every temporal field at once, but that customizer's mapper is *also* the persistence mapper — `JsonSerialization` `@Inject`s the same CDI `ObjectMapper`, which backs `DocumentBuilder` for every Mongo write and Postgres JSONB column. Changing it would alter on-disk formats. Concretely, `GroupConversation.lastModified` is an `Instant` persisted through it and `GroupConversationStore` sorts *server-side* on that field, so mixed old-numeric/new-string rows would sort wrongly and silently in both backends. Commit `dc117cddc` ("keep numeric date format") already reverted a broader version of this change once, for breaking `findDueSchedules`. The wider Instant-format cleanup is therefore left out and tracked separately — it needs the persistence mapper decoupled from the REST mapper first, plus a coordinated EDDI-Manager change (the Schedules dashboard sorts `nextFire` arithmetically, which yields `NaN` on ISO strings). +- **`costMonth` is safe in isolation** precisely because neither store round-trips `YearMonth` through Jackson — verified before changing it. + +### Tests + +New `UsageSnapshotSerializationTest` asserts the shape on the *real* record (not a stand-in holder) through a mapper wired exactly as production is, plus a round-trip. New `MongoTenantQuotaStoreTest.exactlyAtLimit`. All 131 tenancy tests green. + --- ## 🛡️ Orphan admin — fix page-walk truncation and refuse to purge on an incomplete scan (2026-07-21) diff --git a/src/main/java/ai/labs/eddi/engine/tenancy/MongoTenantQuotaStore.java b/src/main/java/ai/labs/eddi/engine/tenancy/MongoTenantQuotaStore.java index 02e97952b5..2f0d070323 100644 --- a/src/main/java/ai/labs/eddi/engine/tenancy/MongoTenantQuotaStore.java +++ b/src/main/java/ai/labs/eddi/engine/tenancy/MongoTenantQuotaStore.java @@ -251,7 +251,10 @@ public QuotaCheckResult tryAddCost(String tenantId, double cost, double limit) { } double totalCost = result.getDouble("monthlyCostUsd"); - if (limit >= 0 && totalCost > limit) { + // >=, not >, to agree with TenantQuotaService.checkCostBudget (currentCost >= + // limit) and InMemoryTenantQuotaStore. With > the pre-call gate denied at + // exactly the limit while post-call accounting allowed. + if (limit >= 0 && totalCost >= limit) { return QuotaCheckResult.denied( "Monthly cost budget exceeded ($%.2f / $%.2f)".formatted(totalCost, limit)); } diff --git a/src/main/java/ai/labs/eddi/engine/tenancy/PostgresTenantQuotaStore.java b/src/main/java/ai/labs/eddi/engine/tenancy/PostgresTenantQuotaStore.java index a9ea2fab9a..50c0f9f382 100644 --- a/src/main/java/ai/labs/eddi/engine/tenancy/PostgresTenantQuotaStore.java +++ b/src/main/java/ai/labs/eddi/engine/tenancy/PostgresTenantQuotaStore.java @@ -416,7 +416,11 @@ ON CONFLICT (tenant_id) DO UPDATE SET try (ResultSet rs = ps.executeQuery()) { if (rs.next()) { double totalCost = rs.getDouble(1); - if (limit >= 0 && totalCost > limit) { + // >=, not >, to agree with TenantQuotaService.checkCostBudget + // (currentCost >= limit) and InMemoryTenantQuotaStore. With > the + // pre-call gate denied at exactly the limit while post-call + // accounting allowed. + if (limit >= 0 && totalCost >= limit) { return QuotaCheckResult.denied( "Monthly cost budget exceeded ($%.2f / $%.2f)".formatted(totalCost, limit)); } diff --git a/src/main/java/ai/labs/eddi/engine/tenancy/model/UsageSnapshot.java b/src/main/java/ai/labs/eddi/engine/tenancy/model/UsageSnapshot.java index a16e64740d..a45c516961 100644 --- a/src/main/java/ai/labs/eddi/engine/tenancy/model/UsageSnapshot.java +++ b/src/main/java/ai/labs/eddi/engine/tenancy/model/UsageSnapshot.java @@ -4,6 +4,8 @@ */ package ai.labs.eddi.engine.tenancy.model; +import com.fasterxml.jackson.annotation.JsonFormat; + import java.time.Instant; import java.time.YearMonth; import java.time.ZoneOffset; @@ -34,7 +36,12 @@ public record UsageSnapshot( double monthlyCostUsd, Instant minuteWindowStart, Instant dayStart, - YearMonth costMonth) { + // Without an explicit shape, Jackson's YearMonthSerializer takes the + // useTimestamp branch under quarkus.jackson.write-dates-as-timestamps=true + // and emits the ARRAY [2026,7] rather than "2026-07". Both quota stores + // already persist this as an ISO string (YearMonth.toString() / + // YearMonth.parse), so only the REST representation disagreed. + @JsonFormat(shape = JsonFormat.Shape.STRING) YearMonth costMonth) { /** * Creates an empty snapshot (zero counters) for an unknown tenant. diff --git a/src/test/java/ai/labs/eddi/engine/tenancy/MongoTenantQuotaStoreTest.java b/src/test/java/ai/labs/eddi/engine/tenancy/MongoTenantQuotaStoreTest.java index 6a7eb5ed71..8a0658f951 100644 --- a/src/test/java/ai/labs/eddi/engine/tenancy/MongoTenantQuotaStoreTest.java +++ b/src/test/java/ai/labs/eddi/engine/tenancy/MongoTenantQuotaStoreTest.java @@ -333,6 +333,19 @@ void withinBudget() { assertTrue(qResult.allowed()); } + @Test + @DisplayName("should return denied at exactly the limit, matching checkCostBudget") + void exactlyAtLimit() { + Document result = new Document("monthlyCostUsd", 100.0); + when(usageCollection.findOneAndUpdate(any(Bson.class), any(Bson.class), any())).thenReturn(result); + + QuotaCheckResult qResult = sut.tryAddCost(TENANT_ID, 10.0, 100.0); + + // TenantQuotaService.checkCostBudget denies on `currentCost >= limit`, so + // post-call accounting must use >= too or the two disagree at the boundary. + assertFalse(qResult.allowed(), "at exactly the limit the budget is spent"); + } + @Test @DisplayName("should return denied when budget exceeded") void overBudget() { diff --git a/src/test/java/ai/labs/eddi/engine/tenancy/UsageSnapshotSerializationTest.java b/src/test/java/ai/labs/eddi/engine/tenancy/UsageSnapshotSerializationTest.java new file mode 100644 index 0000000000..3dc4c5abe6 --- /dev/null +++ b/src/test/java/ai/labs/eddi/engine/tenancy/UsageSnapshotSerializationTest.java @@ -0,0 +1,67 @@ +/* + * Copyright EDDI contributors + * SPDX-License-Identifier: Apache-2.0 + */ +package ai.labs.eddi.engine.tenancy; + +import ai.labs.eddi.datastore.serialization.SerializationCustomizer; +import ai.labs.eddi.engine.tenancy.model.UsageSnapshot; +import com.fasterxml.jackson.databind.ObjectMapper; +import com.fasterxml.jackson.databind.SerializationFeature; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; + +import java.time.Instant; +import java.time.YearMonth; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * Pins the JSON wire shape of {@link UsageSnapshot#costMonth()}. + * + *

+ * The deployment sets {@code quarkus.jackson.write-dates-as-timestamps=true} + * (application.properties). Under that flag Jackson's + * {@code YearMonthSerializer} takes its {@code useTimestamp} branch and writes + * a two-element ARRAY — {@code [2026,7]} — instead of {@code "2026-07"}. Both + * quota stores already persist the value as an ISO string, so the REST + * representation was the only place that disagreed. + *

+ */ +@DisplayName("UsageSnapshot — REST serialization") +class UsageSnapshotSerializationTest { + + /** + * Mirrors the production mapper: the shared customizer plus the + * write-dates-as-timestamps flag Quarkus applies from application.properties. + */ + private static ObjectMapper productionMapper() { + ObjectMapper mapper = SerializationCustomizer.configureObjectMapper(new ObjectMapper(), false); + mapper.configure(SerializationFeature.WRITE_DATES_AS_TIMESTAMPS, true); + return mapper; + } + + @Test + @DisplayName("costMonth serializes as \"YYYY-MM\", not the array [YYYY,M]") + void costMonthIsIsoString() throws Exception { + UsageSnapshot snapshot = new UsageSnapshot("t1", 3, 7, 12.5, Instant.ofEpochMilli(1719964800123L), + Instant.ofEpochMilli(1719964800123L), YearMonth.of(2026, 7)); + + String json = productionMapper().writeValueAsString(snapshot); + + assertTrue(json.contains("\"costMonth\":\"2026-07\""), "expected an ISO year-month string, got: " + json); + } + + @Test + @DisplayName("costMonth round-trips through the REST mapper") + void costMonthRoundTrips() throws Exception { + ObjectMapper mapper = productionMapper(); + UsageSnapshot original = UsageSnapshot.empty("t1"); + + UsageSnapshot parsed = mapper.readValue(mapper.writeValueAsString(original), UsageSnapshot.class); + + assertEquals(original.costMonth(), parsed.costMonth()); + assertEquals(original.tenantId(), parsed.tenantId()); + } +} From e3fbf94c91d0a63e226c2f68c72d4bea5dbac39e Mon Sep 17 00:00:00 2001 From: Gregor Jarisch Date: Tue, 21 Jul 2026 13:59:37 +0200 Subject: [PATCH 03/12] fix(engine): return 429 instead of 500 when the api-call quota denies a turn MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ConversationService.say/sayStreaming throw QuotaExceededException when acquireApiCallSlot() denies, and QuotaExceededExceptionMapper maps that to 429 with {"error":"quota_exceeded"} and Retry-After: 60. But say() is resumed through a JAX-RS AsyncResponse, so the exception is caught inside RestAgentEngine.sayInternal and never reaches the @Provider mapper. Its catch chain did not list QuotaExceededException, so the denial fell into `catch (Exception e)` and surfaced as 500 "An internal error occurred". Only the conversation-start quota ever produced a real 429, because startConversationWithContext is synchronous and its narrower catch block lets the exception escape to the mapper. The per-minute API rate limit — the quota an operator is most likely to hit — was indistinguishable from a server fault, so clients could not back off correctly. Added an explicit catch that resumes with the same status, body and Retry-After header as the mapper. sayStreaming is deliberately not covered: by the time it throws, the SSE response has already committed HTTP 200, so no status can be sent. Giving its generic error event a distinguishable quota type is a separate, client-visible change. The new test was mutation-checked: replacing the catch with an unrelated exception type makes it fail with InternalServerError, reproducing the original bug. --- docs/changelog.md | 21 ++++++++++++++++ .../eddi/engine/internal/RestAgentEngine.java | 15 +++++++++++ .../engine/internal/RestAgentEngineTest.java | 25 +++++++++++++++++++ 3 files changed, 61 insertions(+) diff --git a/docs/changelog.md b/docs/changelog.md index cbf6690145..1561a1989c 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -3,6 +3,27 @@ > **Purpose:** Living document tracking all changes, decisions, and reasoning during implementation. Updated as work progresses for easy reference and review. +--- + +## 🚦 Engine — return 429 instead of 500 when the api-call quota denies a turn (2026-07-21) + +**Repo:** EDDI (`claude/eddi-backend-manager-coverage-0598fe`) + +`ConversationService.say`/`sayStreaming` throw `QuotaExceededException` when `acquireApiCallSlot()` denies, and `QuotaExceededExceptionMapper` maps that to **429** with `{"error":"quota_exceeded"}` and `Retry-After: 60`. But `say()` is resumed through a JAX-RS `AsyncResponse`, so the exception is caught inside `RestAgentEngine.sayInternal` and never reaches the `@Provider` mapper. Its catch chain lists `AgentMismatchException`, `AgentNotReadyException`, `ConversationEndedException`, `ConversationAwaitingApprovalException`, `ProcessingRestrictedException` and `ResourceNotFoundException` — but not `QuotaExceededException` — so the denial fell through to `catch (Exception e)` and surfaced as **500 "An internal error occurred"**. + +Only the *conversation-start* quota ever produced a real 429: `startConversationWithContext` is synchronous and its narrower catch block lets the exception escape to the mapper. The per-minute API rate limit — the quota an operator is most likely to actually hit — was indistinguishable from a server fault, so clients had no way to back off correctly. + +Added an explicit `catch (QuotaExceededException)` that resumes with the same status, body and `Retry-After` header as the mapper, so both quota denials look identical on the wire. + +### Design decisions + +- **Mirror the mapper rather than re-throw.** There is no way to route an already-captured async exception back through the provider chain, so the branch duplicates the mapper's three-line response. Kept adjacent constants and a comment so the two stay in sync. +- **Streaming is not covered here.** `sayStreaming` throws the same exception, but by then the SSE response has already committed HTTP 200, so no status code can be sent — it currently emits a generic `error` event. Giving that event a distinguishable quota type is a separate, client-visible change and is tracked, not slipped in. + +### Tests + +New `RestAgentEngineTest.quotaExceeded` asserts 429, the `Retry-After: 60` header and the exact entity map. Mutation-checked: replacing the new catch with an unrelated exception type makes it fail with `InternalServerError An internal error occurred` — reproducing the original bug — so the test is not vacuous. All 43 `RestAgentEngineTest` tests green. + --- ## 💵 Tenancy — align the at-limit cost comparison and fix costMonth JSON shape (2026-07-21) diff --git a/src/main/java/ai/labs/eddi/engine/internal/RestAgentEngine.java b/src/main/java/ai/labs/eddi/engine/internal/RestAgentEngine.java index 2d71c65f4b..692b663222 100644 --- a/src/main/java/ai/labs/eddi/engine/internal/RestAgentEngine.java +++ b/src/main/java/ai/labs/eddi/engine/internal/RestAgentEngine.java @@ -28,12 +28,14 @@ import ai.labs.eddi.engine.model.InputData; import ai.labs.eddi.engine.security.ConversationAccessGuard; import ai.labs.eddi.engine.security.OwnershipValidator; +import ai.labs.eddi.engine.tenancy.QuotaExceededException; import io.quarkus.security.identity.SecurityIdentity; import jakarta.enterprise.context.ApplicationScoped; import jakarta.inject.Inject; import jakarta.ws.rs.InternalServerErrorException; import jakarta.ws.rs.NotFoundException; import jakarta.ws.rs.container.AsyncResponse; +import jakarta.ws.rs.core.MediaType; import jakarta.ws.rs.core.Response; import org.eclipse.microprofile.config.inject.ConfigProperty; import org.jboss.logging.Logger; @@ -73,6 +75,9 @@ public class RestAgentEngine implements IRestAgentEngine { private static final Logger LOGGER = Logger.getLogger(RestAgentEngine.class); + /** Mirrors QuotaExceededExceptionMapper; jakarta.ws.rs has no 429 constant. */ + private static final int TOO_MANY_REQUESTS = 429; + @Inject public RestAgentEngine(IConversationService conversationService, IConversationMemoryStore conversationMemoryStore, @@ -232,6 +237,16 @@ public void onSkipped(SimpleConversationMemorySnapshot snapshot) { response.resume(Response.status(Response.Status.FORBIDDEN).type(TEXT_PLAIN).entity(e.getMessage()).build()); } catch (ResourceNotFoundException e) { response.resume(new NotFoundException()); + } catch (QuotaExceededException e) { + // Must be caught explicitly: say() is resumed through an AsyncResponse, so + // the exception never reaches QuotaExceededExceptionMapper — without this + // branch it fell through to the generic handler below and the api-call + // quota surfaced as a 500 instead of a 429. Body and headers mirror the + // mapper so both quota denials look identical to clients. + LOGGER.warnf("Quota exceeded for conversation %s: %s", conversationId, e.getMessage()); + response.resume(Response.status(TOO_MANY_REQUESTS) + .entity(Map.of("error", "quota_exceeded", "message", e.getMessage())) + .type(MediaType.APPLICATION_JSON).header("Retry-After", "60").build()); } catch (Exception e) { LOGGER.error(e.getLocalizedMessage(), e); throw new InternalServerErrorException("An internal error occurred"); diff --git a/src/test/java/ai/labs/eddi/engine/internal/RestAgentEngineTest.java b/src/test/java/ai/labs/eddi/engine/internal/RestAgentEngineTest.java index 6a8ead1875..978ca8fc33 100644 --- a/src/test/java/ai/labs/eddi/engine/internal/RestAgentEngineTest.java +++ b/src/test/java/ai/labs/eddi/engine/internal/RestAgentEngineTest.java @@ -18,6 +18,7 @@ import ai.labs.eddi.engine.model.InputData; import ai.labs.eddi.engine.security.ConversationAccessGuard; import ai.labs.eddi.engine.security.OwnershipValidator; +import ai.labs.eddi.engine.tenancy.QuotaExceededException; import io.quarkus.security.ForbiddenException; import io.quarkus.security.identity.SecurityIdentity; import jakarta.ws.rs.InternalServerErrorException; @@ -340,6 +341,30 @@ void gdprRestricted() throws Exception { assertEquals(403, captor.getValue().getStatus()); } + @Test + @DisplayName("should resume with 429 quota_exceeded, not 500, when the api-call quota denies") + void quotaExceeded() throws Exception { + var asyncResponse = mock(AsyncResponse.class); + var inputData = new InputData("Hello", Map.of()); + + doThrow(new QuotaExceededException("API rate limit (5/min) exceeded for tenant 'default'")) + .when(conversationService).say(anyString(), any(), any(), any(), any(), anyBoolean(), any()); + + restAgentEngine.sayWithinContext("conv-1", false, false, + List.of(), inputData, asyncResponse); + + // say() is resumed via AsyncResponse, so QuotaExceededExceptionMapper never + // runs — without an explicit catch this fell through to the generic handler + // and surfaced as a 500. + var captor = ArgumentCaptor.forClass(Response.class); + verify(asyncResponse).resume(captor.capture()); + Response resumed = captor.getValue(); + assertEquals(429, resumed.getStatus()); + assertEquals("60", resumed.getHeaderString("Retry-After")); + assertEquals(Map.of("error", "quota_exceeded", "message", "API rate limit (5/min) exceeded for tenant 'default'"), + resumed.getEntity()); + } + @Test @DisplayName("should resume with NotFoundException for resource not found") void resourceNotFound() throws Exception { From 8b7904012c24a29194380245f8e8622e9f0d143c Mon Sep 17 00:00:00 2001 From: Gregor Jarisch Date: Tue, 21 Jul 2026 14:11:25 +0200 Subject: [PATCH 04/12] docs(specs): record the manager-coverage backend design, findings and deferrals MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Captures what was verified against the code, what shipped, and — more importantly — what deliberately did not and why, so the deferred items are not re-attempted from the original (stale) spec. Notably records that the repo-wide Instant->ISO change was implemented and then backed out: SerializationCustomizer's mapper is also the persistence mapper (JsonSerialization injects the same CDI ObjectMapper), so it would have changed on-disk formats and silently broken the server-side sort on GroupConversation.lastModified. Commit dc117cddc had already reverted a broader version of that change once. --- ...6-07-21-manager-coverage-backend-design.md | 112 ++++++++++++++++++ 1 file changed, 112 insertions(+) create mode 100644 docs/superpowers/specs/2026-07-21-manager-coverage-backend-design.md diff --git a/docs/superpowers/specs/2026-07-21-manager-coverage-backend-design.md b/docs/superpowers/specs/2026-07-21-manager-coverage-backend-design.md new file mode 100644 index 0000000000..bd607887ca --- /dev/null +++ b/docs/superpowers/specs/2026-07-21-manager-coverage-backend-design.md @@ -0,0 +1,112 @@ +# Manager-coverage backend patches — design + +**Date:** 2026-07-21 +**Branch:** `claude/eddi-backend-manager-coverage-0598fe` (branched from `origin/main`) +**Source:** the "EDDI Backend Patches" spec accompanying the EDDI-Manager `feat/eddi-feature-coverage` branch + +## Summary + +The incoming spec proposed four backend changes so the EDDI-Manager could reach full feature coverage. Verifying each claim against the code found the spec directionally right but materially stale, and surfaced several pre-existing defects more serious than anything it asked for — including one that can permanently delete live configuration. + +This document records what was verified, what shipped, and what deliberately did not. + +## Corrections to the incoming spec + +| Spec claim | Reality | +| --- | --- | +| `TenantUsage.monthlyCostUsd` | The type does not exist. It was split into `TenantUsageCounters` (internal) and `UsageSnapshot` (API). `costMonth` already ships — that action item was already done. | +| Item 4 is "optional cleanliness" | It is a live user-visible bug: the Manager renders every `Instant` as a 1970 date. | +| Item 4 = annotate `nextFire` + `lastFired` | `ScheduleConfiguration` has **six** `Instant` fields, `ScheduleFireLog` three. Annotating two of six leaves one JSON object carrying two ISO strings and four fractional-second numbers — worse than the uniform wrongness it replaces. | +| Add an IT in `EDDI-integration-tests` | No such module. `pom.xml` has no ``; the 68 `*IT.java` live in `src/test/java/ai/labs/eddi/integration/`. | +| Branch `feat/manager-coverage-backend` | Does not exist locally or on any remote. | +| `./mvnw -q -pl . test` | Single-module build, so `-pl .` is redundant; Windows needs `.\mvnw.cmd`; `-q` suppresses the surefire summary. | +| Manager sent `undeployAllPreviousVersions` | Zero occurrences anywhere in this repo, including the vendored Manager bundles. Backend param is `undeployThisAndAllPreviousAgentVersions`. Unverifiable from here — confirm in the Manager repo. | +| No per-secret `rotate` endpoint | Correct, but `POST /{tenantId}/rotate-dek` and `POST /admin/rotate-kek` do exist for other key layers. Do not let the phrasing invite a redundant endpoint. | + +## Shipped + +### 1. `fix(admin)` — descriptor page walk and purge safety (`cb9b54ce1`) + +`readAllDescriptors` advanced its cursor by `batch.size()`, but `DescriptorStore.readDescriptors` treats that argument as a **page index** (`skip = index * limit`). The second iteration requested `skip = 40 000`, always came back empty, and every store type truncated at 200 rows. + +The dangerous half was `buildReferencedUrisSet` — the *referenced* set truncated identically, so above 200 agents or 200 workflows, live in-use resources were classified as orphans, and `purgeOrphans` deletes with `permanent=true` (`deleteAllPermanently`: current document **and** all history). + +Separately the reference scan failed **open**: read errors were swallowed (two at `debug`) and the partial set returned as complete. Since that set is what *protects* a resource, every swallowed error made more things look orphaned. + +- page walk advances by page, bounded by `MAX_PAGES`; raises rather than truncating silently +- `scanReferencedUris()` returns a `ReferenceScan` record carrying a `complete` flag +- `purgeOrphans` refuses with **409** on an incomplete scan; `scanOrphans` still returns its best-effort read-only report +- `ResourceNotFoundException` is explicitly not incompleteness — a descriptor whose resource is gone is a genuine orphan + +### 2. `fix(tenancy)` — at-limit comparison and `costMonth` shape (`7641f60f3`) + +`checkCostBudget` denies on `>=` and `InMemoryTenantQuotaStore` matches, but both **production** stores used `>`. At exactly the budget the pre-call gate denied while post-call accounting allowed. + +`UsageSnapshot.costMonth` serialized as the array `[2026,7]`: under `write-dates-as-timestamps=true`, Jackson's `YearMonthSerializer` takes its `useTimestamp` branch. Both stores already persist it as an ISO string and never route it through Jackson, so only the REST shape disagreed — making the annotation safe in isolation. + +### 3. `fix(engine)` — 429 instead of 500 for api-call quota denials (`e3fbf94c9`) + +`say()` resumes through an `AsyncResponse`, so `QuotaExceededException` is caught inside `RestAgentEngine.sayInternal` and never reaches `QuotaExceededExceptionMapper`. It fell into `catch (Exception e)` and surfaced as 500. Only the *conversation-start* quota ever produced a real 429, because that path is synchronous. + +Both new behavioural tests were **mutation-checked** — reverting the production change makes each fail — so neither is vacuous. + +## Deliberately not shipped + +### C3 as designed — repo-wide `Instant` → ISO-8601 + +Approved as "repo-wide via `SerializationCustomizer`", implemented, then **backed out** when the critique surfaced a blocker that invalidated the basis of the decision. + +`SerializationCustomizer`'s mapper is **not only the REST mapper**. `JsonSerialization` `@Inject`s the same CDI `ObjectMapper`, and it backs `DocumentBuilder` for every Mongo resource write, every Postgres JSONB column, the backup/export writer, and the `{json:serialize}` Qute template extension. Changing the shared static therefore changes **on-disk formats**. + +Concretely: `GroupConversation.lastModified` is an `Instant` persisted through that mapper, and `GroupConversationStore.listByGroupId`/`listByState` sort **server-side** on it. Mongo ranks all Doubles before all Strings in cross-type ordering; Postgres does `ORDER BY data->>'lastModified'` lexicographically. Old numeric rows and new ISO rows interleave wrongly, silently, with no backfill. + +Commit `dc117cddc` ("keep numeric date format; harden schedule Instant parsing") already reverted a broader version of this exact change for breaking `findDueSchedules`. + +The empirical part still holds and is worth keeping: a `configOverride(Instant.class)` **does** beat the global flag, old fractional-epoch-second JSON **does** still deserialize, and `java.util.Date` stays epoch millis. The blocker is scope, not mechanism. + +**To land it properly:** +1. Decouple persistence from REST — give `JsonSerialization` its own mapper from `configureObjectMapper(new ObjectMapper(), false)` *without* the override, and apply the override only inside `customize(ObjectMapper)`. Moving the line into `customize()` alone is **not** sufficient, because `JsonSerialization` injects the very mapper `customize()` mutates. +2. Or annotate the enumerable REST-facing DTOs instead (`ScheduleConfiguration`, `ScheduleFireLog`, `PendingApprovalSummary`, `SimpleConversationMemorySnapshot.hitlPausedAt`, `UserMemoryEntry`, `GroupConversation`, `Attachment.createdAt`), per-class-complete — the `SecretMetadata` idiom. +3. Either way: restore the fractional-seconds precondition in `MongoScheduleStoreInstantRoundTripTest.setUp` (otherwise that CRITICAL-bug guard silently stops reproducing its scenario), and land the Manager comparator fix — the schedules dashboard sorts `nextFire` arithmetically, which yields `NaN` on ISO strings. + +### C1(b) — `includeDeleted` semantics + +`includeDeleted` is an equality filter (`Filters.eq("deleted", flag)`), so `scanOrphans` (default `false`) and `purgeOrphans` (default `true`) operate on **disjoint** sets. Redefining it as a true "include" is correct and matches the documented meaning — but with the page walk now complete, doing so *without also flipping the `true` default* turns the default `DELETE /administration/orphans` from "purge ≤200 already-soft-deleted rows" into "permanently wipe every unreferenced resource, unbounded". + +Land as its own commit, together with the default flip and/or an explicit `confirm=true`, and a changelog entry naming the behaviour change. Note the `includeDeleted=true` path currently has **zero** test coverage anywhere in the repo. + +### C2 — selective orphan purge + +Blocked on C1(b): until scan and purge see the same set, a selective purge deletes exactly zero of the user's selections. Design that survived critique: + +- `@QueryParam("resource") List`, null-guarded, no `@DefaultValue`; absent/empty = purge all +- scan runs first and unconditionally; `resources` only **intersects** its result — never a branch that skips the scan, or it becomes a delete-arbitrary-resource primitive on an endpoint whose `@RolesAllowed` is unenforced when `quarkus.oidc.tenant-enabled=false` +- open question the critique raised and this design has not answered: normalising to `host + "/" + id` drops the version, and `extractResourceId` returns a null id for short/non-hex input, so several distinct inputs can collapse to one key. Decide between `type:id` pairs and full-URI matching before implementing. + +### C4 — `maxAgentsPerTenant` enforcement + +No blockers, several confirmed majors: + +- **`autoDeploy=false` bypasses it entirely** — the `deployed` row is only written when `autoDeploy` is true, yet the agent is live in `AgentFactory`. `AgentDeploymentComponentIT` exercises that path. +- The count must be read on the request thread and **outside** the existing `try`, or `catch (Exception) → InternalServerErrorException` converts the 429 into a 500. +- Read-only gate (`checkAgentQuota(tenantId, count)`) modelled on `checkCostBudget`, not an atomic counter — the 10 s re-deploy sweep, the 24 h old-version undeploy, `TeardownAgentTool` and `GroupConversationService` all mutate deployments without passing any acquire point. +- Keep the 1-arg `QuotaExceededException(String)` defaulting to 60 so `TenancyModelsTest` and `RestTenantQuotaTest` keep passing; add a 2-arg overload for capacity denials. +- `RestAgentAdministrationTest` and `RestAgentAdministrationExtendedTest` construct the bean positionally and must be updated in the same commit. + +### C5 — cost metering + +**Blocked on a blocker, not just majors.** The design accumulated per-turn USD from token usage, but agent mode and the HITL resume path *discard* `tokenUsage` — so the accumulator would read ~0 on exactly the paths the change targets. `ObservableChatModel` is the correct interception point, not `LlmTask`. + +Additional confirmed problems: +- `inputPricePer1M`/`outputPricePer1M` already exist on `CascadeStep` **and** `ModelCascadeConfig`. A third copy on `Task` needs a stated precedence rule or it double-counts cascade turns against `runCostUsd`, which `LlmTask` already surfaces as `cascadeCostUsd`. +- Five other LLM call sites in the same turn would remain unmetered. +- Wiring `recordCost` makes a dormant **Mongo E11000** live: `tryAddCost` upserts against a filter pinning `costMonth` while `tenant_usage` has a unique index on `tenantId`, so the first cost of each new UTC month attempts a duplicate insert. `MongoTenantQuotaStoreTest:369` pins the current two-call sequence and will need rewriting. +- Tool cost is structurally $0 — `ToolCostTracker`'s price map is keyed `"websearch"`/`"weather"` while the names passed in are `searchWeb`/`getCurrentWeather`, so every lookup misses. + +Redesign around `ObservableChatModel` before implementing. + +## Verification notes + +- Local `mvnw test` is red out of the box — roughly 8 failures / 288 errors on a pristine tree, all environmental (loopback sockets, network egress, model downloads) in `WebSearchTool`, `WebScraperTool`, `WeatherTool`, `PdfReaderTool`, `SafeHttpClient`, `SlackWebApiClient`, `A2AToolProviderManager*`, `EmbeddingModelFactory*`, `LanguageModelBuilders`. Baseline before attributing failures to a change. +- `-Dtest=Class#method` runs **0 tests and exits green** when the method sits in a `@Nested` class. Filter by whole class. +- Fixtures need 24-char hex ids — `RestUtilities.extractResourceId` returns a null id otherwise, and assertions pass vacuously. From 2dfc9207b2bcd23e665579f7db25db209b3c6f02 Mon Sep 17 00:00:00 2001 From: Gregor Jarisch Date: Tue, 21 Jul 2026 15:01:21 +0200 Subject: [PATCH 05/12] fix(admin): give the purge refusal a response body; review follow-ups MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Self-review of the three preceding commits found three defects. purgeOrphans threw `new WebApplicationException(String, Response.Status)`, whose response carries NO entity — so an operator got a bare 409 and the reason existed only in the server log. Build the Response explicitly with {"error":"incomplete_scan","message":...} instead. Confirmed by mutation: reverting to that constructor makes the new hasEntity() assertion fail, so the fix is real rather than cosmetic. The page-walk javadoc still linked buildReferencedUrisSet(), which was renamed to scanReferencedUris() in the same commit — a broken @link. Also updated three now-inaccurate @DisplayNames in RestOrphanAdminBranchTest. The > to >= change was mutation-verified on Mongo but Postgres had no at-limit test at all. Added PostgresTenantQuotaStoreTest.exactlyAtLimit and mutation-checked it. Documented why a MAX_PAGES trip during orphan collection is safely swallowed while the same failure in the reference scan blocks the purge: an unenumerable type yields fewer delete candidates (under-delete), whereas a missing reference promotes a live resource to "orphan" (over-delete). --- docs/changelog.md | 16 ++++++++++++++++ .../configs/admin/rest/RestOrphanAdmin.java | 18 ++++++++++++++---- .../admin/rest/RestOrphanAdminBranchTest.java | 6 +++--- .../admin/rest/RestOrphanAdminSafetyTest.java | 6 ++++++ .../tenancy/PostgresTenantQuotaStoreTest.java | 13 +++++++++++++ 5 files changed, 52 insertions(+), 7 deletions(-) diff --git a/docs/changelog.md b/docs/changelog.md index 1561a1989c..39f34ac367 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -3,6 +3,22 @@ > **Purpose:** Living document tracking all changes, decisions, and reasoning during implementation. Updated as work progresses for easy reference and review. +--- + +## 🔍 Review follow-ups on the orphan/tenancy fixes (2026-07-21) + +**Repo:** EDDI (`claude/eddi-backend-manager-coverage-0598fe`) + +Self-review of the three preceding commits found three defects, all fixed here. + +- **The 409 refusal carried no body.** `purgeOrphans` threw `new WebApplicationException(String, Response.Status)`, whose response has **no entity** — so an operator saw a bare 409 and the reason existed only in the server log. Confirmed empirically by mutation: reverting to that constructor makes the new `hasEntity()` assertion fail. Now builds the `Response` explicitly with `{"error":"incomplete_scan","message":…}`. +- **Broken javadoc link.** The page-walk javadoc still referenced `buildReferencedUrisSet()`, renamed to `scanReferencedUris()` in the same commit. Also updated three now-inaccurate `@DisplayName`s in `RestOrphanAdminBranchTest`. +- **Postgres had no at-limit test.** The `>` → `>=` change was mutation-verified on Mongo but not Postgres. Added `PostgresTenantQuotaStoreTest.exactlyAtLimit` and mutation-checked it too. + +Also documented why a `MAX_PAGES` trip during orphan *collection* is safely swallowed while the same failure in the *reference* scan blocks the purge: a type that cannot be enumerated yields fewer delete candidates (under-delete), whereas a missing reference promotes a live resource to "orphan" (over-delete). + +**Verification:** full clean suite `tests=11273 failures=8 errors=288 skipped=3` across the same 15 classes as the pre-change baseline — all environmental (loopback sockets, network egress, model downloads). Zero regressions. Checkstyle clean on every changed file; `javadoc:javadoc` builds. + --- ## 🚦 Engine — return 429 instead of 500 when the api-call quota denies a turn (2026-07-21) diff --git a/src/main/java/ai/labs/eddi/configs/admin/rest/RestOrphanAdmin.java b/src/main/java/ai/labs/eddi/configs/admin/rest/RestOrphanAdmin.java index 80f4bac68b..aacaffddaf 100644 --- a/src/main/java/ai/labs/eddi/configs/admin/rest/RestOrphanAdmin.java +++ b/src/main/java/ai/labs/eddi/configs/admin/rest/RestOrphanAdmin.java @@ -20,6 +20,7 @@ import jakarta.enterprise.context.ApplicationScoped; import jakarta.inject.Inject; import jakarta.ws.rs.WebApplicationException; +import jakarta.ws.rs.core.MediaType; import jakarta.ws.rs.core.Response; import org.jboss.logging.Logger; @@ -100,9 +101,14 @@ public OrphanReport purgeOrphans(Boolean includeDeleted) { ReferenceScan scan = scanReferencedUris(); if (!scan.complete()) { log.errorf("Refusing to purge orphans: the reference scan was incomplete (%s)", scan.failureReason()); - throw new WebApplicationException("Refusing to purge orphans: the reference scan was incomplete (" + scan.failureReason() - + "). Purging on a partial reference set could permanently delete resources that are still in use.", - Response.Status.CONFLICT); + // Build the Response explicitly rather than using the (String, Status) + // constructor: that one sets no entity, so the caller would receive a bare + // 409 and the reason would exist only in the server log. + throw new WebApplicationException(Response.status(Response.Status.CONFLICT) + .entity(Map.of("error", "incomplete_scan", "message", + "Refusing to purge orphans: the reference scan was incomplete (" + scan.failureReason() + + "). Purging on a partial reference set could permanently delete resources that are still in use.")) + .type(MediaType.APPLICATION_JSON).build()); } List orphans = collectOrphans(scan.referencedUris(), includeDeleted); @@ -158,6 +164,10 @@ private List collectOrphans(Set referencedUris, boolean incl } } } catch (Exception e) { + // Safe to continue: a type that cannot be enumerated contributes no + // orphan CANDIDATES, so the purge under-deletes rather than over- + // deletes. The opposite failure — an incomplete REFERENCE set — is the + // dangerous one and is handled by ReferenceScan.complete(). log.warnf("Error scanning store type %s: %s", type, e.getMessage()); } } @@ -276,7 +286,7 @@ private void collectExtensionUris(WorkflowConfiguration workflowConfig, Set */ diff --git a/src/test/java/ai/labs/eddi/configs/admin/rest/RestOrphanAdminBranchTest.java b/src/test/java/ai/labs/eddi/configs/admin/rest/RestOrphanAdminBranchTest.java index 7c6c5f0cf3..06c700a7a9 100644 --- a/src/test/java/ai/labs/eddi/configs/admin/rest/RestOrphanAdminBranchTest.java +++ b/src/test/java/ai/labs/eddi/configs/admin/rest/RestOrphanAdminBranchTest.java @@ -586,15 +586,15 @@ void purgeNoOrphans() throws Exception { } // ========================================================= - // buildReferencedUrisSet — exception handling + // scanReferencedUris — exception handling // ========================================================= @Nested - @DisplayName("buildReferencedUrisSet — error handling") + @DisplayName("scanReferencedUris — error handling") class BuildReferencedUrisSetErrors { @Test - @DisplayName("global exception in buildReferencedUrisSet is caught") + @DisplayName("global exception in scanReferencedUris is caught") void globalException() throws Exception { when(documentDescriptorStore.readDescriptors(eq("ai.labs.agent"), anyString(), anyInt(), anyInt(), eq(false))) .thenThrow(new IResourceStore.ResourceStoreException("db error")); diff --git a/src/test/java/ai/labs/eddi/configs/admin/rest/RestOrphanAdminSafetyTest.java b/src/test/java/ai/labs/eddi/configs/admin/rest/RestOrphanAdminSafetyTest.java index 6320044e7c..63d9769109 100644 --- a/src/test/java/ai/labs/eddi/configs/admin/rest/RestOrphanAdminSafetyTest.java +++ b/src/test/java/ai/labs/eddi/configs/admin/rest/RestOrphanAdminSafetyTest.java @@ -149,6 +149,12 @@ void unreadableAgentAbortsPurge() throws Exception { WebApplicationException thrown = assertThrows(WebApplicationException.class, () -> restOrphanAdmin.purgeOrphans(true)); assertEquals(409, thrown.getResponse().getStatus()); + // The reason must reach the caller, not just the server log — the + // (String, Status) constructor sets no entity, so this pins that the + // Response is built explicitly. + assertTrue(thrown.getResponse().hasEntity(), "409 must carry a body explaining the refusal"); + assertTrue(thrown.getResponse().getEntity().toString().contains("mongo down"), + "the body must name the underlying cause, got: " + thrown.getResponse().getEntity()); verify(resourceClientLibrary, never()).deleteResource(any(), anyBoolean()); } diff --git a/src/test/java/ai/labs/eddi/engine/tenancy/PostgresTenantQuotaStoreTest.java b/src/test/java/ai/labs/eddi/engine/tenancy/PostgresTenantQuotaStoreTest.java index 7cfac6b391..8b33b1f498 100644 --- a/src/test/java/ai/labs/eddi/engine/tenancy/PostgresTenantQuotaStoreTest.java +++ b/src/test/java/ai/labs/eddi/engine/tenancy/PostgresTenantQuotaStoreTest.java @@ -484,6 +484,19 @@ void overBudget() throws Exception { assertTrue(result.reason().contains("Monthly cost budget exceeded")); } + @Test + @DisplayName("should return denied at exactly the limit, matching checkCostBudget") + void exactlyAtLimit() throws Exception { + when(resultSet.next()).thenReturn(true); + when(resultSet.getDouble(1)).thenReturn(100.0); + + QuotaCheckResult result = sut.tryAddCost(TENANT_ID, 10.0, 100.0); + + // TenantQuotaService.checkCostBudget denies on `currentCost >= limit`, so + // post-call accounting must use >= too or the two disagree at the boundary. + assertFalse(result.allowed(), "at exactly the limit the budget is spent"); + } + @Test @DisplayName("should return OK when limit is negative (unlimited)") void unlimitedBudget() throws Exception { From bd3f8bdbf5aabd618a984c77e1fe2d038ede20e9 Mon Sep 17 00:00:00 2001 From: Gregor Jarisch Date: Tue, 21 Jul 2026 16:14:40 +0200 Subject: [PATCH 06/12] fix(admin)!: make includeDeleted a true inclusion flag; flip purge default MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit BREAKING (REST): DELETE /administration/orphans now defaults to includeDeleted=false and purges live-but-unreferenced resources only. DescriptorStore.readDescriptors treated includeDeleted as an EQUALITY filter — eq("deleted", includeDeleted) — so includeDeleted=true matched ONLY soft-deleted descriptors instead of adding them to the live ones. The parameter did not mean what its name, its @Parameter text, or docs/deployment-management-of-agents.md said it meant. Worse, the shipped Manager scans with false and purges with true, so the set shown to the user and the set deleted were disjoint: the UI listed live orphans and then purged soft-deleted ones. true now drops the `deleted` constraint entirely (live AND soft-deleted); false constrains to live only. Every other caller in src/main passes a literal false — all ~35 call sites enumerated — so their behaviour is unchanged. purgeOrphans's @DefaultValue flips from true to false. Left at true, the semantics fix would have made the parameterless DELETE dramatically more destructive: combined with the page-walk fix, from "purge <=200 already- soft-deleted rows" to "permanently wipe every unreferenced resource, unbounded". Flipping it makes the bare call the conservative one and makes scan and purge describe the same set by default. Clients relying on the old default now purge LESS; pass includeDeleted=true to also purge soft-deleted resources. Mutation-checked: restoring the equality filter fails the new readDescriptorsIncludesDeleted assertion. --- docs/changelog.md | 25 +++++++++++ docs/deployment-management-of-agents.md | 42 +++++++++++++------ .../eddi/configs/admin/IRestOrphanAdmin.java | 18 ++++---- .../labs/eddi/datastore/DescriptorStore.java | 9 +++- .../admin/rest/RestOrphanAdminSafetyTest.java | 31 ++++++++++++++ .../eddi/datastore/DescriptorStoreTest.java | 40 ++++++++++++++++++ 6 files changed, 144 insertions(+), 21 deletions(-) diff --git a/docs/changelog.md b/docs/changelog.md index 39f34ac367..3f0befc2b2 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -3,6 +3,31 @@ > **Purpose:** Living document tracking all changes, decisions, and reasoning during implementation. Updated as work progresses for easy reference and review. +--- + +## 🧹 Orphan admin — `includeDeleted` becomes a true inclusion flag; purge default flipped (2026-07-21) + +**Repo:** EDDI (`fix/orphan-scan-and-quota-defects`) + +⚠️ **REST behaviour change on `DELETE /administration/orphans`.** + +`DescriptorStore.readDescriptors` treated `includeDeleted` as an **equality** filter — `eq("deleted", includeDeleted)` — so `includeDeleted=true` matched *only* soft-deleted descriptors rather than adding them to the live ones. Two consequences: + +- The parameter did not mean what its name, its `@Parameter` text, and `docs/deployment-management-of-agents.md` all said it meant. +- The shipped Manager scans with `includeDeleted=false` and purges with `includeDeleted=true`, so **the set shown to the user and the set deleted were disjoint**. The UI listed live orphans and then purged soft-deleted ones. + +`true` now drops the `deleted` constraint entirely (live **and** soft-deleted); `false` constrains to live only. Every other caller in `src/main` passes a literal `false` — verified by enumerating all ~35 call sites — so their behaviour is bit-for-bit unchanged. + +**`purgeOrphans`'s `@DefaultValue` flipped from `true` to `false`.** Left at `true`, the semantics fix would have made the parameterless `DELETE` dramatically *more* destructive — combined with the page-walk fix two commits earlier, from "purge ≤200 already-soft-deleted rows" to "permanently wipe every unreferenced resource, unbounded". Flipping the default makes the bare call the conservative one and matches `scanOrphans`, so a scan and a purge with no parameters now describe the same set. + +### Client impact + +A client that relied on the old default now purges **less**: live-but-unreferenced orphans only, not soft-deleted ones. Pass `includeDeleted=true` explicitly to also purge soft-deleted resources. EDDI-Manager should send the flag explicitly, matching whichever set it is displaying. + +### Tests + +`DescriptorStoreTest` gains two tests capturing the actual `QueryFilters` and asserting the `deleted` constraint is present for `false` and **absent** for `true`; mutation-checked by restoring the equality filter, which fails the second. `RestOrphanAdminSafetyTest` gains two tests pinning the flag's propagation. All 86 tests across every descriptor-store consumer green. + --- ## 🔍 Review follow-ups on the orphan/tenancy fixes (2026-07-21) diff --git a/docs/deployment-management-of-agents.md b/docs/deployment-management-of-agents.md index 773797d8ca..fa7738fb9b 100644 --- a/docs/deployment-management-of-agents.md +++ b/docs/deployment-management-of-agents.md @@ -296,13 +296,13 @@ GET /administration/orphans → 200 OK ``` -Returns a report listing all unreferenced resources across all stores (packages, behavior sets, HTTP calls, output sets, langchains, property setters, dictionaries, parsers). +Returns a report listing all unreferenced resources across all stores (workflows, behavior sets, HTTP calls, output sets, LLMs, property setters, dictionaries, parsers). -| Element | Value | -| -------------- | ----------------------------------------------------------------------------- | -| HTTP Method | `GET` | -| API endpoint | `/administration/orphans` | -| includeDeleted | (`Query parameter`) `Boolean` default `false`. Include soft-deleted resources | +| Element | Value | +| -------------- | ------------------------------------------------------------------------------------------ | +| HTTP Method | `GET` | +| API endpoint | `/administration/orphans` | +| includeDeleted | (`Query parameter`) `Boolean` default `false`. `true` also includes soft-deleted resources | **Example Response:** @@ -334,10 +334,26 @@ DELETE /administration/orphans → 200 OK ``` -Permanently deletes all orphaned resources. This is **irreversible**. - -| Element | Value | -| -------------- | ------------------------------------------------------------------------------------- | -| HTTP Method | `DELETE` | -| API endpoint | `/administration/orphans` | -| includeDeleted | (`Query parameter`) `Boolean` default `true`. Include soft-deleted resources in purge | +Permanently deletes all orphaned resources. This is **irreversible** — it removes the current +document *and* its entire version history. + +| Element | Value | +| -------------- | -------------------------------------------------------------------------------------------------------- | +| HTTP Method | `DELETE` | +| API endpoint | `/administration/orphans` | +| includeDeleted | (`Query parameter`) `Boolean` default `false`. `true` also purges soft-deleted resources | +| 409 Conflict | Returned when the reference scan is incomplete; **nothing is deleted**. Body: `{"error":"incomplete_scan"}` | + +Pass the **same** `includeDeleted` value you used for the scan, so the purge acts on the set you +reviewed. + +> **Changed in 6.1.x:** `includeDeleted` was previously an *equality* filter — `true` matched only +> soft-deleted resources instead of adding them to the live ones — and this endpoint defaulted to +> `true` while the scan defaulted to `false`, so a scan followed by a purge operated on **disjoint** +> sets. `true` now means "live and soft-deleted", and the default is `false`. A client relying on the +> old default now purges *less*; pass `includeDeleted=true` to restore the wider sweep. + +The purge refuses with **409** rather than proceeding when the referenced-resource scan could not be +completed (an unreadable Agent or workflow, or a store type exceeding the scan ceiling). A partial +reference set makes live, in-use resources look unreferenced, so purging against one could destroy +working configuration. diff --git a/src/main/java/ai/labs/eddi/configs/admin/IRestOrphanAdmin.java b/src/main/java/ai/labs/eddi/configs/admin/IRestOrphanAdmin.java index b9519e8f04..98010e3436 100644 --- a/src/main/java/ai/labs/eddi/configs/admin/IRestOrphanAdmin.java +++ b/src/main/java/ai/labs/eddi/configs/admin/IRestOrphanAdmin.java @@ -34,24 +34,28 @@ public interface IRestOrphanAdmin { @NoCache @Produces(MediaType.APPLICATION_JSON) @Operation(summary = "Scan for orphaned resources", description = "Scans all stores " - + "(packages, behavior sets, HTTP calls, output sets, langchains, " - + "property setters, dictionaries) and returns resources not referenced by any Agent or package. " - + "Use includeDeleted=true to also include soft-deleted resources in the report.") + + "(workflows, behavior sets, HTTP calls, output sets, LLMs, " + + "property setters, dictionaries, parsers) and returns resources not referenced by any Agent or workflow. " + + "includeDeleted=true additionally includes soft-deleted resources; false (the default) reports live resources only.") @APIResponse(responseCode = "200", description = "Orphan report with list of unreferenced resources.") // @formatter:off OrphanReport scanOrphans( - @Parameter(description = "Include soft-deleted resources in the scan. Default: false.") + @Parameter(description = "Also include soft-deleted resources. Default: false (live resources only).") @QueryParam("includeDeleted") @DefaultValue("false") Boolean includeDeleted); // @formatter:on @DELETE @Produces(MediaType.APPLICATION_JSON) @Operation(summary = "Purge orphaned resources", description = "Scans all stores for orphans and permanently deletes them. " - + "Returns a report of what was deleted. This operation is irreversible.") + + "Returns a report of what was deleted. This operation is irreversible. " + + "Pass the same includeDeleted value used for the scan so the purge acts on the set that was reviewed. " + + "Refuses with 409 when the reference scan is incomplete, since a partial reference set would " + + "misclassify live resources as orphans.") @APIResponse(responseCode = "200", description = "Purge report with count and list of deleted resources.") + @APIResponse(responseCode = "409", description = "The reference scan was incomplete; nothing was deleted.") // @formatter:off OrphanReport purgeOrphans( - @Parameter(description = "Include soft-deleted resources in the purge. Default: true.") - @QueryParam("includeDeleted") @DefaultValue("true") Boolean includeDeleted); + @Parameter(description = "Also purge soft-deleted resources. Default: false (live resources only).") + @QueryParam("includeDeleted") @DefaultValue("false") Boolean includeDeleted); // @formatter:on } diff --git a/src/main/java/ai/labs/eddi/datastore/DescriptorStore.java b/src/main/java/ai/labs/eddi/datastore/DescriptorStore.java index 206d5bce20..232d5ffbd4 100644 --- a/src/main/java/ai/labs/eddi/datastore/DescriptorStore.java +++ b/src/main/java/ai/labs/eddi/datastore/DescriptorStore.java @@ -42,7 +42,14 @@ public List readDescriptors(String type, String filter, Integer index, Intege List queryFiltersRequired = new LinkedList<>(); String filterURI = "eddi://" + type + ".*"; queryFiltersRequired.add(new IResourceFilter.QueryFilter(FIELD_RESOURCE, filterURI)); - queryFiltersRequired.add(new IResourceFilter.QueryFilter(FIELD_DELETED, includeDeleted)); + // includeDeleted is an INCLUSION flag, not an equality filter: true means "do + // not constrain on `deleted` at all" (live AND soft-deleted), false means live + // only. It previously added eq(deleted, includeDeleted), so includeDeleted=true + // matched ONLY soft-deleted descriptors — making a scan and a purge that + // differed on the flag operate on disjoint sets. + if (!includeDeleted) { + queryFiltersRequired.add(new IResourceFilter.QueryFilter(FIELD_DELETED, false)); + } IResourceFilter.QueryFilters required = new IResourceFilter.QueryFilters(queryFiltersRequired); List queryFiltersOptional = new LinkedList<>(); diff --git a/src/test/java/ai/labs/eddi/configs/admin/rest/RestOrphanAdminSafetyTest.java b/src/test/java/ai/labs/eddi/configs/admin/rest/RestOrphanAdminSafetyTest.java index 63d9769109..77b45ba85d 100644 --- a/src/test/java/ai/labs/eddi/configs/admin/rest/RestOrphanAdminSafetyTest.java +++ b/src/test/java/ai/labs/eddi/configs/admin/rest/RestOrphanAdminSafetyTest.java @@ -32,6 +32,7 @@ import static org.mockito.ArgumentMatchers.anyInt; import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.atLeastOnce; import static org.mockito.Mockito.never; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @@ -204,6 +205,36 @@ void scanToleratesIncompleteReferenceSet() throws Exception { } } + @Nested + @DisplayName("includeDeleted is an inclusion flag, not an equality filter") + class IncludeDeletedSemantics { + + @Test + @DisplayName("false constrains to live resources only") + void falseFiltersToLive() throws Exception { + when(documentDescriptorStore.readDescriptors(anyString(), anyString(), anyInt(), anyInt(), anyBoolean())) + .thenReturn(List.of()); + + restOrphanAdmin.scanOrphans(false); + + verify(documentDescriptorStore, atLeastOnce()).readDescriptors(eq("ai.labs.rules"), anyString(), eq(0), anyInt(), eq(false)); + } + + @Test + @DisplayName("true is forwarded so the store drops the deleted constraint entirely") + void trueForwardsInclusion() throws Exception { + when(documentDescriptorStore.readDescriptors(anyString(), anyString(), anyInt(), anyInt(), anyBoolean())) + .thenReturn(List.of()); + + restOrphanAdmin.scanOrphans(true); + + // The store-level change is what makes true mean "live AND soft-deleted"; + // previously it meant "soft-deleted only", so a scan(false)/purge(true) pair + // acted on disjoint sets. + verify(documentDescriptorStore, atLeastOnce()).readDescriptors(eq("ai.labs.rules"), anyString(), eq(0), anyInt(), eq(true)); + } + } + @Nested @DisplayName("referenced resources are protected") class ReferenceProtection { diff --git a/src/test/java/ai/labs/eddi/datastore/DescriptorStoreTest.java b/src/test/java/ai/labs/eddi/datastore/DescriptorStoreTest.java index 720a6d7a15..45566886d8 100644 --- a/src/test/java/ai/labs/eddi/datastore/DescriptorStoreTest.java +++ b/src/test/java/ai/labs/eddi/datastore/DescriptorStoreTest.java @@ -8,6 +8,7 @@ import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.DisplayName; import org.junit.jupiter.api.Test; +import org.mockito.ArgumentCaptor; import java.io.IOException; import java.util.List; @@ -145,6 +146,45 @@ void readDescriptorsDefaultLimit() throws Exception { assertTrue(result.isEmpty()); } + @Test + @DisplayName("readDescriptors — includeDeleted=false constrains `deleted` to false") + void readDescriptorsExcludesDeleted() throws Exception { + when(resourceStorage.findResources(any(IResourceFilter.QueryFilters[].class), anyString(), anyInt(), anyInt())) + .thenReturn(List.of()); + + store.readDescriptors("agents", null, 0, 20, false); + + var captor = ArgumentCaptor.forClass(IResourceFilter.QueryFilters[].class); + verify(resourceStorage).findResources(captor.capture(), anyString(), anyInt(), anyInt()); + assertTrue(hasDeletedFilter(captor.getValue()), "expected a `deleted` constraint when includeDeleted=false"); + } + + @Test + @DisplayName("readDescriptors — includeDeleted=true drops the `deleted` constraint entirely") + void readDescriptorsIncludesDeleted() throws Exception { + when(resourceStorage.findResources(any(IResourceFilter.QueryFilters[].class), anyString(), anyInt(), anyInt())) + .thenReturn(List.of()); + + store.readDescriptors("agents", null, 0, 20, true); + + // It previously added eq(deleted, true), which matched ONLY soft-deleted rows — + // so a caller scanning with false and purging with true saw disjoint sets. + var captor = ArgumentCaptor.forClass(IResourceFilter.QueryFilters[].class); + verify(resourceStorage).findResources(captor.capture(), anyString(), anyInt(), anyInt()); + assertFalse(hasDeletedFilter(captor.getValue()), "includeDeleted=true must not constrain on `deleted` at all"); + } + + private static boolean hasDeletedFilter(IResourceFilter.QueryFilters[] filters) { + for (IResourceFilter.QueryFilters group : filters) { + for (IResourceFilter.QueryFilter f : group.getQueryFilters()) { + if ("deleted".equals(f.getField())) { + return true; + } + } + } + return false; + } + // ==================== findByOriginId ==================== @Test From a4d36868357164e59292f71540e8eea9d99d53f7 Mon Sep 17 00:00:00 2001 From: Gregor Jarisch Date: Tue, 21 Jul 2026 17:04:05 +0200 Subject: [PATCH 07/12] feat(tenancy): enforce maxAgentsPerTenant on the agent deploy path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit TenantQuota.maxAgentsPerTenant was persisted by all three stores and round-tripped through REST, but nothing ever read it — TenantQuotaService had no agent method at all. Operators setting the limit got silent no-enforcement. Adds TenantQuotaService.checkAgentQuota(tenantId, currentDistinctAgents), gating RestAgentAdministration.deployAgent and denying with QuotaExceededException -> 429 via the existing mapper. A read-only gate, not an atomic counter: the deployed-agent count is a stock derived by counting deployments, not a per-window flow. A stored counter would drift, because the 10s re-deploy sweep, the 24h old-version undeploy, TeardownAgentTool, GroupConversationService and the lazy re-deploy on first use all mutate deployments without passing any acquire/release point. No ITenantQuotaStore method added, so the three store impls are untouched. Placement is forced: the gate sits between the null-checks and the try. Inside the try, catch(Exception) -> InternalServerErrorException would turn the 429 into a 500; inside the submitted Callable it runs off the request thread and could never produce a status code. The count unions persisted `deployed` rows with live READY agents, and needs both. autoDeploy=false never writes a row but the in-memory deploy is unconditional, so a rows-only count let a caller deploy unlimited agents with one query param — and those agents are genuinely live and durable: getLatestReadyAgent serves them without consulting the store, and ConversationService.getAgent lazily re-deploys them after a restart. A live-agents-only count would be per-JVM and miss other cluster nodes. Counts distinct agent ids, so redeploys and version bumps are free — required because the old-version undeploy sweep legitimately keeps two versions deployed while the previous drains. Fails open on store error. AgentSetupService.deployAndWait and McpAdminTools.deployAgent call the bean directly, so the mapper never runs there; both now return the quota reason instead of "check server logs", which a model driving create_sub_agent cannot act on and would retry in a loop. Mutation-checked: reverting to a rows-only count fails exactly the loophole test. --- docs/changelog.md | 28 +++ .../engine/api/IRestAgentAdministration.java | 5 +- .../internal/RestAgentAdministration.java | 77 ++++++- .../labs/eddi/engine/mcp/McpAdminTools.java | 7 + .../eddi/engine/setup/AgentSetupService.java | 12 + .../engine/tenancy/TenantQuotaService.java | 43 ++++ .../RestAgentAdministrationExtendedTest.java | 14 +- .../RestAgentAdministrationQuotaTest.java | 218 ++++++++++++++++++ .../internal/RestAgentAdministrationTest.java | 14 +- .../tenancy/TenantQuotaServiceTest.java | 50 ++++ 10 files changed, 461 insertions(+), 7 deletions(-) create mode 100644 src/test/java/ai/labs/eddi/engine/internal/RestAgentAdministrationQuotaTest.java diff --git a/docs/changelog.md b/docs/changelog.md index 3f0befc2b2..da7defc6fe 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -3,6 +3,34 @@ > **Purpose:** Living document tracking all changes, decisions, and reasoning during implementation. Updated as work progresses for easy reference and review. +--- + +## 🎫 Tenancy — enforce `maxAgentsPerTenant` on deploy (2026-07-21) + +**Repo:** EDDI (`fix/orphan-scan-and-quota-defects`) + +`TenantQuota.maxAgentsPerTenant` was persisted end-to-end by all three stores and round-tripped through the REST API, but **nothing ever read it** — `TenantQuotaService` had no agent method at all. Operators could set the limit and get silent no-enforcement. + +New `TenantQuotaService.checkAgentQuota(tenantId, currentDistinctAgents)` gates `RestAgentAdministration.deployAgent`, denying with `QuotaExceededException` → **429** `{"error":"quota_exceeded"}` via the existing mapper. + +### Design decisions + +- **A read-only gate, not an atomic counter.** The deployed-agent count is a *stock* derived by counting current deployments, not a per-window *flow*. A stored counter would drift irrecoverably: the 10s re-deploy sweep, the 24h old-version undeploy, `TeardownAgentTool`, `GroupConversationService` and the lazy re-deploy on first use all add or remove deployments without passing any acquire/release point. Modelled on `checkCostBudget`, so no `ITenantQuotaStore` method was added and the three store implementations are untouched. +- **Placement is forced, not stylistic.** The gate sits between the null-checks and the `try`. Inside the `try`, `catch (Exception) → InternalServerErrorException` would convert the 429 into a **500**; inside the submitted `Callable` it runs off the request thread and could never produce a status code at all. +- **Counts distinct agent ids**, so redeploys and version bumps are free — required because the old-version undeploy sweep legitimately keeps two versions of one agent deployed while the previous drains. +- **The count unions persisted rows with live agents, and needs both.** `autoDeploy=false` never writes a `deployed` row, but the in-memory deploy is unconditional — so a rows-only count let a caller deploy unlimited agents with one query parameter. Those agents are genuinely live: `getLatestReadyAgent` serves them without consulting the store, and `ConversationService.getAgent` lazily re-deploys them after a restart, so the bypass is **durable**, not merely transient. Conversely, a live-agents-only count would be per-JVM and would miss other cluster nodes. +- **Fails open.** A store outage must not block deployments; the denial is logged and metered either way. +- **The two CDI callers surface the reason.** `AgentSetupService.deployAndWait` and `McpAdminTools.deployAgent` call the bean directly, so the exception mapper never runs — both now return the quota reason instead of "check server logs", which an agent designer (or a model driving `create_sub_agent`) cannot act on and would retry in a loop. +- **Scheduled sweeps stay ungated by construction** — they call `IAgentFactory` directly, so lowering the limit never undeploys anything. The limit gates *new* agents only. + +### Not addressed + +Single-tenant only: the gate resolves `getDefaultTenantId()`, exactly as the conversation and api-call quotas already do. This is a deployment-wide cap, not per-organisation, until the multi-tenancy plan's Phase 1 lands. Do not describe it as multi-tenant enforcement. + +### Tests + +New `RestAgentAdministrationQuotaTest` (8 tests): denial throws `QuotaExceededException` and submits nothing to the runtime; two versions of one agent count once; redeploy skips the quota entirely; **live agents with no `deployed` row still count** (the loophole); no double counting; non-`READY` agents skipped; null agent ids skipped; store error fails open. Mutation-checked — reverting to a rows-only count fails exactly the loophole test. `TenantQuotaServiceTest` gains 5 tests including parity with `checkCostBudget` on not inflating the `allowed` counter. Both existing `RestAgentAdministration*Test` classes updated for the new constructor arg in the same commit. + --- ## 🧹 Orphan admin — `includeDeleted` becomes a true inclusion flag; purge default flipped (2026-07-21) diff --git a/src/main/java/ai/labs/eddi/engine/api/IRestAgentAdministration.java b/src/main/java/ai/labs/eddi/engine/api/IRestAgentAdministration.java index 845b16d9c1..10819f0127 100644 --- a/src/main/java/ai/labs/eddi/engine/api/IRestAgentAdministration.java +++ b/src/main/java/ai/labs/eddi/engine/api/IRestAgentAdministration.java @@ -28,9 +28,12 @@ public interface IRestAgentAdministration { @POST @Path("/{environment}/deploy/{agentId}") - @Operation(summary = "Deploy an agent", description = "Deploys the specified agent version to the given environment.") + @Operation(summary = "Deploy an agent", description = "Deploys the specified agent version to the given environment. " + + "Subject to the tenant's maxAgentsPerTenant quota, which counts distinct agent ids — " + + "redeploying an agent or bumping its version never consumes additional capacity.") @APIResponse(responseCode = "200", description = "Agent deployed (or accepted if async).") @APIResponse(responseCode = "404", description = "Agent not found.") + @APIResponse(responseCode = "429", description = "Tenant agent quota exceeded; undeploy an agent before deploying another.") // @formatter:off Response deployAgent(@PathParam("environment") Environment environment, @PathParam("agentId") String agentId, diff --git a/src/main/java/ai/labs/eddi/engine/internal/RestAgentAdministration.java b/src/main/java/ai/labs/eddi/engine/internal/RestAgentAdministration.java index 60ab7e9e37..811aeac703 100644 --- a/src/main/java/ai/labs/eddi/engine/internal/RestAgentAdministration.java +++ b/src/main/java/ai/labs/eddi/engine/internal/RestAgentAdministration.java @@ -22,6 +22,8 @@ import ai.labs.eddi.engine.runtime.internal.IDeploymentListener; import ai.labs.eddi.engine.runtime.model.DeploymentEvent; import ai.labs.eddi.engine.runtime.service.ServiceException; +import ai.labs.eddi.engine.tenancy.QuotaExceededException; +import ai.labs.eddi.engine.tenancy.TenantQuotaService; import ai.labs.eddi.utils.RuntimeUtilities; import jakarta.enterprise.context.ApplicationScoped; import jakarta.inject.Inject; @@ -50,15 +52,18 @@ public class RestAgentAdministration implements IRestAgentAdministration { private final IDeploymentListener deploymentListener; private final IScheduleStore scheduleStore; private final IRuntime runtime; + private final TenantQuotaService tenantQuotaService; private static final Logger log = Logger.getLogger(RestAgentAdministration.class); @Inject public RestAgentAdministration(IRuntime runtime, IAgentFactory agentFactory, IDeploymentStore deploymentStore, IConversationMemoryStore conversationMemoryStore, IRestConversationStore restConversationStore, - IDocumentDescriptorStore documentDescriptorStore, IDeploymentListener deploymentListener, IScheduleStore scheduleStore) { + IDocumentDescriptorStore documentDescriptorStore, IDeploymentListener deploymentListener, IScheduleStore scheduleStore, + TenantQuotaService tenantQuotaService) { this.runtime = runtime; this.agentFactory = agentFactory; + this.tenantQuotaService = tenantQuotaService; this.deploymentStore = deploymentStore; this.conversationMemoryStore = conversationMemoryStore; this.restConversationStore = restConversationStore; @@ -75,6 +80,12 @@ public Response deployAgent(final Deployment.Environment environment, final Stri RuntimeUtilities.checkNotNull(version, "version"); RuntimeUtilities.checkNotNull(autoDeploy, "autoDeploy"); + // MUST sit before the try below: the catch(Exception) there rethrows as + // InternalServerErrorException, which would turn the mapper's 429 into a 500. + // It must also stay on the request thread — anything inside the submitted + // Callable runs on the runtime executor and can never produce a status code. + enforceAgentQuota(environment, agentId); + try { Future deployFuture = deploy(environment, agentId, version, autoDeploy); @@ -99,7 +110,7 @@ public Response deployAgent(final Deployment.Environment environment, final Stri // Return the actual status after waiting Status status = checkDeploymentStatus(environment, agentId, version); - var responseBody = new java.util.LinkedHashMap(); + var responseBody = new LinkedHashMap(); responseBody.put("status", status.toString()); responseBody.put("agentId", agentId); responseBody.put("version", version); @@ -117,6 +128,68 @@ public Response deployAgent(final Deployment.Environment environment, final Stri } } + /** + * Reject a deployment that would push the tenant past + * {@code maxAgentsPerTenant}. + * + *

+ * Counts distinct agent ids, so redeploying an agent or bumping its + * version is always free — necessary because the old-version undeploy sweep + * legitimately keeps two versions of one agent deployed while the previous one + * drains. + *

+ * + *

+ * The count unions two sources, and needs both. Persisted {@code deployed} rows + * are the source of truth across restarts, but they are only written when + * {@code autoDeploy=true} — while the in-memory deploy is unconditional (see + * {@link #deploy}). Counting rows alone would therefore let a caller deploy + * unlimited agents with {@code autoDeploy=false}, and those agents are + * genuinely live: {@code getLatestReadyAgent} serves them without ever + * consulting the deployment store, and a lazy re-deploy re-materialises them + * after a restart. Counting live agents alone would be per-JVM and would miss + * deployments owned by other cluster nodes. + *

+ * + *

+ * Fails open: a store outage must not block deployments. The denial is + * logged and metered either way. + *

+ */ + private void enforceAgentQuota(Deployment.Environment environment, String agentId) { + Set deployedAgentIds = new HashSet<>(); + + try { + for (DeploymentInfo info : deploymentStore.readDeploymentInfos(DeploymentInfo.DeploymentStatus.deployed)) { + if (info.getAgentId() != null) { + deployedAgentIds.add(info.getAgentId()); + } + } + + for (Deployment.Environment env : Deployment.Environment.values()) { + for (IAgent agent : agentFactory.getAllLatestAgents(env)) { + if (agent.getAgentId() != null && agent.getDeploymentStatus() == READY) { + deployedAgentIds.add(agent.getAgentId()); + } + } + } + } catch (Exception e) { + log.warnf("Agent quota check: could not determine the deployed-agent count, allowing deploy of %s: %s", agentId, e.getMessage()); + return; + } + + // Redeploys and version bumps of an already-counted agent are always allowed. + if (deployedAgentIds.contains(agentId)) { + return; + } + + var result = tenantQuotaService.checkAgentQuota(tenantQuotaService.getDefaultTenantId(), deployedAgentIds.size()); + if (!result.allowed()) { + log.warnf("Denying deployment of Agent %s to %s: %s", agentId, environment, result.reason()); + throw new QuotaExceededException(result.reason()); + } + } + private Future deploy(final Deployment.Environment environment, final String agentId, final Integer version, final Boolean autoDeploy) { Callable deployAgentCallable = () -> { try { diff --git a/src/main/java/ai/labs/eddi/engine/mcp/McpAdminTools.java b/src/main/java/ai/labs/eddi/engine/mcp/McpAdminTools.java index 52ad70adcb..33db745da7 100644 --- a/src/main/java/ai/labs/eddi/engine/mcp/McpAdminTools.java +++ b/src/main/java/ai/labs/eddi/engine/mcp/McpAdminTools.java @@ -31,6 +31,7 @@ import ai.labs.eddi.datastore.serialization.IJsonSerialization; import ai.labs.eddi.engine.api.IRestAgentAdministration; import ai.labs.eddi.engine.runtime.client.factory.IRestInterfaceFactory; +import ai.labs.eddi.engine.tenancy.QuotaExceededException; import ai.labs.eddi.engine.runtime.internal.CronDescriber; import ai.labs.eddi.engine.runtime.internal.CronParser; import ai.labs.eddi.engine.runtime.internal.ScheduleFireExecutor; @@ -142,6 +143,12 @@ public String deployAgent(@ToolArg(description = "Agent ID (required)") String a } return resultJson("deployed", result); + } catch (QuotaExceededException e) { + // Return the quota reason rather than the generic message: an MCP client + // (and the model driving it) cannot self-correct from "check server logs" + // and will retry the deploy in a loop. + LOGGER.warn("MCP deploy_agent denied by quota for Agent " + agentId + ": " + e.getMessage()); + return errorJson(e.getMessage()); } catch (Exception e) { LOGGER.error("MCP deploy_agent failed for Agent " + agentId, e); return errorJson("Failed to deploy agent. Check server logs for details."); diff --git a/src/main/java/ai/labs/eddi/engine/setup/AgentSetupService.java b/src/main/java/ai/labs/eddi/engine/setup/AgentSetupService.java index 24e4c027d1..e8201a60d1 100644 --- a/src/main/java/ai/labs/eddi/engine/setup/AgentSetupService.java +++ b/src/main/java/ai/labs/eddi/engine/setup/AgentSetupService.java @@ -34,6 +34,7 @@ import ai.labs.eddi.engine.model.Deployment; import ai.labs.eddi.engine.runtime.client.factory.IRestInterfaceFactory; import ai.labs.eddi.engine.runtime.client.factory.RestInterfaceFactory; +import ai.labs.eddi.engine.tenancy.QuotaExceededException; import ai.labs.eddi.modules.llm.model.LlmConfiguration; import ai.labs.eddi.modules.output.model.types.TextOutputItem; import ai.labs.eddi.secrets.ISecretProvider; @@ -764,6 +765,17 @@ Map deployAndWait(Deployment.Environment env, String agentId, in result.put("deployed", false); result.put("deployError", "Unexpected deploy response: HTTP " + httpStatus); } + } catch (QuotaExceededException quotaError) { + // agentAdmin is the CDI bean, not an HTTP proxy, so + // QuotaExceededExceptionMapper + // never runs here. Surface the reason verbatim — it is actionable ("undeploy an + // agent first") and the generic branch below would hide it behind "check the + // logs", + // leaving an agent designer or a sub-agent-creating model unable to + // self-correct. + LOGGER.warn("Deploy denied by quota for Agent " + agentId + ": " + quotaError.getMessage()); + result.put("deployed", false); + result.put("deployError", quotaError.getMessage()); } catch (Exception deployError) { LOGGER.warn("Deploy failed for Agent " + agentId, deployError); result.put("deployed", false); diff --git a/src/main/java/ai/labs/eddi/engine/tenancy/TenantQuotaService.java b/src/main/java/ai/labs/eddi/engine/tenancy/TenantQuotaService.java index 3a06d6ed27..fd61581775 100644 --- a/src/main/java/ai/labs/eddi/engine/tenancy/TenantQuotaService.java +++ b/src/main/java/ai/labs/eddi/engine/tenancy/TenantQuotaService.java @@ -149,6 +149,49 @@ public QuotaCheckResult acquireApiCallSlot(String tenantId) { return result; } + // ─── Agent Capacity ─── + + /** + * Read-only capacity gate: checks whether deploying one MORE distinct agent + * would exceed {@link TenantQuota#maxAgentsPerTenant()}. + *

+ * Deliberately shaped like {@link #checkCostBudget} rather than + * {@code acquire*Slot()}: the deployed-agent count is a stock derived + * by counting current deployments, not a per-window flow. A stored + * counter would drift irrecoverably, because several paths add or remove + * deployments without passing through any acquire/release point — the scheduled + * re-deploy sweep, the old-version undeploy sweep, teardown tooling, and the + * lazy re-deploy on first use. Passing the count in also keeps this package + * free of a dependency on the deployment store and keeps the method trivially + * testable. + *

+ * Like {@code checkCostBudget}, this does NOT increment + * {@code quotaAllowedCounter} on the happy path — that counter is documented as + * counting slot acquisitions only. + * + * @param currentDistinctAgents + * number of distinct agent ids currently deployed for this tenant, + * excluding the agent being deployed + */ + public QuotaCheckResult checkAgentQuota(String tenantId, int currentDistinctAgents) { + TenantQuota quota = quotaStore.getQuota(tenantId); + if (quota == null || !quota.enabled()) { + return QuotaCheckResult.OK; + } + + int limit = quota.maxAgentsPerTenant(); + if (limit >= 0 && currentDistinctAgents >= limit) { + quotaDeniedCounter.increment(); + meterRegistry.counter("eddi.tenant.quota.denied", "tenant", tenantId, "type", "agent").increment(); + String reason = String.format("Agent limit (%d) reached for tenant '%s' — undeploy an agent before deploying another", limit, + tenantId); + LOGGER.warn(reason); + return QuotaCheckResult.denied(reason); + } + + return QuotaCheckResult.OK; + } + // ─── Cost Budget ─── /** diff --git a/src/test/java/ai/labs/eddi/engine/internal/RestAgentAdministrationExtendedTest.java b/src/test/java/ai/labs/eddi/engine/internal/RestAgentAdministrationExtendedTest.java index e267e50aef..2b47ff8f11 100644 --- a/src/test/java/ai/labs/eddi/engine/internal/RestAgentAdministrationExtendedTest.java +++ b/src/test/java/ai/labs/eddi/engine/internal/RestAgentAdministrationExtendedTest.java @@ -28,6 +28,9 @@ import org.mockito.ArgumentCaptor; import java.time.Instant; +import ai.labs.eddi.engine.tenancy.TenantQuotaService; +import ai.labs.eddi.engine.tenancy.model.QuotaCheckResult; + import java.util.List; import java.util.concurrent.*; @@ -49,10 +52,11 @@ class RestAgentAdministrationExtendedTest { private IDocumentDescriptorStore documentDescriptorStore; private IDeploymentListener deploymentListener; private IScheduleStore scheduleStore; + private TenantQuotaService tenantQuotaService; private RestAgentAdministration admin; @BeforeEach - void setUp() { + void setUp() throws Exception { runtime = mock(IRuntime.class); agentFactory = mock(IAgentFactory.class); deploymentStore = mock(IDeploymentStore.class); @@ -61,9 +65,15 @@ void setUp() { documentDescriptorStore = mock(IDocumentDescriptorStore.class); deploymentListener = mock(IDeploymentListener.class); scheduleStore = mock(IScheduleStore.class); + tenantQuotaService = mock(TenantQuotaService.class); + // Quota gate transparent by default; the gate itself is covered by + // RestAgentAdministrationQuotaTest. + lenient().when(deploymentStore.readDeploymentInfos(any())).thenReturn(List.of()); + lenient().when(agentFactory.getAllLatestAgents(any())).thenReturn(List.of()); + lenient().when(tenantQuotaService.checkAgentQuota(any(), anyInt())).thenReturn(QuotaCheckResult.OK); admin = new RestAgentAdministration(runtime, agentFactory, deploymentStore, conversationMemoryStore, restConversationStore, documentDescriptorStore, - deploymentListener, scheduleStore); + deploymentListener, scheduleStore, tenantQuotaService); } /** diff --git a/src/test/java/ai/labs/eddi/engine/internal/RestAgentAdministrationQuotaTest.java b/src/test/java/ai/labs/eddi/engine/internal/RestAgentAdministrationQuotaTest.java new file mode 100644 index 0000000000..0bf818613b --- /dev/null +++ b/src/test/java/ai/labs/eddi/engine/internal/RestAgentAdministrationQuotaTest.java @@ -0,0 +1,218 @@ +/* + * Copyright EDDI contributors + * SPDX-License-Identifier: Apache-2.0 + */ +package ai.labs.eddi.engine.internal; + +import ai.labs.eddi.configs.deployment.IDeploymentStore; +import ai.labs.eddi.configs.deployment.model.DeploymentInfo; +import ai.labs.eddi.configs.descriptors.IDocumentDescriptorStore; +import ai.labs.eddi.datastore.IResourceStore; +import ai.labs.eddi.engine.memory.IConversationMemoryStore; +import ai.labs.eddi.engine.memory.rest.IRestConversationStore; +import ai.labs.eddi.engine.model.Deployment; +import ai.labs.eddi.engine.runtime.IAgent; +import ai.labs.eddi.engine.runtime.IAgentFactory; +import ai.labs.eddi.engine.runtime.IRuntime; +import ai.labs.eddi.engine.runtime.internal.IDeploymentListener; +import ai.labs.eddi.engine.schedule.IScheduleStore; +import ai.labs.eddi.engine.tenancy.QuotaExceededException; +import ai.labs.eddi.engine.tenancy.TenantQuotaService; +import ai.labs.eddi.engine.tenancy.model.QuotaCheckResult; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Nested; +import org.junit.jupiter.api.Test; + +import java.util.List; +import java.util.concurrent.Callable; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyInt; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.lenient; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +/** + * Covers the {@code maxAgentsPerTenant} gate on the deploy path. + * + *

+ * Two properties matter most here and are asserted explicitly: the denial must + * surface as {@link QuotaExceededException} (so the mapper turns it into a 429) + * rather than being rewrapped as a 500, and it must happen before any + * work is submitted to the runtime. + *

+ */ +@DisplayName("RestAgentAdministration — maxAgentsPerTenant gate") +class RestAgentAdministrationQuotaTest { + + private static final String TENANT = "default"; + private static final String NEW_AGENT = "aabbccddeeff112233445566"; + + private IRuntime runtime; + private IAgentFactory agentFactory; + private IDeploymentStore deploymentStore; + private TenantQuotaService tenantQuotaService; + private RestAgentAdministration admin; + + @BeforeEach + void setUp() throws Exception { + runtime = mock(IRuntime.class); + agentFactory = mock(IAgentFactory.class); + deploymentStore = mock(IDeploymentStore.class); + tenantQuotaService = mock(TenantQuotaService.class); + + lenient().when(tenantQuotaService.getDefaultTenantId()).thenReturn(TENANT); + lenient().when(agentFactory.getAllLatestAgents(any())).thenReturn(List.of()); + lenient().when(deploymentStore.readDeploymentInfos(any())).thenReturn(List.of()); + lenient().when(tenantQuotaService.checkAgentQuota(anyString(), anyInt())).thenReturn(QuotaCheckResult.OK); + + admin = new RestAgentAdministration(runtime, agentFactory, deploymentStore, + mock(IConversationMemoryStore.class), mock(IRestConversationStore.class), + mock(IDocumentDescriptorStore.class), mock(IDeploymentListener.class), + mock(IScheduleStore.class), tenantQuotaService); + } + + private static DeploymentInfo deployed(String agentId, int version) { + DeploymentInfo info = new DeploymentInfo(); + info.setAgentId(agentId); + info.setAgentVersion(version); + info.setDeploymentStatus(DeploymentInfo.DeploymentStatus.deployed); + return info; + } + + private static IAgent liveAgent(String agentId) { + IAgent agent = mock(IAgent.class); + lenient().when(agent.getAgentId()).thenReturn(agentId); + lenient().when(agent.getDeploymentStatus()).thenReturn(Deployment.Status.READY); + return agent; + } + + private void denyAt(int expectedCount) { + when(tenantQuotaService.checkAgentQuota(eq(TENANT), eq(expectedCount))) + .thenReturn(QuotaCheckResult.denied("Agent limit (" + expectedCount + ") reached")); + } + + @Nested + @DisplayName("denial") + class Denial { + + @Test + @DisplayName("throws QuotaExceededException (not 500) and submits nothing") + void deniesNewAgent() throws Exception { + when(deploymentStore.readDeploymentInfos(any())).thenReturn(List.of(deployed("agent-a", 1), deployed("agent-b", 1))); + denyAt(2); + + QuotaExceededException thrown = assertThrows(QuotaExceededException.class, + () -> admin.deployAgent(Deployment.Environment.production, NEW_AGENT, 1, true, false)); + + // assertThrows already pins the type: if the gate sat inside deployAgent's + // try block, catch(Exception) would have rethrown InternalServerErrorException + // and this would fail — which is exactly the 429-becomes-500 regression. + assertEquals("Agent limit (2) reached", thrown.getMessage()); + verify(runtime, never()).submitCallable(any(Callable.class), any()); + } + } + + @Nested + @DisplayName("counting") + class Counting { + + @Test + @DisplayName("counts distinct agent ids — two versions of one agent count once") + void twoVersionsCountOnce() throws Exception { + when(deploymentStore.readDeploymentInfos(any())).thenReturn(List.of(deployed("agent-a", 1), deployed("agent-a", 2))); + + admin.deployAgent(Deployment.Environment.production, NEW_AGENT, 1, true, false); + + verify(tenantQuotaService).checkAgentQuota(TENANT, 1); + } + + @Test + @DisplayName("redeploying an already-counted agent skips the quota entirely") + void redeployIsFree() throws Exception { + when(deploymentStore.readDeploymentInfos(any())).thenReturn(List.of(deployed(NEW_AGENT, 1))); + denyAt(1); + + admin.deployAgent(Deployment.Environment.production, NEW_AGENT, 2, true, false); + + verify(tenantQuotaService, never()).checkAgentQuota(anyString(), anyInt()); + verify(runtime).submitCallable(any(Callable.class), any()); + } + + @Test + @DisplayName("LOOPHOLE: live agents with no deployed row still count") + void autoDeployFalseAgentsCount() throws Exception { + // autoDeploy=false never writes a `deployed` row, but the in-memory deploy + // is unconditional and getLatestReadyAgent serves such agents without + // consulting the store. Counting rows alone would allow unlimited agents. + // Build the agent mocks BEFORE the outer when(...), or their own stubbing + // runs inside it and Mockito reports UnfinishedStubbing. + List ghosts = List.of(liveAgent("ghost-a"), liveAgent("ghost-b")); + when(deploymentStore.readDeploymentInfos(any())).thenReturn(List.of()); + when(agentFactory.getAllLatestAgents(Deployment.Environment.production)).thenReturn(ghosts); + + admin.deployAgent(Deployment.Environment.production, NEW_AGENT, 1, false, false); + + verify(tenantQuotaService).checkAgentQuota(TENANT, 2); + } + + @Test + @DisplayName("an agent both persisted and live is not double counted") + void noDoubleCount() throws Exception { + List live = List.of(liveAgent("agent-a")); + when(deploymentStore.readDeploymentInfos(any())).thenReturn(List.of(deployed("agent-a", 1))); + when(agentFactory.getAllLatestAgents(Deployment.Environment.production)).thenReturn(live); + + admin.deployAgent(Deployment.Environment.production, NEW_AGENT, 1, true, false); + + verify(tenantQuotaService).checkAgentQuota(TENANT, 1); + } + + @Test + @DisplayName("non-READY agents are not counted") + void skipsNonReadyAgents() throws Exception { + IAgent inProgress = mock(IAgent.class); + lenient().when(inProgress.getAgentId()).thenReturn("pending-a"); + when(inProgress.getDeploymentStatus()).thenReturn(Deployment.Status.IN_PROGRESS); + List pending = List.of(inProgress); + when(agentFactory.getAllLatestAgents(Deployment.Environment.production)).thenReturn(pending); + + admin.deployAgent(Deployment.Environment.production, NEW_AGENT, 1, true, false); + + verify(tenantQuotaService).checkAgentQuota(TENANT, 0); + } + + @Test + @DisplayName("rows with a null agent id are skipped") + void skipsNullAgentIds() throws Exception { + when(deploymentStore.readDeploymentInfos(any())).thenReturn(List.of(deployed(null, 1), deployed("agent-a", 1))); + + admin.deployAgent(Deployment.Environment.production, NEW_AGENT, 1, true, false); + + verify(tenantQuotaService).checkAgentQuota(TENANT, 1); + } + } + + @Nested + @DisplayName("failure isolation") + class FailureIsolation { + + @Test + @DisplayName("fails open when the deployment store errors") + void failsOpenOnStoreError() throws Exception { + when(deploymentStore.readDeploymentInfos(any())).thenThrow(new IResourceStore.ResourceStoreException("mongo down")); + + admin.deployAgent(Deployment.Environment.production, NEW_AGENT, 1, true, false); + + verify(tenantQuotaService, never()).checkAgentQuota(anyString(), anyInt()); + verify(runtime).submitCallable(any(Callable.class), any()); + } + } +} diff --git a/src/test/java/ai/labs/eddi/engine/internal/RestAgentAdministrationTest.java b/src/test/java/ai/labs/eddi/engine/internal/RestAgentAdministrationTest.java index 64cdd65a35..f849ead108 100644 --- a/src/test/java/ai/labs/eddi/engine/internal/RestAgentAdministrationTest.java +++ b/src/test/java/ai/labs/eddi/engine/internal/RestAgentAdministrationTest.java @@ -22,6 +22,9 @@ import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; +import ai.labs.eddi.engine.tenancy.TenantQuotaService; +import ai.labs.eddi.engine.tenancy.model.QuotaCheckResult; + import java.util.List; import java.util.concurrent.*; import java.util.Map; @@ -42,10 +45,11 @@ class RestAgentAdministrationTest { private IDocumentDescriptorStore documentDescriptorStore; private IDeploymentListener deploymentListener; private IScheduleStore scheduleStore; + private TenantQuotaService tenantQuotaService; private RestAgentAdministration restAgentAdmin; @BeforeEach - void setUp() { + void setUp() throws Exception { runtime = mock(IRuntime.class); agentFactory = mock(IAgentFactory.class); deploymentStore = mock(IDeploymentStore.class); @@ -54,9 +58,15 @@ void setUp() { documentDescriptorStore = mock(IDocumentDescriptorStore.class); deploymentListener = mock(IDeploymentListener.class); scheduleStore = mock(IScheduleStore.class); + tenantQuotaService = mock(TenantQuotaService.class); + // Quota gate transparent by default so the existing deploy assertions are + // unaffected; the gate itself is covered by RestAgentAdministrationQuotaTest. + lenient().when(deploymentStore.readDeploymentInfos(any())).thenReturn(List.of()); + lenient().when(agentFactory.getAllLatestAgents(any())).thenReturn(List.of()); + lenient().when(tenantQuotaService.checkAgentQuota(any(), anyInt())).thenReturn(QuotaCheckResult.OK); restAgentAdmin = new RestAgentAdministration(runtime, agentFactory, deploymentStore, conversationMemoryStore, restConversationStore, documentDescriptorStore, - deploymentListener, scheduleStore); + deploymentListener, scheduleStore, tenantQuotaService); } @Nested diff --git a/src/test/java/ai/labs/eddi/engine/tenancy/TenantQuotaServiceTest.java b/src/test/java/ai/labs/eddi/engine/tenancy/TenantQuotaServiceTest.java index b390a06977..53bf864a1f 100644 --- a/src/test/java/ai/labs/eddi/engine/tenancy/TenantQuotaServiceTest.java +++ b/src/test/java/ai/labs/eddi/engine/tenancy/TenantQuotaServiceTest.java @@ -133,6 +133,56 @@ void shouldAllowUnlimitedApiCallsWhenMinusOne() { // --- Cost budget --- + // --- Agent capacity --- + + @Test + @DisplayName("checkAgentQuota — allows when below the agent limit") + void shouldAllowBelowAgentLimit() { + quotaStore.setQuota(new TenantQuota(TENANT_ID, -1, 3, -1, -1.0, true)); + + assertTrue(quotaService.checkAgentQuota(TENANT_ID, 2).allowed()); + } + + @Test + @DisplayName("checkAgentQuota — denies at exactly the agent limit") + void shouldDenyAtAgentLimit() { + quotaStore.setQuota(new TenantQuota(TENANT_ID, -1, 3, -1, -1.0, true)); + + QuotaCheckResult result = quotaService.checkAgentQuota(TENANT_ID, 3); + + assertFalse(result.allowed()); + assertTrue(result.reason().contains("Agent limit (3)")); + assertEquals(1.0, meterRegistry.counter("eddi.tenant.quota.denied", "tenant", TENANT_ID, "type", "agent").count()); + } + + @Test + @DisplayName("checkAgentQuota — -1 means unlimited") + void shouldAllowUnlimitedAgents() { + quotaStore.setQuota(new TenantQuota(TENANT_ID, -1, -1, -1, -1.0, true)); + + assertTrue(quotaService.checkAgentQuota(TENANT_ID, 9999).allowed()); + } + + @Test + @DisplayName("checkAgentQuota — disabled quota allows anything") + void shouldAllowWhenAgentQuotaDisabled() { + quotaStore.setQuota(new TenantQuota(TENANT_ID, -1, 1, -1, -1.0, false)); + + assertTrue(quotaService.checkAgentQuota(TENANT_ID, 100).allowed()); + } + + @Test + @DisplayName("checkAgentQuota — does not touch the allowed counter (parity with checkCostBudget)") + void shouldNotCountAllowedOnAgentCheck() { + quotaStore.setQuota(new TenantQuota(TENANT_ID, -1, 3, -1, -1.0, true)); + + quotaService.checkAgentQuota(TENANT_ID, 1); + + // eddi.tenant.quota.allowed counts slot ACQUISITIONS; a read-only gate must + // not inflate it, or the allowed/denied ratio becomes meaningless. + assertEquals(0.0, meterRegistry.counter("eddi.tenant.quota.allowed").count()); + } + @Test @DisplayName("should allow when within cost budget (pre-call check)") void shouldAllowWithinCostBudget() { From 4eb7ec5cb41d86e6f65885298a525632cc1f3f2f Mon Sep 17 00:00:00 2001 From: Gregor Jarisch Date: Tue, 21 Jul 2026 17:20:31 +0200 Subject: [PATCH 08/12] fix(serialization)!: emit Instant as ISO-8601 on REST, keep storage numeric MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit BREAKING (REST): every java.time.Instant on every endpoint changes from a fractional-epoch-seconds number to an ISO-8601 string. Every Instant rendered as a 1970 date in the Manager. With quarkus.jackson.write-dates-as-timestamps=true plus JavaTimeModule an Instant serializes as fractional epoch SECONDS (1719964800.123), while clients call new Date(value), which expects MILLIS. This hit nextFire, lastFired, pausedAt, createdAt, updatedAt, transcript timestamps — essentially every timestamp in the UI. The obvious fix, a configOverride in the shared configureObjectMapper, is a trap: JsonSerialization injects the same CDI ObjectMapper, and it backs DocumentBuilder for every Mongo write, every Postgres JSONB column, the backup writer and the {json:serialize} Qute extension. That would change on-disk formats. GroupConversation.lastModified is a persisted Instant that GroupConversationStore sorts SERVER-SIDE on: Mongo ranks all Doubles before all Strings, Postgres orders data->>'lastModified' lexicographically, so old and new rows would interleave wrongly and silently. Commit dc117cddc already reverted a broader version of this for breaking findDueSchedules. So the mappers are split: - new @PersistenceMapper qualifier + PersistenceMapperProducer, built from the same configureObjectMapper recipe WITHOUT the date override; JsonSerialization injects that - the Instant override moves into SerializationCustomizer.customize(), the REST/CDI path only, never the shared static java.util.Date is deliberately untouched — it already emits epoch millis and its consumers are correct today. application.properties:174 must stay: Quarkus defaults write-dates-as-timestamps to false, so that line is what prevents Quarkus disabling the feature globally. Deserialization is unaffected either way — InstantDeserializer dispatches on the JSON token type, not the shape hint — so numeric rows still parse. REQUIRES a companion EDDI-Manager change: the Schedules dashboard sorts `(a.nextFire ?? 0) - (b.nextFire ?? 0)`, which is NaN on ISO strings. Use `new Date(a.nextFire) - new Date(b.nextFire)`, which works with both encodings and can land independently. The vendored bundle under META-INF/resources/assets/ is a build artifact and was not hand-edited. NOTE: CDI wiring for the qualified producer could not be validated locally — quarkus:build augmentation needs a loopback socket this environment refuses, and the repo has one non-IT @QuarkusTest. The format behaviour itself is fully unit-tested; CI is the gate for the wiring. --- docs/changelog.md | 36 ++++ .../serialization/JsonSerialization.java | 2 +- .../serialization/PersistenceMapper.java | 38 ++++ .../PersistenceMapperProducer.java | 32 ++++ .../SerializationCustomizer.java | 31 ++++ ...ializationCustomizerInstantFormatTest.java | 167 ++++++++++++++++++ 6 files changed, 305 insertions(+), 1 deletion(-) create mode 100644 src/main/java/ai/labs/eddi/datastore/serialization/PersistenceMapper.java create mode 100644 src/main/java/ai/labs/eddi/datastore/serialization/PersistenceMapperProducer.java create mode 100644 src/test/java/ai/labs/eddi/datastore/serialization/SerializationCustomizerInstantFormatTest.java diff --git a/docs/changelog.md b/docs/changelog.md index da7defc6fe..95c63427c7 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -3,6 +3,42 @@ > **Purpose:** Living document tracking all changes, decisions, and reasoning during implementation. Updated as work progresses for easy reference and review. +--- + +## 🕐 Serialization — split the persistence mapper from the REST mapper; Instant is ISO-8601 on the wire (2026-07-21) + +**Repo:** EDDI (`fix/orphan-scan-and-quota-defects`) + +⚠️ **REST contract change**, and it requires a companion EDDI-Manager fix (below). + +Every `java.time.Instant` on every endpoint rendered as a **1970 date** in the Manager. With `quarkus.jackson.write-dates-as-timestamps=true` (`application.properties:174`) plus `JavaTimeModule`, an `Instant` serializes as fractional epoch **seconds** (`1719964800.123`), while clients call `new Date(value)`, which expects **millis**. Affected `nextFire`, `lastFired`, `pausedAt`, `createdAt`, `updatedAt`, transcript timestamps — essentially every timestamp in the UI. + +The obvious fix — a `configOverride(Instant.class)` in the shared `configureObjectMapper` — is the trap. `JsonSerialization` `@Inject`s the **same CDI `ObjectMapper`**, and it backs `DocumentBuilder` for every Mongo write, every Postgres JSONB column, the backup/export writer, and the `{json:serialize}` Qute extension available to agent authors. That would change **on-disk formats**. Concretely, `GroupConversation.lastModified` is a persisted `Instant` that `GroupConversationStore` sorts **server-side** on: Mongo's BSON cross-type ordering ranks all Doubles before all Strings, and Postgres does `ORDER BY data->>'lastModified'` lexicographically — so old numeric rows and new ISO rows would interleave wrongly, silently, with no backfill. Commit `dc117cddc` ("keep numeric date format") already reverted a broader version of this once for breaking `findDueSchedules`. + +So the two mappers are now separated: + +- New `@PersistenceMapper` qualifier + `PersistenceMapperProducer`, built from the same `configureObjectMapper` recipe **without** the date override. `JsonSerialization` injects that. +- The `Instant` → ISO override moved into `SerializationCustomizer.customize()` — the REST/CDI path only, never the shared static. + +### Design decisions + +- **`java.util.Date` deliberately untouched.** It already emits epoch millis and its consumers (e.g. `DocumentDescriptor.lastModifiedOn`) are correct today; widening the override would break working paths. The wire therefore carries two encodings by design: `Instant` = ISO-8601 string, `Date` = epoch-millis number. +- **`application.properties:174` must stay.** Quarkus's own default for `write-dates-as-timestamps` is `false`, so that line is what *prevents* Quarkus disabling the feature globally. Deleting it as "redundant documentation" would flip persistence to ISO — the exact break this change avoids. +- **A qualified CDI producer, not `new ObjectMapper()` inside `JsonSerialization`.** Keeps the persistence mapper a first-class, overridable bean and leaves the existing direct-construction test fixtures working unchanged. +- **Deserialization is unaffected in both directions** — `InstantDeserializer` dispatches on the JSON token type, not the shape hint — so rows written numerically still parse. + +### Required companion change (EDDI-Manager repo) + +The Schedules dashboard sorts arithmetically: `(a.nextFire ?? 0) - (b.nextFire ?? 0)`, which yields `NaN` on ISO strings, leaving the "next fire" tile showing an arbitrary schedule. Change it to `new Date(a.nextFire) - new Date(b.nextFire)`, which works with **both** encodings and can therefore land before or after this commit. Every other consumer (`new Date(x).toLocaleString()`) becomes correct automatically. The vendored bundle under `META-INF/resources/assets/` is a build artifact and was deliberately not hand-edited. + +### Verification limits + +CDI wiring for the new qualified producer **cannot be validated locally** — `quarkus:build` augmentation needs a loopback socket, which this environment refuses, and the repo has only one non-IT `@QuarkusTest`. CI is the gate for that specific aspect. The format behaviour itself is fully covered by unit tests. + +### Tests + +New `SerializationCustomizerInstantFormatTest` (7 tests) asserts both halves: the REST mapper emits ISO and keeps `Date` numeric; the persistence mapper stays numeric and **does not inherit** the REST override despite sharing `configureObjectMapper`; and both still read the old numeric form. + --- ## 🎫 Tenancy — enforce `maxAgentsPerTenant` on deploy (2026-07-21) diff --git a/src/main/java/ai/labs/eddi/datastore/serialization/JsonSerialization.java b/src/main/java/ai/labs/eddi/datastore/serialization/JsonSerialization.java index 5504c3129c..06abb12326 100644 --- a/src/main/java/ai/labs/eddi/datastore/serialization/JsonSerialization.java +++ b/src/main/java/ai/labs/eddi/datastore/serialization/JsonSerialization.java @@ -19,7 +19,7 @@ public final class JsonSerialization implements IJsonSerialization { private final ObjectMapper objectMapper; @Inject - public JsonSerialization(ObjectMapper objectMapper) { + public JsonSerialization(@PersistenceMapper ObjectMapper objectMapper) { this.objectMapper = objectMapper; } diff --git a/src/main/java/ai/labs/eddi/datastore/serialization/PersistenceMapper.java b/src/main/java/ai/labs/eddi/datastore/serialization/PersistenceMapper.java new file mode 100644 index 0000000000..19f8b52cae --- /dev/null +++ b/src/main/java/ai/labs/eddi/datastore/serialization/PersistenceMapper.java @@ -0,0 +1,38 @@ +/* + * Copyright EDDI contributors + * SPDX-License-Identifier: Apache-2.0 + */ +package ai.labs.eddi.datastore.serialization; + +import jakarta.inject.Qualifier; + +import java.lang.annotation.Retention; +import java.lang.annotation.Target; + +import static java.lang.annotation.ElementType.FIELD; +import static java.lang.annotation.ElementType.METHOD; +import static java.lang.annotation.ElementType.PARAMETER; +import static java.lang.annotation.ElementType.TYPE; +import static java.lang.annotation.RetentionPolicy.RUNTIME; + +/** + * Selects the {@code ObjectMapper} used for storage rather + * than for HTTP responses. + * + *

+ * The two must not be the same instance. The CDI/REST mapper deliberately emits + * {@code java.time.Instant} as an ISO-8601 string so clients render timestamps + * correctly, but the storage format has to stay numeric: several stores write + * date fields as epoch-millis for range queries, sort server-side on persisted + * timestamps, and read existing rows that were written numerically. Changing + * the stored shape would silently reorder query results and strand old + * documents rather than failing loudly. + *

+ * + * @see SerializationCustomizer + */ +@Qualifier +@Retention(RUNTIME) +@Target({TYPE, METHOD, FIELD, PARAMETER}) +public @interface PersistenceMapper { +} diff --git a/src/main/java/ai/labs/eddi/datastore/serialization/PersistenceMapperProducer.java b/src/main/java/ai/labs/eddi/datastore/serialization/PersistenceMapperProducer.java new file mode 100644 index 0000000000..11c23f7db4 --- /dev/null +++ b/src/main/java/ai/labs/eddi/datastore/serialization/PersistenceMapperProducer.java @@ -0,0 +1,32 @@ +/* + * Copyright EDDI contributors + * SPDX-License-Identifier: Apache-2.0 + */ +package ai.labs.eddi.datastore.serialization; + +import com.fasterxml.jackson.databind.ObjectMapper; +import jakarta.enterprise.context.ApplicationScoped; +import jakarta.enterprise.inject.Produces; + +/** + * Produces the {@link PersistenceMapper}-qualified {@code ObjectMapper}. + * + *

+ * Built from the same {@link SerializationCustomizer#configureObjectMapper} + * recipe as the CDI/REST mapper, but without the REST-only + * {@code Instant} format override that {@code customize()} applies. Everything + * that affects how a document is shaped — null inclusion, unknown-property + * tolerance, {@code JavaTimeModule} — is shared, so stored documents keep + * exactly the shape they have today. + *

+ */ +@ApplicationScoped +public class PersistenceMapperProducer { + + @Produces + @ApplicationScoped + @PersistenceMapper + public ObjectMapper persistenceMapper() { + return SerializationCustomizer.configureObjectMapper(new ObjectMapper(), false); + } +} diff --git a/src/main/java/ai/labs/eddi/datastore/serialization/SerializationCustomizer.java b/src/main/java/ai/labs/eddi/datastore/serialization/SerializationCustomizer.java index 26eb3a999c..6921c8c595 100644 --- a/src/main/java/ai/labs/eddi/datastore/serialization/SerializationCustomizer.java +++ b/src/main/java/ai/labs/eddi/datastore/serialization/SerializationCustomizer.java @@ -4,6 +4,7 @@ */ package ai.labs.eddi.datastore.serialization; +import com.fasterxml.jackson.annotation.JsonFormat; import com.fasterxml.jackson.annotation.JsonInclude; import com.fasterxml.jackson.databind.DeserializationFeature; import com.fasterxml.jackson.databind.ObjectMapper; @@ -15,6 +16,8 @@ import jakarta.inject.Inject; import jakarta.inject.Singleton; +import java.time.Instant; + import static com.fasterxml.jackson.databind.SerializationFeature.INDENT_OUTPUT; @Singleton @@ -29,6 +32,34 @@ public SerializationCustomizer(@ConfigProperty(name = "json.prettyPrint") Boolea @Override public void customize(ObjectMapper objectMapper) { configureObjectMapper(objectMapper, prettyPrint); + + // REST-ONLY. Emit java.time.Instant as an ISO-8601 string instead of the + // fractional epoch SECONDS that quarkus.jackson.write-dates-as-timestamps=true + // produces via JavaTimeModule. Clients call new Date(value), which expects + // MILLIS, so ~1.7e9 rendered as a 1970 date on every timestamp in the UI. + // + // This deliberately lives here and NOT in configureObjectMapper: that static is + // the shared recipe, and the @PersistenceMapper mapper is built from it. + // Storage + // must keep the numeric shape — MongoScheduleStore normalizes dates to + // epoch-millis for range queries, GroupConversationStore sorts server-side on a + // persisted Instant (mixed numeric/string rows would interleave wrongly in both + // Mongo and Postgres), and existing rows were written numerically. See commit + // dc117cddc, which reverted a broader version of this change for breaking + // findDueSchedules. + // + // configOverride lives in MapperConfig._configOverrides, not the + // SerializationFeature bitmask, so it is order-independent with respect to when + // Quarkus applies write-dates-as-timestamps: JSR310FormattedSerializerBase + // .createContextual consults the per-type override first and only falls back to + // the feature flag when none exists. Deserialization is unaffected — + // InstantDeserializer dispatches on the JSON token type, not the shape hint — + // so + // numeric values written before this change still parse. + // + // Scoped to Instant only: java.util.Date already emits epoch millis and its + // consumers are correct today, so widening this would break working paths. + objectMapper.configOverride(Instant.class).setFormat(JsonFormat.Value.forShape(JsonFormat.Shape.STRING)); } public static ObjectMapper configureObjectMapper(ObjectMapper objectMapper, Boolean prettyPrint) { diff --git a/src/test/java/ai/labs/eddi/datastore/serialization/SerializationCustomizerInstantFormatTest.java b/src/test/java/ai/labs/eddi/datastore/serialization/SerializationCustomizerInstantFormatTest.java new file mode 100644 index 0000000000..4305a0f722 --- /dev/null +++ b/src/test/java/ai/labs/eddi/datastore/serialization/SerializationCustomizerInstantFormatTest.java @@ -0,0 +1,167 @@ +/* + * Copyright EDDI contributors + * SPDX-License-Identifier: Apache-2.0 + */ +package ai.labs.eddi.datastore.serialization; + +import com.fasterxml.jackson.databind.ObjectMapper; +import com.fasterxml.jackson.databind.SerializationFeature; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Nested; +import org.junit.jupiter.api.Test; + +import java.time.Instant; +import java.util.Date; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * Pins the REST/persistence split of the JSON date format. + * + *

+ * The deployment sets {@code quarkus.jackson.write-dates-as-timestamps=true}, + * so {@code JavaTimeModule} renders an {@link Instant} as fractional epoch + * seconds. Clients call {@code new Date(value)}, which expects + * millis, so every timestamp showed as a 1970 date. + *

+ * + *

+ * The fix applies only to the CDI/REST mapper. The + * {@link PersistenceMapper}-qualified mapper must keep the numeric shape, + * because stores write epoch-millis for range queries and sort server-side on + * persisted timestamps — mixing formats would reorder results silently. These + * tests assert both halves, since a change that "fixed" both would be the + * regression commit {@code dc117cddc} already reverted once. + *

+ */ +@DisplayName("SerializationCustomizer — REST vs persistence date format") +class SerializationCustomizerInstantFormatTest { + + private static final Instant FIXED = Instant.ofEpochMilli(1719964800123L); + + /** The CDI/REST mapper: customizer + the Quarkus timestamp flag. */ + private static ObjectMapper restMapper() { + ObjectMapper mapper = new ObjectMapper(); + mapper.configure(SerializationFeature.WRITE_DATES_AS_TIMESTAMPS, true); + new SerializationCustomizer(false).customize(mapper); + return mapper; + } + + /** Exactly what PersistenceMapperProducer builds. */ + private static ObjectMapper persistenceMapper() { + ObjectMapper mapper = SerializationCustomizer.configureObjectMapper(new ObjectMapper(), false); + mapper.configure(SerializationFeature.WRITE_DATES_AS_TIMESTAMPS, true); + return mapper; + } + + @Nested + @DisplayName("REST mapper") + class Rest { + + @Test + @DisplayName("Instant serializes as an ISO-8601 string") + void instantIsIsoString() throws Exception { + assertEquals("{\"value\":\"2024-07-03T00:00:00.123Z\"}", restMapper().writeValueAsString(new InstantHolder(FIXED))); + } + + @Test + @DisplayName("java.util.Date stays epoch millis so working clients keep working") + void dateStaysNumeric() throws Exception { + assertEquals("{\"value\":1719964800123}", restMapper().writeValueAsString(new DateHolder(new Date(1719964800123L)))); + } + } + + @Nested + @DisplayName("persistence mapper") + class Persistence { + + @Test + @DisplayName("Instant stays NUMERIC — the stored shape must not change") + void instantStaysNumeric() throws Exception { + String json = persistenceMapper().writeValueAsString(new InstantHolder(FIXED)); + + assertTrue(json.matches("\\{\"value\":[0-9.]+}"), + "persisted Instants must stay numeric or server-side sorts and range queries break; got: " + json); + } + + @Test + @DisplayName("does not inherit the REST override even though both share configureObjectMapper") + void doesNotInheritRestOverride() throws Exception { + String rest = restMapper().writeValueAsString(new InstantHolder(FIXED)); + String persisted = persistenceMapper().writeValueAsString(new InstantHolder(FIXED)); + + assertTrue(rest.contains("\""), "REST form should be a quoted string, got: " + rest); + assertTrue(!persisted.contains("\"value\":\""), "persistence form must not be quoted, got: " + persisted); + } + } + + @Nested + @DisplayName("deserialization back-compatibility") + class Deserialization { + + @Test + @DisplayName("REST mapper still reads the old fractional-epoch-seconds form") + void restReadsNumeric() throws Exception { + assertEquals(FIXED, restMapper().readValue("{\"value\":1719964800.123000000}", InstantHolder.class).getValue()); + } + + @Test + @DisplayName("persistence mapper reads BOTH numeric and ISO, so existing rows survive") + void persistenceReadsBoth() throws Exception { + ObjectMapper mapper = persistenceMapper(); + + assertEquals(FIXED, mapper.readValue("{\"value\":1719964800.123000000}", InstantHolder.class).getValue()); + assertEquals(FIXED, mapper.readValue("{\"value\":\"2024-07-03T00:00:00.123Z\"}", InstantHolder.class).getValue()); + } + + @Test + @DisplayName("each mapper round-trips its own output") + void roundTrips() throws Exception { + ObjectMapper rest = restMapper(); + ObjectMapper persisted = persistenceMapper(); + + assertEquals(FIXED, rest.readValue(rest.writeValueAsString(new InstantHolder(FIXED)), InstantHolder.class).getValue()); + assertEquals(FIXED, + persisted.readValue(persisted.writeValueAsString(new InstantHolder(FIXED)), InstantHolder.class).getValue()); + } + } + + public static class InstantHolder { + private Instant value; + + public InstantHolder() { + } + + public InstantHolder(Instant value) { + this.value = value; + } + + public Instant getValue() { + return value; + } + + public void setValue(Instant value) { + this.value = value; + } + } + + public static class DateHolder { + private Date value; + + public DateHolder() { + } + + public DateHolder(Date value) { + this.value = value; + } + + public Date getValue() { + return value; + } + + public void setValue(Date value) { + this.value = value; + } + } +} From 15b7a08a7c4f7085bdc4fd2bda9fb41b63fb1f19 Mon Sep 17 00:00:00 2001 From: Gregor Jarisch Date: Tue, 21 Jul 2026 17:37:13 +0200 Subject: [PATCH 09/12] fix(llm): stop discarding agent-mode token usage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit AgentOrchestrator sums TokenUsage across every model call in the tool loop and returns it on ExecutionResult.responseMetadata(), on both the live path (AgentOrchestrator.java:826) and the resume path (:470). LlmTask read .response() and .trace() from that result and never .responseMetadata(), so agent-mode token accounting was computed and dropped on the floor — only the legacy-chat and cascade branches surfaced theirs. Any agent with tools enabled therefore reported {} for responseMetadataObjectName, and no per-turn token figure existed for the paths that dominate real usage. This is the prerequisite for monthly cost metering: there was nothing to meter. Both agent branches now read the metadata, and executeResume surfaces it the way executeTask does (it previously built no metadata map at all). Known gap: a turn that pauses for tool approval loses its pre-pause usage. ToolApprovalRequiredException escapes before the metadata is assembled and carries no usage, and resumeToolLoop starts a fresh accumulator, so a paused turn under-reports by its pre-pause segment. Closing that needs the step threaded into runToolCallLoop so usage is written incrementally. Safety: responseMetadata also feeds applyResponseValidation, which branches on `warning` and `streamingTimeout`. AgentOrchestrator puts only `tokenUsage` into the map, so both keys stay absent exactly as with the previously-empty map — validation behaviour is unchanged. NOT unit-covered: LlmTask constructs its own AgentOrchestrator rather than receiving it injected, so executeIfToolsEnabled cannot be stubbed and every existing LlmTask test exercises only the legacy branch. Verified by reading both sides of the contract plus the validation check above; all 184 LlmTask tests stay green. Making this testable means injecting AgentOrchestrator — a separate refactor of an already 29-argument constructor. --- docs/changelog.md | 24 +++++++++++++++++++ .../labs/eddi/modules/llm/impl/LlmTask.java | 21 ++++++++++++++++ 2 files changed, 45 insertions(+) diff --git a/docs/changelog.md b/docs/changelog.md index 95c63427c7..0870c8fe58 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -3,6 +3,30 @@ > **Purpose:** Living document tracking all changes, decisions, and reasoning during implementation. Updated as work progresses for easy reference and review. +--- + +## 🔢 LLM — stop discarding agent-mode token usage (2026-07-21) + +**Repo:** EDDI (`fix/orphan-scan-and-quota-defects`) + +`AgentOrchestrator` sums `TokenUsage` across every model call in the tool loop and returns it on `ExecutionResult.responseMetadata()` — on both the live path and the resume path. `LlmTask` then read `.response()` and `.trace()` from that result and **never `.responseMetadata()`**, so agent-mode token accounting was computed and dropped on the floor. Only the legacy-chat and cascade branches surfaced theirs. + +Net effect: any agent with tools enabled reported `{}` for `responseMetadataObjectName`, and no per-turn token figure existed for the paths that dominate real usage. This is the prerequisite for monthly cost metering — there was nothing to meter. + +Both agent branches now read the metadata, and `executeResume` surfaces it the same way `executeTask` does (it previously built no metadata map at all). + +### Known gap, deliberately documented rather than papered over + +A turn that pauses for tool approval loses its pre-pause usage: `ToolApprovalRequiredException` escapes before the metadata is assembled and carries no usage, and `resumeToolLoop` starts a fresh accumulator. So a paused turn under-reports by its pre-pause segment. Closing that requires threading the step into `runToolCallLoop` so usage is written incrementally — out of scope here. + +### Safety check + +`responseMetadata` also feeds `applyResponseValidation`, which branches on `warning` and `streamingTimeout`. `AgentOrchestrator` puts **only** `tokenUsage` into the map (`AgentOrchestrator.java:470,826`), so both of those keys stay absent exactly as they were with the previously-empty map — validation behaviour is unchanged. + +### Verification limits + +Not unit-covered: `LlmTask` constructs its `AgentOrchestrator` internally rather than receiving it injected, so `executeIfToolsEnabled` cannot be stubbed to return a non-null result at the `LlmTask` unit level, and every existing `LlmTask` test therefore exercises the legacy branch. The change was verified by reading both sides of the contract and by the safety check above; all 184 `LlmTask` tests stay green. Making this properly testable means injecting `AgentOrchestrator` — a worthwhile refactor of an already 29-argument constructor, but a separate change. + --- ## 🕐 Serialization — split the persistence mapper from the REST mapper; Instant is ISO-8601 on the wire (2026-07-21) diff --git a/src/main/java/ai/labs/eddi/modules/llm/impl/LlmTask.java b/src/main/java/ai/labs/eddi/modules/llm/impl/LlmTask.java index ba6efc0f85..50574ef7f6 100644 --- a/src/main/java/ai/labs/eddi/modules/llm/impl/LlmTask.java +++ b/src/main/java/ai/labs/eddi/modules/llm/impl/LlmTask.java @@ -572,6 +572,10 @@ private void executeTask(IConversationMemory memory, Task task, IWritableConvers if (agentResult != null) { responseContent = agentResult.response(); toolTrace = agentResult.trace(); + // AgentOrchestrator sums TokenUsage across every model call in the tool + // loop and returns it here; not reading it dropped all agent-mode token + // accounting on the floor while the legacy branches below kept theirs. + responseMetadata = agentResult.responseMetadata(); usedToolMode = true; if (eventSink != null && responseContent != null && !addToOutputExplicitlyFalse) { eventSink.onToken(responseContent); @@ -597,6 +601,9 @@ private void executeTask(IConversationMemory memory, Task task, IWritableConvers // available responseContent = agentResult.response(); toolTrace = agentResult.trace(); + // See the skipCascade branch above: without this, agent-mode token usage + // is computed by AgentOrchestrator and then silently discarded. + responseMetadata = agentResult.responseMetadata(); usedToolMode = true; // Stream the final agent response if streaming is active if (eventSink != null && responseContent != null && !addToOutputExplicitlyFalse) { @@ -886,8 +893,22 @@ private void executeResume(IConversationMemory memory, LlmConfiguration llmConfi String responseContent = result != null ? result.response() : null; List> toolTrace = result != null && result.trace() != null ? result.trace() : new ArrayList<>(); + Map responseMetadata = result != null && result.responseMetadata() != null + ? result.responseMetadata() + : new HashMap<>(); // === Store the result EXACTLY like the normal path (executeTask) === + // Surface metadata the same way executeTask does. Note the continuation's + // tokenUsage covers only the post-resume model calls: the usage accumulated + // before the pause dies with ToolApprovalRequiredException and resumeToolLoop + // starts a fresh accumulator, so a paused turn under-reports by its pre-pause + // segment. + var responseMetadataObjectName = task.getResponseMetadataObjectName(); + if (!isNullOrEmpty(responseMetadataObjectName)) { + templateDataObjects.put(responseMetadataObjectName, responseMetadata); + prePostUtils.createMemoryEntry(currentStep, responseMetadata, responseMetadataObjectName, KEY_LANGCHAIN); + } + var responseObjectName = task.getResponseObjectName(); if (isNullOrEmpty(responseObjectName)) { responseObjectName = task.getId(); From 156faf8ca6248f378f8ffef60c1e1674d69cf4bc Mon Sep 17 00:00:00 2001 From: Gregor Jarisch Date: Tue, 21 Jul 2026 17:38:06 +0200 Subject: [PATCH 10/12] docs(specs): record the second round and the refuted cost-metering seam MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Marks includeDeleted, maxAgentsPerTenant, the mapper split and the agent-mode token-usage fix as shipped, and rewrites the cost-metering section: ObservableChatModel was refuted as the interception point (it is optional, never wraps streaming, is shared across conversations with no attribution context, and never reads TokenUsage). Records the verified six-site seam and the three questions that must be settled before wiring recordCost — price-field precedence, the dormant Mongo E11000 that goes live on first call, and the LLM calls that are structurally uncounted. --- ...6-07-21-manager-coverage-backend-design.md | 33 ++++++++++++++++++- 1 file changed, 32 insertions(+), 1 deletion(-) diff --git a/docs/superpowers/specs/2026-07-21-manager-coverage-backend-design.md b/docs/superpowers/specs/2026-07-21-manager-coverage-backend-design.md index bd607887ca..bae9549456 100644 --- a/docs/superpowers/specs/2026-07-21-manager-coverage-backend-design.md +++ b/docs/superpowers/specs/2026-07-21-manager-coverage-backend-design.md @@ -50,8 +50,28 @@ Separately the reference scan failed **open**: read errors were swallowed (two a Both new behavioural tests were **mutation-checked** — reverting the production change makes each fail — so neither is vacuous. +## Shipped in the second round + +### 4. `fix(admin)!` — `includeDeleted` becomes a true inclusion flag (`bd3f8bdbf`) + +It was an equality filter, so `true` matched *only* soft-deleted descriptors. The Manager scans with `false` and purges with `true`, so the set shown and the set deleted were disjoint. `purgeOrphans`'s default flipped `true` → `false`: left at `true`, the semantics fix plus the earlier page-walk fix would have turned the parameterless `DELETE` into "permanently wipe every unreferenced resource, unbounded". + +### 5. `feat(tenancy)` — `maxAgentsPerTenant` enforcement (`a4d368683`) + +Read-only gate, not a counter (several paths mutate deployments without passing any acquire point). Placed between the null-checks and the `try`, because inside it `catch (Exception)` converts the 429 to a 500. Counts distinct agent ids, unioning persisted `deployed` rows with live `READY` agents — `autoDeploy=false` writes no row but deploys unconditionally, and the resulting agent is **durable**, since `ConversationService.getAgent` lazily re-deploys it after restart. + +### 6. `fix(serialization)!` — persistence/REST mapper split (`4eb7ec5cb`) + +New `@PersistenceMapper` qualifier; the `Instant` → ISO override lives in `customize()` only. See the deferral note below for why the shared static could not be touched. + +### 7. `fix(llm)` — agent-mode token usage (`15b7a08a7`) + +`LlmTask` never read `agentResult.responseMetadata()`, so usage the orchestrator had already summed was discarded. Prerequisite for any metering. + ## Deliberately not shipped +> **Superseded:** the C3 and C4 deferrals below were resolved in the second round (`4eb7ec5cb`, `a4d368683`). They are kept for the reasoning, which still governs how *not* to change the shared mapper. + ### C3 as designed — repo-wide `Instant` → ISO-8601 Approved as "repo-wide via `SerializationCustomizer`", implemented, then **backed out** when the critique surfaced a blocker that invalidated the basis of the decision. @@ -95,7 +115,18 @@ No blockers, several confirmed majors: ### C5 — cost metering -**Blocked on a blocker, not just majors.** The design accumulated per-turn USD from token usage, but agent mode and the HITL resume path *discard* `tokenUsage` — so the accumulator would read ~0 on exactly the paths the change targets. `ObservableChatModel` is the correct interception point, not `LlmTask`. +**Still open. The diagnosis was right; the prescription was wrong.** + +A later verification pass **refuted `ObservableChatModel` as the seam** on four independent counts: `wrapIfNeeded` returns the raw model unless timeout/logging is configured, so default agents are unwrapped; it never wraps streaming (it implements `ChatModel`, not `StreamingChatModel`), so the SSE and cascade live-stream paths are invisible; instances are `@ApplicationScoped`-cached and shared across all conversations with the observability params stripped from the cache key, so it has **zero attribution context** and its presence is nondeterministic; and its timeout path runs the delegate on a shared daemon thread. It also never reads `TokenUsage` today. + +The actual loss was in `LlmTask`, and that half is now **fixed** (`15b7a08a7`). + +**Remaining work, with the verified seam:** a small `LlmUsageRecorder` called at the six sites that already hold a `ChatResponse` — `AgentOrchestrator:909`, `LegacyChatExecutor:110`, `StreamingLegacyChatExecutor:297`, `ToolResponseTruncator:236`, `SummarizationService:133`, `ConfidenceEvaluator:254`. That set is closed: a grep for `UserMessage.from|SystemMessage.from` across `src/main/java` returns 8 files, of which only these construct model calls. Then per-turn accumulation into step data and `recordTurnCost` at the two `ConversationService` settle points. + +Three things must be settled first: +1. **Precedence between three price locations.** `inputPricePer1M`/`outputPricePer1M` already exist on `CascadeStep` *and* `ModelCascadeConfig`, are validated, documented, and already drive `runCostUsd`. A third copy on `Task` double-counts cascade turns unless cascade config wins for cascade tasks and task-level applies only to non-cascade paths. +2. **The Mongo E11000 goes live.** `tryAddCost` upserts against a filter pinning `costMonth` while `tenant_usage` has a unique index on `tenantId`, so the first cost of each new UTC month attempts a duplicate insert. Dormant only because nothing calls `recordCost`. `MongoTenantQuotaStoreTest:369` pins the current two-call sequence and needs rewriting. +3. **Uncounted spend.** Judge-model calls, tool-response summarization and the rolling summary all make in-turn LLM calls whose usage is never captured; tool cost is structurally $0 because `ToolCostTracker`'s map is keyed `"websearch"` while the real name is `searchWeb`. Additional confirmed problems: - `inputPricePer1M`/`outputPricePer1M` already exist on `CascadeStep` **and** `ModelCascadeConfig`. A third copy on `Task` needs a stated precedence rule or it double-counts cascade turns against `runCostUsd`, which `LlmTask` already surfaces as `cascadeCostUsd`. From a186dc9034e944c610da3b610c75ca2061da4ae4 Mon Sep 17 00:00:00 2001 From: Gregor Jarisch Date: Wed, 22 Jul 2026 12:03:11 +0200 Subject: [PATCH 11/12] fix: address PR review; make the MAX_PAGES ceiling test non-vacuous MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeRabbit noted the new page ceiling had no coverage. Adding a test made it pass immediately — but mutation-checking it (disabling the ceiling so the walk truncates silently) showed it STILL passed: the fixture left agentStore.read unstubbed, so the traversal NPE'd on a null config, marked the scan incomplete, and produced the expected 409 for the wrong reason. Stubbing the agent read so the ceiling is the only possible failure source, and asserting the refusal names it, makes the test load-bearing. The mutant now dies with "Expected WebApplicationException to be thrown, but nothing was thrown." Copilot flagged that the produced @PersistenceMapper does not set WRITE_DATES_AS_TIMESTAMPS. The stated premise is wrong — Jackson enables that feature by default, so the producer already emitted numeric — but the real defect was next door: SerializationCustomizerInstantFormatTest RECONSTRUCTED the producer instead of calling it, and set the flag itself, so it would have passed even if the producer were broken. The test now builds the mapper via new PersistenceMapperProducer().persistenceMapper(), and the producer states the flag explicitly. Relying on a library default for a persistence-format guarantee is what dc117cddc was reverted for. Mutation-checked: flipping the producer to ISO fails two tests. CodeRabbit correctly flagged the TOCTOU on the agent quota: concurrent deploys observing count == limit - 1 all pass. An earlier internal note called this self-correcting, which was wrong — once over, the gate merely refuses further deploys until an undeploy brings the count down. The javadoc now states the bound honestly and explains why a per-tenant lock is NOT used: it would serialize within one JVM while the count spans the shared deployment store and every node's in-memory registry, giving the appearance of a hard guarantee exactly where it would not hold. Accepted because deploys are rare admin operations and the gate's purpose — stopping runaway growth such as an LLM creating sub-agents in a loop — survives a small transient overrun. Also sanitize(conversationId) on the new quota-denial log line, matching the rest of RestAgentEngine. Not actioned: the advisory that the orphan endpoint blocks a request thread. Pre-existing, raised as advice rather than a blocker, and moving it to AsyncResponse is a separate change. --- docs/changelog.md | 18 ++++++++++++ .../PersistenceMapperProducer.java | 13 ++++++++- .../internal/RestAgentAdministration.java | 23 +++++++++++++++ .../eddi/engine/internal/RestAgentEngine.java | 2 +- .../admin/rest/RestOrphanAdminSafetyTest.java | 28 +++++++++++++++++++ ...ializationCustomizerInstantFormatTest.java | 11 +++++--- 6 files changed, 89 insertions(+), 6 deletions(-) diff --git a/docs/changelog.md b/docs/changelog.md index 0870c8fe58..af3c4aa689 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -3,6 +3,24 @@ > **Purpose:** Living document tracking all changes, decisions, and reasoning during implementation. Updated as work progresses for easy reference and review. +--- + +## 🔍 PR review follow-ups — a vacuous test found and fixed (2026-07-21) + +**Repo:** EDDI (`fix/orphan-scan-and-quota-defects`) + +Addressing CodeRabbit and Copilot review comments. One of them exposed a test that could not fail. + +**The `MAX_PAGES` ceiling test was vacuous.** CodeRabbit noted the new page ceiling had no coverage. Adding a test made it pass immediately — but mutation-checking it (disabling the ceiling so the walk truncates silently) showed it *still* passed. Cause: the fixture left `agentStore.read` unstubbed, so the traversal NPE'd on a null config, marked the scan incomplete, and produced the expected 409 **for the wrong reason**. Stubbing the agent read so the ceiling is the only possible failure source, plus asserting the refusal message names it, makes the test load-bearing — the mutant now dies with "Expected WebApplicationException to be thrown, but nothing was thrown." + +**Copilot's `WRITE_DATES_AS_TIMESTAMPS` finding: premise wrong, instinct right.** It claimed the produced `@PersistenceMapper` could change the on-disk shape. It cannot — Jackson enables that feature by default, so the producer already emitted numeric. But the real defect was next door: `SerializationCustomizerInstantFormatTest` *reconstructed* the producer instead of calling it, and set the flag itself — so it would have passed even if the producer were broken. The test now builds the mapper via `new PersistenceMapperProducer().persistenceMapper()`, and the producer states the flag explicitly. Relying on a library default for a persistence-format guarantee is precisely what `dc117cddc` was reverted for. Mutation-checked: flipping the producer to ISO now fails two tests. + +**TOCTOU on the agent quota — documented, not fixed, and the earlier claim corrected.** CodeRabbit correctly flagged that concurrent deploys observing `count == limit - 1` all pass. An internal note had called this "self-correcting"; that was wrong — once over, the gate merely refuses further deploys until an undeploy brings the count down. The javadoc now states the bound honestly and explains why a per-tenant lock is *not* used: it would serialize within one JVM while the count spans the shared store and every node's registry, giving the appearance of a hard guarantee exactly where it would not hold. Accepted because deploys are rare admin operations and the gate's purpose — stopping runaway growth such as an LLM creating sub-agents in a loop — survives a small transient overrun. + +**Log injection.** `sanitize(conversationId)` added to the new quota-denial log line in `RestAgentEngine`, matching the rest of the class (lines 331, 368, 389, 436). + +**Not actioned:** the advisory note that the orphan endpoint blocks a request thread. Pre-existing, explicitly raised as advice rather than a blocker, and moving it to `AsyncResponse` with a polling status endpoint is a separate change. + --- ## 🔢 LLM — stop discarding agent-mode token usage (2026-07-21) diff --git a/src/main/java/ai/labs/eddi/datastore/serialization/PersistenceMapperProducer.java b/src/main/java/ai/labs/eddi/datastore/serialization/PersistenceMapperProducer.java index 11c23f7db4..398c2582c9 100644 --- a/src/main/java/ai/labs/eddi/datastore/serialization/PersistenceMapperProducer.java +++ b/src/main/java/ai/labs/eddi/datastore/serialization/PersistenceMapperProducer.java @@ -5,6 +5,7 @@ package ai.labs.eddi.datastore.serialization; import com.fasterxml.jackson.databind.ObjectMapper; +import com.fasterxml.jackson.databind.SerializationFeature; import jakarta.enterprise.context.ApplicationScoped; import jakarta.enterprise.inject.Produces; @@ -27,6 +28,16 @@ public class PersistenceMapperProducer { @ApplicationScoped @PersistenceMapper public ObjectMapper persistenceMapper() { - return SerializationCustomizer.configureObjectMapper(new ObjectMapper(), false); + ObjectMapper mapper = SerializationCustomizer.configureObjectMapper(new ObjectMapper(), false); + + // Redundant against today's Jackson default (WRITE_DATES_AS_TIMESTAMPS is + // enabled out of the box), but stated explicitly on purpose: the numeric + // storage format is a correctness requirement, not a preference. Stores write + // epoch-millis for range queries and sort server-side on persisted timestamps, + // so silently inheriting a flipped library default would reorder query results + // rather than fail. This repo has already been bitten once by an implicit date + // format (commit dc117cddc). + mapper.configure(SerializationFeature.WRITE_DATES_AS_TIMESTAMPS, true); + return mapper; } } diff --git a/src/main/java/ai/labs/eddi/engine/internal/RestAgentAdministration.java b/src/main/java/ai/labs/eddi/engine/internal/RestAgentAdministration.java index 811aeac703..21024f23df 100644 --- a/src/main/java/ai/labs/eddi/engine/internal/RestAgentAdministration.java +++ b/src/main/java/ai/labs/eddi/engine/internal/RestAgentAdministration.java @@ -155,6 +155,29 @@ public Response deployAgent(final Deployment.Environment environment, final Stri * Fails open: a store outage must not block deployments. The denial is * logged and metered either way. *

+ * + *

+ * Known limit — bounded overshoot under concurrency. The count + * is read, checked, and only then acted on, with no lock spanning the read and + * the eventual write. Concurrent deploys that all observe {@code count == limit + * - 1} will all pass, so the cap can be exceeded by up to the number of + * simultaneous requests. This does not heal on its own: once over, the + * gate simply refuses further deploys until an undeploy brings the count back + * down. + *

+ * + *

+ * A per-tenant lock is deliberately not used, because it would only serialize + * within one JVM while the count spans the shared deployment store and every + * node's in-memory registry — giving the appearance of a hard guarantee in + * exactly the clustered deployments where it would not hold. A real guarantee + * needs a distributed lock or a storage-level constraint, and there is no + * single row to constrain since the count is derived from two sources. The + * overshoot is accepted instead: deploys are rare, human- or agent-initiated + * admin operations rather than a hot path, and the gate's purpose — stopping + * runaway growth such as an LLM creating sub-agents in a loop — survives a + * small transient overrun. + *

*/ private void enforceAgentQuota(Deployment.Environment environment, String agentId) { Set deployedAgentIds = new HashSet<>(); diff --git a/src/main/java/ai/labs/eddi/engine/internal/RestAgentEngine.java b/src/main/java/ai/labs/eddi/engine/internal/RestAgentEngine.java index 692b663222..3686790a70 100644 --- a/src/main/java/ai/labs/eddi/engine/internal/RestAgentEngine.java +++ b/src/main/java/ai/labs/eddi/engine/internal/RestAgentEngine.java @@ -243,7 +243,7 @@ public void onSkipped(SimpleConversationMemorySnapshot snapshot) { // branch it fell through to the generic handler below and the api-call // quota surfaced as a 500 instead of a 429. Body and headers mirror the // mapper so both quota denials look identical to clients. - LOGGER.warnf("Quota exceeded for conversation %s: %s", conversationId, e.getMessage()); + LOGGER.warnf("Quota exceeded for conversation %s: %s", sanitize(conversationId), e.getMessage()); response.resume(Response.status(TOO_MANY_REQUESTS) .entity(Map.of("error", "quota_exceeded", "message", e.getMessage())) .type(MediaType.APPLICATION_JSON).header("Retry-After", "60").build()); diff --git a/src/test/java/ai/labs/eddi/configs/admin/rest/RestOrphanAdminSafetyTest.java b/src/test/java/ai/labs/eddi/configs/admin/rest/RestOrphanAdminSafetyTest.java index 77b45ba85d..a92e12b104 100644 --- a/src/test/java/ai/labs/eddi/configs/admin/rest/RestOrphanAdminSafetyTest.java +++ b/src/test/java/ai/labs/eddi/configs/admin/rest/RestOrphanAdminSafetyTest.java @@ -122,6 +122,34 @@ void walksEveryPage() throws Exception { "the second page's descriptor must appear in the report"); } + @Test + @DisplayName("a type that never stops paging aborts the purge instead of truncating") + void ceilingAbortsPurgeRatherThanTruncating() throws Exception { + // Every page full, forever: the walk must hit MAX_PAGES and raise, not + // quietly return a partial set. A truncated scan of the REFERENCE side is + // what makes live resources look unreferenced, so it must never reach the + // delete loop. + AgentConfiguration readableAgent = new AgentConfiguration(); + readableAgent.setWorkflows(List.of()); + + when(documentDescriptorStore.readDescriptors(anyString(), anyString(), anyInt(), anyInt(), anyBoolean())) + .thenReturn(List.of()); + when(documentDescriptorStore.readDescriptors(eq("ai.labs.agent"), anyString(), anyInt(), anyInt(), anyBoolean())) + .thenReturn(fullPage("ai.labs.agent")); + // Every descriptor must read cleanly, so the ONLY thing that can mark the + // scan incomplete is the page ceiling. Without this the mock returns null, + // the traversal NPEs, and the test passes for the wrong reason even with + // the ceiling removed. + when(agentStore.read(anyString(), any())).thenReturn(readableAgent); + + WebApplicationException thrown = assertThrows(WebApplicationException.class, () -> restOrphanAdmin.purgeOrphans(false)); + + assertEquals(409, thrown.getResponse().getStatus()); + assertTrue(thrown.getResponse().getEntity().toString().contains("exceeded"), + "the refusal must name the page ceiling, got: " + thrown.getResponse().getEntity()); + verify(resourceClientLibrary, never()).deleteResource(any(), anyBoolean()); + } + @Test @DisplayName("stops on the first partial page") void stopsOnPartialPage() throws Exception { diff --git a/src/test/java/ai/labs/eddi/datastore/serialization/SerializationCustomizerInstantFormatTest.java b/src/test/java/ai/labs/eddi/datastore/serialization/SerializationCustomizerInstantFormatTest.java index 4305a0f722..0ff612027a 100644 --- a/src/test/java/ai/labs/eddi/datastore/serialization/SerializationCustomizerInstantFormatTest.java +++ b/src/test/java/ai/labs/eddi/datastore/serialization/SerializationCustomizerInstantFormatTest.java @@ -48,11 +48,14 @@ private static ObjectMapper restMapper() { return mapper; } - /** Exactly what PersistenceMapperProducer builds. */ + /** + * The REAL producer, not a reconstruction. Building a look-alike here once + * masked the thing under test: the fixture set WRITE_DATES_AS_TIMESTAMPS + * itself, so it would have passed even if the producer had left Instants as ISO + * strings. + */ private static ObjectMapper persistenceMapper() { - ObjectMapper mapper = SerializationCustomizer.configureObjectMapper(new ObjectMapper(), false); - mapper.configure(SerializationFeature.WRITE_DATES_AS_TIMESTAMPS, true); - return mapper; + return new PersistenceMapperProducer().persistenceMapper(); } @Nested From bada0d06cc7a865606ef2945f71e60727523e886 Mon Sep 17 00:00:00 2001 From: Gregor Jarisch Date: Wed, 22 Jul 2026 15:46:29 +0200 Subject: [PATCH 12/12] fix: address further PR review comments on test fixtures MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit UsageSnapshotSerializationTest.productionMapper() stopped mirroring the real REST mapper: it called SerializationCustomizer.configureObjectMapper directly, which since the persistence/REST mapper split builds the persistence recipe (no Instant override), not the REST/CDI one. The assertions still passed because costMonth is a YearMonth with @JsonFormat on the field directly, unaffected by the Instant configOverride — but the test's stated purpose (pinning the REST wire shape) was no longer true and it gave zero coverage of the actual production mapper. Same class of defect already fixed in SerializationCustomizerInstantFormatTest (a186dc903); the fixture now builds via new SerializationCustomizer(false).customize(mapper), matching how Quarkus actually constructs it. Inline FQN in DescriptorStoreTest: new java.util.ArrayList<>() where java.util.List was already imported. Added the top-level import; this file arrived via the origin/main merge, not authored on this branch, but the convention applies regardless of origin. --- docs/changelog.md | 9 +++++++++ .../ai/labs/eddi/datastore/DescriptorStoreTest.java | 3 ++- .../tenancy/UsageSnapshotSerializationTest.java | 11 ++++++++--- 3 files changed, 19 insertions(+), 4 deletions(-) diff --git a/docs/changelog.md b/docs/changelog.md index dadbd04f13..2cb2568ab4 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -3,6 +3,15 @@ > **Purpose:** Living document tracking all changes, decisions, and reasoning during implementation. Updated as work progresses for easy reference and review. +--- + +## 🔍 More PR review follow-ups (2026-07-22) + +**Repo:** EDDI (`fix/orphan-scan-and-quota-defects`) + +- **`UsageSnapshotSerializationTest.productionMapper()` stopped mirroring the real REST mapper.** It called `SerializationCustomizer.configureObjectMapper(...)` directly — which, since the persistence/REST mapper split earlier in this branch, builds the *persistence* recipe (no `Instant` override), not the REST/CDI one. The assertions still passed, because `costMonth` is a `YearMonth` with `@JsonFormat` on the field directly and is unaffected by the `Instant` `configOverride` — but the test's stated purpose ("pins the REST wire shape") was no longer true, and it provided zero coverage of the actual production mapper. Same class of defect fixed earlier in `SerializationCustomizerInstantFormatTest` (`a186dc903`): the fixture now builds the mapper via `new SerializationCustomizer(false).customize(mapper)`, matching how Quarkus actually constructs it. +- **Inline FQN in `DescriptorStoreTest`.** `new java.util.ArrayList<>()` where `java.util.List` was already imported. Added the top-level import; this file came in via the `origin/main` merge, not authored on this branch, but the convention applies regardless of origin. + --- ## 🔀 Merge `origin/main` into `fix/orphan-scan-and-quota-defects` — changelog conflict resolution (2026-07-21) diff --git a/src/test/java/ai/labs/eddi/datastore/DescriptorStoreTest.java b/src/test/java/ai/labs/eddi/datastore/DescriptorStoreTest.java index 3db2835109..5585ebcf7d 100644 --- a/src/test/java/ai/labs/eddi/datastore/DescriptorStoreTest.java +++ b/src/test/java/ai/labs/eddi/datastore/DescriptorStoreTest.java @@ -13,6 +13,7 @@ import org.mockito.ArgumentCaptor; import java.io.IOException; +import java.util.ArrayList; import java.util.List; import static org.junit.jupiter.api.Assertions.*; @@ -212,7 +213,7 @@ void skipIsIndexTimesEffectiveLimit() throws Exception { @DisplayName("results are not post-truncated to the default page size") void returnsMoreThanDefaultPageSize() throws Exception { int count = IDescriptorStore.DEFAULT_LIMIT + 5; - List ids = new java.util.ArrayList<>(); + List ids = new ArrayList<>(); for (int i = 0; i < count; i++) { IResourceStore.IResourceId id = mock(IResourceStore.IResourceId.class); when(id.getId()).thenReturn("res-" + i); diff --git a/src/test/java/ai/labs/eddi/engine/tenancy/UsageSnapshotSerializationTest.java b/src/test/java/ai/labs/eddi/engine/tenancy/UsageSnapshotSerializationTest.java index 3dc4c5abe6..95fa653b03 100644 --- a/src/test/java/ai/labs/eddi/engine/tenancy/UsageSnapshotSerializationTest.java +++ b/src/test/java/ai/labs/eddi/engine/tenancy/UsageSnapshotSerializationTest.java @@ -33,12 +33,17 @@ class UsageSnapshotSerializationTest { /** - * Mirrors the production mapper: the shared customizer plus the - * write-dates-as-timestamps flag Quarkus applies from application.properties. + * The real REST/CDI mapper — {@code customize()}, not the shared + * {@code configureObjectMapper} recipe. Since the persistence/REST mapper + * split, {@code configureObjectMapper} alone builds the persistence + * mapper (no Instant override); only {@code customize()} applies the REST-only + * formatting. Calling the static directly here would silently stop exercising + * the mapper this test claims to pin. */ private static ObjectMapper productionMapper() { - ObjectMapper mapper = SerializationCustomizer.configureObjectMapper(new ObjectMapper(), false); + ObjectMapper mapper = new ObjectMapper(); mapper.configure(SerializationFeature.WRITE_DATES_AS_TIMESTAMPS, true); + new SerializationCustomizer(false).customize(mapper); return mapper; }