CAMEL-23394: Limit OpenAI conversation history size#24901
CAMEL-23394: Limit OpenAI conversation history size#24901atiaomar1978-hub wants to merge 7 commits into
Conversation
Add maxHistoryMessages and maxHistoryTokens options to trim exchange-scoped conversation history before it exceeds model context limits.
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
Claude Code review on behalf of gnodet — AI-generated review
CAMEL-23394: Limit OpenAI conversation history size
Verdict: APPROVE ✅
Addresses a real problem — with conversationMemory=true, the conversation history in CamelOpenAIConversationHistory grows without bound, eventually exceeding the model's context window and causing API errors.
What's good
-
Clean design:
OpenAIConversationHistoryTrimmeris a well-structured utility —final class, private constructor, static methods, DEBUG logging when messages are dropped. Token estimation using chars ÷ 4 is a reasonable and widely-used heuristic. -
Correct trim ordering: message limit applied first (O(1) sublist), then token limit loop. This minimizes the work for token estimation since the list is already smaller.
-
Comprehensive token estimation: counts text content, tool-call function names, tool-call arguments, tool-result content, and multi-part content arrays. The ceiling division
(chars + CHARS_PER_TOKEN - 1) / CHARS_PER_TOKENis correct. -
Dual integration points: trimming applied both when reading history (in
addConversationHistory) and when writing it (in all threeupdateConversationHistoryoverloads +OpenAIToolExecutionProducer). The exchange property is kept in sync, so subsequent calls in the same route see the trimmed state. -
Strong test coverage: 5 unit tests on the trimmer covering no-limit passthrough, message cap, token cap, combined limits, and tool-call argument counting. The integration test (
OpenAIConversationHistorySizeTest) verifies end-to-end with a 4-turn mock conversation, asserting both the HTTP request payload and the exchange property.
Non-blocking observations
-
Tool-call / tool-result pair splitting: the sliding window drops oldest messages without regard to message-type boundaries. With aggressive limits (e.g.,
maxHistoryMessages=2in a tool-using conversation), a tool-result message could be retained without the corresponding assistant tool-call message, which the OpenAI API rejects with an error about missingtool_call_idreferences. This is an edge case users would need to configure around, and adding boundary awareness would significantly complicate the trimmer — fine for a follow-up if needed. -
ArrayList.remove(0)in a loop: the token-trimming while loop callstrimmed.remove(0)which is O(n) per removal, making the worst case O(n²). For practical conversation history sizes (dozens to low hundreds of messages) this is negligible, but aLinkedListor tracking a start index would make it O(n) if this ever matters. -
Image content parts: multi-modal user messages with image parts contribute 0 to the token estimate (only text parts are counted). This means the estimate will undercount for vision-enabled conversations. Acceptable since image token costs are model-dependent and hard to estimate without API metadata.
Well-implemented feature with good test coverage. Nice work! 👏
…imate Drop oldest whole segments to keep assistant tool-call blocks paired with tool results, use O(n) segment-based token trimming, and count image payload size in the token heuristic.
|
All three review points are addressed in commit 1. Tool-call / tool-result pairingTrimmer now works on segments:
New tests: 2. O(n²)
|
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
Claude Code review on behalf of gnodet — AI-generated review
Re-review: 2nd commit (84b52e74) — Segment-based trimming + improved token estimate
Great follow-up that addresses all three observations from my initial review:
-
Tool-call / tool-result pair splitting → fixed: The new
Segmentabstraction groups assistant tool-call messages with their subsequent tool-result messages, so trimming operates on segments rather than individual messages.buildSegments()correctly scans for assistant-with-tool-calls followed by consecutive tool messages.findFirstSegmentForMessageLimit()walks backward from the most recent segment, accumulating counts — clean approach. The description now documents that "slightly more thanmaxHistoryMessagesmay be retained to preserve tool result pairing." -
O(n²)
ArrayList.remove(0)→ fixed: Token-limit trimming now scans forward withfindFirstSegmentForTokenLimit()using index-basedestimateTokens(history, from, to)instead of head-removal. No more repeated shifting. -
Image content parts → fixed:
estimateContentPartChars()now counts bothtext()length andimageUrl().url()length (which includes base64 data URIs). Good improvement for multi-modal conversations.
The three new tests cover the key scenarios well — segment integrity with message limits, segment retention, and image payload in token estimates. The existing tool-call test correctly updated to assertThat(trimmed).isEmpty() since the safety check now clears the list when a single oversized segment remains.
Original APPROVE stands — these changes strengthen the PR. 👍
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
|
@davsclaus I'm working on the issue. |
Commit generated catalog, component schema, and endpoint DSL artifacts for maxHistoryMessages and maxHistoryTokens so CI clean-tree check passes. Co-Authored-By: Cursor <cursoragent@cursor.com>
|
Tests passed. Module:
BUILD SUCCESS (~1 min) |
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
Re-review after 4th commit (5110d97): Generated metadata regeneration for history size options — standard regeneration of catalog JSON and Endpoint DSL builder. My earlier approval stands.
Claude Code on behalf of gnodet — AI-generated review
|
🧪 CI tested the following changed modules:
🔬 Scalpel shadow comparison — Scalpel: 11 tested, 27 compile-only — current: 9 all testedMaveniverse Scalpel detected 38 affected modules (current approach: 9).
|
|
merge conflicts |
|
Should I resolve it ? @davsclaus |
|
yes |
|
@davsclaus Done , could you please trigger the build to make sure all good ? |
|
camel-openai test results (
OpenAIClientConfigurationTest includes the |
oscerd
left a comment
oscerd
left a comment
There was a problem hiding this comment.
Thanks for this — bounded conversation memory is a genuinely useful addition, and the segment-based rework in the second commit is the right design. I want to call out specifically that it gets the hard part right: buildSegments groups an assistant-with-tool_calls together with its following tool replies, and trimming removes only whole segments from the front, so an orphaned tool message can never be exposed to the API. The system/developer messages are also safe, since they are built fresh into messages before the history is appended and never live in the history property. Those are exactly the two ways this feature usually breaks, and both are handled.
Unfortunately I think there is a reachable runtime failure that needs fixing first.
Blocking: trim() can return an immutable list that later gets mutated
OpenAIConversationHistoryTrimmer.java:70 and :76 both assign trimmed = List.of(). That immutable instance is then stored into the exchange property at OpenAIProducer.java:282, :734, :768, :802 and in OpenAIToolExecutionProducer.
The problem is that every reader mutates the stored list in place. AbstractExchange.getProperty hands back the same instance with no defensive copy — core/camel-support/src/main/java/org/apache/camel/support/AbstractExchange.java:207-209 short-circuits on type.isInstance(value), and List.class.isInstance(List.of()) is true. So on the next call updateConversationHistory reads it, passes the null guard, and hits OpenAIProducer.java:703 (history.add(pendingUserMessage)) or :733 (history.add(assistantMessage)) → UnsupportedOperationException. The same applies to the history.addAll(...) at :757 and :792.
The path to List.of() is not exotic. findFirstSegmentForTokenLimit returns segments.size() when even the final segment alone exceeds the budget, which lands on :70. So conversationMemory=true&maxHistoryTokens=1000 plus one ~5 KB tool result (≈1250 estimated tokens) is enough, on any route that makes a second call.
The fix is one character each: new ArrayList<>() instead of List.of() at :70 and :76. It would also be worth returning a mutable empty list rather than the caller's list at :46, so trim never hands back something the caller does not own.
I verified this by reading the sources at PR head a13b6533, not by executing it — but the ownership chain is unambiguous, and I think a route test would confirm it immediately (see below).
Worth reconsidering: an oversized segment wipes the whole history
The two limits behave asymmetrically, and I do not think the token side is what you want:
findFirstSegmentForMessageLimitwalks backwards from the last segment, so the final segment is always retained even if it alone exceedsmaxHistoryMessages. It over-retains, which the option description already discloses ("may retain slightly more than this limit"). Good.findFirstSegmentForTokenLimithas no such floor. If the last segment alone is over budget it returnssegments.size()and everything is dropped, reported only at DEBUG.
That is a silent, total loss of conversation memory triggered by a single large tool result or a base64 image. And it does not even achieve the goal in that case, because the trimmer only bounds history — the oversized current user message is added separately, so the request still overflows the context window. Matching the message-limit semantics (always keep the last segment) and logging at WARN when the limit is unsatisfiable would be more predictable.
Smaller points
- No route-level test for
maxHistoryTokens.OpenAIConversationHistorySizeTestcovers onlymaxHistoryMessages=2. A test with a smallmaxHistoryTokensand two.to("openai:...")hops is what would have surfaced the blocker above, so it is worth adding regardless. intchar accumulator can overflow. The character total accumulates into anint, and base64 image data-URIs are counted in full. Past ~2 GB of accumulated characters it goes negative, the estimate goes negative, and trimming silently switches off — precisely in the pathological case the feature exists for. Alongaccumulator with a clamp would close it.:75-77looks like dead code. When the token path picksfirstSegment, the suffix is already guaranteed within budget; whenMath.maxpicks the message-path index instead, the retained set is a subset of one already within budget. I could not construct an input that reaches the innerList.of(). Harmless, but it reads as a safety net that never fires.- Docs prose.
components/camel-ai/camel-openai/src/main/docs/openai-component.adocstill describes conversation history as unbounded in its "Conversation Memory" section. The options table picks the new options up automatically via the included partial, so this is narrative only.
Checks that passed
Generated files are complete and correct (module openai.json, catalog JSON, OpenAIEndpointConfigurer, OpenAIEndpointUriFactory, endpoint-dsl). Both options carry @Metadata(description=...). Default 0 reproduces the previous behaviour exactly, and negative values are treated as unlimited, so no upgrade-guide entry is needed. No Thread.sleep in the tests, no FQCNs, commit format conforms, and CI is green.
Only the immutable-list issue is blocking from my side — the rest is discussion. Thanks again for the work here, the segment model is a solid foundation.
Reviewed with Claude Code (Opus 4.8) on behalf of Andrea Cosentino (@oscerd). This review was generated by an AI agent and may contain inaccuracies; please verify all suggestions before applying.
Return mutable lists from OpenAIConversationHistoryTrimmer, always retain the last segment for token limits, add route/unit tests, and update docs. Co-Authored-By: Cursor <cursoragent@cursor.com>
|
Applied all review fixes from @oscerd on PR #24901 for CAMEL-23394. Summary: Blocking fix — immutable list
Other review items addressed
Test resultsFull New/updated history tests: 13 total (was 9 before). Files changed
|
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
New commit 5b4f03e68964 reviewed — addresses review feedback with three meaningful fixes plus good test coverage.
Key improvements:
-
Mutable list contract —
List.of()→new ArrayList<>()andhistory.subList(...)→new ArrayList<>(history.subList(...)). Essential fix: callers add subsequent messages to the trimmed list, so immutability would causeUnsupportedOperationExceptionat runtime. -
Always retain the most recent segment — Previously, if the last segment alone exceeded
maxHistoryTokens, the trimmer returned an empty list, effectively wiping the entire conversation. NowfindFirstSegmentForTokenLimitshort-circuits with a WARN log and keeps the most recent segment. This is the right trade-off: a slightly oversized history is far better than a completely amnesia'd conversation. -
Integer overflow protection —
charsaccumulator changed frominttolonginestimateTokens, with aInteger.MAX_VALUEcap intokensFromChars. Prevents silent wraparound with large base64-encoded image payloads.
Tests are thorough:
- New integration test (
OpenAIConversationHistoryTokenLimitTest) with mock server verifying 4-turn trimming end-to-end - Unit tests for mutability, empty-list mutability, and the "last segment exceeds limit" edge case
- Existing test assertion correctly updated to reflect the new "always keep last" semantics
Clean, well-structured commit. LGTM.
Claude Code on behalf of gnodet — AI-generated review
|
@davsclaus could you please trigger the CI build and tests ? |
Sync generated Endpoint DSL javadoc with OpenAIConfiguration metadata so CI sourcecheck passes after the maxHistoryTokens description update. Co-authored-by: Cursor <cursoragent@cursor.com>
4 participants
CAMEL-23394 — Conversation history size limits
Issue: https://issues.apache.org/jira/browse/CAMEL-23394
Branch:
CAMEL-23394-conversation-history-sizeCommit:
f8e2ab40b40Problem
With
conversationMemory=true, history inCamelOpenAIConversationHistorygrew without bound. Long or agentic exchanges could exceed the model context window and cause API errors.Solution
Two new options on
camel-openai:maxHistoryMessages00= unlimited)maxHistoryTokens00= unlimited)System/developer messages are not stored in history (they’re prepended in
buildMessages()), so they’re outside these limits.Implementation
OpenAIConversationHistoryTrimmer— sliding-window trim + DEBUG log when messages are droppedaddConversationHistory) and writing it (OpenAIProducer,OpenAIToolExecutionProducer)openai.json+ endpoint configurer/uri factoryTests (100/100)
OpenAIConversationHistoryTrimmerTest— 5 unit tests (message cap, token cap, both, tool-call args in token estimate, no-op when unset)OpenAIConversationHistorySizeTest— mock multi-turn route;maxHistoryMessages=2drops old turns from requests and exchange property