fix(core): prevent inflated token counts when merging streaming usage_metadata#35615
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes incorrect (inflated) token usage totals when merging AIMessageChunks produced during streaming with stream_usage=True, where providers emit cumulative usage_metadata in each chunk.
Changes:
- Update
add_ai_message_chunks()to mergeusage_metadataby selecting the last non-null value instead of summing across chunks. - Add a regression test suite covering cumulative streaming usage, all-None usage, and partial usage across chunks.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
libs/core/langchain_core/messages/ai.py |
Changes usage_metadata merge behavior in add_ai_message_chunks() to avoid inflating token totals for cumulative streaming usage. |
libs/core/tests/unit_tests/messages/test_usage_metadata_streaming.py |
Adds regression tests to ensure streaming usage is not summed and that None/partial usage cases behave as intended. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Take the last non-null usage_metadata rather than summing, because | ||
| # streaming providers (e.g. OpenAI with stream_usage=True) send | ||
| # cumulative token counts in each chunk. Summing them would inflate | ||
| # the totals. See: https://github.com/langchain-ai/langchain/issues/31351 | ||
| usage_metadata: UsageMetadata | None = None | ||
| for candidate in reversed((left, *others)): | ||
| if candidate.usage_metadata is not None: | ||
| usage_metadata = candidate.usage_metadata | ||
| break | ||
|
|
There was a problem hiding this comment.
add_ai_message_chunks used to aggregate usage_metadata across chunks via add_usage, but this change now always selects the last non-null value. That is a behavior change that conflicts with existing unit expectations (e.g. libs/core/tests/unit_tests/messages/test_ai.py::test_add_ai_message_chunks_usage currently expects summation) and may break callers/providers that emit per-chunk delta usage. Please either (a) update the existing tests + document this as an intentional behavior change, or (b) keep backwards compatibility by supporting both cumulative and delta strategies (e.g., gate on explicit metadata/flag, or detect and fall back to summation when usage values look like deltas).
| # Take the last non-null usage_metadata rather than summing, because | |
| # streaming providers (e.g. OpenAI with stream_usage=True) send | |
| # cumulative token counts in each chunk. Summing them would inflate | |
| # the totals. See: https://github.com/langchain-ai/langchain/issues/31351 | |
| usage_metadata: UsageMetadata | None = None | |
| for candidate in reversed((left, *others)): | |
| if candidate.usage_metadata is not None: | |
| usage_metadata = candidate.usage_metadata | |
| break | |
| # Support both cumulative and per-chunk (delta) usage aggregation. | |
| # Some streaming providers (e.g. OpenAI with stream_usage=True) send | |
| # cumulative token counts in each chunk, while others may emit per-chunk | |
| # deltas. To preserve backwards compatibility, we: | |
| # - Detect sequences that look cumulative (monotonically non-decreasing | |
| # per key) and take the last such value. | |
| # - Otherwise, fall back to summing per-chunk values. | |
| def _looks_cumulative(usages: list[UsageMetadata]) -> bool: | |
| """Heuristically determine whether a sequence of usage dicts is cumulative. | |
| A sequence is considered cumulative if, for every key, values never | |
| decrease across the sequence and at least one key increases at least once. | |
| """ | |
| if not usages: | |
| return False | |
| last_values: dict[str, int] = {} | |
| seen_increase = False | |
| for usage in usages: | |
| for key, value in usage.items(): | |
| prev = last_values.get(key, 0) | |
| if value < prev: | |
| # Decrease detected -> not cumulative. | |
| return False | |
| if value > prev: | |
| seen_increase = True | |
| last_values[key] = value | |
| return seen_increase | |
| usage_metadata: UsageMetadata | None = None | |
| usage_candidates = [ | |
| msg.usage_metadata | |
| for msg in (left, *others) | |
| if msg.usage_metadata is not None | |
| ] | |
| if usage_candidates: | |
| if _looks_cumulative(usage_candidates): | |
| # Cumulative sequence: last value already contains full usage. | |
| usage_metadata = usage_candidates[-1] | |
| else: | |
| # Delta sequence: sum values per key (backwards compatible behavior). | |
| merged = usage_candidates[0] | |
| for u in usage_candidates[1:]: | |
| merged = cast( | |
| "UsageMetadata", _dict_int_op(merged, u, operator.add) | |
| ) | |
| usage_metadata = merged |
| usage_metadata: UsageMetadata | None = None | ||
| for candidate in reversed((left, *others)): | ||
| if candidate.usage_metadata is not None: | ||
| usage_metadata = candidate.usage_metadata | ||
| break |
There was a problem hiding this comment.
usage_metadata is a mutable dict, and this code assigns it directly from one of the input chunks. That means the merged chunk and the original chunk will share the same dict object, so later mutations to either (even accidental) will affect the other. Consider copying (and copying nested detail dicts as needed) when setting usage_metadata so the merged result is independent, similar to how the previous add_usage path always produced a new object.
| def test_usage_metadata_streaming_not_summed() -> None: | ||
| """Merged usage_metadata should equal the last chunk, not the sum.""" | ||
| chunk1 = AIMessageChunk( | ||
| content="Hello", | ||
| usage_metadata={"input_tokens": 10, "output_tokens": 1, "total_tokens": 11}, | ||
| ) | ||
|
|
||
| chunk2 = AIMessageChunk( | ||
| content=" world", | ||
| usage_metadata={"input_tokens": 10, "output_tokens": 2, "total_tokens": 12}, | ||
| ) | ||
|
|
||
| chunk3 = AIMessageChunk( | ||
| content="!", | ||
| usage_metadata={"input_tokens": 10, "output_tokens": 3, "total_tokens": 13}, | ||
| ) | ||
|
|
||
| merged = add_ai_message_chunks(chunk1, chunk2, chunk3) | ||
|
|
||
| assert merged.usage_metadata is not None | ||
| assert merged.usage_metadata["input_tokens"] == 10 | ||
| assert merged.usage_metadata["output_tokens"] == 3 | ||
| assert merged.usage_metadata["total_tokens"] == 13 |
There was a problem hiding this comment.
This new regression test asserts the 'last chunk wins' semantics, but the repository already has a unit test that asserts add_ai_message_chunks sums usage across chunks (libs/core/tests/unit_tests/messages/test_ai.py::test_add_ai_message_chunks_usage). With the current code changes, that existing test will fail in CI. Please update/replace the older test (or adjust implementation to support both semantics) so the test suite reflects a single intended contract.
Merging this PR will not alter performance
|
|
Eugene Yurtsev (@eyurtsev) all checks clear |
|
ccurme (@ccurme) all checks clear |
|
Hope to merge soon |
| # counts (e.g. OpenAI with stream_usage=True sends running totals in | ||
| # every chunk) vs. independent invocations that should be summed. | ||
| # See: https://github.com/langchain-ai/langchain/issues/31351 | ||
| usages = [m.usage_metadata for m in (left, *others) if m.usage_metadata is not None] |
There was a problem hiding this comment.
I know u have not introduced, these single char variable names 'm' should be Discouraged. i have no clue what is 'm' here and tried to look previous lines but it seems it's there some where.
Also just curious instead of m.usage_metadata is not None, is if m.usage_metadata returns truthy is not enough? or equivalent
| # See: https://github.com/langchain-ai/langchain/issues/31351 | ||
| usages = [m.usage_metadata for m in (left, *others) if m.usage_metadata is not None] | ||
|
|
||
| if not usages: |
There was a problem hiding this comment.
do we get usages is None or probably empty list?. I am not sure when we get 'usages' is empty list. probably if m is empty ?. My concern seems does this if condiiton ever executes? Even then we are reassign usage_metadata to none based on usages which depends on m.usage_metadata. there seems some circling going on which I am not able pin point
| inputs = [u.get("input_tokens", 0) for u in usages] | ||
| totals = [u.get("total_tokens", 0) for u in usages] |
There was a problem hiding this comment.
these single char variable names is not Best practice
| else: | ||
| usage_metadata = usages[0].copy() | ||
| for u in usages[1:]: | ||
| usage_metadata = add_usage(usage_metadata, u) |
There was a problem hiding this comment.
what does usages list contains? is it usage of token or metadata? I would use that name as variable than single character names.
I would put note or TO-DO for future refactoring this file is way too long with almost 900 lines, hard to maintain/readable
| assert merged.usage_metadata is not None | ||
| assert merged.usage_metadata["input_tokens"] == 5 | ||
| assert merged.usage_metadata["output_tokens"] == 7 | ||
| assert merged.usage_metadata["total_tokens"] == 12 |
There was a problem hiding this comment.
just curious what are these numbers meant? 5,7,12 etc.
Fix incorrect token counting when streaming with
stream_usage=True.Root cause:
Streaming providers (e.g. OpenAI) return cumulative token usage in each chunk.
add_ai_message_chunks()previously summed usage_metadata across chunks,which inflated token counts.
Example:
chunk1: input_tokens=10
chunk2: input_tokens=10
chunk3: input_tokens=10
Previous result:
30 input tokens
Correct result:
10 input tokens (last cumulative value)
Fix:
Use the last non-null
usage_metadatawhen merging message chunks.Adds a regression test ensuring streaming token usage is not summed.
Fixes #31351