Implement retry, backoff#5
Conversation
…unavailable. In this commit Test is failing due to LLMException is thrown
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR integrates Resilience4j circuit breaker and retry into the ChatService LLM integration, renames and reconfigures the RestClient bean, adds new exception types and handlers, updates POM dependencies (Spring Cloud BOM, AspectJ, WireMock, circuit-breaker starter), adds Resilience4j properties and OpenRouter base URL, updates controller wiring, and adds WireMock-based integration tests. ChangesResilience4j Circuit Breaker & Retry Integration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/org/example/projectbifrost/service/ChatService.java (1)
47-65:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy lift
@CircuitBreakerand@Retryare completely bypassed in the production path — self-invocation skips the Spring AOP proxy.
chatWithLLMcallsfetchResponseFromLLM(apiMessages)onthis, the raw bean instance. Spring's proxy-based AOP only intercepts calls made through the proxy object, not through directthisreferences. The result is that every call entering via the controller goes throughchatWithLLM→this.fetchResponseFromLLM(...), and no retry or circuit breaker logic ever fires.The test passes because
ChatServiceTestautowires the proxy and callsfetchResponseFromLLMdirectly on it, which does go through the AOP machinery. But that test path is not the production path.The cleanest fix is to extract
fetchResponseFromLLMinto a separate@Servicebean and inject it intoChatService:🐛 Proposed fix – extract to a dedicated bean
New bean (e.g.
LLMClient.java):`@Service` public class LLMClient { private final RestClient restClient; `@Value`("${openrouter.model}") private String model; public LLMClient(RestClient restClient) { this.restClient = restClient; } `@CircuitBreaker`(name = "chatService", fallbackMethod = "fallback") `@Retry`(name = "chatService") public String fetchResponseFromLLM(List<OpenRouterRequestDTO.Message> apiMessages) { // ... same body ... } public String fallback(List<OpenRouterRequestDTO.Message> apiMessages, Exception e) { log.warn("LLM unavailable, returning fallback", e); return "Fallback!"; } }
ChatService– injectLLMClientand delegate:- String content = fetchResponseFromLLM(apiMessages); + String content = llmClient.fetchResponseFromLLM(apiMessages);Alternatively, if a separate class is undesirable, inject a
selfreference via@Lazy@Autowiredprivate ChatService selfand callself.fetchResponseFromLLM(...).🤖 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/org/example/projectbifrost/service/ChatService.java` around lines 47 - 65, chatWithLLM is calling fetchResponseFromLLM on the same ChatService instance, which bypasses Spring AOP proxies so `@CircuitBreaker` and `@Retry` never run; extract fetchResponseFromLLM into a separate Spring bean (e.g., LLMClient with a public fetchResponseFromLLM(List<OpenRouterRequestDTO.Message>) method annotated with `@CircuitBreaker` and `@Retry` and a fallback method) and inject that bean into ChatService, then have chatWithLLM delegate to the injected LLMClient.fetchResponseFromLLM(...) (alternatively inject a proxied self reference with `@Lazy` to call self.fetchResponseFromLLM(...)).
🧹 Nitpick comments (2)
pom.xml (1)
115-118: ⚡ Quick winConsider using the non-reactive circuit breaker starter for this synchronous service.
ChatService.fetchResponseFromLLMreturns a plainStringvia a synchronousRestClient. The Spring Cloud docs recommendspring-cloud-starter-circuitbreaker-resilience4jfor non-reactive applications, andspring-cloud-starter-circuitbreaker-reactor-resilience4jfor reactive applications. The Reactor variant adds aReactiveResilience4JCircuitBreakerFactorybean and pulls in WebFlux-related circuit breaker infrastructure that is unused here.♻️ Proposed change
<dependency> <groupId>org.springframework.cloud</groupId> - <artifactId>spring-cloud-starter-circuitbreaker-reactor-resilience4j</artifactId> + <artifactId>spring-cloud-starter-circuitbreaker-resilience4j</artifactId> </dependency>🤖 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 `@pom.xml` around lines 115 - 118, The POM currently pulls the reactive circuit-breaker starter (artifactId spring-cloud-starter-circuitbreaker-reactor-resilience4j) while this service is synchronous; change the dependency to the non-reactive starter (artifactId spring-cloud-starter-circuitbreaker-resilience4j) so the app doesn't register Reactor/WebFlux circuit-breaker infrastructure that ChatService.fetchResponseFromLLM (synchronous RestClient) doesn't use; update the dependency entry in pom.xml accordingly and run a build to ensure no unused reactive classes remain referenced.src/main/java/org/example/projectbifrost/service/ChatService.java (1)
92-94: ⚡ Quick winFallback silently swallows failures — no logging makes production incidents invisible.
When the circuit is open or all retries are exhausted, the caller receives the generic string
"Fallback!"with no trace in the logs. Add at minimum alog.warnorlog.errorwith the exception so that repeated fallback activations are observable.⚙️ Proposed fix
+ private static final Logger log = LoggerFactory.getLogger(ChatService.class); + // (or use `@Slf4j` from Lombok) + public String fallback(List<OpenRouterRequestDTO.Message> apiMessages, Exception e) { + log.error("LLM call failed after retries/circuit open, returning fallback. Cause: {}", e.getMessage(), e); return "Fallback!"; }🤖 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/org/example/projectbifrost/service/ChatService.java` around lines 92 - 94, The fallback method currently returns a generic string and swallows the exception; update the ChatService.fallback(List<OpenRouterRequestDTO.Message> apiMessages, Exception e) to log the failure (use log.warn or log.error) and include the exception as the throwable parameter plus brief context (e.g., apiMessages size or a short summary) so repeated fallbacks are observable in logs; keep the existing return value or make it slightly more descriptive if desired, but do not remove the exception logging.
🤖 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 `@pom.xml`:
- Around line 101-105: The WireMock dependency (groupId
org.wiremock.integrations, artifactId wiremock-spring-boot, version 4.0.9) is
currently declared without a test scope and will be packaged into production
artifacts; update the dependency declaration to add a <scope>test</scope> so
wiremock-spring-boot is only available at test time and not on the
compile/runtime classpath or in the produced JAR/WAR.
In `@src/main/java/org/example/projectbifrost/service/ChatService.java`:
- Line 15: Remove the unused import
org.springframework.resilience.annotation.Retryable from the ChatService class:
locate the import statement at the top of ChatService.java and delete it
(keeping the io.github.resilience4j.retry.annotation.Retry usage intact); then
run an import-organize or build to ensure no other unused imports remain and
compilation passes.
In `@src/main/resources/application.properties`:
- Line 11: The retry config for the "chatService" instance is too broad and
immediate: update the resilience4j properties for
resilience4j.retry.instances.chatService to (1) add retry-exceptions or
ignore-exceptions so only transient failures are retried (e.g.,
retryExceptions=LLMException or use ignoreExceptions=SomeClientErrorException)
or configure a predicate that filters by HTTP status instead of retrying all
exceptions thrown by fetchResponseFromLLM, and (2) add a wait-duration (e.g.,
exponential/backoff-friendly value) so attempts are spaced out; reference the
chatService instance and the fetchResponseFromLLM / LLMException symbols when
making these property changes.
In `@src/test/java/org/example/projectbifrost/service/ChatServiceTest.java`:
- Around line 54-56: The assertion in ChatServiceTest uses
findAll(getRequestedFor("/chat/completions")) which matches GETs while the
service sends POSTs (so it always returns empty) and also duplicates the
existing verify(3, postRequestedFor(...)) check; fix by either replacing
getRequestedFor(...) with postRequestedFor(...) inside findAll to correctly
match POST requests for the endpoint, or simply remove the findAll(...)
assertion (lines calling findAll/getRequestedFor) since verify(3,
postRequestedFor(...)) already asserts the same behavior.
---
Outside diff comments:
In `@src/main/java/org/example/projectbifrost/service/ChatService.java`:
- Around line 47-65: chatWithLLM is calling fetchResponseFromLLM on the same
ChatService instance, which bypasses Spring AOP proxies so `@CircuitBreaker` and
`@Retry` never run; extract fetchResponseFromLLM into a separate Spring bean
(e.g., LLMClient with a public
fetchResponseFromLLM(List<OpenRouterRequestDTO.Message>) method annotated with
`@CircuitBreaker` and `@Retry` and a fallback method) and inject that bean into
ChatService, then have chatWithLLM delegate to the injected
LLMClient.fetchResponseFromLLM(...) (alternatively inject a proxied self
reference with `@Lazy` to call self.fetchResponseFromLLM(...)).
---
Nitpick comments:
In `@pom.xml`:
- Around line 115-118: The POM currently pulls the reactive circuit-breaker
starter (artifactId spring-cloud-starter-circuitbreaker-reactor-resilience4j)
while this service is synchronous; change the dependency to the non-reactive
starter (artifactId spring-cloud-starter-circuitbreaker-resilience4j) so the app
doesn't register Reactor/WebFlux circuit-breaker infrastructure that
ChatService.fetchResponseFromLLM (synchronous RestClient) doesn't use; update
the dependency entry in pom.xml accordingly and run a build to ensure no unused
reactive classes remain referenced.
In `@src/main/java/org/example/projectbifrost/service/ChatService.java`:
- Around line 92-94: The fallback method currently returns a generic string and
swallows the exception; update the
ChatService.fallback(List<OpenRouterRequestDTO.Message> apiMessages, Exception
e) to log the failure (use log.warn or log.error) and include the exception as
the throwable parameter plus brief context (e.g., apiMessages size or a short
summary) so repeated fallbacks are observable in logs; keep the existing return
value or make it slightly more descriptive if desired, but do not remove the
exception logging.
🪄 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: 0c641f49-873b-4dfe-b8ba-a16fea73d2aa
📒 Files selected for processing (6)
pom.xmlsrc/main/java/org/example/projectbifrost/BifrostController.javasrc/main/java/org/example/projectbifrost/configuration/RestClientConfiguration.javasrc/main/java/org/example/projectbifrost/service/ChatService.javasrc/main/resources/application.propertiessrc/test/java/org/example/projectbifrost/service/ChatServiceTest.java
…r LLM errors with retry support. Enhance `RestClientConfiguration` with resilient methods and `HttpComponentsClientHttpRequestFactory`. Update global exception handler and tests.
…llback behavior after consecutive failures
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/org/example/projectbifrost/service/ChatService.java (1)
46-64:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftCritical: self-invocation bypasses Resilience4j AOP — retry/circuit breaker won't fire in production.
chatWithLLMcallsfetchResponseFromLLM(apiMessages)directly via implicitthis, which bypasses the Spring AOP proxy. Resilience4j's@CircuitBreakerand@Retryare proxy-based, so they are skipped on internal calls — meaning the controller flow gets neither retries nor circuit breaking, andfallback(...)will never be invoked along this path. YourChatServiceTestlikely passes because it invokesfetchResponseFromLLMexternally on the autowired bean, masking this bug.Major: fallback method silently swallows exceptions with no logging. The fallback accepts an
Exception eparameter but discards it entirely, returning"Fallback!"with zero observability. This hides the root cause of failures and makes debugging production issues extremely difficult.Minor: RetryableHttpException loses HTTP status code and response details. The
onStatuslambda throws the exception with a hardcoded message but doesn't preserve the original status code (429 vs 500) or response body, losing debugging context.Remediate by moving
fetchResponseFromLLMto a separate Spring bean (preferred — clean separation), or use self-injection /AopContext.currentProxy(). Add logging to the fallback method to record the exception cause.♻️ Proposed fix (self-injection example)
`@Service` public class ChatService { private final ChatSessionStorage chatSessionStorage; private final RestClient restClient; + private final ApplicationContext applicationContext; + private ChatService self; `@Value`("${openrouter.model}") private String model; - public ChatService(ChatSessionStorage chatSessionStorage, RestClient restClient) { + public ChatService(ChatSessionStorage chatSessionStorage, + RestClient restClient, + ApplicationContext applicationContext) { this.chatSessionStorage = chatSessionStorage; this.restClient = restClient; + this.applicationContext = applicationContext; + } + + `@PostConstruct` + void initSelf() { + this.self = applicationContext.getBean(ChatService.class); } @@ - String content = fetchResponseFromLLM(apiMessages); + String content = self.fetchResponseFromLLM(apiMessages); + + public String fallback(List<OpenRouterRequestDTO.Message> apiMessages, Exception e) { + log.error("LLM circuit breaker opened, fallback invoked", e); + return "Fallback!"; }The cleaner long-term fix is to extract
fetchResponseFromLLM(andfallback) into a dedicatedLLMClientbean and inject it.
🧹 Nitpick comments (2)
src/main/java/org/example/projectbifrost/service/ChatService.java (1)
36-45: 💤 Low valueJavadoc no longer reflects the exception contract.
The javadoc claims only
InvalidLLMResponseExceptionis thrown, butfetchResponseFromLLMcan also raiseRetryableHttpException(and any unexpectedRestClienterrors), and — depending on how the self-invocation issue is resolved — may instead silently return"Fallback!". Tighten the@throwsand behavioral notes once the proxy/fallback design is finalized.🤖 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/org/example/projectbifrost/service/ChatService.java` around lines 36 - 45, Update the Javadoc for ChatService.sendChatRequest to accurately reflect all exceptions and behaviors: mention that fetchResponseFromLLM may throw RetryableHttpException and other RestClientException types in addition to InvalidLLMResponseException, and note any fallback string behavior (e.g., returning "Fallback!" if applicable) until the proxy/fallback design is finalized; reference the sendChatRequest method, fetchResponseFromLLM, RetryableHttpException, RestClientException, and InvalidLLMResponseException so callers know what to expect and when to handle retries or fallbacks.src/main/java/org/example/projectbifrost/configuration/RestClientConfiguration.java (1)
8-14: ⚡ Quick winRemove unused
@EnableResilientMethodsannotation.
@EnableResilientMethodsonly activates Spring's native@Retryableand@ConcurrencyLimit, neither of which are used in this codebase. YourChatServiceuses Resilience4j's@CircuitBreakerand@Retryannotations, which are wired automatically by theresilience4j-spring-boot3starter. Remove the unused annotation and import.Proposed fix
-import org.springframework.resilience.annotation.EnableResilientMethods; import org.springframework.web.client.RestClient; import java.net.URI; `@Configuration` -@EnableResilientMethods public class RestClientConfiguration {🤖 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/org/example/projectbifrost/configuration/RestClientConfiguration.java` around lines 8 - 14, Remove the unused Spring annotation and import: the `@EnableResilientMethods` import and annotation on RestClientConfiguration are not needed because the project uses Resilience4j annotations (`@CircuitBreaker`, `@Retry`) wired by resilience4j-spring-boot3; open the RestClientConfiguration class, delete the import for org.springframework.resilience.annotation.EnableResilientMethods and remove the `@EnableResilientMethods` annotation declaration so only needed imports and the `@Configuration` remain.
🤖 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/org/example/projectbifrost/exception/GlobalExceptionHandler.java`:
- Around line 38-48: The handler handleRetryableException for
RetryableHttpException is never reached because ChatService.fetchResponseFromLLM
is wrapped with `@CircuitBreaker` which calls the fallback method instead; either
remove or change the circuit breaker fallback so the exception is propagated
(e.g., remove the fallback attribute or have the fallback rethrow or
wrap/propagate RetryableHttpException) so handleRetryableException can execute,
or if the fallback behavior is intended, delete handleRetryableException to
avoid dead code — reference ChatService.fetchResponseFromLLM, the circuit
breaker fallback method named fallback, and
GlobalExceptionHandler.handleRetryableException when making the change.
In `@src/main/java/org/example/projectbifrost/service/ChatService.java`:
- Around line 75-78: The current lambda throws a generic
RetryableHttpException("The Gods are silent") and discards upstream HTTP
details; update the flow so RetryableHttpException carries the original HTTP
status, optional Retry-After header and a truncated response body excerpt
(similar to InvalidLLMResponseException's statusCode pattern). Change the lambda
inside ChatService (the .onStatus handler) to extract resp.status().value(),
resp.headers().asHttpHeaders().getFirst("Retry-After") and a safe, truncated
resp.bodyAsString() (or buffer peek) and construct a new
RetryableHttpException(statusCode, retryAfter, bodyExcerpt, message). Modify the
RetryableHttpException class to add fields (int statusCode, String retryAfter,
String bodyExcerpt), constructors and getters, and update
GlobalExceptionHandler.handleRetryableException to log and expose these fields
(and make retry logic branch on statusCode==429 using retryAfter) so
logs/metrics can distinguish rate limits from server errors.
- Around line 92-94: The fallback in ChatService.fallback currently swallows
Exception e and returns a literal "Fallback!" which hides errors and prevents
GlobalExceptionHandler.handleRetryableException from handling
RetryableHttpException; update fallback to either (A) rethrow the original
exception (or wrap and throw a RetryableHttpException) so chatWithLLM lets the
error propagate to GlobalExceptionHandler, removing the fallbackMethod if
propagation is desired, or (B) if a persisted sentinel is required, log the full
exception with context via the existing logger, increment/error metrics, and
return a clear sentinel string (e.g., "<LLM_ERROR>") while ensuring the
controller can still map that sentinel to a 503; refer to ChatService.fallback,
chatWithLLM, GlobalExceptionHandler.handleRetryableException, and
RetryableHttpException when making the change.
---
Nitpick comments:
In
`@src/main/java/org/example/projectbifrost/configuration/RestClientConfiguration.java`:
- Around line 8-14: Remove the unused Spring annotation and import: the
`@EnableResilientMethods` import and annotation on RestClientConfiguration are not
needed because the project uses Resilience4j annotations (`@CircuitBreaker`,
`@Retry`) wired by resilience4j-spring-boot3; open the RestClientConfiguration
class, delete the import for
org.springframework.resilience.annotation.EnableResilientMethods and remove the
`@EnableResilientMethods` annotation declaration so only needed imports and the
`@Configuration` remain.
In `@src/main/java/org/example/projectbifrost/service/ChatService.java`:
- Around line 36-45: Update the Javadoc for ChatService.sendChatRequest to
accurately reflect all exceptions and behaviors: mention that
fetchResponseFromLLM may throw RetryableHttpException and other
RestClientException types in addition to InvalidLLMResponseException, and note
any fallback string behavior (e.g., returning "Fallback!" if applicable) until
the proxy/fallback design is finalized; reference the sendChatRequest method,
fetchResponseFromLLM, RetryableHttpException, RestClientException, and
InvalidLLMResponseException so callers know what to expect and when to handle
retries or fallbacks.
🪄 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: 0bbe1f73-77f9-4584-b670-e723ba5f48fc
📒 Files selected for processing (8)
pom.xmlsrc/main/java/org/example/projectbifrost/configuration/RestClientConfiguration.javasrc/main/java/org/example/projectbifrost/exception/GlobalExceptionHandler.javasrc/main/java/org/example/projectbifrost/exception/InvalidLLMResponseException.javasrc/main/java/org/example/projectbifrost/exception/RetryableHttpException.javasrc/main/java/org/example/projectbifrost/service/ChatService.javasrc/main/resources/application.propertiessrc/test/java/org/example/projectbifrost/service/ChatServiceTest.java
✅ Files skipped from review due to trivial changes (1)
- src/main/java/org/example/projectbifrost/exception/RetryableHttpException.java
🚧 Files skipped from review as they are similar to previous changes (3)
- src/test/java/org/example/projectbifrost/service/ChatServiceTest.java
- pom.xml
- src/main/resources/application.properties
…n with exponential backoff and custom exceptions. Enhance fallback handling with detailed logging.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/main/resources/application.properties (1)
17-18: 💤 Low valueConfirm the aspect-order inversion is intentional.
You set
circuitBreakerAspectOrder=1andretryAspectOrder=2, which (in Spring AOP, lower order = outer) makes the effective wrappingCircuitBreaker( Retry( method ) ). Resilience4j's documented default is the opposite: "The Resilience4j Aspects order is the following: Retry ( CircuitBreaker ( RateLimiter ( TimeLimiter ( Bulkhead ( Function ) ) ) ) ) so Retry is applied at the end (if needed)."Practical consequences of the PR's order:
- When the circuit is
OPEN, the call short-circuits tofallbackimmediately —Retrynever runs (often desirable).- A logical call that exhausts all retries counts as one CB failure event, not three (less aggressive CB tripping).
- Conversely, transient failures recovered by
Retryare completely invisible to the CB sliding window, so the CB can't react to a noisy-but-recovering upstream.If you intended the default semantics (each individual attempt counted by the CB, retries run even when the circuit was closed at the first attempt), swap the orders. Otherwise, a one-line comment in the properties file documenting the intent would help future readers.
🤖 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/resources/application.properties` around lines 17 - 18, Current aspect order sets circuitBreakerAspectOrder=1 and retryAspectOrder=2, which makes CircuitBreaker wrap Retry (CircuitBreaker( Retry(method) )); either swap the two properties to restore Resilience4j's documented default Retry outer-of-CircuitBreaker (set retryAspectOrder=1 and circuitBreakerAspectOrder=2) or, if the inversion is intentional, add a one-line comment next to resilience4j.circuitbreaker.circuitBreakerAspectOrder and resilience4j.retry.retryAspectOrder explaining the deliberate semantics (e.g., "Intentional: CircuitBreaker outer of Retry to short-circuit OPEN circuits and avoid counting retries as multiple CB events").src/main/java/org/example/projectbifrost/service/ChatService.java (1)
97-97: 💤 Low valueConsider matching the fallback method signature to be more explicit and robust against future additions.
The current
fallback(Exception e)works today becausefetchResponseFromLLMhas exactly one parameter (Resilience4j's global fallback pattern matches this case). However, if you later add other@CircuitBreaker(name="chatService", fallbackMethod="fallback")methods with different parameter shapes, this fallback won't match them.Making the signature explicit—
public String fallback(List<OpenRouterRequestDTO.Message> apiMessages, Exception e)—is more conventional and defensive, giving the fallback access to request context and ensuring it works correctly regardless of future method additions.🤖 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/org/example/projectbifrost/service/ChatService.java` at line 97, Change the fallback method signature in ChatService from public String fallback(Exception e) to public String fallback(List<OpenRouterRequestDTO.Message> apiMessages, Exception e) so it matches Resilience4j's expected fallback shape for fetchResponseFromLLM and future `@CircuitBreaker` methods; update the method body to use the passed apiMessages instead of relying solely on external state and ensure any references to fallback in the class still resolve to the new signature (method name: fallback, related method: fetchResponseFromLLM).
🤖 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/org/example/projectbifrost/service/ChatService.java`:
- Around line 59-64: The current flow persists the literal fallback string into
history because fetchResponseFromLLM can return the sentinel "Fallback!" and
chatSession.addMessage(new ChatMessage("assistant", content)) always runs;
change the logic so fallback does not become stored assistant content by either
(preferred) making fallback throw a dedicated LLMUnavailableException (e.g.,
from fetchResponseFromLLM) and let
GlobalExceptionHandler.handleRetryableException handle it, or (alternative) have
fetchResponseFromLLM return Optional<String> / null and in chatWithLLM only call
chatSession.addMessage for the assistant when a real response is present (i.e.,
skip persisting when the sentinel/empty Optional is returned); update references
to fetchResponseFromLLM, fallback, chatSession.addMessage, and
GlobalExceptionHandler.handleRetryableException accordingly.
---
Nitpick comments:
In `@src/main/java/org/example/projectbifrost/service/ChatService.java`:
- Line 97: Change the fallback method signature in ChatService from public
String fallback(Exception e) to public String
fallback(List<OpenRouterRequestDTO.Message> apiMessages, Exception e) so it
matches Resilience4j's expected fallback shape for fetchResponseFromLLM and
future `@CircuitBreaker` methods; update the method body to use the passed
apiMessages instead of relying solely on external state and ensure any
references to fallback in the class still resolve to the new signature (method
name: fallback, related method: fetchResponseFromLLM).
In `@src/main/resources/application.properties`:
- Around line 17-18: Current aspect order sets circuitBreakerAspectOrder=1 and
retryAspectOrder=2, which makes CircuitBreaker wrap Retry (CircuitBreaker(
Retry(method) )); either swap the two properties to restore Resilience4j's
documented default Retry outer-of-CircuitBreaker (set retryAspectOrder=1 and
circuitBreakerAspectOrder=2) or, if the inversion is intentional, add a one-line
comment next to resilience4j.circuitbreaker.circuitBreakerAspectOrder and
resilience4j.retry.retryAspectOrder explaining the deliberate semantics (e.g.,
"Intentional: CircuitBreaker outer of Retry to short-circuit OPEN circuits and
avoid counting retries as multiple CB events").
🪄 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: b95978e7-a835-4cb1-92e3-a8b6b1b6cf35
📒 Files selected for processing (2)
src/main/java/org/example/projectbifrost/service/ChatService.javasrc/main/resources/application.properties
…pdate test assertions for clarity
1 participant
This pull request introduces circuit breaker and retry mechanisms for improved resilience when communicating with the LLM service, updates dependencies to support these features, and adds integration tests to verify the retry logic. The changes also include some refactoring and enhancements for clarity and maintainability.
Resilience and Reliability Enhancements:
ChatService, specifically to thefetchResponseFromLLMmethod, to handle failures and retries when communicating with the LLM service (src/main/java/org/example/projectbifrost/service/ChatService.java). [1] [2]application.propertiesfor fine-tuned failure handling (src/main/resources/application.properties).Dependency and Build Updates:
spring-cloud-starter-circuitbreaker-reactor-resilience4jandwiremock-spring-bootdependencies, and importedspring-cloud-dependenciesBOM for dependency management inpom.xml. [1] [2] [3]spring-restdocs-asciidoctordependency from the build plugins section inpom.xml.Testing Improvements:
ChatServiceTest, ensuring that the service retries failed requests and succeeds when the LLM becomes available (src/test/java/org/example/projectbifrost/service/ChatServiceTest.java).Code Refactoring and Cleanup:
sendRequestToLLMtochatWithLLMinChatServiceand updated references inBifrostControllerfor clarity and consistency (src/main/java/org/example/projectbifrost/service/ChatService.java,src/main/java/org/example/projectbifrost/BifrostController.java). [1] [2]openAIWebClientbean toopenRouterRestClientfor better alignment with its purpose (src/main/java/org/example/projectbifrost/configuration/RestClientConfiguration.java).These updates collectively improve the robustness of the chat service, ensure graceful handling of LLM outages, and provide automated tests to validate the new behavior.
Summary by CodeRabbit
New Features
Chores
Tests