fix(runtime): concurrency, lifecycle, cancellation, graceful shutdown, Dream wiring (wave 3) - #619
Merged
Merged
Conversation
…ancellation, graceful shutdown, Dream wiring 16 fixed, 1 partial. The wave where the obvious fix is often the wrong one. C1 (critical): cancellation cancelled nothing. CompletableFuture.cancel(true) does not interrupt a runAsync/supplyAsync body — the JDK documents mayInterruptIfRunning as having no effect there. Five call sites relied on it, so "cancelled" agent threads kept mutating the task list, transcript and errors after the orchestrator had already persisted the group document. Replaced with a cooperative cancellation token checked at the turn's own await points. This is also the root cause of C7, where resetStrandedInProgressTasks could strand the very task it exists to rescue. C4: a ~100-turn scalability cliff. The inner pipeline was submitted through the same bounded pool as the outer coordinator callable and then blocked on get(). At the 200-thread default, ~100 concurrent turns leaves every thread a waiter, no inner task can be scheduled, and every turn fails at the watchdog. Nested submissions now route to a virtual-thread executor via a ThreadLocal marker scoped to the callable body. Chosen over CompletableFuture composition because the coordinator's contract is "callable returns => turn is done"; making the outer non-blocking would need an SPI change and would let the next turn of the same conversation start while the previous one still ran. C9/C13: a completed turn could be re-executed. onComplete sat inside the try whose catch(Throwable) called onFailure, which resubmitted the already-executed callable — LLM calls, tool side effects and cost running twice. Completion moved outside the guarded region and both callbacks gated behind a one-shot flag; the coordinator's blind 3x retry removed. C3: a timed-out turn still persisted over a newer one, because the stale-completion guard used the interrupt flag and the pipeline cleared it. Replaced with a per-submission abandonment token set before delegating cancel(). C10: a throwing submit left the callable queued with nothing scheduled to run it, wedging that conversation permanently and leaking the map entry. C5: /rerun destroyed output and regenerated nothing — the component-cache key was built from a sublist-relative index while the cache stores under the absolute index, so the output task ran with a null component after the prior output was already deleted. It survived because every existing LifecycleManagerTest stubs the component map empty, which is exactly the condition that hides it. B3: no graceful shutdown existed anywhere — no ShutdownEvent observer in src/main at all, so a rolling deploy dropped every queued and in-flight turn. B2: the finding's premise was inverted. It claimed 18 of 20 handlers swallow the interrupt; auditing all 28 sites found 14 already restore correctly and 9 rethrow, with only 4 genuine swallowers. Fixed those, plus a site hidden behind a broad catch(Exception) in ScheduleFireExecutor (the poller kept firing after being interrupted for shutdown) and the mirror bug in NatsConversationCoordinator, which restored the flag unconditionally and would abort its own @PreDestroy. I1/G8: DreamService wired to ScheduleFireExecutor per the owner's decision, with a dollar-based maxCostPerRun ceiling. G8 matters much more now it actually runs: consolidation upgraded self visibility to global whenever a group spanned multiple agents, and cross-agent grouping was the default path. F6 partial: the REST/pipeline half (ConnectionCallback sets cancelled on client disconnect) is done; the in-modules/llm half is deferred to that workstream. Clean compile passed first attempt despite three cross-workstream signature changes. 12,633 tests, 0 non-environmental failures. C1, C5, C9 and B2 all mutation-checked against whole test classes.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe PR updates Dream consolidation, conversation cancellation and shutdown, runtime failure handling, group execution, lifecycle indexing, parser caching, schedule authorization, streaming disconnect handling, interrupt propagation, documentation, and regression tests. ChangesRuntime orchestration and Dream consolidation
Estimated code review effort: 5 (Critical) | ~120 minutes 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 |
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Member
Author
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 7 minutes. |
aisabella-ai
approved these changes
Jul 28, 2026
…review fixes) Both conflicts are two independent changes to the same code, where neither side subsumes the other: - ConversationService.processConversationStep — this branch extracted the body into runConversationStep so a try/finally could guarantee processingTurn.release() (C11); main added the caller-identity capture in the same method. Resolution keeps the C11 wrapper AND main's capture, with the capture staying OUTSIDE the returned lambda — it has to run on the REST request thread, since the lambda executes on a pool thread with no request to capture from. The bound callable is passed through runConversationStep into runGuardedConversationStep, which is exactly where main used it, so behaviour is unchanged in both directions. - GroupConversationService parallel phase — this branch added cancellation propagation (the `cancellation` argument plus a MemberTurnCancelledException catch that surfaces it instead of fabricating a contribution); main wrapped the supplier in withIdentitySupplying so the caller follows the fan-out onto further virtual threads. Both kept. The cancellation catch must stay ABOVE the generic Exception catch, which would otherwise turn a cancellation back into an error transcript entry — the exact behaviour that catch exists to prevent. The silent half, same as the #618 merge: main added a CallerIdentityContext constructor parameter to both services. Git merged the production files cleanly while three test files added by this branch no longer compiled against the new signatures. 1,257 tests pass across ConversationService, GroupConversationService, Conversation and CallerIdentity.
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| return shuttingDown; | ||
| } | ||
|
|
||
| void onShutdown(@Observes ShutdownEvent shutdownEvent) { |
There was a problem hiding this comment.
Pull request overview
This PR hardens EDDI’s runtime and lifecycle execution model around concurrency, cancellation, and shutdown semantics, and finishes wiring Dream consolidation into the cluster-aware schedule machinery. The focus is preventing “cancelled” or watchdog-abandoned work from continuing to mutate/persist state, avoiding executor starvation under load, and making shutdown behavior explicit and observable.
Changes:
- Reworked runtime/coordinator execution to avoid nested thread-pool starvation, prevent post-timeout stale persistence, and enforce one-shot completion/failure callbacks.
- Strengthened cooperative cancellation across lifecycle execution, REST streaming disconnects, group discussions, and long-term property persistence.
- Added graceful shutdown readiness + drain support and wired Dream consolidation schedules through
ScheduleFireExecutor.
Reviewed changes
Copilot reviewed 37 out of 37 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/changelog.md | Wave-3 changelog entry documenting the concurrency/lifecycle/shutdown/Dream work (includes one incorrect detail flagged in review). |
| docs/architecture.md | Updates architecture docs to describe Dream’s schedule-based wiring and credential configuration. |
| src/main/java/ai/labs/eddi/configs/agents/model/AgentConfiguration.java | Extends Dream config with parameters, deprecates call-count ceiling in favor of dollar budget. |
| src/main/java/ai/labs/eddi/engine/internal/ConversationService.java | Adds shutdown accept-gate integration, in-flight tracking for cancellation/metrics, and watchdog/abandonment-related correctness fixes. |
| src/main/java/ai/labs/eddi/engine/internal/GroupConversationService.java | Implements cooperative cancellation for member turns and multiple concurrency fixes around parallel phases/task execution waves. |
| src/main/java/ai/labs/eddi/engine/internal/RestAgentEngine.java | Tightens cancel endpoint to a single, documented graceful mode and uses imported ControlSignal. |
| src/main/java/ai/labs/eddi/engine/internal/RestAgentEngineStreaming.java | Detects SSE client disconnects via send-path checks and cancels the in-flight turn cooperatively. |
| src/main/java/ai/labs/eddi/engine/lifecycle/internal/LifecycleManager.java | Fixes component-cache keying for selective execution via absolute task indices; strengthens cancellation checks. |
| src/main/java/ai/labs/eddi/engine/runtime/BaseRuntime.java | Routes nested submissions to a virtual-thread executor; adds abandonment token + one-shot callback gating to prevent stale persistence and double-execution. |
| src/main/java/ai/labs/eddi/engine/runtime/client/workflows/WorkflowStoreClientLibrary.java | Adds clarifying documentation about component cache key invariants (absolute workflow index). |
| src/main/java/ai/labs/eddi/engine/runtime/internal/Conversation.java | Prevents cancelled turns/resumes from persisting long-term property side effects. |
| src/main/java/ai/labs/eddi/engine/runtime/internal/DreamService.java | Adds schedule entrypoint (processScheduledFire), parameterized summarization calls, privacy-safe grouping for self visibility, and improved metrics/error reporting. |
| src/main/java/ai/labs/eddi/engine/runtime/internal/GracefulShutdownService.java | Introduces shutdown observer: readiness flip + accept gate + bounded drain (one interrupt-handling bug flagged in review). |
| src/main/java/ai/labs/eddi/engine/runtime/internal/InMemoryConversationCoordinator.java | Removes retries-after-start; adds rejection rollback and drain-on-rejection behavior to avoid wedging/leaks. |
| src/main/java/ai/labs/eddi/engine/runtime/internal/NatsConversationCoordinator.java | Fixes interrupt-flag handling so timeout during drain doesn’t poison the shutdown thread. |
| src/main/java/ai/labs/eddi/engine/runtime/internal/ScheduleFireExecutor.java | Adds Dream consolidation fast-path; restores interrupt flag when latch-await is interrupted; logs Dream fire cost/error. |
| src/main/java/ai/labs/eddi/engine/runtime/internal/ShutdownReadinessHealthCheck.java | Adds readiness check that flips DOWN when shutdown is signalled. |
| src/main/java/ai/labs/eddi/modules/nlp/InputParserTask.java | Restores interrupt flag when catching InterruptedException so graceful-stop semantics propagate. |
| src/main/java/ai/labs/eddi/modules/nlp/impl/RestSemanticParser.java | Replaces unsafe/unbounded parser cache with bounded Caffeine cache + TTL; adds invalidation and cache-size visibility for tests. |
| src/main/java/ai/labs/eddi/modules/rules/impl/RulesEvaluationTask.java | Restores interrupt flag when catching InterruptedException so lifecycle interruption is not swallowed. |
| src/test/java/ai/labs/eddi/configs/agents/model/AgentConfigurationTest.java | Adds coverage for Dream parameters defaults and null-safe setter behavior. |
| src/test/java/ai/labs/eddi/engine/internal/ConversationServiceHitlCoverage2Test.java | Adds an interrupt-restore regression test ensuring interrupt restoration happens after state writes. |
| src/test/java/ai/labs/eddi/engine/internal/ConversationServiceProcessingGaugeTest.java | Adds tests for processing gauge correctness under watchdog cancel, rejected turns, and shutdown accept-gate behavior. |
| src/test/java/ai/labs/eddi/engine/internal/ConversationServiceStaleTurnTest.java | End-to-end test preventing watchdog-abandoned turns from persisting stale snapshots. |
| src/test/java/ai/labs/eddi/engine/internal/GroupConversationServiceConcurrencyTest.java | Concurrency regression suite covering cooperative cancellation, transcript snapshot handoff, turn budget enforcement, and batch timeouts. |
| src/test/java/ai/labs/eddi/engine/internal/RestAgentEngineStreamingTest.java | Tests that client disconnect cancels exactly once and does not cancel on unrelated send failures or terminal-frame timing. |
| src/test/java/ai/labs/eddi/engine/lifecycle/internal/LifecycleManagerTest.java | Adds regression coverage for absolute component-cache indices in selective execution and cancellation timing windows. |
| src/test/java/ai/labs/eddi/engine/runtime/BaseRuntimeConcurrencyTest.java | Concurrency tests for nested submission starvation avoidance, abandonment token behavior, and one-shot callback gating. |
| src/test/java/ai/labs/eddi/engine/runtime/internal/ConversationCancelPersistenceTest.java | Verifies cancelled turns do not persist long-term properties, while non-cancel stop paths still persist. |
| src/test/java/ai/labs/eddi/engine/runtime/internal/DreamServiceExtendedTest.java | Updates Dream tests for scheduled-fire path, agent-store resolution, cost ceiling, and visibility constraints. |
| src/test/java/ai/labs/eddi/engine/runtime/internal/GracefulShutdownServiceTest.java | Tests readiness flip, accept gating, bounded drain, and shutdown-event triggering behavior. |
| src/test/java/ai/labs/eddi/engine/runtime/internal/InMemoryConversationCoordinatorTest.java | Updates tests for “no retry after start” and rejection rollback/drain semantics. |
| src/test/java/ai/labs/eddi/engine/runtime/internal/ScheduleFireExecutorTest.java | Adds coverage for interrupt restoration and Dream schedule dispatch behavior. |
| src/test/java/ai/labs/eddi/modules/nlp/InputParserTaskInterruptTest.java | Ensures interrupt restoration and pipeline abort behavior when parser throws InterruptedException. |
| src/test/java/ai/labs/eddi/modules/nlp/impl/RestSemanticParserCacheTest.java | Concurrency/bounding/invalidating tests for the new Caffeine-based parser cache. |
| src/test/java/ai/labs/eddi/modules/rules/impl/RulesEvaluationTaskInterruptTest.java | Ensures interrupt restoration and pipeline abort behavior when rules evaluation throws InterruptedException. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+124
to
+126
| sleepQuietly(readinessGraceMillis); | ||
|
|
||
| long deadline = System.nanoTime() + drainTimeoutMillis * 1_000_000L; |
|
|
||
| ### Partial | ||
|
|
||
| **F6** — the REST/pipeline half is done (a `ConnectionCallback` now sets cancelled on client disconnect, and cancellation is checked at more points). The in-`modules/llm` half — cancellation checks inside the tool loop and the cascade — is deferred, since that module is owned by another workstream. |
Two Copilot findings on #619. A third — "the parameter 'shutdownEvent' is never used" from the code-quality bot — is a false positive and is deliberately NOT actioned: onShutdown(@observes ShutdownEvent) is a CDI observer, the parameter IS the subscription, and removing it would silently unregister the shutdown drain rather than tidy anything. - drain() discarded sleepQuietly's return for the readiness grace while the poll loop right below it honoured the same value, and sleepQuietly's own javadoc says the caller should stop waiting. Now honoured, and interrupt is reported separately from timeout: "timed out after 30000 ms" for a drain cut short after 50 ms sends whoever reads that log hunting the wrong problem. Being precise about the size of this: it is a DIAGNOSIS fix, not a behavioural one, and Copilot's "will still proceed to wait/drain" overstates it. Because sleepQuietly restores the interrupt flag, the old code's next Thread.sleep threw immediately and it broke out just as promptly with the same boolean. Verified by mutation, not assumed: reverting the fix leaves all 9 tests green. The two new tests are therefore labelled as pinning the interrupt CONTRACT (prompt return, honest boolean, gate stays shut) and explicitly say they do not distinguish the fix — a confident javadoc over a test that proves nothing is exactly the test theatre this review pass keeps finding. What the fix does buy: accurate logs, and an exit that no longer depends on that restored-flag side effect. - The F6 changelog entry claimed a ConnectionCallback sets cancelled on client disconnect. RestAgentEngineStreaming documents at the call site that ConnectionCallback is NOT invoked on this path and detects disconnects via SseEventSink.isClosed(). Corrected, and the correction says which approach was tried versus which shipped. Second time in this stack a changelog entry described intent rather than behaviour.
Clean textual merge. Verified beyond that, because a clean auto-merge has already broken compilation twice in this stack when main changed a constructor signature this branch's new tests were written against.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 37 out of 37 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
src/main/java/ai/labs/eddi/engine/runtime/internal/GracefulShutdownService.java:154
- If the drain is interrupted during the poll sleep,
inFlightis not re-checked before deciding success/failure. This can incorrectly returnfalse(and log “still in flight”) when the last turn finished during the sleep and the interrupt arrives immediately after. Re-checking once on the interrupt path keeps the result/logging accurate without changing the shutdown contract.
while (inFlight > 0 && System.nanoTime() < deadline) {
if (!sleepQuietly(pollIntervalMillis)) {
interrupted = true;
break;
}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 50 out of 50 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
src/main/java/ai/labs/eddi/engine/schedule/rest/RestScheduleStore.java:212
- updateSchedule() validates the body but never applies defaults before computeInitialNextFire() and persisting. If the caller omits or blanks fields like timeZone/userId (which the ownership guard currently treats as exempt), this can either throw (ZoneId.of on blank timeZone) or store a schedule that later fires with an unowned/blank userId. Apply defaults on the update path too before computing nextFire and saving.
validateSchedule(schedule);
// Recompute nextFire
computeInitialNextFire(schedule);
…on update
Copilot flagged that updateSchedule never applies defaults before persisting,
citing a blank timeZone throwing and an omitted userId being stored empty. Both
symptoms were real. One mechanism was not, and finding that out was the useful
part.
- The userId half is exactly as reported. Update skipped applyDefaults while
create ran it, so an absent userId persisted as NULL rather than
SCHEDULER_USER_ID — and since ownership treats null as unowned, an editor
updating their OWN schedule silently made it writable by every other editor.
applyDefaults now runs on update too, deliberately AFTER the ownership guards
so they judge what the caller sent rather than what defaulting produced.
- The blank-timeZone half was NOT caused by the missing applyDefaults, and my
first fix did not resolve it — the test I wrote to prove the fix kept failing,
which is the only reason I looked further instead of shipping a plausible
explanation. validateSchedule runs BEFORE defaulting on BOTH paths, and it was
validateSchedule's own ZoneId.of that threw. The three ZoneId.of call sites
disagreed about what "absent" means: one used the raw value, two null-checked
but not blank — and a blank string is non-null, so it reached ZoneId.of("").
So the defect was on CREATE as well as update, in a place the report did not
name. Replaced with a single zoneOf() accessor holding one definition of
absent (null OR blank), used by all three sites, and covered on both paths.
Mutation-checked: reverting zoneOf to a null-only check fails both tests.
I corrected the code comment and the test javadoc that had recorded the wrong
mechanism, rather than leaving a confident explanation that happened to be
false — a misleading comment survives longer than the bug.
149 tests pass across the schedule surface.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (4)
src/main/java/ai/labs/eddi/engine/internal/GroupConversationService.java (3)
2815-2818: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftPropagate cancellation to nested group members.
The cancellation token is ignored for
MemberType.GROUP; synchronousdiscuss(...)can therefore outlive a cancelled parallel batch or aborted task wave. The parent may reclaim its task after five seconds while the child discussion continues consuming resources. Track/cancel the child discussion or make nested execution cancellation-aware.As per coding guidelines, “Conversation-related code must be thread-safe and non-blocking; ... task execution must not block for extended periods.”
🤖 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/GroupConversationService.java` around lines 2815 - 2818, Update the MemberType.GROUP branch in executeGroupMemberTurn so nested executeGroupMemberTurn/discuss execution observes and propagates the parent cancellation token. Ensure cancellation interrupts or terminates the child discussion promptly rather than allowing synchronous discuss(...) to outlive the parent task or parallel batch.Source: Coding guidelines
2066-2086: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winGate failure writes with the same cancellation lock as success writes.
Cancellation can arrive after the outer
isCancelled()check but beforehandleTaskFailure(...). That handler can then fail a task reset byabortWave(...)and append a late error transcript entry. Re-check cancellation while holdingtaskList, with the sametaskList → transcriptlock order used by the success path.🤖 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/GroupConversationService.java` around lines 2066 - 2086, Update the failure handling around handleTaskFailure in the GroupDiscussionException and IllegalStateException catch blocks to re-check cancellation while holding taskList, preserving the taskList → transcript lock order used by successful writes. Skip failure handling and error recording when cancellation is observed, preventing writes after abortWave resets the task.
2070-2077: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftAbort the entire wave on quota or
ABORTfailures.A quota failure only returns from its worker, and
ABORTonly breaks that member’s loop. Other workers continue untilallOffinishes, so they can complete tasks and write results after a terminal failure. Signal the wave cancellation immediately, then runabortWave(...)to reclaim in-progress tasks before propagating the original error.🤖 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/GroupConversationService.java` around lines 2070 - 2077, Update the failure handling in the agent task execution flow around the quota check and protocol.onAgentFailure() to signal wave cancellation immediately for both QuotaExceededException and ABORT failures. Invoke abortWave(...) to reclaim in-progress tasks before propagating the original exception, and ensure sibling workers stop without completing tasks or writing results after the terminal failure.src/main/java/ai/labs/eddi/engine/runtime/internal/DreamService.java (1)
642-658: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDo not classify an unknown configured host as transient.
A bad LLM endpoint hostname raises
UnknownHostException, but this path skips the group and reports a successful cycle indefinitely. That contradicts the documented permanent-endpoint-failure behavior and prevents schedule retries/dead-lettering.Proposed fix
- if (current instanceof SocketTimeoutException || current instanceof TimeoutException - || current instanceof ConnectException || current instanceof UnknownHostException) { + if (current instanceof SocketTimeoutException || current instanceof TimeoutException + || current instanceof ConnectException) { return true; }🤖 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/DreamService.java` around lines 642 - 658, Update isTransientLlmFailure in DreamService to stop treating UnknownHostException as transient; retain transient classification for socket timeouts, general timeouts, connection failures, and matching transient messages so unknown configured hosts follow the permanent endpoint-failure path and enable retries/dead-lettering.
🧹 Nitpick comments (3)
src/main/java/ai/labs/eddi/engine/runtime/internal/DreamService.java (1)
890-893: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the mandated string-validation helper while preserving the blank check.
Replace
message == null || message.isBlank()withRuntimeUtilities.isNullOrEmpty(message) || message.isBlank();RuntimeUtilities.isNullOrEmpty(string)does not cover blank-only strings, and this path intentionally keeps that behavior for exception messages.🤖 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/DreamService.java` around lines 890 - 893, The describe(Throwable e) method must use RuntimeUtilities.isNullOrEmpty(message) for the null/empty check while retaining message.isBlank() to detect whitespace-only messages. Update only the conditional deciding between the exception class name and class-plus-message output.Source: Coding guidelines
src/main/java/ai/labs/eddi/engine/schedule/rest/RestScheduleStore.java (1)
414-499: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAdd metrics for authorization-guard rejections.
requireOwnUserIdandrequireOwnUserIdOfStoredScheduleare new authorization surfaces (403 on cross-user access, 500 on unverifiable ownership) but emit no Micrometer signal — only log lines. A counter (e.g.eddi_schedule_ownership_denied_counttagged by operation/reason) would let ops detect probing/attack patterns against this guard without grepping logs.As per coding guidelines, "Add metrics to new features using Micrometer MeterRegistry, including counters, timers, or gauges as appropriate."
🤖 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/schedule/rest/RestScheduleStore.java` around lines 414 - 499, Add Micrometer counters to the authorization rejection paths in requireOwnUserId and requireOwnUserIdOfStoredSchedule, tagging each increment with the operation and rejection reason (such as denied ownership or ownership verification failure). Reuse the class’s existing MeterRegistry/metric conventions, incrementing before returning the 403 or 500 response while leaving allowed and not-found flows unchanged.Source: Coding guidelines
src/test/java/ai/labs/eddi/engine/schedule/rest/RestScheduleStoreTest.java (1)
537-546: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueImport
jakarta.ws.rs.NotFoundExceptioninstead of using an inline FQN.The test imports other Jakarta REST types and has no conflicting
NotFoundException, so use a top-level import and keepassertThrows(NotFoundException.class, ...)consistent with the coding guideline.🤖 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/schedule/rest/RestScheduleStoreTest.java` around lines 537 - 546, Update the imports in RestScheduleStoreTest to add jakarta.ws.rs.NotFoundException, then replace the fully qualified NotFoundException reference in updateSchedule_ofMissingSchedule_stillReportsNotFound with the imported class name while leaving the 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/main/java/ai/labs/eddi/engine/internal/GroupConversationService.java`:
- Around line 2815-2818: Update the MemberType.GROUP branch in
executeGroupMemberTurn so nested executeGroupMemberTurn/discuss execution
observes and propagates the parent cancellation token. Ensure cancellation
interrupts or terminates the child discussion promptly rather than allowing
synchronous discuss(...) to outlive the parent task or parallel batch.
- Around line 2066-2086: Update the failure handling around handleTaskFailure in
the GroupDiscussionException and IllegalStateException catch blocks to re-check
cancellation while holding taskList, preserving the taskList → transcript lock
order used by successful writes. Skip failure handling and error recording when
cancellation is observed, preventing writes after abortWave resets the task.
- Around line 2070-2077: Update the failure handling in the agent task execution
flow around the quota check and protocol.onAgentFailure() to signal wave
cancellation immediately for both QuotaExceededException and ABORT failures.
Invoke abortWave(...) to reclaim in-progress tasks before propagating the
original exception, and ensure sibling workers stop without completing tasks or
writing results after the terminal failure.
In `@src/main/java/ai/labs/eddi/engine/runtime/internal/DreamService.java`:
- Around line 642-658: Update isTransientLlmFailure in DreamService to stop
treating UnknownHostException as transient; retain transient classification for
socket timeouts, general timeouts, connection failures, and matching transient
messages so unknown configured hosts follow the permanent endpoint-failure path
and enable retries/dead-lettering.
---
Nitpick comments:
In `@src/main/java/ai/labs/eddi/engine/runtime/internal/DreamService.java`:
- Around line 890-893: The describe(Throwable e) method must use
RuntimeUtilities.isNullOrEmpty(message) for the null/empty check while retaining
message.isBlank() to detect whitespace-only messages. Update only the
conditional deciding between the exception class name and class-plus-message
output.
In `@src/main/java/ai/labs/eddi/engine/schedule/rest/RestScheduleStore.java`:
- Around line 414-499: Add Micrometer counters to the authorization rejection
paths in requireOwnUserId and requireOwnUserIdOfStoredSchedule, tagging each
increment with the operation and rejection reason (such as denied ownership or
ownership verification failure). Reuse the class’s existing MeterRegistry/metric
conventions, incrementing before returning the 403 or 500 response while leaving
allowed and not-found flows unchanged.
In `@src/test/java/ai/labs/eddi/engine/schedule/rest/RestScheduleStoreTest.java`:
- Around line 537-546: Update the imports in RestScheduleStoreTest to add
jakarta.ws.rs.NotFoundException, then replace the fully qualified
NotFoundException reference in
updateSchedule_ofMissingSchedule_stillReportsNotFound with the imported class
name while leaving the test behavior unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 674733c9-c966-464c-8072-551145457ff6
📒 Files selected for processing (8)
docs/changelog.mddocs/user-memory.mdsrc/main/java/ai/labs/eddi/engine/internal/GroupConversationService.javasrc/main/java/ai/labs/eddi/engine/runtime/internal/DreamService.javasrc/main/java/ai/labs/eddi/engine/runtime/internal/NatsConversationCoordinator.javasrc/main/java/ai/labs/eddi/engine/schedule/rest/RestScheduleStore.javasrc/test/java/ai/labs/eddi/engine/internal/GroupConversationServiceConcurrencyTest.javasrc/test/java/ai/labs/eddi/engine/schedule/rest/RestScheduleStoreTest.java
💤 Files with no reviewable changes (1)
- docs/user-memory.md
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/ai/labs/eddi/engine/runtime/internal/NatsConversationCoordinator.java
…e host is permanent CodeRabbit's second review of #619. Two of its four Major findings are addressed here; two are heavy-lift behavioural changes tracked separately. - DreamService.isTransientLlmFailure classified UnknownHostException as transient, contradicting its own javadoc two lines above, which names "wrong endpoint" as the example of PERMANENT. A hostname that does not resolve is a misconfiguration: treating it as transient made every cycle skip its groups and report SUCCESS forever, so the schedule never retried, never dead-lettered, and nobody learned the endpoint was wrong. ConnectException stays transient by contrast — the host resolved and refused, which is a restarting service. - The failure paths in the task-execution wave read the cancellation token WITHOUT the task-list monitor while the success path reads it inside. My own review pass refuted this on the grounds that abortWave is cancel() THEN allOf(futures).get(...), so the sweep cannot start until every worker returned. That refutation was WRONG, and I only found out by verifying the claim I was about to write into a comment: the join is bounded by MEMBER_TURN_CANCEL_DRAIN_SECONDS and proceeds to resetStrandedInProgressTasks on TIMEOUT. The timeout is reachable — a member parked in tryResolveMemberToolPause or in a nested GROUP discuss() never observes the token. So a late failure write really can race the sweep, marking a task FAILED that the sweep just reset and appending an error entry to a finished wave. Both failure paths now take the monitor across the check AND the write. handleTaskFailure is split into recordTaskFailure (document mutation, called under the monitor) and notifyTaskFailure (SSE listener, called outside it) — holding the monitor across a client write would stall sibling workers, which is why the success path emits its event outside the lock too. Lock order stays taskList -> transcript, and recordTaskFailure never touches taskList itself, so there is no inversion. Four tests reached handleTaskFailure by reflection and so failed at RUNTIME rather than compile time; repointed at recordTaskFailure, which is the half they actually assert on. Also corrected MEMBER_TURN_CANCEL_DRAIN_SECONDS' javadoc, which claimed cancellation releases turns at their await points so the timeout is "only a safety bound". It is not: the two await points named above ignore the token, so for a turn parked there the timeout is the mechanism, not the backstop. Third docs-describe-intent finding in this stack. 578 tests pass across the group and Dream surfaces.
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 (2)
src/main/java/ai/labs/eddi/engine/internal/GroupConversationService.java (2)
2704-2709: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winUse the transcript monitor for every parallel-phase append.
The snapshot locks
gc.getTranscript(), but success, timeout, cancellation, and error entries are appended without that lock. A delayed worker can snapshot while the orchestrator writes, reintroducingConcurrentModificationException/inconsistent reads. Wrap every append in this phase with the same monitor (ideally via one helper).As per coding guidelines, “Conversation-related code must be thread-safe and non-blocking.”
Also applies to: 2764-2800
🤖 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/GroupConversationService.java` around lines 2704 - 2709, Ensure every transcript append in the parallel phase uses the monitor of gc.getTranscript(), including success, timeout, cancellation, and error paths near the orchestration logic. Centralize the synchronized append in a helper if practical, and update all relevant call sites while preserving the existing snapshot locking and non-blocking behavior.Source: Coding guidelines
3012-3024: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftCancellation releases the waiter but leaves the member execution running.
Completing
responseFutureexceptionally exits this worker, but it does not stop the already-startedconversationService.say(...). Its later callback can still callpropagateDynamicAgentTracking(...)and mutategcafter the abort sweep, terminal persistence, and ephemeral-agent cleanup—potentially losing cleanup tracking or racing the document state. Register cancellation ownership for the underlying member execution and ensure late callbacks cannot mutate the group after cancellation.As per coding guidelines, “Conversation-related code must be thread-safe and non-blocking.”
🤖 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/GroupConversationService.java` around lines 3012 - 3024, Update the member execution flow around responseFuture and conversationService.say so cancellation owns and stops the underlying member execution, not only the waiting future. Guard late completion callbacks, including propagateDynamicAgentTracking, so they cannot mutate gc or related group state after cancellation; keep the flow thread-safe and non-blocking while preserving normal completion behavior.Source: Coding guidelines
🧹 Nitpick comments (1)
src/main/java/ai/labs/eddi/engine/runtime/internal/DreamService.java (1)
175-181: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the project string-validation helper, keeping whitespace-user rejection.
Replace
agentId == null || agentId.isBlank()anduserId == null || userId.isBlank()withisNullOrEmpty(...)fromai.labs.eddi.utils.RuntimeUtilities, then still rejectuserIdwhen whitespace-only or equal toSCHEDULER_PLACEHOLDER_USER_IDper the schedule contract.🤖 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/DreamService.java` around lines 175 - 181, Update the validation in the dream scheduling flow around the agentId and userId checks to use RuntimeUtilities.isNullOrEmpty(...) for null or empty detection. Preserve the existing rejection of whitespace-only userId values and SCHEDULER_PLACEHOLDER_USER_ID, while keeping the current rejection messages and behavior for invalid agentId values.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/main/java/ai/labs/eddi/engine/internal/GroupConversationService.java`:
- Around line 2704-2709: Ensure every transcript append in the parallel phase
uses the monitor of gc.getTranscript(), including success, timeout,
cancellation, and error paths near the orchestration logic. Centralize the
synchronized append in a helper if practical, and update all relevant call sites
while preserving the existing snapshot locking and non-blocking behavior.
- Around line 3012-3024: Update the member execution flow around responseFuture
and conversationService.say so cancellation owns and stops the underlying member
execution, not only the waiting future. Guard late completion callbacks,
including propagateDynamicAgentTracking, so they cannot mutate gc or related
group state after cancellation; keep the flow thread-safe and non-blocking while
preserving normal completion behavior.
---
Nitpick comments:
In `@src/main/java/ai/labs/eddi/engine/runtime/internal/DreamService.java`:
- Around line 175-181: Update the validation in the dream scheduling flow around
the agentId and userId checks to use RuntimeUtilities.isNullOrEmpty(...) for
null or empty detection. Preserve the existing rejection of whitespace-only
userId values and SCHEDULER_PLACEHOLDER_USER_ID, while keeping the current
rejection messages and behavior for invalid agentId values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 23b6e730-d292-4e8b-9da2-94cea5b7741b
📒 Files selected for processing (5)
src/main/java/ai/labs/eddi/engine/internal/GroupConversationService.javasrc/main/java/ai/labs/eddi/engine/runtime/internal/DreamService.javasrc/test/java/ai/labs/eddi/engine/internal/GroupConversationServiceHitlCoverage3Test.javasrc/test/java/ai/labs/eddi/engine/internal/GroupConversationServiceTaskForceTest.javasrc/test/java/ai/labs/eddi/engine/runtime/internal/DreamServiceTest.java
Changelog-only conflict, both entries kept — the collision I flagged when both PRs carried a top entry. #621 merged first, so this branch absorbs it. Compiles clean; no source overlap between the two PRs.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 52 out of 52 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
src/main/java/ai/labs/eddi/engine/runtime/client/workflows/WorkflowStoreClientLibrary.java:96
- The new warn log for skipped workflow steps logs
documentDescriptor.getResource()andextensionTypewithout sanitization. These values come from stored workflow configuration and can contain untrusted data (including newlines), which risks log-forging. Consider sanitizing before logging to match the existing LogSanitizer usage elsewhere in the codebase.
if (!URI_SCHEME_ID.equals(extensionType.getScheme())) {
// Not an error — historically these were skipped silently and a
// stored config that still contains one must keep loading. But it
// disables a pipeline step, so say so instead of no-opping quietly.
LOGGER.warnf("Workflow '%s' declares step '%s', which does not use the '%s' URI scheme — "
+ "the step is skipped and will not run.",
documentDescriptor.getResource(), extensionType, URI_SCHEME_ID);
continue;
… extra lock CodeRabbit round 3, finding 1 of 2 — declined, with the reasoning recorded at the site so it stops cycling. The claim: the snapshot locks gc.getTranscript() but the success/timeout/ cancellation/error appends do not, so a delayed worker could snapshot mid-write and hit ConcurrentModificationException. Verified instead of complied with: GroupConversation guarantees the transcript is always a Collections.synchronizedList — the field initializer wraps it and setTranscript re-wraps, with no path assigning a bare list. That wrapper's mutex IS the wrapper object, which is precisely what the snapshot block locks. So add() and the snapshot already exclude one another; the explicit monitor is needed only because List.copyOf ITERATES, which the wrapper cannot make atomic. Wrapping every append would add lock scope in a hot path without removing a race. Worth being explicit that this is the OPPOSITE conclusion from the taskList guard fixed in the previous commit, where a near-identical-looking asymmetry was a real bug. The difference is what the two sides are: there, a cancellation read and a document write ordered only by the monitor; here, two operations on a single synchronized collection. Recorded in the comment so the next reviewer gets the distinction rather than the pattern match. The second finding (a late conversationService.say callback mutating gc after the abort sweep) is real and heavy-lift; tracked separately rather than redesigned as round-7 feedback on an approved PR.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 52 out of 52 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
src/main/java/ai/labs/eddi/engine/schedule/rest/RestScheduleStore.java:497
- requireOwnUserIdOfStoredSchedule() reads the schedule from the store, and updateSchedule() also calls requireAdminForHitl() which performs its own scheduleStore.readSchedule(scheduleId). On the update path this results in two store round-trips per request (plus any later reads), which is avoidable overhead.
Consider reading the stored schedule once in updateSchedule() and reusing it for BOTH the HITL guard and the stored-owner guard (e.g., by refactoring the helpers to accept a preloaded ScheduleConfiguration, or by introducing a single helper that returns the loaded schedule + any guard Response).
private Response requireOwnUserIdOfStoredSchedule(String scheduleId, String operation) {
ScheduleConfiguration stored;
try {
stored = scheduleStore.readSchedule(scheduleId);
} catch (IResourceStore.ResourceNotFoundException e) {
return null; // no schedule to protect — let the downstream op surface its 404
} catch (Exception e) {
LOGGER.error("Failed to verify schedule ownership for " + sanitize(scheduleId) + " (" + sanitize(operation) + ")", e);
return Response.status(Response.Status.INTERNAL_SERVER_ERROR)
.entity("Unable to verify schedule authorization; refusing to " + operation + " schedule.")
.build();
}
if (stored == null) {
return null;
}
return requireOwnUserId(stored.getUserId(), operation);
Copilot flagged the new skipped-workflow-step WARN, which logged documentDescriptor.getResource() and extensionType raw. Both come from stored workflow configuration, so this is the same log-forging class fixed eight times earlier in this stack — this time in code added by the review-fix pass itself. Rather than wait for a bot to find the rest one at a time, I swept the PR's own diff with a statement-aware scan (the first grep over-reported, because multi-line LOGGER calls put their arguments on continuation lines, and because counts/costs/enum names are not forging vectors). The scan separated inherited from introduced, which is what decided the scope: 79 of the flagged statements exist verbatim on main already, and 10 were introduced or reworded by this PR — all of them in DreamService. Only the latter belong in this PR. DreamService's LOGGER statements now route userId, agentId, group.getKey() and the rejection reason through LogSanitizer (19 statements in that file, being all of them rather than only the 10 newly added — the file is already substantially rewritten here, and leaving half its logs unsanitized would just invite the same finding next round). Counts, costs and durations are untouched; only arguments were rewritten, never the format strings. The 79 pre-existing sites in other files are NOT touched here. They are a real codebase-wide gap, but sanitizing them would balloon an already-approved 52-file PR at round seven of review, and they are tracked separately. 122 tests pass across the Dream and schedule surfaces.
ginccc
added a commit
that referenced
this pull request
Jul 30, 2026
…e 6.2 polish) #620 branched before three waves of security and correctness fixes landed, so this merge had 20 conflicted files / 58 hunks — and the conflicting files were precisely the ones whose current main versions ARE those fixes. Taking the wrong side anywhere would have reverted shipped security work while still compiling, and in several cases while still passing tests. Resolution was per-hunk, with both sides preserved unless they were genuinely irreconcilable. Two files show why no single rule would have worked: - RestAuditStore: both sides had a head-anchor check. #620 used `skip <= 0 && entries.size() < limit`; main uses `entries.size() == countByConversation(id)`. They are not variants — main's IS the fix for #620's, because getEntries pages NEWEST-first, so skip==0 is the most recent page rather than the start of the chain. Combining them either way provably breaks something: OR reopens the false-BROKEN regression that reported ~990 entries deleted, AND drops prefix detection when count==limit exactly. So main's anchor was taken whole, while #620's DEFAULT_VERIFY_LIMIT and its undelivered-attribution path (INCOMPLETE for gaps the ledger itself caused) were kept. - AuditLedgerService: the opposite shape. #620 refactored the queue-full check into reserveQueueSlot so a back-pressure drop can no longer burn a chain sequence and manufacture a BROKEN verdict; main had added LogSanitizer to the one log line that refactor deletes. Both applied — taking #620 alone would have silently dropped the log-injection hardening. Also resolved: an add/add collision where #618 and #620 each independently created LlmTaskStreamingDowngradeTest.java (merged into one file keeping every distinct test from both), and OutputEntry, where the two compareTo designs are contradictory by construction — main's declaration-order behaviour won, because that is what governs the order of chat bubbles the end user sees. RestAuditStoreTest auto-merged as a UNION of both sides and so got no conflict and no scrutiny — which left #620's deletedHeadEntryIsDetected asserting against the superseded heuristic without stubbing countByConversation. Mockito returned 0, the anchor never engaged, and the report came back INTACT. The test was stale, not the code; fixed by adding the stub rather than by restoring the old anchor, which is the tempting "fix" that would revert #617. Kept rather than deleted as a duplicate, because only that copy asserts tamperingSuspected(). Verification, because a green build proves very little on a merge like this: - full suite 13,312 tests — the only non-environmental failure was the audit test above, now fixed (734 tests green across every resolved area); - all 20 shipped fixes explicitly checked still present, by pattern where possible and by reading where not: the @?? jsonpath escape, the 63-byte index truncation, A2 caller-ownership on attachments, clampSkip, the whole-conversation anchor, MAX_REPORTED_MISSING, duplicates-are-BROKEN, SEQUENCE_ORIGIN, supportsSequence gating, global entries keeping their owning agent, most_accessed recency reservation, LlmTask credential isolation, cross-server MCP dedupe, delegation-depth propagation, the identity capture outside the lambda, the C11 release in a finally, the shutdown accept gate, the v6 rename's existing-empty-target handling, and OutputEntry's declaration order. One improvement came out of #620's side rather than main's: the delegation context is now handed to InputData as a mutable copy instead of the immutable Map.of, which closes the risk flagged when that fix was written.
ginccc
added a commit
that referenced
this pull request
Jul 30, 2026
…xemption
chunkStrategy has no reader — ingestion always builds a recursive splitter — so
an unsupported value is inert. Rejecting it at save time so the author hears
about it is right, but it was implemented twice, at two layers, with opposite
intentions:
- RestRagStore.prepareForWrite (create/update): normalise aliases, else 400
- RestRagStore.duplicateRag: normalise only, deliberately no rejection,
because "a copy of an existing document must not be refused just because
the rules tightened after it was stored"
- RagStore.validate (store, every write): normalise aliases, else throw
The store hook runs on create and update, so it silently overrode the duplicate
exemption one layer down, breaking two paths:
- duplicateRag refused to copy a document the same store serves via readRag
- ZIP import via RestImportService.createNewRags writes through
createResourceDirect, straight to the store with no REST layer in front,
and catches only ResourceStoreException — so an IllegalArgumentException
escaped and rolled back the ENTIRE agent import over an inert field.
UpgradeExecutor replays documents the same way.
Both layers were tested and both tests passed.
RestRagStoreWriteValidationTest asserts duplicate returns 201 but builds
RestRagStore with mock(IRagStore.class), so create was a stub and the store's
hook never ran; RagStoreValidationTest asserted the store rejects that exact
value. Neither crossed the boundary, so the contradiction was invisible.
The store now normalises but never rejects. Legacy aliases are still rewritten
to the recursive they always meant — that has to live in the store because
import bypasses REST — while an unsupported value is left verbatim so a
duplicate is a faithful copy of its original. The author-facing 400 stays at
prepareForWrite, the only layer that can tell author input from a replayed
document; same layering as RestMcpCallsStore in #619.
New RagStoreLayeringTest wires the real RagStore behind the real RestRagStore,
mocking only storage, and pins the division of labour. Mutation-checked: with
content.validate() restored it fails 3 of 6 cases with the real
IllegalArgumentException, while RestRagStoreWriteValidationTest stays green —
which is precisely why the defect survived. RagStoreValidationTest's two
rejection cases are rewritten to assert leniency, recording that their original
assertion was the defect.
This was referenced Aug 1, 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.
Stacked on #618 → #617 → #616. 37 files.
16 fixed, 1 partial. This is the wave where the obvious fix is often the wrong one, so the reasoning matters more than usual.
C1 — cancellation cancelled nothing
CompletableFuture.cancel(true)does not interrupt arunAsync/supplyAsyncbody. The JDK documentsmayInterruptIfRunningas having no effect there. Five call sites inGroupConversationServicerelied on it, so "cancelled" agent threads kept mutatinggc.getTaskList(),gc.getTranscript()and the errors list after the orchestrator had already persisted the document.Replaced with a cooperative
MemberTurnCancellationtoken checked at the agent turn's own await points, plus a bounded drain. This is also the root cause of C7 —resetStrandedInProgressTasksiterates a snapshot then mutates by id, so a falsely-"cancelled" thread could flip a task between those two steps and strand the very task the method exists to rescue.C4 — a ~100-turn scalability cliff
The inner pipeline was submitted through the same bounded pool as the outer coordinator callable, which then blocked on
future.get(). With noquarkus.thread-pool.*overrides that's the 200-thread default: at ~100 concurrent turns every thread is a waiter, no inner task can ever be scheduled, and every turn fails at the 60s watchdog. A cliff, not a gradual slowdown.Nested submissions now route to a virtual-thread executor via a
ThreadLocalmarker scoped to the callable body.Why not
CompletableFuturecomposition (the work order's first suggestion): the coordinator's ordering contract is "the callable returns ⇒ the turn is done". Making the outer non-blocking would require anIEventBus/IConversationCoordinatorSPI change and would let the next turn of the same conversation start while the previous one is still running — trading a throughput bug for a correctness bug. Virtual threads are safe here: there is not a single@RequestScopedbean insrc/main, and three existing callers (SlackEventHandler,GroupConversationService,RestGroupConversation) already drive this pipeline with no request context on virtual-thread executors. The marker clears before callbacks run, sosubmitNextstill schedules on the managed executor exactly as before, and watchdog/timeout semantics are unchanged.C9 / C13 — a completed turn could be re-executed
onCompletesat inside thetrywhosecatch (Throwable)calledonFailure— which resubmitted the already-executed callable as a retry. LLM calls, tool side effects and cost, all running twice. Compounded by C13: the coordinator retried 3× blindly, and sinceonFailurecan only be raised from inside the executor task, every retry re-ran a turn that may already have spent money.C5 — /rerun destroyed output and regenerated nothing
The component-cache key was built from a sublist-relative index while the cache stores under the absolute index. The output task then ran with
component == nulland no-op'd — after the prior output was already deleted.indexOffsetwas already threaded in and used only for HITL bookkeeping.LifecycleManagerTeststubs the component map empty — the exact condition that hides the bug. The new test populates it at absolute indices and fails if the offset is removed. This is the J4 problem (mocks that erase the defect) in its purest form.B2 — the finding's premise was inverted, and I'd flag this one
The review claimed "interrupt flags swallowed in 18 of 20 handlers". Auditing all 28 sites individually found the opposite: 14 already restore the flag correctly, 9 rethrow, and only 4 genuinely swallowed it.
The audit found two things the finding didn't:
ScheduleFireExecutor— a broadcatch (Exception)swallowingInterruptedExceptionfromlatch.await(5, MINUTES), so the poller kept firing schedules after being interrupted for shutdown. Invisible to acatch (InterruptedException)grep.NatsConversationCoordinator— the mirror bug:interrupt()called unconditionally oncatch (InterruptedException | TimeoutException), so a drain timeout left the shutdown thread flagged and would abort the@PreDestroysteps running after it.One restore is deliberately in a
finallyafter the store round trips rather than at the top of the catch — the sync Mongo driver aborts withMongoInterruptedExceptionwhen the calling thread is flagged, so an early restore would skip the veryEXECUTION_INTERRUPTEDwrite that branch exists to perform.B3 — no graceful shutdown existed at all
grep -rn ShutdownEvent src/mainmatched nothing. A rolling deploy dropped every queued and in-flight turn, with no drain and no readiness flip.I1 / G8 — Dream wired up
Per your decision, registered with
ScheduleFireExecutorrather than deleted, with the ceiling switched frommaxSummarizationCallsto the dollar-basedmaxCostPerRunthe project's own guidance prescribes.G8 matters much more now that Dream actually runs: consolidation upgraded
selfvisibility toglobalwhenever a group spanned multiple agents — and withsummarizeGroupBydefaulting tocategoryandpreserveAgentProvenancedefaulting to false, cross-agent grouping was the default path. Two agents' private memories became one entry every agent could read.Partial
F6 — REST/pipeline half done (
ConnectionCallbacksets cancelled on client disconnect; cancellation checked at more points). Themodules/llmhalf — checks inside the tool loop and cascade — is deferred to that workstream rather than reached across into.Verification
@Timeout.Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests