fix(agents): wire the deployment-wait machinery that only the ZIP importer ever used - #651
Merged
Conversation
…orter ever used Wave F of the Agent / Group Agent review. AgentFactory.getAgent has always had a branch for "the agent is deploying right now" -- waitForDeploymentCompletion, which awaits a future from DeploymentListener. That future was only ever registered by ONE caller in all of src/main: RestImportService, the startup ZIP importer. Every ordinary deploy fired onDeploymentEvent but never registered, so getRegisteredDeploymentEvent returned null, the wait had nothing to await, and a caller racing a deployment simply got a null agent. The machinery was dead outside one flow while reading as live. - RestAgentAdministration.deploy now registers BEFORE starting the deployment. Ordering matters: agentFactory.deployAgent is what publishes the IN_PROGRESS placeholder a waiter can observe, so registering afterwards would leave exactly the window this closes. - DeploymentListener.registerAgentDeployment self-expires. The map was pruned only by an arriving DeploymentEvent, so a registration whose event never came (a rejected deployment callable, a process that died mid-deploy) stayed for the lifetime of the JVM. Registrations now carry a REGISTRATION_TTL (5 minutes -- a leak bound, not a deployment SLA) and remove themselves on any completion. Removal is remove(key, future), not remove(key), so a stale completion cannot evict a live registration for the same agent. - RestImportService's allOf(...).join() is now tolerant. It could previously only block forever; with self-expiring registrations it can complete exceptionally, and one initial agent that never reports must not hang startup. 347 tests green; 12 new.
Contributor
📝 WalkthroughWalkthroughDeployment registration now occurs before asynchronous deployment. Registrations expire after five minutes and remove only their matching futures. ZIP-import startup logs deployment failures and continues. New tests cover registration, completion, expiry, stale cleanup, and waiter outcomes. ChangesDeployment wait lifecycle
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant RestAgentAdministration
participant DeploymentListener
participant DeploymentTask
participant DeploymentWaiter
RestAgentAdministration->>DeploymentListener: Register agent/version
RestAgentAdministration->>DeploymentTask: Start asynchronous deployment
DeploymentTask->>DeploymentListener: Publish READY or ERROR
DeploymentListener->>DeploymentWaiter: Complete or fail future
DeploymentListener->>DeploymentListener: Remove matching future or expire after five minutes
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
ginccc
added a commit
that referenced
this pull request
Aug 9, 2026
…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*.
aisabella-ai
self-requested a review
August 10, 2026 17:32
aisabella-ai
approved these changes
Aug 10, 2026
…achinery # Conflicts: # docs/changelog.md
…achinery # Conflicts: # docs/changelog.md
…achinery # Conflicts: # docs/changelog.md
![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: 3
🤖 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/backup/impl/RestImportService.java`:
- Around line 156-164: Remove the terminal join from the CompletableFuture chain
in the deployment observation flow, so allOf(...).handle(...) runs
asynchronously without blocking the caller. Preserve the existing warning
behavior for incomplete deployment events, and expose or return the resulting
future through the surrounding method if deployment progress must remain
observable.
In `@src/main/java/ai/labs/eddi/engine/internal/RestAgentAdministration.java`:
- Around line 217-227: Update the deployment flow around
deploymentListener.registerAgentDeployment and the subsequent READY
event/schedule enablement so an observer that encounters IN_PROGRESS does not
publish READY or enable schedules. Track whether this callable started the
deployment, and only perform those completion actions when it started deployment
or the registration was already READY; leave IN_PROGRESS unchanged for the
owning deployment.
In
`@src/test/java/ai/labs/eddi/engine/runtime/internal/DeploymentListenerRegistrationTest.java`:
- Around line 128-142: Update expiryDoesNotEvictALaterRegistration to
synchronize on a deterministic barrier after the first registration is removed
but before its completion callback finishes. Register second during that
callback’s paused window, then complete first and release the barrier, asserting
that getRegisteredDeploymentEvent still returns second; do not rely on
first.complete(null) after whenComplete has already run.
🪄 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: cc33e9dd-a4a2-423c-8aaa-3f6a1b2561e8
📒 Files selected for processing (5)
docs/changelog.mdsrc/main/java/ai/labs/eddi/backup/impl/RestImportService.javasrc/main/java/ai/labs/eddi/engine/internal/RestAgentAdministration.javasrc/main/java/ai/labs/eddi/engine/runtime/internal/DeploymentListener.javasrc/test/java/ai/labs/eddi/engine/runtime/internal/DeploymentListenerRegistrationTest.java
Comment on lines
+156
to
+164
| CompletableFuture.allOf(deploymentFutures.toArray(new CompletableFuture[0])) | ||
| .handle((ignored, error) -> { | ||
| if (error != null) { | ||
| LOGGER.warnf("Not every initial agent reported a deployment event (%s) — continuing with those that did", | ||
| error.getMessage()); | ||
| } | ||
| return null; | ||
| }) | ||
| .join(); |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Remove the five-minute blocking wait.
join() blocks the caller until every deployment settles. A missing event now delays this path for up to REGISTRATION_TTL, which is five minutes. Run deployment observation asynchronously and expose deployment progress separately if callers need it.
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/backup/impl/RestImportService.java` around lines
156 - 164, Remove the terminal join from the CompletableFuture chain in the
deployment observation flow, so allOf(...).handle(...) runs asynchronously
without blocking the caller. Preserve the existing warning behavior for
incomplete deployment events, and expose or return the resulting future through
the surrounding method if deployment progress must remain observable.
Source: Coding guidelines
Comment on lines
+217
to
+227
| // Register BEFORE the deployment starts, so a concurrent getAgent that finds | ||
| // the agent IN_PROGRESS has a future to await. | ||
| // | ||
| // This is what makes AgentFactory's wait machinery reachable at all. Only | ||
| // RestImportService ever registered, so for every ordinary deploy | ||
| // getRegisteredDeploymentEvent returned null, waitForDeploymentCompletion had | ||
| // nothing to await, and a caller racing a deployment simply got a null agent. | ||
| // Registration must precede agentFactory.deployAgent: that call is what | ||
| // publishes the IN_PROGRESS placeholder a waiter can observe, so registering | ||
| // afterwards would leave exactly the window this closes. | ||
| deploymentListener.registerAgentDeployment(agentId, version); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not let an observer request complete the active deployment.
A concurrent deploy request reuses this registration. If it sees IN_PROGRESS, it skips agentFactory.deployAgent but still reaches the unconditional READY event and schedule enablement at Lines 239-242. That completes the shared future before the original deployment finishes.
Only publish READY and enable schedules when this callable started the deployment or when the status was already READY. Leave an IN_PROGRESS registration for the deployment owner.
Proposed fix
- if (EnumSet.of(NOT_FOUND, ERROR).contains(checkDeploymentStatus(environment, agentId, version))) {
+ Status status = checkDeploymentStatus(environment, agentId, version);
+ if (EnumSet.of(NOT_FOUND, ERROR).contains(status)) {
agentFactory.deployAgent(environment, agentId, version, status -> {
if (status == READY && autoDeploy) {
deploymentStore.setDeploymentInfo(environment.toString(), agentId, version, DeploymentInfo.DeploymentStatus.deployed);
}
});
+ status = READY;
}
- deploymentListener.onDeploymentEvent(new DeploymentEvent(agentId, version, environment, READY));
-
- // Lifecycle hook: auto-enable schedules for this agent
- enableSchedulesForAgent(agentId);
+ if (status == READY) {
+ deploymentListener.onDeploymentEvent(new DeploymentEvent(agentId, version, environment, READY));
+ enableSchedulesForAgent(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/engine/internal/RestAgentAdministration.java`
around lines 217 - 227, Update the deployment flow around
deploymentListener.registerAgentDeployment and the subsequent READY
event/schedule enablement so an observer that encounters IN_PROGRESS does not
publish READY or enable schedules. Track whether this callable started the
deployment, and only perform those completion actions when it started deployment
or the registration was already READY; leave IN_PROGRESS unchanged for the
owning deployment.
Comment on lines
+128
to
+142
| @Test | ||
| @DisplayName("an expired registration does not evict a later one for the same agent") | ||
| void expiryDoesNotEvictALaterRegistration() { | ||
| var first = listener.registerAgentDeployment("agent-1", 1); | ||
| listener.onDeploymentEvent(event("agent-1", 1, Deployment.Status.READY)); | ||
|
|
||
| var second = listener.registerAgentDeployment("agent-1", 1); | ||
| assertNotSame(first, second, "precondition: the first was evicted, so this is a fresh registration"); | ||
|
|
||
| // The first future's completion callback must not remove the second entry. | ||
| first.complete(null); | ||
|
|
||
| assertSame(second, listener.getRegisteredDeploymentEvent("agent-1", 1), | ||
| "remove(key, future) — a stale completion must not evict a live registration"); | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline src/test/java/ai/labs/eddi/engine/runtime/internal/DeploymentListenerRegistrationTest.java \
--items all --match 'expiryDoesNotEvictALaterRegistration'
sed -n '128,142p' src/test/java/ai/labs/eddi/engine/runtime/internal/DeploymentListenerRegistrationTest.java
sed -n '59,92p' src/main/java/ai/labs/eddi/engine/runtime/internal/DeploymentListener.javaRepository: labsai/EDDI
Length of output: 2875
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- test structure and helpers ---'
ast-grep outline src/test/java/ai/labs/eddi/engine/runtime/internal/DeploymentListenerRegistrationTest.java
sed -n '1,190p' src/test/java/ai/labs/eddi/engine/runtime/internal/DeploymentListenerRegistrationTest.java
printf '%s\n' '--- production declarations and usages ---'
ast-grep outline src/main/java/ai/labs/eddi/engine/runtime/internal/DeploymentListener.java
rg -n -C 4 'registerAgentDeployment|onDeploymentEvent|deploymentFutures|whenComplete|orTimeout' \
src/main/java/ai/labs/eddi/engine/runtime/internal/DeploymentListener.java \
src/test/java/ai/labs/eddi/engine/runtime/internal
printf '%s\n' '--- repository diff summary ---'
git diff --statRepository: labsai/EDDI
Length of output: 41105
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
test = Path("src/test/java/ai/labs/eddi/engine/runtime/internal/DeploymentListenerRegistrationTest.java")
source = test.read_text()
start = source.index("void expiryDoesNotEvictALaterRegistration")
end = source.find("\n `@Test`", start)
print(source[start:end])
listener = Path("src/main/java/ai/labs/eddi/engine/runtime/internal/DeploymentListener.java").read_text()
start = listener.index("public CompletableFuture<Void> registerAgentDeployment")
end = listener.index("\n }", start) + 6
print("\n--- registration method ---")
print(listener[start:end])
PY
printf '%s\n' '--- all relevant call sites ---'
rg -n -C 3 'registerAgentDeployment\(|onDeploymentEvent\(' src/main src/testRepository: labsai/EDDI
Length of output: 33298
Exercise the stale-completion interleaving.
first.complete(null) runs after the first whenComplete callback has already executed, so it does not test stale cleanup. Add a deterministic barrier after map removal. Register second before completing first, then assert that second remains registered.
🤖 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/engine/runtime/internal/DeploymentListenerRegistrationTest.java`
around lines 128 - 142, Update expiryDoesNotEvictALaterRegistration to
synchronize on a deterministic barrier after the first registration is removed
but before its completion callback finishes. Register second during that
callback’s paused window, then complete first and release the barrier, asserting
that getRegisteredDeploymentEvent still returns second; do not rely on
first.complete(null) after whenComplete has already run.
pull Bot
pushed a commit
to Stars1233/EDDI
that referenced
this pull request
Aug 10, 2026
…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.
This was referenced Aug 10, 2026
Closed
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
2 participants
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.
Wave F of the Agent / Group Agent review (Wave A #648, B #649, C #650).
The finding
AgentFactory.getAgenthas always had a branch for "the agent is deploying right now":waitForDeploymentCompletionawaits a future fromDeploymentListener.getRegisteredDeploymentEvent. That future was only ever registered by one caller in all ofsrc/main—RestImportService, the startup ZIP importer.Every ordinary deploy fires
onDeploymentEventbut never registers, sogetRegisteredDeploymentEventreturnednull, the wait had nothing to await, and a caller racing a deployment simply got a null agent. The machinery was dead outside one flow while reading as live — the worst state for a piece of code to be in.The fix
Register before deploying.
RestAgentAdministration.deploynow callsregisterAgentDeploymentbefore submitting the deployment callable. Ordering matters:agentFactory.deployAgentis what publishes the IN_PROGRESS placeholder a waiter can observe, so registering afterwards would leave exactly the window this closes.Bound the map.
deploymentFutureswas pruned only by an arrivingDeploymentEvent, so a registration whose event never came — a rejected deployment callable, a process that died mid-deploy — stayed for the lifetime of the JVM. Registrations now carry aREGISTRATION_TTL(5 minutes; a leak bound, not a deployment SLA — a waiter that wants tighter arms its own, asAgentFactorydoes at 60s) and remove themselves on any completion: normal, exceptional or timed-out. Removal isremove(key, future), notremove(key), so a stale completion cannot evict a live registration for the same agent+version.Don't let one agent hang startup.
RestImportService'sallOf(...).join()could previously only block forever. With self-expiring registrations it can now complete exceptionally, so it logs and continues with the agents that did deploy rather than wedging boot.Testing
347 tests green across
DeploymentListener*,RestAgentAdministration*,AgentFactory*,RestImportService*; 12 new inDeploymentListenerRegistrationTestcovering discovery, idempotent registration, version independence, READY/ERROR settlement, expiry-without-leak, and that a stale completion cannot evict a live registration.Summary by CodeRabbit
Bug Fixes
Documentation