fix(session_persistence): count persisted items after filtering unpersistable conversation items#3580
fix(session_persistence): count persisted items after filtering unpersistable conversation items#3580mshsheikh wants to merge 4 commits into
Conversation
…sistable conversation items ## Summary Fix an accounting mismatch in `save_result_to_session` for OpenAI Conversations sessions. The function currently calculates `saved_run_items_count` before filtering out items that cannot be persisted to the conversations store, especially reasoning items without a valid `id` or `encrypted_content`. As a result, the run state may advance as though items were saved even when they were not actually written. ## Root cause For OpenAI Conversations sessions: 1. `items_to_save` is built from the deduplicated input plus new run items. 2. `saved_run_items_count` is computed from that unfiltered collection. 3. Unpersistable items are removed afterward. That ordering causes the persisted-item count to drift from the actual items stored in the session. ## Fix Move the `_is_unpersistable_for_openai_conversation(...)` filtering step before: - serialization - fingerprint counting - `saved_run_items_count` calculation This ensures the persisted count reflects only the items that are truly eligible to be saved. ## Expected behavior after the fix - Items that cannot be persisted are excluded before accounting. - `run_state._current_turn_persisted_item_count` stays aligned with actual session writes. - Retry and resume logic no longer assumes an item was persisted when it was filtered out. ## Validation Recommended test coverage: - A conversation session containing an unpersistable reasoning item. - Verify that: - the item is excluded from `items_to_save` - `saved_run_items_count` does not include it - `run_state._current_turn_persisted_item_count` matches the actual persisted items ## Notes This is a logic-ordering fix only. It does not change persistence schema, item normalization, or conversation protocol behavior.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 51e42efd58
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| items_to_save = [ | ||
| item for item in items_to_save if not _is_unpersistable_for_openai_conversation(item) | ||
| ] |
There was a problem hiding this comment.
For OpenAIConversationsSession turns where an unpersistable reasoning item precedes a persistable item, this filtering makes saved_run_items_count exclude the reasoning item even though _current_turn_persisted_item_count is later used as a slice index into the original, unfiltered new_items list (new_items[already_persisted:]). For example, after saving [bad reasoning, assistant message], the counter becomes 1; a retry/resume starts at raw index 1 and saves the already-persisted assistant message again, causing duplicate session items. The accounting needs either a separate raw-item offset or filtering before the slice/counting logic so retries skip the same items that were already considered.
Useful? React with 👍 / 👎.
…d count ## Summary Fix a retry-offset accounting mismatch in `save_result_to_session` for `OpenAIConversationsSession`. The function now distinguishes between: - the raw number of turn items consumed for retry alignment, and - the number of items actually persisted to the session store. This prevents retries from re-slicing the raw turn stream at the wrong index when some items are filtered out as unpersistable. ## Root cause `run_state._current_turn_persisted_item_count` was being updated from `saved_run_items_count`, which reflects only items actually persisted. For OpenAI Conversations sessions, some items can be counted for retry progress but still be filtered out before persistence. In that case, using the persisted count as the retry cursor causes the next retry to start at the wrong raw index and can duplicate already-processed items. ## Fix Introduce a separate raw-turn count derived from `new_run_items` after retry-specific merging, and use that value to advance `run_state._current_turn_persisted_item_count`. Keep `saved_run_items_count` as the persisted-item return value, so persistence accounting remains accurate. ## Expected behavior after the fix - Retry alignment tracks how far the raw turn stream advanced. - Persisted-item counts still reflect only items that were actually saved. - A retry after a leading unpersistable reasoning item no longer re-saves the following persistable item. ## Validation Recommended regression coverage: - `new_items = [unpersistable reasoning item, assistant message]` - first save persists only the assistant message - second save with the same raw `new_items` does not duplicate the assistant message - `run_state._current_turn_persisted_item_count` advances by the raw consumed count, not just the persisted count ## Notes This is an accounting and retry-cursor fix only. It does not change session schema, item serialization, or conversation protocol behavior.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cc47d8d4ff
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| run_state._current_turn_persisted_item_count = ( | ||
| already_persisted + saved_run_items_count | ||
| ) |
There was a problem hiding this comment.
When an OpenAI Conversations turn has an unpersistable reasoning item before a persistable item, saved_run_items_count now excludes the reasoning item after filtering, but _current_turn_persisted_item_count is still used as a raw slice index into new_items on the next call; a retry/resume then starts at the already-saved persistable item and writes it again. The fresh evidence in this revision is that new_run_items_raw_count was added to represent the consumed raw offset but is never used for this state update.
Useful? React with 👍 / 👎.
…state ## Summary Fix a retry-offset accounting mismatch in `save_result_to_session` for `OpenAIConversationsSession`. The function now distinguishes between: - the raw number of turn items consumed for retry alignment, and - the number of items actually persisted to the session store. This prevents retries from re-slicing the raw turn stream at the wrong index when some items are filtered out as unpersistable. ## Root cause `run_state._current_turn_persisted_item_count` was being updated from `saved_run_items_count`, which reflects only items actually persisted. For OpenAI Conversations sessions, some items can be counted for retry progress but still be filtered out before persistence. In that case, using the persisted count as the retry cursor causes the next retry to start at the wrong raw index and can duplicate already-processed items. ## Fix Introduce a separate raw-turn count derived from `new_run_items` after retry-specific merging, and use that value to advance `run_state._current_turn_persisted_item_count`. Keep `saved_run_items_count` as the persisted-item return value, so persistence accounting remains accurate. ## Expected behavior after the fix - Retry alignment tracks how far the raw turn stream advanced. - Persisted-item counts still reflect only items that were actually saved. - A retry after a leading unpersistable reasoning item no longer re-saves the following persistable item. ## Validation Recommended regression coverage: - `new_items = [unpersistable reasoning item, assistant message]` - first save persists only the assistant message - second save with the same raw `new_items` does not duplicate the assistant message - `run_state._current_turn_persisted_item_count` advances by the raw consumed count, not just the persisted count ## Notes This is an accounting and retry-cursor fix only. It does not change session schema, item serialization, or conversation protocol behavior.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3e20b7e738
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if is_openai_conversation_session: | ||
| items_to_save = [ | ||
| item for item in items_to_save if not _is_unpersistable_for_openai_conversation(item) | ||
| ] |
There was a problem hiding this comment.
In the resumed-run path I checked (src/agents/run.py lines 1339-1351), this helper is called with run_state=None and the caller advances _current_turn_persisted_item_count by the returned value. After filtering unpersistable OpenAI Conversations reasoning items here, a resumed turn like [reasoning without id/encrypted_content, assistant message] returns 1 even though two raw run items were consumed, so the next retry slices at the already-saved assistant message and persists it again. The in-function state update now uses the raw count, but the return value still needs the same raw-offset semantics for callers that update state themselves.
Useful? React with 👍 / 👎.
…sation saves ## Summary Fix a retry-offset mismatch in `save_result_to_session` for resumed OpenAI Conversations saves. For resumed saves, the helper is called with `run_state=None`, and the caller advances its retry cursor using the returned value. In that path, returning only `saved_run_items_count` can undercount raw items when an unpersistable reasoning item is filtered out before persistence. ## Root cause The function now tracks: - `saved_run_items_count`: how many items were actually persisted - `new_run_items_raw_count`: how many raw turn items were consumed for retry alignment That split is correct internally, but resumed callers still need the raw offset when they advance retry state themselves. ## Fix Introduce a `returned_count` value and use raw-offset semantics for resumed OpenAI Conversations saves: - `run_state is None` and `is_openai_conversation_session` -> return `new_run_items_raw_count` - otherwise -> return `saved_run_items_count` Keep the in-function state update on the raw count as well. ## Expected behavior after the fix - Internal state advancement stays aligned with the raw turn stream. - Resumed saves no longer re-slice at an already-consumed assistant item. - Persisted-item accounting remains unchanged. ## Validation Recommended regression coverage: - `new_items = [unpersistable reasoning item, assistant message]` - resumed save returns the raw consumed count - next retry does not re-save the assistant message - persisted count remains accurate for the items actually written ## Notes This is a retry-accounting fix only. It does not change schema, serialization, or conversation protocol behavior.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5f050e4ded
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| run_state._current_turn_persisted_item_count = ( | ||
| already_persisted + new_run_items_raw_count | ||
| ) |
There was a problem hiding this comment.
For non-OpenAI sessions, a ToolApprovalItem is skipped by run_item_to_input_item, so items_to_save can be empty even though new_run_items_raw_count is nonzero. This now records the raw item as persisted, leaving _current_turn_persisted_item_count > 0 after nothing was written; later same-turn persistence paths use that counter to skip or slice items, so a retry/resume after an approval-only interruption can miss subsequently persistable output. The raw-offset accounting should be limited to the OpenAI Conversations filtering case, while ordinary sessions keep the actual saved count.
Useful? React with 👍 / 👎.
2 participants
Summary
Fix an accounting mismatch in
save_result_to_sessionfor OpenAI Conversations sessions.The function currently calculates
saved_run_items_countbefore filtering out items that cannot be persisted to the conversations store, especially reasoning items without a valididorencrypted_content. As a result, the run state may advance as though items were saved even when they were not actually written.Root cause
For OpenAI Conversations sessions:
items_to_saveis built from the deduplicated input plus new run items.saved_run_items_countis computed from that unfiltered collection.That ordering causes the persisted-item count to drift from the actual items stored in the session.
Fix
Move the
_is_unpersistable_for_openai_conversation(...)filtering step before:saved_run_items_countcalculationThis ensures the persisted count reflects only the items that are truly eligible to be saved.
Expected behavior after the fix
run_state._current_turn_persisted_item_countstays aligned with actual session writes.Validation
Recommended test coverage:
items_to_savesaved_run_items_countdoes not include itrun_state._current_turn_persisted_item_countmatches the actual persisted itemsNotes
This is a logic-ordering fix only. It does not change persistence schema, item normalization, or conversation protocol behavior.