Add LlmClientException for handling client error responses and upda…#6
Conversation
…te exception handling across components and tests.
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ 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 (4)
📝 WalkthroughWalkthroughA new exception type ChangesHTTP Error Handling Enhancement
Sequence DiagramsequenceDiagram
participant Client
participant Controller
participant Service
participant LlmClient
participant RemoteAPI as Remote LLM API
participant GlobalHandler as GlobalExceptionHandler
Note over Client,GlobalHandler: Old Flow (429 only)
Client->>Controller: POST /api/v1/chat
Controller->>Service: chat(request)
Service->>LlmClient: sendMessages()
LlmClient->>RemoteAPI: HTTP POST
RemoteAPI-->>LlmClient: 429 or 503
LlmClient-->>Service: throws LlmUnavailableException
Service-->>Controller: propagate exception
Controller->>GlobalHandler: handleLlmUnavailable()
GlobalHandler-->>Client: 503 Service Unavailable
rect rgba(100, 150, 200, 0.5)
Note over Client,GlobalHandler: New Flow (4xx vs 5xx)
Client->>Controller: POST /api/v1/chat
Controller->>Service: chat(request)
Service->>LlmClient: sendMessages()
LlmClient->>RemoteAPI: HTTP POST
alt 4xx (400, 401, etc.)
RemoteAPI-->>LlmClient: 4xx Client Error
LlmClient-->>Service: throws LlmClientException
Service-->>Controller: propagate exception
Controller->>GlobalHandler: handleLlmClientError()
GlobalHandler-->>Client: 502 Bad Gateway
else 429 or 5xx
RemoteAPI-->>LlmClient: 429 or 5xx Server Error
LlmClient-->>Service: throws LlmUnavailableException
Service-->>Controller: propagate exception
Controller->>GlobalHandler: handleLlmUnavailable()
GlobalHandler-->>Client: 503 Service Unavailable
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/main/java/org/example/springllmgateway/exception/GlobalExceptionHandler.java (1)
17-24: 💤 Low valueConsider revisiting the HTTP 502 status for
LlmClientException.HTTP 502 semantically means "received an invalid response from an upstream server." Here the upstream LLM returned a perfectly valid 4xx response — it rejected a request. Returning 502 makes
LlmClientExceptionindistinguishable from a network-level bad-gateway scenario from a monitoring/alerting perspective, and it conflates two different failure modes withLlmUnavailableException→ 503.Alternatives that are arguably more accurate:
- 400 if a malformed payload from the original client reached the LLM.
- 422 for semantically invalid requests the LLM rejected.
- 401 / 403 pass-through if the root cause is a credentials/auth misconfiguration.
If the deliberate choice is to hide upstream status codes from callers (a valid gateway pattern), at minimum document why 502 was chosen over 503 or 400 for discoverability.
src/test/java/org/example/springllmgateway/client/LlmClientTest.java (1)
142-151: ⚡ Quick win
hasSize(4)assertion is correct; heads-up on real-delay cost.Spring Framework's
@RetryablecountsmaxRetriesas retries in addition to the initial invocation — total attempts = 1 +maxRetries. WithmaxRetries = 3, the exhaustion test correctly expects 4 total WireMock calls.However, with
delay = 1000,multiplier = 2, and three retries, this test incurs real wall-clock delays of approximately 1 000 ms + 2 000 ms + 4 000 ms ≈ 7+ seconds per run. Combined with the two new single-retry tests (~1 s each), the new tests add ~9 s to the@SpringBootTestsuite. If CI run-time becomes a concern, consider externalizing the retry delay to a property (e.g.,llm.retry.delay) so it can be overridden to0or1inapplication-test.properties.💡 Example: externalize delay for test override
In
LlmClient.java:`@Retryable`( includes = LlmUnavailableException.class, maxRetries = 3, - delay = 1000, + delayString = "${llm.retry.delay:1000}", multiplier = 2, jitter = 200, maxDelay = 5000 )Then in
src/test/resources/application-test.properties(activated via@SpringBootTest):llm.retry.delay=1🤖 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/org/example/springllmgateway/client/LlmClientTest.java` around lines 142 - 151, The test causes real retry delays because the `@Retryable` settings in LlmClient use fixed delay/multiplier; externalize the retry timing into a configurable property (e.g., llm.retry.delay and llm.retry.multiplier) used by the `@Retryable` on the method in LlmClient so tests can override it, then add application-test.properties (activated by the test profile) setting llm.retry.delay=1 (or 0) and llm.retry.multiplier=1 to eliminate long waits during CI; update any references to the retry attributes in LlmClient to read from the new properties (or from a `@ConfigurationProperties/Environment`) so runtime behavior is unchanged but tests can shorten delays.
🤖 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.
Nitpick comments:
In `@src/test/java/org/example/springllmgateway/client/LlmClientTest.java`:
- Around line 142-151: The test causes real retry delays because the `@Retryable`
settings in LlmClient use fixed delay/multiplier; externalize the retry timing
into a configurable property (e.g., llm.retry.delay and llm.retry.multiplier)
used by the `@Retryable` on the method in LlmClient so tests can override it, then
add application-test.properties (activated by the test profile) setting
llm.retry.delay=1 (or 0) and llm.retry.multiplier=1 to eliminate long waits
during CI; update any references to the retry attributes in LlmClient to read
from the new properties (or from a `@ConfigurationProperties/Environment`) so
runtime behavior is unchanged but tests can shorten delays.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4f851cc4-5322-4178-b538-880df6ab342a
📒 Files selected for processing (5)
src/main/java/org/example/springllmgateway/client/LlmClient.javasrc/main/java/org/example/springllmgateway/exception/GlobalExceptionHandler.javasrc/main/java/org/example/springllmgateway/exception/LlmClientException.javasrc/test/java/org/example/springllmgateway/client/LlmClientTest.javasrc/test/java/org/example/springllmgateway/controller/ChatControllerTest.java
… handling in `GlobalExceptionHandler`, and add unit tests for 4xx status mappings
1 participant
…te exception handling across components and tests.
Summary by CodeRabbit
Bug Fixes
Tests