fix: retry resumed Session writes before model calls - #4621
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 112112da20
ℹ️ 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 isinstance(turn_result.next_step, NextStepHandoff): | ||
| # Advance the durable continuation before persisting the resumed outputs. If the | ||
| # atomic Session append fails, the RunState can retry that exact batch and then enter | ||
| # the target agent without re-running the approved tool or losing the handoff. | ||
| run_state._current_agent = turn_result.next_step.new_agent | ||
| run_state._current_step = NextStepRunAgain() |
There was a problem hiding this comment.
Preserve the handoff event across a failed session append
When a resumed streamed turn produces a handoff and the atomic session append fails, this advances _current_agent to the target and replaces the handoff step with NextStepRunAgain before _save_resumed_items() raises. The normal AgentUpdatedStreamEvent is queued only after that save, while the retry path merely flushes the pending items and starts the target agent, so consumers of the failed stream plus its resumed stream never receive the agent-transition event. Preserve enough pending-handoff state to publish the event after a successful retry (without rerunning the handoff).
AGENTS.md reference: AGENTS.md:L147-L150
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Thanks for flagging this. I reproduced the failed-stream + resumed-stream sequence at the public Runner boundary. Every run_streamed() invocation queues its current agent at startup (run_loop.py, before resume processing). After the failed append, the retained RunState already owns the handoff target, and RunResultStreaming.to_state() preserves that target. The retry stream therefore emits AgentUpdatedStreamEvent(target) as its first agent event, then flushes the pending Session batch before any target-model call. The observed sequence was failed stream: agent:source; resumed stream: agent:target, followed by target events, with the approved tool executed once. Persisting another pending handoff event would emit the same target update twice on recovery, so I am leaving the current behavior unchanged.
|
One contract boundary seems worth making explicit in the PR/tests/docs so this recovery mechanism is not accidentally read as a general exactly-once guarantee. This PR closes a very useful window: Within the same retained/serialized There is still an unavoidable earlier crash window, though: After that crash, the SDK cannot in general know whether the external effect happened. Replaying from an older durable state may execute it again unless the tool itself has an idempotency key / transactional protocol with the external system. So I would phrase the invariant narrowly:
rather than anything resembling A small failure-boundary matrix in the regression suite or docs would make the distinction durable: The conceptual split is useful for production users: effect commit != SDK state commit != Session evidence commit. This PR can make the latter two consistent once the completed-effect state exists, without claiming a distributed transaction with the external tool target. |
|
Thanks for sending this patch. The issue was resolved by #4630 |
|
Thanks for the update and for landing the fix. I’ll review #4630 to better understand the narrower recovery boundary and the intended scope for Session write recovery. |
Summary
This pull request fixes resumed approval flows that could execute a local tool successfully, fail the subsequent atomic
Session.add_items()call, and then continue without persisting the missing tool evidence.It records the exact pending resumed-turn batch, owning Session ID, effective
storesetting, persisted frontier, and durable continuation inRunStatebefore the append. Retries must use the same Session and flush that batch before any source or handoff-target model call, without executing the approved tool again. Append acknowledgement clears retry authority before later compaction work, and the same behavior is preserved across streaming/non-streaming runners, failed-resultto_state()conversion, terminal tool outputs, blocked output guardrails, handoffs, and schema-1.17 JSON round trips.Test plan
tests/test_run_impl_resume_paths.py(32 passed)..agents/skills/code-change-verification/scripts/run.sh; format, lint, mypy, pyright, and the full test suite all passed.Issue number
Closes #4615
Checks
.agents/skills/code-change-verification/scripts/run.sh/reviewbefore submitting this PR