docs: correct the OOM-arm discharge in RedisStreamCodec — no publish-time guard ships - #8145
Closed
thiagohora wants to merge 1 commit into
Closed
docs: correct the OOM-arm discharge in RedisStreamCodec — no publish-time guard ships#8145thiagohora wants to merge 1 commit into
thiagohora wants to merge 1 commit into
Conversation
…time guard ships Follow-up to review feedback on #8089, which merged before the comment was addressed. The javadoc justified absorbing an OutOfMemoryError by pointing at a publish-time size guard as the control covering the residual risk, naming onlineScoring.dropOversizedPayloads. That key does not exist: a repo-wide grep returns exactly one hit, the sentence itself. It never shipped. #8060 was merged as the Guice/Dropwizard ordering fix that made the codec actually receive maxStringLength; the publisher-side drop guard it originally carried was cut from that PR before merge, under this or any other name. That matters more than a stale reference normally would, because of the job the sentence was doing. The paragraph above it makes the most serious admission in the design -- under real heap pressure this arm absorbs an OOM that was a symptom rather than a cause and keeps consuming, masking it -- and this sentence was its discharge. With no such guard the risk is open, not delegated, and someone auditing the decision later would go looking for a control that was never built. Wording as suggested in review. Comment-only: the OOM-absorbing call itself is unchanged and not in question. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
java
Contributor
📋 PR Linter Failed❌ Invalid Title Format. Your PR title must include a ticket/issue number and may optionally include component tags (
Example: ❌ Missing Section. The description is missing the |
Contributor
⏱️ pre-commit per-hook timing
⏭️ 42 skipped (no matching files changed)
|
Collaborator
|
No test needed here. Comment-only change: the diff is four lines inside the FaultTolerantCodec Javadoc, correcting the claim that the residual OOM risk is handled by a publish-time Advisory, from the QA test radar. Nothing here blocks this PR, and anything it proposes is a draft for review. |
![baz-reviewer[bot]](https://avatars.githubusercontent.com/in/933528?s=60&v=4){kind=small}
thiagohora
added a commit
that referenced
this pull request
Sep 4, 2026
…time guard ships Folded in from #8145 (closed in favour of carrying it here), addressing review feedback on #8089 that landed after that PR merged. The javadoc justified absorbing an OutOfMemoryError by pointing at a publish-time size guard as the control covering the residual risk, naming onlineScoring.dropOversizedPayloads. That key does not exist: a repo-wide grep returns exactly one hit, the sentence itself. It never shipped. #8060 was merged as the Guice/Dropwizard ordering fix that made the codec actually receive maxStringLength; the publisher-side drop guard it originally carried was cut from that PR before merge, under this or any other name. That matters more than a stale reference normally would, because of the job the sentence was doing. The paragraph above it makes the most serious admission in the design -- under real heap pressure this arm absorbs an OOM that was a symptom rather than a cause and keeps consuming, masking it -- and this sentence was its discharge. With no such guard the risk is open, not delegated, and someone auditing the decision later would go looking for a control that was never built. Wording as suggested in review. Comment-only: the OOM-absorbing call itself is unchanged and not in question. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
Author
2 tasks
thiagohora
added a commit
that referenced
this pull request
Sep 4, 2026
…time guard ships Folded in from #8145 (closed in favour of carrying it here), addressing review feedback on #8089 that landed after that PR merged. The javadoc justified absorbing an OutOfMemoryError by pointing at a publish-time size guard as the control covering the residual risk, naming onlineScoring.dropOversizedPayloads. That key does not exist: a repo-wide grep returns exactly one hit, the sentence itself. It never shipped. #8060 was merged as the Guice/Dropwizard ordering fix that made the codec actually receive maxStringLength; the publisher-side drop guard it originally carried was cut from that PR before merge, under this or any other name. That matters more than a stale reference normally would, because of the job the sentence was doing. The paragraph above it makes the most serious admission in the design -- under real heap pressure this arm absorbs an OOM that was a symptom rather than a cause and keeps consuming, masking it -- and this sentence was its discharge. With no such guard the risk is open, not delegated, and someone auditing the decision later would go looking for a control that was never built. Wording as suggested in review. Comment-only: the OOM-absorbing call itself is unchanged and not in question. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
thiagohora
added a commit
that referenced
this pull request
Sep 4, 2026
Per review: andrescrz's feedback was mostly about the retry logic, so the two fixes are being separated rather than reviewed together. Removed from this PR, moved to a follow-up: - ChatCompletionService's status-code retryability split and its tests. Removed entirely, filed as OPIK-8262: - preferRetryable / the sibling-failure aggregation change. The real fix is refactoring the trace-thread scorers to emit per-message ProcessingResults so the base subscriber's existing per-message ack/remove granularity is used, rather than picking a less-bad victim from a collapsed batch. Restored errors.getFirst(), which is what main already did -- the defect pre-dates this PR. Also dropped the RedisStreamCodec javadoc correction that had been folded in from #8145: it belongs to #8089, which is already merged, and does not need to ride along here. What remains is the XAUTOCLAIM cursor fix alone. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
thiagohora
added a commit
that referenced
this pull request
Sep 4, 2026
* [OPIK-8240] [BE] fix: split provider-error retryability by status, resume 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. 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. Carry getNextId() forward, resetting to MIN on Redis's 0-0 end-of-pass reply. A failed scan deliberately does not advance the cursor, so its window is retried rather than skipped. The sentinel is compared numerically: StreamMessageId.MIN/.ALL serialize 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. Addressed from review: - Status precedence was backwards. The provider mappers synthesize a status when they cannot read one off the body (CustomLlmErrorMessage defaults to 400, OpenAiErrorMessage to 500), and preferring the mapped code let a synthetic 400 mask a real upstream 503 -- classifying a transient failure as permanent and dropping it on first delivery. The cause-chain HttpException now wins, with the mapped code as fallback, matching the precedence findProviderHttpStatus already documents for its own chain walk. - Sibling aggregation could discard retryable work. The trace-thread scorers re-emit one error for a message that fans out over many thread ids, and errors.getFirst() made that an arrival-order race. Harmless while every failure was a blanket 500; not once permanent and transient were split, as a ClientErrorException arriving first would ack and remove the entry with its retryable siblings. Both scorers now share representativeError(), which prefers a retryable sibling: a bounded replay is recoverable, silently dropped work is not. - Test-only: unconditional assertion flow in the parameterized status test, real-mapper-path coverage for the precedence rule, display-name wording. Tests: 145 green across the affected suites. Mutation-checked -- collapsing the status split to the whole 4xx family fails 5, removing it fails 7, reverting the cursor to always-MIN fails 3, inverting the status precedence fails 4, reverting aggregation to getFirst() fails 1. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs: correct the OOM-arm discharge in RedisStreamCodec — no publish-time guard ships Folded in from #8145 (closed in favour of carrying it here), addressing review feedback on #8089 that landed after that PR merged. The javadoc justified absorbing an OutOfMemoryError by pointing at a publish-time size guard as the control covering the residual risk, naming onlineScoring.dropOversizedPayloads. That key does not exist: a repo-wide grep returns exactly one hit, the sentence itself. It never shipped. #8060 was merged as the Guice/Dropwizard ordering fix that made the codec actually receive maxStringLength; the publisher-side drop guard it originally carried was cut from that PR before merge, under this or any other name. That matters more than a stale reference normally would, because of the job the sentence was doing. The paragraph above it makes the most serious admission in the design -- under real heap pressure this arm absorbs an OOM that was a symptom rather than a cause and keeps consuming, masking it -- and this sentence was its discharge. With no such guard the risk is open, not delegated, and someone auditing the decision later would go looking for a control that was never built. Wording as suggested in review. Comment-only: the OOM-absorbing call itself is unchanged and not in question. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix: fold sibling failures pairwise instead of collecting them Second review round on OPIK-8240. Fan-out is not chunked at either call site -- a manual evaluation passes every resolved thread id, and the streaming path every thread that closed in the window -- so collectList() could hold every sibling Throwable and its cause chain in memory at once during a provider outage, only to discard all but one. Reduced pairwise instead, keeping a single accumulator, in both trace-thread scorers. It also drops the isEmpty() check and the second pass: an empty Flux reduces to an empty Mono, which is already the no-failures case. Selection semantics are unchanged -- first retryable wins, else first failure. The trade is that the count of failed siblings is no longer recoverable for reporting; nothing reports it today. Tests: the two order-variant cases are consolidated into one @ParameterizedTest driven through Flux.reduce, so they exercise the accumulator the way the scorers actually use it rather than by direct call, and cover the empty sequence. Added a case pinning order-stability across three siblings, which the previous pair could not distinguish -- mutation-checked: always keeping the incumbent fails 2, always preferring a retryable candidate fails 1. Also: cursor test asserts isEqualTo rather than isSameAs. The contract is that the position is carried forward, not the identity of the object carrying it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test: make the no-failures case a plain @test Parameterizing a single fixed case bought nothing and left an unused testName parameter -- an artefact of mechanically applying the same shape as the consolidated ordering cases, which do have something to vary. Inlined as Flux.empty(), which also says what the case is more directly than an empty list threaded through a MethodSource. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test: cover the claim cursor on real redis, drop method-only test classes Review feedback on #8137, from andrescrz. The claim cursor was pinned by mocked XAUTOCLAIM replies plus a class-per-method test of the mapping function. Neither proves the behaviour: the reply those tests assert on is the one the test itself wrote. Redis's own PEL scan is what produces it in production, and its COUNT * 10 examine budget is the whole reason the cursor exists. BaseRedisSubscriberTest now drives a pending list deeper than one scan window against the real container. Restoring StreamMessageId.MIN makes it time out on the tail, which is the regression it is there to catch. The two mocked unit tests it subsumes are gone; the failed-scan one stays, because a container cannot be made to fail one XAUTOCLAIM and succeed on the next. The scoreTrace cases carried a helper that branched on isPermanentFailure to pick its assertions -- a test deriving its expectation from the classifier under test cannot fail when the classifier is wrong. Split into assertNonRetryable / assertRetryable, with each parameterised case running one unconditional flow over rows partitioned by a literal status set. That keeps the no-branching shape an earlier review asked for. Also restores the status assertion the transient case had lost, adds the permanent half of the parseable branch, turns the isPermanentFailure table into input-vs-expected rows, and randomises the request and workspace the helper builds. nextCursor goes private rather than gaining @VisibleForTesting: with its only direct test removed, there is no test for the annotation to document. isPermanentFailure, which is still called directly, gets the annotation in place of its comment. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * narrow to the claim-cursor fix; move retry classification to its own PR Per review: andrescrz's feedback was mostly about the retry logic, so the two fixes are being separated rather than reviewed together. Removed from this PR, moved to a follow-up: - ChatCompletionService's status-code retryability split and its tests. Removed entirely, filed as OPIK-8262: - preferRetryable / the sibling-failure aggregation change. The real fix is refactoring the trace-thread scorers to emit per-message ProcessingResults so the base subscriber's existing per-message ack/remove granularity is used, rather than picking a less-bad victim from a collapsed batch. Restored errors.getFirst(), which is what main already did -- the defect pre-dates this PR. Also dropped the RedisStreamCodec javadoc correction that had been folded in from #8145: it belongs to #8089, which is already merged, and does not need to ride along here. What remains is the XAUTOCLAIM cursor fix alone. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix: reset the claim cursor whenever the consumer group is recreated recoverFromNoGroup is reached from both readMessages and claimPendingMessages, but the cursor reset sat at the claim call site only. The read path is the likelier of the two to notice NOGROUP first, since reads run on every tick that is not a claim tick -- so the common case left the cursor pointing into the pending list of a group that no longer exists. Not self-correcting on a busy stream. A scan starting above the recreated PEL's entries only wraps once it exhausts the list, and entries arriving after the stale position keep giving it work at the high end, so the wrap can be deferred indefinitely while the oldest entries go unexamined -- the exact starvation this PR exists to remove. Reset moved into recoverFromNoGroup so both paths get identical treatment and there is one place that owns it. Regression test covers the read path specifically. Mutation-checked: moving the reset back to the claim site alone fails it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
2 participants
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
Follow-up to review feedback on #8089, which merged before the comment was addressed.
RedisStreamCodec's javadoc justified absorbing anOutOfMemoryErrorby pointing at a publish-time size guard as the control covering the residual risk, namingonlineScoring.dropOversizedPayloads. That key does not exist — a repo-wide grep returns exactly one hit, the sentence itself:It never shipped. #8060 merged as the Guice/Dropwizard ordering fix that made the codec actually receive
maxStringLength; the publisher-side drop guard it originally carried was cut from that PR before merge, under this or any other name.That matters more than a stale reference normally would, because of the job the sentence was doing. The paragraph above it makes the most serious admission in the design — under real heap pressure this arm absorbs an OOM that was a symptom rather than a cause and keeps consuming, masking it — and this sentence was its discharge. With no such guard, that risk is open, not delegated, and someone auditing the decision later would go looking for a control that was never built.
Wording as suggested in review.
Change checklist
Comment-only. No behaviour change: the OOM-absorbing call itself is unchanged and was not in question —
values.yamlships-XX:+UseG1GC -XX:MaxRAMPercentage=80.0with noExitOnOutOfMemoryErroranywhere in the repo, so the process already survives an OOM, and not absorbing it would buy a live pod with a wedged stream.Issues
AI-WATERMARK
AI-WATERMARK: yes
mvn -o compilepasses. Comment-only change.Testing
mvn -o compile -DskipTests— BUILD SUCCESS. No test changes: the diff is a javadoc comment.