fix(realtime): restore agent after update failure - #4652
Conversation
![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: ea26f7e986
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
seratch
left a comment
seratch
left a comment
There was a problem hiding this comment.
Thanks for the contribution. The underlying state-desynchronization bug is real, but the current rollback is not safe for all supported async outcomes. CancelledError bypasses except Exception, and when two overlapping updates both fail, the newer call restores the older failed call's snapshot rather than the last successfully committed agent.
Please replace the identity-based rollback with a session-owned serialized update_agent() transaction that snapshots and restores only the last successfully committed agent and dispatch snapshot. On cancellation, the outbound send must be allowed to settle: keep the new state if it succeeds, restore the committed predecessor if it fails, and then propagate cancellation. Please add deterministic regressions for the two-failure case and cancellation whose send later succeeds or fails. The current green CI run does not exercise these interleavings.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 000eaf1b37
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| agent=agent, | ||
| ) | ||
| updated_snapshot = self._dispatch_snapshot_from_settings(agent, updated_settings) | ||
| async with self._update_agent_lock: |
There was a problem hiding this comment.
When async_tool_calls=False and a custom or ScriptedRealtimeModel emits a function call while processing this session-update send, a tool that invokes the public update_agent() API blocks on this lock. The original update is simultaneously awaiting send_event(), which cannot finish delivering the function call until that tool returns, so both tasks deadlock; serialize transitions without holding a non-reentrant lock across listener delivery.
AGENTS.md reference: AGENTS.md:L149-L149
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Thanks for the follow-up at exact head 000eaf1. I independently reproduced the requested two-failure and repeated-cancellation send-success/send-failure cases; those now settle correctly on Python 3.10 and 3.13. The full realtime session file is also green (203 tests on each version), along with Ruff, mypy, and Pyright for the changed files.
I reproduced this reentrancy issue with a cleanup-safe public-path probe: async_tool_calls=False, a ScriptedRealtimeModel emits a function call while handling the outer session-update send, and the tool awaits session.update_agent(nested). The outer update holds _update_agent_lock while send_event() awaits listener delivery, and the nested update waits on the same lock.
One caution from prototyping: a ContextVar logical owner is not sufficient by itself. A detached tool task inherits it and can bypass serialization, then overlap a third independent update after the outer transaction releases the lock. Making the nested send inline preserves Scripted transport reentrancy, but a cancelled nested public update can then return before its wire operation settles. Moving that send back to a shielded child task restores cancellation semantics but reintroduces the Scripted delivery-worker deadlock.
I suggest freezing the narrow protocol before adding more owner state: serialize independent outer updates; separate outbound wire commit/failure from synchronous listener completion (or explicitly register listener-originated transitions); shield and drain every outer or nested send before commit/rollback; and enqueue inherited background work as an independent transaction. The identity fence should remain while handoff still mutates the same state outside this protocol. I have deterministic probes for all three boundaries and can contribute them once the intended transport/session contract is agreed.
3 participants
Summary
This pull request fixes
RealtimeSession.update_agent()leaving its local agent and dispatch snapshot changed when the outbound session update fails. It restores the previous state only while the failed update still owns the current transition, so a newer concurrent update is never rolled back.Test plan
.agents/skills/code-change-verification/scripts/run.shaftermake sync; formatting, lint, type checking, and the full test suite passed.Issue number
None.
Checks
.agents/skills/code-change-verification/scripts/run.sh/reviewbefore submitting this PR