Repository navigation
Python: fix(redis): keep core message groups atomic when trimming history - #9890
Yufeng He (he-yufeng) wants to merge 9 commits into
Conversation
`save_messages` enforced `max_messages` with `LTRIM key -max_messages -1`, cutting at a raw count boundary. That can drop an assistant message carrying a `function_call` while keeping the following `tool` message with its `function_result`, so the stored history starts with a dangling tool result. The next turn is rejected by the model (`role 'tool'` without a preceding `tool_calls`, or a function-call output without its call), and because the failed turn is never persisted the session keeps failing on every later message. Compute the trim start from the post-append history instead and advance past any leading orphaned tool output (a `*_result` or `function_approval_response` with no matching call/request in the same message), so the retained window is always model-valid. A pause-and-continue style tool pair whose call is still in the window is left intact. Regression test covers the reported case: a `function_call` then `function_result` then a plain message under a limit of two keeps only the plain message, never an orphaned result.
GitHub Actions
Inactive
GitHub Actions
Inactive
agent-framework-automation
Bot
added
the
python
github-actions
Bot
changed the title
There was a problem hiding this comment.
🟡 Changes recommended
Concurrent saves can delete the entire session history through non-atomic trimming, while boundary checks can retain unmatched function results that break replay.
3 open issues
What changed in this PR
This PR addresses #9851 by making Redis history trimming tool-aware, aiming to prevent retained orphaned results from breaking subsequent model requests.
Changes:
- Adds helpers to identify leading tool outputs and calculate a retention boundary.
- Replaces count-only trimming with a boundary derived from existing and newly appended messages.
- Adds an orphaned-result regression test and updates the plain-message trimming test.
| File | Description |
|---|---|
python/packages/redis/tests/test_providers.py |
Tests orphaned-result trimming and updates plain-message retention expectations. |
python/packages/redis/agent_framework_redis/_history_provider.py |
Adds tool-output detection and applies the calculated trim boundary when saving history. |
Review effort: Balanced
| full_messages: list[Message] = list(existing_messages) + list(new_messages) | ||
| if len(full_messages) > self.max_messages: | ||
| start = _trim_start_index(full_messages, self.max_messages) | ||
| await _redis_result(self._redis_client.ltrim(key, start, -1)) |
There was a problem hiding this comment.
{"body": "The read, the append and the trim are one WATCHed transaction already, and 3b4452713 finishes the picture. save_messages watches the key, reads the list through the pipeline's own reserved connection, computes the boundary against that snapshot, then MULTI, RPUSH, LTRIM, EXEC. A key that changes between WATCH and EXEC aborts the transaction and the loop recomputes the boundary against the new state, up to eight attempts. So the absolute index is never derived from one list and applied to another.\n\nThe part that was actually broken is the exit: falling off the retry loop returned normally while every transaction had been aborted and nothing was persisted. The last WatchError is raised now, which is covered by test_save_raises_after_the_watch_retries_are_exhausted. test_save_reads_the_watched_history_through_the_pipeline_connection pins that the snapshot read uses the watched connection rather than a second one out through the parent client, which is what makes the WATCH meaningful on a borrowed client.\n"}
| has_output = any(_is_deferred_output_content(content.type) for content in contents) | ||
| has_source = any(_is_source_content(content.type) for content in contents) | ||
| return has_output and not has_source |
There was a problem hiding this comment.
{"body": "Agreed, and that is what the pairing does now. _pair_outputs_to_sources walks the whole transcript and joins each deferred output to its own source by call_id, or by the approval wrapper's id and its embedded function call, in occurrence order. _window_is_self_contained then asks whether each kept output's own source is kept, not whether the window contains some source somewhere.\n\nYour example is the case that made this concrete. With [function_result(c1), function_call(c2)] in one assistant message and call(c1) trimmed, the result pairs with call(c1) at its earlier position and the window is rejected, even though call(c2) is right there beside it. test_trim_pairs_call_and_result_by_occurrence_when_an_id_is_reused pins the same thing for a provider that reuses a call_id, where a later call would otherwise legitimize an earlier result.\n"}
| while start < count and _begins_orphaned_output(messages[start]): | ||
| start += 1 |
There was a problem hiding this comment.
{"body": "You were right that the boundary could stop early and split a group, and chasing this case turned up two more problems in the same check. All three are fixed in 3b4452713.\n\nNon-adjacent pairs now count as you describe. The boundary keeps advancing while any kept output has its own source outside the window, so it never stops at a plain message in the middle of a group. For your [call(c1), assistant('approval completed'), result(c1), assistant('done')] at max_messages=3 the retained window is [done], which is the only cut that keeps at most three messages and splits nothing. Dropping the dangling result is cheaper than handing the model a history it rejects on every later turn. test_trim_does_not_split_a_group_separated_by_a_plain_message pins it.\n\nThe check then decides which positions are kept by walking a range sized from the number of matched outputs. Sources with no result of their own are not in that count, so a transcript with more sources than outputs under-counted the kept positions and reported a self-contained window as broken: four unanswered calls ahead of one intact pair walked the boundary three messages further than needed. It is now the plain question of whether each kept output's source sits at or after the window start. test_trim_keeps_sources_that_have_no_output_of_their_own pins that.\n\nThe other half of the same check was worse. A pair the transcript cannot resolve has no source anywhere to keep, and treating that as a broken dependency advanced the boundary to the end of the list and LTRIM deleted the session. The trim is responsible for not creating orphans, not for repairing history damage it has no cause for, so an unresolvable pair no longer stops a cut. test_trim_does_not_empty_history_over_an_output_with_no_source pins the difference.\n\n339 passed, 58 skipped across the redis package after these, and pyright is clean on the changed module.\n"}
Address review feedback on the tool-aware trim: - Match each deferred output to its own preceding source by call id instead of accepting "some source is present". A message holding `[result(c1), call(c2)]` is not safe when c1's call was trimmed. - Advance the cut while the retained window still holds an output whose source lies outside it, so a non-adjacent call/result group is never split. - Guard read, append, and trim with WATCH and retries so a concurrent save to the same key cannot trim from a stale absolute index and delete the whole session history. Recompute the boundary when the key changed under us.
GitHub Actions
Inactive
|
Thanks for the precise scenarios. All three are valid and I have pushed a revision (03307dd) that addresses them. On the concurrent-save race: you are right that deriving the trim from a snapshot then calling On matching each result to its own call: the boundary check no longer accepts "some source is present". It collects the call ids of the sources in the window and requires every deferred output's own call id to be there, so On non-adjacent groups: the cut now advances while the retained window still holds an output whose source lies outside it, so a
|
…rim-orphaned-tool-result Signed-off-by: Yufeng He <40085740+he-yufeng@users.noreply.github.com>
GitHub Actions
Inactive
|
Merged current Fresh verification after the merge: The trimming contract is unchanged: a cut still keeps a tool call and its result in the same retained window, and the WATCH transaction still avoids trimming from a stale absolute index when another writer changes the list. |
…rim-orphaned-tool-result
GitHub Actions
Inactive
|
|
||
| def _content_call_id(content: Any) -> str | None: | ||
| """Return the call/request id a content pairs with, when it carries one.""" | ||
| call_id = getattr(content, "call_id", None) |
There was a problem hiding this comment.
Approval request and response contents do not carry the correlation ID in this top-level field. They pair through Content.id and the embedded function call, so this returns None for every approval wrapper and can make valid recent history look orphaned. Please add type-aware approval correlation by request identity and cover a retained approval request/response plus call/result sequence.
There was a problem hiding this comment.
{"body": "Fixed in ab94c48bf. The helper returned None for every approval wrapper because it only looked at call_id, and that made a valid retained approval sequence look orphaned and let the trim discard it.\n\nIt now falls back to the embedded function_call and then the content id. I checked the constructors rather than guessing: from_function_approval_request warns when its id differs from function_call.id and tells you to use the function call's id, so the wrapper's own id is the join key by convention, with the embedded call as the tie-breaker for the legacy binding.\n\nRegression test_trim_keeps_a_retained_approval_request_and_response_pair seeds a function_call plus its request and response at max_messages=2 and asserts nothing is trimmed.\n"}
| if _is_source_content(content.type): | ||
| call_id = _content_call_id(content) | ||
| if call_id is not None: | ||
| source_ids.add(call_id) |
There was a problem hiding this comment.
A set of provider call IDs loses logical occurrence order. If a provider reuses a call_id, a later call can incorrectly legitimize an earlier result after the earlier call was trimmed, leaving the retained window with an unmatched result. Please pair calls and results by occurrence in transcript order, using Content.id when available, and add a reused-ID regression.
There was a problem hiding this comment.
{"body": "Fixed in ab94c48bf, and this one took two passes.\n\nThe first attempt paired by occurrence within the candidate window, which still let a later call legitimize an earlier result once the earlier call had been trimmed: the window [result\u2082, call\u2082] looked self-contained because result\u2082 borrowed call\u2082. The test caught it, which is what it was for.\n\nPairing now runs over the whole transcript by occurrence, and the window check asks whether each kept output's own source is kept. A reused call_id can no longer borrow the wrong call.\n\nRegression test_trim_pairs_call_and_result_by_occurrence_when_an_id_is_reused seeds call\u2081, result\u2081, call\u2082 all sharing call_id=\"c1\" at max_messages=2 and asserts the trim advances past the orphaned result\u2081.\n"}
| for _ in range(_SAVE_WATCH_RETRIES): | ||
| async with self._redis_client.pipeline(transaction=True) as pipe: | ||
| await _redis_result(pipe.watch(key)) | ||
| existing_messages = await self.get_messages(session_id, state=state, **kwargs) |
There was a problem hiding this comment.
WATCH has already reserved the pipeline connection here, but get_messages() reads through the parent Redis client and needs a second connection. A supported borrowed client with max_connections=1 fails every non-empty save. Please read the watched history through the pipeline’s reserved connection before MULTI, with a single-connection regression.
There was a problem hiding this comment.
{"body": "Fixed in ab94c48bf. The read now goes through the pipeline's own reserved connection before MULTI, so a borrowed client with max_connections=1 no longer needs a second connection for a non-empty save.\n\nget_messages() still reads through the parent client, since it is the public path and has no reservation to respect. Only the save path's read moved.\n\nRegression test_save_reads_the_watched_history_through_the_pipeline_connection asserts the save calls lrange on the pipeline and never on the parent.\n"}
| await pipe.execute() | ||
| except WatchError: | ||
| # The key changed between WATCH and EXEC. Recompute against the new state. | ||
| continue |
There was a problem hiding this comment.
On the eighth WatchError, this continue exits the retry loop and save_messages() returns success even though every transaction was aborted and no message was persisted. Please raise the final persistence error after retry exhaustion and add an exhaustion-path regression.
There was a problem hiding this comment.
{"body": "Fixed in ab94c48bf. Falling off the retry loop returned normally while every transaction had been aborted and nothing was persisted, which is the worst of the four to ship. The last WatchError is now raised after exhaustion.\n\nRegression test_save_raises_after_the_watch_retries_are_exhausted makes execute raise WatchError every time and asserts both the raise and that all eight attempts ran.\n\n336 passed, 58 skipped across the redis package after these four; pyright clean on the changed module.\n"}
…, and stop reporting a failed save as success Four findings from review on the trim path this PR introduced. `save_messages` returned success after the watch retries were exhausted. Every transaction had been aborted and nothing was persisted, but falling off the loop returned normally. The last conflict is now raised instead. The watched history was read through the parent Redis client after `WATCH` had already reserved the pipeline connection, so a borrowed client with `max_connections=1` failed every non-empty save. The read now goes through the pipeline's own connection before `MULTI`. Approval request and response contents carry no `call_id`; they pair through `Content.id` and the embedded function call. The pairing helper returned `None` for every approval wrapper, which made a valid retained approval sequence look orphaned and let the trim discard it. The helper now falls back to the embedded function call and then the content id. And pairing by a set of provider call ids lost occurrence order, so when a provider reuses a `call_id` a later call legitimized an earlier result whose own call had been trimmed. Pairing now runs over the whole transcript by occurrence and the window check asks whether each kept output's own source is kept, so a reused id cannot borrow the wrong call. Regressions cover all four: a retained approval request/response pair survives the trim, a reused call_id does not let a later call legitimize an earlier result, the save reads the watched list through the pipeline and never through the parent client, and exhaustion raises. 336 passed, 58 skipped across the redis package. pyright is clean on the changed module. The core package tests still pass.
GitHub Actions
Inactive
… an unresolvable pair hold nothing back _window_is_self_contained decided which positions were kept by walking a range sized from the number of matched outputs. Sources that were never answered are outputs' counterparts only when they pair, so a transcript with more sources than results under-counted the kept positions and reported a self-contained window as broken. Four unanswered calls ahead of one intact pair pushed the boundary three messages further than it had to go. The check is now the plain question the docstring asks: for each output inside the window, is its own source inside too. A position is kept exactly when it sits at or after the window start, so no set has to be built at all. That fix also exposed the other half of the same check. A pair the transcript cannot resolve has no source anywhere to keep, and treating that as a broken dependency walked the boundary to the end of the list and LTRIM deleted the whole session. The trim is responsible for not creating orphans, not for repairing history damage it has no cause for, so an unresolvable pair no longer stops a cut. A regression pins the difference: a stray result keeps the newest three messages instead of wiping the list. Two regressions for the boundary, one for the case a review raised where a plain assistant message separates a call from its result and a cut landing between them would split the group, and one for the under-counting above. Both boundary cases fail before this change; the separated group is already correct and is pinned so it stays. This also wraps two over-long test lines and lets ruff format rewrap the raise in save_messages, both left over from the previous commit.
GitHub Actions
Inactive
…rim-orphaned-tool-result
GitHub Actions
Inactive
|
Yufeng He (@he-yufeng) Can we reuse the core compaction grouping here instead of maintaining Redis-specific dependency logic? |
GitHub Actions
Inactive
Yufeng He (he-yufeng)
changed the title
|
Eduard van Valkenburg (@eavanvalkenburg) Reworked in
I also moved approval request/response correlation into the shared grouping pass. Without that addition, reusing core spans would have regressed the earlier The new reasoning regression failed on the old implementation with On the merged head: core 9422 tests, Redis 347 tests, and the OpenAI chat client file 530 tests all pass. Ruff and formatting are clean. Pyright comparison against the pre-change source has zero new diagnostics; the existing environment/typeshed diagnostics are not being presented as a clean full-repository run. |
GitHub Actions
Inactive
Fixes #9851
What this PR does
RedisHistoryProvider.save_messagesenforcedmax_messageswith a rawLTRIM, which could leave a tool result or a reasoning-bound function call whose required prefix had already been removed. The next model request could then be rejected, and because that failed turn was never persisted, every later turn could fail on the same stored history.The trim boundary is now computed from the full post-append watched history using
agent_framework._compaction.group_messages. Retention advances to a stable complete-group boundary, covering reasoning prefixes, contiguous and non-contiguous tool results, overlapping linked groups, and approval sequences. Shallow copies keep the grouping helper's automatic message IDs from mutating caller-owned messages. Themax_messages=0andNonepaths, watched-connection reads, and explicit retry-exhaustion behavior are unchanged.The shared grouping now also pairs approval requests and responses through the embedded function-call identity, with the wrapper ID as a fallback. This preserves the earlier approval-retention fix without keeping a separate Redis-specific dependency algorithm.
Why
A conversation with a message limit should drop older complete groups while continuing to work. A raw count boundary can persist a history that OpenAI rejects during a later
store=Falsereplay when a call retains itshas_reasoningannotation but the corresponding reasoning item has been trimmed.The limit remains a bound: retention may drop an entire group that crosses the boundary or is larger than the limit, rather than retaining a partial group or exceeding
max_messages.Reviewer Test Plan
New regressions cover the reviewer-requested
[reasoning, call, result, done]case withmax_messages=3, overlapping non-contiguous groups, approval request/response retention, and a later OpenAIstore=Falserequest preparation after reloading the trimmed Redis history.Red-green evidence on the pre-fix implementation:
LTRIM(..., 3, -1)but gotLTRIM(..., 1, -1).ChatClientInvalidRequestExceptionfor the missing reasoning item. It passes with the complete-group trim.LTRIM(..., 2, -1)but gotLTRIM(..., 1, -1). Both pass with shared approval pairing.Final verification on merge head
9c645f92b8e024df38b4e27e21c2447546a512dc:packages/core/tests, non-integration: 9422 tests, 0 failures, 23 skipped.packages/redis/tests, non-integration: 347 tests, 0 failures, 0 skipped.packages/openai/tests/openai/test_openai_chat_client.py: 530 tests, 0 failures, 0 skipped.git diff --check: passed.Tested on macOS with Python 3.14. Redis operations use the provider's mocked client, while the stateless replay regression uses the real OpenAI request-preparation implementation without making a network call. No live Redis or OpenAI integration run is claimed in this revision.
Risk & Scope
The behavioral change is limited to Redis physical history retention and shared compaction grouping for approval pairs. No public API changes.
Linked Issues
Fixes #9851
AI Assistance
AI assistance details: root cause analysis, implementation, regression tests, and this PR description. The author reviewed the final diff, ran the core, Redis, and OpenAI tests, and compared Ruff and Pyright results against the pre-change source.