[OPIK-7835] [BE] Map UnsupportedFeatureException to 400 instead of 500

[thiagohora]

Details

UnsupportedFeatureException is raised by langchain4j before the provider is ever called — e.g. ToolChoice.REQUIRED against Vertex AI Gemini — so getLlmProviderError() has nothing to map and both create() and scoreTrace() fell through to their catch-all InternalServerErrorException. That 500 is wrong twice over: nothing failed server-side, and no amount of retrying can make the call succeed. It also has a concrete cost — BaseRedisSubscriber.NON_RETRYABLE_EXCEPTIONS holds ClientErrorException but not InternalServerErrorException, so the llm_as_judge online-scoring consumer classified these as retryable and burned its full retry budget before dropping the message.

• New failIfUnsupportedFeature() runs first in the catch blocks of create() and scoreTrace(), throwing BadRequestException (400). It uses ExceptionUtils.indexOfType so it matches whether the exception is thrown bare or wrapped by the retry policy.
• Because BadRequestException extends ClientErrorException, the consumer now drops the message on the first attempt instead of retrying.
• The message carries a distinct Unsupported feature for the selected LLM provider prefix instead of the misleading Unexpected error calling LLM provider.
• Also covers the streaming path (getErrorHandler), which had the same hole — new ErrorMessage(String) defaults to 500.
• buildDetailedErrorMessage gained a baseMessage overload; the existing single-arg version delegates, so all current callers are unchanged.

Measured in production over a 6h window before the fix: 17,134 ToolChoice.REQUIRED is not supported, 10,638 UnsupportedFeatureException, and 500 Max retries reached drops on llm_as_judge.

There is prior art for treating this as a caller problem: LlmProviderOpenAiResponsesMapper (lines 387-396) already avoids 500ing on unsupported sampling params for the same reason.

Not fixed here: the affected rules still won't score. OnlineScoringLlmAsJudgeScorer requests ToolChoice.REQUIRED for agentic scoring, so every agentic LLM-judge rule pointed at Vertex AI Gemini fails deterministically. This PR makes the failure honest and cheap, not absent — the capability gap needs a separate fix. Tracked as a follow-up on the ticket.

Change checklist

• User facing
• Documentation update

Issues

• OPIK-7835

AI-WATERMARK

AI-WATERMARK: yes

• Tools: Claude Code
• Model(s): Claude Opus 5
• Scope: Production log investigation (Loki), root-cause analysis, the code change, and the new tests. Reviewed and directed by me throughout; the 4xx decision and the scope boundary were mine.
• Human verification: Diff reviewed line by line. Test results and the NON_RETRYABLE_EXCEPTIONS claim verified against the source. Production counts independently reproduced via Loki queries.

Testing

Commands run, from apps/opik-backend:

mvn -o compile -DskipTests     # clean
mvn -o test -Dtest=ChatCompletionServiceTest
mvn -o spotless:apply          # clean, no reformatting of surrounding code

ChatCompletionServiceTest: 17/17 pass — 11 pre-existing plus 6 new in an UnsupportedFeatureHandling nested class:

• bare and wrapped UnsupportedFeatureException through create() → 400 (parameterized)
• bare and wrapped UnsupportedFeatureException through scoreTrace() → 400 (parameterized)
• the provider-error lookup is skipped entirely for unsupported features (verify(..., never()))
• regression guard: unrelated RuntimeExceptions still yield 500

Tests re-run after rebasing onto current main.

Not run: ChatCompletionsResourceTest and OnlineScoringEngineTest are Testcontainers integration tests (ClickHouse/MySQL/Redis). Neither references UnsupportedFeatureException or ToolChoice, and this change is an additive branch keyed on one exception type, so I judged the risk low rather than spending the container spin-up. Flagging it since they're the ones that would catch a surprise. CI will cover them.

Documentation

No documentation change — internal error-mapping behavior. The HTTP status for this failure mode changes from 500 to 400, which is user-visible on the chat-completions endpoints; noted via the "User facing" checkbox above.

[github-actions]

⏱️ pre-commit per-hook timing

Hook	Description	Result	Duration
☕ spotless — java backend	Format Java code	✅	5.36s
Total (1 ran)			5.36s

⏭️ 41 skipped (no matching files changed)

Hook	Description	Result
🐍 trim trailing whitespace — python sdk	Strip trailing whitespace	⏭️
🐍 fix end of files — python sdk	Ensure files end in a newline	⏭️
🐍 ruff — python sdk	Lint + autofix Python (ruff)	⏭️
🐍 ruff-format — python sdk	Format Python code (ruff)	⏭️
🐍 mypy — python sdk	Static type check	⏭️
🤖 trim trailing whitespace — optimizer	Strip trailing whitespace	⏭️
🤖 fix end of files — optimizer	Ensure files end in a newline	⏭️
🤖 check yaml — optimizer	Validate YAML syntax	⏭️
🤖 check json — optimizer	Validate JSON syntax	⏭️
🤖 check toml — optimizer	Validate TOML syntax	⏭️
🤖 check for added large files — optimizer	Block large files (>1MB)	⏭️
🔐 detect private key — optimizer	Block committed private keys	⏭️
🤖 check for merge conflicts — optimizer	Block merge-conflict markers	⏭️
🤖 check for case conflicts — optimizer	Block case-only name clashes	⏭️
🤖 pyupgrade — optimizer	Modernize Python syntax	⏭️
🤖 ruff — optimizer	Lint + autofix Python (ruff)	⏭️
🤖 ruff-format — optimizer	Format Python code (ruff)	⏭️
🤖 mypy — optimizer	Static type check	⏭️
📓 nbstripout — optimizer notebooks	Strip notebook output	⏭️
📝 markdownlint — optimizer	Lint Markdown	⏭️
🔤 codespell — optimizer	Fix common misspellings	⏭️
📊 radon cc — optimizer	Cyclomatic-complexity gate	⏭️
📊 radon raw — optimizer	Raw size metrics gate	⏭️
📊 xenon — optimizer	Fail on complexity thresholds	⏭️
📊 lizard — optimizer	Cyclomatic-complexity gate	⏭️
🧹 vulture — optimizer	Find dead code	⏭️
🛡️ trim trailing whitespace — guardrails	Strip trailing whitespace	⏭️
🛡️ fix end of files — guardrails	Ensure files end in a newline	⏭️
🛡️ ruff — guardrails	Lint + autofix Python (ruff)	⏭️
🛡️ ruff-format — guardrails	Format Python code (ruff)	⏭️
🛡️ mypy — guardrails	Static type check	⏭️
⚓ helm-docs	Regenerate Helm chart README	⏭️
block non-public FE plugins	Block non-public FE plugins	⏭️
🧪 pre-commit wrapper smoke tests	Self-test the wrapper scripts	⏭️
🌐 eslint — frontend	Lint + autofix JS/TS	⏭️
🌐 typecheck — frontend	Whole-project tsc type check	⏭️
📘 eslint — typescript sdk	Lint + autofix JS/TS	⏭️
📘 typecheck — typescript sdk	Whole-project tsc type check	⏭️
⚙️ actionlint — github workflows	Lint GitHub Actions workflows	⏭️
🐳 hadolint — dockerfiles	Lint Dockerfiles	⏭️
🌈 zizmor — github workflows security	Security-scan GitHub Actions workflows	⏭️

[JetoPistola]

👋 Review summary

What looks good

• The root-cause analysis is unusually well-grounded — UnsupportedFeatureException extends LangChain4jException rather than NonRetriableException is exactly why RetryUtils burned the budget, and I confirmed that against the disassembled langchain4j-core:1.18.0 withRetry. The NonRetriableException wrapper is the right lever, and rethrowing bare preserves the cause so the downstream match still works.
• The BaseRedisSubscriber.NON_RETRYABLE_EXCEPTIONS claim holds up: ClientErrorException is in the set and matching is isInstance-based, so BadRequestException genuinely becomes terminal on the first attempt.
• Quantifying the impact with real production counts, and being explicit that this makes the failure honest and cheap rather than absent, is the right way to scope a fix. The "Not fixed here" section naming the ToolChoice.REQUIRED capability gap as a separate concern is exactly the boundary I'd want drawn.
• Test coverage is thorough for the paths it targets — parameterized bare/wrapped cases, a never() assertion that the provider mapper is skipped, and a retry-count guard alongside a transient-still-retries counterpart.
• Honest reporting of what wasn't run, with the reasoning, instead of implying full verification.

Overall
The 400 classification is the right call and the retry fix is solid. One issue to resolve before merge: the streaming catch block also catches BadRequestException, which swallows a real HTTP 400 that propagated before this PR — I verified this differentially against the merge-base, and confirmed nothing upstream validates messages, so Anthropic's inline validateRequest is the only gatekeeper on that path. Details and evidence inline; scoping the catch to UnsupportedFeatureException addresses it without touching the rest.

One smaller thing not worth an inline comment: the buildDetailedErrorMessage(String, Throwable) overload ended up with no caller besides the single-arg version passing one constant, since the unsupported-feature path uses buildUnsupportedFeatureMessage. Safe to inline back.

🤖 Review posted via /review-github-pr

[JetoPistola]

👋 Re-review summary — 4cab848 → 697fb61

Both findings from my earlier review are resolved. Thanks for re-verifying the chain independently rather than taking it on trust, and for correcting your own earlier note on the thread — the table you posted checking Anthropic's inline throw, the resource's single guard, @Valid being inert, and the normalizer no-op matches what I found.

What changed

Commit	What
850ed8e	Narrowed the catch to UnsupportedFeatureException; comment now states why BadRequestException is deliberately excluded
d004d96	Inlined the dead buildDetailedErrorMessage overload
697fb61	New resource-layer test — the streaming twin of the existing empty-messages guard, asserting a real HTTP 400

Verification I ran (isolated worktree)

• Unit tests: 25/25 pass — 14 in UnsupportedFeatureHandling.
• Full test-compile: clean.
• The probe from my original finding now passes. Same test and mock across all three revisions, only ChatCompletionService differing:

	thrown	propagates as HTTP 400
pre-PR baseline	BadRequestException: messages cannot be empty	true
4cab848	null	false
697fb61	BadRequestException: messages cannot be empty	true

• Validated the new integration test's premise statically. It manufactures ChatCompletionRequest.Builder.class, and the registered Podam manufacturer only covers ChatCompletionRequest.class — so I probed the Builder path directly: messages = null, stream = true. CollectionUtils.isEmpty(null) is true, so Anthropic's validateRequest throws as the test expects.

Worth calling out: the old unit test whenGenerateStreamThrowsBadRequest__thenErrorDeliveredInStream had been asserting the buggy behavior. Inverting it into __thenPropagates rather than deleting it is the right call — leaving it would have locked the bug in.

No new findings. One thing I checked and am not raising as a defect: a wrapped synchronous UnsupportedFeatureException would now escape as a 500 rather than streaming, and there's no test for it. But it has no producer today — neither LlmProviderOpenAiResponses nor OpenAiStreamingHelper contains a throw new or catch in its streaming path, so your "these arrive unwrapped" is accurate for current code. Latent risk if a provider changes, not a problem now.

Caveat on my own verification: Docker was unavailable in my environment, so I could not execute ChatCompletionsResourceTest — including the new guard in 697fb61. Its premise checks out statically, but that it passes is a prediction, not a result. CI is the authority there.

This is not an approval — a human reviewer should still approve.

🤖 Review posted via /review-github-pr

[JetoPistola]

Following up to close the caveat in my re-review above.

I noted I couldn't execute ChatCompletionsResourceTest locally (no Docker), so the new streaming empty-messages guard in 697fb61 was verified only statically — a prediction, not a result. That's now resolved by CI: all 71 checks pass on 697fb61, including all 16 Backend Tests - Integration groups, which is where that test runs.

So the guard is confirmed green on the exact SHA, and the static Podam Builder check I described (messages = null, stream = true) matched real behavior. Nothing outstanding from my side.

🤖 Review posted via /review-github-pr

[JetoPistola]

Verified the streaming 400 regression is fixed at 697fb61.

The narrowed catch (UnsupportedFeatureException only) restores the real HTTP 400 for Anthropic's inline empty-messages validation, confirmed differentially against the merge-base and 4cab848. Unit tests 25/25 locally, and all 71 CI checks pass on this SHA including the Backend Integration groups covering the new resource-layer guard.