[OPIK-8240] [BE] fix: split provider-error retryability by status, resume XAUTOCLAIM from its cursor - #8137
Draft
thiagohora wants to merge 1 commit into
Draft
Conversation
…sume XAUTOCLAIM from its cursor Two independent defects that each leave a permanently failing online-scoring message cycling instead of retiring it. 1. A permanent provider 4xx was retried as if transient. ChatCompletionService.scoreTrace answered every unmappable provider failure with a blanket InternalServerErrorException, which sits outside BaseRedisSubscriber.NON_RETRYABLE_EXCEPTIONS -- so a request that can never succeed was replayed maxRetries times, once per pendingMessageDuration. The real status was available the whole time on the HttpException in the cause chain; it went unread because the existing recovery path classifies by family, and mapping all of 4xx to ClientErrorException would drop transient 408/429 after one attempt. Hence the blanket 500. Classify by status code instead: 400/401/403 and the rest of 4xx are permanent, 408/425/429 and all 5xx stay retryable. scoreTrace gets its own mapping so create() and the streaming handler keep returning the provider's status verbatim to HTTP callers. This also closes a pre-existing over-eager drop on the parseable branch, where a JSON-parseable 429 already became ClientErrorException and was dropped after one attempt. 2. XAUTOCLAIM never scanned past the first ~100 pending entries. claimPendingMessages passed StreamMessageId.MIN as the scan start every call and discarded the cursor Redis returns. Redis caps each XAUTOCLAIM at COUNT * 10 PEL entries *examined*, so at consumerBatchSize=10 a call inspects only the first 100 -- and restarting at MIN means nothing past that window is ever examined. Any backlog above ~100 grows a permanently unreachable tail, whatever the retry or decode behaviour downstream. Carry getNextId() forward, resetting to MIN on Redis's 0-0 end-of-pass reply so the oldest-first bias is preserved. A failed scan deliberately does not advance the cursor, so its window is retried rather than skipped. The end-of-pass sentinel is compared numerically, not against StreamMessageId.MIN/.ALL: those are wire sentinels serializing to "-" and "0", and neither is equals() to the StreamMessageId(0, 0) Redisson parses 0-0 into (verified against redisson 4.7.0), so matching on the constants would never fire and the scan would park at the end of the PEL. Tests: 132 green across the affected suites. Both fixes mutation-checked -- collapsing the status split to the whole 4xx family fails 5 tests, removing it entirely fails 7; reverting the cursor to always-MIN fails 3. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
java
Contributor
📋 PR Linter Failed❌ Missing Section. The description is missing the |
Contributor
⏱️ pre-commit per-hook timing
⏭️ 42 skipped (no matching files changed)
|
![baz-reviewer[bot]](https://avatars.githubusercontent.com/in/933528?s=60&v=4){kind=small}
Comment on lines
+713
to
+724
| @Test | ||
| @DisplayName("A parseable provider error is split the same way, not just the unparseable path") | ||
| void scoreTrace__whenParseableProviderError__thenSplitByStatusToo() { | ||
| var chatRequest = ChatRequest.builder().messages(UserMessage.from("score this")).build(); | ||
| var modelParameters = podamFactory.manufacturePojo(LlmAsJudgeModelParameters.class); | ||
| var workspaceId = "test-workspace-id"; | ||
| var providerFailure = new RuntimeException("provider said no"); | ||
|
|
||
| when(llmProviderFactory.getLanguageModel(anyString(), any())).thenReturn(chatModel); | ||
| when(chatModel.chat(any(ChatRequest.class))).thenThrow(providerFailure); | ||
| when(llmProviderFactory.getService(anyString(), anyString())).thenReturn(llmProviderService); | ||
| // A parseable 429 used to reach failHandlingLLMProviderError and become a ClientErrorException, |
Contributor
There was a problem hiding this comment.
Missing mapper-boundary retryability coverage
scoreTrace__whenParseableProviderError__thenSplitByStatusToo mocks getLlmProviderError directly, so it bypasses the mapper/provider path and can't catch precedence when a mapper fallback should lose to a nested HttpException, and the matrix skips OpenAI 500 and Custom 400 fallbacks against opposite retryability categories — should we add scoreTrace seam cases that exercise the real provider path and assert both exception type and Redis retryability (ClientErrorException non-retryable vs InternalServerErrorException retryable)?
Want Baz to fix this for you? Activate Fixer You can also update your AI coding guidelines based on this comment by apply pr to [branch name]
Other fix methods
Prompt for AI Agents
In
`apps/opik-backend/src/test/java/com/comet/opik/domain/llm/ChatCompletionServiceTest.java`
around lines 713-724, update
`scoreTrace__whenParseableProviderError__thenSplitByStatusToo` so it exercises the
actual provider mapper path instead of mocking `getLlmProviderError` to return an
`ErrorMessage` directly. Add focused cases covering the OpenAI fallback to 500 and
Custom fallback to 400, each paired with an opposite retryability category, including
nested `HttpException` precedence. Assert both the resulting exception type/status and
the Redis subscriber classification (`ClientErrorException` non-retryable versus
`InternalServerErrorException` retryable).
| } | ||
|
|
||
| @Test | ||
| @DisplayName("A parseable provider error is split the same way, not just the unparseable path") |
Contributor
There was a problem hiding this comment.
Malformed test display name
The test label A parseable provider error is split the same way reads awkwardly — should we rename it to A parseable provider error is classified the same way?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
In
`apps/opik-backend/src/test/java/com/comet/opik/domain/llm/ChatCompletionServiceTest.java`
around line 714, update the
`scoreTrace__whenParseableProviderError__thenSplitByStatusToo` test's `@DisplayName`,
replacing “A parseable provider error is split the same way” with “A parseable
provider error is classified the same way”.
Comment on lines
+786
to
+797
| if (ChatCompletionService.isPermanentFailure(expectedStatus)) { | ||
| // Retrying cannot change the outcome — hand the subscriber a type it drops immediately. | ||
| assertThat(thrown) | ||
| .isInstanceOf(ClientErrorException.class) | ||
| .isNotInstanceOf(InternalServerErrorException.class); | ||
| assertThat(((WebApplicationException) thrown).getResponse().getStatus()).isEqualTo(expectedStatus); | ||
| } else { | ||
| // Transient (or unknown) — stay outside NON_RETRYABLE_EXCEPTIONS so maxRetries is honoured. | ||
| assertThat(thrown) | ||
| .isInstanceOf(InternalServerErrorException.class) | ||
| .isNotInstanceOf(ClientErrorException.class); | ||
| assertThat(((WebApplicationException) thrown).getResponse().getStatus()).isEqualTo(500); |
Contributor
There was a problem hiding this comment.
Conditional assertions obscure retryability coverage
ChatCompletionService.isPermanentFailure(expectedStatus) makes the parameterized test switch between two assertion flows, so failures are harder to scan and localize — should we split permanent and retryable cases into separate tests, or parameterize the expected exception/status and keep one unconditional flow?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
`apps/opik-backend/src/test/java/com/comet/opik/domain/llm/ChatCompletionServiceTest.java`
around lines 786-797, refactor
`scoreTrace__whenProviderErrorUnparsed__thenRetryabilityFollowsStatus` so it no longer
uses an `if` to select two different assertion flows. Split permanent and retryable
statuses into separate parameterized tests, or add expected exception type and response
status to the test parameters and use one unconditional assertion sequence.
Comment on lines
+158
to
+160
| int status = providerError.map(ErrorMessage::getCode) | ||
| .filter(ChatCompletionService::isErrorStatus) | ||
| .or(() -> findProviderHttpStatus(runtimeException)) |
Contributor
There was a problem hiding this comment.
Synthetic statuses cause wrong retry/drop behavior
The status selection at apps/opik-backend/src/main/java/com/comet/opik/domain/llm/ChatCompletionService.java:158-161 trusts fallback provider mappings over valid nested HTTP statuses, so unknown OpenAI codes hide upstream 400s and Custom LLM fallback 400s hide upstream 500/503s, misclassifying failures as retryable or immediately removing them — should we prefer the cause-chain status and add both conflicting-status scoreTrace regressions?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
`apps/opik-backend/src/main/java/com/comet/opik/domain/llm/ChatCompletionService.java`
around lines 158-161, fix the `scoreTrace` status-selection logic so a valid nested
`HttpException` status takes precedence over mapped provider codes, since OpenAI and
Custom LLM mappers may return synthetic fallback 500 or 400 values. Use the
provider-mapped status only when no valid cause-chain HTTP status exists, while
preserving the existing retryability split. Add `scoreTrace` regression tests covering
OpenAI synthetic 500 with nested HTTP 400 and Custom LLM synthetic 400 with nested HTTP
500/503.
Comment on lines
+165
to
+171
| if (isPermanentFailure(status)) { | ||
| // Non-retryable: the subscriber acks and removes on the first delivery instead of burning | ||
| // maxRetries x pendingMessageDuration on a request whose outcome cannot change. This is the | ||
| // OPIK-8193 case - a provider rejecting an oversized/invalid request rejects every retry of | ||
| // it identically. | ||
| throw new ClientErrorException(buildDetailedErrorMessage(runtimeException), status, | ||
| runtimeException); |
Contributor
There was a problem hiding this comment.
Mixed thread failures lose retryable work
OnlineScoringTraceThreadLlmAsJudgeScorer re-emits only errors.getFirst() after flatMap, so arrival order can select ClientErrorException; when it arrives first, BaseRedisSubscriber acknowledges and removes the whole message, dropping the retryable sibling — should we split/requeue thread IDs independently or make the aggregate retryable whenever any sibling is retryable?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
`apps/opik-backend/src/main/java/com/comet/opik/domain/llm/ChatCompletionService.java`
around lines 165-171, review the `scoreTrace` permanent-4xx exception path: emitting a
`ClientErrorException` can cause `OnlineScoringTraceThreadLlmAsJudgeScorer` to retain
only the arrival-order first error and acknowledge a Redis message containing retryable
sibling threads. Refactor the error propagation and/or caller handling so trace-thread
IDs are split and requeued independently, or produce a deterministic aggregate that
remains retryable whenever any sibling failure is retryable. Add coverage for mixed
permanent and transient failures arriving in either order.
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
1 participant
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.
Details
Two independent defects in the online-scoring consumer, both found while investigating a customer's stuck scoring backlog. Each one leaves a permanently failing message cycling instead of retiring it, and they sit at different layers, so neither fix substitutes for the other.
1. A permanent provider 4xx was retried as if it were transient
ChatCompletionService.scoreTraceanswered every provider failure it could not map with a blanketInternalServerErrorException. That type sits outsideBaseRedisSubscriber.NON_RETRYABLE_EXCEPTIONS, so the subscriber replayed the messagemaxRetriestimes, once perpendingMessageDuration, before dropping it.The real status was available the whole time — the
HttpExceptionin the cause chain carriesstatusCode()— but went unread, because the existing recovery path (failHandlingLLMProviderError) classifies by family: it maps all of 4xx toClientErrorException, which would also make transient408/429non-retryable and drop a rate-limited evaluation after one attempt. That is exactly whyscoreTraceskipped status recovery altogether; the comment there said so.The fix is to classify by status code rather than family:
RetriableExceptionBaseRedisSubscriber's own "unknown defaults to retryable for safety"scoreTracegets its own mapping, socreate()and the streaming handler keep returning the provider's status verbatim to HTTP callers — they answer an HTTP client, where a 429 should stay a 429.This also closes a pre-existing over-eager drop on the parseable branch: a JSON-parseable
429already reachedfailHandlingLLMProviderErrorand becameClientErrorException, i.e. dropped after one attempt rather than retried. Same bug, opposite direction, reached through the other branch.2.
XAUTOCLAIMnever scanned past the first ~100 pending entriesclaimPendingMessagespassedStreamMessageId.MINas the scan start on every call and discarded the cursor Redis returns (AutoClaimResult.getNextId()).Redis caps each
XAUTOCLAIMatCOUNT * 10PEL entries examined (not claimed), so at the defaultconsumerBatchSizeof 10 a call only ever inspects the first 100 entries of the pending list. Restarting atMINevery time means anything past that window is never examined at all — any backlog above ~100 grows a permanently unreachable tail, whatever the retry or decode behaviour downstream.Verified against a real Redis before writing the fix: with PEL positions 1–100 made ineligible and 101–150 eligible,
COUNT 10claims nothing and returns cursor101-1;COUNT 20claims 20. That matches the customer's XPENDING data exactly — a cycling head of ~100 entries at delivery-count in the thousands, in front of ~664 entries frozen at delivery-count 1, idle 14–20 days.The fix carries
getNextId()forward, resetting toMINon Redis's0-0end-of-pass reply so the oldest-first bias the original code intended is preserved. A failed scan deliberately does not advance the cursor, so its window is retried rather than skipped.One subtlety worth flagging for review. The end-of-pass sentinel is compared numerically, not against
StreamMessageId.MIN/.ALL. Those are wire sentinels serializing to-and0, and neither isequals()to theStreamMessageId(0, 0)that Redisson parses Redis's literal0-0into — checked against redisson 4.7.0:Matching on the constants would therefore never fire, the cursor would park at the end of the PEL, and the scan would stop finding anything — strictly worse than the always-
MINbehaviour it replaces. There is a dedicated test pinning this.Change checklist
Behaviour change: a provider rejection that can never succeed is now dropped on the first delivery instead of after
maxRetries * pendingMessageDuration; transient failures are unaffected. Separately, a pending backlog deeper than ~100 entries is now fully reachable. No new configuration keys.Issues
AI-WATERMARK
AI-WATERMARK: yes
Testing
Unit
New coverage:
scoreTrace__whenPermanentClientError__thenNonRetryableClientErrorExceptioncarrying the real status, built from a plain-text body exactly like the real gateway rejectionscoreTrace__whenTransientStatus__thenStaysRetryableInternalServerErrorException, i.e. outsideNON_RETRYABLE_EXCEPTIONSscoreTrace__whenParseableProviderError__thenSplitByStatusTooisPermanentFailureSplitsTheFamilyCorrectlyshouldResumeTheNextScanFromTheReturnedCursorXAUTOCLAIMcalls resume at the returned cursor, captured from the actualstartargumentshouldWrapBackToTheStartAfterAFullPass0-0wraps back toMINrather than parking at the endshouldRetryTheSameWindowAfterAFailedScanBaseRedisSubscriberCursorTest(3)The existing
scoreTrace__whenProviderErrorUnparsed__thenStayRetryableasserted the old blanket-500 contract for every row, including 400 and 401. It is updated rather than deleted — same production-shaped data provider, assertion now keyed onisPermanentFailure(expectedStatus), so both halves of the split are exercised by the shapes that actually occurred in production.Mutation-checked, both fixes, both directions:
isPermanentFailurealways false (revert to blanket 500)MINEach restored and re-verified green afterwards.
End-to-end
Local Docker Compose stack, backend image built from this branch, real Redis, real
BaseRedisSubscriber, with a mock gateway returning the exact production error bodies.Fix 1, permanent 400 — mock gateway returns the verbatim redaction rejection (
status code: 400 ... redaction input text is too large ... equal or smaller than: 1000000):Dropped on the first delivery, one gateway call, ~400 ms. On the pre-fix build the same message logged
Retryable ... deliveryCount '1',deliveryCount '2', thenMax retries reached— three deliveries, three gateway calls, three minutes.Fix 1, transient 429 — same stack, gateway switched to a 429: 10 ×
Retryable error for messageId, 0 ×Non-retryable. The carve-out holds in the direction that matters most; this is the case a naive "all 4xx is permanent" fix would have broken.Fix 2, deep backlog — 130 traces posted against the 429 gateway to push the PEL to 142 entries, past the 100-entry scan budget. Captured from
redis-cli MONITOR:The
startargument walks forward instead of being-on every call, and the full 142-entry backlog drained toXLEN=0 pending=0— including the ~42 entries past the scan window that the old code could never have reached.Also confirmed the wrap: with a PEL small enough to scan in one pass, the argument stays
-, which is the correct0-0→MINreset rather than a regression.Not run, with reason:
mvn verify(Testcontainers suite) — relying on CI.Relationship to other work
processMessageat all and so are invisible to any retry-classification fix.All three are needed, and they compose rather than overlap. #8089 makes undecodable entries visible and retirable; this PR stops permanent failures from cycling, and makes a backlog beyond ~100 entries reachable at all.
🤖 Generated with Claude Code