[OPIK-8262] [BE] fix: classify provider errors by status and stop collapsing trace-thread fan-out

[thiagohora]

Rebased onto main. #8137 has merged (as squash commit df66b23dd4), so this branch was replayed onto main with git rebase --onto origin/main, dropping #8137's now-redundant commits. The PR is one commit over 11 files and touches no file #8137 touched.

Earlier stacking note (obsolete)

Stacked on #8137. This PR is based on thiagohora/OPIK-8240-scoring-retry-split-and-claim-cursor, not main. #8137 (the XAUTOCLAIM cursor fix) also touches BaseRedisSubscriber.java and merges first, so this builds on it rather than duplicating its diff. Review and merge after #8137; GitHub retargets this PR's base to main automatically when #8137 lands, and the diff shrinks to just the changes below.

This PR no longer touches BaseRedisSubscriber.java at all. An earlier revision widened isRetryableException to protected static; split-on-read removed the need for it and the change was reverted, so there is now zero file overlap with #8137.

Details

Two fixes to online scoring, in one PR because the second is only a correctness problem once the first lands.

1. ChatCompletionService.scoreTrace classifies provider errors by status code, not by 4xx/5xx family

scoreTrace answered every provider failure with a blanket InternalServerErrorException. That type is absent from BaseRedisSubscriber.NON_RETRYABLE_EXCEPTIONS, so a request the provider can never accept was replayed maxRetries times, once per pendingMessageDuration, before being retired.

The obvious fix — recover the provider status verbatim, as create() and the streaming handler do — is wrong here, and the old code says so in a comment. NON_RETRYABLE_EXCEPTIONS matches on ClientErrorException, so recovering the status would drop the whole 4xx family on first failure, including 408 and 429, which langchain4j models as RetriableException. So the split is by status code:

Status	Thrown	Subscriber behaviour
400, 401, 403, 404, 413, 422, … (client-error family)	ClientErrorException	acked and removed on the first delivery
408, 425, 429	InternalServerErrorException (500)	retried up to maxRetries
all 5xx, and anything unrecognised	InternalServerErrorException (500)	retried up to maxRetries

408/425/429 say "not now", not "not ever", which is exactly why family is too blunt a discriminator. An unrecognised status defaults to retryable, matching BaseRedisSubscriber's own stance on unknown exception types.

Permanence is decided only from a real wire status. The status comes from the cause-chain HttpException via findProviderHttpStatus. The provider mappers are deliberately not consulted for this decision, not even as a fallback.

This is stronger than the first revision of this PR, and the change came out of review (thanks @baz-reviewer — see the thread on ChatCompletionService.java). That revision fell back to providerError.getCode() when no wire status was found. But the mappers synthesize a code when they cannot parse the body — CustomLlmErrorMessage defaults to 400, OpenAiErrorMessage to 500 — and nothing downstream can tell a genuinely parsed 400 from that default. So any unparseable failure of a CustomLlm provider (connection reset, proxy hiccup, truncated body) would have been classified permanent and dropped on its first delivery. That is silent loss of an evaluation a retry might well have completed — the same synthetic-outranks-reality defect the precedence rule was meant to prevent, just in the other direction.

The asymmetry settles it: needlessly retrying a genuinely permanent error wastes at most maxRetries attempts, whereas dropping an unknown failure loses it forever. So an absent wire status falls through to the retryable 500, matching BaseRedisSubscriber's own "unknown defaults to retryable for safety". The mapper's code still shapes the error message; it just has no say in retryability. Both directions are pinned by tests and mutation-checked below.

2. Trace-thread scorers no longer collapse a fan-out — one thread id per stream entry

OnlineScoringTraceThreadLlmAsJudgeScorer and OnlineScoringTraceThreadUserDefinedMetricPythonScorer each took a message carrying a list of thread ids, fanned out over them, collected every error, and re-emitted one:

.collectList()
.flatMap(errors -> errors.isEmpty() ? Mono.<Void>empty() : Mono.error(errors.getFirst()))

That single error decides the fate of the whole entry, so per-thread outcomes are lost.

Why this is coupled to fix 1, since reviewers asked. Before the status split every provider failure was a blanket 500, so all siblings under an entry were retryable and an arbitrary pick could not pick wrong. After the split, one fan-out can mix a permanent 400 with a transient 429 — and then an arbitrary pick either drops retryable work or replays threads that already succeeded. Shipping the split alone would introduce that regression in both trace-thread scorers. They belong in one change.

The decision, and the thing to review. @andrescrz's note was that "the base subscriber can have granularity to ack and remove only permanent failures, as long as the right processing result is emitted for each message… this is a problem in subscribers where they have an implementation that doesn't leverage the capabilities of the base subscriber and how it was designed to work."

Taken literally, BaseRedisSubscriber's ack/remove granularity is per stream entry. So rather than pick a less-bad victim, OnlineScorePublisher.enqueueThreadMessage now publishes one entry per thread id. Failure granularity then matches ack granularity by construction, the collapse disappears instead of being managed, and a retry re-runs exactly the thread that failed. Two consequences worth naming:

• The stream gets N entries where it had 1 for a batch enqueue. Each carries the same rule code. The streams are capped by streamMaxLen and trimmed non-strictly, so this trades stream volume for correct retry scope. On the fan-in side each entry is smaller, since it carries one thread's worth of work instead of the batch's.
• An empty threadIds list now publishes nothing, where it used to publish one entry whose threadIds violated the record's own @NotEmpty.

Rolling-upgrade handling: split-on-read. During the deploy, entries written by the OLD build still carry several thread ids and a NEW consumer must handle them. It does not score them. It republishes the entry as N single-id entries and lets the original be acked away:

• TraceThreadToScoreLlmAsJudge.threadIds (and the Python equivalent) stays a @NotEmpty List<String>. Narrowing it to a single String would make in-flight entries undecodable in exactly the deploy that ships this.
• A multi-id entry is rewritten verbatim apart from the id list — same rule code, workspace and user — so the migration involves no rule lookup and no evaluator-toggle re-check.
• Each replacement entry then gets its own retry budget and is scored by the ordinary single-id path.

Why this replaced the earlier rule. The first revision scored multi-id entries in place and, on partial failure, surfaced the retryable error in preference to a permanent one. That worked, but it meant maintaining a second set of semantics for the legacy format — a rule for reducing N per-thread outcomes into the one verdict the entry gets, which must mis-serve somebody whenever a permanent failure and a retryable one land together. Split-on-read deletes the question instead of answering it: migrate the entry to the new format, and there is no verdict to pick between siblings, no amplification, and nothing lost.

Ack is implicit, and that is what makes the failure modes right. The scorer returns the republish itself, so BaseRedisSubscriber's normal success/failure handling supplies the ack:

Failure mode	Outcome
Republish fails	the Mono errors, the entry is not acked, it redelivers and the split is retried. Nothing lost.
Republish succeeds, ack/remove fails	the entry redelivers and splits again, so some ids get duplicate single-id entries and are scored twice. Tolerable rather than merely unlikely: feedback_scores is a ReplicatedReplacingMergeTree versioned on last_updated_at (migration 000017), so the second score overwrites rather than accumulating a duplicate row. The cost is wasted provider calls, not wrong data.

On amplification. The concern that prompted dropping the earlier rule is moot on the new path anyway: one thread id per entry means a retry re-runs only that thread and no sibling is ever replayed, and maxRetries caps deliveries per entry regardless.

The branch is temporary. It can only fire for entries written before the deploy that ships OPIK-8262. Once no such entry can be in flight — one full streamMaxLen turnover past the rollout, at the latest — the branch and the List<String> shape of threadIds are both deletable. Said so in the javadoc so a future reader knows.

Shared, not duplicated. Per review, the whole per-entry pipeline (migrate-or-score branching) lives in OnlineScoringBaseScorer.migrateOrScoreThreadIds, taking a per-thread scoring callback and a message-copy function. Message construction and each scorer's own logging stay local. Drift between the two trace-thread handlers is exactly what a migration path must not have.

BaseRedisSubscriber.java is untouched. An earlier revision widened isRetryableException to protected static so the collapse rule could ask the base class which sibling error was retryable. With no verdict to pick, that is no longer needed and the change was reverted — removing this PR's only file overlap with #8137.

Call sites checked for anything assuming one message per rule: ManualEvaluationService (both enqueueThreadMessage sites) and TraceThreadOnlineScorerPublisher. None depended on the message count; their "enqueued N threads" logs now say N entries, one per thread id.

Customer impact

A real customer hit this. Their gateway rejects oversized scoring requests with a plain-text 400 (redaction input text is too large... equal or smaller than: 1000000) — no JSON envelope, so getLlmProviderError cannot parse it and the old code fell through to the blanket 500. It affects roughly 6% of their traces.

The main cost is how long a doomed entry stays resident. At production defaults (pendingMessageDuration 10m, maxRetries 3) a permanently-failing entry is delivered at t≈0, again at t≈10m, again at t≈20m, and only then retired. It occupies the pending list for ~20 minutes instead of clearing on the first attempt. Multiply that by a sustained failure rate and the queue takes far longer to catch up than the failure rate alone would suggest.

And it is worst exactly where it hurts most — the large payloads. Requests that trip a size-based rejection like the redaction limit are by definition the big ones. In the incident that prompted this, judge entries averaged ~12 MB and user-defined-metric entries ~56 MB, against a Redis footprint that reached 19.66 GiB. Those are the entries held ~20 minutes longer than necessary, so the population with the worst memory cost per entry is precisely the population lingering longest. Dropping them on the first attempt cuts both the residency and the peak memory they contribute, and lets the queue drain closer to its real throughput.

Secondary, but real: each doomed request costs 3 calls against the customer's own rate-limited gateway instead of 1. At their volume that wasted load can itself provoke 429s for legitimate traffic, so the retries make the problem worse rather than merely slower.

Stated precisely, and please do not conflate these: this fix does not unblock a stuck queue. A permanent failure was already clearing after 3 attempts. What it removes is queue-catch-up delay, the memory residency of the largest entries, wasted gateway load, and head-of-line occupancy. The separate cursor fix (OPIK-8240, PR #8137) is the one that makes an unreachable backlog reachable. Different defect, different fix.

Change checklist

• User facing
• Documentation update

Behaviour change: a permanently-failing provider response is dropped on its first delivery instead of after ~20 minutes and 3 attempts; a batch thread enqueue becomes N stream entries instead of 1. No new configuration keys.

Issues

• OPIK-8262

AI-WATERMARK

AI-WATERMARK: yes

• Tools: Claude Code
• Model(s): Claude Opus 5 (1M context)
• Scope: the fan-out design and implementation, the tests, the mutation checks, and this description. The retry-classification change was authored in a prior session and carried in here with its review feedback intact.
• Human verification: every result below was produced by running the listed commands and reading the output. The gaps under "Not verified" are stated because they were checked for and not found, not assumed.

Testing

cd apps/opik-backend
mvn -o test-compile -DskipTests                                  # passes
mvn -o -q spotless:apply
mvn -o surefire:test -Dtest='ChatCompletionServiceTest,OnlineScoringTraceThreadLlmAsJudgeScorerTest,\
OnlineScoringTraceThreadUserDefinedMetricPythonScorerTest,OnlineScoringLlmAsJudgeScorerTest,\
OnlineScorePublisherTest,BaseRedisSubscriberUnitTest'
# Tests run: 253, Failures: 0, Errors: 0, Skipped: 0
# (includes the new real-Redis OnlineScorePublisherIntegrationTest)

New coverage:

Test	Asserts
ChatCompletionServiceTest.isPermanentFailure__classifiesTheStatus	the split itself, including the 408/425/429 carve-outs and the family boundary (200, 302, 499)
…scoreTrace__whenPermanentClientError__thenNonRetryable	a plain-text-bodied 400/401/403/404/413/422 becomes ClientErrorException, status carried through
…scoreTrace__whenTransientStatus__thenStaysRetryable	408/425/429/500/502/503 stay a retryable 500
…scoreTrace__whenPermanentGaxStatus__thenNonRetryable	six permanent VertexAI GAX codes map through GAX's own translation and are retired on first delivery
…scoreTrace__whenTransientGaxStatus__thenRetryable	RESOURCE_EXHAUSTED/DEADLINE_EXCEEDED/UNAVAILABLE/INTERNAL/UNKNOWN stay retryable
…scoreTrace__whenGaxSaysRetryable__thenRetryable	GAX's own retryable verdict is never overridden into a drop, incl. ABORTED/CANCELLED
…scoreTrace__whenHttpExceptionAndGaxInSameChain__thenHttpExceptionWins	the GAX addition does not disturb HttpException precedence
…whenPermanentGaxStatus__thenNotRetriedInProcess / …whenTransientGaxStatus__thenStillRetriedInProcess	the fail-fast skips in-process retries for permanent only
…scoreTrace__whenPermanentHttpStatus__thenNotRetriedInProcess	from review — a raw HttpException (not a NonRetriableException) at 400/403/404/413/422 is attempted exactly once, so only the status classification can be stopping it
…scoreTrace__whenTransientHttpStatus__thenStillRetriedInProcess	429/503 still consume the retry budget, so the fix isn't "never retry"
OnlineScorePublisherIntegrationTest (new, real Redis)	N single-id entries actually land on the stream and decode back through the shipped codec — whole-object comparison (plain containsExactlyInAnyOrderElementsOf, per .agents/skills/opik-backend/testing.md) against independently built expectations, so a dropped userName/code/projectId is caught, not just a wrong thread id
…scoreTrace__whenNoWireStatus__thenStaysRetryableWhateverTheMapperSays	new from review — with no wire status, a mapper code of 400/401/403 no longer makes the failure permanent; every mapper-only status stays retryable
…whenTransientWireStatusBehindSyntheticPermanent__thenStaysRetryable	a real 503/429 outranks a synthetic 400
…whenPermanentWireStatusBehindSyntheticTransient__thenStaysNonRetryable	a real 400/403 is still permanent behind a synthetic 500, so the rule is pinned both ways
OnlineScorePublisherTest.ThreadMessageFanOutTests.shouldPublishOne{LlmAsJudge,Python}EntryPerThreadId	N thread ids produce N entries, each carrying exactly one id, none lost
…shouldPublishNothingForAnEmptyThreadIdList	zero ids produce zero entries, not one @NotEmpty-violating entry
OnlineScoringTraceThreadLlmAsJudgeScorerTest.SplitOnReadTests.multiIdEntryIsSplitAndNotScored	a 2-id entry republishes 2 single-id entries preserving rule/project/workspace/user/code, and touches no part of the scoring chain
…failedRepublishIsNotAcked	a failing republish fails the entry, so the base subscriber does not ack and the split is retried
…singleIdEntryIsScoredNotSplit	the ordinary path is unchanged and republishes nothing
…migrationDoesNotLogScoringSuccess (both scorers)	a migrated entry emits no Processed trace thread line and does emit a distinct Migrated 'N' legacy thread ids line
…scoringLogsScoringSuccess	the scoring path still logs its success, so the two outcomes are distinguishable in a log search
OnlineScoringTraceThreadUserDefinedMetricPythonScorerTest.SplitOnReadTests	the same four migration cases against the Python scorer, pinning its own copy/scoring callbacks and metric-code preservation

Mutation checks

Every behavioural claim was broken deliberately, the failure observed, then restored and re-run green.

Mutation	Result
M1. restore the mapper fallback for the retryability decision (the reviewed bug)	3 cases fail — scoreTrace__whenNoWireStatus__thenStaysRetryableWhateverTheMapperSays at 400, 401 and 403, i.e. exactly the statuses CustomLlmErrorMessage can synthesize
M2. multi-id entry scored instead of split	fails multiIdEntryIsSplitAndNotScored and failedRepublishIsNotAcked
M3. multi-id entry acked without republishing	fails multiIdEntryIsSplitAndNotScored and failedRepublishIsNotAcked
M4. isPermanentFailure → return false	14 cases fail across isPermanentFailure__classifiesTheStatus, scoreTrace__whenPermanentClientError__thenNonRetryable, …whenPermanentProviderErrorUnparsed…, …whenPermanentWireStatusBehindSyntheticTransient…
M5. publisher restored to one entry carrying the whole list	fails shouldPublishOneLlmAsJudgeEntryPerThreadId (all 3 rows) and shouldPublishNothingForAnEmptyThreadIdList
M6. scoring-success log moved back onto the returned Mono (the false-success bug)	fails migrationDoesNotLogScoringSuccess and scoringLogsScoringSuccess
M7. migration log line deleted	fails migrationDoesNotLogScoringSuccess in both scorer tests
M8. Python scorer's copy callback drops the metric code	fails multiIdEntryIsSplitAndNotScored (Python) — the callback the shared pipeline cannot verify for itself
V1. GAX branch removed from canonicalStatusOf (reverts the Vertex fix)	7 cases fail across scoreTrace__whenPermanentGaxStatus__thenNonRetryable and …thenNotRetriedInProcess
V2. isRetryable() safety valve dropped	4 cases fail — scoreTrace__whenGaxSaysRetryable__thenRetryable at all four codes
V3. failFastOnPermanentFailure wrapper removed	fails scoreTrace__whenPermanentGaxStatus__thenNotRetriedInProcess
V4. GAX status allowed to outrank a real HttpException	8 cases fail, including scoreTrace__whenHttpExceptionAndGaxInSameChain__thenHttpExceptionWins and two pre-existing create/stream tests — the precedence guard is real
V3 re-run after the review fix	kill count 1 → 6 cases. The original permanent cases used InvalidRequestException, already a NonRetriableException, so they did not discriminate; the new raw-HttpException cases do
I1. publisher back to one entry for the list (integration)	fails enqueueThreadMessageWritesOneEntryPerThreadId against real Redis
F1. publisher's copy constructor drops userName	fails enqueueThreadMessageWritesOneEntryPerThreadId
F2. scorer's singleThreadIdCopy drops userName	fails multiIdEntryIsSplitAndNotScored (unit layer — the integration test builds its own copies, so it does not cover this)
F3. enqueueMessage clobbers workspaceName in transit	fails both integration tests — the field-picking version asserted no workspaceName, so this passed before
P1. toDefinedMetricPython drops the whole code object	fails enqueueThreadMessageWritesOnePythonEntryPerThreadId
P2. toDefinedMetricPython replaces the nested metric string	fails the same test — confirms the assertion reaches inside the nested code object, not just the top-level reference

Not verified

• Full mvn verify (the whole Testcontainers suite) was not run — relying on CI. Only the six classes listed above were executed.
• No test drives a real rolling upgrade. Split-on-read is proved by feeding the new consumer a multi-id message of the shape the old publisher wrote, not by running two builds against one stream. Wire compatibility rests on the fact that no serialized field changed type — threadIds is still a List<String> — which is inspection, not measurement.
• The "republish succeeds, ack fails" path is reasoned, not tested. Its safety rests on the ReplacingMergeTree property below; I did not construct a failing ack against a real stream to watch the duplicate resolve.
• Stream volume and partial enqueue failure — see the dedicated section below. Quantified, but not measured under load and not fully mitigated in this PR.
• The customer figures (6% of traces, ~12 MB / ~56 MB averages, 19.66 GiB) come from the incident report, not from anything reproduced here.
• The "a re-score overwrites rather than duplicates" claim is read off the schema (feedback_scores is a ReplicatedReplacingMergeTree versioned on last_updated_at, migration 000017 line 152), not observed end to end.

Stream volume and partial enqueue failure

Raised in review on OnlineScorePublisher. My first reply conceded more than the code supports; corrected here.

The knobs (config.yml, onlineScoring): streamMaxLen 10,000 (REDIS_SCORING_STREAM_MAX_LEN), streamTrimLimit 100 (REDIS_SCORING_STREAM_TRIM_LIMIT), consumerBatchSize 10. Every XADD goes out as MAXLEN ~ 10000 LIMIT 100 via RedisStreamUtils.buildAddArgs → trimNonStrict(), so trimming is approximate and capped at 100 evictions per XADD.

Trim exposure. Previously one rule's close-batch wrote 1 entry regardless of thread count; now it writes N. A single TraceThreadOnlineScorerPublisher.publish would need on the order of 10,000 sampled threads for one rule to churn the cap alone. The cap is per stream and shared across rules and producers, so concurrent batches lower that bar proportionally. A real ceiling worth knowing about; not one this change puts anyone against today.

Partial enqueue failure — not a new failure class. enqueueMessage is Flux.fromIterable(messages).flatMap(stream::add), i.e. N separate XADDs with partial-failure exposure and no reconciliation. That is already how the main scoring paths work and long predates this PR — every one of these call sites already passes a multi-element list:

• OnlineScoringSamplerSupport.publishSampled (List<?> messages, used by the trace and span samplers)
• TestSuiteAssertionSampler:151
• ManualEvaluationService:240, 264, 330, 353

This PR moves the trace-thread path onto the shape everything else already had.

And on the thread payload it reduces per-incident loss. Before, a failed single XADD lost all N thread ids at once; now a partial failure loses a subset. Same silent-unscored outcome, strictly smaller blast radius. Not overclaiming in the other direction: partial failure on the thread path specifically is newly possible, because it was one XADD before. The point is that it is neither a new class of failure nor a worse outcome.

Nor is it silent. OnlineScorePublisher:161-164 increments online_scoring_enqueue_total with result=error (tagged by evaluator type and workspace) and logs at ERROR on every failed XADD.

Note the split-on-read shim is not exposed to it at all: a partially-failed republish errors, the entry is not acked, it redelivers and splits again, and the duplicates are absorbed by feedback_scores being a ReplicatedReplacingMergeTree.

Still worth a follow-up ticket, scoped as "online-scoring enqueue has no partial-failure reconciliation, across all publishers" — not as something this PR introduced. Doing it properly means per-id reconciliation/an outbox or all-or-nothing enqueueMessage semantics, applying to all seven streams.

Provider retry ordering — investigated, no change needed

Review asked whether the permanent-status conversion happening in the catch block means a permanent 400 is retried by retryPolicy.withRetry before we ever classify it. Investigated against langchain4j 1.9.1 / 1.13.0 sources rather than assumed. It cannot, and the reason is mechanical:

• RetryUtils.RetryPolicy.withRetry catches NonRetriableException and rethrows it immediately without retrying.
• Every provider model wraps its HTTP call in RetryUtils.withRetryMappingExceptions, which is withRetry(() -> ExceptionMapper.DEFAULT.withExceptionMapper(action)) — so the mapping happens inside the model, below our retry policy. Verified in OpenAiChatModel:149, AnthropicChatModel:496, GoogleAiGeminiChatModel:50, and our own OpikOpenAiChatModel:125.
• ExceptionMapper.DefaultExceptionMapper.mapHttpStatusCode converts every HttpException into a typed exception, and that split lines up exactly with isPermanentFailure:

status	mapped to	supertype	retried?	isPermanentFailure
400, 413, 422, other 4xx	InvalidRequestException	NonRetriableException	no	true
401, 403	AuthenticationException	NonRetriableException	no	true
404	ModelNotFoundException	NonRetriableException	no	true
408	TimeoutException	RetriableException	yes	false
429	RateLimitException	RetriableException	yes	false
5xx	InternalServerException	RetriableException	yes	false

So converting inside the retry action would be redundant: everything isPermanentFailure calls permanent is already a NonRetriableException that neither langchain4j's inner retry nor our outer policy will retry. Provider coverage: OpenAI, OpenRouter, CustomLLM, Ollama and FreeModel all use OpikOpenAiChatModel/OpenAiChatModel; Anthropic and Gemini use their own mapped models; VertexAI uses the Google Cloud SDK and never produces an HttpException at all. There is no Bedrock provider in this codebase.

VertexAI — the one real gap, now fixed here

Investigating the above surfaced a genuine hole, and it is now fixed in this PR rather than deferred: it is the same defect class (permanent failures consuming retry budget and holding queue residency) in the same function.

VertexAI raises GAX ApiException, which is neither an HttpException nor a NonRetriableException. So a permanent Vertex failure was retried by langchain4j's inner policy and by ours, then fell through to the retryable 500 and was redelivered by the subscriber.

Signal chosen: StatusCode.Code.getHttpStatusCode(), GAX's own translation — not a hand-written gRPC→HTTP table, and not isRetryable() as the primary. Three reasons:

• It is transport-neutral by construction. GrpcStatusCode and HttpJsonStatusCode both reduce to the same StatusCode.Code enum, so the concern about gRPC vs HTTP-JSON does not arise — there is one code space, and GAX owns the translation.
• It preserves the PR's design: classification stays on a wire-derived status, not a synthetic verdict.
• Dumping the enum confirms it reproduces every mapping suggested in review and covers more: INVALID_ARGUMENT→400, UNAUTHENTICATED→401, PERMISSION_DENIED→403, NOT_FOUND→404, RESOURCE_EXHAUSTED→429, DEADLINE_EXCEEDED→504, UNAVAILABLE→503, plus FAILED_PRECONDITION/OUT_OF_RANGE→400, ALREADY_EXISTS/ABORTED→409, UNIMPLEMENTED→501, CANCELLED→499.

isRetryable() is used, but only as a one-way safety valve. Where the gRPC→HTTP translation disagrees with retry semantics — ABORTED→409 and CANCELLED→499 both look permanent by status but can be worth retrying — an exception GAX marks retryable yields no status and therefore falls through to the retryable 500. It can only ever prevent a drop, never cause one, so it never lets a synthetic verdict override a real wire status.

Precedence is untouched: HttpException is still searched across the whole chain before any typed exception, GAX included. Pinned by scoreTrace__whenHttpExceptionAndGaxInSameChain__thenHttpExceptionWins and by mutation V4.

Also fails fast in-process. A permanent status now short-circuits inside the retry action via failFastOnPermanentFailure, mirroring the existing failFastOnUnsupportedFeature. Scoped to scoreTrace only — create() answers an HTTP caller and narrowing its retry behaviour is not this change's business.

No build widening. GAX is already compile-scope through the directly-declared google-cloud-vertexai:1.52.0 (com.google.api:gax:2.76.0), so the import compiles with no pom change; I did not add a direct dependency, and class-name matching was therefore unnecessary. Worth a reviewer's eye: this is the first direct GAX import in opik-backend, and it puts a provider-specific type in domain.llm. I think it is justified — canonicalStatusOf already knows langchain4j's exception zoo and its entire job is "recover a status from whatever the provider threw" — but it is a coupling call, not a mechanical one.

Review follow-ups in this revision

Finding	Action
Mapper's synthetic 400 could make an unknown failure permanent	Fixed. Permanence now requires a real wire status; one test rewritten, mutation-checked (M1)
Unbounded XADDs from expanding threadIds	Answered with numbers above; follow-up ticket proposed, not fixed here
Duplicate pipeline across the two trace-thread scorers	Fixed. Extracted to OnlineScoringBaseScorer.migrateOrScoreThreadIds with a per-thread callback
TRANSIENT_CLIENT_ERRORS mixes a jakarta constant with bare literals	Fixed, then corrected in round two — see below
Thread-id collection interpolated twice in logs	Fixed. Both logs now report size plus a bounded 10-id sample with an omitted count
Malformed javadoc on isRetryableException	Moot. The whole visibility change was reverted; BaseRedisSubscriber.java is untouched

Second review round

Finding	Action
scoreOneThreadIdPerEntry promises scoring but the multi-id path only republishes	Fixed. Renamed migrateOrScoreThreadIds; javadoc names both branches and warns that completion does not mean "scored"
Permanent HTTP errors retried before classification	Investigated, no change. Mechanically impossible on every provider path — see the section above. One unrelated pre-existing VertexAI gap surfaced and flagged
Python scorer untested through the shared migration pipeline	Fixed. Four cases added mirroring the LLM scorer, including metric-code preservation (mutation M8)
Migration logs falsely claim scoring completed	Fixed. Success log moved inside the scoring callback; migration emits a distinct Migrated 'N' legacy thread ids …; no scoring performed line. Tested both directions, mutations M6/M7
(self-correction) my earlier reply said jakarta ships no constant for 429	Wrong, fixed. Response.Status.TOO_MANY_REQUESTS exists in jakarta.ws.rs 4.0.0 and is now used directly. Only 425 needs a named constant; javadoc corrected
(self-correction) my earlier reply called unreconciled partial enqueue loss "new for the trace-thread path"	Overstated, corrected in the section above — the shape predates this PR at six call sites, and per-incident loss on the thread path shrinks from all-N to a subset

Third review round

Finding	Action
Orphaned retrieveFullThreadContext javadoc	Fixed. I had inserted the helper between that javadoc and its method; helper moved below
Unquoted sampled value in the enqueue logs	Fixed. My own inconsistency — both templates now quote the sample like every other value in the file
408/425/429 rationale detached from TRANSIENT_CLIENT_ERRORS	Fixed, and writing it out caught an error: langchain4j does not model 425 as retriable — it has no 425 case, so it falls through to a non-retriable InvalidRequestException. We diverge deliberately per RFC 8470; harmless because retryability reads the wire status, not the mapped type
Null thread ids reach scoring	Pushed back. Not reachable: every producer builds the list via List.copyOf or List.of, both of which reject null elements, and these ids never cross a user-facing DTO boundary. Declined to add unreachable validation
Raw thread ids reach application logs	Pushed back. Not new — the line replaced logged the entire collection; the new one caps at 10 + a count, so exposure strictly decreases. INFO-level thread-id logging is established at TracesResource:815/866, TraceThreadListener:126, OnlineScoringBaseScorer:212. Redaction is a codebase-wide policy call, not this PR's scope
Comment density too high	Trimmed across every comment this PR adds or touches: net −114 lines of prose, no behaviour change, suite still green

Fourth review round

Finding	Action
Permanent-HTTP cases don't discriminate (they'd pass with the fail-fast deleted)	Accepted, fixed. They used InvalidRequestException, already non-retriable. Added raw-HttpException cases asserting exactly-once. Mutation V3 kill count 1 → 6
Mapper ignored; Gemini / OpenAI Responses statuses can be missed	Investigated, no change. Gemini always yields a wire status via withRetryMappingExceptions. OpenAI Responses is unreachable from scoreTrace — getLanguageModel returns the langchain4j model unconditionally; the Responses SDK is only behind getService(), used solely for getLlmProviderError. A mapper fallback stays refused: CustomLlmErrorMessage returns DEFAULT_STATUS=400 on every branch and nothing distinguishes parsed from defaulted
Mapper's parsed message discarded	No change. The raw root-cause message is the response body the mapper slices its message out of — the parsed text is a substring, minus code/status. Switching would reduce the diagnostic
Upstream error details leak to clients	Pushed back. Carries the provider's own error text, never stack frames or headers. scoreTrace has no HTTP caller: it reaches the app log and the workspace's own evaluator log (ClickHouseAppender). Also unchanged by this PR — only a new call site
Subscriber-path integration coverage	Split, and accepted by the reviewer. Added OnlineScorePublisherIntegrationTest (real Redis, mutation-checked) for the cross-layer half AGENTS.md:45 actually calls for. Declined subscriber ACK/XDEL/redelivery: that is BaseRedisSubscriber's logic, untouched here and already covered by BaseRedisSubscriberTest
Migration can silently drop payload fields (on the new integration test)	Accepted, fixed. Both integration tests now compare whole decoded objects instead of picking fields. Mutations F1/F2/F3 above pin the three layers: scorer copy construction, publisher copy construction, and in-transit/codec fidelity
Use plain element equality, not usingRecursiveFieldByFieldElementComparator	Accepted. Verified every payload type is a record (and the one JsonNode leaf has deep equals), so the comparator was redundant. Switched to plain containsExactlyInAnyOrderElementsOf; re-ran F1 and F3 and both kills survive unchanged, so no coverage was traded for the simplification
Real-Redis test covers only the LLM stream; Python codec regressions undetected	Accepted, fixed. The PR changes the fan-out for both payload types but verified only one end-to-end — the same drift mutation M8 exposed at the unit layer, one layer down. Added a mirrored Python test through its own real stream; mutations P1/P2 above confirm it catches a lost code.metric

CI note: Python SDK E2E Tests 3.10 failed once on commit 781e292a. Not this change — the same commit also has a passing run of that workflow, the failure was a 60s container health timeout (running but not healthy (health=starting), no crash), and the uploaded backend log artifact is empty. Verified locally that Guice wiring is intact by booting the full Dropwizard app: TraceThreadOnlineScoringSamplerListenerIntegrationTest passes 6/6 with MySQL + ClickHouse + Redis containers, which would fail outright on a circular dependency from injecting OnlineScorePublisher into the two @EagerSingleton scorers.

Documentation

No documentation change. No new configuration keys.

🤖 Generated with Claude Code

[github-actions]

⏱️ pre-commit per-hook timing

Hook	Description	Result	Duration
🌐 typecheck — frontend	Whole-project tsc type check	✅	35.33s
🌐 eslint — frontend	Lint + autofix JS/TS	✅	6.72s
☕ spotless — java backend	Format Java code	✅	5.49s
🛡️ semgrep — java backend sql	Block SQL injection-prone string formatting	✅	2.03s
Total (4 ran)			49.57s

⏭️ 40 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	⏭️
🧪 rebaseline script tests	Self-test the changelog re-baseline script	⏭️
📘 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	⏭️

[CometActions]

This change looks worth a test.

Both halves land on online scoring and the e2e estate asserts neither. On the classification: online-evaluation-python-metric-errors.spec.ts already proves a python evaluator 400 is terminal and called exactly once, but it never reaches ChatCompletionService — a provider 401 that kept burning onlineScoring.maxRetries would pass it unchanged. On the fan-out: no spec drives a thread-scoped rule at all (rule-scope-thread-span is the taxonomy's own open gap), so if the per-thread split lost an id, or the >1 branch migrated an entry it should have scored, threads would go silently unscored. Both look reachable hermetically: the suite's mock gateway already answers 401 with per-model counters, and POST /v1/private/manual-evaluation/threads enqueues N thread ids on demand, so neither needs a real provider key. The legacy multi-id migrate branch is not reachable on a fresh install and is out of scope for a permanent test.

Would target online-evaluation.llm-judge-scores, online-evaluation.rule-scope-thread-span.

What it would check

1. Register a Custom provider pointed at the suite's mock gateway (mockGatewayUrlForBackend) with a bogus static key, so every chat call answers 401, and create an LLM-judge rule bound to it
2. Seed one trace, then read the rule's log stream and the mock's chat_refused_unknown: counter: the provider must be called once and the failure reported once, not onlineScoring.maxRetries times
3. Seed 3 threads in one project, create a thread-scoped rule against the mock gateway, and POST /v1/private/manual-evaluation/threads with all three ids
4. Confirm all three threads carry the rule's score (the split enqueues one entry per id and drops none) and the mock saw three chat calls, not one
5. Check the thread panel's Feedback scores tab renders those scores, and that the rule log reports per thread rather than one batch line

Deploying a test environment for this PR and exploring it — results will follow in a comment.

Not testable yet. The complement of the classification — 408/425/429 staying retryable — is the direction that loses evaluations if it is wrong (a rate-limited judge acked and dropped instead of retried), but the mock gateway only ever answers 401, so no transient provider status can be provoked on this estate. The VertexAI/GAX branch is separately unreachable without Google Cloud credentials.

also touches Backend (Java API / internal)

Run

Advisory, from the QA test radar. Nothing here blocks this PR, and anything it proposes is a draft for review.

Re-checked after a push on 04 Sep 17:35 UTC.

[github-actions]

🔄 Test environment deployment process has started

Phase 1: Deploying base version 2.2.53-6551 (from main branch) if environment doesn't exist
Phase 2: Building new images from PR branch thiagohora/OPIK-8262-retry-classification-and-fanout
Phase 3: Will deploy newly built version after build completes

You can monitor the progress here.

[CometActions]

✅ Test environment is now available!

To configure additional Environment variables for your environment, run [Deploy Opik AdHoc Environment workflow] (https://github.com/comet-ml/comet-deployment/actions/workflows/deploy_opik_adhoc_env.yaml)

Access Information

• URL: https://pr-8162.dev.comet.com
• Cluster: comet-ml-development
• Namespace: pr-8162
• Version: 2.2.53-8162-merge-3189
• Application logs: View in Grafana

The deployment has completed successfully and the version has been verified.

[CometActions]

Explored this PR on its own test environment.

Worked all 5 items on the triage explore list, plus the also item triage had marked blocked-by-platform (I found a way to reach it — see item 6). 6 checked: 5 work, 1 works with a cosmetic caveat. 0 suspicious, 0 blocked.

Per-item results

#	Item	Verdict	What I saw
1	Provider bound to a permanently-failing endpoint; LLM-judge rule created against it	works	Custom-LLM provider at api.openai.com/v1 with a bogus key → wire 401 on every chat call. Rule created and fired.
2	A permanent provider failure is attempted once, not maxRetries times	works	1 provider call, 1 terminal ERROR. Held over 30 minutes / 3 full redelivery windows — zero redeliveries.
3	3 threads seeded, thread-scoped rule, POST /manual-evaluation/threads with all 3 ids	works	202 {"entities_queued":3,"rules_applied":1}; the split enqueued one entry per id.
4	All 3 threads carry the rule's score; the evaluator saw 3 calls, not 1	works	3 threads × 1 score = 1.0, source: online_scoring. Exactly 3 evaluator calls — not 1, and not 9.
5	Thread panel Feedback scores tab renders them; rule log reports per thread	works	Panel shows "1 scores" and the score row. Log stream is 3 independent per-thread cycles, no batch line.
6	(triage also, marked blocked) A transient 4xx stays retryable	works	429 endpoint → redelivered at exactly 10-min intervals, 3 attempts. The direction that loses evaluations if wrong.
Rule	Wire status	Seeded	Provider-call lines in the rule log
---	---	---	---
…-permfail-judge	401 (permanent)	17:42:28	[17:42:28] — 1, after 30 min
…-transient-judge	429 (transient)	17:50:28	[17:50:28, 18:00:45, 18:10:45] — 3, at 10-min intervals

2 flows look worth a permanent test:

• online-evaluation.automation-logs — A permanent provider failure is attempted once; a transient one is retried
• online-evaluation.rule-scope-thread-span — A thread-scoped rule run over N threads scores every one of them, exactly once

Writing the spec now; a draft PR will follow.

Test env · Run

Advisory, from the QA test radar. Nothing here blocks this PR, and anything it proposes is a draft for review.

[CometActions]

Proposed a permanent test for this change.

Test proposal — opik PR #8162 (OPIK-8262)

Draft PR: #8167

Draft, no reviewers requested, based on thiagohora/OPIK-8262-retry-classification-and-fanout
(the source PR's own head branch, not main) — the spec guards behaviour this PR introduces, so it
sits on top of the change it tests. Written and run against
c20c9e018a23b9fc45927d0224c80959cc7208ae.

Specs proposed: 1

Spec	Capability	Verification
tests/online-evaluation/online-evaluation-thread-fanout.spec.ts — a manual evaluation of 3 threads against one thread-scoped rule produces exactly one evaluator call per thread, one score on each, and renders it in each thread's panel	@t2-cuj @cap:online-evaluation.rule-scope-thread-span	PASSED against https://pr-8162.dev.comet.com (1 passed, 24.0s). A deliberate mutation of the evaluator-call count also failed as intended, so the assertion is known to discriminate.

Supporting changes in the same PR: a threadCohort fixture, a thread-scoped constant-score python
metric builder, backend-client support for trace_thread_user_defined_metric_python rules and
POST /v1/private/manual-evaluation/threads, and the taxonomy update
(rule-scope-thread-span → covered: true, tier: t2-cuj, noted as thread scope only;
automation-logs deliberately left covered: false, since the spec reads the log stream over the
API and never opens the page that key names).

tag_lint.py: 60 specs checked, 1 exempt, 0 problem(s).

Candidates dropped: 1

"A permanent provider failure is attempted once; a transient one is retried" — the
retry-classification half of the PR. Strong candidate, verified by hand during exploration, but not
writable as a spec this suite should own right now:

• It needs a backend-reachable endpoint that answers a chosen HTTP status. The suite's only such
  facility (services/mock-token-auth/) answers 401 only and, per mockAuthSkipReason(), is
  reachable only from a local backend — never from the deployment where this behaviour is
  observable. The exploration's substitute, httpbingo.org, is a third-party service and is ruled
  out by the estate's determinism rule.
• Even with that endpoint, the discriminating assertion costs a full
  onlineScoring.pendingMessageDuration — 10 minutes, deployment config, not settable from a test.
  Asserting only the permanent half would pass equally well if every provider error had been made
  non-retryable, which is the direction that silently loses evaluations.

The prerequisite is small — teach mock_token_auth_service.py to answer a caller-chosen status,
then write the spec on a local OSS run where REDIS_SCORING_PENDING_MESSAGE_DURATION can be turned
down. That needs a local OSS backend to verify against, which this flow did not have.

Incidental finding, not part of the PR

Running the whole tests/online-evaluation/ directory (because shared files were touched) gave
7 passed, 1 skipped, 1 failed. The failure is the pre-existing
online-evaluation-python-metric-errors.spec.ts, which fails identically with these changes
stashed: on this deployment a python metric that exits 0 without a result line still reports
Python evaluation failed (HTTP '500'): 500 Internal Server Error: Failed to execute code, where
the spec expects the classified 400 Bad Request: Execution failed: the metric produced no output.
Either the classification fix is absent from this build's python backend or it has regressed —
worth a look, unrelated to this proposal.

Separately, npx tsc --noEmit does not run on this branch and did not before the change:
tsconfig.json still sets baseUrl, which the pinned typescript@^7.0.2 removed. Typechecking
with TypeScript 5.9 instead reports one pre-existing error (a duplicate deleteDashboard in
core/backend/client.ts) and nothing new from this change.

Run

Advisory, from the QA test radar. Nothing here blocks this PR, and anything it proposes is a draft for review.