diff --git a/docs/changelog.md b/docs/changelog.md index 8e589bd2ff..2cb2568ab4 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -3,6 +3,240 @@ > **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) + +**Repo:** EDDI (`fix/orphan-scan-and-quota-defects`) + +Brought the branch up to date with `origin/main`, which had picked up the `fix/descriptor-store-limit-semantics` PR (`limit=0` now means "unlimited" in `DescriptorStore.readDescriptors`, with a `MAX_RESULT_LIMIT` safety ceiling) via a background task spawned earlier in this effort. Both that PR and this branch touch `DescriptorStore.readDescriptors`, so `docs/changelog.md` conflicted at the top (both sides prepend); `DescriptorStore.java` and its test merged automatically without conflict โ€” this branch's `includeDeleted` inclusion-flag fix and main's `resolveDescriptorLimit`/ceiling-warning logic touch disjoint parts of the same method. Kept both changelog blocks whole, this branch's newest-first on top, main's independent entry below. + +## ๐Ÿ” 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) + +**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) + +**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) + +**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) + +**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) + +**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) + +**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) + +**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) + +**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. + --- ## ๐Ÿ› Fix: `readDescriptors(limit = 0)` silently returned only 20 descriptors (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/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..bae9549456 --- /dev/null +++ b/docs/superpowers/specs/2026-07-21-manager-coverage-backend-design.md @@ -0,0 +1,143 @@ +# 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. + +## 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. + +`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 + +**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`. +- 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. 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/configs/admin/rest/RestOrphanAdmin.java b/src/main/java/ai/labs/eddi/configs/admin/rest/RestOrphanAdmin.java index 04f7e721d0..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 @@ -19,6 +19,9 @@ 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.MediaType; +import jakarta.ws.rs.core.Response; import org.jboss.logging.Logger; import java.net.URI; @@ -59,6 +62,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 +93,25 @@ 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()); + // 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); int deletedCount = 0; for (OrphanInfo orphan : orphans) { @@ -98,12 +128,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) { @@ -119,6 +164,10 @@ private List findOrphans(boolean includeDeleted) { } } } 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()); } } @@ -128,11 +177,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 +206,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 +225,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 +279,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 #scanReferencedUris()}, 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/main/java/ai/labs/eddi/datastore/DescriptorStore.java b/src/main/java/ai/labs/eddi/datastore/DescriptorStore.java index 17cea22c32..05a459e4c3 100644 --- a/src/main/java/ai/labs/eddi/datastore/DescriptorStore.java +++ b/src/main/java/ai/labs/eddi/datastore/DescriptorStore.java @@ -47,7 +47,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/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..398c2582c9 --- /dev/null +++ b/src/main/java/ai/labs/eddi/datastore/serialization/PersistenceMapperProducer.java @@ -0,0 +1,43 @@ +/* + * 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 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() { + 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/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/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..21024f23df 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,91 @@ 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. + *

+ * + *

+ * 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<>(); + + 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/internal/RestAgentEngine.java b/src/main/java/ai/labs/eddi/engine/internal/RestAgentEngine.java index 2d71c65f4b..3686790a70 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", 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()); } catch (Exception e) { LOGGER.error(e.getLocalizedMessage(), e); throw new InternalServerErrorException("An internal error occurred"); 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/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/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/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/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(); 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 new file mode 100644 index 0000000000..a92e12b104 --- /dev/null +++ b/src/test/java/ai/labs/eddi/configs/admin/rest/RestOrphanAdminSafetyTest.java @@ -0,0 +1,296 @@ +/* + * 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.atLeastOnce; +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("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 { + 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()); + // 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()); + } + + @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("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 { + + @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()); + } + } +} diff --git a/src/test/java/ai/labs/eddi/datastore/DescriptorStoreTest.java b/src/test/java/ai/labs/eddi/datastore/DescriptorStoreTest.java index 26e424f457..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); @@ -256,6 +257,45 @@ void resolveLimitIsAlwaysUsable() { } } + @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 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..0ff612027a --- /dev/null +++ b/src/test/java/ai/labs/eddi/datastore/serialization/SerializationCustomizerInstantFormatTest.java @@ -0,0 +1,170 @@ +/* + * 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; + } + + /** + * 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() { + return new PersistenceMapperProducer().persistenceMapper(); + } + + @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; + } + } +} 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/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 { 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/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 { 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() { 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..95fa653b03 --- /dev/null +++ b/src/test/java/ai/labs/eddi/engine/tenancy/UsageSnapshotSerializationTest.java @@ -0,0 +1,72 @@ +/* + * 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 { + + /** + * 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 = new ObjectMapper(); + mapper.configure(SerializationFeature.WRITE_DATES_AS_TIMESTAMPS, true); + new SerializationCustomizer(false).customize(mapper); + 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()); + } +}