fix(runtime): agent deploy/undeploy lifecycle — no-op teardown, dead IN_PROGRESS handshake, idle-sweep age signal - #654
fix(runtime): agent deploy/undeploy lifecycle — no-op teardown, dead IN_PROGRESS handshake, idle-sweep age signal#654ginccc wants to merge 5 commits into
Conversation
…IN_PROGRESS handshake, idle-sweep age signal - undeployAgent(env, id, null) matched nothing because AgentId equality compares the version. Both dynamic-agent teardown callers pass null, so a torn-down (and with delete=true, config-deleted) agent stayed servable via getLatestReadyAgent and the deployed-agents gauge leaked. null now means every version, and the method returns how many were removed so callers cannot claim a teardown that did not happen. TeardownAgentTool retires deployment records unconditionally. - The IN_PROGRESS marker was never published, making getAgent's IN_PROGRESS branch and waitForDeploymentCompletion unreachable; the store load also ran inside ConcurrentHashMap.compute. The key is now claimed atomically, loaded outside the mapping function, and published with a conditional replace so a racing undeploy is not resurrected. - deployedAgents/deploymentInfos were LinkedLists with partial synchronization and concurrent writers; both are concurrent sets now, and deploymentInfos is rebuilt per poll so an undeployed agent can be redeployed. - The idle sweep aged conversations by the AGENT document's lastModifiedOn, and isOlderThanDays ignored Period's months component. Fixing only the arithmetic would have started ending live conversations, so both are fixed together: age now comes from the conversation's own newest step timestamp. +19 tests, each mutation-checked.
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
|
Warning Review limit reached
Next review available in: 42 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughAgent lifecycle handling now supports race-safe deployment claims, all-version undeployment counts, concurrent deployment tracking, and timestamp-based idle-conversation cleanup. Regression tests cover deployment races, status transitions, undeployment behavior, and idle sweeps. ChangesAgent lifecycle management
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant AgentFactory
participant DeploymentMap
participant AgentStore
participant DeploymentEvent
Caller->>AgentFactory: deploy agent
AgentFactory->>DeploymentMap: publish IN_PROGRESS marker
AgentFactory->>AgentStore: load agent
AgentStore-->>AgentFactory: loaded agent or failure
AgentFactory->>DeploymentMap: publish READY or ERROR
AgentFactory->>DeploymentEvent: emit lifecycle event
AgentFactory-->>Caller: return result
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
CI caught two pinned tests: teardownAgent_undeployOnlyLeavesDeploymentRecordsAlone and teardownAgent_failedDeletePreservesDeploymentRecords. Both state the intent explicitly — deployment records track the Agent CONFIG's existence, so they are retired only when the config is deleted, and an undeploy is reversible by design. Retiring them unconditionally was a behavioural change to a deliberate policy, bundled into a bug fix. Reverted; the null-version undeploy fix stands on its own.
CodeQL log-injection alerts 487-489 on the three log calls added by this branch. agentId reaches deployAgent from REST path params and from stored deployment records, so it is user-influenced; LogSanitizer is the repo's existing answer. Also converts the two string-concatenated log.error calls to parameterized log.errorf while touching them.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/ai/labs/eddi/modules/llm/tools/TeardownAgentTool.java (1)
87-96: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse the removal count in the returned message.
undeployedVersionsis only logged. If the count is0,teardownAgentstill returns "✅ Agent '%s' has been undeployed successfully." at Line 113. The LLM then reports a teardown that did not happen. TheIAgentFactory#undeployAgentjavadoc states that callers must not describe a zero result as a teardown.Carry the count out of the
tryblock and report it.🔧 Proposed change
+ int undeployedVersions; try { - int undeployedVersions = agentFactory.undeployAgent(DEFAULT_ENV, agentId, null); + undeployedVersions = agentFactory.undeployAgent(DEFAULT_ENV, agentId, null); LOGGER.infof("[TEARDOWN] Undeployed agent '%s' (%d version(s))", agentId, undeployedVersions); } catch (Exception e) {Then use the count in the final message:
return undeployedVersions > 0 ? "✅ Agent '%s' has been undeployed successfully (%d version(s)).".formatted(agentId, undeployedVersions) : "ℹ️ Agent '%s' was not deployed — nothing to undeploy.".formatted(agentId);🤖 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/modules/llm/tools/TeardownAgentTool.java` around lines 87 - 96, Carry the undeployedVersions value from the agentFactory.undeployAgent call outside the try block in teardownAgent, then update the final success response to report the removed version count only when it is greater than zero; return the no-op message when the count is zero, while preserving the existing failure handling.
🧹 Nitpick comments (1)
src/main/java/ai/labs/eddi/engine/runtime/internal/AgentDeploymentManagement.java (1)
462-466: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCorrect the log wording.
DAYS.between(lastInteraction, Instant.now())is the total idle age, not the excess above the limit. The text "it is %d days older than the maximum idle time of %d days" states the opposite.✏️ Proposed wording
var message = format( - "Ended conversation (id: %s) with Agent (name: %s, id: %s, version: %d) " - + "because it is %d days older than the maximum idle time of %d days", + "Ended conversation (id: %s) with Agent (name: %s, id: %s, version: %d) " + + "because it was idle for %d days, which reaches the maximum idle time of %d days",🤖 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/engine/runtime/internal/AgentDeploymentManagement.java` around lines 462 - 466, Update the message constructed in AgentDeploymentManagement to describe DAYS.between(lastInteraction, Instant.now()) as the conversation’s idle age, not as days older than the configured maximum. Preserve the existing identifiers and maximum idle-time value while correcting the wording to match the logged value.
🤖 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/runtime/internal/AgentDeploymentManagement.java`:
- Around line 448-457: Update the null-handling in manageAgentDeployments around
lastInteractionOf and documentDescriptor.getLastModifiedOn(): when both the
interaction timestamp and descriptor lastModifiedOn are null, skip the current
conversation instead of invoking toInstant or isOlderThanDays. Preserve the
existing descriptor fallback when lastModifiedOn is available and allow
subsequent undeployment attempts to continue.
In `@src/main/java/ai/labs/eddi/engine/runtime/internal/AgentFactory.java`:
- Around line 210-226: Update the deployment completion logic in AgentFactory’s
deployment try block so a failed agentEnvironment.replace(...) does not report
READY. In the false branch, complete finalDeploymentProcess and notify
deploymentListener with an appropriate failure/non-READY Deployment.Status,
while retaining deployedAgents.add(id) and READY completion/event behavior only
for successful replacement.
---
Outside diff comments:
In `@src/main/java/ai/labs/eddi/modules/llm/tools/TeardownAgentTool.java`:
- Around line 87-96: Carry the undeployedVersions value from the
agentFactory.undeployAgent call outside the try block in teardownAgent, then
update the final success response to report the removed version count only when
it is greater than zero; return the no-op message when the count is zero, while
preserving the existing failure handling.
---
Nitpick comments:
In
`@src/main/java/ai/labs/eddi/engine/runtime/internal/AgentDeploymentManagement.java`:
- Around line 462-466: Update the message constructed in
AgentDeploymentManagement to describe DAYS.between(lastInteraction,
Instant.now()) as the conversation’s idle age, not as days older than the
configured maximum. Preserve the existing identifiers and maximum idle-time
value while correcting the wording to match the logged value.
🪄 Autofix
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 Plus
Run ID: 268cd55f-69c5-4f57-929d-6086dc770348
📒 Files selected for processing (8)
docs/changelog.mdsrc/main/java/ai/labs/eddi/engine/runtime/IAgentFactory.javasrc/main/java/ai/labs/eddi/engine/runtime/internal/Agent.javasrc/main/java/ai/labs/eddi/engine/runtime/internal/AgentDeploymentManagement.javasrc/main/java/ai/labs/eddi/engine/runtime/internal/AgentFactory.javasrc/main/java/ai/labs/eddi/modules/llm/tools/TeardownAgentTool.javasrc/test/java/ai/labs/eddi/engine/runtime/internal/AgentDeploymentManagementIdleSweepTest.javasrc/test/java/ai/labs/eddi/engine/runtime/internal/AgentFactoryLifecycleTest.java
…_PROGRESS marker Self-review of this branch: registering after the claim left a window in which a concurrent getAgent saw IN_PROGRESS, found no future to wait on, re-read the still-IN_PROGRESS marker and returned null — the exact symptom the handshake exists to prevent. registerAgentDeployment is computeIfAbsent, so registering before the claim is resolved is harmless: whoever wins the claim completes it.
Three findings, all valid: - AgentDeploymentManagement: the descriptor fallback dereferenced a nullable lastModifiedOn. The enclosing UndeploymentExecutor does not catch NPE, so it would have aborted every remaining undeploy attempt in that pass. With no age signal at all the conversation is now skipped — 'cannot prove it is idle' must never end a conversation. - AgentFactory: when the conditional publish loses to a concurrent undeploy the agent is not in the environment, so reporting READY was a lie with consequences — the deploy callback persists a 'deployed' record, which the redeploy sweep would use to resurrect the agent the teardown removed. That path now reports the non-READY outcome, which also completes the deployment future so no lookup waits out the timeout. - TeardownAgentTool: the removal count was logged but not used, so a teardown of something that was never deployed still returned '✅ undeployed successfully' — precisely the dishonesty this branch set out to remove. teardownAgent_undeploy never stubbed undeployAgent, so it asserted success against a 0-version teardown; stubbed explicitly and paired with a new test for the zero case. The mid-flight-undeploy test now also pins the non-READY report.
|
Thanks — all three findings were valid and are fixed in a2a1349 (the two inline threads plus the outside-diff On That also surfaced a stale test: |
|
Superseded — closing without merging. While this branch was in review, I re-checked every finding in this PR against current
No work is lost; the review threads here remain readable for the reasoning. |
3 participants
Follow-up review of
engine/runtime/internal— the pre-Wave-R deployment machinery everything else stands on. Five confirmed findings, all fixed, each pinned by a mutation-checked test (revert the fix, the test fails).1. HIGH —
undeployAgent(env, id, null)was a silent no-opAgentIdequality compares the version, soremove(new AgentId(id, null))matched nothing. Both callers that tear down a dynamically created agent passnull, because they know the agent only by id:TeardownAgentTool— LLM-callableGroupLifecycleOps#cleanupEphemeralAgentsThe agent stayed in the factory under its real version. So a "torn down" agent — and, with
delete=true, a config-deleted one — remained reachable throughgetLatestReadyAgentand fully conversable until JVM restart, whileeddi_agents_deployedleaked monotonically. The tool reported✅ ... undeployed successfullyregardless.nullnow means every version.undeployAgentreturns how many were actually removed, so a caller cannot report a teardown that did not happen.TeardownAgentToolalso retires the deployment records unconditionally rather than only on the delete path — left atdeployed, the 10s poll loaded the agent straight back in.Pinned by:
removesAllVersions(expected 2, got 0 against the old code),noLongerServable,decrementsGauge.2. The IN_PROGRESS deployment handshake had no producer
The marker agent was a local variable returned only from the failure branch, after being flipped to
ERROR— noIN_PROGRESSvalue ever rested in the map.getAgent's IN_PROGRESS branch andwaitForDeploymentCompletionwere therefore both unreachable, and a lookup arriving mid-deployment got a barenullinstead of waiting. The whole store load also ran insideConcurrentHashMap.compute, holding a bin lock across blocking I/O.The key is now claimed by publishing the marker atomically, the load runs outside the mapping function, and the result is published with a conditional
replace— an undeploy racing the load must not be resurrected by the late write. The factory registers and completes the deployment future itself.Pinned by:
inProgressIsObservable,undeployDuringLoadIsNotResurrected,runtimeExceptionClearsClaim.3. Collections with partial synchronization
deployedAgentswas aLinkedListwithaddundersynchronized,removewithout, and a Micrometer gauge readingsize()from the scrape thread.deploymentInfoshad the same shape —checkDeploymentsruns both on the 10s scheduler (@Scheduleddefaults toConcurrentExecution.PROCEED) and on the runtime executor at startup. Both are concurrent sets now.deploymentInfosis additionally rebuilt from each poll rather than appended to forever:DeploymentInfoequality ignores the status, so a stale entry used to suppress an agent's redeployment permanently.4. The idle sweep was wrong twice — and both had to be fixed together
isOlderThanDayswas written againstPeriod, which normalizes to years/months/days, and read only the years and days components. For a 35-day-old date and a 30-day limit,Period.between(now, date)isP-1M-4D, so the test read-4 <= -30and answered "not old". Whole bands of ages were never reaped.That bug was masking a wrong age signal: the age came from the AGENT document's
lastModifiedOn, which every conversation on that agent version shares. Fixing only the arithmetic would have startedENDingconversations a user was actively talking in, whenever the agent config happened to be old — a strictly worse failure than the one being fixed. So the signal is fixed in the same commit: age now comes from the conversation's own newest step timestamp, with the descriptor kept only as a fallback for a conversation carrying no timestamps at all.Pinned by:
recentConversationOnStaleAgentSurvives(an hour-old conversation on a two-year-stale agent must survive),noGapsAcrossMonthBoundaries(every offset 30→400 days).5.
Agent.deploymentStatusis nowvolatileThe transition to READY/ERROR is what releases a waiting lookup, so it must be visible without further synchronization.
Testing
111 existing tests in the affected classes stay green. +19 new. Every new test was mutation-checked by reverting its fix and confirming the failure.
Summary by CodeRabbit
Bug Fixes
IN_PROGRESS,READY, andERRORstatus visibility.Tests