Feat/error handling recovery#593
Open
ginccc wants to merge 6 commits into
Open
Conversation
…ture - Shared RetryConfiguration with exponential backoff and retryable error classification - LifecycleManager: error classification, failure audit, SSE task_failed events, Micrometer counters - Admin PATCH endpoint to reset stuck conversations (DB + cache atomic update) - HTTP: error body storage for 4xx/5xx, softened JSON parsing - MCP: continueOnError, call-site retry, circuit breaker (3 failures/60s cooldown) - LLM: ResponseValidation config (onEmpty/onTruncation/onContentFilter/onRefusal/onStreamingTimeout) - Streaming: retry on zero-token failures, partial response metadata, configurable timeout - Conversation auto-recovery from EXECUTION_INTERRUPTED state - Code review fixes: JSON injection, cache invalidation, tightened string matching, LifecycleException - 83+ new tests across 7 test files (all passing)
Prevents checked exceptions from runPostResponse() from masking the original LifecycleException or defeating continueOnError when post-response processing fails during MCP call error handling.
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
Contributor
|
Warning Review limit reached
Next review available in: 19 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR adds shared retry and recovery infrastructure across LLM, MCP, API, streaming, lifecycle, and conversation-state flows. It also adds response validation, circuit breaking, structured task-failure reporting, HTTP error persistence, an admin reset endpoint, tests, and changelog entries. ChangesError Handling and Recovery Infrastructure
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant LifecycleManager
participant ConversationService
participant RestAgentEngineStreaming
participant Client
LifecycleManager->>ConversationService: onTaskFailed(taskId, taskType, durationMs, errorType, errorSummary)
ConversationService->>RestAgentEngineStreaming: streamingHandler.onTaskFailed(...)
RestAgentEngineStreaming->>Client: task_failed SSE event
sequenceDiagram
participant LlmTask
participant StreamingLegacyChatExecutor
participant StreamingModel
LlmTask->>StreamingLegacyChatExecutor: execute(model, messages, eventSink, task)
StreamingLegacyChatExecutor->>StreamingModel: stream request
StreamingModel-->>StreamingLegacyChatExecutor: partial response, completion, or error
StreamingLegacyChatExecutor-->>LlmTask: StreamingResult(response, metadata)
LlmTask->>LlmTask: applyResponseValidation(response, metadata, task)
sequenceDiagram
participant Admin
participant RestAgentEngine
participant IConversationMemoryStore
participant ConversationService
Admin->>RestAgentEngine: PATCH /{conversationId}/state?state=READY
RestAgentEngine->>IConversationMemoryStore: load conversation snapshot
RestAgentEngine->>ConversationService: resetConversationState(conversationId, READY)
ConversationService-->>Admin: HTTP response
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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}
| // Auto-recover from transient interrupted state | ||
| if (getConversationState() == ConversationState.EXECUTION_INTERRUPTED) { | ||
| LOGGER.infof("Auto-recovering conversation %s from EXECUTION_INTERRUPTED", | ||
| conversationMemory.getConversationId()); |
There was a problem hiding this comment.
Pull request overview
This PR strengthens EDDI’s resilience and observability by introducing shared retry utilities, richer error classification/reporting, MCP/LLM recovery behaviors, and an admin capability to recover stuck conversations—aimed at making pipeline execution more robust across LLM, HTTP, and MCP subsystems.
Changes:
- Added a shared
RetryConfiguration(and integrated it into LLM + MCP execution paths) plus expanded error classification/audit/SSE reporting inLifecycleManager. - Introduced LLM response validation policies and improved streaming execution metadata/timeout handling.
- Added an admin-only REST endpoint to reset stuck conversation state, and extended streaming/event interfaces to emit structured task-failure events.
Reviewed changes
Copilot reviewed 27 out of 27 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main/java/ai/labs/eddi/configs/shared/RetryConfiguration.java | New shared retry/backoff utility used across subsystems. |
| src/test/java/ai/labs/eddi/configs/shared/RetryConfigurationTest.java | Tests for shared retry/backoff behavior. |
| src/main/java/ai/labs/eddi/modules/llm/model/LlmConfiguration.java | Adds response validation + streaming timeout fields; extracts retry config to shared model while keeping compat wrapper. |
| src/test/java/ai/labs/eddi/modules/llm/model/ResponseValidationTest.java | Tests defaulting/serialization for new response-validation configuration. |
| src/main/java/ai/labs/eddi/modules/llm/impl/AgentExecutionHelper.java | Switches LLM retry implementation to shared RetryConfiguration. |
| src/main/java/ai/labs/eddi/modules/llm/impl/LegacyChatExecutor.java | Null-safety for AiMessage; adds finish-reason warnings into response metadata. |
| src/test/java/ai/labs/eddi/modules/llm/impl/LegacyChatExecutorErrorHandlingTest.java | Tests finish-reason warnings and null-safety. |
| src/main/java/ai/labs/eddi/modules/llm/impl/StreamingLegacyChatExecutor.java | Adds task-aware streaming execution with timeout/retry and metadata capture. |
| src/test/java/ai/labs/eddi/modules/llm/impl/StreamingLegacyChatExecutorRetryTest.java | Tests retry/timeout/metadata behavior for streaming legacy executor. |
| src/main/java/ai/labs/eddi/modules/llm/impl/LlmTask.java | Applies response validation policies after model execution; ingests streaming metadata. |
| src/main/java/ai/labs/eddi/modules/llm/impl/McpToolProviderManager.java | Adds circuit breaker tracking to reduce repeated MCP discovery failures. |
| src/test/java/ai/labs/eddi/modules/llm/impl/McpToolProviderManagerCircuitBreakerTest.java | Tests circuit breaker behavior for MCP tool discovery. |
| src/main/java/ai/labs/eddi/configs/mcpcalls/model/McpCall.java | Adds per-call continueOnError and optional retry config. |
| src/test/java/ai/labs/eddi/configs/mcpcalls/model/McpCallsModelsTest.java | Tests new MCP call fields and JSON round-trips. |
| src/main/java/ai/labs/eddi/modules/mcpcalls/impl/McpCallsTask.java | Adds per-call retry and continueOnError handling for MCP tool execution. |
| src/main/java/ai/labs/eddi/modules/apicalls/impl/ApiCallExecutor.java | Stores non-2xx error body/code in memory; softens JSON parsing when content-type is JSON but body isn’t. |
| src/main/java/ai/labs/eddi/engine/lifecycle/ConversationEventSink.java | Extends sink with onTaskFailed callback for structured failure monitoring. |
| src/main/java/ai/labs/eddi/engine/lifecycle/internal/LifecycleManager.java | Adds error classification + audit summary + SSE task-failure emission + metrics tags. |
| src/test/java/ai/labs/eddi/engine/lifecycle/internal/LifecycleManagerErrorClassificationTest.java | Tests classification + audit summary behavior. |
| src/main/java/ai/labs/eddi/engine/internal/ConversationService.java | Passes through new onTaskFailed streaming callback; adds resetConversationState. |
| src/main/java/ai/labs/eddi/engine/api/IConversationService.java | Adds resetConversationState API + default streaming callback. |
| src/main/java/ai/labs/eddi/engine/api/IRestAgentEngine.java | Adds admin-only PATCH /{conversationId}/state API contract. |
| src/main/java/ai/labs/eddi/engine/internal/RestAgentEngine.java | Implements admin state reset endpoint and wires in conversation memory snapshot loading. |
| src/test/java/ai/labs/eddi/engine/internal/RestAgentEngineTest.java | Updates constructor wiring for new RestAgentEngine dependency. |
| src/main/java/ai/labs/eddi/engine/internal/RestAgentEngineStreaming.java | Emits task_failed SSE events with structured error metadata. |
| src/main/java/ai/labs/eddi/engine/runtime/internal/Conversation.java | Adds automatic recovery from EXECUTION_INTERRUPTED to READY at step start. |
| docs/changelog.md | Documents the error-handling/recovery overhaul and design decisions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
src/main/java/ai/labs/eddi/modules/apicalls/impl/ApiCallExecutor.java (1)
169-175: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider appending a truncation indicator to the error body.
When the error body exceeds 2000 chars, the truncated string is stored without any marker. Downstream templates or rules inspecting the error body cannot distinguish a truncated response from a complete one. Appending a simple suffix (e.g.,
"…[truncated]") would make truncation visible to consumers.This is a minor usability improvement and can be deferred if not a priority.
♻️ Optional: add truncation indicator
- if (errorBody.length() > 2000 - ? errorBody.substring(0, 2000) - : errorBody; + String truncatedError = errorBody.length() > 2000 + ? errorBody.substring(0, 2000) + "…[truncated]" + : errorBody;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/ai/labs/eddi/modules/apicalls/impl/ApiCallExecutor.java` around lines 169 - 175, The fallback path in ApiCallExecutor around responseBody handling currently stores a truncated error body without any visible marker, so consumers can’t tell it was shortened. Update the truncation logic in this JSON-deserialization fallback flow to append a clear suffix like “…[truncated]” whenever the body is cut off, and keep the behavior localized to the responseObject assignment used after the deserialize attempt.src/main/java/ai/labs/eddi/configs/shared/RetryConfiguration.java (1)
20-58: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConfiguration POJO should use Java records per coding guidelines.
RetryConfiguration.javamatches the**/*Configuration.javapattern, and the coding guidelines state: "New feature configuration classes must use Java records for configuration POJOs." The current mutable class with getters/setters violates this guideline.Consider splitting into a
RetryConfigrecord for the configuration fields and a separate utility class (e.g.,RetryExecutor) for the static methods (executeWithRetry,backoff,isRetryableError). Jackson deserialization of records is fully supported via the canonical constructor.As per coding guidelines: "New feature configuration classes must use Java records for configuration POJOs."
♻️ Proposed refactor: split configuration and utility
// RetryConfig.java — configuration record public record RetryConfig( Integer maxAttempts, Long backoffDelayMs, Double backoffMultiplier, Long maxBackoffDelayMs ) { public RetryConfig { if (maxAttempts == null) maxAttempts = 3; if (backoffDelayMs == null) backoffDelayMs = 1000L; if (backoffMultiplier == null) backoffMultiplier = 2.0; if (maxBackoffDelayMs == null) maxBackoffDelayMs = 10000L; } } // RetryExecutor.java — static utility methods public final class RetryExecutor { public static <T> T executeWithRetry(Callable<T> action, RetryConfig config, String desc) { ... } public static void backoff(int attempt, RetryConfig config) { ... } public static boolean isRetryableError(Exception e) { ... } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/ai/labs/eddi/configs/shared/RetryConfiguration.java` around lines 20 - 58, The RetryConfiguration POJO should be converted to a Java record to match the configuration coding guideline, since the current mutable class with getters/setters violates the record requirement. Replace the mutable configuration fields in RetryConfiguration with an immutable record form (or rename to RetryConfig) and keep only the config data there, while moving the retry logic methods such as executeWithRetry, backoff, and isRetryableError into a separate utility class like RetryExecutor. Make sure any existing uses of RetryConfiguration are updated to the new record type and that default values remain preserved through the record canonical constructor.Source: Coding guidelines
src/main/java/ai/labs/eddi/modules/llm/impl/LlmTask.java (1)
570-667: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNew Response Validation feature has no Micrometer metrics.
The validation actions (
warn/fallback/errorper signal) are a good candidate for counters (e.g., validation triggers by type/action), useful for tracking how often responses are being modified or rejected in production.As per path instructions, "Use Micrometer
MeterRegistrymetrics for new features, initializing counters/timers in@PostConstruct."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/ai/labs/eddi/modules/llm/impl/LlmTask.java` around lines 570 - 667, The new response validation flow in applyResponseValidation/applyValidationAction has no Micrometer instrumentation yet. Add MeterRegistry-based counters for each validation signal/action pair (for example, empty/truncated/content_filter/streaming_timeout/refusal and warn/fallback/error) so triggers are tracked in production. Initialize the meters in a `@PostConstruct` method on LlmTask, wire in MeterRegistry via the class constructor or injection, and increment the appropriate counter inside applyValidationAction before storing data, returning fallback content, or throwing LifecycleException.Source: Path instructions
🤖 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/modules/llm/impl/StreamingLegacyChatExecutor.java`:
- Around line 90-96: The retry flow in StreamingLegacyChatExecutor is blocking
the caller by waiting on CountDownLatch and sleeping in
RetryConfiguration.backoff across multiple attempts. Refactor the attempt loop
and related timeout/error handling in the streaming execution path to run
asynchronously so the calling thread is not held for long periods; use the
existing StreamingLegacyChatExecutor methods and retry/backoff logic to complete
work off-thread and signal results without blocking, and update the
retry-related blocks around the latch await and backoff calls accordingly.
- Around line 95-186: The retry loop in StreamingLegacyChatExecutor leaves
timed-out or errored streaming attempts alive, so stale tokens can keep reaching
eventSink during the next attempt. Update the streaming flow around
streamingModel.chat and the StreamingChatResponseHandler callbacks to use a
cancelable context/handle or an active-attempt guard, and ensure the previous
attempt is cancelled or ignored before any continue to the next retry.
In `@src/main/java/ai/labs/eddi/modules/llm/model/LlmConfiguration.java`:
- Around line 671-759: The new ResponseValidation configuration in
LlmConfiguration should be converted from a mutable bean into a Java record to
match the configuration POJO guideline. Update the ResponseValidation type
itself (including its default values and accessors), then adjust any Jackson
binding or deserialization assumptions and update ResponseValidationTest to
construct and verify the record instead of using setters. Keep the existing
field names and semantics so callers can still configure enabled, onEmpty,
onTruncation, onContentFilter, onRefusal, and onStreamingTimeout.
In
`@src/test/java/ai/labs/eddi/modules/llm/impl/McpToolProviderManagerCircuitBreakerTest.java`:
- Around line 122-138: Strengthen the circuit-breaker test in
circuitClosed_attemptsConnection by asserting a recorded failure after
discoverTools(List.of(config)) runs, not just a non-null result. Use the
McpToolProviderManager instance to verify the circuit state or failure count
changed as expected so the test proves the connection attempt actually happened
instead of being skipped by an always-open breaker.
---
Nitpick comments:
In `@src/main/java/ai/labs/eddi/configs/shared/RetryConfiguration.java`:
- Around line 20-58: The RetryConfiguration POJO should be converted to a Java
record to match the configuration coding guideline, since the current mutable
class with getters/setters violates the record requirement. Replace the mutable
configuration fields in RetryConfiguration with an immutable record form (or
rename to RetryConfig) and keep only the config data there, while moving the
retry logic methods such as executeWithRetry, backoff, and isRetryableError into
a separate utility class like RetryExecutor. Make sure any existing uses of
RetryConfiguration are updated to the new record type and that default values
remain preserved through the record canonical constructor.
In `@src/main/java/ai/labs/eddi/modules/apicalls/impl/ApiCallExecutor.java`:
- Around line 169-175: The fallback path in ApiCallExecutor around responseBody
handling currently stores a truncated error body without any visible marker, so
consumers can’t tell it was shortened. Update the truncation logic in this
JSON-deserialization fallback flow to append a clear suffix like “…[truncated]”
whenever the body is cut off, and keep the behavior localized to the
responseObject assignment used after the deserialize attempt.
In `@src/main/java/ai/labs/eddi/modules/llm/impl/LlmTask.java`:
- Around line 570-667: The new response validation flow in
applyResponseValidation/applyValidationAction has no Micrometer instrumentation
yet. Add MeterRegistry-based counters for each validation signal/action pair
(for example, empty/truncated/content_filter/streaming_timeout/refusal and
warn/fallback/error) so triggers are tracked in production. Initialize the
meters in a `@PostConstruct` method on LlmTask, wire in MeterRegistry via the
class constructor or injection, and increment the appropriate counter inside
applyValidationAction before storing data, returning fallback content, or
throwing LifecycleException.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c600bc68-357b-4451-8fd5-1dc75f137356
📒 Files selected for processing (27)
docs/changelog.mdsrc/main/java/ai/labs/eddi/configs/mcpcalls/model/McpCall.javasrc/main/java/ai/labs/eddi/configs/shared/RetryConfiguration.javasrc/main/java/ai/labs/eddi/engine/api/IConversationService.javasrc/main/java/ai/labs/eddi/engine/api/IRestAgentEngine.javasrc/main/java/ai/labs/eddi/engine/internal/ConversationService.javasrc/main/java/ai/labs/eddi/engine/internal/RestAgentEngine.javasrc/main/java/ai/labs/eddi/engine/internal/RestAgentEngineStreaming.javasrc/main/java/ai/labs/eddi/engine/lifecycle/ConversationEventSink.javasrc/main/java/ai/labs/eddi/engine/lifecycle/internal/LifecycleManager.javasrc/main/java/ai/labs/eddi/engine/runtime/internal/Conversation.javasrc/main/java/ai/labs/eddi/modules/apicalls/impl/ApiCallExecutor.javasrc/main/java/ai/labs/eddi/modules/llm/impl/AgentExecutionHelper.javasrc/main/java/ai/labs/eddi/modules/llm/impl/LegacyChatExecutor.javasrc/main/java/ai/labs/eddi/modules/llm/impl/LlmTask.javasrc/main/java/ai/labs/eddi/modules/llm/impl/McpToolProviderManager.javasrc/main/java/ai/labs/eddi/modules/llm/impl/StreamingLegacyChatExecutor.javasrc/main/java/ai/labs/eddi/modules/llm/model/LlmConfiguration.javasrc/main/java/ai/labs/eddi/modules/mcpcalls/impl/McpCallsTask.javasrc/test/java/ai/labs/eddi/configs/mcpcalls/model/McpCallsModelsTest.javasrc/test/java/ai/labs/eddi/configs/shared/RetryConfigurationTest.javasrc/test/java/ai/labs/eddi/engine/internal/RestAgentEngineTest.javasrc/test/java/ai/labs/eddi/engine/lifecycle/internal/LifecycleManagerErrorClassificationTest.javasrc/test/java/ai/labs/eddi/modules/llm/impl/LegacyChatExecutorErrorHandlingTest.javasrc/test/java/ai/labs/eddi/modules/llm/impl/McpToolProviderManagerCircuitBreakerTest.javasrc/test/java/ai/labs/eddi/modules/llm/impl/StreamingLegacyChatExecutorRetryTest.javasrc/test/java/ai/labs/eddi/modules/llm/model/ResponseValidationTest.java
…ration The nested LlmConfiguration.RetryConfiguration was an empty subclass of the extracted ai.labs.eddi.configs.shared.RetryConfiguration, added as a backward-compat shim. It overrode nothing and shadowed the imported shared type within LlmConfiguration's body, triggering the "class has same name as super class" code-quality finding on PR #593. Delete the shim and point the retry field/getter/setter plus all tests at the shared class directly. The subclass added no fields, so the retry JSON structure is byte-for-byte identical and existing stored configs deserialize unchanged. Verified with a clean test-compile and 123 passing unit tests.
Integrates 180 commits from main — the HITL (human-in-the-loop) framework, multimodal-attachments completion, and related work — into the holistic error-handling/recovery branch. Conflicts resolved as unions preserving BOTH feature sets: - IConversationService / IRestAgentEngine: kept error-handling's resetConversationState + PATCH /state endpoint alongside main's HITL endConversation(id, endedBy), cancel/resume/approval endpoints, and onSkipped. - AgentExecutionHelper: kept HEAD's one-liner delegating to the shared RetryConfiguration.executeWithRetry; main's inline retry loop (which referenced the now-removed nested RetryConfiguration and duplicated the shared util) was dropped. Its HITL ToolApprovalRequiredException pass-through was relocated into the shared RetryConfiguration so both LLM and MCP retries still let a tool-approval pause propagate intact. - LlmConfiguration / LlmTask: unioned response-validation (HEAD) with multimodal + tool-approval config and the HITL resume path (main). - RestAgentEngine constructor: unioned to take conversationMemoryStore (HEAD) plus hitlAccessGuard/hitlToolJournalStore (main); updated all affected tests. - Five of main's test files updated for the removed nested RetryConfiguration and the new constructor arity. Verified: clean compile (main + tests); 437 affected unit tests pass; adversarial multi-agent review of all 8 conflict resolutions plus the 3 silent auto-merges (Conversation, ConversationService, LifecycleManager) found no dropped or corrupted code.
Contributor
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 (4)
src/main/java/ai/labs/eddi/engine/lifecycle/internal/LifecycleManager.java (4)
379-380: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winLog SSE callback failures with conversation context.
The current message drops the throwable, conversation ID, and task ID, making callback failures difficult to diagnose.
As per coding guidelines, JBoss Logger entries must include conversation context and use appropriate log levels.
🤖 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/lifecycle/internal/LifecycleManager.java` around lines 379 - 380, Update the SSE callback failure handler around the catch block in LifecycleManager to log the full throwable rather than only its message, and include the conversation ID and task ID in the structured log context. Use the appropriate warning/error level for a callback failure while preserving the existing exception handling flow.Source: Coding guidelines
384-400: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDo not let audit collection bypass strict-write recovery.
If
auditCollector.collect()throws, Lines 403-408 never run, partial task writes remain, and the audit exception masks the original failure. Apply strict-write recovery first, then shield audit collection while attaching any reporting failure as suppressed.As per coding guidelines, recoverable exceptions must not kill the pipeline.
🤖 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/lifecycle/internal/LifecycleManager.java` around lines 384 - 400, The failure path must execute strict-write recovery before attempting audit reporting. In the block around the failure AuditEntry and auditCollector.collect, move or invoke the existing recovery logic first, then wrap auditCollector.collect in guarded recoverable-exception handling; attach any audit exception as suppressed to the original task failure without replacing it, and ensure the pipeline continues through the existing recovery flow.Source: Coding guidelines
834-840: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftRedact exception messages before persisting or streaming them.
Truncation does not make arbitrary exception text safe. This value is written to audit storage and emitted through
task_failedSSE, potentially exposing URLs, payloads, credentials, or PII. Apply a shared redactor before returning it.🤖 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/lifecycle/internal/LifecycleManager.java` around lines 834 - 840, Update summarizeForAudit(Throwable e) to pass the selected exception message or class-name fallback through the project’s shared redactor before truncation and return. Preserve the existing 500-character limit, and ensure the redacted value is used for both audit persistence and task_failed SSE output.
802-823: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winGive typed inner causes precedence over wrapper messages.
Each outer message is checked before its cause. For example, a
"429"wrapper aroundSocketTimeoutExceptionis classified asrate_limit, nottimeout. Scan the entire chain for typed causes before applying message heuristics.🤖 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/lifecycle/internal/LifecycleManager.java` around lines 802 - 823, Update classifyError to scan the full Throwable cause chain for typed timeout or transport exceptions before evaluating any error-message heuristics. Preserve the existing message classifications for rate_limit and content_filter, but only apply them after no higher-priority typed cause is found.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/java/ai/labs/eddi/engine/lifecycle/internal/LifecycleManager.java`:
- Around line 534-538: Update getLifecycleStartIndex to guard both nullable
values before prefix matching: skip lifecycle tasks whose task.getType() is null
and ignore null entries in lifecycleTaskTypes. Preserve the existing
startsWith-based matching and return the first matching task index.
---
Outside diff comments:
In `@src/main/java/ai/labs/eddi/engine/lifecycle/internal/LifecycleManager.java`:
- Around line 379-380: Update the SSE callback failure handler around the catch
block in LifecycleManager to log the full throwable rather than only its
message, and include the conversation ID and task ID in the structured log
context. Use the appropriate warning/error level for a callback failure while
preserving the existing exception handling flow.
- Around line 384-400: The failure path must execute strict-write recovery
before attempting audit reporting. In the block around the failure AuditEntry
and auditCollector.collect, move or invoke the existing recovery logic first,
then wrap auditCollector.collect in guarded recoverable-exception handling;
attach any audit exception as suppressed to the original task failure without
replacing it, and ensure the pipeline continues through the existing recovery
flow.
- Around line 834-840: Update summarizeForAudit(Throwable e) to pass the
selected exception message or class-name fallback through the project’s shared
redactor before truncation and return. Preserve the existing 500-character
limit, and ensure the redacted value is used for both audit persistence and
task_failed SSE output.
- Around line 802-823: Update classifyError to scan the full Throwable cause
chain for typed timeout or transport exceptions before evaluating any
error-message heuristics. Preserve the existing message classifications for
rate_limit and content_filter, but only apply them after no higher-priority
typed cause is found.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: eaa7c192-5fbf-473e-9247-344608980c19
📒 Files selected for processing (14)
docs/changelog.mdsrc/main/java/ai/labs/eddi/configs/shared/RetryConfiguration.javasrc/main/java/ai/labs/eddi/engine/api/IConversationService.javasrc/main/java/ai/labs/eddi/engine/api/IRestAgentEngine.javasrc/main/java/ai/labs/eddi/engine/internal/ConversationService.javasrc/main/java/ai/labs/eddi/engine/internal/RestAgentEngine.javasrc/main/java/ai/labs/eddi/engine/lifecycle/internal/LifecycleManager.javasrc/main/java/ai/labs/eddi/engine/runtime/internal/Conversation.javasrc/main/java/ai/labs/eddi/modules/llm/impl/LlmTask.javasrc/main/java/ai/labs/eddi/modules/llm/model/LlmConfiguration.javasrc/test/java/ai/labs/eddi/engine/internal/RestAgentEngineHitlTest.javasrc/test/java/ai/labs/eddi/engine/internal/RestAgentEngineTest.javasrc/test/java/ai/labs/eddi/engine/internal/RestAgentEngineToolPauseDetailsTest.javasrc/test/java/ai/labs/eddi/modules/llm/impl/AgentOrchestratorToolPauseTest.java
🚧 Files skipped from review as they are similar to previous changes (10)
- src/test/java/ai/labs/eddi/engine/internal/RestAgentEngineTest.java
- src/main/java/ai/labs/eddi/engine/runtime/internal/Conversation.java
- src/main/java/ai/labs/eddi/engine/api/IRestAgentEngine.java
- src/main/java/ai/labs/eddi/engine/internal/ConversationService.java
- src/main/java/ai/labs/eddi/engine/api/IConversationService.java
- src/main/java/ai/labs/eddi/modules/llm/impl/LlmTask.java
- docs/changelog.md
- src/main/java/ai/labs/eddi/engine/internal/RestAgentEngine.java
- src/main/java/ai/labs/eddi/modules/llm/model/LlmConfiguration.java
- src/main/java/ai/labs/eddi/configs/shared/RetryConfiguration.java
Contributor
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
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 (4)
src/main/java/ai/labs/eddi/engine/lifecycle/internal/LifecycleManager.java (4)
379-380: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winLog SSE callback failures with conversation context.
The current message drops the throwable, conversation ID, and task ID, making callback failures difficult to diagnose.
As per coding guidelines, JBoss Logger entries must include conversation context and use appropriate log levels.
🤖 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/lifecycle/internal/LifecycleManager.java` around lines 379 - 380, Update the SSE callback failure handler around the catch block in LifecycleManager to log the full throwable rather than only its message, and include the conversation ID and task ID in the structured log context. Use the appropriate warning/error level for a callback failure while preserving the existing exception handling flow.Source: Coding guidelines
384-400: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDo not let audit collection bypass strict-write recovery.
If
auditCollector.collect()throws, Lines 403-408 never run, partial task writes remain, and the audit exception masks the original failure. Apply strict-write recovery first, then shield audit collection while attaching any reporting failure as suppressed.As per coding guidelines, recoverable exceptions must not kill the pipeline.
🤖 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/lifecycle/internal/LifecycleManager.java` around lines 384 - 400, The failure path must execute strict-write recovery before attempting audit reporting. In the block around the failure AuditEntry and auditCollector.collect, move or invoke the existing recovery logic first, then wrap auditCollector.collect in guarded recoverable-exception handling; attach any audit exception as suppressed to the original task failure without replacing it, and ensure the pipeline continues through the existing recovery flow.Source: Coding guidelines
834-840: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftRedact exception messages before persisting or streaming them.
Truncation does not make arbitrary exception text safe. This value is written to audit storage and emitted through
task_failedSSE, potentially exposing URLs, payloads, credentials, or PII. Apply a shared redactor before returning it.🤖 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/lifecycle/internal/LifecycleManager.java` around lines 834 - 840, Update summarizeForAudit(Throwable e) to pass the selected exception message or class-name fallback through the project’s shared redactor before truncation and return. Preserve the existing 500-character limit, and ensure the redacted value is used for both audit persistence and task_failed SSE output.
802-823: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winGive typed inner causes precedence over wrapper messages.
Each outer message is checked before its cause. For example, a
"429"wrapper aroundSocketTimeoutExceptionis classified asrate_limit, nottimeout. Scan the entire chain for typed causes before applying message heuristics.🤖 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/lifecycle/internal/LifecycleManager.java` around lines 802 - 823, Update classifyError to scan the full Throwable cause chain for typed timeout or transport exceptions before evaluating any error-message heuristics. Preserve the existing message classifications for rate_limit and content_filter, but only apply them after no higher-priority typed cause is found.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/java/ai/labs/eddi/engine/lifecycle/internal/LifecycleManager.java`:
- Around line 534-538: Update getLifecycleStartIndex to guard both nullable
values before prefix matching: skip lifecycle tasks whose task.getType() is null
and ignore null entries in lifecycleTaskTypes. Preserve the existing
startsWith-based matching and return the first matching task index.
---
Outside diff comments:
In `@src/main/java/ai/labs/eddi/engine/lifecycle/internal/LifecycleManager.java`:
- Around line 379-380: Update the SSE callback failure handler around the catch
block in LifecycleManager to log the full throwable rather than only its
message, and include the conversation ID and task ID in the structured log
context. Use the appropriate warning/error level for a callback failure while
preserving the existing exception handling flow.
- Around line 384-400: The failure path must execute strict-write recovery
before attempting audit reporting. In the block around the failure AuditEntry
and auditCollector.collect, move or invoke the existing recovery logic first,
then wrap auditCollector.collect in guarded recoverable-exception handling;
attach any audit exception as suppressed to the original task failure without
replacing it, and ensure the pipeline continues through the existing recovery
flow.
- Around line 834-840: Update summarizeForAudit(Throwable e) to pass the
selected exception message or class-name fallback through the project’s shared
redactor before truncation and return. Preserve the existing 500-character
limit, and ensure the redacted value is used for both audit persistence and
task_failed SSE output.
- Around line 802-823: Update classifyError to scan the full Throwable cause
chain for typed timeout or transport exceptions before evaluating any
error-message heuristics. Preserve the existing message classifications for
rate_limit and content_filter, but only apply them after no higher-priority
typed cause is found.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: eaa7c192-5fbf-473e-9247-344608980c19
📒 Files selected for processing (14)
docs/changelog.mdsrc/main/java/ai/labs/eddi/configs/shared/RetryConfiguration.javasrc/main/java/ai/labs/eddi/engine/api/IConversationService.javasrc/main/java/ai/labs/eddi/engine/api/IRestAgentEngine.javasrc/main/java/ai/labs/eddi/engine/internal/ConversationService.javasrc/main/java/ai/labs/eddi/engine/internal/RestAgentEngine.javasrc/main/java/ai/labs/eddi/engine/lifecycle/internal/LifecycleManager.javasrc/main/java/ai/labs/eddi/engine/runtime/internal/Conversation.javasrc/main/java/ai/labs/eddi/modules/llm/impl/LlmTask.javasrc/main/java/ai/labs/eddi/modules/llm/model/LlmConfiguration.javasrc/test/java/ai/labs/eddi/engine/internal/RestAgentEngineHitlTest.javasrc/test/java/ai/labs/eddi/engine/internal/RestAgentEngineTest.javasrc/test/java/ai/labs/eddi/engine/internal/RestAgentEngineToolPauseDetailsTest.javasrc/test/java/ai/labs/eddi/modules/llm/impl/AgentOrchestratorToolPauseTest.java
🚧 Files skipped from review as they are similar to previous changes (10)
- src/test/java/ai/labs/eddi/engine/internal/RestAgentEngineTest.java
- src/main/java/ai/labs/eddi/engine/runtime/internal/Conversation.java
- src/main/java/ai/labs/eddi/engine/api/IRestAgentEngine.java
- src/main/java/ai/labs/eddi/engine/internal/ConversationService.java
- src/main/java/ai/labs/eddi/engine/api/IConversationService.java
- src/main/java/ai/labs/eddi/modules/llm/impl/LlmTask.java
- docs/changelog.md
- src/main/java/ai/labs/eddi/engine/internal/RestAgentEngine.java
- src/main/java/ai/labs/eddi/modules/llm/model/LlmConfiguration.java
- src/main/java/ai/labs/eddi/configs/shared/RetryConfiguration.java
🛑 Comments failed to post (1)
src/main/java/ai/labs/eddi/engine/lifecycle/internal/LifecycleManager.java (1)
534-538: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Guard nullable task types before prefix matching.
Elsewhere
task.getType()is explicitly treated as nullable, but this dereference crashes selective execution. Null-filter requested types as well.Proposed fix
ILifecycleTask task = this.lifecycleTasks.get(i); -if (lifecycleTaskTypes.stream().anyMatch(type -> task.getType().startsWith(type))) { +String taskType = task.getType(); +if (taskType != null && lifecycleTaskTypes.stream() + .filter(Objects::nonNull) + .anyMatch(taskType::startsWith)) { return i; }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.private int getLifecycleStartIndex(List<String> lifecycleTaskTypes) { for (int i = 0; i < this.lifecycleTasks.size(); i++) { ILifecycleTask task = this.lifecycleTasks.get(i); String taskType = task.getType(); if (taskType != null && lifecycleTaskTypes.stream() .filter(Objects::nonNull) .anyMatch(taskType::startsWith)) { return i;🤖 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/lifecycle/internal/LifecycleManager.java` around lines 534 - 538, Update getLifecycleStartIndex to guard both nullable values before prefix matching: skip lifecycle tasks whose task.getType() is null and ignore null entries in lifecycleTaskTypes. Preserve the existing startsWith-based matching and return the first matching task index.
Both failures pre-date the origin/main merge — CI was already red on c054b43 ("Tests run: 9776, Failures: 1, Errors: 1") with these exact two tests. Each asserts a contract this PR deliberately superseded, so the fix is in the tests; no production behavior changes. StreamingLegacyChatExecutorTest.execute_error_throwsRuntimeException asserted that a streaming error always throws. The PR intentionally changed this: an error after partial tokens returns the partial text with a streaming_error_partial warning, and only a zero-content error throws. Retarget the test at the zero-content case (matching its name) and add execute_errorAfterPartial_returnsPartialContent for the partial contract, keeping the original token-forwarding assertion. ConversationExtendedTest.saySucceeds stubbed getConversationState() with a call-count-sensitive consecutive-return sequence (READY, then IN_PROGRESS). The PR's EXECUTION_INTERRUPTED auto-recovery added a state read at the top of runStep, which consumed the READY, so the in-progress guard saw IN_PROGRESS and threw. The mock now tracks state like real memory, making it robust to how often production reads it.
Four findings on this PR's own failure path, all verified against the code. Each fix reuses existing infrastructure rather than adding a new utility. Audit must not bypass strict-write recovery (Major): if auditCollector.collect() threw, the strict-write rollback was skipped — leaving the partial task writes it exists to remove — and the audit exception propagated out of the catch, replacing the original task failure. Strict-write recovery now runs first, and audit collection is shielded, attaching any reporting error to the original exception via addSuppressed instead of masking it. Redact credentials from audit/SSE summaries (Major): summarizeForAudit() only truncated, yet its output is persisted to the audit ledger and streamed to admins over task_failed SSE. It now applies SecretRedactionFilter.redact() before truncating — cutting first can split a secret so the pattern no longer matches. URLs and class names stay: the audience is privileged and needs them. Typed causes outrank wrapper messages (Minor): classifyError() checked each level's message before descending, so a "429" wrapper around a SocketTimeoutException classified as rate_limit. It now scans the chain for typed causes first, falling back to message heuristics only when none match. SSE failure logging (Minor): the task_failed emission catch logged at DEBUG and dropped the throwable and all context. Now WARN with the throwable, sanitized conversation id (LogSanitizer, CWE-117) and task id. Adds four regression tests, each failing under the previous behavior.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This pull request delivers a comprehensive overhaul of error handling and recovery infrastructure across the EDDI platform. The changes introduce a shared, configurable retry mechanism, robust error classification and reporting, enhanced admin recovery tools, and improved monitoring hooks. These updates span multiple subsystems (LLM, HTTP, MCP), enabling more resilient and observable operations, and providing administrators with better tools to recover from failures.
Error Handling & Retry Infrastructure
RetryConfigurationclass for consistent, exponential-backoff retry logic across all subsystems (LLM, MCP, etc.), including static utility methods for executing actions with retries and detecting retryable errors.McpCallmodel, includingcontinueOnErrorand a configurableretryfield. This allows MCP calls to be retried on transient failures and to optionally not abort the pipeline on error. [1] [2] [3]Admin Recovery & State Management
PATCH /{conversationId}/stateto reset the state of stuck conversations (ERROR or EXECUTION_INTERRUPTED) back to READY, with appropriate validation and error handling. [1] [2] [3] [4] [5]IConversationServiceinterface and implementation to support atomic conversation state resets, affecting both persistent and in-memory state. [1] [2]Monitoring & Observability
ConversationEventSink, streaming handlers) to include structuredonTaskFailedcallbacks, enabling real-time admin monitoring of failed tasks with detailed error metadata. [1] [2] [3] [4]Documentation
Type of Change
Checklist
./mvnw clean verify -DskipITs)Summary by CodeRabbit
task_failedevents witherrorTypeanderrorSummary.continueOnErrorand per-call retry configuration.