Fix/orphan scan and quota defects - #603
Merged
Merged
Conversation
…plete scan readAllDescriptors advanced its cursor with `index += batch.size()`, but DescriptorStore.readDescriptors treats that argument as a PAGE index (skip = index * limit). The second iteration asked for skip=40000, always came back empty, and every store type was silently truncated at 200 rows. The dangerous half was buildReferencedUrisSet: the *referenced* set was truncated the same way, so on a deployment with >200 agents or >200 workflows, live in-use resources were classified as orphans — and purgeOrphans deletes with permanent=true (deleteAllPermanently, which removes the current document and all history). Separately, the reference scan failed open: read errors were swallowed (two at debug level) and the partial set returned as complete. Since that set is what protects a resource, each swallowed error made MORE things look orphaned. - readAllDescriptors: advance by page, bounded by MAX_PAGES; raise rather than truncate silently when the ceiling is hit - scanReferencedUris returns a ReferenceScan carrying a `complete` flag - purgeOrphans refuses with 409 when the scan is incomplete, naming the cause; scanOrphans still returns its best-effort read-only report - ResourceNotFoundException is not incompleteness — a descriptor whose resource is gone is a genuine orphan includeDeleted semantics are deliberately left alone here: with the page walk now complete, redefining that flag without also flipping its `true` default would turn the default DELETE from "purge <=200 already-soft- deleted rows" into "permanently wipe every unreferenced resource".
…hape Two independent defects on the tenant cost-budget surface. 1. TenantQuotaService.checkCostBudget denies on `currentCost >= limit` and InMemoryTenantQuotaStore.tryAddCost matches it, but MongoTenantQuotaStore and PostgresTenantQuotaStore used `totalCost > limit`. At exactly the budget the pre-call gate denied while post-call accounting allowed. The in-memory store was deliberately moved to >= for this reason; the two production stores were never updated. 2. UsageSnapshot.costMonth serialized as the array [2026,7] instead of "2026-07": under quarkus.jackson.write-dates-as-timestamps=true Jackson's YearMonthSerializer takes its useTimestamp branch. Both stores already persist the value as an ISO string via YearMonth.toString()/parse and never route it through Jackson, so only the REST shape disagreed. Deliberately NOT done here: a mapper-wide configOverride(Instant.class). SerializationCustomizer's mapper is also the persistence mapper — JsonSerialization injects the same CDI ObjectMapper, which backs DocumentBuilder for every Mongo write and Postgres JSONB column. It would change on-disk formats; GroupConversation.lastModified is an Instant that GroupConversationStore sorts server-side on, so mixed numeric/string rows would sort wrongly and silently. Commit dc117cd already reverted a broader version of that change for breaking findDueSchedules. The new exactlyAtLimit test was mutation-checked: reverting the Mongo comparison to `>` makes it fail, so it is not vacuous.
… a turn
ConversationService.say/sayStreaming throw QuotaExceededException when
acquireApiCallSlot() denies, and QuotaExceededExceptionMapper maps that to
429 with {"error":"quota_exceeded"} and Retry-After: 60. But say() is
resumed through a JAX-RS AsyncResponse, so the exception is caught inside
RestAgentEngine.sayInternal and never reaches the @Provider mapper. Its
catch chain did not list QuotaExceededException, so the denial fell into
`catch (Exception e)` and surfaced as 500 "An internal error occurred".
Only the conversation-start quota ever produced a real 429, because
startConversationWithContext is synchronous and its narrower catch block
lets the exception escape to the mapper. The per-minute API rate limit —
the quota an operator is most likely to hit — was indistinguishable from
a server fault, so clients could not back off correctly.
Added an explicit catch that resumes with the same status, body and
Retry-After header as the mapper.
sayStreaming is deliberately not covered: by the time it throws, the SSE
response has already committed HTTP 200, so no status can be sent. Giving
its generic error event a distinguishable quota type is a separate,
client-visible change.
The new test was mutation-checked: replacing the catch with an unrelated
exception type makes it fail with InternalServerError, reproducing the
original bug.
… deferrals Captures what was verified against the code, what shipped, and — more importantly — what deliberately did not and why, so the deferred items are not re-attempted from the original (stale) spec. Notably records that the repo-wide Instant->ISO change was implemented and then backed out: SerializationCustomizer's mapper is also the persistence mapper (JsonSerialization injects the same CDI ObjectMapper), so it would have changed on-disk formats and silently broken the server-side sort on GroupConversation.lastModified. Commit dc117cd had already reverted a broader version of that change once.
Self-review of the three preceding commits found three defects.
purgeOrphans threw `new WebApplicationException(String, Response.Status)`,
whose response carries NO entity — so an operator got a bare 409 and the
reason existed only in the server log. Build the Response explicitly with
{"error":"incomplete_scan","message":...} instead. Confirmed by mutation:
reverting to that constructor makes the new hasEntity() assertion fail,
so the fix is real rather than cosmetic.
The page-walk javadoc still linked buildReferencedUrisSet(), which was
renamed to scanReferencedUris() in the same commit — a broken @link. Also
updated three now-inaccurate @displaynames in RestOrphanAdminBranchTest.
The > to >= change was mutation-verified on Mongo but Postgres had no
at-limit test at all. Added PostgresTenantQuotaStoreTest.exactlyAtLimit
and mutation-checked it.
Documented why a MAX_PAGES trip during orphan collection is safely
swallowed while the same failure in the reference scan blocks the purge:
an unenumerable type yields fewer delete candidates (under-delete),
whereas a missing reference promotes a live resource to "orphan"
(over-delete).
…fault
BREAKING (REST): DELETE /administration/orphans now defaults to
includeDeleted=false and purges live-but-unreferenced resources only.
DescriptorStore.readDescriptors treated includeDeleted as an EQUALITY
filter — eq("deleted", includeDeleted) — so includeDeleted=true matched
ONLY soft-deleted descriptors instead of adding them to the live ones.
The parameter did not mean what its name, its @parameter text, or
docs/deployment-management-of-agents.md said it meant. Worse, the shipped
Manager scans with false and purges with true, so the set shown to the
user and the set deleted were disjoint: the UI listed live orphans and
then purged soft-deleted ones.
true now drops the `deleted` constraint entirely (live AND soft-deleted);
false constrains to live only. Every other caller in src/main passes a
literal false — all ~35 call sites enumerated — so their behaviour is
unchanged.
purgeOrphans's @DefaultValue flips from true to false. Left at true, the
semantics fix would have made the parameterless DELETE dramatically more
destructive: combined with the page-walk fix, from "purge <=200 already-
soft-deleted rows" to "permanently wipe every unreferenced resource,
unbounded". Flipping it makes the bare call the conservative one and
makes scan and purge describe the same set by default.
Clients relying on the old default now purge LESS; pass
includeDeleted=true to also purge soft-deleted resources.
Mutation-checked: restoring the equality filter fails the new
readDescriptorsIncludesDeleted assertion.
TenantQuota.maxAgentsPerTenant was persisted by all three stores and round-tripped through REST, but nothing ever read it — TenantQuotaService had no agent method at all. Operators setting the limit got silent no-enforcement. Adds TenantQuotaService.checkAgentQuota(tenantId, currentDistinctAgents), gating RestAgentAdministration.deployAgent and denying with QuotaExceededException -> 429 via the existing mapper. A read-only gate, not an atomic counter: the deployed-agent count is a stock derived by counting deployments, not a per-window flow. A stored counter would drift, because the 10s re-deploy sweep, the 24h old-version undeploy, TeardownAgentTool, GroupConversationService and the lazy re-deploy on first use all mutate deployments without passing any acquire/release point. No ITenantQuotaStore method added, so the three store impls are untouched. Placement is forced: the gate sits between the null-checks and the try. Inside the try, catch(Exception) -> InternalServerErrorException would turn the 429 into a 500; inside the submitted Callable it runs off the request thread and could never produce a status code. The count unions persisted `deployed` rows with live READY agents, and needs both. autoDeploy=false never writes a row but the in-memory deploy is unconditional, so a rows-only count let a caller deploy unlimited agents with one query param — and those agents are genuinely live and durable: getLatestReadyAgent serves them without consulting the store, and ConversationService.getAgent lazily re-deploys them after a restart. A live-agents-only count would be per-JVM and miss other cluster nodes. Counts distinct agent ids, so redeploys and version bumps are free — required because the old-version undeploy sweep legitimately keeps two versions deployed while the previous drains. Fails open on store error. AgentSetupService.deployAndWait and McpAdminTools.deployAgent call the bean directly, so the mapper never runs there; both now return the quota reason instead of "check server logs", which a model driving create_sub_agent cannot act on and would retry in a loop. Mutation-checked: reverting to a rows-only count fails exactly the loophole test.
…umeric
BREAKING (REST): every java.time.Instant on every endpoint changes from a
fractional-epoch-seconds number to an ISO-8601 string.
Every Instant rendered as a 1970 date in the Manager. With
quarkus.jackson.write-dates-as-timestamps=true plus JavaTimeModule an
Instant serializes as fractional epoch SECONDS (1719964800.123), while
clients call new Date(value), which expects MILLIS. This hit nextFire,
lastFired, pausedAt, createdAt, updatedAt, transcript timestamps —
essentially every timestamp in the UI.
The obvious fix, a configOverride in the shared configureObjectMapper, is
a trap: JsonSerialization injects the same CDI ObjectMapper, and it backs
DocumentBuilder for every Mongo write, every Postgres JSONB column, the
backup writer and the {json:serialize} Qute extension. That would change
on-disk formats. GroupConversation.lastModified is a persisted Instant
that GroupConversationStore sorts SERVER-SIDE on: Mongo ranks all Doubles
before all Strings, Postgres orders data->>'lastModified' lexicographically,
so old and new rows would interleave wrongly and silently. Commit
dc117cd already reverted a broader version of this for breaking
findDueSchedules.
So the mappers are split:
- new @PersistenceMapper qualifier + PersistenceMapperProducer, built from
the same configureObjectMapper recipe WITHOUT the date override;
JsonSerialization injects that
- the Instant override moves into SerializationCustomizer.customize(),
the REST/CDI path only, never the shared static
java.util.Date is deliberately untouched — it already emits epoch millis
and its consumers are correct today. application.properties:174 must
stay: Quarkus defaults write-dates-as-timestamps to false, so that line is
what prevents Quarkus disabling the feature globally.
Deserialization is unaffected either way — InstantDeserializer dispatches
on the JSON token type, not the shape hint — so numeric rows still parse.
REQUIRES a companion EDDI-Manager change: the Schedules dashboard sorts
`(a.nextFire ?? 0) - (b.nextFire ?? 0)`, which is NaN on ISO strings. Use
`new Date(a.nextFire) - new Date(b.nextFire)`, which works with both
encodings and can land independently. The vendored bundle under
META-INF/resources/assets/ is a build artifact and was not hand-edited.
NOTE: CDI wiring for the qualified producer could not be validated
locally — quarkus:build augmentation needs a loopback socket this
environment refuses, and the repo has one non-IT @QuarkusTest. The format
behaviour itself is fully unit-tested; CI is the gate for the wiring.
AgentOrchestrator sums TokenUsage across every model call in the tool loop
and returns it on ExecutionResult.responseMetadata(), on both the live
path (AgentOrchestrator.java:826) and the resume path (:470). LlmTask read
.response() and .trace() from that result and never .responseMetadata(),
so agent-mode token accounting was computed and dropped on the floor —
only the legacy-chat and cascade branches surfaced theirs.
Any agent with tools enabled therefore reported {} for
responseMetadataObjectName, and no per-turn token figure existed for the
paths that dominate real usage. This is the prerequisite for monthly cost
metering: there was nothing to meter.
Both agent branches now read the metadata, and executeResume surfaces it
the way executeTask does (it previously built no metadata map at all).
Known gap: a turn that pauses for tool approval loses its pre-pause usage.
ToolApprovalRequiredException escapes before the metadata is assembled and
carries no usage, and resumeToolLoop starts a fresh accumulator, so a
paused turn under-reports by its pre-pause segment. Closing that needs the
step threaded into runToolCallLoop so usage is written incrementally.
Safety: responseMetadata also feeds applyResponseValidation, which
branches on `warning` and `streamingTimeout`. AgentOrchestrator puts only
`tokenUsage` into the map, so both keys stay absent exactly as with the
previously-empty map — validation behaviour is unchanged.
NOT unit-covered: LlmTask constructs its own AgentOrchestrator rather than
receiving it injected, so executeIfToolsEnabled cannot be stubbed and every
existing LlmTask test exercises only the legacy branch. Verified by reading
both sides of the contract plus the validation check above; all 184 LlmTask
tests stay green. Making this testable means injecting AgentOrchestrator —
a separate refactor of an already 29-argument constructor.
Marks includeDeleted, maxAgentsPerTenant, the mapper split and the agent-mode token-usage fix as shipped, and rewrites the cost-metering section: ObservableChatModel was refuted as the interception point (it is optional, never wraps streaming, is shared across conversations with no attribution context, and never reads TokenUsage). Records the verified six-site seam and the three questions that must be settled before wiring recordCost — price-field precedence, the dormant Mongo E11000 that goes live on first call, and the LLM calls that are structurally uncounted.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe PR hardens orphan scanning and purging, separates REST and persistence JSON formats, enforces agent quotas, aligns cost-budget boundaries, preserves LLM response metadata, and adds tests, API documentation, changelog entries, and design notes. ChangesBackend behavior and safety fixes
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant RestAgentAdministration
participant TenantQuotaService
participant Runtime
Client->>RestAgentAdministration: deployAgent()
RestAgentAdministration->>TenantQuotaService: checkAgentQuota()
TenantQuotaService-->>RestAgentAdministration: quota result
RestAgentAdministration->>Runtime: submit deployment
Runtime-->>Client: deployment response or quota error
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Fixes several correctness and safety issues across orphan-resource administration, tenant quota enforcement, and JSON serialization contracts, aligning REST behavior with storage semantics and preventing destructive operations under incomplete scans.
Changes:
- Hardened orphan scan/purge semantics: fixed descriptor paging, made
includeDeleteda true inclusion flag, and blocked purge with HTTP 409 when reference scans are incomplete. - Improved tenancy quota correctness: added agent-capacity quota gate on deploy, aligned “at-limit” cost budget comparisons across stores, and ensured quota denials surface as HTTP 429 in async REST paths.
- Split REST vs persistence JSON concerns: REST
Instantis ISO-8601 while persistence remains numeric; pinned wire shapes with focused serialization tests; fixed agent-mode token usage metadata propagation.
Reviewed changes
Copilot reviewed 32 out of 32 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/test/java/ai/labs/eddi/engine/tenancy/UsageSnapshotSerializationTest.java | Pins UsageSnapshot.costMonth REST JSON shape and round-trip behavior. |
| src/test/java/ai/labs/eddi/engine/tenancy/TenantQuotaServiceTest.java | Adds tests for new agent-capacity quota checks and metric behavior parity. |
| src/test/java/ai/labs/eddi/engine/tenancy/PostgresTenantQuotaStoreTest.java | Tests “exactly at limit” denial behavior for Postgres cost accounting. |
| src/test/java/ai/labs/eddi/engine/tenancy/MongoTenantQuotaStoreTest.java | Tests “exactly at limit” denial behavior for Mongo cost accounting. |
| src/test/java/ai/labs/eddi/engine/internal/RestAgentEngineTest.java | Verifies async say() resumes with 429 on quota denials (not 500). |
| src/test/java/ai/labs/eddi/engine/internal/RestAgentAdministrationTest.java | Updates wiring for new TenantQuotaService dependency in admin resource. |
| src/test/java/ai/labs/eddi/engine/internal/RestAgentAdministrationQuotaTest.java | New focused tests for max-agents-per-tenant deploy gate behavior. |
| src/test/java/ai/labs/eddi/engine/internal/RestAgentAdministrationExtendedTest.java | Updates wiring for new TenantQuotaService dependency. |
| src/test/java/ai/labs/eddi/datastore/serialization/SerializationCustomizerInstantFormatTest.java | Pins REST vs persistence Instant serialization/deserialization behavior. |
| src/test/java/ai/labs/eddi/datastore/DescriptorStoreTest.java | Tests corrected includeDeleted filtering semantics for descriptors. |
| src/test/java/ai/labs/eddi/configs/admin/rest/RestOrphanAdminSafetyTest.java | New safety tests for scan completeness and purge refusal behavior. |
| src/test/java/ai/labs/eddi/configs/admin/rest/RestOrphanAdminBranchTest.java | Renames display text to match refactored referenced-scan method naming. |
| src/main/java/ai/labs/eddi/modules/llm/impl/LlmTask.java | Ensures agent-mode response metadata (token usage) isn’t dropped; surfaces metadata on resume. |
| src/main/java/ai/labs/eddi/engine/tenancy/TenantQuotaService.java | Adds read-only agent-capacity quota gate (checkAgentQuota). |
| src/main/java/ai/labs/eddi/engine/tenancy/PostgresTenantQuotaStore.java | Aligns “over budget” comparison to >= for consistency at the boundary. |
| src/main/java/ai/labs/eddi/engine/tenancy/MongoTenantQuotaStore.java | Aligns “over budget” comparison to >= for consistency at the boundary. |
| src/main/java/ai/labs/eddi/engine/tenancy/model/UsageSnapshot.java | Forces YearMonth REST serialization to ISO string via @JsonFormat. |
| src/main/java/ai/labs/eddi/engine/setup/AgentSetupService.java | Surfaces quota-denial reasons when deploying via CDI bean (no mapper involved). |
| src/main/java/ai/labs/eddi/engine/mcp/McpAdminTools.java | Surfaces quota-denial reasons to MCP clients to avoid retry loops. |
| src/main/java/ai/labs/eddi/engine/internal/RestAgentEngine.java | Catches QuotaExceededException in async say() path and mirrors mapper response (429 + Retry-After). |
| src/main/java/ai/labs/eddi/engine/internal/RestAgentAdministration.java | Enforces max-agents-per-tenant quota prior to async deploy submission; counts distinct deployed agents. |
| src/main/java/ai/labs/eddi/engine/api/IRestAgentAdministration.java | Documents deploy endpoint’s quota behavior and adds 429 response documentation. |
| src/main/java/ai/labs/eddi/datastore/serialization/SerializationCustomizer.java | Applies REST-only Instant ISO formatting override while keeping shared persistence recipe unchanged. |
| src/main/java/ai/labs/eddi/datastore/serialization/PersistenceMapperProducer.java | Produces qualified persistence ObjectMapper distinct from REST mapper. |
| src/main/java/ai/labs/eddi/datastore/serialization/PersistenceMapper.java | Adds CDI qualifier to distinguish persistence mapper from REST mapper. |
| src/main/java/ai/labs/eddi/datastore/serialization/JsonSerialization.java | Switches persistence serialization to use the qualified persistence mapper. |
| src/main/java/ai/labs/eddi/datastore/DescriptorStore.java | Fixes includeDeleted semantics to be an inclusion flag (no deleted constraint when true). |
| src/main/java/ai/labs/eddi/configs/admin/rest/RestOrphanAdmin.java | Fixes descriptor pagination, adds scan ceiling, returns completeness status, and blocks purge on incomplete scans. |
| src/main/java/ai/labs/eddi/configs/admin/IRestOrphanAdmin.java | Updates orphan scan/purge endpoint docs: includeDeleted semantics, defaults, and 409 behavior. |
| docs/superpowers/specs/2026-07-21-manager-coverage-backend-design.md | Adds design/spec verification notes and rationale for shipped vs deferred items. |
| docs/deployment-management-of-agents.md | Updates public docs to match new orphan scan/purge semantics and safety behavior. |
| docs/changelog.md | Records behavior changes, rationale, and client-impact notes for the shipped fixes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/main/java/ai/labs/eddi/configs/admin/rest/RestOrphanAdmin.java (2)
293-313: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNo test covers the new
MAX_PAGESceiling-exceeded path.
RestOrphanAdminSafetyTestcovers normal paging (walksEveryPage,stopsOnPartialPage) but nothing exercises the new safety net itself — i.e., a store returningMAX_PAGESconsecutive full batches soreadAllDescriptorsthrowsResourceStoreExceptioninstead of truncating. This is the exact mechanism the PR introduces to prevent silent truncation, so it's worth pinning with a test (e.g., stubreadDescriptorsto always returnfullPage(...)and assert the exception/message).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/ai/labs/eddi/configs/admin/rest/RestOrphanAdmin.java` around lines 293 - 313, Add a RestOrphanAdminSafetyTest covering readAllDescriptors when the store returns a full batch for every request through MAX_PAGES. Stub readDescriptors to return fullPage(...) consistently, invoke the relevant descriptor scan, and assert that ResourceStoreException is thrown with the expected ceiling/exceeded message rather than returning truncated results.
65-73: 🩺 Stability & Availability | 🔵 TrivialSynchronous, sequential descriptor paging still blocks the JAX-RS request thread.
readAllDescriptorsnow walks up toMAX_PAGES(100) sequential pages per store type, andscanReferencedUris/collectOrphanscall it across 8 store types on every scan/purge. TheMAX_PAGESceiling bounds the worst case but doesn't remove the blocking nature of this endpoint — it holds a request thread for the entire synchronous traversal, which the guideline calls out directly: backend REST endpoints should be non-blocking and useAsyncResponserather than block for extended periods. This is an existing (and here explicitly documented) design tradeoff rather than a regression introduced by this diff, so treating it as advice rather than a blocker for this PR.Consider moving the collection/purge work off the request thread (e.g.,
@Asynchronous/CompletableFuture +AsyncResponse, or a background worker with a polling status endpoint) in a follow-up.As per coding guidelines: "Backend code must be thread-safe and non-blocking; REST endpoints should use JAX-RS
AsyncResponse, and tasks must not block for extended periods."Also applies to: 282-313
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/ai/labs/eddi/configs/admin/rest/RestOrphanAdmin.java` around lines 65 - 73, The synchronous descriptor collection and purge flow blocks the JAX-RS request thread. Update the REST endpoints and the `scanReferencedUris`/`collectOrphans` workflow to execute traversal and purge work asynchronously, using `AsyncResponse` with a suitable async executor or background task mechanism. Preserve existing result and error responses while returning control to the request thread immediately.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/java/ai/labs/eddi/engine/internal/RestAgentAdministration.java`:
- Around line 131-191: The quota check in enforceAgentQuota is vulnerable to
concurrent deployments passing the count check before either deployment becomes
visible. Serialize the complete quota-check and deploy mutation sequence for the
tenant, using the existing deployment flow around enforceAgentQuota and deploy,
so concurrent callers cannot both admit new agent IDs past maxAgentsPerTenant;
preserve redeploy/version-bump behavior and fail-open handling for store-read
failures.
In `@src/main/java/ai/labs/eddi/engine/internal/RestAgentEngine.java`:
- Around line 240-249: Update the QuotaExceededException catch block in
RestAgentEngine to pass sanitize(conversationId) to LOGGER.warnf instead of
logging the raw REST path parameter; leave the response payload and other
behavior unchanged.
---
Nitpick comments:
In `@src/main/java/ai/labs/eddi/configs/admin/rest/RestOrphanAdmin.java`:
- Around line 293-313: Add a RestOrphanAdminSafetyTest covering
readAllDescriptors when the store returns a full batch for every request through
MAX_PAGES. Stub readDescriptors to return fullPage(...) consistently, invoke the
relevant descriptor scan, and assert that ResourceStoreException is thrown with
the expected ceiling/exceeded message rather than returning truncated results.
- Around line 65-73: The synchronous descriptor collection and purge flow blocks
the JAX-RS request thread. Update the REST endpoints and the
`scanReferencedUris`/`collectOrphans` workflow to execute traversal and purge
work asynchronously, using `AsyncResponse` with a suitable async executor or
background task mechanism. Preserve existing result and error responses while
returning control to the request thread immediately.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0656834e-b2eb-4a09-acf3-25f3ff2d56b1
📒 Files selected for processing (32)
docs/changelog.mddocs/deployment-management-of-agents.mddocs/superpowers/specs/2026-07-21-manager-coverage-backend-design.mdsrc/main/java/ai/labs/eddi/configs/admin/IRestOrphanAdmin.javasrc/main/java/ai/labs/eddi/configs/admin/rest/RestOrphanAdmin.javasrc/main/java/ai/labs/eddi/datastore/DescriptorStore.javasrc/main/java/ai/labs/eddi/datastore/serialization/JsonSerialization.javasrc/main/java/ai/labs/eddi/datastore/serialization/PersistenceMapper.javasrc/main/java/ai/labs/eddi/datastore/serialization/PersistenceMapperProducer.javasrc/main/java/ai/labs/eddi/datastore/serialization/SerializationCustomizer.javasrc/main/java/ai/labs/eddi/engine/api/IRestAgentAdministration.javasrc/main/java/ai/labs/eddi/engine/internal/RestAgentAdministration.javasrc/main/java/ai/labs/eddi/engine/internal/RestAgentEngine.javasrc/main/java/ai/labs/eddi/engine/mcp/McpAdminTools.javasrc/main/java/ai/labs/eddi/engine/setup/AgentSetupService.javasrc/main/java/ai/labs/eddi/engine/tenancy/MongoTenantQuotaStore.javasrc/main/java/ai/labs/eddi/engine/tenancy/PostgresTenantQuotaStore.javasrc/main/java/ai/labs/eddi/engine/tenancy/TenantQuotaService.javasrc/main/java/ai/labs/eddi/engine/tenancy/model/UsageSnapshot.javasrc/main/java/ai/labs/eddi/modules/llm/impl/LlmTask.javasrc/test/java/ai/labs/eddi/configs/admin/rest/RestOrphanAdminBranchTest.javasrc/test/java/ai/labs/eddi/configs/admin/rest/RestOrphanAdminSafetyTest.javasrc/test/java/ai/labs/eddi/datastore/DescriptorStoreTest.javasrc/test/java/ai/labs/eddi/datastore/serialization/SerializationCustomizerInstantFormatTest.javasrc/test/java/ai/labs/eddi/engine/internal/RestAgentAdministrationExtendedTest.javasrc/test/java/ai/labs/eddi/engine/internal/RestAgentAdministrationQuotaTest.javasrc/test/java/ai/labs/eddi/engine/internal/RestAgentAdministrationTest.javasrc/test/java/ai/labs/eddi/engine/internal/RestAgentEngineTest.javasrc/test/java/ai/labs/eddi/engine/tenancy/MongoTenantQuotaStoreTest.javasrc/test/java/ai/labs/eddi/engine/tenancy/PostgresTenantQuotaStoreTest.javasrc/test/java/ai/labs/eddi/engine/tenancy/TenantQuotaServiceTest.javasrc/test/java/ai/labs/eddi/engine/tenancy/UsageSnapshotSerializationTest.java
CodeRabbit noted the new page ceiling had no coverage. Adding a test made it pass immediately — but mutation-checking it (disabling the ceiling so the walk truncates silently) showed it STILL passed: the fixture left agentStore.read unstubbed, so the traversal NPE'd on a null config, marked the scan incomplete, and produced the expected 409 for the wrong reason. Stubbing the agent read so the ceiling is the only possible failure source, and asserting the refusal names it, makes the test load-bearing. The mutant now dies with "Expected WebApplicationException to be thrown, but nothing was thrown." Copilot flagged that the produced @PersistenceMapper does not set WRITE_DATES_AS_TIMESTAMPS. The stated premise is wrong — Jackson enables that feature by default, so the producer already emitted numeric — but the real defect was next door: SerializationCustomizerInstantFormatTest RECONSTRUCTED the producer instead of calling it, and set the flag itself, so it would have passed even if the producer were broken. The test now builds the mapper via new PersistenceMapperProducer().persistenceMapper(), and the producer states the flag explicitly. Relying on a library default for a persistence-format guarantee is what dc117cd was reverted for. Mutation-checked: flipping the producer to ISO fails two tests. CodeRabbit correctly flagged the TOCTOU on the agent quota: concurrent deploys observing count == limit - 1 all pass. An earlier internal note called this self-correcting, which was wrong — once over, the gate merely refuses further deploys until an undeploy brings the count down. The javadoc now states the bound honestly and explains why a per-tenant lock is NOT used: it would serialize within one JVM while the count spans the shared deployment store and every node's in-memory registry, giving the appearance of a hard guarantee exactly where it would not hold. Accepted because deploys are rare admin operations and the gate's purpose — stopping runaway growth such as an LLM creating sub-agents in a loop — survives a small transient overrun. Also sanitize(conversationId) on the new quota-denial log line, matching the rest of RestAgentEngine. Not actioned: the advisory that the orphan endpoint blocks a request thread. Pre-existing, raised as advice rather than a blocker, and moving it to AsyncResponse is a separate change.
…uota-defects # Conflicts: # docs/changelog.md
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| } | ||
| } | ||
| } catch (Exception e) { | ||
| log.warnf("Agent quota check: could not determine the deployed-agent count, allowing deploy of %s: %s", agentId, e.getMessage()); |
|
|
||
| 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()); |
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/test/java/ai/labs/eddi/datastore/DescriptorStoreTest.java (1)
211-233: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winInline FQN instead of top-level import for
ArrayList.
new java.util.ArrayList<>()at line 215 uses an inline fully-qualified name wherejava.util.Listis already imported. Add a top-levelimport java.util.ArrayList;and use the simple name instead.As per coding guidelines, "Reference types and annotations through top-level imports; do not use inline fully qualified names except to disambiguate same-named types, and remove unused imports."
♻️ Proposed fix
- List<IResourceStore.IResourceId> ids = new java.util.ArrayList<>(); + List<IResourceStore.IResourceId> ids = new ArrayList<>();(with
import java.util.ArrayList;added to the import block)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/ai/labs/eddi/datastore/DescriptorStoreTest.java` around lines 211 - 233, Replace the inline java.util.ArrayList reference in returnsMoreThanDefaultPageSize with the simple ArrayList name, and add the corresponding top-level import to the test’s import block. Keep the existing list construction and test behavior unchanged.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/test/java/ai/labs/eddi/datastore/DescriptorStoreTest.java`:
- Around line 211-233: Replace the inline java.util.ArrayList reference in
returnsMoreThanDefaultPageSize with the simple ArrayList name, and add the
corresponding top-level import to the test’s import block. Keep the existing
list construction and test behavior unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 928f9d90-7013-4ba8-b4f2-d93c304ef9a2
📒 Files selected for processing (8)
docs/changelog.mdsrc/main/java/ai/labs/eddi/datastore/DescriptorStore.javasrc/main/java/ai/labs/eddi/datastore/serialization/PersistenceMapperProducer.javasrc/main/java/ai/labs/eddi/engine/internal/RestAgentAdministration.javasrc/main/java/ai/labs/eddi/engine/internal/RestAgentEngine.javasrc/test/java/ai/labs/eddi/configs/admin/rest/RestOrphanAdminSafetyTest.javasrc/test/java/ai/labs/eddi/datastore/DescriptorStoreTest.javasrc/test/java/ai/labs/eddi/datastore/serialization/SerializationCustomizerInstantFormatTest.java
🚧 Files skipped from review as they are similar to previous changes (6)
- src/main/java/ai/labs/eddi/datastore/serialization/PersistenceMapperProducer.java
- src/main/java/ai/labs/eddi/engine/internal/RestAgentEngine.java
- src/main/java/ai/labs/eddi/datastore/DescriptorStore.java
- src/main/java/ai/labs/eddi/engine/internal/RestAgentAdministration.java
- src/test/java/ai/labs/eddi/configs/admin/rest/RestOrphanAdminSafetyTest.java
- src/test/java/ai/labs/eddi/datastore/serialization/SerializationCustomizerInstantFormatTest.java
UsageSnapshotSerializationTest.productionMapper() stopped mirroring the real REST mapper: it called SerializationCustomizer.configureObjectMapper directly, which since the persistence/REST mapper split builds the persistence recipe (no Instant override), not the REST/CDI one. The assertions still passed because costMonth is a YearMonth with @jsonformat on the field directly, unaffected by the Instant configOverride — but the test's stated purpose (pinning the REST wire shape) was no longer true and it gave zero coverage of the actual production mapper. Same class of defect already fixed in SerializationCustomizerInstantFormatTest (a186dc9); the fixture now builds via new SerializationCustomizer(false).customize(mapper), matching how Quarkus actually constructs it. Inline FQN in DescriptorStoreTest: new java.util.ArrayList<>() where java.util.List was already imported. Added the top-level import; this file arrived via the origin/main merge, not authored on this branch, but the convention applies regardless of origin.
aisabella-ai
approved these changes
Jul 22, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This pull request contains several important fixes and behavior changes to the orphan resource administration endpoints and their documentation. The most significant changes are to the semantics of the
includeDeletedparameter, the default behavior of the purge endpoint, and improved safety and clarity when deleting orphaned resources. These changes ensure that purges are safer, more predictable, and that the API documentation accurately reflects the true behavior.Orphan admin API changes and documentation updates:
includeDeletedis now a true inclusion flag, not an equality filter. SettingincludeDeleted=trueincludes both live and soft-deleted resources, whilefalseincludes only live ones. Previously,truematched only soft-deleted resources, leading to confusion and mismatched scan/purge sets.DELETE /administration/orphansis nowincludeDeleted=false. This means a parameterless purge is conservative and matches the scan endpoint, preventing accidental deletion of all unreferenced resources. [1] [2]includeDeleted, and the list of resource types is corrected (e.g., "workflows" and "LLMs" instead of "packages" and "langchains"). [1] [2]includeDeleted=trueto restore the previous (wider) behavior. [1] [2]Type of Change
Checklist
./mvnw clean verify -DskipITs)Summary by CodeRabbit
Retry-Afterand structured error details.Instantserialization is now consistently ISO-8601 strings while stored data format is preserved;costMonthis emitted as a JSON string.includeDeletedalignment and purge fails with 409 (no deletions) when the reference scan is incomplete; paging completeness is enforced.