Repository navigation
Python: fix(redis): keep core message groups atomic when trimming history - #9890
Yufeng He (he-yufeng) wants to merge 12 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
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
…, 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
GitHub Actions
Active
GitHub Actions
Active
…rim-orphaned-tool-result
GitHub Actions
Active
|
Merged current main in The related compaction, Redis, and OpenAI chat-client suites are 1071 passed and 25 skipped on the merged head. Strict source Pyright, test Pyright, and the pre-commit hooks pass. The skips are the existing external Redis and OpenAI integration lanes; no live service run is claimed. |
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 pairs approval requests and responses through the embedded function-call identity, with the wrapper ID as a fallback. It also links a separately stored approval request back to its original function call without consuming that declaration before the later tool result is paired. Redis retains groups with unanswered approval requests across saves, including a pending group that predates the raw retention window, so a later approval response and tool result cannot be persisted without their declaration. Once the responses arrive, normal complete-group trimming resumes.
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. Approving a batch across several saves creates the same lifecycle problem: trimming the original call while another approval is still pending leaves the eventual result without its declaration.For completed groups, 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. A group with an unanswered approval request is the deliberate temporary exception because its declaration is still needed by a later save.
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, a separately stored approval request linked to its original call, a pending approval group that predates the raw window, and a three-save two-tool approval batch. The multi-save regression reloads the actual mocked Redis rows and checks both the retained history and a later OpenAIstore=Falserequest preparation.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.doneafter its call had been trimmed. The pending-group boundary regression returned trim index 3 instead of preserving the group at index 0. All three pass with the new declaration links and pending-group retention.Final verification on merge head
986e477ef50f6bea5b19a96906a733e4cf028e1e:packages/core/tests/core/test_compaction.py: 161 passed.packages/redis/tests: 349 passed, 58 skipped, 0 failures.packages/openai/tests/openai/test_openai_chat_client.py: 561 passed, 28 skipped, 0 failures.git diff --check: passed for core and Redis.Tested on macOS with Python 3.14. Redis operations use the provider's mocked client, while the stateless replay regressions use 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 type-checking results against the available environment and hosted checks.