-
-
Notifications
You must be signed in to change notification settings - Fork 5.7k
fix(proxy): avoid in-place mutation in SpendUpdateQueue aggregation #20876
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
fix(proxy): avoid in-place mutation in SpendUpdateQueue aggregation #20876
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR fixes a critical bug in the SpendUpdateQueue aggregation logic where the _get_aggregated_spend_update_queue_item() method was modifying original dictionary objects in place. The bug occurred because when multiple spend updates with the same entity key were aggregated, the code stored a direct reference to the first update's dictionary and then mutated it during aggregation, corrupting the original caller-owned data.
Changes:
- Fixed in-place mutation by creating a defensive copy of the first update dictionary before storing it in the aggregation map
- Added a regression test to verify that original update dictionaries remain unchanged after aggregation
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
litellm/proxy/db/db_transaction_queue/spend_update_queue.py |
Added .copy() call on line 113 to create a defensive copy of update dictionaries before storing them in the aggregation map, preventing in-place mutation of caller-owned objects |
tests/test_litellm/proxy/db/db_transaction_queue/test_spend_update_queue.py |
Added regression test test_get_aggregated_spend_update_queue_item_does_not_mutate_original_updates that verifies original updates remain unchanged, aggregation is correct, and no object identity is shared |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| [original_update, duplicate_key_update] | ||
| ) | ||
|
|
||
| assert original_update["response_cost"] == 10.0 |
Copilot
AI
Feb 10, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider also verifying that duplicate_key_update is not mutated to make the test more comprehensive. While the current implementation doesn't mutate it (only the copied first update is mutated), adding an assertion like assert duplicate_key_update["response_cost"] == 20.0 would make the test more robust against future refactoring.
Greptile OverviewGreptile SummaryThis PR fixes a correctness bug in A regression test was added to ensure the original update object remains unchanged, the aggregated total is correct, and the aggregated item is a different instance. Confidence Score: 4/5
|
| Filename | Overview |
|---|---|
| litellm/proxy/db/db_transaction_queue/spend_update_queue.py | Fixes in-place mutation during queue aggregation by copying the first seen update dict before summing response_cost; prevents caller-owned dicts from being modified. |
| tests/test_litellm/proxy/db/db_transaction_queue/test_spend_update_queue.py | Adds regression test ensuring _get_aggregated_spend_update_queue_item() doesn’t mutate original updates; test currently assumes deterministic ordering of aggregated list output. |
Sequence Diagram
sequenceDiagram
participant Caller
participant SpendUpdateQueue
participant InMemMap as AggregationMap
participant AggList as AggregatedList
Caller->>SpendUpdateQueue: add_update(update)
SpendUpdateQueue->>SpendUpdateQueue: update_queue.put(update)
Caller->>SpendUpdateQueue: aggregate_queue_updates()
SpendUpdateQueue->>SpendUpdateQueue: flush_all_updates_from_in_memory_queue()
SpendUpdateQueue->>SpendUpdateQueue: _get_aggregated_spend_update_queue_item(updates)
loop each update
SpendUpdateQueue->>InMemMap: compute aggregation key
alt first update for key
SpendUpdateQueue->>InMemMap: store shallow copy of update
else later update for key
SpendUpdateQueue->>InMemMap: increment stored response_cost
end
end
SpendUpdateQueue->>AggList: append values from map
SpendUpdateQueue-->>Caller: return aggregated list
Caller->>SpendUpdateQueue: get_aggregated_db_spend_update_transactions(aggregated)
SpendUpdateQueue-->>Caller: DBSpendUpdateTransactions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2 files reviewed, 1 comment
tests/test_litellm/proxy/db/db_transaction_queue/test_spend_update_queue.py
Outdated
Show resolved
Hide resolved
|
@greptile |
Greptile OverviewGreptile SummaryThis PR fixes a correctness issue in spend update queue aggregation where the first Change is localized to the proxy’s in-memory spend update batching ( Confidence Score: 5/5
|
| Filename | Overview |
|---|---|
| litellm/proxy/db/db_transaction_queue/spend_update_queue.py | Fixes in-place mutation during spend update aggregation by storing a shallow copy of the first update per entity key before summing response_cost. |
| tests/test_litellm/proxy/db/db_transaction_queue/test_spend_update_queue.py | Adds regression test ensuring _get_aggregated_spend_update_queue_item does not mutate caller-owned updates; assertions avoid order-dependence by selecting aggregated entry by key. |
Sequence Diagram
sequenceDiagram
participant Caller as Proxy code (caller)
participant SUQ as SpendUpdateQueue
participant AQ as asyncio.Queue
Caller->>SUQ: add_update(update)
SUQ->>AQ: put(update)
alt queue size >= MAX_SIZE_IN_MEMORY_QUEUE
SUQ->>SUQ: aggregate_queue_updates()
SUQ->>SUQ: flush_all_updates_from_in_memory_queue()
SUQ->>SUQ: _get_aggregated_spend_update_queue_item(updates)
note over SUQ: For each entity key:
note over SUQ: store update.copy() for first seen
note over SUQ: then sum response_cost into copy
SUQ-->>SUQ: aggregated_updates
loop each aggregated update
SUQ->>AQ: put(aggregated_update)
end
end
Caller->>SUQ: flush_and_get_aggregated_db_spend_update_transactions()
SUQ->>SUQ: flush_all_updates_from_in_memory_queue()
SUQ->>SUQ: get_aggregated_db_spend_update_transactions(updates)
SUQ-->>Caller: DBSpendUpdateTransactions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2 files reviewed, no comments
Summary
This PR fixes in-place mutation in
SpendUpdateQueue._get_aggregated_spend_update_queue_item().Previously, aggregation stored a direct reference to the first
SpendUpdateQueueItemseen for a key and then mutated that dict when combining costs. If any caller retained a reference to the original update object, itsresponse_costwould be overwritten during queue aggregation.Changes
litellm/proxy/db/db_transaction_queue/spend_update_queue.py:update.copy()) before storing it in the aggregation map.tests/test_litellm/proxy/db/db_transaction_queue/test_spend_update_queue.py:test_get_aggregated_spend_update_queue_item_does_not_mutate_original_updates.10.0)30.0)Validation
poetry run pytest -q tests/test_litellm/proxy/db/db_transaction_queue/test_spend_update_queue.py11 passedCloses #20875