Repository navigation
fix(sessions): recover resumed handoffs after session append failures - #4725
Merged
Merged
Conversation
Co-authored-by: rajarshidattapy <rayan05rio@gmail.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
seratch
changed the title
This was referenced Aug 28, 2026
2 of 4 tasks
3 of 4 tasks
mittalpk
added a commit
to mittalpk/openai-agents-python
that referenced
this pull request
Sep 2, 2026
…ent-update event Two issues from the automated Codex review on this PR, both real: 1. _save_stream_items_without_count() never registered a pending_session_write checkpoint for the handoff batch, unlike the sibling is_resumed_state branch's _save_resumed_stream_items(). A failed append followed by a successful resume invoked the correct (delegate) agent but permanently dropped the handoff's function_call/function_call_output pair from session history, since nothing recorded the batch for the existing resume_pending_session_write() recovery path to replay. Fixed by threading resumed_write_state through _save_stream_items into save_result_to_session, gated on the handoff branch already having set _current_step to NextStepRunAgain. 2. AgentUpdatedStreamEvent was still queued after the fallible session append, so a live stream_events() consumer would see handoff items followed directly by an error with no semantic agent-transition event, even though the result and resumed run both correctly identify the new agent. Moved the event queue call to sit with the other state-transition updates, before the append. Both fixes are scoped to only the generic-loop branch this PR already touches; the already-merged is_resumed_state branch (openai#4725) has the same pre-existing event-ordering gap but is out of scope here. Extended test_fresh_streamed_handoff_preserves_agent_after_session_append_failure with a session-history assertion for issue 1, and added test_fresh_streamed_handoff_publishes_agent_update_before_session_append_failure for issue 2 (using a new session double that yields before failing, since a purely synchronous raise never gives stream_events() a scheduling boundary to prove event delivery either way).
jbeckwith-oai
added a commit
that referenced
this pull request
Sep 27, 2026
…ailures (#4835) * fix(sessions): recover fresh streamed handoffs after session append failures start_streaming()'s generic-loop NextStepHandoff branch awaited the fallible session append (_save_stream_items_without_count) before updating current_agent, run_state._current_agent, streamed_result.current_agent, and run_state._current_step. If that append raised (e.g. a transient session backend error), the run failed with those fields still pointing at the pre-handoff agent, even though the handoff had already fully executed. Resuming from result.to_state() after such a failure then re-invoked the wrong agent with input that already contained its own handoff call/output. This is the same defect PR #4725 fixed in the sibling is_resumed_state branch (used when resuming an interrupted run) by moving the state updates ahead of the fallible save. This applies the same reordering to the generic branch, which every fresh streamed run's handoffs go through, not just resumed ones. * address Codex review: checkpoint the handoff batch and reorder the agent-update event Two issues from the automated Codex review on this PR, both real: 1. _save_stream_items_without_count() never registered a pending_session_write checkpoint for the handoff batch, unlike the sibling is_resumed_state branch's _save_resumed_stream_items(). A failed append followed by a successful resume invoked the correct (delegate) agent but permanently dropped the handoff's function_call/function_call_output pair from session history, since nothing recorded the batch for the existing resume_pending_session_write() recovery path to replay. Fixed by threading resumed_write_state through _save_stream_items into save_result_to_session, gated on the handoff branch already having set _current_step to NextStepRunAgain. 2. AgentUpdatedStreamEvent was still queued after the fallible session append, so a live stream_events() consumer would see handoff items followed directly by an error with no semantic agent-transition event, even though the result and resumed run both correctly identify the new agent. Moved the event queue call to sit with the other state-transition updates, before the append. Both fixes are scoped to only the generic-loop branch this PR already touches; the already-merged is_resumed_state branch (#4725) has the same pre-existing event-ordering gap but is out of scope here. Extended test_fresh_streamed_handoff_preserves_agent_after_session_append_failure with a session-history assertion for issue 1, and added test_fresh_streamed_handoff_publishes_agent_update_before_session_append_failure for issue 2 (using a new session double that yields before failing, since a purely synchronous raise never gives stream_events() a scheduling boundary to prove event delivery either way). * address 2nd Codex review round: guardrail race, event drain, deferred compaction Three more issues from the automated Codex review on commit 75fc64b, all verified with live reproduction before being fixed: 1. The handoff transition committed current_agent/run_state before a still-in-flight parallel input guardrail had resolved. A non-tripwire exception from that guardrail then left the resumable state pointing at the delegate agent, even though the starting agent's input guardrails never definitively cleared. Fixed by explicitly awaiting input_guardrail_tripwire_triggered_for_stream() as the first statement in the handoff branch, before any state mutation. 2. Queuing AgentUpdatedStreamEvent before the fallible session append doesn't guarantee delivery: stream_events() checks a stored exception before draining the queue, so a real (non-instant) consumer can lose an already-queued event to a task that raised without ever being marked for draining. Fixed by marking the session-persistence exception via _mark_error_to_drain_stream_events() before re-raising, the same pattern already used for model-behavior errors. 3. The pending_session_write checkpoint recovers the raw item append but never carried enough information (response_id, store, whether the batch had local tool outputs) for a later, separate resume to replay the same post-write Responses compaction decision save_result_to_session would have applied inline. Extracted the compaction decision into a shared _apply_post_write_compaction() helper, extended the checkpoint schema with those fields (optional, so an old-shaped serialized RunState still round-trips), and call the helper from resume_pending_session_write() once a checkpoint settles -- whether inline or on a separate resume -- instead of duplicating the call at both sites. Added 3 new regression tests to tests/test_run_impl_resume_paths.py (72 total in the file, up from 69), each confirmed to fail against the pre-fix code and pass after. Full verification stack clean: make format/ lint/typecheck, and the full suite (9372 passed, 33 skipped, 0 failed). * address 3rd Codex review round: retain checkpoint until compaction settles One more issue from the automated Codex review on commit 92891a6, verified with live reproduction before being fixed: resume_pending_session_write() cleared run_state._pending_session_write before calling the newly-added _apply_post_write_compaction(), so if that call raised or was cancelled, the checkpoint was already gone. A later retry would then have nothing to redo the compaction step with, silently and permanently losing the requested deferred/forced Responses compaction even though the append itself had already succeeded. Fixed by moving the compaction call inside the try block, before clearing the checkpoint. The append reconciliation above already makes a retry safe against duplicate appends (it detects an already-committed batch via digest matching and skips re-appending), so this only changes when the checkpoint is released, not the retry logic itself. Added test_fresh_streamed_handoff_retains_checkpoint_when_post_write_compaction_fails to tests/test_run_impl_resume_paths.py (73 total, up from 72), confirmed to fail against the pre-fix code (checkpoint cleared despite the compaction failure) and pass after. Full verification stack clean: make format/lint/typecheck, and the full suite (9373 passed, 33 skipped, 0 failed). * address human review: clear the deferred-compaction marker only after success One more issue, this time from a human reviewer (sylvesterkaczmarek) on commit 06e9e40, verified with live reproduction before being fixed: OpenAIResponsesCompactionSession.run_compaction() cleared self._deferred_response_id before calling the fallible client.responses.compact() API. If that call raised, the deferred marker was already gone. The checkpoint-recovery code added in the last two commits recomputes force=True purely from whether this marker is still set, so on retry it silently recomputed force=False and could skip compaction that was still owed -- even though the round-3 fix already let the checkpoint itself survive for a retry. Fixed by moving the clear to after compaction actually settles (after the API call and the underlying session replacement both succeed), not before attempting them. The digest-based retry-safety already added for the append doesn't need any changes; this only moves when one session-internal flag gets cleared. Added test_run_compaction_retains_deferred_marker_when_api_call_fails to tests/memory/test_openai_responses_compaction_session.py, confirmed to fail against the pre-fix code (assert None == 'resp-handoff') and pass after. Full verification stack clean: make format/lint/typecheck, and the full suite (9374 passed, 33 skipped, 0 failed). * fix(streaming): stop the handoff transition on a real input-guardrail tripwire The generic-loop handoff branch already awaited an in-flight parallel input guardrail before committing the transition, but discarded its boolean result. A guardrail that settled normally with tripwire_triggered=True (no exception) therefore still published the delegate and a resumable NextStepRunAgain -- the InputGuardrailTripwireTriggered exception only fired later, from the pre-existing end-of-run safety net, by which point the transition had already leaked to stream consumers and to current_agent. Capture the boolean and raise immediately when it's true, before any part of the transition (current_agent, run_state, published events, session save) commits -- matching the exception-raising branch this same block already had for a guardrail that raises outright. * fix(streaming): trim speculative handoff items on an input tripwire Not raising the handoff transition on a tripwire (1a24069) doesn't undo it: the turn's model response, generated items, and session items are accumulated into streamed_result (and run_state, when resuming) before the guardrail's boolean result is even inspected. Left in place, to_state() would still hand back a RunState carrying a completed handoff built on guardrail-rejected input -- and Runner.run() always treats a RunState input as an already-resumed run (a plain isinstance check), so resuming it would skip the starting agent's own input guardrails entirely and continue under the delegate. Trims every affected owner back to its pre-turn length right before raising InputGuardrailTripwireTriggered, reusing the same _BlockedOutputOwnerStarts snapshot the handoff branch already takes for blocked-output handling rather than adding a second mechanism. New test confirms a tripwired turn 0 leaves state._generated_items, _session_items, and _model_responses empty via to_state() -- fails on unpatched code (asserts the handoff pair is present) and passes after. --------- Co-authored-by: Justin Beckwith <jbeckwith@openai.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request supersedes #4689 and fixes a recovery gap when an approved local function tool and a queued handoff complete during resume but the client-managed Session append then fails.
It checkpoints the committed handoff target and completed tool guardrail state before the fallible append. A retry from a live or JSON-restored
RunStaterepairs the exact pending output batch before another model call without rerunning the tool body, guardrails, tool hooks, or handoff callback. Streaming and non-streaming paths retain the existing failure semantics and serialized schema.This pull request resolves #4685.