fix(agents): null-version undeploy no-op, deploy under a CHM bin lock, EXECUTE wave deadline, protocol defaults - #648
Conversation
…, EXECUTE wave deadline, protocol defaults Wave A of a deep review of the Agent / Group Agent surface. Four of these five come from AgentFactory, two of them from a single six-line method. 1. undeployAgent(env, agentId, null) was a silent no-op. AgentId keys on (id, version), so new AgentId(id, null) equals no key the environment map ever holds. Both dynamic-agent teardown paths pass null (GroupLifecycleOps#cleanupEphemeralAgents after every group discussion, and TeardownAgentTool), so the removals did nothing while the caller logged success at INFO -- and deleteAllPermanently ran anyway. The constructed agent stayed resolvable via getLatestReadyAgent after its config had been deleted from the store, and eddi_agents_deployed grew monotonically for the process lifetime. A null version now means every deployed version, which is what the teardown callers mean; the REST/admin path passes a real version and stays exact. 2. deployAgent ran store I/O and full workflow construction inside ConcurrentHashMap.compute, holding a bin lock across multi-second I/O against the map's documented contract. The claim is now a putIfAbsent of an IN_PROGRESS placeholder with the load outside the map. The placeholder is now actually published (compute never did -- the dummy was only returned on failure), so a concurrent getAgent() can observe "in progress" instead of a bare null. 3. deployedAgents was appended under a synchronized block, removed with no lock at all, and read by the Micrometer gauge thread -- three unordered accesses to a LinkedList. Now CopyOnWriteArrayList with addIfAbsent. 4. The TASK_FORCE EXECUTE wave waited agentTimeoutSeconds x maxTasksPerAgent, which ignores retries (a RETRY member legitimately gets timeout x (maxRetries + 1)) and carries no setup grace. It now sizes itself through parallelBatchBudgetSeconds like the parallel debate batch and the bid round in the same file already did, via a new extracted TaskForceEngine#waveBudgetSeconds. 5. The documented 180s agentTimeoutSeconds default was unreachable: resolveProtocol's fallback and McpGroupTools.create_group both hard-coded 60, while the constant's Javadoc, the shipped templates and docs/group-conversations.md all said 180. Nothing backfills a protocol block at save time, so a group saved without one ran at 60. The defaults now live on ProtocolConfig as the single source of truth. Behaviour change, deliberate: groups with no protocol block (and MCP-created groups) go from a 60s to a 180s per-turn timeout, making the code agree with the documentation rather than the reverse. Also removes an unreachable version comparison in getAllLatestAgents and downgrades waitForDeploymentCompletion's "still IN_PROGRESS" ERROR to DEBUG when no deployment future was registered. 820 existing tests green, 19 new; the AgentFactory regression tests were mutation-checked (4 of 7 fail against the old behaviour).
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
|
Warning Review limit reached
Next review available in: 46 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. 📝 WalkthroughWalkthroughThe change centralizes protocol defaults, recalculates task-force wave budgets for retries and sequential tasks, and updates agent deployment tracking and null-version undeployment. New tests cover fallback resolution, wave budgets, deployment cleanup, gauges, and race handling. ChangesAgent lifecycle and execution budgets
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant AgentFactory
participant ConcurrentHashMap
participant AgentStore
AgentFactory->>ConcurrentHashMap: Claim deployment with IN_PROGRESS placeholder
AgentFactory->>AgentStore: Load agent outside map operation
AgentStore-->>AgentFactory: Return loaded agent or failure
AgentFactory->>ConcurrentHashMap: Publish READY agent or ERROR state
AgentFactory->>ConcurrentHashMap: Remove all versions for null-version undeploy
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-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![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
🤖 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/configs/groups/model/AgentGroupConfiguration.java`:
- Around line 943-955: Update the `agentTimeoutSeconds` record Javadoc to state
that its default is 180 seconds, matching `DEFAULT_AGENT_TIMEOUT_SECONDS`; leave
the constant and other documentation unchanged.
In `@src/main/java/ai/labs/eddi/engine/runtime/internal/AgentFactory.java`:
- Around line 211-218: Synchronize the deployment completion transition in
AgentFactory with undeployAgent for the same AgentId so loading an agent cannot
restore it after teardown. Coordinate agentEnvironment replacement and
deployedAgents updates atomically; do not merely change put to replace. Apply
the same protection to the other deployment completion path around the
referenced logic, and add a regression test that blocks getAgent, undeploys,
then releases the lookup and verifies the agent remains undeployed.
🪄 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: 618afb1e-eba0-4d7e-b36e-a4e58ca34f63
📒 Files selected for processing (10)
docs/changelog.mdsrc/main/java/ai/labs/eddi/configs/groups/model/AgentGroupConfiguration.javasrc/main/java/ai/labs/eddi/engine/internal/GroupConversationService.javasrc/main/java/ai/labs/eddi/engine/internal/groups/TaskForceEngine.javasrc/main/java/ai/labs/eddi/engine/mcp/McpGroupTools.javasrc/main/java/ai/labs/eddi/engine/runtime/IAgentFactory.javasrc/main/java/ai/labs/eddi/engine/runtime/internal/AgentFactory.javasrc/test/java/ai/labs/eddi/engine/internal/GroupConversationServiceProtocolDefaultsTest.javasrc/test/java/ai/labs/eddi/engine/internal/groups/TaskForceEngineWaveBudgetTest.javasrc/test/java/ai/labs/eddi/engine/runtime/internal/AgentFactoryUndeployVersionTest.java
…tion, stale Javadoc Addresses all four findings on #648. CodeRabbit (Major) — and a genuine regression this PR introduced. Moving the store load out of ConcurrentHashMap.compute opened a window in which an undeployAgent for the same id could land mid-load, after which the unconditional put of the finished agent resurrected it: a deployment silently winning a race it started before the undeploy was even requested, and leaving the metric describing a registry that no longer held it. Publication is now replace(key, OUR placeholder, agent), so an interleaved undeploy or competing redeploy keeps its outcome. The reviewer's further point -- coordinate the map and the metric together, do not only swap put for replace -- is addressed by deleting the parallel structure entirely. deployedAgents is gone; eddi_agents_deployed is now a Gauge derived from the environment maps (READY entries only, matching the old semantics). Two structures holding one fact could never be linearized against each other; one structure needs no linearization. CodeQL (4x log injection): the three claim-path log.debug(String.format( ...)) calls and the deploy-failure log.error interpolated an unsanitized agentId. All now go through LogSanitizer.sanitize, and the debug calls use debugf rather than pre-formatting. CodeRabbit (Minor): ProtocolConfig's @PARAM agentTimeoutSeconds Javadoc still said "default: 60". Code-quality bot: a redundant assertion in TaskForceEngineWaveBudgetTest (budget > 30 is implied by budget >= 90) replaced with assertNotEquals(30L, budget), keeping the "not the old formula's output" intent without the always-true comparison. New test undeployDuringLoadIsNotOverwritten blocks the store lookup, undeploys, then releases -- mutation-verified against a put. 111 tests green across the AgentFactory/AgentDeploymentManagement suites.
|
Thanks — all four addressed in @coderabbitai (Major) — deployment restoring an undeployed agent. Correct, and it was a regression this PR introduced: moving the store load out of On your second point — "do not only replace put with replace; coordinate the map and metric tracking transition together" — I took the stronger option and deleted the parallel structure. Interleaving regression test added as requested ( @github-advanced-security (CodeQL, 4× log injection). Fixed: the three claim-path @coderabbitai (Minor) — stale Javadoc. Fixed: @github-code-quality — useless comparison. Fixed as suggested: |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
git show --stat --oneline ba8f566
git show --format=fuller --find-renames --find-copies -- \
src/main/java/ai/labs/eddi/engine/runtime/internal/AgentFactory.java \
src/test/java/ai/labs/eddi/engine/runtime/internal/AgentFactoryUndeployVersionTest.java \
src/main/java/ai/labs/eddi/engine/runtime/IAgentFactory.java \
src/main/java/ai/labs/eddi/engine/internal/groups/TaskForceEngine.java \
ba8f566
printf '\n--- AgentFactory outline ---\n'
ast-grep outline src/main/java/ai/labs/eddi/engine/runtime/internal/AgentFactory.java --items all --view expanded
printf '\n--- Focused deployment/metric/logging references ---\n'
rg -n -C 4 \
'putIfAbsent|replace\(|deployedAgents|Gauge|LogSanitizer|debugf|undeployAgent|IN_PROGRESS|READY' \
src/main/java/ai/labs/eddi/engine/runtime/internal/AgentFactory.java
printf '\n--- Focused regression test ---\n'
rg -n -C 8 \
'undeployDuringLoadIsNotOverwritten|CountDownLatch|gauge|MeterRegistry|undeployAgent' \
src/test/java/ai/labs/eddi/engine/runtime/internal/AgentFactoryUndeployVersionTest.java
printf '\n--- Javadoc and wave-budget assertion ---\n'
rg -n -C 3 \
'`@param` agentTimeoutSeconds|assertNotEquals\(30L' \
src/main/java/ai/labs/eddi/engine/runtime/IAgentFactory.java \
src/main/java/ai/labs/eddi/engine/internal/groups/TaskForceEngine.java \
src/test/java/ai/labs/eddi/engine/internal/groups/TaskForceEngineWaveBudgetTest.javaLength of output: 40694
The derived The claim-path logs now sanitize 🐇 ✅ ✏️ Learnings added
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! You are interacting with an AI system. |
…ure; honest gauge semantics Two findings from a model-independent second pass over the full diff. 1. waitForDeploymentCompletion armed orTimeout(60s) on the SHARED registration future. orTimeout mutates the future it is called on, and DeploymentListener hands the same instance to every waiter and to whoever registered the deployment. Pre-#651 only the ZIP importer ever held one, so the mutation was near-unreachable; once #651 registers on every REST deploy, one impatient getAgent caller would -- at its own 60s deadline -- complete the shared future exceptionally for every other consumer (failing waiters whose deployment was still legitimately in flight) and evict the registration from the listener's map before the real deployment event arrived. Now a timed get(), which waits without writing. The newly-reachable InterruptedException restores the interrupt flag, and a timeout logs at WARN as this caller's outcome rather than ERROR as the deployment's. 2. The gauge comment claimed "matching the old semantics"; one delta is real and deliberate. The old deployedAgents list keyed on (id, version) with no environment, so an agent deployed to both production and test counted once; counting map entries counts it per environment -- per actual deployment. The truthful reading, now stated in the comment and the changelog instead of implied away. 110 tests green across AgentFactory*/DeploymentListener*/ AgentDeploymentManagement*.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/java/ai/labs/eddi/engine/runtime/internal/AgentFactory.java (2)
307-312: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDo not remove a replacement deployment from a stale snapshot.
Line 311 removes each snapshotted key without checking its mapped value. If another caller removes and redeploys one version after the snapshot, this loop can remove the newer placeholder or READY agent.
Capture each matching key and value, then use
remove(key, capturedValue). Add an interleaving test for a null-version undeploy racing with a redeploy of one captured version.Proposed fix
- List<AgentId> allVersions = agentEnvironment.keySet().stream() - .filter(key -> Objects.equals(key.getId(), agentId)) + List<Map.Entry<AgentId, IAgent>> allVersions = agentEnvironment.entrySet().stream() + .filter(entry -> Objects.equals(entry.getKey().getId(), agentId)) + .map(entry -> Map.entry(entry.getKey(), entry.getValue())) .toList(); - allVersions.forEach(agentEnvironment::remove); + allVersions.forEach(entry -> agentEnvironment.remove(entry.getKey(), entry.getValue()));🤖 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/AgentFactory.java` around lines 307 - 312, Update the null-version branch in AgentFactory’s undeploy logic to snapshot each matching key together with its mapped value, then remove entries conditionally via remove(key, capturedValue) so redeployed replacements are preserved. Add an interleaving test covering null-version undeploy racing with redeployment of one captured version.
150-196: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftRemove the synchronous deployment wait.
Line 164 can block a backend caller for 60 seconds. This can exhaust request or worker threads during concurrent deployments.
Return a not-ready result immediately, or propagate deployment completion through a non-blocking API.
As per coding guidelines, backend code must be thread-safe and non-blocking; use AsyncResponse for REST endpoints and avoid extended blocking in tasks.
🤖 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/AgentFactory.java` around lines 150 - 196, Remove the blocking deploymentFuture.get(...) call from waitForDeploymentCompletion. Return the existing not-ready result immediately when deploymentFuture indicates an in-progress deployment, or propagate completion through a non-blocking API; preserve the agent lookup and status handling without synchronously waiting up to DEPLOYMENT_WAIT_SECONDS.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 `@docs/changelog.md`:
- Around line 23-24: Update the two changelog bullets in the deployment-wait and
gauge descriptions: complete the sentence describing waitForDeploymentCompletion
so it directly states that it uses a timed get() without mutating the shared
future, and revise the gauge wording to state clearly that READY deployments are
counted per environment.
---
Outside diff comments:
In `@src/main/java/ai/labs/eddi/engine/runtime/internal/AgentFactory.java`:
- Around line 307-312: Update the null-version branch in AgentFactory’s undeploy
logic to snapshot each matching key together with its mapped value, then remove
entries conditionally via remove(key, capturedValue) so redeployed replacements
are preserved. Add an interleaving test covering null-version undeploy racing
with redeployment of one captured version.
- Around line 150-196: Remove the blocking deploymentFuture.get(...) call from
waitForDeploymentCompletion. Return the existing not-ready result immediately
when deploymentFuture indicates an in-progress deployment, or propagate
completion through a non-blocking API; preserve the agent lookup and status
handling without synchronously waiting up to DEPLOYMENT_WAIT_SECONDS.
🪄 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: aad1d516-2b4a-4501-a7bc-84aab58cf66c
📒 Files selected for processing (5)
docs/changelog.mdsrc/main/java/ai/labs/eddi/configs/groups/model/AgentGroupConfiguration.javasrc/main/java/ai/labs/eddi/engine/runtime/internal/AgentFactory.javasrc/test/java/ai/labs/eddi/engine/internal/groups/TaskForceEngineWaveBudgetTest.javasrc/test/java/ai/labs/eddi/engine/runtime/internal/AgentFactoryUndeployVersionTest.java
🚧 Files skipped from review as they are similar to previous changes (2)
- src/test/java/ai/labs/eddi/engine/internal/groups/TaskForceEngineWaveBudgetTest.java
- src/main/java/ai/labs/eddi/configs/groups/model/AgentGroupConfiguration.java
|
Final review pass (independent second-model review of the full diff) — two further items, fixed in the latest commit:
The pass also re-verified the deploy/undeploy CAS, the null-version undeploy, and the wave-budget derivation against the live code paths — no further findings. |
… (review nitpick)
|
Fixed the changelog wording nitpick in the latest commit — both bullets now lead with the behavior ("waits with a timed |
…not yet supported" section
Wave E of the Agent / Group Agent review -- the documentation drifts the
review turned up, each one a place where docs and code disagreed.
1. Attachments x groups were entirely undocumented. group-conversations.md
had zero mentions of attachments and attachments-guide.md zero mentions
of groups, while POST /groups/{groupId}/conversations accepts them and
GroupAttachmentBinder is a whole subsystem. Both files now cover the
three input shapes, the first-turn grant, how later phases keep access
(history plus the auto-enabled readAttachment tool), and the two
group-specific bounds: the per-turn cap applies per MEMBER turn, and
anything dropped is reported in that member's attachments:errors, not
in the group transcript.
2. The protocol table did not say the defaults apply to an absent block.
Nothing backfills a stored config, so "no protocol block" is the common
shape -- which is what made the 60-vs-180 drift fixed in labsai#648
invisible.
3. maxCreatedAgentsPerDiscussion now reads "counted across all members,
not per member" -- the behaviour labsai#649 delivers.
4. LAST_PHASE was documented as "only the previous phase's entries", but
the filter is phaseIndex >= currentPhaseIdx - 1, which includes the
running phase. The code is right -- in a sequential phase that is what
lets the second speaker react to the first -- so the doc and the enum
Javadoc were corrected, not the filter.
5. New "Not yet supported" section: member-level tool approval inside a
group, nested pauses, groups over the OpenAI-compatible /v1 adapter,
groups over A2A, and the per-node scope of the live-discussion
registry.
Also: an FQN sweep of CreateSubAgentTool (11 inline fully-qualified names,
against AGENTS.md 4.7) and the orphaned Javadoc in LiveDiscussionRegistry,
where the paragraph documenting get() sat above getForMember() so both
attached to the latter and get() had none.
Scoping: the FQN violation is repo-wide (~130 sites). This sweeps only
files no other open PR touches; the rest is a follow-up once labsai#648-labsai#651
land.
3 participants
Wave A of a deep review of the Agent / Group Agent surface. The review's structural finding was that the group feature layer has been reviewed exhaustively while the agent lifecycle layer it stands on has not — four of these five fixes come from
AgentFactory, two of them from a single six-line method.1.
undeployAgent(env, agentId, null)was a silent no-op — ephemeral agents leakedAgentFactory.AgentIdkeys on(id, version), sonew AgentId(id, null)equals no key the environment map ever holds. Both dynamic-agent teardown paths passnull:GroupLifecycleOps#cleanupEphemeralAgents— runs after every group discussionTeardownAgentTool— the LLM-facing teardown toolSo
agentEnvironment.remove(...)anddeployedAgents.remove(...)both did nothing, while the caller logged"undeployed agent '%s'"at INFO.agentStore.deleteAllPermanently(agentId)then ran anyway. Net result: the constructed agent stayed resolvable throughgetLatestReadyAgentafter its configuration had been deleted from the store, andeddi_agents_deployedgrew monotonically for the lifetime of the process.A null version now means every deployed version of that agent — the only reading that matches what the teardown callers mean. The REST/admin path passes a real version and stays exact, which is why this was never noticed.
2.
deployAgentran store I/O insideConcurrentHashMap.computecomputeholds the bin lock for the key while the mapping function runs, and the function's real work wasagentStoreClientLibrary.getAgent()— a store read plus full workflow construction.ConcurrentHashMapdocuments that a mapping function must be short and must not touch other mappings of the same map; this held a bin lock across multi-second I/O, and any re-entrant agent resolution during construction would have deadlocked.The claim is now a
putIfAbsentof an IN_PROGRESS placeholder (atomic, no long hold) with the load outside the map. Side benefit: the placeholder is now actually published, whichcomputenever did — the dummy was only ever returned on the failure path — so a concurrentgetAgent()can observe "deployment in progress" instead of a bare null.3.
deployedAgentswas mutated unsynchronizedAppended under
synchronized (deployedAgents), removed with no lock at all, and read by the Micrometer gauge thread — three unordered accesses to aLinkedList. Now aCopyOnWriteArrayList, withaddIfAbsentreplacing a non-atomiccontains()-then-add().4. The TASK_FORCE EXECUTE wave gave up before its turns could have
The wave waited
agentTimeoutSeconds × maxTasksPerAgent, which ignores retries — underonAgentFailure=RETRYa member legitimately getstimeout × (maxRetries + 1)— and carries no setup grace, so even a one-task no-retry wave could expire while the member was still inside its own budget (a member turn reaches its response wait only after agent lookup, conversation start and attachment grants).That is exactly what
PARALLEL_BATCH_GRACE_FLOOR_SECONDSexists to prevent, and both the parallel debate batch and the bid round in the same file already sized themselves throughparallelBatchBudgetSeconds. The wave now does too, via a new extractedTaskForceEngine#waveBudgetSecondsso the derivation is assertable without timing a real wave.5. The documented 180s
agentTimeoutSecondsdefault was unreachableresolveProtocol's fallback handed out a literal60, andMcpGroupTools.create_grouphard-coded60— while the constant's own Javadoc, the four shipped templates and the published table indocs/group-conversations.mdall said 180 (the value introduced because 60 timed out thinking models during synthesis). Since nothing backfills aprotocolblock at save time, a group saved without one — the common shape — ran at 60.The defaults now live on
ProtocolConfig(DEFAULT_AGENT_TIMEOUT_SECONDS,DEFAULT_MAX_RETRIES) as the single source of truth, referenced by the engine, the MCP tool and the follow-up path (resolveAgentTimeoutSeconds, which had its own strayreturn 60).Also
getAllLatestAgents— it compared the result ofgetLatestAgent, which already returns the highest version, against itself.waitForDeploymentCompletion's "still IN_PROGRESS" ERROR to DEBUG when no deployment future was registered. With the placeholder now published that state is reachable and ordinary. Wiring the registration properly is a follow-up.Testing
AgentFactory*,AgentDeploymentManagement*,TaskForceEngine*,GroupConversationService*,McpGroupTools*,AgentGroupConfiguration*,RestAgentAdministration*.AgentFactoryregression tests were mutation-checked: reverting the null-version branch fails 4 of 7.Summary by CodeRabbit
Bug Fixes
Improvements