Skip to content

fix: retry resumed Session writes before model calls - #4621

Closed
AAA-413 wants to merge 1 commit into
openai:mainfrom
AAA-413:fix/resumed-session-write-retry
Closed

fix: retry resumed Session writes before model calls#4621
AAA-413 wants to merge 1 commit into
openai:mainfrom
AAA-413:fix/resumed-session-write-retry

Conversation

@AAA-413

@AAA-413 AAA-413 commented Aug 24, 2026

Copy link
Copy Markdown

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 store setting, persisted frontier, and durable continuation in RunState before 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-result to_state() conversion, terminal tool outputs, blocked output guardrails, handoffs, and schema-1.17 JSON round trips.

Test plan

  • Added deterministic atomic-failure, repeated-failure, handoff, terminal, output-guardrail, post-append compaction, malformed-state, and cross-mode recovery coverage.
  • Ran tests/test_run_impl_resume_paths.py (32 passed).
  • Ran the RunState compatibility and owning runner suites (819 passed).
  • Ran .agents/skills/code-change-verification/scripts/run.sh; format, lint, mypy, pyright, and the full test suite all passed.
  • Completed two independent final-review passes for persistence lifecycle and durable-schema/cross-mode parity; both returned clean.

Issue number

Closes #4615

Checks

  • I've added new tests, if relevant
  • I've run .agents/skills/code-change-verification/scripts/run.sh
  • I've confirmed all verification steps pass
  • If using Codex, I've run /review before submitting this PR

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment on lines +550 to +555
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()

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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 👍 / 👎.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

dgenio commented Aug 24, 2026

Copy link
Copy Markdown

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:

approved tool side effect completed
  -> pending Session batch retained in RunState
  -> Session append fails
  -> retry flushes evidence before any next model call

Within the same retained/serialized RunState, that gives the property we want: do not re-execute the side effect; reconcile its missing Session evidence first.

There is still an unavoidable earlier crash window, though:

tool side effect commits externally
  -> process dies before the updated RunState / pending batch is durably captured

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:

For a resumable RunState that already records a completed local-tool result and a pending Session batch, recovery MUST persist that evidence before the next model call and MUST NOT re-execute that tool.

rather than anything resembling exactly once for arbitrary tool side effects.

A small failure-boundary matrix in the regression suite or docs would make the distinction durable:

append fails after completed tool, state retained      -> reconcile, no re-execution
append ack lost but exact tail already committed       -> detect/reconcile if provable, no duplicate evidence
crash before completed-tool state is durably captured  -> outside SDK guarantee; tool idempotency required

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.

@seratch

seratch commented Aug 24, 2026

Copy link
Copy Markdown
Member

Thanks for sending this patch. The issue was resolved by #4630

@seratch seratch closed this Aug 24, 2026
@AAA-413

AAA-413 commented Aug 25, 2026

Copy link
Copy Markdown
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

RunState retry after an approved tool's Session write fails leaves an orphan tool call

3 participants